Lessons learned from multiple rounds of agent architecture refactoring where critical issues were repeatedly missed despite "thorough" audits.
Every audit MUST cover all 10 layers. Previous failures came from only covering layers 1-3, then layers 1-7 (missing wire protocol and init parity), then layers 1-9 (missing resolver symmetry).
Method: Pick every domain term and search ALL usages. Build a table:
Common overloaded terms: gateway, session, channel, provider, context, runtime, config, state, manager, handler, bridge, proxy, client.
Read the code as if you've never seen the codebase. For each function/struct:
Run every checklist item. If any fails, the refactor is not complete.
-
Never declare "final" in a plan name — there's always more. Use descriptive names.
-
Build term overloading table FIRST — before any plan, map every domain term to all usages.
-
Trace full call path — from frontend -> Tauri command -> core -> variant code. Issues hide at boundaries.
-
Check default branches — for every enum match, verify _ is intentional and correct.
-
Question "shared" modules — if a "shared" module references specific variants, it's not truly shared.
-
Read adjacent systems — auditing agent definitions? Also audit providers, sessions, tools.
-
Ask "what happens when someone adds a new variant?" — if adding AgentVariant::Research breaks things silently, fix now.
-
Sweep the class, not the instance — when you find a bug, define its category, grep the entire scope, fix all hits. Never fix one and move on.
-
Dump and inspect wire payloads — for any code that sends data to an external service, serialize and inspect the actual output. Source code is not enough.
-
Compare all entry points — build a matrix of (entry point) x (init steps). Missing cells are bugs.
-
Check resolver symmetry — when a function resolves N fields from the same source chain, build a (field) x (source) matrix. Every field should check every source. Asymmetry is a latent bug.
-
Match flag dimension to decision dimension — when a boolean flag drives an if/else, ask: "does this flag's semantic axis match the decision being made?" is_channel (message source) branching on workspace path (agent type) is a dimension mismatch.
-
Consolidate repeated lookups — when state.get_session(&id).await (or any map/lock lookup) appears N times in one function, consolidate into one lookup and extract all needed fields. Each extra lookup is a wasted lock acquisition and a readability tax.
-
Eliminate guaranteed-Some Option wrappers — when a value is produced by an ok_or / ok_or_else (guaranteed non-None), do NOT wrap it in Option just to match a legacy if let Some(ref x) pattern downstream. The Option wrapper erases the guarantee and forces defensive code throughout.
-
Prefer single-pass set derivation over multi-layer mutation — when building a set of items to include/exclude, write a single .iter().filter().collect() with all conditions in the filter predicate. Do NOT build a mutable set and add/remove across multiple passes/layers. The single-pass version is easier to read, harder to break, and eliminates the need for in-file constant patches when the tool catalog evolves.
-
Count sources of truth for "is the queue allowed to flush?" — before finalizing any queue or lifecycle design, list every atom, boolean, and condition that the dispatcher checks before deciding to send. If the count is >1, reduce to exactly 1 by introducing a single FSM phase field. Every other signal becomes a UI mirror or a FSM input, not a decision gate.
-
Add generation counters to every async start/stop protocol — any time a turn, task, or job can start and stop multiple times in a session, and signals can arrive asynchronously, add a monotonically increasing integer generation to every start call. All terminal signals must carry the generation they belong to, and the handler must discard signals whose generation does not match.
-
Audit cancel APIs for postcondition symmetry before implementation — before writing a cancel function, list its postconditions (draft restore? mark interrupted? poison next context?). If two callers have different postconditions, the function must accept an intent parameter or be split into two functions. Never rely on frontend call timing or flag-reset order to differentiate cancel semantics.
-
Delete shadow boolean atoms that replicate FSM phase — when a boolean like holdForStop or isRunning is added "for safety" alongside an FSM, it almost always duplicates an FSM phase. Find the phase it corresponds to, route writers through the FSM transition, and delete the boolean. Having both guarantees they will diverge under race conditions.
-
Trace every event handler that directly sets runtime status — for every handler that writes isRunning, runtimeStatus, or equivalent "turn active" atoms in response to a provider event, ask: "what happens if this event arrives late, out of order, or not at all?" If the answer is "the UI freezes" or "a new turn is reset to idle," route it through the FSM with generation-checking and deadman timers instead.
-
Enumerate all send-path call sites before shipping a queue — before a queue dispatch system is considered complete, grep every call to the backend transport layer (dispatchMessageBySessionType, sendMessage, etc.). If >1 call site can fire for the same logical user action, there is a duplicate path. All UI controls must signal intent to the queue state machine; only the dispatcher calls the transport.
-
Split multi-purpose atoms before they compound — any atom whose name uses a conjunction (e.g. userInitiatedCancelAtom doing "mark stop episode open" AND "gate draft restoration") will cause cross-concern bleed when either concern needs to be cleared independently. At design time, name each concern separately and write one atom per concern.
-
Name fields by their purpose, not their mechanism — disabled_tools / allowed_tools describe the mechanism (deny/allow) but not the intent (user exclusion delta / subagent strict subset). Use excluded_tools ("tools the user/definition removed from the default set") and restrict_to_tools ("if non-empty, only these tools are available"). A new developer should be able to read the field name and understand why the list exists without reading the surrounding code.
-
Separate per-turn data from app-level infrastructure in request structs — if a "per-request" struct contains fields that every single caller sets to the same app-level singleton value, those fields belong on a higher-scoped parameter (e.g. a separate app_handle argument) — not on the per-request struct. The struct should only contain data that genuinely varies per invocation. When app-level resources are needed inside the callee, derive them from the infrastructure handle via small extractors.
-
Eliminate derivable constructor parameters — when a constructor parameter is a pure function of other parameters already being passed, compute it inside the constructor body. External derivation adds maintenance burden and risks divergence when the logic is updated in one call site but not others.
-
Audit config struct field cohesion — for every config struct, ask: "do all fields describe the same concern?" If embedding settings, sub-agent toggles, and LLM overrides coexist in one struct, it needs splitting. One struct = one domain. Name each domain explicitly; find the right owner in the architecture (global config, per-agent definition, session record).
-
Background subsystems must not call session-startup resolvers — ResolvedAgent::resolve(), session init, and similar startup paths often enforce invariants (model present, account configured) that are valid at session-start but not in background/offline contexts. Background jobs (reflection, consolidation, cleanup) should have their own lightweight config accessors that read only what they need.
-
Verify fallback paths for the same failure mode as the primary — before writing primary().unwrap_or_else(|| fallback().expect("always works")), ask: "can fallback() fail for the same reason primary() failed?" If the answer is yes, the .expect() is a latent panic. The fallback must handle the failure case, not assume it cannot occur.
-
"Keep a deprecated snapshot field for convenience" (Added 2026-04) — SessionRuntime.project_path: Option<PathBuf> was a snapshot set at initialization; the live source of truth was workspace_state: Arc<RwLock<SessionWorkspace>> (updated by /add-dir, /rm-dir). The snapshot was never updated, so any mutation made it stale. Four consumers read the snapshot; all could read workspace_state.read().tool_cwd() instead. When a mutable, canonical data source exists (e.g. workspace_state), do NOT also store a snapshot of the same data on the same struct. The snapshot will inevitably drift, and every reader faces a hidden correctness choice between the two sources.
-
"Clone an owned value into a struct constructor instead of moving it" (Added 2026-04) — resolved: resolved.clone() when resolved is an owned ResolvedAgent that is never used after the constructor. The .clone() on a large struct (with nested Vec, HashMap, Arc fields) wastes CPU and memory for zero benefit. Before writing .clone(), check: is the variable used after this point? If not, move it. Same applies to any owned value being assembled into a struct: integrations.clone(), overrides.clone(), log_prefix.clone() — if it's the last use site, move it.
-
"Pass a bare path where a structured workspace type exists" (Added 2026-04) — SessionRuntimeRequest accepted workspace_path: PathBuf. The callee immediately wrapped it in SessionWorkspace::new(path) to get the structured workspace type with project_root, original_cwd, and additional_directories. Worse, a SECOND SessionWorkspace::new(path.clone()) was constructed elsewhere in the same function for AgentToolConfig, creating two independent workspace objects from the same path — a split-brain if additional_directories ever differed. Fix: rename to project_root: PathBuf (semantically precise for what the callee receives) and use the canonical Arc<RwLock<SessionWorkspace>> returned by the factory for all downstream consumers. When a structured type exists for a concept, pass it (or its constituent fields with precise names), not a bare primitive that forces the receiver to re-derive the structure.
-
"Relay struct that mirrors the destination struct field-for-field" (Added 2026-04) — SessionRuntimeRequest (28 fields) existed only to ferry values from init.rs to build_session_runtime, which immediately unpacked 20 of them into ToolDeps (the struct tool constructors actually consume). Three fields (mode_switch_manager, max_tokens, temperature) were packed into the request but never read by the callee — dead code. The struct added no abstraction, no validation, no transformation; it was pure mechanical relay with a .clone() tax on every field transition. Fix: the caller (init.rs) constructs ToolDeps directly and passes it to the factory alongside the small set of factory-specific params (model, account_id, disabled_tools, disabled_mcp_servers, policy_config, log_prefix). Detection rule: when > 60 % of a struct's fields appear verbatim (same name, same type, zero transformation) in the destination struct, the relay struct is overhead. Delete it and let the caller assemble the destination struct directly.
-
"Copy session-level fields into a per-turn request struct via .with_*() chains" (Added 2026-04) — message.rs called UnifiedRequest::new() then apply_standard_config() + 15 .with_*() builder calls to copy fields from SessionRuntime → UnifiedRequest every turn. The same fields (model, max*tokens, temperature, skills, workspace_state, etc.) were set identically for every turn in the same session. Fix: UnifiedRequest::from_runtime(&SessionRuntime) constructor reads all session-level fields from the runtime in a SINGLE place; callers only add per-turn fields (mode, images, cancel_flag, etc.). Detection rule: when > 50 % of a builder's .with*\*()calls are setting values that come from a single source struct and never change across calls, the builder needs afrom_source()constructor. Corollary — workspace split-brain:process_messageconstructed a NEWSessionWorkspace::new(path)fromrequest.project_path+ DB hydration even thoughrequest.workspace_statealready held the identical, fully-hydrated workspace frominit.rs. Fix: prefer workspace_state.read().clone() when available; fall back to DB reconstruction only for callers without a workspace_state (gateway path).
-
"Pass app-level singletons through per-turn input structs" (Added 2026-04) — TurnInput carried infrastructure handles (app_handle, lsp_manager, screenshot_store, project_path) that were app-level singletons or derivable from SessionRuntime. Every caller had to extract these from AgentAppState and pack them into TurnInput; process_message then unpacked them to build EventHandlerConfig and UnifiedMessageProcessor. The singletons never varied per-turn — they were constant for the app's lifetime. Fix: process_message accepts app_handle: Option<tauri::AppHandle> as a separate parameter and derives infrastructure handles internally via small extractors (extract_lsp_manager, extract_screenshot_store). TurnInput is reduced to only per-turn variable data (content, mode, images, ide_context, is_resume, channel, chat_id). Detection rule: if a per-turn/per-request struct field is set to the same value by every single caller and the value comes from a global/app-level source, it belongs on a higher-scoped parameter — not on the per-turn struct. Corollary — inconsistent fallback: when TurnInput.screenshot_store was None, process_message created a NEW empty ScreenshotStore instead of using the global one from AgentAppState. The extractor pattern fixes this: extract_screenshot_store always returns AgentAppState.screenshot_store when available, falling back to an empty store only when no app handle exists (test/public endpoints).
-
"Pass a derived value as a constructor parameter when the constructor already holds the source" (Added 2026-04) — process_message computed agent_id = runtime.agent_definition_id.unwrap_or(session.id) then passed it to UnifiedMessageProcessor::new(runtime, session, ..., agent_id, ...). The processor already held both runtime and session as fields, so agent_id was fully derivable inside the constructor. Passing it externally added a parameter, created a maintenance burden (callers must remember the derivation logic), and risked divergence if the logic were updated in one place but not the other. Fix: compute agent_id inside the constructor body from the already-available runtime and session fields. Detection rule: when a constructor parameter is a pure function of other parameters already being passed to the same constructor, eliminate it and compute inside.
-
"Grep-alive = alive" — reference counting is not a dead code audit (Added 2026-04) — UnifiedSession (253 lines, 10 builder methods) appeared "alive" because grep found 15+ hits — definition, re-export chain (types/session.rs → types/mod.rs → session/mod.rs), from_session() / to_session() conversion methods, and a test calling those methods. But tracing from actual business entry points (Tauri commands, gateway handlers) revealed that no production code path ever constructed or consumed UnifiedSession. The runtime used AgentSession + SessionRuntime; the DB layer used UnifiedSessionRecord directly. The entire type hierarchy existed only to service a single round-trip test. Detection rule: when auditing whether a type is alive, trace from business entry points forward — not from the type's definition outward. A type that only appears in its own definition file, re-export chains, internal conversion methods, and tests is dead regardless of grep hit count. Corollary: "aspirational abstraction" — builder methods (with_label, with_channel, with_project_path) with zero callers are a strong signal the type was designed for a future that never materialized.
-
"Same name, different struct" — cross-module naming collision (Added 2026-04) — Two SessionFilter structs existed in different modules with completely different fields: one in types/filter.rs (6 fields: type_name, status, channel, project_path_prefix, limit, offset) for DB queries, and another in unified_stats/types.rs (9 fields: category, status, key_source, repo_path, text_query, sort_by, sort_order, limit, offset) for frontend API. In aggregation.rs, both appeared in the same file distinguished only by full path (crate::agent_core::session::SessionFilter vs local SessionFilter). A new developer cannot tell which is which without reading both definitions. Fix: rename to make the domain explicit — SessionListFilter (persistence layer) vs SessionFilter (frontend API). Detection rule: after any audit, grep every exported type name across the entire codebase. If the same name appears in >1 module with different field sets, rename the narrower-scoped one.
-
"Overloaded config struct — one name, three unrelated domains" (Added 2026-04) — MemorySearchConfig simultaneously held: (a) embedding engine settings (provider, model, max_chunks, chunk_size), (b) per-agent L3 learnings policy (learnings_enabled, extract_memories_enabled, auto_dream_enabled), and (c) a consolidation model override (consolidation_model_override). The name suggested "memory retrieval tuning" but the struct actually controlled write-path sub-agents and LLM selection. The mismatch meant every new developer had to read the full struct to understand what each field actually did. Fix: split by domain — EmbeddingConfig (app-global embedding engine, lives in IntegrationsConfig) + AgentLearningsConfig (per-agent write-path policy, lives in AgentDefinition). Detection rule: when a config struct's fields span multiple distinct subsystems (storage, LLM selection, sub-agent toggles), it is doing too many jobs. Name each concern, find the right owner, split.
-
"Background subsystem over-couples to full session resolver" (Added 2026-04) — reflection.rs and active_learning.rs both called ResolvedAgent::resolve() just to read one boolean flag (learnings_enabled). But ResolvedAgent::resolve() enforces a strict invariant: selected_model_id must be present. Background subsystems run after a session ends, at which point the in-memory session is gone and the agent definition on disk may not have a model configured (OS agent never has one). Result: reflection-pipeline E2E silently failed with builtin:sde has no selected_model_id. Fix: introduce a lightweight resolve_learnings_for(agent_id) helper that uses resolver::resolve_definition (no model requirement) to extract only config flags. Background subsystems should call this, not ResolvedAgent::resolve(). Detection rule: any background/offline subsystem (consolidation, reflection, cleanup jobs) that calls ResolvedAgent::resolve() is a red flag — ask "does this code path actually need a model?" If not, use the definition resolver instead.
-
"expect() on a fallback path that can never succeed" (Added 2026-04) — GET /agent/config called ResolvedAgent::resolve() on the live OS agent definition, and on failure called .expect("fallback os_agent must resolve") on the compiled-in os_agent() builtin. But os_agent() has selected_model_id: None — the fallback had the same failure mode as the primary path. The .expect() was a guaranteed panic disguised as defensive code. Fix: the fallback must use a code path that actually handles the missing-model case — in this case AgentRuntimeView::from_definition() which reads learnings and embedding without requiring a model. Detection rule: whenever you see X.unwrap_or_else(|| Y.expect("Y must work")), ask "can Y actually fail for the same reason X failed?" If yes, the fallback is a lie.
-
"Session-layer decision misplaced in agent config layer" (Added 2026-04) — MemorySearchConfig.consolidation_model_override let agent definitions override which LLM model consolidation uses. But consolidation processes learnings from a specific past session, and that session already recorded the exact model and account used at runtime (agent_sessions.model, agent_sessions.account_id). The agent-config override could diverge from what the session actually used, creating inconsistency. More importantly: the session layer already owns this decision. Fix: delete the override field; consolidation always reads model/account from the session record. Detection rule: when a background job processes per-session data, its LLM/resource selection should come from the session record — not from the agent definition. The definition controls "how the agent behaves during a live session"; the session record captures "what was actually used." Post-session processing belongs to the session record's data.
-
"Broadcast-only success path" (Added 2026-05) — A Rust turn completed successfully and token usage proved the provider returned text, but the rendered chat showed only user events because the assistant message was only emitted through a transient agent:streaming_complete broadcast path. If the frontend listener misses, filters, or fails to upsert that broadcast, runtime truth and UI truth diverge. Fix: completed assistant output must be pushed to the authoritative EventStore from the runtime side; broadcasts may remain as notifications, but not as the sole persistence/visibility path. Detection rule: whenever an event changes durable UI history, trace whether the backend writes the canonical store directly or only emits a notification that another layer may or may not consume.
-
"Control action has two send/cancel dispatchers" (Added 2026-05) — Force Send appeared to work, but ChatView had a duplicate direct append + send path while useQueueDispatch already owned queued dispatch semantics. Double paths drifted on status, model selection, queue removal, cancel timing, and error handling. Fix: visible queue controls should only promote/cancel/request dispatch; exactly one dispatcher owns append, dequeue, model resolution, send, and error handling. Detection rule: for every control button, trace from click to side effect; more than one mutation path means split ownership. (See planning rule #27.)
-
"Cancel flag has one meaning for Stop and Force Send" (Added 2026-05) — User Stop and programmatic Force Send both called cancel, but their semantics differ: Stop should restore/rollback and may mark the next turn as interrupted; Force Send should cut the current turn without poisoning the follow-up turn or leaking [Request interrupted by user]. Sharing one cancel flag/default branch caused the next Force Send turn to be cancelled immediately or to inherit synthetic interruption text. Fix: cancel APIs must carry intent (user stop vs programmatic interrupt) and runtime code must branch explicitly. Detection rule: when two controls share a cancel path, compare postconditions; differing postconditions require separate entry points. (See planning rule #24.)
-
"Local row/card assertions equal orchestration success" (Added 2026-05) — Agent Org tests proved task rows, inbox rows, and overview cards existed, but a real run still stopped at 4/6 tasks with all sessions terminal and agent_org_runs.status = running. The assertions were local implementation milestones, not final product outcomes. Fix: every orchestration audit must define final user outcome, durable run/session/task invariants, rendered UI evidence, runtime path evidence, and anti-false-positive checks before implementation. Detection rule: if a test would pass with running run + terminal sessions + open/ownerless tasks, it is not an orchestration scenario test.
-
"One status dimension explains the whole UI" (Added 2026-05) — Task completion, member session liveness, member activity/intervention, and run finality are different dimensions. Mixing them makes a session look crossed-out/completed while the run says running, or hides abandoned work behind completed styling. Fix: store and render each dimension separately, and reconcile run finality from durable session/task state before projecting UI. Detection rule: every Agent Org/queue UI row should expose/assert the specific dimension it displays (run.status, sessionRuntime.status, task.status, owner/member activity), not infer one from another.
-
"Single-agent claim semantics copied into a multi-agent runtime" (Added 2026-05) — A task tool can safely infer "current agent owns this" in a single-agent todo list, but the same inference is wrong for a coordinator in a multi-agent org. status=in_progress without owner means different things depending on caller role: a member may self-claim; a coordinator must assign a member or leave work pending. Fix: make task-tool semantics role-aware at the tool boundary, before store invariants fire. Recoverable misuse should return structured guidance, not an execution failure. Detection rule: when a tool mutates shared multi-actor state, audit caller identity (coordinator, member_id, agent_id, session id) before copying behavior from a single-actor system.
-
"Live task-board context hidden in stable prompt cache" (Added 2026-05) — A prompt section can look correct in a dump but be stale during the next turn if it contains live DB state while marked StableUntilClear. For Agent Org, a stale task snapshot causes duplicate task_create, invalid task_update, and model confusion even when the store is correct. Fix: task board / inbox / member activity prompt sections must be Volatile or revision-keyed and tested through the real prompt cache policy matrix. Detection rule: any prompt text containing persisted runtime state must answer: what revision invalidates this, and does the cache policy enforce it?
-
"Clean UI while trajectory leaks tool errors" (Added 2026-05) — Rendered UI can pass while the hidden tool trajectory contains Error executing task_*, SQL constraint errors, or store invariant failures. The user still sees broken agent behavior because the model consumed those errors. Fix: orchestration E2E must assert negative trajectory patterns alongside positive UI and DB evidence. Common model-correction paths should return JSON guidance fields (guidance, already_exists, status_ignored, etc.) rather than exception-shaped text. Detection rule: after fixing a tool error, add an E2E assertion that the previous error string cannot appear in task-tool results or live conversation trajectory.
-
"Pre-user schema change disguised as migration" (Added 2026-05) — During the no-external-user stage, adding ALTER TABLE, legacy-table rebuilds, compatibility tests, or migration shims for a schema change creates dead compatibility logic and hides the real source of truth. Fix: update the canonical CREATE TABLE / initialization DDL directly, reset the affected local and isolated E2E databases, and verify the fresh schema path. Only write a real migration when the user explicitly says existing persisted data must be preserved. Detection rule: if a diff adds ALTER TABLE, PRAGMA table_info compatibility probes, legacy schema rebuild code, or tests named around old-table migration, stop and ask whether persisted-user compatibility is actually required.
-
"Debug helper marks state, test expects rendered product history" (Added 2026-05) — A rendered Agent Org test called a debug drain endpoint that used a throwaway session, marked inbox rows read, and returned rendered messages, then expected the real coordinator chat history to contain those messages. The helper proved helper-side state, not the production caller path. Fix: rendered E2E must drive the same production action a user would trigger (send_message_impl, button click, wake/resume path) before asserting chat history. Debug endpoints may seed prerequisites or inspect state, but cannot be the mutation path for the rendered behavior under test. Detection rule: if a test calls /test/*, debug*, or helper-isolation endpoints and then asserts visible chat/cards changed, trace whether the endpoint writes the same EventStore/session/UI state as production; if not, the test is invalid.
-
"Control/sentinel records registered as user-actionable UI state" (Added 2026-05) — File review polling treated redo:rewind snapshots as normal pending review entries. That re-registered a control snapshot into the pending-review registry and cleared the actual Redo anchor, disabling Redo All immediately after Undo. Similar leaks show up when protocol envelopes like <inbox-batch ...> become visible user text. Fix: every UI registry that consumes durable records must define an explicit inclusion predicate and exclude sentinel/control records (redo:*, internal batch envelopes, synthetic markers) unless the surface is an intentional diagnostic viewer. Detection rule: whenever a backend introduces a sentinel/prefix/control row, grep every generic list/registry consumer and add positive inclusion or explicit exclusion before shipping.
-
"New helper added without call-site sweep" (Added 2026-05) — A setTextarea E2E helper existed, but the Agent Org description field still used setInput, so the spec failed with input-missing. Adding the helper did not update semantically matching call sites. Fix: introducing a specialized helper requires a sweep of all selectors/components with the same DOM/runtime shape and updating call sites in the same change. Detection rule: if a helper name narrows a mechanism (setTextarea, selectOptionBySelector, drainInboxForFixture), grep for nearby generic helper calls (setInput, raw debug drain, generic click) against matching test IDs/components before declaring the E2E fixed.
-
"Split-brain turn finality — multiple independent 'is the turn done?' signals" (Added 2026-06) — The queue had four independent "is the session idle?" signals: runtimeStatus (derived from provider events), a separate isRunning boolean, queueFlushRequestAtom heuristics, and holdSessionQueueForStopAtom. Each defined "done" slightly differently. When they disagreed, the queue either refused to flush (frozen UI where the composer stuck in Stop state permanently) or flushed too early (next queued message sent before the previous turn fully closed, causing duplicate messages in the transcript). Any signal that can independently say "idle" is a split-brain candidate. Fix: introduce one canonical FSM (turnLifecycle.ts) with a single phase field. All other atoms (runtimeStatus, isRunning) become UI mirrors — they are derived from FSM transitions and cannot independently change the decision of whether to flush. The queue dispatcher reads exactly one gate: phase === "idle". Detection rule: count the atoms/booleans your queue dispatcher reads before deciding to send — any count >1 is split-brain; reduce to one named source.
-
"No generation counter — stale signals from old turns poison new turns" (Added 2026-06) — When a turn ended and a new one began immediately (e.g. Force Send into a queued follow-up), late-arriving terminal signals from the OLD turn (stream-end events that crossed the network after the new turn was already dispatching) would reset the NEW turn's FSM to idle, unlocking the queue prematurely. The FSM had no way to distinguish "signal for the current turn" from "signal for a past turn." This caused the queue to flush a third message while the second turn was still initializing. Fix: every beginTurnDispatch bumps a monotonically increasing generation integer synchronously (before the first await). Every terminal signal carries the generation it belongs to. In markTurnTerminal, if signal.generation !== state.generation, the signal is discarded silently. forceTurnIdle (rewind, deadman) also bumps generation to invalidate any in-flight terminal. Detection rule: any async start/stop system where signals can arrive out of order must carry a generation counter on every terminal signal; a terminal handler that does not check which episode it belongs to is a latent stale-signal bug.
-
"Dispatcher duplicates FSM logic — multiple atoms consulted for send-vs-queue" (Added 2026-06) — The message dispatcher (the function that decides "send now or enqueue?") read runtimeStatus, holdSessionQueueForStopAtom, queueFlushRequestAtom, and a timing heuristic to reconstruct a rough picture of whether a turn was active. This was the FSM logic — just scattered across four atoms instead of centralized. When the FSM added a new phase (stopping), the dispatcher didn't know about it and kept flushing during stop. When holdForStop was set, the dispatcher blocked even when runtimeStatus was already idle, creating a 1–2 second frozen window. Fix: the dispatcher reads one field: getTurnPhase(sessionId). If "idle" → send directly; otherwise → enqueue. All phase semantics live inside the FSM; the dispatcher has no conditional logic about what "not idle" means. Detection rule: grep every atom/boolean read inside the dispatch-vs-queue branch — if >1, the dispatcher is reimplementing FSM state; expose a single getTurnPhase() query instead.
-
"Cancel semantics conflated — user Stop and programmatic Force Send share the same cancel path" (Added 2026-06) — User Stop (which should: cancel the current turn, restore the draft text, mark the follow-up as a post-stop dispatch) and programmatic Force Send (which should: cut the current turn cleanly so the queued message can send, without poisoning the next turn with interruption text) both called the same cancelSession() function with the same flags. The shared path set userInitiatedCancelAtom = true, which then caused draft restoration to fire on Force Send too. Worse, the backend received a generic cancel that prepended [Request interrupted by user] synthetic text to both the stopped turn and the next turn's context — causing Force Send to appear in the transcript as "user pressed Stop." Fix: expose two distinct cancel APIs: stopSession() (user intent, restores draft, marks userInitiatedCancelAtom) and interruptSession() (programmatic, no draft restoration, no poisoning of next context). The backend cancel command must carry intent too so it knows whether to prepend synthetic interruption text. Detection rule: if two controls share a cancel function and have differing postconditions (draft restore, interruption text, priority marking), they need separate API entry points — not a shared path plus timing differences.
-
"Separate 'hold' atom duplicating FSM state — two simultaneous sources of truth for queue flush" (Added 2026-06) — holdSessionQueueForStopAtom was an independent boolean that said "don't flush the queue even when runtimeStatus is idle." It was set on Stop and cleared after a delay. This created a window where turnPhase = "idle" AND holdForStop = true simultaneously — the FSM said "go ahead", the hold atom said "wait." The queue checked both, but the clearing of holdForStop was on a timeout rather than tied to the actual terminal signal. Result: queue flush was delayed by the timeout even when the provider had already delivered the terminal and the FSM was idle. Fix: delete holdSessionQueueForStopAtom. The FSM stopping phase is the hold — when Stop is pressed, transition to stopping; when the terminal arrives, transition to idle. The queue only looks at phase. No separate boolean, no timeout-based clearing. Detection rule: for every boolean that says "don't do X even when another condition says yes," ask whether an FSM phase already represents that hold — if so, the boolean is a shadow copy and should be deleted.
-
"Turn finality derived from unreliable provider events instead of FSM" (Added 2026-06) — Provider events (stream-end, tool_call_complete, error) were the sole signal driving runtimeStatus directly: an event handler listened on a WebSocket channel and set isRunning = false upon receiving any of these. But provider events are unreliable: they can arrive out of order (a stream_complete event arriving after a error from the same turn), arrive late (after a new turn has already started), or not arrive at all (network drop, backend crash). When they didn't arrive, the UI stayed frozen in "running" state until a manual refresh. Fix: provider events are FSM inputs that call markTurnTerminal or markTurnRunning. The FSM decides whether to act on them (generation check, phase guard). The UI reflects the FSM phase, not the raw events. Deadman timers on dispatching and stopping phases provide a hard upper bound on freeze time when events drop. Detection rule: grep for event handlers that directly set isRunning, runtimeStatus, or equivalent turn-active atoms without going through the FSM — each is a late/out-of-order signal vulnerability.
-
"Duplicate send paths in UI components — same message can be appended and dispatched twice" (Added 2026-06) — ChatView had a direct appendAndSend() shortcut for Force Send (promoting a queued item to immediate dispatch). useSubmitMessage independently had its own append + dispatch path (the normal submit flow). When Force Send triggered, both paths could fire in the same React event batch: ChatView called appendAndSend directly, and the queue dispatcher also dequeued and called dispatchMessageBySessionType. The message appeared twice in the transcript with two different event IDs. Fix: exactly one dispatcher owns append + dequeue + model-resolution + dispatch + error handling. Visible queue controls (Force Send button, reorder handles) signal intent to the queue state machine (promote priority, mark requiresExplicitDispatch). The single dispatcher observes the change and executes. No UI component directly calls the transport layer. Detection rule: trace every backend-send call site — if >1 can fire for the same user action, there is a duplicate path; route all through the single dispatcher.
-
"Multi-purpose cancel atom causing cross-concern bleed" (Added 2026-06) — userInitiatedCancelAtom carried two unrelated concerns: (1) "the user pressed Stop — any submit during this episode is a post-Stop explicit dispatch that gets priority: now" and (2) "the Stop episode is open — gate draft restoration until the terminal arrives." These two concerns were both gated on the same atom. When Force Send programmatically cleared the cancel (by calling interruptSession without setting userInitiatedCancelAtom), the draft-restoration logic didn't fire. But when the cancel path did set userInitiatedCancelAtom = true (user pressed Stop normally), draft restoration and priority dispatch both shared state — so clearing the atom for dispatch priority also prematurely closed the draft-restoration window. Fix: name each concern explicitly. userInitiatedCancelAtom → renamed to postStopDispatchEpisodeAtom (concern: "next submit is a post-Stop explicit dispatch"). Draft restoration is driven by lastUserMessageAtom + a separate stopDraftRestorationPendingAtom that is only set by actual user-Stop, not by programmatic interrupts. Each concern is cleared independently. Detection rule: when an atom name contains a conjunction (e.g. "cancel AND restore"), it carries two concerns — split it; grep all set/clear sites to confirm each concern has exactly one writer.
