mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-26 22:04:46 -06:00
cf2811fcebec9919db0701df83155eb4ea75bc97
14 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
480a1426b3 |
Fail-closed history-commit handoff (#1005)
* fix(session): fail-closed history-commit handoff (#981) The deleted-workstream discovery is now a terminal, ws_id-keyed latch: keyed conversation commits refuse admission once the durable parent is gone (convergence finalizers and force-abandon are exempt), history handoff refuses to mint a proof token so /history fails closed with a 503 instead of silently wiping the pane, and the SSE stream carries a workstream_gone resync reason. Discarded commits leave a forensic log of commit keys and roles, never content. Conversation rows gain a commit_key (migration 071): keyed saves are idempotent under retry, validated against the full commit identity, and refused when they would cross a workstream deletion. The prune orphan category now requires a NULL alias plus a two-hour updated grace, with cutoffs computed at discovery time and carried into both dialects' rechecks. The mid-turn interjection queue is owner-partitioned with no per-site mode flags: pops take the acting principal's and unowned rows, other participants' rows are structurally retained, and enforcement lives at queue admission plus the shared before_spawn gates. The retraction ledger is bounded by open pop windows: pops open a window atomically with the queue delete, restores close their ids atomically with the ledger consume, every other exit closes through one helper, and misses for unheld ids record nothing. The workstream-gone latch refuses unattended wakes at all three gates (watcher spawn, claim, delivery pre-pop), and the retry dispatcher regained its pre-envelope cancel/error convergence net. Persistence-state reporting derives through the session bound to each UI instead of a registry lookup by id that failed open to healthy during tombstone retention. The dashboard roster no longer re-inserts ghost entries from trailing activity events, the history tool-outcome scan tolerates interleaved non-turn rows, and the shared handoff-deadline handle owns its own retirement. Single-sourced across call sites: keyed-commit row values, attachment save wrappers, tail-truncation and conflict-resolution bodies for both storage dialects; worker-slot lifecycle field sets; the direct-commit admission frame; queued-row layout accessors; the string-aware comment stripper shared by every JS harness suite. Refs #981 #964 * fix(session): sweep handoff fixes to their sibling surfaces The interactive replay loop treated a system row as a tool-batch boundary, so every tool result after an interleaved row vanished from that pane while the coordinator rendered the same history correctly. Only a conversational turn ends the batch window now, matching the shared outcome index. Accepted user turns clear the composer's attachment chips on the same viewer policy that settles optimistic bubbles rather than on having matched a local bubble, so a workstream created with an upload no longer keeps a chip for an attachment the create dispatch already consumed. The coordinator's raced-Stop arm emits the stream-end hook it inherits alongside the idle state, leaving no unfinalized bubble or unflushed tool output. Ending a session surfaces a failure toast when the request never lands or answers with a non-JSON body. The per-second persistence reconcile now probes each session without blocking: a workstream whose generation and handoff locks are held is skipped until the next pass instead of contending the locks every commit needs. The one-shot repair that gates workstream creation at capacity keeps a definite probe — it has no next pass, and the sessions likeliest to be contended are the ones whose unresolved journals emptied its candidate list. Single-sourced: the attachment lane builds its conversation row through the shared commit-identity builder; the ordinary worker exit releases its slot through the lifecycle owner; both operator surfaces snapshot their counters through one non-consuming helper; the replay preamble loses its per-kind wrappers and its config hook; the browser harness suites share one brace walker; and each in-flight history attempt is one record carrying both its abort controller and its deadline. Refs #981 #964 |
||
|
Patrick Buckley
|
cdc360bd98 |
fix(server): sanitize the retry closure's error display
The retry (_run) closure emitted the raw str(exc) to ui.on_error, so a credential-bearing base-URL in a backend ConnectError (https://user:pass@host) crossed into the dashboard SSE — the confidentiality floor _record_fatal_error enforces, bypassed here. Sanitize the display inline with the same sanitize_error_text redactor. This is separable from the reused-session stale-flag hazard that keeps _run off ensure_error_recorded: that hazard is about recording / idempotency (deferred to #865); this is only the display string. The double state emit and the pre-try no-persist remain in #865. Adds a focused test that a retry-error's on_error is redacted. Flagged by review on #866. |
||
|
Patrick Buckley
|
3af80907c7 |
fix(server): init-message worker exits to error, not idle, on first-turn failure
The initial-message worker (_run_initial) collapsed both cancel and backend-error exits into one `except (Exception, GenerationCancelled)` arm that always stamped state=idle, clobbering the state=error that session.send's _record_fatal_error had persisted+emitted. A spawned child's first-turn backend failure (unreachable model server, exhausted quota, auth error) therefore read as an empty, successful turn — the coordinator's wait/inspect surface reads last_error only for state=='error' — and the real error surfaced only after a manual nudge re-ran the turn synchronously. Split the arm: cancel -> idle, exception -> error. The failed child now settles at state=error and the first wait_for_workstream returns the enriched backend error inline. Also fixes the same latent bug for scheduled tasks, which dispatch through the same endpoint and closure. A failed first turn is deliberately terminal for automated wakes: it settles to a non-ready error terminal, not the idle ready-set that timer/watch wakes recur to, so explicit user/coordinator action reactivates it rather than a silent auto-retry (a self-healing wake-from-error would be a separate wake-gate change). The exception arm routes through a new ChatSession.ensure_error_recorded: a no-op when send already recorded the error in-line (the common backend-boundary path — no duplicate state emit), and the recorder when a pre-try exception (model-registry refresh, user-turn append, system-message recompose) bypassed send's own handler, so state=error always carries a meaningful last_error. Its idempotency guard (_has_persisted_error) is session-lifetime, so ensure_error_recorded is scoped to _run_initial's FRESH first-turn session only; the docstring spells out why a session-reuse caller (retry, /send, coord send, wake) must not route through it until the per-turn error-recorded signal of #865 lands. The other half of making an errored workstream cheap for a model to handle is a stable identifier: the enriched backend error now leads with the model ALIAS the coordinator references everywhere (list_nodes, spawn) and annotates the backend id for the operator — "model=DeepSeek-V4-Flash (id=deepseek-v4-flash)" — so a model routing around a failed model correlates it against those surfaces without a lookup, instead of burning reasoning tokens reconciling the alias against a backend id it never sees anywhere else. Collapses to one token when the alias and id coincide. Tests (TestInitialWorkerFailureState) assert the coordinator-visible manager state and the persisted last_error across the matrix — common- backend and pre-try errors both settle error with a readable last_error; cancel-to-idle settles idle with no error recorded. De-forks the create-app fixture and uses the shared monotonic wait_until helper. The completion-notification honesty surface and the error-recording hygiene of the other send-worker closures (retry / main send / coord send / wake) are deferred to #865. |
||
|
Patrick Buckley
|
e60c19befd |
fix(watch): harden nudge/wake delivery across eviction, cancel, and identity rebinds
Wake path: - Denial metacog nudge moves to the tool channel so it drains with the denied tool batch instead of the next user-message seam. - wake_workstream_if_pending: shared wake gate for watch fires on already-idle workstreams (no IDLE transition for the watcher to observe), wired as wake_fn at every set_watch_runner site via the shared _watch_fire_wake_fn helper (closes over the Workstream OBJECT — after eviction+restore an id-keyed manager lookup would miss). - session_worker exit backstop re-runs the wake gate the moment worker ownership clears: IDLE fans out on the worker thread, so transition-time wakes always landed on the reuse path and no-op'd (the coordinator idle_children strand). - deliver_wake_nudge_from_queue contains GenerationCancelled — it is the wake worker's run() closure and only Exception is caught downstream. Watch delivery: - Terminal fires that cannot reach their workstream are HELD and redelivered on min(interval, 60s) without re-running the command, bounded by MAX_DELIVERY_ATTEMPTS per cycle and the watch's own max_polls across cycles; the poll charge commits durably at hold time so restarts stay budget-bounded. - Restore admission control: per-ws dedup + MAX_CONCURRENT_RESTORES cap, presence-only re-check under the lock, detection-only stall alerts (reclaiming a wedged admission would trade capped degradation for total poll-pool collapse). - Permanent-vs-transient restore taxonomy: corrupt persona stamp and genuinely-missing history (confirmed by a raising storage probe — the resume loader swallows read blips into []) deactivate the watch immediately; everything else holds and retries. - Cancel-race defense: delivery paths re-check is_watch_active before stashing/dispatching, cancel paths write the row BEFORE forget_terminal_dispatched, the HTTP cancel endpoint clears runner state, and a per-tick sweep bounds the residual stash-after-clear interleaving to one check_interval. - Abandon/exhaustion commits are write-then-clear so storage that can read but not write retries the row write instead of re-running the command every cycle; the fresh-fire unrestorable path stashes before its deactivation write for the same reason. Registry follows identity: - The dispatch registry is keyed by _ws_id at registration time; every rebind now moves it: non-fork resume() and /new go through _follow_watch_registration (new key live before the old is removed, never stealing a registration another live session holds), removals are owner-checked so tearing down a watch-restore shell or a resumed-away session cannot unregister a live pane, the restore shell yields to a registration that appears mid-restore, CLI --resume registers after the successful resume, and both the open path and the detail-GET lazy rehydrate wire the registration. Teardown gating and backpressure honesty: - cleanup_session_ui marks ws._closed FIRST under ws._lock — every teardown path (close, close_idle, evict, delete, discard) funnels through it — and session_worker.send re-checks under the same lock, so a wake can never spawn a worker on a torn-down workstream. - Create responses carry initial_message_status when the initial message could not be delivered (queue_full / refused_closed) instead of reading as success; staged attachments survive for the retry; /send surfaces a closed workstream as 404 rather than queue_full. Docs/spec: OpenAPI artifacts regenerated; api-reference documents the new create-response field; TS SDK type extended. Tests: ~30 new pins (cancel races, budget durability across restarts, owner-checked registry moves, teardown gating, stall alerts, backpressure surfaces, wait_until final re-check); wide subsystem sweep green (2353 passed). |
||
|
Patrick Buckley
|
b8a8b04042 |
fix(attachments): drain create-time staged uploads synchronously
A create-time attachment is dispatched on the first turn, but the buffer drain
runs at write time inside the async dispatch worker (_append_user_turn). The
freshly-opened pane calls rehydrate() before the worker drains, so it painted
the image as a still-pending composer chip ("thumbnail in the text input box").
The inlined first-turn dispatch is the only consumer of those staged uploads and
always commits at create, so drain them from the buffer synchronously right
after resolving them — both the interactive and coordinator post-install paths.
The worker's own per-id discard then no-ops.
|
||
|
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
|
0f1d03755a |
refactor(storage): drop dead ws_id/user_id from workstream_attachments
The blob store is global content-addressed — identical bytes dedupe across workstreams and users, so the per-tenant ws_id/user_id scope columns are dead: nothing reads them, and a committed blob is authorised via the conversations.attachments ref-list (attachment_referenced_in_ws), not a row scope. Drop both columns and idx_ws_attachments_ws_id from the schema and from the save_attachment signature (protocol + both backends + memory wrapper + the caller); fold the column/index drops and their downgrade into the unshipped migration 060. tool_name stays: it is a live denormalised search label (search_history → recall + /history), not trajectory data — "never rehydrated" held only for the wire path, which already ignores it. |
||
|
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
|
d068366a61 |
rbac: builtin-role override editor + tighten under-enforced perm gates (#585)
* feat(rbac): editable builtin role permissions via overlay layer
Adds a ``role_permission_overrides`` table that stores per-(role_id,
permission) grant/revoke deltas, applied on top of the immutable
``roles.permissions`` baseline at permission-load time. Builtin roles
(``builtin-admin/operator/viewer``) become customizable through the
admin Roles UI without losing the "reset to default" guarantee — every
override is auditable and reversible.
Motivating case: ``model.skills.write`` is deliberately default-ungranted
on every role so operators must consciously opt in before a coordinator
session can mutate the skill catalog. Until now there was no UX path to
do that opt-in — the only options were dropping into SQL or running a
fresh migration. The overrides editor closes that gap.
Backend
- Migration 057 + storage methods on both sqlite + postgresql backends
- ``get_user_permissions`` merges baseline ∪ grants − revokes for builtin
rows; custom rows pass through unchanged
- ``GET /v1/api/admin/roles/{id}/effective`` for inspect
- ``PUT /v1/api/admin/roles/{id}/overrides`` for write — admin.roles gated,
audited, validates against ``_VALID_PERMISSIONS``, refuses non-builtin
targets, strips no-op grants/revokes before persisting
- Lockout guard: cannot revoke ``admin.roles`` if doing so would leave
zero users with the permission (returns 409)
- ``coordinator.trust.send`` added to ``_VALID_PERMISSIONS`` — was
seeded into builtin-admin by migration 042 but never registered with
the validator, so the very first round-trip through the editor 400'd
on it. Drift-detection test guards future migrations from recreating
the same gap
Frontend
- Roles tab redesign: chevron + permission-count chip replace the
"..." truncation; expand-on-click drawer groups perms by namespace
with baseline / grant (green +) / revoke (red −) chip variants
- Edit modal opens for builtin rows ("Customize Built-in Role" title);
toggles show baseline-default vs override state; submit diffs against
the rendered toggle universe (not raw baseline) so future taxonomy
drift can't silently strip unknown perms
- "Modified +N/-N" pill on rows with active overrides; "Reset to default"
drawer action clears the override set
- ``_PERMISSION_SECTIONS`` brought up to date with all currently-seeded
perms (admin.coordinator, admin.cluster.inspect, admin.models,
admin.nodes, admin.prompt_policies, conversation.modify,
coordinator.trust.send were missing)
Tests
- 7 storage tests covering set/list/clear/effective + overlay merge into
``get_user_permissions`` for both builtin and custom roles
- 11 endpoint tests covering effective/overrides happy paths, validation,
lockout guard, builtin-only restriction, no-op normalization, list
enrichment
* feat(rbac): enforce workstreams.{create,close} + tools.approve gates
These three permissions were declared in ``_VALID_PERMISSIONS``, seeded
into ``builtin-operator``'s baseline by migration 008/017, surfaced in
the admin Roles UI as toggles, and documented in ``bootstrap.py`` as
the operator role's capabilities — and never enforced anywhere. The
audit that ran out of the overlay PR found zero ``require_permission``
sites for any of them; any authenticated user could create workstreams,
close any workstream, or approve any pending tool regardless of role.
Behaviour change for callers without the perms:
- ``POST /v1/api/workstreams/new`` (node + console proxy variants)
now 403 without ``workstreams.create``
- ``POST /v1/api/workstreams/{ws_id}/close`` (and ``/route/`` proxy)
now 403 without ``workstreams.close``
- ``POST /v1/api/workstreams/{ws_id}/approve`` (and ``/route/`` proxy)
now 403 without ``tools.approve``
The OR-fallback to ``admin.coordinator`` keeps coord sessions spawning
interactive children unblocked without needing operator-style perms.
Service-scoped inter-cluster calls bypass via the existing
``allow_service_bypass`` path on the new ``require_any_permission``
helper. Builtin admin and operator both already carry these perms;
viewer correctly loses workstream create/close/approve (it already
couldn't do those in spirit).
Implementation
- ``require_any_permission`` (core/auth.py) — OR-semantics variant of
``require_permission`` with per-conditional comments documenting the
security policy at the choke point. 403 body names every accepted
perm so operators get an actionable remediation
- ``make_{create,close,approve}_handler`` (core/session_routes.py)
accept ``fallback_permissions: tuple[str, ...]`` — checked only when
``cfg.permission_gate is None`` (interactive case). Coord's
``permission_gate=_require_admin_coordinator`` continues to take
precedence on the coord-config side
- Console-side ``create_workstream`` and ``route_create`` inline the
same OR check before proxying — fail fast on a forbidden request
without burning a cluster round-trip
- ``route_proxy`` adds a verb-scoped gate on ``approve`` and ``close``
only; ``send``/``cancel``/``dequeue``/``command``/``plan`` remain
authenticated-only (pre-existing, out of scope for this audit)
Tests
- New ``TestPermissionGatesOnLifecycle`` (4 tests) in test_server_authz
pinning 403-without-perm + non-403-with-perm at the node lift sites
- New ``TestRouteProxyPermissionGates`` (5 tests) in
test_console_routing_proxy covering 403 paths, OR fallback via
``admin.coordinator``, and that ``send`` remains ungated
- ``_make_jwt`` helpers in test_server_authz, test_close_reason_
persistence, test_server_attachments_on_create updated to embed
operator-shaped perms by default so existing tests continue to
exercise the post-gate logic rather than 403'ing on the new check
Docs
- ``bootstrap.py`` operator role line corrected to list every perm
it actually carries (was missing ``tools.approve`` and
``conversation.modify``)
* fix(rbac): close lockout + escalation gaps in role-overrides editor
Three issues surfaced by /review of the overlay layer and gate uplift —
all in the RBAC/auth surface, treated as zero-days.
**F-1: lockout guard misses the grant-removal path.** PUT-replace
semantics on ``set_role_overrides`` mean an existing grant of
``admin.roles`` (added via override to e.g. builtin-operator) is
silently dropped when the new payload omits it. The previous guard
short-circuited on ``"admin.roles" not in revokes`` and never noticed.
Concrete cluster-bricking scenario: grant admin.roles to operator via
override, unassign builtin-admin, click "Reset to default" on operator
→ all users lose admin.roles, recoverable only via SQL.
The rewritten guard simulates the post-PUT effective set on the target
role directly: if ``(baseline | new_grants) - new_revokes`` lacks
admin.roles AND nobody holds it via another role, refuse the change.
The "via another role" question is answered by one bulk query rather
than the prior O(users × roles) round-trip loop.
**F-3: lockout check blocked the event loop on moderate deployments.**
The prior check called ``storage.list_user_roles`` per user and
``storage.effective_role_permissions`` per (user, role) pair —
synchronous SQL inside an async handler. 200 users × 5 roles = 1000
connection cycles long enough to trip reverse-proxy timeouts on a
permission revoke.
Replaced with ``storage.users_with_permission(perm, *,
exclude_role_id)`` — one join over ``user_roles ⋈ roles`` plus one IN
fetch on overrides for the builtin role ids in the result, folded
in-process. Two queries total, independent of cluster size. The whole
check now runs under ``asyncio.to_thread`` so even the bulk read
doesn't stall the loop.
**F-2 reframed: admin_assign_role's subset check ignored the overlay.**
The check at lines 6321-6328 reads ``target_role.get("permissions",
"")`` (baseline column) when computing the perms it requires the
caller to hold. After this branch, an admin.roles holder can grant
e.g. ``model.skills.write`` to builtin-operator via override; an
admin.users holder (who happens to NOT hold that perm) could then
assign operator to a new user, silently escalating the assignee. The
existing two-person-rule by perm split (admin.roles for catalog edits,
admin.users for assignments) only holds if the assignment-time check
considers the overlay. Switched ``target_perms`` to
``storage.effective_role_permissions(role_id)["effective"]``.
Note: this PR retains the existing model where admin.roles is the
catalog-edit superuser (admin_create_role, admin_update_role, and now
admin_role_overrides all skip the caller-holds-grants check). The
two-person rule against escalation lives at the assignment gate, which
this fix reinforces.
**F-7: delete_role left orphaned override rows.** No FK on
``role_permission_overrides.role_id`` (migration 057 omitted FKs to
match the rest of the governance schema). Added explicit cleanup in
both sqlite + postgresql ``delete_role`` implementations so a
re-seeded role_id (deterministic for builtins on schema reseed) can't
silently inherit stale overrides from the prior occupant.
Tests
- storage: ``test_users_with_permission_bulk`` exercises the new bulk
helper including ``exclude_role_id`` and overlay folding
- storage: ``test_delete_role_cleans_up_overrides`` pins the F-7 fix
- endpoint: ``test_overrides_lockout_guard_blocks_grant_removal`` is
the F-1 reproduction — operator-overlay grants admin.roles, builtin-
admin has it removed, attempting to reset operator's overrides 409s
- endpoint: ``test_assign_role_blocks_escalation_via_overlay_grant``
pins the F-2 reframed fix — overlay-poisoned operator can't be
assigned by a caller missing the overlay perms
* refactor(rbac): cleanup batch from /review (#584)
Five non-security findings folded into one commit so the security
batch stays focused. All consistent with the existing intent of
``feat/builtin-role-overrides``.
**F-4: presence check on ``_effectivePerms``.** ``governance.js`` was
guarding on ``Array.isArray(role.effective) && role.effective.length > 0``,
falling through to splitting ``role.permissions`` (the baseline) when
the array was empty. For a builtin role whose overrides legitimately
revoke every baseline perm, that path silently rendered the baseline
chips with no override indicators — the inspector lied about what the
role can do. ``_enrich_role`` always sets ``effective: []``, so
presence is the right sentinel.
**F-5: JS-side drift detector.** Commit 1 added a Python-side test
asserting ``_VALID_PERMISSIONS`` covers every baseline perm; the
mirror invariant on the frontend went uncaught. A new perm added to
``_VALID_PERMISSIONS`` without a matching entry in
``_PERMISSION_SECTIONS`` becomes silently un-customizable through the
admin UI (the only documented grant/revoke path). Test parses the
JS const out via regex and asserts set-equality both directions —
detects "missing in UI" and "extra in UI" so the toggle catalog and
validator can't fork.
**F-6: bulk enrich for ``admin_list_roles``.** Was ``1 +
2*builtin_count + 1*custom_count`` SELECTs per admin-tab open;
collapsed to one ``IN``-filtered query via new
``storage.effective_role_permissions_bulk(role_ids)``. Implemented
on both sqlite + postgresql backends following the existing
``effective_role_permissions`` shape.
**F-8: rename ``fallback_permissions`` → ``accepted_permissions``.**
The lift body uses ``if cfg.permission_gate / elif accepted_permissions``
— mutually exclusive — so when ``permission_gate`` is None this IS
the primary gate, not a fallback to anything. The "fallback" name
suggested a tier-2-after-tier-1 semantic that didn't exist. Renamed
across ``make_{approve,close,create}_handler`` factories, the three
call sites in ``turnstone/server.py``, and the docstrings.
**F-9: positive lift-level tests for ``admin.coordinator``-only.**
``TestPermissionGatesOnLifecycle`` previously had a single positive
test for ``workstreams.create`` alone, plus negative-403 tests for
each verb without perms. The OR-fallback to ``admin.coordinator``
(which keeps coord sessions spawning interactive children unblocked)
had no positive coverage at the lift code path — only at the proxy,
which exercises a different verb-dict gate. Added three tests
(create / close / approve) that pass ``admin.coordinator`` alone and
assert non-403, so a future tightening of the accepted_permissions
tuple can't silently regress coord-driven child workstreams.
Out of scope: nit perf-4 (event-delegation refactor on
``_renderGovRoles``). ``setSafeHtml`` rebuild is the existing
pattern across every admin tab; rewriting one tab's render path on
this branch would be drive-by inconsistent with the surrounding
codebase. Filed as a separate concern if the Roles tab grows past
the scale where it bites.
* fix(rbac-ui): aria-expanded + row-click on Roles drawer (#585)
Two Copilot review findings on governance.js:
- Expand button was missing aria-expanded — screen readers couldn't
announce drawer state. Now reflects the row's expanded flag.
- Comment said "row + chevron both work" but only the chevron was
wired. Added data-expand-role to the row element too so the
existing handler loop (querySelectorAll on the attribute) picks up
both — clicking anywhere in the role row toggles the drawer.
Edit/Delete handlers already stopPropagation so they aren't
triggered by the row-level click.
* fix(migrations): rebase role_permission_overrides to 058
PR #560 mitigation #1 landed 057_output_assessments_llm_judge.py on
main in parallel; my migration claimed the same number, forking
alembic's head and breaking postgres. Renumbered to 058 and
re-pointed down_revision at 057 so the chain stays linear.
No behaviour change — same DDL. Full sweep clean (6730 passed).
* fix(migrations): update 058 revision strings to match filename
Previous commit (
|
||
|
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
|
48c9ad2a40 |
refactor(core): split SessionKindAdapter Protocol into construction +… (#412)
* refactor(core): split SessionKindAdapter Protocol into construction + emission (Stage 2 P3)
The single ``SessionKindAdapter`` Protocol that ``SessionManager``
takes is split into two:
* ``SessionKindAdapter`` — kind / build_ui / build_session /
cleanup_ui. Required for every kind. The shared lifecycle
manager always delegates here for construction + cleanup.
* ``SessionEventEmitter`` — emit_created / emit_state /
emit_rehydrated / emit_closed. **Optional**, wired through a new
``event_emitter: SessionEventEmitter | None = None`` kwarg on
``SessionManager``. Reserved for future kinds whose lifecycle
transitions don't fan out anywhere; both production kinds wire
one today.
Both production adapters implement both Protocols. The interactive
lifespan (``server.py``) and console lifespan
(``console/server.py``) pass their adapter as both ``adapter`` and
``event_emitter`` — production behaviour is unchanged. Six lifecycle
sites in ``SessionManager`` (create / open eviction / open rehydrate /
close / set_state / close_idle / _reserve_and_install_locked unwind)
now call ``self._event_emitter.emit_*(...)`` guarded by
``if self._event_emitter is not None``.
InteractiveAdapter asymmetry preserved + documented:
* ``emit_closed`` stays load-bearing — it's the **sole** transport
path for ``ws_closed`` onto the process-wide global SSE queue
(Stage 1 consolidated emission from the create handler here so
there's exactly one emission point; ``name`` powers the
frontend's eviction toast).
* ``emit_created`` / ``emit_state`` / ``emit_rehydrated`` are
documented no-op stubs (``del ws[, state]``). Those events fire
from out-of-band paths — the create HTTP handler enqueues
``ws_created`` directly onto ``global_queue`` *after* attachment
validation (so a rejected upload doesn't surface a phantom
create→close pair); ``WebUI._broadcast_state`` emits the full
``ws_state`` payload (tokens + context_ratio + activity) via the
``SessionUI.on_state_change`` callback chain. The stubs exist
solely to satisfy ``SessionEventEmitter`` Protocol so the
adapter can be wired as the manager's ``event_emitter`` for the
``emit_closed`` path. Each stub has a 1-line inline rationale to
match the in-repo convention (``coordinator_adapter.py:210``).
Test scaffolding:
* ``tests/test_session_manager.py`` — ``_make_manager`` and
``_make_with_writer`` wire ``FakeAdapter`` as both ``adapter``
and ``event_emitter`` for production parity; the standalone
``test_create_uses_configured_node_id`` does the same.
``FakeAdapter.emit_rehydrated`` now records as
``_Event("rehydrated", ...)`` rather than conflating with
``"created"``, and ``test_open_resurrects_closed_state`` asserts
against ``events_of("rehydrated")`` so a regression where the
manager fires the wrong call on the open path actually fails.
* ``tests/_coord_test_helpers.py`` and
``tests/test_coordinator_end_to_end.py`` — wire
``CoordinatorAdapter`` as both args.
* Six interactive test fixtures (``test_skills.py``,
``test_prompt_templates_runtime.py`` x2, ``test_model_registry.py``,
``test_server_authz.py``, ``test_server_attachments_on_create.py``)
— wire ``event_emitter=adapter`` so they match the production
wiring, removing the footgun where a future contributor adds a
``gq.get_nowait()`` assertion and silently loses the only
``ws_closed`` transport.
* ``tests/test_interactive_adapter.py`` — drops the three
tautological no-op-emit_* tests (``test_emit_created_is_noop``,
``test_emit_state_is_noop``, ``test_emit_rehydrated_is_noop``);
keeps the four ``emit_closed`` tests (real behaviour).
Lint + mypy clean. 4475 tests passing.
* docs(core): correct SessionKindAdapter + SessionEventEmitter docstrings to match implementation
Two Copilot review threads on PR #412 caught the same real
discrepancy: my P3 docstrings on ``SessionKindAdapter`` and
``SessionEventEmitter`` described an *intent* — "interactive
doesn't implement ``SessionEventEmitter``; the manager skips emit
calls when no emitter is wired" — that doesn't match the actual
wiring. ``InteractiveAdapter`` does implement both Protocols and
``server.py`` does pass it as ``event_emitter``; only the three
no-op stubs (``emit_created`` / ``emit_state`` / ``emit_rehydrated``)
are dead, while ``emit_closed`` is load-bearing.
Updated both docstrings to:
* State that both production adapters implement both Protocols.
* Explain the asymmetry is in *which* emit methods carry real
bodies (coord: 4; interactive: 1, with 3 documented stubs because
the out-of-band paths — create handler ``ws_created`` after
attachment validation, ``WebUI._broadcast_state`` carrying the
richer ``ws_state`` payload — fire those events).
* Clarify the ``if self._event_emitter is not None`` guard exists
for the kwarg-omitted case (tests that don't care about events,
reserved for future kinds whose transitions don't fan out
anywhere).
Docstring-only change. Lint + mypy clean; the 75 tests in
test_session_manager + test_interactive_adapter + test_coordinator_adapter
pass.
Resolves the two Copilot review threads on PR #412 (commits
PRRC_kwDORcMomM67VyPD, PRRC_kwDORcMomM67VyPI).
|
||
|
Patrick Buckley
|
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
|
6cbd3eb2c1 |
feat: workstream attachments at creation time + SDK + UI parity (#362)
* feat: workstream attachments at creation time + SDK + UI parity Closes the two big deferred items from PR #356: attaching files as part of the initial workstream-creation request, and full SDK coverage of the attachment surface. Server: POST /v1/api/workstreams/new now accepts multipart/form-data (meta JSON + 0..N file parts). Files are validated and saved as pending under the new ws; when initial_message is also set the create handler reserves them onto that turn before the dispatch worker fires, mirroring the /v1/api/send pattern. Validation failure rolls back the workstream via delete_workstream so we don't leak orphan rows or emit a phantom ws_created/ws_closed pair on SSE. JSON path is unchanged. Console routing: route_create accepts multipart with ?ws_id=<hex> as a query parameter (the console hashes the id before the body lands). Added /v1/api/route/workstreams/{ws_id}/attachments POST/GET/DELETE + .../{attachment_id}/content GET proxies that forward raw bytes and preserve upstream headers (Content-Disposition, X-Content-Type-Options, CSP sandbox). Python + TypeScript SDKs: AttachmentUpload type, upload_attachment, list_attachments, get_attachment_content, delete_attachment, and send(attachment_ids=...). create_workstream(attachments=...) sends multipart and pre-generates a ws_id client-side so cluster routing works. SDKs reject attachments+target_node combinations since the multipart route doesn't honor target_node. Web UI: dashboard composer refactored to a single unified create flow. Replaced the inconsistent split (Enter created+sent raw, "New Chat" opened a modal) with one rich composer carrying a textarea, paperclip + chip strip, drag-drop, paste-image, and a collapsible Options panel for model/judge_model/skill. Submit button dynamically labels Create vs Send. New-workstream modal also gained the same paperclip + chip strip + first-message field for the tab-bar + entry point. Tests: 30 new tests across server multipart create, console route multipart + attachment proxies, Python + TS SDK attachment surfaces, plus regressions for the three review-flagged bugs (Content-Type boundary preservation, attachments+target_node rejection, no phantom ws_created on validation failure). * fix: address Copilot review feedback on PR #362 - web_helpers: docstring now matches behaviour — read_multipart_create_or_400 does enforce the optional max_per_file_bytes cap as defense-in-depth. - app.js: drop the duplicated _formatAttachSize definition (one already exists earlier for pane chips); add a shared _isAttachmentAllowed helper that mirrors the server's classifier (png/jpeg/gif/webp images, text/* MIMEs, allowlisted application/* MIMEs, known text extensions) and call it from both _newWsAddFiles and _addDashboardFiles so unsupported files fail fast client-side instead of after a server roundtrip. - app.js: dashboardSubmit catch now suppresses the redundant error toast on authFetch's "auth" Error and falls back to a generic message when err.message is undefined, instead of rendering "Connection error: undefined". - SendResponse (Pydantic + TS): document and expose attached_ids, dropped_attachment_ids, priority, and msg_id so attachment-aware SDK callers can detect partial reservations and dequeue queued messages. - test_server_attachments_on_create: drop the dual `import turnstone.server` + `from turnstone.server import` style — use monkeypatch.setattr by dotted path for module-level mutation and `from … import …` for the helpers, keeping a single import style. |