fix(ai-review): fence untrusted PR content, stop shifting inline anchors, and stop judging a fix you cannot see#9145
Merged
Conversation
…ors, and stop judging a fix you cannot see Three ways the AI reviewer acted confidently on input it had silently altered, truncated, or should never have trusted in the first place. #9035 — attacker-controlled text reached the prompt with no structural defense. Title, body and diff were concatenated straight in. "Judge ONLY the diff" tells the model what to look at; it never says that what it is looking at is DATA. The only defense was regex defang, which is deliberately narrow and which paraphrase or encoding walks past — and the same text reaches BOTH consensus reviewers, so a successful steer suppresses both and never even trips the single-rejection ai_review_split rule that exists to catch one reviewer being wrong. Each untrusted region is now fenced, with a system rule saying content between the markers is data and cannot change the rules, the output format, or the verdict. Forged markers are stripped from inside the region so a body cannot close its own fence early — without that, fencing would be worse than none. And a DETECTED attempt now holds the PR for a human. Defang recorded these and, by design, never let them affect the disposition, so a caught attacker got a normal roll. Manipulation aimed at the reviewer is evidence of intent, and the one thing it must not buy is an automated decision. Held, never closed: the detector is a regex running over a repository whose own subject matter is AI review, so a false positive is entirely possible, and a hold costs a contributor a wait where a close would cost them their PR. It reuses the existing ai_review_inconclusive hold path, so no gate-decision twin changes. #9076 — the defang shifted every inline anchor after it. The injection patterns' `[^.]{0,N}` gaps deliberately span newlines, so one match can swallow two or three diff lines including their `+`/`-` markers and even an `@@` header. Replacing all of that with a single-line literal collapsed those newlines. The reviewer counts a finding's line over the DEFANGED text, but the finding is validated and posted against the ORIGINAL patch — so every anchor after a multi-line redaction shifted, and a shifted anchor that still landed inside the commentable set passed validation and posted publicly on the wrong line of a contributor's PR. The redaction now re-emits one newline per newline consumed, so the two coordinate systems stay congruent. Two more on the same surface. Blocker-severity findings now anchor to ADDED lines only: rightSideLinesFromPatch admits unchanged context too, while the prompt asks for an added line and warns that a wrong line is worse than none — set membership was the only check, and context satisfies it, so a model miscounting by one to three lines landed on context and posted. That file conflated two different properties throughout: "GitHub will accept this anchor" and "this anchor is correct". Nits still allow context, because a misplaced nit is noise while a misplaced blocker costs someone their PR. And an empty patch line now counts as the context line it is instead of desynchronizing every later line number in the file (the trailing split artifact of a patch ending in a newline is dropped first, so the two cases stay distinct). #9075 — a confident "you didn't fix the issue" computed from a window that never contained the fix. linked-issue-satisfaction's own header claims "NO gate wiring, NO disposition change… advisory-only either way". That is stale: under linkedIssueSatisfactionGateMode: "block" an `unaddressed` verdict pushes a critical-path finding reading "this PR does not appear to satisfy its linked issue's scope." Meanwhile the module re-slices the already-truncated diff to 60k under the header "Unified diff (truncated if large):" — a hedge, not a fact, which leaves the model to guess whether it is seeing everything. A PR whose fix sits in a dropped hunk, a filtered file, or past character 60,000 got that public verdict anyway. The confidence floor does not help: it guards against a model being unsure, not against a model being sure about the wrong input. The header now states the fact, and an `unaddressed` verdict over a known-truncated diff degrades to `partial`. Only `unaddressed` — "addressed" over a truncated diff is the model finding POSITIVE evidence, which truncation cannot manufacture. Same reasoning #8961 already applies to truncated PR bodies. POLICY REVERSAL, called out explicitly. selectContextSectionsWithinBudget's `break` was pinned by a test asserting it is "a hard priority cutoff, not a bin-packing optimization". That reasoning holds for sections that are genuinely model context. It does not hold for what actually sat at the bottom of the list: testEvidence is ~200 characters and is not context at all but a deterministic classifier FACT ("this PR changes no test paths"), so one large RAG block silently discarded it on exactly the large PRs where it matters most, with no marker anywhere. An oversized section is now skipped rather than ending the loop. Priority order still decides who gets first refusal on the budget; every included section still genuinely fits. Both tests encoding the old rule are inverted with the reasoning in place. Also fixes main, same one-line change as #9144 (identical, so the two merge cleanly): docs-beta-onboarding asserted the doc still says "base-agent", which #9117 intentionally retired when it repositioned the product. Local gate green end to end (npm run test:ci, exit 0). 100% line and branch coverage on all 317 added src lines. Closes #9035 Closes #9075 Closes #9076
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9145 +/- ##
==========================================
+ Coverage 90.56% 92.67% +2.11%
==========================================
Files 96 815 +719
Lines 22490 80892 +58402
Branches 3884 24573 +20689
==========================================
+ Hits 20367 74970 +54603
- Misses 1945 4844 +2899
- Partials 178 1078 +900
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-27 03:42:07 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
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.
Three ways the AI reviewer acted confidently on input it had silently altered, truncated, or should never have trusted in the first place.
#9035 — attacker-controlled text reached the prompt with no structural defense
Title, body and diff were concatenated straight into the user prompt. "Judge ONLY the diff" tells the model what to look at; it never says that what it is looking at is data. The only defense was regex defang — deliberately narrow, and walked past by paraphrase or encoding. Worse, the same text reaches both consensus reviewers, so a successful steer suppresses both and never even trips the single-rejection
ai_review_splitrule that exists to catch one reviewer being wrong.Each untrusted region is now fenced, with a system rule stating that content between the markers is data and cannot change the rules, the output format, or the verdict. Forged markers are stripped from inside the region, so a body cannot close its own fence early — without that, fencing would be worse than none.
And a detected attempt now holds the PR for a human. Defang recorded these and, by design, never let them affect the disposition, so a caught attacker got a completely normal roll. Manipulation aimed at the reviewer is evidence of intent, and the one thing it must not buy is an automated decision.
Held, never closed — the detector is a regex running over a repository whose own subject matter is AI review, so a false positive on a legitimate PR discussing prompt handling is entirely possible, and a hold costs that contributor a wait where a close would cost them their PR. It reuses the existing
ai_review_inconclusivehold path, so no gate-decision twin changes were needed.#9076 — the defang shifted every inline anchor after it
The injection patterns'
[^.]{0,N}gaps deliberately span newlines, so one match can swallow two or three diff lines — including their+/-markers and even an@@header. Replacing all of that with a single-line literal collapsed those newlines away.The reviewer counts an inline finding's
lineover the defanged text, but the finding is validated and posted against the original patch. So every anchor after a multi-line redaction shifted by the number of collapsed newlines, and a shifted anchor that still landed inside the commentable set passed validation and posted publicly on the wrong line of a contributor's PR. The redaction now re-emits one newline per newline consumed, pinned by a line-count invariant test.Two more on the same surface:
rightSideLinesFromPatchadmits every RIGHT-side line, added and unchanged context, while the prompt explicitly asks for "an ADDED (+) line" and warns "a wrong line is worse than none". Set membership was the only check and context satisfies it, so a model miscounting by one to three lines within a hunk landed on context, passed every check, and posted. That file conflated two different properties throughout: "GitHub will accept this anchor" (avoiding a 422) and "this anchor is correct". Nits still allow context — a misplaced nit is noise, a misplaced blocker costs someone their PR.\nis dropped first, so that case stays distinct from a genuine stripped-space context line.#9075 — a confident "you didn't fix the issue", computed from a window that never contained the fix
linked-issue-satisfaction's own header claims "NO gate wiring, NO disposition change… advisory-only either way". That is stale. UnderlinkedIssueSatisfactionGateMode: "block"anunaddressedverdict pushes a critical-path finding with the public text "this PR does not appear to satisfy its linked issue's scope."Meanwhile the module re-slices the already-truncated diff to 60k under the header "Unified diff (truncated if large):" — a hedge, not a fact, leaving the model to guess whether it is seeing everything. A PR whose issue-satisfying change sits in a dropped hunk, a filtered-out file, or past character 60,000 received that public verdict anyway. The confidence floor is no protection: it guards against a model being unsure, not against a model being sure about the wrong input.
The header now states the fact, and an
unaddressedverdict over a known-truncated diff degrades topartial. Onlyunaddressed— "addressed" over a truncated diff is the model finding positive evidence, which truncation cannot manufacture. Same reasoning #8961 already applies to truncated PR bodies.Policy reversal — please read
selectContextSectionsWithinBudget'sbreakwas pinned by a test asserting it is "a hard priority cutoff, not a bin-packing optimization that skips a large blocked section to squeeze in a smaller lower-priority one."That reasoning holds for sections that are genuinely model context, where priority order encodes what matters most. It does not hold for what actually sat at the bottom of that list:
testEvidenceis ~200 characters and is not context at all but a deterministic classifier fact ("this PR changes no test paths"). Under the old rule a single large RAG block silently discarded it, on precisely the large PRs where a reviewer most needs to know whether tests were touched, with no marker anywhere saying it had been dropped.An oversized section is now skipped rather than ending the loop. Priority order still decides who gets first refusal on the budget; it just no longer lets one oversized section evict everything cheaper behind it, and every included section still genuinely fits. Two existing tests encoded the old rule and are inverted here, with the reasoning written into them.
Notes for review
Also fixes main — the same one-line change as #9144, byte-identical so the two merge cleanly:
docs-beta-onboardingasserted the onboarding doc still says "base-agent", which #9117 intentionally retired when it repositioned the product.Three prompt-shape assertions in unrelated suites (impact-map, RAG, culture-profile) pinned the literal string
"Unified diff (truncated if large):". They now assert the diff section is present without pinning its prose, which is what those suites actually care about.Verification
npm run test:ci— exit 0, full gate green.src/lines, verified by intersectinggit diff -U0againstlcov.infoDA:/BRDA:records.Closes #9035
Closes #9075
Closes #9076