restore hook/result durationMs — review keeps wall-clock audit timing durable

Reverses item 3 of the tighten-hook-protocol-contract RFC per review:
a persistence log is written for future readers, and hook wall-clock
runtime is audit signal (which hook made a turn slow). runHook keeps
its injected now clock and RunHookResult wrapper, the bridges pass the
measured duration through HookResultRecord, the snapshot normalizer
keeps its replay scrub, and the hook fixtures carry the field again.
The RFC records the reversal; the other three prunes stand.
This commit is contained in:
Tianyi Cui
2026-07-04 22:00:12 +08:00
parent 19ae955009
commit 0b97b2f7c1
27 changed files with 122 additions and 85 deletions

View File

@@ -228,8 +228,8 @@ A plugin may declaration-merge extra `SessionEventMap` types. These are **log-on
| 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`); `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? }` | The decided outcome, paired by `handlerId`. `decision` is the neutral outcome `appendHookResult` derives from the parsed output (the hook's decision, else `'stop'` on `continue:false`, else `'pass'`); `exitCode` absent when the hook could not run; `stderrSummary` the trimmed stderr truncated to 500 chars (the block-reason source on exit 2). |
| `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).

View File

@@ -4,29 +4,28 @@ Status: implemented (proposed and accepted 2026-07-04)
## Problem
Five pieces of the `dsh-hook-protocol`/bridge contract missed the discipline the [subagent-observe-enrich RFC](../feature/2026-06-30-subagent-observe-enrich.md) records — it dropped an `agentType` lifecycle field for lacking a consumer, and these failed the same test:
Four pieces of the `dsh-hook-protocol`/bridge contract missed the discipline the [subagent-observe-enrich RFC](../feature/2026-06-30-subagent-observe-enrich.md) records — it dropped an `agentType` lifecycle field for lacking a consumer, and these failed the same test:
1. **`HookDialect`'s `'native'` variant** (`packages/hooks/hook-protocol/src/types.ts`) had zero producers — the bridges stamp `'claude'` and `'codex'`; the only `'native'` constructor anywhere was the lib's own unit test. The field's own JSDoc defines `dialect` as "the bridge that ran it", and native is not a bridge: the [interception-seams RFC](../feature/2026-06-30-interception-seams.md) records that native hooks are not a package and that "a native plugin can already use the typed Decisions" without the durable hook log, and the flagship native-plugin worked example asserts exactly that (no `hook/*` events at all).
2. **`HookOutput.suppressOutput`** (same file) was parsed by the codec and discarded on every path: no bridge branch, no merge fold, no warn, no deferred-list row — uniquely among its parsed-but-unhonored siblings, each of which carries a stated deferral (`updatedInput` → a logged warn plus the [pre-tool-input-rewrite proposal](../../proposed/feature/2026-06-30-pre-tool-input-rewrite.md); `systemMessage` → a logged warn plus a README deferred row; `continue`/`stopReason` → a `TODO(hook-continue-false)` anchor plus the `'stop'` decision record). Structurally there is nothing to suppress: hook stdout never enters any transcript (context flows only via `additionalContext`; the log records only `decision`/`stderrSummary`), so a hook author setting `suppressOutput: true` got silent nothing with no warn.
3. **`hook/result.durationMs`** was durable timing telemetry with no reader. Both bridges wrote it, and the ACP snapshot normalizer scrubbed it to `0` because wall-clock hook runtime is replay noise (`examples/acp-agent/tests/snapshot-normalize.ts`); the remaining consumers were tests and the goldens that existed because the field existed. Deterministic provenance fields (`point`, `matcher`, `turn`, `handlerId`) earn their durability as audit facts; a nondeterministic field that replay must erase and nothing reads earns neither its bytes nor its special-case scrub.
4. **`defaultTimeoutMs` was double-defaulted in both bridge configs** — a schema `.default(600_000)` AND a `?? 600_000` fallback (`packages/hooks/hooks-claude/src/index.ts`, `packages/hooks/hooks-codex/src/index.ts`) — the same two-homes-for-one-literal shape the ACP bridge's `TODO(double-default)` flags, for a knob no shipped config set; the per-hook `timeoutSec` is the real timeout surface.
5. **The `hook/result` semantics lived in the bridges, twice, not in the lib that owns the event.** `summarize()` — the 500-character stderr truncation rule — was byte-identical in `packages/hooks/hooks-claude/src/index.ts` and `packages/hooks/hooks-codex/src/index.ts`, and so was the decision-string rule `output.decision ?? (output.continue === false ? 'stop' : 'pass')`; yet `dsh-hook-protocol` declared `hook/result`, documented `stderrSummary` as "truncated" without owning the truncation, and documented the decision values without owning the mapping. If one bridge drifted (a different cap, a different fallback), the shared durable event's semantics would fork silently.
3. **`defaultTimeoutMs` was double-defaulted in both bridge configs with a floating literal** — a schema `.default(600_000)` AND a `?? 600_000` fallback (`packages/hooks/hooks-claude/src/index.ts`, `packages/hooks/hooks-codex/src/index.ts`), two homes per bridge for one protocol-level constant, so the bridges could silently drift apart on the shared default. *The proposal's original remedy — delete the knob outright — was overtaken by the no-hardcoded-tunables audit, which kept the knob as the explicit bridge-owned config (and added `stderrSummaryMaxChars` beside it); what remained to fix was the literal's home.*
4. **The `hook/result` semantics lived in the bridges, twice, not in the lib that owns the event.** `summarize()` — the stderr truncation rule — was byte-identical in `packages/hooks/hooks-claude/src/index.ts` and `packages/hooks/hooks-codex/src/index.ts`, and so was the decision-string rule `output.decision ?? (output.continue === false ? 'stop' : 'pass')`; yet `dsh-hook-protocol` declared `hook/result`, documented `stderrSummary` as "truncated" without owning the truncation, and documented the decision values without owning the mapping. If one bridge drifted (a different cap, a different fallback), the shared durable event's semantics would fork silently.
## What shipped
`HookDialect` is `'claude' | 'codex'`, its JSDoc names the two bridges, and the lib's unit test constructs a `'codex'` invocation. `suppressOutput` is gone from `HookOutput`, the codec's parse, the codec tests, and the parsed-superset lists in the lib README and the [hook-protocol-lib RFC](../feature/2026-06-30-hook-protocol-lib.md) (amended per [implemented/AGENTS.md](../AGENTS.md)). `durationMs` is gone from the `hook/result` event, the bridge appends, the docs, and the snapshot normalizer's special-case scrub; with no duration to measure, `runHook`'s injected `now` clock and its single-purpose `RunHookResult` wrapper went too — `runHook` returns the `HookOutput` directly. The committed hook fixtures (`session.jsonl`, which double as the expected-log goldens) had the field stripped mechanically; the stdout goldens never carried it. The bridges' `defaultTimeoutMs` config knob is replaced by one reference-default constant, `DEFAULT_HOOK_TIMEOUT_MS` (600 000 ms), exported from the lib's runner and applied inside `runHook`; `RunHookOptions` lost the field entirely, and the per-hook `timeoutSec` stays the override surface. The `hook/result` semantics live in the lib: `HookResultRecord` carries the decoded `HookOutput`, and `appendHookResult` derives `stderrSummary` (500-character truncation) and the decision string from it; both bridges deleted their private copies, and the derived values are byte-identical to what the bridges wrote (the goldens prove it — their only diff is the dropped `durationMs`). Rider: `BLOCKING_EXIT_CODE` is a codec-internal const, no longer exported (it had zero importers; even the codec tests spell the literal `2`).
`HookDialect` is `'claude' | 'codex'`, its JSDoc names the two bridges, and the lib's unit test constructs a `'codex'` invocation. `suppressOutput` is gone from `HookOutput`, the codec's parse, the codec tests, and the parsed-superset lists in the lib README and the [hook-protocol-lib RFC](../feature/2026-06-30-hook-protocol-lib.md) (amended per [implemented/AGENTS.md](../AGENTS.md)). `hook/result.durationMs` stays: review judged wall-clock hook runtime worth its bytes as durable audit timing (which hook made a turn slow), so `runHook` keeps its injected `now` clock and `RunHookResult` wrapper, the bridges keep passing the measured duration through `HookResultRecord`, and the snapshot normalizer keeps scrubbing the one nondeterministic field to `0` for replay. On the tunables, the no-hardcoded-tunables audit set the shape this change keeps: `defaultTimeoutMs` and `stderrSummaryMaxChars` stay explicit bridge configs, and `RunHookOptions.defaultTimeoutMs` stays a required parameter the bridge passes in. What this change adds is one home per literal: the reference defaults live in the lib as `DEFAULT_HOOK_TIMEOUT_MS` (600 000 ms, exported from the runner) and `DEFAULT_STDERR_SUMMARY_MAX_CHARS` (500, exported from the events module), and both bridges' schema defaults and `??` fallbacks read those constants instead of restating the numbers. The `hook/result` semantics live in the lib: `HookResultRecord` carries the decoded `HookOutput` plus the bridge's `stderrSummaryMaxChars`, and `appendHookResult` derives `stderrSummary` (via the exported `summarizeStderr(stderr, maxChars)`) and the decision string from them; both bridges deleted their private copies, and the derived values are byte-identical to what the bridges wrote (the goldens prove it — their only diff is the dropped `durationMs`). Rider: `BLOCKING_EXIT_CODE` is a codec-internal const, no longer exported (it had zero importers; even the codec tests spell the literal `2`).
## Why not keep them?
The [hook-protocol-lib RFC](../feature/2026-06-30-hook-protocol-lib.md) deliberately recorded "parses the full CC superset" — the strongest counterargument was that this proposal re-litigates decisions that RFC records. But parsing a field whose value can never influence anything is not protocol faithfulness, it is a reader trap; a dialect variant that the design's own thesis says will never be stamped is vocabulary without an interpreter; and durable telemetry that replay must scrub is a cost with no buyer. Each returns trivially with its first real consumer (a transcript surface with hook stdout to suppress; a native-provenance feature that logs hook events; a trace viewer that reads timings — as live diagnostics or a deliberately durable telemetry event designed for it). On item 5, the lib RFC chose per-bridge explicitness over a parameterized engine — but that choice governed payload construction and Decision mapping; the semantics of the SHARED durable event are precisely the "primitives where duplication would actually be dangerous" that the same RFC assigns to the lib.
The [hook-protocol-lib RFC](../feature/2026-06-30-hook-protocol-lib.md) deliberately recorded "parses the full CC superset" — the strongest counterargument was that this proposal re-litigates decisions that RFC records. But parsing a field whose value can never influence anything is not protocol faithfulness, it is a reader trap; a dialect variant that the design's own thesis says will never be stamped is vocabulary without an interpreter; Each returns trivially with its first real consumer (a transcript surface with hook stdout to suppress; a native-provenance feature that logs hook events). On `durationMs` the review reached the opposite verdict: a persistence log is written for future readers, and wall-clock hook timing is audit signal worth carrying before a reader exists — so it stays, with replay normalization as the accepted cost. On item 4, the lib RFC chose per-bridge explicitness over a parameterized engine — but that choice governed payload construction and Decision mapping; the semantics of the SHARED durable event are precisely the "primitives where duplication would actually be dangerous" that the same RFC assigns to the lib.
## Acceptance criteria
- `HookDialect` is two-valued; `rg "'native'"` in the hooks packages returns nothing.
- `suppressOutput` and `durationMs` appear nowhere in source, parsed-field doc lists, or the normalizer; the hook fixtures carry no `durationMs` (the refresh was a mechanical field-strip, not a re-record).
- Both bridge configs lost `defaultTimeoutMs`; the reference default lives once, in the lib (`DEFAULT_HOOK_TIMEOUT_MS`); per-hook `timeoutSec` still overrides it.
- `suppressOutput` appears nowhere in source, parsed-field doc lists, or the normalizer; `durationMs` stays on `hook/result` (and in the fixtures), with the normalizer's replay scrub intact.
- Both bridge configs keep `defaultTimeoutMs`/`stderrSummaryMaxChars` (the audit's explicit-tunables shape), but the literals `600_000` and `500` each live once, in the lib's `DEFAULT_HOOK_TIMEOUT_MS`/`DEFAULT_STDERR_SUMMARY_MAX_CHARS`; per-hook `timeoutSec` still overrides the timeout.
- One definition each of the truncation rule and the decision-string rule, in `dsh-hook-protocol`'s `appendHookResult`, exercised by both bridges' suites.
## Risks
The `dialect`, `suppressOutput`, `defaultTimeoutMs`, and semantics changes are invisible on the wire and in the goldens; the `durationMs` removal churned the hook fixtures once (a mechanical field-strip — the field was already normalized to a constant). The cost was churn in `dsh-hook-protocol` and both bridges — cheap under the pre-release stance, and cheaper than letting two copies of a durable event's semantics age apart.
The `dialect`, `suppressOutput`, tunables, and semantics changes are invisible on the wire and in the goldens. The cost was churn in `dsh-hook-protocol` and both bridges — cheap under the pre-release stance, and cheaper than letting two copies of a durable event's semantics age apart.

View File

@@ -27,7 +27,7 @@ Thirteen scenarios under `examples/acp-agent/tests/snapshots/`, naming `hook-<di
- **Authored, no model turn** (keyless, no sidecar — the derived replay script is empty; the `rejected` turn carrying `hook/*` events is compared): `hook-cc-promptsubmit-block`, `hook-codex-promptsubmit-block`.
- **Recorded against the real API, hook active during recording** (the model's reaction to the decision is part of the captured transcript, replayed keyless thereafter): `hook-{cc,codex}-promptsubmit-context` (allow + additionalContext fold), `hook-cc-pretool-deny` / `hook-codex-pretool-block` (deny → `isError` tool result), `hook-cc-pretool-ask` (ask → degrades to deny with the approval-required reason), `hook-{cc,codex}-posttool-block` (block with feedback), `hook-{cc,codex}-posttool-context` (accept + additionalContext), `hook-{cc,codex}-stop-continue` (a blocking Stop hook forces one extra step via steering).
Each hook command emits only FIXED LITERAL strings (no timestamps/pids/`$RANDOM`/cwd echoes), and a `hook/result` carries only deterministic fields (`decision`/`exitCode`/`stderrSummary`), so the hook events need no scrubbing beyond the normalizer's generic `time` zeroing. The `Stop` scenarios self-limit with a marker file (`.stop_fired`) so the force-continue does not loop — the `stop_hook_active` loop-guard is still a bridge `TODO`, so an unconditional Stop hook would force-continue every step.
Each hook command emits only FIXED LITERAL strings (no timestamps/pids/`$RANDOM`/cwd echoes); the snapshot normalizer scrubs the one volatile field a `hook/result` carries (`durationMs`). The `Stop` scenarios self-limit with a marker file (`.stop_fired`) so the force-continue does not loop — the `stop_hook_active` loop-guard is still a bridge `TODO`, so an unconditional Stop hook would force-continue every step.
### Three hook points are deliberately NOT snapshotted