fix(gateway): advertise exec approval node commands#100505
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 933553f57f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const SYSTEM_COMMANDS = [ | ||
| ...NODE_SYSTEM_RUN_COMMANDS, | ||
| ...NODE_EXEC_APPROVALS_COMMANDS, |
There was a problem hiding this comment.
Require admin to approve exec-approval node commands
When a desktop node declares system.execApprovals.get/set, adding those commands to the pairable desktop command defaults means they can now be persisted in the paired-node command surface. However src/infra/node-pairing-authz.ts still only treats NODE_SYSTEM_RUN_COMMANDS as requiring operator.admin, so a caller with operator.pairing + operator.write can approve a node that gains the admin-scoped exec-approval command surface. Please include NODE_EXEC_APPROVALS_COMMANDS in the admin-required pairing-scope check before advertising them for pairing.
Useful? React with 👍 / 👎.
|
Land-ready at exact head Work completed:
Proof:
Known proof gap: direct sanitized AWS contributor-head execution was unavailable because the shared lease pool was over capacity. The same reviewed tree was replayed by a maintainer and proved on Blacksmith Testbox plus exact-head hosted CI. |
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 10:02 PM ET / 02:02 UTC. Summary PR surface: Source +27, Tests +206. Total +233 across 5 files. Reproducibility: yes. as a source-level reproduction. Current source shows the node host advertises system.execApprovals.get/set and the UI requires those command IDs, while the gateway command policy needed this PR to preserve them for approved desktop nodes. Review metrics: 1 noteworthy metric.
Stored data model 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:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Add exec-approval commands to the admin-required node pairing authorization set with focused helper and gateway RPC coverage, then land this focused gateway advertisement fix for #57775. Do we have a high-confidence way to reproduce the issue? Yes, as a source-level reproduction. Current source shows the node host advertises system.execApprovals.get/set and the UI requires those command IDs, while the gateway command policy needed this PR to preserve them for approved desktop nodes. Is this the best way to solve the issue? No, not yet. The command-policy and relay checks are the right layer for the advertisement bug, but the best fix must also update node pairing authorization so these admin-scoped commands require operator.admin when approved. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d095f4755339. Label changesLabel justifications:
Evidence reviewedPR surface: Source +27, Tests +206. Total +233 across 5 files. View PR surface stats
Security concerns:
Acceptance criteria:
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
Review history (2 earlier review cycles) |
|
Land-ready at exact head Review and proof completed:
The CI run's first compact-small shard attempt timed out in unrelated No known proof gaps remain. |
|
Final resync proof for exact PR head
No known proof gaps remain. Landing this exact head now. |
bb7aef5 to
15d1912
Compare
Co-authored-by: Vincent Koc <[email protected]>
72795db to
865b8db
Compare
|
Final land-ready proof for the clean exact head This is the intended five-file policy fix only:
Proof:
The earlier polluted sync head was explicitly discarded; this comment and the native gate apply only to |
|
Merged via squash.
|
Co-authored-by: Vincent Koc <[email protected]>
Co-authored-by: Vincent Koc <[email protected]>
What Problem This Solves
Desktop node hosts advertise
system.execApprovals.getandsystem.execApprovals.set, but Gateway policy removed those commands before the approved node surface reached the Control UI. The dedicated exec-approval RPCs also relayed directly to the node without rechecking the effective paired surface or livegateway.nodes.denyCommandspolicy.Closes #57775. Maintainer replay of #88296 because the stale contributor branch could not be safely updated in place.
Why This Change Was Made
node.invokerejection andoperator.adminRPC scope.Co-authored-bytrailer and changelog credit.User Impact
The Control UI can read and update approval settings on approved capable desktop nodes. Pending pairing and operator deny rules remain fail-closed.
Evidence
pnpm check:changed: passed, including core/type/lint/policy guards.