gnoma/docs/essentials/decisions/004-posttooluse-hook-ordering.md

# ADR-004: PostToolUse Hook Ordering vs. Firewall Redaction

**Status:** Accepted
**Date:** 2026-05-19

## Context

The 2026-05-19 external security audit raised this concern as a "High" finding:

> Ablauf in `internal/engine/loop.go`:
>   tool executes → PostToolUse hook bekommt raw output → danach `Firewall.ScanToolResult()`
>
> Das heißt: Ein `type = "prompt"` Hook kann raw Tool-Output in ein LLM schicken,
> bevor Redaction passiert.

In source order, the relevant code is at `internal/engine/loop.go:699-712`:

```go
// PostToolUse hook: can transform result (Deny treated as Skip).
output := result.Output
if e.cfg.Hooks != nil {
    payload := hook.MarshalPostToolPayload(call.Name, args, output, result.Metadata)
    transformed, _, _ := e.cfg.Hooks.Fire(hook.PostToolUse, payload)
    if s := hook.ExtractTransformedOutput(transformed); s != "" {
        output = s
    }
}

// Scan tool result through firewall
if e.cfg.Firewall != nil {
    output = e.cfg.Firewall.ScanToolResult(output)
}
```

The audit's literal observation is correct: the hook receives `result.Output` before
`Firewall.ScanToolResult` runs. The audit's preferred fix was either:

- **Reorder:** scan first, then fire the hook with redacted input.
- **Split contract:** introduce `PostToolUseRawLocalOnly` (shell hooks only) and
  `PostToolUseRedactedForLLM` (prompt/agent hooks only) as distinct events.

Either fix is non-trivial. The reorder regresses legitimate shell-hook use cases
that need raw access (auditing, log forwarding, transformation). The split is a
contract change that requires migrating every existing hook config.

Before adopting either, we re-examined whether the leak path is actually open
after Wave 1 (SafeProvider boundary) merged.

### What Wave 1 changed

The audit was performed against `main(2).zip`, which did not include the
SafeProvider boundary. With Wave 1 (ADR-pending; PR #1 merged), every
`provider.Provider` registered with the router — and every provider handed to a
non-engine consumer (SLM classifier, summarizer, hook streamer) — is wrapped in
`security.SafeProvider`. The wrapper scans outgoing messages and the system
prompt through the firewall before delegating to the inner provider.

This affects PostToolUse hooks as follows, by hook type:

- **`type = "command"` (shell):** runs a local subprocess. No LLM round-trip.
  The hook sees raw output, but the output stays local. The audit's threat
  model (raw output → cloud LLM) doesn't apply.
- **`type = "prompt"`:** uses `hook.PromptExecutor`, which calls
  `p.streamer.Stream(...)`. In production, `streamer` is the
  `routerStreamer` adapter from `cmd/gnoma/main.go`, which delegates to
  `router.Router.Stream(...)`. The router selects an arm; each arm's `Provider`
  is a `*SafeProvider` after Wave 1. Outbound messages — including the
  rendered template containing `{{.Result}}` — are scanned at the SafeProvider
  boundary before the inner provider sees them.
- **`type = "agent"`:** uses `hook.AgentExecutor`, which spawns an elf via
  `elf.Manager`. The elf engine is constructed with `Firewall: m.firewall`, so
  the elf's `buildRequest()` scans inline. Same effect as the main engine's
  request path.

So the leak path — *raw tool output reaching an LLM* — is already closed
transitively by Wave 1 for both LLM-bound hook types. The audit's literal
observation about source ordering remains true; the practical leak it implied
does not.

### What's left

Two residual concerns survive the transitive guarantee:

1. **Output transformation produces unscannable downstream payloads.** A
   PostToolUse hook (any type) may transform raw output into a form that
   defeats regex-based scanning — e.g. base64-encoding, JSON-wrapping with
   reformulated content, summarisation via LLM that drops the secret-shaped
   string. The firewall scan at line 711 runs on the transformed payload,
   which may no longer match patterns the raw output would have. This is
   *user-configured behaviour* (they chose to add the hook), but it's worth
   acknowledging as a known limit.
2. **Transitive safety is non-obvious.** The "leak is closed" property
   depends on every LLM-bound hook path going through SafeProvider. A future
   change that adds a new hook type with its own LLM transport, or replaces
   the `routerStreamer` adapter with something that bypasses the router,
   would silently reopen the leak. There is no compile-time or test-time
   guarantee that this property holds.

## Decision

**Accept the current ordering. Document the transitive guarantee.**

Concretely:

1. **No reorder, no split.** The PostToolUse contract stays: hooks see raw
   `result.Output`; the firewall scan runs after hook transformation.
2. **Add a doc comment** to `hook.PostToolUse` in
   `internal/hook/event.go` and to the dispatcher call site in
   `internal/engine/loop.go` making the transitive guarantee explicit:
   *"PostToolUse hooks receive pre-scan output. LLM-bound paths
   (prompt/agent) inherit firewall scanning at the SafeProvider boundary;
   shell hooks stay local. Any new hook type that talks to an LLM
   must route through SafeProvider or perform its own scan."*
3. **Add a regression test** that asserts a `type = "prompt"` PostToolUse
   hook firing on a tool result containing a known secret-shaped string
   does not deliver that string verbatim to an inner provider. The test
   builds a recording fake provider, registers it via a router arm wrapped
   in SafeProvider, configures a prompt-type PostToolUse hook that
   includes `{{.Result}}` in its template, fires the hook, and asserts
   the recorded request was redacted.

### Why not reorder?

Reorder eliminates Concern 2 cheaply, but it regresses Concern 1's mirror
case: shell hooks that need raw output for legitimate local-only purposes.
Examples we want to preserve:

- Audit-log hooks that record exact bash output for later inspection.
- Forensic hooks that hash raw output before any transformation.
- Local-only alerting (e.g. fire pagerduty when a specific stderr pattern
  appears in a build hook) that needs the unredacted signal.

A redacted payload changes byte offsets, regex matches, and content size —
all of which break legitimate shell-hook code that's not LLM-bound.

### Why not split the contract?

A split (`PostToolUseRaw` vs. `PostToolUseRedacted`) is technically clean
but introduces a migration cost out of proportion to the residual risk.
Every existing hook config has to be re-categorised. The split also
encourages a footgun: a hook author who picks the wrong variant gets
either unsafe behaviour or broken transformation. The audit-flagged risk
doesn't justify that tax against an already-closed leak path.

### When to revisit

Re-open this decision if any of the following happen:

- A new hook type lands that performs LLM round-trips outside the router
  (e.g. a hook that calls a vendor SDK directly). At that point the
  transitive safety argument breaks and Position D (dispatcher-level
  redaction for LLM-bound paths) becomes necessary.
- The SafeProvider boundary is removed or relaxed (Wave 1 plan flagged
  this as a possible one-release follow-up; if engine-level scanning is
  removed AND a future change weakens SafeProvider, the transitive chain
  loses its second link).
- Telemetry shows that a meaningful number of users configure prompt-type
  PostToolUse hooks. The split contract's migration cost becomes
  proportional to the population it protects.
- A real-world incident proves Concern 1 is exploitable — e.g. a
  user-configured transformation that the firewall fails to scan.

## Alternatives Considered

### Alternative A: Reorder (scan before hook)

- **Pros:** Defense in depth at the hook boundary; eliminates the
  "transitive safety is non-obvious" concern.
- **Cons:** Breaks shell hooks that need raw output. Redacts content
  that may never leave the local machine.

### Alternative B: Split contract
(`PostToolUseRawLocalOnly` + `PostToolUseRedactedForLLM`)

- **Pros:** Each hook type sees exactly what it should. Compile-time
  clarity for hook authors.
- **Cons:** Migration cost for every existing config. New footgun
  (wrong variant chosen → wrong semantics). Two near-identical event
  types in the API surface.

### Alternative C: Dispatcher routes by command type

- **Pros:** Single event, transparent split. Hook authors don't pick.
- **Cons:** Dispatcher needs access to the firewall (new dependency).
  Per-handler redaction means firing the hook chain multiple times
  with different payloads, or pre-computing both raw and scanned
  variants. Increases dispatcher complexity meaningfully.

### Alternative D: Accept current ordering, document, test
(this ADR)

- **Pros:** No code reorder. No migration. Acknowledges the existing
  transitive guarantee from Wave 1. Closes the "non-obvious" concern
  via doc + regression test.
- **Cons:** Concern 1 (transformation defeats scanning) remains as a
  user-configured behaviour. Future hook types that bypass the router
  silently reopen the leak — the regression test catches one shape
  but not all shapes.

## Consequences

**Positive:**
- No churn to the hook contract or existing configs.
- Existing shell-hook use cases (audit, forensic, local alert) keep
  raw access.
- Regression test makes the transitive guarantee load-bearing in CI.

**Negative:**
- The transitive guarantee is documented but not type-enforced. A
  motivated change can still reopen the leak.
- Output-transforming hooks can produce unscannable downstream
  payloads. Same as today; no improvement.

**Neutral:**
- The audit finding is closed by reframing, not by code change.
  This ADR is the artifact that records why.

## Implementation

Three small changes land with this ADR:

1. `internal/hook/event.go` — extend the `PostToolUse` constant's
   doc comment with the transitive-guarantee statement.
2. `internal/engine/loop.go:699` — extend the inline comment above
   the hook fire to point at this ADR.
3. `internal/hook/posttooluse_redaction_test.go` (new) — regression
   test as described in the Decision section.

Total: ~80 LOC including the test.