fix(gateway): exec approvals CLI shows clear error when node lacks approvals capability#97585
fix(gateway): exec approvals CLI shows clear error when node lacks approvals capability#97585Sanjays2402 wants to merge 1 commit into
Conversation
…provals capability
The exec.approvals.node.{get,set} gateway relay invoked the node command
without first checking the node's declared command surface. When the node
host advertised system.run but not system.execApprovals.get/set, the node
returned no payload, the CLI cast that to ExecApprovalsSnapshot, and
formatting threw 'Cannot read properties of undefined (reading socket)'
instead of an actionable capability error.
Preflight nodeSession.commands the same way node.invoke does via
isNodeCommandAllowed and return a structured INVALID_REQUEST with
reason 'command not declared by node' and a hint that names the node,
its platform, and the missing system.execApprovals command. The CLI now
surfaces a clear capability mismatch, matching the docs contract for
--node exec approvals targets.
When the node session is not yet registered the relay still falls
through to nodeRegistry.invoke so existing wake / NOT_CONNECTED handling
is unchanged.
Fixes openclaw#97578
|
Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 7:19 PM ET / 23:19 UTC. Summary PR surface: Source +44, Tests +193. Total +237 across 2 files. Reproducibility: yes. from source, though not live in this read-only review: current main relays Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a narrow combined fix that returns a structured unsupported-capability error while preserving capable desktop node approval management, then add real CLI/Gateway proof. Do we have a high-confidence way to reproduce the issue? Yes from source, though not live in this read-only review: current main relays Is this the best way to solve the issue? No, not as submitted. Gateway-boundary validation is the right layer, but it must be aligned with node command reconciliation so capable nodes are not misclassified as unsupported. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 19cac35a06ed. Label changesLabel justifications:
Evidence reviewedPR surface: Source +44, Tests +193. Total +237 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
|
Superseded by the stronger landed fix #100505 / fccb888. The landed implementation keeps this PR's dedicated relay preflight, but checks the effective approved session surface and the current Gateway allowlist rather than only raw command advertisement. It also includes the paired desktop command-preservation fix needed for capable nodes to succeed. Thank you @Sanjays2402 for the careful report and regression work. |
2 participants
Fixes #97578
What Problem This Solves
Fixes an issue where operators running
openclaw approvals get --node <node-id> --jsonwould see a rawCannot read properties of undefined (reading 'socket')TypeError when the selected node host did not advertise thesystem.execApprovals.get/system.execApprovals.setcommands. The CLI is the affected surface, but the root cause is the Gateway exec-approvals relay accepting any connected node without checking its declared command surface.The reporter's case: a Windows node connected and advertised
system.run,system.run.prepare, andsystem.which, but notsystem.execApprovals.get/system.execApprovals.set. The relay still forwarded the call, the node returned no payload, and the CLI's renderer tried to readfile.socket?.pathonundefined, surfacing an internal stack trace instead of the real operator problem.Why This Change Was Made
Adds a narrow Gateway-boundary preflight inside
respondWithExecApprovalsNodePayloadfor bothexec.approvals.node.getandexec.approvals.node.set. WhennodeRegistry.get(nodeId)returns a session, the relay now checks that the node's declaredcommandsinclude the targetsystem.execApprovals.*method before invoking. If the command is not declared, the relay returns a structuredINVALID_REQUESTerror with:message: names the node id, the platform, and the missingsystem.execApprovals.*command, with the same wording the docs use for--nodetargets ("the node must advertise … or edit the node host approvals file directly").details.reason:"command not declared by node"— the same reason stringnode.invokealready returns viaisNodeCommandAllowed, so existing client error mapping keeps working.details.command/details.nodeId: machine-readable fields for--jsonconsumers.This mirrors the existing invariant in
src/gateway/server-methods/nodes.tsfor the genericnode.invokepath; the approvals relay was the only sibling that skipped it.Boundaries:
nodeRegistry.invokeso existing wake /NOT_CONNECTEDhandling is unchanged.exec.approvals.{get,set}paths.file.socket?.path) is intentionally not touched; with the preflight in place the gateway no longer returns an empty payload here, and changing the renderer would broaden scope past the reported defect.User Impact
openclaw approvals get --node <node-id>against a node that lacks exec-approvals capability now see a clear, actionable message telling them which RPC the node must advertise and that they can edit the node-local approvals file as a workaround, instead of a rawCannot read properties of undefined (reading 'socket')TypeError.--jsonconsumers get the same information inerror.code(INVALID_REQUEST),error.message, anderror.details.{reason,command,nodeId}— so dashboards and scripts can recognize the capability mismatch and route the operator to the correct remediation.NOT_CONNECTEDpath still runs, asserted by test).Evidence
Added regression coverage in
src/gateway/server-methods/exec-approvals.test.ts. The 4 new tests fail onmainand pass with this change (verified both directions locally by stashing the source change and re-running):short-circuits exec.approvals.node.get with INVALID_REQUEST when the node does not declare system.execApprovals.get— reproduces the reported scenario (Windows node,commands: [system.run, system.run.prepare, system.which]). Asserts the relay never reachesnodeRegistry.invoke, the response isINVALID_REQUEST, the message mentionssystem.execApprovals.get, the node id, and the platform, anddetails.reason === "command not declared by node".short-circuits exec.approvals.node.set with INVALID_REQUEST when the node does not declare system.execApprovals.set— covers the write path (node declaresgetbut notset).forwards exec.approvals.node.get to the node when system.execApprovals.get is declared— locks in the happy path so the preflight does not regress the normal case.falls through to invoke when the node session is not registered (preserves NOT_CONNECTED wake/error mapping)— confirms the change does not interfere with the cold-node wake path that depends onnodeRegistry.invokereturningNOT_CONNECTED.Local runs (Vitest is project-sharded; this file runs in both
gateway-clientandgateway-methodsprojects):Adjacent suites still green:
Lint and format on the changed files:
Diff scope:
AI disclosure
This change was written with the assistance of Claude. I have reviewed the diff and the test and take responsibility for the code.