Skip to content

fix(agents): contain provider schema hook failures#89669

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next64-20260603
Closed

fix(agents): contain provider schema hook failures#89669
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next64-20260603

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Contain provider tool-schema normalization hook failures at assistant runtime so one bad provider/plugin hook cannot crash model projection before content.
  • Keep doctor/runtime health checks strict by opting them into rethrowing provider schema normalization failures.
  • Normalize provider schema diagnostics before logging so malformed diagnostic entries cannot crash the warning path.

Verification

  • CI=1 node scripts/run-vitest.mjs run src/agents/embedded-agent-runner/tool-schema-runtime.test.ts src/agents/runtime-plan/tools.test.ts src/flows/doctor-core-checks.runtime-errors.test.ts src/commands/doctor/shared/active-tool-schema-warnings.test.ts --reporter=verbose
  • ./node_modules/.bin/oxfmt --write 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/flows/doctor-core-checks.runtime.ts src/commands/doctor/shared/active-tool-schema-warnings.ts
  • ./node_modules/.bin/oxlint 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/flows/doctor-core-checks.runtime.ts src/commands/doctor/shared/active-tool-schema-warnings.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
  • AWS Crabbox run_23545b82a3f6, lease cbx_29277eee67f0, type c7a.8xlarge: env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed passed with exit 0; lanes core, coreTests; lease stopped.

Real behavior proof

Behavior addressed: Provider-owned tool-schema normalization/diagnostic hooks can no longer crash assistant runtime projection before content, while doctor still reports normalization failures as health findings.
Real environment tested: Local Codex worktree on branch fuzz-tool-schema-next64-20260603 at commit 3662c8cea8284ccbe35c05864419a655959a00d6; remote AWS Crabbox changed gate run_23545b82a3f6 / cbx_29277eee67f0.
Exact steps or command run after this patch: Focused Vitest command above covered runtime hook fallback, runtime-plan flag forwarding, doctor runtime errors, and active doctor warnings; AWS Crabbox ran pnpm check:changed remotely.
Evidence after fix: 4 focused test files passed; oxfmt, oxlint, git diff check, autoreview, and AWS Crabbox check:changed passed.
Observed result after fix: Runtime normalization falls back to the already-filtered tools and logs a warning; doctor call sites opt into rethrowing and keep surfacing provider/plugin schema normalization failures.
What was not tested: Live provider/tool execution against a real third-party provider plugin was not run; this lane covers the runtime boundary with focused unit tests and remote changed gates.

@vincentkoc vincentkoc self-assigned this Jun 3, 2026
@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Jun 3, 2026
@clawsweeper

clawsweeper Bot commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed July 3, 2026, 9:48 AM ET / 13:48 UTC.

Summary
The PR catches provider tool-schema normalization and diagnostic hook failures in agent runtime, adds strict rethrow opt-ins for doctor normalization paths, and normalizes malformed diagnostic entries before logging.

PR surface: Source +138, Tests +95. Total +233 across 5 files.

Reproducibility: yes. by source inspection. Current main directly invokes provider normalize and inspect hooks from runtime projection helpers, so a throwing provider/plugin hook propagates before this PR; no live third-party provider plugin repro was run.

Review metrics: 2 noteworthy metrics.

  • Provider Hook Failure Contract: 2 tolerant catches added, 3 strict normalization opt-ins added. This quantifies the compatibility-sensitive split between assistant runtime fallback and doctor/check validation before merge.
  • SDK-Exported Helper Paths: 2 exported helper paths route through touched behavior. The plugin SDK harness exports runtime tool helpers and normalizeProviderToolSchemas, so default failure semantics can affect plugin-facing callers.

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

Risk before merge

  • [P2] Merging as-is changes provider/plugin schema hook failures from propagated errors to warn-and-continue fallback for default callers unless every strict path opts in.
  • [P1] The new hook warning path can log raw provider/plugin error text and can itself throw while formatting hostile thrown values.
  • [P1] The same behavior overlaps fix(agents): tolerate provider tool schema hook failures #88977, but that branch is also draft and conflicting, so maintainers need choose one refreshed canonical path.
  • [P1] GitHub reports this PR as draft and CONFLICTING against current main, so the exact merge result needs a refreshed review before landing.

