3 Commits

Author	SHA1	Message	Date
Patrick Buckley	dea2729292	refactor(coordinator): rename task_list → tasks, doc/prompt sweep (#437) ... Four themes from a coordinator-feature shakedown: 1. Correctness fixes (return shapes / examples / behavior) - tools_coordinator.md: drop fake skill names from spawn examples; fix wrong kwarg ``node_id=`` → ``target_node=``. - wait_for_workstream.json: document ``message`` + ``truncated`` per-ws fields (always enriched in the client; the JSON shape lagged the docstring). - cancel_workstream.json: document the conditional ``dropped`` payload — ``was_running`` always present when ``dropped`` is, ``pending_approval`` and ``queued_messages`` conditional sub-shapes. - spawn_workstream.json: document full return shape including ``routing_strategy ∈ {rendezvous, target_node, resume}`` and ``status``. - close_all_children.json: clarify ``skipped`` covers BOTH hard-deleted children AND already-closed-and-evicted children (wire shape doesn't distinguish); drop incorrect "echoed back in response" claim — server returns ``{status, closed, failed, skipped}``, never echoes ``reason``. - console/server.py: comment in ``_fanout_on_children`` clarifying that the 400 "No session" branch fires for cancel-cascade callers and is unreachable from close_all_children (close handler 404s instead). - coordinator_client._utc_now_iso(): switch to bare ISO format matching the rest of the storage row format used in the codebase. 2. Tightened the 11 longest tool descriptions (~23% cut on the coord set). Removed ALL-CAPS emphasis, normalised em-dashes, dropped informal phrasing. No new claims. 3. Removed static approval annotations from descriptions. Approval is governed at runtime by the unified ``approve_tools`` body and admin-defined ``tool_policies`` (#436); static "Auto-approved" / "Approval required" / per-action approval tags become a stale signal. Field names (``pending_approval``) and operational verb behaviour ("cancel unblocks pending approvals") stay. 4. Renamed ``task_list`` coord tool → ``tasks``. The previous name compounded the bare word ``task`` (which collides with chat-template channels on local models — same reason ``task_agent`` carries the suffix); the plural form sidesteps the collision and reads more accurately, since the tool acts on the whole list rather than a single task. Sweep covers tool JSON, Python methods (5 client methods + 2 session methods + 1 helper + 1 constant), audit event name (``task_list.update`` → ``tasks.update``), log tag (``task_list.corrupt_envelope`` → ``tasks.corrupt_envelope``), frontend SSE event matcher, prompts, docs, and tests. CHANGELOG entry added. Plus: dropped the ENV block (Output Environment / Available rendering / Formatting principles) from coordinator system prompts. Coordinators orchestrate rather than render rich output to the user, so the rendering capability matrix is not actionable for them. Coord prompt drops ~29% (6309 → 4493 chars). SDK regeneration via ``generate-types.py`` updates both ``openapi-console.json`` (the rename's downstream change) and ``openapi-server.json`` (PR #436 drift — its merge added ``pending_approval_detail`` + ``recent_auto_approvals`` fields to the Python schemas but didn't regenerate the JSON artifact). ## Behavior changes (operator-visible) - Audit event name: ``task_list.update`` → ``tasks.update``. Audit dashboards / SIEM filters / log greps that pinned the old prefix should update. - SSE ``tool_result`` events now ship ``name="tasks"`` for the scratchpad tool. The bundled coord-tree UI is updated atomically; external consumers reading SSE events by tool name need to update. - Existing task envelopes in production storage have ``+00:00`` timestamps from the old ``_utc_now_iso``. New writes are bare; old rows are not backfilled. Within an envelope you may briefly see mixed formats until each row is re-touched. No code path string-compares timestamps within an envelope, so this is cosmetic. ## Validation - ``ruff check`` + ``ruff format --check`` clean - ``mypy turnstone/`` clean (175 source files) - ``pytest -m "not live"`` — 4679 passed, 3 deselected	2026-04-27 22:51:31 -07:00
Patrick Buckley	9826ea15c5	feat(coordinator): phase 7 — governance + skill metadata + cross-cutt… (#383) ... * feat(coordinator): phase 7 — governance + skill metadata + cross-cutting invariants Combines three stacked sub-PRs into a single coordinator phase-7 shipment against the phase-7 plan doc. The sub-PR structure (0 / A / B) preserved on individual branches for reviewer drill-down; this branch is the one reviewers should merge. ## Sub-PR 0 — service-auth boundary invariants Shared helpers and contracts that lock the console ↔ node service-auth boundary so later authz surfaces use them by construction. - ``_effective_user_filter(request)`` in both ``turnstone.console.server`` and ``turnstone.server`` with a shared ``DENY_EMPTY_SUB`` sentinel on ``turnstone.core.auth``. Three-way return — admin/service bypass, scoped caller uid, or fail-closed sentinel on blank sub. Four callsite migrations (``_coordinator_rows``, ``coordinator_children``, ``coordinator_metrics``, ``cluster_ws_live_bulk``). - ``StorageBackend`` class docstring codifies the tenancy contract (every list/count/aggregate method must accept ``user_id: str | None = None`` and push ``WHERE user_id = :user_id`` into SQL) and the ``_mapping`` row-access contract. New ``turnstone.testing.row_contract`` ships ``assert_row_like()``. - ``_verify_collector_service_scope`` probes an upstream node at boot with ``expected_node_id=_scope-probe_``; a 409 proves the scope gate was passed, a 403/401 sets ``collector_scope_error`` and causes ``cluster_snapshot`` / ``cluster_events_sse`` to return 503 with a remediation hint. Probe URL allowlist rejects non-http(s) schemes and 169.254.0.0/16 hosts. - 4xx log-level floor on ``_NodeDashboardCache.get``, ``_fetch_live_block``, and ``_proxy_sse`` — dotted-hierarchy prefixes with bounded body previews. ``_bounded_body_preview`` and ``_bounded_stream_preview`` strip control chars. ## Sub-PR A — coordinator governance core Mid-session governance surface for coordinator workstreams. - **Trusted-session mode.** New ``coordinator.trust.send`` permission (migration 042). ``ChatSession.set_trust_send`` / ``revoke_tools`` methods with a ``_governance_lock``. ``POST /v1/api/coordinator/{ws_id}/trust {send: bool}`` double-gated on ``admin.coordinator`` AND ``coordinator.trust.send`` with ``allow_service_bypass=False`` so service tokens can't escalate. ``_prepare_send_to_workstream`` auto-approves sends whose target is in the coordinator's own subtree; foreign ws_ids still require approval. ``_is_own_subtree`` checks both ``parent_ws_id`` AND ``user_id`` to defend against cross-tenant row corruption. - **Audit-layer credential redaction.** ``record_audit`` walks ``detail`` (dicts, lists, tuples, sets, frozensets; keys too) and routes every string through ``redact_credentials`` + a C0 control-char scrub. New kw-only ``raw_detail=True`` opt-out. ``_has_any_string`` fast-path. Audit action registry extended with the four new governance sub-prefixes. - **Mid-session revocation + cascading stop.** ``POST /v1/api/coordinator/{ws_id}/restrict {revoke: [...]}`` caps 256 entries / 128 chars; ``_prepare_tool`` short-circuits with a tool-error. ``POST /v1/api/coordinator/{ws_id}/stop_cascade`` cancels the coord's in-flight generation then dispatches ``cancel_workstream`` for every direct child in parallel via ``asyncio.gather`` bounded by ``Semaphore(16)``. Per-child outcomes split into ``cancelled`` / ``failed`` / ``skipped`` (404 = already-gone rather than dispatch-broken). Both endpoints apply ``allow_service_bypass=False`` on the admin gate. - **Shared plumbing.** ``_resolve_coord_session`` helper collapses the handler prelude three endpoints shared. ``_emit_coord_audit`` wraps ``record_audit`` in a dedicated ``ThreadPoolExecutor`` (``app.state.audit_executor``) so audit bursts don't starve cancel dispatches. ``_require_json_object`` guards body parsing so non- object JSON returns 400 instead of 500. ## Sub-PR B — skill metadata governance - **Description validator (migration 043).** ``prompt_templates`` rows now require a non-empty ``description``. Existing empty rows get backfilled with a ``"Skill: <name>"`` placeholder on upgrade. The installer (``admin_skill_discover``) and MCP prompt sync both synthesise a placeholder when the upstream description is blank so non-admin write paths satisfy the invariant. - **Skill kind classifier (migration 044).** New ``prompt_templates.kind`` column (``interactive`` / ``coordinator`` / ``any``; defaults to ``any``). New ``turnstone.core.skill_kind.SkillKind`` StrEnum is the single source of truth; Pydantic schemas type ``kind`` as ``SkillKind`` (OpenAPI advertises the enum) and the handler validator catches the ValueError. ``list_skills_filtered`` gains a ``kinds: list[str] | None = None`` SQL filter. ``CoordinatorClient.list_skills`` defaults to ``kinds=["coordinator", "any"]`` so interactive-only skills are hidden from the orchestrator. - **``scan_status`` → ``risk_level`` rename (migration 045).** Lossless column rename to align with ``IntentVerdict.risk_level`` terminology. Swept storage (both backends + schema + protocol), handlers, API schemas, tool JSON, generated OpenAPI specs, TypeScript SDK types, frontend (``governance.js``), tests, and English prose in ``docs/judge.md`` + ``docs/tools.md``. The user-facing on-load warning now reads ``has risk level: {risk_tier}``. Tool JSON's ``risk_level`` enum corrected to the scanner's actual taxonomy (``safe / low / medium / high / critical``; was the never-shipped ``clean / flagged / unscanned / pending``). Historical migration 021 left untouched. ## Migrations 042 (``coordinator.trust.send`` perm — PR A) 043 (description backfill — PR B) 044 (``kind`` column add — PR B) 045 (``scan_status`` → ``risk_level`` rename — PR B) All four use position-anchored permission strings / host-side parse-filter-rejoin on downgrade where SQL ``REPLACE`` could corrupt prefix-overlapping values. ## Verification - ``ruff check turnstone tests`` clean. - ``mypy turnstone`` clean on 165 source files. - ``pytest -m "not live"``: 4431 passed (+85 over the phase-6 baseline). Includes +32 tests in ``tests/test_service_auth_boundary.py`` and +38 in ``tests/test_coordinator_governance.py``; shared fixtures extracted to ``tests/_coord_test_helpers.py``. - Generated OpenAPI JSON (``sdk/typescript/openapi-{console,server}.json``) regenerated via ``sdk/typescript/scripts/generate-types.py``; zero ``scan_status`` occurrences remaining outside the historical migration 021 and the rename migration 045. ## Security reviews Both reviews flagged by the phase-7 plan (items 1 + 5, plus 0a's refuse-to-serve gate) ran through the multi-stage ``/review`` pipeline twice per sub-PR; all confirmed findings landed in-branch. * fixup(phase-7): CI lint + PR #383 review fixups Addresses the lint CI failure (ruff format) plus 12 findings from the two automated PR reviewers. Copilot: - ``_sqlite.list_installed_skill_urls`` / ``_postgresql.list_installed_skill_urls`` used positional row indexing (``r[0]``/``r[1]``/``r[2]``) while this same PR's ``StorageBackend`` class docstring forbids it. Switched both to ``r._mapping["..."]`` access. - ``list_skills.json`` previously advertised ``risk_level=""`` as a filter for unscanned skills, but the implementation treats empty strings as "no filter". Clarified the tool description to say omit the filter entirely to include unscanned rows, and added an explicit ``enum`` on the parameter restricting it to the scanner tiers. ``_prepare_list_skills`` keeps the ``strip() or None`` normalisation — unscanned filtering now has an unambiguous contract. - ``test_storage_skills_filtered.test_risk_level_filter`` used the legacy ``clean`` / ``flagged`` values from the pre-rename column. Rewritten with the scanner's actual taxonomy (``safe`` / ``high``). github-code-quality (CodeQL): - ``test_deny_sentinel_is_singleton`` previously asserted ``cs.DENY_EMPTY_SUB is cs.DENY_EMPTY_SUB`` — an identical-expression comparison. Rewritten as two separate ``from ... import ... as`` aliases (``FIRST_READ`` / ``SECOND_READ``) so the identity check is between distinct bindings. - ``test_restrict_empty_revoke_is_noop_but_audits`` unpacked ``state`` without using it. Renamed to ``_state``. - Mixed import styles in ``test_service_auth_boundary.py`` — the file previously used both ``import turnstone.console.server as cs`` and ``from turnstone.console.server import ...`` for the same module (same story for ``turnstone.core.auth`` and ``turnstone.server``). Consolidated to the ``from X import Y`` style used elsewhere in the file; the ``_fetch_live_block`` test now patches via pytest's ``monkeypatch`` fixture instead of a manual rebind through a module alias. CI: - ``ruff format`` reformatted one line in ``tests/test_coordinator_endpoints.py``. Verification: ruff check + mypy clean (166 files); 4459 non-live pytest pass. * fix(tests): swap asyncio marker for anyio in service-auth boundary tests PR #383 CI caught that the 13 ``@pytest.mark.asyncio`` decorators I added in ``test_service_auth_boundary.py`` are an off-convention choice — the rest of the repo uses ``@pytest.mark.anyio`` (148 sites vs my 13). The CI environment pulls in ``anyio`` but not ``pytest-asyncio``, so every async test in this one file was failing with "async def functions are not natively supported". It passed locally by accident — my dev venv happens to have pytest-asyncio installed ambiently. Swapped all 13 marker sites to ``@pytest.mark.anyio``. No functional change; the tests run under the same default asyncio backend anyio provides. Verification: ruff + mypy clean (166 files); 4459 non-live pytest pass.	2026-04-18 10:20:19 -07:00
Patrick Buckley	7492816ab2	feat: governance — RBAC, tool policies, prompt templates, usage track… (#39) ... * feat: governance — RBAC, tool policies, prompt templates, usage tracking, audit logging Add comprehensive governance layer for the admin console: - RBAC with 15 granular permissions, 3 builtin roles (admin, operator, viewer), custom role CRUD, user-role assignment with privilege escalation prevention - Tool policies with glob pattern matching, priority-ordered evaluation (allow/deny/ask), enforced before auto-approve in WebUI.approve_tools() - Prompt templates with variable substitution, categories, default flag - Usage tracking: per-LLM-request token/tool metrics, aggregated queries (group by day/model/user), automatic 90-day pruning via scheduler - Audit logging: append-only event trail for all admin mutations, filterable/paginated queries, automatic 365-day pruning, X-Forwarded-For aware IP extraction - require_permission() enforced on all 35+ admin endpoints (users, tokens, channels, schedules, watches, roles, orgs, policies, templates, usage, audit) - Field allowlists on storage update methods prevent mass-assignment bugs - Self-deletion guard on admin_delete_user, delete_user cascades user_roles - _row_to_dict helper eliminates ~400 lines of fragile positional row mapping - _audit_context helper deduplicates 18 instances of audit boilerplate - Migration 008: 7 new tables, 3 builtin roles, org_id on users - Console admin panel: 5 new tabs (Roles, Policies, Templates, Usage, Audit) with permission-gated visibility, 7 modal dialogs, full keyboard accessibility - Python + TypeScript SDK methods for all governance endpoints - 120+ new tests (1554 total) * fix: address PR #39 review feedback - Rebuild serialized items after policy evaluation so denied/allowed verdicts are reflected in tool_info/approve_request SSE payloads - Make `since` query param optional in usage OpenAPI spec (handler already defaults to last 7 days) - Add response_model=StatusResponse to DELETE role/policy/template and POST/DELETE role assignment endpoints in OpenAPI spec - Add missing org_id/created/updated fields to UserRoleInfo schema - Add missing created field to AuditEventInfo schema - Show "no permissions" empty state instead of loading inaccessible tab when all admin tabs are permission-gated - Fix "13 permissions" → "15 permissions" in architecture.md and security.md - Fix import sorting in test_audit.py and test_tool_policy.py * fix: address PR #39 round 2 review feedback - Clear stale permissions from sessionStorage on config-token login (auth.js _storePermissions) - Only trust X-Forwarded-For when behind a proxy that sets X-Forwarded-Proto (conditional on is_secure_request trust model) - Thread user_id from auth into WebUI.on_status for usage events - Add created field to TS AuditEventInfo type - Return typed Pydantic models from all SDK governance methods instead of dict[str, Any] — both async and sync clients - Validate group_by param against allowed enum in admin_usage handler - Add deterministic secondary sort (event_id DESC) to list_audit_events in both SQLite and PostgreSQL backends	2026-03-10 19:32:37 -07:00