Skip to content

fix(agents): snapshot SDK custom tools safely#89404

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

fix(agents): snapshot SDK custom tools safely#89404
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-surfaces-next15-20260602

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Snapshot SDK custom tool definitions before session startup and registry refresh code reads caller-owned fields.
  • Drop unreadable custom tool descriptors so one hostile getter cannot crash createAgentSession or block healthy sibling tools.
  • Preserve callable tool receivers when wrapping execute, renderers, and argument preparation.

This is scoped to SDK/session custom tool ingestion. It does not change provider-side schema normalization or extension registration policy.

Linked context

No tracked issue. Part of the tool-schema/runtime ingress fuzzing sweep.

Real behavior proof (required for external PRs)

  • Behavior addressed: SDK custom tools with throwing descriptor getters no longer crash session setup; valid sibling tools remain registered and active.
  • Real environment tested: local OpenClaw Codex worktree on macOS, using the repo Vitest wrapper.
  • Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts -- --reporter=dot; ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/sdk.test.ts; OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/sdk.test.ts; git diff --check origin/main...HEAD; .agents/skills/autoreview/scripts/autoreview --mode local; AWS Crabbox fresh PR OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed.
  • Evidence after fix: src/agents/sessions/sdk.test.ts passed 8/8 after the rebase; oxfmt, targeted oxlint, diff-check, and autoreview were clean. AWS Crabbox run_48879f533e2f on lease cbx_7c36dafccedd (c7a.8xlarge) passed pnpm check:changed with lanes core, coreTests, exit 0.
  • Observed result after fix: unreadable name, promptGuidelines, and parameters getters are dropped before startup while healthy_lookup remains active; stateful custom tool methods keep their original receiver.
  • What was not tested: full repo-wide test suite.
  • Proof limitations or environment constraints: local core-test tsgo could not run from the sparse Codex worktree because required ui/config inputs are not present there; the same lane passed in the full Crabbox fresh PR checkout.
  • Before evidence: the previous SDK code mapped options.customTools?.map((tool) => tool.name) directly, so a throwing name getter crashed before AgentSession could be built.

Tests and validation

Focused commands run:

  • node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts -- --reporter=dot
  • ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/sdk.test.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/agents/sessions/sdk.ts src/agents/sessions/agent-session.ts src/agents/sessions/tools/tool-definition-wrapper.ts src/agents/sessions/sdk.test.ts
  • git diff --check origin/main...HEAD
  • .agents/skills/autoreview/scripts/autoreview --mode local

Regression coverage added:

  • Unreadable SDK custom tool descriptors are dropped before session setup.
  • Snapshot wrapping preserves the original receiver for stateful custom tool methods.

Risk checklist

  • Did user-visible behavior change? Yes
  • Did config, environment, or migration behavior change? No
  • Did security, auth, secrets, network, or tool execution behavior change? Yes
  • Highest-risk area: custom SDK tools implemented with stateful object methods.
  • Mitigation: callable fields are wrapped with Reflect.apply against the original descriptor, and a receiver-preservation regression test covers the compatibility shape.

Current review state

Next action: maintainer review.

Waiting on: CI/maintainer review.

Reviewer comments addressed: local $autoreview initially flagged receiver loss; the patch now preserves receivers and the rerun reported no accepted/actionable findings.

@vincentkoc vincentkoc self-assigned this Jun 2, 2026
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S 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 maintainer review before merge. Reviewed June 29, 2026, 8:25 PM ET / 00:25 UTC.

Summary
The PR snapshots SDK custom tool definitions before session startup and registry refresh, drops unreadable descriptors, preserves callable receivers, and adds SDK regression tests.

PR surface: Source +70, Tests +109. Total +179 across 4 files.

Reproducibility: yes. Source inspection on current main shows createAgentSession directly reads tool.name and AgentSession later rereads descriptor metadata, so hostile SDK getters can throw before or during session setup.

Review metrics: 1 noteworthy metric.

  • SDK customTools behavior: 1 public SDK option behavior changed. The customTools option moves from live caller-owned descriptor reads to session-ingress snapshots, which maintainers should notice before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/sdk.test.ts, serialized state: src/agents/sessions/sdk.ts, serialized state: src/agents/sessions/tools/tool-definition-wrapper.ts, unknown-data-model-change: src/agents/sessions/sdk.test.ts, unknown-data-model-change: src/agents/sessions/tools/tool-definition-wrapper.ts, vector/embedding metadata: src/agents/sessions/sdk.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
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:

  • Confirm the intended SDK customTools descriptor snapshot contract before taking the draft out of maintainer review.

