feat(review): pre-existing findings marked from git and re-derived by the gate
This commit is contained in:
parent
ce7dbc27a9
commit
1464605e35
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
7 changed files with 1015 additions and 5 deletions
|
|
@ -625,6 +625,17 @@ run (no JSON block) — stop. Never fill an id by hand. The frontmatter `finding
|
|||
must use **block-style YAML** (one ID per line, ` - ` prefix). The
|
||||
parser at `lib/util/frontmatter.mjs` does not support flow-style arrays.
|
||||
|
||||
**Then finalize the review** — one deterministic pass after `fill-finding-ids`:
|
||||
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-finalize.mjs --review "{project_dir}/review.md" --repo . --project "{project_dir}" --round 1
|
||||
```
|
||||
|
||||
On exit 1 or 2 the file stays as `fill-finding-ids` wrote it; print the reason in one line and continue.
|
||||
A finding is pre-existing when it sits on a line the range `scope_sha_start..scope_sha_end` did not add or
|
||||
change; finalize marks it from git, the verdict gate re-derives the mark, and a BLOCKER or security finding
|
||||
counts in full whatever the mark.
|
||||
|
||||
**The verdict is in the file (K7).** The frontmatter carries
|
||||
`verdict: ALLOW | WARN | BLOCK`, equal to the trailing JSON block's
|
||||
`verdict`. It is required at write time; a review without it fails Phase 8.
|
||||
|
|
|
|||
318
lib/review/pre-existing.mjs
Normal file
318
lib/review/pre-existing.mjs
Normal file
|
|
@ -0,0 +1,318 @@
|
|||
// lib/review/pre-existing.mjs
|
||||
// Pre-existing findings (review_version 1.2, fix round 4.5b2 part B): a
|
||||
// finding on a line the review range did not add or change. The mark is read
|
||||
// from git and never trusted as written: `lib/review/review-finalize.mjs`
|
||||
// writes it (`markFindings`), and the verdict gate re-derives it
|
||||
// (`preExistingCheck`) before a MAJOR leaves the answer set. A BLOCKER or a
|
||||
// security finding counts in full whatever the mark (lib/review/review-answers.mjs
|
||||
// `countingMajors`); every uncertain case counts as new.
|
||||
//
|
||||
// The line sets come from one `git diff -U0` of `scope_sha_start..scope_sha_end`
|
||||
// with flags that no repo config changes (`--no-color`, `--no-ext-diff`,
|
||||
// `--no-textconv`, `--no-relative`, `--find-renames`, explicit prefixes,
|
||||
// `--ignore-submodules=none`, `core.quotePath=false`):
|
||||
// - added or changed lines are `c..c+d-1` of each `@@ -a,b +c,d @@`;
|
||||
// - a pure deletion (`d = 0`) marks lines `c` and `c+1` (both sides of it);
|
||||
// - a new or binary file is `'all'`;
|
||||
// - a renamed or copied file goes into `renamed` under both names.
|
||||
// Any git failure, ENOBUFS, a range whose start is not a proper ancestor of its
|
||||
// end, or a line the reader cannot parse (a quoted path included) is
|
||||
// `{ok:false}`: then nothing is pre-existing.
|
||||
//
|
||||
// A finding's path and line must also be in the tree at `scope_sha_end`: the
|
||||
// path, byte for byte, names a regular file there (mode 100644 or 100755, read
|
||||
// once with `git ls-tree -r -z --full-tree`), and the line lies inside that
|
||||
// file's line count. A path the diff does not name is otherwise taken as
|
||||
// untouched; this check keeps a case variant (`A.mjs` on a case-insensitive
|
||||
// disk), a subdirectory-relative path, a missing file or a line past the end of
|
||||
// the file from passing as pre-existing. Any git failure here also means
|
||||
// nothing is pre-existing.
|
||||
//
|
||||
// Known limit: `scope_sha_start` is read from review.md and is not bound to
|
||||
// anything the gate checks (`scope_sha_end` is, by `checkReviewedCommit` in
|
||||
// lib/review/reviewed-commit.mjs). A forged narrower start, for example a
|
||||
// later commit of the same run, shrinks the range, so a line the run added can
|
||||
// look pre-existing for a MAJOR finding. BLOCKER and security findings are
|
||||
// never affected: they count in full whatever the mark.
|
||||
//
|
||||
// No CLI of its own.
|
||||
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { FULL_SHA_RE, reviewedSha } from './reviewed-commit.mjs';
|
||||
|
||||
const MAX_BUFFER = 64 * 1024 * 1024;
|
||||
const HUNK_RE = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(?: .*)?$/;
|
||||
const REGULAR_MODES = new Set(['100644', '100755']);
|
||||
const TREE_ENTRY_RE = /^(\d{6}) ([a-z]+) ([0-9a-f]{40}|[0-9a-f]{64})$/;
|
||||
const HEADER_LINE_RE = /^(?:old mode \d+|new mode \d+|deleted file mode \d+|new file mode \d+|index [0-9a-f]+\.\.[0-9a-f]+(?: \d+)?|similarity index \d+%|dissimilarity index \d+%)$/;
|
||||
|
||||
// The argv git helper of lib/review/reviewed-commit.mjs:64-66 (copied, not exported there).
|
||||
function git(repo, args) {
|
||||
return spawnSync('git', args, { cwd: repo, encoding: 'utf8', maxBuffer: MAX_BUFFER });
|
||||
}
|
||||
|
||||
// The `notMeasured` shape of lib/review/reviewed-commit.mjs:72-75 (copied).
|
||||
function notMeasured(reason) {
|
||||
return { ok: false, reason: `pre-existing not measured: ${reason}` };
|
||||
}
|
||||
|
||||
function gitFailed(what, r) {
|
||||
const note = r.error?.code === 'ENOBUFS' ? 'output past the 64 MiB buffer'
|
||||
: (r.error?.message ?? String(r.stderr ?? '').trim().split('\n')[0]);
|
||||
return notMeasured(`git ${what} failed (${note || `exit ${r.status}`})`);
|
||||
}
|
||||
|
||||
/** A frontmatter sha field as written (the `reviewedSha` reading, for any key): a full sha, else ''. */
|
||||
function frontmatterSha(text, key) {
|
||||
const fm = /^---\r?\n([\s\S]*?)\r?\n---\r?\n/.exec(String(text ?? ''));
|
||||
const re = new RegExp(`^${key}:[ \\t]*(["']?)([^"'\\s#]*)\\1[ \\t]*(?:#.*)?$`, 'm');
|
||||
const m = fm && re.exec(fm[1]);
|
||||
return m && FULL_SHA_RE.test(m[2]) ? m[2] : '';
|
||||
}
|
||||
|
||||
/**
|
||||
* The review's range, read raw from the frontmatter text as `reviewedSha` reads
|
||||
* `scope_sha_end`: a YAML number, `null` or a ref name is '' (no range, no mark).
|
||||
* @param {string} text - review.md
|
||||
* @returns {{ start: string, end: string }}
|
||||
*/
|
||||
export function scopeShas(text) {
|
||||
return { start: frontmatterSha(text, 'scope_sha_start'), end: reviewedSha(text) };
|
||||
}
|
||||
|
||||
/** A path as the diff prints it after its prefix; null when it is quoted (not read). */
|
||||
function unprefixed(path, prefix) {
|
||||
const p = path.replace(/\t$/, '');
|
||||
if (p.startsWith('"') || !p.startsWith(prefix) || p.length === prefix.length) return null;
|
||||
return p.slice(prefix.length);
|
||||
}
|
||||
|
||||
/** The one path of a `diff --git a/P b/P` header (both halves equal), else null. */
|
||||
function headerPath(rest) {
|
||||
if ((rest.length - 5) % 2 !== 0) return null;
|
||||
const n = (rest.length - 5) / 2;
|
||||
const p = rest.slice(2, 2 + n);
|
||||
return rest === `a/${p} b/${p}` && !p.startsWith('"') ? p : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a `git diff -U0` text into line sets.
|
||||
* @param {string} diff
|
||||
* @returns {{ok: true, files: Map<string, Set<number>|'all'>, renamed: Set<string>} | {ok: false, reason: string}}
|
||||
*/
|
||||
export function parseDiff(diff) {
|
||||
const files = new Map();
|
||||
const renamed = new Set();
|
||||
let cur = null;
|
||||
let minus = 0;
|
||||
let plus = 0;
|
||||
const bad = (line) => notMeasured(`cannot read diff line ${JSON.stringify(line.slice(0, 120))}`);
|
||||
const finish = () => {
|
||||
if (!cur) return null;
|
||||
const path = cur.to ?? cur.plusPath ?? cur.header;
|
||||
if (cur.from) renamed.add(cur.from);
|
||||
if (cur.to) renamed.add(cur.to);
|
||||
if (cur.deleted) return null;
|
||||
if (!path) return 'no path';
|
||||
if (cur.isNew || cur.binary) { files.set(path, 'all'); return null; }
|
||||
const prev = files.get(path);
|
||||
if (prev === 'all') return null;
|
||||
const set = prev ?? new Set();
|
||||
for (const n of cur.lines) set.add(n);
|
||||
files.set(path, set);
|
||||
return null;
|
||||
};
|
||||
const lines = String(diff).split('\n');
|
||||
if (lines.at(-1) === '') lines.pop();
|
||||
for (const line of lines) {
|
||||
if (minus > 0 || plus > 0) {
|
||||
if (line.startsWith('-') && minus > 0) { minus--; continue; }
|
||||
if (line.startsWith('+') && plus > 0) { plus--; continue; }
|
||||
if (line.startsWith('\\')) continue;
|
||||
return bad(line);
|
||||
}
|
||||
if (line.startsWith('\\')) continue;
|
||||
if (line.startsWith('diff --git ')) {
|
||||
if (finish()) return bad(line);
|
||||
cur = { header: headerPath(line.slice('diff --git '.length)), lines: [] };
|
||||
continue;
|
||||
}
|
||||
if (!cur) return bad(line);
|
||||
let m;
|
||||
if (HEADER_LINE_RE.test(line)) {
|
||||
if (line.startsWith('new file mode')) cur.isNew = true;
|
||||
if (line.startsWith('deleted file mode')) cur.deleted = true;
|
||||
} else if ((m = /^(rename|copy) (from|to) (.+)$/.exec(line))) {
|
||||
if (m[3].startsWith('"')) return bad(line);
|
||||
cur[m[2]] = m[3];
|
||||
} else if (line.startsWith('--- ')) {
|
||||
const p = line.slice(4);
|
||||
if (p !== '/dev/null' && unprefixed(p, 'a/') === null) return bad(line);
|
||||
} else if (line.startsWith('+++ ')) {
|
||||
const p = line.slice(4);
|
||||
if (p === '/dev/null') cur.deleted = true;
|
||||
else {
|
||||
const q = unprefixed(p, 'b/');
|
||||
if (q === null) return bad(line);
|
||||
cur.plusPath = q;
|
||||
}
|
||||
} else if (/^Binary files .* differ$/.test(line)) {
|
||||
cur.binary = true;
|
||||
} else if ((m = HUNK_RE.exec(line))) {
|
||||
const b = m[2] === undefined ? 1 : Number(m[2]);
|
||||
const c = Number(m[3]);
|
||||
const d = m[4] === undefined ? 1 : Number(m[4]);
|
||||
if (d === 0) cur.lines.push(c, c + 1);
|
||||
else for (let n = c; n < c + d; n++) cur.lines.push(n);
|
||||
minus = b;
|
||||
plus = d;
|
||||
} else {
|
||||
return bad(line);
|
||||
}
|
||||
}
|
||||
if (minus > 0 || plus > 0) return notMeasured('the diff ends inside a hunk');
|
||||
if (finish()) return notMeasured('a file in the diff has no path the reader can name');
|
||||
return { ok: true, files, renamed };
|
||||
}
|
||||
|
||||
/**
|
||||
* The regular files of the tree at `end`, by exact repo-root path: path -> blob id.
|
||||
* @returns {{ok: true, blobs: Map<string, string>} | {ok: false, reason: string}}
|
||||
*/
|
||||
function regularFilesAt(repo, end) {
|
||||
const r = git(repo, ['ls-tree', '-r', '-z', '--full-tree', end]);
|
||||
if (r.error || r.status !== 0) return gitFailed('ls-tree', r);
|
||||
const blobs = new Map();
|
||||
for (const entry of r.stdout.split('\0')) {
|
||||
if (entry === '') continue;
|
||||
const tab = entry.indexOf('\t');
|
||||
const m = tab === -1 ? null : TREE_ENTRY_RE.exec(entry.slice(0, tab));
|
||||
if (!m) return notMeasured(`cannot read ls-tree entry ${JSON.stringify(entry.slice(0, 120))}`);
|
||||
if (m[2] === 'blob' && REGULAR_MODES.has(m[1])) blobs.set(entry.slice(tab + 1), m[3]);
|
||||
}
|
||||
return { ok: true, blobs };
|
||||
}
|
||||
|
||||
/** The line count of one blob (a last line without a newline counts); null when git fails. */
|
||||
function blobLineCount(repo, blob) {
|
||||
const r = spawnSync('git', ['cat-file', 'blob', blob], { cwd: repo, maxBuffer: MAX_BUFFER });
|
||||
if (r.error || r.status !== 0 || !Buffer.isBuffer(r.stdout)) return null;
|
||||
const b = r.stdout;
|
||||
let n = 0;
|
||||
for (let i = b.indexOf(10); i !== -1; i = b.indexOf(10, i + 1)) n++;
|
||||
if (b.length > 0 && b[b.length - 1] !== 10) n++;
|
||||
return n;
|
||||
}
|
||||
|
||||
/**
|
||||
* The lines `start..end` added or changed, per file (repo-root paths), plus
|
||||
* `lineCount(path)`: the line count of a regular file at `end` under exactly
|
||||
* that path, or null when there is none (or git fails). Full commit ids only;
|
||||
* `start` must be a proper ancestor of `end`.
|
||||
* @param {string} repo
|
||||
* @param {string} start
|
||||
* @param {string} end
|
||||
* @returns {{ok: true, files: Map<string, Set<number>|'all'>, renamed: Set<string>, lineCount: (path: string) => number|null} | {ok: false, reason: string}}
|
||||
*/
|
||||
export function changedLineSets(repo, start, end) {
|
||||
if (!repo) return notMeasured('no repo');
|
||||
if (!FULL_SHA_RE.test(String(start ?? '')) || !FULL_SHA_RE.test(String(end ?? ''))) {
|
||||
return notMeasured('scope_sha_start and scope_sha_end must be full commit ids');
|
||||
}
|
||||
if (start.toLowerCase() === end.toLowerCase()) return notMeasured('an empty range (start = end) proves nothing');
|
||||
const anc = git(repo, ['merge-base', '--is-ancestor', start, end]);
|
||||
if (anc.error || (anc.status !== 0 && anc.status !== 1)) return gitFailed('merge-base --is-ancestor', anc);
|
||||
if (anc.status === 1) return notMeasured(`${start.slice(0, 12)} is not an ancestor of ${end.slice(0, 12)}`);
|
||||
const r = git(repo, ['-c', 'core.quotePath=false', 'diff', '-U0', '--no-color', '--no-ext-diff', '--no-textconv',
|
||||
'--no-relative', '--find-renames', '--src-prefix=a/', '--dst-prefix=b/', '--ignore-submodules=none', start, end]);
|
||||
if (r.error || r.status !== 0) return gitFailed('diff', r);
|
||||
const sets = parseDiff(r.stdout);
|
||||
if (!sets.ok) return sets;
|
||||
const tree = regularFilesAt(repo, end);
|
||||
if (!tree.ok) return tree;
|
||||
const counts = new Map();
|
||||
sets.lineCount = (path) => {
|
||||
if (typeof path !== 'string' || !tree.blobs.has(path)) return null;
|
||||
if (!counts.has(path)) counts.set(path, blobLineCount(repo, tree.blobs.get(path)));
|
||||
return counts.get(path);
|
||||
};
|
||||
return sets;
|
||||
}
|
||||
|
||||
/** A repo-relative posix path as the diff names it: no `./`, `..`, empty segment, leading `/` or backslash. */
|
||||
function plainPath(p) {
|
||||
return typeof p === 'string' && p !== '' && !p.includes('\\')
|
||||
&& p.split('/').every((s) => s !== '' && s !== '.' && s !== '..');
|
||||
}
|
||||
|
||||
/**
|
||||
* True only when the sets are ok, `f.line >= 1`, the file is not renamed, its
|
||||
* entry is not `'all'`, `f.line` is not in its set, and `f.file` is a regular
|
||||
* file at `end` under exactly that path with at least `f.line` lines
|
||||
* (`sets.lineCount`; sets without it are never pre-existing).
|
||||
* @param {{file?: string, line?: number}} f
|
||||
* @param {ReturnType<typeof changedLineSets>} sets
|
||||
* @returns {boolean}
|
||||
*/
|
||||
export function isPreExisting(f, sets) {
|
||||
if (!sets || sets.ok !== true) return false;
|
||||
if (!f || !Number.isInteger(f.line) || f.line < 1 || !plainPath(f.file)) return false;
|
||||
if (sets.renamed.has(f.file)) return false;
|
||||
const entry = sets.files.get(f.file);
|
||||
if (entry === 'all') return false;
|
||||
if (entry && entry.has(f.line)) return false;
|
||||
if (typeof sets.lineCount !== 'function') return false;
|
||||
const n = sets.lineCount(f.file);
|
||||
return Number.isInteger(n) && f.line <= n;
|
||||
}
|
||||
|
||||
/** True when `file` has uncommitted changes, or git cannot say (the stricter side). */
|
||||
function uncommitted(repo, file) {
|
||||
const r = git(repo, ['status', '--porcelain=v1', '-z', '--', `:(top,literal)${file}`]);
|
||||
return Boolean(r.error) || r.status !== 0 || r.stdout !== '';
|
||||
}
|
||||
|
||||
/**
|
||||
* Set `pre_existing` (a boolean) on every JSON finding of `payload`, in place.
|
||||
* With no repo or no full `start`/`end`, every mark is false; a file with
|
||||
* uncommitted changes is false.
|
||||
* @param {{findings?: object[]}} payload
|
||||
* @param {{ repo?: string, start?: string, end?: string }} range
|
||||
* @returns {object} payload
|
||||
*/
|
||||
export function markFindings(payload, { repo, start, end } = {}) {
|
||||
const findings = Array.isArray(payload?.findings) ? payload.findings : [];
|
||||
const sets = repo && start && end ? changedLineSets(repo, start, end) : notMeasured('no range');
|
||||
const dirty = new Map();
|
||||
for (const f of findings) {
|
||||
if (!f || typeof f !== 'object') continue;
|
||||
let pre = isPreExisting(f, sets);
|
||||
if (pre) {
|
||||
if (!dirty.has(f.file)) dirty.set(f.file, uncommitted(repo, f.file));
|
||||
pre = !dirty.get(f.file);
|
||||
}
|
||||
f.pre_existing = pre;
|
||||
}
|
||||
return payload;
|
||||
}
|
||||
|
||||
/**
|
||||
* The gate's predicate for a review: true only when the finding says
|
||||
* `pre_existing: true` and `changedLineSets(repo, scope_sha_start,
|
||||
* scope_sha_end)` agrees through `isPreExisting`. With no repo, or no full
|
||||
* `scope_sha_start`, it is always false. The sets are read once, when a marked
|
||||
* finding is first asked about.
|
||||
* @param {string} text - review.md
|
||||
* @param {string|undefined} repo
|
||||
* @returns {(f: object) => boolean}
|
||||
*/
|
||||
export function preExistingCheck(text, repo) {
|
||||
const { start, end } = scopeShas(text);
|
||||
if (!repo || !start || !end) return () => false;
|
||||
let sets = null;
|
||||
return (f) => {
|
||||
if (f?.pre_existing !== true) return false;
|
||||
if (sets === null) sets = changedLineSets(repo, start, end);
|
||||
return isPreExisting(f, sets);
|
||||
};
|
||||
}
|
||||
|
|
@ -49,6 +49,7 @@ import { basename, dirname, join, resolve } from 'node:path';
|
|||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
import { reviewedSha } from './reviewed-commit.mjs';
|
||||
import { preExistingCheck } from './pre-existing.mjs';
|
||||
import {
|
||||
appendApproval, isHollowAnswer, logPath, normalizeSubject, readLog, verifyLog,
|
||||
} from '../util/approval-log.mjs';
|
||||
|
|
@ -214,7 +215,9 @@ function latestReviewRecord(records, subject) {
|
|||
|
||||
/**
|
||||
* Decide whether every counting MAJOR of a review is answered. Pure: the K2
|
||||
* records and the review's subject (its path relative to the git top level)
|
||||
* records, the review's subject (its path relative to the git top level) and
|
||||
* the `preExisting` predicate (the gate's is lib/review/pre-existing.mjs
|
||||
* `preExistingCheck`, re-derived from git; absent = nothing is pre-existing)
|
||||
* come in `ctx`; with no subject or no records nothing is answered.
|
||||
* @param {string} text - review.md
|
||||
* @param {{ records?: object[], reviewSubject?: string, preExisting?: (f) => boolean, archived?: unknown }} [ctx]
|
||||
|
|
@ -348,10 +351,12 @@ export function pendingFindings({ reviewPath, repo }) {
|
|||
const payload = payloadOf(text);
|
||||
if (!payload) return { exit: 2, reason: `${reviewPath} has no readable trailing JSON block` };
|
||||
const { records, top, problem } = answerRecords(repo ?? dirname(resolve(reviewPath)));
|
||||
const d = decideAnswers(text, { records, reviewSubject: top ? reviewSubject(top, reviewPath) : undefined });
|
||||
// The gate's pre-existing reading (lib/review/verdict-gate.mjs `decideOpenFindings`), so the list asks what the gate counts.
|
||||
const preExisting = preExistingCheck(text, repo ?? top ?? undefined);
|
||||
const d = decideAnswers(text, { records, reviewSubject: top ? reviewSubject(top, reviewPath) : undefined, preExisting });
|
||||
if (d.ok) return { exit: 0, pending: [] };
|
||||
const open = new Set([...d.unanswered, ...d.fixedPending]);
|
||||
const pending = countingMajors(payload).filter((f) => open.has(f.id)).map((f) => ({
|
||||
const pending = countingMajors(payload, { preExisting }).filter((f) => open.has(f.id)).map((f) => ({
|
||||
id: f.id, title: f.title ?? '', file: f.file ?? '', line: f.line ?? null,
|
||||
security: isSecurityFinding(f),
|
||||
fixed_pending: d.fixedPending.includes(f.id),
|
||||
|
|
|
|||
226
lib/review/review-finalize.mjs
Normal file
226
lib/review/review-finalize.mjs
Normal file
|
|
@ -0,0 +1,226 @@
|
|||
// lib/review/review-finalize.mjs
|
||||
// One deterministic pass over review.md after the coordinator and after
|
||||
// lib/review/fill-finding-ids.mjs, on both paths (review_version 1.2, fix
|
||||
// round 4.5b2 part B). Its passes run in the Architecture order (engine,
|
||||
// instruction, carry, pre-existing, policy); later steps insert theirs at
|
||||
// their places in PASSES. Today it runs:
|
||||
//
|
||||
// - pre-existing: `markFindings` (lib/review/pre-existing.mjs) sets
|
||||
// `pre_existing` on every JSON finding from git over
|
||||
// `scope_sha_start..scope_sha_end` (read raw, as `reviewedSha` reads; a
|
||||
// YAML number or `null` means no mark), writes `pre_existing_counts`, and
|
||||
// writes a `## Pre-existing` list (the count, then one line per title)
|
||||
// just before `## Declined to Judge`. It never moves or lowers a finding.
|
||||
//
|
||||
// Then, always last, the id recomputation: every id is computed again with
|
||||
// `computeFindingIds` (lib/parsers/finding-id.mjs) over the final array, and
|
||||
// any id that changed is rewritten with one simultaneous old -> new map,
|
||||
// applied in one pass so no rewrite chains, in four places only: each JSON
|
||||
// finding's `id`, the frontmatter `findings:` list, the id line of each body
|
||||
// entry (its `### <id>` heading or `**ID:**` line), and each
|
||||
// `**Source finding:**` line. The file is written atomically (temp + rename).
|
||||
//
|
||||
// CLI:
|
||||
// node lib/review/review-finalize.mjs --review <p> --repo <d> --project <dir>
|
||||
// --round <R> [--round-base <sha>] [--previous <p>]
|
||||
// prints one JSON line; exit 0 = written, 1 = refused (a placeholder left,
|
||||
// an id that cannot be computed, two findings under one id), 2 = could not
|
||||
// run (bad arguments, unreadable file, no trailing JSON block). On 1 or 2
|
||||
// the review is left as fill-finding-ids wrote it.
|
||||
|
||||
import { readFileSync, renameSync, statSync, writeFileSync } from 'node:fs';
|
||||
import { basename, dirname, join } from 'node:path';
|
||||
import { computeFindingIds } from '../parsers/finding-id.mjs';
|
||||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { FULL_SHA_RE } from './reviewed-commit.mjs';
|
||||
import { markFindings, scopeShas } from './pre-existing.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
export const MAX_ROUND = 3;
|
||||
const SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
|
||||
const HEX40 = /^[0-9a-f]{40}$/;
|
||||
const HEX40_G = /[0-9a-f]{40}/g;
|
||||
|
||||
/** The review split at the trailing closed `json` block: {head, fence, body, tail}, or null. */
|
||||
function splitTrailing(text) {
|
||||
const last = fencedBlocks(text).at(-1);
|
||||
if (!last || !last.closed || !isJsonBlock(last) || last.body === '') return null;
|
||||
const bodyAt = text.lastIndexOf(last.body);
|
||||
if (bodyAt <= 0) return null;
|
||||
const fenceAt = text.lastIndexOf('\n', bodyAt - 2) + 1;
|
||||
return {
|
||||
head: text.slice(0, fenceAt),
|
||||
fence: text.slice(fenceAt, bodyAt),
|
||||
body: last.body,
|
||||
tail: text.slice(bodyAt + last.body.length),
|
||||
};
|
||||
}
|
||||
|
||||
/** One line of prose: whitespace runs made one space, backticks removed. */
|
||||
const oneLine = (s) => String(s ?? '').replace(/`/g, '').replace(/\s+/g, ' ').trim();
|
||||
|
||||
/** The `## Pre-existing` section for the marked findings. */
|
||||
function preExistingSection(findings, counts) {
|
||||
const marked = findings.filter((f) => f && f.pre_existing === true);
|
||||
if (marked.length === 0) return '## Pre-existing\n\nNone.\n\n';
|
||||
const rows = SEVERITIES.filter((s) => counts[s] > 0).map((s) => `| ${s} | ${counts[s]} |`);
|
||||
const items = marked.map((f) => `- ${oneLine(f.title) || '(no title)'} (\`${oneLine(f.file)}:${f.line}\`)`);
|
||||
return `## Pre-existing\n\nFindings on lines this range did not add or change: ${marked.length}.\n\n`
|
||||
+ `| Severity | Count |\n|----------|-------|\n${rows.join('\n')}\n\n${items.join('\n')}\n\n`;
|
||||
}
|
||||
|
||||
/** `head` with every `## Pre-existing` section taken out and `section` put just before `## Declined to Judge` (else at the end). */
|
||||
function withPreExistingSection(head, section) {
|
||||
const lines = head.split('\n');
|
||||
const kept = [];
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
if (/^##\s+Pre-existing\s*$/.test(lines[i])) {
|
||||
i++;
|
||||
while (i < lines.length && !/^##\s/.test(lines[i])) i++;
|
||||
i--;
|
||||
continue;
|
||||
}
|
||||
kept.push(lines[i]);
|
||||
}
|
||||
const at = kept.findIndex((l) => /^##\s+Declined to Judge\b/.test(l));
|
||||
const sectionLines = section.replace(/\n$/, '').split('\n');
|
||||
if (at === -1) {
|
||||
const out = kept.join('\n').replace(/\n*$/, '\n\n');
|
||||
return `${out}${section}`;
|
||||
}
|
||||
kept.splice(at, 0, ...sectionLines);
|
||||
return kept.join('\n');
|
||||
}
|
||||
|
||||
/** Pass `pre-existing`. */
|
||||
function passPreExisting(payload, { repo, text }) {
|
||||
const { start, end } = scopeShas(text);
|
||||
markFindings(payload, { repo, start, end });
|
||||
const counts = Object.fromEntries(SEVERITIES.map((s) => [s, 0]));
|
||||
for (const f of payload.findings) {
|
||||
const sev = String(f?.severity ?? '').toUpperCase();
|
||||
if (f && f.pre_existing === true && sev in counts) counts[sev] += 1;
|
||||
}
|
||||
payload.pre_existing_counts = counts;
|
||||
return { headEdit: (head) => withPreExistingSection(head, preExistingSection(payload.findings, counts)), counts };
|
||||
}
|
||||
|
||||
/** The passes, in the Architecture order; the id recomputation runs after them, always last. */
|
||||
export const PASSES = Object.freeze([
|
||||
{ name: 'pre-existing', run: passPreExisting },
|
||||
]);
|
||||
|
||||
/** Rewrite the 40-hex tokens of one line through `map` (one pass: no chaining). */
|
||||
const remap = (line, map) => line.replace(HEX40_G, (m) => map.get(m) ?? m);
|
||||
|
||||
/** `head` with ids rewritten in the frontmatter list, body id lines and Source finding lines. */
|
||||
function rewriteIds(head, map) {
|
||||
if (map.size === 0) return head;
|
||||
const lines = head.split('\n');
|
||||
let fmEnd = -1;
|
||||
if (lines[0] === '---') fmEnd = lines.indexOf('---', 1);
|
||||
let inList = false;
|
||||
return lines.map((l, i) => {
|
||||
if (i > 0 && i < fmEnd) {
|
||||
if (/^findings\s*:/.test(l)) { inList = true; return l; }
|
||||
if (inList && /^\s+-\s/.test(l)) return remap(l, map);
|
||||
if (!/^\s/.test(l)) inList = false;
|
||||
return l;
|
||||
}
|
||||
if (i <= fmEnd) return l;
|
||||
if (/^###\s+[0-9a-f]{40}\b/.test(l) || /\*\*ID:\*\*/.test(l) || /\*\*Source finding:\*\*/.test(l)) return remap(l, map);
|
||||
return l;
|
||||
}).join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* Finalize review.md text. Pure apart from the git reads of the passes.
|
||||
* @param {string} text
|
||||
* @param {{ repo: string }} ctx
|
||||
* @returns {{exit: 0, text: string, passes: string[], pre_existing_counts: object, ids_rewritten: number} | {exit: 1|2, reason: string}}
|
||||
*/
|
||||
export function finalizeReview(text, { repo } = {}) {
|
||||
if (/@@ID_\d+@@/.test(text)) return { exit: 1, reason: 'an @@ID_<n>@@ placeholder is left: run fill-finding-ids.mjs first' };
|
||||
const parts = splitTrailing(text);
|
||||
if (!parts) return { exit: 2, reason: 'no trailing closed json block' };
|
||||
let payload;
|
||||
try { payload = JSON.parse(parts.body); } catch (e) { return { exit: 2, reason: `the trailing JSON block does not parse: ${e.message}` }; }
|
||||
if (!payload || typeof payload !== 'object' || Array.isArray(payload) || !Array.isArray(payload.findings)) {
|
||||
return { exit: 2, reason: 'the trailing JSON block has no findings array' };
|
||||
}
|
||||
if (payload.findings.some((f) => !f || typeof f !== 'object')) return { exit: 1, reason: 'a finding is not an object' };
|
||||
const before = payload.findings.map((f) => f.id);
|
||||
const dup = before.find((id, i) => before.indexOf(id) !== i);
|
||||
if (dup !== undefined) return { exit: 1, reason: `two findings carry the id ${JSON.stringify(dup)}` };
|
||||
|
||||
let head = parts.head;
|
||||
const out = { passes: [] };
|
||||
for (const pass of PASSES) {
|
||||
const r = pass.run(payload, { repo, text });
|
||||
if (r.headEdit) head = r.headEdit(head);
|
||||
if (r.counts) out.pre_existing_counts = r.counts;
|
||||
out.passes.push(pass.name);
|
||||
}
|
||||
|
||||
// The id recomputation, always last.
|
||||
const errors = [];
|
||||
const ids = computeFindingIds(payload.findings, (i, e) => errors.push(`findings[${i}]: ${e.message}`));
|
||||
if (errors.length > 0) return { exit: 1, reason: `cannot compute an id: ${errors.join('; ')}` };
|
||||
const map = new Map();
|
||||
payload.findings.forEach((f, i) => {
|
||||
if (f.id !== ids[i] && typeof f.id === 'string' && HEX40.test(f.id)) map.set(f.id, ids[i]);
|
||||
f.id = ids[i];
|
||||
});
|
||||
head = rewriteIds(head, map);
|
||||
out.passes.push('ids');
|
||||
const body = `${JSON.stringify(payload, null, 2)}\n`;
|
||||
return { exit: 0, text: `${head}${parts.fence}${body}${parts.tail}`, ...out, ids_rewritten: map.size };
|
||||
}
|
||||
|
||||
function writeAtomic(p, text) {
|
||||
const tmp = join(dirname(p), `.${basename(p)}.${process.pid}.tmp`);
|
||||
writeFileSync(tmp, text);
|
||||
renameSync(tmp, p);
|
||||
}
|
||||
|
||||
/** The CLI's work: check the arguments, read, finalize, write. */
|
||||
export function finalizeFile({ review, repo, project, round, roundBase }) {
|
||||
if (!review) return { exit: 2, reason: '--review is required' };
|
||||
if (!repo) return { exit: 2, reason: '--repo is required' };
|
||||
if (!project) return { exit: 2, reason: '--project is required' };
|
||||
if (!/^[1-9]\d*$/.test(String(round ?? '')) || Number(round) > MAX_ROUND) {
|
||||
return { exit: 2, reason: `--round must be an integer 1-${MAX_ROUND}` };
|
||||
}
|
||||
if (roundBase !== undefined && !FULL_SHA_RE.test(roundBase)) return { exit: 2, reason: '--round-base must be a full commit id' };
|
||||
try {
|
||||
if (!statSync(project).isDirectory()) return { exit: 2, reason: `--project ${project} is not a directory` };
|
||||
} catch (e) { return { exit: 2, reason: `--project ${project}: ${e.message}` }; }
|
||||
let text;
|
||||
try { text = readFileSync(review, 'utf8'); } catch (e) { return { exit: 2, reason: `cannot read ${review}: ${e.message}` }; }
|
||||
const r = finalizeReview(text, { repo });
|
||||
if (r.exit !== 0) return r;
|
||||
try { writeAtomic(review, r.text); } catch (e) { return { exit: 2, reason: `cannot write ${review}: ${e.message}` }; }
|
||||
const { text: _t, ...rest } = r;
|
||||
return { ...rest, written: true, path: review };
|
||||
}
|
||||
|
||||
function opt(args, name) {
|
||||
const i = args.indexOf(name);
|
||||
return i >= 0 ? args[i + 1] : undefined;
|
||||
}
|
||||
|
||||
if (isMain(import.meta.url)) {
|
||||
const args = process.argv.slice(2);
|
||||
let result;
|
||||
try {
|
||||
result = finalizeFile({
|
||||
review: opt(args, '--review'), repo: opt(args, '--repo'), project: opt(args, '--project'),
|
||||
round: opt(args, '--round'), roundBase: opt(args, '--round-base'),
|
||||
});
|
||||
} catch (e) {
|
||||
result = { exit: 2, reason: `review-finalize crashed: ${e.message}` };
|
||||
}
|
||||
const { exit, ...rest } = result;
|
||||
process.stdout.write(`${JSON.stringify(exit === 0 ? rest : { written: false, ...rest })}\n`);
|
||||
process.exit(exit);
|
||||
}
|
||||
|
|
@ -59,6 +59,7 @@ import { reviewedSha, checkReviewedCommit } from './reviewed-commit.mjs';
|
|||
import { isMain } from '../util/is-main.mjs';
|
||||
import { resolveRepo } from '../util/approval-log.mjs';
|
||||
import { answerRecords, decideAnswers, reviewSubject } from './review-answers.mjs';
|
||||
import { preExistingCheck } from './pre-existing.mjs';
|
||||
|
||||
export const EXIT = Object.freeze({ PASS: 0, BLOCK: 1, UNDECIDED: 2 });
|
||||
|
||||
|
|
@ -189,6 +190,11 @@ function blockerSectionEntries(body) {
|
|||
* answered (lib/review/review-answers.mjs `decideAnswers`). K2 records come
|
||||
* from the top-level approval log of `repo`, else of the review's directory;
|
||||
* a broken, diverged or unreadable log, or no work tree, means no records.
|
||||
* A MAJOR leaves the answer set as pre-existing only when it is marked
|
||||
* `pre_existing: true` and git agrees over `scope_sha_start..scope_sha_end`
|
||||
* in `repo` (lib/review/pre-existing.mjs `preExistingCheck`); with no repo, or
|
||||
* no full `scope_sha_start`, no finding is pre-existing. A BLOCKER or a
|
||||
* security finding counts whatever the mark.
|
||||
* @param {string} text - review.md
|
||||
* @param {{ repo?: string, reviewPath: string }} opts
|
||||
* @returns {{ exit: 0|2, reason: string, unanswered: string[], fixed_pending: string[] }}
|
||||
|
|
@ -196,7 +202,7 @@ function blockerSectionEntries(body) {
|
|||
export function decideOpenFindings(text, { repo, reviewPath } = {}) {
|
||||
const { records, top, problem } = answerRecords(repo ?? (reviewPath ? dirname(reviewPath) : undefined));
|
||||
const subject = top && reviewPath ? reviewSubject(top, reviewPath) : undefined;
|
||||
const d = decideAnswers(text, { records, reviewSubject: subject });
|
||||
const d = decideAnswers(text, { records, reviewSubject: subject, preExisting: preExistingCheck(text, repo) });
|
||||
if (d.ok) return { exit: EXIT.PASS, reason: 'every counting MAJOR is answered', unanswered: [], fixed_pending: [] };
|
||||
return {
|
||||
exit: EXIT.UNDECIDED,
|
||||
|
|
|
|||
2
tests/fixtures/jsonl-schemas.md
vendored
2
tests/fixtures/jsonl-schemas.md
vendored
|
|
@ -25,7 +25,7 @@
|
|||
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:2990-3012 | profile, phase_models, profile_source | none |
|
||||
| trekexecute-stats (autonomy events) | ts, event, known_event, payload | lib/stats/event-emit.mjs `emit()` | event-emit.mjs:78-99 | payload.profile, payload.phase_models, payload.profile_source | none |
|
||||
| trekexecute-stats (PostToolUse Bash) | ts, session_id, command_excerpt, duration_ms, success | hooks/scripts/post-bash-stats.mjs (Bash PostToolUse) | post-bash-stats.mjs:42-54 | none (hook is plugin-level, not profile-aware) | command_excerpt (CWE-212) |
|
||||
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:643-693 | profile, phase_models, profile_source | none |
|
||||
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:654-704 | profile, phase_models, profile_source | none |
|
||||
| trekcontinue-stats | ts, project, next_session_label, status | lib/stats/stats-append.mjs (trekcontinue.md Phase 5) | trekcontinue.md:304-308 | profile, profile_source | none |
|
||||
| token-usage | ts, session_id, scope, model, tokens_input, tokens_output, tokens_cache_creation, tokens_cache_read, cost_usd, is_estimate, price_table_version | lib/stats/token-usage.mjs `captureTokenUsage` (via hooks/scripts/otel-export.mjs Stop hook, opt-in `VOYAGE_TOKEN_METER`) | token-usage.mjs:buildRecord | n/a (SKAL-2 schema) | session_id (stripped at export) |
|
||||
|
||||
|
|
|
|||
444
tests/lib/pre-existing.test.mjs
Normal file
444
tests/lib/pre-existing.test.mjs
Normal file
|
|
@ -0,0 +1,444 @@
|
|||
// tests/lib/pre-existing.test.mjs
|
||||
// [PRX] fix round 4.5b2 part B, Step 5: a finding on a line the range did not
|
||||
// add or change is marked pre-existing (lib/review/review-finalize.mjs, from
|
||||
// git through lib/review/pre-existing.mjs), and the verdict gate re-derives the
|
||||
// mark from git: it never trusts it as written. A pre-existing MAJOR leaves the
|
||||
// answer set; a BLOCKER or a security finding counts in full whatever the mark;
|
||||
// every uncertain case (a renamed file, line 0, no scope_sha_start, a changed
|
||||
// line, a line next to a pure deletion, an uncommitted file) counts as new.
|
||||
// Tests run the CLIs (gate, finalize, fill-finding-ids, validator) in throwaway
|
||||
// git repos with no hooks; `changedLineSets` is also read directly for the
|
||||
// repo-config case. `keep:` cases hold at the base already and stay untagged.
|
||||
// Each tagged gate case carries a Control: the same finding on an unchanged
|
||||
// line exits 0, which only a gate that reads the mark can do.
|
||||
|
||||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { appendFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath, pathToFileURL } from 'node:url';
|
||||
import { computeFindingId, computeFindingIds } from '../../lib/parsers/finding-id.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
|
||||
const PRE = join(ROOT, 'lib', 'review', 'pre-existing.mjs');
|
||||
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
|
||||
const FILL = join(ROOT, 'lib', 'review', 'fill-finding-ids.mjs');
|
||||
const VALIDATOR = join(ROOT, 'lib', 'validators', 'review-validator.mjs');
|
||||
const REVIEW_REL = '.claude/projects/2026-11-02-p/review.md';
|
||||
|
||||
// Loaded so that each new assertion fails on its own while the module is absent.
|
||||
let preMod = {};
|
||||
try {
|
||||
preMod = await import(pathToFileURL(PRE).href);
|
||||
} catch (e) {
|
||||
if (existsSync(PRE)) throw e;
|
||||
}
|
||||
|
||||
const made = [];
|
||||
after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
|
||||
|
||||
function g(cwd, ...args) {
|
||||
const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: childEnv() });
|
||||
if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
|
||||
return r.stdout.trim();
|
||||
}
|
||||
|
||||
function putIn(dir, rel, text) {
|
||||
mkdirSync(dirname(join(dir, rel)), { recursive: true });
|
||||
writeFileSync(join(dir, rel), text);
|
||||
}
|
||||
|
||||
const parse = (r) => { try { return JSON.parse(r.stdout); } catch { return {}; } };
|
||||
// Ten lines `export const <tag><n> = <n>;`; `changes` maps a line number to new text, or to null to remove it.
|
||||
const ten = (tag, changes = {}) => Array.from({ length: 10 }, (_, i) => (i + 1 in changes ? changes[i + 1] : `export const ${tag}${i + 1} = ${i + 1};`))
|
||||
.filter((l) => l !== null).join('\n') + '\n';
|
||||
|
||||
/**
|
||||
* A throwaway repo. The start commit holds a.mjs, d.mjs and old.mjs (ten lines
|
||||
* each). `rounds` are later commits, each a map of path → text (null removes,
|
||||
* `{ mv: from }` renames). The default single round:
|
||||
* - a.mjs line 3 changed;
|
||||
* - d.mjs line 5 removed (a pure deletion: new lines 4 and 5 sit next to it);
|
||||
* - old.mjs renamed to moved.mjs, unchanged;
|
||||
* - n.mjs added.
|
||||
* `config` lines go into .git/config before the rounds are committed.
|
||||
*/
|
||||
function prxRepo({ config = [], rounds } = {}) {
|
||||
const base = mkdtempSync(join(tmpdir(), 'pre-existing-prx-'));
|
||||
made.push(base);
|
||||
const repo = join(base, 'repo');
|
||||
mkdirSync(repo, { recursive: true });
|
||||
mkdirSync(join(base, 'hooks-none'));
|
||||
g(repo, 'init', '-q', '-b', 'main');
|
||||
appendFileSync(join(repo, '.git', 'config'), '[user]\n\tname = Test\n\temail = test@example.invalid\n'
|
||||
+ `[commit]\n\tgpgsign = false\n[core]\n\thooksPath = "${join(base, 'hooks-none')}"\n`);
|
||||
appendFileSync(join(repo, '.git', 'info', 'exclude'), '.claude/projects/\n');
|
||||
putIn(repo, 'a.mjs', ten('a'));
|
||||
putIn(repo, 'd.mjs', ten('d'));
|
||||
putIn(repo, 'old.mjs', ten('o'));
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'chore: start');
|
||||
const start = g(repo, 'rev-parse', 'HEAD');
|
||||
for (const c of config) g(repo, 'config', ...c);
|
||||
const plan = rounds ?? [{
|
||||
'a.mjs': ten('a', { 3: 'export const a3 = 33;' }),
|
||||
'd.mjs': ten('d', { 5: null }),
|
||||
'moved.mjs': { mv: 'old.mjs' },
|
||||
'n.mjs': 'export const n = 1;\n',
|
||||
}];
|
||||
const shas = [];
|
||||
plan.forEach((files, i) => {
|
||||
for (const [rel, text] of Object.entries(files)) {
|
||||
if (text === null) g(repo, 'rm', '-q', rel);
|
||||
else if (typeof text === 'object') g(repo, 'mv', text.mv, rel);
|
||||
else putIn(repo, rel, text);
|
||||
}
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', `feat: round ${i + 1}`);
|
||||
shas.push(g(repo, 'rev-parse', 'HEAD'));
|
||||
});
|
||||
const reviewPath = join(repo, ...REVIEW_REL.split('/'));
|
||||
return {
|
||||
repo, start, shas, end: shas.at(-1), reviewPath,
|
||||
write: (text) => putIn(repo, REVIEW_REL, text),
|
||||
read: () => readFileSync(reviewPath, 'utf8'),
|
||||
gate: () => spawnSync(process.execPath, [GATE, '--json', reviewPath, '--repo', repo], { encoding: 'utf8', env: childEnv() }),
|
||||
finalize: (extra = []) => spawnSync(process.execPath, [FINALIZE, '--review', reviewPath, '--repo', repo,
|
||||
'--project', dirname(reviewPath), '--round', '1', ...extra], { encoding: 'utf8', env: childEnv() }),
|
||||
fill: () => spawnSync(process.execPath, [FILL, reviewPath], { encoding: 'utf8', env: childEnv() }),
|
||||
validate: () => spawnSync(process.execPath, [VALIDATOR, '--strict', reviewPath], { encoding: 'utf8', env: childEnv() }),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* A review_version 1.2 review of `start..end`. `findings` are
|
||||
* [{severity, file, line, rule_key?, engine?, pre_existing?, title?}]; ids are
|
||||
* computed over (file, line, rule_key) unless `ids` names them. `start: null`
|
||||
* writes `scope_sha_start: null`; `round`/`roundBase` add the round fields.
|
||||
*/
|
||||
function prxReview({ start, end, findings, verdict, ids, round, roundBase }) {
|
||||
const list = findings.map((f, i) => ({
|
||||
severity: f.severity, rule_key: f.rule_key ?? 'MISSING_TEST', ...(f.engine ? { engine: f.engine } : {}),
|
||||
file: f.file, line: f.line, title: f.title ?? `t${i + 1}`, detail: `d${i + 1}`, recommended_action: 'Add a test.',
|
||||
...('pre_existing' in f ? { pre_existing: f.pre_existing } : {}),
|
||||
}));
|
||||
const computed = ids ?? computeFindingIds(list);
|
||||
list.forEach((f, i) => { f.id = computed[i]; });
|
||||
const ordered = list.map(({ id, ...rest }) => ({ id, ...rest }));
|
||||
const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
|
||||
for (const f of ordered) counts[f.severity] += 1;
|
||||
const v = verdict ?? (counts.BLOCKER > 0 ? 'BLOCK' : counts.MAJOR > 0 ? 'WARN' : 'ALLOW');
|
||||
const json = JSON.stringify({ verdict: v, counts, allow_blocked_by: [], findings: ordered }, null, 2);
|
||||
const sections = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'].map((sev) => {
|
||||
const of = ordered.filter((f) => f.severity === sev);
|
||||
if (of.length === 0) return '';
|
||||
return `## Findings (${sev})\n\n${of.map((f) => `### ${f.id}\n\n- **Rule:** \`${f.rule_key}\`\n- **Location:** \`${f.file}:${f.line}\`\n- **Title:** ${f.title}\n`).join('\n')}\n`;
|
||||
}).join('');
|
||||
const steps = ordered.filter((f) => f.severity === 'BLOCKER' || f.severity === 'MAJOR')
|
||||
.map((f, i) => `### Step R${i + 1}: fix ${f.title}\n\n- **Files:** \`${f.file}\`\n- **Source finding:** \`${f.id}\`\n- **Test first:** a test that fails today\n- **Verify:** re-run /trekreview\n- **On failure:** escalate\n`).join('\n');
|
||||
return `---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
task: "t"
|
||||
slug: p
|
||||
project_dir: .claude/projects/2026-11-02-p/
|
||||
brief_path: .claude/projects/2026-11-02-p/brief.md
|
||||
scope_sha_start: ${start === null ? 'null' : start}
|
||||
scope_sha_end: ${end}
|
||||
reviewed_files_count: 3
|
||||
verdict: ${v}
|
||||
${round ? `round: ${round}\nround_base: ${roundBase ?? 'null'}\n` : ''}findings:
|
||||
${ordered.map((f) => ` - ${f.id}`).join('\n')}
|
||||
---
|
||||
|
||||
# Review: t
|
||||
|
||||
## Executive Summary
|
||||
|
||||
x
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| a.mjs | deep-review | default |
|
||||
|
||||
${sections}## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
x
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
${steps || 'None.\n'}
|
||||
\`\`\`json
|
||||
${json}
|
||||
\`\`\`
|
||||
`;
|
||||
}
|
||||
|
||||
const jsonOf = (text) => JSON.parse(text.slice(text.lastIndexOf('```json\n') + 8, text.lastIndexOf('```')));
|
||||
|
||||
/** Write a one-finding WARN review of the fixture's range and run the gate on it. */
|
||||
function gateOn(fx, finding, opts = {}) {
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [{ severity: 'MAJOR', ...finding }], ...opts }));
|
||||
return fx.gate();
|
||||
}
|
||||
|
||||
function assertUnanswered(r) {
|
||||
assert.equal(r.status, 2, r.stdout + r.stderr);
|
||||
assert.match(String(parse(r).reason), /^review-unanswered:/, r.stdout);
|
||||
}
|
||||
|
||||
function assertControl(fx, finding = { file: 'a.mjs', line: 7, pre_existing: true }) {
|
||||
const r = gateOn(fx, finding);
|
||||
assert.equal(r.status, 0, `Control: a MAJOR marked pre-existing on an unchanged line opens the gate: ${r.stdout}${r.stderr}`);
|
||||
}
|
||||
|
||||
test('[PRX] a MAJOR outside the -U0 added or changed lines is marked pre-existing and leaves the answer set', () => {
|
||||
assert.ok(existsSync(FINALIZE), 'lib/review/review-finalize.mjs must exist');
|
||||
const fx = prxRepo();
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [{ severity: 'MAJOR', file: 'a.mjs', line: 7, title: 'An old defect' }] }));
|
||||
const f = fx.finalize();
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
const text = fx.read();
|
||||
const payload = jsonOf(text);
|
||||
assert.equal(payload.findings[0].pre_existing, true, text);
|
||||
assert.deepEqual(payload.pre_existing_counts, { BLOCKER: 0, MAJOR: 1, MINOR: 0, SUGGESTION: 0 });
|
||||
assert.equal(payload.findings[0].severity, 'MAJOR', 'the mark never lowers a finding');
|
||||
const pre = text.search(/^## Pre-existing\s*$/m);
|
||||
const declined = text.search(/^## Declined to Judge\s*$/m);
|
||||
assert.ok(pre !== -1 && pre < declined, `## Pre-existing stands just before ## Declined to Judge:\n${text}`);
|
||||
assert.ok(text.slice(pre, declined).includes('An old defect'), text.slice(pre, declined));
|
||||
assert.ok(!/^## Answers/m.test(text), 'no answer was written');
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
});
|
||||
|
||||
test('[PRX] a finding in a renamed file, on line 0, with no scope_sha_start, on a changed line or next to a pure deletion counts as new', () => {
|
||||
assert.equal(typeof preMod.changedLineSets, 'function', 'pre-existing.mjs exports changedLineSets');
|
||||
const fx = prxRepo();
|
||||
assertControl(fx);
|
||||
const cases = [
|
||||
['a renamed file', { file: 'moved.mjs', line: 5, pre_existing: true }],
|
||||
['line 0', { file: 'a.mjs', line: 0, pre_existing: true }],
|
||||
['a changed line', { file: 'a.mjs', line: 3, pre_existing: true }],
|
||||
['the line before a pure deletion', { file: 'd.mjs', line: 4, pre_existing: true }],
|
||||
['the line after a pure deletion', { file: 'd.mjs', line: 5, pre_existing: true }],
|
||||
['a new file', { file: 'n.mjs', line: 1, pre_existing: true }],
|
||||
];
|
||||
for (const [what, finding] of cases) {
|
||||
const r = gateOn(fx, finding);
|
||||
assert.equal(r.status, 2, `${what}: ${r.stdout}${r.stderr}`);
|
||||
assert.match(String(parse(r).reason), /^review-unanswered:/, `${what}: ${r.stdout}`);
|
||||
}
|
||||
const none = gateOn(fx, { file: 'a.mjs', line: 7, pre_existing: true }, { start: null });
|
||||
assert.equal(none.status, 2, `no scope_sha_start: ${none.stdout}${none.stderr}`);
|
||||
assert.match(String(parse(none).reason), /^review-unanswered:/, none.stdout);
|
||||
// Control: a line well away from the deletion is still pre-existing (the file is not 'all').
|
||||
assertControl(fx, { file: 'd.mjs', line: 8, pre_existing: true });
|
||||
});
|
||||
|
||||
test('[PRX] finalize marks none of the uncertain cases, and no finding in a file with uncommitted changes', () => {
|
||||
assert.ok(existsSync(FINALIZE), 'lib/review/review-finalize.mjs must exist');
|
||||
const fx = prxRepo();
|
||||
const findings = [
|
||||
{ severity: 'MAJOR', file: 'moved.mjs', line: 5 },
|
||||
{ severity: 'MAJOR', file: 'a.mjs', line: 3 },
|
||||
{ severity: 'MAJOR', file: 'd.mjs', line: 4 },
|
||||
{ severity: 'MAJOR', file: 'd.mjs', line: 5 },
|
||||
{ severity: 'MINOR', file: 'a.mjs', line: 0 },
|
||||
{ severity: 'MAJOR', file: 'd.mjs', line: 8 },
|
||||
];
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings }));
|
||||
const f = fx.finalize();
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
assert.deepEqual(jsonOf(fx.read()).findings.map((x) => x.pre_existing), [false, false, false, false, false, true]);
|
||||
// An uncommitted change in d.mjs: its findings are not marked.
|
||||
putIn(fx.repo, 'd.mjs', ten('d', { 5: null, 9: 'export const d10 = 0;' }));
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [{ severity: 'MAJOR', file: 'd.mjs', line: 8 }] }));
|
||||
const u = fx.finalize();
|
||||
assert.equal(u.status, 0, u.stdout + u.stderr);
|
||||
assert.equal(jsonOf(fx.read()).findings[0].pre_existing, false, fx.read());
|
||||
// No scope_sha_start: nothing is marked.
|
||||
fx.write(prxReview({ start: null, end: fx.end, findings: [{ severity: 'MAJOR', file: 'a.mjs', line: 7 }] }));
|
||||
const n = fx.finalize();
|
||||
assert.equal(n.status, 0, n.stdout + n.stderr);
|
||||
assert.equal(jsonOf(fx.read()).findings[0].pre_existing, false, fx.read());
|
||||
});
|
||||
|
||||
test('[PRX] in a round-2 review whose scope_sha_start is the run\'s start, a MAJOR on a line round 1 added counts as new', () => {
|
||||
assert.equal(typeof preMod.changedLineSets, 'function', 'pre-existing.mjs exports changedLineSets');
|
||||
const fx = prxRepo({ rounds: [
|
||||
{ 'a.mjs': ten('a', { 5: 'export const a5 = 55;' }) },
|
||||
{ 'a.mjs': ten('a', { 5: 'export const a5 = 55;', 9: 'export const a9 = 99;' }) },
|
||||
] });
|
||||
const [round1] = fx.shas;
|
||||
const opts = { round: 2, roundBase: round1 };
|
||||
const r = gateOn(fx, { file: 'a.mjs', line: 5, pre_existing: true }, opts);
|
||||
assertUnanswered(r);
|
||||
// Control: an unchanged line of the same file is pre-existing in round 2.
|
||||
const c = gateOn(fx, { file: 'a.mjs', line: 7, pre_existing: true }, opts);
|
||||
assert.equal(c.status, 0, `Control: ${c.stdout}${c.stderr}`);
|
||||
// Finalize marks by the run range too, whatever --round-base says.
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [{ severity: 'MAJOR', file: 'a.mjs', line: 5 }], ...opts }));
|
||||
const f = fx.finalize(['--round-base', round1]);
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
assert.equal(jsonOf(fx.read()).findings[0].pre_existing, false, fx.read());
|
||||
});
|
||||
|
||||
test('[PRX] a pre-existing security MAJOR (CODE_REVIEW_DEFECT from security-review) still exits 2 with review-unanswered:', () => {
|
||||
const fx = prxRepo();
|
||||
// Control: the same key from the code-review engine is pre-existing and opens the gate.
|
||||
assertControl(fx, { file: 'a.mjs', line: 7, rule_key: 'CODE_REVIEW_DEFECT', engine: 'code-review', pre_existing: true });
|
||||
const r = gateOn(fx, { file: 'a.mjs', line: 7, rule_key: 'CODE_REVIEW_DEFECT', engine: 'security-review', pre_existing: true });
|
||||
assertUnanswered(r);
|
||||
});
|
||||
|
||||
test('[PRX] after finalize every id is the computed one and the gate reads the file', () => {
|
||||
assert.ok(existsSync(FINALIZE), 'lib/review/review-finalize.mjs must exist');
|
||||
const fx = prxRepo();
|
||||
const findings = [
|
||||
{ severity: 'MAJOR', file: 'a.mjs', line: 7, title: 'An old defect' },
|
||||
{ severity: 'MINOR', file: 'a.mjs', line: 3, title: 'A new nit' },
|
||||
];
|
||||
// The MAJOR carries an id over an old line (8): finalize recomputes it.
|
||||
const stale = computeFindingId('a.mjs', 8, 'MISSING_TEST');
|
||||
const good = computeFindingId('a.mjs', 3, 'MISSING_TEST');
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings, ids: [stale, good] }));
|
||||
const f = fx.finalize();
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
const text = fx.read();
|
||||
const want = computeFindingId('a.mjs', 7, 'MISSING_TEST');
|
||||
assert.deepEqual(jsonOf(text).findings.map((x) => x.id), [want, good]);
|
||||
assert.ok(!text.includes(stale), `no stale id is left anywhere:\n${text}`);
|
||||
assert.ok(text.includes(` - ${want}\n`) && text.includes(`### ${want}\n`) && text.includes(`**Source finding:** \`${want}\``), text);
|
||||
const fill = fx.fill();
|
||||
assert.equal(fill.status, 0, `fill-finding-ids has nothing left to fill: ${fill.stdout}${fill.stderr}`);
|
||||
assert.equal(fx.read(), text, 'fill-finding-ids leaves the finalized file unchanged');
|
||||
const v = fx.validate();
|
||||
assert.equal(v.status, 0, `review-validator --strict: ${v.stdout}${v.stderr}`);
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
assert.doesNotMatch(String(parse(r).reason), /differ/, r.stdout);
|
||||
});
|
||||
|
||||
test('[PRX] a forged pre_existing mark on an added line still needs an answer', () => {
|
||||
const fx = prxRepo();
|
||||
assertControl(fx);
|
||||
assertUnanswered(gateOn(fx, { file: 'a.mjs', line: 3, pre_existing: true }));
|
||||
assertUnanswered(gateOn(fx, { file: 'n.mjs', line: 1, pre_existing: true }));
|
||||
// A path the diff cannot name the same way is never pre-existing.
|
||||
assertUnanswered(gateOn(fx, { file: './a.mjs', line: 3, pre_existing: true }));
|
||||
});
|
||||
|
||||
test('[PRX] a path or line that is not in the tree at scope_sha_end counts as new, at the gate and in finalize', () => {
|
||||
assert.ok(existsSync(FINALIZE), 'lib/review/review-finalize.mjs must exist');
|
||||
// The default round plus a new sub/x.mjs (ten lines). a.mjs has ten lines at the end.
|
||||
const fx = prxRepo({ rounds: [{
|
||||
'a.mjs': ten('a', { 3: 'export const a3 = 33;' }),
|
||||
'd.mjs': ten('d', { 5: null }),
|
||||
'moved.mjs': { mv: 'old.mjs' },
|
||||
'n.mjs': 'export const n = 1;\n',
|
||||
'sub/x.mjs': ten('x'),
|
||||
}] });
|
||||
// Control: the last line of a.mjs is in the file and unchanged, so it is pre-existing.
|
||||
assertControl(fx, { file: 'a.mjs', line: 10, pre_existing: true });
|
||||
const cases = [
|
||||
['a case variant of a changed file (the disk may be case-insensitive)', { file: 'A.mjs', line: 3 }],
|
||||
['a file missing at scope_sha_end', { file: 'zzz.mjs', line: 5 }],
|
||||
['a subdirectory-relative path to the added sub/x.mjs', { file: 'x.mjs', line: 3 }],
|
||||
['a case variant of the added sub/x.mjs', { file: 'Sub/x.mjs', line: 3 }],
|
||||
['a line past the end of the file', { file: 'a.mjs', line: 500 }],
|
||||
['the line just past the end of the file', { file: 'a.mjs', line: 11 }],
|
||||
];
|
||||
for (const [what, finding] of cases) {
|
||||
const r = gateOn(fx, { ...finding, pre_existing: true });
|
||||
assert.equal(r.status, 2, `${what}: ${r.stdout}${r.stderr}`);
|
||||
assert.match(String(parse(r).reason), /^review-unanswered:/, `${what}: ${r.stdout}`);
|
||||
}
|
||||
const findings = [...cases.map(([, f]) => ({ severity: 'MAJOR', ...f })), { severity: 'MAJOR', file: 'a.mjs', line: 10 }];
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings }));
|
||||
const f = fx.finalize();
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
const marks = jsonOf(fx.read()).findings.map((x) => `${x.file}:${x.line}=${x.pre_existing}`);
|
||||
assert.deepEqual(marks, ['A.mjs:3=false', 'zzz.mjs:5=false', 'x.mjs:3=false', 'Sub/x.mjs:3=false',
|
||||
'a.mjs:500=false', 'a.mjs:11=false', 'a.mjs:10=true']);
|
||||
});
|
||||
|
||||
test('[PRX] a symlink to a file the range changed is not a regular file: a finding through it counts as new', () => {
|
||||
assert.ok(existsSync(FINALIZE), 'lib/review/review-finalize.mjs must exist');
|
||||
// The fixture's own range is not used: the run starts at a commit that adds
|
||||
// lnk.mjs -> a.mjs, and ends at one that changes line 1 of a.mjs. The link is
|
||||
// unchanged in the range, so only the tree mode at the end tells it apart.
|
||||
// Line 1 is cited because the link's blob ("a.mjs") is one line long.
|
||||
const fx = prxRepo();
|
||||
symlinkSync('a.mjs', join(fx.repo, 'lnk.mjs'));
|
||||
g(fx.repo, 'add', '--', 'lnk.mjs');
|
||||
g(fx.repo, 'commit', '-q', '-m', 'chore: link');
|
||||
const start = g(fx.repo, 'rev-parse', 'HEAD');
|
||||
assert.equal(g(fx.repo, 'ls-tree', start, '--', 'lnk.mjs').split(' ')[0], '120000', 'lnk.mjs is committed as a symlink');
|
||||
putIn(fx.repo, 'a.mjs', ten('a', { 1: 'export const a1 = 11;' }));
|
||||
g(fx.repo, 'add', '--', 'a.mjs');
|
||||
g(fx.repo, 'commit', '-q', '-m', 'feat: change a.mjs line 1');
|
||||
const end = g(fx.repo, 'rev-parse', 'HEAD');
|
||||
const range = { start, end };
|
||||
// Control: an unchanged line of a.mjs is pre-existing in this range.
|
||||
const c = gateOn(fx, { file: 'a.mjs', line: 7, pre_existing: true }, range);
|
||||
assert.equal(c.status, 0, `Control: ${c.stdout}${c.stderr}`);
|
||||
// Control: the changed line through its real path counts as new.
|
||||
assertUnanswered(gateOn(fx, { file: 'a.mjs', line: 1, pre_existing: true }, range));
|
||||
assertUnanswered(gateOn(fx, { file: 'lnk.mjs', line: 1, pre_existing: true }, range));
|
||||
fx.write(prxReview({ ...range, findings: [
|
||||
{ severity: 'MAJOR', file: 'lnk.mjs', line: 1 },
|
||||
{ severity: 'MAJOR', file: 'a.mjs', line: 7 },
|
||||
] }));
|
||||
const f = fx.finalize();
|
||||
assert.equal(f.status, 0, f.stdout + f.stderr);
|
||||
const marks = jsonOf(fx.read()).findings.map((x) => `${x.file}:${x.line}=${x.pre_existing}`);
|
||||
assert.deepEqual(marks, ['lnk.mjs:1=false', 'a.mjs:7=true']);
|
||||
});
|
||||
|
||||
test('[PRX] repo diff config does not change the line sets', () => {
|
||||
assert.equal(typeof preMod.changedLineSets, 'function', 'pre-existing.mjs exports changedLineSets');
|
||||
const view = (s) => ({
|
||||
ok: s.ok,
|
||||
files: Object.fromEntries([...s.files].filter(([, v]) => v === 'all' || v.size > 0)
|
||||
.map(([k, v]) => [k, v === 'all' ? 'all' : [...v].sort((x, y) => x - y)]).sort()),
|
||||
renamed: [...s.renamed].sort(),
|
||||
});
|
||||
const want = { ok: true, files: { 'a.mjs': [3], 'd.mjs': [4, 5], 'n.mjs': 'all' }, renamed: ['moved.mjs', 'old.mjs'] };
|
||||
const plain = prxRepo();
|
||||
assert.deepEqual(view(preMod.changedLineSets(plain.repo, plain.start, plain.end)), want);
|
||||
const fx = prxRepo({ config: [['diff.renames', 'false'], ['color.diff', 'always'], ['diff.noprefix', 'true']] });
|
||||
assert.deepEqual(view(preMod.changedLineSets(fx.repo, fx.start, fx.end)), want);
|
||||
assertControl(fx);
|
||||
assertUnanswered(gateOn(fx, { file: 'moved.mjs', line: 5, pre_existing: true }));
|
||||
assertUnanswered(gateOn(fx, { file: 'a.mjs', line: 3, pre_existing: true }));
|
||||
});
|
||||
|
||||
test('keep: a pre-existing BLOCKER still exits 1', () => {
|
||||
const fx = prxRepo();
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [{ severity: 'BLOCKER', file: 'a.mjs', line: 7, pre_existing: true }] }));
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 1, r.stdout + r.stderr);
|
||||
});
|
||||
|
||||
test('keep: the verdict gate accepts the pre_existing field without exit 2', () => {
|
||||
const fx = prxRepo();
|
||||
fx.write(prxReview({ start: fx.start, end: fx.end, findings: [
|
||||
{ severity: 'MINOR', file: 'a.mjs', line: 7, pre_existing: true },
|
||||
{ severity: 'MINOR', file: 'a.mjs', line: 3, pre_existing: false },
|
||||
] }));
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue