Skip to content

fix(skills): honor tool policy for inline dispatch#78525

Open
eleqtrizit wants to merge 2 commits intoopenclaw:mainfrom
eleqtrizit:568
Open

fix(skills): honor tool policy for inline dispatch#78525
eleqtrizit wants to merge 2 commits intoopenclaw:mainfrom
eleqtrizit:568

Conversation

@eleqtrizit
Copy link
Copy Markdown
Contributor

@eleqtrizit eleqtrizit commented May 6, 2026 •
edited
Loading

Summary

  • Keeps inline skill tool dispatch aligned with configured tool policy.
  • Adds regression coverage for denied and allowlisted tool dispatch targets.

Changes

  • Resolves inline skill dispatch tools through a runtime helper that applies the effective tool policy before owner-only gating.
  • Preserves the existing createOpenClawTools dispatch surface while passing sender and session context into tool construction.
  • Adds focused regression tests for tools.deny and tools.allow behavior on inline skill tool dispatch.

Validation

  • corepack pnpm test -- src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts
  • corepack pnpm format
  • corepack pnpm check
  • corepack pnpm build
  • claude -p "/review" ran; it requested PR context instead of returning local findings.

Real behavior proof

  • Behavior or issue addressed: Inline skill commands that dispatch directly to tools now honor configured tool allow/deny policy before the dispatch target is available.
  • Real environment tested: Local OpenClaw source checkout on Linux with Node v22.22.2, using the actual runtime resolver and real createOpenClawTools surface.
  • Exact steps or command run after this patch: Ran node --import tsx -e ... to call resolveSkillDispatchTools with tools.deny: ["message"], plugins disabled, and a Telegram-like message context.
  • Evidence after fix: Terminal output from the runtime command:
{
  "deniedTool": "message",
  "messageAvailable": false,
  "toolCount": 14
}
  • Observed result after fix: The message dispatch target was absent from the resolved tool list (messageAvailable: false) while other real tools remained available.
  • What was not tested: Live channel delivery through Telegram, Slack, or Discord was not exercised in this local run.

Notes

  • Local full-suite corepack pnpm test still fails in unrelated test/scripts/test-planner.test.ts assertions; rerunning that file alone reproduces the same three failures.
  • No changelog entry; this is internal behavior alignment.

@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR labels May 6, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 6, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The PR routes inline skill command-dispatch tool calls through a new resolver that applies effective tool, group, sandbox, and subagent policy, with focused auto-reply regression coverage.

Reproducibility: yes. By source inspection against current main, inline skill dispatch constructs raw OpenClaw tools and only applies owner-only filtering while the documented contract requires the full policy chain; the PR also has an exact-head CI reproduction for its introduced test failure.

Real behavior proof
Sufficient (terminal): The PR body includes terminal output from a real local runtime resolver call after the patch showing tools.deny removes the message dispatch target while other real tools remain available.

Next step before merge
A maintainer can promote an automated repair because the remaining blockers are a focused test assertion and a CHANGELOG entry on an otherwise narrow PR.

Security
Cleared: The diff is security-sensitive but now mirrors the normal policy surface for allow/deny, ACP subagent envelopes, and sandbox state, with no supply-chain or remaining security regression found.

Review findings

  • [P2] Fix the sandbox dispatch assertion — src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts:955
  • [P3] Add the required changelog entry — src/auto-reply/reply/get-reply-inline-actions.ts:266-267
Review details

Best possible solution:

Land one canonical policy fix that mirrors normal tool surfaces, passes the focused auto-reply lane, and documents the user-visible hardening in CHANGELOG.md.

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

Yes. By source inspection against current main, inline skill dispatch constructs raw OpenClaw tools and only applies owner-only filtering while the documented contract requires the full policy chain; the PR also has an exact-head CI reproduction for its introduced test failure.

Is this the best way to solve the issue?

No, not yet. The latest design is the right narrow path and fixes the earlier ACP-envelope security gap, but the PR must fix its failing test expectation and add the changelog entry before merge.

Full review comments:

  • [P2] Fix the sandbox dispatch assertion — src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts:955
    The new sandbox runtime-state regression is failing on the PR head: the mocked tool returns { content: "listed" }, and handleInlineActions surfaces that text, so expecting ✅ Done. keeps checks-node-auto-reply-reply-dispatch red. Update the assertion or mock output so it matches the real behavior being tested.
    Confidence: 0.98
  • [P3] Add the required changelog entry — src/auto-reply/reply/get-reply-inline-actions.ts:266-267
    This branch changes visible/security-sensitive skill tool policy behavior by making denied or non-allowlisted inline dispatch targets unavailable. Repo policy requires user-facing fixes to be listed in CHANGELOG.md, but the PR only changes source and tests.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.95

Acceptance criteria:

  • pnpm test src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts -- --reporter=verbose
  • pnpm exec oxfmt --check --threads=1 src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts src/auto-reply/reply/get-reply-inline-actions.ts src/auto-reply/reply/skill-tool-dispatch.runtime.ts CHANGELOG.md
  • pnpm check:changed
  • pnpm build

What I checked:

Likely related people:

  • @vincentkoc: Recent history for the inline actions file includes fixes around inline status/command behavior and requester-agent preservation in the same auto-reply surface. (role: recent inline-dispatch maintainer; confidence: high; commits: 9aa9c3ff6262, 977778100157, ee8baf6766b2; files: src/auto-reply/reply/get-reply-inline-actions.ts, src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts)
  • @steipete: History shows repeated work on the shared tool-policy pipeline and current inline-actions refactors that this PR now reuses. (role: shared tool-policy maintainer; confidence: high; commits: f97ad8f288d2, 268c14f0210b, c4e5ca862517; files: src/agents/tool-policy-pipeline.ts, src/agents/tool-policy.ts, src/auto-reply/reply/get-reply-inline-actions.ts)
  • @pgondhi987: A recent merged change introduced/enforced subagent envelope inheritance on ACP child sessions, which is the policy path this PR now mirrors. (role: ACP subagent-envelope adjacent owner; confidence: medium; commits: 31160dc069b7; files: src/agents/subagent-capabilities.ts, src/agents/pi-tools.ts, src/gateway/tool-resolution.ts)
  • @eleqtrizit: Beyond authoring this PR, the contributor appears in merged history for restrictive tool allowlists and owner-only message-tool gating in adjacent policy code. (role: prior restrictive-policy contributor; confidence: medium; commits: 193fdd6e3b0c, fe0f686c9228; files: src/agents/tool-policy-pipeline.ts, src/agents/tool-policy.ts, src/auto-reply/reply/get-reply-inline-actions.ts)

Remaining risk / open question:

  • Exact-head CI is red for the touched auto-reply dispatch lane.
  • Several exact-head checks were still queued during inspection.
  • CHANGELOG.md is untouched despite a user-visible policy/security hardening change.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 74ec956e429b.

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 6, 2026
@eleqtrizit
Copy link
Copy Markdown
Contributor Author

eleqtrizit commented May 6, 2026

Standards pass on follow-up commit f0105aa:

  • Scoped the change to inline skill dispatch runtime behavior and focused auto-reply regressions.
  • Used the canonical subagent capability-store path for ACP-envelope sessions before applying subagent tool policy.
  • Threaded sandbox runtime state into inline tool construction so sandboxed dispatch uses the same runtime context as normal tool surfaces.
  • Added regressions for ACP-envelope subagent policy filtering and sandboxed inline dispatch construction.
  • Committed only the intended source and test paths via scripts/committer.

Validation:

  • git diff --check: passed.
  • pnpm exec oxfmt --check --threads=1 src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts src/auto-reply/reply/skill-tool-dispatch.runtime.ts: passed.
  • pnpm test src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts -- --reporter=verbose: attempted, but the local checkout cannot start the test because @openclaw/fs-safe/config is unavailable. Per repo rules, I ran pnpm install and retried once; install fails preparing the git-hosted @openclaw/fs-safe package because @types/mdx cannot find the JSX namespace. CI has been re-triggered on the pushed SHA for clean-environment proof.

This was referenced May 7, 2026
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR proof: sufficient ClawSweeper judged the real behavior proof convincing. size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@eleqtrizit