From 6a8a1db9d3e22d3e8edba35d22afeec59686ba9d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 16:24:53 +0000 Subject: [PATCH 1/3] feat(source-control): judge a review lane by what it produced, not by its check row An AI-review lane reports on its session, not on its output: a session that ends without error concludes success even when it posted no review body and no finding, so the check row goes green and an evidence guard that reads posted review bodies has nothing to judge when none was posted. Cost is no signal either, since the session is billed whether or not it reviewed anything. The monitor checklist classified such a lane PASS on its check row alone and the readiness gate cleared. New step B2 requires each review lane to have produced something readable, a posted review body or at least one finding, and classifies a lane that produced nothing as ABSENT rather than PASS, whether it never started, could not load the skill it was told to invoke, or stopped early. An absent lane is substituted with a local review over the same diff and named in the report, and step E will not clear until every review lane is productive or substituted. A lane whose posted body admits a manual fallback gets the same verdict. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP --- plugins/source-control/.claude-plugin/plugin.json | 2 +- plugins/source-control/CHANGELOG.md | 14 ++++++++++++++ .../source-control/skills/pull-request/SKILL.md | 4 +++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 5f5f277a2..2759cdbf5 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.55.74", + "version": "0.55.75", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-authored-by trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop, safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /babysit-loop (the loop-lane merge lane: a standing or drain loop that invokes babysit-prs per cycle, configured through repo-scoped babysit_loop_* keys on the layered source-control.md seam, with merge authority human-only until the target repo's tracked config adopts the lane, a gate-proven C2-mechanical baseline once adopted, and standing merge-rung raises binding from the team-tracked layer only, with one named exception, where an invocation line explicitly typing both the autopilot tier keyword and the dedicated raise argument --merge c3-this-run widens that single invocation's merge authority up to C3 behind a fresh independent frontier-tier resolver, while C4-structural and C5-untrusted-provenance stay unconditionally human-merge), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply, which interviews the repo and writes the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep, never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index b624ace54..16e0c77b1 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,20 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.55.75] + +### Added + +- **The PR monitor judges a review lane by what it produced, not by its check row.** New step B2 + in the per-iteration checklist: an AI-review lane that concluded success having posted no review + body and no finding reviewed nothing, and is classified ABSENT rather than PASS. These lanes + report on their session rather than on their output, so a session that ends without error goes + green whether or not it reviewed anything, and an evidence guard that reads posted review bodies + has nothing to judge when none was posted. An absent lane is now substituted with a local review + over the same diff (`/review:fanout`, or `/review:code-review` for the correctness lane alone) + and named in the report; the readiness gate in step E will not clear until every review lane is + productive or substituted. + ## [0.55.74] ### Changed diff --git a/plugins/source-control/skills/pull-request/SKILL.md b/plugins/source-control/skills/pull-request/SKILL.md index 1932bc447..e9e0f0709 100644 --- a/plugins/source-control/skills/pull-request/SKILL.md +++ b/plugins/source-control/skills/pull-request/SKILL.md @@ -161,6 +161,7 @@ When a channel event, Monitor notification, or poll iteration fires, complete ev - [ ] **A. Terminal state:** `gh pr view --json state -q .state`. MERGED/CLOSED โ†’ self-terminate - [ ] **B. CI checks:** `gh pr checks `. Classify EVERY non-pending check (pass/fail/skipped). Read logs for ANY failure per ยง3.1 fetch chain +- [ ] **B2. Review-lane productivity:** a green AI-review lane is not evidence that a review happened. For every review lane the repository runs, confirm it produced something readable: a posted review body, or at least one finding. A lane that concluded success having posted nothing reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. Classify that lane ABSENT, never PASS, and stand a local review in its place over the same diff: `/review:fanout` for breadth, or `/review:code-review` for the correctness lane alone. A lane that posted a review body admitting it fell back to a manual pass is the same verdict. Name every absent lane in the F report and say the local run substituted for it - [ ] **C. Fetch ALL comments from ALL sources:** read every update on the PR regardless of author or format. Three API surfaces + reviews: - [ ] C1. Review-thread comments: `gh api --paginate "repos///pulls//comments?per_page=100"` - [ ] C2. Issue-level comments: `gh api --paginate "repos///issues//comments?per_page=100"` (includes AI-review summaries, user replies, bot task-completion posts) @@ -181,7 +182,7 @@ When a channel event, Monitor notification, or poll iteration fires, complete ev - [ ] D7. Post a follow-up reply citing the fix commit SHA - [ ] **Verify follow-up reply posted, same surface routing as D5:** inline thread โ†’ `pulls//comments` filtered by `in_reply_to_id`; issue-level โ†’ `gh api --paginate "repos///issues//comments?per_page=100" --jq '.[] | select((.body | contains("")) and .user.login == "") | .body'`. Confirm the follow-up with SHA appears on GitHub. Constrain on BOTH the SHA and the posting identity, and never on `.[-1]`. `.[-1]` is wrong because these endpoints return oldest-first, so on an unpaginated list it is the 30th-oldest comment. SHA alone is wrong because this is a control gate you act on: anyone else quoting the fix SHA, a reviewer, another bot, satisfies it, and the check reports your reply as posted when the write failed. `` is the login you posted as (the bot-identity wrapper's account when the project has one, your own otherwise) - [ ] D7.5. Resolve review thread, **author- and classification-conditional, inline only**. Eligible only when EVERY finding in the thread carries a recorded disposition: `VALID (fix now)` with the fix pushed and cited, `VALID (defer)` grounded per D4.6 with the item id cited, or `INCORRECT` with counter-evidence posted, one dispositioned finding never makes a multi-finding thread eligible, and a single `UNCERTAIN` escalates and holds the thread open. Resolve threads opened by a BOT reviewer that you addressed. NEVER resolve HUMAN-authored threads (the human resolves their own). NEVER resolve your OWN (your posting identity. Bot or personal). **A `VALID (defer)` never clears the gate for a merge this same session performs**. `full` mode monitors and merges in one session, so route the deferral to an independent adjudicating context, or leave the thread unresolved and do not merge. Fail closed. Detect bot via the API surface in use. REST `user.type==Bot`; GraphQL `author.__typename==Bot` (resolution runs via GraphQL). Verify `isResolved == true` via GraphQL -- [ ] **E. Readiness gate:** ALL checks terminal + ALL comments addressed + 2-min cooldown since last activity per [readiness.md](reference/readiness.md) +- [ ] **E. Readiness gate:** ALL checks terminal + every review lane productive or substituted per B2 + ALL comments addressed + 2-min cooldown since last activity per [readiness.md](reference/readiness.md) - [ ] **F, Report:** present the full readiness table OR list remaining blockers **Receiving an event is not processing it.** Each event drives at least steps A-C, and a new comment event drives D1-D7 for that comment. Readiness is declared only after E. @@ -242,6 +243,7 @@ Public action for retrieving failed-CI evidence. Tiered fetch chain. Cheapest si - **Bot comments need reactions AND replies.** React (๐Ÿ‘/๐Ÿ‘Ž/๐Ÿ‘€) on every substantive comment AND reply with a per-finding classification table with evidence. Don't skip any reviewer, each gets individual attention. Verify BOTH the reaction and the reply landed on GitHub via API query. Resolve the thread only when D7.5's conditions are met for EVERY finding in it, and only IF bot-authored; never human-authored, never your own. D7.5 in the checklist above is the single statement of which dispositions qualify and who may act on one, do not restate it here - **Monitor is async, not serial.** Process comments as they arrive while CI is still running. Don't wait for all checks to complete before reading comments, bots post at different times - **Check mergeable BEFORE polling CI.** `gh pr view --json mergeable`, if `CONFLICTING`, GitHub won't trigger workflows. Integrate the default branch first, then poll +- **A green review lane can mean nothing was reviewed.** These lanes report on their session, not on their output: a session that ends without error is a success even when it posted no review and produced no finding, so the check row goes green and any evidence guard beside it has no posted body to judge. Cost is no signal either, since the session is billed whether or not it reviewed anything. Judge a review lane by what it produced (B2), never by its check row, and run the review locally when it produced nothing - **Never merge with unclassified FAILURE check runs.** Every FAILURE must be investigated, classified (real failure vs informational), and documented before merge is even suggested. See [readiness.md](reference/readiness.md) for the full 6-gate checklist - **"No comments" does NOT mean "ready."** Comment-only actors post at unpredictable times. A 2-minute cooldown after the last check-run completion or comment arrival prevents the race condition. See readiness.md Gate 5 - **Security scans are always blocking.** Any check run or bot comment reporting security findings (secrets, vulnerabilities) triggers mandatory triage, even if the finding is a false positive, it must be explicitly classified and documented before merge From 31976587078d4451b41891d2b59c6b1f6bc31af2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 17:08:47 +0000 Subject: [PATCH 2/3] fix(source-control): scope lane productivity to the current round and enforce it on every merge path Three review findings, each verified against the files before being taken. The productivity check was existential over the whole pull request, so a review body or finding left by an earlier head, or by an earlier rerun of the same head, satisfied it while the newly green lane emitted nothing. That is the false pass the step exists to catch. Productivity is now scoped to the head under review through the per-surface commit fields Gate 5 already names, which the step points at rather than restating. The invariant lived only in the monitor checklist, but reference/readiness.md is the single source of truth that both monitor.md and merge.md rerun, so a direct merge invocation and the final re-verification in full did not enforce it. It is now a Gate 5 item there as well, stated as what reaching that gate's bound hands off to: the bound ends the wait for a silent reviewer, it does not supply the review. The local correctness fallback named the CI lane skill, which runs only where the workflow invokes it and depends on that wrapper for the repository, pull request number, head SHA and posting mechanics. A monitor session has no such wrapper. The fallback is now the bundled code-review command against an explicit target, conditioned on it resolving in the session. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP --- plugins/source-control/CHANGELOG.md | 22 ++++++++++++------- .../skills/pull-request/SKILL.md | 2 +- .../pull-request/reference/readiness.md | 2 ++ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 16e0c77b1..1784cc214 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -8,14 +8,20 @@ All notable changes to the `source-control` plugin are documented here. Format f ### Added - **The PR monitor judges a review lane by what it produced, not by its check row.** New step B2 - in the per-iteration checklist: an AI-review lane that concluded success having posted no review - body and no finding reviewed nothing, and is classified ABSENT rather than PASS. These lanes - report on their session rather than on their output, so a session that ends without error goes - green whether or not it reviewed anything, and an evidence guard that reads posted review bodies - has nothing to judge when none was posted. An absent lane is now substituted with a local review - over the same diff (`/review:fanout`, or `/review:code-review` for the correctness lane alone) - and named in the report; the readiness gate in step E will not clear until every review lane is - productive or substituted. + in the per-iteration checklist: an AI-review lane that concluded success having produced no + review body and no finding **for the current round** reviewed nothing, and is classified ABSENT + rather than PASS. These lanes report on their session rather than on their output, so a session + that ends without error goes green whether or not it reviewed anything, and cost is no signal + either since the session is billed either way. Productivity is scoped to the head under review + through the per-surface commit fields Gate 5 already names, so an artifact left by an earlier + head or an earlier rerun cannot stand in for a round that emitted nothing. An absent lane is + substituted with a local review over the same diff (`/review:fanout`, or the bundled + `/code-review` against an explicit target for a correctness lane) and named in the report. +- **The substitution is enforced on every merge path, not only the monitor loop.** The same + invariant is now a Gate 5 item in `reference/readiness.md`, which is the single source of truth + both `monitor.md` and `merge.md` rerun, so a direct `merge` invocation and the final + re-verification in `full` enforce it too. It states what reaching Gate 5's bound hands off to: + the bound ends the wait for a silent reviewer, it does not supply the review. ## [0.55.74] diff --git a/plugins/source-control/skills/pull-request/SKILL.md b/plugins/source-control/skills/pull-request/SKILL.md index e9e0f0709..cebf3155b 100644 --- a/plugins/source-control/skills/pull-request/SKILL.md +++ b/plugins/source-control/skills/pull-request/SKILL.md @@ -161,7 +161,7 @@ When a channel event, Monitor notification, or poll iteration fires, complete ev - [ ] **A. Terminal state:** `gh pr view --json state -q .state`. MERGED/CLOSED โ†’ self-terminate - [ ] **B. CI checks:** `gh pr checks `. Classify EVERY non-pending check (pass/fail/skipped). Read logs for ANY failure per ยง3.1 fetch chain -- [ ] **B2. Review-lane productivity:** a green AI-review lane is not evidence that a review happened. For every review lane the repository runs, confirm it produced something readable: a posted review body, or at least one finding. A lane that concluded success having posted nothing reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. Classify that lane ABSENT, never PASS, and stand a local review in its place over the same diff: `/review:fanout` for breadth, or `/review:code-review` for the correctness lane alone. A lane that posted a review body admitting it fell back to a manual pass is the same verdict. Name every absent lane in the F report and say the local run substituted for it +- [ ] **B2. Review-lane productivity:** a green AI-review lane is not evidence that a review happened. For every review lane the repository runs, confirm it produced something readable **for the current round**: a review body or a finding attributed to this head, scoped by the per-surface commit fields [readiness.md](reference/readiness.md) Gate 5 names. An artifact from an earlier head, or from an earlier rerun of this one, satisfies a bare existence check while the newly green lane emitted nothing, which is the false pass this step exists to catch. A lane that concluded success having produced nothing for this round reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. Classify it ABSENT, never PASS, and stand a local review in its place over the same diff: `/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for the correctness lane alone, whichever resolves in this session. A lane whose posted body admits it fell back to a manual pass is the same verdict. Name every absent lane in the F report and say what stood in for it - [ ] **C. Fetch ALL comments from ALL sources:** read every update on the PR regardless of author or format. Three API surfaces + reviews: - [ ] C1. Review-thread comments: `gh api --paginate "repos///pulls//comments?per_page=100"` - [ ] C2. Issue-level comments: `gh api --paginate "repos///issues//comments?per_page=100"` (includes AI-review summaries, user replies, bot task-completion posts) diff --git a/plugins/source-control/skills/pull-request/reference/readiness.md b/plugins/source-control/skills/pull-request/reference/readiness.md index 008425cbb..9049bedd3 100644 --- a/plugins/source-control/skills/pull-request/reference/readiness.md +++ b/plugins/source-control/skills/pull-request/reference/readiness.md @@ -157,6 +157,8 @@ gh api --paginate "repos/{owner}/{repo}/issues//comments?per_page=100 `commit_id` is the wrong field for this question: it re-anchors to the newest head while a comment's hunk still applies, so it counts surviving prior-round comments as current and short-circuits the wait. Unpaginated, the count also undercounts: the comments you are waiting on are the newest, and on a PR with prior review rounds the newest are exactly what page 1 omits. The count is slurped rather than passed to `--jq` for the reason rule 3 gives: a reduction like `length` inside `--jq` runs per page and prints one number per page, never the total. +- [ ] **A review lane that produced nothing for this round is SUBSTITUTED before the gate clears, not merely reported.** Its own check row cannot settle this: these lanes report on their session rather than on their output, so a session that ends without error is green whether or not it reviewed anything, and cost is no signal either since the session is billed either way. Judge the lane by the artifacts the scoping rule above attributes to this head; zero of them means nothing was reviewed, however green the row. Run a local review over the same diff (`/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for a correctness lane, whichever resolves in this session), then record in the readiness verdict which lanes were absent and what stood in for them. A lane whose posted body admits it fell back to a manual pass counts as absent on the same terms. This is what the bound above hands off to: reaching the bound ends the wait, it does not supply the review. + ### Gate 6: No pending work - [ ] No fix pushes are in flight (a push restarts the entire monitoring loop) From 9a09f79c504aac569d476cb29493a2d21e07fb16 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 17:28:24 +0000 Subject: [PATCH 3/3] fix(source-control): anchor the lane roster, report the substitution, and register the clause Six findings from a local review, run because both AI-review lanes produced nothing for this head and the new rule classes that ABSENT. Five were confirmed against the files; the sixth, that babysit-prs merges through readiness.md, was checked and is false in the direction that mattered: its merge path never reads that file, so the changelog's "every merge path" was the overclaim instead. The roster was the load-bearing hole. Both statements said "for every review lane" without naming where the roster comes from, and the readiness item sat beside a reviewer roster built from the bot authors who posted. A lane that posted nothing is missing from that roster by construction, so on the merge path the item applied to an empty set, which is precisely the lane it exists for. Both surfaces now take the roster from the checks list. The rerun claim was an overclaim. Commit-field scoping distinguishes an earlier HEAD, not an earlier RERUN of the current head, whose artifacts carry the same SHA. Both surfaces now bound that case by timestamp against the run's start rather than implying the commit fields cover it. The surface list was wrong by omission. Issue-level comments carry no commit field and are where AI-review summaries land, so naming only "commit fields" either false-ABSENTs a real review or falls back to the bare existence check. All three surfaces are now named with the field each one actually has. The substitution had nowhere to be reported: both rule statements said to record it in the verdict, and neither readiness nor monitor template had a field, so it would drop out of a report that otherwise reads all-clear. Both templates gain a Review lanes line. The rule is stated on two surfaces, so it is registered as a contract clause with readiness.md canonical and the monitor checklist tagged as a restatement, which puts its four qualifiers under the drift gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP --- plugins/source-control/CHANGELOG.md | 14 +++++---- .../skills/pull-request/SKILL.md | 2 +- .../skills/pull-request/reference/monitor.md | 1 + .../pull-request/reference/readiness.md | 5 +++- scripts/contract-clause-registry.json | 29 +++++++++++++++++++ 5 files changed, 44 insertions(+), 7 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 1784cc214..f5bda6240 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -17,11 +17,15 @@ All notable changes to the `source-control` plugin are documented here. Format f head or an earlier rerun cannot stand in for a round that emitted nothing. An absent lane is substituted with a local review over the same diff (`/review:fanout`, or the bundled `/code-review` against an explicit target for a correctness lane) and named in the report. -- **The substitution is enforced on every merge path, not only the monitor loop.** The same - invariant is now a Gate 5 item in `reference/readiness.md`, which is the single source of truth - both `monitor.md` and `merge.md` rerun, so a direct `merge` invocation and the final - re-verification in `full` enforce it too. It states what reaching Gate 5's bound hands off to: - the bound ends the wait for a silent reviewer, it does not supply the review. +- **The substitution is enforced on the readiness gate, not only in the monitor loop.** The same + invariant is now a Gate 5 item in `reference/readiness.md`, the single source of truth both + `monitor.md` (Phase 3.4) and `merge.md` (Phase 4.1) rerun, so a direct `merge` invocation and + the final re-verification in `full` enforce it too. It states what reaching Gate 5's bound hands + off to: the bound ends the wait for a silent reviewer, it does not supply the review. The + `babysit-prs` worker and autopilot tiers merge through their own gate and do not read this file, + so they are unchanged by this entry. +- **Both readiness templates gained a `Review lanes:` line**, so an absent lane and its local + substitute are reported rather than dropped from a verdict that otherwise reads all-clear. ## [0.55.74] diff --git a/plugins/source-control/skills/pull-request/SKILL.md b/plugins/source-control/skills/pull-request/SKILL.md index cebf3155b..ed81047ea 100644 --- a/plugins/source-control/skills/pull-request/SKILL.md +++ b/plugins/source-control/skills/pull-request/SKILL.md @@ -161,7 +161,7 @@ When a channel event, Monitor notification, or poll iteration fires, complete ev - [ ] **A. Terminal state:** `gh pr view --json state -q .state`. MERGED/CLOSED โ†’ self-terminate - [ ] **B. CI checks:** `gh pr checks `. Classify EVERY non-pending check (pass/fail/skipped). Read logs for ANY failure per ยง3.1 fetch chain -- [ ] **B2. Review-lane productivity:** a green AI-review lane is not evidence that a review happened. For every review lane the repository runs, confirm it produced something readable **for the current round**: a review body or a finding attributed to this head, scoped by the per-surface commit fields [readiness.md](reference/readiness.md) Gate 5 names. An artifact from an earlier head, or from an earlier rerun of this one, satisfies a bare existence check while the newly green lane emitted nothing, which is the false pass this step exists to catch. A lane that concluded success having produced nothing for this round reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. Classify it ABSENT, never PASS, and stand a local review in its place over the same diff: `/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for the correctness lane alone, whichever resolves in this session. A lane whose posted body admits it fell back to a manual pass is the same verdict. Name every absent lane in the F report and say what stood in for it +- [ ] **B2. Review-lane productivity:** a green AI-review lane is not evidence that a review happened. Take the lane roster from step B's `gh pr checks`, never from the authors who posted: a lane that posted nothing is missing from every author-derived roster by construction, so such a roster applies this step to an empty set. For each lane, confirm it produced something readable **for the current round**, scoped per surface the way [readiness.md](reference/readiness.md) Gate 5 scopes its wait: `original_commit_id` on inline review comments, `commit_id` on the reviews endpoint, and `created_at` against the push time on issue-level comments, which carry no commit field at all and are where AI-review summaries land. An artifact from an earlier HEAD cannot satisfy this. An artifact from an earlier RERUN of the current head is indistinguishable by commit field alone, since it carries the same SHA, so bound that case by timestamp against the run's start. A lane that concluded success having produced nothing for this round reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. Classify it ABSENT, never PASS, and stand a local review in its place over the same diff: `/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for a correctness lane, whichever resolves in this session. A lane whose posted body admits it fell back to a manual pass is the same verdict. Name every absent lane and its substitute on the F report's `Review lanes:` line - [ ] **C. Fetch ALL comments from ALL sources:** read every update on the PR regardless of author or format. Three API surfaces + reviews: - [ ] C1. Review-thread comments: `gh api --paginate "repos///pulls//comments?per_page=100"` - [ ] C2. Issue-level comments: `gh api --paginate "repos///issues//comments?per_page=100"` (includes AI-review summaries, user replies, bot task-completion posts) diff --git a/plugins/source-control/skills/pull-request/reference/monitor.md b/plugins/source-control/skills/pull-request/reference/monitor.md index 31a5960d4..4bd4e6b68 100644 --- a/plugins/source-control/skills/pull-request/reference/monitor.md +++ b/plugins/source-control/skills/pull-request/reference/monitor.md @@ -407,6 +407,7 @@ When all readiness gates pass: **Check runs:** X passed, Y skipped, Z failed-informational **Security:** [scanner] evaluated, N findings classified **Comments:** X from N reviewers, Y fixed, Z deferred, W incorrect +**Review lanes:** [each lane on the checks roster: productive, or ABSENT with what was run locally in its place] **Cooldown:** 2+ min since last activity **Fix iterations:** N **Failures classified:** diff --git a/plugins/source-control/skills/pull-request/reference/readiness.md b/plugins/source-control/skills/pull-request/reference/readiness.md index 9049bedd3..22a411a25 100644 --- a/plugins/source-control/skills/pull-request/reference/readiness.md +++ b/plugins/source-control/skills/pull-request/reference/readiness.md @@ -157,7 +157,9 @@ gh api --paginate "repos/{owner}/{repo}/issues//comments?per_page=100 `commit_id` is the wrong field for this question: it re-anchors to the newest head while a comment's hunk still applies, so it counts surviving prior-round comments as current and short-circuits the wait. Unpaginated, the count also undercounts: the comments you are waiting on are the newest, and on a PR with prior review rounds the newest are exactly what page 1 omits. The count is slurped rather than passed to `--jq` for the reason rule 3 gives: a reduction like `length` inside `--jq` runs per page and prints one number per page, never the total. -- [ ] **A review lane that produced nothing for this round is SUBSTITUTED before the gate clears, not merely reported.** Its own check row cannot settle this: these lanes report on their session rather than on their output, so a session that ends without error is green whether or not it reviewed anything, and cost is no signal either since the session is billed either way. Judge the lane by the artifacts the scoping rule above attributes to this head; zero of them means nothing was reviewed, however green the row. Run a local review over the same diff (`/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for a correctness lane, whichever resolves in this session), then record in the readiness verdict which lanes were absent and what stood in for them. A lane whose posted body admits it fell back to a manual pass counts as absent on the same terms. This is what the bound above hands off to: reaching the bound ends the wait, it does not supply the review. + +- [ ] **A review lane that produced nothing for this round is SUBSTITUTED before the gate clears, not merely reported.** Take the roster for this item from `gh pr checks`, NOT from the bot authors the reviewer roster above is built from: a lane that posted nothing is missing from every author-derived roster by construction, so that roster would apply this item to an empty set, which is exactly the lane this item exists for. Its own check row cannot settle it either: these lanes report on their session rather than on their output, so a session that ends without error is green whether or not it reviewed anything, and cost is no signal since the session is billed either way. Judge the lane by the artifacts the scoping rule above attributes to this head, bounding a rerun of the same head by timestamp against the run's start, since a rerun's artifacts carry the same SHA; zero attributed artifacts means nothing was reviewed, however green the row. Run a local review over the same diff (`/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for a correctness lane, whichever resolves in this session), then name every absent lane and its substitute on the verdict's `Review lanes:` line. A lane whose posted body admits it fell back to a manual pass counts as absent on the same terms. This is what the bound above hands off to: reaching the bound ends the wait, it does not supply the review. + ### Gate 6: No pending work @@ -178,6 +180,7 @@ Only when ALL gates pass, present: **Comments:** X from N reviewers: Y fixed, Z deferred, W incorrect **Cooldown:** 2+ min since last activity **Reviewers:** [each discovered reviewer: responded, no-findings signal, or not yet responded at the bound with its missing artifacts named] +**Review lanes:** [each lane on the checks roster: productive, or ABSENT with what was run locally in its place] **Failures classified:** - `review`: FAILURE, usage limit (informational, safe to proceed) - [any other failures with classification] diff --git a/scripts/contract-clause-registry.json b/scripts/contract-clause-registry.json index 372e3db93..cdc899a5e 100644 --- a/scripts/contract-clause-registry.json +++ b/scripts/contract-clause-registry.json @@ -173,6 +173,35 @@ "hint": "carry the self-containment bullet with its three parts: no external requests, no remote scripts, assets inline" } ] + }, + { + "id": "B2-lane-productivity", + "title": "A review lane is judged by what it produced for this round, and an absent one is substituted", + "canonical": "plugins/source-control/skills/pull-request/reference/readiness.md", + "detect": "review.?lane productivity|produced nothing for this round|\\bB2\\b", + "restates": "(ABSENT|absent)[^.]{0,60}(never PASS|not PASS|rather than PASS)|produced nothing for this round", + "qualifiers": [ + { + "name": "roster-from-checks", + "pattern": "gh pr checks|checks roster", + "hint": "take the lane roster from the checks list, never from the authors who posted: a lane that posted nothing is missing from every author-derived roster by construction" + }, + { + "name": "scoped-to-this-round", + "pattern": "current round|original_commit_id|this head", + "hint": "scope productivity to the head under review, so an artifact from an earlier head cannot satisfy the check" + }, + { + "name": "substituted-not-just-reported", + "pattern": "/review:fanout|`/code-review`|local review", + "hint": "run a local review over the same diff; reporting the lane as absent without substituting leaves the change unreviewed" + }, + { + "name": "substitution-reported", + "pattern": "Review lanes:|stood in for|what was run locally", + "hint": "name the absent lane and its substitute in the verdict, or the gap is dropped from a report that otherwise reads all-clear" + } + ] } ] }