Skip to content

fix(agents): tolerate provider tool schema hook failures#88977

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-projection-entrypoints-20260601
Closed

fix(agents): tolerate provider tool schema hook failures#88977
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-projection-entrypoints-20260601

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

  • keep provider normalizeToolSchemas and inspectToolSchemas strict by default for doctor/check validation
  • pass assistant-only warn mode through embedded attempts, Codex dynamic tools, and compaction so hook failures log and keep current runtime tools
  • make error formatting total for arbitrary thrown values, including hostile proxies/functions, before warning logs are emitted

Verification

  • node scripts/run-vitest.mjs src/infra/errors.test.ts src/agents/embedded-agent-runner/tool-schema-runtime.test.ts src/agents/runtime-plan/tools.test.ts extensions/codex/src/app-server/dynamic-tool-build.test.ts src/agents/embedded-agent-runner/compact.hooks.test.ts --reporter=dot
  • node scripts/run-vitest.mjs src/flows/doctor-core-checks.runtime-errors.test.ts src/commands/doctor/shared/active-tool-schema-warnings.test.ts src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.ts --reporter=dot
  • node_modules/.bin/oxfmt --check --threads=1 src/infra/errors.ts src/infra/errors.test.ts src/agents/embedded-agent-runner/tool-schema-runtime.ts src/agents/embedded-agent-runner/tool-schema-runtime.test.ts src/agents/runtime-plan/tools.ts src/agents/runtime-plan/tools.test.ts src/agents/runtime-plan/build.ts src/agents/runtime-plan/types.ts src/agents/embedded-agent-runner/run/attempt.ts extensions/codex/src/app-server/dynamic-tool-build.ts extensions/codex/src/app-server/dynamic-tool-build.test.ts src/agents/embedded-agent-runner/compact.ts src/agents/embedded-agent-runner/compact.hooks.test.ts src/agents/embedded-agent-runner/compact.hooks.harness.ts
  • OPENCLAW_OXLINT_SKIP_LOCK=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.json src/infra/errors.ts src/infra/errors.test.ts src/agents/embedded-agent-runner/tool-schema-runtime.ts src/agents/embedded-agent-runner/tool-schema-runtime.test.ts src/agents/runtime-plan/tools.ts src/agents/runtime-plan/tools.test.ts src/agents/runtime-plan/build.ts src/agents/runtime-plan/types.ts src/agents/embedded-agent-runner/run/attempt.ts extensions/codex/src/app-server/dynamic-tool-build.ts extensions/codex/src/app-server/dynamic-tool-build.test.ts src/agents/embedded-agent-runner/compact.ts src/agents/embedded-agent-runner/compact.hooks.test.ts src/agents/embedded-agent-runner/compact.hooks.harness.ts
  • git diff --check origin/main...HEAD
  • private reported-name scrub across spec and changed files
  • .agents/skills/autoreview/scripts/autoreview --mode local --prompt "..."

Real behavior proof

Behavior addressed: assistant-visible tool projection no longer aborts when a provider tool-schema hook throws; doctor/check paths still surface the failure because strict behavior remains the default.

Real environment tested: local Codex worktree with Node/Vitest routed through scripts/run-vitest.mjs; broad remote proof attempted through AWS Crabbox and Blacksmith Testbox.

Exact steps or command run after this patch: focused Vitest commands, oxfmt, oxlint, git diff --check origin/main...HEAD, private-name scrub, local autoreview, and remote node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "corepack pnpm check:changed".

Evidence after fix: focused tests passed 146 tests across 3 routed shards and 67 tests across 3 routed shards; formatter/lint/diff/scrub passed; final autoreview reported no accepted/actionable findings with overall: patch is correct (0.87).

Observed result after fix: warn-mode paths keep the current runtime tools and log sanitized hook failures, including non-Error and hostile thrown values; strict callers still throw for doctor/check visibility.

What was not tested: remote check:changed did not run because AWS Crabbox allocation failed twice with active lease limit exceeded: 9/8, and Blacksmith Testbox failed with not authenticated -- run 'blacksmith auth login' first.

@vincentkoc vincentkoc self-assigned this Jun 1, 2026
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling extensions: codex size: M maintainer Maintainer-authored PR labels Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 6:07 PM ET / 22:07 UTC.

Summary
The PR adds warn-and-continue provider tool-schema hook handling in assistant runtime, compaction, and Codex dynamic-tool projection while preserving strict default validation and hardening error formatting.

PR surface: Source +93, Tests +270. Total +363 across 14 files.

Reproducibility: yes. by source inspection: current main directly invokes provider normalize and inspect hooks from runtime projection helpers, so a throwing hook propagates. I did not run a live third-party provider plugin repro in this read-only review.

Review metrics: 1 noteworthy metric.

  • Hook Failure Modes: 1 optional mode added; 3 assistant projection surfaces opt in. This captures the compatibility-sensitive split between strict validation and warn-and-continue assistant projection before merge.

