Skip to content

fix(gateway): forward approval denial reason - #70226

Open
s-a-s-k-i-a wants to merge 1 commit into
NousResearch:mainfrom
s-a-s-k-i-a:fix/3-approval-denial-reason
Open

fix(gateway): forward approval denial reason#70226
s-a-s-k-i-a wants to merge 1 commit into
NousResearch:mainfrom
s-a-s-k-i-a:fix/3-approval-denial-reason

Conversation

@s-a-s-k-i-a

@s-a-s-k-i-a s-a-s-k-i-a commented Jul 23, 2026

Copy link
Copy Markdown

Problem

approval.respond accepted a denial reason at the desktop/gateway RPC boundary but silently dropped it before calling the existing approval resolver. Any UI that collected a denial explanation therefore appeared to work while the backend never received the text.

Premise and root cause

The lower-level resolve_gateway_approval() contract already supports an optional reason. The live gateway handler forwarded session_key, choice, and resolve_all, but omitted params["reason"]. This is a boundary propagation bug, not a new approval feature.

During review, upstream commit f67ca220ab had moved the active approval.respond handler from tui_gateway/server.py to tui_gateway/methods_prompt.py. The branch is now rebased onto current main and the one-line fix is applied only at that live call site.

Changes

  • Forward the optional reason from approval.respond to resolve_gateway_approval() in tui_gateway/methods_prompt.py.
  • Keep the boundary-level server.handle_request regression test, which proves choice, reason, and resolve_all arrive together through the registered split-module handler.
  • Keep calls without a reason backward-compatible (None).

Before / after

Request Before After
deny without reason resolves denial unchanged
deny with reason reason silently discarded resolver receives exact reason
allow / resolve-all forwarded unchanged

Validation

RED on current main

HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh \
  tests/test_tui_gateway_server.py \
  -k test_approval_respond_forwards_reason_and_optional_resolve_all -q
# 1 failed: actual resolver call omitted reason

GREEN

HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh \
  tests/test_tui_gateway_server.py \
  -k test_approval_respond_forwards_reason_and_optional_resolve_all -q
# 1 passed

scripts/run_tests.sh \
  tests/test_tui_gateway_server.py \
  tests/tui_gateway/test_protocol.py \
  tests/tools/test_approval.py \
  tests/gateway/test_approve_deny_commands.py \
  -k 'not test_nonrecursive_verification_artifact_cleanup_is_not_dangerous' -q
# 636 passed

uvx ruff check tui_gateway/methods_prompt.py tests/test_tui_gateway_server.py
# All checks passed

git diff --check origin/main...HEAD
# clean

The excluded verification-artifact cleanup case is an unrelated existing macOS /tmp path assertion. It was rerun in a detached worktree at unchanged current base c55159f18 and failed identically there.

Scope receipt

  • 2 files changed
  • 46 insertions, 0 deletions
  • 1 production line; the remaining additions are one focused behavior test
  • branch rebased onto current origin/main (c55159f18)
  • original author and author date preserved

Non-goals

  • No new approval semantics.
  • No desktop/pet UI changes.
  • No session, prompt-store, queue, or IPC changes.

Tracks s-a-s-k-i-a#3
Part of s-a-s-k-i-a#1
Replaces the first narrow prerequisite extracted from #70156.

@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 the focused regression coverage. The underlying propagation bug is still present on current main: tui_gateway/methods_prompt.py:875-879 forwards choice and resolve_all but not reason, while tools/approval.py:2198-2230 accepts and stores the reason for the blocked-action feedback path.

Problems

  • The production hunk now targets a stale location. Commit f67ca220ab5ba1f2a764b84b9c8db7d003a042f6 moved the approval.respond handler from tui_gateway/server.py to tui_gateway/methods_prompt.py; current dispatch installs that module in tui_gateway/server.py:13292-13306.

Suggested changes

  • Salvage the one-line forwarding change into tui_gateway/methods_prompt.py:875-879. The added server.handle_request regression test remains the appropriate boundary-level coverage after resolving its placement conflict.

Automated hermes-sweeper review.

Comment thread tui_gateway/server.py Outdated
@@ -11919,6 +11919,7 @@ def _(rid, params: dict) -> dict:
"resolved": resolve_gateway_approval(

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.

This is the right forwarding change, but current main moved the approval.respond handler to tui_gateway/methods_prompt.py:864-883 in f67ca220ab. Please apply this line at that live call site during salvage; this former server.py location is no longer the active handler.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hold my beer.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 1226bd3. I rebased the branch onto current main (c55159f), dropped the stale server.py hunk, and applied the one-line reason forwarding at the live tui_gateway/methods_prompt.py handler. The existing server.handle_request boundary test was preserved and reproduced RED on current main before the fix, then GREEN after it. The focused gateway/protocol/approval matrix is 636 passed; Ruff and git diff --check are clean. The one excluded macOS /tmp assertion fails identically on an unchanged c55159f worktree. Thanks for the precise salvage pointer.

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

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants