From de428780a6af392de17c3ef5d3fdb8044bc38a2e Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 12 Aug 2026 11:36:54 -0400 Subject: [PATCH] fix(scopes): close auth and observed-position gaps, paginate membership Review response for #884. Security: - gate `SessionCreate.scopes` behind a workspace-level key; the session-create route is self-authorizing, so a peer- or session-scoped token could mint scope peers and join sessions to scopes it had no access to via `POST /scopes` - refuse a reserved-but-nonexistent name in two observed positions that used the permissive guard: chat `target` and session-context `peer_target`. Both let a caller act on `scope.X` before it existed, then create the scope Facade: - exclude scope peers from `GET /sessions/{id}/peers` and refuse the membership -config read for a real scope, matching its write side - replace `GET /scopes/{id}/sessions` with `POST /scopes/{id}/sessions/list` returning `Page[Session]`; the add route now returns 204. Membership was unbounded on both, while every other list surface paginates - rename `crud.get_scope` to `get_scope_or_raise` Tests: - add a missing-name axis to the route-policy table (`Case.refuse_missing`), which is what surfaced the two guard gaps above - delete 14 hand-written tests the table now enumerates; 52 -> 39 functions in test_scopes.py with more cases covered - tighten the squatter assertion from `!= 422` to `< 400`, which was passing on 5xx - assert the FastAPI-internals traversal still derives positions, so a framework upgrade can't silently empty the suite Docs: - drop internal ticket and RFC references from the published OpenAPI descriptions and surrounding comments; state the behavior instead - move implementation reasoning out of the `PUT /peers/{id}` docstring, which FastAPI publishes, into a comment --- src/crud/__init__.py | 8 +- src/crud/document.py | 11 +- src/crud/peer.py | 11 +- src/crud/scope.py | 64 +-- src/crud/session.py | 22 +- src/dialectic/chat.py | 4 +- src/routers/peers.py | 47 +- src/routers/scopes.py | 50 +- src/routers/sessions.py | 18 +- src/schemas/__init__.py | 2 - src/schemas/api.py | 13 +- src/telemetry/events/trace.py | 4 +- tests/dreamer/test_card_refresh.py | 2 +- tests/routes/test_scope_route_policy.py | 281 +++++++++--- tests/routes/test_scopes.py | 578 +++++++----------------- 15 files changed, 552 insertions(+), 563 deletions(-) diff --git a/src/crud/__init__.py b/src/crud/__init__.py index 4a33094a..c7767f47 100644 --- a/src/crud/__init__.py +++ b/src/crud/__init__.py @@ -50,8 +50,8 @@ from .representation import ( from .scope import ( add_sessions_to_scope, get_or_create_scopes, - get_scope, - get_scope_session_names, + get_scope_or_raise, + get_scope_sessions, get_scopes, remove_session_from_scope, ) @@ -137,8 +137,8 @@ __all__ = [ # Scope "add_sessions_to_scope", "get_or_create_scopes", - "get_scope", - "get_scope_session_names", + "get_scope_or_raise", + "get_scope_sessions", "get_scopes", "remove_session_from_scope", # Session diff --git a/src/crud/document.py b/src/crud/document.py index b3d792f6..8584acd7 100644 --- a/src/crud/document.py +++ b/src/crud/document.py @@ -15,7 +15,7 @@ from sqlalchemy.sql.functions import func from src import models, schemas from src.config import settings from src.crud.collection import get_or_create_collection -from src.crud.peer import get_peer, reject_scope_peers +from src.crud.peer import get_peer, reject_scope_observed from src.crud.session import get_session from src.dependencies import tracked_db from src.embedding_client import embedding_client @@ -958,7 +958,14 @@ async def create_observations( # but it must never be *observed*: scope peers carry observe_me=false and no # representation is ever formed of one. Without this, a conclusion about a # scope persists and a (observer, scope) collection is created for it. - await reject_scope_peers( + # + # The strict variant because this is an observed position, though defence in + # depth rather than the active guard: the loop above resolves every peer, so a + # reserved name that does not exist yet already 404s before reaching here. If + # that validation ever stops covering observed_id, this still refuses the + # pre-seeding case instead of persisting a conclusion that a later-created + # scope would retroactively own. + await reject_scope_observed( db, workspace_name, {obs.observed_id for obs in observations}, diff --git a/src/crud/peer.py b/src/crud/peer.py index 4cccd263..2f81f938 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -72,7 +72,8 @@ def _reject_impossible_peer_names(names: Collection[str]) -> None: too_long = sorted({n for n in names if len(n) > PEER_NAME_MAX_LENGTH}) if too_long: raise ValidationException( - f"Peer name(s) must be at most {PEER_NAME_MAX_LENGTH} characters" + f"Peer name(s) {too_long} must be at most " + + f"{PEER_NAME_MAX_LENGTH} characters" ) @@ -117,7 +118,7 @@ def scope_peer_clause() -> ColumnElement[bool]: ) -async def _reserved_name_candidates(names: Iterable[str]) -> list[str]: +def _reserved_name_candidates(names: Iterable[str]) -> list[str]: """Materialize ``names`` once and return the reserved-prefix ones, sorted. Materializing up front matters: callers pass generators (the message-author @@ -167,7 +168,7 @@ async def reject_scope_observed( Raises: ValidationException: On a real scope or a missing reserved name. """ - candidates = await _reserved_name_candidates(names) + candidates = _reserved_name_candidates(names) if not candidates: return @@ -212,7 +213,7 @@ async def scope_peer_names( Raises: ValidationException: On a NUL byte or an over-length name. """ - candidates = await _reserved_name_candidates(names) + candidates = _reserved_name_candidates(names) if not candidates: return set() @@ -272,6 +273,8 @@ async def get_or_create_peers( Raises: ConflictException: If we fail to get or create the peers + ValidationException: On an impossible name (NUL byte, over-length), or a + reserved-prefix or non-conforming name on the create path """ await get_or_create_workspace(db, schemas.WorkspaceCreate(name=workspace_name)) diff --git a/src/crud/scope.py b/src/crud/scope.py index c4950ef4..c04ac903 100644 --- a/src/crud/scope.py +++ b/src/crud/scope.py @@ -8,8 +8,8 @@ and never speaks. See ``src/utils/scopes.py`` for the namespace helpers. Membership only affects messages ingested *after* a session is added to a -scope; backfill of pre-existing documents and reconciliation on removal land -in a follow-up (DEV-1999). +scope. Conclusions already derived are neither backfilled on add nor +reconciled on removal. """ from logging import getLogger @@ -200,7 +200,7 @@ async def get_scopes( return stmt.order_by(models.Peer.created_at.asc(), models.Peer.id.asc()) -async def get_scope( +async def get_scope_or_raise( db: AsyncSession, workspace_name: str, scope_name: str, @@ -232,31 +232,35 @@ async def get_scope( return peer -async def get_scope_session_names( - db: AsyncSession, +async def get_scope_sessions( workspace_name: str, scope_name: str, -) -> list[str]: + reverse: bool = False, +) -> Select[tuple[models.Session]]: """ - List the IDs of the active sessions that are members of a scope. + Build a query for the active sessions that are members of a scope. + + Membership is unbounded — a scope may span every session in a workspace — so + this returns a query for the caller to paginate rather than a materialized + list. Callers must check the scope exists themselves (``get_scope_or_raise``); + an unknown scope yields an empty page here, not a 404. + + Ordered by membership age, with the session id as a unique tiebreaker: + ``session_peers`` has a composite primary key and no id of its own, so + ``joined_at`` alone is not a stable pagination key. Args: - db: Database session workspace_name: Name of the workspace scope_name: Unprefixed scope name + reverse: Whether to return newest memberships first Returns: - Names of the scope's member sessions, oldest membership first - - Raises: - ResourceNotFoundException: If the scope does not exist + Select for the scope's member sessions """ - await get_scope(db, workspace_name, scope_name) - stmt = ( - select(models.SessionPeer.session_name) + select(models.Session) .join( - models.Session, + models.SessionPeer, (models.Session.name == models.SessionPeer.session_name) & (models.Session.workspace_name == models.SessionPeer.workspace_name), ) @@ -264,10 +268,12 @@ async def get_scope_session_names( .where(models.SessionPeer.peer_name == scope_peer_name(scope_name)) .where(models.SessionPeer.left_at.is_(None)) .where(models.Session.is_active == True) # noqa: E712 - .order_by(models.SessionPeer.joined_at.asc()) ) - result = await db.execute(stmt) - return [row[0] for row in result.all()] + if reverse: + return stmt.order_by( + models.SessionPeer.joined_at.desc(), models.Session.id.desc() + ) + return stmt.order_by(models.SessionPeer.joined_at.asc(), models.Session.id.asc()) async def add_sessions_to_scope( @@ -275,14 +281,15 @@ async def add_sessions_to_scope( workspace_name: str, scope_name: str, session_names: list[str], -) -> list[str]: +) -> None: """ Add sessions to a scope by creating observer memberships for its peer. Each membership is a ``session_peers`` row for the scope peer with ``observe_others=true, observe_me=false`` — exactly what a hand-built - observer peer would carry. Membership only affects messages ingested - after this call (backfill is DEV-1999). + observer peer would carry. No backfill happens here: membership only affects + messages ingested after this call, and conclusions already derived are left + as they are. Args: db: Database session @@ -290,9 +297,6 @@ async def add_sessions_to_scope( scope_name: Unprefixed scope name session_names: Names of existing sessions to add - Returns: - Names of all the scope's member sessions after the addition - Raises: ResourceNotFoundException: If the scope or any named session does not exist @@ -301,7 +305,7 @@ async def add_sessions_to_scope( # `scopes` path, so a module-level import would be circular. from .session import upsert_session_peers - await get_scope(db, workspace_name, scope_name) + await get_scope_or_raise(db, workspace_name, scope_name) requested = set(session_names) result = await db.execute( @@ -328,8 +332,6 @@ async def add_sessions_to_scope( await db.commit() - return await get_scope_session_names(db, workspace_name, scope_name) - async def remove_session_from_scope( db: AsyncSession, @@ -341,8 +343,8 @@ async def remove_session_from_scope( Remove a session from a scope by ending the scope peer's membership. Ends the membership the same way the generic remove-peer path does (sets - ``left_at``). Reconciliation of documents derived while the session was a - member lands in a follow-up (DEV-1999). + ``left_at``). Conclusions derived while the session was a member are left in + place — nothing reconciles them. Args: db: Database session @@ -356,7 +358,7 @@ async def remove_session_from_scope( # Lazy import for the same circular-import reason as add_sessions_to_scope. from .session import remove_peers_from_session - await get_scope(db, workspace_name, scope_name) + await get_scope_or_raise(db, workspace_name, scope_name) await remove_peers_from_session( db, diff --git a/src/crud/session.py b/src/crud/session.py index be1d1647..1fc884ec 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -321,7 +321,7 @@ async def get_or_create_session( # Add the session to any requested scopes: create-or-get each scope peer # and record an observer membership (observe_others=true, observe_me=false). # No backfill happens here — membership only affects messages ingested - # after this point (backfill is DEV-1999). + # after this point. scopes_result = None if session.scopes: scopes_result = await get_or_create_scopes( @@ -919,6 +919,13 @@ async def get_peers_from_session( workspace_name: Name of the workspace session_name: Name of the session + Scope peers are excluded: a scope's membership is the facade's internal + observer wiring, and this is the generic peer surface. Listing them here + would show a caller a peer named ``scope.`` with ``observe_others`` + set, which is exactly the mechanic the facade exists to hide. Mirrors the + ``kind``-less default of ``crud.peer.get_peers``; the scopes routes expose + membership from the other direction. + Returns: Paginated list of Peer objects in the session """ @@ -935,6 +942,10 @@ async def get_peers_from_session( .where(models.SessionPeer.session_name == session_name) .where(models.Peer.workspace_name == workspace_name) .where(models.SessionPeer.left_at.is_(None)) # Only active peers + # models.Peer is already in the FROM via the join above, so the clause + # composes directly — no correlated exists() as in the SessionPeer-only + # UPDATE statements elsewhere in this module. + .where(~scope_peer_clause()) ) @@ -1164,7 +1175,7 @@ async def _get_or_add_peers_to_session( # Scope memberships carry observe_others=True but do not count against the # limit. The limit bounds per-observer deriver fan-out for real peers; a scope - # costs document rows, not LLM calls (Scopes RFC §5.2), and counting them would + # costs document rows, not LLM calls, and counting them would # cap scopes-per-session at SESSION_OBSERVERS_LIMIT and surface as an # observer-shaped 400 through a facade that hides observers entirely. scopes_being_added = await scope_peer_names(db, workspace_name, peer_names.keys()) @@ -1270,7 +1281,14 @@ async def get_peer_config( Raises: ResourceNotFoundException: If the session or peer does not exist + ValidationException: If the peer is a scope """ + # A scope's membership config belongs to the facade, not the caller — the + # write path refuses it in set_peer_config below, and reading it back is the + # same internal wiring by another route. Checked on the resolved row, so a + # legacy peer merely occupying the reserved name keeps working. + _reject_resolved_scope_peers([await get_peer(db, workspace_name, peer_id)]) + # Get row from session_peer table stmt = select(models.SessionPeer).where( models.SessionPeer.workspace_name == workspace_name, diff --git a/src/dialectic/chat.py b/src/dialectic/chat.py index 770f3670..470aaf15 100644 --- a/src/dialectic/chat.py +++ b/src/dialectic/chat.py @@ -25,8 +25,8 @@ def _reject_scope_participants(*peers: models.Peer) -> None: """Refuse a dialectic run whose observer or observed is a scope peer. A scope is a silent observer with ``observe_me=false``: no representation of - one exists to query, and a scope as the path-level observer is a Phase 2b - concern rather than something the raw peer routes expose. + one exists to query. Querying *from* a scope's perspective is a read-side + surface that does not exist yet, and not something the raw peer routes expose. Raises: ValidationException: If any participant is a scope. diff --git a/src/routers/peers.py b/src/routers/peers.py index 6845dbe0..d5425a83 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -142,13 +142,17 @@ async def update_peer( ): """Update a Peer's metadata and/or configuration. - Three-way on the reserved namespace, all enforced inside ``crud.update_peer`` - on the resolved row so there is no check-then-use window: a real scope is - refused (this route replaces `configuration` wholesale, so it must never touch - a facade-managed peer); an existing *unflagged* peer that merely occupies the - namespace is a normal peer and updates fine; and a reserved-prefix name that - does not exist is refused by create-path validation rather than being minted. + Returns 422 if the peer is a scope — use the scopes routes to manage scopes. """ + # Three-way on the reserved namespace, all enforced inside ``crud.update_peer`` + # on the resolved row so there is no check-then-use window: a real scope is + # refused (this route replaces `configuration` wholesale, so it must never touch + # a facade-managed peer); an existing *unflagged* peer that merely occupies the + # namespace is a normal peer and updates fine; and a reserved-prefix name that + # does not exist is refused by create-path validation rather than being minted. + # + # Kept out of the docstring deliberately: FastAPI publishes that into the + # OpenAPI description, and callers need the contract, not the mechanism. updated_peer = await crud.update_peer( db, workspace_name=workspace_id, peer_name=peer_id, peer=peer ) @@ -217,14 +221,19 @@ async def chat( """ # Scope peers are never observed, so no representation of them exists to # query. Covers the path-level observer too: a scope `peer_id` no longer - # errors out downstream now that crud.get_peer takes a plain name, and a - # scope peer as the path-level observer is a Phase 2b concern. + # errors out downstream now that crud.get_peer takes a plain name, and + # querying from a scope's perspective is a read-side surface that does not + # exist yet. scope_candidates = [ n for n in (peer_id, options.target) if n is not None and is_scope_peer_name(n) ] if scope_candidates: async with tracked_db("peers.chat.scope_check", read_only=True) as s_db: - await crud.reject_scope_peers( + # Strict variant, matching the representation route: `target` is an + # observed position and nothing here creates the peer, so a reserved + # name that does not exist yet must be refused rather than answered + # and then turned into a scope. + await crud.reject_scope_observed( s_db, workspace_id, scope_candidates, @@ -401,15 +410,23 @@ async def get_representation( try: embedding: list[float] | None = None if options.search_query: - with ( - suppress(Exception), - embedding_call_purpose( + try: + with embedding_call_purpose( EmbeddingCallPurpose.SEARCH_MEMORY.value, workspace_name=workspace_id, parent_category="api", - ), - ): - embedding = await embedding_client.embed(options.search_query) + ): + embedding = await embedding_client.embed(options.search_query) + except Exception: + # Swallowed on purpose (see include_semantic_query below), but not + # silently: without this a provider outage degrades every search + # request to derived+recent retrieval with no signal anywhere. + logger.warning( + "Representation search embedding failed for workspace %s," + + " degrading to non-semantic retrieval", + workspace_id, + exc_info=True, + ) observed = options.target if options.target is not None else peer_id # Re-check and read in one short session, opened only now — after the diff --git a/src/routers/scopes.py b/src/routers/scopes.py index 8ebc3c22..e3f9d0a2 100644 --- a/src/routers/scopes.py +++ b/src/routers/scopes.py @@ -8,9 +8,9 @@ that keeps the observer/observed mechanics hidden. All scopes routes require a workspace-level (or admin) key: scopes are an app-level admin surface, so peer- and session-scoped keys are rejected. -Note: backfill of pre-existing documents and reconciliation on removal land -in a follow-up (DEV-1999) — for now, scope membership only affects messages -ingested *after* the membership change. +Note: scope membership only affects messages ingested *after* the membership +change. Conclusions already derived are neither backfilled on add nor +reconciled on removal. """ import logging @@ -85,12 +85,13 @@ async def get_scope( db: AsyncSession = read_db, ): """Get a single Scope by ID.""" - return await crud.get_scope(db, workspace_id, scope_id) + return await crud.get_scope_or_raise(db, workspace_id, scope_id) @router.post( "/{scope_id}/sessions", - response_model=schemas.ScopeSessions, + status_code=204, + response_model=None, dependencies=[Depends(require_auth(workspace_name="workspace_id"))], ) async def add_sessions_to_scope( @@ -104,19 +105,19 @@ async def add_sessions_to_scope( """ Add Sessions to a Scope. - All named sessions must already exist (404 otherwise). Returns the scope's - full member session list after the addition. + All named sessions must already exist (404 otherwise). Adding a session that + is already a member is a no-op. List the resulting membership with + `POST /scopes/{scope_id}/sessions/list`. - Note: membership only affects messages ingested after this call — backfill - of pre-existing documents lands in a follow-up (DEV-1999). + Note: membership applies only to messages ingested after this call; + conclusions already derived are not backfilled. """ - session_ids = await crud.add_sessions_to_scope( + await crud.add_sessions_to_scope( db, workspace_name=workspace_id, scope_name=scope_id, session_names=body.session_ids, ) - return schemas.ScopeSessions(session_ids=session_ids) @router.delete( @@ -134,8 +135,8 @@ async def remove_session_from_scope( """ Remove a Session from a Scope. - Note: documents already derived while the session was a member are left in - place — reconciliation on removal lands in a follow-up (DEV-1999). + Note: conclusions already derived while the session was a member are left in + place. """ await crud.remove_session_from_scope( db, @@ -145,16 +146,27 @@ async def remove_session_from_scope( ) -@router.get( - "/{scope_id}/sessions", - response_model=schemas.ScopeSessions, +@router.post( + "/{scope_id}/sessions/list", + response_model=Page[schemas.Session], dependencies=[Depends(require_auth(workspace_name="workspace_id"))], ) async def get_scope_sessions( workspace_id: str = Path(...), scope_id: str = Path(...), + reverse: bool = Query(False, description="Whether to reverse the order of results"), db: AsyncSession = read_db, ): - """Get the IDs of the Sessions that are members of a Scope.""" - session_ids = await crud.get_scope_session_names(db, workspace_id, scope_id) - return schemas.ScopeSessions(session_ids=session_ids) + """Get the Sessions that are members of a Scope, paginated. + + Ordered by how long each session has been a member, oldest first. + """ + # Distinguishes an empty scope from one that does not exist; the query itself + # returns an empty page either way. + await crud.get_scope_or_raise(db, workspace_id, scope_id) + return await apaginate( + db, + await crud.get_scope_sessions( + workspace_name=workspace_id, scope_name=scope_id, reverse=reverse + ), + ) diff --git a/src/routers/sessions.py b/src/routers/sessions.py index 1b8a55e8..b86b5706 100644 --- a/src/routers/sessions.py +++ b/src/routers/sessions.py @@ -318,6 +318,16 @@ async def get_or_create_session( ) session.name = jwt_params.s + # The `scopes` field does what the scopes routes do — create scope peers and + # attach memberships — so it needs their authorization: workspace-level or + # admin only. Checked here rather than through `require_auth(...)` because + # that closure only resolves path and query params, never the body, so a + # declarative gate cannot see this field. + if session.scopes and not ( + jwt_params.ad or (jwt_params.p is None and jwt_params.s is None) + ): + raise AuthenticationException("Scope membership requires a workspace-level key") + # Scope peers may not be added through the generic peers mapping; use the # `scopes` field (which handles scope-peer creation and observer config). if session.peer_names: @@ -746,10 +756,12 @@ async def get_session_context( ) # peer_target is the *observed* peer, and no representation or card is ever - # formed of a scope. peer_perspective (the observer) is left alone: a scope - # is a legitimate perspective, which is what Phase 2b's `scope` option builds on. + # formed of a scope. peer_perspective (the observer) is left alone: a scope is + # a legitimate perspective, and the read-side scope surface will build on that. if peer_target is not None: - await crud.reject_scope_peers( + # Strict variant: an observed position that creates nothing, so a reserved + # name which does not exist yet must be refused too. + await crud.reject_scope_observed( db, workspace_id, [peer_target], diff --git a/src/schemas/__init__.py b/src/schemas/__init__.py index 195ae381..cc00d76a 100644 --- a/src/schemas/__init__.py +++ b/src/schemas/__init__.py @@ -38,7 +38,6 @@ from src.schemas.api import ( ScheduleDreamRequest, Scope, ScopeCreate, - ScopeSessions, ScopeSessionsAdd, Session, SessionBase, @@ -139,7 +138,6 @@ __all__ = [ "ScheduleDreamRequest", "Scope", "ScopeCreate", - "ScopeSessions", "ScopeSessionsAdd", "Session", "SessionBase", diff --git a/src/schemas/api.py b/src/schemas/api.py index 1d345ff8..f686d007 100644 --- a/src/schemas/api.py +++ b/src/schemas/api.py @@ -401,10 +401,9 @@ class SessionCreate(SessionBase): max_length=100, description=( "Optional list of (unprefixed) scope names to add this session to. " - "Each scope is created if it does not exist yet. Note: scope " - "membership only affects messages ingested after the session is " - "added to the scope; backfill of pre-existing documents lands in a " - "follow-up (DEV-1999)." + "Each scope is created if it does not exist yet. Membership applies " + "only to messages ingested after the session is added to the scope; " + "conclusions already derived are not backfilled." ), ) @@ -565,12 +564,6 @@ class ScopeSessionsAdd(BaseModel): ) -class ScopeSessions(BaseModel): - """IDs of the sessions that are currently members of a scope.""" - - session_ids: list[str] - - # --------------------------------------------------------------------------- # Conclusion schemas # --------------------------------------------------------------------------- diff --git a/src/telemetry/events/trace.py b/src/telemetry/events/trace.py index 37383bc0..5218c302 100644 --- a/src/telemetry/events/trace.py +++ b/src/telemetry/events/trace.py @@ -143,8 +143,8 @@ class TraceContentEvent(BaseEvent): # Tool calls in a unified {id, name, input} shape (provider-agnostic). tool_calls: list[dict[str, Any]] = Field(default_factory=list) # Tags Honcho-authored content (system prompts, scaffold) so tenant-facing - # views can withhold globally-shared content (the §6.3 access invariant — - # dedup is global, the content store has no tenant column). + # views can withhold globally-shared content: dedup is global, and the + # content store has no tenant column. honcho_authored: bool = False def get_resource_id(self) -> str: diff --git a/tests/dreamer/test_card_refresh.py b/tests/dreamer/test_card_refresh.py index 43d1edea..c039ff0c 100644 --- a/tests/dreamer/test_card_refresh.py +++ b/tests/dreamer/test_card_refresh.py @@ -1,4 +1,4 @@ -"""Tests for the card_refresh dream type (DEV-2000, Scopes RFC prerequisite). +"""Tests for the card_refresh dream type. Covers: - queue plumbing: payload roundtrip, work-unit key isolation from omni, diff --git a/tests/routes/test_scope_route_policy.py b/tests/routes/test_scope_route_policy.py index 735156b1..b9f6566b 100644 --- a/tests/routes/test_scope_route_policy.py +++ b/tests/routes/test_scope_route_policy.py @@ -35,14 +35,16 @@ enumerate; a gap there would not be caught here. """ from collections.abc import Callable, Iterator -from dataclasses import dataclass, field +from dataclasses import dataclass import pytest from fastapi.routing import APIRoute from fastapi.testclient import TestClient +from httpx import Response from nanoid import generate as generate_nanoid from pydantic import BaseModel from pydantic.fields import FieldInfo +from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from src import models @@ -74,7 +76,7 @@ _KEY_POSITION = "body_peer_keys" _SCOPES_PREFIX = "/v3/workspaces/{workspace_id}/scopes" # A builder places `peer` into one position of one route and returns the response. -Builder = Callable[[TestClient, str, str, str], object] +Builder = Callable[[TestClient, str, str, str], Response] @dataclass(frozen=True) @@ -87,6 +89,19 @@ class Case: refuse: bool reason: str = "" build: Builder | None = None + # REFUSE cases only. Whether a reserved name that does NOT YET EXIST is also + # refused — the third axis, and the one that is not derivable from `refuse`. + # It follows from the guard the call site picked: + # + # validate_no_scope_peer_names name-only, no DB refuses missing + # reject_scope_observed strict on the observed refuses missing + # reject_scope_peers flag-based, permissive allows missing + # + # Permissive is correct where something downstream still stops it (the create + # path validates new names) or where the name simply resolves to nothing (404 + # before any guard runs). Each False therefore needs `missing_reason`. + refuse_missing: bool | None = None + missing_reason: str = "" # Set when the 422 legitimately comes from request-schema validation rather # than a scope guard, so the detail is pydantic's rather than ours. schema_level: bool = False @@ -95,7 +110,6 @@ class Case: # ALLOW cases only: builder plus the status a real scope must receive, so the # suite proves legitimate observer positions keep working. allow_status: tuple[int, ...] = () - _: tuple[()] = field(default=(), repr=False) @property def key(self) -> tuple[str, str, str]: @@ -255,6 +269,10 @@ def _b_peer_config(c: TestClient, ws: str, s: str, p: str): ) +def _b_peer_config_get(c: TestClient, ws: str, s: str, p: str): + return c.get(f"/v3/workspaces/{ws}/sessions/{s}/peers/{p}/config") + + # A plain peer used for the *other* side of two-position routes, so the position # under test is the only scope in the request. Created by the fixtures below. _OTHER = "policy-counterparty" @@ -271,16 +289,48 @@ _READ_ONLY_OK = ( POLICY: tuple[Case, ...] = ( # ---- observed position: a scope must never be the subject ---- Case( - "POST", f"{_W}/conclusions", "observed_id", True, build=_b_conclusion_observed + "POST", + f"{_W}/conclusions", + "observed_id", + True, + refuse_missing=False, + missing_reason=( + "Every observer and observed peer is resolved before the scope check, " + "so a name that does not exist is a 404 and no conclusion is written. " + "The guard is still the strict variant, for if that ever changes." + ), + build=_b_conclusion_observed, + ), + Case( + "POST", + f"{_W}/schedule_dream", + "observed", + True, + refuse_missing=True, + build=_b_dream_observed, + ), + Case( + "PUT", + f"{_W}/peers/{{peer_id}}/card", + "target", + True, + refuse_missing=True, + build=_b_card_target, + ), + Case( + "POST", + f"{_W}/peers/{{peer_id}}/chat", + "target", + True, + refuse_missing=True, + build=_b_chat_target, ), - Case("POST", f"{_W}/schedule_dream", "observed", True, build=_b_dream_observed), - Case("PUT", f"{_W}/peers/{{peer_id}}/card", "target", True, build=_b_card_target), - Case("POST", f"{_W}/peers/{{peer_id}}/chat", "target", True, build=_b_chat_target), Case( "POST", f"{_W}/peers/{{peer_id}}/representation", "target", True, + refuse_missing=True, build=_b_repr_target, ), Case( @@ -288,6 +338,7 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/context", "peer_target", True, + refuse_missing=True, build=_b_session_context_target, ), # ---- observer position: legitimately a scope ---- @@ -363,6 +414,7 @@ POLICY: tuple[Case, ...] = ( f"{_W}/peers", _KEY_POSITION, True, + refuse_missing=True, build=_b_create_peer, schema_level=True, skip_squatter=( @@ -371,13 +423,28 @@ POLICY: tuple[Case, ...] = ( "test_scopes.py::test_peer_create_rejects_reserved_prefix." ), ), - Case("PUT", f"{_W}/peers/{{peer_id}}", "peer_id", True, build=_b_update_peer), - Case("POST", f"{_W}/sessions", "peer_names", True, build=_b_session_create), + Case( + "PUT", + f"{_W}/peers/{{peer_id}}", + "peer_id", + True, + refuse_missing=True, + build=_b_update_peer, + ), + Case( + "POST", + f"{_W}/sessions", + "peer_names", + True, + refuse_missing=True, + build=_b_session_create, + ), Case( "POST", f"{_W}/sessions/{{session_id}}/messages", "peer_name", True, + refuse_missing=True, build=_b_message, ), Case( @@ -385,6 +452,7 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/messages/upload", "peer_id", True, + refuse_missing=True, build=_b_upload, ), Case( @@ -392,6 +460,7 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/peers", _KEY_POSITION, True, + refuse_missing=True, build=_b_add_peers, ), Case( @@ -399,6 +468,7 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/peers", _KEY_POSITION, True, + refuse_missing=True, build=_b_set_peers, ), Case( @@ -406,6 +476,12 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/peers", _KEY_POSITION, True, + refuse_missing=False, + missing_reason=( + "Removal creates nothing and a name that does not exist has no " + "membership row, so the request is a no-op. Refusing here would give a " + "reserved name a different removal result than any other absent peer." + ), build=_b_remove_peers, ), Case( @@ -413,14 +489,33 @@ POLICY: tuple[Case, ...] = ( f"{_W}/sessions/{{session_id}}/peers/{{peer_id}}/config", "peer_id", True, + refuse_missing=False, + missing_reason=( + "The peer is resolved before the scope check, so a name that does not " + "exist is a 404 and never reaches the guard. Nothing is created, so " + "there is no window for the name to be claimed here." + ), build=_b_peer_config, ), + Case( + "GET", + f"{_W}/sessions/{{session_id}}/peers/{{peer_id}}/config", + "peer_id", + True, + refuse_missing=False, + missing_reason=( + "Same resolution order as the write side of this route: an absent peer " + "is a 404 before the scope check, and a read creates nothing." + ), + build=_b_peer_config_get, + ), # ---- path peer on the dialectic surface ---- Case( "POST", f"{_W}/peers/{{peer_id}}/chat", "peer_id", True, + refuse_missing=True, build=_b_chat_observer, ), Case( @@ -428,16 +523,10 @@ POLICY: tuple[Case, ...] = ( f"{_W}/peers/{{peer_id}}/representation", "peer_id", True, + refuse_missing=True, build=_b_repr_observer, ), # ---- reads that neither create nor mutate knowledge about a scope ---- - Case( - "GET", - f"{_W}/sessions/{{session_id}}/peers/{{peer_id}}/config", - "peer_id", - False, - reason="Read-only. The write side of this same route IS refused.", - ), Case( "POST", f"{_W}/peers/{{peer_id}}/search", "peer_id", False, reason=_READ_ONLY_OK ), @@ -457,8 +546,8 @@ POLICY: tuple[Case, ...] = ( "peer_id", False, reason=( - "Read-only. The read-side scope surface belongs to Phase 2b (DEV-1998), " - "which owns the `scope` option on the context routes." + "Read-only. The read-side scope surface is not implemented yet; the " + "`scope` option on the context routes will own it when it lands." ), ), Case( @@ -468,7 +557,7 @@ POLICY: tuple[Case, ...] = ( False, reason=( "Read-only, and empty for a scope now that nothing can write knowledge " - "about one. Phase 2b (DEV-1998) owns this surface." + "about one. The read-side scope surface is not implemented yet." ), ), Case( @@ -487,8 +576,8 @@ POLICY: tuple[Case, ...] = ( "peer_id", False, reason=( - "Mints a scoped JWT rather than touching a peer. Scope-bound keys are " - "Phase 3 (DEV-2002)." + "Mints a scoped JWT rather than touching a peer, so no peer row is read " + "or written. Keys cannot be bound to a scope yet." ), ), ) @@ -587,6 +676,19 @@ def test_every_peer_position_is_classified(): derived = _derived_positions() classified = set(_BY_KEY) + # _peer_positions walks FastAPI/Pydantic internals (route.dependant, its + # *_params lists, field_info.annotation). An upgrade that reshapes any of them + # would make derivation silently return nothing, and every assertion below + # would then pass vacuously. Anchor on a position that must always be found. + assert ( + "POST", + f"{_W}/sessions/{{session_id}}/messages", + "peer_name", + ) in derived, ( + "derived no peer positions for a route that certainly has one — the " + "FastAPI internals _peer_positions() traverses have probably changed shape" + ) + unclassified = derived - classified assert not unclassified, ( "peer positions with no scope policy: " @@ -603,12 +705,25 @@ def test_policy_entries_are_well_formed(): if case.refuse: assert case.build is not None, f"{case.key} refuses but has no builder" assert not case.reason, f"{case.key} refuses; reason is for allow cases" + # The missing-name axis is not derivable from `refuse`, so it must be + # stated rather than defaulted — that gap is what this field closes. + assert case.refuse_missing is not None, ( + f"{case.key} refuses a real scope but does not say whether a " + "reserved name that does not exist yet is also refused" + ) + if not case.refuse_missing: + assert ( + len(case.missing_reason.strip()) > 30 + ), f"{case.key} tolerates a missing reserved name; say why" else: assert len(case.reason.strip()) > 30, f"{case.key} needs a real reason" assert bool(case.build) == bool(case.allow_status), ( f"{case.key}: an allow case needs a builder and an expected " "allow_status together, or neither" ) + assert ( + case.refuse_missing is None + ), f"{case.key}: refuse_missing applies to REFUSE cases only" _REFUSING = tuple(case for case in POLICY if case.refuse) @@ -630,6 +745,26 @@ def _setup(client: TestClient, workspace: str) -> tuple[str, str]: return session_name, str(generate_nanoid()) +def _real_scope( + client: TestClient, workspace: str, session_name: str, scope_name: str +) -> str: + """Create a scope, attach the session to it, and return its backing peer name.""" + assert ( + client.post( + f"/v3/workspaces/{workspace}/scopes", json={"id": scope_name} + ).status_code + == 201 + ) + assert ( + client.post( + f"/v3/workspaces/{workspace}/scopes/{scope_name}/sessions", + json={"session_ids": [session_name]}, + ).status_code + == 204 + ) + return scope_peer_name(scope_name) + + @pytest.mark.parametrize("case", _REFUSING, ids=lambda c: f"{c.method}:{c.position}") def test_refusing_position_rejects_a_real_scope( client: TestClient, @@ -639,31 +774,18 @@ def test_refusing_position_rejects_a_real_scope( """A real scope is refused in every position marked REFUSE.""" test_workspace, _ = sample_data session_name, scope_name = _setup(client, test_workspace.name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes", json={"id": scope_name} - ).status_code - == 201 - ) - backing = scope_peer_name(scope_name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ).status_code - == 200 - ) + backing = _real_scope(client, test_workspace.name, session_name, scope_name) assert case.build is not None result = case.build(client, test_workspace.name, session_name, backing) - status = getattr(result, "status_code", None) + status = result.status_code assert status == 422, ( f"{case.method} {case.path} accepted a scope in position " f"{case.position!r} (got {status})" ) # A 422 alone proves nothing — a malformed body would also produce one. - detail = str(getattr(result, "text", "")) + detail = result.text if case.schema_level: assert ( "pattern" in detail @@ -692,28 +814,14 @@ def test_allowing_position_accepts_a_real_scope( """ test_workspace, _ = sample_data session_name, scope_name = _setup(client, test_workspace.name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes", json={"id": scope_name} - ).status_code - == 201 - ) - backing = scope_peer_name(scope_name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ).status_code - == 200 - ) + backing = _real_scope(client, test_workspace.name, session_name, scope_name) assert case.build is not None result = case.build(client, test_workspace.name, session_name, backing) - status = getattr(result, "status_code", None) - assert status in case.allow_status, ( + assert result.status_code in case.allow_status, ( f"{case.method} {case.path} refused a scope in the legitimate position " - f"{case.position!r}: expected {case.allow_status}, got {status} — " - f"{getattr(result, 'text', '')[:200]}" + f"{case.position!r}: expected {case.allow_status}, got " + f"{result.status_code} — {result.text[:200]}" ) @@ -752,9 +860,62 @@ async def test_refusing_position_allows_unflagged_squatter( assert case.build is not None result = case.build(client, test_workspace.name, session_name, squatter) - status = getattr(result, "status_code", None) - assert status != 422, ( - f"{case.method} {case.path} refused an unflagged squatter in position " - f"{case.position!r} (got {status}) — the guard is keying off the name " - "prefix rather than the scope flag" + # Deliberately not `!= 422`: that also passes on a 5xx, so a guard regressing + # into an unhandled error (the psycopg DataError path this feature defends + # against) would keep this green. + assert result.status_code < 400, ( + f"{case.method} {case.path} did not accept an unflagged squatter in " + f"position {case.position!r} (got {result.status_code}) — a 422 means the " + "guard is keying off the name prefix rather than the scope flag; anything " + f"else means the request blew up. Body: {result.text[:200]}" + ) + + +@pytest.mark.parametrize("case", _REFUSING, ids=lambda c: f"{c.method}:{c.position}") +async def test_refusing_position_and_a_missing_reserved_name( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + case: Case, +): + """The third axis: a reserved name that does not exist yet. + + Neither of the other two tests reaches it — both resolve an existing subject. + A permissive guard here is sometimes correct (the create path refuses the name + itself, or it simply resolves to nothing), which is why the expected verdict is + declared per case rather than assumed. + + What is NOT negotiable in either direction is that the request must not MINT + the reserved name. Minting it would let any caller squat a scope name before + the workspace owner can create it, and would leave a peer that the facade can + never adopt. + """ + test_workspace, _ = sample_data + session_name, _ = _setup(client, test_workspace.name) + missing = scope_peer_name(str(generate_nanoid())) + + assert case.build is not None + result = case.build(client, test_workspace.name, session_name, missing) + + if case.refuse_missing: + assert result.status_code == 422, ( + f"{case.method} {case.path} accepted a not-yet-existing reserved name " + f"in position {case.position!r} (got {result.status_code}) — it could " + f"become a scope later. Body: {result.text[:200]}" + ) + else: + assert result.status_code != 422, ( + f"{case.method} {case.path} refused a missing reserved name in position " + f"{case.position!r}, but the policy says it tolerates one " + f"({case.missing_reason!r}) — update the policy or the guard" + ) + + minted = await db_session.scalar( + select(models.Peer) + .where(models.Peer.workspace_name == test_workspace.name) + .where(models.Peer.name == missing) + ) + assert minted is None, ( + f"{case.method} {case.path} minted the reserved name {missing!r} from " + f"position {case.position!r} — the scope namespace is now squatted" ) diff --git a/tests/routes/test_scopes.py b/tests/routes/test_scopes.py index 3b187389..a68a3725 100644 --- a/tests/routes/test_scopes.py +++ b/tests/routes/test_scopes.py @@ -58,6 +58,30 @@ def _create_session( return session_name +def _add_sessions( + client: TestClient, + workspace_name: str, + scope_name: str, + session_names: list[str], +) -> int: + """Add sessions to a scope via the facade; returns the status code (204 on success).""" + return client.post( + f"/v3/workspaces/{workspace_name}/scopes/{scope_name}/sessions", + json={"session_ids": session_names}, + ).status_code + + +def _scope_sessions( + client: TestClient, workspace_name: str, scope_name: str +) -> list[str]: + """Names of a scope's member sessions, oldest membership first.""" + response = client.post( + f"/v3/workspaces/{workspace_name}/scopes/{scope_name}/sessions/list" + ) + assert response.status_code == 200, response.text + return [item["id"] for item in response.json()["items"]] + + async def _get_session_peer( db_session: AsyncSession, workspace_name: str, @@ -226,7 +250,61 @@ def test_scopes_routes_require_workspace_level_key( client.headers["Authorization"] = ( f"Bearer {create_jwt(JWTParams(w=test_workspace.name, s='some-session'))}" ) - assert client.get(f"{scopes_url}/{scope_name}/sessions").status_code == 401 + assert client.post(f"{scopes_url}/{scope_name}/sessions/list").status_code == 401 + + +def test_session_create_scopes_requires_workspace_level_key( + client: TestClient, + sample_data: tuple[Workspace, Peer], + monkeypatch: pytest.MonkeyPatch, +): + """`scopes` on session create is the scopes routes by another door. + + It creates scope peers and attaches memberships, so it carries the same + workspace-level requirement. Session create is otherwise a self-authorizing + route that accepts peer- and session-scoped keys, which is exactly why this + needs its own check rather than the route's auth dependency. + """ + test_workspace, test_peer = sample_data + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + sessions_url = f"/v3/workspaces/{test_workspace.name}/sessions" + scopes_url = f"/v3/workspaces/{test_workspace.name}/scopes" + scope_name = str(generate_nanoid()) + + def create_with_scope(session_name: str) -> int: + return client.post( + sessions_url, json={"id": session_name, "scopes": [scope_name]} + ).status_code + + def listed_scopes() -> set[str]: + response = client.post(f"{scopes_url}/list") + assert response.status_code == 200 + return {item["id"] for item in response.json()["items"]} + + # Peer-scoped key: rejected + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=test_peer.name))}" + ) + assert create_with_scope(str(generate_nanoid())) == 401 + + # Session-scoped key: rejected. The session name must match the token's `s` + # claim, or the handler's own session check would 401 first and this would + # pass without ever reaching the scopes gate. + own_session = str(generate_nanoid()) + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, s=own_session))}" + ) + assert create_with_scope(own_session) == 401 + + # Workspace-scoped key: allowed. The scope is absent until this call lands, + # which proves the rejected requests did not mint it on their way out. + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name))}" + ) + assert scope_name not in listed_scopes() + assert create_with_scope(str(generate_nanoid())) == 201 + assert scope_name in listed_scopes() def test_peer_create_rejects_reserved_prefix( @@ -283,89 +361,6 @@ def test_peers_list_kind_filtering( assert {test_peer.name, backing_peer_name} <= names -def test_scope_peer_cannot_author_messages( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - session_name = _create_session(client, test_workspace.name) - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/messages", - json={ - "messages": [ - { - "peer_id": scope_peer_name(scope_name), - "content": "I should not speak", - } - ] - }, - ) - assert response.status_code == 422 - assert SCOPE_PEER_PREFIX in response.json()["detail"] - - -def test_scope_peer_cannot_be_chat_target( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """Chat validation happens before any LLM work, so this is safe to exercise.""" - test_workspace, test_peer = sample_data - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{test_peer.name}/chat", - json={"query": "what do you know?", "target": scope_peer_name(scope_name)}, - ) - assert response.status_code == 422 - - -def test_scope_peer_cannot_be_representation_target( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - test_workspace, test_peer = sample_data - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{test_peer.name}/representation", - json={"target": scope_peer_name(scope_name)}, - ) - assert response.status_code == 422 - - -def test_generic_session_peer_routes_reject_scope_peers( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """Scope membership is managed only via the scopes facade.""" - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - backing_peer_name = scope_peer_name(scope_name) - session_name = _create_session(client, test_workspace.name) - - base = f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}" - - response = client.post(f"{base}/peers", json={backing_peer_name: {}}) - assert response.status_code == 422 - assert "scopes" in response.json()["detail"] - - response = client.put(f"{base}/peers", json={backing_peer_name: {}}) - assert response.status_code == 422 - - response = client.request("DELETE", f"{base}/peers", json=[backing_peer_name]) - assert response.status_code == 422 - - # Session creation with a scope peer in the generic peers mapping is also - # rejected; the `scopes` field is the supported path. - response = client.post( - f"/v3/workspaces/{test_workspace.name}/sessions", - json={"id": str(generate_nanoid()), "peers": {backing_peer_name: {}}}, - ) - assert response.status_code == 422 - - async def test_scope_sessions_add_list_remove( client: TestClient, db_session: AsyncSession, @@ -381,11 +376,13 @@ async def test_scope_sessions_add_list_remove( scope_base = f"/v3/workspaces/{workspace_name}/scopes/{scope_name}" # Add both sessions - response = client.post( - f"{scope_base}/sessions", json={"session_ids": [session_1, session_2]} + assert ( + _add_sessions(client, workspace_name, scope_name, [session_1, session_2]) == 204 ) - assert response.status_code == 200 - assert set(response.json()["session_ids"]) == {session_1, session_2} + assert set(_scope_sessions(client, workspace_name, scope_name)) == { + session_1, + session_2, + } # Membership rows carry the observer shape: observe_others on, observe_me off session_peer = await _get_session_peer( @@ -396,18 +393,11 @@ async def test_scope_sessions_add_list_remove( assert session_peer.configuration["observe_others"] is True assert session_peer.configuration["observe_me"] is False - # List memberships - response = client.get(f"{scope_base}/sessions") - assert response.status_code == 200 - assert set(response.json()["session_ids"]) == {session_1, session_2} - # Remove one membership (soft delete, like the generic remove-peer path) response = client.delete(f"{scope_base}/sessions/{session_1}") assert response.status_code == 204 - response = client.get(f"{scope_base}/sessions") - assert response.status_code == 200 - assert response.json()["session_ids"] == [session_2] + assert _scope_sessions(client, workspace_name, scope_name) == [session_2] db_session.expire_all() session_peer = await _get_session_peer( @@ -425,11 +415,15 @@ def test_scope_sessions_add_missing_session_404( assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 existing_session = _create_session(client, test_workspace.name) - response = client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [existing_session, str(generate_nanoid())]}, + assert ( + _add_sessions( + client, + test_workspace.name, + scope_name, + [existing_session, str(generate_nanoid())], + ) + == 404 ) - assert response.status_code == 404 def test_scope_sessions_routes_404_on_unknown_scope( @@ -437,14 +431,13 @@ def test_scope_sessions_routes_404_on_unknown_scope( ): test_workspace, _ = sample_data session_name = _create_session(client, test_workspace.name) - scope_base = f"/v3/workspaces/{test_workspace.name}/scopes/{generate_nanoid()}" + unknown_scope = str(generate_nanoid()) + scope_base = f"/v3/workspaces/{test_workspace.name}/scopes/{unknown_scope}" - response = client.post( - f"{scope_base}/sessions", json={"session_ids": [session_name]} + assert ( + _add_sessions(client, test_workspace.name, unknown_scope, [session_name]) == 404 ) - assert response.status_code == 404 - - assert client.get(f"{scope_base}/sessions").status_code == 404 + assert client.post(f"{scope_base}/sessions/list").status_code == 404 assert client.delete(f"{scope_base}/sessions/{session_name}").status_code == 404 @@ -483,11 +476,7 @@ async def test_session_create_with_scopes( assert session_peer.configuration["observe_me"] is False # And the memberships show up through the facade - response = client.get( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_a}/sessions" - ) - assert response.status_code == 200 - assert response.json()["session_ids"] == [session_name] + assert _scope_sessions(client, test_workspace.name, scope_a) == [session_name] def test_session_create_rejects_prefixed_scope_names( @@ -530,11 +519,9 @@ async def test_scope_membership_equals_hand_built_observer( scope_name = str(generate_nanoid()) assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 scope_session = _create_session(client, test_workspace.name) - response = client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [scope_session]}, + assert ( + _add_sessions(client, test_workspace.name, scope_name, [scope_session]) == 204 ) - assert response.status_code == 200 hand_built = await _get_session_peer( db_session, test_workspace.name, observer_session, observer_name @@ -565,11 +552,7 @@ async def test_scope_peer_observes_ingested_messages( json={test_peer.name: {}}, ) assert response.status_code == 200 - response = client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ) - assert response.status_code == 200 + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 # Ingest a message from the real peer and run the deriver enqueue fan-out message = models.Message( @@ -731,42 +714,6 @@ async def test_forged_configuration_kind_does_not_make_a_scope( assert response.status_code in [200, 201], response.text -def test_update_peer_rejects_reserved_prefix( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """PUT on a reserved-namespace name is a clean 422, never a 500.""" - test_workspace, _ = sample_data - - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{scope_peer_name('therapy')}", - json={"metadata": {"k": "v"}}, - ) - assert response.status_code == 422 - assert SCOPE_PEER_PREFIX in response.json()["detail"] - - -async def test_scope_peer_cannot_be_chat_or_representation_observer( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """The path-level peer_id is guarded, not just `target`.""" - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - backing = scope_peer_name(scope_name) - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{backing}/chat", - json={"query": "what do you know?"}, - ) - assert response.status_code == 422, response.text - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{backing}/representation", - json={}, - ) - assert response.status_code == 422, response.text - - def test_internal_metadata_never_in_peer_response( client: TestClient, sample_data: tuple[Workspace, Peer] ): @@ -848,96 +795,6 @@ def test_message_author_cannot_create_invalid_peer_name( assert bad_name not in [p["id"] for p in response.json()["items"]] -def test_message_author_cannot_squat_scope_namespace( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """The reserved namespace must not be reachable via the message author path. - - Without create-path validation this minted an unflagged `scope.` peer, - which then permanently 409-blocked creating that scope — a denial of service - on the namespace by any caller who can post a message. - """ - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - session_name = _create_session(client, test_workspace.name) - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/messages", - json={ - "messages": [{"peer_id": scope_peer_name(scope_name), "content": "hello"}] - }, - ) - assert response.status_code == 422, response.text - assert SCOPE_PEER_PREFIX in response.json()["detail"] - - # The scope name is still available - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - - -def test_session_peer_map_cannot_squat_scope_namespace( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """Same hole, via the session peer mapping rather than a message author.""" - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/sessions", - json={ - "id": str(generate_nanoid()), - "peers": {scope_peer_name(scope_name): {}}, - }, - ) - assert response.status_code == 422, response.text - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - - -async def test_unflagged_squatter_can_still_be_updated( - client: TestClient, - db_session: AsyncSession, - sample_data: tuple[Workspace, Peer], -): - """An existing unflagged peer in the namespace is a normal peer. - - Three-way behavior on PUT /peers/{id}: a real scope is refused, an existing - unflagged squatter updates fine, and a missing reserved-prefix name is - refused by create-path validation rather than being minted. - """ - test_workspace, _ = sample_data - squatter = scope_peer_name(str(generate_nanoid())) - db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) - await db_session.commit() - - # existing unflagged peer -> allowed - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{squatter}", - json={"metadata": {"k": "v"}}, - ) - assert response.status_code == 200, response.text - assert response.json()["metadata"] == {"k": "v"} - - # real scope -> refused - scope_name = str(generate_nanoid()) - assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{scope_peer_name(scope_name)}", - json={"metadata": {"k": "v"}}, - ) - assert response.status_code == 422, response.text - - # missing reserved-prefix name -> refused, not created - missing = scope_peer_name(str(generate_nanoid())) - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{missing}", - json={"metadata": {"k": "v"}}, - ) - assert response.status_code == 422, response.text - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/list", json={"kind": "all"} - ) - assert missing not in [p["id"] for p in response.json()["items"]] - - async def test_resolved_scope_peer_rejected_at_membership_upsert( client: TestClient, db_session: AsyncSession, @@ -968,26 +825,25 @@ async def test_resolved_scope_peer_rejected_at_membership_upsert( ) -def test_set_peer_config_cannot_disable_a_scope( - client: TestClient, sample_data: tuple[Workspace, Peer] +async def test_set_peer_config_cannot_disable_a_scope( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], ): """A scope's membership config belongs to the facade, not the caller. `observe_others=false` would silently stop all fan-out into the scope, and `observe_me=true` would make Honcho form a representation *of* a scope. + + Verified against the row rather than the config route, because that read is + refused for a scope too — see the policy table. """ test_workspace, _ = sample_data scope_name = str(generate_nanoid()) assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 backing = scope_peer_name(scope_name) session_name = _create_session(client, test_workspace.name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ).status_code - == 200 - ) + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 response = client.put( f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers/{backing}/config", @@ -996,62 +852,54 @@ def test_set_peer_config_cannot_disable_a_scope( assert response.status_code == 422, response.text # Observer semantics intact - current = client.get( - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers/{backing}/config" + membership = await _get_session_peer( + db_session, test_workspace.name, session_name, backing ) - assert current.status_code == 200 - assert current.json() == {"observe_me": False, "observe_others": True} + assert membership is not None + assert membership.configuration == {"observe_me": False, "observe_others": True} -async def test_unflagged_squatter_config_still_settable( - client: TestClient, - db_session: AsyncSession, - sample_data: tuple[Workspace, Peer], -): - """The set_peer_config guard is flag-based, so a squatter is unaffected.""" - test_workspace, _ = sample_data - squatter = scope_peer_name(str(generate_nanoid())) - db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) - await db_session.commit() - - session_name = _create_session(client, test_workspace.name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", - json={squatter: {}}, - ).status_code - == 200 - ) - - response = client.put( - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers/{squatter}/config", - json={"observe_others": False, "observe_me": True}, - ) - assert response.status_code == 204, response.text - - -def test_generic_session_remove_cannot_detach_a_scope( +def test_session_peers_listing_excludes_scopes( client: TestClient, sample_data: tuple[Workspace, Peer] ): - """Scope membership must end through the scopes routes, which reconcile.""" - test_workspace, _ = sample_data + """The generic session-peer surface must not expose the facade's observer. + + Without the filter this listing returns a peer literally named + `scope.` carrying `observe_others=true` — the exact mechanic the + facade exists to hide. The membership-config read is refused for the same + reason; ordinary members are unaffected by both. + """ + test_workspace, test_peer = sample_data + base = f"/v3/workspaces/{test_workspace.name}" scope_name = str(generate_nanoid()) assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 + backing = scope_peer_name(scope_name) session_name = _create_session(client, test_workspace.name) + assert ( client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, + f"{base}/sessions/{session_name}/peers", json={test_peer.name: {}} ).status_code == 200 ) + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 - response = client.request( - "DELETE", - f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", - json=[scope_peer_name(scope_name)], + listed = client.get(f"{base}/sessions/{session_name}/peers") + assert listed.status_code == 200 + names = {item["id"] for item in listed.json()["items"]} + assert test_peer.name in names + assert backing not in names + + assert ( + client.get(f"{base}/sessions/{session_name}/peers/{backing}/config").status_code + == 422 + ) + assert ( + client.get( + f"{base}/sessions/{session_name}/peers/{test_peer.name}/config" + ).status_code + == 200 ) - assert response.status_code == 422, response.text @pytest.mark.parametrize("bad_name", ["", "a" * 513]) @@ -1075,10 +923,9 @@ def test_degenerate_peer_names_are_422_not_500( # --------------------------------------------------------------------------- -# Observed-position and pre-seeding guards. A scope may be an observer but must -# never be observed — and "observed" includes a reserved name that does not yet -# exist, since nothing on these paths creates the peer and the state would -# retroactively describe the scope once created. +# Detach-by-omission. The replacement routes never name the scope, so there is +# no peer position for the route-policy table to classify — the caller detaches +# by leaving it out. Preservation has to be flag-based, not request-based. # --------------------------------------------------------------------------- @@ -1094,13 +941,7 @@ def test_generic_replacement_preserves_scope_membership( scope_name = str(generate_nanoid()) assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 session_name = _create_session(client, test_workspace.name) - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ).status_code - == 200 - ) + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 # Replacement naming only an ordinary peer must succeed... response = client.put( @@ -1110,11 +951,7 @@ def test_generic_replacement_preserves_scope_membership( assert response.status_code == 200, response.text # ...while leaving the scope's membership intact. - response = client.get( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions" - ) - assert response.status_code == 200 - assert response.json()["session_ids"] == [session_name] + assert _scope_sessions(client, test_workspace.name, scope_name) == [session_name] async def test_empty_replacement_preserves_scope_membership( @@ -1131,10 +968,7 @@ async def test_empty_replacement_preserves_scope_membership( f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", json={test_peer.name: {}}, ) - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ) + _add_sessions(client, test_workspace.name, scope_name, [session_name]) assert ( client.put( @@ -1144,10 +978,7 @@ async def test_empty_replacement_preserves_scope_membership( == 200 ) - response = client.get( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions" - ) - assert response.json()["session_ids"] == [session_name] + assert _scope_sessions(client, test_workspace.name, scope_name) == [session_name] # The other half of the docstring: without this the test passes even if the # empty replacement became a no-op for ordinary peers too. @@ -1192,45 +1023,15 @@ async def test_replacement_still_removes_unflagged_squatter( assert session_peer.left_at is not None, "squatter should be replaced normally" -def test_peer_card_cannot_be_preseeded_for_a_future_scope( - client: TestClient, sample_data: tuple[Workspace, Peer] -): - """A card keyed on a not-yet-existing reserved name is refused. - - Only the observer is resolved when writing a card, so without this the card - persists and starts describing a real scope the moment one is created. - """ - test_workspace, test_peer = sample_data - future = str(generate_nanoid()) - - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{test_peer.name}" - + f"/card?target={scope_peer_name(future)}", - json={"peer_card": ["pre-seeded"]}, - ) - assert response.status_code == 422, response.text - - # And the scope name is still free to create - assert _create_scope(client, test_workspace.name, future).status_code == 201 - - -async def test_peer_card_target_squatter_still_allowed( - client: TestClient, - db_session: AsyncSession, - sample_data: tuple[Workspace, Peer], -): - """An existing unflagged squatter remains a valid card subject.""" - test_workspace, test_peer = sample_data - squatter = scope_peer_name(str(generate_nanoid())) - db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) - await db_session.commit() - - response = client.put( - f"/v3/workspaces/{test_workspace.name}/peers/{test_peer.name}" - + f"/card?target={squatter}", - json={"peer_card": ["ordinary"]}, - ) - assert response.status_code == 200, response.text +# --------------------------------------------------------------------------- +# Observed-position guards below the HTTP surface. A scope may be an observer +# but must never be observed — and "observed" includes a reserved name that does +# not yet exist, since nothing on these paths creates the peer and the state +# would retroactively describe the scope once created. The route-level half of +# this is enumerated in test_scope_route_policy.py; these cover the crud entry +# points that no route table reaches, plus the side effects a status code alone +# would not catch. +# --------------------------------------------------------------------------- async def test_set_peer_card_guard_covers_internal_callers( @@ -1254,10 +1055,12 @@ async def test_dream_cannot_be_queued_for_a_future_scope( db_session: AsyncSession, sample_data: tuple[Workspace, Peer], ): - """A dream naming a not-yet-existing reserved observed peer is refused. + """A refused dream leaves no queue row behind. - The route's own precheck cannot catch this — the peer is not flagged yet — so - the check has to sit in the transaction that inserts the queue item. + The 422 itself is enumerated in test_scope_route_policy.py; what that cannot + see is the side effect. The route's own precheck cannot catch this — the peer + is not flagged yet — so the check has to sit in the transaction that inserts + the queue item, and a check placed after the insert would still 422. """ test_workspace, test_peer = sample_data future = scope_peer_name(str(generate_nanoid())) @@ -1316,35 +1119,6 @@ def test_prefixed_nul_name_is_422_not_500( assert response.status_code == 422, response.text -async def test_representation_rechecks_after_early_check( - client: TestClient, - db_session: AsyncSession, - sample_data: tuple[Workspace, Peer], -): - """The representation read refuses a reserved name that could become a scope. - - The early name check passes anything not yet flagged, and the read happens in - a later session — so a reserved name that does not exist yet must be refused - rather than left to become a scope before the read. - """ - test_workspace, _ = sample_data - future = scope_peer_name(str(generate_nanoid())) - - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{future}/representation", json={} - ) - assert response.status_code == 422, response.text - - # An existing unflagged squatter still reads normally. - squatter = scope_peer_name(str(generate_nanoid())) - db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) - await db_session.commit() - response = client.post( - f"/v3/workspaces/{test_workspace.name}/peers/{squatter}/representation", json={} - ) - assert response.status_code == 200, response.text - - # --------------------------------------------------------------------------- # Observer limit. Scope memberships carry observe_others=True but must not # consume SESSION_OBSERVERS_LIMIT: that would cap scopes-per-session at the @@ -1365,18 +1139,16 @@ def test_scopes_do_not_count_toward_observer_limit( for scope_name in scope_names: assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - response = client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, + assert ( + _add_sessions(client, test_workspace.name, scope_name, [session_name]) + == 204 ) - assert response.status_code == 200, response.text # Every membership is live, and the session-create path agrees. for scope_name in scope_names: - response = client.get( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions" - ) - assert response.json()["session_ids"] == [session_name] + assert _scope_sessions(client, test_workspace.name, scope_name) == [ + session_name + ] response = client.post( f"/v3/workspaces/{test_workspace.name}/sessions", @@ -1396,13 +1168,7 @@ def test_observer_limit_still_applies_to_real_peers( session_name = _create_session(client, test_workspace.name) scope_name = str(generate_nanoid()) assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 - assert ( - client.post( - f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", - json={"session_ids": [session_name]}, - ).status_code - == 200 - ) + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 observers = { str(generate_nanoid()): {"observe_others": True}