Skip to content

Commit 9b2c024

Browse files
sahrizvisahrizvi
committed
fix(review): [R20 S4] tighten changedLinesForScan marker guard to only emit +/- lines (harness-bot P2)
altimate-harness-bot review, PR #1028 risk-tier.ts:251. The earlier guard `if (marker === " ") continue` skipped only unified-diff context lines (space-prefixed). Free-form lines from a raw `git diff -p` output — `diff --git a/... b/...`, `index abc..def`, `Author:`, `Date:` — carry a `""` marker (they don't start with `+`, `-`, or space), fall past the guard, and reach `dbtRiskYmlKeyMatches` via `out.push(raw)`. In-production `file.diff` comes from the GitHub PR files API, whose per-file diff blocks begin at the first `@@` hunk header (no `diff --git` / `index` / commit-metadata lines). So the exposure was defensive parity, not a live promotion bug. But the code was asymmetric — `+++`/`---`/`@@` headers were explicitly skipped at line 220; other free-form lines weren't — and a future caller passing raw `git diff -p` output would silently see header lines in the scanner. Fix (one operator): `if (marker !== "+" && marker !== "-") continue`. Any line whose leading character isn't `+` or `-` is either a context line (fine, still updated scalar state above) or a free-form header (skipped, no emit). Test: `R20 S4: raw git diff -p free-form headers do NOT reach the scanner` builds a full raw-diff string (four header lines + the `@@` hunk + one changed line) and asserts the same tier verdict as the bare-`tests`-word test — trivial, no promotion. 231/231 review-* tests pass.
1 parent 29c2423 commit 9b2c024

2 files changed

Lines changed: 35 additions & 3 deletions

File tree

‎packages/opencode/src/altimate/review/risk-tier.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,9 +241,16 @@ function changedLinesForScan(diff: string | undefined): string {
241241
scalarIndent = indent
242242
}
243243
// Only added/removed lines are candidates for regex scanning; context
244-
// lines only feed scalar-state tracking. When the changed line lives
245-
// inside a scalar, emit a blank so the regex has nothing to match.
246-
if (marker === " ") continue
244+
// lines only feed scalar-state tracking. Anything without a `+`/`-`
245+
// marker (context lines, or free-form headers like `diff --git`,
246+
// `index abc..def`, `Author:`, `Date:` from a raw `git diff -p`
247+
// output) is not a changed line and must not reach the scanner —
248+
// the earlier `+++`/`---`/`@@` guard covers unified-diff headers,
249+
// but this closes the gap for anything else (altimate-harness-bot
250+
// review, PR #1028 risk-tier.ts:251). In-production `file.diff`
251+
// comes from the GitHub PR files API and hunks start at `@@`, so
252+
// this is defensive parity, not a live bug.
253+
if (marker !== "+" && marker !== "-") continue
247254
out.push(insideScalar ? "" : raw)
248255
}
249256
return out.join("\n")

‎packages/opencode/test/altimate/review.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,31 @@ describe("risk-tier", () => {
688688
expect(r.tier).toBe("full")
689689
})
690690

691+
test("R20 S4: raw `git diff -p` free-form headers do NOT reach the scanner (harness-bot review)", () => {
692+
// altimate-harness-bot review, PR #1028 risk-tier.ts:251. The earlier
693+
// guard skipped only `+++`/`---`/`@@` unified-diff headers and
694+
// context (space-prefixed) lines; free-form lines from a raw
695+
// `git diff -p` output (`diff --git`, `index abc..def`, `Author:`,
696+
// `Date:`) fell through with a `""` marker and reached
697+
// `dbtRiskYmlKeyMatches`. In-production `file.diff` never carries
698+
// these — the GitHub PR files API strips them — so the risk was
699+
// defensive parity only, but exercise it here so a future caller
700+
// passing a raw diff can't silently over-promote.
701+
const diff =
702+
"diff --git a/models/intermediate/int_x.yml b/models/intermediate/int_x.yml\n" +
703+
"index abcdef1..1234567 100644\n" +
704+
"--- a/models/intermediate/int_x.yml\n" +
705+
"+++ b/models/intermediate/int_x.yml\n" +
706+
"@@ -1,3 +1,4 @@\n" +
707+
" models:\n" +
708+
" - name: mrt_x\n" +
709+
'+ description: "See tests documentation for the grain policy"\n'
710+
const r = classifyPR([file("models/intermediate/int_x.yml", diff)])
711+
// Same content as the bare-`tests`-word test below; must stay trivial
712+
// regardless of whether the caller included raw diff headers.
713+
expect(r.tier).toBe("trivial")
714+
})
715+
691716
test("R20 S4: bare `tests` (no colon) does NOT match the risk-key regex (word-boundary guard)", () => {
692717
// FP guard — a description line like `# tests explanation`, or a
693718
// yml value containing the string `tests` without a following colon,

0 commit comments

Comments
 (0)