Skip to content

fix(gateway): reject invoke for unadvertised node commands#98051

Closed
ly-wang19 wants to merge 2 commits into
openclaw:mainfrom
ly-wang19:fix/exec-approvals-node-capability-check-97578
Closed

fix(gateway): reject invoke for unadvertised node commands#98051
ly-wang19 wants to merge 2 commits into
openclaw:mainfrom
ly-wang19:fix/exec-approvals-node-capability-check-97578

Conversation

@ly-wang19

Copy link
Copy Markdown
Contributor

What Problem This Solves

openclaw approvals get --node <id> --json against a connected node that does not advertise system.execApprovals.get/set currently surfaces an unstructured error (Cannot read properties of undefined (reading 'socket')) instead of a clear unsupported-capability message.

Closes #97578.

Why This Change Was Made

nodeRegistry.invoke checks only whether the node is connected. It never consults the per-session command list, so a request for a command the node never advertised is sent over the wire and the gateway surfaces whatever the node-side dispatch produces (in practice, an internal TypeError that varies by node implementation). Adding a capability gate inside invoke itself — returning a structured UNSUPPORTED_CAPABILITY error when the node has explicitly advertised a commands list that does not include the requested command — lets every caller return an actionable error instead of relying on each handler to duplicate the check. The gate mirrors the existing semantics in isNodeCommandAllowed (src/gateway/node-command-policy.ts): an empty declared-commands list keeps legacy behavior, so the gate only fires when the node has opted into command advertisement.

User Impact

  • openclaw approvals get/set --node <id> against a node that does not support exec approvals management now returns a structured error (UNSUPPORTED_CAPABILITY: node does not support command "system.execApprovals.get") instead of a confusing TypeError.
  • Nodes that did not declare a commands list are unaffected (legacy behavior preserved).
  • Other gateway handlers that route through nodeRegistry.invoke (e.g. system.run, canvas commands) keep their existing behavior when the node already advertises those commands.

Evidence

  • New regression tests in src/gateway/node-registry.test.ts:
    • rejects invoke for commands the node did not advertised — verifies a node that advertised system.run* and system.which returns UNSUPPORTED_CAPABILITY for system.execApprovals.get.
    • still invokes when the node did not declare a commands list — verifies the legacy empty-commands-list path still sends the invoke frame.
  • node scripts/run-vitest.mjs node-registry.test exits 0.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S labels Jun 30, 2026
@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: stale review; fresh review needed.

Summary
The latest durable ClawSweeper review was for head f8626483479f209e5fa9b558d651066af8b22e15, but the PR head is now b6f92049e244a99acb69c9a862cfd5d1ab440e60. Its old verdict and PR readiness labels are no longer current.

Next step
Run or wait for a fresh ClawSweeper review on the current PR head.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 30, 2026
ly-wang19 added 2 commits July 2, 2026 00:39
When a node explicitly advertises a commands list but omits a command
the gateway tries to invoke (e.g. system.execApprovals.get on a node
that only advertises system.run* and system.which), the gateway now
returns a structured UNSUPPORTED_CAPABILITY error instead of sending
the invoke and surfacing whatever unstructured error bubbles back.
Nodes that did not declare commands keep legacy behavior so the
capability gate only fires when the node opted into command
advertisement.

Closes openclaw#97578.
The new branch returns the minimal { ok: false, error } shape (matching
the sibling NOT_CONNECTED failure above). The test was over-asserting
payload/payloadJSON fields the code never sets on this failure path.
@ly-wang19
ly-wang19 force-pushed the fix/exec-approvals-node-capability-check-97578 branch from f862648 to b6f9204 Compare July 1, 2026 17:11
@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jul 2, 2026
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Superseded by #100505 / fccb888.

The landed fix enforces this invariant at the dedicated exec-approvals relay, where the admin-scoped exception is owned, instead of changing NodeRegistry.invoke semantics for every node command. It checks both the effective approved surface and the current Gateway allowlist and has focused pairing/relay regressions.

Thank you @ly-wang19 for the candidate and tests.

@steipete steipete closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gateway Gateway runtime P2 Normal backlog priority with limited blast radius. size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OpenClaw CLI should handle node lacks exec approvals capability gracefully

2 participants