Skip to content

fix(codex): quarantine unreadable dynamic tool schemas#91289

Closed
vincentkoc wants to merge 2 commits into
mainfrom
split-tool-schema-quarantine-20260608
Closed

fix(codex): quarantine unreadable dynamic tool schemas#91289
vincentkoc wants to merge 2 commits into
mainfrom
split-tool-schema-quarantine-20260608

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Quarantines Codex dynamic tools whose input schemas cannot be safely normalized before they reach the Codex app-server bridge.
  • Preserves plugin/owner attribution for tool-schema quarantine diagnostics so openclaw doctor can point at the owning source instead of losing context.
  • Cherry-picked from the superseded large split stack in fix(runtime): snapshot tool definitions #90411: 6d7a1b8d0ec7 and 27ef8763e733.

Codex contract checked

  • openai-codex Python protocol types expose dynamic tool inputSchema as unconstrained schema data (sdk/python/src/openai_codex/generated/v2_all.py:807).
  • Codex Rust protocol stores dynamic tool schemas as JSON values (codex-rs/protocol/src/dynamic_tools.rs:8).
  • Codex app-server validates dynamic tool schemas before conversion and rejects unsupported schemas with a tool-specific error (codex-rs/app-server/src/request_processors/thread_processor.rs:316).
  • Codex has coverage for rejecting unsupported dynamic tool schemas (codex-rs/app-server/src/request_processors/thread_processor_tests.rs:91).

Verification

  • node scripts/run-vitest.mjs run extensions/codex/src/app-server/dynamic-tools.test.ts src/agents/tool-schema-projection.test.ts src/agents/tools-effective-inventory.test.ts src/commands/doctor/shared/active-tool-schema-warnings.test.ts --reporter=dot
  • node --max-old-space-size=8192 --import tsx scripts/generate-plugin-sdk-api-baseline.ts --check
  • ./node_modules/.bin/oxfmt --check docs/.generated/plugin-sdk-api-baseline.sha256 extensions/codex/src/app-server/dynamic-tools.test.ts extensions/codex/src/app-server/dynamic-tools.ts src/agents/tool-schema-projection.test.ts src/agents/tool-schema-projection.ts src/agents/tools-effective-inventory-build.ts src/agents/tools-effective-inventory.test.ts src/commands/doctor/shared/active-tool-schema-warnings.test.ts src/commands/doctor/shared/active-tool-schema-warnings.ts src/plugin-sdk/agent-harness-runtime.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/agents/tool-schema-projection.test.ts src/agents/tool-schema-projection.ts src/agents/tools-effective-inventory-build.ts src/agents/tools-effective-inventory.test.ts src/commands/doctor/shared/active-tool-schema-warnings.test.ts src/commands/doctor/shared/active-tool-schema-warnings.ts src/plugin-sdk/agent-harness-runtime.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-bundled-extension-oxlint.mjs extensions/codex/src/app-server/dynamic-tools.test.ts extensions/codex/src/app-server/dynamic-tools.ts
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Behavior addressed

  • Dynamic Codex tools with unreadable or unsupported schema shapes are skipped/quarantined by OpenClaw instead of being allowed to crash or fail later in the Codex app-server path.

Real environment tested

  • macOS linked OpenClaw worktree with shared install and focused Vitest/lint/format checks.

Exact steps or command run after this patch

  • Ran the focused commands listed in Verification after cherry-picking and resolving the SDK baseline conflict.

Evidence after fix

  • 77 focused tests passed across Codex app-server dynamic tools, agent schema projection, effective inventory, and doctor warning ownership.
  • SDK API baseline check passed.
  • Focused format, core oxlint, bundled extension oxlint, and autoreview passed.

Observed result after fix

  • Invalid dynamic tool schemas are quarantined with retained owner/source context, and the Codex bridge keeps building valid dynamic tools.

What was not tested

  • Broad pnpm check:changed was not run locally because this is a linked/sparse worktree; PR CI should cover the wider gate.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation commands Command implementations agents Agent runtime and tooling extensions: codex size: M maintainer Maintainer-authored PR labels Jun 8, 2026
