diff --git a/.cursor/rules/api-auth-and-qc.mdc b/.cursor/rules/api-auth-and-qc.mdc new file mode 100644 index 00000000..dcb9d834 --- /dev/null +++ b/.cursor/rules/api-auth-and-qc.mdc @@ -0,0 +1,39 @@ +--- +description: Auth provider pairing and user-scoped vs playlist-scoped API authorization. +globs: backend/src/**/*.py,frontend/packages/**/* +alwaysApply: false +--- + +# API auth and Note QC + +## Match frontend and backend auth in every environment + +Local dev uses noop auth; GCP production uses Google OAuth. Both sides must agree or you will see 401/403 on protected routes. + +| Environment | Backend `AUTH_PROVIDER` | Frontend `VITE_AUTH_PROVIDER` | +|---------------|-------------------------|-------------------------------| +| Local (default) | `none` | `none` | +| GCP / production | `google` | `google` (baked at Docker build; see `frontend/Dockerfile`) | + +Also set `GOOGLE_CLIENT_ID` (backend) and `VITE_GOOGLE_CLIENT_ID` (frontend build) to the **same** OAuth client. The GCP deploy workflow passes the secret into the frontend image build. + +## Two authorization patterns + +**User-scoped** (`/users/{user_email}/…`): settings, QC check definitions. The path `user_email` must match the authenticated user. Use `emails_match()` from `dna.auth.email` (case-insensitive); do not use raw `!=`. + +**Playlist / draft-scoped** (`/playlists/…`, `publish-notes`, `run-qc-checks`): any authenticated user may act on drafts in the playlist. Do **not** require `body.user_email == current_user` on `run-qc-checks`. + +## Note QC: `user_email` means draft owner + +- **CRUD** `/users/{user_email}/qc-checks`: `user_email` is the signed-in user configuring their checks. +- **Run** `POST …/run-qc-checks` with `{ "user_email": "…" }`: identifies which **draft note** to check (owner), same as draft-note paths. The publish dialog runs QC for every selected draft; the caller’s token email may differ. + +Frontend: `useNoteQCChecks` correctly passes `d.user_email` from each `DraftNote`. Settings / `NoteQCTab` use the auth user’s email. + +## Debugging 403 Forbidden on QC + +1. **Settings tab** (`GET /users/…/qc-checks`): path email ≠ token email (check case; use `emails_match` on the backend). +2. **Publish dialog** (`run-qc-checks`): caller was wrongly required to own the draft — fixed by playlist-scoped auth above. +3. **401** (not 403): missing Bearer token, wrong `AUTH_PROVIDER`, or invalid Google token / client ID mismatch. + +When adding endpoints, copy the pattern from `publish_notes` (auth only) or user settings (auth + `emails_match`), never invent a third rule without documenting it here. diff --git a/backend/src/dna/auth/email.py b/backend/src/dna/auth/email.py new file mode 100644 index 00000000..dc01abc5 --- /dev/null +++ b/backend/src/dna/auth/email.py @@ -0,0 +1,6 @@ +"""Email comparison helpers for authorization checks.""" + + +def emails_match(a: str, b: str) -> bool: + """Return True when two emails refer to the same mailbox (case-insensitive).""" + return a.strip().lower() == b.strip().lower() diff --git a/backend/src/dna/llm_providers/llm_provider_base.py b/backend/src/dna/llm_providers/llm_provider_base.py index 73cf8734..017372dd 100644 --- a/backend/src/dna/llm_providers/llm_provider_base.py +++ b/backend/src/dna/llm_providers/llm_provider_base.py @@ -251,6 +251,7 @@ async def generate_structured_with_tools( max_iterations: int = 5, temperature: float = 0.2, max_tool_result_chars: int = DEFAULT_MAX_TOOL_RESULT_CHARS, + extraction_user_message: str | None = None, ) -> T: """Tool-use phase then instructor-validated structured extraction.""" messages: list[dict[str, Any]] = [ @@ -311,7 +312,8 @@ async def generate_structured_with_tools( extraction_messages.append( { "role": "user", - "content": ( + "content": extraction_user_message + or ( "Provide your final quality-check result for this draft and check. " "Fill every required field in the structured response schema." ), diff --git a/backend/src/dna/models/qc_check.py b/backend/src/dna/models/qc_check.py index d83a779d..82e3782f 100644 --- a/backend/src/dna/models/qc_check.py +++ b/backend/src/dna/models/qc_check.py @@ -46,27 +46,67 @@ class NoteQCCheck(BaseModel): class NoteQCAttributeSuggestion(BaseModel): """Suggested updates to draft note metadata.""" - to: Optional[str] = None - cc: Optional[str] = None - subject: Optional[str] = None + to: Optional[str] = Field( + default=None, + description=( + "Primary recipients when the note is directed at specific users. " + 'JSON string of a JSON array: [{"type":"User","id":123,"name":"Name"}]. ' + "Resolve users via search_entities before suggesting." + ), + ) + cc: Optional[str] = Field( + default=None, + description=( + "Users who should see the note but are not primary To recipients " + "(mentioned in passing, stakeholders). Same JSON array string format as to. " + "Do not duplicate users already in to." + ), + ) + subject: Optional[str] = Field( + default=None, + description="Suggested subject line; only when the check implies it should change.", + ) version_status: Optional[str] = Field( default=None, description="Suggested version status (maps to draft version_status).", ) - links: Optional[list[DraftNoteLink]] = None + links: Optional[list[DraftNoteLink]] = Field( + default=None, + description=( + "Non-user entity links (Shot, Asset, Version, Task, Playlist). " + "Do not put users here—use to or cc for users." + ), + ) class NoteQCLLMOutput(BaseModel): """Structured QC verdict returned by the LLM (instructor-validated).""" - passed: bool - issue: Optional[str] = None - evidence: Optional[str] = None + passed: bool = Field( + description="True only if the check passes and no draft changes are needed.", + ) + issue: Optional[str] = Field( + default=None, + description="What failed; required when passed is false.", + ) + evidence: Optional[str] = Field( + default=None, + description="Quote or reference from transcript/draft supporting the issue.", + ) note_suggestion: Optional[str] = Field( default=None, - description="Full suggested note body when the check fails.", + description=( + "Full corrected note body when content must change. Use @[Name](type:id) " + "for mentions (e.g. user:484). Omit when only metadata changes." + ), + ) + attribute_suggestion: Optional[NoteQCAttributeSuggestion] = Field( + default=None, + description=( + "Metadata fixes only: include fields that differ from the current draft. " + "Use to for direct addressees, cc for other users, links for non-user entities." + ), ) - attribute_suggestion: Optional[NoteQCAttributeSuggestion] = None class NoteQCResult(BaseModel): @@ -97,9 +137,12 @@ class RunQCChecksResponse(BaseModel): DEFAULT_ACTION_ITEM_CHECK = NoteQCCheckCreate( name="Action Item Check", prompt=( - "Review the transcript and note below. " - "If the transcript mentions an action item, task, or decision that is NOT " - "reflected in the note, report it. Otherwise respond with passed=true." + "Review the transcript and draft note. If the transcript mentions an action " + "item, task, owner, or decision that is missing or unclear in the note, fail " + "the check. Suggest a corrected note body that includes the missing items. " + "If someone is assigned the action, add them to To; if others are only " + "mentioned for visibility, add them to Cc. The check only passes when the note " + "fully captures all action items and owners from the transcript." ), severity="warning", enabled=True, diff --git a/backend/src/dna/qc/qc_prompt.py b/backend/src/dna/qc/qc_prompt.py new file mode 100644 index 00000000..7ed0b37a --- /dev/null +++ b/backend/src/dna/qc/qc_prompt.py @@ -0,0 +1,110 @@ +"""System and extraction prompts for note quality checks.""" + +from __future__ import annotations + +QC_EXTRACTION_USER_MESSAGE = """\ +Provide your final quality-check result for this draft and check. + +When passed is false, include actionable suggestions that resolve every issue \ +described in the check instructions: +- Use note_suggestion for the full corrected note body when content must change. +- Use attribute_suggestion only for metadata fields that must change (to, cc, \ +links, subject, version_status). Omit fields that should stay as they are. +- Resolve users and entities with search_entities / get_entity before suggesting \ +ids in links or recipient lists. + +When passed is true, leave note_suggestion and attribute_suggestion null.\ +""" + +QC_SYSTEM_INSTRUCTIONS = """\ +You are a quality-check assistant for VFX dailies notes. Your job is to \ +evaluate the current draft against the check instructions in the user message, \ +then propose concrete fixes when the check fails. + +## Goal + +When the check fails, return suggestions the author can apply to resolve every \ +issue found. Do not only describe problems—supply corrected content and metadata \ +where needed. When the check passes, set passed=true and omit suggestions. + +## Inputs + +- Version context: shot/task/version metadata for the note being published. +- Transcript: spoken dailies discussion (primary source for missing facts). +- Current draft note (JSON): content, subject, to, cc, links, version_status. + +Use the transcript, the current draft note, and the version context together with the check instructions. \ +You may call search_entities and get_entity to resolve production-tracker \ +users, shots, assets, versions, tasks, and playlists before suggesting ids. + +## How to choose which fields to change + +Infer which draft fields need updates from the issue type. Only suggest fields \ +that actually change. + +**note_suggestion (note body / content)** +- Use for wording, missing action items, decisions, feedback, or @-mentions in \ +the note text. +- Return the full proposed note body (not a diff). Preserve correct existing \ +text; fix or extend what the check targets. +- User mentions in the body use: @[Display Name](type:id) with lowercase type \ +and numeric id, e.g. @[Jane Doe](user:484). Resolve users via tools first. + +**attribute_suggestion.to** +- Primary recipients: people the note is directed to or who own the action \ +("tell Maya to…", "John needs to fix…", direct feedback to one person). +- Value: JSON string of a JSON array of User objects, each \ +{"type":"User","id":,"name":""}. Include existing To \ +recipients unless the check requires replacing them. + +**attribute_suggestion.cc** +- Secondary visibility: people mentioned in the transcript or note who should \ +see the note but are not the primary addressee (mentioned in passing, \ +stakeholders, "loop in" / "cc" intent). +- Do not put users in cc when they should be To (direct addressee). Do not \ +duplicate the same user in both to and cc. +- Same JSON array string format as to. + +**attribute_suggestion.links** +- Non-user production references: shots, assets, versions, tasks, playlists. +- Do not put users in links; users belong in to or cc. +- Each item: {"entity_type":"","entity_id":,"entity_name":""} \ +where entity_type matches DNA types (e.g. Shot, Asset, Version, Task, Playlist). +- Also reference linked entities in the note body with @[Name](type:id) when \ +appropriate. + +**attribute_suggestion.subject** +- Only when the check or transcript implies the subject line is wrong or incomplete. + +**attribute_suggestion.version_status** +- Only when the check or transcript implies the version status should change. + +## Response rules + +- passed: true only if the check is satisfied and no fixes are required. +- issue: short summary of what failed (null if passed). +- evidence: brief quote or reference from transcript and/or draft (null if passed). +- When passed is false, provide at least one of note_suggestion or \ +attribute_suggestion with fields that differ from the current draft. +- Prefer attribute_suggestion for routing (to/cc/links) and note_suggestion for \ +prose; use both when the check requires both. +- Do not embed a full JSON note inside note_suggestion unless you also split \ +fields correctly; prefer separate note_suggestion and attribute_suggestion. +""" + + +def build_qc_system_prompt( + version_context: str, + transcript_text: str, + draft_json: str, +) -> str: + """Assemble the QC system prompt with runtime context.""" + return ( + f"{QC_SYSTEM_INSTRUCTIONS}\n\n" + "--- Version context ---\n" + f"{version_context}\n\n" + "--- Transcript ---\n" + f"{transcript_text}\n\n" + "--- Current draft note (JSON) ---\n" + f"{draft_json}\n" + ) diff --git a/backend/src/dna/qc/qc_runner.py b/backend/src/dna/qc/qc_runner.py index 3e1125c9..d4bc4417 100644 --- a/backend/src/dna/qc/qc_runner.py +++ b/backend/src/dna/qc/qc_runner.py @@ -14,6 +14,7 @@ NoteQCResult, ) from dna.prodtrack_providers.prodtrack_provider_base import ProdtrackProviderBase +from dna.qc.qc_prompt import QC_EXTRACTION_USER_MESSAGE, build_qc_system_prompt QC_TOOL_DEFINITIONS: list[dict[str, Any]] = [ { @@ -134,18 +135,7 @@ async def _run_one_check( if isinstance(pid, int): project_id = pid - system_prompt = ( - "You are a quality-check assistant for VFX dailies notes. " - "Use the provided version context, transcript, and draft note. " - "Follow the user's check instructions in the user message. " - "You may call tools to look up production tracker entities when needed.\n\n" - "--- Version context ---\n" - f"{version_context}\n\n" - "--- Transcript ---\n" - f"{transcript_text}\n\n" - "--- Current draft note (JSON) ---\n" - f"{draft_json}\n" - ) + system_prompt = build_qc_system_prompt(version_context, transcript_text, draft_json) user_message = f"Check name: {check.name}\n\nCheck instructions:\n{check.prompt}" tool_executor = _make_tool_executor(prodtrack_provider, project_id) @@ -158,6 +148,7 @@ async def _run_one_check( response_model=NoteQCLLMOutput, max_iterations=5, temperature=0.2, + extraction_user_message=QC_EXTRACTION_USER_MESSAGE, ) except Exception as exc: # pragma: no cover - network/provider errors return _failed_run_result( diff --git a/backend/src/main.py b/backend/src/main.py index d0920220..b50de3b4 100644 --- a/backend/src/main.py +++ b/backend/src/main.py @@ -21,6 +21,7 @@ from fastapi.responses import FileResponse from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer +from dna.auth.email import emails_match from dna.auth_providers.auth_provider_base import AuthProviderBase, get_auth_provider from dna.cors_settings import get_cors_middleware_kwargs from dna.events import EventType, get_event_publisher @@ -1297,7 +1298,7 @@ async def get_user_settings( current_user: CurrentUserDep, ) -> UserSettingsResponse: """Get user settings.""" - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") stored = await provider.get_user_settings(user_email) if stored is None: @@ -1319,7 +1320,7 @@ async def upsert_user_settings( current_user: CurrentUserDep, ) -> UserSettingsResponse: """Create or update user settings.""" - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") updated = await provider.upsert_user_settings(user_email, data) return _user_settings_to_response(updated) @@ -1338,7 +1339,7 @@ async def delete_user_settings( current_user: CurrentUserDep, ) -> bool: """Delete user settings.""" - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") deleted = await provider.delete_user_settings(user_email) if not deleted: @@ -1357,7 +1358,7 @@ async def list_qc_checks( storage_provider: StorageProviderDep, current_user: CurrentUserDep, ) -> list[NoteQCCheck]: - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") return await storage_provider.get_qc_checks(user_email) @@ -1375,7 +1376,7 @@ async def create_qc_check( storage_provider: StorageProviderDep, current_user: CurrentUserDep, ) -> NoteQCCheck: - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") return await storage_provider.create_qc_check(user_email, data) @@ -1393,7 +1394,7 @@ async def update_qc_check( storage_provider: StorageProviderDep, current_user: CurrentUserDep, ) -> NoteQCCheck: - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") updated = await storage_provider.update_qc_check(user_email, check_id, data) if updated is None: @@ -1413,7 +1414,7 @@ async def delete_qc_check( storage_provider: StorageProviderDep, current_user: CurrentUserDep, ) -> None: - if user_email != current_user: + if not emails_match(user_email, current_user): raise HTTPException(status_code=403, detail="Forbidden") deleted = await storage_provider.delete_qc_check(user_email, check_id) if not deleted: @@ -1435,8 +1436,8 @@ async def run_qc_checks( llm_provider: LLMProviderDep, current_user: CurrentUserDep, ) -> RunQCChecksResponse: - if body.user_email != current_user: - raise HTTPException(status_code=403, detail="Forbidden") + # Authenticated callers may QC any draft in the playlist (same as publish-notes). + # body.user_email identifies the draft owner, not the caller. draft = await storage_provider.get_draft_note( body.user_email, playlist_id, version_id ) diff --git a/backend/tests/test_auth_email.py b/backend/tests/test_auth_email.py new file mode 100644 index 00000000..b2d172d6 --- /dev/null +++ b/backend/tests/test_auth_email.py @@ -0,0 +1,12 @@ +"""Tests for email authorization helpers.""" + +from dna.auth.email import emails_match + + +def test_emails_match_case_insensitive(): + assert emails_match("Test@Example.com", "test@example.com") + assert emails_match(" user@corp.com ", "user@corp.com") + + +def test_emails_match_different_addresses(): + assert not emails_match("a@example.com", "b@example.com") diff --git a/backend/tests/test_qc_checks.py b/backend/tests/test_qc_checks.py index 08bea0de..65171961 100644 --- a/backend/tests/test_qc_checks.py +++ b/backend/tests/test_qc_checks.py @@ -190,9 +190,14 @@ def test_run_qc_checks_with_draft( finally: app.dependency_overrides.clear() - def test_run_qc_checks_forbidden_user_mismatch( + def test_run_qc_checks_allows_other_users_draft( self, mock_storage, mock_prodtrack, mock_llm, auth_client: TestClient ): + other_draft = _sample_draft() + other_draft.user_email = "other@example.com" + mock_storage.get_draft_note.return_value = other_draft + mock_storage.get_qc_checks.return_value = [_sample_check()] + mock_storage.get_segments_for_version.return_value = [] app.dependency_overrides[get_storage_provider_cached] = lambda: mock_storage app.dependency_overrides[get_prodtrack_provider_cached] = lambda: mock_prodtrack app.dependency_overrides[get_llm_provider_cached] = lambda: mock_llm @@ -201,7 +206,19 @@ def test_run_qc_checks_forbidden_user_mismatch( "/playlists/1/versions/10/run-qc-checks", json={"user_email": "other@example.com"}, ) - assert r.status_code == 403 + assert r.status_code == 200 + mock_storage.get_qc_checks.assert_called_once_with("other@example.com") + finally: + app.dependency_overrides.clear() + + def test_list_qc_checks_case_insensitive_email( + self, mock_storage, auth_client: TestClient + ): + mock_storage.get_qc_checks.return_value = [_sample_check()] + app.dependency_overrides[get_storage_provider_cached] = lambda: mock_storage + try: + r = auth_client.get("/users/Test%40Example.com/qc-checks") + assert r.status_code == 200 finally: app.dependency_overrides.clear() diff --git a/backend/tests/test_qc_prompt.py b/backend/tests/test_qc_prompt.py new file mode 100644 index 00000000..596a3ae1 --- /dev/null +++ b/backend/tests/test_qc_prompt.py @@ -0,0 +1,28 @@ +"""Tests for QC system prompt assembly.""" + +from dna.qc.qc_prompt import ( + QC_EXTRACTION_USER_MESSAGE, + QC_SYSTEM_INSTRUCTIONS, + build_qc_system_prompt, +) + + +def test_build_qc_system_prompt_includes_context_blocks(): + prompt = build_qc_system_prompt("Version: v1", "Speaker: hello", '{"content":""}') + assert QC_SYSTEM_INSTRUCTIONS in prompt + assert "Version: v1" in prompt + assert "Speaker: hello" in prompt + assert '{"content":""}' in prompt + + +def test_qc_system_instructions_cover_field_routing(): + assert "attribute_suggestion.to" in QC_SYSTEM_INSTRUCTIONS + assert "attribute_suggestion.cc" in QC_SYSTEM_INSTRUCTIONS + assert "attribute_suggestion.links" in QC_SYSTEM_INSTRUCTIONS + assert "note_suggestion" in QC_SYSTEM_INSTRUCTIONS + + +def test_qc_extraction_user_message_requires_suggestions_on_fail(): + assert "passed is false" in QC_EXTRACTION_USER_MESSAGE + assert "note_suggestion" in QC_EXTRACTION_USER_MESSAGE + assert "attribute_suggestion" in QC_EXTRACTION_USER_MESSAGE