feat(#106): reconcile findings that have already been dismissed - #1066
feat(#106): reconcile findings that have already been dismissed#1066guyoron1 wants to merge 3 commits into
Conversation
…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>
Functional tests are runningAuthorization passed for this commit. See the Functional Tests workflow for results. |
PR Summary by QodoReconcile dismissed findings during PR re-reviews
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| authorAssociation | ||
| body createdAt path diffHunk | ||
| line originalLine startLine originalStartLine | ||
| reactionGroups{ content reactors(first:10){ totalCount nodes{ ... on User { login } } } } |
There was a problem hiding this comment.
1. reactors invalidates graphql query 📜 Skill insight ≡ Correctness
GitHub's ReactionGroup exposes reacting accounts through users, not reactors; this invalid selection causes the combined GraphQL request to fail before any reply, resolution, or reaction data is returned. The dismissal mechanism therefore cannot run on GitHub as written.
Agent Prompt
## Issue description
The GraphQL query selects the nonexistent `ReactionGroup.reactors` field, causing GitHub to reject the complete dismissal query.
## Issue Context
GitHub exposes the accounts associated with a reaction group through its `users` connection. Update both the query and the response-reading instructions consistently.
## Fix Focus Areas
- skills/pr-review/github/SKILL.md[87-87]
- skills/pr-review/github/SKILL.md[109-111]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
reactors is valid — the claim is backwards. users is the older ReactionGroup field, deprecated in favor of reactors; its own deprecation notice says to use reactors. Default introspection hides deprecated fields, which may be where the confusion came from:
$ gh api graphql -f query='{ __type(name:"ReactionGroup"){ fields(includeDeprecated:true){ name isDeprecated deprecationReason } } }' \
--jq '.data.__type.fields[] | .name + (if .isDeprecated then " [deprecated: "+.deprecationReason+"]" else "" end)'
content
createdAt
reactors
subject
users [deprecated: Reactors can now be mannequins, bots, and organizations. Use the `reactors` field instead. Removal on 2021-10-01 UTC.]
viewerHasReactedAnd the query doesn't fail — here it is, extracted verbatim from this file at the PR head and run against this PR's own review threads (the ones this bot posted):
$ REPO_FULL_NAME=fullsend-ai/agents PR_NUMBER=1066 # then the snippet exactly as committed
$ echo "$DISMISSALS" | jq -e 'has("errors") | not' && \
echo "$DISMISSALS" | jq '.data.repository.pullRequest.reviewThreads.nodes | length'
true
8exit 0, no errors key, and every thread carries the full reactionGroups array with reactors.totalCount. No change needed.
| reviewThreads(last:100){ | ||
| pageInfo{ hasPreviousPage } | ||
| nodes{ |
There was a problem hiding this comment.
8. Older dismissals are truncated 🐞 Bug ☼ Reliability
The query fetches only the newest 100 review threads and returns no cursor with which to perform the documented continuation. Dismissals attached to older prior findings are therefore unavailable and get re-raised, which is the behavior this feature is intended to prevent.
Agent Prompt
## Issue description
The dismissal query cannot retrieve review threads older than the final 100.
## Issue Context
Request the appropriate page cursor and provide executable pagination/aggregation commands so all review threads needed to correlate prior findings are available.
## Fix Focus Areas
- skills/pr-review/github/SKILL.md[70-93]
- skills/pr-review/github/SKILL.md[101-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| authorAssociation | ||
| body createdAt path diffHunk | ||
| line originalLine startLine originalStartLine | ||
| reactionGroups{ content reactors(first:10){ totalCount nodes{ ... on User { login } } } } |
There was a problem hiding this comment.
9. Trusted reactors are truncated 🐞 Bug ☼ Reliability
The query returns only the first ten reactors for each reaction group and provides no cursor or follow-up fetch. A trusted maintainer's 👎 outside that slice is invisible, so a valid dismissal is not honored even though totalCount indicates additional reactors exist.
Agent Prompt
## Issue description
Only ten reactor identities are available for trust evaluation.
## Issue Context
When the thumbs-down group's total exceeds the returned node count, paginate its reactors and aggregate all logins before applying the trust gate.
## Fix Focus Areas
- skills/pr-review/github/SKILL.md[82-111]
- skills/pr-review/SKILL.md[248-263]
- skills/pr-review/SKILL.md[289-296]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…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>
8b2cadd to
8b6b267
Compare
| any other re-review finding. Someone who dismissed one version of the | ||
| code was not asked about a different version of it. | ||
|
|
||
| This does not suppress the finding — it stays visible to human reviewers |
There was a problem hiding this comment.
HIGH — "Does not suppress the finding" is false under the shipped defaults — every downgrade path targets info, which three layers delete
Line 1287 states "This does not suppress the finding — it stays visible to human reviewers at low urgency." Verified against the head tree, that is untrue for the fleet's default configuration, and every new downgrade path in this PR targets info: dismissed-and-unchanged (1278-1281), refuted dispute (1310-1311), and the engage-once stop for medium-and-below (1327-1328).
Three independent layers strip info:
harness/review.yaml:59and:63setREVIEW_FINDING_SEVERITY_THRESHOLD: "low"for both runner and sandbox.agents/review.md:65instructs "Suppress findings below the threshold — do not mention them in the review body and do not include them in thefindingsarray".scripts/post-review.src.sh:136-151re-filters withinfo=0 >= threshold low=1→ false.
Two verified consequences:
(a) The dismissal annotation "Previously raised and dismissed by @<login> (<signal kind>) — retained at info severity…" never reaches the PR at all, so a dismissal becomes silent deletion rather than the visible-at-low-urgency outcome the safety argument at 1287-1291 rests on.
(b) The engage-once stop at 1318-1321 self-destructs: prior findings are parsed in step 2a from /sandbox/workspace/prior-review.txt (SKILL.md:186-192), i.e. the posted body. A medium finding downgraded to info is stripped from that body, so on the next round the orchestrator cannot find "Author's justification considered:" in the prior text, re-raises the finding fresh at medium, and the cycle repeats — the exact #106 ping-pong, now oscillating and driven from a path that is deliberately not trust-gated (1304-1307).
Distinct from the already-posted "Repeated dispute silences defects" comment (id 3871209957), which argues the downgrade policy is wrong; this is that the chosen severity level is filtered out by the pipeline, so neither the visibility promise nor the marker-persistence mechanism can work as written.
Also note scripts/post-review.src.sh:154-172: if filtering empties findings[] on a request-changes verdict, it downgrades to comment, not approve — a third outcome the skill text does not describe.
Suggestion: Use low, not info, on all three paths. Step 6f already treats "Low or info findings only (no medium+) → approve", so low yields the identical non-blocking verdict while surviving the default threshold — keeping the dismissal annotation on the PR and keeping the "Author's justification considered:" marker in prior-review.txt so the stop condition can actually fire next round. If info is deliberate, replace the sentence at 1287 with an explicit statement that at any threshold above info the finding is removed from the posted review entirely, and delete the marker-detection rule at 1318-1321, which cannot work.
There was a problem hiding this comment.
Fixed in 9c8cfa3 — all three downgrade paths now target low, the stop rules explicitly keep their markers in the emitted description so they survive the posted-body → prior-review round trip, and the threshold dependence is documented. Took the suggestion as-is.
| 2. Thread resolvers and reactors carry no association of their own. Look | ||
| their login up in the associations already returned for this PR's | ||
| thread comments — someone who resolves a thread has usually also | ||
| written in it, or elsewhere on the PR — and apply the same tier. |
There was a problem hiding this comment.
MEDIUM — Trust lookup tells the agent to search "elsewhere on the PR", but the query never fetches PR-level comments or review bodies
Step 2a-1's trust rule 2 (lines 262-265) says resolvers and reactors carry no association of their own, so "Look their login up in the associations already returned for this PR's thread comments — someone who resolves a thread has usually also written in it, or elsewhere on the PR."
The committed GraphQL query at skills/pr-review/github/SKILL.md:71-92 selects only reviewThreads.nodes.comments. PR-level issue comments and review summary bodies — both of which do carry authorAssociation — are never requested. So "or elsewhere on the PR" points at data the agent does not have, and an LLM following it either hallucinates a lookup or issues an unspecified extra call.
The effectiveness gap is structural, not a wording nit: the signal table at 297-301 describes thread resolution as "what a maintainer reaches for when a finding is not worth a sentence," while rule 2 then requires that same maintainer to have written a sentence in some thread, and rule 3's collaborator fallback is documented (274-278) as generally 403ing in the sandbox. Resolution-only and reaction-only dismissals from anyone who never commented inline therefore fall through to "not trusted".
The limitation paragraph at 278-284 attributes this to private orgs where admins report as CONTRIBUTOR, which understates it — it also hits public-org MEMBER resolvers who simply resolved without replying. (One reviewer reported measuring roughly 39% of resolutions unhonored across this repo's last 30 PRs; that measurement was not independently reproduced here and is cited only as the reviewer's own claim.)
Suggestion: Either extend the same query with comments(last:100) and reviews(last:100) association nodes so the lookup matches its own prose (one round trip, no extra cost), or correct line 265 to say the lookup is limited to associations returned for this PR's review-thread comments. Re-frame 278-284 to state the common case up front: resolution-only and reaction-only dismissals are honored only when the actor also commented inline — which is what scopes the runner-side trust-resolution follow-up.
There was a problem hiding this comment.
Fixed in 9c8cfa3 — the query now also fetches PR-level comments and reviews associations in the same request, and the limitation text states the residual gap up front. Verified live on this PR: a review-body-only MEMBER now resolves.
| `category`, the dismissed code itself (the anchor line(s) from the root | ||
| comment's hunk, trimmed to the flagged line and a line or two of | ||
| surrounding context), the signal kind (`reply`, `resolved`, or | ||
| `thumbs-down`), the dismisser's login, and a short excerpt of the decline |
There was a problem hiding this comment.
MEDIUM — 2a-1 → 6e data contract is incomplete in both directions: disputed replies have no carrier, and the recorded decline excerpt is never consumed
Step 2a-1 defines exactly one hand-off structure, DISMISSED_FINDINGS (line 321), and grep over the head file shows it is referenced only at 321, 1254, 1257, and 1306. Two gaps follow.
-
Disputed replies have no carrier. Lines 304-306 say a reply disputing correctness "is handled on its merits in step 6e" and 1306 says it "never reaches
DISMISSED_FINDINGS", but nothing instructs the orchestrator to record disputed replies in any named variable, to preserve the$DISMISSALSpayload, or to re-fetch thread data at 6e. Step 6e sits ~980 lines later and is told to "Evaluate it against the diff and the source at the PR head" (1308) with no defined input, so the engage-once machinery at 1318-1331 can easily never see the thread it exists to handle. -
The reverse direction. Line 325 records "a short excerpt of the decline reply when there was one" and asserts "This feeds into step 6e", but the entire 6e dismissal subsection (1251-1334) uses only
file,category, the recorded snippet, the login, and the signal kind — the excerpt is never read or emitted. It is a declared field with no consumer, which is also why the prepended annotation at 1280-1281 carries no reason text.
Suggestion: Define an explicit DISPUTED_FINDINGS (or equivalent) structure populated in 2a-1 alongside DISMISSED_FINDINGS — thread identity, matched prior finding, the reply text, its author — and name it as 6e's input at 1304. Either give the decline excerpt a consumer in 6e (e.g. include it in the prepended annotation) or drop it from the 2a-1 record and the "feeds into step 6e" claim.
There was a problem hiding this comment.
Fixed in 9c8cfa3 — DISPUTED_FINDINGS is recorded in 2a-1 and named as 6e's input, and the decline excerpt now has its consumer (Dismissal note: in the annotation), sanitized at recording since it enters the posted body.
| cache the result: | ||
|
|
||
| 1. When the signal carries an author association — replies do — accept | ||
| `OWNER`, `MEMBER`, or `COLLABORATOR`, the same tier |
There was a problem hiding this comment.
MEDIUM — Trust tier accepts MEMBER, which is org membership, not the "authority over the repo" the rule claims to require
The trust boundary is stated at 254-255 as "a dismissal counts only from someone other than the PR author who holds authority over the repo," but rule 1 at 258-261 accepts OWNER, MEMBER, or COLLABORATOR from authorAssociation. GraphQL's MEMBER means the author is a member of the organization that owns the repository — it carries no repo-level write implication. The cited precedent (.github/scripts/check-e2e-authorization.sh) gates who may trigger an e2e run, a different risk shape from retiring a review finding.
Concretely: an org member with read-only access to this repo cannot resolve a thread (GitHub requires write for that), but a plain reply — "pre-existing pattern, out of scope" — is a qualifying signal under the table at 295-301, and only critical findings are carved out at 1293. So a non-write org member can take a HIGH finding out of the blocking set. Combined with the info-threshold finding on line 1287, the result is not even annotated on the PR.
Suggestion: Either tighten the prose at 254-255 to say what the mechanism actually requires (org membership or repo collaboration, not repo authority), or tighten the mechanism for high-severity findings — require a resolver/collaborator-confirmed write+ role, or require a written reply rather than a bare reaction, before a high finding can be dismissed.
There was a problem hiding this comment.
Fixed in 9c8cfa3 — the boundary prose now says org member / repo collaborator (what the tier actually attests), and high findings dismiss only by written reply; resolution or 👎 alone no longer qualify. Critical unchanged.
…t gate to its data Addresses waynesun09's review — all four findings, each verified against the head tree before changing anything. The high one first: every downgrade path in this PR targeted `info`, and `info` does not survive the fleet. REVIEW_FINDING_SEVERITY_THRESHOLD defaults to "low" on both runner and sandbox, agents/review.md tells the agent to suppress below-threshold findings from the body and the findings array, and post-review.src.sh re-filters with the same ordinals (info=0 < low=1). So "retained at info severity" was retained nowhere: the annotation never reached the PR, and — the sharper half of the finding — the prior-review context is rebuilt each round from the posted body, so the "Author's justification considered:" marker vanished with it and the engage-once stop could never fire. A medium finding disputed once would re-raise fresh at medium every round: the fullsend-ai#106 ping-pong, rebuilt inside its own fix, driven from the one path that is deliberately not trust-gated. All three paths now target `low`, which survives the default threshold and still resolves to the same non-blocking verdict in 6f. The skill now says why, names the threshold dependence (a repo filtering at medium-or-above trades away this step's round-to-round memory), and the stop rules explicitly keep their markers in the emitted description — a marker that does not reach the posted body does not exist next round. A prior "Author's justification accepted:" marker is honored the same way instead of being re-litigated. The trust gate now matches its data, in both directions. The prose claimed "authority over the repo" while accepting MEMBER, which attests org membership, not write access — the boundary is now named honestly, and a high finding dismisses only by written reply: a resolution or a 👎 is a one-click signal with no stated reason, and membership alone should not be able to silently take a high finding out of the blocking set. Critical stays critical regardless, as before. In the other direction, the lookup rule pointed at data the query never fetched ("or elsewhere on the PR"). The query now fetches it: PR-level comments and review bodies carry authorAssociation, so resolvers and reactors who wrote anywhere on the PR are tierable in the same single request. Verified live against this PR itself — waynesun09 appears only as a review body, no issue comment, no thread reply, and the lookup now resolves him to MEMBER; before this change a resolution-only dismissal from exactly this shape of reviewer fell through to the 403. The limitation paragraph now states the residual gap up front — an actor who never wrote anything on the PR still fails closed — rather than framing it as a private-org corner case. And the 2a-1 → 6e contract is closed in both directions. Disputed replies get a named carrier, DISPUTED_FINDINGS, recorded alongside DISMISSED_FINDINGS and declared as the disputed-findings rule's input — previously 6e was told to evaluate a reply that nothing had preserved. The decline excerpt now has its consumer: it is quoted in the posted annotation as a "Dismissal note", and is sanitized at recording time (control characters stripped, the pipeline's own sentinels redacted, capped) because it is participant text entering the bot's own posted body — the one place a forged **Head SHA:** marker would be read back as pipeline state next round. Dispute replies are never quoted into findings at all; the appended sentence paraphrases. Query re-verified end to end after the extension: executed verbatim from this file against PR fullsend-ai#1066, exit 0, thread fields intact, both new connections returning associations. Signed-off-by: guy oron <goron@redhat.com>
|
@ralphbean I guess your input is needed here |
4 participants
Heyyaa all : )
Quick reminder — this is the take-over of #709 that Ralph asked for on the review-economy thread ("please take over #709 or start it from scratch"), closing agents#106 (review agent re-raising findings that were already legitimately dismissed).
What this does
On re-review, dismissed findings (reply, resolved thread, or 👎 — all from someone with real authority, never the PR author) get downgraded to low instead of re-raised, as long as the dismissed code hasn't changed (low, not info — info is stripped by the default severity threshold, which would silently delete both the finding and its round-to-round markers). Critical findings never downgrade by dismissal, high findings dismiss only by written reply, and a disputed finding is only argued once.
Ralph's design (#709) is the base — trust gate, decline-vs-round-boundary matching, downgrade-not-drop. Added: resolve/👎 as dismissals via one GraphQL query, the critical/high carve-out, engage-once for disputes, and a fail-closed fix for a trust-gate case that 403s inside the read-only review sandbox (details in-thread).
Already went a round with qodo — 5 findings fixed, 1 refuted (schema check in-thread), 2 are fail-closed-by-design truncation limits.
Not included
An eval case (blocked by #245 — self-review token issue) and GitLab parity (signals exist, but no live instance to verify the trust mapping against).
Heads up for review
skills/is a protected path, so a protected-path finding here is expected.functional-tests-completeneeds a maintainer'sok-to-testafter the last push.