Skip to content

fix(agents): guard lean tool name reads#89730

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next74-20260603
Closed

fix(agents): guard lean tool name reads#89730
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-tool-schema-next74-20260603

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Catch-bound local-model lean tool name normalization so unreadable descriptors cannot crash lean filtering.
  • Drop malformed or empty-name descriptors only while lean filtering is active; disabled lean mode still returns the original tool array.
  • Add a regression covering an unreadable name getter while preserving healthy read / exec siblings and filtering cron.

Verification

  • Red repro before the fix: CI=1 node scripts/run-vitest.mjs run src/agents/local-model-lean.test.ts -t "omits unreadable tool names" --reporter=verbose failed with local model lean tool name getter exploded from src/agents/local-model-lean.ts.
  • CI=1 OPENCLAW_VITEST_NO_OUTPUT_TIMEOUT_MS=30000 OPENCLAW_VITEST_NO_OUTPUT_HEARTBEAT_MS=10000 node scripts/run-vitest.mjs run src/agents/local-model-lean.test.ts -t "omits unreadable tool names" --reporter=verbose --pool=forks --maxWorkers=1 --fileParallelism=false --teardownTimeout=1000
  • CI=1 OPENCLAW_VITEST_NO_OUTPUT_TIMEOUT_MS=30000 OPENCLAW_VITEST_NO_OUTPUT_HEARTBEAT_MS=10000 node scripts/run-vitest.mjs run src/agents/local-model-lean.test.ts --reporter=verbose --pool=forks --maxWorkers=1 --fileParallelism=false --teardownTimeout=1000
  • node --run format:check -- src/agents/local-model-lean.ts src/agents/local-model-lean.test.ts
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs src/agents/local-model-lean.ts src/agents/local-model-lean.test.ts
  • git diff --check origin/main..HEAD
  • added-line private-data scrub
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Behavior addressed: local-model lean filtering no longer aborts the assistant tool surface when a malformed descriptor has an unreadable name getter.

Real environment tested: local focused Vitest and repo wrapper checks on the rebased OpenClaw branch.

Exact steps or command run after this patch: ran the focused regression, the full src/agents/local-model-lean.test.ts file, format check, scoped oxlint, diff check, private-data scrub, local autoreview, and branch autoreview.

Evidence after fix: focused Vitest passed 1 file / 10 tests after rebase; format, scoped oxlint, and diff check passed; local and branch autoreview reported no accepted/actionable findings.

Observed result after fix: malformed unreadable-name tools are omitted during active lean filtering, healthy read and exec tools remain visible, and denied heavyweight tools are still removed.

What was not tested: full pnpm check:changed did not run. The AWS Crabbox changed-gate wrapper failed before lease creation because the local sparse-sync cache had less than the required 1 GiB free even after agent-worktree-maintain --force; Blacksmith Testbox is not authenticated in this environment. The plain oxlint wrapper was also blocked before lint by unrelated Discord boundary dts errors, so scoped oxlint was run with OPENCLAW_OXLINT_SKIP_PREPARE=1 on the touched files.

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

clawsweeper Bot commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 6:28 AM ET / 10:28 UTC.

Summary
The PR guards local-model lean tool filtering against tool descriptors whose name getter throws, and adds a regression test for preserving valid tools while dropping malformed descriptors.

PR surface: Source +11, Tests +27. Total +38 across 2 files.

Reproducibility: yes. Current main reads tool.name during active lean filtering before later schema projection, and the PR body records a focused red Vitest for an unreadable name getter.

Review metrics: none identified.

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

  • Refresh or rebase the branch to resolve the live CONFLICTING merge state.
  • Run pnpm check:changed or an equivalent maintainer-approved Testbox/Crabbox gate before landing.

Risk before merge

  • [P1] Live GitHub reports the PR merge state as DIRTY/CONFLICTING, so the branch needs refresh before it can land.
  • [P1] The PR body says the full pnpm check:changed gate did not run, so final landing still needs a changed gate or maintainer-approved equivalent.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow guard and regression after the draft branch is refreshed against current main and the changed gate or accepted equivalent has passed.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] The remaining action is maintainer handling for a draft, protected-label, currently conflicting PR plus final validation; there is no narrow code repair finding from this review.

Security
Cleared: The diff only changes an agent lean-filter helper and colocated test, with no dependency, CI, install, secret, permission, or supply-chain surface change.

Review details

Best possible solution:

Land the narrow guard and regression after the draft branch is refreshed against current main and the changed gate or accepted equivalent has passed.

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

Yes. Current main reads tool.name during active lean filtering before later schema projection, and the PR body records a focused red Vitest for an unreadable name getter.

Is this the best way to solve the issue?

Yes. Guarding only the lean-filter name read is the narrowest maintainable fix because disabled lean mode still returns the original tool array and later schema quarantine cannot protect this earlier read.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The item is a bounded agent-tool crash-path fix for experimental local-model lean mode with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 real behavior proof gate does not apply because this is a MEMBER maintainer-labeled PR; the body still includes focused red/green terminal test evidence.
Evidence reviewed

PR surface:

Source +11, Tests +27. Total +38 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 12 1 +11
Tests 1 27 0 +27
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 39 1 +38

What I checked:

  • Current main crash path: Current main returns the original tools only when lean mode is disabled, then active lean filtering reads tool.name directly through normalizeToolName(tool.name), so an unreadable name getter can throw before filtering completes. (src/agents/local-model-lean.ts:93, 72b9bc730370)
  • Runtime caller order: The embedded runner calls filterLocalModelLeanTools before filterRuntimeCompatibleTools, so the later runtime schema quarantine cannot protect the earlier lean-filter name read. (src/agents/embedded-agent-runner/run/attempt.ts:1638, 72b9bc730370)
  • Sibling schema quarantine: The runtime schema projection path already catches unreadable name and parameters fields, but that protection is only useful after the lean-filter stage has returned a projected tool list. (src/agents/tool-schema-projection.ts:74, 72b9bc730370)
  • Patch shape: The PR diff adds a local try/catch name normalizer and drops empty or unreadable names only while lean filtering is active, plus a focused regression preserving healthy read and exec tools while filtering cron. (src/agents/local-model-lean.ts:17, ae21ac4d47f0)
  • Live PR state: Live GitHub metadata shows the PR is open, draft, assigned to vincentkoc, labeled maintainer, and currently reports mergeStateStatus DIRTY with mergeable CONFLICTING. (ae21ac4d47f0)
  • Proof in PR body: The PR body records a red focused Vitest failure before the patch, focused green regression runs after the patch, format check, scoped oxlint, diff check, private-data scrub, and autoreview; it also states full pnpm check:changed did not run. (ae21ac4d47f0)

Likely related people:

  • vincentkoc: Current checkout blame/log and PR search show repeated recent work on local-model lean filtering, lean catalog behavior, and this file's current implementation. (role: recent area contributor; confidence: high; commits: 0a7b0096477f, f09a4d9ba07d; files: src/agents/local-model-lean.ts, src/agents/local-model-lean.test.ts, src/agents/embedded-agent-runner/run/attempt.ts)
  • dutifulbob: Live PR search shows the merged per-agent local-model lean mode PR as the feature introduction path for this behavior area. (role: introduced feature; confidence: medium; files: src/agents/local-model-lean.ts, src/agents/local-model-lean.test.ts)
  • Peter Steinberger: Recent invalid direct tool-schema quarantine work is relevant sibling behavior because it catches unreadable tool fields after lean filtering has already run. (role: adjacent schema-quarantine contributor; confidence: medium; commits: a02813164dd5; files: src/agents/tool-schema-projection.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. labels Jun 3, 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 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 P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS 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