mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
main
16 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
480a1426b3 |
Fail-closed history-commit handoff (#1005)
* fix(session): fail-closed history-commit handoff (#981) The deleted-workstream discovery is now a terminal, ws_id-keyed latch: keyed conversation commits refuse admission once the durable parent is gone (convergence finalizers and force-abandon are exempt), history handoff refuses to mint a proof token so /history fails closed with a 503 instead of silently wiping the pane, and the SSE stream carries a workstream_gone resync reason. Discarded commits leave a forensic log of commit keys and roles, never content. Conversation rows gain a commit_key (migration 071): keyed saves are idempotent under retry, validated against the full commit identity, and refused when they would cross a workstream deletion. The prune orphan category now requires a NULL alias plus a two-hour updated grace, with cutoffs computed at discovery time and carried into both dialects' rechecks. The mid-turn interjection queue is owner-partitioned with no per-site mode flags: pops take the acting principal's and unowned rows, other participants' rows are structurally retained, and enforcement lives at queue admission plus the shared before_spawn gates. The retraction ledger is bounded by open pop windows: pops open a window atomically with the queue delete, restores close their ids atomically with the ledger consume, every other exit closes through one helper, and misses for unheld ids record nothing. The workstream-gone latch refuses unattended wakes at all three gates (watcher spawn, claim, delivery pre-pop), and the retry dispatcher regained its pre-envelope cancel/error convergence net. Persistence-state reporting derives through the session bound to each UI instead of a registry lookup by id that failed open to healthy during tombstone retention. The dashboard roster no longer re-inserts ghost entries from trailing activity events, the history tool-outcome scan tolerates interleaved non-turn rows, and the shared handoff-deadline handle owns its own retirement. Single-sourced across call sites: keyed-commit row values, attachment save wrappers, tail-truncation and conflict-resolution bodies for both storage dialects; worker-slot lifecycle field sets; the direct-commit admission frame; queued-row layout accessors; the string-aware comment stripper shared by every JS harness suite. Refs #981 #964 * fix(session): sweep handoff fixes to their sibling surfaces The interactive replay loop treated a system row as a tool-batch boundary, so every tool result after an interleaved row vanished from that pane while the coordinator rendered the same history correctly. Only a conversational turn ends the batch window now, matching the shared outcome index. Accepted user turns clear the composer's attachment chips on the same viewer policy that settles optimistic bubbles rather than on having matched a local bubble, so a workstream created with an upload no longer keeps a chip for an attachment the create dispatch already consumed. The coordinator's raced-Stop arm emits the stream-end hook it inherits alongside the idle state, leaving no unfinalized bubble or unflushed tool output. Ending a session surfaces a failure toast when the request never lands or answers with a non-JSON body. The per-second persistence reconcile now probes each session without blocking: a workstream whose generation and handoff locks are held is skipped until the next pass instead of contending the locks every commit needs. The one-shot repair that gates workstream creation at capacity keeps a definite probe — it has no next pass, and the sessions likeliest to be contended are the ones whose unresolved journals emptied its candidate list. Single-sourced: the attachment lane builds its conversation row through the shared commit-identity builder; the ordinary worker exit releases its slot through the lifecycle owner; both operator surfaces snapshot their counters through one non-consuming helper; the replay preamble loses its per-kind wrappers and its config hook; the browser harness suites share one brace walker; and each in-flight history attempt is one record carrying both its abort controller and its deadline. Refs #981 #964 |
||
|
Patrick Buckley
|
7a06f5e8bc |
refactor(session): make ModelLane the provider boundary (#979) (#989)
* refactor(session): make ModelLane the provider boundary (#979) ## Summary This closes the model-lane ownership gap left by #832: `ChatSession` no longer stores raw provider/client handles. `ResolvedModelBinding` now carries the provider, client, model, capabilities, registry generation, and backend-auth configuration as one coherent snapshot. - Atomically rebind existing sessions after model-registry changes while pinning each in-flight send, fallback, judge, output guard, task agent, title, compaction, perception, and voice operation to its initiating principal and binding. - Fence UI publication, canonical trajectory folds, durable writes, streams, retries, child scopes, and judge work by generation. Stop can hand off to a successor without accepting late state; cancelled tools retain typed effect receipts, and concurrent approval batches resolve by exact cycle or call. - Make create, fork, open, close, and delete race-safe with hidden `creating` reservations, incarnation-aware state tails, and an ACL-rechecked transaction that clones checkpoint-bounded history, configuration, project/persona state, and attachment references. - Extend REST/OpenAPI and Python/TypeScript SDK contracts for create/fork inputs, routed-create metadata, live-workstream probes, targeted approvals, and structured cancellation results. - Update architecture, storage, authentication, judge, channel, console, API, and SDK documentation, including regenerated architecture diagrams and OpenAPI artifacts. ## Validation - SQLite suite: 11,188 passed, 9 skipped, 10 deselected - PostgreSQL suite: 11,195 passed, 2 skipped, 10 deselected - Live backend: 3 passed - SSE recovery: 6 passed; browser recovery harness passed all scenarios - Ruff: clean; 595 files correctly formatted - mypy: 243 source files clean - TypeScript: typecheck/build and 35 tests passed - OpenAPI artifacts fresh; all 14 changed diagrams reproduce byte-for-byte - `git diff --check` and Git LFS integrity clean Closes #979. * fix(deps): update nanoid for GHSA-2v37-7h3g-55p8 Refresh the transitive lock entry admitted by PostCSS so the TypeScript security gate no longer resolves the vulnerable custom-generator implementation. Validation: - npm ci - npm audit --audit-level=moderate: 0 vulnerabilities - TypeScript typecheck and build - TypeScript tests: 35 passed * fix(test): assert canonical model registry URLs Replace prefix checks with exact canonical base URL assertions so the tests do not model incomplete URL validation. Validation: tests/test_model_registry.py (185 passed); Ruff check/format; mypy. |
||
|
Patrick Buckley
|
54ebb24374 |
feat(personas): core stamping, four-lever application, create/spawn/SDK threading; remove /creative; add --persona
The persona resolved at creation is snapshotted into workstream_config (five keys, all-or-none) and applied ONLY from the stamp — the personas table is never read post-create, so edits/archives never touch existing workstreams, and a corrupt stamp fails construction loudly instead of silently reverting to a default envelope. Legacy pre-063 workstreams carry no keys and keep today's behavior byte-for-byte. The four levers (turnstone/core/personas.py holds the codec): 1. Base override — compose_system_message(base_override=...) replaces exactly the BASE module; ENV/CONTEXT/TOOLS/POLICIES keep composing so mandatory prompt policies ride on top of every persona. This also closes the old /creative hole where the fork bypassed composition (no CONTEXT, no DB policies). 2. Tool visibility — the allowlist intersects both the composition name set (TOOLS block self-suppresses, tool-gated policies drop, the memory advisory drops) and the END of _get_active_tools so the wire never advertises hidden tools. tool_search in the set = soft (discovered tools union with the allowlist via the session's expanded-names set); absent = hard (the whole pathway is disabled, covering provider-native defer_loading, which has no synthetic name to filter). Persona sets force client-side tool search. 3. MCP gate (session-wide) — an MCP-off persona drops the client reference at construction: no merge into _tools OR _task_tools, no listeners, refresh callbacks inert, resource/prompt catalogs gone. 4. Memory (own hands only) — no recall injection, memory-directed nudges suppressed (MEMORY_NUDGE_TYPES; behavioural nudges keep firing), memory tool hidden. _task_tools is NOT filtered; compaction spill/markers are never persona-gated, and the post-compaction recall pointer is emitted only when the recall tool is actually visible. Threading: the create handler resolves once (explicit name -> 400 on unknown/disabled/kind-mismatch; empty -> the kind's default; pre-seed DB -> unstamped legacy) and stamps via constructor kwargs + config keys + the workstreams.persona column; SessionManager.open threads the stamp pre-construction exactly like the saved model alias. Non-fork resume adopts the target's stamp so _save_config can't clobber it. spawn / spawn_batch gain a persona arg with prep-time validation (children are interactive-kind; omitted = kind default, never the parent's). Python + TS SDKs, OpenAPI specs, the picker feed GET /v1/api/personas (authed, no perm), and console admin CRUD /api/admin/personas (persona.* perms, archive-only — no DELETE) round out the surface. BREAKING: /creative is removed (the REPL command now points at the writer persona); turnstone --persona <name> is the replacement. Also fixes the CLI session factory, which TypeErrored on the project_id kwarg the shared InteractiveAdapter passes unconditionally. |
||
|
Patrick Buckley
|
2169559d6e |
feat(projects): governed project containers — memory scope, grouping, manage UI (#724)
* feat(projects): governed project containers — memory scope, grouping, manage UI
A workstream can attach to a project: a first-class, shareable resource
container that owns a `project` memory scope, groups conversations, and is
managed from the console.
Storage / migration 062: projects + project_members tables, workstreams.
project_id, and the memory type default project→general; grants
project.{create,read,write,delete} (admin-default).
Recall + writes: project memory is recalled iff the workstream is attached AND
the user has access (owner ∨ member ∨ public-for-read), resolved once at session
construction; coordinators recall it too. New saves default to the project when
attached + writable; the save and delete paths are write-gated; deleting a
project purges its scoped memory; archived projects aren't recalled.
Access = RBAC capability ∧ per-project ACL (auth.resolve_project_access, a
single-fetch resolver); visibility changes, member management, and delete are
owner-only.
API: project CRUD routes on both the server and console; project_id threaded
through workstream creation, spawn inheritance, the cluster-create proxy, the
dashboard / snapshot / coordinator row builders, and the collector deltas.
UI: a project picker with an inline "+ New project" creator in every creation
box (console launcher + standalone dialog + dashboard); group-by-project in the
rail; a project badge in the composer and on dashboard rows; a console manage
tab (list + create/edit + members shelves). The admin Memories view gains
coordinator/project scope filters and human scope labels (name, not hex). The
memory tool schema documents the project scope and the attach-aware default.
* fix(projects): client refresh hardening, creator race guard, SDK project_id
Addresses PR #724 review feedback plus two bugs found while validating it.
- projects.js refreshProjects: a non-OK status (e.g. 403 when the caller
lacks project.read) or a network/parse error no longer blanks the cache
or masquerades as "no projects" -- the prior cache is preserved, the
failure is recorded (new projectsError()) and warned. Honors the
long-standing "a transient error can't blank the rail" docstring.
- projects.js _fp: the fingerprint separators were raw control bytes,
which made git treat the whole file as binary (no reviewable diff).
Rewritten as escape sequences instead of raw bytes -- behavior is
byte-identical at runtime.
- project_creator.js: createProject() could reject unhandled (authFetch
throws on network/401; r.json() throws on a non-JSON body), leaving the
widget stuck busy/disabled. Added a .catch, plus a generation guard so a
create whose widget was cancelled/reopened mid-flight drops its result
instead of selecting a project the user backed out of.
- types.ts: add project_id to CreateWorkstreamRequest / WorkstreamInfo /
DashboardWorkstream to match the server schemas (was SDK-invisible).
- test_project_api.py: move side-effecting HTTP calls out of asserts so
the requests run even under python -O.
* fix(projects): JSON.stringify the cache fingerprint, drop control-byte separators
_fp joined fields/rows on raw NUL/SOH bytes, which made projects.js read as binary to git. Replace with a collision-proof, escape-free JSON.stringify encoding -- same change-detection semantics, zero embedded control characters.
|
||
|
Patrick Buckley
|
3f106f98b2 |
fix(metacog): apply-pass fixes from pre-push full-stack review
Round-2 review caught 11 confirmed findings on the 3-commit metacog stack; this commit applies them. * **bug-1 (major)**: Wake source tag was leaking onto real user messages flushed during a wake send. ``_append_user_turn`` and ``send`` now take an explicit ``from_wake: bool`` parameter — only the wake's synthesized first turn passes True, so ``_flush_queued_messages``'s real user input no longer inherits the audit tag. Regression test pins the contract. * **perf-1 (major)**: ``CoordinatorIdleObserver._maybe_enqueue`` was issuing list_workstreams + visible_memory_count storage queries before the cheap cooldown gate could short-circuit. New ``_cooldown_allows`` read-only peek runs first; storage queries only fire when cooldown actually allows the nudge. * **q-1 (major)**: Added the missing coord-side integration test that exercises ``CoordinatorIdleObserver`` + ``IdleNudgeWatcher`` together in the production install order against a real ``SessionManager``, protecting the subscription-order contract from silent regression. * **perf-2/3 (minor)**: Cap check moved above ``_last_assistant_used_wait``; ``_fire_counts`` restructured as ``dict[str, dict[str, int]]`` keyed by ws_id so the leave-IDLE existence check is O(1). * **perf-4 (minor)**: ``NudgeQueue.drain`` fast-paths the all-match case (the common one for chat-loop drain seams) by swapping ``self._items`` directly instead of allocating a fresh ``kept`` deque + per-entry append. * **perf-5 (minor)**: Wake's synthesized empty user turn no longer writes a content-empty row to the conversations table — the ``_source`` audit tag isn't column-backed and the side-channel reminder is stripped before persist, so the row would carry nothing. * **q-3 (minor)**: Split ``IdleNudgeWatcher`` + ``install_*`` / ``shutdown_*`` helpers out of ``metacognition.py`` into the new ``turnstone/core/idle_nudge_watcher.py``; metacog stays a static-template module. * **sec-1 (nit)**: Widened ``_sanitize_child_name``'s control-char regex to cover Unicode bidi-overrides, zero-width chars, line/paragraph separators, BOM, and tag chars. * **q-4/q-5 (nits)**: Docstring referenced the wrong peek primitive (``has_pending`` → ``len()``); ``_last_assistant_used_wait``'s ``session`` parameter now typed ``ChatSession``. 5571 non-live tests pass; ruff + mypy clean. |
||
|
Patrick Buckley
|
3308e3645a |
fix(core): scope rehydrate fallback to manager, fix resume orphan
Address Copilot feedback on PR #465: 1. The has_alias fallback in both session_factories silently rewrote any unknown caller-supplied alias to the default, including on the fresh-create path where the create handler maps the factory's ValueError to a 503 with operator-friendly text. A typo in body.model would now silently start a workstream on the default instead of telling the caller their requested model could not be resolved. Move the fallback out of the factories: each factory raises again on unknown aliases, and SessionManager filters stale aliases out of the rehydrate path via a new ``model_validator`` constructor kwarg (production wiring passes ``registry.has_alias`` on both interactive and coordinator). 2. ChatSession.resume()'s elif branch flipped self.model to the persisted model name even when the alias was unresolvable, leaving the session paired with the constructor's default provider/client but a removed model name — a broken state whose next API call fails. Drop the model copy: keep the constructor's coherent default (provider + model + capabilities) and just log the unreachable saved values so the missing alias is auditable. Tests: - Move stale-alias coverage from the factory level into SessionManager (tests/test_session_manager.py): validator drops stale aliases before reaching build_session; live aliases pass through unchanged. - tests/test_sessions.py renamed test_resume_restores_model → test_resume_keeps_defaults_when_alias_unresolvable to match the new contract. |
||
|
Patrick Buckley
|
0a43bed3d5 |
fix(core): preserve workstream model + config on rehydrate
SessionManager.open() was calling build_session(ws) without a model arg on the rehydrate path. The session_factory then resolved the *current* default alias, ChatSession.__init__'s _save_config() (INSERT OR REPLACE per-key) clobbered the persisted workstream_config with those defaults, and the subsequent resume() "restored" what was now the default — silently resetting model_alias, model, temperature, reasoning_effort, max_tokens, skill, creative_mode, instructions, token_budget, and notify_on_complete on every reopen and every service restart, for both interactive and coordinator workstreams. Three layers: 1. SessionManager.open() now reads workstream_config via self._storage.load_workstream_config(ws_id) and threads the saved model_alias into build_session(ws, model=saved_alias). 2. ChatSession.__init__ now skips its initial _save_config() when a workstream_config row already exists for self._ws_id — protects every other persisted knob without having to plumb each one through the adapter signature, and catches any future construction path that forgets to thread model through build_session. 3. Both session_factories (server.py interactive, console session_factory.py coordinator) now treat an unknown caller- supplied alias the same as an unset alias: fall back to the runtime default rather than raising. Without this, a workstream pinned to an alias an operator has since removed from the registry would 500 on every reopen — defeating the "best effort restore, default if the original is gone" contract this fix is meant to deliver. Mirrors _effective_default_alias's existing has_alias guard against a stale ConfigStore default. |
||
|
Patrick Buckley
|
38a0d9c3b6 |
feat(coord): Stage 3 SessionManager Children primitive lift + cluster bus push paths
Lift the Children primitive out of CoordinatorAdapter into universal SessionManager core primitives, replace the fragile poll + state-event piggyback paths with first-class cluster bus event types for inline approval delivery, and clean up the resulting frontend reducer. Architecture - New `turnstone/core/children_registry.py` — universal parent → children + reverse-lookup primitive with atomic `add_child` (returns parent UI for race-free dispatch). Lifted from `CoordinatorAdapter`. - New `turnstone/core/child_source.py` — `ChildSource` Protocol with `SameNodeChildSource` (in-process via SessionManager state observer) and `ClusterChildSource` (cross-node via ClusterCollector listener). - `SessionManager._on_state_change` upgraded to multi-subscriber (`subscribe_to_state` / `unsubscribe_from_state`) under a dedicated lock; CLI consumer migrated. - `CoordinatorAdapter` shrunk: 731 → ~640 LOC. Children data lives in the registry; fan-out lives in ClusterChildSource. Backward-compat property facades dropped; tests updated to use the registry surface. Cluster bus event vocabulary - New event types `intent_verdict`, `approval_resolved`, `approve_request` flow through both `ClusterCollector._apply_delta` (translation from node SSE) and `emit_console_ws_*` (synthesis on console pseudo-node). - `CoordinatorAdapter._dispatch_child_event` re-emits as `child_ws_intent_verdict` / `child_ws_approval_resolved` / `child_ws_approve_request` on the parent coord's SSE stream. - New `_broadcast_intent_verdict` / `_broadcast_approval_resolved` / `_broadcast_approve_request` no-op hooks on `SessionUIBase`. WebUI pushes to the global queue; ConsoleCoordinatorUI pushes to the collector. `approve_tools` calls `_broadcast_approve_request` right after setting `_pending_approval` so the items reach the coord tree immediately, eliminating the bulk-fetch race. Cleanups - `pending_approval_detail` piggyback on `ws_state` / `cluster_state` removed end-to-end. Bulk fetch + explicit verdict / approve-request push are the canonical carriers. - Browser `_judgePollTick` 90-second poll loop deleted; push path is authoritative. - `urgent` flag on `scheduleLiveFetch` deleted (only caller was 409 retry; replaced with `invalidateLiveBadge` + standard schedule). - Console `_fetch_live_block` derives `pending_approval` from a disjunction (`activity_state="approval"` OR `state="attention"` OR detail present) so the bulk fetch can't return false during the state-transition race window. - Coord-side merge guard in `flushLiveFetches` no longer clobbered: `handleChildState` only stamps `sseUpdatedAt` when authoritatively clearing detail. - `child_locality` capability flag removed (was inert dead code). Reliability - Selective drop on listener queue overflow: critical event types (verdicts, approvals, ws_closed, child_ws_*) evict one oldest item to make room rather than dropping themselves on a full queue. Best-effort events (state ticks, content tokens, status, activity) drop as before. Applied to `SessionUIBase._enqueue`, `ClusterCollector._fanout`, and the `WebUI._global_queue` puts in the new broadcast hooks. - `_state_subscribers` snapshot under a dedicated lock so concurrent subscribe / unsubscribe during dispatch can't shift the iterator. UX / a11y - Loading placeholder in renderChildRow keeps row height stable while the bulk fetch is in-flight (sr-friendly aria-label). - Focus preservation across `_renderChildrenNow` (capture + restore by row + marker) and across targeted `_updateChildRow` swaps. - Layout-shift transition on the approval block max-height; respects `prefers-reduced-motion`. - Sidebar pending count: `(N children · M pending)`. - Risk pill `aria-label` spells out level + confidence for SR users. - Per-coord SSE listener queue depth surfaced in the status bar (`queue N/500`) with color escalation (warn at >50%, danger at >80%). Tests - 305+ test changes across 8 files. New unit tests for `ChildrenRegistry`, `ChildSource` (both impls + multi-subscriber observer), the new collector emit + apply_delta cases, the dispatch cases for new event types, the broadcast hook overrides on both WebUI and ConsoleCoordinatorUI, and the focus / placeholder / pending-count frontend assertions in `test_coordinator_page.py`. 5024 passed, ruff + mypy clean. |
||
|
Patrick Buckley
|
0debc5d061 |
fix(session_manager): scope orphan reaper by services.last_heartbeat
Replaces the ``node_id == self_node_id`` orphan-scoping heuristic from earlier on this branch with liveness-based scoping using ``services.last_heartbeat``. The heuristic was wrong for the post-#384 world: PR #384 (refactor: replace hash-ring rebalancer with rendezvous hashing) deleted the rebalancer that used to keep workstreams.node_id pointing at a live node. Without it, ``workstreams.node_id`` is now stamped at create time and never updated, so in containerized deployments with dynamic hostnames a dead pod's rows have ``node_id`` matching no surviving service — they'd accumulate forever under the old heuristic. services.last_heartbeat is the same primitive the rendezvous router uses for routing. Reusing it here keeps reap scoping aligned with routing: dead pods' rows fall out of the live set after the heartbeat window and become reapable; alive pods' rows stay protected as long as they heartbeat. Mechanics: - ``bulk_close_stale_orphans`` parameter renamed ``node_id: str | None`` → ``live_node_ids: list[str] | None``. The WHERE clause becomes ``(node_id IS NULL OR node_id NOT IN live_node_ids)``. ``None`` skips the filter entirely (single-process / tests / operator backfill). ``[]`` treats every row as unprotected. - ``SessionManager.close_idle`` pass 2 calls ``storage.list_services(self._service_type)`` to enumerate live peers, passes their service_ids as ``live_node_ids``. ``_service_type`` is derived from ``self.kind`` (INTERACTIVE→"server", COORDINATOR→"console") via a module-level mapping — no constructor param, so production wiring can't miswire the kind/service_type pairing. - list_services failure → pass 2 is skipped this tick (conservative; never reap when liveness state is unknown). Pass 1 still runs. - ``workstreams.node_id`` with NULL value is always eligible — defends against ANSI ``NULL NOT IN (...)`` evaluating to NULL (not TRUE) and silently protecting orphans forever. - Migration 048 simplified to ``(kind, updated)``; the new query's ``NOT IN (small list)`` predicate against an unbounded-cardinality column doesn't index well, so leading ``node_id`` would just add write cost. Tests cover the live-services protection (own/dead/null cases), the empty-peers reap-all case, the list_services-failure conservative fallback, both kind/service_type pairings (interactive→"server", coordinator→"console"), and the combined live_node_ids + exclude_ws_ids filter matrix. |
||
|
Patrick Buckley
|
1405afe079 |
fix(session_manager): close DB-orphan workstreams in close_idle
Real bug: workstream rows accumulate in non-closed states (idle, thinking, attention, running) when their owning process restarts or crashes. Empirical diagnosis on a live deployment found ~60 stuck coord rows in DB invisible to the in-memory-keyed dashboard, plus 100+ interactive rows older than the 2h timeout (one stuck "thinking" for 2 weeks — impossible across a process restart). Root cause: close_idle iterates self._workstreams.values() — only the loaded subset. Anything left behind by a prior process incarnation sits in DB forever because nothing ever re-loads it. This commit gives close_idle a second pass. Pass 1 (existing, unchanged): close loaded IDLE rows whose ws.last_active (monotonic) is past timeout. IDLE-only so legitimately- attentive rows (waiting for user response) stay live. Pass 2 (new): bulk-close DB rows of this manager's kind whose updated is past the wall-clock cutoff and which aren't currently loaded. Closes the broader BULK_CLOSE_STATE_VALUES set — any matching row is by definition not loaded by any process and cannot be in a live interaction. Scoped by self._node_id so a sibling node can't reap rows we own (multi-node interactive correctness). No emit_closed — never-loaded rows have no SSE listeners expecting them. Lock invariant: pass 1 holds self._lock briefly to snapshot victims and pop them (existing behavior). Pass 2 holds self._lock briefly to snapshot the loaded keys, then releases before the DB UPDATE so a slow reaper query can't block create/get/set_state. Also fixes a same-process race in open(): the rehydrate path read DB, released the manager lock, then re-acquired to install — a concurrent pass 2 between the two acquisitions snapshots loaded keys without the in-flight ws_id, and could clobber its DB row to closed. open() now calls touch_workstream(ws_id) on rehydrate so the row's updated is fresh against any pass-2 cutoff. Pure timestamp write is safe against concurrent close() (close still wins on the state column). Three new tests cover the DB orphan pass (basic, exclude-loaded, kind filter) plus node_id scoping (own/foreign rows, None-skips-filter) and the open() rehydrate touch. |
||
|
Patrick Buckley
|
d15f182b80 |
fix(coord): tree UI not updating when LLM deletes workstream (#429)
* fix(coord): tree UI not updating when LLM deletes workstream The coord LLM's `delete_workstream` tool wiped the storage row but fired no SSE event, so a long-lived dashboard tab kept the deleted child visible (with its last-known idle/closed state) until a full reload. A coordinator that spawns→completes→deletes children would leave an ever-growing tree. Fix: add `SessionManager.delete()` that drops the in-memory slot if present and emits `ws_closed` with `reason="deleted"` (mirrors `close()`'s shape). Wire `delete_workstream_endpoint` to call it after the storage delete succeeds, snapshotting the workstream's name into the event payload before the row is wiped. The cluster collector → coord adapter chain re-emits as `child_ws_closed`; the browser's existing `handleChildClosed` already keys on `reason === "deleted"` to mark the row, so no JS changes needed. Event emit is best-effort — a fan-out failure logs a warning but doesn't roll back the storage delete (the row is already gone). * fix(coord): apply Copilot review feedback on PR #429 - server.py: clarify that ``name`` is forwarded to mgr.delete only (not into the audit detail) — comment previously claimed both. - test_session_manager.py: extract ``mgr.delete(ws_id)`` to a local before asserting (CodeQL: no side-effecting calls inside ``assert``, which would be stripped under ``python -O``). - test_workstream_endpoints.py: docstring said "Yield" but the fixture ``return``s; switch to "Return". |
||
|
Patrick Buckley
|
c77b237033 |
refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair (#417)
* refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair Eliminates the phantom create→close pair on coord rollback that was documented as a known limitation in PR #416. The pair surfaced on the cluster events stream when a multipart workstream-create request failed attachment validation: coord's ``mgr.create`` fired ``emit_created`` synchronously, then the rollback called ``mgr.close`` which fired ``emit_closed``. Cluster consumers had to reconcile via the collector's diff path. Post-fix, a rejected upload produces zero events. API changes on ``SessionManager``: - ``create(..., defer_emit_created: bool = False)`` — when True, skip the trailing ``emit_created`` so the caller can run additional post-create work (attachment validation in the lifted HTTP handler) before advertising the workstream. Default preserves the existing "advertise immediately" contract for direct callers (test fixtures, CLI REPL, channel adapters). - ``commit_create(ws)`` — fires the deferred ``emit_created`` event after the caller's post-create work confirms the workstream should be advertised. Synchronous; the wrapped work is in-memory and non-blocking on every kind (interactive: documented no-op stub; coord: dict updates under a lock + ``queue.put_nowait`` fan-out). - ``discard(ws_id)`` — releases the in-memory slot + cleans up the UI WITHOUT firing ``emit_closed``. Distinct from ``close`` which advertises the transition; ``discard`` is for the rollback case where the workstream's existence was never advertised. Storage-row deletion stays a separate concern (caller invokes ``delete_workstream``), mirroring ``mgr.create``'s split between slot reservation and ``register_workstream``. Caller-bug detection: ``Workstream._emit_created_fired`` is set inside ``create`` (non-deferred path) and ``commit_create``; ``discard`` logs ``session_mgr.discard.after_emit_created`` warning when invoked on an already-advertised workstream. Slot is still released so capacity isn't stranded. Lifted ``make_create_handler`` updated to use the deferred bracket: pass ``defer_emit_created=True``, validate uploaded attachments, then ``mgr.commit_create(ws)`` on success / ``mgr.discard(ws.id)`` on failure. Ordering invariants (``commit_create`` BEFORE ``audit_emit`` and ``post_install`` so any state events the worker fires reach the cluster collector for an already-known ws_id) are documented in the handler docstring. Tests: - 5 new ``SessionManager`` unit tests (defer skips emit, commit fires it, commit no-ops without emitter, discard releases without emit_closed, discard returns False on unknown id). - 2 caller-bug regression tests (commit_create after discard pins the silent re-emit behaviour; discard after non-deferred create asserts the warning fires + slot still releases). - 1 coord regression test asserting the cluster collector sees zero events when attachment validation fails. ``/review`` pipeline run; M1 (test gap on caller-bug paths) + Mi1 (no runtime guard for already-advertised) + Mi2 (``_make_manager`` event_emitter override) + Mi3 / N2 (duplicated comments + ordering invariant) + N1 (drop ``to_thread`` on ``commit_create``) all addressed. 4509 tests passing; ruff + mypy clean. * fix(core): apply Copilot + code-quality review feedback on PR #417 Copilot review: - ``Workstream._emit_created_fired`` comment claimed the flag was "set under the manager's _lock-protected emit", but the actual ordering set it OUTSIDE the lock. Comment updated to describe the real synchronization (non-deferred ``create`` sets it immediately before ``emit_created``; ``commit_create`` sets it under the manager lock alongside the tracked-ws check). - ``commit_create`` had no guard against duplicate calls, post-discard calls, or calls on workstreams not tracked by this manager — any of those would have fired duplicate or phantom ``ws_created`` events. Added a guard symmetric to ``discard``'s after-emit warning: under ``self._lock``, check ``_emit_created_fired`` + ``_workstreams.get(ws.id) is ws``, no-op + log a warning (``session_mgr.commit_create.already_fired`` / ``session_mgr.commit_create.untracked``) on either failure. The emit itself still runs outside the lock so coord's collector fan-out doesn't couple to the manager mutex. - ``test_commit_create_after_discard_is_caller_bug_no_op`` was internally inconsistent — name + docstring said "must not re-emit" but the assertion expected the re-emit. Renamed to ``test_commit_create_after_discard_is_no_op`` and updated to assert the new no-op + warning behaviour. New test ``test_commit_create_is_idempotent_on_duplicate_call`` pins the second-commit-call code path: exactly one ``ws_created`` event fires, second call short-circuits via the guard with a ``commit_create.already_fired`` warning. Code-quality bot review (3 findings, identical pattern): - Three test ``assert`` statements wrapped side-effecting calls (``assert mgr.discard(ws_id) is True/False``); under ``python -O`` the asserts strip and the side-effect strips with them. Refactored all three to assign the result to a local first, assert on the local. No behaviour change. 4510 tests passing; ruff + mypy clean. |
||
|
Patrick Buckley
|
48c9ad2a40 |
refactor(core): split SessionKindAdapter Protocol into construction +… (#412)
* refactor(core): split SessionKindAdapter Protocol into construction + emission (Stage 2 P3)
The single ``SessionKindAdapter`` Protocol that ``SessionManager``
takes is split into two:
* ``SessionKindAdapter`` — kind / build_ui / build_session /
cleanup_ui. Required for every kind. The shared lifecycle
manager always delegates here for construction + cleanup.
* ``SessionEventEmitter`` — emit_created / emit_state /
emit_rehydrated / emit_closed. **Optional**, wired through a new
``event_emitter: SessionEventEmitter | None = None`` kwarg on
``SessionManager``. Reserved for future kinds whose lifecycle
transitions don't fan out anywhere; both production kinds wire
one today.
Both production adapters implement both Protocols. The interactive
lifespan (``server.py``) and console lifespan
(``console/server.py``) pass their adapter as both ``adapter`` and
``event_emitter`` — production behaviour is unchanged. Six lifecycle
sites in ``SessionManager`` (create / open eviction / open rehydrate /
close / set_state / close_idle / _reserve_and_install_locked unwind)
now call ``self._event_emitter.emit_*(...)`` guarded by
``if self._event_emitter is not None``.
InteractiveAdapter asymmetry preserved + documented:
* ``emit_closed`` stays load-bearing — it's the **sole** transport
path for ``ws_closed`` onto the process-wide global SSE queue
(Stage 1 consolidated emission from the create handler here so
there's exactly one emission point; ``name`` powers the
frontend's eviction toast).
* ``emit_created`` / ``emit_state`` / ``emit_rehydrated`` are
documented no-op stubs (``del ws[, state]``). Those events fire
from out-of-band paths — the create HTTP handler enqueues
``ws_created`` directly onto ``global_queue`` *after* attachment
validation (so a rejected upload doesn't surface a phantom
create→close pair); ``WebUI._broadcast_state`` emits the full
``ws_state`` payload (tokens + context_ratio + activity) via the
``SessionUI.on_state_change`` callback chain. The stubs exist
solely to satisfy ``SessionEventEmitter`` Protocol so the
adapter can be wired as the manager's ``event_emitter`` for the
``emit_closed`` path. Each stub has a 1-line inline rationale to
match the in-repo convention (``coordinator_adapter.py:210``).
Test scaffolding:
* ``tests/test_session_manager.py`` — ``_make_manager`` and
``_make_with_writer`` wire ``FakeAdapter`` as both ``adapter``
and ``event_emitter`` for production parity; the standalone
``test_create_uses_configured_node_id`` does the same.
``FakeAdapter.emit_rehydrated`` now records as
``_Event("rehydrated", ...)`` rather than conflating with
``"created"``, and ``test_open_resurrects_closed_state`` asserts
against ``events_of("rehydrated")`` so a regression where the
manager fires the wrong call on the open path actually fails.
* ``tests/_coord_test_helpers.py`` and
``tests/test_coordinator_end_to_end.py`` — wire
``CoordinatorAdapter`` as both args.
* Six interactive test fixtures (``test_skills.py``,
``test_prompt_templates_runtime.py`` x2, ``test_model_registry.py``,
``test_server_authz.py``, ``test_server_attachments_on_create.py``)
— wire ``event_emitter=adapter`` so they match the production
wiring, removing the footgun where a future contributor adds a
``gq.get_nowait()`` assertion and silently loses the only
``ws_closed`` transport.
* ``tests/test_interactive_adapter.py`` — drops the three
tautological no-op-emit_* tests (``test_emit_created_is_noop``,
``test_emit_state_is_noop``, ``test_emit_rehydrated_is_noop``);
keeps the four ``emit_closed`` tests (real behaviour).
Lint + mypy clean. 4475 tests passing.
* docs(core): correct SessionKindAdapter + SessionEventEmitter docstrings to match implementation
Two Copilot review threads on PR #412 caught the same real
discrepancy: my P3 docstrings on ``SessionKindAdapter`` and
``SessionEventEmitter`` described an *intent* — "interactive
doesn't implement ``SessionEventEmitter``; the manager skips emit
calls when no emitter is wired" — that doesn't match the actual
wiring. ``InteractiveAdapter`` does implement both Protocols and
``server.py`` does pass it as ``event_emitter``; only the three
no-op stubs (``emit_created`` / ``emit_state`` / ``emit_rehydrated``)
are dead, while ``emit_closed`` is load-bearing.
Updated both docstrings to:
* State that both production adapters implement both Protocols.
* Explain the asymmetry is in *which* emit methods carry real
bodies (coord: 4; interactive: 1, with 3 documented stubs because
the out-of-band paths — create handler ``ws_created`` after
attachment validation, ``WebUI._broadcast_state`` carrying the
richer ``ws_state`` payload — fire those events).
* Clarify the ``if self._event_emitter is not None`` guard exists
for the kwarg-omitted case (tests that don't care about events,
reserved for future kinds whose transitions don't fan out
anywhere).
Docstring-only change. Lint + mypy clean; the 75 tests in
test_session_manager + test_interactive_adapter + test_coordinator_adapter
pass.
Resolves the two Copilot review threads on PR #412 (commits
PRRC_kwDORcMomM67VyPD, PRRC_kwDORcMomM67VyPI).
|
||
|
Patrick Buckley
|
a8cd9444b1 |
fix(server): apply Copilot + code-quality review feedback
PR #410 review pass: * **session_worker**: ``except BaseException`` → ``except Exception`` in ``_runner`` (code-quality bot). Daemon threads don't receive SystemExit/KeyboardInterrupt, so the wider catch was unjustified defensive style. Same defense-in-depth for unexpected ``run()`` exceptions; doesn't widen scope to runtime signals. * **session_worker**: ``threading.Thread()`` construction moved inside the spawn branch under ``ws._lock`` (Copilot). The enqueue path no longer allocates and then discards a Thread object on each call against a busy workstream. Thread() construction is microsecond-cheap, so the lock-window growth is negligible vs. the saved allocation churn. * **lifespans**: ``state_writer.shutdown()`` (and the console equivalent) now run via ``asyncio.to_thread`` so the daemon- thread join + sync DB drain don't block the event loop and delay other teardown tasks (Copilot, ×2). * **tests**: five remaining ``writer._flush_once()`` calls switched to the public ``writer.flush()`` API across test_session_manager.py (4) and test_state_writer.py (1) (Copilot, ×5). Tests no longer depend on private internals. |
||
|
Patrick Buckley
|
8240e32704 |
test(core): regression tests for state_writer + close ordering
Five new tests under ``TestSessionManagerWithStateWriter`` exercise the bug-3 invariant under write-behind: * set_state buffers via the writer (long flush_interval → no sync write until drain). * set_state(ERROR) flushes synchronously. * close after a buffered transient writes 'closed' as the final state — the buffered 'running' must NOT be flushed to storage AFTER close's sync 'closed' write. * close_idle exhibits the same invariant. * set_state arriving AFTER close short-circuits on ws._closed and never reaches the buffer. |
||
|
Patrick Buckley
|
c837e3fa6d |
feat(core): Stage 1 SessionManager unification (#408)
* feat(core): scaffold SessionManager + SessionKindAdapter Protocol Stage 1 step 1 — pure addition, no production wiring. Defines the shape later steps will port the shared mechanics onto: slot accounting, per-ws-id refcounted rehydrate locks, kind-agnostic lifecycle; kind-specific event transport + session construction on the adapter. Pruned from the earlier Protocol draft (see design brief): per-kind permission_scope (static handler map is simpler), allows_child_spawn / quota_policy (deleted in #403), on_child_spawned (coordinator tool owns children registry), allows_active_focus / active_id / switch (frontend owns the active-tab state). * feat(core): port shared session-lifecycle mechanics onto SessionManager Stage 1 step 2. Adds create / open / close / set_state / close_idle / get / list_all / count on top of the Step 1 scaffolding. Pure addition — still no production wiring; the new class doesn't replace any call sites yet. Concurrency shape is ported from CoordinatorManager (the more- complete side): single-phase slot reservation under the manager lock, per-ws refcounted open-lock to serialize concurrent lazy rehydrate, placeholder workstreams count toward max_active but can't evict each other. WSM's two-phase eviction outside the lock is not carried over; it had a window where a burst of creates could silently exceed max_active. Deletions (vs. the union of the two old managers): - "refuse to close last workstream" guard — handled by the dashboard; only existed to protect the now-deleted default startup workstream. - active_id / switch / get_active — frontend owns focus; server-side duplicate state is gone. - _active_coords presence cache — defer measurement to Step 4; if it pays for itself at realistic cluster sizes, the CoordinatorAdapter can maintain it by observing emit_* calls. - Children registry + reverse index — coordinator tool owns this, manager stays kind-agnostic. Skill resolution (name → template_id + applied_version) is now shared via SessionManager._resolve_skill, so WSM's pre-resolve-at- callsite pattern and CM's internal-lookup pattern converge. Callers pass the skill name; the manager does the lookup once. 26 smoke tests cover create eviction + overflow, concurrent-create cap, persist/session rollback, open for missing/deleted/wrong- kind/wrong-user rows, concurrent-open serialization, close unblocks UI + emits closed, set_state + storage + adapter observer, close_idle, list_all ordering, count, eviction fires adapter transport, node_id passthrough. * feat(core): add InteractiveAdapter for SessionManager Stage 1 step 3. Adapter that bridges SessionManager to the node's interactive transport: - emit_created/state/closed → pushes onto the process-wide SSE global_queue (same shape current server.py handlers produce inline) - cleanup_ui → ports WorkstreamManager._cleanup_ui body: unblock _approval_event / _plan_event / _fg_event, broadcast ws_closed to per-UI listener queues (with full-queue fallback), cancel + close the session - build_ui/build_session → delegate to injected factories (ui_factory builds WebUI, session_factory is the existing closure from server.py with judge_model + memory_config captures) Also extends SessionKindAdapter.build_session with **extra passthrough so interactive callers can pass judge_model per-call without polluting the manager API; and adds a reason= kwarg to emit_closed so the frontend's "evicted" special-case keeps working (frontend doesn't differentiate "idle" from "closed", so close_idle collapses into close()). 14 new adapter tests cover wire payload shape, queue.Full tolerance, cleanup_ui event unblocking + listener broadcast + queue-full fallback, session cancel+close, graceful handling of stub UIs / None session, and kwarg passthrough to the session factory. * feat(console): add CoordinatorAdapter for SessionManager Stage 1 step 4. Coordinator-side SessionKindAdapter implementation: - emit_created/state/closed → delegate to the existing ClusterCollector.emit_console_ws_* methods (same wire shape the old CoordinatorManager emitted inline) - cleanup_ui → ports the listener-queue + approval/plan event unblocks from CoordinatorManager._cleanup, with queue-full fallback so an unresponsive browser tab can't wedge close - build_ui/build_session → delegate to injected factories; session factory doesn't accept client_type so we strip it at the adapter boundary Collector emission exceptions are swallowed (same policy as today's inline fan-out — dashboard lag on one tick is preferable to breaking the lifecycle path). Intentionally out of scope: the children registry (_children / _child_to_coord) stays in the coordinator tool when wired in Step 5; the _active_coords lock-free presence cache is deferred pending a measurement at realistic cluster sizes. 10 new tests cover transport payloads, collector-exception tolerance, cleanup_ui event unblock + listener broadcast + queue-full eviction, construction passthrough. * feat(server): wire interactive server.py to SessionManager Stage 1 step 5a. Production-path swap: WorkstreamManager → SessionManager(InteractiveAdapter(...)). - Construction at server startup: build the adapter with the process-wide global_queue, a WebUI ui_factory closure, and the existing session_factory. SessionManager gets storage + max_active. - Default startup workstream wiring removed (the CLI-REPL leftover flagged in the handoff's "Convergence is also a pruning opportunity" section). --resume now lazily creates a workstream scoped to the resumed content; no workstream at all if --resume isn't given. The dashboard handles the 0-ws state. - HTTP handler mgr.create() calls switched to the new kw-only signature (user_id, name, model, skill, ws_id, client_type, judge_model, parent_ws_id). ui_factory/skill_id/skill_version/kind no longer threaded through — adapter handles UI construction and manager resolves skill internally. - Dropped the mgr.last_evicted block in the /new handler (adapter emits ws_closed:evicted automatically on capacity eviction). - mgr.max_workstreams → mgr.max_active. - Added active_id / switch / switch_by_index / get_active / index_of / eviction_count to SessionManager because turnstone/cli.py uses them extensively; the handoff's "delete unless there's a live caller" rule flips here — CLI is a live caller. Test fixtures across 9 files updated to build SessionManager + InteractiveAdapter rather than WorkstreamManager. test_workstream.py stays unchanged (it tests WSM directly; it'll be deleted in step 5d alongside the class itself). Full pytest: 4528 passed. Ruff + mypy clean. Next: 5b (console-side wiring, with the children-registry relocation to the coordinator tool). * feat(console): wire console server to SessionManager Stage 1 step 5b. Production-path swap: CoordinatorManager → SessionManager(CoordinatorAdapter(...)). - CoordinatorAdapter now owns the coord-specific bits that were bolted onto the old CoordinatorManager: the children registry (forward + reverse index), the lock-free active-coords presence cache, the cluster-event fan-out thread, and the worker-dispatch path (send / _spawn_worker). The shared SessionManager stays kind-agnostic. - Added CoordinatorAdapter.attach(mgr) for late-binding the owning manager (the manager's ctor takes the adapter, so the dependency has to break here). Used inside _rebuild_children_registry for the tenant- filtered SQL query, inside send/dispatch for mgr.get(ws_id), and inside the fan-out seed path for mgr.list_all(). - emit_created now seeds the children registry + active-coords slot AND calls _rebuild_children_registry (covers both create — empty query — and open/rehydrate, where the subtree is persisted). emit_closed drops both entries. Collapses the three old call-sites in CoordinatorManager's create/open/close into one per-event hook. - Console server.py builds the manager via: coord_adapter = CoordinatorAdapter(collector=..., ...) coord_mgr = SessionManager(coord_adapter, storage=..., max_active=..., node_id=ClusterCollector.CONSOLE_PSEUDO_NODE_ID) coord_adapter.attach(coord_mgr) ConsoleCoordinatorUI._coord_mgr = coord_mgr app.state.coord_adapter = coord_adapter - HTTP handler call-site updates: - coord_mgr.create drops initial_message; the handler now calls coord_adapter.send(ws.id, initial_message) after create so the worker spawn stays out of the shared manager. - coord_mgr.open_admin(ws_id) → coord_mgr.open(ws_id, user_id="", admin=True). Matches SessionManager.open's unified signature. - coord_mgr.list_for_user(uid) inlined as a list comp on list_all() (SessionManager doesn't expose the filter; two callers). - coord_mgr.children_snapshot / send → coord_adapter.*. - coord_mgr.cancel stays (now lives on SessionManager from 5a). - ConsoleCoordinatorUI.on_state_change now flows state transitions through ConsoleCoordinatorUI._coord_mgr.set_state, mirroring the WebUI pattern. The old _on_state_observer / _on_rename_observer closures the manager used to install are dead code now; leaving the fields in place for 5d cleanup. - Lifespan shutdown calls coord_adapter.shutdown() (was coord_mgr. shutdown()) and resets ConsoleCoordinatorUI._coord_mgr on teardown. Test fixture updates in _coord_test_helpers, test_coordinator_end_to_end, test_coordinator_endpoints, test_phase6_endpoints: build SessionManager + CoordinatorAdapter in _build_mgr, set app.state.coord_adapter, switch mgr.register_children / mgr.children_snapshot tests to mgr._adapter.*, and rewrite test_open_admin_uses_open_admin to assert the unified open(user_id="", admin=True) call shape. Full pytest: 4486 passed. Ruff + mypy clean. Next: 5d (remove CoordinatorManager + WorkstreamManager class bodies and their test files). * feat(core): delete WorkstreamManager + CoordinatorManager classes Stage 1 step 5c + 5d. Final step of the unification — the legacy classes and their test files go away now that every production caller has been ported. - Delete turnstone/console/coordinator.py entirely (CoordinatorManager class + the _enqueue_on_ui helper, which CoordinatorAdapter now hosts its own copy of). - Trim turnstone/core/workstream.py to just the Workstream dataclass + WorkstreamKind + WorkstreamState. ~385 lines of WorkstreamManager logic gone; the remaining shape is pure data types shared by both managers. - Delete tests/test_workstream.py (WSM-specific) and tests/test_coordinator_manager.py (CM-specific). - Wire turnstone/cli.py to SessionManager + InteractiveAdapter, same pattern as turnstone/server.py. The CLI's WorkstreamTerminalUI uses manager.set_state + manager.active_id — both preserved on SessionManager (CLI is a live caller that keeps the focus API honest, per the handoff's "delete unless it pulls its weight" rule). - Add an optional manager-level ``_on_state_change`` observer hook restored for the CLI's background-attention notification (the web path uses the adapter's emit_state; this hook covers callers that don't consume SSE). - Drop dead ``_on_state_observer`` / ``_on_rename_observer`` fields from ConsoleCoordinatorUI — the old CoordinatorManager installed them; SessionManager/CoordinatorAdapter handle fan-out directly. Vulture @ 80% confidence: zero unused symbols across the new SessionManager + adapter files. Ruff + mypy clean (170 files). Full pytest (excluding tests/live): 4414 passed. Net across the whole Stage 1 branch: one unified SessionManager + adapter Protocol replaces two ~500-line parallel managers + a ~600-line CoordinatorManager, and the interactive + coordinator transports stay cleanly separated at the adapter boundary. * refactor(auth): drop workstream row-level ownership gates Turnstone is a trusted-team tool (per #400). user_id stays as metadata for audit + display; it no longer rejects requests. Scope- level auth via admin.workstreams / admin.coordinator tokens is the only gate now. Solves sec-1 (cross-tenant delete via collision on caller-supplied ws_id, because the gate was half-implemented) and sec-2 (blank-sub JWT bypass on empty-owner rows). Net: 359 lines of defensive empty-string comparisons and admin=True bypass plumbing deleted. * fix(core): serialize set_state vs close + worker spawn Three concurrency fixes from the multi-stage review: - bug-3: set_state now looks up ws under self._lock and gates its storage write on ws._closed (a new tombstone flag). close() sets ws._closed=True and does its storage write under ws._lock. A set_state that acquires ws._lock after close sees the tombstone and skips its write instead of resurrecting the closed row. - bug-1: _spawn_worker wraps the check-and-spawn in ws._lock so two concurrent send() HTTP requests can't both observe "no live worker" and start duplicate worker threads on the same ChatSession. - bug-2: replaces Thread.is_alive() as the reuse gate with an explicit ws._worker_running flag. The flag is set before the worker thread starts and cleared in its finally block — both under ws._lock. Using is_alive() left a narrow window where the worker could exit between the check and a queue_message call, stranding the user's message with no consumer. perf-2 (lock-held-across-DB-write) is accepted as-is: per-ws serialization of state transitions behind a DB round-trip is real cost but bounded — a given ws's state flips happen sequentially on its worker thread anyway. Dropping ws._lock around the DB write would reintroduce the bug-3 race. Full pytest: 4401 passed. Ruff + mypy clean. * refactor(core): drop _resolve_skill from SessionManager Skill resolution (name → template_id + applied_version) moves out of the shared manager and back to the HTTP handlers that own the create request. The interactive handler already resolved skill_data + applied_skill_version for other purposes (model override, judge config, post-create session seed) and was passing the name to SessionManager which then redundantly re-resolved via get_skill_by_name + count_skill_versions — two wasted DB round-trips per create on a user-visible latency path. - SessionManager.create: accepts skill_id + skill_version as already-resolved kwargs; _resolve_skill helper deleted. - turnstone/server.py create_workstream: passes the skill_id / applied_skill_version it already computed. - turnstone/console/server.py coordinator_create: pre-resolves inline (parity with interactive) before calling coord_mgr.create. Fixes perf-1 (redundant skill queries per create), q-4 (divergent skill-version computation between manager and handler), q-5 (coordinator-specific lookup on the shared manager surface). Full pytest: 4401 passed. Ruff + mypy clean. * refactor(adapters): extract shared cleanup_ui + drop dead child-registry methods Both InteractiveAdapter.cleanup_ui and CoordinatorAdapter.cleanup_ui (plus their _broadcast_ws_closed_to_listeners helpers) were byte-identical. Pull them into turnstone/core/adapters/_ui_cleanup.py:cleanup_session_ui so the two adapters delegate to one implementation. Also drop CoordinatorAdapter.register_children (only test callers — now use _seed_children in tests/_coord_test_helpers.py) and _add_child (zero callers anywhere). * refactor(adapters): symmetric attach() + fail-loud on unattached manager Add InteractiveAdapter.attach(manager) + .manager property mirroring the coord-side pattern. CLI (cli.py) now uses cli_adapter.attach(manager) instead of the _mgr_ref list-ref late-binding hack; server.py picks up the same call for consistency. CoordinatorAdapter.send / _rebuild_children_registry / _prime_children_from_snapshot no longer silently return when self._manager is None — raise RuntimeError so a forgotten attach() at startup fails loud instead of dropping the whole fan-out. * docs: replace stale WorkstreamManager / CoordinatorManager references Both classes were deleted in 965e0b6; prose docstrings across the codebase still named them. Update to SessionManager (or describe the collapsed-into-one-class architecture where the distinction matters). Leaves the 'Ported from …' historical markers in session_manager.py / coordinator_adapter.py / interactive_adapter.py intact — those are deliberate pointers back to the pre-unification code. * fix(core): atomic close_if_idle + batch pop under one lock bug-5: SessionManager.close_idle re-checked ws.state == IDLE outside the lock, so a pending tool result could flip state IDLE→RUNNING between the snapshot and close() acquiring self._lock. Add _close_if_idle_locked that tests state + pops under self._lock. perf-5: drop the per-victim self._lock acquisition; collect + pop the whole batch in one acquisition, then run cleanup_ui / storage write / emit_closed outside the lock. * perf(coord): split emit_created / emit_rehydrated to skip storage query on fresh creates CoordinatorAdapter.emit_created was unconditionally calling _rebuild_children_registry (storage.list_workstreams with parent_ws_id=... limit=10001) on every create, even for fresh-create paths that provably have zero children. Add emit_rehydrated to the SessionKindAdapter Protocol. SessionManager .create still calls emit_created; .open (lazy rehydrate) now calls emit_rehydrated. CoordinatorAdapter.emit_created seeds the registry + fan-out but skips the rebuild; emit_rehydrated seeds + rebuilds + fans out. InteractiveAdapter.emit_rehydrated delegates to emit_created (no children-registry on the interactive transport). * perf(coord): fold _active_coords into _children_lock + mutate payload in place perf-4: _active_coords used a copy-on-write dict-swap pattern so the fan-out dispatch could read it lock-free, but _dispatch_child_event already re-validates the parent under _children_lock anyway — the lock-free snapshot was premature. Replace with a plain dict read+write both under _children_lock; install and remove collapse to one-liners. Value also drops the user_id half — dead after |