Skip to content

fix(gateway): route plain-text approval responses (salvage #46924) - #2

Merged
hashbender merged 1 commit into
mainfrom
mirror/pr-55884
Jun 30, 2026
Merged

fix(gateway): route plain-text approval responses (salvage #46924)#2
hashbender merged 1 commit into
mainfrom
mirror/pr-55884

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Summary

Replying "yes" / "approve" / "deny" (plain text, no slash) now resolves a pending dangerous-command approval on messaging platforms — previously it deadlocked into an auto-deny.

Root cause: when the agent is blocked inside tools/approval.py waiting for approval, a bare-word reply fell through to the steer/queue/interrupt logic in _handle_active_session_busy_message. The reply got queued behind a turn that can't start until the approval resolves, so the approval timed out and auto-denied. Slash forms (/approve, /deny) already worked; bare words (what Signal/SMS users naturally type) did not.

Salvage of @liuhao1024's NousResearch#46924 — their commit's authorship is preserved. Our follow-up commit reuses the canonical handlers and delivers the confirmation reply.

Changes

  • gateway/run.py: in _handle_active_session_busy_message, when has_blocking_approval(session_key) is true, route bare-word approval vocab (yes/approve/ok/y/confirm/deny/no/reject/cancel/n/always/session) through the existing /approve and /deny handlers — which resolve the waiting thread, resume typing, and return a localized confirmation — then deliver that confirmation to the user (it was silent before). Synthesizes a literal /-prefixed command so get_command_args() parses always/session on every platform (is_command() only recognizes /).
  • tests/gateway/test_plaintext_approval_routing.py: E2E tests over the real busy-handler path.

Why this location is correct

The base-adapter guard (gateway/platforms/base.py) invokes the busy-session handler before falling back to queueing, so plain text does reach this handler. The fix sits before the steer/queue logic and after the early-return guards (draining, internal synthetic events). The has_blocking_approval gate is the disambiguator — a conversational "yes" with no pending approval is never treated as command approval (preserving the design intent at run.py's "Pending exec approvals are handled by /approve and /deny" note).

Validation

Before After
Signal/SMS reply "yes" to approve queued → timeout → auto-deny resolves approval, command runs
User feedback after plain-text reply silent localized confirmation sent
always / session modifiers not parsed parsed via synthesized /approve <arg>
Conversational "yes" (no approval pending) n/a not consumed as approval

14 E2E tests green; adjacent approval/busy suites (test_approve_deny_commands.py, test_busy_session_ack.py) pass with no regressions.

Infographic

PR #46924 plain-text approval routing

Closes NousResearch#46866.


Mirror-of: NousResearch#55884
NousResearch#55884

@tenki-reviewer

tenki-reviewer Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Complete

Files Reviewed: 2
Findings: 3

By Severity:

  • 🟡 Medium: 3

Three medium-severity bugs found in the plain-text approval routing: a bound-method identity check always evaluating to False (line 4781), an overbroad exception handler that silently drops user security decisions (line 4803), and a session-key namespace mismatch breaking approval routing under multiplex_profiles (line 4758).

Files Reviewed (2 files)
gateway/run.py
tests/gateway/test_plaintext_approval_routing.py

@tenki-reviewer tenki-reviewer Bot 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.

Risk: 🟠 High (72/100) — 3 medium findings · 271 LOC across 2 files


Summary

This PR adds plain-text approval routing to _handle_active_session_busy_message in gateway/run.py, allowing users to type "yes"/"no"/"deny"/"always" to respond to dangerous-command approval prompts instead of using the /approve or /deny slash commands. The feature works correctly in the happy path but has three distinct bugs uncovered by the review.

Findings

finding-001: Bound-method identity check (is) always False

File: gateway/run.py:4781 — The is operator on CPython bound methods always returns False because each attribute access creates a new bound method object. The _verb variable is always "deny", even when the approve handler is running. The approval works (the handler stored in _approval_handler is correct), but logging and synthesized command text are wrong.

finding-002: Exception handler falls through to busy handling

File: gateway/run.py:4756-4808 — The broad try/except Exception wraps both approval resolution AND reply sending. If the reply send fails after a successful resolution, the function falls through to normal busy handling instead of returning True. Worse, if the approval handler itself raises before resolving, the exception is silently swallowed — the user's explicit security decision is discarded.

finding-003: Session key mismatch under multiplex_profiles

File: gateway/run.py:4758 — The session_key from the adapter is computed without a profile parameter, always landing in the agent:main namespace. Under multiplex_profiles, approval queue entries are keyed with agent:<profile>:..., so has_blocking_approval() returns False and the plain-text routing is silently skipped. The slash-command path is unaffected.

Risk Assessment

All three findings are in the security-critical dangerous-command approval path. The is bug is trivially fixable (one character). The exception-handling fix requires restructuring the try/except scopes. The session-key fix requires recomputing the key. None are exploitable for privilege escalation, but they degrade the reliability and auditability of the approval mechanism.

Comment thread gateway/run.py
# MessageEvent.is_command()/get_command_args() only
# recognize the "/" prefix, not the per-platform display
# prefix ("!" on Slack/Matrix).
_verb = "approve" if _approval_handler is self._handle_approve_command else "deny"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Bound-method identity check (is) always evaluates to False, causing wrong verb in synthesized command (bug)

At gateway/run.py line 4781, the code uses the is operator to compare bound methods:

_verb = "approve" if _approval_handler is self._handle_approve_command else "deny"

In CPython, every access to self._handle_approve_command creates a new bound method object via PyMethod_New. Therefore _approval_handler is self._handle_approve_command always evaluates to False, even when _approval_handler was assigned from self._handle_approve_command moments earlier.

Impact:

  • _verb is always "deny" when the approve handler is invoked (e.g., user types 'yes', 'always', 'session')
  • The logged message at line 4787 records verb=deny even for approve actions, misleading debugging
  • The synthesized event.text (line 4782-4784) becomes /deny or /deny always instead of /approve or /approve always
  • The deny path coincidentally gets the correct verb because the else branch matches
  • The approval resolution itself works correctly because _approval_handler is the correct handler (stored at lines 4765, 4769, 4772) and the handler doesn't check the command name in the synthesized text

💡 Suggestion: Change is to == on line 4781. Bound methods implement __eq__ which correctly compares by __self__ and __func__, returning True when the same method is bound to the same instance.

Suggested change
_verb = "approve" if _approval_handler is self._handle_approve_command else "deny"
_verb = "approve" if _approval_handler == self._handle_approve_command else "deny"
📋 Prompt for AI Agents

In gateway/run.py at line 4781, fix a single-character bug: change _approval_handler is self._handle_approve_command to _approval_handler == self._handle_approve_command. The is operator fails for bound methods in CPython because each attribute access creates a new bound method object; == uses the bound method's __eq__ implementation which correctly compares by __self__ and __func__. Without this fix, the logged verb is always 'deny' (the else branch) regardless of whether the user approved or denied.

Comment thread gateway/run.py
Comment on lines +4803 to +4808
except Exception:
logger.warning(
"Plain-text approval routing failed for session %s; "
"falling through to busy handling",
session_key, exc_info=True,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Exception handler falls through to busy handling after approval resolution, leaking mutated event state (bug)

In gateway/run.py, the _handle_active_session_busy_message method (line 4690) contains a new plain-text approval routing block (lines 4756-4808). This try/except block covers BOTH the approval resolution (await _approval_handler(event) at line 4786) AND the confirmation reply send (await _adapter._send_with_retry(...) at line 4796).

Consequence A — resolved approval not handled: If the approval is successfully resolved (entry.event.set() called inside resolve_gateway_approval()) but the subsequent reply send fails (e.g., permanent network error after retries), the exception is caught and the function falls through to normal busy handling (line 4810+). The approval has already been resolved (agent unblocked), but the function does not return True — it falls through to steer/queue/interrupt logic that may try to interrupt the now-running agent or queue a spurious /approve event.

Consequence B — mutated event.text leaks: At line 4785, event.text is mutated to a synthesized '/approve' or '/deny' string before calling the handler. If any exception occurs after this mutation (even if the handler itself fails), the mutated text persists and flows into the normal busy handling code. The steer/queue/interrupt logic sees a synthetic slash command instead of the original user message, violating event lifecycle expectations.

Consequence C — handler failure silently dropped: If _handle_approve_command or _handle_deny_command raises BEFORE resolving the approval (e.g., session store unavailable, i18n key drift, unexpected synthesized-command parsing), the exception is caught and the user's explicit approval/denial is silently discarded. The slash-command dispatch paths (/approve, /deny) propagate such exceptions to the base adapter's error handler — the plain-text path's silent fallback is a regression relative to slash commands.

💡 Suggestion: Restructure the exception handling so that: (1) the approval handler call and reply send are in separate try/except scopes, (2) after the approval handler returns successfully, the function always returns True regardless of reply send success, and (3) if the approval handler itself raises, restore event.text to its original value and propagate the exception so the base adapter can surface the error to the user.

📋 Prompt for AI Agents

In gateway/run.py, in the _handle_active_session_busy_message method around lines 4756-4808, restructure the try/except block. Save event.text before mutation (line 4785). Move the approval handler call (line 4786) outside the broad try. Wrap only the reply send (lines 4791-4801) in its own try/except that logs and consumes exceptions without falling through. Ensure return True after the approval handler succeeds. If the approval handler itself raises, restore event.text and propagate the exception so the base adapter can log it and the user receives an error message, matching the slash-command dispatch path behavior.

Comment thread gateway/run.py
# we deliver it ourselves (mirroring the draining-case send above).
try:
from tools.approval import has_blocking_approval
if has_blocking_approval(session_key):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Session key mismatch breaks plain-text approval routing under multiplex_profiles (bug)

The plain-text approval routing added in gateway/run.py line 4758 uses the session_key parameter received from the base adapter. The adapter computes this key at gateway/platforms/base.py:4383 via build_session_key(event.source, ...) WITHOUT a profile argument, always producing keys in the agent:main namespace. However, the approval queue entries in tools/approval.py are keyed by the agent's session key, which includes the profile namespace when gateway.multiplex_profiles=True (see gateway/session.py:1023-1030 and _session_key_namespace at line 734-751).

When a non-default profile is active under multiplexing, has_blocking_approval(base_adapter_key) returns False even when a blocking approval exists under the runner's key (agent:<profile>:...). The plain-text response silently falls through to normal busy handling, and the approval times out and auto-denies — exactly the deadlock this PR (NousResearch#46866) was designed to fix. The existing slash-command path (/approve, /deny) is unaffected because it routes through the GatewayRunner's handler which computes its own key via _session_key_for_source().

💡 Suggestion: Use self._session_key_for_source(event.source) instead of the adapter-provided session_key parameter for the has_blocking_approval check and for the logger message at line 4789. This ensures the gate check uses the same key namespace as the approval queue entries.

📋 Prompt for AI Agents

In gateway/run.py, _handle_active_session_busy_message at line 4758: replace the adapter-provided session_key with a recomputed key. Change:

if has_blocking_approval(session_key):

to:

_resolved_key = self._session_key_for_source(event.source)
if has_blocking_approval(_resolved_key):

Also update the logger at line 4789 to use _resolved_key instead of session_key so the log message reflects the correct session namespace.

@hashbender
hashbender merged commit 7e09c6a into main Jun 30, 2026
31 checks passed
hashbender pushed a commit that referenced this pull request Jul 27, 2026
…ch#67140)

The background write guard decided ownership from `isinstance(usage_rec, dict)`,
so a local skill with NO usage record passed. That successful write called
bump_patch(), which created a `created_by: null` record — and the identical
write was refused from then on. "Allowed exactly once, then never" is a race
with our own bookkeeping, not a policy. Reproduced on main: patch #1 succeeds,
patch #2 with the same arguments is refused.

Option B from the issue. Option A (split `session_review` from
`scheduled_curator` and let the session fork patch user-owned skills it
consulted) would widen autonomous write permission onto skills the user owns
with no user present to consent — wrong direction for a no-user-present actor.

- skill_manager_tool: missing and explicit-null records now resolve
  IDENTICALLY, both fail closed. The refusal names the reason and points at
  `hermes curator adopt <name>`.
- background_review: both review prompts told the reviewer to patch any skill
  consulted in the session and claimed pinned skills could be improved, while
  enforcement refused both. Prompts now list pinned, external, and user-owned
  skills as protected, and tell the reviewer to RECOMMEND adoption instead of
  attempting a write that will be refused.
- skill_usage: document that `created_by` is a curator-management policy flag,
  not a provenance claim, and add `is_curator_managed()` so call sites read as
  the question they ask. Field name retained — it is on disk in every
  `.usage.json` and renaming would strand those records.
- curator CLI: `hermes curator list-unmanaged` itemizes unmanaged skills with
  the reason each is unmanaged (completes the NousResearch#67139 spec).

Foreground writes are untouched: a user-directed edit to a user-owned skill
still works, including on pinned skills.

Sibling tests: 9 failures in test_skill_manager_tool.py were fixtures that
created record-less skills to exercise OTHER guards (consolidation-delete,
read-before-write) and relied on ownership falling through. Fixed at the
fixture, since the real curator only ever operates on managed sediment. One
test asserted the old "manually authored" wording; rewritten to assert the
behavior contract instead of the string.

Validation: 274 targeted tests + all 7 background-review files (60 tests) pass.
E2E on a temp HERMES_HOME (30 checks) covers the flip, foreground writes,
adoption unblocking, pin semantics, prompt/enforcement parity, and the new verb.
Each new test sabotage-verified: revert the fix, confirm it goes red.

Fixes NousResearch#67140
hashbender pushed a commit that referenced this pull request Jul 27, 2026
…hat tile (NousResearch#71969)

* fix: Branch button is a dead no-op inside a branched chat tile

session-tile.tsx wired onBranchInNewChat to () => undefined for
tiled/branched sessions (nested branching isn't supported there), but
the button in AssistantMessage's action bar rendered unconditionally
regardless of whether a real handler was supplied. The button looked
clickable but silently did nothing, with no visual feedback.

- AssistantMessage now only renders the Branch button when
  onBranchInNewChat is actually provided, matching the existing
  pattern used for onDismissError/onRestoreToMessage.
- session-tile.tsx no longer passes a no-op handler; the prop is
  simply omitted so the button doesn't render in tiles.
- onBranchInNewChat is now optional on ChatViewProps, and the
  latestChatActions passthrough wrapper uses the existing
  latestOptional helper instead of an unconditional call.

* test: assert Branch button visibility matches handler presence

Adds coverage for the bug #2 fix: renders Thread with and without an
onBranchInNewChat handler and asserts the Branch in new chat button
is shown only when a real handler is supplied, hidden otherwise -
covering both the normal open-chat case and the session-tile
(branched chat) case that used to leave a dead, clickable button.
hashbender pushed a commit that referenced this pull request Aug 4, 2026
… a broken chat

A completely unconfigured install previously booted into a working-looking
chat (banner showed model 'unknown'), accepted a message, spun ~30s, then
failed with 'Set OPENROUTER_API_KEY' — a provider the user never chose —
and never offered setup.

- HermesCLI.run() now probes provider readiness at startup (TTY only) and
  offers the shared provider picker (hermes model flow, which fronts Quick
  Setup / Nous Portal OAuth) when nothing is configured. Decline is
  respected; picker state re-syncs into the live CLI so the next turn works
  without a restart.
- New silent probe _runtime_credentials_ready(): no printing, no state
  mutation; handles keyless local endpoints and callable bearer providers.
- The empty-api-key error is provider-aware: names the actual resolved
  provider and points at 'hermes model' / 'hermes setup' instead of
  hardcoding OPENROUTER_API_KEY.
- Banner: unconfigured installs render 'no model configured — run /model'
  in red instead of the silent 'unknown' model slug.

Consumer-onboarding audit finding #2 (sev 5), Aug 2026.
hashbender pushed a commit that referenced this pull request Aug 4, 2026
A wedged adapter transport (network hang, dead websocket) previously
blocked _check_session_stalls forever: sibling candidates in the same
pass were never evaluated and the watcher stopped ticking. Wrap the
send in asyncio.wait_for (15s); on timeout log a WARNING and do NOT
latch, so the next tick retries. Regression uses a never-resolving fake
adapter and proves the pass completes, a healthy sibling candidate is
still notified in the same pass, and the watcher ticks again
(sabotage-verified against the unbounded send).
hashbender pushed a commit that referenced this pull request Aug 11, 2026
…on delegation callbacks (NousResearch#82592)

* fix(gateway): stop frozen-preview finals and dropped idle-session delegation callbacks

Two relay-plane delivery losses from the 2026-08-09 staging incident:

1. stream_consumer: the skip-redundant-finalize branch recorded _accumulated
   as the delivered turn-final payload even when the last ACKED edit was an
   earlier throttled preview snapshot, so delivered_final_matches reconciled
   True and the gateway suppressed the corrective final send — the user was
   left with a cut-off message ending in the streaming cursor. Extracted
   _mark_skip_redundant_finalize(): records the last acked wire payload
   (cursor-stripped), so a preview/final mismatch now returns False and the
   normal final send fires.

2. run.py: _classify_completion_target classified every ended parent session
   terminal unless it ended by compression. Idle/timeout session ends are the
   norm on scale-to-zero relay deployments and the chat route remains valid;
   completed async delegation results were terminally dropped. Ended parents
   now classify deliver unless the end was an explicit user boundary
   (session_reset / user_exit / session_switch).

* fix(relay): drain in-flight outbound frames before transport teardown

disconnect() failed every pending outbound future immediately with
'relay transport closed', so a trailing finalize edit racing turn
teardown was lost even though the connector socket could still serve
it. Bounded drain grace (5s) lets in-flight requests resolve; silent
connectors still tear down promptly. asyncio.wait (not gather+wait_for)
so a timeout doesn't cancel futures owned by the fail-remaining loop.

* fix(gateway): route completion injection through the alias-aware transport resolver

Third relay-plane delivery loss from the 2026-08-09 staging incidents: a
delegation batch completed while the gateway was up, the watcher drained
the event, and delivery vanished with no log line. _inject_watch_notification
resolved its adapter with a literal p.value == platform_name scan of
self.adapters — a relay-fronted gateway registers ONE adapter under
Platform.RELAY fronting N logical platforms, so 'slack' never matched and
the injection returned None ('no gateway route'), silently dropping the
completion. The handoff path already documents this exact trap and uses
resolve_delivery_transport; the injection path now does the same (native
wins; relay eligible only when it fronts the logical platform), with the
literal scan kept as fallback for stub runners and exotic platforms.

* fix(relay): clamp disconnect drain grace to the runner's adapter-disconnect budget

Review finding (JoaoMarcos44, NousResearch#82592): a fixed 5.0s drain in front of the
three 1.0s sequential teardown awaits gives an 8.0s worst case inside the
runner's 5.0s asyncio.wait_for(adapter.disconnect()) — tripping it cancels
teardown mid-drain, skips the fail-pending loop, and leaves outbound
callers blocked until _OUTBOUND_TIMEOUT_S (30s). The effective grace is
now budget - 3*TEARDOWN - margin (env-aware via the same
HERMES_GATEWAY_ADAPTER_DISCONNECT_TIMEOUT the runner reads), so the drain
can never push teardown past its caller's budget; a budget too small for
any drain disables it cleanly.

* test(gateway): pin the final-send suppression contract across a behaviour matrix

The gateway skips its own final send when the stream consumer claims the turn
final already reached the user. Every incident in that family — NousResearch#71643 (stale
finalize snapshot), NousResearch#78541 (payload-less multi-message split), NousResearch#82656 (frozen
preview left with a visible cursor) — is the same failure: the consumer claimed
delivery for text the platform never rendered, so the corrective send was
suppressed and the answer was lost with no retry.

Each was fixed with a scenario test pinned to one branch of
GatewayStreamConsumer.run(). The got_done handler now has five sibling branches
that each set the suppression flags and record a turn-final payload, and nothing
checks them as a group: a new branch, or a new early `return True` in
_send_or_edit, can reintroduce the class without failing a test.

Pin the invariant instead of the branch — if the consumer offers the gateway any
signal it would trust, the complete final text must have reached the wire — and
assert it across {edit always / dies / never / lies} x {send always / never} x
{fresh-final on / off} x {clean / interrupted stream}.

The adapter records only frames that actually rendered, so an ACK the platform
drops does not count as delivery. 24 honest-transport scenarios hold the
invariant as a hard assertion. The 16 lying-transport scenarios are checked too;
the single combination that still violates it is reported as an expected
failure documenting the open exposure rather than asserting it away.

Refs NousResearch#82656

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(gateway,relay): prime relay egress routing for synthetic injections + cap stale completion replay

Defect #4 from the 2026-08-09 staging incidents (upgrade-robustness):
after every gateway restart the durable async-delegation replay injected
completions correctly (post-741663cf1) but their replies bounced at the
connector — 'slack egress declined: target not routed to an onboarded
tenant'. The relay adapter re-attaches tenant discriminators
(metadata.scope_id / metadata.user_id) from per-chat caches warmed ONLY by
inbound traffic; synthetic turns race those cold caches on every deploy,
scale-to-zero wake, and crash recovery.

- relay adapter: prime_routing_cache() — feeds a synthetic event's
  session-store origin through the same _capture_scope used for real
  inbound (never raises).
- run.py injection path: prime the resolved adapter before handle_message
  (duck-typed; native adapters unaffected).
- async_delegation: 48h staleness cap in restore_undelivered_completions —
  a pending completion older than the cap is terminally dropped (payload
  stays queryable) instead of re-run as a fresh full-context turn; the
  post-restart replay of a July session burned a 102K-token context.

Also carried: JoaoMarcos44's suppression behaviour-matrix harness
(cherry-picked from NousResearch#82676, authorship preserved) — 39 passed + 1 xfail
(the documented ACK-then-drop transport-honesty residue).

* test: use recent timestamps in restored-ownership fixtures

test_restore_stamps_restored_flag persisted its completion with epoch-era
toy timestamps (dispatched_at=1.0), which the new 48h replay staleness cap
correctly classifies as stale — the fixture then exercised the cap instead
of the restored-flag contract (CI slice 4 failure). Timestamps are now
now-relative; the staleness behavior itself is pinned separately in
test_relay_injection_egress_priming.py.

* fix(gateway,relay): close four review findings on the relay delivery fixes

Review follow-ups on this branch (NousResearch#82592):

1. HIGH — classifier/resolver mismatch (falsely-acknowledged loss).
   _classify_completion_target now returns "deliver" for idle-ended
   parents, but _resolve_async_delegation_session still dropped every
   non-compression-ended pin: the durable row was acked at adapter
   acceptance, then the injection died inside the pipeline with no
   retry — strictly worse than the honest terminal drop on main, and
   the delivery leg defect #2's fix depends on did not exist. The
   resolver now retargets non-user-boundary ends (idle/timeout/
   lifecycle) to the chat's current session — session_entry already IS
   the routing key's current session for the same chat — while user
   boundaries (session_reset / new_session / user_exit /
   session_switch) stay fail-closed. Both sides share one module-level
   _USER_BOUNDARY_END_REASONS so the verdict and the routing decision
   cannot drift again; a coherence test asserts deliver-verdicts
   resolve non-None across representative end reasons.

2. HIGH — drain clamp missed adapter-level spend. The effective drain
   grace budgeted drain + 3x teardown, but RelayAdapter.disconnect
   spends revocation-monitor teardown + go_idle time BEFORE the
   transport drain inside the same runner wait_for; worst case still
   blew the budget and cancelled teardown mid-drain (skipping the
   fail-pending loop). The adapter now measures its own elapsed time
   and threads the REMAINING budget into
   transport.disconnect(budget_s=...); legacy/stub transports without
   the keyword fall back to the no-arg signature.

3. P1 — _request_response racing disconnect() could register a future
   after the fail-pending loop already ran, stranding the caller for
   the full _OUTBOUND_TIMEOUT_S (30s). Fail fast with the same
   "relay transport closed" error once _closing is set.

4. P1 — _build_process_event_source's last-resort reconstruction
   dropped scope_id, so a scoped relay completion whose session-store
   origin was unavailable primed no tenant discriminator and could
   still bounce off the connector's fail-closed egress guard.
   scope_id now threads through the reconstructed SessionSource, with
   a warning when a scoped chat reconstructs without one.

All four: RED reproduced with the fix reverted, GREEN after; relay/
delegation delivery families pass (43 + 71 + 179 across the touched
suites); full tests/gateway run shows only failures already failing
identically on merge base 2446c8b (env/dep issues).

* fix(gateway,relay): make pending-frame failure cancellation-safe; persist completion routing origin

Two remaining review findings on this branch (NousResearch#82592):

1. Cancellation could strand outbound waiters past the fail-pending
   loop. transport.disconnect() failed pending futures only at the END
   of the drain + three teardown awaits; a cancellation landing
   mid-drain (the runner's wait_for budget, an outer cleanup deadline)
   skipped the loop entirely and left registered futures unresolved —
   their callers blocked until _OUTBOUND_TIMEOUT_S (30s). The budget
   threading added earlier shrinks the window but is not a hard
   guarantee. The fail-pending loop (and the going_idle ack failure)
   now run in a `finally`, so no exit path — normal, error, or
   cancelled — can leave a registered future unresolved. Idempotent:
   done futures are skipped, a second disconnect() pass is a no-op.

2. Durable completions did not persist their routing origin, so the
   scope_id threading in the fallback SessionSource reconstruction had
   nothing to carry on the exact path it exists for (restart replay
   with session store + source cache gone): the async-delegation event
   producers never populated scope_id and the durable rows never
   stored it. Dispatch now snapshots the originating turn's
   scope_id/user_id/user_name from the session context
   (_capture_routing_origin — a new HERMES_SESSION_SCOPE_ID contextvar
   bound by the gateway at session-bind time alongside the existing
   vars), stores them in the existing task_json payload (no schema
   migration), and re-attaches them to all three completion-event
   shapes (live single, live batch, crash-recovery rebuild). The
   gateway's fallback reconstruction then primes both discriminators
   after a restart.

Tests: cancellation mid-drain -> every pending future resolves with
"relay transport closed" (mutation: moving the loop out of the finally
goes RED); second-pass disconnect idempotence; end-to-end
dispatch -> owner-death recovery -> event carries scope_id -> fallback
SessionSource primes it (mutations: dropping the dispatch capture or
the task_json persistence both go RED); live completion event carries
the origin. 94 passed + 1 xfailed across the delivery/delegation
suites; tests/tools delegation family 73 passed (2 collection errors
pre-existing on merge base 2446c8b).

---------

Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Ben Barclay <ben@nousresearch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signal/Gateway: Approval responses are misrouted as steered mid-turn messages instead of being delivered to the approval handler

1 participant