mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-25 21:34:47 -06:00
e60c19befd5e31376bb606cd380c3564ac4e27df
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
9c9333ebd4 |
fix(tests): isolate metrics-singleton swaps so they don't leak across files
CI failure on main: test_publish_records_metric_outcome saw an empty
calls list — its monkeypatch was patching a different metrics
instance from the one `_publish_models_metadata` reads.
Two changes:
- test_close_reason_persistence.py: replace the bare
`srv_mod._metrics = MetricsCollector()` assignment in `_make_app`
with an autouse `monkeypatch.setattr(srv_mod, "_metrics", ...)`
fixture so the test's metrics swap auto-restores. Other test
files (test_auth.py, test_server_attachments_endpoints.py) carry
the same anti-pattern; left for a follow-up since they're not on
the critical path here.
- test_server_node_models_metadata.py: switch the publish-helper
metric test to a string-form `monkeypatch.setattr("turnstone.
server._metrics", FakeMetrics())` so it replaces whatever binding
the live module currently holds, regardless of what other tests
did to it. Robust against future leaks of the same shape.
|
||
|
Patrick Buckley
|
1358121d52 |
chore(tests): refresh fixtures for path-keyed URL family
Mechanical updates across the test suite to swap legacy
/v1/api/{send,approve,cancel,events,workstreams/close} URLs for the
path-keyed equivalents under /v1/api/workstreams/{ws_id}/<verb>, and
to drop ws_id from request bodies (the path provides it now).
Per file:
- test_session_routes.py: deletes test_close_legacy_mounts_when_handler_provided
(the close_legacy slot is gone); test_send_mounts_post_and_delete_when_dequeue_provided
(added in PR commit 1) stays.
- test_openapi.py: expected-paths set swaps to path-keyed shape;
test_send_endpoint_has_request_body now asserts the OpenAPI for
/v1/api/workstreams/{ws_id}/send.
- test_auth.py / test_auth_identity.py: required_scope and
check_request fixtures swap to path-keyed shape; new tests cover
write/approve/read scope assignment for the path-keyed verbs +
the /node/* proxy mirror.
- test_sdk_server.py / test_sdk_console.py: mock-transport URL keys
swap; bodies drop ws_id.
- test_server_attachments_endpoints.py: ~17 send sites migrated to
/v1/api/workstreams/<ws>/send (a small Python script ran the bulk
rewrite — body ws_id stripped, URL rebuilt).
- test_server_authz.py: cross-tenant approve/close/cancel/events
tests retargeted to path-keyed URLs;
test_events_legacy_query_keyed_url_still_resolves_to_404_for_unknown_ws
renamed to test_events_path_keyed_url_resolves_to_404_for_unknown_ws
with the docstring updated to note the legacy adapter is gone.
- test_close_reason_persistence.py: 7 close sites all swap.
- test_console_routing_proxy.py: route-proxy tests swap to
/v1/api/route/workstreams/{ws_id}/<verb>; the upstream-URL
assertion now reads from .request (route_proxy uses
client.request(method, url, ...) for method passthrough); _wire_proxy
helper installs both .post and .request mocks for compatibility.
- test_route_proxy_audit.py: parametrized URLs migrated;
_make_proxy now also exposes a .request side-effect that delegates
to .post for the same compatibility surface.
- test_api_versioning.py: openapi.json path assertion swaps to the
path-keyed shape.
4557 passing under -m "not live"; ruff + mypy clean.
|
||
|
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
|
553d73109b |
feat(coordinator): phase 5 — harness-test polish + wait_for_workstrea… (#378)
* feat(coordinator): phase 5 — harness-test polish + wait_for_workstream + judge fix
Closes the bug list surfaced by the 2026-04-17 coordinator harness test
plus the post-phase-4 wait_for_workstream ask, and folds in three
adjacent cleanups that landed in the same window. Tightens defense-in-
depth on the model-invoked mutating ops, fixes the LLM judge silent
no-op, kills the inspect-poll token burn, and rounds out a handful of
observability / docstring / spec gaps.
The session-factory pre-resolve at console/session_factory.py and
server.py was rewriting `judge.model` from an alias (e.g. `judge-mini`)
to the resolved underlying id (e.g. `gpt-5-mini`). IntentJudge then
checked `model_registry.has_alias(config.model)`, found nothing, and
fell back to the SESSION's provider/client with that bare model id —
silent `llm_fallback / "did not return a verdict"` whenever the
coordinator and judge alias resolved to different providers.
Pass the alias through unchanged; IntentJudge's existing alias-
resolution path picks up the matching client + provider. Validate
the alias exists so an obvious typo still surfaces, but don't replace
the model field.
Regression: `test_alias_uses_registry_provider_not_session_provider`
constructs an alias whose provider differs from the session's and
asserts the judge picks up the alias's provider/client/model;
`test_coordinator_tool_call_returns_llm_verdict_not_fallback` asserts
the verdict tier is `llm` (not `llm_fallback`) on the happy path.
New `cancel_workstream` tool (approval required, primary_key=ws_id) —
cancels in-flight generation, unblocks any pending approval / plan,
moves the child to idle, leaves the row in storage so a fresh
send_to_workstream lands cleanly. Re-uses the existing
`/v1/api/route/cancel` route + `route.cancel` audit namespace; no
new server endpoint.
`CoordinatorClient.cancel/close_workstream/delete/send` now enforce
a tenant guard inline (`_is_own_subtree`) — only the coordinator
itself or one of its own children is targetable. Foreign ids return
the same 404-shape inspect/wait_for_workstream use, so the model
can't distinguish foreign from missing (no existence oracle).
Defense-in-depth — the upstream node enforcement is the perimeter,
this is the second line.
`list_workstreams` advertised `state="deleted"` and an
`include_closed=true` that surfaced deleted rows. Hard-deletes
cascade the workstream + conversation rows out of storage, so
deleted is unreachable in normal operation. Doc-only fix; the
synthetic-test path that registers `state="deleted"` rows still
works (terminal-state filter still excludes them via
`_terminal_states = {"closed", "deleted"}` in list_children).
Documented that the 120s service-registry heartbeat window means a
node returned by list_nodes can drop out before a follow-up
`spawn_workstream(target_node=…)` lands — the spawn fails with "No
available node for routing" rather than falling back. Two-line
clarification on each tool. No code change (a code fallback is a
bigger discussion deferred to 1.6).
`close_workstream` accepts `reason`; the upstream server handler now
persists it to `workstream_config.close_reason` (capped at 512 BYTES,
sliced on UTF-8 not code points so a CJK / emoji-heavy payload can't
4× the documented budget). `CoordinatorClient.inspect()` reads it
and surfaces as `close_reason` in the result dict — only for
terminal-state children (closed/error/deleted) so the live-child hot
path doesn't pay a per-inspect DB round-trip.
Tests: server-side persistence covers success / no-reason /
length-cap / non-string / storage-failure / multi-byte-utf8 paths;
client-side surface covers terminal vs. live workstreams.
For idle children whose node-dashboard live counter is 0 (the live
block only surfaces in-flight token counters), fall back to
`SUM(prompt_tokens + completion_tokens)` from `usage_events` so the
inspect output reflects cumulative spend.
New `storage.sum_workstream_tokens(ws_id) -> int` on the protocol +
both backends. The fallback is folded INTO `_fetch_cluster_live` so
the merged live block (with persisted total applied) is what gets
cached — back-to-back inspects of an idle child amortize through
the existing 2s LRU cache instead of each firing a fresh aggregation.
`CoordinatorClient.list_skills()` now projects `allowed_tools` per
skill — capped at 20 with a `+N more` sentinel so a skill that
whitelists a wide MCP surface doesn't bloat the per-row payload.
Reads the existing `prompt_templates.allowed_tools` column; no
storage change. Coordinators no longer have to guess what tools a
skill brings.
`route_create` now sets `routing_strategy: "hash_ring" | "target_node"
| "resume"` on the spawn response so the coordinator's spawn
response (and the `spawn_workstream` tool output) carries why a
given node was chosen. 3 lines + 3 covering tests in
test_console_routing_proxy.py.
New coordinator tool `wait_for_workstream(ws_ids, timeout=60,
mode='any'|'all')` that absorbs the wait into a single tool call —
the model sees one call + one result regardless of how long the
children take. Kills the busy-poll inspect loop that burned 20+
turns on a 3-child fan-out.
Storage-poll loop with batched primitives —
`get_workstreams_batch` + `sum_workstream_tokens_batch` issue exactly
two storage calls per tick regardless of N. At the cap (32 ws_ids /
600s / 0.5s tick) that's ~2400 round-trips for a full wait, down
from ~38k under the naive per-id shape.
Validation single-source-of-truth: the client owns mode whitelist,
ws_ids dedup + cap, timeout coerce + clamp. The session preparer
is a thin pass-through that builds the header + dispatches; bad
input surfaces at exec time as a tool error via `result.get("error")`.
Tenant-isolation collapse: missing-row and cross-tenant cases both
return `state="denied"` so wait can't be used as an existence oracle
(matches the 404-mask contract `inspect` uses).
Prompt-side: tools_coordinator.md adds a `wait_for_workstream`
pattern + an explicit "PREFER wait_for_workstream OVER a loop of
inspect_workstream" line in the workflow-shape section.
Replaces the quote-bracketed substring LIKE/ILIKE pattern with proper
JSON-array containment. The previous shape effectively did
`LOWER(tags) LIKE '%"<lower-tag>"%'`, which broke for tag values
containing `"` (the JSON encoder escapes it to `\"` and the literal-
substring search misses), `\` (encoded as `\\`), or non-ASCII
characters that the encoder rendered as `\uXXXX`. Also exposed a
small spoofing surface — `tags=["foo\","bar"]` would have matched a
query for `bar`. Real-world tag values are alphanumeric+dash today
so it hadn't fired in production, but the fix is small.
- SQLite: `EXISTS (SELECT 1 FROM json_each(prompt_templates.tags)
WHERE lower(value) = lower(:tag))` (JSON1 extension; SQLite 3.38+).
- PostgreSQL: `EXISTS (SELECT 1 FROM jsonb_array_elements_text(
prompt_templates.tags::jsonb) AS jat(elem) WHERE lower(jat.elem) =
lower(:tag))`.
Three new tests prove the substring pattern was broken for
quoted / backslash / unicode tag values; the existing case-fold +
wildcard tests continue to pin the contract.
Phase 1 added the coordinator workstream API; phase 2 added only
`/open` to the OpenAPI catalog and missed every other coordinator
endpoint plus phase 3's `/children`, `/tasks`, and the
`/cluster/ws/{ws_id}/detail` aggregator. SDK consumers + operators
browsing `/docs` couldn't discover the surface. Doc-only addition:
12 endpoints + 9 new Pydantic models, all under the `Coordinator`
OpenAPI tag so /docs groups them together.
Sidebar re-fetches `GET /tasks` on every `task_list` `tool_result`
SSE event. A model that runs `add → list` (or any back-to-back
mutation pair) double-fetches the same envelope. Coalesced into
one fetch per 150ms window via a new `loadTasksDebounced` wrapper;
direct UI actions (refresh button, page load) keep calling
`loadTasks` directly so user clicks aren't delayed.
- `ruff check turnstone tests` — clean
- `mypy turnstone` — clean (157 source files)
- `pytest -m "not live"` — 4284 passed, 3 deselected (was 4226 on
main; +58 new tests across coordinator client, tools, judge,
storage, console routing proxy, server close-handler,
storage_skills_filtered, OpenAPI catalog, server close-reason
persistence)
- New tools added: 2 (cancel_workstream, wait_for_workstream) —
TOOLS count 28 → 30; coordinator subset 9 → 11; auto_approve adds
wait_for_workstream; primary_key adds cancel_workstream
- New OpenAPI endpoints: 12 (every phase-1/2/3 coordinator route +
the cluster-inspect aggregator)
- New storage protocol methods: 3 (sum_workstream_tokens,
sum_workstream_tokens_batch, get_workstreams_batch)
All phase 1 / 2 / 3 / 4 invariants preserved: COORDINATOR_TOOLS /
INTERACTIVE_TOOLS disjoint; coordinator sessions have no MCP surface;
list-style tools return {items, truncated}; route-proxy emits
route.<action> audit on 2xx; 404-mask on ownership failures; tenant
filters pushed into SQL; per-coordinator JWT carries scope context.
* fix(coordinator): address Copilot review on PR #378
Three valid Copilot findings on the wait_for_workstream surface:
1. ``wait_for_workstream.json`` description claimed the tool returns a
top-level mapping ``ws_id -> {state, tokens, updated}`` plus
elapsed/complete/mode at the same level, but the actual shape is
``{results: {ws_id: {...}}, elapsed, complete, mode}``. Description
now matches the implementation. Also adds ``deleted`` to the
advertised terminal-state list (it's in ``_WAIT_REAL_TERMINAL_STATES``;
the doc and runtime now agree).
2. ``CoordinatorClient.wait_for_workstream`` docstring listed
``idle / error / closed`` as the real terminal set but the constant
includes ``deleted``. Same fix — list ``deleted`` with a parenthetical
noting it's unreachable in normal operation (hard-delete cascades the
row).
3. Storage protocol docstring math: ``sum_workstream_tokens_batch``
claimed "from ~38k to ~1200" round-trips per wait at the cap, but
``wait_for_workstream`` issues TWO storage calls per tick
(``get_workstreams_batch`` + this one), so 1200 ticks × 2 = ~2400.
Updated to "~2400" with the math spelled out.
Also a clean rebase onto today's main (PR #377 — the rebalancer node_id
snapshot doc — landed since phase 5's last push). Single conflict in
``inspect_workstream.json`` resolved by keeping both notes (rebalancer
node_id binding semantics + the new ``close_reason`` surface from phase
5); ``spawn_workstream.json`` auto-merged.
The github-code-quality bot also flagged three items on
``_protocol.py`` asking to replace ``...`` with ``pass`` in Protocol
method bodies. Refuted: ``...`` is the canonical PEP 544 idiom for
Protocol method bodies and the rest of the file uses it consistently.
The bot's lint rule misfires for ``Protocol`` classes.
Verification:
- ``ruff check turnstone tests`` clean
- ``mypy turnstone`` clean (158 source files)
- ``pytest -m "not live"`` — 4308 passed, 3 deselected (no test count
change; pure doc/comment edits)
|