Skip to content

fix(gateway): bridge Codex approvals to messaging platforms - #25889

Open
Fewmanism wants to merge 1 commit into
NousResearch:mainfrom
Fewmanism:fix/codex-gateway-approval-bridge
Open

fix(gateway): bridge Codex approvals to messaging platforms#25889
Fewmanism wants to merge 1 commit into
NousResearch:mainfrom
Fewmanism:fix/codex-gateway-approval-bridge

Conversation

@Fewmanism

Copy link
Copy Markdown
Contributor

Summary

  • add a reusable gateway approval helper for operations that already know they need user approval
  • bridge Codex app-server exec/apply-patch approval requests into Hermes' existing gateway approval queue
  • cover both exec and file-change approvals without a CLI callback, matching Telegram/Discord/etc. gateway sessions

Verification

  • ./venv/bin/python -m pytest tests/agent/transports/test_codex_app_server_session.py -q -n0
  • ./venv/bin/python -m pytest tests/gateway/test_approve_deny_commands.py tests/tools/test_approval_heartbeat.py tests/tools/test_approval_plugin_hooks.py -q -n0
  • ./scripts/run_tests.sh tests/agent/transports/test_codex_app_server_session.py tests/gateway/test_approve_deny_commands.py tests/tools/test_approval_heartbeat.py tests/tools/test_approval_plugin_hooks.py

@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 tackling the missing interactive gateway path. The premise still holds: current main declines Codex exec and apply-patch requests when no CLI callback exists (agent/transports/codex_app_server_session.py:882, :923). The merged opt-out routing change is complementary, not a replacement (agent/codex_runtime.py:347-381).

Problems

  • The proposed request_gateway_approval() copies the gateway queue wait loop, but omits the interrupt handling current main added in _await_gateway_decision() (tools/approval.py:2491-2507). A /stop or /new during a Codex approval could therefore wait for the full approval timeout.
  • Current main already centralizes queue insertion, heartbeat polling, hooks, cleanup, and interruption in _await_gateway_decision() (tools/approval.py:2419-2534); the new helper would duplicate that stateful logic.

Suggested changes

  • Port the fallback onto current main by using _await_gateway_decision() and mapping its resolved choice with _approval_choice_to_codex_decision().
  • Add a Codex gateway-approval interrupt regression alongside the existing approval interrupt coverage.

Automated hermes-sweeper review.

Comment thread tools/approval.py
@@ -551,6 +551,131 @@ def has_blocking_approval(session_key: str) -> bool:
return bool(_gateway_queues.get(session_key))

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.

Please avoid a second gateway approval wait implementation here. Current main centralizes queue lifecycle and, critically, /stop//new interruption in _await_gateway_decision() (tools/approval.py:2419-2534, especially :2491-2507); this copied loop has no interrupt check.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Four PRs address the missing Codex app-server approval path for gateway sessions. #25889 adds a transport-level fallback with a new queue loop, #27636 adds a runtime callback resolver with another queue loop, #43217 routes through the shared waiter but mixes in unrelated Telegram cache-path changes, and #66925 provides the current-main bridge while preserving policy floors, redaction, interruption, cleanup, and session-choice persistence.

Related pull requests

  • #25889 related — (+257/-25) — superseded by #66925: It directly forwards exec and apply-patch requests to a new gateway helper, but duplicates the approval wait loop and omits the shared interrupt handling. Despite the keep_open review on #25889, #66925 implements the requested salvage path through _await_gateway_decision() and adds the missing policy protections.
  • #27636 duplicate — (+330/-8) — superseded by #66925: It correctly identifies the runtime callback gap and covers smart approval, but re-reads HERMES_YOLO_MODE at request time and duplicates the queue wait loop without /stop or /new interruption. Despite the keep_open reviews on #27636, its requested callback bridge and shared-waiter corrections are present in #66925 without those defects.
  • #43217 [closed] related — (+268/-3) — closed reference implementation: It demonstrates routing through _await_gateway_decision(), but combines the approval fix with unrelated Telegram attachment/cache-path redaction changes and was closed as superseded. Despite its earlier approved review, its approval-specific approach is more cleanly and comprehensively implemented by #66925; the later contributor review also confirms that the merged bypass-only work did not replace the interactive bridge.
  • #66925 related — (+378/-41) — preferred consolidation target: It bridges the runtime callback to the existing shared queue, enforces hardline, sudo-stdin, and user-deny floors, force-redacts gateway payloads, and retains timeout, interruption, cleanup, and hook behavior. The keep_open review's session-persistence and lifecycle-test blockers were explicitly addressed in the shown diff with approve_session() and full-turn construction coverage.

Duplicates

#25889 and #27636 substantially duplicate the same Codex-to-gateway approval bridge; #43217 is an earlier closed implementation of that core change. #66925 is the hardened current-main consolidation of their relevant approval work.

Suggested consolidation

Merge #66925 after maintainer confirmation of the updated head and focused test results; its diff addresses the blocking contributor review while retaining the strongest parts of the competing implementations. Close #25889 and #27636 as superseded duplicates of #66925, and keep closed #43217 as a historical reference rather than reopening it.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup25889 ["PRs duplicating each other"]
        P25889["PR #25889 (open)"]
        P27636["PR #27636 (open)"]
    end
    class P25889 open
    class P27636 open
    class P25889 target
    click P25889 "https://github.com/NousResearch/hermes-agent/pull/25889"
    click P27636 "https://github.com/NousResearch/hermes-agent/pull/27636"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 63 kB of PR diffs, 6 kB of issue/PR text, 12 kB of discussion (15 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@Fewmanism

Copy link
Copy Markdown
Contributor Author

Thanks for consolidating the related approval-bridge work. I agree that #66925 now covers the requested current-main salvage path more comprehensively than this PR, including the shared waiter, interruption behavior, policy floors, redaction, and session-choice persistence.

I’m happy to close #25889 as superseded once maintainers confirm #66925 as the canonical target. No further implementation work is planned here in the meantime.

@teknium1, does that consolidation direction match your preference?

@GottZ GottZ 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.

This was generated by AI during triage.

Delta since our previous triage comment

@Fewmanism explicitly confirms that #66925 now covers #25889’s intended current-main salvage path more comprehensively and that no further implementation work is planned on #25889. They are willing to close #25889 as superseded once maintainers designate #66925 as the canonical target; the PR remains open pending that confirmation.

Changed pull requests

  • #25889 related — (+257/-25) — supersession now supported by the author: @Fewmanism confirms that #66925 includes the shared waiter, interruption handling, policy floors, redaction, and session-choice persistence missing from this diff. Despite the earlier keep_open review on #25889, that review’s requested salvage path is implemented in #66925, and the author now plans no further work here.

Suggested consolidation

The recommendation is unchanged: select #66925 as the canonical target and close #25889 as superseded once maintainers confirm that direction.

Complex graph unchanged since our previous triage comment.

Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 63 kB of PR diffs, 6 kB of issue/PR text, 12 kB of discussion (16 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants