Skip to content

fix(gateway): advertise exec approval node commands#88296

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fix/windows-node-exec-approvals
Closed

fix(gateway): advertise exec approval node commands#88296
vincentkoc wants to merge 1 commit into
mainfrom
fix/windows-node-exec-approvals

Conversation

@vincentkoc

@vincentkoc vincentkoc commented May 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Fixes #57775 by keeping system.execApprovals.get and system.execApprovals.set in the desktop node command policy instead of dropping them after the node host advertises them.

The commands are now eligible during desktop node pairing and preserved on approved reconnects, so nodes describe / Control UI can see exec-approval-capable Windows nodes. Raw node.invoke still rejects system.execApprovals.*; callers must continue through the scoped exec.approvals.node.* RPCs.

Verification

WSL2 status

The WSL2 probe found wsl.exe, Windows 10.0.26100.4946, PowerShell 7.4.11, and default WSL version 2, but Ubuntu import/exec still failed (wsl2_ok=false, wsl_import_exit=-1, wsl_exec_exit=-1). Enabling HypervisorPlatform returned 3010 (reboot required), and Microsoft-Hyper-V-All returned -2146498548. The workflow was intentionally non-failing for WSL2 because the runner needs a reboot/capability recovery before WSL2 can be used as proof.

Notes

This is the narrow exec-approval advertisement fix from the PR #74163 Windows node/exec cluster. #84644 (Commands: none reported) and #77730 (file-transfer nodeHostCommands missing from live handshake) look related but are broader command-surface/plugin-discovery failures; they need separate proof/fix work rather than being bundled into this change.

@vincentkoc vincentkoc self-assigned this May 30, 2026
@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S maintainer Maintainer-authored PR labels May 30, 2026
@clawsweeper

clawsweeper Bot commented May 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 5:41 PM ET / 21:41 UTC.

Summary
The PR adds system.execApprovals.get/set to the desktop node command policy and adds regression tests for pairing and approved reconnect command preservation.

PR surface: Source +3, Tests +74. Total +77 across 3 files.

Reproducibility: yes. source-reproducible: current main's node host advertises system.execApprovals.get/set, while the gateway policy omits those IDs from the allowlist used by pairing and reconnect command preservation.

Review metrics: 1 noteworthy metric.

  • Exec-approval command IDs: 2 added. These IDs decide whether approved desktop node hosts can advertise exec-approval capability without granting raw invoke access.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #57775
Summary: This PR is the focused candidate fix for the canonical exec-approval command advertisement gap; broader Windows node command-surface items overlap but do not supersede it.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] The PR intentionally advertises approval-editing command IDs, so a maintainer should confirm at merge time that raw node.invoke remains blocked and only exec.approvals.node.* can invoke them.
  • [P1] This read-only review did not rerun a live Windows node at the final merge result; the PR body cites focused tests and Windows probe evidence at the PR head.

Maintainer options:

  1. Land after boundary review (recommended)
    Confirm the final merge result still blocks raw node.invoke system.execApprovals.* and then treat this narrow advertisement fix as ready to land.
  2. Hold for exact-head Windows proof
    Keep the draft open if maintainers want a fresh nodes describe or Control UI target-list proof from a real Windows node at the merge head.

Next step before merge

  • [P2] Protected maintainer draft plus security-boundary merge risk require human review rather than cleanup close or automated repair.

Security
Cleared: The diff touches a security-sensitive advertisement path, but no concrete security or supply-chain regression was found because raw node.invoke remains blocked and no dependency/workflow/secret surface changes are introduced.

Review details

Best possible solution:

Land this focused policy fix after maintainer security-boundary review, then close #57775 while keeping broader Windows command-surface reports separate.

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

Yes, source-reproducible: current main's node host advertises system.execApprovals.get/set, while the gateway policy omits those IDs from the allowlist used by pairing and reconnect command preservation.

Is this the best way to solve the issue?

