diff --git a/action.yml b/action.yml index 2f86f13..3fd0b44 100644 --- a/action.yml +++ b/action.yml @@ -821,15 +821,41 @@ runs: echo "::warning::L1 postfilter failed — keeping engine findings" fi L2_OUT="${1%.json}.l2.json" + # Snapshot the engine's policy-block CONTENT (not just presence) + # before the L2 sidecar. The judge call goes through fact-proxy, + # whose `recordPolicyBlock()` can OVERWRITE `$POLICY_BLOCK` if the + # judge itself hits a guardrail — a presence-only flag can't tell + # "engine block preserved" from "engine block silently replaced by + # L2 block". After the L2 call, if we snapshotted engine content + # we always restore it (both success and fail paths) so the + # engine's guardrail record wins over an L2 sidecar overwrite. + POLICY_BLOCK_SNAPSHOT="${POLICY_BLOCK}.pre-l2" + rm -f "$POLICY_BLOCK_SNAPSHOT" + if [ -f "$POLICY_BLOCK" ]; then cp -f "$POLICY_BLOCK" "$POLICY_BLOCK_SNAPSHOT"; fi if node "$JUDGE" "$1" --model "$JUDGE_MODEL" --threshold "$JUDGE_THRESHOLD" --out "$L2_OUT" 2>&1 \ && [ -s "$L2_OUT" ]; then L2_IN_COUNT=$(node -pe "(require('$1').comments||[]).length") L2_OUT_COUNT=$(node -pe "(require('$L2_OUT').comments||[]).length") echo "L2 judge ($JUDGE_MODEL, thr=$JUDGE_THRESHOLD): $L2_IN_COUNT -> $L2_OUT_COUNT" mv "$L2_OUT" "$1" + # L2 succeeded. Restore engine's block if we snapshotted one — + # covers the case where L2 was warn-mode guardrail-hit and + # overwrote engine's authoritative record mid-call. + if [ -f "$POLICY_BLOCK_SNAPSHOT" ]; then + mv -f "$POLICY_BLOCK_SNAPSHOT" "$POLICY_BLOCK" + fi else echo "::warning::L2 judge failed — keeping L1 findings" + # L2 failed. Restore engine's block if we snapshotted one; else + # clear any L2-authored block so a sidecar guardrail hit doesn't + # surface as the engine's own block on the primary review. + if [ -f "$POLICY_BLOCK_SNAPSHOT" ]; then + mv -f "$POLICY_BLOCK_SNAPSHOT" "$POLICY_BLOCK" + else + rm -f "$POLICY_BLOCK" + fi fi + unset POLICY_BLOCK_SNAPSHOT echo "::endgroup::" fi node "$CHECK" "$1" "$rc" diff --git a/scripts/judge.mjs b/scripts/judge.mjs index f6031d1..2a5f9c2 100644 --- a/scripts/judge.mjs +++ b/scripts/judge.mjs @@ -17,11 +17,34 @@ import fs from "node:fs"; import os from "node:os"; +// Parses a strict plain decimal (e.g. "0.7", "-1", "0.5"). Returns the number +// or NaN. Deliberately does NOT use parseFloat — parseFloat stops at the +// first non-numeric character so "0.8oops" would silently become 0.8 and +// "0x1" would become 0. The regex gate rejects any trailing garbage before +// Number() coerces, so callers can trust the returned value is either a +// clean decimal or NaN. +function parseStrictDecimal(raw) { + if (typeof raw !== "string" || !/^\s*-?\d+(?:\.\d+)?\s*$/.test(raw)) return NaN; + return Number(raw); +} + const [file, ...rest] = process.argv.slice(2); let out = null, threshold = 0.7, modelOverride = null; for (let i = 0; i < rest.length; i += 1) { if (rest[i] === "--out") out = rest[++i]; - else if (rest[i] === "--threshold") threshold = parseFloat(rest[++i]); + else if (rest[i] === "--threshold") { + const rawT = rest[++i]; + const parsed = parseStrictDecimal(rawT); + if (!Number.isFinite(parsed)) { + console.error(`judge: --threshold must be a plain decimal, got ${JSON.stringify(rawT)}`); + process.exit(2); + } + if (parsed < 0 || parsed > 1) { + console.error(`judge: --threshold must be in [0,1], got ${JSON.stringify(rawT)}`); + process.exit(2); + } + threshold = parsed; + } else if (rest[i] === "--model") modelOverride = rest[++i]; } if (!file) { console.error("usage: node judge.mjs [--out f] [--threshold 0.7] [--model deepseek/deepseek-v4-pro]"); process.exit(2); } @@ -86,8 +109,8 @@ const body = JSON.stringify({ temperature: 0, // Scales with the finding count (~250 tokens per cluster JSON entry — // member_ids + representative_id + confidence + keep + root_cause + reason — - // plus a bit of overhead). 8k tokens truncated the response at 48 findings - // (observed on minimax runs on 80bffaa72); 32k gives headroom well past 100. + // plus a bit of overhead). 32k gives headroom well past 100 findings; 8k + // truncated the response on large batches in earlier testing. max_tokens: 32000, messages: [{ role: "system", content: system }, { role: "user", content: user }], }); @@ -112,18 +135,65 @@ catch (e) { console.error("judge did not return JSON:\n" + content.slice(0, 600) const groups = parsed.groups || []; const covered = new Set(); for (const g of groups) for (const id of g.member_ids || []) covered.add(id); +// Fail-open for findings the judge did not classify into any group: mark +// them keep with a synthetic confidence that survives ANY user-set +// threshold, so raising the threshold does not silently drop the judge's +// blind spots (the previous 0.5 constant fails closed at threshold > 0.5, +// contradicting the "fail-open" comment). for (let i = 0; i < findings.length; i += 1) if (!covered.has(i)) - groups.push({ member_ids: [i], representative_id: i, confidence: 0.5, keep: true, root_cause: "(uncovered)", reason: "not classified by judge; kept fail-open" }); + groups.push({ member_ids: [i], representative_id: i, confidence: 1.0, keep: true, root_cause: "(uncovered)", reason: "not classified by judge; kept fail-open above any threshold" }); const kept = []; const dropped = []; for (const g of groups) { - const surv = g.keep && g.confidence >= threshold; - const rep = comments[g.representative_id] ?? comments[g.member_ids[0]]; - const others = (g.member_ids || []).filter((id) => id !== (g.representative_id ?? g.member_ids[0])); - const line = `[conf ${g.confidence?.toFixed(2)}] ${(rep?.content || "").match(/\[(P[0-3])\]/)?.[0] || ""} ${rep?.path} :: ${g.root_cause} ${others.length ? "(merged " + others.length + ")" : ""}`; - if (surv) { kept.push(rep); console.error("KEEP " + line); } - else { dropped.push(g); console.error("drop " + line + " — " + (g.reason || "")); } + // Coerce confidence — the judge sometimes serializes it as a JSON string + // (e.g. "0.95") on schema drift. Type-guard first: `Number()` will happily + // return 1 for `true` and 0 for `false`/`null`/`[]`, and 1 for `[1]`, any of + // which would slip past ANY threshold and bypass the whole gate. Accept + // only real numbers or numeric strings; anything else becomes NaN and the + // group is dropped. Then reject values outside the documented [0,1] range + // outright rather than clamping — a stray `2` is a schema violation, and + // silently clamping it to `1` (max confidence) lets an invalid response + // survive every threshold. Match the invalid-type policy: violation → drop. + const rawConfInput = g.confidence; + const rawConf = + typeof rawConfInput === "number" + ? rawConfInput + : typeof rawConfInput === "string" + ? parseStrictDecimal(rawConfInput) + : NaN; + const boundedConf = + Number.isFinite(rawConf) && rawConf >= 0 && rawConf <= 1 ? rawConf : NaN; + // Coerce `keep` strictly — same schema-drift class as `confidence` above. + // Truthy JS treats the STRING `"false"` as true, so a raw `&& g.keep` would + // retain a group the judge intended to drop when the LLM stringifies the + // boolean. Accept only real `true` or the literal string "true" (case- + // insensitive); anything else drops the group as a safe default. + const keepInput = g.keep; + const groupKeep = + keepInput === true || + (typeof keepInput === "string" && keepInput.trim().toLowerCase() === "true"); + const surv = groupKeep && Number.isFinite(boundedConf) && boundedConf >= threshold; + // Resolve the representative comment by trying the primary id then every + // member_id — a malformed group whose representative_id is out of range + // must still surface a valid member, otherwise the coverage pass has + // already marked those members as "handled" and the whole group would + // vanish silently along with real findings. + const candidateIds = [g.representative_id, ...(Array.isArray(g.member_ids) ? g.member_ids : [])]; + let repId = null; + for (const id of candidateIds) { + if (Number.isInteger(id) && id >= 0 && id < comments.length && comments[id]) { repId = id; break; } + } + const rep = repId != null ? comments[repId] : null; + const others = Array.isArray(g.member_ids) + ? g.member_ids.filter((id) => id !== repId) + : []; + const line = `[conf ${Number.isFinite(boundedConf) ? boundedConf.toFixed(2) : "?"}] ${(rep?.content || "").match(/\[(P[0-3])\]/)?.[0] || ""} ${rep?.path ?? "?"} :: ${g.root_cause} ${others.length ? "(merged " + others.length + ")" : ""}`; + if (surv) { + if (!rep) { console.error("skip malformed judge group (no valid rep): " + JSON.stringify(g).slice(0, 120)); continue; } + kept.push(rep); + console.error("KEEP " + line); + } else { dropped.push(g); console.error("drop " + line + " — " + (g.reason || "")); } } if (out) fs.writeFileSync(out, JSON.stringify({ ...data, comments: kept }, null, 1)); diff --git a/scripts/postfilter.mjs b/scripts/postfilter.mjs index 2d415c9..eecfc38 100644 --- a/scripts/postfilter.mjs +++ b/scripts/postfilter.mjs @@ -24,8 +24,34 @@ if (!file || !repo || !commit) { process.exit(2); } -const CODE_EXT = /\.(go|js|jsx|ts|tsx|mjs|cjs|py|java|rb|rs|c|h|cc|cpp|sql|sh)$/i; +// Only drop findings whose snippet did NOT match anywhere in the tree when +// the claimed path is a KNOWN non-code file (locale JSON / doc markdown / +// changelogs / lockfiles / images / binaries). Extensionless code files +// (Dockerfile, Makefile, scripts with no extension) previously fell through +// the "not code-ext" branch and got dropped as if they were locale files — +// reverse the polarity so only clearly-non-code paths incur the drop. +// +// Extensions that are ALWAYS non-code (docs, media, binaries, generated). +const DEFINITELY_NON_CODE_EXT = /\.(md|txt|lock|log|csv|tsv|po|pot|properties|map|png|jpe?g|gif|svg|ico|webp|pdf|woff2?|ttf|eot|otf|zip|tar|gz|tgz|bin|exe|dll|so|dylib|class|jar|wasm|mp3|mp4|mov|wav)$/i; +// JSON / YAML are ambiguous: action.yml, GitHub workflows, package.json, +// tsconfig.json, k8s / IaC manifests are all reviewable configuration. Only +// treat a .json / .yml / .yaml path as non-code when the path itself +// signals "lockfile", "locale/translation bundle", or "generated build +// output" — anything else keeps the finding for L2 to judge. +const CONVENTIONAL_NON_CODE_JSON_YAML_PATH = + /(?:(?:^|\/)(?:package-lock|pnpm-lock)\.(?:json|ya?ml))|(?:[-.]lock\.(?:json|ya?ml)$)|(?:(?:^|\/)(?:locales?|i18n|translations|messages|dist|build|generated|out|coverage)\/)/i; +function isKnownNonCode(p) { + if (!p) return false; + if (DEFINITELY_NON_CODE_EXT.test(p)) return true; + if (!/\.(?:json|ya?ml)$/i.test(p)) return false; + return CONVENTIONAL_NON_CODE_JSON_YAML_PATH.test(p); +} +// Returns a de-duped list of files containing `pattern` at the reviewed +// commit. Uses `-l` (file-list only) so we sidestep the colon-in-filename +// ambiguity of `-n`'s ":::" format — with `-l` +// the output is just ":" and stripping the commit prefix is +// unambiguous. function grepFiles(pattern) { if (!pattern || pattern.length < 12) return []; try { @@ -34,9 +60,34 @@ function grepFiles(pattern) { maxBuffer: 1 << 24, }); return [...new Set(o.split("\n").filter(Boolean).map((l) => l.slice(l.indexOf(":") + 1)))]; - } catch { - return []; // grep exit 1 = no match - } + } catch { return []; } +} + +// Returns line numbers where `pattern` matches inside a specific `file` at +// the commit. We pass `file` as an explicit git pathspec after `--`, so +// git-grep's output has an exact `::` prefix we can strip +// unambiguously — the colon-in-filename concern from raw `-n` output does +// not apply here because we already know the file. +function grepLinesIn(pattern, file) { + if (!pattern || pattern.length < 12) return []; + try { + const o = execFileSync( + "git", + ["-C", repo, "grep", "-F", "-I", "-n", "-e", pattern, commit, "--", file], + { encoding: "utf8", maxBuffer: 1 << 24 }, + ); + const prefix = `${commit}:${file}:`; + const lines = []; + for (const raw of o.split("\n")) { + if (!raw || !raw.startsWith(prefix)) continue; + const rest = raw.slice(prefix.length); + const colon = rest.indexOf(":"); + if (colon < 0) continue; + const n = Number(rest.slice(0, colon)); + if (Number.isFinite(n)) lines.push(n); + } + return lines; + } catch { return []; } } function candidateLines(code) { @@ -54,25 +105,66 @@ const report = []; const kept = []; for (const c of comments) { - let trueFiles = []; + // Try each candidate line from `existing_code` until one has hits. + // Remember which candidate matched so we can look up its line numbers + // in the rehome target with a second targeted grep. + let files = []; + let matchedPattern = null; for (const ln of candidateLines(c.existing_code)) { const f = grepFiles(ln); - if (f.length) { trueFiles = f; break; } + if (f.length) { files = f; matchedPattern = ln; break; } } let path = c.path; + let start_line = c.start_line; + let end_line = c.end_line; let action = "keep"; - if (trueFiles.length) { - if (trueFiles.includes(c.path)) action = "keep (correct)"; - else if (trueFiles.length === 1) { path = trueFiles[0]; action = `REHOME ${c.path} -> ${path}`; } - else action = `keep (ambiguous: ${trueFiles.length} files)`; - } else if (!CODE_EXT.test(c.path)) { - action = "DROP (code snippet, filed on non-code file, not found)"; + if (files.length) { + if (files.includes(c.path)) action = "keep (correct)"; + else if (files.length === 1) { + // Single file elsewhere. Fetch the actual line NUMBERS in that file + // to update `start_line`/`end_line` on rehome — but only if the hit + // is unambiguous (exactly one line). Multiple hits in the same file + // mean the snippet appears more than once, so we cannot pick a + // single line to post on — treat as ambiguous and leave the finding + // on its original path. + const target = files[0]; + const targetLines = matchedPattern ? grepLinesIn(matchedPattern, target) : []; + if (targetLines.length === 1) { + path = target; + start_line = targetLines[0]; + end_line = targetLines[0]; + action = `REHOME ${c.path} -> ${path}:${targetLines[0]}`; + } else if (targetLines.length > 1) { + // The snippet appears more than once in the target file so we cannot + // pick a single line to post on — but L1 has still proven the code + // lives in `target`, NOT in `c.path`. Rehome the path (dropping the + // now-inapplicable line) rather than leaving the finding on a + // known-wrong file just because the line is ambiguous. + path = target; + start_line = null; + end_line = null; + action = `REHOME ${c.path} -> ${path} (line ambiguous: ${targetLines.length} hits)`; + } else { + // Line lookup returned nothing (rare — `-l` said match exists). + // Rehome the path but clear the line: the stale line came from the + // wrong file (`c.path`) and would either post the comment on an + // unrelated line in `target`, or trip GitHub's inline-comment range + // validation and reject the whole review. + path = target; + start_line = null; + end_line = null; + action = `REHOME ${c.path} -> ${path} (line unresolved)`; + } + } + else action = `keep (ambiguous: ${files.length} files)`; + } else if (isKnownNonCode(c.path)) { + action = "DROP (code snippet, filed on known-non-code file, not found)"; } else { action = "keep (snippet unverified)"; } report.push({ sev: (c.content || "").match(/\[(P[0-3])\]/)?.[1] || "?", from: c.path, action }); if (action.startsWith("DROP")) continue; - kept.push({ ...c, path }); + kept.push({ ...c, path, start_line, end_line }); } const norm = (s) =>