From 5e989794b630129166a4b825f183e38dcb3005a4 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:07:52 -0400 Subject: [PATCH] fix(scopes): refuse future scopes in observed positions, preserve scope membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses a fifth review pass against 14136e5b. All six findings reproduced locally before fixing. 1. High — generic peer replacement removed scope memberships. `set_peers_for_session` soft-deleted every active SessionPeer row, and the request-level guard only inspected names *present* in the replacement map. A caller detached a scope by simply omitting it, never naming it — so no request-level guard could ever see it. Reproduced: scope sessions went [''] -> [] on a 200. The exclusion now lives in the UPDATE itself (correlated NOT EXISTS against scope_peer_clause), so replacement means "replace ordinary peers" regardless of request contents or concurrent creation. 2. High — peer cards could be pre-seeded for future scopes. `set_peer_card` resolves only the observer and writes a JSONB key derived from an unchecked observed name, and the route guard rejected only *existing* flagged scopes. Reproduced: PUT card with target=scope. returned 200, creating that scope then returned 201, and the card described the real scope. 3. High — dreams could be queued for future scopes. The route checked `observed` in a read-only session that closed before `enqueue_dream`, and a missing reserved name passes any is-it-a-scope check. Reproduced: 204 with observed=scope.. 2 and 3 share a root cause, so they share a fix: a new `reject_scope_observed` that is stricter than `reject_scope_peers` in exactly one case — a *missing* reserved name is refused, because nothing on these paths creates the peer, so nothing else would ever catch it. Existing unflagged squatters still pass. Both guards moved to the mutation point: card validation into `crud.set_peer_card` (same transaction as the JSONB write, so Dreamer and agent-tool callers are covered), dream validation into `enqueue_dream` (same transaction as the queue insert). The redundant route-level checks are dropped rather than left as weaker duplicates. 4. Medium — prefixed NUL names still reached PostgreSQL. `reject_scope_peers` filtered for the reserved prefix and sent matches to a text comparison, so "scope.future\0name" raised psycopg.DataError — a 500. Both guards now share `_reserved_name_candidates`, which materializes the input once and rejects impossible values before any SQL. Materializing matters independently: the message-author path passes a generator, and validation iterates separately from the prefix filter, so a generator would be half-consumed. `_reject_impossible_peer_names` now takes a Collection so the type checker enforces that. 5. Medium — representation kept a check-to-use race. The previous commit claimed both representation roles were rechecked after resolution; that was wrong — only the dialectic preflight got that check, and the representation route never goes through it. It now opens one short read-only session *after* the embedding call, checks both positions, and passes that same session to `get_working_representation`, so no connection is held across external work and a scope committed later cannot have conclusions in the collection being read. 6. Low — policy coverage was not exhaustive. `sender_id` reaches CRUD as `observed` but was missing from the detected parameter set. ALLOW cases could also not carry builders, so the suite never proved the other half of the contract — that legitimate scope *observers* keep working, which a guard rejecting scopes everywhere would satisfy. Both fixed; observer positions on conclusions, dreams, cards, session context and queue status are now asserted behaviorally. Deliberately not implemented: the scope-creation backstop scanning for pre-existing card keys and queue items naming a future backing peer. Reasoning is recorded in `get_or_create_scopes` — no new such state can be created now, any pre-existing row is coincidental since `scope.` was never a meaningful namespace, the consequence is inert, and detecting card keys means a full table scan per scope creation. Mutation-tested each new guard: removing the replacement exclusion fails both membership-preservation tests; weakening either observed guard to existing-only fails the pre-seeding tests. Co-Authored-By: Claude Opus 5 (1M context) --- src/crud/__init__.py | 2 + src/crud/peer.py | 90 +++++++- src/crud/peer_card.py | 22 +- src/crud/scope.py | 12 ++ src/crud/session.py | 16 +- src/deriver/enqueue.py | 16 ++ src/routers/peers.py | 86 ++++---- src/routers/workspaces.py | 20 +- tests/conftest.py | 1 - tests/routes/test_scope_route_policy.py | 166 ++++++++++++++- tests/routes/test_scopes.py | 261 ++++++++++++++++++++++++ 11 files changed, 628 insertions(+), 64 deletions(-) diff --git a/src/crud/__init__.py b/src/crud/__init__.py index a525149d..4a33094a 100644 --- a/src/crud/__init__.py +++ b/src/crud/__init__.py @@ -39,6 +39,7 @@ from .peer import ( get_peer, get_peers, get_sessions_for_peer, + reject_scope_observed, reject_scope_peers, update_peer, ) @@ -123,6 +124,7 @@ __all__ = [ # Peer "get_or_create_peers", "get_peer", + "reject_scope_observed", "reject_scope_peers", "get_peers", "update_peer", diff --git a/src/crud/peer.py b/src/crud/peer.py index 881fc5b6..37c2caa2 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -1,7 +1,7 @@ """CRUD helpers for peer records and peer-scoped session queries.""" import re -from collections.abc import Iterable +from collections.abc import Collection, Iterable from logging import getLogger from typing import Any, Literal @@ -47,7 +47,7 @@ def peer_cache_key(workspace_name: str, peer_name: str) -> str: ) -def _reject_impossible_peer_names(names: Iterable[str]) -> None: +def _reject_impossible_peer_names(names: Collection[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 @@ -60,6 +60,10 @@ def _reject_impossible_peer_names(names: Iterable[str]) -> None: - Over-length names: the ``peers.name`` CHECK caps them at ``PEER_NAME_MAX_LENGTH``, so no stored row can exceed it. + Takes a ``Collection`` rather than an ``Iterable`` on purpose: it inspects the + input twice, so a generator would be half-consumed and the second check would + silently see nothing. + Raises: ValidationException: On a NUL byte or an over-length name. """ @@ -113,6 +117,82 @@ def scope_peer_clause() -> ColumnElement[bool]: ) +async 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 + path does), and validating impossible names iterates the input separately from + the prefix filter — a generator would be silently half-consumed. + + Impossible values are refused here, before any SQL, because a reserved-prefix + name containing a NUL byte would otherwise reach the text comparison below and + raise ``psycopg.DataError`` inside the query — a 500 instead of the 422 the + caller should get. + + Raises: + ValidationException: On a NUL byte or an over-length name. + """ + materialized = tuple(names) + _reject_impossible_peer_names(materialized) + return sorted({n for n in materialized if scopes_util.is_scope_peer_name(n)}) + + +async def reject_scope_observed( + db: AsyncSession, + workspace_name: str, + names: Iterable[str], + *, + action: str, +) -> None: + """Reject any name that is — or could later become — an observed scope. + + Stricter than ``reject_scope_peers`` in exactly one case: a **missing** + reserved name is refused. Use this for the *observed* position, where nothing + creates the peer and so nothing else would ever catch it. Without it a caller + can pre-seed state about ``scope.future`` while that peer does not exist, then + create the scope and have the state retroactively describe it. + + Three-way on the reserved namespace: + + ============================== ====== + State Result + ============================== ====== + Existing flagged scope reject + Missing reserved name reject + Existing unflagged squatter allow + ============================== ====== + + Non-reserved names are left entirely to the caller's own existence semantics. + + Raises: + ValidationException: On a real scope or a missing reserved name. + """ + candidates = await _reserved_name_candidates(names) + if not candidates: + return + + rows = ( + await db.execute( + select(models.Peer.name, scope_peer_clause()) + .where(models.Peer.workspace_name == workspace_name) + .where(models.Peer.name.in_(candidates)) + ) + ).all() + flagged = {name for name, is_scope in rows if is_scope} + existing = {name for name, _ in rows} + + scopes = sorted(flagged) + if scopes: + raise ValidationException(f"Peer name(s) {scopes} are scopes. {action}") + + missing = sorted(set(candidates) - existing) + if missing: + raise ValidationException( + f"Peer name(s) {missing} are in the reserved scope namespace and do" + + f" not exist, so they may become scopes later. {action}" + ) + + async def reject_scope_peers( db: AsyncSession, workspace_name: str, @@ -127,13 +207,17 @@ async def reject_scope_peers( ``d429de0e5338``, so ``scope.production`` is a possible user name) keeps working normally instead of being locked out of its own data. + A *missing* reserved name passes here — the create paths this guards + (`get_or_create_peers`) refuse it themselves. Positions where nothing creates + the peer need ``reject_scope_observed`` instead. + Costs nothing on the common path: with no reserved-prefix name in ``names`` there is no query at all. Raises: ValidationException: If any name resolves to a real scope peer. """ - candidates = sorted({n for n in names if scopes_util.is_scope_peer_name(n)}) + candidates = await _reserved_name_candidates(names) if not candidates: return diff --git a/src/crud/peer_card.py b/src/crud/peer_card.py index b17361d2..38208a37 100644 --- a/src/crud/peer_card.py +++ b/src/crud/peer_card.py @@ -9,7 +9,12 @@ from sqlalchemy.ext.asyncio import AsyncSession from src import exceptions, models, schemas from src.cache.client import safe_cache_delete -from src.crud.peer import get_or_create_peers, get_peer, peer_cache_key +from src.crud.peer import ( + get_or_create_peers, + get_peer, + peer_cache_key, + reject_scope_observed, +) logger = logging.getLogger(__name__) @@ -68,6 +73,21 @@ async def set_peer_card( observer: Peer name of the observer """ + # A scope may be the card's *observer* — the Dreamer writes (scope, observed) + # cards — but never its subject. Authoritative here rather than only in the + # route, so the Dreamer and agent-tool paths are covered too, and in the same + # transaction as the JSONB write below. + # + # A *missing* reserved name is refused as well: only the observer is resolved + # below, so a card keyed on `scope.future` would otherwise persist while that + # peer does not exist and retroactively describe the scope once created. + await reject_scope_observed( + db, + workspace_name, + [observed], + action="No peer card is ever formed about a scope.", + ) + # Ensure the peer exists (get-or-create) peers_result = await get_or_create_peers( db, workspace_name, [schemas.PeerSpec(name=observer)] diff --git a/src/crud/scope.py b/src/crud/scope.py index 16ee5ca4..7bd554c9 100644 --- a/src/crud/scope.py +++ b/src/crud/scope.py @@ -73,6 +73,18 @@ async def get_or_create_scopes( Note: does not commit; the caller owns the transaction (mirror of ``get_or_create_peers``). Run ``result.post_commit()`` after committing. + Deliberately does NOT scan for pre-existing state naming the backing peer + (peer-card keys, pending dream queue items). ``reject_scope_observed`` now + refuses writes against a not-yet-existing reserved name, so no new such state + can be created; only data written before that guard existed could collide, and + since ``scope.`` was never a meaningful namespace then, any such row is + coincidental. The consequence would also be inert — a card or queue item + describing a scope, which nothing reads, because no representation is formed of + a scope. Detecting card keys means scanning every peer's ``internal_metadata`` + for a label containing this name, i.e. a full table scan per scope creation: + disproportionate to that risk. Revisit if scope names ever become guessable + across tenants. + Args: db: Database session workspace_name: Name of the workspace diff --git a/src/crud/session.py b/src/crud/session.py index 9daa130a..2f508b59 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -1048,13 +1048,27 @@ async def set_peers_for_session( f"Session {session_name} not found in workspace {workspace_name}" ) - # Soft delete specified session peers by setting left_at timestamp + # Soft delete every *ordinary* active membership. Scope memberships are + # deliberately preserved: this route replaces the peers the caller names, and a + # caller detaches a scope by simply *omitting* it from an otherwise valid + # replacement map — never naming it, so no request-level guard can see it. + # Without the exclusion a plain replacement silently bypasses the facade that + # owns scope membership and its removal reconciliation. Being part of the + # UPDATE, this holds regardless of the request body or concurrent scope + # creation. update_stmt = ( update(models.SessionPeer) .where( models.SessionPeer.session_name == session_name, models.SessionPeer.workspace_name == workspace_name, models.SessionPeer.left_at.is_(None), # Only update active peers + ~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) + ), ) .values(left_at=func.now()) ) diff --git a/src/deriver/enqueue.py b/src/deriver/enqueue.py index 7cd42aa3..f62a3db3 100644 --- a/src/deriver/enqueue.py +++ b/src/deriver/enqueue.py @@ -477,6 +477,22 @@ async def enqueue_dream( rebuild: card_refresh only — rebuild the card without the prior card """ async with tracked_db("dream_enqueue") as db_session: + # Authoritative scope check, in the same transaction as the queue insert. + # A route-level precheck cannot be relied on: it runs in its own session, + # and a *missing* reserved name passes it (nothing has flagged that peer + # yet) — so the dream would be enqueued and the scope created before the + # worker picked it up, letting the Dreamer run with a real scope as + # observed. A scope as `observer` stays allowed: consolidating scoped + # collections is exactly what the Dreamer does. + await crud.reject_scope_observed( + db_session, + workspace_name, + [observed], + action=( + "No representation is formed of a scope, so a scope cannot be the" + " observed peer of a dream." + ), + ) try: dream_record = create_dream_record( workspace_name, diff --git a/src/routers/peers.py b/src/routers/peers.py index 20480c79..bbcfa214 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -371,8 +371,10 @@ async def get_representation( If a target is provided, we get the Representation of the target from the perspective of the Peer. If no target is provided, we get the omniscient Honcho Representation of the Peer. """ - # Scope peers are never observed, so no representation of them exists. - # Covers the path-level observer as well as the target. + # Fast-fail before any embedding work. Same guard as the authoritative one + # below, so a reserved name is refused here rather than after paying for an + # embedding; the check is repeated at the read because this session closes and + # a scope could be created in between. scope_candidates = [ n for n in (peer_id, options.target) if n is not None and is_scope_peer_name(n) ] @@ -380,7 +382,7 @@ async def get_representation( async with tracked_db( "peers.representation.scope_check", read_only=True ) as s_db: - await crud.reject_scope_peers( + await crud.reject_scope_observed( s_db, workspace_id, scope_candidates, @@ -409,26 +411,47 @@ async def get_representation( ): embedding = await embedding_client.embed(options.search_query) - # If no target specified, get global representation (omniscient Honcho perspective) - representation = await crud.get_working_representation( - workspace_id, - observer=peer_id, - observed=options.target if options.target is not None else peer_id, - session_allowlist=[options.session_id] - if options.session_id is not None - else session_allowlist, - include_semantic_query=options.search_query, - embedding=embedding, - semantic_search_top_k=options.search_top_k, - semantic_search_max_distance=options.search_max_distance, - include_most_derived=options.include_most_frequent - if options.include_most_frequent is not None - else False, - max_observations=options.max_conclusions - if options.max_conclusions is not None - else settings.DERIVER.WORKING_REPRESENTATION_MAX_OBSERVATIONS, - parent_category="api", - ) + 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 + # embedding call above, so no connection is held across external work. + # The early check ran in a session that has since closed and, being + # name-based, also passed any reserved name that did not yet exist; a scope + # created in between would otherwise be used here. Sharing the session with + # the read means a scope committed after this check cannot have any + # conclusions in the collection the read then examines. + async with tracked_db( + "peers.representation.read", read_only=True + ) as read_session: + await crud.reject_scope_observed( + read_session, + workspace_id, + {peer_id, observed}, + action=( + "No representation is formed of a scope, so a scope cannot be" + " a representation observer or target." + ), + ) + # If no target specified, this is the global (omniscient) representation + representation = await crud.get_working_representation( + workspace_id, + db=read_session, + observer=peer_id, + observed=observed, + session_allowlist=[options.session_id] + if options.session_id is not None + else session_allowlist, + include_semantic_query=options.search_query, + embedding=embedding, + semantic_search_top_k=options.search_top_k, + semantic_search_max_distance=options.search_max_distance, + include_most_derived=options.include_most_frequent + if options.include_most_frequent is not None + else False, + max_observations=options.max_conclusions + if options.max_conclusions is not None + else settings.DERIVER.WORKING_REPRESENTATION_MAX_OBSERVATIONS, + parent_category="api", + ) return schemas.RepresentationResponse( representation=representation.format_as_markdown() ) @@ -494,19 +517,10 @@ 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." - ), - ) - + # The scope guard lives in crud.set_peer_card, in the same transaction as the + # JSONB write, so the Dreamer and agent-tool paths are covered too. Nothing + # expensive happens between here and there, so a duplicate early check would + # only cost an extra query. await crud.set_peer_card( db, workspace_id, diff --git a/src/routers/workspaces.py b/src/routers/workspaces.py index 2184dfcb..f7bcdcd2 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, tracked_db +from src.dependencies import db, read_db from src.deriver.enqueue import enqueue_deletion, enqueue_dream from src.exceptions import AuthenticationException from src.security import JWTParams, require_auth @@ -225,21 +225,9 @@ 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." - ), - ) + # The authoritative observed-position check lives in enqueue_dream, in the same + # transaction as the queue insert. Nothing expensive happens before it here, so + # no early duplicate is needed. await enqueue_dream( workspace_id, diff --git a/tests/conftest.py b/tests/conftest.py index 93db47e5..b3697242 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -840,7 +840,6 @@ 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 2bff9dc1..735156b1 100644 --- a/tests/routes/test_scope_route_policy.py +++ b/tests/routes/test_scope_route_policy.py @@ -59,6 +59,7 @@ _PEER_PARAM_NAMES = { "observer_id", "observed", "observed_id", + "sender_id", "target", "peer_target", "peer_perspective", @@ -91,6 +92,9 @@ class Case: schema_level: bool = False # Set when the squatter direction cannot be asserted here, with why. skip_squatter: str = "" + # 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 @@ -197,6 +201,53 @@ 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 _b_conclusion_observer(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/conclusions", + json={ + "conclusions": [ + { + "observer_id": p, + "observed_id": _OTHER, + "content": "something", + "level": "explicit", + } + ] + }, + ) + + +def _b_dream_observer(c: TestClient, ws: str, _s: str, p: str): + return c.post( + f"/v3/workspaces/{ws}/schedule_dream", + json={"observer": p, "observed": _OTHER, "dream_type": "omni"}, + ) + + +def _b_card_observer_put(c: TestClient, ws: str, _s: str, p: str): + return c.put( + f"/v3/workspaces/{ws}/peers/{p}/card?target={_OTHER}", + json={"peer_card": ["note"]}, + ) + + +def _b_card_observer_get(c: TestClient, ws: str, _s: str, p: str): + return c.get(f"/v3/workspaces/{ws}/peers/{p}/card?target={_OTHER}") + + +def _b_context_perspective(c: TestClient, ws: str, s: str, p: str): + query = f"?peer_perspective={p}&peer_target={_OTHER}" + return c.get(f"/v3/workspaces/{ws}/sessions/{s}/context{query}") + + +def _b_queue_status_observer(c: TestClient, ws: str, _s: str, p: str): + return c.get(f"/v3/workspaces/{ws}/queue/status?observer_id={p}") + + +def _b_queue_status_sender(c: TestClient, ws: str, _s: str, p: str): + return c.get(f"/v3/workspaces/{ws}/queue/status?sender_id={p}") + + def _b_peer_config(c: TestClient, ws: str, s: str, p: str): return c.put( f"/v3/workspaces/{ws}/sessions/{s}/peers/{p}/config", @@ -240,18 +291,72 @@ POLICY: tuple[Case, ...] = ( 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( + "POST", + f"{_W}/conclusions", + "observer_id", + False, + reason=_OBSERVER_OK, + build=_b_conclusion_observer, + allow_status=(200, 201), + ), + Case( + "POST", + f"{_W}/schedule_dream", + "observer", + False, + reason=_OBSERVER_OK, + build=_b_dream_observer, + allow_status=(204,), + ), + Case( + "PUT", + f"{_W}/peers/{{peer_id}}/card", + "peer_id", + False, + reason=_OBSERVER_OK, + build=_b_card_observer_put, + allow_status=(200,), + ), + Case( + "GET", + f"{_W}/peers/{{peer_id}}/card", + "peer_id", + False, + reason=_OBSERVER_OK, + build=_b_card_observer_get, + allow_status=(200,), + ), Case( "GET", f"{_W}/sessions/{{session_id}}/context", "peer_perspective", False, reason=_OBSERVER_OK, + build=_b_context_perspective, + allow_status=(200,), + ), + Case( + "GET", + f"{_W}/queue/status", + "observer_id", + False, + reason=_OBSERVER_OK, + build=_b_queue_status_observer, + allow_status=(200,), + ), + Case( + "GET", + f"{_W}/queue/status", + "sender_id", + False, + reason=( + "Filter only. `sender_id` reaches CRUD as `observed`, but it selects " + "existing queue rows rather than creating knowledge about a peer." + ), + build=_b_queue_status_sender, + allow_status=(200,), ), - Case("GET", f"{_W}/queue/status", "observer_id", False, reason=_OBSERVER_OK), # ---- peer identity / membership mutation: never a scope ---- Case( "POST", @@ -499,11 +604,18 @@ def test_policy_entries_are_well_formed(): 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" + 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" + ) _REFUSING = tuple(case for case in POLICY if case.refuse) +# Allow cases that additionally prove, behaviorally, that a real scope works here. +_ALLOWING_EXERCISED = tuple( + case for case in POLICY if not case.refuse and case.build is not None +) def _setup(client: TestClient, workspace: str) -> tuple[str, str]: @@ -563,6 +675,48 @@ def test_refusing_position_rejects_a_real_scope( ) +@pytest.mark.parametrize( + "case", _ALLOWING_EXERCISED, ids=lambda c: f"{c.method}:{c.position}" +) +def test_allowing_position_accepts_a_real_scope( + client: TestClient, + sample_data: tuple[Workspace, Peer], + case: Case, +): + """A real scope works in every position marked ALLOW. + + The other half of the contract. Refusal tests alone would be satisfied by a + guard that rejected scopes everywhere, which would break the feature: scoped + conclusions, scoped dreams and scoped peer cards all require a scope in the + observer position. + """ + 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 + ) + + 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, ( + 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]}" + ) + + @pytest.mark.parametrize( "case", tuple(c for c in _REFUSING if not c.skip_squatter), diff --git a/tests/routes/test_scopes.py b/tests/routes/test_scopes.py index e301ba7f..1f17d5d5 100644 --- a/tests/routes/test_scopes.py +++ b/tests/routes/test_scopes.py @@ -1071,3 +1071,264 @@ def test_degenerate_peer_names_are_422_not_500( json={"messages": [{"peer_id": bad_name, "content": "hello"}]}, ) assert response.status_code == 422, response.text + + +# --------------------------------------------------------------------------- +# 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. +# --------------------------------------------------------------------------- + + +def test_generic_replacement_preserves_scope_membership( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """`PUT /sessions/{id}/peers` replaces ordinary peers, never scopes. + + The guard cannot key off the request body: a caller detaches a scope by simply + *omitting* it from an otherwise valid replacement map, never naming it. + """ + test_workspace, test_peer = 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) + assert ( + client.post( + f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", + json={"session_ids": [session_name]}, + ).status_code + == 200 + ) + + # Replacement naming only an ordinary peer must succeed... + response = client.put( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={test_peer.name: {}}, + ) + 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] + + +def test_empty_replacement_preserves_scope_membership( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """An empty replacement map clears ordinary peers but not scopes.""" + test_workspace, test_peer = 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) + client.post( + 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]}, + ) + + assert ( + client.put( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={}, + ).status_code + == 200 + ) + + response = client.get( + f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions" + ) + assert response.json()["session_ids"] == [session_name] + + +async def test_replacement_still_removes_unflagged_squatter( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """The preservation is flag-based: a squatter keeps ordinary semantics.""" + 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() + + 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 + ) + + assert ( + client.put( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={test_peer.name: {}}, + ).status_code + == 200 + ) + session_peer = await _get_session_peer( + db_session, test_workspace.name, session_name, squatter + ) + assert session_peer is not None + 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 + + +async def test_set_peer_card_guard_covers_internal_callers( + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """The guard is in crud, so Dreamer and agent-tool paths are covered too.""" + test_workspace, test_peer = sample_data + with pytest.raises(ValidationException): + await crud.set_peer_card( + db_session, + test_workspace.name, + peer_card=["x"], + observer=test_peer.name, + observed=scope_peer_name(str(generate_nanoid())), + ) + + +async def test_dream_cannot_be_queued_for_a_future_scope( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """A dream naming a not-yet-existing reserved observed peer is refused. + + 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. + """ + test_workspace, test_peer = sample_data + future = scope_peer_name(str(generate_nanoid())) + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/schedule_dream", + json={"observer": test_peer.name, "observed": future, "dream_type": "omni"}, + ) + assert response.status_code == 422, response.text + + # No queue row was inserted for it + items = ( + await db_session.execute( + select(QueueItem).where(QueueItem.work_unit_key.contains(future)) + ) + ).all() + assert not items + + +async def test_enqueue_dream_guard_covers_internal_callers( + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """Direct enqueue_dream calls enforce the same invariant as the route.""" + from src.deriver.enqueue import enqueue_dream + from src.schemas.configuration import DreamType + + test_workspace, test_peer = sample_data + scope_name = str(generate_nanoid()) + await db_session.commit() + + with pytest.raises(ValidationException): + await enqueue_dream( + test_workspace.name, + observer=test_peer.name, + observed=scope_peer_name(scope_name), + dream_type=DreamType.OMNI, + ) + + +def test_prefixed_nul_name_is_422_not_500( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """A reserved-prefix name containing NUL must not reach a text comparison. + + It passes the request schemas and PeerSpec, so without pre-SQL rejection it + reaches psycopg inside the scope lookup and raises DataError — a 500. + """ + test_workspace, _ = sample_data + 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.future\x00name", "content": "hi"}]}, + ) + 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