Skip to content

fix: generate non-empty approval_id when gateway approval.request omits it (#6008)#6168

Closed
webtecnica wants to merge 1 commit into
nesquena:masterfrom
webtecnica:fix/6008-approval-id-empty
Closed

fix: generate non-empty approval_id when gateway approval.request omits it (#6008)#6168
webtecnica wants to merge 1 commit into
nesquena:masterfrom
webtecnica:fix/6008-approval-id-empty

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Closes #6008

Problem

When Hermes Agent emits an approval.request SSE event without an approval_id or id field, _gateway_runs_approval_event() in api/gateway_chat.py produces an empty string for approval_id. This causes:

  1. The frontend approval card displays with approval_id: ""
  2. User clicks Allow/Deny → POST /api/approval/respond with empty approval_id
  3. The route handler rejects: if not approval_id: return bad(...)
  4. The run remains permanently blocked

Root cause

The Hermes Agent API server emits approval.request events with only run_id, timestamp, choices — but no approval_id:

# In hermes-agent gateway/platforms/api_server.py
{
    "event": "approval.request",
    "run_id": run_id,
    "timestamp": time.time(),
    "choices": ["once", "session", "always", "deny"],
    # no approval_id field
}

The WebUI adapter then does:

approval_id = str(payload.get("approval_id") or payload.get("id") or "").strip()
  → ""

Fix

Generate a uuid.uuid4().hex as the local approval_id when the upstream payload provides neither approval_id nor id. This is consistent with how route_approvals.submit_pending already handles the same situation for local approvals:

entry.setdefault("approval_id", uuid.uuid4().hex)

The Hermes Agent resolves approvals by run_id, not approval_id, so this ID is purely a WebUI-local correlation key.

Verification

  • 55 tests pass (pre-existing skips: 23)
  • All gateway approval, SSE, and runs-API tests green

@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where _gateway_runs_approval_event() produced an empty approval_id when the upstream Hermes Agent's approval.request SSE event omitted both approval_id and id fields. The empty string caused the frontend approval card to be unresponsive and permanently blocked the associated run.

  • Root cause & fix: A single two-line guard is added after the existing extraction logic: if approval_id is still empty, a uuid.uuid4().hex is generated as a WebUI-local correlation key, consistent with the setdefault pattern already used in route_approvals.submit_pending and reconcile_gateway_pending_mirror_locked.
  • End-to-end correctness: For the runs-API path, _STREAM_RUN_IDS[stream_id] is set at run creation, so the respond endpoint resolves run_id without relying on the generated approval_id. The generated UUID is forwarded to the upstream in the POST body, but the Hermes Agent processes approvals by run_id alone. The legacy gateway path is covered by submit_gateway_pending_mirror's mirror logic.

Confidence Score: 5/5

Safe to merge. The change is a two-line, narrowly scoped fallback that unblocks the approval flow when the upstream emits no approval ID.

The fix adds a UUID fallback in exactly the right place — after the existing extraction logic, before the value is used — and is consistent with the identical pattern already present in route_approvals.py. The runs-API path resolves the upstream run_id from _STREAM_RUN_IDS (set at run creation), so the generated UUID serves purely as a WebUI-local key. No pre-existing logic is disrupted, and the PR description includes passing test evidence.

No files require special attention.

Important Files Changed

Filename Overview
api/gateway_chat.py Adds import uuid and a two-line fallback to generate a UUID approval_id when the upstream payload provides none; change is minimal, targeted, and consistent with existing patterns in route_approvals.py.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Agent as Hermes Agent
    participant GW as gateway_chat.py
    participant FE as Frontend
    participant Route as /api/approval/respond
    participant RC as HttpRunnerClient

    Agent->>GW: SSE approval.request (run_id only, no approval_id)
    GW->>GW: "_gateway_runs_approval_event()<br/>approval_id = "" → uuid4().hex (new fix)"
    GW->>FE: "SSE "approval" event {approval_id: uuid, run_id: ...}"
    FE->>Route: "POST /api/approval/respond {approval_id: uuid, choice: "once"}"
    Route->>Route: _STREAM_RUN_IDS.get(active_sid) → run_id
    Route->>RC: respond_approval(run_id, approval_id, choice)
    RC->>Agent: "POST /v1/runs/{run_id}/approval {choice, approval_id}"
    Agent->>GW: SSE run.completed
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Agent as Hermes Agent
    participant GW as gateway_chat.py
    participant FE as Frontend
    participant Route as /api/approval/respond
    participant RC as HttpRunnerClient

    Agent->>GW: SSE approval.request (run_id only, no approval_id)
    GW->>GW: "_gateway_runs_approval_event()<br/>approval_id = "" → uuid4().hex (new fix)"
    GW->>FE: "SSE "approval" event {approval_id: uuid, run_id: ...}"
    FE->>Route: "POST /api/approval/respond {approval_id: uuid, choice: "once"}"
    Route->>Route: _STREAM_RUN_IDS.get(active_sid) → run_id
    Route->>RC: respond_approval(run_id, approval_id, choice)
    RC->>Agent: "POST /v1/runs/{run_id}/approval {choice, approval_id}"
    Agent->>GW: SSE run.completed
Loading

Reviews (2): Last reviewed commit: "fix: generate non-empty approval_id when..." | Re-trigger Greptile

@nesquena-hermes nesquena-hermes added the size:S Small PR (≤2 files, ≤30 LOC) label Jul 17, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

I traced this against both the WebUI respond path and the agent's approval handler on the read-only agent source, and the fix is correct — the contract claim in your description checks out. A couple of notes to give reviewers confidence, plus one optional robustness idea.

The bug is real and this is the right layer

The empty-approval_id rejection you describe is at api/routes.py:23696, inside the gateway-relay branch of _handle_approval_respond:

        if _run_id:
            if not approval_id:
                return bad(handler, "approval_id is required for gateway approvals")

So once a run_id is resolved (active stream or mirror recovery), an empty approval_id hard-fails before the relay ever fires. Your 2-line addition in _gateway_runs_approval_event (api/gateway_chat.py:337-338) fills that gap by minting a uuid4().hex when the upstream payload has neither approval_id nor id.

Contract verified: the agent resolves by run_id, not approval_id

This is the part worth confirming since it's a cross-repo assumption. The agent's POST /v1/runs/{run_id}/approval handler (_handle_run_approval in gateway/platforms/api_server.py:5086) reads only choice (and all/resolve_all) from the body — it never reads approval_id:

        run_id = request.match_info["run_id"]
        status = self._run_statuses.get(run_id)
        ...
        approval_session_key = self._run_approval_sessions.get(run_id)
        ...
        resolved = resolve_gateway_approval(approval_session_key, choice, resolve_all=resolve_all)

And on the emit side (api_server.py:4835), the approval.request event is built with run_id, timestamp, choices and no approval_id — exactly the omission you described. So a WebUI-minted approval_id is purely a local correlation key; it's safe on the relay path because the agent keys entirely off the URL run_id. Confirmed.

Consistency within the streaming path holds

In the legacy relay path (gateway_chat.py:891), the single approval_data dict (carrying the generated id) is reused for both put_gateway_event("approval", ...) and submit_gateway_pending_mirror(...), so the rendered card, the mirror entry, and the later /api/approval/respond all share the same id. That's what makes _gateway_mirrored_pending_run_id(sid, approval_id) recovery (routes.py:23692) work when the stream pointer is already cleared.

Optional: a run-scoped deterministic id would be more robust than uuid4

One edge case: because the id is minted fresh on every call to _gateway_runs_approval_event, an SSE reconnect that re-delivers the same approval.request would generate a different id, submit a second mirror entry, and re-render the card with the new id — leaving the previously-rendered card's id stale. Approve/deny still works via the active-stream path (agent resolves by run_id), but the mirror-recovery lookup by approval_id could miss on the stale card.

Since there's normally one pending approval per run at a time, deriving the fallback deterministically from run_id (e.g. f"gw-{run_id}" when the payload omits an id) would make re-delivery idempotent and keep card/mirror ids stable across reconnects, at no extra cost. Not a blocker — the current uuid4 is functionally correct — just a hardening idea if you want to touch it.

@webtecnica
webtecnica force-pushed the fix/6008-approval-id-empty branch from 34e8f54 to 143e742 Compare July 18, 2026 02:32
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

🔬 Gate certification — GREEN ✅ (full gate at head 143e742abeb5)

Certified head: 143e742abeb5df3698581f6564843de28b7f8b21 (clean-rebased onto origin/master; cumulative patch-id byte-identical before/after rebase, 330f60d6678c)
Verdict: Correct, minimal, regression-free reliability fix. Ready to merge.

What I ran

  • Codex (reproduce): SAFE TO SHIP — verified supplied approval_id/legacy id preserved, distinct uuids generated only when both empty, and the downstream respond path (api/routes.py:23783) resolves the run by run_id independently (the agent handler ignores the synthetic approval_id), so no mismatch.
  • Opus review: COMMENT — "correct, minimal, safe; verified it unblocks Follow-up to #5269: approval card is displayed, but approval_id is empty and approval cannot be submitted #6008 end-to-end with no regressions"; two optional non-blocking nits (below).
  • My own probe (sandboxed, real module): all invariants hold — omitted/empty → non-empty 32-char hex; provided id and legacy id preserved verbatim; two omitted events → distinct ids (no collision).
  • Full pytest suite to completion: 13356 passed, 14 failed — all 14 reproduce on the clean-origin/master control (env-only: no browser binary / git-check-ignore under a detached worktree / isolation flake); none touch api/gateway_chat.py, none introduced by this diff. Targeted test_gateway_approval_legacy_path.py 17/17. Ruff-forward / scope_undef / py_compile CLEAN.

The fix

_gateway_runs_approval_event (api/gateway_chat.py:339-341): when a gateway approval.request omits both approval_id and legacy id, synthesize uuid.uuid4().hex instead of leaving it empty. This matches the established design (#527: each pending approval needs a stable uuid so /api/approval/respond can target it) and unblocks #6008, where an empty id made the approval un-respondable.

Non-blocking (optional, maintainer's call — do not block merge)

  1. Add a one-line regression assert (assert downgraded["approval_id"]) to tests/test_gateway_approval_runs_api.py — the issue asked for it; the path is already covered structurally and by my probe. The PR ships no test of its own.
  2. The synthetic id is per-call, not per-run, so an SSE reconnect re-delivering the same approval.request yields a new id. Bounded: on the runs-API path (Follow-up to #5269: approval card is displayed, but approval_id is empty and approval cannot be submitted #6008's path) respond resolves by run_id so this is cosmetic; on the legacy-mirror path it's a genuine edge case the maintainer already called non-blocking. A deterministic f"gw-{run_id}" fallback (or a comment noting per-call) would make reconnects idempotent. Not required to ship.

Recommendation to the next agent

Ready to merge. gate-pass at sha:143e742abeb5. Correct, minimal, regression-free; Codex SAFE + Opus COMMENT + full suite clean + reproduced end-to-end. Preserve @webtecnica as author (CHANGELOG credit + release notes). The two items above are optional polish, not merge blockers.

@nesquena-hermes nesquena-hermes added the gate-pass Full gate passed (Codex+Opus+suite+browser); queued Tier 1 for release agent label Jul 19, 2026
nesquena-hermes added a commit that referenced this pull request Jul 19, 2026
…6180) + state-dir test isolation (#6305) (#6332)

* fix: catch unhandled exception in POST /api/updates/check (defensive hardening) (#6180)

* fix: generate non-empty approval_id when gateway approval.request omits it (#6008) (#6168)

* chore: mark update-check try/except as defensive-only guard, drop #6086 linkage

Per maintainer review, the try/except wrapper is defense-in-depth only —
it does NOT fix #6086 (root cause is signal/process-group reaping).
Updated log message and added inline comment to make this explicit.
Leave #6086 open.

* test: isolate state-dir probes from user state

* docs(changelog): stamp #6168 approval_id, #6180 update-check guard, #6305 test isolation

---------

Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
Co-authored-by: pxxD1998 <214340659+pxxD1998@users.noreply.github.com>
Co-authored-by: nesquena-hermes <agent@nesquena-hermes>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in exp-v0.52.117 (experimental channel) via release #6332. Thanks @webtecnica! 🎉

The gateway now synthesizes a non-empty uuid4().hex approval_id when an approval.request omits both approval_id and the legacy id, so the approval card is respondable (matching the #527 stable-uuid design). The downstream respond path resolves by run_id independently, so the synthetic id causes no mismatch, and a provided/legacy id is preserved verbatim.

Gate: full pytest suite (13443 passed / 0 failed), Codex regression gate SAFE TO SHIP, ruff clean. This exact head was also independently gate-certified earlier. Closes the routing gap for #6008.

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

Labels

gate-pass Full gate passed (Codex+Opus+suite+browser); queued Tier 1 for release agent size:S Small PR (≤2 files, ≤30 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up to #5269: approval card is displayed, but approval_id is empty and approval cannot be submitted

2 participants