fix(review): the instruction set holds nested rules files and a link's own name
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
d81bd39754
commit
d4daa9d787
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 287 additions and 19 deletions
|
|
@ -72,8 +72,8 @@ test('[CAT] the armB RULE_TIERS table holds both keys at MINOR', () => {
|
|||
// --- CMD / DPR: the instruction set and its filter (Step 10) ---------------------------
|
||||
// [CMD] lib/review/instruction-scope.mjs builds the set of the repo's tracked
|
||||
// instruction files at the reviewed commit (root and on-path CLAUDE.md and
|
||||
// AGENTS.md, .claude/rules/*.md; a symlink inside the repo is replaced by its
|
||||
// tracked target, one out of it is dropped), and review-finalize.mjs sets aside
|
||||
// AGENTS.md, .claude/rules/*.md; a symlink inside the repo keeps its own name
|
||||
// next to its tracked target, one out of it is dropped), and review-finalize.mjs sets aside
|
||||
// under Declined to Judge a CLAUDE.md-key finding that cites a file outside it.
|
||||
// [DPR] a changed instruction file at any depth, docs/** included, gets deep
|
||||
// review (`isInstructionFile`). Each case runs in a throwaway git repo with no
|
||||
|
|
@ -173,7 +173,7 @@ function scopeRepo() {
|
|||
|
||||
/** The set of the end commit for the three changed paths of the end commit. */
|
||||
const CHANGED = ['sub/deep/x.mjs', 'inner/y.mjs', 'lnk/z.mjs'];
|
||||
const EXPECTED_SET = ['.claude/rules/a.md', 'AGENTS.md', 'CLAUDE.md', 'docs/rules.md', 'sub/CLAUDE.md', 'sub/deep/AGENTS.md'];
|
||||
const EXPECTED_SET = ['.claude/rules/a.md', 'AGENTS.md', 'CLAUDE.md', 'docs/rules.md', 'inner/CLAUDE.md', 'sub/CLAUDE.md', 'sub/deep/AGENTS.md'];
|
||||
|
||||
test('[CMD] the set holds the repo\'s tracked root and on-path CLAUDE.md/AGENTS.md and tracked .claude/rules/*.md', () => {
|
||||
assert.equal(typeof scopeMod.instructionSet, 'function', 'instruction-scope.mjs exports instructionSet');
|
||||
|
|
@ -181,7 +181,7 @@ test('[CMD] the set holds the repo\'s tracked root and on-path CLAUDE.md/AGENTS.
|
|||
const r = scopeMod.instructionSet(fx.repo, fx.end, CHANGED);
|
||||
assert.equal(r.ok, true, JSON.stringify(r));
|
||||
assert.deepEqual([...r.files].sort(), EXPECTED_SET,
|
||||
'root CLAUDE.md and AGENTS.md, sub/CLAUDE.md and sub/deep/AGENTS.md on the changed path, .claude/rules/a.md, and docs/rules.md for the inner/CLAUDE.md link');
|
||||
'root CLAUDE.md and AGENTS.md, sub/CLAUDE.md and sub/deep/AGENTS.md on the changed path, .claude/rules/a.md, and inner/CLAUDE.md with its target docs/rules.md');
|
||||
assert.ok(r.files.every((p) => !isAbsolute(p) && !p.startsWith('..')), `every path is repo-relative: ${r.files.join(', ')}`);
|
||||
// Control: with no changed path the set still holds the root files and the rules.
|
||||
const bare = scopeMod.instructionSet(fx.repo, fx.end, []);
|
||||
|
|
|
|||
245
tests/lib/instruction-scope-depth.test.mjs
Normal file
245
tests/lib/instruction-scope-depth.test.mjs
Normal file
|
|
@ -0,0 +1,245 @@
|
|||
// tests/lib/instruction-scope-depth.test.mjs
|
||||
// Fix round for review gate 4.5b2, Step 3 (tags NRL and SYM): the instruction
|
||||
// set of lib/review/instruction-scope.mjs follows the deep-review classifier
|
||||
// for rules files, and a valid link keeps its own name next to its target.
|
||||
// NRL: a tracked `.md` under `<d>/.claude/rules/` at any depth is in the set,
|
||||
// with `d` the root or a directory on a changed path; a finding citing one
|
||||
// stays in findings after finalize. SYM: a tracked CLAUDE.md -> AGENTS.md link
|
||||
// puts both names in the set; a finding citing the link name stays reported.
|
||||
// Untagged `keep:` cases hold before and after the change. Each case runs in a
|
||||
// throwaway git repo with no hooks; every child process gets childEnv().
|
||||
|
||||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { appendFileSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, isAbsolute, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import * as m from '../../lib/review/instruction-scope.mjs';
|
||||
import { computeFindingIds } from '../../lib/parsers/finding-id.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const SCOPE = join(ROOT, 'lib', 'review', 'instruction-scope.mjs');
|
||||
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
|
||||
const REVIEW_REL = '.claude/projects/2026-11-03-depth/review.md';
|
||||
|
||||
const made = [];
|
||||
after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
|
||||
|
||||
// Copied from tests/lib/claude-md-scope.test.mjs (g, from review-policy.test.mjs).
|
||||
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();
|
||||
}
|
||||
|
||||
// Copied from tests/lib/claude-md-scope.test.mjs (putIn, from review-policy.test.mjs).
|
||||
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 {}; } };
|
||||
const three = (tag) => `export const ${tag}1 = 1;\nexport const ${tag}2 = 2;\nexport const ${tag}3 = 3;\n`;
|
||||
|
||||
/**
|
||||
* A throwaway repo beside an `outside` folder (shape of scopeRepo in
|
||||
* tests/lib/claude-md-scope.test.mjs, copied and changed for these cases).
|
||||
* The start commit holds: AGENTS.md (regular) and CLAUDE.md -> AGENTS.md (a
|
||||
* link), .claude/rules/a.md, .claude/rules/sub/x.md, .claude/rules/notes.txt,
|
||||
* .claude/rules/sub/notes.txt, pkg/.claude/rules/x.md, and out/CLAUDE.md ->
|
||||
* ../../outside/CLAUDE.md (out of the repo). The end commit (the reviewed one)
|
||||
* changes src/a.mjs, pkg/src/p.mjs and out/z.mjs.
|
||||
*/
|
||||
function depthRepo() {
|
||||
const base = mkdtempSync(join(tmpdir(), 'instruction-scope-depth-'));
|
||||
made.push(base);
|
||||
const repo = join(base, 'repo');
|
||||
const outside = join(base, 'outside');
|
||||
mkdirSync(repo, { recursive: true });
|
||||
mkdirSync(join(base, 'hooks-none'));
|
||||
putIn(outside, 'CLAUDE.md', '# outside the repo\n');
|
||||
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');
|
||||
for (const rel of ['AGENTS.md', '.claude/rules/a.md', '.claude/rules/sub/x.md', '.claude/rules/notes.txt',
|
||||
'.claude/rules/sub/notes.txt', 'pkg/.claude/rules/x.md']) putIn(repo, rel, `# ${rel}\n`);
|
||||
for (const [rel, tag] of [['src/a.mjs', 'a'], ['pkg/src/p.mjs', 'p'], ['out/z.mjs', 'z']]) putIn(repo, rel, three(tag));
|
||||
symlinkSync('AGENTS.md', join(repo, 'CLAUDE.md'));
|
||||
symlinkSync('../../outside/CLAUDE.md', join(repo, 'out', 'CLAUDE.md'));
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'chore: start');
|
||||
const start = g(repo, 'rev-parse', 'HEAD');
|
||||
for (const [rel, tag] of [['src/a.mjs', 'a'], ['pkg/src/p.mjs', 'p'], ['out/z.mjs', 'z']]) {
|
||||
putIn(repo, rel, three(tag).replace(`${tag}1 = 1;`, `${tag}1 = 11;`));
|
||||
}
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'feat: the work');
|
||||
const end = g(repo, 'rev-parse', 'HEAD');
|
||||
const reviewPath = join(repo, ...REVIEW_REL.split('/'));
|
||||
return {
|
||||
base, repo, outside, start, end, reviewPath,
|
||||
write: (text) => putIn(repo, REVIEW_REL, text),
|
||||
read: () => readFileSync(reviewPath, 'utf8'),
|
||||
finalize: () => spawnSync(process.execPath, [FINALIZE, '--review', reviewPath, '--repo', repo,
|
||||
'--project', dirname(reviewPath), '--round', '1'], { encoding: 'utf8', env: childEnv() }),
|
||||
};
|
||||
}
|
||||
|
||||
/** The set through the module, with the export checked first so a missing one is a failed assertion. */
|
||||
function setOf(fx, changed) {
|
||||
assert.equal(typeof m.instructionSet, 'function', 'instruction-scope.mjs exports instructionSet');
|
||||
const r = m.instructionSet(fx.repo, fx.end, changed);
|
||||
assert.equal(r.ok, true, JSON.stringify(r));
|
||||
assert.ok(r.files.every((p) => !isAbsolute(p) && !p.split('/').includes('..')), `every path is repo-relative: ${r.files.join(', ')}`);
|
||||
return r.files;
|
||||
}
|
||||
|
||||
// Shape copied from tests/lib/claude-md-scope.test.mjs (cmdReview), MINOR findings only.
|
||||
function depthReview({ start, end, findings }) {
|
||||
const list = findings.map((f, i) => ({
|
||||
severity: 'MINOR', rule_key: f.rule_key, file: f.file, line: f.line, title: f.title,
|
||||
detail: `d${i + 1}`, recommended_action: 'Fix the line.',
|
||||
...(f.instruction_ref !== undefined ? { instruction_ref: f.instruction_ref } : {}),
|
||||
}));
|
||||
const ids = computeFindingIds(list);
|
||||
const ordered = list.map((f, i) => ({ id: ids[i], ...f }));
|
||||
const counts = { BLOCKER: 0, MAJOR: 0, MINOR: ordered.length, SUGGESTION: 0 };
|
||||
const json = JSON.stringify({ verdict: 'ALLOW', counts, allow_blocked_by: [], declined_to_judge: [], findings: ordered }, null, 2);
|
||||
const entries = ordered.map((f) => `### ${f.id}\n\n- **Rule:** \`${f.rule_key}\`\n- **Location:** \`${f.file}:${f.line}\`\n- **Title:** ${f.title}\n`).join('\n');
|
||||
return `---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
task: "t"
|
||||
slug: depth
|
||||
project_dir: .claude/projects/2026-11-03-depth/
|
||||
brief_path: .claude/projects/2026-11-03-depth/brief.md
|
||||
scope_sha_start: ${start}
|
||||
scope_sha_end: ${end}
|
||||
reviewed_files_count: 3
|
||||
verdict: ALLOW
|
||||
findings:
|
||||
${ordered.map((f) => ` - ${f.id}`).join('\n')}
|
||||
---
|
||||
|
||||
# Review: t
|
||||
|
||||
## Executive Summary
|
||||
|
||||
x
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| src/a.mjs | deep-review | default |
|
||||
| pkg/src/p.mjs | deep-review | default |
|
||||
| out/z.mjs | deep-review | default |
|
||||
|
||||
## Findings (MINOR)
|
||||
|
||||
${entries}
|
||||
## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
x
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
None.
|
||||
|
||||
\`\`\`json
|
||||
${json}
|
||||
\`\`\`
|
||||
`;
|
||||
}
|
||||
|
||||
const jsonOf = (text) => JSON.parse(text.slice(text.lastIndexOf('```json\n') + 8, text.lastIndexOf('```')));
|
||||
|
||||
/** Finalize a review with one CLAUDE.md-key finding plus one under another key; return the payload. */
|
||||
function finalizeWith(fx, instructionRef, title) {
|
||||
fx.write(depthReview({ start: fx.start, end: fx.end, findings: [
|
||||
{ rule_key: 'CLAUDE_MD_DEVIATION', file: 'src/a.mjs', line: 1, title, instruction_ref: instructionRef },
|
||||
{ rule_key: 'MISSING_ERROR_HANDLING', file: 'src/a.mjs', line: 2, title: 'another key' },
|
||||
] }));
|
||||
const r = fx.finalize();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
return jsonOf(fx.read());
|
||||
}
|
||||
|
||||
test('[NRL] a tracked .claude/rules/sub/x.md is in the set', () => {
|
||||
const fx = depthRepo();
|
||||
const files = setOf(fx, []);
|
||||
assert.ok(files.includes('.claude/rules/sub/x.md'), `the nested rules file is in the set: ${files.join(', ')}`);
|
||||
assert.ok(files.includes('.claude/rules/a.md'), `Control: the top-level rules file stays: ${files.join(', ')}`);
|
||||
// The CLI prints the same set.
|
||||
const list = join(fx.base, 'changed.txt');
|
||||
writeFileSync(list, 'src/a.mjs\n');
|
||||
const cli = spawnSync(process.execPath, [SCOPE, '--set', '--repo', fx.repo, '--sha', fx.end, '--changed', list],
|
||||
{ encoding: 'utf8', env: childEnv() });
|
||||
assert.equal(cli.status, 0, cli.stdout + cli.stderr);
|
||||
assert.ok((parse(cli).files ?? []).includes('.claude/rules/sub/x.md'), `the CLI set holds the nested rules file: ${cli.stdout}`);
|
||||
});
|
||||
|
||||
test('[NRL] pkg/.claude/rules/x.md is in the set when a changed path is under pkg/', () => {
|
||||
const fx = depthRepo();
|
||||
const on = setOf(fx, ['pkg/src/p.mjs']);
|
||||
assert.ok(on.includes('pkg/.claude/rules/x.md'), `a changed path under pkg/ brings pkg/.claude/rules/x.md: ${on.join(', ')}`);
|
||||
const off = setOf(fx, ['src/a.mjs']);
|
||||
assert.ok(!off.includes('pkg/.claude/rules/x.md'), `with no changed path under pkg/ it is left out: ${off.join(', ')}`);
|
||||
const none = setOf(fx, []);
|
||||
assert.ok(!none.includes('pkg/.claude/rules/x.md'), `with no changed path it is left out: ${none.join(', ')}`);
|
||||
});
|
||||
|
||||
test('[NRL] a CLAUDE_MD_DEVIATION finding citing .claude/rules/sub/x.md stays in findings after finalize', () => {
|
||||
const fx = depthRepo();
|
||||
const json = finalizeWith(fx, '.claude/rules/sub/x.md', 'cites a nested rules file');
|
||||
assert.deepEqual(json.findings.map((f) => f.title).sort(), ['another key', 'cites a nested rules file'],
|
||||
'the finding citing the nested rules file stays reported');
|
||||
assert.deepEqual((json.declined_to_judge ?? []).filter((d) => d.reason === 'instruction-out-of-set').map((d) => d.title), [],
|
||||
'it is not moved to Declined to Judge');
|
||||
});
|
||||
|
||||
test('[SYM] a tracked CLAUDE.md -> AGENTS.md link puts both names in the set', () => {
|
||||
const fx = depthRepo();
|
||||
assert.equal(g(fx.repo, 'ls-tree', fx.end, 'CLAUDE.md').split(' ')[0], '120000', 'Control: CLAUDE.md is tracked as a link');
|
||||
const files = setOf(fx, ['src/a.mjs']);
|
||||
assert.ok(files.includes('CLAUDE.md'), `the link's own name is in the set: ${files.join(', ')}`);
|
||||
assert.ok(files.includes('AGENTS.md'), `its target is in the set: ${files.join(', ')}`);
|
||||
});
|
||||
|
||||
test('[SYM] a finding citing the link name stays in findings after finalize', () => {
|
||||
const fx = depthRepo();
|
||||
const json = finalizeWith(fx, 'CLAUDE.md', 'cites the link name');
|
||||
assert.deepEqual(json.findings.map((f) => f.title).sort(), ['another key', 'cites the link name'],
|
||||
'the finding citing CLAUDE.md, a link to AGENTS.md, stays reported');
|
||||
assert.deepEqual((json.declined_to_judge ?? []).filter((d) => d.reason === 'instruction-out-of-set').map((d) => d.title), [],
|
||||
'it is not moved to Declined to Judge');
|
||||
});
|
||||
|
||||
test('keep: a link to outside the repo still adds nothing', () => {
|
||||
const fx = depthRepo();
|
||||
const files = setOf(fx, ['out/z.mjs']);
|
||||
assert.ok(!files.includes('out/CLAUDE.md'), `the link out of the repo is left out: ${files.join(', ')}`);
|
||||
assert.ok(!files.some((p) => p.includes('outside') || p.includes(fx.outside)), `nothing outside the repo enters: ${files.join(', ')}`);
|
||||
const json = finalizeWith(fx, 'out/CLAUDE.md', 'cites a link out of the repo');
|
||||
assert.deepEqual((json.declined_to_judge ?? []).filter((d) => d.reason === 'instruction-out-of-set').map((d) => d.title),
|
||||
['cites a link out of the repo'], 'a finding citing the outside link is still declined');
|
||||
});
|
||||
|
||||
test('keep: notes.txt under .claude/rules is not in the set', () => {
|
||||
const fx = depthRepo();
|
||||
const files = setOf(fx, ['src/a.mjs', 'pkg/src/p.mjs']);
|
||||
for (const left of ['.claude/rules/notes.txt', '.claude/rules/sub/notes.txt']) {
|
||||
assert.ok(!files.includes(left), `${left} is left out: ${files.join(', ')}`);
|
||||
}
|
||||
assert.ok(files.every((p) => p.endsWith('.md')), `every file in the set is a .md file: ${files.join(', ')}`);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue