mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
d068366a61337f37775a58cfc515bdb6530ff086
950 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
d068366a61 |
rbac: builtin-role override editor + tighten under-enforced perm gates (#585)
* feat(rbac): editable builtin role permissions via overlay layer
Adds a ``role_permission_overrides`` table that stores per-(role_id,
permission) grant/revoke deltas, applied on top of the immutable
``roles.permissions`` baseline at permission-load time. Builtin roles
(``builtin-admin/operator/viewer``) become customizable through the
admin Roles UI without losing the "reset to default" guarantee — every
override is auditable and reversible.
Motivating case: ``model.skills.write`` is deliberately default-ungranted
on every role so operators must consciously opt in before a coordinator
session can mutate the skill catalog. Until now there was no UX path to
do that opt-in — the only options were dropping into SQL or running a
fresh migration. The overrides editor closes that gap.
Backend
- Migration 057 + storage methods on both sqlite + postgresql backends
- ``get_user_permissions`` merges baseline ∪ grants − revokes for builtin
rows; custom rows pass through unchanged
- ``GET /v1/api/admin/roles/{id}/effective`` for inspect
- ``PUT /v1/api/admin/roles/{id}/overrides`` for write — admin.roles gated,
audited, validates against ``_VALID_PERMISSIONS``, refuses non-builtin
targets, strips no-op grants/revokes before persisting
- Lockout guard: cannot revoke ``admin.roles`` if doing so would leave
zero users with the permission (returns 409)
- ``coordinator.trust.send`` added to ``_VALID_PERMISSIONS`` — was
seeded into builtin-admin by migration 042 but never registered with
the validator, so the very first round-trip through the editor 400'd
on it. Drift-detection test guards future migrations from recreating
the same gap
Frontend
- Roles tab redesign: chevron + permission-count chip replace the
"..." truncation; expand-on-click drawer groups perms by namespace
with baseline / grant (green +) / revoke (red −) chip variants
- Edit modal opens for builtin rows ("Customize Built-in Role" title);
toggles show baseline-default vs override state; submit diffs against
the rendered toggle universe (not raw baseline) so future taxonomy
drift can't silently strip unknown perms
- "Modified +N/-N" pill on rows with active overrides; "Reset to default"
drawer action clears the override set
- ``_PERMISSION_SECTIONS`` brought up to date with all currently-seeded
perms (admin.coordinator, admin.cluster.inspect, admin.models,
admin.nodes, admin.prompt_policies, conversation.modify,
coordinator.trust.send were missing)
Tests
- 7 storage tests covering set/list/clear/effective + overlay merge into
``get_user_permissions`` for both builtin and custom roles
- 11 endpoint tests covering effective/overrides happy paths, validation,
lockout guard, builtin-only restriction, no-op normalization, list
enrichment
* feat(rbac): enforce workstreams.{create,close} + tools.approve gates
These three permissions were declared in ``_VALID_PERMISSIONS``, seeded
into ``builtin-operator``'s baseline by migration 008/017, surfaced in
the admin Roles UI as toggles, and documented in ``bootstrap.py`` as
the operator role's capabilities — and never enforced anywhere. The
audit that ran out of the overlay PR found zero ``require_permission``
sites for any of them; any authenticated user could create workstreams,
close any workstream, or approve any pending tool regardless of role.
Behaviour change for callers without the perms:
- ``POST /v1/api/workstreams/new`` (node + console proxy variants)
now 403 without ``workstreams.create``
- ``POST /v1/api/workstreams/{ws_id}/close`` (and ``/route/`` proxy)
now 403 without ``workstreams.close``
- ``POST /v1/api/workstreams/{ws_id}/approve`` (and ``/route/`` proxy)
now 403 without ``tools.approve``
The OR-fallback to ``admin.coordinator`` keeps coord sessions spawning
interactive children unblocked without needing operator-style perms.
Service-scoped inter-cluster calls bypass via the existing
``allow_service_bypass`` path on the new ``require_any_permission``
helper. Builtin admin and operator both already carry these perms;
viewer correctly loses workstream create/close/approve (it already
couldn't do those in spirit).
Implementation
- ``require_any_permission`` (core/auth.py) — OR-semantics variant of
``require_permission`` with per-conditional comments documenting the
security policy at the choke point. 403 body names every accepted
perm so operators get an actionable remediation
- ``make_{create,close,approve}_handler`` (core/session_routes.py)
accept ``fallback_permissions: tuple[str, ...]`` — checked only when
``cfg.permission_gate is None`` (interactive case). Coord's
``permission_gate=_require_admin_coordinator`` continues to take
precedence on the coord-config side
- Console-side ``create_workstream`` and ``route_create`` inline the
same OR check before proxying — fail fast on a forbidden request
without burning a cluster round-trip
- ``route_proxy`` adds a verb-scoped gate on ``approve`` and ``close``
only; ``send``/``cancel``/``dequeue``/``command``/``plan`` remain
authenticated-only (pre-existing, out of scope for this audit)
Tests
- New ``TestPermissionGatesOnLifecycle`` (4 tests) in test_server_authz
pinning 403-without-perm + non-403-with-perm at the node lift sites
- New ``TestRouteProxyPermissionGates`` (5 tests) in
test_console_routing_proxy covering 403 paths, OR fallback via
``admin.coordinator``, and that ``send`` remains ungated
- ``_make_jwt`` helpers in test_server_authz, test_close_reason_
persistence, test_server_attachments_on_create updated to embed
operator-shaped perms by default so existing tests continue to
exercise the post-gate logic rather than 403'ing on the new check
Docs
- ``bootstrap.py`` operator role line corrected to list every perm
it actually carries (was missing ``tools.approve`` and
``conversation.modify``)
* fix(rbac): close lockout + escalation gaps in role-overrides editor
Three issues surfaced by /review of the overlay layer and gate uplift —
all in the RBAC/auth surface, treated as zero-days.
**F-1: lockout guard misses the grant-removal path.** PUT-replace
semantics on ``set_role_overrides`` mean an existing grant of
``admin.roles`` (added via override to e.g. builtin-operator) is
silently dropped when the new payload omits it. The previous guard
short-circuited on ``"admin.roles" not in revokes`` and never noticed.
Concrete cluster-bricking scenario: grant admin.roles to operator via
override, unassign builtin-admin, click "Reset to default" on operator
→ all users lose admin.roles, recoverable only via SQL.
The rewritten guard simulates the post-PUT effective set on the target
role directly: if ``(baseline | new_grants) - new_revokes`` lacks
admin.roles AND nobody holds it via another role, refuse the change.
The "via another role" question is answered by one bulk query rather
than the prior O(users × roles) round-trip loop.
**F-3: lockout check blocked the event loop on moderate deployments.**
The prior check called ``storage.list_user_roles`` per user and
``storage.effective_role_permissions`` per (user, role) pair —
synchronous SQL inside an async handler. 200 users × 5 roles = 1000
connection cycles long enough to trip reverse-proxy timeouts on a
permission revoke.
Replaced with ``storage.users_with_permission(perm, *,
exclude_role_id)`` — one join over ``user_roles ⋈ roles`` plus one IN
fetch on overrides for the builtin role ids in the result, folded
in-process. Two queries total, independent of cluster size. The whole
check now runs under ``asyncio.to_thread`` so even the bulk read
doesn't stall the loop.
**F-2 reframed: admin_assign_role's subset check ignored the overlay.**
The check at lines 6321-6328 reads ``target_role.get("permissions",
"")`` (baseline column) when computing the perms it requires the
caller to hold. After this branch, an admin.roles holder can grant
e.g. ``model.skills.write`` to builtin-operator via override; an
admin.users holder (who happens to NOT hold that perm) could then
assign operator to a new user, silently escalating the assignee. The
existing two-person-rule by perm split (admin.roles for catalog edits,
admin.users for assignments) only holds if the assignment-time check
considers the overlay. Switched ``target_perms`` to
``storage.effective_role_permissions(role_id)["effective"]``.
Note: this PR retains the existing model where admin.roles is the
catalog-edit superuser (admin_create_role, admin_update_role, and now
admin_role_overrides all skip the caller-holds-grants check). The
two-person rule against escalation lives at the assignment gate, which
this fix reinforces.
**F-7: delete_role left orphaned override rows.** No FK on
``role_permission_overrides.role_id`` (migration 057 omitted FKs to
match the rest of the governance schema). Added explicit cleanup in
both sqlite + postgresql ``delete_role`` implementations so a
re-seeded role_id (deterministic for builtins on schema reseed) can't
silently inherit stale overrides from the prior occupant.
Tests
- storage: ``test_users_with_permission_bulk`` exercises the new bulk
helper including ``exclude_role_id`` and overlay folding
- storage: ``test_delete_role_cleans_up_overrides`` pins the F-7 fix
- endpoint: ``test_overrides_lockout_guard_blocks_grant_removal`` is
the F-1 reproduction — operator-overlay grants admin.roles, builtin-
admin has it removed, attempting to reset operator's overrides 409s
- endpoint: ``test_assign_role_blocks_escalation_via_overlay_grant``
pins the F-2 reframed fix — overlay-poisoned operator can't be
assigned by a caller missing the overlay perms
* refactor(rbac): cleanup batch from /review (#584)
Five non-security findings folded into one commit so the security
batch stays focused. All consistent with the existing intent of
``feat/builtin-role-overrides``.
**F-4: presence check on ``_effectivePerms``.** ``governance.js`` was
guarding on ``Array.isArray(role.effective) && role.effective.length > 0``,
falling through to splitting ``role.permissions`` (the baseline) when
the array was empty. For a builtin role whose overrides legitimately
revoke every baseline perm, that path silently rendered the baseline
chips with no override indicators — the inspector lied about what the
role can do. ``_enrich_role`` always sets ``effective: []``, so
presence is the right sentinel.
**F-5: JS-side drift detector.** Commit 1 added a Python-side test
asserting ``_VALID_PERMISSIONS`` covers every baseline perm; the
mirror invariant on the frontend went uncaught. A new perm added to
``_VALID_PERMISSIONS`` without a matching entry in
``_PERMISSION_SECTIONS`` becomes silently un-customizable through the
admin UI (the only documented grant/revoke path). Test parses the
JS const out via regex and asserts set-equality both directions —
detects "missing in UI" and "extra in UI" so the toggle catalog and
validator can't fork.
**F-6: bulk enrich for ``admin_list_roles``.** Was ``1 +
2*builtin_count + 1*custom_count`` SELECTs per admin-tab open;
collapsed to one ``IN``-filtered query via new
``storage.effective_role_permissions_bulk(role_ids)``. Implemented
on both sqlite + postgresql backends following the existing
``effective_role_permissions`` shape.
**F-8: rename ``fallback_permissions`` → ``accepted_permissions``.**
The lift body uses ``if cfg.permission_gate / elif accepted_permissions``
— mutually exclusive — so when ``permission_gate`` is None this IS
the primary gate, not a fallback to anything. The "fallback" name
suggested a tier-2-after-tier-1 semantic that didn't exist. Renamed
across ``make_{approve,close,create}_handler`` factories, the three
call sites in ``turnstone/server.py``, and the docstrings.
**F-9: positive lift-level tests for ``admin.coordinator``-only.**
``TestPermissionGatesOnLifecycle`` previously had a single positive
test for ``workstreams.create`` alone, plus negative-403 tests for
each verb without perms. The OR-fallback to ``admin.coordinator``
(which keeps coord sessions spawning interactive children unblocked)
had no positive coverage at the lift code path — only at the proxy,
which exercises a different verb-dict gate. Added three tests
(create / close / approve) that pass ``admin.coordinator`` alone and
assert non-403, so a future tightening of the accepted_permissions
tuple can't silently regress coord-driven child workstreams.
Out of scope: nit perf-4 (event-delegation refactor on
``_renderGovRoles``). ``setSafeHtml`` rebuild is the existing
pattern across every admin tab; rewriting one tab's render path on
this branch would be drive-by inconsistent with the surrounding
codebase. Filed as a separate concern if the Roles tab grows past
the scale where it bites.
* fix(rbac-ui): aria-expanded + row-click on Roles drawer (#585)
Two Copilot review findings on governance.js:
- Expand button was missing aria-expanded — screen readers couldn't
announce drawer state. Now reflects the row's expanded flag.
- Comment said "row + chevron both work" but only the chevron was
wired. Added data-expand-role to the row element too so the
existing handler loop (querySelectorAll on the attribute) picks up
both — clicking anywhere in the role row toggles the drawer.
Edit/Delete handlers already stopPropagation so they aren't
triggered by the row-level click.
* fix(migrations): rebase role_permission_overrides to 058
PR #560 mitigation #1 landed 057_output_assessments_llm_judge.py on
main in parallel; my migration claimed the same number, forking
alembic's head and breaking postgres. Renumbered to 058 and
re-pointed down_revision at 057 so the chain stays linear.
No behaviour change — same DDL. Full sweep clean (6730 passed).
* fix(migrations): update 058 revision strings to match filename
Previous commit (
|
||
|
Patrick Buckley
|
f94db6e5a9 |
fix(model_registry): tuple-of-ints version sort in _select_best_model
`float("4.20") == 4.2`, so the old version-sort routed `grok-4.20`
under `grok-4.3` despite 4.20 being the newer dated-snapshot line.
Parsing each component as an int via `_version_tuple` makes
`(4, 20) > (4, 3)` as intended.
Applied symmetrically to the openai branch — same shape, same latent
bug against a future `gpt-5.10` vs `gpt-5.2` collision.
Locked in by four tests in `test_provider_xai.py::TestSelectBestModel`.
Spotted by Copilot review on #586.
|
||
|
Patrick Buckley
|
f4f7944832 |
feat(providers): xAI/Grok provider via OpenAIResponsesProvider subclass
Adds xAI as a first-class commercial provider through the officially- documented server-to-server API-key path against https://api.x.ai/v1. XAIProvider is a thin subclass of OpenAIResponsesProvider; xAI's Responses surface is OpenAI-shaped, so the only override needed is _build_kwargs, which merges <tool>_call_output strings into include[] so xAI's server-side tool outputs (hidden by default) become visible. GROK_CAPABILITIES covers the five documented chat models; aliases like grok-4.3-latest resolve via the existing longest-prefix lookup. Two narrow base-class generalisations earn their keep beyond Grok: - ModelCapabilities.server_side_tools: tuple[str, ...] drives the Responses-surface tool injection (previously hardcoded to web_search). resolve_server_side_tools folds in the legacy supports_web_search boolean for backward compat. - extra_headers: dict[str, str] | None threaded through LLMProvider.create_streaming / create_completion so callers can pass x-grok-conv-id: <ws_id> for prompt-cache hit-rate. Session- side population is a follow-up; the plumbing lands here. Out of scope: - OAuth (SuperGrok / X-Premium+) — not officially documented. - Chat Completions surface — deprecated on xAI's comparison page. - Image / voice / video models. - argparse --provider xai in cli.py / server.py — Google isn't there either; both providers configure via config.toml. Closes #583. |
||
|
Patrick Buckley
|
59ccbcddd8 |
fix(judge): address Copilot review on #579
Validated each of the 7 substantive Copilot findings + 5 CodeQL findings via source spike; applied 7 (CP2-CP7, C3-C5), refuted 2 (CP1, C1+C2) with citation. * CP2 (session.py:3551) — pre-truncation budget was reused per output, allowing N parallel tool results to each claim the full remaining context budget and collectively overflow. Maintain a running budget that shrinks as each output is sized. * CP3 (ratelimit.py) — TokenBucket.consume() mutated tokens / last_refill without a lock; ChatSession._batch_evaluate_outputs invokes it concurrently from up to 4 worker threads, so the rate limiter's stated 60-call/min cap was best-effort. Added an internal threading.Lock that protects consume() and retry_after. RateLimiter's outer lock still holds for the bucket-dict it owns; the new lock just makes the class safe-by-default for direct consumers. * CP4 (session_ui_base.py:1642) — docstring claimed "llm" rows were only persisted on success; the session actually persists a failure row too (with reasoning=error_reason) for audit. Match the docstring to behavior. * CP5 (migration 057) — header said "Revises: 055" but down_revision was "056". Fix header. * CP6 (_protocol.py:1761) — :func:`intent_verdicts` is a table, not a callable. Plain reference. * CP7 (test_output_guard_judge.py:271) — test_client_created_once was self-contradictory: header comment claimed caching was being verified, body asserted the patched fake was called once per evaluate (no caching). The next test (test_real_lazy_init_caches_real_client) covers actual caching; drop the misleading one. * C3-C5 (_extract_json) — three empty except-pass blocks now carry a justifying one-liner explaining each strategy's expected failure mode and what falls through. REFUTED: * CP1 (output_guard_judge.py:453 catch of TimeoutError vs concurrent.futures.TimeoutError) — in Python 3.11+ (our minimum, per pyproject.toml requires-python = ">=3.11") the two are the same class. Verified at runtime: ``cf.TimeoutError is TimeoutError`` is True; MRO is (TimeoutError, OSError, Exception, BaseException, object). The polling loop catches the right exception. * C1/C2 (Protocol method bodies and test stubs using ``...``) — `...` is the idiomatic Python pattern for Protocol method bodies and one-line stub functions; the surrounding code in test_model_registry.py uses ``...`` consistently across all stub methods. CodeQL is flagging a single instance while ignoring identical patterns nearby. |
||
|
Patrick Buckley
|
3233719856 |
feat(judge): output_guard LLM stage with capability gate (#560 mitigation #1)
Adds a second, LLM-driven stage to the output guard so domain-camouflaged prompt-injection payloads that the regex stage misses (arXiv:2605.22001 — Llama 3.1 8B evades the existing regex set on ~90% of camouflaged prompts) get caught before the tool output lands in the assistant's context. ## Surface * New `OutputGuardJudge` in `turnstone/core/output_guard_judge.py` — synchronous, single-shot LLM call. Inlines the alias-resolution + client-config + JSON-parsing helpers (copied verbatim from `IntentJudge` at `judge.py:917-969` / `1604-1659`) rather than going through a shared module — when `IntentJudge` lifts its own helpers, both copies move together. * JSON-in-content verdict with a 3-strategy parser (direct / markdown fence / balanced braces). `IntentJudge` ships a 4th regex-field fallback; OutputGuardJudge deliberately doesn't, because strategy-4 hits on broken LLM output can extract a "verdict" from the model's reasoning quote that lands in storage looking identical to a clean strategy-1 result. Failure of all three returns `error="unparseable_verdict"` and the heuristic stage stands. * `OutputJudgeVerdict` is a frozen dataclass with: `risk_level` (none/low/medium/high — normalises `critical`→`high` and `info[rmational]`→`low` for IntentJudge-echo safety), `flags: tuple[str, ...]`, `reasoning`, `confidence: float` (0.0-1.0, parsed + clamped from the LLM's self-report; pass-through to audit, no threshold gating), `judge_model`, `latency_ms`, `error`. * Real wall-clock timeout via `ThreadPoolExecutor.shutdown(wait=False, cancel_futures=True)` on the timeout/cancel path — `with ... as ex:` would block return until the worker drained. 1s `cancel_event` poll mirrors `IntentJudge._run_judge` at `judge.py:1117-1118`. * HTTP client lazy-init + reuse for the judge instance's lifetime. Session-side model swap drops the entire judge, dropping the client with it. * Untrusted tool output wrapped in per-call random-nonced `<tool_output_NONCE>...</tool_output_NONCE>` fence. Closing-tag substrings in the raw text are case-insensitively backslash-escaped first (`</tool_output` → `<\/tool_output`) so an attacker can't break out even if they guess the nonce. System prompt classifies the fenced region as UNTRUSTED DATA so directives inside are evaluated as content, not obeyed. * Judge user prompt carries the heuristic verdict (risk + flags + annotations), the tool description (looked up from the session's tools registry), and the tool args (truncated to 500 chars, also classified UNTRUSTED in the system prompt since they may be caller-supplied). Lets the judge defer to the regex on credential leaks and focus on injection signals the regex set misses; also enables output-vs-request plausibility reasoning. ## Session integration * `_evaluate_output(call_id, output, func_name, *, tool_args="")` — heuristic always runs; LLM stage runs when `judge.output_guard_llm` is enabled. When the LLM produces a usable verdict and the heuristic didn't detect credentials, the LLM verdict is acted on; otherwise the heuristic stands. * Credential redaction is a regex-only signal. When `heuristic. sanitized` is non-None, the heuristic owns the acted assessment regardless of what the LLM said — an LLM asked about prompt- injection can correctly label a credential-bearing output as "none" risk for injection, but the secret still needs redaction. * `_batch_evaluate_outputs` runs the per-tool guard concurrently (4-worker pool) when LLM is enabled and there are ≥2 string outputs — collapses N×LLM-latency to ⌈N/4⌉×latency on the common 5-20 tool-calls-per-turn turn. * Per-session `TokenBucket(rate=1.0, burst=60)` caps adversarial LLM-fan-out cost at 60 calls/min/session. * Pre-truncation: the per-tool loop truncates output before the judge sees it, so the judge evaluates exactly what enters the assistant's context (no wasted tokens on text that won't land). * Both heuristic and LLM tier rows persisted to `output_assessments` when the LLM ran (audit completeness); heuristic-only rows skip when matched-clean to keep the table focused. ## Storage Migration 057 extends `output_assessments` with five LLM-tier columns: `tier` (`heuristic` / `llm`, backfilled to `heuristic`), `reasoning`, `judge_model`, `latency_ms`, `confidence`. Tie-break on `(created DESC, tier='llm' first)` so downstream consumers see the acted verdict first when the two rows tie at second resolution. `StorageBackend.record_output_assessment` + sqlite/pg implementations + `SessionUIBase.record_output_assessment` + `SessionUI` protocol + the test stub overrides (cli, eval, 9 test files) all take the new LLM-tier kwargs. ## Config surface Three new judge.* settings in `settings_registry`: * `judge.output_guard_llm` (bool, default False) — capability gate. Default off; operators opt in once a small/fast model is pointed at `output_guard_model`. * `judge.output_guard_model` (str, default "") — alias for the LLM stage. Empty inherits the session model (same fallback shape as `judge.model`). * `judge.output_guard_llm_timeout` (float, default 30.0, min 1.0) — wall-clock budget per call. Both `server.py` and `console/session_factory.py` wire these into the `JudgeConfig` they hand to `ChatSession`. ## Notes * No backwards-compatibility shims — the LLM stage is purely additive. * No reasoning/threshold gating on confidence; it rides as an audit-only signal per maintainer direction. Surface it in the `on_output_warning` dict so live UI / cluster broadcast can sort flagged outputs by judge certainty. * Tests: 392 lines of judge-only coverage (`test_output_guard_judge. py`) + 629 lines of session-integration coverage in `test_session. py`, plus the storage and stub-shape updates. |
||
|
Patrick Buckley
|
06e16de066 |
chore(skills): drop Anthropic attribution from SKILL.md spec references
Two related cleanups landed together because they touch the same surface
(skill-spec uplift PRs #569/#570/#571/#572):
1. Wording: replace "Anthropic spec" / "Anthropic Claude Code skill spec"
with "SKILL.md spec" across admin UI tooltips, code comments, test
docstrings, migration 056's module docstring, and the user-facing
`arguments` description in tools/skills.json. Renames a parser test
`test_anthropic_tags` -> `test_nested_metadata_tags` and consolidates
a parse-API test of the same shape; fixture author renamed
`Anthropic` -> `Acme` to keep the fixture neutral. Legitimate
provider/SDK/API references (provider name, api.anthropic.com,
`_anthropic.py`, capability comments) are intentionally untouched.
2. Admin UX: in the Create + Edit Skill modals, six fields per modal
(Compatibility, Paths, Hide-from-skill-picker, Arguments, Argument
hint, Activation) had long uppercase label-hint spans crammed into
the visible label. Migrated each to the existing
`.settings-help-btn` + `.settings-help-popover` pattern already used
in the Settings tab — short label + inline `?` button that opens a
styled popover with proper `<code>` formatting for technical tokens.
Pattern reuse required two small generalisations in admin.js:
* `_toggleSettingsHelp` now looks up the popover via a new
`data-help-target="<id>"` attribute first, falling back to the
settings-tab `.settings-label-col` ancestor lookup.
* `_closeAllSettingsHelp` mirrors the same dual-path lookup when
resetting `aria-expanded`, so modal buttons don't get stuck on
`aria-expanded="true"` after another popover opens.
* Added a document-delegated click handler that fires only for
buttons with `data-help-target`; existing per-button binding
in the settings-tab render path is unchanged.
CSS: `.settings-help-btn` now paints its `?` via `::after` with the
button's own `font-size: 0`, so prettier-introduced whitespace
inside the new HTML buttons can't off-center the glyph. The same
rule applies to existing admin.js-generated buttons (text content
hidden, pseudo identical). Small additions for
`.settings-help-popover code` / `strong` styling so technical
tokens render with the same monospace pill treatment used elsewhere
in skill UI.
Known follow-ups (intentionally NOT in this PR):
* Migrate the settings-tab `_renderSettingRow` button assembly to the
empty-`<button>` + `data-help-target` form so the per-button
addEventListener loop can be dropped in favour of pure document
delegation, and the `font-size: 0` rule stops being a workaround for
two markup styles.
* The 12 new popover blocks are duplicated verbatim between the
Create and Edit modals (same as the rest of the create/edit modal
pair). A small renderer that emits popovers from a shared data
object would eliminate the drift risk but is unrelated cleanup.
|
||
|
Patrick Buckley
|
e1c2a05467 |
fix(coord): strip intent-judge verdicts from inspect_workstream output (#580)
* fix(coord): strip intent-judge verdicts from inspect_workstream output Coordinator LLMs repeatedly misread `user_decision="policy"` (the label meaning "auto-approved by an admin policy allow rule") as "blocked, waiting for policy review" — combined with `recommendation="review"` (the heuristic judge's risk class, not a workflow state) the verdict fields read end-to-end as "stuck on policy review" and produced incorrect cancel-and-respawn reasoning against healthy children. The blocking signal already lives on `state` (`"attention"`) and the `live.pending_approval` block, both still in the result. Verdict history remains queryable through admin / audit surfaces — only the LLM-facing inspect surface drops them. Also drops `verdict_count` / `verdicts_by_risk` from the tier-3 skeleton fallback, deletes the now-dead `_serialize_verdicts` helper, and clears the now-stale `"verdicts": []` keys from 10 fixture sites that fed `_format_inspect_tiered` test cases. * fix(coord): correct comment pointer — inline comment, not docstring |
||
|
Patrick Buckley
|
aa9812f2a6 | chore: bump version to 1.6.0a4 v1.6.0a4 | ||
|
Patrick Buckley
|
9309162ac0 |
feat(skills): wire \$ARGUMENTS / \$N / \$<name> / \${CLAUDE_*} substitution (#572)
Implements the Anthropic Claude Code skill spec's placeholder
substitution end to end. The renderer in ``_substitute_skill_args``
handles every spec form except ``\${CLAUDE_SKILL_DIR}`` (deferred):
* ``\$ARGUMENTS`` — full args string as the user/model typed it
* ``\$ARGUMENTS[N]`` / ``\$N`` — Nth positional arg, ``shlex.split``-parsed
* ``\$<name>`` — named arg from the SKILL.md ``arguments:`` list
* ``\${CLAUDE_SESSION_ID}`` / ``\${CLAUDE_EFFORT}`` — session state
Substitution is single-pass (one combined regex, one ``re.sub``).
Append rule: when args are passed but the body has no bare
``\$ARGUMENTS``, append ``ARGUMENTS: …`` at the end.
## Surface
* Parser: ``arguments:`` (list/space-delim) + ``argument-hint:`` (str)
extracted into ``ParsedSkill``.
* Install: persists both to the pre-allocated columns from migration
056 (PR #574). Install path clamps ``argument_hint`` to 128 chars
to match the admin-create cap (untrusted upstream source).
* Admin: ``CreateSkillRequest`` / ``UpdateSkillRequest`` accept both
fields; create + edit modals get inputs; parse-preview echoes.
* Renderer: ``_substitute_skill_args`` runs AFTER ``_render_template``
in ``_load_skills`` so user-supplied args containing ``{{var}}`` can't
be re-expanded by the legacy renderer.
* Session: ``_skill_arguments`` plumbed through ``__init__``,
``set_skill``, and ``_save_config`` so a resumed workstream re-renders
with the original arg payload.
* Model tool: ``skills(action='load')`` accepts an ``arguments`` string.
Approval label includes a SHA-256 digest of the args so a once-
approved skill name can't grant cover for a future payload; preview
surfaces the args inline.
## ``/review`` findings (addressed)
* ``\${CLAUDE_EFFORT}`` referenced ``self._reasoning_effort`` — wrong
attribute; the real one is ``self.reasoning_effort``. Always rendered
empty. Fixed.
* Two-pass layering let user args containing ``{{var}}`` re-expand.
Render order reversed.
* ``_skill_arguments`` wasn't in ``_save_config`` — resumed workstreams
silently lost their payload. Added.
* Approval label omitted ``arguments``. Digest + preview added.
* Install path didn't bound ``argument_hint``. Clamped.
* Added ``_skill_arg_names`` decode tests + "load same skill,
different args → re-render" invariant test.
## Copilot review findings (addressed)
* ``skills.json`` tool description was inaccurate about ``shlex``
stripping quotes and "empty string disables substitution". Rewrote
to match actual behaviour.
* Named-argument regex was stricter than parser/storage contract.
``arguments: [issue-number]`` would partial-match ``\$issue-number``
as ``\$issue``, leaving ``-number`` as stray text. Broadened the
regex to ``[A-Za-z_][A-Za-z0-9_]*`` AND added validation at
``_skill_arg_names`` decode time so names not matching the regex
are dropped with a warning.
## Tests
* ``tests/test_substitute_skill_args.py`` — placeholder forms,
single-pass guarantee, append-at-end rule, shell-quoted input,
unbalanced-quote fallback, uppercase + underscore-prefix names
* ``tests/test_skill_parser.py::TestArgumentsAndHint`` — parser
extraction
* ``tests/test_skill_parse_api.py`` — HTTP parse-preview echoes
both fields
* ``tests/test_skill_discovery_api.py::test_install_seeds_arguments_and_argument_hint``
— install round-trip
* ``tests/test_skills_tool.py::test_load_forwards_arguments_to_set_skill``
+ ``test_load_same_skill_different_args_triggers_resub`` —
wire path through prepare → exec → set_skill
* ``tests/test_skills_tool.py::TestSkillArgNames`` — storage decode
helper including the hyphen/dot/leading-digit filter
|
||
|
Patrick Buckley
|
66400999dd |
fix(skills): address Copilot review on #577
Two findings from Copilot's review of PR #577: * ``_extract_bool`` int branch: Copilot flagged that ``bool(raw)`` treats any non-zero int as True, so ``disable-model-invocation: 2`` silently disables model invocation without warning the author about the typo. Tightened to accept only ``0`` and ``1`` as integer boolean forms — anything else falls back to *default*. Matches spec (which mentions only 0/1) and the broader principle that ambiguous input should not coerce silently. * ``hidden_from_menu`` admin body parse: Copilot flagged that ``bool(body.get("hidden_from_menu", False))`` treats non-empty strings via Python truthiness, so a malformed client sending ``"false"`` would flip the flag to ``True`` — opposite to obvious intent. Extracted a ``_parse_strict_bool`` helper that accepts only Python ``bool`` or int ``0``/``1`` and returns a 400 on anything else. Applied at both admin create and admin update sites; the install path remains untouched because it derives the flag from the typed ``ParsedSkill.user_invocable`` field (not raw HTTP body). ## Tests * ``test_other_ints_fall_back_to_default`` — ``2`` and ``-1`` no longer silently coerce * ``test_create_skill_hidden_from_menu_string_rejected`` — string ``"false"`` returns 400 * ``test_create_skill_hidden_from_menu_int_zero_and_one_accepted`` — 0 / 1 accepted, 2 rejected with 400 No behaviour change to the main surface — both fixes close latent type-coerce hazards a malformed input could have exploited. |
||
|
Patrick Buckley
|
6d28afbe7a |
feat(skills): wire disable-model-invocation / user-invocable (#571)
The Anthropic Claude Code skill spec defines two invocation-control axes Turnstone was parsing but not consuming: * ``disable-model-invocation: true`` — model can't autoload this skill (only user can invoke by name). Stored on ``ParsedSkill`` and echoed on the parse-preview UI; no install consumer because Turnstone hardcodes ``activation="named"`` on source-installs already. The dataclass docstring spells out the no-op so a future reader doesn't try to wire a translation that's already implicit. * ``user-invocable: false`` — skill stays available to the model but disappears from the user-facing picker. Mapped to ``hidden_from_menu=true`` on ``prompt_templates`` (column pre-allocated by PR #574); consumed by ``list_skills_summary`` (both the standalone-server and console-server impls). ## Surface * Parser: new ``_extract_bool`` helper accepts every YAML 1.1 boolean spelling (true/false/yes/no/on/off/1/0) plus their quoted variants — caught by ``/review`` as a real gap, since YAML's ``safe_load`` returns ``int`` for unquoted ``1``/``0`` and ``str`` for the YAML 1.1 spellings when quoted. * Install handler: derives ``hidden_from_menu`` from ``parsed.user_invocable`` on the source-install path. * Admin: ``CreateSkillRequest`` / ``UpdateSkillRequest`` accept ``hidden_from_menu``; both modals get a checkbox; the parse-preview auto-fill flips it when the source SKILL.md sets ``user-invocable: false``. * Runtime config: ``hidden_from_menu`` joined ``SKILL_RUNTIME_CONFIG_FIELDS`` so admin can override on installed (readonly) skills — same precedent as ``model`` / ``effort``. ## list_skills_summary shared helper Two identical implementations of ``list_skills_summary`` had accreted in ``turnstone/server.py`` and ``turnstone/console/server.py``. Both needed the new ``hidden_from_menu`` filter, so extracted the shared body to ``turnstone/core/web_helpers.skill_summary_rows``. Future spec-uplift fields (e.g. #572's ``argument_hint`` for autocomplete) only touch one place now. ## Tests * ``TestInvocationControl`` — bool / quoted / YAML 1.1 / int variants across both fields * ``test_install_user_invocable_false_sets_hidden_from_menu`` + default-unhidden case * ``test_list_skills_summary_excludes_hidden_from_menu`` — picker filter, admin tab unaffected * ``test_update_skill_readonly_hidden_from_menu_allowed`` — admin can hide/unhide installed skills via PUT (pins the runtime-config membership invariant) * Existing parse-API fixture extended with both new fields plus default-case assertions |
||
|
Patrick Buckley
|
4c8f5acd3e |
feat(skills): ingest when_to_use / model / effort from SKILL.md (#570)
The Anthropic Claude Code skill spec defines three frontmatter fields the parser was previously dropping; this PR wires them through to the existing storage shape so the SKILL.md author's intent survives the import. * ``when_to_use`` — concatenated into ``description`` at parse time with a ``\n\nWhen to use: `` separator. Kept as its own field on ``ParsedSkill`` so the admin parse-preview UI can surface it separately. * ``model`` — passed through to ``create_prompt_template(model=...)`` on the source-install path, seeding the existing ``prompt_templates.model`` column. * ``effort`` — same shape, translates to the existing ``reasoning_effort`` column at the install handler boundary. Re-install short-circuits at the source_url dedup, so admin overrides to either column survive an upstream re-install — covered by a new ``test_reinstall_preserves_admin_model_override`` test that pins the load-bearing invariant. ## Description length cap ``_MAX_DESCRIPTION_LEN`` exported as ``MAX_SKILL_DESCRIPTION_LEN`` (public name) and raised from 1024 to 1536 to match the spec's combined ``description`` + ``when_to_use`` listing budget. All five write surfaces now import the same constant rather than each carrying their own magic number: * ``skill_parser.MAX_SKILL_DESCRIPTION_LEN`` — parse-time cap * ``console_schemas.CreateSkillRequest.description`` — Pydantic * ``console_schemas.UpdateSkillRequest.description`` — Pydantic * ``console/server.admin_create_skill`` — handler slice * ``console/server.admin_update_skill`` — handler slice * ``core/session._exec_skills_create`` — coordinator tool slice * ``core/session._exec_skills_update`` — coordinator tool slice The coordinator sites (last two) were the bug ``/review`` caught: they still capped at 1024 after the rest of the surface bumped to 1536, so a model-issued ``skills(action='create')`` with a 1025-1536 char description would silently truncate. Sharing the constant closes that desync. ## when_to_use truncation guard The ``when_to_use`` concat reserves room for the separator + at least one character of the appended value; below that budget, the addition is dropped entirely. Previously the naive concat could truncate mid-separator and leave the description ending in a dangling ``\n\nWhen ``. ## Tests * ``TestWhenToUse`` — concat semantics, no-description fallback, 1536 truncation * ``TestModelAndEffort`` — extraction + defaults * ``test_install_seeds_model_and_effort_from_frontmatter`` — install path persists both columns * ``test_install_no_model_or_effort_leaves_columns_empty`` — bare SKILL.md doesn't invent values * ``test_reinstall_preserves_admin_model_override`` — admin edits survive an upstream re-install (dedup invariant) * ``test_parses_full_frontmatter`` / ``test_parses_minimal_frontmatter`` extended with the new field assertions |
||
|
Patrick Buckley
|
21cf42904d |
fix(skills): address Copilot review on #574
Three findings from Copilot's review of PR #574: * `update_prompt_template` (both backends) coerces every other INTEGER-as-bool field (`is_default`, `auto_approve`, `enabled`) but not `hidden_from_menu`. Without coercion, a caller updating with ``hidden_from_menu=True`` writes a Python bool to a SQLAlchemy Integer column, which is driver-dependent on PostgreSQL and a consistency hazard. Coerce to int alongside the existing trio. Added a focused round-trip test that updates with ``True`` / ``False`` and asserts the read-back bool transitions. * `_canonicalize_skill_string_list` docstring describes its own ``None``-collapses-to-``"[]"`` rule but doesn't mention that `admin_update_skill` intercepts ``None`` before the helper is called. Added a note documenting the layered contract: the helper defines normalization (create semantics), the update endpoint layers no-op semantics on top. * HTML label hints used Markdown-style ``paths:`` backticks inside plain HTML, which render as literal backticks in the browser. Replaced with `<code>paths:</code>` on both the create and edit modal Paths inputs. No behaviour change to PR #574's main surface — the bool coercion addresses a latent bug a future consumer would have hit; the docstring + HTML fix are purely cosmetic. |
||
|
Patrick Buckley
|
93af031fc7 |
feat(skills): parse and store Anthropic spec paths + uplift columns
Implements PR1 of issue #569 — parser + storage + admin UI for the Anthropic Claude Code skill spec `paths:` SKILL.md frontmatter field (glob patterns gating model-initiated autoload). The autoload filter that consumes `paths` is deferred to a follow-up PR pending the workstream-CWD design discussion. Migration 056 bundles three additional columns whose consumer PRs are filed but not yet implemented: * `hidden_from_menu` (boolean) — backs the spec's `user-invocable: false` (issue #571). * `arguments` (JSON list) — backs spec `arguments:` named arg slots (issue #572). * `argument_hint` (string) — autocomplete display string (issue #572). The deferred columns surface in `SkillInfo` (response) so consumers can read them, but are deliberately absent from `CreateSkillRequest` and `UpdateSkillRequest` — the create/update handlers don't yet read them and advertising a writable field the handler would silently ignore would be an OpenAPI lie. Surface - Parser: `ParsedSkill.paths` populated from frontmatter; accepts the spec's YAML-list-or-CSV-string shape via the existing `_extract_list` machinery. - Storage: 4 new columns on `prompt_templates`; `SKILL_MUTABLE` extended; `_row_to_dict` calls extended to cast the new bool; protocol + SQLite + PostgreSQL `create_prompt_template` signatures threaded. - HTTP: admin create/update/install/parse handlers plumb `paths` through. Pydantic schemas extended accordingly. - Admin UI: `skill-paths` and `etm-paths` inputs on the create + edit modals; field map and read/write helpers wired across paste-parse, reset, create-send, edit-load, edit-send, and the readonly-disable list. Notable - `_canonicalize_skill_string_list` collapses the list-or-CSV-or-JSON- string normalization shared between admin_create_skill and admin_update_skill. Treats `None` as no-value so a body containing `{"paths": null}` doesn't CSV-split through `str(None)` and store the literal `["None"]`. Will back `arguments` once #572 wires its consumer. Tests - Parser: TestPaths covers YAML list, CSV string, empty, full- frontmatter integration (tests/test_skill_parser.py). - Storage: round-trip suite covers create + read + update for each of the four new columns on both backends (tests/test_storage_skill_spec_uplift.py). - Helper: focused unit tests for the canonicalizer including the regression-net case for the null-corruption bug (tests/test_canonicalize_skill_string_list.py). - HTTP boundary: extended test_parses_full_frontmatter + test_parses_minimal_frontmatter to assert `paths` survives the admin parse endpoint. |
||
|
Patrick Buckley
|
bf14fc8b45 |
fix(output_guard): harden against domain-camouflaged injection (#560) (#573)
* fix(output_guard): harden against domain-camouflaged injection (#560) Three layered mitigations against the camouflage attack class described in arXiv:2605.22001 (Pai, May 2026), which demonstrates 90.3% evasion on Llama 3.1 8B and 44.4% on Gemini 2.0 Flash against pattern-based detectors: - Sub-agent synthesis is now scanned by output_guard at the sub-agent boundary in _run_agent, in addition to the existing scan at the parent's tool-result loop. Covers all four return paths (clean exit, truncation, context-limit recovery, turn-limit forced synthesis), closing the cross-workstream summary laundering surface. - Adds pair-of-signals camouflage detection: imperative recommendation phrase combined with either an authority frame ("consistent with our risk framework") or a caps action verb (SELL/BUY/TRANSFER/...). New flag camouflaged_injection at medium risk; deliberately partial — the paper's augmented-detector approach recovers only ~10% on Llama-class models, so this is duct-tape pending a semantic-evaluator follow-up. - Bumps output_guard's wall-clock budget default from 5s to 30s and exposes it as judge.output_guard_budget_seconds in ConfigStore, so the expanded regex set has headroom on large tool outputs. * Fix test_budget_kwarg_is_honored to exercise deadline logic path The test previously passed an empty string which short-circuited evaluate_output() before budget_seconds was used. Now uses a non-empty input and monkeypatches time.monotonic() to deterministically verify the deadline path is exercised. |
||
|
Patrick Buckley
|
ad16de001d |
fix(renderer): drop single-$ inline math to stop false positives in prose
Single-$ inline math is too ambiguous in conversational text: currency
amounts ("$5 and $10 each"), shell variables ("$HOME and $PATH"), and
shell prompts all produced false-positive KaTeX spans because the
regex matched any non-$/non-newline span between two dollar signs.
Inline math now requires the unambiguous \(...\) form, which is what
GPT-5 / o-series / Claude with reasoning effort emit by default anyway.
Display math ($$...$$ and \[...\]) is unchanged — the doubled
delimiter has enough mass that ambiguity is not a practical problem.
Three former positive tests are inverted into regression guards so a
future regex change can't quietly resurrect the bug, and new tests
name the currency and env-var cases explicitly. The web env prompt is
updated to advertise \(...\) and to tell the model why $...$ is gone.
|
||
|
github-actions[bot]
|
7404ae46db | chore: download vendored JS files | ||
|
renovate[bot]
|
747ee99220 | chore(deps): update dependency katex to v0.17.0 | ||
|
renovate[bot]
|
291edd005c | chore(deps): update ghcr.io/astral-sh/uv docker tag to v0.11.16 | ||
|
renovate[bot]
|
ce30df2e97 | chore(deps): lock file maintenance | ||
|
renovate[bot]
|
7ab78e4edf | chore(deps): update github actions | ||
|
renovate[bot]
|
1a4cbb90a8 |
chore(deps): update dependency vitest to v4.1.7 (#564)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> |
||
|
Patrick Buckley
|
cdba6dea7a | chore: bump version to 1.6.0a3 v1.6.0a3 | ||
|
Patrick Buckley
|
830305555d |
fix(sse): address PR #561 review round 2
5 follow-up comments from Copilot, all valid: 1. **CRITICAL — snap_seq race with split writer** (concurrency, 001). Round-1's fix lifted snapshot capture into register_listener_with_replay under nested locks, but the WRITER side (on_content_token / on_reasoning_token) still released _ws_lock before calling _enqueue (which bumps _event_id under _listeners_lock). A reader could interleave between writer's release and writer's _enqueue: capture inflight WITH the new text, read STALE _event_id, return snap_seq < new_event_id. The new event's live emit then has _seq > snap_seq, slips past the dedup filter, and double-renders text the snapshot already contained. Fix: move self._enqueue(...) INSIDE the with self._ws_lock: block in both token writers. The inflight mutation and the _event_id advancement are now atomic against any snapshot reader. Lock order _ws_lock (outer) → _listeners_lock (inner via _enqueue) matches the snapshot helpers, so no deadlock. Fan-out's put_nowait calls happen under _ws_lock for token writers — microsecond cost per listener, acceptable for the correctness guarantee. 2. **NIT — stale comment ref to buffered[-1]._event_id** (docs, 002). The comment referenced a local var (buffered) that lives in register_listener_with_replay, not in the events handler. Reworded to describe the cutoff in terms of the last replayed event id and the atomic-against-writers registration. 3. **MODERATE — 401 branch leaves reconnect loop** (bug, 003). The coord's onerror schedules a 5 s CLOSED-state recovery timer unconditionally. In the 401-expired-session branch we close evtSource and showLogin — but the timer still fires 5 s later, observes !evtSource, and calls scheduleReconnect(), which opens a new EventSource that 401s again → infinite reconnect loop while the login overlay is up. Fix: cancel reconnectTimer in the 401 branch. 4. **MODERATE — race test was vacuous** (test_coverage, 004). The previous regression test drained the listener queue after register_listener_with_replay returned, but the helper doesn't backfill buffered events into the queue, so the loop was almost always a no-op and the assertion never executed. Rewrote with a monkey-patched _enqueue that sleeps 50 ms before bumping _event_id — widens the race window deterministically. Verified: the test FAILS on pre-fix code (snap.content has marker but snap.seq=0 < final_event_id=1) and PASSES on post-fix code (writer holds _ws_lock through _enqueue, so the reader blocks until writer fully done). Also pinned the no-backfill contract so a future change adding listener-queue backfill remembers to keep snap_seq the high-water mark. 5. **NIT — except Exception too broad in test** (best_practices, 005). Tightened except Exception: to except queue.Empty: so unexpected exceptions aren't silently swallowed in the drain loop. Tests: - 86 tests in test_sse_reconnect_replay.py + test_session_ui_base.py pass (existing 84 + 2 new race regressions). - Full non-live suite: 6347 passed, 15 skipped, no regressions. - Ruff + mypy clean on changed .py files; JS parses. |
||
|
Patrick Buckley
|
5b449e502d |
fix(sse): address PR #542 review findings
Four issues raised on the merged PR #542, evaluated and fixed: 1. **Truncated-path snap_seq bug (Copilot low-confidence, VALID).** make_events_handler's truncated branch set snap_seq = 0, disabling the live-drain _seq <= snap_seq dedup filter. Any token writer racing between register_listener_with_replay returning and the live drain's first read would land in BOTH the listener queue AND the captured snapshot text (the snapshot is emitted via in_progress_snapshot as the recovery floor), so the client double-renders. Fix lifts the snapshot capture INTO register_listener_with_replay under the same nested-lock acquire as the listener registration + buffer slice + counter read, so the returned snapshot["seq"] is the exact high-water mark the snapshot text corresponds to. Handler now uses snapshot["seq"] as snap_seq on truncated, dropping any token event with _seq <= snap_seq from the live emit. 2. **Lock-held string join in truncated path (Copilot, VALID).** "".join(ui_base._ws_inflight_content) ran inside the with ui_base._ws_lock: block, holding the lock for the duration of the join and blocking on-token writers. Fix (folded into #1's refactor): the new register_listener_with_replay copies the inflight lists under lock and joins outside, matching the existing pattern in register_listener_with_in_progress_snapshot. 3. **_strip_js_comments docstring misclaim (Copilot, VALID).** Docstring claimed the helper preserves "string/regex literals" but the implementation only tracks string delimiters. Fix: docstring updated to call out the regex-literal limitation explicitly + note that current callers don't scan regions containing regex literals. Extending the tracker is left for a future caller that needs it. 4. **Coord scheduleReconnect dead-code regression (Copilot, VALID).** After the PR-D refactor, scheduleReconnect() had no remaining call sites — which meant reconnectAttempts never incremented, wasReconnecting was always false, AND there was no fallback when the browser transitioned the source to CLOSED (hard 4xx after retries, intermediary tearing the connection down with prejudice, etc.). The first failure mode silently broke the post-gap replace-mode refresh of children / tasks / wait indicator / live-badge cache; the second left the coord permanently disconnected on non-transient failures. Fix: - Introduce disconnectedSinceLastOpen flag set in onerror, cleared in onopen. wasReconnecting reads it (with the legacy reconnectAttempts > 0 fallback for the scheduleReconnect-driven case), so the post-gap refresh fires after every reconnect including the common native-reconnect path. - Re-introduce CLOSED-state recovery: onerror schedules a 5 s delayed check via reconnectTimer; if the source is still CLOSED at that point, call scheduleReconnect(), which opens a new EventSource (threading the saved lastEventId via the URL query param so replay still works across the manual reconnect). Cancel/replace successive timers so onerror floods don't pile up multiple checks for the same source. Tests: - New test_truncated_path_snapshot_captures_real_snap_seq pins the snap_seq fix at the helper boundary. - New test_truncated_path_filters_already_in_snapshot_tokens pins the end-to-end dedup invariant — would have caught the double-render under the old code. - Existing test_sse_reconnect_replay.py call sites updated for the new 6-tuple return of register_listener_with_replay. - All 86 tests in those two files pass; full non-live suite (6347 tests) passes; ruff + mypy clean on changed files; both JS files parse-check. |
||
|
Patrick Buckley
|
83a602fef7 |
fix(skills): address /review on flatten — kind validation + stale text
Four /review findings collapsed to one code chokepoint + two
documentation fixes:
1. find's `kind` arg now validated against ``SkillKind`` (matching
create / update's existing pattern at session.py:8298 / :8512).
Closes two failure modes that shared the same root:
- typos (`kind="interactivee"`) silently produced
`kinds=["interactivee", "any"]` filtering to literal-`any` rows
only and masquerading as a narrowed catalog — now returns an
explicit "kind must be one of: ..." error;
- the documented enum value `kind="any"` degenerated to
`kinds=["any", "any"]` which narrowed to literal-`any` rows
instead of returning "every kind" — now collapses to ``None``
so the documented semantic holds.
2. docs/coordinator-skills.md "two-surface model" section rewritten
to reflect the post-flatten reality: kind is metadata, not an
enforcement boundary. The line-67 tools-table row updated from
the long-dead `list_skills` to `skills (action=find)` with the
opt-in kind-filter framing.
3. Three stale "interactive-only" comments in session.py
(:5514, :7857, :8210) that directly contradicted the
`_prepare_skills_load` docstring ("Both kinds can load") — drop
the qualifier so future grep-and-encode hazards don't reintroduce
the rejection.
Tests:
- test_find_kind_invalid_errors — typo case (replaces the silent
degenerate to literal-any-only)
- test_find_kind_any_means_no_filter — documented enum value matches
documented semantic (collapses to None at prepare)
- test_find_kind_narrow_passes_through — valid narrowing values
reach exec as expected
Deferred to release notes (no code change, intentional policy shift):
- skills(action='get') / load can now read full content + scan_report +
allowed_tools on cross-kind rows from any session. Operators with
pre-existing kind=coordinator skills authored under the prior
implicit visibility contract should audit those bodies for
sensitive content (allowed_tools allowlists, embedded credentials,
internal hostnames in examples) before upgrade.
|
||
|
Patrick Buckley
|
9126c2f368 |
refactor(skills): flatten SkillKind enforcement at tool / HTTP layer
Closes #557. SkillKind was authored audience metadata that the
discoverability filter dressed up as a runtime visibility gate. Real
access control is allowed_tools + auto_approve, which apply identically
across kinds. The kind-scoping chokepoints scaled linearly with every
new model-write surface for zero security payoff.
Drop kind consultation from:
- ChatSession._skills_kinds (deleted) and ._lookup_visible_skill
(deleted; callers inlined to storage.get_prompt_template_by_name).
- _exec_skills_find: no longer auto-threads kinds=. The opt-in `kind`
arg is a passable filter (threads [<kind>, "any"]) so the
discoverability win survives without enforcement.
- _exec_skills_get / _exec_skills_load: row lookup is name-only.
Disabled-row gate stays on load (admin quarantine is the actual
boundary). _prepare_task already uses unscoped get_skill_by_name;
session_routes.py already calls storage directly with no kind
check. Both confirmed by the spike, no source change needed.
- tools/skills.json: drop "Coord sessions see / interactive sees"
language; kind arg description re-cast as opt-in discoverability
narrowing.
- storage Protocol docstring + console_schemas.py kind field
description: refresh to reflect passive-metadata role.
Keep:
- SkillKind enum, kind column on prompt_templates, admin Skills tab
editing, kind field in skills.find / skills.get projection. The
field is useful for sorting/grouping at the model layer and as
authored intent.
- storage.list_skills_filtered(kinds=...) parameter — admin-filter
only now; docstring updated to note it's no longer auto-threaded
from the model-tool path.
Design calls:
1. find accepts opt-in `kind` arg: YES. ~5 lines on prepare + exec.
Threads kinds=[<kind>, "any"] only when supplied. Preserves the
model's ability to narrow a browse without enforcing.
2. kind field stays in find/get projection: YES. Already pulled
directly from the row dict in _skills_project_row (session.py
line 8187); the projection survives the flatten unchanged.
Tests:
- Delete TestLookupVisibleSkill (helper gone), the two
TestExecSkillsLoadKindScoping cross-kind reject branches, the two
test_find_kind_scoping_* tests, and test_get_cross_kind_returns_not_found
— the rejections those pinned are gone.
- Add test_find_default_threads_no_kind_filter (kinds=None by default
for both session kinds), test_find_returns_all_kinds_for_session
(interactive sees both interactive- and coord-tagged rows),
test_find_filters_by_kind_when_supplied (opt-in narrowing works),
test_get_returns_row_across_kinds (cross-kind get succeeds),
test_load_works_across_kinds (cross-kind load succeeds in both
directions — the flatten contract), test_load_rejects_missing_skill
(missing-row hint coverage). Keep test_load_rejects_disabled_skill
(admin quarantine still applies), test_load_works_for_coord_on_*
(coord-side load still works on more kinds now).
Storage tests untouched: tests/test_storage_skills_filtered.py
keeps its kinds= coverage (the parameter still works, just no longer
auto-threaded from the model-tool path).
Closes review findings from PR #555 that motivated the rethink:
sec-1 (_prepare_task unscoped) moot, sec-2 (HTTP create unscoped)
moot, sec-3 (no audit on cross-kind probes) moot — there is no
cross-kind concept anymore.
Boundary spike (verified against fresh main at
|
||
|
Patrick Buckley
|
821108310f |
fix(ui): drop local escapeHtml in renderer (Copilot review on #553)
The local-escape posture from the prior commit double-encoded values that inlineMarkdown had already escaped: leading escapeHtml(text) turns `&` into `&`, the local escapeHtml(url) then turned that into `&amp;`, which breaks query-string URLs after browser parse + getAttribute + new URL round-trip. Switch to convention-rename: regex callback params renamed to safeAlt / safeUrl / safeLabel to signal the upstream-escape invariant. The attribute-context lint enforces all future attribute-context concat sites maintain the safe* convention or call escapeHtml explicitly — defense-in-depth preserved without the regression. Two added pin tests verify `&` survives with single (not double) entity encoding through image data-src and link href. Also addresses two test issues from the same review: - Docstring listed `safe[A-Z_]…` but code only checked isupper(). Drop the underscore option (JS uses camelCase anyway). - `_all_attr_names` only recorded attribute-bearing tags, so a bare `<script>` injection would have false-negatived the link- label pin test. Refactored to `_parse_renderer_html` returning both start tags and (tag, attr) pairs. |
||
|
Patrick Buckley
|
849364c49e |
fix(ui): renderer local-escape + attribute-context CI lint (#553)
inlineMarkdown's image and link renderers now escapeHtml each interpolated value (url, alt, label, domain) at the call site instead of relying on the upstream escape pass. Defence-in-depth: a future refactor calling those renderers from outside inlineMarkdown would otherwise silently regress. New CI lint scans renderer.js for `attr="' + ident` patterns; ident must be escapeHtml(...), safe*, or in the reviewer-approved allowlist. Four pin tests use html.parser.HTMLParser to verify attacker URLs and labels don't materialize event-handler attributes on rendered DOM. |
||
|
Patrick Buckley
|
9a98d07d87 |
fix(skills): address /review findings 2/3/4 from PR 555
Three independent fixes flagged by Copilot's review on PR #555: 2. ``update`` auto_approve self-escalation warning false-positive (turnstone/core/session.py:_prepare_skills_update) - The warning was computed against ``existing_auto_approve or proposed_auto_approve`` — meaning an update that explicitly turned auto_approve OFF still triggered the warning because the existing row had it ON. Now computes against the *final state* (``updates["auto_approve"]`` if present, else ``existing.get("auto_approve")``) combined with the final ``allowed_tools`` value. False-positives gone; the inverse case (existing auto_approve=False, update turns it ON without touching allowed_tools) now correctly fires the warning against the inherited allowlist. 3. ``temperature`` validator silent-coerce → explicit error (turnstone/core/skill_field_validation.py:parse_skill_session_config) - Non-numeric temperature input silently coerced to ``None``, unlike ``max_tokens`` / ``token_budget`` which return an error. Numeric-field consistency: temperature now errors on unparseable input with "temperature must be a number between 0 and 2". Range check unchanged; blank / None still → None. 4. Version-snapshot uses max+1, not count+1 (turnstone/core/session.py:_exec_skills_update) - ``count_skill_versions + 1`` re-uses version numbers when any row has been deleted via the existing ``storage.delete_skill_versions`` method, and the schema has no ``(skill_id, version)`` unique constraint to catch the collision. Switched to ``max(list_skill_versions)`` + 1, matching the ``storage.unlock_skill`` pattern. A storage-side atomic allocator is the right architectural fix and is tracked for a future PR. Tests cover both the false-positive and inverse-positive auto_approve cases, the new temperature error path, and the version-numbering edge case where prior versions have been deleted (max diverges from count). |
||
|
Patrick Buckley
|
7085c24530 |
refactor(skills): unify single-row lookup + allow coord-side load
Two changes that share the same kind-scoping touch point.
Lookup unification (closes the bypass Copilot flagged on _exec_skills_load):
- New ChatSession._lookup_visible_skill(name) — single source of truth for
"find me a skill by name, if it's visible to this session". Combines
storage.get_prompt_template_by_name with the kind filter in one call;
returns None for both the missing-row and out-of-kind cases so callers
don't have to branch on the reason.
- _exec_skills_get refactored from inline two-step to one helper call.
- _exec_skills_load refactored from the unscoped memory.get_skill_by_name
to the new helper — the kind-scoping bypass it had (interactive could
load a kind=coordinator skill by name) goes away by construction
because the unscoped path no longer exists on the model-tool surface.
- memory.get_skill_by_name stays available for admin / sub-agent /
rehydrate paths that need full-catalog visibility — those are
deliberate cross-kind callers, not bypass surfaces. Storage exceptions
now propagate from _lookup_visible_skill by design (distinct from the
legacy swallow-and-return-None) so the operator gets a clear signal on
DB outage rather than a misleading "not found".
Coord-side load support:
- _prepare_skills_load no longer rejects coordinator sessions. Parity
with the admin / HTTP create path that already accepts a `skill` body
field on kind=coordinator workstreams — what the operator can do at
create time, the model can now do on its own session. Visibility is
still kind-scoped via _lookup_visible_skill at exec (a coord can only
load {coordinator, any}-tagged skills; interactive can only load
{interactive, any}), matching what `find` / `get` enforce.
The kind-scoping itself is queued for a separate cleanup PR: the marker
turned out to be a discoverability hint that never gated runtime
capability, and the combinatorial complexity (every new model-tool /
HTTP path needs kind awareness) isn't worth the squeeze at this team
size. Follow-up issue to land.
Test coverage:
- TestLookupVisibleSkill — 5 cases: visible / cross-kind / missing /
storage-unavailable / kind=any-on-both-surfaces.
- TestExecSkillsLoadKindScoping — kind-rejection from both directions
(interactive→coord-only, coord→interactive-only), disabled-skill
caller-side gate, and the two new positive coord-load cases (coord
loads kind=coordinator and kind=any).
- Removed test_load_on_coord_session_errors (the rejection it pinned
is gone).
Plus the /review-suggested doc fixes that came with the unification:
- Comment in _exec_skills_load now correctly attributes the disabled
collapse to the caller's enabled check rather than implying the
helper handles it.
- _lookup_visible_skill docstring documents the deliberate
exception-propagation behavior.
|
||
|
Patrick Buckley
|
471d48abd9 |
feat(skills): unify skill + list_skills into dual-kind action-multiplexed tool
Replaces the legacy `skill` (load + search) and `list_skills` tools with a single `skills(action=...)` tool serving both interactive and coordinator sessions. Stacks on the model.skills.write permission introduced in PR 1. Tool surface - `find`: filter by category/tag/risk_level/enabled_only/limit with optional BM25 query ranking; auto-approved on both kinds; kind-scoped at the storage filter (interactive sees interactive+any, coord sees coordinator+any). - `get`: fetch a single skill including content; cross-kind misses collapse to "not found" so a model can't enumerate the other surface by name-probing. - `load`: activate a skill in the current session (interactive-only; coord sessions get an explicit hint pointing at spawn_workstream). - `create`/`update`/`enable`/`disable`: require approval AND model.skills.write; permission re-checked at exec time to catch a revocation between approval and write. - No `delete` — hard-delete stays admin-UI exclusive; tool description documents the soft-delete-via-disable pattern. Defenses on the write surface - Approval cards surface projected risk_level (scanner re-run against the proposed final state) and warn explicitly when allowed_tools + auto_approve combine (auto-fire-on-load consequence is spelled out, not just shown as raw field values). - Toggle preview surfaces existing risk_level + allowed_tools count so re-enabling a critical-tier skill is never a one-click bypass. - Update path now re-fetches the row at exec to catch a readonly flip between approval and write, filters updates back to the runtime-only set if so, refuses if no fields survive. - Update path rejects empty content (hollow-out via emptying bypassed the soft-delete-via-disable invariant), non-list tags, and empty category — failures are loud rather than silent. - Permission denials audit `skill.write_denied` with actor_source=model so probing the permission state leaves a trail. Audit failures log at error (not warning) — a successful write without a row is the exact gap the trail exists to surface. - `_skill_hint` routes both message and system_reminder through escape_wrapper_tags so caller-controlled values can't close the <system-reminder> envelope and let the model fabricate directives in its own future context. Shared validation - `parse_skill_session_config` lifted from console/server.py to turnstone/core/skill_field_validation.py; both the HTTP admin path and the model-tool path consume it. Single source of truth so field rules can't drift between layers. - `SKILL_RUNTIME_CONFIG_FIELDS` lifted similarly (was duplicated as _SKILL_RUNTIME_CONFIG_FIELDS in server.py and _SKILLS_READONLY_FIELDS on ChatSession). - `notify_on_complete` validator now accepts list input from the JSON schema's `array` type — previously rejected because str() of a list yields Python repr that json.loads then refuses. Performance - Update prepare skips the projected-risk scan when neither content nor allowed_tools is changing (storage re-scans on write authoritatively). Metadata-only updates no longer pay the ~25 regex-pass scan cost. Cleanup - CoordinatorClient.list_skills deleted (-91 lines); model-tool path talks to storage directly via list_skills_filtered. - Roles admin UI gains a Model section exposing model.skills.write. - tests/test_load_skill.py renamed to tests/test_skills_tool.py and rewritten for the new tool — 48 tests covering registration, prepare dispatch, permission gating (including TOCTOU-revoked exec deny), audit actor_source on create + disable + permission-denied probe, BM25 ranking, invalid-kind branches, audit-failure swallow, and <system-reminder> envelope injection resistance. |
||
|
Patrick Buckley
|
ecae0f8778 |
feat(auth): add model.skills.write permission and user_has_permission helper
In-process permission check for model-facing tool exec paths that need to gate a write capability without HTTP middleware in the loop. Foundation for the upcoming skills tool refactor: the merged skills(action=create|update|enable|disable) tool will gate on model.skills.write before reaching storage. - Add model.skills.write to _VALID_PERMISSIONS (default-ungranted on every role including builtin-admin — operators opt themselves in explicitly) - Add user_has_permission(user_id, permission, *, storage=None) helper that fails-closed on storage outages and short-circuits on empty user_id - Document service-scope asymmetry with require_permission (no AuthResult in the model-tool path → no bypass; explicit guidance if a legitimate service-scope caller ever needs to reach here) - Pin the "no implicit cache" contract with a regression test asserting every helper call hits storage (call_count == 2 after two calls) - Lock the "builtin-admin default-ungranted" invariant with an alembic migration test that drives the chain to head and asserts the role's permission string omits model.skills.write - Plus the role-create end-to-end test proving the constant flows through the admin endpoint's validator Roles admin UI changes deferred to the PR that lands the gated tool — no operator action needed until the capability exists. Per-call DB hit + warning-log spam on outage deferred to a follow-up PR; the helper is dead code in this commit, so cache TTL would be sized against guesswork — better to wait for a real call-rate signal from the first caller. |
||
|
Patrick Buckley
|
03afb82369 |
chore(deps): raise starlette floor to 1.0.1 (PYSEC-2026-161)
Starlette 1.0.0 reconstructs request URLs without validating the Host header, allowing path-injection that can bypass authentication on apps comparing reconstructed URL paths instead of `request.url.path`. Fixed in 1.0.1. - pyproject.toml: bump `starlette>=0.45` to `starlette>=1.0.1` so the CVE floor is explicit at the dependency declaration, not just in the lockfile. Annotated with the advisory ID so the rationale survives a future floor relax. - uv.lock: regenerated via `uv lock --upgrade-package starlette`; starlette 1.0.0 -> 1.0.1, no transitive bumps. Locally verified `pip-audit --strict` returns clean after the bump and the auth + service-boundary test suites (250 tests covering the URL/ host-header reconstruction surface) continue to pass. |
||
|
Patrick Buckley
|
79eeb25e3f |
feat(sse): raise default event buffer cap 2000 -> 50000
2000 was sized for the cloud-provider regime (50–200 events/sec) and was too small for the two regimes that actually shape PR-D's recovery floor: 1. **Local inference**: vLLM / llama.cpp hit 500–2000 tok/s per active stream. Each token is an _enqueue call, so a single busy workstream burns through 2000 events in ~1 s. Reconnects after any disconnect longer than a network blip immediately fall through to the replay_truncated recovery path on a stream that was supposed to be transparently resumable. 2. **Backgrounded tabs**: Chrome (and Firefox to a lesser extent) throttle the SSE-drain microtask aggressively when a tab isn't visible — Chrome's background-tab budget drops to ~1 wake/min after ~5 min hidden, so a backgrounded pane can legitimately sit on tens of seconds of un-drained events. PR-G (drop-pings- let-it-die) deliberately closes those connections on hide and re-opens on focus return; reconnect-with-replay is the only recovery path, and if the buffer evicted in the interim, the snapshot floor is all that's left for past-turn structural events (tool calls, state changes, approvals). 50000 at the 2000-tok/s local-inference rate buys ~25 s of pure token streaming before truncation; at cloud rates it's minutes of coverage. Memory cost is ~200–500 bytes per event (deque node + dict + payload), so 50000 × 100-ws design ceiling caps at roughly 2.5 GB worst-case — and practically nowhere close because the cap is per-ws ceiling, not per-ws steady-state. Operators on heavier workloads can raise via TURNSTONE_SSE_EVENT_BUFFER_MAX. Considered and rejected: in-buffer coalescing of consecutive content/reasoning tokens. A naive text-merge breaks the replay- slice semantic — a coalesced entry has the latest _event_id but text that includes content the client already received under an earlier id, so any consumer with last_event_id falling INSIDE the coalesced span would double-render on replay. A correctness-preserving coalesce would need a per-consumer high- water tracker we deliberately don't maintain. Bigger cap + simple per-event storage avoids the trap; the rationale is captured inline in _resolve_event_buffer_max. |
||
|
Patrick Buckley
|
2dd71fb869 |
feat(ui): browser onerror preserves native EventSource reconnect
The browser-side completion of PR-D reconnect-with-replay. Today's `onerror` handlers on `Pane.connectSSE`, `connectGlobalSSE`, and the coordinator's `connectSSE` all explicitly call `evtSource.close()` on the transient-error path — that forces the source into the terminal CLOSED state, defeating EventSource's native auto-reconnect (which would otherwise reconnect with the `Last-Event-ID` header that PR-D commit 1 now honours server-side). Three handler refactors share the same shape: - Remove the unconditional `close()` from the transient-error branch. Native EventSource handles CONNECTING -> CONNECTING -> OPEN with replay automatically. - Keep UI updates (status bar dim, Reconnecting… text) — those are orthogonal visualizations of the disconnected state. - Keep terminal-branch closes: a 401 expired-session still does an explicit close + showLogin (the user must re-authenticate); a workstream-reassignment to a different ws still disconnects + connects on the new wsId (it's a different stream, not a same- stream replay). - Capture `lastEventId` in `onmessage` BEFORE `JSON.parse` so a malformed event doesn't desync the manual-reconnect fallback from native auto-reconnect. - Thread `?last_event_id=N` on the URL when constructing a fresh `new EventSource(url)` — the constructor can't set custom headers so the query-param fallback covers the manual-reconnect path (initial connect with a saved id, scheduleReconnect after an explicit close, etc.). For `Pane.connectSSE`, the long focused-pane workstream-refetch body inside `onerror` is lifted to a dedicated `_refetchWorkstreamsAndReassign` method so it survives the refactor as an orthogonal trigger (handles the workstream-evicted- during-disconnect recovery case, which is independent of the SSE reconnect mechanics). The reassignment branch's existing `disconnectSSE + connectSSE(newWsId)` sequence stays — different workstream genuinely needs a fresh stream. When reassigning, the saved `_lastEventId` is dropped because replay is per-ws and an id from ws-A is meaningless against ws-B. Tests in `tests/test_app_js.py` add 3 static lint guards that fail loudly if any future refactor reintroduces a naked `evtSource.close()` in a transient-error path of any of the three handlers. The guards understand the allowed terminal-branch exceptions (401, login overlay, reassignment) and ship with an escape hatch (functions that explicitly reference `last_event_id` have taken explicit responsibility for the replay header and are exempt). A small `_strip_js_comments` helper handles the apostrophe-in-comment hazard that pre-existing `_slice_balanced_body` doesn't (comments are stripped before brace-walking; offsets preserved by space substitution). |
||
|
Patrick Buckley
|
6b6c8eb263 |
feat(console): forward Last-Event-ID through SSE proxy
The console SSE proxy (`_proxy_sse`) is the inbound SSE path for
multi-node deployments — every browser EventSource that targets a
per-node route traverses it. Today's proxy strips client request
headers (only `Accept`, `Cache-Control`, and the re-minted auth
token make it upstream), so the per-ws / global SSE handlers'
`Last-Event-ID` resume (PR-D commit 1) never sees the header in
the multi-node shape — every reconnect would be a fresh connect
and silently drop events from the disconnect window.
Builds the upstream headers dict conditionally: copy `Last-Event-ID`
from the incoming request when present, omit otherwise (no
fabricated value on fresh connects). Starlette's header dict is
case-insensitive so the `request.headers.get("last-event-id")`
lookup catches both the spec-recommended capitalization and any
intermediary normalisation.
The query-param fallback (`?last_event_id=N`) needs no proxy
change — `request.url.query` is already forwarded verbatim at the
top of the function.
Tests in `tests/test_service_auth_boundary.py::TestProxySseLastEventIdForwarding`:
- Positive: browser header → upstream header (value preserved).
- Negative: browser sends nothing → upstream gets nothing (no
fabricated value).
|
||
|
Patrick Buckley
|
08f6f146bc |
feat(sse): per-ws ring buffer + Last-Event-ID replay foundation
Adds the server-side foundation for SSE reconnect-with-replay (PR-D in issue #540's sequencing): a per-ws monotonic ring buffer that holds the last N events for replay against a client's `Last-Event-ID` header (or `?last_event_id=N` query-param fallback for manual reconnect paths that can't set custom headers). Per-ws lane (SessionUIBase + make_events_handler): - `_event_buffer` deque (cap 2000, env-overridable via `TURNSTONE_SSE_EVENT_BUFFER_MAX`) holds (event_id, event_dict) tuples; `maxlen` evicts the oldest automatically. - Existing `_ws_inflight_seq` renamed to `_event_id` and lifted to live alongside the listeners — one monotonic counter drives both the new replay slice AND the existing `_seq`/`snap_seq` snapshot dedup (byte-identical contract on token events). - `_enqueue` now stamps every event with `_event_id` (and `_seq` on `content`/`reasoning` token events) under `_listeners_lock`, so the buffer append + listener fan-out + new listener registration are all atomic against each other. - New `register_listener_with_replay` returns (queue, replay_events, status, lost_count, earliest_id) where status ∈ {replay_ok, truncated}. `make_events_handler` reads `Last-Event-ID` (header or query), branches three ways (fresh / replay_ok / truncated), and emits the SSE `id:` field on every event sourced from the buffer. On `replay_ok` the in-progress snapshot is skipped (the buffered events already cover it); on `truncated` an explicit envelope precedes the fresh-style recovery path. - Every events stream emits a jittered `retry:` in [2500, 4500] ms on first yield so 6-pane reconnects don't lockstep on EventSource's default ~3 s interval. Global lane (server.py / _global_fanout_thread / global_events_sse): - Parallel buffer + counter on `app.state.global_event_buffer` and `app.state.global_event_id_holder`; fanout thread stamps each event with `_event_id` and appends to the buffer under `global_listeners_lock`. `global_events_sse` branches on `Last-Event-ID` with the same three shapes. Tests: - 16 new tests in `tests/test_sse_reconnect_replay.py` cover the ring buffer semantics (empty-listeners hold, last_event_id slicing, truncation, atomic registration), the counter invariants (monotonic under concurrent writers, no skip on queue.Full, persists across turn boundaries, cross-thread consistency), and the handler branching (retry on first yield, id: on buffered events, snapshot-skip on replay_ok, envelope on truncated, query-param fallback, malformed header → fresh). - Existing `tests/test_session_ui_base.py` updated for the `_ws_inflight_seq` → `_event_id` rename and the new `_event_id` field on enqueued events. Backward-compat: all consumers that don't send `Last-Event-ID` (today's browser, Python SDK, TypeScript SDK, channel adapter) see behaviour identical to pre-PR — the server change is purely additive on the request side. |
||
|
Patrick Buckley
|
5042b0cfdb | chore: bump version to 1.6.0a2 v1.6.0a2 | ||
|
Patrick Buckley
|
4117f45067 |
test(ci): allow whitespace before \( in insertAdjacentHTML lint clause (post-review)
Mirror the \s* posture used by the other unsafe-sink clauses (eval\s*\(,
Function\s*\(, setTimeout\s*\() so a regression like
``el.insertAdjacentHTML ("beforeend", x)`` — or a multi-line form with a
newline before the paren — still trips the lint. The trailing ``HTML``
literal continues to discriminate against insertAdjacentElement and
insertAdjacentText.
Caught by Copilot review on #541.
|
||
|
Patrick Buckley
|
67ca3a5ba7 |
test(ci): broaden insertAdjacent-HTML lint, retire renderVerdictBadge carve-out
Extend _UNSAFE_CODE_SINK_RE with an `.insertAdjacent` + `HTML\(` alternation so insertAdjacentHTML(...) is flagged across all 8 tracked JS bundles. The `HTML\(` suffix excludes insertAdjacentElement, which takes a DOM node and is not an XSS sink — the five remaining sites in ui/static/app.js (lines 170, 216, 328, 330, 1578) stay clear. Retire the two carve-out paragraphs (file-level comment + function docstring) that named ui/static/app.js's verdict-badge writers as the reason the lint hadn't already broadened. Commit 1 of this PR cleaned both writers, so the carve-out is no longer load-bearing. After this commit the DOM-cleanup arc (started in #532) is complete: every unsafe-write sink family — inner/outer-HTML assignment (plain + concat), insertAdjacentHTML, document.write, string-eval, dynamic- Function, string-first-arg setTimeout/setInterval — is forbidden across all 8 LLM-rendering bundles. |
||
|
Patrick Buckley
|
c3ddcd0b24 |
refactor(ui): renderVerdictBadge returns DocumentFragment, callers use appendChild
Rewrite the verdict-badge HTML builder from string-concat into DOM
construction (createElement + textContent + setAttribute + append).
The helper now returns a DocumentFragment of two top-level siblings
(.verdict-badge and .verdict-detail), which appendChild expands into
the parent — preserving the sibling-traversal invariants relied on by
Pane.updateVerdictBadge, toggleVerdictDetail, and the d-key keyboard
shortcut.
Inline onclick="toggleVerdictDetail(this)" replaced with an
addEventListener click handler; the non-arrow callback keeps the
`this`→button binding the old inline form had.
Both call sites (replayHistory + the live approval flow) swap from
el.insertAdjacentHTML("beforeend", X) to el.appendChild(X).
This is the last unsafe-write site in the DOM-cleanup arc started in
#532; commit 2 broadens the test_app_js.py lint regex to forbid the
insertAdjacent-HTML sink across all 8 tracked JS bundles.
|
||
|
Patrick Buckley
|
dac541d304 | fix(ui): close SSE connections on beforeunload to unblock multi-pane refresh (#539) | ||
|
Patrick Buckley
|
ba57d6f7c9 |
fix(ui): _paneCounter const-reassign + harden lint test (post-review)
The pre-push /review pass surfaced a second const-reassign that mirrors
the original `redacted` bug but in prefix-increment form:
const _paneCounter = 0; // turnstone/ui/static/app.js:10
class Pane {
constructor(wsId) {
this.id = "p" + ++_paneCounter; // line 14 — TypeError at runtime
…
}
}
`new Pane(...)` throws `TypeError: Assignment to constant variable.`
on every pane construction. The first iteration of the const-reassign
guard in tests/test_app_js.py missed it because the regex matched
postfix `X++` / `X--` but not prefix `++X` / `--X`.
Two changes:
1. Change `const _paneCounter = 0` to `let _paneCounter = 0` at
turnstone/ui/static/app.js:10. Same fix shape as the `redacted`
bug — original walker tightened to const because its reassignment
regex also only matched postfix forms.
2. Extend the reassignment regex in test_swept_bundle_has_no_const_reassign
to detect prefix `++X` / `--X` so a third repeat of this class
can't ship. Verified by injection: temporarily reverting (1)
makes the new guard fire with a clear source-text diagnostic.
Quality polish on the same test (q-1/q-2 from the pre-push pass):
- Failure message now prints the offending decl + reassignment line
text alongside line numbers, so CI failures are self-contained
(was: opaque tuples requiring two file-jumps to interpret).
- Comment on `_SWEPT_BUNDLES` documents the maintenance contract
(add only after sweeping; coordinator.js intentionally excluded).
|
||
|
Patrick Buckley
|
20895aa6e0 |
test(ci): pin var-free + const-reassign invariants across 7 swept JS bundles
After the var → const/let sweep, four guards keep the post-sweep state
honest in CI:
1. node --check per bundle (parse-level smoke; catches a future edit
that drops a brace or mis-balances a string before it reaches the
browser).
2. Static var-free assertion per bundle pins the keyword-swap result —
any future `var X = …` in these 7 files fails CI loudly.
3. Scope-aware static const-reassign guard per bundle. For each
`const X = …`, scans only the enclosing block (innermost { … } via
brace tracking with regex/string/comment awareness) for X
reassignments, so a same-named `let X` in an unrelated function
doesn't false-positive against a `const X` in this one. Catches
the bug class that shipped through the original sweep:
_redactApiKeys's `const redacted; redacted = …` threw TypeError
at call-time, invisible to node --check.
4. Runtime smoke for _redactApiKeys via `node -e` — calls the
function with both query-string (`api_key=…`) and JSON
(`"api_key": "…"`) shapes. This is the bit that would have
caught the actual shipped TypeError; (3) is the equivalent
static check that catches the class without needing a runtime
invocation.
Bundle list:
- turnstone/ui/static/app.js
- turnstone/console/static/admin.js
- turnstone/console/static/governance.js
- turnstone/console/static/app.js
- turnstone/shared_static/auth.js
- turnstone/shared_static/kb.js
- turnstone/shared_static/utils.js
Verified by injection: temporarily reverting `let redacted` to
`const redacted` makes both guard (3) and guard (4) fail loudly.
|
||
|
Patrick Buckley
|
5053ab5611 |
refactor(console): scope-aware const-tightening pass on 3 swept bundles
Follow-up to the initial var-sweep commits. The walker used a flat, file-wide reassignment check to decide let vs const, which was conservative when the same name appeared in multiple unrelated scopes — e.g. `let i` as a loop counter in one function and an unrelated `let i` reassigned in another would both stay `let`. This second pass uses brace-tracking block-scope analysis (regex literal aware) so tightening considers only reassignments within the same block: - console/static/app.js: +5 const -5 let - console/static/governance.js: +15 const -15 let - console/static/admin.js: +26 const -26 let Mirrors q-2 from the /review pipeline. ui/static/app.js was tightened in the same way already in its sweep commit. Mechanical; no behavioural change. |
||
|
Patrick Buckley
|
14e0197e74 |
refactor(ui): ui/static/app.js — var → const/let sweep
754 line-start var + 50 for-loop counters converted: 663 const, 96 let
(line-start), plus 50 for-init let counters.
Hand-fix sites (surfaced by spike § 2):
- 4 try-block hoists where the var was referenced from outside the
try (var hoists out, let does not):
- tryParseMedia()'s `obj`
- _tryPrettyJson()'s `obj`
- tryParseMcpError()'s `obj`
- inline-plan render's `action` (used in the catch handler)
- showNewWsModal() cleanup (was: 2 same-scope redeclarations):
- submitBtn — first lookup at the top of the modal kept; the
redundant re-fetch + duplicate textContent at the bottom
dropped; submitBtn.disabled = false now sits as a bare
property write
- defaultOpt → renamed second occurrence to tplDefaultOpt
(genuinely distinct DOM element — modelSelect vs tplSelect),
both can be const
Post-review fix to the walker output:
- _redactApiKeys(): the walker tightened `let redacted` to `const`
but missed the `redacted = redacted.replace(...)` reassignment
on the JSON-style pass. Root cause was the walker's
find_decl_extent not recognising JS regex literals — the
unescaped " inside the character class [^&\s"] opened an
in_str state that never closed on the same line, spilling
the declaration span past `);` and pulling the reassignment
line into the skip set. The /review pipeline's bug finder and
security finder both caught it (rendering would have thrown
TypeError on every tool-output render). Now `let redacted`.
Scope-aware const-tightening pass on top of the walker (mirrors q-2
from /review): 45 additional `let` → `const` flips where the walker
was conservative because the name happened to be reassigned in an
unrelated function elsewhere in the file. Examples: `let pane` in
the 4 plan-dialog helpers; `let el` in the small Pane class methods.
Each tightening is verified safe by a brace-tracking block-scope
analysis (regex-literal aware).
Mechanical; no behavioural change.
|
||
|
Patrick Buckley
|
52d716658f |
refactor(console): admin.js — var → const/let sweep
744 line-start var + 87 for-loop counters converted: 606 const, 138 let. Includes 2 multi-decl sites (counter accumulators at 3389 and 4021, both `let` because the names are reassigned via += in the loop body), and the spike-identified `indicator` redeclaration in `_toggleOidcPanel` (now two `const indicator` declarations in disjoint block scopes — inner if-block at 455 and function body at 482, so block-scoping makes them independent). Mechanical; no behavioural change. |
||
|
Patrick Buckley
|
d3f9e78715 |
refactor(console): governance.js — var → const/let sweep
470 line-start var + 64 for-loop counters converted: 337 const, 133 let. Includes 2 multi-decl sites (`let url, method;` at 3951 and 4504 — both uninitialised pairs that stay `let`) and two sibling `for (var k …)` loops at lines 112/119 in the same function (now `for (let k …)` — block-scoped to each loop init, no collision). Mechanical; no behavioural change. |
||
|
Patrick Buckley
|
8025d24b63 |
refactor(console): console/static/app.js — var → const/let sweep
270 line-start var + 5 for-loop counters converted: 232 const, 38 let. Includes 3 multi-decl sites correctly handled: - `let totalTokens, totalToolCalls, totalWs` (counter accumulators) - `let mcpServers, mcpResources, mcpPrompts` (counter accumulators) - `const au, bu` (sort comparator helpers — never reassigned) The walker extends the const-tighten reassignment check across multi-line declarations, so continuation lines (`bu = b.updated || 0,` belonging to a `let au = …,` decl) aren't mis-counted as reassignments of `bu`. Mechanical; no behavioural change. |