fix(pipeline): the required review set can no longer shrink in silence (#4520, #4730) - #4742
Conversation
No preview deploy
|
|
review-code: advisory — blocking-set PR (§CP — approval-gated) PR #4742 is §CP — it touches the control plane ( Reviewed-head: @ 326721b Verified against #4520's acceptance criteria, one at a time, against the PR head in an isolated worktree — all pass: Acceptance criteria — #4520
Checks — my own runs at this head, not a citation of the PR's
Run-evidence bundle: PRESENT for head The narrowing — checked, and the body does not overstate it The PR body states the limit correctly and I verified each half independently: Advisory findings — none blocking; recorded for the control-plane approver
My verdict authorizes nothing. A Verdict-written: 2026-08-02T07:17:48Z |
|
review-skill: advisory — blocking-set PR (§CP — approval-gated) PR #4742 touches the control plane (a gate-critical skill: Reviewed-head: @ 326721b Verified against #4730's acceptance criteria + the skill-rigor checklist, at the PR head — all checks pass: Acceptance criteria — #4730
Skill rigor
Advisory findings — none blocking; recorded for the control-plane approver
My verdict authorizes nothing. A Verdict-written: 2026-08-02T07:17:59Z |
#4520, #4730) Two ends of one too-small required-review set, on the merge-authorization path. `verdict gate --require` was a single-valued `Flag.string`, so a repeated flag kept only the first occurrence and dropped the rest without a word: `--require review-code --require review-doc` cleared PR #4724 as enqueueable while `--require review-code,review-doc` refused on an absent `review-doc` — the same required set, opposite answers, decided by spelling. The `parseRequired` guard one layer below already refuses to shrink the conjunction; it never saw occurrence 2..n, so it was born dead rather than drifted. The flag is now `Flag.atLeast(1)` and every occurrence unions in, which also routes a later occurrence's bogus token back into that guard. Beside it, a coverage assertion: an affirmative gate answer must cover as many distinct namespaces as argv asked about. That is the general form — it refuses any future path that answers about fewer things than it was asked, which is the shape of this whole class (a check that runs, exits clean, and returns a plausible value that is simply smaller). Its operands come from two origins on purpose: the CLI's parse of argv against the decision returned over the service boundary. ship-it Step 0 re-derived the artifact-class set instead of delegating to `class-probe`, and its copy could only ever answer with a SUBSET: a file matching none of the three predicates produced silence, and silence subtracts a class. On PR #4724's list it printed `has-skills` alone where the probe printed `has-code, has-skills` — the plugin-docs-beside-skills shape any plugin home reproduces. The #2765 no-class rule lived there as five comment lines and zero executable ones. Step 0 now prints `class-probe`'s answer, which is what #2765's own commit said it was always meant to do, and the no-class rule lands as an executable fail-closed branch keyed on the §CPREAD file count against the probe's stdout. Scope note: the executed gate was not weakened by the Step-0 half alone — Step 2 re-derives the set from the CLI and no executable consumer of `step0-classify` exists. The exposure was the prose contract telling an agent to carry Step 0's set forward, and turning it live took a second deviation, which is the `--require` defect. Neither half demonstrates an un-gated merge on its own; the pair is the urgency. In-place edit to the frozen v1 `claude-plugins/kampus-pipeline/` corpus under the founder's #4650 No-go exception, scoped to this defect only (recorded on #4730). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… invocation `cli-invocation-guard` scans a `.sh` file as one implicit runnable fence, so the CLI's name in a leading word position — even inside an `echo`'d diagnostic that executes nothing — matches BARE_INVOCATION and reds the gate. Lead with what is wrong instead, leaving the single-source shim path in the `/`-prefixed position the matcher excludes; the message keeps the path and gains the remedy. The resolution itself is untouched: PCLI is already resolved by path and called as "$PCLI", the canonical shape §CLI asks for. #4432 owns removing the need for this reword. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
326721b to
0a875ac
Compare
|
review-code: advisory — blocking-set PR (§CP — approval-gated) PR #4742 is §CP — it touches the control plane ( Reviewed-head: @ 0a875ac Round 4 — body-only re-gate at the same head. This clears.The head has not moved. Why the form returns to the advisory. Round 3 emitted a What round 3 held on, and its dischargeRound 3 passed every acceptance criterion of #4520 and #4730 and failed on exactly one row: Discharged, and verified against the issues themselves rather than against the body's say-so. All five exist, are
None is a thin ticket. Each is actionable cold by a stranger: file, line, quote, why-it-matters, and a named fix shape. That is the bar the disclosure had to reach, and it reaches it. The two advisory findings round 3 carried are now folded in as entries 6 and 7 — #4743 (merge-boundary class resolution reads the local file rather than The splice was additive — checked, not accepted
The tree and CI are where they were
Carried forward from rounds 1–3 — the tree is byte-identical, so these standNot re-derived, and not re-asserted as fresh work: every acceptance criterion of #4520 (the Sub-gates
A Overall — clearEvery acceptance criterion of #4520 and #4730 passes. The body edit is verifiably additive and the narrowing survived it intact. The three disclosed class-3 defects are filed as #4745 / #4746 / #4747, each substantive enough to act on cold, with #4743 / #4744 folded in as non-blocking entries with durable homes. The tree is unchanged and CI is genuinely green at 46 of 46 received. Nothing is outstanding on the This authorizes nothing. A Note on tooling, not a finding: Verdict-written: round 4, body-only re-gate at 0a875ac |
|
review-skill: advisory — blocking-set PR (§CP — approval-gated) PR #4742 is §CP — it touches the control plane ( Reviewed-head: @ 0a875ac Round 4 — body-only re-gate at the same head. This clears.The head has not moved. Why the form returns to the advisory. Round 3 emitted a The skills class on this PRTwo of the five changed files are behavioral artifacts under
The behavioral verification of both — the delegation, the no-class rule landed as executable code with two independent origins ( What this round re-checked
The body edit — additive, and the narrowing survived
Sub-gates
Overall — clearThe skills class is verified: Step 0 delegates rather than re-implements, the no-class rule is executable and can fire, the harness is measurably discriminating, the drift anchor is untouched, and This authorizes nothing. A Note on tooling, not a finding: Verdict-written: round 4, body-only re-gate at 0a875ac |
3 participants
Fixes #4520
Fixes #4730
#4729 is a duplicate of #4520 and is NOT closed by this PR — see Loose end at the bottom.
Two ends of one too-small required-review set, on the merge-authorization path. They land
together because neither half is the whole hazard.
Part A —
verdict gatedropped a namespace on a repeated--require(#4520)--requirewas declaredFlag.string("require")— single-valued. A repeated flag kept thefirst occurrence and discarded the rest in silence, and the gate still answered
enqueueable: true, exit 0.Reproduced against PR #4724 at its merged head, where
review-dochas no verdict at all:--require review-code --require review-docenqueueable: true, exit 0 —review-docnever mentioned--require review-code,review-docenqueueable: false, exit 1 — refuses on the absentreview-docThe identical required set, opposite answers, decided by spelling alone. Both prior reports
happened to test with every named namespace passing, which is why it stayed invisible.
The guard that should have caught it was already there and was structurally unreachable.
parseRequiredrefuses an unrecognized token with "Refusing to drop it from the required set(that would shrink the gate conjunction)" — its docblock names shrinking the conjunction as the
exact fail-open this verb exists to prevent. It only ever received one occurrence's string,
so for occurrence 2..n it never ran. Born dead, not drifted. So this is not "add a guard".
The fix: UNION, via
Flag.atLeast(1)Grounded in the flag API, not memory —
effect/unstable/cli'sFlagexposesatLeast/atMost/betweenunder@category repetition, andatLeast(1)turnsFlag<string>intoFlag<ReadonlyArray<string>>collecting every occurrence while keeping the flag mandatory(
packages/pipeline-clipinseffect@4.0.0-beta.92; the combinator delegates toParam.atLeast).Union over usage-error because union is the shape that makes the two spellings the same
input rather than one legal and one rejected — the caller's intent is unambiguous, and a usage
error would break every existing correct repeated-flag caller for no safety gain. A later
occurrence's bogus token now reaches the existing token guard instead of vanishing upstream of
it, which is the property that guard was written for.
Verified live on this branch, both spellings against PR #4724:
The independent check —
coverageDefectAn affirmative gate answer must be about as many distinct namespaces as argv asked about; if
decisions.lengthand the parsed distinct set disagree, the pass is refused and the reason nameswhat was never decided.
This is the part that outlives the spelling. Union-or-error fixes this path; the assertion
refuses any future path that answers about fewer things than it was asked — the general form
of a defect whose signature is a plausible value rather than an error: it runs, exits clean,
returns a well-formed set, just a smaller one, so every natural guard (did it run? exit 0? return
a set?) is satisfied by the wrong answer.
Its operands come from two different origins deliberately: the CLI's own parse of argv
against the decision returned over the
Githubservice boundary. An assertion that re-deriveddecisionsfromrequiredGatesinsidedecideGatewould be true by construction and couldnever fire. It is applied to a pass only — a refusal already refuses, and its own reason is
the more useful one.
Part B — ship-it Step 0 under-reported the class set (#4730)
step0-classify.shcarried a hand-rolled reimplementation of the §CLASS probes: threegrep -Eqclass tests plus a UI test, over four re-resolved boundary regexes. A changed filematching none of the three produced no output at all, and
class-probedoes the opposite incode — an unclassified file rides
has-code. So the shell could only ever answer a strictsubset, never a superset. The #2765 no-class rule sat in that file as five comment lines and
zero executable ones.
Step 0 now prints
class-probe classify's output. That is not a redesign — the commit thatclosed #2765 states the rule "lives once in the shared class-probe core that ship-it Step 0 and
the reviewer fan both run", i.e. Step 0 was always meant to delegate. It is also the verb Step 2
already re-derives the required set from, so Step 0's printed set and Step 2's enforced set are
now identical by construction instead of by matched maintenance. Two implementations of a
merge-gating question is worse than either bug.
The no-class rule landed as executable code, not a second comment. It is a real guard, not a
restatement: it compares the §CPREAD file count (
$CP_FILES_N, already proven ≥ 1 —cp_changed_filesfails closed on a zero-length list) againstclass-probe's stdout. Twoorigins, so it can fire. A non-empty diff whose class set comes back empty prints
BLOCKING+STOPand exits 1 — UNKNOWN, never "no gates required".The regression harness, and why its fixture is the point
claude-plugins/kampus-pipeline/skills/ship-it/scripts/verify-step0-class-parity.sh— hermetic,reviewer-runnable, asserts Step 0's printed class words equal the real
class-probe's overthe same list.
The failure mode is silence and silence subtracts, so a fixture that always classifies as
something proves nothing. Case 1 is therefore the plugin-docs-beside-skills shape — a plugin
home's
README.md+docs/**beside itsskills/**, files that classify as nothing underall three predicates. Case 2 is a diff where every file classifies, kept so a green case 1 is
visibly discriminating rather than the harness being green on everything.
The stub serves the real
gh-issue-intake-formats.md/ship-it/SKILL.mdbytes for the twocontents/reads, not an empty body — an empty body would hand a re-deriving step the.-matches-everything fail-closed sentinel, which over-dispatches and would mask the verysubtraction under test.
Falsifiability, measured rather than argued. The pre-fix script run under this harness's stub
on case 1's fixture prints
has-skillsalone, againstclass-probe'shas-code, has-skills—the harness fails on it. Post-fix it prints
has-code,has-skills, matching #4730's AC2 (alive run against PR #4724's 11-file list gives the same).
The drift anchor is intact
Removing the
HAS_*_RE/UI_REliterals from this script does not remove drift detection.validate-gate-path-drift.shlocks each boundary's canonical (invariants 1b/1d, ingh-issue-intake-formats.md§CLASS andship-it/SKILL.md) against the typed const inpackages/pipeline-cli/src/gate-boundaries.ts; 1c value-locks whatever copies exist. A copy thatno longer exists cannot drift, and the anchor it was checked against is untouched. The guard runs
green on this branch — 34 checks, including
UI_RE/UI_EXCLUDE_REcanonical-appears-once inship-it/SKILL.md, which this PR does not touch.What this PR does NOT claim
The executed gate was not weakened by the Step-0 half alone. ship-it Step 2 re-derives the
required set from the CLI rather than trusting the carried one, and no executable consumer of
step0-classifyexists. The exposure was the prose contract telling an agent to carry theclass set forward, and turning it live needed a second deviation — which is Part A.
So the pair is the urgency; neither half alone demonstrates an un-gated merge end to end.
That is why #4730 priced
p1and #4520p0. This PR does not claim either half "would havemerged a FAIL" on its own.
The reading contract in
ship-it/SKILL.md("carry the class set into Step 2") is left unchangedand is now correct as written, because Step 0 prints the same verb's answer Step 2 re-derives
from. That also keeps this PR inside the freeze exception's scope.
Scope, freeze exception, and §CP
claude-plugins/kampus-pipeline/skills/ship-it/runs under the founder's explicit fabrika execution core: the ~16 pipeline skills rebuilt skill-first, contract-driven #4650 No-go exception recorded on ship-it Step 0 shell probe under-reports the class set vs class-probe #4730, scoped to this
defect only — not a general licence to edit v1. Nothing outside the Step-0 classifier and its
new sibling harness is touched in that tree.
pipeline-cli cp-classify classify --files-file <this PR's file list>returnscontrol-plane [path-match]— BLOCKING (human merge), onclaude-plugins/kampus-pipeline/skills/ship-it/scripts/step0-classify.shmatching the liveCONTROL_PLANE_RE. This PR needs a@kamp-us/control-planeapproval at head and cannotauto-merge on a review PASS.
Verification run on this branch
pipeline-clisuite: 3180 tests / 191 files pass, including the new cases (flag-shapeparity, the repeated-shape acceptance set,
coverageDefect).typecheckclean;biome checkclean;shellcheck -xclean on both scripts.verify-step0-class-parity.sh: OK, 2 cases.verify-chain-resolves.sh: PASS (static, executable, and the$CP_FLAGseam).validate-gate-path-drift.sh: OK, 34 checks.trap-status-guard checkover the plugin corpus: clean (349 files).gh-phoenix lint-skillsover the corpus: clean.verdict gateruns against PR feat(fabrika): the /adr skill and its derived CLI contract — wave-0 pilot (#4704) #4724 in both spellings, as tabled above.Loose end for the dispatcher — #4729 not closed
#4729 (duplicate of #4520) is still open. Closing it is a number-targeting mutation and the
write-code claim guard refused it, verbatim:
I stopped rather than reasoning past a fail-closed refusal. It needs closing as a duplicate with
a pointer to #4520 by whoever holds that lane. It is deliberately not linked with a
Fixeskeyword here, so nothing closes it silently on merge.
Deviations
This section was owed at PR-open (§DEV) and was not written — round 1 rendered no such row, so this
is a skipped step, not a prior
[N/A]. It is added in a body-only repair round; the head has notmoved, so every entry below describes the round-1 diff as it stands.
Class 3 — known defect left unfixed: verdict gate: a repeated --require silently drops namespaces and still reports enqueueable #4729 not closed by this PR. Said: verdict gate: a repeated --require silently drops namespaces and still reports enqueueable #4729 is a
duplicate of verdict gate: a repeated --require flag keeps only ONE namespace, reports enqueueable past a live FAIL #4520, so the lane should end with it closed. Did: this PR left it open, and
disclosed that only as free prose (Loose end, above) rather than here. Why: closing it is a
number-targeting mutation and the write-code claim guard refused it —
{"issue":4729,"mine":false,"reason":"no-winner"}— and a fail-closed refusal is not overridableby reasoning. Disposition: resolved. The dispatching engine has since closed verdict gate: a repeated --require silently drops namespaces and still reports enqueueable #4729 as a
duplicate of verdict gate: a repeated --require flag keeps only ONE namespace, reports enqueueable past a live FAIL #4520. Nothing is outstanding on it; the Loose end paragraph is the round-1 record
and is stale on that point.
Class 3 —
coverageDefectcannot fire against any input reachable today. Said: Part A'sindependent check is presented as the assertion that outlives the spelling fix. Did:
Github.gatepassesrequiredGatesstraight through, anddecideGatebuildsdecisionsfrom aSetover that same array, so the postcondition is identicallynullfor every input reachablethrough the wired call path. Why: it constrains an interface, not a single call frame —
implementations of
Github(test doubles, a future short-circuiting refactor) can violate it.That is what separates it from the born-dead shape Part A fixes: the old
parseRequiredguard wasunreachable within one call frame, because
Flag.stringcould never hand it a secondoccurrence. This is an ordinary unfired postcondition, and it can only refuse, so it carries
zero false-pass risk. Disposition: kept as written, and the genuine gap is named rather than
argued away — nothing in this PR stands the wired call site up against a
Githubdouble, soonly the pure predicate is pinned by tests. Coverage of the wired path is not delivered here.
Class 2 — the
coverageDefectdocblock cites a rule that does not exist. Said: thedocblock attributes a "two different origins" rule to
.patterns/skill-script-shell-shape.md.Did: no such rule is written there, or anywhere else in the repo — the gate grepped
.patterns/and.decisions/for "two different origins" and "two origins" and found zerohits, twice, in two separate review rounds. The citation is wrong, not merely loose.
Disposition: disclosed, not fixed here. Correcting the docblock is a code change; it would
move a head that is fully green and carries two head-bound §CP advisory markers, invalidating both
under ADR 0058 for a comment. A follow-up must either write that rule down as a real pattern or
drop the citation — as it stands the comment points a reader at a document that will not confirm
it. Filed as coverageDefect docblock cites a "two different origins" rule that does not exist in .patterns/ #4745.
Class 3 — the coverage-refusal path still prints
"enqueueable":trueunder exit 1. Said:a gate that refuses should not also emit an affirmative answer. Did: on the
coverageDefectrefusal path stdout still carries
"enqueueable":truewhile the process exits 1. Why: therefusal was added around the existing answer-emitting path instead of restructuring it.
Disposition: left as-is and disclosed with the uncomfortable framing stated plainly — no live
consumer is affected (ship-it Step 2 reads exit status only), but this is this PR's own subject
shape, a well-formed affirmative value that is not the real answer, appearing inside the fix for
it. Filed as verdict gate: the coverageDefect refusal path still prints "enqueueable":true while exiting 1 #4746.
Class 3 / class 1 —
ship-it/SKILL.mddeliberately untouched, with a known stale line left init. Said: the founder's fabrika execution core: the ~16 pipeline skills rebuilt skill-first, contract-driven #4650 No-go exception recorded on ship-it Step 0 shell probe under-reports the class set vs class-probe #4730 is scoped to this defect
only. Did: nothing in
claude-plugins/kampus-pipeline/skills/ship-it/SKILL.mdwas edited —including a line this PR makes stale: the
UI_REfence describes "the classification shell thatconsumes them", and after this change that shell no longer consumes them. Why: editing it
would spend a narrowly-scoped freeze exception on something outside the defect it was granted for.
Disposition: knowingly left alone; it needs its own ticket, under its own exception or after
the freeze lifts. Filed as ship-it/SKILL.md: the UI_RE fence still says a classification shell consumes them — stale after Step 0 delegated to class-probe #4747.
Class 3 — known defect left unfixed: merge-boundary class resolution reads the local file, not
origin/main(Merge-boundary class resolution shifted from origin/main to the local file — the anti-self-authorization read was lost #4743). Said: the docblock inpackages/pipeline-cli/src/gate-boundaries.tsstates the class regexes are re-resolved from
origin/main, so a boundary-editing PR is classifiedagainst MAIN's rules rather than its own — the ship-it/review-code §CP decision runs off the injected skill snapshot — a stale snapshot auto-merged a now-control-plane guard-pkg PR (#830 across the ADR-0100 boundary) #981 anti-self-authorization read. Did:
class-probeparses §CLASS from the local file, and this PR routes Step 0 throughclass-probe, so the verb this PR now delegates to is the one carrying that read; the docblockstill asserts the older behaviour. Why: the shift is pre-existing on
main, not introducedhere. Whether the remedy is restoring the
origin/mainread or correcting the docblock to matchturns on whether ship-it/review-code §CP decision runs off the injected skill snapshot — a stale snapshot auto-merged a now-control-plane guard-pkg PR (#830 across the ADR-0100 boundary) #981's threat model is still live — a decision, and outside this defect's scope.
Disposition: filed as Merge-boundary class resolution shifted from origin/main to the local file — the anti-self-authorization read was lost #4743, left to triage. Surfaced by this PR's review, durably homed,
non-blocking.
Class 3 — known defect left unfixed:
verify-step0-class-parity.shis wired into nothing(verify-step0-class-parity.sh is wired into nothing — a regression harness no job runs #4744). Said: the new harness is described above as hermetic and reviewer-runnable, which it
is. Did: nothing invokes it — no CI job runs it, and its filename appears nowhere in the repo
outside the script itself, while its sibling
verify-chain-resolves.shis a step in.github/workflows/ci.yml. A harness nothing runs cannot fail, so it rots silently while readingas coverage. Why: wiring a CI job is outside the founder's fabrika execution core: the ~16 pipeline skills rebuilt skill-first, contract-driven #4650 freeze exception, which is
scoped to this defect only. Disposition: filed as verify-step0-class-parity.sh is wired into nothing — a regression harness no job runs #4744, left to triage —
reviewer-runnable in the meantime, non-blocking.
🤖 Generated with Claude Code