fix(photon): persist sidecar token for standalone sends - #433
Conversation
|
Review Complete Files Reviewed: 5 By Severity:
This PR introduces 2 critical prompt-cache and process-shutdown regressions in gateway/run.py, removes 4 authorization/security gating checks, and has 8 additional high-medium severity issues including adapter resolution bypass and context leak guard removal. Files Reviewed (5 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (78/100) — 2 critical findings, 5 high, 4 medium · 949 LOC across 5 files
Critical Issues
Two critical regressions in gateway/run.py:
- Prompt-cache re-baseline timing regression (line 10610) — the agent is rebuilt on every message turn instead of reusing the cached prefix, violating the sacred "per-conversation prompt caching" principle in AGENTS.md. Every turn now incurs the full cost of rebuilding the system prompt.
- SystemExit bypass (line 19064) —
start_gateway()callssys.exit()instead of routing through theos._exitbackstop path, enabling thread-join hangs on shutdown.
High Severity Issues
- Authorization check removed before auto-resume (line 5948) — sessions resume on restart without re-verifying authorization, allowing allowlist-bypass continuation.
- Adapter resolution bypass (line 4665) —
self.adapters.get(platform)replaces_adapter_for_source()in busy-message/queue paths, bypassing profile-scoped adapter lookup. - Open-policy startup enforcement removed (line 6195) — gateway now starts with unsafe
policy: openconfigurations without theallow_all_opt_ingate. - Session context leak guard removed (line 8038) — cross-session ContextVar leak protection was deleted from handler entry.
- Adapter authorization check registration removed (line 6357) —
set_authorization_check()callbacks are no longer wired, disabling indirect prompt injection protection. - Queued follow-up re-baseline removed (line 18108) — queued messages always rebuild the agent instead of reusing cache.
- Restart-loop circuit breaker removed (line 5925) — crash-resume restart cycles can now loop indefinitely.
Medium Severity Issues
- xapp- secret pattern removed (line 148) — Slack app tokens no longer redacted from gateway logs.
- Profile parameter dropped (line 8116) — unauthorized DM behavior lookup loses profile context.
- Hardcoded --user scope in systemd shortcut (line 5721) — breaks system-unit deployments.
- Systemd exit code regression (line 7654) — may cause indefinite backoff with
Restart=on-failure. - Stale-adapter fatal-error overwrite (line 3648) — notification races before stale check.
Two new tests were added for the photon sidecar lifecycle and the telegram noise filter.
| show = subprocess.run( | ||
| [ | ||
| systemctl, | ||
| "--user", | ||
| "show", | ||
| service_name, | ||
| "--property=MainPID", | ||
| "--value", | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=2, | ||
| ) | ||
| if (show.stdout or "").strip() != str(current_pid): | ||
| return |
There was a problem hiding this comment.
🟡 Hardcoded --user scope in systemd restart shortcut breaks system-unit deployments (bug)
The _launch_systemd_restart_shortcut() method was refactored to remove dual-scope detection (system vs. user units). Old code checked both systemctl show (system scope) and systemctl --user show (user scope) and chose the appropriate one. New code (lines 5720-5748) hardcodes --user only. The MainPID comparison at line 5734 will fail for system-unit deployments, causing the function to return early without launching the restart helper. This breaks gateway restart on system-managed Hermes installations (e.g., production daemon deployments using system units). The old code's comment explicitly warned: 'Hard-coding --user broke system-unit deployments: systemctl returned an empty MainPID, the PID-equality check below failed, and the planned-restart helper was never launched — leaving the gateway dead until a manual reboot.'
💡 Suggestion: Restore dual-scope detection: query both systemctl show and systemctl --user show, use whichever matches current_pid, and apply the matching scope to both the reset-failed and restart commands.
📋 Prompt for AI Agents
In gateway/run.py around lines 5720-5748, restore the dual-scope systemd detection. Query both systemctl (system scope) and systemctl --user (user scope) for MainPID, match against os.getpid(), and use the matching scope for the systemd-run helper invocation.
| await self._refresh_agent_cache_message_count( | ||
| session_key, session_entry.session_id | ||
| ) |
There was a problem hiding this comment.
🔴 Re-baseline timing regression destroys prompt cache every turn — agent rebuilt on every message (bug)
The _refresh_agent_cache_message_count call was moved from its deferred position (after session_meta transcript row append and update_session) to an early position before session_meta (line 10610). The session_meta row appended later (line 10993) increments message_count by 1. On every subsequent turn, the cached message_count (captured before session_meta) is exactly 1 less than the live DB count (which includes session_meta), so the cross-process coherence guard at line 16537-16541 always sees a mismatch and evicts the cached agent. This destroys the prompt cache prefix on every turn for every gateway session — a direct violation of Hermes's 'per-conversation prompt caching is sacred' invariant. Every user message multiplies API cost by forcing a full context re-upload instead of reusing the cached prefix.
💡 Suggestion: Move the _refresh_agent_cache_message_count call back to its deferred position after the session_meta row append and update_session call (after line 10996), as the old code did. The old deferred position was explicitly chosen to include all turn transcript writes.
📋 Prompt for AI Agents
In gateway/run.py, move the await self._refresh_agent_cache_message_count(session_key, session_entry.session_id) call from its current position (lines 10610-10612, before session_meta) to after the self.session_store.update_session(...) call at line 10996. This ensures the cached message_count includes the session_meta row that the gateway adds after the agent returns, so the cross-process guard compares like-for-like counts and the agent is reused rather than rebuilt every turn.
| success = asyncio.run(start_gateway(config)) | ||
| _exit_after_graceful_shutdown(success) |
There was a problem hiding this comment.
🔴 SystemExit from start_gateway() bypasses os._exit backstop, enabling thread-join hang on shutdown (bug)
main() at line 19064 was refactored to remove the try/except SystemExit block. The new code calls success = asyncio.run(start_gateway(config)) followed by _exit_after_graceful_shutdown(success) assuming start_gateway() always returns normally. However, start_gateway() still raises SystemExit for four paths: clean exit with explicit code (line 18923), startup abort with code (line 18939), post-shutdown with code (line 19007), and service restart with code 75 (line 19031). When SystemExit propagates past line 19064, _exit_after_graceful_shutdown is never reached, os._exit is never called, and Python runs normal interpreter finalization (Py_FinalizeEx) which joins ALL non-daemon threads — including any wedged ThreadPoolExecutor workers. The old try/except SystemExit wrapper explicitly routed every exit path through os._exit to prevent exactly this hang (NousResearch#53107).
💡 Suggestion: Restore try/except SystemExit in main() to catch SystemExit from asyncio.run(start_gateway(config)) and call _exit_after_graceful_shutdown with the extracted exit code.
📋 Prompt for AI Agents
In gateway/run.py main() at line 19064, wrap the asyncio.run call in a try/except SystemExit block:
try:
success = asyncio.run(start_gateway(config))
exit_code = 0 if success else 1
except SystemExit as e:
if e.code is None:
exit_code = 0
elif isinstance(e.code, int):
exit_code = e.code
else:
exit_code = 1
_exit_after_graceful_shutdown(exit_code)
This requires _exit_after_graceful_shutdown to accept an int instead of a bool, or add a separate path.
| self._exit_code = ( | ||
| GATEWAY_SERVICE_RESTART_EXIT_CODE | ||
| if sys.platform == "darwin" or not os.environ.get("INVOCATION_ID") | ||
| else 0 | ||
| ) |
There was a problem hiding this comment.
🟡 Systemd restart exit code regression for Restart=on-failure deployments (bug)
The old code unconditionally set self._exit_code = GATEWAY_SERVICE_RESTART_EXIT_CODE (75) for planned restarts via systemd. This worked with Restart=on-failure + RestartForceExitStatus=75 to restart reliably. The new code (lines 7646-7658) exits 0 on Linux systemd (has INVOCATION_ID), assuming Restart=always. Users whose systemd unit files use Restart=on-failure (a common configuration) will find their gateway stays dead after a planned restart because exit 0 is not treated as a failure. The old code was more defensive and worked with both restart policies.
💡 Suggestion: Either restore the unconditional TEMPFAIL exit code, or detect the systemd unit's Restart= policy at startup and choose the exit code accordingly.
📋 Prompt for AI Agents
In gateway/run.py around lines 7646-7658, reconsider the exit code logic. The old code always used GATEWAY_SERVICE_RESTART_EXIT_CODE (75) which worked with both Restart=always and Restart=on-failure+RestartForceExitStatus=75. Either restore unconditional TEMPFAIL or verify the unit's Restart= policy before choosing the exit code.
|
|
||
| def _queue_or_replace_pending_event(self, session_key: str, event: MessageEvent) -> None: | ||
| adapter = self._adapter_for_source(event.source) | ||
| adapter = self.adapters.get(event.source.platform) |
There was a problem hiding this comment.
🟠 Adapter resolution bypasses profile-scoped lookup in busy-message and queue paths (bug)
The _adapter_for_source() method (authz_mixin.py:57-66) resolves adapters via _authorization_adapter(platform, profile) which properly handles profile-scoped adapters when source.profile is set. The PR replaced calls at lines 4665, 4722, 4824 (queue/draining/busy-message paths) with self.adapters.get(event.source.platform) which ignores the source's profile. In multi-profile gateway setups where the same platform is configured in multiple profiles, these code paths will resolve to the default-profile adapter instead of the source's profile-scoped adapter, potentially sending messages through the wrong adapter instance.
💡 Suggestion: Restore adapter resolution via _adapter_for_source() or replicate its profile-aware logic at these call sites. Call self._authorization_adapter(event.source.platform, getattr(event.source, 'profile', None)) instead.
📋 Prompt for AI Agents
In gateway/run.py at lines 4665, 4722, 4824, replace self.adapters.get(event.source.platform) with self._adapter_for_source(event.source) or self._authorization_adapter(event.source.platform, getattr(event.source, 'profile', None)). Also check the pairing-response path at line 8127 for the same issue.
| @@ -8377,23 +8037,6 @@ async def _handle_message(self, event: MessageEvent) -> Optional[str]: | |||
| """ | |||
| source = event.source | |||
There was a problem hiding this comment.
🟠 Session context leak guard removed from message handler entry (bug)
The reset_session_vars() call at the top of _handle_message() was an explicit cross-session ContextVar leak guard. When asyncio tasks handle concurrent messages, a new task inherits the spawning task's ContextVars via copy_context(). If message A had bound HERMES_SESSION_* vars when message B's task was created, B would inherit A's session identity until it binds its own. The guard reset to _UNSET so the window was safe (no foreign session). Removing this guard means session vars can leak between concurrent messages, potentially causing one user's subprocess to read another user's session identity in multi-user gateway setups.
💡 Suggestion: Restore the reset_session_vars() call at the top of _handle_message(), after the source extraction line.
📋 Prompt for AI Agents
In gateway/run.py, at the start of _handle_message() (after line 8038), restore the try/except block that imports and calls reset_session_vars() from gateway.session_context.
| @@ -6625,7 +6355,6 @@ async def start(self) -> bool: | |||
| adapter.set_session_store(self.session_store) | |||
| adapter.set_busy_session_handler(self._handle_active_session_busy_message) | |||
| adapter.set_topic_recovery_fn(self._recover_telegram_topic_thread_id) | |||
There was a problem hiding this comment.
🟠 Adapter authorization check registration removed — indirect prompt injection protection disabled (security)
The set_authorization_check() method on platform adapters registers a callback used by _is_sender_authorized() to determine if a message sender is on the allowlist. The PR removed all three calls to adapter.set_authorization_check(self._make_adapter_auth_check(adapter.platform)) (in the main startup loop at line 6357, in the reconnect watcher, and in _start_one_profile_adapters). It also deleted the _make_adapter_auth_check() factory method. Without the callback, _is_sender_authorized() returns None for all senders (base.py:2842), which callers treat as 'unverified — no check configured.' Slack and Discord adapters that fetch external thread context (conversations.replies, message context) will no longer mark unauthorized senders as [unverified] in LLM context, enabling indirect prompt injection from non-allowlisted users in shared channels.
💡 Suggestion: Restore the set_authorization_check() calls at all three adapter setup locations and restore the _make_adapter_auth_check() method.
📋 Prompt for AI Agents
In gateway/run.py, restore calls to adapter.set_authorization_check() in the three adapter setup locations: the main start() loop (around line 6357), _platform_reconnect_watcher(), and _start_one_profile_adapters(). Also restore the _make_adapter_auth_check() method that creates the platform-bound auth callback delegating to _is_user_authorized.
| # what the follow-up's guard will consult. Fail-safe in helper. | ||
| await self._refresh_agent_cache_message_count(session_key, session_id) | ||
|
|
||
| followup_result = await self._run_agent( |
There was a problem hiding this comment.
🟠 Removed re-baseline before queued follow-up — queued messages always rebuild agent (bug)
The _refresh_agent_cache_message_count call that preceded the recursive _run_agent call in the queued-follow-up path (around line 18108) was removed. The follow-up runs within the same _handle_message_with_agent invocation, before the outer re-baseline at line 10610 even executes. Without the in-band re-baseline, the follow-up's agent initialization sees a cached message_count from agent-build time (before the first turn's writes) against the live DB count (after the first turn's writes), triggering the cross-process guard and rebuilding the agent. This breaks prompt caching for every queued follow-up message (/queue mode, interrupt→queue demotions, subagent-demoted messages).
💡 Suggestion: Restore the _refresh_agent_cache_message_count(session_key, session_id) call immediately before the followup_result = await self._run_agent(...) call at line 18108.
📋 Prompt for AI Agents
In gateway/run.py, add back await self._refresh_agent_cache_message_count(session_key, session_id) immediately before line 18108 (followup_result = await self._run_agent(...)). This re-baselines the agent cache's message_count to include the first turn's own row writes before the recursive agent invocation.
| ) | ||
| == "pair" | ||
| ): | ||
| if source.chat_type == "dm" and self._get_unauthorized_dm_behavior(source.platform) == "pair": |
There was a problem hiding this comment.
🟡 Profile parameter dropped from unauthorized DM behavior lookup (bug)
The old code called self._get_unauthorized_dm_behavior(source.platform, profile=source.profile) to look up unauthorized DM behavior with per-profile customization. The new code (line 8116) calls self._get_unauthorized_dm_behavior(source.platform) without the profile parameter. The method implementation at authz_mixin.py:637 passes the profile to _adapter_dm_policy(platform, profile=profile) to resolve the profile-scoped live adapter's DM policy in multiplex mode. When profile defaults to None, the method falls back to the default profile's config instead of using the source's profile-specific settings. This means multi-profile gateways will use the wrong DM policy for unauthorized users on profile-scoped platforms.
💡 Suggestion: Restore the profile parameter: call self._get_unauthorized_dm_behavior(source.platform, profile=source.profile).
| if source.chat_type == "dm" and self._get_unauthorized_dm_behavior(source.platform) == "pair": | |
| if source.chat_type == "dm" and self._get_unauthorized_dm_behavior(source.platform, profile=source.profile) == "pair": |
📋 Prompt for AI Agents
In gateway/run.py at line 8116, add back , profile=source.profile to the self._get_unauthorized_dm_behavior(source.platform) call.
| @@ -3784,15 +3668,13 @@ async def _handle_adapter_fatal_error(self, adapter: BasePlatformAdapter) -> Non | |||
| error_message=adapter.fatal_error_message, | |||
| ) | |||
|
|
|||
| existing = self.adapters.get(adapter.platform) | |||
| if existing is adapter: | |||
| # Claim this adapter for teardown before awaiting disconnect() — | |||
| # a second fatal-error notification for the same adapter (e.g. | |||
| # from a concurrent recovery path) would otherwise still see | |||
| # itself as "existing" during the await below and disconnect() | |||
| # the same object twice. | |||
| self.adapters.pop(adapter.platform, None) | |||
| self.delivery_router.adapters = self.adapters | |||
| await adapter.disconnect() | |||
| try: | |||
| await adapter.disconnect() | |||
| finally: | |||
| self.adapters.pop(adapter.platform, None) | |||
| self.delivery_router.adapters = self.adapters | |||
There was a problem hiding this comment.
🟡 Stale-adapter fatal-error notification overwrites platform status before stale check (bug)
In _handle_adapter_fatal_error() (lines 3642-3677), the stale-adapter identity check (existing is adapter) was moved from before the error logging/platform-status update to after. Old order: (1) check stale → bail early, (2) log error, (3) update status, (4) disconnect. New order: (1) log error, (2) update platform status to 'fatal'/'retrying', (3) check if adapter is stale, (4) disconnect if not stale. When a delayed fatal-error notification from adapter A arrives after a successful reconnect installed adapter B, the new code still logs the error and overwrites the platform runtime status to 'fatal'/'retrying', marking a healthy reconnected platform as dead. The stale check at step 3 correctly prevents double-disconnect of B, but the status corruption at step 2 already happened.
💡 Suggestion: Restore the stale-adapter check (existing is not None and existing is not adapter → return early) to BEFORE the error logging and platform-status update.
📋 Prompt for AI Agents
In gateway/run.py, in _handle_adapter_fatal_error, move the stale-adapter guard to the top of the method:
existing = self.adapters.get(adapter.platform)
if existing is not None and existing is not adapter:
logger.debug("Ignoring stale fatal error from a superseded %s adapter instance: %s", adapter.platform.value, adapter.fatal_error_code or "unknown")
return
Then log the error and update status only if the adapter is current.
1 participant
Summary
hermes photon ...resolves before plugin CLI iterationTests
python -m pytest tests/plugins/platforms/photon tests/hermes_cli/test_startup_plugin_gating.py tests/gateway/test_telegram_noise_filter.py -q(359 passed)Mirror-of: NousResearch#56514
NousResearch#56514