fix(cli): validate mutually-exclusive flags instead of silently resolving by precedence - #2039
Conversation
…ving by precedence `commit --split --plan --apply`, `amend --dry-run --apply`, `changelog --only-diff --range`, and `doctor --clear --cost` all parsed cleanly but silently let one flag win, leaving scripts that pass a contradictory combination with a different result than requested and no diagnostic (#1889). - commit: reject --plan+--apply, --print-message with any split flag, --apply without --split/--plan/`split` positional, and --strict-split without a split mode - amend: reject --dry-run+--apply - doctor: reject combining --clear/--cost/--fix - changelog: reject --only-diff combined with --range/--tag/--since-last-tag (--only-diff --branch still works — branch is the diff base)
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — BLOCK
REVIEW: BLOCK
RESOLVES: partial
The new .check() validators correctly enforce the mutually-exclusive flag rules for commit/amend/doctor/changelog, but the commit builder's check runs during yargs parsing — before the handler — so --json --apply and --json --plan --apply now throw a plain-text yargs error instead of the machine-readable emitJson payload the PR's own acceptance criteria require to stay unchanged. The regression is real and untested; everything else is sound.
1 blocker · 1 concern — 2 inline on the diff
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — CONCERNS
REVIEW: CONCERNS
RESOLVES: partial
The revise correctly fixes both prior findings (the check() bail-out for --json defers to handler's emitJson contract, and new tests cover --json + split-flag combos), verified by running the actual test suite. But the bail-out is broader than needed: it skips ALL check() validation whenever --json is set, not just the split/plan/apply combos handler.ts actually guards against, silently reintroducing the ticket's own bug for --json --strict-split (no --split/--plan).
1 concern — 1 inline on the diff
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — BLOCK
REVIEW: BLOCK
RESOLVES: partial
The mutually-exclusive-flag validation is correct for amend, doctor, and changelog, but the new commit .check() fires during yargs parsing before handler.ts ever runs, so it preempts commit's pre-existing --json structured-error contract for --split/--plan/--apply combos and replaces it with a plain-text yargs failure.
1 blocker — 1 inline on the diff
The commit builder's .check() ran during yargs parsing, before the handler — so --json combined with --plan/--apply threw a plain-text yargs error instead of reaching handler.ts's structured emitJson error contract for machine consumers. Scope the check's early-return to only combos handler.ts's own json guard already covers (json + one of split/plan/apply); --strict-split alone still validates in .check() since the handler guard doesn't consider it.
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
The revise commit correctly scopes the commit builder's .check() to defer json+split/plan/apply conflicts to handler.ts's existing structured emitJson guard, fixing the plain-text-vs-JSON regression it introduced without weakening any other validation (--strict-split-alone-under-json still throws in check() as required). New test coverage matches the implementation and the full test suite plus lint pass clean.
What
commit,amend,changelog, anddoctoraccepted several contradictory flag combinations and silently picked a winner by code-path precedence instead of erroring — so a user (or a script) passing a redundant/incorrect combination got a different behavior than requested, with no diagnostic.Why
Closes #1889
Plane: OSS-1744
How
commit: builder.check()rejects--plan+--apply,--print-messagecombined with any split flag (--split/--plan/--apply/--strict-split),--applywithout split mode (--split/--plan/thesplitpositional), and--strict-splitwithout split mode.coco commit split --applystill works since the positional counts as split mode. The existing--json+splitemitJsonerror inhandler.tsis untouched.amend: builder.check()rejects--dry-run+--apply.doctor: builder.check()rejects combining--clear/--cost/--fix.changelog: extended the existinghandler.tsexclusivity guard (kept in the handler, not the builder, since it reads config-mergedsinceLastTag/range) to reject--only-diffcombined with--range/--tag/--since-last-tag.--only-diff --branchstill works — branch is the diff base ref for--only-diff.All four follow the
.check()pattern already used byreview's builder.Testing
npm run build)npm run test:jest— newconfigCheck.test.tsfor commit/amend/doctor using thereview/config.test.tsextractCheckFnstub pattern, plus--only-diffcases added tochangelog.test.ts)npm run lint)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Closes #1889