Skip to content

Surface approval rule keys in gateway clients - #59298

Open
doncazper wants to merge 1 commit into
NousResearch:mainfrom
doncazper:codex/desktop-gateway-approval-rule-key
Open

doncazper wants to merge 1 commit into
NousResearch:mainfrom
doncazper:codex/desktop-gateway-approval-rule-key

Conversation

@doncazper

@doncazper doncazper commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Rebuilt directly on current main after the original plugin approval base from #59163 landed through #60504.

  • add shared approval metadata (pattern_keys, allowlist_key, allow_permanent, and plugin rule_key) to gateway approval payloads
  • preserve the current blocking _await_gateway_decision() notify/response flow for plugin approvals
  • carry rule metadata and Smart DENY capability constraints through command, execute_code, no-callback, Desktop, and TUI paths
  • display the effective approval rule in Desktop and TUI prompts
  • add backend round-trip, Desktop event-parsing/display, and TUI event/display coverage

Validation

  • .venv/bin/python -m pytest tests/tools/test_request_tool_approval.py tests/tools/test_command_guards.py::TestGatewayApprovalAllowPermanent -q
  • .venv/bin/ruff check tools/approval.py tests/tools/test_request_tool_approval.py tests/tools/test_command_guards.py
  • npm --workspace apps/desktop run test:ui -- src/app/session/hooks/use-message-stream/approval-rule-metadata-event.test.tsx src/store/prompts.test.ts src/components/assistant-ui/tool/approval.test.tsx
  • npm --workspace apps/desktop run typecheck
  • npm --workspace @hermes/ink run build
  • npm --workspace ui-tui run typecheck
  • npm --workspace ui-tui test -- src/__tests__/createGatewayEventHandler.test.ts src/__tests__/approvalAction.test.ts
  • Prettier and git diff --check
  • GitHub CI: all required checks pass

@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels Jul 6, 2026
@Vansh5632

Vansh5632 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Pulled this stacked on #59163 while tracing #51221 (plugin pre_tool_call approve - works on CLI, no controls on desktop/gateway).

Nice improvement on metadata (_approval_metadata, rule_key / allowlist_key in Desktop/TUI). But _run_approval_gate() gateway branch still calls submit_pending() only - it doesn't use _await_gateway_decision() + the notify callback like check_all_command_guards() does. So approval.request never fires for plugin approvals, /approve won't see a pending item, and #51221's repro may still fail even after #59163 + this land.

Suggest wiring plugin gateway approvals through the same blocking path as terminal commands before calling this done for #51221.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for carrying the approval metadata through Desktop and TUI. The metadata gap is still present on current main: ui-tui/src/gatewayTypes.ts:695-697 and ui-tui/src/app/createGatewayEventHandler.ts:783-795 do not retain the allowlist or plugin-rule fields.

Problems

  • The stacked base is already present as 36308f066, but current main subsequently added the required plugin gateway notify/response round-trip in 117f49b7d (tools/approval.py:2209-2272). The PR's older submit_pending()-only branch must not replace that behavior; it would reintroduce the failure described in the existing review comment.

Suggested changes

  • Salvage only the metadata propagation onto the current _await_gateway_decision() and no-callback fallback payloads in tools/approval.py:2222-2291, then carry it through the existing command and execute_code payload paths.
  • Keep the Desktop/TUI event parsing and display tests, and add a gateway-round-trip assertion that a plugin approval emits the metadata and unblocks after approval.respond.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch from 7a5a7a0 to 77de5ca Compare July 16, 2026 22:25

Copy link
Copy Markdown
Contributor Author

Rebuilt this PR directly on current main and force-updated the branch in 77de5ca79, removing the obsolete stacked-base history.

The salvage preserves the newer blocking _await_gateway_decision() notify/response path and layers approval metadata onto both notify and no-callback payloads. Command and execute_code paths retain Smart DENY capability constraints. I also added a plugin gateway round-trip regression plus a Desktop event-parsing regression.

Verification:

  • focused Python approval tests: 16 passed; Ruff passed
  • Desktop approval/event tests: 26 passed; typecheck passed
  • TUI approval/event tests: 90 passed; typecheck passed
  • @hermes/ink build passed
  • Prettier and git diff --check passed

@alt-glitch alt-glitch added comp/plugins Plugin system and bundled plugins area/auth Authentication, OAuth, credential pools and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins labels Jul 17, 2026
@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch from 77de5ca to 47e7ec7 Compare July 22, 2026 00:40

