Skip to content

fix(#6357): make PostToolUse sanitizers honor Claude Code's hook contract - #6468

Closed
waynesun09 wants to merge 4 commits into
mainfrom
fix-6357-posttool-hooks
Closed

fix(#6357): make PostToolUse sanitizers honor Claude Code's hook contract#6468
waynesun09 wants to merge 4 commits into
mainfrom
fix-6357-posttool-hooks

Conversation

@waynesun09

@waynesun09 waynesun09 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Summary

  • PostToolUse scripts read Claude Code's tool_response (fallback tool_result) and replace output via hookSpecificOutput.updatedToolOutput, preserving structured shapes such as Bash {stdout, stderr, …}.
  • A single posttool_chain.py driver on * applies suppress → unicode → redact → canary in-process. Claude Code runs matching hooks in parallel and does not merge two updatedToolOutput rewrites, so canary cannot be a separate PostToolUse matcher.
  • scan_text flattens every string field (including stderr); a leak only on stderr is no longer invisible. apply_text writes a suppress summary into stdout and blanks other text slots, or leaves unrecognized structured shapes unchanged.
  • Canary still exits 1 with decision: block and redacts the token in updatedToolOutput — Claude Code's PostToolUse block only appends a reason and still shows the original result.
  • Hook contract is v2 in docs/runtimes.md. Settings loading (Sandbox tool hooks are never loaded: settings.json is written to /sandbox/workspace/.claude but Claude Code runs from /sandbox/workspace/<repo> #6358) is already on main, so this contract is effective once this PR merges.

Closes #6357

Test plan

  • uvx pytest internal/security/hooks/ (140 passed)
  • go test ./internal/security/
  • CI green on this PR
  • One sandbox run with --debug hooks confirming tool_response in / updatedToolOutput out

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix PostToolUse hooks: honor Claude Code v2 contract with chained sanitizers

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Update PostToolUse hooks to read tool_response and emit updatedToolOutput with preserved
 shapes.
• Enforce sanitizer ordering via a single posttool_chain.py driver (Claude runs hooks in
 parallel).
• Extend canary post-tool to redact leaked tokens even when returning decision:block.
Diagram

graph TD
  A["Claude Code runtime"] --> B["Generated settings.json"] --> C["PostToolUse hook runner"]
  C --> D["posttool_chain.py"] --> E["hook_io.py"] --> F["Sanitizer libs (suppress/unicode/redact)"]
  C --> G["canary_posttool.py"] --> E
  D --> H[("findings.jsonl")]
  G --> H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Adapter-level sequential chaining (runtime enforces order)
  • ➕ Keeps individual sanitizer hooks as standalone executables
  • ➕ Centralizes ordering and plumbing in one runtime implementation
  • ➖ Does not work for Claude Code today because matching hooks run in parallel with no stdout piping
  • ➖ Requires every runtime adapter to correctly implement the invariant ordering
2. Merge sanitizers into one monolithic PostToolUse script (no dynamic imports)
  • ➕ Simpler runtime behavior (single file, no stage discovery)
  • ➕ Less complexity around module loading
  • ➖ Duplicates logic already maintained in existing sanitizer scripts
  • ➖ Harder to keep unit tests and behavior aligned with the standalone scripts
3. Implement PostToolUse sanitization in Go (remove Python for post-tool)
  • ➕ Single-language implementation, potentially easier to integrate with Go harness/runtime
  • ➕ Avoids Python import/exec behavior inside the sandbox
  • ➖ Rebuilds existing, tested Python logic (unicode + redact + suppress)
  • ➖ Higher migration risk and more surface area to re-validate across runtimes

Recommendation: Keep the PR’s approach: a single posttool_chain.py entrypoint plus a shared hook_io.py library. This matches Claude Code’s actual hook execution model (parallel hooks, no piped stdout) while preserving the existing sanitizer implementations as reusable libraries and maintaining the security invariant ordering (suppress → unicode → redact).

Files changed (15) +604 / -162

Enhancement (2) +275 / -0
hook_io.pyAdd shared PostToolUse v2 IO utilities (tool_response + updatedToolOutput) +126/-0

Add shared PostToolUse v2 IO utilities (tool_response + updatedToolOutput)

• Introduces a helper module to unify PostToolUse v2 handling: selecting payload fields, flattening scan text, applying text back into structured outputs, transforming nested strings, emitting updatedToolOutput, and canary redaction across all string fields.

internal/security/hooks/hook_io.py

posttool_chain.pyAdd a single PostToolUse sanitizer driver enforcing suppress→unicode→redact +149/-0

Add a single PostToolUse sanitizer driver enforcing suppress→unicode→redact

• Adds a new driver hook that loads enabled sanitizer siblings from disk and applies them sequentially in-process to enforce ordering despite Claude Code’s parallel hook execution. Emits v2 'hookSpecificOutput.updatedToolOutput' and aggregates metadata/findings from unicode and redaction stages.

internal/security/hooks/posttool_chain.py

Bug fix (5) +134 / -100
hooks.goEmbed hook_io + posttool_chain and schedule chain as the PostToolUse sanitizer +44/-26

Embed hook_io + posttool_chain and schedule chain as the PostToolUse sanitizer

• Adds embedded assets for 'hook_io.py' and 'posttool_chain.py', and changes HookPlan to schedule a single PostToolUse sanitizer driver for Bash/WebFetch/Read. HookFiles now ships the chain driver when any sanitizer is enabled and ships hook_io whenever PostToolUse protocol v2 is needed; introduces helpers to detect library-only scripts not referenced by HookPlan.

internal/security/hooks.go

canary_posttool.pyUse v2 PostToolUse payload and redact canary via updatedToolOutput +20/-13

Use v2 PostToolUse payload and redact canary via updatedToolOutput

• Switches input handling to 'tool_response' (fallback 'tool_result') via 'hook_io'. On canary detection, still blocks but also emits 'hookSpecificOutput.updatedToolOutput' with the token redacted to avoid leaving the leak in Claude Code context.

internal/security/hooks/canary_posttool.py

context_suppress_posttool.pyEmit updatedToolOutput for suppression (v2 contract) via hook_io +10/-8

Emit updatedToolOutput for suppression (v2 contract) via hook_io

• Updates the context suppression hook to read output from 'tool_response'/'tool_result' and write replacements using 'hookSpecificOutput.updatedToolOutput'. Uses 'hook_io' to preserve structured output shape while still emitting 'tool_result' as scan text.

internal/security/hooks/context_suppress_posttool.py

secret_redact_posttool.pyRedact secrets across structured outputs and emit updatedToolOutput (v2) +29/-24

Redact secrets across structured outputs and emit updatedToolOutput (v2)

• Moves from string-only tool_result handling to scanning/redacting every string field via 'hook_io.transform_strings'. Emits v2 updatedToolOutput and metadata about redacted secrets/patterns while keeping fail-open behavior for PostToolUse redaction errors.

internal/security/hooks/secret_redact_posttool.py

unicode_posttool.pyNormalize unicode in structured outputs and emit updatedToolOutput (v2) +31/-29

Normalize unicode in structured outputs and emit updatedToolOutput (v2)

• Updates the unicode sanitizer to operate over all string fields via 'hook_io.transform_strings', collecting findings and emitting updatedToolOutput with metadata. Preserves existing behavior of logging critical findings and failing open on scan errors.

internal/security/hooks/unicode_posttool.py

Tests (6) +181 / -54
canary_posttool_test.pyAdd tests for v2 tool_response input and structured Bash output redaction +35/-0

Add tests for v2 tool_response input and structured Bash output redaction

• Adds coverage verifying 'tool_response' triggers a block response that includes 'updatedToolOutput', and validates structured Bash payloads have stdout redacted while preserving object shape.

internal/security/hooks/canary_posttool_test.py

context_suppress_posttool_test.pySupport tool_response payloads and assert updatedToolOutput emission +10/-2

Support tool_response payloads and assert updatedToolOutput emission

• Extends test helpers to generate either 'tool_result' or 'tool_response' inputs. Adds assertions that v2 responses include 'hookSpecificOutput.updatedToolOutput' and that tool_response inputs work.

internal/security/hooks/context_suppress_posttool_test.py

posttool_chain_test.pyTest chain driver ordering, tool_result fallback, and structured output preservation +64/-12

Test chain driver ordering, tool_result fallback, and structured output preservation

• Refactors test helpers to read updatedToolOutput when present, adds tests for the chain driver path (tool_response primary, tool_result fallback), and verifies Bash-object tool_response outputs preserve shape while redacting secrets. Keeps a legacy sequential-chain test to validate stage compatibility.

internal/security/hooks/posttool_chain_test.py

secret_redact_posttool_test.pyAdd contract v2 test coverage for tool_response + updatedToolOutput +18/-0

Add contract v2 test coverage for tool_response + updatedToolOutput

• Adds a focused test asserting tool_response inputs are redacted and that updatedToolOutput is emitted consistently with tool_result for simple string outputs.

internal/security/hooks/secret_redact_posttool_test.py

unicode_posttool_test.pyAssert updatedToolOutput is present and support tool_response inputs +11/-0

Assert updatedToolOutput is present and support tool_response inputs

• Extends tests to verify v2 'hookSpecificOutput' fields are present and equal to the sanitized output for string payloads. Adds coverage for 'tool_response' inputs containing zero-width characters.

internal/security/hooks/unicode_posttool_test.py

hooks_test.goUpdate Go tests for chain driver scheduling and library-only hook files +43/-40

Update Go tests for chain driver scheduling and library-only hook files

• Adjusts GenerateClaudeSettings and HookPlan/HookFiles assertions to expect a single 'posttool_chain.py' PostToolUse sanitizer entry and additional shipped files ('hook_io.py', 'posttool_chain.py'). Updates coverage checks to treat sanitizer stages and hook_io as library files that are shipped but not directly scheduled.

internal/security/hooks_test.go

Documentation (2) +14 / -8
0090-runtime-neutral-sandbox-hooks-contract.mdMark ADR 0090 as done for PostToolUse contract v2 +6/-0

Mark ADR 0090 as done for PostToolUse contract v2

• Adds a Done note referencing #6357 and summarizes the v2 PostToolUse contract changes (tool_response + updatedToolOutput + posttool_chain ordering). Links to the updated runtimes contract documentation.

docs/ADRs/0090-runtime-neutral-sandbox-hooks-contract.md

runtimes.mdBump sandbox hook contract to v2 and document Claude Code behavior +8/-8

Bump sandbox hook contract to v2 and document Claude Code behavior

• Updates the security matrix to reflect which hooks are effective under Claude Code once loaded, and clarifies canary behavior under PostToolUse blocking. Rewrites the Sandbox hook contract section to v2, documenting 'tool_response' input, 'hookSpecificOutput.updatedToolOutput' output, and the 'posttool_chain.py' ordering workaround.

docs/runtimes.md

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 9:40 PM UTC · Ended 9:46 PM UTC

Commit: 2fae3b5 · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Canary misses stderr leaks ✓ Resolved 🐞 Bug ⛨ Security
Description
canary_posttool.py now detects canary leakage by scanning hook_io.scan_text(payload), but
scan_text() returns only the first matching dict field among (stdout, content, text, output) and
ignores stderr/other string fields. A Bash tool_response object leaking the canary in stderr (with
stdout clean/empty) will not be blocked or redacted, leaving the leaked token in context.
Code

internal/security/hooks/canary_posttool.py[R87-90]

+    original = hook_io.payload(hook_input)
+    text = hook_io.scan_text(original)
+    if canary.lower() not in text.lower():
+        sys.exit(0)
Relevance

●●● Strong

Accepted history favors fail-closed security fixes for scanner bypasses and structured-output
omissions.

PR-#1178
PR-#2085

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The canary hook’s new detection path depends on hook_io.scan_text(), and scan_text() explicitly only
returns the first matching field in dict outputs based on _TEXT_KEYS, which does not include stderr.
Therefore a canary present only in stderr will not be detected and the hook exits 0 without
redacting.

internal/security/hooks/canary_posttool.py[83-98]
internal/security/hooks/hook_io.py[22-48]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`canary_posttool.py` uses `hook_io.scan_text()` to decide whether the canary token is present. `scan_text()` only returns the first string field found in dict outputs (preferring `stdout`) and can miss canary occurrences in other fields like `stderr`, causing a canary leak to go undetected and unredacted.

## Issue Context
This regression is especially relevant for Claude Code’s structured Bash outputs (`{stdout, stderr, ...}`), where sensitive data can appear in `stderr`.

## Fix Focus Areas
- internal/security/hooks/hook_io.py[24-48]
- internal/security/hooks/canary_posttool.py[87-90]

## Implementation notes
- Update `hook_io.scan_text()` to truly *flatten* structured outputs for scanning by including **all** string leaves (at minimum: include both `stdout` and `stderr` for Bash objects).
 - Prefer a recursive collection (e.g., walk dict/list and concatenate string values with separators) over returning the first matching key.
 - Keep size bounded (respect `MAX_INPUT_CHARS`) to avoid pathological concatenations.
- Add a unit test proving the regression:
 - Bash `tool_response = {"stdout": "", "stderr": "...SECRET_CANARY...", ...}` must block and emit `updatedToolOutput` with stderr redacted.
- (Optional but recommended) consider whether `emit_updated()` / `emit_block()` should set `tool_result` to the concatenated flattened scan text as well, so sequential adapters don’t lose visibility of non-stdout fields.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 58 rules

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/security/hooks/canary_posttool.py
…ract

Read tool_response (fallback tool_result) and replace output via
hookSpecificOutput.updatedToolOutput, with suppress → unicode → redact
enforced in a single posttool_chain.py driver because Claude runs hooks
in parallel. Canary post-tool also redacts leaked tokens, since
decision:block does not hide the original result.

Signed-off-by: Wayne Sun <gsun@redhat.com>
Claude Code Bash payloads always have stdout, so first-key scan_text
missed stderr-only canary leaks. Two PostToolUse hooks also raced on
updatedToolOutput; the chain now owns suppress → unicode → redact →
canary in one process.

Assisted-by: Grok (fix), Claude (review), Gemini (review), Codex (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
FULLSEND_POSTTOOL_SKIP was a test-only env knob the agent could write
into workspace .env and disable the chain. Stages are gated by sibling
files only. Also type the chain metadata map and add Bash-object tests
for unicode and suppress.

Assisted-by: Grok (fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Site preview

Preview: https://7f863de0-site.fullsend-ai.workers.dev

Commit: 574bc1eab7536d92b05854b8d511cc1dceb81c6b

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:48 PM UTC · Completed 10:06 PM UTC

Commit: e3eb507 · View workflow run →

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [documentation inconsistency] internal/security/hooks.go:142 — The HookPlan comment states the chain ordering as "suppress → unicode → redact → canary" but the actual execution order in posttool_chain.py is unicode → canary-detect → suppress → redact → canary-block. The posttool_chain.py docstring, runtimes.md, and ADR annotation all correctly state "unicode → canary → suppress → redact". This could mislead a future adapter implementor into thinking suppress-first is correct.
    Remediation: Update the comment to read "unicode → canary → suppress → redact" to match the actual code and three other documentation sites.

Low

  • [fail-open gap] docs/runtimes.md — PostToolUseFailure is not wired in HookPlan. Failed tool calls bypass all sanitizers and canary detection. The PR documents this gap clearly (runtimes.md caveat 3, posttool_chain.py docstring) and labels it as a follow-up to PostToolUse sandbox hooks read tool_result but Claude Code sends tool_response — sanitizers are inert under Claude Code #6357. The old standalone canary_posttool.py was never effective under Claude Code (the bug this PR fixes), so the consolidation does not remove a working workaround.

  • [backward-incompatible] internal/security/hooks/hook_io.py — PostToolUse stdout now includes hookSpecificOutput alongside the existing tool_result field. The v1 tool_result is preserved for backward compatibility so existing adapters reading only that field are unaffected. Already documented in runtimes.md.

  • [breaking-api] internal/security/hooks.go:146 — HookPlan() now returns a single PostToolUse HookGroup with Tools=[*] and Scripts=[posttool_chain.py] instead of two groups. The Go type contract is unchanged but the semantics have shifted. Internal function; runtimes.md wiring section already documents this for adapter authors.

  • [edge-case] internal/security/hooks/posttool_chain.py:109 — Module-level _STAGE_CACHE dict is shared across invocations within the same process. Fine for production (one-shot subprocess invocations) but latent fragility for future test authors. Existing tests work around this.

  • [code-organization] internal/security/hooks.go:190postToolChainEnabled(hooks) guard is checked twice in HookFiles — once for posttool_chain.py and once for hook_io.py. Both are part of the same feature; the shared guard is correct.

  • [naming-convention] internal/security/hooks.go:36 — Exported variable HookIO breaks the *Hook naming pattern. Intentional since it is a library, not a hook script; the comment explains this.

  • [library-files-not-in-plan] internal/security/hooks.go:187 — HookFiles ships library files (hook_io.py, individual sanitizer scripts) not referenced by HookPlan. The hookLibraryFile() test helper codifies this distinction.

  • [provenance-warning] — Prior review context discarded: provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments.

Previous run

Review

Findings

Medium

  • [comment/code discrepancy] internal/security/hooks.go:141 — The HookPlan comment says "suppress → unicode → redact → canary must share one process" but the actual chain order in posttool_chain.py (and its docstring) is "unicode → canary → suppress → redact". Since the ordering is a security invariant (unicode normalization must precede canary/secret detection to prevent obfuscation bypass), a contributor reading the Go comment could incorrectly "fix" the Python code to match the wrong order.
    Remediation: Update the comment to read "unicode → canary → suppress → redact must share one process".

  • [incomplete-security-coverage] internal/security/hooks/posttool_chain.pyPostToolUseFailure is not wired in HookPlan. Claude Code fires PostToolUseFailure for failed tool calls (including non-zero-exit Bash), so failed output bypasses all post-tool sanitization including canary detection. The PR is transparent about this gap (documented in at least four places) and it is not a regression — under v1 these hooks were entirely inert for all calls. Still, a canary token leaked via a failing tool call (e.g. cat /etc/canary; exit 1) would go undetected. Worth tracking as a follow-up.
    Remediation: Wire PostToolUseFailure to posttool_chain.py in HookPlan, or file a tracked follow-up issue.

Low

  • [unnecessary construct] internal/security/hooks/posttool_chain.py:136except SystemExit: raise is unnecessary. In Python 3, SystemExit inherits from BaseException, not Exception, so the subsequent except json.JSONDecodeError and except Exception clauses will never catch it. The clause is harmless but adds noise.
    Remediation: Remove the except SystemExit: raise clause.

  • [code organization] internal/security/hooks.go:189postToolChainEnabled guard appears twice in HookFiles at non-adjacent locations (once for posttool_chain.py, once for hook_io.py). Every other feature in the function has a single guard block.
    Remediation: Consolidate into a single if block.

Previous run (2)

Review

Findings

High

  • [fail-open on security-critical path] internal/security/hooks/posttool_chain.py:155 — When canary_hit is True but an exception occurs during the block path (redact_canary, emit_block, or sys.exit(1)), the except Exception clause catches it, sets metadata['canary_error'] = True, and falls through to emit_updated + sys.exit(0). This silently degrades a canary leak detection from a blocking exit(1) to a non-blocking exit(0). If the exception occurs before redact_canary completes, updated still contains the un-redacted canary token, which is then emitted in updatedToolOutput. The standalone canary_posttool.py fails closed on errors (blocks with hard-coded error JSON); the chain version fails open.
    Remediation: After _stage_error(metadata, 'canary'), add a fallback block path — try to emit_block with a generic reason, and if even that fails, sys.exit(1) unconditionally. A canary hit that was detected must never silently degrade to allow.

Medium

  • [fail-open sanitizer bypass] internal/security/hooks/posttool_chain.py:72 — When canary detection itself throws during hook_io.scan_text(original).lower() (e.g., an unexpected type), canary_hit stays False and the chain continues without blocking. The standalone canary_posttool.py script fails closed on exceptions (blocks with hard-coded error JSON). The chain version fails open — a malformed tool output structure that causes scan_text to throw would silently bypass canary checking.
    Remediation: Consider setting canary_hit = True when the canary scan throws (fail-closed for canary), or emit a block. The standalone canary hook's fail-closed behavior should be preserved in the chain.

Low

  • [edge-case] internal/security/hooks/hook_io.py:87scan_text concatenates all string values from a dict without separators. This means canary detection (which uses scan_text) could match a canary that spans the boundary between two fields (e.g., stdout ends with 'ABC', stderr starts with 'DEF', canary is 'CDEF'). However, redact_canary uses transform_strings which processes each string field independently, so it would not find or redact the cross-boundary match. In practice this requires the canary token to coincidentally split across field boundaries, which is extremely unlikely for random canary tokens.

  • [test-inadequate] internal/security/hooks/posttool_chain_test.py — There is a test for redact-stage exception (fail-open, exit 0), but no equivalent test for canary-stage exception behavior. Adding a test for this path would verify whether the current behavior is intentional or a bug.

  • [code-organization] internal/security/hooks.gopostToolProtocolEnabled is a trivially identical wrapper over postToolChainEnabled — it returns the exact same boolean. Having two functions with different names that return the same value adds indirection with no semantic benefit today.

  • [docstring] internal/security/hooks/hook_io.pyemit_updated and emit_block lack docstrings, breaking the pattern set by other public functions in this module (payload, scan_text, has_text_slot, apply_text, looks_failed, transform_strings) which all have docstrings.

  • [provenance-warning] — Prior review context discarded: provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

Medium

  • [edge-case] internal/security/hooks/hook_io.py:39scan_text() returns only the first matching field from _TEXT_KEYS (e.g., stdout) and ignores other string fields (e.g., stderr). For canary_posttool.py, this creates a detection gap: if a canary token appears only in stderr of a structured Bash response {"stdout": "", "stderr": "CANARY_HERE", ...}, scan_text returns the empty stdout, the canary check passes, and the leak is missed. The old code used json.dumps(tool_result) which serialized all fields, though it was inert under Claude Code. redact_canary uses transform_strings (which processes all fields) for replacement, so the redaction path is correct — only the detection path is incomplete. Consider adding a scan_all_text() variant or having canary detection use json.dumps for full-object scanning.

Low

  • [test-inadequate] internal/security/hooks/canary_posttool_test.py:221 — No test covers the case where a canary appears only in stderr of a structured Bash response. Adding such a test would surface the detection gap above.

  • [test-inadequate] internal/security/hooks/posttool_chain_test.py:105 — No chain tests cover context-suppress or unicode sanitization with a structured Bash object tool_response. The existing test_bash_object_tool_response_preserves_shape only exercises the redact stage.

  • [sandbox-escape] internal/security/hooks/posttool_chain.py:34FULLSEND_POSTTOOL_SKIP reads from the environment to skip sanitizer stages. While documented as test-only, and stage_enabled() also gates on the script file's existence on disk, the env var is readable at runtime. If the sourced .env file is agent-writable, this could provide a bypass vector. Consider restricting to a compile-time test flag or moving it outside the agent-writable workspace tree.

  • [fail-open] internal/security/hooks/posttool_chain.py:60 — The consolidated chain catches all exceptions during input parsing and exits 0 (pass-through). This is consistent with the documented contract for sanitizing scripts but increases blast radius vs. separate scripts — a single parse failure now disables all three stages instead of just one.

  • [naming-convention] internal/security/hooks.go:35 — Embed variable HookIO breaks the <Name>Hook naming pattern used by all other embed vars (SSRFPreToolHook, SecretRedactPostToolHook, etc.). Since hook_io.py is a library rather than a hook, the deviation is intentional, but a comment clarifying its role would help.

  • [pattern-inconsistency] internal/security/hooks/posttool_chain.py:76metadata: dict = {} uses a bare dict type annotation while all other annotations in the new code use the parameterized form dict[str, Any].

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:47 PM UTC · Ended 10:49 PM UTC

Commit: e75a0d9 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:50 PM UTC · Completed 11:07 PM UTC

Commit: d2df43f · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:04 AM UTC · Completed 12:22 AM UTC

Commit: d3d954b · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review August 22, 2026 00:22

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Aug 22, 2026
@waynesun09 waynesun09 added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Aug 22, 2026
@waynesun09

Copy link
Copy Markdown
Member Author

Local sandbox validation — closes test plan item 3

Built this branch and ran the triage harness against a real sandbox to close out the last unchecked test-plan item (--debug=hooks run confirming tool_response in / updatedToolOutput out).

Setup: fullsend run triage on a real Podman + OpenShell sandbox, claude-opus-4-6, 78 turns / 72 tool calls. --no-post-script, so the target issue was left untouched.

Wiring

/sandbox/claude-config/hooks.json contains exactly one PostToolUse entry, on *:

"PostToolUse": [
  { "matcher": "*",
    "hooks": [{ "type": "command",
                "command": "python3 /sandbox/claude-config/hooks/posttool_chain.py" }] }
]

hook_io.py and the four stage scripts are uploaded alongside as libraries. Hook loading is independently confirmed in the same run by a PreToolUse tirith_check deny (pipe_to_interpreter).

1. updatedToolOutput is honored by Claude Code

Had the agent run printf "token ghp_FAKEtesttoken0000..." and report verbatim what it received:

The stdout shows that the token value was redacted by the system (displayed as ghp_...)

The model never saw the secret. The chain read tool_response and its updatedToolOutput replaced the tool result — the v2 contract working end to end under Claude Code.

2. Stage ordering blocks an obfuscated canary

Planted a canary token split with zero-width characters — invisible to the PreToolUse canary check, which only sees the raw command — and had the agent read it. From /sandbox/workspace/.security/findings.jsonl:

hook_posttool  secret_redact_posttool  github_pat   high      Redacted github_pat: ghp_...        redact
hook_posttool  unicode_posttool        zero_width   high      27 zero width character(s) removed  sanitize
hook_posttool  canary_posttool         canary_leak  critical  CANARY_LEAKED: ... in Bash result   block

Unicode normalization strips the separators first, then canary detection fires and blocks. The token appears nowhere in the emitted output, and the session halts via continue: false.

This is the ordering fix in d3d954b. On the previous commit the same input exits 0 with no block, and the unicode stage reassembles the clean token into updatedToolOutput — the leak the hook exists to prevent.

3. No-op path

Across the 72 tool calls of the normal triage run, the chain was invoked on every call and emitted nothing when there was nothing to change (empty stdout, exit 0). No spurious rewrites.

Not covered by this run

Claude Code fires PostToolUse only for tool calls that succeed; failed calls go to PostToolUseFailure, which HookPlan does not wire yet. Failed commands therefore still bypass suppression, unicode, redaction and canary. This is called out in docs/runtimes.md and the ADR annotation, and left as follow-up.

Canary detection ran on the raw payload before the unicode stage, so a
token split with zero-width or fullwidth characters evaded it and the
unicode stage then reassembled the clean token into updatedToolOutput.
Unicode now runs first and detection reads its result.

Detection and redaction share one case-insensitive matcher. Redaction
walked a str.lower() copy using indices from the original string; a
character that case-folds to more code points than it started with
desynchronized the two and could leave a detected token in the output.

The canary path fails closed throughout: a scan that raises counts as a
hit, output that cannot be verified free of the token is withheld, exit 1
is unconditional, and input the driver cannot read (malformed JSON,
oversized) blocks rather than skipping detection - the chain is the only
PostToolUse entry point Claude Code schedules. A leak also sets
continue: false, the documented field that halts the session.

Also: scan_text joins fields on a newline so a match cannot span a
boundary the redactors rewrite independently; unicode normalization skips
identifier fields so a rewritten path is never reported back to Claude;
stage exceptions are guarded per field and recorded in findings.jsonl;
and a stage error alone no longer emits a no-op rewrite that could
clobber another hook's.

docs/runtimes.md now records that PostToolUse covers successful tool
calls only - Claude Code routes failures to PostToolUseFailure, which is
not wired yet.

Assisted-by: Claude (fix, review), Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:20 AM UTC · Completed 1:36 AM UTC

Commit: 574bc1e · View workflow run →

@waynesun09

Copy link
Copy Markdown
Member Author

Merged into #6467 at 9780c6c (merge commit, conflicts resolved only in docs/runtimes.md) so the v2 PostToolUse contract and the pi runtime land together; the pi hook adapter already speaks tool_response / updatedToolOutput and its tests pass against posttool_chain.py. #6467's body carries "closes #6357". This PR can be closed once #6467 lands.

@waynesun09
waynesun09 removed the request for review from maruiz93 August 22, 2026 18:49
@waynesun09 waynesun09 closed this Aug 22, 2026
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 7:00 PM UTC · Completed 7:20 PM UTC

Commit: 574bc1e · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6468 — PostToolUse sanitizer hook contract fix

Workflow shape: Triage → 2× Code (both failed) → human-authored PR → 4× Review (COMMENT → REQUEST_CHANGES → COMMENT → COMMENT) → closed (folded into #6467).

Agents repo: fullsend-ai/agents@ee288907 (discovered from workflow run logs).

Timeline

  1. Triage (run 32203712194, Aug 19): Rated issue PostToolUse sandbox hooks read tool_result but Claude Code sends tool_response — sanitizers are inert under Claude Code #6357 severity-high — four PostToolUse security hooks silently inert under Claude Code.
  2. Code run 1 (run 32523934680, Aug 21 20:30): /fs-code — FAILED. Agent wrote code and committed, but post-code.sh pre-commit check found 6 ruff errors (SIM102, E501×5) and ruff format failures. Auto-fix resolved all but the SIM102 (not auto-fixable).
  3. Code run 2 (run 32526610854, Aug 21 21:02): /fs-code fix, pay attention on python SIM102... — FAILED. Agent fixed SIM102 but introduced new errors: F841 (unused variable), E501, and ty check type errors (error[unsupported-operator]). Auto-fix couldn't resolve F841 or ty errors.
  4. Human author (waynesun09) manually authored PR fix(#6357): make PostToolUse sanitizers honor Claude Code's hook contract #6468 at 21:38.
  5. Review cycle 1 (run 32530041820, commit e3eb507): COMMENT — found medium scan_text gap (stderr canaries missed), low sandbox-escape via FULLSEND_POSTTOOL_SKIP env var.
  6. Review cycle 2 (run 32534497373, commit d2df43f): REQUEST_CHANGES — caught HIGH fail-open canary bypass (exception during block path degrades to exit(0) with un-redacted token) and MEDIUM fail-open scan bypass (exception leaves canary_hit=False). Both genuine security vulnerabilities.
  7. Author fixes (commits 574bc1e): Comprehensive fail-closed rewrite. Author also independently discovered unicode case-folding desynchronization in redact_canary and canary-ordering bypass (unicode normalization reassembling evaded tokens) — issues the review agent did not flag.
  8. Review cycles 3–4: COMMENT — only documentary/advisory items remained. Correct de-escalation.
  9. PR closed (Aug 22): Folded into PR feat(#6464): add the pi runtime (stream parser, Bootstrap/Run, Vertex provider, enablement) #6467 so v2 PostToolUse contract and pi runtime land together.

Assessment

Review agent: strong performance. The cycle-2 HIGH fail-open finding was the most valuable agent contribution — a real, exploitable security vulnerability where a detected canary token could escape un-redacted. The escalation arc (COMMENT → REQUEST_CHANGES → COMMENT) was correctly calibrated. The review agent outperformed Qodo's review bot, which only found the same medium-severity scan_text gap.

Code agent: two wasted runs (~70 min compute). Both failures trace to two root causes: (1) pre-commit cannot install tools inside the network-isolated sandbox, so the agent cannot self-verify with ruff or ty, and (2) AGENTS.md contains no Python lint/type-check guidance, making ty check undiscoverable. The whack-a-mole pattern in run 2 (fixed SIM102, introduced F841 + ty errors) is characteristic of blind-to-linter code generation.

Evidence for existing issues (not filing duplicates)

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PostToolUse sandbox hooks read tool_result but Claude Code sends tool_response — sanitizers are inert under Claude Code

1 participant