refactor(hooks): tighten the hook-protocol contract surface

Implement the tighten-hook-protocol-contract RFC (moved to implemented/):

- HookDialect narrows to 'claude' | 'codex': the 'native' variant had zero
  producers (native plugins on the seams write no hook/* provenance), and the
  dialect is defined as the bridge that ran the hook.
- HookOutput.suppressOutput is gone: the codec parsed it and every path
  discarded it with no warn and no deferral — hook stdout never enters a
  transcript, so there is nothing to suppress.
- hook/result.durationMs is gone: durable timing telemetry with no reader
  that the snapshot normalizer had to scrub as replay noise. With no duration
  to measure, runHook loses its injected now clock and the single-field
  RunHookResult wrapper — it returns the HookOutput directly. The committed
  hook fixtures had the field stripped mechanically (field-only diff); the
  stdout goldens never carried it.
- The bridges' double-defaulted defaultTimeoutMs config knob is replaced by
  one reference-default constant, DEFAULT_HOOK_TIMEOUT_MS, exported from the
  lib's runner and applied inside runHook; per-hook timeoutSec stays the
  override surface.
- The hook/result semantics move into the lib that declares the event:
  HookResultRecord now carries the decoded HookOutput and appendHookResult
  derives the decision string (decision ?? stop-on-continue:false ?? pass)
  and the 500-char stderrSummary truncation; both bridges delete their
  byte-identical private copies. The snapshot suite passes against the
  existing goldens, proving the derived values are unchanged.
- Rider: BLOCKING_EXIT_CODE is codec-internal again (zero importers).

Amend the hook-protocol-lib and hook-snapshot-matrix RFCs to the new facts,
update the lib/bridge READMEs and the session.md event tables, and retarget
the affected unit tests (including new lib-level coverage of the derivation
rules).
This commit is contained in:
Tianyi Cui
2026-07-04 15:44:26 +08:00
parent 226a8b5e4c
commit cd49670f4e
36 files changed
+233 -235

No files matched your search

@@ -0,0 +1,32 @@
# RFC: Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics
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:
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.
## 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`).
## 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.
## 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.
- 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.