mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
main
46 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
3252f3fd95 | fix(memory): preserve replay identity and validation | ||
|
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
|
a40ff249ec |
fix: address review — request-scoped storage in coord tenancy checks
- _coordinator_tenant_check and _coord_attachment_owner resolved storage from the global registry (get_workstream_row / for_request without a storage arg), which can evaluate the project-tenancy decision against a different or auto-initialised backend and fail OPEN on a missing project row. Use request.app.state.auth_storage explicitly, matching cluster_ws_detail and _resolve_coordinator_or_404; fail closed (404) when it is unset. - reject_unassignable_scopes now derives its allowed-scope error message from ASSIGNABLE_SCOPES so validation and the message can't drift. |
||
|
Patrick Buckley
|
36419a9809 |
fix: scope private-project workstream visibility to members, not admins
Workstreams attached to a private project were visible -- including their conversation content -- to holders of admin.cluster.inspect / admin.coordinator (both default builtin-admin permissions), defeating the project's confidentiality boundary. Enforce that a private project's resources are visible only to people IN the project (owner, workstream creator, or an explicit member), even for admins. Surfaces closed: - WorkstreamProjectVisibility bypass narrowed to service scope only (node->console machine plumbing, re-filtered per-user at the console edge). No human principal bypasses; admin.cluster.inspect gates the inspect surfaces, not tenancy. This flows to /dashboard, session listings, the attachment row-gate, cluster_workstreams, cluster_node_detail, and cluster_snapshot/SSE. - cluster_ws_detail 404-masks a workstream in a private project the caller can't see; cluster_ws_live_bulk routes such ids to the denied list (no private-project oracle). - Coordinator operator verbs (history/export/detail/send/approve/set_title/open/ children/tasks/attachments) now enforce project tenancy: _coordinator_tenant_check on coord_endpoint_config, the gate in _resolve_coordinator_or_404 (children/tasks), the tenant_check now run in make_open_handler before rehydrate, and a project-visibility check in _coord_attachment_owner. admin.coordinator gates the surface cluster-wide, but a non-member is 404-masked. The tenant-check mirrors the manager-first + coordinator-kind ladder so kind-isolation is preserved. - service scope is no longer user-assignable: admin_create_token and both turnstone-admin CLI mint paths reject it via reject_unassignable_scopes, so an admin.users holder cannot self-mint a service token and restore the bypass. Service scope is minted only by ServiceTokenManager / the JWT secret. - The events/global node proxy (service-elevated cross-tenant firehose) is gated on admin.cluster.inspect so a plain authenticated user cannot reach it through the console proxy. Updates the OpenAPI description, the row-gate/tenancy-filter docstrings, and adds tests for every surface (visibility predicate + cluster detail/bulk + coordinator history/export/children/open/attachments + events/global proxy + scope-mint rejection); inverts the tests that pinned the old admin-bypass contract. |
||
|
Patrick Buckley
|
7f0e0406b3 |
test(approvals): concurrency matrix + suite migration to the cycle model
New regression matrix for the release blockers: cross-approval independence, lost-wakeup at gate entry, FIFO selector-less resolution, resolve-all sweep, double-resolution no-op, cards/legacy view tracking, and the generation-exactness set — stale delivery rejection, Smart-Approvals origin check, purge keep_origin, the purge-to-register window eviction, late cross-generation "superseded" stamping, concurrent smart+human gates, and the pre-delivered-verdict fast path. Plus sub-agent judge wiring (agent_gate off the main slot, close() firing all generations) and endpoint tests for cycle pinning and the Approve+Always race guard. Gate threads run under one shared mock-patch harness — mock.patch start/stop of the same target from concurrent threads corrupts the patcher's restore stack — with a sweep-until-dead teardown so the conftest leak guard can't trip. Existing suites migrate off the singleton fields to cycle assertions and the pending_approval_details wire shape. |
||
|
Patrick Buckley
|
5d1d34cd82 |
fix(personas): close review findings across the envelope, resume, and RBAC lanes
Provider search gating (replace-only): native web search now stands in for
a client web_search def that survived the persona visibility filter — on
both OpenAI surfaces and both injection lanes (web_search_options, the
server_side_tools loop, and _convert_tools' capability lane). A scribe or
any envelope hiding web_search stays search-free on search-capable models;
coordinators and tool-less utility calls stop receiving search too.
Resume stamp discipline: resume() loads config and parses the target's
stamp BEFORE touching session identity/history, so a corrupt stamp raises
with the session intact instead of half-adopting and then 'repairing' the
target's stamp on the next config save. The MCP lever now follows the
stamp on mid-session adoption: an MCP-off stamp drops the live surface in
place (listeners deregistered, toolsets reset); adopting an MCP-on stamp
into a session whose persona gated the client off is refused loudly (the
surface cannot be rebuilt post-construction). The REPL /resume handler
reports these errors instead of crashing the CLI.
Fail-closed default lane: a FAILED default-persona lookup at create is a
503 (routes) / clear exit (CLI) instead of silently degrading to the
unstamped stock envelope; a clean 'no default configured' still creates
legacy. resolve_persona_for_kind reports storage-unavailable distinctly
from unknown-persona.
Soft-set governance: tool_search expansion under a persona visibility set
recomposes the system prompt so tool-gated policy segments land with the
tool they gate. MCP resource/prompt catalogs gate on read_resource /
use_prompt visibility. Spawn judge/audit projections carry persona (the
human approval header already did). Active-list rows carry persona like
their project_id twin.
RBAC catalogs: persona.{create,read,write} join _VALID_PERMISSIONS and
the roles-editor sections, making the documented grant-outward path real.
Storage hardening: default-persona invariants move to a shared _utils
helper (validate + demote) with a pg advisory xact lock serializing
promotions and a post-promote single-default assertion; create maps the
unique-name race to the same ValueError as the pre-check; reads validate
JSON shape loudly (naming the persona); serialize enforces size caps;
field validation runs before invariant checks so malformed input is a 400,
never a TypeError-500. org_id guards explicit null and caps at 64.
Also: base_override='' means 'no override' at the compose boundary;
persona tag flattened/capped before the spawn approval header; /creative
redirect resolves the writer persona before advertising it; memory-nudge
gating unified through _nudges_enabled.
Provider/row-shape tests updated to the new contracts (the old ones
pinned the injection hole and the pre-persona row shape).
|
||
|
Patrick Buckley
|
80b8997b88 |
fix(projects): full-suite findings — type-guard the visibility gate, bind acting user without breaking send stubs
ws_visible only treats real strings as project links (a test double or corrupted value means no-project, not private-and-denied), the mgr-path project_id is coerced likewise, and the HTTP send path binds the acting user via a getattr-guarded bind_acting_user call inside the fresh-turn closure instead of a send() kwarg — per-kind session stubs with explicit send signatures keep working. Row-shape contract tests (interactive + coordinator twins) grow the intentional project_id key. |
||
|
Patrick Buckley
|
bc93b1f748 |
fix(cancel): address code-review findings before PR
The multi-stage review of this branch surfaced four major + two minor issues, three of them in the new cancellation code. All fixed here (bug-3, the stale generated TS SDK spec, stays deferred — it regenerates out-of-band). - sec-1: cancelling a coordinator now auto-cascades to its children, but the cancel route allows the service-scope bypass while the removed stop_cascade gated the same destructive subtree-cancel at no-bypass — a service token without admin.coordinator could trigger the cascade. Re-assert the no-service-bypass gate inside _cascade_cancel_to_children, so a plain cancel by an under-privileged service token still cancels the coordinator's own turn but no longer cascades. - bug-1: _cancelled_agent_disposition took the LAST issued tool call as the in-flight one. _run_agent executes a turn's calls sequentially, so the in-flight call is the FIRST unanswered one — taking the last inverted unknown/none on a multi-call turn (a SIGKILL'd bash mislabelled "not started", the never-run tail mislabelled UNKNOWN, inviting a re-run of the destructive call). Fixed to first-unanswered. - perf-1: the per-child cancel fan-out was awaited inline before the cancel's 200, so a cancel could block for tens of seconds on slow/unreachable children. Return the fan-out as a response BackgroundTask so it runs after the 200 (trigger, not drain). - bug-2: the initial-send worker (_run_initial) cleared _worker_running unconditionally — the same clobber the session_worker guard just fixed. Apply the identity guard there too. - sec-2: restore the per-child cascade audit row (coordinator.cancel_cascaded) the removed stop_cascade wrote; it had become log-only. - q-1: extract the shared UNKNOWN-outcome clause (UNOBSERVED_OUTCOME_CLAUSE) so the wire-repair fallback and the session-layer synthesis can't drift. |
||
|
Patrick Buckley
|
776430d860 |
feat(cancel): honest cancellation dispositions + coordinator subtree propagation
A cancelled agent previously discarded its own ledger and reported a bare "(task interrupted by user)" — fabricating the *outcome* (read downstream as "nothing happened"), which invites a double-send as readily as a dropped record causes an orphan. Make the fold-back honest, and propagate an owner's cancel down the coordinator subtree. - task_agent (single + parallel): on cancel, fold back a deterministic disposition built from the agent's in-memory ledger — actions completed, the in-flight action flagged outcome-UNKNOWN, and not-started calls — instead of the opaque interrupted string. - coordinator cancel now auto-propagates to its direct children via a post_cancel hook on the shared cancel handler (cooperative fan-out; no blocking drain). - synthesized cancelled tool results now read outcome-UNKNOWN rather than implying the call never ran. - remove the now-redundant stop_cascade operator endpoint (handler, route, OpenAPI spec + schema, tests, docs); a coordinator cancel supersedes it. |
||
|
Patrick Buckley
|
cc0fa53077 |
feat(coordinator): port Regenerate/Edit title to coordinators
Coordinators carry LLM/auto titles like interactive workstreams but had no way to regenerate or rename them. Port the interactive "Refresh title" (LLM regenerate) + "Edit title" (manual alias) dropdown actions by lifting the two handlers — the last shared verbs that weren't yet lifted — and opting coordinators in. - session_routes.py: add make_refresh_title_handler / make_set_title_handler factories (cfg pattern, mirroring make_close_handler). set_title resolves the workstream BEFORE the alias write and 404s when the kind has no tenant_check storage gate and the in-memory manager doesn't own it: set_workstream_alias is a global, kind-unscoped UPDATE, so this prevents an operator renaming a workstream the coord manager doesn't own (e.g. an interactive ws via the coord route) and the silent-200 on a bogus id. - server.py: re-point the interactive bundle to the lifted handlers; drop the standalone refresh_workstream_title / set_workstream_title. - console/server.py: wire refresh_title / set_title into the coord bundle (gated by the existing admin.coordinator operator check). - shell.js: enable titleVerbs on the coordinator pane's tab menu; the base-aware lane posts to the console-origin coord routes. Tests: coord refresh/set-title (regenerate, operator-gate, 404 unknown, alias store + broadcast, empty, conflict, cross-kind reject); interactive title tests re-pointed to the lifted handlers for lift-parity; shell.js coord-menu assertion. |
||
|
Patrick Buckley
|
1860d14a65 |
fix(coordinator): persist + eagerly generate workstream titles
Coordinator workstream LLM titles were written to workstreams.title but never read back, and were rarely generated in the first place: - Read path: the dashboard's `_coordinator_rows` builder hardcoded title="" and used the synthetic `ws.name`, so a generated title (or a user alias) reverted to `ws-xxxx` on every refresh. Interactive rows resolve via get_workstream_display_name, so the gap was coord-only. - Write path: the auto-title trigger only fired on a tool-call-free assistant turn, which coordinators (near-constant tool use) seldom reach — so the title almost never generated. Read path: - Project `title` + `alias` in list_workstreams (appended after user_id so existing positional fallbacks stay valid). `_coordinator_rows` resolves the display name (alias > title > name) for both lanes — live names via the bulk get_workstream_display_names (exact ids, no row cap), persisted rows from their own _mapping. - Seed the console pseudo-node fan-out with the resolved display name so a rehydrated coordinator shows its title in the live tree immediately (one bulk lookup instead of an N+1 over mgr.list_all()). Write path: - Fire auto-title right after the user turn is recorded in send(), gated on a real (non-wake, non-empty) user message, instead of waiting for the terminal tool-call-free turn. Applies to interactive + coordinator. - Snapshot self.messages in _generate_title since it can now run concurrently with the streaming turn. |
||
|
Patrick Buckley
|
701ae46c72 |
feat(attachments): native PDF + audio translators, accept on upload
PDF and audio attachments now work end-to-end on the native provider lanes; non-native lanes degrade to a placeholder (client-side fallback lands next). Capability flags are populated but not yet consumed by a wire-build gate. - providers: Anthropic PDF -> base64 document; OpenAI Responses PDF -> input_file; compat/Google inline_document_parts PDF -> placeholder (fixes the base64-as-text mangle); audio = input_audio passthrough on the compat lane (omni), defensive text placeholders on Anthropic + Responses - capabilities: supports_pdf on cloud Claude + OpenAI chat models; local/default/compat stay False (-> client-side fallback) - upload: classifier accepts pdf (32 MiB) + audio (25 MiB); endpoint multipart read cap raised to PDF_SIZE_CAP - hygiene: consolidate the duplicated upload classification into one attachments.classify_upload (+ UploadRejection); collapse AttachmentUploadHelpers to a single classify_upload callable - tests: PDF/audio translator shapes, capability flags, classify_upload |
||
|
Patrick Buckley
|
4927efe942 |
refactor: pre-push review — content-addressed upload buffer, GC dedup, security hardening
- Buffer (attachment_buffer.py): content-address staged bytes once and track the
per-(ws_id,user_id) references to them, so identical bytes staged from two tabs
dedupe to one copy yet neither scope's send can drop the other's pending upload
(the prior hash-only key let one overwrite the other). Single lock; add a public
clear() that replaces test reaches into the private store.
- GC: lift the byte-identical _release_attachment_refs out of both backends into one
dialect-agnostic storage/_utils.release_attachment_refs with a portable searched-
CASE single-query decrement (was one UPDATE per id in a Python loop).
- _format_messages_for_summary: mark by-reference vision results
({type:image, attachment_id}) as [image], not just inline image_url.
- Security: escape_like() the attachment_referenced_in_ws LIKE needle on both
backends; secrets.compare_digest for the output-guard operator-fence leak check.
|
||
|
Patrick Buckley
|
98cee4d20c |
feat(storage): content-addressed refcounted attachments + in-memory upload buffer
Replace the persisted pending/reserved/consumed upload lifecycle (and its orphan-sweep and per-user cap) with a content-addressed, refcounted blob store fronted by the per-node in-memory pending buffer: - Upload stages bytes in the buffer (keyed by sha256); send-commit drains the referenced handles, writes each blob content-addressed (INSERT-OR-IGNORE then refcount += 1, so a stored blob is born referenced and dedupes across messages/workstreams), and records the ordered conversations.attachments ref-list — the sole message->blob link. - reconstruct rebuilds inline image_url/document multipart content from the ref-list, role-agnostically (so tool-produced images via _exec_read_image now persist + rehydrate instead of being flattened to text and lost). Output shape unchanged. - GC is reference counting: delete_messages_after / delete_workstream decrement once per reference and prune a blob at 0; a deduped blob shared with a kept turn (or another ws) survives. - get_content for a committed blob is gated by reference-ownership (the requester owns a turn in the ws whose ref-list names the id), replacing the dropped ws_id/user_id scope. - Migration 060 re-keys legacy consumed attachments to their content hash, dedups, sets refcounts, writes the ref-lists, and drops message_id/reserved_*; pending legacy rows are dropped (pending now lives only in the buffer). Both backends symmetric; the reservation methods, cap, and orphan-sweep are removed across storage/facade/protocol/endpoints/coordinator. Wire harness byte-identical; full suite green. |
||
|
Patrick Buckley
|
e20aae732c |
feat(storage): add content-addressed attachment columns (additive)
Additive schema for the attachment cutover: workstream_attachments gains refcount + origin, conversations gains the attachments ref-list column (migration 060 + _schema in lockstep). Columns sit unused until the cutover, which fills them and retires the message_id/reserved_* upload-lifecycle in favour of a content-addressed, refcounted blob store keyed by the conversations ref-list. Also registers the coordinator test's backend via init_storage: the attachment handlers resolve storage through the global registry, so a bare SQLiteBackend left get_attachment hitting a stale default db — latent until the new column made the schema drift bite. |
||
|
Patrick Buckley
|
110d44b07e |
refactor(tools): remove man, math, and plan_agent built-in tools
`man` and `math` duplicated capabilities already reachable through `bash`; `plan_agent` is better expressed as a `task_agent` running a planning skill, and carried a large amount of special-case machinery (plan-review gate, refinement loop, per-kind model routing). Removing all three shrinks the tool surface and cuts per-call token cost. Also removed, as dead-once-the-tools-are-gone: - the `math` sandbox executor (`turnstone.core.sandbox`) and its `[sandbox]` extra; the eval analyst now runs bash-only - the read-only `AGENT_TOOLS` sub-agent tool set and the `agent` tool-metadata key (`task_agent`/`TASK_AGENT_TOOLS` retained) - the plan-review protocol end to end: the `on_plan_review` UI hook, `resolve_plan`, `POST /v1/api/plan` + `POST /v1/api/route/plan`, the `plan_review`/`plan_resolved` SSE events, and their Python SDK / TypeScript SDK / OpenAPI / frontend / Discord+Slack bindings - the `model.plan_alias` / `model.plan_effort` settings and the registry `plan_model` / `plan_effort` routing fields TOOLS 31->28, TASK_AGENT_TOOLS 13->11; COORDINATOR_TOOLS unchanged. BREAKING CHANGE: removes the `man`, `math`, `plan_agent` tools, the plan-review SSE/HTTP/SDK surface, and the plan_* model-routing settings from the experimental 1.6 line. |
||
|
Patrick Buckley
|
a2834349b0 |
feat(export): export workstream conversations as OpenAI messages JSON
Add a workstream conversation export on three surfaces, all sharing one
serializer (turnstone/core/export.py):
- `turnstone-admin export <ws_id> [--children] [-o FILE|-]` — offline,
direct-DB. `--children` bundles a coordinator's parent conversation
plus one JSON per child into a zip (parent.json + children/<id>.json,
no manifest).
- `GET /v1/api/workstreams/{ws_id}/export` — conversation-only file
download, mounted on both the node (interactive) and console
(coordinator) lifespans via `make_export_handler(cfg)`, reusing the
/history gate ladder (permission_gate, tenant_check, list_kind
cross-kind isolation) so ownership and isolation come for free.
- Web UI — an "Export conversation" item in the interactive per-tab
dropdown (scoped to that tab's workstream) and an Export button on the
coordinator appbar.
Format is OpenAI Chat Completions messages JSON (`{"messages": [...]}`),
built from `sanitize_messages(load_messages(repair=True))`. Persisted
reasoning is surfaced on assistant messages as a flat `reasoning_content`
field (the convention OpenAI-compatible inference servers use) via a
dedicated helper that runs before sanitize strips the internal
_provider_content lane. Attachments ride along as the standard image_url
/ inlined-document content parts.
Lets users get conversations out in a portable interchange format
(backup, fine-tuning datasets, sharing, interop) without lock-in.
Closes #613.
Non-obvious decisions:
- Single format (openai-json); children/zip is CLI-only. The HTTP
endpoint and web UI are conversation-only, keeping the served surface
— and its security surface (no child rows read through the coordinator
handler) — small.
- `reasoning_content`, not the `reasoning` field /history and the
reasoning-replay path use: export targets the chat-completions
convention. Documented in export.py to prevent a "consistency fix".
- list_workstreams exposes no cursor, so the child walk passes an
explicit high limit rather than inheriting the default 100, which
would silently drop a coordinator's children past 100.
- Interactive export lives in the per-tab menu (interactive is
per-tab/pane — avoids focused-workstream ambiguity); the coordinator
is one conversation, so it keeps an appbar button.
Tested: 25 new tests through real storage + handlers (TestClient), incl.
cross-kind isolation 404, misconfig 500, the reasoning + attachment
pipeline, and the coordinator children zip. The shared frontend helper
is verified by a node sandbox harness (re-entrancy guard, button
disable/aria-busy, no-button tab-menu path). Full non-live suite green
(6714 passed); ruff + format + mypy clean; OpenAPI spec updated.
|
||
|
Patrick Buckley
|
352a27915a |
feat(coord): per-coordinator status bar + richer history replay
Bring the coord dashboard toward parity with the interactive pane on two operator-visible surfaces: - Status bar pinned above the composer. Same four cells as the interactive pane (model, token / context-window usage with effort suffix, tool calls this turn, conversation turn) driven by the same on_status SSE events. ws-status-bar CSS hoisted from ui/static/style.css to shared_static/chat.css so both UIs read one copy. StatusBar.paint helper extracted to shared_static/status_bar.js; both Pane.prototype.updateStatus and the new coord updateStatusBar delegate to it so warn/danger thresholds, prefix glyphs, and effort-suffix rules can't drift. CTX_WARN_PCT / CTX_DANGER_PCT now named constants on a single line. - _coord_events_replay now yields the connected + status preamble via a shared session_replay_preamble helper in turnstone/core/session_replay.py. _interactive_events_replay routes through the same helper so a future field add lands once. Coord still skips conversation history in the SSE replay (the dashboard fetches it via GET /history); only the status preamble is shared. - History replay reconstructs tool calls. Pre-fix, an assistant turn that only dispatched tools rendered as an empty bubble followed by raw tool-result text — the call's intent and parameters were lost on reload. synthesizeHistoricalToolCall builds an appendToolCall-shaped item from the persisted function.name + function.arguments (special-casing bash so the shell line shows in the header). Tool result rows now resolve their label from the matching tool_call_id instead of always printing "tool". - onopen restores the tokens placeholder when no prior status was seen, so a transient SSE blip on a fresh coord doesn't leave the dim "Reconnecting…" copy stuck until the next live tick. Tests: 4 new tests for the shared replay preamble (connected first, status only when last_usage present, status payload shape, no-session fallthrough); existing approval/verdict ordering tests refactored through a shared make_replay_mocks helper in tests/_replay_helpers.py that both interactive and coord suites import. |
||
|
Patrick Buckley
|
39aa493d76 |
feat(console): per-call model + judge_model on coord composer (#440)
* feat(console): per-call model + judge_model on coord composer Brings the landing-page coordinator composer toward parity with the interactive new-ws modal — operators can now pick a model and judge model per session without round-tripping through the Models admin tab. - Add Model + Judge Model selects to the home composer's options panel, populated from /v1/api/models. Empty / non-string fields collapse to None so the factory falls back to ConfigStore defaults (coordinator.model_alias, judge.model). - _coord_create_build_kwargs threads the body fields onto mgr.create. - Console session factory accepts judge_model and overrides the JudgeConfig via dataclasses.replace, mirroring the server-side interactive factory's pattern (alias preserved for IntentJudge's provider/client resolution). - Sanitise the 503 factory-misconfig response across make_open_handler, make_create_handler, and make_detail_handler: a new _safe_factory_misconfig_message helper strips control characters and caps at 200 chars before echoing exc text. Operators still get the full alias in the warning log; clients see a bounded printable string. Defends the user-controlled body["model"] reflection surface on the create path. - _build_mgr_with_factory test helper extracted from _build_mgr so tests that need to capture factory kwargs don't reconstruct the CoordinatorAdapter + SessionManager scaffolding inline. - Tests cover: passthrough of model + judge_model, empty / whitespace / non-string body fields collapsing to None, and the 503 sanitiser truncating + scrubbing a hostile alias payload. * fixup: address PR #440 Copilot review - _safe_factory_misconfig_message: hard-cap return at _FACTORY_MISCONFIG_MAX_LEN total (was MAX_LEN+1 because the slice was MAX_LEN long with the ellipsis appended on top). Reserve one codepoint for the ellipsis so the cap is honoured. Update the regression test to assert the tighter bound. - Composer judge_model placeholder: "Default (agent model)" was misleading when ConfigStore judge.model is set — the actual fallback is judge.model when set, IntentJudge's agent-model fallback when not. Use "Default judge model" instead so the label matches both configs. |
||
|
Patrick Buckley
|
6f5cb33923 |
feat(coord): composer parity with interactive — stop/queue/attach (#438)
* feat(coord): composer parity with interactive — stop/queue/attach
Bring the coordinator one-pane UI to feature parity with the
interactive composer: in-composer Stop button replaces Send during a
turn, queue-while-busy with !!! priority + dismiss, paperclip attach
+ drag/drop/paste. The coord backend already supported all three
(lifted send/cancel/attachment handlers, emit_message_queued=True,
supports_attachments=True); this wires the UI through.
Backend:
- Wire make_dequeue_handler(coord_endpoint_config) so DELETE
/v1/api/workstreams/{ws_id}/send works for coord-kind workstreams.
- Add the matching OpenAPI EndpointSpec.
- Five new test_dequeue_* tests (success, not_found, missing msg_id,
unknown ws, scope gate) pin the URL/method/scope contract.
Frontend extraction:
- New shared modules composer_attachments.js (createAttachmentController)
and composer_queue.js (createQueueController) replace ~300 LOC of
pre-existing duplication between the interactive Pane and the coord
IIFE. Both panes now share one source of truth for the chip pipeline,
optimistic queue bubble, and busy-edge promote sweep.
Coordinator pane:
- Composer constructor adds attachments/stopBtn/queueWhileBusy/
busyPlaceholder/dragDrop options.
- setBusy now drives off SSE state_change (running/thinking/attention →
busy; idle/error → idle), with composer.setBusy unconditional and the
edge-only work (timer cleanup + queue.onIdleEdge) gated on the actual
transition.
- Cancel uses the in-composer Stop with a 2s "Force Stop" affordance +
10s safety auto-recover; the legacy header-mounted #coord-cancel-btn
is removed.
- coordCloseSession suspends SSE before close and re-establishes it on
any failure path so the UI never goes dark on a still-alive session.
- Race handling: bind() releases the queued slot server-side when the
bubble was already dismissed or promoted; rehydrate re-checks getWsId
in its .then so a stale-tab response can't clobber the new tab's
chips.
Interactive pane:
- Pane class adopts the same controllers via this.attachments /
this.queue. Pane.prototype.uploadAttachment, _renderAttachmentChip,
_swapPlaceholderChip, _removeAttachmentChip, removeAttachment,
rehydrateAttachments wrapper, addQueuedMessage, _dequeueMessage, and
_promoteQueuedMessages are all gone — the controllers own the state.
- setBusy collapses to the same shape as coord: composer.setBusy +
edge calc + queue.onIdleEdge on idle.
CSS:
- Move .msg-queued / .queued-badge / .queued-dismiss styles from
ui/static/style.css into shared_static/chat.css so both panes share
one rendering.
- Add .coord-drop-target overlay rule so the coord pane shows the
drag-and-drop affordance.
Tests pass: 160 in the impacted suites (coord endpoints + attachments
+ session routes), including 5 new dequeue tests for coord.
* fix(coord): Copilot review + lint follow-ups
Lint:
- ruff: cast(MagicMock, ...) → cast("MagicMock", ...) under
``from __future__ import annotations`` (UP037).
Copilot review (PR #438):
- composer_queue _sendDelete now invokes onAfterDequeue on success
so a bind() race-DELETE (queued bubble dismissed pre-bind or
promote sweep raced ahead) still rehydrates the caller's chip pile;
released attachment reservations no longer linger invisibly until
the next page load.
- Coord's createQueueController gains onAfterDequeue: attachments.
rehydrate(). The previous omission was a v2 review carry-over from
before coord supported attachments — now it does, so the same
contract as interactive applies.
- Both panes' send-response handler now accepts status:queued without
a queuedEl (SSE-not-yet-connected race on initial load): flips busy
so subsequent sends queue correctly. The current message keeps its
optimistic user bubble — accepted UX gap (no in-UI dismiss for
THIS message) since flipping a rendered user bubble into a queued
one mid-stream would be jarring.
- Doc updates: chat.css comment + composer_queue.js module docstring
refer to the renamed onIdleEdge() instead of the removed
promote()/promoteQueuedMessages.
|
||
|
Patrick Buckley
|
4d08a19bd5 |
fix(approve): replay cached LLM verdicts on coord SSE reconnect
The coord's _coord_events_replay re-yielded _pending_approval on connect but not the cached _llm_verdicts entries. A tab refreshing mid-approval saw the approve_request prompt without the judge chip because intent_verdict is a one-shot SSE event with no late-subscriber push — the chip would only ever land if the operator re-invoked the tool call. Mirrored the interactive path at turnstone/server.py:875-878: after re-injecting the pending_approval prompt, walk ui._llm_verdicts under _ws_lock and yield each cached verdict as an intent_verdict event. Pre-existing bug surfaced during the inline-child-approvals work but the coord-self dock UX was always affected on reconnect — not introduced by this PR. Two new tests: cached verdicts replay after pending_approval; stale verdicts from a prior round don't replay when no approval is pending. |
||
|
Patrick Buckley
|
7d2d7db9d2 |
feat(approve): pass pending_approval_detail through cluster live-bulk
Threads the field added by Chunk 1 through the console's live-bulk endpoint so coord tree UI can read it without a separate per-child fetch. Three touchpoints: - _CLUSTER_WS_LIVE_KEYS gains the new key so _fetch_live_block's projection forwards it from the upstream /dashboard response on node-backed child rows. - _coordinator_live_snapshot synthesizes the same shape from ConsoleCoordinatorUI._pending_approval for in-process coord rows (no upstream /dashboard exists on the console pseudo-node). - One source of truth: SessionUIBase.serialize_pending_approval_detail. Both branches now emit the same 12-key live block; coord judge isn't wired today so coord-self judge_verdict is always None — flagged in the plan as a stretch follow-up. Plan: docs/design/inline-child-approvals.md (chunk 2 of 4). |
||
|
Patrick Buckley
|
fbb9be27f9 |
feat(approve): expose pending_approval_detail on /dashboard + guard stale call_id
Lays the server-side groundwork for inline approve/deny buttons + judge verdict on the coordinator children-tree UI. Two surgical changes: 1. SessionUIBase.serialize_pending_approval_detail() merges the active _pending_approval items[] with per-call_id verdicts from _llm_verdicts. The dashboard handler embeds this on every per-ws row so cluster live-bulk callers can render inline UI without an extra per-child round-trip. 2. make_approve_handler now returns 409 when the body sends a call_id that doesn't match any currently-pending item. Closes the stale call_id race where an operator clicks approve on a row showing call A while the child has rolled over to call B. Empty/missing call_id preserves backwards compatibility with CLI + channel adapters that don't track it. Cross-tenant exposure on /dashboard is consistent with the trusted-team posture already in place for activity / tokens — documented in the new method's docstring so the choice survives the next reviewer. Plan: docs/design/inline-child-approvals.md (chunk 1 of 4). |
||
|
Patrick Buckley
|
6572437c5d |
refactor(server): rename dashboard row id → ws_id for v1 row-shape consistency
The /v1/api/dashboard endpoint was the last workstream-listing surface
keyed on `id` rather than `ws_id`. The Stage 2 list-verb lift converged
the active list (`/v1/api/workstreams`) and saved list
(`/v1/api/workstreams/saved`) on `ws_id` but explicitly left dashboard
alone to keep that PR's diff focused. This lands the same rename on
the remaining endpoint so v1 row shape is consistent across the family.
Scope kept narrow:
- Pydantic `DashboardWorkstream` and TS SDK `DashboardWorkstream`
interface both rename `id: str/string` → `ws_id`.
- The bundled web UI (`turnstone/ui/static/app.js`) is the only consumer
reading `dashboard.workstreams[].id` and is updated atomically.
- Console `_fetch_live_block` (cluster-inspect's projection over a
remote node's dashboard payload at `turnstone/console/server.py`)
flips its `entry.get("id")` lookup to `entry.get("ws_id")`.
- Drive-by: stale `id` example in `docs/api-reference.md` for the
earlier `/v1/api/workstreams` rename also fixed.
`_build_node_snapshot` (the global-events SSE node_snapshot payload
consumed by the cluster collector) deliberately stays on `id` — it's
part of a separate cluster-row family (collector → cluster_workstreams
→ console UI) that is internally consistent on `id` and would need its
own coordinated sweep. CHANGELOG documents the bounded blast radius.
Tests: 4554 passing (-m "not live"). ruff + mypy clean.
|
||
|
Patrick Buckley
|
d555816016 |
refactor(core): lift history + detail verb bodies across both kinds (Stage 2 verb lift)
Last verb-shape lift before v1.5.0 stable can tag. Adds two new
factories to ``turnstone/core/session_routes.py``:
- ``make_history_handler(cfg)`` — body lifted from coord's
``coordinator_history`` near-verbatim. ``?limit=`` query param
defaults to 100, clamps to [1, 500], malformed values fall back
to 100. Storage operations (``get_workstream`` on the
storage-fallback path, ``load_messages`` for the row read) now
run via ``asyncio.to_thread`` (was inline pre-lift on coord).
- ``make_detail_handler(cfg)`` — body lifted from coord's
``coordinator_detail``. Lazy-rehydrates a closed/evicted
workstream via ``mgr.open()`` on miss; mirrors
:func:`make_open_handler`'s exception envelope (``ValueError``
→ 503 with the session-factory's remediation text; bare
``Exception`` → correlation_id'd 500 with the per-kind noun
via ``cfg.audit_action_prefix``).
NO new ``SessionEndpointConfig`` fields — the factories reuse
``permission_gate``, ``manager_lookup``, ``not_found_label``,
``audit_action_prefix``, and (for history's storage-fallback
kind check) ``list_kind`` — all already wired by both production
lifespans for the list/saved factories.
Coord side: ``coordinator_history`` and ``coordinator_detail``
standalone handler bodies removed from ``console/server.py``;
``register_session_routes`` now wires
``history=make_history_handler(coord_endpoint_config)`` and
``detail=make_detail_handler(coord_endpoint_config)``.
Interactive side: GAINS both endpoints as a feature gain. Pre-lift
interactive had no ``GET /v1/api/workstreams/{ws_id}`` and no
``GET /v1/api/workstreams/{ws_id}/history`` — SDK consumers had to
subscribe to ``/events`` SSE just to read display fields or
message rows. The same lifted factories are wired with the
interactive endpoint config; cross-kind isolation is preserved on
both sides (history via ``cfg.list_kind`` storage-fallback gate
+ fail-loud-on-misconfig 500; detail via ``mgr.open()``'s internal
kind check).
Pydantic schemas: ``CoordinatorDetailResponse`` /
``CoordinatorHistoryResponse`` removed from ``console_schemas.py``;
``WorkstreamDetailResponse`` / ``WorkstreamHistoryResponse`` added
to ``server_schemas.py`` (mirrors the list lift's pattern for
``WorkstreamInfo``). Both server and console OpenAPI specs
reference the unified schemas; ``server_spec.py`` gains
``EndpointSpec`` entries for the new interactive endpoints. TS
SDK gains both interfaces in ``sdk/typescript/src/types.ts``;
``openapi-{server,console}.json`` regenerated.
Tests: 6 new coord regression/parity tests in
``test_coordinator_endpoints.py`` (limit clamping, cross-kind 404
on storage fallback, storage-only history, detail 503 on
session-factory misconfig, detail 500 with correlation_id on
unexpected rehydrate failure, history swallows
``load_messages`` exception → 200 with empty messages). 10 new
interactive parity tests in ``test_workstream_endpoints.py``
(``TestHistoryInteractive`` + ``TestDetailInteractive``). 1 new
openapi spec test pinning the server-side ``?limit=`` query param.
Total: ``4490 → 4491`` after the new exception-swallow
regression test landed. ``ruff check`` clean, ``mypy`` clean on
touched files.
/review pipeline (4 finders → verify → dedupe) caught 1 Minor
defense-in-depth (bug-1/sec-1, merged: ``make_history_handler``
fail-closed gate when ``cfg.list_kind is None``, mirroring
``make_saved_handler``'s same gate) + 1 Minor test-helper rename
(q-1: ``_interactive_history_cfg`` → ``_interactive_endpoint_cfg``)
+ 4 Nits (q-2 unused fixture parameter, q-3 CHANGELOG TS SDK
mention, q-4 missing exception-swallow regression test, q-5
misleading test comment) — all addressed in the same commit.
|
||
|
Patrick Buckley
|
edf52016ac |
refactor(core): lift list + saved verb bodies across both kinds (Stage 2 verb lift)
New ``make_list_handler(cfg)`` and ``make_saved_handler(cfg)``
factories in ``turnstone/core/session_routes.py`` replace four
pre-lift bodies (interactive ``list_workstreams`` +
``list_saved_workstreams``; coord ``coordinator_list`` +
``coordinator_saved``). Same factory + capability-flag pattern as
the merged cancel / open / events / create lifts.
Four new ``SessionEndpointConfig`` fields:
- ``list_resolve_titles: ListResolveTitles | None`` — bulk lookup
``(ws_ids) -> {ws_id: title-or-None}``. Interactive wires
``get_workstream_display_names`` (new bulk helper added on the
storage layer + memory.py); the lifted body resolves every active
row in ONE ``SELECT ... WHERE ws_id IN (...)`` instead of the
pre-lift N+1 (one SELECT per row).
- ``list_kind: WorkstreamKind | None`` — explicit kind classifier
for the saved-list storage filter. Replaces the initial draft's
``audit_action_prefix == "coordinator"`` string compare which
would have silently leaked INTERACTIVE rows for any future kind
whose audit prefix didn't match. Required when a kind mounts
list/saved; misconfig surfaces as a 500 with a clear log line.
- ``saved_state_filter: str | None`` — coord wires ``"closed"``;
interactive wires ``None``.
- ``saved_loaded_lookup: SavedLoadedLookup | None`` — coord-only
defence-in-depth filter that excludes ws_ids in the warm pool.
Behaviour changes (all observable in CHANGELOG):
- **Active-list row shape converges on always-include** ``{ws_id,
name, state, kind, parent_ws_id, user_id}``. Interactive renames
``id`` → ``ws_id``; both kinds populate every field (coord adds
kind + parent_ws_id; interactive adds user_id).
- **Top-level response key converges on ``"workstreams"``** on
both endpoints. Coord ``coordinators`` key removed — coord is a
1.5.0aN-only surface (never shipped stable) so the convergence
has no compat shim; SDK / frontend consumers swap once.
- **Storage + manager-lock work moved off the event loop on
interactive**. ``list_workstreams_with_history`` runs through
``asyncio.to_thread`` on both kinds (matches coord's pre-existing
perf-2 pattern from the saved-coordinators review); ``mgr.list_all``
+ per-row work also offloaded.
- **N+1 storage round-trips on /v1/api/workstreams eliminated**.
Pre-lift interactive resolved the alias for every active row in a
separate SELECT (up to 50 round-trips per dashboard refresh on a
saturated node). Lifted body issues one bulk SELECT.
Pydantic schemas: ``WorkstreamInfo.id`` renamed → ``ws_id``,
``WorkstreamInfo.user_id`` field added. ``CoordinatorInfo`` and
``CoordinatorListResponse`` removed (folded into the unified
``WorkstreamInfo`` / ``ListWorkstreamsResponse``). OpenAPI spec
snapshots regenerated. TS SDK types updated (``WorkstreamInfo``
interface gains ws_id + the always-include fields); TS test
mock + assertion updated to match.
``GET /v1/api/dashboard`` is intentionally NOT in this PR's scope
and still returns rows keyed on ``id``. Tracked as a separate
cleanup PR (tombstone-note added at the dashboard handler).
/review pipeline run; the four Major findings + one Minor + six
nits all addressed in the same commit:
- M1: TS SDK ``WorkstreamInfo`` interface stale (id: string) →
renamed + fields added.
- M2: TS SDK test masked the type-mismatch with stale mock → updated.
- M3: N+1 alias resolution on active list → bulk
``get_workstream_display_names`` helper + ``list_resolve_titles``
bulk cfg hook.
- M4: Missing interactive parity regression test for unified row
shape → mirror of coord's added in test_server_authz.py.
- Mi1: ``audit_action_prefix`` string-compare deriving kind →
explicit ``cfg.list_kind: WorkstreamKind`` field.
- Six nits: redundant inner asyncio import, forward-ref quotes on
Awaitable, duplicated frontend comments, dashboard ``id`` field
has no tombstone-note, empty-coord_mgr short-circuit on
``saved_loaded_lookup``.
4512 tests passing; ruff + mypy clean.
|
||
|
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
|
16916dc257 |
fix(core,console): coord create-time attachments coordination + Copilot review feedback on PR #416
Coord initial-message + create-time-attachments coordination: - ``CoordinatorAdapter.send`` gains optional ``attachments`` + ``send_id`` kwargs so the worker dispatched at create time can carry the uploaded files onto the first turn. Mirrors interactive's pre-existing worker-thread pattern. The ``send_id`` reservation token soft-locks the rows; the adapter's failure path unreserves so a worker crash returns them to pending. - ``_coord_create_post_install`` reserves any uploaded ``attachment_ids`` via the lifted ``reserve_and_resolve_attachments`` helper before dispatching through the adapter — closes the parity gap with interactive's create-with-attachments+initial_message flow. - ``_reserve_and_resolve_attachments`` lifted from ``turnstone/server.py`` to ``turnstone/core/attachments.py`` as ``reserve_and_resolve_attachments`` so both processes use one kind-agnostic implementation. Copilot review fixes on PR #416: - Skill lookup now calls ``storage.get_prompt_template_by_name`` directly rather than going through ``turnstone.core.memory.get_skill_by_name``; that helper swallows storage exceptions into ``None`` which would have masked outages as the 400 "Skill not found" branch. Calling storage directly lets exceptions bubble to the lifted body's correlation_id'd 500 path so operators chasing skill-related reports can distinguish real misses from registry outages. - ``_interactive_create_build_kwargs`` / ``_coord_create_build_kwargs`` thread ``skill_data["name"]`` (the canonical row name) into ``mgr.create`` instead of the raw ``body["skill"]`` value. Pre-fix a whitespace-padded request body ``"skill": " my-skill "`` would have persisted the dirty name even though the lookup ran on the stripped key. - ``make_create_handler`` docstring corrected: audit-emit failures return 200 (not 201). - ``_audit_workstream_created`` docstring corrected: factory keeps the successful 200 response on audit-emit failure (was 201). New regression test: ``test_create_with_multipart_attachments_and_initial_message_reserves`` asserts attachments are reserved (not pending) when both ``initial_message`` and uploads land in the same coord create request. Updated ``_SendSession`` stub in ``test_coordinator_adapter.py`` to match the new ``send`` / ``queue_message`` signatures. 4501 tests passing; ruff + mypy clean. |
||
|
Patrick Buckley
|
9ed8b1e0b5 |
refactor(core): lift create verb body across both kinds (Stage 2 verb lift)
New ``make_create_handler(cfg, *, audit_emit=None)`` factory in ``turnstone/core/session_routes.py`` consumes five new ``SessionEndpointConfig`` fields (``create_supports_attachments``, ``create_supports_user_id_override``, ``create_validate_request``, ``create_build_kwargs``, ``create_post_install``) and replaces both ``create_workstream`` and ``coordinator_create`` bodies. Same factory + capability-flag pattern as the merged cancel / open / events lifts. ``_validate_and_save_uploaded_files`` lifted to ``turnstone.core.attachments`` so both processes call one kind-agnostic implementation. Coord parity gains (§ Post-P3 reckoning item #1 + carry-forward): - Create-time attachments: multipart parsing, validate+save+rollback, ``attachment_ids`` on the response. Coord adapter ``send`` doesn't yet reserve attachments at create time, so the rows save as pending and the next ``/send`` picks them up via the standard send-with-attachments path. - Disabled-skill rejection (matches interactive's pre-lift gate). - Always-include response shape ``{ws_id, name, resumed, message_count, attachment_ids}`` populated with default ``False``/``0``/``[]`` on the fields coord doesn't fill. - 200 status (was 201). - Audit-emit failures swallow + warning log instead of 500. Both kinds converge on the manager-at-capacity 429, factory-misconfig 503, and correlation_id'd 500 for unexpected ``mgr.create`` failure (interactive lifted up to coord's safer error envelope). Three /review fixes folded in: - ``notify_targets`` malformed input gates at the validator (400) instead of bubbling out of post_install as a 500 — pre-fix the workstream had already been created + audited + broadcast by the time the validation raised. - Skill-lookup storage failures now share the correlation_id'd 500 path with ``mgr.create`` (was masquerading as 400 "Skill not found"). - Whitespace-only ``skill`` field treated as empty (matches pre-lift coord). CHANGELOG entry under [Unreleased] documents every observable behaviour change. OpenAPI spec regenerated. Three new coord regression tests (create-time-attachments save pending rows, always-include parity fields, disabled-skill rejection) plus one interactive regression test (notify_targets 400). 4500 tests passing. |
||
|
Patrick Buckley
|
577ad2824f |
refactor(core): lift events verb body across both kinds (Stage 2 verb lift) (#415)
* refactor(core): lift events verb body across both kinds (Stage 2 verb lift)
The interactive ``GET /v1/api/events?ws_id=...`` and coord
``GET /v1/api/workstreams/{ws_id}/events`` SSE handlers now share
one body via ``make_events_handler(cfg)``. Per-kind divergence
captured by two new ``SessionEndpointConfig`` fields:
* ``events_replay: EventsReplay | None`` — Protocol-typed callback
that yields the kind-specific initial replay payload. Interactive
wires ``_interactive_events_replay`` (connected + status + history
+ pending_approval + cached intent verdicts + pending_plan_review);
coord wires ``_coord_events_replay`` (just pending_approval +
pending_plan_review). The lifted body iterates the callback
before starting the live event loop.
* ``sse_executor_lookup: SseExecutorLookup | None`` — per-kind
executor for the live loop's blocking ``client_queue.get``.
Interactive returns the dedicated 200-thread ``sse_executor``
from app state so SSE polling stays isolated from every other
``asyncio.to_thread`` caller in the process; coord returns
``None`` and the lifted body falls through to the default executor.
Also adds ``make_legacy_query_keyed_adapter(handler)`` (sister to
``make_legacy_body_keyed_adapter`` from earlier lifts): reads
``ws_id`` from the query string and splices into ``request.path_params``
before delegating to the lifted body. Preserves the
``GET /v1/api/events?ws_id=...`` legacy URL shape so any 1.x SDK
consumer keeps working.
Old ``events_sse`` (server.py) + ``coordinator_events``
(console/server.py) bodies deleted.
Two convergence wins for coord:
* **SSE connect/disconnect metrics** — pre-lift coord didn't record
per-stream metrics; the lifted body always calls
``metrics.record_sse_connect()`` / ``record_sse_disconnect()``,
giving the cluster dashboard the same per-stream observability
interactive's had since 1.0.
* **Both kinds now check ``request.is_disconnected()`` AND the
``ws_closed`` event** to terminate. Pre-lift interactive relied
solely on ``ws_closed`` (which never fires if the client just
goes away without closing the workstream); pre-lift coord relied
solely on ``is_disconnected``. The lifted body uses both.
One observable shape change for coord callers: the lifted body
returns 409 ``"session has no UI"`` when ``ws.ui`` is missing
(placeholder / build-failed UI), matching pre-lift coord.
Pre-lift interactive returned 404 in this case; the lift converges
on 409 because the workstream EXISTS (404 would imply it doesn't).
Item #2 from § Post-P3 reckoning (rich ``ws_state`` payload parity
for coord) split out during scoping — touches different files
(``coordinator_ui.py`` + ``collector.py`` + ``session_ui_base.py``)
with different reviewer concerns. Tracked as standalone follow-up
``feat/coord-rich-ws-state-payload``.
Two /review fixes folded in:
* **Dedicated SSE thread pool restored.** Initial draft used
``asyncio.to_thread`` (default executor, ~32 workers). Pre-lift
interactive deliberately used a dedicated 200-thread
``sse_executor`` to avoid pool starvation; the
``sse_executor_lookup`` cfg field above restores that isolation.
* **5s poll timeout restored.** Initial draft shortened to 1s,
multiplying thread-wakeup rate 5x while the pool was already
starving. ``is_disconnected()`` between polls covers cancel-
detection latency.
Plus minor cleanups: stale ``coordinator_events`` comment
references in coordinator.js refreshed; ``TestInteractiveEventsLifted``
gets a ``_make_interactive_replay_mocks`` fixture so per-test
intent stays clear; live-loop coverage gap documented in the
test class docstring.
Lint + mypy clean. 4497 tests passing (+8 new events tests).
* fix(core): stream events replay from inside the generator instead of pre-building
PR #415 review caught that ``make_events_handler`` pre-built the
full replay payload (``connected`` + ``status`` + ``history`` +
pending prompts) into a list before constructing the
``EventSourceResponse``. Two real costs:
* **TTFB delay** — the client saw nothing until the heaviest
replay event finished serialising (``_build_history`` on a
long-running interactive workstream can take 10s of ms). With
pre-build, the ``connected`` event was buried at the end of
the materialisation pass instead of streaming first.
* **Listener-queue accumulation** — registering the per-UI
listener BEFORE building the replay let live events queue
during the build window. On a chatty mid-generation
workstream that window can fill the 500-slot listener queue
and drop events before the live loop starts draining.
Fix: iterate ``cfg.events_replay`` inside the async generator
so each event ships as soon as the callback yields it. The
observational-failure swallow semantics are preserved by
wrapping the iteration in the same try/except + log.debug as
before — partial replay is still acceptable; the live loop
continues either way.
Resolves the Copilot review thread on PR #415. Lint + mypy
clean. 4497 tests passing (no test changes — the replay
callbacks themselves are unchanged; only the lifted body's
consumption pattern flipped from eager-build to lazy-stream).
|
||
|
Patrick Buckley
|
f9ed4d3071 |
refactor(core): lift open verb body across both kinds (Stage 2 verb lift) (#414)
* refactor(core): lift open verb body across both kinds (Stage 2 verb lift)
The interactive ``POST /v1/api/workstreams/{ws_id}/open`` and coord
``POST /v1/api/workstreams/{ws_id}/open`` handlers now share one
body via ``make_open_handler(cfg, *, audit_emit=None)``. Per-kind
divergence captured by two new ``SessionEndpointConfig`` fields:
* ``open_resolve_alias: AliasResolver | None`` — interactive wires
``resolve_workstream`` so callers can pass user-friendly aliases
in the path param. Coord wires ``None``.
* ``open_post_load: OpenPostLoad | None`` — interactive wires
``_interactive_open_post_load`` (display-name sync + UI replay
via ``clear_ui`` + history + handler-side ``ws_created`` enqueue
onto the global SSE queue). Coord wires ``None`` and relies on
the cluster collector fan-out from
``CoordinatorAdapter.emit_rehydrated``.
Plus an optional ``audit_emit`` parameter (interactive wires
``_audit_workstream_opened``; coord wires ``None`` — coord doesn't
audit open today). Old ``open_workstream`` (server.py) +
``coordinator_open`` (console/server.py) bodies deleted.
**Load-bearing fix** (§ Post-P3 reckoning item #3 from the planning
docs): pre-lift interactive's ``open_workstream`` called
``mgr.create(ws_id=resolved_id)`` + ``ws.session.resume(...)`` to
rehydrate, bypassing ``mgr.open()`` entirely. After the lift both
kinds route through ``mgr.open()`` — which makes
``InteractiveAdapter.emit_rehydrated`` reachable on interactive
(it had been dead-by-routing) and gives the manager a single
rehydrate code path to maintain. ``emit_rehydrated`` stays a
documented no-op stub on the interactive adapter; the handler-side
``ws_created`` enqueue from the post-load callback is the
load-bearing emission for the SSE consumers.
Behaviour changes for interactive callers (documented in CHANGELOG):
* **Cross-kind open returns 404** (was 400 with
``"Workstream is not an interactive kind"``). The lift consolidates
on ``mgr.open()``'s single ``None``-return contract for missing /
wrong-kind / tombstoned rows. Security boundary unchanged.
* **Already-loaded response uses ``ws.name`` directly** (was
``get_workstream_display_name(resolved_id) or resolved_id``).
The dashboard listing endpoint still resolves aliases on its own
pass, so the user-visible name in the tab strip isn't affected.
Two /review fixes folded in:
* **Resume failures now return 5xx instead of broken-200.**
``SessionManager.open()`` previously caught and ``log.debug``-
swallowed exceptions from ``ChatSession.resume``. Since
``ChatSession.resume`` assigns ``self.messages`` *before* the
config-restore block, a partial-failure resume (corrupted
``workstream_config`` row, model-registry mismatch on a saved
alias, malformed ``temperature`` / ``max_tokens``) would leave
the session with history but with default config. Pre-lift the
interactive open handler called ``ws.session.resume`` directly
and let exceptions propagate as 500. Restored that behaviour:
``mgr.open()`` now re-raises resume exceptions after rolling
back the slot (``cleanup_ui`` + ``_remove_locked``), so the
lifted handler returns 500 with a correlation id and the storage
row stays available for a retry.
* **Bare ``except Exception`` documents intent.** A one-line
rationale in the handler body explains why the catch is broad
(no documented exception spec on ``adapter.build_session``;
resume can propagate via the new contract above). Keeps a future
contributor from narrowing it incorrectly.
Test scaffolding:
* ``tests/test_workstream_endpoints.py`` — fixture rebuilt to
use ``make_open_handler`` + a minimal cfg with a lazy alias
resolver so per-test ``@patch`` calls take effect. Added 5 new
tests: already-loaded uses ws.name, alias resolution runs first,
``mgr.open`` is called (NOT ``mgr.create``), post-load callback
fires with (request, ws) only on the load-from-storage path
(not the already-loaded shortcut), post-load exception swallowed
→ 200.
* ``tests/test_coordinator_endpoints.py`` — fixture imports
updated to ``make_open_handler``.
* ``tests/test_server_authz.py`` — ``TestOpenKindGate`` now expects
404 (not pre-lift's 400) for cross-kind open attempts. Docstring
explains the consolidation.
Two nit cleanups: dropped the unnecessary ``import secrets as
_secrets`` aliasing in the exception handler; refreshed the stale
``open_workstream`` reference in the ``AliasResolver`` doc-comment.
Lint + mypy clean. 4488 tests passing (was 4475; +13 new open
tests).
* fix(core): use cfg.audit_action_prefix for the per-kind noun in open's 500 error
PR #414 review caught the hardcoded ``"failed to open workstream"``
in ``make_open_handler``'s 500 path: coord callers got misleading
text (pre-lift coord said ``"failed to open coordinator"``).
The fix derives the noun from ``cfg.audit_action_prefix``
("workstream" interactive, "coordinator" coord) — a field both
production lifespans already construct, and which the previous
/review pipeline (q-5) flagged as dead config (set but read by
no factory). Reusing it here both fixes the wording AND gives
the field its first runtime reader.
Pinned by a new test
(``test_open_500_message_uses_kind_noun_from_cfg``) that wires a
coord-shaped cfg, forces ``mgr.open`` to raise, and asserts the
500 body contains ``"failed to open coordinator"`` + the
correlation id, without echoing the exception text.
Lint + mypy clean. 4489 tests passing (+1 new).
|
||
|
Patrick Buckley
|
412c99f486 |
refactor(core): lift cancel verb body across both kinds (Stage 2 verb lift) (#413)
* refactor(core): lift cancel verb body across both kinds (Stage 2 verb lift)
The interactive ``/v1/api/cancel`` (body-keyed ws_id) and coord
``/v1/api/workstreams/{ws_id}/cancel`` (path-keyed) handlers now
share one body via ``make_cancel_handler(cfg, *, audit_emit=None)``
in ``turnstone.core.session_routes``. Per-kind divergence captured
by a new ``cancel_forensics: CancelForensics | None`` field on
``SessionEndpointConfig`` (interactive wires
``_capture_cancel_forensics``; coord wires ``None``) plus an
optional ``audit_emit`` (coord wires ``_audit_cancel_coordinator``;
interactive wires ``None`` — pre-lift interactive didn't audit
cancel).
Same factory + capability-flag pattern as P1.5's ``make_send_handler``
+ make_attachment_handlers. Old ``cancel_generation`` body deleted
from ``server.py``; old ``coordinator_cancel`` body deleted from
``console/server.py``.
Behavior changes (documented in CHANGELOG):
* **Coord gains the ``force`` flag.** Pre-lift coord ignored
``force``; the lifted body honours it on both kinds. Stuck-worker
recovery becomes available on coord (parity gain — coord workers
hang the same way interactive's can).
* **Coord cancel response always includes ``"dropped"``.** Pre-lift
returned bare ``{"status": "ok"}``; lifted returns
``{"status": "ok", "dropped": {}}``. Always-include parity with
interactive so SDK consumers don't branch on kind.
* **Coord cancel returns 400 ``"No session"``** on placeholder /
build-failed workstreams (was a silent 200 no-op pre-lift). Parity
with interactive's existing 400 branch.
* **Coord ``coordinator.cancel`` audit detail now includes
``force``** so operator-driven recovery is distinguishable from
routine cancels.
Three /review fixes folded in:
* **bug-1**: lifted body's ``resolve_approval`` is now gated on
``ui._pending_approval is not None``. Pre-fix, the unconditional
call leaked a stale ``approval_resolved`` SSE event on every
idle cancel — listener UIs that key on the event would dismiss
prompts they didn't have. ``resolve_plan`` keeps its existing
internal no-pending guard so the unconditional call is still
safe there.
* **bug-2**: force-cancel now clears ``_worker_running`` alongside
``worker_thread`` inside the same ``with ws._lock`` block. Prior
half-state ``(_worker_running=True, worker_thread=None)`` routed
follow-up sends through the queue-enqueue path onto the abandoned
worker (whose cancel flag short-circuits the queue-drain seam,
leaving messages orphaned until next spawn). Restores the
``(worker_thread, _worker_running)`` invariant
``session_worker.send`` documents.
* **bug-3**: ``coordinator_stop_cascade._fanout_on_children`` now
treats child cancel ``400 + "No session"`` as ``skipped`` (was
``failed``). Lifted coord cancel returns 400 on placeholder
children; matches the pre-lift outcome where those children were
silently no-op'd, so the cascade response's ``failed`` bucket
stops firing spurious operator alerts.
Test scaffolding:
* ``tests/test_coordinator_endpoints.py`` — replace ``coordinator_cancel``
fixture with ``make_cancel_handler(...)`` wiring; add 6 new
tests covering always-include shape, force-flag worker-abandon,
400-on-null-session, cancel_forensics swallowed-exception,
audit_emit swallowed-exception, no-stale-approval-resolved-on-idle.
* ``tests/test_server_authz.py`` — new ``TestInteractiveCancelLifted``
class with HTTP-level coverage of ``/v1/api/cancel`` for the
dropped shape, force-flag + ``_worker_running`` clearing, and
400-on-null-session. Pre-lift ``cancel_generation`` had no
HTTP-level test; this is the first.
One observable change for interactive (pre-existing call site):
``resolve_approval`` / ``resolve_plan`` now run on every cancel
regardless of ``was_running`` (was gated). Lifts coord's
unconditional behaviour onto interactive — a stuck approval-pending
state from a crashed worker can now be cleared via cancel without
requiring close + rehydrate.
Lint + mypy clean. 4484 tests passing (was 4475; +9 new cancel
tests minus the moved one that became part of the new suite).
* docs(core,changelog): correct cancel-lift behaviour description for resolve_approval
Two review comments on PR #413 caught the same drift between the
implementation and its documentation: my bug-1 fix gated
``resolve_approval`` on ``_pending_approval is not None`` (because
it broadcasts ``approval_resolved`` unconditionally), but the
``make_cancel_handler`` docstring and the CHANGELOG entry still
claimed both ``resolve_approval`` and ``resolve_plan`` "run on
every cancel" and "the calls are idempotent and no-op when
nothing is blocked".
Reality:
* ``resolve_plan`` does run on every cancel and its no-op-when-
nothing-pending behaviour is real (the method has an internal
``_pending_plan_review is None`` short-circuit).
* ``resolve_approval`` runs only when ``ui._pending_approval is
not None``. Without the gate, every idle cancel would broadcast
a stale ``approval_resolved`` SSE event and overwrite
``_approval_result``.
Updated:
* ``make_cancel_handler`` docstring (turnstone/core/session_routes.py
in the "Behavior changes vs the pre-lift handlers" section) —
splits the two methods into separate bullets, explains why
``resolve_approval`` is gated and ``resolve_plan`` isn't.
* CHANGELOG.md ``[Stage 2 Verb Lift — cancel]`` entry — same
split + rationale; the asymmetric coord pre-lift parity is
still flagged as the recovery path that drove the lift.
Docs-only change; lint + mypy clean; cancel test suite (59 tests)
unchanged.
* style(core): replace CancelForensics ellipsis stub with docstring
github-code-quality bot flagged the ``...`` body of
``CancelForensics.__call__`` as "Statement has no effect". The
ellipsis is the canonical Protocol method-body idiom (no real
issue), but switching to a one-line docstring satisfies the bot
AND adds a small piece of method-level documentation. The class-
level rationale (why Protocol-typed instead of a plain Callable
alias) moves from a wall of leading ``#`` comments into a proper
class docstring at the same time.
Style-only change; the Protocol semantics are identical.
|
||
|
Patrick Buckley
|
ad56192a96 |
fix(core,console): address /review feedback on Stage 2 P1.5
Six fixes from the local /review pipeline (find-bug + find-security +
find-quality, all confirmed by verify):
* **sec-1 (major)** — coord ``attachment_owner_resolver`` now
resolves through ``coord_mgr.get(ws_id)`` only and does NOT fall
back to storage. Without the kind-strict check, an
``admin.coordinator``-scoped caller could pass an *interactive*
workstream ws_id to the new coord attachment endpoints; the
generic ``get_workstream_owner`` storage call (kind-agnostic)
would resolve and grant cross-kind read / write access to
interactive attachments. New regression test
``test_coord_attachment_endpoints_404_on_interactive_ws_id``
pins the surface.
* **bug-1 (minor)** — UI hook calls in the spawn-path ``_run``
closure are now wrapped per-hook (via ``_emit_ui``) so a failure
in ``ui.on_error`` doesn't suppress the subsequent
``ui.on_stream_end`` / ``ui.on_state_change`` calls. Mirrors the
pre-P1.5 coord_adapter.send per-hook defense.
* **bug-2 (minor)** — ``make_dequeue_handler`` now 404s when
``ws.ui is None`` (preserves the pre-P1.5 ``_get_ws`` contract;
a partially-constructed or close-window workstream shouldn't
answer DELETE).
* **bug-3 (minor)** — ``coordinator.js`` gains a
``case "message_queued":`` handler that surfaces the queueing
as an info row. Coord wires ``emit_message_queued=True`` for
parity with interactive but the dashboard had no router branch
for these events, silently dropping them.
* **bug-4 (minor)** — error-message format on coord regressed
from ``f"{type(exc).__name__}: {exc}"`` to ``f"Error: {e}"``
(lost the exception class name, which coord operators rely on
to triage failures). Restored.
* **q-1 (major)** — duplicate ``_auth_user_id`` and
``_require_ws_access`` helpers in ``server.py`` and
``console/server.py`` now delegate to the lifted
``turnstone.core.web_helpers.auth_user_id`` /
``resolve_workstream_owner``. The lifted versions are the
canonical implementations; the shims keep existing call sites
working without a sweeping rename.
CHANGELOG entry adds a Security section noting the kind-strict
resolver fix and a behaviour callout for the cancel-state semantic.
|
||
|
Patrick Buckley
|
e0c78e2aec |
test,docs: coord attachment + queue parity tests + spec regen + CHANGELOG
Five new TestCoordinatorAttachments tests in ``tests/test_coordinator_endpoints.py`` exercising the lifted attachment surface end-to-end on coord: * upload → list round-trip * get_content returns raw bytes with text/plain forced for text * delete removes pending entries and clears them from the listing * send with attachment_ids consumes pending under the send_id token * send response carries attached_ids / dropped_attachment_ids even on plain-text sends (unified shape parity) The existing ``_coord_endpoint_config`` fixture grew capability flags to mirror the production console wiring, and ``_make_client`` now mounts the four coord attachment routes via ``make_attachment_handlers``. OpenAPI specs regenerated; TS SDK bumped to 0.5.0. CHANGELOG entry under [Unreleased] documents the verb-shape lift, the coord attachment surface coming online, the response-shape change for ``coordinator_send``, the unification of the three lifted classifier / lock helpers under ``turnstone.core.attachments``, and the new SDK helpers. |
||
|
Patrick Buckley
|
74670cd53e |
refactor(server): apply 2nd-pass /review fixups
Addresses the eight verified findings from the second review pass on the body-convergence work (one bug-flagged behavior change, one defensive-style nit, six quality items). One quality item (q-6, ``request.scope[\"path_params\"]`` mutation in the legacy adapter) is documented but not refactored — restructuring the lifted handler signatures to take ``ws_id`` as an explicit param is bigger than this fixup's scope; the adapter docstring already explains the choice. Findings addressed: - **bug-1 + q-5**: hoist module-level ``log = get_logger(__name__)`` in ``session_routes.py``; bump audit-failure log from ``debug`` to ``warning`` (compliance signal). Document the interactive 500-on-audit-failure → 200+log behavior change in CHANGELOG + in ``make_close_handler``'s docstring. - **bug-2**: switch ``_audit_close_workstream`` to ``getattr(request.app.state, \"auth_storage\", None)`` for consistency with the upstream gate. Same fix on coord side. - **q-1**: pass ``SessionEndpointConfig`` into ``make_approve_handler(cfg)`` and ``make_close_handler(cfg, *, audit_emit, supports_close_reason)`` via closure capture. Removes the implicit ``app.state`` contract and parallels the two factory signatures. Tests + production wiring updated. - **q-2**: promote ``_audit_close_coordinator`` to a module-level function in ``turnstone/console/server.py``. Both test fixtures import it instead of duplicating the body. The previous three near-identical implementations collapse to one. - **q-3**: lift ``_interactive_tenant_check`` and ``_audit_close_workstream`` from nested ``create_app`` closures to module-level functions in ``turnstone/server.py``, beside the other ``_audit_*`` / ``_require_*`` helpers. Add ``_interactive_manager_lookup`` so the config doesn't need a lambda. ``create_app`` shrinks accordingly. - **q-4**: merge the bottom ``if TYPE_CHECKING`` block into the one at the top of ``session_routes.py``. - **q-7**: replace ``assert mgr is not None`` with ``mgr = cast(\"SessionManager\", mgr_opt)`` in both lifted handlers — survives ``python -O`` and makes the type-checker-only intent explicit. - **q-8**: update ``test_coordinator_endpoints.py`` file docstring to mention the lifted-handler wiring. ruff + mypy + 4366 pytest pass. Live console smoke against the unified URLs returns 503 (no coord_mgr in smoke env) — proves the factory-captured config is reachable + manager_lookup fires. CHANGELOG ``[Unreleased]`` entry expanded to flag the audit-failure swallow as an interactive behavior change alongside the existing 500→404 standardization. |
||
|
Patrick Buckley
|
06c91294a4 |
refactor(server): lift close handler into shared session_routes body
Stage 2 Priority 0 Step 0.2 body-convergence — second verb.
``make_close_handler(audit_emit=..., supports_close_reason=...)``
factory in ``turnstone/core/session_routes.py`` produces the lifted
body; both interactive and coord pass their kind-specific audit
emitter at app construction.
The two body-keyed close URL aliases on the interactive side reach
the same lifted body:
- ``POST /v1/api/workstreams/{ws_id}/close`` (new, path-keyed)
via ``register_session_routes(handlers.close=...)``.
- ``POST /v1/api/workstreams/close`` (legacy, body-keyed) via
``make_legacy_body_keyed_adapter(close_handler)``.
Coord exposes only the path-keyed shape.
Behavior gains:
- ``supports_close_reason=True`` (interactive only) keeps the 512-
byte UTF-8 cap + credential redaction + ``workstream_config``
persistence path. Coord stays at ``False``; if coord ever wants
close-reason metadata, flipping the flag is a one-line change.
- ``audit_emit`` is per-kind so each owns its detail dict shape
(``{kind, parent_ws_id, reason}`` vs ``{coord_ws_id, src}``) and
audit action name (``workstream.closed`` vs ``coordinator.close``).
- Standardizes the close-failure status code to 404 across both
kinds. The coord code previously returned 500 on a
``mgr.close()`` race-loss, which was overly pessimistic — the
semantic is "the ws was popped between .get() and .close()", i.e.
not-found.
Coord-side test fixtures (``test_coordinator_endpoints``,
``test_coordinator_end_to_end``) swap the imported
``coordinator_close`` for the lifted handler + a local audit_emit
adapter so the tests exercise the same code path the live console
does.
ruff + mypy + 4366 pytest pass. Live console smoke against
``POST /v1/api/workstreams/abc/close`` returns 503 (no coord_mgr
loaded in the smoke env) — proves the lifted handler is reachable
+ the manager_lookup callable fires correctly.
Two verbs converged so far (``approve`` + ``close``); the remaining
pairs (``send``, ``cancel``, ``open``, ``events``, ``create``,
``list``, ``saved``, ``history``, ``detail``) have substantive
behavior divergence that doesn't factor cleanly into the
SessionEndpointConfig + factory-handler pattern — see the
session_routes module docstring for the per-verb status.
|
||
|
Patrick Buckley
|
6415eeb91e |
refactor(server): lift approve handler into shared session_routes body
Stage 2 Priority 0 Step 0.2 body-convergence — first verb. Both
interactive ``approve`` and coord ``coordinator_approve`` handler
bodies collapse into ``make_approve_handler()`` in
``turnstone/core/session_routes.py``. Each kind sets a
``SessionEndpointConfig`` on ``app.state`` carrying the kind-
specific policies (auth gate, manager lookup, tenant check, audit
prefix, not-found label) the lifted body consults at request time.
The two interactive URLs converge:
- ``POST /v1/api/workstreams/{ws_id}/approve`` (new, path-keyed)
reaches the lifted body directly via ``register_session_routes``.
- ``POST /v1/api/approve`` (legacy, body-keyed) keeps shipping;
``make_legacy_body_keyed_adapter`` peeks the body for ``ws_id``,
splices it into ``request.path_params``, and forwards to the same
lifted body. Frontend can keep using the legacy URL — no caller
churn.
Coord exposes only the path-keyed shape (its URLs were experimental
in 1.5.0aN; the URL-shape commit already removed the ``coordinator/``
prefix).
Tenant-check is split out from permission-gate so interactive's
``_require_ws_access`` (404 on cross-owner) and coord's
``_require_admin_coordinator`` (cluster-wide scope) coexist without
either kind triggering the wrong gate.
Coord-side test fixture (``test_coordinator_endpoints._make_client``)
swaps the imported ``coordinator_approve`` for the lifted handler
and seeds ``app.state.session_endpoint_config`` so the tests
exercise the same code path the live console does.
Net delta: ~−25 LOC for this verb on top of the SessionEndpointConfig
+ legacy-adapter scaffolding (~80 LOC paid once). Subsequent verb
lifts amortize against that scaffolding.
ruff + mypy + 4366 pytest pass. Live console smoke against the
unified URL returns 503 (no coord_mgr loaded in the smoke env) —
proves the lifted handler is reachable + the manager_lookup callable
fires correctly.
Verbs still kind-specific (deferred — bodies have substantive
behavior divergence, not just naming): ``send`` (Priority 1
worker dispatch), ``cancel`` (interactive forensics + force flag),
``close`` (interactive close-reason cap+redact+persist), ``open``
(interactive resume vs coord rehydrate), ``events`` (different SSE
replay shapes), ``create`` (interactive attachments vs coord
initial_message), ``list`` / ``saved`` (different response keys).
|
||
|
Patrick Buckley
|
df7c0c2f44 |
refactor(server): delete legacy /v1/api/coordinator/ URL tree
Stage 2 Priority 0 Steps 0.4–0.7 — collapses the four migration
steps into one commit since they have to land together. The legacy
``/v1/api/coordinator/`` URL prefix never shipped in a stable release
(it appeared in 1.5.0aN experimental), so there's no compat carry-
forward — just rip and replace.
What moves:
- Step 0.4: deletes the eighteen ``Route("/api/coordinator/...")``
entries from ``console/server.py``. Coord traffic now flows
exclusively through the unified ``/v1/api/workstreams/`` shape
mounted via ``register_session_routes`` + ``register_coord_verbs``
(Steps 0.2 and 0.3).
- Step 0.5: rewrites the OpenAPI spec (``console_spec.py``) and
schemas (``console_schemas.py``, ``server_schemas.py``) to
document the new paths. ``test_openapi.py`` parity assertions
swap with them.
- Step 0.6: mechanical URL sweep across the frontend
(``console/static/app.js`` — 9 sites; ``coordinator/coordinator.js``
— 16 sites; ``index.html`` — 1 comment).
- Step 0.7: same sweep across the test suite
(``test_coordinator_endpoints.py``, ``test_coordinator_end_to_end.py``,
``test_coordinator_governance.py``, ``test_coordinator_close_all_children.py``,
``test_coordinator_client.py``, ``test_phase6_endpoints.py``).
Also touched:
- Server-side ``CoordinatorClient`` (``coordinator_client.py``) —
the coord agent's HTTP path for ``close_all_children`` updates
to the new shape.
- Handler docstrings in ``console/server.py`` say
``POST /v1/api/workstreams/...`` not ``/coordinator/...`` so a
``grep`` for a verb's URL lands on the right line.
- ``settings_registry.py`` setting descriptions, ``server.py``
cross-process error message, migration 042 docstring — all
updated to the unified shape.
The handler functions stay named ``coordinator_*`` until the
body-convergence follow-on lifts them into ``session_routes`` with
kind branching behind ``SessionRouteConfig`` flags. URL surface is
the only thing that changes here.
The ``test_session_routes`` route-walk now asserts the legacy paths
are GONE — previously it asserted both shapes coexisted. A future
accidental remount of ``/api/coordinator/`` would fail that test.
|
||
|
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 |
||
|
Patrick Buckley
|
436ce5630b |
feat(coord): saved coordinators surface + shared session-card primitives (#399)
* feat(coord): saved coordinators surface + shared session-card primitives
The console home view now lists explicitly-closed coordinators in a
"Saved Coordinators" card grid below the active list. Click a card →
POST /v1/api/coordinator/{ws_id}/open then navigate; capacity issues
surface as a toast instead of a broken detail page. Card click is
de-duped by an `is-busy` class so rapid double-clicks don't fire
parallel resurrects.
GET /v1/api/coordinator/saved is the new backend endpoint (mirrors the
interactive list_saved_workstreams shape). Filters at the SQL layer
to state='closed' via a new optional `state` parameter on
list_workstreams_with_history (added to the protocol + both backends);
also drops any rows currently loaded into coord_mgr as defence in
depth. The blocking storage call + the lock-acquiring list_all are
offloaded via asyncio.to_thread to match coordinator_create's pattern.
CoordinatorManager._open_impl now allows resurrect of state='closed'
rows (deleted is still a tombstone). The DB state-flip on resurrect
that the first cut had is gone — it raced concurrent close()s and the
next set_state() call syncs the DB naturally; the saved list filters
already keep a still-loaded coordinator from appearing as a saved
card even when its on-disk state lags.
Frontend dedup that paid for the saved surface ships in the same diff:
- shared_static/cards.css: lifted from ui/static/style.css so both
surfaces share the basic card primitive (delete-mode rules stay
interactive-only until coordinator gets the same UX)
- shared_static/cards.js: new renderSessionCard(sess, opts) helper
used by both renderSavedWorkstreams (interactive) and
renderSavedCoordinators (console)
- shared_static/utils.js: formatRelativeTime moved here from
ui/static/app.js
Coordinator landing visual fixes folded in:
- .home-section-title now uses var(--accent) so the COORDINATORS
heading reads as a peer of the NODES heading
- .home-panel dropped its bg/border/padding so the composer is no
longer double-framed (matching the dashboard-composer feel)
- "Active coordinators" → "Saved Coordinators" rename + "Coordinators"
on the active list
ws_closed SSE handler now gates on the closed ws's kind so interactive
closes don't spam /v1/api/coordinator/saved on busy clusters.
loadSavedCoordinators in-flight de-dup coalesces close-event bursts to
one fetch instead of N.
Tests cover: caller-scoping, admin sees-all, blank-uid fail-closed,
loaded-coordinator filtering, state filter (idle rows excluded), plus
the manager-level open-resurrect / open-refuses-deleted contracts.
Closes the bug-{1,2,3}, perf-{1,2,3,4}, sec-{1,2}, q-{1,2,3,4,5,6,7}
findings from the prior multi-stage review.
* fix(design): restore amber accent on the v1 design system
The Claude Design handoff swapped the accent hue to teal (h=182).
Walking back to amber (h=75) — turnstone's original brand colour.
Lightness + chroma bumped slightly (0.62→0.7, 0.10→0.13) so the
restored gold matches the visual weight of the legacy #e5a042 token.
Hue map header comment updated to record what happened so the next
person doesn't repeat the swap. Only surfaces with data-design="v1"
on <html> pick this up — currently just turnstone-server's webui.
* chore: gitignore design_ideas/ and .claude/ dev directories
design_ideas/ holds personal Claude Design handoff scratch + reference
HTML; .claude/ holds per-user Claude Code state (worktrees, settings,
plugin caches). Neither belongs in version control.
* fix(coord): address PR #399 review nits
- tests/test_coordinator_endpoints.py: split `assert mgr.close(ws.id)`
in `_seed_closed_coord_with_history` so the close call always runs
even under `python -O` (asserts stripped). Same fix in
test_coordinator_manager.py's `test_open_refuses_deleted_coordinator`
for the open() and open_admin() calls.
- shared_static/cards.css: `.card-wsid` now reads `var(--font-mono, "IBM
Plex Mono", monospace)` so design-v1 surfaces pick up the JetBrains
Mono token while console (still pre-v1) keeps the literal fallback.
|
||
|
Patrick Buckley
|
9826ea15c5 |
feat(coordinator): phase 7 — governance + skill metadata + cross-cutt… (#383)
* feat(coordinator): phase 7 — governance + skill metadata + cross-cutting invariants
Combines three stacked sub-PRs into a single coordinator phase-7
shipment against the phase-7 plan doc. The sub-PR structure (0 / A /
B) preserved on individual branches for reviewer drill-down; this
branch is the one reviewers should merge.
## Sub-PR 0 — service-auth boundary invariants
Shared helpers and contracts that lock the console ↔ node service-auth
boundary so later authz surfaces use them by construction.
- ``_effective_user_filter(request)`` in both ``turnstone.console.server``
and ``turnstone.server`` with a shared ``DENY_EMPTY_SUB`` sentinel
on ``turnstone.core.auth``. Three-way return — admin/service
bypass, scoped caller uid, or fail-closed sentinel on blank sub.
Four callsite migrations (``_coordinator_rows``,
``coordinator_children``, ``coordinator_metrics``,
``cluster_ws_live_bulk``).
- ``StorageBackend`` class docstring codifies the tenancy contract
(every list/count/aggregate method must accept ``user_id: str |
None = None`` and push ``WHERE user_id = :user_id`` into SQL) and
the ``_mapping`` row-access contract. New
``turnstone.testing.row_contract`` ships ``assert_row_like()``.
- ``_verify_collector_service_scope`` probes an upstream node at boot
with ``expected_node_id=_scope-probe_``; a 409 proves the scope
gate was passed, a 403/401 sets ``collector_scope_error`` and
causes ``cluster_snapshot`` / ``cluster_events_sse`` to return 503
with a remediation hint. Probe URL allowlist rejects non-http(s)
schemes and 169.254.0.0/16 hosts.
- 4xx log-level floor on ``_NodeDashboardCache.get``,
``_fetch_live_block``, and ``_proxy_sse`` — dotted-hierarchy
prefixes with bounded body previews. ``_bounded_body_preview`` and
``_bounded_stream_preview`` strip control chars.
## Sub-PR A — coordinator governance core
Mid-session governance surface for coordinator workstreams.
- **Trusted-session mode.** New ``coordinator.trust.send``
permission (migration 042). ``ChatSession.set_trust_send`` /
``revoke_tools`` methods with a ``_governance_lock``. ``POST
/v1/api/coordinator/{ws_id}/trust {send: bool}`` double-gated on
``admin.coordinator`` AND ``coordinator.trust.send`` with
``allow_service_bypass=False`` so service tokens can't escalate.
``_prepare_send_to_workstream`` auto-approves sends whose target is
in the coordinator's own subtree; foreign ws_ids still require
approval. ``_is_own_subtree`` checks both ``parent_ws_id`` AND
``user_id`` to defend against cross-tenant row corruption.
- **Audit-layer credential redaction.** ``record_audit`` walks
``detail`` (dicts, lists, tuples, sets, frozensets; keys too)
and routes every string through ``redact_credentials`` + a C0
control-char scrub. New kw-only ``raw_detail=True`` opt-out.
``_has_any_string`` fast-path. Audit action registry extended
with the four new governance sub-prefixes.
- **Mid-session revocation + cascading stop.** ``POST
/v1/api/coordinator/{ws_id}/restrict {revoke: [...]}`` caps 256
entries / 128 chars; ``_prepare_tool`` short-circuits with a
tool-error. ``POST /v1/api/coordinator/{ws_id}/stop_cascade``
cancels the coord's in-flight generation then dispatches
``cancel_workstream`` for every direct child in parallel via
``asyncio.gather`` bounded by ``Semaphore(16)``. Per-child
outcomes split into ``cancelled`` / ``failed`` / ``skipped``
(404 = already-gone rather than dispatch-broken). Both endpoints
apply ``allow_service_bypass=False`` on the admin gate.
- **Shared plumbing.** ``_resolve_coord_session`` helper collapses
the handler prelude three endpoints shared. ``_emit_coord_audit``
wraps ``record_audit`` in a dedicated ``ThreadPoolExecutor``
(``app.state.audit_executor``) so audit bursts don't starve cancel
dispatches. ``_require_json_object`` guards body parsing so non-
object JSON returns 400 instead of 500.
## Sub-PR B — skill metadata governance
- **Description validator (migration 043).** ``prompt_templates``
rows now require a non-empty ``description``. Existing empty rows
get backfilled with a ``"Skill: <name>"`` placeholder on upgrade.
The installer (``admin_skill_discover``) and MCP prompt sync both
synthesise a placeholder when the upstream description is blank
so non-admin write paths satisfy the invariant.
- **Skill kind classifier (migration 044).** New
``prompt_templates.kind`` column (``interactive`` / ``coordinator``
/ ``any``; defaults to ``any``). New
``turnstone.core.skill_kind.SkillKind`` StrEnum is the single
source of truth; Pydantic schemas type ``kind`` as ``SkillKind``
(OpenAPI advertises the enum) and the handler validator catches
the ValueError. ``list_skills_filtered`` gains a
``kinds: list[str] | None = None`` SQL filter.
``CoordinatorClient.list_skills`` defaults to
``kinds=["coordinator", "any"]`` so interactive-only skills are
hidden from the orchestrator.
- **``scan_status`` → ``risk_level`` rename (migration 045).**
Lossless column rename to align with ``IntentVerdict.risk_level``
terminology. Swept storage (both backends + schema + protocol),
handlers, API schemas, tool JSON, generated OpenAPI specs,
TypeScript SDK types, frontend (``governance.js``), tests, and
English prose in ``docs/judge.md`` + ``docs/tools.md``. The
user-facing on-load warning now reads ``has risk level:
{risk_tier}``. Tool JSON's ``risk_level`` enum corrected to the
scanner's actual taxonomy (``safe / low / medium / high /
critical``; was the never-shipped ``clean / flagged / unscanned /
pending``). Historical migration 021 left untouched.
## Migrations
042 (``coordinator.trust.send`` perm — PR A)
043 (description backfill — PR B)
044 (``kind`` column add — PR B)
045 (``scan_status`` → ``risk_level`` rename — PR B)
All four use position-anchored permission strings / host-side
parse-filter-rejoin on downgrade where SQL ``REPLACE`` could
corrupt prefix-overlapping values.
## Verification
- ``ruff check turnstone tests`` clean.
- ``mypy turnstone`` clean on 165 source files.
- ``pytest -m "not live"``: 4431 passed (+85 over the phase-6
baseline). Includes +32 tests in ``tests/test_service_auth_boundary.py``
and +38 in ``tests/test_coordinator_governance.py``; shared fixtures
extracted to ``tests/_coord_test_helpers.py``.
- Generated OpenAPI JSON (``sdk/typescript/openapi-{console,server}.json``)
regenerated via ``sdk/typescript/scripts/generate-types.py``; zero
``scan_status`` occurrences remaining outside the historical
migration 021 and the rename migration 045.
## Security reviews
Both reviews flagged by the phase-7 plan (items 1 + 5, plus 0a's
refuse-to-serve gate) ran through the multi-stage ``/review``
pipeline twice per sub-PR; all confirmed findings landed in-branch.
* fixup(phase-7): CI lint + PR #383 review fixups
Addresses the lint CI failure (ruff format) plus 12 findings from the
two automated PR reviewers.
Copilot:
- ``_sqlite.list_installed_skill_urls`` / ``_postgresql.list_installed_skill_urls``
used positional row indexing (``r[0]``/``r[1]``/``r[2]``) while this
same PR's ``StorageBackend`` class docstring forbids it. Switched
both to ``r._mapping["..."]`` access.
- ``list_skills.json`` previously advertised ``risk_level=""`` as a
filter for unscanned skills, but the implementation treats empty
strings as "no filter". Clarified the tool description to say
omit the filter entirely to include unscanned rows, and added an
explicit ``enum`` on the parameter restricting it to the scanner
tiers. ``_prepare_list_skills`` keeps the ``strip() or None``
normalisation — unscanned filtering now has an unambiguous contract.
- ``test_storage_skills_filtered.test_risk_level_filter`` used the
legacy ``clean`` / ``flagged`` values from the pre-rename column.
Rewritten with the scanner's actual taxonomy (``safe`` / ``high``).
github-code-quality (CodeQL):
- ``test_deny_sentinel_is_singleton`` previously asserted
``cs.DENY_EMPTY_SUB is cs.DENY_EMPTY_SUB`` — an identical-expression
comparison. Rewritten as two separate ``from ... import ... as`` aliases
(``FIRST_READ`` / ``SECOND_READ``) so the identity check is between
distinct bindings.
- ``test_restrict_empty_revoke_is_noop_but_audits`` unpacked ``state``
without using it. Renamed to ``_state``.
- Mixed import styles in ``test_service_auth_boundary.py`` — the
file previously used both ``import turnstone.console.server as cs``
and ``from turnstone.console.server import ...`` for the same
module (same story for ``turnstone.core.auth`` and
``turnstone.server``). Consolidated to the ``from X import Y`` style
used elsewhere in the file; the ``_fetch_live_block`` test now
patches via pytest's ``monkeypatch`` fixture instead of a manual
rebind through a module alias.
CI:
- ``ruff format`` reformatted one line in
``tests/test_coordinator_endpoints.py``.
Verification: ruff check + mypy clean (166 files); 4459 non-live
pytest pass.
* fix(tests): swap asyncio marker for anyio in service-auth boundary tests
PR #383 CI caught that the 13 ``@pytest.mark.asyncio`` decorators I
added in ``test_service_auth_boundary.py`` are an off-convention
choice — the rest of the repo uses ``@pytest.mark.anyio`` (148 sites
vs my 13). The CI environment pulls in ``anyio`` but not
``pytest-asyncio``, so every async test in this one file was failing
with "async def functions are not natively supported". It passed
locally by accident — my dev venv happens to have pytest-asyncio
installed ambiently.
Swapped all 13 marker sites to ``@pytest.mark.anyio``. No functional
change; the tests run under the same default asyncio backend anyio
provides.
Verification: ruff + mypy clean (166 files); 4459 non-live pytest
pass.
|
||
|
Patrick Buckley
|
334edbd580 |
fix(server,console): kind filter on saved-workstreams + closed coords on landing (#380)
* fix(server,console): kind filter on saved-workstreams + closed coords on landing Two independent bugs folded into one hotfix: 1. Coordinators leaking into the interactive UI's "saved workstreams" sidebar. ``list_workstreams_with_history`` (SQLite + postgres) was kind-agnostic — every coordinator row with conversation history came back alongside interactive rows, and ``list_saved_workstreams`` serialized them uniformly with no kind field so the interactive UI rendered coordinators as regular interactive entries. Fix: add optional ``kind: WorkstreamKind | str | None = None`` kwarg on ``list_workstreams_with_history`` (storage protocol + both backends + the ``turnstone.core.memory`` helper). Pass ``kind=WorkstreamKind.INTERACTIVE`` from the /v1/api/workstreams/saved handler so the interactive surface only sees interactive rows. Default ``None`` preserves legacy all-kinds behaviour for any other caller that wants both. 2. Closed coordinators vanish from the console landing page. ``_coordinator_rows`` in console/server.py built dashboard rows exclusively from the in-memory ``CoordinatorManager`` registry, which pops rows on ``close()``. The persisted storage row stays (state='closed') but never reached the landing-page poller at /v1/api/cluster/workstreams?node=console. Fix: two-lane merge in ``_coordinator_rows``. The in-memory lane (manager) stays authoritative for live session state (model / model_alias / current state / tokens). A new persisted lane queries ``storage.list_workstreams(kind=COORDINATOR, user_id=uid, limit=200)`` and appends rows NOT already in the in-memory set — surfacing closed / error / deleted coordinators so the operator can still see them on the landing page. Ownership semantics unchanged — non-admin callers only see their own tenant, admin-bypass via admin.users/admin.roles honored on both lanes, empty-string defense-in-depth matches _check_row_owner_or_404. Tests: - tests/test_storage_sqlite.py — two new tests: kind filter excludes coordinators from the history list; string form of kind accepted (matches the memory.py forwarding shape). - tests/test_coordinator_endpoints.py — four new tests: - closed coordinators from storage surface alongside active ones. - in-memory row wins on ws_id dedup (live state authoritative). - persisted rows respect tenant filter (non-admin, admin bypass). - orphan rows (empty user_id) never leak to empty-sub callers. Gate: ruff + mypy + pytest -m "not live" (4315 passed) all clean. * fix(server,console): address Copilot review on PR #380 Three review comments folded in: 1. Tenancy leak in /v1/api/workstreams/saved — the handler called list_workstreams_with_history without a user_id filter, so any authenticated user could see every other user's saved workstream aliases / titles / names. Fix: - Add ``user_id: str | None = None`` kwarg to list_workstreams_with_history on the protocol + both backends (SQLite + postgres). Pushes the filter into SQL. - memory.py helper forwards the kwarg. - /v1/api/workstreams/saved reads ``_auth_scopes(request)``: a service-scoped caller gets cluster-wide visibility (None), a non-service caller with a blank ``sub`` returns an empty list, otherwise the SQL filter is scoped to the caller's uid. Matches the _visible_workstreams pattern used on /workstreams and /dashboard. 2. Loose type annotation on the memory.py helper — ``kind: Any`` tightened to ``WorkstreamKind | str | None`` so mypy catches invalid callers. WorkstreamKind was already imported in the module. 3. Brittle positional indexing in _coordinator_rows persisted-rows lane — ``row[10]`` for user_id encoded a column offset that would silently corrupt the projection on any future SELECT reorder. Drop the test-double fallback entirely; the storage-protocol contract already requires SQLAlchemy Row with _mapping, and every real caller (SQLite + postgres) provides it. Tests: - test_server_authz.py TestSavedWorkstreamsTenantScoping — four new regression tests covering: non-service caller sees only own rows, service scope sees cluster-wide, blank-sub non-service returns empty, and coordinator rows excluded even for service callers. Gate: ruff + mypy + pytest -m "not live" (4319 passed) all clean. |
||
|
Patrick Buckley
|
c397668d21 |
feat(coordinator): tree-view UI, cluster-wide live inspect, dashboard… (#370)
* feat(coordinator): tree-view UI, cluster-wide live inspect, dashboard grouping — phase 3
Closes out the 1.5 coordinator UX surface: a right-sidebar tree view at
/coordinator/{ws_id} showing spawned children + task list, a new
cluster-wide live inspect endpoint that powers the tree's live badges,
and 2-level dashboard tree grouping that nests spawned children under
their coordinator parent.
## Cluster-wide live `inspect_workstream`
New `GET /v1/api/cluster/ws/{ws_id}/detail` on the console, gated by a
new `admin.cluster.inspect` permission (unassigned to any builtin role;
operators opt in). Aggregates `storage.get_workstream` with a
short-timeout (2s) HTTP fetch against the owning node's
`/v1/api/dashboard`. Coordinator-hosted workstreams get their `live`
block from the in-process `CoordinatorManager` instead of a proxy hop.
Response shape `{persisted, live, messages}` — `live: null` on node
unreachability / 5xx / missing-entry with status 200 so the UI can
degrade gracefully without an error state. Correlation-id masks
unexpected exceptions. 404-masks cross-tenant reads (non-admin
callers see only their own workstreams).
`CoordinatorClient.inspect()` best-effort merges the `live` block onto
its storage snapshot so the model-facing `inspect_workstream` tool
gains a `live` key without any schema change. Model-facing tool
schema stays identical.
## Tree-view UI
New right sidebar at `/coordinator/{ws_id}` with a 2-level children
tree + the phase-2 task list.
Backend:
- New `GET /v1/api/coordinator/{ws_id}/children` returns
`{items, truncated}` — identical row shape to the `list_children`
tool — filtered via `storage.list_workstreams(parent_ws_id=..., kind=None)`.
- New `GET /v1/api/coordinator/{ws_id}/tasks` returns the
`{version, tasks}` envelope via the shared module-level
`load_task_envelope` decoder (extracted from `CoordinatorClient`
so both the tool path and the UI read share corruption semantics).
Corrupt envelopes return an empty list for UI resilience — the
`task_list` tool remains the authoritative write + error path.
- `CoordinatorManager` subscribes to the `ClusterCollector`'s
listener channel from the console lifespan and dispatches filtered
`child_ws_created / child_ws_state / child_ws_closed / child_ws_rename`
events onto each coordinator's SSE stream. Filter authoritative
on the server via a per-coordinator child-ws_id registry populated
lazily on `open()` from storage and incrementally on `ws_created`
events; cleared on `close()` / eviction. One SSE connection per
client, no client-side filtering.
Frontend:
- DOM-method-only child-row rendering (no innerHTML of user content).
- State glyph vocabulary (● running / ◐ thinking / ⚠ attention /
✗ error / ○ idle) plus text labels — WCAG 1.4.1 carries info in
both glyph and label.
- Live badges (tokens + pending-approval pip) fetched via
`/cluster/ws/{ws_id}/detail` with a 5s TTL cache and 250ms debounce
per child. One request per state change, not per second.
- SSE child events update in place; renderChildren() re-sorts.
- Mobile (<700px) sidebar collapses to an accordion above the chat
with a toggle button flipping aria-expanded; a `.highlight` flash
marks task→child scroll targets; `prefers-reduced-motion` respected.
- Deep-link child rows to `/node/{node_id}/?ws_id=<child>` via
`<a target="_blank" rel="noopener">` with encodeURIComponent on
regex-validated ids.
## Dashboard tree grouping
Cluster dashboard rows now group by `parent_ws_id`. Coordinator rows
(`kind == "coordinator"` or children present) get an expand/collapse
caret (button with `aria-expanded`); collapsed shows "(N children)".
Expanded renders children indented as sibling rows with a left-border
gutter. Orphaned children (parent missing or closed) render at top
level with a muted "orphan" badge. Expansion state persisted in
`localStorage` keyed per coordinator ws_id so operator preference
survives reloads. Coordinator rows deep-link to `/coordinator/{id}`;
node-backed workstreams keep their existing proxy deep-link.
Per-node `ws_created / ws_state / ws_activity` SSE event payloads
gained `parent_ws_id` + `kind` so the collector can propagate them
through its fan-out to browser clients without a second lookup;
`_build_node_snapshot` and `/v1/api/dashboard` rows include the
same. Coordinators (which don't live on cluster nodes) merge into
`/cluster/workstreams` via a new `_coordinator_rows` helper that
threads them through the collector's `get_workstreams(extra_rows=...)`
parameter — extras share the filter / sort / paginate pipeline with
node-backed rows.
## Tests
- `tests/test_coordinator_endpoints.py` — 19 new cases covering
children (empty / populated / ownership 404 / admin bypass /
invalid ws_id / truncation), tasks (empty / round-trip / corrupt /
ownership), and cluster-inspect (auth gates / 400 / 404 / ownership /
coordinator self-path / unloaded-live-null / message-limit clamp).
- `tests/test_coordinator_manager.py` — 8 new cases covering registry
bootstrap on create + open, dispatch for each event type,
unrelated-parent filtering, shutdown idempotency.
- `tests/test_console.py` — existing `cluster_workstreams` assert
updated for the new `extra_rows` kwarg.
## Verification
- `ruff check turnstone tests` clean.
- `mypy turnstone` clean.
- `pytest -m "not live"` — 4184 passed, 3 deselected.
* fix(coordinator): race in dispatch + ui_factory kwarg filtering — PR #370 review
Addresses feedback from the GitHub Copilot + code-quality bot review
passes on PR #370.
## Race in _dispatch_child_event ws_created branch
Copilot flagged a TOCTOU where the lock-free read of
``self._active_coords`` (line 912) could see the parent coordinator,
then ``close()`` / eviction pops ``_children[parent]`` + drops the
coord from ``_active_coords`` before we acquire ``_children_lock``,
and then ``setdefault(parent, set())`` resurrects the entry —
leaking the registry key forever and fanning events to a closed UI.
Fix: re-check ``parent in self._active_coords`` inside
``_children_lock``. The reference swap is still atomic; holding
``_children_lock`` and re-reading the snapshot catches the race
without serializing back through ``self._lock``.
Regression test: create → close → dispatch a ws_created → assert
neither ``_children`` nor ``_active_coords`` regained the entry.
## ui_factory kwarg filtering via inspect.signature
code-quality bot flagged that the previous ``try ui_factory(…, kind=,
parent_ws_id=) except TypeError`` dance fired on every call with
legacy test factories (``lambda wid: WebUI(ws_id=wid)``) — wasteful
and masks real signature mismatches.
Fix: inspect the factory's signature and only pass kwargs it
actually accepts (explicit param name OR ``**kwargs`` absorber).
Keep a conservative ``except TypeError`` fallback for C-callables
and odd signatures ``inspect`` can't introspect.
Copilot also flagged a comment mismatch (the old comment said
"KeyError on **kwargs" — it's ``TypeError``, which is what the code
caught). The rewritten comment is correct.
## Nit: side-effect in assert
code-quality bot flagged ``assert mgr.close(ws.id)`` in
test_coordinator_manager.py. Split into two statements.
## Verification
- ``ruff check`` clean.
- ``mypy turnstone`` clean.
- ``pytest -m "not live"`` — 4223 passed, 3 deselected, 0 failed.
|
||
|
Patrick Buckley
|
8650370790 |
Feat/coordinator phase2 audit (#369)
* feat(coordinator): audit middleware on routing proxy — phase 2
Adds per-tool-call audit attribution to the multi-node routing proxy
handlers so coordinator → server hops land observable rows in
``audit_events``. Phase 1 preserved the ``src="coordinator"`` claim
through ``_proxy_auth_headers``'s upstream re-mint; this commit
closes the recording side. Was the last real security gap from
phase 1 — an enterprise deployment with ``admin.coordinator``
granted got only the three console-side
``coordinator.{create,close,cancel}`` rows; per-tool-call
attribution was missing.
## Action-naming scheme
route.workstream.create POST /v1/api/route/workstreams/new
route.workstream.send POST /v1/api/route/send
route.workstream.close POST /v1/api/route/workstreams/close
route.workstream.delete POST /v1/api/route/workstreams/delete
route.approve POST /v1/api/route/approve
route.cancel POST /v1/api/route/cancel
route.command POST /v1/api/route/command
route.plan POST /v1/api/route/plan
Action-name conventions documented in ``turnstone/core/audit.py``
module docstring alongside the existing namespaces — the docstring
is now ``<resource>.<verb>`` shaped (non-exhaustive) rather than
trying to enumerate every prefix.
## Recording rules
- ``record_audit()`` fires only on a 2xx upstream response. 4xx/5xx
are observable via ``_record_route``'s metrics path; doubling the
audit-events table size for failure rows would dilute signal
without giving operators much extra value.
- ``detail`` JSON carries ``{src, node_id, coord_ws_id?}`` — ``src``
lands verbatim from ``auth.token_source`` so non-coordinator
origins (``"jwt"``, ``"console-proxy"``) also get attribution;
``coord_ws_id`` only appears when the inbound JWT carried it.
- Wrapped in ``try/except`` + ``log.debug("route.audit_failed", ...)``
defence-in-depth. ``record_audit`` itself is fire-and-forget;
the outer try guards against a programmer error in the call site.
## Routing-proxy specifics
- ``route_create``: emits at the post-multipart/JSON convergence
``if resp.status_code == 200`` block. Both branches set
``audit_ws_id`` correctly — multipart from the query-string ws_id,
JSON from ``body["ws_id"]`` (post-503-retry) or ``body["resume_ws"]``.
- ``route_proxy``: emits the URL-method-mapped action. ``ref`` is
reassigned to ``new_ref`` after a successful 404→cache-refresh
retry so audit attribution uses the retried node, not the failed
first node.
- ``route_workstream_delete``: emits on 2xx using the ws_id from
the request body.
- ``route_attachment_proxy``: out of scope (upstream attachment
endpoints emit their own ``workstream.attachment.*`` rows;
auditing here would double-count).
## Tests
16 new tests in ``tests/test_route_proxy_audit.py`` covering:
- Coordinator-origin emission with full detail payload.
- 502 / 400 / 503-retry-final-node-id paths.
- Parametrised method→action mapping for the 6 ``route_proxy`` URLs.
- Plain-JWT origin (no ``coord_ws_id`` in detail).
- Delete handler 2xx + 502.
- Audit-storage exception swallowed (proxied response unchanged).
- ``auth_storage`` absent → no-op (existing route-handler tests
unaffected).
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4087 passed,
3 deselected (live-backend), 0 failed.
* feat(coordinator): discovery tools and /open parity — list_nodes, list_skills, POST /coordinator/{ws_id}/open
Adds the read-side surface coordinators need to make informed
orchestration decisions plus an explicit rehydration endpoint
matching the server's ``POST /v1/api/workstreams/{ws_id}/open``.
## list_nodes (auto-approved)
``list_nodes(filters={key: value, ...})`` reads ``node_metadata`` via
``storage.filter_nodes_by_metadata`` + ``get_all_node_metadata`` —
one query each, no N+1. Each row carries its full metadata dict so
the coordinator has both auto keys (``arch`` / ``cpu_count`` /
``fqdn`` / ``hostname`` / ``os`` / ``os_release`` / ``python``;
always present) and operator-supplied user keys (``capability`` /
``region`` / ``tenant`` / ``role``) without a second round-trip.
Tool description enumerates the auto keys explicitly so the model
knows what's always available vs deployment-specific.
Storage stores metadata values as JSON-encoded strings (the write
path in ``server.py`` / ``admin.py`` / ``console/server.py`` all go
through ``json.dumps``). The client re-encodes filter values
before the stored-text comparison and decodes stored values before
returning them to the model — so ``{"capability": "gpu"}`` is the
natural form the model uses, not ``{"capability": "\"gpu\""}``.
Ints round-trip as ints.
Returns ``{nodes, truncated}``; ``truncated=True`` when the page
was full.
## list_skills (auto-approved)
``list_skills(category?, tag?, scan_status?, enabled_only?, limit?)``
surfaces the skill registry so coordinators can discover worker
profiles. New storage protocol method ``list_skills_filtered(...)``
on both SQLite and PostgreSQL backends pushes filters into SQL.
``tag`` filter matches against the JSON-array ``tags`` column with
quote-bracketed substring (``%"tag"%``) — quote-safe against
``foo`` vs ``foobar`` collisions on both backends.
Returns ``{skills, truncated}`` with ``name`` / ``category`` /
``tags`` (decoded to list) / ``version`` / ``description`` /
``model`` / ``enabled`` / ``scan_status`` / ``activation`` — the
discovery projection, not the full row.
## POST /v1/api/coordinator/{ws_id}/open
Explicit rehydration endpoint. Lazy ``GET`` rehydration works for
the UI; this gives SDK callers and operators a way to warm a
coordinator without browsing to it. Same ownership / 404-on-
mismatch / correlation-id-masked error semantics as
``coordinator_detail``. Returns ``{ws_id, name, already_loaded?}``.
Registered in ``turnstone/api/console_spec.py`` with a dedicated
``CoordinatorOpenResponse`` Pydantic model so the OpenAPI schema
matches the wire shape.
## Tests
- ``tests/test_storage_skills_filtered.py`` — 8 cases validated on
BOTH SQLite and PostgreSQL backends (``pytest --storage-backend
postgresql``). Covers no-filter ordering, category exact-match,
tag quote-safety (``"foo"`` matches ``["foo","bar"]`` but not
``["foobar"]``), scan_status, enabled_only, limit, AND semantics,
empty result.
- ``tests/test_coordinator_client.py`` — 11 new cases covering
node/skill shape decoding, JSON-encoded filter round-trip (the
``"gpu"`` vs ``'"gpu"'`` case), int filter encoding, truncation,
no-match empty, no N+1 (``get_prompt_template`` /
``get_node_metadata`` call counts asserted zero).
- ``tests/test_coordinator_tools.py`` — 11 new cases for
``_prepare``/``_exec`` dispatch, filter type-drop, limit clamping
(``limit=0`` falls back to 100, negatives clamp to 1),
truncation-signal summary.
- ``tests/test_coordinator_endpoints.py`` — 8 new cases for
``/open``: ``already_loaded`` on in-memory hit, 404 on ownership
mismatch, lazy rehydrate on miss, admin bypass, unknown ws_id,
503 on ``coord_mgr`` unavailable, 500 with correlation-id mask on
factory failure, 503 passthrough on ``ValueError``.
- ``tests/test_workstream_kind.py`` / ``test_tools_schema.py``
updated to include ``list_nodes`` and ``list_skills`` in the
disjoint-namespace regression guard and the tool-count check.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4122 passed, 3
deselected (live-backend), 0 failed. Postgres backend storage
tests green (``pytest --storage-backend postgresql
tests/test_storage_skills_filtered.py`` 8 passed).
* feat(coordinator): task_list tool — persistent planning state
Adds a coordinator-only ``task_list`` tool persisted on the
coordinator's own ``workstream_config`` row. Gives coordinators a
scratch surface for work decomposition that survives restarts so the
UI can render planned-vs-done state once the tree view lands.
## Tool surface
``task_list(action, ...)`` with five actions:
- ``list`` auto-approved read. Returns ``{tasks, truncated}``;
truncated=True when the list exceeded the 200-row
page cap.
- ``add`` needs approval. ``title`` required; optional
``status`` and ``child_ws_id``. Title clamped at
200 chars. Capacity cap at 500 tasks — hitting the
cap is an explicit signal to prune done/blocked rows.
- ``update`` needs approval. Mutate by ``task_id``; fields
``title`` / ``status`` / ``child_ws_id`` optional.
- ``remove`` needs approval. Drop by ``task_id``.
- ``reorder`` needs approval. Pass ``task_ids``; validated as an
exact permutation of the current set (rejects
partial, extra, or substituted ids — prevents silent
task loss).
Status enum: ``pending`` / ``in_progress`` / ``done`` / ``blocked``.
``child_ws_id`` links a task to the child workstream spawned for it
(no enforcement; the coordinator owns the relationship).
## Persistence
Stored as a single JSON-envelope value on ``workstream_config`` —
``{"version": 1, "tasks": [...]}``. No new table; the kanban v2
work will supersede this row via a format migration keyed on
``version``. ``_save_task_list`` writes only the ``tasks`` key so
concurrent writers to other ``workstream_config`` keys (e.g. the
admin Settings UI updating ``reasoning_effort``) aren't clobbered
by a read-modify-write on the full row.
## Corrupt-envelope safety
A hand-edited or legacy config row that doesn't parse as the
expected shape logs a warning and returns an empty envelope from
``task_list_get``. Mutators refuse to overwrite corrupt data —
they detect the sentinel and return a clear error so the operator
can inspect or clear the row rather than losing work silently.
## Concurrency
Per-(ws) ``threading.Lock`` cached on the client. The worker
thread is single-threaded for tool execs so this is mostly
defence-in-depth against future maintenance-script call sites.
Cache never grows beyond one entry per coordinator session because
the scope guard short-circuits foreign ``ws_id`` before the lock
is acquired.
## Malformed-JSON recovery
``_prepare_tool`` fallback-1 regex-extract allowlist extended with
``action`` / ``status`` / ``task_id`` / ``title`` (alphabetized) so
slightly-malformed ``task_list`` calls get the same
self-correction behaviour as the other coordinator tools.
## Tests
- ``tests/test_coordinator_client.py`` — 15 new cases covering:
fresh-envelope shape, add/get roundtrip, empty-title + invalid-
status rejection, 200-char title clamp, update by id + missing
id, remove semantics, reorder permutation validation (partial +
extra + wrong id + valid), cross-ws scope violation, corrupt-
JSON read recovery, corrupt-envelope write refusal (all four
mutators), 500-task capacity cap, workstream_config key
preservation across ``_save_task_list``.
- ``tests/test_coordinator_tools.py`` — 12 new cases covering the
dispatch layer: list auto-approved, each mutating action needs
approval, unknown-action / missing-required-arg errors, list
returns tasks, page-cap at 200 with truncated signal, add
dispatches to client, reorder surfaces permutation error,
remove-not-found.
- ``tests/test_tools_schema.py`` / ``tests/test_workstream_kind.py``
extend the tool-count + disjoint-namespace + primary-key
regression guards with ``task_list``.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4148 passed,
3 deselected (live-backend), 0 failed.
|
||
|
Patrick Buckley
|
e42add1b77 |
feat(coordinator): coordinator workstream kind — phase 1 (#368)
* feat(coordinator): coordinator workstream kind — phase 1
Adds a new ``kind="coordinator"`` workstream that runs inside the
``turnstone-console`` process (first ChatSession hosted on the console)
with a dedicated tool set for spawning and driving child workstreams.
Supersedes the external ``turnstone-coordinator`` MCP side-car for new
installs; the extension is marked deprecated in
``examples/mcp-cluster-ops/README.md`` but still works on 1.4-and-earlier
clusters.
Phase 1 ships: the workstream class, 6 lifecycle tools, console hosting,
9 HTTP endpoints, per-user audit attribution, and a one-pane web UI at
``/coordinator/{ws_id}``. Node/skill discovery tools, task-list tool,
tree-view UI, and routing-proxy audit middleware follow in a later PR.
## Schema
Migration 039 adds ``kind`` / ``parent_ws_id`` columns + indexes to
``workstreams``. Both SQLite and PostgreSQL backends take the new
kwargs on ``register_workstream``; empty-string ``parent_ws_id``
normalises to ``NULL`` at the storage edge. PostgreSQL uses
``INSERT ... ON CONFLICT DO NOTHING`` to match SQLite's ``OR IGNORE``
and close a pre-existing SELECT-then-INSERT TOCTOU window.
``list_workstreams`` gains optional ``parent_ws_id`` / ``kind`` filters;
new ``get_workstream(ws_id)`` returns the full row (the existing
``get_workstream_metadata`` stays untouched for back-compat).
## Core session + kind routing
- ``ChatSession.__init__`` accepts ``kind`` / ``parent_ws_id`` /
``coord_client``. On ``kind="coordinator"`` it swaps
``_tools = COORDINATOR_TOOLS`` and zeros sub-agent tool lists.
- ``Workstream`` dataclass extended with ``user_id`` / ``kind`` /
``parent_ws_id``. Both ``WorkstreamManager`` and the new
``CoordinatorManager`` use the same type — no parallel hierarchy.
- ``_SessionFactory`` Protocol + server / cli factory closures thread
the new kwargs. ``POST /v1/api/workstreams/new`` rejects
``kind != "interactive"`` with 400; ``POST
/v1/api/workstreams/{ws_id}/open`` refuses coordinator rows so a
server node can't accidentally rehydrate one.
## Coordinator tool set
Six tools (``spawn``, ``inspect``, ``send``, ``close``, ``delete``,
``list_workstreams``) with a ``coordinator: true`` metadata flag,
scoped to coordinator-kind sessions only. ``inspect`` and ``list`` are
auto-approved reads; the four mutators need approval. ``list`` returns
``{"children": [...], "truncated": bool}`` so the model can detect
post-filter under-fill and paginate.
## CoordinatorClient (in-process, sync)
Mutating ops HTTP-POST to the console's own ``/v1/api/route/*`` on the
local bind URL so every existing middleware (auth, rate-limit) runs.
Read ops hit ``storage.list_workstreams`` / ``get_workstream`` /
``load_messages`` directly — the routing proxy doesn't expose
list/inspect paths. URL paths are a validated constant table (avoids
an httpx ``base_url``-merge trap). A new
``/v1/api/route/workstreams/delete`` proxy handler joins the existing
route-proxy endpoints.
## Per-session coordinator JWT
``CoordinatorTokenManager`` mints short-lived JWTs with ``sub=<real
user>`` (attribution preserved), ``src="coordinator"``,
``aud="turnstone-console"``, ``coord_ws_id=<ws>`` custom claim.
``_proxy_auth_headers`` preserves ``src`` + ``coord_ws_id`` across the
upstream re-mint so server-side middleware sees coordinator-origin,
not ``console-proxy``. ``AuthResult.extra_claims`` carries
non-reserved claims through validate→remint; ``create_jwt``'s
reserved-claim set (now including ``nbf`` / ``jti``) is symmetric with
``validate_jwt``.
## Console hosts the ChatSession
- New ConfigStore settings: ``coordinator.model_alias`` (required),
``reasoning_effort``, ``max_active`` (default 5),
``session_jwt_ttl_seconds``.
- Console lifespan builds a ``ModelRegistry`` +
``CoordinatorManager``. Missing / unresolvable alias returns **503**
with remediation text — never 500.
- ``CoordinatorManager``: placeholder-slot reservation under lock,
rollback on factory failure, per-ws_id rehydration lock to serialise
concurrent lazy-opens, ``max_active`` enforced via ``close_idle``
eviction semantics.
- ``ConsoleCoordinatorUI`` is a thin ``SessionUI`` implementation — no
global broadcast, no per-node metrics, shared
``_APPROVAL_WAIT_TIMEOUT`` constant across approval + plan paths.
- No eager startup rehydration: persisted coordinator rows load lazily
on first ``GET /v1/api/coordinator/{ws_id}``.
## Console coordinator API
Nine endpoints under ``/v1/api/coordinator/*`` gated by ``approve``
scope + new **``admin.coordinator``** permission (added to
``_VALID_PERMISSIONS``; not in any builtin role — operators opt in
explicitly). Ownership failures return **404, not 403** and use
strict equality so empty-owner rows don't leak across tenants.
Correlation-id masking on every factory-raising path
(``coordinator_create`` + ``coordinator_detail`` lazy rehydrate) — no
stack traces to the client.
## Audit attribution
Three console-side events (``coordinator.create`` / ``.close`` /
``.cancel``) with the real creator's ``user_id`` plus
``detail={coord_ws_id, src="coordinator"}``. No schema migration
required. Per-tool-call audit across the routing proxy is deferred
(needs either a ``source`` column on ``audit_events`` or
``record_audit`` calls wired into the route-proxy handlers).
## Web UI (``/coordinator/{ws_id}``)
One-pane chat served by the console. Reuses ``shared_static``
(``base.css``, ``auth.js``, ``theme.js``, ``toast.js``, ``utils.js``,
``kb.js``) and the server UI's ``renderer.js`` pipeline (KaTeX, Mermaid,
highlight.js already bundled).
- SSE to ``/v1/api/coordinator/{ws_id}/events`` with exponential-
backoff reconnect; status line carries a leading glyph
(● / ○ / ⚠) so state isn't conveyed by colour alone.
- Renders content, reasoning (dimmed italic
``.role-reasoning``), tool_result, approve_request, intent_verdict,
output_warning.
- Child ws_id references auto-wrap to
``/node/{node_id}/?ws_id={child}`` links — both ids regex-validated
before interpolation, everything else HTML-escaped.
- Non-modal approval bar (``role="region"``) with a batch header
("Approve N tool calls"), initial focus on the approve button,
buttons disabled during the in-flight POST, red-bordered deny.
``aria-live`` flips to ``off`` during streaming.
- "New coordinator" button on the dashboard header — permission-gated
on the UI side, matching the backend 403.
- Mobile composer capped under ``@media (max-width: 700px)``.
## Tests
~120 new tests across 8 files: workstream-kind storage + dataclass
semantics, CoordinatorClient URL map + token minting + storage reads +
truncation signalling, tool prepare/exec dispatch and approval gating,
CoordinatorManager create / rollback / eviction / lazy rehydration +
concurrency, HTTP endpoint auth + 404-on-ownership + 503-on-misconfig,
proxy-auth ``src`` preservation, full lifecycle end-to-end, coordinator
page HTML-injection guard. ``test_tools_schema.py`` widened to 25
tools (19 existing + 6 coordinator).
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 files), ``pytest`` 4054 passed (5 pre-existing failures unrelated
to this change — confirmed against ``main``).
* polish(coordinator): address PR review + CI + tool-namespace isolation
CI:
- `ruff format`: two files reformatted, matches the in-repo pre-commit config.
- `wheel-completeness`: add `turnstone/console/static/coordinator/*.html` +
`*.js` to the hatch wheel-include list. Without this the coordinator UI
was missing from published wheels.
- `test (3.11/3.12/3.13)` + `test-postgres`: three `TestExecReadImage`
tests were masking a real bug — my 6 new tool JSONs pushed tool count
19→25, crossing the default `tool_search.auto` threshold (20), which
made `ChatSession.__init__` construct a `ToolSearchManager` and cache
`_cached_capabilities` during init. Tests that later patched
`session._provider.get_capabilities` saw the cached value instead.
Root-cause fix: the tool-search threshold code path now reads
capabilities through `_resolve_capabilities(...)` directly — no cache
populate — so the patch takes.
Tool-namespace isolation (bigger fix than CI symptoms suggested):
- `TOOLS` was the union of all loaded tool JSONs including the 6 new
coordinator tools. Interactive sessions were getting coordinator
tools in their function-calling surface (which is nonsense — they
require a console-hosted `coord_client`), and coordinator sessions
counted against the interactive tool-search threshold. Fix:
- New `INTERACTIVE_TOOLS` / `INTERACTIVE_TOOL_NAMES` in
`turnstone/core/tools.py` exclude anything with `coordinator: true`
metadata. `TOOLS` stays as the union for schema introspection +
eval catalog.
- `ChatSession.__init__` selects tool set by kind: coordinator gets
fixed `COORDINATOR_TOOLS` (no MCP merge, no listeners registered);
interactive gets `INTERACTIVE_TOOLS` (+ MCP if configured).
Coordinators are meta-orchestrators that spawn child workstreams;
MCP tools / resources / prompts live on the children, not on the
coordinator's own surface.
- `_on_mcp_tools_changed` no-ops for coordinator sessions
(defence-in-depth in case listeners were registered).
- `always_on_names` on `ToolSearchManager` is now the set of builtin
tools actually present in the session (kind-aware) rather than the
full `BUILTIN_TOOL_NAMES` frozenset.
- `turnstone/eval.py` uses `INTERACTIVE_TOOLS` (coordinator tools
aren't in scope for the eval harness which tests interactive agent
behaviour).
- Regression tests in `tests/test_workstream_kind.py`:
- `INTERACTIVE_TOOLS ∩ COORDINATOR_TOOLS == ∅` and their union is
`TOOLS`.
- Interactive `ChatSession._tools` does not include any
coordinator tool name.
- Coordinator `ChatSession._tools` contains `spawn_workstream` but
not `bash` / `edit_file` / `memory`; sub-agent lists are empty.
- Coordinator `ChatSession` with an MCP client attached does NOT
merge MCP tools and does NOT register any MCP listeners.
PR review findings:
- **#10 / #11** (Copilot): coordinator UI claimed to reuse the server
renderer pipeline but loaded none of its JS. Mirrored
`turnstone/ui/static/renderer.js` into
`turnstone/console/static/coordinator/renderer.js` (flagged in-file
as a cleanup candidate to promote into `shared_static/`), added
`katex.min.js` / `highlight.min.js` / `renderer.js` script tags to
`coordinator/index.html`. `coordinator.js` now buffers raw markdown
via `textContent` during streaming, then swaps to `renderMarkdown` +
`postRenderMarkdown` on `stream_end`.
- **#7** (Copilot): N+1 query pattern in
`CoordinatorClient.list_children()` — per-row `storage.get_workstream`
just to read `skill_id`. Pushed `skill_id` + `skill_version` into
the `list_workstreams` SELECT projection on both backends; the
client reads them from `row._mapping` directly. New
`test_list_children_skill_filter_avoids_n_plus_one` pins the
behaviour (asserts `storage.get_workstream` call count is 0).
- **#8 / #9** (Copilot): `spawn_workstream` tool JSON said "if empty,
the workstream is created idle" but the prepare method rejected
empty and the field was marked required. Resolved by allowing
empty end-to-end: removed from `required`, prepare builds a
"spawn idle workstream" header + empty preview when empty,
updated `test_spawn_prepare_allows_empty_initial_message`.
- **#1–#5** (github-code-quality): five asserts with side-effecting
method calls in `test_coordinator_manager.py` (`mgr.close`,
`mgr.open`, `mgr.create` in a dead `_c = ...`). Extracted each
call to a local variable so `python -O` can't strip the side
effect.
Verification:
- `ruff check turnstone tests` clean.
- `mypy turnstone` clean (156 source files).
- `pytest -m "not live"` — 4063 passed, 3 deselected (live-backend
tests), 0 failed. The 3 image tests that were failing on this
branch now pass; wheel + lint both green locally.
* polish(coordinator): address Copilot re-review findings
Two findings from the re-review of #368 after the first polish commit.
**user_id wired into `mgr.create()` at the server handlers.** Phase 1
added ``user_id`` to the ``Workstream`` dataclass and
``WorkstreamManager.create()`` signature, but the two call sites in
``turnstone/server.py`` forgot to pass the authenticated caller
through. Result: interactive workstreams created via
``POST /v1/api/workstreams/new`` (including coordinator-spawned
children, which route through this handler) were landing with blank
``user_id``, defeating ownership-based access control on subsequent
sends / approvals / closes (``_require_ws_access`` treats blank
owners as legacy/allowed). Two changes:
- ``server.py:create_workstream`` forwards ``user_id=uid`` — the same
``uid`` already resolved from the auth result (with trusted-service
forwarding preserved).
- ``server.py:open_workstream`` prefers the persisted owner on the
workstream row over the rehydrating caller so reloading someone
else's workstream doesn't silently re-parent it. Falls back to
the authenticated caller when the stored row has no owner
recorded (pre-phase-1 rows).
Regression test in ``tests/test_workstream.py`` pins
``WorkstreamManager.create(user_id=X)`` → ``ws.user_id == X`` so the
manager seam can't regress silently on a future refactor.
**Malformed-JSON recovery allowlist expanded for coordinator args.**
``_prepare_tool()`` has a two-stage salvage path for models that
emit malformed JSON: a regex-extract (fallback 1) and a bare-string
→ primary_key wrap (fallback 2). The fallback-1 key list didn't
include coordinator argument names, so a slightly malformed
``spawn_workstream`` / ``send_to_workstream`` / etc. call would
hard-fail instead of salvaging into a minimal-args dict for retry.
Added ``ws_id`` / ``message`` / ``initial_message`` / ``parent_ws_id``
to the allowlist (kept alphabetised) so the coordinator tools get
the same model-self-correction behaviour as the interactive tools.
Fallback 2 already covers the ``ws_id``-primary-key tools via
``PRIMARY_KEY_MAP``; the regex path matters when the model emits
``{"ws_id": "abc", "message": "..."}`` with a trailing syntax error.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` → 4065 passed, 3
deselected (live-backend), 0 failed.
* fix(coordinator): address ultrareview findings on coordinator workstream kind
Security
- Cross-tenant leak: CoordinatorClient.inspect/list_children now constrain
to the coordinator's own ws_id + direct children; an LLM coerced via
prompt injection can no longer exfiltrate other tenants' workstreams.
- Empty-owner short-circuit bypass: strict equality at coordinator.py
ownership gate and at the storage-fallback branch in coordinator_history;
orphan/system-owned coordinator rows can no longer be rehydrated by
arbitrary holders of admin.coordinator (DoS + history disclosure vector).
- Closed coordinators no longer silently resurrect on subsequent GET —
the Close button is now actually durable across URL revisits and tab
refreshes; rows with state in {closed, deleted} refuse rehydration.
Correctness
- ChatSession.close() now releases the CoordinatorClient httpx.Client
pool; previously every closed/evicted coordinator dropped a connection
pool on the floor until non-deterministic GC.
- open_workstream rehydration now forwards parent_ws_id + kind, so
coordinator-spawned children survive node restart / idle eviction
with their parent link intact instead of becoming silent orphans.
- list_children truncated flag now signals whenever the SQL fetch hit
the page cap (previously permanently False in the no-filter case,
causing confident-but-incomplete summaries from the coordinator).
- ConsoleCoordinatorUI.approve_tools: per-tool auto-approve now checks
auto_approve_tools independently of the blanket auto_approve flag,
so 'Always approve this tool' actually works on the next invocation.
Concurrency
- _spawn_worker no longer falls through to start a second concurrent
worker thread on the same ChatSession when queue.Full fires; instead
send() returns False and the endpoint surfaces HTTP 429.
- _open_locks entries are now refcounted under self._lock and only
popped when the last waiter releases — eliminates the race where a
rehydration-failure path lets two threads serialize on different lock
instances for the same ws_id and trip the "already tracked" guard.
Tests: +6 regression cases covering closed-coordinator refusal,
empty-owner non-admin refusal, queue.Full no-duplicate-worker,
inspect/list_children cross-tenant rejection, and truncated semantics.
|