doncazper commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this PR onto current main (8208fc527) and force-pushed the refreshed commit as 47e7ec7a9.

Conflict resolution preserved current main’s newer allow_session behavior and the shared _await_gateway_decision path, while retaining this PR’s approval metadata propagation. I also adapted the rebased code from the removed has_tirith variable to current main’s has_permanent_capable capability calculation.

Validation:

  • backend PR-focused tests: 17 passed
  • broader approval/command-guard suite: 358 passed; 1 unrelated /tmp path test fails identically on untouched current main
  • Desktop focused tests: 26 passed; Desktop typecheck passed
  • TUI focused tests: 97 passed; TUI typecheck passed
  • Ink build passed
  • Ruff, Prettier (touched TS/TSX), and git diff --check passed
  • GitHub CI: all 25 executed jobs passed; 5 path-filtered jobs skipped

@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 22, 2026
@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch from 47e7ec7 to e7dfb72 Compare August 1, 2026 02:37

Copy link
Copy Markdown
Contributor Author

Rebase update: this is now based on current main, including the current Desktop/TUI gateway client structure and lint rules. Validation: 41 focused Python tests, 26 Desktop tests, 96 TUI tests, both typechecks, changed-file ESLint, and Ruff pass; the full PR check suite is green.

@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch 2 times, most recently from ded4e9f to 3b2ff95 Compare August 8, 2026 19:57
@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch 2 times, most recently from c7610d0 to 0fb64bd Compare August 15, 2026 16:06
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix: surface approval rule keys in gateway clients

  1. allowlist_key is always == pattern_key: _approval_metadata (tools/approval.py line 2695-2712) sets allowlist_key to the same value as pattern_key unconditionally. Carrying two keys that can never differ invites future drift (one consumer may start distinguishing them) — either keep only pattern_key and let renderers alias it, or document the intended semantic difference (allowlist key = storage key, pattern key = match key) with a case where they'd differ.
  2. rule_key derivation is prefix-coupled: rule_key is only emitted when pattern_key starts with plugin_rule: (line 2704-2706) — a non-plugin allowlist (e.g. a user cmd: rule or hardline pattern) gets no rule_key, so the desktop/TUI "Rule" line silently falls back to the raw allowlist key. Fine, but the fallback chain (ruleKey || allowlistKey || patternKey in approval.tsx / prompts.tsx) means the label shown can be a long internal key rather than a human rule name — consider a display-name mapping.
  3. Positive behavior change worth calling out: moving approval_data construction before the notify_cb branch (line 3439-3453) means the queued submit_pending fallback now carries the redacted command/description (previously raw display_target/description). That's a security improvement — the pending-approval queue no longer stores unredacted text.
  4. allow_permanent handling in check_all_command_guards now flows through _approval_metadata with the smart_denied_for_owner condition — consistent. Test coverage across backend, desktop store/UI, TUI, and docs (ui-tui/README event schema) is thorough.

@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch from 0fb64bd to b978eb1 Compare August 22, 2026 16:52
@doncazper

Copy link
Copy Markdown
Contributor Author

Rebased onto current main, resolved the desktop message-module split, and ported approval rule metadata through the current gateway event path. Added an explicit backend contract note that allowlist_key is the compatibility alias for the exact persisted Session/Always rule, while pattern_keys remains the full matched set.

Validation:

  • backend approval suites: 41 passed
  • desktop focused tests: 29 passed
  • TUI gateway-event tests: 105 passed
  • desktop and TUI typechecks: passed
  • Ruff and git diff --check: passed

@alt-glitch alt-glitch added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 22, 2026
@doncazper
doncazper force-pushed the codex/desktop-gateway-approval-rule-key branch from b978eb1 to 6bf1337 Compare August 24, 2026 16:42
@doncazper

Copy link
Copy Markdown
Contributor Author

Refreshed this branch onto current main (057dcdf236) after the upstream drift. Rebase was conflict-free.

Current-head verification (6bf13379bf):

  • uv run --extra dev pytest tests/tools/test_command_guards.py tests/tools/test_request_tool_approval.py -q — 41 passed
  • Ruff on the touched Python files — passed
  • desktop focused Vitest — 31 passed
  • desktop TypeScript typecheck — passed
  • TUI focused Vitest — 105 passed
  • TUI TypeScript typecheck — passed
  • git diff --check origin/main...HEAD — passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/desktop Electron desktop app (apps/desktop/*) comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants