Feat/harvest aligned base v2#43
Conversation
Summary of ChangesHello @jerry609, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's ability to enrich paper data by integrating GitHub repository information. It establishes the necessary database schema, provides robust API mechanisms for data interaction, and automates the collection of this valuable metadata within the daily paper processing pipeline. These changes lay the groundwork for more comprehensive paper insights and align with the strategic vision for advanced browser automation capabilities outlined in the updated project roadmap. 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds DB migration, a new PaperRepo ORM, store ingestion/listing, API endpoints and async enqueue for repo enrichment, plus unit tests and supporting docs and agent/runtime abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as "API Route\n(paperscool.py)"
participant Enqueuer as "Async\nEnqueuer"
participant Worker as "Background\nWorker"
participant Store as "Research\nStore"
participant DB as "Database"
Client->>API: POST /paperscool/repos (persist=True)
API->>API: collect repos (multi-step)
API->>API: build enrichment rows
API->>Enqueuer: _enqueue_repo_enrichment_async(rows, source)
Enqueuer->>Worker: dispatch background task
Worker->>Store: ingest_repo_enrichment_rows(rows)
Store->>DB: upsert PaperRepoModel rows
DB-->>Store: commit
Store-->>Worker: persist_summary
Worker-->>API: (async) persist_summary
API-->>Client: 200 OK + persist_summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end “paper ↔ GitHub repo” enrichment persistence: repo metadata can be extracted/enriched from PapersCool results, stored against canonical papers, and retrieved via new research API endpoints. It also aligns the paper registry/store layer with the newer harvest baseline schema and expands unit test coverage for these new behaviors.
Changes:
- Add
PaperRepoModel+ Alembic migration and persistence APIs inSqlAlchemyResearchStore(ingest_repo_enrichment_rows,list_paper_repos, include repos in paper detail). - Add API endpoint
GET /api/research/papers/{paper_id}/reposand extend PapersCool repo-enrichment route to optionally persist results; enqueue async persistence after DailyPaper generation. - Add/extend unit tests covering repo persistence, retrieval, and PapersCool persistence flow.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_research_paper_registry_routes.py | Extends detail-route assertions to include repos; adds /repos route test. |
| tests/unit/test_paperscool_route.py | Adds coverage for persisting repo enrichment via PapersCool route and enqueue behavior after daily run. |
| tests/unit/test_paper_repo_persistence.py | New persistence-focused tests for ingest/list/update and unresolved paper behavior. |
| src/paperbot/infrastructure/stores/research_store.py | Implements repo ingest/list and injects repos into paper detail response; adds parsing helpers. |
| src/paperbot/infrastructure/stores/paper_store.py | Aligns paper upsert/list behaviors with harvest baseline and adds backward-compat aliasing. |
| src/paperbot/infrastructure/stores/models.py | Adds PaperRepoModel and relationship to PaperModel. |
| src/paperbot/api/routes/research.py | Adds GET /research/papers/{paper_id}/repos response model and handler. |
| src/paperbot/api/routes/paperscool.py | Adds persist option to repos API; adds async best-effort repo persistence after daily report generation. |
| docs/ROADMAP_TODO.md | Marks repo enrichment milestones as completed; adds Phase 4 (agent-browser) roadmap. |
| docs/PROJECT_ISSUE_BACKLOG.md | Adds Phase 4 issues (agent-browser runner, CI E2E, benchmarking, security/rate control). |
| alembic/versions/0008_paper_repos.py | Adds migration for paper_repos table and indexes. |
Comments suppressed due to low confidence (1)
src/paperbot/infrastructure/stores/research_store.py:1304
PaperModelno longer defines anexternal_urlcolumn (seePaperModelinmodels.py), but_resolve_paper_ref_id()still collectsexternal_urlcandidates and later queriesPaperModel.external_url. That will raise an AttributeError at runtime when this code path executes. Remove thePaperModel.external_urlfilter (and treat incomingexternal_urlvalues as additional candidates forPaperModel.url/pdf_urlmatching, or reintroduce anexternal_urlcolumn if it’s required).
url_candidates = []
for key in ("paper_url", "url", "external_url", "pdf_url"):
value = metadata.get(key)
if isinstance(value, str) and value.strip():
url_candidates.append(value.strip())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -107,7 +104,6 @@ def upsert_paper( | |||
| seen_at: Optional[datetime] = None, | |||
| ) -> Dict[str, Any]: | |||
| now = _utcnow() | |||
There was a problem hiding this comment.
seen_at is accepted by upsert_paper() (and is passed by callers like the DailyPaper registry ingest), but it’s no longer used to set any timestamp. As a result, newly created papers always get created_at=now, losing the caller-provided observation time. Either apply seen_at when setting created_at for new rows (and/or preserve the earliest created_at for existing rows), or remove the parameter and update callers accordingly.
| now = _utcnow() | |
| observed_at = _as_utc(seen_at) if seen_at is not None else None | |
| now = observed_at or _utcnow() |
| class SqlAlchemyPaperStore: | ||
| """Canonical paper registry with idempotent upsert for daily workflows.""" | ||
|
|
||
|
|
There was a problem hiding this comment.
This file defines an empty class SqlAlchemyPaperStore and later rebinds SqlAlchemyPaperStore = PaperStore. The empty class is effectively dead code and makes imports/type hints confusing. Consider removing the empty class entirely and keeping only the alias (or turning SqlAlchemyPaperStore into the actual implementation and deprecating PaperStore).
| class SqlAlchemyPaperStore: | |
| """Canonical paper registry with idempotent upsert for daily workflows.""" |
| if not _env_flag("PAPERBOT_REPO_ENRICH_ASYNC", default=True): | ||
| return | ||
| Thread( | ||
| target=_persist_repo_enrichment_async, args=(copy.deepcopy(report),), daemon=True | ||
| ).start() |
There was a problem hiding this comment.
_enqueue_repo_enrichment_async() spawns an unbounded daemon Thread per request by default. Under load this can exhaust threads/DB connections and make repo-enrichment behavior hard to control operationally. Prefer FastAPI BackgroundTasks, a bounded executor/queue, or a single worker/cron-style job; at minimum add concurrency limiting and logging on failures in _persist_repo_enrichment_async() so enrichment issues are observable.
| from typing import Any, Dict, Iterable, List, Optional, Tuple | ||
|
|
||
| from sqlalchemy import Integer, String, cast, func, or_, select | ||
| from sqlalchemy import Integer, String, cast, desc, func, or_, select |
There was a problem hiding this comment.
Import of 'Integer' is not used.
| from sqlalchemy import Integer, String, cast, desc, func, or_, select | |
| from sqlalchemy import String, cast, desc, func, or_, select |
| data = json.loads(self.topics_json or "[]") | ||
| if isinstance(data, list): | ||
| return [str(v) for v in data if str(v).strip()] | ||
| 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: | |
| # If topics_json is invalid or cannot be parsed, treat it as having no topics. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
450-455:⚠️ Potential issue | 🟡 Minor
scalar_one_or_none()on title_hash lookup can raiseMultipleResultsFound.Two papers with the same normalized title will share the same
title_hash. Unlike theupsert_papermethod (line 179-187) which uses.limit(1).scalars().first(), this path usesscalar_one_or_none()which will throw if duplicates exist.🐛 Proposed fix
# Fallback to title hash title_hash = paper.compute_title_hash() - result = session.execute( - select(PaperModel).where(PaperModel.title_hash == title_hash) - ).scalar_one_or_none() + result = ( + session.execute( + select(PaperModel).where(PaperModel.title_hash == title_hash).limit(1) + ) + .scalars() + .first() + ) return resultsrc/paperbot/infrastructure/stores/research_store.py (1)
1333-1346:⚠️ Potential issue | 🟠 Major
scalar_one_or_none()will raiseMultipleResultsFoundif URLs or titles collide.The TODO on Line 1333 already flags this. Since multiple papers can share the same URL (e.g., conference pages) or title, this is a real crash risk on production data. Switching to
.first()as the TODO suggests is a low-effort fix worth doing now.Proposed fix
if url_candidates: row = session.execute( select(PaperModel).where( or_( PaperModel.url.in_(url_candidates), PaperModel.external_url.in_(url_candidates), PaperModel.pdf_url.in_(url_candidates), ) ) - ).scalar_one_or_none() + ).scalars().first() if row is not None: return int(row.id) title = str(metadata.get("title") or "").strip() if title: row = session.execute( select(PaperModel).where(func.lower(PaperModel.title) == title.lower()) - ).scalar_one_or_none() + ).scalars().first() if row is not None: return int(row.id)
🤖 Fix all issues with AI agents
In `@docs/PROJECT_ISSUE_BACKLOG.md`:
- Around line 270-300: The two duplicated headings "Issue 15" and "Issue 16" in
the new entries should be renumbered to avoid collision: change the headings at
the duplicated entries to "Issue 17" and "Issue 18" respectively, and update any
internal references in those blocks (titles like `[Feature] Add agent-browser
source runner with fallback connectors` and `[Feature] Add browser-agent E2E for
workflow streaming UX`) so the issue numbers match the new headings; ensure any
cross-references elsewhere in the file that point to the old numbers are updated
to the new numbers to keep consistency.
In `@docs/ROADMAP_TODO.md`:
- Around line 212-257: The Phase 3.7 section uses subsection numbers "4.1–4.4"
which conflict with the global Phase 4 numbering; rename the headings under
"Phase 3.7 — Agent Browser 自动化能力" from "4.1 Source Capture Agent", "4.2 Workflow
UX Agent", "4.3 Community/Platform Agent", and "4.4 运维与安全" to "3.7.1 Source
Capture Agent", "3.7.2 Workflow UX Agent", "3.7.3 Community/Platform Agent", and
"3.7.4 运维与安全" respectively, and update any internal references/TOC entries and
cross-references that mention those subsection labels so links and mentions
remain correct (search for the exact heading strings "4.1 Source Capture Agent",
etc., to locate all occurrences).
In `@src/paperbot/api/routes/paperscool.py`:
- Around line 770-794: The function _persist_repo_enrichment_async currently
swallows all exceptions; update its outer except block to capture the exception
as e and log the error and stack trace before returning so failures are
discoverable; include contextual info (e.g., report identifier/counts) and
reference the function names involved (_flatten_report_papers,
_collect_repo_enrichment_rows,
SqlAlchemyResearchStore.ingest_repo_enrichment_rows) when logging to make
debugging easier.
In `@src/paperbot/infrastructure/stores/paper_store.py`:
- Around line 574-575: The ILIKE patterns built from user-provided keywords and
venues are vulnerable to unintended wildcard expansion; before creating patterns
in the list comprehensions that build keyword_conditions (using
PaperModel.keywords_json.ilike(...)) and the analogous venue conditions, escape
'%' '_' and '\' in each kw/venue (e.g., replace '\' with '\\', '%' with '\%',
'_' with '\_'), then construct the pattern as f"%{escaped}%" and pass it as a
bind parameter to ilike with an explicit escape character (e.g., ilike(pattern,
escape='\\')) so the database treats those characters literally; update the list
comprehensions that produce keyword_conditions and the venue condition creation
to perform this escaping.
In `@src/paperbot/infrastructure/stores/research_store.py`:
- Around line 595-656: In ingest_repo_enrichment_rows, wrap each row's DB work
in a per-row savepoint so a DB error doesn't abort the entire batch: inside the
loop around the calls that resolve and upsert (the call sites to
_resolve_paper_ref_id and _upsert_paper_repo_row), use session.begin_nested()
(or create a savepoint), run the per-row operations and session.flush() inside
that nested transaction, catch DB exceptions (e.g.,
sqlalchemy.exc.IntegrityError/SQLAlchemyError), roll back the nested transaction
on error, increment skipped (or a new error counter) and continue to the next
row; keep the final session.commit() to persist successful savepoints. Ensure
counters (created/updated/skipped/unresolved) are updated only when the nested
transaction succeeds.
- Around line 1247-1260: The current fallbacks use truthy checks like
github.get("x") or repo_row.get("x") which incorrectly treat valid falsy values
(0, False, "") as missing; update assignments to use explicit None checks or a
small helper so we prefer github values when they are present even if falsy. For
example, replace patterns used in the block that sets row.full_name,
row.description, row.stars, row.forks, row.open_issues, row.watchers,
row.language, row.license, row.archived, row.html_url, row.updated_at_remote and
row.pushed_at_remote to use github.get("key") if github.get("key") is not None
else repo_row.get("key") (and feed the chosen value into _safe_int or
_parse_datetime where applicable) so that 0 and False from GitHub are preserved
instead of falling back to repo_row.
- Around line 1093-1133: The query uses a nonexistent attribute
PaperModel.external_url causing AttributeError; in the
select(PaperModel).where(...) clause that checks url_candidates (the block using
PaperModel.url.in_(url_candidates), PaperModel.external_url.in_(...),
PaperModel.pdf_url.in_(...)), remove the
PaperModel.external_url.in_(url_candidates) clause or instead map external_url
candidates to the real column by replacing that check with
PaperModel.url.in_(external_url_candidates) (i.e., dedupe/merge external_url
values into url_candidates) so the query only references valid model attributes.
🧹 Nitpick comments (3)
src/paperbot/infrastructure/stores/paper_store.py (1)
68-70: EmptySqlAlchemyPaperStoreclass is dead code — redefined at line 885.The class body is empty (just a docstring) and is overwritten by the alias
SqlAlchemyPaperStore = PaperStoreat line 885. This is confusing for readers and flagged by Ruff (F811). Remove the empty class and keep only the alias.♻️ Proposed fix
-class SqlAlchemyPaperStore: - """Canonical paper registry with idempotent upsert for daily workflows.""" - - `@dataclass` class LibraryPaper:src/paperbot/api/routes/paperscool.py (2)
797-802: Bare daemonThreadfor background work is fragile in FastAPI.A daemon thread will be killed mid-transaction on process shutdown (e.g., during a graceful restart). Additionally, each invocation creates a fresh
SqlAlchemyResearchStore()(Line 790), which spins up a new SQLAlchemy engine and connection pool — this doesn't share the app's connection pool and could lead to resource leaks under load.Consider using FastAPI's
BackgroundTasks(which runs after the response is sent but within the request lifecycle) or a proper task queue (ARQ is already in the project). IfThreadis kept for now, consider at least reusing a shared store instance.
805-834: Same store instantiation concern in the request path.Line 823 creates a new
SqlAlchemyResearchStore()per request. If this endpoint is called frequently, you get a new engine each time. Consider injecting a shared store instance (e.g., via FastAPI dependency injection or a module-level singleton) to reuse the connection pool.
| ## Issue 15 - Agent Browser Source Runner | ||
|
|
||
| - Title: `[Feature] Add agent-browser source runner with fallback connectors` | ||
| - Labels: `roadmap`, `phase-4`, `backend`, `integration`, `priority-p1` | ||
| - Source TODO: | ||
| - `新增 Browser Source Runner` | ||
| - `失败后 fallback 到 API connector` | ||
| - `DOM 抽取模板化` | ||
| - Scope: | ||
| - Integrate `vercel-labs/agent-browser` as optional source collector | ||
| - Support structured extraction for HF Papers / arXiv / OpenReview | ||
| - Persist capture traces (steps/screenshots) for debugging | ||
| - Acceptance: | ||
| - Browser collector can produce normalized paper candidates | ||
| - Fallback path is observable and does not block workflow | ||
|
|
||
| ## Issue 16 - Browser-Driven Workflow E2E in CI | ||
|
|
||
| - Title: `[Feature] Add browser-agent E2E for workflow streaming UX` | ||
| - Labels: `roadmap`, `phase-4`, `frontend`, `infra`, `priority-p1` | ||
| - Source TODO: | ||
| - `SSE 增量渲染 E2E` | ||
| - `关键截图和性能指标` | ||
| - `CI artifacts` | ||
| - Scope: | ||
| - Run end-to-end test for Search → DailyPaper → Analyze | ||
| - Assert DAG restore, non-blank loading state, incremental judge/trend render | ||
| - Upload run traces and screenshots in CI | ||
| - Acceptance: | ||
| - Failing UX regressions are detectable in CI | ||
| - Artifacts are attached for triage |
There was a problem hiding this comment.
Duplicate issue numbers: Issues 15 and 16 are defined twice.
The new entries at lines 270 and 286 reuse "Issue 15" and "Issue 16" which are already defined at lines 222 and 236. This will cause confusion when referencing issues. Renumber the new entries starting from Issue 17.
✏️ Suggested renumbering
-## Issue 15 - Agent Browser Source Runner
+## Issue 17 - Agent Browser Source Runner
...
-## Issue 16 - Browser-Driven Workflow E2E in CI
+## Issue 18 - Browser-Driven Workflow E2E in CI
...
-## Issue 17 - Platform Benchmark Monitor Agent
+## Issue 19 - Platform Benchmark Monitor Agent
...
-## Issue 18 - Push Preview Validation Agent
+## Issue 20 - Push Preview Validation Agent
...
-## Issue 19 - Browser Extension Smoke Automation
+## Issue 21 - Browser Extension Smoke Automation
...
-## Issue 20 - Browser Agent Security & Rate Control
+## Issue 22 - Browser Agent Security & Rate Control🤖 Prompt for AI Agents
In `@docs/PROJECT_ISSUE_BACKLOG.md` around lines 270 - 300, The two duplicated
headings "Issue 15" and "Issue 16" in the new entries should be renumbered to
avoid collision: change the headings at the duplicated entries to "Issue 17" and
"Issue 18" respectively, and update any internal references in those blocks
(titles like `[Feature] Add agent-browser source runner with fallback
connectors` and `[Feature] Add browser-agent E2E for workflow streaming UX`) so
the issue numbers match the new headings; ensure any cross-references elsewhere
in the file that point to the old numbers are updated to the new numbers to keep
consistency.
| ## Phase 3.7 — Agent Browser 自动化能力（基于 vercel-labs/agent-browser） | ||
|
|
||
| > 参考仓库：<https://github.com/vercel-labs/agent-browser> | ||
| > 目标：把"网页交互型"任务从静态 API 拉取升级为可观测、可回放的浏览器 Agent 流程。 | ||
| > 定位：**仅用于复杂/高交互采集与回归验证场景**，默认流程保持现有 API connector，不强制改造。 | ||
|
|
||
| ### 4.1 Source Capture Agent（网页采集增强） | ||
|
|
||
| - [ ] 新增 Browser Source Runner：支持登录后抓取（HF Papers、arXiv、OpenReview） | ||
| - 输入：source 配置、cookies/session、抓取策略 | ||
| - 输出：结构化 paper candidates + 抓取轨迹（screenshots + step logs） | ||
| - [ ] 新增反爬/失败回退链路 | ||
| - 失败后自动 fallback 到现有 API connector（papers.cool / arXiv API） | ||
| - 记录 fallback 原因到 run metadata | ||
| - [ ] 新增 DOM 语义抽取模板 | ||
| - 把标题/作者/摘要/链接抽取规则模板化，支持 source 版本升级时快速修复 | ||
|
|
||
| ### 4.2 Workflow UX Agent（前端交互与 E2E 验证） | ||
|
|
||
| - [ ] 为 Search → DailyPaper → Analyze 流程增加 browser-driven E2E 回归 | ||
| - 覆盖 SSE 增量渲染（Judge/Trend/Insight）和 DAG 状态恢复 | ||
| - [ ] 自动录制关键节点截图与性能指标 | ||
| - 首屏可见时间、分析阶段空白时长、首条增量结果时间（TTFR） | ||
| - [ ] 将 E2E 结果接入 CI artifacts | ||
| - 每次 PR 自动上传步骤日志和失败页面快照 | ||
|
|
||
| ### 4.3 Community/Platform Agent（平台对标能力） | ||
|
|
||
| - [ ] 新增 HF/AlphaXiv 对标监测 Agent | ||
| - 周期性抓取公开页面能力矩阵（发现/排序/交互/推送） | ||
| - 生成差距报告写入 `docs/benchmark/`（markdown） | ||
| - [ ] 新增 Daily push 预览 Agent | ||
| - 自动打开邮件/Slack/钉钉渲染预览页面并截图 | ||
| - 验证 BestBlogs 风格模板在多端一致性 | ||
| - [ ] 新增 Browser Extension smoke test | ||
| - 校验 arXiv 页面注入按钮、详情弹层、跳转链路 | ||
|
|
||
| ### 4.4 运维与安全 | ||
|
|
||
| - [ ] 新增 Browser session 密钥管理 | ||
| - cookies/token 通过环境变量或密钥服务注入，禁止明文入库 | ||
| - [ ] 新增 Agent 审计日志 | ||
| - 记录访问域名、操作步骤、耗时、失败原因（可用于问题追踪） | ||
| - [ ] 新增速率限制与并发隔离策略 | ||
| - 避免批量采集触发封禁，支持 source 级并发控制 | ||
|
|
There was a problem hiding this comment.
Section numbering conflict: Phase 3.7 subsections clash with Phase 4.
Phase 3.7 uses subsection numbers 4.1–4.4, which collide with Phase 4's own 4.1–4.5 (Lines 391–491). Consider renaming these to 3.7.1–3.7.4 to avoid ambiguity.
🤖 Prompt for AI Agents
In `@docs/ROADMAP_TODO.md` around lines 212 - 257, The Phase 3.7 section uses
subsection numbers "4.1–4.4" which conflict with the global Phase 4 numbering;
rename the headings under "Phase 3.7 — Agent Browser 自动化能力" from "4.1 Source
Capture Agent", "4.2 Workflow UX Agent", "4.3 Community/Platform Agent", and
"4.4 运维与安全" to "3.7.1 Source Capture Agent", "3.7.2 Workflow UX Agent", "3.7.3
Community/Platform Agent", and "3.7.4 运维与安全" respectively, and update any
internal references/TOC entries and cross-references that mention those
subsection labels so links and mentions remain correct (search for the exact
heading strings "4.1 Source Capture Agent", etc., to locate all occurrences).
| def _persist_repo_enrichment_async(report: Dict[str, Any]) -> None: | ||
| try: | ||
| max_items_raw = os.getenv("PAPERBOT_REPO_ENRICH_MAX_ITEMS", "100") | ||
| max_items = max(1, int(max_items_raw)) | ||
| except Exception: | ||
| max_items = 100 | ||
|
|
||
| include_github_api = _env_flag("PAPERBOT_REPO_ENRICH_INCLUDE_GITHUB_API", default=True) | ||
|
|
||
| try: | ||
| papers = _flatten_report_papers(report) | ||
| if not papers: | ||
| return | ||
| _, repos = _collect_repo_enrichment_rows( | ||
| papers=papers, | ||
| max_items=max_items, | ||
| include_github_api=include_github_api, | ||
| ) | ||
| if not repos: | ||
| return | ||
| store = SqlAlchemyResearchStore() | ||
| store.ingest_repo_enrichment_rows(rows=repos, source="paperscool_daily_async") | ||
| except Exception: | ||
| # Async best-effort hook: ignore failures to avoid affecting daily report flow. | ||
| return |
There was a problem hiding this comment.
Silent exception swallowing hides bugs — add logging.
The bare except Exception (Line 792) silently discards all errors, including programming mistakes or DB connection issues. Since this is a background "best-effort" path, at minimum log the exception so failures are diagnosable in production.
Proposed fix
+import logging
+
+_logger = logging.getLogger(__name__)
+
def _persist_repo_enrichment_async(report: Dict[str, Any]) -> None:
...
try:
...
store.ingest_repo_enrichment_rows(rows=repos, source="paperscool_daily_async")
except Exception:
- # Async best-effort hook: ignore failures to avoid affecting daily report flow.
- return
+ # Async best-effort hook: log but don't propagate to avoid affecting daily report flow.
+ _logger.exception("Background repo enrichment failed")
+ return📝 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 _persist_repo_enrichment_async(report: Dict[str, Any]) -> None: | |
| try: | |
| max_items_raw = os.getenv("PAPERBOT_REPO_ENRICH_MAX_ITEMS", "100") | |
| max_items = max(1, int(max_items_raw)) | |
| except Exception: | |
| max_items = 100 | |
| include_github_api = _env_flag("PAPERBOT_REPO_ENRICH_INCLUDE_GITHUB_API", default=True) | |
| try: | |
| papers = _flatten_report_papers(report) | |
| if not papers: | |
| return | |
| _, repos = _collect_repo_enrichment_rows( | |
| papers=papers, | |
| max_items=max_items, | |
| include_github_api=include_github_api, | |
| ) | |
| if not repos: | |
| return | |
| store = SqlAlchemyResearchStore() | |
| store.ingest_repo_enrichment_rows(rows=repos, source="paperscool_daily_async") | |
| except Exception: | |
| # Async best-effort hook: ignore failures to avoid affecting daily report flow. | |
| return | |
| import logging | |
| _logger = logging.getLogger(__name__) | |
| def _persist_repo_enrichment_async(report: Dict[str, Any]) -> None: | |
| try: | |
| max_items_raw = os.getenv("PAPERBOT_REPO_ENRICH_MAX_ITEMS", "100") | |
| max_items = max(1, int(max_items_raw)) | |
| except Exception: | |
| max_items = 100 | |
| include_github_api = _env_flag("PAPERBOT_REPO_ENRICH_INCLUDE_GITHUB_API", default=True) | |
| try: | |
| papers = _flatten_report_papers(report) | |
| if not papers: | |
| return | |
| _, repos = _collect_repo_enrichment_rows( | |
| papers=papers, | |
| max_items=max_items, | |
| include_github_api=include_github_api, | |
| ) | |
| if not repos: | |
| return | |
| store = SqlAlchemyResearchStore() | |
| store.ingest_repo_enrichment_rows(rows=repos, source="paperscool_daily_async") | |
| except Exception: | |
| # Async best-effort hook: log but don't propagate to avoid affecting daily report flow. | |
| _logger.exception("Background repo enrichment failed") | |
| return |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 774-774: Do not catch blind exception: Exception
(BLE001)
[warning] 792-792: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/paperscool.py` around lines 770 - 794, The function
_persist_repo_enrichment_async currently swallows all exceptions; update its
outer except block to capture the exception as e and log the error and stack
trace before returning so failures are discoverable; include contextual info
(e.g., report identifier/counts) and reference the function names involved
(_flatten_report_papers, _collect_repo_enrichment_rows,
SqlAlchemyResearchStore.ingest_repo_enrichment_rows) when logging to make
debugging easier.
| keyword_conditions = [PaperModel.keywords_json.ilike(f"%{kw}%") for kw in keywords] | ||
| stmt = stmt.where(or_(*keyword_conditions)) |
There was a problem hiding this comment.
LIKE pattern injection via unescaped keyword values.
User-provided kw values are interpolated directly into ILIKE patterns. Characters like % and _ in keywords would act as SQL wildcards, broadening the search unexpectedly. The same applies to venues at line 589.
🛡️ Proposed fix — escape LIKE wildcards
+def _escape_like(value: str) -> str:
+ """Escape SQL LIKE wildcard characters."""
+ return value.replace("%", r"\%").replace("_", r"\_")
+
...
- keyword_conditions = [PaperModel.keywords_json.ilike(f"%{kw}%") for kw in keywords]
+ keyword_conditions = [PaperModel.keywords_json.ilike(f"%{_escape_like(kw)}%") for kw in keywords]🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/paper_store.py` around lines 574 - 575,
The ILIKE patterns built from user-provided keywords and venues are vulnerable
to unintended wildcard expansion; before creating patterns in the list
comprehensions that build keyword_conditions (using
PaperModel.keywords_json.ilike(...)) and the analogous venue conditions, escape
'%' '_' and '\' in each kw/venue (e.g., replace '\' with '\\', '%' with '\%',
'_' with '\_'), then construct the pattern as f"%{escaped}%" and pass it as a
bind parameter to ilike with an explicit escape character (e.g., ilike(pattern,
escape='\\')) so the database treats those characters literally; update the list
comprehensions that produce keyword_conditions and the venue condition creation
to perform this escaping.
| def ingest_repo_enrichment_rows( | ||
| self, | ||
| *, | ||
| rows: List[Dict[str, Any]], | ||
| source: str = "paperscool_repo_enrich", | ||
| ) -> Dict[str, int]: | ||
| now = _utcnow() | ||
| created = 0 | ||
| updated = 0 | ||
| skipped = 0 | ||
| unresolved = 0 | ||
|
|
||
| with self._provider.session() as session: | ||
| for raw in rows or []: | ||
| if not isinstance(raw, dict): | ||
| skipped += 1 | ||
| continue | ||
|
|
||
| github = raw.get("github") if isinstance(raw.get("github"), dict) else {} | ||
| repo_url = str(raw.get("repo_url") or github.get("repo_url") or "").strip() | ||
| if not repo_url: | ||
| skipped += 1 | ||
| continue | ||
|
|
||
| paper_meta = { | ||
| "title": raw.get("title"), | ||
| "paper_url": raw.get("paper_url"), | ||
| "url": raw.get("paper_url"), | ||
| } | ||
| paper_hint = str(raw.get("paper_id") or raw.get("paper_ref_id") or "").strip() | ||
| paper_ref_id = self._resolve_paper_ref_id( | ||
| session=session, | ||
| paper_id=paper_hint, | ||
| metadata=paper_meta, | ||
| ) | ||
| if not paper_ref_id: | ||
| unresolved += 1 | ||
| continue | ||
|
|
||
| was_created = self._upsert_paper_repo_row( | ||
| session=session, | ||
| paper_ref_id=int(paper_ref_id), | ||
| repo_row=raw, | ||
| source=source, | ||
| now=now, | ||
| ) | ||
| if was_created is None: | ||
| skipped += 1 | ||
| elif was_created: | ||
| created += 1 | ||
| else: | ||
| updated += 1 | ||
|
|
||
| session.commit() | ||
|
|
||
| return { | ||
| "total": created + updated, | ||
| "created": created, | ||
| "updated": updated, | ||
| "skipped": skipped, | ||
| "unresolved_paper": unresolved, | ||
| } |
There was a problem hiding this comment.
Batch ingestion commits once at the end — a single bad row can abort all prior work.
If an unexpected DB error (e.g., a unique constraint race or a data-type violation) occurs partway through the loop, SQLAlchemy will invalidate the session and the final session.commit() on Line 648 will fail, losing all preceding upserts. Consider wrapping individual row processing in a savepoint (session.begin_nested()) or using flush() per-row with error recovery, so partial progress is preserved.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 595 - 656,
In ingest_repo_enrichment_rows, wrap each row's DB work in a per-row savepoint
so a DB error doesn't abort the entire batch: inside the loop around the calls
that resolve and upsert (the call sites to _resolve_paper_ref_id and
_upsert_paper_repo_row), use session.begin_nested() (or create a savepoint), run
the per-row operations and session.flush() inside that nested transaction, catch
DB exceptions (e.g., sqlalchemy.exc.IntegrityError/SQLAlchemyError), roll back
the nested transaction on error, increment skipped (or a new error counter) and
continue to the next row; keep the final session.commit() to persist successful
savepoints. Ensure counters (created/updated/skipped/unresolved) are updated
only when the nested transaction succeeds.
| @staticmethod | ||
| def _paper_to_dict(p: PaperModel) -> Dict[str, Any]: | ||
| metadata_raw = getattr(p, "metadata_json", "{}") or "{}" | ||
| try: | ||
| metadata = json.loads(p.metadata_json or "{}") | ||
| metadata = json.loads(metadata_raw) | ||
| if not isinstance(metadata, dict): | ||
| metadata = {} | ||
| except Exception: | ||
| metadata = {} | ||
|
|
||
| published_at = None | ||
| if getattr(p, "publication_date", None): | ||
| published_at = str(getattr(p, "publication_date")) | ||
|
|
||
| source = getattr(p, "primary_source", None) or getattr(p, "source", "") | ||
|
|
||
| return { | ||
| "id": int(p.id), | ||
| "arxiv_id": p.arxiv_id, | ||
| "doi": p.doi, | ||
| "semantic_scholar_id": getattr(p, "semantic_scholar_id", None), | ||
| "openalex_id": getattr(p, "openalex_id", None), | ||
| "title": p.title, | ||
| "authors": p.get_authors(), | ||
| "abstract": p.abstract, | ||
| "url": p.url, | ||
| "external_url": p.external_url, | ||
| "external_url": p.url, | ||
| "pdf_url": p.pdf_url, | ||
| "source": p.source, | ||
| "source": source, | ||
| "primary_source": source, | ||
| "venue": p.venue, | ||
| "published_at": p.published_at.isoformat() if p.published_at else None, | ||
| "first_seen_at": p.first_seen_at.isoformat() if p.first_seen_at else None, | ||
| "year": getattr(p, "year", None), | ||
| "publication_date": getattr(p, "publication_date", None), | ||
| "published_at": published_at, | ||
| "first_seen_at": p.created_at.isoformat() if p.created_at else None, | ||
| "keywords": p.get_keywords(), | ||
| "fields_of_study": p.get_fields_of_study(), | ||
| "sources": p.get_sources(), | ||
| "citation_count": int(getattr(p, "citation_count", 0) or 0), | ||
| "metadata": metadata, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PaperModel definition
fd -t f -e py -g '*/models.py' | head -20Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for PaperModel class definition across the codebase
rg 'class PaperModel' -t py -A 50
# Also search for external_url references
rg 'external_url' -t py -C 3Repository: jerry609/PaperBot
Length of output: 30554
🏁 Script executed:
#!/bin/bash
# Find the exact line with PaperModel.external_url query
rg 'PaperModel\.external_url' -n -t py
# Also check the PaperModel definition more completely
cat -n src/paperbot/infrastructure/stores/models.py | head -100Repository: jerry609/PaperBot
Length of output: 4805
🏁 Script executed:
#!/bin/bash
# Read around line 1340 in research_store.py
sed -n '1330,1350p' src/paperbot/infrastructure/stores/research_store.pyRepository: jerry609/PaperBot
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Check if there are tests that exercise this code path (the query with external_url)
rg -n 'external_url' tests/ --type=py | grep -i 'research\|store'
# Also verify if PaperModel has any dynamic property or __getattr__ that might handle external_url
sed -n '1,200p' src/paperbot/infrastructure/stores/models.py | grep -A 50 'class PaperModel'Repository: jerry609/PaperBot
Length of output: 272
Line 1119 mapping is intentional; fix the query bug at line 1340.
PaperModel has no external_url column—only url and pdf_url. The dict key "external_url": p.url at line 1119 is a deliberate consolidation for API consumers.
However, line 1340 contains a critical bug: the query references the nonexistent PaperModel.external_url attribute:
Query at line 1340 (buggy)
row = session.execute(
select(PaperModel).where(
or_(
PaperModel.url.in_(url_candidates),
PaperModel.external_url.in_(url_candidates), # ← AttributeError at runtime
PaperModel.pdf_url.in_(url_candidates),
)
)
).scalar_one_or_none()Remove the PaperModel.external_url.in_(url_candidates) clause or map external_url input values to the url column.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 1100-1100: Do not catch blind exception: Exception
(BLE001)
[warning] 1105-1105: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 1093 -
1133, The query uses a nonexistent attribute PaperModel.external_url causing
AttributeError; in the select(PaperModel).where(...) clause that checks
url_candidates (the block using PaperModel.url.in_(url_candidates),
PaperModel.external_url.in_(...), PaperModel.pdf_url.in_(...)), remove the
PaperModel.external_url.in_(url_candidates) clause or instead map external_url
candidates to the real column by replacing that check with
PaperModel.url.in_(external_url_candidates) (i.e., dedupe/merge external_url
values into url_candidates) so the query only references valid model attributes.
| row.full_name = str(github.get("full_name") or repo_row.get("full_name") or "").strip() | ||
| row.description = str(github.get("description") or repo_row.get("description") or "") | ||
| row.stars = _safe_int(github.get("stars") or repo_row.get("stars"), 0) | ||
| row.forks = _safe_int(github.get("forks") or repo_row.get("forks"), 0) | ||
| row.open_issues = _safe_int(github.get("open_issues") or repo_row.get("open_issues"), 0) | ||
| row.watchers = _safe_int(github.get("watchers") or repo_row.get("watchers"), 0) | ||
| row.language = str(github.get("language") or repo_row.get("language") or "").strip() | ||
| row.license = str(github.get("license") or repo_row.get("license") or "").strip() | ||
| row.archived = bool(github.get("archived") or repo_row.get("archived")) | ||
| row.html_url = str(github.get("html_url") or repo_row.get("html_url") or repo_url).strip() | ||
| row.updated_at_remote = _parse_datetime( | ||
| github.get("updated_at") or repo_row.get("updated_at") | ||
| ) | ||
| row.pushed_at_remote = _parse_datetime(github.get("pushed_at") or repo_row.get("pushed_at")) |
There was a problem hiding this comment.
or-based fallback is incorrect for falsy-but-valid values (0, False).
The pattern github.get("x") or repo_row.get("x") treats 0, False, and "" as missing, falling through to the repo_row fallback. This is a real problem for:
archived(Line 1255): ifgithub["archived"]is explicitlyFalse, it falls back torepo_row["archived"], which could beTrue(stale data).- Integer fields (Lines 1249–1252): a repo with
0stars from GitHub would fall back to a stale non-zero value inrepo_row.
Use github.get("x") if github.get("x") is not None else repo_row.get("x") or a small helper.
Proposed fix (helper + usage)
+def _first_not_none(*values: Any, default: Any = None) -> Any:
+ for v in values:
+ if v is not None:
+ return v
+ return default
+
...
- row.stars = _safe_int(github.get("stars") or repo_row.get("stars"), 0)
- row.forks = _safe_int(github.get("forks") or repo_row.get("forks"), 0)
- row.open_issues = _safe_int(github.get("open_issues") or repo_row.get("open_issues"), 0)
- row.watchers = _safe_int(github.get("watchers") or repo_row.get("watchers"), 0)
+ row.stars = _safe_int(_first_not_none(github.get("stars"), repo_row.get("stars")), 0)
+ row.forks = _safe_int(_first_not_none(github.get("forks"), repo_row.get("forks")), 0)
+ row.open_issues = _safe_int(_first_not_none(github.get("open_issues"), repo_row.get("open_issues")), 0)
+ row.watchers = _safe_int(_first_not_none(github.get("watchers"), repo_row.get("watchers")), 0)
...
- row.archived = bool(github.get("archived") or repo_row.get("archived"))
+ row.archived = bool(_first_not_none(github.get("archived"), repo_row.get("archived"), default=False))📝 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.
| row.full_name = str(github.get("full_name") or repo_row.get("full_name") or "").strip() | |
| row.description = str(github.get("description") or repo_row.get("description") or "") | |
| row.stars = _safe_int(github.get("stars") or repo_row.get("stars"), 0) | |
| row.forks = _safe_int(github.get("forks") or repo_row.get("forks"), 0) | |
| row.open_issues = _safe_int(github.get("open_issues") or repo_row.get("open_issues"), 0) | |
| row.watchers = _safe_int(github.get("watchers") or repo_row.get("watchers"), 0) | |
| row.language = str(github.get("language") or repo_row.get("language") or "").strip() | |
| row.license = str(github.get("license") or repo_row.get("license") or "").strip() | |
| row.archived = bool(github.get("archived") or repo_row.get("archived")) | |
| row.html_url = str(github.get("html_url") or repo_row.get("html_url") or repo_url).strip() | |
| row.updated_at_remote = _parse_datetime( | |
| github.get("updated_at") or repo_row.get("updated_at") | |
| ) | |
| row.pushed_at_remote = _parse_datetime(github.get("pushed_at") or repo_row.get("pushed_at")) | |
| row.full_name = str(github.get("full_name") or repo_row.get("full_name") or "").strip() | |
| row.description = str(github.get("description") or repo_row.get("description") or "") | |
| row.stars = _safe_int(_first_not_none(github.get("stars"), repo_row.get("stars")), 0) | |
| row.forks = _safe_int(_first_not_none(github.get("forks"), repo_row.get("forks")), 0) | |
| row.open_issues = _safe_int(_first_not_none(github.get("open_issues"), repo_row.get("open_issues")), 0) | |
| row.watchers = _safe_int(_first_not_none(github.get("watchers"), repo_row.get("watchers")), 0) | |
| row.language = str(github.get("language") or repo_row.get("language") or "").strip() | |
| row.license = str(github.get("license") or repo_row.get("license") or "").strip() | |
| row.archived = bool(_first_not_none(github.get("archived"), repo_row.get("archived"), default=False)) | |
| row.html_url = str(github.get("html_url") or repo_row.get("html_url") or repo_url).strip() | |
| row.updated_at_remote = _parse_datetime( | |
| github.get("updated_at") or repo_row.get("updated_at") | |
| ) | |
| row.pushed_at_remote = _parse_datetime(github.get("pushed_at") or repo_row.get("pushed_at")) |
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 1247 -
1260, The current fallbacks use truthy checks like github.get("x") or
repo_row.get("x") which incorrectly treat valid falsy values (0, False, "") as
missing; update assignments to use explicit None checks or a small helper so we
prefer github values when they are present even if falsy. For example, replace
patterns used in the block that sets row.full_name, row.description, row.stars,
row.forks, row.open_issues, row.watchers, row.language, row.license,
row.archived, row.html_url, row.updated_at_remote and row.pushed_at_remote to
use github.get("key") if github.get("key") is not None else repo_row.get("key")
(and feed the chosen value into _safe_int or _parse_datetime where applicable)
so that 0 and False from GitHub are preserved instead of falling back to
repo_row.
![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 significant feature for enriching papers with associated repository data, persisting it to a new paper_repos table, and exposing it via new API endpoints. It also refactors data stores to align with a new harvest baseline and implements asynchronous repository enrichment. However, a high-severity Denial of Service (DoS) vulnerability was identified due to sequential synchronous API calls in a request handler, alongside a medium-severity Broken Access Control issue on a new data persistence endpoint. Additionally, consider improving logging for better observability of the new asynchronous task. Addressing these security concerns and the logging suggestion is crucial for the application's stability and security.
| if req.persist: | ||
| store = SqlAlchemyResearchStore() | ||
| persist_summary = store.ingest_repo_enrichment_rows( | ||
| rows=repos, source="paperscool_repos_api" | ||
| ) |
There was a problem hiding this comment.
The enrich_papers_with_repo_data endpoint allows any user to persist repository enrichment data to the database by setting the persist flag to true. There is no authentication or authorization check to ensure that the requester has the necessary permissions to modify the research store. This could be exploited to inject arbitrary or malicious data into the paper_repos table. Ensure that sensitive state-changing operations are protected by proper authentication and authorization mechanisms.
| except Exception: | ||
| # Async best-effort hook: ignore failures to avoid affecting daily report flow. | ||
| return |
There was a problem hiding this comment.
While this is a best-effort asynchronous task, silently swallowing all exceptions can make debugging difficult if the enrichment process consistently fails. It would be beneficial to log the exception to aid in monitoring and troubleshooting.
You'll need to add from paperbot.utils.logging_config import Logger to the file's imports.
| except Exception: | |
| # Async best-effort hook: ignore failures to avoid affecting daily report flow. | |
| return | |
| except Exception as exc: | |
| # Async best-effort hook: ignore failures to avoid affecting daily report flow. | |
| Logger.warning(f"Async repo enrichment failed: {exc}") | |
| return |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/ROADMAP_TODO.md`:
- Line 625: Update the progress log entry string "2026-02-11: 新增 Phase 4（Agent
Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流" to read "2026-02-11: 新增 Phase 3.7（Agent
Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流" so it matches the Phase 3.7 content
described elsewhere (see the Agent Browser 自动化 section referenced around lines
211-257); ensure only the phase number is changed and the rest of the entry text
remains identical.
🧹 Nitpick comments (1)
docs/ROADMAP_TODO.md (1)
300-301: Consider relaxing the strict 1 issue = 1 commit rule.The engineering rule "每个 issue 对应 1 个 commit（禁止跨 issue 混提）" may be overly rigid. In practice, complex issues often benefit from incremental commits for reviewability, while trivial fixes can be efficiently bundled. Consider allowing flexibility: "每个 commit 应关联对应的 issue，避免无关改动混在同一 commit" (each commit should reference its issues, avoid mixing unrelated changes).
| - 2025-02-10: 创建 ROADMAP_TODO.md，完成对标分析与功能规划 | ||
| - 2025-02-10: 新增多智能体系统现状盘点（5 套管线 + 15 个 Agent）与 OpenClaw 迁移评估 | ||
| - 2026-02-11: 对齐远端 `origin/master` 的 Harvest 基线，保留旧实现到 `backup/feat-dailypaper-sse-stream-pre-harvest-20260211` | ||
| - 2026-02-11: 新增 Phase 4（Agent Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流 |
There was a problem hiding this comment.
Fix typo: Phase 4 should be Phase 3.7.
The progress log entry states "新增 Phase 4（Agent Browser 自动化）", but the content actually refers to Phase 3.7 — Agent Browser 自动化能力 (lines 211-257). Update for accuracy.
✏️ Suggested fix
-- 2026-02-11: 新增 Phase 4（Agent Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流
+- 2026-02-11: 新增 Phase 3.7（Agent Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流🤖 Prompt for AI Agents
In `@docs/ROADMAP_TODO.md` at line 625, Update the progress log entry string
"2026-02-11: 新增 Phase 4（Agent Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流" to read
"2026-02-11: 新增 Phase 3.7（Agent Browser 自动化）任务清单，覆盖采集、E2E、对标监测、安全与限流" so it
matches the Phase 3.7 content described elsewhere (see the Agent Browser 自动化
section referenced around lines 211-257); ensure only the phase number is
changed and the rest of the entry text remains identical.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperbot/api/routes/gen_code.py (1)
81-81:⚠️ Potential issue | 🟠 MajorTemporary directory is never cleaned up.
When
request.output_dirisNone,tempfile.mkdtemp()creates a directory that persists indefinitely. Over time, these accumulate on disk. Consider usingtempfile.TemporaryDirectory()with a context manager, or documenting the expectation that callers are responsible for cleanup.
🤖 Fix all issues with AI agents
In `@docs/agent_inventory.md`:
- Around line 54-59: Update the "5) Known Gaps" section in
docs/agent_inventory.md to reflect that the gap "No single `AgentRuntime`
interface yet" is addressed by the new AgentRuntime implementation; specifically
replace or annotate that bullet to indicate `AgentRuntime` (class name
AgentRuntime) has been introduced (e.g., "Resolved — see AgentRuntime") and
optionally add a short pointer that the implementation now exists in the
codebase (reference the symbol AgentRuntime) so readers know the gap is closed.
- Line 24: The table cell containing the route string `POST
/api/research/scholar/network|trends` has an unescaped pipe which breaks
Markdown table columns; fix it by escaping the pipe (e.g., `POST
/api/research/scholar/network\|trends`) or replacing it with a safe separator
(e.g., `network-trends` or `network/trends`) or the HTML entity `|` so the
row returns to the intended number of columns; update the cell content where
`POST /api/research/scholar/network|trends` appears.
In `@src/paperbot/core/abstractions/agent_runtime.py`:
- Around line 66-76: AgentError is a dataclass subclassing Exception but its
dataclass __init__ doesn't call Exception.__init__, leaving exc.args empty; add
a __post_init__ to AgentError that calls Exception.__init__(self, self.message)
(or sets self.args = (self.message,)) so standard exception machinery and
logging see the message—update the AgentError class (next to the existing fields
and __str__) to define def __post_init__(self): Exception.__init__(self,
self.message) to wire up args.
🧹 Nitpick comments (5)
src/paperbot/application/ports/__init__.py (1)
12-19:__all__is not sorted (Ruff RUF022).✏️ Suggested fix
__all__ = [ "EventLogPort", "HarvesterPort", + "NullSourceCollector", "SourceCollectRequest", "SourceCollectResult", "SourceCollector", - "SourceCollectResult", - "NullSourceCollector", ]src/paperbot/core/abstractions/__init__.py (1)
5-13: Potential naming collision:AgentMessageexists in two modules.
AgentMessageis defined both here (viaagent_runtime.py) and incore/collaboration/messages.py. They serve different purposes — runtime events vs. inter-agent communication. This could confuse consumers who import from both modules. Consider distinguishing them (e.g.,RuntimeMessageorAgentRuntimeMessage) before the API solidifies.src/paperbot/api/routes/gen_code.py (1)
40-54:AgentRunContextcreated but not propagated to the agent.
runtime_contextis constructed here but only used to readrun_id/trace_idfor the initial progress event (lines 52–53). The actualreproduce_from_papercall (line 94) still receives rawrun_id/trace_idstrings. This is presumably a transitional step — onceReproAgentsupports the unified runtime, the context object should be passed directly.tests/unit/test_agent_runtime_contract.py (1)
24-36: Good coverage of the happy path — consider adding error-path tests.The event sequence assertion (
["input", "plan", "result", "finalize"]) and output validation are solid. Consider adding tests for:
- An
AgentErrorraised inexecute→ verifiesok=Falseand error event emission.- A missing method name in
LegacyMethodRuntime→ verifiesAgentErrorwithcode="method_not_found".These would strengthen confidence in the error-handling branches of
BaseAgentRuntime.run.src/paperbot/core/abstractions/agent_runtime.py (1)
114-126:EmitFnis synchronous — async emit callbacks are not supported.
EmitFn = Callable[[AgentMessage], None]prevents async callbacks. If future consumers need to emit events to a database or message bus, they'd need to work around this (e.g., fire-and-forget withasyncio.create_task). This is fine for now, but worth keeping in mind as adoption grows. Consider documenting this design choice or introducing anAsyncEmitFnvariant later.
| | `POST /api/analyze` (`src/paperbot/api/routes/analyze.py`) | Research API | `ResearchAgent.analyze_paper` | title, abstract | SSE progress + paper analysis result | P50 < 15s, P95 < 45s | Emits SSE `error` event | **Must use agent** (unstructured paper reasoning) | | ||
| | `POST /api/review` (`src/paperbot/api/routes/review.py`) | Research API | `ReviewerAgent.review` | title, abstract | SSE progress + review decision payload | P50 < 20s, P95 < 60s | Emits SSE `error` event | **Must use agent** (deep critique + recommendation) | | ||
| | `GET /api/track` (`src/paperbot/api/routes/track.py`) | Scholar Tracking | `PaperTrackerAgent.track_scholar` + `ScholarPipeline.analyze_paper` | scholar id/name, flags | SSE run status + per-paper analysis | P50 < 60s for <=5 papers | Partial result or SSE `error`; closes S2 client in `finally` | **Must use agent** (multi-stage pipeline + external APIs) | | ||
| | `POST /api/research/scholar/network|trends` identity resolve (`src/paperbot/api/routes/research.py`) | Research API | `ScholarProfileAgent` (lookup only) | scholar_id or scholar_name | resolved scholar id then service stats | P50 < 2s lookup + downstream client time | HTTP 4xx/5xx | **Service preferred** (simple profile lookup; agent optional) | |
There was a problem hiding this comment.
Unescaped pipe in table cell breaks column alignment.
The | in network|trends is parsed as a column delimiter, producing 9 columns instead of 8. Escape it or use a different separator.
✏️ Suggested fix
-| `POST /api/research/scholar/network|trends` identity resolve (`src/paperbot/api/routes/research.py`) | Research API | `ScholarProfileAgent` (lookup only) | scholar_id or scholar_name | resolved scholar id then service stats | P50 < 2s lookup + downstream client time | HTTP 4xx/5xx | **Service preferred** (simple profile lookup; agent optional) |
+| `POST /api/research/scholar/network\|trends` identity resolve (`src/paperbot/api/routes/research.py`) | Research API | `ScholarProfileAgent` (lookup only) | scholar_id or scholar_name | resolved scholar id then service stats | P50 < 2s lookup + downstream client time | HTTP 4xx/5xx | **Service preferred** (simple profile lookup; agent optional) |📝 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.
| | `POST /api/research/scholar/network|trends` identity resolve (`src/paperbot/api/routes/research.py`) | Research API | `ScholarProfileAgent` (lookup only) | scholar_id or scholar_name | resolved scholar id then service stats | P50 < 2s lookup + downstream client time | HTTP 4xx/5xx | **Service preferred** (simple profile lookup; agent optional) | | |
| | `POST /api/research/scholar/network\|trends` identity resolve (`src/paperbot/api/routes/research.py`) | Research API | `ScholarProfileAgent` (lookup only) | scholar_id or scholar_name | resolved scholar id then service stats | P50 < 2s lookup + downstream client time | HTTP 4xx/5xx | **Service preferred** (simple profile lookup; agent optional) | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 24-24: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 24-24: Table column count
Expected: 8; Actual: 9; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In `@docs/agent_inventory.md` at line 24, The table cell containing the route
string `POST /api/research/scholar/network|trends` has an unescaped pipe which
breaks Markdown table columns; fix it by escaping the pipe (e.g., `POST
/api/research/scholar/network\|trends`) or replacing it with a safe separator
(e.g., `network-trends` or `network/trends`) or the HTML entity `|` so the
row returns to the intended number of columns; update the cell content where
`POST /api/research/scholar/network|trends` appears.
| ## 5) Known Gaps (for Issue #45-#47) | ||
|
|
||
| - Lifecycle contracts are fragmented (route-level SSE events differ by feature) | ||
| - Some agent paths emit `trace_id`, others do not expose consistent envelopes | ||
| - Agent invocation style differs between `agents/*` and `repro/agents/*` | ||
| - No single `AgentRuntime` interface yet for plan/execute/finalize semantics |
There was a problem hiding this comment.
Known gap #4 is addressed by this PR — consider updating.
Line 59 states "No single AgentRuntime interface yet" but this PR introduces AgentRuntime in src/paperbot/core/abstractions/agent_runtime.py. Consider marking this gap as resolved or adding a note that it's now addressed.
🤖 Prompt for AI Agents
In `@docs/agent_inventory.md` around lines 54 - 59, Update the "5) Known Gaps"
section in docs/agent_inventory.md to reflect that the gap "No single
`AgentRuntime` interface yet" is addressed by the new AgentRuntime
implementation; specifically replace or annotate that bullet to indicate
`AgentRuntime` (class name AgentRuntime) has been introduced (e.g., "Resolved —
see AgentRuntime") and optionally add a short pointer that the implementation
now exists in the codebase (reference the symbol AgentRuntime) so readers know
the gap is closed.
| @dataclass | ||
| class AgentError(Exception): | ||
| """Structured runtime error.""" | ||
|
|
||
| message: str | ||
| code: str = "agent_runtime_error" | ||
| retryable: bool = False | ||
| details: Dict[str, Any] = field(default_factory=dict) | ||
|
|
||
| def __str__(self) -> str: | ||
| return self.message |
There was a problem hiding this comment.
AgentError(Exception) as a @dataclass doesn't initialize Exception.args.
The dataclass-generated __init__ bypasses Exception.__init__, leaving self.args as an empty tuple. This can surprise logging frameworks and traceback utilities that inspect exc.args. Add a __post_init__ to wire it up:
✏️ Suggested fix
`@dataclass`
class AgentError(Exception):
"""Structured runtime error."""
message: str
code: str = "agent_runtime_error"
retryable: bool = False
details: Dict[str, Any] = field(default_factory=dict)
+ def __post_init__(self) -> None:
+ super().__init__(self.message)
+
def __str__(self) -> str:
return self.message🤖 Prompt for AI Agents
In `@src/paperbot/core/abstractions/agent_runtime.py` around lines 66 - 76,
AgentError is a dataclass subclassing Exception but its dataclass __init__
doesn't call Exception.__init__, leaving exc.args empty; add a __post_init__ to
AgentError that calls Exception.__init__(self, self.message) (or sets self.args
= (self.message,)) so standard exception machinery and logging see the
message—update the AgentError class (next to the existing fields and __str__) to
define def __post_init__(self): Exception.__init__(self, self.message) to wire
up args.
Summary by CodeRabbit
New Features
Documentation
Tests