add copilot dashboard session - #960
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change adds Copilot scope request schemas and resolution, verifies merchant and template authorization, computes date windows, and injects server-owned scope metadata into chat sessions while rejecting client attempts to override reserved metadata. ChangesBuddy Copilot scope flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatSessionHandler
participant ScopeResolver
participant TemplateAccessor
participant SessionPersistence
Client->>ChatSessionHandler: Create session with copilot_scope
ChatSessionHandler->>ScopeResolver: resolve_copilot_scope(request, current_user)
ScopeResolver->>TemplateAccessor: get_template_merchant_id(template_id)
TemplateAccessor-->>ScopeResolver: template merchant_id
ScopeResolver-->>ChatSessionHandler: resolved CopilotScope
ChatSessionHandler->>SessionPersistence: persist merged session metadata
SessionPersistence-->>ChatSessionHandler: created chat session
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a “Buddy Copilot” data-scope contract that can be requested by the dashboard when creating a normal Breeze Buddy chat session, with server-side validation and persistence into chat_session.metadata for downstream tool/prompt use.
Changes:
- Adds Copilot scope request/response schemas and a scope resolver service (permissions + merchant/template authorization + timezone-aware date window).
- Extends chat session creation to (a) protect server-owned metadata namespaces and (b) resolve/persist Copilot scope under
metadata.copilot. - Adds a lightweight “template → merchant_id” DB query/accessor and comprehensive unit tests for scope resolution + session metadata behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/api/routers/breeze_buddy/chat/handlers.py |
Validates reserved metadata keys and optionally resolves/persists Copilot scope during chat session creation. |
app/schemas/breeze_buddy/chat.py |
Extends CreateChatSessionRequest with optional copilot_scope. |
app/schemas/breeze_buddy/copilot.py |
Introduces Copilot scope schemas and session-metadata serialization helper. |
app/services/breeze_buddy/copilot/scope.py |
Implements server-authoritative scope resolution + validation logic. |
app/services/breeze_buddy/copilot/__init__.py |
Exposes Copilot scope resolver symbols for import convenience. |
app/database/queries/breeze_buddy/template.py |
Adds a query to fetch only merchant_id for a template. |
app/database/accessor/breeze_buddy/template.py |
Adds accessor get_template_merchant_id() used by scope validation to avoid loading full templates. |
tests/test_copilot_session.py |
Tests session creation behavior, server-owned metadata protection, and Copilot metadata persistence. |
tests/test_copilot_scope.py |
Tests scope resolver behavior (permissions, merchant/template authorization, date window normalization, timezone errors). |
app/services/breeze_buddy/__init__.py |
Adds package docstring for Breeze Buddy services namespace. |
a15cc9f to
9da33e0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/schemas/breeze_buddy/copilot.py (2)
38-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a type hint for
info.
infoinvalidate_orderlacks a type annotation; usepydantic.ValidationInfo.As per coding guidelines, "Include required type hints on all function signatures."✏️ Proposed fix
-from pydantic import BaseModel, ConfigDict, Field, computed_field, field_validator +from pydantic import ( + BaseModel, + ConfigDict, + Field, + ValidationInfo, + computed_field, + field_validator, +) @@ - def validate_order(cls, value: date, info) -> date: + def validate_order(cls, value: date, info: ValidationInfo) -> date:🤖 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 `@app/schemas/breeze_buddy/copilot.py` around lines 38 - 44, Update the validate_order method signature to annotate info with pydantic.ValidationInfo, importing ValidationInfo through the project’s existing Pydantic import style. Preserve the validator’s current date-order validation behavior.Source: Coding guidelines
106-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
labelhardcodes "Last 7 days", decoupled from the actual window size.The DEFAULT-source label is a fixed string rather than derived from
date_from/date_to. If the default window size in_resolve_date_window(scope.py) ever changes, this label will silently go stale.🤖 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 `@app/schemas/breeze_buddy/copilot.py` around lines 106 - 122, The CopilotDateWindow.label property hardcodes the DEFAULT label instead of reflecting the resolved dates. Update label to derive its DEFAULT-source text from date_from and date_to (or the shared default-window configuration), so it remains accurate if _resolve_date_window changes; preserve the existing explicit-date label format.
🤖 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.
Nitpick comments:
In `@app/schemas/breeze_buddy/copilot.py`:
- Around line 38-44: Update the validate_order method signature to annotate info
with pydantic.ValidationInfo, importing ValidationInfo through the project’s
existing Pydantic import style. Preserve the validator’s current date-order
validation behavior.
- Around line 106-122: The CopilotDateWindow.label property hardcodes the
DEFAULT label instead of reflecting the resolved dates. Update label to derive
its DEFAULT-source text from date_from and date_to (or the shared default-window
configuration), so it remains accurate if _resolve_date_window changes; preserve
the existing explicit-date label format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78ec55e8-46ce-4703-b000-90d16be0b19b
📒 Files selected for processing (10)
app/api/routers/breeze_buddy/chat/handlers.pyapp/database/accessor/breeze_buddy/template.pyapp/database/queries/breeze_buddy/template.pyapp/schemas/breeze_buddy/chat.pyapp/schemas/breeze_buddy/copilot.pyapp/services/breeze_buddy/__init__.pyapp/services/breeze_buddy/copilot/__init__.pyapp/services/breeze_buddy/copilot/scope.pytests/test_copilot_scope.pytests/test_copilot_session.py
868228f to
93ea20c
Compare
dbd5844 to
9166288
Compare
|
Reviewed at head This directly fixes the actor-info-leak the Copilot review flagged inline on Also checked: No blockers from me — worth clicking the Copilot thread resolved since the fix it asked for is already in this diff. |
9166288 to
48f452e
Compare
48f452e to
acfb8c4
Compare
murdore
left a comment
murdore
left a comment
There was a problem hiding this comment.
Reviewed at acfb8c42c6fd. Traced the authorization path end to end rather than reading the summary — this is a data-scoping change on merchant data, so the question I cared about was whether a caller can bind a session to a merchant they don't own, or keep access after losing it.
No findings. This is a careful piece of work, and a few of the choices are ones I see missed far more often than not:
Re-validation happens on every access, not just at creation. validate_persisted_copilot_scope_access is wired into all six session operations — get_session, send_message, approve_tool, cancel_turn, end_session, get_transcript — via _validate_chat_and_copilot_session_access. Nothing is left on the old validate_chat_session_access-only path. That means a user whose merchant access is revoked loses access to sessions they previously created, which is the property that gets skipped when scope is authorized once and then trusted forever.
The 403/404 split is deliberate and correct. At creation, _validate_data_template and _resolve_data_merchant_id return 403 — the caller named the merchant, so telling them it's denied leaks nothing. On resume, every path in validate_persisted_copilot_scope_access passes status_code=404, and _hidden_scope_error collapses that to a bare "Chat session not found". So an attacker holding a guessed session_id can't distinguish "exists but you lack merchant access" from "doesn't exist". That asymmetry looks like an inconsistency at a glance and is actually the point.
Clients can no longer plant a scope. _validate_client_metadata rejects template_vars and COPILOT_SCOPE_METADATA_KEY outright. Without that, the re-validation above would be defeated by a caller supplying their own metadata.copilot — the two changes only work as a pair, and both are here.
Persisted metadata is treated as untrusted on read. _load_persisted_data_scope and the merchant/template extraction check isinstance, strip, and reject empty strings before use, raising invalid_persisted_scope rather than propagating a malformed value into an authorization decision.
The one thing I wanted to confirm was allowed_merchant_ids is None meaning "unrestricted", since a resolver that returns None on an unexpected path would be a total bypass at scope.py:283-286. It holds: resolve_merchant_ids (app/core/security/scope.py:111) returns None only for UserRole.ADMIN and for an owner chain terminating at an admin owner; a non-wildcard user gets a concrete list (and [] when empty, which fails closed since x not in [] is always true); and resellers-with-wildcard are explicitly excluded, with a comment saying why:
# Reseller with wildcard → resolve to merchants they OWN.
# Never return None for resellers — they should only see their own merchants,
# not every merchant in the system.Both call sites treat None identically, so there's no asymmetry between the create path and the resume path either.
One compatibility note, not a defect. _validate_client_metadata changes behaviour for existing clients. Previously create_chat_session_handler did:
persisted_metadata = {
**(req.metadata or {}),
"template_vars": transformed_template_vars,
}— a client sending metadata.template_vars had it silently overwritten. Now it gets a 422 metadata.template_vars is server-owned. Failing loudly is the right call and I'd keep it, but it is a breaking change for any caller currently sending that key and getting away with it. Worth a line in the release note, or a quick check of whether anything in Loom/dashboard does.
Approving.
Summary by CodeRabbit
New Features
Bug Fixes