09f131f78e
Deduplicate the parallel sweep from codex/exhaustive-simplification-rfcs (merged in the parent commit) against the twelve RFCs already here, on the merits per item: Ported (rewritten to this set's evidence standard): - fold-stdio-ui-helper — verified: dsh-stdio-agent is the only runtime importer; the fold resolves the documented product-app-depends-on- support-package tension. The false acp-agent analogy is replaced with the real distinction (protocol product surface vs one app's front-door scaffolding). - generate-rfc-index-tables — verified: the classification RFC records rejecting generation; the index tables are the repo's only systematically conflicting docs region. Process framing made timeless. Consolidated into existing owners: - web searchStatus()/fetchStatus()/WebCapabilityStatus (verified: zero production callers; the tool-web README and architecture.md claims are drift) → drop-web-providers-change-event, renamed drop-unconsumed-web-observation-surface. - hook/result.durationMs (unread, nondeterministic, normalizer-scrubbed) and the double-defaulted defaultTimeoutMs knob → tighten-hook-protocol-contract. - the exercised-but-unadvertised exec.arguments mutation path (a tool-bash integration shim rewrites through it) → a sanction-or-seal note in the pre-tool-input-rewrite proposal. - the dormant-guard critique of subagent depth machinery → recorded in prune-unimplemented-subagent-vocabulary as the considered-and-rejected alternative, with the keep sharpened (uncapped-today acknowledged; wiring the cap is the completion, not deletion). getProvider()/list() and lastAssistantMessage recorded as examined-and-kept (bash-revert precedent; observe-enrich recorded keep). Not ported (with reasons): - tools/change + system-prompt/change removal — recorded keeps in the adapter-change RFC, unengaged by the sweep; no new facts. - LlmService.models() — flagged by both surveys, but two lines with a plausible consumer: TODO-or-drive-by territory per the RFC bar, not a proposal. - SchemaProp.default RFC — already XXX(unused-default)-tagged; the RFC bar excludes TODO-tracked provisional cleanups. - PreToolDecision 'ask' removal — FIXME(permissions)-anchored deferral with the permission system as its named consumer. - Codex bridge Config.model, merged systemMessages — wire-faithful tested surface / README-documented deferral. Their in-code TODO notes (acp-test-harness, hook-snapshot-noise, catalog-type-links) and the stale hook-prompt-block name fixes ride the merge unchanged.
33 lines
6.7 KiB
Markdown
33 lines
6.7 KiB
Markdown
# RFC: Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics
|
|
|
|
Status: proposed
|
|
|
|
## Problem
|
|
|
|
Five pieces of the `dsh-hook-protocol`/bridge contract miss the discipline the [subagent-observe-enrich RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md) records — it dropped an `agentType` lifecycle field for lacking a consumer, and these fail the same test:
|
|
|
|
1. **`HookDialect`'s `'native'` variant** (`packages/hooks/hook-protocol/src/types.ts`) has zero producers — the bridges stamp `'claude'` and `'codex'`; the only `'native'` constructor anywhere is 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](../../implemented/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) is 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](../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` gets silent nothing with no warn.
|
|
3. **`hook/result.durationMs`** is durable timing telemetry with no reader. Both bridges write it, and the ACP snapshot normalizer scrubs it to `0` because wall-clock hook runtime is replay noise (`examples/acp-agent/tests/snapshot-normalize.ts`); the remaining consumers are tests and the goldens that exist because the field exists. 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` is 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 sets; the per-hook `timeoutSec` is the real timeout surface.
|
|
5. **The `hook/result` semantics live in the bridges, twice, not in the lib that owns the event.** `summarize()` — the 500-character stderr truncation rule — is byte-identical in `packages/hooks/hooks-claude/src/index.ts` and `packages/hooks/hooks-codex/src/index.ts`, and so is the decision-string rule `output.decision ?? (output.continue === false ? 'stop' : 'pass')`; yet `dsh-hook-protocol` declares `hook/result`, documents `stderrSummary` as "truncated" without owning the truncation, and documents the decision values without owning the mapping. If one bridge drifts (a different cap, a different fallback), the shared durable event's semantics fork silently.
|
|
|
|
## Proposal
|
|
|
|
Narrow `HookDialect` to `'claude' | 'codex'` and fix its JSDoc; retarget the lib's one `'native'` test. Drop `suppressOutput` from `HookOutput`, the codec's parse lines, its codec-test assertions, and the parsed-superset lists in the lib README and [hook-protocol-lib RFC](../../implemented/feature/2026-06-30-hook-protocol-lib.md) (amended per [implemented/AGENTS.md](../../implemented/AGENTS.md)). Drop `durationMs` from `HookResultRecord`, `RunHookResult`, the `hook/result` event, the bridge appends, the docs/catalog, and the snapshot normalizer's special-case scrub (retiring `runHook`'s injected clock if nothing else needs it); the hook goldens refresh mechanically as the scrubbed field disappears. Replace the bridges' `defaultTimeoutMs` config knob with one shared reference-default constant in `dsh-hook-protocol` (per-hook `timeoutSec` stays the override surface). Move the `hook/result` semantics into the lib: `appendHookResult` (or a helper it exposes) derives `stderrSummary` and the decision string from the `HookOutput` + exit outcome, and both bridges delete their private copies. Rider: un-export `BLOCKING_EXIT_CODE` (zero importers; even the codec tests spell the literal `2`).
|
|
|
|
## Why not keep them?
|
|
|
|
The [hook-protocol-lib RFC](../../implemented/feature/2026-06-30-hook-protocol-lib.md) deliberately records "parses the full CC superset" — the strongest counterargument is 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 only this RFC's amended references.
|
|
- `suppressOutput` and `durationMs` appear nowhere in source, parsed-field doc lists, the catalog, or the normalizer; the hook goldens are re-recorded or refreshed without the field.
|
|
- Both bridge configs lose `defaultTimeoutMs`; the reference default lives once, in the lib; per-hook `timeoutSec` still overrides it.
|
|
- One definition each of the truncation rule and the decision-string rule, in `dsh-hook-protocol`, 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 churns the hook goldens once (a mechanical refresh — the field was already normalized to a constant). The cost is 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.
|