mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
fix(hooks): address Codex review — tighten codec to the reference schemas, preserve stdout
Codex's PR-E review found three protocol-fidelity blockers + two doc gaps, all
verified against ~/repos/refs:
- (A) Top-level `decision` accepted allow/deny/ask, but both reference schemas
reserve those for hookSpecificOutput.permissionDecision — the legacy top-level
decision is approve/block ONLY. Split topLevelDecisionOf (approve/block) from
permissionDecisionOf (allow/deny/ask), so an out-of-band {"decision":"deny"} is
now invalid and ignored instead of becoming a real blocking decision.
- (A) hookSpecificOutput was parsed without its hookEventName discriminator.
HookOutput now surfaces hookEventName so a bridge can discard a block whose
claimed event doesn't match the firing one (the schemas key the block by event).
- (A) runHook discarded raw stdout. HookOutput now carries `stdout` (trimmed,
verbatim) so a bridge can reproduce CC's plain-stdout rendering / Codex's
plain-stdout-as-additionalContext behavior.
- (B) hook/* SessionEventMap variants were only named in prose; added a payload/role
table to core-data-structures/session.md (a maintained catalog surface).
- (B) Removed PR-stack-position references (PR-F / "future bridge packages") from a
test comment and the RFC, per the current-state-wording rule.
New codec tests: top-level allow/deny/ask invalid+ignored, hookEventName capture,
raw stdout preserved on plain + JSON + empty stdout. 51 tests, per-file 100%.
This commit is contained in:
@@ -211,6 +211,17 @@ interface TurnEndReasonMap {
|
||||
|
||||
Every session event lives **inside** a turn (between a `turn/start` and its `turn/end`). The loop appends queued `user/message` events *after* `turn/start`, and an idle `agent.inject()` wraps its `context/message` in a one-shot `injection` turn. This makes the turn the single durability/replay boundary: a backend can treat anything after the last `turn/end` as an interrupted-crash tail without risking the loss of legitimately-recorded between-turn context. The `dsh-invariants` plugin enforces it in dev (a message event outside an open turn throws). See [the turn-enclosure invariant RFC](../rfc/implemented/architecture/2026-06-15-turn-enclosure-invariant.md).
|
||||
|
||||
## Plugin-contributed log-only events
|
||||
|
||||
A plugin may declaration-merge extra `SessionEventMap` types. These are **log-only**: NOT `SurfaceEventType`s (they carry no `surfaceOp` and contribute nothing to derived history), but, like every event, they must sit inside an open turn. The compaction seam's `compact/*` are documented on [compaction.md](compaction.md); the hook bridges' `hook/*` provenance (from `@deepseek-ai/dsh-hook-protocol`) are:
|
||||
|
||||
| Event | Payload | Role |
|
||||
|---|---|---|
|
||||
| `hook/invoked` | `{ turn, point, dialect, matcher?, handlerId }` | A hook command was invoked at a hook `point` (`PreToolUse`, `Stop`, …). `dialect` is the bridge (`claude`/`codex`/`native`); `matcher` the matcher-group pattern that selected it (absent for match-all); `handlerId` correlates with the result. |
|
||||
| `hook/result` | `{ turn, point, handlerId, decision, exitCode?, stderrSummary?, durationMs }` | The decided outcome, paired by `handlerId`. `decision` is the resolved neutral outcome (`deny`/`allow`/`block`/`stop`/`pass`/…); `exitCode` absent when the hook could not run; `stderrSummary` the truncated block-reason source. |
|
||||
|
||||
The mid-turn hook points (`PreToolUse`/`PostToolUse`/`UserPromptSubmit`/`Stop`) fire inside the loop's open turn, so their `hook/*` records are turn-enclosed by construction. `SessionStart` gets no `hook/*` record — its injected `context/message` is the durable evidence — because it has no open turn to enclose one (see the hooks RFC).
|
||||
|
||||
## Durability contract
|
||||
|
||||
What a persistence backend relies on: the durable log persists every event verbatim, **including** `assistant/chunk` — `seq` must stay contiguous, so chunks cannot be filtered out of the canonical log. All `event.data` must be JSON-serializable; `Session.append` enforces this at the source (throwing on non-serializable data), so a bad event never enters the log and `session.events` always equals what a backend can persist. Adding an event type that carries non-serializable data, or that breaks the turn/step nesting the invariants plugin checks, is a breaking change to the on-disk format.
|
||||
|
||||
@@ -12,7 +12,7 @@ This RFC introduces `@deepseek-ai/dsh-hook-protocol`, a **library** (not a plugi
|
||||
|
||||
## Decision
|
||||
|
||||
A new `packages/hooks/` group with `hook-protocol` as a pure library. It owns four primitive families and the `hook/*` session events; each bridge (PR-F) owns what genuinely differs.
|
||||
A new `packages/hooks/` group with `hook-protocol` as a pure library. It owns four primitive families and the `hook/*` session events; each bridge plugin (`dsh-hooks-claude`, `dsh-hooks-codex`) owns what genuinely differs.
|
||||
|
||||
**Shared (here):**
|
||||
- **Matcher** — `matchesMatcher(pattern, query, mode)`. The ONE axis the dialects differ on is collapsed to the `mode` parameter: `claude` treats a pure `[A-Za-z0-9_|]+` pattern as a literal (pipe = exact-match alternation) and anything else as a regex; `codex` is always an unanchored regex. Match-all on absent/`''`/`'*'`; an invalid regex matches nothing (never throws into the loop).
|
||||
@@ -21,7 +21,7 @@ A new `packages/hooks/` group with `hook-protocol` as a pure library. It owns fo
|
||||
- **Merge** — `mergeHookOutputs(outputs)`, folding multiple matched hooks into one most-restrictive `MergedHookOutcome`: permission precedence **deny > ask > allow**, halt sticky on the first `continue:false`, block reasons joined `\n\n`, context/system-messages accumulated in order.
|
||||
- **`hook/*` session events** — `hook/invoked` / `hook/result`, declaration-merged into `SessionEventMap` (log-only, like `compact/*` — NOT `SurfaceEventType`s), with `appendHookInvoked`/`appendHookResult` helpers so the invoked/result pairing and turn-enclosure stay consistent across bridges.
|
||||
|
||||
**Per-dialect (the bridges, PR-F):** building each event's stdin payload (CC's base+per-event field sets vs Codex's snake_case with `turn_id`/`model` extras), the dialect's env + `${CLAUDE_PLUGIN_ROOT}` substitution (CC) vs none (Codex), and mapping the neutral `HookOutput`/`MergedHookOutcome` onto the harness's seam-specific typed Decisions (`PreToolDecision`, `PromptDecision`, `ContinuationDecision`, `PostToolDecision`).
|
||||
**Per-dialect (the bridge plugins):** building each event's stdin payload (CC's base+per-event field sets vs Codex's snake_case with `turn_id`/`model` extras), the dialect's env + `${CLAUDE_PLUGIN_ROOT}` substitution (CC) vs none (Codex), and mapping the neutral `HookOutput`/`MergedHookOutcome` onto the harness's seam-specific typed Decisions (`PreToolDecision`, `PromptDecision`, `ContinuationDecision`, `PostToolDecision`).
|
||||
|
||||
### Why "shared core + per-dialect adapters", not "one parameterized engine"
|
||||
|
||||
@@ -29,4 +29,4 @@ A single engine parameterized by a full `dialect` descriptor was considered and
|
||||
|
||||
## Consequences
|
||||
|
||||
The two bridges (PR-F) become thin: parse the config file, pick a matcher mode, build the per-event payload+env, call `runHook` + `mergeHookOutputs`, map the outcome to a Decision, and append `hook/*`. The protocol's correctness-critical halves (matcher semantics, exit-code contract, merge precedence) live in one tested place — `hook-protocol` ships with heavy unit tests (matcher per-mode, codec per exit-code/field, runner plumbing with a stub executor, merge precedence, the `hook/*` helpers) at per-file 100%. Input rewrite (`updatedInput`) is parsed but not honored (the deferred [pre-tool-input-rewrite RFC](../../proposed/feature/2026-06-30-pre-tool-input-rewrite.md)); a bridge logs+warns on it. The package is a library, so it has no `cordis.yml` load path of its own — its real-load-path coverage comes through the bridge plugins that consume it (PR-F).
|
||||
The two bridge plugins become thin: parse the config file, pick a matcher mode, build the per-event payload+env, call `runHook` + `mergeHookOutputs`, map the outcome to a Decision, and append `hook/*`. The protocol's correctness-critical halves (matcher semantics, exit-code contract, merge precedence) live in one tested place — `hook-protocol` ships with heavy unit tests (matcher per-mode, codec per exit-code/field, runner plumbing with a stub executor, merge precedence, the `hook/*` helpers) at per-file 100%. Input rewrite (`updatedInput`) is parsed but not honored (the deferred [pre-tool-input-rewrite RFC](../../proposed/feature/2026-06-30-pre-tool-input-rewrite.md)); a bridge logs+warns on it. The package is a library, so it has no `cordis.yml` load path of its own — its real-load-path coverage comes through the bridge plugins that consume it.
|
||||
|
||||
@@ -42,14 +42,19 @@ function obj(value: unknown): Record<string, unknown> | undefined {
|
||||
: undefined
|
||||
}
|
||||
|
||||
/** Normalize a raw `decision`/`permissionDecision` string to the neutral enum. */
|
||||
function decisionOf(value: string | undefined): HookOutput['decision'] {
|
||||
switch (value) {
|
||||
case 'approve': case 'allow': case 'block': case 'deny': case 'ask':
|
||||
return value
|
||||
default:
|
||||
return undefined
|
||||
}
|
||||
/**
|
||||
* The legacy TOP-LEVEL `decision` is only `approve`/`block` in both reference
|
||||
* schemas — `allow`/`deny`/`ask` are reserved for `hookSpecificOutput.
|
||||
* permissionDecision`. So an out-of-band `{"decision":"deny"}` is invalid and
|
||||
* ignored here (it must not become a real blocking decision).
|
||||
*/
|
||||
function topLevelDecisionOf(value: string | undefined): HookOutput['decision'] {
|
||||
return value === 'approve' || value === 'block' ? value : undefined
|
||||
}
|
||||
|
||||
/** A `hookSpecificOutput.permissionDecision` is `allow`/`deny`/`ask` only. */
|
||||
function permissionDecisionOf(value: string | undefined): HookOutput['decision'] {
|
||||
return value === 'allow' || value === 'deny' || value === 'ask' ? value : undefined
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -62,7 +67,11 @@ function decisionOf(value: string | undefined): HookOutput['decision'] {
|
||||
*/
|
||||
export function parseHookOutput(exitCode: number | undefined, stdout: string, stderr: string): HookOutput {
|
||||
const trimmedErr = stderr.trim()
|
||||
const output: HookOutput = { exitCode, stderr: trimmedErr }
|
||||
const trimmedOut = stdout.trim()
|
||||
// Keep the raw stdout verbatim: a clean-exit hook may emit PLAIN text the
|
||||
// protocol renders/uses (CC output; Codex SessionStart/UserPromptSubmit
|
||||
// additionalContext), so the bridge needs it even when there's no JSON.
|
||||
const output: HookOutput = { exitCode, stderr: trimmedErr, stdout: trimmedOut }
|
||||
|
||||
// Exit 2 is a blocking error in both dialects: stderr is the reason. Surface
|
||||
// it as a `block` decision so the bridge maps it uniformly with a structured
|
||||
@@ -76,7 +85,6 @@ export function parseHookOutput(exitCode: number | undefined, stdout: string, st
|
||||
// the stderr channel is authoritative. A non-zero/undefined exit other than 2
|
||||
// carries no decision (the bridge records it as a non-blocking error).
|
||||
if (exitCode === 0) {
|
||||
const trimmedOut = stdout.trim()
|
||||
// Only attempt JSON when stdout looks like a JSON object — matches the
|
||||
// reference engines, which treat other stdout as plain text, not an error.
|
||||
if (trimmedOut.startsWith('{')) {
|
||||
@@ -106,18 +114,23 @@ function applyStructured(output: HookOutput, parsed: Record<string, unknown>): v
|
||||
const sysMsg = str(parsed, 'systemMessage')
|
||||
if (sysMsg !== undefined) output.systemMessage = sysMsg
|
||||
|
||||
// Top-level legacy `decision` + `reason` (CC approve/block; Codex block).
|
||||
const topDecision = decisionOf(str(parsed, 'decision'))
|
||||
// Top-level legacy `decision` (approve/block ONLY — allow/deny/ask there are
|
||||
// invalid per both schemas) + its `reason`.
|
||||
const topDecision = topLevelDecisionOf(str(parsed, 'decision'))
|
||||
if (topDecision !== undefined) output.decision = topDecision
|
||||
const topReason = str(parsed, 'reason')
|
||||
if (topReason !== undefined) output.reason = topReason
|
||||
|
||||
// hookSpecificOutput: the per-event channel. permissionDecision (allow/deny/
|
||||
// ask) OVERRIDES the legacy top-level decision when present; additionalContext
|
||||
// and updatedInput live here too.
|
||||
// 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
|
||||
// permissionDecision (allow/deny/ask) OVERRIDES the legacy top-level decision;
|
||||
// additionalContext and updatedInput live here too.
|
||||
const hso = obj(parsed.hookSpecificOutput)
|
||||
if (hso) {
|
||||
const permission = decisionOf(str(hso, 'permissionDecision'))
|
||||
const eventName = str(hso, 'hookEventName')
|
||||
if (eventName !== undefined) output.hookEventName = eventName
|
||||
const permission = permissionDecisionOf(str(hso, 'permissionDecision'))
|
||||
if (permission !== undefined) output.decision = permission
|
||||
const permissionReason = str(hso, 'permissionDecisionReason')
|
||||
if (permissionReason !== undefined) output.reason = permissionReason
|
||||
|
||||
@@ -100,6 +100,14 @@ export interface HookOutput {
|
||||
exitCode: number | undefined
|
||||
/** Trimmed stderr — the block-reason source on a blocking (exit 2) hook. */
|
||||
stderr: string
|
||||
/**
|
||||
* Trimmed stdout, verbatim. On a clean exit a hook may emit PLAIN (non-JSON)
|
||||
* stdout that the protocol renders as output (CC) or treats as
|
||||
* `additionalContext` (Codex SessionStart/UserPromptSubmit) — so the bridge
|
||||
* needs the raw text, not just the parsed structured fields. Empty string when
|
||||
* the hook produced no stdout.
|
||||
*/
|
||||
stdout: string
|
||||
/**
|
||||
* `false` ⇒ the hook asked to halt (CC/Codex `continue:false`); pairs with
|
||||
* {@link stopReason}. `true`/absent ⇒ proceed.
|
||||
@@ -110,14 +118,26 @@ export interface HookOutput {
|
||||
/** Hide the hook's stdout from the transcript (CC `suppressOutput`). */
|
||||
suppressOutput?: boolean
|
||||
/**
|
||||
* The blocking decision a hook expressed via structured stdout (CC's
|
||||
* `decision` / `hookSpecificOutput.permissionDecision`): `'block'`/`'deny'`
|
||||
* forbid the action, `'approve'`/`'allow'` permit it, `'ask'` requests
|
||||
* confirmation. Absent ⇒ no explicit decision (exit code governs).
|
||||
* The neutral blocking decision a hook expressed, folded from the two channels
|
||||
* the reference protocols keep DISTINCT: the legacy top-level `decision`
|
||||
* (`approve`/`block` only) and `hookSpecificOutput.permissionDecision`
|
||||
* (`allow`/`deny`/`ask`). We normalize them to one enum — `'block'`/`'deny'`
|
||||
* forbid, `'approve'`/`'allow'` permit, `'ask'` requests confirmation — but
|
||||
* `'allow'`/`'deny'`/`'ask'` arise ONLY from a `permissionDecision`, never from
|
||||
* a top-level `decision` (an out-of-band `{"decision":"deny"}` is invalid and
|
||||
* ignored, matching the schemas). Absent ⇒ no explicit decision (exit code governs).
|
||||
*/
|
||||
decision?: 'approve' | 'allow' | 'block' | 'deny' | 'ask'
|
||||
/** The reason/explanation accompanying {@link decision}. */
|
||||
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
|
||||
* when the hook emitted no `hookSpecificOutput`.
|
||||
*/
|
||||
hookEventName?: string
|
||||
/** Extra context to inject for the next model request (CC `additionalContext`). */
|
||||
additionalContext?: string
|
||||
/** A warning surfaced to the user (CC `systemMessage`). */
|
||||
|
||||
@@ -48,11 +48,25 @@ describe('parseHookOutput — structured stdout (exit 0 only)', () => {
|
||||
expect(out.systemMessage).toBe('heads up')
|
||||
})
|
||||
|
||||
it('parses legacy top-level decision + reason (approve/block)', () => {
|
||||
it('parses legacy top-level decision + reason (approve/block ONLY)', () => {
|
||||
expect(parseHookOutput(0, JSON.stringify({ decision: 'block', reason: 'nope' }), '').decision).toBe('block')
|
||||
expect(parseHookOutput(0, JSON.stringify({ decision: 'approve' }), '').decision).toBe('approve')
|
||||
})
|
||||
|
||||
it('a top-level decision of allow/deny/ask is INVALID and ignored (reserved for permissionDecision)', () => {
|
||||
// Both reference schemas restrict the legacy top-level `decision` to
|
||||
// approve/block; allow/deny/ask must come from hookSpecificOutput.permissionDecision.
|
||||
expect(parseHookOutput(0, JSON.stringify({ decision: 'deny' }), '').decision).toBeUndefined()
|
||||
expect(parseHookOutput(0, JSON.stringify({ decision: 'allow' }), '').decision).toBeUndefined()
|
||||
expect(parseHookOutput(0, JSON.stringify({ decision: 'ask' }), '').decision).toBeUndefined()
|
||||
})
|
||||
|
||||
it('captures hookEventName from hookSpecificOutput (the discriminator a bridge validates)', () => {
|
||||
const out = parseHookOutput(0, JSON.stringify({ hookSpecificOutput: { hookEventName: 'PreToolUse', permissionDecision: 'deny' } }), '')
|
||||
expect(out.hookEventName).toBe('PreToolUse')
|
||||
expect(out.decision).toBe('deny')
|
||||
})
|
||||
|
||||
it('hookSpecificOutput.permissionDecision OVERRIDES the legacy top-level decision', () => {
|
||||
const out = parseHookOutput(0, JSON.stringify({
|
||||
decision: 'approve',
|
||||
@@ -89,6 +103,20 @@ describe('parseHookOutput — structured stdout (exit 0 only)', () => {
|
||||
const out = parseHookOutput(0, 'just some text output', '')
|
||||
expect(out.decision).toBeUndefined()
|
||||
expect(out.continue).toBeUndefined()
|
||||
// The raw stdout is preserved verbatim so the bridge can render/use it
|
||||
// (CC output; Codex additionalContext) — trimmed.
|
||||
expect(out.stdout).toBe('just some text output')
|
||||
})
|
||||
|
||||
it('preserves raw stdout (trimmed) alongside parsed structured fields', () => {
|
||||
const json = JSON.stringify({ decision: 'block' })
|
||||
const out = parseHookOutput(0, ` ${json} \n`, '')
|
||||
expect(out.stdout).toBe(json)
|
||||
expect(out.decision).toBe('block')
|
||||
})
|
||||
|
||||
it('stdout is empty string when the hook emits none', () => {
|
||||
expect(parseHookOutput(0, '', '').stdout).toBe('')
|
||||
})
|
||||
|
||||
it('a JSON array stdout parses but yields no fields (not an object)', () => {
|
||||
|
||||
@@ -3,7 +3,7 @@ import { mergeHookOutputs } from '@deepseek-ai/dsh-hook-protocol'
|
||||
import type { HookOutput } from '@deepseek-ai/dsh-hook-protocol'
|
||||
|
||||
function out(over: Partial<HookOutput> = {}): HookOutput {
|
||||
return { exitCode: 0, stderr: '', ...over }
|
||||
return { exitCode: 0, stderr: '', stdout: '', ...over }
|
||||
}
|
||||
|
||||
describe('mergeHookOutputs — permission precedence deny > ask > allow', () => {
|
||||
|
||||
@@ -6,8 +6,8 @@ import { runHook } from '@deepseek-ai/dsh-hook-protocol'
|
||||
* A minimal stand-in for the bits of {@link BashExecutor} that {@link runHook}
|
||||
* actually calls (`resolve` then `run`). `runHook` is pure plumbing over those
|
||||
* two methods, so a duck-typed recorder is the right test seam — the REAL
|
||||
* executor (dsh-bash-local) is exercised end-to-end by the bridge e2e tests in
|
||||
* PR-F, not here.
|
||||
* executor (dsh-bash-local) is exercised end-to-end by the hook-bridge plugins
|
||||
* that consume this library, not here.
|
||||
*/
|
||||
function recordingBash(run: (spec: BashExecSpec) => Promise<BashRunResult>): {
|
||||
bash: BashExecutor
|
||||
|
||||
Reference in New Issue
Block a user