fix(codex): quarantine unreadable dynamic tools#90022
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 9:22 PM ET / 01:22 UTC. Summary PR surface: Source +133, Tests +110. Total +243 across 2 files. Reproducibility: yes. from source: current main directly reads Codex dynamic-tool descriptor fields during projection/spec creation, so poisoned getters can throw before bridge creation completes. I did not execute tests in this read-only review. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the bridge-level quarantine after maintainer review and any desired broad/live Codex proof, keeping malformed plugin tools out of Codex specs while healthy sibling tools remain available. Do we have a high-confidence way to reproduce the issue? Yes from source: current main directly reads Codex dynamic-tool descriptor fields during projection/spec creation, so poisoned getters can throw before bridge creation completes. I did not execute tests in this read-only review. Is this the best way to solve the issue? Yes. The patch fixes the bridge boundary by caching validated descriptor data, quarantining unreadable/wrapping failures, and preventing Codex from seeing tools that are unavailable to execute; relying only on build-stage filtering would leave direct bridge callers less protected. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 5c5391836b1e. Label changesLabel justifications:
Evidence reviewedPR surface: Source +133, Tests +110. Total +243 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
keeper decision for this duplicate family: keep and land #90022.
Proof before undraft/merge:
After this lands I am folding #88774, #89063, #89397, and #89481 into this canonical PR. |
Summary
name,parameters, or wrapping-timeexecutegetter.specsso tools quarantined from the available executable set are not still advertised to Codex.Verification
node scripts/run-vitest.mjs extensions/codex/src/app-server/dynamic-tools.test.tsnode scripts/run-vitest.mjs extensions/codex/src/app-server/dynamic-tools.test.ts extensions/codex/src/app-server/dynamic-tool-build.test.ts./node_modules/.bin/oxfmt --check --threads=1 extensions/codex/src/app-server/dynamic-tools.ts extensions/codex/src/app-server/dynamic-tools.test.ts./node_modules/.bin/oxlint extensions/codex/src/app-server/dynamic-tools.ts extensions/codex/src/app-server/dynamic-tools.test.tsgit diff --check origin/main...HEAD.agents/skills/autoreview/scripts/autoreview --mode local.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/mainnode scripts/crabbox-wrapper.mjs run --shell -- env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changedblocked before execution: coordinator HTTP 401 unauthorized, providerazure, slugcrimson-barnacle.Real behavior proof
Behavior addressed: A malformed Codex dynamic tool descriptor can no longer crash dynamic tool bridge creation before the assistant turn gets content; the bad tool is quarantined and healthy sibling tools remain available.
Real environment tested: Local linked OpenClaw worktree using the repo Vitest wrapper for the Codex extension app-server tests; broad remote changed gate attempted through the Crabbox wrapper.
Exact steps or command run after this patch: Ran the focused Codex dynamic tool bridge tests, the adjacent dynamic-tool-build tests, formatter check, oxlint, diff check, local autoreview, branch autoreview, and the Crabbox changed-gate command listed above.
Evidence after fix: Focused Vitest passed 2 files / 66 tests; formatter, lint, diff check, local autoreview, and branch autoreview all passed.
Observed result after fix: Poisoned dynamic tool
name,parameters, invalid emptyname, and wrapping-timeexecutegetters are quarantined; only the healthymessagetool remains inavailableSpecsand durablespecs.What was not tested: Full
pnpm check:changeddid not execute because the Crabbox coordinator rejected run/lease creation with HTTP 401. No live Codex assistant turn was run.