feat(#106): reconcile findings maintainers have already declined - #709
feat(#106): reconcile findings maintainers have already declined#709ralphbean wants to merge 2 commits into
Conversation
Adds an experimental step to the pr-review orchestrator: on re-review, fetch reply threads on the agent's own prior inline findings and check whether a repo maintainer explicitly declined one as out of scope. Declines are trusted only from users with OWNER/MEMBER/COLLABORATOR association (or write+ via the collaborator permission API) — the same tier .github/scripts/check-e2e-authorization.sh already uses — so an untrusted commenter cannot forge a dismissal to suppress a real finding. Matched, unchanged findings are downgraded to info with a note rather than dropped, so they stay visible without re-inflating the verdict on every push. This narrows one specific case of #106 (declined-via-reply) and is explicitly marked experimental in the skill doc — it doesn't address findings dropped silently or non-reply dismissals. Intended as a first iteration to observe before deciding whether to expand it. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoHonor maintainer-declined prior findings on re-review (experimental)
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 9 findings from an independent review pass (1 critical, 2 high, 6 medium), verified against live GitHub API data and the PR's own cited motivating example (agents PR #90). None overlap in substance with the existing qodo-code-review bot comments on this PR (protected-path, decline-key category collision, unbounded comments fetch) — see inline comments for details.
| silently truncates file lists at 300 files), treat all files as | ||
| changed — no anchoring for this run. | ||
|
|
||
| ### 2a-1. Trusted dismissals of prior findings (re-reviews) |
There was a problem hiding this comment.
MEDIUM: Experimental decline-detection heuristics merge into the live pipeline with the PR's own manual-verification checkboxes unchecked
The PR's test plan states SKILL.md has no automated test harness and lists two unchecked, manual-only items: "Observe behavior on a real PR where a maintainer declines a finding via reply, across 2+ subsequent pushes..." and "Confirm a decline reply from a non-maintainer... is not honored." Both boxes remain unchecked on the current PR body. This step merges directly into SKILL.md, which every future pr-review invocation on every repo executes live, with no feature flag or dry-run gate beyond the "experimental" prose label. Notably, the first unchecked item -- observing behavior on a real PR -- is exactly the kind of manual check that would have surfaced the performed_via_github_app bug identified in this same review before merge.
Suggestion: hold this PR until at least one real-world round-trip is observed and documented (which would likely have caught the performed_via_github_app issue), or land it behind an explicit opt-in/kill-switch so a bad decline-classification can be disabled without reverting the whole change.
|
Marking this as draft until fullsend-ai/fullsend#6045 is resolved. |
The trusted-decline mechanism from 5393b60 never activated: the pulls/comments endpoint doesn't populate performed_via_github_app, so the app-identity gate never matched and DECLINED_FINDINGS stayed empty. Switch the gate to user.login/user.type, which the endpoint actually returns (verified against this repo's own review comments). Filed fullsend-ai/fullsend#6045 to expose the harness slug as a FULLSEND_SLUG env var so this can stop hardcoding the app's identity. Also, since exact file/line matching plus a file-level changed-since-prior guard couldn't survive the line drift a declined finding's location goes through across rounds (the motivating PR #90 case this feature targets), switch decline matching to file + category + presence of the actual declined code, dropping the round-boundary check entirely — a decline now expires exactly when its code changes, not when some unrelated line in the file does. Smaller fixes bundled in: - exclude the PR author from qualifying as a decliner of their own findings - most recent qualifying reply in a thread wins, so a maintainer can reverse an earlier decline - fall back to original_line/original_start_line when GitHub nulls line on outdated comments - reword the author_association fallback to match what check-e2e-authorization.sh actually documents (defense-in-depth for membership-visibility edge cases, not an API bug) - project the review-comments fetch to needed fields and cache collaborator-permission lookups per login Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:13 PM UTC · Completed 3:34 PM UTC Commit: |
|
These three are the same items flagged in the inline threads — replied and resolved there (protected-path warning is intentional; decline-key and unbounded-fetch are fixed in 7e98ad7). |
ReviewFindingsMedium
Low
Labels: PR modifies the review agent skill (skills/pr-review/SKILL.md) to add trusted-dismissal behavior. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| For each thread whose most recent qualifying reply declines the | ||
| finding, resolve the root comment's location — falling back to | ||
| `original_line`/`original_start_line` when `line`/`start_line` are null |
There was a problem hiding this comment.
[medium] logic-error
Step 2a-1 matches dismissed comment threads to prior findings using 'same file, same line, matching category/description,' but step 6e's declined-findings check deliberately avoids line-number matching ('not line number, since a finding's line can drift'). This asymmetry means line drift at the 2a-1 stage can prevent a finding from entering DECLINED_FINDINGS, so the resilient file+category match in 6e never fires. The failure mode is conservative (findings re-raised rather than suppressed), but the feature will silently fail to recognize valid declines in common rebase/amend scenarios.
Suggested fix: Relax the 2a-1 cross-reference to match on file + category + description similarity (consistent with step 6e), or document the line-drift gap as a known limitation.
| @@ -1052,6 +1136,46 @@ attention. | |||
| If no protected files are modified, do not add a `protected-path` | |||
There was a problem hiding this comment.
[low] edge-case
If a maintainer declines a protected-path finding, the declined-findings mechanism would downgrade it to info severity. The existing 'never approve with protected-path findings' constraint independently prevents an approve outcome, so no schema violation occurs, but a clarifying note would avoid reader confusion about this interaction.
| `id`, built from `REVIEW_COMMENTS`) until reaching a comment with no | ||
| `in_reply_to_id`. Skip the thread unless the root comment's `login` is | ||
| `fullsend-ai-review[bot]` and `user_type` is `Bot`. (The | ||
| `pulls/.../comments` endpoint does not populate |
There was a problem hiding this comment.
[low] rbac-violation
The hardcoded bot identity check (fullsend-ai-review[bot]) is fragile if the harness slug changes. The PR acknowledges this limitation and links to fullsend#6045 for a fix via FULLSEND_SLUG env var. Failure mode is fail-closed.
| silently truncates file lists at 300 files), treat all files as | ||
| changed — no anchoring for this run. | ||
|
|
||
| ### 2a-1. Trusted dismissals of prior findings (re-reviews) |
There was a problem hiding this comment.
[low] pattern-inconsistency
Issue references use Markdown hyperlinks while the existing file consistently uses plain GitHub shorthand references (e.g., fullsend-ai/fullsend#2096). Consider using the established shorthand format.
Suggested fix: Use plain-text shorthand format: #106 and fullsend-ai/fullsend#6045.
| finding history to check dismissals against). | ||
|
|
||
| Fetch the PR's inline review comment threads, projecting to only the | ||
| fields this step needs (the unfiltered payload can get large on |
There was a problem hiding this comment.
[low] edge-case
The --paginate loop on the gh api call has no upper bound or timeout. PRs with very many review comments could cause excessive API calls. Failure mode is fail-closed (no dismissals applied).
| var so this can reference that instead of a literal string.) | ||
|
|
||
| **Trust boundary — only maintainer replies from someone other than the | ||
| PR author count as a dismissal.** A reply only qualifies when the |
There was a problem hiding this comment.
[low] rbac-violation
The PR author exclusion relies on PR_META.user.login, which reflects the current author. In the rare case of PR authorship transfer, the exclusion may not cover the original author. Extremely narrow edge case with no practical attack vector.
|
|
||
| **Trust boundary — only maintainer replies from someone other than the | ||
| PR author count as a dismissal.** A reply only qualifies when the | ||
| replying user is not the PR author (`PR_META.user.login`, step 2), and |
There was a problem hiding this comment.
[low] edge-case
Bot-authored PRs (renovate, dependabot) are excluded from self-dismissing findings by the author exclusion rule. A human maintainer must dismiss on their behalf. This is a safe default.
fullsend-ai-review
Bot
added
requires-manual-review
|
Need to wait for the next release after 0.35.1 before I can use FULLSEND_SLUG and FULLSEND_ROLE. |
…issed Picks up ralphbean's fullsend-ai#709 at his invitation on the review-economy thread ("please take over ... or start it from scratch and we can discard mine"). The design is his; this keeps it, closes three of the gaps its own non-goals list, and moves the forge calls where they now belong. Kept from fullsend-ai#709, unchanged in substance: the two-tier trust gate (author_association in OWNER/MEMBER/COLLABORATOR, falling back to the collaborator permission API for admin/maintain/write, the same pair check-e2e-authorization.sh uses); the PR author never dismissing their own findings even holding a qualifying role; keying the dismissal to whether the dismissed code is still present rather than to a round boundary; matching on file + category, never line; downgrading to info + actionable:false rather than dropping; and the experimental framing with explicit non-goals. Split across the forge boundary. fullsend-ai#709 puts a `gh api` call in the shared SKILL.md. That skill is now forge-abstracted — it delegates every fetch to "the forge-specific review skill's <section>" and pr-review/github and pr-review/gitlab supply the commands — so the dismissal fetch follows suit: the shared skill carries the semantics, github/SKILL.md carries the GraphQL query, and step 2a-1 skips when a forge has no such section. That keeps GitLab on today's behavior instead of breaking it. What this adds: 1. Non-reply dismissals. fullsend-ai#709's non-goals name these; they are the two things people actually reach for when a finding is not worth a sentence. Resolving the conversation and a thumbs-down on the bot's comment now count, and both are trust-gated exactly like a reply. One GraphQL reviewThreads query replaces the paginated REST call and carries all three signals: resolution is GraphQL-only, reactions cost a request per comment over REST, and threads arrive pre-grouped, so the in_reply_to_id chain walk goes away with it. 2. A critical carve-out. As fullsend-ai#709 stands, any severity downgrades to info, critical included, and stays there for as long as the code is unchanged — which is the case where it should stay unchanged. Critical findings are now emitted at critical with the dismissal noted alongside. info + actionable:false resolves to `approve` in 6f, and that is the one outcome a critical finding must not produce. Refutation still downgrades a critical finding, because that is a verified judgment about the code rather than a dismissal of it. 3. Disputes, engaged exactly once. "This isn't a bug, because X" is not a decline, so under fullsend-ai#709 it falls through and the finding re-raises verbatim next push — fullsend-ai#106 wearing a different hat. The argument is judged on its merits and is deliberately not trust-gated (the PR author is usually the one making it, and correctness is not a permission). Refuted, it downgrades; not refuted, the finding stands with one sentence engaging it, and the exchange is over. 4. A fail-closed trust boundary, because the collaborator-permission fallback does not work from where this runs. GitHub rejects that endpoint without push access ("Must have push access to view collaborator permission"), and the review agent is deliberately read-only — readonly_repo: true, providers/github-ro.yaml, and a policy whose own comment says "No write access to GitHub". So the second tier generally 403s in the sandbox. Any error is treated as not trusted: the dismissal does not count and the finding is emitted normally. That has a consequence worth stating rather than leaving to be discovered: on a private organization, where a real admin's association reports as CONTRIBUTOR, tier one under-reports and tier two cannot compensate. Rather than leave the new signals dead on arrival — resolvers and reactors carry no association at all — a middle tier looks the login up among the associations the same query already returned for this PR's thread comments, which covers the common case at no extra request. Closing the gap properly means resolving trust on the runner, where a write-scoped token exists, and passing the result in; that is a separate change. Two corrections to fullsend-ai#709's text, both verified rather than assumed: - fullsend#6045 has shipped, so the review app's identity no longer has to be a literal. FULLSEND_SLUG is exported into the sandbox from the harness identity and is in reservedSandboxKeys so env.sandbox cannot shadow it. The configured login stays only as the fallback for a harness that declares no slug. - The bot's login has two spellings and the query returns both at once. GraphQL reports a Bot-typed author without the [bot] suffix — the form FULLSEND_SLUG holds, so it compares directly — while REST's user.login and a bot appearing under resolvedBy (typed User, not Bot) both carry it. fullsend#6456 corrected this same mismatch in another skill. Verified against live data rather than from the schema: every field in the query — isResolved, resolvedBy, authorAssociation, diffHunk, reactionGroups.reactors, the pageInfo flags — was run against real review threads on fullsend-ai#1003, and the snippet was executed exactly as it appears in github/SKILL.md (exit 0). Three behaviours worth knowing came out of that and are documented next to the query: reviewThreads returns oldest-first, so it uses last: 100 while comments within a thread stay first: 50 so nodes[0] is the root; `line` comes back null with originalLine set once a comment's diff position goes stale, which on a re-review is the common case; and reactionGroups returns all eight contents even at zero, so totalCount must be checked before reading reactors. Not included, and neither omission is a shortcut: An eval case is blocked twice over. eval/review/cases/*/input.yaml expresses only forge, seed_issues and fixture — there is no way to seed a prior review, review threads, replies, resolution or reactions, and the case lifecycle is a single agent run. Even given that, the runner reuses GH_TOKEN as REVIEW_TOKEN, so a seeded review comment would carry no performed_via_github_app.client_id, PRIOR_REVIEW_PROVENANCE would be unverifiable-no-app, and step 2a-1 would skip by design. That is agents#245. Worth revisiting when fullsend-ai#245 lands, because this is behaviour that will rot silently. GitLab parity is left explicitly unimplemented rather than guessed. The signals exist there — discussions carry resolved and resolved_by, award_emoji carries the reaction — but the trust boundary has no verified field mapping and I have no live instance to check one against. That is the part that must not be approximated, so gitlab/SKILL.md says so and step 2a-1 skips when a forge provides no section. Signed-off-by: guy oron <goron@redhat.com>
…iliation Five of the eight findings on this PR were real. Taking them in order of how badly they broke things. The GitLab guard did not guard. Step 2a-1 said it skips when the forge skill "has no Review thread dismissals section" — and then gitlab/SKILL.md was given a section with exactly that heading to explain that GitLab is unimplemented. So the check passed, and a GitLab re-review would have entered the mechanism looking for commands that are not there. The condition now keys on whether the forge supplies a fetch, not on whether a heading exists, and the GitLab section says in its first line that it supplies none. Self-inflicted in the forge split; the whole point of that split was that GitLab keeps working. Thread replies were truncated without anyone noticing. comments(first:50) has to stay `first` so nodes[0] is the root, but "the most recent qualifying reply wins" needs the tail, and on a thread past 50 comments the tail is exactly what is missing — including a reversal like "actually, let's fix this after all". The query now selects comments.pageInfo, and a thread whose own hasNextPage is true is treated as undetermined and dismisses nothing. Acting on a truncated view could have honored a dismissal the author had already withdrawn. File + category could silence a finding nobody dismissed. One file often holds several findings of one category; dismissing one of them matched all of them, because the "is the code still present" check asks about the file, not about the finding. A match now also requires the finding to be about the dismissed code — the recorded snippet covers its location, or it describes the same defect in the same construct — and anything that cannot be established that way is emitted normally. This one is inherited from fullsend-ai#709 rather than introduced here, but it is the failure that ends in a wrongly clean approval. Disputing a high-severity defect twice stopped it blocking. The engage-once rule downgraded anything below critical once the single exchange was spent, so a real high finding could stop blocking without ever being refuted — the ping-pong cure reintroducing the disease one notch down. The stop now ends the argument, not the finding: critical and high keep their severity however often they are disputed, only medium and below downgrade, and a reply that actually refutes is honored at any severity because refutation is judged on the code and is never used up. The no-slug fallback pointed at nothing. It said to fall back to "the configured review-app login" without naming a source, which is not actionable in the one case it exists for. Since there is no reliable way to tell this agent's threads from another bot's without the slug, that case now skips the step. harness/review.yaml declares slug: fullsend-ai-review, so it is always set here. Not changed, with reasons: - reactors is correct and `users` does not exist. Live introspection of ReactionGroup returns content, createdAt, reactors, subject, viewerHasReacted — no users field — and the query in this PR was run against real review threads for exactly this reason. Nothing to fix. - reviewThreads(last:100) and reactors(first:10) both truncate, and both fail closed: a dismissal that is not read is a dismissal not honored, and the finding is emitted. The pagination note already says a thread's absence must not be read as the absence of a dismissal. Fail-open truncation would deserve a fix; this does not. Signed-off-by: guy oron <goron@redhat.com>
…iliation Five of the eight findings on this PR were real. Taking them in order of how badly they broke things. The GitLab guard did not guard. Step 2a-1 said it skips when the forge skill "has no Review thread dismissals section" — and then gitlab/SKILL.md was given a section with exactly that heading to explain that GitLab is unimplemented. So the check passed, and a GitLab re-review would have entered the mechanism looking for commands that are not there. The condition now keys on whether the forge supplies a fetch, not on whether a heading exists, and the GitLab section says in its first line that it supplies none. Self-inflicted in the forge split; the whole point of that split was that GitLab keeps working. Thread replies were truncated without anyone noticing. comments(first:50) has to stay `first` so nodes[0] is the root, but "the most recent qualifying reply wins" needs the tail, and on a thread past 50 comments the tail is exactly what is missing — including a reversal like "actually, let's fix this after all". The query now selects comments.pageInfo, and a thread whose own hasNextPage is true is treated as undetermined and dismisses nothing. Acting on a truncated view could have honored a dismissal the author had already withdrawn. File + category could silence a finding nobody dismissed. One file often holds several findings of one category; dismissing one of them matched all of them, because the "is the code still present" check asks about the file, not about the finding. A match now also requires the finding to be about the dismissed code — the recorded snippet covers its location, or it describes the same defect in the same construct — and anything that cannot be established that way is emitted normally. This one is inherited from fullsend-ai#709 rather than introduced here, but it is the failure that ends in a wrongly clean approval. Disputing a high-severity defect twice stopped it blocking. The engage-once rule downgraded anything below critical once the single exchange was spent, so a real high finding could stop blocking without ever being refuted — the ping-pong cure reintroducing the disease one notch down. The stop now ends the argument, not the finding: critical and high keep their severity however often they are disputed, only medium and below downgrade, and a reply that actually refutes is honored at any severity because refutation is judged on the code and is never used up. The no-slug fallback pointed at nothing. It said to fall back to "the configured review-app login" without naming a source, which is not actionable in the one case it exists for. Since there is no reliable way to tell this agent's threads from another bot's without the slug, that case now skips the step. harness/review.yaml declares slug: fullsend-ai-review, so it is always set here. Not changed, with reasons: - reactors is correct; the finding has it backwards. `users` is the older ReactionGroup field, deprecated in favor of reactors — its own deprecation notice says "Use the `reactors` field instead" — and hidden from default introspection, which is likely where the confusion came from. The query as committed executes cleanly against this repo's own review threads, including the threads on this very PR. Nothing to fix. - reviewThreads(last:100) and reactors(first:10) both truncate, and both fail closed: a dismissal that is not read is a dismissal not honored, and the finding is emitted. The pagination note already says a thread's absence must not be read as the absence of a dismissal. Fail-open truncation would deserve a fix; this does not. Signed-off-by: guy oron <goron@redhat.com>
|
Heyyaa Ralph : ) Following up from the review thread — picked this up per your "take over #709 or start it from scratch": #1066. Your design is the spine, kept nearly intact: the two-tier trust gate, PR-author exclusion, decline keyed to the code still being present, file+category matching, downgrade instead of drop. Added on top: resolve-thread and 👎 count as dismissals (one GraphQL One heads-up before you review: the collaborator-permission fallback 403s inside the review sandbox — the endpoint needs push access and the review agent is read-only by design. #1066 fails closed on any lookup error and recovers the common case from associations the same query already returned. Closing it fully needs a second, narrowly-scoped token role plus a runner-side lookup that passes the result in — an infra ask; happy to write the code once the role exists. Two small text corrections along the way: fullsend#6045 shipped, so the bot identity reads Qodo and Wayne both did a round on #1066 already — everything addressed in-thread. If the direction looks right, could you or another maintainer drop |
4 participants
Summary
Addresses #106: the review agent re-raises a finding on every subsequent push even after a maintainer explicitly replied declining it (e.g. "pre-existing pattern, out of scope for this PR").
pr-revieworchestrator: on re-review, fetch reply threads on the agent's own prior inline findings viagh api pulls/{pr}/comments, and identify explicit declines.author_associationisOWNER/MEMBER/COLLABORATOR(falling back to the collaborator permission API for write+ access) — the same tier.github/scripts/check-e2e-authorization.shalready uses elsewhere in this repo. Replies from anyone else, including a PR author without maintainer status, are inert. This is specifically to prevent an untrusted commenter from forging a "not a bug, dismissing this" reply to suppress a real finding.infoseverity with a note rather than dropped — stays visible for transparency, doesn't re-inflate the verdict, and reverts to full re-evaluation the moment the code changes.This is explicitly marked experimental in the skill doc. It only covers the "declined via an explicit reply" case from #106 — not findings silently dropped across rounds, not self-contradictory reconciliation, not non-reply dismissals (e.g. a
wontfixlabel). The intent is to try this narrower behavior, see whether it actually improves the review experience in practice, and use that to decide whether/how to broaden it. #106 should stay open until the fuller picture is addressed.Test plan
SKILL.mdis a prompt document with no automated test harness (unlikescripts/*.sh, which have*-test.shcounterparts), so this can't be exercised with a unit test.pre-commit run --files skills/pr-review/SKILL.mdpasses🤖 Generated with Claude Code