Skip to content

feat(runs): recover pending approvals by exact ID - #75707

Open
dliu120 wants to merge 19 commits into
NousResearch:mainfrom
dliu120:fix/runs-approval-recovery
Open

feat(runs): recover pending approvals by exact ID#75707
dliu120 wants to merge 19 commits into
NousResearch:mainfrom
dliu120:fix/runs-approval-recovery

Conversation

@dliu120

@dliu120 dliu120 commented Jul 31, 2026

Copy link
Copy Markdown

What does this PR do?

Adds recoverable, exactly correlated approvals to the Runs API and every interactive client path. A client that loses its event stream can poll the complete pending queue, answer one immutable approval ID, and safely reject stale, unavailable, or post-stop controls.

The implementation keeps approval authority on the backend: clients render the choices actually offered, send the exact approval ID, and fail closed when that request is no longer pending.

Related Issue

Related: https://github.com/dliu120/voice-pilot/issues/33

Companion VoicePilot client PR: https://github.com/dliu120/voice-pilot/pull/163

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Return every pending run approval in FIFO order from the pollable Runs API.
  • Require exact approval IDs, validate the advertised choice set, and reject stale, bulk, or post-stop decisions.
  • Bind relay and messaging-platform controls to the backend approval request so old buttons cannot fall through to newer work.
  • Preserve queued approvals and exact identities across Desktop, native notifications, and TUI presentation.
  • Document the run approval request/response contract and recovery behavior.

How to Test

  1. Run the focused isolated Python suite:
    scripts/run_tests.sh tests/gateway/test_approve_deny_commands.py tests/gateway/test_api_server_runs.py tests/gateway/relay/test_relay_interactive.py tests/gateway/relay/test_relay_slack_prompt_dm_root.py tests/gateway/test_whatsapp_cloud.py tests/gateway/test_qqbot.py tests/gateway/test_slack_approval_buttons.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_feishu_approval_buttons.py tests/gateway/test_matrix_exec_approval.py tests/gateway/test_teams.py tests/test_tui_gateway_server.py -q
  2. From ui-tui/, run mise exec node@22 -- npm run check and mise exec node@22 -- npm run build.
  3. From apps/desktop/, run typecheck, lint, tests, and build under Node 22.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.2 arm64 using automated gates

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — automated adapter coverage only
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Validation Limits

No physical Xbox Adaptive Controller validation, human VoiceOver walkthrough, or live Hermes end-to-end validation was performed. Reach, placement, activation force, fatigue, switch bounce, timing, and end-to-end latency claims remain unvalidated. Palm, wrist, forearm, and elbow activation or positioning remain field-validation risks with the target user.

Unsupported choices are not offered. Unsafe or missing action details remain visible but deny-only.

The two repositories have no shared integration CI, so cross-repository contract evolution remains a risk.

Screenshots / Logs

N/A. The interaction contract is covered by automated tests; no screenshot was captured.

@dliu120

dliu120 commented Jul 31, 2026

Copy link
Copy Markdown
Author

Companion VoicePilot client PR: https://github.com/dliu120/voice-pilot/pull/163

The two changes share one contract: Hermes owns exact-ID approval authority and recoverable FIFO snapshots; VoicePilot presents only the bounded choices for that immutable request and fails closed when it becomes stale.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) platform/discord Discord bot adapter platform/feishu Feishu / Lark adapter platform/matrix Matrix adapter (E2EE) platform/qqbot QQ Bot adapter labels Jul 31, 2026
@alt-glitch alt-glitch added platform/slack Slack app adapter platform/telegram Telegram bot adapter platform/whatsapp WhatsApp Business adapter area/auth Authentication, OAuth, credential pools sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-decision Awaiting maintainer decision before any implementation labels Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #63197: both introduce backend-owned approval identities, but this PR carries exact-ID recovery through existing Desktop, TUI, relay, and messaging surfaces while #63197 is the opt-in mobile contract stack. Maintainer contract/consolidation decision needed; this is not a duplicate.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carrying exact approval identity through the Runs API and interactive clients. The current main implementation is still FIFO-based (tools/approval.py:2198-2231; gateway/platforms/api_server.py:6566-6600), so the underlying problem is real.

Problems

  • tui_gateway/methods_prompt.py now requires an ID, but validates a response only against the global canonical set before resolving it. The unchanged TUI event producer in tui_gateway/server.py:1599-1606 derives choices without considering allow_session. A request that disallows session scope can therefore be displayed with session, and the exact-ID handler will accept it. This conflicts with the new offered-choice contract already enforced for Runs API requests.

Suggested changes

  • Derive TUI choices from allow_session too, and validate the selected choice against the exact queued approval before calling resolve_gateway_approval. Add a regression for allow_session=false rejecting session.

Automated hermes-sweeper review.

Comment thread tui_gateway/methods_prompt.py
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 31, 2026
@dliu120

dliu120 commented Jul 31, 2026

Copy link
Copy Markdown
Author

Addressed the review finding in 4b8bc66. The queue authority now rejects choices not offered by the exact approval, and the TUI uses that same policy when rendering choices. Added resolver, TUI emission, and JSON-RPC regressions; the affected 545-test suite passed, and the broader focused run passed 754 approval/adapter tests with one unrelated concurrent-write timing test passing immediately in isolation.

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/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/discord Discord bot adapter platform/feishu Feishu / Lark adapter platform/matrix Matrix adapter (E2EE) platform/qqbot QQ Bot adapter platform/slack Slack app adapter platform/telegram Telegram bot adapter platform/whatsapp WhatsApp Business adapter sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants