Skip to content

fix(agents): validate extension tool names#89149

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-extension-register-tool-20260601
Closed

fix(agents): validate extension tool names#89149
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-extension-register-tool-20260601

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

  • validate extension-registered tool names before inserting tools into the extension registry
  • fail closed through the existing extension load error path for non-string, empty, or unreadable names
  • add regression coverage for numeric tool names and throwing name accessors

Verification

  • node scripts/run-vitest.mjs src/agents/sessions/extensions/loader.test.ts --reporter=dot
  • node_modules/.bin/oxfmt --check src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts
  • node_modules/.bin/oxlint src/agents/sessions/extensions/loader.ts src/agents/sessions/extensions/loader.test.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
  • 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 CI=1 corepack pnpm check:changed" (run_8fd80c3ac42a, cbx_3db5b164cc01, provider aws, exit 0)

Real behavior proof

Behavior addressed: a malformed local session extension can no longer register a tool with a non-string or unreadable name and leave a poisoned entry for later session/runtime paths.
Real environment tested: focused local Vitest on Node 24 plus AWS Crabbox Linux changed gate.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/sessions/extensions/loader.test.ts --reporter=dot; 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 CI=1 corepack pnpm check:changed".
Evidence after fix: focused Vitest passed 3 tests; Crabbox changed gate selected core and coreTests, completed typecheck/lint/guards, and exited 0 in run run_8fd80c3ac42a on lease cbx_3db5b164cc01. After one final fast-forward rebase, focused Vitest, oxfmt, oxlint, and git diff --check were rerun on pushed head f372abd07e48f167d80e65efb8b01c6b3c4affc4.
Observed result after fix: invalid extension tool names now reject the extension through loadExtensions().errors with a clear registration message instead of inserting malformed tool entries.
What was not tested: full repository suite on the pushed SHA; GitHub PR checks are expected to cover the final branch head.

@vincentkoc vincentkoc self-assigned this Jun 1, 2026
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S 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, 6:53 AM ET / 10:53 UTC.

Summary
The branch adds extension tool-name validation in registerTool() and regression tests for numeric and throwing tool names.

PR surface: Source +20, Tests +71. Total +91 across 2 files.

Reproducibility: yes. source-reproducible: current main reads extension tool.name directly at registration and later uses definition.name in registry paths. I did not execute tests because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Extension registration policy: 1 fail-closed validation path added. The new registration guard changes malformed local extension behavior before merge, which is compatibility-sensitive even though the diff is small.

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, unknown-data-model-change: src/agents/sessions/extensions/loader.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:

  • Choose whether malformed extension tool metadata should fail the whole extension or quarantine only the bad tool.
  • Resolve the whitespace-name policy by preserving, rejecting, or normalizing names intentionally.

Risk before merge

  • [P1] Merging as-is can make an existing local extension with one malformed registered tool lose healthy sibling tools, commands, handlers, flags, shortcuts, and renderers because validation throws into the full-extension load failure path.
  • [P1] The open sibling branches choose skip/quarantine/ignore semantics, so landing this branch first would settle the malformed metadata contract without an explicit maintainer choice.
  • [P1] Whitespace-padded string names pass the new emptiness check but are returned verbatim, so maintainers still need to decide whether exact whitespace is allowed, rejected, or normalized.

Maintainer options:

  1. Choose one metadata contract (recommended)
    Decide before merge whether malformed registered tools should invalidate the whole extension or be skipped/quarantined while healthy sibling registrations remain available.
  2. Accept full-extension rejection
    Maintainers can intentionally accept this branch if invalid local extension tool metadata should reject the full extension load and exact whitespace preservation is desired.
  3. Continue with quarantine branches
    Pause this branch if preserving healthy sibling registrations is the preferred compatibility policy and carry review forward on the quarantine-oriented PRs.

Next step before merge

  • [P2] The remaining blocker is a maintainer compatibility decision, not a narrow mechanical repair suitable for automation.

Security
Cleared: No concrete security or supply-chain regression was found; the diff only changes trusted local extension registration behavior and colocated tests.

Review findings

  • [P1] Preserve healthy extension registrations on bad metadata — src/agents/sessions/extensions/loader.ts:248-249
Review details

Best possible solution:

Choose and land one canonical malformed extension-tool metadata policy across the open branches, with tests proving either intended whole-extension rejection or per-tool quarantine behavior.

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

Yes, source-reproducible: current main reads extension tool.name directly at registration and later uses definition.name in registry paths. I did not execute tests because this review is read-only.

Is this the best way to solve the issue?

