fix(trekexecute): park a re-planned step's earlier new files before the fresh build
The revert of a step sent back to the plan leaves its new files on disk, and --resume built the revised step on top of them: a stale tests/batch.sh holding the rejected line would have passed Verify, the manifest and the success criterion. lib/util/park-attempt-files.mjs moves every still-untracked file the attempt created (from step-N-upward.diff) into the report dir, never deleting it; /trekexecute runs it before the fresh build helper (voyage-lekerepo trial 5, finding 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
b47cabdf30
commit
3238327483
4 changed files with 267 additions and 4 deletions
|
|
@ -1522,8 +1522,18 @@ and the skill to work by: `skill: test-first` for a build round,
|
|||
|
||||
**A step at `fix_rounds` 5 is never built again** (Sub-step F): skip the
|
||||
launch and run only the checks (B, D1, D2, D3, D4, E) on whatever the operator
|
||||
changed. A step at `fix_rounds` 4 on `--resume` was re-planned: launch a fresh
|
||||
`build` helper for the revised step.
|
||||
changed. A step at `fix_rounds` 4 on `--resume` was re-planned: first park
|
||||
the earlier attempt's new files, which the revert left on disk untracked, so a
|
||||
stale file can never pass the revised step's checks (voyage-lekerepo trial 5,
|
||||
finding 4):
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/util/park-attempt-files.mjs --diff {report dir}/step-{N}-upward.diff --to {report dir}/step-{N}-attempt-{attempts}
|
||||
```
|
||||
It moves every file the attempt created that is still untracked into that
|
||||
directory under its own path: moved, never deleted. Name the `parked` paths
|
||||
and every `kept` path with its reason in the step report; exit 2 is a stop
|
||||
(the tree was not checked). 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
|
||||
|
|
@ -1862,7 +1872,9 @@ directly (Sub-step B), and so does a second `plan-unreadable` after one correcte
|
|||
git rm --cached -q -- {the step's new files: registered with --intent-to-add since start_sha}
|
||||
```
|
||||
The new files stay on disk, untracked, and `step-{N}-upward.md` names them —
|
||||
nothing the step made is deleted, and none of it is left registered.
|
||||
nothing the step made is deleted, and none of it is left registered. A
|
||||
`--resume` after re-planning parks them in the report dir before the fresh
|
||||
build (Sub-step C).
|
||||
When the guard refuses the restore (it is destructive git: in the permission
|
||||
mode that shows no prompts the guard refuses it, exit 2, "no one to ask"), or
|
||||
the operator declines it, nothing was restored: write in `step-{N}-upward.md`
|
||||
|
|
|
|||
114
lib/util/park-attempt-files.mjs
Normal file
114
lib/util/park-attempt-files.mjs
Normal file
|
|
@ -0,0 +1,114 @@
|
|||
#!/usr/bin/env node
|
||||
// lib/util/park-attempt-files.mjs
|
||||
// A step sent back to the plan (`fix_rounds` 4) is reverted with its new files
|
||||
// left on disk, untracked: nothing the step made is deleted. On --resume the
|
||||
// re-planned step is built fresh, but the stale files were still there, and in
|
||||
// voyage-lekerepo trial 5 (finding 4) an old tests/batch.sh holding the
|
||||
// rejected line would have passed Verify, the manifest and the success
|
||||
// criterion. Only the plan critic caught it.
|
||||
//
|
||||
// Before the fresh build the executor parks them: every file the attempt
|
||||
// created (a `new file mode` entry in step-{N}-upward.diff) that is still
|
||||
// untracked on disk moves into the report dir under its own path. Moved,
|
||||
// never deleted. A file tracked since then, already gone, or outside the
|
||||
// repository is kept where it is and named.
|
||||
//
|
||||
// Usage:
|
||||
// node lib/util/park-attempt-files.mjs --diff <step-N-upward.diff> --to <dir> [--cwd <repo>]
|
||||
// Prints {"parked": [paths], "kept": [{"path", "reason"}]} on stdout.
|
||||
// Exit: 0 = done (also when nothing was parked), 2 = could not run (bad usage,
|
||||
// unreadable diff, not a repository). 2 never means the tree is clean.
|
||||
|
||||
import { readFileSync, existsSync, mkdirSync, renameSync, lstatSync } from 'node:fs';
|
||||
import { join, dirname, resolve, relative, isAbsolute } from 'node:path';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { isMain } from './is-main.mjs';
|
||||
|
||||
/** The paths a unified git diff creates (`new file mode` after its `diff --git` line). */
|
||||
export function newFilesInDiff(diffText) {
|
||||
const paths = [];
|
||||
let current = null;
|
||||
for (const line of String(diffText).split('\n')) {
|
||||
const head = line.match(/^diff --git a\/(\S+) b\/(\S+)$/);
|
||||
if (head) {
|
||||
current = head[2];
|
||||
continue;
|
||||
}
|
||||
if (line.startsWith('diff --git ')) {
|
||||
current = null; // a quoted or spaced path: not read, so never moved
|
||||
continue;
|
||||
}
|
||||
if (current !== null && /^new file mode \d+$/.test(line)) {
|
||||
paths.push(current);
|
||||
current = null;
|
||||
}
|
||||
}
|
||||
return paths;
|
||||
}
|
||||
|
||||
function tracked(cwd, path) {
|
||||
return spawnSync('git', ['ls-files', '--error-unmatch', '--', path], { cwd, encoding: 'utf8' }).status === 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {{ diffText: string, to: string, cwd?: string }} args
|
||||
* @returns {{ parked: string[], kept: Array<{path: string, reason: string}> }}
|
||||
*/
|
||||
export function parkAttemptFiles({ diffText, to, cwd = process.cwd() }) {
|
||||
const root = resolve(cwd);
|
||||
const dest = resolve(root, to);
|
||||
const parked = [];
|
||||
const kept = [];
|
||||
for (const path of newFilesInDiff(diffText)) {
|
||||
const abs = resolve(root, path);
|
||||
const rel = relative(root, abs);
|
||||
if (isAbsolute(path) || rel === '' || rel.startsWith('..') || isAbsolute(rel) || rel.split(/[\\/]/)[0] === '.git') {
|
||||
kept.push({ path, reason: 'outside the repository: refused' });
|
||||
continue;
|
||||
}
|
||||
if (!existsSync(abs) || !lstatSync(abs).isFile()) {
|
||||
kept.push({ path, reason: 'not on disk as a file: nothing to park' });
|
||||
continue;
|
||||
}
|
||||
if (tracked(root, rel)) {
|
||||
kept.push({ path, reason: 'tracked since the attempt: not the executor\'s to move' });
|
||||
continue;
|
||||
}
|
||||
const target = join(dest, rel);
|
||||
try {
|
||||
mkdirSync(dirname(target), { recursive: true });
|
||||
renameSync(abs, target);
|
||||
parked.push(rel);
|
||||
} catch (e) {
|
||||
kept.push({ path, reason: `could not move: ${e.message}` });
|
||||
}
|
||||
}
|
||||
return { parked, kept };
|
||||
}
|
||||
|
||||
if (isMain(import.meta.url)) {
|
||||
const argv = process.argv.slice(2);
|
||||
const opt = (name) => {
|
||||
const i = argv.indexOf(name);
|
||||
return i === -1 ? undefined : argv[i + 1];
|
||||
};
|
||||
const diff = opt('--diff');
|
||||
const to = opt('--to');
|
||||
const cwd = opt('--cwd') ?? process.cwd();
|
||||
if (!diff || !to) {
|
||||
process.stderr.write('Usage: park-attempt-files.mjs --diff <step-N-upward.diff> --to <dir> [--cwd <repo>]\n');
|
||||
process.exit(2);
|
||||
}
|
||||
let diffText;
|
||||
try {
|
||||
diffText = readFileSync(diff, 'utf8');
|
||||
} catch (e) {
|
||||
process.stderr.write(`park-attempt-files: cannot read ${diff}: ${e.message}\n`);
|
||||
process.exit(2);
|
||||
}
|
||||
if (spawnSync('git', ['rev-parse', '--git-dir'], { cwd, encoding: 'utf8' }).status !== 0) {
|
||||
process.stderr.write(`park-attempt-files: ${cwd} is not a git repository\n`);
|
||||
process.exit(2);
|
||||
}
|
||||
process.stdout.write(JSON.stringify(parkAttemptFiles({ diffText, to, cwd }), null, 2) + '\n');
|
||||
}
|
||||
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:1127-1151 | 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:782-808 | 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:1277-1305 | 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:2892-2914 | 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:2904-2926 | 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:636-686 | profile, phase_models, profile_source | none |
|
||||
|
|
|
|||
137
tests/lib/park-attempt-files.test.mjs
Normal file
137
tests/lib/park-attempt-files.test.mjs
Normal file
|
|
@ -0,0 +1,137 @@
|
|||
// tests/lib/park-attempt-files.test.mjs
|
||||
// A step sent back to the plan leaves its new files on disk; before the fresh
|
||||
// build of the re-planned step they are moved into the report dir, never
|
||||
// deleted (voyage-lekerepo trial 5, finding 4).
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync } from 'node:fs';
|
||||
import { join, dirname } from 'node:path';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const CLI = join(ROOT, 'lib/util/park-attempt-files.mjs');
|
||||
|
||||
function git(cwd, ...args) {
|
||||
const r = spawnSync('git', args, { cwd, encoding: 'utf8' });
|
||||
assert.equal(r.status, 0, r.stderr);
|
||||
return r.stdout;
|
||||
}
|
||||
|
||||
function repo() {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'voyage-park-'));
|
||||
git(dir, 'init', '-q');
|
||||
git(dir, 'config', 'user.email', 't@example.invalid');
|
||||
git(dir, 'config', 'user.name', 't');
|
||||
mkdirSync(join(dir, 'bin'));
|
||||
writeFileSync(join(dir, 'bin/slug.mjs'), 'old\n');
|
||||
git(dir, 'add', 'bin/slug.mjs');
|
||||
git(dir, 'commit', '-q', '-m', 'base');
|
||||
return dir;
|
||||
}
|
||||
|
||||
// The shape `git diff {start_sha}` gives with the new files registered --intent-to-add.
|
||||
const DIFF = [
|
||||
'diff --git a/bin/slug.mjs b/bin/slug.mjs',
|
||||
'index 3367afd..e69de29 100644',
|
||||
'--- a/bin/slug.mjs',
|
||||
'+++ b/bin/slug.mjs',
|
||||
'@@ -1 +1 @@',
|
||||
'-old',
|
||||
'+new',
|
||||
'diff --git a/tests/batch.sh b/tests/batch.sh',
|
||||
'new file mode 100755',
|
||||
'index 0000000..1111111',
|
||||
'--- /dev/null',
|
||||
'+++ b/tests/batch.sh',
|
||||
'@@ -0,0 +1 @@',
|
||||
'+cat',
|
||||
'diff --git a/tests/cli-stdin.test.mjs b/tests/cli-stdin.test.mjs',
|
||||
'new file mode 100644',
|
||||
'--- /dev/null',
|
||||
'+++ b/tests/cli-stdin.test.mjs',
|
||||
'@@ -0,0 +1 @@',
|
||||
'+t',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
function run(dir, extra = []) {
|
||||
writeFileSync(join(dir, 'upward.diff'), DIFF);
|
||||
return spawnSync(process.execPath, [CLI, '--diff', join(dir, 'upward.diff'), '--to', join(dir, 'parked'), '--cwd', dir, ...extra], { encoding: 'utf8' });
|
||||
}
|
||||
|
||||
test('park: the step\'s untracked new files move into --to with their paths; tracked files stay', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
mkdirSync(join(dir, 'tests'));
|
||||
writeFileSync(join(dir, 'tests/batch.sh'), 'stale | cat\n');
|
||||
writeFileSync(join(dir, 'tests/cli-stdin.test.mjs'), 'stale test\n');
|
||||
writeFileSync(join(dir, 'bin/slug.mjs'), 'new\n');
|
||||
const r = run(dir);
|
||||
assert.equal(r.status, 0, r.stderr);
|
||||
const out = JSON.parse(r.stdout);
|
||||
assert.deepEqual(out.parked, ['tests/batch.sh', 'tests/cli-stdin.test.mjs']);
|
||||
assert.equal(existsSync(join(dir, 'tests/batch.sh')), false, 'the stale file is out of the tree');
|
||||
assert.equal(readFileSync(join(dir, 'parked/tests/batch.sh'), 'utf8'), 'stale | cat\n', 'and kept, never deleted');
|
||||
assert.equal(readFileSync(join(dir, 'bin/slug.mjs'), 'utf8'), 'new\n', 'a tracked file is not the helper\'s to move');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('park: a new file that is tracked now, or already gone, is kept and named, never moved', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
mkdirSync(join(dir, 'tests'));
|
||||
writeFileSync(join(dir, 'tests/batch.sh'), 'committed by the operator\n');
|
||||
git(dir, 'add', 'tests/batch.sh');
|
||||
git(dir, 'commit', '-q', '-m', 'operator');
|
||||
const r = run(dir);
|
||||
assert.equal(r.status, 0, r.stderr);
|
||||
const out = JSON.parse(r.stdout);
|
||||
assert.deepEqual(out.parked, []);
|
||||
assert.deepEqual(out.kept.map((k) => k.path), ['tests/batch.sh', 'tests/cli-stdin.test.mjs']);
|
||||
assert.match(out.kept[0].reason, /tracked/);
|
||||
assert.match(out.kept[1].reason, /not on disk/);
|
||||
assert.equal(existsSync(join(dir, 'tests/batch.sh')), true);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('park: a path that leaves the repository is refused, never moved', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
const evil = DIFF.replaceAll('tests/batch.sh', '../outside.sh');
|
||||
writeFileSync(join(dir, 'upward.diff'), evil);
|
||||
const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, 'upward.diff'), '--to', join(dir, 'parked'), '--cwd', dir], { encoding: 'utf8' });
|
||||
assert.equal(r.status, 0, r.stderr);
|
||||
const out = JSON.parse(r.stdout);
|
||||
assert.ok(out.kept.some((k) => k.path === '../outside.sh' && /outside the repository/.test(k.reason)), r.stdout);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('park: an unreadable diff or missing arguments exit 2 and move nothing', () => {
|
||||
const dir = repo();
|
||||
try {
|
||||
const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, 'nope.diff'), '--to', join(dir, 'parked'), '--cwd', dir], { encoding: 'utf8' });
|
||||
assert.equal(r.status, 2);
|
||||
assert.equal(spawnSync(process.execPath, [CLI], { encoding: 'utf8' }).status, 2);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('trekexecute: a re-planned step parks the earlier attempt\'s new files before the fresh build', () => {
|
||||
const md = readFileSync(join(ROOT, 'commands/trekexecute.md'), 'utf8');
|
||||
const at = md.indexOf('A step at `fix_rounds` 4 on `--resume` was re-planned');
|
||||
assert.ok(at > 0);
|
||||
const para = md.slice(at, at + 1200);
|
||||
assert.match(para, /lib\/util\/park-attempt-files\.mjs/);
|
||||
assert.match(para, /step-\{N\}-upward\.diff/);
|
||||
assert.match(para, /never deleted/);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue