* fix(#2305): stage shared guard hooks for Kilo — drop skipSharedHooksInstall Kilo's capability descriptor declared BOTH hostBehaviors.nativePlugin (a plugin that spawns the shared PreToolUse guard scripts as subprocesses) AND hostBehaviors.skipSharedHooksInstall:true, which suppresses staging of hooks/*.js into the Kilo config dir. The plugin's runHook treats an absent hook script as a silent allow, so every guard it spawned (gsd-prompt-guard, gsd-read-guard, gsd-worktree-path-guard) no-opped on every Kilo install. OpenCode uses the byte-identical plugin with hook staging on and is unaffected — it is the reference shape. The skip flag predates Kilo's plugin surface: it dates to #1821 (hooks were dead weight for a runtime with no hook consumer), and #2093 added the hooks-dependent nativePlugin without revisiting it. - capabilities/kilo/capability.json: remove skipSharedHooksInstall (regenerated gsd-core/bin/lib/capability-registry.cjs accordingly) - bin/install.js: correct the stale #1821 comments claiming Kilo has no plugin surface - tests/kilo-upgrades.test.cjs: install-fixture tests (global + local) asserting the guard scripts land where the plugin's walk-up resolves them; an end-to-end test driving a disallowed out-of-worktree write through the REAL installed Kilo tree and asserting the guard rejects it; a cross-runtime descriptor invariant (nativePlugin and skipSharedHooksInstall:true must never coexist) - tests/kilo-imperative-reference.test.cjs: flip the pinned assertion - golden fixtures regenerated (kilo now stages the 24 hook files, same set as OpenCode) Fixes #2305 * fix(#2305): warn loudly when a guard hook script is missing (runHook) runHook's absent-file branch returned a silent exit-0 allow — the mechanism that let #2305 ship undetected: with the hooks bundle never staged on Kilo, every PreToolUse guard the plugin spawned resolved to "file not found → allow" with zero signal anywhere. Keep the adapter's design contract (a missing hook must never break the tool call — pinned by the existing adapter test) but make the absence loud: console.error once per hook file, naming the unresolved path and the remediation. Applied identically to .kilo/ and .opencode/ plugin copies (byte-parity guard). Golden parity fixtures regenerated (the installed plugin file's hash changed). Fixes #2305 * chore(#2305): add changeset fragment * test(#2305): include gsd-workflow-guard.js in the staged-guards regression list The native plugin spawns four guards on write-like tool calls — the regression test's PLUGIN_GUARD_HOOKS list covered three. Staging itself was already asserted via the golden fixtures (the full bundle), but the named per-guard assertion should cover every guard the plugin actually dispatches. Surfaced by cross-AI review of PR #2327. * test(#2305): update the #1821 tests that encoded Kilo's false no-plugin premise The #1821 hook-copy test asserted Kilo must receive no staged hooks — the exact behavior this PR reverses (and the cause of all 8 CI failures). Kilo moves from the ZCode "no dead hooks" loop to the OpenCode group, with positive assertions on the new contract: the three guard hooks the plugin spawns, hooks/lib/git-cmd.js, and plugins/gsd-core.js all staged. The integration runtime contract flips kilo packageJson to true (the CommonJS marker ships with the bundle), and the pi contract comment no longer cites Kilo as a no-plugin runtime. * chore(#2305): scope the queued #1821 changeset fragment to ZCode only The fragment still claimed the installer skips hooks for Kilo — rendering both it and this PR's fragment into the same release would ship two contradictory statements about Kilo's install behavior. It now claims ZCode only and notes that #2327 reverses the Kilo half. * chore(#2305): rename changeset fragment to the generator naming convention 2305-kilo-stage-guard-hooks.md -> loud-guard-hooks.md, matching the <adjective>-<noun>-<noun> shape npm run changeset generates (review nit). --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
@@ -15,7 +15,9 @@
|
||||
*
|
||||
* Cross-platform note: filesystem-failure paths are not exercised here; the
|
||||
* adapter's own error handling swallows spawn failures by design (a broken hook
|
||||
* must never break a tool call), which the "missing hook" case covers.
|
||||
* must never break a tool call). A MISSING hook script likewise never breaks
|
||||
* the tool call, but since #2305 it warns loudly (once per hook file) — a
|
||||
* silently-absent guard script was exactly how every Kilo guard no-opped.
|
||||
*/
|
||||
|
||||
const { test } = require('node:test');
|
||||
@@ -279,9 +281,15 @@ test('tool.execute.after: Read content rewriting maps ~/.claude/gsd-core paths',
|
||||
);
|
||||
});
|
||||
|
||||
test('missing hook script is a silent allow (never breaks the tool call)', async (t) => {
|
||||
// No hook stubs written at all → every runHook finds no file → silent allow.
|
||||
test('missing hook script warns loudly but still allows (never breaks the tool call, #2305)', async (t) => {
|
||||
// No hook stubs written at all → every runHook finds no file → allow, but
|
||||
// each absent guard script must be warned about (once per hook file): a
|
||||
// silently-missing guard is how #2305 no-opped every Kilo guard.
|
||||
const { mod } = buildInstalledLayout(t, {});
|
||||
const warnings = [];
|
||||
const realConsoleError = console.error;
|
||||
t.after(() => { console.error = realConsoleError; });
|
||||
console.error = (...args) => { warnings.push(args.join(' ')); };
|
||||
const handlers = await mod.server({ directory: process.cwd() });
|
||||
await assert.doesNotReject(() =>
|
||||
handlers['tool.execute.before'](
|
||||
@@ -289,6 +297,21 @@ test('missing hook script is a silent allow (never breaks the tool call)', async
|
||||
{ args: { filePath: '/proj/a.md', old_string: 'a', new_string: 'b' } },
|
||||
),
|
||||
);
|
||||
const missingWarnings = warnings.filter((w) => w.includes('hook script missing'));
|
||||
assert.ok(missingWarnings.length > 0, 'a missing guard script must be warned about');
|
||||
// Warn-once: driving a second identical tool call must not re-warn.
|
||||
const warnedOnce = missingWarnings.length;
|
||||
await assert.doesNotReject(() =>
|
||||
handlers['tool.execute.before'](
|
||||
{ tool: 'edit' },
|
||||
{ args: { filePath: '/proj/a.md', old_string: 'a', new_string: 'b' } },
|
||||
),
|
||||
);
|
||||
assert.equal(
|
||||
warnings.filter((w) => w.includes('hook script missing')).length,
|
||||
warnedOnce,
|
||||
'the missing-hook warning fires once per hook file, not once per tool call',
|
||||
);
|
||||
});
|
||||
|
||||
test('config hook is a no-op in installed (non-package) layout', async (t) => {
|
||||
|
||||
Reference in New Issue
Block a user