Maintainer options:

  1. Refresh One Canonical Hook Contract (recommended)
    Choose this PR or the sibling branch as canonical, keep strict defaults, make assistant warn mode explicit, fix redacted total formatting, resolve conflicts, and rerun current-main proof.
  2. Accept Runtime Warn Mode Deliberately
    Maintainers can intentionally accept warn-and-continue behavior after confirming every doctor/check/plugin SDK caller that must stay strict is protected.
  3. Pause Behind The Broader Branch
    If fix(agents): tolerate provider tool schema hook failures #88977 becomes canonical, keep this PR only as context until that branch is refreshed and proof-positive.

Next step before merge

  • [P2] Protected maintainer draft changes provider/plugin failure semantics and overlaps a broader draft branch, so maintainers need choose the canonical direction before repair or landing.

Security
Needs attention: The diff does not touch dependencies or CI, but the new provider hook warning path can leak raw provider/plugin error text and can fail while formatting hostile thrown values.

Review findings

  • [P1] Keep hook failure mode explicit — src/agents/embedded-agent-runner/tool-schema-runtime.ts:71-78
  • [P1] Redact hook failure details before logging — src/agents/embedded-agent-runner/tool-schema-runtime.ts:146-150
Review details

Best possible solution:

Land one refreshed canonical hook-containment branch that keeps provider hook helpers strict by default, opts assistant projection into warn mode explicitly, uses total redacted error formatting, and preserves doctor/check strictness with 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 provider/plugin hook propagates before this PR; no live third-party provider plugin repro was run.

Is this the best way to solve the issue?

No as-is. The shared runtime helper is a plausible layer, but the best fix should preserve strict defaults, make assistant warn mode explicit, and use a total redacted error formatter before warning logs are emitted.

Full review comments:

  • [P1] Keep hook failure mode explicit — src/agents/embedded-agent-runner/tool-schema-runtime.ts:71-78
    normalizeProviderToolSchemas is visible through plugin-facing harness paths, but this catch makes the helper warn-and-continue unless every strict caller opts in. Keep the helper strict by default and pass an explicit warn mode only from assistant projection entry points so plugin SDK and future internal callers do not silently lose fail-fast behavior.
    Confidence: 0.84
  • [P1] Redact hook failure details before logging — src/agents/embedded-agent-runner/tool-schema-runtime.ts:146-150
    describeProviderHookError returns raw Error.message or calls String(error) before warning metadata is built. Provider/plugin errors can contain credentials, and hostile thrown values can make this catch path throw; route these failures through total redacted formatting and cover hostile values.
    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 8c915f068525.

Label changes

Label justifications:

  • P2: This is a bounded agent/provider runtime hardening PR with limited blast radius, but it changes compatibility-sensitive failure behavior.
  • merge-risk: 🚨 compatibility: The diff changes existing provider/plugin schema hook failures from propagated errors to warn-and-continue fallback for default callers.
  • merge-risk: 🚨 security-boundary: The new warning formatter can expose sensitive provider/plugin error text and can fail while handling hostile thrown values.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • 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 to this maintainer-labeled MEMBER PR; the body reports focused Vitest and AWS Crabbox changed-gate proof, but no live third-party provider plugin run.
Evidence reviewed

PR surface:

Source +138, Tests +95. Total +233 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 4 165 27 +138
Tests 1 96 1 +95
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 261 28 +233

Security concerns:

  • [medium] Redact provider hook errors before logging — src/agents/embedded-agent-runner/tool-schema-runtime.ts:146
    The formatter returns raw error text or calls String(error) before warning metadata is built; provider/plugin errors may contain credentials, and hostile thrown values can crash the warning path.
    Confidence: 0.82

What I checked:

Likely related people:

  • vincentkoc: Commit 39d2a71 added provider family replay and tool hooks, and this PR plus the sibling branch are in the same provider schema hook failure area. (role: provider hook contributor and current branch author with prior area history; confidence: high; commits: 39d2a719c964, 3662c8cea828, 6e20a32791b8; files: src/plugin-sdk/provider-tools.ts, src/plugins/provider-runtime.ts, src/agents/embedded-agent-runner/tool-schema-runtime.ts)
  • steipete: Commit 057be10 changed strict tool-schema and provider handle reuse across runtime-plan, embedded runner tests, and provider runtime files. (role: recent adjacent runtime/provider contributor; confidence: medium; commits: 057be10e5b46; files: src/agents/runtime-plan/tools.ts, src/agents/embedded-agent-runner/tool-schema-runtime.test.ts, src/plugins/provider-runtime.ts)
  • jalehman: Commit 7134694 added provider replay runtime hook surfaces in provider runtime and plugin contract files adjacent to this hook contract. (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.

@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 3, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 14, 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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. 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 added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 1, 2026
@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 commands Command implementations maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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