Przeglądaj źródła

fix(hook-protocol): discard a hookSpecificOutput block whose hookEventName mismatches the firing event

The reference schemas key the `hookSpecificOutput` block by `hookEventName`, so
a block naming a DIFFERENT event than the one firing is malformed — a hook
emitting `hookSpecificOutput.hookEventName: "PreToolUse"` on a `Stop` event must
not deny the Stop. The codec surfaced `hookEventName` for a bridge to compare but
never enforced the discard, so both bridges pushed every parsed output into the
merge unconditionally.

parseHookOutput now takes an optional `expectedEventName`; when the block's
`hookEventName` names a different event, its event-scoped fields
(permissionDecision/permissionDecisionReason/additionalContext/updatedInput) are
discarded (the discriminator is still surfaced for the log, and the
event-agnostic top-level decision/continue/etc. are unaffected). runHook threads
it via RunHookOptions.expectedEventName; a caller that omits it opts out.

Codex review finding on the bridges PR (PR-F); fixed here on the codec that owns
the fold and knows field provenance, then flows down to both bridges.
Tianyi Cui 2 miesięcy temu
rodzic
commit
24e9c0fa70

+ 2 - 2
packages/hooks/hook-protocol/README.md

@@ -17,8 +17,8 @@ Why a shared lib at all: Codex deliberately reimplements a *subset* of the Claud
 ## Primitives
 
 - **`matchesMatcher(matcher, query, mode)`** — match-all on absent/`''`/`'*'`; `claude` mode treats a pure `[A-Za-z0-9_|]+` pattern as a literal (pipe = exact-match alternation) and anything else as a regex; `codex` mode is always an unanchored regex. An invalid regex matches nothing (never throws).
-- **`runHook(bash, hook, options, now)`** — serialize `options.payload` to the hook's stdin (with a trailing newline iff `options.trailingNewline`), merge `options.env` after the executor's credential scrub (the `dsh-bash` trusted-plugin surface), honor the hook's `timeoutSec` (else `defaultTimeoutMs`), and decode the result. Never throws: an executor rejection (infra fault) becomes a `HookOutput` with `exitCode: undefined` (a non-blocking error). `now` is injected for testable durations.
-- **`parseHookOutput(exitCode, stdout, stderr)`** — the exit-code + structured-stdout codec. Exit `0` → parse JSON stdout (lenient: non-JSON is left for the bridge); exit `2` → blocking error, `stderr` is the block reason (surfaced as `decision: 'block'`); other → non-blocking error. `hookSpecificOutput.permissionDecision` (allow/deny/ask) overrides a legacy top-level `decision`; `additionalContext`/`updatedInput`/`systemMessage`/`continue`/`stopReason`/`suppressOutput` are parsed too. Pure and total.
+- **`runHook(bash, hook, options, now)`** — serialize `options.payload` to the hook's stdin (with a trailing newline iff `options.trailingNewline`), merge `options.env` after the executor's credential scrub (the `dsh-bash` trusted-plugin surface), honor the hook's `timeoutSec` (else `defaultTimeoutMs`), and decode the result (threading `options.expectedEventName` to the codec). Never throws: an executor rejection (infra fault) becomes a `HookOutput` with `exitCode: undefined` (a non-blocking error). `now` is injected for testable durations.
+- **`parseHookOutput(exitCode, stdout, stderr, expectedEventName?)`** — the exit-code + structured-stdout codec. Exit `0` → parse JSON stdout (lenient: non-JSON is left for the bridge); exit `2` → blocking error, `stderr` is the block reason (surfaced as `decision: 'block'`); other → non-blocking error. `hookSpecificOutput.permissionDecision` (allow/deny/ask) overrides a legacy top-level `decision`; `additionalContext`/`updatedInput`/`systemMessage`/`continue`/`stopReason`/`suppressOutput` are parsed too. The schemas key the `hookSpecificOutput` block by `hookEventName`, so passing `expectedEventName` (the firing event) DISCARDS a block whose `hookEventName` names a different event — its event-scoped fields don't take effect (a `PreToolUse` block on a `Stop` hook is malformed), while the event-agnostic top-level fields still apply. Pure and total.
 - **`mergeHookOutputs(outputs)`** — fold the results of every hook that matched one point: permission precedence **deny > ask > allow**, halt sticky on the first `continue:false`, block reasons joined with `\n\n`, `additionalContext`/`systemMessages` accumulated in order.
 
 ## `hook/*` session events

+ 30 - 7
packages/hooks/hook-protocol/src/codec.ts

@@ -64,8 +64,19 @@ function permissionDecisionOf(value: string | undefined): HookOutput['decision']
  * JSON on a 0 exit is treated as "no structured output" (the plain stdout is
  * still on the bridge to use), matching both reference engines' lenient parse of
  * non-JSON stdout.
+ *
+ * `expectedEventName` is the event the hook is FIRING for (e.g. `'PreToolUse'`).
+ * The reference schemas key the `hookSpecificOutput` block by `hookEventName`,
+ * so a block whose `hookEventName` names a DIFFERENT event is malformed and its
+ * event-scoped fields (`permissionDecision`/`permissionDecisionReason`/
+ * `additionalContext`/`updatedInput`) are DISCARDED — a `PreToolUse` block on a
+ * `Stop` hook must not deny the `Stop`. The block's `hookEventName` is still
+ * surfaced (for the log/diagnostics), and the event-agnostic top-level fields
+ * (`decision`/`reason`/`continue`/`stopReason`/`suppressOutput`/`systemMessage`)
+ * are unaffected. Omit `expectedEventName` (or pass a matching one) to apply the
+ * block as-is — a caller that doesn't key by event opts out of the check.
  */
-export function parseHookOutput(exitCode: number | undefined, stdout: string, stderr: string): HookOutput {
+export function parseHookOutput(exitCode: number | undefined, stdout: string, stderr: string, expectedEventName?: string): HookOutput {
   const trimmedErr = stderr.trim()
   const trimmedOut = stdout.trim()
   // Keep the raw stdout verbatim: a clean-exit hook may emit PLAIN text the
@@ -96,15 +107,20 @@ export function parseHookOutput(exitCode: number | undefined, stdout: string, st
         // reference engines are). The plain stdout remains the bridge's to use.
         parsed = undefined
       }
-      if (parsed) applyStructured(output, parsed)
+      if (parsed) applyStructured(output, parsed, expectedEventName)
     }
   }
 
   return output
 }
 
-/** Fold a parsed structured-stdout object into `output` (mutates in place). */
-function applyStructured(output: HookOutput, parsed: Record<string, unknown>): void {
+/**
+ * Fold a parsed structured-stdout object into `output` (mutates in place).
+ * `expectedEventName` (the firing event) gates the per-event `hookSpecificOutput`
+ * block: a block whose `hookEventName` names a different event has its
+ * event-scoped fields discarded (only its `hookEventName` is recorded).
+ */
+function applyStructured(output: HookOutput, parsed: Record<string, unknown>, expectedEventName?: string): void {
   const cont = bool(parsed, 'continue')
   if (cont !== undefined) output.continue = cont
   const stopReason = str(parsed, 'stopReason')
@@ -121,15 +137,22 @@ function applyStructured(output: HookOutput, parsed: Record<string, unknown>): v
   const topReason = str(parsed, 'reason')
   if (topReason !== undefined) output.reason = topReason
 
-  // hookSpecificOutput: the per-event channel, keyed by `hookEventName`. We
-  // surface that discriminator so the bridge can DISCARD a block whose event
-  // doesn't match the firing one (the schemas make it the discriminator). The
+  // hookSpecificOutput: the per-event channel, keyed by `hookEventName`. The
   // permissionDecision (allow/deny/ask) OVERRIDES the legacy top-level decision;
   // additionalContext and updatedInput live here too.
   const hso = obj(parsed.hookSpecificOutput)
   if (hso) {
     const eventName = str(hso, 'hookEventName')
+    // Always surface the discriminator (for the log/diagnostics), even on a
+    // mismatch — the record should show what the malformed block claimed.
     if (eventName !== undefined) output.hookEventName = eventName
+    // The schemas key this block by event: if it names a DIFFERENT event than the
+    // one firing, it is malformed — discard its event-scoped fields (a PreToolUse
+    // block must not deny a Stop hook). A caller that passes no expectedEventName
+    // opts out of the check (applies the block as-is).
+    if (expectedEventName !== undefined && eventName !== undefined && eventName !== expectedEventName) {
+      return
+    }
     const permission = permissionDecisionOf(str(hso, 'permissionDecision'))
     if (permission !== undefined) output.decision = permission
     const permissionReason = str(hso, 'permissionDecisionReason')

+ 8 - 1
packages/hooks/hook-protocol/src/runner.ts

@@ -31,6 +31,13 @@ export interface RunHookOptions {
   defaultTimeoutMs: number
   /** Whether to append a trailing newline to the stdin payload (CC yes, Codex no). */
   trailingNewline: boolean
+  /**
+   * The event this hook is firing for (e.g. `'PreToolUse'`). When set, a
+   * structured `hookSpecificOutput` block whose `hookEventName` names a DIFFERENT
+   * event is treated as malformed and its event-scoped fields are discarded (see
+   * {@link parseHookOutput}). Omit it to apply any block as-is.
+   */
+  expectedEventName?: string
 }
 
 /** The {@link HookOutput} plus the wall-clock duration of the run (for `hook/result`). */
@@ -75,7 +82,7 @@ export async function runHook(
     // `undefined` (a non-blocking error — no clean exit code to act on).
     const exitCode = result.exitCode ?? undefined
     return {
-      output: parseHookOutput(exitCode, result.stdout.text, result.stderr.text),
+      output: parseHookOutput(exitCode, result.stdout.text, result.stderr.text, options.expectedEventName),
       durationMs: now() - started,
     }
   } catch (error: unknown) {

+ 6 - 3
packages/hooks/hook-protocol/src/types.ts

@@ -132,9 +132,12 @@ export interface HookOutput {
   reason?: string
   /**
    * The `hookSpecificOutput.hookEventName` discriminator, when the hook emitted
-   * a `hookSpecificOutput` block. The reference schemas key that block by event;
-   * a bridge compares this to the firing event and DISCARDS a mismatched block
-   * (a hook claiming `PreToolUse` output on a `Stop` event is malformed). Absent
+   * a `hookSpecificOutput` block. The reference schemas key that block by event,
+   * so a block whose `hookEventName` names a DIFFERENT event than the one firing
+   * is malformed: {@link parseHookOutput} DISCARDS its event-scoped fields when
+   * given the firing event's `expectedEventName` (a hook claiming `PreToolUse`
+   * output on a `Stop` event does not affect the `Stop`). This field is still
+   * surfaced even on a mismatch — the record shows what the block claimed. Absent
    * when the hook emitted no `hookSpecificOutput`.
    */
   hookEventName?: string

+ 48 - 0
packages/hooks/hook-protocol/tests/codec.spec.ts

@@ -93,6 +93,54 @@ describe('parseHookOutput — structured stdout (exit 0 only)', () => {
     expect(parseHookOutput(0, JSON.stringify({ decision: 'maybe' }), '').decision).toBeUndefined()
   })
 
+  it('DISCARDS a hookSpecificOutput block whose hookEventName mismatches the firing event', () => {
+    // A PreToolUse block emitted on a Stop hook is malformed — its event-scoped
+    // fields must not take effect (a stray PreToolUse deny must not deny the Stop).
+    const out = parseHookOutput(0, JSON.stringify({
+      hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'deny', permissionDecisionReason: 'no', additionalContext: 'x', updatedInput: { command: 'y' } },
+    }), '', 'Stop')
+    expect(out.hookEventName).toBe('PreToolUse') // still recorded for the log
+    expect(out.decision).toBeUndefined() // event-scoped fields discarded
+    expect(out.reason).toBeUndefined()
+    expect(out.additionalContext).toBeUndefined()
+    expect(out.updatedInput).toBeUndefined()
+  })
+
+  it('APPLIES a hookSpecificOutput block whose hookEventName matches the firing event', () => {
+    const out = parseHookOutput(0, JSON.stringify({
+      hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'deny', additionalContext: 'x' },
+    }), '', 'PreToolUse')
+    expect(out.decision).toBe('deny')
+    expect(out.additionalContext).toBe('x')
+  })
+
+  it('applies the block when expectedEventName is omitted (opt-out) even if it names an event', () => {
+    const out = parseHookOutput(0, JSON.stringify({
+      hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'deny' },
+    }), '')
+    expect(out.decision).toBe('deny')
+  })
+
+  it('applies a block that has NO hookEventName regardless of expectedEventName', () => {
+    // No discriminator to mismatch — the block applies (a hook that omits the key).
+    const out = parseHookOutput(0, JSON.stringify({
+      hookSpecificOutput: { permissionDecision: 'deny' },
+    }), '', 'Stop')
+    expect(out.decision).toBe('deny')
+  })
+
+  it('a mismatched block does NOT discard the event-agnostic top-level decision/continue', () => {
+    // Only the per-event block is scoped; top-level fields are event-agnostic.
+    const out = parseHookOutput(0, JSON.stringify({
+      decision: 'block', reason: 'top', continue: false, stopReason: 'halt',
+      hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'allow' },
+    }), '', 'Stop')
+    expect(out.decision).toBe('block') // top-level survives; the allow block was discarded
+    expect(out.reason).toBe('top')
+    expect(out.continue).toBe(false)
+    expect(out.stopReason).toBe('halt')
+  })
+
   it('malformed JSON on a clean exit is lenient (no structured output, no throw)', () => {
     const out = parseHookOutput(0, '{ not valid json', '')
     expect(out.decision).toBeUndefined()

+ 13 - 0
packages/hooks/hook-protocol/tests/runner.spec.ts

@@ -131,4 +131,17 @@ describe('runHook — outcome decoding + duration', () => {
     const { output } = await runHook(bash, { command: 'h' }, { payload: {}, defaultTimeoutMs: 1000, trailingNewline: true }, clock())
     expect(output.stderr).toBe('plain string fault')
   })
+
+  it('threads expectedEventName so a mismatched hookSpecificOutput block is discarded', async () => {
+    const { bash } = recordingBash(async () => result({
+      exitCode: 0,
+      stdout: { text: JSON.stringify({ hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'deny' } }), truncated: false },
+    }))
+    const { output } = await runHook(bash, { command: 'h' }, {
+      payload: {}, defaultTimeoutMs: 1000, trailingNewline: true, expectedEventName: 'Stop',
+    }, clock())
+    // A PreToolUse block on a Stop hook is malformed → its decision is discarded.
+    expect(output.hookEventName).toBe('PreToolUse')
+    expect(output.decision).toBeUndefined()
+  })
 })