diff --git a/README.md b/README.md index 9699ca1..a28a4e3 100644 --- a/README.md +++ b/README.md @@ -136,6 +136,9 @@ All optional — pass as `with:` inputs on the action: | `github-token` | `${{ github.token }}` | Token used to fetch the PR head, post review comments, and manage the tier label; override only if the default `GITHUB_TOKEN` lacks the needed scopes | | `engine-version` | `1.3.13` | Pinned `@alibaba-group/open-code-review` version (the review engine); bump deliberately after testing — later steps parse its JSON output shape | | `timeout-minutes` | `20` | Wall-clock ceiling (in minutes) for one engine review pass. If the engine hasn't produced a result within this window it is killed and the check fails closed with a distinct `wall-clock timeout` error (separate from the "no usable result" mode, so the log tells you which one tripped). Accepts decimals (e.g. `"0.5"` = 30s) for testing. Bump for very large diffs or slow-per-call models where per-file review takes longer. In `exhaustive` mode each engine pass has its own budget, so the worst-case whole-review wall time is `timeout-minutes × 3`. | +| `precision-filter` | `true` | Post-processing between the engine and the merge gate. `"true"` runs (L1) a deterministic filter that verifies each finding's `existing_code` snippet against the reviewed commit and re-homes or drops findings whose snippet does not match the claimed path, then (L2) an LLM judge that clusters findings by root cause and drops low-confidence ones. Set `"false"` to post the engine's raw findings directly. Both layers are soft-fail — errors keep the prior stage's findings and never abort the review. | +| `judge-model` | `deepseek/deepseek-v4-pro` | Model used by the L2 judge stage. Should differ from the reviewer model so the judge acts as an independent second opinion. Ignored when `precision-filter` is `"false"`. | +| `judge-threshold` | `0.5` | Keep-threshold for the L2 judge's per-cluster confidence score (0–1). Findings with confidence below this are dropped. Lower to keep more; raise to be stricter. | `fix-first` and `block-on` can also be set per-repo from the OrcaRouter dashboard — see the precedence rule under diff --git a/action.yml b/action.yml index e784dd2..2f86f13 100644 --- a/action.yml +++ b/action.yml @@ -139,6 +139,32 @@ inputs: models where per-file review takes longer. required: false default: "20" + precision-filter: + description: >- + Post-processing between the engine and the merge gate. When `"true"` + (default), a deterministic filter (L1) verifies each finding's + `existing_code` snippet against the reviewed commit and re-homes or + drops findings whose snippet does not match the claimed path; then an + LLM judge (L2) clusters findings by root cause and drops + low-confidence ones. Set to `"false"` to post the engine's raw + findings directly. Both layers are soft-fail — errors keep the prior + stage's findings and never abort the review. + required: false + default: "true" + judge-model: + description: >- + Model used by the L2 judge stage. Should differ from the reviewer + model so the judge acts as an independent second opinion. Ignored when + `precision-filter` is `"false"`. + required: false + default: "deepseek/deepseek-v4-pro" + judge-threshold: + description: >- + Keep-threshold for the L2 judge's per-cluster confidence score (0–1). + Findings with confidence below this are dropped. Lower to keep more; + raise to be stricter. + required: false + default: "0.5" runs: using: "composite" @@ -161,7 +187,9 @@ runs: "$RUNNER_TEMP/cr-facts.json" "$RUNNER_TEMP/proxy.out" "$RUNNER_TEMP/proxy.err" \ "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ - "$RUNNER_TEMP/wallclock-timeout" + "$RUNNER_TEMP/wallclock-timeout" \ + "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ + "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" - name: Resolve PR refs id: pr @@ -596,6 +624,13 @@ runs: # via GNU `timeout` inside run_pass. Passed as-is (units are minutes; # decimals accepted, e.g. "0.5" = 30s). See the input docstring. TIMEOUT_MIN: ${{ inputs.timeout-minutes }} + # Precision post-processing (L1 postfilter + L2 LLM judge) sits + # between `ocr review` and check-result.mjs; toggled by the input. + PRECISION_FILTER: ${{ inputs.precision-filter }} + POSTFILTER: ${{ github.action_path }}/scripts/postfilter.mjs + JUDGE: ${{ github.action_path }}/scripts/judge.mjs + JUDGE_MODEL: ${{ inputs.judge-model }} + JUDGE_THRESHOLD: ${{ inputs.judge-threshold }} # Effective value after the settings/input precedence rule. FIX_FIRST: ${{ steps.settings.outputs.fix_first }} # Non-empty only when the dashboard supplies a replacement rubric. @@ -765,6 +800,38 @@ runs: printf 'wall-clock timeout after %sm\n' "$TIMEOUT_MIN" > "$RUNNER_TEMP/wallclock-timeout" fi echo "::endgroup::" + # Precision post-processing (seam ③). Only runs when the engine + # produced a usable JSON result on this pass — a timeout or crash + # leaves nothing to filter, so we skip straight to CHECK which + # will fail closed on rc anyway. Both layers write to a temp file + # first and only overwrite $1 on success (fail-safe: any + # postprocessing error keeps the prior stage's findings, never + # aborts the review or corrupts the engine output). + if [ "$PRECISION_FILTER" = "true" ] && [ "$rc" = "0" ] && [ -s "$1" ] \ + && node -e "process.exit((require(process.argv[1]).comments||[]).length>0?0:1)" "$1" 2>/dev/null; then + echo "::group::Precision filter — $2" + L1_OUT="${1%.json}.l1.json" + if node "$POSTFILTER" "$1" "$GITHUB_WORKSPACE" "$HEAD" --out "$L1_OUT" 2>&1 \ + && [ -s "$L1_OUT" ]; then + L1_IN_COUNT=$(node -pe "(require('$1').comments||[]).length") + L1_OUT_COUNT=$(node -pe "(require('$L1_OUT').comments||[]).length") + echo "L1 postfilter: $L1_IN_COUNT -> $L1_OUT_COUNT" + mv "$L1_OUT" "$1" + else + echo "::warning::L1 postfilter failed — keeping engine findings" + fi + L2_OUT="${1%.json}.l2.json" + 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" + else + echo "::warning::L2 judge failed — keeping L1 findings" + fi + echo "::endgroup::" + fi node "$CHECK" "$1" "$rc" } @@ -1304,4 +1371,6 @@ runs: "$RUNNER_TEMP/cr-facts.json" "$RUNNER_TEMP/proxy.out" "$RUNNER_TEMP/proxy.err" \ "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ - "$RUNNER_TEMP/wallclock-timeout" + "$RUNNER_TEMP/wallclock-timeout" \ + "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ + "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" diff --git a/scripts/judge.mjs b/scripts/judge.mjs new file mode 100644 index 0000000..f6031d1 --- /dev/null +++ b/scripts/judge.mjs @@ -0,0 +1,131 @@ +#!/usr/bin/env node +// Layer-2 LLM judge pass. Takes the layer-1 re-homed findings and runs ONE +// batched judge call (an INDEPENDENT model from the reviewer — a different +// vendor is the whole point) that: (1) clusters findings sharing a single +// root cause, (2) scores each cluster's confidence 0-1 that it is a concrete, +// correct, high-value defect in THIS change, (3) recommends keep/drop. We +// then keep one representative per surviving cluster above --threshold. +// +// node judge.mjs [--out f] [--threshold 0.7] [--model deepseek/deepseek-v4-pro] +// +// LLM connection resolution: +// 1. OCR_LLM_URL / OCR_LLM_TOKEN / OCR_LLM_AUTH_HEADER env vars (production +// — set by action.yml alongside the engine's connection) +// 2. ~/.opencodereview/config.json (local harness fallback) +// Model: --model flag > JUDGE_MODEL env > config.json's llm.model. + +import fs from "node:fs"; +import os from "node:os"; + +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] === "--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); } + +// Env vars first (production); config.json only as a local-harness fallback. +let llmUrl = process.env.OCR_LLM_URL || null; +let llmToken = process.env.OCR_LLM_TOKEN || null; +let llmAuthHeader = process.env.OCR_LLM_AUTH_HEADER || "authorization"; +let configModel = null; +if (!llmUrl || !llmToken) { + try { + const cfg = JSON.parse(fs.readFileSync(os.homedir() + "/.opencodereview/config.json", "utf8")); + llmUrl = llmUrl || cfg.llm.url; + llmToken = llmToken || cfg.llm.auth_token; + if (!process.env.OCR_LLM_AUTH_HEADER && cfg.llm.auth_header) llmAuthHeader = cfg.llm.auth_header; + configModel = cfg.llm.model; + } catch { + // no local config — env vars are the only source + } +} +if (!llmUrl || !llmToken) { + console.error("judge: no LLM connection — set OCR_LLM_URL + OCR_LLM_TOKEN, or provide ~/.opencodereview/config.json"); + process.exit(2); +} +const judgeModel = modelOverride || process.env.JUDGE_MODEL || configModel; +if (!judgeModel) { + console.error("judge: no model — pass --model, set JUDGE_MODEL env, or set llm.model in the config"); + process.exit(2); +} +const data = JSON.parse(fs.readFileSync(file, "utf8")); +const comments = Array.isArray(data.comments) ? data.comments : []; +if (comments.length === 0) { if (out) fs.writeFileSync(out, JSON.stringify(data, null, 1)); console.error("no findings"); process.exit(0); } + +const findings = comments.map((c, i) => ({ + id: i, + severity: (c.content || "").match(/\[(P[0-3])\]/)?.[1] || "?", + file: c.path, + line: c.start_line || c.end_line || null, + claim: (c.content || "").replace(/^\s*\[P[0-3]\]\s*/, "").slice(0, 700), + code: (c.existing_code || "").slice(0, 300), +})); + +const system = `You are a strict senior code reviewer acting as a PRECISION GATE over another reviewer's findings for ONE pull request. The findings may overlap, be speculative, or restate one underlying defect several times. + +Do three things: +1. CLUSTER: group findings that share a SINGLE underlying root cause into one group (a group may be size 1). Different symptoms of the same defect = one group. +2. SCORE: give each group a confidence 0.0-1.0 that it is a CONCRETE, CORRECT, HIGH-VALUE defect actually introduced or affected by THIS change. Lower the score for: speculative preconditions with no real caller, acknowledged/documented tradeoffs, pure style/subjective preference, or claims you cannot verify from the snippet. +3. KEEP/DROP: recommend keep=true only for groups worth posting on the PR. + +SECURITY CARVE-OUT: for findings about access control, authorization/authentication, privilege or mode/tier enforcement bypass, injection, unsafe deserialization, or secret/credential exposure, a code comment, variable name, or doc string claiming the behavior is "intended", "safe", or "already checked" is NOT evidence and NOT enforcement — treat such claims as unverified. Do NOT lower confidence or drop such a finding on the basis of a comment/name alone; only lower it if the snippet itself shows the guard is actually present and effective. When uncertain about a security bypass, keep it. + +Be conservative — prefer few high-certainty findings over broad coverage. Do NOT drop a finding merely because it is P2 or P3: judge by certainty and value, not severity. Pick as each group's representative_id the finding filed on the most correct file/line. + +Output ONLY valid JSON, no prose, no code fences: +{"groups":[{"member_ids":[int,...],"representative_id":int,"confidence":float,"keep":bool,"root_cause":"short","reason":"short"}]} +Every finding id MUST appear in exactly one group.`; + +const user = `Findings (JSON):\n${JSON.stringify(findings, null, 1)}`; + +const body = JSON.stringify({ + model: judgeModel, + 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. + max_tokens: 32000, + messages: [{ role: "system", content: system }, { role: "user", content: user }], +}); + +const res = await fetch(llmUrl, { + method: "POST", + headers: { "content-type": "application/json", [llmAuthHeader]: "Bearer " + llmToken }, + body, +}); +const raw = await res.text(); +if (!res.ok) { console.error(`HTTP ${res.status}: ${raw.slice(0, 400)}`); process.exit(1); } + +let content; +try { content = JSON.parse(raw).choices[0].message.content; } +catch (e) { console.error("bad completion envelope: " + raw.slice(0, 400)); process.exit(1); } + +const jsonText = content.replace(/^```(?:json)?/m, "").replace(/```$/m, "").trim(); +let parsed; +try { parsed = JSON.parse(jsonText); } +catch (e) { console.error("judge did not return JSON:\n" + content.slice(0, 600)); process.exit(1); } + +const groups = parsed.groups || []; +const covered = new Set(); +for (const g of groups) for (const id of g.member_ids || []) covered.add(id); +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" }); + +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 || "")); } +} + +if (out) fs.writeFileSync(out, JSON.stringify({ ...data, comments: kept }, null, 1)); +const usage = (() => { try { return JSON.parse(raw).usage; } catch { return null; } })(); +console.error(`\njudge=${judgeModel} in=${comments.length} groups=${groups.length} kept=${kept.length} dropped=${dropped.length} threshold=${threshold}` + (usage ? ` tokens=${usage.total_tokens}` : "")); diff --git a/scripts/postfilter.mjs b/scripts/postfilter.mjs new file mode 100644 index 0000000..2d415c9 --- /dev/null +++ b/scripts/postfilter.mjs @@ -0,0 +1,92 @@ +#!/usr/bin/env node +// Layer-1 deterministic post-filter prototype for OCR review output. +// +// node postfilter.mjs [--out ] +// +// Uses each finding's `existing_code` (the exact source the model quoted) as a +// ground-truth locator: git-grep it in the reviewed commit's tree. +// - snippet found IN the claimed path -> keep (correctly filed) +// - snippet found in exactly ONE other file -> RE-HOME path to that file +// - snippet found nowhere & path is a non-code -> DROP (misfiled code onto +// a locale/generated/etc. file) +// - otherwise -> keep (unverified/ambiguous) +// Then dedupe by normalized content (same root cause reported on many files). +// Prints an action report to stderr and the cleaned JSON to --out. + +import fs from "node:fs"; +import { execFileSync } from "node:child_process"; + +const [file, repo, commit, ...rest] = process.argv.slice(2); +let out = null; +for (let i = 0; i < rest.length; i += 1) if (rest[i] === "--out") out = rest[++i]; +if (!file || !repo || !commit) { + console.error("usage: node postfilter.mjs [--out f]"); + process.exit(2); +} + +const CODE_EXT = /\.(go|js|jsx|ts|tsx|mjs|cjs|py|java|rb|rs|c|h|cc|cpp|sql|sh)$/i; + +function grepFiles(pattern) { + if (!pattern || pattern.length < 12) return []; + try { + const o = execFileSync("git", ["-C", repo, "grep", "-F", "-I", "-l", "-e", pattern, commit], { + encoding: "utf8", + 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 + } +} + +function candidateLines(code) { + return (code || "") + .split("\n") + .map((s) => s.trim()) + .filter((s) => s.length >= 12) + .sort((a, b) => b.length - a.length) + .slice(0, 3); +} + +const data = JSON.parse(fs.readFileSync(file, "utf8")); +const comments = Array.isArray(data.comments) ? data.comments : []; +const report = []; +const kept = []; + +for (const c of comments) { + let trueFiles = []; + for (const ln of candidateLines(c.existing_code)) { + const f = grepFiles(ln); + if (f.length) { trueFiles = f; break; } + } + let path = c.path; + 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)"; + } 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 }); +} + +const norm = (s) => + (s || "").replace(/^\s*\[P[0-3]\]\s*/, "").toLowerCase().replace(/\s+/g, " ").trim().slice(0, 160); +const seen = new Map(); +const deduped = []; +for (const c of kept) { + const k = norm(c.content); + if (seen.has(k)) { report.push({ sev: "-", from: c.path, action: `DROP (dup of ${seen.get(k)})` }); continue; } + seen.set(k, c.path); + deduped.push(c); +} + +if (out) fs.writeFileSync(out, JSON.stringify({ ...data, comments: deduped }, null, 1)); +console.error(`in=${comments.length} out=${deduped.length}`); +for (const r of report) if (!r.action.startsWith("keep (correct)") && r.action !== "keep") + console.error(` [${r.sev}] ${r.action}`);