@clawsweeper

clawsweeper Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed June 30, 2026, 4:19 PM ET / 20:19 UTC.

Summary
The PR quarantines unreadable Codex dynamic tool schemas, preserves tool-schema quarantine ownership in doctor/effective-inventory diagnostics, and adds a shared schema-projection helper that is re-exported through the agent-harness Plugin SDK surface.

PR surface: Source +145, Tests +162, Generated 0. Total +307 across 10 files.

Reproducibility: yes. from source. Current main can lose plugin ownership for schema diagnostics that use fallback names such as tool[0], while descriptor-level Codex dynamic-tool quarantine is already present on main.

Review metrics: 1 noteworthy metric.

  • Plugin SDK public surface: 1 helper export, 2 type exports added. These additions are under an exported Plugin SDK subpath, so they matter as compatibility surface before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/commands/doctor/shared/active-tool-schema-warnings.test.ts, migration/backfill/repair: src/commands/doctor/shared/active-tool-schema-warnings.ts, unknown-data-model-change: src/commands/doctor/shared/active-tool-schema-warnings.test.ts, unknown-data-model-change: src/commands/doctor/shared/active-tool-schema-warnings.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🌊 off-meta tidepool
Patch quality: 🦐 gold shrimp
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Remove or explicitly approve/document the new Plugin SDK exports.
  • Refresh the branch over current main and rerun focused validation.

Risk before merge

  • [P1] The PR is currently conflicting/dirty against current main, so maintainers need a refreshed branch or merge result before landing.
  • [P1] The new openclaw/plugin-sdk/agent-harness-runtime exports can become third-party Plugin SDK compatibility surface unless maintainers explicitly approve and document them.
  • [P1] Current main already includes descriptor-level Codex quarantine from fix(codex): quarantine unreadable dynamic tools #90022, so the refreshed PR must preserve that behavior while carrying only the remaining owner-attribution repair.

Maintainer options:

  1. Keep Projection Internal (recommended)
    Remove the new agent-harness-runtime re-exports and refresh the API baseline so the bug fix does not create public Plugin SDK surface.
  2. Approve The SDK Contract
    If maintainers want plugins to call this helper, add the matching SDK docs and contract coverage before merge.
  3. Refresh Before Further Review
    Because GitHub reports the branch as conflicting, refresh the branch first so review applies to the actual current-main merge result.

Next step before merge

  • [P2] The remaining blockers are SDK API direction and a dirty/conflicting branch, so this needs maintainer/author follow-up rather than autonomous repair dispatch.

Security
Cleared: No concrete security or supply-chain regression was found; the diff does not change dependencies, workflows, permissions, package resolution, secrets handling, or artifact execution.

Review findings

  • [P1] Keep the projection helper out of the public SDK until approved — src/plugin-sdk/agent-harness-runtime.ts:181-185
Review details

Best possible solution:

Land the owner-attribution repair after refreshing over current main, and keep the projection helper internal unless maintainers explicitly approve it as documented Plugin SDK API.

Do we have a high-confidence way to reproduce the issue?

Yes, from source. Current main can lose plugin ownership for schema diagnostics that use fallback names such as tool[0], while descriptor-level Codex dynamic-tool quarantine is already present on main.

Is this the best way to solve the issue?

No as submitted. The owner-attribution repair is useful, but the public SDK re-exports are only the best fix if maintainers intentionally approve and document that contract, and the branch needs a current-main refresh.

Full review comments:

  • [P1] Keep the projection helper out of the public SDK until approved — src/plugin-sdk/agent-harness-runtime.ts:181-185
    This re-exports projectRuntimeCompatibleToolInputSchemas and two types from openclaw/plugin-sdk/agent-harness-runtime, an exported Plugin SDK subpath. If the helper is only needed for the Codex/core repair, publishing it creates an unintended third-party API contract; remove the re-exports or add maintainer-approved SDK docs and contract coverage.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 640258d7b31d.

Label changes

