mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
eb2a119da98994a7d559fd8eebc2ed1f1d0b5e3e
736 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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.
|
||
|
Patrick Buckley
|
0a8083e6d5 |
feat(skills): paste SKILL.md to auto-fill the Create Skill modal (#477)
* feat(skills): paste SKILL.md to auto-fill the Create Skill modal When a user pastes an Anthropic-style SKILL.md (YAML frontmatter + markdown body) into the Create Skill content textarea, the frontend sniffs the leading ``---``, posts the raw text to a new backend parse endpoint, and populates name / description / tags / author / version / license / compatibility / allowed_tools from the parsed fields. The textarea is left with the body only (frontmatter stripped), and a toast reports how many fields were set vs. kept (already-typed values are preserved). Backend - ``POST /v1/api/admin/skills/parse`` (admin.skills permission) wraps the existing ``turnstone.core.skill_parser.parse_skill_md`` so admin imports and external installs share one parser. ``ParseSkillRequest`` / ``ParseSkillResponse`` schemas added; OpenAPI spec + sync/async console SDK methods updated. - Hardening: 32 KiB cap on ``raw`` (Pydantic ``max_length`` + handler enforcement); ``Content-Length`` pre-check returns 413 before any body buffering; parse offloaded via ``asyncio.to_thread`` so deeply-nested YAML cannot stall the event loop. Frontend (turnstone/console/static) - New paste handler with optimistic paint (raw text shown immediately, textarea disabled + ``aria-busy`` flipped, hint switches to "Parsing...") so the round-trip is visible on slow networks. - ``AbortController`` + generation guard (``_ctmPasteController``) so a fresh paste or modal close cancels a stale fetch — the previous handler's callbacks see the controller has been replaced and bail before touching the DOM. - Non-destructive overwrite: ``_setSkillFormField`` returns "filled" / "skipped" / "absent" and refuses to clobber non-empty values. Toast reports counts. - Bumps ``#toast`` z-index above modal overlays (was 200 vs. modal 600 — toasts fired while a modal was open were invisible). Console-wide fix exposed by this being the first feature to fire toasts mid-modal. HTML / CSS - New ``.skill-paste-hint`` line above the textarea announcing the affordance, sized to match surrounding ``.label-hint`` text. - ``aria-describedby`` ties the hint to the textarea; ``aria-live= "polite"`` announces the busy-state transition to screen readers. - "Skill Content" heading hint reworded "system message — ..." → "available: ..." and the variables row label "Variables" → "Used" to disambiguate available vs. in-use template variables. Tests - 11 new cases in ``tests/test_skill_parse_api.py``: happy paths (full / minimal / nested-metadata / unquoted-colon recovery), malformed YAML 400, missing/blank/missing-name 400, RBAC 403, raw body 32 KiB cap (Content-Length pre-check), chunked-encoding bypass forces the application-layer cap. Test pins ``raw_frontmatter`` omission so a future ``dataclasses.asdict`` refactor can't silently leak the full YAML dict back to clients. Validation - 5146 / 5146 ``pytest -k "not live"`` pass. - ``ruff`` + ``mypy`` clean on changed sources. - ``node -c`` clean on governance.js. - Two-stage code review (full pipeline + bug+quality re-review of the fix patches) applied; all confirmed findings addressed. * fix(skills): Copilot PR #477 review fixes (cumulative bug-1, bug-2, q-1) bug-1 (server.py): Content-Length pre-check was clamped to 32 KiB — the same number as the per-string char cap on ``raw``. A legitimate ``raw`` of exactly 32 KiB produces a JSON body well above 32 KiB once the ``{"raw":"..."}`` wrapper and any escaping is added, so valid near-max requests were 413'd. New constant ``_PARSE_SKILL_MAX_BODY_BYTES = _PARSE_SKILL_MAX_CHARS * 4`` admits the wrapper + multibyte expansion while still refusing obviously oversized payloads early; the per-string ``len(raw)`` check stays authoritative. bug-2 (governance.js): hideCreateTemplateModal aborted the inflight paste controller and nulled the global, but the handler's ``.catch`` and ``.finally`` guard each DOM mutation behind ``_isCurrent()`` — both bail when the controller has been nulled, leaving the textarea ``disabled`` + ``aria-busy`` and the hint stuck on "Parsing…". Reopening the modal landed on a poisoned state. The second-pass review's q-2 cleanup that dropped the show-side defensive reset missed this scenario — the verifier's reachability argument confused "controller is null" with "UI state is reset"; the two are independent. Hide now resets the paste-induced visible state alongside the abort. q-1 (console_spec.py): error_codes for the parse endpoint listed only 400; handler also returns 413 for oversized bodies. Added 413; kept 403 implicit per the convention sibling admin endpoints follow. Test fixup: bumped the Content-Length test payload to 200 KB so it clearly exceeds the new 128 KB pre-check threshold; otherwise it was falling through to the per-string check and duplicating test_oversized_raw_chunked_returns_413's coverage. |
||
|
Patrick Buckley
|
b2153d907f |
fix(oidc): close transient client on disable paths + correct docstring
PR #476 review feedback (Copilot, oidc.py:584,616): 1. initialize_oidc_state's docstring claimed "on any failure enabled is False" but the JWKS-prefetch failure branch intentionally keeps enabled=True so the callback's lazy-fetch retry can recover from a transient IdP issue at startup. Docstring rewritten to spell out the three post-conditions: disable, JWKS-failure-keeps-enabled, success. 2. The long-lived httpx.AsyncClient was created up front, then three disable branches (discovery exception, discovery-returned- disabled, missing redirect_base) returned without closing it, leaving sockets held until shutdown. Restructured: discovery now uses a transient AsyncClient inside a context manager (closed at exit). The long-lived client is only created after the disable checks pass. The JWKS-failure branch still legitimately keeps the client open because the lazy-retry path needs it. The pre-existing single-client-passthrough test was replaced with three more specific tests: long-lived client only goes to fetch_jwks (not discover_oidc); discovery-exception path leaves http_client=None; missing-redirect_base path leaves http_client=None. |
||
|
Patrick Buckley
|
5d4a50d2cd |
chore(oidc): consolidate test OIDCConfig helper + fix exceptions banner (cumulative q-4, q-5)
q-4: tests/test_oidc.py's _make_config and tests/test_oidc_handlers.py's _make_oidc_config built the same OIDCConfig with sensible defaults but had drifted — only the handlers helper set redirect_base. After b3 made redirect_base operationally required, every test_oidc.py test that exercised redirect_base had to override it explicitly. A future test could omit redirect_base and silently exercise the wrong production path. Moves make_oidc_test_config to tests/conftest.py with the more complete handler-version defaults (including redirect_base). Both test files import it under their existing local alias (_make_config / _make_oidc_config) so the 60+ call sites in test_oidc.py and the handler tests don't have to change. q-5: section banner '# Exception' (singular) at oidc.py:79 became inconsistent after b5 (callback robustness) added OIDCKeyNotFoundError. Renamed to '# Exceptions'. |
||
|
Patrick Buckley
|
7c6bc22d02 |
perf(auth): migrate handle_auth_status to count_users (cumulative q-3)
The OIDC perf batch added storage.count_users() and migrated the two OIDC handlers (handle_oidc_authorize, handle_oidc_callback) but missed handle_auth_status — which still ran storage.list_users() then len(users) > 0 for the same has-any-users gate. count_users() is one COUNT(*) round-trip vs list_users() rehydrating every row dict. Wrapped in asyncio.to_thread to match the OIDC handler pattern; the async handler no longer blocks the event loop on storage I/O for what's effectively an existence probe. |
||
|
Patrick Buckley
|
d5087ef3b9 |
fix(oidc): serialise role-mapping concurrency + skip no-op write lock (cumulative bug-2, perf-1)
bug-2 (Postgres) — replace_oidc_roles read existing rows under default READ COMMITTED with no row lock. Two concurrent OIDC callbacks for the same user_id (racing token refreshes with differing claim sets) could both observe the same baseline and produce a final role state matching neither caller's intent. Adds .with_for_update() to the SELECT so the existing rows for this user are locked for the duration of the transaction. The lock is per-user_id, not table-wide; unrelated user writes are unaffected. Empty result sets acquire no locks, so a brand-new user with no rows yet still allows two callers to proceed and merge via ON CONFLICT DO NOTHING — that's a permissive race that self-heals on the next reconciliation cycle, documented in code. perf-1 (SQLite) — replace_oidc_roles took the SQLite global write lock unconditionally via BEGIN IMMEDIATE before reading. Steady-state re-logins (claims unchanged, no INSERT/DELETE needed) paid the lock cost for nothing and serialised against unrelated writers. Replaces with a double-check pattern: phase 1 reads under the default deferred transaction (no write lock), computes the diff, and returns (set(), set()) on no-op. Phase 2, only when mutation is needed, commits the read txn, escalates to BEGIN IMMEDIATE, RE-READS, and re-computes the diff under the lock before writing. The returned (added, removed) reflects what was actually written, so caller logging in apply_role_mapping stays truthful even when concurrent writers shifted state between the two reads. The OR IGNORE on insert is now defense-in-depth (the lock makes it unnecessary) but kept as a safety net. |
||
|
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. |
||
|
Patrick Buckley
|
1c41212f15 |
fix(oidc): self-heal stranded user when role mapping fails post-create (cumulative bug-1)
If apply_role_mapping raised after create_oidc_user committed (transient storage failure, race with role deletion, etc.), provision_oidc_user's inline safety-net was skipped — and on retry the existing-identity branch never reached the safety-net code, leaving the user permanently stranded with zero roles. Extracts _ensure_default_role(storage, user_id, desired_role_ids=None) helper. Calls it on BOTH the new-user and existing-identity paths so a user stranded by a transient failure recovers on next login. desired_role_ids is a hint that lets the helper skip list_user_roles when claim-driven mapping populated at least one role; the new-user path was already paying that query, the existing-identity path now pays it only when claim mapping returned an empty desired set. Documents the admin-strip behavior in the helper docstring: stripping all roles from an OIDC user no longer locks them out, since the next login will re-grant builtin-viewer (assigned_by='oidc-default'). The documented way to deny an OIDC user is to unlink their OIDC identity via the admin endpoint, not to strip roles. The pre-fix behavior (stripped user actually locked out) was the bug. The 'oidc-default' vs 'oidc' assigned_by distinction is preserved: apply_role_mapping's revocation lane only touches 'oidc' rows, so the safety-net role survives every subsequent login regardless of claims. Six new tests cover both paths, the hint short-circuit, the list_user_roles fallback, the missing-builtin-viewer no-op, and the self-heal regression case for already-stranded users. |
||
|
Patrick Buckley
|
5c11ab985f |
test(oidc): close coverage gaps + tighten fetch_jwks shape check (q-5, q-8)
q-5: _derive_username's UUID-retry tier (oidc.py:923-933) was untested.
After perf-6 collapsed tier-2 to a single find_existing_usernames call,
the only remaining tail was the 3-attempt UUID-retry loop and the final
raise. New TestDeriveUsername class covers:
- falls into UUID retry when all 10 suffix candidates are taken
- UUID retry succeeds on the second attempt after one collision
- UUID retry exhausted -> raises OIDCError
q-8: filled the unit-level coverage holes the multi-stage review flagged:
- test_validate_id_token_retry_after_kid_rotation — direct unit test of
the OIDCKeyNotFoundError path with real RS256 keys + JWKS rotation
(previously only exercised end-to-end through the handler).
- test_callback_uses_pending_audience_not_handler_audience — pins down
the bug-3 fix by decoding the issued JWT cookie and asserting aud
matches the audience stored at /authorize time, not the handler param.
- test_apply_role_mapping_int_claim / _dict_claim — exercises the
else: values = [str(claim_value)] branch for non-string non-list
claim shapes.
- TestFetchJWKS — non-200 status, non-dict body, dict-missing-keys,
keys-not-list, transport network error.
- TestExchangeCode network/4xx/5xx error tests (the non-dict-body case
already shipped in batch 5).
Also a small production hardening that fell out of writing the
TestFetchJWKS::test_fetch_jwks_non_dict_body_raises test: fetch_jwks now
guards isinstance(result, dict) before result.get("keys"), matching the
shape-check pattern that discover_oidc and exchange_code already use.
A list/null body now surfaces as OIDCError("...not a JSON object") rather
than AttributeError leaking up to the lifespan.
|
||
|
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.
|
||
|
Patrick Buckley
|
39a647f39c |
perf(oidc): batch perf hardening (perf-1..8)
Eight independent perf wins on the OIDC hot path: perf-1: list_users() full-scan setup-gate replaced with new count_users() on both authorize and callback. Saves a full users-table fetch per login. perf-2: handle_oidc_callback's sync DB chain wrapped in asyncio.to_thread for cleanup, pop_oidc_pending_state, count_users, and provision_oidc_user. handle_oidc_authorize gets the same treatment for count_users and create_oidc_pending_state. Event loop no longer blocks for the full callback duration on Postgres deployments. perf-3: apply_role_mapping N+1 collapsed via new replace_oidc_roles storage method. One transaction handles the diff + insert + delete instead of 2N+1 commits per login. Returns (added, removed) so the caller can still emit per-role audit logs. The diff respects the documented invariant "manually-assigned roles are never touched" — desired_role_ids is filtered against rows where assigned_by != 'oidc' before computing added/removed. This prevents a PK conflict (Postgres lockout) or silent OR-IGNORE no-op (SQLite lying return) when admin-ui or oidc-default already holds the same role_id. perf-4: provision_oidc_user no longer re-queries list_user_roles after apply_role_mapping. The new-user builtin-viewer fallback is gated on desired_role_ids being empty, which is information apply_role_mapping already returned. perf-5: JWKS refetch dedup via asyncio.Lock on app.state. Both lazy-fetch (cold-start recovery) and rotation paths share the same lock with a double-check pattern: re-resolve kid against the current cache before issuing a new GET. N concurrent callbacks during rotation now produce at most 1 fetch. perf-6: _derive_username's 9-suffix loop collapsed via new find_existing_usernames(candidates) -> set query. Worst case drops from 13 sequential queries to 1 + up-to-3 UUID-retry queries. perf-7: cleanup_expired_oidc_states gated to once-per-60s per process via app.state.oidc_last_cleanup_monotonic. The pop already deletes the consumed row; the bulk cleanup is only relevant for abandoned authorize flows, so frequency was overkill. perf-8: Long-lived httpx.AsyncClient stashed on app.state.oidc_http_client by initialize_oidc_state. discover_oidc/fetch_jwks/exchange_code accept an optional client= kwarg; when set, skip the per-call AsyncClient context-manager. New close_oidc_state lifespan teardown closes it. Tests pass client=None to keep the transient-client legacy path. New storage methods (sqlite + postgresql): - count_users() -> int - find_existing_usernames(candidates) -> set[str] - replace_oidc_roles(user_id, desired) -> (added, removed) |
||
|
Patrick Buckley
|
0af3adae1d |
fix(oidc): callback robustness — typed exceptions, shape checks, log sanitize, JS race (bug-4, bug-5, bug-6, sec-4)
Four small hardening fixes on the OIDC callback hot path: bug-4: JWKS rotation retry was matching the substring 'not found in JWKS' inside an OIDCError message. A future rephrasing would silently break key rotation. Adds OIDCKeyNotFoundError(OIDCError); validate_id_token raises the subclass at the kid-not-found site; handle_oidc_callback catches it explicitly. Other 'not found' errors in validate_id_token remain as plain OIDCError. bug-5: tokens['id_token'] raised KeyError if the IdP returned 200 without id_token. exchange_code now rejects non-dict response bodies; the callback validates id_token shape (must be non-empty str) before passing to validate_id_token. Both raise OIDCError, surfaced as the standard 'Authentication failed' redirect. bug-6: shared_static/auth.js — the OIDC error display raced showLogin's /v1/api/auth/status fetch via a 300ms setTimeout. showLogin now takes an optional oidcError parameter and paints it after _switchMode clears the error, in both the success and catch branches of the fetch. sec-4: oidc.py exchange_code's non-200 OIDCError interpolated up to 500 bytes of attacker-controlled IdP body, which then went to log.warning via 'OIDC callback failed: %s'. CRLF in resp.text could forge log lines. New _sanitize_log_text helper escapes control chars via unicode_escape and caps at the rendered length. |
||
|
Patrick Buckley
|
11618bb1d7 |
fix(oidc): atomic user + identity provisioning to prevent orphan rows (bug-1)
provision_oidc_user previously called create_user (INSERT OR IGNORE on SQLite — silent no-op on UNIQUE conflict), then create_oidc_identity (also INSERT OR IGNORE), then apply_role_mapping which writes user_role rows for the supposedly-new user_id. On a username TOCTOU race or concurrent (issuer, sub) double-create, both inserts no-opped but user_role rows were already written — leaving orphan rows pointing at a user_id that doesn't exist. PostgreSQL's create_user raised IntegrityError instead of silently no-opping so it produced a misleading 'Authentication failed' error without orphans, but the user-facing UX was equally poor. Adds StorageConflictError to the storage protocol and create_oidc_user that does both inserts in one transaction. Username collision and (issuer, subject) collision both raise StorageConflictError, mapped to OIDCError by provision_oidc_user. Crucially the new code does not silently bind a colliding-username new identity to the existing user — that would be an account-takeover vector. It raises. SQLite uses BEGIN IMMEDIATE inside the try block so lock-contention errors surface as StorageConflictError instead of leaking the raw sqlalchemy OperationalError. PostgreSQL relies on SQLAlchemy 2.x begin-on-demand semantics; the explicit conn.commit()/rollback() in the catch block is the only materialization path. Discrimination on PG uses exc.orig.diag.constraint_name with message-substring fallback. |
||
|
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.
|
||
|
Patrick Buckley
|
6f9e140a41 |
refactor(oidc): unify server+console lifespan via initialize_oidc_state (q-2, bug-2)
The OIDC discovery + JWKS prefetch block was duplicated byte-for-byte between turnstone/server.py and turnstone/console/server.py. The bare except branch in that block also left app.state.oidc_config unchanged on unexpected exceptions — leaving the runtime with enabled=True and empty endpoints, producing malformed authorize URLs. Extracts initialize_oidc_state(app_state) into turnstone/core/oidc.py which guarantees a coherent post-condition on every code path: - discovery exception -> oidc_config replaced with enabled=False, jwks_data=None - discovery returns enabled=False -> jwks_data=None - JWKS prefetch fails -> jwks_data=None but enabled=True preserved (the callback's lazy-fetch retry path remains the recovery) - success -> oidc_config + jwks_data both populated Also hardens discover_oidc against non-dict discovery responses (list/null/string/int) — previously these raised AttributeError out of doc.get and propagated past the lifespan's bare except. server.py and console/server.py lifespan blocks collapse to a single await initialize_oidc_state(app.state) call. |
||
|
Patrick Buckley
|
0df7dc026b |
fix(oidc): SSRF + plaintext credential exfil via discovery doc (sec-1, sec-3)
OIDC discovery-document endpoints (token_endpoint, jwks_uri, userinfo_endpoint) were stored verbatim in OIDCConfig and later passed to httpx without revalidation. Only the issuer URL was checked. A hostile or compromised IdP could return token_endpoint pointing to an internal IP (169.254.169.254, 10.0.0.0/8, etc.) and Turnstone would POST the client_secret there. Extracts the existing scheme/userinfo/SSRF check into _validate_url_no_ssrf, adds validate_discovered_endpoint that runs the same checks plus an issuer-binding check, and wires it into discover_oidc for authorization_endpoint, token_endpoint, jwks_uri, and userinfo_endpoint (when present). Issuer binding accepts: - Same (scheme, hostname, effective port) as the issuer. - A hostname in _KNOWN_TRUSTED_ENDPOINT_HOSTS for the issuer (Google's multi-origin discovery is in the allow-map by default). - A hostname in OIDCConfig.trusted_endpoint_hosts, settable via TURNSTONE_OIDC_TRUSTED_ENDPOINT_HOSTS env var or config.toml, for IdPs not in the static map. Effective port comparison treats https://host and https://host:443 as the same origin (urllib.parse.urlparse leaves the explicit form's port as 443 and the implicit form's as None). 24 new tests cover the validator, the Google known-hosts path, the operator allow-list, default-port equivalence, foreign-host rejection, private-IP rejection, embedded credentials, and DNS rotation between issuer check and endpoint use. |
||
|
Patrick Buckley
|
5d6d4436fb |
feat(console): inline node picker replaces back-to-console banner (#475)
* feat(console): inline node picker replaces back-to-console banner Drops the 32px banner the console proxy used to inject above proxied server-UI pages and replaces it with an inline node-id pill in the existing #ui-header. Click the pill to open a dropdown that lists healthy nodes (health dot, ws count, reachable/degraded/unreachable text) plus a top-row link back to the console. Reuses the .ws-tab-dropdown shell from ui/static/style.css for animation, shadow, theme override, and item layout, so the picker visually matches the workstream-tab chevron menu it sits next to. Keyboard nav (ArrowDown/Up/Home/End/Tab/Escape) mirrors the chevron menu's handler with cross-reference comments at both sites. Lazy-fetches /v1/api/cluster/nodes against the console origin (bypassing the prefix shim) on first open. Reclaims 32px of vertical space, consolidates three separate "you're on node X via console" indicators into one, and turns the wayfinding chrome into a real cluster-nav primitive. * fix(console): address Copilot review on node picker - Request /v1/api/cluster/nodes?limit=1000 (collector's hard cap) instead of relying on the default 100 — clusters with more than 100 nodes were silently dropping rows from the picker. - Hand off focus to the first menu item after the async fetch resolves: openMenu()'s deferred focus hook ran while only the skeleton was in the DOM, so first-open keyboard users were stranded on the trigger until they pressed an arrow key. - Tab now closes the menu without preventDefault, so focus moves to the next focusable element on the first press (ARIA APG menu pattern). Escape still preventDefault + returns to the pill. - Cap pill max-width at 240px and ellipsize the id span; node ids are accepted up to 256 chars upstream and could otherwise push the title and right-side controls off the appbar. Pill carries a title attribute so the full id is still legible on hover. |
||
|
Patrick Buckley
|
11f0813329 |
fix(session): properly inject queued user messages mid-loop (#474)
* fix(session): properly inject queued user messages mid-loop
Two queued-user-message bugs in ``ChatSession.send()``.
**Mid-tool-call: ``Unexpected role 'tool' after role 'user'`` on Mistral.**
The ``supports_tool_advisories`` capability flag (default False for
unknown openai-compatible models) routed cap-off providers down a
short-circuit branch in ``_collect_advisories`` that called
``_flush_queued_messages`` directly. That appended a ``user`` turn
between ``assistant(tool_calls)`` and ``tool``, which mistral-common's
``_validate_message_order`` rejects with a 400.
Drop the flag. All providers now run the unified path: queued user
messages become ``UserInterjection`` advisories that ride inside the
tool result envelope via ``wrap_tool_result``, splicing
``<system-reminder>`` text into the tool message's content. Role
sequence stays ``assistant → tool``. Live-confirmed on Mistral
medium and Qwen3 — both correctly distinguish system-reminder from
tool stdout in their reasoning.
**Mid-stream: queued message orphaned until next user send.**
After a no-tool assistant turn, ``_flush_queued_messages`` would
append the queued user message to history and the loop would
``break``, leaving the message at the tail of history with no
model response. Visible as "two sends to get one reply".
``_flush_queued_messages`` now returns ``bool``. The no-tool branch
``continue``s on drain instead of ``break``ing, so the model gets a
turn over the extended history.
Tests:
- ``test_collect_advisories_drains_text_queued_messages_to_persistent``
pins the unified-path drain (text-only queue → ``UserInterjection``,
no separate user turn appended to ``self.messages``).
- ``test_send_continues_when_messages_queued_during_streaming`` pins
the loop-continue behavior (fails with 1 stream call pre-fix,
passes with 2 post-fix).
* fix(session,ui): reject queued attachments + paperclip busy state
Copilot pointed out that the attachment-bearing branch in
``_collect_advisories`` had the same role-ordering bug as the
text-only path that
|
||
|
Patrick Buckley
|
c339615e39 |
Bound search tool output against pathological inputs (#473)
* Bound search tool output against pathological inputs
Replaces the per-line truncation with a fully bounded pipeline so the
search tool can no longer overflow the LLM context — or OOM the parent —
on minified bundles, multi-GB JSONL records, or huge result sets.
Backend:
- Prefer ripgrep when on PATH; grep is the fallback. Detection is
cached via functools.cache.
- ripgrep flags do most of the bounding natively: --max-columns 1024
+ --max-columns-preview, --max-filesize 10M, --max-count 100,
--no-config, --no-messages, plus negative globs for the same
noisy directories grep has been excluding.
- ripgrep added to the Dockerfile.
Streaming subprocess (_search_capture):
- subprocess.Popen with a streaming, byte-capped stdout read (4 MB).
Defends against single-line files (training data, minified bundles)
that would have OOM'd the previous subprocess.run capture.
- threading.Timer watchdog enforces tool_timeout even when the
pipe read is blocked in the kernel — proc.wait(timeout=…) alone
was insufficient because the read sat ahead of it.
- Stderr drained in a daemon thread to avoid pipe-deadlock when the
child writes to stderr while we're still reading stdout. Cap on
captured stderr keeps a hostile child from growing the buffer.
Tier-based formatter (_format_search_results):
- Tier 1: full path:line:content output, stream-emitted with a
running-cost short-circuit so we never materialize past the budget.
- Tier 2: K samples per file with overflow notes; K is computed
analytically from budget / file_count / avg-line-length so we hit
the right ladder rung in a single pass.
- Tier 3: per-file counts only, also budget-bounded with a tail line
reporting the omitted files. Sorted by descending count.
- Total output budget (32 KB) is well under tool_truncation, so the
head+tail _truncate_output strategy never silently drops middle
files in a search result.
Argument injection fix:
- The ripgrep arg list was missing the `--` separator that the grep
branch already had. With auto_approve on the search tool, that was
exploitable: path='--pre=COMMAND' would have made ripgrep run the
script as a per-file preprocessor and surface its stdout. Added
`--` and a regression test.
State-machine cleanup in _exec_search:
- rc < 0 (signal-killed by something other than us) now surfaces a
dedicated 'killed by signal N' message instead of being parsed as
success.
- capped + zero parsed records (e.g. one multi-MB line with no \n)
now returns a dedicated byte-cap message instead of the malformed-
output message that previously masked the real cause.
- _report_tool_result descriptions now match the returned payload
(no more 'no matches' tag on a 'malformed' payload).
Defence-in-depth on env scrub:
- RIPGREP_CONFIG_PATH, GIT_CONFIG, GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM
added to _EXPLICIT_SCRUB. We pass --no-config on the rg CLI today,
but if a future caller forgets the flag, an attacker who can set
one of these env vars could plant a config containing --pre=… and
recreate the same RCE shape.
Tests:
- TestSearchLineTruncation rewritten to mock _search_capture instead
of subprocess.run (the previous tests passed ChatSession kwargs
that no longer satisfy the constructor).
- TestSearchBackendSelection covers rg/grep detection and arg
construction, including the --pre flag-injection regression.
- TestSearchOutputBudget exercises Tier 1/2/3 directly.
- TestSearchCaptureStreaming spawns real Python subprocess writers
to exercise the byte-cap trim, mega-line-no-newline edge case, the
watchdog timeout when the child writes nothing, and the stderr
drain under load.
- test_env_scrub picks up the new tool-config keys.
* Address Copilot review on #473
- Budget the Tier 2/3 header up front so the formatter's emission stays
strictly within _SEARCH_OUTPUT_BUDGET. Previously the fit checks only
counted body bytes, letting the final string overflow by ~120 chars
(header + separator) and triggering _truncate_output's head+tail
dropout — exactly the shape this code was trying to avoid.
- Restore the (5, 3, 1) ladder in Tier 2: the analytical K from perf-2
is kept as a starting estimate, but if that K's actual emission
doesn't fit (the estimate ignores the header and overweights shared-
path compression) we step down through the ladder before falling
through to Tier 3. The previous one-shot K could collapse to counts-
only when 3/file or 1/file would have fit.
- Only normalise rc to 0 in the capped-output path when rc < 0 (our
SIGKILL). There's a narrow race where the child can exit naturally
between our read and our kill; preserving a non-negative rc means
rg's rc=2 ('matches found but some files had errors') no longer
silently turns into a clean success when the byte cap also fires.
- Clarify _MAX_SEARCH_LINE_LENGTH doc: the cap applies to the content
portion (after path:lineno:), not the whole emitted line.
- Add explanatory comments on the two intentional `except Exception:
pass` blocks in _search_capture (stderr drain, pipe close in the
cleanup finally) so static analysis and future readers can see the
silence is deliberate.
- Tighten the budget tests: now assert strict `<= _SEARCH_OUTPUT_BUDGET`
instead of the +512-char slack that was masking the header overflow.
- New regression tests:
- Tier 2 ladder step-down (K=5 over budget, K=3 fits, no Tier 3 fall-through)
- capped + rc=2 surfaces stderr instead of being normalised to success
- capped + rc<0 (our SIGKILL) flows through as a partial-result success
* chore(search): post-review cleanup
Follow-up to the Copilot-review fixes in
|
||
|
renovate[bot]
|
171c8e438f | chore(deps): lock file maintenance | ||
|
renovate[bot]
|
b9b723ba93 | chore(deps): update github actions | ||
|
Patrick Buckley
|
32fd8f29c7 |
feat(providers): api_surface toggle + mistral medium reasoning fix (#469)
* feat(providers): api_surface toggle + mistral medium reasoning fix
Mistral medium open-weights served by vLLM expects reasoning_effort via
the Responses API (`reasoning.effort`), not as a `chat_template_kwargs`
entry on Chat Completions. The session was unconditionally injecting
`{"reasoning_effort": ...}` into `chat_template_kwargs` for every
openai-compatible request, which corrupted the prompt rendering for any
backend whose chat template didn't consume that key (Mistral medium,
Mistral cloud, Groq, OpenRouter).
Changes:
- Add `api_surface` ("chat" | "responses") to `ModelConfig.server_compat`
and thread it through `create_provider` / `model_registry.get_provider`.
`openai-compatible` defaults to Chat Completions; operators can flip
individual aliases to Responses for endpoints that support it.
- New `vllm-mistral-medium` profile that pre-fills api_surface=responses
on Detect for known Mistral medium model ids.
- Drop the unconditional `reasoning_effort` injection into
`chat_template_kwargs`. Operators running gpt-oss-style local
templates that consume `reasoning_effort` from the chat template now
opt in via `server_compat.extra_body.chat_template_kwargs`.
- New "API Surface" select in the Models admin tab; allowlist-validated
server-side at create/update time; pre-filled by Detect via the
profile suggestion.
- Evict the cached provider singleton in `ModelRegistry.reload()` when
api_surface changes (previously only cfg.provider triggered eviction).
- Fix `_run_agent` fallback path to inherit the session's primary alias
for capability and server_compat resolution; previously the fallback
passed `alias=None`, which silently dropped per-model caps on the
agent path.
Tests: 5117 passed (-m "not live"); ruff + mypy clean.
* fix(providers): don't auto-suggest Responses for Mistral medium
vLLM's Responses API surface for Mistral medium open-weights doesn't
wire up the Mistral tool-call parser as of vLLM 0.x — tool calls leak
into the response as ``[TOOL_CALLS]<name>{...}`` text instead of
structured tool_calls. Chat Completions on the same engine handles
tools cleanly via ``--tool-call-parser mistral``, and reasoning can be
turned on via the vLLM CLI ``--reasoning-parser`` flag.
Drop the auto-suggest mapping so Detect falls back to the generic
``vllm`` profile. Keep the ``vllm-mistral-medium`` profile definition
in place so an operator who specifically wants per-request effort and
accepts the tool-calling limitation can still pick "Responses API"
manually in the admin UI.
* fix(providers): address Copilot review on PR #469
- providers/__init__.py: drop the redundant *_responses_provider /
*_chat_provider names; have create_provider use _openai_provider and
_openai_compat_provider directly so they're not flagged as unused
globals.
- console/server.py: tighten _validate_api_surface to a strict equality
match against the canonical {"chat", "responses"} set. The previous
strip().lower() membership check accepted ' Responses '/'CHAT' but
stored the raw string verbatim, which then failed to round-trip
through the admin <select>.
- console/static/admin.js: gate the entire server_compat block (server
type, api_surface, extra_body) on provider == "openai-compatible" at
save time so toggling provider away can't leave a stale hidden surface
selection in the persisted capabilities JSON.
- tests/test_session.py: splat the bad kwarg via **dict so CodeQL no
longer flags the call as a wrong-name keyword (the point of the test
is the runtime contract, not the static type).
- tests/test_admin_model_registry_refresh.py: add endpoint-level tests
for the api_surface validation on both create and update — covers the
bogus-value rejection, non-canonical-string rejection, and the happy
path persisting through to the refreshed registry.
|
||
|
Patrick Buckley
|
89b6b299f7 |
fix(memory): query-aware candidate selection + OR-of-terms search (#468)
* fix(memory): query-aware candidate selection + OR-of-terms search The system-message memory composition path used a recency-ordered candidate set (`_list_visible_memories(limit=fetch_limit)`). On deployments with more than `fetch_limit` (default 50) visible memories, BM25 only ever ranked the 50 most-recently-touched memories — a relevant memory written months ago was silently invisible regardless of how well it matched the recent context. Multi-word search at the SQL layer used AND-of-terms, killing recall on any multi-word query without an exact field overlap. ## Functional changes - `_init_system_messages` (`turnstone/core/session.py`): extract recent context first, then `_search_visible_memories(context)` to pull query-aware candidates. Search hits below `fetch_limit` union with the recency list (deduped by memory_id) so the BM25 candidate pool is always a SUPERSET of the prior recency-only pool — even on noisy queries where the cap fills with stopwords, the recency-50 the original bug surfaced still reaches BM25. Empty context skips search entirely. Candidate-selection logic extracted into `_select_memory_candidates`. - `search_structured_memories` (PostgreSQL + SQLite): per-term clauses join with OR instead of AND. A row matches if ANY term matches ANY of name/description/content. Downstream BM25 narrows back down by relevance. ## Perf hardening - Collapse the 1-3 fanned scope queries into a single SQL. New backend methods `list_visible_structured_memories` / `search_visible_structured_memories` union the visibility scopes into one WHERE OR-group, so a composition rebuild now hits the DB at most twice (search + recency) instead of up to six times. - Cap and normalize search terms. Composition can hand a multi-KB pasted message to ILIKE-based search; without a cap, every distinct token would emit one unindexable predicate per scope-fanned query. `normalize_search_terms` (`storage/_utils.py`) de-dupes case-insensitively, drops <2-char tokens, and hard-caps at 16. - Per-turn search cache. `_init_system_messages` fires from many call sites within one turn (state transitions, MCP refresh, tool results) and the recent-context query is identical across them. Session-instance cache keyed by (query, mem_type, limit) absorbs the duplicates; invalidated in `_append_user_turn` and after memory save/delete tool actions. - Stable secondary sort by `memory_id`. `updated` is second-precision and `touch_structured_memories` can land a batch on identical timestamps; without a tie-breaker SQL returns rows in implementation-defined order, BM25 input shuffles, and the LLM-side prompt cache misses across calls. All four backend ORDER BYs now break ties on `memory_id ASC`. ## Quality cleanups - Coalesce `memory.search.term_count` + `memory.search.zero_results` into a single `memory.search` log carrying both `term_count` and `result_count`. - New `memory.composition` log: source / candidates / injected. - Promote a shared `make_chat_session` factory to `tests/_helpers.py`. - Rename SQL builder local `extra` -> `scope_filters` for clarity. - Add docstrings on `search_structured_memories` so the AND->OR flip survives future readers. ## Tests Adds 20 tests across `tests/test_structured_memory.py`, `tests/test_structured_memory_storage.py`, and `tests/test_memory_relevance.py`: recency-ceiling regression, empty-query fallback, sparse-match union, recency-preserved-when- search-returns-noise (locks in the pool-superset invariant), OR-of-terms on both backends, scope filtering preserved, search-facade multi-word behavior, term-cap normalization, the new visible-scope helpers (list + search + empty-scopes guard), coord-scope composition isolation, end-to-end `memory(action='search')` tool execution, per-turn cache hit + invalidation, and stable ordering under tied `updated` timestamps. Memory test sweep: 102/102. Broader regression (session, storage, coordinator, load_skill): 411/411. * fix(memory): address Copilot review on PR #468 Three follow-ups from Copilot's inline review: 1. SUPERSET invariant violation (Copilot, session.py:5510). `(search_hits + extra)[:fetch_limit]` capped the union back down to fetch_limit, evicting the recency tail when search added distinct hits. Recency tail is exactly where ancient-but-recently-touched memories live — the recall this PR is supposed to improve — so tail eviction recreated the bug for the narrow case where a query term fell off the 16-cap and the matching memory sat in recency[40-49]. Drop the cap; both halves are already SQL-capped at fetch_limit, so the union is at most 2 × fetch_limit (~100 with defaults). BM25 over 100 candidates in pure Python is sub-ms; irrelevant recency fillers get score=0 and don't pollute ranking. Updates the docstring to actually be honest about the invariant. Adds `test_recency_tail_preserved_when_search_adds_distinct_hits` that locks the behavior in: 5 search hits + 10 recency = 15-item pool, every recency item present, source="union". 2. Unbounded `query.split()` in normalize_search_terms (Copilot, _utils.py:74). `str.split()` allocates the full token list before the cap-after-16 break, so a 100KB pasted query did MB of throwaway work even though only 16 tokens entered SQL. Switch to `re.finditer(r'\S+', query)` — streaming iterator, stops scanning at the first 16 normalized terms regardless of input size. 3. Misleading + unbounded log term_count (Copilot, session.py:8571). `len(item["query"].split())` had two problems: same unbounded split as #2, and the value reported the raw input token count rather than the normalized term count that actually hit the SQL WHERE clause — misleading metric for an operator trying to understand storage-side behavior. Switch to `len(normalize_search_terms(item["query"]))` — accurate count, and bounded for free via #2. Refuted: github-code-quality flagged `...` bodies in the new Protocol methods as "statement has no effect." False positive — `...` is the canonical Protocol body convention, used 213 other times in the same file. Memory test sweep: 103/103. Broader regression: 411/411. |
||
|
Patrick Buckley
|
9c9333ebd4 |
fix(tests): isolate metrics-singleton swaps so they don't leak across files
CI failure on main: test_publish_records_metric_outcome saw an empty
calls list — its monkeypatch was patching a different metrics
instance from the one `_publish_models_metadata` reads.
Two changes:
- test_close_reason_persistence.py: replace the bare
`srv_mod._metrics = MetricsCollector()` assignment in `_make_app`
with an autouse `monkeypatch.setattr(srv_mod, "_metrics", ...)`
fixture so the test's metrics swap auto-restores. Other test
files (test_auth.py, test_server_attachments_endpoints.py) carry
the same anti-pattern; left for a follow-up since they're not on
the critical path here.
- test_server_node_models_metadata.py: switch the publish-helper
metric test to a string-form `monkeypatch.setattr("turnstone.
server._metrics", FakeMetrics())` so it replaces whatever binding
the live module currently holds, regardless of what other tests
did to it. Robust against future leaks of the same shape.
|
||
|
Patrick Buckley
|
47cf6dea24 |
feat(coord): expose healthy model aliases per node on list_nodes (#466)
* feat(coord): expose healthy model aliases per node on list_nodes
Surfaces a `model_aliases` field on each `list_nodes` row so a
coordinator can discover which model aliases each cluster node will
accept on `spawn_workstream(model=...)` without an HTTP fan-out.
Each server projects its registry into a `models` entry on
`node_metadata` (`{alias, provider, healthy}` per alias) at lifespan
startup, on every 30s heartbeat tick, and after `internal_model_reload`.
The publish helper short-circuits on a payload-equality cache so a
stable cluster doesn't pay UPSERT churn — exposed via the new
`turnstone_node_models_publish_total{outcome="written|skipped"}`
Prometheus counter so operators can graph cache hit-rate.
Coord client filters the per-alias rows to healthy aliases only and
drops the provider-side model identifier (`cfg.model`) — coords kept
reaching for it when they should pass the local alias.
* fix(coord): address Copilot+CodeQL feedback on list_nodes models work
- internal_model_reload: reuse a single get_storage() local across the
registry load and the metadata publish (Copilot:3047)
- _collect_node_models_metadata: iterate sorted aliases so two
structurally identical registries built in different insertion orders
serialize to the same JSON — directly improves the publish-cache hit
rate exposed via turnstone_node_models_publish_total (Copilot:3105)
- tests: drop mixed turnstone.server import style flagged by CodeQL —
hoist _metrics into the from-import block, and use sys.modules in
the shutdown-race regression test instead of `import as srv`
|
||
|
Patrick Buckley
|
3308e3645a |
fix(core): scope rehydrate fallback to manager, fix resume orphan
Address Copilot feedback on PR #465: 1. The has_alias fallback in both session_factories silently rewrote any unknown caller-supplied alias to the default, including on the fresh-create path where the create handler maps the factory's ValueError to a 503 with operator-friendly text. A typo in body.model would now silently start a workstream on the default instead of telling the caller their requested model could not be resolved. Move the fallback out of the factories: each factory raises again on unknown aliases, and SessionManager filters stale aliases out of the rehydrate path via a new ``model_validator`` constructor kwarg (production wiring passes ``registry.has_alias`` on both interactive and coordinator). 2. ChatSession.resume()'s elif branch flipped self.model to the persisted model name even when the alias was unresolvable, leaving the session paired with the constructor's default provider/client but a removed model name — a broken state whose next API call fails. Drop the model copy: keep the constructor's coherent default (provider + model + capabilities) and just log the unreachable saved values so the missing alias is auditable. Tests: - Move stale-alias coverage from the factory level into SessionManager (tests/test_session_manager.py): validator drops stale aliases before reaching build_session; live aliases pass through unchanged. - tests/test_sessions.py renamed test_resume_restores_model → test_resume_keeps_defaults_when_alias_unresolvable to match the new contract. |
||
|
Patrick Buckley
|
0a43bed3d5 |
fix(core): preserve workstream model + config on rehydrate
SessionManager.open() was calling build_session(ws) without a model arg on the rehydrate path. The session_factory then resolved the *current* default alias, ChatSession.__init__'s _save_config() (INSERT OR REPLACE per-key) clobbered the persisted workstream_config with those defaults, and the subsequent resume() "restored" what was now the default — silently resetting model_alias, model, temperature, reasoning_effort, max_tokens, skill, creative_mode, instructions, token_budget, and notify_on_complete on every reopen and every service restart, for both interactive and coordinator workstreams. Three layers: 1. SessionManager.open() now reads workstream_config via self._storage.load_workstream_config(ws_id) and threads the saved model_alias into build_session(ws, model=saved_alias). 2. ChatSession.__init__ now skips its initial _save_config() when a workstream_config row already exists for self._ws_id — protects every other persisted knob without having to plumb each one through the adapter signature, and catches any future construction path that forgets to thread model through build_session. 3. Both session_factories (server.py interactive, console session_factory.py coordinator) now treat an unknown caller- supplied alias the same as an unset alias: fall back to the runtime default rather than raising. Without this, a workstream pinned to an alias an operator has since removed from the registry would 500 on every reopen — defeating the "best effort restore, default if the original is gone" contract this fix is meant to deliver. Mirrors _effective_default_alias's existing has_alias guard against a stale ConfigStore default. |
||
|
Patrick Buckley
|
7db7f99dd8 |
fix(console): address Copilot feedback on Models → Roles sub-tab
Three changes from PR review: - Permission gating: hide the Roles sub-tab button when the user lacks ``admin.settings``. The sub-tab loads/saves through ``/v1/api/admin/settings``, so an admin with ``admin.models`` but no ``admin.settings`` would otherwise see a perpetual 403 loader. When Roles is the active sub-tab and the permission check fails, snap the panel back to Definitions so the user lands somewhere usable. - Drop the redundant ``/v1/api/admin/model-definitions`` fetch from ``loadAdminModelRoles``. Both entry points (initial Models-tab open + ``models_changed`` SSE refresh) flow through ``loadAdminModels`` first, which already populates ``_modelDefs`` + ``_modelDefaultAlias``; ``_saveModelRole`` doesn't touch model definitions, so the cached snapshot stays accurate when the save chains back here. Halves the per-render request count and removes a wasted round-trip on every cluster-wide model edit. - Add ``test_models_changed_event.py`` covering the SSE fanout the prior commit introduced: each model-definition CRUD endpoint emits exactly one ``models_changed``, settings PUT/DELETE only emit for keys in ``_MODEL_AFFECTING_SETTING_KEYS`` (parametrised over all eight), and unrelated settings (e.g. ``session.retention_days``) don't trigger spurious refreshes. The expected key set is pinned in the test so a stray addition to the allowlist doesn't silently bypass coverage. |
||
|
Patrick Buckley
|
35a1c50e60 |
feat(console): add plan_agent + task_agent to Models → Roles
Same shape as the coordinator/judge rows already there: alias dropdown + reasoning_effort dropdown sourced from the existing ``model.plan_alias`` / ``model.plan_effort`` and ``model.task_alias`` / ``model.task_effort`` settings. Adds the four keys to the SSE ``models_changed`` allowlist so changes from the Settings API also trigger a live dropdown refresh, and filters them out of the Settings tab so they only render in one place. |
||
|
Patrick Buckley
|
4afb192f94 |
feat(console): consolidate role-model settings + live-refresh dropdowns
Lifts judge and coordinator model assignments out of their respective
admin tabs and into a new Models → Roles sub-tab so role overrides live
next to the model definitions they reference. Forward-looking shape for
the upcoming perception.{audio,image,video} model settings — adding a
new role is one entry in the declarative MODEL_ROLES array.
Also drops the misleading "Coordinator subsystem not configured" home
banner. The session factory already falls back to the registry's
default model when coordinator.model_alias is unset, so the banner was
nagging on fresh installs where the system was actually working. The
related _probeCoordSubsystem / _homeCoordReady plumbing went with it.
Wires SSE-driven live refresh: the console now emits a models_changed
event when a model definition is created/updated/deleted/reloaded, or
when a model-affecting setting (model.default_alias, judge.model,
coordinator.model_alias, coordinator.reasoning_effort) changes.
Connected browsers refetch /v1/api/models on receipt so the home
composer's model dropdown and the Roles sub-tab stay accurate without
a manual reload — fixes the case where editing the underlying model
for an existing alias left the dropdown showing the old model id.
Companion cleanups:
- Renamed .judge-section-* CSS classes to .admin-subtab-* and shared
them with the Models sub-tab switcher (same a11y attrs, arrow-key
nav). Old names had no other callers.
- Filtered judge.model out of the Judge Settings sub-tab and
coordinator.model_alias / coordinator.reasoning_effort out of the
Settings tab — they live exclusively under Models → Roles now.
- Reworded the _require_coord_mgr 503 messages to point operators at
the Models tab instead of suggesting they set coordinator.model_alias.
|
||
|
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. |
||
|
Patrick Buckley
|
ae4fddfc5a |
fix(console): home composer attachments + coord chat user-message pills (#462)
* fix(console): home composer attachments + coord chat user-message pills Two parity gaps in the console's coordinator surface: - The embedded creator on the home page accepted only text — the paperclip / paste / drop pipeline that the in-coord composer and the interactive new-ws modal both expose was missing, so a user couldn't attach files at create time. Stage Files in memory (no ws_id yet) and ship them multipart on Start; the coord create endpoint already accepts multipart via create_supports_attachments=True. - User messages with attachments rendered as plain text on both live send and history replay — no chip cluster like the interactive pane. Added appendUserMessageWithAttachments and a structured userAttachments list built from _attachments_meta (preferred) or the multipart parts themselves, then rendered the same .msg-user-attach pill strip the interactive pane uses. Polish from a designer pass: - Pill background was --panel-2, equal to the .msg bubble background in both themes (border contrast ≈1.4:1, below WCAG 1.4.11). Switched to --panel so the pill sits on a different surface than the bubble. - Capped chip filename width inside the home composer (max-width 200px + ellipsis) so a long filename doesn't push the strip past the textarea. - aria-live="assertive" → "polite" on #home-coord-error; client-side validation isn't an interrupt-level event. - Reserved min-height on .home-composer-error and dropped the display: none/block toggling so validation messages no longer reflow the active-coordinators list below. * fix(console): address PR #462 review feedback - Block home-composer submit when files are staged but the task field is empty. Server's _coord_create_post_install short-circuits on an empty initial_message, so the multipart upload would create pending attachment rows that never reserve onto a turn — orphaned until the GC sweep. Fail in the browser instead. - Drop the redundant `part &&` guard in coordinator.js's history-replay multipart loop; the earlier `if (!part || ...) continue` already filtered. - Rewrite the home-mount .composer-chip-name CSS comment. shared/chat.css defines .composer-chip{,-size,-remove} but no .composer-chip-name rule — the span inherits the parent chip font with no width cap. - Add smoke-guard string assertions in test_coordinator_page.py for appendUserMessageWithAttachments and msg-user-attach so a future rename can't silently regress the attachment affordance. |
||
|
Patrick Buckley
|
eaabc79eb3 |
fix(replay): repair saved-workstream tool result rendering + extend audit-trail decoration (#461)
* fix(replay): repair saved-workstream tool result rendering + extend audit-trail decoration Loading a saved workstream silently dropped tool results and missed verdict / output-guard / truncation signals on replay. Root cause was in `Pane.prototype.replayHistory`: an assistant message carrying both content and tool_calls cleared the `lastToolBlock` anchor before the following tool-result iteration could attach. The fix reorders content to render before the tool block (matching live SSE order) and restructures the tool-result branch to anchor by `data-call-id` so multi-tool batches render `[hdr A][out A][hdr B][out B]` rather than bunching outputs at the bottom. Beyond the bug, replay now reaches near-parity with the live UX: - Persisted intent verdicts and output_assessments flow through both the SSE replay (`_build_history`) and the `/history` REST endpoint used by coord. Single shared helper module owns the wire shape. - Memory/recall calls persist instead of being filtered at storage time — full audit trail; UI dims them by default with hover-reveal so heavy memory usage doesn't crowd the narrative. - Truncation indicator surfaces as a sibling pill (consistent across interactive + coord) when a tool result hit the 2000-char cap. - `replayHistory` wraps DOM work in `aria-busy` so screen readers don't get a chatty announce-flood on long replays. - `_build_history`'s storage I/O moves off the event loop via a new `events_replay_prepare` async hook for the SSE path; other async callers wrap in `asyncio.to_thread`. Coord parity: - `/history` REST endpoint decorates tool_calls with verdict + output_assessment + truncation flag (was previously raw `load_messages` output). - Coord JS stamps `judge_verdict` / `heuristic_verdict` from history-loaded `tc.verdict` so the existing batch render paints the persisted pill, seeds the verdict cache to dedupe later live SSE events, and emits an inline `.coord-tool-row-warning` chip per call instead of a generic chat line. - Memory/recall dim rule mirrored on `.coord-tool-row[data-tool-name=...]`. * fix(replay): address PR #461 review feedback + raise tool-result storage cap Copilot review feedback: - Sibling-chain dim rule (memory/recall) now adds :focus-within alongside :hover for .tool-output / .media-embed / .output-warning / .tool-output-truncated — keyboard users tabbing into a faded subtree now get full opacity. - ``cfg.open_post_load`` is now invoked via ``await asyncio.to_thread`` so its sync ``_build_history`` call (storage I/O for verdict indexes + message reconstruction) doesn't block the event loop on every workstream open. Mirrors the SSE replay path that's already protected via ``events_replay_prepare``. - Replaced the hardcoded ``2000`` literal in server.py and session.py with ``TOOL_RESULT_STORAGE_CAP`` from the shared decoration module so the UI truncation-pill detection can't silently desync from the storage write side. While here: - Raised ``TOOL_RESULT_STORAGE_CAP`` from 2000 → 10000. A 2000-char clip routinely cut grep / file-read bodies mid-line, leaving the audit trail useless for retrospective debugging. FTS5 + row size grow proportionally; the per-tool upper bound is still bounded upstream by ``_truncate_output``'s context-budget clamp. - Updated the user-visible truncation-pill tooltip on both interactive and coord to reflect the new cap. - ``test_decorates_tool_calls_and_marks_truncated`` now references the constant instead of a literal so it stays correct on future cap changes. |
||
|
Patrick Buckley
|
29181687d3 |
refactor(coord): remove priority queue + queue depth indicator + broken CSS
Speculative reliability machinery from the Stage 3 push that turned out not to address any user-visible bug. The actual fixes (state / activity disjunction in handleChildState, bulk-fetch race fix in _fetch_live_block, push approve_request via cluster bus) are what resolved the wedged-row issues. Manual testing showed the per-tab SSE listener queue depth never climbed past single digits even when rows were stuck — overflow was never the cause. Removed - ``_CRITICAL_EVENT_TYPES`` + ``_put_with_priority`` helper. - Per-tab listener queue selective drop (back to plain ``contextlib.suppress(queue.Full)`` everywhere). - ``ClusterCollector._fanout`` reverts to the same. - WebUI ``_broadcast_intent_verdict`` / ``_broadcast_approval_resolved`` / ``_broadcast_approve_request`` revert to plain ``put_nowait``. - ``_queue_stats`` periodic SSE emit + frontend status-bar indicator + the supporting CSS rules. - Broken ``.approval-block`` ``transition: max-height`` / ``max-height: 80vh`` / ``overflow: hidden`` rules — the transition never fired (nothing toggled max-height) and ``overflow: hidden`` clipped long verdict reasoning. Layout-shift on auto-expand jumps again, which is preferable to clipped content (Copilot review). Tidied - ``_CollectorProtocol`` / ``_ManagerProtocol`` method bodies switch from ``...`` ellipsis to docstring-only bodies, silencing four CodeQL "statement has no effect" warnings without changing the Protocol contract. 5024 passed, ruff + mypy clean. |
||
|
Patrick Buckley
|
38a0d9c3b6 |
feat(coord): Stage 3 SessionManager Children primitive lift + cluster bus push paths
Lift the Children primitive out of CoordinatorAdapter into universal SessionManager core primitives, replace the fragile poll + state-event piggyback paths with first-class cluster bus event types for inline approval delivery, and clean up the resulting frontend reducer. Architecture - New `turnstone/core/children_registry.py` — universal parent → children + reverse-lookup primitive with atomic `add_child` (returns parent UI for race-free dispatch). Lifted from `CoordinatorAdapter`. - New `turnstone/core/child_source.py` — `ChildSource` Protocol with `SameNodeChildSource` (in-process via SessionManager state observer) and `ClusterChildSource` (cross-node via ClusterCollector listener). - `SessionManager._on_state_change` upgraded to multi-subscriber (`subscribe_to_state` / `unsubscribe_from_state`) under a dedicated lock; CLI consumer migrated. - `CoordinatorAdapter` shrunk: 731 → ~640 LOC. Children data lives in the registry; fan-out lives in ClusterChildSource. Backward-compat property facades dropped; tests updated to use the registry surface. Cluster bus event vocabulary - New event types `intent_verdict`, `approval_resolved`, `approve_request` flow through both `ClusterCollector._apply_delta` (translation from node SSE) and `emit_console_ws_*` (synthesis on console pseudo-node). - `CoordinatorAdapter._dispatch_child_event` re-emits as `child_ws_intent_verdict` / `child_ws_approval_resolved` / `child_ws_approve_request` on the parent coord's SSE stream. - New `_broadcast_intent_verdict` / `_broadcast_approval_resolved` / `_broadcast_approve_request` no-op hooks on `SessionUIBase`. WebUI pushes to the global queue; ConsoleCoordinatorUI pushes to the collector. `approve_tools` calls `_broadcast_approve_request` right after setting `_pending_approval` so the items reach the coord tree immediately, eliminating the bulk-fetch race. Cleanups - `pending_approval_detail` piggyback on `ws_state` / `cluster_state` removed end-to-end. Bulk fetch + explicit verdict / approve-request push are the canonical carriers. - Browser `_judgePollTick` 90-second poll loop deleted; push path is authoritative. - `urgent` flag on `scheduleLiveFetch` deleted (only caller was 409 retry; replaced with `invalidateLiveBadge` + standard schedule). - Console `_fetch_live_block` derives `pending_approval` from a disjunction (`activity_state="approval"` OR `state="attention"` OR detail present) so the bulk fetch can't return false during the state-transition race window. - Coord-side merge guard in `flushLiveFetches` no longer clobbered: `handleChildState` only stamps `sseUpdatedAt` when authoritatively clearing detail. - `child_locality` capability flag removed (was inert dead code). Reliability - Selective drop on listener queue overflow: critical event types (verdicts, approvals, ws_closed, child_ws_*) evict one oldest item to make room rather than dropping themselves on a full queue. Best-effort events (state ticks, content tokens, status, activity) drop as before. Applied to `SessionUIBase._enqueue`, `ClusterCollector._fanout`, and the `WebUI._global_queue` puts in the new broadcast hooks. - `_state_subscribers` snapshot under a dedicated lock so concurrent subscribe / unsubscribe during dispatch can't shift the iterator. UX / a11y - Loading placeholder in renderChildRow keeps row height stable while the bulk fetch is in-flight (sr-friendly aria-label). - Focus preservation across `_renderChildrenNow` (capture + restore by row + marker) and across targeted `_updateChildRow` swaps. - Layout-shift transition on the approval block max-height; respects `prefers-reduced-motion`. - Sidebar pending count: `(N children · M pending)`. - Risk pill `aria-label` spells out level + confidence for SR users. - Per-coord SSE listener queue depth surfaced in the status bar (`queue N/500`) with color escalation (warn at >50%, danger at >80%). Tests - 305+ test changes across 8 files. New unit tests for `ChildrenRegistry`, `ChildSource` (both impls + multi-subscriber observer), the new collector emit + apply_delta cases, the dispatch cases for new event types, the broadcast hook overrides on both WebUI and ConsoleCoordinatorUI, and the focus / placeholder / pending-count frontend assertions in `test_coordinator_page.py`. 5024 passed, ruff + mypy clean. |
||
|
renovate[bot]
|
92d4602da3 | chore(deps): update ghcr.io/astral-sh/uv docker tag to v0.11.8 | ||
|
Patrick Buckley
|
435289ce6c |
feat(console): multi-select delete UX for Saved Coordinators (#458)
* feat(console): multi-select delete UX for Saved Coordinators
Mirror the per-server "Saved Workstreams" multi-select delete onto the
console's "Saved Coordinators" section. Coordinator deletes go through
the existing routing proxy at POST /v1/api/route/workstreams/delete
(body-keyed by ws_id, since coordinators live on the node that owns
them) — no backend change required.
Pagination caps the visible page (and therefore the Select-All fan-out)
at 24. Without it, a Select-All on a busy cluster would pin the
console proxy pool with hundreds of parallel deletes through the
fan-out router. While in delete mode the saved-coordinators list is
frozen against SSE re-renders so visible cards don't shuffle out from
under the user's selections (drained on cancel / post-delete close).
Refactor: shared logic now lives in turnstone/shared_static/cards.{css,js}.
* .ws-delete-* CSS moved out of ui/static/style.css into the shared
sheet alongside .dashboard-card; the existing ui/static modal
markup picks up class hooks instead of id-scoped rules.
* createSavedCardsController() owns mode state, checkbox decoration,
toolbar wiring, focus trap, modal lifecycle, and batch fan-out.
Both ui/static (Saved Workstreams) and console/static (Saved
Coordinators) instantiate one controller; ui/static is now ~300
LOC lighter as a result.
* Internalises stale-selection prune across SSE re-renders, the
wsId->item lookup map (was O(selected x N)), and the aria-hidden
wrap on the toggle button's emoji glyph.
Designer review tightened the affordance:
* Modal close restores focus to the toggle button (was landing on
<body>) — WCAG 2.4.3.
* Modal [role="alert"] gets a red-chip treatment when populated,
stays invisible at rest via :not(:empty).
* Pagination consolidated onto the existing .pagination control
(terse "X / Y" label + arrow-glyph buttons) instead of a parallel
.coord-pagination treatment.
* Filled destructive buttons darkened to #dc2626 in dark theme so
the white label clears WCAG AA contrast (was 3.0:1 on --red).
Light theme keeps --red unchanged (5.9:1 already passes).
* Toolbar wraps below 700px viewport — Delete Selected drops to its
own full-width row underneath count + Cancel + Select All for
thumb-target separation.
* .ws-card-check:focus-visible outline + word-break on
.ws-delete-item for narrow-modal long aliases.
* fix(cards): address Copilot review feedback on PR #458
* closeModal focus restore now falls back to the section toggle button
(opts.buttonId) when prevFocus is hidden or detached. The post-delete
Close path runs cancel() before closeModal(), which puts the bar at
display:none — so the captured prevFocus (the bar's "Delete Selected"
button) is no longer focusable and focus would land on <body>,
defeating the WCAG 2.4.3 fix. Esc / Cancel paths still land on the
original focus owner because the bar stays visible in those flows.
* Saved Coordinators onClose drains _savedCoordsRetry before reloading.
Without it, SSE events that arrived during the delete-mode freeze
leave the retry flag true, so loadSavedCoordinators's .finally()
re-fires a second fetch immediately after the first resolves. Mirrors
the same idiom in cancelCoordDeleteMode.
|
||
|
Patrick Buckley
|
0e25bad94e |
fix(storage): address PR #457 review feedback
Three issues from the Copilot review on PR #457: 1. SQLite race in bulk_close_stale_orphans (Copilot): the SELECT-then- UPDATE flow doesn't re-apply the eligibility predicates on the UPDATE, so a row that gets touch_workstream-bumped (or set_state- transitioned) between the two statements would still be flipped to closed. Postgres dodges this via UPDATE...RETURNING (one atomic statement); SQLite needs the explicit re-application. Fix: rebuild the WHERE conditions list once, apply on both SELECT and UPDATE, then SELECT-back by ``state='closed' AND updated=now`` to get the accurate closed-id list. A row that became fresh between the two statements skips the UPDATE entirely. 2. SQLite IN-clause bind-parameter limit (Copilot): default 999 cap could be exceeded on a backlog reap (e.g. after a long outage). Chunked the candidate id list at 500 — same chunk size prune_workstreams (line 453) uses for the same reason. 3. Wall-clock-dependent test asserts (Copilot, two locations): the tests asserted ``updated > '2024-01-01T00:00:00'`` which is fragile on systems with skewed clocks or pre-2024 dates. Replaced with ``updated != stale_seed`` — captures the same intent (the value was bumped) without depending on wall-clock date. Two ``...``-as-no-op flags from github-code-quality were false positives — ``...`` is the standard Python idiom for Protocol method bodies and matches every other method in _protocol.py. No code change. |
||
|
Patrick Buckley
|
0debc5d061 |
fix(session_manager): scope orphan reaper by services.last_heartbeat
Replaces the ``node_id == self_node_id`` orphan-scoping heuristic from earlier on this branch with liveness-based scoping using ``services.last_heartbeat``. The heuristic was wrong for the post-#384 world: PR #384 (refactor: replace hash-ring rebalancer with rendezvous hashing) deleted the rebalancer that used to keep workstreams.node_id pointing at a live node. Without it, ``workstreams.node_id`` is now stamped at create time and never updated, so in containerized deployments with dynamic hostnames a dead pod's rows have ``node_id`` matching no surviving service — they'd accumulate forever under the old heuristic. services.last_heartbeat is the same primitive the rendezvous router uses for routing. Reusing it here keeps reap scoping aligned with routing: dead pods' rows fall out of the live set after the heartbeat window and become reapable; alive pods' rows stay protected as long as they heartbeat. Mechanics: - ``bulk_close_stale_orphans`` parameter renamed ``node_id: str | None`` → ``live_node_ids: list[str] | None``. The WHERE clause becomes ``(node_id IS NULL OR node_id NOT IN live_node_ids)``. ``None`` skips the filter entirely (single-process / tests / operator backfill). ``[]`` treats every row as unprotected. - ``SessionManager.close_idle`` pass 2 calls ``storage.list_services(self._service_type)`` to enumerate live peers, passes their service_ids as ``live_node_ids``. ``_service_type`` is derived from ``self.kind`` (INTERACTIVE→"server", COORDINATOR→"console") via a module-level mapping — no constructor param, so production wiring can't miswire the kind/service_type pairing. - list_services failure → pass 2 is skipped this tick (conservative; never reap when liveness state is unknown). Pass 1 still runs. - ``workstreams.node_id`` with NULL value is always eligible — defends against ANSI ``NULL NOT IN (...)`` evaluating to NULL (not TRUE) and silently protecting orphans forever. - Migration 048 simplified to ``(kind, updated)``; the new query's ``NOT IN (small list)`` predicate against an unbounded-cardinality column doesn't index well, so leading ``node_id`` would just add write cost. Tests cover the live-services protection (own/dead/null cases), the empty-peers reap-all case, the list_services-failure conservative fallback, both kind/service_type pairings (interactive→"server", coordinator→"console"), and the combined live_node_ids + exclude_ws_ids filter matrix. |
||
|
Patrick Buckley
|
58975ba02a |
perf(storage): partial composite index for the orphan reaper query
bulk_close_stale_orphans runs every min(300s, idle_timeout/4) on
every server and console process. Its WHERE shape is:
WHERE kind = ?
AND state IN ('idle','thinking','attention','running')
AND updated < ?
AND node_id = ? -- multi-node interactive only
At current scale the existing single-column indexes are sufficient —
idx_workstreams_state prunes to non-closed and the planner filters the
rest sequentially. At 100k+ rows that filter becomes a tablescan-
shaped cost.
A partial index covering only BULK_CLOSE_STATE_VALUES rows matches the
reaper's query exactly while staying tiny — closed rows (typically
95%+ of the table) and error rows are excluded, so the index is
roughly 5% the size a full multi-column index would be. Write
amplification only kicks in for transitions touching one of the four
covered states.
Column order (node_id, kind, updated): node_id is the most selective
filter for multi-node interactive (each server prunes to its own
node's rows), kind second so coord-only and interactive-only queries
within a node still get index-only scans, updated last so the range
comparison rides the trailing column.
Postgres uses CREATE INDEX CONCURRENTLY so the build is non-blocking
on a live system; SQLite has no concurrent concept and the table-
level write lock already serializes, so a plain CREATE INDEX is fine.
|
||
|
Patrick Buckley
|
bf17b0511e |
fix(console): periodic idle cleanup for the coordinator pool
The console's coord SessionManager had no idle thread — close_idle was never called for coordinator workstreams. This is the worse half of the lifecycle leak: the dashboard filters via the in-memory pool, so DB-only orphan coords were invisible. At empirical diagnosis, coord closure was 16% (10 closed / 64 total) vs interactive 63%. Adds _coord_idle_cleanup_thread mirroring turnstone/server.py's _idle_cleanup_thread but skipping the rate-limiter / global-queue arms the console doesn't have. Started from the lifespan when coord_mgr is constructed and server.workstream_idle_timeout > 0 (reuses the existing setting — same cadence works for both kinds). Initial sweep runs INSIDE the thread before the first sleep, not synchronously in the lifespan: cold-start orphans are reaped without blocking Starlette boot. Important because cold start with many DB orphans (the precise condition this code targets) is exactly when the UPDATE is most likely to be slow. Helper takes an optional stop_event parameter purely for tests — production callers pass None and the daemon runs for process lifetime. This avoids the SystemExit-from-stub + module-wide filterwarnings fragility a previous iteration relied on. Four tests: initial sweep runs before first sleep, ticks fire each loop, exceptions don't kill the thread, stop_event exits cleanly. |
||
|
Patrick Buckley
|
1405afe079 |
fix(session_manager): close DB-orphan workstreams in close_idle
Real bug: workstream rows accumulate in non-closed states (idle, thinking, attention, running) when their owning process restarts or crashes. Empirical diagnosis on a live deployment found ~60 stuck coord rows in DB invisible to the in-memory-keyed dashboard, plus 100+ interactive rows older than the 2h timeout (one stuck "thinking" for 2 weeks — impossible across a process restart). Root cause: close_idle iterates self._workstreams.values() — only the loaded subset. Anything left behind by a prior process incarnation sits in DB forever because nothing ever re-loads it. This commit gives close_idle a second pass. Pass 1 (existing, unchanged): close loaded IDLE rows whose ws.last_active (monotonic) is past timeout. IDLE-only so legitimately- attentive rows (waiting for user response) stay live. Pass 2 (new): bulk-close DB rows of this manager's kind whose updated is past the wall-clock cutoff and which aren't currently loaded. Closes the broader BULK_CLOSE_STATE_VALUES set — any matching row is by definition not loaded by any process and cannot be in a live interaction. Scoped by self._node_id so a sibling node can't reap rows we own (multi-node interactive correctness). No emit_closed — never-loaded rows have no SSE listeners expecting them. Lock invariant: pass 1 holds self._lock briefly to snapshot victims and pop them (existing behavior). Pass 2 holds self._lock briefly to snapshot the loaded keys, then releases before the DB UPDATE so a slow reaper query can't block create/get/set_state. Also fixes a same-process race in open(): the rehydrate path read DB, released the manager lock, then re-acquired to install — a concurrent pass 2 between the two acquisitions snapshots loaded keys without the in-flight ws_id, and could clobber its DB row to closed. open() now calls touch_workstream(ws_id) on rehydrate so the row's updated is fresh against any pass-2 cutoff. Pure timestamp write is safe against concurrent close() (close still wins on the state column). Three new tests cover the DB orphan pass (basic, exclude-loaded, kind filter) plus node_id scoping (own/foreign rows, None-skips-filter) and the open() rehydrate touch. |
||
|
Patrick Buckley
|
fff7840de7 |
fix(storage): add bulk_close_stale_orphans + touch_workstream primitives
Two new methods on the StorageBackend Protocol, with implementations on both Postgres (UPDATE ... RETURNING) and SQLite (SELECT-then-UPDATE in one transaction). No callers yet — wiring lands in subsequent commits. bulk_close_stale_orphans(kind, cutoff, exclude_ws_ids, node_id=None) flips rows in BULK_CLOSE_STATE_VALUES (idle/thinking/attention/running) to closed when their updated timestamp is lex-older than cutoff. The node_id filter scopes the reap to a single node's partition — required for multi-node interactive deployments where each node only has authority over its own workstreams.node_id rows. Excludes loaded ids so the in-memory pass owns those. touch_workstream(ws_id) bumps updated without changing state. Used by the open() rehydrate path to defend against the orphan reaper clobbering a freshly-loaded row whose DB updated is older than the cutoff. Pure timestamp write is safe against concurrent close() because close still wins on the state column. BULK_CLOSE_STATE_VALUES is centralized in workstream.py so the two backend implementations and FakeStorage all agree; if a new transient state is added to WorkstreamState, deciding whether it joins this set is part of the change rather than an after-the-fact audit across three files. Storage tests (run against both backends via the conftest fixture) cover the kind/state/cutoff/exclude/node_id matrix plus touch_workstream. |
||
|
Patrick Buckley
|
7a36ab95e4 |
fix(metacog): drop duplicate [repeat: tool()] info line
The themed ``tool_reminder`` bubble below the tool block already shows the metacog text, and the tool block immediately above it carries the tool name — so a separate gray ``[repeat: list_workstreams() called with same arguments]`` info line was just duplicate visual noise (operator-visible in the screenshot below the bubble). Drop the ``ui.on_info`` call inside ``_apply_post_execute_advisories`` that emitted the diagnostic line. Update the docstring to reflect that the bubble is the canonical signal. Rename ``test_emit_repeat_ui_line_on_streak_fire`` → ``test_no_legacy_repeat_info_line_on_streak_fire`` and invert the assertion. |
||
|
Patrick Buckley
|
b07d7f19b6 |
fix(cli): add on_user_reminder + on_tool_reminder to TerminalUI
CI typecheck failed because ``WorkstreamTerminalUI(TerminalUI)`` inherits from ``SessionUI`` (the Protocol), and the Protocol's ``on_user_reminder`` / ``on_tool_reminder`` declarations have empty bodies — mypy treats those as implicitly abstract, so the subclass became un-instantiable. Add real implementations on ``TerminalUI`` that render reminders as ``[metacognition · type] text`` lines in yellow. This also restores the metacog signal on the CLI surface (the legacy ``[metacognition: nudge injected — …]`` info-line went away with ``_emit_nudge_ping``; without this commit the CLI showed no signal at all for metacog nudges). Tool-channel and user-channel render identically because terminal output is anchored by stdout flow rather than by DOM anchor — the line lands directly after the message it advises. |
||
|
Patrick Buckley
|
5bd5593f95 |
docs(metacog): align comments with side-channel + tool-channel scope
Address Copilot's review feedback on PR #456 — the docstrings and inline comments hadn't all caught up with the architectural shift across the branch: - ``_apply_reminders_for_provider`` docstring: "every user message" → role-agnostic, since tool messages also carry ``_reminders`` (tool_error / repeat). - ``_mark_reminders_delivered`` docstring: same role-agnostic update; explicitly note both channels. - ``_append_user_turn`` callsite comment near ``_attach_pending_user_reminders``: still described splicing ``<system-reminder>`` blocks into user content; updated to reflect the side-channel attach + transient-copy splice at the provider boundary. - ``_build_history`` block comment: was user-message-only; now mentions tool messages and both ``user_reminder`` / ``tool_reminder`` SSE events. - ``_build_history`` propagation comment: same role-agnostic note on the per-entry surface. - ``app.js`` ``user_reminder`` SSE handler comment: said the bubble renders "above" the user message, but ``insertAdjacentElement('afterend', el)`` drops it BELOW. - ``app.js`` ``replayHistory`` comment: said "insertBefore drops the reminder directly above the just-rendered user bubble"; same fix — bubble lands BELOW. No behaviour change. |
||
|
Patrick Buckley
|
845dbab616 |
fix(metacog): drop write-success-clear so sequential same-call streaks fire
The repeat-detection block in ``_apply_post_execute_advisories`` had
a leftover "clear streak when a write tool succeeded" branch from
when ``RepeatDetector`` tracked cumulative counts. With the
consecutive-streak semantics introduced earlier in the branch the
branch became:
1. Redundant — any different (name, args) signature already resets
the streak via ``RepeatDetector.record``, so an intervening
read/write naturally breaks the streak.
2. Actively wrong — the clear runs ONCE at the top of each
``_apply_post_execute_advisories`` call, before the per-result
loop records sigs. In a single parallel batch
``[bash, bash, bash]`` the clear runs once and then three
``record`` calls accumulate to count=3 in the same call → fires.
But across three sequential turns, each turn calls
``_apply_post_execute_advisories`` fresh, the clear runs at the
top of each call, and only one ``record`` per call follows — so
the count never gets above 1 and the canonical
"small local model stuck on ``bash('echo test')``" pattern
never triggered the nudge.
The asymmetry only existed for successful calls — failures don't
satisfy the ``not _tool_error_flags.get(tc["id"])`` predicate, so
the clear didn't fire and sequential failures already worked. The
fix is to drop the clear entirely; ``RepeatDetector``'s
consecutive-streak semantics handle every case uniformly.
Tests:
- ``test_successful_write_clears_streak`` →
``test_intervening_different_call_resets_streak`` —
rewords the assertion to reflect the actual mechanism (any
different sig resets, write-or-otherwise) since "writes clear"
was the bug, not the contract.
- ``test_failed_write_does_not_clear_streak`` →
``test_sequential_bash_failures_fire_repeat`` — same shape, just
framing fixed.
- New ``test_sequential_bash_same_command_fires_repeat`` —
regression for the bug user hit (three sequential successful
``bash('echo test')`` calls now correctly fire the nudge).
|
||
|
Patrick Buckley
|
c0fd951764 |
feat(metacog): themed reminder bubble unifies user + tool channels
The yellow themed reminder card introduced for user-channel nudges
(correction / denial / resume / start / completion) now also fronts
tool-channel nudges (tool_error / repeat). Pre-fix the tool channel
shipped its reminders inside the tool-result envelope via
``wrap_tool_result``, leaking the ``<system-reminder>`` block into
``self.messages`` content (same problem the user channel had before
the side-channel refactor) and surfacing the legacy gray
``[metacognition: nudge injected — …]`` info line as the only
operator-visible signal — duplicated alongside the new themed bubble
for user-channel nudges.
Tool-channel parity:
- ``_collect_advisories`` now returns
``(persistent_advisories, metacog_reminders)``. Persistent
advisories (``GuardAdvisory`` / ``UserInterjection``) keep
riding ``wrap_tool_result`` because they ARE conversation
history. Metacognitive reminders extract to the second tuple
element; the caller attaches them to the tool message dict's
``_reminders`` side-channel and emits ``on_tool_reminder``.
- ``_apply_reminders_for_provider`` already handles ``_reminders``
on any role, so the tool-channel splice into wire content is
free. ``_build_history`` also already propagates
``entry["reminders"]`` regardless of role, so reload renders the
bubble too.
- ``SessionUI`` Protocol gains ``on_tool_reminder(reminders,
tool_call_id)``; ``SessionUIBase`` enqueues a ``tool_reminder``
SSE event with the ``tool_call_id`` anchor.
- ``_emit_nudge_ping`` had no remaining callers and was removed —
the themed bubble (live SSE + ``/history`` reload) is the
canonical operator signal for both channels now.
UI polish (the four fixes the screenshot caught for the user
channel + their tool-channel mirror):
- Bubble renders BELOW the message it advises (semantically: a
hint to the model right before its turn). ``addUserReminder``
swaps ``insertBefore`` for ``insertAdjacentElement('afterend',
el)``; ``addToolReminder`` anchors below the ``.ts-approval``
block whose tool result triggered the batch's reminder.
- Label uses the full feature name ``metacognition`` (was the
``metacog`` shorthand).
- Card width / alignment inherits from the base ``.msg`` rule —
``align-self: flex-end`` and the explicit ``max-width`` are
gone, so the card matches the user / assistant column instead
of pinning right-aligned narrow.
- The legacy ``[metacognition: nudge injected — …]`` gray info
line is gone for both channels.
Frontend additions:
- ``Pane.prototype.addToolReminder(reminders, toolCallId)``
anchors below the ``.ts-approval`` block (live: by
``data-call-id``; replay: by "last block in messagesEl"
fallback, which is correct because messages render in order).
- SSE switch case ``"tool_reminder"`` calls ``addToolReminder``.
- ``replayHistory``'s tool-message branch now calls
``addToolReminder`` when ``msg.reminders`` is present.
- ``addUserReminder`` advances its anchor on each loop iteration
so multiple reminders stack in queued order rather than
reversed.
Coord console parity:
- ``coordinator.js`` gains ``appendReminderBubble`` /
``appendUserReminderLive`` / ``appendToolReminderLive`` mirroring
the interactive UI. The tool-channel anchor walks
``toolRows[callId].batch`` to attach below the
``.coord-tool-batch`` construct (one bubble per dispatch turn,
matching the "one nudge per batch even with many failing tools"
drain).
- SSE switch handles ``user_reminder`` and ``tool_reminder`` on
the coord conversation surface.
- ``/history`` replay propagates ``msg.reminders`` for user and
tool messages — same wire shape as the interactive pane.
- ``.msg.user-reminder`` styles moved to
``shared_static/chat.css`` so both surfaces inherit the same
yellow themed bubble from the shared base.
Defensive read on ``_apply_reminders_for_provider`` (per Copilot
review on the closed PR): a malformed ``_reminders`` entry (string,
None, etc. — corruption / partial state) used to abort ``send`` via
AttributeError on the ``.get("text", "")`` call. Filter to dicts
before building the block, mirroring the same filter
``_build_history`` already applies on the wire-out side; an
all-malformed list passes through as no-reminders.
Tests:
- ``test_collect_advisories_drains_tool_buffer_on_last_result``
rewritten to assert the ``(persistent, metacog)`` tuple shape
and that ``MetacognitiveAdvisory`` no longer appears in the
persistent list.
- ``test_collect_advisories_holds_*`` and ``_drops_*`` updated for
tuple return.
- ``test_attach_emits_visibility_ping`` /
``test_collect_advisories_emits_visibility_ping`` inverted to
assert the legacy gray line is gone on both channels.
- ``TestSessionUIBaseToolReminderHook`` covers the new SSE event
shape with the ``tool_call_id`` anchor.
- ``test_malformed_reminders_filtered_out`` and
``test_all_malformed_reminders_passes_through`` cover the
Copilot-flagged defensive filter.
|
||
|
Patrick Buckley
|
3aa9f53fd8 |
fix(session): metacog reminders ride a side-channel, not user content
User-channel metacognitive nudges (correction, denial, resume, start,
completion) used to be spliced into ``user_msg["content"]`` permanently,
which leaked the ``<system-reminder>`` envelope into every consumer of
``self.messages`` — UI replay (mitigated by a regex strip in /history),
compaction, title generation, and any future channel adapter that
echoes conversation context. The /history strip was a band-aid;
compaction and title-gen still saw the raw spliced text.
Switch to a side-channel: ``_attach_pending_user_reminders`` writes the
rendered reminder list to ``user_msg["_reminders"]`` (sibling key,
leading-underscore convention shared with ``_attachments_meta`` /
``_provider_content``). At the provider boundary, a new
``_apply_reminders_for_provider`` builds a transient shallow-copy with
the reminder spliced into ``content``; the original message dict
stays clean. ``sanitize_messages`` drops the sibling key on the wire.
Once-per-session-not-per-turn semantics for the wire: after stream
success the loop calls ``_mark_reminders_delivered``, which flips a
``_reminders_delivered`` flag on every user message that carried
reminders into that call. ``_apply_reminders_for_provider`` skips
already-delivered messages so the model sees each reminder exactly
once (the turn it advised). ``_build_history`` ignores the delivered
flag entirely, so reconnecting tabs render the same nudge bubble the
originating tab saw via the live ``user_reminder`` SSE event.
UI surface:
- ``SessionUIBase.on_user_reminder`` enqueues a
``{type: "user_reminder", reminders: [...]}`` SSE event with the
same shape ``_build_history`` surfaces.
- ``app.js`` renders a ``.msg.user-reminder`` bubble (yellow accent,
pill-styled) anchored above the user message it advises, both
live and on history replay.
- ``replayHistory`` renders ``addUserMessage`` before
``addUserReminder`` so the anchor lookup finds the just-rendered
turn (not a prior one).
- Multi-tab caveat documented inline: non-originating tabs receive
no ``user_message`` SSE event today, so a reminder may anchor to
a stale prior bubble until ``/history`` reload corrects it.
Pre-existing bug surfaced by the audit: cancel handlers
(``GenerationCancelled`` / ``KeyboardInterrupt`` / generic
``Exception``) in ``ChatSession.send`` cleared
``_pending_tool_advisories`` but not the user-channel buffer. Both
now drain through a shared ``_drain_pending_advisories`` helper.
Removed the ``/history`` regex strip — the side-channel approach
makes it redundant. Hoisted ``escape_wrapper_tags`` +
``render_system_reminder`` imports to module top (called 2-3× per
turn).
Tests:
- ``TestApplyRemindersForProvider`` — pass-through-by-reference,
string + list content splice, escape on user-typed wrapper tags,
multi-reminder ordering, source-untouched invariant, delivered
flag skip path, fallback for unexpected content shape.
- ``TestMarkRemindersDelivered`` — flag idempotency, no-reminders
no-flag, only marks user messages with reminders.
- ``TestUpdateTokenTableMsgsParam`` — calibration uses pre-built
msgs when provided, falls back when not.
- ``TestUserAdvisoryCancelClear`` — all three cancel branches drain
the user buffer.
- ``TestReminderSidechannelIsolation`` — compaction's
``_format_messages_for_summary`` and the title-gen extraction
loop cannot see reminders by construction.
- ``TestSessionUIBaseUserReminderHook`` — ``on_user_reminder``
enqueues the right SSE shape.
- ``TestBuildHistoryReminderPropagation`` — ``entry["reminders"]``
propagation, absent / empty / multi / coexist-with-attachments
cases, malformed input filtering, all-malformed elision.
- ``test_sanitize_messages_strips_underscore_sibling_keys`` covers
``_reminders`` and ``_reminders_delivered``.
|
||
|
Patrick Buckley
|
7c8cb8c595 |
fix(metacog): N>=3 streak detector + drop redundant error-prefix list
Cleanup pass on the metacognitive nudge stack — restores pre-split errored-counts-toward-repeat behaviour and tightens the is_error plumbing through the per-batch advisory hook. The per-batch hook in ``_run_loop`` was duplicating the is_error signal: ``self._tool_error_flags`` (set by ``_report_tool_result``) and a string-prefix tuple (``Error`` / ``JSON parse error`` / …). Two truth sources is what got us here — bash commands that exit non-zero with normal stdout matched the flag but not the prefix, the deny path matched the prefix but not the flag, and the result was that stuck-loop detection silently broke for the most common failure mode (the model bashing the same broken command). Single source of truth now: - ``_execute_tools.run_one`` deny branch routes through ``_report_tool_result(is_error=True)`` so denied calls populate ``_tool_error_flags`` like every other error path. - The error-prefix tuple is gone; the write-success-clear gate and the tool-error-nudge gate both read ``_tool_error_flags`` only. Repeat-detection state moves from a ``set[str]`` (fired on the second identical call, ignored errors entirely) to a ``RepeatDetector`` helper in ``metacognition.py`` with consecutive-streak semantics: - Threshold raised from 2 to 3 — two-in-a-row was noisy on legitimate transient retries; three is the cheapest stuck-loop signal. - Recording a different signature resets the count, so [A, A, B, A] is two short streaks of 2 and not a streak of 4. Bounded by O(1) state regardless of session length. - Errored calls now count toward the streak (the split into a separate metacog module unintentionally introduced a "skip errors" branch — restored). While there: - ``metacognition._COOLDOWN_SECS`` default aligned to 300s (matches ``MemoryConfig.nudge_cooldown`` and the ``memory.nudge_cooldown`` config-store default; was set to 30 by an earlier investigation). - The per-batch advisory block (~80 lines of mixed orchestration inside ``_run_loop``) is extracted to ``ChatSession._apply_post_execute_advisories`` so the wired behaviour is testable without driving ``_run_loop`` end-to-end. Producer extraction to a dedicated module is deferred to a follow-up; advisory producers all live on ``ChatSession`` for now per existing convention. - Frontend ``appendToolOutput`` (turnstone/ui/static/app.js) now skips rendering when the parent approval block is denied or the output starts with ``Denied by user`` / ``Blocked``, mirroring the history-replay guard at ``_build_history``. Previously the live SSE path didn't need this guard because the deny path never emitted a ``tool_result`` event; the is_error routing change above means it does now, so without this guard the badge from ``resolveApproval`` and the SSE output would both render. Tests: 8 unit tests for ``RepeatDetector`` covering streak, threshold, clear, and intervening-sig reset; 9 integration tests for ``_apply_post_execute_advisories`` covering the wired behaviour (3-identical fires warning + advisory + UI line, errored calls count toward streak as a regression guard, intervening sig resets streak, successful write clears, failed write does not, JSON outputs tracked but not inline-warned, tool_error nudge gates on memory_count, repeat UI line emitted on streak fire). |