Yes. Adding the shared exec-approval command constants to the existing desktop pairing/runtime command policy is the narrowest maintainable fix, provided the raw node.invoke boundary remains intact.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded gateway/node advertisement bug with limited blast radius while scoped CLI approval RPCs still work.
  • merge-risk: 🚨 security-boundary: The PR changes advertisement of approval-editing node commands, so the raw invoke block and scoped RPC boundary need maintainer attention before merge.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this maintainer/member PR; the body still cites focused tests and Windows probe evidence.
Evidence reviewed

PR surface:

Source +3, Tests +74. Total +77 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 3 0 +3
Tests 2 74 0 +74
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 77 0 +77

What I checked:

  • Root policy read: Root AGENTS.md was read and applied because the PR touches gateway command policy and a security-boundary command advertisement path. (AGENTS.md:23, 15de9d881a84)
  • Scoped gateway policy read: The scoped gateway guide was read; it reinforced reviewing gateway hot paths and test shape beyond the small diff. (src/gateway/AGENTS.md:1, 15de9d881a84)
  • Current main still has the gap: Current main's SYSTEM_COMMANDS and DESKTOP_HOST_COMMANDS include run/notify/browser/screen commands but not NODE_EXEC_APPROVALS_COMMANDS. (src/gateway/node-command-policy.ts:55, 15de9d881a84)
  • Latest release still has the gap: The latest release tag v2026.6.10 shows the same omission, so the PR is not already shipped. (src/gateway/node-command-policy.ts:55, aa69b12d0086)
  • PR implements the missing policy entries: The PR head imports NODE_EXEC_APPROVALS_COMMANDS and adds it to both SYSTEM_COMMANDS and DESKTOP_HOST_COMMANDS. (src/gateway/node-command-policy.ts:49, 486d285c24f1)
  • Node host already declares the commands: The node host sends ...NODE_EXEC_APPROVALS_COMMANDS in its gateway command list, so the mismatch is in gateway policy filtering rather than node-host declaration. (src/node-host/runner.ts:282, 15de9d881a84)

Likely related people:

  • vincentkoc: Authored this focused PR and recent gateway/node-host approval seam work in the affected area. (role: current fix owner and recent area contributor; confidence: high; commits: 486d285c24f1, 0f7d9c957093; files: src/gateway/node-command-policy.ts, src/gateway/node-command-policy.test.ts, src/gateway/node-connect-reconcile.test.ts)
  • Peter Steinberger: History shows earlier exec-approval tooling, shared node command catalog, and raw node.invoke exec-approval boundary work. (role: feature-history contributor; confidence: high; commits: 3686bde783fd, d06632ba45a8, 01b3226ecbea; files: src/infra/node-commands.ts, src/gateway/node-command-policy.ts, src/gateway/server-methods/nodes.ts)
  • Agustin Rivera: Authored the node reconnect command-upgrade pairing behavior that shares this command preservation invariant. (role: adjacent node pairing contributor; confidence: medium; commits: a383878e97b7; files: src/gateway/node-connect-reconcile.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T09:02:53.493Z sha 486d285 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels May 30, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 14, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 19, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 20, 2026
Closed
This was referenced Jun 28, 2026
Closed
Closed
Closed
Closed
@steipete steipete mentioned this pull request Jul 6, 2026
Merged
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Superseded and landed through the refreshed maintainer replay #100505 in commit fccb888.

The landed version preserves this PR's pairing/reconnect fix and adds the missing security boundary at the dedicated relay: effective approved commands and the live Gateway allowlist are checked before node invocation. Raw node.invoke remains prohibited for these commands.

Thank you @vincentkoc for finding and implementing the core advertisement fix. The stale branch could not be refreshed safely, so I replayed it with co-author credit and stronger regression coverage. Enabling Allow edits by maintainers on future contributor branches lets us update the original directly.

@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

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows headless node host supports exec approvals via CLI, but nodes describe / Control UI do not advertise system.execApprovals.get/set

2 participants

@vincentkoc @steipete