ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-12 23:12:23 -06:00
Code Issues Packages Projects Releases Wiki Activity
1,154 Commits 12 Branches 135 Tags
a3ff07a86df7a4bd4f577eaa9cf54ff3179f50cd
Commit Graph

157 Commits

Author SHA1 Message Date
Patrick Buckley a3ff07a86d fix(tls): mTLS-aware container healthcheck + boot-time init retry 
A whole-stack restart races every node against the console for the CA
fetch (compose re-enforces depends_on ordering only on `up`): losers
logged one warning and served plain HTTP for their lifetime, while
winners served mTLS that the plain-HTTP container healthcheck could
never probe — leaving "healthy" plaintext nodes and "unhealthy"
working ones.

- TLSClient.init() grows attempts/base_delay retry (server passes 6
  attempts, ~31 s backoff) absorbing the boot race; per-attempt CA-fetch
  failures log warning + debug traceback instead of error tracebacks.
- healthcheck.py falls back to HTTPS when the plain probe fails,
  presenting the node's own cert as the client cert with the cluster CA
  pinned; dials localhost because the internal CA issues DNS SANs only.
  Default plain-HTTP deployments are unchanged.
- The server writes boot PEMs under a fixed root (TURNSTONE_TLS_PEM_DIR,
  default <tmpdir>/turnstone-tls) so the probe can find them; boot
  clears stale dirs and refuses a symlinked/foreign-owned root; renewal
  rewrites the PEM dir so the probe's client cert never outlives the
  served cert.
- /health reports tls: "active"|"fallback" (absent when TLS is
  disabled) so a silently downgraded node is observable.
 2026-06-09 18:37:46 -07:00
Patrick Buckley b3c1b9c9e0 build: promote anthropic, postgres, console, tls to core dependencies 
The Anthropic SDK provider was the lone first-class provider gated behind
an optional extra, while OpenAI ships in core and Google rides the
OpenAI-compatible path. Fold anthropic, psycopg (postgres), croniter
(console), and lacme (tls) into the base dependency set so a default
`pip install turnstone` yields a complete single- or multi-node
deployment; only the Discord/Slack channel gateways stay optional.

- pyproject: four extras → base deps; `all` is now discord+slack; drop the
  redundant croniter from the `test` extra; regenerate uv.lock.
- ci: the postgres test job installs `.[test]` (psycopg is base now).
- providers: `_ensure_anthropic` becomes a thin SDK accessor for
  `create_client`; drop the now-redundant eager import-guard calls from
  the streaming/completion hot path (anthropic is always present).
- bootstrap: import anthropic directly.
- tests/docs: drop the anthropic importorskips and stale extra-install hints.
 2026-06-04 11:03:13 -07:00
Patrick Buckley 8b4b8b3fd5 refactor(rerank): reranker is a per-model definition only (drop global endpoint settings) 
The reranker_alias -> model-definition path (added when reranking became a model
role) made the older global endpoint settings redundant. Resolve reranking
solely through the Reranker role and remove the parallel global config.

- Removed settings tools.rerank_url / rerank_model / rerank_api_key, their
  config.py getters (+ $TURNSTONE_RERANK_URL / $TURNSTONE_RERANK_MODEL and the
  module caches), and the fallback branch in resolve_rerank_client_from. The
  resolver now returns a client only when a Reranker model (capability
  supports_rerank, base_url = its /rerank endpoint) is selected, else None.
- Kept as global knobs: reranker_alias (the selector), rerank_web_search,
  rerank_bm25, rerank_bm25_threshold, and rerank_instruction -- a task-level
  query knob (Qwen3-style), not endpoint identity.
- The Settings tab is registry-driven, so the three fields disappear with their
  SettingDefs. Updated the Reranker role help, example config, and docs/tools.md.

BREAKING: a reranker configured via [tools] rerank_url (config.toml / env /
Settings tab) no longer works -- add the reranker in the admin Models tab and
pick it under Models -> Roles -> Reranker. No migration: reranking is days old
and disabled by default, so any orphaned tools.rerank_* config rows are inert.

Tests: the resolver covers no-store / no-alias / non-rerank-alias -> None and the
model-definition happy path; the obsolete global-fallback tests are removed.
 2026-06-01 21:34:12 -07:00
Patrick Buckley f6bae70ea6 feat(rerank): calibration CLI, 0-1 normalization, instruction support 
Phase 2 of BM25 reranking (follows #627). Makes the rerank_bm25_threshold floor
usable across reranker models and adds tooling to pick it.

- normalize_scores (rerank.py): map a rerank batch into a 0-1 relevance
  probability -- sigmoid when any score falls outside [0,1] (logit endpoints
  like bge/TEI), identity otherwise (Cohere/Jina/Qwen already 0-1). Applied in
  the _bm25_reranker closure AND calibration so the threshold means the same on
  every endpoint. Monotonic, so ranking order is unchanged.

- rerank_calibrate.py + `turnstone-admin rerank-calibrate [--apply]`: probe the
  endpoint with labelled relevant/irrelevant groups, normalise, and recommend a
  recall-biased floor -- or report "no clean separation" (a mis-served/weak
  reranker). A warmup loop absorbs a cold endpoint's first-request compile so
  calibration doesn't time out. Validated live against Qwen3-Reranker 0.6B and
  4B: the calibrated floor differs sharply per model (~0.95 vs ~0.33 for the
  same task) -- exactly why per-endpoint calibration exists.

- rerank_config.py: extract resolve_rerank_client_from(config_store, registry);
  the alias/url precedence now lives in one place, shared by ChatSession (which
  delegates) and the CLI.

- tools.rerank_instruction (config + setting + client): wrap the query as
  <Instruct>:/<Query>: for instruction-aware rerankers (Qwen3) on endpoints that
  don't apply the model's own chat template. Docs note the critical vLLM serving
  detail: Qwen3-Reranker needs --chat-template or its scores are near-random and
  reranking hurts retrieval.

Negative-tested: normalize sigmoid/identity branches, closure-normalises-before-
floor, calibration separation/recall-bias/warmup-absorbs-cold-start, the CLI
apply/no-apply/no-separation paths, and instruction query-wrapping through the
real httpx boundary.
 2026-06-01 14:44:30 -07:00
Patrick Buckley 215f7506ba feat(rerank): wire endpoint-backed reranking into BM25 retrieval surfaces 
Reuse the shipped Cohere/Jina rerank client as an optional post-process on
the BM25 surfaces (tool search, skill search, memory composition) via one
seam: BM25Index gains an injected reranker + a two-stage search (BM25 recall
top-50 -> rerank -> top-k). No new storage.

Gated on a configured endpoint plus tools.rerank_bm25 (default on, matching
rerank_web_search). tools.rerank_bm25_threshold (default 0.0 = off) is a
relevance FLOOR for proactive memory surfacing: BM25 always returns something,
so without a floor every-turn memory injection spends tokens on the top-k of
whatever lexically matched; the reranker score is what makes a meaningful
"inject nothing" gate possible.

Two reranker modes (BM25Index rerank_filters):
- REORDER (reactive tool/skill search): the reranker must never drop results
  -> fall back to BM25 order on empty, backfill omitted pool items, so a
  misbehaving endpoint can't silently lose tools.
- FILTER (memory, rerank_filters = threshold > 0): a clean empty/short result
  is honoured (inject nothing) -- a deliberate divergence from
  web_search._rerank_results.
Parse/endpoint failure is a discrete branch from the floor: an empty result
for non-empty input means an unparseable response (a conforming reranker
scores every doc), so the closure raises RerankError and BM25Index falls back
to BM25 order in BOTH modes -- the floor only acts on valid scores.

Also: cap the rerank client timeout at 15s (the per-turn memory path can't
afford tools.timeout's 120s default); move the Reranker alias to rerank.py
(shared, no import cycle); document the endpoint egress in the rerank_bm25
help, the admin Reranker-role description, and docs/tools.md; add
scripts/bench_bm25_rerank.py (manual, needs a live endpoint) to measure
precision@k/MRR lift and recommend a threshold default.

Negative-tested: reorder fallback-on-empty and omitted-item backfill,
filter-mode honor-empty, singleton-still-floored, the parse-fail RerankError
raise, the >= floor boundary, and pool-position-to-doc-index mapping -- each
guard reverted to confirm its test fails, then restored.
 2026-06-01 12:56:45 -07:00
Patrick Buckley 6a0bc852d9 feat(rerank): endpoint-backed reranking for web_search 
Reranking is delegated to an external Cohere/Jina-compatible /rerank endpoint
(self-hosted vLLM/TEI/llama.cpp, or hosted Cohere/Jina/Voyage); Turnstone runs
no reranker model itself. Disabled until an endpoint is configured.

- core/rerank.py: CohereJinaRerankClient (tolerant of results-wrapped and
  bare-list responses) + resolver.
- web_search: rerank the SearxNG result pool by query relevance before top-k,
  with a native-order fallback on error; answers/infoboxes untouched.
- Reranker as a model definition: add a model with the supports_rerank
  capability and pick it under Models -> Roles -> Reranker
  (tools.reranker_alias); takes precedence over the tools.rerank_url settings.

Settings: tools.rerank_url/model/api_key, tools.rerank_web_search,
tools.reranker_alias. Docs: docs/tools.md, turnstone.example.toml.

(web_fetch reranking was evaluated and dropped: for single-document chunk
selection it did not reliably beat head-truncation. Reranking is reserved for
multi-item ranking.)
 2026-06-01 11:01:30 -07:00
Patrick Buckley 110d44b07e refactor(tools): remove man, math, and plan_agent built-in tools 
`man` and `math` duplicated capabilities already reachable through
`bash`; `plan_agent` is better expressed as a `task_agent` running a
planning skill, and carried a large amount of special-case machinery
(plan-review gate, refinement loop, per-kind model routing). Removing
all three shrinks the tool surface and cuts per-call token cost.

Also removed, as dead-once-the-tools-are-gone:
- the `math` sandbox executor (`turnstone.core.sandbox`) and its
  `[sandbox]` extra; the eval analyst now runs bash-only
- the read-only `AGENT_TOOLS` sub-agent tool set and the `agent`
  tool-metadata key (`task_agent`/`TASK_AGENT_TOOLS` retained)
- the plan-review protocol end to end: the `on_plan_review` UI hook,
  `resolve_plan`, `POST /v1/api/plan` + `POST /v1/api/route/plan`,
  the `plan_review`/`plan_resolved` SSE events, and their Python SDK /
  TypeScript SDK / OpenAPI / frontend / Discord+Slack bindings
- the `model.plan_alias` / `model.plan_effort` settings and the
  registry `plan_model` / `plan_effort` routing fields

TOOLS 31->28, TASK_AGENT_TOOLS 13->11; COORDINATOR_TOOLS unchanged.

BREAKING CHANGE: removes the `man`, `math`, `plan_agent` tools, the
plan-review SSE/HTTP/SDK surface, and the plan_* model-routing settings
from the experimental 1.6 line.
 2026-05-31 19:54:43 -07:00
Patrick Buckley 15b3aad815 feat(web-search): let the model pick a SearxNG category 
Rename the web_search tool's `topic` parameter to `category` and expand the
enum to general/news/it/science, mapped to SearxNG `categories=`. The model can
now target the right corpus per query (e.g. `it` for code, `science` for
papers) — useful when generic engines rate-limit. The Tavily-era `finance`
topic (no SearxNG equivalent) is dropped. Threaded consistently through
_prepare_web_search / _exec_web_search / both search clients.

BREAKING: the web_search `topic` argument is now `category`.
 2026-05-31 19:03:16 -07:00
Patrick Buckley 1728a4c0af feat(web-search): replace Tavily/DuckDuckGo backends with self-hosted SearxNG 
Drop the Tavily and DuckDuckGo (ddgs) web_search backends for a single
self-hosted SearxNG service bundled into the docker-compose stacks.

Core:
- New SearXNGClient + _format_searxng; rewrite resolve_web_search_client to
  (backend, searxng_url, searxng_engines, ...). MCP backend + oauth_user guard
  unchanged. _resolve_search_client follows storage -> toml -> env -> default
  precedence (explicit "" disables, via ConfigStore.stored_keys()).
- Drop the Tavily-era topic=finance (no SearxNG category); topic is now
  general/news.

Settings/config:
- Remove tools.tavily_api_key, get_tavily_key, $TAVILY_API_KEY, [api].tavily_key.
- Add tools.searxng_url (default http://searxng:8080) + tools.searxng_engines,
  with get_searxng_url/get_searxng_engines.

Compose + bundled config:
- Internal-only searxng service (no published API port, :ro config, /healthz
  healthcheck, persistent searxng-cache volume) in both stacks; bundle
  turnstone/deploy/searxng/settings.yml (JSON output on, limiter off).
- Caddy serves the SearxNG web UI on :8444 (dev: localhost-only; prod: opt-in).
- bootstrap extractor + wheel packaging updated.

Deps: drop the ddg extra + ddgs mypy override (regenerates uv.lock, removing the
lxml/h2/brotli transitives).

Docs: tools/docker/architecture/openshell + diagrams + config example + CHANGELOG;
docs/docker.md carries the AGPL-3.0 §13 operator note.

BREAKING: tools.web_search_backend no longer accepts "tavily"/"ddg";
tools.tavily_api_key and the ddg extra are removed. Run the bundled SearxNG (ships
in the compose stacks) or set TURNSTONE_SEARXNG_URL to an external instance.

Closes #545
 2026-05-31 19:03:16 -07:00
Patrick Buckley 631f1b0021 feat(compose): cluster-by-default Caddy-fronted stack with bare-metal join 
`docker compose up` from a clone builds one image and brings up the whole stack
— PostgreSQL, console, Caddy, channel, and 10 server nodes — sharing one
Postgres so the console discovers every node. The dashboard is reachable only
through Caddy (HTTP/2 avoids the browser's 6-connection cap on the dashboard's
SSE streams); the console's plain-HTTP port is no longer published. Postgres
binds 127.0.0.1 so a bare-metal turnstone-server can join the cluster — the
bare-metal overlay is folded in and removed. Insecure dev defaults keep it
zero-config; the bundled production stack mirrors the shape but pulls ghcr
images and requires real secrets.

Move the Caddyfile under turnstone/deploy so it ships in the wheel; update docs,
QUICKSTART, and .env.example to match.
 2026-05-31 16:05:21 -07:00
Patrick Buckley da07554693 docs(judge): correct Smart Approvals heuristic-floor wording 
The floor blocks only explicit heuristic deny/critical verdicts — it is
not a general "never lower the heuristic" rule. The heuristic default for
an unmatched tool is `review`, and letting a confident LLM `approve`
upgrade a `review` is the feature's purpose. Matches the implementation
and addresses PR review feedback.
 2026-05-30 19:48:17 -07:00
Patrick Buckley 948e413f66 feat(judge): add Smart Approvals (auto-approve trusted judge verdicts) 
Opt-in judge.smart_approvals (default off): when the intent-validation
LLM judge returns a high-confidence "approve" verdict, the tool batch is
approved automatically with no operator prompt. review/deny recommendations,
low confidence, judge errors (llm_fallback), and a deterministic heuristic
deny/critical finding all still require a human. Requires judge.enabled.

- Batch-atomic: a parallel tool batch auto-approves only if every call
  qualifies; one non-qualifying call holds the whole batch for a human.
- Gate: tier==llm + recommendation==approve + confidence >=
  judge.confidence_threshold (default raised 0.7 -> 0.95), with a floor
  that never clears an explicit heuristic deny/critical verdict.
- approve_tools waits for the async LLM verdicts, finalises the audit
  trail (AutoApproveReason.smart_approval), and re-emits verdicts after
  the card so the live chip updates; the auto-approved row renders the
  LLM verdict rather than the cautious heuristic carry-over.
- judge: always deliver exactly one verdict per call (fallback on error);
  reject non-finite confidence so NaN can't clear the bar.
- Drop verdicts from a superseded judge generation so a reused call_id
  from a prior turn's still-running daemon can't satisfy the gate's wait.

Config plumbed through the server/console/CLI builders and the live
_judge_cfg; admin Judge tab renders the toggle. Docs + example config
updated. ~35 tests covering the gate matrix, batch-atomicity, the
heuristic floor, audit stamping, the streaming re-emit, NaN/duplicate-id
defenses, and the cross-turn generation guard.
 2026-05-30 19:48:17 -07:00
Patrick Buckley d820168f3f fix(tls): repair cluster mTLS — cert identity, renewal scoping, hot-reload 
Enabling mTLS broke the cluster in three layered ways:

- Service certs were keyed on socket.gethostname() (the container ID) and
  never carried the advertised service name as a SAN, so every collector and
  routing-proxy handshake failed the hostname check. build_cert_hostnames()
  now puts the advertised host first: it becomes the cert's primary domain
  (hence a SAN) and a stable store key that survives container recreation.

- lacme's RenewalManager renews everything in the store; with the store shared
  cluster-wide, every node renewed every other node's (and every dead
  container's) cert — an N×M renewal storm. _SingleDomainStore scopes each
  node's sweep to its own cert, and the console adds a periodic GC for the
  certs of long-departed nodes.

- uvicorn loads its cert once at boot and never reloads, so renewed certs
  never reached the listener and the served cert expired mid-process.
  swap_context_cert() hot-swaps renewed material into the live SSL context
  (server listener and console client context) via load_cert_chain.

Observability and browser access:

- The collector logged connection/TLS failures at DEBUG, so a persistent
  mTLS-verify failure was invisible. It now logs the first failure per node
  (reachable->unreachable) at WARNING and stays at DEBUG on retries.

- The console serves plain HTTP (it is the ACME bootstrap endpoint) and no
  longer rewrites its advertised URL to https://. Browser->console TLS is
  terminated by a reverse proxy: the cluster profile gains a caddy service
  (browser h2/HTTPS -> caddy -> console h1.1/HTTP) plus browser-TLS docs.

Tests: tests/test_tls_san_renewal.py, tests/test_collector_reachability.py.
 2026-05-30 16:27:15 -07:00
Patrick Buckley 30d670338e feat(judge): merge output-guard heuristic + LLM judge, annotate findings 
Surface the output-guard LLM judge on the inline finding chip and merge it
with the regex heuristic instead of one stage winning outright.

Merge rule (issue #560, "show, annotated"):
- risk_level = max(heuristic, llm); flags = union. The judge can escalate
  but never lower a heuristic positive — it evaluates adversarial tool
  output, so defeating it must not erase a deterministic regex finding.
  Credentials stay heuristic-only and are always redacted.
- The judge's own verdict rides along as a dissent-aware annotation
  (judge_risk / confidence / reasoning / judge_model) on the chip in both
  the interactive and coordinator UIs, live and on reconnect. One shared
  merge_guard_display_payload drives both paths so they cannot drift.
- The model is shown the merged risk + flags but never the judge's "benign"
  verdict — a fooled judge must not talk the model out of caution.

Fixes a reconnect bug: a judge that ran but failed wrote a risk="none" row
that won the replay dedup and hid the heuristic finding (it showed live but
vanished on refresh). Failed judges now persist under tier="llm_error",
excluded from the display merge; the max-merge also floors the displayed
risk at the heuristic level so the chip never vanishes.

Also adds a regression test confirming the LLM judge runs on every tool
output, not just heuristic-flagged ones.

Tests: merge unit tests, storage-backed replay regression, live/replay
wire-shape parity, SDK-event drift guard. ruff + mypy clean.
 2026-05-29 11:17:12 -07:00
Patrick Buckley ee8dc7c1c3 refactor(history): project the /history wire shape server-side 
Collapse the three hand-synced "raw storage -> render shape" projections
into one server-side projection. The projection previously lived in a
test-only `_build_history` (SSE-era reference impl), a client-side JS
normaliser (`history_normalize.js`, the transitional bridge), and coord's
inline `init()` handling -- drifting silently with no parity test.

Add `project_history_messages` to `history_decoration.py` and run it as the
final step of the `make_history_handler` pipeline (load_messages -> decorate
-> extract_reasoning -> project), so `GET /history` emits the canonical
render shape directly: flat tool_calls (with verdict / output_assessment),
top-level source / reminders / attachments, collapsed multipart content,
derived denied / is_error / pending, reasoning, and advisories. Interactive
`replayHistory` now consumes the payload verbatim.

Close two gaps the JS bridge deferred:
- list-content <tool_output> advisory extraction (decorate handles only
  string content; the projection extracts list-carrier advisories, then
  joins remaining text parts to the string the renderers require);
- orphan->pending marks ONLY the last orphan tool-call turn, so a
  mid-conversation cancelled tool still renders instead of vanishing.

Delete `history_normalize.js` (+ its <script> tag and node test) and the
test-only `_build_history` (+ orphaned imports); retarget its direct tests
onto the projection helpers. Update the WorkstreamHistoryResponse
description and the Web UI Resilience architecture note to the projected
shape.

Coord's `init()` still reads the raw side-channels; migrating it to the
projected shape is the next commit, browser-verified separately.

Refs #549.
 2026-05-28 17:14:50 -07:00
Patrick Buckley b02e4312ac feat(tools): make notify dual-kind, expose to coordinator sessions (#559) 
notify was interactive-only — a coord with a natural "fan-out complete"
or "batch failed" beat could only post by spawning a child for the
single message, which is a lot of ceremony.  Routing is session-kind-
agnostic in _prepare_notify / _exec_notify; this is a metadata flip
that adds the coord flag (plus the explicit interactive flag the loader
needs once coordinator is set) and updates the dual-kind whitelists,
coord tool-set assertions, and skill-author docs accordingly.  Adds
two coord-session tests pinning the prepare dispatch contract
(needs_approval=False matches notify.json auto_approve) and the exec
→ channel-gateway path.
 2026-05-26 17:07:10 -07:00
github-actions[bot] 7404ae46db chore: download vendored JS files 2026-05-23 11:33:36 -07:00
Patrick Buckley 83a602fef7 fix(skills): address /review on flatten — kind validation + stale text 
Four /review findings collapsed to one code chokepoint + two
documentation fixes:

1. find's `kind` arg now validated against ``SkillKind`` (matching
   create / update's existing pattern at session.py:8298 / :8512).
   Closes two failure modes that shared the same root:
   - typos (`kind="interactivee"`) silently produced
     `kinds=["interactivee", "any"]` filtering to literal-`any` rows
     only and masquerading as a narrowed catalog — now returns an
     explicit "kind must be one of: ..." error;
   - the documented enum value `kind="any"` degenerated to
     `kinds=["any", "any"]` which narrowed to literal-`any` rows
     instead of returning "every kind" — now collapses to ``None``
     so the documented semantic holds.

2. docs/coordinator-skills.md "two-surface model" section rewritten
   to reflect the post-flatten reality: kind is metadata, not an
   enforcement boundary. The line-67 tools-table row updated from
   the long-dead `list_skills` to `skills (action=find)` with the
   opt-in kind-filter framing.

3. Three stale "interactive-only" comments in session.py
   (:5514, :7857, :8210) that directly contradicted the
   `_prepare_skills_load` docstring ("Both kinds can load") — drop
   the qualifier so future grep-and-encode hazards don't reintroduce
   the rejection.

Tests:
- test_find_kind_invalid_errors — typo case (replaces the silent
  degenerate to literal-any-only)
- test_find_kind_any_means_no_filter — documented enum value matches
  documented semantic (collapses to None at prepare)
- test_find_kind_narrow_passes_through — valid narrowing values
  reach exec as expected

Deferred to release notes (no code change, intentional policy shift):
- skills(action='get') / load can now read full content + scan_report +
  allowed_tools on cross-kind rows from any session. Operators with
  pre-existing kind=coordinator skills authored under the prior
  implicit visibility contract should audit those bodies for
  sensitive content (allowed_tools allowlists, embedded credentials,
  internal hostnames in examples) before upgrade.
 2026-05-22 17:57:26 -07:00
Patrick Buckley c4495c0c48 fix(coord): address PR review threads on spawn_workstream rename 
Three Copilot threads from PR #526:

1. ``_exec_spawn_workstream`` success path emitted
   ``{"child_ws_id": null}`` when the upstream response unexpectedly
   omitted ``ws_id`` (200-shape with no error field, no id field).
   Adds the missing guard — mirrors ``_exec_spawn_batch`` which
   already surfaces ``"spawn returned no ws_id"`` as a denied row.
   The LLM now sees a tool error and can retry instead of chasing
   a null id through follow-up tools.

2. ``docs/coordinator-skills.md`` UI render note said "keep the
   ws_id as the click-through key" in a paragraph that had just
   introduced ``child_ws_id`` — readable as "the ws_id value" but
   confusable as a field-name claim.  Clarifies that the value
   class is the same regardless of which key carried it.

3. ``docs/bulk-endpoints.md`` ``spawn_batch`` example shows
   ``child_ws_id`` (coord-tool output shape).  The doc title and
   the "model tool" column label already disambiguate it from HTTP
   API responses, but a reader landing at the example section
   directly could miss the framing.  Adds one explicit sentence.
 2026-05-18 19:35:10 -07:00
Patrick Buckley 6948ea21cb fix(coord): rename ws_id->child_ws_id in spawn return JSON 
Coordinator LLMs on large fan-outs recency-bias on seeing `ws_id`
in a `spawn_workstream` / `spawn_batch` return -- calling
`spawn_workstream(ws_id=...)` again instead of progressing to
`wait_for_workstream(ws_ids=[...])`. On 10+ child fan-outs this
cascades into self-inflicted re-spawn loops.

Rename to `child_ws_id` (already an existing project term -- see
`tasks` tool, `child_event_bus.py`) defuses the recency bias.
Scope is the LLM-facing JSON only -- the server HTTP API at the
spawn endpoint still returns `ws_id`, and the internal reads of
that HTTP response are unchanged.

Also updates the two tool descriptions, the operator-facing skill
doc, and the bulk-endpoints example so docs don't undo the rename.
 2026-05-18 19:35:10 -07:00
github-actions[bot] 6998b442a9 chore: download vendored JS files 2026-05-17 06:35:24 -07:00
github-actions[bot] f28a3533a2 chore: download vendored JS files 2026-05-13 15:47:12 -07:00
Patrick Buckley adeb10bc2c feat(mcp): admin status, deferred-consent persistence, operator docs (Phase 9) (#516) 
* feat(mcp): admin status, deferred-consent persistence, operator docs (Phase 9)

Completes the OAuth-MCP build-out (Phases 0-8 shipped) by closing the
operator + deferred-consent gaps:

1. **Per-(user, server) deferred-consent persistence** — when a
   non-interactive run (scheduled / channel) hits ``mcp_consent_required``
   or ``mcp_insufficient_scope``, the sync pool dispatchers now upsert a
   row into a new ``mcp_pending_consent`` table.  The dashboard hydrates
   the gear-icon badge from this table on load, so users who weren't
   online to see the in-flight SSE prompt still surface the deferred
   work on next login.  Cleared automatically by the OAuth callback
   handler on consent completion; manual user dismiss via new DELETE
   endpoints.  Composite PK ``(user_id, server_name)`` collapses repeat
   occurrences for the same server — no NULLs-not-distinct trap.

2. **Admin status pill + bulk-revoke** — the MCP Servers admin row now
   shows ``consented_users_count`` for ``auth_type=oauth_user`` rows
   when ≥1, with a two-step-confirm ``bulk-revoke`` button that drops
   every user's token for the server via the existing
   ``delete_mcp_oauth_rows_by_server_name`` primitive.  Upstream RFC
   7009 revoke is intentionally NOT attempted in bulk (avoids N
   upstream HTTP calls per admin click); audit detail records
   ``upstream_revoke_outcome=bulk_admin_no_upstream``.  A "last
   refresh" pill (age + outcome) renders on each row, sourced from a
   new ``_last_refresh`` dict populated by ``_refresh_server`` on every
   call (both manual ``refresh_sync`` and the ``_cb_auto_reconnect``
   follow-up).

3. **ClientType.SCHEDULED** added to the prompts module + scheduler
   passes it through to ``create_workstream``.  ``ChatSession`` now
   computes ``_is_interactive_for_consent`` at construction (WEB / CLI
   are interactive; CHAT / SCHEDULED are not) and plumbs the flag
   through ``call_tool_sync`` / ``read_resource_sync`` /
   ``get_prompt_sync`` to the three sync dispatchers.  The wrap at the
   ``_is_structured_error`` gate routes consent codes to the new
   ``_record_pending_consent_best_effort`` helper for non-interactive
   callers only; interactive sessions stay on the in-flight SSE path
   Phase 8 ships unchanged.

4. **Operator docs** — ``docs/mcp-oauth.md`` (operator guide, parallel
   to ``docs/oidc.md``: ``auth_type`` choice, OAuth client setup,
   encryption-key rotation, troubleshooting matrix) and
   ``docs/operations/mcp-oauth-headless.md`` (one-paragraph runbook
   per ``feedback_runbook_trust_llm.md``: pre-consent recipe for
   scheduled / channel-driven runs).

Schema
- Migration 054_mcp_pending_consent.py — composite PK
  ``(user_id, server_name)``, ``occurrence_count`` + ``first_seen_at`` /
  ``last_seen_at`` for recency metadata, ``idx_mcp_pending_consent_user``
  for the badge-load query.  No FKs (matches the rest of the
  oauth_user schema).
- Migration 055_mcp_user_tokens_server_index.py — adds
  ``idx_mcp_user_tokens_server`` on ``(server_name, expires_at)`` so
  the admin pill's ``count_mcp_consented_users_*`` queries don't
  full-scan against the leading-``user_id`` composite PK.
- Cross-backend: works on SQLite + PostgreSQL via dialect-specific
  ``on_conflict_do_update`` (PG ``postgresql.insert`` / SQLite
  ``sqlalchemy.dialects.sqlite.insert``).  No ``NULLS NOT DISTINCT``
  needed — the simplified PK eliminates the cross-version trap.

Endpoints
- ``GET /v1/api/mcp/oauth/pending`` — list deferred-consent records for
  the authenticated user.  Install-level gate via cached
  ``any_oauth_user_mcp_servers`` short-circuits to ``{pending: 0}`` on
  installs with no oauth_user MCP servers — local-auth deployments
  exercise zero new storage queries on this path.  The gate result is
  cached on ``app.state`` with a 60s TTL to spare repeat dashboard
  loads.
- ``DELETE /v1/api/mcp/oauth/pending/{server_name}`` — single dismiss.
  Returns 204 in both existed-and-deleted and never-existed cases
  (no cross-tenant existence leak); audits
  ``mcp_server.oauth.pending_consent_dismissed`` with
  ``mode=single`` + ``cleared=0|1`` so a session-hijack attacker
  scrubbing breadcrumbs leaves an audit trail.
- ``DELETE /v1/api/mcp/oauth/pending`` — bulk dismiss; audits
  ``mode=bulk`` + ``cleared=N``.
- ``POST /v1/api/admin/mcp-servers/{name}/bulk-revoke`` — admin
  bulk-revoke for the named server's per-user tokens.  Requires
  ``admin.mcp`` permission + 400s when the row isn't ``oauth_user``.

All four registered on both ``turnstone-server`` and
``turnstone-console`` (mirrors the Phase 8 ``/connections`` endpoint
shape).

Performance
- Admin list handler now uses a single ``GROUP BY`` bulk-count query
  (``count_mcp_consented_users_grouped_by_server``) wrapped in
  ``asyncio.to_thread`` rather than N per-row sync DB round-trips
  inside the async handler.  Skipped entirely when no row is
  oauth_user.

Frontend
- ``ui/static/app.js``: ``loadPendingConsents()`` hydrates the
  existing ``_pendingConsentServers`` set on dashboard init + after
  the user opens the settings modal.  Endpoint failures stay silent
  — the badge will be re-driven by the next in-flight tool error.
- ``console/static/admin.js``: ``consented_users_count`` pill +
  ``bulk-revoke`` button on each MCP row (only when ≥1 consented),
  two-step confirm matching the existing delete pattern.  ``last-
  refresh`` age + outcome pill in the per-row status cell, sourced
  from the freshest per-node entry in ``status[*].last_refresh_at`` /
  ``last_refresh_outcome``.  CSS for the pills in ``style.css``.

Tests
- ``test_mcp_pending_consent_storage`` — 13 tests covering upsert
  idempotency, list ordering, per-user isolation, single/bulk delete,
  count-by-server + grouped variant, install-level gate.
- ``test_mcp_pending_consent_dispatch`` — 9 tests, including the
  boundary-cross gate per ``feedback_tests_through_boundaries.md``:
  drives the real ``call_tool_sync`` → ``_dispatch_pool_sync`` →
  ``_is_structured_error`` → ``_record_pending_consent_best_effort``
  with a mocked classified-lookup so the structural plumb-through is
  verified end-to-end.  Includes a storage-failure test that pins
  the docstring's "envelope unchanged on storage failure" promise.
- ``test_mcp_pending_consent_endpoints`` — 11 tests: install gate,
  list-for-self, no-cross-user-leak, single/bulk delete, idempotent
  not-found, audit emission on single + bulk + cross-tenant dismiss.
- ``test_chat_session_interactivity_flag`` — 7 tests pinning the
  ``ClientType`` → ``_is_interactive_for_consent`` mapping against
  the module-level ``INTERACTIVE_CONSENT_CLIENT_TYPES`` frozenset.
- ``test_mcp_admin_bulk_revoke`` — 7 tests covering admin.mcp
  permission gate, 404 on missing, 400 on non-oauth_user, 200 with
  ``rows_deleted`` + ``consented_users_before``, audit row with
  ``upstream_revoke_outcome=bulk_admin_no_upstream``, cross-server
  isolation.
- ``test_mcp_oauth_handlers`` — 2 new callback tests pin the post-
  callback ``delete_mcp_pending_consent`` invocation: success-clears
  + storage-failure-still-redirects.
- 636 tests pass on the impacted surface (47 new + Phase 0-8 OAuth-MCP
  + session + prompts + storage admin).  ruff + mypy clean.

Hard invariants honored
- Static path byte-identical for ``auth_type ∈ {none, static}`` — the
  flag flows only through the pool dispatchers, which only fire when
  the row resolves to ``oauth_user``.
- ``asyncio.timeout`` (not ``asyncio.wait_for``) preserved on every
  AS / SDK / pool-loop await — no new awaits added to the hot path.
- Install-level gate on the badge endpoint: cached
  ``any_oauth_user_mcp_servers`` returns False on a row-less
  deployment → endpoint short-circuits without touching the pending-
  consent table; 60s TTL bounds the staleness window after admin
  flips ``auth_type``.
- Operator-actionable codes (key-unknown, url-insecure, *_forbidden)
  explicitly filtered out of persistence — they're outside the
  user-facing consent badge scope.
- Best-effort write: the structured-error envelope returned to the
  agent is identical whether the persistence write succeeds or fails
  (storage exception is logged with type name only — no chained
  context that could carry an ``httpx.Request`` bearer header).
- No ``exc_info=True`` on any new path that can chain a bearer-bearing
  ``httpx.Request``.
- Defensive parsing: ``_parse_pending_consent_envelope`` mirrors
  ``_is_structured_error``'s ``isinstance(decoded, dict)`` guard plus
  filters scope tokens through ``is_valid_scope_token`` capped at
  ``MAX_INSUFFICIENT_SCOPE_REPORTED`` — defense-in-depth even though
  production callers already validate upstream.
- Audit events on every dismiss endpoint so a session-control attacker
  scrubbing dashboard breadcrumbs still leaves a trail.

Cross-backend
- Tested on SQLite via the conftest backend fixture.
- PostgreSQL path uses ``postgresql.insert(...).on_conflict_do_update``
  parallel to the existing ``mcp_user_tokens`` upsert in Phase 3.

Deferred (not Phase 9 blockers)
- Multi-node pool eviction on bulk-revoke: only local-node sessions
  would be evicted if we built it, and there's no bulk-by-server
  primitive on MCPClientManager today; remote nodes will surface as
  a 401 on next dispatch which refreshes through the (now empty)
  token row.
- RFC 8693 / Azure OBO ``auth_type=oauth_token_exchange`` — captured
  in the design doc as a future architectural direction (~600 LOC +
  IdP-side admin work); requires OIDC token capture and per-MCP-server
  resource-trust configuration that v1 does not ship.

* docs(mcp): address Copilot review feedback on Phase 9

- Fix misleading admin.js comment that claimed the refresh pill rendered
  "<short-relative> <outcome>" — the pill actually renders only the short
  age, with outcome reflected via CSS class and tooltip.
- Replace broken feedback_secrets_not_in_env.md repo-root link in
  mcp-oauth.md with the inlined rationale (env-borne secrets reachable
  via shell tools / os.environ; TOML secrets are not).
 2026-05-12 13:15:09 -07:00
Patrick Buckley 752fea0fdd feat(console): reactive node discovery via PG LISTEN/NOTIFY dispatcher (#505) 
* feat(console): reactive node discovery via PG LISTEN/NOTIFY dispatcher

Add a console-side `NotifyDispatcher` that holds a dedicated PostgreSQL
`LISTEN` connection and fans wake-ups out to per-channel handlers on a
separate dispatch thread. Cluster collector subscribes to a new
`services` channel and runs node discovery reactively — new-node /
graceful-deregister visibility drops from up-to-60 s to ~500 ms on
Postgres, with the 60 s discovery loop retained as the backstop for
crash-shaped node loss (NOTIFY only fires on real writes).

Storage layer gains a uniform `notify` / `listen` API:
- PostgreSQL: real `pg_notify` / `LISTEN` on a dedicated session-mode
  connection that bypasses pgbouncer (mandatory: pgbouncer is required
  in transaction-pool mode per docs, which is incompatible with LISTEN).
- SQLite: in-process fan-out + synthetic-sweep fallback so consumer
  code is identical across backends.

`TURNSTONE_DB_LISTEN_URL` (or `[database] listen_url` in config.toml)
points the dispatcher's connection direct-to-Postgres. Defaults to the
main DB URL when unset.

Migration 053 installs the `services_notify` trigger; it filters
heartbeat-only UPDATEs in-trigger so the 30 s × N-nodes heartbeat tick
stays quiet, while INSERT, DELETE, and url/metadata-changing UPDATE
still fire.

Dispatcher detail:
- Two threads: listener (drains stream → bounded queue) and dispatch
  (invokes handlers under exception suppression). Same-channel notifies
  coalesce per dispatch batch so an N-node deploy burst is one
  `_discover_nodes` per channel.
- Reconnect uses exponential backoff (1 s → 30 s cap). After any
  successful reopen — whether the prior failure was a stream-poll error
  or a connect / initial-LISTEN error — one synthetic Notify with
  payload="reconcile" is enqueued per channel so handlers re-read on
  the same code path they use for real events.

Future consumers (ConfigStore live reload, scheduler immediate
dispatch, audit live-tail) plug in by adding their channel to the
dispatcher's construction list.

Tests: 22 dispatcher tests (incl. reconnect + coalescing under stub
storage), 7 SQLite notify-stream tests, 4 PG-gated trigger-filter
tests, 4 collector wire-in tests. All pass; ruff + mypy clean.

* fix(notify): address Copilot review on #505

- _sqlite.py: SQLiteBackend.listen() now de-dupes channel names via
  dict.fromkeys before constructing the stream — duplicates would
  otherwise register the queue twice and double-deliver each notify.
- _sqlite.py: SQLiteBackend.listen() gains a keyword-only sweep_interval
  parameter (defaults to _SQLITE_NOTIFY_SWEEP_INTERVAL) — matches what
  the comment at the constant already promised, and lets future
  consumers without their own polling timer pick a tighter cadence
  without reaching into private stream attributes.
- _sqlite.py: documented the `except queue.Empty: pass` end-of-drain
  termination so it's not mistaken for swallowing an unexpected error.
- _postgresql.py: docstring referenced :func:`_pg_listen_url` which
  was renamed to _resolve_pg_listen_url during PR development.
- notify_dispatcher.py: module docstring referenced a non-existent
  _bootstrap_console_subsystem; wire-in is at console/server.py::main.

Refuted (no change, false positives from github-code-quality bot):
- 4× "Statement has no effect" on Protocol-method `...` ellipsis bodies
  (idiomatic Python Protocol declaration, not dead code).
- 2× "Mixed import style" in tests — `import ... as nd_mod` is
  intentional to allow attribute assignment for monkey-patching the
  module's `_RECONNECT_BACKOFF_INITIAL` constant inside try/finally.
 2026-05-11 00:51:19 -07:00
Patrick Buckley 6f8574eef3 fix(reasoning): synthesize reasoning_text alongside non-reasoning provider_blocks 
GoogleProvider attaches raw tool_call dicts as ``provider_blocks`` on
the finish chunk for ``thought_signature`` round-trip
(``_google.py:_iter_stream``).  When the same turn streamed Gemini's
``reasoning_content`` as ``reasoning_delta`` chunks, the prior
synthesizer bailed out the moment ``provider_blocks`` was non-empty
— so the captured reasoning was visible live but lost on page reload.

Replace the early-return-if-non-empty check with a reasoning-bearing
type test (``thinking`` / ``redacted_thinking`` / ``reasoning`` /
``reasoning_text``).  When none of those types appear, append the
synthetic ``reasoning_text`` block to the existing list rather than
replacing it — preserving Google's tool-call fidelity blocks.

Also addresses two doc-accuracy review findings:
- ``LLMProvider.extract_reasoning_text`` docstring no longer claims
  OpenAI Chat / Responses are unwired (Phase 3+4 shipped extractors).
- Add the method to the Protocol methods table in
  ``docs/architecture.md`` (was missing alongside the class diagram).
 2026-05-09 02:45:13 -07:00
Patrick Buckley 20e1e7b110 fix(reasoning): apply Copilot review feedback + docs sync 
PR #498 round-robin review surfaced 5 findings.  4 applied; 1 rejected
with rationale.

Applied

* **Copilot finding 5** (history_decoration.py:341): dispatcher
  inspected only ``provider_content[0]['type']``.  OpenAI Responses
  captures EVERY ``output_item.done`` event into ``provider_blocks``
  (not just reasoning) — in practice the order is
  ``[reasoning, message, ...]`` but the API doesn't guarantee that;
  a hypothetical ``[message, reasoning]`` ordering would silently
  drop the reasoning under an index-only check.  Now walks the list
  for the first block whose type is in ``_BLOCK_TYPE_PROVIDER_FACTORY``,
  then dispatches the WHOLE list to that provider's extractor.  Each
  provider's extractor already filters internally by its own block
  type, so passing the full list is correct.  Regression test added
  (``test_dispatcher_scans_past_unrecognized_first_blocks``).

* **Copilot finding 3** (migration 052 docstring): the previous
  review-fix wave used sed to rename ``persist_reasoning`` →
  ``surface_persisted_reasoning`` everywhere, which mangled a
  historical reference in the migration docstring ("The earlier name
  ``surface_persisted_reasoning`` was renamed...").  Restored to
  point at the actual pre-rename name (``persist_reasoning``).

* **Copilot finding 4** (sdk/typescript/src/events.ts:26):
  ``HistoryEvent`` JSDoc still referenced ``persist_reasoning`` —
  the sed rename only walked ``turnstone/`` and ``tests/``, missing
  the TypeScript SDK.  Updated to ``surface_persisted_reasoning``.
  Also widened the comment to cover all three reasoning-bearing
  block types (Anthropic ``thinking``, OpenAI Responses ``reasoning``,
  synthetic ``reasoning_text``) instead of mentioning only Anthropic.

* **github-code-quality finding** (session.py:1120): ``_resolve_server_type``
  had a bare ``except Exception: pass``.  Replaced with a
  ``log.debug(..., exc_info=True)`` + explanatory comment.  Behaviour
  unchanged (still returns ``""`` on any lookup failure); failures
  are now observable under DEBUG triage.

Rejected (with rationale)

* **github-code-quality finding** (_protocol.py:265):
  ``extract_reasoning_text``'s body is ``...`` per ``LLMProvider``
  Protocol convention.  Every method in the file uses ``...`` (PEP
  544 idiomatic Protocol style).  Changing only this one to
  ``raise NotImplementedError`` would be inconsistent with the rest
  of the file.  CodeQL's "statement has no effect" warning is
  technically correct for ``...`` as a standalone expression but
  ignores the documented Python Protocol convention.  No fix.

Docs sync

* docs/api-reference.md: ``history`` SSE event message-shape table
  gains the optional ``reasoning`` field.
* docs/architecture.md: ``ModelCapabilities`` row in the type table
  gains ``supports_reasoning_replay``; ``StreamChunk`` and
  ``CompletionResult`` rows gain the existing ``provider_blocks``
  field (was missing pre-PR).  New "Per-model reasoning persistence"
  subsection under the Models config section, documenting the two
  flags + capability gate + three reasoning paths + cross-provider
  shape filter.
* docs/settings.md: new "Reasoning persistence (per-model)"
  subsection with the two-flag table and capability-gate note.
* docs/diagrams/03-core-engine-classes.puml: ``LLMProvider`` interface
  adds ``extract_reasoning_text`` + the new ``replay_reasoning_to_model``
  kwarg; ``ModelCapabilities`` class adds ``supports_reasoning_replay``.
  PNG regenerated.

Lint + test gate

* ruff check + ruff format clean.
* mypy clean (191 source files).
* pytest -m 'not live' — 6116 passed (3 deselected), +1 net new test
  (``test_dispatcher_scans_past_unrecognized_first_blocks``).
 2026-05-09 02:45:13 -07:00
Patrick Buckley 57cb09c871 docs(sse): document state_change + in_progress_snapshot events 
Updates the docs that describe the per-workstream SSE event stream and
the SessionUI lifecycle to match the refresh-resume changes:

- api-reference.md: documented the `state_change` event (previously
  undocumented despite already being a live event) and the new
  `in_progress_snapshot` event; rewrote the multi-consumer fan-out
  paragraph to mention the kind-specific replay tail (state_change +
  optional in_progress_snapshot) so the "no catch-up needed" claim
  is no longer misleading.
- architecture.md: bumped the SessionUI Protocol stub to 16 methods
  (added `on_turn_start` / `on_turn_committed`) and pointed at the
  in_progress_snapshot section in the API reference.
- sdk.md: added rows for `state_change`, `in_progress_snapshot`, and
  `approval_resolved` (preexisting gap) to the per-workstream event
  table.
- coordinator-api-tour.md: added an `in_progress_snapshot` row to the
  event table and rewrote the reconnection-contract paragraph to
  cover mid-stream content/reasoning restoration.
- diagrams/04-conversation-turn.puml: added `on_turn_start()` before
  the thinking-start emit and `on_turn_committed()` immediately after
  `messages.append(assistant_msg)`, with notes explaining the inflight-
  buffer reset semantics. PNG regenerated.
 2026-05-08 18:23:38 -07:00
Patrick Buckley eb2a119da9 refactor(mcp): remove periodic refresh, add manual refresh/reconnect controls 
Deletes the _periodic_refresh task and its supporting state
(_refresh_task, _refresh_failures, _refresh_backoff_until,
_REFRESH_BACKOFF_BASE/MAX, _DEFAULT_REFRESH_INTERVAL, refresh_interval
kwarg) from MCPClientManager. Push notifications and operator-driven
manual refresh now cover all catalog-update needs; the long-running
4-hour timer was dead complexity that obscured the per-user pool
work to come.

Catalog freshness on auto-reconnect is preserved by scheduling an
unblocking _refresh_server task on the mcp-loop after _connect_one
succeeds; the calling thread returns immediately so half-open
recovery latency does not double. Adds MCPClientManager.reconnect_sync
(clears the circuit, closes any existing session, calls _connect_one,
clears stale catalog on failure).

Wires a new pair of operator endpoints —
POST /v1/api/admin/mcp-servers/{name}/refresh and
/v1/api/admin/mcp-servers/{name}/reconnect — that fan out to all
nodes through the existing _internal route family, with per-row
"Refresh" and "Reconnect" buttons in the MCP Servers admin tab.
The new node-internal paths /api/_internal/mcp-{refresh,reconnect}/
are gated to the approve scope to prevent direct unprivileged
reconnects bypassing the console's admin.mcp gate. Internal
endpoints return generic error messages and a filtered status
payload (no command/url) to keep transport details admin-gated.

Drops the [mcp] refresh_interval setting, the
--mcp-refresh-interval CLI flag, and the matching config-mapping
entry; updates docs/architecture.md, docs/tools.md,
docs/settings.md, and the three PlantUML diagrams that referenced
the periodic loop.

Tradeoffs (intentional):
- Idle nodes will not auto-rejoin a recovered MCP server until
  traffic arrives or an operator clicks Reconnect. The previous
  background reconnection loop is gone by design — push
  notifications + operator controls replace it.
- Console fan-out blocks on the slowest node (existing pattern);
  not changed here.

This is Phase 1 of the OAuth-MCP series — feature subtraction
ahead of per-user state.
 2026-05-04 22:00:23 -07:00
Patrick Buckley 3cf87628d2 docs(oidc): document TRUSTED_ENDPOINT_HOSTS + fix three-vs-four required drift (cumulative q-1, q-2) 
The 8-commit OIDC stack added TURNSTONE_OIDC_TRUSTED_ENDPOINT_HOSTS
(operator allow-list for cross-host IdP discovery endpoints) and
promoted TURNSTONE_OIDC_REDIRECT_BASE to required, but the docs drifted
in two places:

q-1 — Troubleshooting > "OIDC not configured" still listed three
required env vars. An operator hitting the missing-redirect-base
startup error landed on a debugging entry that didn't mention the
variable they were missing. Fixed; added a separate troubleshooting
entry naming the exact log message produced by initialize_oidc_state
when redirect_base is unset.

q-2 — TURNSTONE_OIDC_TRUSTED_ENDPOINT_HOSTS was undocumented entirely.
Added a row to the env-var table and a new "Cross-host endpoints"
section explaining when the knob is needed (Google is the canonical
multi-origin IdP, but it's auto-handled; the env var is for any other
IdP whose discovery doc legitimately references hosts beyond the
issuer's origin). Added a troubleshooting entry pointing at the new
section.
 2026-05-04 14:27:19 -07:00
Patrick Buckley bae4adca12 refactor(oidc): quality cleanup (bug-3, q-1/3/4/6/7/9/10/11/12/13) 
Eleven small maintenance fixes; no behavior change beyond bug-3.

bug-3: pending.get('audience', audience) couldn't fall back because
  pop_oidc_pending_state always returns a dict with the audience key
  set verbatim from a non-null TEXT column. Replaced with
  pending.get('audience') or audience to cover the empty-string case
  defensively. Comment explains the security rationale.

q-1: extract _env_or_cfg_str / _env_or_cfg_bool helpers in oidc.py;
  load_oidc_config's six near-identical env-or-config blocks collapse
  to one-liners. role_map / trusted_endpoint_hosts / redirect_base
  retain bespoke parsing.

q-3: discover_oidc narrows except (httpx.HTTPError, ValueError, KeyError)
  with exc_info=True.

q-4: OIDC_STATE_TTL_SECONDS = 300 constant in oidc.py; auth.py imports
  and passes it explicitly. Storage signatures keep the literal default
  (storage layer doesn't know OIDC TTL semantics).

q-6: hoist runtime imports (OIDCError, OIDCKeyNotFoundError, exchange_code,
  fetch_jwks, provision_oidc_user, validate_id_token, build_authorize_url,
  generate_pkce_verifier) to module scope in auth.py. The genuine cycle
  is only oidc._derive_username -> auth.is_valid_username, kept
  function-scoped. test_oidc_handlers.py mock targets repointed to
  turnstone.core.auth.X to match the new binding.

q-7: comment + docs explain the 'oidc' vs 'oidc-default' assigned_by
  marker distinction.

q-9: OIDCIdentity / OIDCPendingState TypedDicts in storage protocol.
  Implementations construct via TypedDict syntax so mypy structurally
  verifies all required fields.

q-10: fetch_jwks narrows except (httpx.HTTPError, ValueError); docstring
  matches.

q-11: rename generate_pkce_pair -> generate_pkce_verifier; return only
  the verifier (build_authorize_url already recomputes the challenge).

q-12: extract _buildOidcRow helper in admin.js so future field additions
  go in one place.

q-13: OIDCConfig docstring lists startup-config vs discovery-derived
  field groups.
 2026-05-04 14:27:19 -07:00
Patrick Buckley 52aba17740 fix(oidc): require TURNSTONE_OIDC_REDIRECT_BASE; drop Host-header fallback (sec-2) 
_build_oidc_redirect_uri previously fell back to the request Host
header when redirect_base was unset. With a permissive reverse proxy
or direct backend access, a spoofed Host minted an authorize URL
pointing to attacker-controlled host — combined with a permissive
IdP redirect_uri allowlist this enables auth-code interception.

There is no production scenario where a Host-derived redirect_uri is
correct, so this fails closed:

- initialize_oidc_state checks redirect_base after discovery succeeds
  and disables OIDC (with an explicit error log naming the env var)
  if it's empty. Runs before fetch_jwks so a misconfigured deploy
  doesn't make a wasted JWKS call.
- _build_oidc_redirect_uri simplifies to f"{redirect_base}/v1/api/auth/oidc/callback".
  request parameter dropped; both call sites (handle_oidc_authorize,
  handle_oidc_callback) updated.
- docs/oidc.md promotes TURNSTONE_OIDC_REDIRECT_BASE from "Recommended"
  to "Required" with the security rationale.
 2026-05-04 14:27:19 -07:00
Patrick Buckley 6c28ac828f docs(skills): add import-conversation-history SKILL.md 
Source-agnostic guide that teaches an agent Turnstone's destination
contracts (workstream + conversations schema, ws_id routing, OpenAI
message shape, tool-call/result pairing, provider_data fidelity blob,
attachment lifecycle) so it can map any external chat export onto them.
Validated against turnstone.core.skill_parser.
 2026-05-01 16:16:17 -07:00
Patrick Buckley f24c6d6c73 docs(readme): refresh hero image to coordinator UX shot 
Replaces the old mermaid-rendering shot with a coordinator session
mid-attention — parallel tool batches, judge-graded approval,
children + tasks side panels — which more accurately represents
what the platform does today.
 2026-04-29 00:13:48 -07:00
Patrick Buckley 7d6b31e18a fix(coord): close gaps an operator's harness shakedown surfaced (#444) 
* fix(coord): close gaps an operator's harness shakedown surfaced

Operator-driven shakedown of the coordinator tool surface flagged
five issues; this commit addresses all of them plus the review
findings against the initial fix.

1. Cancelled-mid-stream partial assistant content now carries a
   "[generation cancelled before completion]" marker.  Without it,
   ``inspect_workstream`` / ``wait_for_workstream`` callers and the
   next coord-LLM turn read the truncated text as a complete answer.
   ``_cancelled_partial_msg`` no longer ships ``_provider_content``
   (Anthropic would otherwise read that lane verbatim and bypass the
   marker; partial tool_use blocks could also leak through).

2. ``spawn_workstream`` / ``spawn_batch`` no longer surface the
   routing-proxy ``status`` field (always HTTP 200 on the success
   path).  The tool description claimed it was "lifecycle state at
   creation"; code that did ``if result["status"] == "idle"``
   silently never matched.  Lifecycle state lives on the workstream
   row — ``inspect_workstream`` is the read.  Tool JSON descriptions
   plus docs/coordinator-skills.md and docs/bulk-endpoints.md
   examples updated to match.

3. ``inspect_workstream`` not-found error string is bare ("workstream
   not found"); the structured ``ws_id`` field carries the queried
   id.  Pre-fix the error STRING echoed the id back at the caller
   who just sent it — redundant and out of step with the rest of the
   surface.  Cross-tenant + missing rows still return the same shape,
   preserving the existence-leak guarantee.

4. ``tasks(...)`` is now rejected when called in a parallel tool
   batch.  The prior shape relied on a docstring warning ("a list
   paralleled with writes can reflect pre-write state") that put
   cognitive overhead on every model invocation; turning the silent
   footgun into an explicit error means the model only thinks about
   the rule the moment it actually breaks it.  Warning dropped from
   the tasks tool description.  ``_PARALLEL_INCOMPATIBLE_TOOLS``
   constant in session.py is the extension point for any future
   tool with the same read-after-write hazard.

Plus the multi-stage code review's findings against the initial
fix (q-1 / q-2 docs drift, q-3 idiom, q-4 keys-assertion, q-5
duplicate guard) — all addressed in the same pass.

Tests: 4752 pass, +6 net since the pre-fix baseline.  Ruff + mypy
clean.  Three new tests pin the parallel-batch-rejection behaviour
on tasks (rejected when batched, runs alone, sibling tools
unaffected); existing cancel + spawn + inspect tests updated to
match the new shape.

* fix(coord): close two copilot review gaps on PR 444

Copilot review on PR 444 flagged two follow-ups:

1. Empty-content cancel divergence — when ``GenerationCancelled``
   races BEFORE the first content token, the prior shape skipped
   ``save_message`` and only appended an empty-content msg in
   memory.  In-memory and storage diverged: a rehydrate would see
   nothing in storage but the session would carry an empty
   assistant turn.  Both branches now persist; on the empty-content
   shape the marker becomes the entire message
   ("[generation cancelled before completion]") so storage matches
   the in-memory history.

2. Test stub cleanup — three new tests injected ``ui.approve_tools``
   via ad-hoc ``lambda + type: ignore[attr-defined]``.  Replaced
   with a permissive ``approve_tools`` method on ``_StubUI`` so the
   stub matches the SessionUI surface the dispatcher actually
   reads.  Tests that exercise approval pathways can still override
   per-instance.

Tests: 4752 pass.  Ruff + mypy clean.
 2026-04-28 12:52:36 -07:00
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 a23ef7306c fix(approve): apply Copilot feedback + remove plan doc 
Copilot review on PR #424 flagged three items:

1. Schema drift on /v1/api/dashboard — DashboardWorkstream didn't
   declare the new pending_approval_detail field, so generated
   OpenAPI / typed clients were out of sync. Added
   PendingApprovalItem + PendingApprovalDetail Pydantic models
   and referenced PendingApprovalDetail from DashboardWorkstream.

2. deepcopy under _ws_lock in serialize_pending_approval_detail
   could extend lock hold under contention with on_intent_verdict
   (daemon judge thread) and per-token activity writes that also
   take _ws_lock. _llm_verdicts entries are only assigned/cleared,
   never mutated in place, so a snapped reference is stable after
   the lock drops. Snapshot refs under lock; deepcopy after release.

3. Plan doc removed from the branch — design docs are local-only
   working artifacts, same posture as PROGRESS.md.
 2026-04-27 11:41:14 -07:00
Patrick Buckley fbb9be27f9 feat(approve): expose pending_approval_detail on /dashboard + guard stale call_id 
Lays the server-side groundwork for inline approve/deny buttons + judge
verdict on the coordinator children-tree UI. Two surgical changes:

1. SessionUIBase.serialize_pending_approval_detail() merges the active
   _pending_approval items[] with per-call_id verdicts from
   _llm_verdicts. The dashboard handler embeds this on every per-ws
   row so cluster live-bulk callers can render inline UI without an
   extra per-child round-trip.

2. make_approve_handler now returns 409 when the body sends a call_id
   that doesn't match any currently-pending item. Closes the stale
   call_id race where an operator clicks approve on a row showing
   call A while the child has rolled over to call B. Empty/missing
   call_id preserves backwards compatibility with CLI + channel
   adapters that don't track it.

Cross-tenant exposure on /dashboard is consistent with the trusted-team
posture already in place for activity / tokens — documented in the new
method's docstring so the choice survives the next reviewer.

Plan: docs/design/inline-child-approvals.md (chunk 1 of 4).
 2026-04-27 11:41:14 -07:00
Patrick Buckley 5874159ffd fix(close): require non-empty body, restore CloseWorkstreamRequest 
Copilot caught three real issues in PR #422 review, all clustered
around the close request body contract:

1. The interactive close handler runs with
   ``supports_close_reason=True``, which calls
   ``read_json_or_400(request)`` — an empty / non-JSON body returns
   ``400 {"error": "Invalid JSON body"}``. The previous SDK fix
   sent NO body via ``json_body=None``, which would 400 against a
   real server. The mock-transport test silently masked it because
   the mock answered without inspecting the body.
2. The doc said the body was empty (or ``{}``), with no mention
   of the optional ``reason`` field, its 512-byte cap, or the
   credential-redaction guard.
3. The Pydantic schema for close was deleted outright; OpenAPI
   and SDKs lost their typed shape for the optional ``reason``.

Changes:

- ``turnstone/api/server_schemas.py``: reintroduce
  ``CloseWorkstreamRequest`` with a single optional
  ``reason: str | None = None`` field. Docstring documents the
  must-be-valid-JSON contract and notes that coord ignores the body
  (``supports_close_reason=False``).
- ``turnstone/api/server_spec.py``: re-import the schema, point the
  close ``EndpointSpec`` at it via ``request_model=``, restore the
  ``_ALL_MODELS`` entry. OpenAPI JSON regenerated.
- ``turnstone/sdk/server.py``: ``close_workstream`` (sync + async)
  gains an optional ``reason: str | None = None`` parameter and
  always sends ``json_body={}`` (or ``{"reason": ...}``) so the
  body is never empty. Adds a regression test
  (``test_close_workstream_sends_valid_json_body``) that inspects the
  raw transport content rather than relying on a path-keyed mock —
  the kind of check that would have caught this bug pre-merge.
- ``sdk/typescript/src/server.ts``: ``closeWorkstream`` gains an
  optional ``opts.reason`` parameter; reintroduce
  ``CloseWorkstreamRequest`` interface in ``types.ts`` and re-export
  from ``index.ts``.
- ``docs/api-reference.md``: close section documents the JSON-body
  requirement, the ``reason`` field, the 512-byte cap, the
  multibyte-safe behavior, the credential-redaction guard, and the
  non-string-coercion path.
- ``CHANGELOG.md``: amend the 1.5.0 BREAKING block to reflect the
  schema reintroduction (slim form, ``reason`` optional) instead of
  the prior "removed outright" claim.

4558 tests passing under ``-m "not live"`` (was 4557 — +1 from the
regression test). ruff + mypy clean.
 2026-04-26 22:14:22 -07:00
Patrick Buckley d6e615d324 fix: apply /review feedback on legacy URL cleanup 
Reviewer caught real misses on the consumer-swap claim:

- TypeScript SDK still defined and re-exported `CloseWorkstreamRequest`
  (types.ts + index.ts) — drop both. Now matches the Python-side
  removal.
- Four `tests/test_auth.py` cases (`test_write_full_token_ok`,
  `test_approve_full_token_ok`, `test_bearer_takes_precedence_over_cookie`,
  `test_cookie_full_on_write_ok`) were tautological after the legacy
  URL removal: they posted to `/api/send` / `/api/approve` and asserted
  `allowed is True`, but those paths now classify as `read` so a read
  token would also pass — they no longer tested the write/approve
  scope enforcement. Swap to path-keyed URLs to restore the original
  intent.
- `is_public_path("/api/send")` test renamed + retargeted to a
  path-keyed URL.

Doc-table drift the previous commit missed:

- `docs/security.md` path-to-scope mapping rewritten for the
  path-keyed verb family (write set, DELETE-on-/send dequeue,
  per-ws_id approve).
- `docs/architecture.md` scope-model row text swap from `/api/send`
  / `/api/approve` to the path-keyed equivalents.
- `docs/diagrams/01-system-context.puml` channel→server edge label
  swap.
- `docs/diagrams/15-auth-architecture.puml` scope class swap.

Cosmetic comment-only stragglers:

- `tests/test_session_worker.py` module docstring URL update.
- `tests/test_ratelimit.py` ~11 `/api/send` fixture-key strings
  retargeted to `/api/workstreams/abc/send` so the URL fixtures
  reflect the post-1.5 surface (rate limiter is path-agnostic; the
  swap is purely cosmetic).

4557 tests still passing under -m "not live"; ruff + mypy clean.
 2026-04-26 22:14:22 -07:00
Patrick Buckley ad0e7ce6eb docs: mark 1.5.0 legacy URL surface removal 
CHANGELOG [Unreleased] / Removed (BREAKING — 1.5.0) block calling out
the legacy URL family removal with the swap table. Doc passes on
api-reference.md (per-endpoint sections rewritten with path
parameters and slimmer body shapes), architecture.md (handler-list
diagram and console-proxy URL example), console.md (URL-rewriting
JS shim docstring + SSE proxy example), and the two PlantUML
diagrams (11-console-data-flow, 16-channel-architecture).

Also picks up two test-side stragglers from step 5 that referenced
the legacy adapters in a docstring + a stale /v1/api/events SSE
test: turn into path-keyed equivalents. OpenAPI JSON dump regenerated
to reflect the catalog edits from step 3.

After this commit:
- 4557 tests passing under -m "not live"
- ruff + mypy clean on turnstone/ tests/ sdk/
- grep for "/v1/api/send", "/v1/api/approve", "/v1/api/cancel",
  "/v1/api/workstreams/close" returns zero hits across turnstone/
  sdk/ docs/ tests/ (excluding CHANGELOG.md, which intentionally
  documents the old shape).
- grep for make_legacy_body_keyed_adapter, make_legacy_query_keyed_adapter,
  _make_method_dispatch, close_legacy returns zero hits.
 2026-04-26 22:14:22 -07:00
Patrick Buckley fef266dbd9 docs: apply Copilot review feedback on PR #421 
Switch fenced-code language tag from `json` to `http` on the seven
example blocks that mix an HTTP request line with a JSON body
(/trust, /restrict, /stop_cascade, /close_all_children, /approve,
/cancel, /close). Pure JSON response blocks stay tagged `json`.

Pre-existing pattern in the doc that Copilot flagged on the lines
this PR touched; fixed across all instances for consistency. No
content / URL changes — only fence-tag adjustment for correct
syntax highlighting.
 2026-04-26 20:00:45 -07:00
Patrick Buckley 059bbc3729 docs: update coord URL tree to post-Stage-2 unified /v1/api/workstreams 
The Stage 2 verb-shape lift converged coord and interactive on the
unified /v1/api/workstreams/{ws_id}/<verb> URL tree; the
/v1/api/coordinator/* tree was removed in P0. Two docs still
documented the pre-lift surface:

- coordinator-api-tour.md (the integrator's lifecycle walk-through):
  rewrites all 9 step URLs to the post-lift paths, keeps a one-block
  callout noting the historical /v1/api/coordinator/* tree and why
  it converged, and drops the operation-id column (operation ids
  shifted with the URL move and are now best looked up live via
  /openapi.json + Swagger UI rather than baked into prose).
- bulk-endpoints.md (the cascade-mutation shape contract): two table
  rows for stop_cascade / close_all_children fixed.

No code changes. CHANGELOG entry kept implicit since this is doc-only
and the URL convergence itself was already documented under the P0
verb-lift CHANGELOG block.
 2026-04-26 20:00:45 -07:00
Patrick Buckley 6572437c5d refactor(server): rename dashboard row id → ws_id for v1 row-shape consistency 
The /v1/api/dashboard endpoint was the last workstream-listing surface
keyed on `id` rather than `ws_id`. The Stage 2 list-verb lift converged
the active list (`/v1/api/workstreams`) and saved list
(`/v1/api/workstreams/saved`) on `ws_id` but explicitly left dashboard
alone to keep that PR's diff focused. This lands the same rename on
the remaining endpoint so v1 row shape is consistent across the family.

Scope kept narrow:

- Pydantic `DashboardWorkstream` and TS SDK `DashboardWorkstream`
  interface both rename `id: str/string` → `ws_id`.
- The bundled web UI (`turnstone/ui/static/app.js`) is the only consumer
  reading `dashboard.workstreams[].id` and is updated atomically.
- Console `_fetch_live_block` (cluster-inspect's projection over a
  remote node's dashboard payload at `turnstone/console/server.py`)
  flips its `entry.get("id")` lookup to `entry.get("ws_id")`.
- Drive-by: stale `id` example in `docs/api-reference.md` for the
  earlier `/v1/api/workstreams` rename also fixed.

`_build_node_snapshot` (the global-events SSE node_snapshot payload
consumed by the cluster collector) deliberately stays on `id` — it's
part of a separate cluster-row family (collector → cluster_workstreams
→ console UI) that is internally consistent on `id` and would need its
own coordinated sweep. CHANGELOG documents the bounded blast radius.

Tests: 4554 passing (-m "not live"). ruff + mypy clean.
 2026-04-26 20:00:45 -07:00
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 a46dab1 removed
row-level ownership gates — so _active_coords is now just
coord_ws_id → ui.

perf-6: _enqueue_on_ui was doing {**payload, "ws_id": coord_ws_id} on
every dispatch. The dispatch path owns payload and doesn't reuse it —
mutate in place.

* test(coord): add adapter tests for worker dispatch + children registry + fan-out

Fills the coverage gap on CoordinatorAdapter — the review (q-3) flagged the
coord-specific concurrency paths ported from the deleted CoordinatorManager
as untested. Three new test classes:

- TestCoordinatorAdapterWorkerDispatch: _spawn_worker reuse gate, queue.Full
  backpressure, concurrent-call bug-1 reproducer (two threads → exactly one
  worker via ws._lock + _worker_running), finally-clears-flag.
- TestCoordinatorAdapterChildrenRegistry: registry seed on emit_created vs
  emit_rehydrated rebuild, _pop_coord_registry_locked reverse-index cleanup,
  _merge_child_ids_locked idempotency, _prime_children_from_snapshot merge.
- TestCoordinatorAdapterDispatchChildEvent: unknown-parent drop, ws_created
  fan-out, cluster_state / ws_closed reverse-index routing, perf-6 in-place
  ws_id stamp.

* fix: regressions flagged by ultrareview

Verify stage of the cloud review surfaced 6 confirmed regressions
from Stage 1's adapter layer. Fixing together since they share the
same root cause (plumbing moved into adapters without retiring the
old emission paths).

- Interactive adapter emit_created / emit_state / emit_rehydrated
  become no-ops. The create_workstream HTTP handler still fires
  ws_created (after attachment validation, per the pre-Stage-1
  "no phantom events on rejected upload" contract); WebUI
  _broadcast_state still fires ws_state with the full payload
  (tokens + context_ratio + activity). Firing from the adapter too
  was duplicating both events. Also closes the phantom-ws-created
  regression (adapter fired before attachment validation ran).

- emit_closed Protocol gains a ``name`` kwarg; the adapter is the
  sole emitter for ws_closed on interactive now, and the frontend
  eviction toast needs the name. Manager passes ws.name from
  close() / create()+open() eviction / close_idle paths.

- _idle_cleanup_thread stops firing its own reason="idle" ws_closed
  — close_idle already fires via the adapter with reason="closed",
  and the frontend never differentiated the two anyway.

- close_workstream_endpoint fix: "Cannot close last workstream" 400
  was a stale error (the guard went away with the default-startup
  workstream). Return 404 on close() == False (which now means the
  ws was already closed or unknown). Also switches the audit actor
  from _require_ws_access's stored owner to _auth_user_id — the
  stored owner is metadata post-#400, so attributing actions to it
  misrepresents who actually did them.

- CLI /ws close mirrors the same stale-error fix.

- SessionManager.close now calls storage.delete_workstream_override
  alongside update_workstream_state, same as the old
  WorkstreamManager.close did. Without it overrides leak until
  tombstone cleanup. close_idle does the same.

- SessionManager._reserve_and_install_locked records the eviction
  on turnstone.core.metrics so the global eviction counter keeps
  working. Old WSM did this inline; the unification dropped it.

- ConsoleCoordinatorUI.on_rename now fans out to the cluster
  collector via a new class attribute ``_collector`` (set at
  console startup alongside ``_coord_mgr``). The old
  ``_on_rename_observer`` plumbing went away with
  CoordinatorManager and the "adapter emit_console_ws_rename runs
  from whichever code path renames" comment was aspirational —
  nothing actually did it.

Full pytest: 4375 passed (tests/live + test_server_live.py excluded;
both pre-existing live-backend failures unrelated to this branch).
Ruff + mypy clean.

* refactor(ui): extract SessionUIBase for shared UI scaffolding

Direct response to review feedback that the unification wasn't
merging enough of the two workstream kinds. WebUI (node) and
ConsoleCoordinatorUI (console) both:

- Keep a per-UI list of SSE listener queues guarded by a lock
- Block a worker thread on _approval_event / _plan_event
- Fan enqueued events out with the same ws_id-stamping pattern
- Resolve approvals / plans with the same broadcast-then-signal
  pattern

All of that now lives once in turnstone/core/session_ui_base.py.
Both UIs subclass SessionUIBase; kind-specific bodies (WebUI's
per-UI metrics + _broadcast_state + intent-verdict bookkeeping,
ConsoleCoordinatorUI's collector fan-out) stay in the subclasses.

WebUI.resolve_approval still overrides the base (it adds intent-
verdict updates) but now calls super() for the shared broadcast +
event-set steps. Same shape as the other approval/plan hooks:
subclasses extend, base provides skeleton.

Net file-level: +156 LOC for the base, -144 LOC across the two
subclasses. The raw number is unexciting — but there's now a
single source of truth for the listener + blocking-gate machinery,
and bugs (like the duplicate ws_created / ws_state events that
prompted this refactor) can't arise from the two implementations
drifting.

Full pytest: 4375 passed. Ruff + mypy clean.

* refactor(ui): move metrics + verdict bookkeeping into SessionUIBase

Second pass at unifying the two UIs. Per-workstream metrics
accumulators (token counts, tool-call counts, context ratio,
activity tracking), intent-judge verdict cache + pending-decision
list, and the verdict-persistence path all move to SessionUIBase.

Before: WebUI tracked all of it; ConsoleCoordinatorUI tracked none
of it (a comment on the old on_intent_verdict literally admitted
the deferral — "skip the persistence + late-decision plumbing that
WebUI does"). Coord sessions never got verdict rows in storage, never
had a user_decision stamped, and the dashboard had no way to show
coord token usage because the data wasn't captured.

Now the base class captures the data and persists the rows for
every kind. Kind-specific broadcast (WebUI's _broadcast_state with
rich per-UI payloads) stays on WebUI; prometheus counters on the
node (_metrics.record_judge_verdict) stay on WebUI's on_intent_verdict
override. Everything else shared.

Behaviour change worth flagging: coord sessions now write
intent_verdicts and output_assessments rows for every judge call
and every output-guard warning. Previously silent; the storage rows
now exist and any future coord-dashboard surface can read them.

Shape of the unification:
- resolve_approval: was overridden on WebUI (intent-verdict decision
  propagation); now lives on the base. Both kinds inherit unchanged.
- on_intent_verdict: WebUI overrides only to add _metrics.record_*;
  rest of the body is the base.
- on_output_warning: was on both separately; fully base-shared now.

Full pytest: 4375 passed. Ruff + mypy clean.

* fix: regressions flagged by second-pass review

Three confirmed findings with direct fixes + a dedicated test file
for SessionUIBase (was previously uncovered).

bug-1 — Coord approve_tools didn't reset _last_verdict_decision or
clear _llm_verdicts between approval rounds. WebUI did (inline).
Coord inherited SessionUIBase.on_intent_verdict which stamps via
the decision flag, so after the first resolve every subsequent
round's verdicts were stamped with the prior round's user_decision
before the user had decided the new round.

Fix: add SessionUIBase._reset_approval_cycle() clearing both under
_ws_lock; call from the top of both subclass approve_tools methods.
Single-source invariant — can't drift again.

sec-1, sec-2 — delete_workstream_endpoint and open_workstream's
rehydrate path recorded the audit row under the stored ws.user_id
("owner_uid") rather than the authenticated caller. With row-level
ownership gating gone (a46dab1), any team member acting on a peer's
workstream produced an audit row naming the victim as the actor.
Fix: pass _auth_user_id(request) as the audit actor, matching the
pattern close_workstream already follows.

q-2 — SessionUIBase had no direct tests. The new
tests/test_session_ui_base.py covers listener fan-out, approval +
plan blocking gates, intent-verdict cache + FIFO eviction, verdict
persistence paths, output-guard persistence, the reset-between-rounds
invariant (bug-1 regression test), a cross-subclass test that
verifies BOTH WebUI.approve_tools and ConsoleCoordinatorUI.approve_tools
call _reset_approval_cycle (verified it fails without the fix), and
a concurrent enqueue/register smoke.

Full pytest: 4395 passed (+20 new). Ruff + mypy clean.

* fix: PR #408 review findings from copilot + code-quality

Three substantive fixes + mechanical side-effect-in-assert cleanup.

Copilot findings:

- session_ui_base.py: on_intent_verdict had a race with
  resolve_approval. Previously acquired _ws_lock twice (read decision
  → release → if unset, acquire again to append). resolve_approval
  could interleave between the two acquisitions, swap-and-clear the
  pending list and set the decision — our verdict then got appended
  to the fresh (empty) list and stamped with the NEXT round's
  decision on the following resolve. Fix: decision-check + append
  under ONE acquisition; storage UPDATE (if decision already set)
  runs outside the lock. New regression test counts lock
  acquisitions during on_intent_verdict and fails if the two-phase
  pattern returns.

- server.py close_workstream_endpoint: comment said "treat as
  already-closed success" but handler returned 404. Comment
  rewritten to match the 404 behaviour ("the ws isn't tracked here"
  is the only reachable meaning for close() → False now).

- test_session_ui_base.py concurrency smoke: the test ended with
  ``pytest.assume = lambda ...`` — a leftover that mutates pytest
  globals and can surprise other tests. Replaced with explicit
  ``not is_alive()`` assertions so the "threads completed cleanly"
  intent survives -O optimization stripping.

Code-quality (assert side-effects):

Six ``assert mgr.open(...)`` / ``assert mgr.close(...)`` in
test_session_manager.py stripped under ``python -O``. Mechanical
fix: extract to local before asserting.

Ignored the two "Protocol method body is `...`" flags — that's the
standard Protocol idiom; replacing with ``pass`` or
``NotImplementedError`` changes typing semantics.

Full pytest: 4396 passed.
 2026-04-24 14:28:51 -07:00
Patrick Buckley a76d93b6c6 docs(coordinator): phase 8 PR C — API tour, skills guide, bulk-endpoints contract + wait diagram (#388) 
* docs(coordinator): phase 8 PR C — API tour, skills guide, bulk-endpoints contract

Four deliverables that close out the phase 8 doc debt carried since
phase 1:

- docs/coordinator-api-tour.md — 9-step lifecycle walkthrough
  (create → subscribe → send → inspect children / detail → wait for
  fan-out → govern (trust / restrict / stop_cascade / close_all_children)
  → approve / cancel → close), one request + response per step, every
  SSE event type the UI has to handle, and every operation id cross-
  referenced against the live /openapi.json.  Integrators driving a
  coord session from a custom UI or SDK can work end-to-end from this
  doc without reverse-engineering the console page.

- docs/coordinator-skills.md — writing a SkillKind=COORDINATOR skill.
  Tool-surface diff (13 orchestration tools, no bash / edit / web /
  sub-agent), persona diff (orchestrator vs maker, composing on
  base_coordinator.md), SkillKind enum + migration 044, task_list
  integration, ws_id handling, wait vs inspect cost profile, three
  orchestration patterns (delegate-and-summarise, fan-out-and-
  synthesise, plan-then-delegate), testing surface.

- docs/bulk-endpoints.md — codifies the two shape idioms that shipped
  across phases 6–8: {results, denied, truncated} for bulk-read /
  bulk-create-with-payload (cluster/ws/live, spawn_batch); {<bucket>,
  failed, skipped} for cascade-mutation (stop_cascade,
  close_all_children).  Picks-by-semantics guidance so the next bulk
  endpoint author doesn't coin a third shape.

- docs/diagrams/27-coordinator-wait-for-workstream.puml + rendered
  PNG — sequence diagram covering spawn → wait (blocking, with
  bounded progress emission) → inspect → close.  Embedded in the
  API tour doc's §6 so the "why is my coord session blocking?"
  question has a visible answer.

No code changes.  All operation ids in the API tour verified against
a live build of the console spec; all markdown internal links
resolve; PlantUML renders clean on the system plantuml jar.

* docs(coordinator): address PR #388 copilot review

- api-tour.md child-event payload keys: events stamp `ws_id` as the
  coord's own id and carry the child's id separately as
  `child_ws_id`.  Doc previously listed `ws_id` as the child
  identifier on all four child_ws_* events, which would send SDK /
  UI implementers parsing the wrong field.
- api-tour.md SSE table: add the `status` event emitted by
  ConsoleCoordinatorUI.on_status (token usage + context_window +
  effort snapshot; fires on every streaming tick).  Previously
  omitted from the "every event type a UI has to handle" list.
- api-tour.md /children response key: server returns `{items,
  truncated}`, not `{children, truncated}`.  Also drop the
  `state=closed` query-param claim — the endpoint has no state
  filter; clients filter locally on the returned `state` field.
- skills.md task_list shape: the persisted row uses `id` (not
  `task_id` — the input schema uses `task_id`, the row uses `id`),
  has `child_ws_id` / `created` / `updated` (no `notes` field),
  and supports a 5th `reorder` action alongside add/update/remove/
  list.  Adds the parallel-dispatch caveat from the tool
  description.
- skills.md tenant-guard behaviour: foreign / hallucinated ws_ids
  don't return an empty result — they return explicit
  error/not-found/denied shapes that differ by op (mutating ops
  return `{error, status: 404}`; inspect returns `{error}`; wait
  reports state=denied).  Important distinction — a skill that
  expects empty on mismatch will mishandle every single case.

Docs-only; no code / schema / SDK changes.  All internal links
still resolve.
 2026-04-19 10:15:08 -07:00
Patrick Buckley 7c16b0dfa8 refactor(routing): replace hash-ring rebalancer with rendezvous (HRW)… (#384) 
* refactor(routing): replace hash-ring rebalancer with rendezvous (HRW) hashing

Routing was a stored bucket table maintained by a central rebalancer
daemon, which shared its liveness primitive (services.last_heartbeat)
with the collector — when a heartbeat-fresh node went into a zombie
HTTP-handler-broken state, neither the collector nor the rebalancer
could self-correct, and the router kept directing traffic at it.
Rendezvous hashing makes the route a pure function of (ws_id,
live_services) so the heartbeat is the single source of truth and any
liveness-eviction propagates to the next route call without a separate
state-publication step.

The rebalancer's central state has no analogue: the new router computes
the per-key node winner on every call, the collector pushes membership
updates into the router cache from its discovery thread, and per-route
overrides survive on workstream_overrides. Eager workstream migration
goes away; in-flight workstreams lazily rehydrate from storage on the
new owner — already the dead-node behaviour.

* fix(tools): describe rendezvous re-routing on spawn/inspect node_id

The first pass overclaimed `node_id` "stays canonical for this
workstream's lifetime" — under rendezvous routing the active owner
re-derives per-call from live membership, so a node join/drop after
spawn can shift it.  Tool descriptions now say `node_id` is the
spawn-time binding; subsequent ops re-route via rendezvous over the
current live-node set; the new owner lazily rehydrates from shared
storage; coordinators should re-read with inspect_workstream rather
than caching the value.
 2026-04-18 19:02:52 -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 cab57f244d refactor(channels): backfill review of Slack/Discord adapters (#382) 
* refactor(channels): backfill review of Slack/Discord adapters

Retrospective multi-stage review of the Slack (PR #355) and Discord
channel adapters — they shipped before the review pipeline existed,
so this pass goes back and fixes everything the pipeline would have
caught plus a follow-up round of ultrareview findings.

## Security (8 fixes)

- Adapter-side owner checks on all interactive flows: Discord
  ApprovalView / PlanReviewView encode the owner Discord user ID in
  the embed footer (`{ws_id}|{corr_id}|{owner_id}`) and reject
  non-owner clicks; Slack plan-approve / request-changes /
  feedback-modal gain owner tracking in `_pending_plan_review_ts`
  and a shared `_ensure_plan_review_owner` gate.  These closed the
  two critical authz gaps where the gateway's service-scoped JWT
  bypassed server-side ownership checks.
- Discord thread-message gate: only the registered invoker can
  drive the workstream (prevents a linked user posting in another
  user's public thread from injecting into their assistant).
  Invoker recorded explicitly so `/ask` follow-ups survive the
  `channel.create_thread` bot-as-owner quirk.
- Slack /link flow + per-user identity gate: unlinked Slack users
  see an ephemeral `/turnstone link <token>` prompt on every
  message instead of silently creating workstreams under the
  shared gateway identity.  Rate-limited (5/hour) to block online
  token enumeration.
- Gateway `/v1/api/notify` requires `write` scope on the validated
  JWT; low-scope tokens get 403 + audit.
- Thumbnail URL validator DNS-resolves the hostname before fetch
  and rejects any resolved IP that's loopback / link-local /
  multicast / reserved, plus an explicit deny-list for IPv6 cloud
  metadata (`fd00:ec2::/32` — AWS Nitro IMDS + ECS task metadata)
  that would otherwise slip past the `is_private` allowance.
- Per-user rate limit (10 msgs / 60s) + 8 KiB inbound size cap on
  Slack DMs / channels / notification-reply threads so one user
  can't exhaust the shared LLM budget.
- Discord /link rate limit (5/hour) for token-enumeration defense.

## Bug fixes (9 correctness issues)

- Slack DM routing: each top-level DM no longer spawns a fresh
  workstream (was using per-message `ts` as the route key).
- Multi-chunk Slack responses thread correctly under the first
  chunk's ts instead of fragmenting as independent top-level
  messages.
- Finalize the outgoing StreamingMessage before swapping channel /
  thread_ts mid-stream, so buffered tokens still land on the old
  thread.
- Redundant `chat_update` on approve/deny eliminated by popping
  `_pending_approval[ws_id]` after local resolution.
- Notification reply tracking on Discord only registers for DMs
  (guild-channel targets were storing channel IDs where user IDs
  were expected, so legitimate replies were always rejected).
- `get_channel_default_alias` rolls `_channel_default_ts` back on
  `list_models()` failure so the next caller retries instead of
  serving an empty alias for the full TTL.
- Slack `subscribe_ws` purges dead SSE tasks before the
  membership short-circuit (previously an unhandled exception left
  the ws_id in `_subscribed_ws` forever, silently no-opping
  subsequent subscribes).
- ChannelRouter `_create_locks` is now an LRU-bounded OrderedDict
  that evicts only unheld locks (original dict grew unbounded;
  naive LRU could evict a held lock and let a second caller race
  through the critical section, creating duplicate workstreams).
- Slack `_parse_ts` pads the fractional field to 6 digits so
  `"1.2"` and `"1.000002"` stop colliding as `(1, 2)` in the
  latest-session tiebreaker.

## Performance (6 fixes)

- StreamingMessage keeps a rolling truncated display string capped
  at `max_length` so per-flush cost is O(max_length) instead of
  O(total_streamed_chars) — long streaming responses no longer do
  quadratic work every edit interval.
- `StreamingMessage.finalize()` caches the joined content so the
  Discord stream-end DM-forward path doesn't re-join a multi-MB
  buffer twice.
- `PendingApproval` stores the Block Kit payload posted to Slack;
  `IntentVerdictEvent` appends the verdict in-place and
  `chat_update`s, skipping an extra `conversations_history`
  round-trip.
- ChannelRouter `lookup_ws_id()` TTL-caches the channel →
  ws_id resolution (30s TTL, 4096-entry LRU); hot inbound paths
  skip storage on every message.
- Service-discovery startup retry uses exponential backoff
  (1s → 8s cap) with a 30s deadline instead of 30 × 1s fixed
  sleep.
- `_archive_session` now calls `router.close_workstream` so the
  `_node_urls` cache entry is dropped (was leaking one entry per
  archived session).

## Quality / refactors (19 improvements)

- `cli.main()` extracted from a 365-line function into focused
  helpers; imports carefully kept lazy where test patches target
  source-module paths.
- `_run_gateway` finally block now awaits `adapter.stop()` on
  every adapter so SSE tasks, httpx clients, and the Slack socket
  handler close cleanly on shutdown.
- Shared SSE reconnect loop extracted to `turnstone/channels/_sse.py`
  (`run_sse_stream` with `on_event` + `on_stale` callbacks); both
  adapters' `_sse_listener` methods just wire up callbacks. The
  "404 stops reconnect" invariant is enforced inside the helper
  so a broken `on_stale` can't livelock.
- `_on_ws_event` god-dispatchers split into per-event `_handle_*`
  methods with a thin isinstance dispatcher at the top.
- Slack `_on_approve` / `_on_deny` collapsed into a single
  `_resolve_approval(*, approved: bool)`.
- `ApproveRequestEvent` policy evaluation hoisted into
  `ChannelRouter.evaluate_tool_policies` returning a
  `PolicyVerdict`; adapters switch on the verdict kind.
- `ChannelAdapter` protocol trimmed to the four methods adapters
  actually implement; unused `ChannelEvent` dataclass removed.
- Shared constants lifted to `turnstone/channels/_config.py`.
- `_cleanup_stale_route` and `unsubscribe_ws` share a
  `_clear_ws_state` helper.
- `StreamingMessage` private attrs promoted to `message` /
  `message_ts` / `accumulated_text` properties so callers don't
  reach past the `_`-prefix.
- Various cleanups: dead var, noqa'd lambdas, renamed
  `_policy_handled` → `policy_handled`, inlined single-use
  helpers, added module docstrings, documented
  `SlackRoute.parse` edge cases.
- `chunk_message` plain-text fast path (no backticks → skip
  fence bookkeeping).

## Test coverage

Added 45 tests (178 → 223):

- `tests/test_channel_sse.py` (new) — SSE reconnect / backoff /
  404-stale-route / on-stale-exception / invalid-JSON-skip /
  on-event-exception-doesn't-kill-stream / per-connection token
  refresh / ConnectError retry.
- ApprovalView + PlanReviewView owner-check regression tests
  (owner allowed, non-owner rejected, legacy 2-pipe footer fails
  closed, modal path rejected for non-owner, `/ask`
  bot-as-thread-owner follow-up allowed).
- Slack `_recover_routes` latest-ts-wins, `_archive_session`
  drops route + closes workstream.
- SSRF tests: DNS rebinding rejected, IPv4 link-local metadata
  rejected, IPv6 ULA metadata (fd00:ec2::254 / fd00:ec2::23)
  rejected.
- Slack link prefix match (natural-language prompts don't
  hijack), link rate-limit ceiling.
- SlackRoute round-trip across all three shapes + lax-parse
  behaviour.

Lint (ruff) + mypy clean; 210 channel-focused tests pass.

* chore(channels): address PR #382 review-bot feedback

Three line-level findings from github-code-quality on the backfill
review PR.  Copilot had no line-level comments.

- _sse.py:132 — the `except httpx.HTTPStatusError: pass` branch was
  flagged as an empty except.  The original status was already logged
  at WARNING inside the try block (we re-raise ourselves after
  logging), so the handler has real intent.  Added a debug log of the
  exception text + a comment explaining the control flow, so the
  empty-except lint stops firing and the next reader sees why we
  fall through to backoff.
- discord/bot.py:430, cli.py:354, slack/bot.py:1127 — `await task`
  inside `contextlib.suppress` was flagged as "statement has no
  effect".  It's a false positive (await is an effect) and the
  alternative try/except/pass triggers ruff SIM105.  Kept the
  contextlib.suppress pattern and added an explanatory comment above
  each call so the intent (await CancelledError propagation before
  state cleanup) is obvious; will reply on the PR thread noting the
  false positive.

No behavior change.  Lint + mypy clean; 210 channel tests pass.
 2026-04-18 05:49:54 -07:00
Patrick Buckley 37ed6bbf5b feat(core): WorkstreamKind enum + list_workstreams user_id filter (#374) 
Foundation PR for the multi-stage-review follow-up.  Introduces a
single source of truth for workstream kind values and pushes tenant
scoping into the storage protocol so list callers can't forget to
filter client-side.

- WorkstreamKind(StrEnum) replaces bare "interactive" / "coordinator"
  literals across 17 production modules.  Strict mypy narrows every
  internal call site; raw strings still work at wide boundaries
  (HTTP body, DB row) via WorkstreamKind(raw) parse at the edge.
- StorageBackend.list_workstreams(..., user_id=None) adds a SQL-level
  WHERE user_id = :user_id gate on both sqlite and postgres impls.
  Memory wrapper forwards the new filters.
- register_workstream now validates kind at the storage edge so SDK /
  restore / internal callers can't silently corrupt the NOT NULL
  column with empty / mis-cased / unknown values.
- WebUI.__init__ normalizes empty-string parent_ws_id to None, matching
  the storage-edge and WorkstreamManager invariants.
- POST /v1/api/workstreams/new parses body["kind"] through the enum
  and returns 400 on unknown kinds instead of silent coercion.

Absorbs bug-1, bug-2, bug-4/q-6, q-1, q-8, and partial q-2 (wrapper
signature forwards the new filters; full deletion of the unused
wrapper stays in the cleanup PR).
 2026-04-17 22:10:57 -07:00
Patrick Buckley a917bf2690 docs: apply Copilot review feedback on PR #367 
All eight suggestions verified against source before applying:

- docs/settings.md — ConfigStore key names are `model.plan_alias` /
  `model.task_alias` (not `plan_model` / `task_model`); updated in
  both the overview list and the plan/task overrides table.
- docs/security.md — `src` claim values now reflect what actually
  gets minted: `password`, `database` (from API-token exchange),
  `oidc`, plus service origins `console`, `cli`, `channel`.
- docs/sdk.md — `upload_attachment(ws_id, filename, data, *,
  mime_type=...)` matches the real SDK signature; `bytes`-returning
  helper is `get_attachment_content` (not `download_attachment`);
  code example reordered so it doesn't collide on `filename=` kwarg.
- docs/architecture.md — "prior `plan` tool call" → "prior
  `plan_agent` tool call" so wording stays consistent with the
  renamed tool.
- docs/tools.md — `plan_agent` `primary_key` is `goal`, not
  `prompt`, in both the primary-key table and the summary table
  (matches the JSON schema in turnstone/tools/plan_agent.json).
 2026-04-16 16:01:24 -07:00