fix(pipeline): a banked §CP PR with no ticking approval-watcher now reds (#4754) - #4983
Conversation
…eds (#4754) Banking a control-plane PR and arming the approval-watcher were two independent acts, and the board state after a bank-without-arm was identical to a bank-with-arm. PR #4742 sat approved-and-unenqueued for 8h34m and emitted nothing; a human on a routine pass caught it. Two breaks were stacked: the loop was never armed, and the watch set it was told to re-derive had no shipped implementation anywhere in the tree. The new `cp-bank` tool closes both: - `cp-bank apply` is the whole banking act as one verb — provision + apply the `status:cp-banked` label, assign the approver, request their review — then read the PR back and fail if it did not land. Banking always writes the label the derivation keys on. - `cp-bank set` is the shipped board derivation of the banked watch set, replacing `banked_cp_prs_awaiting_approval_json`, a helper name with no definition. - `cp-bank check` is the outside reader: it correlates that board set against the approval-watcher's tick ledger and reds when banked work exists with no tick inside a bounded window. `.github/workflows/cp-bank-guard.yml` runs it hourly, so an unarmed watcher surfaces without a human running `ticks`. Arming is a loop, so banking cannot *be* the arming — a one-shot armed flag would move the same silence one level up. The coupling is therefore on the read side: the absence of a tick is what reds. `check` is three-state (0 green / 1 RED / 2 UNKNOWN). A repo whose label set lacks `status:cp-banked` derives an empty board, so that state is UNKNOWN and never a vacuous pass (ADR 0092); the label is added to the vocabulary-preflight required set so adoption is asserted rather than assumed. The ledger read is `approval-watcher`'s own new `liveness`, resolved without provisioning — a guard must not mint board state.
No preview deploy
|
|
review-code: advisory — §CP blocking-set verdict, no bindable first-line Reviewed-head: @ d955121
Required namespaces, from Acceptance criteria — #4754
AC 1. Shape (b), stated in the PR body and in the EM definition: arming is a loop, so no one-shot act can guarantee it is still running — the coupling lives on the read side. The reasoning is right, and it is the reasoning the issue's own framing invites. What banking can be made atomic is the label the derivation keys on, and it is ( AC 2. AC 3. AC 4. AC 5. The load-bearing claim, attackedThe claim under test: an empty watch set can never resolve GREEN unless it was proven empty. I enumerated every path by which the set could come back empty, unreadable, short-read, or non-conforming.
I could not construct a vacuous pass. The single GREEN-with-an-empty-set branch is reachable only after the scoping label has been proven present against the live universe, and every unreadable input on the way there lands on 2 or on a hard error. Both non-zero states fail the job, and the report says which. The claim holds. Two observations, neither blocking and neither a counterexample:
|
|
review-skill: advisory — §CP blocking-set verdict, no bindable first-line Reviewed-head: @ d955121 Scope: The outstanding question — does any other shipped path bank a §CP PR without writing the label?This is the one that decides whether the fix is real, because a bank that skips the label produces an empty derived set, a GREEN guard, and a stranded PR: the original defect wearing a new coat. Answered directly, not by intent-reading: Code. Instructions. One residual, non-blocking. Three files still describe banking generically as "assigns it to the approver and labels it banked" without naming the verb or the label: The bash the definition ships
Prose hygieneThe new "arming is a loop, so banking cannot be the arming" bullet states the shape, the reader, the schedule, and the operational consequence for the engine ("if you are holding banked §CP PRs, your loop ticking is what keeps that guard green") — a behavioral instruction, not a restatement of the tool's docblock. It does not re-derive the tool-owned decision, which is what the crew-corpus lint checks; that job is green. Governing rulings on both threads — read, and honored
Disclosed out-of-diff write — creating
|
usirin
added
ready-for:human
BANKED — awaiting
|
| namespace | verdict | binds |
|---|---|---|
review-code |
advisory PASS (§CP) | d955121d |
review-skill |
advisory PASS (§CP) | d955121d |
Required set {review-code, review-skill} derived via class-probe over 11 files. No apps/web/src/ path, so review-design correctly not required. Head re-verified unmoved immediately before banking. CI at head: 43 success, 3 skipped, 0 failure, 0 pending.
What this fixes. Issue #4754: banking a §CP PR did not arm the approval-watcher, so an approved-but-unnoticed §CP PR could stall forever — the human does their part and nothing tells the engine. The build took the issue's second option on the reasoning that arming is a loop, not an act: a "banking writes an armed record" fix reproduces the same defect one level up, since an armed record with no ticks after it is again indistinguishable from armed-and-quiet. So banking writes the label the watch set derives from, and the absence of a tick is what reds.
The load-bearing claim was attacked, not read for intent. The claim is that check is three-state — 0 GREEN / 1 RED / 2 UNKNOWN — so an unreadable or empty watch set can never pass vacuously. The gate enumerated ten paths to an empty / unreadable / short / non-conforming set; every one lands on UNKNOWN or a hard error, none on GREEN. The decisive details: judge tests label presence before it will read banked.length === 0; resolvePresence treats an empty label universe as absent by construction; and the banked-set decoder is Array(Array(RawIssue)), so a {"message":"Not Found"} error body cannot flatten to []. No vacuous-pass counterexample was constructible.
The question a previous stalled gate died before answering, now answered: does any other shipped path bank a §CP PR without writing the label? No. cp-bank/github.ts:79 is the only POST .../requested_reviewers in the tree and it writes the label. ship-it does not bank; the chief-of-staff is forbidden from banking. Three files still describe banking generically (claude-plugins/pipeline-crew/HOW-TO.md:123-125, the chief-of-staff agent def, and the personalization doc) — stale prose with no instruction to perform a bank, so not a second path. Non-blocking.
cp-cardinality was not forked — zero cardinality/approval/discharge references under cp-bank/. The §CP discharge stays single-source.
Two non-blocking observations recorded by the gate: an infra error and a RED share exit 1 (a fidelity limit, not a safety one — both fail the job); and a future-dated lastAt would read as fresh, unreachable from any shipped path.
Out-of-diff action, disclosed and assessed: the build created the status:cp-banked label on the live repo, outside the diff. The gate judged it properly disclosed and the PR coherent without it — correctness does not depend on the write (an absent label yields UNKNOWN, apply provisions it, vocabulary-preflight requires it), and the live label matches createLabelArgs exactly. For the approver's awareness: that label is already in use — three §CP PRs were banked with it tonight.
An approval binds the head it is given at (ADR 0058). Any push after approval invalidates it and re-banks the PR.
3 participants
Fixes #4754
The shape taken, and why
The issue offers two shapes. This PR takes the second — make the absence of a tick reportable — and it does not take the first.
Arming is not an act; it is a loop. Banking writes board state once, but the watcher has to keep ticking, and no single act at bank time can guarantee that a loop is still running an hour later. A "banking also writes an
armedrecord" fix would produce exactly the defect it was meant to close, one level up: anarmedrecord with no ticks after it is again indistinguishable from an armed-and-quiet watcher. So the coupling has to live on the read side.What is now true: banking is one verb that always writes the label the watch set is derived from, and a non-empty banked set with no ledger tick inside a bounded window is a RED that fires on a schedule, with no human running anything by hand.
The new tool —
pipeline-cli cp-bankapplyprovisions thestatus:cp-bankedlabel if the repo lacks it, applies it, assigns the approver, requests their review, then re-reads the PR and fails if the label or assignment did not land. Four 2xx responses are not evidence the board carries the state; the read-back is.setis the shipped implementation of the derivation the engine definition called by name only.banked_cp_prs_awaiting_approval_jsonappeared exactly once repo-wide, at its call site, with no definition anywhere — each engine session was expected to invent it. It now prints the open PRs carrying the banked label as a JSON array, in the exact shape the existing tick frame already consumes (all_pages_are_arrays, thenjq -r '.[].number').checkcorrelates that board-derived set against the approval-watcher's tick ledger and reds when the set is non-empty and the newest tick is missing or older than the window (default 2h).The reader (AC 3)
.github/workflows/cp-bank-guard.ymlrunscp-bank checkhourly. This is a schedule, not apull_requestgate, because the failure has nothing to do with any diff — it is a state of the board that appears between PRs.cp-bankis deliberately a different tool fromapproval-watcher. The watcher writes the ledger, and a surface that only ever writes its own trace can never notice that it stopped writing. The ledger read itself is not re-derived here:approval-watchergrew alivenessmethod (the newest tick instant across the ledger, taken as a max rather than off the last comment, because coalescing patches an older comment and bumps itslastAt), andcp-bankconsumes that. That read resolves the ledger without provisioning one — a guard must not mint board state, sofindLedgerwas split out ofresolveLedgerand only the write path still creates.Nothing in the §CP discharge predicate was touched.
cp-cardinalityremains the single source of the approve-then-enqueue decision; this PR is about whether the loop that consults it is running at all.The three-state contract, and why the third state is the point
checkexits 0 = proven GREEN, 1 = proven RED, 2 = UNKNOWN.The defect class here is "a check that cannot see what it is looking for returns a plausible value instead of an error." A repo whose label set lacks
status:cp-bankedwould derive an empty banked set and pass — vacuously, forever, exactly the shape #4272 fixed for the triage guards. So the scoping label's presence is proven first, against the live label universe (RepoLabels.presence, the existing seam), and its absence resolves UNKNOWN, never "nothing is banked". An unparseable tick instant, an unparseable clock, and a non-positive window resolve UNKNOWN too.1and2both fail the job; keeping them apart is what makes the report say which happened.Adoption is asserted rather than assumed:
status:cp-bankedis added to thevocabulary-preflightrequired label set, so a repo that adopts the pipeline without the label reds at the preflight instead of silently getting a guard that can never fire. The label has been created on this repo.Live evidence, run against the real board
Before the label existed:
After creating it:
vocabulary-preflight check→prerequisites met — all 17 required labels exist.The regression exercise (AC 4)
packages/pipeline-cli/src/tools/cp-bank/cp-bank.unit.test.tspins the identity finding 1 established, and asserts it is broken. The same board — one banked §CP PR — judged twice:with an explicit assertion that the two verdicts differ. The staleness cases replay the incident's own numbers (a tick 8.6h stale reds); the fail-closed cases cover the absent label, the unreadable instants, and the non-positive window. 13 tests; the full pipeline-cli suite is 3031 passing.
Explicitly out of scope (AC 5)
Nothing here improves a watcher that is already ticking.
checkkeys on tick recency only — not coverage, cadence jitter, or disposition fidelity. Set membership follows from the derivation being board-sourced: any engine ticking against the same board derives every banked PR, so recency is the right signal for "is anything watching at all". Tick quality once armed is #4790, which the founder deliberately left in milestone #38.§CP status
This PR is control-plane, confirmed with
pipeline-cli cp-classify classify→control-plane [path-match](packages/pipeline-cli/src/registry.ts, plus.github/workflows/). It needs a control-plane approval at its current head. The triage note anticipated this: the reader has to live where it can run on a schedule, and that is inside §CP.