Fix/studio output dir validation p2c design#148
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Paper‑to‑Context (P2C) feature: multi‑stage extraction pipeline (LLM + heuristics), orchestrator, persistence (DB models, migration, store), FastAPI SSE generation and CRUD endpoints, Next.js proxy routes, frontend types/hooks/components/store changes, benchmarks, tests, CI/config and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Web Client
participant NextJS as Next.js API Route
participant Python as Python Backend
participant Orchestrator as ExtractionOrchestrator
participant Stage as Extraction Stage
participant LLM as LLM Service
participant Store as ReproContextStore
participant DB as Database
Client->>NextJS: POST /api/research/repro/context (paper_id,...)
NextJS->>Python: Proxy POST (SSE)
Python->>Orchestrator: run(GenerateContextRequest)
Orchestrator->>Store: save(initial pack, status=running)
Store->>DB: INSERT repro_context_pack
loop per stage
Orchestrator->>Stage: run(stage input)
Stage->>LLM: prompt -> response (or fallback heuristics)
Stage-->>Orchestrator: StageResult (observations, warnings)
Orchestrator->>Store: save_stage_result(...)
Orchestrator->>Python: emit SSE (stage_progress, stage_observations)
Python-->>NextJS: forward SSE
NextJS-->>Client: SSE events delivered
end
Orchestrator->>Store: update_status(pack_id, completed, pack_data)
Store->>DB: UPDATE repro_context_pack
Client->>NextJS: GET /api/research/repro/context/{pack_id}
NextJS->>Python: Proxy GET
Python->>Store: get(pack_id)
Store->>DB: SELECT pack
Store-->>Python: pack JSON
Python-->>NextJS: JSON
NextJS-->>Client: Context pack rendered
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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! 此拉取请求主要围绕 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
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 PR 实现了 paperToContext (P2C) 模块，这是一个重大的新功能，用于从论文中提取上下文以支持代码复现。整体实现结构清晰，涵盖了后端服务、数据库迁移、前端集成和设计文档，非常完整。代码质量很高，尤其是在模块化设计、可扩展性（如 PaperInputAdapter）和鲁棒性（如 LLM 失败后的回退机制）方面做得很好。
我的评审意见主要集中在一些可以进一步提升代码健壮性和可维护性的地方，例如并发安全、更精确的异常处理和 API 数据结构的一致性。这些修改将有助于确保新功能在生产环境中的稳定性和长期可维护性。
Comment on lines
+50
to
+60
| def _save_runtime_allowed_dir(dir_path: Path) -> None: | ||
| f = _runtime_allowed_dirs_file() | ||
| f.parent.mkdir(parents=True, exist_ok=True) | ||
| existing = _load_runtime_allowed_dirs() | ||
| resolved = dir_path.resolve() | ||
| if resolved not in existing: | ||
| existing.append(resolved) | ||
| f.write_text( | ||
| json.dumps([str(p) for p in existing], ensure_ascii=False, indent=2), | ||
| encoding="utf-8", | ||
| ) |
There was a problem hiding this comment.
_save_runtime_allowed_dir 函数存在竞态条件。它采用“读取-修改-写入”的模式操作 JSON 文件，当有多个并发请求访问此端点时，一个请求的写入操作可能会覆盖另一个请求的更改。为了保证操作的原子性，建议在读写此共享文件时使用文件锁（例如，在类 Unix 系统上使用 fcntl，或使用提供跨平台文件锁的库）。
| op.create_table( | ||
| 'repro_context_pack', | ||
| sa.Column('id', sa.String(length=64), nullable=False), | ||
| sa.Column('user_id', sa.String(length=64), nullable=False, server_default='default'), |
There was a problem hiding this comment.
在 user_id 列上使用字符串字面量 'default' 作为 server_default 可能会导致一些问题。这会将一个特殊值与实际的用户 ID 混合在同一列中，可能会使查询和数据分析变得复杂（例如，需要 WHERE user_id != 'default'）。建议考虑将此列设置为可空（nullable=True），并在应用层处理默认用户的情况。这样可以保持 user_id 列中数据类型的一致性。
Comment on lines
+41
to
+47
| try: | ||
| data = json.loads(f.read_text(encoding="utf-8")) | ||
| if isinstance(data, list): | ||
| return [Path(p).resolve() for p in data if isinstance(p, str) and p.strip()] | ||
| except Exception: | ||
| pass | ||
| return [] |
There was a problem hiding this comment.
捕获宽泛的 except Exception: 可能会掩盖具体的错误，使调试变得更加困难。建议捕获更具体的预期异常，例如 FileNotFoundError 和 json.JSONDecodeError。这有助于实现更精细的错误处理和更清晰的日志记录。
Suggested change
| try: | |
| data = json.loads(f.read_text(encoding="utf-8")) | |
| if isinstance(data, list): | |
| return [Path(p).resolve() for p in data if isinstance(p, str) and p.strip()] | |
| except Exception: | |
| pass | |
| return [] | |
| try: | |
| data = json.loads(f.read_text(encoding="utf-8")) | |
| if isinstance(data, list): | |
| return [Path(p).resolve() for p in data if isinstance(p, str) and p.strip()] | |
| except (FileNotFoundError, json.JSONDecodeError): | |
| pass | |
| return [] |
Comment on lines
+156
to
+162
| except Exception as exc: # noqa: BLE001 | ||
| Logger.error( | ||
| f"[M2] generation_failed pack_id={pack_id} error={exc}", | ||
| file=LogFiles.ERROR, | ||
| ) | ||
| result_holder.append(exc) | ||
| await queue.put(_ERROR) |
There was a problem hiding this comment.
捕获宽泛的 except Exception 可能会掩盖应分别处理的不同类型的错误。例如，来自业务逻辑的 ValueError 可能表示客户端错误 (4xx)，而其他意外异常可能表示服务器错误 (5xx)。建议捕获更具体的异常，以便为客户端提供更准确的错误响应和状态码，这也有助于提高后端的可调试性。
| return paper_id.strip().isdigit() | ||
|
|
||
| async def fetch(self, paper_id: str) -> RawPaperData: | ||
| from paperbot.utils.logging_config import Logger, LogFiles |
There was a problem hiding this comment.
这里的 from paperbot.utils.logging_config import Logger, LogFiles 是一个方法内的局部导入。根据 PEP 8 的建议，所有导入都应放在文件的顶部。这能提高代码的可读性，并使模块的依赖关系更加清晰。请将此导入语句移至文件顶部。
Comment on lines
+212
to
+222
| def _row_to_full_dict(self, row: ReproContextPackModel) -> Dict[str, Any]: | ||
| base = self._row_to_summary_dict(row) | ||
| base.update({ | ||
| "user_id": row.user_id, | ||
| "project_id": row.project_id, | ||
| "objective": row.objective, | ||
| "version": row.version, | ||
| "updated_at": row.updated_at.isoformat() if row.updated_at else None, | ||
| "pack": row.get_pack(), | ||
| }) | ||
| return base |
There was a problem hiding this comment.
_row_to_full_dict 方法将完整的上下文包数据嵌套在 "pack" 键下。然而，这导致前端 studio/page.tsx 组件需要一个 normalizePack 函数来处理两种可能的数据结构。为了简化前端逻辑并使 API 响应更一致，建议修改 _row_to_full_dict，将 pack 的内容直接合并到顶层字典中，实现扁平化的数据结构。例如，返回 {"paper": ...} 而不是 {"pack": {"paper": ...}}。
Suggested change
| def _row_to_full_dict(self, row: ReproContextPackModel) -> Dict[str, Any]: | |
| base = self._row_to_summary_dict(row) | |
| base.update({ | |
| "user_id": row.user_id, | |
| "project_id": row.project_id, | |
| "objective": row.objective, | |
| "version": row.version, | |
| "updated_at": row.updated_at.isoformat() if row.updated_at else None, | |
| "pack": row.get_pack(), | |
| }) | |
| return base | |
| def _row_to_full_dict(self, row: ReproContextPackModel) -> Dict[str, Any]: | |
| pack_data = row.get_pack() | |
| pack_data.update({ | |
| "context_pack_id": row.id, | |
| "user_id": row.user_id, | |
| "project_id": row.project_id, | |
| "paper_id": row.paper_id, | |
| "paper_title": row.paper_title, | |
| "depth": row.depth, | |
| "status": row.status, | |
| "objective": row.objective, | |
| "confidence_overall": row.confidence_overall, | |
| "warning_count": row.warning_count, | |
| "version": row.version, | |
| "created_at": row.created_at.isoformat() if row.created_at else None, | |
| "updated_at": row.updated_at.isoformat() if row.updated_at else None, | |
| }) | |
| return pack_data |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
.github/workflows/gemini-review.yml-7-12 (1)
7-12:⚠️ Potential issue | 🟠 MajorTighten workflow permissions to least privilege.
issues: writeandid-token: writeare not required forgh pr review --commentand unnecessarily expand token capabilities.🔒 Suggested permission reduction
permissions: contents: read pull-requests: write - issues: write - id-token: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gemini-review.yml around lines 7 - 12, The workflow's permissions list grants unnecessary scopes; remove or tighten `issues: write` and `id-token: write` in the permissions block and keep only the scopes required for `gh pr review --comment` (e.g., `contents: read` and `pull-requests: write`); update the permissions stanza so it no longer includes `issues: write` or `id-token: write`, leaving only the minimum required keys..github/workflows/gemini-review.yml-40-40 (1)
40-40:⚠️ Potential issue | 🟠 MajorPin action and CLI versions to deterministic values.
Using
@v0plusgemini_cli_version: latestintroduces avoidable supply-chain and reproducibility risk.📌 Suggested hardening
- uses: google-github-actions/run-gemini-cli@v0 + uses: google-github-actions/run-gemini-cli@<full_commit_sha> ... - gemini_cli_version: ${{ vars.GEMINI_CLI_VERSION || 'latest' }} + gemini_cli_version: ${{ vars.GEMINI_CLI_VERSION || '<tested_version>' }}Applies to lines 40 and 48.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gemini-review.yml at line 40, Replace the floating action and CLI version references with pinned, deterministic versions: change the action usage string "google-github-actions/run-gemini-cli@v0" to an exact tag or commit SHA (e.g., "@v0.1.2" or "@<commit-sha>") and replace the "gemini_cli_version: latest" value with a concrete version string or checksum (e.g., "gemini_cli_version: 1.2.3" or a verifiable SHA). Update both occurrences referenced by the symbols "google-github-actions/run-gemini-cli@v0" and "gemini_cli_version: latest" so the workflow uses fixed, auditable versions.web/src/app/api/runbook/project-dir/prepare/route.ts-9-17 (1)
9-17:⚠️ Potential issue | 🟠 MajorAdd timeout and failure handling to this upstream proxy call.
The fetch at lines 9-16 lacks timeout protection and will hang indefinitely if the upstream service becomes unresponsive. Proxy routes depend directly on external APIs and require resilience hardening.
Implement
AbortSignal.timeout(10_000)with a try-catch fallback returning a 504 status to gracefully degrade under upstream incidents rather than cascading the failure to clients.🔧 Suggested implementation
export async function POST(req: Request) { const body = await req.text() - const upstream = await fetch(`${apiBaseUrl()}/api/runbook/project-dir/prepare`, { - method: "POST", - headers: { - "Content-Type": "application/json", - Accept: "application/json", - }, - body, - }) + let upstream: Response + try { + upstream = await fetch(`${apiBaseUrl()}/api/runbook/project-dir/prepare`, { + method: "POST", + headers: { + "Content-Type": "application/json", + Accept: "application/json", + }, + body, + signal: AbortSignal.timeout(10_000), + }) + } catch { + return Response.json( + { error: "Upstream /api/runbook/project-dir/prepare unavailable" }, + { status: 504, headers: { "Cache-Control": "no-cache" } }, + ) + } const text = await upstream.text()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/runbook/project-dir/prepare/route.ts` around lines 9 - 17, The upstream fetch call that assigns to upstream should be wrapped with an AbortSignal.timeout(10_000) and a try/catch so the proxy won't hang indefinitely; create an AbortSignal via AbortSignal.timeout(10_000) and pass it as the signal option to the fetch call in route.ts (the same call that currently sets method/headers/body), then catch any thrown AbortError or other errors and return a Response with status 504 (Gateway Timeout) and a brief error message instead of letting the request hang or crash; ensure the existing downstream handling of the variable upstream/text remains inside the try block so only successful fetches proceed.src/paperbot/api/routes/runbook.py-154-177 (1)
154-177:⚠️ Potential issue | 🟠 MajorAdd tests for the new allowed-dirs endpoints.
The PR adds mutable allowlist API behavior, but no corresponding tests are shown for add/list success cases and rejection cases.
As per coding guidelines
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/runbook.py` around lines 154 - 177, Add unit tests exercising the new endpoints add_allowed_dir and get_allowed_dirs: use a TestClient to POST to "/runbook/allowed-dirs" with a valid temporary directory and assert a 200 response, returned "directory" matches resolved path and that GET "/runbook/allowed-dirs" (or calling get_allowed_dirs) includes that prefix; also add negative tests that POSTing an invalid path (non-resolvable string) returns 400 and that POSTing a path that doesn't exist or is a file returns 400. Ensure tests isolate/reset mutable state between cases by clearing or restoring the runtime allowlist (use _save_runtime_allowed_dir/_allowed_workdir_prefixes or test setup/teardown) so tests do not leak state.web/src/app/api/runbook/allowed-dirs/route.ts-1-39 (1)
1-39:⚠️ Potential issue | 🟠 MajorAdd tests for the new allowed-dirs proxy route.
This introduces new GET/POST proxy behavior, but no matching tests are shown for success/error passthrough and header handling.
As per coding guidelines
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/runbook/allowed-dirs/route.ts` around lines 1 - 39, Add unit tests for the new allowed-dirs proxy route (route.ts) covering both exported handlers GET and POST: mock global fetch for upstream calls to assert that successful upstream responses have their status, body, and content-type forwarded and that the "Cache-Control": "no-cache" header is present; add a test for upstream error responses (non-2xx status + error body) to ensure status and body passthrough; for POST, verify the incoming request body and "Content-Type" header are forwarded to fetch (use apiBaseUrl()/allowed-dirs via the mocked fetch), and include tests that validate the fallback when upstream headers lack content-type (route uses "application/json"). Ensure tests restore/cleanup fetch mocks.src/paperbot/application/services/p2c/benchmark.py-69-72 (1)
69-72:⚠️ Potential issue | 🟠 MajorNormalize metric payload type before
extend.Line 71 assumes
metricsis a list. If it is a string,extendadds characters and corrupts F1 scoring.🔧 Suggested patch
metric_candidates: List[str] = [] for obs in pack.get_by_type("metric"): - metric_candidates.extend((obs.structured_data.get("metrics") or [])) + raw_metrics = obs.structured_data.get("metrics") + if isinstance(raw_metrics, str): + metric_candidates.append(raw_metrics) + elif isinstance(raw_metrics, list): + metric_candidates.extend(str(item) for item in raw_metrics if item)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/benchmark.py` around lines 69 - 72, The code in benchmark (within the loop over pack.get_by_type("metric")) assumes obs.structured_data.get("metrics") is a list before calling metric_candidates.extend, which corrupts results when it's a string; update the loop that builds metric_candidates so you normalize the payload from obs.structured_data.get("metrics") to a list (e.g., if value is None skip, if it's a str wrap it as [value], if it's already a list use it, otherwise coerce to a single-element list) before calling metric_candidates.extend, so that _f1_score(case.expected_metrics, metric_candidates) receives proper token-level metric items.src/paperbot/api/routes/runbook.py-50-60 (1)
50-60:⚠️ Potential issue | 🟠 MajorUse atomic + synchronized writes for runtime allowlist file updates.
_save_runtime_allowed_dirdoes read-modify-write on a shared JSON file. Concurrent requests can lose updates or leave partial/corrupt content.🔧 Suggested patch (atomic replace baseline)
def _save_runtime_allowed_dir(dir_path: Path) -> None: @@ - f.write_text( - json.dumps([str(p) for p in existing], ensure_ascii=False, indent=2), - encoding="utf-8", - ) + payload = json.dumps([str(p) for p in existing], ensure_ascii=False, indent=2) + tmp = f.with_suffix(f"{f.suffix}.tmp") + tmp.write_text(payload, encoding="utf-8") + tmp.replace(f)(Also add file locking for multi-process safety.)
🤖 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 50 - 60, _save_runtime_allowed_dir currently does a read-modify-write to the JSON allowlist which can race and corrupt the file; change it to perform a synchronized, atomic update by acquiring an exclusive file lock on the allowlist file (or its directory) before calling _load_runtime_allowed_dirs(), then update the in-memory list, write the new JSON to a temporary file in the same directory, fsync the temp file and its directory, atomically replace the original using os.replace (or equivalent), and finally release the lock; keep references to _runtime_allowed_dirs_file(), _load_runtime_allowed_dirs(), and _save_runtime_allowed_dir() so the locking and atomic temp-then-replace logic wraps the existing read-modify-write and preserves parent directory creation.web/src/app/papers/[id]/page.tsx-37-37 (2)
37-37:⚠️ Potential issue | 🟠 MajorAdd/adjust tests for the Studio link contract change.
This changed navigation payload semantics (
paper_id+generateand extra query fields), but no test updates are shown.As per coding guidelines
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/papers/`[id]/page.tsx at line 37, The Studio link on the paper page now encodes and passes new query params (paper_id, generate=true, title, abstract) via the Link href, so update or add tests that assert the new navigation contract: find the component rendering the Link (the page component that contains the Link href) and change its test expectations to assert the href includes encodeURIComponent(paper.id) as paper_id, generate=true, and encoded title/abstract parameters (or add a new test that mounts the page component and checks the exact href string). Ensure tests check encoding behavior (encodeURIComponent usage) and cover both presence and exact values of paper_id, title, abstract, and generate.
37-37:⚠️ Potential issue | 🟠 MajorDon’t pass full abstract in URL query params.
Line 37 puts
paper.abstractin the URL, which can exceed practical URL limits and leaks full text into browser history/server logs. Prefer passing onlypaper_idand loading content in Studio.🔧 Suggested patch
-<Link href={`/studio?paper_id=${encodeURIComponent(paper.id)}&title=${encodeURIComponent(paper.title)}&abstract=${encodeURIComponent(paper.abstract)}&generate=true`}> +<Link href={`/studio?paper_id=${encodeURIComponent(paper.id)}&generate=true`}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/papers/`[id]/page.tsx at line 37, The Link in page.tsx currently embeds paper.abstract (and title) via encodeURIComponent into the query string which can leak full text and exceed URL limits; change the Link to only include paper_id (paper.id) and any small flags (e.g., generate=true), removing encodeURIComponent(paper.abstract) and long fields, and then update the Studio entry point to load the paper content server-/client-side by reading the paper_id route/query and fetching the paper record (e.g., in the Studio page/component) instead of relying on full text in the URL.web/src/app/api/runbook/allowed-dirs/route.ts-8-10 (1)
8-10:⚠️ Potential issue | 🟠 MajorAdd timeout and error handling to upstream fetch calls, and add tests for the new route handlers.
Lines 8–10 and 23–30 call upstream without timeout or error handling. If the upstream service stalls, the route will hang indefinitely and block server capacity. The codebase already establishes this pattern—see
web/src/app/api/research/_base.tswhich implements timeout + error handling viaAbortControllerand try-catch blocks.Per coding guidelines, behavior changes require tests. Add test coverage for both GET and POST handlers.
🔧 Suggested patch
+const UPSTREAM_TIMEOUT_MS = 10_000 + export async function GET() { - const upstream = await fetch(`${apiBaseUrl()}/api/runbook/allowed-dirs`, { - headers: { Accept: "application/json" }, - }) + let upstream: Response + try { + upstream = await fetch(`${apiBaseUrl()}/api/runbook/allowed-dirs`, { + headers: { Accept: "application/json" }, + signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS), + }) + } catch { + return new Response(JSON.stringify({ detail: "upstream unavailable" }), { + status: 502, + headers: { "Content-Type": "application/json", "Cache-Control": "no-cache" }, + }) + } const text = await upstream.text() return new Response(text, { @@ export async function POST(req: Request) { const body = await req.text() - const upstream = await fetch(`${apiBaseUrl()}/api/runbook/allowed-dirs`, { - method: "POST", - headers: { - "Content-Type": "application/json", - Accept: "application/json", - }, - body, - }) + let upstream: Response + try { + upstream = await fetch(`${apiBaseUrl()}/api/runbook/allowed-dirs`, { + method: "POST", + headers: { + "Content-Type": "application/json", + Accept: "application/json", + }, + body, + signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS), + }) + } catch { + return new Response(JSON.stringify({ detail: "upstream unavailable" }), { + status: 502, + headers: { "Content-Type": "application/json", "Cache-Control": "no-cache" }, + }) + } const text = await upstream.text()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/runbook/allowed-dirs/route.ts` around lines 8 - 10, The upstream fetch calls in the GET/POST handlers in route.ts currently lack timeout and error handling; update both handlers (the GET and POST functions that call fetch to `${apiBaseUrl()}/api/runbook/allowed-dirs`) to use an AbortController with a configurable timeout (match the pattern from web/src/app/api/research/_base.ts), wrap the fetch in try/catch, abort and return an appropriate 504 timeout Response on timeout, and return a 502/500 Response for upstream/fetch errors while logging details; then add tests that exercise successful, timeout, and error cases for both GET and POST route handlers to cover the new behavior.web/src/app/api/research/repro/context/[packId]/route.ts-3-6 (1)
3-6:⚠️ Potential issue | 🟠 MajorAsync params required in Next.js 15+.
Same issue as the session route — dynamic route
paramsmust be awaited in Next.js 15+.Proposed fix
-export async function GET(req: Request, { params }: { params: { packId: string } }) { - const packId = encodeURIComponent(params.packId) +export async function GET(req: Request, { params }: { params: Promise<{ packId: string }> }) { + const { packId: rawPackId } = await params + const packId = encodeURIComponent(rawPackId) return proxyJson(req, `${apiBaseUrl()}/api/research/repro/context/${packId}`, "GET") } -export async function DELETE(req: Request, { params }: { params: { packId: string } }) { - const packId = encodeURIComponent(params.packId) +export async function DELETE(req: Request, { params }: { params: Promise<{ packId: string }> }) { + const { packId: rawPackId } = await params + const packId = encodeURIComponent(rawPackId) return proxyJson(req, `${apiBaseUrl()}/api/research/repro/context/${packId}`, "DELETE") }Also applies to: 8-11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/research/repro/context/`[packId]/route.ts around lines 3 - 6, The GET handler must await the dynamic route params (Next.js 15+) before using them; update the GET function to destructure packId from the awaited params (e.g., const { packId } = await params), then use encodeURIComponent(packId) in the proxyJson call (affects the GET function and any other route handlers in this file that read params).web/src/app/api/research/repro/context/[packId]/session/route.ts-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorUpdate route params to async Promise pattern required in Next.js 16.
In Next.js 16, dynamic route
paramsmust be awaited. The current synchronous access violates Next.js 16 requirements and will cause runtime issues.Required fix
-export async function POST(req: Request, { params }: { params: { packId: string } }) { - const packId = encodeURIComponent(params.packId) +export async function POST(req: Request, { params }: { params: Promise<{ packId: string }> }) { + const { packId } = await params + const packId = encodeURIComponent(packId) return proxyJson(req, `${apiBaseUrl()}/api/research/repro/context/${packId}/session`, "POST") }Note: Other route handlers in
web/src/app/api/research/follow the async pattern; similar files like[packId]/route.tsand[packId]/observation/[observationId]/route.tsalso need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/research/repro/context/`[packId]/session/route.ts around lines 3 - 5, The POST handler currently reads params synchronously; update POST to accept the route context as a Promise and await it before using params (e.g., change the second parameter to a Promise<{ params: { packId: string } }> or a generic ctx Promise, then do const { params } = await ctx), then keep using encodeURIComponent(params.packId) and the existing proxyJson call; update the function signature for POST and any references to params so they await the context first (refer to the POST function, the params variable, and the proxyJson call to locate the code).src/paperbot/application/services/p2c/stages.py-424-446 (1)
424-446:⚠️ Potential issue | 🟠 MajorHeuristic roadmap fallback is not paper-specific.
At Line 424,
_run_heuristicignoresdataand always returns the same static 3-step roadmap. In fallback/offline mode this breaks the “paper-specific roadmap” expectation.If useful, I can draft a lightweight paper-aware fallback (title/method-section keyword driven) and open a follow-up issue with the patch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/stages.py` around lines 424 - 446, _run_heuristic currently ignores the incoming StageInput and returns a static 3-step TaskCheckpoint roadmap; change it to build a paper-aware fallback by extracting paper metadata from the provided data (e.g., data.paper.title, data.paper.abstract, data.paper.sections or keywords) and generate TaskCheckpoint entries (using TaskCheckpoint id/title/acceptance_criteria/depends_on) that reflect detected paper scope (e.g., dataset prep, architecture, experiments) falling back to the existing static roadmap only if no useful metadata is present; ensure the function still returns a StageResult and preserves existing TaskCheckpoint field names so callers of _run_heuristic, StageInput, StageResult, and TaskCheckpoint keep the same contract.src/paperbot/api/routes/repro_context.py-303-341 (1)
303-341:⚠️ Potential issue | 🟠 MajorAvoid returning synthetic session success before real session creation.
At Line 308, this endpoint is still TODO-backed, while Lines 319-341 return generated IDs without persistence/integration and ignore
override_env/override_roadmap. That can report a successful session that does not exist.If you want, I can draft a follow-up patch that either wires the real runbook/session creation flow now or safely returns
501 Not Implementeduntil wired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 303 - 341, The endpoint create_repro_session currently fabricates session_id/runbook_id and returns success without persisting or honoring CreateSessionRequest fields (override_env, override_roadmap); either wire it to the real creation flow by calling the existing runbook/session creation functions (e.g., invoke the service that creates runbooks/sessions, pass request.override_env and request.override_roadmap, wait for persistence and error handling, then return the real IDs), or if that integration isn’t ready change create_repro_session to return HTTP 501 Not Implemented (and log via Logger) instead of generating synthetic IDs so callers don’t get a false successful response.src/paperbot/application/services/p2c/stages.py-268-292 (1)
268-292:⚠️ Potential issue | 🟠 MajorDon’t emit a concrete Python version when evidence is missing.
At Lines 268-292, when framework is
unknown, the code still emits"python_version": "3.11". That is fabricated metadata and can mislead reproduction setup.🔧 Suggested fix
- python_version = "3.10" if framework != "unknown" else "3.11" + python_version = "3.10" if framework != "unknown" else "not_specified" @@ if framework == "unknown": warnings.append("Environment extraction could not find explicit framework evidence.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/stages.py` around lines 268 - 292, The code currently assigns python_version = "3.10" if framework != "unknown" else "3.11" and then emits that value in ExtractionObservation; change this so no concrete Python version is emitted when framework == "unknown": set python_version to None (or omit the key) in that branch, adjust the narrative in ExtractionObservation (e.g., "Inferred runtime stack: framework unknown" or omit version text) and ensure structured_data either excludes "python_version" or sets it to null when framework is unknown; update places referencing python_version variable to handle the None/absent case safely.src/paperbot/infrastructure/stores/repro_context_store.py-137-163 (1)
137-163:⚠️ Potential issue | 🟠 MajorSoft-deleted packs can still leak observations.
At Lines 139-155, stage rows are read before checking whether the pack is soft-deleted. A deleted pack can still return observation payloads through this path.
🔧 Suggested fix
def get_observation(self, pack_id: str, observation_id: str) -> Optional[Dict[str, Any]]: with self._provider.session() as session: + pack_row = session.get(ReproContextPackModel, pack_id) + if pack_row is None or pack_row.deleted_at is not None: + return None + stage_rows = session.scalars( select(ReproContextStageResultModel) .where(ReproContextStageResultModel.context_pack_id == pack_id) .order_by(ReproContextStageResultModel.created_at.asc()) ).all() @@ - pack_row = session.get(ReproContextPackModel, pack_id) - if pack_row is None or pack_row.deleted_at is not None: - return None - pack = pack_row.get_pack() if pack_row else {} + pack = pack_row.get_pack() for obs in pack.get("observations", []) if isinstance(pack, dict) else []: if isinstance(obs, dict) and obs.get("id") == observation_id: return obs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/stores/repro_context_store.py` around lines 137 - 163, The function get_observation currently reads ReproContextStageResultModel rows before verifying whether the pack is soft-deleted, allowing deleted packs to leak observations; fix by first loading the pack via session.get(ReproContextPackModel, pack_id) and checking pack_row is not None and pack_row.deleted_at is None, returning None if deleted/missing, and only then querying ReproContextStageResultModel (and finally falling back to pack_row.get_pack() for pack observations); update references in get_observation to use the early pack_row deleted_at check to short-circuit before reading stage rows.src/paperbot/api/routes/repro_context.py-246-248 (1)
246-248:⚠️ Potential issue | 🟠 MajorValidate pagination bounds before querying storage.
At Line 246 and Line 247,
limit/offsetare user-controlled and unvalidated. Negative or very large values can cause invalid or expensive queries.🔧 Suggested fix
async def list_context_packs( @@ - limit: int = 20, - offset: int = 0, + limit: int = 20, + offset: int = 0, ): @@ + if limit < 1 or limit > 100: + raise HTTPException(status_code=400, detail="limit must be between 1 and 100.") + if offset < 0: + raise HTTPException(status_code=400, detail="offset must be >= 0.") + items, total = _store.list_by_user(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 246 - 248, The limit and offset query parameters in the route handler in repro_context.py (parameters limit and offset) are unvalidated and may be negative or excessively large; add bounds checking before any storage query—either annotate the parameters with FastAPI Query constraints (e.g., limit: int = Query(20, ge=1, le=1000) and offset: int = Query(0, ge=0)) or explicitly validate at the start of the handler (ensure offset >= 0 and 1 <= limit <= MAX_LIMIT, where MAX_LIMIT is a sensible constant like 1000) and return a 400 HTTPException for invalid values or clamp them before calling the storage functions so invalid/expensive queries are prevented.web/src/lib/types/p2c.ts-3-10 (1)
3-10:⚠️ Potential issue | 🟠 Major
ObservationTypeis missing"roadmap"even though backend emits it.The backend produces observations with
type="roadmap"(insrc/paperbot/application/services/p2c/stages.pyat lines 415 and 450), but the frontendObservationTypeunion excludes it, creating a type contract mismatch.🔧 Suggested fix
export type ObservationType = | "method" | "architecture" | "hyperparameter" | "metric" | "environment" + | "roadmap" | "limitation"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/types/p2c.ts` around lines 3 - 10, ObservationType union in web/src/lib/types/p2c.ts is missing the "roadmap" variant which the backend emits; update the ObservationType type alias (ObservationType) to include | "roadmap" so the frontend type matches backend outputs, and search for code that narrows or switches on ObservationType (e.g., any switch/case or render logic using ObservationType) to ensure it handles the new "roadmap" case or adds a safe default.src/paperbot/api/routes/repro_context.py-164-176 (1)
164-176:⚠️ Potential issue | 🟠 MajorRetain explicit reference to background task.
At line 164,
asyncio.create_task(_run())is fire-and-forget. While the queue-based synchronization ensures the generator waits for_DONEor_ERRORsignals, a task without a reference can be garbage-collected by asyncio, especially in future Python versions. The suggested fix holds the reference explicitly and awaits task completion before breaking/returning, ensuring both safety and clear task lifecycle management.🔧 Suggested fix
- asyncio.create_task(_run()) + run_task = asyncio.create_task(_run()) # Drain queue, yielding progress events until the orchestrator finishes. while True: item = await queue.get() if item is _DONE: + await run_task break elif item is _ERROR: + await run_task _store.update_status(pack_id, status="failed") yield StreamEvent(type="error", data={"message": str(result_holder[0])}) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 164 - 176, The background task started with asyncio.create_task(_run()) should be kept in a local variable and awaited before returning to avoid garbage collection and to ensure proper lifecycle; replace the fire-and-forget call with something like task = asyncio.create_task(_run()), and then when handling the _DONE or _ERROR queue signals, await task (or cancel/await if needed) before breaking/returning; update the block that checks queue items (the while loop handling _DONE, _ERROR, and yielding StreamEvent from on_stage_complete) to await the stored task and then proceed to call _store.update_status(pack_id, status="failed") and yield the error StreamEvent using result_holder[0] as before.src/paperbot/api/routes/repro_context.py-219-357 (1)
219-357:⚠️ Potential issue | 🟠 MajorAdd integration tests for the new repro_context API endpoints.
The endpoints
POST /generate,GET,GET /{pack_id},GET /{pack_id}/observation/{observation_id},POST /{pack_id}/session, andDELETE /{pack_id}lack test coverage. Per coding guidelines, behavior changes require corresponding tests, and new API endpoints should have integration tests following the pattern intests/integration/test_research_track_routes.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 219 - 357, Add integration tests covering the new repro_context endpoints: generate_context_pack, list_context_packs, get_context_pack, get_observation_detail, create_repro_session, and delete_context_pack. Create tests that follow the existing integration pattern: exercise POST /generate and assert SSE stream content and status; test list (default and with paper_id/project_id filters) returns items/total; test GET /{pack_id} and GET /{pack_id}/observation/{observation_id} for both existing and 404 cases; test POST /{pack_id}/session returns session_id/runbook_id/initial_steps and handles missing pack (404); and test DELETE /{pack_id} for successful soft-delete and delete-not-found (404). Use the same test fixtures, setup/teardown, and mock store interactions used by the other integration tests and assert key response fields and logger-visible side effects where applicable.src/paperbot/application/services/p2c/orchestrator.py-52-55 (1)
52-55:⚠️ Potential issue | 🟠 MajorImport
ExtractionObservationfor type-checking to resolve forward-referenced annotations.The type alias
StageCompleteCallbackat lines 52–55 and the parameter annotation at line 301 both reference"ExtractionObservation"as a string forward reference, butExtractionObservationis not imported in this module. Even thoughfrom __future__ import annotationsdefers evaluation, type checkers like pyright will attempt to resolve the name and fail with an unresolved symbol error.Add the import under
TYPE_CHECKING:Suggested fix
if TYPE_CHECKING: from paperbot.application.services.llm_service import LLMService + from .models import ExtractionObservation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/orchestrator.py` around lines 52 - 55, StageCompleteCallback and the parameter annotation referencing "ExtractionObservation" are unresolved by static type checkers; add a TYPE_CHECKING-only import for ExtractionObservation (e.g. add "from typing import TYPE_CHECKING" if not present and inside "if TYPE_CHECKING: from <module_where_ExtractionObservation_is_defined> import ExtractionObservation") so the forward reference resolves for tools like pyright while keeping runtime behavior unchanged; update the file where StageCompleteCallback and the function using the annotation (the orchestrator definitions including the parameter at the place currently annotated with "ExtractionObservation") are defined.
🟡 Minor comments (12)
docs/P2C_MODULE_3_FRONTEND.md-302-302 (1)
302-302:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
Several fenced blocks are unlabeled, which is currently linted as warnings.
📝 Example fix pattern
-``` +```text ┌─────────────────────────────────────────┐ ... -``` +```Also applies to: 326-326, 400-400, 510-510, 597-597, 675-675, 720-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/P2C_MODULE_3_FRONTEND.md` at line 302, Several fenced code blocks in the document are unlabeled (MD040 warnings); locate the unlabeled triple-backtick blocks (the examples shown and the other occurrences noted) and add an appropriate language identifier after the opening backticks (e.g., ```text, ```bash, ```json, ```js) so the linter recognizes them; ensure each modified fenced block matches the content type and keep the closing backticks unchanged.docs/P2C_MODULE_3_FRONTEND.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorResolve the “no new entry button” contradiction in the design doc.
Line 13 states no new entry button, while Line 728 lists
GenerateReproButton.tsxas newly added. Keep one direction to avoid implementation ambiguity.Also applies to: 728-728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/P2C_MODULE_3_FRONTEND.md` at line 13, The doc contains a contradiction: the top-level requirement says "no new entry button" but the feature list includes GenerateReproButton.tsx as newly added; reconcile by choosing one approach and updating the doc accordingly—either remove GenerateReproButton.tsx from the "newly added" list (and state the Papers Library's existing entry button will be reused), or change the initial statement to allow a new button and keep GenerateReproButton.tsx listed; update any mentions of reuse to reference GenerateReproButton.tsx or the Papers Library entry button so the design intent is consistent.docs/P2C_MODULE_1_CORE_ENGINE.md-156-202 (1)
156-202:⚠️ Potential issue | 🟡 MinorLabel non-code fenced blocks with a language token (MD040).
The diagram/tree fences should specify a language (e.g.,
text) to satisfy markdownlint and keep formatting checks green.Also applies to: 329-335, 451-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/P2C_MODULE_1_CORE_ENGINE.md` around lines 156 - 202, The fenced ASCII diagrams (the block starting with "GenerateContextRequest" and "InputNormalizer" and the other similar diagram blocks around the file) are missing a language token; update each triple-backtick fence that wraps those non-code ASCII diagrams to include a language token like text (e.g., change ``` to ```text) so markdownlint rule MD040 is satisfied; apply the same change to the other diagram/code-fence blocks called out (the blocks that include "Stage A: Literature", "Stage B: Blueprint", "EvidenceLinker + ContextAssembler", etc.) so all three problematic regions are annotated.src/paperbot/api/routes/runbook.py-125-126 (1)
125-126:⚠️ Potential issue | 🟡 MinorPreserve exception chaining on 400 conversions.
Use
except Exception as excandraise HTTPException(...) from excso root causes remain traceable.🔧 Suggested patch
- except Exception: - raise HTTPException(status_code=400, detail="invalid project_dir") + except Exception as exc: + raise HTTPException(status_code=400, detail="invalid project_dir") from exc @@ - except Exception: - raise HTTPException(status_code=400, detail="invalid directory path") + except Exception as exc: + raise HTTPException(status_code=400, detail="invalid directory path") from excAlso applies to: 159-160
🤖 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 125 - 126, Replace the bare "except Exception" handlers in this module with "except Exception as exc" and re-raise the HTTPException using "raise HTTPException(status_code=400, detail='invalid project_dir') from exc" so the original traceback is preserved; update both occurrences (the handler around the project_dir validation and the other occurrence noted) to use the "exc" variable and "from exc" chaining when raising HTTPException.src/paperbot/api/routes/runbook.py-41-47 (1)
41-47:⚠️ Potential issue | 🟡 MinorAvoid blind
except ...: passin allowlist loading/building.Silent failure here can hide malformed JSON or filesystem problems and unexpectedly drop allowed prefixes.
Also applies to: 65-68
🤖 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 41 - 47, Replace the silent broad except in the allowlist-loading code that surrounds json.loads(f.read_text(...)) and the similar block at lines 65-68: catch and handle specific exceptions (e.g., json.JSONDecodeError, OSError/IOError) instead of bare except, log the error with context (which file/path and the exception) using the module logger, and only swallow the error after logging; preserve the existing return [] fallback but ensure malformed JSON or filesystem problems are visible in logs so callers can diagnose failures.src/paperbot/application/services/p2c/evidence.py-29-31 (1)
29-31:⚠️ Potential issue | 🟡 MinorSkip blank keywords before regex matching.
If
keywordscontains""/whitespace, Line 30 compiles\b\b, which can generate spurious evidence spans.🔧 Suggested patch
for keyword in keywords: + if not keyword or not keyword.strip(): + continue pattern = re.compile(rf"\b{re.escape(keyword)}\b", re.IGNORECASE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/evidence.py` around lines 29 - 31, The loop that builds regex patterns from each keyword can compile empty/whitespace keywords (producing \b\b) and yield spurious matches; before compiling (inside the for keyword in keywords: loop or by pre-filtering the list) skip any keyword that is empty or only whitespace (e.g., if not keyword.strip(): continue) so you never call re.compile on blank strings; update the code around the use of keyword, re.compile(rf"\b{re.escape(keyword)}\b", re.IGNORECASE) and pattern.finditer(text) to operate only on non-empty trimmed keywords.web/src/app/api/research/repro/context/route.ts-10-30 (1)
10-30:⚠️ Potential issue | 🟡 MinorPOST handler lacks timeout and error handling for the upstream fetch.
Unlike the
proxyJsonhelper which includes a 2-minute timeout viaAbortControllerand proper error handling, this SSE streaming implementation has no timeout and will not gracefully handle fetch failures.🛡️ Suggested improvement
export async function POST(req: Request) { const body = await req.text() + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 300_000) // 5 min for SSE + + let upstream: Response + try { - const upstream = await fetch(`${apiBaseUrl()}/api/research/repro/context/generate`, { + upstream = await fetch(`${apiBaseUrl()}/api/research/repro/context/generate`, { method: "POST", headers: { "Content-Type": req.headers.get("content-type") || "application/json", Accept: "text/event-stream", }, body, + signal: controller.signal, }) + } catch (error) { + clearTimeout(timeout) + return Response.json( + { detail: "Upstream API unreachable" }, + { status: 502 } + ) + } const headers = new Headers() headers.set("Content-Type", upstream.headers.get("content-type") || "text/event-stream") headers.set("Cache-Control", "no-cache") headers.set("Connection", "keep-alive") return new Response(upstream.body, { status: upstream.status, headers, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/research/repro/context/route.ts` around lines 10 - 30, The POST handler (function POST) must use an AbortController with a 2-minute timeout and add try/catch around the upstream fetch to handle failures; create an AbortController, start a timer to call controller.abort() after 120000ms, pass controller.signal to fetch, await the fetch inside try/catch, clear the timer on success, and on error return an appropriate Response (e.g., 504 for timeout or 502 for other fetch errors) with a useful error message and proper headers instead of letting the request hang. Ensure you reference the same headers logic and stream upstream.body to the response only when fetch succeeds.web/src/components/studio/WorkspaceSetupDialog.tsx-77-82 (1)
77-82:⚠️ Potential issue | 🟡 MinorEdge case in parent directory extraction for root paths.
The parent directory extraction logic
parts.slice(0, -1).join("/")may produce unexpected results for root-level paths. For example:
- Input
/home→ parts =["", "home"]→ parent =""(empty string)- Input
/→ parts =["", ""]→ parent =""(empty string)This could result in an empty
pendingAllowDirbeing set, which may cause issues in the allow flow.🛡️ Suggested improvement
if (res.status === 403) { // Extract parent directory to add as allowed prefix const parts = directory.replace(/\/+$/, "").split("/") - const parentDir = parts.length > 1 ? parts.slice(0, -1).join("/") : directory + const parentDir = parts.length > 1 + ? parts.slice(0, -1).join("/") || "/" + : directory setPendingAllowDir(parentDir) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/WorkspaceSetupDialog.tsx` around lines 77 - 82, The parent directory extraction can yield an empty string for root-level paths; update the logic around setPendingAllowDir so after trimming trailing slashes and computing parentParts = parts.slice(0, -1).join("/"), you normalize an empty parentParts to "/" (and ensure directory "/" also maps to "/") before calling setPendingAllowDir; adjust the branch that currently sets parentDir to use this normalized value so root and top-level inputs like "/home" produce "/" instead of an empty string.web/src/components/studio/ContextPackPanel.tsx-107-109 (1)
107-109:⚠️ Potential issue | 🟡 MinorUsing warning text as React key may cause issues with duplicate warnings.
If the pack contains duplicate warning messages, React will produce key collisions.
Proposed fix
- {pack.warnings.map((warning) => ( - <li key={warning}>{warning}</li> + {pack.warnings.map((warning, idx) => ( + <li key={`${warning}-${idx}`}>{warning}</li> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/ContextPackPanel.tsx` around lines 107 - 109, The map over pack.warnings in ContextPackPanel uses the warning text as the React key which can collide for duplicate messages; update the mapping in the render inside ContextPackPanel (the pack.warnings.map callback) to use a stable unique key instead—either use an identifier on each warning if warnings become objects (e.g., warning.id) or combine the warning text with the index (e.g., `${warning}-${index}`) or simply use the array index as a fallback key to ensure uniqueness and avoid React key collisions.web/src/lib/store/studio-store.ts-252-256 (1)
252-256:⚠️ Potential issue | 🟡 MinorMissing
liveObservationsreset inselectPaper.The
addPaperaction resets bothgenerationProgressandliveObservations, butselectPaperonly resetsgenerationProgress. This inconsistency may leave stale observations visible when switching papers.Proposed fix
contextPack: null, contextPackLoading: false, contextPackError: null, generationProgress: [], + liveObservations: [], })🤖 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 252 - 256, selectPaper does not clear liveObservations while addPaper does, which can leave stale observations when switching papers; update the selectPaper action (function/method selectPaper in the store) to also reset liveObservations to the same initial value used in addPaper (e.g., set liveObservations: [] or null depending on how addPaper resets it) alongside resetting generationProgress so both flows behave consistently.web/src/app/studio/page.tsx-80-98 (1)
80-98:⚠️ Potential issue | 🟡 MinorPotential race condition with parallel context pack and generation flows.
When both
contextPackIdandgenerateFlagare present in URL params, both flows will execute. IfgenerateFlagis set with apaperId, the generation will overwrite any loaded context pack. Consider clarifying the intended priority or making them mutually exclusive.Suggested fix
if (contextPackId) { shouldCleanUrl = true + // Skip generation if loading an existing pack + } else if (generateFlag) { - } - - if (generateFlag) { shouldCleanUrl = true if (paperId) { console.info("[P2C:M3] studio:trigger_generate", { paperId }) generate({ paperId }) } else { console.warn("[P2C:M3] studio:missing_paper_id") setContextPackError("Missing paper_id for generation.") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/studio/page.tsx` around lines 80 - 98, When both contextPackId and generateFlag/paperId can trigger parallel flows, make them mutually exclusive or serialize them to avoid one overwriting the other: decide the desired priority (e.g., context pack should win or generation should wait), then update the logic around the context pack fetch (the block referencing contextPackId, setContextPackLoading, setContextPackError, clearGenerationProgress, fetch(`/api/research/repro/context/${contextPackId}`), normalizePack and setContextPack) to either skip fetching when generateFlag/paperId is present (if generation should take precedence) or defer starting the generation flow until after the fetch completes and setContextPack runs (if context pack should take precedence); implement this by adding a guard check for generateFlag/paperId or by converting the flows to async/await and awaiting the pack fetch promise before kicking off the generation flow so setContextPack cannot be overwritten by concurrent generation.CLAUDE.md-221-221 (1)
221-221:⚠️ Potential issue | 🟡 MinorCapitalize “GitHub” consistently in docs.
At Line 221, use
GitHub(capital “H”) to match official naming.✏️ Suggested doc fix
-GitHub Actions (`.github/workflows/ci.yml`) runs on push/PR to `master`: +GitHub Actions (`.github/workflows/ci.yml`) runs on push/PR to `master`:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 221, In CLAUDE.md update the occurrence of "Github Actions (`.github/workflows/ci.yml`) runs on push/PR to `master`:" to use the correct branding "GitHub" (i.e., change "Github Actions" to "GitHub Actions") so the document consistently capitalizes GitHub; locate the exact text string and replace the lower-case "b" version with "GitHub".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
.coderabbit.yaml.github/CODEOWNERS.github/workflows/gemini-review.ymlCLAUDE.mdalembic/versions/b94c1a2be26e_add_p2c_repro_context_pack_tables.pydocs/AUTO_REVIEW_SETUP.mddocs/P2C_MODULE1_BENCHMARK.mddocs/P2C_MODULE_1_CORE_ENGINE.mddocs/P2C_MODULE_2_API_STORAGE.mddocs/P2C_MODULE_3_FRONTEND.mddocs/PAPER_TO_CONTEXT_MODULE_DESIGN.mdevals/fixtures/p2c/module1_gold.jsonscripts/p2c_module1_benchmark.pysrc/paperbot/api/main.pysrc/paperbot/api/routes/__init__.pysrc/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/runbook.pysrc/paperbot/application/ports/repro_context_port.pysrc/paperbot/application/services/__init__.pysrc/paperbot/application/services/p2c/__init__.pysrc/paperbot/application/services/p2c/benchmark.pysrc/paperbot/application/services/p2c/evidence.pysrc/paperbot/application/services/p2c/input_pipeline.pysrc/paperbot/application/services/p2c/models.pysrc/paperbot/application/services/p2c/orchestrator.pysrc/paperbot/application/services/p2c/prompts.pysrc/paperbot/application/services/p2c/stages.pysrc/paperbot/infrastructure/stores/models.pysrc/paperbot/infrastructure/stores/repro_context_store.pytests/unit/test_p2c_benchmark.pytests/unit/test_p2c_module1.pytests/unit/test_runbook_project_dir_prepare_route.pyweb/src/app/api/research/repro/context/[packId]/observation/[observationId]/route.tsweb/src/app/api/research/repro/context/[packId]/route.tsweb/src/app/api/research/repro/context/[packId]/session/route.tsweb/src/app/api/research/repro/context/route.tsweb/src/app/api/runbook/allowed-dirs/route.tsweb/src/app/api/runbook/project-dir/prepare/route.tsweb/src/app/papers/[id]/page.tsxweb/src/app/studio/page.tsxweb/src/components/studio/ContextPackPanel.tsxweb/src/components/studio/FilesPanel.tsxweb/src/components/studio/GenerationProgressPanel.tsxweb/src/components/studio/ReproductionLog.tsxweb/src/components/studio/WorkspaceSetupDialog.tsxweb/src/hooks/useContextPackGeneration.tsweb/src/lib/store/studio-store.tsweb/src/lib/types/p2c.ts
There was a problem hiding this comment.
Pull request overview
Implements the Paper-to-Context (P2C) pipeline end-to-end: backend extraction stages + persistence + SSE API, and Studio UI support to generate/view context packs and create reproduction sessions. Also enhances Studio workspace directory validation with a runtime allowlist mechanism.
Changes:
- Added P2C core engine (stages, prompts, orchestrator), API routes (generate/list/get/session/observation), and SQLAlchemy persistence with Alembic migration.
- Extended Studio UI/store with P2C generation progress + context pack visualization and deep-linking from Papers → Studio.
- Added project directory preparation endpoint + UI validation flow, including runtime allowlisted directories.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/types/p2c.ts | Frontend type contract for P2C payloads |
| web/src/lib/store/studio-store.ts | Adds P2C state/actions to store |
| web/src/hooks/useContextPackGeneration.ts | SSE-driven context pack generation hook |
| web/src/components/studio/WorkspaceSetupDialog.tsx | Workspace dir validation + allowlist UX |
| web/src/components/studio/ReproductionLog.tsx | Adds tabs for progress/context/chat |
| web/src/components/studio/GenerationProgressPanel.tsx | Progress + live observations UI |
| web/src/components/studio/FilesPanel.tsx | Validates directory and improves errors |
| web/src/components/studio/ContextPackPanel.tsx | Context pack viewer + session creation |
| web/src/app/studio/page.tsx | Supports generate/deep-link query params |
| web/src/app/papers/[id]/page.tsx | “Run Reproduction” triggers generation |
| web/src/app/api/runbook/project-dir/prepare/route.ts | Next.js proxy for prepare endpoint |
| web/src/app/api/runbook/allowed-dirs/route.ts | Next.js proxy for allowlist endpoints |
| web/src/app/api/research/repro/context/route.ts | Next.js proxy for SSE generation |
| web/src/app/api/research/repro/context/[packId]/route.ts | Proxy: get/delete pack |
| web/src/app/api/research/repro/context/[packId]/session/route.ts | Proxy: create session |
| web/src/app/api/research/repro/context/[packId]/observation/[observationId]/route.ts | Proxy: observation detail |
| tests/unit/test_runbook_project_dir_prepare_route.py | Tests for prepare-project-dir behavior |
| tests/unit/test_p2c_module1.py | Unit tests for P2C module 1 |
| tests/unit/test_p2c_benchmark.py | Benchmark unit tests and helpers |
| src/paperbot/infrastructure/stores/repro_context_store.py | P2C persistence store implementation |
| src/paperbot/infrastructure/stores/models.py | Adds P2C SQLAlchemy models |
| src/paperbot/application/services/p2c/stages.py | Extraction stages (LLM + heuristic) |
| src/paperbot/application/services/p2c/prompts.py | Prompt templates for stages |
| src/paperbot/application/services/p2c/orchestrator.py | Orchestrates stage sequence + scoring |
| src/paperbot/application/services/p2c/models.py | P2C datamodels (pack/obs/confidence) |
| src/paperbot/application/services/p2c/input_pipeline.py | Paper input adapters + section extractor |
| src/paperbot/application/services/p2c/evidence.py | Evidence linking + confidence calibration |
| src/paperbot/application/services/p2c/benchmark.py | Benchmark runner + scoring utilities |
| src/paperbot/application/services/p2c/init.py | Exposes P2C public API |
| src/paperbot/application/services/init.py | Re-exports orchestrator |
| src/paperbot/application/ports/repro_context_port.py | Persistence port interface |
| src/paperbot/api/routes/runbook.py | Runtime allowlist + prepare dir endpoints |
| src/paperbot/api/routes/repro_context.py | P2C HTTP API + SSE generation |
| src/paperbot/api/routes/init.py | Registers new repro_context route |
| src/paperbot/api/main.py | Includes P2C router |
| scripts/p2c_module1_benchmark.py | CLI benchmark runner |
| evals/fixtures/p2c/module1_gold.json | Benchmark fixture dataset |
| docs/PAPER_TO_CONTEXT_MODULE_DESIGN.md | P2C design doc |
| docs/P2C_MODULE_2_API_STORAGE.md | API/storage design doc |
| docs/P2C_MODULE_1_CORE_ENGINE.md | Core engine design doc |
| docs/P2C_MODULE1_BENCHMARK.md | Benchmark documentation |
| docs/AUTO_REVIEW_SETUP.md | Auto-review setup documentation |
| alembic/versions/b94c1a2be26e_add_p2c_repro_context_pack_tables.py | DB migration for P2C tables |
| CLAUDE.md | Updated dev/test instructions |
| .github/workflows/gemini-review.yml | Adds Gemini PR review workflow |
| .github/CODEOWNERS | Adds default/area codeowners |
| .coderabbit.yaml | Enables CodeRabbit auto review |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+18
| export type ObservationType = | ||
| | "method" | ||
| | "architecture" | ||
| | "hyperparameter" | ||
| | "metric" | ||
| | "environment" | ||
| | "limitation" | ||
|
|
||
| export type ObservationConcept = | ||
| | "core_method" | ||
| | "gotcha" | ||
| | "trade_off" | ||
| | "limitation" | ||
| | "baseline" | ||
| | "reproduction_hint" | ||
|
|
There was a problem hiding this comment.
ObservationType/ObservationConcept don’t cover values produced by the backend P2C pipeline (e.g. RoadmapPlanningStage emits observation type="roadmap", and concepts like "architecture", "environment", "hyperparameter"). This makes the frontend type contract inconsistent with the API payloads and will make it harder/unsafe to handle these values (filters/exhaustive handling, future UI logic). Align these unions with the backend model outputs (or relax them to string if the backend is intentionally open-ended).
Comment on lines
+77
to
+82
| if (res.status === 403) { | ||
| // Extract parent directory to add as allowed prefix | ||
| const parts = directory.replace(/\/+$/, "").split("/") | ||
| const parentDir = parts.length > 1 ? parts.slice(0, -1).join("/") : directory | ||
| setPendingAllowDir(parentDir) | ||
| return false |
There was a problem hiding this comment.
When handling a 403 from /api/runbook/project-dir/prepare, the parent directory is derived via split("/"). This breaks for Windows paths (\ separators) and also yields an empty string for paths like /foo (parent becomes ""), which prevents the allow-access prompt from rendering. Consider using a separator-agnostic parent extraction (handling root / and drive roots) before calling setPendingAllowDir.
Comment on lines
+154
to
+170
| @router.post("/runbook/allowed-dirs") | ||
| async def add_allowed_dir(body: AddAllowedDirRequest): | ||
| """Add a directory to the runtime-allowed prefixes list.""" | ||
| try: | ||
| resolved = Path(body.directory).expanduser().resolve() | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail="invalid directory path") | ||
|
|
||
| if not resolved.exists() or not resolved.is_dir(): | ||
| raise HTTPException(status_code=400, detail="directory does not exist or is not a directory") | ||
|
|
||
| _save_runtime_allowed_dir(resolved) | ||
| return { | ||
| "ok": True, | ||
| "directory": str(resolved), | ||
| "allowed_prefixes": [str(p) for p in _allowed_workdir_prefixes()], | ||
| } |
There was a problem hiding this comment.
POST /runbook/allowed-dirs lets any caller add an arbitrary existing directory (including / or the app user’s home) to the runtime allowlist. Since other runbook endpoints gate filesystem access on _allowed_workdir, this effectively allows privilege escalation to read/snapshot/write outside the intended workspace. This endpoint should be protected (authn/authz), feature-flagged to local/dev only, and/or restricted to subpaths of already-allowed prefixes.
Merges logging from fix/studio-output-dir-validation-p2c-design while keeping structured error dict for JSON-serializable SSE response. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
refactor(p2c)
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 160-166: The except blocks currently log full exception details
with Logger.error (using pack_id and exc) but append str(exc) into result_holder
and send _ERROR to the client; change result_holder.append({"kind": "server",
"message": str(exc)}) to a generic client-safe message such as
{"kind":"server","message":"Internal server error"} (keep the detailed exc in
Logger.error), and apply the same change to the other except block that uses
result_holder and queue (the one around lines 175-178); leave Logger.error(...)
as-is so full details remain in logs and only redact messages returned to
clients.
- Around line 24-31: Remove the duplicate imports of Logger and LogFiles: keep a
single import line that brings in Logger, LogFiles, and set_trace_id (and any
other needed names) and delete the earlier redundant import; locate the import
block containing GenerateContextRequest/P2CRequest and new_context_pack_id (from
paperbot.application.services.p2c.models) and the two Logger/LogFiles import
lines and consolidate them into one to avoid the Ruff F811 redefinition error.
- Around line 60-79: The async routes are directly calling synchronous _store
methods which will block the event loop; wrap every _store.* call in
run_in_threadpool and await it (e.g., replace direct _store.save(...) with await
run_in_threadpool(_store.save, ...) and similarly for _store.save_stage_result,
_store.update_status, _store.list_by_user, _store.get, _store.get_observation,
and _store.soft_delete). Apply this pattern inside _generate_stream (initial
save), inside the nested on_stage_complete callback (save_stage_result), in the
error handler and finalization paths (update_status), and in the other async
endpoints in this file (list_by_user, get, get_observation, soft_delete) so no
synchronous DB call runs on the event loop. Ensure any callback that becomes
async awaits the run_in_threadpool call or schedules it properly.
- Around line 224-362: Add integration tests covering the new P2C API routes and
SSE behavior: write tests that exercise generate_context_pack (POST /generate)
verifying SSE stream output from wrap_generator/_generate_stream,
list_context_packs (GET) with filters and pagination, get_context_pack (GET
/{pack_id}) and get_observation_detail (GET
/{pack_id}/observation/{observation_id}) for missing and present items,
create_repro_session (POST /{pack_id}/session) to validate session/runbook
payload and roadmap->initial_steps mapping, and delete_context_pack (DELETE
/{pack_id}) to confirm soft delete via _store.soft_delete and 404 handling; use
the existing test harness to mock or seed _store persistence, assert
Logger-invoked flows where relevant, and include an integration test that
connects to the StreamingResponse to consume SSE events and assert expected
event structure and termination.
In `@src/paperbot/infrastructure/stores/repro_context_store.py`:
- Around line 137-158: get_observation currently reads
ReproContextStageResultModel rows before checking whether the
ReproContextPackModel for pack_id is soft-deleted, allowing observations to be
returned for deleted packs; fix by querying the pack first
(session.get(ReproContextPackModel, pack_id)) and returning None if pack_row is
None or pack_row.deleted_at is not None, and only then proceed to load/select
ReproContextStageResultModel rows to search observations for the given
observation_id.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/paperbot/api/routes/repro_context.pysrc/paperbot/infrastructure/stores/repro_context_store.py
Comment on lines
+160
to
+166
| except Exception as exc: # noqa: BLE001 | ||
| Logger.error( | ||
| f"[M2] generation_failed pack_id={pack_id} error={exc}", | ||
| file=LogFiles.ERROR, | ||
| ) | ||
| result_holder.append({"kind": "server", "message": str(exc)}) | ||
| await queue.put(_ERROR) |
There was a problem hiding this comment.
Do not stream raw server exception text to clients.
Returning str(exc) in server error events may leak internals. Keep full details in logs, return a generic message to clients.
🔧 Suggested fix
except Exception as exc: # noqa: BLE001
Logger.error(
f"[M2] generation_failed pack_id={pack_id} error={exc}",
file=LogFiles.ERROR,
)
- result_holder.append({"kind": "server", "message": str(exc)})
+ result_holder.append({"kind": "server", "message": "Internal server error"})
await queue.put(_ERROR)Also applies to: 175-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/repro_context.py` around lines 160 - 166, The except
blocks currently log full exception details with Logger.error (using pack_id and
exc) but append str(exc) into result_holder and send _ERROR to the client;
change result_holder.append({"kind": "server", "message": str(exc)}) to a
generic client-safe message such as {"kind":"server","message":"Internal server
error"} (keep the detailed exc in Logger.error), and apply the same change to
the other except block that uses result_holder and queue (the one around lines
175-178); leave Logger.error(...) as-is so full details remain in logs and only
redact messages returned to clients.
Comment on lines
+224
to
+362
| @router.post("/generate") | ||
| async def generate_context_pack(request: GenerateContextPackRequest): | ||
| """Generate a P2C context pack for the given paper. Returns SSE stream.""" | ||
| trace_id = set_trace_id() | ||
| Logger.info( | ||
| f"[M2] generate_request trace_id={trace_id} paper_id={request.paper_id} user_id={request.user_id}", | ||
| file=LogFiles.API, | ||
| ) | ||
| return StreamingResponse( | ||
| wrap_generator( | ||
| _generate_stream(request), | ||
| workflow="p2c_generate", | ||
| ), | ||
| media_type="text/event-stream", | ||
| headers={"Cache-Control": "no-cache", "Connection": "keep-alive"}, | ||
| ) | ||
|
|
||
|
|
||
| # ------------------------------------------------------------------ # | ||
| # GET / (list) # | ||
| # ------------------------------------------------------------------ # | ||
|
|
||
| @router.get("") | ||
| async def list_context_packs( | ||
| user_id: str = "default", | ||
| paper_id: Optional[str] = None, | ||
| project_id: Optional[str] = None, | ||
| limit: int = 20, | ||
| offset: int = 0, | ||
| ): | ||
| """List context packs for a user, with optional filters.""" | ||
| set_trace_id() | ||
| Logger.info( | ||
| f"[M2] list_packs user_id={user_id} paper_id={paper_id} project_id={project_id} limit={limit} offset={offset}", | ||
| file=LogFiles.API, | ||
| ) | ||
| items, total = _store.list_by_user( | ||
| user_id=user_id, | ||
| paper_id=paper_id, | ||
| project_id=project_id, | ||
| limit=limit, | ||
| offset=offset, | ||
| ) | ||
| return {"items": items, "total": total} | ||
|
|
||
|
|
||
| # ------------------------------------------------------------------ # | ||
| # GET /{pack_id} # | ||
| # ------------------------------------------------------------------ # | ||
|
|
||
| @router.get("/{pack_id}") | ||
| async def get_context_pack(pack_id: str): | ||
| """Return the full context pack detail.""" | ||
| set_trace_id() | ||
| Logger.info(f"[M2] get_pack pack_id={pack_id}", file=LogFiles.API) | ||
| pack = _store.get(pack_id) | ||
| if pack is None: | ||
| Logger.warning(f"[M2] pack_not_found pack_id={pack_id}", file=LogFiles.API) | ||
| raise HTTPException(status_code=404, detail="Context pack not found.") | ||
| return pack | ||
|
|
||
|
|
||
| @router.get("/{pack_id}/observation/{observation_id}") | ||
| async def get_observation_detail(pack_id: str, observation_id: str): | ||
| """Return a single observation detail by ID.""" | ||
| set_trace_id() | ||
| Logger.info( | ||
| f"[M2] get_observation pack_id={pack_id} observation_id={observation_id}", | ||
| file=LogFiles.API, | ||
| ) | ||
| observation = _store.get_observation(pack_id, observation_id) | ||
| if observation is None: | ||
| Logger.warning( | ||
| f"[M2] observation_not_found pack_id={pack_id} observation_id={observation_id}", | ||
| file=LogFiles.API, | ||
| ) | ||
| raise HTTPException(status_code=404, detail="Observation not found.") | ||
| return observation | ||
|
|
||
|
|
||
| # ------------------------------------------------------------------ # | ||
| # POST /{pack_id}/session # | ||
| # ------------------------------------------------------------------ # | ||
|
|
||
| @router.post("/{pack_id}/session") | ||
| async def create_repro_session(pack_id: str, request: CreateSessionRequest): | ||
| """ | ||
| Convert a context pack into a runbook session. | ||
|
|
||
| TODO: integrate with existing runbook creation once Module 1 is wired. | ||
| """ | ||
| pack = _store.get(pack_id) | ||
| if pack is None: | ||
| Logger.warning(f"[M2] session_pack_not_found pack_id={pack_id}", file=LogFiles.API) | ||
| raise HTTPException(status_code=404, detail="Context pack not found.") | ||
| Logger.info( | ||
| f"[M2] create_session pack_id={pack_id} executor={request.executor_preference}", | ||
| file=LogFiles.API, | ||
| ) | ||
|
|
||
| session_id = f"sess_{uuid.uuid4().hex[:16]}" | ||
| runbook_id = f"rb_{uuid.uuid4().hex[:12]}" | ||
|
|
||
| roadmap = pack.get("task_roadmap", []) | ||
| initial_steps = [ | ||
| { | ||
| "step_id": f"S{i + 1}", | ||
| "title": cp.get("title", f"Step {i + 1}"), | ||
| "command": "", | ||
| "status": "pending", | ||
| } | ||
| for i, cp in enumerate(roadmap) | ||
| ] or [{"step_id": "S1", "title": "Setup environment", "command": "", "status": "pending"}] | ||
|
|
||
| return { | ||
| "session_id": session_id, | ||
| "runbook_id": runbook_id, | ||
| "initial_steps": initial_steps, | ||
| "initial_prompt": ( | ||
| f"Based on the reproduction context pack for paper {pack.get('paper_id', '')}, " | ||
| "implement the code step by step following the roadmap." | ||
| ), | ||
| } | ||
|
|
||
|
|
||
| # ------------------------------------------------------------------ # | ||
| # DELETE /{pack_id} # | ||
| # ------------------------------------------------------------------ # | ||
|
|
||
| @router.delete("/{pack_id}") | ||
| async def delete_context_pack(pack_id: str): | ||
| """Soft-delete a context pack.""" | ||
| set_trace_id() | ||
| Logger.info(f"[M2] delete_pack pack_id={pack_id}", file=LogFiles.API) | ||
| deleted = _store.soft_delete(pack_id) | ||
| if not deleted: | ||
| Logger.warning(f"[M2] delete_pack_not_found pack_id={pack_id}", file=LogFiles.API) | ||
| raise HTTPException(status_code=404, detail="Context pack not found.") | ||
| return {"status": "deleted", "context_pack_id": pack_id} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find test files structure
echo "=== Test directory structure ==="
fd -t d tests
echo
echo "=== Python test files related to repro_context ==="
fd -t f -e py tests | head -20
echo
echo "=== Search for repro_context test coverage ==="
rg -l "repro_context|context_pack|generate_context_pack" tests --type py
echo
echo "=== Check if test file exists for the route ==="
fd -t f "test_repro_context.py" tests
echo
echo "=== Check imports or references to repro_context route functions ==="
rg -n "from.*repro_context|import.*repro_context|generate_context_pack|list_context_packs|get_context_pack|create_repro_session|delete_context_pack" tests --type pyRepository: jerry609/PaperBot
Length of output: 482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for integration test directory ==="
fd -t d integration tests 2>/dev/null || echo "No integration directory found"
echo
echo "=== Search for any API route tests ==="
fd -t f -e py tests | xargs grep -l "router\|@router\|APIRouter" 2>/dev/null || echo "No router tests found"
echo
echo "=== Check test_p2c_module1.py for route testing patterns ==="
if [ -f tests/unit/test_p2c_module1.py ]; then
head -50 tests/unit/test_p2c_module1.py
fi
echo
echo "=== Search for StreamingResponse or SSE tests ==="
rg -n "StreamingResponse|text/event-stream" tests --type py
echo
echo "=== Check if src/paperbot/api/routes/repro_context.py was newly added ==="
git log --oneline --follow src/paperbot/api/routes/repro_context.py 2>/dev/null | head -5 || echo "Git history check skipped"
echo
echo "=== List all files in tests/unit ==="
ls -la tests/unit/ 2>/dev/null | head -20Repository: jerry609/PaperBot
Length of output: 1690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List integration test files ==="
ls -la tests/integration/
echo
echo "=== Check for any route or API endpoint tests in integration ==="
find tests/integration -type f -name "*.py" -exec grep -l "router\|APIRouter\|/repro\|context" {} \;
echo
echo "=== Check one integration test to see the pattern ==="
fd -t f -e py tests/integration | head -1 | xargs cat | head -60
echo
echo "=== Verify the repro_context.py file exists and check imports ==="
head -40 src/paperbot/api/routes/repro_context.pyRepository: jerry609/PaperBot
Length of output: 1820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check test_research_track_routes.py ==="
cat tests/integration/test_research_track_routes.py
echo
echo "=== Check test_repro_deepcode.py for any route testing ==="
head -100 tests/integration/test_repro_deepcode.py
echo
echo "=== Search across all test files for repro_context routes ==="
rg -n "generate_context_pack|list_context_packs|get_context_pack|create_repro_session|delete_context_pack" tests --type py -A 2Repository: jerry609/PaperBot
Length of output: 9528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if repro_context.py route file exists ==="
ls -la src/paperbot/api/routes/repro_context.py
echo
echo "=== Check api/main.py to see if route is registered ==="
grep -n "repro_context\|/repro/context" src/paperbot/api/main.py
echo
echo "=== Examine the imports and setup in repro_context.py ==="
head -80 src/paperbot/api/routes/repro_context.py
echo
echo "=== Check what the _generate_stream function does ==="
rg -n "_generate_stream" src/paperbot/api/routes/repro_context.py -A 5Repository: jerry609/PaperBot
Length of output: 3988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for any test files that might test repro_context routes ==="
find tests -type f -name "*.py" | xargs grep -l "repro/context\|generate_context_pack\|list_context_packs\|get_context_pack" 2>/dev/null || echo "No test files found"
echo
echo "=== Search the exact prefix path in tests ==="
rg -n "/api/research/repro/context" tests --type py
echo
echo "=== Check integration test pattern - does one exist for repro_context? ==="
ls -la tests/integration/test_repro_context*.py 2>/dev/null || echo "No integration test file for repro_context routes"
echo
echo "=== Check if git shows this as a new file ==="
git diff --name-only HEAD 2>/dev/null | grep repro_context || echo "Check git log instead"
echo
echo "=== Verify the entire repro_context.py file and check _store usage ==="
wc -l src/paperbot/api/routes/repro_context.py
tail -100 src/paperbot/api/routes/repro_context.pyRepository: jerry609/PaperBot
Length of output: 197
Add integration tests for the new P2C API endpoints and SSE streaming behavior.
This change introduces five new API routes with persistence operations (save, get, list, delete) and SSE streaming, but no corresponding tests are present. Per coding guidelines, all behavior changes in {src,web/src}/**/*.{py,ts,tsx} must include tests. Additionally, integration tests should be added for the new API endpoints (POST /generate, GET, POST /{pack_id}/session, DELETE /{pack_id}) and the streaming response pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/repro_context.py` around lines 224 - 362, Add
integration tests covering the new P2C API routes and SSE behavior: write tests
that exercise generate_context_pack (POST /generate) verifying SSE stream output
from wrap_generator/_generate_stream, list_context_packs (GET) with filters and
pagination, get_context_pack (GET /{pack_id}) and get_observation_detail (GET
/{pack_id}/observation/{observation_id}) for missing and present items,
create_repro_session (POST /{pack_id}/session) to validate session/runbook
payload and roadmap->initial_steps mapping, and delete_context_pack (DELETE
/{pack_id}) to confirm soft delete via _store.soft_delete and 404 handling; use
the existing test harness to mock or seed _store persistence, assert
Logger-invoked flows where relevant, and include an integration test that
connects to the StreamingResponse to consume SSE events and assert expected
event structure and termination.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/integration/test_repro_context_routes.py (1)
215-269: Good: SSE streaming test properly mocks the orchestrator.The
_FakeOrchestratoravoids real LLM calls while exercising the full SSE flow. The assertions verify event types and completion status.Consider prefixing the unused
storevariable with underscore per Ruff suggestion:💡 Minor fix
- client, store = client_and_store + client, _store = client_and_store🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_repro_context_routes.py` around lines 215 - 269, The test function test_generate_stream_returns_sse receives an unused fixture variable named store; rename it to _store in the function signature (test_generate_stream_returns_sse(client_and_store, _store) or if unpacking, client, _store = client_and_store) to satisfy Ruff's unused-variable convention and keep the rest of the test (including _FakeOrchestrator, monkeypatching repro_context_module.ExtractionOrchestrator, and assertions) unchanged.src/paperbot/application/services/p2c/input_pipeline.py (2)
34-44: Mutable class attribute is safe here but consider typing annotation.
_DEFAULT_FIELDSis a mutable list used as a class attribute. While it's only read (never mutated), adding aClassVartype hint would make the intent clearer and silence RUF012.💡 Suggested improvement
+from typing import ClassVar + class SemanticScholarAdapter(PaperInputAdapter): name = "semantic_scholar" - _DEFAULT_FIELDS = [ + _DEFAULT_FIELDS: ClassVar[List[str]] = [ "paperId", "title", ... ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/input_pipeline.py` around lines 34 - 44, The class attribute _DEFAULT_FIELDS on SemanticScholarAdapter is a mutable list; add an explicit type hint using typing.ClassVar to mark it as an immutable class-level constant (e.g., ClassVar[List[str]]), import ClassVar and List from typing, and annotate _DEFAULT_FIELDS accordingly in the SemanticScholarAdapter class so linters (RUF012) know it is a class variable rather than an instance attribute.
509-527: Paper type classification uses simple heuristics — consider adding "dataset" type.The classifier covers survey, theoretical, benchmark, system, and experimental types. If dataset papers are common in the domain, consider adding detection for terms like "dataset", "corpus", "collection".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/application/services/p2c/input_pipeline.py` around lines 509 - 527, The PaperTypeClassifier.classify currently only detects SURVEY, THEORETICAL, BENCHMARK, SYSTEM, and EXPERIMENTAL using keyword heuristics; add detection for dataset papers by adding a new conditional in classify that checks text (built from normalized.paper.title, normalized.abstract, normalized.sections.get("method", "")) for tokens like "dataset", "corpus", "collection", "annotations", "benchmark-dataset" and returns PaperType.DATASET before falling back to EXPERIMENTAL; also ensure the PaperType enum includes DATASET if it's not already present.src/paperbot/api/routes/repro_context.py (1)
311-349: TODO comment indicates incomplete integration.The
TODO: integrate with existing runbook creation once Module 1 is wired.suggests this is a placeholder. Consider creating a tracking issue if this won't be addressed in this PR.Would you like me to open an issue to track the runbook integration work mentioned in the TODO?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/repro_context.py` around lines 311 - 349, The TODO in create_repro_session indicates incomplete integration with runbook creation; create a proper tracking issue (or ticket) describing the required work to "integrate with existing runbook creation once Module 1 is wired" and update the TODO in the create_repro_session function to reference that issue ID (or a short URL) so future work is discoverable; if you create the issue, include its ID in the comment and optionally add a short one-line todo like "// TODO (ISSUE-1234): integrate with runbook creation when Module 1 is available" to replace the current bare TODO.src/paperbot/infrastructure/stores/repro_context_store.py (2)
64-88: Consider returning a boolean to indicate if the update was applied.Currently,
update_statussilently returns if the pack doesn't exist. The caller has no way to know if the update succeeded. Consider returningboolto indicate success.💡 Suggested signature change
def update_status( self, pack_id: str, *, status: str, pack_data: Optional[Dict[str, Any]] = None, confidence_overall: Optional[float] = None, warning_count: Optional[int] = None, objective: Optional[str] = None, - ) -> None: + ) -> bool: with self._provider.session() as session: row = session.get(ReproContextPackModel, pack_id) if row is None: - return + return False # ... existing update logic ... session.commit() + return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/stores/repro_context_store.py` around lines 64 - 88, The update_status method currently returns None and silently exits when the pack doesn't exist; change its signature in ReproContextStore.update_status to return bool and update its type hints, then return False when row is None and True after successful commit (ensure to still perform updates to row.status, timestamps and optional fields before session.commit()). Also update any callers of update_status to handle the boolean result accordingly.
214-227: Potential key collision when merging pack into base dict.
base.update(pack)could overwrite keys likestatusorconfidence_overallif they exist in the stored pack JSON. If this is intentional (pack takes precedence), consider documenting it; otherwise, be selective about which keys to merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/stores/repro_context_store.py` around lines 214 - 227, In _row_to_full_dict, base.update(pack) can unintentionally overwrite top-level keys (e.g., status, confidence_overall); change this to a selective merge: enumerate allowed pack keys or skip keys that already exist on base before updating (e.g., for k,v in pack.items(): if k not in base: base[k]=v), or explicitly map/rename pack fields into distinct top-level names so pack contents do not clobber existing fields; update the function _row_to_full_dict (and adjust any callers if you rename/move fields) and add a short comment explaining the precedence rule you chose.src/paperbot/api/routes/runbook.py (3)
144-148: Chain the exception for better error context.When re-raising an
HTTPExceptionfrom within anexceptblock, useraise ... from exc(orfrom Noneif suppressing the original) to preserve the exception chain and satisfy B904.🔧 Proposed fix
try: resolved = requested.resolve(strict=False) - except Exception: - raise HTTPException(status_code=400, detail="invalid project_dir") + except Exception as exc: + raise HTTPException(status_code=400, detail="invalid project_dir") from exc🤖 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 144 - 148, The except block around resolving the Path for requested = Path(body.project_dir).expanduser() currently raises HTTPException without chaining; update the except to capture the exception (e.g., except Exception as exc:) and re-raise the HTTPException using "raise HTTPException(status_code=400, detail='invalid project_dir') from exc" so the original error chain is preserved (refer to requested, resolved, Path and HTTPException in runbook.py).
85-110: Consider logging the exception for easier debugging.The
try-except-passat lines 89-90 silently swallows errors when resolvingPath.cwd(). While this is defensive, logging the exception would help diagnose issues in environments wherecwdis unavailable.💡 Suggested improvement
try: prefixes.append(Path.cwd().resolve()) - except Exception: - pass + except Exception as exc: + Logger.warning(f"[runbook] Could not resolve cwd: {exc}", file=LogFiles.ERROR)🤖 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 85 - 110, The silent except in _allowed_workdir_prefixes hides failures from Path.cwd(). Change it to log the exception: import logging if needed, get a module logger (e.g. logger = logging.getLogger(__name__)), and in the except block replace pass with logger.exception("Failed to resolve current working directory in _allowed_workdir_prefixes") or logger.warning(..., exc_info=True) so the error and stacktrace are recorded while still continuing execution.
176-215: Security hardening is in place — verify the denied paths list is comprehensive.The
_DENIED_PATHSblocklist and_runtime_allowlist_mutation_enabled()guard provide good defense-in-depth. However, consider whether paths like/homeor/mntshould also be blocked, depending on deployment context.Also, chain the exception at line 199:
🔧 Proposed fix for exception chaining
try: resolved = Path(body.directory).expanduser().resolve() - except Exception: - raise HTTPException(status_code=400, detail="invalid directory path") + except Exception as exc: + raise HTTPException(status_code=400, detail="invalid directory path") from exc🤖 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 176 - 215, Update the denied-paths and exception handling in add_allowed_dir: extend the _DENIED_PATHS frozenset to include additional sensitive mount points such as "/home" and "/mnt" (and any other deployment-specific mounts you need blocked) and modify the error handling in add_allowed_dir by changing "except Exception:" to "except Exception as e:" and re-raising the HTTPException with exception chaining (raise HTTPException(status_code=400, detail="invalid directory path") from e) so the original error is preserved for debugging; the relevant symbols are _DENIED_PATHS and the add_allowed_dir function (which calls _runtime_allowlist_mutation_enabled and _save_runtime_allowed_dir).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Line 169: The background coroutine started with asyncio.create_task(_run())
must be assigned to a persistent reference to avoid silent cancellation; change
the call to store the Task (e.g., background_task = asyncio.create_task(_run())
or attach it to a long-lived container like app.state.repro_task or a
module-level _repro_task) and register a done callback (task.add_done_callback)
to log or re-raise exceptions from _run to ensure errors are surfaced and the
task lifecycle is managed.
---
Nitpick comments:
In `@src/paperbot/api/routes/repro_context.py`:
- Around line 311-349: The TODO in create_repro_session indicates incomplete
integration with runbook creation; create a proper tracking issue (or ticket)
describing the required work to "integrate with existing runbook creation once
Module 1 is wired" and update the TODO in the create_repro_session function to
reference that issue ID (or a short URL) so future work is discoverable; if you
create the issue, include its ID in the comment and optionally add a short
one-line todo like "// TODO (ISSUE-1234): integrate with runbook creation when
Module 1 is available" to replace the current bare TODO.
In `@src/paperbot/api/routes/runbook.py`:
- Around line 144-148: The except block around resolving the Path for requested
= Path(body.project_dir).expanduser() currently raises HTTPException without
chaining; update the except to capture the exception (e.g., except Exception as
exc:) and re-raise the HTTPException using "raise HTTPException(status_code=400,
detail='invalid project_dir') from exc" so the original error chain is preserved
(refer to requested, resolved, Path and HTTPException in runbook.py).
- Around line 85-110: The silent except in _allowed_workdir_prefixes hides
failures from Path.cwd(). Change it to log the exception: import logging if
needed, get a module logger (e.g. logger = logging.getLogger(__name__)), and in
the except block replace pass with logger.exception("Failed to resolve current
working directory in _allowed_workdir_prefixes") or logger.warning(...,
exc_info=True) so the error and stacktrace are recorded while still continuing
execution.
- Around line 176-215: Update the denied-paths and exception handling in
add_allowed_dir: extend the _DENIED_PATHS frozenset to include additional
sensitive mount points such as "/home" and "/mnt" (and any other
deployment-specific mounts you need blocked) and modify the error handling in
add_allowed_dir by changing "except Exception:" to "except Exception as e:" and
re-raising the HTTPException with exception chaining (raise
HTTPException(status_code=400, detail="invalid directory path") from e) so the
original error is preserved for debugging; the relevant symbols are
_DENIED_PATHS and the add_allowed_dir function (which calls
_runtime_allowlist_mutation_enabled and _save_runtime_allowed_dir).
In `@src/paperbot/application/services/p2c/input_pipeline.py`:
- Around line 34-44: The class attribute _DEFAULT_FIELDS on
SemanticScholarAdapter is a mutable list; add an explicit type hint using
typing.ClassVar to mark it as an immutable class-level constant (e.g.,
ClassVar[List[str]]), import ClassVar and List from typing, and annotate
_DEFAULT_FIELDS accordingly in the SemanticScholarAdapter class so linters
(RUF012) know it is a class variable rather than an instance attribute.
- Around line 509-527: The PaperTypeClassifier.classify currently only detects
SURVEY, THEORETICAL, BENCHMARK, SYSTEM, and EXPERIMENTAL using keyword
heuristics; add detection for dataset papers by adding a new conditional in
classify that checks text (built from normalized.paper.title,
normalized.abstract, normalized.sections.get("method", "")) for tokens like
"dataset", "corpus", "collection", "annotations", "benchmark-dataset" and
returns PaperType.DATASET before falling back to EXPERIMENTAL; also ensure the
PaperType enum includes DATASET if it's not already present.
In `@src/paperbot/infrastructure/stores/repro_context_store.py`:
- Around line 64-88: The update_status method currently returns None and
silently exits when the pack doesn't exist; change its signature in
ReproContextStore.update_status to return bool and update its type hints, then
return False when row is None and True after successful commit (ensure to still
perform updates to row.status, timestamps and optional fields before
session.commit()). Also update any callers of update_status to handle the
boolean result accordingly.
- Around line 214-227: In _row_to_full_dict, base.update(pack) can
unintentionally overwrite top-level keys (e.g., status, confidence_overall);
change this to a selective merge: enumerate allowed pack keys or skip keys that
already exist on base before updating (e.g., for k,v in pack.items(): if k not
in base: base[k]=v), or explicitly map/rename pack fields into distinct
top-level names so pack contents do not clobber existing fields; update the
function _row_to_full_dict (and adjust any callers if you rename/move fields)
and add a short comment explaining the precedence rule you chose.
In `@tests/integration/test_repro_context_routes.py`:
- Around line 215-269: The test function test_generate_stream_returns_sse
receives an unused fixture variable named store; rename it to _store in the
function signature (test_generate_stream_returns_sse(client_and_store, _store)
or if unpacking, client, _store = client_and_store) to satisfy Ruff's
unused-variable convention and keep the rest of the test (including
_FakeOrchestrator, monkeypatching repro_context_module.ExtractionOrchestrator,
and assertions) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/paperbot/api/routes/repro_context.pysrc/paperbot/api/routes/runbook.pysrc/paperbot/application/services/p2c/benchmark.pysrc/paperbot/application/services/p2c/input_pipeline.pysrc/paperbot/infrastructure/stores/repro_context_store.pytests/integration/test_repro_context_routes.pyweb/src/app/api/research/repro/context/[packId]/observation/[observationId]/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/app/api/research/repro/context/[packId]/observation/[observationId]/route.ts
- src/paperbot/application/services/p2c/benchmark.py
| result_holder.append({"kind": "server", "message": "Internal error during context pack generation."}) | ||
| await queue.put(_ERROR) | ||
|
|
||
| asyncio.create_task(_run()) |
There was a problem hiding this comment.
Store a reference to the background task to prevent silent cancellation.
asyncio.create_task(_run()) without storing the reference means the task could be garbage-collected if no other references exist. Store it to ensure proper lifecycle management and to catch exceptions.
🔧 Proposed fix
- asyncio.create_task(_run())
+ task = asyncio.create_task(_run())
+ # Keep reference to prevent GC; exceptions are handled inside _run()🧰 Tools
🪛 Ruff (0.15.2)
[warning] 169-169: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/repro_context.py` at line 169, The background
coroutine started with asyncio.create_task(_run()) must be assigned to a
persistent reference to avoid silent cancellation; change the call to store the
Task (e.g., background_task = asyncio.create_task(_run()) or attach it to a
long-lived container like app.state.repro_task or a module-level _repro_task)
and register a done callback (task.add_done_callback) to log or re-raise
exceptions from _run to ensure errors are surfaced and the task lifecycle is
managed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/app/studio/page.tsx (1)
72-147:⚠️ Potential issue | 🟠 MajorPlease add tests for the new URL-param workflows.
This PR changes runtime behavior for
paper_id,title/abstract,context_pack_id,generate, and URL cleanup, but there are no corresponding test updates in the provided changes.As per coding guidelines,
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/studio/page.tsx` around lines 72 - 147, Add unit/integration tests for the studio URL-param useEffect workflows: write tests that mock searchParams and verify (1) when paper_id matches an existing paper the selectPaper flow is invoked (selectPaper called with paperId) and router.replace cleans the URL, (2) when paper_id is absent but title+abstract are present addPaper is called, (3) when paper_id is present but unknown and title is provided addPaper is called, (4) when context_pack_id is present the fetch to /api/research/repro/context/:id is issued and on success setContextPack is called and on error setContextPackError is set, (5) when generate=true with a valid paper_id generate is called and with no paper_id setContextPackError is set, and (6) hasProcessedParams prevents reprocessing (re-running the effect does not re-trigger actions); mock fetch, router.replace, and the action props (addPaper, selectPaper, generate, setContextPack*, clearGenerationProgress) and assert calls and state changes accordingly.
🧹 Nitpick comments (1)
web/src/app/studio/page.tsx (1)
53-56: Consider deferring URL parameter processing until papers are loaded from storage.With React 19's automatic effect batching, the race condition is unlikely to manifest, but the current code lacks explicit synchronization: if the second effect runs before the first completes,
papers.find()on line 86 evaluates against an empty array. Whenpaper_idis supplied withouttitle, no fallback occurs, and line 95 still setsshouldCleanUrl = true, consuming the URL parameter permanently viahasProcessedParams.current.No tests exist documenting the expected deep-link behavior. This is a defensive improvement rather than a critical issue.
💡 Suggested approach
-import { useEffect, Suspense, useRef } from "react" +import { useEffect, Suspense, useRef, useState } from "react" function StudioContent() { + const [papersReady, setPapersReady] = useState(false) // Load papers from localStorage on mount useEffect(() => { - loadPapers() + void Promise.resolve(loadPapers()).finally(() => setPapersReady(true)) // eslint-disable-next-line react-hooks/exhaustive-deps }, []) // Handle URL params from the Papers detail entry point and context pack deep links useEffect(() => { + if (!papersReady) return if (hasProcessedParams.current) return ... }, [ ... + papersReady, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/studio/page.tsx` around lines 53 - 56, The URL-param processing effect runs before papers finish loading, causing papers.find(...) to run on an empty array and incorrectly consume params via hasProcessedParams.current; modify the logic in page.tsx so parameter handling waits until loadPapers() has completed (e.g., track a loaded flag or have loadPapers return a promise and set isPapersLoaded state), then run the effect that reads paper_id/title only when isPapersLoaded is true and papers.length > 0; also adjust the branch that handles paper_id without title (the block that uses papers.find and sets shouldCleanUrl) to only mark hasProcessedParams.current = true and shouldCleanUrl = true if a matching paper was actually found or if all fallback logic completed, ensuring params are not consumed prematurely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/app/studio/page.tsx`:
- Around line 113-117: The code currently ignores invalid payloads because
normalizePack(payload) returning null skips setting state; update the promise
handler in the .then branch to detect when pack is null/invalid and call
setContextPackError with a descriptive message (e.g., "Invalid context pack
payload" plus payload details or type) instead of doing nothing, and only call
setContextPack when pack is non-null; reference normalizePack, setContextPack,
and setContextPackError to implement this explicit error branch so users see why
nothing loaded.
---
Outside diff comments:
In `@web/src/app/studio/page.tsx`:
- Around line 72-147: Add unit/integration tests for the studio URL-param
useEffect workflows: write tests that mock searchParams and verify (1) when
paper_id matches an existing paper the selectPaper flow is invoked (selectPaper
called with paperId) and router.replace cleans the URL, (2) when paper_id is
absent but title+abstract are present addPaper is called, (3) when paper_id is
present but unknown and title is provided addPaper is called, (4) when
context_pack_id is present the fetch to /api/research/repro/context/:id is
issued and on success setContextPack is called and on error setContextPackError
is set, (5) when generate=true with a valid paper_id generate is called and with
no paper_id setContextPackError is set, and (6) hasProcessedParams prevents
reprocessing (re-running the effect does not re-trigger actions); mock fetch,
router.replace, and the action props (addPaper, selectPaper, generate,
setContextPack*, clearGenerationProgress) and assert calls and state changes
accordingly.
---
Nitpick comments:
In `@web/src/app/studio/page.tsx`:
- Around line 53-56: The URL-param processing effect runs before papers finish
loading, causing papers.find(...) to run on an empty array and incorrectly
consume params via hasProcessedParams.current; modify the logic in page.tsx so
parameter handling waits until loadPapers() has completed (e.g., track a loaded
flag or have loadPapers return a promise and set isPapersLoaded state), then run
the effect that reads paper_id/title only when isPapersLoaded is true and
papers.length > 0; also adjust the branch that handles paper_id without title
(the block that uses papers.find and sets shouldCleanUrl) to only mark
hasProcessedParams.current = true and shouldCleanUrl = true if a matching paper
was actually found or if all fallback logic completed, ensuring params are not
consumed prematurely.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AI4Sweb/src/app/api/research/repro/context/[packId]/route.tsweb/src/app/api/research/repro/context/[packId]/session/route.tsweb/src/app/studio/page.tsxweb/src/hooks/useContextPackGeneration.ts
💤 Files with no reviewable changes (1)
- AI4S
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/hooks/useContextPackGeneration.ts
Comment on lines
+113
to
+117
| .then((payload) => { | ||
| const pack = normalizePack(payload) | ||
| if (pack) setContextPack(pack) | ||
| }) | ||
| .catch((err) => setContextPackError(err instanceof Error ? err.message : String(err))) |
There was a problem hiding this comment.
Invalid context-pack payload is silently ignored.
If normalizePack(payload) returns null, no error is surfaced. Add an explicit error branch so users know why nothing loaded.
💡 Suggested fix
.then((payload) => {
const pack = normalizePack(payload)
- if (pack) setContextPack(pack)
+ if (pack) {
+ setContextPack(pack)
+ } else {
+ setContextPackError("Invalid context pack payload.")
+ }
})📝 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
| .then((payload) => { | |
| const pack = normalizePack(payload) | |
| if (pack) setContextPack(pack) | |
| }) | |
| .catch((err) => setContextPackError(err instanceof Error ? err.message : String(err))) | |
| .then((payload) => { | |
| const pack = normalizePack(payload) | |
| if (pack) { | |
| setContextPack(pack) | |
| } else { | |
| setContextPackError("Invalid context pack payload.") | |
| } | |
| }) | |
| .catch((err) => setContextPackError(err instanceof Error ? err.message : String(err))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/studio/page.tsx` around lines 113 - 117, The code currently
ignores invalid payloads because normalizePack(payload) returning null skips
setting state; update the promise handler in the .then branch to detect when
pack is null/invalid and call setContextPackError with a descriptive message
(e.g., "Invalid context pack payload" plus payload details or type) instead of
doing nothing, and only call setContextPack when pack is non-null; reference
normalizePack, setContextPack, and setContextPackError to implement this
explicit error branch so users see why nothing loaded.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| If the path is under allowed prefixes and does not exist yet, this endpoint | ||
| can create it to make Studio directory selection smoother. | ||
| """ | ||
| requested = Path(body.project_dir).expanduser() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 5 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| ) | ||
|
|
||
| try: | ||
| resolved = Path(body.directory).expanduser().resolve() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix uncontrolled data in path expressions, normalize the user‑provided path and strictly validate it against an allowlist policy before using it in any security‑sensitive way. For “add to allowlist” style endpoints, you usually want to ensure the directory is a subdirectory of some pre‑configured safe roots, and reject or coerce anything else. Normalization (resolve / normpath) must be done before validation, and validation must go beyond a minimal denylist of a few top‑level directories.
For this specific function, the safest fix that preserves existing behavior while closing the vulnerability is to restrict which directories can be added at runtime. The rest of this module already uses _allowed_workdir and _allowed_workdir_prefixes() (as seen in prepare_project_dir) to limit where projects can live. We should re‑use the same policy in add_allowed_dir by only allowing new prefixes that themselves pass _allowed_workdir and that are not under obviously dangerous roots. Concretely:
- Keep the existing
expanduser().resolve()normalization and existence checks. - Add a new helper
_is_directory_safe_for_allowlist(path: Path) -> boolthat:- Rejects any directory equal to, or a parent of, the current working directory of the application root that you don’t want broadened; and most importantly
- Requires that the path itself is within the same allowed workdir prefixes as used by
prepare_project_dir(i.e., call_allowed_workdir(resolved)). - Optionally re‑use
_DENIED_PATHSto reject not just exact matches but also subdirectories, if you want stronger protection.
- In
add_allowed_dir, after resolving the path and checking it exists and is a directory, call this helper and return 403 if it fails.
This way, even though body.directory is user‑controlled, the only directories that can be added to the runtime allowlist are ones that are already within the configured safe areas for runbooks. That closes the privilege‑escalation vector without changing how valid project directories behave. Implementation‑wise, all changes are confined to src/paperbot/api/routes/runbook.py: add the helper function near the other helpers (_runtime_allowlist_mutation_enabled, _DENIED_PATHS, etc.), and update add_allowed_dir to call it. No new imports are required; we can use Path and existing helper functions already imported in the file.
Suggested changeset
1
src/paperbot/api/routes/runbook.py
| @@ -184,6 +184,30 @@ | ||
| }) | ||
|
|
||
|
|
||
| def _is_directory_safe_for_allowlist(path: Path) -> bool: | ||
| """ | ||
| Return True if the given resolved directory path is safe to add to the | ||
| runtime allowlist. | ||
|
|
||
| The path must: | ||
| - Not be one of the explicitly denied system directories. | ||
| - Be within the configured allowed workdir prefixes, so that users cannot | ||
| expand access to arbitrary locations on the filesystem. | ||
| """ | ||
| # Reject paths that are exactly in the deny list. | ||
| if str(path) in _DENIED_PATHS: | ||
| return False | ||
|
|
||
| # Reuse the same policy used for project workdirs. | ||
| # This ensures that adding a directory at runtime cannot broaden access | ||
| # beyond what _allowed_workdir() already considers acceptable. | ||
| try: | ||
| return _allowed_workdir(path) | ||
| except Exception: | ||
| # If for any reason the policy check fails, be safe and reject. | ||
| return False | ||
|
|
||
|
|
||
| @router.post("/runbook/allowed-dirs") | ||
| async def add_allowed_dir(body: AddAllowedDirRequest): | ||
| """Add a directory to the runtime-allowed prefixes list.""" | ||
| @@ -198,15 +222,15 @@ | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail="invalid directory path") | ||
|
|
||
| if str(resolved) in _DENIED_PATHS: | ||
| if not resolved.exists() or not resolved.is_dir(): | ||
| raise HTTPException(status_code=400, detail="directory does not exist or is not a directory") | ||
|
|
||
| if not _is_directory_safe_for_allowlist(resolved): | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail=f"adding '{resolved}' is not allowed — path is too broad or sensitive" | ||
| detail=f"adding '{resolved}' is not allowed — directory is outside allowed workdirs or is too broad" | ||
| ) | ||
|
|
||
| if not resolved.exists() or not resolved.is_dir(): | ||
| raise HTTPException(status_code=400, detail="directory does not exist or is not a directory") | ||
|
|
||
| _save_runtime_allowed_dir(resolved) | ||
| return { | ||
| "ok": True, |
Copilot is powered by AI and may make mistakes. Always verify output.
This was referenced Mar 7, 2026
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.
Closes #139 #140
完成paperToContext模块开发，实现paper library->p2c提取一篇论文上下文->DeepStudio页面输入prompt给cc复现代码
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests