Skip to content

fix(pipeline-cli): a refused invocation is not a verdict — bad flags exit 4, not stop (#5072) - #5091

Merged
usirin merged 2 commits into
mainfrom
usirin/fix-5072-cp-cardinality-bad-input-exit-5FD3FF4A
Aug 9, 2026
Merged

usirin merged 2 commits into
mainfrom
usirin/fix-5072-cp-cardinality-bad-input-exit-5FD3FF4A

Conversation

@usirin

@usirin usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member

The crew engine's approval-watcher asked pipeline-cli cp-cardinality decide for 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 down definite-stop:no approval at current head for 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: new BAD_INVOCATION_EXIT_CODE = 4, deliberately the same number as STDIN_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's CliError and exits 4 instead of letting runMain land on 1. This is router-wide on purpose: 1 is 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 neither 0 nor 1, 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:

  • it derives --non-author-approval-at-head itself (exact-login match against an active control-plane member, .commit_id == $HEAD, non-author), and --self-approval-at-head in the sole-owner shape only;
  • the approver read pipes into real jqgh api --jq takes 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 to unknown:, never to a definite "no approval";
  • membership goes through the shared three-way cp-read.sh team-membership relay, so an unreadable probe stays UNKNOWN rather than under-counting into a stop;
  • the exit table enumerates cp-cardinality's codes — 0 fired, 1 definite stop, anything else unknown: — so a bad-input exit can no longer be transcribed as definite-stop;
  • the false sentence "That call resolves the active approver set and the SHA-bound signal flags itself — do NOT re-derive either here" is gone, replaced by what the shipped CLI actually requires of a caller.

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:

#5051 → §CP discharged at d91e9d8fac154319bd78065e3f17f24be7ef0c98 — TICK_NOTES=5051=fired
#5043 → §CP discharged at f00ac85ceccbfc35079d847641df3a1cf213704e — TICK_NOTES=5043=fired
#5037 → §CP discharged at 1fd2cb1736501cb2b6d0c48def7a08d2b77a77e7 — TICK_NOTES=5037=fired
#5014 → §CP discharged at f01b5935e3da1bf8d213f9ed4f2d7ab346e09d7a — TICK_NOTES=5014=fired

The old call, before and after the CLI change:

$ printf 'usirin\nnotusirin\n' | pipeline-cli cp-cardinality decide --pr 5051 --head abc --author usirin
ERRORS
  Unrecognized flag: --pr in command pipeline-cli cp-cardinality decide
  Unrecognized flag: --head in command pipeline-cli cp-cardinality decide
rc=4          # was 1 — the same code as `stop`

Local checks: pnpm typecheck --force (30/30, uncached), pnpm lint:worktree clean, pipeline-cli suite 3055/3055 across 183 files, cli-invocation-guard check clean over the 399-file plugin corpus, gh-phoenix lint-skills clean.

Ledger #4753

The four false definite-stop records 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

  • Known defect left unfixed (class 3)Said: approval-watcher's cp-cardinality call passes flags the CLI rejects; exit 1 recorded as a definite 'no approval' #5072 asks that a refused cp-cardinality decide call can no longer be read as a definite verdict. Did: only the crew-EM approval-watcher tick block was reseated. ship-it's step0-cp-approval.sh still reads the discharge as if … 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).
  • Scope narrowing (class 1) — None.
  • Governing-ADR departure (class 2) — None.
  • Declined guidance (class 4) — None.
  • Guard or gate bypassed (class 5) — None.
  • Pre-existing test or fixture changed (class 6) — None. command.test.ts is new; no existing assertion was modified, weakened, or deleted.
  • Out-of-scope change (class 7) — None. src/exit-codes.ts and src/run.ts are the router-wide seat the issue's fix shape implies.

Control plane

This touches claude-plugins/** and packages/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

…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
@usirin
usirin requested a review from a team as a code owner August 9, 2026 07:42
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

No preview deploy

  • No preview deploy for this PR — its diff touches no deploy-relevant path, so no preview stack was minted and e2e is not applicable. (643d5ac)
  • web — Stage pr-5091 torn down.

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

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 gh-issue-intake-formats.md and the whole packages/pipeline-cli/src/tools/cp-cardinality/ + cp-classify/ set, plus src/exit-codes.ts and src/run.ts. GUARD_TOUCHING empty, ADR_N=0. BLOCKING (human-approval-gated) — confirmed, and broader than the reported single-path match. This verdict is deliberately non-binding: no first-line SHA marker, head bound in the body above (ADR 0111/0151).

Acceptance criteria

  • [PASS] bad-input exit code distinct from the verdict codes; an unrecognized flag no longer exits 1 — I reproduced the defect and the fix on the real bin. At base, decide --pr 5051 --head abc --author usirin with the roster piped in returns rc=1, the verb's own stop. At this head the same call returns rc=4. BAD_INVOCATION_EXIT_CODE = 4 lands in packages/pipeline-cli/src/exit-codes.ts, and the router seats it in src/run.ts.
  • [PASS] a unit test pins the bad-input code as neither 0 nor 1packages/pipeline-cli/src/tools/cp-cardinality/command.test.ts drives the real bin (not the pure core, which never sees the invocation boundary) and asserts the code equals BAD_INVOCATION_EXIT_CODE, is not 0, is not 1, and that neither stop nor discharge appears as a stdout line. I ran it: 21/21 pass across the two cp-cardinality files.
  • [PASS] the tick block carries a literal, runnable discharge using only accepted flags, roster on stdin, no --pr/--head — line 442 of crew-engineering-manager.md is the only cp-cardinality invocation, passing --author plus the two conditional signal flags with the roster piped in. I searched the whole def: no --pr or --head on that verb anywhere.
  • [PASS] the block derives --non-author-approval-at-head itself; the false sentence is gone — steps 6 and 7 derive both signals. Searching the def at head for the old "do NOT re-derive" sentence returns nothing; it is replaced by a correct statement that what is re-used is the DECISION, not the signals.
  • [PASS] the derivation pipes into real jq; an empty or failed read routes to unknown: — the reviews payload is piped into real jq with a proper --arg, and a non-zero jq status routes to the unknown branch. Correctly, an empty result from a successful read still routes to the definite stop — that is a genuine "no approver at head". The old false-empty is now impossible, because real jq exits non-zero where gh api --jq --arg silently yielded an empty string.
  • [PASS] the exit table enumerates the codes and maps anything outside a verdict code to unknown: — the case statement reads 0 as fired, 1 as the definite stop, and everything else as unknown. A 4 can no longer be transcribed as definite-stop.
  • [PASS] the four false ledger records are annotated in place, appended never rewritten — comment 5230417071 on approval-watcher tick ledger #4753 has created_at == updated_at (unedited). The false tick 5230210599 still carries all four definite-stop rows verbatim. Its one edit (06:44:18Z to 06:49:25Z) is the ledger tool's own rolling upsert during the live session (two ticks, firstAt to lastAt), and it predates the bug report at 06:53:21Z — so it is not this PR's doing. Nothing was deleted.
  • [PASS] a reader of approval-watcher tick ledger #4753 can tell which tick lied and why, from the ledger alone — the annotation names comment 5230210599 as the false one, tables the true state per PR with full head SHAs, explains the exit-1 collision with stop, and points at comment 5230236014 as the correct fired tick.
  • [FAIL] deviation-disclosure (§DEV) — the PR body carries no ## Deviations heading. Its headings are What changed / Run evidence / Ledger approval-watcher tick ledger #4753 / Not this PR. The PR is write-code-authored (the progress comment on approval-watcher's cp-cardinality call passes flags the CLI rejects; exit 1 recorded as a definite 'no approval' #5072 at 07:43:58Z), so neither §DEV N/A shape applies, and absence fails closed ("Absent is not None."). This is a body-only remedy, not a repair round: the one real deviation — deferring the ship-it step-0 §CP approval script — is already disclosed in substance under "Not this PR", with its why and its disposition. Re-file it under a ## Deviations heading as a class-3 entry (Said / Did / Why / Disposition) and assert None. for the other six classes. No code change, and nothing else in this verdict is contingent on it.

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, decide --author usirin --non-author-approval-at-head prints discharge and exits 0; decide --author usirin prints stop and exits 1.

(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:

  1. The predicate is narrow. CliError.isCliError is a branded-property check against effect-cli's own type id, not a structural or catch-all test. Only effect-cli's own error classes carry that brand.
  2. No pipeline-cli tool can produce one. Searching packages/pipeline-cli/src/ for CliError and UserError, the only match is run.ts itself. No tool constructs CliError.UserError — the one variant a handler could legitimately raise — so today no handler failure satisfies the predicate.
  3. Empirically confirmed. A genuine handler runtime failure keeps its own code: checks read against a nonexistent PR returns rc=2, that verb's typed unknown, not 4. And --help still exits 0 at both base and head — worth checking, because ShowHelp is a CliError and effect re-fails it after printing the help text; the zero-error help path resolves before the catch, so there is no regression there.

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 1

Not 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 exitWith helper in run.ts calls process.exit(1) on two pre-runtime paths that never reach Command.run, and therefore never reach the new catch:

  • an unknown or typo'd tool selector (a dispatch failure), and
  • a tool-load error (a registered tool that will not load, for example an unlinked dep).

Verified: a typo'd tool name returns rc=1, while a typo'd subcommand correctly returns rc=4. Both exitWith paths are "the verb never ran" seated on the exact code that means stop — against the doctrine exit-codes.ts states in this very PR, that a verdict a tool proved must never share an exit code with a failure to invoke. The realistic trigger is a tool rename or a broken install rather than a typo, which is why I read it as narrower than the flag case and out of this PR's scope. The docs added here are narrowly accurate — they claim an unrecognized flag and a typo'd subcommand — so nothing shipped is false.

Verification performed

CI at this head — one pre-existing red, NOT this PR's

44 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 packages/fabrika-cli/ that was renamed. This PR touches neither ADR 0244 nor fabrika-cli — the check fails on every PR right now and the fix is banked as #5087. Shipper: expect this red, do not chase it.

Ruling on the deferred ship-it step-0 §CP approval script — I agree with the author

I read that file at this head. The deferral is right, on three grounds:

  1. It genuinely cannot fire today. Its call passes --author plus the same two conditional signal flags — no --pr, no --head. It also refuses up front unless the shim is executable, so a 127 never reaches the branch either.
  2. Its failure direction is the safe one. Reading the discharge as "if … then discharge else stop" sends a malformed call to stop — it refuses to merge. That is the opposite polarity from the watcher, where the same collision produced a false record and a silent permanent stall.
  3. Reseating a §CP merge gate is itself §CP work. Folding a change to the gate that decides whether §CP PRs may merge into the PR that fixes the watcher would widen this diff's blast radius into the merge path, and it would ride in on this PR's approval instead of earning its own.

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 @kamp-us/control-plane approval at this head, after which ship-it enqueues it (ADR 0135 / 0048).

Verdict-written: 2026-08-09T08:05:55Z

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

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 gh-issue-intake-formats.md §CLI contract. This is the second of two namespaces class-probe named for this diff (has-code, has-skills); the code class carries its own advisory in its own comment.

§CP: BLOCKING. gh-issue-intake-formats.md is one of the named gate-critical skills, so the path clause fires on its own. Non-binding by construction: no first-line SHA marker, head bound above (ADR 0111/0151).

What this class had to get right

The 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 --pr/--head. The code half (exit 4) makes that failure visible; only this half stops it happening again.

  • [PASS] The tick block is literal and runnable, not prose. The discharge is now written out end to end — signal 1 derivation, signal 2 derivation, then the call. I did not take this on faith: I ran the block's derivation verbatim against all four PRs named in the false ledger tick, and it produced fired at each one's exact head (fix(pipeline): give the lane stamp a lifecycle, so a finished lane stops reading as live (#4868) #5051 d91e9d8…, docs(decisions): ADR 0244 — a recorded row's stage key is provenance, not a pointer into STAGES (#4977) #5043 f00ac85…, fix(ci): skill-gh-lint walks every plugin dir, and reds when it does not (#5004) #5037 1fd2cb1…, fix(pipeline-cli): the subprocess-budget guard scans every workspace member, not just its own package (#4858) #5014 f01b593…). A def whose block I can lift and run is exactly the property that was missing.
  • [PASS] The false instruction is gone, and it was replaced rather than merely deleted. Searching the def at head for "do NOT re-derive" returns nothing. In its place is a correction that draws the right line — what is single-sourced is the decision, not the signals — which is the distinction whose absence caused the bug. Deleting the sentence alone would have left a reader with no model of why the call takes booleans.
  • [PASS] The call matches the shipped interface. Only --author, --non-author-approval-at-head, --self-approval-at-head, roster on stdin. No --pr, no --head on that verb anywhere in the def.
  • [PASS] Every helper the block calls actually exists in the shape it calls it. This is where a "literal" block can still lie. I checked the one non-obvious call — the shared cp-read helper, invoked in executed mode as <ORG> team-membership <login> — and confirmed it dispatches that subcommand and emits the CP_MEMBERSHIP= line the block pattern-matches on. I also confirmed the helper path the block derives from the shim location resolves under both shim layouts in use.
  • [PASS] Three-way membership handling, not two-way. An unreadable membership probe sets an unknown flag and the scan continues, so a later approver proving active still discharges honestly; only an all-unknown scan with no positive routes to unknown:. That is the same shape ship-it's step-0 script uses, and it is the difference between "no approval" and "could not tell".
  • [PASS] The exit table is enumerated, and the def now teaches the general rule. 0 fired, 1 definite stop, everything else unknown:. The formats §CLI section gains a row for exit 4 plus the standing 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.

Skill hygiene

  • Portability — no home, absolute, machine-local or sibling-repo paths. The shim resolves through the plugin-root variable with a repo-root fallback, and the shared helper is derived from the shim's own location rather than hardcoded.
  • REST only — every read in the block is gh api REST. No GraphQL.
  • Shell shape — the block is a function body using return, consistent with the surrounding tick harness; no errexit, and no EXIT trap. CI's "no errexit + EXIT trap in one shell unit" and "no bare pipeline-cli invocation in runnable shell" are both green at this head, as are "validate skill frontmatter", the skill-corpus GraphQL/frontmatter/path lint, and "lint crew corpus for inline re-derivations of tool-owned decisions" — that last one matters here, since the change adds derivation to a def, and it confirms the added derivation is signal-shaped, not a second copy of the decision.
  • Single-sourcing is preserved, and this is the subtle one. Adding a derivation to a def could easily fork a second copy of the §CP discharge. It does not: the block derives inputs and hands the decision to cp-cardinality, and the def says so explicitly. The one thing now duplicated between this def and ship-it's step-0 script is the signal derivation, and the def names that pairing and asks for the two to be kept in step. That is the honest disposition — the alternative (a shared script) is a bigger change than this bug warrants.
  • Documentation consistency — six stale "usage error (1)" references across the formats contract, the pipeline-cli README, the cp-cardinality README and the cp-classify README are corrected to 4. I read each: they are accurate at head, and the cp-classify README's exit table now correctly shows a bad flag at 4 rather than 1.

Two non-blocking nits

Neither blocks; both are worth a moment if the body is being edited anyway.

  1. The decide call suppresses stdout and stderr, so on a refused call the "unrecognized flag" diagnostic never reaches the operator — only unknown:cp-cardinality decide exit 4 lands in the tick record. That is still correct and unambiguous, and it is a large improvement on a silent false definite. But given the whole point of this fix is making a refused call visible, letting stderr through would make the next drift self-explaining rather than requiring a re-run.
  2. The approver loop iterates with unquoted word splitting where ship-it's step-0 script reads line by line. Safe in practice — GitHub logins contain no whitespace — but it is a small divergence from the reference caller the def points at, in a block whose value is that the two stay in step.

Deviation disclosure

  • [FAIL] deviation-disclosure (§DEV) — the PR body has no ## Deviations heading. Recorded identically on the code-class advisory: §DEV is single-sourced precisely so two gates do not render opposite rows on the same head. The PR is write-code-authored, so neither N/A shape applies and absence fails closed. The substance of the one real deviation (deferring the ship-it step-0 reseat) is disclosed under "Not this PR"; the remedy is to move it under a ## Deviations heading as a class-3 entry and assert None. for the other six classes. Body-only.

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 @kamp-us/control-plane approval at this head, after which ship-it enqueues it (ADR 0135 / 0048).

Verdict-written: 2026-08-09T08:07:17Z

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-code: advisory — blocking-set PR (§CP — approval-gated)

PR #5091 is §CP — it touches the control plane (a gate-critical skill, gh-issue-intake-formats.md, plus the CODEOWNERS-owned cp-cardinality/ and cp-classify/ tool set — ADR 0053/0065). My verdict is advisory only: it does not authorize a merge. Under the §CP hard gate (ADR 0135), a @kamp-us/control-plane member approves this at its current head and ship-it then enqueues it (ADR 0048 single merge authority) — there is no human hand-merge in the §CP path.

Reviewed-head: @ 643d5ac

Re-gate at a new head. Verdict: PASS (advisory). The prior gate at 4f5521177a FAILED on exactly one row — the missing ## Deviations section — and passed everything else. That row is now clean, and I proved the substance is untouched rather than assuming it.

The head is a merge — and it dragged in nothing

643d5ac1 is a merge of 4f5521177a (the reviewed tree) and 22a48864 (the #5087 doc-links fix on main). Two decisive checks, both empty-is-the-answer:

  • The reviewed content is byte-identical. git diff 4f5521177a 643d5ac1 restricted to all 11 changed files returns nothing — every blob is the same object at both heads. No code, test, README or def content moved in the repair round.
  • The merge added nothing of its own. git diff 22a48864 643d5ac1 (main-side parent vs head) returns exactly those same 11 files, 253 insertions / 45 deletions. So the head is main plus the reviewed change and nothing else — no smuggled edit, no conflict resolution that rewrote a hunk.

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 CliError.isCliError branded-check analysis, the ledger annotation).

Class routing, re-derived

class-probe classify --namespaces over the live changed-file set: 11 files → has-code, has-skills → review-code + review-skill. No review-doc — the three READMEs sit under packages/, which §CLASS's has-docs exclude carves out — and no review-design (nothing under the UI path set). Both required namespaces carry a verdict at this head; this is the code half.

§CP classification, re-run not inherited

The classifier at this head reports CONTROL_PLANE_TOUCHED non-empty over 8 of the 11 changed files: gh-issue-intake-formats.md, src/exit-codes.ts, src/run.ts, and the whole cp-cardinality/ (README, command.ts, command.test.ts) plus cp-classify/ (README, command.ts) set. GUARD_TOUCHING='', ADR_N=0, CP_FILES_N=11. BLOCKING (approval-gated) — same result as the prior gate, confirmed independently.

The one row that changed: deviation-disclosure (§DEV)

  • [PASS] deviation-disclosure — the body now carries a ## Deviations heading and it is well-formed. The heading set reads What changed / Run evidence / Ledger approval-watcher tick ledger #4753 / Deviations / Control plane; the old non-canonical Not this PR heading is gone, its substance re-filed under the canonical one. Judged against §DEV's rules rather than counted:
    • The class-3 entry is in full Said / Did / Why / Disposition shape and its substance matches the deferral the prior gate saw: ship-it's step0-cp-approval.sh still reads the discharge two-way, so a future bad invocation there lands on the stop branch. §DEV's disclosed-branch test asks three questions and this answers all three — authorized (reseating a §CP merge-gate script is its own change, not a ride-along), needs no ADR (class 3, not class 2), and has a filed follow-up.
    • The other six classes each assert None., which is what makes None. a checked claim against a closed list rather than a shrug. Classes 6 and 7 carry a one-clause justification (command.test.ts is new, so no existing assertion was weakened; exit-codes.ts and run.ts are the router-wide seat the issue's own fix shape implies) — both correct against the diff I read.
    • Repair-appends discipline held: the disclosure was moved under the canonical heading, not rewritten to a different story, and nothing in the earlier trail was destroyed.
    • One note, disclosed rather than held against the PR: the cited follow-up ship-it Step-0 §CP approval script reads any non-zero cp-cardinality exit as a definite stop #5098 is now closed as a duplicate of step0-cp-approval.sh reads its §CP discharge as a two-way if/else #5094, which is open and carries the same fix shape. The disposition therefore still resolves to live tracked work through the duplicate chain. Not a finding.
  • [PASS] §DEV Tier-M scan at this head — section present; 0 removed-assertion lines; 1 suppression/skip hit, and it is a scanner false positive, not a deviation. The flagged line is Effect.sync(() => process.exit(BAD_INVOCATION_EXIT_CODE)), in src/run.ts, which matches the scan's xit\( alternative on the substring inside pro­cess.e·xit(. It is the PR's own subject line, not a skipped test. A hit is a line to judge, never a FAIL on its own — judged, and dismissed. (Worth a scanner follow-up; not this PR's debt.)

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 head

Verified at 4f5521177a one criterion at a time and re-bound here on proven blob identity:

  • [PASS] bad-input exit distinct from the verdict codes; an unrecognized flag no longer exits 1 — reproduced on the real bin: rc=1 at base, rc=4 at head. BAD_INVOCATION_EXIT_CODE = 4 in src/exit-codes.ts, seated router-wide in src/run.ts.
  • [PASS] a unit test pins the code as neither 0 nor 1cp-cardinality/command.test.ts spawns the real bin and asserts exactly that, plus non-regression on both verdict codes.
  • [PASS] the tick block carries a literal, runnable discharge using only accepted flags--author plus the two conditional signal flags, roster on stdin; no --pr/--head on that verb anywhere in the def.
  • [PASS] the block derives --non-author-approval-at-head itself; the false "do NOT re-derive" sentence is gone — replaced, not merely deleted, by a correct statement of what is single-sourced (the decision) versus what is not (the signals).
  • [PASS] the derivation pipes into real jq; an empty or failed read routes to unknown: — a non-zero jq status routes to unknown, while an empty result from a successful read still routes to the definite stop, which is the honest reading.
  • [PASS] the exit table enumerates the codes; anything outside a verdict code maps to unknown: — 0 fired, 1 definite stop, everything else unknown. A 4 can no longer be transcribed as definite-stop.
  • [PASS] the four false ledger records are annotated in place, appended never rewritten — the false tick still carries all four rows verbatim; the annotation is a separate, unedited comment.
  • [PASS] a reader of approval-watcher tick ledger #4753 can tell which tick lied and why, from the ledger alone — the annotation names the false comment, tables the true state per PR with full head SHAs, and points at the correct fired tick.

CI and evidence at this head

Run-evidence bundle: PRESENT for head 643d5ac1 — producer run 31302929752, artifact 9035095935, manifest.commit == head, schemaVersion 1; checks 2/2 pass; tests 2420/2420 passed, 0 failed, 0 skipped.

44 check runs: 40 green, 4 conditionally skipped, 0 failures. Two I read specifically because the merge changed their inputs:

  • check docs have no dead internal links is now GREEN. This was the prior gate's one pre-existing red (ADR 0244 citing a renamed fabrika-cli path). Merging 22a48864 cleared it. The shipper no longer has a red to expect.
  • scan PR commits for secrets is GREEN at this head. This one walks merge-base..HEAD commits, not the tree, so the merge genuinely changed its scan set — re-read rather than inherited. Clean.

Also green and relevant: check §CP paths are all owned in CODEOWNERS, lint / format / typecheck, packages unit tests, no errexit + EXIT trap in one shell unit, no bare pipeline-cli invocation in runnable shell, check no unaccounted unresolved review thread reaches merge-ready.

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 643d5ac1f341ec21f81ba98a22d04dcd45ffb603 via a per-run ref (git fetch origin pull/5091/head into refs/pr/5091-regate, asserted equal to the live head) and read with git show. No working tree was switched, reset or checked out; the launched checkout's working copy was never the source.

Non-blocking, carried forward — not re-raised

Both already have tickets and neither holds this PR: #5094 (the step0-cp-approval.sh two-way read, the class-3 disclosure's subject) and #5097 (run.ts's exitWith still exiting 1 on two pre-runtime paths — the same defect class one branch over, out of this PR's scope and not made worse by it).


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 @kamp-us/control-plane approval at 643d5ac1f341ec21f81ba98a22d04dcd45ffb603, after which ship-it enqueues it (ADR 0135 / 0048). Merging will auto-close #5072 via Fixes #5072.

Verdict-written: 2026-08-09T08:38:24Z

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-skill: advisory — blocking-set PR (§CP — approval-gated)

PR #5091 is §CP — gh-issue-intake-formats.md is one of the named gate-critical skills, so the path clause fires on its own (ADR 0053/0065). My verdict is advisory only: it does not authorize a merge. Under ADR 0135 a @kamp-us/control-plane member approves this at its current head and ship-it then enqueues it (ADR 0048 single merge authority).

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 gh-issue-intake-formats.md §CLI contract. This is the second of the two namespaces class-probe named for this diff (has-code, has-skills); the code class carries its own advisory in its own comment.

Why this is a re-bind, not a re-review

The prior gate at 4f5521177a FAILED on exactly one row — the absent ## Deviations section — and passed the whole skills substance. The repair round touched only the PR body, and the head moved solely because the branch was updated onto new main to clear a repo-wide doc-links break.

I proved that rather than taking it on report. 643d5ac1 is a merge of 4f5521177a and 22a48864; git diff between the two heads restricted to all 11 changed files is empty, so both skills artifacts (crew-engineering-manager.md, gh-issue-intake-formats.md) are the same blob objects I passed before. And git diff 22a48864 643d5ac1 returns exactly those 11 files and nothing more — the merge smuggled in no edit of its own. So the behavioral artifacts are untouched and every finding below stands at this head.

What this class had to get right — all PASS, carried

The 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 --pr/--head. The code half (exit 4) makes that failure visible; only this half stops it happening again.

Skill hygiene — unchanged at this head

Portability (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 return, no errexit, no EXIT trap) all hold. The corpus lints confirm it at this head: validate skill frontmatter, the skill-corpus GraphQL/frontmatter/path lint, no errexit + EXIT trap in one shell unit, no bare pipeline-cli invocation in runnable shell, and — the one that matters most for a change that adds derivation to a deflint crew corpus for inline re-derivations of tool-owned decisions, all green. Single-sourcing is preserved: the block derives inputs and hands the decision to cp-cardinality, and the def says so.

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 cp-classify README's exit table is correct.

Deviation disclosure (§DEV) — the row that flipped

  • [PASS] deviation-disclosure — the body now carries a canonical ## Deviations heading (headings read What changed / Run evidence / Ledger approval-watcher tick ledger #4753 / Deviations / Control plane). The one real deviation — deferring the step0-cp-approval.sh reseat — is filed as a class-3 entry in full Said / Did / Why / Disposition shape, and the other six classes each assert None., with classes 6 and 7 carrying a one-clause justification rather than a bare assertion. §DEV's disclosed-branch test passes on all three questions: authorized (reseating a §CP merge-gate script is its own change), no ADR needed (class 3, not class 2), and a follow-up is filed. The cited ship-it Step-0 §CP approval script reads any non-zero cp-cardinality exit as a definite stop #5098 is now closed as a duplicate of the open step0-cp-approval.sh reads its §CP discharge as a two-way if/else #5094, which carries the same fix shape — the disposition still resolves to live tracked work. Not a finding.

    Recorded identically on the code-class advisory. §DEV is single-sourced precisely so two gates cannot render opposite rows on the same head.

  • The §DEV Tier-M scan's single class-5 hit is a scanner false positive (process.e·xit( matching the xit\( alternative on a run.ts line) — judged and dismissed, detailed on the code-class advisory.

Two non-blocking nits — restated, still not blocking

Unchanged from the prior head, since the def is byte-identical. Neither holds this PR:

  1. The decide call suppresses stdout and stderr, so a refused call's "unrecognized flag" diagnostic never reaches the operator — only unknown:cp-cardinality decide exit 4 lands in the tick record. Correct and unambiguous, and a large improvement on a silent false definite; letting stderr through would make the next drift self-explaining.
  2. The approver loop iterates with unquoted word splitting where ship-it's step-0 script reads line by line. Safe in practice (logins carry no whitespace), but a small divergence from the reference caller the def points at.

Read the PR head (§HEAD)

Both skills artifacts sourced from 643d5ac1f341ec21f81ba98a22d04dcd45ffb603 via a per-run ref (refs/pr/5091-regate, asserted equal to the live head) and read with git show. No working tree was switched, reset or checked out.


Verdict: PASS (advisory). The deviation-disclosure row is clean and the skills substance is provably the same tree I passed at 4f5521177a. This half remains the strongest part of the PR: it fixes the actual root cause rather than only the symptom, and its block is genuinely runnable — I ran it.

review-skill does not merge. PR #5091 is §CP-blocking and needs a @kamp-us/control-plane approval at this head, after which ship-it enqueues it (ADR 0135 / 0048).

Verdict-written: 2026-08-09T08:39:15Z

@usirin usirin added the status:cp-banked Control-plane PR banked on the board, awaiting a control-plane approval label Aug 9, 2026
@usirin
usirin requested a review from notusirin August 9, 2026 08:40
@usirin
usirin added this pull request to the merge queue Aug 9, 2026
Merged via the queue into main with commit b757a65 Aug 9, 2026
46 checks passed
@usirin
usirin deleted the usirin/fix-5072-cp-cardinality-bad-input-exit-5FD3FF4A branch August 9, 2026 08:56
@github-actions github-actions Bot mentioned this pull request Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:cp-banked Control-plane PR banked on the board, awaiting a control-plane approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

approval-watcher's cp-cardinality call passes flags the CLI rejects; exit 1 recorded as a definite 'no approval'

2 participants