fix: make a resumed PR read its comments, not only its reviews - #631
Conversation
Reviews and conversation comments are two different feeds, and `gh pr view --json reviews` returns only the first. Step 0's rehydration list said "the diff itself, and any inline review comments" and never mentioned the conversation, so a session that followed it exactly still missed the thing that mattered most. That is not hypothetical. On #620 the maintainer posted "this PR should shrink" with a four-step plan: branch protection now rejects pushes to `main` server-side, so the protected-ref enumeration should be DELETED rather than extended. A later session read the reviews, did not read the comments, and spent a full rework ADDING protected-ref patterns — the exact opposite of a standing instruction sitting in the PR's own thread. Two further holes the maintainer had found by hand (`git push origin refs/tags/v1.2.3`, `git push origin HEAD`) were in that same comment and stayed open. The asymmetry is structural, not incidental: a review must attach to a diff, so direction — "do this differently", "this whole approach changed" — can only be expressed in a comment. That makes the feed the skill ignored the one carrying the highest-authority input. So: `comments` joins `reviews` in the Step 0 query; the rehydration list names the conversation explicitly and gives both commands; and a maintainer comment is stated to outrank every bot review on the PR, including later ones. Step 11 re-checks comments each round rather than only at Step 0, because a direction posted mid-loop is invisible to the verdict feed — the bot will keep approving a diff the maintainer has already asked you to redo. The CI-mode table gets the same note, where it matters more: Stage 3's re-trigger IS a comment, so the reason is usually in the same thread. Two Rationalizations rows and two Red Flags name the failure directly, since "I read the reviews, so I know what this PR needs" is exactly how it reads from the inside. No CHANGELOG entry: agent tooling, no user-visible effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Scope: Single file, .claude/skills/implement-issue/SKILL.md, 58 insertions / 2 deletions, prose-only (no code path). No linked GitHub issue — consistent with this skill's own stated norm that PRs for refactors/process fixes never had one.
Root cause verified against live data, not just trusted: I independently ran gh pr view 620 --json reviews,comments and confirmed the claim in the PR body: the maintainer's "this PR should shrink" direction comment (enforce_admins now true, delete the protected-ref enumeration, two more push-guard holes found by hand) exists only in the comments feed, not reviews. The described failure mode — a resumed session reading only --json reviews and doing the opposite of the maintainer's standing instruction — is accurately diagnosed.
Fix matches root cause: The added text (Step 0's gh pr view query now includes comments; a new "Reviews and conversation comments are two different feeds" block in the Rehydrate section with the two-command read-both pattern; a corresponding check inside Step 11's per-round verdict handling so a mid-loop comment isn't missed; matching CI-mode table note; two new Rationalizations rows and two new Red Flags entries) is redundant by design — the PR states this is deliberate, since no fixture pins prose-document adherence. Read through the whole file (/tmp/skill_head.md), the additions land in distinct, relevant sections without duplicating or contradicting existing content.
Test coverage: None added; the PR explicitly states this is N/A for a prose-only skill-doc change with no code path to redden. This matches precedent — PR #596 (.claude/settings.json, CLAUDE.md, docs/agents/rules.md, scripts/quality-check.sh, scripts/verify-sandbox.sh) is the same "agent tooling, no code path" shape and also shipped without a test or CHANGELOG entry. No CHANGELOG.md entry here either, and per CLAUDE.md that's correct only for changes with no user-visible effect — agent-instruction files qualify.
Minimality: No scope creep. Every hunk is directly in service of the stated fix; nothing unrelated was touched.
Checklist items not applicable: Optional[x]/hasattr/new classes/sensor access/convert_keys_to_camel_case/exception-string-matching/Black/Ruff/mypy — none apply, this is a markdown file.
No blockers found.
…e-argued (#636) * feat: put PRs on the board so a decision about one is recorded, not re-argued The board held issues only, so every judgement about a PR had nowhere to live. "#167 and #354 are blocked", "#437 and #490 are lower priority, later" were real decisions the maintainer had already made, and the rhythm pass re-reported all four as due on every tick because nothing recorded them. The same conversation happened every 30 minutes. Projects v2 takes PRs as items with the identical field set, so the fix is membership rather than a parallel mechanism. A PR card carries the same `Priority` and `Awaiting` an issue card does; the digest emits them as `pr_board`, and `backlog-rhythm.sh` joins by number and suppresses on them: an `Awaiting` means parked on someone, `P4` means later-not-never. `content.type` is what separates PR cards from issue ones, confirmed against a real card rather than assumed — an added PR reports "PullRequest" with number/title/url/repository alongside it. Numbers are unique across issues and PRs in one repository, so this cannot collide with the existing issue lookup. Suppressed PRs are COUNTED AND LISTED, never dropped: the pass ends with `deferred: 4 (#490 priority P4; #167 awaiting discussion; ...)`. Silently vanishing would trade one failure for another — the goal is to stop re-asking about a settled decision, not to lose the item. CONTRACT CHANGE: an APPROVED, green, still-draft PR is now its own action, `mark_ready`, and it is the one thing no board decision can defer. It used to hand back to `implement-issue` like any other unfinished draft, on the principle that this pass must not grow a second review loop. That principle still holds, but it is what left #629 sitting approved, green and draft: the remedy on offer was a whole `implement-issue` session, and nobody spends one of those to run a single command. `gh pr ready` is a terminal action, not a loop, so naming it here duplicates nothing. `awaiting_maintainer` is deliberately NOT carved out the same way. An approved PR waiting on a merge is not broken; it is the maintainers call when to take it, and P4 is exactly how they say later. #490 sat approved for a day and was reported every tick as though that were news. Live effect: 31 actions -> 27, with one `deferred: 4` line in place of four recurring items, and #631 correctly still reported as merge-ready. No CHANGELOG entry: agent tooling, no user-visible effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR * fix: mark_ready needs green checks, not just a clean merge Found by running the rule against the live fleet on its first pass, which is the only reason it was caught before the PR merged. `mergeable` reports whether the branch merges cleanly and nothing else, so it reads MERGEABLE while CI is still running or has failed outright. #633 was APPROVED, MERGEABLE and had Algorithm tests and E2E still IN_PROGRESS, and the rule duly reported "gh pr ready 633 — then it is the maintainers to merge". GitHub itself disagreed: `mergeStateStatus` was BLOCKED. Flipping a red or pending PR out of draft is worse than leaving it there. `ready` is supposed to mean the maintainer can merge without checking anything else, and that claim is the only thing making the flag worth setting. So `mark_ready` now also requires every check to have concluded SUCCESS, SKIPPED or NEUTRAL. SKIPPED is green on purpose: this repo path-filters Algorithm tests and Docker build, so every backend-only PR skips them and treating that as not-green would withhold the action from almost everything. An empty rollup is green too — a PR with no checks configured has nothing failing. The deferred list mirrors the same condition, so an approved-but-pending PR with a P4 card appears in exactly one place rather than both. Live effect: #633 falls back to resume_implementation until its CI settles, which is the correct answer and the one the first version got wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
implement-issueStep 0 now fetches a resumed PR's conversation comments alongside its reviews, and states that a maintainer comment outranks every bot review on the PR.Root cause
Reviews and conversation comments are two different feeds, and
gh pr view --json reviewsreturns only the first. Step 0's rehydration list read:It never mentioned the conversation. So a session following the skill exactly still missed the highest-authority input on the PR.
The asymmetry is structural rather than incidental: a review has to attach to a diff. "Do this differently", "branch protection changed, so this whole approach should shrink" — none of that can be expressed as a review, so it lands as a comment. The feed the skill ignored is the one carrying direction.
What it cost
On #620 the maintainer posted "this PR should shrink" with a four-step plan:
enforce_adminsis nowtrueon both remotes, so GitHub rejects pushes tomainserver-side and the protected-ref enumeration should be deleted rather than extended.A later session read the reviews, did not read the comments, and spent a full rework adding protected-ref patterns — the exact opposite of a standing instruction sitting in that PR's own thread. Two further holes the maintainer had already found by hand (
git push origin refs/tags/v1.2.3,git push origin HEAD) were named in the same comment and stayed open.The work was CI-green and internally verified. Correctness checks cannot catch this class of error, because the diff was not wrong — it was pointed the wrong way.
Fix
--json ...,reviews,comments, with a note not to drop itghcommands side by side, and the precedence ruleTest plan
./scripts/quality-check.shpasses locallygh pr view 620 --json reviewsreturns only the bot's review; the maintainer's direction comment appears solely under--json comments.Evidence the test discriminates
Not applicable — this is a prose change to a skill file, with no code path and nothing to redden. Stating that rather than filling the section with a suite result, per the skill's own instruction.
The empirical check that does discriminate is the one above: the two feeds were queried separately against #620 and confirmed to return different content, which is the whole premise of the change.
Outcome-level coverage
None, because this is a skill document. The behaviour it governs is a model's reading order, which no fixture pins. The mitigation is redundancy — the instruction now appears in the Step 0 query, the rehydration list, Step 11, the CI table, two Rationalizations rows and two Red Flags, so a session skimming any one section still meets it.
Scope
One file,
.claude/skills/implement-issue/SKILL.md. No CHANGELOG entry: agent tooling, no user-visible effect.🤖 Generated with Claude Code