[US-234] feat: PR state flow (gate≠review) + pair review as required check - #390
Open
rucka wants to merge 6 commits into
Open
[US-234] feat: PR state flow (gate≠review) + pair review as required check#390rucka wants to merge 6 commits into
rucka wants to merge 6 commits into
Conversation
Collaborator
Author
Verdict
PR: #390 · Author: rucka · Reviewer: independent review agent · Date: 2026-07-28 · Story: US-234 · Type: feature Classification matrix — per dimension
Tier = max(assessed) = risk:red (confirms the refinement-time tier, no drift). Cost = highest detected signal = green. AssessmentsSecurity — Input validationVerdict: green — the only external inputs (PR labels, PR author, head SHA) reach the shell through Details
Security — Output handlingVerdict: green — no rendering surface touched; the only outputs are check-run summaries and Security — AuthenticationVerdict: yellow — the recipe assumes the ambient Security — AuthorizationVerdict: yellow — the two-check split (agent verdict vs deterministic human-approval job) is the right authorization design and the reviewing agent genuinely cannot self-grant the 🔴 gate; but as shipped the 🔴 control can never be satisfied at all (Critical 2, Major 3), i.e. it fails closed and is unusable rather than unsafe. Security — Introduced vulnerabilitiesVerdict: green — 0 introduced, 0 pre-existing. DetailsNo CostVerdict: Architecture (Coupling)Verdict: green — the synthesis lives once in DetailsFindings by severityCritical (must fix before merge)
Major (should fix before merge)
Minor (consider)
Questions
Positive feedback
Functionality & requirements (AC coverage)
Testing & quality gates
Adoption compliance
Tech debt
Documentation
Performance & deployment
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
Author
Remediation — final (verified at head
|
rucka
added a commit
that referenced
this pull request
Jul 30, 2026
…te no longer depends on the caller Two defects observed driving the #277/#230/#281/#236/#234/#279 batch. 1. A batch that drove NOTHING reported success. `args` was normalised with JSON.parse in a try/catch whose catch set `_args = undefined`, so `STORIES = _args?.stories ?? []` silently became empty: a run invoked with `args: "#234 #236 …"` exited in ~30ms with zero agents and returned `{batch: [], note: 'PRs are ready-for-merge or escalated…'}` — the shape of a COMPLETED batch, indistinguishable from a real run whose stories all failed. Now validated loudly: a non-JSON string, absent args, an object without `stories`, or a story missing id/title/branch throws with a message naming the required shape and why title/branch cannot be derived (no gh/fs in the sandbox). An EXPLICITLY empty list stays a legal no-op — a computed 'nothing to do' is not a mistake. A bare array and a JSON string are both still accepted, and a leading '#' on an id is normalised so no shell path carries it. 2. The duplicate-first-review guard depended on the caller's bookkeeping. The #373 continuation probe sat inside `if (resuming)`, and `resuming` is `Number.isInteger(story.prNumber)` — so it only ran when the caller passed `prNumber`. A `Workflow({resumeFromRunId})` resume replays implement/PR from cache with the SAME args, so `story.prNumber` is absent, the probe never ran, `firstReviewPosted` stayed false, and round-0 posted ANOTHER first review on a PR that already had one — three times on story #234 (PR #390) across three pause/resume cycles, which is why that story ended the batch least-progressed with 4 Major still open while the others were at zero. The gate is now the PR's existence, a fact the script knows. One sonnet/low probe per story per run is far cheaper than one duplicated opus/xhigh review, and on a fresh story both signals return false so the fresh path is unchanged. Also: the args contract is now in meta.whenToUse, so a caller reads it BEFORE invoking — the skill wrapper interpolates received arguments verbatim into `Workflow({args})`, which is how the wrong shape was reachable by construction. Tests 46 -> 55. One pre-existing assertion was INVERTED on purpose: '#373 fresh story: no continuation probe runs' pinned the cost saving that caused defect 2. Its real intent (fresh path unchanged) is preserved and now asserted on the OUTCOME — the first review still posts — plus a new test proving a garbage probe return cannot silence it (fail-open). Adoption: way-of-working.md now records the assignment rule — every story, tech-debt, epic and PR that is worked on carries an assignee. Same class of tracking hole as the two defects above: the board is read filtered by assignee, so an unassigned item is invisible there even when open, PR'd and green. The follow-ups filed *during* a batch are the ones most often left orphaned, and a PR with an author but no assignee does not show up either. Closes #401 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 30, 2026
Coverage baseline auto commit-back — CI persists/ratchets the baseline (opt-in, on top of #282)
#372
Closed
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
…doption (T-6) Surgical edits only — three files carry live hunks from parked PRs #389/#390, so each change is a single-line replacement with no reflow or reordering. - way-of-working.md: `Coverage baseline commit-back: disabled` appended to the existing Coverage guardrail bullet, with the credential requirement and why it stays off here until #234's protection lands and the secret exists - tier-aware-pipeline.md (both corpora): the nested opt-in, the push-not-PR trigger, the bot-PR landing, monotonicity, termination and the degrade-to-warning contract - coverage-baseline.md: what writes to this file and when, and that a human remains the only writer while the flag is disabled Two dead markdown links removed: both pointed at adr-018-pr-state-flow-required-checks.md, which exists only in parked PR #390. The local gate does not catch these — the broken-.md-link check is an e2e test, outside the local gate set — so they would have surfaced in CI or, worse, only once #390 merged. Refs #372 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
…r capability yet (M4) The shipped, adopter-facing KB documented the flag, the trigger, the bot-PR landing and the credential as if an adopter could enable it. They cannot: the logic lives in packages/knowledge-hub/src/tools/ (pair-internal — the shipped assets are coverage-gate.sh and tier-resolve.sh), and /pair-capability-setup-gates never asks about the nested flag, never records it and never emits the step. An adopter setting `Coverage baseline commit-back: enabled` got a SILENT NO-OP — the worst shape for an opt-in: config says on, docs say what it does, nothing happens, nothing complains. Both the guideline and the config example now state the pair-internal scope, in both corpora, and point at #409 for the adopter path. Deliberately NOT fixed here: the stale "automated commit-back is story #372" note in setup-gates/SKILL.md:199. It is one line, but that file is modified by BOTH parked PRs #389 and #390 — a three-way contention in a PR whose disjointness is a stated business rule. It belongs to #409, which has to edit that skill anyway to add the flag to the interview. Refs #372 · #409 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
… as a bot PR, opt-in and off by default (#405) * [US-372] docs: ADL — ratchet lands as a bot PR, never a base push Decide AC5 (PR vs push) and the credential model against the POST-#234 regime, not today's unprotected main: - a pull_request run NEVER writes (uniform for forks; no head-SHA mutation, so a human approval is never invalidated) - the post-merge push proposes the raise as a bot PR from chore/coverage-baseline-ratchet — no branch-protection bypass is requested, which is the point of #234 - credential: repo-scoped COVERAGE_RATCHET_TOKEN (contents:write + pull-requests:write, no admin, no bypass list); GITHUB_TOKEN rejected because a PR it opens triggers no run and can never satisfy the required contexts - loop termination: marker predicate PLUS a floor(measured)-1pp fixpoint; [skip ci] rejected (it would skip the checks the ratchet PR must pass) - rejected options recorded with the reason each fails - Task: T-1 — Decide and record PR-vs-push behaviour + credential model Refs: #372 * [US-372] feat: monotonic coverage-baseline ratchet + opt-in CI wiring Gate logic in a tested module (ADL 2026-07-13); ci.yml stays a thin entrypoint that only forwards the measured numbers and exits 0. - ratchet writer: raises `^baseline.<type>=` values IN PLACE, never rewrites the surrounding markdown; only a value strictly above the committed one is a raise (equal/drop/unknown-type/malformed all hold) - concurrency: the config is re-read immediately before writing and every proposal re-checked, so a higher value from a concurrent run is dropped, never clobbered - skip predicate: flag-disabled (default) / not-base-push (every PR run) / automated-commit (marker or ratchet branch in the head commit) - git plan is data, so its non-negotiables are asserted by tests: one push, to the ratchet ref only, no `git add -A`, no branch switch, always restored to the checked-out SHA - push uses --force-with-lease AND first maps+fetches a remote-tracking ref for the ratchet branch: without it git rejects every later non-fast-forward push as `stale info` (verified), so the ratchet would have worked exactly once and then warned forever - refused write => named ::warning:: and exit 0; the guardrail's verdict is untouched (step runs after it) - token bound to the base-branch push event in ci.yml, so a PR run never has the credential in its environment - smoke scenario DEMONSTRATES loop termination as a real CLI sequence (marked commit, merge commit, and the fixpoint that needs no marker) - Tasks: T-2 ratchet writer, T-3 flag + CI wiring, T-4 loop termination, T-5 degradation on refused write Refs: #372 * [US-372] docs: document the ratchet flag, token and default in KB + adoption (T-6) Surgical edits only — three files carry live hunks from parked PRs #389/#390, so each change is a single-line replacement with no reflow or reordering. - way-of-working.md: `Coverage baseline commit-back: disabled` appended to the existing Coverage guardrail bullet, with the credential requirement and why it stays off here until #234's protection lands and the secret exists - tier-aware-pipeline.md (both corpora): the nested opt-in, the push-not-PR trigger, the bot-PR landing, monotonicity, termination and the degrade-to-warning contract - coverage-baseline.md: what writes to this file and when, and that a human remains the only writer while the flag is disabled Two dead markdown links removed: both pointed at adr-018-pr-state-flow-required-checks.md, which exists only in parked PR #390. The local gate does not catch these — the broken-.md-link check is an e2e test, outside the local gate set — so they would have surfaced in CI or, worse, only once #390 merged. Refs #372 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-372] fix: review round 1 — credential layering, lease-safe write, guarded I/O, anchored flags Code-side findings from PR #405's review. Work started by an implementer the watchdog killed; recovered from the worktree, verified, completed and refactored rather than redone. M1 credential layering — `extraHeader` is MULTI-valued and `actions/checkout` already persists the read-only GITHUB_TOKEN into the same key, so ADDING ours made git send two Authorization headers and left the winner to the server. The likely loser was ours: a refusal that AC6 turns into a warning on every push — a feature that never works and never says so. `gitAuthConfig` + `EXTRAHEADER_CONFIG_KEY` now override the single key instead of layering. M2 unguarded happy path — every file/exec call could throw and exit non-zero, contradicting AC6's own rule that persistence never changes the gate's verdict. `main` now funnels everything except an argument-parsing bug (a workflow-authoring error, which must be loud) into the same warning + exit 0. M3 concurrent clobber — the story's exceptional scenario was unimplemented. `applyRaises` re-checks every proposal against BOTH the text on disk and the config as committed on the open ratchet branch, so a higher value written by a concurrent run is never clobbered on either ref. Minor — the flag parse was an unanchored whole-document regex, first match wins, and the KB sentence this PR ships contains the literal flag string: an adopter quoting that sentence would have turned the flag ON. Now anchored to a list bullet. `readGuardrailFlag` makes the "nested under the guardrail" framing enforced rather than documentation-only. The loop guard no longer fires on a commit message that merely mentions the ratchet branch. `git config --add remote.fetch` no longer mutates the checkout's persisted config, so the plan's "leaves no trace" is now true; `restore` is `--mixed` + a checkout of the one edited path, not `--hard`, so it cannot revert unrelated tracked work before the E2E step. `--margin` and the measured values are validated instead of silently becoming NaN (which would have made the ratchet never raise, quietly). `ratchetGitPlan` split via `ratchetWriteCommands` — it had grown to 64 lines against a 50-line ceiling. Caught by the pre-push gate, not by the test run: vitest was green while lint was red, the same trap a PR fell into earlier today. 80 tests (was 70). `pnpm quality-gate` green. Refs #372 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-372] docs: state that the ratchet is pair-internal, not an adopter capability yet (M4) The shipped, adopter-facing KB documented the flag, the trigger, the bot-PR landing and the credential as if an adopter could enable it. They cannot: the logic lives in packages/knowledge-hub/src/tools/ (pair-internal — the shipped assets are coverage-gate.sh and tier-resolve.sh), and /pair-capability-setup-gates never asks about the nested flag, never records it and never emits the step. An adopter setting `Coverage baseline commit-back: enabled` got a SILENT NO-OP — the worst shape for an opt-in: config says on, docs say what it does, nothing happens, nothing complains. Both the guideline and the config example now state the pair-internal scope, in both corpora, and point at #409 for the adopter path. Deliberately NOT fixed here: the stale "automated commit-back is story #372" note in setup-gates/SKILL.md:199. It is one line, but that file is modified by BOTH parked PRs #389 and #390 — a three-way contention in a PR whose disjointness is a stated business rule. It belongs to #409, which has to edit that skill anyway to add the flag to the interview. Refs #372 · #409 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-372] fix: validate coverage values before writing them to GITHUB_ENV `$GITHUB_ENV` is the classic Actions environment-injection sink: a value carrying a newline defines arbitrary further variables for every later step in the job. The two measured percentages were written unvalidated. These come from a coverage report, so a stray value is a tooling bug rather than an attack — but the guard is one case statement and removes the sink outright. A non-numeric value is dropped with a `::warning::`, so the ratchet sees no measurement and proposes nothing: the conservative outcome, and it still cannot block the gate (AC6). Verified: a value of `20\nEVIL=1` is rejected and only the legitimate `85.04` reaches the file; `sh -n` clean on the snippet. Refs #372 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
rucka
force-pushed
the
feature/US-234-pr-state-flow-required-check
branch
from
July 31, 2026 18:16
6ba4f82 to
f3455bf
Compare
…check - pr-states.md: 3 PR states (to-be-reviewed → ready-to-merge / not-approved), synthesis table (gates × verdict × tier × explicit approval), enforcement layers, edge cases, degraded mode - assets/pr-state.sh: provider-agnostic evaluator (resolve_pr_state, merge_allowed, explicit_approval_required, review_check_conclusion) — tags/signals only, no criteria (D18), fail-safe red - publish-pr: registers required pair-review check pending at creation + labels pr-state:to-be-reviewed + dispatches review to a clean-context subagent (AC1) - review: gate-first cap on the verdict (AC2), Step 5.4 publishes the check + synthesizes the state + reads per-tier reqs from quality-model §4 (AC3/AC4), Phase 6 merge_allowed precondition - setup-gates: Step 4.5 wires pair-review + pair-explicit-approval required checks + stale-approval dismissal, DEGRADED report when no host API (AC5) - github-implementation: check-run publication, explicit-approval job (human, non-author, current head), branch-protection payload, manual degraded steps (R2.12) - ADR-018: required checks are the merge authority, pr-state labels are a view - tests: conformance pr-state-flow.test.ts (RED→GREEN) + smoke scenario executing the state machine per tier - docs: concepts/pr-state-flow page + catalog rows regenerated; root .pair/.claude mirrors via pair update Refs: #234
…ST reviews) + doc coherence Critical - pair-review published via the commit-statuses API, not check-runs: POST /check-runs is GitHub-App-only (verified 403 on this repo) while the skills run with an ordinary token; statuses accepts it and branch protection treats the context identically. Token prereq (repo:status) + "publication refused ⇒ advisory, reported" degradation added. - pair-explicit-approval approval query rewritten on the REST reviews endpoint (commit_id + user.type=="User"): gh pr view --json reviews has no author.is_bot, so the old filter always returned 0 and no red PR could ever pass. Smoke audit token updated. Major - solo maintainer: a red merge needs a SECOND human account (author cannot self-approve) — corrected in pr-states.md, the docs page, ADR-018 and way-of-working; alternative solo-approval token explicitly deferred to its own story. - pr-state:* labels provisioned (gh label create) + non-blocking degradation in publish-pr and review. - executed host assertions added to the smoke scenario (read-only, skip offline): statuses API reachable with the agent token, reviews payload carries commit_id + user.type, and gh pr view exposes no bot flag. Minor / questions - pending guard in the publication snippet; required_approving_review_count: 0 explicit and strict: false justified (per-head review churn); least-privilege permissions block in the workflow template; ordering constraint (labels+job → observe → protect, enforce_admins last) in the guide, setup-gates and way-of-working; quality-model §4 + tier-aware-pipeline reconciled with the red-gate refinement; cross-links (canonical-states back-link, guidelines-catalog row, how-to-11 merge precondition); dataset way-of-working status placeholder; 🟡 reviewer approval mapped to the pair review; force-push re-run stated as triggered, not automatic. Tests: 50 conformance (20 new, RED→GREEN) + smoke scenario incl. 3 executed host assertions. Refs: #234
…host) + executable review dispatch Round 2: 13 findings, all resolved, none escalated. 16 new conformance assertions RED->GREEN. - approval job is an authorization control now: pull_request_target + checkout pinned to base.sha + persist-credentials:false. Reproduced the bypass first (PR-supplied job published pair-explicit-approval=success at risk:red), then verified the pin kills it. - verdict pinned to the PR head SHA as a commit status (GITHUB_SHA is base/merge-ref, measured), job renamed explicit-approval-gate (one producer per context), concurrency + fresh label read (observed label-event race). - dispatch: publish-pr returns review-dispatch-required (no nested subagent); implement Step 3.3 dispatches; prompt bounded to phases 1-5, never merge; /review gains $dispatched + a non-interactive contract for its two human prompts. - DoD: merge-block executed end-to-end on a throwaway repo (pending/failing/absent check, green merge, red block, tier raise, approval-time re-run, tamper, protected push) — evidence table in github-implementation.md + ADR-018. - recipe fixes: one $REPO form, enforce_admins:false in the payload, --paginate on reviews. - pr-states.md stops restating quality-model thresholds; quality-model defines checklist depth (standard vs extended); ADR-018 records the reviewer-identity option as deferred. - smoke: real tier transition, approval filter asserted against a committed fixture, live host probes warn-only (no third-party PR dependency).
…ing-first, both merge paths - Pin the authorization context: branch protection uses required_status_checks.checks with app_id (GitHub Actions) instead of the unpinned legacy contexts array; state the companion settings (read-only default GITHUB_TOKEN, CODEOWNERS on workflows) and the residual (App/check-run is the only unforgeable form, #398). - Stop overclaiming: pair-review is documented as an ANTI-ACCIDENT control (forgeable by any push-access principal, by construction), not authorization — guide, pr-states, ADR-018, docs page. - Fail closed on interruption: the approval job posts state=pending on the head SHA as its FIRST step (property 5), so a cancelled/aborted evaluation cannot leave a stale lower-tier success standing. - Both merge paths carry the precondition: /implement Phase 4 Step 4.1 re-synthesizes and HALTs on merge_allowed (was "at least one approval"); pr-states actor table names it. - One projection, no transliteration: the 🔴 approval predicate ships as human_approval_jq_filter in pr-state.sh; the workflow and the smoke fixture both read it. - Cite #398 wherever the solo-maintainer token is deferred; dataset way-of-working mirrors the full ordering (approval-time re-run) and links github-implementation.md § Ordering; publish-pr no longer calls /implement a "future" caller (#256 closed); the host evidence table states it is a point-in-time observation, re-verified by re-running the ordering. - Tests: +19 conformance (80 total), +24 smoke assertions (108) — RED first. Refs: #234
…, review 0.9.0) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r call) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rucka
force-pushed
the
feature/US-234-pr-state-flow-required-check
branch
from
August 1, 2026 08:16
329a4c4 to
f913890
Compare
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
What Changed
Introduces the PR state flow that separates mechanical gates (lint/type/build/tests) from judgment review (the pair-review verdict), and synthesizes both — plus tier and, at 🔴, explicit human approval — into a single PR macrostate:
to-be-reviewed→ready-to-merge/not-approved. Registerspair-review(agent-published verdict) andpair-explicit-approval(deterministic host job) as required status checks so merge is blocked until gates are green AND the review is approved AND, at 🔴, a human has explicitly approved.Why This Change
Today gate-passing and review-approval are not tied together as a single enforced merge condition — a green CI run says nothing about whether a human/agent judgment review happened, and there was no host-level mechanism preventing merge before that review lands. This closes that gap: the state model + required checks make "reviewed" a fact the code host enforces, not a label anyone could leave stale.
Story Context
User Story: As a maintainer, I want the PR's mergeability to reflect both passing gates and an actual review verdict (with explicit human approval at high risk), so a PR can't be merged just because CI is green.
Acceptance Criteria:
to-be-reviewed,ready-to-merge,not-approved) synthesized from gates × review verdict × tier × explicit approval.Changes Made
Implementation Details
pr-states.md(new) is the single home of the flow — 3 states, gate≠review table, synthesis truth table, per-tier requirements read from quality-model §4 (no criteria duplicated, D18).pr-state.sh(new) is the provider-agnostic evaluator:resolve_pr_state,merge_allowed,explicit_approval_required,review_check_conclusion— signals + tags only, fail-safe on unknown/non-pass input.tech-debtcounts as an approving verdict (matches review Step 5.3's TECH-DEBT→APPROVE mapping).publish-prSKILL.md gains Phase 5 — registerspair-reviewpending on the head commit (merge blocked from t0), labelspr-state:to-be-reviewed, and dispatches/review $pr=<n>to an anonymous clean-context subagent (D23). Degraded path never runs the review inline in the authoring session.setup-gatesSKILL.md gains Step 4.5 wiringpair-review+pair-explicit-approvalas required checks (both pipeline modes, never tier-scoped away), plus stale-approval dismissal and aDEGRADED — enforcement advisoryreport with manual steps when the host/token lacks admin scope.github-implementation.mddocuments the host mechanics: check-run publication, the explicit-approval job (readsrisk:*only, requires non-bot/non-author approval on the current head), and branch-protection payload.process/reviewSKILL.md Step 2.1 gains a gate-first cap (a red gate can never yield APPROVED/TECH-DEBT, so synthesis never reachesready-to-merge); new Step 5.4 publishes thepair-reviewconclusion and resolves tier requirements (reviewers/SLA/checklist/explicit approval) from quality-model §4, then swaps thepr-state:*label.merge-and-cascade.mdgets a new Step 6.0 that re-synthesizes and HALTs rather than bypassing a required check.pr-state-flow.test.ts(new, 30 tests, written RED-first per the conformance ADL) covers every artifact above;pr-state-flow.shsmoke scenario (new, 55 assertions) executes the state machine end-to-end (tier × fail-safe × merge_allowed × conclusion mapping × D18 greps); docs site pageconcepts/pr-state-flow.mdx+ catalog rows regenerated viagenerateCatalogRows(). Root.claude/skills+.pair/knowledgemirrors regenerated with the real copy pipeline (pair-cli update --offline) — not hand-edited — so the tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352 per-skill byte-equality guard (PR [US-352] chore: extend mirror-equality guard to per-skill SKILL.md pairs #377) passes.Review round 1 fixes (commit
94251636)pair-reviewis published as a commit status, not a check run.POST /repos/{owner}/{repo}/check-runsis writable only by a GitHub App installation token — verified 403 on this repo with the token the skills actually hold — so the previously documented recipe could never publish, which would have made every PR permanently blocked once protection was applied. The recipe now usesPOST /repos/{owner}/{repo}/statuses/{sha}(verified usable: 422 for a bogus SHA, not 403), which branch protection accepts as a required context identically. Token prerequisite (repo:status/statuses: write) and a "publication refused ⇒ advisory, reported" degradation are documented ingithub-implementation.md,publish-prandreview.gh pr view --json reviewshas noauthor.is_botfield, so the old filter returned 0 for every review (verified on a real APPROVED review) and no 🔴 PR could ever pass. Replaced with the REST reviews endpoint carrying both fields:select(.state=="APPROVED" and .commit_id==env.HEAD_SHA and .user.type=="User" and .user.login!=env.PR_AUTHOR); the smoke audit token moved fromis_botto the new expression.pair-explicit-approval. The impossible "a solo maintainer satisfies it deliberately" claim is corrected inpr-states.md,pr-state-flow.mdx, ADR-018 andway-of-working.md; an alternative solo-approval token is explicitly deferred to its own story rather than half-specified.pr-state:*labels are provisioned (gh label create× 3 in the guide) with a non-blocking degradation in both skills — they never auto-create.PUTprotection,enforce_adminslast) in the guide,setup-gatesStep 4.5 and this repo'sway-of-working.md, which also states that the workflow is not yet added here. This repo's job is deliberately not added: with a single maintainer it would report red on every 🔴 PR forever (see the finding above).required_approving_review_count: 0stated explicitly (the tier job is the approval authority; an unstated ≥1 default would contradict 🟢 self-merge) andstrict: falsewith the rationale (per-headpair-reviewwould have to be re-earned on every base update). Pending-verdict guard added to the publication snippet; least-privilegepermissions:block added to the workflow template.tier-aware-pipeline.mdnow carry the red-gate refinement clause;canonical-states.mdback-linkspr-states.md; the guidelines-catalog PM row links the PR State Flow page; how-to-11's decision table names themerge_allowedprecondition; the datasetway-of-working.mdline got aStatus: [applied | not yet applied]placeholder; 🟡's "reviewer approval" is explicitly mapped to the pair review; the post-force-push re-run is stated as triggered, not automatic.Files Changed
pr-states.md,pr-state.sh,adr-018-pr-state-flow-required-checks.md,github-implementation.md,pr-state-flow.test.ts,pr-state-flow.sh(smoke),pr-state-flow.mdx(docs) — plus the mirrored copies under.pair/knowledge/**,.claude/skills/**,.pair/llms.txt(regenerated, never hand-authored).publish-pr/SKILL.md(0.4.1→0.5.0),setup-gates/SKILL.md(0.5.0→0.6.0),process/review/SKILL.md(0.7.0→0.8.0),merge-and-cascade.md,quality-model.md,way-of-working.md,skills-catalog.mdx,tag-driven-gates.mdx,smoke-tests/run-all.sh(registers new scenario).Testing
Test Coverage
pr-state-flow.test.ts(30 original + 20 added in review round 1, all written RED-first), part of knowledge-hub's passing suite.pr-state-flow.shexercises the state machine per tier + fail-safe +merge_allowed+ conclusion mapping + doc/skill wiring; registered inrun-all.sh.commit_id+user.type, (3)gh pr view --json reviewsexposes no bot flag (the trap the guide warns about). TheyWARN-skip offline/unauthenticated, never fail. Manually executed against the live host for this round:POST …/check-runs⇒ 403 "You must authenticate via a GitHub App";POST …/statuses/<bogus-sha>⇒ 422 "No commit found" (i.e. permitted); documentedis_botfilter on a real APPROVED review ⇒ 0, corrected filter ⇒ 1. Only the branch-protectionPUTneeds admin scope and remains the human step.pnpm e2e— 39/39 passing (unaffected, run as part of the full 🔴-tier gate set).Test Results
Testing Strategy
pr-states.md.risk:*tag (fail-safe → 🔴), missing review check (pending, not merge-enabling),tech-debtverdict (still approving), stale approval on a new head commit (dismissed).passgate signal is treated as not merge-enabling (fail-safe direction is always "blocked"); host/token without admin scope degrades to an advisory report with manual steps, never a silent no-op.Quality Assurance
Code Quality Checklist
pr-state.sh)Deployment Information
Environment Impact
This PR changes process/skill documentation, a shell evaluator asset, and conformance/smoke tests only — no deployable application code. Nothing to release; the two required checks (
pair-review,pair-explicit-approval) still need to be applied to this repo's branch protection, which requires admin scope on the code host and is called out below as a deliberate human step (seeway-of-working.md's new line and thesetup-gatesDEGRADED report).Deployment Notes
pair-review+pair-explicit-approvalrequired checks) needs code-host admin scope — out of a code change's reach. Until applied, enforcement here is advisory (documented degraded mode insetup-gates' Step 4.5 report and theway-of-working.mdstatus line).Documentation
Documentation Updates
pr-states.md(flow spec),github-implementation.md(host mechanics), ADR-018 (design + rejected alternatives), docs siteconcepts/pr-state-flow.mdx+ cross-link fromtag-driven-gates.mdx.Knowledge Sharing
pr-state:*labels are a view only (a forged/stale label cannot enable a merge). Two checks, not one:pair-review(verdict) +pair-explicit-approval(deterministic host job) so the reviewing agent can never self-grant the 🔴 human approval. Rejected alternatives: labels-as-policy (no enforcement, R5.7 violated) and a single combined check (collapses two authorities, hides which condition is unmet).Reviewer Guide
Review Focus Areas
pr-states.md's truth table andpr-state.sh's fail-safe defaults — especially that unknown/pending inputs never resolve toready-to-merge.tier-resolve.sh/ quality-model §4 — no restated criteria..claude/skills/**and.pair/knowledge/**mirrors were regenerated viapair-cli update --offline(never hand-edited) so the tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352 per-skill byte-equality guard (PR [US-352] chore: extend mirror-equality guard to per-skill SKILL.md pairs #377) passes — worth a spot diff if in doubt.Testing the Changes
pnpm --filter @pair/knowledge-hub test -- pr-state-flow bash scripts/smoke-tests/scenarios/pr-state-flow.sh pnpm quality-gateKey Test Scenarios
pair-reviewcheck → synthesizesready-to-mergewithout needing explicit approval.pair-reviewAPPROVED but no explicit human approval yet → staysto-be-reviewed,merge_allowedis false.not-approved/to-be-reviewedregardless of review verdict (gate-first cap).Dependencies & Related Work
Related PRs
Follow-up Work
pair-review+pair-explicit-approvalfrom advisory to actually enforced — in the documented order (labels + workflow → observe the contexts → protect,enforce_adminslast), tracked via theway-of-working.mdstatus line until done.approved:humanlabel with actor verification, or an/approvecomment command) so a single-account repo can satisfy the 🔴 explicit-approval rule without a second human account.Type emphasis: Feature — user impact (unenforceable review → host-enforced merge gate) + technical decisions (ADR-018's two-check split) are the focus.