feat(Harvest): add -- Paper Search and Storage#27
Conversation
📝 WalkthroughWalkthroughRegisters a new Harvest API and adds a complete multi-source harvesting subsystem: harvesters (ArXiv, Semantic Scholar, OpenAlex), domain models, pipeline orchestration, deduplication, storage (PaperStore + migrations), API routes for search/library/harvest runs, frontend wiring, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as "API Route\n(/harvest)"
participant Pipeline as "HarvestPipeline"
participant Rewriter as "QueryRewriter"
participant Recommender as "VenueRecommender"
participant Arxiv as "ArxivHarvester"
participant S2 as "SemanticScholarHarvester"
participant OpenAlex as "OpenAlexHarvester"
participant Dedup as "PaperDeduplicator"
participant Store as "PaperStore"
Client->>API: POST /harvest (config)
API->>Pipeline: run(config)
Pipeline->>Rewriter: rewrite(keywords)
Rewriter-->>Pipeline: expanded keywords
Pipeline->>Recommender: recommend(venues)
Recommender-->>Pipeline: venue suggestions
Pipeline->>Arxiv: search(...)
Arxiv-->>Pipeline: papers (arXiv)
Pipeline->>S2: search(...)
S2-->>Pipeline: papers (Semantic Scholar)
Pipeline->>OpenAlex: search(...)
OpenAlex-->>Pipeline: papers (OpenAlex)
Pipeline->>Dedup: deduplicate(all papers)
Dedup-->>Pipeline: deduplicated papers
Pipeline->>Store: upsert_papers_batch(...)
Store-->>Pipeline: upsert result
Pipeline-->>API: stream progress + final result
API-->>Client: SSE events + final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
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 (1)
web/src/lib/api.ts (1)
187-189:⚠️ Potential issue | 🟡 Minor
fetchScholarDetailsnow implicitly depends on a live backend.
fetchScholarDetailscallsfetchPapers()(Line 188), which now hits the real API instead of returning mock data. This means the scholar details page will fail or show empty publications if the backend is unavailable, even though all other data in this function is still mocked.
🤖 Fix all issues with AI agents
In `@alembic/versions/0003_paper_harvest_tables.py`:
- Around line 54-59: The upgrade() function returns early when _is_offline() is
true, so _upgrade_create_indexes() is never called and generated offline SQL
omits index creation; remove the early return or restructure upgrade() so it
always calls _upgrade_create_indexes() after _upgrade_create_tables() (rely on
_create_index/op.create_index which already handles offline mode) — modify
upgrade() to invoke _upgrade_create_tables() then _upgrade_create_indexes()
regardless of _is_offline() so indexes are emitted in offline-generated SQL.
In `@src/paperbot/api/routes/harvest.py`:
- Around line 323-331: get_user_library currently accepts user_id from the query
string (user_id param in get_user_library) which allows callers to access/modify
arbitrary users' libraries; change this to derive the user ID from the
authenticated session/token (e.g., replace the user_id Query param with a
dependency that extracts the current_user/current_user.id from your auth
middleware or token validation function used elsewhere) and remove the default
"default" value; apply the same change to the other endpoint that accepts
user_id in the request body (lines referenced) so both endpoints use the
authenticated user context; if no authentication exists for this MVP, add a
clear TODO comment inside get_user_library and the other endpoint stating that
user_id is temporary and must be replaced with auth-derived user identification
before production.
- Around line 390-414: save_paper_to_library creates a new
SqlAlchemyResearchStore() per request and never closes it, leaking DB resources;
change this to reuse a shared/lazy-initialized research store or inject one
instead of instantiating inline: add a module-level lazy getter (similar to
_get_paper_store) or accept a dependency and use that shared instance, then call
the store's close/dispose method if it has one after record_paper_feedback (or
rely on the shared instance's lifecycle); update references in
save_paper_to_library and any other handlers that instantiate
SqlAlchemyResearchStore() so they use the new get_research_store() or injected
research_store and remove the per-request constructor.
- Line 52: The Field declaration for keywords in HarvestRequest uses the
deprecated Pydantic v1 arg min_items which causes a TypeError; update the Field
call on the keywords attribute (keywords: List[str] = Field(..., min_items=1,
description="Search keywords")) to use min_length=1 instead (i.e., replace
min_items with min_length) so Pydantic v2 accepts the constraint. Ensure the
change is made on the keywords field in the HarvestRequest/schema where Field is
used.
In `@src/paperbot/api/routes/research.py`:
- Around line 693-706: The feedback currently records mixed IDs because
actual_paper_id is replaced with the internal DB ID on save; change the call to
_research_store.add_paper_feedback to always use the original external paper id
from the request (e.g., req.paper_id or a saved variable representing the
external id) as the paper_id parameter, and move the internal library_paper_id
(library_paper_id) into the metadata argument (meta) before calling
add_paper_feedback; keep returning PaperFeedbackResponse(feedback=fb,
library_paper_id=library_paper_id) so the client still sees the internal id
while the feedback table consistently stores the external identifier.
- Around line 677-688: The route handler directly imports PaperModel and uses
paper_store._provider.session() with a SQLAlchemy select
(PaperModel.semantic_scholar_id ...) which breaks the layering; add or use a
PaperStore method like get_paper_by_source_id(source, source_id) on the existing
paper_store to encapsulate the lookup, remove the direct ORM import and session
usage in the handler, call
paper_store.get_paper_by_source_id("semantic_scholar", req.paper_id) to obtain
the Paper entity (then extract id/actual_paper_id from that returned
domain/model), and update tests or callers accordingly.
- Around line 652-691: The save-to-library flow hardcodes
HarvestSource.SEMANTIC_SCHOLAR and constructs a new PaperStore per request;
update PaperFeedbackRequest to include an optional paper_source and use that
when building the HarvestedPaper (falling back to
HarvestSource.SEMANTIC_SCHOLAR), and replace the per-request PaperStore()
instantiation by using a module-scoped or lazy singleton store (e.g.,
_research_store) so this handler reuses the same PaperStore instance when
calling upsert_papers_batch and accessing _provider.session().
In `@src/paperbot/context_engine/engine.py`:
- Line 506: The Logger.info call on the Paper search config line exceeds the
100-char limit; modify the call in engine.py (the Logger.info invocation
referencing self.config.offline and self.config.paper_limit and
LogFiles.HARVEST) to wrap or split the formatted message so each line stays
under 100 characters (for example build the message into a shorter variable or
use multiple string parts concatenated across lines) without changing the logged
content or the LogFiles.HARVEST target.
In `@src/paperbot/domain/harvest.py`:
- Around line 87-111: from_dict can raise ValueError when constructing
HarvestSource from an empty/invalid string; wrap the conversion in a safe parse
(in from_dict) by attempting HarvestSource(source) in a try/except and falling
back to a safe value (e.g., HarvestSource.UNKNOWN if you add that enum member,
or None/default enum) so deserialization never throws; update the from_dict
logic around the source handling and, if adding HarvestSource.UNKNOWN, add that
enum member to HarvestSource so the fallback is available.
In `@src/paperbot/infrastructure/harvesters/openalex_harvester.py`:
- Around line 42-45: Add an HTTP timeout to prevent indefinite hangs by creating
the aiohttp session with a ClientTimeout in _get_session (use
aiohttp.ClientTimeout with an appropriate total/Connect/read values) instead of
a bare aiohttp.ClientSession, and ensure individual calls that use
session.get(...) also pass a timeout (or rely on the session default) so
requests to the OpenAlex API cannot block forever; apply the same changes to the
ArxivHarvester equivalent methods.
In `@src/paperbot/infrastructure/stores/paper_store.py`:
- Around line 318-331: The join currently casts PaperFeedbackModel.paper_id to
Integer (in base_stmt) which will raise on PostgreSQL for non-numeric strings;
replace the direct cast with a safe conditional expression (e.g., build a
case/boolean expression that first checks
PaperFeedbackModel.paper_id.op("~")(r"^\d+$") then compares
cast(PaperFeedbackModel.paper_id, Integer) only when the regex matches) and use
that safe_int_match in the or_ alongside PaperModel.semantic_scholar_id ==
PaperFeedbackModel.paper_id; update the join condition in base_stmt (where
cast(...) is referenced) to use the safe case/regex approach so non-numeric
paper_id values don’t trigger a CAST error (alternatively, normalize paper_id at
write time to always store numeric IDs).
- Around line 244-251: The current falsy checks (if year_from:, if year_to:, if
min_citations:) skip valid zero values; change them to explicit None checks
(e.g., use "is not None") so filters on PaperModel.year and
PaperModel.citation_count are applied when year_from, year_to or min_citations
equal 0. Update the conditionals around the stmt.where calls that reference
PaperModel.year and PaperModel.citation_count (the blocks using year_from,
year_to, min_citations) to use "is not None" checks to ensure zero is treated as
a valid filter value.
- Around line 209-278: The search_papers method currently ignores the keywords
parameter and unsafely uses getattr(PaperModel, sort_by...), so remove or
implement keywords filtering and restrict sort_by to a whitelist: update
search_papers to either apply keywords (e.g., build OR ilike conditions on
PaperModel.title/abstract/keywords) or delete the unused keywords argument and
its callers, and replace the dynamic getattr with a mapping/whitelist of allowed
sort keys (e.g., {"citation_count": PaperModel.citation_count, "year":
PaperModel.year, "title": PaperModel.title}) then look up sort_col =
ALLOWED_SORT_COLUMNS.get(sort_by, PaperModel.citation_count) before ordering;
reference function search_papers, parameter keywords, parameter sort_by, and
model PaperModel when making the changes.
In `@web/src/app/api/papers/`[paperId]/save/route.ts:
- Around line 7-9: The paperId is being interpolated directly into the upstream
URL in route.ts (upstream and apiBaseUrl()) and in the POST handler, allowing
path-traversal style values; sanitize or encode paperId before use (e.g.,
validate it against an allowed pattern like /^[A-Za-z0-9_-]+$/ or otherwise
reject invalid values, or use a safe encoder such as encodeURIComponent on the
paperId) and then use that sanitized variable for building upstream and any
other requests in the POST handler to prevent crafted path segments from
escaping the intended route.
🟡 Minor comments (9)
web/src/app/api/papers/[paperId]/save/route.ts-13-19 (1)
13-19:⚠️ Potential issue | 🟡 MinorInconsistent query string forwarding between DELETE and POST.
DELETEforwardsurl.searchbutPOSTdoes not. If this is intentional, a brief comment would clarify the design choice. If not, the POST handler should also forward the query string for consistency.src/paperbot/domain/harvest.py-57-62 (1)
57-62:⚠️ Potential issue | 🟡 MinorTitle hash collision on empty/whitespace-only titles.
compute_title_hash()on a paper with an empty title will produce the same hash as any other empty-titled paper, causing false deduplication matches. Consider guarding against this in the deduplicator or returningNone/a sentinel for empty titles.tests/unit/test_harvested_paper.py-95-104 (1)
95-104:⚠️ Potential issue | 🟡 Minor
paper3is assigned but never used — either assert on it or remove it.Ruff flags
paper3(Line 99) as unused (F841). The comment on Lines 103–104 acknowledges the hyphen case may differ, but the test neither asserts equality nor inequality forpaper3. Either add an explicit assertion (e.g.,assert paper1.compute_title_hash() != paper3.compute_title_hash()to document the known divergence) or removepaper3.Proposed fix — assert the known divergence
# All should have same hash after removing punctuation assert paper1.compute_title_hash() == paper2.compute_title_hash() - # Note: hyphens are removed, making it "deeplearning" vs "deep learning" - # This might differ, which is intentional for similar titles + # Hyphen removal joins words: "Deep-Learning?" → "deeplearning" vs "deep learning" + assert paper1.compute_title_hash() != paper3.compute_title_hash()src/paperbot/application/services/venue_recommender.py-133-142 (1)
133-142:⚠️ Potential issue | 🟡 MinorExact matches are double-counted by the partial-match loop.
When
keyword_lowermatches a key exactly, it scores +3 from the exact-match branch (Line 136), then the partial-match loop at Line 140 also fires becausekeyword_lower in mapped_kwisTruewhen they are equal. This adds an extra +1 per venue for every exact match.If the intent is that exact matches should score strictly +3, guard the partial loop:
Proposed fix
# Exact match (highest priority) if keyword_lower in self.mappings: for venue in self.mappings[keyword_lower]: venue_scores[venue] = venue_scores.get(venue, 0) + 3 # Partial match (medium priority) for mapped_kw, venues in self.mappings.items(): - if keyword_lower in mapped_kw or mapped_kw in keyword_lower: + if mapped_kw == keyword_lower: + continue # already scored as exact match + if keyword_lower in mapped_kw or mapped_kw in keyword_lower: for venue in venues: venue_scores[venue] = venue_scores.get(venue, 0) + 1src/paperbot/api/routes/research.py-617-624 (1)
617-624:⚠️ Potential issue | 🟡 MinorMissing tests for the new save-to-library flow.
The
add_paper_feedbackendpoint gained significant new behavior (paper save, library lookup, ID mapping), but no corresponding tests were added. As per coding guidelines, behavior changes should include test updates.As per coding guidelines:
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.src/paperbot/infrastructure/stores/paper_store.py-95-133 (1)
95-133:⚠️ Potential issue | 🟡 Minor
_find_existingdoesn't filter out soft-deleted papers.The lookup queries don't include
PaperModel.deleted_at.is_(None), so a soft-deleted paper will be found and updated rather than creating a new record. If soft-delete is meant to logically remove a paper, re-harvesting should probably create a fresh record or un-delete it explicitly.src/paperbot/infrastructure/stores/paper_store.py-233-241 (1)
233-241:⚠️ Potential issue | 🟡 MinorLIKE pattern does not escape user-supplied wildcards.
The
queryvalue is interpolated directly into the%{query}%pattern. If a user searches for100%, it becomes%100%%, which alters the matching semantics. While this isn't a SQL injection (parameters are bound), it can produce unexpected results.Consider escaping
%and_in the user's query string before wrapping with wildcards.src/paperbot/api/routes/harvest.py-118-118 (1)
118-118:⚠️ Potential issue | 🟡 MinorRemove unused
trace_idassignment.The return value from
set_trace_id()is captured but never used.🧹 Fix
- trace_id = set_trace_id() + set_trace_id()src/paperbot/application/workflows/harvest_pipeline.py-314-335 (1)
314-335:⚠️ Potential issue | 🟡 MinorPipeline-level error handler may fail if the harvest run was never created.
If an exception occurs before
create_harvest_runon Line 208 (e.g., during keyword expansion on Line 187), theupdate_harvest_runcall on Line 317 will silently fail because therun_iddoesn't exist in the DB yet. The error won't be recorded.Consider guarding this with a flag or a try/except around
update_harvest_run.🛡️ Suggested guard
+ run_created = False try: # ... self.paper_store.create_harvest_run(...) + run_created = True # ... except Exception as e: logger.exception(f"Harvest pipeline failed: {e}") - self.paper_store.update_harvest_run( - run_id=run_id, - status="failed", - errors={"pipeline": str(e)}, - ) + if run_created: + self.paper_store.update_harvest_run( + run_id=run_id, + status="failed", + errors={"pipeline": str(e)}, + )
🧹 Nitpick comments (32)
src/paperbot/infrastructure/stores/research_store.py (1)
329-356: Log messages lack contextual identifiers, reducing observability value.These log statements don't include
user_id,track_id, orpaper_id, making them hard to correlate in a multi-user system. Consider including key identifiers:♻️ Suggested improvement
- Logger.info("Recording paper feedback", file=LogFiles.HARVEST) + Logger.info(f"Recording paper feedback user={user_id} track={track_id} paper={paper_id}", file=LogFiles.HARVEST) now = _utcnow() with self._provider.session() as session: ... if track is None: - Logger.error("Track not found", file=LogFiles.HARVEST) + Logger.warning(f"Track not found: track_id={track_id} user={user_id}", file=LogFiles.HARVEST) return None - Logger.info("Creating new feedback record", file=LogFiles.HARVEST) + Logger.info(f"Creating feedback record: paper={paper_id} action={action}", file=LogFiles.HARVEST) ... - Logger.info("Feedback record created successfully", file=LogFiles.HARVEST) + Logger.info(f"Feedback recorded: id={row.id} paper={paper_id}", file=LogFiles.HARVEST)Also, Line 338: "Track not found" is a normal client-input condition (not a system error) —
warninglevel would be more appropriate thanerror.src/paperbot/application/services/__init__.py (1)
6-12:__all__is not sorted (RUF022).Static analysis flags that
__all__entries should be sorted alphabetically.♻️ Proposed fix
__all__ = [ "LLMService", + "PaperDeduplicator", + "QueryRewriter", + "VenueRecommender", "get_llm_service", - "PaperDeduplicator", - "QueryRewriter", - "VenueRecommender", ]src/paperbot/infrastructure/harvesters/__init__.py (1)
9-17: Sort imports and__all__alphabetically.Both the imports and
__all__entries are out of alphabetical order. The static analysis tool (Ruff RUF022) flags__all__as unsorted, and the project's coding guidelines require isort with Black profile.♻️ Proposed fix
-from .arxiv_harvester import ArxivHarvester -from .semantic_scholar_harvester import SemanticScholarHarvester -from .openalex_harvester import OpenAlexHarvester +from .arxiv_harvester import ArxivHarvester +from .openalex_harvester import OpenAlexHarvester +from .semantic_scholar_harvester import SemanticScholarHarvester __all__ = [ "ArxivHarvester", - "SemanticScholarHarvester", "OpenAlexHarvester", + "SemanticScholarHarvester", ]As per coding guidelines, "Use isort with Black profile for Python import sorting".
tests/integration/test_harvesters.py (1)
436-478: Resource cleanup: harvester instances created in interface tests are never closed.Each test in
TestHarvesterInterfaceinstantiates all three harvesters but never callsclose(). While no real HTTP sessions are opened (onlyhasattr/inspectchecks are performed), this could become a problem if harvester constructors ever acquire resources eagerly. Consider adding cleanup or using a fixture with teardown.tests/integration/test_paper_store.py (1)
238-242: Unused unpacked variables flagged by linter.Ruff flags
total(Lines 240, 265, 281) andall_papers(Line 265) as unused. Use_for discarded tuple elements to satisfy the linter.Example fix (applied to all occurrences)
- papers, total = self.store.search_papers(sources=["arxiv"]) + papers, _ = self.store.search_papers(sources=["arxiv"])- all_papers, total = self.store.search_papers(limit=100) + _all_papers, _total = self.store.search_papers(limit=100)- papers, total = self.store.search_papers( + papers, _ = self.store.search_papers(src/paperbot/infrastructure/harvesters/openalex_harvester.py (1)
47-55: Moveimport timeto the module level.
import timeon Line 49 is executed on every_rate_limit()call. While Python caches module imports, the lookup overhead is unnecessary. Move it to the top of the file with other stdlib imports.Proposed fix
At the top of the file (after
import asyncio):import asyncio import logging +import time from typing import Any, Dict, List, OptionalThen in the method:
async def _rate_limit(self) -> None: """Enforce rate limiting between requests.""" - import time - now = time.time()src/paperbot/application/services/venue_recommender.py (1)
25-81: AnnotateDEFAULT_MAPPINGSwithClassVar.Static analysis (RUF012) flags this mutable class attribute. While
.copy()in__init__prevents accidental cross-instance mutation, adding theClassVarannotation makes the intent explicit.Proposed fix
-from typing import Dict, List, Optional +from typing import ClassVar, Dict, List, Optional ... - DEFAULT_MAPPINGS: Dict[str, List[str]] = { + DEFAULT_MAPPINGS: ClassVar[Dict[str, List[str]]] = {tests/unit/test_query_rewriter.py (2)
51-57: Assertion is overly lenient — both the original and expansion should be present.
expand_all(["ML", "deep learning"])should include both"ML"(original) and"machine learning"(expansion). Theorlets the test pass even if the expansion is missing entirely. Consider asserting both:- assert "ML" in expanded or "machine learning" in expanded + assert "ML" in expanded + assert "machine learning" in expanded
92-96: Line 94 likely exceeds the 100-character line limit.Consider splitting across multiple lines.
Proposed fix
- known_abbrevs = ["llm", "ml", "dl", "nlp", "cv", "rl", "gan", "cnn", "rnn", "bert", "gpt", "rag"] + known_abbrevs = [ + "llm", "ml", "dl", "nlp", "cv", "rl", + "gan", "cnn", "rnn", "bert", "gpt", "rag", + ]As per coding guidelines, "Use Black formatting with
line-length = 100for Python code".src/paperbot/application/services/query_rewriter.py (1)
28-68: AnnotateDEFAULT_ABBREVIATIONSwithClassVar.Same as the
VenueRecommenderclass — static analysis (RUF012) flags this mutable class attribute. AddingClassVarmakes the intent explicit.Proposed fix
-from typing import Dict, List, Optional +from typing import ClassVar, Dict, List, Optional ... - DEFAULT_ABBREVIATIONS: Dict[str, str] = { + DEFAULT_ABBREVIATIONS: ClassVar[Dict[str, str]] = {src/paperbot/application/services/paper_deduplicator.py (2)
183-186:_find_indexreturns-1as a sentinel — this could silently corrupt index mappings.If
_find_indexever fails to find the paper (e.g., due to a future code change),-1would be stored in the identifier indexes, pointing at the last element inunique_papersrather than the intended paper. Consider raising an error or using a safer pattern:Proposed fix
def _find_index(self, paper: HarvestedPaper) -> int: """Find the index of a paper in the title hash index.""" title_hash = paper.compute_title_hash() - return self._title_hash_index.get(title_hash, -1) + idx = self._title_hash_index.get(title_hash) + if idx is None: + raise KeyError(f"Paper not found in title hash index: {paper.title!r}") + return idx
179-181:list(set(...))produces non-deterministic ordering for keywords and fields_of_study.This won't cause bugs, but it makes deduplication results non-deterministic across runs (set iteration order varies). If stable output matters for tests or downstream consumers, consider using
dict.fromkeys()to preserve insertion order:- existing.keywords = list(set(existing.keywords + new.keywords)) - existing.fields_of_study = list(set(existing.fields_of_study + new.fields_of_study)) + existing.keywords = list(dict.fromkeys(existing.keywords + new.keywords)) + existing.fields_of_study = list(dict.fromkeys(existing.fields_of_study + new.fields_of_study))alembic/versions/0003_paper_harvest_tables.py (2)
92-94:created_atandupdated_atlack server defaults — NULLs are likely if application code doesn't set them.Consider adding
server_default=sa.func.now()(orsa.text("CURRENT_TIMESTAMP")) so that rows always have a creation timestamp, even if the application layer misses setting one.Proposed fix
- sa.Column("created_at", sa.DateTime(timezone=True), nullable=True), - sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("created_at", sa.DateTime(timezone=True), server_default=sa.text("CURRENT_TIMESTAMP"), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), server_default=sa.text("CURRENT_TIMESTAMP"), nullable=False),
62-117: No unique constraints on deduplication identifier columns (doi, arxiv_id, etc.).The
paperstable relies entirely on the application layer for deduplication. If multiple processes or API calls race, duplicate rows can be inserted. Consider adding unique indexes (at least ondoiandtitle_hash) to enforce deduplication at the database level. Partial unique indexes (e.g.,CREATE UNIQUE INDEX ... WHERE doi IS NOT NULL) would handle the nullable columns.src/paperbot/infrastructure/harvesters/semantic_scholar_harvester.py (2)
28-41: AnnotateFIELDSwithClassVarto satisfy the mutable class-attribute lint.Ruff RUF012 flags mutable class attributes that aren't annotated with
ClassVar. A one-line fix silences the warning and communicates intent.Proposed fix
+from typing import Any, ClassVar, Dict, List, Optional -from typing import Any, Dict, List, Optional ... - FIELDS = [ + FIELDS: ClassVar[List[str]] = [
50-101: Search method is well-structured; one subtle note on year-filter truthiness.Lines 63-68:
if year_from and year_touses truthiness, soyear_from=0would be skipped. Since publication years are always positive this is fine in practice, but usingis not Nonewould be more precise and consistent with how the other harvesters (arXiv) might handle it.Overall the search→filter→return flow, the cap at 100, and the graceful error fallback look good.
More explicit year checks
- if year_from and year_to: + if year_from is not None and year_to is not None: year_filter = f" year:{year_from}-{year_to}" - elif year_from: + elif year_from is not None: year_filter = f" year:{year_from}-" - elif year_to: + elif year_to is not None: year_filter = f" year:-{year_to}"tests/integration/test_harvest_pipeline.py (2)
94-105: Pipeline fixture lacks teardown — potential resource leak across tests.The
pipelinefixture creates aHarvestPipelinewith a SQLite DB but never callsclose(). The error-handling tests (Lines 338, 380, 417) close their locally-created pipelines, but the fixture-based tests don't. Consider adding a yield-based cleanup.Proposed fix
`@pytest.fixture` -def pipeline(tmp_path, mock_harvesters): +async def pipeline(tmp_path, mock_harvesters): """Create HarvestPipeline with mocked dependencies.""" db_url = f"sqlite:///{tmp_path / 'test_harvest.db'}" pipeline = HarvestPipeline(db_url=db_url) # Inject mock harvesters def get_mock_harvester(source): return mock_harvesters.get(source) pipeline._get_harvester = get_mock_harvester - return pipeline + yield pipeline + await pipeline.close()
225-226: Rename unused loop variableitemto_.Ruff B007 flags this. Minor fix for lint compliance.
Proposed fix
- async for item in pipeline.run(config): - pass # Just run to completion + async for _ in pipeline.run(config): + pass # Just run to completionAlso applies to: 267-268
web/src/components/research/SavedPapersList.tsx (1)
99-104: Consider extracting the HTML error-response handling into a shared helper.The same pattern for detecting and sanitizing HTML error responses is duplicated across
loadSavedPapers,unsavePaper, andtoggleReadStatus. A small utility would reduce repetition.Example extraction
async function extractErrorMessage(res: Response): Promise<string> { const text = await res.text() if (text.startsWith("<!DOCTYPE") || text.startsWith("<html")) { return `Server error: ${res.status} ${res.statusText}` } return text }Also applies to: 140-146, 176-181
src/paperbot/infrastructure/harvesters/arxiv_harvester.py (3)
133-162: Version suffix stripping could incorrectly split IDs containing "v" in unusual positions.Line 140-141:
arxiv_id.split("v")[0]splits on the first lowercase"v"character, which works for standard arXiv IDs but is fragile. Using a regex to strip only a trailing version suffix is more robust.Proposed fix
+import re ... # Remove version suffix (e.g., "2301.12345v1" -> "2301.12345") - if "v" in arxiv_id: - arxiv_id = arxiv_id.split("v")[0] + arxiv_id = re.sub(r"v\d+$", "", arxiv_id)
58-74: Same year-filter truthiness note as the S2 harvester.Line 69:
if year_from or year_towill skipyear_from=0. Consistent with the suggestion on the S2 harvester, usingis not Nonewould be more precise.Proposed fix
- if year_from or year_to: + if year_from is not None or year_to is not None: start_date = f"{year_from}0101" if year_from else "199101" end_date = f"{year_to}1231" if year_to else "209912"Note: the inner ternaries on lines 70-71 would also need updating to use
is not Nonefor full consistency.
48-56: Moveimport timeto the top of the file.The
timemodule is imported inside_rate_limit()on every call. It's a stdlib module with negligible import cost — it should be at module level.Proposed fix
import asyncio import logging +import time from typing import List, Optional ... async def _rate_limit(self) -> None: """Enforce rate limiting between requests.""" - import time - now = time.time()src/paperbot/application/workflows/harvest_pipeline.py (3)
216-261: Harvesting is sequential, not parallel as documented.The comment on Line 216 says "Harvest from each source in parallel", but the
forloop on Line 223awaits each harvester one at a time. This means sources are harvested sequentially, which may significantly impact latency with multiple sources.Consider using
asyncio.gather(orasyncio.TaskGroup) for true concurrency. If you intentionally want sequential execution (e.g., to avoid rate-limiting), update the comment to match.♻️ Sketch for parallel harvesting
- # Phase 4: Harvest from each source in parallel - all_papers: List[HarvestedPaper] = [] - - # Build search query from expanded keywords - search_query = " ".join(expanded_keywords) - - # Harvest from each source - for source in sources: - yield HarvestProgress( - phase="Harvesting", - message=f"Fetching from {source}...", - details={"source": source}, - ) - - harvester = self._get_harvester(source) - if harvester is None: - errors[source] = f"Unknown source: {source}" - source_results[source] = {"papers": 0, "error": errors[source]} - continue - - try: - result = await harvester.search( - query=search_query, - max_results=config.max_results_per_source, - year_from=config.year_from, - year_to=config.year_to, - venues=venues, - ) - - all_papers.extend(result.papers) - source_results[source] = { - "papers": result.total_found, - "error": result.error, - } - - if result.error: - errors[source] = result.error - logger.warning(f"Error from {source}: {result.error}") - else: - logger.info(f"Harvested {result.total_found} papers from {source}") - - except Exception as e: - error_msg = str(e) - errors[source] = error_msg - source_results[source] = {"papers": 0, "error": error_msg} - logger.exception(f"Exception harvesting from {source}") + # Phase 4: Harvest from each source + all_papers: List[HarvestedPaper] = [] + search_query = " ".join(expanded_keywords) + + yield HarvestProgress( + phase="Harvesting", + message=f"Fetching from {len(sources)} source(s)...", + ) + + async def _harvest_one(source: str): + harvester = self._get_harvester(source) + if harvester is None: + return source, None, f"Unknown source: {source}" + try: + result = await harvester.search( + query=search_query, + max_results=config.max_results_per_source, + year_from=config.year_from, + year_to=config.year_to, + venues=venues, + ) + return source, result, None + except Exception as e: + return source, None, str(e) + + tasks = [_harvest_one(s) for s in sources] + results = await asyncio.gather(*tasks) + + for source, result, err in results: + if err: + errors[source] = err + source_results[source] = {"papers": 0, "error": err} + logger.warning(f"Error from {source}: {err}") + else: + all_papers.extend(result.papers) + source_results[source] = { + "papers": result.total_found, + "error": result.error, + } + if result.error: + errors[source] = result.error + else: + logger.info( + f"Harvested {result.total_found} papers from {source}" + )
337-355:run_syncis anasyncmethod — the name is misleading.The method is declared
async def run_sync(...)but "sync" typically implies it blocks without requiringawait. Callers still needawait pipeline.run_sync(...). Consider renaming torun_to_completionorrun_collectto better express the intent (consume the generator and return the final result).
357-370: Silently swallowing exceptions during cleanup.Lines 361-364 catch all exceptions with a bare
pass. If a harvester'sclose()fails, the error is completely lost, making debugging difficult.🛡️ Proposed fix: log instead of swallowing
for harvester in self._harvesters.values(): try: await harvester.close() - except Exception: - pass + except Exception: + logger.debug("Error closing harvester", exc_info=True)src/paperbot/api/routes/harvest.py (3)
163-181: DuplicatedHarvestRunResponseconstruction logic.The conversion from
HarvestRunModeltoHarvestRunResponseis repeated verbatim betweenlist_harvest_runsandget_harvest_run. Extract it into a helper to keep things DRY.♻️ Suggested helper
def _run_to_response(run: "HarvestRunModel") -> HarvestRunResponse: return HarvestRunResponse( run_id=run.run_id, keywords=run.get_keywords(), venues=run.get_venues(), sources=run.get_sources(), max_results_per_source=run.max_results_per_source or 50, status=run.status or "unknown", papers_found=run.papers_found or 0, papers_new=run.papers_new or 0, papers_deduplicated=run.papers_deduplicated or 0, errors=run.get_errors(), started_at=run.started_at.isoformat() if run.started_at else None, ended_at=run.ended_at.isoformat() if run.ended_at else None, )Also applies to: 193-206
230-252:PaperResponsemodel is defined but never used.No endpoint uses
PaperResponseas aresponse_model.PaperSearchResponse.papersis typed asList[Dict[str, Any]]andget_paperreturns a raw dict. Consider either usingPaperResponseas the response model (for schema documentation and validation) or removing it to avoid dead code.
264-290:search_papersuses POST for a query-like operation.Using POST for a search endpoint means results are not cacheable and the operation isn't idempotent from an HTTP semantics perspective. GET with query parameters is more conventional for search. If the reason for POST is the complexity of the filter body, this is a common pragmatic choice — but worth a brief comment in the docstring for clarity.
src/paperbot/infrastructure/stores/paper_store.py (4)
59-93: N+1 query pattern inupsert_papers_batch— up to 5 SELECTs per paper.
_find_existingissues up to 5 sequential queries per paper (doi, arxiv_id, semantic_scholar_id, openalex_id, title_hash). For a batch of 200 papers, this could mean ~1000 queries within a single transaction.Consider batching the lookups: pre-fetch all existing papers matching any of the incoming identifiers in a single query (or a small number of queries), then do the upsert loop against the in-memory results.
290-381:get_user_libraryloads all matching rows into memory for sorting/pagination.The method fetches all matching paper-feedback pairs (Line 338), deduplicates in Python, sorts in Python, and then applies offset/limit. For users with large libraries, this won't scale.
The dual-type join (numeric ID vs. Semantic Scholar ID) makes SQL-level pagination difficult, but this should be documented as a known limitation with a TODO for future optimization — e.g., by normalizing
paper_idstorage to always use the numeric DB ID.
498-503:close()swallows all exceptions silently.Same pattern as in the pipeline. Consider logging at debug level.
🧹 Fix
def close(self) -> None: """Close database connections.""" try: self._provider.engine.dispose() - except Exception: - pass + except Exception: + logger.debug("Error disposing engine", exc_info=True)
53-57:Base.metadata.create_allin__init__may conflict with Alembic migrations.The AI summary mentions Alembic migration
0003_paper_harvest_tables.pyfor these tables. Havingauto_create_schema=Trueby default means everyPaperStore()instantiation (including from API route handlers) will callcreate_all, which can create tables outside of Alembic's migration tracking. Consider defaultingauto_create_schematoFalsein production and only using it for tests.
| @router.get("/papers/library", response_model=LibraryResponse) | ||
| def get_user_library( | ||
| user_id: str = Query("default", description="User ID"), | ||
| track_id: Optional[int] = Query(None, description="Filter by track"), | ||
| actions: Optional[str] = Query(None, description="Filter by actions (comma-separated)"), | ||
| sort_by: str = Query("saved_at", description="Sort field"), | ||
| sort_order: str = Query("desc", description="Sort order"), | ||
| limit: int = Query(50, ge=1, le=500), | ||
| offset: int = Query(0, ge=0), |
There was a problem hiding this comment.
user_id accepted from client without authentication — security concern.
user_id is supplied as a query parameter (Line 325) or request body field (Line 386) defaulting to "default". Any caller can read or modify any user's library by passing a different user_id. If this is intentional for an MVP with no auth, consider adding a TODO/comment. Otherwise, user_id should come from an authenticated session or token.
Also applies to: 386-387
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/harvest.py` around lines 323 - 331, get_user_library
currently accepts user_id from the query string (user_id param in
get_user_library) which allows callers to access/modify arbitrary users'
libraries; change this to derive the user ID from the authenticated
session/token (e.g., replace the user_id Query param with a dependency that
extracts the current_user/current_user.id from your auth middleware or token
validation function used elsewhere) and remove the default "default" value;
apply the same change to the other endpoint that accepts user_id in the request
body (lines referenced) so both endpoints use the authenticated user context; if
no authentication exists for this MVP, add a clear TODO comment inside
get_user_library and the other endpoint stating that user_id is temporary and
must be replaced with auth-derived user identification before production.
| async def _get_session(self) -> aiohttp.ClientSession: | ||
| if self._session is None or self._session.closed: | ||
| self._session = aiohttp.ClientSession() | ||
| return self._session |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the OpenAlex harvester file around the lines mentioned
cat -n src/paperbot/infrastructure/harvesters/openalex_harvester.py | head -220Repository: jerry609/PaperBot
Length of output: 8522
🏁 Script executed:
# Search for other harvesters and their timeout patterns
fd -e py "harvester" src/paperbot/infrastructure/harvesters/ | head -20Repository: jerry609/PaperBot
Length of output: 248
🏁 Script executed:
# Check if ClientTimeout is used anywhere in the codebase
rg "ClientTimeout" --type py -B 2 -A 2Repository: jerry609/PaperBot
Length of output: 10041
🏁 Script executed:
# Check all ClientSession instantiations
rg "ClientSession\(" --type py -B 2 -A 2Repository: jerry609/PaperBot
Length of output: 8422
🏁 Script executed:
# Check for test files related to harvesters
find . -name "*test*harvester*" -o -name "*harvester*test*" | head -20Repository: jerry609/PaperBot
Length of output: 99
🏁 Script executed:
# Check semantic scholar harvester for comparison
cat -n src/paperbot/infrastructure/harvesters/semantic_scholar_harvester.py | grep -A 5 "_get_session"Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
# Check if semantic_scholar_harvester exists
ls -la src/paperbot/infrastructure/harvesters/Repository: jerry609/PaperBot
Length of output: 457
🏁 Script executed:
# Look at the test file to understand coverage
cat -n tests/integration/test_harvesters.pyRepository: jerry609/PaperBot
Length of output: 20610
Add HTTP request timeout to prevent indefinite hangs.
The aiohttp.ClientSession at line 44 and the session.get() call at line 90 lack timeout configuration. If the OpenAlex API becomes unresponsive, the harvester will block indefinitely, stalling the harvest pipeline. The codebase already establishes timeout patterns elsewhere (see src/paperbot/utils/downloader.py and src/paperbot/infrastructure/services/api_client.py).
Proposed fix — add ClientTimeout to session creation
async def _get_session(self) -> aiohttp.ClientSession:
if self._session is None or self._session.closed:
- self._session = aiohttp.ClientSession()
+ timeout = aiohttp.ClientTimeout(total=30)
+ self._session = aiohttp.ClientSession(timeout=timeout)
return self._sessionNote: ArxivHarvester has the identical pattern and should also be fixed.
📝 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.
| async def _get_session(self) -> aiohttp.ClientSession: | |
| if self._session is None or self._session.closed: | |
| self._session = aiohttp.ClientSession() | |
| return self._session | |
| async def _get_session(self) -> aiohttp.ClientSession: | |
| if self._session is None or self._session.closed: | |
| timeout = aiohttp.ClientTimeout(total=30) | |
| self._session = aiohttp.ClientSession(timeout=timeout) | |
| return self._session |
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/harvesters/openalex_harvester.py` around lines 42
- 45, Add an HTTP timeout to prevent indefinite hangs by creating the aiohttp
session with a ClientTimeout in _get_session (use aiohttp.ClientTimeout with an
appropriate total/Connect/read values) instead of a bare aiohttp.ClientSession,
and ensure individual calls that use session.get(...) also pass a timeout (or
rely on the session default) so requests to the OpenAlex API cannot block
forever; apply the same changes to the ArxivHarvester equivalent methods.
Summary of ChangesHello @CJBshuosi, 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 introduces a comprehensive paper harvesting and management system. It includes new API endpoints for initiating paper harvests from various sources, searching and filtering papers, managing user libraries, and tracking harvest run history. The changes involve database schema updates, new API routes, and the implementation of a multi-source paper harvest pipeline. 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
|
![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 introduces a comprehensive paper harvesting and management system. Key changes include the addition of new database tables (papers and harvest_runs) via an Alembic migration to store harvested paper metadata and track harvest execution. A new FastAPI router (/api/harvest) is added, exposing endpoints for initiating multi-source paper harvests with streaming progress updates, listing and retrieving harvest run details, searching stored papers with various filters, and managing a user's paper library (saving/unsaving papers). The backend now includes new application services: PaperDeduplicator for in-memory deduplication of papers from different sources, QueryRewriter for expanding search keywords (e.g., abbreviations), and VenueRecommender for suggesting relevant academic venues. A HarvestPipeline workflow orchestrates these services and harvesters (Arxiv, Semantic Scholar, OpenAlex) to perform the end-to-end harvesting process. The research API's add_paper_feedback endpoint is enhanced to optionally save paper metadata to the new papers table when a 'save' action is performed, and logging is improved across several API routes. Frontend changes in web/ update the ResearchDashboard to pass full paper metadata when saving, and SavedPapersList is refactored to use the new /api/papers/library endpoint for fetching and managing saved papers, including new unsavePaper and toggleReadStatus functions. Additionally, several integration and unit tests for the new harvesting components and services are added.
Review comments highlight several security and efficiency concerns. Multiple endpoints (/api/papers/{paper_id}/save, /harvest/runs, /api/papers/library, /research/papers/feedback, /research/context) are identified as vulnerable to Insecure Direct Object Reference (IDOR) or unauthorized data access due to client-supplied user_id parameters without proper authentication. Information exposure is noted in harvest_stream for directly exposing raw exception messages. Efficiency concerns are raised regarding PaperDeduplicator's _merge_paper logic for re-indexing, PaperStore._find_existing's sequential database queries, and client-side venue filtering in OpenAlexHarvester and SemanticScholarHarvester which can lead to inefficient API usage. Other comments suggest improving full-text search performance, optimizing LIKE queries for venue filtering, standardizing paper_id types across frontend/backend, and reviewing column lengths in the papers table. The compute_title_hash method's handling of hyphens and the HarvestPipeline's paper count reporting during pipeline-level failures are also noted for potential refinement.
| existing.doi = new.doi | ||
| self._doi_index[new.doi.lower().strip()] = self._find_index(existing) | ||
| if not existing.arxiv_id and new.arxiv_id: | ||
| existing.arxiv_id = new.arxiv_id | ||
| self._arxiv_index[new.arxiv_id.lower().strip()] = self._find_index(existing) | ||
| if not existing.semantic_scholar_id and new.semantic_scholar_id: | ||
| existing.semantic_scholar_id = new.semantic_scholar_id | ||
| self._s2_index[new.semantic_scholar_id.lower().strip()] = self._find_index(existing) | ||
| if not existing.openalex_id and new.openalex_id: | ||
| existing.openalex_id = new.openalex_id | ||
| self._openalex_index[new.openalex_id.lower().strip()] = self._find_index(existing) |
There was a problem hiding this comment.
The logic for updating _doi_index, _arxiv_index, _s2_index, and _openalex_index within _merge_paper relies on self._find_index(existing). This _find_index method computes the title hash and looks it up in _title_hash_index. This approach is problematic because if the existing paper was initially indexed by an identifier (e.g., DOI) and not its title hash, _find_index might return -1, leading to incorrect index updates for the canonical identifiers. The indexes should be updated directly using the idx of the existing paper, which is already known from _find_duplicate.
| existing.doi = new.doi | |
| self._doi_index[new.doi.lower().strip()] = self._find_index(existing) | |
| if not existing.arxiv_id and new.arxiv_id: | |
| existing.arxiv_id = new.arxiv_id | |
| self._arxiv_index[new.arxiv_id.lower().strip()] = self._find_index(existing) | |
| if not existing.semantic_scholar_id and new.semantic_scholar_id: | |
| existing.semantic_scholar_id = new.semantic_scholar_id | |
| self._s2_index[new.semantic_scholar_id.lower().strip()] = self._find_index(existing) | |
| if not existing.openalex_id and new.openalex_id: | |
| existing.openalex_id = new.openalex_id | |
| self._openalex_index[new.openalex_id.lower().strip()] = self._find_index(existing) | |
| # Fill in missing identifiers | |
| if not existing.doi and new.doi: | |
| existing.doi = new.doi | |
| self._doi_index[new.doi.lower().strip()] = self._find_index(existing) # This line is problematic | |
| if not existing.arxiv_id and new.arxiv_id: | |
| existing.arxiv_id = new.arxiv_id | |
| self._arxiv_index[new.arxiv_id.lower().strip()] = self._find_index(existing) # This line is problematic | |
| if not existing.semantic_scholar_id and new.semantic_scholar_id: | |
| existing.semantic_scholar_id = new.semantic_scholar_id | |
| self._s2_index[new.semantic_scholar_id.lower().strip()] = self._find_index(existing) # This line is problematic | |
| if not existing.openalex_id and new.openalex_id: | |
| existing.openalex_id = new.openalex_id | |
| self._openalex_index[new.openalex_id.lower().strip()] = self._find_index(existing) # This line is problematic |
| @router.post("/papers/{paper_id}/save") | ||
| def save_paper_to_library(paper_id: int, request: SavePaperRequest): |
There was a problem hiding this comment.
The endpoint /api/papers/{paper_id}/save is vulnerable to Broken Access Control (IDOR) because it accepts a user_id in the request body without verification. This allows an attacker to save papers to any user's library, leading to unauthorized modification of user data. Additionally, there's an inconsistency where paper_id is an integer in the path but expected as a string in SqlAlchemyResearchStore, which could lead to confusion if paper_id can also be a Semantic Scholar ID. Ensure the user_id in the request matches the authenticated requester's identity and clarify the paper_id type handling.
| @router.get("/harvest/runs", response_model=HarvestRunListResponse) | ||
| def list_harvest_runs( |
There was a problem hiding this comment.
The /harvest/runs endpoint lists all harvest execution records in the system without any user-based filtering or authentication. Since harvest runs contain sensitive research data such as search keywords and targeted venues, this allows any user to monitor the research activities of others. Access to these records should be restricted to the user who initiated the run.
| @router.get("/papers/library", response_model=LibraryResponse) | ||
| def get_user_library( | ||
| user_id: str = Query("default", description="User ID"), |
There was a problem hiding this comment.
The endpoint /api/papers/library accepts a user_id parameter directly from the client without any authentication. In a multi-user environment, this allows any user to view the saved papers of any other user by simply providing their user_id. This is an Insecure Direct Object Reference (IDOR) vulnerability. Authorization should be enforced based on an authenticated session or token.
| @router.delete("/papers/{paper_id}/save") | ||
| def remove_paper_from_library( | ||
| paper_id: int, | ||
| user_id: str = Query("default", description="User ID"), | ||
| ): |
There was a problem hiding this comment.
The DELETE endpoint for /api/papers/{paper_id}/save allows removing papers from any user's library by providing their user_id in the query string. Without authentication, this is a critical access control flaw that permits unauthorized deletion of user data.
| if query: | ||
| pattern = f"%{query}%" | ||
| stmt = stmt.where( | ||
| or_( | ||
| PaperModel.title.ilike(pattern), | ||
| PaperModel.abstract.ilike(pattern), | ||
| ) |
There was a problem hiding this comment.
The full-text search for query uses ilike(pattern) on both title and abstract. While this works for basic substring matching, ilike can be inefficient for large text fields and complex queries, especially without proper full-text indexing. For more advanced and performant full-text search capabilities, consider integrating with a dedicated full-text search solution (e.g., PostgreSQL's tsvector, SQLite's FTS5, or an external search engine like Elasticsearch).
| venue_conditions = [PaperModel.venue.ilike(f"%{v}%") for v in venues] | ||
| stmt = stmt.where(or_(*venue_conditions)) |
There was a problem hiding this comment.
The venue filter uses PaperModel.venue.ilike(f"%{v}%") for each venue. This LIKE operator with leading wildcards (%) can be very slow as it prevents the use of indexes on the venue column. If venue filtering is a common operation, consider normalizing venue names and storing them in a separate table or using a more efficient search mechanism for this field.
| sa.Column("doi", sa.String(length=256), nullable=True), | ||
| sa.Column("arxiv_id", sa.String(length=64), nullable=True), | ||
| sa.Column("semantic_scholar_id", sa.String(length=64), nullable=True), | ||
| sa.Column("openalex_id", sa.String(length=64), nullable=True), |
There was a problem hiding this comment.
The lengths for doi, arxiv_id, semantic_scholar_id, and openalex_id columns in the papers table seem arbitrary. It's best practice to define these lengths based on the maximum possible length of these identifiers from their respective sources to prevent truncation or unnecessary over-allocation. For example, DOIs can be up to 256 characters, arXiv IDs are typically shorter (e.g., 10-15 chars plus optional version), Semantic Scholar IDs are UUID-like (32-36 chars), and OpenAlex IDs are also UUID-like. Consider reviewing the actual maximum lengths from the sources to set more precise limits.
| all_results = session.execute(base_stmt).all() | ||
| Logger.info(f"Query returned {len(all_results)} results before deduplication", file=LogFiles.HARVEST) | ||
|
|
||
| # Deduplicate by paper.id, keeping the one with latest timestamp | ||
| Logger.info("Deduplicating results by paper id", file=LogFiles.HARVEST) | ||
| paper_map: Dict[int, Tuple[PaperModel, PaperFeedbackModel]] = {} | ||
| for row in all_results: | ||
| paper = row[0] | ||
| feedback = row[1] | ||
| if paper.id not in paper_map or feedback.ts > paper_map[paper.id][1].ts: | ||
| paper_map[paper.id] = (paper, feedback) |
There was a problem hiding this comment.
The deduplication logic for get_user_library is performed in Python (paper_map: Dict[int, Tuple[PaperModel, PaperFeedbackModel]] = {}). While this works for smaller result sets, fetching all results from the database and then deduplicating in Python can be inefficient for large user libraries. It would be more performant to perform this deduplication at the database level, for example, using DISTINCT ON (PostgreSQL) or a subquery with ROW_NUMBER() to select the latest feedback per paper ID.
| if (action === "save" && paper) { | ||
| body.paper_title = paper.title | ||
| body.paper_abstract = paper.abstract || "" | ||
| body.paper_authors = paper.authors || [] | ||
| body.paper_year = paper.year | ||
| body.paper_venue = paper.venue | ||
| body.paper_citation_count = paper.citation_count | ||
| body.paper_url = paper.url | ||
| } |
There was a problem hiding this comment.
The if (action === "save" && paper) block conditionally adds paper metadata to the request body. This is a good improvement for the new save functionality. However, the paper_id sent in the sendFeedback function is p.paper_id, which is a string (e.g., Semantic Scholar ID). The backend's add_paper_feedback expects paper_id to be an integer if it's an internal ID, or a string if it's an external ID. The PaperStore's get_user_library handles both, but the save_paper_to_library in harvest.py explicitly converts paper_id to str(paper_id) before passing it to record_paper_feedback. This inconsistency in paper_id types (string vs. integer) across frontend and backend, and within backend logic, could lead to confusion or subtle bugs. It's best to standardize the paper_id type for feedback actions.
There was a problem hiding this comment.
Pull request overview
Implements backend paper harvesting + storage/search/library APIs (arXiv, Semantic Scholar, OpenAlex) and wires the web UI to saved-paper library + “Save” feedback flow, addressing issue #26 (“Paper Search and Storage”).
Changes:
- Added multi-source harvest pipeline with SSE progress streaming, deduplication, persistence, and harvest-run tracking.
- Introduced paper search + user library endpoints (save/unsave, list library, stats) plus DB schema/migrations for
papersandharvest_runs. - Updated web research UI to save papers via feedback, fetch saved papers from the new library endpoint, and added Next.js proxy routes.
Reviewed changes
Copilot reviewed 31 out of 33 changed files in this pull request and generated 33 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/api.ts | Replaced mock fetchPapers() with real library fetch + response mapping. |
| web/src/components/research/SavedPapersList.tsx | Switched saved-papers UI to /api/papers/library; added unsave + reading-status toggles via feedback. |
| web/src/components/research/ResearchDashboard.tsx | Sends paper metadata in feedback payload on save action. |
| web/src/app/api/papers/library/route.ts | Next.js proxy for backend /api/papers/library. |
| web/src/app/api/papers/[paperId]/save/route.ts | Next.js proxy for backend save/unsave endpoints. |
| web/package-lock.json | Removes an optional @types/trusted-types entry. |
| tests/unit/test_venue_recommender.py | Unit tests for venue recommendation behavior. |
| tests/unit/test_query_rewriter.py | Unit tests for keyword/query expansion/normalization. |
| tests/unit/test_paper_deduplicator.py | Unit tests for multi-identifier dedup + merge rules. |
| tests/unit/test_harvested_paper.py | Unit tests for harvest domain models + serialization/title-hash behavior. |
| tests/integration/test_paper_store.py | Integration tests for persistence, dedup, search, library, and harvest run tracking. |
| tests/integration/test_harvesters.py | Integration tests for harvesters with mocked HTTP responses. |
| tests/integration/test_harvest_pipeline.py | Integration tests for end-to-end pipeline orchestration and error handling. |
| src/paperbot/infrastructure/stores/research_store.py | Adds logging around feedback creation. |
| src/paperbot/infrastructure/stores/paper_store.py | New paper persistence/search/library repository + harvest run tracking. |
| src/paperbot/infrastructure/stores/models.py | Adds PaperModel + HarvestRunModel ORM models. |
| src/paperbot/infrastructure/harvesters/semantic_scholar_harvester.py | New Semantic Scholar harvester. |
| src/paperbot/infrastructure/harvesters/openalex_harvester.py | New OpenAlex harvester (aiohttp + polite pool email). |
| src/paperbot/infrastructure/harvesters/arxiv_harvester.py | New arXiv Atom harvester (aiohttp + rate limiting). |
| src/paperbot/infrastructure/harvesters/init.py | Exposes harvester implementations. |
| src/paperbot/domain/harvest.py | New harvest domain models and helpers (title hash, run results). |
| src/paperbot/context_engine/engine.py | Adds harvest-focused logging + improved exception logging during paper fetch. |
| src/paperbot/application/workflows/harvest_pipeline.py | New workflow orchestrating expansion → venue recommendation → harvesting → dedup → store → run update. |
| src/paperbot/application/services/venue_recommender.py | New static-mapping venue recommender (optional YAML config). |
| src/paperbot/application/services/query_rewriter.py | New abbreviation expansion + normalization service. |
| src/paperbot/application/services/paper_deduplicator.py | New in-memory deduplicator and merge logic. |
| src/paperbot/application/services/init.py | Exports new services. |
| src/paperbot/application/ports/harvester_port.py | Defines harvester protocol/interface. |
| src/paperbot/api/routes/research.py | Extends feedback request model; on save upserts into papers then records feedback. |
| src/paperbot/api/routes/harvest.py | New API routes for harvest SSE, paper search, library, and save/unsave. |
| src/paperbot/api/main.py | Registers the new harvest router. |
| alembic/versions/0003_paper_harvest_tables.py | Migration creating papers and harvest_runs tables + indexes. |
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.
| start_date = f"{year_from}0101" if year_from else "199101" | ||
| end_date = f"{year_to}1231" if year_to else "209912" |
There was a problem hiding this comment.
_build_query uses fallback dates "199101" and "209912" when year_from/year_to are missing, but arXiv's submittedDate filter expects YYYYMMDD. These 6-digit values can produce invalid queries. Use full 8-digit defaults (e.g., 19910101 / 20991231).
| start_date = f"{year_from}0101" if year_from else "199101" | |
| end_date = f"{year_to}1231" if year_to else "209912" | |
| start_date = f"{year_from}0101" if year_from else "19910101" | |
| end_date = f"{year_to}1231" if year_to else "20991231" |
| select(PaperModel, PaperFeedbackModel) | ||
| .join( | ||
| PaperFeedbackModel, | ||
| or_( | ||
| PaperModel.id == cast(PaperFeedbackModel.paper_id, Integer), | ||
| PaperModel.semantic_scholar_id == PaperFeedbackModel.paper_id, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The library join condition casts paper_feedback.paper_id to Integer: cast(PaperFeedbackModel.paper_id, Integer). If paper_id sometimes contains non-numeric IDs (e.g. Semantic Scholar IDs), this cast can raise runtime SQL errors on databases like Postgres even inside an OR. Consider guarding the cast (numeric-only) or restructuring the join so non-numeric values never hit an integer cast.
| from paperbot.domain.harvest import HarvestedPaper, HarvestSource | ||
| from paperbot.infrastructure.stores.paper_store import PaperStore | ||
|
|
||
| paper_store = PaperStore() | ||
| paper = HarvestedPaper( | ||
| title=req.paper_title, | ||
| source=HarvestSource.SEMANTIC_SCHOLAR, | ||
| abstract=req.paper_abstract or "", | ||
| authors=req.paper_authors or [], | ||
| semantic_scholar_id=req.paper_id, | ||
| year=req.paper_year, | ||
| venue=req.paper_venue, | ||
| citation_count=req.paper_citation_count or 0, | ||
| url=req.paper_url, | ||
| ) | ||
| Logger.info("Calling paper store to upsert paper", file=LogFiles.HARVEST) | ||
| new_count, _ = paper_store.upsert_papers_batch([paper]) | ||
|
|
||
| # Get the paper ID from database | ||
| from paperbot.infrastructure.stores.models import PaperModel | ||
| from sqlalchemy import select | ||
| with paper_store._provider.session() as session: | ||
| result = session.execute( | ||
| select(PaperModel).where( | ||
| PaperModel.semantic_scholar_id == req.paper_id | ||
| ) | ||
| ).scalar_one_or_none() | ||
| if result: |
There was a problem hiding this comment.
For action == "save", this route instantiates a new PaperStore() inside the request, then reaches into paper_store._provider.session() to query the inserted row. This bypasses the existing store lifecycle and can leak connections/engines over time. Consider reusing a shared store instance (or explicitly closing/dispose), and avoid accessing private internals by adding a get_paper_by_semantic_scholar_id/upsert_and_get_id method on PaperStore.
| from paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore | ||
|
|
||
| # Verify paper exists | ||
| store = _get_paper_store() | ||
| paper = store.get_paper_by_id(paper_id) | ||
| if not paper: | ||
| raise HTTPException(status_code=404, detail="Paper not found") | ||
|
|
||
| # Use research store to record feedback | ||
| research_store = SqlAlchemyResearchStore() | ||
| feedback = research_store.record_paper_feedback( | ||
| user_id=request.user_id, | ||
| paper_id=str(paper_id), | ||
| action="save", | ||
| track_id=request.track_id, | ||
| ) | ||
|
|
There was a problem hiding this comment.
SqlAlchemyResearchStore doesn’t define record_paper_feedback (the store exposes add_paper_feedback(...) / list_paper_feedback(...)). As written, POST /api/papers/{paper_id}/save will raise AttributeError at runtime. Update this to call the correct research store method (or add the missing method) so saving works.
| citations: item.paper.citation_count ? `${item.paper.citation_count}` : "0", | ||
| status: item.action === "save" ? "Saved" : "pending", | ||
| tags: Array.isArray(item.paper.fields_of_study) ? item.paper.fields_of_study.slice(0, 3) : [] |
There was a problem hiding this comment.
Paper.status is typed as "pending" | "analyzing" | "Reproduced" (see web/src/lib/types.ts), but this mapping returns "Saved" when action === "save", which will fail TypeScript compilation. Consider mapping to one of the allowed statuses or extending the Paper.status union to include the new value consistently across the app.
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| if record.published: | ||
| try: | ||
| year = int(record.published[:4]) | ||
| except (ValueError, IndexError): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except (ValueError, IndexError): | |
| except (ValueError, IndexError): | |
| # If the published date is missing or malformed, leave year as None. |
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| logger.exception("Error while closing harvester %r: %s", harvester, exc) |
| """Close database connections.""" | ||
| try: | ||
| self._provider.engine.dispose() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Best-effort cleanup: ignore errors when disposing the engine. |
Closes jerry609#26 Signed-off-by: LIU BOYU <oor2020@163.com>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/components/research/SavedPapersList.tsx (2)
157-205:⚠️ Potential issue | 🟡 Minor
toggleReadStatusonly cycles between"read"and"reading"— the other two states ("unread","archived") are unreachableLine 161 hardcodes the toggle:
currentStatus === "read" ? "reading" : "read". A paper starting as"unread"will jump straight to"read"and can never return to"unread"or transition to"archived". If all fourReadingStatusvalues (Line 15) are intentional, this toggle doesn't cover them. Confirm whether this is the desired UX.
77-377: 🛠️ Refactor suggestion | 🟠 MajorTests needed for the significant behavior changes in this component.
The data source, endpoint, response shape, and two core handlers (
unsavePaper,toggleReadStatus) all changed. These paths should have corresponding integration or unit tests (e.g., using React Testing Library with mocked fetch).As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: "If behavior changes, add or update tests".src/paperbot/infrastructure/stores/paper_store.py (1)
1-36:⚠️ Potential issue | 🔴 CriticalUnresolved merge conflict markers — file is broken and CI is failing.
The file contains Git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>>) at multiple locations (Lines 1, 11, 36, 44, 87, 108, 116, 369, 841). This causes aSyntaxErrorand blocks all downstream functionality. The CI pipeline confirms this failure.This must be resolved immediately by completing the merge.
🤖 Fix all issues with AI agents
In `@alembic/versions/0003_paper_harvest_tables.py`:
- Around line 65-95: The migration's op.create_table call defines doi, arxiv_id,
semantic_scholar_id, and openalex_id but does not add DB-level uniqueness; add
UniqueConstraints (or unique indexes) for these columns in the same
op.create_table call to match PaperModel's unique=True (e.g., append
sa.UniqueConstraint("doi", name="uq_papers_doi"),
sa.UniqueConstraint("arxiv_id", name="uq_papers_arxiv_id"),
sa.UniqueConstraint("semantic_scholar_id",
name="uq_papers_semantic_scholar_id"), sa.UniqueConstraint("openalex_id",
name="uq_papers_openalex_id") so that the op.create_table invocation enforces
uniqueness at the database level and prevents concurrent upsert duplicates).
In `@src/paperbot/api/main.py`:
- Around line 23-27: Resolve the leftover Git conflict markers by removing the
lines `<<<<<<< HEAD`, `=======`, and `>>>>>>>` and ensure both modules are
imported and their routers registered: add both `newsletter` and `harvest` to
the import list (the existing import that currently toggles between them) and
include both `newsletter` and `harvest` in the router registration call (e.g.,
the call that registers routers in main.py); verify there are no other conflict
markers in the file and run the module to confirm it loads.
In `@src/paperbot/api/routes/harvest.py`:
- Line 118: Remove the unused local variable by either calling set_trace_id()
without assigning its return or actually using the returned trace_id;
specifically, in the route where trace_id = set_trace_id() is present, either
replace the assignment with a bare call set_trace_id() or propagate the returned
trace_id (for example add it to the response headers or include it in logging)
so the variable is referenced (symbols: trace_id, set_trace_id()).
In `@src/paperbot/application/services/paper_deduplicator.py`:
- Around line 183-186: The _find_index method currently returns -1 on miss which
can be interpreted as a valid list index; change _find_index (in class
PaperDeduplicator) to raise a KeyError (or custom LookupError) with a
descriptive message that includes the paper.compute_title_hash() when the title
hash is not present, and update callers in _merge_paper to wrap calls to
_find_index in try/except so they do not assign or insert -1 into identifier
indexes—only store the returned index on success and handle or log the KeyError
path appropriately.
In `@src/paperbot/application/services/venue_recommender.py`:
- Around line 128-146: The exact-match path in the loop (checking keyword_lower
in self.mappings and adding +3 to venue_scores) is also being counted again by
the subsequent partial-match loop, inflating exact hits; modify the logic in the
partial-match pass inside the same for keyword in keywords loop (the block that
iterates for mapped_kw, venues in self.mappings.items()) to skip when mapped_kw
== keyword_lower (or only run the partial-match logic in an else branch when the
exact-match was not applied) so exact matches get +3 and only non-exact partial
matches get +1; keep all other variables (keyword_lower, venue_scores,
sorted_venues, result, max_venues) unchanged.
In `@src/paperbot/application/workflows/harvest_pipeline.py`:
- Around line 314-335: The except block that logs pipeline errors calls
self.paper_store.update_harvest_run(run_id=run_id, ...) even when
create_harvest_run may not have been called, so ensure the run record exists
before updating: either check for an existing run (e.g., call
self.paper_store.get_harvest_run(run_id) or inspect run_id for None) and only
call update_harvest_run if it exists, or create the run record earlier (call
create_harvest_run before operations that may raise) so update_harvest_run has a
valid target; update the exception handler around
update_harvest_run/HarvestFinalResult to handle a missing run gracefully.
- Around line 216-261: The "Phase 4" comment is inaccurate: the loop using "for
source in sources" in harvest_pipeline.py (around the block that yields
HarvestProgress and calls self._get_harvester(...).search(...)) runs
sequentially, not in parallel; update the comment text from "Harvest from each
source in parallel" to "Harvest from each source sequentially" (or similar) so
it matches the actual behavior, or alternatively replace the sequential loop
with an asyncio.gather/asyncio.TaskGroup-based concurrency pattern that launches
per-source tasks and awaits them while preserving per-source rate limits and
error handling if you prefer true parallelism.
In `@src/paperbot/context_engine/engine.py`:
- Line 519: The Logger.info f-string in engine.py (the call to Logger.info(...,
file=LogFiles.HARVEST)) exceeds the 100-char line length; refactor it to fit
within 100 chars by computing the count in a short variable (e.g., papers_count
= len(getattr(resp, 'papers', []) or [])) and then call Logger.info with a
shorter f-string or concatenated string, or wrap the f-string in parentheses
across multiple lines so the line length stays ≤100 while preserving the same
message and file=LogFiles.HARVEST argument.
In `@src/paperbot/domain/harvest.py`:
- Around line 57-62: compute_title_hash currently uses re.sub(r"[^\w\s]", "",
normalized) which preserves underscores so "Deep_Learning" hashes differently
than "Deep-Learning"; update compute_title_hash to also strip or normalize
underscores from the normalized string (e.g., remove or replace "_" before
hashing) by adjusting the regex or adding a dedicated replacement on the
normalized variable so underscores are treated the same as other punctuation
prior to returning the SHA-256 of normalized.
In `@src/paperbot/infrastructure/stores/models.py`:
- Around line 714-786: There is a duplicate PaperModel class definition; remove
the second definition (the one with
semantic_scholar_id/openalex_id/title_hash/etc.) and merge its new columns and
helper methods into the original PaperModel (the existing class that already
defines external_url, first_seen_at, published_at, metadata_json, relationships
judge_scores/feedback_rows/reading_status_rows and set_authors) so ORM mappings
stay unique; specifically add the fields semantic_scholar_id, openalex_id,
title_hash, year, publication_date, citation_count, fields_of_study_json,
primary_source, sources_json, deleted_at and the helper methods
get_fields_of_study, get_sources, set_fields_of_study, set_sources to the
original PaperModel class and then delete the duplicate class block entirely.
In `@src/paperbot/infrastructure/stores/paper_store.py`:
- Around line 380-403: The per-paper Logger.info calls inside the batch upsert
loop are causing excessive I/O; change the per-paper log statements that call
Logger.info around the calls to _find_existing, _update_paper, and _create_model
to Logger.debug (or remove them) and leave only the existing batch-level
Logger.info messages before the loop and after commit; ensure the logic updating
new_count and updated_count and calling session.add()/session.commit() remains
unchanged and that references to _find_existing, _update_paper, and
_create_model are preserved.
In `@src/paperbot/infrastructure/stores/research_store.py`:
- Around line 11-15: The file currently contains unresolved merge conflict
markers in the imports; remove the conflict markers and import both sets needed
by the module so it doesn't crash: add/import normalize_arxiv_id and
normalize_doi from paperbot.domain.paper_identity alongside Logger and LogFiles
from paperbot.utils.logging_config so functions like _resolve_paper_ref_id and
the new logging calls can reference their symbols correctly.
- Around line 349-358: In add_paper_feedback restore the call to
self._resolve_paper_ref_id(session=session, paper_id=(paper_id or "").strip(),
metadata=metadata) and assign it to resolved_paper_ref_id, and also keep the
Logger.info("Creating new feedback record", file=LogFiles.HARVEST) line so both
the resolution logic (used later for paper_ref_id and the if
resolved_paper_ref_id check) and the new log remain; ensure the
resolved_paper_ref_id variable is defined before it is used to create the
feedback record.
In `@tests/integration/test_harvest_pipeline.py`:
- Around line 94-106: The pipeline fixture creates a HarvestPipeline instance
but never closes it; change the fixture to yield the pipeline and call its
close() afterwards so DB connections are released. Specifically, convert the
pipeline fixture to a yield-style fixture (yield pipeline instead of return),
keep the same _get_harvester injection (function get_mock_harvester assigned to
pipeline._get_harvester), and after the yield call pipeline.close() to ensure
HarvestPipeline.close() runs when the test finishes.
In `@tests/unit/test_harvested_paper.py`:
- Around line 95-104: The test test_compute_title_hash_ignores_punctuation
currently creates paper3 but never asserts anything; update the test around
HarvestedPaper and compute_title_hash to make the expected behavior explicit —
either remove paper3 or (preferred) add an assertion using
paper3.compute_title_hash() (e.g., assert it is not equal to
paper1.compute_title_hash() with a short comment explaining that hyphens are
removed producing "deeplearning" vs "deep learning") so the test no longer
leaves an unused variable and Ruff F841 is resolved.
In `@web/src/lib/api.ts`:
- Around line 397-418: fetchPapers now calls the live API and transforms
responses, so add unit tests for fetchPapers covering (1) successful fetch
returning transformed Paper[] (validate id, title, venue, authors string,
citations string, status mapping, tags slice), (2) non-ok HTTP response (mock
fetch to return ok=false and expect [] and console.error), and (3)
network/JSON-parse errors (mock fetch to throw or return invalid JSON and expect
[] and console.error); also add/update a test ensuring fetchScholarDetails
fallback to fetchPapers (reference fetchScholarDetails and fetchPapers) behaves
correctly when the primary path fails. Ensure tests mock global fetch and
restore it after each test.
- Around line 398-403: fetchPapers is using
fetch(`${API_BASE_URL}/papers/library`) without cache control, so Next.js server
components may return stale cached results; update the fetch in the fetchPapers
function (and any other direct fetch to `${API_BASE_URL}/papers/library` such as
calls from fetchScholarDetails) to include the RequestInit option cache:
"no-store" so responses are not cached by Next.js and fresh data is returned
after save/unsave actions.
🧹 Nitpick comments (21)
src/paperbot/context_engine/engine.py (1)
586-589: Moveimport tracebackto the top of the file.Placing imports inside
exceptblocks is discouraged — it delays discovery of import errors and is unconventional.tracebackis a stdlib module and has no cost at import time.♻️ Proposed fix
Add at the top of the file alongside other stdlib imports:
import tracebackThen simplify the except block:
except Exception as e: - import traceback tb = traceback.format_exc()web/src/components/research/SavedPapersList.tsx (3)
87-116: Client-side pagination over a hardcodedlimit: "500"fetch is fragile at scaleAll saved papers (up to 500) are fetched in one request and paginated client-side. The response type defines
total/limit/offsetbut they're never used for server-side pagination. As the library grows, this will degrade both network performance and rendering. Consider wiring up server-side pagination in a follow-up.
93-95:sort_order: "desc"is hardcoded —sortBydependency won't cover order changesIf sort order is always descending, this is fine for now. But note that
useCallbackdepends only on[sortBy], so ifsort_orderis ever made dynamic, the dependency array will need updating.
132-155: Hardcodeduser_id=defaultappears in three separate locations — extract to a constant
"default"is used as the user ID on Lines 95, 136, and 168. Extract it to a module-level constant (e.g.,const DEFAULT_USER_ID = "default") to avoid silent drift if one occurrence is updated but others are not.Proposed fix
const PAGE_SIZE_OPTIONS = [10, 20, 50] +const DEFAULT_USER_ID = "default"Then replace all three occurrences:
- user_id: "default", + user_id: DEFAULT_USER_ID,- const res = await fetch(`/api/papers/${paperId}/save?user_id=default`, { + const res = await fetch(`/api/papers/${paperId}/save?user_id=${DEFAULT_USER_ID}`, {- user_id: "default", + user_id: DEFAULT_USER_ID,src/paperbot/infrastructure/stores/models.py (1)
788-838:HarvestRunModellooks good — minor line-length violations.The model is well-structured. A few lines exceed the 100-character limit per coding guidelines (e.g., lines 803, 810).
♻️ Example fix for line-length
- status: Mapped[Optional[str]] = mapped_column(String(32), default="running", index=True) # running/success/partial/failed + # running / success / partial / failed + status: Mapped[Optional[str]] = mapped_column( + String(32), default="running", index=True + )As per coding guidelines, "Use Black formatting with
line-length = 100for Python code".tests/integration/test_paper_store.py (2)
1-11: Unused import:datetimeandtimezone.
datetimeandtimezoneare imported on line 8 but never used in the file.Proposed fix
import pytest -from datetime import datetime, timezone from paperbot.domain.harvest import HarvestedPaper, HarvestSource
238-298: Prefix unused unpacked variables with_.Several tests unpack
total(andall_papers) without using them (lines 240, 265, 281). Prefixing with_signals intent and silences linter warnings.Proposed fix (representative example)
- papers, total = self.store.search_papers(sources=["arxiv"]) + papers, _total = self.store.search_papers(sources=["arxiv"])- all_papers, total = self.store.search_papers(limit=100) + _all_papers, _total = self.store.search_papers(limit=100)- papers, total = self.store.search_papers( + papers, _total = self.store.search_papers(src/paperbot/infrastructure/harvesters/openalex_harvester.py (3)
79-84: Falsy check onyear_from/year_toskips valid year value0.Using
if year_from:instead ofif year_from is not None:means a (theoretical) year value of0would be silently skipped. While unlikely for publication years,is not Noneis the correct idiom for optional ints.Proposed fix
- if year_from: + if year_from is not None: filters.append(f"publication_year:>={year_from}") - if year_to: + if year_to is not None: filters.append(f"publication_year:<={year_to}")
47-55: Moveimport timeto the top of the file.The
timemodule is imported inside_rate_limiton every call. Moving it to the module-level imports is cleaner and avoids repeated import overhead.Proposed fix
import asyncio import logging +import time from typing import Any, Dict, List, Optionalasync def _rate_limit(self) -> None: """Enforce rate limiting between requests.""" - import time - now = time.time()
100-110: Client-side venue filtering may silently discard results.Venue filtering happens after fetching from the API. If the API returns
max_resultspapers and none match the venue filter, the caller gets zero papers despitetotal_foundreporting a higher count. Consider documenting this limitation or adjustingtotal_foundto reflect post-filter count.tests/integration/test_harvest_pipeline.py (1)
223-227: Rename unused loop variableitemto_item.In the loops at lines 225 and 267,
itemis not used in the loop body.Proposed fix
- async for item in pipeline.run(config): - pass # Just run to completion + async for _item in pipeline.run(config): + pass # Just run to completionAlso applies to: 265-269
src/paperbot/application/services/query_rewriter.py (2)
28-68: AnnotateDEFAULT_ABBREVIATIONSasClassVar.Mutable class attributes should be annotated with
typing.ClassVarto signal they are shared class-level state and not instance attributes.Proposed fix
+from typing import ClassVar, Dict, List, Optional -from typing import Dict, List, Optional ... - DEFAULT_ABBREVIATIONS: Dict[str, str] = { + DEFAULT_ABBREVIATIONS: ClassVar[Dict[str, str]] = {
120-122: Inconsistent generic syntax:set[str]vsDict/Listfromtyping.Line 121 uses the builtin
set[str](Python 3.9+) while the rest of the file usestyping.Dict,typing.List. Use consistent style — either alltypinggenerics or all builtin generics.Proposed fix
- seen: set[str] = set() + seen: Set[str] = set()And add
Setto the typing import.src/paperbot/application/services/paper_deduplicator.py (1)
131-181: Merge strategy is comprehensive but doesn't update indexes for the new paper's identifiers that already exist on the existing paper.When
_merge_paperis called, it only fills in missing identifiers on the existing paper. This is correct for merge semantics, but note that thenewpaper's other identifiers (that the existing paper already has) are not cross-indexed. This means if paper A has DOI "x" and paper B (duplicate by title) has DOI "y", the DOI "y" won't be indexed — a third paper C with DOI "y" won't deduplicate against A.This is an edge case (same paper with different DOIs across sources) and may be acceptable, but worth documenting.
src/paperbot/application/services/venue_recommender.py (1)
25-81: AnnotateDEFAULT_MAPPINGSasClassVar.Same as
QueryRewriter.DEFAULT_ABBREVIATIONS— mutable class attributes should usetyping.ClassVar.Proposed fix
+from typing import ClassVar, Dict, List, Optional -from typing import Dict, List, Optional ... - DEFAULT_MAPPINGS: Dict[str, List[str]] = { + DEFAULT_MAPPINGS: ClassVar[Dict[str, List[str]]] = {src/paperbot/api/routes/research.py (1)
668-668: Line exceeds 100-character limit.Line 668 is over 100 characters. As per coding guidelines, "Use Black formatting with
line-length = 100for Python code."src/paperbot/infrastructure/harvesters/arxiv_harvester.py (2)
136-141: Fragile version-suffix stripping — splitting on first"v"can mangle IDs.
arxiv_id.split("v")[0]splits on the first occurrence of"v". While rare, old-style arXiv IDs could theoretically contain"v"in category prefixes before the version suffix. A safer approach is to strip only a trailing version pattern.Proposed fix
+ import re # Remove version suffix (e.g., "2301.12345v1" -> "2301.12345") - if "v" in arxiv_id: - arxiv_id = arxiv_id.split("v")[0] + arxiv_id = re.sub(r"v\d+$", "", arxiv_id)
67-74:year_from=0is falsy and would skip the date filter.
if year_from or year_to:treats0as falsy. Whileyear_from=0is unrealistic, usingis not Noneis more defensive and consistent with the pattern expected across the codebase.src/paperbot/infrastructure/harvesters/semantic_scholar_harvester.py (1)
78-85: Venue substring matching may be too permissive.
any(v in p.venue.lower() for v in venue_set)uses substring containment, so filtering by"ACL"would also match papers from"NAACL","TACL", etc. If this is intentional for broad matching, a brief comment would help. Otherwise, consider word-boundary or exact matching.src/paperbot/application/workflows/harvest_pipeline.py (1)
357-364: Silently swallowing exceptions inclose()hides resource cleanup failures.Per Ruff S110,
try-except-passshould at least log the error so cleanup failures are diagnosable.Proposed fix
for harvester in self._harvesters.values(): try: await harvester.close() - except Exception: - pass + except Exception: + logger.debug("Failed to close harvester", exc_info=True)src/paperbot/api/routes/harvest.py (1)
66-108:HarvestPipelinecreated per request withoutdb_url— relies on default.
HarvestPipeline()at Line 79 is created with nodb_url, relying onget_db_url()default insidePaperStore. This is fine if all components share the same DB config, but consider whether the pipeline should share the module-level_paper_storeinstance to avoid creating duplicate connection pools.
| op.create_table( | ||
| "papers", | ||
| sa.Column("id", sa.Integer(), primary_key=True, autoincrement=True), | ||
| # Canonical identifiers (for deduplication) | ||
| sa.Column("doi", sa.String(length=256), nullable=True), | ||
| sa.Column("arxiv_id", sa.String(length=64), nullable=True), | ||
| sa.Column("semantic_scholar_id", sa.String(length=64), nullable=True), | ||
| sa.Column("openalex_id", sa.String(length=64), nullable=True), | ||
| sa.Column("title_hash", sa.String(length=64), nullable=False), | ||
| # Core metadata | ||
| sa.Column("title", sa.Text(), nullable=False), | ||
| sa.Column("abstract", sa.Text(), server_default="", nullable=False), | ||
| sa.Column("authors_json", sa.Text(), server_default="[]", nullable=False), | ||
| sa.Column("year", sa.Integer(), nullable=True), | ||
| sa.Column("venue", sa.String(length=256), nullable=True), | ||
| sa.Column("publication_date", sa.String(length=32), nullable=True), | ||
| sa.Column("citation_count", sa.Integer(), server_default="0", nullable=False), | ||
| # URLs | ||
| sa.Column("url", sa.String(length=1024), nullable=True), | ||
| sa.Column("pdf_url", sa.String(length=1024), nullable=True), | ||
| # Classification | ||
| sa.Column("keywords_json", sa.Text(), server_default="[]", nullable=False), | ||
| sa.Column("fields_of_study_json", sa.Text(), server_default="[]", nullable=False), | ||
| # Source tracking | ||
| sa.Column("primary_source", sa.String(length=32), nullable=False), | ||
| sa.Column("sources_json", sa.Text(), server_default="[]", nullable=False), | ||
| # Timestamps | ||
| sa.Column("created_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PaperModel definition and its unique constraints
rg -n "class PaperModel" --type py -A 50 | head -100Repository: jerry609/PaperBot
Length of output: 9639
🏁 Script executed:
#!/bin/bash
# Search for UniqueConstraint declarations related to papers
rg -n "UniqueConstraint.*arxiv_id\|UniqueConstraint.*doi" --type pyRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find PaperStore and _find_existing method
rg -n "class PaperStore" --type py -A 5Repository: jerry609/PaperBot
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Find _find_existing method
rg -n "def _find_existing" --type py -B 2 -A 20Repository: jerry609/PaperBot
Length of output: 4328
🏁 Script executed:
#!/bin/bash
# Check the migration file more thoroughly to see full context
fd "0003_paper_harvest_tables.py" --type fRepository: jerry609/PaperBot
Length of output: 106
Add unique constraints to canonical identifier columns in the papers table migration.
The PaperModel declares doi, arxiv_id, semantic_scholar_id, and openalex_id as unique=True, but the migration creates these columns without unique constraints. Without DB-level enforcement, concurrent upserts can bypass the application's deduplication logic in PaperStore._find_existing and create duplicate rows for the same paper.
🤖 Prompt for AI Agents
In `@alembic/versions/0003_paper_harvest_tables.py` around lines 65 - 95, The
migration's op.create_table call defines doi, arxiv_id, semantic_scholar_id, and
openalex_id but does not add DB-level uniqueness; add UniqueConstraints (or
unique indexes) for these columns in the same op.create_table call to match
PaperModel's unique=True (e.g., append sa.UniqueConstraint("doi",
name="uq_papers_doi"), sa.UniqueConstraint("arxiv_id",
name="uq_papers_arxiv_id"), sa.UniqueConstraint("semantic_scholar_id",
name="uq_papers_semantic_scholar_id"), sa.UniqueConstraint("openalex_id",
name="uq_papers_openalex_id") so that the op.create_table invocation enforces
uniqueness at the database level and prevents concurrent upsert duplicates).
| <<<<<<< HEAD | ||
| newsletter, | ||
| ======= | ||
| harvest, | ||
| >>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage) |
There was a problem hiding this comment.
Unresolved merge conflict markers break the application.
The file contains Git conflict markers (<<<<<<< HEAD, =======, >>>>>>>) at lines 23–27 and 71–75, which cause syntax errors and prevent the module from loading. Both newsletter and harvest should be imported and registered.
🐛 Proposed fix — resolve both conflict regions
Import block:
paperscool,
-<<<<<<< HEAD
newsletter,
-=======
harvest,
->>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage)
)Router registration:
app.include_router(paperscool.router, prefix="/api", tags=["PapersCool"])
-<<<<<<< HEAD
app.include_router(newsletter.router, prefix="/api", tags=["Newsletter"])
-=======
app.include_router(harvest.router, prefix="/api", tags=["Harvest"])
->>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage)📝 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.
| <<<<<<< HEAD | |
| newsletter, | |
| ======= | |
| harvest, | |
| >>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage) | |
| paperscool, | |
| newsletter, | |
| harvest, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 23-23: Expected ,, found <<
(invalid-syntax)
[warning] 23-23: Expected ,, found <<
(invalid-syntax)
[warning] 23-23: Expected ,, found <<
(invalid-syntax)
[warning] 23-23: Expected ,, found <
(invalid-syntax)
[warning] 24-24: Expected ,, found name
(invalid-syntax)
[warning] 25-25: Expected ,, found ==
(invalid-syntax)
[warning] 25-25: Expected ,, found ==
(invalid-syntax)
[warning] 25-25: Expected ,, found ==
(invalid-syntax)
[warning] 25-25: Expected ,, found =
(invalid-syntax)
[warning] 27-27: Expected ,, found >>
(invalid-syntax)
[warning] 27-27: Expected ,, found >>
(invalid-syntax)
[warning] 27-27: Expected ,, found >>
(invalid-syntax)
[warning] 27-27: Expected ,, found >
(invalid-syntax)
[warning] 27-27: Invalid decimal integer literal
(invalid-syntax)
[warning] 27-27: Expected ), found (
(invalid-syntax)
[warning] 27-27: Invalid annotated assignment target
(invalid-syntax)
[warning] 27-27: Expected ), found :
(invalid-syntax)
[warning] 27-27: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 27-27: Expected a statement
(invalid-syntax)
🤖 Prompt for AI Agents
In `@src/paperbot/api/main.py` around lines 23 - 27, Resolve the leftover Git
conflict markers by removing the lines `<<<<<<< HEAD`, `=======`, and `>>>>>>>`
and ensure both modules are imported and their routers registered: add both
`newsletter` and `harvest` to the import list (the existing import that
currently toggles between them) and include both `newsletter` and `harvest` in
the router registration call (e.g., the call that registers routers in main.py);
verify there are no other conflict markers in the file and run the module to
confirm it loads.
|
|
||
| Returns Server-Sent Events with progress updates. | ||
| """ | ||
| trace_id = set_trace_id() |
There was a problem hiding this comment.
Unused trace_id variable.
trace_id is assigned but never referenced. Either use it (e.g., in a response header) or call set_trace_id() without capturing the return value.
Fix
- trace_id = set_trace_id()
+ set_trace_id()📝 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.
| trace_id = set_trace_id() | |
| set_trace_id() |
🧰 Tools
🪛 Ruff (0.15.0)
[error] 118-118: Local variable trace_id is assigned to but never used
Remove assignment to unused variable trace_id
(F841)
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/harvest.py` at line 118, Remove the unused local
variable by either calling set_trace_id() without assigning its return or
actually using the returned trace_id; specifically, in the route where trace_id
= set_trace_id() is present, either replace the assignment with a bare call
set_trace_id() or propagate the returned trace_id (for example add it to the
response headers or include it in logging) so the variable is referenced
(symbols: trace_id, set_trace_id()).
| def _find_index(self, paper: HarvestedPaper) -> int: | ||
| """Find the index of a paper in the title hash index.""" | ||
| title_hash = paper.compute_title_hash() | ||
| return self._title_hash_index.get(title_hash, -1) |
There was a problem hiding this comment.
_find_index returning -1 is a dangerous fallback.
If _find_index ever fails to locate a paper (returns -1), the callers in _merge_paper (lines 144, 147, 150, 153) will store -1 in the identifier indexes. In Python, -1 is a valid list index (pointing to the last element), so subsequent lookups would silently match the wrong paper instead of raising an error.
While this shouldn't happen in normal operation (indexed papers always have their title_hash), a defensive fix would be safer.
Proposed fix
def _find_index(self, paper: HarvestedPaper) -> int:
"""Find the index of a paper in the title hash index."""
title_hash = paper.compute_title_hash()
- return self._title_hash_index.get(title_hash, -1)
+ idx = self._title_hash_index.get(title_hash)
+ if idx is None:
+ raise ValueError(f"Paper not found in index: {paper.title!r}")
+ return idx🤖 Prompt for AI Agents
In `@src/paperbot/application/services/paper_deduplicator.py` around lines 183 -
186, The _find_index method currently returns -1 on miss which can be
interpreted as a valid list index; change _find_index (in class
PaperDeduplicator) to raise a KeyError (or custom LookupError) with a
descriptive message that includes the paper.compute_title_hash() when the title
hash is not present, and update callers in _merge_paper to wrap calls to
_find_index in try/except so they do not assign or insert -1 into identifier
indexes—only store the returned index on success and handle or log the KeyError
path appropriately.
| for keyword in keywords: | ||
| keyword_lower = keyword.lower().strip() | ||
| if not keyword_lower: | ||
| continue | ||
|
|
||
| # Exact match (highest priority) | ||
| if keyword_lower in self.mappings: | ||
| for venue in self.mappings[keyword_lower]: | ||
| venue_scores[venue] = venue_scores.get(venue, 0) + 3 | ||
|
|
||
| # Partial match (medium priority) | ||
| for mapped_kw, venues in self.mappings.items(): | ||
| if keyword_lower in mapped_kw or mapped_kw in keyword_lower: | ||
| for venue in venues: | ||
| venue_scores[venue] = venue_scores.get(venue, 0) + 1 | ||
|
|
||
| # Sort by score descending | ||
| sorted_venues = sorted(venue_scores.items(), key=lambda x: -x[1]) | ||
| result = [v[0] for v in sorted_venues[:max_venues]] |
There was a problem hiding this comment.
Exact matches are double-scored due to partial match also firing.
When keyword_lower exactly matches a key in self.mappings, the exact-match block (line 134) adds +3 and then the partial-match loop (line 140: keyword_lower in mapped_kw) also matches the same key, adding +1. So exact matches effectively score 4, not the intended 3.
This doesn't break ranking (all exact matches are inflated equally), but if the intent is for exact=3, partial=1, add a guard:
Proposed fix
# Partial match (medium priority)
for mapped_kw, venues in self.mappings.items():
- if keyword_lower in mapped_kw or mapped_kw in keyword_lower:
+ if mapped_kw == keyword_lower:
+ continue # Already scored as exact match
+ if keyword_lower in mapped_kw or mapped_kw in keyword_lower:
for venue in venues:
venue_scores[venue] = venue_scores.get(venue, 0) + 1📝 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.
| for keyword in keywords: | |
| keyword_lower = keyword.lower().strip() | |
| if not keyword_lower: | |
| continue | |
| # Exact match (highest priority) | |
| if keyword_lower in self.mappings: | |
| for venue in self.mappings[keyword_lower]: | |
| venue_scores[venue] = venue_scores.get(venue, 0) + 3 | |
| # Partial match (medium priority) | |
| for mapped_kw, venues in self.mappings.items(): | |
| if keyword_lower in mapped_kw or mapped_kw in keyword_lower: | |
| for venue in venues: | |
| venue_scores[venue] = venue_scores.get(venue, 0) + 1 | |
| # Sort by score descending | |
| sorted_venues = sorted(venue_scores.items(), key=lambda x: -x[1]) | |
| result = [v[0] for v in sorted_venues[:max_venues]] | |
| for keyword in keywords: | |
| keyword_lower = keyword.lower().strip() | |
| if not keyword_lower: | |
| continue | |
| # Exact match (highest priority) | |
| if keyword_lower in self.mappings: | |
| for venue in self.mappings[keyword_lower]: | |
| venue_scores[venue] = venue_scores.get(venue, 0) + 3 | |
| # Partial match (medium priority) | |
| for mapped_kw, venues in self.mappings.items(): | |
| if mapped_kw == keyword_lower: | |
| continue # Already scored as exact match | |
| if keyword_lower in mapped_kw or mapped_kw in keyword_lower: | |
| for venue in venues: | |
| venue_scores[venue] = venue_scores.get(venue, 0) + 1 | |
| # Sort by score descending | |
| sorted_venues = sorted(venue_scores.items(), key=lambda x: -x[1]) | |
| result = [v[0] for v in sorted_venues[:max_venues]] |
🤖 Prompt for AI Agents
In `@src/paperbot/application/services/venue_recommender.py` around lines 128 -
146, The exact-match path in the loop (checking keyword_lower in self.mappings
and adding +3 to venue_scores) is also being counted again by the subsequent
partial-match loop, inflating exact hits; modify the logic in the partial-match
pass inside the same for keyword in keywords loop (the block that iterates for
mapped_kw, venues in self.mappings.items()) to skip when mapped_kw ==
keyword_lower (or only run the partial-match logic in an else branch when the
exact-match was not applied) so exact matches get +3 and only non-exact partial
matches get +1; keep all other variables (keyword_lower, venue_scores,
sorted_venues, result, max_venues) unchanged.
| <<<<<<< HEAD | ||
| resolved_paper_ref_id = self._resolve_paper_ref_id( | ||
| session=session, | ||
| paper_id=(paper_id or "").strip(), | ||
| metadata=metadata, | ||
| ) | ||
|
|
||
| ======= | ||
| Logger.info("Creating new feedback record", file=LogFiles.HARVEST) | ||
| >>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage) |
There was a problem hiding this comment.
Unresolved merge conflict drops _resolve_paper_ref_id call — NameError at runtime.
The conflict removed the resolved_paper_ref_id assignment (lines 350–354) from the feature branch side, but line 363 (paper_ref_id=resolved_paper_ref_id) and line 371 (if resolved_paper_ref_id) still reference it. This will raise a NameError at runtime whenever add_paper_feedback is called.
Resolve by keeping both the paper-ref resolution logic and the new logging statements.
🐛 Proposed fix
-<<<<<<< HEAD
resolved_paper_ref_id = self._resolve_paper_ref_id(
session=session,
paper_id=(paper_id or "").strip(),
metadata=metadata,
)
-=======
Logger.info("Creating new feedback record", file=LogFiles.HARVEST)
->>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage)📝 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.
| <<<<<<< HEAD | |
| resolved_paper_ref_id = self._resolve_paper_ref_id( | |
| session=session, | |
| paper_id=(paper_id or "").strip(), | |
| metadata=metadata, | |
| ) | |
| ======= | |
| Logger.info("Creating new feedback record", file=LogFiles.HARVEST) | |
| >>>>>>> 09ca42d (feat(Harvest): add -- Paper Search and Storage) | |
| resolved_paper_ref_id = self._resolve_paper_ref_id( | |
| session=session, | |
| paper_id=(paper_id or "").strip(), | |
| metadata=metadata, | |
| ) | |
| Logger.info("Creating new feedback record", file=LogFiles.HARVEST) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 349-349: Expected a statement
(invalid-syntax)
[warning] 349-349: Expected a statement
(invalid-syntax)
[warning] 349-349: Expected a statement
(invalid-syntax)
[warning] 349-349: Expected a statement
(invalid-syntax)
[warning] 350-350: Unexpected indentation
(invalid-syntax)
[warning] 356-356: Expected a statement
(invalid-syntax)
[warning] 356-356: Expected a statement
(invalid-syntax)
[warning] 356-356: Expected a statement
(invalid-syntax)
[warning] 356-356: Expected a statement
(invalid-syntax)
[warning] 356-357: Expected a statement
(invalid-syntax)
[warning] 357-357: Unexpected indentation
(invalid-syntax)
[warning] 358-358: Expected a statement
(invalid-syntax)
[warning] 358-358: Expected a statement
(invalid-syntax)
[warning] 358-358: Expected a statement
(invalid-syntax)
[warning] 358-358: Expected a statement
(invalid-syntax)
[warning] 358-358: Invalid decimal integer literal
(invalid-syntax)
[warning] 358-358: Expected ,, found :
(invalid-syntax)
[warning] 358-358: Expected ,, found name
(invalid-syntax)
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 349 - 358,
In add_paper_feedback restore the call to
self._resolve_paper_ref_id(session=session, paper_id=(paper_id or "").strip(),
metadata=metadata) and assign it to resolved_paper_ref_id, and also keep the
Logger.info("Creating new feedback record", file=LogFiles.HARVEST) line so both
the resolution logic (used later for paper_ref_id and the if
resolved_paper_ref_id check) and the new log remain; ensure the
resolved_paper_ref_id variable is defined before it is used to create the
feedback record.
| @pytest.fixture | ||
| def pipeline(tmp_path, mock_harvesters): | ||
| """Create HarvestPipeline with mocked dependencies.""" | ||
| db_url = f"sqlite:///{tmp_path / 'test_harvest.db'}" | ||
| pipeline = HarvestPipeline(db_url=db_url) | ||
|
|
||
| # Inject mock harvesters | ||
| def get_mock_harvester(source): | ||
| return mock_harvesters.get(source) | ||
|
|
||
| pipeline._get_harvester = get_mock_harvester | ||
| return pipeline | ||
|
|
There was a problem hiding this comment.
Pipeline fixture does not close the pipeline after the test.
The pipeline fixture creates a HarvestPipeline with a real SQLite database but never calls close() after the test completes. This could leave DB connections open. The error-handling tests (lines 338, 380, 417) close explicitly — apply the same here.
Proposed fix
`@pytest.fixture`
def pipeline(tmp_path, mock_harvesters):
"""Create HarvestPipeline with mocked dependencies."""
db_url = f"sqlite:///{tmp_path / 'test_harvest.db'}"
pipeline = HarvestPipeline(db_url=db_url)
# Inject mock harvesters
def get_mock_harvester(source):
return mock_harvesters.get(source)
pipeline._get_harvester = get_mock_harvester
- return pipeline
+ yield pipeline
+ import asyncio
+ asyncio.get_event_loop().run_until_complete(pipeline.close())📝 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.
| @pytest.fixture | |
| def pipeline(tmp_path, mock_harvesters): | |
| """Create HarvestPipeline with mocked dependencies.""" | |
| db_url = f"sqlite:///{tmp_path / 'test_harvest.db'}" | |
| pipeline = HarvestPipeline(db_url=db_url) | |
| # Inject mock harvesters | |
| def get_mock_harvester(source): | |
| return mock_harvesters.get(source) | |
| pipeline._get_harvester = get_mock_harvester | |
| return pipeline | |
| `@pytest.fixture` | |
| def pipeline(tmp_path, mock_harvesters): | |
| """Create HarvestPipeline with mocked dependencies.""" | |
| db_url = f"sqlite:///{tmp_path / 'test_harvest.db'}" | |
| pipeline = HarvestPipeline(db_url=db_url) | |
| # Inject mock harvesters | |
| def get_mock_harvester(source): | |
| return mock_harvesters.get(source) | |
| pipeline._get_harvester = get_mock_harvester | |
| yield pipeline | |
| import asyncio | |
| asyncio.get_event_loop().run_until_complete(pipeline.close()) |
🤖 Prompt for AI Agents
In `@tests/integration/test_harvest_pipeline.py` around lines 94 - 106, The
pipeline fixture creates a HarvestPipeline instance but never closes it; change
the fixture to yield the pipeline and call its close() afterwards so DB
connections are released. Specifically, convert the pipeline fixture to a
yield-style fixture (yield pipeline instead of return), keep the same
_get_harvester injection (function get_mock_harvester assigned to
pipeline._get_harvester), and after the yield call pipeline.close() to ensure
HarvestPipeline.close() runs when the test finishes.
| def test_compute_title_hash_ignores_punctuation(self): | ||
| """Title hash ignores punctuation.""" | ||
| paper1 = HarvestedPaper(title="Deep Learning", source=HarvestSource.ARXIV) | ||
| paper2 = HarvestedPaper(title="Deep, Learning!", source=HarvestSource.ARXIV) | ||
| paper3 = HarvestedPaper(title="Deep-Learning?", source=HarvestSource.ARXIV) | ||
|
|
||
| # All should have same hash after removing punctuation | ||
| assert paper1.compute_title_hash() == paper2.compute_title_hash() | ||
| # Note: hyphens are removed, making it "deeplearning" vs "deep learning" | ||
| # This might differ, which is intentional for similar titles |
There was a problem hiding this comment.
Incomplete test — paper3 is assigned but never asserted.
paper3 (line 99) is created but never used in an assertion. The comment at lines 103–104 acknowledges the behavior difference, but the test should either explicitly assert the expected outcome or remove paper3 to avoid confusion (also flagged by Ruff F841).
💚 Proposed fix — make the assertion explicit
def test_compute_title_hash_ignores_punctuation(self):
"""Title hash ignores punctuation."""
paper1 = HarvestedPaper(title="Deep Learning", source=HarvestSource.ARXIV)
paper2 = HarvestedPaper(title="Deep, Learning!", source=HarvestSource.ARXIV)
- paper3 = HarvestedPaper(title="Deep-Learning?", source=HarvestSource.ARXIV)
# All should have same hash after removing punctuation
assert paper1.compute_title_hash() == paper2.compute_title_hash()
- # Note: hyphens are removed, making it "deeplearning" vs "deep learning"
- # This might differ, which is intentional for similar titles
+
+ # Hyphenated title produces "deeplearning" (no space) — intentionally different
+ paper3 = HarvestedPaper(title="Deep-Learning?", source=HarvestSource.ARXIV)
+ assert paper1.compute_title_hash() != paper3.compute_title_hash()📝 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.
| def test_compute_title_hash_ignores_punctuation(self): | |
| """Title hash ignores punctuation.""" | |
| paper1 = HarvestedPaper(title="Deep Learning", source=HarvestSource.ARXIV) | |
| paper2 = HarvestedPaper(title="Deep, Learning!", source=HarvestSource.ARXIV) | |
| paper3 = HarvestedPaper(title="Deep-Learning?", source=HarvestSource.ARXIV) | |
| # All should have same hash after removing punctuation | |
| assert paper1.compute_title_hash() == paper2.compute_title_hash() | |
| # Note: hyphens are removed, making it "deeplearning" vs "deep learning" | |
| # This might differ, which is intentional for similar titles | |
| def test_compute_title_hash_ignores_punctuation(self): | |
| """Title hash ignores punctuation.""" | |
| paper1 = HarvestedPaper(title="Deep Learning", source=HarvestSource.ARXIV) | |
| paper2 = HarvestedPaper(title="Deep, Learning!", source=HarvestSource.ARXIV) | |
| # All should have same hash after removing punctuation | |
| assert paper1.compute_title_hash() == paper2.compute_title_hash() | |
| # Hyphenated title produces "deeplearning" (no space) — intentionally different | |
| paper3 = HarvestedPaper(title="Deep-Learning?", source=HarvestSource.ARXIV) | |
| assert paper1.compute_title_hash() != paper3.compute_title_hash() |
🧰 Tools
🪛 Ruff (0.15.0)
[error] 99-99: Local variable paper3 is assigned to but never used
Remove assignment to unused variable paper3
(F841)
🤖 Prompt for AI Agents
In `@tests/unit/test_harvested_paper.py` around lines 95 - 104, The test
test_compute_title_hash_ignores_punctuation currently creates paper3 but never
asserts anything; update the test around HarvestedPaper and compute_title_hash
to make the expected behavior explicit — either remove paper3 or (preferred) add
an assertion using paper3.compute_title_hash() (e.g., assert it is not equal to
paper1.compute_title_hash() with a short comment explaining that hyphens are
removed producing "deeplearning" vs "deep learning") so the test no longer
leaves an unused variable and Ruff F841 is resolved.
| export async function fetchPapers(): Promise<Paper[]> { | ||
| return [ | ||
| { | ||
| id: "attention-is-all-you-need", | ||
| title: "Attention Is All You Need", | ||
| venue: "NeurIPS 2017", | ||
| authors: "Vaswani et al.", | ||
| citations: "100k+", | ||
| status: "Reproduced", | ||
| tags: ["Transformer", "NLP"] | ||
| }, | ||
| { | ||
| id: "bert-pretraining", | ||
| title: "BERT: Pre-training of Deep Bidirectional Transformers", | ||
| venue: "NAACL 2019", | ||
| authors: "Devlin et al.", | ||
| citations: "80k+", | ||
| status: "analyzing", | ||
| tags: ["NLP", "Language Model"] | ||
| }, | ||
| { | ||
| id: "resnet-deep-residual", | ||
| title: "Deep Residual Learning for Image Recognition", | ||
| venue: "CVPR 2016", | ||
| authors: "He et al.", | ||
| citations: "150k+", | ||
| status: "pending", | ||
| tags: ["CV", "ResNet"] | ||
| try { | ||
| const res = await fetch(`${API_BASE_URL}/papers/library`) | ||
| if (!res.ok) { | ||
| console.error("Failed to fetch papers library:", res.status) | ||
| return [] | ||
| } | ||
| ] | ||
| const data = await res.json() | ||
| // Transform backend response to frontend Paper type | ||
| return (data.papers || []).map((item: { paper: Record<string, unknown>; action: string }) => ({ | ||
| id: String(item.paper.id), | ||
| title: item.paper.title || "Untitled", | ||
| venue: item.paper.venue || "Unknown", | ||
| authors: Array.isArray(item.paper.authors) ? item.paper.authors.join(", ") : "Unknown", | ||
| citations: item.paper.citation_count ? `${item.paper.citation_count}` : "0", | ||
| status: item.action === "save" ? "Saved" : "pending", | ||
| tags: Array.isArray(item.paper.fields_of_study) ? item.paper.fields_of_study.slice(0, 3) : [] | ||
| })) | ||
| } catch (e) { | ||
| console.error("Error fetching papers:", e) | ||
| return [] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Behavior changed significantly — tests needed.
fetchPapers shifted from returning static mock data to a live API call with transformation and error handling. This is a critical data path (also used as fallback in fetchScholarDetails at Line 246). Unit tests should cover: successful fetch + transformation, non-ok response, and network/parse errors.
As per coding guidelines, {src,web/src}/**/*.{py,ts,tsx}: "If behavior changes, add or update tests".
🤖 Prompt for AI Agents
In `@web/src/lib/api.ts` around lines 397 - 418, fetchPapers now calls the live
API and transforms responses, so add unit tests for fetchPapers covering (1)
successful fetch returning transformed Paper[] (validate id, title, venue,
authors string, citations string, status mapping, tags slice), (2) non-ok HTTP
response (mock fetch to return ok=false and expect [] and console.error), and
(3) network/JSON-parse errors (mock fetch to throw or return invalid JSON and
expect [] and console.error); also add/update a test ensuring
fetchScholarDetails fallback to fetchPapers (reference fetchScholarDetails and
fetchPapers) behaves correctly when the primary path fails. Ensure tests mock
global fetch and restore it after each test.
| try { | ||
| const res = await fetch(`${API_BASE_URL}/papers/library`) | ||
| if (!res.ok) { | ||
| console.error("Failed to fetch papers library:", res.status) | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Missing cache: "no-store" — Next.js may serve stale library data
postJson (Line 19) sets cache: "no-store", but this fetch doesn't. In Next.js App Router, fetch results are cached by default in server components. Since fetchPapers is also called from fetchScholarDetails (a server-side path), stale data may be returned after a save/unsave action.
Proposed fix
- const res = await fetch(`${API_BASE_URL}/papers/library`)
+ const res = await fetch(`${API_BASE_URL}/papers/library`, { cache: "no-store" })📝 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.
| try { | |
| const res = await fetch(`${API_BASE_URL}/papers/library`) | |
| if (!res.ok) { | |
| console.error("Failed to fetch papers library:", res.status) | |
| return [] | |
| } | |
| try { | |
| const res = await fetch(`${API_BASE_URL}/papers/library`, { cache: "no-store" }) | |
| if (!res.ok) { | |
| console.error("Failed to fetch papers library:", res.status) | |
| return [] | |
| } |
🤖 Prompt for AI Agents
In `@web/src/lib/api.ts` around lines 398 - 403, fetchPapers is using
fetch(`${API_BASE_URL}/papers/library`) without cache control, so Next.js server
components may return stale cached results; update the fetch in the fetchPapers
function (and any other direct fetch to `${API_BASE_URL}/papers/library` such as
calls from fetchScholarDetails) to include the RequestInit option cache:
"no-store" so responses are not cached by Next.js and fresh data is returned
after save/unsave actions.
Closes jerry609#26 Signed-off-by: LIU BOYU <oor2020@163.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/paperbot/infrastructure/stores/paper_store.py (1)
1-25:⚠️ Potential issue | 🔴 CriticalDuplicate import blocks — will shadow earlier imports and fail linting.
Lines 1–9 and 11–25 duplicate imports of
datetime,timezone,Any,Dict,List,Optional,func,select,Base, andPaperModel. Ruff flags all of these as F811. Consolidate into a single import block.Proposed consolidated imports
from __future__ import annotations +import json -from datetime import datetime, timezone -from typing import Any, Dict, Iterable, List, Optional - -from sqlalchemy import desc, func, select - -from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -from paperbot.infrastructure.stores.models import Base, PaperJudgeScoreModel, PaperModel - -import json from dataclasses import dataclass from datetime import datetime, timezone -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, Iterable, List, Optional, Tuple -from sqlalchemy import Integer, cast, func, or_, select +from sqlalchemy import Integer, cast, desc, func, or_, select +from paperbot.domain.harvest import HarvestedPaper, HarvestSource +from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi from paperbot.utils.logging_config import Logger, LogFiles -from paperbot.domain.harvest import HarvestedPaper, HarvestSource from paperbot.infrastructure.stores.models import ( Base, HarvestRunModel, PaperFeedbackModel, + PaperJudgeScoreModel, PaperModel, ) from paperbot.infrastructure.stores.sqlalchemy_db import SessionProvider, get_db_urlalembic/versions/0003_paper_registry.py (1)
54-94:⚠️ Potential issue | 🔴 CriticalTwo migrations creating the same
paperstable with incompatible schemas will cause upgrade failures.Migrations 0003 and 0007 both attempt to create the
paperstable with completely different schemas. Because migrations execute in strict order (0003 → 0004 → 0005 → 0006 → 0007), 0003 always creates the table first with the old schema (17 columns:arxiv_id,doi,title,external_url,source,published_at,first_seen_at,metadata_json, etc.).When 0007 runs, it correctly skips table creation (
if _is_offline() or not _has_table("papers")), but then unconditionally calls_upgrade_create_indexes(), which attempts to create indexes on columns that don't exist in the 0003 schema:semantic_scholar_id,openalex_id,title_hash,year,citation_count,primary_source. The_create_index()function only validates that the index name doesn't already exist—it cannot prevent the database from rejecting attempts to index non-existent columns.Result: A fresh
alembic upgrade headwill fail partway through 0007 with a database error about missing columns.Consider replacing this conditional approach with a single canonical migration that creates the final schema, or add an
ALTER TABLEstep in 0007 to add the missing columns when the 0003 schema is detected.
🤖 Fix all issues with AI agents
In `@alembic/versions/0007_paper_harvest_tables.py`:
- Around line 136-138: The downgrade() in this migration blindly drops "papers"
even though that table may have been created by the earlier 0003 migration;
change the downgrade so it only drops tables this migration actually creates
(keep op.drop_table("harvest_runs") but remove or guard
op.drop_table("papers")). Update the downgrade() in 0007_paper_harvest_tables.py
to either remove the op.drop_table("papers") call or wrap it in a runtime check
that ensures this migration created the table (e.g., check current revision or
whether this migration added the table) so you don't break
0003_paper_registry.py's expectations.
In `@src/paperbot/infrastructure/stores/models.py`:
- Line 641: The PaperModel's non-nullable mapped column title_hash currently
lacks a default which will cause IntegrityError on inserts that don't set it;
update the mapped_column declaration for title_hash in the PaperModel to provide
a default (e.g., default="" or server_default text matching the migration) or
ensure all creation paths call the title hashing helper (where title
normalization/hash is computed) before persisting, and keep the symbol name
title_hash and class PaperModel so callers and migrations remain consistent.
- Around line 629-706: The PaperModel redesign removed columns and methods still
referenced elsewhere causing AttributeError; restore compatibility by
reintroducing the missing fields and accessors (external_url, published_at as
DateTime, first_seen_at as DateTime, source, metadata_json) and methods
set_authors(), set_metadata(), get_metadata() on the PaperModel class, ensuring
metadata_json is kept as JSON text and set_metadata/get_metadata
serialize/deserialize it; alternatively, update callers (calls to set_authors,
set_metadata, get_metadata and attribute accesses p.external_url, p.source,
p.published_at, p.first_seen_at and queries referencing PaperModel.external_url)
to use the new schema names (publication_date, primary_source, sources_json) and
JSON helpers (set_keywords/set_fields_of_study/set_sources/get_sources) so all
usages align with the model—pick one approach and apply consistently across
PaperModel, the code invoking set_authors/set_metadata/get_metadata, and the
locations that read external_url/source/published_at/first_seen_at.
In `@src/paperbot/infrastructure/stores/paper_store.py`:
- Around line 575-583: get_paper_by_id (and similarly get_harvest_run,
list_harvest_runs, search_papers) currently returns ORM objects from inside a
closed session causing DetachedInstanceError for lazy relationships; fix by
either creating the session with expire_on_commit=False, eagerly loading the
required relationships via joinedload/selectinload on the select() for
PaperModel (e.g., feedback_rows, judge_scores, reading_status_rows), or
expunging the instances before returning with
session.expunge()/session.expunge_all(); update the relevant methods
(get_paper_by_id, get_harvest_run, list_harvest_runs, search_papers) to use one
of these approaches so returned models can safely access relationships after the
context manager closes.
- Around line 801-824: There are two duplicate serializers: module-level
paper_to_dict and SqlAlchemyPaperStore._paper_to_dict; consolidate them by
creating a single canonical serializer (keep the module-level paper_to_dict
name) that returns a superset of both schemas (include semantic_scholar_id,
openalex_id, fields_of_study, primary_source, sources plus legacy fields
external_url, first_seen_at, source, metadata and all existing keys like id,
doi, title, authors, created_at/updated_at isoformat, etc.), use existing model
helpers (get_authors/get_keywords/get_fields_of_study/get_sources) and ensure
date formatting, then modify SqlAlchemyPaperStore._paper_to_dict to call this
unified paper_to_dict and remove its duplicate implementation.
- Around line 73-95: The SqlAlchemyPaperStore class currently contains only a
docstring and should be removed or implemented: either delete the empty
SqlAlchemyPaperStore declaration and update any references/usages to use
PaperStore (or the correct concrete class), or move the intended implementation
(methods like upsert, search, get/save library functions) into
SqlAlchemyPaperStore’s class body so it becomes the concrete repository
implementation; ensure you also adjust/retain symbols such as LibraryPaper and
PaperStore and fix any imports or references elsewhere that expected
SqlAlchemyPaperStore.
🧹 Nitpick comments (8)
src/paperbot/infrastructure/stores/research_store.py (2)
337-384: Log messages lack contextual identifiers — hard to correlate in production.All four log calls emit static strings with no request-specific context (e.g.,
user_id,track_id,paper_id). When multiple requests run concurrently, these messages are nearly useless for debugging. Consider interpolating key identifiers.♻️ Example for a couple of the lines
- Logger.info("Recording paper feedback", file=LogFiles.HARVEST) + Logger.info( + f"Recording paper feedback user={user_id} track={track_id} paper={paper_id}", + file=LogFiles.HARVEST, + ) ... - Logger.error("Track not found", file=LogFiles.HARVEST) + Logger.error( + f"Track not found user={user_id} track={track_id}", + file=LogFiles.HARVEST, + )
1104-1115: Existing TODO:scalar_one_or_none()can raiseMultipleResultsFound.The comment on lines 1104-1105 acknowledges this, and the same risk applies to the title lookup on lines 1121-1123. Both could raise
MultipleResultsFoundat runtime if duplicate data exists. Switching to.first()(as the TODO suggests) would prevent an unhandled exception.Would you like me to open an issue to track replacing these
scalar_one_or_none()calls with.first()across both the URL and title lookups?src/paperbot/infrastructure/stores/models.py (1)
638-640: Several lines exceed 100 characters.Lines 638–640, 661–662, 665, 723, and 730 exceed the configured
line-length = 100. As per coding guidelines, "Use Black formatting withline-length = 100for Python code."Example fix for lines 638-640
- arxiv_id: Mapped[Optional[str]] = mapped_column(String(32), unique=True, nullable=True, index=True) - semantic_scholar_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) - openalex_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) + arxiv_id: Mapped[Optional[str]] = mapped_column( + String(32), unique=True, nullable=True, index=True + ) + semantic_scholar_id: Mapped[Optional[str]] = mapped_column( + String(64), unique=True, nullable=True, index=True + ) + openalex_id: Mapped[Optional[str]] = mapped_column( + String(64), unique=True, nullable=True, index=True + )docs/architecture_overview.md (2)
3-6: Authorship attribution says "Claude Code" — consider using the actual author or team name.Attributing official project documentation to an AI tool may confuse contributors and doesn't convey ownership or accountability.
25-63: Add language identifiers to fenced code blocks for ASCII diagrams.The markdownlint MD040 rule flags these blocks. Using
```textfor ASCII-art diagrams satisfies the linter and signals to renderers that no syntax highlighting is needed. This applies to all diagram blocks throughout the file (lines 25, 67, 88, 217, 268, 318, 393, 449, 499, 578, 637, 725).alembic/versions/0007_paper_harvest_tables.py (1)
131-131: Redundant index onharvest_runs.run_id.Line 99 declares
run_idwithunique=True, which already creates a unique index. The explicitix_harvest_runs_run_idindex at line 131 is redundant.src/paperbot/infrastructure/stores/paper_store.py (2)
692-715:create_harvest_runreturns a detached model aftersession.commit()+session.refresh().After the
withblock exits, the session is closed and the returnedHarvestRunModelis detached. Whilesession.refresh(run)loads current state, any subsequent attribute access that triggers lazy loading will raiseDetachedInstanceError. SinceHarvestRunModelhas no relationships, this is less risky but still worth noting — the pattern is fragile if relationships are added later.
793-798: Bareexcept Exception: passsilently swallows errors inclose().Per Ruff S110/BLE001, this hides errors that could indicate resource leaks or connection pool issues. At minimum, log the exception.
Proposed fix
def close(self) -> None: """Close database connections.""" try: self._provider.engine.dispose() - except Exception: - pass + except Exception as exc: + Logger.warning(f"Error closing database connections: {exc}", file=LogFiles.HARVEST)
| def downgrade() -> None: | ||
| op.drop_table("harvest_runs") | ||
| op.drop_table("papers") |
There was a problem hiding this comment.
downgrade() drops tables but doesn't account for the 0003 migration also creating papers.
If 0003 created the papers table and 0007 skipped creation, running downgrade on 0007 will drop a table it didn't create, breaking 0003's expectations. The two migrations need coordinated downgrade logic.
This is a downstream effect of the dual-migration issue flagged in 0003_paper_registry.py.
🤖 Prompt for AI Agents
In `@alembic/versions/0007_paper_harvest_tables.py` around lines 136 - 138, The
downgrade() in this migration blindly drops "papers" even though that table may
have been created by the earlier 0003 migration; change the downgrade so it only
drops tables this migration actually creates (keep op.drop_table("harvest_runs")
but remove or guard op.drop_table("papers")). Update the downgrade() in
0007_paper_harvest_tables.py to either remove the op.drop_table("papers") call
or wrap it in a runtime check that ensures this migration created the table
(e.g., check current revision or whether this migration added the table) so you
don't break 0003_paper_registry.py's expectations.
| class PaperModel(Base): | ||
| """Harvested paper metadata from multiple sources.""" | ||
|
|
||
| __tablename__ = "papers" | ||
|
|
||
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
|
|
||
| # Canonical identifiers (for deduplication) | ||
| doi: Mapped[Optional[str]] = mapped_column(String(128), unique=True, nullable=True, index=True) | ||
| arxiv_id: Mapped[Optional[str]] = mapped_column(String(32), unique=True, nullable=True, index=True) | ||
| semantic_scholar_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) | ||
| openalex_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) | ||
| title_hash: Mapped[str] = mapped_column(String(64), index=True) # SHA256 of normalized title | ||
|
|
||
| # Core metadata | ||
| title: Mapped[str] = mapped_column(Text, default="") | ||
| abstract: Mapped[str] = mapped_column(Text, default="") | ||
| authors_json: Mapped[str] = mapped_column(Text, default="[]") | ||
| year: Mapped[Optional[int]] = mapped_column(Integer, nullable=True, index=True) | ||
| venue: Mapped[Optional[str]] = mapped_column(String(256), nullable=True, index=True) | ||
| publication_date: Mapped[Optional[str]] = mapped_column(String(32), nullable=True) | ||
| citation_count: Mapped[int] = mapped_column(Integer, default=0, index=True) | ||
|
|
||
| # URLs (no PDF download, just references) | ||
| url: Mapped[Optional[str]] = mapped_column(String(512), nullable=True) | ||
| pdf_url: Mapped[Optional[str]] = mapped_column(String(512), nullable=True) | ||
|
|
||
| # Classification | ||
| keywords_json: Mapped[str] = mapped_column(Text, default="[]") | ||
| fields_of_study_json: Mapped[str] = mapped_column(Text, default="[]") | ||
|
|
||
| # Source tracking | ||
| primary_source: Mapped[str] = mapped_column(String(32), default="") # First source that found this paper | ||
| sources_json: Mapped[str] = mapped_column(Text, default="[]") # All sources that returned this paper | ||
|
|
||
| # Timestamps | ||
| created_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True), nullable=True, index=True) | ||
| updated_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True), nullable=True) | ||
| deleted_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True), nullable=True) # Soft delete | ||
|
|
||
| # Relationships | ||
| feedback_rows = relationship("PaperFeedbackModel", back_populates="paper") | ||
| judge_scores = relationship("PaperJudgeScoreModel", back_populates="paper") | ||
| reading_status_rows = relationship("PaperReadingStatusModel", back_populates="paper") | ||
|
|
||
| def get_authors(self) -> list: | ||
| try: | ||
| return json.loads(self.authors_json or "[]") | ||
| except Exception: | ||
| return [] | ||
|
|
||
| def get_keywords(self) -> list: | ||
| try: | ||
| return json.loads(self.keywords_json or "[]") | ||
| except Exception: | ||
| return [] | ||
|
|
||
| def get_fields_of_study(self) -> list: | ||
| try: | ||
| return json.loads(self.fields_of_study_json or "[]") | ||
| except Exception: | ||
| return [] | ||
|
|
||
| def get_sources(self) -> list: | ||
| try: | ||
| return json.loads(self.sources_json or "[]") | ||
| except Exception: | ||
| return [] | ||
|
|
||
| def set_keywords(self, keywords: list) -> None: | ||
| self.keywords_json = json.dumps(keywords or [], ensure_ascii=False) | ||
|
|
||
| def set_fields_of_study(self, fields: list) -> None: | ||
| self.fields_of_study_json = json.dumps(fields or [], ensure_ascii=False) | ||
|
|
||
| def set_sources(self, sources: list) -> None: | ||
| self.sources_json = json.dumps(sources or [], ensure_ascii=False) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file structure
echo "=== Locating paper_store.py ==="
find . -name "paper_store.py" -type f
echo -e "\n=== Checking models.py for old vs new PaperModel definitions ==="
rg -n "class PaperModel" --type py
echo -e "\n=== Searching for references to removed PaperModel attributes in all Python files ==="
rg -n '\b(first_seen_at|external_url|set_authors|set_metadata|get_metadata|published_at)\b' --type py -A 2 | head -100Repository: jerry609/PaperBot
Length of output: 8004
🏁 Script executed:
#!/bin/bash
# Check the actual structure of paper_store.py to see if SqlAlchemyPaperStore exists
echo "=== Checking paper_store.py for SqlAlchemyPaperStore class ==="
rg -n "class.*PaperStore" --type py -A 5Repository: jerry609/PaperBot
Length of output: 3407
🏁 Script executed:
#!/bin/bash
# Check research_store.py around lines 993-998 to see context
echo "=== research_store.py lines 990-1010 ==="
sed -n '990,1010p' src/paperbot/infrastructure/stores/research_store.py
echo -e "\n=== research_store.py lines 1108-1115 ==="
sed -n '1108,1115p' src/paperbot/infrastructure/stores/research_store.py
echo -e "\n=== paper_store.py lines 110-160 ==="
sed -n '110,160p' src/paperbot/infrastructure/stores/paper_store.py
echo -e "\n=== paper_store.py lines 73-90 (SqlAlchemyPaperStore definition) ==="
sed -n '73,90p' src/paperbot/infrastructure/stores/paper_store.pyRepository: jerry609/PaperBot
Length of output: 3819
🏁 Script executed:
#!/bin/bash
# Search for method that uses these attributes in paper_store.py
echo "=== Searching for where PaperModel attributes are set/accessed in paper_store.py ==="
rg -n "\.set_authors\(|\.set_metadata\(|\.get_metadata\(" src/paperbot/infrastructure/stores/paper_store.pyRepository: jerry609/PaperBot
Length of output: 344
🏁 Script executed:
#!/bin/bash
# Check the full PaperModel definition to confirm removed columns
echo "=== PaperModel columns and methods (lines 629-706) ==="
sed -n '629,706p' src/paperbot/infrastructure/stores/models.py | grep -E "^\s*(def |[a-z_]+:)"Repository: jerry609/PaperBot
Length of output: 2440
🏁 Script executed:
#!/bin/bash
# Double-check: are there any property decorators or __getattr__ that might provide backwards compatibility?
echo "=== Checking for property decorators or __getattr__ in PaperModel ==="
sed -n '629,750p' src/paperbot/infrastructure/stores/models.py | grep -E "@property|__getattr__|external_url|published_at|first_seen_at|source|set_metadata|get_metadata|set_authors"Repository: jerry609/PaperBot
Length of output: 868
PaperModel drops columns and methods that PaperStore and research_store.py still reference — runtime AttributeError.
The redesigned PaperModel removes external_url, published_at (as datetime), first_seen_at, source, metadata_json, set_authors(), set_metadata(), and get_metadata(). However:
paper_store.py:213callsrow.set_authors(authors)paper_store.py:215callsrow.set_metadata(metadata)paper_store.py:350callsrow.get_metadata()research_store.py:993–998accessesp.external_url,p.source,p.published_at(as datetime), andp.first_seen_atresearch_store.py:1111queriesPaperModel.external_urlcolumn
These will raise AttributeError at runtime. Either restore the removed columns/methods on PaperModel or update all dependent code to use the new schema (publication_date as string, primary_source/sources_json, etc.).
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 677-677: Do not catch blind exception: Exception
(BLE001)
[warning] 683-683: Do not catch blind exception: Exception
(BLE001)
[warning] 689-689: Do not catch blind exception: Exception
(BLE001)
[warning] 695-695: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/models.py` around lines 629 - 706, The
PaperModel redesign removed columns and methods still referenced elsewhere
causing AttributeError; restore compatibility by reintroducing the missing
fields and accessors (external_url, published_at as DateTime, first_seen_at as
DateTime, source, metadata_json) and methods set_authors(), set_metadata(),
get_metadata() on the PaperModel class, ensuring metadata_json is kept as JSON
text and set_metadata/get_metadata serialize/deserialize it; alternatively,
update callers (calls to set_authors, set_metadata, get_metadata and attribute
accesses p.external_url, p.source, p.published_at, p.first_seen_at and queries
referencing PaperModel.external_url) to use the new schema names
(publication_date, primary_source, sources_json) and JSON helpers
(set_keywords/set_fields_of_study/set_sources/get_sources) so all usages align
with the model—pick one approach and apply consistently across PaperModel, the
code invoking set_authors/set_metadata/get_metadata, and the locations that read
external_url/source/published_at/first_seen_at.
| arxiv_id: Mapped[Optional[str]] = mapped_column(String(32), unique=True, nullable=True, index=True) | ||
| semantic_scholar_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) | ||
| openalex_id: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) | ||
| title_hash: Mapped[str] = mapped_column(String(64), index=True) # SHA256 of normalized title |
There was a problem hiding this comment.
title_hash has no default at the DB level — inserts without explicit value will fail.
title_hash is declared as Mapped[str] (non-nullable) with no default or server_default. If any code path creates a PaperModel without explicitly setting title_hash, SQLAlchemy will raise an IntegrityError. Consider adding default="" (as in the migration) or ensuring all creation paths compute the hash.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/models.py` at line 641, The PaperModel's
non-nullable mapped column title_hash currently lacks a default which will cause
IntegrityError on inserts that don't set it; update the mapped_column
declaration for title_hash in the PaperModel to provide a default (e.g.,
default="" or server_default text matching the migration) or ensure all creation
paths call the title hashing helper (where title normalization/hash is computed)
before persisting, and keep the symbol name title_hash and class PaperModel so
callers and migrations remain consistent.
| class SqlAlchemyPaperStore: | ||
| """Canonical paper registry with idempotent upsert for daily workflows.""" | ||
| @dataclass | ||
| class LibraryPaper: | ||
| """Paper with library metadata (saved_at, track_id, action).""" | ||
|
|
||
| paper: PaperModel | ||
| saved_at: datetime | ||
| track_id: Optional[int] | ||
| action: str | ||
|
|
||
|
|
||
| class PaperStore: | ||
| """ | ||
| Paper storage repository. | ||
|
|
||
| Handles: | ||
| - Batch upsert with DB-level deduplication | ||
| - Filter-based search with pagination | ||
| - Source tracking | ||
| - User library (saved papers) | ||
| """ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual indentation structure in paper_store.py
echo "=== Class definitions and their indentation ==="
rg -n "^class |^ class |^@dataclass" src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== References to SqlAlchemyPaperStore ==="
rg -rn "SqlAlchemyPaperStore" --type pyRepository: jerry609/PaperBot
Length of output: 1438
🏁 Script executed:
# Get the full context around lines 70-105 to see the class structure
sed -n '70,105p' src/paperbot/infrastructure/stores/paper_store.py | cat -nRepository: jerry609/PaperBot
Length of output: 1237
🏁 Script executed:
# Check the entire file structure and look for aliases
echo "=== Full class structure ==="
head -150 src/paperbot/infrastructure/stores/paper_store.py | tail -100
echo ""
echo "=== Look for 'n =' assignment or alias ==="
rg -n "^n = |^n: " src/paperbot/infrastructure/stores/paper_store.pyRepository: jerry609/PaperBot
Length of output: 3232
🏁 Script executed:
# Check the end of the file for alias or assignment
tail -20 src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== Also check for any single-letter assignments ==="
rg -n "^[a-z] = " src/paperbot/infrastructure/stores/paper_store.pyRepository: jerry609/PaperBot
Length of output: 956
🏁 Script executed:
# Search the entire file for 'n' definition
rg -n "^n " src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== Check if 'n' is assigned after classes ==="
grep -n "^n" src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== Count total lines and check near end ==="
wc -l src/paperbot/infrastructure/stores/paper_store.py
tail -50 src/paperbot/infrastructure/stores/paper_store.py | head -30Repository: jerry609/PaperBot
Length of output: 1130
🏁 Script executed:
# Search specifically for 'class n'
rg -n "class n" src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== Show context around it ==="
rg -n -B 3 -A 10 "class n" src/paperbot/infrastructure/stores/paper_store.pyRepository: jerry609/PaperBot
Length of output: 92
🏁 Script executed:
# Check if there's a __all__ export or if n is assigned to PaperStore
rg -n "__all__|^n = PaperStore|^n=PaperStore|n: Type|SqlAlchemyPaperStore.*=" src/paperbot/infrastructure/stores/paper_store.py
echo ""
echo "=== Search imports in test to understand usage ==="
head -5 tests/unit/test_paper_store.pyRepository: jerry609/PaperBot
Length of output: 230
Remove or implement SqlAlchemyPaperStore—it contains only a docstring with no class body.
SqlAlchemyPaperStore (line 73) is defined with a docstring but has no methods or attributes. The next class definition LibraryPaper (line 75) immediately follows at module level, leaving SqlAlchemyPaperStore empty and non-functional. If this class is being superseded by PaperStore, delete SqlAlchemyPaperStore entirely; otherwise, move its implementation into the class body.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/paper_store.py` around lines 73 - 95, The
SqlAlchemyPaperStore class currently contains only a docstring and should be
removed or implemented: either delete the empty SqlAlchemyPaperStore declaration
and update any references/usages to use PaperStore (or the correct concrete
class), or move the intended implementation (methods like upsert, search,
get/save library functions) into SqlAlchemyPaperStore’s class body so it becomes
the concrete repository implementation; ensure you also adjust/retain symbols
such as LibraryPaper and PaperStore and fix any imports or references elsewhere
that expected SqlAlchemyPaperStore.
| def get_paper_by_id(self, paper_id: int) -> Optional[PaperModel]: | ||
| """Get a paper by its ID.""" | ||
| with self._provider.session() as session: | ||
| return session.execute( | ||
| select(PaperModel).where( | ||
| PaperModel.id == paper_id, | ||
| PaperModel.deleted_at.is_(None), | ||
| ) | ||
| ).scalar_one_or_none() |
There was a problem hiding this comment.
Returned PaperModel from get_paper_by_id is detached from session — lazy-loaded relationships will fail.
The with self._provider.session() context manager closes the session when the block exits. The returned PaperModel instance becomes detached, and any access to lazy-loaded relationships (feedback_rows, judge_scores, reading_status_rows) will raise DetachedInstanceError.
The same issue applies to get_harvest_run (line 755), list_harvest_runs (line 762), and search_papers (line 504).
Either use expire_on_commit=False on the session, eagerly load needed relationships, or call session.expunge() before returning.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/paper_store.py` around lines 575 - 583,
get_paper_by_id (and similarly get_harvest_run, list_harvest_runs,
search_papers) currently returns ORM objects from inside a closed session
causing DetachedInstanceError for lazy relationships; fix by either creating the
session with expire_on_commit=False, eagerly loading the required relationships
via joinedload/selectinload on the select() for PaperModel (e.g., feedback_rows,
judge_scores, reading_status_rows), or expunging the instances before returning
with session.expunge()/session.expunge_all(); update the relevant methods
(get_paper_by_id, get_harvest_run, list_harvest_runs, search_papers) to use one
of these approaches so returned models can safely access relationships after the
context manager closes.
| def paper_to_dict(paper: PaperModel) -> Dict[str, Any]: | ||
| """Convert PaperModel to dictionary for API response.""" | ||
| return { | ||
| "id": paper.id, | ||
| "doi": paper.doi, | ||
| "arxiv_id": paper.arxiv_id, | ||
| "semantic_scholar_id": paper.semantic_scholar_id, | ||
| "openalex_id": paper.openalex_id, | ||
| "title": paper.title, | ||
| "abstract": paper.abstract, | ||
| "authors": paper.get_authors(), | ||
| "year": paper.year, | ||
| "venue": paper.venue, | ||
| "publication_date": paper.publication_date, | ||
| "citation_count": paper.citation_count, | ||
| "url": paper.url, | ||
| "pdf_url": paper.pdf_url, | ||
| "keywords": paper.get_keywords(), | ||
| "fields_of_study": paper.get_fields_of_study(), | ||
| "primary_source": paper.primary_source, | ||
| "sources": paper.get_sources(), | ||
| "created_at": paper.created_at.isoformat() if paper.created_at else None, | ||
| "updated_at": paper.updated_at.isoformat() if paper.updated_at else None, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Module-level paper_to_dict duplicates SqlAlchemyPaperStore._paper_to_dict with a different schema.
paper_to_dict (line 801) includes harvest-specific fields (semantic_scholar_id, openalex_id, fields_of_study, primary_source, sources) while _paper_to_dict (line 334) includes legacy fields (external_url, first_seen_at, source, metadata). Having two serializers for the same model is a maintenance hazard. Consolidate into a single function.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/paper_store.py` around lines 801 - 824,
There are two duplicate serializers: module-level paper_to_dict and
SqlAlchemyPaperStore._paper_to_dict; consolidate them by creating a single
canonical serializer (keep the module-level paper_to_dict name) that returns a
superset of both schemas (include semantic_scholar_id, openalex_id,
fields_of_study, primary_source, sources plus legacy fields external_url,
first_seen_at, source, metadata and all existing keys like id, doi, title,
authors, created_at/updated_at isoformat, etc.), use existing model helpers
(get_authors/get_keywords/get_fields_of_study/get_sources) and ensure date
formatting, then modify SqlAlchemyPaperStore._paper_to_dict to call this unified
paper_to_dict and remove its duplicate implementation.
Closes jerry609#33, jerry609#34, jerry609#35, jerry609#36, jerry609#37, jerry609#38 Signed-off-by: LIU BOYU <oor2020@163.com>
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 (3)
src/paperbot/api/routes/research.py (2)
28-46:⚠️ Potential issue | 🟡 MinorFix forward reference to
PaperStoreto avoid F821.Ruff flags the annotation as undefined; add a TYPE_CHECKING import so type checking passes without runtime import cost.
🧩 Suggested fix
-from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING @@ -_paper_store: Optional["PaperStore"] = None +if TYPE_CHECKING: + from paperbot.infrastructure.stores.paper_store import PaperStore + +_paper_store: Optional["PaperStore"] = None
655-733:⚠️ Potential issue | 🟡 MinorAdd/update tests for the save-to-library feedback flow and new listing endpoint.
This changes persisted behavior and API surface; please add coverage for save-with-metadata and list feedback results.
As per coding guidelines, "If behavior changes, add or update tests".src/paperbot/infrastructure/stores/paper_store.py (1)
3-25:⚠️ Potential issue | 🟠 MajorRemove duplicate imports to satisfy Ruff F811.
The file currently redefines several imports; consolidate to a single import block.
🧩 Suggested fix
-from datetime import datetime, timezone -from typing import Any, Dict, Iterable, List, Optional - -from sqlalchemy import desc, func, select - -from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -from paperbot.infrastructure.stores.models import Base, PaperJudgeScoreModel, PaperModel - -import json -from dataclasses import dataclass -from datetime import datetime, timezone -from typing import Any, Dict, List, Optional, Tuple - -from sqlalchemy import Integer, String, cast, func, or_, select +import json +from dataclasses import dataclass +from datetime import datetime, timezone +from typing import Any, Dict, Iterable, List, Optional, Tuple + +from sqlalchemy import Integer, String, cast, desc, func, or_, select + +from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi +from paperbot.infrastructure.stores.models import Base, PaperJudgeScoreModel, PaperModel
🤖 Fix all issues with AI agents
In `@src/paperbot/api/routes/harvest.py`:
- Around line 32-52: The forward reference to SqlAlchemyResearchStore triggers
F821 because the symbol isn't imported for type checkers; add "from typing
import TYPE_CHECKING" at the top and then use a guarded import block "if
TYPE_CHECKING: from paperbot.infrastructure.stores.research_store import
SqlAlchemyResearchStore" so the annotation Optional["SqlAlchemyResearchStore"]
and the _get_research_store function resolve cleanly for linters without
importing the class at runtime.
- Around line 122-139: Add or update tests to cover the new streaming harvest
endpoint: write tests that call the harvest_papers route (POST with a
HarvestRequest payload) and assert you receive a StreamingResponse with
media_type "text/event-stream" and the expected headers ("Cache-Control" and
"Connection"); consume the generator produced by
wrap_generator(harvest_stream(...)) and validate SSE-formatted events and key
progress messages (start, intermediate progress, completion or error) are
emitted in order; use the same request shapes used by HarvestRequest and
mock/stub harvest_stream to produce deterministic SSE output so you can assert
event payloads and that trace_id logging behavior (set_trace_id + Logger.info)
is exercised where appropriate.
In `@src/paperbot/api/routes/research.py`:
- Around line 707-709: The variable new_count assigned from
paper_store.upsert_papers_batch([paper]) is unused and should be removed to
satisfy linters; update the call in the code (the block that currently does
Logger.info("Calling paper store to upsert paper", file=LogFiles.HARVEST)
followed by new_count, _ = paper_store.upsert_papers_batch([paper])) to either
ignore the return value (call paper_store.upsert_papers_batch([paper]) without
assignment) or use a placeholder for the first value (e.g. _, _ =
paper_store.upsert_papers_batch([paper])) so only needed values are captured.
In `@src/paperbot/infrastructure/harvesters/openalex_harvester.py`:
- Around line 103-116: The UI shows an incorrect total after client-side venue
filtering because total_found is taken from data.get("meta", {}).get("count",
...) before filtering; update the logic in the OpenAlex harvester (around
total_found, venues, and papers variables) so that when venues is truthy you set
total_found = len(papers) (i.e., after filtering) otherwise keep the original
meta count; ensure this change occurs after the venue filter and before the
logger.info call that reports the found count.
In `@src/paperbot/infrastructure/stores/paper_store.py`:
- Around line 725-733: The delete in remove_from_library only matches rows where
PaperFeedbackModel.paper_id equals the numeric id string, so feedback rows
stored with external IDs are not removed; update the delete WHERE clause on
PaperFeedbackModel.__table__.delete() to match either the numeric paper_id OR
the external id column used for external sources (e.g.,
PaperFeedbackModel.external_id == str(paper_id)), keeping the existing
conditions PaperFeedbackModel.user_id == user_id and PaperFeedbackModel.action
== "save", and ensure you cast the incoming paper_id to string before comparing.
🧹 Nitpick comments (1)
web/src/app/api/papers/[paperId]/save/route.ts (1)
27-38: Add tests for this new route.Both handlers are new and introduce validation logic plus upstream proxying. Consider adding unit/integration tests covering at least: valid ID proxying, invalid/negative/non-numeric ID rejection (400), and upstream error handling.
As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: "If behavior changes, add or update tests".
| # Lazy-initialized stores | ||
| _paper_store: Optional[PaperStore] = None | ||
| _research_store: Optional["SqlAlchemyResearchStore"] = None | ||
|
|
||
|
|
||
| def _get_paper_store() -> PaperStore: | ||
| """Lazy initialization of paper store.""" | ||
| global _paper_store | ||
| if _paper_store is None: | ||
| _paper_store = PaperStore() | ||
| return _paper_store | ||
|
|
||
|
|
||
| def _get_research_store() -> "SqlAlchemyResearchStore": | ||
| """Lazy initialization of research store.""" | ||
| from paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore | ||
|
|
||
| global _research_store | ||
| if _research_store is None: | ||
| _research_store = SqlAlchemyResearchStore() | ||
| return _research_store |
There was a problem hiding this comment.
Fix forward reference to SqlAlchemyResearchStore to avoid F821.
Add a TYPE_CHECKING import so the annotation resolves cleanly.
🧩 Suggested fix
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, TYPE_CHECKING
@@
-_research_store: Optional["SqlAlchemyResearchStore"] = None
+if TYPE_CHECKING:
+ from paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore
+
+_research_store: Optional["SqlAlchemyResearchStore"] = None🧰 Tools
🪛 Ruff (0.15.0)
[error] 34-34: Undefined name SqlAlchemyResearchStore
(F821)
[error] 45-45: Undefined name SqlAlchemyResearchStore
(F821)
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/harvest.py` around lines 32 - 52, The forward
reference to SqlAlchemyResearchStore triggers F821 because the symbol isn't
imported for type checkers; add "from typing import TYPE_CHECKING" at the top
and then use a guarded import block "if TYPE_CHECKING: from
paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore" so
the annotation Optional["SqlAlchemyResearchStore"] and the _get_research_store
function resolve cleanly for linters without importing the class at runtime.
| @router.post("/harvest") | ||
| async def harvest_papers(request: HarvestRequest): | ||
| """ | ||
| Harvest papers from multiple sources. | ||
|
|
||
| Returns Server-Sent Events with progress updates. | ||
| """ | ||
| trace_id = set_trace_id() | ||
| Logger.info(f"Starting harvest request: keywords={request.keywords}", file=LogFiles.HARVEST) | ||
| return StreamingResponse( | ||
| wrap_generator(harvest_stream(request)), | ||
| media_type="text/event-stream", | ||
| headers={ | ||
| "Cache-Control": "no-cache", | ||
| "Connection": "keep-alive", | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add/update tests for the new harvest streaming endpoint.
This adds new API behavior and SSE output; please add coverage.
As per coding guidelines, "If behavior changes, add or update tests".
🧰 Tools
🪛 Ruff (0.15.0)
[error] 129-129: Local variable trace_id is assigned to but never used
Remove assignment to unused variable trace_id
(F841)
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/harvest.py` around lines 122 - 139, Add or update
tests to cover the new streaming harvest endpoint: write tests that call the
harvest_papers route (POST with a HarvestRequest payload) and assert you receive
a StreamingResponse with media_type "text/event-stream" and the expected headers
("Cache-Control" and "Connection"); consume the generator produced by
wrap_generator(harvest_stream(...)) and validate SSE-formatted events and key
progress messages (start, intermediate progress, completion or error) are
emitted in order; use the same request shapes used by HarvestRequest and
mock/stub harvest_stream to produce deterministic SSE output so you can assert
event payloads and that trace_id logging behavior (set_trace_id + Logger.info)
is exercised where appropriate.
| Logger.info("Calling paper store to upsert paper", file=LogFiles.HARVEST) | ||
| new_count, _ = paper_store.upsert_papers_batch([paper]) | ||
|
|
There was a problem hiding this comment.
Avoid unused new_count assignment.
Ruff will flag this; either ignore the return value or use _ placeholders.
🧩 Suggested fix
- new_count, _ = paper_store.upsert_papers_batch([paper])
+ paper_store.upsert_papers_batch([paper])🧰 Tools
🪛 Ruff (0.15.0)
[warning] 708-708: Unpacked variable new_count is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/research.py` around lines 707 - 709, The variable
new_count assigned from paper_store.upsert_papers_batch([paper]) is unused and
should be removed to satisfy linters; update the call in the code (the block
that currently does Logger.info("Calling paper store to upsert paper",
file=LogFiles.HARVEST) followed by new_count, _ =
paper_store.upsert_papers_batch([paper])) to either ignore the return value
(call paper_store.upsert_papers_batch([paper]) without assignment) or use a
placeholder for the first value (e.g. _, _ =
paper_store.upsert_papers_batch([paper])) so only needed values are captured.
| results = data.get("results", []) | ||
| papers = [self._to_paper(r, rank=i) for i, r in enumerate(results)] | ||
|
|
||
| # Filter by venue if specified | ||
| if venues: | ||
| venue_set = {v.lower() for v in venues} | ||
| papers = [ | ||
| p | ||
| for p in papers | ||
| if p.venue and any(v in p.venue.lower() for v in venue_set) | ||
| ] | ||
|
|
||
| total_found = data.get("meta", {}).get("count", len(papers)) | ||
| logger.info(f"OpenAlex harvester found {len(papers)} papers for query: {query}") |
There was a problem hiding this comment.
Adjust total_found when venue filtering is applied client-side.
After filtering, the API’s meta count no longer matches the returned list, which can mislead pagination/UI. Consider setting total_found to the filtered length when venues is provided.
🩹 Suggested fix
- total_found = data.get("meta", {}).get("count", len(papers))
+ total_found = data.get("meta", {}).get("count", len(papers))
+ if venues:
+ total_found = len(papers)📝 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.
| results = data.get("results", []) | |
| papers = [self._to_paper(r, rank=i) for i, r in enumerate(results)] | |
| # Filter by venue if specified | |
| if venues: | |
| venue_set = {v.lower() for v in venues} | |
| papers = [ | |
| p | |
| for p in papers | |
| if p.venue and any(v in p.venue.lower() for v in venue_set) | |
| ] | |
| total_found = data.get("meta", {}).get("count", len(papers)) | |
| logger.info(f"OpenAlex harvester found {len(papers)} papers for query: {query}") | |
| results = data.get("results", []) | |
| papers = [self._to_paper(r, rank=i) for i, r in enumerate(results)] | |
| # Filter by venue if specified | |
| if venues: | |
| venue_set = {v.lower() for v in venues} | |
| papers = [ | |
| p | |
| for p in papers | |
| if p.venue and any(v in p.venue.lower() for v in venue_set) | |
| ] | |
| total_found = data.get("meta", {}).get("count", len(papers)) | |
| if venues: | |
| total_found = len(papers) | |
| logger.info(f"OpenAlex harvester found {len(papers)} papers for query: {query}") |
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/harvesters/openalex_harvester.py` around lines
103 - 116, The UI shows an incorrect total after client-side venue filtering
because total_found is taken from data.get("meta", {}).get("count", ...) before
filtering; update the logic in the OpenAlex harvester (around total_found,
venues, and papers variables) so that when venues is truthy you set total_found
= len(papers) (i.e., after filtering) otherwise keep the original meta count;
ensure this change occurs after the venue filter and before the logger.info call
that reports the found count.
| def remove_from_library(self, user_id: str, paper_id: int) -> bool: | ||
| """Remove paper from user's library by deleting 'save' feedback.""" | ||
| with self._provider.session() as session: | ||
| stmt = ( | ||
| PaperFeedbackModel.__table__.delete().where( | ||
| PaperFeedbackModel.user_id == user_id, | ||
| PaperFeedbackModel.paper_id == str(paper_id), | ||
| PaperFeedbackModel.action == "save", | ||
| ) |
There was a problem hiding this comment.
remove_from_library can’t delete saves stored with external IDs.
Feedback entries created via /research/papers/feedback store external IDs (semantic_scholar/arxiv/openalex). Deleting only by numeric paper_id leaves those rows behind, so “remove” appears to fail for many items.
🛠️ Suggested fix
def remove_from_library(self, user_id: str, paper_id: int) -> bool:
"""Remove paper from user's library by deleting 'save' feedback."""
with self._provider.session() as session:
+ paper = session.execute(
+ select(PaperModel).where(PaperModel.id == paper_id)
+ ).scalar_one_or_none()
+ ids = {str(paper_id)}
+ if paper:
+ ids.update(
+ {
+ pid
+ for pid in [
+ paper.semantic_scholar_id,
+ paper.arxiv_id,
+ paper.openalex_id,
+ ]
+ if pid
+ }
+ )
stmt = (
PaperFeedbackModel.__table__.delete().where(
PaperFeedbackModel.user_id == user_id,
- PaperFeedbackModel.paper_id == str(paper_id),
+ PaperFeedbackModel.paper_id.in_(ids),
PaperFeedbackModel.action == "save",
)
)🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/paper_store.py` around lines 725 - 733,
The delete in remove_from_library only matches rows where
PaperFeedbackModel.paper_id equals the numeric id string, so feedback rows
stored with external IDs are not removed; update the delete WHERE clause on
PaperFeedbackModel.__table__.delete() to match either the numeric paper_id OR
the external id column used for external sources (e.g.,
PaperFeedbackModel.external_id == str(paper_id)), keeping the existing
conditions PaperFeedbackModel.user_id == user_id and PaperFeedbackModel.action
== "save", and ensure you cast the incoming paper_id to string before comparing.
Closes #26
Summary by CodeRabbit
New Features
Documentation
Tests