Skip to content

fix(handler): never silently drop a turn β€” re-prompt until model commits - #152

Closed
claudiusthebot wants to merge 1 commit into
mainfrom
fix/flow-violation-never-silent-drop
Closed

fix(handler): never silently drop a turn β€” re-prompt until model commits#152
claudiusthebot wants to merge 1 commit into
mainfrom
fix/flow-violation-never-silent-drop

Conversation

@claudiusthebot

Copy link
Copy Markdown
Collaborator

Why

Two failure modes caused turns to silently drop with no user-visible response and no observability beyond "turn completed with empty output":

1. Tool calls without end_turn (NEW β€” the bug Dylan hit today)

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. Dylan had to ask "Why u didn't reply??" β€” and the handler exited silently because:

const flowViolation =
  trailing.length > 0 &&  // false β€” no prose was written
  !state.turnTerminated &&
  !isDuplicateOfDelivered(...);

With no trailing prose, flowViolation = false regardless of whether a delivery tool was called. Tool-only turns slipped through the safety net.

2. Single-retry cap

Even when the trailing-prose case DID fire, the handler re-prompted once and silent-dropped on the second violation. Dylan's design rule (2026-05-12 13:52):

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. end_turn() with no args is the explicit "no reply" close β€” the only valid silent close.

Changes

  • New FLOW_VIOLATION_MAX_RETRIES = 3 constant. Re-prompts up to 3 times before accepting a drop (instead of 1). 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.

  • _retried: boolean β†’ _retryCount: number = 0. Error-recovery branches (session_expired / context_length / model fallback) now check _retryCount === 0 instead of !_retried to preserve "retry once" semantics for transient infra failures.

  • flowViolation check detects BOTH failure modes:

    const hadActivity = trailing.length > 0 || state.toolCalls > 0;
    const flowViolation =
      !state.turnTerminated && hadActivity && !isTrailingDuplicate;

    The toolCalls > 0 leg catches the case I hit today. 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.

Tests

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

  • Re-prompt fires for trailing prose without delivery (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; the test I personally wish had existed yesterday)
  • 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 β€” must be respected without re-prompting)

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 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
  • npm run lint β€” 0 errors, 0 warnings on changed files

Real-world impact

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 up to 3 times before giving up β€” and giving up itself is now an ERROR-level log instead of silent.

πŸ€– Generated with Claude Code

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)
@dylanneve1
dylanneve1 force-pushed the fix/flow-violation-never-silent-drop branch from d40f457 to b1665b3 Compare May 19, 2026 10:20
@dylanneve1

Copy link
Copy Markdown
Owner

Superseded by #246. I reviewed the old changes and ported the valid/current fixes onto current main in the consolidated PR, while leaving out stale removed-backend changes and broad churn that no longer applies.

@dylanneve1 dylanneve1 closed this May 22, 2026
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.

2 participants