diff --git a/src/crud/peer.py b/src/crud/peer.py index 2ea1ac03..4e85d78d 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -28,6 +28,9 @@ 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" @@ -58,6 +61,12 @@ def _validate_new_peer_names(names: list[str]) -> None: scopes_util.validate_no_scope_peer_names( names, action="Use the scopes routes to create scopes." ) + too_long = sorted({n for n in names if len(n) > PEER_NAME_MAX_LENGTH}) + if too_long: + raise ValidationException( + f"Peer name(s) must be at most {PEER_NAME_MAX_LENGTH} characters" + ) + # RESOURCE_NAME_PATTERN's `+` already rejects the empty name. offenders = sorted({n for n in names if not re.fullmatch(RESOURCE_NAME_PATTERN, n)}) if offenders: raise ValidationException( @@ -386,6 +395,17 @@ async def update_peer( ) 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/scope.py b/src/crud/scope.py index 1a2853ee..16ee5ca4 100644 --- a/src/crud/scope.py +++ b/src/crud/scope.py @@ -353,4 +353,6 @@ async def remove_session_from_scope( 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 8d07559d..9a1df3df 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -34,7 +34,7 @@ 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 from .scope import SCOPE_MEMBERSHIP_CONFIG, get_or_create_scopes from .workspace import get_or_create_workspace @@ -822,6 +822,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. @@ -831,16 +833,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. Checked here rather than in the route so it is adjacent + # to the UPDATE below, leaving no window for a concurrently-created scope to + # be silently detached from its sessions. + 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) @@ -1227,10 +1245,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, 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/routers/peers.py b/src/routers/peers.py index b112fd0c..08ea5ecb 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -30,7 +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_name, validate_no_scope_peer_names +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 @@ -138,16 +142,13 @@ async def update_peer( ): """Update a Peer's metadata and/or configuration. - Three-way on the reserved namespace: a real scope is refused here (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 ``get_or_create_peers``' create-path validation rather than - being minted. + 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. """ - await crud.reject_scope_peers( - db, workspace_id, [peer_id], action="Use the scopes routes to manage scopes." - ) updated_peer = await crud.update_peer( db, workspace_name=workspace_id, peer_name=peer_id, peer=peer ) @@ -279,6 +280,15 @@ async def chat( workspace_name=workspace_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() diff --git a/src/schemas/api.py b/src/schemas/api.py index fce81812..3040cee3 100644 --- a/src/schemas/api.py +++ b/src/schemas/api.py @@ -179,9 +179,16 @@ class PeerSpec(PeerBase): 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: Annotated[str, Field(min_length=1, max_length=512)] + name: str metadata: _SanitizedMetadata | None = None configuration: dict[str, Any] | None = None diff --git a/tests/routes/test_scopes.py b/tests/routes/test_scopes.py index ba5cd355..e301ba7f 100644 --- a/tests/routes/test_scopes.py +++ b/tests/routes/test_scopes.py @@ -965,3 +965,109 @@ async def test_resolved_scope_peer_rejected_at_membership_upsert( ), workspace_name=test_workspace.name, ) + + +def test_set_peer_config_cannot_disable_a_scope( + client: TestClient, 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. + """ + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 + backing = scope_peer_name(scope_name) + session_name = _create_session(client, test_workspace.name) + assert ( + client.post( + f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", + json={"session_ids": [session_name]}, + ).status_code + == 200 + ) + + 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 + current = client.get( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers/{backing}/config" + ) + assert current.status_code == 200 + assert current.json() == {"observe_me": False, "observe_others": True} + + +async def test_unflagged_squatter_config_still_settable( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +): + """The set_peer_config guard is flag-based, so a squatter is unaffected.""" + test_workspace, _ = sample_data + squatter = scope_peer_name(str(generate_nanoid())) + db_session.add(models.Peer(workspace_name=test_workspace.name, name=squatter)) + await db_session.commit() + + session_name = _create_session(client, test_workspace.name) + assert ( + client.post( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json={squatter: {}}, + ).status_code + == 200 + ) + + response = client.put( + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers/{squatter}/config", + json={"observe_others": False, "observe_me": True}, + ) + assert response.status_code == 204, response.text + + +def test_generic_session_remove_cannot_detach_a_scope( + client: TestClient, sample_data: tuple[Workspace, Peer] +): + """Scope membership must end through the scopes routes, which reconcile.""" + test_workspace, _ = sample_data + scope_name = str(generate_nanoid()) + assert _create_scope(client, test_workspace.name, scope_name).status_code == 201 + session_name = _create_session(client, test_workspace.name) + assert ( + client.post( + f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions", + json={"session_ids": [session_name]}, + ).status_code + == 200 + ) + + response = client.request( + "DELETE", + f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers", + json=[scope_peer_name(scope_name)], + ) + assert response.status_code == 422, response.text + + +@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