fix(park): read HEAD membership once and move nothing when it cannot be read
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
5ddbcd3de6
commit
bf4736a254
4 changed files with 190 additions and 15 deletions
|
|
@ -1568,8 +1568,9 @@ Sub-step C.2 never registers it again): moved, never deleted, never over a file
|
|||
already parked. Name the `parked` paths and every `kept` path with its reason
|
||||
in the step report. Exit 1 means a file the attempt made is still in the tree
|
||||
(a header it could not read, a move that failed): stop and name it; exit 2 is
|
||||
a stop too (the tree was not checked). Then launch a fresh `build` helper for
|
||||
the revised step.
|
||||
a stop too, nothing unindexed or moved (HEAD could not be read, a path in HEAD
|
||||
is missing from the index, or another git call failed). Then launch a fresh
|
||||
`build` helper for the revised step.
|
||||
|
||||
- **Never pass the Agent tool's `name` parameter.** A named spawn becomes an
|
||||
`in_process_teammate`: it does the work, but its result never reaches this
|
||||
|
|
|
|||
|
|
@ -15,7 +15,14 @@
|
|||
// Moved, never deleted, and never over a file already parked there.
|
||||
// A new file the helper registered --intent-to-add is in the index, not in
|
||||
// HEAD: it is taken out of the index (`git rm --cached`) and parked like any
|
||||
// other; a file committed since the attempt (in HEAD) is never touched. The
|
||||
// other; a file committed since the attempt (in HEAD) is never touched. HEAD
|
||||
// is read once, before anything changes (`git rev-parse --verify --quiet HEAD`,
|
||||
// then `git ls-tree --full-tree -r -z --name-only HEAD`; an unborn HEAD holds
|
||||
// nothing). HEAD counts as unborn only when it names a branch that is stored
|
||||
// nowhere (no loose ref file, no packed-refs line): a corrupt branch ref looks
|
||||
// the same to `rev-parse` but exists, and is a failed HEAD read (exit 2, nothing
|
||||
// unindexed or moved). `ls-tree` reads tree entries, not blobs: a HEAD path whose blob is
|
||||
// missing from the object store is still in HEAD, and kept. The
|
||||
// paths are read from the repository root (`git rev-parse --show-toplevel`),
|
||||
// whatever --cwd inside it is given, and colour codes in the diff are ignored.
|
||||
//
|
||||
|
|
@ -25,9 +32,10 @@
|
|||
// Exit: 0 = no file the attempt made is left in the tree; 1 = one still is
|
||||
// (a header it could not read, a non-empty diff with no readable header, a
|
||||
// move that failed, a target already taken): the run stops; 2 = could not run
|
||||
// (bad usage, unreadable diff, not a repository, a git call that failed, an
|
||||
// index entry that could not be removed; nothing is moved). Neither 1 nor 2
|
||||
// is ever "the tree is clean".
|
||||
// (bad usage, unreadable diff, not a repository, a git call that failed, the
|
||||
// HEAD read that failed (a corrupt branch ref too), a path in HEAD that is missing from the index, an
|
||||
// index entry that could not be removed; nothing is unindexed or moved).
|
||||
// Neither 1 nor 2 is ever "the tree is clean".
|
||||
|
||||
import { readFileSync, mkdirSync, renameSync, lstatSync } from 'node:fs';
|
||||
import { join, dirname, resolve, relative, isAbsolute } from 'node:path';
|
||||
|
|
@ -76,11 +84,47 @@ function git(root, args, opts = {}) {
|
|||
return r.stdout;
|
||||
}
|
||||
|
||||
/** Is `path` (root-relative) in HEAD? A repository with no commit has no HEAD: not in it. */
|
||||
function inHead(root, path) {
|
||||
const r = spawnSync('git', ['-C', root, 'cat-file', '-e', `HEAD:${path}`], { encoding: 'utf8' });
|
||||
if (r.error) throw new Error(`git cat-file -e HEAD:${path}: ${r.error.message}`);
|
||||
return r.status === 0;
|
||||
/**
|
||||
* Is HEAD unborn? Only when HEAD points at a branch ref R (`symbolic-ref -q
|
||||
* HEAD` exit 0) and R is stored nowhere: no loose ref file
|
||||
* (`rev-parse --git-path R`) and no line for R in packed-refs. A corrupt branch
|
||||
* ref (garbage in the loose file, or a bad packed line) makes
|
||||
* `rev-parse --verify --quiet HEAD` exit 1 with no output just like an unborn
|
||||
* HEAD, and so does `show-ref --verify --quiet R` (measured on git 2.50.1), so
|
||||
* the exit codes alone cannot tell them apart; where R is stored can.
|
||||
*/
|
||||
function headIsUnborn(root) {
|
||||
const sym = spawnSync('git', ['-C', root, 'symbolic-ref', '-q', 'HEAD'], { encoding: 'utf8' });
|
||||
if (sym.error || sym.status !== 0) return false;
|
||||
const ref = String(sym.stdout ?? '').trim();
|
||||
if (!/^refs\/heads\/./.test(ref)) return false;
|
||||
if (lexists(resolve(root, git(root, ['rev-parse', '--git-path', ref]).trim()))) return false;
|
||||
const packed = resolve(root, git(root, ['rev-parse', '--git-path', 'packed-refs']).trim());
|
||||
if (lexists(packed)) {
|
||||
const lines = readFileSync(packed, 'utf8').split('\n');
|
||||
if (lines.some((l) => l.endsWith(` ${ref}`))) return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Every path in HEAD (root-relative), read once before anything changes. An
|
||||
* unborn HEAD (`rev-parse --verify --quiet HEAD` exit 1 with no output, and
|
||||
* headIsUnborn: the branch HEAD names is stored nowhere) is an empty set; a
|
||||
* branch ref that exists but cannot be read, and any other failure of either
|
||||
* call, throws (exit 2). `ls-tree` reads tree entries, so a path whose blob is
|
||||
* missing from the object store is still "in HEAD".
|
||||
*/
|
||||
function headPaths(root) {
|
||||
const args = ['rev-parse', '--verify', '--quiet', 'HEAD'];
|
||||
const r = spawnSync('git', ['-C', root, ...args], { encoding: 'utf8' });
|
||||
if (r.error) throw new Error(`git ${args.join(' ')}: ${r.error.message}`);
|
||||
if (r.status === 1 && String(r.stdout ?? '').trim() === '') {
|
||||
if (headIsUnborn(root)) return new Set();
|
||||
throw new Error(`git ${args.join(' ')} exited 1, and the branch HEAD names exists: a corrupt or unreadable HEAD ref`);
|
||||
}
|
||||
if (r.status !== 0) throw new Error(`git ${args.join(' ')} exited ${r.status}: ${String(r.stderr ?? '').trim().split('\n')[0]}`);
|
||||
return new Set(git(root, ['ls-tree', '--full-tree', '-r', '-z', '--name-only', 'HEAD']).split('\0').filter(Boolean));
|
||||
}
|
||||
|
||||
function lexists(abs) {
|
||||
|
|
@ -101,11 +145,13 @@ export function parkAttemptFiles({ diffText, step, attempt, cwd = process.cwd()
|
|||
const gitDir = git(root, ['rev-parse', '--absolute-git-dir']).trim();
|
||||
const to = join(gitDir, 'voyage', 'parked', `step-${step}-attempt-${attempt}`);
|
||||
const trackedPaths = new Set(git(root, ['ls-files', '-z']).split('\0').filter(Boolean));
|
||||
const inHead = headPaths(root);
|
||||
const { paths, unread } = newFilesInDiff(diffText);
|
||||
const parked = [];
|
||||
const kept = unread.map((h) => ({ path: h, reason: 'diff header not read: move this file out by hand', blocking: true }));
|
||||
const moves = [];
|
||||
const unindex = [];
|
||||
const headNotIndexed = [];
|
||||
for (const path of paths) {
|
||||
const abs = resolve(root, path);
|
||||
const rel = relative(root, abs);
|
||||
|
|
@ -118,8 +164,13 @@ export function parkAttemptFiles({ diffText, step, attempt, cwd = process.cwd()
|
|||
continue;
|
||||
}
|
||||
const posix = rel.split('\\').join('/');
|
||||
if (trackedPaths.has(posix) && inHead(root, posix)) {
|
||||
kept.push({ path, reason: 'tracked since the attempt: not the executor\'s to move', blocking: false });
|
||||
if (inHead.has(posix)) {
|
||||
if (trackedPaths.has(posix)) {
|
||||
kept.push({ path, reason: 'tracked since the attempt: not the executor\'s to move', blocking: false });
|
||||
} else {
|
||||
// Committed, then taken out of the index: not the attempt's to move.
|
||||
headNotIndexed.push(posix);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const target = join(to, rel);
|
||||
|
|
@ -131,6 +182,10 @@ export function parkAttemptFiles({ diffText, step, attempt, cwd = process.cwd()
|
|||
// In the index but not in HEAD: registered --intent-to-add by the attempt.
|
||||
if (trackedPaths.has(posix)) unindex.push(posix);
|
||||
}
|
||||
// A path in HEAD that is missing from the index: nothing is unindexed or moved.
|
||||
if (headNotIndexed.length > 0) {
|
||||
throw new Error(`in HEAD but missing from the index, nothing moved: ${headNotIndexed.join(', ')}`);
|
||||
}
|
||||
// Out of the index first, all in ONE call before anything moves: git
|
||||
// refuses the whole call when one path cannot be removed, so a failure
|
||||
// throws (exit 2) with every entry still in the index and nothing moved.
|
||||
|
|
|
|||
2
tests/fixtures/jsonl-schemas.md
vendored
2
tests/fixtures/jsonl-schemas.md
vendored
|
|
@ -22,7 +22,7 @@
|
|||
| trekbrief-stats | ts, task, slug, mode, interview_turns, review_iterations, brief_quality, research_topics, auto_research, auto_result, intent_approved, project_dir, order_id, order_claimed_at, order_reason | commands/trekbrief.md (orchestrator-emit Phase 7; order_* added by lib/stats/stats-append.mjs) | trekbrief.md:1141-1165 | profile, phase_models, profile_source | none |
|
||||
| trekresearch-stats | ts, question, mode, scope, engine, slug, project_dir, brief_path, dimensions, dimensions_baseline, dimensions_baseline_preserved, effort, conv_turns, empty_turns, unique_sources, agents_local, agents_external, gemini_used, confidence, contradictions, open_questions | lib/stats/stats-append.mjs (trekresearch.md Stats tracking) | trekresearch.md:822-855 | profile, phase_models, parallel_agents, external_research_enabled, profile_source | none |
|
||||
| trekplan-stats | ts, task, mode, slug, brief_path, project_dir, codebase_size, codebase_files, agents_deployed, deep_dives, research_briefs_used, research_scout_used, critic_verdict, guardian_verdict, outcome | lib/stats/stats-append.mjs (trekplan.md Phase 12) | trekplan.md:1307-1335 | profile, phase_models, parallel_agents, profile_source | none |
|
||||
| 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:2972-2994 | profile, phase_models, profile_source | none |
|
||||
| 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:2973-2995 | 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:640-690 | profile, phase_models, profile_source | none |
|
||||
|
|
|
|||
|
|
@ -6,10 +6,11 @@
|
|||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync, symlinkSync, cpSync } from 'node:fs';
|
||||
import { join, dirname } from 'node:path';
|
||||
import { join, dirname, delimiter } from 'node:path';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const CLI = join(ROOT, 'lib/util/park-attempt-files.mjs');
|
||||
|
|
@ -369,3 +370,121 @@ test('[F4a] one index entry that cannot be removed exits 2 with every entry stil
|
|||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// --- fix round 4-5b: M7 ----------------------------------------------------------
|
||||
// [M7] HEAD membership was one `git cat-file -e HEAD:<path>` per path, and any
|
||||
// failure of it read as "not in HEAD": a git that could not read HEAD unindexed
|
||||
// and moved the attempt's files. A path in HEAD but gone from the index was
|
||||
// moved like an untracked file. Both now exit 2 before anything changes.
|
||||
|
||||
test('[M7] a failing HEAD read exits 2 and moves nothing', async () => {
|
||||
const { pathWith } = await import('../helpers/bin-path.mjs');
|
||||
const real = pathWith(['git']);
|
||||
const fake = mkdtempSync(join(tmpdir(), 'voyage-fake-git-'));
|
||||
const dir = repo();
|
||||
try {
|
||||
const realGit = join(real.path, 'git');
|
||||
assert.ok(!realGit.includes("'"), 'control: the real git path carries no quote');
|
||||
writeFileSync(join(fake, 'git'), [
|
||||
'#!/bin/sh',
|
||||
'for a in "$@"; do',
|
||||
' if [ "$a" = ls-tree ]; then echo "fatal: ls-tree failed (fake git)" >&2; exit 128; fi',
|
||||
'done',
|
||||
`exec '${realGit}' "$@"`,
|
||||
'',
|
||||
].join('\n'), { mode: 0o755 });
|
||||
attemptFiles(dir);
|
||||
git(dir, 'add', '--intent-to-add', '--', 'tests/batch.sh', 'tests/cli-stdin.test.mjs');
|
||||
const env = childEnv({ PATH: `${fake}${delimiter}${process.env.PATH ?? ''}` });
|
||||
const ctl = spawnSync('git', ['ls-tree', 'HEAD'], { cwd: dir, encoding: 'utf8', env });
|
||||
assert.equal(ctl.status, 128, `control: the fake git must fail ls-tree: ${ctl.stderr}`);
|
||||
assert.equal(spawnSync('git', ['rev-parse', 'HEAD'], { cwd: dir, encoding: 'utf8', env }).status, 0, 'control: the fake git runs every other call');
|
||||
writeFileSync(join(dir, '.git', 'upward.diff'), DIFF);
|
||||
const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, '.git', 'upward.diff'), '--step', '1', '--attempt', '1', '--cwd', dir], { encoding: 'utf8', env });
|
||||
assert.equal(r.status, 2, `a HEAD read that failed must be exit 2: ${r.stdout}${r.stderr}`);
|
||||
assert.match(r.stderr, /ls-tree/, 'the message does not name the git call that failed');
|
||||
const listed = git(dir, 'ls-files').split('\n');
|
||||
assert.ok(listed.includes('tests/batch.sh'), 'the intent-to-add entry was taken out of the index although HEAD could not be read');
|
||||
assert.ok(listed.includes('tests/cli-stdin.test.mjs'), 'the second intent-to-add entry was taken out of the index');
|
||||
assert.equal(readFileSync(join(dir, 'tests/batch.sh'), 'utf8'), 'stale | cat\n', 'the file was moved although HEAD could not be read');
|
||||
assert.equal(existsSync(join(dir, 'tests/cli-stdin.test.mjs')), true, 'the second file was moved');
|
||||
assert.equal(existsSync(parkedPath(dir, 'tests/batch.sh')), false, 'a file was parked although HEAD could not be read');
|
||||
} finally {
|
||||
real.cleanup();
|
||||
rmSync(fake, { recursive: true, force: true });
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('[M7] a HEAD path missing from the index exits 2 and moves nothing', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
attemptFiles(dir);
|
||||
git(dir, 'add', '--', 'tests/batch.sh');
|
||||
git(dir, 'commit', '-q', '-m', 'operator');
|
||||
git(dir, 'rm', '--cached', '-q', '--', 'tests/batch.sh');
|
||||
assert.doesNotMatch(git(dir, 'ls-files'), /tests\/batch\.sh/, 'control: the committed file is out of the index');
|
||||
assert.match(git(dir, 'ls-tree', '-r', '--name-only', 'HEAD'), /tests\/batch\.sh/, 'control: the file is in HEAD');
|
||||
const r = run(dir);
|
||||
assert.equal(r.status, 2, `a HEAD path missing from the index must be exit 2: ${r.stdout}${r.stderr}`);
|
||||
assert.match(r.stderr, /tests\/batch\.sh/, 'the message does not name the path');
|
||||
assert.equal(readFileSync(join(dir, 'tests/batch.sh'), 'utf8'), 'stale | cat\n', 'the committed file was moved');
|
||||
assert.equal(existsSync(join(dir, 'tests/cli-stdin.test.mjs')), true, 'the untracked file was moved although the run could not finish');
|
||||
assert.equal(existsSync(parkedPath(dir, 'tests/batch.sh')), false, 'the committed file was parked');
|
||||
assert.equal(existsSync(parkedPath(dir, 'tests/cli-stdin.test.mjs')), false, 'a file was parked although the run could not finish');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
/** The CLI run on `diff` written into the repo's git dir, its child given childEnv(). */
|
||||
function runIsolated(dir, diff = DIFF) {
|
||||
writeFileSync(join(dir, '.git', 'upward.diff'), diff);
|
||||
return spawnSync(process.execPath, [CLI, '--diff', join(dir, '.git', 'upward.diff'), '--step', '1', '--attempt', '1', '--cwd', dir], { encoding: 'utf8', env: childEnv() });
|
||||
}
|
||||
|
||||
test('[M7] a corrupt branch ref is a failed HEAD read: exit 2, nothing unindexed or moved', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
attemptFiles(dir);
|
||||
git(dir, 'add', '--', 'tests/batch.sh');
|
||||
git(dir, 'commit', '-q', '-m', 'operator');
|
||||
const ref = git(dir, 'symbolic-ref', '-q', 'HEAD').trim();
|
||||
writeFileSync(join(dir, '.git', ref), 'garbage\n');
|
||||
const ctl = spawnSync('git', ['rev-parse', '--verify', '--quiet', 'HEAD'], { cwd: dir, encoding: 'utf8', env: childEnv() });
|
||||
assert.equal(ctl.status, 1, `control: the corrupt ref reads like an unborn HEAD (exit 1): ${ctl.stderr}`);
|
||||
assert.equal(ctl.stdout.trim(), '', 'control: and prints nothing');
|
||||
const r = runIsolated(dir);
|
||||
assert.equal(r.status, 2, `a corrupt branch ref must be exit 2: ${r.stdout}${r.stderr}`);
|
||||
const listed = git(dir, 'ls-files').split('\n');
|
||||
assert.ok(listed.includes('tests/batch.sh'), 'the committed file was taken out of the index although HEAD could not be read');
|
||||
assert.equal(readFileSync(join(dir, 'tests/batch.sh'), 'utf8'), 'stale | cat\n', 'the committed file was moved');
|
||||
assert.equal(existsSync(join(dir, 'tests/cli-stdin.test.mjs')), true, 'the untracked file was moved although HEAD could not be read');
|
||||
assert.equal(existsSync(parkedPath(dir, 'tests/batch.sh')), false, 'the committed file was parked');
|
||||
assert.equal(existsSync(parkedPath(dir, 'tests/cli-stdin.test.mjs')), false, 'a file was parked although HEAD could not be read');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('[M7] an unborn HEAD parks the attempt\'s files', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'voyage-park-unborn-'));
|
||||
try {
|
||||
git(dir, 'init', '-q');
|
||||
attemptFiles(dir);
|
||||
git(dir, 'add', '--intent-to-add', '--', 'tests/batch.sh');
|
||||
assert.match(git(dir, 'ls-files'), /tests\/batch\.sh/, 'control: the intent-to-add file is in the index');
|
||||
const ctl = spawnSync('git', ['rev-parse', '--verify', '--quiet', 'HEAD'], { cwd: dir, encoding: 'utf8', env: childEnv() });
|
||||
assert.equal(ctl.status, 1, 'control: HEAD is unborn');
|
||||
const r = runIsolated(dir);
|
||||
assert.equal(r.status, 0, `an unborn HEAD must park: ${r.stdout}${r.stderr}`);
|
||||
assert.deepEqual(JSON.parse(r.stdout).parked, ['tests/batch.sh', 'tests/cli-stdin.test.mjs']);
|
||||
assert.doesNotMatch(git(dir, 'ls-files'), /tests\/batch\.sh/, 'the intent-to-add entry is still in the index');
|
||||
assert.equal(existsSync(join(dir, 'tests/batch.sh')), false, 'the intent-to-add file is still in the work tree');
|
||||
assert.equal(existsSync(join(dir, 'tests/cli-stdin.test.mjs')), false, 'the untracked file is still in the work tree');
|
||||
assert.equal(readFileSync(parkedPath(dir, 'tests/batch.sh'), 'utf8'), 'stale | cat\n', 'the file is not under the parked directory');
|
||||
assert.equal(readFileSync(parkedPath(dir, 'tests/cli-stdin.test.mjs'), 'utf8'), 'stale test\n');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue