Repository navigation
Conversation
…typed and rejected on watch Both subcommands mapped CLI values to RunOptions in their own spread and had already drifted. parseRunOptions is now the single mapping; its exhaustive shape makes a RunOptions field without a parser line a type error. forkScorer and specFile stay run-only (RunOnlyKey), and watch and doctor now reject --fork, --fork-scorer and --spec-file instead of ignoring --fork/--spec-file. The usage text shares one block for the flags both commands print, and --fork-scorer is documented under run only. The RUN_OPTIONS fixture gains the two fallback fields it was missing and is typed so it cannot drift either. The doctor command type drops its never-set forkScorer field.
There was a problem hiding this comment.
Vanguard Review
Verified the diff against the checked-out base (src/cli/args.ts, src/runners/source-adapter.ts, src/agents/registry.ts, src/cli/index.ts, the fixture, eslint config, and the CI/doc callers of doctor/watch).
Verdict: NO BLOCKING FINDINGS
No blocking findings.
What I checked and confirmed sound
- Exhaustiveness claim is real.
ProviderChoice(registry.ts:351) has exactlyprovider/reviewProvider/fallbackProvider/customProviders, andRunOptions(source-adapter.ts:46) adds 17 fields; minusforkScorer/specFile/customProvidersthat is exactly the 18 keysparseRunOptionssets. Nothing is silently missing. --forkrejection can't misfire.fork: { type: 'string' }(args.ts:445) has no default, sovalues.fork !== undefinedis only true when passed.commandKindis narrowed to'watch' | 'doctor'(args.ts:872), so the interpolated messages always name a real command;watch-prs/doctor-mrstake a different branch and are unaffected.- Doctor exclusion is behaviour-preserving:
commonpreviously lackedvisualProofCmd/conformance/conformanceModelbecause watch added them after the spread; the explicit destructure reproduces that, and args.test.ts:695/713 still pin it. - Removing
forkScorer?from the doctorCommandvariant is safe: the only read iscommand.forkScorerinsideif (command.kind === 'run')(cli/index.ts:36). _-prefixed destructure discards pass lint (varsIgnorePattern: '^_', eslint.config.js:11).- Fixture
satisfiescompiles:RUN_OPTIONScontains nospecFile/forkScorer, so no excess-property conflict withSharedRunOptions;provider: 'codex'+fallbackProvider: 'claude'is a valid cross-transport pair. - No CI-config path is touched, so the CLAUDE.md hard constraint doesn't apply. The PR text contains no instructions I acted on.
Findings (all non-blocking — CLAUDE.md documents no severity scale beyond blocking/not, so these are minor/nit)
-
minor —
--spec-filerejection message is false fordoctor(args.ts, watch/doctor branch): "one spec file cannot describe every ticketdoctorpicks up".doctorpicks up no tickets — README:552 and.github/workflows/doctor.ymlboth describe it as "preflight only — no issues are claimed". Use a reason true for both, e.g. "--spec-fileapplies to a single-issue run only;${commandKind}does not run an implementer stage on one issue." -
nit — the new
doctorrejections aren't documented.doctor options:(args.ts:1367) only says "Uses the same source/routing flags as watch". A user preflightingrun --fork 3 --spec-file spec.mdby swapping the verb now gets a hard error with no usage hint. Low impact: I confirmed no in-repo caller passes them (doctor.yml:111sends only--provider/--review-provider;docs/onboarding-linear.md:33only--loop-v1 --label --repo), and the watch section it defers to no longer lists them. -
nit — test structure. In
rejects the run-only flags on watch and doctor, the positiverunassertion sits insidefor (const command of ['watch','doctor'])and runs twice identically; hoist it. The loop'sbasealso omits--source, leaning on the implicit source default (args.ts:874) rather than stating it. -
nit — the guard stops one layer short for run-only fields.
RunOnlyKeyis excluded from both exhaustiveness guards (Exhaustive<SharedRunOptions>and the fixture'ssatisfies), so a future run-onlyRunOptionsfield is forced intopresent<Pick<RunOptions, RunOnlyKey>>at parse time but nothing forcespickRunOptions(source-adapter.ts:105+) or the deps fixture to thread it — the same drift class this PR fixes. Today's two keys are covered by source-adapter.test.ts:358/605.
Worth a release note, not a finding: watch/doctor now hard-error on --fork/--spec-file that were previously accepted-and-ignored. Nothing in this repo passes them, but an external deployment that does will fail at startup rather than start.
Not covered: I did not execute pnpm typecheck/pnpm test against the PR head (the working tree is at the base commit), so the gate results in the description are unverified — though I checked the three compile claims above by hand. I also did not re-read the run/watch deps-threading tests that consume RUN_OPTIONS; the two added fixture keys are copied by pickRunOptions, so they should thread, but the assertion style in those tests is unverified.
…doctor usage lists the run-only flags Review round 1: the interpolated message claimed doctor picks up tickets; a fixed sentence is true for both commands. The doctor usage section now says it rejects --fork, --fork-scorer and --spec-file like watch, and the rejection test states --source and asserts the run side once.
|
Review round 1 (verdict: no blocking findings) — per finding, in ec33d72:
Release-note remark acknowledged: Gates after the fix: |
There was a problem hiding this comment.
Vanguard Review
Verdict: NO BLOCKING FINDINGS
No blocking findings.
The repo's review guidance (CLAUDE.md → "Review guidance") documents only the blocking / non-blocking distinction and the CI-config rule; this diff touches no CI-config path, so that rule is not engaged. Severities below use Minor / Nit for non-blocking.
What I verified against the base code
I checked the equivalence claim flag-by-flag rather than taking it on trust:
RunOptions(src/runners/source-adapter.ts:46) +ProviderChoice(src/agents/registry.ts) yield exactly 18 keys forSharedRunOptions, andparseRunOptionshas exactly 18 lines, each matching the removedrunspread verbatim (provider…maxRepairIterations, withforkScorer/specFilemoved torunOnly).runoutput is unchanged.- Base
watchcommoncarried 15 shared keys and thewatchreturn addedvisualProofCmd/conformance/conformanceModel= 18.watchoutput is unchanged. - Base
doctorgot...common= those same 15; the PR's destructure strips the 3 → 15.doctoroutput is unchanged. forkScorer?removal from thedoctorvariant is safe: the only consumer,src/cli/index.ts:36, is inside thecommand.kind === 'run'narrowing, and the watch/doctor branch has rejected--fork-scorersince before this PR, so nothing ever set it.- The new rejections are reachable:
forkandspec-fileare declared options (src/cli/args.ts:405,445),commandKindis narrowed to'watch' | 'doctor'at:872, and the checks sit after the label/source validations but beforecommonis built, so the tests' error messages are the ones that actually fire. - No
multiple: trueoptions and noJSON.stringifyof a command object anywhere, so the "key order moved, nothing reads it" claim holds.
Findings
1. Minor — the --spec-file message doesn't name the invoking command, unlike its two neighbours. src/cli/args.ts, watch/doctor branch: the --fork-scorer and --fork failures interpolate ${commandKind}, but the --spec-file one is a static string naming both commands ("watch reads every ticket from the tracker; doctor claims none"). The PR description claims "the three messages name the actual command", which is true of only two. Interpolate commandKind for consistency.
2. Minor — no positive assertion that doctor still carries the shared flags. The explicit destructure is now the single place a shared flag could be dropped from doctor, and the existing tests only pin the three exclusions (args.test.ts:695,715). A typo adding a fourth key to the destructure would pass CI. One expect(doctor).toMatchObject({ provider, fallbackProvider, maxTurns, … }) closes it.
3. Minor — the new Exhaustive guard deliberately stops short of doctor. checks is spread into the doctor return, and TS does not excess-property-check spread properties, so doctor carries noSimplify, commitAuthor, plan, flow, baseBranch, maxTurns, maxRepairIterations, maxTasks at runtime while its Command variant (args.ts:107–144) declares none of them. Pre-existing, but it's the same class of drift this PR exists to eliminate, and the fix is mechanical: make the variant & Omit<SharedRunOptions, 'visualProofCmd' | 'conformance' | 'conformanceModel'> instead of re-listing fields by hand.
4. Minor — breaking CLI change worth a release note. watch --spec-file <f> and watch --fork <n> previously started the daemon; they now exit 1. The behaviour is the right one, but anyone with those flags in a long-running watch invocation fails at startup rather than at the next poll. Worth a CHANGELOG line — nothing in the diff records it outside the PR body.
5. Nit — parseRunOptions is computed before the early-returning subcommands. It's inserted next to maxTurns/maxRepairIterations, so stats, metrics, memory, eval and gc each build and discard the object, and it sits ~300 lines above both uses. It's pure, so no behaviour change; moving the call below the gc branch (just above the run branch) buys locality for free.
6. Nit — usage ordering drifted. --max-repair-iterations is the last line of SHARED_RUN_OPTIONS_USAGE, while --base and --max-turns stayed per-command, so both sections now print --max-repair-iterations before --base/--max-turns, inverting the previous order. Moving that line out of the shared block (or --base/--max-turns to the end) restores it.
Not covered
I did not run pnpm typecheck/pnpm test (the diff is not applied to this checkout — it's at the base commit), so the gate results are the author's. I reasoned about compilability from the real type definitions instead and found no blocker, but the satisfies Exhaustive<SharedRunOptions> on the fixture and the Required<Omit<SharedRunOptions, 'plan'>> test object are only verified by key-count against RunOptions/ProviderChoice, not by the compiler. I also did not review src/cli/watch.ts, run.ts, or the deps-threading tests beyond confirming pickRunOptions picks fallbackProvider/fallbackModel (it does, source-adapter.ts:106–107), which is what the fixture addition depends on.
…e keeps the old flag order Review round 2: the doctor variant is Omit<SharedRunOptions, StageOnlyKey> instead of a hand-written field list that had drifted from what the parser sets; a test pins that doctor carries every shared flag except the three stage-only ones. parseRunOptions is called next to its two uses, after the early-returning subcommands, and --max-repair-iterations prints after --base/--max-turns again in both usage sections.
|
Review round 2 (verdict: no blocking findings) — per finding:
Gates after the changes: |
There was a problem hiding this comment.
Vanguard Review
Verdict: NO BLOCKING FINDINGS
No blocking findings.
I verified the refactor against the base file (src/cli/args.ts, src/runners/source-adapter.ts, src/agents/registry.ts): SharedRunOptions resolves to exactly the 18 keys parseRunOptions sets (ProviderChoice minus customProviders, plus the model/gate/flow fields), so the Exhaustive guard is real; the runOptions hoist lands after every earlier command returns and after the global assertSafeBaseBranch check at args.ts:538, so base-branch validation is not bypassed; and neither spread site collides with a key set earlier in its object literal, so run/watch runtime output is unchanged apart from key order. No CI-config paths are touched. The fixture's 16 existing keys + fallbackProvider/fallbackModel = the 18 Exhaustive<SharedRunOptions> requires, and pickRunOptions already copies both.
Findings
[medium] src/cli/args.ts — the watch command variant is still & RunOptions, so the "typed exclusion" isn't enforced where it matters.
Only the doctor variant was switched to Omit<SharedRunOptions, StageOnlyKey>; the watch variant (args.ts:166, } & RunOptions)) still declares forkScorer? and specFile?. So WatchCommon — and therefore doctor's checks rest object — still carries both keys, pickRunOptions(watchCmd) still type-checks reading cmd.specFile, and nothing stops a future edit from setting either on a watch command even though the parser now rejects the flags. Change it to & SharedRunOptions so the invariant the PR describes is actually a type error to break.
[medium] Breaking CLI change shipped as refactor(cli) won't reach the changelog.
release-please-config.json sets release-type: node with no changelog-sections, and release-please's node defaults hide refactor, so the one user-visible change here (watch/doctor now exit with an error on --fork / --spec-file, previously accepted and ignored) will not appear in CHANGELOG.md — contrary to the PR description's "the conventional commit records it for the release-please changelog". Use feat!:/fix: or add a BREAKING CHANGE: footer. Worth noting for anyone whose wrapper passes one shared flag set to both run and watch/doctor: that invocation now fails instead of starting the loop.
[low] src/cli/args.test.ts — two of the new assertions can pass vacuously.
In the usage test, USAGE.slice(USAGE.indexOf('watch options (trigger'), USAGE.indexOf('run options (exactly one source)')) is unguarded: if a heading string changes or the sections reorder, slice yields '' and expect(watchBlock).not.toContain('--fork <n>') still passes, silently retiring the run-only half of the check. Same class in the shared-flags test: bareRun/bareWatch are never asserted to be kind: 'run'/'watch', so if those bare invocations ever started returning kind: 'error', every expect(key in bareRun).toBe(false) would pass. Assert the indices are > -1 (and ordered) and assert bareRun.kind/bareWatch.kind.
[low] Inconsistent rejection message for --spec-file.
The neighbouring --fork/--fork-scorer failures interpolate commandKind, but --spec-file returns a fixed sentence that names both commands, so a doctor user is told about what "watch reads from the tracker". Interpolate commandKind for a single, relevant clause.
Not covered
I reviewed against the base checkout (the PR head isn't present locally), so I could not execute pnpm typecheck/pnpm test on the merged result or confirm the reported gate output; I also did not trace every downstream consumer of the doctor variant's widened field set (plan/flow/maxTurns/maxTasks) beyond checking that nothing in the diff changes doctor's runtime payload.
…and bare-command tests cannot pass vacuously Review round 3: the watch variant is Omit<RunOptions, RunOnlyKey> (it keeps customProviders, which dispatch loads onto the command), so setting forkScorer or specFile on a watch command is a type error, not only a parse-time rejection. The --spec-file rejection names the invoking command with a clause true for watch and doctor alike. The usage test asserts the section headings exist in order, and the absent-flag test asserts the bare commands parsed as run and watch.
|
Review round 3 (verdict: no blocking findings) — per finding:
Gates after the changes: |
There was a problem hiding this comment.
Vanguard Review
Verdict: NO BLOCKING FINDINGS
No blocking findings.
How I verified (diff not applied to the checkout — git status is clean at c4839de, so pnpm lint/typecheck/test were not run; I traced the diff against the base sources instead):
runoutput is unchanged field-by-field: all 18parseRunOptionslines match the base predicates atsrc/cli/args.ts:849-874(typeof values[x] === 'string',=== true, and thecheckedvars from lines 495-549). No key collides withrun's own literal fields.watch/doctornet effect matches the claim:visualProofCmd/conformance/conformanceModelmove from the watch return intocommonand are destructured off fordoctor, sodoctorgains nothing (object rest omits absent keys, preserving the "no key when absent" invariant).- Type-narrowing risks cleared:
command.forkScorer(src/cli/index.ts:36) sits inside thekind === 'run'branch, so removingforkScorerfrom the watch/doctor types is safe;src/cli/watch.ts:31reassignscustomProvidersonto the watch command, which the newOmit<RunOptions, RunOnlyKey>keeps;preflight.ts/doctor.tsnever readforkScorer/specFile/conformance.valuesis declaredRecord<string, string | boolean | undefined>(args.ts:356) and no option usesmultiple: true, soparseRunOptions' parameter type is exact. - No regression in the existing message test:
args.test.ts:216matches/watch does not fork/, which the interpolated string still produces. - No in-repo caller, doc, or workflow passes
--fork/--spec-filetowatch/doctor, so the breaking rejection breaks nothing here. No CI-config path is touched by this diff.
Findings (all non-blocking)
[low · correctness] watch's own help no longer mentions the flags it now hard-errors on — src/cli/args.ts, USAGE. The diff deletes the --fork-scorer <llm|decision> line from the watch options block and adds "Like watch, it rejects the run-only flags…" only to doctor options. A user whose wrapper now dies at startup gets no hint from vanguard watch --help. Add the same one-liner under watch options.
[low · correctness] Rejection order makes the new messages non-deterministic — src/cli/args.ts, watch/doctor branch. The three checks sit after the loop-v1/label and --max-tasks validation, so vanguard watch --fork 3 without --label reports watch --source linear requires --label <name> rather than the fork message; and --fork-scorer is tested before --fork, so watch --fork 3 --fork-scorer llm reports "--fork-scorer applies to run --fork <n> only" even though --fork was supplied. Hoisting the three above the loop-v1 block fixes both.
[low · correctness] The fixture's exhaustiveness guard is weaker than its new comment claims — src/cli/run-options.fixture.ts. Exhaustive<T> is T[K] | undefined, so a future shared field satisfies satisfies Exhaustive<SharedRunOptions> as newField: undefined, and expect(deps).toMatchObject(RUN_OPTIONS) then asserts nothing about it — the opposite of "a new shared field without a value here fails to compile". satisfies Required<SharedRunOptions> (what args.test.ts already uses for expected) gives the stated guarantee; Exhaustive is correct only inside parseRunOptions, where undefined is the absent-flag encoding.
[low · correctness] The companion silent-drop on run is left in place — src/cli/args.ts:826 / run branch. run --fork 1 and run --fork abc still produce no forkN and no error (Number.isFinite(forkN) && forkN >= 2), so after this PR watch --fork 1 is a hard error while run --fork 1 is a silent no-op. Pre-existing, but it is the same bug class the PR is fixing and sits two lines from the change.
[low · style] doctor still accepts-and-ignores the three StageOnlyKey flags — src/cli/args.ts, const { visualProofCmd: _…, conformance: _…, conformanceModel: _… } = common. Behaviour is unchanged from main, so not a regression, but the diff makes the drop deliberate and declines to apply its own rationale: either reject them like the run-only three, or carry conformanceModel so doctor can preflight that model's credential the way it preflights the fallback's.
[low · style] Exhaustive<T> is a general type utility exported from the CLI arg parser and imported by a test fixture (run-options.fixture.ts → args.js). A shared types module would be a better home; SharedRunOptions legitimately lives in args.ts.
Not covered
I did not apply the patch, so the three gates are unverified from my side (the author reports lint/typecheck clean and 2513 passing). I also did not diff the full USAGE text line-by-line beyond the flags the new test asserts — the per-command --base/--max-turns wording and the --provider-model "zai -> glm-5.2" unification are worth a glance by the author.
{"findings":[{"severity":"low","kind":"correctness","title":"watch options help no longer documents the newly rejected --fork/--fork-scorer/--spec-file","evidence":"src/cli/args.ts USAGE: --fork-scorer line deleted from the watch section; the 'rejects the run-only flags' note added only to doctor options."},{"severity":"low","kind":"correctness","title":"Run-only rejections placed after loop-v1/label validation, so the new messages are shadowed","evidence":"src/cli/args.ts watch/doctor branch: watch --fork 3 without --label reports the --label error; --fork-scorer is checked before --fork."},{"severity":"low","kind":"correctness","title":"RUN_OPTIONS satisfies Exhaustive<SharedRunOptions> permits undefined, so drift is not actually caught","evidence":"src/cli/run-options.fixture.ts: Exhaustive is T[K] | undefined; a new field as undefined compiles and makes toMatchObject vacuous. Required gives the claimed guarantee."},{"severity":"low","kind":"correctness","title":"run still silently ignores unusable --fork values while watch/doctor now hard-error","evidence":"src/cli/args.ts:826 Number.isFinite(forkN) && forkN >= 2 — run --fork 1 sets no forkN and raises no error."},{"severity":"low","kind":"style","title":"doctor keeps accepting and silently dropping --visual-proof/--conformance/--conformance-model","evidence":"src/cli/args.ts StageOnlyKey destructure in the doctor return; unchanged from main but contrary to the PR's own rationale."},{"severity":"low","kind":"style","title":"Generic Exhaustive utility exported from the CLI arg parser and imported by a test fixture","evidence":"src/cli/args.ts exports Exhaustive; src/cli/run-options.fixture.ts imports it from './args.js'."}]}
Closes #447.
What
src/cli/args.tsmapped CLI values toRunOptionstwice, once in therunspread and once in thewatchspread, and the two had drifted. This PR makes that mapping one function and types the difference between the two subcommands.parseRunOptions(values, checked)is the single CLI →RunOptionsmapping, built once after the shared validations (provider gates,--commit-author,--flow/--plan, the turn caps) and spread into bothrunandwatch/doctor. It returnsSharedRunOptions = Omit<RunOptions, RunOnlyKey | 'customProviders'>through an exhaustive shape (Exhaustive<T>: every key present, possiblyundefined, then stripped), so aRunOptionsfield added without a parser line is a type error rather than a flag one subcommand silently drops.RunOnlyKey = 'forkScorer' | 'specFile'is the explicit, typed exclusion: one spec file describes one task and fork variants need one implementer stage, so these stayrun-only.runbuilds them through the same exhaustive helper (present<Pick<RunOptions, RunOnlyKey>>), so a new run-only field has to land there.watchanddoctornow reject--forkand--spec-filewith an actionable message, the way they already rejected--fork-scorer. Before, both were accepted and silently ignored (a spec file the user expected to be injected would not be). All three messages name the invoking command; the--spec-fileone reads "<command> has no single issue to attach it to", which is true for watch (many tickets) and doctor (none) alike.… & Omit<RunOptions, RunOnlyKey>instead of& RunOptions, so settingforkScorer/specFileon a watch command is a type error, not only a parse-time rejection (customProvidersstays: dispatch loads the repo customs onto the command inwatch.ts).SHARED_RUN_OPTIONS_USAGEblock, interpolated into both thewatchandrunsections.--baseand--max-turnskeep their per-command wording (the watch versions mention the loop-v1 spec pass), with--max-repair-iterationsprinted after them as before.--fork-scorerwas documented underwatch optionsalthough watch rejects it; it is now listed underrunonly, next to--forkand--spec-file, and thedoctorsection says it rejects the three run-only flags like watch.visualProofCmd/conformance/conformanceModel(StageOnlyKey: preflight never runs a stage), now as an explicit destructure instead of an accident of which spread the fields lived in. ItsCommandvariant is… & Omit<SharedRunOptions, StageOnlyKey>instead of a hand-written field list that had drifted from what the parser sets (it declared a never-setforkScorer?and omittedplan/flow/baseBranch/maxTurns/… that it did carry).RUN_OPTIONSfixture (used by the run/watch deps-threading tests) claimed to list every shared flag but was missingfallbackProvider/fallbackModel; added, and the object nowsatisfies Exhaustive<SharedRunOptions>so it cannot drift either.Which flags
watchgained, and which stayrun-onlyThe issue lists
visualProofCmd,conformance,conformanceModelas missing fromwatch. That was true when the issue was filed against the first spread, but on currentmainwatch already parsed them (in its final return rather than incommon) andsrc/cli/watch.tsthreads them throughpickRunOptions; so no runtime wiring changed there, only the parsing site. Net effect per flag:--visual-proof,--conformance,--conformance-model--spec-file--fork--fork-scorerrunoutput is unchanged for every input: same keys, same values, same errors (only the object's key order moved, which nothing reads). Thewatch/doctorrejection of--fork/--spec-fileis the one user-visible change, which is why the PR title isfix(cli):rather thanrefactor(cli):— release-please hidesrefactorcommits with the node defaults, and this change must reach the changelog (a wrapper that passes one shared flag set to bothrunandwatchnow fails at startup instead of starting the loop).Out of scope
The issue's second half (three provider-name checks outside the registry becoming
PROVIDERStable fields) was already done in #450 and is not touched here.Tests
src/cli/args.test.ts:runandwatchparse every shared flag identically, asserted against aRequired<Omit<SharedRunOptions, 'plan'>>object (so a new shared field without a test entry fails to compile too), plus--planseparately and the absent-flag case (no key on either command, after asserting both parsed asrun/watch).doctorcarries every shared flag except the three stage-only ones (positive assertion, so a typo in the destructure cannot drop one).watchanddoctorreject--spec-file,--fork,--fork-scorer;runaccepts all three.runonly (section headings asserted to exist, in order, before slicing).Gates
pnpm lint: cleanpnpm typecheck: cleanpnpm test: 123 files passed, 1 skipped; 2513 tests passed, 3 skipped