Skip to content

fix(agents): ignore unreadable session tool metadata#89513

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next30-20260602
Closed

fix(agents): ignore unreadable session tool metadata#89513
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next30-20260602

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Guard SDK custom-tool and extension-tool metadata reads so unreadable name / schema metadata skips the malformed tool instead of crashing session startup or refresh.
  • Snapshot readable tool definitions before wrapping while preserving the original execution path for valid tools.
  • Sanitize synthetic SDK/builtin tool source labels used in session tool metadata.

Verification

  • node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts src/agents/sessions/extensions/loader.test.ts -- --reporter=dot
  • ./node_modules/.bin/oxfmt --check src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/extensions/runner.ts src/agents/sessions/extensions/wrapper.ts src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/sdk.test.ts src/agents/sessions/tools/index.ts
  • ./node_modules/.bin/oxlint src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/extensions/runner.ts src/agents/sessions/extensions/wrapper.ts src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/sdk.test.ts src/agents/sessions/tools/index.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • AWS Crabbox pnpm check:changed: provider aws, lease cbx_bebed30ed07e, run run_b3a68dec96fd, exit 0

Real behavior proof

Behavior addressed: Session startup and tool-registry refresh skip malformed SDK custom tools or extension tools with unreadable registry metadata instead of throwing before a valid tool registry can be built.

Real environment tested: Local macOS sparse gwt worktree with shared repo dependencies; AWS Crabbox Linux c7a.8xlarge fresh PR checkout at head b74a7ed8d7c.

Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts src/agents/sessions/extensions/loader.test.ts -- --reporter=dot; env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed on AWS Crabbox.

Evidence after fix: sdk.test.ts covers SDK custom tools and already-registered extension tools with throwing name / parameters getters; loader.test.ts covers production api.registerTool registration with a throwing name getter; remote check:changed selected core, coreTests and completed successfully.

Observed result after fix: Focused Vitest shards passed, formatter/lint/diff checks passed, autoreview reported no accepted/actionable findings, and AWS Crabbox check:changed exited 0.

What was not tested: No live extension package was installed; coverage uses session/loader regression tests around the registration and registry paths.

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

clawsweeper Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 29, 2026, 8:56 PM ET / 00:56 UTC.

Summary
The PR changes agent session tool registration and refresh to skip unreadable SDK and extension tool definitions, snapshots readable definitions before wrapping, sanitizes synthetic source labels, and adds focused regression tests.

PR surface: Source +118, Tests +130. Total +248 across 9 files.

Reproducibility: yes. Source inspection shows current main and v2026.6.10 directly read tool.name, definition.name, and schema metadata during extension registration and session registry refresh; I did not execute tests in this read-only review.

Review metrics: 1 noteworthy metric.

  • Session SDK helper exports: 5 added. These helpers become reachable through the public agent-sessions SDK subpath, so maintainers should review the compatibility surface before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/agent-session.ts, serialized state: src/agents/sessions/extensions/loader.test.ts, serialized state: src/agents/sessions/sdk.test.ts, serialized state: src/agents/sessions/tools/tool-definition-wrapper.ts, unknown-data-model-change: src/agents/sessions/agent-session.ts, unknown-data-model-change: src/agents/sessions/sdk.test.ts, and 1 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
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 the wrapper/snapshot helper exports from src/agents/sessions/tools/index.ts.
  • Choose the canonical malformed-tool behavior across the related open branches.
  • Refresh the draft branch against current main before final merge review.

Risk before merge

  • [P1] The new tools-barrel exports would make internal snapshot/wrapper helpers reachable through openclaw/plugin-sdk/agent-sessions, creating an unintended plugin API compatibility contract.
  • [P1] The PR changes malformed SDK and extension tool behavior from crash/fail paths to silent skips in some paths, while related open PRs choose diagnostics quarantine or fail-closed validation.
  • [P1] The PR is a draft based on an older base SHA, so the final merge result should be reconciled with current main before landing.

Maintainer options:

  1. Keep Helpers Internal (recommended)
    Remove the new tools-barrel exports and switch internal callers to direct imports so this hardening does not create a new plugin SDK contract.
  2. Approve The SDK Surface
    Maintainers can intentionally keep the helper exports only if they want them supported through openclaw/plugin-sdk/agent-sessions with matching docs and compatibility coverage.
  3. Pause For Contract Choice
    Pause or reconcile this PR until maintainers choose between silent skip, diagnostics quarantine, and fail-closed malformed-tool handling across the related branches.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Keep the unreadable tool metadata guard and regression tests, but remove the new exports for createToolDefinitionFromAgentTool, readToolDefinitionName, snapshotReadableToolDefinition, wrapToolDefinition, and wrapToolDefinitions from src/agents/sessions/tools/index.ts; update src/agents/sessions/sdk.ts to import readToolDefinitionName from src/agents/sessions/tools/tool-definition-wrapper.ts directly. Do not change the malformed-tool policy in this repair.

Next step before merge

  • [P2] A narrow automated repair can remove the accidental SDK exports; final merge still needs maintainer contract review and branch reconciliation.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes trusted local session runtime/tests and does not touch dependencies, workflows, permissions, packaging, or secrets.

Review findings

  • [P1] Keep wrapper helpers out of the SDK barrel — src/agents/sessions/tools/index.ts:72-78
Review details

Best possible solution:

Keep the metadata hardening, remove accidental public SDK helper exports, and land one maintainer-approved malformed-tool policy across the overlapping branches.

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

Yes. Source inspection shows current main and v2026.6.10 directly read tool.name, definition.name, and schema metadata during extension registration and session registry refresh; I did not execute tests in this read-only review.

Is this the best way to solve the issue?

No, not as submitted. The session hardening boundary is plausible, but the helper exports should stay internal and maintainers still need to choose the intended malformed-tool contract.

Full review comments:

  • [P1] Keep wrapper helpers out of the SDK barrel — src/agents/sessions/tools/index.ts:72-78
    Exporting these wrapper/snapshot helpers from tools/index.ts makes internal session-tool implementation details reachable through openclaw/plugin-sdk/agent-sessions. Runtime callers can import tool-definition-wrapper.ts directly, so remove these exports unless this PR intentionally documents and tests a new public SDK contract.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded agents/session robustness PR with limited blast radius and no evidence of an urgent live outage.
  • merge-risk: 🚨 compatibility: The diff changes malformed tool handling and adds public SDK-barrel helper exports that can affect plugin-facing compatibility.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit 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 to this maintainer-labeled MEMBER PR, though the body reports focused Vitest, lint/format, autoreview, and AWS Crabbox changed-gate proof.
Evidence reviewed

PR surface:

Source +118, Tests +130. Total +248 across 9 files.

View PR surface stats
Area Files Added Removed Net
Source 7 141 23 +118
Tests 2 133 3 +130
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 9 274 26 +248

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts src/agents/sessions/extensions/loader.test.ts -- --reporter=dot.
  • [P1] ./node_modules/.bin/oxfmt --check src/agents/sessions/tools/index.ts src/agents/sessions/sdk.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/agent-session.ts src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/wrapper.ts src/agents/sessions/extensions/runner.ts.
  • [P1] ./node_modules/.bin/oxlint src/agents/sessions/tools/index.ts src/agents/sessions/sdk.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/agent-session.ts src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/wrapper.ts src/agents/sessions/extensions/runner.ts.
  • [P1] git diff --check.

What I checked:

  • Current main extension registration still reads tool.name directly: At origin/main, registerTool stores extension tools with extension.tools.set(tool.name, ...), so an unreadable name getter remains source-reproducible on current main. (src/agents/sessions/extensions/loader.ts:227, 587eefe5ad91)
  • Current main session refresh still reads SDK and extension tool metadata directly: refreshToolRegistry builds custom tool entries from definition.name and later filters by tool.definition.name, so unreadable SDK or extension definitions can still affect session startup or refresh. (src/agents/sessions/agent-session.ts:2426, 587eefe5ad91)
  • Latest release has the same unfixed behavior: v2026.6.10 still reads tool.name in extension registration and definition.name in session refresh, so this PR is not superseded by a shipped fix. (src/agents/sessions/extensions/loader.ts:227, aa69b12d0086)
  • PR head adds internal wrapper helpers to the tools barrel: The PR adds five exports from tool-definition-wrapper.ts to tools/index.ts, making snapshot and wrapper helpers part of the tools barrel. (src/agents/sessions/tools/index.ts:72, b74a7ed8d7c9)
  • The tools barrel is exported through the public agent-sessions SDK surface: extension-sdk.ts re-exports ./tools/index.js, and package.json exposes openclaw/plugin-sdk/agent-sessions, so new tools-barrel exports become plugin-facing. (src/agents/sessions/extension-sdk.ts:43, 587eefe5ad91)
  • Plugin SDK policy makes this export compatibility-sensitive: The scoped plugin SDK policy says not to expose implementation convenience from src/agents internals unless intentionally promoting a supported public contract. (src/plugin-sdk/AGENTS.md:22, 587eefe5ad91)

Likely related people:

  • steipete: GitHub path history shows major agent runtime internalization plus extension loader and session tools barrel work that established the affected SDK/runtime surfaces. (role: feature owner and SDK boundary introducer; confidence: high; commits: bb46b79d3c14, 57a3dbe73639, 600a57e60f1e; files: src/agents/sessions/extensions/loader.ts, src/agents/sessions/tools/tool-definition-wrapper.ts, src/agents/sessions/tools/index.ts)
  • vincentkoc: They authored this PR and the related malformed tool hardening branches, and recent path history shows current-main session work in the affected area. (role: recent area contributor and likely follow-up owner; confidence: high; commits: b74a7ed8d7c9, 179ab368b746, f372abd07e42; files: src/agents/sessions/agent-session.ts, src/agents/sessions/sdk.ts, src/agents/sessions/extensions/loader.ts)
  • shakkernerd: Recent session history includes active-tool and base-prompt work that consumes the tool registry and prompt metadata behavior this PR changes. (role: adjacent session registry contributor; confidence: medium; commits: 73cf516def3d; files: src/agents/sessions/agent-session.ts, src/agents/sessions/sdk.test.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 force-pushed the fuzz-tool-schema-next30-20260602 branch from 8ad0c58 to be3e85a Compare June 2, 2026 15:39
@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. labels Jun 2, 2026
@vincentkoc
vincentkoc force-pushed the fuzz-tool-schema-next30-20260602 branch from be3e85a to c62833d Compare June 2, 2026 15:43
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. P2 Normal backlog priority with limited blast radius. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 2, 2026
@vincentkoc
vincentkoc force-pushed the fuzz-tool-schema-next30-20260602 branch from c62833d to b74a7ed Compare June 2, 2026 15:52
@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. proof: sufficient ClawSweeper judged the real behavior proof convincing. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 2, 2026
This was referenced Jun 15, 2026
Closed
Closed
@clawsweeper clawsweeper Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 19, 2026
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this one. The nine-file change is based on custom/extension tool objects with poisoned typed fields and no installed extension or real startup failure.

Session startup should not accumulate catch-and-skip metadata rules across loader, wrapper, runner, SDK, and registry. If an untyped extension boundary is observed to admit bad data, validate it once there with an explicit diagnostic. Thanks for mapping the full path.

@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 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