fix(auth): replace user_id query param with JWT-based identity. Relat…#372
Conversation
|
@wen-placeholder is attempting to deploy a commit to the Jerry's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc/default user_id usage with FastAPI dependency injection via get_required_user_id across many backend routes, threads the authenticated user_id into handlers and downstream calls, updates request models to remove embedded user_id fields, and adds/adjusts frontend proxying and auth flows to propagate backend auth tokens. Streaming endpoints are wired to use the injected user_id. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI as FastAPI (Depend resolver)
participant Route as API Route Handler
participant Agent as Agent/Generator
participant Store as Store/Service
participant Stream as StreamingResponse
Client->>FastAPI: POST /api/gen-code (Authorization header)
FastAPI->>FastAPI: resolve Depends(get_required_user_id)
FastAPI->>Route: call generate_code(request, user_id)
Route->>Agent: start gen_code_stream(request, user_id)
Agent->>Store: call agent/store operations with user_id
Agent-->>Route: yield events (SSE)
Route->>Stream: wrap_generator(agent) -> StreamingResponse(text/event-stream)
Stream-->>Client: stream SSE chunks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the application by transitioning user identification from vulnerable client-provided query parameters or request body fields to a robust, server-side validated JWT-based authentication system. This change ensures that all user-specific operations are performed under a verified identity, mitigating potential security risks associated with Identity Disclosure or manipulation. The refactoring touches a broad range of API endpoints, streamlining how user context is handled throughout the application's core functionalities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR advances the auth migration for Issue #151 by removing client-controlled user_id inputs from multiple API routes and instead deriving user identity from JWT via a FastAPI dependency.
Changes:
- Updated many research, memory, harvest, repro-context, and gen-code endpoints to use
Depends(get_user_id)instead ofuser_idquery/body fields. - Removed
user_idfrom several Pydantic request models inresearch.pyand rewired store calls to use the authenticateduser_id. - Began refactoring some SSE endpoints to use
StreamingResponse+ generator wrappers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/paperbot/api/routes/research.py |
Switches most research endpoints from client-supplied user_id to Depends(get_user_id) and removes user_id from request bodies. |
src/paperbot/api/routes/repro_context.py |
Adds Depends(get_user_id) to generation endpoint and attempts to prefer authenticated identity. |
src/paperbot/api/routes/memory.py |
Migrates memory ingest/list/context routes to Depends(get_user_id) and adds “effective user id” logic. |
src/paperbot/api/routes/harvest.py |
Migrates library routes to Depends(get_user_id) and removes MVP TODOs about unauthenticated user_id. |
src/paperbot/api/routes/gen_code.py |
Adds Depends(get_user_id) to gen-code endpoint and changes SSE response construction approach. |
Comments suppressed due to low confidence (1)
src/paperbot/api/routes/memory.py:185
items = _store.search_memories(...)can crash because_storeis anOptional[...] = Noneglobal that is only initialized in_get_store(). Use_get_store().search_memories(...)here (or otherwise guarantee_storeis initialized) to avoidAttributeErroron the first request.
items = _store.search_memories(
user_id=effective_user_id,
workspace_id=req.workspace_id,
query=req.query,
limit=req.limit,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| req: PrecomputeEmbeddingsRequest, | ||
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| result = _track_router.precompute_track_embeddings( |
There was a problem hiding this comment.
_track_router is Optional[...] = None and is intended to be accessed via _get_track_router(). Calling _track_router.precompute_track_embeddings(...) can fail if the router hasn't been initialized yet. Use _get_track_router() (or ensure _track_router is initialized) before invoking methods.
| result = _track_router.precompute_track_embeddings( | |
| result = _get_track_router().precompute_track_embeddings( |
| # Respect authenticated user id where available; fall back to request.user_id | ||
| request.user_id = request.user_id or user_id | ||
|
|
There was a problem hiding this comment.
request.user_id = request.user_id or user_id means the request body's user_id (defaulting to "default") will override the authenticated user_id from JWT, and a malicious client can set it to another user's id. To prevent IDOR and actually scope to the JWT identity, prefer the dependency-provided user_id (and only fall back to request data when AUTH_OPTIONAL is enabled / dependency returned "default"), and make the body user_id optional or remove it.
|
|
||
| from fastapi import APIRouter, HTTPException, Query, Request | ||
| from fastapi import APIRouter, Depends, HTTPException, Query, Request | ||
| from fastapi.responses import StreamingResponse |
There was a problem hiding this comment.
StreamingResponse is imported but never used in this file (the route uses sse_response). Consider removing the unused import to avoid lint failures / reduce confusion about which SSE helper should be used.
| from fastapi.responses import StreamingResponse |
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| effective_user_id = user_id | ||
| track = _research_store.create_track( |
There was a problem hiding this comment.
_research_store is declared as Optional[...] = None and is only initialized via _get_research_store(). Calling _research_store.create_track(...) will raise at runtime on the first request if the getter hasn't been called yet. Use _get_research_store() (or eagerly initialize _research_store) and apply the same pattern to other new direct _research_store.* calls in this module.
| track = _research_store.create_track( | |
| track = _get_research_store().create_track( |
| def memory_context(req: ContextRequest, user_id: str = Depends(get_user_id)): | ||
| effective_user_id = req.user_id or user_id | ||
| items = _store.search_memories( | ||
| user_id=effective_user_id, |
There was a problem hiding this comment.
effective_user_id = req.user_id or user_id lets the client-provided req.user_id override the authenticated identity, which reintroduces the IDOR this PR is trying to remove. Also, ContextRequest.user_id defaults to "default", so this will almost always ignore the JWT-derived user_id. Prefer using the dependency-provided user_id (and only allow a fallback when AUTH_OPTIONAL is enabled / dependency returns "default"), and make the request user_id optional or remove it.
| f"[M2] generate_request trace_id={trace_id} paper_id={request.paper_id} user_id={effective_user_id}", | ||
| file=LogFiles.API, | ||
| ) | ||
| return sse_response(_generate_stream(request), workflow="p2c_generate") |
There was a problem hiding this comment.
This endpoint returns sse_response(...), but sse_response is no longer imported in this module (the import was changed to wrap_generator). This will raise a NameError at runtime. Either import sse_response again, or switch the return to StreamingResponse(wrap_generator(...), media_type="text/event-stream", ...) consistently.
| return sse_response(_generate_stream(request), workflow="p2c_generate") | |
| return StreamingResponse( | |
| wrap_generator(_generate_stream(request), workflow="p2c_generate"), | |
| media_type="text/event-stream", | |
| ) |
| # Prefer authenticated user id when available | ||
| effective_user_id = request.user_id or user_id |
There was a problem hiding this comment.
effective_user_id = request.user_id or user_id allows a client-supplied request.user_id to override the authenticated JWT identity, which defeats the purpose of moving to JWT-scoped data and can reintroduce IDOR. Additionally GenerateContextPackRequest.user_id defaults to "default", so the JWT-derived value will usually be ignored. Prefer always using the dependency-provided user_id (or only falling back when AUTH_OPTIONAL is enabled / the dependency returned "default"), and make the request user_id optional or remove it.
| # Prefer authenticated user id when available | |
| effective_user_id = request.user_id or user_id | |
| # Prefer authenticated user id when available; only fall back to request.user_id when auth is default | |
| effective_user_id = user_id | |
| if effective_user_id == "default" and request.user_id and request.user_id != "default": | |
| effective_user_id = request.user_id |
| return StreamingResponse( | ||
| wrap_generator( | ||
| gen_code_stream(request, event_log=event_log, run_id=run_id, trace_id=trace_id), | ||
| workflow="gen_code", | ||
| run_id=run_id, | ||
| trace_id=trace_id, | ||
| ), | ||
| media_type="text/event-stream", |
There was a problem hiding this comment.
wrap_generator(...) is used here but is not imported in this module, which will raise a NameError at runtime. Either import wrap_generator from paperbot.api.streaming/..streaming, or revert to using sse_response(...) which already wraps generators correctly.
| status=req.status, | ||
| ) | ||
| created, _, rows = _get_memory_store().add_memories(user_id=req.user_id, memories=[cand]) | ||
| created, _, rows = _memory_store.add_memories(user_id=user_id, memories=[cand]) |
There was a problem hiding this comment.
_memory_store is Optional[...] = None and is lazily created by _get_memory_store(). Using _memory_store.add_memories(...) directly can crash with AttributeError if _get_memory_store() hasn't been invoked yet. Call _get_memory_store() (or initialize _memory_store) before using it, and audit other new _memory_store.* calls in this file.
| created, _, rows = _memory_store.add_memories(user_id=user_id, memories=[cand]) | |
| memory_store = _get_memory_store() | |
| created, _, rows = memory_store.add_memories(user_id=user_id, memories=[cand]) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly replaces the insecure practice of passing user_id in query parameters or request bodies with a JWT-based identity obtained via FastAPI dependencies. The changes are extensive and applied across multiple API routes, which is a significant security enhancement. However, I've identified a critical and recurring issue in three of the modified files where the user_id from the request body is still prioritized over the authenticated user's ID from the token. This reintroduces the same IDOR vulnerability this PR aims to fix and must be addressed. I've also left a minor comment regarding code clarity.
| run_id=run_id, | ||
| trace_id=trace_id, | ||
| # Respect authenticated user id where available; fall back to request.user_id | ||
| request.user_id = request.user_id or user_id |
There was a problem hiding this comment.
The logic to determine the user ID is incorrect and introduces a security vulnerability. The expression request.user_id or user_id gives precedence to the user_id from the request body over the authenticated user_id from the JWT token. This allows a user to impersonate another user by simply providing a different user_id in the request, which this pull request aims to prevent. The authenticated user ID from the token should be the sole source of truth. The comment on the preceding line is also misleading.
| request.user_id = request.user_id or user_id | |
| request.user_id = user_id |
| items = _get_store().search_memories( | ||
| user_id=req.user_id, | ||
| def memory_context(req: ContextRequest, user_id: str = Depends(get_user_id)): | ||
| effective_user_id = req.user_id or user_id |
There was a problem hiding this comment.
The logic to determine the user ID is incorrect and introduces a security vulnerability. The expression req.user_id or user_id gives precedence to the user_id from the request body over the authenticated user_id from the JWT token. This allows a user to impersonate another user by simply providing a different user_id in the request, which this pull request aims to prevent. The authenticated user ID from the token should be the sole source of truth.
| effective_user_id = req.user_id or user_id | |
| effective_user_id = user_id |
| # Prefer authenticated user id when available | ||
| effective_user_id = request.user_id or user_id | ||
| request.user_id = effective_user_id |
There was a problem hiding this comment.
The logic to determine the user ID is incorrect and introduces a security vulnerability. The expression request.user_id or user_id gives precedence to the user_id from the request body over the authenticated user_id from the JWT token. This allows a user to impersonate another user by simply providing a different user_id in the request, which this pull request aims to prevent. The authenticated user ID should be the sole source of truth. The comment on line 341 is also misleading as the code does the opposite of what it claims.
| # Prefer authenticated user id when available | |
| effective_user_id = request.user_id or user_id | |
| request.user_id = effective_user_id | |
| # Use authenticated user ID as the source of truth | |
| effective_user_id = user_id | |
| request.user_id = user_id |
| background_tasks: BackgroundTasks, | ||
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| effective_user_id = user_id |
There was a problem hiding this comment.
The effective_user_id variable is redundant here. You can directly use user_id in the _research_store.create_track call on line 434 to improve code clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/paperbot/api/routes/memory.py (1)
180-185:⚠️ Potential issue | 🔴 CriticalReintroduce lazy store initialization here.
_storeisNoneuntil_get_store()runs. Calling_store.search_memories(...)makes the first/memory/contextrequest crash withAttributeError.Suggested fix
- items = _store.search_memories( + items = _get_store().search_memories( user_id=effective_user_id, workspace_id=req.workspace_id, query=req.query, limit=req.limit, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/memory.py` around lines 180 - 185, The code calls _store.search_memories(...) but _store may be None until _get_store() runs, causing an AttributeError; fix by ensuring you initialize the store first (call _get_store() and assign its result to _store or a local variable) before invoking search_memories, e.g., obtain store = _get_store() (or set _store = _get_store()) and then call store.search_memories(...) so the first /memory/context request does not crash.src/paperbot/api/routes/gen_code.py (1)
186-200:⚠️ Potential issue | 🔴 CriticalAlways overwrite
request.user_idwith the authenticated value.Line 23 initializes
GenCodeRequest.user_idto"default", sorequest.user_id or user_idkeeps routing work under the placeholder user instead of the injected identity. It also lets callers spoof another user in the JSON body.Suggested fix
- # Respect authenticated user id where available; fall back to request.user_id - request.user_id = request.user_id or user_id + request.user_id = user_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/gen_code.py` around lines 186 - 200, The handler generate_code currently assigns request.user_id using "request.user_id = request.user_id or user_id", which allows the request body default "default" or a spoofed value to override the authenticated identity; change this to always set request.user_id = user_id (the value injected by Depends(get_user_id)) so the authenticated user is authoritative (refer to generate_code and the GenCodeRequest.user_id field).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Around line 15-17: The NameError comes from calling wrap_generator(...) before
its module is imported; import the streaming wrapper (wrap_generator) from the
same module that provides sse_response before any route code that calls
wrap_generator. Update the top imports so wrap_generator (and any other
streaming helper you use) is imported alongside sse_response (or move the
wrap_generator import immediately above the route that calls wrap_generator) so
wrap_generator is defined when the route executes.
In `@src/paperbot/api/routes/memory.py`:
- Around line 177-180: The memory endpoint currently allows callers to override
the authenticated identity by using ContextRequest.user_id (default "default")
and the memory_context function uses effective_user_id = req.user_id or user_id;
change memory_context to always use the injected user_id (from get_user_id) when
calling _store.search_memories and remove the fallback to req.user_id, and
update the ContextRequest model (remove the default "default" or remove the
user_id field) so request bodies cannot supply another user's id.
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 335-346: The request body is currently allowed to override the
authenticated identity because GenerateContextPackRequest.user_id has a truthy
default; in generate_context_pack you should ignore the client-supplied user_id
unless there is truly no authenticated identity. Change the effective_user_id
logic to prefer the injected parameter user_id (e.g., effective_user_id =
user_id if user_id else None) and only fall back to request.user_id when user_id
is empty and request.user_id is not the sentinel "default"; do not trust or
accept arbitrary user-supplied IDs from the request body (avoid assigning
request.user_id = effective_user_id or, if you must store it, ensure it is the
authenticated id).
- Around line 19-24: The endpoint is failing because sse_response was removed
from the import list; restore the SSE helper by importing sse_response so the
code that calls sse_response(...) can resolve it. Update the import line that
currently imports StreamEvent and wrap_generator (from paperbot.api.streaming)
to also include sse_response so functions like sse_response used later in this
module resolve correctly.
---
Outside diff comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Around line 186-200: The handler generate_code currently assigns
request.user_id using "request.user_id = request.user_id or user_id", which
allows the request body default "default" or a spoofed value to override the
authenticated identity; change this to always set request.user_id = user_id (the
value injected by Depends(get_user_id)) so the authenticated user is
authoritative (refer to generate_code and the GenCodeRequest.user_id field).
In `@src/paperbot/api/routes/memory.py`:
- Around line 180-185: The code calls _store.search_memories(...) but _store may
be None until _get_store() runs, causing an AttributeError; fix by ensuring you
initialize the store first (call _get_store() and assign its result to _store or
a local variable) before invoking search_memories, e.g., obtain store =
_get_store() (or set _store = _get_store()) and then call
store.search_memories(...) so the first /memory/context request does not crash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5df52924-befc-4995-aa2e-418aa718e3fa
📒 Files selected for processing (5)
src/paperbot/api/routes/gen_code.pysrc/paperbot/api/routes/harvest.pysrc/paperbot/api/routes/memory.pysrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/research.py
| @router.post("/research/tracks", response_model=TrackResponse) | ||
| def create_track(req: TrackCreateRequest, background_tasks: BackgroundTasks): | ||
| track = _get_research_store().create_track( | ||
| user_id=req.user_id, | ||
| def create_track( | ||
| req: TrackCreateRequest, | ||
| background_tasks: BackgroundTasks, | ||
| user_id: str = Depends(get_user_id), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add route coverage for the new identity source.
This file rewires user scoping on many endpoints, but the change set doesn't add a regression proving create/list/update flows actually use the injected user id. Please add at least one integration test around the new dependency path before merging.
As per coding guidelines, {src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Fix IDOR: always assign JWT-derived user_id, ignore request body user_id in gen_code, memory, and repro_context routes - Fix AttributeError: replace bare _research_store/_memory_store with _get_research_store()/_get_memory_store() in research.py; replace _store with _get_store() in memory.py - Fix missing import: add wrap_generator to gen_code.py, add sse_response to repro_context.py - Remove unused StreamingResponse import from harvest.py - Remove redundant effective_user_id variable in research.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/paperbot/api/routes/research.py (1)
426-431:⚠️ Potential issue | 🟠 MajorUpdate the route tests to authenticate through
get_user_id.The current snippets in
tests/integration/test_research_track_routes.py,tests/integration/test_research_track_memory_routes.py, andtests/unit/test_anchor_action_routes.pystill passuser_idvia query params or request JSON. After this migration those tests no longer exercise the authenticated path and can quietly fall back to"default", so this change needs matching dependency overrides or auth headers in the test suite.As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.Also applies to: 457-475, 769-805, 2212-2248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/research.py` around lines 426 - 431, Tests for routes that now depend on the get_user_id dependency (e.g., create_track in research.py) still supply user_id via query/JSON and therefore bypass the auth dependency; update the affected tests (tests/integration/test_research_track_routes.py, tests/integration/test_research_track_memory_routes.py, tests/unit/test_anchor_action_routes.py and the other ranges noted) to override the dependency or supply proper auth headers so get_user_id returns the intended test user: use FastAPI's dependency_overrides to replace get_user_id with a stub that returns the expected user_id (or configure test client auth headers if get_user_id reads headers), and remove passing user_id in request bodies/queries so the tests exercise the authenticated path used by functions like create_track.
🧹 Nitpick comments (1)
src/paperbot/api/routes/gen_code.py (1)
201-213: Prefer the shared SSE helper here.
sse_response()already wrapswrap_generatorand applies the repo's standard SSE headers. Building theStreamingResponseinline makes this endpoint drift from the other streaming routes.♻️ Suggested cleanup
-from fastapi.responses import StreamingResponse @@ -from ..streaming import StreamEvent, wrap_generator +from ..streaming import StreamEvent, sse_response @@ - return StreamingResponse( - wrap_generator( - gen_code_stream(request, event_log=event_log, run_id=run_id, trace_id=trace_id), - workflow="gen_code", - run_id=run_id, - trace_id=trace_id, - ), - media_type="text/event-stream", - headers={ - "Cache-Control": "no-cache", - "Connection": "keep-alive", - }, - ) + return sse_response( + gen_code_stream(request, event_log=event_log, run_id=run_id, trace_id=trace_id), + workflow="gen_code", + run_id=run_id, + trace_id=trace_id, + )Based on learnings, Use SSE streaming for long-running API endpoints - see
src/paperbot/api/streaming.pyfor utilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/gen_code.py` around lines 201 - 213, Replace the inline StreamingResponse with the shared SSE helper by calling sse_response(...) and passing the wrapped generator for gen_code_stream (preserve args: request, event_log=event_log, run_id=run_id, trace_id=trace_id) and the workflow/run_id/trace_id metadata currently passed to wrap_generator; this ensures the standard SSE headers and behavior from sse_response are used instead of constructing StreamingResponse inline and keeps this endpoint consistent with other streaming routes that use the utilities in streaming.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 335-347: The other context-pack routes expose an IDOR because they
use caller-supplied identity or fetch packs by pack_id alone; update each
handler (list_context_packs, get_context_pack, get_context_pack_session,
delete_context_pack) to accept user_id via Depends(get_user_id) (same as
generate_context_pack) and then pass that user_id into store calls or validate
ownership after load (e.g., fetch_by_id_and_user or assert pack.user_id ==
user_id) so every pack lookup/enumeration is scoped to the authenticated user;
also ensure any request objects set request.user_id where applicable.
In `@src/paperbot/api/routes/research.py`:
- Line 831: The handlers call module-level singletons (_memory_store,
_research_store, _track_router) directly which can be None at import time;
replace direct accesses with the module's lazy getter functions (e.g., call the
lazy getter that returns the initialized instance instead of using
_memory_store/_research_store/_track_router directly) so the instance is created
on first use; update the line "created, skipped, rows =
_memory_store.add_memories(...)" and all other occurrences referenced (lines
around 859-860, 1093-1095, 1387-1389, 1671-1778, 2097-2103, 2215-2289,
2386-2387, 3956-4022) to use the appropriate lazy getter (the functions that
wrap/return the singleton) before calling methods like add_memories, push
(Zotero), anchor, related-work, paper-detail, and activate_track_id.
- Around line 563-565: get_track_context is still IDOR-prone because it accepts
user_id with a default string and forwards it to the service; change the route
signature to require the authenticated user via dependency injection (replace
user_id: str = "default" with user_id: str = Depends(get_user_id)) and pass that
user_id into _get_research_store().get_track_context so callers cannot supply an
arbitrary user_id via query string; ensure the function name get_track_context
and the call to _get_research_store().get_track_context are updated accordingly.
---
Duplicate comments:
In `@src/paperbot/api/routes/research.py`:
- Around line 426-431: Tests for routes that now depend on the get_user_id
dependency (e.g., create_track in research.py) still supply user_id via
query/JSON and therefore bypass the auth dependency; update the affected tests
(tests/integration/test_research_track_routes.py,
tests/integration/test_research_track_memory_routes.py,
tests/unit/test_anchor_action_routes.py and the other ranges noted) to override
the dependency or supply proper auth headers so get_user_id returns the intended
test user: use FastAPI's dependency_overrides to replace get_user_id with a stub
that returns the expected user_id (or configure test client auth headers if
get_user_id reads headers), and remove passing user_id in request bodies/queries
so the tests exercise the authenticated path used by functions like
create_track.
---
Nitpick comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Around line 201-213: Replace the inline StreamingResponse with the shared SSE
helper by calling sse_response(...) and passing the wrapped generator for
gen_code_stream (preserve args: request, event_log=event_log, run_id=run_id,
trace_id=trace_id) and the workflow/run_id/trace_id metadata currently passed to
wrap_generator; this ensures the standard SSE headers and behavior from
sse_response are used instead of constructing StreamingResponse inline and keeps
this endpoint consistent with other streaming routes that use the utilities in
streaming.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b30b9f96-1af2-4d19-8bef-c63f43da78ae
📒 Files selected for processing (5)
src/paperbot/api/routes/gen_code.pysrc/paperbot/api/routes/harvest.pysrc/paperbot/api/routes/memory.pysrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/research.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/paperbot/api/routes/memory.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (2)
src/paperbot/api/routes/research.py:2014
_research_storeis optional and may not be initialized when this endpoint is called; direct use can raise. Use_get_research_store()forlist_paper_feedback_ids(...)/add_paper_feedback(...)to guarantee initialization.
existing_saved_ids = _research_store.list_paper_feedback_ids(
user_id=user_id,
track_id=track_pk,
action="save",
limit=5000,
)
src/paperbot/api/routes/research.py:1902
_research_storemay beNonehere (lazy init happens in_get_research_store()), so this can throw. Use_get_research_store().list_paper_feedback_ids(...)(and similarly for other_research_store.*calls in this flow).
existing_saved_ids = _research_store.list_paper_feedback_ids(
user_id=user_id,
track_id=track_pk,
action="save",
limit=5000,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| _ensure_anchor_feature_enabled() | ||
|
|
||
| track = _get_research_store().get_track(user_id=req.user_id, track_id=track_id) | ||
| track = _research_store.get_track(user_id=user_id, track_id=track_id) |
There was a problem hiding this comment.
_research_store is lazily initialized via _get_research_store(). This direct call can raise if the store hasn't been initialized yet; use _get_research_store().get_track(...) here.
| track = _research_store.get_track(user_id=user_id, track_id=track_id) | |
| track = _get_research_store().get_track(user_id=user_id, track_id=track_id) |
| return StreamingResponse( | ||
| wrap_generator( | ||
| gen_code_stream(request, event_log=event_log, run_id=run_id, trace_id=trace_id), | ||
| workflow="gen_code", | ||
| run_id=run_id, | ||
| trace_id=trace_id, | ||
| ), | ||
| media_type="text/event-stream", | ||
| headers={ | ||
| "Cache-Control": "no-cache", | ||
| "Connection": "keep-alive", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
generate_code now constructs a StreamingResponse manually but omits the project-standard SSE headers (notably X-Accel-Buffering: no from paperbot.api.streaming.SSE_HEADERS), which can cause proxy buffering and break streaming. Consider using sse_response(...) directly, or merge in SSE_HEADERS when building the response.
| req: PaperReadingStatusRequest, | ||
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| status = _research_store.set_paper_reading_status( |
There was a problem hiding this comment.
_research_store is an optional global initialized via _get_research_store(). This direct call can fail with NoneType if the store hasn't been initialized yet; use _get_research_store().set_paper_reading_status(...).
| status = _research_store.set_paper_reading_status( | |
| status = _get_research_store().set_paper_reading_status( |
| detail = _research_store.get_paper_detail(paper_id=paper_id, user_id=user_id) | ||
| if not detail: |
There was a problem hiding this comment.
_research_store is an optional global; using it directly can raise if not initialized. Prefer _get_research_store().get_paper_detail(...) for safe lazy init.
| active = _research_store.get_active_track(user_id=user_id) | ||
| if not active: | ||
| return RouterSuggestResponse(suggestion=None) | ||
| suggestion = _get_track_router().suggest_track( | ||
| user_id=req.user_id, query=req.query, active_track_id=int(active["id"]) | ||
| suggestion = _track_router.suggest_track( | ||
| user_id=user_id, query=req.query, active_track_id=int(active["id"]) | ||
| ) |
There was a problem hiding this comment.
Both _research_store and _track_router are optional globals that are intended to be accessed via _get_research_store() / _get_track_router(). Calling them directly can crash if they haven't been initialized yet; use the getters here.
| updated = _memory_store.update_item( | ||
| user_id=user_id, | ||
| item_id=item_id, |
There was a problem hiding this comment.
_memory_store is an optional global and may be None when this route is hit, leading to an AttributeError. Use _get_memory_store().update_item(...) (or initialize _memory_store before calling).
| collection = _research_store.create_collection( | ||
| user_id=user_id, | ||
| name=req.name, |
There was a problem hiding this comment.
_research_store is an optional global (initialized via _get_research_store()), so calling it directly here can crash if it hasn't been set. Use _get_research_store().create_collection(...) for consistent lazy initialization.
| track = _research_store.get_track(user_id=user_id, track_id=req.track_id) | ||
| if not track: | ||
| raise HTTPException(status_code=404, detail="Track not found") | ||
|
|
There was a problem hiding this comment.
_research_store is an optional global; calling _research_store.get_track(...) can fail if the store hasn't been initialized. Use _get_research_store().get_track(...) (and likewise for the list_saved_papers(...) call below).
| from fastapi import APIRouter, HTTPException, Depends | ||
| from fastapi.responses import StreamingResponse | ||
| from pydantic import BaseModel | ||
|
|
||
| from paperbot.api.streaming import StreamEvent, sse_response | ||
| from paperbot.api.streaming import StreamEvent, wrap_generator, sse_response | ||
| from paperbot.api.auth.dependencies import get_user_id |
There was a problem hiding this comment.
StreamingResponse and wrap_generator are imported but unused in this module. Either remove these imports or switch generate_context_pack to use them (right now it returns sse_response(...)).
| user_id: str = Depends(get_user_id), | ||
| ): |
There was a problem hiding this comment.
This route now derives user_id from auth (Depends(get_user_id)), which breaks existing tests that pass ?user_id=... (e.g. tests/integration/test_research_track_routes.py). Update tests to set Authorization: Bearer ... (or override the get_user_id dependency) so user-scoped behavior is still exercised.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that replaces query parameter and request body user_id fields with a centralized, JWT-based authentication dependency. This greatly improves security and consistency across the API. The changes are extensive and well-executed. My review includes a few suggestions to address a potential runtime bug related to store initialization and to improve consistency in handling request data, which will enhance the overall maintainability of the code.
| req: MemoryModerateRequest, | ||
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| updated = _memory_store.update_item( |
There was a problem hiding this comment.
The global variable _memory_store is accessed directly, but it might not be initialized, which could lead to an AttributeError at runtime. The _get_memory_store() function handles lazy initialization. You should use _get_memory_store().update_item(...) to ensure the store is initialized. This issue appears in multiple places in this file where _get_memory_store(), _get_research_store(), and _get_track_router() calls were replaced with direct access to the global variables (_memory_store, _research_store, _track_router). Please review all such changes to prevent potential runtime errors.
| updated = _memory_store.update_item( | |
| updated = _get_memory_store().update_item( |
| workflow="gen_code", | ||
| run_id=run_id, | ||
| trace_id=trace_id, | ||
| request.user_id = user_id |
There was a problem hiding this comment.
Mutating the request object in place can be a source of confusion. A cleaner pattern, which is also used in other parts of this PR (e.g., in src/paperbot/api/routes/harvest.py), is to remove the user_id field from the GenCodeRequest model and pass the user_id from the dependency directly to the service function (gen_code_stream). This would make the data flow more explicit and improve maintainability.
| ): | ||
| """Generate a P2C context pack for the given paper. Returns SSE stream.""" | ||
| trace_id = set_trace_id() | ||
| request.user_id = user_id |
There was a problem hiding this comment.
Similar to my comment in gen_code.py, mutating the request object can be confusing. Consider removing user_id from GenerateContextPackRequest and passing it as a separate argument to _generate_stream for better clarity and consistency with other parts of this PR.
| Logger.info("Recording paper feedback to research store", file=LogFiles.HARVEST) | ||
| fb = research_store.add_paper_feedback( | ||
| user_id=req.user_id, | ||
| fb = _research_store.add_paper_feedback( |
There was a problem hiding this comment.
The local variable research_store is assigned on line 1148 but is not used. Instead, the global _research_store is used here. This is inconsistent and makes the code harder to follow. It would be cleaner to use the local variable research_store.
| fb = _research_store.add_paper_feedback( | |
| fb = research_store.add_paper_feedback( |
…erry609#372 review - repro_context: add Depends(get_user_id) + ownership checks to all context-pack routes - research: replace all bare _memory_store/_research_store/_track_router accesses with lazy getters; fix get_track_context to use Depends(get_user_id)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperbot/api/routes/repro_context.py (1)
354-375:⚠️ Potential issue | 🟠 MajorBackfill legacy pack ownership before rolling out these user filters.
repro_context_pack.user_idstill defaults to"default", so older packs will disappear fromlist_context_packs, and the new get/session/delete ownership checks will reject them for authenticated users. If optional auth remains enabled, those same"default"rows are still readable as the anonymous/default user. This rollout needs a data migration/backfill or an explicit legacy-ownership strategy before these filters go live.Also applies to: 383-408, 423-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 354 - 375, The user-filtering change hides legacy packs whose repro_context_pack.user_id == "default"; either perform a data migration/backfill to set correct user ownership for existing rows, or make the listing/ownership checks explicitly handle the legacy "default" owner (e.g., treat "default" as the anonymous/legacy bucket when resolving ownership). Update the store method used here (_get_store().list_by_user) and the ownership checks used by get/session/delete to either include "default" rows for unauthenticated/legacy access or map "default" to the proper user id before filtering; ensure list_context_packs and the other endpoints that call _get_store().list_by_user (and the get/session/delete ownership validators referenced in this file) consistently apply the same legacy-ownership logic or run a one-time DB backfill to replace "default" with the correct user ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 334-347: The change to authenticated/ownership semantics for the
P2C endpoints (notably generate_context_pack which returns
sse_response(_generate_stream(request)) and related handlers for
list/get/session/delete) lacks regression tests; add/modify pytest (or frontend
test) coverage to assert authenticated access succeeds, unauthorized cross-user
access is denied, owner-only operations are enforced, and legacy "default" row
behavior remains unchanged—specifically write tests that call
generate_context_pack (or the underlying _generate_stream),
list/get/session/delete handlers with authenticated user_id, with a different
user_id to verify 403/deny, and with legacy entries labeled "default" to ensure
existing behavior is preserved. Ensure tests reference the same request shapes
(GenerateContextPackRequest) and simulate SSE or the stream consumer as needed,
and update test fixtures/mocks for auth (get_user_id) and ownership checks to
reflect the new logic.
In `@src/paperbot/api/routes/research.py`:
- Around line 427-431: The route handlers (e.g., create_track) currently use
Depends(get_user_id) which returns the sentinel "default" in AUTH_OPTIONAL mode,
allowing anonymous access; change the handler to require a strict authenticated
user by either swapping the dependency to a strict variant (e.g.,
Depends(get_user_id_strict) or Depends(get_authenticated_user_id)) or add an
explicit guard at the start of each handler (create_track and the other
converted routes in this module) that checks if user_id == "default" and raises
an HTTPException(401/403) before touching any stores; apply the same change to
all other research module routes converted to user-scoped handlers.
---
Outside diff comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 354-375: The user-filtering change hides legacy packs whose
repro_context_pack.user_id == "default"; either perform a data
migration/backfill to set correct user ownership for existing rows, or make the
listing/ownership checks explicitly handle the legacy "default" owner (e.g.,
treat "default" as the anonymous/legacy bucket when resolving ownership). Update
the store method used here (_get_store().list_by_user) and the ownership checks
used by get/session/delete to either include "default" rows for
unauthenticated/legacy access or map "default" to the proper user id before
filtering; ensure list_context_packs and the other endpoints that call
_get_store().list_by_user (and the get/session/delete ownership validators
referenced in this file) consistently apply the same legacy-ownership logic or
run a one-time DB backfill to replace "default" with the correct user ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7becfbb1-7b71-4ce2-97f1-f2ce74a4fbdb
📒 Files selected for processing (2)
src/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/research.py
| @router.post("/generate") | ||
| async def generate_context_pack(request: GenerateContextPackRequest): | ||
| async def generate_context_pack( | ||
| request: GenerateContextPackRequest, | ||
| user_id: str = Depends(get_user_id), | ||
| ): | ||
| """Generate a P2C context pack for the given paper. Returns SSE stream.""" | ||
| trace_id = set_trace_id() | ||
| request.user_id = user_id | ||
|
|
||
| Logger.info( | ||
| f"[M2] generate_request trace_id={trace_id} paper_id={request.paper_id} user_id={request.user_id}", | ||
| f"[M2] generate_request trace_id={trace_id} paper_id={request.paper_id} user_id={user_id}", | ||
| file=LogFiles.API, | ||
| ) | ||
| return sse_response(_generate_stream(request), workflow="p2c_generate") |
There was a problem hiding this comment.
Add regression tests for the new auth and ownership path.
This changes generate/list/get/session/delete semantics materially, but the PR doesn't add coverage for authenticated access, cross-user denial, and the legacy "default" row behavior.
As per coding guidelines, {src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.
Also applies to: 354-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/repro_context.py` around lines 334 - 347, The change
to authenticated/ownership semantics for the P2C endpoints (notably
generate_context_pack which returns sse_response(_generate_stream(request)) and
related handlers for list/get/session/delete) lacks regression tests; add/modify
pytest (or frontend test) coverage to assert authenticated access succeeds,
unauthorized cross-user access is denied, owner-only operations are enforced,
and legacy "default" row behavior remains unchanged—specifically write tests
that call generate_context_pack (or the underlying _generate_stream),
list/get/session/delete handlers with authenticated user_id, with a different
user_id to verify 403/deny, and with legacy entries labeled "default" to ensure
existing behavior is preserved. Ensure tests reference the same request shapes
(GenerateContextPackRequest) and simulate SSE or the stream consumer as needed,
and update test fixtures/mocks for auth (get_user_id) and ownership checks to
reflect the new logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant step forward in securing and standardizing your API. By replacing user_id query parameters with a JWT-based dependency, you've greatly improved authentication handling. The addition of ownership checks and standardization of streaming responses are also valuable improvements. My review focuses on a few areas to enhance consistency and code clarity, particularly around how the new user_id is propagated and how access-denied errors are handled. Overall, these are excellent changes.
| workflow="gen_code", | ||
| run_id=run_id, | ||
| trace_id=trace_id, | ||
| request.user_id = user_id |
There was a problem hiding this comment.
Modifying the request object in-place by adding user_id can lead to unexpected side effects and makes the data flow harder to trace. This pattern is also inconsistent with other endpoints in this PR where user_id is passed as an explicit argument to service functions. A cleaner approach would be to update gen_code_stream to accept user_id as a parameter and pass it directly.
| @router.post("/memory/context", response_model=ContextResponse) | ||
| def memory_context(req: ContextRequest): | ||
| def memory_context(req: ContextRequest, user_id: str = Depends(get_user_id)): | ||
| effective_user_id = user_id |
There was a problem hiding this comment.
The intermediate variable effective_user_id is unnecessary. You can use the user_id from the dependency directly in the call to _get_store().search_memories on line 181 and in the ContextResponse on line 200. This will make the code slightly more concise.
| ): | ||
| """Generate a P2C context pack for the given paper. Returns SSE stream.""" | ||
| trace_id = set_trace_id() | ||
| request.user_id = user_id |
There was a problem hiding this comment.
Modifying the request object in-place by adding user_id can lead to unexpected side effects and makes the data flow harder to trace. This pattern is also inconsistent with other endpoints in this PR where user_id is passed as an explicit argument to service functions. A cleaner approach would be to update _generate_stream to accept user_id as a parameter and pass it directly.
| if pack is None or pack.get("user_id") != user_id: | ||
| raise HTTPException(status_code=404, detail="Context pack not found.") |
There was a problem hiding this comment.
For consistency with get_context_pack and to provide a more accurate error, you should separate the 'not found' and 'access denied' checks. If the pack exists but belongs to another user, a 403 Forbidden status code would be more appropriate than a 404 Not Found.
| if pack is None or pack.get("user_id") != user_id: | |
| raise HTTPException(status_code=404, detail="Context pack not found.") | |
| if pack is None: | |
| raise HTTPException(status_code=404, detail="Context pack not found.") | |
| if pack.get("user_id") != user_id: | |
| raise HTTPException(status_code=403, detail="Access denied.") |
| Logger.info("Recording paper feedback to research store", file=LogFiles.HARVEST) | ||
| fb = research_store.add_paper_feedback( | ||
| user_id=req.user_id, | ||
| fb = _get_research_store().add_paper_feedback( |
There was a problem hiding this comment.
You're calling _get_research_store() again, but you've already assigned its result to the research_store variable on line 1148. You can reuse the research_store variable here for consistency and to avoid an unnecessary function call.
| fb = _get_research_store().add_paper_feedback( | |
| fb = research_store.add_paper_feedback( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @router.get("/papers/library", response_model=LibraryResponse) | ||
| def get_user_library( | ||
| user_id: str = Query("default", description="User ID"), | ||
| user_id: str = Depends(get_user_id), | ||
| track_id: Optional[int] = Query(None, description="Filter by track"), | ||
| actions: Optional[str] = Query(None, description="Filter by actions (comma-separated)"), |
There was a problem hiding this comment.
These library endpoints use Depends(get_user_id), which can fall back to the shared "default" namespace when AUTH_OPTIONAL=true (see get_user_id). For user-scoped reads, this should use get_required_user_id to prevent anonymous access to another user's data / the shared namespace.
| @router.post("/papers/{paper_id}/save") | ||
| def save_paper_to_library(paper_id: int, request: SavePaperRequest): | ||
| def save_paper_to_library( | ||
| paper_id: int, | ||
| request: SavePaperRequest, | ||
| user_id: str = Depends(get_user_id), | ||
| ): |
There was a problem hiding this comment.
save_paper_to_library writes user-scoped data but uses Depends(get_user_id), which may resolve to "default" under AUTH_OPTIONAL=true. To avoid anonymous callers mutating the shared namespace, switch this to get_required_user_id.
| from paperbot.infrastructure.stores.memory_store import SqlAlchemyMemoryStore | ||
| from paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore | ||
| from paperbot.infrastructure.stores.workflow_metric_store import WorkflowMetricStore | ||
| from paperbot.api.auth.dependencies import get_user_id, get_required_user_id |
There was a problem hiding this comment.
get_user_id is imported but not used in this module. Please drop it to avoid unused-import lint failures and keep the auth surface clear (this file now consistently uses get_required_user_id).
| from paperbot.api.auth.dependencies import get_user_id, get_required_user_id | |
| from paperbot.api.auth.dependencies import get_required_user_id |
| async def gen_code_stream( | ||
| request: GenCodeRequest, *, event_log=None, run_id: str = "", trace_id: str = "" | ||
| request: GenCodeRequest, *, user_id: str, event_log=None, run_id: str = "", trace_id: str = "" | ||
| ): | ||
| """Stream code generation progress""" |
There was a problem hiding this comment.
gen_code_stream now takes the authenticated user_id separately, but GenCodeRequest still exposes a user_id field in the request body (and it is no longer used). This can confuse clients and invites incorrect assumptions; remove user_id from the request model (or mark it excluded) to make the API contract explicit.
| @router.delete("/papers/{paper_id}/save") | ||
| def remove_paper_from_library( | ||
| paper_id: int, | ||
| user_id: str = Query("default", description="User ID"), | ||
| user_id: str = Depends(get_user_id), | ||
| ): |
There was a problem hiding this comment.
remove_paper_from_library deletes user-scoped data but uses Depends(get_user_id), which can fall back to the shared "default" namespace when AUTH_OPTIONAL=true. This should use get_required_user_id so unauthenticated callers cannot modify shared state.
| @router.post("/memory/context", response_model=ContextResponse) | ||
| def memory_context(req: ContextRequest): | ||
| def memory_context(req: ContextRequest, user_id: str = Depends(get_required_user_id)): | ||
| items = _get_store().search_memories( | ||
| user_id=req.user_id, | ||
| user_id=user_id, | ||
| workspace_id=req.workspace_id, |
There was a problem hiding this comment.
This endpoint now derives user_id from auth, but ContextRequest still includes a user_id field in the JSON body (and it is ignored). To avoid confusing clients (and to align with the PR goal of removing user_id from payloads), remove user_id from ContextRequest or mark it excluded/ignored in the schema.
| media_type="text/event-stream", | ||
| headers={ | ||
| "Cache-Control": "no-cache", | ||
| "Connection": "keep-alive", |
There was a problem hiding this comment.
The SSE headers here are missing X-Accel-Buffering: no, which paperbot.api.streaming.sse_response includes via SSE_HEADERS. Without it, nginx/proxies may buffer the stream and break live updates; consider using sse_response(...) again or reusing SSE_HEADERS so headers stay consistent across endpoints.
| "Connection": "keep-alive", | |
| "Connection": "keep-alive", | |
| "X-Accel-Buffering": "no", |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperbot/api/routes/gen_code.py (1)
186-212: 🛠️ Refactor suggestion | 🟠 MajorAdd regression coverage for the authenticated stream path.
This endpoint now rejects anonymous callers and changed how the SSE response is built, but the PR doesn't add a test proving an authenticated request still starts a
text/event-streamresponse and an unauthenticated one gets a 401.As per coding guidelines:
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/gen_code.py` around lines 186 - 212, Add regression tests covering the authenticated and unauthenticated paths for the generate_code SSE endpoint: create tests that call the generate_code route (invoking gen_code_stream via wrap_generator) with a valid user token/Depends(get_required_user_id) and assert the response status is 200 and the Content-Type (or media_type) is "text/event-stream" (and that the response starts streaming), and another test that calls the endpoint without authentication and asserts a 401 response; place these tests alongside existing API route tests and name them clearly (e.g., test_generate_code_authenticated_stream and test_generate_code_unauthenticated_401) so future changes to generate_code, gen_code_stream, or wrap_generator will be covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Line 10: Replace the hand-rolled StreamingResponse in the gen_code route with
the shared sse_response(...) helper from src/paperbot/api/streaming.py so the
route reuses the canonical SSE headers (including X-Accel-Buffering: no) and
event flushing behavior; locate the StreamingResponse creation in
src/paperbot/api/routes/gen_code.py (where StreamingResponse is imported and
used) and pass the same generator/async iterator and status headers into
sse_response instead of rebuilding headers manually. Ensure you import
sse_response and remove the ad-hoc header assembly so the route behavior matches
other SSE endpoints.
In `@src/paperbot/api/routes/memory.py`:
- Line 60: The memory routes now require auth via get_required_user_id and will
return 401 on missing/invalid tokens; add integration tests that (1) call at
least one of the memory endpoints in src/paperbot/api/routes/memory.py without
an Authorization header and assert a 401 response, and (2) perform authenticated
requests with two different test users (each using a valid token) to the same
memory endpoints and assert that data returned/created is scoped to the caller
(no leakage between user A and user B). Locate handlers that use
get_required_user_id in memory.py and write tests that exercise both the
no-token 401 path and the authenticated scoping path, creating/reading resources
as needed to prove isolation.
---
Outside diff comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Around line 186-212: Add regression tests covering the authenticated and
unauthenticated paths for the generate_code SSE endpoint: create tests that call
the generate_code route (invoking gen_code_stream via wrap_generator) with a
valid user token/Depends(get_required_user_id) and assert the response status is
200 and the Content-Type (or media_type) is "text/event-stream" (and that the
response starts streaming), and another test that calls the endpoint without
authentication and asserts a 401 response; place these tests alongside existing
API route tests and name them clearly (e.g.,
test_generate_code_authenticated_stream and
test_generate_code_unauthenticated_401) so future changes to generate_code,
gen_code_stream, or wrap_generator will be covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32b304ce-401f-4536-8b55-804c7219a575
📒 Files selected for processing (5)
src/paperbot/api/auth/dependencies.pysrc/paperbot/api/routes/gen_code.pysrc/paperbot/api/routes/memory.pysrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/research.py
| async def ingest_memory( | ||
| file: UploadFile = File(...), | ||
| user_id: str = Query("default", description="Memory namespace; use one id per person/team."), | ||
| user_id: str = Depends(get_required_user_id), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add route coverage for the new required-auth memory handlers.
These handlers no longer fall back to "default"; they now fail closed on missing or invalid auth. Please add at least one integration test that exercises 401 on no token and verifies authenticated requests stay scoped to the caller.
As per coding guidelines: {src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.
Also applies to: 126-126, 178-178, 231-231, 265-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/memory.py` at line 60, The memory routes now require
auth via get_required_user_id and will return 401 on missing/invalid tokens; add
integration tests that (1) call at least one of the memory endpoints in
src/paperbot/api/routes/memory.py without an Authorization header and assert a
401 response, and (2) perform authenticated requests with two different test
users (each using a valid token) to the same memory endpoints and assert that
data returned/created is scoped to the caller (no leakage between user A and
user B). Locate handlers that use get_required_user_id in memory.py and write
tests that exercise both the no-token 401 path and the authenticated scoping
path, creating/reading resources as needed to prove isolation.
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
web/src/components/research/FeedTab.tsx (1)
74-110:⚠️ Potential issue | 🟠 MajorBehavior changed but no corresponding test update is included.
This change alters request construction and auth context handling in the component; please add/update tests covering the fetch URL contract and track-selection load behavior.
As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/FeedTab.tsx` around lines 74 - 110, The FeedTab component's behavior changed around request construction and auth context but no tests were updated; add/update unit tests for the load function and component mount behavior: write a test that mounts FeedTab (or calls its exported load behavior) with a sample trackId and mock global fetch to assert the request URL includes the expected query string (e.g. ?limit=20&offset=0) and that the component sets items/loading/error correctly, and add a test that switching trackId triggers a new fetch (use rerender or updating props to confirm fetch called again and previous items cleared). Also add a test that the fetch call is made under the proper auth context (wrap with the same auth/provider used in app or mock auth headers) and assert error handling by mocking a non-ok response to verify setError and setItems([]) behavior; target functions/components: FeedTab, load, and the useEffect trackId dependency.web/next.config.ts (1)
41-45:⚠️ Potential issue | 🟠 MajorUse environment variable for backend proxy destination in rewrites.
The rewrite fallback at line 44 hardcodes
http://localhost:8000/api/:path*, which bypasses thePAPERBOT_API_BASE_URLenvironment variable already used throughout the client-side code. This breaks in staging/prod and will misroute API requests that don't match the explicit rewrite rules above.Use
process.env.PAPERBOT_API_BASE_URLin the rewrite destination to match the configuration pattern already established inweb/src/lib/config.tsandweb/src/lib/dashboard-api.ts:Example fix
const backendApiBase = (process.env.PAPERBOT_API_BASE_URL || "http://localhost:8000").replace(/\/$/, "") const nextConfig: NextConfig = { async rewrites() { return [ { source: '/api/:path*', - destination: 'http://localhost:8000/api/:path*', + destination: `${backendApiBase}/api/:path*`, }, ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/next.config.ts` around lines 41 - 45, The rewrite currently hardcodes the backend URL in the rewrite object that matches source '/api/:path*' and destination 'http://localhost:8000/api/:path*'; change the destination to use the environment variable process.env.PAPERBOT_API_BASE_URL (and fall back to the existing localhost:8000 if you want a safe default) and append '/api/:path*' so requests are routed to `${process.env.PAPERBOT_API_BASE_URL}/api/:path*` (ensure you normalize/truncate any trailing slash from PAPERBOT_API_BASE_URL to avoid double slashes).web/src/components/research/ResearchDashboard.tsx (1)
496-503:⚠️ Potential issue | 🔴 CriticalReferences to undefined
userIdandsetUserIdwill cause runtime errors.The UI references
userIdandsetUserIdwhich were removed as part of the auth migration, but the JSX wasn't updated. This will crash the component.🐛 Proposed fix — remove the manual user_id input
<div className="flex items-center justify-between"> <div> <h2 className="text-3xl font-bold tracking-tight">Research</h2> <p className="text-muted-foreground">Tracks, memory inbox, and personalized paper recommendations</p> </div> <div className="flex items-center gap-3"> - <Label className="text-sm text-muted-foreground">user_id</Label> - <Input value={userId} onChange={(e) => setUserId(e.target.value)} className="w-[200px]" /> <Button variant="secondary" onClick={() => refreshTracks()} disabled={loading}> Refresh </Button> </div> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/ResearchDashboard.tsx` around lines 496 - 503, The JSX references undefined userId and setUserId (from ResearchDashboard) causing runtime errors; remove the manual user_id input and Label and update the surrounding div so it only renders the Refresh Button (keep the existing Button with onClick={() => refreshTracks()} and disabled={loading}); ensure any state or imports for userId/setUserId are also removed from the ResearchDashboard component to avoid unused symbols.src/paperbot/api/routes/research.py (1)
904-923:⚠️ Potential issue | 🟠 MajorAvoid persisting raw
user_idin metrics payloads.These metric writes now store JWT-derived identifiers directly (
evaluator_id/detail.user_id). That expands PII retention and observability exposure. Prefer a stable pseudonymous actor id (hash/HMAC) and omit raw user ids from metric details.🔧 Minimal hardening pattern
+import hashlib + +def _metric_actor(user_id: str) -> str: + return hashlib.sha256(user_id.encode("utf-8")).hexdigest()[:16] ... - evaluator_id=f"user:{user_id}", + evaluator_id=f"user:{_metric_actor(user_id)}", ... - detail={"author_id": author_id, "user_id": user_id}, + detail={"author_id": author_id},Also applies to: 1010-1014, 1064-1068, 2233-2242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/research.py` around lines 904 - 923, The current metric write in the block using _get_metric_collector() and collector.record_false_positive_rate(...) persists the raw JWT-derived user_id in evaluator_id and in detail; replace this with a stable pseudonymous actor id (e.g., HMAC or SHA256 of user_id with an app secret) and remove the raw user id from the detail payload. Specifically, compute a pseudonymous id (not the plain user_id) before calling record_false_positive_rate and set evaluator_id to that pseudonymous value, and ensure detail does not include user_id (only include non-PII fields like item_ids and action). Apply the same change for other occurrences referenced (around lines where record_false_positive_rate and similar metric writes use evaluator_id/detail).web/src/app/api/studio/chat/route.ts (1)
1-8: 🛠️ Refactor suggestion | 🟠 MajorMove import to the top of the file.
The import statement on line 7 is placed after the runtime declaration and
apiBaseUrl()function. Imports should be at the top of the file for consistency and readability.♻️ Suggested fix
export const runtime = "nodejs" +import { withBackendAuth } from "../../_utils/auth-headers" + function apiBaseUrl() { return process.env.PAPERBOT_API_BASE_URL || "http://127.0.0.1:8000" } -import { withBackendAuth } from "../../_utils/auth-headers" - export async function POST(req: Request) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/studio/chat/route.ts` around lines 1 - 8, Move the import for withBackendAuth to the top of the module above the runtime export and the apiBaseUrl function: place the line importing withBackendAuth before the existing export const runtime = "nodejs" and the apiBaseUrl() declaration so all imports are grouped at the top of the file and preserve the existing runtime and apiBaseUrl symbols.web/src/lib/api.ts (2)
40-45:⚠️ Potential issue | 🟠 MajorPropagate auth token through
fetchStatsto avoid false-zero dashboard stats.
fetchStats()still callsfetchPapers()without token. With JWT-backed identity, this can return unauthorized/empty data and produce misleading metrics.✅ Suggested fix
-export async function fetchStats(): Promise<Stats> { +export async function fetchStats(accessToken?: string): Promise<Stats> { try { const [usage, papers, scholars] = await Promise.allSettled([ fetchLLMUsage(), - fetchPapers(), + fetchPapers(accessToken), fetchScholars(), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/api.ts` around lines 40 - 45, fetchStats currently calls fetchPapers (and likely fetchScholars/fetchLLMUsage) without forwarding the caller's auth token, causing unauthenticated/empty responses; update fetchStats to accept an optional auth token parameter (e.g., token?: string) and pass that token into the downstream calls — specifically call fetchPapers(token), fetchScholars(token) and fetchLLMUsage(token) (or the appropriate parameter names those functions expect) so the JWT is propagated; ensure fetchStats signature and all internal Promise.allSettled entries are updated to use the token and adjust any callers to supply the token where needed.
63-668:⚠️ Potential issue | 🟠 MajorAdd tests for the new token-based behavior across updated fetchers.
This file changed request auth behavior in multiple public fetch helpers, but no corresponding test updates are included in the provided changes.
As per coding guidelines "If behavior changes, add or update tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/api.ts` around lines 63 - 668, The tests are missing for the new token-based auth behavior introduced in several public fetch helpers; add unit/integration tests that assert Authorization header usage and fallback behavior for functions like fetchActivities, fetchReadingQueue, fetchDeadlineRadar, fetchPaperDetails, fetchPapers and any other helpers that accept accessToken (also include postJson-backed ones used in fetchScholarDetails indirectly); mock the global fetch (and postJson) to capture request headers and responses for both with-token and without-token cases, verify that when an accessToken is provided the Authorization: Bearer <token> header is sent and the functions parse successful payloads correctly, and that when the token is absent or the endpoint returns non-OK the functions return the documented fallback values/empty arrays.web/src/app/settings/page.tsx (2)
689-740:⚠️ Potential issue | 🟡 MinorBind the provider dialog labels to their controls.
These standalone
<label>elements are not associated with the matchingInput/select, so assistive tech will not announce the field names correctly. UseLabelplus matchinghtmlFor/idpairs here, consistent with the rest of the page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/settings/page.tsx` around lines 689 - 740, The labels for the provider dialog fields (e.g., the Name label paired with Input using form.name, Vendor label with the select for form.vendor, Base URL label with Input for form.base_url, API Key Env with form.api_key_env, API Key with form.api_key, Models with form.models, and Task Routing with form.task_types) are not associated with their controls; update each to use the shared Label component and add matching htmlFor/id attributes (give each Input/select an id like provider-name, provider-vendor, provider-base-url, provider-api-key-env, provider-api-key, provider-models, provider-task-types) so the Label's htmlFor matches the control id, ensuring consistency with the rest of the page and accessibility support.
529-533:⚠️ Potential issue | 🟠 MajorSeparate the write from the follow-up refresh.
Each mutation shares a
tryblock withload(). If the backend write succeeds and the reload fails, the UI reports the whole action as failed even though state already changed on the server. That is especially risky on create, because users are likely to retry and duplicate the provider.💡 Suggested pattern
- await load() - setDialogOpen(false) - setMessage(editing ? "Provider updated." : "Provider created.") + setDialogOpen(false) + setMessage(editing ? "Provider updated." : "Provider created.") + load().catch((e) => { + setError(`Saved, but refresh failed: ${getErrorMessage(e)}`) + })Apply the same separation to delete/activate so a refresh failure never masquerades as a failed mutation.
Also applies to: 546-549, 559-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/settings/page.tsx` around lines 529 - 533, The current try/catch conflates the backend mutation and the subsequent refresh (load()), causing a successful write to be treated as failed if load() errors; update each mutation handler (the block calling load(), setDialogOpen, setMessage) to first perform the write-only operation inside its own try/catch and handle mutation errors with setError(getErrorMessage(e)), then in a separate try/catch call load() to refresh UI (handle refresh errors separately or log them without marking the mutation as failed). Apply this pattern to the create/edit handler shown (references: load, setDialogOpen, setMessage, setError, getErrorMessage) and mirror the same separation for the delete and activate flows mentioned at the other locations.
🟡 Minor comments (5)
web/src/app/api/auth/register/route.ts-4-13 (1)
4-13:⚠️ Potential issue | 🟡 MinorPlease add/update tests for this new route behavior.
This adds a new API behavior path, but no associated test updates are visible in the provided changes.
As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/auth/register/route.ts` around lines 4 - 13, Add tests exercising the new POST handler in route.ts (function POST) to cover the new behavior: mock fetch calls to backendBaseUrl()/api/auth/register and assert that when the backend returns success the NextResponse.json contains the parsed data and status, and when the backend returns non-JSON or errors the handler returns null body with the backend status. Write tests that simulate both 200 and error statuses, ensure Content-Type is sent, and verify the request body is forwarded; use the project's test runner (jest/vitest) and mock global fetch so tests target the POST function behavior.web/src/lib/config.ts-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorNormalize trailing slash before appending
/api.Current concatenation can produce
//apiwhenPAPERBOT_API_BASE_URLalready ends with/.🔧 Suggested fix
-export const API_BASE_URL = - (process.env.PAPERBOT_API_BASE_URL || "http://127.0.0.1:8000") + "/api" +const base = (process.env.PAPERBOT_API_BASE_URL || "http://127.0.0.1:8000").replace(/\/+$/, "") +export const API_BASE_URL = `${base}/api`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/config.ts` around lines 1 - 2, The API_BASE_URL constant currently concatenates process.env.PAPERBOT_API_BASE_URL with "/api" which can produce a double slash if the env value ends with a slash; update the logic in API_BASE_URL to normalize the base URL by removing any trailing slash (e.g., using .replace(/\/+$/, "") or similar) before appending "/api" so the result is always ".../api" without duplicated slashes.web/src/app/papers/[id]/page.tsx-18-20 (1)
18-20:⚠️ Potential issue | 🟡 MinorRemove unnecessary
as anycast; use the proper Session type already defined.The NextAuth session type already properly augments
SessionwithaccessToken?: string(inweb/src/types/next-auth.d.ts). Using(session as any)?.accessTokenbypasses this type safety without benefit—usesession?.accessTokendirectly, consistent with other pages likeweb/src/app/dashboard/page.tsx(line 489).🔧 Simpler pattern
- const session = await auth() - const accessToken = (session as any)?.accessToken as string | undefined + const session = await auth() + const accessToken = session?.accessToken const paper = await fetchPaperDetails(id, accessToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/papers/`[id]/page.tsx around lines 18 - 20, The code is using an unnecessary cast "(session as any)?.accessToken" which bypasses typing; update the usage to read the access token via the properly typed session object (use "session?.accessToken" after calling auth()), so change the accessToken assignment to use session?.accessToken and pass that into fetchPaperDetails(id, accessToken); ensure the auth() return is the NextAuth Session type (no as any) and adjust imports/types if needed so session has accessToken defined for fetchPaperDetails.web/src/app/api/auth/forgot-password/route.ts-4-12 (1)
4-12:⚠️ Potential issue | 🟡 MinorHandle JSON parsing error for the incoming request.
req.json()on line 5 will throw if the request body is not valid JSON, resulting in an unhandled error. Consider wrapping it with error handling similar to the backend response parsing on line 11.🛡️ Suggested fix
export async function POST(req: NextRequest) { - const body = await req.json() + const body = await req.json().catch(() => null) + if (body === null) { + return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 }) + } const res = await fetch(`${backendBaseUrl()}/api/auth/forgot-password`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/auth/forgot-password/route.ts` around lines 4 - 12, The POST handler currently calls req.json() without guarding against malformed JSON; wrap the req.json() call in a try/catch (inside the POST function) and handle JSON parsing errors by returning a clear NextResponse.json error (using NextResponse.json) with a 400 status and an explanatory message; keep downstream behavior the same when parsing succeeds and still parse backend response with its existing catch logic. Ensure you reference the POST function and the req.json() invocation when making the change so the handler returns a 400 JSON response on invalid incoming JSON instead of throwing.web/src/app/settings/page.tsx-311-318 (1)
311-318:⚠️ Potential issue | 🟡 MinorKeep these icon-only actions keyboard and screen-reader accessible.
The password toggles are removed from the tab order with
tabIndex={-1}, and the provider action buttons only expose atitle. Addaria-labels and leave the toggles focusable so non-pointer users can operate the same controls.Also applies to: 335-342, 633-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/settings/page.tsx` around lines 311 - 318, The password-visibility toggle buttons (e.g., the button using showCurrentPw and setShowCurrentPw with Eye/EyeOff icons) should not be removed from keyboard focus and must be accessible to screen readers; remove tabIndex={-1}, add a descriptive aria-label (e.g., aria-label="Show current password" / "Hide current password" toggled based on showCurrentPw) and keep the onClick handler as-is. Do the same for the other two toggle instances (lines around 335-342 and 633-645) and for the provider action buttons that currently only use title — add explicit aria-label attributes describing their action (e.g., "Sign in with X" or "Disconnect provider X") so the controls are keyboard-focusable and screen-reader accessible while preserving existing click behavior.
🧹 Nitpick comments (11)
web/package.json (1)
59-59: Consider using stablenext-auth@4.24.13instead of beta.Line 59 pins
next-auth@5.0.0-beta.30, a beta version of a security-critical package. A stable version (next-auth@4.24.13, released Oct 29, 2025) is available and officially supports Next.js 16+ and React 19+. Unless beta-specific features are required, prefer the stable release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/package.json` at line 59, Change the pinned dependency "next-auth" from the beta release to the stable release by updating package.json to use "next-auth": "4.24.13"; then regenerate your lockfile (npm install or yarn install) so package-lock.json / yarn.lock reflect the stable version and run the app/test suite to verify compatibility (look for references to "next-auth" in package.json and any imports of NextAuthConfig or NextAuthProvider to catch API differences).web/src/components/research/ResearchDashboard.tsx (1)
100-100: Session data is fetched but never used.The
useSessionhook is called andsessionis destructured, but it's never referenced anywhere in the component. If the intent is to rely on session-based auth, either use the session data (e.g., for conditional rendering or passing to API calls) or remove the unused hook call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/ResearchDashboard.tsx` at line 100, The component ResearchDashboard calls useSession() and destructures session but never uses it; either remove the unused hook/destructuring to avoid dead code or use the session object where needed (e.g., guard rendering, pass session.user or token into API calls like fetchResearchData, or conditionally show auth-only controls). Update the ResearchDashboard component to either delete the line "const { data: session } = useSession()" and any unused import, or reference session in the component logic (conditional JSX, API call params, or feature gating) so the variable is actually used.web/src/components/research/SavedPapersList.tsx (1)
152-152: Session data is fetched but never used.Similar to
ResearchDashboard.tsx, theuseSessionhook is called butsessionis never referenced. If session data isn't needed in this component, consider removing the unused import and hook call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/SavedPapersList.tsx` at line 152, The component calls the useSession hook and assigns const { data: session } = useSession() but never uses session; remove the unused hook call and its import (useSession) from SavedPapersList.tsx, or if session is intended to gate behavior, update logic to reference session where needed (e.g., replace anonymous checks with session?.user). Ensure no leftover references remain and run type checks to remove the unused variable error.web/src/app/login/page.tsx (1)
159-167: Consider adding an accessible label to the password visibility toggle.The toggle button lacks an
aria-label, which may impact screen reader users.Suggested improvement
<button type="button" tabIndex={-1} onClick={() => setShowPassword(v => !v)} className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground transition-colors" + aria-label={showPassword ? "Hide password" : "Show password"} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/login/page.tsx` around lines 159 - 167, The password visibility toggle button (the element using setShowPassword and conditional icons Eye/EyeOff keyed by showPassword) lacks an accessible label; update that button to include an appropriate aria-label (and optionally aria-pressed) that reflects current state (e.g., "Show password" when showPassword is false and "Hide password" when true) so screen readers announce the control and its action, ensuring the label updates when setShowPassword toggles showPassword.web/src/app/reset-password/page.tsx (1)
13-28: Consider hoisting thelevelsarray outside the function.The
levelsarray is recreated on every call togetStrength. Since it's a constant, it could be defined at module scope for a minor performance improvement.♻️ Proposed refactor
+const STRENGTH_LEVELS: Strength[] = [ + { score: 0, label: "", color: "" }, + { score: 1, label: "Weak", color: "bg-red-500" }, + { score: 2, label: "Fair", color: "bg-orange-400" }, + { score: 3, label: "Good", color: "bg-yellow-400" }, + { score: 4, label: "Strong", color: "bg-green-500" }, +] + function getStrength(pw: string): Strength { if (!pw) return { score: 0, label: "", color: "" } let score = 0 if (pw.length >= 8) score++ if (pw.length >= 12) score++ if (/[A-Z]/.test(pw) && /[a-z]/.test(pw)) score++ if (/\d/.test(pw) && /[^A-Za-z0-9]/.test(pw)) score++ - const levels: Strength[] = [ - { score: 0, label: "", color: "" }, - { score: 1, label: "Weak", color: "bg-red-500" }, - { score: 2, label: "Fair", color: "bg-orange-400" }, - { score: 3, label: "Good", color: "bg-yellow-400" }, - { score: 4, label: "Strong", color: "bg-green-500" }, - ] - return levels[score as 0 | 1 | 2 | 3 | 4] + return STRENGTH_LEVELS[score as 0 | 1 | 2 | 3 | 4] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/reset-password/page.tsx` around lines 13 - 28, Hoist the constant strength mapping out of getStrength by moving the levels array to module scope (e.g., const LEVELS or STRENGTH_LEVELS) so it is not recreated on every call; keep its contents identical and update getStrength to return LEVELS[score as 0|1|2|3|4] instead of the inline levels variable.web/src/auth.ts (1)
80-86: Remove unnecessaryas anycasts in session callback using existing type augmentation.The
web/src/types/next-auth.d.tsfile already extends the Session interface withaccessToken,userId, andprovider, so theas anycasts in lines 81-83 are unnecessary. Update to:async session({ session, token }) { session.accessToken = token.accessToken session.userId = token.userId session.provider = token.provider if (token.name !== undefined) session.user.name = token.name as string return session },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/auth.ts` around lines 80 - 86, The session callback is using unnecessary "as any" casts when assigning token values to the augmented Session; remove the casts and assign directly (i.e., set session.accessToken = token.accessToken, session.userId = token.userId, session.provider = token.provider) so the existing type augmentation in next-auth.d.ts is used; keep the token.name handling (session.user.name = token.name as string) as-is if you still need that cast.web/src/components/dashboard/ActivityFeed.tsx (1)
16-20: Avoidas anytype assertion for session access.The
(session as any)?.accessTokenpattern bypasses TypeScript's type checking. Consider extending the session type in your NextAuth configuration to includeaccessToken, or create a typed helper function.♻️ Suggested improvement
If you have a typed session interface (e.g., in
@/author NextAuth config), use it:- const accessToken = (session as any)?.accessToken as string | undefined + const accessToken = session?.accessTokenThis requires augmenting the NextAuth session type in your auth configuration to include
accessToken: string | undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboard/ActivityFeed.tsx` around lines 16 - 20, The code uses an unsafe assertion `(session as any)?.accessToken`; replace this with a proper typed session access by defining/using a Session type that includes accessToken (or a small helper). Update your NextAuth session augmentation or add an interface like SessionWithAccessToken and cast to that type when calling auth(), then read `accessToken` from that typed session; modify the block around auth() and fetchActivities to use the typed session (functions: auth, fetchActivities, variable: activities) rather than `as any`.web/src/app/api/runbook/delete/route.ts (1)
11-14: Use a static import for consistency.Same feedback as
revert-project/route.ts— prefer a static import at the top of the file rather than the inline dynamic import pattern.♻️ Suggested refactor
export const runtime = "nodejs" +import { withBackendAuth } from "../../_utils/auth-headers" + function apiBaseUrl() { return process.env.PAPERBOT_API_BASE_URL || "http://127.0.0.1:8000" } export async function POST(req: Request) { const body = await req.text() const upstream = await fetch(`${apiBaseUrl()}/api/runbook/delete`, { method: "POST", - headers: await (await import("../../_utils/auth-headers")).withBackendAuth(req, { + headers: await withBackendAuth(req, { "Content-Type": req.headers.get("content-type") || "application/json", Accept: "application/json", }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/runbook/delete/route.ts` around lines 11 - 14, Replace the inline dynamic import of "../../_utils/auth-headers" in the headers construction with a static top-level import of the module and call its exported withBackendAuth function directly; locate the usage of withBackendAuth in route.ts (the headers: await (await import("../../_utils/auth-headers")).withBackendAuth(req, {...}) expression) and change it to use the statically imported withBackendAuth(req, {...}) so the module is imported once at file load time for consistency with revert-project/route.ts.web/src/app/api/runbook/revert-project/route.ts (1)
11-14: Use a static import for consistency with other routes.Other routes in this PR (e.g.,
studio/chat/route.ts,research/repro/context/route.ts) use static imports forwithBackendAuth. The double-await pattern here adds unnecessary complexity.♻️ Suggested refactor
export const runtime = "nodejs" +import { withBackendAuth } from "../../_utils/auth-headers" + function apiBaseUrl() { return process.env.PAPERBOT_API_BASE_URL || "http://127.0.0.1:8000" } export async function POST(req: Request) { const body = await req.text() const upstream = await fetch(`${apiBaseUrl()}/api/runbook/revert-project`, { method: "POST", - headers: await (await import("../../_utils/auth-headers")).withBackendAuth(req, { + headers: await withBackendAuth(req, { "Content-Type": req.headers.get("content-type") || "application/json", Accept: "application/json", }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/runbook/revert-project/route.ts` around lines 11 - 14, Replace the dynamic double-await import of withBackendAuth in the headers object with a static import at the top of the module and call that function directly; specifically, add a static import for withBackendAuth and then use withBackendAuth(req, { "Content-Type": req.headers.get("content-type") || "application/json", Accept: "application/json" }) instead of await (await import("../../_utils/auth-headers")).withBackendAuth(...), to match other routes and remove the unnecessary double-await pattern.web/src/lib/api.ts (1)
88-90: Extract a shared auth-header helper to remove repeated token plumbing.The same
headersconstruction is duplicated across multiple functions; centralizing it will reduce drift and make future auth changes safer.♻️ Suggested refactor
+function authHeaders(accessToken?: string): Record<string, string> { + return accessToken ? { Authorization: `Bearer ${accessToken}` } : {} +} ... - const headers: Record<string, string> = {} - if (accessToken) headers["Authorization"] = `Bearer ${accessToken}` + const headers = authHeaders(accessToken)Also applies to: 122-124, 165-167, 208-210, 306-310, 648-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/api.ts` around lines 88 - 90, Several fetch calls in web/src/lib/api.ts duplicate the same headers construction (const headers: Record<string, string> = {} / if (accessToken) headers["Authorization"] = `Bearer ${accessToken}`) — extract a small helper like getAuthHeaders(accessToken?: string): Record<string,string> and replace the duplicated blocks (including the occurrences around the saved papers fetch and the ranges noted) to call getAuthHeaders(accessToken) and pass its result to fetch; update all fetch invocations that currently build a local headers object (the blocks at ~88-90, 122-124, 165-167, 208-210, 306-310, 648-650) to use this helper so token handling is centralized and future changes only touch getAuthHeaders.web/src/app/settings/page.tsx (1)
158-240: Please add coverage for the new settings flows.This refactor introduces session-dependent account mutations, a destructive delete flow, and provider-management CRUD, but the PR context does not include corresponding tests. Please add at least a few RTL/integration cases around the OAuth/password branch, delete confirmation, and provider create/update flows. As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.Also applies to: 448-798
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/settings/page.tsx` around lines 158 - 240, Add RTL/integration tests covering the new session-dependent account flows in AccountSection: write tests that (1) assert branching by mocking session.provider (isOAuth) to exercise the OAuth path vs password-change UI and ensure changePassword handles success and mismatched confirmPw, (2) exercise saveName by mocking PATCH /api/auth/me to return display_name and asserting updateSession was called and success/err messages show, (3) exercise deleteAccount by mocking DELETE /api/auth/me to return ok and non-ok (error body) and verify the confirmation flow, error rendering (deleteErr) and signOut redirect on success, and (4) add provider create/update flow tests if provider-management components exist (mock network responses and assert UI updates). Target functions/components: AccountSection, saveName, changePassword, deleteAccount and useSession/signOut mocks; use fetch/fetchJson mocks or msw to simulate API responses and assert UI messages/loading states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e1e8521-7dd8-4627-ac1a-395e7e5279a2
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
src/paperbot/api/routes/chat.pysrc/paperbot/api/routes/gen_code.pysrc/paperbot/api/routes/harvest.pysrc/paperbot/api/routes/intelligence.pysrc/paperbot/api/routes/memory.pysrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/research.pysrc/paperbot/application/services/identity_resolver.pysrc/paperbot/infrastructure/stores/paper_store.pytests/unit/test_intelligence_routes.pyweb/next.config.tsweb/package.jsonweb/src/app/api/_utils/auth-headers.tsweb/src/app/api/auth/[...nextauth]/route.tsweb/src/app/api/auth/forgot-password/route.tsweb/src/app/api/auth/login-check/route.tsweb/src/app/api/auth/me/change-password/route.tsweb/src/app/api/auth/me/route.tsweb/src/app/api/auth/register/route.tsweb/src/app/api/auth/reset-password/route.tsweb/src/app/api/gen-code/route.tsweb/src/app/api/research/_base.tsweb/src/app/api/research/paperscool/daily/route.tsweb/src/app/api/research/repro/context/route.tsweb/src/app/api/research/tracks/[trackId]/context/route.tsweb/src/app/api/runbook/delete/route.tsweb/src/app/api/runbook/revert-project/route.tsweb/src/app/api/runbook/smoke/route.tsweb/src/app/api/studio/chat/route.tsweb/src/app/dashboard/page.tsxweb/src/app/forgot-password/page.tsxweb/src/app/layout.tsxweb/src/app/login/page.tsxweb/src/app/papers/[id]/page.tsxweb/src/app/papers/page.tsxweb/src/app/register/page.tsxweb/src/app/research/page.tsxweb/src/app/reset-password/page.tsxweb/src/app/settings/page.tsxweb/src/auth.tsweb/src/components/dashboard/ActivityFeed.tsxweb/src/components/dashboard/DashboardReadingQueuePanel.tsxweb/src/components/dashboard/TrackSpotlightSection.tsxweb/src/components/layout/Sidebar.tsxweb/src/components/research/DiscoveryGraphWorkspace.tsxweb/src/components/research/FeedTab.tsxweb/src/components/research/MemoryTab.tsxweb/src/components/research/ResearchDashboard.tsxweb/src/components/research/ResearchDiscoveryPage.tsxweb/src/components/research/ResearchPageNew.tsxweb/src/components/research/SavedPapersList.tsxweb/src/components/research/SavedTab.tsxweb/src/components/scholars/ScholarsWatchlist.tsxweb/src/hooks/useContextPackGeneration.tsweb/src/lib/api.tsweb/src/lib/config.tsweb/src/lib/dashboard-api.tsweb/src/middleware.tsweb/src/types/next-auth.d.ts
💤 Files with no reviewable changes (1)
- web/src/hooks/useContextPackGeneration.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/paperbot/api/routes/harvest.py
- src/paperbot/api/routes/repro_context.py
- src/paperbot/api/routes/memory.py
| async def chat(request: ChatRequest, user_id: str = Depends(get_required_user_id)): | ||
| """ | ||
| Chat with PaperBot AI and stream response. | ||
|
|
||
| Returns Server-Sent Events with streaming text. | ||
| """ | ||
| trace_id = new_trace_id() | ||
| # Override any client-provided user_id with authenticated user | ||
| request.user_id = user_id | ||
| return sse_response(chat_stream(request, trace_id=trace_id), workflow="chat", trace_id=trace_id) |
There was a problem hiding this comment.
Add route tests for the new auth dependency path.
This changed endpoint behavior (dependency-required user identity and payload override), but no corresponding chat-route test updates are included in the provided changes.
Please add tests for: unauthenticated request rejection, authenticated request success, and precedence of dependency-injected user_id over any body-provided value.
As per coding guidelines {src,web/src}/**/*.{py,ts,tsx}: “If behavior changes, add or update tests”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/chat.py` around lines 133 - 142, Add route tests for
the updated chat endpoint (function chat) to cover the new auth dependency path:
1) unauthenticated rejection — call the chat route without satisfying
get_required_user_id and assert it returns the appropriate 401/unauthorized
response; 2) authenticated success — mock or override get_required_user_id to
return a test user_id, call chat and assert the endpoint returns a successful
SSE/stream response (or expected status) and that chat_stream is invoked; 3)
dependency precedence — call chat with an authenticated get_required_user_id
value but include a different user_id in the request body and assert that the
dependency-injected user_id (from get_required_user_id) is used (e.g., by
inspecting arguments to chat_stream or final behavior), ensuring request.user_id
is overridden; place tests alongside existing route tests and use the same test
client/fixtures and mocking approach used in other route tests.
| # 0a. library_paper_id hint → already-resolved internal ID, use directly | ||
| lib_id = hints.get("library_paper_id") | ||
| if lib_id is not None: | ||
| try: | ||
| return int(lib_id) | ||
| except (TypeError, ValueError): | ||
| pass |
There was a problem hiding this comment.
Do not return library_paper_id without validating it exists.
Line 56 returns an internal ID directly, unlike Line 61-67 numeric path which validates against PaperModel. This can return nonexistent IDs.
Proposed fix
lib_id = hints.get("library_paper_id")
if lib_id is not None:
try:
- return int(lib_id)
+ candidate_id = int(lib_id)
+ with self._provider.session() as session:
+ row = session.execute(
+ select(PaperModel).where(PaperModel.id == candidate_id)
+ ).scalar_one_or_none()
+ if row:
+ return int(row.id)
except (TypeError, ValueError):
pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/application/services/identity_resolver.py` around lines 52 - 58,
The code currently returns the provided hints["library_paper_id"] as an int
without verifying the ID exists; update the branch in identity_resolver.py that
handles lib_id to parse lib_id to int and then validate the ID against the
PaperModel (e.g., use PaperModel.get_by_id / PaperModel.exists or an equivalent
lookup) before returning it, and fall back to the existing resolution flow if
the lookup fails or the ID is not found; ensure you catch TypeError/ValueError
on parsing and treat non-existent IDs as invalid rather than returning them.
| console.warn("[auth-headers] session.accessToken is missing", { | ||
| hasSession: !!session, | ||
| userId: (session as any)?.userId, | ||
| provider: (session as any)?.provider, | ||
| }) | ||
| } | ||
| } catch (e) { | ||
| console.error("[auth-headers] auth() threw:", e) | ||
| } |
There was a problem hiding this comment.
Remove user-identifying and raw auth error logging from auth path.
Line 27–35 currently logs userId and raw thrown errors in an auth utility. That can leak identifiers and sensitive auth diagnostics into logs.
🔒 Suggested hardening
- } else {
- console.warn("[auth-headers] session.accessToken is missing", {
- hasSession: !!session,
- userId: (session as any)?.userId,
- provider: (session as any)?.provider,
- })
- }
- } catch (e) {
- console.error("[auth-headers] auth() threw:", e)
+ } else {
+ console.warn("[auth-headers] session access token is missing", {
+ hasSession: !!session,
+ })
+ }
+ } catch {
+ console.error("[auth-headers] auth() failed")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/api/_utils/auth-headers.ts` around lines 27 - 35, The auth path
currently logs user-identifying fields and raw error objects; update the
auth-headers logic (the auth() handling and the console.warn/console.error
calls) to stop emitting session.userId or session.provider and to avoid logging
raw thrown errors—remove those fields from the console.warn payload and replace
console.error("[auth-headers] auth() threw:", e) with a generic, non-sensitive
message (e.g., console.error("[auth-headers] auth() failed")) or log a sanitized
error id/message only; ensure you only emit non-identifying debug flags like
hasSession or a boolean failure indicator.
| export async function GET(req: NextRequest) { | ||
| const headers = await withBackendAuth(req) | ||
| const res = await fetch(`${backendBaseUrl()}/api/auth/me`, { headers }) | ||
| const data = await res.json().catch(() => null) | ||
| return NextResponse.json(data, { status: res.status }) | ||
| } | ||
|
|
||
| export async function PATCH(req: NextRequest) { | ||
| const body = await req.json() | ||
| const headers = await withBackendAuth(req, { "Content-Type": "application/json" }) | ||
| const res = await fetch(`${backendBaseUrl()}/api/auth/me`, { | ||
| method: "PATCH", | ||
| headers, | ||
| body: JSON.stringify(body), | ||
| }) | ||
| const data = await res.json().catch(() => null) | ||
| return NextResponse.json(data, { status: res.status }) | ||
| } | ||
|
|
||
| export async function DELETE(req: NextRequest) { | ||
| const headers = await withBackendAuth(req) | ||
| const res = await fetch(`${backendBaseUrl()}/api/auth/me`, { method: "DELETE", headers }) | ||
| return new NextResponse(null, { status: res.status }) | ||
| } |
There was a problem hiding this comment.
Add guarded upstream fetch handling (timeout + network-failure mapping).
These handlers call backend fetch without timeout/catch. A backend outage or socket hang will bubble up as uncaught 500s instead of a controlled 502/504.
🛠️ Suggested pattern
+async function safeProxy(url: string, init: RequestInit) {
+ try {
+ const res = await fetch(url, { ...init, signal: AbortSignal.timeout(10_000) })
+ return res
+ } catch {
+ return null
+ }
+}
+
export async function GET(req: NextRequest) {
const headers = await withBackendAuth(req)
- const res = await fetch(`${backendBaseUrl()}/api/auth/me`, { headers })
+ const res = await safeProxy(`${backendBaseUrl()}/api/auth/me`, { headers })
+ if (!res) return NextResponse.json({ detail: "Service unavailable" }, { status: 502 })
const data = await res.json().catch(() => null)
return NextResponse.json(data, { status: res.status })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/api/auth/me/route.ts` around lines 4 - 27, The GET, PATCH and
DELETE handlers call fetch (via backendBaseUrl()) without any timeout or error
mapping, so backend outages or socket hangs propagate uncaught; create and use a
guarded fetch wrapper (e.g., guardedFetch) that accepts URL, options and a
timeout, uses AbortController to enforce the timeout, catches fetch errors and
maps network failures to a 502 response and timeouts to a 504, and returns the
original Response on success; replace direct fetch calls in GET, PATCH and
DELETE with guardedFetch and ensure withBackendAuth headers are passed through
to the wrapper.
| const body = await req.json() | ||
| const res = await fetch(`${backendBaseUrl()}/api/auth/register`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }) |
There was a problem hiding this comment.
Handle invalid request JSON and upstream fetch failures explicitly.
Line 5 and Line 6 can throw, which currently turns expected client/upstream errors into generic 500s. Please map these cases to explicit statuses (e.g., 400 for malformed JSON, 502 for upstream failure).
Suggested fix
export async function POST(req: NextRequest) {
- const body = await req.json()
- const res = await fetch(`${backendBaseUrl()}/api/auth/register`, {
- method: "POST",
- headers: { "Content-Type": "application/json" },
- body: JSON.stringify(body),
- })
+ let body: unknown
+ try {
+ body = await req.json()
+ } catch {
+ return NextResponse.json({ detail: "Invalid JSON body" }, { status: 400 })
+ }
+
+ let res: Response
+ try {
+ res = await fetch(`${backendBaseUrl()}/api/auth/register`, {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify(body),
+ })
+ } catch {
+ return NextResponse.json({ detail: "Auth service unavailable" }, { status: 502 })
+ }
const data = await res.json().catch(() => null)
return NextResponse.json(data, { status: res.status })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/api/auth/register/route.ts` around lines 5 - 10, The handler
currently calls req.json() and then
fetch(`${backendBaseUrl()}/api/auth/register`, ...) without distinguishing
parsing or upstream failures; update the route logic to catch JSON parsing
errors from req.json() and return a 400 response, and catch network/response
failures from fetch() (or non-2xx upstream responses) and return a 502 response
including any upstream error/message; use the existing backendBaseUrl(), the
req.json() call, and the fetch() call as the points to add try/catch (or promise
error handling) and map errors to the appropriate HTTP statuses.
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| track_id: String(trackId), | ||
| limit: "100", | ||
| }) | ||
| const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/memory/inbox`) |
There was a problem hiding this comment.
Query parameters are built but never used in the fetch call.
The qs URLSearchParams is constructed with track_id and limit but is not appended to the fetch URL. The API call will always hit /api/research/memory/inbox without any filtering parameters.
🐛 Proposed fix
const qs = new URLSearchParams({
track_id: String(trackId),
limit: "100",
})
- const res = await fetch(`/api/research/memory/inbox`)
+ const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`)📝 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.
| const qs = new URLSearchParams({ | |
| user_id: userId, | |
| track_id: String(trackId), | |
| limit: "100", | |
| }) | |
| const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`) | |
| const res = await fetch(`/api/research/memory/inbox`) | |
| const qs = new URLSearchParams({ | |
| track_id: String(trackId), | |
| limit: "100", | |
| }) | |
| const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/research/MemoryTab.tsx` around lines 41 - 45, The
URLSearchParams instance qs (built from trackId and limit) is not used when
calling fetch in MemoryTab.tsx; update the fetch call that currently targets
`/api/research/memory/inbox` to include the serialized query string (e.g.,
append `?${qs.toString()}`) so the API receives track_id and limit; keep the
existing qs and String(trackId) usage and ensure you URL-encode or use
qs.toString() when appending.
| const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | ||
| const qs = new URLSearchParams({ format, user_id: userId }) | ||
| const qs = new URLSearchParams({ format }) | ||
| if (trackId != null) qs.set("track_id", String(trackId)) | ||
| try { | ||
| const res = await fetch(`/api/papers/export?${qs.toString()}`) | ||
| const res = await fetch(`/api/papers/export`) |
There was a problem hiding this comment.
Query parameters are built but never used in the fetch call.
The qs URLSearchParams is constructed with format and optionally track_id, but is not appended to the fetch URL. The export request will not include the format parameter.
🐛 Proposed fix
const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => {
const qs = new URLSearchParams({ format })
if (trackId != null) qs.set("track_id", String(trackId))
try {
- const res = await fetch(`/api/papers/export`)
+ const res = await fetch(`/api/papers/export?${qs.toString()}`)📝 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.
| const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | |
| const qs = new URLSearchParams({ format, user_id: userId }) | |
| const qs = new URLSearchParams({ format }) | |
| if (trackId != null) qs.set("track_id", String(trackId)) | |
| try { | |
| const res = await fetch(`/api/papers/export?${qs.toString()}`) | |
| const res = await fetch(`/api/papers/export`) | |
| const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | |
| const qs = new URLSearchParams({ format }) | |
| if (trackId != null) qs.set("track_id", String(trackId)) | |
| try { | |
| const res = await fetch(`/api/papers/export?${qs.toString()}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/research/SavedTab.tsx` around lines 99 - 103, The fetch in
handleExport builds a URLSearchParams named qs (with format and optional
track_id) but never uses it; update the call in handleExport so the request
includes the query string (append qs.toString() to the `/api/papers/export` URL,
e.g., `/api/papers/export?${qs}`) so the backend receives format and track_id
parameters when calling fetch; ensure you use the existing qs variable and keep
the rest of the try/catch logic intact.
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| sort_by: "saved_at", | ||
| limit: "100", | ||
| }) | ||
| if (trackId != null) { | ||
| qs.set("track_id", String(trackId)) | ||
| } | ||
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/papers/saved`) |
There was a problem hiding this comment.
Query parameters are built but never used in the fetch call.
Same issue as the export handler: qs is constructed with sort_by, limit, and track_id but not appended to the URL. The saved papers request will return unfiltered results.
🐛 Proposed fix
const qs = new URLSearchParams({
sort_by: "saved_at",
limit: "100",
})
if (trackId != null) {
qs.set("track_id", String(trackId))
}
- const res = await fetch(`/api/research/papers/saved`)
+ const res = await fetch(`/api/research/papers/saved?${qs.toString()}`)📝 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.
| const qs = new URLSearchParams({ | |
| user_id: userId, | |
| sort_by: "saved_at", | |
| limit: "100", | |
| }) | |
| if (trackId != null) { | |
| qs.set("track_id", String(trackId)) | |
| } | |
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) | |
| const res = await fetch(`/api/research/papers/saved`) | |
| const qs = new URLSearchParams({ | |
| sort_by: "saved_at", | |
| limit: "100", | |
| }) | |
| if (trackId != null) { | |
| qs.set("track_id", String(trackId)) | |
| } | |
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/research/SavedTab.tsx` around lines 166 - 173, The code
builds a URLSearchParams object named qs (with sort_by, limit, and optional
track_id from trackId) but never uses it in the fetch call; update the fetch
invocation in SavedTab (where qs is created) to append the serialized query
string (e.g., `?${qs.toString()`) to the `/api/research/papers/saved` URL so the
request includes sort_by, limit, and track_id filters; ensure you only append
the `?` when qs is non-empty and reuse the same pattern used in the export
handler for consistency.
|
I pushed I checked these against
This patch does the following:
Validation I ran locally on this PR branch:
One caveat: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 68 changed files in this pull request and generated 13 comments.
Files not reviewed (1)
- web/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
web/src/components/research/SavedPapersList.tsx:385
- This export call relies on
/api/papers/exportto forward the authenticated user context to the backend. Since the backend export endpoint is now protected with required auth, ensure the proxy route attaches the NextAuth session token / Authorization header, otherwise this will consistently return 401 for logged-in users.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | ||
| const qs = new URLSearchParams({ format, user_id: userId }) | ||
| const qs = new URLSearchParams({ format }) | ||
| if (trackId != null) qs.set("track_id", String(trackId)) | ||
| try { | ||
| const res = await fetch(`/api/papers/export?${qs.toString()}`) | ||
| const res = await fetch(`/api/papers/export`) | ||
| if (!res.ok) throw new Error(`${res.status}`) |
| export default function SavedPapersList() { | ||
| const { data: session } = useSession() | ||
| const [items, setItems] = useState<SavedPaperItem[]>([]) |
| async function refreshEval() { | ||
| const qs = new URLSearchParams({ user_id: userId, days: String(evalDays) }) | ||
| const qs = new URLSearchParams({ days: String(evalDays) }) | ||
| if (activeTrackId) qs.set("track_id", String(activeTrackId)) | ||
| const data = await fetchJson<{ summary: EvalSummary }>(`/api/research/evals/summary?${qs.toString()}`) | ||
| const data = await fetchJson<{ summary: EvalSummary }>(`/api/research/evals/summary`) | ||
| setEvalSummary(data.summary) |
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| track_id: String(trackId), | ||
| limit: "100", | ||
| }) | ||
| const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/memory/inbox`) | ||
| if (!res.ok) { |
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| limit: "20", | ||
| offset: "0", | ||
| }) | ||
| const res = await fetch(`/api/research/tracks/${trackId}/feed?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/tracks/${trackId}/feed`) | ||
| if (!res.ok) { |
| export default function ResearchDiscoveryPage() { | ||
| const { data: session } = useSession() | ||
| const searchParams = useSearchParams() | ||
| const [userId] = useState("default") | ||
| const [tracks, setTracks] = useState<Track[]>([]) |
| export default function ResearchDashboard() { | ||
| const [userId, setUserId] = useState("default") | ||
| const { data: session } = useSession() | ||
| const [tracks, setTracks] = useState<Track[]>([]) | ||
| const [activeTrackId, setActiveTrackId] = useState<number | null>(null) | ||
| const [query, setQuery] = useState("") |
| import type { Metadata } from "next"; | ||
| import { Geist, Geist_Mono } from "next/font/google"; | ||
| import "./globals.css"; | ||
| import { LayoutShell } from "@/components/layout/LayoutShell"; | ||
| import { SessionProvider } from "next-auth/react"; | ||
|
|
||
| const geistSans = Geist({ | ||
| variable: "--font-geist-sans", | ||
| subsets: ["latin"], | ||
| }); | ||
|
|
||
| const geistMono = Geist_Mono({ | ||
| variable: "--font-geist-mono", | ||
| subsets: ["latin"], | ||
| }); | ||
|
|
||
| export const metadata: Metadata = { | ||
| title: "PaperBot", | ||
| description: "Research paper analysis and scholar tracking", | ||
| }; | ||
|
|
||
| export default function RootLayout({ | ||
| children, | ||
| }: Readonly<{ | ||
| children: React.ReactNode; | ||
| }>) { | ||
| return ( | ||
| <html lang="en" suppressHydrationWarning> | ||
| <body | ||
| className={`${geistSans.variable} ${geistMono.variable} antialiased`} | ||
| > | ||
| <LayoutShell>{children}</LayoutShell> | ||
| <SessionProvider> | ||
| <LayoutShell>{children}</LayoutShell> | ||
| </SessionProvider> |
| export function TrackSpotlightSection({ | ||
| initialTracks, | ||
| initialActiveTrack, | ||
| initialFeedItems, | ||
| initialFeedTotal, | ||
| initialAnchors, | ||
| userId = "default", | ||
| initialAnchors = "default", | ||
| }: TrackSpotlightSectionProps) { |
| async function refreshInbox(trackId?: number | null) { | ||
| const tid = trackId ?? activeTrackId | ||
| const qs = new URLSearchParams({ user_id: userId }) | ||
| if (tid) qs.set("track_id", String(tid)) | ||
| const data = await fetchJson<{ items: MemoryItem[] }>(`/api/research/memory/inbox?${qs.toString()}`) | ||
| const data = await fetchJson<{ items: MemoryItem[] }>(`/api/research/memory/inbox`) | ||
| setInbox(data.items || []) | ||
| setSelectedInboxIds(new Set()) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 68 changed files in this pull request and generated 15 comments.
Files not reviewed (1)
- web/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| async session({ session, token }) { | ||
| ;(session as any).accessToken = token.accessToken | ||
| ;(session as any).userId = token.userId | ||
| ;(session as any).provider = token.provider | ||
| if (token.name !== undefined) session.user.name = token.name as string | ||
| return session |
| <button | ||
| type="button" | ||
| tabIndex={-1} | ||
| onClick={() => setShowPassword(v => !v)} | ||
| className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground transition-colors" | ||
| > | ||
| {showPassword ? <EyeOff className="h-4 w-4" /> : <Eye className="h-4 w-4" />} | ||
| </button> |
| const handleExport = async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | ||
| const qs = new URLSearchParams({ format, user_id: userId }) | ||
| const qs = new URLSearchParams({ format }) | ||
| if (trackId != null) qs.set("track_id", String(trackId)) | ||
| try { | ||
| const res = await fetch(`/api/papers/export?${qs.toString()}`) | ||
| const res = await fetch(`/api/papers/export`) | ||
| if (!res.ok) throw new Error(`${res.status}`) |
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| sort_by: "saved_at", | ||
| limit: "100", | ||
| }) | ||
| if (trackId != null) { | ||
| qs.set("track_id", String(trackId)) | ||
| } | ||
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/papers/saved`) | ||
| if (!res.ok) { |
| export default function SavedPapersList() { | ||
| const { data: session } = useSession() | ||
| const [items, setItems] = useState<SavedPaperItem[]>([]) |
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| track_id: String(trackId), | ||
| limit: "100", | ||
| }) | ||
| const res = await fetch(`/api/research/memory/inbox?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/memory/inbox`) | ||
| if (!res.ok) { |
| const qs = new URLSearchParams({ | ||
| user_id: userId, | ||
| limit: "20", | ||
| offset: "0", | ||
| }) | ||
| const res = await fetch(`/api/research/tracks/${trackId}/feed?${qs.toString()}`) | ||
| const res = await fetch(`/api/research/tracks/${trackId}/feed`) | ||
| if (!res.ok) { |
| <button | ||
| type="button" | ||
| tabIndex={-1} | ||
| onClick={() => setShowNewPw((v) => !v)} | ||
| className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground transition-colors" | ||
| > | ||
| {showNewPw ? <EyeOff className="h-4 w-4" /> : <Eye className="h-4 w-4" />} | ||
| </button> |
| const baseHeaders = { | ||
| method, | ||
| Accept: "application/json", | ||
| "Content-Type": req.headers.get("content-type") || "application/json", | ||
| } as Record<string, string> | ||
| const { withBackendAuth } = await import("../_utils/auth-headers") | ||
| const headers = await withBackendAuth(req, baseHeaders) |
| import { auth } from "@/auth" | ||
|
|
||
| export function backendBaseUrl(): string { | ||
| return process.env.BACKEND_BASE_URL || "http://127.0.0.1:8000" |
- Fix IDOR: always assign JWT-derived user_id, ignore request body user_id in gen_code, memory, and repro_context routes - Fix AttributeError: replace bare _research_store/_memory_store with _get_research_store()/_get_memory_store() in research.py; replace _store with _get_store() in memory.py - Fix missing import: add wrap_generator to gen_code.py, add sse_response to repro_context.py - Remove unused StreamingResponse import from harvest.py - Remove redundant effective_user_id variable in research.py
…erry609#372 review - repro_context: add Depends(get_user_id) + ownership checks to all context-pack routes - research: replace all bare _memory_store/_research_store/_track_router accesses with lazy getters; fix get_track_context to use Depends(get_user_id)
f552389 to
9146582
Compare
- Fix IDOR: always assign JWT-derived user_id, ignore request body user_id in gen_code, memory, and repro_context routes - Fix AttributeError: replace bare _research_store/_memory_store with _get_research_store()/_get_memory_store() in research.py; replace _store with _get_store() in memory.py - Fix missing import: add wrap_generator to gen_code.py, add sse_response to repro_context.py - Remove unused StreamingResponse import from harvest.py - Remove redundant effective_user_id variable in research.py
…372 review - repro_context: add Depends(get_user_id) + ownership checks to all context-pack routes - research: replace all bare _memory_store/_research_store/_track_router accesses with lazy getters; fix get_track_context to use Depends(get_user_id)
…ed to #151
Summary by CodeRabbit
Refactor
New Features
Bug Fixes