From a294658cfcd141c240e46ff8c79ee2fbfc41eb70 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Wed, 8 Jul 2026 17:26:28 +0100 Subject: [PATCH 1/4] fix(cli): remove doubled "Expected: Expected" prefix from Flag.choice/primitive errors Several `Primitive`s in effect@4.0.0-beta.93's `effect/unstable/cli` (`choice` -- used by `Flag.choice`/`Flag.choiceWithValue` on `--size`, `--dns-resolver`, `--output-format`, `--output`/`-o`, `--agent` -- plus the schema-backed `integer`, `float`, `boolean`, and `date`) fail with a raw message that already starts with the word "Expected", and `CliError.InvalidValue`'s own `message` getter independently prepends its own "Expected: " label on top of that, producing e.g.: Invalid value for flag --size: "nano". Expected: Expected "micro" | ... , got "nano" This is a bug in the vendored `effect` beta package itself, not this repo's code, so work around it in the shared CLI error-formatting layer (`subcommand-flag-suggestions.ts`) that already carries similar per-tag Go-parity rewrites for UnrecognizedOption/UnknownSubcommand: collapse the literal doubled substring rather than rebuilding the message template ourselves, so every other part of the message keeps tracking upstream's wording. Fixes CLI-1898 --- .../shared/cli/subcommand-flag-suggestions.ts | 28 +++++++ .../subcommand-flag-suggestions.unit.test.ts | 75 +++++++++++++++++++ .../shared/output/text-formatter.unit.test.ts | 18 +++++ 3 files changed, 121 insertions(+) diff --git a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts index 1cc5124f98..6f5202e6aa 100644 --- a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts +++ b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts @@ -199,6 +199,23 @@ function buildSubcommandFlagHint( }; } +// Workaround for a doubled "Expected: Expected ..." prefix in +// effect@4.0.0-beta.93's own primitive parsers. Several `Primitive`s under +// `effect/unstable/cli` (`choice` — used by `Flag.choice`/ +// `Flag.choiceWithValue` — plus the schema-backed `integer`, `float`, +// `boolean`, and `date`) fail with a raw message that already starts with +// the word "Expected" (e.g. `Expected "micro" | "small", got "nano"` or +// `Expected a valid date, got Invalid Date`), and `CliError.InvalidValue`'s +// own `message` getter independently prepends its own `"Expected: "` label +// on top of that — so any flag or argument backed by one of these +// primitives renders "Expected: Expected ...". Collapsing the literal +// doubled substring (rather than rebuilding the surrounding "Invalid value +// for ..." template ourselves) keeps this tied to the exact defect and +// covers every affected primitive uniformly, letting every other part of +// the message keep tracking upstream's own wording. Remove once upstream +// `effect` fixes this (see CLI-1898). +const DOUBLED_EXPECTED_PREFIX = "Expected: Expected "; + export function formatCliErrorsForDisplay( errors: ReadonlyArray, context?: CliErrorSuggestionContext, @@ -228,6 +245,17 @@ export function formatCliErrorsForDisplay( continue; } + if (error._tag === "InvalidValue" && error.message.includes(DOUBLED_EXPECTED_PREFIX)) { + changed = true; + formatted.push({ + _tag: error._tag, + message: error.message.replace(DOUBLED_EXPECTED_PREFIX, "Expected "), + source: error, + changed: true, + }); + continue; + } + formatted.push({ _tag: error._tag, message: error.message, source: error, changed: false }); } diff --git a/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts index 079256d354..8f6d8da8d2 100644 --- a/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts +++ b/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts @@ -122,4 +122,79 @@ describe("subcommand flag placement suggestions", () => { ); expect(errors.errors[0]?.message).not.toContain("--project-ref=jacraenyzrorgjhsdvvf "); }); + + it("collapses the doubled 'Expected: Expected' prefix for an invalid choice flag value", () => { + const errors = formatCliErrorsForDisplay([ + new CliError.InvalidValue({ + option: "size", + value: "nano", + expected: 'Expected "micro" | "small" | "medium", got "nano"', + kind: "flag", + }), + ]); + + expect(errors.changed).toBe(true); + expect(errors.errors).toHaveLength(1); + expect(errors.errors[0]?.message).toBe( + 'Invalid value for flag --size: "nano". Expected "micro" | "small" | "medium", got "nano"', + ); + expect(errors.errors[0]?.message).not.toMatch(/Expected:\s*Expected/); + }); + + it("collapses the doubled 'Expected: Expected' prefix for an invalid choice argument value", () => { + const errors = formatCliErrorsForDisplay([ + new CliError.InvalidValue({ + option: "level", + value: "bogus", + expected: 'Expected "debug" | "info", got "bogus"', + kind: "argument", + }), + ]); + + expect(errors.changed).toBe(true); + expect(errors.errors[0]?.message).toBe( + 'Invalid value for argument : "bogus". Expected "debug" | "info", got "bogus"', + ); + expect(errors.errors[0]?.message).not.toMatch(/Expected:\s*Expected/); + }); + + it("also collapses the doubled prefix for a non-choice primitive whose failure text starts with 'Expected' (e.g. an invalid integer flag value)", () => { + // Real failure text from effect@4.0.0-beta.93's schema-backed `Primitive.integer` + // (also affects `float`, `boolean`, and `date` — every primitive whose parse + // failure happens to start with the word "Expected" hits the same doubling). + const errors = formatCliErrorsForDisplay([ + new CliError.InvalidValue({ + option: "port", + value: "abc", + expected: 'Expected a string representing a finite number, got "abc"', + kind: "flag", + }), + ]); + + expect(errors.changed).toBe(true); + expect(errors.errors[0]?.message).toBe( + 'Invalid value for flag --port: "abc". Expected a string representing a finite number, got "abc"', + ); + expect(errors.errors[0]?.message).not.toMatch(/Expected:\s*Expected/); + }); + + it("leaves invalid-value errors whose expected text does not start with 'Expected' unchanged", () => { + // Real failure text from effect@4.0.0-beta.93's `Primitive.keyValuePair` — + // it never starts with the word "Expected", so it isn't doubled by + // `CliError.InvalidValue`'s own "Expected: " prefix and needs no rewriting. + const errors = formatCliErrorsForDisplay([ + new CliError.InvalidValue({ + option: "define", + value: "bogus", + expected: "Invalid key=value format. Expected format: key=value, got: bogus", + kind: "flag", + }), + ]); + + expect(errors.changed).toBe(false); + expect(errors.errors[0]?.changed).toBe(false); + expect(errors.errors[0]?.message).toBe( + 'Invalid value for flag --define: "bogus". Expected: Invalid key=value format. Expected format: key=value, got: bogus', + ); + }); }); diff --git a/apps/cli/src/shared/output/text-formatter.unit.test.ts b/apps/cli/src/shared/output/text-formatter.unit.test.ts index f49cf597ea..fff1a4a17b 100644 --- a/apps/cli/src/shared/output/text-formatter.unit.test.ts +++ b/apps/cli/src/shared/output/text-formatter.unit.test.ts @@ -53,4 +53,22 @@ describe("textCliOutputFormatter", () => { expect(text).toContain("Did you mean this?"); expect(text).toContain("--plan"); }); + + it("does not double the 'Expected' prefix for an invalid choice flag value", () => { + const formatter = textCliOutputFormatter(); + + const text = formatter.formatErrors([ + new CliError.InvalidValue({ + option: "size", + value: "nano", + expected: 'Expected "micro" | "small" | "medium", got "nano"', + kind: "flag", + }), + ]); + + expect(text).toContain( + 'Invalid value for flag --size: "nano". Expected "micro" | "small" | "medium", got "nano"', + ); + expect(text).not.toMatch(/Expected:\s*Expected/); + }); }); From e6be301da9292a5e393d6028858fb8a62190f359 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Wed, 8 Jul 2026 19:16:33 +0100 Subject: [PATCH 2/4] fix(cli): anchor doubled-Expected-prefix fix on error.expected, not error.message (review: #PRRT_kwDOErm0O86PVbq8) error.message interpolates the user-controlled error.value twice (once directly, once again inside error.expected's "got " suffix), so a message-wide, first-occurrence string replace could target a doubled "Expected: Expected " substring embedded in the value itself instead of the real doubled prefix from the primitive parser, silently mangling the user's input and leaving the actual bug unfixed. Detect and strip the doubled prefix from error.expected directly and rebuild the message from CliError.InvalidValue's own template instead. --- .../shared/cli/subcommand-flag-suggestions.ts | 27 ++++++++++++------- .../subcommand-flag-suggestions.unit.test.ts | 22 +++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts index 6f5202e6aa..52803208e0 100644 --- a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts +++ b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts @@ -208,13 +208,18 @@ function buildSubcommandFlagHint( // `Expected a valid date, got Invalid Date`), and `CliError.InvalidValue`'s // own `message` getter independently prepends its own `"Expected: "` label // on top of that — so any flag or argument backed by one of these -// primitives renders "Expected: Expected ...". Collapsing the literal -// doubled substring (rather than rebuilding the surrounding "Invalid value -// for ..." template ourselves) keeps this tied to the exact defect and -// covers every affected primitive uniformly, letting every other part of -// the message keep tracking upstream's own wording. Remove once upstream -// `effect` fixes this (see CLI-1898). -const DOUBLED_EXPECTED_PREFIX = "Expected: Expected "; +// primitives renders "Expected: Expected ...". Detect this from +// `error.expected` (the field the buggy primitives actually populate) +// rather than searching the fully composed `error.message`: `error.value` +// is user-controlled and interpolated into that same message (including a +// second time inside `expected` itself, via `choice`'s "got " +// suffix), so a message-wide, first-occurrence string replace can target +// the wrong spot if the value itself happens to contain the literal text +// "Expected: Expected ". Anchoring on `error.expected` and rebuilding the +// message from the same template `CliError.InvalidValue` uses avoids ever +// scanning `error.value`. Remove once upstream `effect` fixes this (see +// CLI-1898). +const EXPECTED_PREFIX = "Expected "; export function formatCliErrorsForDisplay( errors: ReadonlyArray, @@ -245,11 +250,15 @@ export function formatCliErrorsForDisplay( continue; } - if (error._tag === "InvalidValue" && error.message.includes(DOUBLED_EXPECTED_PREFIX)) { + if (error._tag === "InvalidValue" && error.expected.startsWith(EXPECTED_PREFIX)) { changed = true; + const message = + error.kind === "argument" + ? `Invalid value for argument <${error.option}>: "${error.value}". ${error.expected}` + : `Invalid value for flag --${error.option}: "${error.value}". ${error.expected}`; formatted.push({ _tag: error._tag, - message: error.message.replace(DOUBLED_EXPECTED_PREFIX, "Expected "), + message, source: error, changed: true, }); diff --git a/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts index 8f6d8da8d2..595b0969c6 100644 --- a/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts +++ b/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts @@ -197,4 +197,26 @@ describe("subcommand flag placement suggestions", () => { 'Invalid value for flag --define: "bogus". Expected: Invalid key=value format. Expected format: key=value, got: bogus', ); }); + + it("does not corrupt a value that itself contains the literal 'Expected: Expected' text", () => { + // Regression test: the fix must anchor on `error.expected` (the field the + // buggy primitive actually populates) rather than searching the fully + // composed `error.message`, since `error.value` is user-controlled and is + // interpolated into that same message twice (once directly, once again + // inside `expected`'s "got " suffix). A value that happens to + // contain the literal doubled-prefix text must be left untouched. + const errors = formatCliErrorsForDisplay([ + new CliError.InvalidValue({ + option: "env", + value: "Expected: Expected nano", + expected: 'Expected "dev" | "staging" | "prod", got "Expected: Expected nano"', + kind: "flag", + }), + ]); + + expect(errors.changed).toBe(true); + expect(errors.errors[0]?.message).toBe( + 'Invalid value for flag --env: "Expected: Expected nano". Expected "dev" | "staging" | "prod", got "Expected: Expected nano"', + ); + }); }); From 3d843e536a4ef236ce5c72aa968729f35c292e0b Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Wed, 8 Jul 2026 19:54:20 +0100 Subject: [PATCH 3/4] fix(cli): also collapse doubled Expected prefix for global setting flags (review: #PRRT_kwDOErm0O86PV7Le) GlobalFlag.setting flags (--output-format, and the legacy --output/-o, --dns-resolver, --agent) are validated by Command.runWith in a step that runs after the ShowHelp bundling, so a bad value never reaches CliOutput.Formatter/formatCliErrorsForDisplay -- it surfaces as a raw CliError.InvalidValue through runCli's catch-all instead, where normalize-error.ts read error.message directly and still showed the doubled "Expected: Expected" prefix. Extract the doubled-prefix workaround into a shared invalid-value-message.ts helper and apply it from normalize-error.ts's mappedError too, so both paths render the same way. Verified by tracing effect's Command.runWith (Setting flags parsed at step 7, outside the step-4 ShowHelp path) and reproducing directly: `--output-format bogus`, `--dns-resolver bogus`, `--agent bogus`, and legacy `-o bogus` all showed the doubled prefix before this change and a single "Expected" after. --- .../src/shared/cli/invalid-value-message.ts | 54 ++++++++++++++++++ .../shared/cli/subcommand-flag-suggestions.ts | 48 +++++----------- apps/cli/src/shared/output/normalize-error.ts | 26 +++++++++ .../output/normalize-error.unit.test.ts | 56 +++++++++++++++++++ 4 files changed, 149 insertions(+), 35 deletions(-) create mode 100644 apps/cli/src/shared/cli/invalid-value-message.ts diff --git a/apps/cli/src/shared/cli/invalid-value-message.ts b/apps/cli/src/shared/cli/invalid-value-message.ts new file mode 100644 index 0000000000..8bf079581d --- /dev/null +++ b/apps/cli/src/shared/cli/invalid-value-message.ts @@ -0,0 +1,54 @@ +// Workaround for a doubled "Expected: Expected ..." prefix in +// effect@4.0.0-beta.93's own primitive parsers. Several `Primitive`s under +// `effect/unstable/cli` (`choice` — used by `Flag.choice`/ +// `Flag.choiceWithValue` — plus the schema-backed `integer`, `float`, +// `boolean`, and `date`) fail with a raw message that already starts with +// the word "Expected" (e.g. `Expected "micro" | "small", got "nano"` or +// `Expected a valid date, got Invalid Date`), and `CliError.InvalidValue`'s +// own `message` getter independently prepends its own `"Expected: "` label +// on top of that — so any flag or argument backed by one of these +// primitives renders "Expected: Expected ...". Detect this from +// `error.expected` (the field the buggy primitives actually populate) +// rather than searching the fully composed `error.message`: `error.value` +// is user-controlled and interpolated into that same message (including a +// second time inside `expected` itself, via `choice`'s "got " +// suffix), so a message-wide, first-occurrence string replace can target +// the wrong spot if the value itself happens to contain the literal text +// "Expected: Expected ". Anchoring on `error.expected` and rebuilding the +// message from the same template `CliError.InvalidValue` uses avoids ever +// scanning `error.value`. Remove once upstream `effect` fixes this (see +// CLI-1898). +// +// Shared by two call sites that each see `InvalidValue` failures at a +// different point in `effect`'s CLI runtime: +// - `subcommand-flag-suggestions.ts` formats errors that reach the +// `CliOutput.Formatter` via the `ShowHelp` envelope — i.e. ordinary +// subcommand/argument flags, validated while `Command.runWith` parses the +// command tree. +// - `normalize-error.ts` formats errors from `GlobalFlag.setting` flags +// (`--output-format`, and the legacy `--output`/`-o`, `--dns-resolver`, +// `--agent`), which `Command.runWith` validates in a later step that runs +// *outside* the `ShowHelp` path and therefore never reaches the +// formatter — it surfaces as a raw failure through `runCli`'s catch-all +// instead. +const EXPECTED_PREFIX = "Expected "; + +export interface InvalidValueMessageFields { + readonly option: string; + readonly value: string; + readonly expected: string; + readonly kind: "flag" | "argument"; +} + +/** + * Rebuilds a `CliError.InvalidValue` message from its own template when + * `expected` carries the doubled "Expected" prefix. Returns `undefined` when + * `expected` is unaffected, so callers can fall back to the error's own + * untouched `message`. + */ +export function formatInvalidValueMessage(error: InvalidValueMessageFields): string | undefined { + if (!error.expected.startsWith(EXPECTED_PREFIX)) return undefined; + return error.kind === "argument" + ? `Invalid value for argument <${error.option}>: "${error.value}". ${error.expected}` + : `Invalid value for flag --${error.option}: "${error.value}". ${error.expected}`; +} diff --git a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts index 52803208e0..c6da39c2e1 100644 --- a/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts +++ b/apps/cli/src/shared/cli/subcommand-flag-suggestions.ts @@ -1,4 +1,5 @@ import type { CliError, Command, HelpDoc } from "effect/unstable/cli"; +import { formatInvalidValueMessage } from "./invalid-value-message.ts"; export interface CliErrorSuggestionContext { readonly rootCommand: Command.Command.Any; @@ -199,28 +200,6 @@ function buildSubcommandFlagHint( }; } -// Workaround for a doubled "Expected: Expected ..." prefix in -// effect@4.0.0-beta.93's own primitive parsers. Several `Primitive`s under -// `effect/unstable/cli` (`choice` — used by `Flag.choice`/ -// `Flag.choiceWithValue` — plus the schema-backed `integer`, `float`, -// `boolean`, and `date`) fail with a raw message that already starts with -// the word "Expected" (e.g. `Expected "micro" | "small", got "nano"` or -// `Expected a valid date, got Invalid Date`), and `CliError.InvalidValue`'s -// own `message` getter independently prepends its own `"Expected: "` label -// on top of that — so any flag or argument backed by one of these -// primitives renders "Expected: Expected ...". Detect this from -// `error.expected` (the field the buggy primitives actually populate) -// rather than searching the fully composed `error.message`: `error.value` -// is user-controlled and interpolated into that same message (including a -// second time inside `expected` itself, via `choice`'s "got " -// suffix), so a message-wide, first-occurrence string replace can target -// the wrong spot if the value itself happens to contain the literal text -// "Expected: Expected ". Anchoring on `error.expected` and rebuilding the -// message from the same template `CliError.InvalidValue` uses avoids ever -// scanning `error.value`. Remove once upstream `effect` fixes this (see -// CLI-1898). -const EXPECTED_PREFIX = "Expected "; - export function formatCliErrorsForDisplay( errors: ReadonlyArray, context?: CliErrorSuggestionContext, @@ -250,19 +229,18 @@ export function formatCliErrorsForDisplay( continue; } - if (error._tag === "InvalidValue" && error.expected.startsWith(EXPECTED_PREFIX)) { - changed = true; - const message = - error.kind === "argument" - ? `Invalid value for argument <${error.option}>: "${error.value}". ${error.expected}` - : `Invalid value for flag --${error.option}: "${error.value}". ${error.expected}`; - formatted.push({ - _tag: error._tag, - message, - source: error, - changed: true, - }); - continue; + if (error._tag === "InvalidValue") { + const message = formatInvalidValueMessage(error); + if (message !== undefined) { + changed = true; + formatted.push({ + _tag: error._tag, + message, + source: error, + changed: true, + }); + continue; + } } formatted.push({ _tag: error._tag, message: error.message, source: error, changed: false }); diff --git a/apps/cli/src/shared/output/normalize-error.ts b/apps/cli/src/shared/output/normalize-error.ts index a143b414b3..221451685c 100644 --- a/apps/cli/src/shared/output/normalize-error.ts +++ b/apps/cli/src/shared/output/normalize-error.ts @@ -1,4 +1,5 @@ import { Cause, Option } from "effect"; +import { formatInvalidValueMessage } from "../cli/invalid-value-message.ts"; type NormalizedCliError = { readonly code: string; @@ -75,6 +76,31 @@ const mappedError = (error: ErrorRecord): NormalizedCliError | undefined => { : "Error: required flag(s) not set", }; } + case "InvalidValue": { + // `CliError.InvalidValue` for a `GlobalFlag.setting` flag (e.g. + // `--output-format`, or the legacy `--output`/`-o`, `--dns-resolver`, + // `--agent`) never reaches `CliOutput.Formatter` — `Command.runWith` + // validates those flags in a step that runs outside the `ShowHelp` + // path, so the failure lands here instead. Apply the same + // doubled-"Expected"-prefix workaround `subcommand-flag-suggestions.ts` + // applies for the `ShowHelp`-formatted case (see CLI-1898), so every + // `InvalidValue` failure — whichever path it takes — renders the same + // way. + const option = readString(error, "option"); + const value = readString(error, "value"); + const expected = readString(error, "expected"); + const kind = readString(error, "kind"); + if ( + option !== undefined && + value !== undefined && + expected !== undefined && + (kind === "flag" || kind === "argument") + ) { + const message = formatInvalidValueMessage({ option, value, expected, kind }); + if (message !== undefined) return { code: tag, message }; + } + return undefined; + } case "ShowHelp": { // Effect CLI wraps parse errors in a ShowHelp envelope (`CliError.ts`) // whose `errors` array holds the underlying causes. If exactly one of diff --git a/apps/cli/src/shared/output/normalize-error.unit.test.ts b/apps/cli/src/shared/output/normalize-error.unit.test.ts index 1eba0f07f6..5e401270a4 100644 --- a/apps/cli/src/shared/output/normalize-error.unit.test.ts +++ b/apps/cli/src/shared/output/normalize-error.unit.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "vitest"; import { Cause } from "effect"; +import { CliError } from "effect/unstable/cli"; import { formatCliError, normalizeCause, normalizeCliError } from "./normalize-error.ts"; describe("normalizeCliError", () => { @@ -50,6 +51,61 @@ describe("normalizeCliError", () => { }); }); + test("InvalidValue collapses the doubled 'Expected: Expected' prefix (e.g. a bad GlobalFlag.setting value)", () => { + // Regression test for CLI-1898: `--output-format`/`--dns-resolver`/`--agent`/ + // legacy `--output` are `GlobalFlag.setting` flags backed by `Flag.choice`. + // `Command.runWith` validates their values in a step that runs outside the + // `ShowHelp` path, so a bad value never reaches `CliOutput.Formatter` (and + // `subcommand-flag-suggestions.ts`'s fix) — it surfaces here instead. + const error = new CliError.InvalidValue({ + option: "output-format", + value: "bogus", + expected: 'Expected "text" | "json" | "stream-json", got "bogus"', + kind: "flag", + }); + + expect(normalizeCliError(error)).toEqual({ + code: "InvalidValue", + message: + 'Invalid value for flag --output-format: "bogus". Expected "text" | "json" | "stream-json", got "bogus"', + }); + }); + + test("InvalidValue leaves an already-clean 'expected' message untouched", () => { + const error = new CliError.InvalidValue({ + option: "define", + value: "bogus", + expected: "Invalid key=value format. Expected format: key=value, got: bogus", + kind: "flag", + }); + + expect(normalizeCliError(error)).toEqual({ + code: "InvalidValue", + message: + 'Invalid value for flag --define: "bogus". Expected: Invalid key=value format. Expected format: key=value, got: bogus', + }); + }); + + test("ShowHelp envelope unwraps a single InvalidValue with the same doubled-prefix fix", () => { + const error = { + _tag: "ShowHelp", + commandPath: ["db", "lint"], + errors: [ + new CliError.InvalidValue({ + option: "level", + value: "bogus", + expected: 'Expected "warning" | "error", got "bogus"', + kind: "flag", + }), + ], + }; + + expect(normalizeCliError(error)).toEqual({ + code: "InvalidValue", + message: 'Invalid value for flag --level: "bogus". Expected "warning" | "error", got "bogus"', + }); + }); + test("ShowHelp envelope unwraps a single MissingOption to Cobra wording", () => { // Effect CLI raises `ShowHelp` containing the parse error in its `errors` // array. We unwrap to surface the actionable message instead of "Help requested". From cff55253ddd9af08f26aa0f1421025f0ca4af9e6 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Wed, 8 Jul 2026 20:14:32 +0100 Subject: [PATCH 4/4] fix(cli): preserve raw InvalidValue.value instead of trimming it (review: #PRRT_kwDOErm0O86PWZlY) `value` on `CliError.InvalidValue` is the raw argv token the user typed and can legitimately be empty or carry surrounding whitespace (e.g. `--output-format ''` or `--output-format ' json'`). Reading it through the trim-and-reject-empty `readString` helper either dropped the rewrite entirely (leaking the original doubled "Expected: Expected" message for empty values) or silently reported a different value than what was typed. Added `readRawString` and used it only for `value`; `option`/`expected`/ `kind` stay on `readString` since those are Effect/CLI-generated, never user input. --- apps/cli/src/shared/output/normalize-error.ts | 16 +++++++- .../output/normalize-error.unit.test.ts | 37 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/apps/cli/src/shared/output/normalize-error.ts b/apps/cli/src/shared/output/normalize-error.ts index 221451685c..19cc375913 100644 --- a/apps/cli/src/shared/output/normalize-error.ts +++ b/apps/cli/src/shared/output/normalize-error.ts @@ -18,6 +18,16 @@ const readString = (value: ErrorRecord, key: string): string | undefined => { return typeof field === "string" && field.trim().length > 0 ? field.trim() : undefined; }; +// Unlike `readString`, does not trim or reject empty strings. Use this for +// fields that carry raw user input (e.g. `CliError.InvalidValue#value`), +// where an empty string or meaningful surrounding whitespace is a legitimate +// value the user typed (`supabase --output-format ''`) and must be preserved +// and reported verbatim rather than normalized away. +const readRawString = (value: ErrorRecord, key: string): string | undefined => { + const field = value[key]; + return typeof field === "string" ? field : undefined; +}; + const mappedError = (error: ErrorRecord): NormalizedCliError | undefined => { const tag = readString(error, "_tag"); switch (tag) { @@ -87,7 +97,11 @@ const mappedError = (error: ErrorRecord): NormalizedCliError | undefined => { // `InvalidValue` failure — whichever path it takes — renders the same // way. const option = readString(error, "option"); - const value = readString(error, "value"); + // Raw read: `value` is the exact argv token the user typed and can + // legitimately be `""` or carry surrounding whitespace — `readString` + // would trim it or drop it entirely, either masking the bug this case + // exists to fix or misreporting what the user actually typed. + const value = readRawString(error, "value"); const expected = readString(error, "expected"); const kind = readString(error, "kind"); if ( diff --git a/apps/cli/src/shared/output/normalize-error.unit.test.ts b/apps/cli/src/shared/output/normalize-error.unit.test.ts index 5e401270a4..107600ac13 100644 --- a/apps/cli/src/shared/output/normalize-error.unit.test.ts +++ b/apps/cli/src/shared/output/normalize-error.unit.test.ts @@ -71,6 +71,43 @@ describe("normalizeCliError", () => { }); }); + test("InvalidValue preserves an empty invalid value (e.g. `--output-format ''`)", () => { + // Regression test for a Codex review finding on CLI-1898: `value` is raw + // user input read straight off argv, so `''` is a legitimate way to + // trigger this failure. Reading it through the trim-and-reject-empty + // `readString` helper would fail the guard and leak the original + // doubled "Expected: Expected" message instead of fixing it. + const error = new CliError.InvalidValue({ + option: "output-format", + value: "", + expected: 'Expected "text" | "json" | "stream-json", got ""', + kind: "flag", + }); + + expect(normalizeCliError(error)).toEqual({ + code: "InvalidValue", + message: + 'Invalid value for flag --output-format: "". Expected "text" | "json" | "stream-json", got ""', + }); + }); + + test("InvalidValue preserves surrounding whitespace in the invalid value (e.g. `--output-format ' json'`)", () => { + // Regression test for the same Codex finding: trimming `value` would + // report a different string than what the user actually typed. + const error = new CliError.InvalidValue({ + option: "output-format", + value: " json", + expected: 'Expected "text" | "json" | "stream-json", got " json"', + kind: "flag", + }); + + expect(normalizeCliError(error)).toEqual({ + code: "InvalidValue", + message: + 'Invalid value for flag --output-format: " json". Expected "text" | "json" | "stream-json", got " json"', + }); + }); + test("InvalidValue leaves an already-clean 'expected' message untouched", () => { const error = new CliError.InvalidValue({ option: "define",