Skip to content

fix(plugins): stabilize plugin tool schemas#91291

Closed
vincentkoc wants to merge 4 commits into
mainfrom
split-plugin-tool-schema-stability-20260608
Closed

fix(plugins): stabilize plugin tool schemas#91291
vincentkoc wants to merge 4 commits into
mainfrom
split-plugin-tool-schema-stability-20260608

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Snapshots plugin tool schemas and tool metadata before plugin-owned values can mutate or throw during later registry/MCP use.
  • Guards implicit registration names and provider SDK schema helpers so plugin tooling keeps clear ownership and stable diagnostics.
  • Cherry-picked from the superseded large split stack in fix(runtime): snapshot tool definitions #90411: 2a872029f02f, 068518b4f117, 8858b44278b5, and 03f859237a88.

Verification

  • node scripts/run-vitest.mjs run src/plugins/tools.optional.test.ts src/plugins/loader.test.ts src/plugin-sdk/provider-tools.test.ts src/mcp/plugin-tools-handlers.test.ts --reporter=dot
  • ./node_modules/.bin/oxfmt --check src/mcp/plugin-tools-handlers.test.ts src/mcp/plugin-tools-handlers.ts src/plugin-sdk/provider-tools.test.ts src/plugin-sdk/provider-tools.ts src/plugins/loader.test.ts src/plugins/registry.ts src/plugins/tool-metadata.ts src/plugins/tools.optional.test.ts src/plugins/tools.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/mcp/plugin-tools-handlers.test.ts src/mcp/plugin-tools-handlers.ts src/plugin-sdk/provider-tools.test.ts src/plugin-sdk/provider-tools.ts src/plugins/loader.test.ts src/plugins/registry.ts src/plugins/tool-metadata.ts src/plugins/tools.optional.test.ts src/plugins/tools.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Behavior addressed

  • Plugin tool registration, provider SDK helper output, and MCP plugin-tool bridge metadata now preserve stable schema/name snapshots instead of re-reading mutable or hostile plugin values later.

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 the plugin/schema commits.

Evidence after fix

  • 238 focused tests passed across plugin registration, provider SDK tool helpers, bundled plugin behavior, and MCP plugin tool handlers.
  • Focused format, core oxlint, whitespace check, and autoreview passed.

Observed result after fix

  • Plugin tool schemas and metadata are copied at the boundary, preserving ownership while preventing later getter/mutation traps from changing behavior.

What was not tested

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

@openclaw-barnacle openclaw-barnacle Bot added size: XL maintainer Maintainer-authored PR labels Jun 8, 2026
@clawsweeper

clawsweeper Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 2:59 PM ET / 18:59 UTC.

Summary
This PR snapshots plugin tool schema/name/metadata reads across plugin resolution, implicit registration, provider SDK tool helpers, and the MCP plugin-tool bridge, with focused regression coverage.

PR surface: Source +447, Tests +420. Total +867 across 9 files.

Reproducibility: yes. from source: current main directly reads plugin-owned tool fields in plugin resolution, provider helper normalization/inspection, and MCP tool listing/wrapping, so throwing accessors can still abort those paths. No runtime command was run because this review is read-only.

Review metrics: 2 noteworthy metrics.

  • Provider SDK helper behavior: 3 provider families changed. Gemini, OpenAI, and DeepSeek helper semantics are exported through openclaw/plugin-sdk/provider-tools, so maintainers should accept the quarantine behavior before merge.
  • Current-main plugin metadata contract: 1 metadata field to preserve. Current main tracks replaySafe in plugin tool metadata, and the rebase must carry that field into the extracted metadata helper.

Stored data model
Persistent data-model change detected: persistent cache schema: src/plugins/loader.test.ts, vector/embedding metadata: src/mcp/plugin-tools-handlers.test.ts, vector/embedding metadata: src/mcp/plugin-tools-handlers.ts, vector/embedding metadata: src/plugins/tool-metadata.ts, vector/embedding metadata: src/plugins/tools.optional.test.ts, vector/embedding metadata: src/plugins/tools.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
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:

  • Rebase onto current main while preserving replaySafe and request-local plugin metadata behavior.
  • Rerun focused plugin, Plugin SDK, and MCP tests plus the changed gate after rebase.
  • Get explicit maintainer acceptance for the exported provider-helper quarantine semantics.

Risk before merge

  • [P1] GitHub reports the PR as dirty/conflicting, so the rebase must preserve current-main request-local tool discovery and replaySafe plugin metadata while applying the snapshotting changes.
  • [P1] The PR changes exported Plugin SDK provider helper behavior for unreadable plugin tool fields: normalization skips those tools while inspection emits diagnostics, which is a third-party plugin compatibility decision.
  • [P1] The focused test/lint proof in the PR body predates the current base conflict and needs to be rerun after rebase before merge.

Maintainer options:

  1. Rebase And Preserve Tool Metadata (recommended)
    Resolve the plugin resolver and registry conflicts while keeping current-main replaySafe and request-local metadata behavior, then rerun focused checks and the changed gate.
  2. Accept Provider Helper Quarantine
    Maintainers can explicitly accept that exported provider-family helpers skip unreadable tools during normalization and report diagnostics during inspection.
  3. Split SDK Semantics If Needed
    If the provider-helper contract is not ready, separate the internal plugin/MCP snapshotting repair from the exported Plugin SDK helper behavior.

Next step before merge

  • [P2] Manual maintainer review is required because this protected maintainer PR is dirty/conflicting and changes exported Plugin SDK compatibility semantics.

