-
Notifications
You must be signed in to change notification settings - Fork 0
fix(source-control): D6 verifies commit presence, not the branch tip; per_page=100 on every paginated read #2262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,9 +163,9 @@ When a channel event, Monitor notification, or poll iteration fires, complete AL | |
| - [ ] **A — Terminal state:** `gh pr view <N> --json state -q .state` — MERGED/CLOSED → self-terminate | ||
| - [ ] **B — CI checks:** `gh pr checks <N>` — 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/<owner>/<repo>/pulls/<N>/comments --paginate` | ||
| - [ ] C2 — Issue-level comments: `gh api repos/<owner>/<repo>/issues/<N>/comments --paginate` (includes AI-review summaries, user replies, bot task-completion posts) | ||
| - [ ] C3 — PR reviews: `gh api repos/<owner>/<repo>/pulls/<N>/reviews --paginate` (review bodies contain findings — APPROVED/CHANGES_REQUESTED/COMMENTED reviews all may carry actionable content) | ||
| - [ ] C1 — Review-thread comments: `gh api --paginate "repos/<owner>/<repo>/pulls/<N>/comments?per_page=100"` | ||
| - [ ] C2 — Issue-level comments: `gh api --paginate "repos/<owner>/<repo>/issues/<N>/comments?per_page=100"` (includes AI-review summaries, user replies, bot task-completion posts) | ||
| - [ ] C3 — PR reviews: `gh api --paginate "repos/<owner>/<repo>/pulls/<N>/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/<owner>/<repo>/pulls/<N>/comments/<id>/replies`; issue-level / review-level → `gh pr comment <N>`. 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/<owner>/<repo>/pulls/<N>/comments?per_page=100" --jq '.[] | select(.in_reply_to_id == <original-id>)'`; issue-level replies — `gh api --paginate "repos/<owner>/<repo>/issues/<N>/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 <files>`, commit, push | ||
| - [ ] **Verify commit pushed:** `gh api "repos/<owner>/<repo>/commits?sha=<branch>&per_page=1" --jq '.[0].sha'` — confirm the fix commit SHA appears on the remote | ||
| - [ ] **Verify commit pushed:** `git fetch origin <branch> && git merge-base --is-ancestor <fix-sha> origin/<branch>` — 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=<branch>&per_page=1` + `.[0]`), which any later push turns into a false "missing", and never a repository-scoped `commits/<fix-sha>` lookup alone, which can pass when the commit was force-pushed off the PR branch | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the checkout uses the skill's supported non- Useful? React with 👍 / 👎. |
||
| - [ ] D7 — Post a follow-up reply citing the fix commit SHA | ||
| - [ ] **Verify follow-up reply posted — same surface routing as D5:** inline thread → `pulls/<N>/comments` filtered by `in_reply_to_id`; issue-level → `gh api --paginate "repos/<owner>/<repo>/issues/<N>/comments?per_page=100" --jq '.[] | select((.body | contains("<sha>")) and .user.login == "<posting-identity>") | .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. `<posting-identity>` 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 <!-- contract-restatement: D7.5-thread-eligibility --> <!-- contract-restatement: D7.5-merge-authorization --> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"matching the reachability primitive
babysit-prsalready uses inverify_fix_commit" overstates the similarity.verify_fix_commit(skills/babysit-prs/scripts/babysit_resolve_thread.py:546) doesn't callgit merge-base --is-ancestor— it resolves the PR's liveheadRefOid/headRepositoryviagh pr view --jsonand then hits the GitHub compare API,repos/{owner}/{repo}/compare/{sha}...{head_oid}. That's a deliberately different implementation: it works cross-fork (the function's own docstring calls this out — "differs from the base repository on a fork PR") and needs no local clone/fetch, where D6'sgit fetch origin <branch> && git merge-base --is-ancestorrequires both.Both check the same property (is the fix commit an ancestor of the branch tip), so "matching reachability guarantee" would be accurate — but "matching...primitive" reads as "same mechanism," which isn't the case, and a future reader who goes looking for a shared implementation to consolidate on won't find one. Given this PR's own stated impetus (2 rounds of review already caught overstated guarantees on this exact D6 gate — Codex's P1 and the prior Claude inline comment on "cannot be satisfied by the wrong record"), this is worth tightening rather than shipping a third instance of the same overstatement class in the CHANGELOG.
Fix this →