No, not yet as a merge decision. registerTool() is the right hardening boundary, but maintainers still need to choose fail-closed whole-extension rejection versus per-tool quarantine semantics.

Full review comments:

  • [P1] Preserve healthy extension registrations on bad metadata — src/agents/sessions/extensions/loader.ts:248-249
    This throws inside registerTool(), and loadExtension() converts that throw into a full extension load failure. An existing local extension with one malformed tool can lose healthy sibling tools, commands, handlers, flags, shortcuts, and renderers on upgrade; preserve those registrations or get explicit maintainer signoff for fail-closed semantics.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

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 bounded agent session extension-loader hardening PR with limited blast radius and no evidence of an urgent live outage.
  • merge-risk: 🚨 compatibility: The PR changes malformed extension tool registration into load-time rejection, which can affect existing local extensions on upgrade.
  • 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 maintainer-labeled draft, though the PR body records focused tests and AWS Crabbox changed-gate proof.
Evidence reviewed

PR surface:

Source +20, Tests +71. Total +91 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 21 1 +20
Tests 1 71 0 +71
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 92 1 +91

What I checked:

  • Repository policy applied: Root and scoped agents policy were read; root policy treats plugin/session fallback and fail-closed behavior as compatibility-sensitive PR review risk. (AGENTS.md:31, c5d34c8376f8)
  • Protected live PR state: Live metadata shows a MEMBER-authored draft with the protected maintainer label, so conservative cleanup should keep it open for maintainer handling. (f372abd07e42)
  • Current main registration behavior: Current main still inserts extension tools with extension.tools.set(tool.name, ...), so malformed tool-name handling is not already implemented. (src/agents/sessions/extensions/loader.ts:227, c5d34c8376f8)
  • PR implementation path: The PR reads and validates tool.name, then stores the checked value as the map key. (src/agents/sessions/extensions/loader.ts:151, f372abd07e42)
  • Whole-extension failure path: loadExtension() catches any thrown factory or registration error and returns extension: null, so this PR's validation throw rejects all registrations from that extension. (src/agents/sessions/extensions/loader.ts:485, c5d34c8376f8)
  • Sibling registry consumers: Registered extension tools are later deduplicated and indexed by tool.definition.name, which confirms the malformed metadata reaches runtime registry paths if not handled consistently. (src/agents/sessions/extensions/runner.ts:398, c5d34c8376f8)

Likely related people:

  • steipete: GitHub path history shows the agent-runtime internalization and loader follow-up work touched the extension loader, extension SDK loading, and session-runtime surfaces that own this behavior. (role: feature owner and adjacent owner; confidence: high; commits: bb46b79d3c14, 57a3dbe73639, 00d8d7ead059; files: src/agents/sessions/extensions/loader.ts, src/agents/sessions/extensions/runner.ts, src/agents/sessions/resource-loader.ts)
  • vincentkoc: Vincent authored this branch and the related malformed tool-metadata branches, and recent main history also shows session work in the affected area. (role: recent area contributor and likely follow-up owner; confidence: high; commits: f372abd07e42, 179ab368b746, b74a7ed8d7c9; files: src/agents/sessions/extensions/loader.ts, src/agents/sessions/extensions/loader.test.ts, src/agents/sessions/agent-session.ts)
  • shakkernerd: Recent agent-session history includes active-tool and prompt-registry work that consumes registered extension tools after loader registration. (role: adjacent session registry contributor; confidence: medium; commits: 73cf516def3d; files: src/agents/sessions/agent-session.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
@byungskers

byungskers commented Jun 1, 2026

Copy link
Copy Markdown

This looks like the right boundary to fail closed. One thing I noticed while reading the helper: readRegisteredToolName() validates with name.trim().length > 0 but returns the original name instead of the trimmed one. That means names like " message " still register verbatim and could end up as distinct map keys from "message". If preserving exact whitespace is intentional, no problem — but if not, normalizing to the trimmed value here might avoid a subtle duplicate/lookup edge case.

This was referenced Jun 15, 2026
Closed
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. 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. labels Jun 19, 2026
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this fuzz-only hardening PR. The only failing state is a synthetic fixture using hostile in-process objects, throwing getters or callbacks, or similarly malformed metadata; there is no linked production report or supported external contract requiring that state.

This adds broad fail-soft behavior and substantial regression surface across consumers. If a real plugin or provider demonstrates the failure, please open a focused issue with the observed payload and stack so validation can live at the single owning registration/parsing boundary. Thanks for exploring it.

@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: S 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.

3 participants

@vincentkoc @byungskers @steipete