Skip to content

fix(gateway): rewind MarkdownV2-escaped pipe tables in cron Telegram deliveries - #53661

Open
Kewe63 wants to merge 8 commits into
NousResearch:mainfrom
Kewe63:fix/53175-v7-final
Open

fix(gateway): rewind MarkdownV2-escaped pipe tables in cron Telegram deliveries#53661
Kewe63 wants to merge 8 commits into
NousResearch:mainfrom
Kewe63:fix/53175-v7-final

Conversation

@Kewe63

@Kewe63 Kewe63 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the broken-Telegram-table symptom from cron jobs that instruct the agent to emit MarkdownV2 tables (#53632). The model emits pipe-table rows already-escaped (\| instead of |), and Telegram's MarkdownV2 renderer prints the backslashes verbatim — the recipient sees literal \| instead of a native Telegram table render.

This PR adds a scoped unescape guard in DeliveryRouter._deliver_to_platform that detects cron-routed Telegram deliveries (metadata.job_id) carrying a MarkdownV2-escaped pipe-table, and rewinds \|| inside those table rows. The downstream adapter's normal MarkdownV2 escape re-applies once and Telegram renders the table natively.


Problem

Reporter (daily-news cron delivered to Telegram) saw tables render as escaped-pipe rows:

📅 2-Week Forward Calendar
\| Date \| Event \| Category \| Expected Impact \|
\|:-----\|:------\|:---------\|:----------------\|
\| 2026-07-15 \| Fed Beige Book \| Macro \| Regional growth \|

instead of a Telegram-rendered table. The full copy in chat history / session view showed the table correctly — only the final Telegram send was broken.

Root cause: when the model interprets a prompt as "use Telegram MarkdownV2 syntax", it pre-escapes the pipe bars itself. The Telegram adapter's format_message() then re-applies MarkdownV2 escape on top, producing double-escaped output (\\|) which MarkdownV2 renders as the escaped literal \|.


Fix

Detect-then-rewind helpers in gateway/delivery.py:

_MARKDOWNV2_ESCAPED_TABLE_BAR = re.compile(r'\\\|')

def _looks_like_cron_markdownv2_table(content: Optional[str]) -> bool:
    """Conservative shape check: >= 2 pipe-table-shaped rows AND >= 6
    total escaped pipes (a real table has header + separator + body)."""

def _unescape_markdownv2_tables(content: str) -> str:
    """Rewind \\| back to | inside rows that match pipe-table shape,
    leaving prose, code fences, and arithmetic untouched."""

Gate in _deliver_to_platform:

is_cron_path = bool((metadata or {}).get("job_id"))
if (
    target.platform == Platform.TELEGRAM
    and is_cron_path
    and _looks_like_cron_markdownv2_table(content)
):
    content = _unescape_markdownv2_tables(content)

The gate sits between the silence-narration filter and the send_metadata build-up. Logging at DEBUG level records size delta and chat_id when a rewind fires.


Scope — Why It Doesn't Collide with Open Rich PRs

Three open PRs add sendRichMessage fast-paths to tools/send_message_tool.py (#46118, #46952, #47190). This PR is intentionally non-overlapping:

  • Touches gateway/delivery.py only — no _send_telegram edit
  • Scope: cron-only (metadata.job_id), Telegram-only
  • tools/send_message_tool.py is untouched

Both paths can coexist: cron-routed send_message goes via DeliveryRouter._deliver_to_platform and benefits from this rewind even if/when the _send_telegram rich PR lands.


Files Changed

File Change
gateway/delivery.py +130 (helpers + gate)
tests/gateway/test_delivery_cron_table_unescape.py +206 (new, 15 tests)

Tests

15 new cases in tests/gateway/test_delivery_cron_table_unescape.py, all passing:

Class Coverage
TestUnescapeMarkdownv2Tables canonical reporter case, prose untouched, short escaped strings untouched, arithmetic untouched (≥6-pipe cut-off), empty/None, idempotency
TestLooksLikeCronMarkdownv2Table real cron table detected, plain text not detected, arithmetic not detected, short 2-row tables not detected, empty/None, separator-only not detected
TestCronGateContract cron + Telegram + table is rewound; same payload without job_id is left alone; cron + non-Telegram is left alone

Checklist


Risk & Impact

Low. Gate is triple-gated (platform == TELEGRAM AND job_id present AND table shape detected). Prose, code fences, arithmetic, and non-cron deliveries are all left untouched. Idempotency confirmed by test.

Type: 🐛 Bug fix
Closes: #53632

Kewe63 added 7 commits June 27, 2026 13:25
…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.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter comp/cron Cron scheduler and job management sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P2 Medium — degraded but workaround exists labels Jun 27, 2026
…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.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the cron/Telegram symptom. The current formatter still misses pre-escaped table separators: gateway/platforms/helpers.py:287-292 requires literal pipes, while plugins/platforms/telegram/adapter.py:6537-6539 runs that converter before its generic MarkdownV2 escape at :6636-6637. The cron path reaches DeliveryRouter._deliver_to_platform() with job_id metadata (cron/scheduler.py:1675, :1694-1711), so the core premise remains valid.

Problems

  • gateway/delivery.py:106-141 intentionally does not isolate code fences, yet it rewrites every matching row. A fenced, table-shaped escaped payload is therefore changed despite the stated code-block safety claim.
  • tests/gateway/test_delivery_cron_table_unescape.py:155-165 mirrors the production predicate locally rather than exercising DeliveryRouter and the Telegram formatter; it would still pass if the real gate drifted or disappeared.
  • The submitted diff at 07f1470e23312122684d1ad74739e5aa20c0bf96 also carries unrelated gateway cleanup, Docker CI, artifact, and process-test changes. The linked #54302 discussion likewise identifies the need for branch isolation.

Suggested changes

  • Salvage the delivery fix alone, protect fenced regions, and add an end-to-end router-to-adapter regression for rich and legacy formatting.

Automated hermes-sweeper review.

Comment thread gateway/delivery.py
re-applies the ``\|`` once (the documented behavior) and Telegram
renders the table natively.

Code-block isolation is intentionally NOT done here: the only caller

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This explicitly skips fence isolation, but the subsequent row-level replacement mutates matching \| content inside a fenced code block. Please preserve fenced regions and add a negative regression; the adapter does not extract fences until after this DeliveryRouter transform.

"""Simulate the cron+TELEGRAM+table gate without spinning up the router."""

@staticmethod
def gate(content, *, target_platform, metadata):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This mirrors the production conditional rather than invoking DeliveryRouter._deliver_to_platform, so it cannot catch removal or drift of the actual gate or verify the adapter receives the corrected content. Please replace it with an async router integration test using a recording Telegram adapter.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants