diff --git a/src/crud/document.py b/src/crud/document.py index 38c735bd..b3d792f6 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 +from src.crud.peer import get_peer, reject_scope_peers from src.crud.session import get_session from src.dependencies import tracked_db from src.embedding_client import embedding_client @@ -954,6 +954,17 @@ async def create_observations( for peer_name in peers_to_validate: await get_peer(db, workspace_name, peer_name) + # A scope may be an *observer* — that is how scoped conclusions are stored — + # 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( + db, + workspace_name, + {obs.observed_id for obs in observations}, + action="No conclusion is ever formed about a scope.", + ) + # Get or create all collections for observer, observed in collection_pairs: await get_or_create_collection( diff --git a/src/crud/peer.py b/src/crud/peer.py index 4e85d78d..881fc5b6 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -47,6 +47,31 @@ def peer_cache_key(workspace_name: str, peer_name: str) -> str: ) +def _reject_impossible_peer_names(names: Iterable[str]) -> None: + """Reject names that cannot correspond to any stored row, before querying. + + ``PeerSpec`` accepts anything so existing names can be looked up, and the + full new-name rules run later on the insert path — but a couple of values + cannot be a legacy row *by construction*, and sending them to Postgres first + fails before that 422 can happen: + + - NUL bytes: Postgres text cannot hold them, so psycopg raises DataError + during the lookup itself, surfacing as a 500. + - Over-length names: the ``peers.name`` CHECK caps them at + ``PEER_NAME_MAX_LENGTH``, so no stored row can exceed it. + + Raises: + ValidationException: On a NUL byte or an over-length name. + """ + if any("\x00" in name for name in names): + raise ValidationException("Peer name(s) must not contain NUL (0x00) bytes") + 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" + ) + + def _validate_new_peer_names(names: list[str]) -> None: """Validate peer names that are about to be created. @@ -61,12 +86,8 @@ def _validate_new_peer_names(names: list[str]) -> None: scopes_util.validate_no_scope_peer_names( names, action="Use the scopes routes to create scopes." ) - 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" - ) - # RESOURCE_NAME_PATTERN's `+` already rejects the empty name. + # Length and NUL bytes are already refused before the lookup by + # _reject_impossible_peer_names; RESOURCE_NAME_PATTERN's `+` rejects empty. offenders = sorted({n for n in names if not re.fullmatch(RESOURCE_NAME_PATTERN, n)}) if offenders: raise ValidationException( @@ -156,6 +177,9 @@ async def get_or_create_peers( await get_or_create_workspace(db, schemas.WorkspaceCreate(name=workspace_name)) peer_names = [p.name for p in peers] + # Before the lookup: these values cannot match a stored row and would fail + # inside the query itself rather than as a clean 422. + _reject_impossible_peer_names(peer_names) stmt = ( select(models.Peer) .where(models.Peer.workspace_name == workspace_name) diff --git a/src/crud/session.py b/src/crud/session.py index 9a1df3df..9daa130a 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -7,7 +7,18 @@ from typing import cast as typing_cast from cashews import NOT_NONE from nanoid import generate as generate_nanoid -from sqlalchemy import Select, and_, case, cast, delete, func, insert, select, update +from sqlalchemy import ( + Select, + and_, + case, + cast, + delete, + exists, + func, + insert, + select, + update, +) from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.engine import CursorResult from sqlalchemy.exc import IntegrityError @@ -34,7 +45,12 @@ from src.utils.scopes import is_scope_peer, scope_peer_name from src.utils.types import GetOrCreateResult from src.vector_store import get_external_vector_store -from .peer import get_or_create_peers, get_peer, reject_scope_peers +from .peer import ( + get_or_create_peers, + get_peer, + reject_scope_peers, + scope_peer_clause, +) from .scope import SCOPE_MEMBERSHIP_CONFIG, get_or_create_scopes from .workspace import get_or_create_workspace @@ -848,9 +864,9 @@ async def remove_peers_from_session( await get_session(db, session_name, workspace_name) # Scope membership is ended through the scopes routes, which also reconcile - # the scope's copies. Checked here rather than in the route so it is adjacent - # to the UPDATE below, leaving no window for a concurrently-created scope to - # be silently detached from its sessions. + # the scope's copies. Rejected up front for a clear 422 rather than a silent + # no-op — but this check alone is only advisory: under READ COMMITTED a scope + # can be created between it and the UPDATE below. if not _allow_scope_peers: await reject_scope_peers( db, @@ -870,6 +886,20 @@ async def remove_peers_from_session( ) .values(left_at=func.now()) ) + if not _allow_scope_peers: + # Closes the window the advisory check above cannot: the exclusion is + # evaluated by Postgres as part of the UPDATE, so a scope committed after + # that check still cannot be detached here. Correlated rather than a join + # so the statement stays a plain UPDATE. + update_stmt = update_stmt.where( + ~exists( + select(models.Peer.id) + .where(models.Peer.workspace_name == workspace_name) + .where(models.Peer.name == models.SessionPeer.peer_name) + .where(scope_peer_clause()) + .correlate(models.SessionPeer) + ) + ) await db.execute(update_stmt) await db.commit() diff --git a/src/dialectic/chat.py b/src/dialectic/chat.py index a0cb89ba..770f3670 100644 --- a/src/dialectic/chat.py +++ b/src/dialectic/chat.py @@ -10,15 +10,38 @@ from collections.abc import AsyncIterator from pydantic import BaseModel -from src import crud +from src import crud, models from src.config import ReasoningLevel from src.dependencies import tracked_db from src.dialectic.core import DialecticAgent +from src.exceptions import ValidationException from src.utils.config_helpers import get_configuration +from src.utils.scopes import is_scope_peer logger = logging.getLogger(__name__) +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. + + Raises: + ValidationException: If any participant is a scope. + """ + offenders = sorted( + {p.name for p in peers if is_scope_peer(p.name, p.internal_metadata)} + ) + if offenders: + raise ValidationException( + f"Peer name(s) {offenders} are scopes." + + " No representation is formed of a scope, so a scope cannot be a" + + " dialectic observer or target." + ) + + async def agentic_chat( workspace_name: str, session_name: str | None, @@ -48,9 +71,18 @@ async def agentic_chat( """ # Short-lived DB session for validation + config async with tracked_db("dialectic.preflight", read_only=True) as db: - await crud.get_peer(db, workspace_name, observer) + observer_peer = await crud.get_peer(db, workspace_name, observer) + observed_peer = observer_peer if observer != observed: - await crud.get_peer(db, workspace_name, observed) + observed_peer = await crud.get_peer(db, workspace_name, observed) + + # Resolved-row scope check, not a name check. The routes reject scope + # names up front for a clear error, but that runs before resolution: a + # scope created in between would otherwise be used here as observer or + # target. Checking the rows we just resolved closes that window — an + # absent name already failed above, and an existing unflagged squatter + # cannot retroactively become a scope. + _reject_scope_participants(observer_peer, observed_peer) session = None if session_name: @@ -120,9 +152,18 @@ async def agentic_chat_stream( """ # Short-lived DB session for validation + config async with tracked_db("dialectic.preflight", read_only=True) as db: - await crud.get_peer(db, workspace_name, observer) + observer_peer = await crud.get_peer(db, workspace_name, observer) + observed_peer = observer_peer if observer != observed: - await crud.get_peer(db, workspace_name, observed) + observed_peer = await crud.get_peer(db, workspace_name, observed) + + # Resolved-row scope check, not a name check. The routes reject scope + # names up front for a clear error, but that runs before resolution: a + # scope created in between would otherwise be used here as observer or + # target. Checking the rows we just resolved closes that window — an + # absent name already failed above, and an existing unflagged squatter + # cannot retroactively become a scope. + _reject_scope_participants(observer_peer, observed_peer) session = None if session_name: diff --git a/src/routers/peers.py b/src/routers/peers.py index 08ea5ecb..20480c79 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -494,6 +494,19 @@ async def set_peer_card( # If no target specified, set the observer's own card observed = target if target is not None else peer_id + # A scope may be the *observer* of a card — the Dreamer writes (scope, observed) + # cards, which is how scoped peer cards exist — but never the observed. With no + # target, observed collapses to peer_id, so this also refuses a scope's self-card. + await crud.reject_scope_peers( + db, + workspace_id, + [observed], + action=( + "No representation is formed of a scope, so a scope cannot be the" + " subject of a peer card." + ), + ) + await crud.set_peer_card( db, workspace_id, diff --git a/src/routers/sessions.py b/src/routers/sessions.py index 9dc15a4d..1b8a55e8 100644 --- a/src/routers/sessions.py +++ b/src/routers/sessions.py @@ -745,6 +745,20 @@ async def get_session_context( "peer_target must be provided if peer_perspective is provided" ) + # 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. + if peer_target is not None: + await crud.reject_scope_peers( + db, + workspace_id, + [peer_target], + action=( + "No representation is formed of a scope, so a scope cannot be a" + " context target." + ), + ) + if not peer_target: # No representation or card needed summary, messages = await _get_session_context_task( diff --git a/src/routers/workspaces.py b/src/routers/workspaces.py index b7e15a71..2184dfcb 100644 --- a/src/routers/workspaces.py +++ b/src/routers/workspaces.py @@ -9,7 +9,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from src import crud, schemas from src.config import settings -from src.dependencies import db, read_db +from src.dependencies import db, read_db, tracked_db from src.deriver.enqueue import enqueue_deletion, enqueue_dream from src.exceptions import AuthenticationException from src.security import JWTParams, require_auth @@ -225,6 +225,22 @@ async def schedule_dream( observed = request.observed if request.observed is not None else request.observer dream_type = request.dream_type + # A scope is a legitimate dream *observer* — the Dreamer consolidates scoped + # collections — but never the observed: no representation is formed of a scope, + # so such a dream would build knowledge about one. + async with tracked_db( + "workspaces.schedule_dream.scope_check", read_only=True + ) as db: + await crud.reject_scope_peers( + db, + workspace_id, + [observed], + action=( + "No representation is formed of a scope, so a scope cannot be the" + " observed peer of a dream." + ), + ) + await enqueue_dream( workspace_id, observer=observer, diff --git a/tests/conftest.py b/tests/conftest.py index b3697242..93db47e5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -840,6 +840,7 @@ def mock_tracked_db(request: pytest.FixtureRequest): "src.deriver.consumer.tracked_db", "src.deriver.enqueue.tracked_db", "src.routers.peers.tracked_db", + "src.routers.workspaces.tracked_db", "src.crud.representation.tracked_db", "src.dreamer.orchestrator.tracked_db", "src.dreamer.dream_scheduler.tracked_db", diff --git a/tests/routes/test_scope_route_policy.py b/tests/routes/test_scope_route_policy.py index 0f9b1871..2bff9dc1 100644 --- a/tests/routes/test_scope_route_policy.py +++ b/tests/routes/test_scope_route_policy.py @@ -1,33 +1,41 @@ -"""Route-policy enumeration for the scopes facade. +"""Route-policy enumeration for the scopes facade, per peer *position*. -Three review passes over the scopes work each found the same *class* of defect — -a route nobody had checked, not logic that was subtly wrong. One of them -(`PUT /sessions/{id}/peers/{id}/config`, which let any caller set a scope to -``observe_others=false`` and silently stop all fan-out into it) predated the -work entirely: the guardrail set was assembled guardrail-by-guardrail rather -than derived from the route list. Sampling review cannot close that kind of gap. +Four review passes over the scopes work each found the same class of defect: a +place nobody had checked, rather than logic that was subtly wrong. The first +version of this module enumerated routes and classified each one guarded or +exempt — and that model was itself the fifth defect. A binary per-route verdict +cannot express the actual invariant, which is positional: -So this module enumerates instead. It derives every route through which a peer -name can reach the system, and requires each to be classified as either -GUARDED (a real scope is refused) or EXEMPT (with a stated reason). A newly -added peer-touching route fails `test_every_peer_touching_route_is_classified` -until someone consciously classifies it. + A scope may be an OBSERVER. A scope may never be OBSERVED. -Two invariants are then asserted *behaviorally* — by calling the routes, not by -inspecting annotations, because the guards deliberately live in crud (which is -what makes `messages/upload` guarded for free, via `crud.create_messages`): +`POST /conclusions` is the case that proves it: a scope as `observer_id` is how +scoped conclusions are stored and must work, while a scope as `observed_id` +persisted a conclusion about something that carries ``observe_me=false``. One +route, two positions, opposite verdicts. The same split applies to +`schedule_dream`, the peer-card routes, and session context. -1. A real scope peer is refused on every GUARDED route. -2. An *unflagged* peer merely occupying the reserved namespace is NOT a scope and - is unaffected. This is the half most easily broken by a well-meaning guard — - it regressed once already when `update_peer` used a name-based check. +So classification here is keyed by ``(method, path, position)``, where position is +the request parameter carrying the peer name. Every derived triple must appear in +`POLICY` as either REFUSE or ALLOW-with-a-reason; a new one fails +`test_every_peer_position_is_classified` until someone classifies it. + +Each REFUSE case is then asserted behaviorally — by calling the route, because the +guards deliberately live in crud (which is what makes `messages/upload` guarded +for free via `crud.create_messages`) — in both directions: + +1. a real scope is refused, and the rejection must actually name it, so an + unrelated 422 cannot pass the assertion; +2. an *unflagged* peer merely occupying the reserved namespace is NOT a scope and + is unaffected. That half regressed once already when `update_peer` keyed off + the name prefix. Known limitation: this covers the HTTP surface only. Peer names also reach the system through the deriver, dreamer, and queue, which have no route table to -enumerate; a guard gap there would not be caught here. +enumerate; a gap there would not be caught here. """ from collections.abc import Callable, Iterator +from dataclasses import dataclass, field import pytest from fastapi.routing import APIRoute @@ -42,176 +50,353 @@ from src.main import app from src.models import Peer, Workspace from src.utils.scopes import scope_peer_name -# Parameter and model-field names that carry a peer name. -_PEER_PARAM_NAMES = {"peer_id", "peer_name", "peer_names"} +# Request parameters and model fields that carry a peer name, in any position. +_PEER_PARAM_NAMES = { + "peer_id", + "peer_name", + "peer_names", + "observer", + "observer_id", + "observed", + "observed_id", + "target", + "peer_target", + "peer_perspective", +} + +# Peer names arriving as dict keys or an aliased body field are invisible to +# parameter-name detection, so these paths are matched by shape instead. The +# position recorded for them is the body field or key role. +_KEY_POSITION = "body_peer_keys" # The scopes router *is* the facade; scope peers are its whole subject. _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] -def _request_update_peer(client: TestClient, ws: str, _session: str, peer: str): - return client.put( - f"/v3/workspaces/{ws}/peers/{peer}", json={"metadata": {"k": "v"}} +@dataclass(frozen=True) +class Case: + """Policy for one peer position on one route.""" + + method: str + path: str + position: str + refuse: bool + reason: str = "" + build: Builder | None = None + # 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 + # Set when the squatter direction cannot be asserted here, with why. + skip_squatter: str = "" + _: tuple[()] = field(default=(), repr=False) + + @property + def key(self) -> tuple[str, str, str]: + return (self.method, self.path, self.position) + + +_W = "/v3/workspaces/{workspace_id}" + + +def _b_create_peer(c: TestClient, ws: str, _s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/peers", json={"id": p}) + + +def _b_update_peer(c: TestClient, ws: str, _s: str, p: str): + return c.put(f"/v3/workspaces/{ws}/peers/{p}", json={"metadata": {"k": "v"}}) + + +def _b_chat_observer(c: TestClient, ws: str, _s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/peers/{p}/chat", json={"query": "hi"}) + + +def _b_chat_target(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/peers/{_OTHER}/chat", json={"query": "hi", "target": p} ) -def _request_create_peer(client: TestClient, ws: str, _session: str, peer: str): - return client.post(f"/v3/workspaces/{ws}/peers", json={"id": peer}) +def _b_repr_observer(c: TestClient, ws: str, _s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/peers/{p}/representation", json={}) -def _request_chat(client: TestClient, ws: str, _session: str, peer: str): - return client.post( - f"/v3/workspaces/{ws}/peers/{peer}/chat", json={"query": "what do you know?"} +def _b_repr_target(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/peers/{_OTHER}/representation", json={"target": p} ) -def _request_representation(client: TestClient, ws: str, _session: str, peer: str): - return client.post(f"/v3/workspaces/{ws}/peers/{peer}/representation", json={}) +def _b_card_target(c: TestClient, ws: str, _s: str, p: str): + return c.put( + f"/v3/workspaces/{ws}/peers/{_OTHER}/card?target={p}", + json={"peer_card": ["note"]}, + ) -def _request_create_session_with_peer( - client: TestClient, ws: str, _session: str, peer: str -): - return client.post( +def _b_conclusion_observed(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/conclusions", + json={ + "conclusions": [ + { + "observer_id": _OTHER, + "observed_id": p, + "content": "something", + "level": "explicit", + } + ] + }, + ) + + +def _b_dream_observed(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/schedule_dream", + json={"observer": _OTHER, "observed": p, "dream_type": "omni"}, + ) + + +def _b_session_create(c: TestClient, ws: str, _s: str, p: str): + return c.post( f"/v3/workspaces/{ws}/sessions", - json={"id": str(generate_nanoid()), "peers": {peer: {}}}, + json={"id": str(generate_nanoid()), "peers": {p: {}}}, ) -def _request_create_message(client: TestClient, ws: str, session: str, peer: str): - return client.post( - f"/v3/workspaces/{ws}/sessions/{session}/messages", - json={"messages": [{"peer_id": peer, "content": "hello"}]}, +def _b_session_context_target(c: TestClient, ws: str, s: str, p: str): + return c.get(f"/v3/workspaces/{ws}/sessions/{s}/context?peer_target={p}") + + +def _b_message(c: TestClient, ws: str, s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/sessions/{s}/messages", + json={"messages": [{"peer_id": p, "content": "hello"}]}, ) -def _request_upload_message(client: TestClient, ws: str, session: str, peer: str): - return client.post( - f"/v3/workspaces/{ws}/sessions/{session}/messages/upload", - data={"peer_id": peer}, +def _b_upload(c: TestClient, ws: str, s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/sessions/{s}/messages/upload", + data={"peer_id": p}, files={"file": ("note.txt", b"hello there", "text/plain")}, ) -def _request_add_session_peers(client: TestClient, ws: str, session: str, peer: str): - return client.post(f"/v3/workspaces/{ws}/sessions/{session}/peers", json={peer: {}}) +def _b_add_peers(c: TestClient, ws: str, s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/sessions/{s}/peers", json={p: {}}) -def _request_set_session_peers(client: TestClient, ws: str, session: str, peer: str): - return client.put(f"/v3/workspaces/{ws}/sessions/{session}/peers", json={peer: {}}) +def _b_set_peers(c: TestClient, ws: str, s: str, p: str): + return c.put(f"/v3/workspaces/{ws}/sessions/{s}/peers", json={p: {}}) -def _request_remove_session_peers(client: TestClient, ws: str, session: str, peer: str): - return client.request( - "DELETE", f"/v3/workspaces/{ws}/sessions/{session}/peers", json=[peer] - ) +def _b_remove_peers(c: TestClient, ws: str, s: str, p: str): + return c.request("DELETE", f"/v3/workspaces/{ws}/sessions/{s}/peers", json=[p]) -def _request_set_peer_config(client: TestClient, ws: str, session: str, peer: str): - return client.put( - f"/v3/workspaces/{ws}/sessions/{session}/peers/{peer}/config", +def _b_peer_config(c: TestClient, ws: str, s: str, p: str): + return c.put( + f"/v3/workspaces/{ws}/sessions/{s}/peers/{p}/config", json={"observe_others": False, "observe_me": True}, ) -# Routes that must refuse a real scope peer. Each maps to a request builder so -# the guard is proven by calling it, not by trusting an annotation. -SCOPE_GUARDED_ROUTES: dict[tuple[str, str], Builder] = { - ("POST", "/v3/workspaces/{workspace_id}/peers"): _request_create_peer, - ("PUT", "/v3/workspaces/{workspace_id}/peers/{peer_id}"): _request_update_peer, - ("POST", "/v3/workspaces/{workspace_id}/peers/{peer_id}/chat"): _request_chat, - ( - "POST", - "/v3/workspaces/{workspace_id}/peers/{peer_id}/representation", - ): _request_representation, - ( - "POST", - "/v3/workspaces/{workspace_id}/sessions", - ): _request_create_session_with_peer, - ( - "POST", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/messages", - ): _request_create_message, - ( - "POST", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/messages/upload", - ): _request_upload_message, - ( - "POST", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers", - ): _request_add_session_peers, - ( - "PUT", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers", - ): _request_set_session_peers, - ( - "DELETE", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers", - ): _request_remove_session_peers, - ( - "PUT", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers/{peer_id}/config", - ): _request_set_peer_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" -# Routes a scope peer may legitimately reach. Every entry needs a reason: adding -# one is a deliberate decision that a scope in this position is harmless. -SCOPE_EXEMPT_ROUTES: dict[tuple[str, str], str] = { - ("GET", "/v3/workspaces/{workspace_id}/peers/{peer_id}/card"): ( - "peer_id is the *observer*. The Dreamer writes (scope, observed) cards, so " - "reading a scope's card of another peer is normal operation." +_OBSERVER_OK = ( + "Observer position. A scope observing others is the entire mechanism scopes " + "are built on, so this must keep working." +) +_READ_ONLY_OK = ( + "Read-only. Returns nothing meaningful for a scope rather than creating or " + "mutating knowledge about one." +) + +POLICY: tuple[Case, ...] = ( + # ---- observed position: a scope must never be the subject ---- + Case( + "POST", f"{_W}/conclusions", "observed_id", True, build=_b_conclusion_observed ), - ("PUT", "/v3/workspaces/{workspace_id}/peers/{peer_id}/card"): ( - "Same: peer_id is the observer. See DEV-1998 follow-up for the narrower " - "question of a scope's *self* card (target omitted), which is junk data " - "rather than a leak since nothing forms a representation of a scope." + 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, + build=_b_repr_target, ), - ("GET", "/v3/workspaces/{workspace_id}/peers/{peer_id}/context"): ( - "Read-only. Scope-as-path-peer on the context routes is closed in Phase 2b " - "(DEV-1998), which owns the read-side scope surface." - ), - ("POST", "/v3/workspaces/{workspace_id}/peers/{peer_id}/search"): ( - "Read-only, and empty by construction: a scope can never author messages, " - "so there is nothing to search." - ), - ("POST", "/v3/workspaces/{workspace_id}/peers/{peer_id}/sessions"): ( - "Read-only. A scope legitimately has member sessions; this is the " - "observer-mechanics view of what GET /scopes/{id}/sessions exposes." - ), - ("GET", "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers"): ( - "Read-only membership listing, which includes the scope observer." - ), - ( + Case( "GET", - "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers/{peer_id}/config", - ): "Read-only. The write side of this route IS guarded.", - ("POST", "/v3/keys"): ( - "Mints a scoped JWT rather than touching a peer. Scope-bound keys are " - "Phase 3 (DEV-2002)." + f"{_W}/sessions/{{session_id}}/context", + "peer_target", + True, + build=_b_session_context_target, ), -} + # ---- observer position: legitimately a scope ---- + Case("POST", f"{_W}/conclusions", "observer_id", False, reason=_OBSERVER_OK), + Case("POST", f"{_W}/schedule_dream", "observer", False, reason=_OBSERVER_OK), + Case("PUT", f"{_W}/peers/{{peer_id}}/card", "peer_id", False, reason=_OBSERVER_OK), + Case("GET", f"{_W}/peers/{{peer_id}}/card", "peer_id", False, reason=_OBSERVER_OK), + Case( + "GET", + f"{_W}/sessions/{{session_id}}/context", + "peer_perspective", + False, + reason=_OBSERVER_OK, + ), + Case("GET", f"{_W}/queue/status", "observer_id", False, reason=_OBSERVER_OK), + # ---- peer identity / membership mutation: never a scope ---- + Case( + "POST", + f"{_W}/peers", + _KEY_POSITION, + True, + build=_b_create_peer, + schema_level=True, + skip_squatter=( + "Creating any name in the reserved namespace is refused whether flagged " + "or not — that is what reserving it means. Covered by " + "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( + "POST", + f"{_W}/sessions/{{session_id}}/messages", + "peer_name", + True, + build=_b_message, + ), + Case( + "POST", + f"{_W}/sessions/{{session_id}}/messages/upload", + "peer_id", + True, + build=_b_upload, + ), + Case( + "POST", + f"{_W}/sessions/{{session_id}}/peers", + _KEY_POSITION, + True, + build=_b_add_peers, + ), + Case( + "PUT", + f"{_W}/sessions/{{session_id}}/peers", + _KEY_POSITION, + True, + build=_b_set_peers, + ), + Case( + "DELETE", + f"{_W}/sessions/{{session_id}}/peers", + _KEY_POSITION, + True, + build=_b_remove_peers, + ), + Case( + "PUT", + f"{_W}/sessions/{{session_id}}/peers/{{peer_id}}/config", + "peer_id", + True, + build=_b_peer_config, + ), + # ---- path peer on the dialectic surface ---- + Case( + "POST", + f"{_W}/peers/{{peer_id}}/chat", + "peer_id", + True, + build=_b_chat_observer, + ), + Case( + "POST", + f"{_W}/peers/{{peer_id}}/representation", + "peer_id", + 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 + ), + Case( + "POST", + f"{_W}/peers/{{peer_id}}/sessions", + "peer_id", + False, + reason=( + "Read-only. A scope legitimately has member sessions; this is the " + "observer-mechanics view of GET /scopes/{scope_id}/sessions." + ), + ), + Case( + "GET", + f"{_W}/peers/{{peer_id}}/context", + "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." + ), + ), + Case( + "GET", + f"{_W}/peers/{{peer_id}}/context", + "target", + False, + reason=( + "Read-only, and empty for a scope now that nothing can write knowledge " + "about one. Phase 2b (DEV-1998) owns this surface." + ), + ), + Case( + "GET", + f"{_W}/peers/{{peer_id}}/card", + "target", + False, + reason=( + "Read-only. The write side (PUT with target) IS refused, so this can only " + "return pre-existing rows, never create them." + ), + ), + Case( + "POST", + "/v3/keys", + "peer_id", + False, + reason=( + "Mints a scoped JWT rather than touching a peer. Scope-bound keys are " + "Phase 3 (DEV-2002)." + ), + ), +) -# Routes excluded from the squatter check, with why. The guarded direction is -# still asserted for all of these. -_SQUATTER_CHECK_SKIPS: dict[tuple[str, str], str] = { - ("POST", "/v3/workspaces/{workspace_id}/peers"): ( - "Creating a reserved-prefix peer is refused for ANY name in the namespace, " - "flagged or not — that is the point of reserving it. Covered by " - "test_scopes.py::test_peer_create_rejects_reserved_prefix." - ), -} +_BY_KEY = {case.key: case for case in POLICY} -# Routes whose 422 legitimately comes from request-schema validation rather than a -# scope guard, so the response detail is pydantic's rather than ours. Everything -# else must name the offending peer AND say why, or the test would happily pass on -# an unrelated 422 (a malformed body, say) and prove nothing. -_SCHEMA_LEVEL_REJECTION: dict[tuple[str, str], str] = { - ("POST", "/v3/workspaces/{workspace_id}/peers"): ( - "PeerCreate.name carries RESOURCE_NAME_PATTERN, which the reserved prefix " - "violates, so pydantic refuses it before the route body runs." - ), +# Routes whose peer names arrive as dict keys or an aliased body field, invisible +# to parameter-name detection and therefore matched by path shape. +_KEY_POSITION_PATHS = { + ("POST", f"{_W}/peers"), + ("POST", f"{_W}/sessions/{{session_id}}/peers"), + ("PUT", f"{_W}/sessions/{{session_id}}/peers"), + ("DELETE", f"{_W}/sessions/{{session_id}}/peers"), } @@ -227,19 +412,19 @@ def _nested_models(annotation: object, seen: set[object]) -> Iterator[type[BaseM seen.add(model) yield model fields: dict[str, FieldInfo] = model.model_fields - for field in fields.values(): - stack = [field.annotation] + for f in fields.values(): + stack = [f.annotation] while stack: current = stack.pop() yield from _nested_models(current, seen) stack.extend(getattr(current, "__args__", ()) or ()) -def _peer_param_names(route: APIRoute) -> set[str]: - """Peer-name-carrying params anywhere in a route's dependant tree. +def _peer_positions(route: APIRoute) -> set[str]: + """Peer-name-carrying parameter names anywhere in a route's dependant tree. Walks sub-dependencies so `Form(...)` params behind a parser dependency are - seen (this is how `messages/upload` takes its `peer_id`), and descends into + seen — this is how `messages/upload` takes its `peer_id` — and descends into request-body models so `MessageCreate.peer_name` is seen too. """ found: set[str] = set() @@ -266,95 +451,89 @@ def _peer_param_names(route: APIRoute) -> set[str]: return found -def _peer_touching_routes() -> set[tuple[str, str]]: - """Every (method, path) through which a peer name can reach the system. - - Union of two signals, because neither alone is sufficient: parameter names - miss routes where peer names are dict *keys* (`POST /sessions/{id}/peers` - takes `dict[str, SessionPeerConfig]`), and path shape misses names carried in - a body or form field. - """ - found: set[tuple[str, str]] = set() +def _derived_positions() -> set[tuple[str, str, str]]: + """Every (method, path, position) through which a peer name can be supplied.""" + found: set[tuple[str, str, str]] = set() for route in app.routes: if not isinstance(route, APIRoute): continue path = route.path.rstrip("/") or route.path if path.startswith(_SCOPES_PREFIX): continue - by_shape = "{peer_id}" in path or path.endswith("/peers") - if not (by_shape or _peer_param_names(route)): - continue + positions = _peer_positions(route) for method in route.methods or set(): - if method not in ("HEAD", "OPTIONS"): - found.add((method, path)) + if method in ("HEAD", "OPTIONS"): + continue + if (method, path) in _KEY_POSITION_PATHS: + found.add((method, path, _KEY_POSITION)) + for position in positions: + found.add((method, path, position)) return found -def test_every_peer_touching_route_is_classified(): - """Each peer-touching route is either guarded or explicitly exempt. +def test_every_peer_position_is_classified(): + """Each (route, peer position) pair has an explicit scope policy. - A new route fails here until classified. If this fails for a route you added, - decide whether a scope peer reaching it is harmful: add it to - SCOPE_GUARDED_ROUTES with a request builder, or to SCOPE_EXEMPT_ROUTES with a - reason. Do not add a mutating route to the exempt set. + A new one fails here until classified. Decide whether a scope in that + *position* is harmful — the rule is that a scope may be an observer but never + observed — then add a Case with `refuse=True` and a builder, or `refuse=False` + and a reason. """ - classified = set(SCOPE_GUARDED_ROUTES) | set(SCOPE_EXEMPT_ROUTES) - actual = _peer_touching_routes() + derived = _derived_positions() + classified = set(_BY_KEY) - unclassified = actual - classified + unclassified = derived - classified assert not unclassified, ( - "peer-touching routes with no scope policy: " - + f"{sorted(unclassified)} — classify each as guarded or exempt" + "peer positions with no scope policy: " + + f"{sorted(unclassified)} — classify each as refuse or allow" ) - stale = classified - actual - assert not stale, f"classified routes that no longer exist: {sorted(stale)}" + stale = classified - derived + assert not stale, f"classified positions that no longer exist: {sorted(stale)}" -def test_guarded_and_exempt_sets_are_disjoint(): - overlap = set(SCOPE_GUARDED_ROUTES) & set(SCOPE_EXEMPT_ROUTES) - assert not overlap, f"routes classified both ways: {sorted(overlap)}" +def test_policy_entries_are_well_formed(): + assert len(_BY_KEY) == len(POLICY), "duplicate (method, path, position) in POLICY" + for case in POLICY: + 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" + else: + assert case.build is None, f"{case.key} allows but has a builder" + assert len(case.reason.strip()) > 30, f"{case.key} needs a real reason" -def test_exempt_routes_all_state_a_reason(): - for route, reason in SCOPE_EXEMPT_ROUTES.items(): - assert len(reason.strip()) > 30, f"{route} needs a real reason, got {reason!r}" +_REFUSING = tuple(case for case in POLICY if case.refuse) -def test_skip_list_only_covers_guarded_routes(): - """The exception lists must not drift out of the guarded set.""" - for name, entries in ( - ("_SQUATTER_CHECK_SKIPS", _SQUATTER_CHECK_SKIPS), - ("_SCHEMA_LEVEL_REJECTION", _SCHEMA_LEVEL_REJECTION), - ): - unknown = set(entries) - set(SCOPE_GUARDED_ROUTES) - assert not unknown, f"{name} references non-guarded routes: {sorted(unknown)}" - - -@pytest.mark.parametrize( - ("method", "path"), - sorted(SCOPE_GUARDED_ROUTES), - ids=lambda v: v if isinstance(v, str) else str(v), -) -def test_guarded_route_refuses_a_real_scope( - client: TestClient, - sample_data: tuple[Workspace, Peer], - method: str, - path: str, -): - """Every guarded route refuses a peer that is a real scope.""" - test_workspace, _ = sample_data - scope_name = str(generate_nanoid()) - response = client.post( - f"/v3/workspaces/{test_workspace.name}/scopes", json={"id": scope_name} - ) - assert response.status_code == 201, response.text - backing = scope_peer_name(scope_name) - +def _setup(client: TestClient, workspace: str) -> tuple[str, str]: + """Create the counterparty peer and a session, returning (session, scope name).""" + assert client.post( + f"/v3/workspaces/{workspace}/peers", json={"id": _OTHER} + ).status_code in (200, 201) session_name = str(generate_nanoid()) assert client.post( - f"/v3/workspaces/{test_workspace.name}/sessions", json={"id": session_name} + f"/v3/workspaces/{workspace}/sessions", json={"id": session_name} ).status_code in (200, 201) + return session_name, str(generate_nanoid()) + + +@pytest.mark.parametrize("case", _REFUSING, ids=lambda c: f"{c.method}:{c.position}") +def test_refusing_position_rejects_a_real_scope( + client: TestClient, + sample_data: tuple[Workspace, Peer], + case: Case, +): + """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", @@ -363,55 +542,52 @@ def test_guarded_route_refuses_a_real_scope( == 200 ) - build = SCOPE_GUARDED_ROUTES[(method, path)] - result = build(client, test_workspace.name, session_name, backing) + assert case.build is not None + result = case.build(client, test_workspace.name, session_name, backing) status = getattr(result, "status_code", None) - assert status == 422, f"{method} {path} accepted a scope peer (got {status})" + 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 request body would also produce - # one. Require the rejection to actually be about this scope. + # A 422 alone proves nothing — a malformed body would also produce one. detail = str(getattr(result, "text", "")) - if (method, path) in _SCHEMA_LEVEL_REJECTION: - assert "pattern" in detail, ( - f"{method} {path} was expected to be refused by schema validation, " - f"but the detail does not mention the pattern: {detail[:200]}" - ) + if case.schema_level: + assert ( + "pattern" in detail + ), f"{case.key} expected a schema-level refusal; detail: {detail[:200]}" else: assert "scope" in detail.lower() and backing in detail, ( - f"{method} {path} returned 422 but not because of the scope — " + f"{case.key} returned 422 but not because of the scope; " f"detail: {detail[:200]}" ) @pytest.mark.parametrize( - ("method", "path"), - sorted(set(SCOPE_GUARDED_ROUTES) - set(_SQUATTER_CHECK_SKIPS)), - ids=lambda v: v if isinstance(v, str) else str(v), + "case", + tuple(c for c in _REFUSING if not c.skip_squatter), + ids=lambda c: f"{c.method}:{c.position}", ) -async def test_guarded_route_allows_unflagged_squatter( +async def test_refusing_position_allows_unflagged_squatter( client: TestClient, db_session: AsyncSession, sample_data: tuple[Workspace, Peer], - method: str, - path: str, + case: Case, ): """A peer merely occupying the reserved namespace is not a scope. Peer names were length-validated only before migration d429de0e5338, so `scope.production` is a possible real user name. Such a peer has only the name - half of the invariant and must keep working — guards that key off the prefix - alone lock a tenant out of its own data. + half of the invariant and must keep working — a guard keying off the prefix + alone locks a tenant out of its own data. """ test_workspace, _ = sample_data + session_name, _ = _setup(client, test_workspace.name) 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 = str(generate_nanoid()) - assert client.post( - f"/v3/workspaces/{test_workspace.name}/sessions", json={"id": session_name} - ).status_code in (200, 201) - # Give the squatter a membership so config/removal routes have a row to act on. + # Give it a membership so config and removal have a row to act on. assert ( client.post( f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", @@ -420,10 +596,11 @@ async def test_guarded_route_allows_unflagged_squatter( == 200 ) - build = SCOPE_GUARDED_ROUTES[(method, path)] - result = build(client, test_workspace.name, session_name, 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"{method} {path} refused an unflagged squatter (got {status}) — " - "the guard is keying off the name prefix rather than the scope flag" + 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" )