From e8a6b0632d1575bcfee6faeff0de42714957fb6a Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sun, 26 Apr 2026 13:10:05 -0700 Subject: [PATCH] docs: apply Copilot review feedback on PR #418 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three docstring + CHANGELOG drift items from the post-review M3 + Mi1 fixes: - ``make_list_handler`` docstring referenced ``cfg.list_resolve_title`` (singular) but the field renamed to ``list_resolve_titles`` (bulk variant) when the N+1 fix landed. Updated to the plural name + a one-line note about the bulk SELECT pattern. - ``make_saved_handler`` docstring still claimed kind was derived from ``cfg.audit_action_prefix`` string-compare. The Mi1 fix replaced that with the explicit ``cfg.list_kind`` field + fail-loud-on-missing semantic; docstring now describes the current contract. - CHANGELOG ``[Unreleased]`` entry said "Three new ``SessionEndpointConfig`` fields" and listed the singular ``list_resolve_title`` wired to ``get_workstream_display_name``. Updated to "Four" + the bulk plural names + the new ``list_kind`` field with its rationale (distinct from ``audit_action_prefix``; fail-loud on misconfig). The fourth review comment — code-quality bot flagging the ``...`` ellipsis body on the new ``get_workstream_display_names`` Protocol method as "statement has no effect" — is a false positive. ``...`` is the canonical Protocol method body throughout ``turnstone/core/storage/_protocol.py`` (every other method uses it). Refuting; the file's pattern wins over the bot's per-method suggestion. No code changes; docstring + CHANGELOG only. Tests + lint + mypy unchanged. --- CHANGELOG.md | 20 +++++++++++++++----- turnstone/core/session_routes.py | 18 ++++++++++++------ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be71806c..f940ba6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,14 +22,24 @@ Three release tracks are maintained: ``GET /v1/api/workstreams`` + ``GET /v1/api/workstreams/saved`` and coord ``GET /v1/api/workstreams`` + ``GET /v1/api/workstreams/saved`` handlers now share two factory bodies via - ``make_list_handler(cfg)`` and ``make_saved_handler(cfg)``. Three + ``make_list_handler(cfg)`` and ``make_saved_handler(cfg)``. Four new ``SessionEndpointConfig`` fields capture the per-kind divergence: - - ``list_resolve_title: ListResolveTitle | None`` — interactive - wires :func:`turnstone.core.memory.get_workstream_display_name` - so user-set aliases override the auto-generated ws-XXXX name in - the active list. Coord wires ``None`` (no alias surface). + - ``list_resolve_titles: ListResolveTitles | None`` — interactive + wires :func:`turnstone.core.memory.get_workstream_display_names` + (new bulk helper added on the storage layer + ``memory.py``) + so the active-list endpoint resolves every user-set alias in + ONE ``SELECT ... WHERE ws_id IN (...)`` instead of the pre-lift + per-row N+1. Coord wires ``None`` (no alias surface today). + - ``list_kind: WorkstreamKind | None`` — required storage-side + kind classifier passed to ``list_workstreams_with_history``. + Interactive wires ``WorkstreamKind.INTERACTIVE``; coord wires + ``WorkstreamKind.COORDINATOR``. Distinct from + ``audit_action_prefix`` (audit-action namespacing) so adding a + third kind doesn't have to overload the audit prefix as a + classifier; missing value surfaces as 500 with a clear log + line rather than silently filtering for the wrong kind. - ``saved_state_filter: str | None`` — coord wires ``"closed"`` so only explicitly-closed coordinators surface in the saved-card grid. Interactive wires ``None`` (the storage diff --git a/turnstone/core/session_routes.py b/turnstone/core/session_routes.py index 75574265..6a815255 100644 --- a/turnstone/core/session_routes.py +++ b/turnstone/core/session_routes.py @@ -1886,8 +1886,10 @@ def make_list_handler(cfg: SessionEndpointConfig) -> Handler: interactive ``None`` (auth middleware covers it). - ``cfg.manager_lookup`` — already used by every other lifted verb. - - ``cfg.list_resolve_title`` — interactive's user-alias override; - coord ``None``. + - ``cfg.list_resolve_titles`` — interactive's bulk user-alias + lookup; coord ``None``. Single ``SELECT ... WHERE ws_id IN + (...)`` resolves every active row's title in one storage + round-trip (replaces the pre-lift per-row N+1). Always-include row shape: ``{ws_id, name, state, kind, parent_ws_id, user_id}``. SDK consumers don't branch on kind. @@ -1976,10 +1978,14 @@ def make_saved_handler(cfg: SessionEndpointConfig) -> Handler: Per-kind divergence: - ``cfg.permission_gate`` — coord's ``admin.coordinator`` check. - - ``cfg.audit_action_prefix`` — used to derive the kind filter - ("workstream" → INTERACTIVE; "coordinator" → COORDINATOR). - Already wired on every endpoint cfg; reuse here keeps the - cfg surface tight. + - ``cfg.list_kind`` — required ``WorkstreamKind`` passed straight + through to ``list_workstreams_with_history(kind=...)``. The + handler treats a missing value as a configuration error and + surfaces 500 with a clear log line — fails loud rather than + silently filtering for the wrong kind. Distinct from + ``audit_action_prefix`` (audit-action namespacing) so adding a + third kind doesn't have to overload the audit prefix as a + kind classifier. - ``cfg.saved_state_filter`` — coord wires ``"closed"`` so only explicitly-closed coordinators surface; interactive wires ``None`` (any state except the tombstoned ``deleted`` rows the