Skip to content

fix(heartbeat): frontend-agnostic outbound + comprehensive test coverage - #151

Merged
claudiusthebot merged 2 commits into
mainfrom
fix/heartbeat-frontend-agnostic
May 12, 2026
Merged

fix(heartbeat): frontend-agnostic outbound + comprehensive test coverage#151
claudiusthebot merged 2 commits into
mainfrom
fix/heartbeat-frontend-agnostic

Conversation

@claudiusthebot

@claudiusthebot claudiusthebot commented May 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

PR #150 introduced an outbound-messaging block in the heartbeat system prompt that hard-codes telegram-tools, "Telegram ID", and Telegram-only examples:

OUTBOUND MESSAGING: You also have access to telegram-tools β€” `send`,
`react`, ...
...
Known chat IDs live in your memory.md (see the Users section β€” Dylan's
Telegram ID is 352042062 for DM; ...

Talon supports multiple frontends (Telegram, Teams, Terminal, etc) and buildMcpServers already spawns one ${frontend}-tools MCP server per non-terminal frontend β€” including multiple at once. The hard-coded prompt misleads any heartbeat running on a non-Telegram deployment or a multi-frontend deployment.

The follow-up commit (test(heartbeat-outbound): comprehensive unit + integration coverage) expands test coverage across the whole heartbeat outbound surface β€” not just the new prompt builder, but the bridge, gateway, and helper layers PR #150 introduced.

What this PR does

Fix (fix(heartbeat): frontend-agnostic outbound-messaging prompt)

  1. New getActiveFrontends() in backend/claude-sdk/options.ts β€” small helper returning the list of non-terminal frontends. Same filter buildMcpServers was running inline; now de-duplicated. Exported via the barrel.

  2. New buildHeartbeatSystemPrompt() in core/heartbeat.ts (exported for tests) β€” builds the prompt dynamically:

    • Zero non-terminal frontends β†’ base prompt only, no OUTBOUND MESSAGING block, no chat_id mention.
    • One or more frontends β†’ lists every ${frontend}-tools server by name, uses the first frontend in the example send(...) call, refers to chat IDs as "per-frontend".
    • Falls back to the base prompt if getActiveFrontends() throws (test paths) β€” same try/catch pattern PR feat(heartbeat): outbound telegram β€” explicit chat_id routingΒ #150 used for buildMcpServers.

The hard-coded Telegram chat_id (352042062) is no longer in the prompt. It lives in memory.md anyway.

Test expansion (test(heartbeat-outbound): comprehensive unit + integration coverage)

Total: 29 new test cases across 4 files. 96 tests across the directly-touched surface, all passing.

claude-sdk-options.test.ts (+11 cases)

  • getActiveFrontends β€” 8 cases: scalar terminal/telegram/teams, array combinations, terminal filtering, multi-frontend order preservation, throw propagation.
  • buildMcpServers heartbeat-tier β€” 7 cases including the TALON_CHAT_ID="heartbeat" sentinel path, per-frontend env isolation (no cross-pollination between telegram-tools and teams-tools env), multi-frontend + brave-search combos.

heartbeat.test.ts (+7 cases, from the initial fix commit)

  • buildHeartbeatSystemPrompt shape β€” no frontends β†’ no OUTBOUND MESSAGING, telegram only β†’ references telegram-tools not teams-tools, teams only β†’ references teams-tools not telegram-tools, multi-frontend lists both, first frontend goes into the example send(...), no chat_id mention when no frontends configured, throws-path fallback.

bridge.test.ts (+6 cases)

  • Negative numeric chat_id (Telegram supergroup IDs are -100<id> β€” most common heartbeat target in practice; sign-handling bug would silently misroute).
  • chat_id=0 promoted correctly (gateway decides what to do with falsy).
  • chat_id=null forwarded as String(null)="null" (defensive β€” schema validates upstream).
  • Heartbeat sentinel default ("heartbeat") MUST be overridden when explicit chat_id supplied β€” otherwise outbound would silently fail to route.
  • Heartbeat sentinel passed through verbatim when no explicit chat_id (gateway's rawChatId !== "heartbeat" guard catches it).
  • String chat_id with leading zeros preserved (no Number() coercion).

gateway-http.test.ts (+5 cases)

  • Negative chat_id routes without active context (the supergroup case).
  • chat_id=0 rejected by !chatId falsy guard.
  • Heartbeat sentinel as explicit chat_id rejected β€” defends against routing-bypass if a heartbeat-tier MCP ever ships the sentinel literally.
  • Negative chat_id sign preserved through the handler (defensive against Math.abs / int32 truncation).
  • Coexistence: explicit chat_id matching an active context still routes via the explicit-routing branch (no exception, no double-dispatch).

Why these specific edge cases

  • Negative chat_id is the most subtle silent-bug surface. Every Telegram supergroup has a negative ID, so heartbeat β†’ group outbound depends on this working. A coercion bug here misroutes silently.
  • Heartbeat sentinel guard is the routing-safety net. If rawChatId === "heartbeat" ever stopped triggering the rejection branch, heartbeats could spam unrelated chats.
  • getActiveFrontends is the multi-frontend correctness backbone for the new prompt builder.
  • Bridge sentinel-override is the path heartbeats actually take in production β€” explicit chat_id always wins over the spawn-time TALON_CHAT_ID="heartbeat" default.

Verification

  • npx vitest run β€” full suite 1848/1861 pass (1 pre-existing package.functional failure unrelated, 12 skipped live-tier)
  • 96 tests across the 4 directly-touched files all pass
  • tsc --noEmit clean
  • prettier --check clean
  • npm run lint β€” 0 errors, 0 warnings on changed files

Stacking with PR #144

This branches off main (77e7771, post-#150). It does not include PR #144 (heartbeat-eviction). When both land, the second to merge will need a trivial rebase β€” different sections of heartbeat.ts, no real conflict.

πŸ€– Generated with Claude Code

PR #150 introduced an outbound-messaging block in the heartbeat system
prompt that hard-codes `telegram-tools`, "Telegram ID", and Telegram-only
examples. Talon supports multiple frontends β€” including running multiple
at once β€” and the heartbeat agent gets one `${frontend}-tools` MCP server
spawned per non-terminal frontend via `buildMcpServers`. The hard-coding
in the prompt misleads heartbeats running on non-Telegram deployments.

What changes

- New `getActiveFrontends()` helper in `backend/claude-sdk/options.ts`,
  exported via the barrel. Returns the list of non-terminal frontends β€”
  same filter `buildMcpServers` was using inline, now de-duplicated.
- New `buildHeartbeatSystemPrompt()` in `core/heartbeat.ts` (exported for
  tests). Builds the prompt dynamically based on `getActiveFrontends()`:
  * Zero non-terminal frontends β†’ base prompt only, no outbound block,
    no `chat_id` mention. Terminal-only deployments don't have an
    outbound messaging surface so the section would be confusing.
  * One or more frontends β†’ lists every `${frontend}-tools` server by
    name in the OUTBOUND MESSAGING block, uses the first frontend in
    the example `send(...)` call, refers to chat IDs as "per-frontend"
    (Telegram-specific guidance moved to memory.md territory).
- Falls back to the base prompt if `getActiveFrontends()` throws (test
  paths where the agent config isn't initialised) β€” same try/catch
  pattern as the `buildMcpServers` call in #150.

The hard-coded Telegram chat_id (`352042062`) is no longer in the system
prompt β€” that lives in memory.md anyway. Other deployments' user IDs are
unaffected.

Tests

7 new vitest cases in `src/__tests__/heartbeat.test.ts`:
- no frontends β†’ base only, no `OUTBOUND MESSAGING`, no `telegram-tools`
- config throws β†’ base only
- `telegram` only β†’ references `telegram-tools`, not `teams-tools`
- `teams` only β†’ references `teams-tools`, not `telegram-tools`
- multi-frontend (`telegram` + `teams`) β†’ both listed
- first frontend goes into the example send() call
- no `chat_id` mention when no frontends are configured

27/27 heartbeat tests pass. Full suite 1821/1834 (1 pre-existing
`package.functional` failure unrelated, 12 skipped live-tier).
typecheck / prettier / lint all clean.

Stacking note

This branches off `main` (`77e7771`, post-#150). It does NOT include
PR #144 (heartbeat-eviction). When both land, the second to merge will
need a trivial rebase β€” different sections of `heartbeat.ts`, no real
conflict.

πŸ€– Generated with [Claude Code](https://claude.com/claude-code)
Expands the test surface around PR #150's heartbeat outbound flow and
the frontend-agnostic prompt builder added earlier in this PR. Adds
22 new test cases across three files focused on the gaps the existing
coverage left.

claude-sdk-options.test.ts (+11)
  getActiveFrontends helper β€” 8 cases covering scalar/array config,
  terminal filtering, multi-frontend order preservation, throw
  propagation when config not initialised.
  buildMcpServers heartbeat-tier β€” 7 cases (3 new) covering the
  heartbeat sentinel (TALON_CHAT_ID="heartbeat"), per-frontend env
  isolation (no cross-pollination between telegram-tools and
  teams-tools env vars), multi-frontend + brave-search combos.

bridge.test.ts (+6)
  Edge cases the original 6 didn't cover:
  - Negative numeric chat_id (Telegram supergroup IDs are negative β€”
    -100<id> β€” these are the most common heartbeat targets in
    practice; coercion bugs would have been silent).
  - chat_id=0 promoted correctly (gateway decides what to do).
  - chat_id=null forwarded as String(null)="null" (defensive; the
    schema validates upstream but the bridge mustn't crash).
  - Heartbeat sentinel default ("heartbeat") MUST be overridden when
    explicit chat_id is supplied β€” otherwise outbound from heartbeat
    would silently fail to route.
  - Heartbeat sentinel passed through verbatim when no explicit
    chat_id (gateway's rawChatId !== "heartbeat" guard catches it).
  - String chat_id with leading zeros preserved (no Number()
    coercion).

gateway-http.test.ts (+5)
  Edge cases on the explicit-routing branch:
  - Negative chat_id (Telegram supergroup) routes without active
    context.
  - chat_id=0 rejected by the !chatId falsy guard.
  - Heartbeat sentinel as explicit chat_id rejected (rawChatId !==
    "heartbeat" guard).
  - Negative chat_id sign preserved through the handler β€” defensive
    against accidental Math.abs / int32 truncation.
  - Coexistence: explicit chat_id matching an active context still
    routes via the explicit-routing branch (no exception, no
    double-dispatch).

Verification
- npx vitest run β€” full suite 1848/1861 pass
  (1 pre-existing package.functional failure unrelated, 12 skipped
  live-tier)
- The four directly-touched files (claude-sdk-options, bridge,
  gateway-http, heartbeat) account for 96 tests, all passing
- tsc --noEmit clean
- prettier --check clean
- lint β€” 0 errors, 0 warnings on changed files

Why these specifically: negative chat_id is the most subtle silent-bug
surface (Telegram groups all have negative IDs, a sign-handling bug
would route to the wrong chat or fail silently). The heartbeat
sentinel guard is the routing-safety net (if it ever stopped working,
heartbeats could spam unrelated chats). The getActiveFrontends helper
is the multi-frontend correctness backbone for the new prompt builder.

πŸ€– Generated with [Claude Code](https://claude.com/claude-code)
@claudiusthebot claudiusthebot changed the title fix(heartbeat): frontend-agnostic outbound-messaging prompt fix(heartbeat): frontend-agnostic outbound + comprehensive test coverage May 12, 2026
@claudiusthebot
claudiusthebot merged commit 85251d3 into main May 12, 2026
22 checks passed
dylanneve1 pushed a commit that referenced this pull request May 12, 2026
…153)

PR #150 wired heartbeat outbound `send` and `react` with
`chat_id: idSchema`, but `idSchema` enforces `.positive()` β€”
designed for message/user/reply IDs (always positive in Telegram).
Telegram supergroup/channel chat IDs are NEGATIVE
(e.g. -1001426819337), so the schema rejected them at the MCP
tool-input layer before the gateway ever saw the request:

  "expected number, received string" (the model sees a number-typed
  JSON Schema field, zod rejects the negative integer)

The gateway-side handling of negative chat_ids was already
correct and tested (`src/__tests__/gateway-http.test.ts:343-401`
explicitly covers `chat_id: -1001426819337` round-tripping). The
bug was the tool-input schema alone.

Surfaced by Dylan's heartbeat outbound test in
Pandario chat at 2026-05-12 18:13Z β€” the canonical PR #150 / #151
usage path (`send(type="text", text="...",
chat_id=-1001426819337)`) failed validation at the door.

Fixes:

1. New `chatIdSchema` in `src/core/tools/schemas.ts` β€” a union of
   non-zero signed integer or signed-integer string. Accepts both
   negative (group/channel) and positive (DM/user) chat IDs.
   Rejects zero (the gateway's existing falsy-guard sentinel),
   non-integers, non-numeric strings, booleans, null. `idSchema`
   itself is unchanged β€” message/user/reply fields still get the
   strict positive contract.

2. `src/core/tools/messaging.ts` β€” `send.chat_id` and `react.chat_id`
   now use `chatIdSchema` (line 167 + line 338). Field descriptions
   updated to mention the supergroup-negative convention.

3. New `src/__tests__/chat-id-schema.test.ts` (24 tests) β€”
   regression pinning:
   - chatIdSchema accepts Dylan's DM ID (positive) AND the Pandario
     supergroup ID (negative) β€” both as numbers AND as strings.
   - chatIdSchema rejects zero, `-0`, non-integers, non-numeric
     strings, booleans, null, whitespace-padded numbers.
   - idSchema STILL rejects negatives (confirms the two schemas
     don't drift back into the same contract).
   - send.chat_id and react.chat_id (wired into ALL_TOOLS) accept
     both the Dylan-DM and Pandario-supergroup cases. The exact
     -1001426819337 from Dylan's test is in the test list.

Verification:

- `npx tsc --noEmit` clean.
- `npx prettier --check` clean on all three changed files.
- `npx vitest run src/__tests__/chat-id-schema.test.ts
  src/__tests__/tool-id-coercion.test.ts
  src/__tests__/gateway-http.test.ts` β†’ 290/290 pass. The existing
  tool-id-coercion tests confirm `idSchema` is unchanged; the
  existing gateway-http negative-chat_id tests still pass.

Surfaced-by: 2026-05-12 18:13Z heartbeat outbound test (Dylan)

Co-authored-by: claudiusthebot <claudiusthebot@users.noreply.github.com>
dylanneve1 pushed a commit that referenced this pull request May 19, 2026
The flow-violation handler previously silently dropped turns under two
conditions that are now both fixed:

1. **Tool calls without end_turn**: if the model ran tool calls (e.g.
   Bash) then exited without calling `end_turn` / `send` / `react`, the
   `flowViolation` check required trailing prose to fire. With prose
   absent, the handler exited silently β€” user gets no response, no
   observability beyond "turn completed with empty output." This was
   the exact bug Dylan hit on 2026-05-12 after the PR #151 merge: I ran
   `git pull` + `talon restart` in a Bash tool, the result returned,
   and I never called `end_turn`. He had to ask "Why u didn't reply??"

2. **Single-retry cap**: even when the trailing-prose case DID fire,
   the handler re-prompted once and then silent-dropped on the second
   violation. Dylan's design rule (2026-05-12): "automatically dropping
   should never happen β€” you can choose to not reply but the system
   shouldn't drop on its own." The model has to commit by calling
   `end_turn()` (with no args is the explicit "no reply" close).

Changes

- New `FLOW_VIOLATION_MAX_RETRIES = 3` constant. The handler now
  re-prompts up to 3 times before accepting a drop (instead of once).
  Set high enough that real-world model slip-ups recover; low enough
  that a pathologically broken model doesn't loop forever burning
  tokens. After cap exhaustion the drop is logged at ERROR level so
  observability catches it instead of disappearing into the void.

- `_retried: boolean` β†’ `_retryCount: number = 0`. Error-recovery
  branches (session_expired / context_length / model fallback) now
  check `_retryCount === 0` instead of `!_retried` to preserve the
  "retry once" semantics for transient infra failures. Each error path
  still gets exactly one retry attempt.

- `flowViolation` check now detects BOTH failure modes:
    flowViolation = !state.turnTerminated && hadActivity && !isDup
    where `hadActivity = trailing.length > 0 || state.toolCalls > 0`
  The toolCalls > 0 leg catches the case I personally hit. Empty turns
  with no prose AND no tool calls (rare) still don't trigger β€” those
  are unambiguously "nothing to deliver."

- Reminder body rewritten to be more directive: lists ALL four valid
  termination paths (`end_turn(text=)`, `end_turn()`, `send(...)`,
  `react(...)`) and explicitly mentions that tool calls alone don't
  close a turn.

- Stale comment about "give up loudly and accept the silent drop" on
  the second retry removed β€” the new behavior is "re-prompt up to 3,
  then ERROR-log if still violating."

Tests

Four new integration cases in `src/__tests__/integration/talon-functional.test.ts`:

  - **Re-prompt fires for trailing prose** (existing case, now verifying
    the new reminder body text)
  - **Re-prompt fires for tool-calls-without-end_turn** (NEW β€” the
    canonical no-delivery case that wasn't caught before)
  - **Cap exhausts at exactly 3 retries** when the model never recovers
    (would have been 1 in the old code, would loop forever if cap was
    accidentally removed)
  - **No re-prompt on clean end_turn() with no args** (the explicit
    "I chose not to reply" close)

Infrastructure note: each `handleMessage` call spawns a fresh stub
subprocess that resets `turnIndex` to 0, so we can verify re-prompts
fire (via STDIN protocol log inspection) but can't easily test "model
recovers via script.turns[1]" without infra surgery. The unit assertion
that matters most β€” "re-prompt was attempted, not silent-dropped" β€” is
fully covered.

Verification

- npx vitest run β€” full suite 1852/1865 pass (1 pre-existing
  package.functional failure unrelated, 12 skipped live-tier)
- 9/9 functional tests pass (5 existing + 4 new)
- tsc --noEmit clean
- prettier --check clean
- lint β€” 0 errors, 0 warnings on changed files

Real-world value: today's failure mode is impossible going forward.
Even if a future me forgets to call end_turn, the handler will surface
[FLOW VIOLATION] reminders three times before giving up β€” and the
giving up itself is now an ERROR-level log instead of silent.

πŸ€– Generated with [Claude Code](https://claude.com/claude-code)
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.

1 participant