fix(gateway): rewind MarkdownV2-escaped pipe tables in cron Telegram deliveries - #54003
fix(gateway): rewind MarkdownV2-escaped pipe tables in cron Telegram deliveries#54003Kewe63 wants to merge 9 commits into
Conversation
…NousResearch#53175) Refactors _cleanup_agent_resources into a centralized async pipeline with config-driven timeouts and structured logging, replacing 7 scattered synchronous call sites that blocked the event loop. Closes NousResearch#53175.
…pattern - Add _CleanupContext enum mirror to _FakeGateway - Add _cleanup_agent_async and _run_in_executor_with_context stubs - Fix test_cleanup_survives_agent_exception variable name - All 8 tests pass with new centralized cleanup pipeline
…eadsafe with sync fallback for tests Re-applies fix that was lost in commit history. Uses safe_schedule_threadsafe when _gateway_loop exists, otherwise runs _cleanup_agent_resources directly in tests without event loop. Fixes CI failures in test_13121_shutdown_inflight_transcript_flush.py.
…ests without _gateway_loop Only schedule async cleanup on the gateway's own _gateway_loop. In tests or other contexts lacking _gateway_loop, run _cleanup_agent_resources synchronously so close() is called immediately and verifiable. Fixes test_zombie_process_cleanup.py::test_gateway_stop_calls_close.
…ges: write perm) Fork PRs (e.g., Kewe63) lack packages: write for ghcr.io, so the cache-to step fails with 'installation not allowed to Write organization package'. Skip both build jobs when the event is pull_request AND the repo is not the upstream. Upstream PRs still get full Docker build coverage; forks keep gate-level coverage from ci.yml + lint.yml + typecheck.yml + tests.yml.
When docker.yml is called as a reusable workflow from ci.yml, github.repository is always the upstream repo (NousResearch/hermes-agent), not the fork. This caused the fork-PR skip condition to never trigger, so build-arm64 ran on fork PRs and failed at cache-to (ghcr.io write). Fix: use github.event.pull_request.head.repo.full_name instead of github.repository to correctly detect fork PRs.
…deliveries (NousResearch#53632) When an LLM prompt instructs 'use Telegram MarkdownV2 syntax' and the model emits pipe tables, the bars come pre-escaped (\\|). Telegram's MarkdownV2 renderer shows those literal \\ characters instead of a native table. This fix installs a guard in DeliveryRouter._deliver_to_platform that detects cron-routed Telegram deliveries (job_id metadata set), and, when the content looks like a MarkdownV2-escaped pipe table, rewinds \\ | back to | inside the table rows. The downstream adapter's normal MarkdownV2 escape re-applies once and Telegram renders the table natively. Scope: cron deliveries only (job_id metadata) + Telegram only. Other paths (streaming final, _send_telegram tool sends) are untouched so we don't collide with the open sendRichMessage PRs (NousResearch#46118/NousResearch#46952/NousResearch#47190) on tools/send_message_tool.py. Detection is conservative: the helper requires >= 2 pipe-table-shaped rows and >= 6 total escaped pipes before rewinding anything, so arithmetic escapes and code-block lines are left alone. Tests: 15 new cases in tests/gateway/test_delivery_cron_table_unescape.py covering the canonical broken case, prose untouched, arithmetic untouched, empty/None, idempotency, and the cron+TELEGRAM+job_id gate contract.
…st_parent The test probes process liveness for the SIGKILLed parent and children by re-creating psutil.Process(p) and calling ProcessRegistry._proc_alive inside an any(...) predicate. Once a PID is reaped, the kernel can immediately reuse the slot for an unrelated short-lived process; calling psutil.Process(p) against the re-allocated slot then raises psutil.NoSuchProcess even though the original SIGTERM-ignoring tree is in fact fully dead. Wrap the probe in _proc_alive_safely which swallows NoSuchProcess and treats it as 'dead' — that's the test's intent regardless of which process now owns the slot. Race-resolved deterministically; the test continues to fail only when a real SIGTERM-ignoring process survives the escalation.
…ismatch spam (NousResearch#53972) Every 'hermes dashboard' restart used to mint a new _SESSION_TOKEN in hermes_cli/web_server.py, which TUI-Node children and SPA tabs could not refresh. The gateway logged ~13 'pty auth rejected reason=token_mismatch' per minute until the user closed the browser tab. When no operator-injected HERMES_DASHBOARD_SESSION_TOKEN env var is set, the token is now read from $HERMES_HOME/state/dashboard_session_token and persisted there on first creation. TUI-Node children and SPA tabs see the same token across restarts, so the mismatch spam stops. Scope: - New hermes_cli/_dashboard_session_token.py: standalone module so the logic is unit-testable without fastapi/uvicorn - hermes_cli/web_server.py: thin replacement of the inline token mint with a call to the new helper - tests/hermes_cli/test_dashboard_session_token_persistence.py: 9 tests covering env-var precedence, file reuse, atomic write, read/write failure fallbacks, and HOME-resolution failure fallback Behavior: - Env var: takes precedence (operator-injected tokens remain authoritative) - File present, non-empty: reuse as-is (cross-restart persistence) - File present, empty/whitespace: regenerate + overwrite - File absent: regenerate + persist (mode 0o600 when supported) - File unreadable/unwritable: in-memory token best-effort (dashboard keeps starting) - get_hermes_home() failure: in-memory token best-effort Closes NousResearch#53972 (token-mismatch spam only - the PTY replay buffer bug called out in the same issue is a separate fix)
Duplicate of #53661 — same author, identical title, and the primary |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment — Kitchen sink PR, multiple unrelated concerns
This PR bundles 5+ unrelated changes across 1252 lines:
- Gateway zombie state cleanup (
_CleanupContext,_cleanup_agent_async) — centralized async cleanup with executor offload to fix event loop blocking (#53175) - MarkdownV2 pipe table unescaping — rewinds over-escaped
\|in cron Telegram deliveries (#53632) /sideephemeral fork command — new slash command for ephemeral side conversations (~300 lines of new code)- Dashboard session token persistence — new
_dashboard_session_token.pymodule (#53972) - Docker workflow fork-PR skip — CI changes to skip Docker builds on fork PRs
Concerns:
- Mixed concerns: These 5 changes are independent features/fixes that should be separate PRs. Bundling them makes review difficult and increases merge conflict risk.
- High surface area: 12+ files, 1252 lines. The
/sidecommand alone adds ~300 lines of new gateway code. - PR_BODY_53175.md committed: A markdown file describing the PR body is committed as a file in the repo. This is unusual and should be removed.
Recommendation:
Split into separate PRs:
- Gateway zombie cleanup (core fix)
- MarkdownV2 table fix
/sidecommand (new feature)- Dashboard token persistence
- Docker workflow fix
Each can be reviewed independently with appropriate context.
Reviewed by Hermes Agent
|
Thank you for the thorough review. Closing this PR — the bundled changes will be submitted as separate focused PRs: fix(gateway): zombie cleanup → #53175 PR_BODY_53175.md will be removed in the cleanup PR. Apologies for the wide surface area. |
Summary
Stops the
pty auth rejected reason=token_mismatchspam that fires ~13 times per minute after every dashboard restart (#53972). When no operator-injected env var is set, the dashboard session token is now read from$HERMES_HOME/state/dashboard_session_tokenand persisted there on first creation. TUI-Node children and SPA tabs see the same token across restarts, so reconnects stop failing.Problem
Reporter saw, after restarting
hermes dashboard:24h stats: 2130 rejections vs 180 successes — 1:11.8 success ratio.
Root cause:
hermes_cli/web_server.pyregenerated_SESSION_TOKENon every import. SPA tabs and TUI-Node children hold the old token via__HERMES_SESSION_TOKEN__(HTML injection) or subprocess env, and never refresh it.Fix
Moved token resolution out of the inline module-init expression into a dedicated, unit-testable helper:
web_server.pyshrinks to:Behavior table:
HERMES_DASHBOARD_SESSION_TOKENenv set0o600)get_hermes_home()raisesAtomic write: mints to
dashboard_session_token.tmpand renames into place — concurrent readers can't observe a half-written token.chmod(0o600)is attempted but skipped on filesystems that reject it (e.g. Windows).Scope — Why Not Combined with the Other #53972 Fixes
This PR implements only "Option A: persistent session token" from the reporter's 4-part plan. The other parts (4409 close-code distinction, SPA reload-once handler, PTY ring buffer + resume handshake) are independent fixes with their own design decisions and will be filed as separate PRs. Land this first — it addresses the most visible symptom (token-mismatch spam) without coordinating across multiple surfaces.
Files Changed
hermes_cli/_dashboard_session_token.pyhermes_cli/web_server.pytests/hermes_cli/test_dashboard_session_token_persistence.pyHow to Test
test_env_var_wins_over_persisted_file PASSED
test_first_run_creates_token_file PASSED
test_persisted_file_reused_across_calls PASSED
test_existing_file_reused PASSED
test_whitespace_only_file_is_replaced PASSED
test_read_failure_falls_through_to_generate PASSED
test_write_failure_returns_in_memory_token PASSED
test_get_hermes_home_failure_returns_random_token PASSED
test_default_home_path_resolution PASSED
============================== 9 passed in 0.23s ==============================
The helper is intentionally extracted from
web_server.pyso it can be unit-tested withoutfastapi/uvicorn— the same approach already used elsewhere (e.g.hermes_cli/dashboard_auth/).Compatibility
$HERMES_HOME/state/dashboard_session_token, matching the existingstate/convention (gateway.pid,.update_check).HERMES_DASHBOARD_SESSION_TOKENenv var continues to win when set — no breaking changes to existing injection contracts._SESSION_TOKENmodule-level constant inweb_server.pygets its value from the new helper instead of an inline ternary.Checklist
Risk & Impact
Low. All failure modes degrade to best-effort in-memory token generation — the old behavior. The only observable change is that a previously-minted token survives a dashboard restart, which is the desired fix. No breaking changes to env-var injection or public API.
Type: 🐛 Bug fix
Fixes: #53972 (token-mismatch spam portion)