feat: install the Claude CLI on the monolith shard only, with a fail-loud gate-armed backstop (#1830) - #1962
Merged
The01Geek merged 13 commits intoAug 25, 2026
Conversation
…shard only, with a fail-loud gate-armed backstop
…s-the-claude-cli-on-every-shard
5 tasks
…ard.sh backstop regex ↔ run.sh #671 skip-line coupling; tidy comment
… backstop annotation with ::error::; add migration-detection test proving the guard fires on a non-monolith shard; trim over-cap comment
…he backstop regex on skip()'s NOTE/SKIP emission prefix (fixes a false-positive where a log line merely quoting the pattern, e.g. a mutation-routing finding, tripped it); replace the raw-grep coupling pins (which tripped the mutation-routing declaration gate) with a false-positive regression control
…ve the actual skip() with a #671-shaped CLI-absence call and feed its genuine output through the backstop, so a skip()-format drift reddens instead of silently disarming the guard (addresses the shadow's coverage gap)
…s-the-claude-cli-on-every-shard
…mment — the 77-commit base merge brought test_residual_prose_retirement_manifest.py which uses that phrase in ci.yml as a comment-subtraction control fixture; the AC4 rewrite had dropped it (§2.3.0 post-merge coupled-site reconciliation)
Author
|
/prflow:review |
![prflow-reviewer[bot]](https://avatars.githubusercontent.com/u/223979750?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE with notes (correct, well-tested change; one Important future-drift test-gap and comment-length suggestions, none blocking at the critical threshold)
Issue Compliance
Reviewed against issue #1830: CI installs the Claude CLI on every shard — scope unchanged. The three acceptance criteria are each satisfied against the reviewed head (7ceb14b8):
- AC1 — install step gated
if: matrix.shard == 'monolith'(ci.yml), pinned executably with a non-vacuous negative control (no-such-shardreadsno). - AC2 — verify step gated
if: always() && matrix.shard == 'monolith', correctly decoupled from the install step'ssuccess()so a corrupt install cannot pass unverified; pinned. - AC3 —
run-shard.shruns the backstop on every shard (keyed on the gate's skip line, not a shard name), setsshard_rc=1and emits::error::when the #671 gate self-skipped for CLI absence;shard_rcflows intoshard-tally.py extract, which fails closed, so the shard cannot recombine green.
Test Evidence
CI for the reviewed commit 7ceb14b8 reports every required check green: lib + python tests: success; lint (shellcheck + actionlint + ruff): success; all five shards (monolith, modules-rest, modules-pin, modules-large, python-pool): success. (Authoritative CI conclusions read from the GitHub API for this exact commit.)
Code Review Findings
🟠 Important / Major
- The fail-loud backstop's regex is coupled to
skip()'s output format (via therealnotecoupling probe) but not to the real #671 gate call-site literals atlib/test/run.sh(the twoskip "#671 claude plugin validate --strict …" blocking-gate "claude CLI not on PATH …"calls). Both real call-sites match the regex today, so the backstop is correctly armed now — but a future rename of the gate's skip name or reason would silently stop matching the real production line while the suite stays green (the crafted coupling probe still matches the stale regex). This reintroduces, one level up, the same silent-disarm class the PR closes. Low-cost remedy: derive the probe's name/reason from the same source the real call-sites use, or add an assertion that grepsrun.shfor a liveskip "#671 claude plugin validate --strict…"call whose text the backstop regex matches, pinning the regex to the real producer line. (raised by 2/5 agents)
🟡 Suggestion / Minor
run-shard.shdoes not fail closed if the log is unreadable —grep -Eqfires only on exit 0 (match); a grep error (exit 2) reads as "no skip found". In practice the log is created andcat'd earlier in the same script, so this is minor; branching on grep's three exit statuses would harden it. (raised by 1/5 agents)- Non-coupling fixtures (
reverted,only434,migration) emit aSKIPprefix, but the realskip()only ever emitsNOTE. Harmless (the regex accepts both and the genuineNOTEpath is covered byrealnote), but the positive cases would mirror reality more faithfully usingNOTE. (raised by 1/5 agents) - Comment-length / derivation (prevention-only comment standard) on the rewritten ci.yml install block, the run.sh #1830 test block and pin scope-note, and the run-shard.sh backstop comment: several added/modified blocks exceed three physical source lines or carry derivation/worked-example content. Note the ci.yml rewrite is a substantial net reduction and removed the prior dated-snapshot provenance — an improvement; the remaining content is accurate throughout (no
documented_falsehood). (raised by 1/5 agents)
Assessment
The change is correct and unusually well-tested: AC-pinned with positive, negative/vacuity, false-positive (quote-without-prefix), scope/migration, and real-skip() coupling controls; fixtures are isolated under mktemp with tally dirs and SKIPS_FILE redirected away from the suite's own log, so nothing self-trips. No documented_falsehood — every rewritten comment (including the removal of the "installed on EVERY shard" prose) is accurate against HEAD. No checklist FAIL/INCONCLUSIVE and no self-contradicting diff, so nothing drives a REJECT at the critical threshold. The single Important item is a future-drift hardening of the backstop's own coupling and is worth addressing but does not block merge.
Reviewed HEAD: 7ceb14b
…s-the-claude-cli-on-every-shard
Review reception for PR #1962 (verdict APPROVE with notes; repo fix threshold is `suggestion`, so every note was triaged). Important / Major — FIXED. The `realnote` probe couples run-shard.sh's backstop regex to skip()'s output FORMAT, but its name/reason are crafted, so renaming either real #671 gate call-site would disarm the guard with the suite still green. lib/test/run.sh now extracts both real `skip "#671 …"` call-sites from its own source, re-drives each one's own literals through the real skip() AND through lib/test/summary.sh's ` SKIP ` renderer (a real monolith log carries both prefixes, which is why the regex accepts both), and feeds each genuine emission through the real run-shard.sh backstop. A count assertion fails closed if a call-site is renamed or rewrapped so the extraction can no longer drive it. Mutation-checked on copies (route (a)): * producer drift — renaming call-site 1's reason in a copied run.sh turns both of that site's coupling assertions RED, site 2 unaffected (3 passed, 2 failed); * consumer drift — renaming the regex in a copied run-shard.sh turns all four RED (1 passed, 4 failed). Suggestion 1 — FIXED. `grep -Eq` folded a scan error (rc > 1: absent or unreadable log) into the no-match arm, failing the guard open on exactly the input it exists to catch. run-shard.sh now branches on the three exit statuses and fails the shard closed on rc > 1 with a specific `::error::`. Driven by a new assertion whose fixture pre-creates the tally dir's log.txt as a directory; it is guarded by a host-capability probe rather than asserted unconditionally, so a host whose grep answers 1 there does not pin host behavior. Suggestion 2 — REJECTED, with evidence. The premise ("the real skip() only ever emits ` NOTE `") holds for skip() but not for the log the backstop scans: devflow_render_test_summary in lib/test/summary.sh emits one ` SKIP <name> [<kind>] — <reason>` line per self-skip at end of run (the same shape the #456 block already greps for). Converting the `reverted`/`only434`/ `migration` fixtures to `NOTE` would delete the only coverage of the regex's `SKIP` alternative. Addressed instead by driving both real producers above. Revisit only if summary.sh stops rendering ` SKIP ` lines. Suggestion 3 — PARTIALLY FIXED. The comments this change touches in run.sh and run-shard.sh are rewritten to the prevention-only standard (prohibition plus consequence, three physical lines). The ci.yml install-step comment is left as reviewed: issue #1830 AC4 requires it to describe the conditional shape and the loud-failure backstop, so trimming it to three lines would break that criterion. Revisit if AC4 is superseded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
/prflow:review |
PRFlow Review — PR #1962Status: ✅ Approved with notes Blueprint
Verdict: APPROVE with notes (monolith-only CLI install + fail-loud backstop; two demotable Suggestions, no blocking findings)Issue ComplianceReviewed against issue #1830: install the Claude CLI on the monolith shard only, with a fail-loud gate-armed backstop — criteria from workpad; scope unchanged. Requirement-based checklist items are included in the verification results below. All five acceptance criteria are satisfied against HEAD (
Verification Checklist ResultsDiff-claim verification was carried out directly by the Phase 3 roster against the reviewed head bytes ( Code Review Findings🟡 Suggestion / Minor
over-grade annotation: no finding flagged Notes
|
… the pins review-and-fix iteration 1 (engine verdict REJECT; fix threshold `suggestion`, so every finding was triaged). Eight deduped defect classes; six fixed, two rejected or deferred with evidence. 1. Backstop fired on EVERY host (Important, 4/5 agents). `run-parallel.sh` and `run-shard.sh monolith` failed on any desk without the claude CLI while `lib/test/run.sh` on the same commit exited 0 with a skip — two CLAUDE.md-sanctioned local instruments disagreeing over one condition, with a CI-framed remedy string. Issue #1830 AC3 scopes the required failure to a CI run, so run-shard.sh now fails only when GITHUB_ACTIONS is set and warns (non-fatal, same cause named) otherwise. Migration detection is untouched: still every shard, still keyed on the gate's own skip line rather than a shard name. 2. The `always()` rationale was FALSE against HEAD (Critical/Important, 5/5). A non-zero install already fails the job (the shard job sets no continue-on-error) and a zero-exit corrupt install satisfies success(), so the claimed "would skip verify after a corrupt install, letting it pass" describes no reachable outcome. Rewritten in ci.yml and in the mirrored `structural-pin-ok` rationale to state what dropping `always()` actually does: re-couple verify to every earlier step in the job, the coupling AC2 forbids. `always()` itself is unchanged — it satisfies AC2. 3. My own new fail-closed assertion was VACUOUS (Important/Critical, 3/5). Probed and confirmed: with the entire backstop block deleted the directory-shaped log.txt fixture still exits 1, because that fixture breaks run-shard.sh's log truncation and dispatch redirect first. Replaced the exit-code assertion with one on the guard's own `could not scan shard <n> log` breadcrumb, plus a positive control proving a scannable log emits none. Now flips under the deletion mutation (verified). 4. Both ci.yml `if:` pins were job-scoped, not step-scoped (Important, 3/5) — a condition carried by any step in the `shard` job satisfied them. Added `devflow_ci_step_has`, which scans one named step's own lines, plus a scope control asserting the install step's bare condition does NOT match inside the verify step. Mutation-checked against both falsifying edits the review demonstrated (delete the install `if:` and add one elsewhere; swap the two steps' conditions) — the job-scoped form stayed green on both, the step-scoped form goes RED on both. 5. The failure cause never reached the recombined recap (Important, 1/5). The `::error::` went only to stderr, so shard-tally.py's `Failure recap:` bullet parse saw nothing and the required check reported a generic synthetic message. The CI arm now appends the cause to $LOG_FILE in that bullet shape, and `shard_rc=1` no longer clobbers a dispatch-set non-zero rc. Asserted in the log and in the extracted `names` tally. 6. The guard's diagnostic was untested (Suggestion) — discharged by 5's assertions and by 1's warning-text assertion; every fixture previously discarded stderr. 7. Stub comment named a `migration` CGA_CASE arm that does not exist (Suggestion) — reworded to describe what the module stub actually does. 8. ci.yml comment block over the prevention-only cap (Suggestion) — trimmed; the skip-exits-0 fact now stated once instead of three times. AC4's required content (conditional shape + loud-failure backstop) is preserved. REJECTED with evidence: `!cancelled()` in place of `always()` (Suggestion, 1/5). It would need the `${{ }}` wrapper — bare `!` opens a YAML tag, which no other `if:` in this workflow uses — to re-shape an AC2-load-bearing condition and its pin, against a benefit of not spending a seconds-long verify step on superseded runs. Revisit if the false-red-on-cancellation cost is measured rather than argued. Every fixture invocation now pins GITHUB_ACTIONS explicitly: unpinned, each assertion would exercise one tier at the desk and the other on CI, leaving the untaken arm untested on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Step 3.5 fix-delta gate caught this: 3535aba narrowed run-shard.sh's backstop to CI runs and updated the sibling sentence in ci.yml ("fails a CI shard"), but left this one ~160 lines above its own edits in the file it was editing. It read "FAILS any shard", which became false for every non-CI shard the moment the tier gate landed at run-shard.sh's `[ -n "${GITHUB_ACTIONS:-}" ]`. Grep confirms this was the only stale site and that both now agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3535aba's ci.yml comment rewrite deleted the phrase `self-referential-ordinal`. lib/test/test_residual_prose_retirement_manifest.py's _COMMENT_SUBTRACTION_CONTROLS uses that phrase, homed in .github/workflows/ci.yml, as a live control: the token must be PRESENT in the raw file and ABSENT from the comment-subtracted machine-consumer corpus, which together prove comment subtraction is still running. Deleting it made the raw-presence half fail. Restored as an authoring prohibition rather than a bare fixture string — do not enumerate the descent arms in that comment — and the comment now names the test that depends on the phrase, so the next rewrite has the coupling in front of it. This is the second time the branch has dropped it (7ceb14b was the first restore). ONE root cause, not two. All three red shards trace to this single deletion: * modules-pin — #810 residual pin-retirement manifests: the control assertion itself. * modules-rest — #1055 measured floor reconciliation: its RealRunnerContractTests case runs the REAL module runner against the working tree (cwd=ROOT) and asserts failed==0; the in-tree #810 failure made that 1 != 0. * python-pool — test_module_runner.py's focused-shard-path case, same mechanism. Attributed by experiment, not inference. With the token removed as the ONLY change, `test_reconcile_module_floors.py RealRunnerContractTests.test_reconciler_summary_matches_ the_real_runner_for_a_smoke_bounded_module` fails `actual: 1`; restoring the token alone turns it green. No floor literal was touched in either arm. The measured-floor hypothesis is refuted: `git diff --name-only fcfee65 8a82e4e` is ci.yml, run-shard.sh, run.sh — no lib/test/modules/*.sh and no registry file — so no module's assertion count moved and no floor could have drifted. Nothing was edited to make an assertion pass. Verified locally: test_residual_prose_retirement_manifest.py 13/13 OK; test_reconcile_module_floors.py 26/26 OK; test_module_runner.py focused case OK; `lib/test/run-shard.sh monolith` 9865 passed, 0 failed, 1 skipped (the expected #434 dirty-tree self-scan, cleared by this commit). actionlint + YAML parse clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
/prflow:review |
There was a problem hiding this comment.
Verdict: APPROVE with notes
Reviewed HEAD: 22c95c29b83b77cbc4befd0018aace353cf6fd97 — PR #1962 (issue #1830).
Summary
Gates the Claude Code CLI install to the monolith shard only (was: all five shards, four of which consumed nothing from the CLI), gates the verify step to if: always() && matrix.shard == 'monolith', and closes the silent-revert exposure that narrowing would reopen with a fail-loud backstop in lib/test/run-shard.sh. Adds a thorough #1830 test block in lib/test/run.sh plus a coverage-map.json entry.
Test evidence
CI is the authoritative evidence for this commit and is all green: lib + python tests, lint (shellcheck + actionlint + ruff), and all five shards (monolith, python-pool, modules-pin, modules-large, modules-rest) each report success.
Diff classification
engine_self_modifying (touches lib/**) → full checklist profile. has_new_types false (no type-design-analyzer); test-relevant (pr-test-analyzer included). Prompt-surface edit-routing gate: not triggered — the diff touches none of the trigger globs (skills/*/SKILL.md, skills/**/phases|references/*.md, .prflow/prompt-extensions/*.md), so no writing-skills discharge is owed.
Acceptance criteria
- AC1 (install gated to monolith) — met.
.github/workflows/ci.yml: install step carriesif: matrix.shard == 'monolith', pinned with a new step-scopeddevflow_ci_step_hasmatcher plus vacuous-yes and step-scope negative controls. - AC2 (verify
always() && matrix.shard == 'monolith') — met.always()correctly decouples verify from the install step'ssuccess()while keeping it shard-scoped; pinned and controlled. - AC3 (backstop fails a CI shard on the #671 CLI-absence self-skip) — met.
lib/test/run-shard.shgreps every shard's retained log for the gate's own skip line, and underGITHUB_ACTIONSemits::error::, appends aFailure recap:bullet (soshard-tally.pysurfaces an actionable name), and setsshard_rc=1. Correctly keyed on the skip line rather than a shard name, so a future migration of the gate is still caught.
Findings (Phase 3, five reviewers)
No Critical or Important findings. Correctness of the shell logic was verified in depth:
- grep exit-code handling is three-way and fails closed on rc>1 (scan error), not folded into the no-match arm.
- No false positive on the monolith shard: every
#1830fixture that emits a matchingNOTE/SKIPline captures it to a scratch file or runs under>/dev/null 2>&1, andskip()'sSKIPS_FILEis redirected to a sink, so nothing leaks intorun.sh's own stdout (the log the backstop scans). The(NOTE|SKIP)␠␠two-space anchor and thequotecontrol guard the false-positive path. shard_rcis defined before the block; the[ "$shard_rc" -ne 0 ] || shard_rc=1guard preserves a more-specific dispatch rc; the loud-failure guarantee rests onshard_rc=1+shard-tally.py's fail-closed synthesis, not on any swallowed-error construct.- Verify's
always()catches a corrupt/absent install two independent ways (claude --versionunderbash -e, andassert-cli-version.sh). - Test block genuinely exercises all three ACs end-to-end (real
run-shard.sh+shard-tally.pyover a fixture tree), with real negative/scope/migration/unscannable-log/tier controls and a coupling loop that re-drives the realskip()call-sites so a rename or format drift reddens rather than silently disarming the guard.structural-pin-ok: cross-file-phase-contractdeclarations are valid over executableif:boundaries. - Rewritten comments were checked for rot: no documented falsehood; the
test_residual_prose_retirement_manifest.pycomment-subtraction-control claim is accurate.
Suggestions (non-blocking)
- Comment length (
lib/test/run-shard.sh,.github/workflows/ci.yml,lib/test/run.sh): several new/rewritten comment paragraphs run 4–6 physical lines where CLAUDE.md §2.3 caps a surviving inline comment at three source lines (prohibition + one consequence). Each names a wrong change it prevents, so they pass the substance test but exceed the line cap; some design narrative would sit better in the issue / internal docs. The net direction is a large improvement (the ci.yml install comment was cut from ~55 dated-snapshot lines to ~25). Worth a light trim; not blocking.
Verification-evidence advisory (non-blocking)
Standalone review; advisory only — not a verdict change.
The change is correct, complete against its ACs, and merge-ready.
The01Geek
deleted the
issue-1830-ci-installs-the-claude-cli-on-every-shard
branch
August 25, 2026 23:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shardmatrix legs, uncached and unconditional, in front of the required checklib + python tests— yet only themonolithshard consumes the CLI, so four installs were pure claude.ai exposure.monolithshard, cutting five network installs per push to one.run-shard.shfails any shard whose log shows the issue-671 plugin-validate gate self-skipped for CLI absence.Changes
.github/workflows/ci.yml: Addif: matrix.shard == 'monolith'to the CLI install step andif: always() && matrix.shard == 'monolith'to the verify step (always()so verify runs whenever the shard runs, gated on the shard and not on the install step's outcome, so a corrupt install cannot pass unverified). Noactions/cachestep is added. The step comment is rewritten to describe the conditional shape and the loud-failure backstop, replacing the prose that presented the five-shard install as the standing trade.lib/test/run-shard.sh: Add a backstop that runs on every shard and greps the shard's retained log for the issue-671 gate's own CLI-absence skip line (anchored onskip()'s(NOTE|SKIP)emission prefix, keyed on the#671+blocking-gate+claude CLI not on PATHtext, excluding the issue-434 self-scan). On a match it emits::error::and setsshard_rc=1. It is deliberately not scoped tomonolithby name, so a future migration of the gate to an install-less shard fails there instead of reverting silently.lib/test/run.sh: Update the existing issue-671 install pin to the monolith-only shape and add the issue-1830 test block (see Test Plan).lib/test/modules/coverage-map.jsonrecords the1830unit asunmodularized.Resolves
Resolves #1830
Test Plan
#1830 ci.yml: the CLI install step is gated on the monolith shard— pinsif: matrix.shard == 'monolith'on the install (AC1).#1830 ci.yml: the verify step runs on monolith independent of the install outcome— pinsif: always() && matrix.shard == 'monolith'(AC2).#1830 ci.yml gate control: an unused shard condition is NOT present— negative control proving the matcher is not vacuouslyyes.#1830 run-shard: the #671 CLI-absence skip FAILS the monolith shard— therevertedfixture case; the backstop fires (AC3).#1830 run-shard: an unrelated #434 blocking-gate skip does NOT fail the monolith shard— theonly434case, proving the guard's scope.#1830 run-shard: a clean log passes the monolith shard— thecleancase.#1830 run-shard: a line quoting the pattern without the NOTE/SKIP prefix does NOT fail the shard— thequotefalse-positive control for the emission-prefix anchor.#1830 run-shard: a non-monolith (module) shard whose log carries the #671 skip ALSO fails— themigrationcase, proving the backstop is notmonolith-scoped.#1830 run-shard: skip()'s REAL #671 CLI-absence emission trips the backstop— therealnotecase, coupling the run-shard.sh regex to run.sh's genuineskip()format.Visual Changes
N/A
Breaking Changes
None
prompt-surface growth: no tracked
*.mdunderskills/,agents/, or.prflow/prompt-extensions/changed between8c898a5b6e779c4383b4142673897aca94ec76a1and03ea8bcb538d5d8dc5a0c1141a576dc9225b6637— no table rendered.Generated via /prflow:implement (v2.34.22, claude-opus-4-8, low)