fix(trekendsession): Phase 3 re-reads the arguments instead of splicing the label into the shell line
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
2c0a98c1ba
commit
89975a1fd6
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
5 changed files with 223 additions and 15 deletions
|
|
@ -11,9 +11,12 @@
|
|||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { readFileSync, readdirSync } from 'node:fs';
|
||||
import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = join(HERE, '..', '..');
|
||||
|
|
@ -125,3 +128,108 @@ test('trekendsession Phase 4 — validator call is runtime Bash (no eager prefix
|
|||
'Phase 4 must not use <angle> placeholders in commands — zsh parses <x> as input redirection',
|
||||
);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------
|
||||
// DJ5 (fix round 4.5b): Phase 3 splices no label into its shell line.
|
||||
// The block used to end in '{project_dir}' '{next_brief_path}'
|
||||
// '{next_label}', filled in by the model: a label holding a ' closed the
|
||||
// quote and the rest ran as shell. Phase 3 now re-reads $ARGUMENTS through
|
||||
// the quoted heredoc on stdin, parses it with parseArgs and re-derives the
|
||||
// project directory itself. The runtime tests run the block as Claude Code
|
||||
// would after it substitutes ${CLAUDE_PLUGIN_ROOT} and $ARGUMENTS.
|
||||
// ---------------------------------------------------------------
|
||||
|
||||
const ARGS_DELIM = 'VOYAGE_ARGS_EOF_7F3C9E';
|
||||
const PROJECT_REL = join('.claude', 'projects', '2026-01-01-demo');
|
||||
const NEXT_BRIEF = '.claude/projects/2026-01-01-demo/next-brief.md';
|
||||
|
||||
/** The one runtime ```bash block of Phase 3 (the atomic-write block). */
|
||||
function phase3Block() {
|
||||
const phase3 = extractPhase(readCommand(), '## Phase 3 ');
|
||||
const blocks = phase3.split('\n```bash\n').slice(1).map((b) => b.split('\n```')[0])
|
||||
.filter((b) => b.includes('atomic-write.mjs'));
|
||||
assert.equal(blocks.length, 1, `Phase 3 must hold one \`\`\`bash block that runs atomic-write.mjs; found ${blocks.length}`);
|
||||
return blocks[0];
|
||||
}
|
||||
|
||||
/** True when `bin` can be started from PATH. */
|
||||
function onPath(bin) {
|
||||
const r = spawnSync(bin, ['-c', 'exit 0'], { env: childEnv(), encoding: 'utf8' });
|
||||
return r.error === undefined && r.status === 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the Phase 3 block with `shell` in a fresh fixture project, with
|
||||
* `argText` standing where Claude Code puts $ARGUMENTS. Returns the result,
|
||||
* what the run wrote, and whether a PWNED file appeared; the fixture is gone
|
||||
* on return.
|
||||
*/
|
||||
function runPhase3(shell, argText) {
|
||||
const block = phase3Block();
|
||||
assert.ok(block.includes('$ARGUMENTS'),
|
||||
'Phase 3 does not re-read $ARGUMENTS: its values still come from text the model splices into the shell line');
|
||||
const script = block.split('${CLAUDE_PLUGIN_ROOT}').join(ROOT).split('$ARGUMENTS').join(argText);
|
||||
const fixture = mkdtempSync(join(tmpdir(), 'trekendsession-dj5-'));
|
||||
try {
|
||||
mkdirSync(join(fixture, PROJECT_REL), { recursive: true });
|
||||
writeFileSync(join(fixture, PROJECT_REL, 'brief.md'), '# demo brief\n');
|
||||
const r = spawnSync(shell, ['-c', script], { cwd: fixture, env: childEnv(), encoding: 'utf8' });
|
||||
const stateFile = join(fixture, PROJECT_REL, '.session-state.local.json');
|
||||
const promptFile = join(fixture, PROJECT_REL, 'NEXT-SESSION-PROMPT.local.md');
|
||||
return {
|
||||
r,
|
||||
pwned: ['PWNED', 'PWNED2'].filter((f) => existsSync(join(fixture, f))),
|
||||
state: existsSync(stateFile) ? JSON.parse(readFileSync(stateFile, 'utf8')) : null,
|
||||
prompt: existsSync(promptFile) ? readFileSync(promptFile, 'utf8') : null,
|
||||
};
|
||||
} finally {
|
||||
rmSync(fixture, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
/** Assert one run wrote `label` whole and ran nothing from it. */
|
||||
function assertLabelKept(shell, label) {
|
||||
const { r, pwned, state, prompt } = runPhase3(shell, `${NEXT_BRIEF} ${label}`);
|
||||
const how = `${shell}, label ${JSON.stringify(label)}: exit ${r.status}\n${r.stdout}${r.stderr}`;
|
||||
assert.deepEqual(pwned, [], `the label ran as shell under ${how}`);
|
||||
assert.equal(r.status, 0, `Phase 3 did not run cleanly under ${how}`);
|
||||
assert.ok(state, `no .session-state.local.json was written under ${how}`);
|
||||
assert.equal(state.next_session_label, label, `the written label is not the typed one under ${how}`);
|
||||
assert.equal(state.next_session_brief_path, NEXT_BRIEF, `the written brief path is not the typed one under ${how}`);
|
||||
assert.equal(state.project, PROJECT_REL, `the project directory was not re-derived under ${how}`);
|
||||
assert.ok(prompt && prompt.includes(`\n# ${label}\n`), `NEXT-SESSION-PROMPT.local.md does not carry the label under ${how}`);
|
||||
}
|
||||
|
||||
test('[DJ5] Phase 3 has no quoted placeholder for the label, brief or project', () => {
|
||||
const block = phase3Block();
|
||||
const placeholders = block.match(/\{[a-z][a-z0-9]*(?:[_ -][a-z0-9]+)*\}/g) || [];
|
||||
assert.deepEqual(placeholders, [], 'Phase 3 still has the model fill {placeholders} into its shell line');
|
||||
assert.doesNotMatch(block, /'\{/, 'Phase 3 still quotes a placeholder as a shell word');
|
||||
const lines = block.split('\n');
|
||||
const open = lines.findIndex((l) => l.trimEnd().endsWith(`<<'${ARGS_DELIM}'`));
|
||||
assert.ok(open !== -1, `Phase 3 does not re-read the arguments through the quoted heredoc <<'${ARGS_DELIM}'`);
|
||||
assert.equal(lines[open + 1], '$ARGUMENTS', 'the line inside the Phase 3 heredoc is not $ARGUMENTS alone');
|
||||
const filled = lines.filter((l) => l.trim() !== '');
|
||||
assert.ok(lines[open + 2] === ARGS_DELIM && filled[filled.length - 1] === ARGS_DELIM,
|
||||
`the closing line ${ARGS_DELIM} does not follow $ARGUMENTS as the block's last non-empty line`);
|
||||
assert.equal(lines.filter((l, i) => i !== open + 1 && l.includes('$ARGUMENTS')).length, 0,
|
||||
'a line outside the Phase 3 heredoc carries $ARGUMENTS');
|
||||
assert.match(block, /import \{ parseArgs \} from '\$\{CLAUDE_PLUGIN_ROOT\}\/lib\/parsers\/arg-parser\.mjs'/,
|
||||
'Phase 3 does not import parseArgs at runtime from the plugin root');
|
||||
});
|
||||
|
||||
test('[DJ5] Phase 3 run with an injection label creates no file and keeps the label whole', (t) => {
|
||||
// The step's label, and the same with a command substitution: under bash,
|
||||
// and under zsh (the shell the Bash tool runs) when zsh is on PATH.
|
||||
const labels = ["a'; touch PWNED; echo 'b", "a'; touch PWNED; echo 'b $(touch PWNED2)"];
|
||||
for (const label of labels) assertLabelKept('bash', label);
|
||||
if (onPath('zsh')) {
|
||||
for (const label of labels) assertLabelKept('zsh', label);
|
||||
} else {
|
||||
t.diagnostic('zsh is not on PATH: the zsh half of this test was not run');
|
||||
}
|
||||
});
|
||||
|
||||
test("[DJ5] a label with an apostrophe is kept", () => {
|
||||
assertLabelKept('bash', "Fix the user's login");
|
||||
});
|
||||
|
|
|
|||
|
|
@ -90,4 +90,20 @@ run_tagged "$TMP/base.jsonl" --test-timeout=300000 --test-force-exit $PATS`,
|
|||
},
|
||||
],
|
||||
},
|
||||
{
|
||||
key: 'E16a',
|
||||
id: 'E16a',
|
||||
what: 'the next-session-prompt validator CLI loses its refusal of a --state-file with no value',
|
||||
spans: [
|
||||
{
|
||||
file: 'lib/validators/next-session-prompt-validator.mjs',
|
||||
find: String.raw`
|
||||
if (v === undefined || v.startsWith('--')) {
|
||||
process.stderr.write('next-session-prompt-validator: --state-file needs a value\n');
|
||||
process.exit(2);
|
||||
}`,
|
||||
replace: '',
|
||||
},
|
||||
],
|
||||
},
|
||||
];
|
||||
|
|
|
|||
|
|
@ -382,3 +382,38 @@ test("[ARG] the guard's added lines are ASCII", () => {
|
|||
assert.match(block, /VOYAGE_ARGS_EOF_7F3C9E/);
|
||||
assert.doesNotMatch(block, /[^\x00-\x7f]/, 'a hook script is ASCII (~/.claude/CLAUDE.md, Miljo)');
|
||||
});
|
||||
|
||||
// The four parser heredoc blocks the commands ship, as Claude Code runs them
|
||||
// after it substitutes $ARGUMENTS and ${CLAUDE_PLUGIN_ROOT}: a block the guard
|
||||
// refuses would disable its command while every other test stays green.
|
||||
// [file, the heading the section starts with]
|
||||
const SHIPPED_ARGS_BLOCKS = [
|
||||
['commands/trekendsession.md', '## Phase 0 '],
|
||||
['commands/trekendsession.md', '## Phase 3 '],
|
||||
['commands/trekcontinue.md', '## Phase 0 '],
|
||||
['commands/trekreview.md', '## Phase 1 '],
|
||||
];
|
||||
|
||||
/** The ```bash blocks of the `## ` section of `rel` that starts with `heading` (fence-aware). */
|
||||
function sectionBashBlocks(rel, heading) {
|
||||
const lines = readFileSync(join(ROOT, rel), 'utf8').split('\n');
|
||||
const s = lines.findIndex((l) => l.startsWith(heading));
|
||||
assert.ok(s !== -1, `${rel} must hold a heading starting "${heading}"`);
|
||||
let e = lines.length;
|
||||
let fenced = false;
|
||||
for (let i = s + 1; i < lines.length; i++) {
|
||||
if (/^\s{0,3}(`{3,}|~{3,})/.test(lines[i])) fenced = !fenced;
|
||||
if (!fenced && lines[i].startsWith('## ')) { e = i; break; }
|
||||
}
|
||||
const section = lines.slice(s, e).join('\n');
|
||||
return section.split('\n```bash\n').slice(1).map((b) => b.split('\n```')[0]);
|
||||
}
|
||||
|
||||
test('[ARG] every shipped parser heredoc block passes the guard', async () => {
|
||||
for (const [rel, heading] of SHIPPED_ARGS_BLOCKS) {
|
||||
const blocks = sectionBashBlocks(rel, heading).filter((b) => b.includes(`<<'${ARGS_DELIM}'`));
|
||||
assert.equal(blocks.length, 1, `${rel} ${heading.trim()} must hold one \`\`\`bash block with the parser heredoc <<'${ARGS_DELIM}'; found ${blocks.length}`);
|
||||
const command = blocks[0].split('$ARGUMENTS').join('brief.md Session 2 of 3').split('${CLAUDE_PLUGIN_ROOT}').join(ROOT);
|
||||
await expectArgsAllowed(command);
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import { tmpdir } from 'node:os';
|
|||
import { join } from 'node:path';
|
||||
import { execFileSync, spawnSync } from 'node:child_process';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
import {
|
||||
validateNextSessionPromptContent,
|
||||
|
|
@ -204,3 +205,37 @@ test('[#102] sibling: rb.valid counts: an invalid B makes the consistency result
|
|||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// --- E16a (fix round 4.5b): --state-file without a value is refused ----------
|
||||
// The CLI refuses --state-file when no value follows it, or when the next word
|
||||
// is another option, with exit 2 and "needs a value". Without the refusal the
|
||||
// consistency call runs on the same pair and exits 0 or 1, so each test runs
|
||||
// that call and asserts the exit code and the message.
|
||||
|
||||
function runCliClean(args) {
|
||||
return spawnSync(process.execPath, [CLI, ...args], { encoding: 'utf-8', env: childEnv() });
|
||||
}
|
||||
|
||||
test('[E16a] --state-file as the last argument exits 2 with needs a value', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'nspv-cli-'));
|
||||
try {
|
||||
const { a, b } = stalePair(dir);
|
||||
const r = runCliClean(['--json', '--consistency', a, b, '--state-file']);
|
||||
assert.equal(r.status, 2, `a --state-file with no value was not refused: exit ${r.status}: ${r.stdout}${r.stderr}`);
|
||||
assert.match(r.stderr, /--state-file needs a value/, `the refusal does not say the flag needs a value: ${r.stderr}`);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('[E16a] --state-file followed by another option exits 2', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'nspv-cli-'));
|
||||
try {
|
||||
const { a, b } = stalePair(dir);
|
||||
const r = runCliClean(['--state-file', '--json', '--consistency', a, b]);
|
||||
assert.equal(r.status, 2, `--state-file took the option after it as its value: exit ${r.status}: ${r.stdout}${r.stderr}`);
|
||||
assert.match(r.stderr, /--state-file needs a value/, `the refusal does not say the flag needs a value: ${r.stderr}`);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue