fix(hook-protocol): discard a discriminator-less hookSpecificOutput block

Address review on the hook-protocol PR: the event-scope guard only rejected a
`hookSpecificOutput` block whose `hookEventName` NAMED a different event than
the firing one. A block with NO `hookEventName` slipped through and applied its
event-scoped permission fields to whatever event was firing. Under the keyed
Claude Code schema (where `hookEventName` is part of the block) a missing
discriminator is as malformed as a mismatched one — a Stop/UserPromptSubmit
hook emitting a bare `{ permissionDecision: 'deny' }` could deny the current
point.

Drop the `eventName !== undefined` clause so the guard fires on both a
mismatch and an omission when the caller passes `expectedEventName`; the
opt-out (no expectedEventName) still applies a discriminator-less block as-is.
Flipped the test that pinned the old behavior (it documented an artifact, not
a contract) and proved the corrected one red on the old guard.
This commit is contained in:
Tianyi Cui
2026-07-02 17:13:42 +08:00
parent 9c60fa83f9
commit 9428acdc96
3 changed files with 26 additions and 11 deletions
+1 -1
View File
@@ -18,7 +18,7 @@ Why a shared lib at all: Codex deliberately reimplements a *subset* of the Claud
- **`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 (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.
- **`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 — or omits it entirely — its event-scoped fields don't take effect (a `PreToolUse` block on a `Stop` hook is malformed, and so is a discriminator-less block that would otherwise apply to any event), 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
+10 -7
View File
@@ -117,8 +117,8 @@ export function parseHookOutput(exitCode: number | undefined, stdout: string, st
/**
* 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).
* block: a block whose `hookEventName` names a different event — OR omits it — has
* its event-scoped fields discarded (any present `hookEventName` is still recorded).
*/
function applyStructured(output: HookOutput, parsed: Record<string, unknown>, expectedEventName?: string): void {
const cont = bool(parsed, 'continue')
@@ -146,11 +146,14 @@ function applyStructured(output: HookOutput, parsed: Record<string, unknown>, ex
// 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) {
// The schemas key this block by event: when a caller passes the firing event
// (`expectedEventName`), the block's `hookEventName` MUST name it. A different
// name — or a MISSING one — is malformed under the keyed schema, so discard the
// event-scoped fields (a PreToolUse block must not deny a Stop hook; nor may a
// discriminator-less block silently apply PreToolUse-scoped permission fields to
// whatever event is firing). A caller that passes no expectedEventName opts out
// of the check (applies the block as-is).
if (expectedEventName !== undefined && eventName !== expectedEventName) {
return
}
const permission = permissionDecisionOf(str(hso, 'permissionDecision'))
@@ -121,11 +121,23 @@ describe('parseHookOutput — structured stdout (exit 0 only)', () => {
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).
it('DISCARDS a block with NO hookEventName when a firing event is expected', () => {
// Under the keyed schema a missing discriminator is as malformed as a
// mismatched one: a discriminator-less block must not apply its event-scoped
// permission fields to whatever event happens to be firing.
const out = parseHookOutput(0, JSON.stringify({
hookSpecificOutput: { permissionDecision: 'deny', additionalContext: 'x' },
}), '', 'Stop')
expect(out.hookEventName).toBeUndefined() // none to record
expect(out.decision).toBeUndefined() // event-scoped fields discarded
expect(out.additionalContext).toBeUndefined()
})
it('applies a discriminator-less block when expectedEventName is omitted (opt-out)', () => {
// With no firing event to validate against, the block applies as-is.
const out = parseHookOutput(0, JSON.stringify({
hookSpecificOutput: { permissionDecision: 'deny' },
}), '', 'Stop')
}), '')
expect(out.decision).toBe('deny')
})