Skip to content

perf: carry callable presence through the tool run plan - #138024

Merged
steipete merged 1 commit into
mainfrom
codex/perf8-callable-fact
Sep 4, 2026
Merged

perf: carry callable presence through the tool run plan#138024
steipete merged 1 commit into
mainfrom
codex/perf8-callable-fact

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor
Additional instructions

Maintainers can edit this branch.

What Problem This Solves

Each attempt built synthetic names for every catalog entry only to answer whether any callable tool remained. The explicit-allowlist guard then normalized that discarded list again.

Why This Change Was Made

Record callable presence in the run-plan owner and refresh it with the catalog. This removes the synthetic catalog/client list, duplicate normalization and refresh splice. The visible, replay, live and capability name sets keep their existing policies and identities; the boolean is private to the empty-allowlist check.

User Impact

Tool selection and allowlist errors stay the same with less temporary catalog work. Production LOC is +25/−30 (net −5); tests are +18/−18 (net 0). No configuration or protocol change.

Evidence

A fixed 180-case comparison of the actual original and candidate owners preserves all 1,440 guard decisions, four name sets and alias identities. The owners produced 91,836 synthetic catalog names before and zero afterward. All 21 existing owner tests and the targeted integration caller pass. The complete changed gate and fresh isolated Codex P0–P2 review pass.

Single functional test-process observations were 19.28 → 23.13 seconds and 1,890,304,000 → 1,884,700,672 peak RSS bytes. They establish no speed or process-memory improvement. Initial worktree dependency setup failures were resolved with one consistent existing donor; logs remain in local evidence.

The separate 500-tool real Gateway comparison is complete. Both the original baseline and a composed candidate containing this change completed one real OpenAI turn in Code Mode and one in Tool Search: discover 500 plugin tools, select and describe the designated tool, invoke it once with the expected integer argument, and verify the exact result and nested call history. Both gateways shut down normally; recorded processes and listener ports were gone afterward.

This single sequential pair is behavior proof under catalog pressure, not a per-PR performance estimate. Main-isolate catalog allocation samples were 370,367,384 → 371,463,040 bytes in Code Mode and 356,922,288 → 354,779,456 bytes in Tool Search, effectively flat. After the fixed idle period and two requested main-isolate GCs, heap used fell by about 3.0 and 2.3 MiB respectively, while whole-Gateway RSS rose by about 4.7 and 1.3 MiB. Sampling includes collected objects and is not an exact retained-byte count; these mixed results do not establish a general memory or speed improvement.

@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Sep 4, 2026
@steipete
steipete marked this pull request as ready for review September 4, 2026 07:24
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 4:57 AM ET / 08:57 UTC.

ClawSweeper review

What this changes

The agent’s tool plan replaces temporary callable-name lists with a boolean, reducing transient work while preserving tool selection and empty-allowlist errors.

Merge readiness

Ready for maintainer review

No blocking findings. This remains useful cleanup: current main and the latest release retain the redundant list construction, and the inspected change preserves the existing guard decisions.

Priority: P3
Reviewed head: d138a596fe504859eb1d400d0f814ceb46f1b7c5

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, net-negative refactor with preserved behavioral coverage and no identified blocking defect.
Proof confidence 🌊 off-meta tidepool Not applicable: The contributor-proof gate does not apply to this maintainer-authored internal refactor. The supplied 500-tool Gateway comparison reports successful discovery and invocation through both affected OpenClaw modes; it is supplemental behavior evidence, not an isolated performance measurement.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The contributor-proof gate does not apply to this maintainer-authored internal refactor. The supplied 500-tool Gateway comparison reports successful discovery and invocation through both affected OpenClaw modes; it is supplemental behavior evidence, not an isolated performance measurement.
Evidence reviewed 10 items Introduced scope and source identity: The pinned merge-base-to-head delta contains the callable-presence refactor and corresponding test updates. Production changes total +25/-30; tests total +18/-18. Base-only media and Gateway changes are excluded from this review. The working tree remained clean and diff whitespace checks passed.
Presence computation preserves the decision: The planner retains the four existing name sets and their alias relationship. Callable presence uses the same catalog, explicitly allowed client-tool, and non-auto-added visible-tool conditions. collectAllowedToolNames already removes blank names; catalog counts come from array lengths, making the removed synthetic-name normalization unnecessary.
Caller and refresh lifecycle: The catalog owner passes the boolean into the guard and refreshes it with the newly computed error. The attempt orchestrator refreshes bundle tools, catalog, session tools, and prompt policy in order. The prompt phase still records the error and skips submission when required.
Findings None None.
Security None None.

How this fits together

The embedded agent runner prepares available tools and operator allowlists before submitting a model turn. Its tool plan supports direct tools, Tool Search, and OpenClaw Code Mode, while an empty-allowlist guard reports unusable tool selections.

flowchart TD
  A[Available tools and catalog] --> C[Build tool run plan]
  B[Explicit allowlists] --> C
  C --> D{Callable tools remain?}
  D -->|Yes| E[Continue model turn]
  D -->|No| F[Apply existing empty-allowlist error policy]
  A -->|Catalog refresh| C
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -5, Tests 0. Total -5 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 3 25 30 -5
Tests 3 18 18 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 43 48 -5

Review metrics

Metric Value Why it matters
Production versus test LOC Production +25/-30 (net -5); tests +18/-18 (net 0) The refactor removes redundant representation and refresh work without adding a parallel implementation or expanding test volume.

Technical review

Best possible solution:

Keep the presence decision in the existing run-plan owner, with catalog refresh maintaining it and existing tool-selection and error policies preserved.

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

Not applicable as a bug reproduction: the redundant allocation is directly visible in current main, and this PR proposes a behavior-preserving optimization.

Is this the best way to solve the issue?

Yes. Computing existence in the existing planner removes unnecessary representation at its owner; caching the synthetic lists or reconstructing them in the guard would retain needless work.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P3: This is bounded internal allocation cleanup with no demonstrated user-facing failure or changed product contract.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor-proof gate does not apply to this maintainer-authored internal refactor. The supplied 500-tool Gateway comparison reports successful discovery and invocation through both affected OpenClaw modes; it is supplemental behavior evidence, not an isolated performance measurement.

Label justifications:

  • P3: This is bounded internal allocation cleanup with no demonstrated user-facing failure or changed product contract.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool 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 contributor-proof gate does not apply to this maintainer-authored internal refactor. The supplied 500-tool Gateway comparison reports successful discovery and invocation through both affected OpenClaw modes; it is supplemental behavior evidence, not an isolated performance measurement.

Evidence

What I checked:

  • Introduced scope and source identity: The pinned merge-base-to-head delta contains the callable-presence refactor and corresponding test updates. Production changes total +25/-30; tests total +18/-18. Base-only media and Gateway changes are excluded from this review. The working tree remained clean and diff whitespace checks passed. (d138a596fe50)
  • Presence computation preserves the decision: The planner retains the four existing name sets and their alias relationship. Callable presence uses the same catalog, explicitly allowed client-tool, and non-auto-added visible-tool conditions. collectAllowedToolNames already removes blank names; catalog counts come from array lengths, making the removed synthetic-name normalization unnecessary. (src/agents/embedded-agent-runner/run/attempt-tool-search-run-plan.ts:123, d138a596fe50)
  • Caller and refresh lifecycle: The catalog owner passes the boolean into the guard and refreshes it with the newly computed error. The attempt orchestrator refreshes bundle tools, catalog, session tools, and prompt policy in order. The prompt phase still records the error and skips submission when required. (src/agents/embedded-agent-runner/run/attempt-tool-catalog.ts:305, d138a596fe50)
  • Existing guard contracts retained: The guard still distinguishes command-time allowlists from inherited configuration when tools are intentionally disabled. Its error reasons and remediation text are unchanged. Repository-wide search found one production caller, which is migrated; no old callable-list consumers remain. (src/agents/tool-allowlist-guard.ts:42, d138a596fe50)
  • Focused tests and integration assertion: Inspected the 12 run-plan tests, nine guard tests, and the changed isolated-cron integration case. Coverage retains control-only rejection, explicit and wildcard client matching, directory mode, replay names, capability exclusions, and intentional tool-less runs. Tests were not executed during this read-only review. (src/agents/embedded-agent-runner/run/attempt.tool-search-run-plan.test.ts:84, d138a596fe50)
  • Current main still contains the redundant work: Fetched main still exposes emptyAllowlistCallableNames and constructs synthetic catalog/client names. Its three affected production modules match the pinned merge-base versions, so the proposed optimization is not already present there. (src/agents/embedded-agent-runner/run/attempt-tool-search-run-plan.ts:32, be967f4a514f)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Gio Della-Libera: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-04T08:20:21.471Z sha f02c9e1 :: needs maintainer review before merge. :: none

@steipete
steipete force-pushed the codex/perf8-callable-fact branch from f02c9e1 to d138a59 Compare September 4, 2026 08:47
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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 Sep 4, 2026
@steipete
steipete merged commit dc93635 into main Sep 4, 2026
180 checks passed
@steipete
steipete deleted the codex/perf8-callable-fact branch September 4, 2026 09:29
@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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

Development

Successfully merging this pull request may close these issues.

1 participant