Risk before merge

  • [P1] SDK customTools descriptors become snapshot-based instead of live caller-owned objects; maintainers should explicitly accept that compatibility contract before merge.
  • [P1] Several maintainer fuzzing branches overlap around malformed tool-definition quarantine policy, so landing should coordinate the narrow SDK path with the broader session/provider policy.

Maintainer options:

  1. Accept SDK Snapshot Semantics (recommended)
    Maintainers can intentionally accept that SDK customTools descriptors are read once at session ingress and post-start descriptor mutation is unsupported.
  2. Pause For Canonical Quarantine Branch
    If the permanent SDK/session/extension quarantine policy is not settled, keep this draft paused while a broader canonical branch is selected or rebased.

Next step before merge

  • [P2] The protected maintainer-labeled draft needs an SDK compatibility decision and coordination with overlapping hardening branches, not an automated repair.

Security
Cleared: No concrete security or supply-chain regression was found; the diff hardens caller-owned SDK descriptor ingestion without adding dependencies, workflows, permissions, secrets handling, or new tool execution capability.

Review details

Best possible solution:

Land one maintainer-approved custom-tool descriptor contract: accept process-stable SDK snapshots for this narrow PR, or reconcile it into a broader canonical quarantine branch before merging.

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

Yes. Source inspection on current main shows createAgentSession directly reads tool.name and AgentSession later rereads descriptor metadata, so hostile SDK getters can throw before or during session setup.

Is this the best way to solve the issue?

Unclear as a merge decision. Snapshotting at SDK/session ingress is a clean bounded fix if process-stable descriptors are accepted, but maintainers still need to choose that SDK compatibility contract against the broader branches.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded agent SDK/runtime hardening PR with limited blast radius around custom tool ingestion.
  • merge-risk: 🚨 compatibility: The PR changes how existing SDK customTools descriptor objects are observed after session creation.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster 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 to this maintainer-labeled MEMBER PR; the body reports focused tests, format/lint checks, local autoreview, and AWS Crabbox changed-gate proof.
Evidence reviewed

PR surface:

Source +70, Tests +109. Total +179 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 3 74 4 +70
Tests 1 109 0 +109
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 183 4 +179

What I checked:

Likely related people:

  • steipete: Path history shows the large agent runtime internalization and session tool documentation work that established the current session/tool wrapper structure. (role: major runtime refactor and adjacent boundary owner; confidence: high; commits: bb46b79d3c14, 600a57e60f1e; files: src/agents/sessions/sdk.ts, src/agents/sessions/agent-session.ts, src/agents/sessions/tools/tool-definition-wrapper.ts)
  • vincentkoc: Path history and PR context show recent session SDK work and multiple related malformed tool-schema hardening branches by the same login. (role: recent area contributor and likely follow-up owner; confidence: high; commits: 68ec783e74b5, 9dbf8f718ff8, 3025863a64aa; files: src/agents/sessions/sdk.ts, src/agents/sessions/agent-session.ts, src/agents/sessions/tools/tool-definition-wrapper.ts)
  • shakkernerd: Recent history touched AgentSession and sdk.test around active-tool prompt behavior, overlapping the registry and prompt metadata behavior this PR changes. (role: adjacent session-runtime contributor; confidence: medium; commits: 73cf516def3d; files: src/agents/sessions/agent-session.ts, src/agents/sessions/sdk.test.ts)
  • fuller-stack-dev: Recent SDK history changed tool exposure and custom/client tool paths near the customTools option behavior reviewed here. (role: adjacent SDK/custom-tool contributor; confidence: medium; commits: 965fa05df38b; files: src/agents/sessions/sdk.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-21T12:52:51.633Z sha 3025863 :: needs maintainer review before merge. :: none

@vincentkoc
vincentkoc force-pushed the fuzz-tool-schema-surfaces-next15-20260602 branch from 38301c5 to 3025863 Compare June 2, 2026 10:00
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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 2, 2026
This was referenced Jun 2, 2026
Closed
Closed
@clawsweeper clawsweeper Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 19, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Closed
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this one. The PR has no tracked issue and explicitly comes from the fuzzing sweep; its SDK tests manufacture mutable/throwing descriptor accessors rather than a real custom-tool integration.

Snapshotting and rebinding every SDK tool changes object identity and method semantics for a hypothetical state. Keep the typed SDK contract strict; add a narrow ingestion check only if a real external caller demonstrates the need. Thanks for investigating receiver preservation.

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