refactor(agents): split before-tool-call policy pipeline by steipete · Pull Request #113885 · openclaw/openclaw

steipete · 2026-07-25T21:55:11Z

Additional instructions

MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.

What Problem This Solves

The pre-tool execution boundary had grown to 2,143 lines while owning approval transport, ordered policy decisions, diagnostics and skill telemetry, loop bookkeeping, and wrapped execution. That made security-sensitive ordering and parameter-identity invariants hard to review and kept the file on the max-lines baseline.

This is a maintainer-requested god-file reduction. It is AI-assisted.

Why This Change Was Made

Split the implementation into approval transport, policy-chain orchestration, diagnostics/skill/loop handling, and wrapped execution/finalization modules, plus a type-only shared contract module that prevents facade back-edges. The original path remains a small facade. Every implementation module is below 700 lines, so the target baseline entry is removed.

The split preserves the exact policy sequence and both execution paths. A shared finalization helper keeps the original prepared argument object alive through reconciliation because exec preparation keys private state by that exact WeakMap identity. It also preserves the adapter and wrapped paths' distinct historical finalizer receiver/nullish-return semantics.

A high-severity pre-existing control bypass was found during this audit and is deliberately not fixed here: Skill Workshop is revalidated on final params, while client-voice confirmation still runs only on initial params before later permitted rewrites. The maintainer has confirmed and routed that behavior to a dedicated product/security fix. The related pre-existing loop-history mismatch and voice-denial attribution remain untouched too.

User Impact

No user-visible behavior change. This makes the pre-execution security boundary smaller and clearer to review without changing policy decisions, approval timing, parameter rewriting, diagnostics, or tool execution.

Beyond the Split

Removed the redundant local preparing-tool intersection because AnyAgentTool already owns the optional preparation and finalization callbacks. The initial Skill Workshop probe remains unchanged because its truthiness controls the fast return; no approval-path optimization was attempted.

Ordering Verification

I re-traced source order after the split, independently for the policy chain and both execution paths:

wrapped path prepares exact preparedParams, then code-mode normalizes;
policy chain detects/records loops, probes Skill Workshop, checks voice on initial params, runs trusted policies, resolves trusted approval, renormalizes, runs ordinary hooks, applies hook approval/rewrites, and rechecks Skill Workshop on final params;
wrapped execution checks abort, then the shared helper reconciles and finalizes before adjusted-param recording and execution;
the unwrapped adapter follows prepare -> normalize -> policy -> reconcile/finalize -> adjusted-param record -> execute through the same helper.

The policy sequence is visible in src/agents/agent-tools.before-tool-call.policy.ts:113, :178, :185, :235, :284, :306, :342, :368, :390, and :393. The two execution paths are visible in src/agents/agent-tools.before-tool-call.wrapper.ts:348 and src/agents/agent-tool-definition-adapter.ts:351. The two-object WeakMap contract and historical finalizer modes are centralized and documented at src/agents/agent-tools.before-tool-call.wrapper.ts:92.

Evidence

Final rebased proof: Blacksmith Testbox tbx_01kydn82ymsw1j5atn6fcj50ty: https://github.com/openclaw/openclaw/actions/runs/30177045792

pnpm check:changed: passed on the final reviewed tree in 16m48s, including format, max-lines ratchet, SDK API baseline, core/core-test typechecks, all core lint shards, boundary guards, and zero runtime import cycles.
Exact full-CI recovery lanes passed: pnpm check:architecture reports zero runtime and madge cycles; pnpm deadcode:dependencies and pnpm deadcode:exports report zero unused entries.
Focused before-tool/adapter/runtime suites: 199 passed. Four current-main private diagnostic-content cases were excluded from the clean command; an archived origin/main run independently reproduced the same three failures because those tests still use the retired per-field captureContent shape. This PR does not alter that behavior or those tests.
pnpm build: passed in 1m30s; all 142 public Plugin SDK subpaths verified and no [INEFFECTIVE_DYNAMIC_IMPORT] warning appeared.
Redacted real exec proof through both production paths: https://github.com/openclaw/openclaw/actions/runs/30177961708. The wrapped and unwrapped-adapter calls each executed pwd in a disposable prepared workdir, returned status=completed / exitCode=0, preserved the final adjusted workdir, and observed that workdir in real shell output:
```
{"wrapped":{"status":"completed","exitCode":0,"adjustedWorkdirMatches":true,"outputContainsWorkspace":true},"adapter":{"status":"completed","exitCode":0,"adjustedWorkdirMatches":true,"outputContainsWorkspace":true}}
```
Plugin SDK baseline regeneration changed only the agent-harness-runtime source-link hash; exported declarations remained identical.
Final full-branch autoreview: clean, no accepted/actionable findings (patch is correct, 0.98).
Owner review: the maintainer/orchestrator explicitly accepted the ordering and prepared-state analysis, confirmed B17/B18/B19 are separate work, and directed this split through the full review-and-land workflow.

clawsweeper · 2026-07-25T22:26:41Z

Codex review: needs real behavior proof before merge. Reviewed July 25, 2026, 5:59 PM ET / 21:59 UTC.

ClawSweeper review

What this changes

This PR decomposes the agent pre-tool-call approval, policy, diagnostics, and execution flow into focused modules while keeping agent-tools.before-tool-call.ts as the public facade.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 5 items remain

Keep this PR open for maintainer review. It is a broad but focused refactor of the security-sensitive pre-tool execution boundary; the supplied diff preserves the public facade and centralizes the prepared-parameter/finalization contract, but the protected maintainer label and approval-ordering risk make this unsuitable for automated cleanup or a no-risk merge verdict.

Priority: P3
Reviewed head: 3f58d14d469291efc49f74897cce0d90eeeb29ed
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The refactor has strong stated static and test validation, but merge confidence is capped by the lack of directly observed real-behavior proof for an approval-sensitive execution boundary.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The PR body reports focused suites, checks, and a build, but it does not include after-fix live output, logs, a recording, or another redacted real execution proof for either execution path; attach that evidence and redact private details before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment `@clawsweeper re-review`.
Patch quality	🦐 gold shrimp (3/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR body reports focused suites, checks, and a build, but it does not include after-fix live output, logs, a recording, or another redacted real execution proof for either execution path; attach that evidence and redact private details before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment `@clawsweeper re-review`.
Evidence reviewed	5 items	Facade and ownership split: The proposed public facade delegates the former monolithic pre-tool-call implementation into approval, ordered policy, diagnostics, and wrapper/finalization modules; the PR removes the old max-lines-baseline entry as part of that split. Ordering-sensitive policy path: The new policy module explicitly identifies ordering as behavior and contains the loop, voice-confirmation, trusted-policy, approval, ordinary-hook, and final-owner-approval chain described in the PR body. Prepared-parameter identity contract: The wrapper module centralizes preparation and finalization, including the prepared-object identity that execution preparation uses for private WeakMap state; the adapter now calls these shared helpers.
Findings	None	None.
Security	None	None.

How this fits together

Before an agent executes a tool, OpenClaw prepares parameters, applies loop and security policies, obtains approvals, records diagnostics, and then runs the tool. This PR reorganizes that boundary without intending to change the decisions, parameter identity, or execution order that downstream tool adapters rely on.

flowchart LR
  A[Agent requests tool call] --> B[Prepare parameters]
  B --> C[Ordered policy chain]
  C --> D[Approval transport]
  D --> E[Diagnostics and loop tracking]
  E --> F[Finalize prepared parameters]
  F --> G[Tool execution]
  G --> H[Terminal diagnostics and replay state]

Decision needed

Question	Recommendation
Should maintainers accept this ownership split after confirming that the wrapped and adapter paths preserve the existing approval ordering, finalizer receiver semantics, and prepared-parameter identity contract?	Review and retain the split: Verify the two execution paths against their established policy and finalization invariants, then merge once live proof is attached.

Why: This is a maintainer-labeled refactor of a security-sensitive control boundary; source-level checks alone cannot choose the acceptable risk level for preserving historical execution semantics.

Before merge

Add real behavior proof - Needs real behavior proof before merge: The PR body reports focused suites, checks, and a build, but it does not include after-fix live output, logs, a recording, or another redacted real execution proof for either execution path; attach that evidence and redact private details before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Resolve merge risk (P1) - A behavioral regression in this split could weaken or reorder approval, trusted-policy, loop, or final-parameter checks even though the public API and intended policy sequence are unchanged.
Resolve merge risk (P1) - The PR body supplies test and build evidence, but not an after-fix live execution transcript demonstrating both wrapped and adapter execution paths with real prepared/finalized parameters.
Complete next step (P2) - The protected maintainer label and approval-ordering/security boundary require owner review and contributor-supplied real execution proof; there is no narrow mechanical repair to dispatch.

Agent review details

Security

None.

PR surface

Source +133, Generated 0, Other -1. Total +132 across 8 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	6	2239	2106	+133
Tests	0	0	0	0
Docs	0	0	0	0
Config	0	0	0	0
Generated	1	1	1	0
Other	1	0	1	-1
Total	8	2240	2108	+132

Review metrics

Metric	Value	Why it matters
Regression-test files	0 changed	The patch reorganizes an ordering-sensitive approval boundary without adding focused regression coverage in the submitted diff, so existing and live contract proof carry the merge confidence.

Stored data model

Persistent data-model change detected: vector/embedding metadata: src/agents/agent-tools.before-tool-call.diagnostics.ts, vector/embedding metadata: src/agents/agent-tools.before-tool-call.ts, vector/embedding metadata: src/agents/agent-tools.before-tool-call.wrapper.ts. Confirm migration or upgrade compatibility proof before merge.

Merge-risk options

Maintainer options:

Prove the preserved execution contract (recommended)
Before merge, add redacted live evidence covering wrapped and adapter calls through prepare, policy, reconcile/finalize, and execution with the existing parameter-identity semantics.
Accept owner-reviewed refactor risk
Merge only after the responsible owners explicitly accept that the supplied focused tests and source-order audit adequately cover the historical security and compatibility contract.
Pause for a narrower split
Pause this branch if reviewing the combined policy and execution reorganization cannot establish parity with the existing boundary.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Attach redacted live proof for both wrapped and adapter execution paths, including the prepared-parameter identity and finalization order.

Technical review

Best possible solution:

Retain the focused module split, then obtain owner review of the complete ordering and prepared-parameter identity contract plus redacted live evidence for both execution paths before merge.

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

Not applicable: this PR is an internal refactor and does not report a user-facing bug. The relevant verification is behavioral parity across the existing wrapped and adapter execution paths.

Is this the best way to solve the issue?

Unclear: splitting the god file by approval, policy, diagnostics, and execution ownership is a maintainable direction, but this review cannot establish the required parity from test/build claims alone without live path evidence and owner confirmation.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

add P3: This is a maintainer-requested internal refactor with no claimed user-visible behavior change.
add merge-risk: 🚨 compatibility: The adapter and wrapper paths preserve historical prepared-parameter and finalizer semantics that existing tools can depend on.
add merge-risk: 🚨 security-boundary: The changed pipeline controls tool approval and policy ordering before execution, so an unintended ordering change could weaken a security control.
add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused suites, checks, and a build, but it does not include after-fix live output, logs, a recording, or another redacted real execution proof for either execution path; attach that evidence and redact private details before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

P3: This is a maintainer-requested internal refactor with no claimed user-visible behavior change.
merge-risk: 🚨 compatibility: The adapter and wrapper paths preserve historical prepared-parameter and finalizer semantics that existing tools can depend on.
merge-risk: 🚨 security-boundary: The changed pipeline controls tool approval and policy ordering before execution, so an unintended ordering change could weaken a security control.
rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused suites, checks, and a build, but it does not include after-fix live output, logs, a recording, or another redacted real execution proof for either execution path; attach that evidence and redact private details before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Facade and ownership split: The proposed public facade delegates the former monolithic pre-tool-call implementation into approval, ordered policy, diagnostics, and wrapper/finalization modules; the PR removes the old max-lines-baseline entry as part of that split. (src/agents/agent-tools.before-tool-call.ts:1, 3f58d14d4692)
Ordering-sensitive policy path: The new policy module explicitly identifies ordering as behavior and contains the loop, voice-confirmation, trusted-policy, approval, ordinary-hook, and final-owner-approval chain described in the PR body. (src/agents/agent-tools.before-tool-call.policy.ts:1, 3f58d14d4692)
Prepared-parameter identity contract: The wrapper module centralizes preparation and finalization, including the prepared-object identity that execution preparation uses for private WeakMap state; the adapter now calls these shared helpers. (src/agents/agent-tools.before-tool-call.wrapper.ts:92, 3f58d14d4692)
Repository review policy: The repository policy treats policy ordering, approval behavior, and security-sensitive execution boundaries as paths requiring whole-surface review and owner handling rather than a diff-only approval. (AGENTS.md:1, c0e4caae5589)
Validation supplied by the PR: The PR body reports a final-tree changed-check, focused suites, build, SDK baseline check, and autoreview; CI also shows many relevant checks passing, while several shard and QA checks were still running in the supplied state. (3f58d14d4692)

Likely related people:

steipete: The maintainer-labeled PR contains the detailed current policy-order and prepared-parameter contract for this agent execution boundary, indicating direct current involvement with the affected path. (role: recent area contributor; confidence: low; commits: 3f58d14d4692; files: src/agents/agent-tools.before-tool-call.ts, src/agents/agent-tools.before-tool-call.policy.ts, src/agents/agent-tools.before-tool-call.wrapper.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

Attach redacted live wrapped-path and adapter-path evidence showing prepare, policy, finalization, and execution behavior.
Obtain explicit owner review of policy order and finalizer/parameter-identity parity.

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.

clawsweeper · 2026-07-25T22:39:29Z

ClawSweeper status: review started.

I am starting a fresh review of this pull request: refactor(agents): split before-tool-call policy pipeline This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

steipete · 2026-07-25T22:59:34Z

Merged via squash.

Prepared head SHA: 1e0a149d4dcfce46d25ba8865bafa6b1c16b0b1b
Landed commit: ddd3d4fdb32135808e601474a8b90a41bb087926

…3885) * refactor(agents): split before-tool-call pipeline * refactor(agents): isolate before-tool call types * fix(agents): restore approval resolution type import

openclaw-barnacle Bot added docs Improvements or additions to documentation agents Agent runtime and tooling size: XL maintainer Maintainer-authored PR labels Jul 25, 2026

steipete self-assigned this Jul 25, 2026

steipete added 2 commits July 25, 2026 18:09

refactor(agents): split before-tool-call pipeline

b8bb7cc

refactor(agents): isolate before-tool call types

e0b9af4

fix(agents): restore approval resolution type import

1e0a149

steipete force-pushed the codex/l3-beforetool branch from 3f58d14 to 1e0a149 Compare July 25, 2026 22:36

steipete merged commit ddd3d4f into main Jul 25, 2026
266 of 274 checks passed

steipete deleted the codex/l3-beforetool branch July 25, 2026 22:59

github-actions Bot mentioned this pull request Jul 26, 2026

📡 Upstream Digest — 2026-07-26 01:59 UTC curtismercier/openclaw-mods#1407

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(agents): split before-tool-call policy pipeline#113885

refactor(agents): split before-tool-call policy pipeline#113885
steipete merged 3 commits into
mainfrom
codex/l3-beforetool

steipete commented Jul 25, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 25, 2026

Security

PR surface

Review metrics

Stored data model

Merge-risk options

Technical review

Labels

Evidence

Rank-up moves

Rating scale

Workflow

Uh oh!

clawsweeper Bot commented Jul 25, 2026

Uh oh!

Uh oh!

steipete commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

steipete commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

User Impact

Beyond the Split

Ordering Verification

Evidence

Uh oh!

clawsweeper Bot commented Jul 25, 2026

ClawSweeper review

What this changes

Merge readiness

Review scores

Verification

How this fits together

Decision needed

Before merge

Security

PR surface

Review metrics

Stored data model

Merge-risk options

Technical review

Labels

Evidence

Rank-up moves

Rating scale

Workflow

Uh oh!

clawsweeper Bot commented Jul 25, 2026

Uh oh!

Uh oh!

steipete commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

steipete commented Jul 25, 2026 •

edited

Loading