Stored data model
Persistent data-model change detected: serialized state: src/infra/errors.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:

  • Choose one canonical provider-hook containment branch.
  • Refresh the chosen branch against current main and rerun broad changed-gate proof.

Risk before merge

  • [P1] Merging changes selected assistant runtime paths from fail-fast provider/plugin schema hook errors to warn-and-continue with current runtime tools, which is compatibility-sensitive for provider/plugin behavior.
  • [P1] The branch is draft and GitHub reports it as conflicting, so the actual current-main merge result and broad proof are not meaningful until it is refreshed.
  • [P1] The open overlapping branch at fix(agents): contain provider schema hook failures #89669 targets the same provider hook failure contract; maintainers should choose one canonical branch to avoid divergent semantics.

Maintainer options:

  1. Choose One Canonical Hook Contract (recommended)
    Select this PR or the overlapping provider-schema branch as canonical, resolve conflicts against current main, and rerun broad proof before merge.
  2. Accept Runtime Warn Mode Deliberately
    Maintainers may intentionally accept that assistant runtime keeps current tools after provider hook failures while doctor and check paths remain strict.
  3. Pause Behind Replacement
    If the overlapping branch becomes canonical, keep this PR as context or close it only after the replacement is refreshed, proof-positive, and linked.

Next step before merge

  • [P2] Maintainer review is needed because this protected draft is conflicting, compatibility-sensitive, and overlaps another open provider-hook containment branch.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes runtime error handling and redacted logging without workflows, dependencies, lockfiles, permissions, downloads, or secret handling changes.

Review details

Best possible solution:

Land one refreshed canonical provider hook containment branch that keeps doctor/check validation strict, uses total redacted warning logs, covers assistant and Codex entrypoints, and has broad current-main proof.

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

Yes, by source inspection: current main directly invokes provider normalize and inspect hooks from runtime projection helpers, so a throwing hook propagates. I did not run a live third-party provider plugin repro in this read-only review.

Is this the best way to solve the issue?

Unclear until maintainers choose the canonical branch. This PR's strict-default plus assistant-only warn mode is a maintainable shape, but the dirty overlap with the sibling branch must be resolved first.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 843ad143647e.

Label changes

Label justifications:

  • P2: This is a bounded agent/provider runtime hardening PR with limited blast radius and no evidence of an emergency outage.
  • merge-risk: 🚨 compatibility: The diff changes selected provider/plugin schema hook failures from propagated runtime errors to warn-and-continue behavior in assistant paths.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this maintainer-labeled MEMBER PR; the body reports focused tests and notes blocked broad remote proof.
Evidence reviewed

PR surface:

Source +93, Tests +270. Total +363 across 14 files.

View PR surface stats
Area Files Added Removed Net
Source 9 118 25 +93
Tests 5 271 1 +270
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 14 389 26 +363

What I checked:

Likely related people:

  • vincentkoc: Authored this maintainer PR and prior provider-family replay/tool-hook work, and merged the current-main commit that reintroduced the central helper files. (role: feature owner and recent area contributor; confidence: high; commits: 39d2a719c964, 68ddb9744f78, 6e20a32791b8; files: src/agents/embedded-agent-runner/tool-schema-runtime.ts, src/agents/runtime-plan/tools.ts, extensions/codex/src/app-server/dynamic-tool-build.ts)
  • Peter Steinberger: Recent history includes provider handle reuse, strict tool-schema behavior, provider runtime caching, and provider diagnostic work adjacent to this runtime-plan/provider boundary. (role: recent adjacent provider/tool contributor; confidence: medium; commits: 057be10e5b46, a7e029fde9be, 32a25b865fa7; files: src/agents/runtime-plan/tools.ts, src/plugins/provider-runtime.ts, src/plugin-sdk/provider-tools.ts)
  • jalehman: Commit history shows Josh Lehman added adjacent provider replay/runtime hook surfaces in provider runtime and plugin contract files. (role: adjacent provider runtime hook contributor; confidence: medium; commits: 71346940adaf; files: src/plugins/provider-runtime.ts, src/plugins/types.ts, src/plugin-sdk/plugin-entry.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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T08:58:42.233Z sha 6e20a32 :: needs maintainer review before merge. :: none

@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. labels Jun 1, 2026
@vincentkoc vincentkoc mentioned this pull request Jun 1, 2026
Closed
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this fuzz-only hardening PR. The only failing state is a synthetic fixture using hostile in-process objects, throwing getters or callbacks, or similarly malformed metadata; there is no linked production report or supported external contract requiring that state.

This adds broad fail-soft behavior and substantial regression surface across consumers. If a real plugin or provider demonstrates the failure, please open a focused issue with the observed payload and stack so validation can live at the single owning registration/parsing boundary. Thanks for exploring it.

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

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

agents Agent runtime and tooling 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: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete