From 81d8409b8c1cfcf625490d09b5d335fd2c10af0f Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:21:40 -0400 Subject: [PATCH] Scopes Phase 2a: scope-kind peers, guardrails, and scopes CRUD routes (#884) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: reserve scope__ peer namespace with kind flag and guardrails Introduce the scope peer namespace (scope__) and the authoritative {"kind": "scope"} configuration flag, plus the server-side guardrails: - src/utils/scopes.py: single source of truth for the prefix, kind flag, and name helpers (scope_peer_name / is_scope_peer_name / scope_name_from_peer / validate_no_scope_peer_names) - reject reserved-prefix names on peer get-or-create (422) - reject scope peers as message authors in crud.create_messages (422) - reject scope peers as chat/representation targets (422); a scope peer as the path-level observer is deferred to Phase 2b - reject scope peers on the generic session-peer add/set/remove routes and the session-create peers mapping (422, directing to scopes routes) - peers.list excludes scope peers by default; new PeerGet.kind option ("scope" | "all") switches the view via a configuration JSONB filter - schemas: Scope / ScopeCreate / ScopeSessions(Add) and SessionCreate.scopes (unprefixed scope names, validated) Part of DEV-1997 (Scopes RFC DEV-1970). Co-Authored-By: Claude Fable 5 * feat: add scopes CRUD routes and session-create scopes wiring New /v3/workspaces/{workspace_id}/scopes facade (workspace-level auth; peer- and session-scoped keys are rejected): POST "" create-or-get (201/200) POST /list paginated scope list GET /{scope_id} single scope POST /{scope_id}/sessions add memberships DELETE /{scope_id}/sessions/{session_id} remove membership GET /{scope_id}/sessions list member session ids - crud/scope.py: get_or_create_scopes stamps the backing peer with {"kind": "scope", "observe_me": false} and refuses to adopt a legacy peer occupying the reserved name without the flag (409) - memberships are session_peers rows with observe_others=true / observe_me=false — identical to a hand-built observer peer - SessionCreate.scopes: create-or-get each scope peer and add the membership at session creation (the no-backfill common path) - crud/session.py: public upsert_session_peers wrapper so the facade bypasses the route-level guardrails without reaching into privates Backfill of pre-existing documents and reconciliation on removal land in DEV-1999; membership only affects messages ingested after the change. Part of DEV-1997 (Scopes RFC DEV-1970). Co-Authored-By: Claude Fable 5 * test: cover scopes facade, guardrails, and observer semantics - create-or-get idempotency, list/get, name validation, legacy-collision rejection (409), auth scoping (workspace key ok, peer/session keys 401) - reserved prefix rejected on peer create; peers.list kind filtering - scope peers rejected as message authors, chat/representation targets, and on the generic session-peer routes - membership add/list/remove with observe_others=true / observe_me=false row shape asserted via DB, and facade-less equivalence with a hand-built observer peer - end-to-end litmus: after adding a session to a scope, the deriver enqueue fan-out includes the scope peer as an observer - session creation with scopes: [a, b] creates both memberships Part of DEV-1997 (Scopes RFC DEV-1970). Co-Authored-By: Claude Fable 5 * fix(crud): preserve cache invalidation across get_or_create retry `get_or_create_peers` and `get_or_create_scopes` mutate existing rows, then insert new ones inside `db.begin_nested()`. When a concurrent writer creates one of those rows first, the insert raises IntegrityError and the function retries. `begin_nested()` autoflushes the pending UPDATEs *before* opening the savepoint, so the rollback neither undoes them nor expires the now-clean ORM state. The retry then compared already-updated values, found no change, and dropped those peers from `changed_peers` — skipping the cache purge while the row change committed anyway, leaving entries stale until the 300s TTL. Carry the mutated names into the retry via `_pending_invalidation` so the purge cannot be lost. The scopes facade mirrors `get_or_create_peers`, so both copies carried this. The peer path is pre-existing and runs on every message ingest. Also add the missing /v3 prefix to `_SCOPES_ROUTE_GUIDANCE`, which pointed callers at a 404. Adds tests/crud/test_get_or_create_retry_invalidation.py, which drives a real racing session and fails without this change. * fix(scopes): make scope identity unforgeable, unblock non-pattern peer names Three coupled changes to the scopes facade. 1. `PeerCreate` no longer gates internal lookups. It exists to validate a new, user-supplied peer id at the API boundary, but crud used it as a DTO for names that already exist, so any name outside RESOURCE_NAME_PATTERN raised a raw pydantic ValidationError — which is not a HonchoException, so it fell through to the catch-all handler as an HTTP 500. Adds `PeerSpec` (same fields, no charset pattern) as `PeerCreate`'s base, widens `get_or_create_peers` to accept it, and changes `get_peer` to take a plain str. All 13 construction sites converted; the create route keeps full validation. This unbreaks the Dreamer: DreamScheduler passes `collection.observer` straight into the specialist preflight, and scope peers have `observe_others=true`, so every `(scope.x, peer)` dream died there — the feature scopes exist to enable. It also fixes a pre-existing bug unrelated to scopes: a peer named `alice.smith` (legal before d429de0e5338, which validated names by length alone) 500s on message create, session peer add, and peer update. 2. The `kind` flag moves from `configuration` to `internal_metadata`. `configuration` is user-writable — `PeerCreate`/`PeerUpdate` accept a free-form dict and `update_peer` replaces it wholesale — so a legitimate `{"observe_me": true}` update silently dropped the flag, and a forged `{"kind": "scope"}` injected an ordinary peer into `POST /scopes/list`. `internal_metadata` appears in no API schema. `observe_me: false` stays in `configuration`, where it belongs. 3. Scope identity requires prefix AND flag, via `is_scope_peer()` and `scope_peer_clause()`. Neither half is forgeable: the prefix sits outside RESOURCE_NAME_PATTERN, `internal_metadata` is unreachable. Usage-site guards become flag-based so a legacy peer merely occupying the namespace keeps working rather than 422-ing on its own traffic; peer create and update stay name-based, since those must stop new names entering the namespace. `update_peer` now returns 422 instead of 500. Also swaps the reserved prefix from `scope__` to `scope.`: `_` is inside RESOURCE_NAME_PATTERN, so any tenant could already own a `scope__x` peer. No DB migration — `internal_metadata` already exists on `peers`, and no scope peers exist in any deployment yet. Co-Authored-By: Claude Opus 5 (1M context) * fix(scopes): validate peer names on create, close namespace squatting and upsert race Addresses three review findings against 48047a6a. 1. `PeerSpec` let API callers create invalid and reserved-prefix peers. Widening `get_or_create_peers` to accept a pattern-free schema fixed the lookup 500s but also removed validation from the *insert* path, and request-controlled names reach it via message authors, session peer maps, and the chat observer path — none of which carry a charset pattern of their own. Confirmed: `POST /sessions/{id}/messages` with `peer_id: "scope.x"` returned 201 and minted an unflagged squatter, after which `POST /scopes {id: x}` was permanently 409-blocked — namespace denial of service by any caller able to post a message. `peer_id: "not a valid name!@#"` was likewise created. Fixed by validating only names about to be INSERTed (`_validate_new_peer_names`), so already-existing names — legacy dotted names, scope peers — still resolve without a spurious 422. That keeps the Dreamer fix intact, since it reads through `get_peer`. 2. Existing reserved-prefix squatters could not be updated. The name-based guard on `PUT /peers/{peer_id}` refused every `scope.` name, contradicting the invariant that an unflagged squatter stays a normal peer. Now flag-based, so behavior is three-way: a real scope is refused, an existing unflagged peer updates, and a missing reserved-prefix name is refused by (1) rather than minted. 3. Scope checks raced with get-or-create and the membership upsert. The route-level guards run before peers are resolved, so a scope created concurrently in that window would be attached by the generic path with a default `SessionPeerConfig()`, clobbering its observer membership config. Adds `_reject_resolved_scope_peers`, which runs on the resolved rows in the same transaction as the upsert — no window, no extra query. The early checks stay for better error messages. Co-Authored-By: Claude Opus 5 (1M context) * fix(scopes): guard scope membership config, move checks to the mutation point Addresses a second review pass against 10655792. 1. Scope membership configuration was directly user-mutable. `PUT /sessions/{id}/peers/{peer_id}/config` had no scope guard at all, and `crud.set_peer_config` resolved the peer only to discard the row. Confirmed: posting `{"observe_others": false, "observe_me": true}` for a scope returned 204 and persisted, which silently stops all fan-out into the scope and makes Honcho form a representation *of* a scope — neither of which is reachable through the facade. Deterministic, no race required. Now checked on the row `get_peer` already returns, so it costs nothing and cannot race. 2. Empty and over-long names were still 500s. Removing the charset pattern from `PeerSpec` fixed one trap but left its length bounds, and request-bound peer names carry no length limits of their own — so `peer_id: ""` or a 513-char name reached `PeerSpec(...)` and raised a raw pydantic ValidationError that the catch-all turned into a 500. `PeerSpec` now carries no constraints at all (matching its documented purpose) and every rule for a new name lives in `_validate_new_peer_names` on the insert path. 3. Resolved-row protection generalized. The previous pass applied it only to membership upserts, leaving check-then-use windows elsewhere: peer update could have a concurrently-created scope's configuration replaced wholesale (create-path validation does not fire for a peer that now exists), the chat observer get-or-create could resolve a fresh scope as its observer, and the generic session-peer removal could silently detach a scope from its sessions. Each now inspects the resolved peer immediately before acting; the redundant name-level guard on the update route is dropped in favor of the race-free one. `remove_peers_from_session` grows an internal `_allow_scope_peers` flag because the scopes facade ends membership through that same path and must not be blocked by its own guard. Co-Authored-By: Claude Opus 5 (1M context) * test(scopes): enumerate every peer-touching route against a scope policy Three review passes each found the same class of defect: a route nobody had checked, rather than 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 this work entirely, because the guardrail set was assembled guardrail-by-guardrail instead of derived from the route list. Sampling review cannot close that kind of gap; enumeration can. Derives every route through which a peer name can reach the system and requires each to be classified as GUARDED or EXEMPT-with-a-reason, so a newly added peer-touching route fails the suite until someone classifies it. Detection is the union of path shape and a walk of the dependant tree (including sub-dependency `Form(...)` params and nested request-body models), because neither signal alone suffices: parameter names miss `POST /sessions/{id}/peers`, whose peer names are dict keys, and path shape misses `messages/upload`, whose `peer_id` arrives as a form field behind a parser dependency. Both invariants are then asserted behaviorally, by calling the routes rather than inspecting annotations — the guards deliberately live in crud, which is what makes `messages/upload` guarded for free via `crud.create_messages`: - a real scope is refused on all 11 guarded routes, and the rejection must name the scope, so an unrelated 422 (a malformed body) cannot pass the assertion; - an *unflagged* peer merely occupying the reserved namespace is unaffected. That half regressed once already when `update_peer` used a name-based check. Mutation-tested all three failure modes: disabling the `set_peer_config` guard fails the guarded test naming that route; regressing `update_peer` to name-based fails the squatter test; adding an unclassified peer route fails the enumeration. Covers the HTTP surface only. Peer names also reach the system through the deriver, dreamer, and queue, which have no route table to enumerate — noted in the module docstring rather than implied to be covered. Co-Authored-By: Claude Opus 5 (1M context) * fix(scopes): refuse a scope in every observed position; enumerate per position Addresses a fourth review pass against 62681e4d. The headline finding is that the previous commit's enumeration test had the wrong *model*, not a missing entry. 1. Manual conclusions could create knowledge about a scope. `POST /conclusions` validated only that observer_id and observed_id exist, so a scope as `observed_id` persisted a conclusion about a peer carrying observe_me=false and created an (observer, scope) collection for it. Confirmed: 201, and it read back. `POST /schedule_dream` had the same hole via `observed`. The fix is positional, because the invariant is: A scope may be an OBSERVER. A scope may never be OBSERVED. A scope as `observer_id` is how scoped conclusions are stored and must keep working (verified still 201); as `observed_id` it is now refused. Same split applied to schedule_dream `observed`, the peer-card `target` (which also covers a scope's self-card, since target-omitted collapses observed to peer_id), and session-context `peer_target`. 2. Chat target and both representation roles kept check-to-use races. Only the chat path-level observer was re-checked on its resolved row; the target was checked by name and then resolved without inspecting scope identity. Both are now checked at the dialectic preflight, where observer and observed are already resolved — an absent name has already failed by then, and an existing squatter cannot retroactively become a scope. 3. Generic membership removal was still racy. The adjacent SELECT narrowed the window but could not close it under READ COMMITTED. The UPDATE now carries its own correlated NOT EXISTS against scope_peer_clause(), so Postgres evaluates the exclusion as part of the statement and a scope committed after the advisory check still cannot be detached. 4. New-name validation ran after the name reached Postgres. A NUL byte passed the request schemas and PeerSpec, then raised psycopg.DataError inside the lookup — a 500. Values that cannot correspond to a stored row by construction (NUL bytes, over-length names) are now refused before the query. (Over-length names already returned 422; only the wasted query was real there.) The enumeration test is rekeyed from (method, path) to (method, path, position). A binary per-route verdict cannot express finding 1 at all: `POST /conclusions` is one route with two positions and opposite verdicts. Detection widens to observer / observed / target / peer_target / peer_perspective, which surfaced four routes the previous version never saw — conclusions, schedule_dream, queue/status, and session context. Also registers `src.routers.workspaces.tracked_db` in the conftest patch list; the new guard there would otherwise have run against the real configured database instead of the per-test one. Mutation-tested: disabling the conclusions observed-guard fails the positional test naming that position; adding an unclassified `observed_id` param fails enumeration. Co-Authored-By: Claude Opus 5 (1M context) * fix(scopes): refuse future scopes in observed positions, preserve scope membership 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) * fix(scopes): exclude scope memberships from the session observer limit Scope memberships carry observe_others=true, so every scope counted against SESSION_OBSERVERS_LIMIT (default 10) — capping scopes-per-session at the limit minus the session's real observers, and reporting the failure as `400 Cannot create session with 11 observers. ... Observers are peers with 'observe_others' set to true.` on a membership call. Wrong on three counts: the ceiling is undocumented and contradicts RFC §5.1 ("sessions belong to any number of scopes"), the message describes session creation, and it leaks the word "observer" through a facade whose entire job is hiding observers (RFC goal 5). The limit exists to bound per-observer deriver fan-out for real peers; a scope costs document rows, not LLM calls (RFC §5.2), so it does not belong in that budget. Excluded from both halves of the check in `_get_or_add_peers_to_session`: the incoming names via a flag-based lookup, existing memberships via a correlated NOT EXISTS on `scope_peer_clause()` — the same pattern the replacement and removal paths already use, so the exclusion holds regardless of concurrent scope creation. The early `count_observers_in_config(session.peer_names)` check in `get_or_create_session` is left alone: `peer_names` cannot contain a scope, and `scopes` is a separate field. `reject_scope_peers` is split into a `scope_peer_names()` query helper plus a two-line raiser so the observer count reuses the authoritative name-AND-flag predicate instead of growing a third copy of it. Still costs nothing on the common path — no reserved-prefix name in the input means no query at all. Also caps `SessionCreate.scopes` at 100, matching `ScopeSessionsAdd.session_ids`. This belongs in the same commit: the observer limit was the only thing bounding that list, so removing it turns an unbounded `scopes` array into a peer row and a membership row per element, committed — the single-request path to the cardinality anti-pattern RFC §8 warns about. Partly answers OQ6: no per-session cap, 100 per request. Tests: a session joins SESSION_OBSERVERS_LIMIT + 2 scopes through both the facade and session creation; real observers over the limit still 400, so the carve-out cannot quietly disable the limit; 101 scopes is a 422. Co-Authored-By: Claude Opus 5 (1M context) * fix(scopes): skip semantic retrieval when the embedding precompute failed Addresses three open review comments. 1. Major — the representation read could embed inside its DB session. The route's precompute is suppressed, and both `RepresentationManager.get_working_representation` and `crud.query_documents` fall back to embedding when a query arrives without one, so a failed precompute meant an external call inside the read session this branch opens for the scope re-check — the connection-holding rule the route's own comment claimed to satisfy. The innermost fallback also only catches ValueError, so a provider outage surfaced as a 500. The semantic query is now passed only when an embedding exists, degrading to derived+recent retrieval. (`crud.query_documents` embedding inside a caller's session predates this branch and is left alone.) 2. Minor — `test_resolved_scope_peer_rejected_at_membership_upsert` described a race it does not perform. It creates an already-flagged scope and calls crud directly; the unflagged → flagged transition is not simulated. Docstring now says what the test actually pins. 3. Minor — `test_empty_replacement_preserves_scope_membership` asserted only half its docstring. It passed if the empty PUT left every ordinary membership intact; now asserts the ordinary peer's left_at is set. Co-Authored-By: Claude Opus 5 (1M context) * 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 * test(scopes): assert exact statuses for permissive missing-name cases Follow-up review pass on #884. - add `Case.missing_status` so a permissive missing-name position asserts the status it should actually get (404, or 200 for the no-op removal) instead of `!= 422`, which also passed on a 5xx — the same hole already closed in the squatter assertion - require it whenever `refuse_missing` is False, and require its absence when True, so the policy table can't drift from the assertion - repoint a stale allow-reason at POST /scopes/{scope_id}/sessions/list; the GET it named was removed - document the membership list's ordering under `reverse` --------- Co-authored-by: Claude Fable 5 --- src/crud/__init__.py | 19 + src/crud/document.py | 22 +- src/crud/message.py | 16 + src/crud/peer.py | 302 ++++- src/crud/peer_card.py | 26 +- src/crud/scope.py | 370 +++++ src/crud/session.py | 203 ++- src/deriver/enqueue.py | 16 + src/dialectic/chat.py | 51 +- src/dreamer/specialists.py | 10 +- src/main.py | 2 + src/routers/peers.py | 185 ++- src/routers/scopes.py | 173 +++ src/routers/sessions.py | 63 +- src/routers/workspaces.py | 4 + src/schemas/__init__.py | 8 + src/schemas/api.py | 141 +- src/telemetry/events/trace.py | 4 +- src/utils/scopes.py | 89 ++ .../test_get_or_create_retry_invalidation.py | 197 +++ tests/dreamer/test_card_refresh.py | 2 +- tests/routes/test_scope_route_policy.py | 941 +++++++++++++ tests/routes/test_scopes.py | 1201 +++++++++++++++++ tests/test_cache_redaction.py | 4 +- 24 files changed, 3972 insertions(+), 77 deletions(-) create mode 100644 src/crud/scope.py create mode 100644 src/routers/scopes.py create mode 100644 src/utils/scopes.py create mode 100644 tests/crud/test_get_or_create_retry_invalidation.py create mode 100644 tests/routes/test_scope_route_policy.py create mode 100644 tests/routes/test_scopes.py diff --git a/src/crud/__init__.py b/src/crud/__init__.py index b34e4d17..c7767f47 100644 --- a/src/crud/__init__.py +++ b/src/crud/__init__.py @@ -39,12 +39,22 @@ from .peer import ( get_peer, get_peers, get_sessions_for_peer, + reject_scope_observed, + reject_scope_peers, update_peer, ) from .peer_card import get_peer_card, set_peer_card from .representation import ( get_working_representation, ) +from .scope import ( + add_sessions_to_scope, + get_or_create_scopes, + get_scope_or_raise, + get_scope_sessions, + get_scopes, + remove_session_from_scope, +) from .session import ( SessionDeletionResult, clone_session, @@ -114,6 +124,8 @@ __all__ = [ # Peer "get_or_create_peers", "get_peer", + "reject_scope_observed", + "reject_scope_peers", "get_peers", "update_peer", "get_sessions_for_peer", @@ -122,6 +134,13 @@ __all__ = [ "set_peer_card", # Representation "get_working_representation", + # Scope + "add_sessions_to_scope", + "get_or_create_scopes", + "get_scope_or_raise", + "get_scope_sessions", + "get_scopes", + "remove_session_from_scope", # Session "SessionDeletionResult", "get_sessions", diff --git a/src/crud/document.py b/src/crud/document.py index 36d9048d..ba80712d 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_observed from src.crud.session import get_session from src.dependencies import tracked_db from src.embedding_client import embedding_client @@ -955,7 +955,25 @@ async def create_observations( # Validate all peers exist for peer_name in peers_to_validate: - await get_peer(db, workspace_name, schemas.PeerCreate(name=peer_name)) + 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. + # + # 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}, + action="No conclusion is ever formed about a scope.", + ) # Get or create all collections for observer, observed in collection_pairs: diff --git a/src/crud/message.py b/src/crud/message.py index 684ddb4d..fbfb1698 100644 --- a/src/crud/message.py +++ b/src/crud/message.py @@ -17,6 +17,7 @@ from src.utils.formatting import ILIKE_ESCAPE_CHAR, escape_ilike_pattern from src.utils.types import embedding_call_purpose from src.vector_store import get_external_vector_store +from .peer import reject_scope_peers from .session import get_or_create_session logger = getLogger(__name__) @@ -312,7 +313,22 @@ async def create_messages( Returns: List of created message objects + + Raises: + ValidationException: If a message is authored by a scope peer """ + # Scope peers are silent observers — they can never author messages. Keyed + # off name+flag so a legacy peer merely occupying the reserved namespace + # keeps ingesting. Must stay *before* get_or_create_session below: that call + # would create the scope peer and add it with a default SessionPeerConfig(), + # clobbering its observe_others=True/observe_me=False membership config. + await reject_scope_peers( + db, + workspace_name, + (message.peer_name for message in messages), + action="Scope peers cannot author messages.", + ) + # Get or create session with peers in messages list peers = {message.peer_name: schemas.SessionPeerConfig() for message in messages} await get_or_create_session( diff --git a/src/crud/peer.py b/src/crud/peer.py index f7936761..2f81f938 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -1,10 +1,12 @@ """CRUD helpers for peer records and peer-scoped session queries.""" +import re +from collections.abc import Collection, Iterable from logging import getLogger -from typing import Any +from typing import Any, Literal from cashews import NOT_NONE -from sqlalchemy import Select, select +from sqlalchemy import ColumnElement, Select, and_, select from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import make_transient_to_detached @@ -13,13 +15,22 @@ from src import models, schemas from src.cache.client import cache, get_cache_namespace, safe_cache_delete from src.config import settings from src.crud.workspace import get_or_create_workspace -from src.exceptions import ConflictException, ResourceNotFoundException +from src.exceptions import ( + ConflictException, + ResourceNotFoundException, + ValidationException, +) from src.models import Peer +from src.schemas.api import RESOURCE_NAME_PATTERN +from src.utils import scopes as scopes_util from src.utils.filter import apply_filter from src.utils.types import GetOrCreateResult logger = getLogger(__name__) +# Matches the peers.name CHECK constraint and PeerCreate's max_length. +PEER_NAME_MAX_LENGTH = 512 + PEER_CACHE_KEY_TEMPLATE = "v2:workspace:{workspace_name}:peer:{peer_name}" PEER_LOCK_PREFIX = f"{get_cache_namespace()}:lock:v2" @@ -36,12 +47,214 @@ def peer_cache_key(workspace_name: str, peer_name: str) -> str: ) +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 + 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. + + 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. + """ + 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) {too_long} must be at most " + + f"{PEER_NAME_MAX_LENGTH} characters" + ) + + +def _validate_new_peer_names(names: list[str]) -> None: + """Validate peer names that are about to be created. + + Mirrors ``PeerCreate``'s contract for peers arriving through crud rather than + the peers route. The reserved prefix is reported separately because it is + also outside ``RESOURCE_NAME_PATTERN``, so the charset check would otherwise + mask the real problem. + + Raises: + ValidationException: On a reserved-prefix or non-conforming name. + """ + scopes_util.validate_no_scope_peer_names( + names, action="Use the scopes routes to create scopes." + ) + # 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( + f"Peer name(s) {offenders} must match pattern {RESOURCE_NAME_PATTERN}" + ) + + +def scope_peer_clause() -> ColumnElement[bool]: + """SQL form of ``is_scope_peer()``: reserved name prefix AND the internal kind flag. + + Lives here rather than in ``crud/scope.py`` because that module already imports + from this one, and ``get_peers`` below needs the clause — the other direction + would be a cycle. + + ``autoescape=True`` is future-proofing: '.' is not a LIKE wildcard, but '_' is, + so under a ``scope__``-style prefix an unescaped ``startswith`` would also match + ``scopeXY...``. Both columns are NOT NULL with defaults, so the negation + ``~scope_peer_clause()`` has no NULL-semantics trap. + """ + return and_( + models.Peer.name.startswith(scopes_util.SCOPE_PEER_PREFIX, autoescape=True), + models.Peer.internal_metadata.contains({"kind": scopes_util.SCOPE_KIND}), + ) + + +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 = _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 scope_peer_names( + db: AsyncSession, + workspace_name: str, + names: Iterable[str], +) -> set[str]: + """Return the subset of ``names`` that are really scope peers (name AND flag). + + Unlike a pure name check, a legacy peer that merely occupies the reserved + namespace (names were length-only validated before migration + ``d429de0e5338``, so ``scope.production`` is a possible user name) is not + reported, so it keeps its ordinary semantics instead of being locked out of + its own data. A *missing* reserved name is likewise not reported. + + Costs nothing on the common path: with no reserved-prefix name in ``names`` + there is no query at all. + + Raises: + ValidationException: On a NUL byte or an over-length name. + """ + candidates = _reserved_name_candidates(names) + if not candidates: + return set() + + result = await db.execute( + select(models.Peer.name) + .where(models.Peer.workspace_name == workspace_name) + .where(models.Peer.name.in_(candidates)) + .where(scope_peer_clause()) + ) + return {row[0] for row in result.all()} + + +async def reject_scope_peers( + db: AsyncSession, + workspace_name: str, + names: Iterable[str], + *, + action: str, +) -> None: + """Reject peers that really are scopes, keyed off name AND flag. + + 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. See ``scope_peer_names`` for + the name-vs-flag semantics. + + Raises: + ValidationException: If any name resolves to a real scope peer. + """ + offenders = sorted(await scope_peer_names(db, workspace_name, names)) + if offenders: + raise ValidationException(f"Peer name(s) {offenders} are scopes. {action}") + + async def get_or_create_peers( db: AsyncSession, workspace_name: str, - peers: list[schemas.PeerCreate], + peers: list[schemas.PeerSpec], *, _retry: bool = False, + _pending_invalidation: list[str] | None = None, ) -> GetOrCreateResult[list[models.Peer]]: """ Get an existing list of peers or create new peers if they don't exist. @@ -52,16 +265,23 @@ async def get_or_create_peers( workspace_name: Name of the workspace peers: List of peer creation schemas _retry: Whether to retry the operation + _pending_invalidation: Names of peers already mutated by a prior attempt, + whose cache keys must still be purged. See the retry branch below. Returns: GetOrCreateResult containing the list of peers and whether any were created 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)) 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) @@ -104,6 +324,17 @@ async def get_or_create_peers( existing_names = {p.name for p in existing_peers} peers_to_create = [p for p in peers if p.name not in existing_names] + # Names are validated on the *create* path only. `PeerSpec` deliberately + # carries no charset pattern so already-existing names (legacy dotted names, + # scope peers) can be looked up without a spurious 422 — but a name we are + # about to INSERT is a new peer, and new peers must obey the public contract. + # Without this, request-controlled names reach here unvalidated via message + # authors, session peer maps, and the chat observer path, letting a caller + # mint `scope.x` squatters (permanently 409-blocking that scope) or peers + # that violate RESOURCE_NAME_PATTERN outright. + if peers_to_create: + _validate_new_peer_names([p.name for p in peers_to_create]) + # Create new peers new_peers = [ models.Peer( @@ -122,11 +353,26 @@ async def get_or_create_peers( raise ConflictException( f"Unable to create or get peers: {peer_names}" ) from None - return await get_or_create_peers(db, workspace_name, peers, _retry=True) + # `begin_nested()` autoflushes the mutations above *before* opening the + # savepoint, so they are already committed-in-transaction and the rollback + # doesn't undo them — nor does it expire the now-clean ORM state. The retry + # would therefore compare already-updated values, find no change, and skip + # the purge. Carry the names forward so the invalidation can't be lost. + return await get_or_create_peers( + db, + workspace_name, + peers, + _retry=True, + _pending_invalidation=(_pending_invalidation or []) + + [p.name for p in changed_peers], + ) # Capture peer names eagerly so the closure holds plain strings, not ORM objects _cache_keys_to_invalidate = [ - peer_cache_key(workspace_name, p.name) for p in changed_peers + new_peers + peer_cache_key(workspace_name, name) + for name in dict.fromkeys( + (_pending_invalidation or []) + [p.name for p in changed_peers + new_peers] + ) ] async def _invalidate_peer_cache(): @@ -181,15 +427,20 @@ async def _fetch_peer( async def get_peer( db: AsyncSession, workspace_name: str, - peer: schemas.PeerCreate, + peer_name: str, ) -> models.Peer: """ Get an existing peer. + Takes a plain name, not a create schema: this is a pure read, and validating + an already-existing name against ``PeerCreate``'s charset pattern turns a + lookup into a raw pydantic ValidationError (an HTTP 500) for legacy dotted + names and every ``scope.``-prefixed peer. + Args: db: Database session workspace_name: Name of the workspace - peer: Peer creation schema + peer_name: Name of the peer Returns: The peer if found @@ -197,10 +448,10 @@ async def get_peer( Raises: ResourceNotFoundException: If the peer does not exist """ - data = await _fetch_peer(db, workspace_name, peer.name) + data = await _fetch_peer(db, workspace_name, peer_name) if data is None: raise ResourceNotFoundException( - f"Peer {peer.name} not found in workspace {workspace_name}" + f"Peer {peer_name} not found in workspace {workspace_name}" ) # Reconstruct ORM object from cached dict and merge into session @@ -215,10 +466,26 @@ async def get_peers( workspace_name: str, filters: dict[str, Any] | None = None, reverse: bool = False, + kind: Literal["scope", "all"] | None = None, ) -> Select[tuple[models.Peer]]: - """Build a filtered peer list query ordered by creation time.""" + """Build a filtered peer list query ordered by creation time. + + Args: + workspace_name: Name of the workspace + filters: Filter peers by metadata + reverse: Whether to reverse the default creation order + kind: Which kinds of peers to include. None (default) excludes scope + peers (see ``scope_peer_clause``: reserved name prefix AND the + ``{"kind": "scope"}`` internal_metadata flag), "scope" returns only + scope peers, and "all" returns everything. + """ stmt = select(models.Peer).where(models.Peer.workspace_name == workspace_name) + if kind is None: + stmt = stmt.where(~scope_peer_clause()) + elif kind == "scope": + stmt = stmt.where(scope_peer_clause()) + stmt = apply_filter(stmt, models.Peer, filters) if reverse: @@ -250,10 +517,21 @@ async def update_peer( the peer """ peers_result = await get_or_create_peers( - db, workspace_name, [schemas.PeerCreate(name=peer_name)] + db, workspace_name, [schemas.PeerSpec(name=peer_name)] ) honcho_peer = peers_result.resource[0] + # Refuse a real scope on the row just resolved, not on the name beforehand: + # this route replaces `configuration` wholesale, and a name-level check leaves + # a window in which a concurrently-created scope is resolved as existing (so + # create-path validation never fires) and then overwritten. An existing + # *unflagged* peer in the reserved namespace is an ordinary peer and passes. + if scopes_util.is_scope_peer(honcho_peer.name, honcho_peer.internal_metadata): + raise ValidationException( + f"Peer '{peer_name}' is a scope." + + " Use the scopes routes to manage scopes." + ) + needs_update = False if peer.metadata is not None and honcho_peer.h_metadata != peer.metadata: diff --git a/src/crud/peer_card.py b/src/crud/peer_card.py index 6697130f..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__) @@ -38,7 +43,7 @@ async def get_peer_card( Raises: ResourceNotFoundException: If the peer does not exist. """ - peer = await get_peer(db, workspace_name, schemas.PeerCreate(name=observer)) + peer = await get_peer(db, workspace_name, observer) return cast( list[str] | None, peer.internal_metadata.get( @@ -68,9 +73,24 @@ 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.PeerCreate(name=observer)] + db, workspace_name, [schemas.PeerSpec(name=observer)] ) stmt = ( diff --git a/src/crud/scope.py b/src/crud/scope.py new file mode 100644 index 00000000..c04ac903 --- /dev/null +++ b/src/crud/scope.py @@ -0,0 +1,370 @@ +"""CRUD helpers for scopes. + +A scope is a named grouping of sessions, implemented as a peer named +``scope.`` carrying ``{"kind": "scope"}`` in ``internal_metadata`` (the +authoritative, user-unwritable flag) and ``{"observe_me": false}`` in +``configuration``, that observes its member sessions (``observe_others=true``) +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. Conclusions already derived are neither backfilled on add nor +reconciled on removal. +""" + +from logging import getLogger + +from sqlalchemy import Select, select +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from src import models, schemas +from src.cache.client import safe_cache_delete +from src.exceptions import ConflictException, ResourceNotFoundException +from src.utils.scopes import ( + SCOPE_KIND, + is_scope_peer, + scope_peer_name, +) +from src.utils.types import GetOrCreateResult + +from .peer import peer_cache_key, scope_peer_clause +from .workspace import get_or_create_workspace + +logger = getLogger(__name__) + +# Internal metadata stamped on every scope peer at creation. `kind` is the +# authoritative scope flag and lives here — NOT in `configuration` — because +# `configuration` is user-writable (`PeerCreate`/`PeerUpdate` accept a free-form +# dict, and `update_peer` replaces it wholesale), so a user could forge or clear +# the flag. `internal_metadata` appears in no API schema at all. +SCOPE_PEER_INTERNAL_METADATA: dict[str, str] = { + "kind": SCOPE_KIND, +} + +# Peer-level configuration stamped on every scope peer at creation. +# `observe_me: false` ensures no representation is ever formed *of* a scope peer. +# This one stays user-visible: `observe_me` is a legitimate config knob. +SCOPE_PEER_CONFIGURATION: dict[str, str | bool] = { + "observe_me": False, +} + +# Session-level configuration for a scope peer's membership in a session. +SCOPE_MEMBERSHIP_CONFIG = schemas.SessionPeerConfig( + observe_others=True, observe_me=False +) + + +async def get_or_create_scopes( + db: AsyncSession, + workspace_name: str, + scopes: list[schemas.ScopeCreate], + *, + _retry: bool = False, + _pending_invalidation: list[str] | None = None, +) -> GetOrCreateResult[list[models.Peer]]: + """ + Get existing scopes or create new ones if they don't exist. + + Existing scope peers have their metadata updated when provided. A + pre-existing peer that occupies a scope's reserved name *without* the + authoritative ``kind`` flag (a legacy collision) is never adopted. + + 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 + scopes: List of scope creation schemas (unprefixed names) + _retry: Whether this is the retry attempt + _pending_invalidation: Names of scope peers already mutated by a prior + attempt, whose cache keys must still be purged. See the retry branch. + + Returns: + GetOrCreateResult containing the backing peers and whether any were + created + + Raises: + ConflictException: If a peer already occupies a scope's reserved name + without the scope kind flag, or if we fail to get or create the + scope peers + """ + await get_or_create_workspace(db, schemas.WorkspaceCreate(name=workspace_name)) + + peer_names = {scope_peer_name(s.name): s for s in scopes} + stmt = ( + select(models.Peer) + .where(models.Peer.workspace_name == workspace_name) + .where(models.Peer.name.in_(peer_names.keys())) + ) + result = await db.execute(stmt) + existing_peers: list[models.Peer] = list(result.scalars().all()) + + changed_peers: list[models.Peer] = [] + for existing_peer in existing_peers: + if not is_scope_peer(existing_peer.name, existing_peer.internal_metadata): + raise ConflictException( + f"A peer named '{existing_peer.name}' already exists in workspace " + + f"{workspace_name} but is not a scope. Rename or delete that " + + "peer before creating this scope." + ) + scope_schema = peer_names[existing_peer.name] + if ( + scope_schema.metadata is not None + and existing_peer.h_metadata != scope_schema.metadata + ): + existing_peer.h_metadata = scope_schema.metadata + changed_peers.append(existing_peer) + + existing_names = {p.name for p in existing_peers} + new_peers = [ + models.Peer( + workspace_name=workspace_name, + name=name, + h_metadata=scope_schema.metadata or {}, + internal_metadata=dict(SCOPE_PEER_INTERNAL_METADATA), + configuration=dict(SCOPE_PEER_CONFIGURATION), + ) + for name, scope_schema in peer_names.items() + if name not in existing_names + ] + try: + async with db.begin_nested(): + db.add_all(new_peers) + except IntegrityError: + if _retry: + raise ConflictException( + f"Unable to create or get scopes: {sorted(peer_names)}" + ) from None + # `begin_nested()` autoflushes the mutations above *before* opening the + # savepoint, so they survive the rollback and leave the ORM state clean — + # the retry would compare already-updated values, find no change, and skip + # the purge. Carry the names forward so the invalidation can't be lost. + return await get_or_create_scopes( + db, + workspace_name, + scopes, + _retry=True, + _pending_invalidation=(_pending_invalidation or []) + + [p.name for p in changed_peers], + ) + + _cache_keys_to_invalidate = [ + peer_cache_key(workspace_name, name) + for name in dict.fromkeys( + (_pending_invalidation or []) + [p.name for p in changed_peers + new_peers] + ) + ] + + async def _invalidate_peer_cache(): + for cache_key in _cache_keys_to_invalidate: + await safe_cache_delete(cache_key) + + return GetOrCreateResult( + existing_peers + new_peers, + created=len(new_peers) > 0, + on_commit=_invalidate_peer_cache if _cache_keys_to_invalidate else None, + ) + + +async def get_scopes( + workspace_name: str, + reverse: bool = False, +) -> Select[tuple[models.Peer]]: + """Build a scope list query, ordered by creation time. + + Requires both halves via ``scope_peer_clause`` (reserved name prefix AND the + internal kind flag), so a peer carrying a forged ``configuration`` cannot + inject itself into the scope list. + """ + stmt = ( + select(models.Peer) + .where(models.Peer.workspace_name == workspace_name) + .where(scope_peer_clause()) + ) + if reverse: + return stmt.order_by(models.Peer.created_at.desc(), models.Peer.id.desc()) + return stmt.order_by(models.Peer.created_at.asc(), models.Peer.id.asc()) + + +async def get_scope_or_raise( + db: AsyncSession, + workspace_name: str, + scope_name: str, +) -> models.Peer: + """ + Get an existing scope's backing peer by its unprefixed scope name. + + Args: + db: Database session + workspace_name: Name of the workspace + scope_name: Unprefixed scope name + + Returns: + The backing peer if found and flagged as a scope + + Raises: + ResourceNotFoundException: If no scope with that name exists (a peer + occupying the reserved name without the kind flag does not count) + """ + peer = await db.scalar( + select(models.Peer) + .where(models.Peer.workspace_name == workspace_name) + .where(models.Peer.name == scope_peer_name(scope_name)) + ) + if peer is None or not is_scope_peer(peer.name, peer.internal_metadata): + raise ResourceNotFoundException( + f"Scope {scope_name} not found in workspace {workspace_name}" + ) + return peer + + +async def get_scope_sessions( + workspace_name: str, + scope_name: str, + reverse: bool = False, +) -> Select[tuple[models.Session]]: + """ + 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: + workspace_name: Name of the workspace + scope_name: Unprefixed scope name + reverse: Whether to return newest memberships first + + Returns: + Select for the scope's member sessions + """ + stmt = ( + select(models.Session) + .join( + models.SessionPeer, + (models.Session.name == models.SessionPeer.session_name) + & (models.Session.workspace_name == models.SessionPeer.workspace_name), + ) + .where(models.SessionPeer.workspace_name == workspace_name) + .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 + ) + 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( + db: AsyncSession, + workspace_name: str, + scope_name: str, + session_names: 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. 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 + workspace_name: Name of the workspace + scope_name: Unprefixed scope name + session_names: Names of existing sessions to add + + Raises: + ResourceNotFoundException: If the scope or any named session does not + exist + """ + # Imported lazily: crud.session imports this module for the session-create + # `scopes` path, so a module-level import would be circular. + from .session import upsert_session_peers + + await get_scope_or_raise(db, workspace_name, scope_name) + + requested = set(session_names) + result = await db.execute( + select(models.Session.name) + .where(models.Session.workspace_name == workspace_name) + .where(models.Session.name.in_(requested)) + .where(models.Session.is_active == True) # noqa: E712 + ) + found = {row[0] for row in result.all()} + missing = sorted(requested - found) + if missing: + raise ResourceNotFoundException( + f"Session(s) {missing} not found in workspace {workspace_name}" + ) + + for session_name in sorted(requested): + await upsert_session_peers( + db, + workspace_name=workspace_name, + session_name=session_name, + peer_names={scope_peer_name(scope_name): SCOPE_MEMBERSHIP_CONFIG}, + fetch_after_upsert=False, + ) + + await db.commit() + + +async def remove_session_from_scope( + db: AsyncSession, + workspace_name: str, + scope_name: str, + session_name: str, +) -> None: + """ + 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``). Conclusions derived while the session was a member are left in + place — nothing reconciles them. + + Args: + db: Database session + workspace_name: Name of the workspace + scope_name: Unprefixed scope name + session_name: Name of the session to remove + + Raises: + ResourceNotFoundException: If the scope or session does not exist + """ + # Lazy import for the same circular-import reason as add_sessions_to_scope. + from .session import remove_peers_from_session + + await get_scope_or_raise(db, workspace_name, scope_name) + + await remove_peers_from_session( + db, + workspace_name=workspace_name, + session_name=session_name, + peer_names={scope_peer_name(scope_name)}, + # This *is* the supported path for ending scope membership. + _allow_scope_peers=True, + ) diff --git a/src/crud/session.py b/src/crud/session.py index 4310cb3a..1fc884ec 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 @@ -27,12 +38,21 @@ from src.exceptions import ( ConflictException, ObserverException, ResourceNotFoundException, + ValidationException, ) from src.utils.filter import apply_filter +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 +from .peer import ( + get_or_create_peers, + get_peer, + reject_scope_peers, + scope_peer_clause, + scope_peer_names, +) +from .scope import SCOPE_MEMBERSHIP_CONFIG, get_or_create_scopes from .workspace import get_or_create_workspace logger = getLogger(__name__) @@ -99,6 +119,30 @@ async def _fetch_session( } +def _reject_resolved_scope_peers(peers: list[models.Peer]) -> None: + """Reject scope peers among rows already resolved for a membership upsert. + + The route-level guards check names *before* peers are resolved, which leaves a + check-then-upsert window: if a scope is created concurrently between that + check and the upsert below, the generic path would attach the now-flagged + scope peer with a default ``SessionPeerConfig()``, clobbering its + ``observe_others=True/observe_me=False`` membership config. This runs on the + resolved rows inside the same transaction as the upsert, so there is no + window and no extra query. + + Raises: + ValidationException: If any resolved peer 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." + + " Scope membership is managed via the scopes routes." + ) + + def count_observers_in_config( peer_configs: dict[str, schemas.SessionPeerConfig], ) -> int: @@ -262,14 +306,39 @@ async def get_or_create_session( db, workspace_name=workspace_name, peers=[ - schemas.PeerCreate(name=peer_name) for peer_name in session.peer_names + schemas.PeerSpec(name=peer_name) for peer_name in session.peer_names + ], + ) + _reject_resolved_scope_peers(peers_result.resource) + await _get_or_add_peers_to_session( + db, + workspace_name=workspace_name, + session_name=session.name, + peer_names=session.peer_names, + fetch_after_upsert=False, + ) + + # 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. + scopes_result = None + if session.scopes: + scopes_result = await get_or_create_scopes( + db, + workspace_name=workspace_name, + scopes=[ + schemas.ScopeCreate(name=scope_name) for scope_name in session.scopes ], ) await _get_or_add_peers_to_session( db, workspace_name=workspace_name, session_name=session.name, - peer_names=session.peer_names, + peer_names={ + scope_peer_name(scope_name): SCOPE_MEMBERSHIP_CONFIG + for scope_name in session.scopes + }, fetch_after_upsert=False, ) @@ -280,6 +349,8 @@ async def get_or_create_session( await ws_result.post_commit() if peers_result is not None: await peers_result.post_commit() + if scopes_result is not None: + await scopes_result.post_commit() # Only update cache if session data changed or was newly created if needs_cache_update: @@ -768,6 +839,8 @@ async def remove_peers_from_session( workspace_name: str, session_name: str, peer_names: set[str], + *, + _allow_scope_peers: bool = False, ) -> bool: """ Remove specified peers from a session. @@ -777,16 +850,32 @@ async def remove_peers_from_session( workspace_name: Name of the workspace session_name: Name of the session peer_names: Set of peer names to remove from the session + _allow_scope_peers: Internal. Set only by the scopes facade, which ends + scope membership through this same path and must not be blocked by + the guard below. Returns: True if peers were removed successfully Raises: ResourceNotFoundException: If the session does not exist + ValidationException: If any named peer is a scope """ # Verify session exists await get_session(db, session_name, workspace_name) + # Scope membership is ended through the scopes routes, which also reconcile + # 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, + workspace_name, + peer_names, + action="Scope membership is managed via the scopes routes.", + ) + # Soft delete specified session peers by setting left_at timestamp update_stmt = ( update(models.SessionPeer) @@ -798,6 +887,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() @@ -816,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 """ @@ -832,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()) ) @@ -946,13 +1060,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()) ) @@ -962,8 +1090,9 @@ async def set_peers_for_session( peers_result = await get_or_create_peers( db, workspace_name=workspace_name, - peers=[schemas.PeerCreate(name=peer_name) for peer_name in peer_names], + peers=[schemas.PeerSpec(name=peer_name) for peer_name in peer_names], ) + _reject_resolved_scope_peers(peers_result.resource) # Add new peers to session peers = await _get_or_add_peers_to_session( @@ -978,6 +1107,30 @@ async def set_peers_for_session( return peers +async def upsert_session_peers( + db: AsyncSession, + workspace_name: str, + session_name: str, + peer_names: dict[str, schemas.SessionPeerConfig], + *, + fetch_after_upsert: bool = True, +) -> list[models.SessionPeer]: + """Public wrapper around the session-peer membership upsert. + + Exists for other crud modules (currently the scopes facade in + ``src/crud/scope.py``) that manage memberships directly, bypassing the + route-level scope-peer guardrails. See ``_get_or_add_peers_to_session`` + for semantics. + """ + return await _get_or_add_peers_to_session( + db, + workspace_name=workspace_name, + session_name=session_name, + peer_names=peer_names, + fetch_after_upsert=fetch_after_upsert, + ) + + async def _get_or_add_peers_to_session( db: AsyncSession, workspace_name: str, @@ -1020,8 +1173,17 @@ async def _get_or_add_peers_to_session( result = await db.execute(select_stmt) return list(result.scalars().all()) - # Only validate observer limit if we're adding peers with observe_others=True - new_observer_count = count_observers_in_config(peer_names) + # 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, 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()) + + # Only validate observer limit if we're adding non-scope peers with observe_others=True + new_observer_count = count_observers_in_config( + {n: c for n, c in peer_names.items() if n not in scopes_being_added} + ) if new_observer_count > 0: # Use a single efficient query to count existing observers not being updated @@ -1036,6 +1198,14 @@ async def _get_or_add_peers_to_session( models.SessionPeer.configuration["observe_others"].astext.cast( Boolean ), # Only observers + # Existing scope memberships are excluded for the same reason as above. + ~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) + ), ) result = await db.execute(existing_observers_stmt) existing_observer_count = result.scalar() or 0 @@ -1111,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, @@ -1148,10 +1325,18 @@ async def set_peer_config( Raises: ObserverException: If the update would exceed the observer limit + ValidationException: If the peer is a scope """ # First, get the session and peer to ensure they exist await get_session(db, session_name, workspace_name) - await get_peer(db, workspace_name, schemas.PeerCreate(name=peer_name)) + peer = await get_peer(db, workspace_name, peer_name) + + # A scope's membership config is the facade's, not the caller's: setting + # observe_others=false silently stops all fan-out into the scope, and + # observe_me=true makes Honcho form a representation *of* a scope, which + # never happens by design. Checked on the row just resolved above, so there + # is no check-then-use window and no extra query. + _reject_resolved_scope_peers([peer]) # Check if a SessionPeer entry already exists stmt = ( 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/dialectic/chat.py b/src/dialectic/chat.py index d2f7e741..470aaf15 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, schemas +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. 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. + """ + 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, schemas.PeerCreate(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, schemas.PeerCreate(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, schemas.PeerCreate(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, schemas.PeerCreate(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/dreamer/specialists.py b/src/dreamer/specialists.py index 61e8a9d3..28ff4577 100644 --- a/src/dreamer/specialists.py +++ b/src/dreamer/specialists.py @@ -20,7 +20,7 @@ from typing import Any, cast from nanoid import generate as generate_nanoid -from src import crud, schemas +from src import crud from src.config import ConfiguredModelSettings, settings from src.dependencies import tracked_db from src.exceptions import ValidationException @@ -266,13 +266,9 @@ If you update it, send the full deduplicated list and remove stale entries. try: # Short-lived DB session for preflight operations async with tracked_db("dream.specialist.preflight") as db: - await crud.get_peer( - db, workspace_name, schemas.PeerCreate(name=observer) - ) + await crud.get_peer(db, workspace_name, observer) if observer != observed: - await crud.get_peer( - db, workspace_name, schemas.PeerCreate(name=observed) - ) + await crud.get_peer(db, workspace_name, observed) # Determine if peer card tools should be included. Specialists that # cannot write to the peer card (e.g., induction) skip the fetch and diff --git a/src/main.py b/src/main.py index 3c8bf3e8..75eac455 100644 --- a/src/main.py +++ b/src/main.py @@ -24,6 +24,7 @@ from src.routers import ( keys, messages, peers, + scopes, sessions, webhooks, workspaces, @@ -171,6 +172,7 @@ add_pagination(app) app.include_router(workspaces.router, prefix="/v3") app.include_router(peers.router, prefix="/v3") app.include_router(sessions.router, prefix="/v3") +app.include_router(scopes.router, prefix="/v3") app.include_router(messages.router, prefix="/v3") app.include_router(conclusions.router, prefix="/v3") app.include_router(keys.router, prefix="/v3") diff --git a/src/routers/peers.py b/src/routers/peers.py index bccb2dd0..d5425a83 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -30,6 +30,11 @@ from src.telemetry import prometheus_metrics from src.telemetry.events import EmbeddingCallPurpose, GetContextEvent, emit from src.utils.filter import extract_session_allowlist from src.utils.schema_conversion import json_response_schema_to_pydantic +from src.utils.scopes import ( + is_scope_peer, + is_scope_peer_name, + validate_no_scope_peer_names, +) from src.utils.search import search from src.utils.types import embedding_call_purpose @@ -54,7 +59,11 @@ async def get_peers( reverse: bool = Query(False, description="Whether to reverse the order of results"), db: AsyncSession = read_db, ): - """Get all Peers for a Workspace, paginated with optional filters.""" + """Get all Peers for a Workspace, paginated with optional filters. + + Scope peers are excluded by default; set `kind` to "scope" for scope peers + only, or "all" for everything. + """ filter_param = None if options and hasattr(options, "filters"): filter_param = options.filters @@ -67,6 +76,7 @@ async def get_peers( workspace_name=workspace_id, filters=filter_param, reverse=reverse, + kind=options.kind if options else None, ), ) @@ -100,6 +110,14 @@ async def get_or_create_peer( if not jwt_params.p: raise AuthenticationException("Peer ID not found in query parameter or JWT") peer.name = jwt_params.p + + # The scope namespace is reserved: scope peers are only created through + # the scopes facade (POST /workspaces/{workspace_id}/scopes). + validate_no_scope_peer_names( + [peer.name], + action="Use the scopes routes to create scopes.", + ) + result = await crud.get_or_create_peers( db, workspace_name=workspace_id, peers=[peer] ) @@ -122,7 +140,19 @@ async def update_peer( peer: schemas.PeerUpdate = Body(..., description="Updated peer parameters"), db: AsyncSession = db, ): - """Update a Peer's metadata and/or configuration.""" + """Update a Peer's metadata and/or configuration. + + 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 ) @@ -189,6 +219,30 @@ async def chat( Query a Peer's representation using natural language. Performs agentic search and reasoning to comprehensively answer the query based on all latent knowledge gathered about the peer from their messages and conclusions. """ + # 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 + # 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: + # 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, + action=( + "No representation is formed of a scope, so a scope cannot " + "be a chat observer or target." + ), + ) + # The session id arrives in the body, so require_auth can't gate on it. A # peer-scoped key may only scope a chat to a session its peer belongs to; # without this check it could read any session's messages (the dialectic @@ -233,8 +287,17 @@ async def chat( peers_result = await crud.get_or_create_peers( peer_db, workspace_name=workspace_id, - peers=[schemas.PeerCreate(name=peer_id)], + peers=[schemas.PeerSpec(name=peer_id)], ) + # Re-check on the resolved row: the name-level check above ran before the + # peer was resolved, so a scope created in between would be picked up here + # as existing and used as the chat observer. + observer = peers_result.resource[0] + if is_scope_peer(observer.name, observer.internal_metadata): + raise ValidationException( + "No representation is formed of a scope, so a scope cannot be a " + + "chat observer or target." + ) await peer_db.commit() await peers_result.post_commit() @@ -317,6 +380,27 @@ 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. """ + # 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) + ] + if scope_candidates: + async with tracked_db( + "peers.representation.scope_check", read_only=True + ) as s_db: + await crud.reject_scope_observed( + s_db, + workspace_id, + scope_candidates, + action=( + "No representation is formed of a scope, so a scope cannot " + "be a representation observer or target." + ), + ) + # Parse the session allowlist from filters (422 on unsupported keys/shapes, # and on a session_id the allowlist doesn't cover). session_allowlist = extract_session_allowlist( @@ -326,36 +410,75 @@ 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, + ) - # 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, + # Only ask for the semantic branch when we actually have an + # embedding. The precompute above is suppressed, and both + # `RepresentationManager.get_working_representation` and + # `crud.query_documents` fall back to embedding internally when a + # query arrives without one — which would run an external call + # inside this session, and the innermost fallback is unsuppressed + # (a provider outage would surface as a 500). Degrading to + # derived+recent retrieval keeps the session DB-only. + include_semantic_query=options.search_query + if embedding is not None + else None, + 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() ) @@ -421,6 +544,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 + # 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/scopes.py b/src/routers/scopes.py new file mode 100644 index 00000000..9d9087c0 --- /dev/null +++ b/src/routers/scopes.py @@ -0,0 +1,173 @@ +"""FastAPI routes for scope resources. + +A scope is a named grouping of sessions that provides a visibility boundary +within a peer. Internally a scope is a peer named ``scope.`` that +observes its member sessions and never speaks; these routes are the facade +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: scope membership only affects messages ingested *after* the membership +change. Conclusions already derived are neither backfilled on add nor +reconciled on removal. +""" + +import logging + +from fastapi import APIRouter, Body, Depends, Path, Query, Response +from fastapi_pagination import Page +from fastapi_pagination.ext.sqlalchemy import apaginate +from sqlalchemy.ext.asyncio import AsyncSession + +from src import crud, schemas +from src.dependencies import db, read_db +from src.security import require_auth + +logger = logging.getLogger(__name__) + +router = APIRouter( + prefix="/workspaces/{workspace_id}/scopes", + tags=["scopes"], +) + + +@router.post( + "", + response_model=schemas.Scope, + dependencies=[Depends(require_auth(workspace_name="workspace_id"))], +) +async def get_or_create_scope( + response: Response, + workspace_id: str = Path(...), + scope: schemas.ScopeCreate = Body(..., description="Scope creation parameters"), + db: AsyncSession = db, +): + """ + Get a Scope by ID or create a new Scope with the given ID. + + Returns 201 when the scope is created and 200 when it already exists. + A pre-existing peer occupying the scope's reserved internal name is never + adopted; that conflict returns 409. + """ + result = await crud.get_or_create_scopes(db, workspace_id, [scope]) + await db.commit() + await result.post_commit() + response.status_code = 201 if result.created else 200 + return result.resource[0] + + +@router.post( + "/list", + response_model=Page[schemas.Scope], + dependencies=[Depends(require_auth(workspace_name="workspace_id"))], +) +async def get_scopes( + workspace_id: str = Path(...), + reverse: bool = Query(False, description="Whether to reverse the order of results"), + db: AsyncSession = read_db, +): + """Get all Scopes for a Workspace. Results are paginated.""" + return await apaginate( + db, + await crud.get_scopes(workspace_name=workspace_id, reverse=reverse), + ) + + +@router.get( + "/{scope_id}", + response_model=schemas.Scope, + dependencies=[Depends(require_auth(workspace_name="workspace_id"))], +) +async def get_scope( + workspace_id: str = Path(...), + scope_id: str = Path(...), + db: AsyncSession = read_db, +): + """Get a single Scope by ID.""" + return await crud.get_scope_or_raise(db, workspace_id, scope_id) + + +@router.post( + "/{scope_id}/sessions", + status_code=204, + response_model=None, + dependencies=[Depends(require_auth(workspace_name="workspace_id"))], +) +async def add_sessions_to_scope( + workspace_id: str = Path(...), + scope_id: str = Path(...), + body: schemas.ScopeSessionsAdd = Body( + ..., description="IDs of the sessions to add to the scope" + ), + db: AsyncSession = db, +): + """ + Add Sessions to a Scope. + + 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 applies only to messages ingested after this call; + conclusions already derived are not backfilled. + """ + await crud.add_sessions_to_scope( + db, + workspace_name=workspace_id, + scope_name=scope_id, + session_names=body.session_ids, + ) + + +@router.delete( + "/{scope_id}/sessions/{session_id}", + status_code=204, + response_model=None, + dependencies=[Depends(require_auth(workspace_name="workspace_id"))], +) +async def remove_session_from_scope( + workspace_id: str = Path(...), + scope_id: str = Path(...), + session_id: str = Path(...), + db: AsyncSession = db, +): + """ + Remove a Session from a Scope. + + Note: conclusions already derived while the session was a member are left in + place. + """ + await crud.remove_session_from_scope( + db, + workspace_name=workspace_id, + scope_name=scope_id, + session_name=session_id, + ) + + +@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 Sessions that are members of a Scope, paginated. + + Ordered by how long each session has been a member: longest-standing member + first, or most recently added first when `reverse` is true. + """ + # 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 191748d4..b86b5706 100644 --- a/src/routers/sessions.py +++ b/src/routers/sessions.py @@ -35,6 +35,15 @@ router = APIRouter( tags=["sessions"], ) +# Guidance appended to guardrail errors when a scope peer is passed to the +# generic session-peer surface. Scope membership is managed only through the +# scopes facade so the observer mechanics stay internal. +_SCOPES_ROUTE_GUIDANCE = ( + "Scope membership is managed via the scopes routes " + "(/v3/workspaces/{workspace_id}/scopes/{scope_id}/sessions) or the `scopes` " + "field at session creation." +) + async def _get_working_representation_task( db: AsyncSession, @@ -309,6 +318,23 @@ 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: + await crud.reject_scope_peers( + db, workspace_id, session.peer_names.keys(), action=_SCOPES_ROUTE_GUIDANCE + ) + # Handle session creation with proper error handling try: result = await crud.get_or_create_session( @@ -440,7 +466,13 @@ async def add_peers_to_session( ), db: AsyncSession = db, ): - """Add Peers to a Session. If a Peer does not yet exist, it will be created automatically.""" + """Add Peers to a Session. If a Peer does not yet exist, it will be created automatically. + + Scope peers cannot be added here; scope membership is managed via the scopes routes. + """ + await crud.reject_scope_peers( + db, workspace_id, peers.keys(), action=_SCOPES_ROUTE_GUIDANCE + ) try: result = await crud.get_or_create_session( db, @@ -476,7 +508,12 @@ async def set_session_peers( Set the Peers in a Session. If a Peer does not yet exist, it will be created automatically. This will fully replace the current set of Peers in the Session. + + Scope peers cannot be set here; scope membership is managed via the scopes routes. """ + await crud.reject_scope_peers( + db, workspace_id, peers.keys(), action=_SCOPES_ROUTE_GUIDANCE + ) try: await crud.set_peers_for_session( db, @@ -512,7 +549,13 @@ async def remove_peers_from_session( ), db: AsyncSession = db, ): - """Remove Peers by ID from a Session.""" + """Remove Peers by ID from a Session. + + Scope peers cannot be removed here; scope membership is managed via the scopes routes. + """ + await crud.reject_scope_peers( + db, workspace_id, peers, action=_SCOPES_ROUTE_GUIDANCE + ) try: await crud.remove_peers_from_session( db, @@ -712,6 +755,22 @@ 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, and the read-side scope surface will build on that. + if peer_target is not None: + # 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], + 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..f7bcdcd2 100644 --- a/src/routers/workspaces.py +++ b/src/routers/workspaces.py @@ -225,6 +225,10 @@ async def schedule_dream( observed = request.observed if request.observed is not None else request.observer dream_type = request.dream_type + # 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, observer=observer, diff --git a/src/schemas/__init__.py b/src/schemas/__init__.py index 85d5da44..cc00d76a 100644 --- a/src/schemas/__init__.py +++ b/src/schemas/__init__.py @@ -31,10 +31,14 @@ from src.schemas.api import ( PeerCreate, PeerGet, PeerRepresentationGet, + PeerSpec, PeerUpdate, QueueStatus, RepresentationResponse, ScheduleDreamRequest, + Scope, + ScopeCreate, + ScopeSessionsAdd, Session, SessionBase, SessionContext, @@ -125,12 +129,16 @@ __all__ = [ "PeerContext", "PeerCreate", "PeerGet", + "PeerSpec", "PeerRepresentationGet", "PeerUpdate", "QueueStatus", "RESOURCE_NAME_PATTERN", "RepresentationResponse", "ScheduleDreamRequest", + "Scope", + "ScopeCreate", + "ScopeSessionsAdd", "Session", "SessionBase", "SessionContext", diff --git a/src/schemas/api.py b/src/schemas/api.py index 78e5a125..f686d007 100644 --- a/src/schemas/api.py +++ b/src/schemas/api.py @@ -6,7 +6,8 @@ API contract. import datetime import ipaddress -from typing import Annotated, Any, Self, cast +import re +from typing import Annotated, Any, Literal, Self, cast from urllib.parse import urlparse import tiktoken @@ -29,6 +30,11 @@ from src.schemas.configuration import ( SessionPeerConfig, WorkspaceConfiguration, ) +from src.utils.scopes import ( + SCOPE_PEER_PREFIX, + is_scope_peer_name, + scope_name_from_peer, +) from src.utils.types import DocumentLevel # --------------------------------------------------------------------------- @@ -86,6 +92,31 @@ def _validate_metadata(v: Any) -> Any: _SanitizedMetadata = Annotated[dict[str, Any], BeforeValidator(_validate_metadata)] +# Scope names are stored as peer names with the reserved prefix prepended, so +# they must leave room for the prefix within the 512-character peer name limit. +_SCOPE_NAME_MAX_LENGTH = 512 - len(SCOPE_PEER_PREFIX) + + +def _validate_scope_name(name: str) -> str: + """Validate an unprefixed scope name.""" + if not 1 <= len(name) <= _SCOPE_NAME_MAX_LENGTH: + raise ValueError( + f"Scope name must be between 1 and {_SCOPE_NAME_MAX_LENGTH} characters" + ) + # Checked before the charset pattern: the reserved prefix is itself outside + # RESOURCE_NAME_PATTERN, so the pattern would otherwise reject a + # double-prefixed name first and report the charset instead of the real + # mistake. + if name.startswith(SCOPE_PEER_PREFIX): + raise ValueError( + "Scope name must not start with the reserved prefix " + + f"'{SCOPE_PEER_PREFIX}' (scope names are unprefixed)" + ) + if not re.fullmatch(RESOURCE_NAME_PATTERN, name): + raise ValueError(f"Scope name must match pattern {RESOURCE_NAME_PATTERN}") + return name + + # --------------------------------------------------------------------------- # Workspace schemas # --------------------------------------------------------------------------- @@ -139,19 +170,47 @@ class PeerBase(BaseModel): pass -class PeerCreate(PeerBase): +class PeerSpec(PeerBase): + """Peer identity plus optional updates, for callers that already have a name. + + ``PeerCreate`` narrows ``name`` with ``pattern=RESOURCE_NAME_PATTERN`` because it + validates a *new, user-supplied* peer id at the API boundary. crud paths reach + ``get_or_create_peers`` with names that already exist — a path param, a message + author, an existing row — including pre-``d429de0e5338`` legacy names containing + '.' and every ``scope.``-prefixed peer name. Re-validating those turns a lookup + into a raw pydantic ValidationError, i.e. an HTTP 500. + + Carries **no** constraints at all, deliberately. Length limits here were the + same trap as the charset pattern: request-bound peer names (message authors, + session peer-map keys) have no length bound of their own, so an empty or + over-long name reached ``PeerSpec(...)`` and raised internally — again a 500. + Every rule for a *new* name lives in ``crud.peer._validate_new_peer_names``, + which runs on the insert path only. + """ + + name: str + metadata: _SanitizedMetadata | None = None + configuration: dict[str, Any] | None = None + + +class PeerCreate(PeerSpec): name: Annotated[ str, Field(alias="id", min_length=1, max_length=512, pattern=RESOURCE_NAME_PATTERN), ] - metadata: _SanitizedMetadata | None = None - configuration: dict[str, Any] | None = None model_config = ConfigDict(populate_by_name=True) # pyright: ignore class PeerGet(PeerBase): filters: dict[str, Any] | None = None + kind: Literal["scope", "all"] | None = Field( + default=None, + description=( + "Which kinds of peers to list. Omitted (default): regular peers only " + "(scope peers are excluded). 'scope': scope peers only. 'all': every peer." + ), + ) class PeerUpdate(PeerBase): @@ -337,6 +396,25 @@ class SessionCreate(SessionBase): metadata: _SanitizedMetadata | None = None peer_names: dict[str, SessionPeerConfig] | None = Field(default=None, alias="peers") configuration: SessionConfiguration | None = None + scopes: list[str] | None = Field( + default=None, + 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. Membership applies " + "only to messages ingested after the session is added to the scope; " + "conclusions already derived are not backfilled." + ), + ) + + @field_validator("scopes") + @classmethod + def validate_scopes(cls, v: list[str] | None) -> list[str] | None: + if v is None: + return v + for scope_name in v: + _validate_scope_name(scope_name) + return v model_config = ConfigDict(populate_by_name=True) # pyright: ignore @@ -431,6 +509,61 @@ class SessionSummaries(SessionBase): ) +# --------------------------------------------------------------------------- +# Scope schemas +# --------------------------------------------------------------------------- + + +class ScopeCreate(BaseModel): + """Schema for creating (or getting) a scope by its unprefixed name.""" + + name: Annotated[str, Field(alias="id", min_length=1)] + metadata: _SanitizedMetadata | None = None + + @field_validator("name") + @classmethod + def validate_name(cls, v: str) -> str: + return _validate_scope_name(v) + + model_config = ConfigDict(populate_by_name=True) # pyright: ignore + + +class Scope(BaseModel): + """Scope response — external view of the peer backing a scope. + + The ``id`` is the unprefixed scope name; the reserved peer-name prefix is + an internal implementation detail and never surfaces here. + """ + + name: str = Field(serialization_alias="id") + h_metadata: dict[str, Any] = Field( + default_factory=dict, serialization_alias="metadata" + ) + created_at: datetime.datetime + + @field_validator("name", mode="after") + @classmethod + def strip_scope_prefix(cls, v: str) -> str: + # Constructed from Peer ORM rows whose names carry the prefix; accept + # already-unprefixed names too so manual construction works. + return scope_name_from_peer(v) if is_scope_peer_name(v) else v + + model_config = ConfigDict( # pyright: ignore + from_attributes=True, populate_by_name=True + ) + + +class ScopeSessionsAdd(BaseModel): + """Schema for adding sessions to a scope.""" + + session_ids: list[str] = Field( + ..., + min_length=1, + max_length=100, + description="IDs of existing sessions to add to the scope", + ) + + # --------------------------------------------------------------------------- # 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/src/utils/scopes.py b/src/utils/scopes.py new file mode 100644 index 00000000..c7b89192 --- /dev/null +++ b/src/utils/scopes.py @@ -0,0 +1,89 @@ +"""Scope namespace helpers. + +A *scope* is a named grouping of sessions that provides a visibility boundary +within a peer. Under the hood a scope named ``therapy`` is a peer named +``scope.therapy`` that observes its member sessions and never speaks. +Developers manage scopes exclusively through the ``/scopes`` routes (and the +``scopes`` field on session creation) and never see the observer/observed +mechanics. + +This module is the single source of truth for the reserved name prefix and the +``kind`` flag. Being a scope requires **both**: the reserved name prefix (the +namespace) and ``{"kind": "scope"}`` in the peer's ``internal_metadata`` JSONB +(the authoritative marker). Neither half is forgeable — the prefix sits outside +``RESOURCE_NAME_PATTERN``, and ``internal_metadata`` appears in no API schema — +so requiring both means a peer that merely occupies the namespace, or merely +carries a look-alike ``configuration``, is not a scope. +""" + +from collections.abc import Iterable +from typing import Any + +from src.exceptions import ValidationException + +# Reserved peer-name prefix for scope peers. User-created peers may not use it. +# +# The '.' is load-bearing: it is outside RESOURCE_NAME_PATTERN +# (^[a-zA-Z0-9_-]+$), the charset every peer name created through the API must +# match. No peer created through the validated API can therefore occupy this +# namespace. (Peers carried over by the users->peers rename in +# d429de0e5338 predate that pattern and were never charset-validated, so the +# legacy-collision path in crud/scope.py stays as a backstop.) +SCOPE_PEER_PREFIX = "scope." + +# Value of the `kind` configuration flag carried by scope peers. +SCOPE_KIND = "scope" + + +def scope_peer_name(scope_name: str) -> str: + """Return the peer name backing the given (unprefixed) scope name.""" + return f"{SCOPE_PEER_PREFIX}{scope_name}" + + +def is_scope_peer_name(name: str) -> bool: + """Return whether a peer name lives in the reserved scope namespace.""" + return name.startswith(SCOPE_PEER_PREFIX) + + +def scope_name_from_peer(peer_name: str) -> str: + """Return the unprefixed scope name for a scope peer name. + + Raises: + ValueError: If the peer name is not in the scope namespace. + """ + if not is_scope_peer_name(peer_name): + raise ValueError(f"{peer_name} is not a scope peer name") + return peer_name[len(SCOPE_PEER_PREFIX) :] + + +def is_scope_peer(name: str, internal_metadata: dict[str, Any] | None) -> bool: + """Authoritative scope test: reserved name AND the internal kind flag. + + Takes ``(name, internal_metadata)`` rather than a ``Peer`` so it is callable + from an ORM instance, the cached plain dict built by ``crud.peer._fetch_peer``, + or a raw row. + """ + return ( + is_scope_peer_name(name) + and bool(internal_metadata) + and internal_metadata.get("kind") == SCOPE_KIND + ) + + +def validate_no_scope_peer_names(names: Iterable[str], *, action: str) -> None: + """Reject any peer name that uses the reserved scope namespace. + + Args: + names: Peer names to check. + action: Human-readable guidance appended to the error, directing the + caller to the supported path (e.g. the ``/scopes`` routes). + + Raises: + ValidationException: If any name starts with the reserved prefix. + """ + offenders = sorted({name for name in names if is_scope_peer_name(name)}) + if offenders: + raise ValidationException( + f"Peer name(s) {offenders} use the reserved scope prefix " + + f"'{SCOPE_PEER_PREFIX}'. {action}" + ) diff --git a/tests/crud/test_get_or_create_retry_invalidation.py b/tests/crud/test_get_or_create_retry_invalidation.py new file mode 100644 index 00000000..fc5e93b9 --- /dev/null +++ b/tests/crud/test_get_or_create_retry_invalidation.py @@ -0,0 +1,197 @@ +"""Regression tests for cache invalidation across the get_or_create retry path. + +`get_or_create_peers` / `get_or_create_scopes` mutate existing rows, then insert +new ones inside `db.begin_nested()`. A concurrent writer that creates one of those +rows first makes the insert raise `IntegrityError`, and the function retries. + +The subtlety: `begin_nested()` autoflushes the pending mutations *before* opening +the savepoint, so the rollback neither undoes them nor expires the ORM state. A +retry that recomputed "what changed" from that state would see no change and skip +the cache purge — while the row change still commits anyway, leaving the cache +stale until TTL. These tests pin the purge. + +The race is real (a second session committing a real row, producing a real +IntegrityError from the database); only its *timing* is made deterministic, by +hooking the one point that sits between the SELECT and the INSERT. +""" + +from unittest.mock import AsyncMock, patch + +import pytest +from nanoid import generate as generate_nanoid +from sqlalchemy.ext.asyncio import ( + AsyncEngine, + AsyncSession, + AsyncSessionTransaction, + async_sessionmaker, +) + +from src import crud, models, schemas +from src.crud.peer import peer_cache_key +from src.crud.scope import SCOPE_PEER_CONFIGURATION, SCOPE_PEER_INTERNAL_METADATA +from src.utils.scopes import scope_peer_name + + +class _RaceOnBeginNested: + """Commit a racing row on entry to `begin_nested()`, then delegate. + + That entry point is after the function's SELECT and metadata mutation but + before its INSERT flushes — precisely the window a real concurrent writer + has to slip through to trigger the IntegrityError retry. + """ + + _db: AsyncSession + _engine: AsyncEngine + _rows: list[models.Peer] + _real: AsyncSessionTransaction | None + fired: bool + + def __init__(self, db: AsyncSession, engine: AsyncEngine, rows: list[models.Peer]): + self._db = db + self._engine = engine + self._rows = rows + self._real = None + self.fired = False + + def __call__(self): + return self + + async def __aenter__(self): + if self._rows: + Session = async_sessionmaker(bind=self._engine, expire_on_commit=False) + async with Session() as other: + other.add_all(self._rows) + await other.commit() + self._rows = [] # race only once; the retry must succeed + self.fired = True + self._real = AsyncSession.begin_nested(self._db) + return await self._real.__aenter__() + + async def __aexit__(self, *exc_info: object): + assert self._real is not None + return await self._real.__aexit__(*exc_info) + + +@pytest.mark.asyncio +async def test_peer_retry_still_invalidates_mutated_peer( + db_session: AsyncSession, + db_engine: AsyncEngine, + sample_data: tuple[models.Workspace, models.Peer], +): + """A peer mutated before a losing race still gets its cache key purged.""" + test_workspace, existing_peer = sample_data + racer_name = str(generate_nanoid()) + + # Give the existing peer metadata we will then change, so it is a real update. + existing_peer.h_metadata = {"v": "old"} + await db_session.commit() + + race = _RaceOnBeginNested( + db_session, + db_engine, + [models.Peer(name=racer_name, workspace_name=test_workspace.name)], + ) + + with ( + patch("src.crud.peer.safe_cache_delete", new=AsyncMock()) as mock_delete, + patch.object(db_session, "begin_nested", race), + ): + result = await crud.get_or_create_peers( + db_session, + test_workspace.name, + [ + schemas.PeerCreate(name=existing_peer.name, metadata={"v": "new"}), + schemas.PeerCreate(name=racer_name), + ], + ) + await db_session.commit() + await result.post_commit() + + assert race.fired, "the race must actually have fired" + + purged = {call.args[0] for call in mock_delete.await_args_list} + assert ( + peer_cache_key(test_workspace.name, existing_peer.name) in purged + ), "the mutated peer's cache key must still be purged after the retry" + + # The mutation really did land — which is what makes a missed purge stale. + await db_session.refresh(existing_peer) + assert existing_peer.h_metadata == {"v": "new"} + + +@pytest.mark.asyncio +async def test_scope_retry_still_invalidates_mutated_scope( + db_session: AsyncSession, + db_engine: AsyncEngine, + sample_data: tuple[models.Workspace, models.Peer], +): + """Same guarantee for the scopes facade, which mirrors get_or_create_peers.""" + test_workspace, _ = sample_data + kept_scope, racing_scope = str(generate_nanoid()), str(generate_nanoid()) + + seeded = await crud.get_or_create_scopes( + db_session, + test_workspace.name, + [schemas.ScopeCreate(name=kept_scope, metadata={"v": "old"})], + ) + await db_session.commit() + await seeded.post_commit() + + # The racer creates the second scope's backing peer — as a *valid* scope peer, + # so the flow reaches the insert rather than tripping the legacy-collision 409. + race = _RaceOnBeginNested( + db_session, + db_engine, + [ + models.Peer( + name=scope_peer_name(racing_scope), + workspace_name=test_workspace.name, + internal_metadata=dict(SCOPE_PEER_INTERNAL_METADATA), + configuration=dict(SCOPE_PEER_CONFIGURATION), + ) + ], + ) + + with ( + patch("src.crud.scope.safe_cache_delete", new=AsyncMock()) as mock_delete, + patch.object(db_session, "begin_nested", race), + ): + result = await crud.get_or_create_scopes( + db_session, + test_workspace.name, + [ + schemas.ScopeCreate(name=kept_scope, metadata={"v": "new"}), + schemas.ScopeCreate(name=racing_scope), + ], + ) + await db_session.commit() + await result.post_commit() + + assert race.fired, "the race must actually have fired" + + purged = {call.args[0] for call in mock_delete.await_args_list} + assert ( + peer_cache_key(test_workspace.name, scope_peer_name(kept_scope)) in purged + ), "the mutated scope peer's cache key must still be purged after the retry" + + +@pytest.mark.asyncio +async def test_peer_no_race_does_not_invalidate_unchanged_peer( + db_session: AsyncSession, + sample_data: tuple[models.Workspace, models.Peer], +): + """Baseline: with no race, an unchanged peer is not purged.""" + test_workspace, existing_peer = sample_data + existing_peer.h_metadata = {"v": "same"} + await db_session.commit() + + with patch("src.crud.peer.safe_cache_delete", new=AsyncMock()) as mock_delete: + result = await crud.get_or_create_peers( + db_session, + test_workspace.name, + [schemas.PeerCreate(name=existing_peer.name, metadata={"v": "same"})], + ) + await db_session.commit() + await result.post_commit() + + assert mock_delete.await_count == 0, "an unchanged peer must not be purged" 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 new file mode 100644 index 00000000..79884d1f --- /dev/null +++ b/tests/routes/test_scope_route_policy.py @@ -0,0 +1,941 @@ +"""Route-policy enumeration for the scopes facade, per peer *position*. + +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: + + A scope may be an OBSERVER. A scope may never be OBSERVED. + +`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. + +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 gap there would not be caught here. +""" + +from collections.abc import Callable, Iterator +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 +from src.main import app +from src.models import Peer, Workspace +from src.utils.scopes import scope_peer_name + +# 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", + "sender_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], Response] + + +@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 + # 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 = "" + # Required when refuse_missing is False: the exact status(es) a missing + # reserved name may receive. Deliberately not a bare `!= 422` — that passes on + # a 5xx too, which is the same hole the squatter assertion below had. + missing_status: tuple[int, ...] = () + # 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 = "" + # 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, ...] = () + + @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 _b_repr_observer(c: TestClient, ws: str, _s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/peers/{p}/representation", json={}) + + +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 _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 _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": {p: {}}}, + ) + + +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 _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 _b_add_peers(c: TestClient, ws: str, s: str, p: str): + return c.post(f"/v3/workspaces/{ws}/sessions/{s}/peers", json={p: {}}) + + +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 _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", + json={"observe_others": False, "observe_me": True}, + ) + + +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" + +_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, + 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." + ), + missing_status=(404,), + 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}/peers/{{peer_id}}/representation", + "target", + True, + refuse_missing=True, + build=_b_repr_target, + ), + Case( + "GET", + f"{_W}/sessions/{{session_id}}/context", + "peer_target", + True, + refuse_missing=True, + build=_b_session_context_target, + ), + # ---- observer position: legitimately a scope ---- + 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,), + ), + # ---- peer identity / membership mutation: never a scope ---- + Case( + "POST", + f"{_W}/peers", + _KEY_POSITION, + True, + refuse_missing=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, + 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( + "POST", + f"{_W}/sessions/{{session_id}}/messages/upload", + "peer_id", + True, + refuse_missing=True, + build=_b_upload, + ), + Case( + "POST", + f"{_W}/sessions/{{session_id}}/peers", + _KEY_POSITION, + True, + refuse_missing=True, + build=_b_add_peers, + ), + Case( + "PUT", + f"{_W}/sessions/{{session_id}}/peers", + _KEY_POSITION, + True, + refuse_missing=True, + build=_b_set_peers, + ), + Case( + "DELETE", + 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." + ), + missing_status=(200,), + build=_b_remove_peers, + ), + Case( + "PUT", + 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." + ), + missing_status=(404,), + 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." + ), + missing_status=(404,), + 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( + "POST", + 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( + "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 POST /scopes/{scope_id}/sessions/list." + ), + ), + Case( + "GET", + f"{_W}/peers/{{peer_id}}/context", + "peer_id", + False, + reason=( + "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( + "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. The read-side scope surface is not implemented yet." + ), + ), + 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, so no peer row is read " + "or written. Keys cannot be bound to a scope yet." + ), + ), +) + +_BY_KEY = {case.key: case for case in POLICY} + +# 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"), +} + + +def _nested_models(annotation: object, seen: set[object]) -> Iterator[type[BaseModel]]: + """Yield `annotation` and every pydantic model nested inside it.""" + if ( + not isinstance(annotation, type) + or not issubclass(annotation, BaseModel) + or annotation in seen + ): + return + model: type[BaseModel] = annotation + seen.add(model) + yield model + fields: dict[str, FieldInfo] = model.model_fields + 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_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 + request-body models so `MessageCreate.peer_name` is seen too. + """ + found: set[str] = set() + seen: set[object] = set() + stack = [route.dependant] + while stack: + dependant = stack.pop() + params = ( + dependant.path_params + + dependant.query_params + + dependant.header_params + + dependant.body_params + ) + for param in params: + if param.name in _PEER_PARAM_NAMES: + found.add(param.name) + annotations = [param.field_info.annotation] + while annotations: + annotation = annotations.pop() + for model in _nested_models(annotation, seen): + found |= set(model.model_fields) & _PEER_PARAM_NAMES + annotations.extend(getattr(annotation, "__args__", ()) or ()) + stack.extend(dependant.dependencies) + return found + + +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 + positions = _peer_positions(route) + for method in route.methods or set(): + 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_position_is_classified(): + """Each (route, peer position) pair has an explicit scope policy. + + 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. + """ + 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: " + + f"{sorted(unclassified)} — classify each as refuse or allow" + ) + + stale = classified - derived + assert not stale, f"classified positions that no longer exist: {sorted(stale)}" + + +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" + # 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 case.refuse_missing: + assert not case.missing_status, ( + f"{case.key} refuses a missing reserved name, so the expected " + "status is 422 — missing_status is for the permissive cases" + ) + else: + assert ( + len(case.missing_reason.strip()) > 30 + ), f"{case.key} tolerates a missing reserved name; say why" + assert case.missing_status, ( + f"{case.key} tolerates a missing reserved name; name the exact " + "status(es) it should get, so a 5xx cannot satisfy the case" + ) + 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) +# 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]: + """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/{workspace}/sessions", json={"id": session_name} + ).status_code in (200, 201) + 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, + 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) + 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 = 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 = result.text + 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"{case.key} returned 422 but not because of the scope; " + f"detail: {detail[:200]}" + ) + + +@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) + 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) + 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 " + f"{result.status_code} — {result.text[:200]}" + ) + + +@pytest.mark.parametrize( + "case", + tuple(c for c in _REFUSING if not c.skip_squatter), + ids=lambda c: f"{c.method}:{c.position}", +) +async def test_refusing_position_allows_unflagged_squatter( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + 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 — 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() + + # 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", + json={squatter: {}}, + ).status_code + == 200 + ) + + assert case.build is not None + result = case.build(client, test_workspace.name, session_name, squatter) + # 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: + # Exact, not `!= 422`: a permissive position still has one correct answer, + # and a 5xx must not read as tolerance. + assert result.status_code in case.missing_status, ( + f"{case.method} {case.path} gave {result.status_code} for a missing " + f"reserved name in position {case.position!r}; the policy expects " + f"{case.missing_status} because {case.missing_reason!r} — update the " + f"policy or the guard. Body: {result.text[:200]}" + ) + + 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 new file mode 100644 index 00000000..a68a3725 --- /dev/null +++ b/tests/routes/test_scopes.py @@ -0,0 +1,1201 @@ +"""Tests for the scopes facade: scope-kind peers, guardrails, and CRUD routes. + +A scope is a named grouping of sessions, implemented as a peer named +``scope.`` carrying ``{"kind": "scope"}`` in ``internal_metadata`` and +``{"observe_me": false}`` in ``configuration``, that observes its member sessions +and never speaks. See src/utils/scopes.py. +""" + +import re +from typing import Any + +import pytest +from fastapi.testclient import TestClient +from nanoid import generate as generate_nanoid +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from src import crud, models, schemas +from src.config import settings +from src.deriver.enqueue import enqueue +from src.exceptions import ValidationException +from src.models import Peer, QueueItem, Workspace +from src.schemas.api import RESOURCE_NAME_PATTERN +from src.security import JWTParams, create_jwt +from src.utils.scopes import ( + SCOPE_PEER_PREFIX, + is_scope_peer, + is_scope_peer_name, + scope_name_from_peer, + scope_peer_name, +) + + +def _create_scope( + client: TestClient, + workspace_name: str, + scope_name: str, + metadata: dict[str, Any] | None = None, +): + body: dict[str, Any] = {"id": scope_name} + if metadata is not None: + body["metadata"] = metadata + return client.post(f"/v3/workspaces/{workspace_name}/scopes", json=body) + + +def _create_session( + client: TestClient, + workspace_name: str, + session_name: str | None = None, + **extra: Any, +): + session_name = session_name or str(generate_nanoid()) + response = client.post( + f"/v3/workspaces/{workspace_name}/sessions", + json={"id": session_name, **extra}, + ) + assert response.status_code in [200, 201] + 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, + session_name: str, + peer_name: str, +) -> models.SessionPeer | None: + return await db_session.scalar( + select(models.SessionPeer) + .where(models.SessionPeer.workspace_name == workspace_name) + .where(models.SessionPeer.session_name == session_name) + .where(models.SessionPeer.peer_name == peer_name) + ) + + +def test_scope_namespace_helpers(): + assert scope_peer_name("therapy") == "scope.therapy" + assert is_scope_peer_name("scope.therapy") + assert not is_scope_peer_name("therapy") + assert scope_name_from_peer("scope.therapy") == "therapy" + # The prefix must stay outside the peer-name charset, or an existing peer + # could occupy the scope namespace. + assert not re.fullmatch(RESOURCE_NAME_PATTERN, SCOPE_PEER_PREFIX) + + +async def test_create_scope_creates_flagged_peer( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """Creating a scope creates a peer with the kind flag and observe_me=false.""" + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + + response = _create_scope( + client, test_workspace.name, scope_name, metadata={"purpose": "testing"} + ) + assert response.status_code == 201 + data = response.json() + # The response id is the UNPREFIXED scope name + assert data["id"] == scope_name + assert data["metadata"] == {"purpose": "testing"} + assert "created_at" in data + + peer = await db_session.scalar( + select(models.Peer) + .where(models.Peer.workspace_name == test_workspace.name) + .where(models.Peer.name == scope_peer_name(scope_name)) + ) + assert peer is not None + # The kind flag lives in internal_metadata (not user-writable); only + # observe_me is in the user-visible configuration. + assert peer.internal_metadata == {"kind": "scope"} + assert peer.configuration == {"observe_me": False} + + +def test_create_scope_idempotent( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + + first = _create_scope(client, test_workspace.name, scope_name) + assert first.status_code == 201 + + second = _create_scope(client, test_workspace.name, scope_name) + assert second.status_code == 200 + assert second.json()["id"] == scope_name + + +def test_create_scope_rejects_invalid_names( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + test_workspace, _ = sample_data + + # Names must match the resource name pattern + response = _create_scope(client, test_workspace.name, "bad name!") + assert response.status_code == 422 + + # Scope names are unprefixed: double-prefixing is rejected + response = _create_scope(client, test_workspace.name, "scope.therapy") + assert response.status_code == 422 + + +async def test_create_scope_rejects_legacy_collision( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """A pre-existing plain peer occupying the reserved name is never adopted.""" + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + + legacy_peer = models.Peer( + workspace_name=test_workspace.name, + name=scope_peer_name(scope_name), + ) + db_session.add(legacy_peer) + await db_session.commit() + + response = _create_scope(client, test_workspace.name, scope_name) + assert response.status_code == 409 + + # And the collision peer is invisible to the scope read routes + response = client.get(f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}") + assert response.status_code == 404 + + +def test_get_scope(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 + + response = client.get(f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}") + assert response.status_code == 200 + assert response.json()["id"] == scope_name + + response = client.get( + f"/v3/workspaces/{test_workspace.name}/scopes/{generate_nanoid()}" + ) + assert response.status_code == 404 + + +def test_list_scopes(client: TestClient, sample_data: tuple[Workspace, Peer]): + """The scopes list contains only scopes, with unprefixed ids.""" + test_workspace, test_peer = sample_data + scope_names = {str(generate_nanoid()), str(generate_nanoid())} + 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/list") + assert response.status_code == 200 + items = response.json()["items"] + listed = {item["id"] for item in items} + assert listed == scope_names + assert test_peer.name not in listed + + +def test_scopes_routes_require_workspace_level_key( + client: TestClient, + sample_data: tuple[Workspace, Peer], + monkeypatch: pytest.MonkeyPatch, +): + """Scopes are an app-level admin surface: workspace keys work, peer- and + session-scoped keys are rejected.""" + test_workspace, test_peer = sample_data + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + scope_name = str(generate_nanoid()) + scopes_url = f"/v3/workspaces/{test_workspace.name}/scopes" + + # Workspace-scoped key: allowed + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name))}" + ) + assert client.post(scopes_url, json={"id": scope_name}).status_code == 201 + + # Peer-scoped key: rejected + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=test_peer.name))}" + ) + assert client.post(scopes_url, json={"id": scope_name}).status_code == 401 + assert client.post(f"{scopes_url}/list").status_code == 401 + assert client.get(f"{scopes_url}/{scope_name}").status_code == 401 + + # Session-scoped key: rejected + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, s='some-session'))}" + ) + 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( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """User-created peers may not use the reserved scope prefix. + + The prefix sits outside RESOURCE_NAME_PATTERN, so PeerCreate's own charset + validation rejects it at the schema boundary — one layer earlier than the + route's validate_no_scope_peer_names guard. Either way the caller gets 422. + """ + test_workspace, _ = sample_data + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/peers", + json={"name": f"{SCOPE_PEER_PREFIX}{generate_nanoid()}"}, + ) + assert response.status_code == 422 + assert RESOURCE_NAME_PATTERN in str(response.json()["detail"]) + + +def test_peers_list_kind_filtering( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """peers.list excludes scope peers by default; kind switches the view.""" + test_workspace, test_peer = 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) + + # Default: scope peers excluded + response = client.post(f"/v3/workspaces/{test_workspace.name}/peers/list") + assert response.status_code == 200 + names = {item["id"] for item in response.json()["items"]} + assert test_peer.name in names + assert backing_peer_name not in names + + # kind=scope: only scope peers + response = client.post( + f"/v3/workspaces/{test_workspace.name}/peers/list", + json={"kind": "scope"}, + ) + assert response.status_code == 200 + names = {item["id"] for item in response.json()["items"]} + assert names == {backing_peer_name} + + # kind=all: everything + response = client.post( + f"/v3/workspaces/{test_workspace.name}/peers/list", + json={"kind": "all"}, + ) + assert response.status_code == 200 + names = {item["id"] for item in response.json()["items"]} + assert {test_peer.name, backing_peer_name} <= names + + +async def test_scope_sessions_add_list_remove( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + test_workspace, _ = sample_data + workspace_name = test_workspace.name + scope_name = str(generate_nanoid()) + assert _create_scope(client, workspace_name, scope_name).status_code == 201 + session_1 = _create_session(client, workspace_name) + session_2 = _create_session(client, workspace_name) + + scope_base = f"/v3/workspaces/{workspace_name}/scopes/{scope_name}" + + # Add both sessions + assert ( + _add_sessions(client, workspace_name, scope_name, [session_1, session_2]) == 204 + ) + 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( + db_session, workspace_name, session_1, scope_peer_name(scope_name) + ) + assert session_peer is not None + assert session_peer.left_at is None + assert session_peer.configuration["observe_others"] is True + assert session_peer.configuration["observe_me"] is False + + # 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 + + assert _scope_sessions(client, workspace_name, scope_name) == [session_2] + + db_session.expire_all() + session_peer = await _get_session_peer( + db_session, workspace_name, session_1, scope_peer_name(scope_name) + ) + assert session_peer is not None + assert session_peer.left_at is not None + + +def test_scope_sessions_add_missing_session_404( + 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 + existing_session = _create_session(client, test_workspace.name) + + assert ( + _add_sessions( + client, + test_workspace.name, + scope_name, + [existing_session, str(generate_nanoid())], + ) + == 404 + ) + + +def test_scope_sessions_routes_404_on_unknown_scope( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + test_workspace, _ = sample_data + session_name = _create_session(client, test_workspace.name) + unknown_scope = str(generate_nanoid()) + scope_base = f"/v3/workspaces/{test_workspace.name}/scopes/{unknown_scope}" + + assert ( + _add_sessions(client, test_workspace.name, unknown_scope, [session_name]) == 404 + ) + assert client.post(f"{scope_base}/sessions/list").status_code == 404 + assert client.delete(f"{scope_base}/sessions/{session_name}").status_code == 404 + + +async def test_session_create_with_scopes( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """`scopes` on session creation creates the scope peers and memberships.""" + test_workspace, _ = sample_data + scope_a = str(generate_nanoid()) + scope_b = str(generate_nanoid()) + session_name = str(generate_nanoid()) + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={"id": session_name, "scopes": [scope_a, scope_b]}, + ) + assert response.status_code == 201 + + for scope_name in (scope_a, scope_b): + peer = await db_session.scalar( + select(models.Peer) + .where(models.Peer.workspace_name == test_workspace.name) + .where(models.Peer.name == scope_peer_name(scope_name)) + ) + assert peer is not None + assert peer.internal_metadata == {"kind": "scope"} + assert peer.configuration == {"observe_me": False} + + session_peer = await _get_session_peer( + db_session, test_workspace.name, session_name, scope_peer_name(scope_name) + ) + assert session_peer is not None + assert session_peer.configuration["observe_others"] is True + assert session_peer.configuration["observe_me"] is False + + # And the memberships show up through the facade + assert _scope_sessions(client, test_workspace.name, scope_a) == [session_name] + + +def test_session_create_rejects_prefixed_scope_names( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + test_workspace, _ = sample_data + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={"id": str(generate_nanoid()), "scopes": ["scope.x"]}, + ) + assert response.status_code == 422 + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={"id": str(generate_nanoid()), "scopes": ["bad name!"]}, + ) + assert response.status_code == 422 + + +async def test_scope_membership_equals_hand_built_observer( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """Facade-less equivalence: a scope membership row is exactly what a + hand-built observer peer would have (name and kind flag aside).""" + test_workspace, _ = sample_data + + # Hand-built observer peer added through the generic session-peer route + observer_name = str(generate_nanoid()) + observer_session = _create_session(client, test_workspace.name) + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{observer_session}/peers", + json={observer_name: {"observe_others": True, "observe_me": False}}, + ) + assert response.status_code == 200 + + # Scope membership added through the facade + 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) + assert ( + _add_sessions(client, test_workspace.name, scope_name, [scope_session]) == 204 + ) + + hand_built = await _get_session_peer( + db_session, test_workspace.name, observer_session, observer_name + ) + via_facade = await _get_session_peer( + db_session, test_workspace.name, scope_session, scope_peer_name(scope_name) + ) + assert hand_built is not None and via_facade is not None + assert hand_built.configuration == via_facade.configuration + assert hand_built.left_at is None and via_facade.left_at is None + + +async def test_scope_peer_observes_ingested_messages( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """End-to-end litmus: after adding a session to a scope, a message from a + real peer fans out a representation task with the scope peer as observer.""" + 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) + + # Add the speaking peer and the scope membership + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={test_peer.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( + workspace_name=test_workspace.name, + session_name=session_name, + peer_name=test_peer.name, + content="I love hiking in the mountains", + public_id=generate_nanoid(), + seq_in_session=1, + token_count=10, + ) + db_session.add(message) + await db_session.commit() + + await enqueue( + [ + { + "workspace_name": test_workspace.name, + "session_name": session_name, + "message_id": message.id, + "content": message.content, + "peer_name": test_peer.name, + "created_at": message.created_at, + "message_public_id": message.public_id, + "seq_in_session": message.seq_in_session, + } + ] + ) + + result = await db_session.execute( + select(QueueItem) + .where(QueueItem.task_type == "representation") + .where(QueueItem.message_id == message.id) + ) + representation_items = list(result.scalars().all()) + assert len(representation_items) == 1 + payload = representation_items[0].payload + assert payload.get("observed") == test_peer.name + observers = payload.get("observers") + assert observers is not None + assert test_peer.name in observers # self-observation + assert scope_peer_name(scope_name) in observers # the scope observes + + +# --------------------------------------------------------------------------- +# Dotted / legacy peer names must not 500 (regression for the PeerCreate-as-DTO +# chokepoint). Names were length-only validated before migration +# d429de0e5338, so pre-existing peers can contain '.' — and re-validating them +# against PeerCreate's charset pattern raised a raw pydantic error, i.e. a 500. +# --------------------------------------------------------------------------- + + +async def test_legacy_dotted_peer_name_is_fully_usable( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """A pre-existing dotted peer name must work end to end, not 500.""" + test_workspace, _ = sample_data + legacy_name = f"alice.smith.{generate_nanoid()}" + + db_session.add(models.Peer(workspace_name=test_workspace.name, name=legacy_name)) + await db_session.commit() + + session_name = _create_session(client, test_workspace.name) + + # Can author messages + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/messages", + json={"messages": [{"peer_id": legacy_name, "content": "hello"}]}, + ) + assert response.status_code in [200, 201], response.text + + # Can be added to a session through the generic route + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={legacy_name: {}}, + ) + assert response.status_code == 200, response.text + + # Can be updated through the generic peer route + response = client.put( + f"/v3/workspaces/{test_workspace.name}/peers/{legacy_name}", + json={"metadata": {"k": "v"}}, + ) + assert response.status_code == 200, response.text + + +async def test_legacy_prefixed_peer_without_flag_is_not_a_scope( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """A peer merely occupying the reserved namespace keeps working. + + Only the name half of the invariant is present, so it is not a scope: it can + still author messages and shows up in the default peers list, while the + scopes facade refuses to treat it as a scope. + """ + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + squatter = scope_peer_name(scope_name) + + db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) + await db_session.commit() + + session_name = _create_session(client, test_workspace.name) + + # Not a scope, so the message-author guard must not fire + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/messages", + json={"messages": [{"peer_id": squatter, "content": "hello"}]}, + ) + assert response.status_code in [200, 201], response.text + + # Visible in the default (non-scope) peers list + response = client.post(f"/v3/workspaces/{test_workspace.name}/peers/list") + assert squatter in [p["id"] for p in response.json()["items"]] + + # But the facade does not recognise it + response = client.get(f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}") + assert response.status_code == 404 + + response = client.post(f"/v3/workspaces/{test_workspace.name}/scopes/list") + assert scope_name not in [s["id"] for s in response.json()["items"]] + + +async def test_forged_configuration_kind_does_not_make_a_scope( + client: TestClient, + sample_data: tuple[Workspace, Peer], +): + """`configuration` is user-writable, so it must not be load-bearing. + + A peer that forges `{"kind": "scope"}` in configuration has neither the + reserved name nor the internal flag, so it stays an ordinary peer. + """ + test_workspace, _ = sample_data + peer_name = str(generate_nanoid()) + + response = client.put( + f"/v3/workspaces/{test_workspace.name}/peers/{peer_name}", + json={"configuration": {"kind": "scope"}}, + ) + assert response.status_code == 200, response.text + + # Still an ordinary peer: present in the default list, absent from scopes + response = client.post(f"/v3/workspaces/{test_workspace.name}/peers/list") + assert peer_name in [p["id"] for p in response.json()["items"]] + + response = client.post(f"/v3/workspaces/{test_workspace.name}/scopes/list") + assert peer_name not in [s["id"] for s in response.json()["items"]] + + # And can still author messages + 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": peer_name, "content": "hello"}]}, + ) + assert response.status_code in [200, 201], response.text + + +def test_internal_metadata_never_in_peer_response( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """The scope flag must not leak into any peer response body.""" + test_workspace, _ = 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/list", json={"kind": "all"} + ) + assert response.status_code == 200 + for peer in response.json()["items"]: + assert "internal_metadata" not in peer + assert "kind" not in peer.get("configuration", {}) + + +async def test_crud_get_peer_resolves_scope_and_dotted_names( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """crud.get_peer must accept names outside RESOURCE_NAME_PATTERN. + + This is the Dreamer's preflight path: DreamScheduler passes + ``collection.observer`` straight through, and scope peers have + ``observe_others=true``, so ``(scope.x, peer)`` collections exist and get + dreamt. While get_peer took a PeerCreate, every such dream died at preflight + on a raw pydantic ValidationError. + """ + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 + + resolved = await crud.get_peer( + db_session, test_workspace.name, scope_peer_name(scope_name) + ) + assert resolved.name == scope_peer_name(scope_name) + assert is_scope_peer(resolved.name, resolved.internal_metadata) + + dotted = f"legacy.dotted.{generate_nanoid()}" + db_session.add(models.Peer(workspace_name=test_workspace.name, name=dotted)) + await db_session.commit() + + resolved = await crud.get_peer(db_session, test_workspace.name, dotted) + assert resolved.name == dotted + # Has neither half of the invariant + assert not is_scope_peer(resolved.name, resolved.internal_metadata) + + +# --------------------------------------------------------------------------- +# Name validation on the create path. PeerSpec carries no charset pattern so +# existing names can be *looked up*, but anything crud is about to INSERT is a +# new peer and must obey the public contract — otherwise request-controlled +# names (message authors, session peer maps) mint squatters. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "bad_name", + ["not a valid name!@#", "has spaces", "emoji-\U0001f600"], +) +def test_message_author_cannot_create_invalid_peer_name( + client: TestClient, sample_data: tuple[Workspace, Peer], bad_name: str +): + """A message author must not be able to create a non-conforming peer.""" + 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": bad_name, "content": "hello"}]}, + ) + assert response.status_code == 422, response.text + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/peers/list", json={"kind": "all"} + ) + assert bad_name 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, + sample_data: tuple[Workspace, Peer], +): + """The last-line guard runs on resolved rows, closing the check-then-upsert race. + + Calls crud directly, bypassing the route-level name check, so the only thing + standing between the caller and a scope membership is the guard on the + resolved peer row — the guard the racing caller would hit. The unflagged → + flagged transition itself is not simulated here. + """ + 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) + + # crud-level: the resolved row is a scope, so membership must be refused even + # though the peer already exists (no create-path validation fires). + with pytest.raises(ValidationException): + await crud.get_or_create_session( + db_session, + session=schemas.SessionCreate( + name=session_name, peers={backing: schemas.SessionPeerConfig()} + ), + workspace_name=test_workspace.name, + ) + + +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 _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", + json={"observe_others": False, "observe_me": True}, + ) + assert response.status_code == 422, response.text + + # Observer semantics intact + membership = await _get_session_peer( + db_session, test_workspace.name, session_name, backing + ) + assert membership is not None + assert membership.configuration == {"observe_me": False, "observe_others": True} + + +def test_session_peers_listing_excludes_scopes( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """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"{base}/sessions/{session_name}/peers", json={test_peer.name: {}} + ).status_code + == 200 + ) + assert _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 + + 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 + ) + + +@pytest.mark.parametrize("bad_name", ["", "a" * 513]) +def test_degenerate_peer_names_are_422_not_500( + client: TestClient, sample_data: tuple[Workspace, Peer], bad_name: str +): + """Empty and over-long author names must not reach PeerSpec and 500. + + Request-bound peer names carry no length bound of their own, so these used to + raise a raw pydantic ValidationError inside crud, which the catch-all handler + turned into 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": bad_name, "content": "hello"}]}, + ) + assert response.status_code == 422, response.text + + +# --------------------------------------------------------------------------- +# 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. +# --------------------------------------------------------------------------- + + +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 _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 + + # 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. + assert _scope_sessions(client, test_workspace.name, scope_name) == [session_name] + + +async def test_empty_replacement_preserves_scope_membership( + client: TestClient, + db_session: AsyncSession, + 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: {}}, + ) + _add_sessions(client, test_workspace.name, scope_name, [session_name]) + + assert ( + client.put( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={}, + ).status_code + == 200 + ) + + 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. + session_peer = await _get_session_peer( + db_session, test_workspace.name, session_name, test_peer.name + ) + assert session_peer is not None + assert session_peer.left_at is not None + + +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" + + +# --------------------------------------------------------------------------- +# 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( + 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 refused dream leaves no queue row behind. + + 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())) + + 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 + + +# --------------------------------------------------------------------------- +# Observer limit. Scope memberships carry observe_others=True but must not +# consume SESSION_OBSERVERS_LIMIT: that would cap scopes-per-session at the +# limit and report it as an observer-shaped 400 through a facade whose whole +# job is hiding observers. +# --------------------------------------------------------------------------- + + +def test_scopes_do_not_count_toward_observer_limit( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """A session can join more scopes than SESSION_OBSERVERS_LIMIT allows observers.""" + test_workspace, _ = sample_data + session_name = _create_session(client, test_workspace.name) + scope_names = [ + str(generate_nanoid()) for _ in range(settings.SESSION_OBSERVERS_LIMIT + 2) + ] + + for scope_name in scope_names: + assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 + assert ( + _add_sessions(client, test_workspace.name, scope_name, [session_name]) + == 204 + ) + + # Every membership is live, and the session-create path agrees. + for scope_name in scope_names: + assert _scope_sessions(client, test_workspace.name, scope_name) == [ + session_name + ] + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={"id": str(generate_nanoid()), "scopes": scope_names}, + ) + assert response.status_code == 201, response.text + + +def test_observer_limit_still_applies_to_real_peers( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """The exclusion is scope-only: real observers are still capped. + + Without this the scope carve-out could quietly disable the limit entirely. + """ + test_workspace, _ = sample_data + 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 _add_sessions(client, test_workspace.name, scope_name, [session_name]) == 204 + + observers = { + str(generate_nanoid()): {"observe_others": True} + for _ in range(settings.SESSION_OBSERVERS_LIMIT + 1) + } + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json=observers, + ) + assert response.status_code == 400, response.text + + +def test_session_create_scopes_list_is_capped( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """`scopes` is bounded like `session_ids` on the add-sessions route. + + Nothing downstream bounds it now that scopes are outside the observer limit, + so an unbounded list would mint a scope peer per element in one request. + """ + test_workspace, _ = sample_data + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={ + "id": str(generate_nanoid()), + "scopes": [str(generate_nanoid()) for _ in range(101)], + }, + ) + assert response.status_code == 422, response.text diff --git a/tests/test_cache_redaction.py b/tests/test_cache_redaction.py index 3a8e6891..805b31c7 100644 --- a/tests/test_cache_redaction.py +++ b/tests/test_cache_redaction.py @@ -2,7 +2,9 @@ import pytest -from src.cache.client import _redact_cache_url # pyright: ignore[reportPrivateUsage] +from src.cache.client import ( + _redact_cache_url, # pyright: ignore[reportPrivateUsage] +) class TestRedactCacheUrl: