fix(review): a failed git check-ignore writes no pointer, whatever language git speaks
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
eca974aaa6
commit
1b9d5fd459
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 314 additions and 5 deletions
|
|
@ -40,7 +40,7 @@
|
|||
import {
|
||||
readFileSync, writeFileSync, renameSync, existsSync, mkdirSync, readdirSync, statSync, realpathSync, lstatSync,
|
||||
} from 'node:fs';
|
||||
import { join, relative, isAbsolute, sep } from 'node:path';
|
||||
import { join, relative, isAbsolute, sep, delimiter, resolve, dirname } from 'node:path';
|
||||
import { execFileSync, spawnSync } from 'node:child_process';
|
||||
import { validateReviewContent } from '../validators/review-validator.mjs';
|
||||
import { extractFindingsBlock } from './findings-schema.mjs';
|
||||
|
|
@ -327,8 +327,15 @@ function atomicWriteText(repo, rel, text) {
|
|||
|
||||
/**
|
||||
* Is the lessons file ignored by git? `git check-ignore -q` exit 0 = ignored,
|
||||
* 1 = not ignored, 128 = not a git repository (treated as not ignored); a spawn
|
||||
* error or any other exit is a failed check.
|
||||
* 1 = not ignored; a spawn error or any exit other than 0, 1 or 128 is a
|
||||
* failed check. Exit 128 means "not a git repository" only some of the time:
|
||||
* inside a repository it also comes from a path beyond a symbolic link, a
|
||||
* broken `.git/HEAD` or a `GIT_DIR` that does not exist (CKI). So on 128 a
|
||||
* probe decides (see notARepoReason), and only a directory that is truly no
|
||||
* repository gives `no-git-repo`; everything else, including a probe that
|
||||
* throws, is `failed`. No branch reads git's message text, so the answer does
|
||||
* not depend on the language git speaks. Every `failed` note after a 128
|
||||
* starts `git check-ignore exit 128: <first stderr line>` and adds the reason.
|
||||
* @returns {{ state: 'ignored' | 'not-ignored' | 'no-git-repo' | 'failed', note?: string }}
|
||||
*/
|
||||
function lessonsIgnored(repo, rel) {
|
||||
|
|
@ -336,12 +343,65 @@ function lessonsIgnored(repo, rel) {
|
|||
if (r.error) return { state: 'failed', note: `git check-ignore could not start: ${r.error.message}` };
|
||||
if (r.status === 0) return { state: 'ignored' };
|
||||
if (r.status === 1) return { state: 'not-ignored' };
|
||||
if (r.status === 128) return { state: 'no-git-repo' };
|
||||
const err = String(r.stderr ?? '').trim().split('\n')[0] ?? '';
|
||||
if (r.status === 128) {
|
||||
const head = `git check-ignore exit 128: ${err}`;
|
||||
try {
|
||||
const why = notARepoReason(repo);
|
||||
return why === null ? { state: 'no-git-repo' } : { state: 'failed', note: `${head}; ${why}` };
|
||||
} catch (e) {
|
||||
return { state: 'failed', note: `${head}; the repository check threw: ${e.message}` };
|
||||
}
|
||||
}
|
||||
const how = r.status === null ? `signal ${r.signal}` : `exit ${r.status}`;
|
||||
return { state: 'failed', note: `git check-ignore ${how}${err ? `: ${err}` : ''}` };
|
||||
}
|
||||
|
||||
/**
|
||||
* After a check-ignore exit 128: null when `repo` is no git repository, else
|
||||
* the reason it may be one. `git rev-parse --git-dir` exit 0 proves a
|
||||
* repository; its exit 128 alone proves nothing (dubious ownership, a bad
|
||||
* `.git/config` and a garbage `.git/HEAD` all exit 128 there), so then the
|
||||
* answer is null only when GIT_DIR is unset and no `.git` entry (file or
|
||||
* directory) is found by gitEntryDir. A spawn error or any other exit is a
|
||||
* reason too. May throw (realpath of a vanished repo); the caller reads that
|
||||
* as a failed check.
|
||||
* @returns {string | null}
|
||||
*/
|
||||
function notARepoReason(repo) {
|
||||
const p = spawnSync('git', ['-C', repo, 'rev-parse', '--git-dir'], { encoding: 'utf-8', stdio: ['ignore', 'ignore', 'ignore'] });
|
||||
if (p.error) return `git rev-parse --git-dir could not start: ${p.error.message}`;
|
||||
if (p.status !== 128) return `git rev-parse --git-dir ${p.status === null ? `signal ${p.signal}` : `exit ${p.status}`}`;
|
||||
if (process.env.GIT_DIR !== undefined) return 'GIT_DIR is set';
|
||||
const found = gitEntryDir(repo);
|
||||
return found === null ? null : `a .git entry exists in ${found}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The directory holding a `.git` entry, walking up from `realpath(repo)`, or
|
||||
* null. `realpath(repo)` itself is always checked; the walk stops before the
|
||||
* nearest proper ancestor that equals a GIT_CEILING_DIRECTORIES entry (split
|
||||
* on the path delimiter, empty entries skipped, each entry realpath'd, or
|
||||
* resolved when that throws), and without one it goes up to and including
|
||||
* `/`. An entry equal to `realpath(repo)` stops nothing, as in git. Unlike
|
||||
* git, the walk does not stop at a filesystem boundary: crossing one can only
|
||||
* find a `.git` git would not, which reads as a failed check (the safe side).
|
||||
* @returns {string | null}
|
||||
*/
|
||||
function gitEntryDir(repo) {
|
||||
const ceilings = new Set(String(process.env.GIT_CEILING_DIRECTORIES ?? '')
|
||||
.split(delimiter)
|
||||
.filter((e) => e !== '')
|
||||
.map((e) => { try { return realpathSync(e); } catch { return resolve(e); } }));
|
||||
let dir = realpathSync(repo);
|
||||
for (;;) {
|
||||
if (existsSync(join(dir, '.git'))) return dir;
|
||||
const up = dirname(dir);
|
||||
if (up === dir || ceilings.has(up)) return null;
|
||||
dir = up;
|
||||
}
|
||||
}
|
||||
|
||||
export function withPointer(claudeText) {
|
||||
if (claudeText.includes(POINTER_START) || claudeText.includes(POINTER_END)) return null;
|
||||
const sep = claudeText.endsWith('\n') ? '\n' : '\n\n';
|
||||
|
|
@ -357,7 +417,11 @@ export function withPointer(claudeText) {
|
|||
* The result's `pointer` (when written): 'written' | 'present' | 'no-claude-md' |
|
||||
* 'symlink-skipped' (CLAUDE.md is a symlink, DJ4) | 'ignored-skipped' (git
|
||||
* ignores the lessons file, PTR) | 'check-failed' (git check-ignore did not
|
||||
* answer; `pointer_note` says how). Nothing is written through a symlinked
|
||||
* answer, or answered exit 128 for a directory that may be a repository: a
|
||||
* path beyond a symbolic link, a broken repository, GIT_DIR set, or a `.git`
|
||||
* entry found; `pointer_note` says how, and always names check-ignore). Only
|
||||
* a directory that is truly no repository gets the pointer without the check.
|
||||
* Nothing is written through a symlinked
|
||||
* lessons file or a lessons directory outside the repo (R3): written false.
|
||||
* @returns {{ written: boolean, reason: string, path?: string, pointer?: string, pointer_note?: string }}
|
||||
*/
|
||||
|
|
|
|||
245
tests/lib/review-recurrence-check-ignore.test.mjs
Normal file
245
tests/lib/review-recurrence-check-ignore.test.mjs
Normal file
|
|
@ -0,0 +1,245 @@
|
|||
// tests/lib/review-recurrence-check-ignore.test.mjs
|
||||
// CKI (fix round 4.5b, part 2). `git check-ignore` exits 128 for "not a git
|
||||
// repository" AND for other failures inside a repository (a path beyond a
|
||||
// symbolic link, a broken .git/HEAD, a GIT_DIR that does not exist). The
|
||||
// lessons pointer in CLAUDE.md may only be written when the directory is truly
|
||||
// no repository; every other 128 is a failed check (`check-failed`, no
|
||||
// pointer). The answer must not depend on the language git speaks.
|
||||
//
|
||||
// Everything runs through the CLI as a child process, so this file loads at
|
||||
// the base. The git environment is fixed here: an empty global config, no
|
||||
// system config, a ceiling at the sandbox and the C locale, so a global
|
||||
// core.excludesFile, the temp directory's location or a translated git cannot
|
||||
// change the result. Every fixture lives under the one sandbox this test makes.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import {
|
||||
mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync, symlinkSync, realpathSync, accessSync, statSync, constants,
|
||||
} from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join, dirname, delimiter } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const CLI = join(ROOT, 'lib', 'review', 'recurrence.mjs');
|
||||
const POINTER_START = '<!-- voyage:review-lessons-pointer:start -->';
|
||||
const CLAUDE_TEXT = '# Project\n';
|
||||
|
||||
// ---- review fixtures (copied from tests/lib/review-recurrence.test.mjs:23-84) ----
|
||||
|
||||
function reviewText({ slug, sha, findings }) {
|
||||
const json = JSON.stringify({
|
||||
verdict: 'WARN',
|
||||
counts: { BLOCKER: 0, MAJOR: findings.length, MINOR: 0, SUGGESTION: 0 },
|
||||
findings,
|
||||
}, null, 2);
|
||||
return `---
|
||||
type: trekreview
|
||||
review_version: "1.1"
|
||||
task: "${slug}"
|
||||
slug: ${slug}
|
||||
project_dir: .claude/projects/2026-10-0${sha.length % 9}-${slug}/
|
||||
brief_path: .claude/projects/x/brief.md
|
||||
scope_sha_end: ${sha}
|
||||
reviewed_files_count: 1
|
||||
verdict: WARN
|
||||
findings:
|
||||
${findings.map((f) => ` - ${f.id}`).join('\n')}
|
||||
---
|
||||
|
||||
# Review: ${slug}
|
||||
|
||||
## Executive Summary
|
||||
|
||||
x
|
||||
|
||||
## Coverage
|
||||
|
||||
x
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
x
|
||||
|
||||
\`\`\`json
|
||||
${json}
|
||||
\`\`\`
|
||||
`;
|
||||
}
|
||||
|
||||
function finding(id, rule_key, extra = {}) {
|
||||
return {
|
||||
id: id.padEnd(40, '0'), severity: 'MAJOR', rule_key, file: 'lib/a.mjs', line: 3,
|
||||
title: `${rule_key} title ${id}`, detail: 'd', recommended_action: `Fix it (${id}).`, ...extra,
|
||||
};
|
||||
}
|
||||
|
||||
/** The two recurring MISSING_TEST reviews written as `<claudeDir>/projects/...`. */
|
||||
function reviewsInto(claudeDir) {
|
||||
const reviews = [
|
||||
{ slug: 'one', sha: 'aaa', findings: [finding('a1', 'MISSING_TEST')] },
|
||||
{ slug: 'two', sha: 'bbb', findings: [finding('b1', 'MISSING_TEST')] },
|
||||
];
|
||||
reviews.forEach((r, i) => {
|
||||
const p = join(claudeDir, 'projects', `2026-10-0${i + 1}-${r.slug}`);
|
||||
mkdirSync(p, { recursive: true });
|
||||
writeFileSync(join(p, 'review.md'), reviewText(r));
|
||||
});
|
||||
}
|
||||
|
||||
// ---- the sandbox and its git environment ----
|
||||
|
||||
/** Where the current PATH finds `name`, as an absolute path. */
|
||||
function onPath(name) {
|
||||
for (const dir of String(process.env.PATH ?? '').split(delimiter).filter(Boolean)) {
|
||||
const p = join(dir, name);
|
||||
try {
|
||||
accessSync(p, constants.X_OK);
|
||||
if (statSync(p).isFile()) return p;
|
||||
} catch { /* not here */ }
|
||||
}
|
||||
throw new Error(`${name} is not on PATH`);
|
||||
}
|
||||
|
||||
function makeSandbox() {
|
||||
const base = realpathSync(mkdtempSync(join(tmpdir(), 'recurrence-cki-')));
|
||||
const gitConfig = join(base, 'empty-gitconfig');
|
||||
writeFileSync(gitConfig, '');
|
||||
const home = join(base, 'home');
|
||||
mkdirSync(home);
|
||||
const realGit = onPath('git'); // resolved before any stub dir is put first on PATH
|
||||
const env = (over = {}) => childEnv({
|
||||
PATH: process.env.PATH ?? '',
|
||||
HOME: home,
|
||||
XDG_CONFIG_HOME: home,
|
||||
GIT_CONFIG_GLOBAL: gitConfig,
|
||||
GIT_CONFIG_NOSYSTEM: '1',
|
||||
GIT_CEILING_DIRECTORIES: base,
|
||||
LC_ALL: 'C',
|
||||
LANG: 'C',
|
||||
LANGUAGE: undefined,
|
||||
GIT_DIR: undefined,
|
||||
GIT_WORK_TREE: undefined,
|
||||
...over,
|
||||
});
|
||||
const git = (args) => {
|
||||
const r = spawnSync('git', args, { encoding: 'utf-8', env: env() });
|
||||
assert.equal(r.status, 0, `git ${args.join(' ')}: ${r.stderr}`);
|
||||
};
|
||||
/** A directory under the sandbox holding the two reviews in `.claude` and a plain CLAUDE.md. */
|
||||
const plainDir = (name) => {
|
||||
const dir = join(base, name);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
reviewsInto(join(dir, '.claude'));
|
||||
writeFileSync(join(dir, 'CLAUDE.md'), CLAUDE_TEXT);
|
||||
return dir;
|
||||
};
|
||||
const repo = (name) => {
|
||||
const dir = plainDir(name);
|
||||
git(['-C', dir, 'init', '-q']);
|
||||
return dir;
|
||||
};
|
||||
/** A PATH whose first `git` fails only `check-ignore` (exit 128, `message`) and execs the real git otherwise. */
|
||||
const stubPath = (name, message) => {
|
||||
const bin = join(base, name);
|
||||
mkdirSync(bin);
|
||||
if (realGit.includes("'") || message.includes("'")) throw new Error('a quote in the stub text');
|
||||
writeFileSync(join(bin, 'git'),
|
||||
`#!/bin/sh\nfor a in "$@"; do\n if [ "$a" = "check-ignore" ]; then\n echo '${message}' >&2\n exit 128\n fi\ndone\nexec '${realGit}' "$@"\n`,
|
||||
{ mode: 0o755 });
|
||||
return `${bin}${delimiter}${process.env.PATH ?? ''}`;
|
||||
};
|
||||
return { base, env, git, plainDir, repo, stubPath, cleanup: () => rmSync(base, { recursive: true, force: true }) };
|
||||
}
|
||||
|
||||
function apply(sb, dir, over = {}) {
|
||||
const r = spawnSync(process.execPath, [CLI, 'apply', '--repo', dir, '--rule-key', 'MISSING_TEST'], {
|
||||
encoding: 'utf-8', env: sb.env(over),
|
||||
});
|
||||
assert.equal(r.status, 0, `the lesson itself is written (exit 0): ${r.stdout}\n${r.stderr}`);
|
||||
const out = JSON.parse(r.stdout);
|
||||
assert.equal(out.written, true, r.stdout);
|
||||
return out;
|
||||
}
|
||||
|
||||
function assertNoPointer(out, dir, label) {
|
||||
assert.equal(out.pointer, 'check-failed', `${label}: ${JSON.stringify(out)}`);
|
||||
assert.equal(typeof out.pointer_note, 'string', `${label}: check-failed carries a note: ${JSON.stringify(out)}`);
|
||||
assert.match(out.pointer_note, /check-ignore/, `${label}: ${JSON.stringify(out)}`);
|
||||
assert.equal(readFileSync(join(dir, 'CLAUDE.md'), 'utf-8'), CLAUDE_TEXT, `${label}: CLAUDE.md is byte-unchanged`);
|
||||
}
|
||||
|
||||
function assertPointer(out, dir, label) {
|
||||
assert.equal(out.pointer, 'written', `${label}: ${JSON.stringify(out)}`);
|
||||
assert.ok(readFileSync(join(dir, 'CLAUDE.md'), 'utf-8').includes(POINTER_START), `${label}: CLAUDE.md holds the pointer`);
|
||||
}
|
||||
|
||||
const ENGLISH_NOT_A_REPO = 'fatal: not a git repository (or any of the parent directories): .git';
|
||||
// Simulates a translated git: the same "not a git repository" message in German (ASCII only).
|
||||
const GERMAN_NOT_A_REPO = 'fatal: Kein Git-Repository (oder irgendeines der Elternverzeichnisse): .git';
|
||||
|
||||
test('[CKI] a git check-ignore exit 128 inside a repository writes no pointer, in any language', () => {
|
||||
const sb = makeSandbox();
|
||||
try {
|
||||
const ceilingMix = ['', join(sb.base, 'no-such-ceiling'), sb.base].join(delimiter);
|
||||
|
||||
// 1. Red: `.claude` is a relative symlink to an in-repo directory; check-ignore
|
||||
// exits 128 "beyond a symbolic link" inside a real repository.
|
||||
const linked = join(sb.base, 'linked');
|
||||
mkdirSync(linked);
|
||||
sb.git(['-C', linked, 'init', '-q']);
|
||||
reviewsInto(join(linked, 'claude-data'));
|
||||
symlinkSync('claude-data', join(linked, '.claude'));
|
||||
writeFileSync(join(linked, 'CLAUDE.md'), CLAUDE_TEXT);
|
||||
assertNoPointer(apply(sb, linked), linked, 'case 1 (symlinked .claude)');
|
||||
|
||||
// 2. Red: a real repository, and a git whose check-ignore alone fakes the
|
||||
// English "not a git repository" with exit 128.
|
||||
const faked = sb.repo('faked');
|
||||
assertNoPointer(apply(sb, faked, { PATH: sb.stubPath('stub-en', ENGLISH_NOT_A_REPO) }), faked,
|
||||
'case 2 (stub check-ignore 128 in a real repo)');
|
||||
|
||||
// 3. Red: a repository whose .git/HEAD holds garbage; check-ignore and
|
||||
// rev-parse both exit 128, so only the .git-entry walk can tell.
|
||||
const broken = sb.repo('broken');
|
||||
writeFileSync(join(broken, '.git', 'HEAD'), 'garbage\n');
|
||||
assertNoPointer(apply(sb, broken), broken, 'case 3 (garbage .git/HEAD)');
|
||||
|
||||
// 4. Control: a plain directory under the ceiling (no repository) gets its
|
||||
// pointer; the ceiling list holds an empty entry and a missing path too.
|
||||
const plain = sb.plainDir('plain');
|
||||
assertPointer(apply(sb, plain, { GIT_CEILING_DIRECTORIES: ceilingMix }), plain, 'case 4 (plain directory)');
|
||||
|
||||
// 5. Control: as case 4, with a git that says "not a git repository" in another language.
|
||||
const plainDe = sb.plainDir('plain-de');
|
||||
assertPointer(apply(sb, plainDe, {
|
||||
GIT_CEILING_DIRECTORIES: ceilingMix, PATH: sb.stubPath('stub-de', GERMAN_NOT_A_REPO),
|
||||
}), plainDe, 'case 5 (translated git, plain directory)');
|
||||
|
||||
// 6. Control: the target is a plain directory inside a repository that sits
|
||||
// at the ceiling; git does not look into the ceiling, nor may the walk.
|
||||
const outer = sb.repo('outer');
|
||||
const inner = sb.plainDir(join('outer', 'inner'));
|
||||
assertPointer(apply(sb, inner, { GIT_CEILING_DIRECTORIES: realpathSync(outer) }), inner,
|
||||
'case 6 (repository at the ceiling)');
|
||||
|
||||
// 7. Red: as case 4, with GIT_DIR set to a path that does not exist.
|
||||
const plainGitDir = sb.plainDir('plain-git-dir');
|
||||
const out7 = apply(sb, plainGitDir, {
|
||||
GIT_CEILING_DIRECTORIES: ceilingMix, GIT_DIR: join(sb.base, 'no-such-git-dir'),
|
||||
});
|
||||
assertNoPointer(out7, plainGitDir, 'case 7 (GIT_DIR set)');
|
||||
assert.match(out7.pointer_note, /GIT_DIR is set/, JSON.stringify(out7));
|
||||
|
||||
// 8. Red: case 3's garbage-HEAD repository with a ceiling entry equal to the
|
||||
// repository itself (it stops nothing) plus the sandbox.
|
||||
const brokenSelf = sb.repo('broken-self');
|
||||
writeFileSync(join(brokenSelf, '.git', 'HEAD'), 'garbage\n');
|
||||
assertNoPointer(apply(sb, brokenSelf, {
|
||||
GIT_CEILING_DIRECTORIES: [realpathSync(brokenSelf), sb.base].join(delimiter),
|
||||
}), brokenSelf, 'case 8 (ceiling equal to the repository)');
|
||||
} finally { sb.cleanup(); }
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue