diff --git a/.claude/skills/pair-capability-publish-pr/SKILL.md b/.claude/skills/pair-capability-publish-pr/SKILL.md index cf5ce7de..9be753e1 100644 --- a/.claude/skills/pair-capability-publish-pr/SKILL.md +++ b/.claude/skills/pair-capability-publish-pr/SKILL.md @@ -1,17 +1,19 @@ --- name: pair-capability-publish-pr -description: "Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, and updates the board state. Standalone — driven by a handoff/checkpoint, not by /pair-process-implement having run in the same session. Composed by a future closing phase of /pair-process-implement; reused by hotfix and automation loops. Composes /pair-capability-verify-quality, /pair-capability-checkpoint, /pair-capability-write-issue." -version: 0.6.0 +description: "Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, updates the board state, then enters the PR state flow — registers the required `pair-review` check as pending (merge blocked from t0) and dispatches the review to a clean-context subagent. Standalone — driven by a handoff/checkpoint, not by /pair-process-implement having run in the same session. Composed by /implement's closing phase (Step 3.3); reused by hotfix and automation loops. Composes /pair-capability-verify-quality, /pair-capability-checkpoint, /pair-capability-write-issue." +version: 0.5.0 author: Foomakers --- # /pair-capability-publish-pr — Publish a Story Branch as a PR -Take a completed story branch to a review-ready pull request in one standalone step: **gate → compose PR → propagate tags → ready-for-review → board state**. Reliable on a clean context (input is a handoff document, not session memory) and reusable outside `/pair-process-implement` — hotfix branches and automation loops (#212, G10) invoke it directly. +Take a completed story branch to a review-ready pull request in one standalone step: **gate → compose PR → propagate tags → ready-for-review → board state → review dispatch**. Reliable on a clean context (input is a handoff document, not session memory) and reusable outside `/pair-process-implement` — hotfix branches and automation loops (#212, G10) invoke it directly. **One PR per story:** the story lands on ONE branch with ONE PR. If a PR already exists for the branch, this skill UPDATES it — it never opens a second PR for the same story. -**Never merges.** This skill stops at a ready-for-review PR. Merge is a separate, human-gated step (`/pair-process-review` / `/pair-process-implement` Phase 4). +**Never merges.** This skill stops at a PR under review. Merge is a separate, human-gated step (`/pair-process-review` / `/pair-process-implement` Phase 4). + +**The review is never optional.** Every PR this skill publishes enters the [PR state flow](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) as `to-be-reviewed` with the **required** `pair-review` check registered as pending (so the merge is blocked from t0), and the review itself is dispatched to a clean-context subagent (Phase 5). This skill never renders the verdict — it only guarantees a review is pending and mechanically enforced. ## Composed Skills @@ -107,6 +109,32 @@ Each phase follows the **check → skip → act → verify** pattern. Phases run 7. **Act — board state:** update the story's board state on the **PM tool** via the `## State Mapping` (canonical target: `Review`), using `/pair-capability-write-issue` (default write mode, `$status: Review`) when installed. If `/pair-capability-write-issue` is not installed or the PM tool is inaccessible, warn and continue — the PR is already ready. PR state itself is never mirrored onto the board. 8. **Verify**: A single ready-for-review PR exists on the code host, tags reflect the story (or their absence is noted), the cross-link exists in both directions when the tools differ — **exactly one** back-link comment, whether this run posted it or found it (or the missing back-link is reported) — and the board state is updated (or the failure is reported). +### Phase 5: Enter the PR State Flow & Dispatch the Review (AC1) + +The PR is ready; it must now be **under review and mechanically blocked** — see the [PR state flow](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the state model, the required checks, and the host mechanics ([github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § PR state flow). + +1. **Check**: Does the current head commit already carry a `pair-review` check (from an earlier run on the same head)? +2. **Skip**: If a `pair-review` check already exists on this head **and** a review has been submitted for it, do nothing here — the PR is already in the flow (idempotency). A new head commit always needs a fresh check + review. +3. **Act — register the check as pending**: publish `pair-review` on the head commit with a pending status **before** dispatching the review, using the mechanism the host's implementation guide prescribes for an ordinary (non-App) agent token — on GitHub a **commit status**, not a check run. This is what blocks the merge from t0: a crashed or never-started review leaves the required check unsatisfied instead of leaving the PR mergeable. If the publication is **refused** (missing token scope, no status API), do not claim enforcement: report `pair-review: NOT PUBLISHED — advisory` and continue (degradation below). +4. **Act — label the state**: apply `pr-state:to-be-reviewed`, removing any other `pr-state:*` label (exactly one at a time). The label family is provisioned once per repository (implementation guide) and does **not** auto-create; if the label is absent or the label API is unavailable, report it and continue — **non-blocking** (degradation below). The label is a **view** — the required checks are the authority (pr-states.md). +5. **Act — hand the review dispatch to whoever can actually spawn it (AC1)**: the review must run on a **clean context** — an **anonymous** subagent whose prompt is the PR reference only, no context from this session (mechanical isolation, D23: the reviewing context must not inherit the authoring context). **Who spawns it depends on where this skill is running**, because harnesses commonly forbid **nested** subagent dispatch — and this skill is itself frequently running *inside* a subagent (`/pair-process-implement` Step 3.3 publishes the PR from a handoff-only subagent), where spawning a second level would fail on the primary path: + - **This skill is running inside a subagent** (or nested dispatch is unavailable/refused) → do **not** attempt to nest. Emit the signal **`Review: review-dispatch-required — /pair-process-review $pr=`** and return it to the caller; **the caller dispatches** (for `/pair-process-implement`, Step 3.3 in the top-level session — the actor is named there). The `pair-review` check is already `pending`, so the merge stays blocked until the review lands: nothing is lost, and the dispatch happens one frame up where it is allowed. + - **This skill is running at the top level** (invoked directly in a session that can spawn subagents) → spawn the anonymous review subagent here, now. +6. **The dispatch prompt** (identical for either actor) is the PR reference plus a **bounded** instruction — the review must not merge: + + ```text + Run /review $pr= $dispatched=true. + Phases 1–5 only: produce the verdict, publish the `pair-review` check, synthesize the PR state. + NEVER run Phase 6 and NEVER merge, even if the verdict is APPROVED and every gate is green — + the merge is a human act (pr-states.md). Stop after Phase 5 and report. + ``` + + `/pair-process-review`'s own non-interactive contract (its Arguments section) already forbids Phase 6 and defines what to do at its two human prompts (Step 1.4 "Proceed with review?" → continue without asking; Step 5.5 "Merge now?" → always "the author merges"), so a dispatched review neither stalls on an unanswerable question nor self-answers itself into a merge. The prompt states it too: belt and braces, since the constraint is an authorization boundary. + +7. **Act — degraded path (no dispatch anywhere)**: if neither this skill nor its caller can spawn a subagent, do **not** run the review inline in this session (a self-context review defeats the isolation). Leave `pair-review` pending, note `Review: pending — dispatch unavailable, run /pair-process-review in a fresh session` in the output, and post the same instruction as a PR comment. The merge stays blocked meanwhile, so nothing is lost — only deferred. +8. **Act — dispatch failure**: if the subagent fails or times out, the pending `pair-review` check stays in place (merge blocked) and the re-run guidance is posted as a PR comment. Re-invoking this skill re-dispatches (idempotent). +9. **Verify**: The PR carries a `pair-review` check on its head commit, exactly one `pr-state:*` label, and either a dispatched/submitted review or a recorded reason why it is still pending (`review-dispatch-required` counts as recorded **only** because the caller's contract picks it up — see Composition Interface). This skill never publishes a verdict itself and never merges. + ## Output Format ```text @@ -120,17 +148,20 @@ PUBLISH-PR REPORT: ├── Code host: [same as PM tool | (board updates → PM tool)] ├── Cross-link: [n-a (single tool) | Refs: + PR URL posted on | already linked — comment present, not re-posted | back-link failed — manual link needed] ├── Conditional: [Services to Release: N deployable packages / n-a | Screenshots: UI touched / n-a] -└── Board: [→ Review | not updated — reason] +├── Board: [→ Review | not updated — reason] +├── PR state: [pr-state:to-be-reviewed] +└── Review: [dispatched — subagent (clean context) | review-dispatch-required — /review $pr= (caller dispatches) | pending — dispatch unavailable, run /review | already submitted on this head] -RESULT: [PR READY FOR REVIEW | HALTED — ] +RESULT: [PR UNDER REVIEW — merge blocked by required check `pair-review` | HALTED — ] ``` ## Composition Interface -When composed by a future closing phase of `/pair-process-implement` (wired in #256): +When composed by `/pair-process-implement`'s closing phase (Step 3.3): - **Input**: `/pair-process-implement` invokes `/pair-capability-publish-pr` after the last task's commit, passing `$story` (and, when it wrote one, the checkpoint as `$handoff`). `/pair-process-implement` owns task iteration; `/pair-capability-publish-pr` owns the gate→PR→board sequence. - **Output**: The PR number/URL and board-state result flow back to `/pair-process-implement`'s Phase 3 output. A HALTed gate propagates as `/pair-process-implement`'s HALT. +- **Review-dispatch contract (AC1)**: `/pair-process-implement` publishes the PR from a **handoff-only subagent**, so this skill's Phase 5 runs one level down and must not nest a second subagent. It therefore returns **`Review: review-dispatch-required — /pair-process-review $pr=`**, and `/pair-process-implement` Step 3.3 — running in the **top-level** session — spawns the anonymous review subagent with the bounded prompt (phases 1–5, never Phase 6/merge). The `pair-review` check is already `pending` before the return, so the merge is blocked across the handoff. When this skill runs at the top level instead, it dispatches itself and returns `Review: dispatched`. When invoked **independently** (hotfix, automation loop #212): @@ -155,11 +186,18 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **No classification tags on the story**: create the PR without tags and note it (edge case) — never invent tags. - **`/pair-capability-checkpoint` not installed**: gather state from branch + story directly (Phase 0). - **`/pair-capability-write-issue` not installed**: skip the board-state update, warn, leave the PR ready. +- **Nested subagent dispatch unavailable** (Phase 5 — the common case: this skill is itself running in `/pair-process-implement`'s handoff subagent and the harness forbids a second level): return `Review: review-dispatch-required — /pair-process-review $pr=` and let the **caller** dispatch (`/pair-process-implement` Step 3.3). This is the primary path when nested, not a degradation — the review still runs, one frame up, on a clean context. +- **Subagent spawning unavailable anywhere** (Phase 5 — the caller cannot spawn either, or this skill was invoked at top level in an environment without subagents): leave `pair-review` pending, post the re-run instruction as a PR comment, and note the deferral in the output. Never run the review inline in the authoring session (isolation, D23) and never mark the PR reviewed. Merge stays blocked — the outcome is deferred, not skipped. +- **Code host has no check-run/required-check API** (Phase 5): publish the state as a `pr-state:*` label only, note `enforcement: advisory — see host manual setup` in the output, and continue. Documented degradation, never a silent claim of enforcement (pr-states.md). +- **`pair-review` publication refused** (Phase 5 — token without the status scope, host API rejects it): the check is not published, so enforcement is **advisory**; report `pair-review: NOT PUBLISHED — advisory` (with the host error) and still dispatch the review. Never report a blocked merge that is not blocked. +- **`pr-state:*` label absent / no label API** (Phase 5): report `pr-state label: not applied` and continue — **non-blocking**, the required checks remain the merge authority. The labels are provisioned once per repository (host implementation guide); never invent a different label name. +- **`/pair-process-review` not installed**: still register the pending `pair-review` check and label the state, then report that no reviewer flow is available. The PR stays blocked rather than silently mergeable. ## Notes -- This skill **creates git-host artifacts** (a pushed branch, one PR) and updates board state — it does not modify source files and never merges. -- **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). Re-invocation detects the existing PR and updates it in place; detects an existing back-link comment and does not post a second one (Phase 4 step 5's Check — the one composed step that cannot dedupe itself); re-runs the gate (fast if already green); re-parses the handoff. Never a duplicate PR, never a duplicate back-link. +- This skill **creates git-host artifacts** (a pushed branch, one PR, a pending `pair-review` check, a `pr-state:*` label) and updates board state — it does not modify source files, never renders a review verdict, and never merges. +- **Gate ≠ review** ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): the Phase 1 gate is mechanical; the judgment verdict belongs to `/pair-process-review`, dispatched here on a clean context and enforced by the required `pair-review` check (R5.7). +- **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). Re-invocation detects the existing PR and updates it in place; re-runs the gate (fast if already green); re-parses the handoff. Never a duplicate PR. - Tag propagation is a **copy**; the authoritative classification is (re)done in `/pair-process-review` (G6). - The gate here is a local pre-flight only — CI remains authoritative (#210). - The handoff/checkpoint is the input contract (see the [checkpoint template](../../../.pair/knowledge/guidelines/collaboration/templates/checkpoint-template.md)); it is consumed here, never loaded as ambient context elsewhere. diff --git a/.claude/skills/pair-capability-setup-gates/SKILL.md b/.claude/skills/pair-capability-setup-gates/SKILL.md index bdb42653..0c51eccc 100644 --- a/.claude/skills/pair-capability-setup-gates/SKILL.md +++ b/.claude/skills/pair-capability-setup-gates/SKILL.md @@ -1,7 +1,7 @@ --- name: pair-capability-setup-gates -description: "Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24) — for the adopted tech stack. Invoke directly to set up or reconfigure gates; idempotent — confirms existing configuration rather than re-configuring." -version: 0.5.1 +description: "Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24), and the required pair-review + pair-explicit-approval branch-protection checks that make the review unskippable — for the adopted tech stack. Invoke directly to set up or reconfigure gates; idempotent — confirms existing configuration rather than re-configuring." +version: 0.6.0 author: Foomakers --- @@ -125,11 +125,26 @@ Configure CI/CD quality gates for the project. Reads quality assurance guideline - **`TIERING = disabled` (default) → full-suite pipeline**: every PR runs base (install + lint + type + build) **and** unit **and** integration/E2E **and** coverage — all jobs unconditional, no tag resolution, no `risk:*` reads. This is the safe default; do **not** generate the tier-aware jobs. - **`TIERING = enabled` (opt-in) → tier-aware pipeline** following [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md): the pipeline **reads the PR's `risk:*` tag only** (via [`tier-resolve.sh`](../../../.pair/knowledge/assets/tier-resolve.sh) — tags only, no classification criteria of its own, D18) and schedules the [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 matrix: base every tier, unit from 🟡, integration/E2E from 🔴; **untagged/unknown/malformed ⇒ 🔴** (fail-safe); a required suite missing at a tier ⇒ **explicit failure** (`require_suite`), never a silent pass. Do not copy any tier threshold into the generated config — it is a projection of the model, not a second source. - **Coverage job (opt-in)**: emit the `coverage` job from [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) **only when `COVERAGE_GUARDRAIL = enabled`** (Step 3.4). It sources [`coverage-gate.sh`](../../../.pair/knowledge/assets/coverage-gate.sh) (config + a number only, no criteria — D18), reads `tech/coverage-baseline.md`, and blocks a regression below the committed baseline (maintain/improve passes; bootstrap-only when unset). With the flag absent (default) do **not** emit a coverage job in either pipeline mode. - - **Wire required checks**: mark `base` and `secret-scan` as required status checks on the protected branch. In full-suite mode all suite jobs are required; in tier-aware mode the tier-scoped jobs are required-when-scheduled (or via one aggregating gate job). A red gate blocks merge either way. The coverage job (when enabled) is required-when-scheduled like the other tier-scoped jobs. + - **Wire required checks**: mark `base` and `secret-scan` as required status checks on the protected branch. In full-suite mode all suite jobs are required; in tier-aware mode the tier-scoped jobs are required-when-scheduled (or via one aggregating gate job). A red gate blocks merge either way. The coverage job (when enabled) is required-when-scheduled like the other tier-scoped jobs. The two **pair review** checks are wired in Step 4.5 — they are required in both pipeline modes and are not tier-scoped away. - **Post-merge staging** (on merge to the main branch): **build + deploy only, no gate re-run** — the gate already passed pre-merge (identical in both modes). 3. **Act**: Write the secret-scanning job — required, not an optional proposal line, in **both** pipeline modes: resolve the scanner (Argument > Adoption's Custom Gate Registry override > KB default gitleaks), then write the [CI Job Template](../../../.pair/knowledge/guidelines/quality-assurance/security/secret-scanning.md#ci-job-template-github-actions) into the same pipeline file as a `required` job, and provision a starting `.gitleaks.toml` at the project root (see the [allowlist mechanism](../../../.pair/knowledge/guidelines/quality-assurance/security/secret-scanning.md#allowlist-mechanism-adoption-controlled)) if one doesn't already exist. Never write the job with `continue-on-error` — fail-closed is not optional (R6.5). 4. **Verify**: Configuration files written, including the pre-merge pipeline (full-suite by default; tag-only tier-aware resolution + tier-scoped jobs only when `TIERING = enabled`) + required-check wiring, the post-merge staging (build+deploy only) config, the secret-scanning job, and `.gitleaks.toml`. If tiering is enabled, grep the generated pipeline for classification tokens (schema/diff/path heuristics) — there must be none (D18). If disabled, confirm no `risk:*` resolution was generated and every suite job runs unconditionally. +### Step 4.5: Wire the Pair Review Required Checks + Branch Protection + +The gate is only half the merge block. The judgment review must be **unskippable**, which means it is a required check like any mechanical one — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the state model and the two check contracts. This step is **host-agnostic**: the concrete API calls live in the code host's implementation guide (R2.12), e.g. [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § "PR state flow — required checks & branch protection". + +1. **Check**: Does the protected branch already require `pair-review` and `pair-explicit-approval`? Read the current branch-protection configuration from the code host. +2. **Skip**: If both contexts are already required (and stale-review dismissal is on) → confirm and move to Step 5. Idempotent: never re-apply an identical protection payload. +3. **Act — prerequisites first (ordering is load-bearing)**: before writing any protection, (a) **provision the `pr-state:*` labels** from the host guide (they never auto-create; absence is non-blocking but the view is lost), (b) **write the `pair-explicit-approval` job** into the repository's workflows on the **target branch** — neither needs admin scope — and (c) **observe both contexts reporting on a real PR, twice**: at PR open, and again after a review submission (the approval-time re-evaluation), each time **on the PR's head commit** — that is the only commit the host's protection reads. Requiring a context that has never reported leaves **every** PR permanently unmergeable, and a context that reports only at PR open leaves a 🔴 PR blocked *after* the human approves; so protection is applied last, with the host's admin-bypass setting (`enforce_admins` on GitHub) enabled only after one PR has merged through the new rule. +4. **Act — required checks**: add both contexts to the protected branch's required status checks, alongside the gate jobs from Step 4: + - **`pair-review`** — published by `/pair-process-review` on the head commit (pending at PR creation via `/pair-capability-publish-pr`). Required in **both** pipeline modes and at **every** tier: it is never tier-scoped away, exactly like `secret-scan`. A `pending`/absent check blocks the merge, so a review that never ran or crashed cannot yield a mergeable PR (R5.7). + - **`pair-explicit-approval`** — verifies a **non-author human** approval on the current head when the tier requires it (🔴, and untagged ⇒ 🔴 fail-safe); auto-passes at 🟢/🟡 (D10). Generate its job from the host guide's template **unmodified in three respects**, because all three are authorization properties rather than style: it must run from a **trusted ref** (the target branch's version of the job and of any projection it sources — never the pull request's own tree, or the change under review could rewrite the check that authorizes it); its verdict must be **pinned to the PR head commit** on every re-evaluation (including the approval-triggered one), since that is the commit protection evaluates; and it must publish a **pending result as its first step**, so a cancelled or aborted re-evaluation leaves a merge-blocking context instead of a stale `success` from a lower tier. When registering it, **pin the context's producer** where the host allows it (on GitHub: the `checks[].app_id` form, not the legacy `contexts` array) — an unpinned status context can be satisfied by any principal with push access, including the reviewing agent, which would void the 🔴 gate. `pair-review` stays unpinned by construction (no app publishes it): it is an anti-accident control, not an authorization one — see the host guide's "What each context proves". It reads the `risk:*` label only — no classification criteria (D18). **Single-maintainer repositories**: the host rejects a self-approval, so 🔴 PRs cannot satisfy this context without a second human account — say so explicitly and offer to leave the context out of the required list (🔴 rule advisory, recorded) rather than making every 🔴 PR unmergeable. + - Also state the **approval count** explicitly in the protection payload when the host has one (GitHub: `required_approving_review_count: 0`) — the tier-scoped job is the approval authority; an unstated default of ≥1 would demand a human approval on **every** PR, contradicting the 🟢 self-merge row of quality-model §4. +5. **Act — stale-approval invalidation**: enable the host's dismiss-stale-reviews equivalent so a force-push invalidates a previous human approval (pr-states.md edge case). +6. **Act — degraded mode**: if the host has **no required-check / branch-protection API**, or the token lacks permission, do **not** silently continue: report `Required checks: DEGRADED — enforcement advisory` and emit the host guide's **manual** setup steps (which contexts to require, where). Record the gap in [way-of-working.md](../../../.pair/adoption/tech/way-of-working.md)'s Quality Gates section so it is visible until applied. +7. **Verify**: Both contexts are required on the protected branch (or the degradation is reported with manual instructions), the labels exist (or their absence is reported as non-blocking), the `pair-explicit-approval` job is committed to the repository, and stale approvals are dismissed on force-push. Grep the generated `pair-explicit-approval` job for classification tokens — there must be none (D18). + ### Step 5: Provision Shared Lint/Format Config + Hooks 1. **Check**: For each config already detected in Step 1 that is not conflicting, skip provisioning it and note it as already present. @@ -167,6 +182,7 @@ GATE CONFIGURATION COMPLETE: ├── Pre-merge tiering: [disabled (default — full suite every PR) | enabled (risk-tier-scoped, opt-in)] ├── Coverage guardrail: [disabled (default — no coverage job) | enabled (opt-in — coverage job + tech/coverage-baseline.md created)] ├── Required checks: [base + secret-scan required; full-suite: all suite jobs required | tier-aware: tier-scoped jobs required-when-scheduled] +├── Pair review: [pair-review + pair-explicit-approval required on (stale approvals dismissed) | DEGRADED — enforcement advisory, manual setup steps emitted | already configured] ├── Secret Scan: [gitleaks (KB default) | — job + .gitleaks.toml written | already configured] ├── Shared configs: [package list | N/A — non-JS, documented pointer only] ├── Hooks: [husky pre-commit + pre-push | override: | N/A] @@ -200,6 +216,7 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **Full checks are the default; risk-tier reduction is opt-in** (ADL 2026-07-20): unless the project sets `Pre-merge tiering: enabled` in way-of-working.md (Step 3.3 opt-in), the generated pipeline runs the **full suite on every PR** — the safe, pre-tiering behavior. Tiering only *reduces* checks on lower-risk PRs, so it must be enabled explicitly. - **Coverage guardrail is opt-in, off by default** (story #282): the `coverage` regression job and `tech/coverage-baseline.md` are generated **only** when the project sets `Coverage guardrail: enabled` (Step 3.4 opt-in) — the twin of the tiering opt-in. Absent the flag, no coverage job and no baseline file. The baseline is human-committed; the gate bootstraps advisory-only (suggests on stderr, never persists — a CI checkout is ephemeral; automated commit-back is story #372). See [coverage-config-example.md](../../../.pair/knowledge/assets/coverage-config-example.md). - **When tiering is enabled, the generated pipeline reads classification tags only, never classifies** (D18): tier criteria live in [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §3/§4; the pipeline is the deterministic Automation layer that projects the matrix from the PR's `risk:*` tag. Untagged ⇒ 🔴 (fail-safe). See [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md). +- **The pair review is a required check, not a convention** (story #234): Step 4.5 wires `pair-review` + `pair-explicit-approval` on the protected branch so the judgment review is mechanically unskippable (R5.7) and 🔴 PRs need an explicit human approval (D10). The state model, the check contracts, and the degraded mode live in [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md); host API specifics live in the host's implementation guide (R2.12). This skill wires the checks — it never renders a verdict and never computes a PR state. - **Secret scanning is CI config, not a judgment call** (D24, anti-complexity): this skill provisions the job mechanically; it never evaluates whether a diff contains a secret itself — that is gitleaks' (or the adopted scanner's) job at runtime, with no LLM in the loop. `/pair-capability-assess-security` never re-implements this — see that skill's own Notes. - **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). This skill's check: an already-configured project (incl. provisioned shared configs and hooks) is confirmed; update only on explicit developer request. Conflicting local config is always resolved by asking first (see Edge Cases above). - Gate commands must be executable in the project's development environment. Verify commands exist before writing. diff --git a/.claude/skills/pair-process-implement/SKILL.md b/.claude/skills/pair-process-implement/SKILL.md index 795ee2a4..8232bfe2 100644 --- a/.claude/skills/pair-process-implement/SKILL.md +++ b/.claude/skills/pair-process-implement/SKILL.md @@ -305,14 +305,25 @@ The PR is produced on a **guaranteed-clean context**: a fresh subagent whose ent 1. **Check**: Is subagent spawning available in this tool/environment? 2. **Act — primary path (subagent)**: Spawn an **anonymous subagent** whose prompt is the **handoff document only** — the checkpoint contents (or the path `.pair/working/checkpoints/.md`) plus a single instruction: run `/pair-capability-publish-pr $story= $handoff=.pair/working/checkpoints/.md`. Pass **no other session context** — the subagent's context is the handoff and nothing else (clean context / fresh context reset within one execution, R4.1). The subagent runs `/pair-capability-publish-pr` (gate → PR → tags → ready-for-review → board) and returns the PR number/URL and board result. 3. **Act — degraded inline path (AC3)**: If subagent spawning is **unavailable** (tool/environment cannot spawn one — subagent spawning unavailable), do NOT skip the split: the checkpoint is already written (Step 3.2), then compose `/pair-capability-publish-pr` **inline** in the current session (`$story`, `$handoff` = the checkpoint). **Note the degradation in the output** (`Context: degraded — inline publish, no subagent reset`). Behavior is otherwise equivalent to the primary path. -4. **Act — edge case (subagent fails mid-PR)**: The checkpoint remains valid. Re-invoking `/pair-process-implement` re-runs this closing phase; `/pair-capability-publish-pr` is **idempotent** — it detects an existing PR and updates it in place rather than opening a second one. Never open a second PR for the story. -5. **Verify**: A single ready-for-review PR exists (created or updated), and its number/URL is captured for the output. A red quality gate inside `/pair-capability-publish-pr` propagates here as a **HALT** (no PR side effects) — implement does not create the PR itself. +4. **Act — dispatch the review (AC1 of #234; this session is the actor)**: `/pair-capability-publish-pr` Phase 5 registers the `pair-review` check as **pending** (merge blocked from t0) but cannot spawn the review subagent from inside the handoff subagent — nested dispatch is commonly forbidden — so it returns **`Review: review-dispatch-required — /pair-process-review $pr=`**. When that signal comes back, **this top-level session** spawns the anonymous review subagent (a sibling of the publish subagent, not a nested one), prompt = the PR reference plus the bounded instruction: + + ```text + Run /review $pr= $dispatched=true. + Phases 1–5 only: verdict, `pair-review` check, PR-state synthesis. + NEVER run Phase 6 and NEVER merge, whatever the verdict — the merge is a human act. + ``` + + Pass no authoring context (D23 isolation: the reviewer must not inherit the author's assumptions). If `/pair-capability-publish-pr` returns `Review: dispatched` instead (it ran at top level), there is nothing to do here. If this session cannot spawn the reviewer either, record `Review: pending — dispatch unavailable, run /pair-process-review in a fresh session`: the merge stays blocked, so the review is deferred, never skipped. +5. **Act — edge case (subagent fails mid-PR)**: The checkpoint remains valid. Re-invoking `/pair-process-implement` re-runs this closing phase; `/pair-capability-publish-pr` is **idempotent** — it detects an existing PR and updates it in place rather than opening a second one. Never open a second PR for the story. +6. **Verify**: A single ready-for-review PR exists (created or updated), its number/URL is captured for the output, and the review is either dispatched or explicitly recorded as deferred. A red quality gate inside `/pair-capability-publish-pr` propagates here as a **HALT** (no PR side effects) — implement does not create the PR itself. Note: the checkpoint is **not** removed here — it must survive the review/fix loop so a re-review or fix round can resume from it. Cleanup happens at merge (Phase 4). ## Phase 4: Post-Review Merge -After code review approval (typically via `/pair-process-review`), re-invoke `/pair-process-implement` to merge and close — see [post-review-merge.md](./post-review-merge.md) for the verify-approval, merge-commit, merge, parent-cascade, and checkpoint-cleanup steps (Steps 4.1–4.5) plus the completion output. +After code review approval (typically via `/pair-process-review`), re-invoke `/pair-process-implement` to merge and close — see [post-review-merge.md](./post-review-merge.md) for the merge-precondition, merge-commit, merge, parent-cascade, and checkpoint-cleanup steps (Steps 4.1–4.5) plus the completion output. + +This is one of the flow's **two** merge paths (the other is `/pair-process-review` Phase 6), and both carry the **same** blocking precondition: the PR-state synthesis must be `ready-to-merge` (`merge_allowed` from [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh)), re-evaluated on the current head — an approving review alone is not the condition. See [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). On story completion (Done, at merge), the checkpoint is no longer needed — `post-review-merge.md` Step 4.5 removes `.pair/working/checkpoints/.md` so finished-story state never lingers (checkpoint lifecycle: written at the closing phase, cleaned up at merge). @@ -330,6 +341,7 @@ IMPLEMENTATION COMPLETE: ├── Checkpoint: [.pair/working/checkpoints/.md — written] ├── Context: [clean — subagent handoff-only | degraded — inline publish, no subagent reset] ├── PR: [#PR-number — URL — Created | Updated (from /publish-pr)] +├── Review: [dispatched — subagent (clean context) | pending — dispatch unavailable, run /review ] └── Quality: [All gates passing] ``` @@ -347,7 +359,7 @@ Implementation stops immediately when: - **Commit template not found** (Step 2.8 / Step 3.1) — cannot commit without template - **`/pair-capability-publish-pr` not installed** (Step 3.3) — implement composes the PR sequence, never re-implements it - **Quality gate red inside `/pair-capability-publish-pr`** (Step 3.3) — propagates as implement's HALT; no PR side effects (the PR-template-not-found and gate HALTs live in `/pair-capability-publish-pr`) -- **PR not approved** (Step 4.1) — cannot merge without review approval +- **PR state is not `ready-to-merge`** (Step 4.1) — `merge_allowed` fails: red gate, review not approved/still pending, or 🔴 without an explicit non-author human approval on the current head. Never bypass a required check to merge On HALT: report the blocker clearly, propose resolution, wait for developer. @@ -361,7 +373,7 @@ See [idempotency convention](../../../.pair/knowledge/guidelines/technical-stand 4. **Tasks**: scans task checklist and git log to identify completed tasks. Skips them. 5. **PR**: the closing phase re-composes `/pair-capability-publish-pr`, which detects an existing PR and updates it in place — never a duplicate. A subagent that failed mid-PR is recovered this way (the checkpoint stays valid, the rerun is idempotent). 6. **Quality gates**: re-runs all gates (fast if already passing). -7. **Merge**: if PR exists and is approved, proceeds directly to Phase 4 (merge). +7. **Merge**: if a PR exists and its state synthesizes to `ready-to-merge`, proceeds directly to Phase 4 (merge); otherwise Phase 4's Step 4.1 HALTs with the unmet condition. The skill resumes from the first incomplete step — never re-does completed work. @@ -369,7 +381,7 @@ The skill resumes from the first incomplete step — never re-does completed wor See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/graceful-degradation.md) (PM tool not accessible → ask the developer directly; adoption file missing → proceed with guideline defaults) for the standard scenarios. Additional cases: -- **Subagent spawning unavailable**: take the degraded inline path (Step 3.3) — checkpoint still written, `/pair-capability-publish-pr` composed inline, degradation noted in the output. Never skip the checkpoint/PR split. +- **Subagent spawning unavailable**: take the degraded inline path (Step 3.3) — checkpoint still written, `/pair-capability-publish-pr` composed inline, degradation noted in the output. Never skip the checkpoint/PR split. The review dispatch degrades the same way: `pair-review` stays pending (merge blocked) and the re-run instruction is recorded, never an inline self-review. - **`/pair-capability-checkpoint` not installed**: skip the resume probe and the write step; resume from git+PM state (Idempotent Re-invocation) and synthesize the handoff inline for `/pair-capability-publish-pr`. - **`/pair-capability-publish-pr` not installed**: **HALT** — implement composes the gate/PR/board sequence, it never re-implements it. Report the missing dependency. - **`/pair-capability-assess-stack` not installed**: Warn on new dependency, continue without validation. diff --git a/.claude/skills/pair-process-implement/post-review-merge.md b/.claude/skills/pair-process-implement/post-review-merge.md index 89afa627..64a6fe09 100644 --- a/.claude/skills/pair-process-implement/post-review-merge.md +++ b/.claude/skills/pair-process-implement/post-review-merge.md @@ -2,11 +2,14 @@ Disclosed from [SKILL.md](./SKILL.md) Phase 4 — only reached when `/pair-process-implement` is re-invoked after code review approval (typically via `/pair-process-review`), to merge and close the story. -### Step 4.1: Verify Review Approval +### Step 4.1: Merge Precondition — PR state must be `ready-to-merge` (BLOCKING) -1. **Check**: Is the PR approved by the reviewer? -2. **Skip**: If not approved → **HALT**. Wait for review completion. -3. **Verify**: PR has at least one approval. +This is the author-side merge path, and it carries the **same** precondition as the reviewer-side one (`/pair-process-review` Phase 6, Step 6.0): the merge is permitted by the **synthesis**, never by "a review happened". An approving verdict alone is not the condition — a 🔴 PR with an approved review and no explicit human approval must not merge here either (D10). See [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). + +1. **Act**: Re-read the **current** signals on the PR's head commit — gate checks, the review verdict, the `risk:*` tier, and non-author human approvals (they may have changed since the verdict: a new commit, a tier raise, a dismissed approval). +2. **Act**: Synthesize with the shipped evaluator [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh) — `resolve_pr_state `, then `merge_allowed `. Tier comes from the label via `resolve_tier`; the human-approval input uses `human_approval_jq_filter` (non-bot, non-author, on the current head), never a raw approval count. +3. **Skip**: Review not submitted yet → **HALT**, wait for review completion. +4. **Verify**: State is `ready-to-merge` → continue to Step 4.2. Any other state → **HALT**, naming the unmet condition (red gate, review not approved / still pending, or 🔴 without an explicit human approval). Never bypass, dismiss, or re-run a required check to get a green merge button. ### Step 4.2: Prepare Merge Commit Message diff --git a/.claude/skills/pair-process-review/SKILL.md b/.claude/skills/pair-process-review/SKILL.md index 113b16f3..dff3342e 100644 --- a/.claude/skills/pair-process-review/SKILL.md +++ b/.claude/skills/pair-process-review/SKILL.md @@ -1,7 +1,7 @@ --- name: pair-process-review -description: "Reviews a pull request through 6 sequential phases (5 review + optional merge with parent cascade) — validation, technical review, adoption compliance, completeness, decision — to decide whether it merges. Not a quick build/test sanity check (use /pair-capability-verify-quality). Composes /pair-capability-classify, /pair-capability-verify-quality, /pair-capability-verify-done, /pair-capability-record-decision, /pair-capability-analyze-debt, /pair-capability-assess-security (required), /pair-capability-verify-adoption, /pair-capability-assess-stack (optional)." -version: 0.8.0 +description: "Reviews a pull request through 6 sequential phases (5 review + optional merge with parent cascade) — validation, technical review, adoption compliance, completeness, decision — to decide whether it merges. Gate before judgment: a red mechanical gate caps the verdict, and the decision is published as the required `pair-review` check plus the synthesized PR state (to-be-reviewed / ready-to-merge / not-approved), so merge stays blocked until gates are green, the review is approved, and (at risk:red) a human approves explicitly. Not a quick build/test sanity check (use /pair-capability-verify-quality). Composes /pair-capability-classify, /pair-capability-verify-quality, /pair-capability-verify-done, /pair-capability-record-decision, /pair-capability-analyze-debt, /pair-capability-assess-security (required), /pair-capability-verify-adoption, /pair-capability-assess-stack (optional)." +version: 0.5.0 author: Foomakers --- @@ -29,10 +29,21 @@ Review a pull request through 6 sequential phases (5 review + 1 optional merge). ## Arguments -| Argument | Required | Description | -| -------- | -------- | -------------------------------------------------------------------------------- | -| `$pr` | Yes | PR number or URL to review | -| `$story` | No | Story ID for requirements validation. If omitted, extracted from PR description. | +| Argument | Required | Description | +| ------------ | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `$pr` | Yes | PR number or URL to review | +| `$story` | No | Story ID for requirements validation. If omitted, extracted from PR description. | +| `$dispatched` | No | `true` when this run was **dispatched** (spawned by `/pair-capability-publish-pr`'s caller, an automation loop, a CI job) rather than invoked by a human in an interactive session. Default `false`. Sets the **non-interactive contract** below. Never merges (Phase 6 is unreachable). | + +### The non-interactive (dispatched) contract + +Two steps of this flow ask a human a question: Step 1.4 ("Proceed with review?") and Step 5.5 ("Merge now or let the author merge?"). In a **dispatched** run there is nobody to answer, so guessing is not an option in either direction — stalling wastes the run, and self-answering Step 5.5 would let the reviewing agent merge its own APPROVED verdict. With `$dispatched: true` (or whenever the run has no interactive human — e.g. the invocation prompt is a bare `/pair-process-review $pr=` from another agent): + +- **Step 1.4** — do **not** ask. Emit the same READY block as output and continue directly to Step 1.5. +- **Step 5.5** — do **not** ask, and never self-answer "Merge now". The outcome is always option 2 (**author/human merges**): produce the report and stop after Phase 5. +- **A dispatched run never reaches Phase 6**: it does not merge, does not cascade, does not delete a branch, even on APPROVED with `merge_allowed` true. The human merge gate is the point of the flow ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md): "a human still performs the merge; pair never auto-merges"). The dispatch instruction says so explicitly, and this contract holds even if it does not. + +Everything else (phases 1–5, the verdict, the `pair-review` publication, the state synthesis) is identical to an interactive run. ## Session State @@ -94,7 +105,7 @@ REVIEW READY: └── AC: [N criteria to validate] ``` -Ask: _"Proceed with review?"_ +Ask: _"Proceed with review?"_ — **only in an interactive run**. A **dispatched** run (`$dispatched: true`, or no human in the loop) emits this block and continues to Step 1.5 without asking; it never stalls waiting for an answer that cannot come (see the non-interactive contract above). ### Step 1.5: Classification (risk matrix from the diff) @@ -107,10 +118,12 @@ Ask: _"Proceed with review?"_ ### Step 2.1: Quality Gates +**The gate is the first filter** (R5.4, [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): mechanical checks run before judgment, and a red gate **caps** the outcome of this review. + 1. **Check**: Has `/pair-capability-verify-quality` already run on the current PR head commit? 2. **Skip**: If all gates passing on current commit — record results, move to Step 2.2. 3. **Act**: Compose `/pair-capability-verify-quality` with `$scope = all`. -4. **Verify**: Record quality gate results. Failures become review findings (do not HALT — /pair-process-review reports them). +4. **Verify**: Record quality gate results. Failures become review findings (do not HALT — /pair-process-review reports them) **and cap the decision**: with any gate red the review can **never** reach APPROVED / TECH-DEBT and the Step 5.4 synthesis can **never** yield `ready-to-merge` — the judgment review does not override a mechanical failure. Continue the review (the findings are still useful to the author) but carry `Gates: red` into Step 5.2. ### Step 2.2: Code Quality Assessment @@ -223,7 +236,7 @@ Based on compiled findings: | Decision | Condition | | --------------------- | ----------------------------------------------------------------------------------------- | -| **APPROVED** | No critical or major issues. All AC met. Quality gates pass. | +| **APPROVED** | No critical or major issues. All AC met. Quality gates pass (a red gate caps the decision — Step 2.1). | | **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/pair-capability-assess-security` (AC4), failing tests, AC not met. A **red** `cost:*` class does not itself block — it surfaces a **blocking human sign-off** requirement in the Verdict (the human, not the skill, gates on cost). | | **TECH-DEBT** | Only minor issues or debt items. Approve current PR, track debt separately. | @@ -242,21 +255,35 @@ The review is submitted on the **code host only** (where it gates the merge). It 2. **Act**: On re-review, submit a **fresh** native review — both documented paths append (MCP `create`; `gh pr review` CLI), neither edits a submitted body. GitHub's latest-review-governs semantics mean the newest review carries the verdict while earlier reviews stay as visible history, so re-invocation is safe without editing in place (idempotency). 3. **Verify**: The native review is submitted with the verdict-first body — no separate review-comment artifact exists. -### Step 5.4: Determine Next Action +### Step 5.4: Publish the `pair-review` Check & Synthesize the PR State (AC3, AC4, AC5) + +The verdict is judgment; the **merge block is mechanical**. This step turns the verdict into the required check and the PR state — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the model and [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § PR state flow for the host commands. Nothing here re-derives criteria: the tier comes from the PR's `risk:*` label via `resolve_tier` and the per-tier requirements come from [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 (**no classification criteria in this flow — D18**). + +1. **Check**: Does the current head commit already carry a `pair-review` check whose conclusion matches this verdict, and a matching `pr-state:*` label? +2. **Skip**: If yes — nothing to publish, move to Step 5.5 (idempotent re-invocation). +3. **Act — publish the check**: source the shipped [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh) and map the verdict with `review_check_conclusion` (`approved`/`tech-debt` ⇒ `success`, `changes-requested` ⇒ `failure`, anything else ⇒ `pending`). Publish `pair-review` on the **head commit** with that conclusion, through the mechanism the host guide prescribes for an ordinary agent token — on GitHub a **commit status** (the Checks API is writable only by a GitHub App). A `pending` result is never published as resolved — the required check stays unsatisfied so the merge stays blocked. If publication is **refused** (missing scope, no status API), report `pair-review: NOT PUBLISHED — advisory` and continue: the verdict still lives in the native review, but never claim a merge is blocked when it is not. +4. **Act — resolve the requirements for the tier**: read the tier from the PR labels (`resolve_tier`, tags only, **untagged/malformed ⇒ 🔴 fail-safe**), then read that tier's row from quality-model §4 through its `Argument > Adoption > KB default` cascade: reviewer count, SLA, **checklist depth** — `standard` vs `extended` as [quality-model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 defines it (there is no separate extended-checklist artifact: `extended` is the same code-review template with **no section skipped**, "not applicable" written out rather than omitted) — and whether **explicit approval** is required. Record them in the review output; never invent or hardcode a threshold here. At 🔴, state the explicit-approval requirement in the Verdict block so the human reading the PR knows what is still missing (D10). +5. **Act — synthesize the state**: `resolve_pr_state ` — `gates` from Step 2.1, `explicit-approval` = a **non-author human** approval recorded on the current head (the pair review itself never counts; on a single-maintainer repo 🔴 therefore needs a second human account — see pr-states.md). Apply the resulting `pr-state:to-be-reviewed` / `pr-state:ready-to-merge` / `pr-state:not-approved` label, removing any other `pr-state:*` label (exactly one at a time); the labels are provisioned once per repository and if absent this step is **non-blocking** (degradation below). A red gate or a 🔴 tier without explicit approval yields `to-be-reviewed`, never `ready-to-merge` (AC2, AC4). +6. **Verify**: The head commit carries the `pair-review` check (or the publication failure is reported), the PR carries exactly one `pr-state:*` label matching the synthesis (or its absence is reported), and the tier requirements are recorded in the output. The label is a **view** — enforcement is the required checks (R5.7); this skill never edits branch protection and never bypasses a check. + +### Step 5.5: Determine Next Action 1. **Check**: What was the review decision? 2. **Skip**: If CHANGES-REQUESTED → output review report and stop. Author addresses findings, then re-invokes `/pair-process-review`. -3. **Act**: If APPROVED or TECH-DEBT → ask reviewer: +3. **Act — dispatched run**: if this run is **dispatched** (non-interactive — see the contract in Arguments), do **not** ask and do **not** self-answer: the outcome is option 2 below (the human merges). Output the report and stop. A self-answered "Merge now" here would let the reviewing agent merge on its own verdict, which is exactly what the human merge gate forbids. +4. **Act — interactive run**: If APPROVED or TECH-DEBT → ask reviewer: > PR approved. Merge now or let the author merge? > 1. **Merge now** — proceed to Phase 6 > 2. **Author merges** — stop here, author re-invokes `/pair-process-implement` Phase 4 -4. **Verify**: If "Merge now" selected → proceed to Phase 6. Otherwise → output and stop. + Either route enforces the **same** precondition before merging (`merge_allowed` on the re-synthesized state — Step 6.0 here, Step 4.1 there), so routing to the author never skips the 🔴 explicit-approval requirement. + +5. **Verify**: If a human explicitly selected "Merge now" → proceed to Phase 6. Otherwise (including every dispatched run) → output and stop. ## Phase 6: Merge & Close (APPROVED only, optional) -Only reached when the reviewer picked "Merge now" in Step 5.4 — see [merge-and-cascade.md](./merge-and-cascade.md) for the merge-strategy, merge-commit, merge, parent-cascade, branch-cleanup, and post-merge-manual-test steps (Steps 6.1–6.6) plus the completion output. +Only reached when a **human reviewer** picked "Merge now" in Step 5.5 of an **interactive** run — a dispatched run stops at Phase 5 by contract (Arguments § non-interactive) — see [merge-and-cascade.md](./merge-and-cascade.md) for the merge-precondition (Step 6.0: `merge_allowed` on the synthesized PR state — HALT unless `ready-to-merge`), merge-strategy, merge-commit, merge, parent-cascade, branch-cleanup, and post-merge-manual-test steps (Steps 6.1–6.6) plus the completion output. ## Output Format @@ -275,7 +302,10 @@ REVIEW COMPLETE: ├── DoD: [N/N criteria met] ├── Adoption: [Level N — summary] ├── Debt: [N items flagged] -└── Review: [Submitted as native review body — no separate comment] +├── Review: [Submitted as native review body — no separate comment] +├── Check: [pair-review → success | failure | pending (blocks merge)] +├── Tier req.: [🟢/🟡/🔴 — N reviewer(s) / SLA / standard|extended checklist / explicit approval: required-and-present | required-and-MISSING | n-a] +└── PR state: [pr-state:to-be-reviewed | pr-state:ready-to-merge | pr-state:not-approved] ``` At merge (Phase 6): see [merge-and-cascade.md](./merge-and-cascade.md). @@ -287,6 +317,7 @@ Review stops immediately when: - **PR not found or not open** (Phase 1) - **Missing ADR for new technical decision** (Phase 2, Step 2.3) — compose `/pair-capability-record-decision`, then resume - **Unresolved architectural non-conformity** (Phase 3) — must be addressed before decision +- **PR state is not `ready-to-merge`** (Phase 6) — `merge_allowed` fails: gates red, review not approved, or a 🔴 PR without explicit human approval. Report which condition is unmet and stop; never bypass a required check. On HALT: report the blocker, compose the resolution skill if available, wait for developer. @@ -299,7 +330,8 @@ See [idempotency convention](../../../.pair/knowledge/guidelines/technical-stand 3. **Skill compositions**: /pair-capability-verify-quality, /pair-capability-verify-done, /pair-capability-assess-security results cached in session. Not re-run if already passing/current on current commit. 4. **New commits**: if PR updated since last check, re-validates affected phases only. 5. **Review report**: re-review appends a fresh native review (MCP `create` / `gh pr review`); GitHub's latest-review-governs semantics make the newest one carry the verdict while prior reviews remain as history. The report is always the review body, never a separate comment — so no duplicate comment artifact is created. -6. **Merge**: detects already-merged PR. Skips Phase 6 if already merged. Resumes parent cascade if merge succeeded but status updates are incomplete. +6. **PR state & check** (Step 5.4): re-publishing is a no-op when the head commit already carries a `pair-review` check matching the verdict and the `pr-state:*` label already matches the synthesis. A **new head commit** (including a force-push) has no check of its own, so the review re-runs on it and the merge stays blocked meanwhile. A tier raised between runs re-synthesizes on the new tier — raise-only, so a re-run never loosens a requirement. +7. **Merge**: detects already-merged PR. Skips Phase 6 if already merged. Resumes parent cascade if merge succeeded but status updates are incomplete. ## Graceful Degradation @@ -317,12 +349,18 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **PM tool not accessible**: the PR-side work (review, merge) still runs on the code host; the PM-side writes (issue close, parent cascade) are reported as not done rather than guessed. In a single-tool project this is the same tool, so the merge falls back to CLI only. - **Code host declared but unreachable/unauthenticated**: **HALT** with a setup pointer before any review is submitted — there is nothing to review against. PM-side reads already done are not rolled back. - **Merge fails** (conflicts, branch protection): Report the failure, ask reviewer to resolve. Do not force-push or bypass protections. +- **Code host has no check-run/required-check API**: publish the `pr-state:*` label only, report `enforcement: advisory — host manual setup required` (see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) degraded mode), and do NOT claim the merge is blocked. Does NOT HALT. +- **`pair-review` publication refused** (Step 5.4 — token lacks the status scope, host rejects the write): report `pair-review: NOT PUBLISHED — advisory` with the host error; the verdict stays in the native review and the state is still synthesized. Enforcement is advisory until publication works — never asserted otherwise. Does NOT HALT. +- **`pr-state:*` label absent / no label API** (Step 5.4): report `pr-state label: not applied` — **non-blocking**, the required checks remain the authority. Does NOT HALT and does not block Phase 6's `merge_allowed` evaluation, which reads the synthesized state, not the label. +- **🔴 PR on a single-maintainer repository**: the non-author human approval is unobtainable (GitHub rejects a self-approval), so the state stays `to-be-reviewed` and Phase 6 HALTs. Report it as a repository constraint — a second human account is required, or `pair-explicit-approval` must deliberately not be a required context there (pr-states.md edge cases). Never self-approve to unblock. - **/execute-manual-tests not installed**: Skip Step 6.6. Log "Manual test validation skipped — skill not installed." Does NOT block merge. - **No manual test suite**: Skip Step 6.6. Log "No manual test suite found." Does NOT block merge. ## Notes -- This skill **reads code, submits the native review (verdict = the review action), and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). +- This skill **reads code, submits the native review (verdict = the review action), publishes the required `pair-review` check + the `pr-state:*` label, and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). +- **Gate ≠ review** ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): the mechanical gate is the first filter (Step 2.1) and the judgment verdict never overrides it; the merge block itself is the required check, not this skill's opinion. The `pr-state:*` label is a view — enforcement lives in branch protection (R5.7), which this skill never edits or bypasses. +- **Per-tier requirements are read, never invented** — reviewer count, SLA, checklist depth, and the 🔴 explicit-approval requirement come from [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 through its `Argument > Adoption > KB default` cascade (D10). The tier comes from the PR's `risk:*` label (`resolve_tier`, tags only, untagged ⇒ 🔴 fail-safe): **this flow contains no classification criteria** (D18). - Review phases are sequential — each phase builds on findings from prior phases. - The reviewer can stop between phases; re-invoke to resume (see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md)). - Output follows [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) — the template defines structure, /pair-process-review fills it with findings. diff --git a/.claude/skills/pair-process-review/merge-and-cascade.md b/.claude/skills/pair-process-review/merge-and-cascade.md index 1067cdce..6b1dba2d 100644 --- a/.claude/skills/pair-process-review/merge-and-cascade.md +++ b/.claude/skills/pair-process-review/merge-and-cascade.md @@ -1,6 +1,14 @@ # Phase 6: Merge & Close — Detail -Disclosed from [SKILL.md](./SKILL.md) Phase 6 — only reached when the reviewer picked "Merge now" in Step 5.4. +Disclosed from [SKILL.md](./SKILL.md) Phase 6 — only reached when the reviewer picked "Merge now" in Step 5.5, and only executed when the PR state synthesis says `ready-to-merge` (Step 6.0). + +### Step 6.0: Merge Precondition — PR state must be `ready-to-merge` (BLOCKING) + +The merge is permitted by the **synthesis**, not by the reviewer's enthusiasm — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). + +1. **Act**: Re-read the current signals (gates, verdict, `risk:*` tier, human approvals on the current head) and re-run the Step 5.4 synthesis — signals may have changed since the verdict was submitted (a new commit, a tier raise, a dismissed approval). +2. **Check**: `merge_allowed ` (from [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh)). +3. **Verify**: State is `ready-to-merge` → continue to Step 6.1. Any other state → **HALT**, reporting the unmet condition (red gate, review not approved, or 🔴 without explicit human approval). Never bypass, dismiss, or re-run a required check to get a green merge button. ### Step 6.1: Read Merge Strategy @@ -80,6 +88,7 @@ The merge happens on the **code host**; the item writes in Step 6.4 happen on th STORY DONE: ├── Story: [#ID: Title] ├── PR: [#PR-number — merged] +├── PR state: [ready-to-merge — synthesis verified at Step 6.0] ├── Merge: [squash | merge | rebase] ├── Story: Done ├── Epic: [#ID — Done | In Progress (X/Y stories done)] diff --git a/.pair/adoption/tech/adr/adr-018-pr-state-flow-required-checks.md b/.pair/adoption/tech/adr/adr-018-pr-state-flow-required-checks.md new file mode 100644 index 00000000..84ce0e1e --- /dev/null +++ b/.pair/adoption/tech/adr/adr-018-pr-state-flow-required-checks.md @@ -0,0 +1,228 @@ +# ADR-018: PR State Flow — Required Code-Host Checks as the Merge Authority (Labels Are a View) + +## Status + +Accepted + +## Date + +2026-07-28 + +## Context + +- Story #234 (epic #208) delivers the **gate ≠ review** PR flow: mechanical gates first, judgment + review at green gates, and the synthesis of `gates × review verdict × risk tier × explicit approval` + as the PR state (`to-be-reviewed` → `ready-to-merge` / `not-approved`). R5.4 fixes the ordering + (gate is the first filter); R5.7 requires that the pair review be registered as a **required** + code-host check; D10 adds the 🔴 requirement of an explicit human approval. +- The gate side already exists and is deterministic: the pre-merge pipeline (#258) reads the `risk:*` + label only (D18) via the shipped `tier-resolve.sh`, fail-safe 🔴 when untagged, and `verify-quality` + (#259) mirrors it locally through the same helper. +- What did **not** exist is an enforcement point for the review half. Review was documented as "always + runs" (R5.3/D10) but nothing mechanically prevented merging a PR whose review never ran, crashed + mid-flight, or was simply skipped — and nothing prevented merging a 🔴 PR with no human approval. +- pair already exposes state as chromatic **labels** (`risk:*`, `cost:*`). Extending that pattern to + PR state (`pr-state:*`) is natural for humans and for tag-filter automation (`pair-next`), but a + label is trivially mutable by anyone with write access and is not evaluated by branch protection — + so it cannot be the thing that authorizes a merge. +- The review verdict itself is judgment produced by an agent, and the agent must not be able to grant + itself merge rights: the authorization surface has to live outside the reviewing agent. + +## Options Considered + +### Option 1: Labels as the state machine, merge policed by convention + +- **Description**: The review flow applies `pr-state:ready-to-merge`; humans and automation are + expected to only merge PRs carrying that label. +- **Pros**: Zero host configuration; works identically on every code host; visible at a glance. +- **Cons**: Not enforcement — anyone (or any automation, or an agent with a stale view) can merge a + PR whose label is wrong, missing, or hand-applied. A crashed review leaves **no** label at all, which + is indistinguishable from "not yet evaluated" and blocks nothing. Directly violates R5.7 ("the review + can never be skipped") and gives 🔴 PRs no mechanical approval trail for audit. + +### Option 2: A single required check that also encodes the tier requirement + +- **Description**: One `pair-merge-gate` check whose conclusion is the synthesized state — success only + when gates are green, the review approved, and (at 🔴) a human approved. +- **Pros**: One context to require on the protected branch; one place to compute everything. +- **Cons**: Collapses two different authorities into one opaque check: the review verdict (produced by + the reviewing agent) and the human-approval assertion (which must **not** be assertable by that agent). + A reviewing agent that publishes a single success conclusion effectively self-grants the 🔴 human + approval. It also hides *which* condition is unmet in a single red check, and re-evaluating on a label + change would require the agent to re-run rather than a deterministic host job. + +### Option 3: Two required checks — agent-published verdict + deterministic approval job (chosen) + +- **Description**: `pair-review` (published by the review flow on the head commit as a **commit status** + — the Checks API is writable only by a GitHub App installation token, while branch protection accepts a + status context as a required check identically: `success` on + APPROVED/TECH-DEBT, `failure` on CHANGES-REQUESTED, left **pending** when no decision exists) plus + `pair-explicit-approval` (a deterministic host job that reads the `risk:*` label only, auto-passes + below 🔴, and at 🔴 asserts a non-bot, non-author approving review on the current head). Both are + required status checks; `pr-state:*` labels are a **view** of the same synthesis. +- **Pros**: The merge authority is branch protection, not an agent's opinion or a mutable label; the + agent cannot satisfy the human-approval requirement **as long as the context's producer is pinned** + (`required_status_checks.checks[].app_id` — see the third property below; it is asserted by a + deterministic job reading the host's own review records, and a status posted by the agent's own token + does not count); a crashed/never-run review leaves a **pending** required check, so the + fail-safe direction is "blocked", not "open"; per-head semantics give force-push invalidation for free; + a tier raise re-evaluates via the `labeled`/`unlabeled` trigger already introduced in #258; each unmet + condition is legible as its own red/pending check. +- **Cons**: Two contexts to wire (mitigated: `setup-gates` Step 4.5 does it and is idempotent); hosts + without a required-check API degrade to advisory enforcement (documented degraded mode, reported + rather than silently assumed); the checks are host artifacts, so the host mechanics live in an + implementation guide instead of the host-agnostic model. + +### Option 4: A separate reviewer identity (second human account, or a GitHub App) + +- **Description**: The reviewing agent authenticates as its **own** identity — a second user account or a + GitHub App installation — instead of as the author. Its native review is then a real `APPROVE` event by + a non-author, and the 🔴 explicit-approval requirement could be satisfied by a reviewer that is not the + author (with the human approval still required as a second, human-typed signal, or replaced by it). +- **Pros**: Removes the two constraints this design accepts — the verdict would be a native APPROVE + instead of a `--comment` degradation, and the App path additionally unlocks the Checks API + (`checks: write`), so `pair-review` could be a real check run rather than a commit status. It also makes + "who reviewed" auditable per-identity rather than per-body-token. +- **Cons / why not now**: An identity is **project infrastructure**, not a code change — it needs a + second seat (or an App registration, install, private key, and a secret store), which the flow cannot + provision for an adopting project and which a solo-maintainer repository may not want to pay for. It + also does **not** by itself satisfy the 🔴 rule as specified: a bot/App approval is exactly what + `pair-explicit-approval` excludes (`user.type == "User"`), so adopting it would mean *changing the rule* + (deciding that a distinct non-human reviewer identity may grant the 🔴 gate) — a design decision, not a + configuration one. **Deferred, explicitly**: it is the same missing ingredient as the second-human-account + constraint below, and it belongs to the same follow-up story as the solo-approval token, where the two + options (human token vs. distinct reviewer identity) are weighed together instead of one being adopted + by accident: [#398](https://github.com/foomakers/pair/issues/398). + +## Decision + +**Adopt Option 3.** The merge authority is the set of **required code-host checks**; the `pr-state:*` +label is an advisory view that can never enable a merge. Concretely: + +- The **model** — three states, the synthesis table, the check contracts, fail-safe and edge cases — + lives in one KB guideline, `guidelines/collaboration/project-management-tool/pr-states.md`, the + pull-request companion to `canonical-states.md`. +- The **synthesis is executable and shared**: `assets/pr-state.sh` (`resolve_pr_state`, `merge_allowed`, + `explicit_approval_required`, `review_check_conclusion`, `human_approval_jq_filter`), provider-agnostic + and composed by the skills + and by host jobs alike — the same "one executable projection" pattern as `tier-resolve.sh`, so the + flow cannot drift between local reasoning and CI. The 🔴 approval **predicate** is part of that + projection on purpose: shipped as `human_approval_jq_filter`, the host workflow and the tests that + verify it read one text, so authorization logic cannot exist only as prose in a doc code block with a + hand-copied twin in a test. +- **No criteria in the flow** (D18): the tier is read from the `risk:*` label via `resolve_tier`; + per-tier requirements (reviewers, SLA, checklist depth, explicit approval) are read from + `quality-model.md` §4 through `Argument > Adoption > KB default`. Untagged/malformed ⇒ 🔴 (fail-safe), + consistent with the gate side. +- **Gate before judgment** (R5.4): a red gate caps the review decision and the synthesis can never + return `ready-to-merge` while any mechanical check is red. +- **Host specifics** (check-run publication, branch-protection payload, the `pair-explicit-approval` + job, manual/degraded setup) live in `github-implementation.md` (R2.12), not in the model. +- **The explicit-approval job is an authorization control, and is designed as one.** "Deterministic" + is only true if its code cannot come from the change it authorizes, so two properties are part of the + decision, not implementation detail: + - **Trusted ref.** The job and every projection it sources are read from the **target branch** + (`pull_request_target` + a checkout pinned to `base.sha`), never from the PR's tree. Verified by + counter-example: with the pre-fix `pull_request` + default-checkout form, a `risk:red` PR carrying a + neutered `explicit_approval_required` published `pair-explicit-approval=success` on its own head — + a self-granted 🔴 gate. With the base-pinned form the identical tamper changes nothing. + - **Head-pinned verdict.** The result is published as a commit status on + `github.event.pull_request.head.sha` — the only commit branch protection evaluates. `$GITHUB_SHA` is + *not* that commit for either trigger (measured: base tip for `pull_request_target`, the + `refs/pull//merge` commit for `pull_request_review`), so a verdict published against it would + leave a 🔴 PR blocked *after* the human approves. + - **Pinned producer + pending-first.** A commit status is writable by **any** principal with push + access, for any context name, and protection evaluates the most recent status per context — so an + unpinned context is satisfied by one `POST …/statuses/$HEAD_SHA -f context='pair-explicit-approval'` + from the very token the reviewing agent holds (it needs `repo:status` to publish `pair-review`). The + protection payload therefore uses the **`checks` form with `app_id`** pinned to the producer + (GitHub Actions), not the legacy `contexts` array, and the job publishes **`pending` as its first + step** so a cancelled/aborted re-evaluation cannot leave a stale lower-tier `success` standing. + Residual, recorded rather than assumed away: a PR that *adds* a workflow requesting + `statuses: write` can still post as the Actions app — narrowed by the two companion repository + settings (default `GITHUB_TOKEN` permissions read-only, `CODEOWNERS` + `require_code_owner_reviews` + on `/.github/workflows/**`), and eliminated only by the GitHub-App/check-run form (Option 4). +- **`pair-review` is an anti-accident control, not an authorization control** — stated here because the + Pros above are otherwise easy to over-read. It is published with the authoring/reviewing agent's own + token, so it is **forgeable by any push-access principal by construction** (an `app_id` pin is + impossible: no app publishes it). What it guarantees is that a review which never ran, crashed, or was + skipped leaves the context absent/pending and the merge blocked — i.e. it removes the *accident*, not + the *forgery*. This is strictly stronger than the label of Option 1 in exactly one respect (an + absent/pending status blocks, a missing label does not), and no stronger in mutability. The + identity/authorization half is `pair-explicit-approval`'s, which is why the design keeps two checks. + A genuinely unforgeable verdict requires a distinct GitHub App publishing a check run whose `app_id` + the repository's own workflows cannot present — Option 4, deferred with + [#398](https://github.com/foomakers/pair/issues/398). + + A project unwilling to enable `pull_request_target` gets the same guarantee by inlining the projection + into the job (no checkout at all), at the cost of a second copy to keep in sync. +- **The review is dispatched, never nested, and never merges.** `/pair-capability-publish-pr` runs inside `/pair-process-implement`'s + handoff subagent, so it returns a `review-dispatch-required` signal and the **top-level session** + spawns the reviewing subagent (harnesses commonly forbid nested delegation, which would have made the + documented primary path unreachable). The dispatch prompt is bounded to phases 1–5, and `/pair-process-review` + carries a **non-interactive contract** for its two human prompts so a dispatched review neither stalls + nor self-answers itself into a merge. The merge stays a human act at every tier. + +## Consequences + +- `publish-pr` registers `pair-review` as pending **before** dispatching the review to a clean-context + subagent, so a PR is blocked from t0 and a dispatch failure defers the review instead of skipping it. +- `review` gains a step that publishes the check conclusion, resolves the tier requirements, and + synthesizes/labels the state; its Phase 6 merge is preconditioned on `merge_allowed` and HALTs + otherwise. The reviewing agent never edits branch protection and never bypasses a check. +- **Both merge paths carry the same precondition, not just the reviewer's.** The flow has two mergers — + `/pair-process-review` Phase 6 (Step 6.0) and `/pair-process-implement` Phase 4 (Step 4.1, the author + re-invoked after approval, which is where every *dispatched* review routes by contract). A precondition + on only one of them is no precondition: the same re-synthesis + `merge_allowed` HALT is required in + both, and both are asserted by the conformance suite. This matters most in the documented advisory + mode (protection not yet applied), where nothing host-side blocks. +- `setup-gates` wires both contexts (idempotently) and reports **degraded** enforcement when the host + or token cannot configure protection — the degradation is visible, never assumed away. +- 🔴 PRs carry a mechanical, auditable explicit-approval trail (a human, non-author approving review on + the merged head), satisfying the compliance angle of the story without a new artifact. +- **Consequence accepted knowingly: a 🔴 merge requires a second human account.** GitHub rejects an + approving review from the PR author, so on a single-maintainer repository (including this one) no 🔴 PR + can reach `ready-to-merge` while `pair-explicit-approval` is a required context. Such a project either + adds a second human reviewer account or deliberately leaves that context out of the required list and + records the 🔴 rule as advisory. An alternative solo-approval token (a human-applied `approved:human` + label with actor verification, or an `/approve` comment command) is a **design change tracked as + [#398](https://github.com/foomakers/pair/issues/398)** — intentionally absent here rather than + half-specified. +- **Ordering is part of the decision**: the workflow job and the `pr-state:*` labels must exist and be + observed reporting on a real PR *before* the protection is written (and `enforce_admins` enabled), + otherwise a required context that never reports blocks every merge with no escape hatch. +- Adding a code host means adding an implementation-guide section, not touching the model or the + evaluator. Hosts lacking required checks remain usable in advisory mode. +- **A distinct reviewer identity stays out of scope**, deliberately (Option 4): the dispatched reviewer + runs as the author's account, so its verdict is a `--comment` review by construction, and a second + human account remains the only way to satisfy 🔴. The identity question is recorded here so it is not + rediscovered per project; it is weighed in the same follow-up story as the solo-approval token + ([#398](https://github.com/foomakers/pair/issues/398)). +- **Merge blocking was verified end-to-end on a live code host** (throwaway repository, 2026-07-30): + labels → workflow → contexts observed on the head commit → protection applied with + `enforce_admins: true` → one PR per tier. Observed: a **pending** `pair-review` blocks + (`Required status check "pair-review" is pending.`), a **failing** one blocks, an **absent** one blocks + as "expected", 🟢 with both contexts green **merges**, 🔴 without a human approval blocks, a 🟡→🔴 raise + re-blocks within one run, a `pull_request_review` submission re-reports the approval context **on the + same head SHA**, and the tampered PR above stays blocked. The evidence table lives in + `github-implementation.md` § "Verified on a throwaway repository". It is a **point-in-time + observation**, not re-auditable evidence: the sandbox was disposable and is gone, so no run URL or API + dump survives, and the rows are re-verified by re-running the documented ordering steps on the adopting + repository (that is stated in the table's own preamble). The producer pin, the companion settings and + the pending-first step were added after that session and are explicitly **not** covered by it. + Applying protection to *this* repository remains a human act (admin scope) — see the way-of-working + status line. +- Verification split per the gate-tooling ADL (2026-07-13): `pr-state.sh` behavior is executed by + `scripts/smoke-tests/scenarios/pr-state-flow.sh`; the content invariants of the guideline and the + skills are asserted in `packages/knowledge-hub/src/conformance/pr-state-flow.test.ts`. +- Applying the branch protection to a real repository (including this one) is a **repo-configuration** + act with admin scope — out of a code change's reach and left to a human, which is why `setup-gates` + reports the degraded state rather than assuming success. + +## Adoption Impact + +- `way-of-working.md`: Quality Gates section gains the required-check line for the two pair checks + (surfacing; the same place the tiering and coverage-guardrail flags live). +- KB: new `pr-states.md` + `assets/pr-state.sh`; `quality-model.md` §4 gains a pointer to the flow + (criteria stay in the model); `github-implementation.md` gains the host recipe. +- Docs site: new `concepts/pr-state-flow` page, cross-linked from `concepts/tag-driven-gates`. diff --git a/.pair/adoption/tech/way-of-working.md b/.pair/adoption/tech/way-of-working.md index 69560867..6ec2a7c2 100644 --- a/.pair/adoption/tech/way-of-working.md +++ b/.pair/adoption/tech/way-of-working.md @@ -43,6 +43,7 @@ Maps this project's GitHub Projects board columns to the 5 canonical macrostates - Quality gate includes: type checking (`ts:check`), testing (`test`), linting (`lint`), formatting (`prettier:fix`), markdown lint (`mdlint:fix`). - **Pre-merge tiering**: `disabled` (default) — every PR runs the full pre-merge check suite. Set to `enabled` to opt into risk-tier-scoped pre-merge checks (lighter checks on lower-risk PRs) per [tier-aware-pipeline.md](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. - **Coverage guardrail**: `enabled` — pair dogfoods its own capability: the [`Coverage guardrail` step](../../../.github/workflows/ci.yml) in CI sources [`coverage-gate.sh`](../../knowledge/assets/coverage-gate.sh), extracts the line-coverage % from each package's istanbul `coverage-summary.json`, and blocks a PR whose coverage drops below the human-committed baseline in [`tech/coverage-baseline.md`](./coverage-baseline.md) (maintaining/improving passes — not an absolute wall). The framework **default remains `disabled`** (the dataset template ships off); this line is pair's project-level opt-in only. See [coverage guardrail](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) + [config format](../../knowledge/assets/coverage-config-example.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. **Coverage baseline commit-back**: `disabled` — the separate, nested opt-in ratchet (#372, framework default also `disabled`): when `enabled`, a **push to the base branch** (never a PR run, never a fork) proposes a raised `baseline.` as a **bot pull request** from `chore/coverage-baseline-ratchet`, never a push to `main`, and requires a repo-scoped `COVERAGE_RATCHET_TOKEN` (`contents: write` + `pull requests: write`, no protection bypass) — without it the step warns and the gate's verdict is unchanged. It stays `disabled` here until story #234's branch protection is applied and that secret is provisioned (ADR-018 lands with that story, so it is not linked from here yet); see ADL [2026-07-30-coverage-ratchet-pr-not-push.md](../decision-log/2026-07-30-coverage-ratchet-pr-not-push.md). +- **Pair review required checks**: `pair-review` + `pair-explicit-approval` are the required status checks that make the judgment review unskippable (R5.7) and enforce the 🔴 explicit-human-approval rule (D10) — see [pr-states.md](../../knowledge/guidelines/collaboration/project-management-tool/pr-states.md) and [ADR-018](adr/adr-018-pr-state-flow-required-checks.md). Status on this repo: **not yet applied** — writing branch protection needs admin scope, so it is a deliberate human step; until applied, enforcement here is advisory (the documented degraded mode). **Ordering constraint** (applies in this order, or every merge stops): 1. provision the `pr-state:*` labels + add the `pair-explicit-approval` workflow (neither needs admin scope — this repo has not added the workflow yet, so the context does not report today); 2. confirm on a real PR that `pair-review` and `pair-explicit-approval` both report on the head commit, **and** that the approval context re-reports on that same head SHA after a review submission; 3. only then `PUT` the branch protection, keeping `enforce_admins` off until one PR has merged through it. The whole sequence (including the merge-block outcomes per tier) was executed on a throwaway repository — see `github-implementation.md` § "Verified on a throwaway repository" — so what remains here is applying it, not discovering whether it works. **This repo is single-maintainer**, so a 🔴 PR cannot satisfy `pair-explicit-approval` (GitHub rejects a self-approval): a second human reviewer account is a prerequisite for making that context required here — otherwise leave it out of the required list and keep the 🔴 rule advisory. The solo-maintainer alternative (a verified human approval token instead of a second account) is tracked as [#398](https://github.com/foomakers/pair/issues/398). **When the protection is written here, use the `checks` form with `app_id` pinned** for `pair-explicit-approval` (an unpinned status context is satisfiable by any push-access token, including the agent's); `pair-review` stays unpinned and is an anti-accident control, not an authorization control — see `github-implementation.md` § "What each context proves". - **Gate & tooling code:** a gate's logic lives in a tested module in its owning package (white-box unit tests); scripts/CLIs are thin entrypoints and a root gate delegates (`pnpm --filter `). Scripts are never unit-tested — CLI-level checks go to smoke tests. See ADL [2026-07-13-gate-tooling-code-in-tested-modules.md](../decision-log/2026-07-13-gate-tooling-code-in-tested-modules.md). Gate/tooling packages are organized by bounded context, not one package per tool family — a new tool family sharing an existing package's bounded context is a new folder there, not a new package. See [ADR-014](adr/adr-014-tool-package-boundary-by-bounded-context.md). - **Conformance tests** (`packages/knowledge-hub/src/conformance/`): one test file per target KB artifact (a `SKILL.md`, guideline, or template), not per introducing story — a new story extends the matching file's `describe` block instead of adding a new story-named file. See ADL [2026-07-18-conformance-test-per-file-not-per-story.md](../decision-log/2026-07-18-conformance-test-per-file-not-per-story.md). - **Monorepo tooling gotchas** (e.g. `pnpm --filter` bypassing turbo's `dependsOn` graph on a fresh checkout): documented once, centrally, in `DEVELOPMENT.md`'s `Turbo Caching` section — affected packages' READMEs carry only a short pointer, not a full copy. See ADL [2026-07-18-workspace-gotcha-doc-placement.md](../decision-log/2026-07-18-workspace-gotcha-doc-placement.md). diff --git a/.pair/knowledge/assets/pr-state.sh b/.pair/knowledge/assets/pr-state.sh new file mode 100644 index 00000000..ba57d698 --- /dev/null +++ b/.pair/knowledge/assets/pr-state.sh @@ -0,0 +1,140 @@ +#!/usr/bin/env bash +# pr-state.sh — provider-agnostic PR-state synthesis for the gate≠review merge flow. +# +# THIS FILE READS STATE SIGNALS AND CLASSIFICATION TAGS ONLY. It contains NO +# classification criteria (D18): it never inspects the diff, the code, file paths, +# or change size. The tier is decided upstream by `classify` from the quality +# model, carried on the PR as a `risk:*` label, and resolved into `green|yellow|red` +# by the sibling helper `tier-resolve.sh` (`resolve_tier`). This script is the +# deterministic Automation layer of the quality model's three-layer principle: +# it synthesizes already-produced signals and nothing else. +# +# Gate ≠ review (R5.4): the mechanical gate is the FIRST filter — a non-green gate +# can never produce a merge-enabling state, no matter what the judgment review said. +# +# Fail-safe: an unknown gate/review signal is treated as not-passing, and an absent, +# unknown, or malformed tier is treated as `red` (the strictest requirements) — never +# a silent `ready-to-merge`. See: +# .pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md +# .pair/knowledge/guidelines/quality-assurance/quality-model.md (§3.2 fail-safe, §4 per-tier requirements) +# +# Usage (in a code host's automation, or by /pair-process-review): +# source tier-resolve.sh # tags only +# source pr-state.sh +# TIER="$(resolve_tier "$PR_LABELS")" +# STATE="$(resolve_pr_state "$GATES" "$REVIEW" "$TIER" "$EXPLICIT_APPROVAL")" +# merge_allowed "$STATE" || exit 1 + +# resolve_pr_state +# gates : pass | fail | pending | +# review : approved | tech-debt | changes-requested | pending | missing | error +# (`tech-debt` is an approving verdict — approve now, track the debt +# separately; anything unrecognized is treated as no decision yet) +# tier : green | yellow | red | +# explicit_approval : 1 when a human approval is recorded on the CURRENT head, else 0 +# +# Echoes exactly one of: to-be-reviewed | ready-to-merge | not-approved. Always exits 0. +resolve_pr_state() { + local gates="${1:-}" review="${2:-}" tier="${3:-}" approval="${4:-0}" + + # A changes-requested verdict is the one state that routes to a human author, + # whatever the gate says — the finding list is the actionable output. + if [ "$review" = "changes-requested" ]; then + echo "not-approved" + return 0 + fi + + # Gate first (R5.4): review never unlocks a merge over a non-green gate. + if [ "$gates" != "pass" ]; then + echo "pr-state: gates are not green ('${gates:-unknown}') — no merge-enabling state (gate is the first filter)" >&2 + echo "to-be-reviewed" + return 0 + fi + + # Review must be an approving verdict; pending/missing/error/unknown stay blocked. + case "$review" in + approved | tech-debt) ;; + *) + echo "pr-state: review is '${review:-unknown}', not an approving verdict — staying to-be-reviewed (fail-safe)" >&2 + echo "to-be-reviewed" + return 0 + ;; + esac + + # Tier requirements: 🔴 (and any unknown tier, fail-safe) needs explicit human approval. + if explicit_approval_required "$tier" && [ "$approval" != "1" ]; then + echo "pr-state: tier '${tier:-unknown}' requires explicit human approval — staying to-be-reviewed" >&2 + echo "to-be-reviewed" + return 0 + fi + + echo "ready-to-merge" +} + +# explicit_approval_required — exit 0 (required) for red and for any +# unknown/absent tier (fail-safe), exit 1 (not required) for green/yellow. +# The requirement itself is the quality model's §4 row, not a rule invented here. +explicit_approval_required() { + case "${1:-}" in + green | yellow) return 1 ;; + red) return 0 ;; + *) + echo "pr-state: unknown tier '${1:-}' — requiring explicit approval (fail-safe red)" >&2 + return 0 + ;; + esac +} + +# merge_allowed — exit 0 ONLY for ready-to-merge; any other state exits 1 +# with the reason on stderr. Never a silent pass. +merge_allowed() { + case "${1:-}" in + ready-to-merge) return 0 ;; + not-approved) + echo "pr-state: state is 'not-approved' — merge blocked, findings go back to the author" >&2 + return 1 + ;; + *) + echo "pr-state: state is '${1:-unknown}' — merge blocked (gates green + approved review required; explicit approval at 🔴)" >&2 + return 1 + ;; + esac +} + +# human_approval_jq_filter — echoes the ONE jq predicate that decides whether an +# explicit human approval (D10) exists on the CURRENT head commit. It ships here, +# next to the synthesis it feeds, instead of being written inline in each code host's +# workflow: the host job and its tests then read the SAME text, so the authorization +# predicate cannot drift between the recipe and what is verified ("one executable +# projection", as for tier-resolve.sh). +# +# Input : a REST `GET /repos/{owner}/{repo}/pulls/{n}/reviews` payload (an array). +# Env : HEAD_SHA (the only commit branch protection evaluates), PR_AUTHOR (login). +# Output : one line per qualifying review id — count them; and always read ALL pages +# (`--paginate`), since an approval can sit past page 1. +# +# Rejects by construction: a non-APPROVED review, an approval on any other commit +# (i.e. stale after a force-push), a non-human account (`user.type != "User"` — bots +# and GitHub Apps, so the pair review itself can never satisfy the gate), and the PR +# author's own approval. +human_approval_jq_filter() { + printf '%s' '.[] | select(.state=="APPROVED" and .commit_id==env.HEAD_SHA and .user.type=="User" and .user.login!=env.PR_AUTHOR) | .id' +} + +# review_check_conclusion — maps a review verdict onto the conclusion the +# REQUIRED `pair-review` check must carry on the code host (R5.7): +# approved | tech-debt ⇒ success +# changes-requested ⇒ failure +# anything else (pending, missing, crashed, timed out, unknown) ⇒ pending +# A pending check blocks the merge exactly like a failing one — a crashed or skipped +# review can never leave the PR mergeable. +review_check_conclusion() { + case "${1:-}" in + approved | tech-debt) echo "success" ;; + changes-requested) echo "failure" ;; + *) + echo "pr-state: review verdict '${1:-}' is not a decision — check stays pending (blocks merge)" >&2 + echo "pending" + ;; + esac +} diff --git a/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md b/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md index 258687f3..e45f8f1f 100644 --- a/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md +++ b/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md @@ -34,6 +34,8 @@ This framework does not cover: **[definition-of-ready-and-done.md](definition-of-ready-and-done.md)** - Canonical Definition of Ready (R3.8, 6 criteria) and Definition of Done (R3.9, 4 criteria + per-tier hooks) — companion to canonical-states.md, the source `/pair-capability-verify-done` and the user-story template read +**[pr-states.md](pr-states.md)** - The PR state flow (gate ≠ review): the 3 PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`), the synthesis of gates × review verdict × tier × explicit approval, and the required `pair-review` / `pair-explicit-approval` checks that make merge blocking mechanical — the pull-request companion to canonical-states.md + ### Implementation Guides **[filesystem-implementation.md](filesystem-implementation.md)** - Complete setup and usage guide for filesystem-based project management diff --git a/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md b/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md index 6b2f15a9..57c56d1f 100644 --- a/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md +++ b/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md @@ -193,6 +193,7 @@ Rollout across the rest of the skill catalog happens organically in the stories ## Related +- [pr-states.md](pr-states.md) — the **pull-request** companion to this document: the three PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`) all live *under* the `Review` macrostate; `Done` is produced by the merge, never by a PR state - [way-of-working.md](../../../../adoption/tech/way-of-working.md) — hosts the optional `## State Mapping` section - [github-implementation.md](github-implementation.md) · [azure-devops-implementation.md](azure-devops-implementation.md) · [filesystem-implementation.md](filesystem-implementation.md) — PM tool status-field mechanics - [decision-records.md](../decision-records.md) — ADR/ADL process (this schema was adopted via ADR) diff --git a/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md b/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md index 14936c43..a12bcb18 100644 --- a/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md +++ b/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md @@ -607,8 +607,285 @@ mcp__github__issue_write: gh issue close [NUMBER] --reason completed ``` +## PR state flow — required checks & branch protection + +GitHub mechanics for the two pair checks the [PR state flow](pr-states.md) requires. The **model** (states, synthesis, tier requirements, edge cases) lives in that document; only host specifics live here (R2.12). + +### The two pair checks + +| Check | Published by | Semantics | +| --- | --- | --- | +| `pair-review` | `/pair-process-review`, as a **commit status** (registered `pending` at PR creation by `/pair-capability-publish-pr`) | `success` on APPROVED/TECH-DEBT, `failure` on CHANGES-REQUESTED, `pending` when the review has not produced a decision (never ran, crashed, timed out) | +| `pair-explicit-approval` | a workflow job (below), also as a **commit status pinned to the PR head SHA** | `success` when the tier does not require explicit approval, or when a **human** approving review exists on the current head; `failure` at 🔴 without one | + +Both are **required status checks** — a `pending` or absent required check blocks the merge on GitHub exactly like a failing one, which is what makes the review unskippable (R5.7). + +#### What each context proves — and what it does not + +A commit status is written by `POST /repos/{owner}/{repo}/statuses/{sha}`, which **any principal with push access can call, for any context name**, and branch protection evaluates the **most recent** status per context. There is no producer-identity restriction on a status context unless one is pinned in the protection payload. Read the two checks accordingly: + +| Context | What it guarantees | What it does **not** guarantee | +| --- | --- | --- | +| `pair-review` | **Anti-accident**: an absent or `pending` context blocks, so a review that never ran, crashed, or was skipped can never leave the PR mergeable. Verified on a live host. | **Unforgeability.** It is published with the ordinary agent token, so by construction anyone with push access — including the authoring agent — can `POST` `state=success` for the context. It is **not** an authorization control; treat it as "the review cannot be silently skipped", never as "someone independent approved". | +| `pair-explicit-approval` | **Authorization**, to the extent the producer is pinned: the payload below pins the context to the **GitHub Actions app id**, so a status posted by a user token or PAT does not satisfy it, and the job that does post it runs from a trusted ref (property 1 below). | Absolute unforgeability — see the residual immediately below. The only form with no residual is a **GitHub App check-run** whose `app_id` is pinned to an App the repository's own workflows cannot present (Option 4 in ADR-018 — deferred, [#398](https://github.com/foomakers/pair/issues/398)). | + +**Residual on the app pin, and the companion repository settings that shrink it.** A status posted by _any_ workflow using the default `GITHUB_TOKEN` is attributed to the GitHub Actions app, so a pull request that **adds** a `pull_request` workflow declaring `permissions: statuses: write` can post the pinned context itself. Two settings close that in practice — apply both when the flow is used as an authorization control: + +- **Settings → Actions → General → Workflow permissions: _Read repository contents and packages permissions_.** This removes the implicit write token, so any workflow needing `statuses: write` must request it explicitly in its own YAML — visible in the diff instead of ambient. (It does not _prevent_ an explicit request; that is what the next setting is for.) +- **Require review on workflow changes**: a `CODEOWNERS` entry for `/.github/workflows/**` plus `"required_pull_request_reviews": { "require_code_owner_reviews": true }`, so a PR that edits or adds a workflow needs a human code-owner approval before it can merge — the same human signal the 🔴 gate is asking for. + +Stated plainly rather than assumed away: with neither setting applied, `pair-explicit-approval` is hardened (no PAT/user-token path) but not unforgeable, and `pair-review` is forgeable by design. The unforgeable form is the App/check-run path. + +**Variables used by every snippet in this section** — defined once, and every call below uses the **same single-variable form** `repos/$REPO/…` (never a two-variable owner+name form, which is what produces `repos/owner/owner/repo/…` and a 404 when `REPO` already holds `owner/repo`): + +```bash +REPO="$(gh repo view --json nameWithOwner -q .nameWithOwner)" # owner/repo — matches `github.repository` +PR= +HEAD_SHA="$(gh pr view "$PR" --json headRefOid -q .headRefOid)" +``` + +**Token prerequisite (why a commit status, not a check run).** The Checks API (`POST /repos/{owner}/{repo}/check-runs`) is writable **only by a GitHub App installation token**: with an ordinary user token or PAT it answers `403 You must authenticate via a GitHub App`. The skills that publish the verdict (`/pair-capability-publish-pr` Phase 5, `/pair-process-review` Step 5.4) run agent-side with exactly that ordinary token, so a check run is not an option for them. The **commit-statuses API** accepts the same token and branch protection treats a status **context** as a required check identically. The token needs `repo:status` (classic PAT) / `Commit statuses: write` (fine-grained); inside a workflow that is `permissions: statuses: write`. If a project does publish through a GitHub App instead, keep the check-run form — but then the publication must happen inside a workflow holding `checks: write`, plus a relay that carries the agent's verdict there. + +### Provision the `pr-state:*` labels (once per repository) + +Labels are never auto-created: `gh pr edit --add-label pr-state:to-be-reviewed` fails with `not found` until they exist. + +```bash +gh label create "pr-state:to-be-reviewed" --color "fbca04" --description "PR open, not yet cleared" --force +gh label create "pr-state:ready-to-merge" --color "0e8a16" --description "Gates green + review approved (+ explicit human approval at red)" --force +gh label create "pr-state:not-approved" --color "d73a4a" --description "Review verdict: CHANGES-REQUESTED" --force +``` + +Run this before the first `/pair-capability-publish-pr`. **Non-blocking**: if the labels are absent or the label API is unavailable, the flow reports the gap and continues — the label is only a view, and the required checks remain the merge authority. + +### Publish the `pair-review` status on the head commit + +```bash +# Register it pending at PR creation (merge is blocked from t0, even if the review crashes) +gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state='pending' -f context='pair-review' \ + -f description='Review dispatched — merge blocked until the verdict lands' + +# Publish the verdict (success | failure), mapped by review_check_conclusion +source .pair/knowledge/assets/pr-state.sh +STATE="$(review_check_conclusion "$VERDICT")" # approved|tech-debt ⇒ success · changes-requested ⇒ failure +[ "$STATE" = pending ] && { echo "no decision yet — leaving the pending status in place"; exit 0; } +gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state="$STATE" -f context='pair-review' \ + -f description="$(printf '%.140s' "$VERDICT_SUMMARY")" \ + -f target_url="$REVIEW_URL" +``` + +`review_check_conclusion` returns `pending` when the review produced no decision — the guard above is mandatory: never overwrite the pending status with a resolved one, so the required context stays unsatisfied and the merge stays blocked. (`description` is capped at 140 characters by the API; the full report lives in the native review that `target_url` points at.) + +If the POST is refused (`403`/`404` — token without `repo:status`), enforcement degrades to **advisory**: the verdict is still recorded in the native review and the `pr-state:*` label is still computed, and the skill reports `pair-review: NOT PUBLISHED — advisory` instead of claiming the merge is blocked. + +### `pair-explicit-approval` job (🔴 only, auto-passes below) + +Reads the `risk:*` label only (D18, fail-safe red) and asserts a **human** approval — the pair review's own submission never satisfies it, and `dismiss_stale_reviews` keeps it from surviving a force-push. + +Five properties are load-bearing and easy to get wrong; all five are encoded in the template below, and the first four were **verified on a live repository** (§ Verified on a throwaway repository). + +1. **The job must run from a trusted ref.** This job is an **authorization control**, so its code must not come from the change it authorizes. On a `pull_request` event GitHub runs the **PR's own** version of the workflow file, and `actions/checkout` with the default ref checks out the PR's tree — so a PR could **tamper** with the job body or with the sourced `explicit_approval_required()` and make the required context report `success` with no human approval, i.e. **self-grant** the 🔴 gate. That is not theoretical: it was reproduced (a `risk:red` PR shipping a neutered `explicit_approval_required` published `pair-explicit-approval=success — tier red — explicit approval not required` on its own head). `pull_request_target` runs the **base** version of the workflow, and pinning the checkout to `github.event.pull_request.base.sha` makes the sourced assets base versions too. The pin matters for the review trigger as well: on `pull_request_review` the workflow _file_ comes from the default branch, but `GITHUB_REF` is `refs/pull//merge`, so a **default checkout there also lands on the PR's merged tree**. Distinguish this from workflows whose job body only _narrows_ what is tested — there tampering weakens a check; here it would remove an authorization control. +2. **The verdict must be pinned to the PR head commit.** Branch protection evaluates required contexts **on the head SHA**, and `$GITHUB_SHA` is _not_ that commit for either trigger — measured: `pull_request_target` ⇒ the base branch tip, `pull_request_review` ⇒ the ephemeral `refs/pull//merge` commit. A job that published against `$GITHUB_SHA` would report on a commit the PR's protection never reads, leaving a 🔴 PR blocked _after_ the human approves. The job therefore `POST`s a **commit status** pinned to `github.event.pull_request.head.sha`, which is deterministic and independent of any event-to-SHA association (that is why the workflow needs `statuses: write`). GitHub _does_ currently attach a run's own check-run to the PR head for both triggers (measured), but that association is incidental where this one is explicit. +3. **One producer per required context.** The job is therefore named `explicit-approval-gate`, **not** `pair-explicit-approval`: the required context is the commit status it posts, and giving a check-run the same name would leave two independent producers writing one context. The job's own check-run stays as unrequired, human-readable detail. +4. **Only the newest evaluation may publish.** Two label mutations seconds apart (e.g. `--remove-label risk:yellow --add-label risk:red`) fire two events, and without a concurrency guard a slower older run can overwrite the newer verdict — observed: a stale run re-published the previous tier's result _after_ the current one. A `concurrency` group with `cancel-in-progress` plus a **fresh label read** (the event payload's label list is a snapshot; the API's is current) makes the verdict of the last **completed** evaluation the one that stands. Runs that never complete are property 5's job. +5. **An interrupted evaluation must leave the context blocking.** A job that publishes only at the end leaves the **previous** status standing when it dies mid-flight — and the previous status can be a `success` from a lower tier, so a 🟡 → 🔴 raise whose evaluation is cancelled (`cancel-in-progress`) or aborted (`set -euo pipefail` on a transient `gh` failure, or the base branch not yet carrying `tier-resolve.sh`/`pr-state.sh`) would silently keep the merge enabled at 🔴 with no human approval. Nothing required reports red in that state, because the job's own check-run is deliberately not a required context (property 3). The job therefore posts **`state=pending` on the head SHA as its very first step**, before resolving anything: an interrupted evaluation leaves a pending — i.e. merge-blocking — context, and only a completed one resolves it. Same pending-first pattern `pair-review` already uses. (An `if: ${{ failure() }}` publish step is weaker: it does not run on hard cancellation.) + +```yaml +name: pair-explicit-approval +on: + # `pull_request_target`, NOT `pull_request`: the workflow file and the sourced + # assets must come from the BASE branch (property 1 above). + pull_request_target: + # `labeled`/`unlabeled` are REQUIRED: a tier raised mid-review (🟡 → 🔴) must + # re-evaluate and re-block until a human approval is recorded. + types: [opened, synchronize, reopened, labeled, unlabeled] + pull_request_review: + types: [submitted, dismissed] + +# Only the NEWEST evaluation may publish (property 4): two label mutations seconds +# apart race, and a stale run must never overwrite the current verdict. +concurrency: + group: pair-explicit-approval-${{ github.event.pull_request.number }} + cancel-in-progress: true + +# Least privilege. `statuses: write` is what lets the job pin its verdict to the PR +# head commit (property 2); nothing here ever checks out or executes PR code. +permissions: + contents: read + pull-requests: read + statuses: write + +jobs: + # NOT named `pair-explicit-approval` (property 3): the required context is the + # head-pinned commit status this job posts, and one context = one producer. + explicit-approval-gate: + runs-on: ubuntu-latest + steps: + # PENDING FIRST (property 5), before anything that can fail or be cancelled: + # a run that dies later must not leave a previous `success` standing. No + # checkout needed — this step only writes the blocking placeholder. + - env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state='pending' -f context='pair-explicit-approval' \ + -f description='evaluating tier + human approval on this head' + - uses: actions/checkout@v4 + with: + # TRUSTED REF — the base commit, never the PR head. This is what makes the + # two sourced functions untamperable from the pull request's side. + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + - env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + source .pair/knowledge/assets/tier-resolve.sh # tags only, no criteria + source .pair/knowledge/assets/pr-state.sh + # Labels read FRESH from the API (property 4), not from the event payload: + # a payload snapshot is stale the moment a second label event lands. + PR_LABELS="$(gh pr view "$PR" --repo "$REPO" --json labels --jq '[.labels[].name] | join(" ")')" + TIER="$(resolve_tier "$PR_LABELS")" + STATE=success + DESC="tier $TIER — explicit approval not required" + if explicit_approval_required "$TIER"; then + # A HUMAN approval on the CURRENT head: exclude bots and the PR author. + # The predicate is NOT written out here — it is `human_approval_jq_filter` + # from the sourced pr-state.sh, so this job and the tests that verify it + # read one text and cannot drift (it rejects non-APPROVED, another commit, + # `user.type != "User"`, and the author's own approval). + # Use the REST reviews endpoint — it is the only one carrying BOTH the + # account type and the reviewed commit. `gh pr view --json reviews` exposes + # `author.login` and NO bot flag whatsoever, so filtering there on a + # bot-exclusion field matches nothing and silently yields 0 approvals. + # `--paginate` (no per_page cap): reviews accumulate over a PR's life, so + # the approval can sit on page 2 — a single page would read it as zero and + # block the PR forever. Aggregate by counting the paginated stream. + APPROVALS="$(gh api --paginate "repos/$REPO/pulls/$PR/reviews" \ + --jq "$(human_approval_jq_filter)" | grep -c . || true)" + if [ "${APPROVALS:-0}" -ge 1 ]; then + DESC="explicit human approval recorded on the current head" + else + STATE=failure + DESC="risk:red — needs an explicit human approval on the current head (D10)" + fi + fi + # Pin the context to the PR head commit (property 2). Publish BEFORE failing + # the job, so the required context always reports. + gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state="$STATE" -f context='pair-explicit-approval' \ + -f description="$(printf '%.140s' "$DESC")" + [ "$STATE" = success ] || { echo "::error::$DESC"; exit 1; } +``` + +**Residual caveats of the trusted-ref form**, stated explicitly rather than assumed away: + +- **Forks.** With `pull_request_target` the workflow runs in the **base** repository's context with a write-capable `GITHUB_TOKEN`, which is only safe because this job **never checks out or executes head-ref code** — the checkout is pinned to `base.sha` and no step runs anything from the PR. Never add a step here that builds, installs, or runs the PR's code; put that in the ordinary `pull_request` gate pipeline, which has a read-only token on forks. Conversely, a plain `pull_request` run from a fork gets a **read-only** token and could not `POST` the status at all — another reason for the target trigger. +- **Base-branch assets.** `tier-resolve.sh` and `pr-state.sh` are sourced from the base commit, so they must already exist on the base branch: the workflow is added and observed **before** protection is applied (ordering below), and a PR that only _introduces_ those assets cannot satisfy the context on its own. +- **Projects that refuse `pull_request_target` entirely** (a common blanket policy) get the same guarantee by keeping the plain `pull_request` trigger and writing the projections **inline** in the job body — `resolve_tier` / `explicit_approval_required` are a handful of `case` lines and `human_approval_jq_filter` is one string — with no checkout step at all, so there is nothing from the PR's tree to tamper with. The cost is a second copy of the projection, which must then be kept in sync with the shipped assets (a conformance grep is the usual mitigation). + +### Branch protection payload + +Use the **`checks` form**, not the legacy `contexts` array: `contexts` accepts a status from _any_ source, while each `checks[]` entry may pin an `app_id` — which is what keeps a user/PAT-posted status from satisfying the authorization gate (see § What each context proves). + +```bash +# The producer of `pair-explicit-approval` is the GitHub Actions app (the job posts the +# status with the default GITHUB_TOKEN). Resolve its id rather than hardcoding it: +ACTIONS_APP_ID="$(gh api /apps/github-actions --jq .id)" # 15368 on github.com + +gh api "repos/$REPO/branches/main/protection" -X PUT --input - < Adoption > KB default` cascade. This document copies **none** of those values — not even as an illustration: a project that overrides `tier.red.reviewers` or `tier.red.sla_days` in `tech/risk-matrix.md` changes them in exactly one place, and a restatement here would be the first thing to go stale. The only requirement this flow *names* is the one that changes the merge topology: at 🔴 (`risk:red`) the quality model demands an **explicit human approval** (D10), which the synthesis below consumes as its fourth input. + +**How the pair review maps onto those requirements:** at 🟡 the **pair review satisfies** the tier's single **reviewer approval** — an approving `pair-review` verdict on green gates *is* that approval, and no additional human approval is demanded (so 🟡 differs from 🟢 in gate breadth and in the review being blocking, not in an extra approver). 🔴 is the only tier adding a requirement the review cannot meet by itself: an **explicit human approval** from a non-author account (D10). + +The tier itself is read from the PR's `risk:*` label via [`tier-resolve.sh`](../../../assets/tier-resolve.sh) (`resolve_tier`). **This flow contains no classification criteria** (D18): it never inspects the diff, the code, file paths, or change size — grep-verifiable, in both the guideline and the evaluator. **Fail-safe: an untagged, unknown, or malformed tier is treated as 🔴 red** (consistent with the pre-merge gate's own fail-safe, [tier-aware-pipeline.md](../../infrastructure/cicd-strategy/tier-aware-pipeline.md)) — so a missing classification demands *more*, never less. + +## Enforcement: required checks are the authority, labels are a view + +Merge blocking is **mechanical**. Three required-check layers sit on the protected branch: + +| Layer | Check | Required | Blocks when | +| --- | --- | --- | --- | +| Gate | the pre-merge pipeline's jobs (base, secret-scan, and the suites for the tier) | always | any job red, or a required suite missing | +| Review | **`pair-review`** — the check the review flow publishes on the head commit (on GitHub a **commit status**, which the agent's own token can write; the Checks API cannot — see the implementation guide) | always | conclusion is `failure` (CHANGES-REQUESTED) **or** still `pending`/absent (review never ran, crashed, or was skipped) | +| Explicit approval | **`pair-explicit-approval`** — a deterministic host job that verifies a human approval exists on the current head when the tier demands it | always (auto-passes below 🔴) | tier is 🔴 (or untagged ⇒ 🔴) and no human approval is recorded on the current head | + +Three properties of the explicit-approval layer are **host-agnostic invariants**, not GitHub trivia — an implementation that misses any one of them silently voids AC4: + +- **It runs from a trusted ref.** The job's code (and any shared projection it reads) comes from the **target branch**, never from the pull request's own tree. Otherwise the change under review can rewrite the check that authorizes it and self-grant the 🔴 approval — an authorization control defined by the thing it authorizes is not a control. +- **Its verdict is pinned to the head commit** the host's protection actually evaluates, on **every** re-evaluation — including the one triggered by the approval itself. A verdict recorded against any other commit leaves a 🔴 PR blocked *after* the human approves. +- **Its producer is pinned, and an interrupted evaluation leaves it blocking.** Hosts generally let *any* principal with write access publish *any* check/status name, so the protected branch must constrain **who** may satisfy the authorization context (on GitHub: `checks[].app_id`) — otherwise one API call by the authoring/reviewing agent satisfies the human gate. And because the previous verdict survives a crashed or cancelled evaluation, the job publishes a **pending** result before it starts resolving, so an interrupted re-evaluation blocks instead of leaving a stale `success` from a lower tier. + +### What the two checks prove + +The two layers are **not** equally strong, and conflating them is how AC4 gets quietly lost: + +| Check | Control type | Guarantee | +| --- | --- | --- | +| `pair-review` | **anti-accident** | A review that never ran, crashed, or was skipped leaves the context absent/pending, and the host blocks the merge. It is published with the same credentials the authoring/reviewing agent holds, so **it is forgeable by any principal with write access, by construction** — it proves "the review was not silently skipped", never "an independent party approved". | +| `pair-explicit-approval` | **authorization** | The 🔴 human approval is asserted by a deterministic job the pull request cannot modify, publishing a producer-pinned context — so the reviewing agent cannot grant it. Fully unforgeable only in the host's App/check-run form (see the implementation guide's residuals, and ADR-018 Option 4 — deferred, [#398](https://github.com/foomakers/pair/issues/398)). | + +Because `pair-review` is a **required status check** (R5.7), the review **cannot be skipped**: a PR with no review check, a failed one, or one still pending is not mergeable on the code host — not by convention, but by branch protection. That is a guarantee about *skipping*, not about *identity*: the check does not prove who published the verdict. The identity half is `pair-explicit-approval`'s job at 🔴 (AC4/D10), which is why the flow keeps two checks instead of one (ADR-018). + +**The `pr-state:*` label is a view, not the authority.** The flow keeps exactly one of `pr-state:to-be-reviewed` / `pr-state:ready-to-merge` / `pr-state:not-approved` on the PR so humans and `pair-next`-style automation can filter at a glance. A label is never the thing that permits a merge: editing or forging a label cannot enable a merge, because branch protection evaluates the required checks and ignores labels entirely. If a label and the checks ever disagree, the checks win and the next evaluation re-labels. + +Because it is only a view, the label family is a **prerequisite, not a dependency**: hosts do not auto-create labels, so they are provisioned once per repository (implementation guide). A missing label family — or no label API at all — is **non-blocking**: the flow reports `pr-state label: not applied` and continues, since the required checks remain the merge authority. + +Host mechanics — how the check run is published, the exact branch-protection payload, the `pair-explicit-approval` job — live in the code host's implementation guide (R2.12): [github-implementation.md](github-implementation.md) § "PR state flow — required checks & branch protection". + +## Edge cases + +| Case | Behavior | +| --- | --- | +| **Tier raised after review started** (e.g. review raises 🟡 → 🔴) | Requirements are re-evaluated on the **new** tier: a raise to 🔴 **re-blocks** the merge until explicit human approval is recorded. The label-change event re-runs the gate and re-evaluates `pair-explicit-approval`; the state drops back to `to-be-reviewed`. Tiers are raise-only (D17), so re-evaluation never loosens a requirement. | +| **Review subagent crashes or times out** | The `pair-review` check stays `pending` (or is published as `failure` with the error) — merge stays **blocked**, never silently open. The flow posts a PR comment with the re-run command so a human can restart the review. | +| **Force-push after approval** | Checks and reviews are per head commit: the new head has no `pair-review` check, so the merge is **blocked** — fail-safe by construction. The re-run is **triggered, not automatic**: nothing host-side re-registers `pair-review` on `synchronize`, so a human (or `/pair-capability-publish-pr` / `/pair-process-review`, re-invoked) starts the review on the new head. A project that wants it automatic adds a `synchronize`-triggered job that re-registers `pair-review` as `pending` on every new head. Branch protection additionally sets `dismiss_stale_reviews`, and `pair-explicit-approval` pins the approval to the current head, so a human approval does not survive a rewrite either. | +| **Code host without a required-check API** | **Degraded mode**, documented rather than silently skipped: the flow still computes and labels the state and still posts the verdict in the native review, but enforcement becomes advisory. `/pair-capability-setup-gates` reports the degradation and points to the host's **manual** branch-protection setup steps in its implementation guide; the project is told, explicitly, that merge blocking is not mechanical until those steps are applied. | +| **PR with no `risk:*` label** | Treated as 🔴 (fail-safe): the widest gate matrix **and** explicit human approval are required. | +| **Self-authored PR** (solo maintainer) | The code host rejects APPROVE on your own PR, so the verdict is recorded as a review comment led by the verdict token (see `/pair-process-review`'s degradation). The `pair-review` check still carries the machine-readable conclusion, so enforcement is unaffected — 🟢 and 🟡 work normally. **At 🔴 it does not:** the requirement is a non-author human approving review, and the author cannot supply one, so on a single-maintainer repository **no 🔴 PR can reach `ready-to-merge`** — a 🔴 merge needs a **second human account**. This is a real constraint, not a workaround: a solo maintainer either adds a second reviewer account, or deliberately keeps `pair-explicit-approval` out of the required contexts and accepts that the 🔴 human-approval rule is advisory there (recorded, per the degraded-mode row). An alternative solo-approval token (a human-applied `approved:human` label with actor verification, or an `/approve` comment command) is a **design change tracked as [#398](https://github.com/foomakers/pair/issues/398)** — deliberately not part of this flow rather than half-specified here. | + +## Who does what + +| Actor | Responsibility | +| --- | --- | +| `/pair-capability-publish-pr` | Creates the PR, propagates the story's classification tags, registers `pair-review` as **pending**, labels the PR `pr-state:to-be-reviewed`, and triggers the review in a clean-context subagent (AC1) | +| `/pair-process-review` | Produces the judgment verdict in the native review, publishes the `pair-review` check conclusion (`review_check_conclusion`), computes the state (`resolve_pr_state`), swaps the `pr-state:*` label, and refuses to merge unless `merge_allowed` passes | +| `/pair-process-implement` Phase 4 | The **other** merge path (the author re-invoked after an approving review): runs the *same* precondition as `/pair-process-review`'s — re-synthesize the current signals, `merge_allowed`, HALT otherwise — before merging. "At least one approval" is **not** the condition; a 🔴 PR with an approving verdict and no explicit human approval must not merge here either | +| `/pair-capability-setup-gates` | Wires `pair-review` + `pair-explicit-approval` as required checks on the protected branch alongside the gate jobs, or reports degraded mode | +| Code host | Enforces: blocks the merge button while any required check is red, pending, or absent | +| Human | Fixes findings, gives the explicit approval at 🔴, and presses merge | + +## Related + +- [canonical-states.md](canonical-states.md) — the five work-item macrostates (`Review` is the macrostate a PR lives under) +- [quality-model.md](../../quality-assurance/quality-model.md) — §3.2 fail-safe tier, §4 per-tier requirements (the single source of the thresholds this flow reads) +- [tier-aware-pipeline.md](../../infrastructure/cicd-strategy/tier-aware-pipeline.md) — the gate side of the flow (tags-only pre-merge pipeline, fail-safe red) +- [`pr-state.sh`](../../../assets/pr-state.sh) — the provider-agnostic synthesis evaluator (`resolve_pr_state`, `merge_allowed`, `explicit_approval_required`, `review_check_conclusion`, `human_approval_jq_filter`) +- [`tier-resolve.sh`](../../../assets/tier-resolve.sh) — the provider-agnostic, tags-only tier resolver +- [code-review-template.md](../templates/code-review-template.md) — the verdict-first review body whose verdict this flow consumes +- [github-implementation.md](github-implementation.md) — GitHub check-run + branch-protection mechanics for the two pair checks diff --git a/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md b/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md index 130bdaab..369ee351 100644 --- a/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md +++ b/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md @@ -201,7 +201,7 @@ Scheduling the jobs is not enough; the code host must **require** them: - Mark `base` and `secret-scan` as required status checks on the protected branch (`main`). - Mark `unit`, `integration`, `e2e` as required **when scheduled**. On green/yellow PRs the higher-tier jobs are not scheduled, so a "required-when-present" convention (or a single aggregating gate job that `needs:` the scheduled set and reports one status) keeps branch protection satisfiable across tiers. - **GitHub note:** a job skipped via a job-level `if:` (as `unit`/`integration`/`e2e` are on lower tiers) reports its required check as **passing**, not pending — GitHub has treated skipped-via-`if:` required checks as successful since ~2021. So on GitHub you can mark all three required directly and lower-tier PRs stay mergeable without an aggregating job; the aggregating-gate pattern above is only needed on hosts that leave an unscheduled required check pending. (This template omits the aggregating job for that reason — add one only if your host needs it.) -- A failing required job (including a missing-suite failure or a secret finding) blocks merge until the gate is green — this is the gate acting before review (R5.4): review starts only at a green gate. +- A failing required job (including a missing-suite failure or a secret finding) blocks merge until the gate is green — this is the gate acting before review (R5.4): review starts only at a green gate. (Refinement: a review may run earlier to report findings, but it can produce **no merge-enabling verdict** at a red gate — see [pr-states.md](../../collaboration/project-management-tool/pr-states.md).) - **A label change must force gate re-evaluation before merge.** Because the tier is read from the `risk:*` label, the `pull_request` trigger includes `labeled`/`unlabeled` (above) so a tier raised mid-review re-runs the gate at the wider matrix. The latest run — not a stale earlier one — is what branch protection evaluates, so a PR can never merge on a gate that ran at a lower tier than its current label. ## Post-merge staging pipeline: build + deploy only diff --git a/.pair/knowledge/guidelines/quality-assurance/quality-model.md b/.pair/knowledge/guidelines/quality-assurance/quality-model.md index 29a17cb2..cdf3a95a 100644 --- a/.pair/knowledge/guidelines/quality-assurance/quality-model.md +++ b/.pair/knowledge/guidelines/quality-assurance/quality-model.md @@ -64,6 +64,13 @@ Cost class = **highest detected signal**. The signal catalog (paid-SDK imports, | 🟡 Yellow | Blocked until reviewed | 1 reviewer | 1 working day | standard | reviewer approval | | 🔴 Red | Blocked until reviewed and approved | 1 reviewer | 2 working days | extended | explicit approval required | +**Checklist depth** is that table's third column and means exactly this — there is no separate "extended checklist" artifact anywhere, and none is needed: + +| Depth | Meaning | +| --- | --- | +| `standard` (🟢/🟡) | The [code-review template](../collaboration/templates/code-review-template.md) — resolved per [template resolution](../technical-standards/ai-development/skill-conventions/template-resolution.md) — as designed: verdict first, the Assessments block, and the Details sections that the change actually touches. Sections with nothing to say are collapsed/omitted. | +| `extended` (🔴) | The **same** template with **no section skipped**: every Assessments subsection (all Security dimensions, Cost, Architecture) and every Details section is answered explicitly — "not applicable" is written out rather than omitted — and the Definition-of-Done check is run in full. Depth, not a different document. | + Reviewer counts and SLAs are **KB defaults** (D10), resolved through the same **Argument > Adoption > KB default** cascade as every other rule in this document — not fixed forever. A project may override either per tier in `tech/risk-matrix.md`'s Overrides section (§6), e.g. requiring 2 reviewers at 🔴 Red for a larger team: ```markdown @@ -73,7 +80,7 @@ Reviewer counts and SLAs are **KB defaults** (D10), resolved through the same ** - tier.red.sla_days: 3 ``` -Review always runs, tests are always green, at every tier (R5.3 + D10) — this part is not overridable, only the reviewer count and SLA are. Gate (mechanical) and review (judgment) are distinct enforcers — gate blocks first, review starts only once gates are green: +Review always runs, tests are always green, at every tier (R5.3 + D10) — this part is not overridable, only the reviewer count and SLA are. Gate (mechanical) and review (judgment) are distinct enforcers — gate blocks first, review starts only once gates are green. **Refinement** (the merge-side companion below, [pr-states.md](../collaboration/project-management-tool/pr-states.md)): a review *may* run at a red gate to report findings early, but it produces **no merge-enabling verdict** — a red gate never yields `ready-to-merge`, whatever the judgment says. "Starts only once gates are green" is about the *merge-enabling* review, not about a prohibition on reading a red-gated diff. | Tier | Gate checks | | --- | --- | @@ -83,6 +90,8 @@ Review always runs, tests are always green, at every tier (R5.3 + D10) — this Install runs at every tier (implicit in each row). Deterministic secret scanning also runs at **every** tier, unconditionally — it is not tier-scoped (security/[secret-scanning.md](security/secret-scanning.md)). How this matrix becomes an actual pipeline that reads the `risk:*` tag only (fail-safe 🔴 when untagged, explicit failure on a missing suite, build+deploy-only post-merge staging) is the delivery-side companion [tier-aware-pipeline.md](../infrastructure/cicd-strategy/tier-aware-pipeline.md) — this table stays the single source of the criteria; that document owns the wiring. +How the two enforcers combine into a merge decision — the PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`), the synthesis of gates × verdict × tier × explicit approval, and the required checks that make the review unskippable — is the review-side companion [pr-states.md](../collaboration/project-management-tool/pr-states.md). It **reads** the requirements above (including the 🔴 explicit-approval row) and declares none of its own. + ## 5. Tag Projection Chromatic, no semantic tag beyond color. **`risk:green|yellow|red` (§3.2) is the only tag family the KB names and proposes by default.** diff --git a/.pair/knowledge/how-to/11-how-to-code-review.md b/.pair/knowledge/how-to/11-how-to-code-review.md index 435d2ff9..8a726950 100644 --- a/.pair/knowledge/how-to/11-how-to-code-review.md +++ b/.pair/knowledge/how-to/11-how-to-code-review.md @@ -99,7 +99,7 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- | Decision | Condition | Next Step | | --------------------- | ------------------------------------------------------- | ------------------------------------------------------------------------ | -| **APPROVED** | All requirements met, quality gates pass | Squash merge, mark story "Done" | +| **APPROVED** | All requirements met, quality gates pass | Squash merge **only when the PR state is `ready-to-merge`** (`merge_allowed`: required checks green — `pair-review` + `pair-explicit-approval` at 🔴 — see [pr-states.md](../guidelines/collaboration/project-management-tool/pr-states.md)), then mark story "Done" | | **CHANGES REQUESTED** | Critical issues, missing ADR, failing tests, AC not met | Return to [10-how-to-implement-a-task.md](10-how-to-implement-a-task.md) | | **TECH DEBT** | Only minor issues, tracked as debt | Approve PR, create debt items | diff --git a/.pair/llms.txt b/.pair/llms.txt index cbc416c2..89fc1c88 100644 --- a/.pair/llms.txt +++ b/.pair/llms.txt @@ -130,6 +130,7 @@ - [Filesystem - Complete Implementation Guide](.pair/knowledge/guidelines/collaboration/project-management-tool/filesystem-implementation.md) - [GitHub Projects - Complete Implementation Guide](.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) - [Linear - Complete Implementation Guide](.pair/knowledge/guidelines/collaboration/project-management-tool/linear-implementation.md) +- [PR State Flow — gate ≠ review](.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) - [Project Management Tool Framework](.pair/knowledge/guidelines/collaboration/project-management-tool/README.md) - [Azure DevOps Project Tracking](.pair/knowledge/guidelines/collaboration/project-tracking/azure-devops-tracking.md) - [Filesystem Project Tracking](.pair/knowledge/guidelines/collaboration/project-tracking/filesystem-tracking.md) diff --git a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts index 5909c13e..59b56adb 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts @@ -82,7 +82,7 @@ describe('formatVersionCheckHuman', () => { ) const output = formatVersionCheckHuman(result) - const installedLine = output.split('\n').find((l) => l.includes('Installed:')) ?? '' + const installedLine = output.split('\n').find(l => l.includes('Installed:')) ?? '' expect(installedLine).toContain('non-stable') }) }) diff --git a/apps/pair-cli/src/commands/kb-info/version-check.test.ts b/apps/pair-cli/src/commands/kb-info/version-check.test.ts index 090a9d67..b71a6b68 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check.test.ts @@ -28,9 +28,7 @@ describe('compareVersions', () => { const result = compareVersions(installed({ version: '1.1.0' }), current({ version: '1.2.0' })) expect(result.status).toBe('drift') - expect(result.migrationUrl).toBe( - 'https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0', - ) + expect(result.migrationUrl).toBe('https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0') }) it('reports drift WITHOUT a migration URL on downgrade (installed newer than current)', () => { diff --git a/apps/website/content/docs/concepts/meta.json b/apps/website/content/docs/concepts/meta.json index bc8545d4..15d1793c 100644 --- a/apps/website/content/docs/concepts/meta.json +++ b/apps/website/content/docs/concepts/meta.json @@ -9,6 +9,7 @@ "code-host", "definition-of-ready-and-done", "tag-driven-gates", + "pr-state-flow", "agent-integration", "llms-txt" ] diff --git a/apps/website/content/docs/concepts/pr-state-flow.mdx b/apps/website/content/docs/concepts/pr-state-flow.mdx new file mode 100644 index 00000000..905c0fd9 --- /dev/null +++ b/apps/website/content/docs/concepts/pr-state-flow.mdx @@ -0,0 +1,88 @@ +--- +title: PR State Flow (gate ≠ review) +description: How pair turns mechanical gates, a judgment review, and the risk tier into one PR state — and blocks the merge with required code-host checks instead of good intentions. +--- + +import { Callout } from 'fumadocs-ui/components/callout' + +A pull request in pair has exactly **three** states, and nobody types them in. The state is the **synthesis** of signals that already exist on the pull request: the CI gate result, the review verdict, the `risk:*` tier, and whether a human approved explicitly. + +```text +to-be-reviewed → ready-to-merge + ↘ not-approved +``` + +| PR state | Meaning | Mergeable | +| --- | --- | --- | +| **to-be-reviewed** | Gates not green, review not submitted (or crashed), or a 🔴 PR still waiting for explicit human approval | No | +| **ready-to-merge** | Gates green, review approved, and (at 🔴) a human approval on the current head | Yes — a human still presses merge; pair never auto-merges | +| **not-approved** | The review asked for changes; the findings go back to the author | No | + +## Two enforcers that never blur + +| Enforcer | Nature | Judgment | +| --- | --- | --- | +| **Gate** — CI checks | deterministic pass/fail | **zero judgment** | +| **Review** — the `review` process skill | a verdict on the change | **zero mechanical checks** — it reads gate results, never re-runs them | + +**The gate goes first.** While any mechanical check is red the review may still report findings, but it cannot produce a merge-enabling outcome: a red gate never yields `ready-to-merge`, whatever the verdict says. The gate answers "does it build and pass"; the review answers "should it exist". + +## The review is a required check, not a convention + +At PR creation `publish-pr` registers a **`pair-review`** check as pending — on GitHub a *commit status*, which an ordinary agent token can write (the Checks API is GitHub-App only) and which branch protection accepts as a required context all the same — and dispatches the review to a clean-context subagent — a reviewing context that inherits nothing from the authoring session. Because `pair-review` is a **required status check** on the protected branch, the merge is blocked from the first second: + +- review approved ⇒ check `success` +- changes requested ⇒ check `failure` +- review never ran, crashed, or timed out ⇒ check stays **pending**, which blocks the merge exactly like a failure + +So the review cannot be skipped, and a crashed review can never leave a PR quietly mergeable. + +A second required check, **`pair-explicit-approval`**, enforces the 🔴 tier's extra requirement: a **human, non-author** approval on the current head commit (the AI review does not count, and a bot cannot substitute). It auto-passes at 🟢 and 🟡, so lower-risk PRs are never slowed down by it. + +Because it is an *authorization* check, two implementation properties are non-negotiable — the host guide's job template has both: + +- **it runs from the target branch's code**, never the pull request's own — otherwise a PR could rewrite the check that authorizes it and self-grant the 🔴 approval; +- **its verdict is pinned to the PR head commit** on every re-evaluation, including the one the approval itself triggers — a verdict recorded anywhere else would leave a 🔴 PR blocked *after* the human approved; +- **its producer is pinned and it publishes "pending" first** — a status context is writable by anyone with push access, so the protected branch pins *who* may satisfy it (`checks[].app_id`), and the job marks the context pending before it evaluates, so a cancelled run can't leave a stale `success` from a lower tier. + + + **The two checks are not equally strong, by design.** `pair-explicit-approval` is the *authorization* + control. `pair-review` is an *anti-accident* control: it is published with the reviewing agent's own + credentials, so anyone with push access could post it — what it guarantees is that a review which never + ran or crashed leaves the merge blocked, not that an independent party approved. Enforcing identity is + the other check's job. + + +The review is dispatched to that clean-context subagent with a bounded instruction: produce the verdict and the checks, **never merge**. Merging stays a human act at every tier. + + + **A 🔴 merge needs a second human account.** GitHub rejects an approving review from the PR author, + so on a single-maintainer repository no 🔴 PR can satisfy `pair-explicit-approval`. Either add a + second human reviewer account, or deliberately keep that context out of the required list there and + accept that the 🔴 human-approval rule is advisory (recorded, not silently dropped). At 🟡 there is + no such constraint: the pair review itself *is* the reviewer approval the quality model asks for. + A solo-maintainer approval token (a verified human token instead of a second account) is tracked as + [pair#398](https://github.com/foomakers/pair/issues/398). + + +## Labels are a view; checks are the authority + +The flow keeps exactly one `pr-state:*` label on the PR so humans and automation can filter at a glance. That label never permits a merge — branch protection evaluates the required checks and ignores labels entirely. Editing or forging a label changes nothing about mergeability; if label and checks disagree, the checks win and the next evaluation re-labels. + +## Tiers, fail-safe, and no criteria in the flow + +Reviewer count, SLA, checklist depth, and the 🔴 explicit-approval rule come from the [Quality Model](/docs/reference/quality-model) — one place, overridable in adoption. The flow reads the PR's `risk:*` label and nothing else: **no classification criteria live in it** (it never inspects the diff, paths, or change size). An untagged, unknown, or malformed tier is treated as 🔴, so a missing classification always demands *more*: the widest gate matrix **and** an explicit human approval. + +## Things that change mid-flight + +- **Tier raised during review** (🟡 → 🔴): requirements are re-evaluated on the new tier and the merge **re-blocks** until a human approves. Tiers are raise-only, so re-evaluation never loosens anything. +- **Force-push after approval**: checks and reviews are per head commit, so the new head has no review check — the merge blocks. The re-run is **triggered, not automatic**: re-invoke the review (or add a `synchronize` job that re-registers the pending check) — the fail-safe direction is "blocked", so nothing slips through in the meantime. Stale human approvals are dismissed too. +- **Review subagent crashes**: the pending check keeps the merge blocked, and the PR gets a comment with the re-run command. +- **Host without a required-check API**: documented **degraded mode** — the state is still computed and labeled, but enforcement is advisory until the branch protection is applied manually. `setup-gates` reports the gap instead of pretending the flow is enforced. + +## Related + +- [Tag-Driven Quality Gates](/docs/concepts/tag-driven-gates) — the gate side of the flow (the first filter) +- [Canonical States](/docs/concepts/canonical-states) — the work-item macrostates a PR lives under (`Review`) +- [Quality Model](/docs/reference/quality-model) — the per-tier requirements this flow reads +- [Quality Gates & Configuration](/docs/reference/quality-gates-configuration) — what is always on, and what is opt-in diff --git a/apps/website/content/docs/concepts/tag-driven-gates.mdx b/apps/website/content/docs/concepts/tag-driven-gates.mdx index 45e87142..3b5caaa1 100644 --- a/apps/website/content/docs/concepts/tag-driven-gates.mdx +++ b/apps/website/content/docs/concepts/tag-driven-gates.mdx @@ -57,7 +57,7 @@ Coverage can be protected by a **baseline regression guardrail** — a job insid ## Post-merge is build + deploy only -Once a PR merges to the main branch, the gate does not run again — it already passed. The post-merge staging pipeline performs **build + deploy only**. The gate is the deterministic first filter before merge; review starts only once the gate is green. +Once a PR merges to the main branch, the gate does not run again — it already passed. The post-merge staging pipeline performs **build + deploy only**. The gate is the deterministic first filter before merge; review starts only once the gate is green — see [PR State Flow](/docs/concepts/pr-state-flow) for what happens after that (the review as a required check, and the synthesis that decides whether the PR is mergeable). ## Setting it up (opt-in) diff --git a/apps/website/content/docs/reference/guidelines-catalog.mdx b/apps/website/content/docs/reference/guidelines-catalog.mdx index 1c11472f..395d497f 100644 --- a/apps/website/content/docs/reference/guidelines-catalog.mdx +++ b/apps/website/content/docs/reference/guidelines-catalog.mdx @@ -70,7 +70,7 @@ Process, methodology, and team coordination. | **Estimation** | Story points, T-shirt sizing, estimation techniques | | **Issue Management** | Issue tracking, triage, prioritization | | **Project Tracking** | Velocity, burndown, sprint metrics | -| **Project Management Tools** | GitHub Projects, Linear, Jira configuration; [canonical states + state-mapping schema](/docs/concepts/canonical-states); [Definition of Ready & Done](/docs/concepts/definition-of-ready-and-done) | +| **Project Management Tools** | GitHub Projects, Linear, Jira configuration; [canonical states + state-mapping schema](/docs/concepts/canonical-states); [PR state flow (gate ≠ review)](/docs/concepts/pr-state-flow); [Definition of Ready & Done](/docs/concepts/definition-of-ready-and-done) | | **Team** | Team structure, roles, communication patterns | | **Templates** | Branch, commit, PR, epic, story, task, initiative, subdomain, bounded context, code review, context map | diff --git a/apps/website/content/docs/reference/skills-catalog.mdx b/apps/website/content/docs/reference/skills-catalog.mdx index f1e8e8cd..64fd0f96 100644 --- a/apps/website/content/docs/reference/skills-catalog.mdx +++ b/apps/website/content/docs/reference/skills-catalog.mdx @@ -93,7 +93,7 @@ All `analyze-*` skills are **output-only report producers**: they analyze and re | Skill | Command | Description | | ----- | ------- | ----------- | | **setup-pm** | `/pair-capability-setup-pm` | Configures the project management tool — creates/wires it up, applies its implementation guide, updates way-of-working, records the decision — whether the tool was already chosen or picked here. | -| **setup-gates** | `/pair-capability-setup-gates` | Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24) — for the adopted tech stack. | +| **setup-gates** | `/pair-capability-setup-gates` | Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24), and the required pair-review + pair-explicit-approval branch-protection checks that make the review unskippable — for the adopted tech stack. | | **manage-flags** | `/pair-capability-manage-flags` | Manages a feature flag's lifecycle — creation, activation, deactivation, cleanup — per the feature-flags guidelines. | ### Quality Assurance Capabilities @@ -122,7 +122,7 @@ All `analyze-*` skills are **output-only report producers**: they analyze and re | Skill | Command | Description | | ----- | ------- | ----------- | | **checkpoint** | `/pair-capability-checkpoint` | Writes and resumes a self-contained progress checkpoint (story, branch, tasks done, decisions, remaining todos) so work survives a context reset. | -| **publish-pr** | `/pair-capability-publish-pr` | Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, and updates the board state. | +| **publish-pr** | `/pair-capability-publish-pr` | Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, updates the board state, then enters the PR state flow — registers the required `pair-review` check as pending (merge blocked from t0) and dispatches the review to a clean-context subagent. | ## Skill Properties diff --git a/packages/knowledge-hub/dataset/.pair/adoption/tech/way-of-working.md b/packages/knowledge-hub/dataset/.pair/adoption/tech/way-of-working.md index 8aa65fa4..1faf1bb5 100644 --- a/packages/knowledge-hub/dataset/.pair/adoption/tech/way-of-working.md +++ b/packages/knowledge-hub/dataset/.pair/adoption/tech/way-of-working.md @@ -15,6 +15,7 @@ This document defines the **validated development practices and team workflows** - **Hook manager**: `husky` (KB default, decision D21/Q11) — pre-commit runs fast local checks, pre-push runs lint. Override here if the project uses a different hook manager (e.g. `lefthook`, `simple-git-hooks`); `/pair-capability-setup-gates` reads this override before provisioning. - **Pre-merge tiering**: `disabled` (default) — every PR runs the full pre-merge check suite. Set to `enabled` to opt into risk-tier-scoped pre-merge checks (lighter checks on lower-risk PRs) per [tier-aware-pipeline.md](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. - **Coverage guardrail**: `disabled` (default) — no coverage regression job. Set to `enabled` to opt into a job that blocks a PR whose coverage drops below a human-committed baseline (maintaining/improving passes — not an absolute wall); this also creates `tech/coverage-baseline.md`. See [coverage guardrail](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) + [config format](../../knowledge/assets/coverage-config-example.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. +- **Pair review required checks**: `pair-review` + `pair-explicit-approval` are the required status checks that make the review unskippable (R5.7) and enforce the 🔴 explicit-human-approval rule (D10). Not a flag: they are wired by `/pair-capability-setup-gates` in both pipeline modes. Status: [applied | not yet applied] — writing branch protection needs admin scope, so it is a deliberate human step; until applied, enforcement is advisory (the documented degraded mode). Ordering matters: provision the `pr-state:*` labels and the `pair-explicit-approval` workflow, confirm both contexts report on a real PR **and that the approval context re-reports on the same head SHA after a review submission**, and only then apply the protection — otherwise a never-reporting (or base-branch-reporting) required context blocks every merge, before or after the human approves. The steps live in [github-implementation.md](../../knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § Ordering (the model is in [pr-states.md](../../knowledge/guidelines/collaboration/project-management-tool/pr-states.md)); a host without a required-check API degrades to advisory enforcement and the gap is recorded here. - **Shared config packages**: see [shared-config-packages.md](../../knowledge/guidelines/code-design/quality-standards/shared-config-packages.md) for the lint/format/type-config pattern and per-type override structure. ### Custom Gate Registry diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/assets/pr-state.sh b/packages/knowledge-hub/dataset/.pair/knowledge/assets/pr-state.sh new file mode 100644 index 00000000..ba57d698 --- /dev/null +++ b/packages/knowledge-hub/dataset/.pair/knowledge/assets/pr-state.sh @@ -0,0 +1,140 @@ +#!/usr/bin/env bash +# pr-state.sh — provider-agnostic PR-state synthesis for the gate≠review merge flow. +# +# THIS FILE READS STATE SIGNALS AND CLASSIFICATION TAGS ONLY. It contains NO +# classification criteria (D18): it never inspects the diff, the code, file paths, +# or change size. The tier is decided upstream by `classify` from the quality +# model, carried on the PR as a `risk:*` label, and resolved into `green|yellow|red` +# by the sibling helper `tier-resolve.sh` (`resolve_tier`). This script is the +# deterministic Automation layer of the quality model's three-layer principle: +# it synthesizes already-produced signals and nothing else. +# +# Gate ≠ review (R5.4): the mechanical gate is the FIRST filter — a non-green gate +# can never produce a merge-enabling state, no matter what the judgment review said. +# +# Fail-safe: an unknown gate/review signal is treated as not-passing, and an absent, +# unknown, or malformed tier is treated as `red` (the strictest requirements) — never +# a silent `ready-to-merge`. See: +# .pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md +# .pair/knowledge/guidelines/quality-assurance/quality-model.md (§3.2 fail-safe, §4 per-tier requirements) +# +# Usage (in a code host's automation, or by /pair-process-review): +# source tier-resolve.sh # tags only +# source pr-state.sh +# TIER="$(resolve_tier "$PR_LABELS")" +# STATE="$(resolve_pr_state "$GATES" "$REVIEW" "$TIER" "$EXPLICIT_APPROVAL")" +# merge_allowed "$STATE" || exit 1 + +# resolve_pr_state +# gates : pass | fail | pending | +# review : approved | tech-debt | changes-requested | pending | missing | error +# (`tech-debt` is an approving verdict — approve now, track the debt +# separately; anything unrecognized is treated as no decision yet) +# tier : green | yellow | red | +# explicit_approval : 1 when a human approval is recorded on the CURRENT head, else 0 +# +# Echoes exactly one of: to-be-reviewed | ready-to-merge | not-approved. Always exits 0. +resolve_pr_state() { + local gates="${1:-}" review="${2:-}" tier="${3:-}" approval="${4:-0}" + + # A changes-requested verdict is the one state that routes to a human author, + # whatever the gate says — the finding list is the actionable output. + if [ "$review" = "changes-requested" ]; then + echo "not-approved" + return 0 + fi + + # Gate first (R5.4): review never unlocks a merge over a non-green gate. + if [ "$gates" != "pass" ]; then + echo "pr-state: gates are not green ('${gates:-unknown}') — no merge-enabling state (gate is the first filter)" >&2 + echo "to-be-reviewed" + return 0 + fi + + # Review must be an approving verdict; pending/missing/error/unknown stay blocked. + case "$review" in + approved | tech-debt) ;; + *) + echo "pr-state: review is '${review:-unknown}', not an approving verdict — staying to-be-reviewed (fail-safe)" >&2 + echo "to-be-reviewed" + return 0 + ;; + esac + + # Tier requirements: 🔴 (and any unknown tier, fail-safe) needs explicit human approval. + if explicit_approval_required "$tier" && [ "$approval" != "1" ]; then + echo "pr-state: tier '${tier:-unknown}' requires explicit human approval — staying to-be-reviewed" >&2 + echo "to-be-reviewed" + return 0 + fi + + echo "ready-to-merge" +} + +# explicit_approval_required — exit 0 (required) for red and for any +# unknown/absent tier (fail-safe), exit 1 (not required) for green/yellow. +# The requirement itself is the quality model's §4 row, not a rule invented here. +explicit_approval_required() { + case "${1:-}" in + green | yellow) return 1 ;; + red) return 0 ;; + *) + echo "pr-state: unknown tier '${1:-}' — requiring explicit approval (fail-safe red)" >&2 + return 0 + ;; + esac +} + +# merge_allowed — exit 0 ONLY for ready-to-merge; any other state exits 1 +# with the reason on stderr. Never a silent pass. +merge_allowed() { + case "${1:-}" in + ready-to-merge) return 0 ;; + not-approved) + echo "pr-state: state is 'not-approved' — merge blocked, findings go back to the author" >&2 + return 1 + ;; + *) + echo "pr-state: state is '${1:-unknown}' — merge blocked (gates green + approved review required; explicit approval at 🔴)" >&2 + return 1 + ;; + esac +} + +# human_approval_jq_filter — echoes the ONE jq predicate that decides whether an +# explicit human approval (D10) exists on the CURRENT head commit. It ships here, +# next to the synthesis it feeds, instead of being written inline in each code host's +# workflow: the host job and its tests then read the SAME text, so the authorization +# predicate cannot drift between the recipe and what is verified ("one executable +# projection", as for tier-resolve.sh). +# +# Input : a REST `GET /repos/{owner}/{repo}/pulls/{n}/reviews` payload (an array). +# Env : HEAD_SHA (the only commit branch protection evaluates), PR_AUTHOR (login). +# Output : one line per qualifying review id — count them; and always read ALL pages +# (`--paginate`), since an approval can sit past page 1. +# +# Rejects by construction: a non-APPROVED review, an approval on any other commit +# (i.e. stale after a force-push), a non-human account (`user.type != "User"` — bots +# and GitHub Apps, so the pair review itself can never satisfy the gate), and the PR +# author's own approval. +human_approval_jq_filter() { + printf '%s' '.[] | select(.state=="APPROVED" and .commit_id==env.HEAD_SHA and .user.type=="User" and .user.login!=env.PR_AUTHOR) | .id' +} + +# review_check_conclusion — maps a review verdict onto the conclusion the +# REQUIRED `pair-review` check must carry on the code host (R5.7): +# approved | tech-debt ⇒ success +# changes-requested ⇒ failure +# anything else (pending, missing, crashed, timed out, unknown) ⇒ pending +# A pending check blocks the merge exactly like a failing one — a crashed or skipped +# review can never leave the PR mergeable. +review_check_conclusion() { + case "${1:-}" in + approved | tech-debt) echo "success" ;; + changes-requested) echo "failure" ;; + *) + echo "pr-state: review verdict '${1:-}' is not a decision — check stays pending (blocks merge)" >&2 + echo "pending" + ;; + esac +} diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md index e87746f9..fcc523ea 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/README.md @@ -34,6 +34,8 @@ This framework does not cover: **[definition-of-ready-and-done.md](definition-of-ready-and-done.md)** - Canonical Definition of Ready (R3.8, 6 criteria) and Definition of Done (R3.9, 4 criteria + per-tier hooks) — companion to canonical-states.md, the source `/pair-capability-verify-done` and the user-story template read +**[pr-states.md](pr-states.md)** - The PR state flow (gate ≠ review): the 3 PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`), the synthesis of gates × review verdict × tier × explicit approval, and the required `pair-review` / `pair-explicit-approval` checks that make merge blocking mechanical — the pull-request companion to canonical-states.md + ### Implementation Guides **[filesystem-implementation.md](filesystem-implementation.md)** - Complete setup and usage guide for filesystem-based project management diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md index 5b6fc399..8adb59a2 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md @@ -193,6 +193,7 @@ Rollout across the rest of the skill catalog happens organically in the stories ## Related +- [pr-states.md](pr-states.md) — the **pull-request** companion to this document: the three PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`) all live *under* the `Review` macrostate; `Done` is produced by the merge, never by a PR state - [way-of-working.md](../../../../adoption/tech/way-of-working.md) — hosts the optional `## State Mapping` section - [github-implementation.md](github-implementation.md) · [azure-devops-implementation.md](azure-devops-implementation.md) · [filesystem-implementation.md](filesystem-implementation.md) — PM tool status-field mechanics - [decision-records.md](../decision-records.md) — ADR/ADL process (this schema was adopted via ADR) diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md index cacc4b76..142574e0 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md @@ -607,8 +607,285 @@ mcp__github__issue_write: gh issue close [NUMBER] --reason completed ``` +## PR state flow — required checks & branch protection + +GitHub mechanics for the two pair checks the [PR state flow](pr-states.md) requires. The **model** (states, synthesis, tier requirements, edge cases) lives in that document; only host specifics live here (R2.12). + +### The two pair checks + +| Check | Published by | Semantics | +| --- | --- | --- | +| `pair-review` | `/review`, as a **commit status** (registered `pending` at PR creation by `/publish-pr`) | `success` on APPROVED/TECH-DEBT, `failure` on CHANGES-REQUESTED, `pending` when the review has not produced a decision (never ran, crashed, timed out) | +| `pair-explicit-approval` | a workflow job (below), also as a **commit status pinned to the PR head SHA** | `success` when the tier does not require explicit approval, or when a **human** approving review exists on the current head; `failure` at 🔴 without one | + +Both are **required status checks** — a `pending` or absent required check blocks the merge on GitHub exactly like a failing one, which is what makes the review unskippable (R5.7). + +#### What each context proves — and what it does not + +A commit status is written by `POST /repos/{owner}/{repo}/statuses/{sha}`, which **any principal with push access can call, for any context name**, and branch protection evaluates the **most recent** status per context. There is no producer-identity restriction on a status context unless one is pinned in the protection payload. Read the two checks accordingly: + +| Context | What it guarantees | What it does **not** guarantee | +| --- | --- | --- | +| `pair-review` | **Anti-accident**: an absent or `pending` context blocks, so a review that never ran, crashed, or was skipped can never leave the PR mergeable. Verified on a live host. | **Unforgeability.** It is published with the ordinary agent token, so by construction anyone with push access — including the authoring agent — can `POST` `state=success` for the context. It is **not** an authorization control; treat it as "the review cannot be silently skipped", never as "someone independent approved". | +| `pair-explicit-approval` | **Authorization**, to the extent the producer is pinned: the payload below pins the context to the **GitHub Actions app id**, so a status posted by a user token or PAT does not satisfy it, and the job that does post it runs from a trusted ref (property 1 below). | Absolute unforgeability — see the residual immediately below. The only form with no residual is a **GitHub App check-run** whose `app_id` is pinned to an App the repository's own workflows cannot present (Option 4 in ADR-018 — deferred, [#398](https://github.com/foomakers/pair/issues/398)). | + +**Residual on the app pin, and the companion repository settings that shrink it.** A status posted by _any_ workflow using the default `GITHUB_TOKEN` is attributed to the GitHub Actions app, so a pull request that **adds** a `pull_request` workflow declaring `permissions: statuses: write` can post the pinned context itself. Two settings close that in practice — apply both when the flow is used as an authorization control: + +- **Settings → Actions → General → Workflow permissions: _Read repository contents and packages permissions_.** This removes the implicit write token, so any workflow needing `statuses: write` must request it explicitly in its own YAML — visible in the diff instead of ambient. (It does not _prevent_ an explicit request; that is what the next setting is for.) +- **Require review on workflow changes**: a `CODEOWNERS` entry for `/.github/workflows/**` plus `"required_pull_request_reviews": { "require_code_owner_reviews": true }`, so a PR that edits or adds a workflow needs a human code-owner approval before it can merge — the same human signal the 🔴 gate is asking for. + +Stated plainly rather than assumed away: with neither setting applied, `pair-explicit-approval` is hardened (no PAT/user-token path) but not unforgeable, and `pair-review` is forgeable by design. The unforgeable form is the App/check-run path. + +**Variables used by every snippet in this section** — defined once, and every call below uses the **same single-variable form** `repos/$REPO/…` (never a two-variable owner+name form, which is what produces `repos/owner/owner/repo/…` and a 404 when `REPO` already holds `owner/repo`): + +```bash +REPO="$(gh repo view --json nameWithOwner -q .nameWithOwner)" # owner/repo — matches `github.repository` +PR= +HEAD_SHA="$(gh pr view "$PR" --json headRefOid -q .headRefOid)" +``` + +**Token prerequisite (why a commit status, not a check run).** The Checks API (`POST /repos/{owner}/{repo}/check-runs`) is writable **only by a GitHub App installation token**: with an ordinary user token or PAT it answers `403 You must authenticate via a GitHub App`. The skills that publish the verdict (`/publish-pr` Phase 5, `/review` Step 5.4) run agent-side with exactly that ordinary token, so a check run is not an option for them. The **commit-statuses API** accepts the same token and branch protection treats a status **context** as a required check identically. The token needs `repo:status` (classic PAT) / `Commit statuses: write` (fine-grained); inside a workflow that is `permissions: statuses: write`. If a project does publish through a GitHub App instead, keep the check-run form — but then the publication must happen inside a workflow holding `checks: write`, plus a relay that carries the agent's verdict there. + +### Provision the `pr-state:*` labels (once per repository) + +Labels are never auto-created: `gh pr edit --add-label pr-state:to-be-reviewed` fails with `not found` until they exist. + +```bash +gh label create "pr-state:to-be-reviewed" --color "fbca04" --description "PR open, not yet cleared" --force +gh label create "pr-state:ready-to-merge" --color "0e8a16" --description "Gates green + review approved (+ explicit human approval at red)" --force +gh label create "pr-state:not-approved" --color "d73a4a" --description "Review verdict: CHANGES-REQUESTED" --force +``` + +Run this before the first `/publish-pr`. **Non-blocking**: if the labels are absent or the label API is unavailable, the flow reports the gap and continues — the label is only a view, and the required checks remain the merge authority. + +### Publish the `pair-review` status on the head commit + +```bash +# Register it pending at PR creation (merge is blocked from t0, even if the review crashes) +gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state='pending' -f context='pair-review' \ + -f description='Review dispatched — merge blocked until the verdict lands' + +# Publish the verdict (success | failure), mapped by review_check_conclusion +source .pair/knowledge/assets/pr-state.sh +STATE="$(review_check_conclusion "$VERDICT")" # approved|tech-debt ⇒ success · changes-requested ⇒ failure +[ "$STATE" = pending ] && { echo "no decision yet — leaving the pending status in place"; exit 0; } +gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state="$STATE" -f context='pair-review' \ + -f description="$(printf '%.140s' "$VERDICT_SUMMARY")" \ + -f target_url="$REVIEW_URL" +``` + +`review_check_conclusion` returns `pending` when the review produced no decision — the guard above is mandatory: never overwrite the pending status with a resolved one, so the required context stays unsatisfied and the merge stays blocked. (`description` is capped at 140 characters by the API; the full report lives in the native review that `target_url` points at.) + +If the POST is refused (`403`/`404` — token without `repo:status`), enforcement degrades to **advisory**: the verdict is still recorded in the native review and the `pr-state:*` label is still computed, and the skill reports `pair-review: NOT PUBLISHED — advisory` instead of claiming the merge is blocked. + +### `pair-explicit-approval` job (🔴 only, auto-passes below) + +Reads the `risk:*` label only (D18, fail-safe red) and asserts a **human** approval — the pair review's own submission never satisfies it, and `dismiss_stale_reviews` keeps it from surviving a force-push. + +Five properties are load-bearing and easy to get wrong; all five are encoded in the template below, and the first four were **verified on a live repository** (§ Verified on a throwaway repository). + +1. **The job must run from a trusted ref.** This job is an **authorization control**, so its code must not come from the change it authorizes. On a `pull_request` event GitHub runs the **PR's own** version of the workflow file, and `actions/checkout` with the default ref checks out the PR's tree — so a PR could **tamper** with the job body or with the sourced `explicit_approval_required()` and make the required context report `success` with no human approval, i.e. **self-grant** the 🔴 gate. That is not theoretical: it was reproduced (a `risk:red` PR shipping a neutered `explicit_approval_required` published `pair-explicit-approval=success — tier red — explicit approval not required` on its own head). `pull_request_target` runs the **base** version of the workflow, and pinning the checkout to `github.event.pull_request.base.sha` makes the sourced assets base versions too. The pin matters for the review trigger as well: on `pull_request_review` the workflow _file_ comes from the default branch, but `GITHUB_REF` is `refs/pull//merge`, so a **default checkout there also lands on the PR's merged tree**. Distinguish this from workflows whose job body only _narrows_ what is tested — there tampering weakens a check; here it would remove an authorization control. +2. **The verdict must be pinned to the PR head commit.** Branch protection evaluates required contexts **on the head SHA**, and `$GITHUB_SHA` is _not_ that commit for either trigger — measured: `pull_request_target` ⇒ the base branch tip, `pull_request_review` ⇒ the ephemeral `refs/pull//merge` commit. A job that published against `$GITHUB_SHA` would report on a commit the PR's protection never reads, leaving a 🔴 PR blocked _after_ the human approves. The job therefore `POST`s a **commit status** pinned to `github.event.pull_request.head.sha`, which is deterministic and independent of any event-to-SHA association (that is why the workflow needs `statuses: write`). GitHub _does_ currently attach a run's own check-run to the PR head for both triggers (measured), but that association is incidental where this one is explicit. +3. **One producer per required context.** The job is therefore named `explicit-approval-gate`, **not** `pair-explicit-approval`: the required context is the commit status it posts, and giving a check-run the same name would leave two independent producers writing one context. The job's own check-run stays as unrequired, human-readable detail. +4. **Only the newest evaluation may publish.** Two label mutations seconds apart (e.g. `--remove-label risk:yellow --add-label risk:red`) fire two events, and without a concurrency guard a slower older run can overwrite the newer verdict — observed: a stale run re-published the previous tier's result _after_ the current one. A `concurrency` group with `cancel-in-progress` plus a **fresh label read** (the event payload's label list is a snapshot; the API's is current) makes the verdict of the last **completed** evaluation the one that stands. Runs that never complete are property 5's job. +5. **An interrupted evaluation must leave the context blocking.** A job that publishes only at the end leaves the **previous** status standing when it dies mid-flight — and the previous status can be a `success` from a lower tier, so a 🟡 → 🔴 raise whose evaluation is cancelled (`cancel-in-progress`) or aborted (`set -euo pipefail` on a transient `gh` failure, or the base branch not yet carrying `tier-resolve.sh`/`pr-state.sh`) would silently keep the merge enabled at 🔴 with no human approval. Nothing required reports red in that state, because the job's own check-run is deliberately not a required context (property 3). The job therefore posts **`state=pending` on the head SHA as its very first step**, before resolving anything: an interrupted evaluation leaves a pending — i.e. merge-blocking — context, and only a completed one resolves it. Same pending-first pattern `pair-review` already uses. (An `if: ${{ failure() }}` publish step is weaker: it does not run on hard cancellation.) + +```yaml +name: pair-explicit-approval +on: + # `pull_request_target`, NOT `pull_request`: the workflow file and the sourced + # assets must come from the BASE branch (property 1 above). + pull_request_target: + # `labeled`/`unlabeled` are REQUIRED: a tier raised mid-review (🟡 → 🔴) must + # re-evaluate and re-block until a human approval is recorded. + types: [opened, synchronize, reopened, labeled, unlabeled] + pull_request_review: + types: [submitted, dismissed] + +# Only the NEWEST evaluation may publish (property 4): two label mutations seconds +# apart race, and a stale run must never overwrite the current verdict. +concurrency: + group: pair-explicit-approval-${{ github.event.pull_request.number }} + cancel-in-progress: true + +# Least privilege. `statuses: write` is what lets the job pin its verdict to the PR +# head commit (property 2); nothing here ever checks out or executes PR code. +permissions: + contents: read + pull-requests: read + statuses: write + +jobs: + # NOT named `pair-explicit-approval` (property 3): the required context is the + # head-pinned commit status this job posts, and one context = one producer. + explicit-approval-gate: + runs-on: ubuntu-latest + steps: + # PENDING FIRST (property 5), before anything that can fail or be cancelled: + # a run that dies later must not leave a previous `success` standing. No + # checkout needed — this step only writes the blocking placeholder. + - env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state='pending' -f context='pair-explicit-approval' \ + -f description='evaluating tier + human approval on this head' + - uses: actions/checkout@v4 + with: + # TRUSTED REF — the base commit, never the PR head. This is what makes the + # two sourced functions untamperable from the pull request's side. + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + - env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + source .pair/knowledge/assets/tier-resolve.sh # tags only, no criteria + source .pair/knowledge/assets/pr-state.sh + # Labels read FRESH from the API (property 4), not from the event payload: + # a payload snapshot is stale the moment a second label event lands. + PR_LABELS="$(gh pr view "$PR" --repo "$REPO" --json labels --jq '[.labels[].name] | join(" ")')" + TIER="$(resolve_tier "$PR_LABELS")" + STATE=success + DESC="tier $TIER — explicit approval not required" + if explicit_approval_required "$TIER"; then + # A HUMAN approval on the CURRENT head: exclude bots and the PR author. + # The predicate is NOT written out here — it is `human_approval_jq_filter` + # from the sourced pr-state.sh, so this job and the tests that verify it + # read one text and cannot drift (it rejects non-APPROVED, another commit, + # `user.type != "User"`, and the author's own approval). + # Use the REST reviews endpoint — it is the only one carrying BOTH the + # account type and the reviewed commit. `gh pr view --json reviews` exposes + # `author.login` and NO bot flag whatsoever, so filtering there on a + # bot-exclusion field matches nothing and silently yields 0 approvals. + # `--paginate` (no per_page cap): reviews accumulate over a PR's life, so + # the approval can sit on page 2 — a single page would read it as zero and + # block the PR forever. Aggregate by counting the paginated stream. + APPROVALS="$(gh api --paginate "repos/$REPO/pulls/$PR/reviews" \ + --jq "$(human_approval_jq_filter)" | grep -c . || true)" + if [ "${APPROVALS:-0}" -ge 1 ]; then + DESC="explicit human approval recorded on the current head" + else + STATE=failure + DESC="risk:red — needs an explicit human approval on the current head (D10)" + fi + fi + # Pin the context to the PR head commit (property 2). Publish BEFORE failing + # the job, so the required context always reports. + gh api "repos/$REPO/statuses/$HEAD_SHA" -X POST \ + -f state="$STATE" -f context='pair-explicit-approval' \ + -f description="$(printf '%.140s' "$DESC")" + [ "$STATE" = success ] || { echo "::error::$DESC"; exit 1; } +``` + +**Residual caveats of the trusted-ref form**, stated explicitly rather than assumed away: + +- **Forks.** With `pull_request_target` the workflow runs in the **base** repository's context with a write-capable `GITHUB_TOKEN`, which is only safe because this job **never checks out or executes head-ref code** — the checkout is pinned to `base.sha` and no step runs anything from the PR. Never add a step here that builds, installs, or runs the PR's code; put that in the ordinary `pull_request` gate pipeline, which has a read-only token on forks. Conversely, a plain `pull_request` run from a fork gets a **read-only** token and could not `POST` the status at all — another reason for the target trigger. +- **Base-branch assets.** `tier-resolve.sh` and `pr-state.sh` are sourced from the base commit, so they must already exist on the base branch: the workflow is added and observed **before** protection is applied (ordering below), and a PR that only _introduces_ those assets cannot satisfy the context on its own. +- **Projects that refuse `pull_request_target` entirely** (a common blanket policy) get the same guarantee by keeping the plain `pull_request` trigger and writing the projections **inline** in the job body — `resolve_tier` / `explicit_approval_required` are a handful of `case` lines and `human_approval_jq_filter` is one string — with no checkout step at all, so there is nothing from the PR's tree to tamper with. The cost is a second copy of the projection, which must then be kept in sync with the shipped assets (a conformance grep is the usual mitigation). + +### Branch protection payload + +Use the **`checks` form**, not the legacy `contexts` array: `contexts` accepts a status from _any_ source, while each `checks[]` entry may pin an `app_id` — which is what keeps a user/PAT-posted status from satisfying the authorization gate (see § What each context proves). + +```bash +# The producer of `pair-explicit-approval` is the GitHub Actions app (the job posts the +# status with the default GITHUB_TOKEN). Resolve its id rather than hardcoding it: +ACTIONS_APP_ID="$(gh api /apps/github-actions --jq .id)" # 15368 on github.com + +gh api "repos/$REPO/branches/main/protection" -X PUT --input - < Adoption > KB default` cascade. This document copies **none** of those values — not even as an illustration: a project that overrides `tier.red.reviewers` or `tier.red.sla_days` in `tech/risk-matrix.md` changes them in exactly one place, and a restatement here would be the first thing to go stale. The only requirement this flow *names* is the one that changes the merge topology: at 🔴 (`risk:red`) the quality model demands an **explicit human approval** (D10), which the synthesis below consumes as its fourth input. + +**How the pair review maps onto those requirements:** at 🟡 the **pair review satisfies** the tier's single **reviewer approval** — an approving `pair-review` verdict on green gates *is* that approval, and no additional human approval is demanded (so 🟡 differs from 🟢 in gate breadth and in the review being blocking, not in an extra approver). 🔴 is the only tier adding a requirement the review cannot meet by itself: an **explicit human approval** from a non-author account (D10). + +The tier itself is read from the PR's `risk:*` label via [`tier-resolve.sh`](../../../assets/tier-resolve.sh) (`resolve_tier`). **This flow contains no classification criteria** (D18): it never inspects the diff, the code, file paths, or change size — grep-verifiable, in both the guideline and the evaluator. **Fail-safe: an untagged, unknown, or malformed tier is treated as 🔴 red** (consistent with the pre-merge gate's own fail-safe, [tier-aware-pipeline.md](../../infrastructure/cicd-strategy/tier-aware-pipeline.md)) — so a missing classification demands *more*, never less. + +## Enforcement: required checks are the authority, labels are a view + +Merge blocking is **mechanical**. Three required-check layers sit on the protected branch: + +| Layer | Check | Required | Blocks when | +| --- | --- | --- | --- | +| Gate | the pre-merge pipeline's jobs (base, secret-scan, and the suites for the tier) | always | any job red, or a required suite missing | +| Review | **`pair-review`** — the check the review flow publishes on the head commit (on GitHub a **commit status**, which the agent's own token can write; the Checks API cannot — see the implementation guide) | always | conclusion is `failure` (CHANGES-REQUESTED) **or** still `pending`/absent (review never ran, crashed, or was skipped) | +| Explicit approval | **`pair-explicit-approval`** — a deterministic host job that verifies a human approval exists on the current head when the tier demands it | always (auto-passes below 🔴) | tier is 🔴 (or untagged ⇒ 🔴) and no human approval is recorded on the current head | + +Three properties of the explicit-approval layer are **host-agnostic invariants**, not GitHub trivia — an implementation that misses any one of them silently voids AC4: + +- **It runs from a trusted ref.** The job's code (and any shared projection it reads) comes from the **target branch**, never from the pull request's own tree. Otherwise the change under review can rewrite the check that authorizes it and self-grant the 🔴 approval — an authorization control defined by the thing it authorizes is not a control. +- **Its verdict is pinned to the head commit** the host's protection actually evaluates, on **every** re-evaluation — including the one triggered by the approval itself. A verdict recorded against any other commit leaves a 🔴 PR blocked *after* the human approves. +- **Its producer is pinned, and an interrupted evaluation leaves it blocking.** Hosts generally let *any* principal with write access publish *any* check/status name, so the protected branch must constrain **who** may satisfy the authorization context (on GitHub: `checks[].app_id`) — otherwise one API call by the authoring/reviewing agent satisfies the human gate. And because the previous verdict survives a crashed or cancelled evaluation, the job publishes a **pending** result before it starts resolving, so an interrupted re-evaluation blocks instead of leaving a stale `success` from a lower tier. + +### What the two checks prove + +The two layers are **not** equally strong, and conflating them is how AC4 gets quietly lost: + +| Check | Control type | Guarantee | +| --- | --- | --- | +| `pair-review` | **anti-accident** | A review that never ran, crashed, or was skipped leaves the context absent/pending, and the host blocks the merge. It is published with the same credentials the authoring/reviewing agent holds, so **it is forgeable by any principal with write access, by construction** — it proves "the review was not silently skipped", never "an independent party approved". | +| `pair-explicit-approval` | **authorization** | The 🔴 human approval is asserted by a deterministic job the pull request cannot modify, publishing a producer-pinned context — so the reviewing agent cannot grant it. Fully unforgeable only in the host's App/check-run form (see the implementation guide's residuals, and ADR-018 Option 4 — deferred, [#398](https://github.com/foomakers/pair/issues/398)). | + +Because `pair-review` is a **required status check** (R5.7), the review **cannot be skipped**: a PR with no review check, a failed one, or one still pending is not mergeable on the code host — not by convention, but by branch protection. That is a guarantee about *skipping*, not about *identity*: the check does not prove who published the verdict. The identity half is `pair-explicit-approval`'s job at 🔴 (AC4/D10), which is why the flow keeps two checks instead of one (ADR-018). + +**The `pr-state:*` label is a view, not the authority.** The flow keeps exactly one of `pr-state:to-be-reviewed` / `pr-state:ready-to-merge` / `pr-state:not-approved` on the PR so humans and `pair-next`-style automation can filter at a glance. A label is never the thing that permits a merge: editing or forging a label cannot enable a merge, because branch protection evaluates the required checks and ignores labels entirely. If a label and the checks ever disagree, the checks win and the next evaluation re-labels. + +Because it is only a view, the label family is a **prerequisite, not a dependency**: hosts do not auto-create labels, so they are provisioned once per repository (implementation guide). A missing label family — or no label API at all — is **non-blocking**: the flow reports `pr-state label: not applied` and continues, since the required checks remain the merge authority. + +Host mechanics — how the check run is published, the exact branch-protection payload, the `pair-explicit-approval` job — live in the code host's implementation guide (R2.12): [github-implementation.md](github-implementation.md) § "PR state flow — required checks & branch protection". + +## Edge cases + +| Case | Behavior | +| --- | --- | +| **Tier raised after review started** (e.g. review raises 🟡 → 🔴) | Requirements are re-evaluated on the **new** tier: a raise to 🔴 **re-blocks** the merge until explicit human approval is recorded. The label-change event re-runs the gate and re-evaluates `pair-explicit-approval`; the state drops back to `to-be-reviewed`. Tiers are raise-only (D17), so re-evaluation never loosens a requirement. | +| **Review subagent crashes or times out** | The `pair-review` check stays `pending` (or is published as `failure` with the error) — merge stays **blocked**, never silently open. The flow posts a PR comment with the re-run command so a human can restart the review. | +| **Force-push after approval** | Checks and reviews are per head commit: the new head has no `pair-review` check, so the merge is **blocked** — fail-safe by construction. The re-run is **triggered, not automatic**: nothing host-side re-registers `pair-review` on `synchronize`, so a human (or `/publish-pr` / `/review`, re-invoked) starts the review on the new head. A project that wants it automatic adds a `synchronize`-triggered job that re-registers `pair-review` as `pending` on every new head. Branch protection additionally sets `dismiss_stale_reviews`, and `pair-explicit-approval` pins the approval to the current head, so a human approval does not survive a rewrite either. | +| **Code host without a required-check API** | **Degraded mode**, documented rather than silently skipped: the flow still computes and labels the state and still posts the verdict in the native review, but enforcement becomes advisory. `/setup-gates` reports the degradation and points to the host's **manual** branch-protection setup steps in its implementation guide; the project is told, explicitly, that merge blocking is not mechanical until those steps are applied. | +| **PR with no `risk:*` label** | Treated as 🔴 (fail-safe): the widest gate matrix **and** explicit human approval are required. | +| **Self-authored PR** (solo maintainer) | The code host rejects APPROVE on your own PR, so the verdict is recorded as a review comment led by the verdict token (see `/review`'s degradation). The `pair-review` check still carries the machine-readable conclusion, so enforcement is unaffected — 🟢 and 🟡 work normally. **At 🔴 it does not:** the requirement is a non-author human approving review, and the author cannot supply one, so on a single-maintainer repository **no 🔴 PR can reach `ready-to-merge`** — a 🔴 merge needs a **second human account**. This is a real constraint, not a workaround: a solo maintainer either adds a second reviewer account, or deliberately keeps `pair-explicit-approval` out of the required contexts and accepts that the 🔴 human-approval rule is advisory there (recorded, per the degraded-mode row). An alternative solo-approval token (a human-applied `approved:human` label with actor verification, or an `/approve` comment command) is a **design change tracked as [#398](https://github.com/foomakers/pair/issues/398)** — deliberately not part of this flow rather than half-specified here. | + +## Who does what + +| Actor | Responsibility | +| --- | --- | +| `/publish-pr` | Creates the PR, propagates the story's classification tags, registers `pair-review` as **pending**, labels the PR `pr-state:to-be-reviewed`, and triggers the review in a clean-context subagent (AC1) | +| `/review` | Produces the judgment verdict in the native review, publishes the `pair-review` check conclusion (`review_check_conclusion`), computes the state (`resolve_pr_state`), swaps the `pr-state:*` label, and refuses to merge unless `merge_allowed` passes | +| `/implement` Phase 4 | The **other** merge path (the author re-invoked after an approving review): runs the *same* precondition as `/review`'s — re-synthesize the current signals, `merge_allowed`, HALT otherwise — before merging. "At least one approval" is **not** the condition; a 🔴 PR with an approving verdict and no explicit human approval must not merge here either | +| `/setup-gates` | Wires `pair-review` + `pair-explicit-approval` as required checks on the protected branch alongside the gate jobs, or reports degraded mode | +| Code host | Enforces: blocks the merge button while any required check is red, pending, or absent | +| Human | Fixes findings, gives the explicit approval at 🔴, and presses merge | + +## Related + +- [canonical-states.md](canonical-states.md) — the five work-item macrostates (`Review` is the macrostate a PR lives under) +- [quality-model.md](../../quality-assurance/quality-model.md) — §3.2 fail-safe tier, §4 per-tier requirements (the single source of the thresholds this flow reads) +- [tier-aware-pipeline.md](../../infrastructure/cicd-strategy/tier-aware-pipeline.md) — the gate side of the flow (tags-only pre-merge pipeline, fail-safe red) +- [`pr-state.sh`](../../../assets/pr-state.sh) — the provider-agnostic synthesis evaluator (`resolve_pr_state`, `merge_allowed`, `explicit_approval_required`, `review_check_conclusion`, `human_approval_jq_filter`) +- [`tier-resolve.sh`](../../../assets/tier-resolve.sh) — the provider-agnostic, tags-only tier resolver +- [code-review-template.md](../templates/code-review-template.md) — the verdict-first review body whose verdict this flow consumes +- [github-implementation.md](github-implementation.md) — GitHub check-run + branch-protection mechanics for the two pair checks diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md index 7f88b77b..93de841c 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md @@ -201,7 +201,7 @@ Scheduling the jobs is not enough; the code host must **require** them: - Mark `base` and `secret-scan` as required status checks on the protected branch (`main`). - Mark `unit`, `integration`, `e2e` as required **when scheduled**. On green/yellow PRs the higher-tier jobs are not scheduled, so a "required-when-present" convention (or a single aggregating gate job that `needs:` the scheduled set and reports one status) keeps branch protection satisfiable across tiers. - **GitHub note:** a job skipped via a job-level `if:` (as `unit`/`integration`/`e2e` are on lower tiers) reports its required check as **passing**, not pending — GitHub has treated skipped-via-`if:` required checks as successful since ~2021. So on GitHub you can mark all three required directly and lower-tier PRs stay mergeable without an aggregating job; the aggregating-gate pattern above is only needed on hosts that leave an unscheduled required check pending. (This template omits the aggregating job for that reason — add one only if your host needs it.) -- A failing required job (including a missing-suite failure or a secret finding) blocks merge until the gate is green — this is the gate acting before review (R5.4): review starts only at a green gate. +- A failing required job (including a missing-suite failure or a secret finding) blocks merge until the gate is green — this is the gate acting before review (R5.4): review starts only at a green gate. (Refinement: a review may run earlier to report findings, but it can produce **no merge-enabling verdict** at a red gate — see [pr-states.md](../../collaboration/project-management-tool/pr-states.md).) - **A label change must force gate re-evaluation before merge.** Because the tier is read from the `risk:*` label, the `pull_request` trigger includes `labeled`/`unlabeled` (above) so a tier raised mid-review re-runs the gate at the wider matrix. The latest run — not a stale earlier one — is what branch protection evaluates, so a PR can never merge on a gate that ran at a lower tier than its current label. ## Post-merge staging pipeline: build + deploy only diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/quality-assurance/quality-model.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/quality-assurance/quality-model.md index 29a17cb2..cdf3a95a 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/quality-assurance/quality-model.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/quality-assurance/quality-model.md @@ -64,6 +64,13 @@ Cost class = **highest detected signal**. The signal catalog (paid-SDK imports, | 🟡 Yellow | Blocked until reviewed | 1 reviewer | 1 working day | standard | reviewer approval | | 🔴 Red | Blocked until reviewed and approved | 1 reviewer | 2 working days | extended | explicit approval required | +**Checklist depth** is that table's third column and means exactly this — there is no separate "extended checklist" artifact anywhere, and none is needed: + +| Depth | Meaning | +| --- | --- | +| `standard` (🟢/🟡) | The [code-review template](../collaboration/templates/code-review-template.md) — resolved per [template resolution](../technical-standards/ai-development/skill-conventions/template-resolution.md) — as designed: verdict first, the Assessments block, and the Details sections that the change actually touches. Sections with nothing to say are collapsed/omitted. | +| `extended` (🔴) | The **same** template with **no section skipped**: every Assessments subsection (all Security dimensions, Cost, Architecture) and every Details section is answered explicitly — "not applicable" is written out rather than omitted — and the Definition-of-Done check is run in full. Depth, not a different document. | + Reviewer counts and SLAs are **KB defaults** (D10), resolved through the same **Argument > Adoption > KB default** cascade as every other rule in this document — not fixed forever. A project may override either per tier in `tech/risk-matrix.md`'s Overrides section (§6), e.g. requiring 2 reviewers at 🔴 Red for a larger team: ```markdown @@ -73,7 +80,7 @@ Reviewer counts and SLAs are **KB defaults** (D10), resolved through the same ** - tier.red.sla_days: 3 ``` -Review always runs, tests are always green, at every tier (R5.3 + D10) — this part is not overridable, only the reviewer count and SLA are. Gate (mechanical) and review (judgment) are distinct enforcers — gate blocks first, review starts only once gates are green: +Review always runs, tests are always green, at every tier (R5.3 + D10) — this part is not overridable, only the reviewer count and SLA are. Gate (mechanical) and review (judgment) are distinct enforcers — gate blocks first, review starts only once gates are green. **Refinement** (the merge-side companion below, [pr-states.md](../collaboration/project-management-tool/pr-states.md)): a review *may* run at a red gate to report findings early, but it produces **no merge-enabling verdict** — a red gate never yields `ready-to-merge`, whatever the judgment says. "Starts only once gates are green" is about the *merge-enabling* review, not about a prohibition on reading a red-gated diff. | Tier | Gate checks | | --- | --- | @@ -83,6 +90,8 @@ Review always runs, tests are always green, at every tier (R5.3 + D10) — this Install runs at every tier (implicit in each row). Deterministic secret scanning also runs at **every** tier, unconditionally — it is not tier-scoped (security/[secret-scanning.md](security/secret-scanning.md)). How this matrix becomes an actual pipeline that reads the `risk:*` tag only (fail-safe 🔴 when untagged, explicit failure on a missing suite, build+deploy-only post-merge staging) is the delivery-side companion [tier-aware-pipeline.md](../infrastructure/cicd-strategy/tier-aware-pipeline.md) — this table stays the single source of the criteria; that document owns the wiring. +How the two enforcers combine into a merge decision — the PR states (`to-be-reviewed` → `ready-to-merge` / `not-approved`), the synthesis of gates × verdict × tier × explicit approval, and the required checks that make the review unskippable — is the review-side companion [pr-states.md](../collaboration/project-management-tool/pr-states.md). It **reads** the requirements above (including the 🔴 explicit-approval row) and declares none of its own. + ## 5. Tag Projection Chromatic, no semantic tag beyond color. **`risk:green|yellow|red` (§3.2) is the only tag family the KB names and proposes by default.** diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md b/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md index d30fa0f6..316706d5 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/how-to/11-how-to-code-review.md @@ -99,7 +99,7 @@ Follow [project management tool guidelines](../guidelines/collaboration/project- | Decision | Condition | Next Step | | --------------------- | ------------------------------------------------------- | ------------------------------------------------------------------------ | -| **APPROVED** | All requirements met, quality gates pass | Squash merge, mark story "Done" | +| **APPROVED** | All requirements met, quality gates pass | Squash merge **only when the PR state is `ready-to-merge`** (`merge_allowed`: required checks green — `pair-review` + `pair-explicit-approval` at 🔴 — see [pr-states.md](../guidelines/collaboration/project-management-tool/pr-states.md)), then mark story "Done" | | **CHANGES REQUESTED** | Critical issues, missing ADR, failing tests, AC not met | Return to [10-how-to-implement-a-task.md](10-how-to-implement-a-task.md) | | **TECH DEBT** | Only minor issues, tracked as debt | Approve PR, create debt items | diff --git a/packages/knowledge-hub/dataset/.skills/capability/publish-pr/SKILL.md b/packages/knowledge-hub/dataset/.skills/capability/publish-pr/SKILL.md index cadb2fcc..9b06cbf1 100644 --- a/packages/knowledge-hub/dataset/.skills/capability/publish-pr/SKILL.md +++ b/packages/knowledge-hub/dataset/.skills/capability/publish-pr/SKILL.md @@ -1,17 +1,19 @@ --- name: publish-pr -description: "Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, and updates the board state. Standalone — driven by a handoff/checkpoint, not by /implement having run in the same session. Composed by a future closing phase of /implement; reused by hotfix and automation loops. Composes /verify-quality, /checkpoint, /write-issue." -version: 0.6.0 +description: "Publishes a completed story branch as a pull request: runs the quality gate, creates or updates ONE PR from the pr-template (conditional sections filled only when pertinent), copies the story's classification tags, marks it ready-for-review, updates the board state, then enters the PR state flow — registers the required `pair-review` check as pending (merge blocked from t0) and dispatches the review to a clean-context subagent. Standalone — driven by a handoff/checkpoint, not by /implement having run in the same session. Composed by /implement's closing phase (Step 3.3); reused by hotfix and automation loops. Composes /verify-quality, /checkpoint, /write-issue." +version: 0.5.0 author: Foomakers --- # /publish-pr — Publish a Story Branch as a PR -Take a completed story branch to a review-ready pull request in one standalone step: **gate → compose PR → propagate tags → ready-for-review → board state**. Reliable on a clean context (input is a handoff document, not session memory) and reusable outside `/implement` — hotfix branches and automation loops (#212, G10) invoke it directly. +Take a completed story branch to a review-ready pull request in one standalone step: **gate → compose PR → propagate tags → ready-for-review → board state → review dispatch**. Reliable on a clean context (input is a handoff document, not session memory) and reusable outside `/implement` — hotfix branches and automation loops (#212, G10) invoke it directly. **One PR per story:** the story lands on ONE branch with ONE PR. If a PR already exists for the branch, this skill UPDATES it — it never opens a second PR for the same story. -**Never merges.** This skill stops at a ready-for-review PR. Merge is a separate, human-gated step (`/review` / `/implement` Phase 4). +**Never merges.** This skill stops at a PR under review. Merge is a separate, human-gated step (`/review` / `/implement` Phase 4). + +**The review is never optional.** Every PR this skill publishes enters the [PR state flow](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) as `to-be-reviewed` with the **required** `pair-review` check registered as pending (so the merge is blocked from t0), and the review itself is dispatched to a clean-context subagent (Phase 5). This skill never renders the verdict — it only guarantees a review is pending and mechanically enforced. ## Composed Skills @@ -107,6 +109,32 @@ Each phase follows the **check → skip → act → verify** pattern. Phases run 7. **Act — board state:** update the story's board state on the **PM tool** via the `## State Mapping` (canonical target: `Review`), using `/write-issue` (default write mode, `$status: Review`) when installed. If `/write-issue` is not installed or the PM tool is inaccessible, warn and continue — the PR is already ready. PR state itself is never mirrored onto the board. 8. **Verify**: A single ready-for-review PR exists on the code host, tags reflect the story (or their absence is noted), the cross-link exists in both directions when the tools differ — **exactly one** back-link comment, whether this run posted it or found it (or the missing back-link is reported) — and the board state is updated (or the failure is reported). +### Phase 5: Enter the PR State Flow & Dispatch the Review (AC1) + +The PR is ready; it must now be **under review and mechanically blocked** — see the [PR state flow](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the state model, the required checks, and the host mechanics ([github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § PR state flow). + +1. **Check**: Does the current head commit already carry a `pair-review` check (from an earlier run on the same head)? +2. **Skip**: If a `pair-review` check already exists on this head **and** a review has been submitted for it, do nothing here — the PR is already in the flow (idempotency). A new head commit always needs a fresh check + review. +3. **Act — register the check as pending**: publish `pair-review` on the head commit with a pending status **before** dispatching the review, using the mechanism the host's implementation guide prescribes for an ordinary (non-App) agent token — on GitHub a **commit status**, not a check run. This is what blocks the merge from t0: a crashed or never-started review leaves the required check unsatisfied instead of leaving the PR mergeable. If the publication is **refused** (missing token scope, no status API), do not claim enforcement: report `pair-review: NOT PUBLISHED — advisory` and continue (degradation below). +4. **Act — label the state**: apply `pr-state:to-be-reviewed`, removing any other `pr-state:*` label (exactly one at a time). The label family is provisioned once per repository (implementation guide) and does **not** auto-create; if the label is absent or the label API is unavailable, report it and continue — **non-blocking** (degradation below). The label is a **view** — the required checks are the authority (pr-states.md). +5. **Act — hand the review dispatch to whoever can actually spawn it (AC1)**: the review must run on a **clean context** — an **anonymous** subagent whose prompt is the PR reference only, no context from this session (mechanical isolation, D23: the reviewing context must not inherit the authoring context). **Who spawns it depends on where this skill is running**, because harnesses commonly forbid **nested** subagent dispatch — and this skill is itself frequently running *inside* a subagent (`/implement` Step 3.3 publishes the PR from a handoff-only subagent), where spawning a second level would fail on the primary path: + - **This skill is running inside a subagent** (or nested dispatch is unavailable/refused) → do **not** attempt to nest. Emit the signal **`Review: review-dispatch-required — /review $pr=`** and return it to the caller; **the caller dispatches** (for `/implement`, Step 3.3 in the top-level session — the actor is named there). The `pair-review` check is already `pending`, so the merge stays blocked until the review lands: nothing is lost, and the dispatch happens one frame up where it is allowed. + - **This skill is running at the top level** (invoked directly in a session that can spawn subagents) → spawn the anonymous review subagent here, now. +6. **The dispatch prompt** (identical for either actor) is the PR reference plus a **bounded** instruction — the review must not merge: + + ```text + Run /review $pr= $dispatched=true. + Phases 1–5 only: produce the verdict, publish the `pair-review` check, synthesize the PR state. + NEVER run Phase 6 and NEVER merge, even if the verdict is APPROVED and every gate is green — + the merge is a human act (pr-states.md). Stop after Phase 5 and report. + ``` + + `/review`'s own non-interactive contract (its Arguments section) already forbids Phase 6 and defines what to do at its two human prompts (Step 1.4 "Proceed with review?" → continue without asking; Step 5.5 "Merge now?" → always "the author merges"), so a dispatched review neither stalls on an unanswerable question nor self-answers itself into a merge. The prompt states it too: belt and braces, since the constraint is an authorization boundary. + +7. **Act — degraded path (no dispatch anywhere)**: if neither this skill nor its caller can spawn a subagent, do **not** run the review inline in this session (a self-context review defeats the isolation). Leave `pair-review` pending, note `Review: pending — dispatch unavailable, run /review in a fresh session` in the output, and post the same instruction as a PR comment. The merge stays blocked meanwhile, so nothing is lost — only deferred. +8. **Act — dispatch failure**: if the subagent fails or times out, the pending `pair-review` check stays in place (merge blocked) and the re-run guidance is posted as a PR comment. Re-invoking this skill re-dispatches (idempotent). +9. **Verify**: The PR carries a `pair-review` check on its head commit, exactly one `pr-state:*` label, and either a dispatched/submitted review or a recorded reason why it is still pending (`review-dispatch-required` counts as recorded **only** because the caller's contract picks it up — see Composition Interface). This skill never publishes a verdict itself and never merges. + ## Output Format ```text @@ -120,17 +148,20 @@ PUBLISH-PR REPORT: ├── Code host: [same as PM tool | (board updates → PM tool)] ├── Cross-link: [n-a (single tool) | Refs: + PR URL posted on | already linked — comment present, not re-posted | back-link failed — manual link needed] ├── Conditional: [Services to Release: N deployable packages / n-a | Screenshots: UI touched / n-a] -└── Board: [→ Review | not updated — reason] +├── Board: [→ Review | not updated — reason] +├── PR state: [pr-state:to-be-reviewed] +└── Review: [dispatched — subagent (clean context) | review-dispatch-required — /review $pr= (caller dispatches) | pending — dispatch unavailable, run /review | already submitted on this head] -RESULT: [PR READY FOR REVIEW | HALTED — ] +RESULT: [PR UNDER REVIEW — merge blocked by required check `pair-review` | HALTED — ] ``` ## Composition Interface -When composed by a future closing phase of `/implement` (wired in #256): +When composed by `/implement`'s closing phase (Step 3.3): - **Input**: `/implement` invokes `/publish-pr` after the last task's commit, passing `$story` (and, when it wrote one, the checkpoint as `$handoff`). `/implement` owns task iteration; `/publish-pr` owns the gate→PR→board sequence. - **Output**: The PR number/URL and board-state result flow back to `/implement`'s Phase 3 output. A HALTed gate propagates as `/implement`'s HALT. +- **Review-dispatch contract (AC1)**: `/implement` publishes the PR from a **handoff-only subagent**, so this skill's Phase 5 runs one level down and must not nest a second subagent. It therefore returns **`Review: review-dispatch-required — /review $pr=`**, and `/implement` Step 3.3 — running in the **top-level** session — spawns the anonymous review subagent with the bounded prompt (phases 1–5, never Phase 6/merge). The `pair-review` check is already `pending` before the return, so the merge is blocked across the handoff. When this skill runs at the top level instead, it dispatches itself and returns `Review: dispatched`. When invoked **independently** (hotfix, automation loop #212): @@ -155,11 +186,18 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **No classification tags on the story**: create the PR without tags and note it (edge case) — never invent tags. - **`/checkpoint` not installed**: gather state from branch + story directly (Phase 0). - **`/write-issue` not installed**: skip the board-state update, warn, leave the PR ready. +- **Nested subagent dispatch unavailable** (Phase 5 — the common case: this skill is itself running in `/implement`'s handoff subagent and the harness forbids a second level): return `Review: review-dispatch-required — /review $pr=` and let the **caller** dispatch (`/implement` Step 3.3). This is the primary path when nested, not a degradation — the review still runs, one frame up, on a clean context. +- **Subagent spawning unavailable anywhere** (Phase 5 — the caller cannot spawn either, or this skill was invoked at top level in an environment without subagents): leave `pair-review` pending, post the re-run instruction as a PR comment, and note the deferral in the output. Never run the review inline in the authoring session (isolation, D23) and never mark the PR reviewed. Merge stays blocked — the outcome is deferred, not skipped. +- **Code host has no check-run/required-check API** (Phase 5): publish the state as a `pr-state:*` label only, note `enforcement: advisory — see host manual setup` in the output, and continue. Documented degradation, never a silent claim of enforcement (pr-states.md). +- **`pair-review` publication refused** (Phase 5 — token without the status scope, host API rejects it): the check is not published, so enforcement is **advisory**; report `pair-review: NOT PUBLISHED — advisory` (with the host error) and still dispatch the review. Never report a blocked merge that is not blocked. +- **`pr-state:*` label absent / no label API** (Phase 5): report `pr-state label: not applied` and continue — **non-blocking**, the required checks remain the merge authority. The labels are provisioned once per repository (host implementation guide); never invent a different label name. +- **`/review` not installed**: still register the pending `pair-review` check and label the state, then report that no reviewer flow is available. The PR stays blocked rather than silently mergeable. ## Notes -- This skill **creates git-host artifacts** (a pushed branch, one PR) and updates board state — it does not modify source files and never merges. -- **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). Re-invocation detects the existing PR and updates it in place; detects an existing back-link comment and does not post a second one (Phase 4 step 5's Check — the one composed step that cannot dedupe itself); re-runs the gate (fast if already green); re-parses the handoff. Never a duplicate PR, never a duplicate back-link. +- This skill **creates git-host artifacts** (a pushed branch, one PR, a pending `pair-review` check, a `pr-state:*` label) and updates board state — it does not modify source files, never renders a review verdict, and never merges. +- **Gate ≠ review** ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): the Phase 1 gate is mechanical; the judgment verdict belongs to `/review`, dispatched here on a clean context and enforced by the required `pair-review` check (R5.7). +- **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). Re-invocation detects the existing PR and updates it in place; re-runs the gate (fast if already green); re-parses the handoff. Never a duplicate PR. - Tag propagation is a **copy**; the authoritative classification is (re)done in `/review` (G6). - The gate here is a local pre-flight only — CI remains authoritative (#210). - The handoff/checkpoint is the input contract (see the [checkpoint template](../../../.pair/knowledge/guidelines/collaboration/templates/checkpoint-template.md)); it is consumed here, never loaded as ambient context elsewhere. diff --git a/packages/knowledge-hub/dataset/.skills/capability/setup-gates/SKILL.md b/packages/knowledge-hub/dataset/.skills/capability/setup-gates/SKILL.md index b5acbb46..c97712f4 100644 --- a/packages/knowledge-hub/dataset/.skills/capability/setup-gates/SKILL.md +++ b/packages/knowledge-hub/dataset/.skills/capability/setup-gates/SKILL.md @@ -1,7 +1,7 @@ --- name: setup-gates -description: "Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24) — for the adopted tech stack. Invoke directly to set up or reconfigure gates; idempotent — confirms existing configuration rather than re-configuring." -version: 0.5.1 +description: "Configures CI/CD quality gates — pipeline config, shared lint/format packages, husky hooks, the required deterministic secret-scanning job (gitleaks by default, R6.5/D24), and the required pair-review + pair-explicit-approval branch-protection checks that make the review unskippable — for the adopted tech stack. Invoke directly to set up or reconfigure gates; idempotent — confirms existing configuration rather than re-configuring." +version: 0.6.0 author: Foomakers --- @@ -125,11 +125,26 @@ Configure CI/CD quality gates for the project. Reads quality assurance guideline - **`TIERING = disabled` (default) → full-suite pipeline**: every PR runs base (install + lint + type + build) **and** unit **and** integration/E2E **and** coverage — all jobs unconditional, no tag resolution, no `risk:*` reads. This is the safe default; do **not** generate the tier-aware jobs. - **`TIERING = enabled` (opt-in) → tier-aware pipeline** following [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md): the pipeline **reads the PR's `risk:*` tag only** (via [`tier-resolve.sh`](../../../.pair/knowledge/assets/tier-resolve.sh) — tags only, no classification criteria of its own, D18) and schedules the [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 matrix: base every tier, unit from 🟡, integration/E2E from 🔴; **untagged/unknown/malformed ⇒ 🔴** (fail-safe); a required suite missing at a tier ⇒ **explicit failure** (`require_suite`), never a silent pass. Do not copy any tier threshold into the generated config — it is a projection of the model, not a second source. - **Coverage job (opt-in)**: emit the `coverage` job from [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) **only when `COVERAGE_GUARDRAIL = enabled`** (Step 3.4). It sources [`coverage-gate.sh`](../../../.pair/knowledge/assets/coverage-gate.sh) (config + a number only, no criteria — D18), reads `tech/coverage-baseline.md`, and blocks a regression below the committed baseline (maintain/improve passes; bootstrap-only when unset). With the flag absent (default) do **not** emit a coverage job in either pipeline mode. - - **Wire required checks**: mark `base` and `secret-scan` as required status checks on the protected branch. In full-suite mode all suite jobs are required; in tier-aware mode the tier-scoped jobs are required-when-scheduled (or via one aggregating gate job). A red gate blocks merge either way. The coverage job (when enabled) is required-when-scheduled like the other tier-scoped jobs. + - **Wire required checks**: mark `base` and `secret-scan` as required status checks on the protected branch. In full-suite mode all suite jobs are required; in tier-aware mode the tier-scoped jobs are required-when-scheduled (or via one aggregating gate job). A red gate blocks merge either way. The coverage job (when enabled) is required-when-scheduled like the other tier-scoped jobs. The two **pair review** checks are wired in Step 4.5 — they are required in both pipeline modes and are not tier-scoped away. - **Post-merge staging** (on merge to the main branch): **build + deploy only, no gate re-run** — the gate already passed pre-merge (identical in both modes). 3. **Act**: Write the secret-scanning job — required, not an optional proposal line, in **both** pipeline modes: resolve the scanner (Argument > Adoption's Custom Gate Registry override > KB default gitleaks), then write the [CI Job Template](../../../.pair/knowledge/guidelines/quality-assurance/security/secret-scanning.md#ci-job-template-github-actions) into the same pipeline file as a `required` job, and provision a starting `.gitleaks.toml` at the project root (see the [allowlist mechanism](../../../.pair/knowledge/guidelines/quality-assurance/security/secret-scanning.md#allowlist-mechanism-adoption-controlled)) if one doesn't already exist. Never write the job with `continue-on-error` — fail-closed is not optional (R6.5). 4. **Verify**: Configuration files written, including the pre-merge pipeline (full-suite by default; tag-only tier-aware resolution + tier-scoped jobs only when `TIERING = enabled`) + required-check wiring, the post-merge staging (build+deploy only) config, the secret-scanning job, and `.gitleaks.toml`. If tiering is enabled, grep the generated pipeline for classification tokens (schema/diff/path heuristics) — there must be none (D18). If disabled, confirm no `risk:*` resolution was generated and every suite job runs unconditionally. +### Step 4.5: Wire the Pair Review Required Checks + Branch Protection + +The gate is only half the merge block. The judgment review must be **unskippable**, which means it is a required check like any mechanical one — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the state model and the two check contracts. This step is **host-agnostic**: the concrete API calls live in the code host's implementation guide (R2.12), e.g. [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § "PR state flow — required checks & branch protection". + +1. **Check**: Does the protected branch already require `pair-review` and `pair-explicit-approval`? Read the current branch-protection configuration from the code host. +2. **Skip**: If both contexts are already required (and stale-review dismissal is on) → confirm and move to Step 5. Idempotent: never re-apply an identical protection payload. +3. **Act — prerequisites first (ordering is load-bearing)**: before writing any protection, (a) **provision the `pr-state:*` labels** from the host guide (they never auto-create; absence is non-blocking but the view is lost), (b) **write the `pair-explicit-approval` job** into the repository's workflows on the **target branch** — neither needs admin scope — and (c) **observe both contexts reporting on a real PR, twice**: at PR open, and again after a review submission (the approval-time re-evaluation), each time **on the PR's head commit** — that is the only commit the host's protection reads. Requiring a context that has never reported leaves **every** PR permanently unmergeable, and a context that reports only at PR open leaves a 🔴 PR blocked *after* the human approves; so protection is applied last, with the host's admin-bypass setting (`enforce_admins` on GitHub) enabled only after one PR has merged through the new rule. +4. **Act — required checks**: add both contexts to the protected branch's required status checks, alongside the gate jobs from Step 4: + - **`pair-review`** — published by `/review` on the head commit (pending at PR creation via `/publish-pr`). Required in **both** pipeline modes and at **every** tier: it is never tier-scoped away, exactly like `secret-scan`. A `pending`/absent check blocks the merge, so a review that never ran or crashed cannot yield a mergeable PR (R5.7). + - **`pair-explicit-approval`** — verifies a **non-author human** approval on the current head when the tier requires it (🔴, and untagged ⇒ 🔴 fail-safe); auto-passes at 🟢/🟡 (D10). Generate its job from the host guide's template **unmodified in three respects**, because all three are authorization properties rather than style: it must run from a **trusted ref** (the target branch's version of the job and of any projection it sources — never the pull request's own tree, or the change under review could rewrite the check that authorizes it); its verdict must be **pinned to the PR head commit** on every re-evaluation (including the approval-triggered one), since that is the commit protection evaluates; and it must publish a **pending result as its first step**, so a cancelled or aborted re-evaluation leaves a merge-blocking context instead of a stale `success` from a lower tier. When registering it, **pin the context's producer** where the host allows it (on GitHub: the `checks[].app_id` form, not the legacy `contexts` array) — an unpinned status context can be satisfied by any principal with push access, including the reviewing agent, which would void the 🔴 gate. `pair-review` stays unpinned by construction (no app publishes it): it is an anti-accident control, not an authorization one — see the host guide's "What each context proves". It reads the `risk:*` label only — no classification criteria (D18). **Single-maintainer repositories**: the host rejects a self-approval, so 🔴 PRs cannot satisfy this context without a second human account — say so explicitly and offer to leave the context out of the required list (🔴 rule advisory, recorded) rather than making every 🔴 PR unmergeable. + - Also state the **approval count** explicitly in the protection payload when the host has one (GitHub: `required_approving_review_count: 0`) — the tier-scoped job is the approval authority; an unstated default of ≥1 would demand a human approval on **every** PR, contradicting the 🟢 self-merge row of quality-model §4. +5. **Act — stale-approval invalidation**: enable the host's dismiss-stale-reviews equivalent so a force-push invalidates a previous human approval (pr-states.md edge case). +6. **Act — degraded mode**: if the host has **no required-check / branch-protection API**, or the token lacks permission, do **not** silently continue: report `Required checks: DEGRADED — enforcement advisory` and emit the host guide's **manual** setup steps (which contexts to require, where). Record the gap in [way-of-working.md](../../../.pair/adoption/tech/way-of-working.md)'s Quality Gates section so it is visible until applied. +7. **Verify**: Both contexts are required on the protected branch (or the degradation is reported with manual instructions), the labels exist (or their absence is reported as non-blocking), the `pair-explicit-approval` job is committed to the repository, and stale approvals are dismissed on force-push. Grep the generated `pair-explicit-approval` job for classification tokens — there must be none (D18). + ### Step 5: Provision Shared Lint/Format Config + Hooks 1. **Check**: For each config already detected in Step 1 that is not conflicting, skip provisioning it and note it as already present. @@ -167,6 +182,7 @@ GATE CONFIGURATION COMPLETE: ├── Pre-merge tiering: [disabled (default — full suite every PR) | enabled (risk-tier-scoped, opt-in)] ├── Coverage guardrail: [disabled (default — no coverage job) | enabled (opt-in — coverage job + tech/coverage-baseline.md created)] ├── Required checks: [base + secret-scan required; full-suite: all suite jobs required | tier-aware: tier-scoped jobs required-when-scheduled] +├── Pair review: [pair-review + pair-explicit-approval required on (stale approvals dismissed) | DEGRADED — enforcement advisory, manual setup steps emitted | already configured] ├── Secret Scan: [gitleaks (KB default) | — job + .gitleaks.toml written | already configured] ├── Shared configs: [package list | N/A — non-JS, documented pointer only] ├── Hooks: [husky pre-commit + pre-push | override: | N/A] @@ -200,6 +216,7 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **Full checks are the default; risk-tier reduction is opt-in** (ADL 2026-07-20): unless the project sets `Pre-merge tiering: enabled` in way-of-working.md (Step 3.3 opt-in), the generated pipeline runs the **full suite on every PR** — the safe, pre-tiering behavior. Tiering only *reduces* checks on lower-risk PRs, so it must be enabled explicitly. - **Coverage guardrail is opt-in, off by default** (story #282): the `coverage` regression job and `tech/coverage-baseline.md` are generated **only** when the project sets `Coverage guardrail: enabled` (Step 3.4 opt-in) — the twin of the tiering opt-in. Absent the flag, no coverage job and no baseline file. The baseline is human-committed; the gate bootstraps advisory-only (suggests on stderr, never persists — a CI checkout is ephemeral; automated commit-back is story #372). See [coverage-config-example.md](../../../.pair/knowledge/assets/coverage-config-example.md). - **When tiering is enabled, the generated pipeline reads classification tags only, never classifies** (D18): tier criteria live in [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §3/§4; the pipeline is the deterministic Automation layer that projects the matrix from the PR's `risk:*` tag. Untagged ⇒ 🔴 (fail-safe). See [tier-aware-pipeline.md](../../../.pair/knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md). +- **The pair review is a required check, not a convention** (story #234): Step 4.5 wires `pair-review` + `pair-explicit-approval` on the protected branch so the judgment review is mechanically unskippable (R5.7) and 🔴 PRs need an explicit human approval (D10). The state model, the check contracts, and the degraded mode live in [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md); host API specifics live in the host's implementation guide (R2.12). This skill wires the checks — it never renders a verdict and never computes a PR state. - **Secret scanning is CI config, not a judgment call** (D24, anti-complexity): this skill provisions the job mechanically; it never evaluates whether a diff contains a secret itself — that is gitleaks' (or the adopted scanner's) job at runtime, with no LLM in the loop. `/assess-security` never re-implements this — see that skill's own Notes. - **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md). This skill's check: an already-configured project (incl. provisioned shared configs and hooks) is confirmed; update only on explicit developer request. Conflicting local config is always resolved by asking first (see Edge Cases above). - Gate commands must be executable in the project's development environment. Verify commands exist before writing. diff --git a/packages/knowledge-hub/dataset/.skills/process/implement/SKILL.md b/packages/knowledge-hub/dataset/.skills/process/implement/SKILL.md index 03e368e4..535194bd 100644 --- a/packages/knowledge-hub/dataset/.skills/process/implement/SKILL.md +++ b/packages/knowledge-hub/dataset/.skills/process/implement/SKILL.md @@ -305,14 +305,25 @@ The PR is produced on a **guaranteed-clean context**: a fresh subagent whose ent 1. **Check**: Is subagent spawning available in this tool/environment? 2. **Act — primary path (subagent)**: Spawn an **anonymous subagent** whose prompt is the **handoff document only** — the checkpoint contents (or the path `.pair/working/checkpoints/.md`) plus a single instruction: run `/publish-pr $story= $handoff=.pair/working/checkpoints/.md`. Pass **no other session context** — the subagent's context is the handoff and nothing else (clean context / fresh context reset within one execution, R4.1). The subagent runs `/publish-pr` (gate → PR → tags → ready-for-review → board) and returns the PR number/URL and board result. 3. **Act — degraded inline path (AC3)**: If subagent spawning is **unavailable** (tool/environment cannot spawn one — subagent spawning unavailable), do NOT skip the split: the checkpoint is already written (Step 3.2), then compose `/publish-pr` **inline** in the current session (`$story`, `$handoff` = the checkpoint). **Note the degradation in the output** (`Context: degraded — inline publish, no subagent reset`). Behavior is otherwise equivalent to the primary path. -4. **Act — edge case (subagent fails mid-PR)**: The checkpoint remains valid. Re-invoking `/implement` re-runs this closing phase; `/publish-pr` is **idempotent** — it detects an existing PR and updates it in place rather than opening a second one. Never open a second PR for the story. -5. **Verify**: A single ready-for-review PR exists (created or updated), and its number/URL is captured for the output. A red quality gate inside `/publish-pr` propagates here as a **HALT** (no PR side effects) — implement does not create the PR itself. +4. **Act — dispatch the review (AC1 of #234; this session is the actor)**: `/publish-pr` Phase 5 registers the `pair-review` check as **pending** (merge blocked from t0) but cannot spawn the review subagent from inside the handoff subagent — nested dispatch is commonly forbidden — so it returns **`Review: review-dispatch-required — /review $pr=`**. When that signal comes back, **this top-level session** spawns the anonymous review subagent (a sibling of the publish subagent, not a nested one), prompt = the PR reference plus the bounded instruction: + + ```text + Run /review $pr= $dispatched=true. + Phases 1–5 only: verdict, `pair-review` check, PR-state synthesis. + NEVER run Phase 6 and NEVER merge, whatever the verdict — the merge is a human act. + ``` + + Pass no authoring context (D23 isolation: the reviewer must not inherit the author's assumptions). If `/publish-pr` returns `Review: dispatched` instead (it ran at top level), there is nothing to do here. If this session cannot spawn the reviewer either, record `Review: pending — dispatch unavailable, run /review in a fresh session`: the merge stays blocked, so the review is deferred, never skipped. +5. **Act — edge case (subagent fails mid-PR)**: The checkpoint remains valid. Re-invoking `/implement` re-runs this closing phase; `/publish-pr` is **idempotent** — it detects an existing PR and updates it in place rather than opening a second one. Never open a second PR for the story. +6. **Verify**: A single ready-for-review PR exists (created or updated), its number/URL is captured for the output, and the review is either dispatched or explicitly recorded as deferred. A red quality gate inside `/publish-pr` propagates here as a **HALT** (no PR side effects) — implement does not create the PR itself. Note: the checkpoint is **not** removed here — it must survive the review/fix loop so a re-review or fix round can resume from it. Cleanup happens at merge (Phase 4). ## Phase 4: Post-Review Merge -After code review approval (typically via `/review`), re-invoke `/implement` to merge and close — see [post-review-merge.md](post-review-merge.md) for the verify-approval, merge-commit, merge, parent-cascade, and checkpoint-cleanup steps (Steps 4.1–4.5) plus the completion output. +After code review approval (typically via `/pair-process-review`), re-invoke `/implement` to merge and close — see [post-review-merge.md](post-review-merge.md) for the merge-precondition, merge-commit, merge, parent-cascade, and checkpoint-cleanup steps (Steps 4.1–4.5) plus the completion output. + +This is one of the flow's **two** merge paths (the other is `/pair-process-review` Phase 6), and both carry the **same** blocking precondition: the PR-state synthesis must be `ready-to-merge` (`merge_allowed` from [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh)), re-evaluated on the current head — an approving review alone is not the condition. See [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). On story completion (Done, at merge), the checkpoint is no longer needed — `post-review-merge.md` Step 4.5 removes `.pair/working/checkpoints/.md` so finished-story state never lingers (checkpoint lifecycle: written at the closing phase, cleaned up at merge). @@ -330,6 +341,7 @@ IMPLEMENTATION COMPLETE: ├── Checkpoint: [.pair/working/checkpoints/.md — written] ├── Context: [clean — subagent handoff-only | degraded — inline publish, no subagent reset] ├── PR: [#PR-number — URL — Created | Updated (from /publish-pr)] +├── Review: [dispatched — subagent (clean context) | pending — dispatch unavailable, run /review ] └── Quality: [All gates passing] ``` @@ -347,7 +359,7 @@ Implementation stops immediately when: - **Commit template not found** (Step 2.8 / Step 3.1) — cannot commit without template - **`/publish-pr` not installed** (Step 3.3) — implement composes the PR sequence, never re-implements it - **Quality gate red inside `/publish-pr`** (Step 3.3) — propagates as implement's HALT; no PR side effects (the PR-template-not-found and gate HALTs live in `/publish-pr`) -- **PR not approved** (Step 4.1) — cannot merge without review approval +- **PR state is not `ready-to-merge`** (Step 4.1) — `merge_allowed` fails: red gate, review not approved/still pending, or 🔴 without an explicit non-author human approval on the current head. Never bypass a required check to merge On HALT: report the blocker clearly, propose resolution, wait for developer. @@ -361,7 +373,7 @@ See [idempotency convention](../../../.pair/knowledge/guidelines/technical-stand 4. **Tasks**: scans task checklist and git log to identify completed tasks. Skips them. 5. **PR**: the closing phase re-composes `/publish-pr`, which detects an existing PR and updates it in place — never a duplicate. A subagent that failed mid-PR is recovered this way (the checkpoint stays valid, the rerun is idempotent). 6. **Quality gates**: re-runs all gates (fast if already passing). -7. **Merge**: if PR exists and is approved, proceeds directly to Phase 4 (merge). +7. **Merge**: if a PR exists and its state synthesizes to `ready-to-merge`, proceeds directly to Phase 4 (merge); otherwise Phase 4's Step 4.1 HALTs with the unmet condition. The skill resumes from the first incomplete step — never re-does completed work. @@ -369,7 +381,7 @@ The skill resumes from the first incomplete step — never re-does completed wor See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/graceful-degradation.md) (PM tool not accessible → ask the developer directly; adoption file missing → proceed with guideline defaults) for the standard scenarios. Additional cases: -- **Subagent spawning unavailable**: take the degraded inline path (Step 3.3) — checkpoint still written, `/publish-pr` composed inline, degradation noted in the output. Never skip the checkpoint/PR split. +- **Subagent spawning unavailable**: take the degraded inline path (Step 3.3) — checkpoint still written, `/publish-pr` composed inline, degradation noted in the output. Never skip the checkpoint/PR split. The review dispatch degrades the same way: `pair-review` stays pending (merge blocked) and the re-run instruction is recorded, never an inline self-review. - **`/checkpoint` not installed**: skip the resume probe and the write step; resume from git+PM state (Idempotent Re-invocation) and synthesize the handoff inline for `/publish-pr`. - **`/publish-pr` not installed**: **HALT** — implement composes the gate/PR/board sequence, it never re-implements it. Report the missing dependency. - **`/assess-stack` not installed**: Warn on new dependency, continue without validation. diff --git a/packages/knowledge-hub/dataset/.skills/process/implement/post-review-merge.md b/packages/knowledge-hub/dataset/.skills/process/implement/post-review-merge.md index 286cbb2c..92873a04 100644 --- a/packages/knowledge-hub/dataset/.skills/process/implement/post-review-merge.md +++ b/packages/knowledge-hub/dataset/.skills/process/implement/post-review-merge.md @@ -2,11 +2,14 @@ Disclosed from [SKILL.md](SKILL.md) Phase 4 — only reached when `/implement` is re-invoked after code review approval (typically via `/review`), to merge and close the story. -### Step 4.1: Verify Review Approval +### Step 4.1: Merge Precondition — PR state must be `ready-to-merge` (BLOCKING) -1. **Check**: Is the PR approved by the reviewer? -2. **Skip**: If not approved → **HALT**. Wait for review completion. -3. **Verify**: PR has at least one approval. +This is the author-side merge path, and it carries the **same** precondition as the reviewer-side one (`/pair-process-review` Phase 6, Step 6.0): the merge is permitted by the **synthesis**, never by "a review happened". An approving verdict alone is not the condition — a 🔴 PR with an approved review and no explicit human approval must not merge here either (D10). See [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). + +1. **Act**: Re-read the **current** signals on the PR's head commit — gate checks, the review verdict, the `risk:*` tier, and non-author human approvals (they may have changed since the verdict: a new commit, a tier raise, a dismissed approval). +2. **Act**: Synthesize with the shipped evaluator [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh) — `resolve_pr_state `, then `merge_allowed `. Tier comes from the label via `resolve_tier`; the human-approval input uses `human_approval_jq_filter` (non-bot, non-author, on the current head), never a raw approval count. +3. **Skip**: Review not submitted yet → **HALT**, wait for review completion. +4. **Verify**: State is `ready-to-merge` → continue to Step 4.2. Any other state → **HALT**, naming the unmet condition (red gate, review not approved / still pending, or 🔴 without an explicit human approval). Never bypass, dismiss, or re-run a required check to get a green merge button. ### Step 4.2: Prepare Merge Commit Message diff --git a/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md b/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md index 811d0ff7..ea0e7da5 100644 --- a/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md +++ b/packages/knowledge-hub/dataset/.skills/process/review/SKILL.md @@ -1,7 +1,7 @@ --- name: review -description: "Reviews a pull request through 6 sequential phases (5 review + optional merge with parent cascade) — validation, technical review, adoption compliance, completeness, decision — to decide whether it merges. Not a quick build/test sanity check (use /verify-quality). Composes /classify, /verify-quality, /verify-done, /record-decision, /analyze-debt, /assess-security (required), /verify-adoption, /assess-stack (optional)." -version: 0.8.0 +description: "Reviews a pull request through 6 sequential phases (5 review + optional merge with parent cascade) — validation, technical review, adoption compliance, completeness, decision — to decide whether it merges. Gate before judgment: a red mechanical gate caps the verdict, and the decision is published as the required `pair-review` check plus the synthesized PR state (to-be-reviewed / ready-to-merge / not-approved), so merge stays blocked until gates are green, the review is approved, and (at risk:red) a human approves explicitly. Not a quick build/test sanity check (use /verify-quality). Composes /classify, /verify-quality, /verify-done, /record-decision, /analyze-debt, /assess-security (required), /verify-adoption, /assess-stack (optional)." +version: 0.5.0 author: Foomakers --- @@ -29,10 +29,21 @@ Review a pull request through 6 sequential phases (5 review + 1 optional merge). ## Arguments -| Argument | Required | Description | -| -------- | -------- | -------------------------------------------------------------------------------- | -| `$pr` | Yes | PR number or URL to review | -| `$story` | No | Story ID for requirements validation. If omitted, extracted from PR description. | +| Argument | Required | Description | +| ------------ | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `$pr` | Yes | PR number or URL to review | +| `$story` | No | Story ID for requirements validation. If omitted, extracted from PR description. | +| `$dispatched` | No | `true` when this run was **dispatched** (spawned by `/publish-pr`'s caller, an automation loop, a CI job) rather than invoked by a human in an interactive session. Default `false`. Sets the **non-interactive contract** below. Never merges (Phase 6 is unreachable). | + +### The non-interactive (dispatched) contract + +Two steps of this flow ask a human a question: Step 1.4 ("Proceed with review?") and Step 5.5 ("Merge now or let the author merge?"). In a **dispatched** run there is nobody to answer, so guessing is not an option in either direction — stalling wastes the run, and self-answering Step 5.5 would let the reviewing agent merge its own APPROVED verdict. With `$dispatched: true` (or whenever the run has no interactive human — e.g. the invocation prompt is a bare `/review $pr=` from another agent): + +- **Step 1.4** — do **not** ask. Emit the same READY block as output and continue directly to Step 1.5. +- **Step 5.5** — do **not** ask, and never self-answer "Merge now". The outcome is always option 2 (**author/human merges**): produce the report and stop after Phase 5. +- **A dispatched run never reaches Phase 6**: it does not merge, does not cascade, does not delete a branch, even on APPROVED with `merge_allowed` true. The human merge gate is the point of the flow ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md): "a human still performs the merge; pair never auto-merges"). The dispatch instruction says so explicitly, and this contract holds even if it does not. + +Everything else (phases 1–5, the verdict, the `pair-review` publication, the state synthesis) is identical to an interactive run. ## Session State @@ -94,7 +105,7 @@ REVIEW READY: └── AC: [N criteria to validate] ``` -Ask: _"Proceed with review?"_ +Ask: _"Proceed with review?"_ — **only in an interactive run**. A **dispatched** run (`$dispatched: true`, or no human in the loop) emits this block and continues to Step 1.5 without asking; it never stalls waiting for an answer that cannot come (see the non-interactive contract above). ### Step 1.5: Classification (risk matrix from the diff) @@ -107,10 +118,12 @@ Ask: _"Proceed with review?"_ ### Step 2.1: Quality Gates +**The gate is the first filter** (R5.4, [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): mechanical checks run before judgment, and a red gate **caps** the outcome of this review. + 1. **Check**: Has `/verify-quality` already run on the current PR head commit? 2. **Skip**: If all gates passing on current commit — record results, move to Step 2.2. 3. **Act**: Compose `/verify-quality` with `$scope = all`. -4. **Verify**: Record quality gate results. Failures become review findings (do not HALT — /review reports them). +4. **Verify**: Record quality gate results. Failures become review findings (do not HALT — /review reports them) **and cap the decision**: with any gate red the review can **never** reach APPROVED / TECH-DEBT and the Step 5.4 synthesis can **never** yield `ready-to-merge` — the judgment review does not override a mechanical failure. Continue the review (the findings are still useful to the author) but carry `Gates: red` into Step 5.2. ### Step 2.2: Code Quality Assessment @@ -223,7 +236,7 @@ Based on compiled findings: | Decision | Condition | | --------------------- | ----------------------------------------------------------------------------------------- | -| **APPROVED** | No critical or major issues. All AC met. Quality gates pass. | +| **APPROVED** | No critical or major issues. All AC met. Quality gates pass (a red gate caps the decision — Step 2.1). | | **CHANGES-REQUESTED** | Critical issues found, missing ADRs, any **introduced** red security finding from `/assess-security` (AC4), failing tests, AC not met. A **red** `cost:*` class does not itself block — it surfaces a **blocking human sign-off** requirement in the Verdict (the human, not the skill, gates on cost). | | **TECH-DEBT** | Only minor issues or debt items. Approve current PR, track debt separately. | @@ -242,21 +255,35 @@ The review is submitted on the **code host only** (where it gates the merge). It 2. **Act**: On re-review, submit a **fresh** native review — both documented paths append (MCP `create`; `gh pr review` CLI), neither edits a submitted body. GitHub's latest-review-governs semantics mean the newest review carries the verdict while earlier reviews stay as visible history, so re-invocation is safe without editing in place (idempotency). 3. **Verify**: The native review is submitted with the verdict-first body — no separate review-comment artifact exists. -### Step 5.4: Determine Next Action +### Step 5.4: Publish the `pair-review` Check & Synthesize the PR State (AC3, AC4, AC5) + +The verdict is judgment; the **merge block is mechanical**. This step turns the verdict into the required check and the PR state — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) for the model and [github-implementation.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md) § PR state flow for the host commands. Nothing here re-derives criteria: the tier comes from the PR's `risk:*` label via `resolve_tier` and the per-tier requirements come from [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 (**no classification criteria in this flow — D18**). + +1. **Check**: Does the current head commit already carry a `pair-review` check whose conclusion matches this verdict, and a matching `pr-state:*` label? +2. **Skip**: If yes — nothing to publish, move to Step 5.5 (idempotent re-invocation). +3. **Act — publish the check**: source the shipped [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh) and map the verdict with `review_check_conclusion` (`approved`/`tech-debt` ⇒ `success`, `changes-requested` ⇒ `failure`, anything else ⇒ `pending`). Publish `pair-review` on the **head commit** with that conclusion, through the mechanism the host guide prescribes for an ordinary agent token — on GitHub a **commit status** (the Checks API is writable only by a GitHub App). A `pending` result is never published as resolved — the required check stays unsatisfied so the merge stays blocked. If publication is **refused** (missing scope, no status API), report `pair-review: NOT PUBLISHED — advisory` and continue: the verdict still lives in the native review, but never claim a merge is blocked when it is not. +4. **Act — resolve the requirements for the tier**: read the tier from the PR labels (`resolve_tier`, tags only, **untagged/malformed ⇒ 🔴 fail-safe**), then read that tier's row from quality-model §4 through its `Argument > Adoption > KB default` cascade: reviewer count, SLA, **checklist depth** — `standard` vs `extended` as [quality-model](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 defines it (there is no separate extended-checklist artifact: `extended` is the same code-review template with **no section skipped**, "not applicable" written out rather than omitted) — and whether **explicit approval** is required. Record them in the review output; never invent or hardcode a threshold here. At 🔴, state the explicit-approval requirement in the Verdict block so the human reading the PR knows what is still missing (D10). +5. **Act — synthesize the state**: `resolve_pr_state ` — `gates` from Step 2.1, `explicit-approval` = a **non-author human** approval recorded on the current head (the pair review itself never counts; on a single-maintainer repo 🔴 therefore needs a second human account — see pr-states.md). Apply the resulting `pr-state:to-be-reviewed` / `pr-state:ready-to-merge` / `pr-state:not-approved` label, removing any other `pr-state:*` label (exactly one at a time); the labels are provisioned once per repository and if absent this step is **non-blocking** (degradation below). A red gate or a 🔴 tier without explicit approval yields `to-be-reviewed`, never `ready-to-merge` (AC2, AC4). +6. **Verify**: The head commit carries the `pair-review` check (or the publication failure is reported), the PR carries exactly one `pr-state:*` label matching the synthesis (or its absence is reported), and the tier requirements are recorded in the output. The label is a **view** — enforcement is the required checks (R5.7); this skill never edits branch protection and never bypasses a check. + +### Step 5.5: Determine Next Action 1. **Check**: What was the review decision? 2. **Skip**: If CHANGES-REQUESTED → output review report and stop. Author addresses findings, then re-invokes `/review`. -3. **Act**: If APPROVED or TECH-DEBT → ask reviewer: +3. **Act — dispatched run**: if this run is **dispatched** (non-interactive — see the contract in Arguments), do **not** ask and do **not** self-answer: the outcome is option 2 below (the human merges). Output the report and stop. A self-answered "Merge now" here would let the reviewing agent merge on its own verdict, which is exactly what the human merge gate forbids. +4. **Act — interactive run**: If APPROVED or TECH-DEBT → ask reviewer: > PR approved. Merge now or let the author merge? > 1. **Merge now** — proceed to Phase 6 > 2. **Author merges** — stop here, author re-invokes `/implement` Phase 4 -4. **Verify**: If "Merge now" selected → proceed to Phase 6. Otherwise → output and stop. + Either route enforces the **same** precondition before merging (`merge_allowed` on the re-synthesized state — Step 6.0 here, Step 4.1 there), so routing to the author never skips the 🔴 explicit-approval requirement. + +5. **Verify**: If a human explicitly selected "Merge now" → proceed to Phase 6. Otherwise (including every dispatched run) → output and stop. ## Phase 6: Merge & Close (APPROVED only, optional) -Only reached when the reviewer picked "Merge now" in Step 5.4 — see [merge-and-cascade.md](merge-and-cascade.md) for the merge-strategy, merge-commit, merge, parent-cascade, branch-cleanup, and post-merge-manual-test steps (Steps 6.1–6.6) plus the completion output. +Only reached when a **human reviewer** picked "Merge now" in Step 5.5 of an **interactive** run — a dispatched run stops at Phase 5 by contract (Arguments § non-interactive) — see [merge-and-cascade.md](merge-and-cascade.md) for the merge-precondition (Step 6.0: `merge_allowed` on the synthesized PR state — HALT unless `ready-to-merge`), merge-strategy, merge-commit, merge, parent-cascade, branch-cleanup, and post-merge-manual-test steps (Steps 6.1–6.6) plus the completion output. ## Output Format @@ -275,7 +302,10 @@ REVIEW COMPLETE: ├── DoD: [N/N criteria met] ├── Adoption: [Level N — summary] ├── Debt: [N items flagged] -└── Review: [Submitted as native review body — no separate comment] +├── Review: [Submitted as native review body — no separate comment] +├── Check: [pair-review → success | failure | pending (blocks merge)] +├── Tier req.: [🟢/🟡/🔴 — N reviewer(s) / SLA / standard|extended checklist / explicit approval: required-and-present | required-and-MISSING | n-a] +└── PR state: [pr-state:to-be-reviewed | pr-state:ready-to-merge | pr-state:not-approved] ``` At merge (Phase 6): see [merge-and-cascade.md](merge-and-cascade.md). @@ -287,6 +317,7 @@ Review stops immediately when: - **PR not found or not open** (Phase 1) - **Missing ADR for new technical decision** (Phase 2, Step 2.3) — compose `/record-decision`, then resume - **Unresolved architectural non-conformity** (Phase 3) — must be addressed before decision +- **PR state is not `ready-to-merge`** (Phase 6) — `merge_allowed` fails: gates red, review not approved, or a 🔴 PR without explicit human approval. Report which condition is unmet and stop; never bypass a required check. On HALT: report the blocker, compose the resolution skill if available, wait for developer. @@ -299,7 +330,8 @@ See [idempotency convention](../../../.pair/knowledge/guidelines/technical-stand 3. **Skill compositions**: /verify-quality, /verify-done, /assess-security results cached in session. Not re-run if already passing/current on current commit. 4. **New commits**: if PR updated since last check, re-validates affected phases only. 5. **Review report**: re-review appends a fresh native review (MCP `create` / `gh pr review`); GitHub's latest-review-governs semantics make the newest one carry the verdict while prior reviews remain as history. The report is always the review body, never a separate comment — so no duplicate comment artifact is created. -6. **Merge**: detects already-merged PR. Skips Phase 6 if already merged. Resumes parent cascade if merge succeeded but status updates are incomplete. +6. **PR state & check** (Step 5.4): re-publishing is a no-op when the head commit already carries a `pair-review` check matching the verdict and the `pr-state:*` label already matches the synthesis. A **new head commit** (including a force-push) has no check of its own, so the review re-runs on it and the merge stays blocked meanwhile. A tier raised between runs re-synthesizes on the new tier — raise-only, so a re-run never loosens a requirement. +7. **Merge**: detects already-merged PR. Skips Phase 6 if already merged. Resumes parent cascade if merge succeeded but status updates are incomplete. ## Graceful Degradation @@ -317,12 +349,18 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - **PM tool not accessible**: the PR-side work (review, merge) still runs on the code host; the PM-side writes (issue close, parent cascade) are reported as not done rather than guessed. In a single-tool project this is the same tool, so the merge falls back to CLI only. - **Code host declared but unreachable/unauthenticated**: **HALT** with a setup pointer before any review is submitted — there is nothing to review against. PM-side reads already done are not rolled back. - **Merge fails** (conflicts, branch protection): Report the failure, ask reviewer to resolve. Do not force-push or bypass protections. +- **Code host has no check-run/required-check API**: publish the `pr-state:*` label only, report `enforcement: advisory — host manual setup required` (see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md) degraded mode), and do NOT claim the merge is blocked. Does NOT HALT. +- **`pair-review` publication refused** (Step 5.4 — token lacks the status scope, host rejects the write): report `pair-review: NOT PUBLISHED — advisory` with the host error; the verdict stays in the native review and the state is still synthesized. Enforcement is advisory until publication works — never asserted otherwise. Does NOT HALT. +- **`pr-state:*` label absent / no label API** (Step 5.4): report `pr-state label: not applied` — **non-blocking**, the required checks remain the authority. Does NOT HALT and does not block Phase 6's `merge_allowed` evaluation, which reads the synthesized state, not the label. +- **🔴 PR on a single-maintainer repository**: the non-author human approval is unobtainable (GitHub rejects a self-approval), so the state stays `to-be-reviewed` and Phase 6 HALTs. Report it as a repository constraint — a second human account is required, or `pair-explicit-approval` must deliberately not be a required context there (pr-states.md edge cases). Never self-approve to unblock. - **/execute-manual-tests not installed**: Skip Step 6.6. Log "Manual test validation skipped — skill not installed." Does NOT block merge. - **No manual test suite**: Skip Step 6.6. Log "No manual test suite found." Does NOT block merge. ## Notes -- This skill **reads code, submits the native review (verdict = the review action), and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). +- This skill **reads code, submits the native review (verdict = the review action), publishes the required `pair-review` check + the `pr-state:*` label, and optionally merges PRs** — it does not modify source code and posts no separate review comment (AC2). +- **Gate ≠ review** ([pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md)): the mechanical gate is the first filter (Step 2.1) and the judgment verdict never overrides it; the merge block itself is the required check, not this skill's opinion. The `pr-state:*` label is a view — enforcement lives in branch protection (R5.7), which this skill never edits or bypasses. +- **Per-tier requirements are read, never invented** — reviewer count, SLA, checklist depth, and the 🔴 explicit-approval requirement come from [quality-model.md](../../../.pair/knowledge/guidelines/quality-assurance/quality-model.md) §4 through its `Argument > Adoption > KB default` cascade (D10). The tier comes from the PR's `risk:*` label (`resolve_tier`, tags only, untagged ⇒ 🔴 fail-safe): **this flow contains no classification criteria** (D18). - Review phases are sequential — each phase builds on findings from prior phases. - The reviewer can stop between phases; re-invoke to resume (see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md)). - Output follows [code-review-template.md](../../../.pair/knowledge/guidelines/collaboration/templates/code-review-template.md) — the template defines structure, /review fills it with findings. diff --git a/packages/knowledge-hub/dataset/.skills/process/review/merge-and-cascade.md b/packages/knowledge-hub/dataset/.skills/process/review/merge-and-cascade.md index 4c890b87..03c86e12 100644 --- a/packages/knowledge-hub/dataset/.skills/process/review/merge-and-cascade.md +++ b/packages/knowledge-hub/dataset/.skills/process/review/merge-and-cascade.md @@ -1,6 +1,14 @@ # Phase 6: Merge & Close — Detail -Disclosed from [SKILL.md](SKILL.md) Phase 6 — only reached when the reviewer picked "Merge now" in Step 5.4. +Disclosed from [SKILL.md](SKILL.md) Phase 6 — only reached when the reviewer picked "Merge now" in Step 5.5, and only executed when the PR state synthesis says `ready-to-merge` (Step 6.0). + +### Step 6.0: Merge Precondition — PR state must be `ready-to-merge` (BLOCKING) + +The merge is permitted by the **synthesis**, not by the reviewer's enthusiasm — see [pr-states.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md). + +1. **Act**: Re-read the current signals (gates, verdict, `risk:*` tier, human approvals on the current head) and re-run the Step 5.4 synthesis — signals may have changed since the verdict was submitted (a new commit, a tier raise, a dismissed approval). +2. **Check**: `merge_allowed ` (from [`pr-state.sh`](../../../.pair/knowledge/assets/pr-state.sh)). +3. **Verify**: State is `ready-to-merge` → continue to Step 6.1. Any other state → **HALT**, reporting the unmet condition (red gate, review not approved, or 🔴 without explicit human approval). Never bypass, dismiss, or re-run a required check to get a green merge button. ### Step 6.1: Read Merge Strategy @@ -80,6 +88,7 @@ The merge happens on the **code host**; the item writes in Step 6.4 happen on th STORY DONE: ├── Story: [#ID: Title] ├── PR: [#PR-number — merged] +├── PR state: [ready-to-merge — synthesis verified at Step 6.0] ├── Merge: [squash | merge | rebase] ├── Story: Done ├── Epic: [#ID — Done | In Progress (X/Y stories done)] diff --git a/packages/knowledge-hub/src/conformance/pr-state-flow.test.ts b/packages/knowledge-hub/src/conformance/pr-state-flow.test.ts new file mode 100644 index 00000000..9e3c2260 --- /dev/null +++ b/packages/knowledge-hub/src/conformance/pr-state-flow.test.ts @@ -0,0 +1,620 @@ +import { describe, it, expect } from 'vitest' +import { readFileSync, existsSync } from 'fs' +import { dirname, join, resolve } from 'path' + +// Story #234 — PR state flow (gate ≠ review) + pair review as a required check. +// +// The flow is: mechanical gates first, judgment review only at green gates, and the +// synthesis of (gates × review verdict × tier × explicit approval) IS the PR state +// (`to-be-reviewed` → `ready-to-merge` / `not-approved`). Merge is mechanically blocked +// by REQUIRED code-host checks, never by a label — so the review can never be skipped. +// +// These are content invariants on the sources of record (dataset KB + dataset skills), +// asserted the same way the rest of the KB/skill corpus is tested (see +// verify-quality-gate-matrix.test.ts, quality-model.test.ts). The *behavior* of the +// shipped synthesis evaluator (pr-state.sh) is executed end-to-end by +// scripts/smoke-tests/scenarios/pr-state-flow.sh per the gate-tooling ADL +// (2026-07-13: shell assets are smoke-tested, never vitest-unit-tested). + +const DATASET = join(__dirname, '../../dataset') +const GUIDELINE_PATH = join( + DATASET, + '.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md', +) +const GUIDELINE = readFileSync(GUIDELINE_PATH, 'utf-8') +const EVALUATOR_PATH = join(DATASET, '.pair/knowledge/assets/pr-state.sh') +const EVALUATOR = readFileSync(EVALUATOR_PATH, 'utf-8') +const GITHUB_GUIDE = readFileSync( + join( + DATASET, + '.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md', + ), + 'utf-8', +) +const PUBLISH_PR_PATH = join(DATASET, '.skills/capability/publish-pr/SKILL.md') +const PUBLISH_PR = readFileSync(PUBLISH_PR_PATH, 'utf-8') +const REVIEW_PATH = join(DATASET, '.skills/process/review/SKILL.md') +const REVIEW = readFileSync(REVIEW_PATH, 'utf-8') +const MERGE_CASCADE = readFileSync( + join(DATASET, '.skills/process/review/merge-and-cascade.md'), + 'utf-8', +) +const SETUP_GATES_PATH = join(DATASET, '.skills/capability/setup-gates/SKILL.md') +const SETUP_GATES = readFileSync(SETUP_GATES_PATH, 'utf-8') +const QUALITY_MODEL = readFileSync( + join(DATASET, '.pair/knowledge/guidelines/quality-assurance/quality-model.md'), + 'utf-8', +) +const CANONICAL_STATES = readFileSync( + join( + DATASET, + '.pair/knowledge/guidelines/collaboration/project-management-tool/canonical-states.md', + ), + 'utf-8', +) +const HOW_TO_CODE_REVIEW = readFileSync( + join(DATASET, '.pair/knowledge/how-to/11-how-to-code-review.md'), + 'utf-8', +) +const WOW_TEMPLATE = readFileSync(join(DATASET, '.pair/adoption/tech/way-of-working.md'), 'utf-8') +const IMPLEMENT_PATH = join(DATASET, '.skills/process/implement/SKILL.md') +const IMPLEMENT = readFileSync(IMPLEMENT_PATH, 'utf-8') + +/** Every relative markdown link in `content` resolves to a file on disk. */ +const assertLinksResolve = (content: string, fromPath: string, targets: string[]): void => { + const dir = dirname(fromPath) + for (const t of targets) { + const m = content.match(new RegExp(`\\]\\(([^)]*${t.replace(/\./g, '\\.')})\\)`)) + expect(m, `${fromPath} must link to ${t}`).not.toBeNull() + const linkPath = (m as RegExpMatchArray)[1] as string + expect( + existsSync(resolve(dir, linkPath.split('#')[0] as string)), + `link to ${t} must resolve`, + ).toBe(true) + } +} + +describe('pr-states.md — the PR state flow model (#234)', () => { + it('names exactly the three PR states of the flow', () => { + expect(GUIDELINE).toContain('to-be-reviewed') + expect(GUIDELINE).toContain('ready-to-merge') + expect(GUIDELINE).toContain('not-approved') + }) + + it('states gate ≠ review: gates are mechanical/zero-judgment, review is judgment/zero-checks', () => { + expect(GUIDELINE).toMatch(/gate\s*≠\s*review|gate is not (a )?review/i) + expect(GUIDELINE).toMatch(/zero judgment/i) + expect(GUIDELINE).toMatch(/zero mechanical/i) + }) + + it('AC2 — the gate is the FIRST filter: a red gate never yields a merge-enabling state (R5.4)', () => { + expect(GUIDELINE).toMatch(/first filter/i) + expect(GUIDELINE).toMatch(/R5\.4/) + expect(GUIDELINE).toMatch( + /(red|failing) gate[\s\S]{0,200}never[\s\S]{0,80}(merge-enabling|ready-to-merge)/i, + ) + }) + + it('AC3 — synthesis: green gates AND approved review ⇒ ready-to-merge; not-approved routes to a human', () => { + expect(GUIDELINE).toMatch(/## Synthesis/i) + expect(GUIDELINE).toMatch( + /green gates[\s\S]{0,160}approved review[\s\S]{0,200}ready-to-merge|ready-to-merge[\s\S]{0,200}approved/i, + ) + expect(GUIDELINE).toMatch(/not-approved[\s\S]{0,160}human/i) + }) + + it('AC4 — 🔴 red additionally requires explicit human approval before merge (D10)', () => { + expect(GUIDELINE).toMatch(/explicit approval/i) + expect(GUIDELINE).toMatch(/D10/) + expect(GUIDELINE).toMatch(/risk:red/) + }) + + it('AC5 — the pair review is a REQUIRED check; missing/failed ⇒ code host blocks the merge (R5.7)', () => { + expect(GUIDELINE).toContain('pair-review') + expect(GUIDELINE).toMatch(/required (status )?check/i) + expect(GUIDELINE).toMatch(/R5\.7/) + expect(GUIDELINE).toMatch(/(missing|absent)[\s\S]{0,200}block/i) + }) + + it('labels are a VIEW; the required checks are the authority (a stale label cannot enable a merge)', () => { + expect(GUIDELINE).toMatch(/pr-state:/) + expect(GUIDELINE).toMatch( + /label[\s\S]{0,300}(never|cannot|not)[\s\S]{0,120}(enable|authoritative|authority)/i, + ) + }) + + it('fail-safe: an untagged / malformed tier is treated as 🔴 (consistent with #258)', () => { + expect(GUIDELINE).toMatch(/fail-safe/i) + expect(GUIDELINE).toMatch(/untagged[\s\S]{0,160}red/i) + }) + + it('reads TAGS only — contains no classification criteria (D18)', () => { + expect(GUIDELINE).toMatch(/no classification criteria|contains no[^\n]*criteria/i) + expect(GUIDELINE).toMatch(/D18/) + }) + + it('covers the four edge cases: tier raise, review crash/timeout, force-push, host without required-check API', () => { + expect(GUIDELINE).toMatch(/rais(e|ed)[\s\S]{0,200}re-?block|re-?block[\s\S]{0,200}rais/i) + expect(GUIDELINE).toMatch(/(crash|timeout)[\s\S]{0,240}(pending|blocked)/i) + expect(GUIDELINE).toMatch(/force-push[\s\S]{0,240}(invalidat|re-?run)/i) + expect(GUIDELINE).toMatch(/degraded[\s\S]{0,400}manual/i) + }) + + it('defers per-tier requirements to quality-model §4 (single source) instead of restating thresholds', () => { + assertLinksResolve(GUIDELINE, GUIDELINE_PATH, [ + 'quality-model.md', + 'pr-state.sh', + 'tier-resolve.sh', + ]) + }) + + // --- review round 1 on PR #390 --- + + it('states plainly that a 🔴 merge needs a SECOND human account (the author cannot approve)', () => { + expect(GUIDELINE).toMatch(/second human/i) + expect(GUIDELINE).not.toMatch(/what a solo maintainer satisfies deliberately/i) + expect(GUIDELINE).toMatch(/solo[\s\S]{0,400}(cannot|impossible|no 🔴)/i) + }) + + it('maps 🟡’s "reviewer approval" onto the pair review explicitly (no two readings)', () => { + expect(GUIDELINE).toMatch(/pair review[\s\S]{0,120}satisfies[\s\S]{0,80}reviewer approval/i) + }) + + it('says the post-force-push review re-run is skill/human triggered, not automatic', () => { + expect(GUIDELINE).toMatch(/re-?run is[\s\S]{0,120}(triggered|manual)/i) + }) + + it('carries a non-blocking degradation for a missing pr-state label family', () => { + expect(GUIDELINE).toMatch(/label[\s\S]{0,240}(non-blocking|not applied)/i) + }) +}) + +describe('pr-state.sh — the deterministic synthesis evaluator (#234)', () => { + it('exposes the five entry points the flow composes', () => { + for (const fn of [ + 'resolve_pr_state', + 'merge_allowed', + 'explicit_approval_required', + 'review_check_conclusion', + 'human_approval_jq_filter', + ]) { + expect(EVALUATOR).toContain(fn) + } + }) + + it('is provider-agnostic and reads state signals + tags only — no classification criteria (D18)', () => { + expect(EVALUATOR).toMatch(/D18/) + expect(EVALUATOR).not.toMatch(/git diff|--numstat|files?-changed|\bmigration\b/i) + }) + + it('documents the fail-safe defaults (unknown tier ⇒ red, non-pass gates ⇒ not merge-enabling)', () => { + expect(EVALUATOR).toMatch(/fail-safe/i) + }) +}) + +describe('publish-pr — review at PR creation (AC1)', () => { + it('registers the pair-review check and enters to-be-reviewed at creation', () => { + expect(PUBLISH_PR).toContain('pair-review') + expect(PUBLISH_PR).toContain('to-be-reviewed') + expect(PUBLISH_PR).toMatch(/pending/i) + }) + + it('runs the review through a clean-context (handoff-only, anonymous) subagent', () => { + expect(PUBLISH_PR).toMatch(/subagent/i) + expect(PUBLISH_PR).toMatch(/clean context|anonymous/i) + expect(PUBLISH_PR).toMatch(/\/review|process-review/) + }) + + it('never merges and never approves its own PR — the review verdict is the reviewer flow’s', () => { + expect(PUBLISH_PR).toMatch(/never merges/i) + }) + + it('degrades: no subagent ⇒ documented fallback; the check still blocks the merge meanwhile', () => { + expect(PUBLISH_PR).toMatch(/subagent[\s\S]{0,300}(unavailable|not available)/i) + }) + + it('links the state-flow model instead of restating it', () => { + assertLinksResolve(PUBLISH_PR, PUBLISH_PR_PATH, ['pr-states.md']) + }) + + // --- review round 1 on PR #390 --- + + it('degrades when the status publication is refused (advisory, reported — never assumed)', () => { + expect(PUBLISH_PR).toMatch(/(status|publication)[\s\S]{0,200}refused[\s\S]{0,300}advisory/i) + }) + + it('degrades non-blocking when the pr-state:* label family is absent', () => { + expect(PUBLISH_PR).toMatch( + /pr-state[^\n]*label[\s\S]{0,240}(absent|missing|not found)[\s\S]{0,300}non-blocking/i, + ) + }) +}) + +describe('review — gate-first, synthesis, and the merge precondition (AC2, AC3, AC4)', () => { + it('AC2 — a red gate caps the decision: no merge-enabling verdict while gates are red', () => { + expect(REVIEW).toMatch(/first filter|gate[\s\S]{0,80}before[\s\S]{0,40}review/i) + expect(REVIEW).toMatch( + /(red|failing) gate[\s\S]{0,300}(never|cannot)[\s\S]{0,120}(APPROVED|merge-enabling|ready-to-merge)/i, + ) + }) + + it('AC3/AC4 — computes the PR state via the shipped evaluator and applies the pr-state label', () => { + expect(REVIEW).toContain('pr-state.sh') + expect(REVIEW).toContain('resolve_pr_state') + expect(REVIEW).toContain('ready-to-merge') + expect(REVIEW).toContain('not-approved') + }) + + it('AC5 — publishes the pair-review check conclusion from the verdict', () => { + expect(REVIEW).toContain('pair-review') + expect(REVIEW).toContain('review_check_conclusion') + }) + + it('AC4 — 🔴 red: reviewer/SLA/extended-checklist/explicit-approval requirements are read, never invented', () => { + expect(REVIEW).toMatch(/extended checklist/i) + expect(REVIEW).toMatch(/explicit approval/i) + expect(REVIEW).toMatch(/quality-model/) + }) + + it('merge is gated on ready-to-merge — Phase 6 HALTs on any other state', () => { + expect(MERGE_CASCADE).toContain('ready-to-merge') + expect(MERGE_CASCADE).toMatch(/HALT/) + expect(MERGE_CASCADE).toContain('merge_allowed') + }) + + it('links the state-flow model instead of restating it', () => { + assertLinksResolve(REVIEW, REVIEW_PATH, ['pr-states.md']) + }) + + // --- review round 1 on PR #390 --- + + it('publishes pair-review as a COMMIT STATUS (the check-runs API is GitHub-App only)', () => { + expect(REVIEW).toMatch(/commit status/i) + expect(REVIEW).toMatch(/(status|publication)[\s\S]{0,200}refused[\s\S]{0,300}advisory/i) + }) + + it('degrades non-blocking when the pr-state:* label family is absent', () => { + expect(REVIEW).toMatch( + /pr-state[^\n]*label[\s\S]{0,240}(absent|missing|not found)[\s\S]{0,300}non-blocking/i, + ) + }) +}) + +describe('setup-gates — pair review + explicit approval as required checks (AC5)', () => { + it('wires the pair-review and explicit-approval required checks on the protected branch', () => { + expect(SETUP_GATES).toContain('pair-review') + expect(SETUP_GATES).toContain('pair-explicit-approval') + expect(SETUP_GATES).toMatch(/branch protection/i) + }) + + it('is host-agnostic: host mechanics are deferred to the implementation guide (R2.12)', () => { + assertLinksResolve(SETUP_GATES, SETUP_GATES_PATH, ['pr-states.md']) + }) + + it('documents the degraded mode when the host has no required-check API', () => { + expect(SETUP_GATES).toMatch(/degraded[\s\S]{0,300}(manual|required-check)/i) + }) +}) + +describe('github-implementation.md — host mechanics for the required checks (R2.12)', () => { + it('carries the concrete check-run + branch-protection recipe', () => { + expect(GITHUB_GUIDE).toContain('pair-review') + expect(GITHUB_GUIDE).toContain('pair-explicit-approval') + expect(GITHUB_GUIDE).toMatch(/required_status_checks/) + expect(GITHUB_GUIDE).toMatch(/dismiss_stale_reviews/) + }) + + it('the 🔴 explicit-approval check demands a HUMAN approval, not the pair review itself', () => { + expect(GITHUB_GUIDE).toMatch(/human[\s\S]{0,200}approv/i) + }) + + // --- review round 1 on PR #390: every host command must be runnable with the + // token the skills actually hold. Verified against the live API. --- + + it('publishes pair-review through the commit-statuses API, never POST /check-runs (403, App-only)', () => { + expect(GITHUB_GUIDE).toMatch(/statuses\/\$HEAD_SHA/) + expect(GITHUB_GUIDE).toMatch(/context=('|")?pair-review/) + // the prose may NAME the Checks API to explain why it is unusable; no command may call it + expect(GITHUB_GUIDE).not.toMatch(/repos\/\$OWNER\/\$REPO\/check-runs/) + expect(GITHUB_GUIDE).toMatch(/GitHub App/) + expect(GITHUB_GUIDE).toMatch(/repo:status/) + }) + + it('guards the pending verdict in the copy-pasteable snippet (pending is not a publishable state)', () => { + expect(GITHUB_GUIDE).toMatch(/=\s*pending\s*\][\s\S]{0,160}exit 0/) + }) + + it('queries approvals via the REST reviews endpoint — `author.is_bot` does not exist', () => { + expect(GITHUB_GUIDE).not.toMatch(/is_bot/) + expect(GITHUB_GUIDE).toMatch(/pulls\/\$PR\/reviews/) + // the predicate itself lives in the shipped evaluator (round 3), not inline here + expect(GITHUB_GUIDE).toMatch(/human_approval_jq_filter/) + expect(EVALUATOR).toMatch(/user\.type\s*==\s*"User"/) + expect(EVALUATOR).toMatch(/commit_id\s*==\s*env\.HEAD_SHA/) + }) + + it('provisions the pr-state:* label family (labels never autocreate)', () => { + for (const state of ['to-be-reviewed', 'ready-to-merge', 'not-approved']) { + expect(GITHUB_GUIDE).toMatch(new RegExp(`gh label create ["']?pr-state:${state}`)) + } + }) + + it('states required_approving_review_count explicitly and justifies the strict choice', () => { + expect(GITHUB_GUIDE).toMatch(/"required_approving_review_count": 0/) + expect(GITHUB_GUIDE).toMatch(/"strict": false/) + expect(GITHUB_GUIDE).toMatch(/strict[\s\S]{0,400}(per head|head commit|up to date)/i) + }) + + it('pins least-privilege permissions on the workflow template', () => { + expect(GITHUB_GUIDE).toMatch(/permissions:[\s\S]{0,200}pull-requests: read/) + }) + + it('records the ordering constraint (add job → observe the contexts → protect) and the enforce_admins trap', () => { + expect(GITHUB_GUIDE).toMatch(/## Ordering|### Ordering/i) + expect(GITHUB_GUIDE).toMatch(/enforce_admins[\s\S]{0,400}(after|escape|bypass)/i) + expect(GITHUB_GUIDE).toMatch(/second human/i) + }) + + it('documents the three independent degradations: status publication, label API, branch protection', () => { + expect(GITHUB_GUIDE).toMatch(/NOT PUBLISHED/) + expect(GITHUB_GUIDE).toMatch(/label API/i) + expect(GITHUB_GUIDE).toMatch(/branch protection[\s\S]{0,300}advisory/i) + }) +}) + +describe('doc coherence & cross-links for the PR state flow (#234)', () => { + it('quality-model §4 points at pr-states.md for the red-gate refinement (no contradictory reading)', () => { + expect(QUALITY_MODEL).toMatch( + /red gate[\s\S]{0,200}(no merge-enabling|never[\s\S]{0,40}merge-enabling)/i, + ) + }) + + it('canonical-states.md back-links the PR state flow companion', () => { + expect(CANONICAL_STATES).toMatch(/pr-states\.md/) + }) + + it('how-to-11’s decision table points at the merge precondition (required checks, merge_allowed)', () => { + expect(HOW_TO_CODE_REVIEW).toMatch(/merge_allowed|pr-states\.md/) + }) + + it('the way-of-working TEMPLATE carries a status placeholder, not an applied-as-fact claim', () => { + expect(WOW_TEMPLATE).toMatch(/Status: \[applied \| not yet applied\]/) + }) +}) + +// --- review round 2 on PR #390 ------------------------------------------------- +// The authorization control must not be readable from the PR's own tree, the +// required contexts must land on the commit branch protection evaluates, and the +// review dispatch must be executable (no nested delegation) and non-mergeable. + +describe('pair-explicit-approval runs from a TRUSTED ref (round 2)', () => { + it('triggers on pull_request_target, never on plain pull_request (the PR cannot ship its own gate)', () => { + expect(GITHUB_GUIDE).toMatch(/^\s*pull_request_target:/m) + // a bare `pull_request:` trigger would run the PR's own workflow file + expect(GITHUB_GUIDE).not.toMatch(/^\s{2}pull_request:\s*$/m) + }) + + it('pins the checkout to the BASE sha, never to the PR head, and disables credentials', () => { + expect(GITHUB_GUIDE).toMatch(/ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/) + expect(GITHUB_GUIDE).not.toMatch( + /ref:\s*\$\{\{\s*github\.event\.pull_request\.head\.(sha|ref)\s*\}\}/, + ) + expect(GITHUB_GUIDE).toMatch(/persist-credentials:\s*false/) + }) + + it('explains the tampering threat the trusted ref defends against, and the fork caveat', () => { + expect(GITHUB_GUIDE).toMatch(/tamper/i) + expect(GITHUB_GUIDE).toMatch(/(self-grant|self-approve|bypass)/i) + expect(GITHUB_GUIDE).toMatch(/fork/i) + // the inline alternative for projects that refuse pull_request_target + expect(GITHUB_GUIDE).toMatch(/inline/i) + }) + + it('publishes pair-explicit-approval as a commit status pinned to the PR HEAD sha', () => { + expect(GITHUB_GUIDE).toMatch(/statuses\/\$HEAD_SHA[\s\S]{0,400}pair-explicit-approval/) + expect(GITHUB_GUIDE).toMatch(/statuses:\s*write/) + }) + + it('states WHY the status is posted explicitly: the run’s own SHA is not the PR head', () => { + expect(GITHUB_GUIDE).toMatch( + /(GITHUB_SHA|the run'?s? (own )?(head )?sha)[\s\S]{0,320}(base|not the (PR )?head)/i, + ) + expect(GITHUB_GUIDE).toMatch(/pull_request_review[\s\S]{0,400}head (commit|sha)/i) + }) +}) + +describe('copy-pasteable host recipe is actually runnable (round 2)', () => { + it('defines the repository variables once and uses ONE form (repos/$REPO/...)', () => { + expect(GITHUB_GUIDE).toMatch(/REPO="\$\(gh repo view --json nameWithOwner/) + // no mixed `repos/$OWNER/$REPO/` form in the new section + expect(GITHUB_GUIDE).not.toMatch(/repos\/\$OWNER\/\$REPO/) + }) + + it('ships enforce_admins FALSE in the payload, pointing at the step that flips it', () => { + expect(GITHUB_GUIDE).toMatch(/"enforce_admins": false/) + expect(GITHUB_GUIDE).not.toMatch(/"enforce_admins": true/) + }) + + it('paginates the reviews query — an approval on page 2 must not read as zero', () => { + expect(GITHUB_GUIDE).toMatch(/--paginate[^\n]*pulls\/\$PR\/reviews/) + expect(GITHUB_GUIDE).not.toMatch(/reviews\?per_page=100/) + }) + + it('ordering step 2 covers the approval-time re-run, not only PR open', () => { + expect(GITHUB_GUIDE).toMatch(/### Ordering/) + expect(GITHUB_GUIDE).toMatch(/(approv|review submission)[\s\S]{0,240}head (commit|sha)/i) + }) +}) + +describe('pr-states.md does not restate tier thresholds (round 2)', () => { + it('links quality-model §4 instead of copying reviewer/SLA/checklist values', () => { + expect(GUIDELINE).not.toMatch(/1 working day/) + expect(GUIDELINE).not.toMatch(/2 working days/) + expect(GUIDELINE).not.toMatch(/extended checklist/i) + expect(GUIDELINE).toMatch(/quality-model\.md/) + }) +}) + +describe('review dispatch is executable and never merges (round 2)', () => { + it('publish-pr emits a review-dispatch-required signal instead of nesting a subagent', () => { + expect(PUBLISH_PR).toMatch(/review-dispatch-required/) + expect(PUBLISH_PR).toMatch(/nest(ed|ing)?[\s\S]{0,200}subagent|subagent[\s\S]{0,200}nest/i) + }) + + it('the dispatch prompt forbids the merge phase (phases 1–5 only)', () => { + expect(PUBLISH_PR).toMatch(/never merge|do not merge|phases 1–5 only/i) + expect(PUBLISH_PR).toMatch(/Phase 6/) + }) + + it('implement Step 3.3 is the actor that dispatches the review when the signal comes back', () => { + expect(IMPLEMENT).toMatch(/review-dispatch-required/) + expect(IMPLEMENT).toMatch(/\/review \$pr=/) + expect(IMPLEMENT).toMatch(/never merge|do not merge|phases 1–5 only/i) + }) + + it('review defines NON-INTERACTIVE behaviour for the two human prompts when dispatched', () => { + expect(REVIEW).toMatch(/dispatched/i) + // Step 1.4 confirmation is skipped rather than stalling + expect(REVIEW).toMatch(/Proceed with review\?[\s\S]{0,600}(dispatched|non-interactive)/i) + // Step 5.5 never self-answers "merge now" + expect(REVIEW).toMatch( + /(dispatched|non-interactive)[\s\S]{0,400}(never|does not)[\s\S]{0,80}Phase 6/i, + ) + }) +}) + +describe('"extended" checklist depth is defined, not decorative (round 2)', () => { + it('quality-model §4 defines what standard vs extended checklist depth means', () => { + expect(QUALITY_MODEL).toMatch(/checklist depth/i) + expect(QUALITY_MODEL).toMatch(/extended[\s\S]{0,400}(no section skipped|every section)/i) + expect(QUALITY_MODEL).toMatch(/code-review-template\.md/) + }) + + it('review Step 5.4 reads that definition instead of naming a non-existent artifact', () => { + expect(REVIEW).toMatch(/checklist depth[\s\S]{0,300}quality-model/i) + }) +}) + +// --- review round 3 on PR #390 ------------------------------------------------- +// A required status context is writable by any push-access principal, so the +// authorization context must pin its producer and fail closed when interrupted; and +// the docs must not claim more than the mechanism delivers. Both merge paths — not +// only the reviewer's — must carry the synthesis precondition. + +const POST_REVIEW_MERGE_PATH = join(DATASET, '.skills/process/implement/post-review-merge.md') +const POST_REVIEW_MERGE = readFileSync(POST_REVIEW_MERGE_PATH, 'utf-8') +const ADR_018 = readFileSync( + join(__dirname, '../../../../.pair/adoption/tech/adr/adr-018-pr-state-flow-required-checks.md'), + 'utf-8', +) +const DOCS_PAGE = readFileSync( + join(__dirname, '../../../../apps/website/content/docs/concepts/pr-state-flow.mdx'), + 'utf-8', +) +const ROOT_WOW = readFileSync( + join(__dirname, '../../../../.pair/adoption/tech/way-of-working.md'), + 'utf-8', +) + +describe('the authorization context pins its producer (round 3)', () => { + it('uses the checks form with an app_id, never the legacy unpinned contexts array', () => { + expect(GITHUB_GUIDE).toMatch(/"checks":\s*\[/) + expect(GITHUB_GUIDE).toMatch(/"context":\s*"pair-explicit-approval",\s*"app_id"/) + expect(GITHUB_GUIDE).not.toMatch(/"contexts":\s*\[/) + expect(GITHUB_GUIDE).toMatch(/\/apps\/github-actions/) + }) + + it('setup-gates wires the pin and the pending-first property, not just the contexts', () => { + expect(SETUP_GATES).toMatch(/app_id/) + expect(SETUP_GATES).toMatch(/pending result as its first step/) + expect(SETUP_GATES).toMatch(/anti-accident/) + }) + + it('states the companion repository settings that shrink the residual', () => { + expect(GITHUB_GUIDE).toMatch(/read-only/i) + expect(GITHUB_GUIDE).toMatch(/CODEOWNERS/) + expect(GITHUB_GUIDE).toMatch(/require_code_owner_reviews/) + }) + + it('records that pair-review is anti-accident, NOT authorization (and names the unforgeable form)', () => { + for (const doc of [GITHUB_GUIDE, GUIDELINE, ADR_018]) { + expect(doc).toMatch(/anti-accident/i) + expect(doc).toMatch(/forgeable/i) + } + expect(GITHUB_GUIDE).toMatch(/App[\s\S]{0,120}check-run|check-run[\s\S]{0,120}App/i) + expect(DOCS_PAGE).toMatch(/anti-accident/i) + }) + + it('publishes a pending status FIRST, so an interrupted evaluation fails closed', () => { + const pendingAt = GITHUB_GUIDE.indexOf("state='pending' -f context='pair-explicit-approval'") + const checkoutAt = GITHUB_GUIDE.indexOf('uses: actions/checkout') + const resolveAt = GITHUB_GUIDE.indexOf('TIER="$(resolve_tier') + expect(pendingAt).toBeGreaterThan(-1) + expect(pendingAt).toBeLessThan(checkoutAt) + expect(pendingAt).toBeLessThan(resolveAt) + expect(GITHUB_GUIDE).toMatch(/cancel|interrupt/i) + }) +}) + +describe('the 🔴 approval predicate is shipped once, not transliterated (round 3)', () => { + it('lives in pr-state.sh and is consumed by name in the workflow snippet', () => { + expect(EVALUATOR).toMatch(/human_approval_jq_filter\(\)/) + expect(GITHUB_GUIDE).toMatch(/--jq "\$\(human_approval_jq_filter\)"/) + }) +}) + +describe('BOTH merge paths carry the synthesis precondition (round 3)', () => { + it('implement Phase 4 Step 4.1 re-synthesizes and HALTs instead of counting approvals', () => { + expect(POST_REVIEW_MERGE).toContain('merge_allowed') + expect(POST_REVIEW_MERGE).toContain('resolve_pr_state') + expect(POST_REVIEW_MERGE).toContain('ready-to-merge') + expect(POST_REVIEW_MERGE).toContain('human_approval_jq_filter') + expect(POST_REVIEW_MERGE).toMatch(/HALT/) + expect(POST_REVIEW_MERGE).not.toMatch(/at least one approval/i) + }) + + it('review Phase 6 keeps the same precondition (the reviewer-side path)', () => { + expect(MERGE_CASCADE).toContain('merge_allowed') + expect(MERGE_CASCADE).toMatch(/HALT/) + }) + + it('pr-states.md names /implement Phase 4 in the actor table', () => { + expect(GUIDELINE).toMatch(/`\/implement` Phase 4/) + }) + + it('implement links the state model instead of restating it', () => { + assertLinksResolve(POST_REVIEW_MERGE, POST_REVIEW_MERGE_PATH, ['pr-states.md', 'pr-state.sh']) + }) +}) + +describe('the deferred solo-maintainer token is citable everywhere it is deferred (round 3)', () => { + it('cites #398 in the model, the ADR, the docs page and this repo’s way-of-working', () => { + expect(GUIDELINE).toContain('#398') + expect(ADR_018).toContain('#398') + expect(DOCS_PAGE).toContain('398') + expect(ROOT_WOW).toContain('#398') + }) + + it('drops the "does not exist in this flow" phrasing in favour of the tracked issue', () => { + expect(GUIDELINE).not.toMatch(/it does not exist in this flow/) + }) +}) + +describe('doc coherence, round 3', () => { + it('the way-of-working TEMPLATE carries the approval-time re-run and links the ordering steps', () => { + expect(WOW_TEMPLATE).toMatch(/same head SHA/) + expect(WOW_TEMPLATE).toMatch(/github-implementation\.md/) + }) + + it('publish-pr documents /implement’s closing phase as a current caller, not a future one', () => { + expect(PUBLISH_PR).not.toMatch(/future closing phase/) + expect(PUBLISH_PR).not.toMatch(/wired in #256/) + expect(PUBLISH_PR).toMatch(/closing phase \(Step 3\.3\)/) + }) + + it('the host evidence table says what kind of artifact it is (point-in-time, re-runnable)', () => { + expect(GITHUB_GUIDE).toMatch(/point-in-time/i) + expect(GITHUB_GUIDE).toMatch(/re-running the ordering steps/i) + expect(ADR_018).toMatch(/point-in-time/i) + }) +}) diff --git a/scripts/smoke-tests/fixtures/github-pr-reviews.json b/scripts/smoke-tests/fixtures/github-pr-reviews.json new file mode 100644 index 00000000..03d1f8d7 --- /dev/null +++ b/scripts/smoke-tests/fixtures/github-pr-reviews.json @@ -0,0 +1,47 @@ +[ + { + "author_association": "CONTRIBUTOR", + "body": "", + "commit_id": "cc1fba122f0c912ba01288fe90ab2632e7e41057", + "id": 4796154484, + "node_id": "PRR_kwDODKw3uc8AAAABHd-CdA", + "state": "APPROVED", + "submitted_at": "2026-07-28T10:12:12Z", + "user": { + "id": 1083228, + "login": "sergiou87", + "site_admin": true, + "type": "User" + } + }, + { + "author_association": "NONE", + "body": "", + "commit_id": "cc1fba122f0c912ba01288fe90ab2632e7e41057", + "id": 4796154999, + "node_id": "PRR_kwDODKw3uc8AAAABHd-Cdb", + "state": "APPROVED", + "submitted_at": "2026-07-28T10:20:00Z", + "user": { + "id": 41898282, + "login": "github-actions[bot]", + "site_admin": false, + "type": "Bot" + } + }, + { + "author_association": "OWNER", + "body": "", + "commit_id": "0000000000000000000000000000000000000000", + "id": 4796155111, + "node_id": "PRR_kwDODKw3uc8AAAABHd-Cdc", + "state": "APPROVED", + "submitted_at": "2026-07-27T09:00:00Z", + "user": { + "id": 1083229, + "login": "pr-author", + "site_admin": false, + "type": "User" + } + } +] diff --git a/scripts/smoke-tests/run-all.sh b/scripts/smoke-tests/run-all.sh index 93cba090..21661a1e 100755 --- a/scripts/smoke-tests/run-all.sh +++ b/scripts/smoke-tests/run-all.sh @@ -267,6 +267,7 @@ if [ "$IS_CI" = "true" ]; then "default-resolution.sh" "tier-aware-gate.sh" "coverage-gate.sh" + "pr-state-flow.sh" ) for t in "${CI_TESTS[@]}"; do diff --git a/scripts/smoke-tests/scenarios/pr-state-flow.sh b/scripts/smoke-tests/scenarios/pr-state-flow.sh new file mode 100755 index 00000000..f3142e7e --- /dev/null +++ b/scripts/smoke-tests/scenarios/pr-state-flow.sh @@ -0,0 +1,415 @@ +#!/usr/bin/env bash +# OFFLINE_SAFE=true +# +# PR state flow (gate ≠ review) + pair review as a required check — verification +# scenario (story #234). +# +# Exercises the shipped, provider-agnostic synthesis evaluator end-to-end and audits +# the flow surfaces for the non-negotiable properties: +# 1. gate FIRST: a non-green gate never yields a merge-enabling state (AC2, R5.4), +# 2. synthesis: green gates + approved review ⇒ ready-to-merge; changes-requested +# ⇒ not-approved (AC3), +# 3. 🔴 needs an explicit HUMAN approval on top (AC4, D10), untagged ⇒ 🔴 fail-safe, +# 4. the pair review is a REQUIRED check — a pending/missing verdict blocks the +# merge (AC5, R5.7), and +# 5. the flow reads TAGS only — no classification criteria (D18), grep-verifiable. +# +# Per the gate-tooling ADL (2026-07-13) this shell/doc surface is verified with a +# smoke test, not a vitest unit test; the content invariants of the same surfaces are +# asserted in packages/knowledge-hub/src/conformance/pr-state-flow.test.ts. +source "$(dirname "$0")/../lib/utils.sh" +ensure_tmp_dir + +TEST_NAME="PR State Flow (gate != review)" +echo "=== Running $TEST_NAME ===" + +REPO_ROOT="${REPO_ROOT:-$(cd "$(dirname "$0")/../../.." && pwd)}" +DATASET="$REPO_ROOT/packages/knowledge-hub/dataset" +EVALUATOR="$DATASET/.pair/knowledge/assets/pr-state.sh" +TIER_RESOLVER="$DATASET/.pair/knowledge/assets/tier-resolve.sh" +GUIDELINE="$DATASET/.pair/knowledge/guidelines/collaboration/project-management-tool/pr-states.md" +GITHUB_GUIDE="$DATASET/.pair/knowledge/guidelines/collaboration/project-management-tool/github-implementation.md" +PUBLISH_PR="$DATASET/.skills/capability/publish-pr/SKILL.md" +REVIEW="$DATASET/.skills/process/review/SKILL.md" +MERGE_CASCADE="$DATASET/.skills/process/review/merge-and-cascade.md" +SETUP_GATES="$DATASET/.skills/capability/setup-gates/SKILL.md" +IMPLEMENT="$DATASET/.skills/process/implement/SKILL.md" +QUALITY_MODEL="$DATASET/.pair/knowledge/guidelines/quality-assurance/quality-model.md" +ADR="$REPO_ROOT/.pair/adoption/tech/adr/adr-018-pr-state-flow-required-checks.md" + +FAILED=0 +check() { # check + if [ "$2" = "$3" ]; then log_succ "$1 => $3"; else log_fail "$1: expected '$2' got '$3'"; FAILED=1; fi +} +audit() { # audit + local desc="$1" file="$2" + shift 2 + local pat + for pat in "$@"; do + if ! grep -q "$pat" "$file"; then + log_fail "$desc — missing '$pat' in $(basename "$file")" + FAILED=1 + return + fi + done + log_succ "$desc" +} + +for f in "$EVALUATOR" "$TIER_RESOLVER" "$GUIDELINE" "$GITHUB_GUIDE" "$PUBLISH_PR" "$REVIEW" \ + "$MERGE_CASCADE" "$SETUP_GATES" "$IMPLEMENT" "$QUALITY_MODEL" "$ADR"; do + assert_file "$f" || exit 1 +done + +# shellcheck source=/dev/null +source "$TIER_RESOLVER" +# shellcheck source=/dev/null +source "$EVALUATOR" + +# --- AC3: the happy paths (green gates + approved review) --- +check "🟢 gates green + approved" ready-to-merge "$(resolve_pr_state pass approved green 0 2>/dev/null)" +check "🟡 gates green + approved" ready-to-merge "$(resolve_pr_state pass approved yellow 0 2>/dev/null)" +check "tech-debt verdict is approving" ready-to-merge "$(resolve_pr_state pass tech-debt yellow 0 2>/dev/null)" + +# --- AC3: a changes-requested verdict routes to a human --- +check "changes-requested" not-approved "$(resolve_pr_state pass changes-requested green 0 2>/dev/null)" +check "changes-requested + red gate" not-approved "$(resolve_pr_state fail changes-requested green 0 2>/dev/null)" + +# --- AC2: the gate is the FIRST filter — judgment never unlocks a red gate (R5.4) --- +check "red gate + approved review" to-be-reviewed "$(resolve_pr_state fail approved green 1 2>/dev/null)" +check "pending gate + approved review" to-be-reviewed "$(resolve_pr_state pending approved green 1 2>/dev/null)" +check "unknown gate signal (fail-safe)" to-be-reviewed "$(resolve_pr_state '' approved green 1 2>/dev/null)" +if resolve_pr_state fail approved green 1 2>&1 >/dev/null | grep -q 'first filter'; then + log_succ "red gate warns that the gate is the first filter" +else + log_fail "red gate did not warn (gate-first invariant not surfaced)"; FAILED=1 +fi + +# --- AC5: a review that never produced a decision blocks the merge (R5.7) --- +check "review pending" to-be-reviewed "$(resolve_pr_state pass pending green 0 2>/dev/null)" +check "review missing" to-be-reviewed "$(resolve_pr_state pass missing green 0 2>/dev/null)" +check "review crashed/errored" to-be-reviewed "$(resolve_pr_state pass error green 0 2>/dev/null)" + +# --- AC4: 🔴 additionally requires an explicit human approval (D10) --- +check "🔴 approved, no human approval" to-be-reviewed "$(resolve_pr_state pass approved red 0 2>/dev/null)" +check "🔴 approved + human approval" ready-to-merge "$(resolve_pr_state pass approved red 1 2>/dev/null)" +# A real transition: the SAME PR, gates green and review approved throughout, with the +# tier driven by the label set through resolve_tier — 🟡 clears, the raise to 🔴 re-blocks. +LABELS_BEFORE="risk:yellow user story" +LABELS_AFTER="risk:red user story" # raise-only (D17): the yellow label is replaced +STATE_BEFORE="$(resolve_pr_state pass approved "$(resolve_tier "$LABELS_BEFORE" 2>/dev/null)" 0 2>/dev/null)" +STATE_AFTER="$(resolve_pr_state pass approved "$(resolve_tier "$LABELS_AFTER" 2>/dev/null)" 0 2>/dev/null)" +check "🟡 label set clears the merge" ready-to-merge "$STATE_BEFORE" +check "🟡→🔴 raise re-blocks it" to-be-reviewed "$STATE_AFTER" +check "the raise actually transitioned" "ready-to-merge->to-be-reviewed" "$STATE_BEFORE->$STATE_AFTER" + +# --- Fail-safe: untagged / malformed tier is treated as 🔴 --- +check "untagged tier (fail-safe red)" to-be-reviewed "$(resolve_pr_state pass approved '' 0 2>/dev/null)" +check "malformed tier (fail-safe red)" to-be-reviewed "$(resolve_pr_state pass approved banana 0 2>/dev/null)" +check "untagged + human approval" ready-to-merge "$(resolve_pr_state pass approved '' 1 2>/dev/null)" +if resolve_pr_state pass approved '' 0 2>&1 >/dev/null | grep -q 'fail-safe'; then + log_succ "untagged tier emits the fail-safe warning" +else + log_fail "untagged tier did not warn"; FAILED=1 +fi + +# --- Per-tier requirement lookup (the quality-model §4 row, projected) --- +if explicit_approval_required red 2>/dev/null; then log_succ "🔴 requires explicit approval"; else log_fail "🔴 did not require explicit approval"; FAILED=1; fi +if explicit_approval_required green 2>/dev/null; then log_fail "🟢 wrongly requires explicit approval"; FAILED=1; else log_succ "🟢 does not require explicit approval"; fi +if explicit_approval_required yellow 2>/dev/null; then log_fail "🟡 wrongly requires explicit approval"; FAILED=1; else log_succ "🟡 does not require explicit approval"; fi +if explicit_approval_required '' 2>/dev/null; then log_succ "untagged requires explicit approval (fail-safe)"; else log_fail "untagged did not require explicit approval"; FAILED=1; fi + +# --- merge_allowed: only ready-to-merge passes, never a silent pass --- +if merge_allowed ready-to-merge 2>/dev/null; then log_succ "ready-to-merge allows the merge"; else log_fail "ready-to-merge was blocked"; FAILED=1; fi +for state in to-be-reviewed not-approved '' garbage; do + if merge_allowed "$state" 2>/dev/null; then + log_fail "state '${state:-}' allowed a merge (silent pass)"; FAILED=1 + else + log_succ "state '${state:-}' blocks the merge" + fi +done + +# --- The required `pair-review` check conclusion mapping (AC5) --- +check "approved => success" success "$(review_check_conclusion approved 2>/dev/null)" +check "tech-debt => success" success "$(review_check_conclusion tech-debt 2>/dev/null)" +check "changes-requested => failure" failure "$(review_check_conclusion changes-requested 2>/dev/null)" +check "no decision => pending" pending "$(review_check_conclusion '' 2>/dev/null)" +check "crashed => pending" pending "$(review_check_conclusion crashed 2>/dev/null)" + +# --- Tags-only composition: PR labels -> tier -> state (no criteria anywhere) --- +PR_LABELS="risk:red user story" +check "labels->tier->state (🔴, no approval)" to-be-reviewed \ + "$(resolve_pr_state pass approved "$(resolve_tier "$PR_LABELS" 2>/dev/null)" 0 2>/dev/null)" +PR_LABELS="risk:green" +check "labels->tier->state (🟢)" ready-to-merge \ + "$(resolve_pr_state pass approved "$(resolve_tier "$PR_LABELS" 2>/dev/null)" 0 2>/dev/null)" +PR_LABELS="" +check "labels->tier->state (untagged ⇒ 🔴)" to-be-reviewed \ + "$(resolve_pr_state pass approved "$(resolve_tier "$PR_LABELS" 2>/dev/null)" 0 2>/dev/null)" + +# --- D18 grep audit: neither the evaluator nor the guideline's code blocks carry +# classification criteria. The flow decides nothing about risk — it reads the tag. +CRITERIA='\b(schema|migration|git diff|--numstat|lines?[ -]changed|loc\b|files?[ -]changed)\b' +if grep -Eiq "$CRITERIA" "$EVALUATOR"; then + log_fail "evaluator leaks classification criteria"; grep -Ein "$CRITERIA" "$EVALUATOR"; FAILED=1 +else + log_succ "evaluator contains no classification criteria" +fi +# fenced code blocks only (prose legitimately names the invariant) +for doc in "$GUIDELINE" "$GITHUB_GUIDE"; do + CODE="$(awk '/^```/{infb=!infb; next} infb' "$doc")" + if printf '%s\n' "$CODE" | grep -Eiq "$CRITERIA"; then + log_fail "$(basename "$doc") code blocks leak classification criteria" + printf '%s\n' "$CODE" | grep -Ein "$CRITERIA"; FAILED=1 + else + log_succ "$(basename "$doc") code blocks contain no classification criteria" + fi +done + +# --- Doc/skill wiring audits (the flow is only real if the skills compose it) --- +audit "pr-states.md defines the three states + the gate≠review split" "$GUIDELINE" \ + 'to-be-reviewed' 'ready-to-merge' 'not-approved' 'zero judgment' 'zero mechanical' 'R5.4' 'R5.7' 'D10' 'D18' +audit "publish-pr registers pair-review pending + dispatches a clean-context review" "$PUBLISH_PR" \ + 'pair-review' 'pr-state:to-be-reviewed' 'subagent' 'pr-states.md' +audit "review publishes the check + synthesizes the state via the shipped evaluator" "$REVIEW" \ + 'pr-state.sh' 'resolve_pr_state' 'review_check_conclusion' 'pr-state:ready-to-merge' 'quality-model.md' +audit "merge is gated on ready-to-merge (HALT otherwise)" "$MERGE_CASCADE" \ + 'merge_allowed' 'ready-to-merge' 'HALT' +audit "setup-gates wires both pair required checks + degraded mode" "$SETUP_GATES" \ + 'pair-review' 'pair-explicit-approval' 'DEGRADED' +audit "github guide carries the status + branch-protection recipe" "$GITHUB_GUIDE" \ + 'pair-review' 'pair-explicit-approval' 'required_status_checks' 'dismiss_stale_reviews' \ + 'statuses/\$HEAD_SHA' 'human_approval_jq_filter' 'gh label create "pr-state:' \ + 'required_approving_review_count": 0' +# The authorization predicate itself lives in ONE place — the shipped evaluator — +# and the job/test read it from there (round 3), so it cannot drift. +audit "the approval predicate ships in the evaluator, not as doc prose" "$EVALUATOR" \ + 'human_approval_jq_filter' 'user.type=="User"' 'commit_id==env.HEAD_SHA' \ + 'user.login!=env.PR_AUTHOR' + +# The publication command must be runnable with the ORDINARY token the skills hold: +# POST /check-runs is GitHub-App-only (403), so no command may call it, and the +# non-existent `author.is_bot` field must not be filtered on anywhere. +if grep -q 'repos/\$OWNER/\$REPO/check-runs' "$GITHUB_GUIDE"; then + log_fail "github guide still publishes via the App-only check-runs API"; FAILED=1 +else + log_succ "github guide publishes pair-review via the commit-statuses API" +fi +if grep -q 'is_bot' "$GITHUB_GUIDE"; then + log_fail "github guide still filters on the non-existent author.is_bot field"; FAILED=1 +else + log_succ "github guide does not rely on a non-existent is_bot field" +fi + +# --- The `pair-explicit-approval` job must read the tag only and demand a HUMAN +# approval on the CURRENT head (a bot/self approval can never satisfy 🔴). --- +if grep -q 'resolve_tier' "$GITHUB_GUIDE" && grep -q 'explicit_approval_required' "$GITHUB_GUIDE" \ + && grep -q 'HEAD_SHA' "$GITHUB_GUIDE"; then + log_succ "explicit-approval job resolves the tag and pins the approval to the current head" +else + log_fail "explicit-approval job lost the tag resolution / head pinning"; FAILED=1 +fi + +# --- Round 2: the approval job is an AUTHORIZATION control, so its code must come +# from the base ref and its verdict must land on the commit protection reads. --- +audit "approval job runs from a trusted ref + posts a head-pinned status" "$GITHUB_GUIDE" \ + 'pull_request_target:' 'ref: \${{ github.event.pull_request.base.sha }}' \ + 'persist-credentials: false' 'statuses: write' 'cancel-in-progress: true' \ + "statuses/\\\$HEAD_SHA" "context='pair-explicit-approval'" +if grep -Eq '^ pull_request:[[:space:]]*$' "$GITHUB_GUIDE"; then + log_fail "approval job trigger fell back to plain pull_request (PR ships its own gate)"; FAILED=1 +else + log_succ "no plain pull_request trigger on the authorization job" +fi +if grep -q 'ref: ${{ github.event.pull_request.head.sha }}' "$GITHUB_GUIDE"; then + log_fail "a checkout is pinned to the PR head (tamperable authorization control)"; FAILED=1 +else + log_succ "no checkout is pinned to the PR head" +fi +# The copy-pasteable payload must not walk into the enforce_admins trap, and the +# approval query must paginate (an approval on page 2 is not zero approvals). +if grep -q '"enforce_admins": true' "$GITHUB_GUIDE"; then + log_fail "branch-protection payload ships enforce_admins: true (no escape hatch)"; FAILED=1 +else + log_succ "branch-protection payload ships enforce_admins: false" +fi +if grep -q 'reviews?per_page=100' "$GITHUB_GUIDE"; then + log_fail "approval query is capped at one page (an approval on page 2 reads as zero)"; FAILED=1 +else + log_succ "approval query is not capped at a single page" +fi +if grep -q -- '--paginate' "$GITHUB_GUIDE"; then + log_succ "approval query paginates the reviews stream" +else + log_fail "approval query lost --paginate"; FAILED=1 +fi +# One variable convention: no snippet may mix `repos/$OWNER/$REPO` with `repos/$REPO`. +if grep -q 'repos/\$OWNER/\$REPO' "$GITHUB_GUIDE"; then + log_fail "guide mixes repos/\$OWNER/\$REPO with repos/\$REPO (404 for a copy-paster)"; FAILED=1 +else + log_succ "guide uses one repository-variable form" +fi + +# --- Round 2: the review dispatch must be executable (no nested delegation) and +# must never merge; pr-states must not restate quality-model thresholds. --- +audit "publish-pr hands the dispatch to the caller instead of nesting" "$PUBLISH_PR" \ + 'review-dispatch-required' 'NEVER run Phase 6' +audit "implement dispatches the review from the top-level session" "$IMPLEMENT" \ + 'review-dispatch-required' 'NEVER merge' +audit "review defines the non-interactive (dispatched) contract" "$REVIEW" \ + 'dispatched' 'non-interactive' +for pat in '1 working day' '2 working days' 'extended checklist'; do + if grep -q "$pat" "$GUIDELINE"; then + log_fail "pr-states.md restates a quality-model threshold ('$pat')"; FAILED=1 + else + log_succ "pr-states.md does not restate '$pat'" + fi +done +audit "quality-model defines what 'extended' checklist depth means" "$QUALITY_MODEL" \ + 'Checklist depth' 'no section skipped' +audit "ADR-018 records the trusted-ref decision and the reviewer-identity question" "$ADR" \ + 'Trusted ref' 'reviewer identity' 'Head-pinned verdict' 'throwaway repository' + +# --- The 🔴 approval filter, executed against a COMMITTED payload fixture ------ +# The approval query is the one piece of the recipe that decides authorization, so +# it is asserted deterministically — offline, in CI, with no dependency on any +# third-party pull request staying reachable. The fixture is a real REST +# `pulls/{n}/reviews` payload (captured 2026-07-30) extended with a Bot review and +# a stale approval on another commit, so the filter has something to reject. +FIXTURE="$REPO_ROOT/scripts/smoke-tests/fixtures/github-pr-reviews.json" +assert_file "$FIXTURE" || exit 1 +audit "fixture carries the fields the documented filter reads" "$FIXTURE" \ + '"commit_id"' '"state"' '"type"' '"login"' +if command -v jq >/dev/null 2>&1; then + # NOT a transliteration (round 3): the predicate under test is the *shipped* + # `human_approval_jq_filter` from pr-state.sh — the same string the workflow feeds + # to `gh api --jq` — sourced above and pushed through the same + # `jq … | grep -c .` pipeline the job uses. Drift between recipe and test is + # therefore impossible rather than merely unlikely. + FILTER="$(human_approval_jq_filter)" + HEAD='cc1fba122f0c912ba01288fe90ab2632e7e41057' + count_approvals() { # count_approvals + HEAD_SHA="$1" PR_AUTHOR="$2" jq -r "$FILTER" "$FIXTURE" | grep -c . || true + } + check "filter counts the human non-author approval on the head" 1 \ + "$(count_approvals "$HEAD" pr-author)" + check "filter rejects a stale approval on another commit" 0 \ + "$(count_approvals 1111111111111111111111111111111111111111 pr-author)" + check "filter rejects the author's own approval" 0 \ + "$(count_approvals 0000000000000000000000000000000000000000 pr-author)" + # With the only human-on-head review authored by the PR author, the Bot approval + # on the same head commit is all that is left — and it must not count. + check "filter rejects a Bot approval on the head" 0 \ + "$(count_approvals "$HEAD" sergiou87)" +else + log_warn "approval-filter assertions skipped — jq not installed (fixture shape still asserted)" +fi + +# --- LIVE host probes: WARN-ONLY by construction ------------------------------- +# Useful (they catch an endpoint that starts refusing the skills' token) but never +# authoritative: they never set FAILED, never fall back to a third-party +# repository, and are skipped whole offline/unauthenticated. The behaviour they +# probe is already asserted above against the fixture. +if [ "${IS_OFFLINE:-false}" != "true" ] && command -v gh >/dev/null 2>&1 && + gh auth status >/dev/null 2>&1; then + PROBE_REPO="${PAIR_HOST_PROBE_REPO:-$(gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)}" + PROBE_SHA="$(git -C "$REPO_ROOT" rev-parse HEAD 2>/dev/null || true)" + if [ -n "$PROBE_REPO" ] && [ -n "$PROBE_SHA" ] && + gh api "repos/$PROBE_REPO/commits/$PROBE_SHA/status" --jq '.state' >/dev/null 2>&1; then + log_succ "host: commit-statuses API reachable with the agent token ($PROBE_REPO)" + else + log_warn "host probe skipped — commit-statuses read not available" + fi + # A reviewed PR of THIS repository only — no external fallback. + PROBE_PR="" + for n in $(gh api "repos/$PROBE_REPO/pulls?state=all&per_page=10" --jq '.[].number' 2>/dev/null); do + if [ "$(gh api "repos/$PROBE_REPO/pulls/$n/reviews?per_page=1" --jq 'length' 2>/dev/null)" = "1" ]; then + PROBE_PR="$n" + break + fi + done + if [ -z "$PROBE_PR" ]; then + log_warn "host probe skipped — no PR of $PROBE_REPO carries a native review" + else + SHAPE="$(gh api "repos/$PROBE_REPO/pulls/$PROBE_PR/reviews?per_page=1" \ + --jq '.[0] | (has("commit_id") and has("state") and (.user | has("type")))' 2>/dev/null)" + [ "$SHAPE" = true ] && + log_succ "host: live reviews payload matches the fixture shape (#$PROBE_PR)" || + log_warn "host probe inconclusive — live reviews payload unreadable (#$PROBE_PR)" + # The trap the recipe documents: `gh pr view --json reviews` carries NO bot flag. + HAS_BOT_FLAG="$(gh pr view "$PROBE_PR" --repo "$PROBE_REPO" --json reviews \ + --jq '.reviews[0].author | has("is_bot")' 2>/dev/null)" + [ "$HAS_BOT_FLAG" = false ] && + log_succ "host: gh pr view reviews exposes no bot flag (trap confirmed)" || + log_warn "host probe inconclusive — bot-flag trap not observable (#$PROBE_PR)" + fi +else + log_warn "live host probes skipped — offline or gh not authenticated (fixture assertions still ran)" +fi + +# --- Round 3: the authorization context must be producer-pinned, the job must fail +# closed when interrupted, and neither doc may overclaim what `pair-review` proves. +audit "branch protection pins the producer of the authorization context" "$GITHUB_GUIDE" \ + '"checks": \[' 'app_id' '/apps/github-actions' +if grep -q '"contexts": \[' "$GITHUB_GUIDE"; then + log_fail "payload still uses the legacy contexts form (any principal satisfies the gate)"; FAILED=1 +else + log_succ "payload uses the checks form (no unpinned required context)" +fi +audit "the companion repository settings are stated, not assumed" "$GITHUB_GUIDE" \ + 'read-only' 'CODEOWNERS' 'require_code_owner_reviews' +audit "setup-gates carries the producer pin + pending-first as authorization properties" "$SETUP_GATES" \ + 'app_id' 'pending result as its first step' 'anti-accident' +audit "pair-review is documented as anti-accident, not authorization" "$GITHUB_GUIDE" \ + 'anti-accident' 'forgeable' 'What each context proves' +audit "pr-states.md draws the same distinction" "$GUIDELINE" \ + 'anti-accident' 'forgeable' 'authorization' +audit "ADR-018 records the pin, the residual and the honest claim" "$ADR" \ + 'app_id' 'anti-accident' 'forgeable' 'point-in-time' +# Pending FIRST: the status must be published before the job can fail or be cancelled, +# otherwise a stale lower-tier `success` survives the interruption. +PENDING_LINE="$(grep -n "state='pending' -f context='pair-explicit-approval'" "$GITHUB_GUIDE" | head -1 | cut -d: -f1)" +CHECKOUT_LINE="$(grep -n 'uses: actions/checkout' "$GITHUB_GUIDE" | head -1 | cut -d: -f1)" +RESOLVE_LINE="$(grep -n 'TIER="\$(resolve_tier' "$GITHUB_GUIDE" | head -1 | cut -d: -f1)" +if [ -n "$PENDING_LINE" ] && [ -n "$CHECKOUT_LINE" ] && [ -n "$RESOLVE_LINE" ] && + [ "$PENDING_LINE" -lt "$CHECKOUT_LINE" ] && [ "$PENDING_LINE" -lt "$RESOLVE_LINE" ]; then + log_succ "approval job publishes pending BEFORE checkout/tier resolution (fails closed)" +else + log_fail "approval job does not publish pending first (an interrupted run leaves a stale success)"; FAILED=1 +fi +# Both merge paths — not just the reviewer's — carry the synthesis precondition. +POST_REVIEW_MERGE="$DATASET/.skills/process/implement/post-review-merge.md" +assert_file "$POST_REVIEW_MERGE" || exit 1 +audit "implement Phase 4 (the author-side merge) runs the same precondition" "$POST_REVIEW_MERGE" \ + 'merge_allowed' 'ready-to-merge' 'HALT' 'resolve_pr_state' 'human_approval_jq_filter' +if grep -q 'at least one approval' "$POST_REVIEW_MERGE"; then + log_fail "implement Phase 4 still merges on 'at least one approval' (skips the synthesis)"; FAILED=1 +else + log_succ "implement Phase 4 no longer merges on an approval count" +fi +audit "pr-states.md names implement Phase 4 as a merge actor" "$GUIDELINE" \ + '/implement` Phase 4' +# The deferred solo-maintainer token is citable from every place that defers to it. +audit "pr-states.md cites the deferral issue" "$GUIDELINE" '#398' +audit "ADR-018 cites the deferral issue" "$ADR" '#398' +DOCS_PAGE="$REPO_ROOT/apps/website/content/docs/concepts/pr-state-flow.mdx" +ROOT_WOW="$REPO_ROOT/.pair/adoption/tech/way-of-working.md" +DATASET_WOW="$DATASET/.pair/adoption/tech/way-of-working.md" +for f in "$DOCS_PAGE" "$ROOT_WOW" "$DATASET_WOW"; do assert_file "$f" || exit 1; done +audit "the docs page cites the deferral issue" "$DOCS_PAGE" '398' +audit "this repo's way-of-working cites the deferral issue + the app pin" "$ROOT_WOW" '#398' 'app_id' +audit "the way-of-working TEMPLATE carries the full ordering (incl. the approval re-run)" \ + "$DATASET_WOW" 'same head SHA' 'github-implementation.md' +audit "publish-pr no longer describes its caller in the future tense" "$PUBLISH_PR" \ + "closing phase (Step 3.3)" +if grep -q 'future closing phase' "$PUBLISH_PR" || grep -q 'wired in #256' "$PUBLISH_PR"; then + log_fail "publish-pr still documents /implement as a future caller"; FAILED=1 +else + log_succ "publish-pr documents /implement's closing phase as the current caller" +fi +audit "the host evidence table states what kind of artifact it is" "$GITHUB_GUIDE" \ + 'point-in-time' 're-running the ordering steps' + +if [ "$FAILED" -ne 0 ]; then + log_fail "$TEST_NAME had failures"; exit 1 +fi +log_succ "$TEST_NAME passed"