From cdb72f21f32df9b369be8e4bda508f5c5ddbf9f6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:19:08 -0400 Subject: [PATCH] fix(source-control): D6 verifies commit presence, not the branch tip; per_page=100 on every paginated read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D6's verify-commit-pushed gate read only the branch tip (commits?sha=&per_page=1 + .[0].sha), answering "what is the tip?" instead of "is my fix commit on the remote?" — a false negative the moment anything is pushed after the fix, and a positional index on a list, which readiness.md's rule 2 forbids. Both sites (review- discipline.md D6 and pull-request SKILL.md D6) now use the single- resource form repos/{owner}/{repo}/commits/: SHA on exit 0 when present, HTTP 422 when absent. Verified live in both directions. Also adds per_page=100 to the six remaining --paginate sites in this plugin that lacked it (merge.md x3, SKILL.md C1-C3, monitor.md, fetch-all-pr-comments.sh, telemetry-upsert.md) per readiness.md rule 1. babysit_gh.py and request_review.py were swept as violations but are already conformant (per_page=100 on the adjacent line). Closes #2244 Co-Authored-By: Claude Opus 5 (1M context) --- .../source-control/.claude-plugin/plugin.json | 2 +- plugins/source-control/CHANGELOG.md | 27 +++++++++++++++++++ .../reference/review-discipline.md | 8 ++++-- .../scripts/fetch-all-pr-comments.sh | 2 +- .../reference/telemetry-upsert.md | 2 +- .../skills/pull-request/SKILL.md | 8 +++--- .../skills/pull-request/reference/merge.md | 6 ++--- .../skills/pull-request/reference/monitor.md | 2 +- 8 files changed, 44 insertions(+), 13 deletions(-) diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index f939d88df..0a0748e7b 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.51.11", + "version": "0.51.12", "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 — interview the repo and write 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 64f152bda..9af9fa0e5 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,33 @@ 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.51.12] + +### Fixed + +- **D6's verify-commit-pushed gate checks branch reachability, not repo-wide presence or the branch + tip** (`reference/review-discipline.md`, `skills/pull-request/SKILL.md`; #2244). The published + form — `commits?sha=&per_page=1` with `--jq '.[0].sha'` — asked "is my fix commit on the + remote?" but read only the branch tip, so any later push made it report the fix missing while it + was present: a false negative on a control gate, and a positional index on a list. A + repository-scoped `commits/` lookup fixed the tip-read false negative but still answered + "does this object exist anywhere in the repo?" — satisfied by a force-pushed-off commit or an + identical commit on another branch. The gate now fetches the PR branch and runs + `git merge-base --is-ancestor origin/` (exit 0 when the fix commit is on the + remote PR branch), matching the reachability primitive `babysit-prs` already uses in + `verify_fix_commit`. +- **Every remaining `--paginate` list read carries `per_page=100`**, conforming to rule 1 as + `readiness.md` publishes it (#2246): `skills/pull-request/reference/merge.md` (three + comment-source re-checks), `skills/pull-request/SKILL.md` (C1–C3), + `skills/pull-request/reference/monitor.md` (poll-loop comment fetch), + `scripts/fetch-all-pr-comments.sh` (the shared surface pager), and + `skills/babysit-loop/reference/telemetry-upsert.md` (sentinel LOOKUP). Not truncation defects — + `--paginate` alone fetches every page — but the default 30-per-page form costs 3.3x the + requests and diverges from the rule the same skill states as absolute. + `skills/babysit-prs/scripts/babysit_gh.py` and `scripts/request_review.py` were reported in the + #2246 sweep but were already conformant: their `per_page=100` sits in the endpoint URL on the + line adjacent to the `--paginate` flag the line-based sweep matched. + ## [0.51.11] ### Changed diff --git a/plugins/source-control/reference/review-discipline.md b/plugins/source-control/reference/review-discipline.md index 7e6c3a3e9..a87243010 100644 --- a/plugins/source-control/reference/review-discipline.md +++ b/plugins/source-control/reference/review-discipline.md @@ -230,8 +230,12 @@ D1–D7 cycles. Exploration and validation must run on the PR's head branch. since these endpoints return 30 per page oldest-first and your reply is the newest item - [ ] D6 — Fix if VALID (fix now) → edit, `git add ` (never `-A` or `.`), commit, push - - [ ] **verify commit pushed:** `gh api "repos/{owner}/{repo}/commits?sha=&per_page=1" - --jq '.[0].sha'` — confirm the fix commit SHA on the remote + - [ ] **verify commit pushed:** `git fetch origin && git merge-base --is-ancestor origin/` — + exit 0 means the fix commit is on the remote PR branch; non-zero means it is not. A reachability + check after fetch, never the branch-tip read (`commits?sha=&per_page=1` + `.[0]`), which + any later push turns into a false "missing", and never a repository-scoped `commits/` + lookup alone, which answers "does this object exist anywhere in the repo?" and can pass when the + commit was force-pushed off the PR branch - [ ] 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 diff --git a/plugins/source-control/scripts/fetch-all-pr-comments.sh b/plugins/source-control/scripts/fetch-all-pr-comments.sh index 3b4699afa..d0b5f5703 100755 --- a/plugins/source-control/scripts/fetch-all-pr-comments.sh +++ b/plugins/source-control/scripts/fetch-all-pr-comments.sh @@ -138,7 +138,7 @@ SURFACE_JSON="" fetch_surface() { local endpoint="$1" projection="$2" raw SURFACE_JSON="" - raw=$(gh api --paginate "repos/$OWNER/$REPO/$endpoint" 2>/dev/null) || { + raw=$(gh api --paginate "repos/$OWNER/$REPO/$endpoint?per_page=100" 2>/dev/null) || { printf 'fetch-all-pr-comments: gh api %s failed\n' "$endpoint" >&2 exit 2 } diff --git a/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md b/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md index ce9eb2ddc..b3fec6a1f 100644 --- a/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md +++ b/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md @@ -36,7 +36,7 @@ esac } MARKER="source-control:babysit-loop@$INSTANCE" SENT="" # $BODY_FILE MUST open with this line -LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments" \ +LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments?per_page=100" \ --jq ".[] | select(.body | startswith(\"$SENT\")) | .id"; } SENTINEL_OK() { # $1 = text; true iff line 1 is exactly $SENT and >=16 payload bytes follow [ "$(printf '%s' "$1" | head -c ${#SENT})" = "$SENT" ] && diff --git a/plugins/source-control/skills/pull-request/SKILL.md b/plugins/source-control/skills/pull-request/SKILL.md index bc99bf7b7..a12eeaf94 100644 --- a/plugins/source-control/skills/pull-request/SKILL.md +++ b/plugins/source-control/skills/pull-request/SKILL.md @@ -163,9 +163,9 @@ When a channel event, Monitor notification, or poll iteration fires, complete AL - [ ] **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 - [ ] **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 repos///pulls//comments --paginate` - - [ ] C2 — Issue-level comments: `gh api repos///issues//comments --paginate` (includes AI-review summaries, user replies, bot task-completion posts) - - [ ] C3 — PR reviews: `gh api repos///pulls//reviews --paginate` (review bodies contain findings — APPROVED/CHANGES_REQUESTED/COMMENTED reviews all may carry actionable content) + - [ ] 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) + - [ ] C3 — PR reviews: `gh api --paginate "repos///pulls//reviews?per_page=100"` (review bodies contain findings — APPROVED/CHANGES_REQUESTED/COMMENTED reviews all may carry actionable content) - [ ] C4 — Read every comment body in full. Summaries and review posts from ANY AI agent (claude[bot], codex, cursor, copilot) contain findings that require classification — these are NOT informational. **Extract individual findings** per [`${CLAUDE_PLUGIN_ROOT}/reference/review-discipline.md`](../../reference/review-discipline.md) §2 — one comment with N findings = N work items, each needing individual D1-D7. **For ≥3 findings, MANDATORY subagent dispatch** per the same §2 — preserves main session context, structurally enforces per-finding ledger shape - [ ] **D — For EACH unaddressed **finding** (not comment — one comment may contain multiple findings):** - [ ] D1 — Read full finding context (parent comment body + surrounding findings). For multi-finding comments dispatched to a subagent ([review-discipline](../../reference/review-discipline.md) §2), this work is in the subagent; the main session receives the ledger @@ -178,7 +178,7 @@ When a channel event, Monitor notification, or poll iteration fires, complete AL - [ ] D5 — Reply with a per-finding classification table + evidence (before fixing). **Route by comment type — REQUIRED, not interchangeable:** inline review comments MUST reply THREADED via `gh api repos///pulls//comments//replies`; issue-level / review-level → `gh pr comment `. Answering an inline finding with a detached `pr comment` is a routing error, not a style choice. Use the project's bot-identity wrapper for these writes when it has one; plain `gh` otherwise - [ ] **Verify reply exists — on the surface it was posted to:** inline threaded replies land on the review-comment surface — `gh api --paginate "repos///pulls//comments?per_page=100" --jq '.[] | select(.in_reply_to_id == )'`; issue-level replies — `gh api --paginate "repos///issues//comments?per_page=100" --jq '.[].body'`. Querying only issues/comments false-fails a correctly posted inline reply; so does dropping `--paginate`, since these endpoints return 30 per page oldest-first and your reply is the newest item - [ ] D6 — Fix if VALID (fix now) — edit, `git add `, commit, push - - [ ] **Verify commit pushed:** `gh api "repos///commits?sha=&per_page=1" --jq '.[0].sha'` — confirm the fix commit SHA appears on the remote + - [ ] **Verify commit pushed:** `git fetch origin && git merge-base --is-ancestor origin/` — exit 0 means the fix commit is on the remote PR branch; non-zero means it is not. A reachability check after fetch, never the branch-tip read (`commits?sha=&per_page=1` + `.[0]`), which any later push turns into a false "missing", and never a repository-scoped `commits/` lookup alone, which can pass when the commit was force-pushed off the PR branch - [ ] 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 diff --git a/plugins/source-control/skills/pull-request/reference/merge.md b/plugins/source-control/skills/pull-request/reference/merge.md index 31018e8eb..9835ebe2d 100644 --- a/plugins/source-control/skills/pull-request/reference/merge.md +++ b/plugins/source-control/skills/pull-request/reference/merge.md @@ -11,9 +11,9 @@ Resolve `` via `gh pr view --json number -q '.number'`. Pass it expli gh pr checks --json name,state,bucket # 2. Re-check for new comments since monitoring completed (all 3 sources, paginated) -gh api --paginate repos/{owner}/{repo}/pulls//reviews | jq -r '.[].user.login' -gh api --paginate repos/{owner}/{repo}/pulls//comments | jq -r '.[].user.login' -gh api --paginate repos/{owner}/{repo}/issues//comments | jq -r '.[].user.login' +gh api --paginate "repos/{owner}/{repo}/pulls//reviews?per_page=100" | jq -r '.[].user.login' +gh api --paginate "repos/{owner}/{repo}/pulls//comments?per_page=100" | jq -r '.[].user.login' +gh api --paginate "repos/{owner}/{repo}/issues//comments?per_page=100" | jq -r '.[].user.login' ``` **If any readiness gate fails on re-verification:** diff --git a/plugins/source-control/skills/pull-request/reference/monitor.md b/plugins/source-control/skills/pull-request/reference/monitor.md index 5f184a212..e613ea9b4 100644 --- a/plugins/source-control/skills/pull-request/reference/monitor.md +++ b/plugins/source-control/skills/pull-request/reference/monitor.md @@ -192,7 +192,7 @@ After each push, run this loop until convergence (**every** check in a terminal 1. **Mergeable pre-check (MANDATORY before polling)** — `gh pr view --json mergeable,mergeStateStatus` FIRST. If `mergeable == "CONFLICTING"`, GitHub will NOT trigger workflows — integrate the default branch, resolve conflicts, force-push with lease, and restart the loop. Only proceed to CI polling when `mergeable == "MERGEABLE"`. **Never blame the platform for missing CI runs before checking this.** 2. **Poll CI** — `gh pr checks ` every 30s (the standard monitor cadence), max 15 minutes per cycle. **Wait for ALL checks to reach a terminal state** (pass/fail/skipped) before suggesting merge — no exceptions, regardless of PR type. Never merge while any check is still pending or in_progress -3. **Check for new comments** — on each poll, also fetch new review comments (`gh api repos///pulls//comments --paginate`) +3. **Check for new comments** — on each poll, also fetch new review comments (`gh api --paginate "repos///pulls//comments?per_page=100"`) 4. **Process comments immediately** — if a bot comments while CI is still running, start evaluating/researching that comment now. Don't wait for CI 5. **On CI failure** — route to 3.2 (research-driven fix) 6. **On new comment** — route to 3.3 (evaluate + respond)