docs(review): tell each review dimension what not to flag - #1004
Draft
guyoron1 wants to merge 5 commits into
Draft
docs(review): tell each review dimension what not to flag#1004guyoron1 wants to merge 5 commits into
guyoron1 wants to merge 5 commits into
Conversation
An instrumented review on fullsend-ai/fullsend#4080 posted 16 inline comments, 6 of them false — a 37.5% false-positive rate. Nothing in the pr-review prompts told a sub-agent what not to report, so every recognizable pattern was a candidate finding. meta-prompt.md reaches every dimension sub-agent and the challenger, so the cross-cutting rules go there once: six classes of non-issue (code the PR did not change, prior findings already fixed, duplicates of deterministic linters, defense-in-depth over an adequate primary defense, preference, and risks the change does not enable), and a severity bar that anchors each level to named impact. The bar is deliberately two-sided. Under-rating was measured alongside over-rating: a regex bug that deleted lines from a user's config was reported as low and "benign". So removing an existing defense is explicitly exempt from the theoretical-risk rule and rates on what the defense protected. Signed-off-by: guy oron <goron@redhat.com>
A security reviewer's non-issues are not a docs reviewer's, so the generic bar in the meta-prompt is not enough on its own. Each dimension gets the baits it actually falls for. security is the largest section, because the vocabulary of a vulnerability appears constantly in safe code: a digest used as a cache key is not a weak hash, an argv list of literals with the shell disabled has nothing to inject into, and a value the program computed is not attacker controlled. Paired with the inverse rule — a control the diff removes is always flagged on what it protected, whatever the PR description claims about it. style-conventions is dispatched on every PR and so is the largest noise source; it now carries a low/info severity ceiling and a requirement to cite precedent before calling anything inconsistent. correctness keeps its teeth: the narrowing rules end with the counterweight that a real finding names the input and the wrong value that comes out. intent-coherence must anchor architectural opinions in CLAUDE.md, an ADR, or two existing files. docs-currency must grep before claiming staleness. cross-repo-contracts must name the consumer. Signed-off-by: guy oron <goron@redhat.com>
The challenger is already the adversarial verification pass — it removes false positives, deduplicates, and recalibrates severity. It did not need a second one; it needed the measured failure modes named. Three additions to its procedure: confirm a finding is about lines this PR changed (stacked-PR misattribution was a named source of the 6 false comments on fullsend-ai/fullsend#4080), remove restatements of prior findings the author already fixed, and remove duplicates of deterministic lint. Severity calibration is now explicitly bidirectional — it must upgrade an under-rated defect, not only downgrade an inflated one. The blanket "err on the side of keeping" default was doing the damage it was meant to prevent: it made every plausible-looking pattern unremovable. It splits by what is actually ambiguous. Unclear whether the code is wrong, keep it. Unclear whether a risky-looking construct is exploitable, downgrade to info — the reviewer looked and concluded it was fine, which is worth a sentence and never a high. Findings about a control the diff removed are exempt from downward adjudication. Signed-off-by: guy oron <goron@redhat.com>
A review with no upper bound on findings converges on an audit, and the long tail is where the false positives live. The cap is 10: the instrumented review on fullsend-ai/fullsend#4080 posted 16 comments of which 10 were true, so a cap at 10 would have cost that review nothing real. Published large-scale AI review systems average around 1.2 findings per review, so an ordinary PR never reaches the limit. The cap is presentation only. The verdict is computed in 6f on the complete adjudicated set and is never recomputed, and critical and high findings are never dropped — in practice only low and info overflow. Silent truncation would read as "nothing else was wrong", which is a worse distortion than the noise the cap removes, so anything held back is disclosed as a single summary bullet naming the categories and files. The bullet is body text: it never enters the findings array, never becomes an inline comment, and never moves the verdict. Signed-off-by: guy oron <goron@redhat.com>
…cerns Four corrections from an adversarial pass over the new guidance. The 'downgrade to info' path was a deletion, not a footnote: the shipped REVIEW_FINDING_SEVERITY_THRESHOLD is 'low', and the agent is told to suppress everything below it. That is the right outcome for a pattern the reviewer verified is safe, and the wrong one for a pattern it simply could not reach — an unreadable helper, a callee outside the provided context. Those now route to 'keep at stated severity', and the rules say plainly that info means dropped so the choice is made with open eyes. A docstring is the author's claim about the code, not evidence of it, written by whoever wrote the bug. It narrows where to look; it does not end the search. 'Not a security boundary' above a function a second file uses as one is precisely what a reviewer is for. Secret-scanning is no longer deferred to gitleaks: this reviewer is installed across repos and cannot assume the one under review runs it, and scanners miss non-standard formats regardless. The wider linter-duplication rule now says to check the repo's own CI rather than trust a fixed list. The finding cap can drop medium findings when more than ten sit above low, so the overflow bullet names the severities actually held back instead of claiming low/info. Signed-off-by: guy oron <goron@redhat.com>
Functional tests are runningAuthorization passed for this commit. See the Functional Tests workflow for results. |
Author
|
Relates to fullsend-ai/fullsend#1972 — the challenger change requires positive evidence before a finding is downgraded, and routes 'could not inspect what this reaches' to keep rather than dismiss. Measurable once #1003 lands. |
This was referenced Aug 25, 2026
This was referenced Sep 1, 2026
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
2 participants
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.
A measured 37.5% false-positive rate (6 of 16 inline comments on
fullsend-ai/fullsend#4080), against a review skill with no negative guidance anywhere in it and no bound on how many findings a review can post.What changes
A shared precision bar in
meta-prompt.md, which is injected into every dimension dispatch and the challenger: six categorical non-issue classes, a severity ladder anchored to named impact, and two tiebreakers — name the impact or lower the severity, and removing an existing defense is never theoretical.Per-dimension "what not to flag" sections, written for each dimension's own failure modes rather than copied. Security's non-issues are not correctness's.
Sharper adjudication.
challenger.mdwas already the adversarial pass; its closing rule was "err on the side of keeping findings when evidence is ambiguous," which makes every plausible-looking safe pattern unremovable. That now splits by what is ambiguous:info.The distinction matters because
infosits below the defaultREVIEW_FINDING_SEVERITY_THRESHOLD, so downgrading is deletion, not a footnote. That is the right outcome for something the reviewer actually verified and the wrong one for something it simply could not reach. Regression findings are never adjudicated down.A 10-finding cap at step 6g — after the verdict is computed and never recomputed, so it governs presentation, not conclusions.
criticalandhighare never truncated. Anything held back is disclosed, naming the severities actually dropped, because silent truncation reads as "nothing else was wrong."Two claims this PR does not make
A docstring is not evidence. It is the author's claim about the code, written by whoever wrote the bug. It narrows where to look; it never ends the search. "Not a security boundary" above a function a second file uses as one is exactly what a reviewer exists to catch.
A linter you do not run is not a backstop. This reviewer is installed across repos, so secret-scanning no longer defers to
gitleaksbeing present, and the linter-duplication rule says to check the repo's own CI rather than trust a fixed list.How to judge whether this suppresses real defects
eval/review-precision-cases(#1003) is the instrument.004-seeded-bugsand008-reassuring-docstringmust still trip;005/006/007must stay quiet.008is the one that matters here — it is a real vulnerability wearing a reassuring docstring, and it is the case that fails if this PR ever talks a reviewer out of a genuine finding.Known asymmetry, stated rather than left to be found: the meta-prompt reaches both the generating dimension and the challenger, and the challenger may only subtract. So the non-issue classes are applied twice to every finding while the protective ambiguity rule is applied once.
Follow-ups this PR does not address
skills/code-review/, the pre-push path, has none of this hardening.Scope
This tunes the claude review path —
skills/pr-review's meta-prompt,dimension sub-agents, and challenger. The pi-runtime review runs without
sub-agents and has its own prompt surface; the same precision bar should
port there once that path stabilizes.