Skip to content

fix(#6920): withhold unsafe codex tool results - #6952

Merged
waynesun09 merged 4 commits into
mainfrom
codex-security-hooks
Sep 22, 2026
Merged

waynesun09 merged 4 commits into
mainfrom
codex-security-hooks

Conversation

@waynesun09

@waynesun09 waynesun09 commented Sep 3, 2026 •

Copy link
Copy Markdown
Member

Summary

  • verify the shared SSRF pre-tool hook blocks loopback requests through the Codex adapter
  • withhold Codex tool results when the shared post-tool chain reports secrets or unsafe Unicode
  • allow context-only suppression and terminal-escape cleanup to pass unchanged; before suppressed output passes, run the original through the full chain again with suppression disabled
  • before each post-tool chain run, check the whole hooks directory the way the run guard does: every entry a regular file with its runner-held digest, nothing else present (a rewritten stage, a disabled stage's file, or a hook_io/ package shadowing hook_io.py would otherwise change what the chain runs); hashing refuses FIFOs, symlinks and oversized files so it cannot stall
  • keep every hook script run inside codex's 30 s handler timeout: each run gets only what is left of the budget, and a run with under a second left is withheld instead of started and killed (a hook codex kills does not block)
  • fail closed on malformed Unicode category metadata and document the runtime behavior

Accepted ADR edit: adds an implementation note to ADR 0100 recording that post-tool rewrites the runtime cannot apply now block and withhold the result, where the Decision text describes an additionalContext warning. The note sits next to the Decision rather than rewriting it, per docs/contributing/adrs.md.

Test plan

  • pre-commit run --files <changed files>
  • go test ./internal/runtime ./internal/security -count=1
  • the shared hook suite: pytest internal/security/hooks (656 passed)
  • reproduced before fixing, each now withheld: a redactor rewritten to find nothing, and a planted hook_io/ package, each let a plain API key through (exit 0); a FIFO in place of a stage hung the adapter until it was killed

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Withhold unsafe Codex tool results

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Withhold Codex results when sanitizers detect secrets or unsafe Unicode.
• Pass benign context suppression and terminal-escape rewrites without warnings.
• Verify real SSRF and sanitizer chains through Codex adapter tests.
Diagram

graph TD
  A["Codex Tool"] --> B["Hook Adapter"] --> C["Post-tool Chain"] --> D{"Rewrite class"}
  D -->|Secret or unsafe Unicode| E["Withhold Result"]
  D -->|Context or escapes| F["Pass Original"]
  D -->|Unclassified| G["Warn Model"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Block every sanitizer rewrite
  • ➕ Provides simple fail-closed behavior
  • ➕ Avoids relying on sanitizer metadata classification
  • ➖ Withholds safe output for context compaction and terminal formatting
  • ➖ Could unnecessarily disrupt normal Codex workflows
2. Add an explicit rewrite disposition
  • ➕ Creates a clear contract between sanitizers and runtime adapters
  • ➕ Avoids inferring security impact from individual metadata fields
  • ➖ Requires coordinated changes across the shared hook protocol and runtimes
  • ➖ Expands the scope and regression surface of this targeted fix

Recommendation: Use the PR's metadata-based classification for this targeted security fix: it closes the exposure without penalizing benign rewrites and fails closed for malformed Unicode metadata. An explicit security-disposition field would be a cleaner long-term protocol if more sanitizer categories are introduced.

Files changed (4) +247 / -34

Bug fix (1) +58 / -5
fullsend-codex-hook.pyClassify and withhold security-sensitive sanitizer rewrites +58/-5

Classify and withhold security-sensitive sanitizer rewrites

• Adds metadata-based classification for secret and Unicode rewrites, blocking results that Codex cannot safely sanitize. Context suppression and terminal-escape-only cleanup pass silently, malformed Unicode metadata fails closed, and unknown rewrite types continue to warn the model.

internal/runtime/codex_hook/fullsend-codex-hook.py

Tests (1) +175 / -20
codex_hook_test.goCover Codex SSRF and post-tool withholding paths +175/-20

Cover Codex SSRF and post-tool withholding paths

• Adds an embedded-hook test helper and integration coverage using the real SSRF and post-tool chains. Tests verify withholding for secrets, hidden Unicode, mixed normalization findings, and malformed metadata while preserving benign cleanup and context suppression behavior.

internal/runtime/codex_hook_test.go

Documentation (2) +14 / -9
runtime-implementation.mdDocument Codex sanitizer classification and withholding behavior +8/-4

Document Codex sanitizer classification and withholding behavior

• Updates the runtime implementation guide and capability matrix to explain that Codex withholds secret and unsafe-Unicode results. It also documents unchanged handling for context suppression and terminal escapes, plus warnings for unclassified rewrites.

docs/contributing/runtime-implementation.md

codex.mdClarify user-visible Codex security hook behavior +6/-5

Clarify user-visible Codex security hook behavior

• Explains that Codex cannot edit built-in tool output, so unsafe results are withheld while benign cleanup rewrites are skipped. Artifact redaction behavior remains documented separately.

docs/runtimes/codex.md

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:10 AM UTC · Ended 10:17 AM UTC

Commit: 32528d3 · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. context_suppressed can leak secrets ✓ Resolved 📘 Rule violation ⛨ Security
Description
Context suppression runs before secret redaction, so successful test output containing a secret can
be summarized before the redactor scans it. The adapter then treats the rewrite as benign and allows
Codex to receive the original unredacted output, contradicting the documented runtime security
behavior.
Code

internal/runtime/codex_hook/fullsend-codex-hook.py[R478-479]

+            if benign_rewrite(verdict["output"]):
+                continue
Relevance

●●● Strong

Allowing context suppression before secret redaction can expose original secrets, violating the
documented Codex security contract.

PR-#6926

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2889480 requires runtime backend hook behavior to follow the documented security contract. The
chain at posttool_chain.py[376-433] suppresses updated before redaction scans it; the newly
changed adapter at fullsend-codex-hook.py[463-480] then permits any context_suppressed rewrite,
while the runtime guide claims secret-bearing results are withheld.

Rule 2889480: Consult runtime implementation guide when modifying runtime.Runtime backends
internal/runtime/codex_hook/fullsend-codex-hook.py[463-480]
internal/security/hooks/posttool_chain.py[376-433]
docs/contributing/runtime-implementation.md[107-109]

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

## Issue description
Codex can receive an original tool result containing secrets when context suppression removes the secret before the redaction stage scans the rewritten value.

## Issue Context
The post-tool chain performs context suppression before secret redaction. Because Codex cannot apply the summary rewrite, classifying `context_suppressed` as benign is safe only after the original result has independently passed security scanning.

## Fix Focus Areas
- internal/runtime/codex_hook/fullsend-codex-hook.py[463-480]
- internal/security/hooks/posttool_chain.py[376-433]
- internal/runtime/codex_hook_test.go[368-386]

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


2. Benign Unicode rewrites blocked ✓ Resolved 🐞 Bug ≡ Correctness
Description
security_rewrite treats every category except ANSI/OSC as removed security content, but
fullwidth is detection-only and deliberately remains unchanged. A context-suppression or ordinary
terminal-cleanup rewrite containing a fullwidth character is therefore incorrectly blocked and
withheld from Codex.
Code

internal/runtime/codex_hook/fullsend-codex-hook.py[393]

+    return any(category not in {"ansi_escape", "osc_escape"} for category in categories)
Relevance

●●● Strong

Detection-only fullwidth findings should not be blocked; this conflicts with the adapter’s benign
Unicode classification intent.

PR-#6926

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The Unicode scanner explicitly retains compatibility/fullwidth characters, except when NFKC
reconstructs an escape. The chain nevertheless copies every finding name—including detection-only
fullwidth—into metadata.categories; the new adapter then classifies any category outside
ANSI/OSC as security-sensitive, so unrelated benign rewrites are blocked.

internal/runtime/codex_hook/fullsend-codex-hook.py[377-412]
internal/security/hooks/unicode_posttool.py[169-211]
internal/security/hooks/posttool_chain.py[314-353]
internal/security/hooks/posttool_chain.py[376-391]

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

## Issue description
The Codex adapter classifies all Unicode metadata categories as rewrite causes, although `fullwidth` is only detected and retained. Consequently, a benign context-suppression or direct terminal-escape rewrite can be withheld merely because its input also contained a compatibility character.

## Issue Context
The distinction must preserve blocking for escapes reconstructed through NFKC while allowing independent ANSI/OSC cleanup and context suppression. The existing category list cannot distinguish a directly removed escape from an NFKC-reassembled escape.

## Fix Focus Areas
- internal/runtime/codex_hook/fullsend-codex-hook.py[377-412]
- internal/security/hooks/posttool_chain.py[314-353]
- internal/security/hooks/unicode_posttool.py[169-211]
- internal/runtime/codex_hook_test.go[285-386]

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


Grey Divider

Context sources
✅ Compliance rules (platform): 65 rules
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Site preview

Preview: https://790d11cf-site.fullsend-ai.workers.dev

Commit: a05954539022505bb2711ec6ef12ab73c2a1bb76

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:18 AM UTC · Ended 10:24 AM UTC

Commit: 6722d79 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:25 AM UTC · Ended 10:31 AM UTC

Commit: b495cf5 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:32 AM UTC · Completed 11:11 AM UTC

Commit: 1e7ac66 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.32

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Score holds at 2 (moderate), consistent with the prior review: Tier 1 metadata signals are unchanged (7 files, 778 lines, no protected paths, no dependency/CI changes, non-first-time author), Tier 2 shows continued elevated churn/fix-revert/coupling concentrated in the widely-shared docs/contributing/runtime-implementation.md rather than the PRs own hook logic, and Tier 3s single security-relevant label and no-flag behavior change are offset by the PRs narrow, well-scoped nature relative to the large tracking issue.

Previous run

Risk Assessment: moderate (2/5)

Details

Score holds at 2 (moderate), consistent with the prior review: Tier 1 signals are unchanged (7 files, no protected paths, no dependency changes, non-first-time author) aside from correcting the security-sensitive path miss for internal/security/hooks/; elevated Tier 2 churn/fix-revert/coupling on the heavily-trafficked runtime-implementation.md and codex hook files is counterbalanced by a moderate test ratio, no CI/dependency changes, and a loosely-coupled Tier 3 issue link, keeping the weighted composite at ~2.4, which rounds to the same moderate level as the prior assessment.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Medium-sized follow-up fix to the codex runtime hook with moderate test coverage ratio; high fix/revert churn and change coupling in Tier 2 are counterbalanced by no protected or security-sensitive paths, no dependency changes, and an experienced member author, yielding a moderate overall risk.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Review

Findings

Low

  • [adr-fidelity] docs/ADRs/0100-codex-sandbox-hooks.md:48 — The added "Implementation note" records that security-sensitive and unclassified rewrites now block-and-withhold, which is no longer consistent with the Decision paragraph a few lines above ("a rewrite the runtime cannot apply becomes an additionalContext warning") — the adapter has removed the additionalContext-warning emission path entirely. docs/contributing/adrs.md permits short notes on Accepted ADRs and requires PR-body disclosure of the edit (both satisfied here), but a reader comparing the Decision text to the note will see two contradictory descriptions of the same translation rule with no formal supersession.
    Remediation: Optional — a short follow-up ADR, or an explicit "supersedes this part of the Decision" sentence in the note, would make clear which description is current. Not required if maintainers are comfortable treating this as a minor annotation under the current adrs.md wording.

The prior review's two medium fail-open findings (rescan timeout budgeting; integrity verification by name-presence only) and two low style findings on codex_hook_test.go (missing t.Helper(); missing a withheld stderr assertion) are not observed in the current diff — the adapter now caps/withholds on insufficient handler budget and re-hashes every digest-map file before each chain spawn, and the test file includes both previously-missing elements.

Previous run

Review

Findings

Medium

  • [fail-open] internal/runtime/codex_hook/fullsend-codex-hook.py:83 — The new suppress-rescan path runs posttool_chain.py a second time, but SCRIPT_TIMEOUT_S=25 cannot fit two runs under the hooks.json handler timeout of 30s. A single run_script exceeding 25s is translated to a block, but a first pass in ~5–25s plus a rescan that crosses the 30s handler deadline is not covered. The adapter documents that a Failed/timed-out codex hook does not block, so the original tool result would be delivered unfiltered — the exact suppress-then-secret case this PR is meant to withhold.
    Remediation: Make two chain runs fit the fail-open ceiling, or fail closed when they will not. Options: (1) set SCRIPT_TIMEOUT_S so 2*SCRIPT_TIMEOUT_S plus margin is < 30s; (2) raise the codex PostToolUse adapter handler timeout to at least 2*SCRIPT_TIMEOUT_S+margin; (3) withhold (exit 2) if a suppress rescan is required and remaining handler budget is insufficient, rather than starting a second spawn that codex will kill.

  • [fail-open] internal/runtime/codex_hook/fullsend-codex-hook.py:506 — Allowing a suppressed result through is gated on hook_is_installed('secret_redact_posttool.py'), whose docstring claims integrity verification but only tests that the name appears in FULLSEND_CODEX_HOOK_DIGESTS — it does not re-hash the file. The invoked script (posttool_chain.py) is re-verified via verify_script_digest on both passes, but the siblings it importlib-loads (unicode_posttool.py, secret_redact_posttool.py, canary_posttool.py, context_suppress_posttool.py) are never independently re-hashed at call time. The adapter already documents that Run's directory guard runs once per iteration and the agent can rewrite scripts before the next tool call — name-in-map is not equivalent proof for a sibling the adapter never re-hashes; a no-op sibling would let the rescan pass and deliver the original with no additionalContext warning (a regression vs. the previous warn-and-pass behavior).
    Remediation: Before allowing a suppress pass-through, call verify_script_digest on every sibling the chain will import for the safety proof (at least secret_redact_posttool.py; unicode_posttool.py and canary_posttool.py if present in the digest map) and withhold on a missing file or digest mismatch. Do not treat name-in-map as integrity verification.

  • [edge-case] internal/runtime/codex_hook/fullsend-codex-hook.py:507 — The suppress-rescan allow path treats posttool_chain.py plus secret_redact_posttool.py's name in the digest map as proof the original is safe, but posttool_chain.py skips any sibling HookFiles omitted, and unicode_posttool.py is independently disableable. With unicode omitted, an ANSI-split openai_key secret in suppressible go-test output is condensed on the first pass (so secrets_redacted is never set); the rescan then loads no unicode stage, and secret_redact_posttool.py's pattern stops at the ESC character, so rescan_changed stays false. benign_rewrite then accepts the context-suppressed-only first-pass metadata and the adapter forwards the original unredacted. Claude/pi apply the suppress rewrite and hide the secret in this configuration; codex deliberately forwards the original, so the rescan gate must require the stages that make that original safe. TestCodexAdapter_PostToolUseRescanFindsObfuscatedSecrets always installs unicode_posttool.py, so this configuration is untested.
    Remediation: Fail closed on context suppression unless unicode_posttool.py is also in the digest map (same hook_is_installed check as secret_redact_posttool.py). Add a harness test that installs the chain without unicode_posttool.py and asserts an ANSI-split secret in go-test output is withheld.

Low

  • [pattern-inconsistency] internal/runtime/codex_hook_test.go:67 — installPostToolChain() does not call h.t.Helper(), unlike sibling codexAdapterHarness methods (script, embeddedScript, run) which invoke h.t.Helper() so test runner failures point to the test call site.
    Remediation: Add h.t.Helper() at the beginning of installPostToolChain().

  • [pattern-inconsistency] internal/runtime/codex_hook_test.go:361 — TestCodexAdapter_PostToolUseWithholdsUnclassifiedUnicodeRewrite asserts exit code 2 and empty stdout, but omits asserting that got.stderr contains "withheld", which is consistently checked across the other PostToolUse withholding tests in this file.
    Remediation: Add assert.Contains(t, got.stderr, "withheld") to match the assertion pattern in sibling withholding tests.

  • [adr-annotation-process] docs/ADRs/0100-codex-sandbox-hooks.md:33 — This PR adds an "Implementation note" block inside the Decision section of Accepted ADR 0100, changing the documented behavior from "a rewrite the runtime cannot apply becomes an additionalContext warning" (left unedited and now contradicted) to "block and withhold." docs/contributing/adrs.md requires edits to Accepted ADRs to be called out in the PR description; the PR body does not mention the ADR edit.
    Remediation: Add a line to the PR description noting the annotation added to ADR 0100, per the ADR review-process requirement in docs/contributing/adrs.md.


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 (2)

Review

Findings

Medium

  • [edge-case] internal/runtime/codex_hook/fullsend-codex-hook.py:491 — The context-suppression rescan path (lines 491–524) does not return or continue after the rescan passes cleanly. Control falls through to benign_rewrite(verdict["output"]) at line 525, which must also pass for the result to be allowed. A context-suppression-only rewrite always satisfies benign_rewrite today, but the coupling is implicit: if a future chain change adds a new metadata key alongside context_suppressed, benign_rewrite would reject the unknown key and block even though the rescan proved the content safe. This is documented as intentional fail-closed behavior.

Low

  • [pattern-inconsistency] internal/runtime/codex_hook/fullsend-codex-hook.py:406 — Three new helper functions (context_was_suppressed, hook_is_installed, scan_reported_error) lack docstrings, while every other function in this file has one — even one-liners like block(), parse_json(), and claude_tool_name(). The two new classification functions (security_rewrite, benign_rewrite) correctly follow the convention.
    Remediation: Add a single-line docstring to each of the three functions.

  • [test-weakened] internal/runtime/codex_hook_test.go:441 — TestCodexAdapter_PostToolUseIgnoresContextSuppression assigns input["tool_response"] twice in succession. The first assignment ("many lines of successful test output") is immediately overwritten by the second ("ok example.test 0.5s\nmany safe lines") and is dead code — a leftover from iterating on the test fixture.
    Remediation: Remove the first assignment so only the intended value remains.

  • [naming-convention] internal/runtime/codex_hook/fullsend-codex-hook.py:418 — scan_reported_error returns a bool but reads as a verb rather than following the predicate pattern used by the other new boolean helpers (security_rewrite, benign_rewrite, context_was_suppressed, hook_is_installed).

  • [pattern-inconsistency] internal/runtime/codex_hook_test.go:253 — TestCodexAdapter_PostToolUseWithholdsSecretWithTheRealChain manually inlines the same six-entry map that installPostToolChain() encapsulates. Other new tests in this PR (TestCodexAdapter_PostToolUseRescansSuppressedOutputForSecrets, TestCodexAdapter_PostToolUseRescanFindsObfuscatedSecrets, TestCodexAdapter_PostToolUseIgnoresContextSuppression) correctly use h.installPostToolChain().
    Remediation: Replace the inline map with h.installPostToolChain().


Labels: PR modifies security hook behavior in the codex runtime adapter and fixes a category naming issue in the shared unicode posttool hook


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

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the security Security threat model and related concerns label Sep 3, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 11:14 AM UTC · Completed 11:56 AM UTC

Commit: 570cd7f · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high

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

fullsend-ai-review Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:44 PM UTC · Completed 7:06 PM UTC

Commit: 9b474af · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.90

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

…x's budget

The rescan that lets suppressed output through trusted the chain's stages by
name. The chain imports them from the agent-writable hooks directory, so a
redactor rewritten mid-iteration to find nothing let a plain API key through
(reproduced: exit 0 on the previous adapter). Before every chain spawn the
adapter now re-hashes hook_io.py and each stage against the runner-held
digests, and refuses a stage file that is present but was never installed.

codex kills a hook at the 30 s handler timeout and a killed hook does not
block. A chain pass plus its rescan could each take up to 25 s, so each spawn
now gets only what remains of the budget, and one with under a second left is
withheld instead of started.

hook_is_installed claimed integrity it did not check; it becomes stage_enabled
with an honest docstring. Tests: neutered stage, changed helper, missing stage,
planted disabled stage, the budget arithmetic, the handler timeout matching
hooks.json, CHAIN_SIBLINGS matching the chain, and the rescan still catching
an obfuscated key with the unicode stage disabled.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
The digest check read each hook file with a plain open() before the budget
check ran. The hooks directory is agent-writable, so a FIFO in place of a
stage made the adapter block until codex killed it at the handler timeout,
and a killed hook does not block (reproduced: the adapter hung until killed).
Hashing now opens with O_NONBLOCK and O_NOFOLLOW and refuses anything but a
regular file under 1 MiB, and a missing digest map fails the sibling check
closed on its own.

The CHAIN_SIBLINGS drift test now also collects every fullsend hook module the
chain or a stage imports, and the budget test derives its offset from the
adapter's constants.

Assisted-by: Claude (fix), Grok 4.6 (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
Hashing the chain's known stage files was not enough: the chain does a plain
`import hook_io`, and Python prefers a package over a module of the same name
on one path entry. A planted hooks/hook_io/ package shadowed the verified
hook_io.py and let an API key through (reproduced: exit 0 on the previous
commit). Before every chain spawn the adapter now checks the hooks directory
the way Run's guard does: every entry a regular file with its runner-held
digest, and nothing else present. That subsumes the stage list, so
CHAIN_SIBLINGS and its drift test are gone.

Assisted-by: Claude (fix), Codex gpt-5.6-sol (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09

Copy link
Copy Markdown
Member Author

On the two review-body findings that could not be posted inline:

  • Two chain runs vs. the 30 s handler timeout: fixed in 802137a. Each spawn's timeout is now what remains of codex's budget (HANDLER_TIMEOUT_S = security.HookTimeoutSeconds, less a 2 s margin), and a spawn with under a second left is withheld instead of started, so codex never kills the adapter mid-check. Tests: TestCodexAdapter_ScriptTimeoutFitsTheHandlerBudget, TestCodexAdapter_HandlerTimeoutMatchesHooksJSON, TestCodexAdapter_PostToolUseWithholdsWhenTheBudgetIsSpent.
  • ADR 0100 annotation: the PR description now calls out the implementation note added to the accepted ADR.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:38 PM UTC · Completed 7:57 PM UTC

Commit: a059545 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.09

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself September 22, 2026 19:57

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 22, 2026
@waynesun09
waynesun09 added this pull request to the merge queue Sep 22, 2026
Merged via the queue into main with commit 4a5d26f Sep 22, 2026
144 checks passed
@waynesun09
waynesun09 deleted the codex-security-hooks branch September 22, 2026 21:35
@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:36 PM UTC · Completed 9:58 PM UTC

Commit: a059545 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.77

@fullsend-ai-retro

Copy link
Copy Markdown

PR #6952 ("withhold unsafe codex tool results") was human-authored, not agent-generated, and took 19 days to merge in two bursts. Burst 1 (Sep 3): opened, force-pushed 3x, review agent flagged 2 medium fail-open findings (fixed same day), then the next review run FAILED outright and the PR went dormant for 19 days. Burst 2 (Sep 22): author reactivated the branch, review ran cleanly, flagged 2 more medium fail-open findings plus style nits, author fixed all of them with targeted commits/tests within ~1 hour, a human reviewer approved with no further findings, and the PR merged.

Root cause of the 19-day stall: four review sub-agent dimensions (risk-assessment, intent-coherence, style-conventions, docs-currency) failed because claude-sonnet-4-5@20250929/claude-sonnet-4-6@default wasn't served by the Vertex deployment. Instead of degrading gracefully (the review skill's documented fallback is to record a sub-agent-failure finding and continue), the orchestrator retried repeatedly and never wrote output/agent-result.json, so both harness iterations failed validation and the job failed after ~43 minutes. A status comment did post, but only the generic "validation failed after 2 iteration(s)" message — the captured root-cause detail was never threaded into it — so it blended in among ~13 other bot comments and went unnoticed until the human manually reactivated the branch.

This mechanism and its downstream symptoms are already tracked, and this run adds corroborating evidence: fullsend#6922 (sonnet unavailable causing wasted review retries) — new evidence that the orchestrator's retry loop consumes the entire time budget rather than failing fast, traceable to review.md's "report failure, don't produce partial results" constraint conflicting with the pr-review skill's own documented per-sub-agent degrade-and-continue fallback. fullsend#7023 (generic "validation failed after N iterations" message hides root cause) — PR #6952's failure comment is a verbatim match. fullsend#4766 (alert on persistent dispatch failures) — this PR is a concrete 19-day case study of exactly that gap. agents#379 (sub-agent model-unavailability fallback/alerting) — the conflicting-instructions root cause found here is new supporting detail. fullsend#7346 (422-on-inline-comment fallback degrades findings to prose) — independently reproduced: the Sep 22 19:06 review run hit the same 422 fallback while posting its fail-open findings.

One new, previously untracked gap is filed below: the 2026-09-08 workaround in this repo's own .fullsend/config.yaml that rerouted review sub-agents off the unavailable sonnet alias is incomplete — it covers 5 of 7 personas but leaves risk-assessment and intent-coherence on the bare sonnet alias, verified still true as of 2026-09-22. This is the same latent single point of failure that caused this incident, now sitting unpatched in exactly the two dimensions that failed both times.

On the positive side: agent review caught real, non-trivial fail-open security findings in both rounds (secret-leak-before-redaction ordering; a suppress-rescan step not fitting inside the 30s handler timeout; a hook-integrity check that only verified filenames, not content hashes) that the author fixed with targeted commits and new tests, and the human reviewer approved without needing to add further findings — a solid signal that agent review is pulling its weight on this class of security-hardening change with strong test coverage.

Proposals filed

This branch was successfully deployed

1 active and 1 inactive deployments
site-preview — a0595453 Deployed Sep 22, 2026 by github-actions[bot]
dev — a0595453 Deployed Sep 22, 2026 by waynesun09 via behaviour #13543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge risk/moderate PR risk: moderate security Security threat model and related concerns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants