Skip to content

fix(agents): quarantine invalid session custom tools#89006

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-call-context-20260601
Closed

fix(agents): quarantine invalid session custom tools#89006
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-call-context-20260601

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

  • Quarantines SDK and extension-registered custom tools whose definitions cannot be read safely or whose parameter schemas are unsupported by the runtime projection.
  • Materializes session custom tool definitions before registry activation so hostile getters or unsupported schema keywords cannot poison session startup.
  • Preserves valid custom tool behavior, including method receivers, disabled-tool allowlist behavior, and client tools that omit a parameter schema.
  • Intentionally out of scope: builtin tool schema handling and provider-specific tool projection, which are already covered by the existing runtime filter path.
  • Reviewers should focus on the custom-tool materialization boundary in AgentSession.refreshToolRegistry and whether the diagnostic surface is the right place to report rejected extension tools.

Linked context

Closes #

Related #88994

Was this requested by a maintainer or owner?

Maintainer fuzzing follow-up for unsupported runtime/tool schema hardening.

Real behavior proof (required for external PRs)

  • Behavior addressed: unsupported or unreadable SDK/extension custom tool definitions are quarantined before they enter the active session tool registry.
  • Real environment tested: local macOS checkout for focused Vitest/lint/format proof; AWS Crabbox Linux remote for changed-gate proof.
  • Exact steps or command run after this patch:
    • node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts --reporter=dot
    • OPENCLAW_OXLINT_SKIP_LOCK=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.json src/agents/sessions/agent-session.ts src/agents/sessions/sdk.ts src/agents/sessions/sdk.test.ts
    • ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/agent-session.ts src/agents/sessions/sdk.ts src/agents/sessions/sdk.test.ts
    • git diff --check origin/main...HEAD
    • node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed"
  • Evidence after fix: focused Vitest reported 1 passed file and 12 passed tests; oxlint reported Found 0 warnings and 0 errors; oxfmt reported all matched files use the correct format; AWS Crabbox run run_fc68d25ad837 on lease cbx_9ebbab4a3dfb exited 0 with check:changed lanes core, coreTests.
  • Observed result after fix: bad SDK and extension custom tools are omitted while healthy custom tools remain active; disabled bad tools are not inspected; method-backed custom tools keep their receiver; no-parameter client tools stay active.
  • What was not tested: live provider execution against an external model, and unrelated builtin tool schema behavior.
  • Proof limitations or environment constraints: Blacksmith Testbox was unavailable locally because the CLI was not authenticated, so the broad changed gate used direct AWS Crabbox instead.
  • Before evidence (optional but encouraged): local fuzzing showed session custom tools bypassed runtime schema projection before this patch.

Tests and validation

Which commands did you run?

  • node scripts/run-vitest.mjs src/agents/sessions/sdk.test.ts --reporter=dot
  • OPENCLAW_OXLINT_SKIP_LOCK=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.json src/agents/sessions/agent-session.ts src/agents/sessions/sdk.ts src/agents/sessions/sdk.test.ts
  • ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/agent-session.ts src/agents/sessions/sdk.ts src/agents/sessions/sdk.test.ts
  • git diff --check origin/main...HEAD
  • Private marker scrub over touched files and the local fuzzing spec.
  • node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed"

What regression coverage was added or updated?

  • SDK custom tool with unsupported runtime schema is quarantined.
  • SDK custom tool with unreadable name getter does not crash startup.
  • Disabled SDK custom tools are not schema-inspected.
  • Extension-registered custom tool with unsupported runtime schema is quarantined.
  • Materialized custom tools preserve method receivers.
  • Client tools without parameter schemas remain active.

What failed before this fix, if known?

Session custom tools could enter the registry without the runtime-compatible schema filter used by assistant/model tool projection.

If no test was added, why not?

Tests were added.

Risk checklist

Did user-visible behavior change? (Yes/No)

Yes.

Did config, environment, or migration behavior change? (Yes/No)

No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

Yes.

What is the highest-risk area?

Accidentally rejecting valid custom tools while quarantining bad definitions.

How is that risk mitigated?

The tests cover valid SDK tools, extension tools, method-backed tools, disabled tools, and client tools with omitted parameter schemas. The filter reuses the existing runtime-compatible tool projection instead of inventing a second schema policy.

