Skip to content

fix(agents): quarantine unreadable extension tools#89406

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

fix(agents): quarantine unreadable extension tools#89406
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-surfaces-next16-20260602

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Guard api.registerTool(...) so unreadable extension tool descriptors are skipped instead of failing the whole extension load.
  • Preserve healthy tools, commands, and stateful method-style tool receivers after snapshotting registered tools.
  • Propagate skipped-tool diagnostics through both path-based and inline extension loading.

This is scoped to extension registration. It does not add provider-specific schema compatibility checks or doctor runtime projection validation.

Linked context

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

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: one extension tool with a throwing descriptor getter no longer prevents the extension from loading or blocks healthy sibling registrations.
  • 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/extensions/loader.test.ts -- --reporter=dot; ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/resource-loader.ts; OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/resource-loader.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: focused loader test passed 2/2 after the rebase; oxfmt, targeted oxlint, diff-check, and autoreview were clean. AWS Crabbox run_009f40ac4028 on lease cbx_816e7d563d3a (c7a.8xlarge) passed pnpm check:changed with lanes core, coreTests, exit 0.
  • Observed result after fix: bad registered tools produce loader diagnostics, healthy healthy_lookup remains registered, the extension command registered after the bad tools remains present, and the healthy tool method keeps its original receiver.
  • What was not tested: full repo-wide test suite.
  • Proof limitations or environment constraints: local proof is focused on extension registration and loader diagnostics; Crabbox proved the changed core, coreTests lanes in a fresh PR checkout.
  • Before evidence: registerTool read tool.name directly and stored the raw definition, so a throwing name or later schema getter could fail extension loading or crash later registry reads.

Tests and validation

Focused commands run:

  • node scripts/run-vitest.mjs src/agents/sessions/extensions/loader.test.ts -- --reporter=dot
  • ./node_modules/.bin/oxfmt --check --threads=1 src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/resource-loader.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts src/agents/sessions/resource-loader.ts
  • git diff --check origin/main...HEAD
  • .agents/skills/autoreview/scripts/autoreview --mode local

Regression coverage added:

  • Extension loader skips unreadable registered tools while keeping the extension alive.
  • Loader diagnostics identify skipped tool registrations.
  • A healthy stateful tool registered after bad tools keeps its method receiver.

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: changing extension tool registration from fail-fast to per-tool quarantine.
  • Mitigation: only invalid/unreadable tool descriptors are skipped; extension factory failures still fail the extension load, registration diagnostics are surfaced, and regression coverage verifies healthy registrations continue.

Current review state

Next action: maintainer review.

Waiting on: CI/maintainer review.

Reviewer comments addressed: local $autoreview 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, 7:56 PM ET / 23:56 UTC.

Summary
The PR snapshots extension tool definitions during registerTool, skips unreadable descriptors with diagnostics, preserves healthy tool receiver binding, and forwards registration diagnostics through path-based and inline extension loading.

PR surface: Source +105, Tests +78. Total +183 across 3 files.

Reproducibility: yes. Current main reads tool.name directly during extension registration, so a throwing getter can fail the extension factory path; the PR adds a focused test for that path, but this read-only review did not run it.

Review metrics: 1 noteworthy metric.

  • Extension Registration Policy: 1 registration path changed. registerTool changes from direct insertion to descriptor snapshot/quarantine, which affects malformed local extension upgrade behavior before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/extensions/loader.test.ts, serialized state: src/agents/sessions/extensions/loader.ts, serialized state: src/agents/sessions/resource-loader.ts, unknown-data-model-change: src/agents/sessions/extensions/loader.test.ts, unknown-data-model-change: src/agents/sessions/extensions/loader.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:

  • [P1] Choose the canonical malformed extension-tool metadata contract against the fail-closed and broader session-registry branches before merge.

Risk before merge

Maintainer options:

  1. Choose One Tool-Metadata Contract (recommended)
    Decide whether malformed extension tools should be per-tool quarantined, fail the extension load, or be handled by the broader session-registry branch before merge.
  2. Accept Per-Tool Quarantine
    Merge this PR as the intended compatibility policy if maintainers want healthy sibling tools and commands to survive malformed descriptors with diagnostics.
  3. Defer To Broader Session Hardening
    Pause or close this PR if maintainers want fix(agents): ignore unreadable session tool metadata #89513 to own both extension registration and registry refresh after it is refreshed.

Next step before merge

  • [P2] The protected maintainer PR needs maintainer selection of the compatibility contract, not an automated code repair.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes trusted local extension registration behavior, with partial-load semantics tracked as compatibility risk.

Review details

Best possible solution:

Choose one canonical malformed extension-tool metadata policy across registration and session refresh, then land the selected implementation or fold this narrow patch into that branch.

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

Yes. Current main reads tool.name directly during extension registration, so a throwing getter can fail the extension factory path; the PR adds a focused test for that path, but this read-only review did not run it.

Is this the best way to solve the issue?

Unclear as the final policy. registerTool is a reasonable boundary for descriptor hardening, but maintainers still need to choose per-tool quarantine versus fail-closed validation or the broader session-registry approach.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded agent extension-loader hardening PR with limited blast radius and no evidence of an urgent live outage.
  • merge-risk: 🚨 compatibility: The diff intentionally changes malformed extension registration from fail/crash behavior to partial load with diagnostics, which can affect existing invalid local extensions.
  • 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 MEMBER maintainer-labeled PR; the body still reports focused loader tests, formatter/lint checks, local autoreview, and AWS Crabbox changed-gate proof.
Evidence reviewed

PR surface:

Source +105, Tests +78. Total +183 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 113 8 +105
Tests 1 78 0 +78
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 191 8 +183

What I checked:

Likely related people:

  • steipete: GitHub path history shows the session extension loader and SDK loading surfaces were introduced or substantially reshaped during agent-runtime internalization and follow-up loader work. (role: feature owner and adjacent owner; confidence: high; commits: bb46b79d3c14, 57a3dbe73639, 00d8d7ead059; files: src/agents/sessions/extensions/loader.ts, src/agents/sessions/resource-loader.ts, src/agents/sessions/extensions/types.ts)
  • shakkernerd: Recent history on resource-loader.ts includes session skill/resource loader layout refactors adjacent to extension resource discovery and diagnostics. (role: adjacent session contributor; confidence: medium; commits: d9278c8efdb5, 4d4609877256; files: src/agents/sessions/resource-loader.ts)
  • vincentkoc: They authored this PR and multiple related malformed tool-schema ingress hardening branches that need one consistent extension registration policy. (role: likely follow-up owner; confidence: high; commits: 179ab368b746, f372abd07e42, b74a7ed8d7c9; files: src/agents/sessions/extensions/loader.ts, src/agents/sessions/extensions/loader.test.ts, src/agents/sessions/resource-loader.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-21T10:22:18.870Z sha 179ab36 :: needs maintainer review before merge. :: none

@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 2, 2026
This was referenced Jun 2, 2026
Closed
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 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. 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: 🦐 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. labels Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Closed
@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: 🚨 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