fix(cli): remove doubled "Expected: Expected" prefix from Flag.choice/primitive errors#5831
Conversation
…/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
|
@codex review |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a294658cfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rror.message (review: #PRRT_kwDOErm0O86PVbq8) error.message interpolates the user-controlled error.value twice (once directly, once again inside error.expected's "got <value>" 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6be301da9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ags (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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d843e536a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…iew: #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.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What changed
Any
Flag.choice(...)/Flag.choiceWithValue(...)-backed flag (--sizeonprojects create/branches create,--dns-resolver,--output-format,--output/-o,--agent) showed a doubled "Expected: Expected ..." prefixwhen given an invalid value, e.g.:
Root cause: several
Primitives undereffect@4.0.0-beta.93'seffect/unstable/cli(choice— used byFlag.choice/Flag.choiceWithValue— plus the schema-backed
integer,float,boolean, anddate) fail witha raw message that already starts with the word "Expected" (e.g.
`Expected ${validChoices}, got ${value}`forchoice, orExpected a valid date, got Invalid Datefordate), andCliError.InvalidValue's ownmessagegetter (CliError.ts:359-364)independently prepends its own
"Expected: "label on top of that —producing the doubling for any flag or argument backed by one of these
primitives. This is a bug in the vendored
effectbeta package itself, notin this repo's code.
Workaround (until upstream
effectis fixed) added to the shared CLIerror-formatting layer (
subcommand-flag-suggestions.ts), which alreadycarries similar per-tag rewrites for
UnrecognizedOption/UnknownSubcommand:collapse the literal
"Expected: Expected "substring down to a single"Expected "whenever it appears in anInvalidValueerror's renderedmessage. Collapsing the exact doubled substring (rather than rebuilding the
surrounding "Invalid value for ..." template ourselves) keeps the fix tied
to the precise defect, covers every affected primitive uniformly, and lets
every other part of the message keep tracking upstream's own wording if it
changes.
Verified against
go-parity-auditor: Go's real CLI shows a completelydifferent message for the same scenario (
Error: invalid argument "bogus" for "--size" flag: must be one of [ large | medium | ... ], fromapps/cli-go/internal/utils/enum.gowrapped by cobra/pflag). Fullbyte-parity with Go's wording is out of scope here — this ticket is
specifically about the doubled-prefix defect in the TS/Effect-native
message, not a "TS wording differs from Go" report; rewriting every
Flag.choiceflag's phrasing to match Go's template would be a separate,larger change.
Known related (not fixed here)
The underlying
Primitive.choicefailure text also appends a redundant, got "X"suffix, so the invalid value still appears twice in the finalmessage (once right after the flag name, once after "got"). That's a
pre-existing wart in the same vendored
effectfailure string, notsomething this fix introduces, and collapsing it would mean post-processing
effect'sexpectedstring further — bigger than this "doubled prefix"ticket calls for. Left as-is.
Fixes CLI-1898