Conversation
* feat(push): implement Epic #179 — daily push optimization Multi-channel push infrastructure with structured digest cards: - #187: Integrate HF upvotes into Judge scoring prompt - #180: Add digest_card extraction (highlight/method/finding/tags) with new LLM prompt, email template rendering, markdown output - #181: MinerU Cloud API client for PDF figure extraction - #182: Apprise multi-channel push layer with YAML config - #183-185: Channel formatters (Telegram MarkdownV2, Discord Rich Embed, WeCom markdown+news, Feishu/Lark interactive card) - #186: RSS 2.0 + Atom feed endpoints (/api/feed/daily.xml, .atom) Also creates follow-up issues #190 (Push Channel Settings UI) and #191 (Dashboard Digest Card Enhancement). 56 new tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: integrate MinerU figure extraction into CLI and ARQ worker daily-paper workflow Refs #179 * fix(daily-push): enable MinerU flags in ARQ cron payload (refs #181) * feat(push): wire channel formatters into apprise digest delivery (refs #182 #183 #184 #185) * feat(feed): add keyword RSS, feed cache, and figure enclosure support (refs #186) * feat(mineru): add extraction result cache for daily push figure pipeline (refs #181) * feat(push): add telegram bot command subscription endpoint (refs #191) * fix(push): add retry/backoff for channel delivery failures (refs #191) * feat(hf): add trending modes, cache, and arxiv identity dedupe (refs #193) * feat(push): add idempotency and error-code mapping for channel delivery (refs #191) * test(push): add mock channel e2e and failure-injection coverage (refs #191) * docs(push): add audit script and ops runbook for acceptance closure (refs #192) * docs: update README for AgentSwarm execution transition * docs: archive stale docs and rename AgentSwarm todo * docs: add AgentSwarm design proposal + web Playwright e2e setup - Add docs/proposals/agentswarm-design.md covering multi-agent orchestration architecture, adapter pattern, session management, skills ecosystem, and OpenCode/OpenClaw/oMo integration (refs #197, #214) - Add Playwright e2e test scaffolding for web dashboard (smoke, papers, research specs) - Update web/.gitignore for Playwright artifacts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…LI chat errors (#223) * fix(studio): fix context pack generation, duplicate import UX, and CLI chat errors - 支持内联标题/摘要，以绕过 PaperInputRouter 获取 studio 论文 ID - 使上下文包生成 SSE 流期间的数据库持久化操作不再致命 - 从库中导入重复论文时显示错误消息 - 将左侧文件面板宽度缩小至约 18%（默认宽度）；移除默认模板文件 - 为 Claude CLI 添加 --verbose 标志，以兼容 stream-json 输出 Related to #222 * fix(studio): fix context pack generation, duplicate import UX, and CLI chat errors - 修改Gemini AI review代码问题 * fix(studio): fix context pack generation, duplicate import UX, and CLI chat errors - 修改Gemini AI review代码问题 --------- Co-authored-by: Jerry Zhang <1772030600@qq.com>
* fix(mineru): support v4 async task extraction flow * refactor(mineru): standardize on v4 task API for daily push (#179)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntegrates MinerU v4 async task-based figure extraction with inline-data fallback; adds title/abstract inline paper support for context-pack generation and structured CLI streaming; implements Studio PaperGallery, PaperIcon, per-paper state caching and streaming improvements; expands CLI/ARQ wiring and Playwright/Vitest test infra; adds AgentSwarm design doc and README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Caller)
participant MineruClient as MineruClient
participant MineruAPI as MinerU API
participant ZipHandler as ZIP/Markdown Handler
Client->>MineruClient: extract_figures(pdf_url, model_version, base_url)
MineruClient->>MineruClient: _validate_source_url()
MineruClient->>MineruAPI: POST /extract/task (pdf_url, model_version)
MineruAPI-->>MineruClient: { task_id }
loop Poll until done
MineruClient->>MineruAPI: GET /extract/task/{task_id}
MineruAPI-->>MineruClient: { status, payload, zip_url }
end
alt payload contains inline figures
MineruClient->>MineruClient: parse inline payload → List[Figure]
else zip available
MineruClient->>ZipHandler: _extract_figures_from_zip(zip_url)
ZipHandler->>ZipHandler: _parse_figures_from_markdown()
ZipHandler->>ZipHandler: _build_inline_data_url()
ZipHandler-->>MineruClient: List[Figure]
end
MineruClient-->>Client: List[Figure] (url + inline_data_url)
sequenceDiagram
participant User as Frontend User
participant API as Repro Context API
participant Orchestrator as Orchestrator/Service
participant LLM as LLM/Claude
participant Store as Persistence Store
User->>API: POST /repro-context/generate (title?, abstract?, depth)
API->>Orchestrator: run(raw_paper=RawPaperData?)
Orchestrator->>LLM: request generation
LLM-->>Orchestrator: stream cli_event JSON lines
Orchestrator->>API: SSE events (progress/observations/errors)
Orchestrator->>Store: save context pack / save stage results (guarded)
API-->>User: stream events and final pack id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求主要通过集成 MinerU v4 提升了“每日推送”功能，改进了图片提取和邮件模板渲染。同时，它优化了 Studio 的用户体验，包括新的论文画廊、更清晰的状态指示和更强大的上下文包处理。一个重要的里程碑是引入了 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 significant, high-quality updates primarily focused on enhancing the Daily Push pipeline, refactoring the Studio UI/UX, and optimizing integration with the Claude CLI. Key improvements include MinerU v4 integration, a refactored Studio with a "Gallery-Workspace" mode, and upgraded Claude CLI interaction using --output-format stream-json for richer chat UI. Comprehensive testing has also been added, and efforts were made to enhance robustness and security in various modules. However, the pull request also introduces several critical security vulnerabilities, most notably a Path Traversal vulnerability within the Claude CLI integration. This vulnerability could allow arbitrary directory execution with elevated permissions, potentially leading to Remote Code Execution. Additionally, instances of Broken Access Control (IDOR) and insecure file handling logic were identified, which could result in unauthorized data access or modification. A potential security risk related to default configurations was also noted. To mitigate these critical issues, it is strongly recommended to implement strict directory allowlisting for all subprocess operations and enforce ownership checks on all resource-accessing endpoints.
Comment on lines
372
to
375
| cwd = request.project_dir or os.getcwd() | ||
| if not os.path.isdir(cwd): | ||
| cwd = os.getcwd() | ||
|
|
There was a problem hiding this comment.
The stream_claude_cli function uses request.project_dir directly as the working directory (cwd) for a subprocess without any validation against an allowlist. Since the Claude CLI is executed with the --dangerously-skip-permissions flag (line 75), an attacker can specify any directory on the system (e.g., /, /etc, /home/user/.ssh) and have the AI agent perform arbitrary file operations or execute commands in that context. This leads to Remote Code Execution (RCE) and full system compromise.
Comment on lines
+377
to
+382
| if request.context_pack_id: | ||
| pack_path = await asyncio.to_thread( | ||
| _ensure_context_pack_on_disk, request.context_pack_id, cwd, | ||
| ) | ||
| if pack_path: | ||
| log.info("Wrote context pack to %s", pack_path) |
There was a problem hiding this comment.
The _ensure_context_pack_on_disk function writes a CONTEXT.md file to the project_dir provided in the request. Because project_dir is not validated, an attacker can use path traversal to write this file into arbitrary directories on the system. Combined with the ability to control context pack content, this allows for arbitrary file creation/overwrite in sensitive locations.
|
|
||
| def _runtime_allowlist_mutation_enabled() -> bool: | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" |
There was a problem hiding this comment.
此处的默认值从 "false" 更改为了 "true"，这使得在默认情况下启用了用于在运行时修改目录白名单的 API 端点。
虽然该端点本身包含验证逻辑，但默认启用一个安全敏感的功能可能会增加攻击面。通常建议遵循“默认安全”的原则，即此类功能应为选择性加入（opt-in）。
建议考虑将默认值改回 "false"，除非在目标部署环境中默认启用此功能是经过深思熟虑的设计决策。
Suggested change
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" | |
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" |
There was a problem hiding this comment.
Pull request overview
This PR merges recent dev work into the default branch, focusing on stabilizing the Daily Push pipeline (MinerU v4 async workflow + figure rendering), improving Studio UX/state handling, and adding web test scaffolding (Vitest + Playwright).
Changes:
- Integrate MinerU v4 async task flow (create + poll), add URL validation, and support inline
data:image/...fallback for main figures in DailyPaper/email rendering. - Improve Studio UX: paper gallery view, deterministic paper icons, per-paper cached state across switches, and structured Claude CLI streaming (tool/text/thinking events).
- Add web testing setup: Vitest unit tests for the store, Playwright config + basic E2E smoke specs, plus ignore rules for test artifacts.
Reviewed changes
Copilot reviewed 32 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/vitest.config.ts | Adds Vitest configuration for web unit tests. |
| web/src/lib/store/studio-store.ts | Adds per-paper cached state and streaming text append support for tasks. |
| web/src/lib/store/studio-store.test.ts | Adds unit tests for per-paper cache switching and streaming append behavior. |
| web/src/lib/store/project-context.ts | Removes default virtual files (starts with empty workspace). |
| web/src/hooks/useContextPackGeneration.ts | Allows sending optional inline title/abstract to the context pack generator and hardens error parsing. |
| web/src/components/studio/ReproductionLog.tsx | Consumes structured SSE events (text/tool/thinking), adds “Generate Context Pack” idle CTA, passes context_pack_id to chat backend. |
| web/src/components/studio/PaperIcon.tsx | Adds deterministic SVG identicon + abbreviation helper for papers. |
| web/src/components/studio/PaperGallery.tsx | Adds gallery-based paper selection/search and deletion flow. |
| web/src/components/studio/NewPaperModal.tsx | Adds duplicate-title detection for manual add/import and improves error display. |
| web/src/app/studio/page.tsx | Switches Studio to gallery-first UX and simplifies workspace panels (Files + Log). |
| web/playwright.config.ts | Adds Playwright configuration for web E2E tests. |
| web/package.json | Adds test scripts plus Vitest + Playwright dev dependencies. |
| web/package-lock.json | Locks new Playwright/Vitest dependency graph. |
| web/next.config.ts | Enables HTTP keep-alive agent options. |
| web/e2e/smoke.spec.ts | Adds basic navigation smoke coverage. |
| web/e2e/research.spec.ts | Adds basic Research page E2E checks. |
| web/e2e/papers.spec.ts | Adds Papers page E2E checks. |
| web/.gitignore | Ignores Playwright/Vitest artifact outputs. |
| tests/unit/test_mineru_client.py | Adds MinerU URL validation + markdown/zip reference parsing coverage. |
| tests/unit/test_dailypaper.py | Adds publishable-URL filtering tests and inline main-figure behavior test. |
| tests/unit/test_daily_digest_card.py | Verifies inline main figure is rendered into email HTML. |
| tests/unit/test_arq_daily_papers.py | Verifies MinerU env settings propagate through ARQ cron/job kwargs. |
| src/paperbot/presentation/cli/main.py | Adds CLI flags/env wiring for MinerU base URL/model/wait time and passes them into figure extraction. |
| src/paperbot/infrastructure/queue/arq_worker.py | Adds MinerU env parsing and passes MinerU settings through cron/job. |
| src/paperbot/infrastructure/extractors/mineru_client.py | Reworks MinerU client to v4 async task API, adds polling, zip/markdown parsing, and inline image extraction. |
| src/paperbot/application/workflows/dailypaper.py | Adds publishable URL filter and inline fallback for main figure; passes MinerU client params. |
| src/paperbot/application/services/email_template.py | Adds main-figure block rendering supporting URL or inline data URL. |
| src/paperbot/api/routes/studio_chat.py | Adds structured CLI streaming (stream-json), optional context-pack materialization into workspace, and keepalive heartbeats. |
| src/paperbot/api/routes/runbook.py | Adjusts runtime allowlist mutation toggle default and refines path normalization/denies. |
| src/paperbot/api/routes/repro_context.py | Allows inline title/abstract bypassing router lookup; adds persistence error handling and better error events. |
| env.example | Documents MinerU v4 settings and DailyPaper figure toggles. |
| docs/proposals/agentswarm-design.md | Adds AgentSwarm design proposal document. |
| README.md | Updates Phase 6 / Daily push documentation and swaps email screenshot reference. |
| .gitignore | Expands env file ignore patterns (while keeping .env.example). |
Files not reviewed (1)
- web/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| ); | ||
| const count = await importBtn.count(); | ||
| // At least some action button should be present | ||
| expect(count).toBeGreaterThanOrEqual(0); |
There was a problem hiding this comment.
This assertion is currently a no-op: count is always >= 0, so the test will pass even if no import/export controls exist. If the intent is to ensure an action is present, assert count > 0 (or check for a specific, stable locator).
Suggested change
| expect(count).toBeGreaterThanOrEqual(0); | |
| expect(count).toBeGreaterThan(0); |
|
|
||
| def _runtime_allowlist_mutation_enabled() -> bool: | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" |
There was a problem hiding this comment.
PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION now defaults to "true", which enables runtime allowlist mutation in all environments unless explicitly disabled. This expands the attack surface (an unauthenticated/misconfigured deployment could allow arbitrary directories to be added for file operations). Consider defaulting back to "false" and only enabling this endpoint explicitly in trusted/dev setups (or require auth).
Suggested change
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" | |
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" |
Comment on lines
384
to
390
| process = await asyncio.create_subprocess_exec( | ||
| *cmd, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| cwd=cwd, | ||
| env={**os.environ, "FORCE_COLOR": "0"}, # Disable ANSI colors | ||
| env={**os.environ, "FORCE_COLOR": "0"}, | ||
| ) |
There was a problem hiding this comment.
The Claude CLI subprocess sets stderr=PIPE but stderr is never drained while the process runs (only read after exit on non-zero return code). If claude writes enough to stderr (likely with --verbose), this can deadlock the subprocess. Consider redirecting stderr to stdout, or concurrently reading/storing stderr in a background task while streaming stdout.
|
|
||
| if inline_data_url.startswith("data:image/"): | ||
| image_src = inline_data_url | ||
| elif url.startswith(("http://", "https://")) and ".zip#" not in url and not url.endswith(".zip"): |
There was a problem hiding this comment.
_main_figure_html filters out .zip# and URLs ending with .zip, but it does not exclude presigned zip URLs that include query strings (e.g. result.zip?token=...#/images/...). Those are still zip artifacts and won't render as images. Consider reusing the same publishable-URL check as dailypaper._is_publishable_figure_url (or extend this condition to also reject .zip? / .zip# variants).
Suggested change
| elif url.startswith(("http://", "https://")) and ".zip#" not in url and not url.endswith(".zip"): | |
| elif ( | |
| url.startswith(("http://", "https://")) | |
| and ".zip#" not in url | |
| and ".zip?" not in url | |
| and not url.endswith(".zip") | |
| ): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
web/src/lib/store/project-context.ts (1)
21-25:⚠️ Potential issue | 🟡 MinorAdd tests for project-context store initialization and state handling.
The
useProjectContextstore is used by multiple components (DeepCodeEditor, FilesPanel, ReproductionLog, WorkspacePanel) and initializes with empty files and null activeFile. Per coding guidelines, this store behavior should have unit tests to verify initialization and ensure consumers handle the empty/null state correctly, similar to the pattern instudio-store.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/project-context.ts` around lines 21 - 25, Add unit tests that assert useProjectContext initializes correctly: verify the store's files equals DEFAULT_FILES (empty record) and activeFile is null, and that consumers (DeepCodeEditor, FilesPanel, ReproductionLog, WorkspacePanel) handle these empty/null values by rendering their fallback states; specifically create tests mirroring the pattern in studio-store.test.ts that (1) creates/reads the store via useProjectContext(), (2) asserts files === DEFAULT_FILES and activeFile === null, and (3) mounts or renders the consumer components to assert they show the "ready/empty" UI paths when files is empty and activeFile is null.
🧹 Nitpick comments (7)
web/src/lib/store/project-context.ts (1)
35-40: Add defensive check inupdateFileto prevent creating invalid file objects.If
updateFileis called with anamethat doesn't exist infiles, spreadingundefined({ ...state.files[name], content }) produces{ content }— an object missing the requirednameandlanguageproperties, violating theVirtualFileinterface.🛡️ Proposed defensive fix
updateFile: (name, content) => set((state) => { + if (!state.files[name]) return state return { files: { ...state.files, [name]: { ...state.files[name], content } - } + }, } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/project-context.ts` around lines 35 - 40, The updateFile setter currently spreads state.files[name] even when it may be undefined, producing an object missing required VirtualFile fields; modify updateFile to first check whether state.files[name] exists in the current state.files map and abort (return the unchanged state) or throw a clear error if it does not, rather than creating a new object, so you don’t end up with { content } lacking name and language; reference the updateFile function and state.files/VirtualFile when making this guard.tests/unit/test_daily_digest_card.py (1)
157-181: Add one URL-branch and one ZIP-rejection assertion test.Nice inline-path coverage. Please add companion cases for
main_figure.url(valid HTTP image) and a rejected zip-style URL (.zip?/.zip#) to lock renderer behavior.As per coding guidelines, "If behavior changes, add or update tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_daily_digest_card.py` around lines 157 - 181, Extend the existing test_main_figure_inline_in_email_html scenario by adding two additional assertions/cases: construct a report via build_daily_paper_report and set item["main_figure"]["url"] to a valid HTTP(S) image URL and assert build_digest_html(report) includes that URL and renders the expected caption/text, and then set item["main_figure"]["url"] to a zip-like URL containing ".zip?" or ".zip#" and assert build_digest_html(report) does NOT embed or accept that URL (i.e., renderer rejects it); locate and update tests around build_daily_paper_report, build_digest_html, and the test_main_figure_inline_in_email_html function to add these two branches.web/src/lib/store/studio-store.ts (1)
262-298: Per-paper state caching works correctly, but streaming operations may continue after paper switch.The cache save/restore logic is sound. However, based on
ReproductionLog.tsx(lines 280-395), when a user switches papers mid-stream, thefor awaitloop inhandleSendMessageWithDircontinues running with a capturedtaskId. Since tasks are global, appends still target the correct task, but there's noAbortControllerto cancel the fetch whenselectPaperis called.Consider adding a cleanup mechanism (e.g., store an
AbortControllerper active stream and abort it inselectPaper) to prevent background streams from consuming resources and potentially causing confusing state updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/studio-store.ts` around lines 262 - 298, selectPaper needs to cancel any in-flight streaming fetches before switching papers: add a per-stream AbortController map on the store (e.g., _streamControllers keyed by taskId or selectedPaperId), update handleSendMessageWithDir to create and register an AbortController and pass its signal into its fetch/stream loop, and ensure the streaming logic removes its controller when the stream completes or errors; finally, in selectPaper (before writing _paperCache/selectedPaperId) iterate any controllers related to the leaving paper or activeTaskId and call abort() and remove them so background streams are cancelled when switching papers.web/src/lib/store/studio-store.test.ts (1)
1-1: Remove unnecessary"use client"directive from test file.The
"use client"directive is a Next.js convention for client components. Test files run in Vitest/Node and don't need this directive. It's harmless but misleading.Suggested fix
-"use client" - import { beforeEach, describe, expect, it } from "vitest"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/studio-store.test.ts` at line 1, Remove the unnecessary "use client" directive from the test file by deleting the top-level "use client" string in the studio-store.test file; tests run under Vitest/Node and should not include Next.js client component directives, so simply remove that line to avoid confusion.tests/unit/test_mineru_client.py (1)
20-41: Consider usingpytest.raisesfor cleaner exception testing.The
assert Falsepattern works butpytest.raisesis more idiomatic and won't be stripped bypython -O. This addresses the static analysis warning.♻️ Suggested refactor using pytest.raises
def test_validate_source_url_requires_http(): client = MineruClient(api_key="test-key") - try: - client._validate_source_url("file:///tmp/a.pdf") - assert False, "expected ValueError for non-http source" - except ValueError as exc: - assert "http(s)" in str(exc) + with pytest.raises(ValueError, match=r"http\(s\)"): + client._validate_source_url("file:///tmp/a.pdf") def test_validate_source_url_rejects_github_and_aws(): client = MineruClient(api_key="test-key") for url in ( "https://github.com/a/b/raw/main/paper.pdf", "https://raw.githubusercontent.com/a/b/main/paper.pdf", "https://bucket.s3.amazonaws.com/paper.pdf", ): - try: - client._validate_source_url(url) - assert False, f"expected ValueError for URL: {url}" - except ValueError as exc: - assert "github/aws" in str(exc) + with pytest.raises(ValueError, match="github/aws"): + client._validate_source_url(url)Add
import pytestat the top if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_mineru_client.py` around lines 20 - 41, Replace the manual try/except + assert False pattern in test_validate_source_url_requires_http and test_validate_source_url_rejects_github_and_aws with pytest.raises for cleaner, robust exception assertions: add "import pytest" if missing, use "with pytest.raises(ValueError) as exc:" around the call to MineruClient._validate_source_url(...) and then assert the expected substring appears in str(exc.value) (e.g., "http(s)" and "github/aws") to preserve the existing checks.web/src/components/studio/NewPaperModal.tsx (1)
118-150: Consider clearing error state when switching tabs.The
errorstate is shared between both tabs but only cleared inresetAndClose. If a user triggers a duplicate error in the library tab, then switches to the manual tab, the error message will still be visible. Consider clearing the error when the tab changes.♻️ Suggested improvement
Add an
onValueChangehandler to theTabscomponent:<Tabs defaultValue="library" className="w-full" onValueChange={() => setError(null)}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/NewPaperModal.tsx` around lines 118 - 150, The error state (error / setError) stays set when switching tabs because it’s only cleared in resetAndClose; update the Tabs usage (component named Tabs) to clear the error on tab change by adding an onValueChange handler that calls setError(null) (or otherwise reset the error). Alternatively, handle tab switches in the component’s tab-change callback to call setError(null) so manual/library tab switches clear the shared error state; ensure this integrates with the existing resetAndClose and any other places that set error.src/paperbot/infrastructure/extractors/mineru_client.py (1)
160-186: Consider adding jitter to the polling interval.The fixed polling interval could cause thundering herd issues if many tasks are polled simultaneously. Adding a small random jitter would help distribute load.
♻️ Suggested improvement
+import random + def _poll_until_done( self, client: httpx.Client, *, headers: Dict[str, str], task_id: str, ) -> Dict[str, Any]: deadline = time.time() + self._max_wait_seconds while time.time() <= deadline: body = self._request_json(...) detail = body.get("data") or {} state = str(detail.get("state") or "").strip().lower() if state == "done": return detail if state == "failed": err_msg = str(detail.get("err_msg") or "task failed") raise RuntimeError(err_msg) - time.sleep(self._poll_interval_seconds) + jitter = random.uniform(0, self._poll_interval_seconds * 0.2) + time.sleep(self._poll_interval_seconds + jitter) raise TimeoutError(f"MinerU task timed out after {self._max_wait_seconds:.0f}s")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/extractors/mineru_client.py` around lines 160 - 186, The polling loop in _poll_until_done uses a fixed sleep (time.sleep(self._poll_interval_seconds)) which can cause thundering-herd issues; modify _poll_until_done to add a small random jitter to the sleep duration (e.g., sample jitter = random.uniform(-j, +j) or random.uniform(0, j) and add to self._poll_interval_seconds), clamp to a positive minimum, and use that jittered value for time.sleep; import random at the top and keep behavior bounded relative to _poll_interval_seconds and _max_wait_seconds so timeout logic remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/application/services/email_template.py`:
- Around line 167-168: The URL guard should be case-insensitive and also exclude
query-form zip URLs; convert the URL to lower case (e.g., url_lower =
url.lower()) and change the condition in the branch that sets image_src so it
only allows http/https when url_lower startswith("http://"/"https://") and does
not contain ".zip?" or ".zip#" and does not endwith(".zip"); update the check
around the existing url and image_src variables accordingly to reject .zip,
.zip? and .zip# (case-insensitive).
In `@web/e2e/papers.spec.ts`:
- Around line 19-27: The test "papers page has import/export options" currently
uses importBtn and count but asserts count >= 0 which always passes; update the
test to actually verify the UI: either change the assertion to
expect(count).toBeGreaterThan(0) on the correct selectors (note that "BibTeX"
and "Zotero" are DropdownMenuItem elements, not buttons, so target their menu
item selectors) or implement the UI flow by clicking the dropdown trigger / menu
toggle to open the menu, then locate the "BibTeX" and "Zotero" items and assert
they are visible, and for the "Import" action open the import dialog (click the
import trigger) before asserting the import button exists; adjust the locator
usage (replace button:has-text with the appropriate dropdown/menu item selectors
and the dialog trigger) and replace the ineffective
expect(count).toBeGreaterThanOrEqual(0) with a strict existence/visibility
assertion.
In `@web/src/components/studio/PaperGallery.tsx`:
- Around line 97-118: The fetch call to "/api/runbook/delete" is sending only {
project_dir: paperToDelete.outputDir } but the API requires both project_dir and
path and rejects directory deletion; update the deletion logic around
paperToDelete.outputDir and the fetch("/api/runbook/delete") call to supply a
valid `path` (not a directory) for each file you want removed—either iterate
over each generated file and POST { project_dir, path } per file or call a new
backend endpoint that accepts a directory and performs recursive deletion; also
preserve existing error handling (e.g., setDeleteError) when requests fail.
In `@web/src/components/studio/PaperIcon.tsx`:
- Around line 1-104: Add unit tests covering getAbbreviation and PaperIcon
deterministic output: create tests that call getAbbreviation with empty string,
title containing only stop-words, and Unicode/title with punctuation to assert
expected two-letter outputs (reference function getAbbreviation), and add a
snapshot or string-compare test that renders PaperIcon with a fixed paperId and
title and verifies the SVG output is stable (reference component PaperIcon and
djb2-derived coloring/pattern). Ensure tests import getAbbreviation and
PaperIcon from PaperIcon.tsx, exercise size variations if relevant, and include
assertions for color/pattern or full SVG string equality to guarantee
determinism.
- Around line 29-37: The getAbbreviation function currently strips all non-ASCII
characters; change the cleanup regex to keep Unicode letters and numbers (e.g.
replace(/[^\\p{L}\\p{N}\\s]/gu, "") ) so non‑English letters are preserved, and
when taking the first N characters use a codepoint-aware approach (e.g. use
Array.from(...) or spread [...word] instead of slice on the string) to avoid
breaking multi‑byte characters; update logic in getAbbreviation and ensure
SKIP_WORDS checks still use the same lowercasing (use w.toLowerCase()) so
comparisons work with preserved Unicode letters.
- Around line 45-70: PaperIcon currently uses the incoming size directly which
can be 0, negative, or NaN and will break SVG geometry; before any use of size
(and before computing cellSize) validate and clamp it to a safe positive number
(e.g., if size is not finite or <= 0 fallback to 48, or clamp into a min/max
range) and then use this sanitizedSize for width/height, viewBox and cellSize
calculations so all downstream geometry (cellSize, SVG attributes) is always
valid; update references of size to the sanitized variable within PaperIcon
(keep function name PaperIcon and variables cellSize, bits, cells the same).
---
Outside diff comments:
In `@web/src/lib/store/project-context.ts`:
- Around line 21-25: Add unit tests that assert useProjectContext initializes
correctly: verify the store's files equals DEFAULT_FILES (empty record) and
activeFile is null, and that consumers (DeepCodeEditor, FilesPanel,
ReproductionLog, WorkspacePanel) handle these empty/null values by rendering
their fallback states; specifically create tests mirroring the pattern in
studio-store.test.ts that (1) creates/reads the store via useProjectContext(),
(2) asserts files === DEFAULT_FILES and activeFile === null, and (3) mounts or
renders the consumer components to assert they show the "ready/empty" UI paths
when files is empty and activeFile is null.
---
Nitpick comments:
In `@src/paperbot/infrastructure/extractors/mineru_client.py`:
- Around line 160-186: The polling loop in _poll_until_done uses a fixed sleep
(time.sleep(self._poll_interval_seconds)) which can cause thundering-herd
issues; modify _poll_until_done to add a small random jitter to the sleep
duration (e.g., sample jitter = random.uniform(-j, +j) or random.uniform(0, j)
and add to self._poll_interval_seconds), clamp to a positive minimum, and use
that jittered value for time.sleep; import random at the top and keep behavior
bounded relative to _poll_interval_seconds and _max_wait_seconds so timeout
logic remains correct.
In `@tests/unit/test_daily_digest_card.py`:
- Around line 157-181: Extend the existing test_main_figure_inline_in_email_html
scenario by adding two additional assertions/cases: construct a report via
build_daily_paper_report and set item["main_figure"]["url"] to a valid HTTP(S)
image URL and assert build_digest_html(report) includes that URL and renders the
expected caption/text, and then set item["main_figure"]["url"] to a zip-like URL
containing ".zip?" or ".zip#" and assert build_digest_html(report) does NOT
embed or accept that URL (i.e., renderer rejects it); locate and update tests
around build_daily_paper_report, build_digest_html, and the
test_main_figure_inline_in_email_html function to add these two branches.
In `@tests/unit/test_mineru_client.py`:
- Around line 20-41: Replace the manual try/except + assert False pattern in
test_validate_source_url_requires_http and
test_validate_source_url_rejects_github_and_aws with pytest.raises for cleaner,
robust exception assertions: add "import pytest" if missing, use "with
pytest.raises(ValueError) as exc:" around the call to
MineruClient._validate_source_url(...) and then assert the expected substring
appears in str(exc.value) (e.g., "http(s)" and "github/aws") to preserve the
existing checks.
In `@web/src/components/studio/NewPaperModal.tsx`:
- Around line 118-150: The error state (error / setError) stays set when
switching tabs because it’s only cleared in resetAndClose; update the Tabs usage
(component named Tabs) to clear the error on tab change by adding an
onValueChange handler that calls setError(null) (or otherwise reset the error).
Alternatively, handle tab switches in the component’s tab-change callback to
call setError(null) so manual/library tab switches clear the shared error state;
ensure this integrates with the existing resetAndClose and any other places that
set error.
In `@web/src/lib/store/project-context.ts`:
- Around line 35-40: The updateFile setter currently spreads state.files[name]
even when it may be undefined, producing an object missing required VirtualFile
fields; modify updateFile to first check whether state.files[name] exists in the
current state.files map and abort (return the unchanged state) or throw a clear
error if it does not, rather than creating a new object, so you don’t end up
with { content } lacking name and language; reference the updateFile function
and state.files/VirtualFile when making this guard.
In `@web/src/lib/store/studio-store.test.ts`:
- Line 1: Remove the unnecessary "use client" directive from the test file by
deleting the top-level "use client" string in the studio-store.test file; tests
run under Vitest/Node and should not include Next.js client component
directives, so simply remove that line to avoid confusion.
In `@web/src/lib/store/studio-store.ts`:
- Around line 262-298: selectPaper needs to cancel any in-flight streaming
fetches before switching papers: add a per-stream AbortController map on the
store (e.g., _streamControllers keyed by taskId or selectedPaperId), update
handleSendMessageWithDir to create and register an AbortController and pass its
signal into its fetch/stream loop, and ensure the streaming logic removes its
controller when the stream completes or errors; finally, in selectPaper (before
writing _paperCache/selectedPaperId) iterate any controllers related to the
leaving paper or activeTaskId and call abort() and remove them so background
streams are cancelled when switching papers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c2e8927-8445-4b4f-8fbc-9feb02295205
⛔ Files ignored due to path filters (2)
asset/ui/dailypaperdemo.pngis excluded by!**/*.pngweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.gitignoreREADME.mddocs/proposals/agentswarm-design.mdenv.examplesrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/runbook.pysrc/paperbot/api/routes/studio_chat.pysrc/paperbot/application/services/email_template.pysrc/paperbot/application/workflows/dailypaper.pysrc/paperbot/infrastructure/extractors/mineru_client.pysrc/paperbot/infrastructure/queue/arq_worker.pysrc/paperbot/presentation/cli/main.pytests/unit/test_arq_daily_papers.pytests/unit/test_daily_digest_card.pytests/unit/test_dailypaper.pytests/unit/test_mineru_client.pyweb/.gitignoreweb/e2e/papers.spec.tsweb/e2e/research.spec.tsweb/e2e/smoke.spec.tsweb/next.config.tsweb/package.jsonweb/playwright.config.tsweb/src/app/studio/page.tsxweb/src/components/studio/NewPaperModal.tsxweb/src/components/studio/PaperGallery.tsxweb/src/components/studio/PaperIcon.tsxweb/src/components/studio/ReproductionLog.tsxweb/src/hooks/useContextPackGeneration.tsweb/src/lib/store/project-context.tsweb/src/lib/store/studio-store.test.tsweb/src/lib/store/studio-store.tsweb/vitest.config.ts
Comment on lines
+1
to
+104
| "use client" | ||
|
|
||
| /** | ||
| * Deterministic geometric icon for papers. | ||
| * Hashes the paper ID (djb2) to derive a color and 4×4 grid pattern, | ||
| * producing a blocky identicon-like SVG. Also extracts a 2-letter | ||
| * abbreviation from the title. | ||
| */ | ||
|
|
||
| const PALETTE = [ | ||
| "#e63946", "#457b9d", "#2a9d8f", "#e9c46a", | ||
| "#f4a261", "#264653", "#6a4c93", "#1982c4", | ||
| "#8ac926", "#ff595e", "#6d6875", "#3a86a7", | ||
| ] | ||
|
|
||
| function djb2(str: string): number { | ||
| let hash = 5381 | ||
| for (let i = 0; i < str.length; i++) { | ||
| hash = ((hash << 5) + hash + str.charCodeAt(i)) >>> 0 | ||
| } | ||
| return hash | ||
| } | ||
|
|
||
| const SKIP_WORDS = new Set([ | ||
| "a", "an", "the", "of", "in", "on", "at", "to", "for", | ||
| "and", "or", "is", "by", "with", "from", "as", "via", | ||
| ]) | ||
|
|
||
| function getAbbreviation(title: string): string { | ||
| const words = title | ||
| .replace(/[^a-zA-Z0-9\s]/g, "") | ||
| .split(/\s+/) | ||
| .filter(w => w.length > 0 && !SKIP_WORDS.has(w.toLowerCase())) | ||
| if (words.length === 0) return "??" | ||
| if (words.length === 1) return words[0].slice(0, 2).toUpperCase() | ||
| return (words[0][0] + words[1][0]).toUpperCase() | ||
| } | ||
|
|
||
| interface PaperIconProps { | ||
| paperId: string | ||
| title: string | ||
| size?: number | ||
| } | ||
|
|
||
| export function PaperIcon({ paperId, title, size = 48 }: PaperIconProps) { | ||
| const hash = djb2(paperId) | ||
| const color = PALETTE[hash % PALETTE.length] | ||
| const abbr = getAbbreviation(title) | ||
|
|
||
| // Generate a 4×4 symmetric pattern (mirror left half → right half) | ||
| // Use 8 bits from the hash for 2×4 cells, then mirror horizontally | ||
| const bits = (hash >>> 8) & 0xffff | ||
| const cellSize = size / 4 | ||
|
|
||
| const cells: { x: number; y: number }[] = [] | ||
| for (let row = 0; row < 4; row++) { | ||
| for (let col = 0; col < 2; col++) { | ||
| const bitIndex = row * 2 + col | ||
| if ((bits >> bitIndex) & 1) { | ||
| cells.push({ x: col, y: row }) | ||
| cells.push({ x: 3 - col, y: row }) // mirror | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <svg | ||
| width={size} | ||
| height={size} | ||
| viewBox={`0 0 ${size} ${size}`} | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| role="img" | ||
| aria-label={abbr} | ||
| > | ||
| <rect width={size} height={size} rx={size * 0.15} fill={color} opacity={0.15} /> | ||
| {cells.map((cell, i) => ( | ||
| <rect | ||
| key={i} | ||
| x={cell.x * cellSize + cellSize * 0.1} | ||
| y={cell.y * cellSize + cellSize * 0.1} | ||
| width={cellSize * 0.8} | ||
| height={cellSize * 0.8} | ||
| rx={cellSize * 0.15} | ||
| fill={color} | ||
| /> | ||
| ))} | ||
| <text | ||
| x={size / 2} | ||
| y={size / 2} | ||
| textAnchor="middle" | ||
| dominantBaseline="central" | ||
| fill="white" | ||
| fontSize={size * 0.3} | ||
| fontWeight="bold" | ||
| fontFamily="system-ui, sans-serif" | ||
| style={{ textShadow: "0 1px 2px rgba(0,0,0,0.3)" }} | ||
| > | ||
| {abbr} | ||
| </text> | ||
| </svg> | ||
| ) | ||
| } | ||
|
|
||
| export { getAbbreviation } |
There was a problem hiding this comment.
Add tests for this new behavior and exported helper.
This file adds new deterministic rendering behavior and exports getAbbreviation, but no test updates are included in the provided changes. Please add tests for at least: abbreviation edge cases (empty/stop-word-only/unicode) and deterministic SVG output for a fixed paperId.
As per coding guidelines, "{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/PaperIcon.tsx` around lines 1 - 104, Add unit tests
covering getAbbreviation and PaperIcon deterministic output: create tests that
call getAbbreviation with empty string, title containing only stop-words, and
Unicode/title with punctuation to assert expected two-letter outputs (reference
function getAbbreviation), and add a snapshot or string-compare test that
renders PaperIcon with a fixed paperId and title and verifies the SVG output is
stable (reference component PaperIcon and djb2-derived coloring/pattern). Ensure
tests import getAbbreviation and PaperIcon from PaperIcon.tsx, exercise size
variations if relevant, and include assertions for color/pattern or full SVG
string equality to guarantee determinism.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 216-217: The function _runtime_allowlist_mutation_enabled
currently defaults to true; change its default to "false" so the code reads
os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" to
preserve opt-in semantics, and add a unit test (e.g.,
test_runtime_allowlist_mutation_disabled) that sets
PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION="false" in the environment, calls
_runtime_allowlist_mutation_enabled(), and asserts False; if this behavior was
intentional instead, add documentation noting the breaking change and a test
that verifies enabled behavior when set to "true".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e4aa64d-5a72-42ec-908d-40ee74d0f530
📒 Files selected for processing (1)
src/paperbot/api/routes/runbook.py
Comment on lines
+216
to
+217
| def _runtime_allowlist_mutation_enabled() -> bool: | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" | ||
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" |
There was a problem hiding this comment.
Security posture change: allowlist mutation now enabled by default.
Changing the default from "false" to "true" inverts the security model from opt-in to opt-out. Existing deployments relying on the implicit default will now permit runtime allowlist mutations without explicit configuration.
Consider:
- Keeping the default as
"false"to maintain opt-in security - If intentional, documenting this breaking change and adding a test that verifies the disabled behavior when
PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION=false
Suggested fix (preserve opt-in default)
def _runtime_allowlist_mutation_enabled() -> bool:
- return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true"
+ return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true"📝 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.
Suggested change
| def _runtime_allowlist_mutation_enabled() -> bool: | |
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" | |
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true").lower() == "true" | |
| def _runtime_allowlist_mutation_enabled() -> bool: | |
| return os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/runbook.py` around lines 216 - 217, The function
_runtime_allowlist_mutation_enabled currently defaults to true; change its
default to "false" so the code reads
os.getenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "false").lower() == "true" to
preserve opt-in semantics, and add a unit test (e.g.,
test_runtime_allowlist_mutation_disabled) that sets
PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION="false" in the environment, calls
_runtime_allowlist_mutation_enabled(), and asserts False; if this behavior was
intentional instead, add documentation noting the breaking change and a test
that verifies enabled behavior when set to "true".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 39 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- web/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
50
to
56
| selectPaper, | ||
| loadPapers, | ||
| papers, | ||
| selectedPaperId, | ||
| setContextPack, | ||
| setContextPackLoading, | ||
| setContextPackError, |
There was a problem hiding this comment.
The URL-param processing effect in this component relies on papers being loaded from loadPapers(), but loadPapers() is called in a separate useEffect and its state update won’t be visible to the param effect on the first effect flush. This can cause existingPaper checks to miss stored papers (creating duplicates) and, when paper_id isn’t found, addPaper(...)’s returned id is ignored while generate({ paperId }) still uses the query-string id (which won’t match the newly created paper). Consider gating param processing on a “papers loaded” flag, and/or capturing the id returned by addPaper() and using it for selectPaper/generate (optionally passing title/abstract into generate for the studio-ID case).
Comment on lines
+193
to
+197
| onClick={(e) => | ||
| handleDeleteClick(e, paper) | ||
| } | ||
| className="absolute right-2 top-2 z-10 rounded-md p-1.5 opacity-0 group-hover:opacity-100 hover:bg-destructive/10 hover:text-destructive transition-all" | ||
| title="Delete paper" |
There was a problem hiding this comment.
The delete icon-only <button> relies on title and is visually hidden via opacity-0 until hover. For accessibility, add an explicit aria-label (or screen-reader-only text) and ensure it becomes visible on keyboard focus as well (e.g., group-focus-within:opacity-100 / focus-visible:opacity-100) so keyboard and screen-reader users can discover/operate it.
Suggested change
| onClick={(e) => | |
| handleDeleteClick(e, paper) | |
| } | |
| className="absolute right-2 top-2 z-10 rounded-md p-1.5 opacity-0 group-hover:opacity-100 hover:bg-destructive/10 hover:text-destructive transition-all" | |
| title="Delete paper" | |
| type="button" | |
| onClick={(e) => | |
| handleDeleteClick(e, paper) | |
| } | |
| className="absolute right-2 top-2 z-10 rounded-md p-1.5 opacity-0 group-hover:opacity-100 focus-visible:opacity-100 hover:bg-destructive/10 hover:text-destructive focus-visible:bg-destructive/10 transition-all" | |
| title="Delete paper" | |
| aria-label="Delete paper" |
Comment on lines
460
to
+469
| if enable_figures: | ||
| mineru_key = os.getenv("MINERU_API_KEY", "") | ||
| if mineru_key: | ||
| report = extract_figures_for_report( | ||
| report, | ||
| api_key=mineru_key, | ||
| max_items=max(1, int(figures_max_items)), | ||
| base_url=mineru_api_base_url, | ||
| model_version=mineru_model_version, | ||
| max_wait_seconds=max(5.0, float(mineru_max_wait_seconds)), |
There was a problem hiding this comment.
daily_papers_job is async, but extract_figures_for_report(...) is synchronous and (via MineruClient) performs blocking HTTP calls and polling with time.sleep. This will block the event loop for up to mineru_max_wait_seconds per item, potentially stalling other async work in the worker.
Consider running figure extraction in asyncio.to_thread(...) (quick fix), or converting MinerU extraction to use httpx.AsyncClient + asyncio.sleep so the job remains non-blocking.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/studio/PapersPanel.tsx (1)
119-129:⚠️ Potential issue | 🟠 MajorDo not delete the paper record when generated-file cleanup fails.
At Line 121-Line 124, cleanup errors are swallowed and Line 128 still removes the paper. This can leave orphaned files while the UI implies cleanup succeeded.
Suggested fix
setDeleting(true) try { // Delete generated files if outputDir exists if (paperToDelete.outputDir) { - try { - await deleteProjectFiles(paperToDelete.outputDir) - } catch (e) { - console.error('Failed to delete project files:', e) - } + await deleteProjectFiles(paperToDelete.outputDir) } // Delete paper from store deletePaper(paperToDelete.id) + setDeleteConfirmOpen(false) + setPaperToDelete(null) + } catch (e) { + console.error("Failed to delete paper and project files:", e) } finally { setDeleting(false) - setDeleteConfirmOpen(false) - setPaperToDelete(null) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/PapersPanel.tsx` around lines 119 - 129, The current flow calls deletePaper(paperToDelete.id) regardless of whether deleteProjectFiles(paperToDelete.outputDir) succeeded, causing the record to be removed even if cleanup failed; modify the logic so that deletePaper is only invoked when deleteProjectFiles completes without throwing: move the deletePaper call into the try block after await deleteProjectFiles(...) (or check a success flag), and ensure that exceptions from deleteProjectFiles are either rethrown or cause early return so the paper record is not removed; use the existing identifiers paperToDelete, deleteProjectFiles, and deletePaper to locate and update the code.
♻️ Duplicate comments (1)
web/e2e/papers.spec.ts (1)
19-22:⚠️ Potential issue | 🟠 MajorExport visibility check misses required precondition (selection).
On Line 20-Line 21, this asserts Export is visible before selecting any paper. In
web/src/components/research/SavedPapersList.tsx(Line 254 and Line 794-Line 809), Export renders only whenselectedIds.size > 0, so this test does not model the real UI flow.Proposed test-flow fix
test("papers page has import/export options", async ({ page }) => { - const exportButton = page.getByRole("button", { name: /export/i }); - await expect(exportButton).toBeVisible(); + // Precondition: select at least one paper so Export control is rendered + await page.getByRole("checkbox").first().check(); + + const exportButton = page.getByRole("button", { name: /export/i }); + await expect(exportButton).toBeVisible(); + + await exportButton.click(); + await expect(page.getByRole("menuitem", { name: /bibtex/i })).toBeVisible(); + await expect(page.getByRole("menuitem", { name: /zotero/i })).toBeVisible(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/e2e/papers.spec.ts` around lines 19 - 22, The test "papers page has import/export options" asserts the Export button is visible without selecting any paper, but Export in SavedPapersList.tsx is only rendered when selectedIds.size > 0; update the test to first select at least one paper (e.g., locate and click a paper row or its selection checkbox using page.getByRole/getByTestId) so selectedIds becomes non-empty, then assert the exportButton is visible; keep the test name and use the existing exportButton locator to verify visibility after selection.
🧹 Nitpick comments (6)
web/e2e/papers.spec.ts (1)
9-17: Tighten this assertion to avoid false positives.On Line 10-Line 16, matching generic tokens like
"Paper"/"Library"can pass on unrelated UI states. Prefer asserting a stable heading/landmark specific to the Papers page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/e2e/papers.spec.ts` around lines 9 - 17, The current test "papers page has saved papers section" is too broad by scanning body text for generic tokens; replace that with a targeted assertion for a stable page landmark or heading (e.g., assert page.getByRole('heading', { name: /Papers|Saved Papers|My Library/i }) or a specific main/section aria-label exists and is visible) so the test reliably checks the Papers page UI; update the assertion to locate the heading/landmark (using getByRole or locator('main[aria-label="Papers"]') or similar) and expect it toBeVisible()/toHaveCount(1) instead of checking body text includes.web/src/components/studio/NewPaperModal.tsx (1)
101-102: Extract a sharednormalizeTitlehelper to avoid normalization drift.
trim().toLowerCase()is repeated across manual submit, import, and rendering checks. Centralizing this reduces accidental inconsistency later.♻️ Suggested refactor
+const normalizeTitle = (value: string) => value.trim().toLowerCase() -const normalizedTitle = title.trim().toLowerCase() -const duplicate = papers.find(p => p.title.trim().toLowerCase() === normalizedTitle) +const normalizedTitle = normalizeTitle(title) +const duplicate = papers.find(p => normalizeTitle(p.title) === normalizedTitle) -const existingTitles = new Set(papers.map(p => p.title.trim().toLowerCase())) +const existingTitles = new Set(papers.map(p => normalizeTitle(p.title))) -const normalizedTitle = paper.title.trim().toLowerCase() +const normalizedTitle = normalizeTitle(paper.title) -const studioPaperTitles = new Set(papers.map(p => p.title.trim().toLowerCase())) +const studioPaperTitles = new Set(papers.map(p => normalizeTitle(p.title))) -const isInStudio = studioPaperTitles.has(paper.title.trim().toLowerCase()) +const isInStudio = studioPaperTitles.has(normalizeTitle(paper.title))Also applies to: 118-118, 127-127, 175-175, 227-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/NewPaperModal.tsx` around lines 101 - 102, Extract a single helper function (e.g., normalizeTitle) in NewPaperModal.tsx that returns title.trim().toLowerCase(), and replace all ad-hoc normalizations (the normalizedTitle variable and the papers.find comparison: papers.find(p => p.title.trim().toLowerCase() === normalizedTitle)) as well as the other occurrences flagged (uses around handling submit/import and rendering checks) to call normalizeTitle(title) and normalizeTitle(p.title) so all duplicate checks and render logic use the centralized function.src/paperbot/api/routes/studio_chat.py (2)
133-150: Consider extracting shared path-loading logic and adding debug logging for swallowed exceptions.This function duplicates logic from
runbook.py:_load_runtime_allowed_dirs(see context snippet 4). The runbook version also tries multiple potential paths as fallbacks. Additionally, exceptions caught at lines 148-149 are silently swallowed—adding debug-level logging would aid troubleshooting.♻️ Suggested improvement
def _load_runtime_allowed_dirs() -> List[Path]: - f = Path("data/runbook_allowed_dirs.json") - if not f.exists(): - return [] - try: - data = json.loads(f.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError, ValueError): + potential_paths = [ + Path("data/runbook_allowed_dirs.json"), + Path(__file__).parent.parent.parent / "data" / "runbook_allowed_dirs.json", + ] + + for f in potential_paths: + if not f.exists(): + continue + try: + data = json.loads(f.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError, ValueError): + continue ...Consider extracting this to a shared module (e.g.,
infrastructure/config/allowed_dirs.py) to eliminate duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/studio_chat.py` around lines 133 - 150, The _load_runtime_allowed_dirs function duplicates logic from runbook.py:_load_runtime_allowed_dirs and silently swallows exceptions; refactor by extracting the shared path-loading logic into a single helper (e.g., infrastructure.config.allowed_dirs) and have both studio_chat._load_runtime_allowed_dirs and runbook._load_runtime_allowed_dirs call that helper, implement the runbook-style fallback over multiple candidate files, and replace bare exception swallowing in the helper with debug-level logging via the existing logger (log the exception message/traceback when Path(...).resolve() or file reads/json loads fail) so failures are visible during troubleshooting.
469-536: Consider adding an overall timeout to prevent indefinite hangs.The keepalive mechanism handles temporary stdout silence, but if the Claude CLI process itself hangs indefinitely (e.g., deadlock, network partition), this loop will never terminate. Consider adding a maximum execution time:
♻️ Suggested improvement
+ _MAX_EXECUTION_SECONDS = 600 # 10 minutes max + start_time = asyncio.get_event_loop().time() + # Read stdout line-by-line (NDJSON) with keepalive heartbeats. while True: + elapsed = asyncio.get_event_loop().time() - start_time + if elapsed > _MAX_EXECUTION_SECONDS: + process.kill() + yield StreamEvent(type="error", message="Claude CLI execution timed out") + break + try: chunk = await asyncio.wait_for( process.stdout.read(4096), timeout=_KEEPALIVE_SECONDS, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/studio_chat.py` around lines 469 - 536, Add a global maximum execution timeout around the CLI read/wait loop to avoid indefinite hangs: introduce a deadline (e.g., MAX_EXEC_SECONDS) and check it in the main loop that reads process.stdout (the block using _KEEPALIVE_SECONDS, line_buffer, and asyncio.wait_for), or wrap the entire streaming coroutine in asyncio.wait_for; on timeout, cancel/terminate the subprocess (process.terminate()/process.kill()), await stderr_task and process.wait(), and yield a final StreamEvent (type="error" or "progress" with timeout info and request.mode) before returning so callers see the failure. Ensure cleanup still runs (await stderr_task and process.wait()) and surface stderr_chunks for debugging.web/src/components/studio/PapersPanel.tsx (1)
113-134: Add tests for the new deletion workflow.The confirm-delete behavior changed (new cleanup utility + error handling path), but I don’t see tests for success/failure branches in this change set.
As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/PapersPanel.tsx` around lines 113 - 134, The deletion workflow in handleConfirmDelete now calls deleteProjectFiles and handles errors, but there are no tests covering the success and failure branches; add unit tests that exercise handleConfirmDelete (or the component PapersPanel) to assert: (1) when paperToDelete has an outputDir and deleteProjectFiles resolves, deleteProjectFiles is called with the correct path and deletePaper is invoked with paperToDelete.id and UI state (setDeleting, setDeleteConfirmOpen) updates appropriately; and (2) when deleteProjectFiles rejects, it logs the error (mock console.error), still calls deletePaper, and UI state resets; mock deleteProjectFiles and deletePaper, mount or render PapersPanel, trigger the confirm action, and assert the expected calls and state transitions for both success and error cases.src/paperbot/api/routes/runbook.py (1)
137-173: Please add regression tests for the new path-normalization and allowlist-mutation behavior.This change materially alters security-sensitive path handling (
realpathnormalization, denied-root checks, home-directory rejection). I don’t see corresponding route/unit tests in this change set for these branches.As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.Also applies to: 227-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/runbook.py` around lines 137 - 173, The change to _normalize_user_directory introduces new security-sensitive branches (realpath normalization, denied-root checks, '~' expansion, and allowlist-based path mutation) but lacks tests; add regression/unit tests that call _normalize_user_directory (and exercises behavior of _allowed_workdir_prefixes and _is_under_prefix) to cover: empty input and NUL-char rejection; literal "~" and "~/sub" expansion to Path.home(); relative paths resolved against cwd; absolute paths that are allowed vs denied by the allowlist; a path that equals an allowed prefix; a path under an allowed prefix that requires suffix reconstruction and returns the mutated Path; and cases involving symlinked paths where realpath differs (ensure denied-root and startswith checks behave as expected). Ensure tests assert HTTPException status codes and exact error messages for denied cases and that returned Path objects match expected canonicalized prefixes when allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 162-169: The check that gated child paths uses
"normalized_real.startswith(prefix_real + os.sep)" which fails when prefix_real
is root ("/"); change the conditional that decides children to handle the
root-prefix edge case by allowing any absolute path under "/" — e.g. replace
that condition with a combined check like "if prefix_real == os.sep or
normalized_real == prefix_real or normalized_real.startswith(prefix_real +
os.sep):" so the code still computes suffix and candidate as before and then
validates with _is_under_prefix(prefix, candidate).
In `@tests/unit/test_studio_chat_path_validation.py`:
- Around line 22-28: The test currently creates a persistent directory outside
the test sandbox; instead create the disallowed directory under the provided
tmp_path and then mock the source of allowed prefixes so the path is considered
"outside" allowed prefixes; e.g., create disallowed = tmp_path /
"paperbot-studio-chat-disallowed" and then monkeypatch the mechanism used to
compute allowed prefixes (or monkeypatch Path.home()/Path.home().parent) so that
disallowed is not within allowed prefixes, then call
_resolve_cli_project_dir(str(disallowed)) expecting ValueError; this keeps files
inside tmp_path and avoids permission/artifact issues.
In `@web/src/components/studio/NewPaperModal.tsx`:
- Around line 101-106: The PR added title-based dedup logic in NewPaperModal.tsx
(look for the NewPaperModal component and the normalizedTitle/duplicate check
that calls setError) and additional branches for manual duplicate rejection,
import-skip behavior, and singular vs plural error messages; add regression
tests that render NewPaperModal and simulate: submitting a title that differs
only by case/whitespace to assert the duplicate branch sets the correct error
message, the manual-reject path (simulate the user choosing "reject duplicate")
to assert the modal closes/sets state as expected, the import-skip flow to
assert it returns the expected skip result instead of error, and both
singular/plural message variations (1 vs >1 existing duplicates) to assert
correct wording; place tests alongside other web/src component tests (tsx test
files), use the component's public handlers (submit button, props) to drive the
scenarios, and assert on DOM error text and any emitted callbacks.
In `@web/src/lib/runbook/deleteProjectFiles.ts`:
- Around line 2-13: The file listing handler in deleteProjectFiles.ts currently
ignores the response's truncation flag and may proceed to delete only a subset;
update the code that parses listRes.json() (payload) to check for a boolean
payload.truncated (or equivalent) and, if true, abort and surface an error
instead of continuing; ensure the same function (where listRes, payload, and
files are defined) throws/reports a clear error when truncated is true so
deletions are not performed on a partial file list.
---
Outside diff comments:
In `@web/src/components/studio/PapersPanel.tsx`:
- Around line 119-129: The current flow calls deletePaper(paperToDelete.id)
regardless of whether deleteProjectFiles(paperToDelete.outputDir) succeeded,
causing the record to be removed even if cleanup failed; modify the logic so
that deletePaper is only invoked when deleteProjectFiles completes without
throwing: move the deletePaper call into the try block after await
deleteProjectFiles(...) (or check a success flag), and ensure that exceptions
from deleteProjectFiles are either rethrown or cause early return so the paper
record is not removed; use the existing identifiers paperToDelete,
deleteProjectFiles, and deletePaper to locate and update the code.
---
Duplicate comments:
In `@web/e2e/papers.spec.ts`:
- Around line 19-22: The test "papers page has import/export options" asserts
the Export button is visible without selecting any paper, but Export in
SavedPapersList.tsx is only rendered when selectedIds.size > 0; update the test
to first select at least one paper (e.g., locate and click a paper row or its
selection checkbox using page.getByRole/getByTestId) so selectedIds becomes
non-empty, then assert the exportButton is visible; keep the test name and use
the existing exportButton locator to verify visibility after selection.
---
Nitpick comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 137-173: The change to _normalize_user_directory introduces new
security-sensitive branches (realpath normalization, denied-root checks, '~'
expansion, and allowlist-based path mutation) but lacks tests; add
regression/unit tests that call _normalize_user_directory (and exercises
behavior of _allowed_workdir_prefixes and _is_under_prefix) to cover: empty
input and NUL-char rejection; literal "~" and "~/sub" expansion to Path.home();
relative paths resolved against cwd; absolute paths that are allowed vs denied
by the allowlist; a path that equals an allowed prefix; a path under an allowed
prefix that requires suffix reconstruction and returns the mutated Path; and
cases involving symlinked paths where realpath differs (ensure denied-root and
startswith checks behave as expected). Ensure tests assert HTTPException status
codes and exact error messages for denied cases and that returned Path objects
match expected canonicalized prefixes when allowed.
In `@src/paperbot/api/routes/studio_chat.py`:
- Around line 133-150: The _load_runtime_allowed_dirs function duplicates logic
from runbook.py:_load_runtime_allowed_dirs and silently swallows exceptions;
refactor by extracting the shared path-loading logic into a single helper (e.g.,
infrastructure.config.allowed_dirs) and have both
studio_chat._load_runtime_allowed_dirs and runbook._load_runtime_allowed_dirs
call that helper, implement the runbook-style fallback over multiple candidate
files, and replace bare exception swallowing in the helper with debug-level
logging via the existing logger (log the exception message/traceback when
Path(...).resolve() or file reads/json loads fail) so failures are visible
during troubleshooting.
- Around line 469-536: Add a global maximum execution timeout around the CLI
read/wait loop to avoid indefinite hangs: introduce a deadline (e.g.,
MAX_EXEC_SECONDS) and check it in the main loop that reads process.stdout (the
block using _KEEPALIVE_SECONDS, line_buffer, and asyncio.wait_for), or wrap the
entire streaming coroutine in asyncio.wait_for; on timeout, cancel/terminate the
subprocess (process.terminate()/process.kill()), await stderr_task and
process.wait(), and yield a final StreamEvent (type="error" or "progress" with
timeout info and request.mode) before returning so callers see the failure.
Ensure cleanup still runs (await stderr_task and process.wait()) and surface
stderr_chunks for debugging.
In `@web/e2e/papers.spec.ts`:
- Around line 9-17: The current test "papers page has saved papers section" is
too broad by scanning body text for generic tokens; replace that with a targeted
assertion for a stable page landmark or heading (e.g., assert
page.getByRole('heading', { name: /Papers|Saved Papers|My Library/i }) or a
specific main/section aria-label exists and is visible) so the test reliably
checks the Papers page UI; update the assertion to locate the heading/landmark
(using getByRole or locator('main[aria-label="Papers"]') or similar) and expect
it toBeVisible()/toHaveCount(1) instead of checking body text includes.
In `@web/src/components/studio/NewPaperModal.tsx`:
- Around line 101-102: Extract a single helper function (e.g., normalizeTitle)
in NewPaperModal.tsx that returns title.trim().toLowerCase(), and replace all
ad-hoc normalizations (the normalizedTitle variable and the papers.find
comparison: papers.find(p => p.title.trim().toLowerCase() === normalizedTitle))
as well as the other occurrences flagged (uses around handling submit/import and
rendering checks) to call normalizeTitle(title) and normalizeTitle(p.title) so
all duplicate checks and render logic use the centralized function.
In `@web/src/components/studio/PapersPanel.tsx`:
- Around line 113-134: The deletion workflow in handleConfirmDelete now calls
deleteProjectFiles and handles errors, but there are no tests covering the
success and failure branches; add unit tests that exercise handleConfirmDelete
(or the component PapersPanel) to assert: (1) when paperToDelete has an
outputDir and deleteProjectFiles resolves, deleteProjectFiles is called with the
correct path and deletePaper is invoked with paperToDelete.id and UI state
(setDeleting, setDeleteConfirmOpen) updates appropriately; and (2) when
deleteProjectFiles rejects, it logs the error (mock console.error), still calls
deletePaper, and UI state resets; mock deleteProjectFiles and deletePaper, mount
or render PapersPanel, trigger the confirm action, and assert the expected calls
and state transitions for both success and error cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b5a5e11-2916-4202-8d3e-35b8959097af
📒 Files selected for processing (14)
src/paperbot/api/routes/runbook.pysrc/paperbot/api/routes/studio_chat.pysrc/paperbot/application/services/email_template.pytests/unit/test_daily_digest_card.pytests/unit/test_mineru_client.pytests/unit/test_studio_chat_path_validation.pyweb/e2e/papers.spec.tsweb/src/components/studio/NewPaperModal.tsxweb/src/components/studio/PaperGallery.tsxweb/src/components/studio/PaperIcon.test.tsxweb/src/components/studio/PaperIcon.tsxweb/src/components/studio/PapersPanel.tsxweb/src/lib/runbook/deleteProjectFiles.tsweb/src/lib/store/studio-store.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/components/studio/PaperGallery.tsx
- web/src/lib/store/studio-store.test.ts
- web/src/components/studio/PaperIcon.tsx
Comment on lines
+162
to
169
| prefix_real = os.path.realpath(prefix_str) | ||
| if normalized_real == prefix_real: | ||
| return prefix | ||
| if normalized.startswith(prefix_str + os.sep): | ||
| suffix = normalized[len(prefix_str):].lstrip("/\\") | ||
| if normalized_real.startswith(prefix_real + os.sep): | ||
| suffix = normalized_real[len(prefix_real):].lstrip("/\\") | ||
| candidate = (prefix / suffix).resolve(strict=False) if suffix else prefix | ||
| if _is_under_prefix(candidate, prefix): | ||
| return candidate |
There was a problem hiding this comment.
Root-prefix edge case can reject valid paths.
At Line 165, normalized_real.startswith(prefix_real + os.sep) breaks when prefix_real is / (checks for //). That makes valid children under root fail normalization.
Suggested fix
- if normalized_real.startswith(prefix_real + os.sep):
+ try:
+ under_prefix = os.path.commonpath([normalized_real, prefix_real]) == prefix_real
+ except ValueError:
+ under_prefix = False
+ if under_prefix and normalized_real != prefix_real:
suffix = normalized_real[len(prefix_real):].lstrip("/\\")
candidate = (prefix / suffix).resolve(strict=False) if suffix else prefix
- if _is_under_prefix(candidate, prefix):
- return candidate
- raise HTTPException(status_code=403, detail=f"{field_name} is not allowed")
+ return candidate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/runbook.py` around lines 162 - 169, The check that
gated child paths uses "normalized_real.startswith(prefix_real + os.sep)" which
fails when prefix_real is root ("/"); change the conditional that decides
children to handle the root-prefix edge case by allowing any absolute path under
"/" — e.g. replace that condition with a combined check like "if prefix_real ==
os.sep or normalized_real == prefix_real or
normalized_real.startswith(prefix_real + os.sep):" so the code still computes
suffix and candidate as before and then validates with _is_under_prefix(prefix,
candidate).
Comment on lines
+22
to
+28
| def test_resolve_cli_project_dir_rejects_outside_allowed_prefixes(tmp_path: Path, monkeypatch): | ||
| monkeypatch.chdir(tmp_path) | ||
| disallowed = Path.home().parent / "paperbot-studio-chat-disallowed" | ||
| disallowed.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with pytest.raises(ValueError, match="not allowed"): | ||
| _resolve_cli_project_dir(str(disallowed)) |
There was a problem hiding this comment.
Test creates directory outside tmp_path and never cleans it up.
Lines 24-25 create a directory at Path.home().parent / "paperbot-studio-chat-disallowed", which:
- Creates a persistent directory outside the test sandbox (e.g.,
/home/paperbot-studio-chat-disallowed) - May fail with permission errors if
Path.home().parentis root-owned - Leaves artifacts after test completion
Per coding guidelines, use tmp_path fixture for temporary test directories.
🔧 Proposed fix using environment variable to create truly disallowed path
def test_resolve_cli_project_dir_rejects_outside_allowed_prefixes(tmp_path: Path, monkeypatch):
- monkeypatch.chdir(tmp_path)
- disallowed = Path.home().parent / "paperbot-studio-chat-disallowed"
- disallowed.mkdir(parents=True, exist_ok=True)
+ # Create two isolated directories - one as cwd, one as "disallowed"
+ cwd_dir = tmp_path / "cwd"
+ cwd_dir.mkdir()
+ disallowed = tmp_path / "disallowed"
+ disallowed.mkdir()
+
+ monkeypatch.chdir(cwd_dir)
+ # Clear any env overrides and runtime config to ensure only cwd is allowed
+ monkeypatch.setenv("PAPERBOT_RUNBOOK_ALLOW_DIR_PREFIXES", "")
+ monkeypatch.setattr(
+ "paperbot.api.routes.studio_chat._load_runtime_allowed_dirs",
+ lambda: [],
+ )
with pytest.raises(ValueError, match="not allowed"):
_resolve_cli_project_dir(str(disallowed))This approach keeps everything within tmp_path while still testing the rejection logic by mocking the allowed-dirs sources.
📝 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.
Suggested change
| def test_resolve_cli_project_dir_rejects_outside_allowed_prefixes(tmp_path: Path, monkeypatch): | |
| monkeypatch.chdir(tmp_path) | |
| disallowed = Path.home().parent / "paperbot-studio-chat-disallowed" | |
| disallowed.mkdir(parents=True, exist_ok=True) | |
| with pytest.raises(ValueError, match="not allowed"): | |
| _resolve_cli_project_dir(str(disallowed)) | |
| def test_resolve_cli_project_dir_rejects_outside_allowed_prefixes(tmp_path: Path, monkeypatch): | |
| # Create two isolated directories - one as cwd, one as "disallowed" | |
| cwd_dir = tmp_path / "cwd" | |
| cwd_dir.mkdir() | |
| disallowed = tmp_path / "disallowed" | |
| disallowed.mkdir() | |
| monkeypatch.chdir(cwd_dir) | |
| # Clear any env overrides and runtime config to ensure only cwd is allowed | |
| monkeypatch.setenv("PAPERBOT_RUNBOOK_ALLOW_DIR_PREFIXES", "") | |
| monkeypatch.setattr( | |
| "paperbot.api.routes.studio_chat._load_runtime_allowed_dirs", | |
| lambda: [], | |
| ) | |
| with pytest.raises(ValueError, match="not allowed"): | |
| _resolve_cli_project_dir(str(disallowed)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_studio_chat_path_validation.py` around lines 22 - 28, The
test currently creates a persistent directory outside the test sandbox; instead
create the disallowed directory under the provided tmp_path and then mock the
source of allowed prefixes so the path is considered "outside" allowed prefixes;
e.g., create disallowed = tmp_path / "paperbot-studio-chat-disallowed" and then
monkeypatch the mechanism used to compute allowed prefixes (or monkeypatch
Path.home()/Path.home().parent) so that disallowed is not within allowed
prefixes, then call _resolve_cli_project_dir(str(disallowed)) expecting
ValueError; this keeps files inside tmp_path and avoids permission/artifact
issues.
Comment on lines
+101
to
+106
| const normalizedTitle = title.trim().toLowerCase() | ||
| const duplicate = papers.find(p => p.title.trim().toLowerCase() === normalizedTitle) | ||
| if (duplicate) { | ||
| setError("A paper with this title already exists in the studio.") | ||
| return | ||
| } |
There was a problem hiding this comment.
Add regression tests for the new title-based dedup flows.
Line 101 and Line 117 introduce new behavior branches (manual duplicate rejection, import-skip handling, and singular/plural error messaging), but there are no corresponding test updates in the provided changes. Please add tests for these paths before merge.
As per coding guidelines, {src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.
Also applies to: 117-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/NewPaperModal.tsx` around lines 101 - 106, The PR
added title-based dedup logic in NewPaperModal.tsx (look for the NewPaperModal
component and the normalizedTitle/duplicate check that calls setError) and
additional branches for manual duplicate rejection, import-skip behavior, and
singular vs plural error messages; add regression tests that render
NewPaperModal and simulate: submitting a title that differs only by
case/whitespace to assert the duplicate branch sets the correct error message,
the manual-reject path (simulate the user choosing "reject duplicate") to assert
the modal closes/sets state as expected, the import-skip flow to assert it
returns the expected skip result instead of error, and both singular/plural
message variations (1 vs >1 existing duplicates) to assert correct wording;
place tests alongside other web/src component tests (tsx test files), use the
component's public handlers (submit button, props) to drive the scenarios, and
assert on DOM error text and any emitted callbacks.
Comment on lines
+2
to
+13
| const listRes = await fetch( | ||
| `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true` | ||
| ); | ||
| if (!listRes.ok) { | ||
| throw new Error(`Failed to list project files (${listRes.status})`); | ||
| } | ||
|
|
||
| const payload = (await listRes.json()) as { files?: unknown }; | ||
| const files = Array.isArray(payload.files) | ||
| ? payload.files.filter((v): v is string => typeof v === "string" && v.length > 0) | ||
| : []; | ||
|
|
There was a problem hiding this comment.
Prevent silent partial deletion when file listing is truncated.
Line 2-Line 12 ignores the truncated response flag from /api/runbook/files. If the project has more files than the API cap, this function deletes only a subset and still reports success.
Suggested fix
- const listRes = await fetch(
- `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true`
- );
+ const listRes = await fetch(
+ `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true&max_files=20000`
+ );
@@
- const payload = (await listRes.json()) as { files?: unknown };
+ const payload = (await listRes.json()) as {
+ files?: unknown;
+ truncated?: boolean;
+ max_files?: number;
+ };
+ if (payload.truncated) {
+ throw new Error(
+ `File list truncated at ${payload.max_files ?? "unknown"} entries; aborting partial delete`
+ );
+ }📝 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.
Suggested change
| const listRes = await fetch( | |
| `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true` | |
| ); | |
| if (!listRes.ok) { | |
| throw new Error(`Failed to list project files (${listRes.status})`); | |
| } | |
| const payload = (await listRes.json()) as { files?: unknown }; | |
| const files = Array.isArray(payload.files) | |
| ? payload.files.filter((v): v is string => typeof v === "string" && v.length > 0) | |
| : []; | |
| const listRes = await fetch( | |
| `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true&max_files=20000` | |
| ); | |
| if (!listRes.ok) { | |
| throw new Error(`Failed to list project files (${listRes.status})`); | |
| } | |
| const payload = (await listRes.json()) as { | |
| files?: unknown; | |
| truncated?: boolean; | |
| max_files?: number; | |
| }; | |
| if (payload.truncated) { | |
| throw new Error( | |
| `File list truncated at ${payload.max_files ?? "unknown"} entries; aborting partial delete` | |
| ); | |
| } | |
| const files = Array.isArray(payload.files) | |
| ? payload.files.filter((v): v is string => typeof v === "string" && v.length > 0) | |
| : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/lib/runbook/deleteProjectFiles.ts` around lines 2 - 13, The file
listing handler in deleteProjectFiles.ts currently ignores the response's
truncation flag and may proceed to delete only a subset; update the code that
parses listRes.json() (payload) to check for a boolean payload.truncated (or
equivalent) and, if true, abort and surface an error instead of continuing;
ensure the same function (where listRes, payload, and files are defined)
throws/reports a clear error when truncated is true so deletions are not
performed on a partial file list.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
说明
将
dev分支最新 9 个提交合并到master。主要变更
/extract/task+ 轮询）。data:image/...inline 回退。.env相关忽略规则完善。关联提交
备注
仓库默认分支当前为
master（非main），本 PR 以master为目标分支。Summary by CodeRabbit
New Features
Improvements
Tests
Documentation