Add Supabase authentication and Railway production deployment - #4
CodeWithMoin wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughDocuLens now supports adaptive, evidence-grounded investigations with LangGraph orchestration, citation validation, durable checkpoints, SSE streaming, Supabase authentication, configurable document storage, lightweight extraction, PostgreSQL/pgvector retrieval, and Railway deployment. ChangesInvestigation platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant InvestigationRouter
participant InvestigationRuntime
participant InvestigationTools
participant InvestigationModel
participant InvestigationService
Client->>InvestigationRouter: POST /investigations/stream
InvestigationRouter->>InvestigationService: Create investigation record
InvestigationRouter->>InvestigationRuntime: Stream investigation state
InvestigationRuntime->>InvestigationModel: Request bounded decision
InvestigationRuntime->>InvestigationTools: Execute scoped tool
InvestigationTools-->>InvestigationRuntime: Return documents or evidence
InvestigationRuntime->>InvestigationModel: Generate report
InvestigationRuntime->>InvestigationRuntime: Validate citations
InvestigationRuntime-->>InvestigationRouter: Emit activity, evidence, report, and completion events
InvestigationRouter->>InvestigationService: Persist state snapshots
InvestigationRouter-->>Client: Stream SSE events
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
Superseded by #5 on the public release/railway-production branch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
docs/deploy-railway.md-186-189 (1)
186-189: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not describe Profile B as fully Railway-hosted.
Production authentication requires
SUPABASE_URLandSUPABASE_PUBLISHABLE_KEYwhenDOCULENS_REQUIRE_AUTH=true. This profile therefore still depends on a Supabase project for Auth. The current wording can cause a deployment to omit Supabase and fail production validation.Rename this profile to describe Railway-hosted processing infrastructure, or document a supported non-Supabase authentication path.
🤖 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 `@docs/deploy-railway.md` around lines 186 - 189, Update the Profile B heading and description in the deployment documentation to clarify that Railway hosts the processing infrastructure but authentication still requires a Supabase project when DOCULENS_REQUIRE_AUTH=true. Remove the “everything on Railway” wording, or document an explicitly supported non-Supabase authentication path.docs/deploy-railway.md-228-230 (1)
228-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvide model-provider credentials to Web.
The Web service executes the active SSE investigation request. The agent therefore needs its configured provider credential in Web. Assigning provider keys only to Worker makes
/api/v1/investigations/streamfail when it calls the model.Add the selected provider key to both services when both can execute model calls.
🤖 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 `@docs/deploy-railway.md` around lines 228 - 230, The deployment variable assignment described in docs/deploy-railway.md must provide the selected model-provider credential to both the Web service and Worker. Update the service configuration so Web receives the provider key alongside its existing authentication and public-domain variables, while preserving Worker’s provider, database, Redis, and storage variables.docs/deploy-railway.md-30-31 (1)
30-31: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMake Free-tier cost guidance conditional on actual Serverless sleep eligibility.
Railway Serverless sleeps a service only after at least 10 minutes without outbound traffic. Active database pools, telemetry, or provider traffic can prevent sleep. Do not describe this profile as inherently sleeping or imply a predictable Free-tier cost without an operational verification step. (docs.railway.com)
docs/deploy-railway.md#L30-L31: describe Serverless as an optional cost control and document its outbound-traffic condition.docs/deploy-railway.md#L238-L239: replace mutable plan figures with a pricing-reference instruction and require users to set a hard usage limit.🤖 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 `@docs/deploy-railway.md` around lines 30 - 31, Update docs/deploy-railway.md lines 30-31 to present Railway Serverless as an optional cost-control measure, explicitly noting that sleeping requires at least 10 minutes without outbound traffic and must be operationally verified. Update docs/deploy-railway.md lines 238-239 to replace mutable plan-price figures with instructions to consult the current pricing reference and set a hard usage limit.app/doc_utils/extraction.py-51-57 (1)
51-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unsupported binary file types.
The non-PDF branch accepts every suffix. A
.docx,.xlsx, or other binary upload can decode into non-empty replacement text and be indexed as evidence.Allow only configured plain-text suffixes in this profile. Raise a clear error for other types, or route them through the Docling profile.
🤖 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/doc_utils/extraction.py` around lines 51 - 57, Update the non-PDF branch in the extraction flow to validate the input suffix against the profile’s configured plain-text suffixes before calling path.read_text. Reject unsupported binary types with a clear error, or explicitly route them through the Docling profile, while preserving plain-text extraction for allowed suffixes.app/services/vector_store.py-116-149 (1)
116-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd an Alembic migration for the production vector schema.
The Celery worker has no initialization path. Alembic does not create the
vectorextension,embeddingstable, or indexes. A fresh deployment can process worker tasks before web startup creates this schema.🤖 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/services/vector_store.py` around lines 116 - 149, Add an Alembic migration that creates the vector extension, embeddings table, metadata GIN index, and HNSW cosine embedding index represented by the schema setup in the vector store methods. Include a downgrade that removes these objects in dependency-safe order, and ensure the migration is available independently of web startup so Celery workers can rely on the production schema.app/doc_utils/chunking.py-50-60 (1)
50-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce
max_tokensduring lightweight chunking.
embed_and_upsert_chunkstruncates text at 8,191 tokens. It does not enforce the requested chunk limit. CJK text or long unbroken strings can form oversized chunks, causing silent content loss during embedding.Tokenize each page with
OpenAITokenizerWrapper, create overlapping windows ofmax_tokens, and decode each window before creatingLightweightChunk.🤖 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/doc_utils/chunking.py` around lines 50 - 60, Update the lightweight chunking loop in the document chunking function to use OpenAITokenizerWrapper for each page instead of word splitting and an estimated max_words limit. Tokenize page text, create overlapping token windows capped at max_tokens, decode each window, and use the decoded text when constructing LightweightChunk so embed_and_upsert_chunks cannot receive oversized chunks.app/config/settings.py-86-88 (1)
86-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the local storage path with the Docker volume mount.
docker/docker-compose.ymlmountsingestion_dataat/workspace/data/ingestionfor both API and worker services. These defaults use/workspace/app/data/ingestion. Local uploads can remain in the API container filesystem, and workers can fail to find uploaded documents.
app/config/settings.py#L86-L88: use/workspace/data/ingestion, or change both Compose mounts to this configured path..env.example#L33-L35: document the same shared path as the runtime default.🤖 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/config/settings.py` around lines 86 - 88, Update app/config/settings.py lines 86-88 in the storage_local_path setting to use /workspace/data/ingestion, matching the Docker volume mount. Update .env.example lines 33-35 to document the same shared path as the runtime default.Makefile-16-17 (1)
16-17: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet
DOCULENS_TASK_MODE=localfor the API inpreview. The target omitscelery_worker, while the default mode iscelery; uploads will queue in Redis without processing.🤖 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 `@Makefile` around lines 16 - 17, Update the Makefile preview target’s API startup configuration to set DOCULENS_TASK_MODE=local, while preserving the existing service list and detached build behavior.app/config/database_config.py-20-20 (1)
20-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoad database variables through Pydantic Settings.
Settings.model_config.env_filedoes not apply to nestedDatabaseConfig. The import-timeos.getenvdefaults can therefore ignore.envvalues and use fallback connection settings. ConfigureDatabaseConfigwithenv_file=".env"andvalidation_aliasvalues forDATABASE_URL,DATABASE_HOST,DATABASE_PORT,DATABASE_NAME,DATABASE_USER, andDATABASE_PASSWORD.🤖 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/config/database_config.py` at line 20, Update DatabaseConfig to load all database fields through Pydantic Settings instead of import-time os.getenv defaults: configure its model settings with env_file=".env" and add validation aliases for DATABASE_URL, DATABASE_HOST, DATABASE_PORT, DATABASE_NAME, DATABASE_USER, and DATABASE_PASSWORD, preserving the existing field types and defaults.app/start.sh-7-7 (1)
7-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTrust the Railway proxy CIDR.
Uvicorn ignores
X-Forwarded-Protofrom untrusted peers. Add Railway’s100.0.0.0/8proxy range so HTTPS requests receive the HSTS header.🤖 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/start.sh` at line 7, Update the uvicorn invocation in start.sh to trust Railway’s 100.0.0.0/8 proxy CIDR by configuring the appropriate forwarded-header trusted-host option, while preserving the existing proxy-header handling and startup arguments.app/api/investigation_router.py-151-165 (1)
151-165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe failure handler can raise before it emits the
errorevent.
persist_investigation_stateraisesLookupErrorwhen the record is absent, and_save_snapshotopens a new session that can also fail. The most likely cause of the original exception is a database or connection problem, so the recovery write at line 158 tends to fail for the same reason. That exception propagates out of the generator, and theerrorevent at lines 159-165 never reaches the client. The client then observes a truncated stream with no terminal event and waits indefinitely.Emit the terminal event first, or isolate the recovery write.
🐛 Proposed fix
except Exception as exc: logger.exception("Investigation %s failed", investigation_id) failed_state: InvestigationState = { **final_state, "status": "failed", "error": str(exc), } - _save_snapshot(investigation_id, failed_state) + try: + _save_snapshot(investigation_id, failed_state) + except Exception: + logger.exception( + "Could not persist the failed state for investigation %s", investigation_id + ) yield _sse( "error", { "id": str(investigation_id), "message": "The investigation stopped before completion.", }, )🤖 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/api/investigation_router.py` around lines 151 - 165, Update the exception handler in the investigation stream generator to ensure the terminal “error” SSE event is emitted even when `_save_snapshot` fails. Emit the event before the recovery write, or isolate `_save_snapshot(investigation_id, failed_state)` in its own guarded error-handling block so persistence failures cannot escape the generator.app/services/auth_service.py-151-173 (1)
151-173: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject sign-in for deactivated accounts.
_upsert_supabase_usernever inspectsuser.is_active.authenticate_userat lines 85-91 rejects inactive users, anddecode_access_tokenat lines 283-285 rejects them too. The Supabase path does not. A deactivated user therefore receives HTTP 200 and a session cookie from/auth/supabase, and only fails on the next request. Add the same check so deactivation is enforced consistently at the login boundary.🔒 Proposed fix
if user is None: user = User( ... ) else: + if not user.is_active: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="This account is deactivated.", + ) user.email = email🤖 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/services/auth_service.py` around lines 151 - 173, Update _upsert_supabase_user to check an existing user’s is_active status before updating, committing, or returning it; reject deactivated users at the Supabase login boundary using the same behavior as authenticate_user and decode_access_token. Preserve creation of new users and normal handling of active users.app/services/auth_service.py-142-168 (1)
142-168: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire a verified email before linking a Supabase identity to an existing local account.
Line 144 falls back to
get_user_by_email. Lines 164-168 then rebind that existing row to the Supabase subject. The claims are not checked for email verification. If any enabled Supabase provider returns an unverified email address, a sign-in with that address takes over the pre-existing local account and inherits itspersona,role, andaccess_level. The seededadmin@doculens.aiaccount is the highest-value target.Read
email_confirmed_atfrom the Supabase user payload and reject the link when the address is unconfirmed.🔒 Proposed fix
def _upsert_supabase_user(self, claims: Dict[str, object]) -> User: subject = str(claims.get("id") or claims.get("sub") or "").strip() email = str(claims.get("email") or "").strip().lower() if not subject or not email: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Supabase account identity is incomplete.", ) self._enforce_supabase_allowlist(email=email) + email_verified = bool(claims.get("email_confirmed_at")) + user = self.get_user_by_provider_subject(provider="supabase", subject=subject) if user is None: - user = self.get_user_by_email(email=email) + existing = self.get_user_by_email(email=email) + if existing is not None and not email_verified: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Confirm this email address before linking the account.", + ) + user = existing🤖 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/services/auth_service.py` around lines 142 - 168, Update the Supabase authentication flow around get_user_by_email and the existing-user rebinding logic to read email_confirmed_at from the Supabase user payload and reject authentication before linking any unconfirmed email to a local account. Only allow the fallback email lookup and updates to auth_provider, provider_subject, and related fields when the email is confirmed; preserve new-user creation behavior for valid confirmed identities.app/services/auth_service.py-170-172 (1)
170-172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
IntegrityErroron the upsert commit.The method performs check-then-insert. Two concurrent first-time sign-ins for the same subject both pass the lookups at lines 142-144 and both insert. One commit then violates
uq_users_auth_identityor the uniqueBoth cases raise
IntegrityErrorout of an unauthenticated endpoint, so the caller receives HTTP 500. Catch the error, roll back, and either re-read the winning row or return a deterministic conflict response.🛡️ Proposed fix
- self.session.add(user) - self.session.commit() - self.session.refresh(user) - return user + self.session.add(user) + try: + self.session.commit() + except IntegrityError: + self.session.rollback() + existing = self.get_user_by_provider_subject(provider="supabase", subject=subject) + if existing is None: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="This email address is already linked to another identity.", + ) from None + return existing + self.session.refresh(user) + return userAdd the import:
from sqlalchemy.exc import IntegrityError🤖 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/services/auth_service.py` around lines 170 - 172, Update the upsert flow around the session.commit call in the relevant auth service method to catch SQLAlchemy IntegrityError, roll back the session, and re-read the existing winning user when the conflict represents a concurrent insert; return a deterministic conflict response when the email belongs to a different subject. Add the IntegrityError import and preserve normal refresh behavior for successful commits.app/api/auth_router.py-107-117 (1)
107-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
Nonefrom the 204 logout handler.FastAPI sets the injected
Response.status_codetoNoneand removescontent-length. Returning it bypasses the declared 204 response and causes Uvicorn/h11 to reject the status at runtime. ReturnNoneso FastAPI creates the 204 response and merges the cookie headers.🤖 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/api/auth_router.py` around lines 107 - 117, Update the logout handler to return None instead of the injected Response object, while preserving the existing delete_cookie call and its settings. Keep the declared 204 response behavior and allow FastAPI to merge the cookie headers.app/api/investigation_router.py-38-39 (1)
38-39: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthorize
document_idsbefore buildingdocument_scope.The route stores
user_idon the investigation, but does not pass it to the document tools. The tools filter only by caller-supplied IDs, so private documents lack an ownership or workspace authorization check before evidence is returned. Also bound each identifier becausemax_length=100limits the list count, not individual strings persisted in the JSONB state.🤖 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/api/investigation_router.py` around lines 38 - 39, Update the investigation request flow around the document_ids field and document_scope construction to authorize every requested document against the authenticated user or workspace before building the scope or returning evidence; pass the stored user_id through to the document tools and preserve rejection of unauthorized IDs. Add a per-identifier length constraint in addition to the existing maximum list size, so persisted document IDs cannot exceed the intended bound.app/agents/tools.py-86-108 (1)
86-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPush the document scope into the SQL query.
The query takes the 100 most recent
document_uploadevents, and the scope filter runs afterwards in Python. If a scoped document is not in that window,list_documentsreports zero documents and the investigation proceeds without it. The step also loads fulldataandtask_contextJSONB for 100 rows on every call.Filter by the requested document ids in SQL when
document_scopeis non-empty.🐛 Proposed fix
def list_documents(self, document_scope: list[str]) -> ToolObservation: with SessionLocal() as session: - rows = session.execute( - text( - """ - SELECT id, data, task_context - FROM events - WHERE data->>'event_type' = 'document_upload' - ORDER BY created_at DESC - LIMIT 100 - """ - ) - ).mappings() + statement = text( + """ + SELECT id, data, task_context + FROM events + WHERE data->>'event_type' = 'document_upload' + AND ( + :scope_is_empty + OR COALESCE(task_context->'metadata'->'document'->>'id', id::text) + = ANY(:scope) + ) + ORDER BY created_at DESC + LIMIT 100 + """ + ) + rows = session.execute( + statement, + {"scope_is_empty": not document_scope, "scope": document_scope or [""]}, + ).mappings()🤖 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/agents/tools.py` around lines 86 - 108, Update list_documents to apply document_scope in the SQL query before ordering and limiting, matching ids extracted from the document metadata while preserving all documents when the scope is empty. Select only the fields needed for downstream processing instead of loading full data and task_context JSONB for unrelated rows, and remove the redundant Python-side scope filtering.app/agents/graph.py-50-60 (1)
50-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_merge_by_keydiscards the newest items when the limit is reached.
list(merged.values())[:limit]keeps insertion order, anddictpreserves the original position of an overwritten key. Once the evidence ledger reachesmax_evidence, every new passage fromsearch_evidenceorinspect_documentis dropped, while the oldest passages stay. The agent then re-queries, verification keeps failing because the new references are absent from the ledger, and the run consumes the remaining step budget before finishing aspartial.Keep the most recent items instead.
🐛 Proposed fix
merged = {str(item.get(key)): item for item in existing} for item in incoming: merged[str(item.get(key))] = item - return list(merged.values())[:limit] + return list(merged.values())[-limit:] if limit > 0 else []🤖 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/agents/graph.py` around lines 50 - 60, Update _merge_by_key so the limit retains the most recent merged items rather than slicing from the beginning; preserve deduplication by key and ensure newer incoming items replace older entries while remaining eligible for the returned limit.app/pipelines/doculens_pipeline.py-166-170 (1)
166-170: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftBound and validate the ingestion download.
DocumentUploadEvent.file_urlreachesrequests.getwithout URL validation, enabling SSRF. Redirects can bypass an initial host allowlist.response.contentalso loads the full body into memory, and this path does not enforcemax_upload_bytes.Allowlist the scheme and resolved host, disable unsafe redirects, then stream the response with a byte cap.
🤖 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/pipelines/doculens_pipeline.py` around lines 166 - 170, Update the download logic in the event.file_url ingestion block to validate the URL scheme and resolved host against the configured allowlist, disable redirects, and enforce max_upload_bytes while streaming response chunks to local_path instead of using response.content. Reject disallowed URLs, redirects, and responses exceeding the byte cap before completing the write.Source: Linters/SAST tools
app/agents/tools.py-74-130 (1)
74-130: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd server-side document authorization.
document_scopecomes directly frompayload.document_ids, and an empty scope reads all documents.eventshas no owner or workspace field, and uploads do not record the authenticated user. Add ownership or workspace data at upload time, then enforce it for event and vector queries. Do not rely on the client-provideddocument_scope.🤖 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/agents/tools.py` around lines 74 - 130, Add server-derived ownership or workspace metadata during document upload, then update DocumentInvestigationTools methods including list_documents, search_evidence, and inspect_document to constrain event and vector queries using the authenticated server context. Treat payload.document_ids/document_scope only as an additional filter within that authorized set, never as the authorization source, and ensure an empty scope cannot expose all documents.frontend/src/auth/ProtectedRoute.tsx-24-26 (1)
24-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe bypass fails open when the runtime config is missing.
If
serverConfigisundefined,!serverConfig?.session_auth_requiredevaluates totrueand the route renders without a user. That state is reachable.SettingsProvidersetsisLoadedtotruein itsfinallyblock even whenfetchRuntimeConfigrejects, and leavesserverConfigundefined. A failed or blocked/events/configrequest therefore unlocks every protected route in the UI.The backend still enforces authorization on each API call, so this exposes an unauthenticated shell rather than data. Require an explicit config value before bypassing.
🔒️ Proposed fail-closed condition
- if (serverConfig?.showcase_read_only || !serverConfig?.session_auth_required) { + if (serverConfig && (serverConfig.showcase_read_only || !serverConfig.session_auth_required)) { return <>{children}</>; }🤖 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 `@frontend/src/auth/ProtectedRoute.tsx` around lines 24 - 26, Update the bypass condition in ProtectedRoute so it only renders children when serverConfig exists and explicitly indicates showcase_read_only or session_auth_required is false; keep missing or failed runtime configuration fail-closed and require authentication instead.frontend/src/auth/AuthProvider.tsx-96-127 (1)
96-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSubscribe to auth state before the awaits, and respect
cancelled.Two defects come from the same cause.
initialiseregistersonAuthStateChangeonly after awaitingfetchProfileandexchangeToken.
exchangeTokenre-throws on failure (line 92). At line 104 it is awaited inside thecatchblock, so the rejection escapesinitialise. Thefinallyblock clears the loading flag, then lines 110-119 never run. The provider then has no auth-state subscription. A laterSIGNED_INorTOKEN_REFRESHEDevent is never exchanged, so the user stays signed out until a full page reload.void initialise()also discards the rejection, so no error is reported.- The cleanup function on lines 123-126 can run before line 118 assigns
unsubscribe. If the effect re-runs, the earlier subscription is never removed and leaks.Register the subscription first, guard it with
cancelled, and isolate the exchange failure.🐛 Proposed fix for subscription resilience and cleanup race
const initialise = async () => { + if (supabaseUrl && publishableKey) { + const supabase = getSupabaseClient(supabaseUrl, publishableKey); + const { data } = supabase.auth.onAuthStateChange((event, session) => { + if ((event === 'SIGNED_IN' || event === 'TOKEN_REFRESHED') && session?.access_token) { + void exchangeToken(session.access_token).catch(() => clearLocalSession(false)); + } + if (event === 'SIGNED_OUT') clearLocalSession(false); + }); + if (cancelled) { + data.subscription.unsubscribe(); + return; + } + unsubscribe = () => data.subscription.unsubscribe(); + } + try { const profile = await fetchProfile(); if (!cancelled) setUser(profile); } catch { if (supabaseUrl && publishableKey) { const supabase = getSupabaseClient(supabaseUrl, publishableKey); const { data } = await supabase.auth.getSession(); - if (data.session?.access_token) await exchangeToken(data.session.access_token); + if (data.session?.access_token) { + await exchangeToken(data.session.access_token).catch(() => undefined); + } } } finally { if (!cancelled) setIsLoading(false); } - - if (supabaseUrl && publishableKey) { - const supabase = getSupabaseClient(supabaseUrl, publishableKey); - const { data } = supabase.auth.onAuthStateChange((event, session) => { - if ((event === 'SIGNED_IN' || event === 'TOKEN_REFRESHED') && session?.access_token) { - void exchangeToken(session.access_token).catch(() => clearLocalSession(false)); - } - if (event === 'SIGNED_OUT') clearLocalSession(false); - }); - unsubscribe = () => data.subscription.unsubscribe(); - } };🤖 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 `@frontend/src/auth/AuthProvider.tsx` around lines 96 - 127, Update the initialise flow in AuthProvider so the onAuthStateChange subscription is created before any awaited fetchProfile or exchangeToken calls, assigning unsubscribe synchronously for reliable cleanup. Guard the subscription callback and initialization work with cancelled, and isolate exchangeToken failures with local handling so a rejection cannot abort initialization or prevent the subscription. Preserve setIsLoading cleanup and ensure callbacks do nothing after cancellation.frontend/src/api/client.ts-243-260 (1)
243-260: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle malformed frames and release the reader.
Wrap the read loop in
try/finallyand callreader.releaseLock()infinally. CatchJSON.parseerrors per frame so one malformeddata:frame does not terminate the stream. The backend emits explicitevent:fields, so no fallback event name is required.🤖 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 `@frontend/src/api/client.ts` around lines 243 - 260, The stream read loop must release the reader even when processing fails and must continue after malformed individual frames. Wrap the loop in try/finally with reader.releaseLock() in finally, and handle JSON.parse failures per frame without terminating subsequent frame processing. Keep event types sourced exclusively from the explicit event: line.
🟡 Minor comments (9)
README.md-292-294 (1)
292-294: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCorrect the deployment-scope limitation.
Line 290 says that the included deployment manifest is read-only. Lines 163-170 describe a writable Railway application with durable uploads.
docs/deploy-railway.mdalso configures uploads and verifies write operations.State that the Cloudflare showcase is read-only. State that the Railway profile is a writable single-workspace deployment.
🤖 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 `@README.md` around lines 292 - 294, Update the deployment-scope wording in README.md to distinguish the Cloudflare showcase as read-only from the Railway profile as a writable single-workspace deployment, aligning with the deployment manifest and docs/deploy-railway.md. Replace the conflicting limitation statement without changing the surrounding investigation or retrieval-metrics descriptions.app/doc_utils/embedding.py-90-90 (1)
90-90: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReplace UUID v1 identifiers with random UUIDs.
UUID v1 encodes creation time and can expose a host node identifier. These IDs are persisted and returned through vector retrieval paths. Use
uuid4()unless time-sortable identifiers are a defined requirement.
app/doc_utils/embedding.py#L90-L90: replaceuuid1()withuuid4()for chunk IDs.app/utils/insert_vectors.py#L42-L42: replaceuuid1()withuuid4()for inserted vector IDs.🤖 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/doc_utils/embedding.py` at line 90, Replace uuid1() with uuid4() when generating chunk IDs in app/doc_utils/embedding.py lines 90-90 and inserted vector IDs in app/utils/insert_vectors.py lines 42-42, preserving the existing ID generation flow.app/main.py-91-94 (1)
91-94: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe log message promises readiness behavior that the endpoint does not implement.
Line 94 states that readiness will remain unhealthy after a dependency initialization failure.
readinessonly runsSELECT 1. Ifinitialize_dependenciesfails while the database is reachable, for example when vector store index creation fails, the endpoint still returns{"status": "ready"}and Railway routes traffic to a partially initialized process.Record the failure in application state and report it from
readiness.🐛 Proposed fix
try: initialize_dependencies(runtime) + application.state.dependencies_ready = True except Exception: + application.state.dependencies_ready = False logger.exception("Dependency initialization failed; readiness will remain unhealthy")def readiness() -> dict[str, str]: + if getattr(application.state, "dependencies_ready", True) is False: + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Dependency initialization failed.", + ) try: with engine.connect() as connection:Also applies to: 143-154
🤖 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/main.py` around lines 91 - 94, Update the dependency initialization flow around initialize_dependencies to record failures in shared application state instead of only logging them. Update readiness to check that state and return an unhealthy response when initialization failed, while preserving the existing database connectivity check for successful initialization; remove or revise the misleading log claim so the endpoint behavior matches the recorded state.app/agents/graph.py-83-90 (1)
83-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA report with no findings reports full citation coverage.
If
report.findingsis empty and any evidence exists,coveragebecomes1.0andvalidbecomesTrue.finalizethen marks the investigationcompleted, andevaluate_investigationrecordscitation_coverage == 1.0. An empty report passes verification.Treat an empty findings list as unverified.
🐛 Proposed fix
finding_count = len(report.findings) - coverage = cited_findings / finding_count if finding_count else (1.0 if evidence else 0.0) + coverage = cited_findings / finding_count if finding_count else 0.0 return VerificationResult( - valid=bool(evidence) and not unsupported and not unknown, + valid=bool(evidence) and finding_count > 0 and not unsupported and not unknown,🤖 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/agents/graph.py` around lines 83 - 90, Update the verification logic around finding_count and VerificationResult so an empty report.findings list is always treated as unverified, regardless of whether evidence exists. Ensure coverage and valid do not allow an empty report to pass, while preserving the existing behavior for reports containing findings.app/pipelines/doculens_pipeline.py-171-175 (1)
171-175: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against a missing storage reference.
materializeacceptsreference: str, butevent.filenameis optional. Line 162 already handles a missing filename forsource_name. If an event carries neitherfile_urlnorfilename,Path(None)raisesTypeErrorinside the storage layer instead of a clear pipeline error.🛡️ Proposed guard
else: + if not event.filename: + raise ValueError("document_upload event requires filename or file_url.") local_path, temporary_local_path = get_document_storage().materialize( event.filename, destination_dir=ingestion_dir, )🤖 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/pipelines/doculens_pipeline.py` around lines 171 - 175, Guard the storage materialization branch around get_document_storage().materialize so it is only called when event.filename is present; when both file_url and filename are missing, raise a clear pipeline error instead of passing None to the storage layer. Preserve the existing source_name handling and file_url path behavior.app/evaluation/investigation.py-58-73 (1)
58-73: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle investigations that produced no report or verification.
InvestigationStateinitializesreportandverificationto{}. A failed run or aneeds_inputrun keeps them empty.InvestigationReport.model_validate({})then raisesValidationError, so the evaluation suite crashes instead of scoring the run as a failure.Score an empty report and empty verification as zero.
🐛 Proposed fix
- report = InvestigationReport.model_validate(state["report"]) - report_text = " ".join( - [ - report.title, - report.executive_summary, - *(finding.claim for finding in report.findings), - *(finding.explanation for finding in report.findings), - ] - ).lower() + raw_report = state.get("report") or {} + report = InvestigationReport.model_validate(raw_report) if raw_report else None + report_text = ( + " ".join( + [ + report.title, + report.executive_summary, + *(finding.claim for finding in report.findings), + *(finding.explanation for finding in report.findings), + ] + ).lower() + if report + else "" + ) @@ - verification = VerificationResult.model_validate(state["verification"]) + raw_verification = state.get("verification") or {} + verification = ( + VerificationResult.model_validate(raw_verification) + if raw_verification + else VerificationResult(valid=False, citation_coverage=0.0) + )🤖 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/evaluation/investigation.py` around lines 58 - 73, Update the evaluation flow around InvestigationReport.model_validate and VerificationResult.model_validate to handle empty state["report"] or state["verification"] without raising validation errors. Treat either empty result as a failed run with a score of zero, while preserving the existing validation and scoring behavior when both outputs are present.frontend/src/pages/LoginPage.tsx-31-31 (1)
31-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the protected route after sign-in.
ProtectedRoutestores the requested location instate.from, butLoginPage.tsxignores it and always redirects authenticated users to/app. Usestate.fromas the redirect target, with/appas the fallback.🤖 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 `@frontend/src/pages/LoginPage.tsx` at line 31, Update the authenticated redirect in LoginPage so it reads the stored location from navigation state, using state.from as the target and /app as the fallback. Preserve the existing serverConfig?.showcase_read_only condition and replace navigation behavior.app/api/endpoint.py-95-99 (1)
95-99: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the local processing contract.
task_idis not consumed by the current frontend or application. In local mode, it islocal-<event_id>, not a Celery task ID. Document this format and direct clients toGET /events/{event_id}fortask_context. The202response is returned beforeBackgroundTasksruns, and pipeline failures are not recorded. Add explicit failure logging and status handling if clients must observe local failures.🤖 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/api/endpoint.py` around lines 95 - 99, Document the local-mode contract near the endpoint logic: state that task_id uses the local-<event_id> format rather than a Celery ID, direct clients to GET /events/{event_id} for task_context, and note that the 202 response precedes BackgroundTasks execution. If local failures must be observable, add explicit failure logging and status handling around process_incoming_event.run.frontend/src/api/types.ts-284-293 (1)
284-293: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle empty report and verification sentinels
The backend initializes
state.reportandstate.verificationas{}. The API can return these values for queued or failed investigations. Since{}is truthy,?? nulldoes not preventReportPanelfrom rendering. Guard the report shape before accessingfindings,recommended_actions, orlimitations.🤖 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 `@frontend/src/api/types.ts` around lines 284 - 293, Update InvestigationDetail.state report and verification handling so empty-object sentinels from queued or failed investigations are treated as absent values rather than valid data. Validate the expected report/verification shape before exposing these fields to ReportPanel or accessing report properties such as findings, recommended_actions, and limitations, while preserving valid populated responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: decfaebc-bbac-44c4-a971-3778206196fc
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
.env.example.github/workflows/backend-ci.ymlMakefileREADME.mdapp/agents/__init__.pyapp/agents/graph.pyapp/agents/model_client.pyapp/agents/models.pyapp/agents/tools.pyapp/alembic/env.pyapp/alembic/versions/20260730_0005_add_federated_identity.pyapp/alembic/versions/20260730_0006_create_investigations.pyapp/api/auth_router.pyapp/api/endpoint.pyapp/api/investigation_router.pyapp/api/router.pyapp/config/celery_config.pyapp/config/database_config.pyapp/config/settings.pyapp/core/observability.pyapp/database/database_utils.pyapp/database/investigation.pyapp/database/user.pyapp/doc_utils/chunking.pyapp/doc_utils/embedding.pyapp/doc_utils/extraction.pyapp/doc_utils/search.pyapp/doc_utils/utils/tokenizer.pyapp/evaluation/investigation.pyapp/main.pyapp/pipelines/doculens_pipeline.pyapp/services/auth_service.pyapp/services/document_storage.pyapp/services/investigation_service.pyapp/services/vector_store.pyapp/start.shapp/tasks/tasks.pyapp/utils/insert_vectors.pydocker/Dockerfile.celerydocker/Dockerfile.railwaydocker/docker-compose.ymldocs/deploy-railway.mdfrontend/package.jsonfrontend/src/api/client.tsfrontend/src/api/types.tsfrontend/src/auth/AuthProvider.tsxfrontend/src/auth/ProtectedRoute.tsxfrontend/src/auth/supabase.tsfrontend/src/auth/types.tsfrontend/src/components/layout/AppShell.tsxfrontend/src/main.tsxfrontend/src/pages/InvestigationPage.tsxfrontend/src/pages/LoginPage.tsxpyproject.tomlpyrightconfig.jsonrailway.tomlrailway.worker.tomltests/conftest.pytests/test_api_endpoints.pytests/test_auth_service.pytests/test_investigation_agent.pytests/test_investigation_evaluation.pytests/test_lightweight_extraction.pytests/test_settings.py
| def materialize(self, reference: str, *, destination_dir: Path) -> tuple[Path, bool]: | ||
| path = Path(reference) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f"Stored document '{reference}' does not exist.") | ||
| return path.resolve(), False |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
The ingestion path trusts caller-supplied source locations in the event payload. Both sites resolve a document source directly from persisted event fields (filename and file_url) with no allowlist or containment check, so a caller who can post an event controls what the worker reads.
app/services/document_storage.py#L80-L84: resolve the reference and reject any path that is not underself.root, matching the bucket-prefix check inS3DocumentStorage.materialize.app/pipelines/doculens_pipeline.py#L166-L170: validate the URL scheme and host against an allowlist, and stream the response with a byte cap equal tomax_upload_bytes.
📍 Affects 2 files
app/services/document_storage.py#L80-L84(this comment)app/pipelines/doculens_pipeline.py#L166-L170
🤖 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/services/document_storage.py` around lines 80 - 84, The local
materialization path in DocumentStorage.materialize must reject
caller-controlled references outside self.root after resolving the path,
matching S3DocumentStorage.materialize containment behavior; update
app/services/document_storage.py lines 80-84 accordingly. The remote ingestion
logic in app/pipelines/doculens_pipeline.py lines 166-170 must validate the URL
scheme and host against the configured allowlist and stream downloads with a
byte cap of max_upload_bytes.
1 participant
Summary
Validation
npm --prefix frontend run buildnpm --prefix frontend run lint.venv/bin/pytest tests/test_auth_service.py tests/test_settings.py -q(8 passed)git diff --check/health/liveand/health/readyreturn 200Tradeoff
Supabase adds one external token-verification request during sign-in, while subsequent API traffic continues to use the existing secure DocuLens session cookie. This keeps the backend authorization model simple and avoids validating an external JWT on every request.
Summary by CodeRabbit