Security
Cleared: No workflow, dependency, lockfile, package-resolution, secret-handling, install-script, generated/vendor, or supply-chain concern was found in the inspected diff.

Review details

Best possible solution:

Rebase the split onto current main, preserve replaySafe/request-local plugin metadata semantics, rerun focused plugin/SDK/MCP checks plus the changed gate, and land only if maintainers accept the provider-helper quarantine contract.

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

Yes from source: current main directly reads plugin-owned tool fields in plugin resolution, provider helper normalization/inspection, and MCP tool listing/wrapping, so throwing accessors can still abort those paths. No runtime command was run because this review is read-only.

Is this the best way to solve the issue?

Yes after rebase and maintainer acceptance: snapshotting at the plugin, provider-helper, registration, and MCP boundaries is the narrow maintainable fix. It is not merge-ready until the stale-base conflict and exported SDK compatibility decision are resolved.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded plugin/runtime robustness fix with normal-priority impact across plugin resolver, Plugin SDK, and MCP surfaces.
  • merge-risk: 🚨 compatibility: The PR changes exported Plugin SDK helper behavior and must preserve current-main plugin metadata semantics during conflict resolution.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • 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 because this is a MEMBER-authored, maintainer-labeled PR, though the PR body records focused local validation.
Evidence reviewed

PR surface:

Source +447, Tests +420. Total +867 across 9 files.

View PR surface stats
Area Files Added Removed Net
Source 5 552 105 +447
Tests 4 421 1 +420
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 9 973 106 +867

What I checked:

  • Repository policy applied: Root AGENTS.md was read fully and its ClawSweeper/plugin-API compatibility guidance applies because this PR touches plugin resolver, Plugin SDK, provider helper, and MCP tool surfaces. (AGENTS.md:30, edd1d3319cce)
  • Scoped plugin policy applied: The plugins scoped guide says loader and registry behavior must stay aligned with documented Plugin SDK and manifest contracts, making the registry/tool changes a public plugin-boundary review item. (src/plugins/AGENTS.md:3, edd1d3319cce)
  • Scoped SDK policy applied: The Plugin SDK scoped guide identifies src/plugin-sdk as the public contract between plugins and core, so changing provider helper quarantine behavior is compatibility-sensitive. (src/plugin-sdk/AGENTS.md:1, edd1d3319cce)
  • Live PR state: GitHub reports this PR open, non-draft, author association MEMBER, head 80980fe, base 5c53918, mergeable false, and mergeable_state dirty. (80980fe49566)
  • Current-main plugin resolver still reads tool fields directly: Current main reads plugin-owned tool.name directly in readPluginToolName and later reads tool.name/parameters/execute during filtering, validation, conflict detection, metadata attachment, and cached descriptor execution, so throwing getters remain source-reproducible on main. (src/plugins/tools.ts:328, edd1d3319cce)
  • Current-main provider helpers still read schemas directly: Gemini, OpenAI, and DeepSeek provider helper normalizers/inspectors read tool.parameters and tool.name directly, while docs expose buildProviderToolCompatFamilyHooks through openclaw/plugin-sdk/provider-tools. (src/plugin-sdk/provider-tools.ts:67, edd1d3319cce)

Likely related people:

  • vincentkoc: Authored this focused split, the superseded broad snapshot stack, and recent plugin/MCP tool work touching the same surfaces beyond this PR. (role: current split owner and recent area contributor; confidence: high; commits: 858a390dc24d, 6d5c0f91337c, 71b13594c244; files: src/plugins/tools.ts, src/plugins/registry.ts, src/plugin-sdk/provider-tools.ts)
  • steipete: Recent history shows repeated plugin resolver, Plugin SDK, MCP handler, and public helper documentation work around the affected surfaces. (role: adjacent owner and recent contributor; confidence: high; commits: 4d54d196c99a, 82a0ba8c4c90, d99c824ac1db; files: src/plugins/tools.ts, src/plugin-sdk/provider-tools.ts, src/mcp/plugin-tools-handlers.ts)
  • fuller-stack-dev: Authored current-main replaySafe plugin metadata work that intersects this PR's extraction of plugin tool metadata during rebase. (role: adjacent metadata contributor; confidence: medium; commits: 77a682c5def2; files: src/plugins/tools.ts)
  • medns: Recent current-main request-local plugin tool discovery work changed src/plugins/tools.ts after this PR's base and contributes to the conflict surface. (role: recent plugin resolver contributor; confidence: medium; commits: 0781dae6204e; files: src/plugins/tools.ts)
  • joshavant: Recent MCP tool handler work in the same bridge path is relevant to reviewing MCP tool wrapping/listing behavior. (role: recent MCP handler contributor; confidence: medium; commits: b7d61c8daffc; files: src/mcp/plugin-tools-handlers.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.

@vincentkoc vincentkoc mentioned this pull request Jun 8, 2026
Closed
@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 8, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 15, 2026
This was referenced Jun 18, 2026
Open
Closed
Closed
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing this as part of the old fuzz-hardening cleanup. The branch only proves synthetic hostile-object states such as unreadable getters, proxies, or fabricated oversized collections; it has no linked production report or supported contract requiring those states.

The proposed fail-soft behavior adds compatibility branches at downstream consumers, and several PRs in this cluster overlap. If a real plugin or provider reproduces the failure, please file the observed payload and stack so one canonical registration/parsing boundary can own validation. 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

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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XL 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