Current review state

What is the next action?

Maintainer review and CI.

What is still waiting on author, maintainer, CI, or external proof?

CI after the draft PR opens.

Which bot or reviewer comments were addressed?

Local autoreview found and the patch addressed three issues before PR creation: method receivers are preserved, disabled tools are not inspected, and client tools without parameter schemas remain active. Final autoreview was clean.

AI-assisted: yes.

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

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed June 27, 2026, 8:38 AM ET / 12:38 UTC.

Summary
The PR adds session custom-tool materialization and runtime-schema filtering for invalid SDK and extension-registered custom tools.

PR surface: Source +283, Tests +177. Total +460 across 3 files.

Reproducibility: yes. Current main directly reads SDK custom-tool names in createAgentSession, extension tool names in registerTool(), and extension definition names in getAllRegisteredTools(), so hostile getters can be traced from source without running tests.

Review metrics: 1 noteworthy metric.

  • Custom tool ingress behavior: 2 ingress paths changed. SDK customTools and extension-registered tools move toward quarantine semantics, which is the compatibility decision maintainers need to notice before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/agent-session.ts, serialized state: src/agents/sessions/sdk.test.ts, serialized state: src/agents/sessions/sdk.ts, unknown-data-model-change: src/agents/sessions/agent-session.ts, unknown-data-model-change: src/agents/sessions/sdk.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
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:

  • Move extension-tool snapshot/quarantine to the real registration or enumeration boundary, or narrow this PR to SDK/runtime-schema filtering.
  • Refresh the branch against current main and rerun exact-head CI.

Risk before merge

  • [P1] The PR intentionally changes invalid or unsupported SDK and extension custom tools from previously reachable or fail-fast behavior to fail-closed quarantine, which maintainers need to accept as a compatibility choice.
  • [P1] The extension side is filtered after registerTool() and getAllRegisteredTools(), so unreadable extension names can still abort before the new diagnostic path runs.
  • [P1] The branch is draft and GitHub reports it dirty/conflicting, so exact-head CI and the final three-way merge result are not current.
  • [P1] Open sibling maintainer branches cover SDK-only and broader session/extension snapshotting, so landing multiple paths without selection could create competing quarantine semantics.

Maintainer options:

  1. Refresh and complete this branch
    Resolve the dirty merge state and move extension-tool snapshotting to the real registration or enumeration boundary before this branch is considered for merge.
  2. Adopt a canonical sibling
    Maintainers can choose fix(agents): snapshot SDK custom tools safely #89404 for SDK-only hardening or fix(agents): snapshot session tool definitions #91293 for broader hardening only after that chosen branch is clean and its review blockers are resolved.
  3. Pause overlapping drafts
    If the permanent SDK/session/extension quarantine contract is not settled, keep the drafts open but do not merge competing partial implementations.

Next step before merge

  • [P1] Protected maintainer PR with a compatibility-sensitive fail-closed behavior change, dirty head, and overlapping branches needs maintainer selection rather than automated repair.

Security
Cleared: The diff hardens tool-registration behavior and does not add dependencies, workflows, secret handling, downloads, permissions, or broader supply-chain surface.

Review findings

  • [P2] Snapshot extension tools at registration — src/agents/sessions/agent-session.ts:2660-2662
Review details

Best possible solution:

Land one refreshed canonical session-tool snapshot/quarantine implementation that guards SDK and extension registration boundaries before unsafe reads, preserves valid tools, and explicitly accepts the fail-closed compatibility impact.

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

Yes. Current main directly reads SDK custom-tool names in createAgentSession, extension tool names in registerTool(), and extension definition names in getAllRegisteredTools(), so hostile getters can be traced from source without running tests.

Is this the best way to solve the issue?

No as written. The SDK-side mitigation is plausible, but the extension-side fix belongs at registration/snapshot ingress or should be narrowed out of this PR before merge.

