fix(review-remote-pr): triage CodeRabbit findings posted as issue comments - #575
Merged
Conversation
…review-ledger.sh cover A one-shot adversarial receipt writes covered_heads exactly once, at publish time. Any later fix, merge-down, or retarget makes review-ledger.sh status report stale, and the one-spend rule forbids re-reviewing to clear it -- deadlocking green, reviewed PRs (issue #567, evidence from #552/#555/#558/#562). Add a `cover` subcommand that appends {sha, reason, covered_at} onto the latest matching entry after proving the new head is a git descendant of the reviewed head, wire a best-effort call site into chain-advance.sh retargets, and document the pr-to-green fix-batch/merge-down call sites so a later transition is recorded instead of falsified or parked. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
…er in review-ledger.sh cover Adversarial-review fix batch on PR #569 (four findings, root-accepted). F1: cmd_cover proved ancestry only against an entrys original head_sha, so a force-push that dropped an already-covered fix commit could still pass cover for a sibling child of that head_sha; ancestry is now proven against the entire covered frontier (head_sha plus every already-covered SHA). F2: idempotence keyed on the SHA alone, silently dropping the coverage event for the ordinary head-unchanged/base-changed retarget case; it now keys on the (sha, reason) pair. F3: chain-advance.sh unfiltered cover call could extend a non-adversarial (e.g. bot) entry instead of the receipt; it now passes --kind adversarial, documented at the auto-merge.md call site too. F4: the retarget lineage hooks gh api --paginate call emitted one bare JSON array per page instead of the single array review-ledger.sh requires; it now slurps and flattens with --slurp --jq add. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
…ged base branch merge-pr.sh --delete-branch deleted a merged head without checking for open PRs still based on it; GitHub closes such a dependent instead of retargeting when it is a draft or not cleanly mergeable, silently ending a chain (issue #564, evidence: PR #484, #561). --delete-branch now checks for open dependents first: by default it retargets each to the merge target before deleting (or, with --no-retarget, refuses the delete and names them); a post-delete safety net recovers any dependent GitHub still closed via a new chain-advance.sh --recover-closed helper. authorize-queue.sh additionally records a per-PR deleteBranch of "deferred" (from pr-queue.sh's new hasOpenSuccessor fact) when a confirmed predecessor still has an open successor in the same queue, so merge-pr.sh can refuse the delete from the authorization record alone. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
… --kind adversarial, paginate fix) # Conflicts: # tests/test-chain-advance.sh
…ments CodeRabbit and github-code-quality[bot] sometimes post a real finding (a "**P1 -- ...**" call-out, an "**Actionable**" block, or an "outside diff range" note) as a plain issue comment instead of a review or inline thread -- agent-kit PR #552s blocking P1 landed exactly that way and was never triaged. Add classify-issue-comment-findings.sh (list/count/mark-answered) to turn each matching block into a comment_id#index-keyed finding with surface=issue-comment; wire its open count into gh-pr-state.sh's digest (independent of GraphQL thread availability) and pr-loop-setups icf-open terminal signal; document the assess/reply/mark-answered cycle and the settle rule (a PR is not settled while any issue-comment finding is open) in provider-rules.md. Closes #566. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…r-closed cleanup/resume Adversarial-review fix batch for PR #572 (three root-accepted P2 findings). F1: merge-pr.sh raw-PATCH-retargeted open dependents before their own merge-down/ancestry proof, which can reintroduce the predecessor's diff into an unrebuilt dependent and leave its CI evidence stale after a squash merge; refusing the delete is now the default, and the PATCH retarget moves behind an explicit --retarget-dependents opt-in documented as safe only after the caller has already merged the updated default branch into each dependent and completed chain-advance.sh --retarget's proof for it. F2: chain-advance.sh --recover-closed deleted its temporary base ref unconditionally, even when this invocation only reused a pre-existing one; it now tracks whether this run created the ref and re-verifies the ref still points at the recreated SHA before ever deleting it. F3: a partial recovery that reopened the PR but failed before retargeting left it permanently refused on retry; recover-closed now recognises that verified state (open, live base ref still at the recorded base SHA) and resumes at the retarget step instead of refusing. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
Adversarial review of the issue-comment finding classifier (agent-kit#566) found four accepted P2s: F1: pr-loop-setup called classify-issue-comment-findings.sh count with no --answered, so any finding a prior cycle already answered read open forever. Define one durable per-PR answered-ledger path ($RUN_DIR/state/pr_<N>_issue_comment_answered.ndjson) and thread it through every count/list/mark-answered call in the template and prose. F2: "**Actionable comments posted: 0**" -- CodeRabbits own zero-findings summary -- classified as a finding. Parse the count and only emit a finding for N>0; a bare "**Actionable**" block elsewhere in the comment is matched separately so real blocks are never dropped. F3: a priority call-out silently suppressed every other finding in the same comment (an exclusive if/else). Collect all three finding shapes independently per comment, then dedupe only overlapping/identical header text -- genuinely distinct findings in one comment now all survive. F4: identity was comment_id#index alone, so a bot editing a comment at the same index would read as already answered. Add a sha256 content fingerprint (kind+priority+header) to every finding; list/count/mark-answered now require id AND fingerprint to match before treating a finding as answered -- an edited comment correctly reopens. Tests: zero-count summary (F2), mixed priority+actionable comment (F3), an overlap case verifying dedup still collapses truly identical blocks, and an edited-comment-same-index case proving the finding reopens (F4) and that re-answering it under its new fingerprint is a fresh ledger record. Also hardened a related gap found in review: a missing sha256sum/shasum now fails closed with a diagnosed evidence-unavailable message instead of an unexplained set -e abort mid-fingerprint. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
Collaborator
Author
|
This was written agentically; verify its assertions: Adversarial review receipt
🤖 Co-authored by Claude Fable 5 (root). |
Collaborator
Author
|
This was written agentically; verify its assertions: Review ledgerMachine-readable record of every review already performed on this PR. {
"version": 1,
"pr": 575,
"repo": "wrzonance/agent-kit",
"reviews": [
{
"kind": "adversarial",
"provider": "openai",
"model": "gpt-5.6-sol",
"effort": "xhigh",
"mode": "cross-provider",
"harness": "claude",
"head_sha": "09753b1544888ce634370ccc962d08a1256beee9",
"covered_heads": [
"09753b1544888ce634370ccc962d08a1256beee9",
"15965d2194ddb316d89170f824fe732bf79feca8",
"a4d1a2a20e15e92ca052c7c581b1eb290a9bf755"
],
"diff_payload": "wrzonance/agent-kit:575:cac002d018a0354eaeca3f4cd16f9e5dccb8996dccec99c18f3bf030a49bee3b",
"counts": {
"p1": 0,
"p2": 4
},
"reviewed_at": "2026-08-30T07:21:18Z",
"coverage": [
{
"sha": "15965d2194ddb316d89170f824fe732bf79feca8",
"reason": "merge-down:ed78b46669c85b94f9b3b1525d7ed20797d4a0d2",
"covered_at": "2026-08-30T07:43:14Z"
},
{
"sha": "a4d1a2a20e15e92ca052c7c581b1eb290a9bf755",
"reason": "merge-down:3d82486e8057f8fcbf5a10800f5b051cec7ca972",
"covered_at": "2026-08-30T07:54:31Z"
}
]
}
]
}🤖 Co-authored by Claude Fable 5 (root). |
thewrz
marked this pull request as ready for review
August 30, 2026 07:54
thewrz
pushed a commit
that referenced
this pull request
Aug 30, 2026
…cks stop growing wait-ci settled the instant zero checks were pending, so a repo whose four-check matrix registers one workflow at a time (agent-kit#571/#574/#575) reported ci=1/4 as done before the other three checks even existed, costing the caller a second wait and a false red at the merge gate. Settling now requires the registered check count to be identical across two consecutive rounds with nothing queued/in_progress, plus an optional --expect-checks floor (defaulted from the base branch's own head) that refuses to settle below it before the round budget is exhausted. Closes #578. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
4 tasks
thewrz
added a commit
that referenced
this pull request
Aug 30, 2026
… the first zero-pending snapshot (#580) * fix(review-remote-pr): gh-pr-state.sh --wait-ci settles only once checks stop growing wait-ci settled the instant zero checks were pending, so a repo whose four-check matrix registers one workflow at a time (agent-kit#571/#574/#575) reported ci=1/4 as done before the other three checks even existed, costing the caller a second wait and a false red at the merge gate. Settling now requires the registered check count to be identical across two consecutive rounds with nothing queued/in_progress, plus an optional --expect-checks floor (defaulted from the base branch's own head) that refuses to settle below it before the round budget is exhausted. Closes #578. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com> * fix(review-remote-pr): honor --expect-checks floor and stop inferring a default gh-pr-state.sh --wait-ci settled ci=0/0 as none-configured after the zero-check grace window even when an --expect-checks floor was set and real checks were still about to register, hiding the exact evidence the floor exists to protect (F1). Separately, the default floor auto-resolved from ALL of the base branchs completed check-runs over-counted push-only workflows (e.g. deploy), so a PR with one green check could never settle and would burn its whole --rounds budget (F2). Keep polling past the grace window whenever a floor is set, report the shortfall instead of settling early when the round budget runs out, and drop the auto-resolved default entirely so --expect-checks only ever reflects what the caller explicitly asked for. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com> --------- Co-authored-by: mergetest <merge@test.local> Co-authored-by: Claude claude-sonnet-5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was written agentically; verify its assertions:
Why
pr-to-green triage read CodeRabbit reviews and inline threads only. CodeRabbit also posts findings as plain issue comments — chat replies,
@user I found one blocking issue, "outside diff range" notes — and those never entered triage: PR #552 (2026-08-28) was declared settled with an unaddressed P1 sitting in its conversation tab, and the operator reported the same miss on #555/#558.Stacked on #572 — merge that PR first. Agent-driven merges run
chain-advance.sh --retarget --pr <this-PR> --base mainand require its full proof before merging; interactive human merges may merge then delete the predecessor branch for GitHub's automatic retarget.What
review-remote-pr/scripts/classify-issue-comment-findings.sh(list/count/mark-answered): one finding per**P[0-9] —**/**Actionable**/ "outside diff range" block incoderabbitai[bot]orgithub-code-quality[bot]issue comments, keyedcomment_id#index,surface=issue-comment; a local append-only answered-ledger since no thread exists to resolve.gh-pr-state.shdigest gains an unconditionalissue-comment-findings: N openline (REST evidence, independent of GraphQL thread capability) and--issue-comment-answered FILE.icf-open: N.Decisions
finding-ledger.sh/review-ledger.shuntouched on purpose: both track different concepts (adversarial verdicts; review coverage); issue-comment answered-state got its own small ledger.base=26ee61ba6b76302681bf83ff80ce1e0168e63206
files=7
total.insertions=572
total.deletions=4
total.lines=576
operational.files=7
operational.insertions=572
operational.deletions=4
operational.lines=576
generated.files=0
generated.insertions=0
generated.deletions=0
generated.lines=0
lockfile.files=0
lockfile.insertions=0
lockfile.deletions=0
lockfile.lines=0
fixture.files=0
fixture.insertions=0
fixture.deletions=0
fixture.lines=0
non_operational.files=0
non_operational.insertions=0
non_operational.deletions=0
non_operational.lines=0
Testing
tests/test-classify-issue-comment-findings.sh(27) and extendedtests/test-gh-pr-state.shgreenagent-run.sh --cmd testALL GREEN on the final tree (.agent/logs/20260830T064513Z-test.log)🤖 Co-authored by Claude Fable 5 (root) / claude-sonnet-5 high (worker).
Closes #566