Label justifications:

  • P2: This is a bounded Codex/runtime hardening PR with limited blast radius but real diagnostic and compatibility impact.
  • merge-risk: 🚨 compatibility: The diff adds public Plugin SDK exports that could become a supported third-party compatibility contract.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply because this is a maintainer-labeled MEMBER PR, though the body records focused after-patch validation rather than a live Codex assistant turn.
Evidence reviewed

PR surface:

Source +145, Tests +162, Generated 0. Total +307 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 5 175 30 +145
Tests 4 165 3 +162
Docs 0 0 0 0
Config 0 0 0 0
Generated 1 2 2 0
Other 0 0 0 0
Total 10 342 35 +307

What I checked:

  • Protected maintainer PR: Live PR metadata shows author association MEMBER, label maintainer, head 39705ed, and GitHub reports mergeable CONFLICTING / mergeStateStatus DIRTY. (39705edd3ed4)
  • Public SDK export risk: The PR diff adds projectRuntimeCompatibleToolInputSchemas plus two projection types to src/plugin-sdk/agent-harness-runtime.ts, which makes the helper part of the plugin-facing surface unless maintainers intentionally accept it. (src/plugin-sdk/agent-harness-runtime.ts:181, 39705edd3ed4)
  • SDK subpath is exported: Current main exports ./plugin-sdk/agent-harness-runtime from package.json, so new symbols there are reachable through openclaw/plugin-sdk/agent-harness-runtime. (package.json:628, 640258d7b31d)
  • Repository policy applied: The scoped Plugin SDK guide says this directory is the public contract between plugins and core and warns against exposing internals without intentionally promoting a supported public contract. (src/plugin-sdk/AGENTS.md:1, 640258d7b31d)
  • Current main already covers descriptor quarantine: Current main already defensively reads Codex dynamic tool entries and descriptor fields, including catching unreadable parameters and reporting <tool>.inputSchema is unreadable, so the PR must be refreshed around existing descriptor-level quarantine behavior. (extensions/codex/src/app-server/dynamic-tools.ts:808, 640258d7b31d)
  • Current main still has owner-attribution gap: Doctor warnings currently store pre-normalization diagnostics without their source tool array and resolve plugin ownership only by diagnostic name, so fallback names such as tool[0] can lose plugin attribution. (src/commands/doctor/shared/active-tool-schema-warnings.ts:156, 640258d7b31d)

Likely related people:

  • vincentkoc: Authored this PR, the merged descriptor-level Codex quarantine PR, and the closed parent split-stack commits for Codex schema quarantine and doctor ownership preservation. (role: feature-history owner; confidence: high; commits: 26135a1bc9d0, 6d7a1b8d0ec7, 27ef8763e733; files: extensions/codex/src/app-server/dynamic-tools.ts, src/agents/tool-schema-projection.ts, src/commands/doctor/shared/active-tool-schema-warnings.ts)
  • eleqtrizit: The shallow current-main history and blame for the central files point to PR fix(acp): require owner for runtime controls #97953 touching the same dynamic-tools, schema projection, inventory, doctor warning, and agent-harness runtime files shortly before this review. (role: recent adjacent contributor; confidence: medium; commits: 6ead09230284; files: extensions/codex/src/app-server/dynamic-tools.ts, src/agents/tool-schema-projection.ts, src/agents/tools-effective-inventory-build.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 8, 2026
@vincentkoc vincentkoc mentioned this pull request Jun 8, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Closed
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing this Codex/trajectory fuzz-hardening branch. Direct Codex contract review at openai/codex@be33f80bc65159c094ecd06bf155afa3061ce23d shows that dynamic tools require string names/descriptions plus serialized JsonValue input schemas, and app-server validates each schema centrally before use: dynamic_tools.rs, thread_processor.rs.

OpenClaw main already projects and quarantines dynamic tools once through 3ffb3609a14 and d7d037b46f3. The remaining throwing-getter/proxy fixtures target downstream typed or projected data without a linked production report. If a real serialized payload fails, please file that payload and stack against the canonical projection boundary. Thanks for the investigation.

@steipete steipete closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling commands Command implementations docs Improvements or additions to documentation extensions: codex maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: M status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete