Repository navigation
fix(#6920): withhold unsafe codex tool results - #6952
Conversation
PR Summary by QodoWithhold unsafe Codex tool results
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
32528d3 to
6722d79
Compare
Site previewPreview: https://790d11cf-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · Commit: |
6722d79 to
b495cf5
Compare
|
🤖 Review · Commit: |
b495cf5 to
1e7ac66
Compare
|
🤖 Finished Review · ✅ Success · Started 10:32 AM UTC · Completed 11:11 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.32 |
|
Risk Assessment: moderate (2/5) DetailsScore 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 runRisk Assessment: moderate (2/5) DetailsScore 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) DetailsMedium-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. |
ReviewFindingsLow
The prior review's two medium fail-open findings (rescan timeout budgeting; integrity verification by name-presence only) and two low style findings on Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
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:
|
1e7ac66 to
570cd7f
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 11:14 AM UTC · Completed 11:56 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
Assisted-by: Codex (fix), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
570cd7f to
9b474af
Compare
|
🤖 Finished Review · ✅ Success · Started 6:44 PM UTC · Completed 7:06 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.90 |
…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>
|
On the two review-body findings that could not be posted inline:
|
|
🤖 Finished Review · ✅ Success · Started 7:38 PM UTC · Completed 7:57 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.09 |
Superseded by updated review
|
🤖 Finished Retro · ✅ Success · Started 9:36 PM UTC · Completed 9:58 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.77 |
|
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 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 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 |
Summary
hook_io/package shadowinghook_io.pywould otherwise change what the chain runs); hashing refuses FIFOs, symlinks and oversized files so it cannot stallAccepted 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
additionalContextwarning. The note sits next to the Decision rather than rewriting it, perdocs/contributing/adrs.md.Test plan
pre-commit run --files <changed files>go test ./internal/runtime ./internal/security -count=1pytest internal/security/hooks(656 passed)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