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 1cc5124f98..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; @@ -228,6 +229,20 @@ export function formatCliErrorsForDisplay( 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/cli/subcommand-flag-suggestions.unit.test.ts b/apps/cli/src/shared/cli/subcommand-flag-suggestions.unit.test.ts index 079256d354..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 @@ -122,4 +122,101 @@ 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', + ); + }); + + 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"', + ); + }); }); diff --git a/apps/cli/src/shared/output/normalize-error.ts b/apps/cli/src/shared/output/normalize-error.ts index a143b414b3..19cc375913 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; @@ -17,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) { @@ -75,6 +86,35 @@ 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"); + // 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 ( + 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..107600ac13 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,98 @@ 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 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", + 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". 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/); + }); });