fix(gateway): never bind a routing key to a delegate subagent session (#92859)

[karanmish92-stack]

Related issue

Fixes #92859.

Summary

A delegate_task batch dispatched from a Discord DM ended with the DM's routing key bound to one of the spawned children. That bind went through SessionStore.switch_session(), which promotes the outgoing session to the un-resurrectable session_switch boundary — so the human parent was killed while its own children were still running, every in-flight delegation from that parent was then classified terminal and dropped, and the user's next DM was routed into a leaf subagent that knew nothing about the conversation.

Reproduced three times on the reporter's machine (twice after the issue was filed). 4/4 batches whose origin_session was a gateway routing key were dropped; the only batch that ever reached the user had a plain session id as its origin.

Root cause and where the guard goes

The report's own suggested invariant — "a session with a non-empty parent_session_id should never be bindable to a gateway routing key" — is right in spirit but wrong in predicate: compression continuations also carry parent_session_id and legitimately continue the route. The stable signal is delegate provenance, which delegate_tool already writes durably at creation (source='subagent' + model_config._delegate_from, tools/delegate_tool.py:2024).

The guard is placed at the sink, not at a caller. switch_session() is the single funnel every route rebind passes through — async-completion pinning (gateway/run.py:16357), /resume, CLI handoff (:13777), Telegram topic rebinding (:19127) — so refusing there closes the class rather than the one observed call path, and no future call site can reintroduce the hijack.

Provenance rather than the parent edge, deliberately:

• a grandchild's parent_session_id points at another child, not at the routed session;
• a row that has already been hijacked once has had its source overwritten with the platform name by record_gateway_session_peer — that is the literal shape of 20260823_041917_519cdb in the report (source='discord', but _delegate_from still set). Either signal alone is therefore sufficient.

Restart recovery gets the same fence

find_latest_gateway_session_for_peer() ranked the hijacked child above the real conversation on recency, so a gateway restart would have handed the chat straight back to the subagent even after the bind path was fixed. Both its keyed query and its peer-tuple fallback now exclude subagent rows.

create_session already refused to let delegate children inherit a routing key at creation (hermes_state.py:5218 — "must NOT inherit routing keys, or peer recovery could repoint gateway traffic into a subagent's session"). This PR extends that existing, deliberate rule to the two paths that could grant one afterwards.

Relationship to the other open PRs

• fix(gateway): keep delegate completions on parent session #92620 (_delegate_from provenance walk in _resolve_async_delegation_session) — same family, complementary rather than overlapping: it canonicalizes the completion resolver's pinned id; this PR refuses the bind itself and also fences restart recovery. They compose cleanly — with this PR the resolver's switch_session call can no longer land on a child even if the walk is bypassed.
• fix(gateway): prevent delegated children from stealing parent routes #92872 (closed) — guarded on pinned_row.parent_session_id == prior_session_id, which its own blocking review correctly noted is not a stable invariant (compression can advance the route off that exact row). This PR does not use the parent edge at all.

Testing

tests/gateway/test_subagent_route_bind_guard.py — 15 tests against real SessionDB rows, not mocks:

• the exact reported sequence: parent DM + live child + rebind attempt → refused, and the parent is not ended (the assertion that pins the delivery-drop consequence, not just the routing symptom);
• the two harder shapes a parent-edge predicate misses (nested grandchild, already-hijacked row);
• /resume to a real prior conversation still switches — the feature switch_session exists for;
• restart recovery prefers the real conversation over a hijacked child, on both the keyed query and the peer-tuple fallback;
• is_internal_subagent_row detection matrix, including a /branch child (a real, user-addressable conversation) staying out of scope.

scripts/run_tests.sh tests/gateway/test_subagent_route_bind_guard.py -q
→ 15 tests passed, 0 failed

Sabotage-verified. Disabling the switch_session guard and removing the SQL fence:

→ 4 failed, 11 passed

The 4 failures are exactly the behavioral tests; the 11 detection/regression tests stay green, so each test is bound to the code it claims to pin.

Wider suite:

scripts/run_tests.sh tests/gateway/ tests/state/ -q
→ 695 files, 6207 passed, 6 failed, 40 skipped

The 6 failures reproduce identically on unmodified origin/main (test_scale_to_zero, test_shutdown_forensics, test_systemd_notify, test_wecom_callback — macOS-local baseline, untouched by this diff).

Risk

Refusal is fail-closed and logged at WARNING with the routing key and target id, so the failure mode becomes greppable instead of a silent DM hijack. For a normal session there is no behavior change: a real gateway row has neither source='subagent' nor _delegate_from, and the /resume regression test pins that.

[karanmish92-stack]

Following up on the blocking review on #92872, since it names the acceptance bar for this path and I would rather test against it than assert past it.

Both named shapes were already refused here, and are now pinned. Pushed 8fa49c3 adding TestGuardSurvivesSessionGenerationChanges:

• test_child_cannot_take_the_route_after_its_parent_compressed — P spawns C, P compresses, route legitimately advances to tip P2 (fix(gateway): follow async delegation completions across compression #69312), completion stamped C arrives. C.parent_session_id == P, route is on P2.
• test_grandchild_after_compression_is_also_refused — both objections composed: P → C → G nested provenance and a rotated route.

Neither test needed a code change, because this guard never reads the parent edge — it reads the target's own durable provenance (source='subagent' OR model_config._delegate_from). Route generation is therefore irrelevant to it: the child is refused whether the route sits on P, P2, or anything else.

Discrimination check. "Already correct" is not regression coverage, so I verified these tests actually bite by replacing is_internal_subagent_row() with the #92872 predicate — target_row.parent_session_id == <currently routed id>:

→ 3 failed, 14 passed
FAILED test_child_cannot_take_the_route_after_its_parent_compressed
FAILED test_grandchild_after_compression_is_also_refused
FAILED test_refusal_holds_for_nested_and_already_hijacked_rows

Exactly the three shapes the review predicted an edge-keyed guard would miss, and nothing else. The tests are bound to the predicate, not to the surrounding logic.

On review item 4 (existing rebind + verified compression-continuation cases stay green) — test_real_gateway_session_still_switches covers the unrelated-live-session rebind directly, and the compression path is exercised unchanged: tests/gateway/test_session.py (65), test_async_delegation_session_binding.py (6), test_resume_command.py (27), test_completion_session_boundary.py (10) all pass, 125 total with this file.

On not landing two competing fixes — agreed, and I don't think this competes with #92620. They sit at different layers and compose:

• fix(gateway): keep delegate completions on parent session #92620 canonicalizes the pinned id inside _resolve_async_delegation_session before it decides where to deliver, walking _delegate_from transitively. That is the delivery-correctness half, and it is what [Bug]: delegate child process completion can replace and end the human-facing gateway session #92611 asks for.
• This PR refuses the bind itself at switch_session() — the shared sink for /resume, CLI handoff, and Telegram topic rebinding as well as the completion resolver — and fences find_latest_gateway_session_for_peer() so a restart cannot re-adopt a subagent row.

With both, the resolver resolves to the right owner and cannot mutate the route onto a child even if a future call path bypasses the walk. With only #92620, switch_session remains callable with a subagent target from four sites and restart recovery still ranks a hijacked child above the real conversation on recency. I'd be glad to see #92620 land first and rebase onto it; there is no textual conflict (different functions, different files).

Provenance over the parent edge, one further reason not in the review: a row that has already been hijacked has had its source overwritten with the platform name by record_gateway_session_peer. On the reporter's machine 20260823_041917_519cdb reads source='discord' today, with _delegate_from still set — so source alone would not re-detect it either. test_refusal_holds_for_nested_and_already_hijacked_rows pins that shape.

CI has not reported on this branch yet; I'm not claiming green. Local: 17 in the new file, 125 across the related gateway session/delegation files, and 6207 across tests/gateway/ tests/state/ with 6 failures that reproduce identically on unmodified main (macOS-local baseline, verified by stashing the diff).

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

This is a well-built fix: guarding the single sink (switch_session) rather than one call site, fencing both restart-recovery queries so a pre-fix hijack isn't re-adopted, provenance-keyed detection instead of the fragile parent-edge comparison, and real-SessionDB tests that include the compression-rotation and already-hijacked shapes. The discrimination argument in the thread is sound. Remaining items:

1. Definition drift between the Python and SQL detectors. is_internal_subagent_row (gateway/session.py:1238) treats _delegate_from: "" as NOT a subagent (.strip() empties it), while _NOT_SUBAGENT_ROW_SQL (hermes_state_common.py:192-196) uses json_extract(...) IS NULL, under which an empty string is not NULL — so the same row is bindable via switch_session but invisible to restart recovery. Nothing writes empty strings today, but the two halves of one invariant should agree exactly; NULLIF(json_extract(...), '') IS NULL closes the gap cheaply.

2. Caller behavior on refusal is untested. The guard returns None (gateway/session.py:~3536) and every caller previously assumed success. For async-completion pinning, dropping the completion is the desired outcome; for /resume, a user who pastes a subagent id explicitly now gets a silent no-op. Worth either a follow-up that surfaces a short user-facing message on that path, or at least a quick audit note that all four callers tolerate None.

3. The guard is fail-open on DB error (gateway/session.py:3521-3527): get_session raising leaves target_row=None and the bind proceeds. Given the alternative is breaking /resume when SQLite hiccups, that's the right trade — but consider logging at warning instead of debug there, since it's precisely the shape that silently reintroduces the incident.

Minor: subagent-ness now has two homes (Python helper + SQL fragment); the new tests pin both sides independently, which mostly covers the drift risk — item 1 is the residual case they can't catch.

[karanmish92-stack]

@Enough1122
Thanks — all three applied in 57173f1, plus one correction to the suggested fix for item 1.

1. Definition drift (fixed, with a wider predicate than suggested)

You proposed NULLIF(json_extract(...), '') IS NULL. That closes the empty-string case, but the Python side uses str(...).strip(), so it also treats whitespace-only (" ") as absent — which plain NULLIF still misses. Landed with TRIM as well so the halves agree on both shapes:

NULLIF(TRIM(COALESCE(json_extract(COALESCE({a}.model_config, '{}'), '$._delegate_from'), '')), '') IS NULL

Verified the new test actually regresses, rather than assuming it does. With only the SQL reverted to the pre-fix predicate and everything else in place:

FAILED TestDetectorHalvesAgree::test_blank_marker_treated_identically_by_both_halves[-False]
FAILED TestDetectorHalvesAgree::test_blank_marker_treated_identically_by_both_halves[   -False]
2 failed, 19 passed

assert python_says is is_subagent
assert sql_says is is_subagent
E       assert True is False

Restore the fix and it is 21 passed. TestDetectorHalvesAgree drives both halves over the same row and asserts they agree — the residual case you noted the per-half tests can't catch, since each only pinned its own side.

2. Caller behavior on refusal (audited + fixed the one path that needed it)

Audited all five switch_session call sites:

Call site	Behavior on None	Verdict
run.py:13777 CLI handoff	raises RuntimeError	correct — loud
run.py:16357 async-completion pinning	logs, drops the injection	correct — the desired outcome
run.py:19127 Telegram topic rebinding	if switched is not None	correct — keeps incumbent
slash_commands.py:5149 /branch	returns branch.switch_failed	correct
slash_commands.py:4901 /resume	returned generic switch_failed	misleading

So not a silent no-op — /resume already returned "Failed to switch session." — but that reads as a transient failure when nothing failed; the target simply isn't a conversation. It now pre-checks and returns a message naming the reason:

⚠️ /resume blocked: '{name}' is an internal subagent transcript, not a conversation. Delegate runs are execution records owned by the session that spawned them — resume that session instead.

The pre-check is guarded on self._session_db being present (it's Optional and guarded elsewhere in that file) and mirrors the guard's own fail-open posture on read error.

3. Fail-open logging (fixed)

Agreed on the trade — breaking /resume whenever SQLite hiccups is worse. Now logger.warning with the routing key included, since this is exactly the shape that silently reintroduces the incident:

logger.warning(
    "switch_session subagent pre-check failed for %s; allowing the bind "
    "(fail-open). A delegate row could be bound to routing key %s if this "
    "recurs (#92859).",
    target_session_id, session_key, exc_info=True,
)

Suite

tests/gateway: 5953 passed, 12 failed. tests/state + state-related: 1101 passed, 6 failed. Every one of those 18 failures reproduces with this commit stashed — pre-existing and environment-dependent (systemd socket, network, FTS corruption-recovery on this platform), none in the touched files. Happy to paste the baseline runs if useful.

On the "two homes" point: they now agree by construction on every shape I could find, and TestDetectorHalvesAgree is the executable statement of that. Consolidating to one detector would mean either running Python over every candidate row in recovery (a query becomes a scan) or pushing the whole predicate into SQL and calling it from Python — both worse than two small definitions with a test pinning their equivalence. Open to it if you'd rather, though.

[karanmish92-stack]

Rebased onto current main (94cd030) — the branch had gone CONFLICTING after ~1484 commits of drift. Now MERGEABLE.

One real conflict, in hermes_state.py. Upstream replaced the two hardcoded end-reason lists with the named constant _RECOVERABLE_END_REASONS_SQL; this branch added the subagent fence to the same two WHERE clauses. Both changes are wanted, so both are kept:

AND (s.ended_at IS NULL OR s.end_reason IN ({_RECOVERABLE_END_REASONS_SQL}))
AND {_NOT_SUBAGENT_ROW_SQL.format(a='s')}

Applied identically to both recovery queries — the session-key path and the peer-tuple fallback — so the keyed and keyless paths stay fenced. _RECOVERABLE_END_REASONS_SQL widens the recoverable set beyond the ('agent_close', 'ws_orphan_reap') this branch was written against; that is upstream's call and orthogonal to the fence, which filters on provenance, not end reason. The other four files auto-merged.

Verified after the rebase, not assumed:

• tests/gateway/test_subagent_route_bind_guard.py — 21 passed, including TestDetectorHalvesAgree.
• tests/gateway + tests/state + tests/test_hermes_state.py — 6929 passed, 21 failed.
• All 21 failures reproduce on a clean origin/main worktree with none of this branch's code (12 of them in the subset re-run above; the remainder are the same network/systemd-dependent Discord/Telegram cases). Pre-existing and environment-dependent — FTS corruption-recovery, systemd abstract sockets, network fetches — none in the touched files.

Both fences confirmed still present in hermes_state.py after the rebase (2 occurrences), and the NULLIF(TRIM(...)) alignment from the last commit is intact.