mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(server): address PR #409 review feedback
PR #409 line-level review feedback. Three of four findings valid; the fourth (code-quality bot's "unused TYPE_CHECKING imports") verified as false-positive — removing the imports breaks mypy on the string-form annotations in ``ManagerLookup`` / ``TenantCheck`` / ``CloseAuditEmitter``. CI lint failure (ruff format on ``tests/_coord_test_helpers.py``) addressed alongside. Findings addressed: - **Copilot #1** (``session_routes.py`` SessionEndpointConfig docstring): said the config is "stored on ``app.state.session_endpoint_config``" and "handler bodies pull this config from app.state". Stale after the previous fixup switched the factories to capture ``cfg`` via closure. Rewrote the class docstring + the lifted-handler comment block + the module docstring + the ``create_app`` block comments in both ``server.py`` and ``console/server.py``. - **Copilot #2** (``server.py:_interactive_manager_lookup`` docstring): referenced ``:data:SessionRouteHandlers`` which was renamed to ``SharedSessionVerbHandlers`` AND wasn't the right reference anyway — the callable matches ``SessionEndpointConfig.manager_lookup``. Fixed. - **Bonus**: dropped the now-dead ``app.state.session_endpoint_config = ...`` assignments in both servers (nothing reads them since the closure-capture switch). - **Bonus**: dropped the stale "close (interactive caps + redacts + persists close_reason)" entry from the deferred-verbs comment in ``session_routes.py`` — close was lifted in the previous commit and is no longer in the deferred set. - **CI lint**: ``ruff format`` joined the ``MockStorage.list_services`` signature in ``tests/_coord_test_helpers.py`` to a single line (95 chars, fits the 100-char limit). ruff + ruff format + mypy clean. 88 affected tests pass.
This commit is contained in:
committed by
Patrick Buckley
parent
74670cd53e
commit
abf7f62301
@@ -114,7 +114,5 @@ class MockStorage:
|
||||
def __init__(self) -> None:
|
||||
self.services: list[dict[str, str]] = []
|
||||
|
||||
def list_services(
|
||||
self, service_type: str, max_age_seconds: int = 120
|
||||
) -> list[dict[str, str]]:
|
||||
def list_services(self, service_type: str, max_age_seconds: int = 120) -> list[dict[str, str]]:
|
||||
return list(self.services)
|
||||
|
||||
@@ -10106,13 +10106,13 @@ def create_app(
|
||||
_docs_handler = make_docs_handler()
|
||||
|
||||
# Coord workstream HTTP tree mounts under the unified
|
||||
# ``/api/workstreams/`` shape. Lifted handlers (e.g. ``approve``)
|
||||
# consult ``app.state.session_endpoint_config`` for the kind's
|
||||
# auth + manager-lookup policies. Per-kind handlers
|
||||
# (``coordinator_*``) still look the coord manager up via
|
||||
# ``request.app.state.coord_mgr`` because the manager is built in
|
||||
# the lifespan, after this app construction; future verb lifts
|
||||
# carry that lookup into the config callable.
|
||||
# ``/api/workstreams/`` shape. Lifted handlers (e.g. ``approve``,
|
||||
# ``close``) capture the kind-specific ``SessionEndpointConfig``
|
||||
# via the factory closure. Per-kind handlers (``coordinator_*``)
|
||||
# still look the coord manager up via ``request.app.state.coord_mgr``
|
||||
# at request time because the manager is built in the lifespan,
|
||||
# after this app construction; future verb lifts carry that lookup
|
||||
# into the config callable.
|
||||
coord_endpoint_config = SessionEndpointConfig(
|
||||
permission_gate=_require_admin_coordinator,
|
||||
manager_lookup=_require_coord_mgr,
|
||||
@@ -10593,7 +10593,6 @@ def create_app(
|
||||
lifespan=_lifespan,
|
||||
)
|
||||
app.state.collector = collector
|
||||
app.state.session_endpoint_config = coord_endpoint_config
|
||||
app.state.jwt_secret = jwt_secret
|
||||
app.state.auth_storage = auth_storage
|
||||
app.state.proxy_token_mgr = proxy_token_mgr
|
||||
|
||||
@@ -5,7 +5,8 @@ Both node and console processes mount the workstream HTTP tree at
|
||||
:class:`~turnstone.core.session_manager.SessionManager` (interactive
|
||||
on the node, coordinator on the console). One URL shape, two
|
||||
processes, kind-specific policy in :class:`SessionEndpointConfig`
|
||||
that handlers consult at request time via ``app.state``.
|
||||
captured by closure when the handler factory is called at app
|
||||
construction.
|
||||
|
||||
Three registrar functions:
|
||||
|
||||
@@ -20,10 +21,12 @@ Three registrar functions:
|
||||
that doesn't exist on interactive workstreams.
|
||||
|
||||
Some verbs in :class:`SharedSessionVerbHandlers` ship as factory-
|
||||
returned closures (e.g. :func:`make_approve_handler`) that bake the
|
||||
:class:`SessionEndpointConfig` in at app-construction time. Both
|
||||
node and console call the factory during startup and pass the
|
||||
result as ``handlers.approve``.
|
||||
returned closures (e.g. :func:`make_approve_handler`,
|
||||
:func:`make_close_handler`) that bake their
|
||||
:class:`SessionEndpointConfig` (and any verb-specific args like
|
||||
``audit_emit``) in at app-construction time. Both node and console
|
||||
call the factory during startup and pass the result as
|
||||
``handlers.approve`` / ``handlers.close``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -61,12 +64,11 @@ TenantCheck = Callable[
|
||||
class SessionEndpointConfig:
|
||||
"""Per-kind policy the lifted handler bodies consult at request time.
|
||||
|
||||
Instantiated once per process during app construction and stored
|
||||
on ``app.state.session_endpoint_config``. The unified handler
|
||||
bodies pull this config + the kind manager from ``app.state``
|
||||
rather than taking either as a per-request parameter — keeps the
|
||||
handler signatures uniform (``Handler = Request -> Response``)
|
||||
so the registrar mounts them like any other route.
|
||||
Instantiated once per process during app construction and passed
|
||||
to the verb factory (e.g. :func:`make_approve_handler`,
|
||||
:func:`make_close_handler`), which captures it via closure. The
|
||||
request-time handler reads ``cfg`` from the closure rather than
|
||||
``app.state`` so the dependency is visible at the wire-up site.
|
||||
|
||||
- ``permission_gate``: kind's pre-handler permission check
|
||||
(e.g. ``admin.coordinator`` for coord, ``None`` for interactive
|
||||
@@ -304,20 +306,18 @@ def register_coord_verbs(
|
||||
#
|
||||
# Each verb here was previously implemented twice (once in
|
||||
# ``turnstone/server.py`` for interactive, once in
|
||||
# ``turnstone/console/server.py`` for coord). The lifted body uses
|
||||
# the kind-specific :class:`SessionEndpointConfig` from
|
||||
# ``app.state.session_endpoint_config`` to branch on the few places
|
||||
# the kinds legitimately differ.
|
||||
# ``turnstone/console/server.py`` for coord). The lifted body
|
||||
# branches on the kind-specific :class:`SessionEndpointConfig` the
|
||||
# factory captured at app-construction time.
|
||||
#
|
||||
# Verbs not lifted yet (intentional — bodies have substantive
|
||||
# behavior divergence that needs SessionManager-side refactoring,
|
||||
# not just kind branching): send (worker dispatch — Priority 1
|
||||
# territory), cancel (interactive does inline forensics + force-cancel
|
||||
# ws._lock manipulation), close (interactive caps + redacts +
|
||||
# persists close_reason), open (interactive resume vs coord rehydrate),
|
||||
# events (different SSE replay shapes), create (interactive
|
||||
# attachments vs coord initial_message), list / saved (different
|
||||
# response keys: ``workstreams`` vs ``coordinators``).
|
||||
# territory), cancel (interactive does inline forensics + force-
|
||||
# cancel ws._lock manipulation), open (interactive resume vs coord
|
||||
# rehydrate), events (different SSE replay shapes), create
|
||||
# (interactive attachments vs coord initial_message), list / saved
|
||||
# (different response keys: ``workstreams`` vs ``coordinators``).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
+5
-6
@@ -815,8 +815,8 @@ def _interactive_manager_lookup(
|
||||
|
||||
Interactive always has the manager loaded (it's constructed
|
||||
synchronously at server startup), so the 503 branch is unused
|
||||
on this side. Match the :data:`SessionRouteHandlers` signature
|
||||
so the lifted handler bodies can call it uniformly.
|
||||
on this side. Matches the :attr:`SessionEndpointConfig.manager_lookup`
|
||||
callable shape so the lifted handler bodies can call it uniformly.
|
||||
"""
|
||||
return request.app.state.workstreams, None
|
||||
|
||||
@@ -4369,9 +4369,9 @@ def create_app(
|
||||
|
||||
# Workstream HTTP tree — owned by the shared registrar in
|
||||
# ``turnstone.core.session_routes`` so the console mounts the same
|
||||
# shape against its coord manager. ``session_endpoint_config``
|
||||
# carries the kind-specific policy (auth, manager lookup, audit
|
||||
# prefix) the lifted handler bodies consult.
|
||||
# shape against its coord manager. The lifted handler factories
|
||||
# (``make_approve_handler``, ``make_close_handler``) capture the
|
||||
# kind-specific ``SessionEndpointConfig`` via closure.
|
||||
interactive_endpoint_config = SessionEndpointConfig(
|
||||
permission_gate=None, # interactive auth is enforced at the middleware layer
|
||||
manager_lookup=_interactive_manager_lookup,
|
||||
@@ -4473,7 +4473,6 @@ def create_app(
|
||||
lifespan=_lifespan,
|
||||
)
|
||||
app.state.workstreams = workstreams
|
||||
app.state.session_endpoint_config = interactive_endpoint_config
|
||||
app.state.global_queue = global_queue
|
||||
app.state.global_listeners = global_listeners
|
||||
app.state.global_listeners_lock = global_listeners_lock
|
||||
|
||||
Reference in New Issue
Block a user