fix(review): scope_sha_start is bound to the fork point from main
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
57e7eba0a0
commit
a09e6c96a1
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
7 changed files with 494 additions and 9 deletions
|
|
@ -723,7 +723,9 @@ writes an `## Answers` row with `Carried from` for each carried MAJOR the operat
|
|||
earlier round; on exit 1 or 2 print the reason in one line and continue: no answer is carried.
|
||||
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. A carried finding (`carried_from`) keeps its line from the previous round's end,
|
||||
counts in full whatever the mark. The range counts only when `scope_sha_start` is bound to the fork point from main
|
||||
(`main`, else `master`): the fork point itself, or a later commit reached through process files only (the approval
|
||||
log, `.claude/projects/`); otherwise, and for a range that ends on main itself, nothing is pre-existing. A carried finding (`carried_from`) keeps its line from the previous round's end,
|
||||
so it is never pre-existing, whatever the mark.
|
||||
|
||||
**The review policy.** The policy file is `REVIEW.md` at the repo root, read at `scope_sha_start` from git, never
|
||||
|
|
|
|||
|
|
@ -477,7 +477,7 @@ Since 1.1 (ordre 5):
|
|||
- Working files of one review run, in the project folder (not part of `review.md`): the engine transcriptions `.review-engine-{code-review|security-review}.json` (`/trekreview` Phase 4.7) and, beside them, the converted payloads `.review-engine-payload-{engine}.json` (the `engine-findings.mjs` output, `{reviewer, findings, out_of_scope}`). `review-finalize.mjs` reads the payloads to record which engines ran and to put back, at its own severity, an engine finding at MAJOR or BLOCKER that no finding of `review.md` carries (the standard-path comparison); `review-rounds.mjs prepare` deletes the payloads at the start of each round.
|
||||
|
||||
Since 1.2 (every field and section optional; a malformed one present is a strict error, a soft warning):
|
||||
- Trailing JSON block, additive: per finding `pre_existing` (boolean: the range did not add or change the cited line), `carried_from` (the previous round's finding id) and `instruction_ref` (the repo path of the instruction file a CLAUDE.md-key finding cites); top level `pre_existing_counts` (`{SEVERITY: n}`), `resolved` (the previous round's finding ids no longer raised) and `ran` (`{engines: [code-review|security-review], reviewers: [string]}`, written with `finalized: true`). Codes `REVIEW_PRE_EXISTING_MALFORMED`, `REVIEW_ROUND_MALFORMED`, `REVIEW_POLICY_MALFORMED`.
|
||||
- Trailing JSON block, additive: per finding `pre_existing` (boolean: the range did not add or change the cited line and `scope_sha_start` is bound to the fork point from main (the fork point itself, or a later commit reached through process files only); otherwise it is never true), `carried_from` (the previous round's finding id) and `instruction_ref` (the repo path of the instruction file a CLAUDE.md-key finding cites); top level `pre_existing_counts` (`{SEVERITY: n}`), `resolved` (the previous round's finding ids no longer raised) and `ran` (`{engines: [code-review|security-review], reviewers: [string]}`, written with `finalized: true`). Codes `REVIEW_PRE_EXISTING_MALFORMED`, `REVIEW_ROUND_MALFORMED`, `REVIEW_POLICY_MALFORMED`.
|
||||
- `## Pre-existing` — the titles and counts of the findings marked `pre_existing`; written by `review-finalize.mjs`.
|
||||
- `## Resolved since round <n>` — the previous round's findings this round no longer raises; written by `review-finalize.mjs`.
|
||||
- `## Answers` — the operator's answer to each counting MAJOR, written by `/trekreview` Phase 8.8 after `## Remediation Steps` and before the trailing JSON block: a pipe table with the header exactly `| Finding | Answer | Reason | Record | Carried from |`; per row a 40-hex finding id (optionally `#n`), `accepted` or `fixed`, a reason (non-empty when `accepted`), the 64-hex hash of the K2 `review` record, and `-` or `review-round-<run12>-<n>.md#<id>`; at most one row per finding. A MAJOR answered `fixed` needs its `### Step R<n>` with `Source finding: <id>` under `## Remediation Steps`. Code `REVIEW_ANSWERS_MALFORMED`.
|
||||
|
|
|
|||
|
|
@ -28,17 +28,39 @@
|
|||
// 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.
|
||||
// The binding (fix round 4.5b2 part B, Step 14): `scope_sha_start` is read from
|
||||
// review.md, so it counts only when it is bound to the fork point from main
|
||||
// (`forkPoint`: `refs/heads/main`, else `refs/heads/master`, the rule of
|
||||
// `/trekexecute` Phase 8). Exactly two starts are bound (`startBound`):
|
||||
// - the fork point itself;
|
||||
// - a later start reached through process files only: the tree diff from the
|
||||
// fork point to the start names only the approval log
|
||||
// (`.claude/voyage/approvals.jsonl`, LOG_RELPATH) and paths under the
|
||||
// top-level `.claude/projects/`, the commits `/trekbrief` and `/trekplan`
|
||||
// make on the branch (lib/util/artifact-commit.mjs).
|
||||
// Every other start, and a range that ends on the main line itself, is
|
||||
// `{ok:false}`, so nothing is pre-existing. A finding on the approval log or on
|
||||
// a `.claude/projects/` file is never pre-existing, whatever the start. Limits:
|
||||
// - a range that ends on the main line (a run committed straight to main, or
|
||||
// a fast-forward merge), a start older than the fork point (the line sets
|
||||
// are a tree diff, so a line main changed between that start and the fork
|
||||
// point and the branch put back would not show), and a repo with neither
|
||||
// branch have nothing pre-existing; a subdirectory project's own
|
||||
// `{prefix}.claude/projects/` is not a process path (the stricter side);
|
||||
// - trust limit: the fork point is read from `refs/heads/main` (or
|
||||
// `master`), a ref the session that writes review.md can also move (`git
|
||||
// update-ref`, `git branch -f`, or a plain commit on main before the branch
|
||||
// is made), and the Bash guard does not ask about that. The binding narrows
|
||||
// the forged start to a forged ref move as well. It is a trace, not a
|
||||
// signature, like the K2 log;
|
||||
// - BLOCKER and security findings are unaffected, as before: 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';
|
||||
import { LOG_RELPATH } from '../util/approval-log.mjs';
|
||||
|
||||
const MAX_BUFFER = 64 * 1024 * 1024;
|
||||
const HUNK_RE = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(?: .*)?$/;
|
||||
|
|
@ -62,6 +84,74 @@ function gitFailed(what, r) {
|
|||
return notMeasured(`git ${what} failed (${note || `exit ${r.status}`})`);
|
||||
}
|
||||
|
||||
/** A reason without the `notMeasured` prefix, to be wrapped once more. */
|
||||
function bare(reason) {
|
||||
return String(reason ?? '').replace(/^pre-existing not measured: /, '');
|
||||
}
|
||||
|
||||
/** True for the process files `/trekbrief` and `/trekplan` commit: the approval log and the top-level project folder. */
|
||||
function processPath(p) {
|
||||
return p === LOG_RELPATH || p.startsWith('.claude/projects/');
|
||||
}
|
||||
|
||||
/**
|
||||
* The fork point of `end` from the main branch (`refs/heads/main`, else
|
||||
* `refs/heads/master`): the main line's commit when it holds nothing `end`
|
||||
* lacks, else the first parent of the oldest first-parent commit of the main
|
||||
* line that `end` lacks.
|
||||
* @param {string} repo
|
||||
* @param {string} end - a full commit id
|
||||
* @returns {{ok: true, sha: string, ref: string} | {ok: false, reason: string}}
|
||||
*/
|
||||
export function forkPoint(repo, end) {
|
||||
if (!repo) return notMeasured('no repo');
|
||||
if (!FULL_SHA_RE.test(String(end ?? ''))) return notMeasured('scope_sha_end must be a full commit id');
|
||||
let ref = null;
|
||||
let tip = null;
|
||||
for (const b of ['main', 'master']) {
|
||||
const r = git(repo, ['rev-parse', '-q', '--verify', `refs/heads/${b}`]);
|
||||
if (r.error || (r.status !== 0 && r.status !== 1)) return gitFailed('rev-parse --verify', r);
|
||||
if (r.status === 0) { ref = `refs/heads/${b}`; tip = r.stdout.trim(); break; }
|
||||
}
|
||||
if (!ref) return { ok: false, reason: 'no main or master branch' };
|
||||
const list = git(repo, ['rev-list', '--first-parent', ref, `^${end}`]);
|
||||
if (list.error || list.status !== 0) return gitFailed('rev-list --first-parent', list);
|
||||
const shas = list.stdout.split('\n').filter((s) => s !== '');
|
||||
if (shas.length === 0) return { ok: true, sha: tip, ref };
|
||||
const last = shas.at(-1);
|
||||
const parent = git(repo, ['rev-parse', '-q', '--verify', `${last}^1`]);
|
||||
if (parent.error || (parent.status !== 0 && parent.status !== 1)) return gitFailed('rev-parse --verify', parent);
|
||||
if (parent.status === 1) return { ok: false, reason: `${last.slice(0, 12)} on ${ref} has no parent: no fork point` };
|
||||
return { ok: true, sha: parent.stdout.trim(), ref };
|
||||
}
|
||||
|
||||
/**
|
||||
* Is `start` bound to the fork point `fork`? Only the fork point itself, or a
|
||||
* later start whose tree diff from the fork point names only process files.
|
||||
* @param {string} repo
|
||||
* @param {string} start - scope_sha_start
|
||||
* @param {string} fork - forkPoint(...).sha
|
||||
* @returns {{ok: true} | {ok: false, reason: string}}
|
||||
*/
|
||||
export function startBound(repo, start, fork) {
|
||||
if (!repo) return notMeasured('no repo');
|
||||
if (!FULL_SHA_RE.test(String(start ?? '')) || !FULL_SHA_RE.test(String(fork ?? ''))) {
|
||||
return { ok: false, reason: 'the start and the fork point must be full commit ids' };
|
||||
}
|
||||
if (start.toLowerCase() === fork.toLowerCase()) return { ok: true };
|
||||
const anc = git(repo, ['merge-base', '--is-ancestor', fork, start]);
|
||||
if (anc.error || (anc.status !== 0 && anc.status !== 1)) return gitFailed('merge-base --is-ancestor', anc);
|
||||
if (anc.status === 1) return { ok: false, reason: 'the start is not after the fork point (an older or unrelated start)' };
|
||||
const r = git(repo, ['diff', '--no-color', '--no-ext-diff', '--no-relative', '--ignore-submodules=none',
|
||||
'--name-only', '-z', '--no-renames', fork, start]);
|
||||
if (r.error || r.status !== 0) return gitFailed('diff --name-only', r);
|
||||
const other = r.stdout.split('\0').find((p) => p !== '' && !processPath(p));
|
||||
if (other !== undefined) {
|
||||
return { ok: false, reason: `the fork point..start changes ${JSON.stringify(other.slice(0, 120))}, not a process file` };
|
||||
}
|
||||
return { ok: true };
|
||||
}
|
||||
|
||||
/** 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 ?? ''));
|
||||
|
|
@ -223,6 +313,15 @@ export function changedLineSets(repo, start, end) {
|
|||
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 fp = forkPoint(repo, end);
|
||||
if (!fp.ok) return notMeasured(bare(fp.reason));
|
||||
const onMain = git(repo, ['merge-base', '--is-ancestor', end, fp.sha]);
|
||||
if (onMain.error || (onMain.status !== 0 && onMain.status !== 1)) return gitFailed('merge-base --is-ancestor', onMain);
|
||||
if (onMain.status === 0) return notMeasured(`the range ends on ${fp.ref}; scope_sha_start cannot be bound`);
|
||||
const bound = startBound(repo, start, fp.sha);
|
||||
if (!bound.ok) {
|
||||
return notMeasured(`scope_sha_start ${start.slice(0, 12)} is not bound to the fork point ${fp.sha.slice(0, 12)} from ${fp.ref}: ${bare(bound.reason)}`);
|
||||
}
|
||||
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);
|
||||
|
|
@ -257,6 +356,8 @@ function plainPath(p) {
|
|||
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;
|
||||
// A process file can be changed by a bound start's own commits: never pre-existing.
|
||||
if (processPath(f.file)) return false;
|
||||
if (sets.renamed.has(f.file)) return false;
|
||||
const entry = sets.files.get(f.file);
|
||||
if (entry === 'all') return false;
|
||||
|
|
|
|||
|
|
@ -163,6 +163,7 @@ mkrepo() {
|
|||
ten b > "$r/b.mjs"
|
||||
git -C "$r" add a.mjs b.mjs && git -C "$r" commit -q -m 'chore: start' || return 1
|
||||
START="$(git -C "$r" rev-parse HEAD)"
|
||||
git -C "$r" checkout -q -b feat || return 1
|
||||
ten a 3 'export const a3 = 33;' > "$r/a.mjs"
|
||||
ten b 3 'export const b3 = 33;' > "$r/b.mjs"
|
||||
git -C "$r" add a.mjs b.mjs && git -C "$r" commit -q -m 'feat: round 1' || return 1
|
||||
|
|
|
|||
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:3016-3038 | 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:756-809 | 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:758-811 | 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) |
|
||||
|
||||
|
|
|
|||
379
tests/lib/pre-existing-bound.test.mjs
Normal file
379
tests/lib/pre-existing-bound.test.mjs
Normal file
|
|
@ -0,0 +1,379 @@
|
|||
// tests/lib/pre-existing-bound.test.mjs
|
||||
// [PXB] fix round 4.5b2 part B, Step 14: nothing is pre-existing unless
|
||||
// scope_sha_start is bound to the fork point from main (`main`, else
|
||||
// `master`). Exactly two starts are bound: the fork point itself, and a later
|
||||
// start reached through process files only (the approval log
|
||||
// `.claude/voyage/approvals.jsonl` and the top-level `.claude/projects/`). A
|
||||
// forged narrower start, an older start, a start reached through a source
|
||||
// change, and a range that ends on main itself mark nothing pre-existing, in
|
||||
// finalize and at the gate. A finding on the approval log or on a
|
||||
// `.claude/projects/` file is never pre-existing.
|
||||
// The process-files and --no-ff cases expect a mark kept and may already pass
|
||||
// before the binding; the file as a whole is red there.
|
||||
|
||||
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, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath, pathToFileURL } from 'node:url';
|
||||
import { 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 REVIEW_REL = '.claude/projects/2026-11-02-p/review.md';
|
||||
const LOG_REL = '.claude/voyage/approvals.jsonl';
|
||||
|
||||
// Loaded so that each new assertion fails on its own while a module or export is absent.
|
||||
const preMod = existsSync(PRE) ? await import(pathToFileURL(PRE).href) : {};
|
||||
const gateMod = existsSync(GATE) ? await import(pathToFileURL(GATE).href) : {};
|
||||
const finalizeMod = existsSync(FINALIZE) ? await import(pathToFileURL(FINALIZE).href) : {};
|
||||
|
||||
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';
|
||||
|
||||
// Copied from prxRepo in tests/lib/pre-existing.test.mjs:70-115, without its .claude/projects/ exclude line.
|
||||
/**
|
||||
* A throwaway repo on `branch` (default `main`) whose start commit holds a.mjs,
|
||||
* d.mjs, old.mjs and the `seed` files (path -> text, staged with `add -f`).
|
||||
* `commit(files, msg)` writes each path (null removes) and commits exactly
|
||||
* those paths with `git add -f`; it returns the new sha.
|
||||
*/
|
||||
function pxbRepo({ branch = 'main', seed = {} } = {}) {
|
||||
const base = mkdtempSync(join(tmpdir(), 'pre-existing-pxb-'));
|
||||
made.push(base);
|
||||
const repo = join(base, 'repo');
|
||||
mkdirSync(repo, { recursive: true });
|
||||
mkdirSync(join(base, 'hooks-none'));
|
||||
g(repo, 'init', '-q', '-b', branch);
|
||||
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`);
|
||||
const commit = (files, msg) => {
|
||||
for (const [rel, text] of Object.entries(files)) {
|
||||
if (text === null) g(repo, 'rm', '-q', '--', rel);
|
||||
else putIn(repo, rel, text);
|
||||
}
|
||||
const kept = Object.entries(files).filter(([, t]) => t !== null).map(([rel]) => rel);
|
||||
if (kept.length > 0) g(repo, 'add', '-f', '--', ...kept);
|
||||
g(repo, 'commit', '-q', '-m', msg);
|
||||
return g(repo, 'rev-parse', 'HEAD');
|
||||
};
|
||||
const start = commit({ 'a.mjs': ten('a'), 'd.mjs': ten('d'), 'old.mjs': ten('o'), ...seed }, 'chore: start');
|
||||
const reviewPath = join(repo, ...REVIEW_REL.split('/'));
|
||||
return {
|
||||
repo, start, reviewPath, commit,
|
||||
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: () => spawnSync(process.execPath, [FINALIZE, '--review', reviewPath, '--repo', repo,
|
||||
'--project', dirname(reviewPath), '--round', '1'], { encoding: 'utf8', env: childEnv() }),
|
||||
};
|
||||
}
|
||||
|
||||
// Copied from prxReview in tests/lib/pre-existing.test.mjs:123-187 (no round fields).
|
||||
function pxbReview({ start, end, findings }) {
|
||||
const list = findings.map((f, i) => ({
|
||||
severity: f.severity, rule_key: f.rule_key ?? 'MISSING_TEST',
|
||||
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 = 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 = 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}
|
||||
scope_sha_end: ${end}
|
||||
reviewed_files_count: 3
|
||||
verdict: ${v}
|
||||
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('```')));
|
||||
|
||||
/** Finalize a review of start..end holding `findings` (all MAJOR unless said); return the marks. */
|
||||
function marksAfterFinalize(fx, { start, end }, findings) {
|
||||
assert.equal(typeof finalizeMod.finalizeFile, 'function', 'review-finalize.mjs exports finalizeFile');
|
||||
fx.write(pxbReview({ start, end, findings: findings.map((f) => ({ severity: 'MAJOR', ...f })) }));
|
||||
const r = fx.finalize();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
return jsonOf(fx.read()).findings.map((x) => `${x.file}:${x.line}=${x.pre_existing}`);
|
||||
}
|
||||
|
||||
/** Run the gate on a one-MAJOR WARN review of start..end whose finding says `pre_existing: true`. */
|
||||
function gateMarked(fx, { start, end }, finding) {
|
||||
fx.write(pxbReview({ start, end, findings: [{ severity: 'MAJOR', pre_existing: true, ...finding }] }));
|
||||
return fx.gate();
|
||||
}
|
||||
|
||||
function assertUnanswered(r, what = '') {
|
||||
assert.equal(r.status, 2, `${what}${r.stdout}${r.stderr}`);
|
||||
assert.match(String(parse(r).reason), /^review-unanswered:/, `${what}${r.stdout}`);
|
||||
}
|
||||
|
||||
function forkOf(fx, end) {
|
||||
assert.equal(typeof preMod.forkPoint, 'function', 'pre-existing.mjs exports forkPoint');
|
||||
const fp = preMod.forkPoint(fx.repo, end);
|
||||
assert.equal(fp.ok, true, JSON.stringify(fp));
|
||||
return fp.sha;
|
||||
}
|
||||
|
||||
test('[PXB] a forged start one commit after the fork point: finalize marks nothing pre-existing and the gate exits 2 review-unanswered for a MAJOR on a line the first run commit added', () => {
|
||||
const fx = pxbRepo();
|
||||
const b = fx.start;
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const c1 = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: C1 changes a.mjs line 3');
|
||||
const c2 = fx.commit({ 'd.mjs': ten('d', { 8: 'export const d8 = 88;' }) }, 'feat: C2 changes d.mjs');
|
||||
// Control: with the true start B, the same MAJOR is not pre-existing either (C1 added its line).
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start: b, end: c2 }, [{ file: 'a.mjs', line: 3 }]), ['a.mjs:3=false']);
|
||||
// The forged start C1: finalize marks nothing.
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start: c1, end: c2 }, [{ file: 'a.mjs', line: 3 }]), ['a.mjs:3=false']);
|
||||
const fin = fx.gate();
|
||||
assertUnanswered(fin, 'the finalized review: ');
|
||||
// A mark written by hand is re-derived by the gate and refused.
|
||||
assertUnanswered(gateMarked(fx, { start: c1, end: c2 }, { file: 'a.mjs', line: 3 }), 'a forged mark: ');
|
||||
});
|
||||
|
||||
test('[PXB] a start after two approval-log and .claude/projects/ commits on the branch still binds: finalize marks a finding on an unchanged line pre-existing', () => {
|
||||
const fx = pxbRepo();
|
||||
const fork = fx.start;
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
fx.commit({ [LOG_REL]: '{"seq":1,"kind":"brief"}\n' }, 'chore(voyage): approval log');
|
||||
const start = fx.commit({
|
||||
'.claude/projects/2026-11-02-p/brief.md': '# Brief\n',
|
||||
'.claude/projects/2026-11-02-p/plan.md': '# Plan\n',
|
||||
}, 'chore(voyage): brief and plan');
|
||||
const end = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: change a.mjs line 3');
|
||||
// Control: the tree diff from the fork point names only process files.
|
||||
const names = g(fx.repo, 'diff', '--name-only', fork, start).split('\n');
|
||||
assert.ok(names.includes(LOG_REL), names.join(','));
|
||||
assert.ok(names.some((p) => p.startsWith('.claude/projects/')), names.join(','));
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start, end }, [{ file: 'a.mjs', line: 7 }, { file: 'a.mjs', line: 3 }]),
|
||||
['a.mjs:7=true', 'a.mjs:3=false']);
|
||||
});
|
||||
|
||||
test('[PXB] a start after a commit that changes .claude/hooks/x.sh, .claude/voyage/other.json, sub/.claude/projects/x.md, .claude/projectsX/x.md, or a source file beside the log does not bind', () => {
|
||||
const variants = [
|
||||
['.claude/hooks/x.sh', { '.claude/hooks/x.sh': 'echo x\n' }],
|
||||
['.claude/voyage/other.json', { '.claude/voyage/other.json': '{}\n' }],
|
||||
['sub/.claude/projects/x.md', { 'sub/.claude/projects/x.md': '# x\n' }],
|
||||
['.claude/projectsX/x.md', { '.claude/projectsX/x.md': '# x\n' }],
|
||||
['a source file beside the log', { '.claude/voyage/approvals.mjs': 'export const x = 1;\n' }],
|
||||
['a source file committed with the log', { [LOG_REL]: '{"seq":1}\n', 'src/beside.mjs': 'export const y = 1;\n' }],
|
||||
];
|
||||
for (const [what, files] of variants) {
|
||||
const fx = pxbRepo();
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const start = fx.commit(files, `chore: ${what}`);
|
||||
const end = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: change a.mjs line 3');
|
||||
const fork = forkOf(fx, end);
|
||||
assert.equal(fork, fx.start, `${what}: the fork point is the start commit on main`);
|
||||
assert.equal(typeof preMod.startBound, 'function', 'pre-existing.mjs exports startBound');
|
||||
assert.equal(preMod.startBound(fx.repo, start, fork).ok, false, `${what}: the start is not bound`);
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start, end }, [{ file: 'a.mjs', line: 7 }]), ['a.mjs:7=false'], what);
|
||||
}
|
||||
});
|
||||
|
||||
test('[PXB] a start reached through a source commit and then a commit that changes only .claude/projects/ does not bind: the tree diff from the fork point holds the source change', () => {
|
||||
const fx = pxbRepo();
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
fx.commit({ 'a.mjs': ten('a', { 5: 'export const a5 = 55;' }) }, 'feat: change a.mjs line 5');
|
||||
const start = fx.commit({ '.claude/projects/2026-11-02-p/brief.md': '# Brief\n' }, 'chore(voyage): brief');
|
||||
const end = fx.commit({ 'd.mjs': ten('d', { 8: 'export const d8 = 88;' }) }, 'feat: change d.mjs');
|
||||
// Neither the start commit nor start..end touches a.mjs.
|
||||
assert.equal(g(fx.repo, 'diff', '--name-only', `${start}^`, start), '.claude/projects/2026-11-02-p/brief.md');
|
||||
assert.equal(g(fx.repo, 'diff', '--name-only', start, end), 'd.mjs');
|
||||
const fork = forkOf(fx, end);
|
||||
assert.equal(typeof preMod.startBound, 'function', 'pre-existing.mjs exports startBound');
|
||||
assert.equal(preMod.startBound(fx.repo, start, fork).ok, false, 'the start is not bound');
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start, end }, [{ file: 'a.mjs', line: 5 }]), ['a.mjs:5=false']);
|
||||
assertUnanswered(gateMarked(fx, { start, end }, { file: 'a.mjs', line: 5 }));
|
||||
});
|
||||
|
||||
test('[PXB] a start whose one commit changes .claude/projects/p.md and src/x.mjs together does not bind', () => {
|
||||
const fx = pxbRepo({ seed: { 'src/x.mjs': ten('x') } });
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const start = fx.commit({
|
||||
'.claude/projects/p.md': '# p\n',
|
||||
'src/x.mjs': ten('x', { 2: 'export const x2 = 22;' }),
|
||||
}, 'chore: a project file and a source file');
|
||||
const end = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: change a.mjs line 3');
|
||||
const fork = forkOf(fx, end);
|
||||
assert.equal(typeof preMod.startBound, 'function', 'pre-existing.mjs exports startBound');
|
||||
assert.equal(preMod.startBound(fx.repo, start, fork).ok, false, 'the start is not bound');
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start, end }, [{ file: 'src/x.mjs', line: 7 }]), ['src/x.mjs:7=false']);
|
||||
});
|
||||
|
||||
test('[PXB] a finding on .claude/voyage/approvals.jsonl or on a file under .claude/projects/ is never pre-existing, under a bound start', () => {
|
||||
const notes = '.claude/projects/2026-11-02-p/notes.md';
|
||||
const findings = [{ file: LOG_REL, line: 1 }, { file: notes, line: 1 }, { file: 'a.mjs', line: 7 }];
|
||||
const want = [`${LOG_REL}:1=false`, `${notes}:1=false`, 'a.mjs:7=true'];
|
||||
assert.equal(typeof preMod.startBound, 'function', 'pre-existing.mjs exports startBound');
|
||||
// 1. The fork point itself: main holds the log and the project file.
|
||||
const one = pxbRepo({ seed: { [LOG_REL]: '{"seq":1}\n', [notes]: '# Notes\n' } });
|
||||
g(one.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const end1 = one.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: change a.mjs line 3');
|
||||
const fork1 = forkOf(one, end1);
|
||||
assert.equal(fork1, one.start);
|
||||
assert.equal(preMod.startBound(one.repo, one.start, fork1).ok, true, 'Control: the fork point is bound');
|
||||
assert.deepEqual(marksAfterFinalize(one, { start: one.start, end: end1 }, findings), want);
|
||||
// 2. A later start whose own process commit added the cited lines.
|
||||
const two = pxbRepo();
|
||||
g(two.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const start2 = two.commit({ [LOG_REL]: '{"seq":1}\n', [notes]: '# Notes\n' }, 'chore(voyage): log and notes');
|
||||
const end2 = two.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: change a.mjs line 3');
|
||||
const fork2 = forkOf(two, end2);
|
||||
assert.equal(preMod.startBound(two.repo, start2, fork2).ok, true, 'Control: the process-files start is bound');
|
||||
assert.deepEqual(marksAfterFinalize(two, { start: start2, end: end2 }, findings), want);
|
||||
});
|
||||
|
||||
test('[PXB] docs/HANDOVER-CONTRACTS.md\'s pre_existing row says it is never true unless scope_sha_start is bound to the fork point from main', () => {
|
||||
const text = readFileSync(join(ROOT, 'docs', 'HANDOVER-CONTRACTS.md'), 'utf8');
|
||||
const row = text.split('\n').find((l) => l.includes('per finding `pre_existing` (boolean:'));
|
||||
assert.ok(row, 'the Handover 6 pre_existing row');
|
||||
assert.ok(row.includes('the range did not add or change the cited line and `scope_sha_start` is bound to the fork point from main (the fork point itself, or a later commit reached through process files only); otherwise it is never true'), row);
|
||||
});
|
||||
|
||||
test('[PXB] forkPoint is the fork commit before the merge, after a --no-ff merge into main, and after main moved on; master is used when there is no main; neither branch is ok:false', () => {
|
||||
assert.equal(typeof preMod.forkPoint, 'function', 'pre-existing.mjs exports forkPoint');
|
||||
const fx = pxbRepo();
|
||||
const b = fx.start;
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: C1');
|
||||
const c2 = fx.commit({ 'd.mjs': ten('d', { 8: 'export const d8 = 88;' }) }, 'feat: C2');
|
||||
assert.deepEqual(preMod.forkPoint(fx.repo, c2), { ok: true, sha: b, ref: 'refs/heads/main' }, 'before the merge');
|
||||
g(fx.repo, 'checkout', '-q', 'main');
|
||||
fx.commit({ 'old.mjs': ten('o', { 1: 'export const o1 = 11;' }) }, 'feat: main moves on');
|
||||
assert.deepEqual(preMod.forkPoint(fx.repo, c2), { ok: true, sha: b, ref: 'refs/heads/main' }, 'after main moved on');
|
||||
g(fx.repo, 'merge', '-q', '--no-ff', '-m', 'merge feat', 'feat');
|
||||
assert.deepEqual(preMod.forkPoint(fx.repo, c2), { ok: true, sha: b, ref: 'refs/heads/main' }, 'after a --no-ff merge');
|
||||
const ms = pxbRepo({ branch: 'master' });
|
||||
g(ms.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const mEnd = ms.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: C1');
|
||||
assert.deepEqual(preMod.forkPoint(ms.repo, mEnd), { ok: true, sha: ms.start, ref: 'refs/heads/master' }, 'master');
|
||||
const none = pxbRepo({ branch: 'trunk' });
|
||||
g(none.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const nEnd = none.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: C1');
|
||||
assert.equal(preMod.forkPoint(none.repo, nEnd).ok, false, 'neither main nor master');
|
||||
});
|
||||
|
||||
test('[PXB] a range that ends on main itself: finalize marks nothing pre-existing on a line the range did not change', () => {
|
||||
const fx = pxbRepo();
|
||||
const end = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: straight on main');
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start: fx.start, end }, [{ file: 'a.mjs', line: 7 }]), ['a.mjs:7=false']);
|
||||
assertUnanswered(gateMarked(fx, { start: fx.start, end }, { file: 'a.mjs', line: 7 }));
|
||||
});
|
||||
|
||||
test('[PXB] after a --no-ff merge into main, with HEAD on main, decideOpenFindings with the true start leaves a MAJOR on an unchanged line out of the answer set', () => {
|
||||
assert.equal(typeof gateMod.decideOpenFindings, 'function', 'verdict-gate.mjs exports decideOpenFindings');
|
||||
const fx = pxbRepo();
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
const end = fx.commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }) }, 'feat: C1');
|
||||
g(fx.repo, 'checkout', '-q', 'main');
|
||||
g(fx.repo, 'merge', '-q', '--no-ff', '-m', 'merge feat', 'feat');
|
||||
assert.equal(g(fx.repo, 'rev-parse', 'HEAD^2'), end, 'HEAD is the --no-ff merge on main');
|
||||
const text = pxbReview({ start: fx.start, end, findings: [{ severity: 'MAJOR', file: 'a.mjs', line: 7, pre_existing: true }] });
|
||||
fx.write(text);
|
||||
const d = gateMod.decideOpenFindings(text, { repo: fx.repo, reviewPath: fx.reviewPath });
|
||||
assert.equal(d.exit, 0, JSON.stringify(d));
|
||||
// Control: a MAJOR on the line the branch changed stays in the answer set.
|
||||
const changed = pxbReview({ start: fx.start, end, findings: [{ severity: 'MAJOR', file: 'a.mjs', line: 3, pre_existing: true }] });
|
||||
fx.write(changed);
|
||||
assert.equal(gateMod.decideOpenFindings(changed, { repo: fx.repo, reviewPath: fx.reviewPath }).exit, 2);
|
||||
});
|
||||
|
||||
test('[PXB] a start older than the fork point does not bind: main changes line L between that start and the fork point, the branch puts L back, and a MAJOR on L is not pre-existing; the gate exits 2 review-unanswered', () => {
|
||||
const fx = pxbRepo();
|
||||
const older = fx.start;
|
||||
const fork = fx.commit({ 'a.mjs': ten('a', { 5: 'export const a5 = 55;' }) }, 'feat: main changes a.mjs line 5');
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
fx.commit({ 'a.mjs': ten('a') }, 'feat: put a.mjs line 5 back');
|
||||
const end = fx.commit({ 'd.mjs': ten('d', { 8: 'export const d8 = 88;' }) }, 'feat: change d.mjs');
|
||||
// The tree diff older..end does not show line 5.
|
||||
assert.equal(g(fx.repo, 'diff', '--name-only', older, end), 'd.mjs');
|
||||
assert.equal(forkOf(fx, end), fork);
|
||||
assert.equal(typeof preMod.startBound, 'function', 'pre-existing.mjs exports startBound');
|
||||
assert.equal(preMod.startBound(fx.repo, older, fork).ok, false, 'an older start is not bound');
|
||||
assert.deepEqual(marksAfterFinalize(fx, { start: older, end }, [{ file: 'a.mjs', line: 5 }]), ['a.mjs:5=false']);
|
||||
assertUnanswered(gateMarked(fx, { start: older, end }, { file: 'a.mjs', line: 5 }));
|
||||
});
|
||||
|
||||
test('[PXB] /trekreview Phase 7 says the range counts only when scope_sha_start is bound to the fork point from main', () => {
|
||||
const text = readFileSync(join(ROOT, 'commands', 'trekreview.md'), 'utf8').replace(/\s+/g, ' ');
|
||||
const want = 'The range counts only when `scope_sha_start` is bound to the fork point from main (`main`, else `master`): '
|
||||
+ 'the fork point itself, or a later commit reached through process files only (the approval log, `.claude/projects/`); '
|
||||
+ 'otherwise, and for a range that ends on main itself, nothing is pre-existing.';
|
||||
const phase7 = text.indexOf('## Phase 7');
|
||||
const phase8 = text.indexOf('## Phase 8 ');
|
||||
assert.ok(phase7 !== -1 && phase8 > phase7, 'Phase 7 and Phase 8 headings');
|
||||
const at = text.indexOf(want);
|
||||
assert.ok(at > phase7 && at < phase8, `Phase 7 holds: ${want}`);
|
||||
});
|
||||
|
|
@ -83,6 +83,7 @@ function prxRepo({ config = [], rounds } = {}) {
|
|||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'chore: start');
|
||||
const start = g(repo, 'rev-parse', 'HEAD');
|
||||
g(repo, 'checkout', '-q', '-b', 'feat');
|
||||
for (const c of config) g(repo, 'config', ...c);
|
||||
const plan = rounds ?? [{
|
||||
'a.mjs': ten('a', { 3: 'export const a3 = 33;' }),
|
||||
|
|
@ -386,6 +387,7 @@ test('[PRX] a symlink to a file the range changed is not a regular file: a findi
|
|||
g(fx.repo, 'add', '--', 'lnk.mjs');
|
||||
g(fx.repo, 'commit', '-q', '-m', 'chore: link');
|
||||
const start = g(fx.repo, 'rev-parse', 'HEAD');
|
||||
g(fx.repo, 'branch', '-f', 'main', start);
|
||||
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');
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue