fix(kilo): make the chat flow actually work end-to-end - #170
Merged
Conversation
claudiusthebot
force-pushed
the
fix/kilo-strict-delivery-and-logging
branch
from
May 15, 2026 15:04
883f1c1 to
004f598
Compare
claudiusthebot
changed the title
dylanneve1
enabled auto-merge (squash)
May 15, 2026 21:36
dylanneve1
force-pushed
the
fix/kilo-strict-delivery-and-logging
branch
from
May 15, 2026 21:36
2201904 to
efafdc5
Compare
…ck turns
Symptoms in prod (~/.talon/talon.log) on the kilo backend with
deepseek-v4-flash:free:
[chatId] flow violation (Kilo): trailing prose (16 chars) without
end_turn/send. Re-prompting with reminder.
... [retry produces another tiny chunk of prose]
[chatId] flow violation (Kilo): trailing prose (53 chars) without
end_turn/send. Already retried — accepting silent drop.
Two issues compounded:
1. The KILO_SYSTEM_PROMPT_SUFFIX told the model BOTH end_turn AND plain
text would be delivered, but the handler always dropped trailing
prose as a flow violation. Weaker models that don't reflexively reach
for end_turn (DeepSeek free et al.) believed they had replied while
the user saw nothing.
2. The flow-violation log only said "trailing prose (N chars)" — no
preview of what the dropped content actually was, no per-event SSE
counts to tell whether the model genuinely went silent or whether the
handler missed an event, no timing breakdown to distinguish "MCP
setup is slow" from "model is slow".
Fixes:
- Rewrite KILO_SYSTEM_PROMPT_SUFFIX as a strict tool-only contract.
end_turn / send / react are the ONLY ways a reply reaches the user.
Drop the misleading "plain text also works" wording.
- Reword FLOW_VIOLATION_REMINDER along the same lines (drop the
"scratchpad" framing, list every delivery tool by name).
- Log a JSON-quoted preview of the dropped trailing prose (first ~120
chars, whitespace-collapsed) so operators can tell at a glance whether
the model produced a meaningful reply Talon dropped.
- Log Kilo model resolution result (provider/model + lookup vs cached).
- Add `eventCounts` to shared StreamState; events.ts increments per SSE
type; the end-of-turn handler line shows `events=delta×42,part.updated×1`
style so silent turns are visible at a glance.
- Log every terminator firing with structured args
(`terminator fired: end_turn text=42chars`).
- Beef up session.error logging to dump name/message/data, not just name.
- Stamp ensureChatMcpServer / ensurePluginMcpServers / Kilo server
spawn with elapsed-ms; flag MCP registrations slower than 1s as
`[slow]` so misbehaving plugins are visible in the log tail.
- Extend the end-of-turn line with `terminator/delivered/respLen/setup/turn`
fields to make the success path auditable too.
Test updates:
- shared-flow-violation: rename the assertion that previously checked
for "scratchpad" to check for "react" — the surface area expanded.
- StreamState now ships an `eventCounts: {}` field by default;
no test changes needed (only handler/events read it).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…user
Two bugs caused chat 352042062 to look "stuck" with no reply on the
kilo backend:
1. MCP re-registration storm. Kilo's `GET /mcp` (which oc.mcp.status()
hits) empirically always returns `{}` regardless of which servers
are actually connected — a curl probe confirmed an empty response
even after 16 successful adds. Talon's `existingServers[name]?.status
=== "connected"` check therefore always evaluated false, so
ensureChatMcpServer + ensurePluginMcpServers re-registered every
server on every turn (~12s of wasted setup per message). Kilo's POST
/mcp is idempotent for already-connected names, so it worked, but
slowly. Compounded by an unconditional `disconnectChatMcpServer` in
the handler's finally block that explicitly torpedoed the chat-tools
subprocess after every turn just to spawn it again on the next.
Fix: track registered server names in a process-local Set
(`registeredMcpServers`) — this is the source of truth Talon trusts
instead of the broken status endpoint. Drop the per-turn
disconnectChatMcpServer call (chat-scoped server safely outlives a
single turn). Expected: setup time falls from ~12s to ~1ms after
the first message of a process.
2. Silent stuck on retry exhaustion. With strict end_turn-only delivery,
weaker models (DeepSeek free) keep narrating "Dylan sees a 'Hey' in
the chat — clean delivery." instead of calling end_turn(). The
first violation triggered a retry; the second went through the
`Already retried — accepting silent drop` path and the user saw
nothing at all. The new path emits a visible warning instead so the
user knows the model failed to use a delivery tool and can pick a
stronger model with /model.
Tests rewritten for the new contract: registrations are skipped via the
local cache, not Kilo's status response. A regression test covers the
disconnect→re-register round-trip via the cache.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pped
Curling Kilo's `/global/event` endpoint shows the wire format is:
data: {"payload":{"type":"server.connected","properties":{}}}
data: {"payload":{"type":"server.heartbeat","properties":{}}}
But the handler reads `evt.type` / `evt.properties` directly. Those
fields don't exist at the top level — the actual event lives under
`evt.payload`. So `event.type` was always undefined, the switch hit the
default branch every time, no event-type counts incremented, no tool
calls / terminators / turn.close ever recognized mid-flight.
The end-of-turn `events=none` summary wasn't a quirk; it was a 100%
miss rate. The handler quietly fell back to draining the sync
`session.prompt` parts list, which meant:
- No mid-turn streaming UX (no `onStreamDelta` ever fired).
- Tool-call detection lost the SSE fast path; only the post-completion
parts walk caught anything.
- Terminator-fired short-circuits (the `oc.session.abort` call after
end_turn) never happened, so every successful turn waited for the
model's full wrap-up round-trip before the prompt() promise resolved.
- The flow-violation log preview showed the model literally typing
`end_turn(text="Hey sur, doing well.", reply_to=2538)` because the
model's tool call landed as a text part and we never got the SSE
`message.part.updated` for the tool half.
Fix: unwrap `evt.payload` before the type/properties read. existing
tests pass through `processStreamEvent` directly with the already-
unwrapped shape, so no test surgery needed; the bug was purely in the
handler's SSE consumer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…egram) After the SSE-payload unwrap landed (every event was being silently dropped before that), `message.part.delta` events started actually firing through to `onStreamDelta`. The Telegram frontend's `onStreamDelta` calls `bot.api.sendMessageDraft` to live-edit the message as the model streams — so the user suddenly started seeing the model's chain-of-thought appear in their chat as it generated, which is not the contract we want. Telegram delivery is "send the final reply once" via `end_turn` / `send`. The mid-turn deltas are diagnostic state for tool detection + event-count summary, not a UI feed. Stop forwarding `onStreamDelta` through the SSE consumer; final delivery still flows through `onTextBlock` exactly once when a delivery tool fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulled the actual session messages from Kilo and discovered the real flow: Kilo's models (DeepSeek, GLM, openrouter routes) emit `type: "text"` parts as their natural reply. They do NOT call `end_turn` the way Claude does. msg role=assistant parts=['step-start', 'reasoning', 'step-finish', 'text'] Talon was modelled on Claude SDK's tool-driven contract — it expected `end_turn(text=...)` and treated everything else as scratchpad. With Kilo that meant: 1. SSE `message.part.delta` events with `field: "text"` accumulated into `state.allResponseText` regardless of source. ReasoningPart and TextPart both store content in a `text` field, so reasoning leaked into the response buffer. 2. The handler's flow-violation check then saw "trailing prose without end_turn" and dropped everything (including the actual reply that landed via the text part). Fix: - `shared/stream-state.ts`: add `partTypes: Map<partID, type>` so delta handlers can classify deltas against their source part. - `kilo/events.ts processPartUpdate`: track every part's type by id (not just tool parts). - `kilo/events.ts processPartDelta`: when we already know a delta's part is reasoning/thinking, don't accumulate. When unknown, accumulate optimistically — finalize will rewrite. - `kilo/events.ts finalizePartsIntoState`: parts list is now the authoritative source. Walk text parts, write `state.allResponseText` from them only (reasoning ignored). Walk tool parts as before. - `kilo/handler.ts`: drop the flow-violation re-prompt path entirely. Text-part content is now `responseText` and gets shipped via `onTextBlock` directly. The only error path is "truly empty turn" (no text part, no tool call) — that gets a visible warning. Tests updated: a finalize test that checked the old "skip text part if SSE captured" behaviour now checks the new "rewrite from text parts (parts list is source of truth)" behaviour, plus a new test that explicitly verifies reasoning parts are ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous handler tried to handle "no delivery happened" with a heuristic that conflated three distinct outcomes: tool already delivered, plain text part to ship, and truly empty turn. When tools fired but didn't include `end_turn`/`send`/`react` (e.g. a search plugin) the handler couldn't tell whether it should re-emit the text-part response or the dedup branch was firing — so deliveries went silently missing or got duplicated. Make the routing explicit: - `tool` — delivery tool already shipped the message, nothing to do. Detected by `deliveredTextNorms` being non-empty AND containing the responseText (or responseText being empty). - `text-part` — Kilo's natural reply lives in `state.allResponseText` from `finalizePartsIntoState`. Ship via `onTextBlock`. - `empty` — no text part, no delivery tool. Surface a concise notice so the user knows the model returned nothing instead of staring at silence. Tone the warning down (was "Try /model to switch", now just "(no reply — model returned no output)" or, if a tool fired, "(no reply — model called tools but didn't produce output text)"). Each turn logs `[chatId] delivery: <route> (<chars> chars)` so operators can see at a glance which path fired. Removed the now-unused `detectFlowViolation` import and the `formatProsePreview` helper that was only called from the dead flow-violation log line. System prompt updated to match the actual contract: text parts work, delivery tools work, dedup if you do both. The previous wording said end_turn was the ONLY way, which contradicted the handler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….error to our session Two related issues found while debugging a chat that hung for 5 minutes on the user's prod deployment: 1. **Heartbeat misroute.** The user's config had `heartbeatModel: "kilo/deepseek/deepseek-v4-flash:free"` (with `kilo/` prefix as a Talon-side hint). `parseStoredKiloModelSelection` returned the whole thing as the model id, so the Kilo router got `model.id = kilo/deepseek/deepseek-v4-flash:free` and concatenated its own provider in front, producing the upstream error `Model not found: opencode/kilo/deepseek/deepseek-v4-flash:free`. Now the parser strips the `kilo/` prefix and pins `providerID = "kilo"` so both forms (`foo` and `kilo/foo`) work. 2. **Cross-session error attribution.** Kilo's `/global/event` SSE stream is global — every session's events are interleaved. The handler's `session.error` branch was logging events under our chat's `[chatId]` regardless of which session they actually belonged to, so the heartbeat session's "Model not found" error appeared under chat 352042062 in the log. That's the kind of misattribution that derails a debugging session: I spent ages thinking the chat model was failing when it was actually the heartbeat. Scope-filter the error branch to our `sessionId` before logging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t as a reply User saw their chat reply 4× as: "The model hit its output limit while reasoning and produced no actionable output. Try disabling reasoning or increasing the output limit." That string is Kilo's internal failure marker — not a model reply. Kilo emits it as a `type: "text"` part with `synthetic: true` when the upstream model bombs (output limit, reasoning loop, etc.). The handler was treating any text part as the delivery and shipping it verbatim, so the user saw what looks like the model giving them technical advice about itself. Fix: - `kilo/sessions.ts extractPartsSummary`: peel off `synthetic: true` text parts into a separate `syntheticErrorText` field. Also skip `ignored: true` parts (Kilo's other "this isn't really part of the reply" flag). - `shared/stream-state.ts`: add `syntheticError?: string` to the state. - `kilo/events.ts finalizePartsIntoState`: when only synthetic text exists, write `state.syntheticError` (not `state.allResponseText`) and clear any speculative SSE accumulation. - `kilo/handler.ts`: new `synthetic-error` delivery branch — surface the Kilo error to the user as `⚠️ Kilo: <message>` so they can recognise it as an upstream failure and not as the model talking about itself. Logged with `kilo.synthetic_error` counter and a prose-preview log line so operators can spot the pattern. Two regression tests cover the synthetic and ignored part flags landing through finalize. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…plies Pulled the actual session messages and discovered Kilo flags every single text-part reply for deepseek/deepseek-v4-flash:free with `ignored: true` (137-char text parts at the end of every `step-start, reasoning, step-finish, text` assistant message). That defeats the whole point of the schema field's name, but it's what the upstream actually does. My previous commit (peeling off synthetic+ignored parts) wiped out every reply because of this. The user saw nothing but the empty-turn warning "(no reply — model returned no output)" for several minutes. Keep the `synthetic: true` filter (still correct — those are upstream failure markers like "model hit its output limit while reasoning"), but drop the `ignored` filter and document the counterintuitive behaviour with an explicit regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Confirmed via session-message dump: Kilo names MCP tools as `<server>_<tool>` (single underscore boundary), e.g. `talon-tools-352042062_send`, `talon-tools-352042062_end_turn`. Talon's `stripMcpPrefix` only handled the canonical Claude SDK form `mcp__<server>__<tool>`, so on Kilo every MCP tool name slipped through unchanged. Two visible consequences: 1. **Duplicate replies.** `captureDeliveredText` checks `bareName === "send"` after stripping. With Kilo, the bare name was `talon-tools-352042062_send`, the check failed, no `deliveredTextNorms` was captured, and the handler's text-part fallback ran on top of the tool-driven delivery. The user saw two identical messages per reply. 2. **`turnTerminated` never fired** for Kilo turns where the model actually called `end_turn`. `isTurnTerminator(toolName)` strips the prefix before checking against the terminator set; with the prefix un-stripped, the check missed and we kept logging `terminator=no delivered=0` even when the model behaved correctly. Fix: extend `stripMcpPrefix` to walk underscore boundaries from the right and return the longest tail that matches the registered tool catalog (`ALL_TOOLS`). The walk is needed because bare names contain underscores (`end_turn`) — naive "split on _ and take last" would mis-resolve `..._end_turn` as `turn`. Tests cover both formats and the negative case where no suffix matches a known tool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reply After fixing stripMcpPrefix, Kilo's `delivered=N` tracking finally worked, but the user still saw double messages: Dylan: How are you Bot: "6:30PM Friday in Ireland, running on DeepSeek V4 Flash..." ← send tool Bot: "Sent with chat_id and reply_to." ← text part Kilo models (stepfun/step-3.5-flash:free, etc.) routinely emit a follow-up text part *after* a delivery tool fires. The text part is the model's chain-of-thought commentary on what it just did — not a separate reply. The previous dedup-by-substring check only caught it when the commentary echoed the delivered text, so the "Sent with chat_id"-style postscripts slipped through and got shipped as their own message. Replace the substring dedup with a hard rule: any non-zero `deliveredTextNorms` ⇒ the tool already delivered, drop the text part wholesale. The previous behaviour of preserving the text part when it differed from the tool text was theoretical; the only real case where it fired was the false-positive double-message bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous handler awaited `oc.session.prompt(...)`, a synchronous
HTTP POST that Kilo holds open until the upstream model finishes.
When the upstream stalls (free providers especially), the POST hangs
and our `await` blocks indefinitely. Manual `curl /session/.../abort`
was the only way to recover.
Switch to `oc.session.promptAsync`, which returns immediately, and
drive the turn from SSE events alone. Talon's await is now on the SSE
iterator (event-loop-bound work we control), not on a long-running
HTTP call we can't interrupt:
1. Subscribe to SSE first so no early `session.turn.open` /
`message.part.updated` events are missed.
2. Fire `promptAsync` (returns instantly with a messageID).
3. Await the SSE close event for our session — `session.turn.close`,
`session.idle`, or `session.error`.
4. On `session.error`, stash the upstream error message on
`state.syntheticError` and exit the SSE loop. The handler's
existing synthetic-error delivery branch surfaces it as
`⚠️ Kilo: <message>` instead of leaving the user with silence.
5. Read the authoritative parts list via `session.messages` and
drain via `finalizePartsIntoState`.
Removes the `waitForAssistantReply` race fallback (no longer needed —
session.messages is read once at end-of-turn) and the now-unused
`appendText` / `extractPartsSummary` imports from the handler.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… hangs
Group-chat turns kept hanging after the promptAsync refactor. Pulled
the stuck session's parts and the cause was clear:
last assistant parts:
[0] step-start
[1] reasoning
[2] tool tool=read status=running ← stuck
The model called Kilo's built-in `read` tool, which raised a
`permission.asked` event (visible as `permission.asked×1` in earlier
end-of-turn event-count summaries). Talon's question watchdog only
auto-rejects `question.*` events via `oc.question.reject` — it has no
handler for `permission.*`. So the tool sat in `running` forever, no
`turn.close` ever fired, the SSE iterator waited indefinitely.
DM hadn't surfaced this because the model didn't call file tools in
DM context — group-chat-shaped prompts triggered different behaviour.
Fix: when `ensureSession` creates a new Kilo session, call
`oc.permission.allowEverything({enable: true, sessionID})` so Kilo
skips its permission gate for that session entirely. Equivalent to
the Claude SDK backend's `permissionMode: "bypassPermissions"`.
Failure of the allowEverything call (older Kilo without the endpoint)
is swallowed — the worst case is permission prompts continue to hang,
which is no worse than today.
Two tests cover the new path: success populates the bypass, and a
404-style failure doesn't crash the session creation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reproduced live: Dylan: React to my message Bot:⚠️ Kilo: MessageAbortedError When the model called `react` (a terminator tool), Talon's `onTerminator` callback fired `oc.session.abort` to short-circuit Kilo's post-tool wrap-up. Kilo then emitted `session.error` with `name: "MessageAbortedError"` — the canonical close signal for an aborted session. The promptAsync refactor's new error-stashing path treated that as an upstream failure and pushed it through the `⚠️ Kilo: <message>` delivery branch, surfacing our own abort to the user as if Kilo had bombed. Filter `MessageAbortedError` (or anything with /abort/ in the name or message) when `state.turnTerminated` is set — that's the proof we asked for the abort. Real upstream failures (rate limit, model not found, network) still flow through the syntheticError path. Also still exits the SSE loop cleanly so the turn closes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dissecting the SDK types showed the correct fix. The deprecated
per-prompt `tools` map and our `permission.allowEverything` workaround
are both subsumed by `session.create({permission: PermissionRuleset})`
— Kilo's own type definitions even call out:
/**
* @deprecated tools and permissions have been merged, you can set
* permissions on the session itself now
*/
tools?: { [key: string]: boolean };
`PermissionRuleset` is `Array<{permission: string, pattern: string,
action: "allow" | "deny" | "ask"}>` — first match wins.
`ensureSession` now creates each Kilo session with a 5-rule set:
1. allow tool talon-tools-<thisChat>_* ← this chat's MCP tools
2. deny tool talon-tools-* ← every OTHER chat's
3. allow tool * ← Kilo built-ins, plugins
4. allow edit * ← skip permission.asked
5. allow bash * ← skip permission.asked
This fixes two prod symptoms in one stroke:
- The model in chat A could see and call `talon-tools-<chatB>_send`
(Kilo exposes every MCP server's tools to every session). The bridge
routed by the called server's TALON_CHAT_ID, hitting the gateway's
active-context check and returning "No active chat context", or
worse, leaking content cross-chat. Rule (2) hides the other chats'
tools entirely.
- Built-in `read` / `bash` raised `permission.asked` mid-turn. Talon's
watchdog only auto-rejects `question.*` events; permission stayed
pending forever, the tool sat in `running`, the SSE iterator waited
for a `turn.close` that never fired. Rules (3-5) auto-allow.
Drop the now-redundant `oc.permission.allowEverything({enable: true})`
call from ensureSession, and the per-prompt `tools: toolOverrides`
plumbing in handler.ts (deprecated and useless once the session has
permission rules).
`buildToolOverrides` stays exported because OpenCode's handler/one-shot
and Kilo's one-shot still call it; that's a follow-up.
Test updated to assert the full ruleset on session.create instead of
the deprecated `permission.allowEverything` invocation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hat tools)
The session-level permission ruleset blocks tool *execution* but not
*visibility* — Kilo still lists every connected MCP server's tools in
the model's catalog. Result: a model in chat A could see and call
`talon-tools-<chatB>_send`. Even with the deny rule firing, the model
wasted a turn calling the wrong tool, and the bridge log showed no
real react/send fire (silently denied).
The only way to actually hide cross-chat tools from the model is to
hold only one chat-namespaced MCP server connected at a time. When
chat A starts a turn and `ensureChatMcpServer("chat-a")` runs, walk
the cache and disconnect every other `talon-tools-*` server first
(except `talon-tools-heartbeat`, which is the sentinel for background
agents and must always stay connected). Then proceed with the
existing register-or-cache-hit logic.
Cost: ~800ms re-spawn whenever the active chat switches. Same chat
on subsequent turns is still ~1ms (cache hit). Heartbeat and plugin
servers are untouched.
Two new tests cover the chat-switch behaviour: the previous chat's
server gets disconnected on switch; the heartbeat server is exempt.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OpenCode shares the same SDK shape as Kilo (Kilo is a fork) and the
same upstream behaviour: every connected MCP server's tools land in
the model's catalog regardless of which session is asking, and
permission events on built-in `read` / `bash` hang on
`permission.asked` waiting for a watchdog reply Talon doesn't fire.
Port the four kilo fixes verbatim:
1. **Local registration cache.** OpenCode's `GET /mcp` returns `{}`
regardless of state — same quirk as Kilo's. Track our own
registrations in a process-local Set so subsequent turns don't
re-register every plugin server (~12s of wasted setup per message).
2. **Per-turn chat-MCP isolation.** When `ensureChatMcpServer(chatA)`
runs, walk the cache and disconnect every other `talon-tools-*`
server first (heartbeat sentinel exempt). Holding only one
chat-namespaced server connected at a time is the only way to
physically hide cross-chat tools from the model — permission rules
only block execution, not visibility.
3. **Per-session permission ruleset on session.create.** Allow this
chat's tools, deny other chats' tools (defense in depth), allow
built-ins so `read` / `bash` don't sit in `permission.asked`
forever.
4. **Cache-invalidating disconnect helper.** `disconnectChatMcpServer`
now drops the entry from the local cache so a future ensure
re-registers correctly.
`stopOpenCodeServer` clears the cache too.
Also fixes the type cast in the kilo-server test that broke CI's
Code Quality job.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First message after restart paid ~12s of subprocess-spawn time as ensurePluginMcpServers walked all 16 plugin MCP servers in series (extras-tools, wikipedia-tools, github-tools, etc.). The user-facing result was a long "typing…" pause before the first reply lands, visible on every restart. Move that work into the agent's `init` path: kick off ensurePluginMcpServers in the background as soon as the agent is configured, so by the time the first chat message arrives the subprocesses are already up and the per-turn ensure short-circuits via the local cache. Per-chat MCP servers (`talon-tools-<chatId>`) can't be pre-warmed (chatId is unknown until a message arrives) so the first turn for each chat still pays one ~800ms spawn. The dominant cost — 16 plugin servers in series — is amortised away. Errors are swallowed (best-effort): per-turn ensure still runs and would log any real failures. The `"prewarm"` sentinel chat id keeps plugin tools that DON'T use the bridge (most of them — wikipedia, ffmpeg, github, etc.) working correctly. Plugin tools that DO bind to chat context get rebound when a real chat starts because their bridge calls accept `chat_id` from tool params at call time, not from MCP-level env. Same change applied to both kilo/server.ts and opencode/server.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an integration test that drives Talon's production `bootstrap()` +
`initBackendAndDispatcher()` end-to-end against a real `kilo serve` —
no test-specific bootstrap helper, no mocked SDK client.
The only test-side fakes:
1. `vi.mock("../../util/paths.js")` — redirects `dirs.root` to a
tempdir so the test never touches the developer's `~/.talon/`.
2. `vi.hoisted(() => process.env.KILO_PORT = ...)` — points the
production `KILO_PORT` const at a port distinct from prod's 4097
so the real `ensureServer()` reuses our pre-spawned isolated kilo
instead of binding to whatever's on the default port.
3. A minimal `Frontend` object — same shape as TelegramFrontend /
DiscordFrontend / TeamsFrontend, just wired to a Gateway +
RecordingHandler instead of an external chat platform.
To make `KILO_PORT`/`OPENCODE_PORT` overridable, both server modules
now read them from env at module load time, defaulting to the
production values when unset. Backward compatible.
Isolation that matters most: the spawned `kilo serve` runs with
`cwd` set to a fresh tempdir + HOME / XDG_DATA_HOME / XDG_CONFIG_HOME
all redirected inside that tempdir + `--pure` to skip external
plugins. Without this, the test kilo reads prod kilo's persisted MCP
catalog (`talon-tools-dream`, every `talon-tools-<chatId>` from prod
sessions) and the model in our test sees + cheerfully calls those
tools, routing bridge calls to the wrong context. Caught + fixed.
Dream pre-seeded as well: `maybeStartDream()` (called from
dispatcher.execute) would otherwise fire on first dispatch and its
`runOneShotAgent` registers `talon-tools-dream`, racing the chat MCP.
What runs unmocked:
- `bootstrap()` — config load, env vars, plugin load, workspace
init, storage load, daily-log cleanup.
- `initBackendAndDispatcher()` — backend factory registration +
lookup, `factory.init(config, ctx)`, dispatcher init, pulse,
cron, triggers, dream, heartbeat.
- `ensureServer()` — real Kilo SDK reuse path, picks up the
pre-spawned isolated kilo on TEST_PORT.
- `dispatcher.execute()` — real per-chat serial chain, real
`handleMessage` against the real spawned `kilo serve`.
Coverage: a single tool/text-delivery smoke test for now. Cross-chat
MCP isolation + synthetic output-cap path queued as follow-ups.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`incrementCounter(\`tool_calls.${tool.name}\`)` was using the raw
MCP-namespaced tool name, so each per-chat MCP server's tools landed
as a distinct metric label. Live example:
tool_calls talon-tools--1001426819337_react 1
That makes aggregation impossible — `react` from chat A and `react`
from chat B count as different metrics. The dashboard fragments by
chatID instead of summarising tool usage.
Strip via the existing `stripMcpPrefix()` helper, which already
handles both Claude SDK's `mcp__server__tool` form and Kilo's
`<server>_<tool>` form. After the fix:
tool_calls react N
Both backends fixed (claude-sdk + kilo). Restart the bot to start
collecting clean metrics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related live bugs reported on the running bot:
1. Doubled message — model called e.g. `send(type="photo")` (or any
non-text `send`), the bridge shipped the photo, then the model
emitted a follow-up text-part containing chain-of-thought commentary
that landed as a SECOND visible message. Talon's existing
text-part suppression only fired when `deliveredTextNorms` was
non-empty, but `captureDeliveredText` (in shared/delivered-text.ts)
only tracks `end_turn(text=...)` and `send(type="text", text=...)`
for dedup-by-substring purposes — not photo/poll/voice/sticker
sends. Add a separate `state.hadBridgeDelivery` boolean set true on
ANY `end_turn(...)` or `send(...)`. The kilo handler now suppresses
text-part on either signal.
2. Tool counter parity — kilo only incremented `tool_calls.<name>`
when a *terminator* fired, so `/metrics` only ever showed end_turn /
send / react. Claude SDK counts every tool. Move the counter
increment from kilo's handler-level terminator branch into
`processPartUpdate` (events.ts) so every tool the model calls is
counted, matching claude-sdk semantics. Bare-name normalization
(`stripMcpPrefix`) is preserved.
Tests:
- shared-stream-state: 6 new cases covering hadBridgeDelivery
behavior — flips for end_turn / send-of-any-type / MCP-prefixed
send, stays false for react / read_history.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Builds out the real-bootstrap integration suite in two directions:
1. Kilo gets a second test — `chat-switch disconnects the previous
chat's MCP server`. Run a turn for chat A, then chat B; assert the
cache contains chat B but not chat A after the switch (heartbeat
sentinel exempt). Catches regressions in `ensureChatMcpServer`'s
chat-rotation logic against a live `kilo serve`.
2. Adds `opencode-real-bootstrap.test.ts` — same shape as the kilo
suite, real `opencode serve`, real `bootstrap()` +
`initBackendAndDispatcher()`. Two tests:
- `real bootstrap delivers a response` (wiring smoke)
- `per-turn MCP teardown` — opencode's isolation model differs
from kilo's: it disconnects the chat MCP in handleMessage's
`finally` block rather than via chat-switch. The test asserts
the per-turn invariant (cache empty of `talon-tools-<chatId>_*`
after every turn, heartbeat exempt).
3. Both backends export `getRegisteredMcpServerNames()` — test-only
accessor for the module-private `registeredMcpServers` Set.
`GET /mcp` returns `{}` regardless of state on both servers, so
the test can't query the daemon directly.
Synthetic output-cap path is `it.skip` in the kilo suite with a
comment explaining why we can't reliably trigger it from a real
upstream model — the synthetic-handling code path is covered at unit
level by kilo-events.test.ts.
Both suites run against the live free-tier model the catalog returns,
under the same env-var override + cwd/HOME/XDG_* isolation strategy
as the kilo suite. Total runtime ~36s for both.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After a Talon restart, kilo/opencode serve still has the previous process's `talon-tools-<chatId>` MCP servers registered (their state lives outside our process). The new Talon's `registeredMcpServers` cache is empty, so the chat-switch disconnect logic in `ensureChatMcpServer` doesn't find anything to clear — and the model in a brand-new chat sees stale per-chat tools from prior sessions. Live observation that triggered this: the model in chat 352042062 emitted "I can see talon-tools-352042062_react and talon-tools--1001426819337_react in the available list" right after a Talon restart. Both chat MCPs were still registered with kilo from the previous process. Fix: at startup (inside the existing `prewarmPluginMcpServers` cycle, before plugin registration runs), call `oc.tool.ids()` to discover which `talon-tools-*` tool ids exist, derive the per-chat MCP server name from each (strip the bare-tool suffix via `stripMcpPrefix`), and disconnect each. Heartbeat sentinel (`talon-tools-heartbeat`) is exempt — heartbeat re-registers it lazily. Also fix package.functional test failure when a co-tenant Talon daemon is running on the developer's machine: `talon status` was hitting the prod gateway port 19876 and reporting it as the test's own running instance. Make `HEALTH_URL` env-overridable (`TALON_HEALTH_PORT`); the test sets it to a port nothing's listening on so status correctly reports "Stopped". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…SDK)
Kilo and OpenCode were calling oc.mcp.add() with the raw command array
directly. Claude SDK has always wrapped its MCP servers via
wrapMcpServer() — they run under src/util/mcp-launcher.mjs which
supervises the child's lifecycle. Bringing kilo/opencode into the same
discipline.
Two reinforcing kill paths in the launcher:
1. stdin EOF (existing behavior) — when the SDK-side pipe closes,
SIGTERM the child, then SIGKILL after 1s. Catches the in-process
Claude SDK exit case.
2. NEW: TALON_BRIDGE_URL/health watchdog — when set on env, the
launcher pings the bridge every 15s. After 4 consecutive failures
(~1 minute), SIGTERM the child. Catches the kilo/opencode-serve-
outlives-Talon case: those daemons stay running across Talon
restarts and keep our stdin open, so EOF alone never fires. Once
the child dies, kilo/opencode notice the stdio close on the next
interaction and drop the MCP registration.
This replaces the `disconnectOrphanChatMcpServers` startup helper from
the previous commit — that was a defensive sweep that the launcher's
self-termination now makes redundant. Same observable behavior (no
stale `talon-tools-<otherChatId>_*` after restart) via a more proper
mechanism.
Implementation:
- mcp-launcher.ts: add wrapMcpCommand(["cmd", ...args]) for the
single-array shape the kilo + opencode SDKs use, alongside the
existing wrapMcpServer({command, args}) for Claude SDK.
- mcp-launcher.mjs: TALON_BRIDGE_URL ping with stagger to avoid
thundering-herd at restart; gracefully no-ops when env var unset.
- kilo/server.ts: ensureChatMcpServer + ensurePluginMcpServers run
`command` through wrapMcpCommand. Drops the orphan-cleanup helper.
- opencode/server.ts: same treatment, parity.
Tests:
- mcp-launcher.test.ts: wrapMcpCommand asserts node + launcher
prepended to the array; throws on empty input.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's "Code Quality > Check for .only/.skip" rejects any `.skip(` in test files. The synthetic-output-cap test in kilo-real-bootstrap was an `it.skip` placeholder explaining why the live coverage isn't runnable (requires Kilo debug-inject endpoint we don't have). Move the rationale into a comment block — the unit-level coverage in kilo-events / kilo-server already exercises the synthetic-handling code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three test files were untouched by prettier on the way in. Re-run fixed them; CI's Format-check step rejected the original. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…red"
`finalizeExit` was calling `stream.end()` (async flush) immediately
after writing the exit footer, then proceeding to set the trigger
status to "fired". Callers that observe `status === "fired"` and then
read the log file with `readFileSync` could see only `--- spawn …`
because the footer was still buffered.
Caught as a macOS-only CI flake on the existing
`writes interleaved stdout + stderr to the log file` test:
expected '--- spawn 2026-05-15T21:38:15.893Z pi…'
to match /--- exit code=0/
Linux's typical pipe buffering happened to flush in time; macOS's
didn't. Awaiting the flush callback makes the contract explicit:
status === "fired" implies the log is on disk.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claudiusthebot
force-pushed
the
fix/kilo-strict-delivery-and-logging
branch
from
May 15, 2026 21:43
efafdc5 to
2d0143a
Compare
claudiusthebot
added a commit
that referenced
this pull request
May 15, 2026
The README hadn't kept pace with what landed across PRs #96, #160, #161, #165, #169, #170, and #172: - Kilo and OpenCode backends were missing or misrepresented (the badge still said "Claude Agent SDK", the backend config row listed only claude/opencode, the architecture tree didn't mention kilo, remote-server, or shared). - Discord frontend (PR #160) was absent from every list. - Triggers (PR #96) were absent from the features table. - Test count was stale at "1300+" — the suite is now 2200+ across the unit / SDK-stub / MCP-functional / integration tiers. - Prerequisites assumed a single backend (Claude CLI on PATH). Changes: - New "Backends" section explaining the three options + their transport shape + shared remote-server infrastructure. - Backends badge replaces the Claude Agent SDK badge. - Features table: dedicated "Pluggable backend" row, new "Triggers" row, MCP tools row mentions triggers. - Architecture tree refreshed: backend/registry.ts, backend/shared/, backend/remote-server/, kilo/, plus discord/ under frontend. - Backend-specific prerequisites called out under Quick Start. - Dependency rule paragraph mentions the QueryBackend interface. - Config table: backend accepts claude/kilo/opencode, frontend accepts discord, model description is backend-agnostic. - Development: test count updated to 2200+ across the tier matrix, added `npm run format`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Started as a logging/system-prompt clean-up; ended up reworking most of the Kilo (and OpenCode) chat flow after observing live behaviour against
kilo serveon real chats. The branch landed 20 fixes; the structural ones:events=noneevery turn)delivery: empty)stripMcpPrefixlearns Kilo's<server>_<tool>formend_turn/sendcalls weren't recognised →terminator=no delivered=0even when the model behaved + duplicate messagesoc.mcp.addcalls per turnsynthetic: truetext-part peel-offkilo/model-id prefixModel not found: opencode/kilo/deepseek/deepseek-v4-flash:freeevery hoursession.errorto our sessionIdonStreamDeltadelivery: text-part | tool | synthetic-error | empty (N chars)per turn + SSE event-type countssession.promptAsync+ SSE-driven loopsession.promptblocked on Kilo's HTTP POST until the upstream model finished. Free providers stalled the POST for 5+ minutes; Talon'sawaitwas unrecoverable. Now the await is on SSE iteration we ownMessageAbortedErrorreact/end_turnfired we calledoc.session.abort()to short-circuit Kilo's wrap-up; the resultingsession.errorwas being shipped to the user as⚠️ Kilo: MessageAbortedErrortoolsmap. Allow this chat's MCP tools, deny other chats' tools, allow built-ins soread/bashdon't hang onpermission.askedtalon-tools-<chatB>_send.ensureChatMcpServernow disconnects every othertalon-tools-*server (heartbeat exempt) before connecting oursBugs by file
src/backend/kilo/handler.tsevt.payloadin the SSE consumer (Kilo wraps every event as{payload: {type, properties}}).onStreamDeltato the SSE pipeline.tool/text-part/synthetic-error/empty. Each turn logs which branch fired.deliveredTextNorms.length > 0, drop the text-part wholesale.session.errorto oursessionIdbefore logging.MessageAbortedError(our own abort) from the synthetic-error path.session.prompt(sync, blocks on Kilo's HTTP POST) tosession.promptAsync+ await SSE close events.session.messagesafter SSE close.terminator/delivered/respLen/setup/turn/events.src/backend/kilo/events.tspartTypesmap forpartID → typedistinction (reasoning vs text both usefield: "text").finalizePartsIntoStatewalks parts as the source of truth — text parts go to response, reasoning is dropped, tool parts go throughrecordToolUse.terminator fired: <tool> text=Ncharswhen a delivery tool lands.src/backend/kilo/server.tsregisteredMcpServerscache (Kilo'sGET /mcpreturns{}regardless of state).parseStoredKiloModelSelectionstrips a leadingkilo/prefix and pinsproviderID = "kilo".KILO_SYSTEM_PROMPT_SUFFIXto match what the handler actually accepts.addcalls with elapsed-ms; flag slower than 1s as[slow].ensureChatMcpServerdisconnects every othertalon-tools-*server (heartbeat exempt) before connecting.src/backend/kilo/sessions.tsextractPartsSummarypeelssynthetic: truetext parts into asyntheticErrorTextchannel.ignored: true(Kilo flags every real reply with it).src/backend/opencode/server.tssrc/core/tools/index.tsstripMcpPrefixextended to recognise Kilo's<server>_<tool>naming. Walks underscore boundaries from the right and matches against the registered tool catalog sotalon-tools-352042062_end_turnresolves toend_turn(notturn).src/backend/shared/stream-state.ts: addpartTypes,eventCounts,syntheticError.flow-violation.ts: drop the "private scratchpad" framing.Tests
kilo-events.test.ts— synthetic part filtering,ignored: truedeliver-anyway, reasoning/text classification, finalize-as-source-of-truth.kilo-server.test.ts— local MCP cache,kilo/prefix stripping, per-session permission ruleset, chat-switch disconnect, heartbeat exempt from disconnect.end-turn.test.ts—stripMcpPrefixhandles bothmcp__server__toolAND<server>_<tool>forms.shared-flow-violation.test.ts— updated reminder-text assertions.Test plan
npx tsc --noEmitcleannpx vitest run— 2195 pass; one pre-existing failure (package.functional.test.ts > source CLI status command) reproduces on cleanmainand is unrelated.npm run lint/npm run format:checkcleanstepfun/step-3.5-flash:free— single message per reply,delivery: text-partanddelivery: toolboth fire,terminator=yes delivered=Nshows when the model usesend_turn, group chats now route reactions correctly (model in group callstalon-tools-<groupId>_reactnottalon-tools-<dmId>_react).talon-bootstrap.tsfor claude-sdk) — substantial framework, planned as a follow-up PR.Known follow-ups (not in this PR)
talon-bootstrap.ts-style fixture. Building a stub Kilo HTTP server (or wiring realkilo serveinto the bootstrap) is meaningful work for a follow-up.⚠️ Kilo: ...prefix when synthetic).🤖 Generated with Claude Code