diff --git a/.changeset/gentle-ibex-forage.md b/.changeset/gentle-ibex-forage.md new file mode 100644 index 000000000..05e42b898 --- /dev/null +++ b/.changeset/gentle-ibex-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2860 +--- +**The .planning/ write reminder can no longer be suppressed or fabricated by a model-supplied file_path** — the phase-boundary hook now treats `tool_input.path` (the field kimi-cli actually executes on) as authoritative and `file_path` as the fallback, reaching the same "path authoritative" outcome the JS guards establish via upstream normalization (#2595). Previously a model-controlled decoy `file_path` could silence the reminder for a genuine .planning/ write or raise one naming a file never touched. (#2752) diff --git a/hooks/gsd-phase-boundary.sh b/hooks/gsd-phase-boundary.sh index d791d8c49..d383424b3 100755 --- a/hooks/gsd-phase-boundary.sh +++ b/hooks/gsd-phase-boundary.sh @@ -22,7 +22,15 @@ INPUT=$(cat) # and its file tools name the field `path`, not `file_path` (kimi-cli # src/kimi_cli/tools/file/write.py + replace.py) — fall back to tool_input.path # when file_path is absent, mirroring normalizeKimiPayload in the JS guards. -FILE=$(echo "$INPUT" | node -e "let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d).tool_input||{};process.stdout.write(i.file_path||(typeof i.path==='string'?i.path:'')||'')}catch{}})" 2>/dev/null) +# #2752: `path` is AUTHORITATIVE (kimi-cli executes on it; it sends `path` only, +# never `file_path`). `file_path` is model-controlled on Kimi, so consulting it +# first let a model-supplied decoy suppress/fabricate the reminder. `path` wins, +# `file_path` is the fallback (Claude Code emits `file_path` and no `path`, so the +# fallback must remain). The JS guards reach the same "path authoritative" outcome +# via an upstream normalizeKimiPayload step (copies path→file_path before any guard +# reads); this shell hook parses tool_input once, raw, so it applies the precedence +# directly at the read site. +FILE=$(echo "$INPUT" | node -e "let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d).tool_input||{};process.stdout.write((typeof i.path==='string'&&i.path)||(typeof i.file_path==='string'&&i.file_path)||'')}catch{}})" 2>/dev/null) # Emit a structured JSON envelope (#2974). additionalContext carries the # user-visible reminder text; the typed `planning_modified` boolean and diff --git a/tests/hooks-opt-in.test.cjs b/tests/hooks-opt-in.test.cjs index 6a22985e5..e9458d22c 100644 --- a/tests/hooks-opt-in.test.cjs +++ b/tests/hooks-opt-in.test.cjs @@ -420,22 +420,50 @@ describe('hook execution when enabled', { skip: isWindows ? 'bash hooks require assert.strictEqual(parsed.hookSpecificOutput.file_path, '.planning/STATE.md'); }); - test('phase-boundary prefers Claude file_path when both fields are present (#2304)', () => { + // #2752 — `path` is the AUTHORITATIVE field (kimi-cli executes on it; its file + // tools send `path` only). `file_path` is model-controlled on Kimi (kimi-cli never + // sends it). The old precedence (`file_path || path`) let a model-supplied decoy + // `file_path` suppress the reminder for a real write or fabricate one for a file + // never touched. Mirrors the #2595 JS-guard fix: `path` wins, `file_path` is the + // fallback (Claude emits `file_path` and no `path`, so the fallback must remain). + test('phase-boundary prefers Kimi tool_input.path when both fields are present (#2752)', () => { const hookPath = path.join(HOOKS_DIR, 'gsd-phase-boundary.sh'); - const input = JSON.stringify({ - tool_input: { file_path: '.planning/STATE.md', path: 'unrelated.txt' } + // Suppression repro: a real .planning/ write WITH a decoy non-empty file_path. + const suppressionInput = JSON.stringify({ + tool_name: 'kimi_cli.tools.file:StrReplaceFile', + tool_input: { path: '.planning/STATE.md', file_path: 'unrelated.txt', edit: { old: 'a', new: 'b' } } }); - const result = spawnHook(hookPath, { - input, + const suppressionResult = spawnHook(hookPath, { + input: suppressionInput, encoding: 'utf-8', cwd: tmpDir, }); - assert.strictEqual(result.status, 0, `Should exit 0: ${result.stderr}`); - const parsed = JSON.parse(result.stdout); - assert.strictEqual(parsed.hookSpecificOutput.file_path, '.planning/STATE.md', - 'file_path must win over path — normalization is a fallback, not an override'); + assert.strictEqual(suppressionResult.status, 0, `Should exit 0: ${suppressionResult.stderr}`); + const suppressionParsed = JSON.parse(suppressionResult.stdout); + assert.strictEqual(suppressionParsed.hookSpecificOutput.planning_modified, true, + 'A real .planning/STATE.md write must NOT be suppressed by a model-supplied decoy file_path (#2752)'); + assert.strictEqual(suppressionParsed.hookSpecificOutput.file_path, '.planning/STATE.md', + 'path must win over file_path — the runtime executes on path, file_path is the fallback'); + + // Fabrication repro: a write ELSEWHERE with a decoy file_path pointing into .planning/. + const fabricationInput = JSON.stringify({ + tool_name: 'kimi_cli.tools.file:StrReplaceFile', + tool_input: { path: 'src/index.ts', file_path: '.planning/STATE.md', edit: { old: 'a', new: 'b' } } + }); + + const fabricationResult = spawnHook(hookPath, { + input: fabricationInput, + encoding: 'utf-8', + cwd: tmpDir, + }); + + assert.strictEqual(fabricationResult.status, 0, `Should exit 0: ${fabricationResult.stderr}`); + // No reminder emitted — the write was to src/index.ts; the decoy .planning/ + // file_path must NOT fabricate a reminder for a file never touched. + assert.strictEqual(fabricationResult.stdout, '', + 'A decoy .planning/ file_path must NOT fabricate a reminder when the real path is outside .planning/ (#2752)'); }); test('phase-boundary negative control: Kimi path outside .planning/ stays silent (#2304)', () => { diff --git a/tests/kimi-guard-normalization-parity.test.cjs b/tests/kimi-guard-normalization-parity.test.cjs index 86329c916..0c1f766c7 100644 --- a/tests/kimi-guard-normalization-parity.test.cjs +++ b/tests/kimi-guard-normalization-parity.test.cjs @@ -159,12 +159,13 @@ describe('Kimi shell-guard vocabulary parity (#2304)', () => { ); }); - test('gsd-phase-boundary.sh falls back to Kimi\'s tool_input.path field', () => { + test('gsd-phase-boundary.sh prefers Kimi\'s authoritative tool_input.path, falls back to file_path (#2752)', () => { const src = readHook('hooks/gsd-phase-boundary.sh'); assert.ok( - src.includes('i.file_path||(typeof i.path===\'string\'?i.path:\'\')'), - 'gsd-phase-boundary.sh no longer falls back to tool_input.path — ' + - 'the hook reads an empty path on Kimi (#2304)' + src.includes('(typeof i.path===\'string\'&&i.path)||(typeof i.file_path===\'string\'&&i.file_path)||\'\''), + 'gsd-phase-boundary.sh no longer prefers tool_input.path over file_path — ' + + 'path is the authoritative field (kimi-cli executes on it); a model-supplied ' + + 'decoy file_path must not suppress or fabricate a reminder (#2752, mirrors #2595)' ); }); });