feat(review): the code-correctness reviewer checks the repo's tracked instruction files

lib/review/instruction-scope.mjs

@@ -0,0 +1,218 @@
+// lib/review/instruction-scope.mjs
+// The target repo's instruction files for /trekreview (review_version 1.2, fix
+// round 4.5b2 part B): the code-correctness reviewer checks the diff against
+// exactly this set (rule keys CLAUDE_MD_DEVIATION and CLAUDE_MD_STALE, MINOR),
+// and lib/review/review-finalize.mjs sets aside under Declined to Judge
+// (`instruction-out-of-set`) a finding under those keys that cites a file
+// outside it.
+//
+// `instructionSet(repo, sha, changedPaths)` reads the tree of the reviewed
+// commit with `git ls-tree -r -z --full-tree`, so only a file tracked at that
+// commit can enter the set (never an untracked file, never one committed
+// later, never a file outside the repo such as a global ~/.claude/CLAUDE.md):
+//   - the root `CLAUDE.md` and `AGENTS.md`;
+//   - the same names in each ancestor directory of each changed path (a
+//     changed path that is absolute, holds `..`, `.`, an empty segment or a
+//     backslash names no directory of the repo and adds nothing);
+//   - `.claude/rules/*.md`.
+// An entry with mode 120000 (a symlink) has its target read with
+// `git cat-file`. A target resolved against the link's directory that stays
+// inside the repo and is a tracked regular file replaces the link; any other
+// target (absolute, out of the repo, missing, another link) drops the entry.
+// Every path is repo-relative posix. A git failure, or a sha that is not a
+// full commit id, gives `{ok:false, reason}`.
+//
+// `isInstructionFile(path)` is the deep-review classifier of /trekreview
+// Phase 4: the base name is `CLAUDE.md` or `AGENTS.md` at any depth, or the
+// path is a `.md` file under a `.claude/rules/` directory. `docs/**` is
+// included, so a changed instruction file there gets deep review although the
+// triage table makes the rest of `docs/**` summary-only.
+//
+// `changedPaths(repo, start, end)` is the changed-path list the filter in
+// lib/review/review-finalize.mjs builds the set from: the paths of
+// /trekreview Phase 5's `git -c core.quotePath=false diff --name-only`, here
+// NUL-separated and with flags no repo config changes (`--no-relative`,
+// `--no-ext-diff`, `--ignore-submodules=none`, `--no-renames`). A deleted path
+// is listed (its directory's CLAUDE.md is in the set the reviewer was given),
+// and `--no-renames` lists both sides of a rename, so the filter's set always
+// holds the prompt's set.
+//
+// CLI:
+//   node lib/review/instruction-scope.mjs --set --repo <d> --sha <s> --changed <file>
+//     <file> holds the changed paths, one per line (`git diff --name-only`).
+//     Prints {"ok": true, "files": [...]}; exit 0. A git failure or bad
+//     arguments print {"ok": false, "reason"}; exit 2.
+//   node lib/review/instruction-scope.mjs --triage <paths...>
+//     Prints {"files": [...]}, the given paths that are instruction files, in
+//     the order given; exit 0.
+//
+// Every git call is an argv array with no shell.
+
+import { spawnSync } from 'node:child_process';
+import { readFileSync } from 'node:fs';
+import { posix } from 'node:path';
+import { FULL_SHA_RE } from './reviewed-commit.mjs';
+import { isMain } from '../util/is-main.mjs';
+
+const MAX_BUFFER = 64 * 1024 * 1024;
+// The symlink mode of lib/review/reviewed-commit.mjs:48 (copied, not exported there).
+const SYMLINK_MODE = '120000';
+const REGULAR_MODES = new Set(['100644', '100755']);
+const TREE_ENTRY_RE = /^(\d{6}) ([a-z]+) ([0-9a-f]{40}|[0-9a-f]{64})$/;
+const NAMES = ['CLAUDE.md', 'AGENTS.md'];
+const RULES_RE = /^\.claude\/rules\/[^/]+\.md$/;
+
+// 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 });
+}
+
+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 { ok: false, reason: `instruction set not measured: git ${what} failed (${note || `exit ${r.status}`})` };
+}
+
+/** A repo-relative posix path: no leading `/`, no `.`, `..` or empty segment, no backslash. */
+function plainPath(p) {
+  return typeof p === 'string' && p !== '' && !p.includes('\\') && !p.includes('\0')
+    && p.split('/').every((s) => s !== '' && s !== '.' && s !== '..');
+}
+
+/** Every ancestor directory of a plain path, the root ('') first. */
+function ancestors(p) {
+  const parts = p.split('/').slice(0, -1);
+  const out = [''];
+  for (let i = 1; i <= parts.length; i++) out.push(parts.slice(0, i).join('/'));
+  return out;
+}
+
+/** The tree of `sha`: Map<path, {mode, type, hash}>, or {ok:false}. */
+function treeAt(repo, sha) {
+  const r = git(repo, ['ls-tree', '-r', '-z', '--full-tree', sha]);
+  if (r.error || r.status !== 0) return gitFailed(`ls-tree ${sha.slice(0, 12)}`, r);
+  const entries = 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 { ok: false, reason: `instruction set not measured: cannot read ls-tree entry ${JSON.stringify(entry.slice(0, 120))}` };
+    entries.set(entry.slice(tab + 1), { mode: m[1], type: m[2], hash: m[3] });
+  }
+  return { ok: true, entries };
+}
+
+/**
+ * The path an instruction entry stands for: itself when it is a regular file;
+ * for a symlink, its tracked regular target inside the repo; else null.
+ */
+function resolveEntry(repo, entries, path) {
+  const e = entries.get(path);
+  if (!e || e.type !== 'blob') return { ok: true, path: null };
+  if (REGULAR_MODES.has(e.mode)) return { ok: true, path };
+  if (e.mode !== SYMLINK_MODE) return { ok: true, path: null };
+  const r = git(repo, ['cat-file', 'blob', e.hash]);
+  if (r.error || r.status !== 0) return gitFailed(`cat-file ${path}`, r);
+  const target = r.stdout;
+  if (target === '' || target.includes('\0') || target.includes('\\') || posix.isAbsolute(target)) return { ok: true, path: null };
+  const resolved = posix.normalize(posix.join(posix.dirname(path), target));
+  if (!plainPath(resolved)) return { ok: true, path: null };
+  const t = entries.get(resolved);
+  return { ok: true, path: t && t.type === 'blob' && REGULAR_MODES.has(t.mode) ? resolved : null };
+}
+
+/**
+ * The paths changed over `start..end`, deleted ones included.
+ * @param {string} repo
+ * @param {string} start - a full commit id
+ * @param {string} end - a full commit id
+ * @returns {{ok: true, paths: string[]} | {ok: false, reason: string}}
+ */
+export function changedPaths(repo, start, end) {
+  if (!repo) return { ok: false, reason: 'changed paths not measured: no repo' };
+  if (!FULL_SHA_RE.test(String(start ?? '')) || !FULL_SHA_RE.test(String(end ?? ''))) {
+    return { ok: false, reason: 'changed paths not measured: start and end must be full commit ids' };
+  }
+  const r = git(repo, ['-c', 'core.quotePath=false', 'diff', '--name-only', '-z', '--no-relative', '--no-ext-diff',
+    '--ignore-submodules=none', '--no-renames', start, end]);
+  if (r.error || r.status !== 0) return gitFailed('diff --name-only', r);
+  return { ok: true, paths: r.stdout.split('\0').filter((p) => p !== '') };
+}
+
+/**
+ * The repo's tracked instruction files at `sha` for the changed paths.
+ * @param {string} repo
+ * @param {string} sha - a full commit id
+ * @param {string[]} changedPaths - repo-relative paths; any other is ignored
+ * @returns {{ok: true, files: string[]} | {ok: false, reason: string}}
+ */
+export function instructionSet(repo, sha, changedPaths = []) {
+  if (!repo) return { ok: false, reason: 'instruction set not measured: no repo' };
+  if (!FULL_SHA_RE.test(String(sha ?? ''))) return { ok: false, reason: 'instruction set not measured: the sha must be a full commit id' };
+  const commit = git(repo, ['rev-parse', '--verify', '--quiet', `${sha}^{commit}`]);
+  if (commit.error || commit.status !== 0) return gitFailed(`rev-parse ${sha.slice(0, 12)}`, commit);
+  const tree = treeAt(repo, sha);
+  if (!tree.ok) return tree;
+  const dirs = new Set(['']);
+  for (const p of Array.isArray(changedPaths) ? changedPaths : []) {
+    if (plainPath(p)) for (const d of ancestors(p)) dirs.add(d);
+  }
+  const candidates = [];
+  for (const d of dirs) for (const n of NAMES) candidates.push(d === '' ? n : `${d}/${n}`);
+  for (const p of tree.entries.keys()) if (RULES_RE.test(p)) candidates.push(p);
+  const files = new Set();
+  for (const c of candidates) {
+    const r = resolveEntry(repo, tree.entries, c);
+    if (!r.ok) return r;
+    if (r.path !== null) files.add(r.path);
+  }
+  return { ok: true, files: [...files].sort() };
+}
+
+/**
+ * Does a changed file get deep review as an instruction file? The base name
+ * is `CLAUDE.md` or `AGENTS.md` at any depth, or it is a `.md` file under a
+ * `.claude/rules/` directory; `docs/**` included.
+ * @param {string} path
+ * @returns {boolean}
+ */
+export function isInstructionFile(path) {
+  if (typeof path !== 'string' || path === '') return false;
+  const p = path.replace(/^\.\//, '');
+  if (NAMES.includes(posix.basename(p))) return true;
+  return /(?:^|\/)\.claude\/rules\/(?:[^/]+\/)*[^/]+\.md$/.test(p);
+}
+
+function opt(args, name) {
+  const i = args.indexOf(name);
+  return i >= 0 ? args[i + 1] : undefined;
+}
+
+/** The CLI's work: {exit, out}. */
+export function runCli(args) {
+  if (args[0] === '--triage') return { exit: 0, out: { files: args.slice(1).filter(isInstructionFile) } };
+  if (args[0] !== '--set') return { exit: 2, out: { ok: false, reason: 'usage: --set --repo <d> --sha <s> --changed <file> | --triage <paths...>' } };
+  const repo = opt(args, '--repo');
+  const sha = opt(args, '--sha');
+  const changed = opt(args, '--changed');
+  if (!repo || !sha || !changed) return { exit: 2, out: { ok: false, reason: '--set needs --repo, --sha and --changed' } };
+  let list;
+  try {
+    list = readFileSync(changed, 'utf8').split('\n').map((l) => l.replace(/\r$/, '')).filter((l) => l !== '');
+  } catch (e) {
+    return { exit: 2, out: { ok: false, reason: `cannot read --changed ${changed}: ${e.message}` } };
+  }
+  const r = instructionSet(repo, sha, list);
+  return { exit: r.ok ? 0 : 2, out: r };
+}
+
+if (isMain(import.meta.url)) {
+  let result;
+  try {
+    result = runCli(process.argv.slice(2));
+  } catch (e) {
+    result = { exit: 2, out: { ok: false, reason: `instruction-scope crashed: ${e.message}` } };
+  }
+  process.stdout.write(`${JSON.stringify(result.out)}\n`);
+  process.exit(result.exit);
+}

lib/review/review-finalize.mjs

@@ -2,8 +2,7 @@
 // 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 engine, carry, pre-existing and policy:
+// instruction, carry, pre-existing, policy), each at its place in PASSES:
 //
 //   - engine, always the first pass: `putBack` (lib/review/engine-compare.mjs)
 //     compares the engine payloads Phase 4.7 saved in the project folder
@@ -18,6 +17,22 @@
 //     the frontmatter list, counts, a verdict that is only raised, and, as a
 //     BLOCKER, its `### Step R<n>`. An engine finding in `out_of_scope` is
 //     not compared (it stays declined, `engine:out-of-scope`).
+//   - instruction, right after engine: a finding under `CLAUDE_MD_DEVIATION`
+//     or `CLAUDE_MD_STALE` whose `instruction_ref` is missing or names a file
+//     outside the repo's tracked instruction set (lib/review/instruction-scope.mjs
+//     `instructionSet` at `scope_sha_end`, for the paths changed over
+//     `scope_sha_start..scope_sha_end`) leaves the JSON findings, the
+//     frontmatter list, `counts` and its `### ` entry under
+//     `## Findings (MINOR|SUGGESTION)`, and goes to `declined_to_judge` with
+//     `instruction-out-of-set` and its `instruction_ref` (the cited file, or
+//     null); `## Declined to Judge` gets one line for it, by title, naming
+//     the cited file. A git failure or no range is an empty set, so every
+//     such finding is declined. Only a MINOR or SUGGESTION that is not a
+//     security finding moves; one whose entry is not found exactly once
+//     there, that an R-step names or that finalize added stays reported
+//     (stricter). The same breach reported by an engine and a reviewer
+//     (same triplet) was already merged by the coordinator's dedup, which
+//     keeps the higher severity.
 //   - carry (round 2 and later, `--previous <archive>`): `carry`
 //     (lib/review/review-rounds.mjs) keeps every previous finding that this
 //     round gives no positive evidence of being resolved, with `carried_from`;
@@ -90,6 +105,8 @@ import { computeFindingIds } from '../parsers/finding-id.mjs';
 import { fencedBlocks, isJsonBlock, validateReviewerOutput } from './findings-schema.mjs';
 import { FULL_SHA_RE } from './reviewed-commit.mjs';
 import { markFindings, scopeShas } from './pre-existing.mjs';
+import { changedPaths, instructionSet } from './instruction-scope.mjs';
+import { isSecurityFinding } from './review-answers.mjs';
 import { ENGINES } from './engine-findings.mjs';
 import { carry, frontmatterField, MAX_REVIEW_ROUNDS, runId } from './review-rounds.mjs';
 import { applyPolicy, readPolicy } from './review-policy.mjs';
@@ -220,6 +237,94 @@ function passEngine(payload, ctx) {
   return { headEdit: (head) => withVerdict(head, r.raisedVerdict) };
 }
 
+// ---- pass: instruction -------------------------------------------------------------
+
+const INSTRUCTION_KEYS = new Set(['CLAUDE_MD_DEVIATION', 'CLAUDE_MD_STALE']);
+// One of coordinator-contract `NOTED_REASONS`: the gate accepts it on a MINOR or SUGGESTION only.
+const OUT_OF_SET = 'instruction-out-of-set';
+const INSTRUCTION_MOVABLE = new Set(['MINOR', 'SUGGESTION']);
+
+/**
+ * The repo's tracked instruction files at `scope_sha_end` for the paths
+ * changed over the review's range (`changedPaths`, the list of Phase 5's
+ * `git diff --name-only`, deleted paths included); an empty set when either
+ * cannot be read.
+ */
+function instructionFilesOf(repo, text) {
+  const { start, end } = scopeShas(text);
+  const changed = changedPaths(repo, start, end);
+  if (!changed.ok) return { measured: false, files: new Set() };
+  const r = instructionSet(repo, end, changed.paths);
+  return r.ok ? { measured: true, files: new Set(r.files) } : { measured: false, files: new Set() };
+}
+
+/** One `## Declined to Judge` line for a finding the instruction filter moved. */
+function instructionLine(d) {
+  const cited = typeof d.instruction_ref === 'string' && oneLine(d.instruction_ref) !== ''
+    ? oneLine(d.instruction_ref) : 'no instruction file';
+  return `- ${oneLine(d.file) || '(no file)'}:${d.line ?? '?'} ${oneLine(d.rule_key) || '(no rule_key)'} — ${OUT_OF_SET}, cites ${cited} (${oneLine(d.title) || '(no title)'})`;
+}
+
+/**
+ * Pass `instruction`, right after engine: a CLAUDE.md-key finding that cites
+ * no file of the instruction set goes to Declined to Judge.
+ */
+function passInstruction(payload, ctx) {
+  const keyed = payload.findings.filter((f) => INSTRUCTION_KEYS.has(f.rule_key));
+  if (keyed.length === 0) return {};
+  if (payload.declined_to_judge !== undefined && !Array.isArray(payload.declined_to_judge)) {
+    ctx.instruction = { measured: false, moved: 0 };
+    return {};
+  }
+  const set = instructionFilesOf(ctx.repo, ctx.text);
+  const lines = ctx.head.split('\n');
+  const fmEnd = frontmatterEnd(lines);
+  const named = new Set(remediationSteps(lines).steps.map((st) => st.source).filter(Boolean));
+  const take = keyed.filter((f) => (typeof f.instruction_ref !== 'string' || !set.files.has(f.instruction_ref))
+    && INSTRUCTION_MOVABLE.has(f.severity) && !isSecurityFinding(f)
+    && typeof f.id === 'string' && HEX40.test(f.id) && !named.has(f.id) && !ctx.added.has(f)
+    && movableEntry(lines, fmEnd, f.id) !== null);
+  ctx.instruction = { measured: set.measured, moved: take.length };
+  if (take.length === 0) return {};
+  const declined = take.map((f) => ({
+    file: f.file ?? null,
+    line: f.line ?? null,
+    rule_key: f.rule_key ?? null,
+    severity: f.severity ?? null,
+    title: f.title ?? null,
+    ...(f.engine !== undefined ? { engine: f.engine } : {}),
+    instruction_ref: typeof f.instruction_ref === 'string' ? f.instruction_ref : null,
+    reason: OUT_OF_SET,
+  }));
+  const takeSet = new Set(take);
+  payload.findings = payload.findings.filter((f) => !takeSet.has(f));
+  payload.declined_to_judge = [...(payload.declined_to_judge ?? []), ...declined];
+  ctx.recount = true;
+  const ids = take.map((f) => f.id);
+  return {
+    headEdit: (head) => {
+      let out = head.split('\n');
+      const fe = frontmatterEnd(out);
+      for (const id of ids) {
+        const e = movableEntry(out, fe, id);
+        if (e) out.splice(e.at, e.end - e.at);
+      }
+      out = emptySectionsSayNone(out);
+      const at = out.findIndex((l) => DECLINED_RE.test(l));
+      if (at !== -1) {
+        for (let i = sectionEnd(out, at) - 1; i > at; i--) {
+          if (out[i].trim() !== 'None.') continue;
+          out.splice(i, 1);
+          if (out[i] === '' && out[i - 1] === '') out.splice(i, 1);
+        }
+      }
+      out = appendToSection(out, DECLINED_RE, '## Declined to Judge', declined.map(instructionLine),
+        [/^##\s+Remediation Summary\b/, /^##\s+Remediation Steps\b/, /^##\s+Answers\s*$/]);
+      return out.join('\n');
+    },
+  };
+}
+
 // ---- pass: carry -------------------------------------------------------------------
 
 const LATER_SECTIONS = [/^##\s+Suppressed Findings\b/, /^##\s+Pre-existing\s*$/, /^##\s+Resolved since round\b/,
@@ -425,6 +530,7 @@ function passPolicy(payload, ctx) {
 /** The passes, in the Architecture order; the id recomputation runs after them, always last. */
 export const PASSES = Object.freeze([
   { name: 'engine', run: passEngine },
+  { name: 'instruction', run: passInstruction },
   { name: 'carry', run: passCarry },
   { name: 'pre-existing', run: passPreExisting },
   { name: 'policy', run: passPolicy },
@@ -669,7 +775,7 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
   return {
     exit: 0, text: `${head}${parts.fence}${body}${parts.tail}`, ...out, ids_rewritten: map.size,
     carried: ctx.added.size, resolved: payload.resolved ?? [], ran: payload.ran, r_steps_added: steps.added,
-    policy: ctx.policy ?? null, put_back: ctx.engine?.put_back ?? 0,
+    policy: ctx.policy ?? null, put_back: ctx.engine?.put_back ?? 0, instruction: ctx.instruction ?? null,
   };
 }