Full review comments:

  • [P2] Snapshot extension tools at registration — src/agents/sessions/agent-session.ts:2660-2662
    This filter starts from getAllRegisteredTools(), but real extension registration and enumeration still read tool.name before the new materializer can diagnose or quarantine it. A hostile extension tool name can still crash before this branch’s filter runs, and the new test bypasses api.registerTool() by building the registered-tool map directly.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority agent/session hardening PR with real but limited custom-tool blast radius.
  • merge-risk: 🚨 compatibility: The diff can reject SDK or extension custom tools that previously registered, stayed reachable, or failed differently.
  • merge-risk: 🚨 availability: Filtering too late can leave session startup crashes unresolved, while over-filtering can make existing custom tools unavailable.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster 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 member PR, though the body includes focused local test/lint/format output and AWS Crabbox changed-gate proof.
Evidence reviewed

PR surface:

Source +283, Tests +177. Total +460 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 290 7 +283
Tests 1 202 25 +177
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 492 32 +460

What I checked:

  • Repository policy applied: Root and scoped agents policy were read; the review applied the protected maintainer item rule, compatibility-risk guidance for fail-closed behavior, and best-fix requirements for agent/session PRs. (AGENTS.md:1, c5d34c8376f8)
  • Current main SDK unsafe read: Current main still derives active custom tools by directly reading each SDK custom tool name, so a throwing name getter is source-reproducible before the PR fix. (src/agents/sessions/sdk.ts:351, c5d34c8376f8)
  • Current main extension registration unsafe read: Current main registerTool() stores extension tools by reading tool.name, which happens before this PR's later session-refresh filter could quarantine or diagnose an unreadable name. (src/agents/sessions/extensions/loader.ts:227, c5d34c8376f8)
  • Current main extension enumeration unsafe read: Current main getAllRegisteredTools() de-duplicates extension tools by reading tool.definition.name, another pre-filter extension path that can still throw. (src/agents/sessions/extensions/runner.ts:398, c5d34c8376f8)
  • PR filter runs too late for extension unreadable names: At PR head, the new filter starts from currentExtensionRunner.getAllRegisteredTools(), so it runs after extension registration and enumeration have already read registered tool names. (src/agents/sessions/agent-session.ts:2660, d21b95e17235)
  • PR extension test bypasses real registration: The PR's extension-tool helper manually constructs the registered-tool map and does not exercise api.registerTool(), so it misses the earlier extension boundary that still reads tool.name. (src/agents/sessions/sdk.test.ts:68, d21b95e17235)

Likely related people:

  • vincentkoc: Recent current-main history touches SDK/session/tool-schema projection, and this login also owns the overlapping malformed tool hardening branches beyond merely opening this PR. (role: recent area contributor and likely follow-up owner; confidence: high; commits: 9dbf8f718ff8, 68ec783e74b5, 664948e7bfd7; files: src/agents/sessions/agent-session.ts, src/agents/sessions/sdk.ts, src/agents/tool-schema-projection.ts)
  • steipete: Recent history shows major agent runtime internalization plus extension-loader and direct/custom tool-schema quarantine work on adjacent boundaries. (role: major runtime refactor and adjacent boundary owner; confidence: high; commits: bb46b79d3c14, 00d8d7ead059, a02813164dd5; files: src/agents/sessions/agent-session.ts, src/agents/sessions/extensions/loader.ts, src/agents/tool-schema-projection.ts)
  • shakkernerd: Recent session work preserved embedded base prompts through active tool selection, 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 tool-search directory mode work changed tool exposure and custom/client tool paths near the SDK customTools behavior reviewed here. (role: adjacent SDK/custom-tool contributor; confidence: medium; commits: 965fa05df38b; files: src/agents/sessions/sdk.ts, src/agents/embedded-agent-runner/run/attempt.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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 1, 2026
This was referenced Jun 1, 2026
Closed
Closed
@clawsweeper clawsweeper Bot added 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. 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. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 20, 2026
Closed
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. 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. proof: sufficient ClawSweeper judged the real behavior proof convincing. labels Jun 21, 2026
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this fuzz-only hardening PR. The failure is constructed with hostile in-process objects, throwing getters, or similarly malformed metadata; there is no linked production report or external contract requiring that state. The patch spreads fail-soft handling through consumers and adds substantial branches/tests for a synthetic case instead of validating once at the real registration or parsing boundary.

If an observed plugin/provider failure demonstrates that malformed external data can cross a supported boundary, please open a focused issue with the real payload and stack. The right fix would be canonical boundary validation with diagnostics, not per-consumer fallback behavior. Thanks for exploring the surface.

@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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. 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: L 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