Scopes Phase 2a: scope-kind peers, guardrails, and scopes CRUD routes#884
Open
VVoruganti wants to merge 4 commits into
Open
Scopes Phase 2a: scope-kind peers, guardrails, and scopes CRUD routes#884VVoruganti wants to merge 4 commits into
VVoruganti wants to merge 4 commits into
Conversation
Introduce the scope peer namespace (scope__<name>) 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR introduces reserved scope peers that observe session members without speaking. It adds scope naming utilities, CRUD operations, schemas, API routes, session integration, peer filtering, misuse guardrails, and comprehensive route and end-to-end tests. ChangesScope Peers Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionsRouter
participant SessionCRUD
participant ScopeCRUD
participant Database
Client->>SessionsRouter: Create session with scopes
SessionsRouter->>SessionCRUD: get_or_create_session
SessionCRUD->>ScopeCRUD: get_or_create_scopes
ScopeCRUD->>Database: Create or retrieve scope peers
Database-->>ScopeCRUD: Scope peers
ScopeCRUD-->>SessionCRUD: Scope result
SessionCRUD->>Database: Upsert scope memberships and commit
SessionCRUD-->>Client: Session response
sequenceDiagram
participant Client
participant ScopesRouter
participant ScopeCRUD
participant Database
Client->>ScopesRouter: Add sessions to scope
ScopesRouter->>ScopeCRUD: add_sessions_to_scope
ScopeCRUD->>Database: Validate scope and active sessions
ScopeCRUD->>Database: Upsert observer memberships
Database-->>ScopeCRUD: Updated memberships
ScopeCRUD-->>Client: ScopeSessions response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/crud/scope.py (1)
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelative imports vs. guideline.
from .peer import ...,from .workspace import ..., and the lazyfrom .session import ...imports use relative paths. As per coding guidelines,src/**/*.pyshould "follow isort conventions with absolute imports preferred." If this deviates intentionally to avoid the noted circular-import issue with.session, consider at least converting the non-circular ones (.peer,.workspace) to absolute imports for consistency.Also applies to: 258-258, 313-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/crud/scope.py` around lines 22 - 30, The imports in scope.py are using relative paths where the project prefers absolute imports; update the non-circular imports in get_or_create_scope-related code to use absolute module paths for consistency, while keeping the lazy session import only if needed to avoid the circular dependency. Use the existing symbols peer_cache_key, get_or_create_workspace, and the lazy session import sites as the locations to convert the import style without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crud/scope.py`:
- Around line 90-139: The cache invalidation list in get_or_create_scopes is
being derived from mutable ORM objects, so a retry can lose peers that need
purging after h_metadata is updated. In src/crud/scope.py, snapshot the peers or
cache keys to invalidate before mutating existing_peer.h_metadata, and build
_cache_keys_to_invalidate from that stable snapshot rather than changed_peers
alone. Keep the retry path in get_or_create_scopes from recomputing invalidation
candidates from mutated state so previously changed peers still trigger
safe_cache_delete.
In `@src/routers/sessions.py`:
- Around line 42-46: The route example in _SCOPES_ROUTE_GUIDANCE is missing the
/v3 prefix, so update the guidance text in sessions.py to match how
scopes.router is mounted in main.py. Make sure the documented sessions route
path includes /v3/workspaces/{workspace_id}/scopes/{scope_id}/sessions so
callers copy the correct API URL.
---
Nitpick comments:
In `@src/crud/scope.py`:
- Around line 22-30: The imports in scope.py are using relative paths where the
project prefers absolute imports; update the non-circular imports in
get_or_create_scope-related code to use absolute module paths for consistency,
while keeping the lazy session import only if needed to avoid the circular
dependency. Use the existing symbols peer_cache_key, get_or_create_workspace,
and the lazy session import sites as the locations to convert the import style
without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21fbacb6-6497-4301-a605-207096c6f8c5
📒 Files selected for processing (13)
src/crud/__init__.pysrc/crud/message.pysrc/crud/peer.pysrc/crud/scope.pysrc/crud/session.pysrc/main.pysrc/routers/peers.pysrc/routers/scopes.pysrc/routers/sessions.pysrc/schemas/__init__.pysrc/schemas/api.pysrc/utils/scopes.pytests/routes/test_scopes.py
Comment on lines
+90
to
+139
| changed_peers: list[models.Peer] = [] | ||
| for existing_peer in existing_peers: | ||
| if not is_scope_peer_configuration(existing_peer.configuration): | ||
| 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 {}, | ||
| 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 | ||
| return await get_or_create_scopes(db, workspace_name, scopes, _retry=True) | ||
|
|
||
| _cache_keys_to_invalidate = [ | ||
| peer_cache_key(workspace_name, 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, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first
ast-grep outline src/crud/scope.py --view expanded
# Read the relevant section with line numbers
sed -n '1,240p' src/crud/scope.py | cat -n
# Find the result type and cache invalidation plumbing
rg -n "class GetOrCreateResult|on_commit|safe_cache_delete|peer_cache_key|get_or_create_scopes" src -SRepository: plastic-labs/honcho
Length of output: 13605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read GetOrCreateResult
sed -n '232,270p' src/utils/types.py | cat -n
# Read the analogous peer helper for the same retry pattern
sed -n '90,160p' src/crud/peer.py | cat -n
# Find call sites that may trigger scope-cache invalidation elsewhere
rg -n "get_or_create_scopes\(|peer_cache_key\(workspace_name, p.name\)|post_commit\(" src -SRepository: plastic-labs/honcho
Length of output: 5397
Preserve invalidation candidates across the retry path The ORM instance is mutated before the nested insert, so a retry can see the same in-memory metadata and drop that peer from changed_peers, letting the update commit without invalidating its cache key. Snapshot the names to invalidate before mutating the objects, or keep a separate set of invalidation keys, so retry can’t lose a purge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crud/scope.py` around lines 90 - 139, The cache invalidation list in
get_or_create_scopes is being derived from mutable ORM objects, so a retry can
lose peers that need purging after h_metadata is updated. In src/crud/scope.py,
snapshot the peers or cache keys to invalidate before mutating
existing_peer.h_metadata, and build _cache_keys_to_invalidate from that stable
snapshot rather than changed_peers alone. Keep the retry path in
get_or_create_scopes from recomputing invalidation candidates from mutated state
so previously changed peers still trigger safe_cache_delete.
Comment on lines
+42
to
+46
| _SCOPES_ROUTE_GUIDANCE = ( | ||
| "Scope membership is managed via the scopes routes " | ||
| "(/workspaces/{workspace_id}/scopes/{scope_id}/sessions) or the `scopes` " | ||
| "field at session creation." | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error message path is missing the /v3 API prefix.
The route example in _SCOPES_ROUTE_GUIDANCE reads /workspaces/{workspace_id}/scopes/{scope_id}/sessions, but scopes.router is actually mounted under /v3 in src/main.py (app.include_router(scopes.router, prefix="/v3")). Callers copying this hint verbatim will hit a 404.
💚 Proposed fix
_SCOPES_ROUTE_GUIDANCE = (
"Scope membership is managed via the scopes routes "
- "(/workspaces/{workspace_id}/scopes/{scope_id}/sessions) or the `scopes` "
+ "(/v3/workspaces/{workspace_id}/scopes/{scope_id}/sessions) or the `scopes` "
"field at session creation."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _SCOPES_ROUTE_GUIDANCE = ( | |
| "Scope membership is managed via the scopes routes " | |
| "(/workspaces/{workspace_id}/scopes/{scope_id}/sessions) or the `scopes` " | |
| "field at session creation." | |
| ) | |
| _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." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routers/sessions.py` around lines 42 - 46, The route example in
_SCOPES_ROUTE_GUIDANCE is missing the /v3 prefix, so update the guidance text in
sessions.py to match how scopes.router is mounted in main.py. Make sure the
documented sessions route path includes
/v3/workspaces/{workspace_id}/scopes/{scope_id}/sessions so callers copy the
correct API URL.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Foundation of the Scopes feature (visibility boundaries within a peer, built on observer peers behind a facade). A scope is a named grouping of sessions; under the hood a scope named
therapyis a peer namedscope__therapycarrying the authoritative{"kind": "scope", "observe_me": false}configuration flag, which observes its member sessions (observe_others=true, observe_me=false) and never speaks. Developers manage scopes exclusively through the new routes (and thescopesfield at session creation) and never see the observer/observed mechanics.src/utils/scopes.pyis the single source of truth for the reserved prefix, the kind flag, and the name helpers (scope_peer_name,is_scope_peer_name,scope_name_from_peer).src/crud/scope.pyimplements the facade: scope-peer create-or-get (legacy peers occupying the reserved name without the kind flag are refused with 409, never adopted), membership add/list/remove reusing the session-peer upsert / soft-delete paths.SessionCreategains optionalscopes: list[str]— the no-backfill common path: each scope is created-or-gotten and the observer membership added at session creation.Guardrails
crud.create_messages, covers both message routes)target(path-level observer is Phase 2b)peers.listexcludes scope peers by default;PeerGet.kind="scope"/"all"switches the view (JSONBconfiguration @> '{"kind": "scope"}'filter)peersmapping) reject scope peers, directing to the scopes routesscope__) namesRoutes
All under
/v3/workspaces/{workspace_id}/scopes, gated byrequire_auth(workspace_name=...)— workspace-level key required; peer- and session-scoped keys get 401.""{id, metadata?}; response{id (unprefixed), metadata, created_at}/list/{scope_id}/{scope_id}/sessions{session_ids: [...]}); sessions must exist (404)/{scope_id}/sessions/{session_id}/{scope_id}/sessionssession_peersLitmus test
tests/routes/test_scopes.py::test_scope_peer_observes_ingested_messagesproves the end-to-end observer semantics: after creating a scope and adding a session, a message ingested from a real peer fans out a representation queue item whoseobserverslist includes the scope peer.test_scope_membership_equals_hand_built_observeradditionally asserts the membership row is byte-identical in configuration to a hand-built observer peer — facade-less equivalence.Follow-ups
{scope}resolution is DEV-1998.Fixes DEV-1997 (part of DEV-1970 Scopes RFC)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
kindoption.Bug Fixes