Skip to content

orb(ai-review): prompt-injection defang collapses newlines, shifting every inline anchor after it onto the wrong line #9076

Description

@JSONbored

neutralizePromptInjection (src/review/prompt-injection.ts ~76-79) replaces each match with the single-line literal [external-instruction-redacted]. The patterns' [^.]{0,40} gaps are documented (~11-15) as deliberately spanning newlines, so one match can swallow 2-3 diff lines — including their leading +/-/space markers and, worst case, an @@ hunk header or a ### path file header — collapsing them into one line.

The model then counts line "forward from the + start in the nearest @@ header" (src/services/ai-review.ts ~1119) over the collapsed text. But inline findings are validated and posted against file.payload.patch, the original (src/review/inline-comments-select.ts ~72-73).

So every anchor after a multi-line redaction is shifted by the number of collapsed newlines — and a shifted anchor that still lands in the commentable set passes validation and gets posted publicly on the wrong line of a contributor's PR. Secondary: a redacted + marker makes an added line indistinguishable from a removed one for the reviewer's own reasoning.

This is not about injection-detection efficacy (covered separately in #9035) — it is the diff-integrity side effect. LOOPOVER_REVIEW_SAFETY=true is live on our box, so this path is active.

Related, same surface: anchors accept context lines

rightSideLinesFromPatch (inline-comments-select.ts ~12-28) adds every RIGHT-side line — added and unchanged context (only - and \ are skipped). The prompt explicitly asks for "an ADDED (+) line" and warns "a wrong line is worse than none", but the only validation is set membership, which a context line satisfies. A model miscounting by 1-3 lines within a hunk lands on context, passes every check, and the comment posts. The file's own promise ("so GitHub never 422s") is about API acceptance, not correctness — the two are conflated throughout.

The codebase already has the stricter primitive: addedLinesByPath (used for suggestion blocks) is added-only.

Minor sub-bug (SUSPECTED): ~22-23 skips a line whose marker is undefined without incrementing right, so an empty-string patch line desynchronizes every subsequent line number in that file. Git emits " " for blank context lines so it should not fire on GitHub payloads, but nothing enforces it.

Fix

  1. Make the redaction newline-preserving — emit the literal plus one \n per newline consumed, or defang per diff line rather than over the assembled string. Pin it with expect(defanged.split("\n").length).toBe(original.split("\n").length).
  2. Anchor blocker-severity inline findings to addedLinesByPath only; leave context lines allowed for nits.
  3. Count the empty-string line as context (right += 1) instead of skipping it.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions