fix: director inheritance, copilot streaming, agent DM UX, and audit fixes - #4
Merged
Conversation
…word dupes) Director silently dead (lead → director messages dropped): - An unconfigured director role fell back to the global default runtime (codex), which may not be installed. Director now inherits the Lead's runtime/model unless explicitly configured (new ModelConfig.hasRoleConfig, applied in both gateway registration paths and re-read in spawnDirector). - AcpAdapter.spawn now fails fast when the runtime binary is missing — previously the ENOENT arrived async after spawn() "succeeded", leaving a dead session that dropped every steer. - Director spawn failures are surfaced to the user's chat as a system message (LeadManager.onSystemNotice, wired to persist + ws broadcast in wireWsToLead) instead of being swallowed by empty catch blocks. - Lead/Director spawns now actually pass the configured model to the adapter (previously model was only persisted for display). Copilot SDK streaming: - Main chat showed no streaming: copilot sessions never invoked the ACP-style onOutputChunk that LeadManager wires for chat:stream. The adapter now synthesizes SessionUpdate chunks (message/thought/tool_call) and emits them on the session, so the existing lead pipeline works. - Agents page repeated words: the SDK emits both incremental assistant.message_delta events AND a full assistant.message at turn end; both were broadcast as deltas. The full-content event is now dropped when deltas were streamed this turn (tracked per session, reset at session.idle), while non-streaming turns still pass it through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Agent↔agent messages were effectively invisible and unattributed in the
web UI, three stacked causes:
- GET /messages unconditionally filtered out every dm: channel message,
so agent DMs never reached the main chat (new include_agent_dms param,
only admits authorType=agent DMs to keep user steers out)
- the recipient indicator checked msg.channelId, but DM recipients are
stored in msg.channel ('dm:<agentId>') — wrong field, never rendered
- live dm:message ws events were never ingested by the chat hook
New display config: agentMessages ('off' | 'summary' | 'detail',
default 'summary') in shared DisplayConfig, presets, merge + validation.
- summary: consecutive agent DMs collapse into one expandable line
("worker-x → director · N agent messages") so the main chat stays
readable by default
- detail: full bubbles, each showing sender → recipient
- off: hidden entirely (and not fetched)
Settings popover gets an "Agent messages" visibility selector.
Also: HTTP-level regression test pinning the model-selection roundtrip
(PUT /agents/:id/model visible in next GET /agents).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
From a 6-dimension audit (40 agents, adversarially verified — 15 confirmed of 34 raw findings), fixing the low-risk high-value ones: - CopilotSdkAdapter.steer(): every call leaked a session.on handler (and the 5-min timeout left another) — unsubscribe on idle/timeout. N steers used to mean N live handlers firing on every subsequent event. - LeadManager.resumeLead/resumeDirector: insertAgent on an existing agent row hit the primary key, so resumes of persisted leads/directors always failed into "marking offline". Upsert instead. resumeDirector also never passed runtime, routing copilot-sdk directors to the ACP adapter. - DecisionLog.readAll(): one corrupted JSONL line crashed every route that lists decisions — skip bad lines (matches SuggestionStore behavior). - useAgents: dmMessagesRef survived project switches — unbounded memory growth across projects; clear it with the other per-project state. - HTTP DM path now stores recipient (was only derivable from channel). - drizzle schema: read_state composite PK (matches sql/schema.sql). - web ws types: declare the project field the server actually sends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remaining verified audit findings (items 2-5): - purgeOfflineAgents now skips hibernated agents that are still resumable (saved acp_session_id) or have in-flight tasks assigned — purging those orphaned tasks and lost resumable sessions on every Lead spawn. - DM channel naming unified on 'dm:<recipient>' + recipient column. Three formats coexisted (bare 'dm'+recipient via appendDM, 'dm:<id>' without recipient via AgentManager steers, 'dm:<id>'+recipient via HTTP), so no single query could see all DMs — and bare-'dm' rows leaked past the main-chat dm: filter unattributed. Idempotent migration in SqliteStore normalizes existing rows; getUnreadDMs tolerates legacy shapes; web recipient arrow reads channel/recipient/channelId. - MultiAdapter sessionAdapterMap is now cleaned when sessions end naturally (crash/exit), not just on kill() — transparent onSessionEnd property hook that preserves gateway-assigned handlers. - CopilotSdkAdapter spawn/resume no longer stack session.on handlers when the same session is re-registered; disposer stored per session and released on kill. - Deflake messagelog markRead test (ms-granularity boundary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness and UX issues across the gateway, server, and web UI: fixing Director runtime/model inheritance and visibility of spawn failures, adding proper Copilot SDK streaming integration + deduplication, improving agent↔agent DM handling/UX, and applying a set of audited reliability fixes (session cleanup, resume upserts, DM normalization, safer decision-log parsing).
Changes:
- Director now inherits Lead runtime/model by default, spawn failures are surfaced to the user, and ACP spawns fail fast when the runtime binary is missing.
- Copilot SDK sessions now drive the existing streaming pipeline (ACP-style chunks) and avoid duplicated final messages; event handler leaks are disposed.
- Agent↔agent DMs are normalized (storage + API), can be included in main chat with configurable visibility (off/summary/detail), and are grouped/collapsed in the UI.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/pages/Chat.tsx | Adds DM recipient display + collapsible grouping for consecutive agent↔agent DMs in main chat. |
| packages/web/src/lib/ws.ts | Updates WS event typing for project-scoped chat/task events. |
| packages/web/src/lib/types.ts | Extends ChatMessage with channel and recipient for DM routing. |
| packages/web/src/lib/api.ts | Adds include_agent_dms support to message fetching API call. |
| packages/web/src/hooks/useChat.tsx | Loads/filters agent messages based on display config; ingests live dm:message WS events. |
| packages/web/src/hooks/useAgents.tsx | Clears DM state on project switch to avoid memory growth. |
| packages/web/src/components/DisplaySettings.tsx | Adds “Agent messages” visibility setting (off/summary/detail). |
| packages/shared/src/display.ts | Adds agentMessages to DisplayConfig + presets/validation/merge defaults. |
| packages/server/tests/storage/sqlite.test.ts | Tests DM migration normalization to canonical dm:<recipient> format. |
| packages/server/tests/storage/messagelog.test.ts | Deflakes markRead timing test by avoiding same-ms boundary. |
| packages/server/tests/orchestrator/suspended.test.ts | Adds coverage for purgeOfflineAgents skip rules (resumable/in-flight). |
| packages/server/tests/lead/lead-manager.test.ts | Adds director inheritance + spawn failure surfacing tests. |
| packages/server/tests/comms/message-store.test.ts | Tests canonical DM writes and legacy DM read compatibility. |
| packages/server/tests/api/model-roundtrip.test.ts | Adds HTTP regression for agent model/runtime persistence roundtrip. |
| packages/server/tests/agents/multi-adapter.test.ts | Tests sessionAdapterMap cleanup on natural session end + handler preservation. |
| packages/server/tests/agents/model-config.test.ts | Tests hasRoleConfig behavior for explicit vs fallback config. |
| packages/server/tests/agents/copilot-sdk-adapter.test.ts | Adds tests for stream dedup + ACP-style chunk synthesis + turn-boundary reset. |
| packages/server/tests/agents/adapters.test.ts | Tests ACP spawn fails fast when runtime binary missing. |
| packages/server/src/storage/SqliteStore.ts | Adds idempotent DM normalization migration; refines purgeOfflineAgents delete rules. |
| packages/server/src/storage/DecisionLog.ts | Skips corrupted JSONL lines instead of crashing decision listing. |
| packages/server/src/lead/LeadManager.ts | Implements director inheritance, model propagation, spawn failure notices, resume upserts, and better routing. |
| packages/server/src/db/schema.ts | Adds composite PK to read_state to match ON CONFLICT usage. |
| packages/server/src/comms/MessageStore.ts | Canonicalizes DM channel format and broadens unread DM query to include legacy rows. |
| packages/server/src/cli/gateway.ts | Wires director inheritance defaults + system notices + DM WS broadcast. |
| packages/server/src/api/routes/messages.ts | Adds include_agent_dms query param and writes DM recipient; filters DMs from main chat by default. |
| packages/server/src/agents/MultiAdapter.ts | Hooks onSessionEnd to clean up sessionAdapterMap on natural session end. |
| packages/server/src/agents/ModelConfig.ts | Adds hasRoleConfig() to distinguish explicit role config from fallbacks. |
| packages/server/src/agents/CopilotSdkAdapter.ts | Synthesizes ACP-style streaming chunks; dedupes full-message events; disposes stacked handlers on steer/spawn/resume/end. |
| packages/server/src/agents/AgentManager.ts | Ensures DM messages include canonical recipient fields. |
| packages/server/src/agents/AcpAdapter.ts | Adds runtime binary existence check to fail fast on missing commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Streaming dedup upgraded from drop-the-duplicate to replace-on-final: deltas stream live as before, but when the authoritative assistant.message arrives the UI swaps the accumulated text for it. This self-corrects any delta-level glitches — duplicated fragments inside the stream (seen with stacked event handlers before 31612b4, and possible upstream), dropped ws frames, out-of-order delivery — instead of trusting the accumulation. - CopilotSdkAdapter: after a streamed turn, assistant.message is forwarded as synthetic 'assistant.message_final' (previously dropped) - mapper/gateway: message_final broadcasts agent:stream with replace: true - web useAgents: streamed text chunks are marked ephemeral and the per-turn output offset is tracked; a replace event swaps them for the final text while preserving interleaved tool/thinking chunks - ACP runtimes are unaffected (never send replace); main chat already had these semantics via the persisted chat:message Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- GET /messages: listMessages returns newest-first, but slice(-limit) kept the OLDEST entries of the fetched window — chats longer than `limit` never showed their most recent messages. Keep the newest `limit` and reverse to ascending. Also over-fetch (limit + 200) so DM filtering can't underfill the page. Route-level regression test pins newest-window + ascending order. - AgentDmGroup: pass the full visible message list to MessageBubble so reply previews resolve parents outside the collapsed group. - useChat: drop the `as any` on dm:message ingestion — use the typed event payload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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
Four commits covering two user-reported bugs, a UX feature, and 13 verified findings from a multi-agent repo audit (34 raw findings, each adversarially verified; 19 refuted).
Director silently dead + model not actually applied (
9565fed)directorrole fell back to the global default runtime (codex) — which may not be installed. Director now inherits the Lead's runtime/model unless explicitly configured.AcpAdapter.spawnfails fast when the runtime binary is missing (previously the ENOENT arrived async after spawn "succeeded", leaving a dead session that silently dropped every message).Copilot SDK streaming (
9565fed)onOutputChunkthat feedschat:stream. The adapter now synthesizes SessionUpdate chunks so the existing lead pipeline works.assistant.messageat turn end; both were broadcast. The redundant full-content event is now dropped when deltas were streamed (per-turn tracking).Agent DM sender→recipient display, collapsed by default (
a814b49)GET /messagesunconditionally dropped everydm:message; the recipient indicator read the wrong field (channelIdvschannel); livedm:messageevents were never ingested. All fixed.agentMessages(off | summary | detail, default summary): consecutive agent↔agent DMs collapse into one expandable line ("worker-x → director · N agent messages"); detail mode shows full bubbles with sender → recipient.Verified audit fixes (
6b48abe,31612b4)CopilotSdkAdapter.steer()leaked a session event handler per call (plus one per 5-min timeout); spawn/resume stacked handlers on re-registration — all disposed properly now.resumeLead/resumeDirectorhit a primary-key violation on the existing agent row, so resumes always failed into "marking offline"; now upsert.resumeDirectoralso never passedruntime, routing copilot directors to the ACP adapter.purgeOfflineAgentsdeleted hibernated agents that were still resumable or had in-flight tasks; now skips both.dm:<recipient>+recipientcolumn (three formats coexisted; bare-dmrows leaked into main chat unattributed). Idempotent migration normalizes existing rows.MultiAdapter.sessionAdapterMapnow cleaned on natural session end, not justkill().DecisionLog.readAll()no longer crashes every decision-listing route on one corrupted JSONL line.dmMessagesRefcleared on project switch (unbounded memory growth); ws type contract fixes;read_statecomposite PK in drizzle schema.Known remaining (not in this PR)
thread_idbut the server has no threads table/handler — tracked separately as feature work.Test plan
tsc --noEmitclean on shared/server; webvite buildpassesmessagelogmarkRead test (ms-granularity race, ~50% failure → 6/6 stable)🤖 Generated with Claude Code