fix(cli): report an unknown install target as an argument error - #3604
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7190ab56ae
ℹ️ 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".
`veryfront install not-a-tool` exited 1. The target was only checked inside `installCommand`, where `parseTargetFlag` throws a plain runtime error, so a typo was indistinguishable from an installation failure. AGENTS.md reserves exit 1 for runtime errors and exit 2 for usage and argument errors. Validate the target while parsing arguments instead, so the failure goes through `parseArgsOrThrow` and the router's "Invalid ..." usage path and the message names the valid targets. `--target not-a-tool` takes the same path and also exits 2 now. `parseTargetFlag` keeps its own check for programmatic callers. The positional-target fix this branch originally carried landed on main in #3609, so only the exit-code correction remains here. The end-to-end case #3609 added for an unknown positional asserted exit 1 and is updated to 2.
9e731b3 to
26ead02
Compare
Scope after rebase
This branch originally carried two things: the positional tool id for
veryfront install <tool>, and the exit code for an unknown tool id. Thepositional fix landed on main independently as #3609, so only the exit-code
correction remains here. The branch has been rebased onto main and the diff
is now four files in
cli/commands/install/.Symptom
A typo in the tool id exited 1, the code AGENTS.md reserves for a runtime or
command error. A caller — a script, CI, or a person reading the exit status —
could not tell a malformed argument from an installation that genuinely failed
partway through.
Root cause
parseInstallArgsacceptedtargetas an arbitrary string. The value was onlychecked later, inside
installCommand, whereparseTargetFlagthrows anordinary runtime error. By then the CLI is past argument parsing, so the
router's usage path — the one that produces exit 2 — is no longer in play.
Fix
cli/commands/install/handler.ts— validatetargetwhile parsing arguments,via a
refineon the schema. The failure now goes throughparseArgsOrThrowand the router's
Invalid ...usage path, and the message names the validtargets:
--target not-a-tooltakes the same path and also exits 2.uninstallsharesthe parser, so it is covered too.
cli/commands/install/install.ts— exportisValidTargetSpecandVALID_TARGET_VALUESso the check and the message both come from the singletool registry rather than a second hand-maintained list.
parseTargetFlagkeeps its own check as a guard for programmatic callers.
AGENTS.md contract
12Tests
install.integration.test.ts— the end-to-end case fix(cli): accept a bare tool name for veryfront install #3609 added for anunknown positional asserted exit 1; it now asserts exit 2 and that the output
names the valid targets. A sibling case covers
--target. Both still assertthat no integration file is written.
handler.test.ts— two parser-level cases: an unknown positional and anunknown
--targetboth fail argument validation.Changing the existing assertion from 1 to 2 is the behaviour change this PR
owns, so that test is red on main by construction.
Review
Raised by codex as a P2 on the original branch. The finding was correct and is
fixed rather than argued with.