Skip to content

[codex] Route app-server approvals through gateway - #43217

Closed
BenKalsky wants to merge 2 commits into
NousResearch:mainfrom
Digitizers:fix/codex-app-server-gateway-approvals
Closed

BenKalsky wants to merge 2 commits into
NousResearch:mainfrom
Digitizers:fix/codex-app-server-gateway-approvals

Conversation

@BenKalsky

@BenKalsky BenKalsky commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds prompt_gateway_approval() as a synchronous bridge onto the existing gateway approval queue.
  • Wires codex_app_server turns to use that bridge when running inside a gateway session without a CLI approval callback.
  • Adds regression coverage for queue resolution and Codex app-server callback wiring.

Why

When Codex app-server requested approval for an exec or file-change action from a Telegram/Gateway session, Hermes had no CLI approval callback to pass into the Codex runtime. The Codex side then failed closed, which surfaced as a rejection even though no Telegram approval buttons were shown to the user.

Validation

.venv/bin/python -m pytest \
  tests/tools/test_approval.py::TestApprovalTimeoutIsNotConsent::test_prompt_gateway_approval_uses_queue_and_returns_choice \
  tests/tools/test_approval.py::TestApprovalTimeoutIsNotConsent::test_prompt_gateway_approval_maps_disallowed_always_to_session \
  tests/run_agent/test_codex_app_server_integration.py::TestRunConversationCodexPath::test_gateway_session_wires_approval_callback \
  tests/agent/transports/test_codex_app_server_session.py::TestServerRequestRouting::test_callback_raises_falls_back_to_decline

Result: 4 passed in 2.39s.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/acp Agent Communication Protocol adapter comp/gateway Gateway runner, session dispatch, delivery codex labels Jun 10, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Code Review — Verified Clean ✅

Reviewed the diff (222 lines across 4 files). The change bridges Codex app-server approval prompts through 's per-session queue when no terminal-local callback is available (gateway/cron context).

What's good:

  • Fallback chain is correct: → gateway queue → codex fail-closed default
  • gracefully handles missing session context
  • scopes approval patterns per-command
  • distinguishes from terminal approval UI
  • Both new tests ( and ) verify the full callback chain including session key propagation and return value

No issues found. The approval routing correctly addresses the gateway gap where terminal-local callbacks are unavailable.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Looks Good

  • Clean fix: route app-server approvals through the gateway for proper authorization handling.
  • Changes are focused on codex_runtime.py and tools/approval.py.
  • Good test coverage in test_codex_app_server_integration.py and test_approval.py.
  • No security concerns - this improves the security model by routing approvals through the gateway.

Reviewed by Hermes Agent

@BenKalsky
BenKalsky force-pushed the fix/codex-app-server-gateway-approvals branch from ba3036a to 48d0272 Compare June 10, 2026 10:38
@BenKalsky
BenKalsky marked this pull request as ready for review June 10, 2026 10:38
@BenKalsky

Copy link
Copy Markdown
Contributor Author

Rebased on latest upstream main and resolved the conflict in tests/run_agent/test_codex_app_server_integration.py by keeping both the new upstream token-accounting coverage and the gateway approval callback coverage.

Updated head: 48d0272970bfa6e3be0ccdf8cfbec402d3d12c23

Validation run locally:

uv run --extra dev pytest tests/run_agent/test_codex_app_server_integration.py tests/tools/test_approval.py -q
# 227 passed in 7.56s

The PR is now out of draft and GitHub reports it as mergeable; it appears blocked only on normal review/maintainer requirements.

@BenKalsky
BenKalsky force-pushed the fix/codex-app-server-gateway-approvals branch from 48d0272 to cee68d4 Compare June 11, 2026 20:15
@BenKalsky

Copy link
Copy Markdown
Contributor Author

Updated the PR head to the clean rebased commit:

  • Head is now cee68d47ffa5082b98fe3ab922499a9c54f450c3
  • Rebased on current upstream main at a09343cc98b5df5374bda72575c78992527c71f7
  • The branch was updated via --force-with-lease from the stale 48d0272970bfa6e3be0ccdf8cfbec402d3d12c23 head

The PR is open, not draft, and GitHub reports it as mergeable. Since the previous approval was attached to an older commit, this should be ready for a fresh maintainer review / merge consideration.

@BenKalsky
BenKalsky force-pushed the fix/codex-app-server-gateway-approvals branch 2 times, most recently from 6eda791 to eed769b Compare June 19, 2026 21:48
@BenKalsky

Copy link
Copy Markdown
Contributor Author

Follow-up note, no code change needed right now: if another user-visible Telegram response leaks an internal document cache path outside the currently covered .hermes/cache/documents/ and /tmp/hermes*/cache/documents/ shapes, extend _GATEWAY_CACHE_DOCUMENT_PATH_RE with a path-specific regression test. For now this is intentionally left as tracking only, since the current patch covers the observed leak and avoids broad redaction behavior changes.

@BenKalsky
BenKalsky force-pushed the fix/codex-app-server-gateway-approvals branch from ffebf9d to abfc131 Compare June 22, 2026 15:26
@BenKalsky

Copy link
Copy Markdown
Contributor Author

Refreshed the PR state against current main.

GitHub now reports:

  • mergeable: MERGEABLE
  • mergeStateStatus: BLOCKED
  • head: abfc131a2
  • base: b1b20270c

Local validation on the current head:

uv run --extra dev pytest tests/run_agent/test_codex_app_server_integration.py tests/tools/test_approval.py tests/gateway/test_document_context_note.py tests/gateway/test_telegram_group_gating.py -q
# 306 passed in 7.60s

There does not appear to be any remaining code/rebase action needed from the contributor side; this looks ready for maintainer review / merge consideration.

@BenKalsky

Copy link
Copy Markdown
Contributor Author

Closing this as superseded by #56534, which has been merged and already references this PR. The gateway-context approval/yolo issue is resolved there.

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

Labels

codex comp/acp Agent Communication Protocol adapter comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants