fix(pipeline-cli): a refused invocation is not a verdict — bad flags exit 4, not stop (#5072) - #5091
Conversation
…exit 4, not `stop` (#5072) `cp-cardinality decide` exits 1 for `stop`, and effect-cli also exited 1 for an unrecognized flag. The crew engine's approval-watcher, following prose that described the verb's interface instead of showing the call, invented `--pr`/`--head` — flags the verb never accepted — and recorded the resulting exit 1 as a definite "no approval at current head" for four §CP PRs that were approved at their exact heads. - the router seats every effect-cli parse error on `BAD_INVOCATION_EXIT_CODE` (4, the same never-ran band as `STDIN_READ_FAILED_EXIT_CODE`), so a refused call can never land on a verdict code - the approval-watcher tick block now carries the literal, runnable discharge: it derives `--non-author-approval-at-head` itself (exact-login match against an active control-plane member, `.commit_id == $HEAD`, non-author) through real `jq` — `gh api --jq` takes no `--arg`, and that misuse yields an empty string a caller reads as "no approvers" — and enumerates cp-cardinality's codes, mapping anything that is not 0/1 to `unknown:` - the false "the call resolves the approver set and the signal flags itself — do NOT re-derive either here" sentence is gone Fixes #5072
No preview deploy
|
|
review-code: advisory — blocking-set PR (§CP — approval-gated) Reviewed-head: @ 4f55211 Verified PR #5091 against the acceptance criteria of #5072, one at a time. Every AC passes and the load-bearing exit-code change is sound. One blocking finding, and it is a PR-body gap rather than a code defect — see the deviation-disclosure row. §CP classification (re-run, not inherited)The classifier at this head reports CONTROL_PLANE_TOUCHED non-empty over 8 of the 11 changed files — the formats contract Acceptance criteria
The exit-code change, judged specifically(a) A bad invocation really returns 4 — verified on the real bin, base versus head, as above. (b) Genuine verdicts are untouched — at this head, (c) The catch cannot swallow a real handler error and mislabel it as bad input. I checked this specifically, since it was the worst available outcome, and it holds for three independent reasons:
So the fix does not introduce a new false-verdict path. On reusing 4 for both BAD_INVOCATION and STDIN_READ_FAILED — the collapse is right. Both mean "the tool never got usable input, so it never decided", and every caller's exit table wants the same branch for them: not a verdict. One never-ran band makes the safe reading a single test — anything outside {0,1} is unknown — instead of a list a caller can under-enumerate, which is the very failure mode this issue is about. A caller that genuinely needs to tell them apart still can, from stderr. Splitting them would buy nothing and would tempt callers to branch on the difference. Non-blocking finding — a residual "never ran" path still lands on exit 1Not an acceptance criterion, not introduced here, and not a reason to hold this PR — but it is the same defect class one branch over in the same file, so it deserves a follow-up ticket. The
Verified: a typo'd tool name returns rc=1, while a typo'd subcommand correctly returns rc=4. Both Verification performed
CI at this head — one pre-existing red, NOT this PR's44 check runs: 43 green or skipped, 1 failure. The failure is "check docs have no dead internal links", and its sole error is ADR 0244 citing an eval-corpus file under Ruling on the deferred ship-it step-0 §CP approval script — I agree with the authorI read that file at this head. The deferral is right, on three grounds:
One thing the follow-up ticket must carry, so it is not lost: the deferral is not cost-free. That script's stop branch prints "awaiting control-plane approval — cardinality branch not satisfied", and its own inline comment asserts the message is reachable only with every input proven readable, so it "states a fact". With exit 4 now distinct, that claim is falsifiable — a malformed call would reach that line and state a false definite, exactly the class this PR exists to close. The fix is small (read the status and enumerate 0 / 1 / else, mirroring the block this PR just wrote) and it is now cheaper than before, because 4 exists. File it. Verdict: FAIL — on the deviation-disclosure row alone. The engineering is sound and I would have passed it otherwise: the exit-code fix is correct and opens no new false-verdict path, the def block is literally runnable and I ran it, and the ledger was annotated rather than rewritten. The remedy is a PR-body edit, not a code change and not a re-run of anything above. review-code does not merge, and this advisory authorizes nothing on its own: PR #5091 is §CP-blocking and needs a Verdict-written: 2026-08-09T08:05:55Z |
|
review-skill: advisory — blocking-set PR (§CP — approval-gated) Reviewed-head: @ 4f55211 The skills class of PR #5091: the crew engineering-manager agent def and the §CP: BLOCKING. What this class had to get rightThe root cause of #5072 is a behavioral artifact defect, not a code defect: the def described a tool's flags instead of showing the call, so a seat following it invented
Skill hygiene
Two non-blocking nitsNeither blocks; both are worth a moment if the body is being edited anyway.
Deviation disclosure
Verdict: FAIL — on the deviation-disclosure row alone. The skills half is otherwise the strongest part of this PR: it fixes the actual root cause rather than only the symptom, it replaced the false instruction with a correct model instead of deleting it, and its block is genuinely runnable — I ran it. Nothing here needs re-verification after the body edit. review-skill does not merge. PR #5091 is §CP-blocking and needs a Verdict-written: 2026-08-09T08:07:17Z |
|
review-code: advisory — blocking-set PR (§CP — approval-gated) PR #5091 is §CP — it touches the control plane (a gate-critical skill, Reviewed-head: @ 643d5ac Re-gate at a new head. Verdict: PASS (advisory). The prior gate at The head is a merge — and it dragged in nothing
Therefore every substantive finding in the prior verdict transfers unchanged, and I did not re-run what it verified hard (the base-vs-head exit-code reproduction, the four-PR tick replay, the Class routing, re-derived
§CP classification, re-run not inheritedThe classifier at this head reports The one row that changed: deviation-disclosure (§DEV)
This row reads as §DEV requires it to: nothing undisclosed that this gate could see — never no deviations exist. Acceptance criteria — all 8 PASS, carried at the new headVerified at
CI and evidence at this headRun-evidence bundle: PRESENT for head 44 check runs: 40 green, 4 conditionally skipped, 0 failures. Two I read specifically because the merge changed their inputs:
Also green and relevant: Typecheck is CI-authoritative here rather than re-run locally: the prior gate ran a forced uncached typecheck (30/30) on the identical tree, and the merged head's typecheck is covered green by CI plus the 2420-test bundle. Noting the trap for the record — a ~150ms "FULL TURBO" run is a replayed cache and proves nothing. Read the PR head (§HEAD)All files under review sourced from Non-blocking, carried forward — not re-raisedBoth already have tickets and neither holds this PR: #5094 (the Verdict: PASS (advisory). Every acceptance criterion passes, the deviation-disclosure row is now clean, and the merge is provably content-neutral. review-code does not merge. PR #5091 is §CP-blocking: it needs a Verdict-written: 2026-08-09T08:38:24Z |
|
review-skill: advisory — blocking-set PR (§CP — approval-gated) PR #5091 is §CP — Reviewed-head: @ 643d5ac Re-gate at a new head. Verdict: PASS (advisory). The skills half of this diff — the crew engineering-manager agent def and the Why this is a re-bind, not a re-reviewThe prior gate at I proved that rather than taking it on report. What this class had to get right — all PASS, carriedThe root cause of #5072 is a behavioral artifact defect, not a code defect: the def described a tool's flags instead of showing the call, so a seat following it invented
Skill hygiene — unchanged at this headPortability (no home, absolute, machine-local or sibling-repo paths; the shim resolves through the plugin-root variable with a repo-root fallback), REST-only reads, and the bash-3.2 shell shape (function body with Documentation consistency also re-confirmed: six stale "usage error (1)" references across the formats contract and the three READMEs read 4 at head, and the Deviation disclosure (§DEV) — the row that flipped
Two non-blocking nits — restated, still not blockingUnchanged from the prior head, since the def is byte-identical. Neither holds this PR:
Read the PR head (§HEAD)Both skills artifacts sourced from Verdict: PASS (advisory). The deviation-disclosure row is clean and the skills substance is provably the same tree I passed at review-skill does not merge. PR #5091 is §CP-blocking and needs a Verdict-written: 2026-08-09T08:39:15Z |
The crew engine's approval-watcher asked
pipeline-cli cp-cardinality decidefor a §CP verdict using two flags that verb has never accepted. The CLI refused the call and exited 1 — which is also the verb's word for "stop" — so the watcher wrote downdefinite-stop:no approval at current headfor four §CP PRs that were, in fact, approved at their exact heads. This PR makes a refused call exit on a code no verdict uses, and replaces the prose that invented the flags with the actual, runnable call.It fails closed either way (nothing merges wrongly). What it fixes is the silent permanent stall, and a ledger that confidently recorded the opposite of the truth.
What changed
The CLI — a refused invocation gets its own exit code.
packages/pipeline-cli/src/exit-codes.ts: newBAD_INVOCATION_EXIT_CODE = 4, deliberately the same number asSTDIN_READ_FAILED_EXIT_CODE. Both mean "the tool never got usable input, so it never decided" — a caller's exit table wants one never-ran band, not a new integer per way of failing to start.packages/pipeline-cli/src/run.ts: the router catches effect-cli'sCliErrorand exits 4 instead of lettingrunMainland on 1. This is router-wide on purpose:1is a verdict for more than one verb, so the collision had to be removed at the one place every parse error passes through.packages/pipeline-cli/src/tools/cp-cardinality/command.test.ts(new): spawns the real bin and pins the bad-input code as neither0nor1, plus non-regression on the two verdict codes.The agent def — a literal call instead of a description of one.
claude-plugins/pipeline-crew/agents/crew-engineering-manager.md's approval-watcher tick block now carries the discharge, written out:--non-author-approval-at-headitself (exact-login match against an active control-plane member,.commit_id == $HEAD, non-author), and--self-approval-at-headin the sole-owner shape only;jq—gh api --jqtakes no--arg, and that misuse errors to stderr while yielding an empty string a caller reads as "no approvers". An empty or failed read routes tounknown:, never to a definite "no approval";cp-read.sh team-membershiprelay, so an unreadable probe stays UNKNOWN rather than under-counting into a stop;0fired,1definite stop, anything elseunknown:— so a bad-input exit can no longer be transcribed asdefinite-stop;The contract + the stale exit-code prose.
gh-issue-intake-formats.md§CLI gains a row for exit 4 and states the rule this bug is an instance of: read a verdict off a verb's own enumerated codes, and when a def tells an agent to call a verb, show the literal invocation — prose that describes an interface drifts from the shipped one silently, and the drift lands as a false verdict instead of a visible error. Six other places that said "usage error (1)" are corrected to 4.Run evidence
The tick block's discharge, run verbatim against the four PRs the false records name:
The old call, before and after the CLI change:
Local checks:
pnpm typecheck --force(30/30, uncached),pnpm lint:worktreeclean,pipeline-clisuite 3055/3055 across 183 files,cli-invocation-guard checkclean over the 399-file plugin corpus,gh-phoenix lint-skillsclean.Ledger #4753
The four false
definite-stoprecords are annotated in place by a follow-up comment — appended, never rewritten. The false tick is the evidence; deleting it would erase the only trace of the defect.Deviations
cp-cardinality decidecall can no longer be read as a definite verdict. Did: only the crew-EM approval-watcher tick block was reseated.ship-it'sstep0-cp-approval.shstill reads the discharge asif … then discharge else stop, so a future bad invocation there would land on the stop branch — and its stop message asserts it "states a fact", which exit 4 now makes falsifiable. Why: it cannot fire today (its flags are correct), and reseating a §CP merge-gate script is its own change, not a ride-along on this one. Disposition: follow-up ship-it Step-0 §CP approval script reads any non-zero cp-cardinality exit as a definite stop #5098 filed, carrying the falsifiable-"states a fact" note and the fix shape (enumerate 0 / 1 / else, mirroring the block this PR wrote).command.test.tsis new; no existing assertion was modified, weakened, or deleted.src/exit-codes.tsandsrc/run.tsare the router-wide seat the issue's fix shape implies.Control plane
This touches
claude-plugins/**andpackages/pipeline-cli/src/tools/cp-cardinality/, so expect §CP classification. The classifier at head reports 8 of the 11 changed files as control-plane-owned.Fixes #5072