feat(Claude coplit): 增加Claude可视化，删除local docker，E2B，codex broker#136
Conversation
synchronous
Related to jerry609#134 Signed-off-by: LIU BOYU <oor2020@163.com>
feat(DeepStudio): 重构DeepStudio页面，删除local docker和E2B，更换成codex 沙箱
Closes jerry609#134 Signed-off-by: LIU BOYU <oor2020@163.com>
|
@CJBshuosi is attempting to deploy a commit to the Jerry's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces a major architectural upgrade to the Studio feature, replacing E2B/Docker execution with Claude CLI integration for code generation, redesigning the Studio UI with a 3-panel layout (papers, reproduction log, files), implementing paper management with localStorage persistence, and adding comprehensive file management and snapshot capabilities to the runbook API. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Browser (Next.js)
participant Proxy as /api/studio/chat<br/>(Node.js Route)
participant Backend as Backend<br/>studio_chat.py
participant Claude as Claude CLI
participant Anthropic as Anthropic API
User->>Frontend: Request Studio Chat with paper context
Frontend->>Proxy: POST /api/studio/chat (text/event-stream)
Proxy->>Backend: Forward POST with request body
Backend->>Backend: Discover Claude CLI path
alt Claude CLI Available
Backend->>Claude: Execute CLI with mode,<br/>model, and prompt
Claude->>Claude: Stream response
Claude-->>Backend: Stdout + progress
Backend->>Backend: Strip ANSI,<br/>emit SSE events
else Claude CLI Not Available
Backend->>Backend: Emit fallback notification
Backend->>Anthropic: stream_anthropic_api<br/>(mode, context)
Anthropic-->>Backend: Streaming deltas
Backend->>Backend: Emit SSE events
end
Backend-->>Proxy: SSE stream<br/>(progress, result, error)
Proxy-->>Frontend: Response headers +<br/>upstream stream
Frontend-->>User: Display streamed<br/>chat results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @CJBshuosi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在通过集成 Claude 可视化功能来显著增强 DeepStudio 的用户体验，使其能够直接在开发环境中进行代码生成和交互。同时，它通过移除对本地 Docker 和 E2B 执行器的依赖来简化后端架构，将代码执行的重心转移到 Claude CLI。此外，PR 还包含了多份详细的设计文档，为未来的语义搜索和 UI 改进奠定了基础，体现了产品在核心功能和用户界面方面的持续演进。 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
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces an interactive 'copilot' feature based on the Claude CLI, replacing old Docker and E2B backends to provide a more interactive, AI-driven workflow. While the overall submission is high-quality with good design documentation, it introduces a critical security vulnerability in the studio_chat route, allowing arbitrary directory access without validation and dangerous CLI permissions. Furthermore, a CORS misconfiguration in the main API entry point will cause startup failures and poses a security risk. These critical issues, along with other areas for code robustness, need to be addressed.
| cwd = request.project_dir or os.getcwd() | ||
| if not os.path.isdir(cwd): | ||
| cwd = os.getcwd() |
There was a problem hiding this comment.
The studio_chat endpoint has a critical security vulnerability. It allows users to specify an arbitrary project_dir which is used as the current working directory (cwd) for spawning the claude CLI subprocess via asyncio.create_subprocess_exec. Since the claude CLI is started with the --dangerously-skip-permissions flag in Code mode, an attacker can manipulate the project_dir and chat message to read, modify, or delete arbitrary files on the server's filesystem. This endpoint lacks any validation against an allowed directory list, unlike other file-related endpoints (e.g., in runbook.py).
Remediation: Implement a directory whitelist check for the project_dir parameter in studio_chat.py, similar to the _allowed_workdir function used in runbook.py. Ensure that the claude CLI is only spawned within authorized directories.
| | Dimension | Before | After | | ||
| |-----------|--------|-------| | ||
| | Width | 672px (max-w-2xl) | 576px (max-w-xl) | | ||
| | Min Height | 100-120px | 120-140px | | ||
| | Aspect Ratio | ~5.6:1 | ~4.1:1 (more balanced) | |
There was a problem hiding this comment.
在 "Golden Ratio Design" 表格中存在一个不一致之处。表格中 "After" 列的宽度显示为 576px (max-w-xl)，但随后的说明文字和实现细节中提到的是 max-w-3xl (768px)。这可能会在实现过程中造成混淆，请澄清最终期望的宽度。
| chunk = await process.stdout.read(100) | ||
| if not chunk: | ||
| break | ||
|
|
||
| text = chunk.decode("utf-8", errors="replace") |
There was a problem hiding this comment.
在流式读取子进程输出时，使用 read(100) 读取固定大小的字节块，然后用 UTF-8 解码，可能会在多字节字符被分割到两个块中时导致 UnicodeDecodeError。虽然 errors="replace" 可以防止程序崩溃，但它可能会导致文本中出现乱码（例如 ``）。一个更健壮的方法是按行读取（如果输出是行缓冲的），这可以避免字符被不完整地分割。
| chunk = await process.stdout.read(100) | |
| if not chunk: | |
| break | |
| text = chunk.decode("utf-8", errors="replace") | |
| line_bytes = await process.stdout.readline() | |
| if not line_bytes: | |
| break | |
| text = line_bytes.decode("utf-8", errors="replace") |
| import re | ||
| text = re.sub(r'\x1B\[[0-9;]*[a-zA-Z]', '', text) |
There was a problem hiding this comment.
import re 语句被放在了嵌套的 read_stream 函数内部。通常，为了提高代码的可读性并避免在每次函数调用时重复导入，所有的 import 语句都应该放在文件的顶部。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/paperbot/infrastructure/logging/execution_logger.py (4)
217-221:⚠️ Potential issue | 🟡 MinorNon-thread-safe singleton initialization.
Two concurrent callers can each pass the
is Nonecheck and create separateExecutionLoggerinstances (with distinct DB connections and in-memory buffers), causing divergent log state.🔒 Proposed fix: use a module-level lock
+_global_lock = threading.Lock() def get_execution_logger() -> ExecutionLogger: """Get or create global ExecutionLogger instance.""" global _global_logger - if _global_logger is None: - _global_logger = ExecutionLogger() + if _global_logger is None: + with _global_lock: + if _global_logger is None: + _global_logger = ExecutionLogger() return _global_logger🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/logging/execution_logger.py` around lines 217 - 221, The get_execution_logger() singleton is not thread-safe: multiple threads can pass the "_global_logger is None" check and create distinct ExecutionLogger instances; fix by adding a module-level lock (e.g., _global_logger_lock = threading.Lock()) and using it inside get_execution_logger() to synchronize initialization, perform a second "_global_logger is None" check after acquiring the lock (double-checked locking), create and assign the ExecutionLogger only while holding the lock, then release the lock so concurrent callers get the same _global_logger.
106-128:⚠️ Potential issue | 🟠 Major
asyncio.Queue.put_nowaitis not thread-safe — useloop.call_soon_threadsafewhen the callback fires from a worker thread.
asyncio.Queueis designed for single-threaded async use. The callback returned byget_log_callbackis synchronous and likely invoked from a subprocess output reader or thread pool (typical for CLI/executor integrations like the new Claude CLI flow). Callingput_nowaitfrom outside the event loop thread causes undefined behavior or silent data loss.Additionally, the synchronous
session.commit()insideappendblocks the event loop ifappendis ever invoked from within async code (directly or transitively). Consider offloading DB writes to a background thread viaasyncio.get_event_loop().run_in_executor.🔒 Proposed fix: thread-safe queue insertion
+import threading + def append(self, entry: LogEntry) -> None: """Append a log entry (persists to DB and buffers for streaming).""" # Persist to database try: with self._provider.session() as session: model = ExecutionLogModel( run_id=entry.run_id, ts=entry.ts, level=entry.level, message=entry.message, source=entry.source, ) session.add(model) session.commit() except Exception: pass # Best-effort persistence # Buffer for real-time streaming if entry.run_id in self._buffers: try: - self._buffers[entry.run_id].put_nowait(entry) + loop = asyncio.get_event_loop() + if loop.is_running(): + loop.call_soon_threadsafe(self._buffers[entry.run_id].put_nowait, entry) + else: + self._buffers[entry.run_id].put_nowait(entry) except Exception: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/logging/execution_logger.py` around lines 106 - 128, The append method currently calls session.commit() synchronously and uses self._buffers[entry.run_id].put_nowait(entry) which is unsafe from non-event-loop threads; change append to offload DB persistence to a background executor (use asyncio.get_event_loop().run_in_executor or similar) so commits don't block the event loop, and replace direct put_nowait with a thread-safe enqueue via loop.call_soon_threadsafe(lambda: self._buffers[entry.run_id].put_nowait(entry)) (or schedule a coroutine on the loop) to ensure safe cross-thread queue insertion; update references in append (ExecutionLogModel, session.commit, self._buffers) accordingly.
106-121:⚠️ Potential issue | 🟡 Minor
LogEntry.metadatais silently dropped on DB persistence.The
ExecutionLogModelinsert omits themetadatafield, so any structured metadata attached to aLogEntryis lost at persistence time, even thoughto_dict()andto_sse()both surface it.If
ExecutionLogModelhas a corresponding column (e.g., a JSON column), add it to the insert; otherwise document the intentional omission in the docstring or drop themetadatafield fromLogEntryto avoid misleading callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/logging/execution_logger.py` around lines 106 - 121, The append method currently drops LogEntry.metadata when persisting; update the ExecutionLogModel construction in append (method name append, class ExecutionLogModel, type LogEntry and its metadata) to include the metadata field if the model has a corresponding column (serialize to JSON/dict as needed) so structured metadata is persisted, and ensure serialization/deserialization matches to_dict()/to_sse(); if ExecutionLogModel does not support metadata, either document the deliberate omission in the append docstring or remove metadata from LogEntry to avoid misleading callers.
91-104:⚠️ Potential issue | 🟠 MajorMemory leak:
_buffersand_active_runsentries are never evicted.Every run ID accumulates an
asyncio.Queueand a boolean in memory indefinitely — there is no cleanup instop_runor afterstream_logsexits. In a long-running service processing many runs, this is an unbounded leak.🧹 Proposed fix: clean up state after stream is consumed
Add a cleanup helper and call it at the end of
stream_logsor fromstop_runafter a grace period:def stop_run(self, run_id: str) -> None: """Stop capturing logs for a run.""" self._active_runs[run_id] = False # Signal end of stream if run_id in self._buffers: try: self._buffers[run_id].put_nowait(None) # type: ignore except Exception: pass +def cleanup_run(self, run_id: str) -> None: + """Release in-memory state for a completed run.""" + self._buffers.pop(run_id, None) + self._active_runs.pop(run_id, None)Then in
stream_logs, after thewhileloop exits:except asyncio.TimeoutError: if not self._active_runs.get(run_id, False) and queue.empty(): break continue + + self.cleanup_run(run_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/infrastructure/logging/execution_logger.py` around lines 91 - 104, The _buffers and _active_runs dict entries created by start_run are never removed, causing an unbounded memory leak; add a cleanup path to remove entries for a run_id when it's finished: implement a helper (e.g., _cleanup_run(run_id)) that deletes self._buffers[run_id] and self._active_runs[run_id] (safely handling missing keys) and call it at the end of stream_logs when the stream loop exits (after receiving the sentinel None) and/or from stop_run after signalling the end (optionally after a short grace period) so queues are drained and references released; reference start_run, stop_run, stream_logs, _buffers and _active_runs when making the change.src/paperbot/repro/base_executor.py (1)
15-49:⚠️ Potential issue | 🔴 CriticalFix ExecutionResult constructor call - missing required
statusparameter.The
ExecutionResultconstructor call is incorrect.ExecutionResultrequiresstatus(str) andexit_code(int) as positional arguments;successis a read-only property, not a constructor parameter. This will raise a TypeError at runtime.Should be:
return ExecutionResult( status="error", exit_code=1, logs="Executor not available. Use Claude CLI for code execution.", )Additionally:
docker_executor.pyande2b_executor.pystill exist but are unused dead code and should be removed (the module docstring incorrectly claims they've been removed).- Per coding guidelines, this behavior change requires tests. Add or update tests in the
tests/directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/base_executor.py` around lines 15 - 49, The ExecutionResult construction in BaseExecutor.run is invalid because ExecutionResult expects positional status and exit_code (success is a read-only property); update BaseExecutor.run to call ExecutionResult with status="error", exit_code=1 and pass logs as the logs argument (i.e., create ExecutionResult(status="error", exit_code=1, logs=...)) so BaseExecutor.run and its return value are valid; also remove or delete the unused dead modules docker_executor.py and e2b_executor.py (they are misleadingly referenced in the module docstring) and add/update unit tests under tests/ to cover the stub behavior (e.g., test BaseExecutor.run returns an ExecutionResult with status "error" and exit_code 1) per the coding guidelines.src/paperbot/api/routes/sandbox.py (1)
288-304:⚠️ Potential issue | 🟠 MajorAdd tests for the
/sandbox/statusendpoint to cover the simplified response shape.The endpoint code and response shape are correct and consistent with
SystemStatus.to_dict(). However, the behavior changed (error response simplified to queue-only status) and coding guidelines require tests for behavior changes in{src,web/src}/**/*.{py,ts,tsx}files. No existing tests for this endpoint were found in the test suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 288 - 304, Add tests that call the GET /sandbox/status route (exercise the get_system_status handler) and verify both the normal and error response shapes: mock paperbot.infrastructure.monitoring.resource_monitor.get_resource_monitor().get_system_status to return a SystemStatus-like object and assert its to_dict() is returned, and mock it to raise an exception and assert the simplified error response {"queue": {"status": "unknown", "error": ...}} is returned. Reference the get_system_status function in src/paperbot/api/routes/sandbox.py and the SystemStatus.to_dict() shape when asserting fields and error message contents. Ensure tests live under the existing test suite pattern for API routes and include both success and exception cases.src/paperbot/api/routes/runbook.py (3)
160-194:⚠️ Potential issue | 🟠 MajorTwo separate
session.commit()calls create a split transaction —AgentRunModelpersists even ifArtifactModelinsertion fails.If the second
commit()(Line 194) fails, theAgentRunModelfrom the firstcommit()(Line 174) is already persisted, leaving an orphaned run record with no corresponding artifact or snapshot file reference.🐛 Proposed fix — single commit
session.merge(run) - session.commit() snapshot_path = _snapshot_root() / f"{run_id}.json" snapshot_path.write_text(json.dumps(snapshot_payload, ensure_ascii=False), encoding="utf-8") artifact = ArtifactModel( run_id=run_id, step_id=None, type="snapshot", path_or_uri=str(snapshot_path), mime="application/json", size_bytes=snapshot_path.stat().st_size if snapshot_path.exists() else None, sha256=None, created_at=created_at, metadata_json=json.dumps( {"project_dir": str(root.resolve()), "label": body.label, "file_count": len(files), "total_bytes": total_bytes}, ensure_ascii=False, ), ) session.add(artifact) session.commit()Note: writing the snapshot file between the two DB operations also means if the file write fails, the run record is committed. Consider writing the file before any DB operations, or wrapping in a try/except that cleans up.
🤖 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 160 - 194, The code currently commits AgentRunModel with session.commit() before creating ArtifactModel, which can leave an orphaned run if the artifact insert or snapshot write fails; fix by writing the snapshot file (snapshot_path.write_text / snapshot_payload) first, then open a single DB transaction (use the existing _provider.session() or session.begin()) and within that transaction create and merge the AgentRunModel and add the ArtifactModel and call a single session.commit() (or let the context manager commit once), ensuring both records are persisted atomically; also add try/except around the file write to clean up any partial file on failure or raise before touching the DB so no run is committed without its artifact.
403-417:⚠️ Potential issue | 🟡 Minor
write_project_filehas no content size limit — a large payload could exhaust memory or disk.
read_project_fileenforcesmax_bytes, butwrite_project_fileaccepts arbitrary content size. Consider adding a content length check.🛡️ Suggested guard
class WriteFileRequest(BaseModel): project_dir: str path: str content: str + # Pydantic v2 field constraint or manual check: + # max_length on content can be set via Field `@router.post`("/runbook/file") async def write_project_file(body: WriteFileRequest): """Write a file under project_dir (creates parents).""" + if len(body.content) > 10_000_000: # 10MB guard + raise HTTPException(status_code=413, detail="content too large")🤖 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 403 - 417, The write_project_file handler accepts unlimited body.content which can exhaust memory/disk; add a size guard similar to read_project_file by defining a MAX_WRITE_BYTES constant and checking the byte length of body.content (e.g., len(body.content.encode("utf-8")) or len(body.content) if already bytes) before creating/writing the file in write_project_file, and return an HTTPException (status 413 or 400) when the content exceeds the limit; keep the check early (before target.parent.mkdir/target.write_text) and reference write_project_file, body.content, and the MAX_WRITE_BYTES symbol in your changes.
616-656:⚠️ Potential issue | 🟠 MajorSequential hunk application shifts line indices, causing later hunks to mismatch or apply at wrong positions.
current_linesis mutated in-place (Line 653–655) as each hunk is applied. If a hunk adds or removes lines, subsequent hunks' pattern matches shift. Hunks should be applied in reverse order (bottom-to-top) or offsets should be tracked.🐛 Suggested approach — apply hunks in reverse line order
- for i, hunk in enumerate(body.hunks): + # Sort hunks by their match position (bottom-first) to avoid index drift. + # As a simpler alternative, apply all hunks in reverse order of input: + for i, hunk in reversed(list(enumerate(body.hunks))):A more robust approach would be to find all match positions first, validate they don't overlap, then apply from bottom to top.
🤖 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 616 - 656, The loop mutates current_lines as each hunk is applied, which shifts indices for later hunks; instead first resolve match positions for every hunk without mutating current_lines (use the existing find_once logic to record start_idx and used_pattern per hunk, adding failures when no unique match), validate that recorded matches do not overlap, then apply the successful hunks in descending start_idx order so replacements happen bottom-to-top (use current_lines[start:...]=... with the recorded start_idx and whether to replace using "core" or "context"); ensure you still increment applied only for actually applied hunks and append failures for unmatched/overlapping hunks.src/paperbot/repro/repro_agent.py (2)
22-22:⚠️ Potential issue | 🟡 MinorRemove unused
Literalimport.
Literalwas imported for theExecutorTypealias, which no longer exists in this file. It is not referenced anywhere.Fix
-from typing import Dict, Any, Optional, Literal, TYPE_CHECKING +from typing import Dict, Any, Optional, TYPE_CHECKING🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/repro_agent.py` at line 22, Remove the unused Literal import from the top-level import statement in repro_agent.py: update the import line that currently reads "from typing import Dict, Any, Optional, Literal, TYPE_CHECKING" to exclude Literal since ExecutorType no longer exists and Literal isn't referenced anywhere; ensure only the used typing names (Dict, Any, Optional, TYPE_CHECKING) remain to avoid linter/flake warnings.
393-412:⚠️ Potential issue | 🟠 MajorThe legacy
run()method will always fail and report a score of 0.0.
ReproAgent.run()is marked as legacy but remains callable from the CLI (main.py line 642) with the--reproflag, as well as from tests. Sinceself.executoris now aBaseExecutorstub that always returnsexit_code=1, the method will unconditionally reportpassed=Falseand a score of0.0, silently producing incorrect results.Remove this method, raise
NotImplementedError, or add a clear deprecation warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/repro_agent.py` around lines 393 - 412, The legacy method ReproAgent.run currently invokes self.executor (now a BaseExecutor stub returning exit_code=1) causing all runs to fail; replace the body of ReproAgent.run with an explicit deprecation/unsupported signal: emit a deprecation warning (warnings.warn or logger.warning) and raise NotImplementedError (or remove the method), so callers can't silently get a false failure; update any tests/CLI that call ReproAgent.run to use the new API or handle the exception.
🟡 Minor comments (8)
web/src/lib/store/studio-store.ts-87-98 (1)
87-98:⚠️ Potential issue | 🟡 Minor
JSON.parsecast skips schema validation — stale localStorage data can silently produce type-incorrect objects.
JSON.parse(stored) as StudioPaper[]is purely a TypeScript assertion; if the stored data was written by an older schema (e.g., missingtaskIdsorstatus), downstream code accessingpaper.taskIds.map(...)will throw a runtime TypeError that thetry/catchhere won't catch (it only guards against malformed JSON).Consider adding a lightweight guard before returning:
🛡️ Proposed fix with basic shape validation
function loadPapersFromStorage(): StudioPaper[] { if (typeof window === 'undefined') return [] try { const stored = localStorage.getItem(STORAGE_KEY) if (stored) { - return JSON.parse(stored) as StudioPaper[] + const parsed = JSON.parse(stored) + if (!Array.isArray(parsed)) return [] + // Filter out entries missing required fields introduced in this schema version + return parsed.filter( + (p): p is StudioPaper => + typeof p?.id === 'string' && + typeof p?.title === 'string' && + Array.isArray(p?.taskIds) + ) } } catch (e) { console.error('Failed to load papers from localStorage:', e) } return [] }🤖 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 87 - 98, The parsed localStorage value in loadPapersFromStorage is currently asserted to StudioPaper[] without validating shape; update loadPapersFromStorage to validate the parsed value (from localStorage.getItem(STORAGE_KEY)) is an array and each element has the required fields (e.g., ensure each item has a taskIds array and a status string) before returning it — for invalid entries either fix defaults (e.g., taskIds = [] and status = 'unknown' or a safe default) or filter them out and return the cleaned array; keep the try/catch around JSON.parse and return [] if the top-level structure is not an array or every entry is invalid so downstream uses like paper.taskIds.map(...) cannot crash at runtime.web/src/app/api/studio/cwd/route.ts-21-31 (1)
21-31:⚠️ Potential issue | 🟡 MinorFallback leaks the server's
HOMEpath to the client.
process.env.HOMEis the Next.js server's home directory (e.g.,/home/deploy), not the user's machine. Exposing it gives clients server filesystem information. Use a fixed safe default instead.Proposed fix
return Response.json( { - cwd: process.env.HOME || "/tmp", + cwd: "/tmp", source: "fallback", error: "Failed to get working directory from backend", }, { status: 200 } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/studio/cwd/route.ts` around lines 21 - 31, The fallback currently returns process.env.HOME which exposes the server's home path; update the catch block in route.ts (the Response.json fallback) to use a fixed safe default (e.g., "/tmp" or "/") instead of process.env.HOME and keep the rest of the payload (cwd, source: "fallback", error) intact so the client doesn't receive server filesystem details.web/src/components/studio/ReproductionLog.tsx-333-370 (1)
333-370:⚠️ Potential issue | 🟡 MinorSSE stream may end without emitting
resultorerror, leavingstatusstuck at"running".If the server closes the stream prematurely or sends only
progressevents, thefor awaitloop completes but neither theresultnorerrorbranch fires. The component stays inisBusystate permanently. Add a finalizer after the loop to handle this edge case.🛡️ Suggested guard after the loop (applies to both functions)
} } + // If stream ended without explicit result/error + if (status === "running") { + setStatus("success") + updateTaskStatus(taskId, "completed") + } } catch (e) {Also applies to: 413-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/ReproductionLog.tsx` around lines 333 - 370, The SSE reader loop can exit without a final "result" or "error", leaving the component stuck in running: after the for-await loop in the function using readSSE (and apply the same change to the other loop around 413-445), add a final guard that checks whether the run finished (e.g., inspect setStatus state, lastGenCodeResult, or lastError); if no result/error was observed, call addAction(taskId, {type: "error", content: "Stream ended unexpectedly"}), updateTaskStatus(taskId, "error"), updatePaper(selectedPaperId, {status: "error"}) when selectedPaperId exists, setLastError("Stream ended unexpectedly"), and setStatus("error") so the UI exits the busy state.docs/design/research-page-redesign-v4.md-29-36 (1)
29-36:⚠️ Potential issue | 🟡 MinorTable contradicts the prose and implementation — "After" width is wrong.
Line 29 states "Increase width to
max-w-3xl(768px)" and the implementation on Line 68 usesmax-w-3xl, but the table on Line 33 shows the "After" as576px (max-w-xl), which is actually smaller than the "Before" value of672px (max-w-2xl).📝 Proposed fix
| Dimension | Before | After | |-----------|--------|-------| -| Width | 672px (max-w-2xl) | 576px (max-w-xl) | +| Width | 672px (max-w-2xl) | 768px (max-w-3xl) | | Min Height | 100-120px | 120-140px | -| Aspect Ratio | ~5.6:1 | ~4.1:1 (more balanced) | +| Aspect Ratio | ~5.6:1 | ~5.5:1 (or recalculate) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/research-page-redesign-v4.md` around lines 29 - 36, The table's "After" width entry contradicts the prose and implementation: update the table so the "After" width reads "768px (max-w-3xl)" to match the statement "Increase width to `max-w-3xl` (768px)" and the implementation that uses `max-w-3xl`; ensure any dependent values (Aspect Ratio or Width column) reflect that change so the table is consistent with the `max-w-3xl` setting.src/paperbot/api/routes/studio_chat.py-391-417 (1)
391-417:⚠️ Potential issue | 🟡 Minor
~/Documentsfallback may not exist;os.getcwd()would be safer.Line 408 falls back to
~/Documentsif~/Projectsdoesn't exist, but~/Documentsisn't guaranteed to exist on all platforms (especially Linux servers). This would return a non-existent path to the caller.♻️ Proposed fix
projects_dir = os.path.expanduser("~/Projects") if os.path.isdir(projects_dir): suggested_cwd = projects_dir else: - suggested_cwd = os.path.expanduser("~/Documents") + suggested_cwd = default_cwd # Fall back to home directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/studio_chat.py` around lines 391 - 417, The fallback to ~/Documents in studio_cwd can return a non-existent path; change the logic in the studio_cwd function so that if projects_dir (computed as os.path.expanduser("~/Projects")) does not exist you set suggested_cwd to the actual cwd (from os.getcwd()) or to default_cwd only after verifying it exists; update the branch that assigns suggested_cwd (replace the "~/Documents" fallback) to use cwd (or a verified existing home directory) and ensure suggested_cwd points to an existing directory before returning.docs/design/search-architecture-upgrade.md-254-280 (1)
254-280:⚠️ Potential issue | 🟡 MinorReturn type hint mismatch in
compute_rrf_scoressketch.The function signature declares
-> Dict[str, float]but Line 279 returns a tuple(scores, paper_by_hash). Since this is a design doc code sketch, just worth correcting before implementation so the spec is accurate.-) -> Dict[str, float]: +) -> tuple[Dict[str, float], Dict[str, "PaperCandidate"]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/search-architecture-upgrade.md` around lines 254 - 280, The function compute_rrf_scores declares a return type of Dict[str, float] but actually returns a tuple (scores, paper_by_hash); update the signature to accurately reflect the returned types (e.g., -> Tuple[Dict[str, float], Dict[str, PaperCandidate]]) and import/annotate Tuple and PaperCandidate as needed, or alternatively change the return to only scores if you intend a single Dict; adjust the type hints for scores and paper_by_hash accordingly so compute_rrf_scores, scores, and paper_by_hash are consistent.web/src/components/studio/FilesPanel.tsx-255-263 (1)
255-263:⚠️ Potential issue | 🟡 MinorAccessibility: icon-only Refresh button is missing an
aria-labelScreen readers will announce this button with no meaningful name.
♿ Proposed fix
<Button variant="ghost" size="icon" className="h-6 w-6" onClick={refreshIndex} disabled={!projectDir || loadingIndex} + aria-label="Refresh file list" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 255 - 263, The Refresh icon-only Button in FilesPanel.tsx (the Button rendering RefreshCw with onClick={refreshIndex} and disabled={!projectDir || loadingIndex}) lacks an accessible name; add an aria-label (e.g., aria-label="Refresh index" or similar) to the Button so screen readers can announce its purpose, keeping existing props (variant, size, className, onClick, disabled) and ensuring the label text is concise and descriptive; no changes to RefreshCw required.web/src/components/studio/FilesPanel.tsx-49-87 (1)
49-87:⚠️ Potential issue | 🟡 Minor
buildFileTreemisclassifies a node when a bare filename is processed before a same-named directory prefixIf the
filesarray contains a bare entry (e.g.,"src") before a nested one (e.g.,"src/utils.py"), thenodeMapcaches the first node asisDirectory: false. The second iteration finds the cached node and skips creation, so"src"keepsisDirectory: falseand its child"utils.py"is never rendered. The API almost certainly never returns this combination, but the logic isn't defensive about it.Consider flipping the
isDirectoryflag when an existing leaf node is encountered as an intermediate segment:🛡️ Proposed defensive fix
let node = nodeMap.get(currentPath) if (!node) { node = { name: part, path: currentPath, isDirectory: !isLast, children: [], } nodeMap.set(currentPath, node) currentLevel.push(node) + } else if (!isLast && !node.isDirectory) { + // Promote an existing leaf to a directory when we discover + // it is actually an intermediate path segment. + node.isDirectory = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 49 - 87, In buildFileTree, the nodeMap can contain a cached leaf node (isDirectory: false) for a path like "src" when later processing "src/utils.py", so detect when an existing node is encountered but should be a directory (i.e., node exists and !node.isDirectory while not isLast) and flip node.isDirectory = true and ensure node.children is initialized before descending; this makes intermediate segments become directories and allows adding the child nodes correctly while still using nodeMap to avoid duplicates.
🧹 Nitpick comments (9)
web/src/lib/store/studio-store.ts (1)
83-107: No tests for the new storage helpers.
loadPapersFromStorage,savePapersToStorage, andgenerateIdintroduce new persistent behavior (localStorage round-trip, ID generation) but no tests have been added for them.Key cases worth covering:
loadPapersFromStoragereturns[]in SSR context (windowundefined)loadPapersFromStoragehandles corrupted/stale JSON gracefullysavePapersToStorageserializes and round-trips correctlygenerateIdproduces unique IDs across rapid callsAs per coding guidelines, "if behavior changes, add or update tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/studio-store.ts` around lines 83 - 107, Add unit tests for the new storage helpers: write tests that exercise loadPapersFromStorage, savePapersToStorage, and generateId. Specifically, mock/undefine window to assert loadPapersFromStorage returns [] in SSR, mock localStorage.getItem to return corrupted JSON and assert loadPapersFromStorage handles it and returns [], test savePapersToStorage by mocking localStorage.setItem and then mocking getItem to verify the JSON round-trips back to the original StudioPaper[] shape, and test generateId by calling it many times in rapid succession and asserting all returned strings are unique and match the expected pattern; use the existing STORAGE_KEY and the function names loadPapersFromStorage, savePapersToStorage, and generateId to locate targets.web/src/components/studio/WorkspaceSetupDialog.tsx (1)
39-63: MissingAbortControllercleanup for the fetch call.If the dialog closes (or
paper.titlechanges) while the fetch is in-flight, the.thenhandlers will still fire and update state. Add a cleanup function to abort the request.Proposed fix
useEffect(() => { if (open) { + const controller = new AbortController() setLoading(true) setError(null) - fetch("/api/studio/cwd") + fetch("/api/studio/cwd", { signal: controller.signal }) .then((res) => res.json()) .then((data) => { // ... }) .catch(() => { - setCurrentDir("/tmp") - setError("Could not detect current directory") + if (!controller.signal.aborted) { + setCurrentDir("/tmp") + setError("Could not detect current directory") + } }) .finally(() => { - setLoading(false) + if (!controller.signal.aborted) { + setLoading(false) + } }) + return () => controller.abort() } }, [open, paper.title])🤖 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 39 - 63, The effect in WorkspaceSetupDialog that calls fetch("/api/studio/cwd") needs an AbortController and cleanup to avoid updating state after the dialog closes or paper.title changes; create an AbortController inside the useEffect, pass controller.signal to fetch, check controller.signal.aborted (or catch the DOMException) before calling setLoading/setError/setCurrentDir/setCustomDir in the .then/.catch, and return a cleanup function that calls controller.abort() so inflight requests are canceled when the component unmounts or dependencies change.web/src/app/api/studio/chat/route.ts (1)
3-5:apiBaseUrl()is duplicated across three new route files.This identical helper already exists in
web/src/app/api/research/_base.ts. Consider importing it from there (or extracting to a shared location) instead of redefining it in each route file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/studio/chat/route.ts` around lines 3 - 5, The file defines a duplicate helper function apiBaseUrl; instead of redefining it in this route, import and use the existing exported apiBaseUrl helper from the shared research base module (the module that already exports apiBaseUrl) so all routes reuse the single implementation; remove the local apiBaseUrl definition and replace its usages with the imported symbol to avoid duplication across the three new route files.src/paperbot/api/routes/runbook.py (1)
29-30:Base.metadata.create_allat module import time is a side effect.This runs DDL against the database every time this module is imported (including during tests, linting tools that inspect imports, etc.). Consider deferring to an app startup hook or a dedicated migration step.
🤖 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 29 - 30, The call to Base.metadata.create_all(_provider.engine) is causing DDL to run at module import time; remove that side-effect from the module and instead invoke schema creation during application startup or a dedicated migration step. Specifically, delete or comment out the module-level Base.metadata.create_all(...) line near SessionProvider() and expose or add a function like init_db or a startup handler that accepts the SessionProvider (or its .engine) and calls Base.metadata.create_all(provider.engine) there so tests and import-time tooling no longer execute DDL implicitly.web/src/components/studio/ReproductionLog.tsx (1)
230-242: Fetch inuseEffectdoesn't guard against stale updates after unmount.If the component unmounts before the fetch resolves,
setCliStatuswill be called on an unmounted component. While React 18+ tolerates this without errors, it's still a wasted state update.🛡️ Minor cleanup guard
useEffect(() => { + let cancelled = false fetch("/api/studio/status") .then(res => res.json()) .then(data => { + if (cancelled) return setCliStatus({ available: data.claude_cli === true, version: data.claude_version, }) }) .catch(() => { + if (cancelled) return setCliStatus({ available: false }) }) + return () => { cancelled = true } }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/ReproductionLog.tsx` around lines 230 - 242, The fetch in the useEffect may call setCliStatus after the component unmounts; wrap the request with an AbortController (or a mounted flag) and return a cleanup that aborts/marks unmounted so you avoid stale updates. Specifically, in the effect that calls fetch("/api/studio/status") use an AbortController signal (or a let mounted = true) and check the signal/mounted before calling setCliStatus in both the .then and .catch handlers, and call controller.abort() (or set mounted = false) in the effect cleanup to prevent state updates after unmount.src/paperbot/api/routes/studio_chat.py (2)
36-38:ChatMessage.roleis unvalidated — consider constraining to known roles.Using a plain
strforroleallows invalid values to flow into message construction. ALiteral["user", "assistant", "system"]would catch mistakes at request validation time.♻️ Proposed fix
+from typing import List, Optional, Literal, AsyncGenerator + +ChatRole = Literal["user", "assistant", "system"] + class ChatMessage(BaseModel): - role: str + role: ChatRole content: str🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/studio_chat.py` around lines 36 - 38, ChatMessage.role is currently an unconstrained str which allows invalid roles; change its type to a constrained union such as typing.Literal["user", "assistant", "system"] (or a small Enum) on the ChatMessage Pydantic model so request validation fails for unknown roles; update imports (e.g., add typing.Literal or enum.Enum) and adjust any callers that construct ChatMessage to use one of the allowed values.
11-16: Unused import:json.
jsonis imported at Line 12 but never used in this file.♻️ Proposed fix
import asyncio -import json import os +import re import shutil import subprocess🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/studio_chat.py` around lines 11 - 16, Remove the unused import of json from the top-level imports in the studio_chat module: locate the import line that currently reads "import json" alongside asyncio, os, shutil, subprocess and delete that token (or remove the entire import statement if it only contains json); if json is intended to be used later, instead add a clear usage or a TODO comment where it's needed, but do not leave an unused import in the module.web/src/components/studio/FilesPanel.tsx (2)
215-225: Silent failures inopenFilegive no user feedbackBoth the
!res.okbranch (line 219) and thecatchblock (line 222) swallow errors without notifying the user. If a file fails to load, the editor just stays unchanged with no indication of what went wrong.Consider showing a brief error toast/notification (using whatever toast primitive the project already has) or at minimum logging to the console so failures are observable during development. As per coding guidelines, behavior changes should be accompanied by tests, and testable error paths help here too.
Based on learnings: "Applies to
{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 215 - 225, The openFile function currently swallows failures in both the !res.ok branch and the catch block; update openFile to surface errors by calling the project's toast/notification primitive (or at minimum console.error) with a clear message including the path and error/status, e.g. when res.ok is false log/toast the response status and path, and in the catch include the caught error; keep using addFile and languageForPath unchanged, and add/adjust tests to cover the failing fetch path and thrown errors so the new notification behavior is asserted.
186-213: Race condition: concurrentrefreshIndexfetches can render stale data
refreshIndexis an async function that isn't guarded by anAbortController. IfprojectDirchanges quickly (e.g., switching between papers while a previous fetch is still in-flight), both fetches settle and the last one to complete wins — potentially painting the UI with data from the wrong directory. Additionally, theeslint-disableon line 212 suppresses the legitimate missing-dependency warning forrefreshIndex; wrapping it inuseCallbackis the idiomatic fix.♻️ Proposed refactor with AbortController + useCallback
- const refreshIndex = async () => { + const refreshIndex = useCallback(async (signal?: AbortSignal) => { if (!projectDir) return setLoadingIndex(true) try { const res = await fetch( `/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true`, + { signal } ) if (!res.ok) return const data = (await res.json()) as FileIndexResponse setFileIndex(data.files || []) const firstLevelDirs = new Set<string>() for (const f of data.files || []) { const firstPart = f.split('/')[0] if (f.includes('/')) firstLevelDirs.add(firstPart) } setExpandedDirs(firstLevelDirs) } catch (e) { + if (e instanceof Error && e.name === 'AbortError') return setFileIndex([]) } finally { setLoadingIndex(false) } - } + }, [projectDir]) useEffect(() => { + const controller = new AbortController() setFileIndex([]) setQuery("") setExpandedDirs(new Set()) - if (projectDir) refreshIndex() - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [projectDir]) + if (projectDir) refreshIndex(controller.signal) + return () => controller.abort() + }, [projectDir, refreshIndex])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 186 - 213, refreshIndex can race when projectDir changes causing stale UI; wrap refreshIndex in useCallback (dependent on projectDir) and inside it create an AbortController to pass signal to fetch, abort previous requests (store controller in a ref and abort before creating a new one), check controller.signal.aborted before applying results, and only call setFileIndex/setExpandedDirs when the response matches the current projectDir; then remove the eslint-disable in the useEffect and add refreshIndex to the dependency array so useEffect calls the memoized refreshIndex when projectDir changes.
| """ | ||
| Studio Chat API Route - Claude CLI Integration for DeepStudio | ||
|
|
||
| Spawns Claude CLI as a subprocess and streams responses. | ||
| Supports three modes like CodePilot: | ||
| - Code: Can edit files, run commands (permissionMode: acceptEdits) | ||
| - Plan: Planning only, no execution (permissionMode: plan) | ||
| - Ask: Text-only conversation, no tools (permissionMode: default) | ||
| """ | ||
|
|
||
| import asyncio | ||
| import json | ||
| import os | ||
| import shutil | ||
| import subprocess | ||
| from typing import List, Optional, Literal, AsyncGenerator | ||
|
|
||
| from fastapi import APIRouter | ||
| from fastapi.responses import StreamingResponse | ||
| from pydantic import BaseModel | ||
|
|
||
| from ..streaming import StreamEvent, wrap_generator | ||
|
|
||
| router = APIRouter() | ||
|
|
||
| Mode = Literal["Code", "Plan", "Ask"] | ||
| Model = Literal["claude-sonnet-4-5", "claude-opus-4-5", "claude-haiku-4-5"] | ||
|
|
||
|
|
||
| class PaperContext(BaseModel): | ||
| title: str | ||
| abstract: str | ||
| method_section: Optional[str] = None | ||
|
|
||
|
|
||
| class ChatMessage(BaseModel): | ||
| role: str | ||
| content: str | ||
|
|
||
|
|
||
| class StudioChatRequest(BaseModel): | ||
| message: str | ||
| mode: Mode = "Code" | ||
| model: Model = "claude-sonnet-4-5" | ||
| paper: Optional[PaperContext] = None | ||
| project_dir: Optional[str] = None | ||
| history: List[ChatMessage] = [] | ||
| session_id: Optional[str] = None | ||
|
|
||
|
|
||
| def find_claude_cli() -> Optional[str]: | ||
| """Find Claude CLI executable path.""" | ||
| # Check common locations | ||
| candidates = [ | ||
| shutil.which("claude"), | ||
| os.path.expanduser("~/.npm-global/bin/claude"), | ||
| os.path.expanduser("~/.local/bin/claude"), | ||
| "/opt/homebrew/bin/claude", | ||
| "/usr/local/bin/claude", | ||
| ] | ||
|
|
||
| for path in candidates: | ||
| if path and os.path.isfile(path): | ||
| return path | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def get_mode_flag(mode: Mode) -> str: | ||
| """Map mode to Claude CLI permission flag.""" | ||
| if mode == "Code": | ||
| return "--dangerously-skip-permissions" # Allow all operations | ||
| elif mode == "Plan": | ||
| return "--plan" # Planning mode - no execution | ||
| else: # Ask | ||
| return "" # Default - no tools | ||
|
|
||
|
|
||
| def get_model_id(model: Model, for_cli: bool = False) -> str: | ||
| """Map model selection to Claude model ID. | ||
|
|
||
| Args: | ||
| model: The model selection from the UI | ||
| for_cli: If True, returns CLI-friendly alias; if False, returns full model ID for API | ||
| """ | ||
| if for_cli: | ||
| # Claude CLI accepts short aliases | ||
| cli_mapping = { | ||
| "claude-sonnet-4-5": "sonnet", | ||
| "claude-opus-4-5": "opus", | ||
| "claude-haiku-4-5": "haiku", | ||
| } | ||
| return cli_mapping.get(model, "sonnet") | ||
| else: | ||
| # Full model IDs for Anthropic API | ||
| api_mapping = { | ||
| "claude-sonnet-4-5": "claude-sonnet-4-5-20250514", | ||
| "claude-opus-4-5": "claude-opus-4-5-20250514", | ||
| "claude-haiku-4-5": "claude-haiku-4-5-20250514", | ||
| } | ||
| return api_mapping.get(model, "claude-sonnet-4-5-20250514") | ||
|
|
||
|
|
||
| def build_prompt_with_context(message: str, paper: Optional[PaperContext], mode: Mode) -> str: | ||
| """Build the prompt with paper context if available.""" | ||
| parts = [] | ||
|
|
||
| if paper: | ||
| parts.append(f"# Paper Context\n**Title:** {paper.title}\n\n**Abstract:** {paper.abstract}") | ||
| if paper.method_section: | ||
| parts.append(f"\n**Method Section:** {paper.method_section}") | ||
| parts.append("\n---\n") | ||
|
|
||
| if mode == "Code": | ||
| parts.append("You are helping implement this research paper as working code. ") | ||
| elif mode == "Plan": | ||
| parts.append("You are creating an implementation plan for this research paper. Do not write code, only plan. ") | ||
| else: | ||
| parts.append("You are answering questions about this research paper. ") | ||
|
|
||
| parts.append(f"\n# User Request\n{message}") | ||
|
|
||
| return "\n".join(parts) | ||
|
|
||
|
|
||
| async def stream_claude_cli(request: StudioChatRequest) -> AsyncGenerator[StreamEvent, None]: | ||
| """Stream Claude CLI output as SSE events.""" | ||
|
|
||
| claude_path = find_claude_cli() | ||
|
|
||
| if not claude_path: | ||
| yield StreamEvent( | ||
| type="error", | ||
| message="Claude CLI not found. Please install it with: npm install -g @anthropic-ai/claude-code" | ||
| ) | ||
| return | ||
|
|
||
| # Build command | ||
| cmd = [claude_path] | ||
|
|
||
| # Add model flag (use CLI alias) | ||
| model_id = get_model_id(request.model, for_cli=True) | ||
| cmd.extend(["--model", model_id]) | ||
|
|
||
| # Add mode flag | ||
| mode_flag = get_mode_flag(request.mode) | ||
| if mode_flag: | ||
| cmd.append(mode_flag) | ||
|
|
||
| # Build prompt with context | ||
| prompt = build_prompt_with_context(request.message, request.paper, request.mode) | ||
|
|
||
| # Add the prompt with proper flags | ||
| # Note: --print requires --verbose for stream-json, so we use regular output | ||
| cmd.extend(["--print", prompt]) | ||
|
|
||
| yield StreamEvent( | ||
| type="progress", | ||
| data={ | ||
| "phase": "Starting", | ||
| "message": f"[{request.mode}] Connecting to Claude CLI...", | ||
| "model": request.model, | ||
| } | ||
| ) | ||
|
|
||
| try: | ||
| # Set working directory | ||
| cwd = request.project_dir or os.getcwd() | ||
| if not os.path.isdir(cwd): | ||
| cwd = os.getcwd() | ||
|
|
||
| # Spawn Claude CLI process | ||
| process = await asyncio.create_subprocess_exec( | ||
| *cmd, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| cwd=cwd, | ||
| env={**os.environ, "FORCE_COLOR": "0"}, # Disable ANSI colors | ||
| ) | ||
|
|
||
| full_content = "" | ||
|
|
||
| # Stream stdout - read chunks for real-time streaming | ||
| async def read_stream(): | ||
| nonlocal full_content | ||
| while True: | ||
| # Read in chunks for smoother streaming | ||
| chunk = await process.stdout.read(100) | ||
| if not chunk: | ||
| break | ||
|
|
||
| text = chunk.decode("utf-8", errors="replace") | ||
|
|
||
| # Skip ANSI escape codes | ||
| import re | ||
| text = re.sub(r'\x1B\[[0-9;]*[a-zA-Z]', '', text) | ||
|
|
||
| if text: | ||
| full_content += text | ||
| yield StreamEvent( | ||
| type="progress", | ||
| data={ | ||
| "delta": text, | ||
| "content": full_content, | ||
| "mode": request.mode, | ||
| } | ||
| ) | ||
|
|
||
| async for event in read_stream(): | ||
| yield event | ||
|
|
||
| # Wait for process to complete | ||
| await process.wait() | ||
|
|
||
| # Check for errors | ||
| if process.returncode != 0: | ||
| stderr = await process.stderr.read() | ||
| error_msg = stderr.decode("utf-8", errors="replace").strip() | ||
| if error_msg: | ||
| yield StreamEvent(type="error", message=error_msg) | ||
| return | ||
|
|
||
| # Emit final result | ||
| yield StreamEvent( | ||
| type="result", | ||
| data={ | ||
| "content": full_content, | ||
| "mode": request.mode, | ||
| "model": request.model, | ||
| } | ||
| ) | ||
|
|
||
| except FileNotFoundError: | ||
| yield StreamEvent( | ||
| type="error", | ||
| message=f"Claude CLI not found at: {claude_path}" | ||
| ) | ||
| except Exception as e: | ||
| yield StreamEvent( | ||
| type="error", | ||
| message=f"Claude CLI error: {str(e)}" | ||
| ) | ||
|
|
||
|
|
||
| async def stream_anthropic_api(request: StudioChatRequest) -> AsyncGenerator[StreamEvent, None]: | ||
| """Fallback: Stream response using Anthropic API directly.""" | ||
|
|
||
| yield StreamEvent( | ||
| type="progress", | ||
| data={"phase": "Processing", "message": f"[{request.mode}] Thinking..."}, | ||
| ) | ||
|
|
||
| try: | ||
| from ...infrastructure.llm.providers.anthropic_provider import AnthropicProvider | ||
|
|
||
| api_key = os.getenv("ANTHROPIC_API_KEY", "") | ||
| if not api_key: | ||
| yield StreamEvent(type="error", message="ANTHROPIC_API_KEY not set") | ||
| return | ||
|
|
||
| model_id = get_model_id(request.model, for_cli=False) | ||
| provider = AnthropicProvider( | ||
| api_key=api_key, | ||
| model_name=model_id, | ||
| max_tokens=8192, | ||
| ) | ||
|
|
||
| # Build system prompt based on mode | ||
| if request.mode == "Code": | ||
| system = "You are an expert AI coding assistant. Generate high-quality, well-documented code. Include type hints and docstrings. Wrap code in appropriate markdown code blocks." | ||
| elif request.mode == "Plan": | ||
| system = "You are a research implementation architect. Create detailed implementation plans WITHOUT generating actual code. Focus on architecture, components, and step-by-step planning." | ||
| else: | ||
| system = "You are a helpful research assistant. Answer questions clearly and concisely." | ||
|
|
||
| if request.paper: | ||
| system += f"\n\n# Paper Context\n**Title:** {request.paper.title}\n**Abstract:** {request.paper.abstract}" | ||
| if request.paper.method_section: | ||
| system += f"\n**Method Section:** {request.paper.method_section}" | ||
|
|
||
| messages = [{"role": "system", "content": system}] | ||
|
|
||
| for msg in request.history[-10:]: | ||
| messages.append({"role": msg.role, "content": msg.content}) | ||
|
|
||
| messages.append({"role": "user", "content": request.message}) | ||
|
|
||
| full_content = "" | ||
| async for chunk in provider.stream(messages): | ||
| if chunk.delta: | ||
| full_content += chunk.delta | ||
| yield StreamEvent( | ||
| type="progress", | ||
| data={ | ||
| "delta": chunk.delta, | ||
| "content": full_content, | ||
| "mode": request.mode, | ||
| } | ||
| ) | ||
|
|
||
| yield StreamEvent( | ||
| type="result", | ||
| data={ | ||
| "content": full_content, | ||
| "mode": request.mode, | ||
| "model": request.model, | ||
| } | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| yield StreamEvent(type="error", message=f"API error: {str(e)}") | ||
|
|
||
|
|
||
| async def studio_chat_stream(request: StudioChatRequest) -> AsyncGenerator[StreamEvent, None]: | ||
| """Stream studio chat response - tries Claude CLI first, falls back to API.""" | ||
|
|
||
| # Check if Claude CLI is available | ||
| claude_path = find_claude_cli() | ||
|
|
||
| if claude_path: | ||
| # Use Claude CLI | ||
| async for event in stream_claude_cli(request): | ||
| yield event | ||
| else: | ||
| # Fallback to Anthropic API | ||
| yield StreamEvent( | ||
| type="progress", | ||
| data={ | ||
| "phase": "Info", | ||
| "message": "Claude CLI not found, using Anthropic API directly", | ||
| } | ||
| ) | ||
| async for event in stream_anthropic_api(request): | ||
| yield event | ||
|
|
||
|
|
||
| @router.post("/studio/chat") | ||
| async def studio_chat(request: StudioChatRequest): | ||
| """ | ||
| Interactive chat for DeepStudio with Claude CLI integration. | ||
|
|
||
| Modes: | ||
| - Code: Generate/modify code (full tool access) | ||
| - Plan: Create implementation plans (no execution) | ||
| - Ask: Answer questions (no tools) | ||
|
|
||
| Returns Server-Sent Events with streaming text. | ||
| """ | ||
| return StreamingResponse( | ||
| wrap_generator(studio_chat_stream(request), workflow="studio_chat"), | ||
| media_type="text/event-stream", | ||
| headers={ | ||
| "Cache-Control": "no-cache", | ||
| "Connection": "keep-alive", | ||
| }, | ||
| ) | ||
|
|
||
|
|
||
| @router.get("/studio/status") | ||
| async def studio_status(): | ||
| """Check if Claude CLI is available.""" | ||
| claude_path = find_claude_cli() | ||
|
|
||
| if claude_path: | ||
| # Try to get version | ||
| try: | ||
| result = subprocess.run( | ||
| [claude_path, "--version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=5, | ||
| ) | ||
| version = result.stdout.strip() if result.returncode == 0 else "unknown" | ||
| except Exception: | ||
| version = "unknown" | ||
|
|
||
| return { | ||
| "claude_cli": True, | ||
| "claude_path": claude_path, | ||
| "claude_version": version, | ||
| } | ||
|
|
||
| return { | ||
| "claude_cli": False, | ||
| "claude_path": None, | ||
| "claude_version": None, | ||
| "fallback": "anthropic_api", | ||
| } | ||
|
|
||
|
|
||
| @router.get("/studio/cwd") | ||
| async def studio_cwd(): | ||
| """Get the current working directory for Claude CLI session.""" | ||
| # Default to user's home directory or a sensible default | ||
| default_cwd = os.path.expanduser("~") | ||
|
|
||
| # Try to get the current working directory | ||
| cwd = os.getcwd() | ||
|
|
||
| # Check if we're in a reasonable project directory | ||
| # If we're in the PaperBot source directory, suggest a better location | ||
| if "PaperBot" in cwd or "paperbot" in cwd.lower(): | ||
| # Suggest a projects directory instead | ||
| projects_dir = os.path.expanduser("~/Projects") | ||
| if os.path.isdir(projects_dir): | ||
| suggested_cwd = projects_dir | ||
| else: | ||
| suggested_cwd = os.path.expanduser("~/Documents") | ||
| else: | ||
| suggested_cwd = cwd | ||
|
|
||
| return { | ||
| "cwd": suggested_cwd, | ||
| "actual_cwd": cwd, | ||
| "home": default_cwd, | ||
| "source": "system", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing tests for new API endpoints and streaming logic.
This file introduces three new endpoints and complex streaming behavior (CLI subprocess + API fallback). As per coding guidelines, behavior changes in src/**/*.py should have corresponding tests. Consider adding tests for:
find_claude_cli()with mocked pathsget_mode_flag()/get_model_id()mapping correctnessstream_claude_cli()with a mocked subprocessstream_anthropic_api()with a mocked provider- Endpoint integration tests for
/studio/chat,/studio/status,/studio/cwd
As per coding guidelines, "{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests".
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 238-238: Do not catch blind exception: Exception
(BLE001)
[warning] 241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 310-310: Do not catch blind exception: Exception
(BLE001)
[warning] 311-311: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[error] 367-367: subprocess call: check for execution of untrusted input
(S603)
[warning] 374-374: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/studio_chat.py` around lines 1 - 417, Add unit and
integration tests covering the new endpoints and streaming logic: write unit
tests for find_claude_cli() (mock shutil.which/os.path), get_mode_flag() and
get_model_id() mappings, stream_claude_cli() (mock
asyncio.create_subprocess_exec to simulate stdout/stderr chunks and
returncodes), and stream_anthropic_api() (mock AnthropicProvider.stream to yield
chunks); add endpoint tests for /studio/chat (use FastAPI TestClient and
wrap_generator behavior or mock streaming), /studio/status (mock subprocess.run
and find_claude_cli()), and /studio/cwd (verify suggested_cwd logic); ensure
tests import and exercise functions/routers (studio_chat_stream,
stream_claude_cli, stream_anthropic_api, studio_status, studio_cwd,
find_claude_cli) and use fixtures/mocks to avoid real CLI/API calls and
filesystem side effects.
| router = APIRouter() | ||
|
|
||
| Mode = Literal["Code", "Plan", "Ask"] | ||
| Model = Literal["claude-sonnet-4-5", "claude-opus-4-5", "claude-haiku-4-5"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude model names Anthropic 2025 claude-sonnet claude-opus claude-haiku
💡 Result:
In Anthropic’s Claude lineup, Opus / Sonnet / Haiku are the family tiers (most capable → balanced → fastest). In the API, you generally use versioned model IDs (or “*-latest” aliases), not bare claude-opus / claude-sonnet / claude-haiku. [1][2]
Common Claude model name patterns (as used in 2025)
1) Fully versioned API model IDs (recommended for stability)
claude-opus-4-20250514(Opus 4) [1]claude-sonnet-4-20250514(Sonnet 4) [1]claude-3-7-sonnet-20250219(Sonnet 3.7) [1]claude-3-5-sonnet-20241022(Sonnet 3.5) [1]claude-3-5-haiku-20241022(Haiku 3.5) [1]
2) Moving “latest” aliases (convenient, but can change what they point to)
claude-3-7-sonnet-latest[1]claude-3-5-sonnet-latest[1]claude-3-5-haiku-latest[1]
3) Short names like opus, sonnet, haiku
These are tool/app aliases (e.g., Claude Code config), not the raw API model IDs:
opus→ “currently Opus 4.1” (per that doc)sonnet→ “currently Sonnet 4”haiku→ Haiku tier [3]
If you tell me where you’re using Claude (Anthropic API vs Claude Code vs Bedrock/Vertex), I can list the exact identifiers you should paste for that environment.
Update Claude model identifiers to match Anthropic API naming conventions.
The model names claude-sonnet-4-5, claude-opus-4-5, and claude-haiku-4-5 are not valid Anthropic API identifiers. The -4-5 suffix does not exist in Anthropic's model naming scheme. Use documented model IDs like claude-opus-4-20250514, claude-sonnet-4-20250514, or claude-3-5-haiku-20241022 instead, or use -latest aliases such as claude-3-7-sonnet-latest. The dated variants in the code (e.g., claude-sonnet-4-5-20250514) are also malformed and will fail at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/studio_chat.py` at line 27, The Model literal
currently lists invalid Anthropic IDs ("claude-sonnet-4-5", "claude-opus-4-5",
"claude-haiku-4-5"); replace those strings in the Model type definition with
valid Anthropic model identifiers or latest aliases (e.g.,
"claude-opus-4-20250514", "claude-sonnet-4-20250514",
"claude-3-5-haiku-20241022", or "claude-3-7-sonnet-latest") so runtime calls
using Model in src/paperbot/api/routes/studio_chat.py match Anthropic's naming
conventions and will not fail. Ensure any other usages expecting the old names
are updated to the chosen valid IDs.
| def get_mode_flag(mode: Mode) -> str: | ||
| """Map mode to Claude CLI permission flag.""" | ||
| if mode == "Code": | ||
| return "--dangerously-skip-permissions" # Allow all operations | ||
| elif mode == "Plan": | ||
| return "--plan" # Planning mode - no execution | ||
| else: # Ask | ||
| return "" # Default - no tools |
There was a problem hiding this comment.
--dangerously-skip-permissions is a significant security risk for a web-exposed API.
In "Code" mode, this flag tells Claude CLI to skip all permission prompts, allowing arbitrary file edits and command execution on the server. Since this endpoint is HTTP-accessible, any authenticated (or unauthenticated?) client can trigger unrestricted file system operations and command execution on the host.
At minimum:
- Add authentication/authorization checks on this endpoint.
- Restrict
project_dirto an allow-listed set of directories. - Consider whether this flag is truly needed or if a sandboxed alternative exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/studio_chat.py` around lines 69 - 76, The
get_mode_flag function currently returns the dangerous
"--dangerously-skip-permissions" flag for Mode "Code"; remove or never emit that
flag and instead require an explicit authorization check before enabling any
elevated mode: add an authz gate in the studio chat endpoint that calls a
central permission check (e.g., is_authorized_for_code_mode or similar) and only
then allow a safer, limited flag or none at all; additionally enforce that any
project_dir passed into the studio chat handler is validated against a
maintained allow-list of paths (reject or canonicalize and compare to allowed
roots) and document replacing the flag with a sandboxed execution alternative
(container/worker/sandboxed runner) if true file/command execution is
needed—update get_mode_flag, the studio chat handler, and any callers to reflect
these checks.
| try: | ||
| # Set working directory | ||
| cwd = request.project_dir or os.getcwd() | ||
| if not os.path.isdir(cwd): | ||
| cwd = os.getcwd() |
There was a problem hiding this comment.
Path traversal: project_dir is user-controlled with no validation.
A caller can set project_dir to any path (e.g., /etc, /root, ../../sensitive) and the subprocess will execute with that as cwd. Combined with --dangerously-skip-permissions, this grants arbitrary filesystem access.
Validate project_dir against an allow-list or confine it under a known safe base directory:
🛡️ Proposed fix
# Set working directory
- cwd = request.project_dir or os.getcwd()
- if not os.path.isdir(cwd):
- cwd = os.getcwd()
+ cwd = os.getcwd()
+ if request.project_dir:
+ resolved = os.path.realpath(request.project_dir)
+ allowed_base = os.path.realpath(os.path.expanduser("~/Projects"))
+ if resolved.startswith(allowed_base) and os.path.isdir(resolved):
+ cwd = resolved
+ else:
+ yield StreamEvent(
+ type="error",
+ message=f"project_dir must be under {allowed_base}"
+ )
+ return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/studio_chat.py` around lines 166 - 170, The code
reads a user-controlled request.project_dir into cwd without validation (cwd
used as subprocess working directory), allowing path traversal; to fix, validate
and canonicalize request.project_dir before using it: resolve with
os.path.realpath, ensure it exists and is a directory, then verify the resolved
path is within a configured safe base directory (e.g., compare prefixes against
SAFE_BASE_DIR_REALPATH) or check against an explicit allow-list; if validation
fails, fall back to os.getcwd() and log a warning; update the code paths around
cwd and request.project_dir to use these checks so subprocesses never run with
an arbitrary user-supplied cwd.
| async def read_stream(): | ||
| nonlocal full_content | ||
| while True: | ||
| # Read in chunks for smoother streaming | ||
| chunk = await process.stdout.read(100) | ||
| if not chunk: | ||
| break | ||
|
|
||
| text = chunk.decode("utf-8", errors="replace") | ||
|
|
||
| # Skip ANSI escape codes | ||
| import re | ||
| text = re.sub(r'\x1B\[[0-9;]*[a-zA-Z]', '', text) | ||
|
|
||
| if text: | ||
| full_content += text | ||
| yield StreamEvent( | ||
| type="progress", | ||
| data={ | ||
| "delta": text, | ||
| "content": full_content, | ||
| "mode": request.mode, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
import re inside the read loop — move to module level.
Line 195 re-imports the re module on every chunk read iteration. While Python caches module imports, this is still unnecessary overhead in a hot loop and a code smell.
♻️ Proposed fix
import asyncio
import json
import os
+import re
import shutil
import subprocessThen at Line 195-196:
- import re
- text = re.sub(r'\x1B\[[0-9;]*[a-zA-Z]', '', text)
+ text = re.sub(r'\x1B\[[0-9;]*[a-zA-Z]', '', text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/paperbot/api/routes/studio_chat.py` around lines 184 - 207, The inline
"import re" inside the read_stream loop is unnecessary; add "import re" at
module-level (top of src/paperbot/api/routes/studio_chat.py) and remove the
in-loop import in the read_stream coroutine so the chunk decoding/ANSI-stripping
uses the top-level re; update only the import location and delete the two lines
"import re" and the redundant blank so full_content accumulation and
StreamEvent(delta=text, content=full_content, mode=request.mode) behavior
remains unchanged.
| const runPaper2Code = async () => { | ||
| if (!selectedPaper || !canRun || isBusy) return | ||
|
|
||
| setStatus("running") | ||
| setLastError(null) | ||
|
|
||
| if (selectedPaperId) { | ||
| updatePaper(selectedPaperId, { status: 'generating' }) | ||
| } | ||
|
|
||
| const taskId = addTask(`Paper2Code — ${selectedPaper.title.slice(0, 40)}${selectedPaper.title.length > 40 ? "…" : ""}`) | ||
| addAction(taskId, { type: "thinking", content: "Starting Paper2Code run…" }) | ||
| runIdRef.current = null | ||
| let outputDir: string | undefined | ||
|
|
||
| try { | ||
| const res = await fetch("/api/gen-code", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| title: selectedPaper.title, | ||
| abstract: selectedPaper.abstract, | ||
| method_section: selectedPaper.methodSection || undefined, | ||
| use_orchestrator: true, | ||
| use_rag: true, | ||
| }), | ||
| }) | ||
|
|
||
| if (!res.ok || !res.body) { | ||
| throw new Error(`Failed to start run (${res.status})`) | ||
| } | ||
|
|
||
| updateTaskStatus(taskId, "running") | ||
|
|
||
| for await (const evt of readSSE(res.body)) { | ||
| if (evt?.type === "progress") { | ||
| const data = (evt.data ?? {}) as { phase?: string; message?: string; run_id?: string } | ||
| if (data.run_id && !runIdRef.current) { | ||
| runIdRef.current = data.run_id | ||
| } | ||
| addAction(taskId, { | ||
| type: "thinking", | ||
| content: `${data.phase ? `[${data.phase}] ` : ""}${data.message || "Working…"}`, | ||
| }) | ||
| } else if (evt?.type === "result") { | ||
| const result = evt.data as GenCodeResult | ||
| outputDir = result?.outputDir | ||
| setLastGenCodeResult(result) | ||
| addAction(taskId, { type: "complete", content: "Run completed" }) | ||
| updateTaskStatus(taskId, "completed") | ||
| if (selectedPaperId && outputDir) { | ||
| updatePaper(selectedPaperId, { status: 'ready', outputDir }) | ||
| } | ||
| setStatus("success") | ||
| } else if (evt?.type === "error") { | ||
| addAction(taskId, { type: "error", content: evt.message || "Run failed" }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(evt.message || "Run failed") | ||
| setStatus("error") | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| const message = e instanceof Error ? e.message : String(e) | ||
| addAction(taskId, { type: "error", content: message }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(message) | ||
| setStatus("error") | ||
| } | ||
| } |
There was a problem hiding this comment.
No cancellation mechanism for SSE streams — risk of state updates after unmount and leaked connections.
Both runPaper2Code and handleSendMessage issue fetch calls and iterate over SSE streams without an AbortController. If the user navigates away or the component unmounts mid-stream, the loop continues running and calls setStatus, addAction, etc. on stale/unmounted state.
🔧 Suggested fix — add AbortController
Add a ref at the component level and wire it into both async functions:
+ const abortRef = useRef<AbortController | null>(null)
+
+ useEffect(() => {
+ return () => { abortRef.current?.abort() }
+ }, [])Then in each fetch call:
+ abortRef.current?.abort()
+ const controller = new AbortController()
+ abortRef.current = controller
const res = await fetch("/api/gen-code", {
method: "POST",
headers: { "Content-Type": "application/json" },
+ signal: controller.signal,
body: JSON.stringify({ ... }),
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` around lines 299 - 370, The
SSE loops in runPaper2Code (and handleSendMessage) lack cancellation and can
continue updating state after unmount; add a component-level AbortController ref
(e.g., sseAbortRef) that you create/reset before each fetch, pass its signal
into fetch calls, and call sseAbortRef.current.abort() on component unmount (and
before starting a new run) to cancel the stream; inside the for-await-of readSSE
loop check the controller.signal.aborted (or catch the aborted fetch error) to
break early and avoid calling setStatus/addAction/updatePaper when aborted, and
ensure your catch/finally handles aborts without marking the task as an error.
| const safeName = paper.title | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, "-") | ||
| .slice(0, 40) | ||
| setCustomDir(`${dir}/${safeName}`) |
There was a problem hiding this comment.
safeName produces an empty string for non-Latin paper titles.
The regex /[^a-z0-9]+/g strips all non-ASCII characters. For Chinese (or other non-Latin) paper titles — which this project clearly supports — the result will be "" or "-", producing paths like /home/user/ or /home/user/-.
Proposed fix — fall back to a hash or generic name
- const safeName = paper.title
- .toLowerCase()
- .replace(/[^a-z0-9]+/g, "-")
- .slice(0, 40)
- setCustomDir(`${dir}/${safeName}`)
+ const safeName = paper.title
+ .toLowerCase()
+ .replace(/[^a-z0-9\u4e00-\u9fff]+/g, "-")
+ .replace(/^-+|-+$/g, "")
+ .slice(0, 40)
+ setCustomDir(`${dir}/${safeName || "paper-workspace"}`)Alternatively, use a short hash of the title as a reliable fallback for any locale.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const safeName = paper.title | |
| .toLowerCase() | |
| .replace(/[^a-z0-9]+/g, "-") | |
| .slice(0, 40) | |
| setCustomDir(`${dir}/${safeName}`) | |
| const safeName = paper.title | |
| .toLowerCase() | |
| .replace(/[^a-z0-9\u4e00-\u9fff]+/g, "-") | |
| .replace(/^-+|-+$/g, "") | |
| .slice(0, 40) | |
| setCustomDir(`${dir}/${safeName || "paper-workspace"}`) |
🤖 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 49 - 53,
safeName generation strips non-ASCII characters (using
paper.title.toLowerCase().replace(/[^a-z0-9]+/g, "-").slice(0, 40)) which yields
empty or "-" for non-Latin titles; update the logic in WorkspaceSetupDialog
where safeName is computed so that after creating safeName from paper.title you
check if it's empty or only separators and, if so, replace it with a
deterministic short fallback (e.g., a short hash derived from paper.title or a
generic name like "paper-{shortHash}"), then call
setCustomDir(`${dir}/${fallbackOrSafeName}`) — reference the safeName variable,
paper.title, and setCustomDir to locate and modify the code.
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/src/lib/store/project-context.ts (3)
17-17:⚠️ Potential issue | 🟡 Minor
setActiveFilecannot clear the active file — type gap with nullableactiveFile.
activeFileis typedstring | null(line 12), butsetActiveFileonly acceptsstring. There is no way for consumers to reset the active file tonull(e.g., afterremoveFileempties the store). Consider widening the action signature.♻️ Proposed fix
In the interface:
- setActiveFile: (name: string) => void + setActiveFile: (name: string | null) => voidIn the implementation:
- setActiveFile: (name) => set({ activeFile: name }), + setActiveFile: (name) => set({ activeFile: name }), // name may be nullAlso applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/project-context.ts` at line 17, The action signature for setActiveFile doesn’t allow clearing the nullable activeFile; change its type from (name: string) => void to (name: string | null) => void (or name?: string | null) in the interface and update the corresponding implementation of setActiveFile to accept and handle null by setting activeFile = null (and ensure callers that clear a file call setActiveFile(null)); update any usages/tests accordingly so clearing is possible after removeFile.
33-38:⚠️ Potential issue | 🟡 Minor
updateFilesilently corrupts state when called with an unknown filename.If
state.files[name]isundefined(file doesn't exist),{ ...undefined, content }is valid JS and produces{ content }— aVirtualFilemissing itsnameandlanguagefields. This is a silent corruption with no error or guard.🛡️ Proposed fix
updateFile: (name, content) => set((state) => ({ + ...(name in state.files && { files: { ...state.files, [name]: { ...state.files[name], content } } + }) })),Or with an explicit guard:
updateFile: (name, content) => set((state) => { + if (!(name in state.files)) return state; return { files: { ...state.files, [name]: { ...state.files[name], content } } }; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/project-context.ts` around lines 33 - 38, The updateFile updater silently corrupts files when called with an unknown name because it spreads state.files[name] even if undefined; modify updateFile to first check existence of state.files[name] (the files map / VirtualFile entry) and either throw or return the unchanged state when missing, e.g. guard inside updateFile before spreading, or create a proper default VirtualFile object with required fields (name, language, content) only after validating the key; reference updateFile and state.files in your change.
21-50:⚠️ Potential issue | 🔴 CriticalDefine
DEFAULT_FILESconstant and add tests for the store.
DEFAULT_FILESis referenced on line 22 but is not defined, imported, or exported anywhere. This will cause aReferenceErrorat runtime. Additionally, there are no TypeScript tests for this Zustand store. Per coding guidelines, behavior should be tested.Required:
- Define and export
DEFAULT_FILES(e.g.,export const DEFAULT_FILES = {}or with initial files)- Add tests for
useProjectContextcovering the initial state,addFile,updateFile,setActiveFile, andremoveFileactions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/store/project-context.ts` around lines 21 - 50, Define and export a DEFAULT_FILES constant and add unit tests for the Zustand store: create and export e.g. export const DEFAULT_FILES = { /* initial file(s) or {} */ } in the same module so useProjectContext can reference it without a runtime ReferenceError, then add TypeScript tests for useProjectContext that assert the initial state equals DEFAULT_FILES and activeFile is null (or expected), and cover actions addFile, updateFile, setActiveFile, and removeFile (resetting or recreating the store between tests to avoid state bleed); reference the symbols useProjectContext, DEFAULT_FILES, addFile, updateFile, setActiveFile, and removeFile when locating code to change and tests to implement.src/paperbot/repro/agents/coding_agent.py (1)
59-73:⚠️ Potential issue | 🟡 MinorAdd tests for
CodingAgent.execute()to verify theAnalysisNode.run()tuple input.The analysis step now receives
(paper_context, plan)as a tuple instead of individual arguments. Existing or future tests mockingAnalysisNode.run()must expect this tuple signature, or they risk failing silently or providing false confidence. This is required per the coding guideline: "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/repro/agents/coding_agent.py` around lines 59 - 73, Tests for CodingAgent.execute need to be updated/added so they assert AnalysisNode.run is called with a single tuple (paper_context, plan) rather than separate args; modify or add unit tests that mock self.analysis_node.run in CodingAgent.execute (reference CodingAgent.execute and AnalysisNode.run) to expect a single positional argument equal to (paper_context, plan), and update any existing mocks/stubs/fixtures to match this tuple signature and validate it is passed during the "analysis" step.
🟠 Major comments (18)
web/next.config.ts-5-12 (1)
5-12:⚠️ Potential issue | 🟠 MajorHardcoded
localhost:8000destination will break every non-local deployment.
rewrites()executes on the server in all environments (development, preview, production). When the app is deployed to any cloud host (e.g. Vercel),http://localhost:8000resolves to the deployment container itself — no backend will be listening there, so every/api/*call will fail. At minimum, gate the rule onNODE_ENVor drive the target from an environment variable.🛠️ Proposed fix — environment-variable-backed URL with dev fallback
- async rewrites() { - return [ - { - source: '/api/:path*', - destination: 'http://localhost:8000/api/:path*', - }, - ] - }, + async rewrites() { + // Only proxy in development; in production the backend URL must be set explicitly. + const backendUrl = process.env.BACKEND_URL ?? 'http://localhost:8000'; + if (!process.env.BACKEND_URL && process.env.NODE_ENV !== 'development') { + return []; + } + return [ + { + source: '/api/:path*', + destination: `${backendUrl}/api/:path*`, + }, + ]; + },Alternatively, if the proxy is strictly a dev-only concern, a simpler guard suffices:
- async rewrites() { - return [ - { - source: '/api/:path*', - destination: 'http://localhost:8000/api/:path*', - }, - ] - }, + async rewrites() { + if (process.env.NODE_ENV !== 'development') return []; + return [ + { + source: '/api/:path*', + destination: 'http://localhost:8000/api/:path*', + }, + ]; + },Add
BACKEND_URL=https://your-api-host.example.comto.env.production(or the hosting provider's environment secrets) when deploying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/next.config.ts` around lines 5 - 12, The rewrite uses a hardcoded destination ('http://localhost:8000/api/:path*') in the rewrites() export which will break non-local deployments; change rewrites() to select the backend target from an environment variable (e.g., process.env.BACKEND_URL) with a fallback to the localhost dev URL only when NODE_ENV === 'development' (or gate the entire rule to dev), and ensure the returned rewrite uses the computed target for '/api/:path*' instead of the hardcoded string so production picks up the configured BACKEND_URL.src/paperbot/api/routes/sandbox.py-413-419 (1)
413-419:⚠️ Potential issue | 🟠 MajorUnauthenticated endpoint that starts system-level processes.
This endpoint spawns Docker Desktop or the Docker daemon with no visible authorization check. If the API is exposed beyond
localhost, any caller can trigger process execution on the host. Ensure this is gated behind authentication or restricted to local-only access.Also, the docstring says "macOS/Windows only" but the handler also covers Linux.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 413 - 419, The start_docker request handler (router.post "/sandbox/docker/start", function start_docker) is an unauthenticated endpoint that can spawn system processes and the docstring incorrectly omits Linux; fix it by gating execution with an authorization check (e.g., require an auth dependency like a current_user or role check) and/or explicitly restrict access to localhost by validating the incoming request origin (inspect Request.client.host and deny non-local addresses) before any process spawn, and update the docstring to list macOS/Windows/Linux as appropriate and document the access restriction.src/paperbot/api/routes/sandbox.py-329-344 (1)
329-344:⚠️ Potential issue | 🟠 MajorAPI key stored in plaintext with no file-permission restrictions.
_save_settingswrites the E2B API key as plain JSON. The file inherits the default umask, so on shared systems other users may be able to read it. Restrict permissions when creating the file.🔒 Proposed fix
def _save_settings(settings: Dict[str, Any]) -> None: """Save settings to file.""" import json SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True) - SETTINGS_FILE.write_text(json.dumps(settings, indent=2)) + import stat + SETTINGS_FILE.write_text(json.dumps(settings, indent=2)) + SETTINGS_FILE.chmod(stat.S_IRUSR | stat.S_IWUSR) # 600 — owner only🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 329 - 344, The settings file is written in plaintext without restrictive permissions; update _save_settings (and optionally _load_settings) to write atomically and set restrictive file permissions (owner read/write only) after creation: create the parent directory if needed, write JSON to a temporary file, fsync/flush, atomically replace SETTINGS_FILE (e.g., os.replace), and call os.chmod(SETTINGS_FILE, 0o600) to ensure owner-only access; reference the SETTINGS_FILE constant and the _save_settings/_load_settings functions so the change is applied where settings are written and read.src/paperbot/api/routes/sandbox.py-472-497 (1)
472-497:⚠️ Potential issue | 🟠 Major
subprocess.runblocks the async event loop on Linux.
subprocess.runis synchronous. Inside anasync defendpoint it will freeze the entire FastAPI event loop untilsystemctlcompletes (which can take seconds when starting a service). Useasyncio.create_subprocess_execor wrap it withasyncio.to_thread.⚡ Proposed fix using asyncio
+ import asyncio elif system == "Linux": - try: - result = subprocess.run( - ["systemctl", "start", "docker"], - capture_output=True, - text=True, - ) + try: + proc = await asyncio.create_subprocess_exec( + "systemctl", "start", "docker", + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await proc.communicate() + if proc.returncode == 0: - if result.returncode == 0: return { "status": "starting", "message": "Docker daemon is starting.", "platform": "Linux", } else: + err_msg = stderr.decode().strip() if stderr else "unknown error" return { "status": "error", - "message": f"Failed to start Docker: {result.stderr}. Try 'sudo systemctl start docker'", + "message": ( + f"Failed to start Docker: {err_msg}. " + "Try 'sudo systemctl start docker'" + ), "platform": "Linux", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 472 - 497, The Linux branch currently uses subprocess.run (inside the system == "Linux" block), which blocks the async event loop; replace the synchronous call in that block by running the command asynchronously—either use asyncio.create_subprocess_exec to spawn "systemctl start docker" and await its completion (capture stdout/stderr via process.communicate()) or call subprocess.run inside asyncio.to_thread and await that; update the return logic to check the awaited result.returncode and use result.stderr/result.exception text as before, and keep the same returned dict keys ("status","message","platform") and exception handling flow around the async call.src/paperbot/api/routes/sandbox.py-347-502 (1)
347-502: 🛠️ Refactor suggestion | 🟠 MajorAdd tests for the new sandbox endpoints (E2B settings and Docker start).
The new E2B settings endpoints (GET/POST/DELETE) and Docker start endpoint introduce significant behavior changes including file I/O, environment variable mutations, and cross-platform subprocess execution, but no corresponding tests are included. Per coding guidelines, "If behavior changes, add or update tests" (
{src,web/src}/**/*.{py,ts,tsx}).Add integration tests covering:
- E2B settings CRUD operations (read, save, delete from persistent storage)
- Environment variable state after operations
- Docker start across platforms (macOS, Windows, Linux)
- Error handling for missing/invalid paths
src/paperbot/repro/e2b_executor.py-190-200 (1)
190-200: 🛠️ Refactor suggestion | 🟠 MajorSandbox creation logic duplicated —
_get_sandboxis bypassed.
run()(lines 196-200) andrun_code()(lines 317-320) both duplicate theSandbox.create()→ fallback pattern already implemented in_get_sandbox(). This means any fix to sandbox creation must be applied in three places. Consider reusing_get_sandbox()or extracting a shared factory method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/e2b_executor.py` around lines 190 - 200, The sandbox creation pattern is duplicated in run() and run_code() bypassing the existing _get_sandbox(); remove the inline Sandbox.create()/fallback blocks in both run() and run_code() and call the shared _get_sandbox() helper instead (or extract a single factory method and call it from all three places) so sandbox creation/timeout/env-key logic is centralized; ensure _get_sandbox() accepts or uses the same timeout and api_key behavior (setting E2B_API_KEY if needed) so behavior remains identical after the refactor.src/paperbot/repro/e2b_executor.py-148-150 (1)
148-150:⚠️ Potential issue | 🟠 MajorBinary files silently corrupted by
read_text().
file_path.read_text(errors="ignore")andfs.write(sandbox_path, content)treat all files as text. Binary files (images,.pkl,.h5, compiled extensions, etc.) will be corrupted. Consider usingread_bytes()and a binary write path, or at minimum filtering out common binary extensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/e2b_executor.py` around lines 148 - 150, The code is treating all files as text which corrupts binaries: replace the text-only operations (file_path.read_text(errors="ignore") and fs.write(sandbox_path, content)) with binary-safe reads/writes—use file_path.read_bytes() and write the bytes to the sandbox (or add a binary branch) or alternatively filter out known binary extensions before using read_text; update the upload logic in e2b_executor (the block that builds content and calls fs.write) to handle bytes vs str correctly so images/.pkl/.h5/compiled files are not corrupted.src/paperbot/repro/e2b_executor.py-96-109 (1)
96-109:⚠️ Potential issue | 🟠 MajorThread-unsafe: mutating
os.environfor API key.Lines 98-99 (and duplicated at lines 192-193 and 312-314) set
os.environ["E2B_API_KEY"]as a side effect. This is global mutable state — if multipleE2BExecutorinstances with different API keys are used concurrently (e.g., in an async web server), they will overwrite each other's key. Consider passing the key directly to the SDK constructor or using a thread-local approach instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/e2b_executor.py` around lines 96 - 109, The code mutates global os.environ["E2B_API_KEY"] which is thread-unsafe; instead, avoid setting environment variables in E2BExecutor and pass the API key directly to the SDK constructors: call Sandbox.create with an explicit api_key parameter when supported (or detect support and pass api_key), and keep the existing fallback to Sandbox(api_key=self.api_key, timeout=self.timeout_sandbox). Remove all assignments to os.environ["E2B_API_KEY"] (including duplicates around lines where Sandbox.create/Sandbox are invoked) so each E2BExecutor uses its own key without global state.web/src/components/studio/RunbookPanel.tsx-167-171 (1)
167-171:⚠️ Potential issue | 🟠 MajorStep marked as "success" when status poll fails.
If the status response returns a non-OK HTTP status (e.g., 500, 404), the
elsebranch on line 167 silently marks the step as"success". This masks failures — a server error during status polling shouldn't be treated as a successful step.🐛 Proposed fix
} else { - setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) - updateTaskStatus(taskId, "completed") - return { ok: true } + const message = `Failed to poll status for ${stepName} (HTTP ${statusRes.status})` + setStepStatuses((prev) => ({ ...prev, [stepName]: "error" })) + updateTaskStatus(taskId, "error") + addAction(taskId, { type: "error", content: message }) + setLastError(message) + return { ok: false, error: message } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/RunbookPanel.tsx` around lines 167 - 171, The current else branch unconditionally marks the step success even when the status poll returns a non-OK HTTP response; instead, in the status-polling flow where setStepStatuses and updateTaskStatus are used (referencing setStepStatuses, updateTaskStatus, taskId, stepName), check the HTTP response.ok (or response.status) and treat non-OK responses as failures: set the step status to "error" (or an appropriate failure state), call updateTaskStatus(taskId, "failed") (or similar), log or surface the response error, and return a failure object (e.g., { ok: false, error: ... }) rather than marking success. Ensure no code path marks success unless the response is explicitly OK and parsed as a successful result.src/paperbot/api/routes/gen_code.py-150-165 (1)
150-165:⚠️ Potential issue | 🟠 MajorInconsistent defensive attribute access — potential
AttributeError.Line 152 uses
hasattr(result, "blueprint")defensively, but lines 157, 162, and 164 accessresult.spec,result.verification_results, andresult.verificationdirectly withouthasattrguards. If theReproductionResultobject lacks any of these attributes (e.g., due to an older code path or a partial result), this will raise anAttributeErrorinside the generator, which gets caught by the broadexcepton line 178 and surfaces as a generic error — hiding the real issue.Additionally, line 165 calls
.get("all_passed", False)assumingresult.verificationis a dict, but if it's a structured object, this will fail withAttributeError.🛡️ Proposed fix: add consistent hasattr guards
# Extract blueprint info from plan/spec if available blueprint_info = {"architectureType": "unknown", "domain": "unknown"} if hasattr(result, "blueprint") and result.blueprint: blueprint_info = { "architectureType": getattr(result.blueprint, "architecture_type", "unknown"), "domain": getattr(result.blueprint, "domain", "unknown"), } - elif result.spec and result.spec.model_type: - blueprint_info["architectureType"] = result.spec.model_type + elif hasattr(result, "spec") and result.spec and getattr(result.spec, "model_type", None): + blueprint_info["architectureType"] = result.spec.model_type # Check verification status verification_passed = False - if result.verification_results: + if getattr(result, "verification_results", None): verification_passed = all(v.passed for v in result.verification_results) - elif result.verification: - verification_passed = result.verification.get("all_passed", False) + elif getattr(result, "verification", None): + v = result.verification + if isinstance(v, dict): + verification_passed = v.get("all_passed", False) + else: + verification_passed = getattr(v, "all_passed", False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/gen_code.py` around lines 150 - 165, The code accesses result.spec, result.verification_results, and result.verification without defensive checks which can raise AttributeError; update the block that builds blueprint_info and computes verification_passed to use hasattr/result attribute checks similar to the existing hasattr(result, "blueprint") guard (refer to blueprint_info and verification_passed variables and the ReproductionResult instance "result"), and when handling result.verification, accept both dict and object shapes by first testing isinstance(result.verification, dict) and using .get("all_passed", False) only for dicts, otherwise access a boolean attribute like getattr(result.verification, "all_passed", False); ensure all direct accesses to result.spec, result.verification_results, and result.verification are protected with hasattr/getattr to avoid AttributeError.web/src/components/studio/NewPaperModal.tsx-110-127 (1)
110-127:⚠️ Potential issue | 🟠 Major"Already in studio" dedup logic will never match — ID spaces are disjoint.
Library papers get their IDs from the API (e.g.,
item.paper.id), butaddPaperin the studio store generates a new UUID viagenerateId(). The original API ID is not persisted on theStudioPaper. Consequently:
studioPaperIds(Line 149) contains generated UUIDs, never matching library paper IDs.existingIds.has(paperId)at Line 115 will always befalse.- The "Already in studio" indicator at Line 225 will never display.
Users can inadvertently import the same paper multiple times. Consider storing the library/source paper ID on
StudioPaper(e.g.,sourcePaperId) and checking against that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/NewPaperModal.tsx` around lines 110 - 127, The dedup logic in handleImportSelected fails because studio items get new UUIDs via addPaper/generateId so existingIds (studioPaperIds) never match library paper IDs; modify the StudioPaper model to include a sourcePaperId (or similar), update addPaper (or its call site in handleImportSelected) to pass sourcePaperId: paper.id when importing, and change the dedupe check to build existingIds from studioPapers.map(p => p.sourcePaperId) (and also update the "Already in studio" indicator to check sourcePaperId) so library IDs correctly detect already-imported papers.src/paperbot/repro/openai_ci_executor.py-374-391 (1)
374-391:⚠️ Potential issue | 🟠 MajorPotential
NameErrorforclientinfinallyblock.If
_openai_client()raises an exception that is not aRuntimeError(e.g., an unexpectedTypeError), theexcept RuntimeErrorat Line 208 won't catch it, so execution falls to the outerexcept Exceptionat Line 365. In thefinallyblock,clientis referenced at Line 376 but was never bound — causing anUnboundLocalError.🐛 Proposed fix
+ client = None try: client, api_key, base_url = _openai_client()Or initialize
client = Nonebefore the outertry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/openai_ci_executor.py` around lines 374 - 391, Initialize client to None before the outer try (or ensure the finally block checks "if client is not None") to avoid an UnboundLocalError when _openai_client() raises an unexpected exception; update the code around the outer try (where _openai_client() is called) to set client = None at function scope and keep the existing finally cleanup using "if client" (or change to "if client is not None") when calling client.beta.assistants.delete, client.beta.threads.delete, and client.files.delete to ensure client is safely referenced.src/paperbot/api/routes/runbook.py-1392-1401 (1)
1392-1401:⚠️ Potential issue | 🟠 MajorUnsanitized
checkpoint_pathinterpolated into shell command.
body.checkpoint_pathis user-supplied and directly interpolated into a shell command string at Line 1398. While this eventually runs inside a sandboxed environment (Docker/E2B/OpenAI CI), a malicious value like; rm -rf /could still cause damage within the sandbox or exfiltrate data.Use
shlex.quote()to sanitize:🔒 Proposed fix
if body.checkpoint_path: - commands.append(f"{base} --checkpoint {body.checkpoint_path}") + commands.append(f"{base} --checkpoint {shlex.quote(body.checkpoint_path)}")🤖 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 1392 - 1401, Sanitize the user-supplied checkpoint path before interpolating into the shell command in _build_eval_commands: when building the "--checkpoint" argument from body.checkpoint_path, wrap the value with shlex.quote() (import shlex at top) so the f-string uses a safely quoted value; keep using base (body.command_override or detected.get("command") or "python eval.py") but ensure checkpoint_path is passed through shlex.quote to prevent shell injection.web/src/components/studio/ReproductionLog.tsx-366-371 (1)
366-371:⚠️ Potential issue | 🟠 MajorDefaulting to "success" when the status check fails is incorrect.
When
statusRes.okisfalse(Lines 367–370), the step is marked as"success". A non-OK response means we couldn't verify the outcome — it should not be treated as success. The same pattern appears inrunSmoke(Lines 522–525).🐛 Proposed fix
} else { - setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) - updateTaskStatus(taskId, "completed") - return { ok: true } + const message = `Could not verify ${stepName} status (HTTP ${statusRes.status})` + setStepStatuses((prev) => ({ ...prev, [stepName]: "error" })) + updateTaskStatus(taskId, "error") + addAction(taskId, { type: "error", content: message }) + setLastError(message) + return { ok: false, error: message } }Apply the same fix in
runSmokearound Lines 522–525.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/ReproductionLog.tsx` around lines 366 - 371, The code incorrectly marks a step as "success" when a status response is not OK; in the block that currently calls setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) and updateTaskStatus(taskId, "completed") when statusRes.ok is false, change it to mark the step as failed/unverified (e.g., setStepStatuses(..., [stepName]: "error") or "failed"), call updateTaskStatus(taskId, "failed") (or equivalent failure state), and return { ok: false } so failures are propagated; apply the identical fix inside runSmoke where the same pattern appears.src/paperbot/repro/openai_ci_executor.py-96-107 (1)
96-107:⚠️ Potential issue | 🟠 MajorThread-unsafe mutation of
os.environ.
os.environ.pop/os.environ[...] = ...modifies global process state. If two requests invoke_openai_client()concurrently (which is likely in an async web server), they race on the environment variable, potentially causing one request to use the wrongOPENAI_BASE_URL.Consider passing
base_urldirectly to theOpenAIconstructor without mutating the environment, or use a lock if env mutation is truly necessary.🔒 Proposed fix — avoid env mutation
- # Force remove OPENAI_BASE_URL from env to prevent SDK from using it - # This is important when the env has a different base_url than what we want - env_base_url = os.environ.pop("OPENAI_BASE_URL", None) - if base_url: client = OpenAI(api_key=api_key, base_url=base_url) else: - client = OpenAI(api_key=api_key) - - # Restore env var if it was set (for other code that might need it) - if env_base_url: - os.environ["OPENAI_BASE_URL"] = env_base_url + # Explicitly pass base_url=None to override any env default + client = OpenAI(api_key=api_key, base_url="https://api.openai.com/v1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/repro/openai_ci_executor.py` around lines 96 - 107, The code mutates process-wide os.environ["OPENAI_BASE_URL"] in _openai_client which is thread-unsafe; instead remove the pop/restore and call the OpenAI constructor with base_url explicitly when provided (use OpenAI(api_key=api_key, base_url=base_url) if base_url is set, otherwise OpenAI(api_key=api_key)), and stop modifying os.environ or relying on OPENAI_BASE_URL so concurrent requests don't race.src/paperbot/api/routes/runbook.py-443-476 (1)
443-476:⚠️ Potential issue | 🟠 MajorXSS vulnerability in HTML report generation.
run_id,workdir, and file paths fromreport_dataare interpolated directly into the HTML template without escaping. If any of these contain HTML metacharacters (e.g.,<script>in a filename), they will be rendered as active HTML.Use
html.escape()for all interpolated values:🔒 Proposed fix
+import html + # In the f-string template: - <title>Execution Report - {run_id}</title> + <title>Execution Report - {html.escape(run_id)}</title> ... - <p>Run ID: <code>{run_id}</code></p> - <p>Project: <code>{workdir}</code></p> + <p>Run ID: <code>{html.escape(run_id)}</code></p> + <p>Project: <code>{html.escape(str(workdir))}</code></p> ... - {'<br>'.join(report_data['files'][:50])} + {'<br>'.join(html.escape(f) for f in report_data['files'][:50])}🤖 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 443 - 476, The HTML report in runbook.py interpolates unescaped values (run_id, workdir, report_data['generated_at'], report_data['files'], report_data['logs']) which is an XSS risk; import html and html.escape all user/FS-derived values before assembling the template (e.g. create escaped_run_id = html.escape(run_id), escaped_workdir = html.escape(workdir), escaped_generated_at = html.escape(report_data['generated_at']), and map/escape the file and log lists with escaped_files = [html.escape(x) for x in report_data['files']] and escaped_logs = [html.escape(x) for x in report_data['logs']]); then use these escaped variables in the string joins (respecting the existing slicing/ellipsis logic for the first 50 files).web/src/components/studio/PapersPanel.tsx-112-137 (1)
112-137:⚠️ Potential issue | 🟠 Major
AlertDialogActionauto-closes the dialog, so the async loading state is never visible.Radix UI's
AlertDialogActioncloses the dialog immediately when clicked. SincehandleConfirmDeleteisasync, the dialog dismisses before the fetch completes — the user never sees the<Loader2>spinner or the disabled state. To keep the dialog open during the async operation, calle.preventDefault()on the click event.Proposed fix
- const handleConfirmDelete = async () => { + const handleConfirmDelete = async (e: React.MouseEvent) => { + e.preventDefault() if (!paperToDelete) return setDeleting(true)Also applies to: 265-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/PapersPanel.tsx` around lines 112 - 137, The dialog is auto-closing because AlertDialogAction triggers dismissal before your async work runs; change the click handler signature to accept the click event (e: React.MouseEvent), call e.preventDefault() at the start to stop Radix from auto-closing, then run your existing async logic in handleConfirmDelete (keep setDeleting(true) while awaiting the fetch and deletePaper(paperToDelete.id)), and only call setDeleteConfirmOpen(false) after the async work completes; apply the same change to the other occurrence of AlertDialogAction/handleConfirmDelete usage (around the second instance).web/src/app/studio/page.tsx-20-52 (1)
20-52:⚠️ Potential issue | 🟠 MajorRace condition: URL-param effect can create a duplicate paper before
loadPaperspopulates state.Both effects run after the first render.
loadPapers()(Effect 1) updates the store, but by the time Effect 2 first executes,papersis still the initial empty array. If the URL containspaper_id=Xfor an existing paper, thepapers.find(...)on Line 36 won't find it, soaddPapercreates a duplicate. ThehasProcessedParamsref then prevents the corrected re-run.Consider gating the URL-param processing on a "papers loaded" signal — e.g., a boolean flag set after
loadPaperscompletes, or combining both concerns into a single effect.Sketch: gate on a loaded flag
+ const [papersLoaded, setPapersLoaded] = useState(false) + useEffect(() => { loadPapers() + setPapersLoaded(true) // eslint-disable-next-line react-hooks/exhaustive-deps }, []) useEffect(() => { if (hasProcessedParams.current) return + if (!papersLoaded) return const paperId = searchParams.get("paper_id")🤖 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 20 - 52, The URL-param effect can run before loadPapers finishes causing duplicates; modify the logic so the effect that reads searchParams waits until papers are loaded by introducing a "papersLoaded" boolean set to true when loadPapers completes (or return a Promise/callback from loadPapers), then in the effect that uses hasProcessedParams, searchParams, and papers add a guard like if (!papersLoaded || hasProcessedParams.current) return; and only then run the existing flow (use papers.find(...), selectPaper, addPaper, router.replace). Ensure loadPapers sets papersLoaded after populating papers or combine the loadPapers call and URL-param processing into a single effect that first awaits loadPapers and then handles selectPaper/addPaper and router.replace.
🟡 Minor comments (7)
src/paperbot/api/routes/sandbox.py-486-491 (1)
486-491:⚠️ Potential issue | 🟡 MinorLine 489 exceeds the 100-character limit.
Break the f-string to comply with the Black
line-length = 100guideline.✏️ Proposed fix
else: return { "status": "error", - "message": f"Failed to start Docker: {result.stderr}. Try 'sudo systemctl start docker'", + "message": ( + f"Failed to start Docker: {result.stderr}." + " Try 'sudo systemctl start docker'" + ), "platform": "Linux", }As per coding guidelines, "Use Black formatting with
line-length = 100for Python code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 486 - 491, The return dict's long f-string in the error branch should be broken into multiple parts to satisfy Black's 100-character limit: assign or construct the "message" value using a parenthesized multi-line string (e.g., message = (f"...{result.stderr}..." " more text")) or concatenate two shorter f-strings so the line with the f-string is under 100 chars; update the return in the same block (the dict literal that includes "status", "message", "platform") to use that shorter "message" expression referencing result.stderr.src/paperbot/api/routes/sandbox.py-374-392 (1)
374-392:⚠️ Potential issue | 🟡 MinorNo validation on the API key value — empty or whitespace-only strings are silently accepted.
An empty
api_keywill be persisted and exported toE2B_API_KEY, which will cause silent failures downstream. Add a minimum-length or non-blank check.🛡️ Proposed fix
`@router.post`("/sandbox/settings/e2b") async def save_e2b_settings(body: E2BSettingsRequest, http_request: Request): + if not body.api_key or not body.api_key.strip(): + raise HTTPException(status_code=422, detail="API key must not be empty") + settings = _load_settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/sandbox.py` around lines 374 - 392, The save_e2b_settings endpoint currently accepts empty or whitespace API keys; update the save_e2b_settings function to validate body.api_key (e.g., strip() and require non-blank and a reasonable minimum length such as >8) before calling _load_settings/_save_settings and setting os.environ["E2B_API_KEY"]; if validation fails, raise a FastAPI HTTPException (422 or 400) with a clear error message so invalid keys are not persisted or exported.src/paperbot/api/routes/gen_code.py-170-170 (1)
170-170:⚠️ Potential issue | 🟡 MinorRelaxed success criterion may mask failures.
successisTruewhen status is"completed"or when any files were generated. This means a run that errors out or is marked"failed"but still managed to produce partial files will reportsuccess: true. If this is intentional (partial results are useful), consider adding a separatepartialflag so the frontend can distinguish complete vs. partial success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/paperbot/api/routes/gen_code.py` at line 170, The current response sets "success" true when result.status.value == "completed" OR len(result.generated_files) > 0 which treats partial outputs as full success; change this so "success" is true only when result.status.value == "completed" and introduce a separate "partial" (or "partial_success") boolean set to True when len(result.generated_files) > 0 but status is not "completed" (i.e., result.generated_files indicates partial output); update the dict keys around "success" and add the new "partial" key wherever the response dict is constructed (reference result.status.value, result.generated_files, and the existing "success" key).web/src/components/studio/RunbookPanel.tsx-262-262 (1)
262-262:⚠️ Potential issue | 🟡 MinorUnreachable code after try-catch.
Line 262 is unreachable. The
tryblock either returns explicitly (lines 244, 251, 253) or throws (caught bycatchon line 254, which returns on line 260). The same pattern appears at line 347 inrunSmoke.🧹 Proposed fix: remove unreachable returns
} catch (e) { const message = e instanceof Error ? e.message : String(e) addAction(taskId, { type: "error", content: message }) updateTaskStatus(taskId, "error") setLastError(message) if (!pipeline) setStatus("error") return { ok: false, error: message } } - return { ok: false, error: "Paper2Code did not complete." } }And similarly for
runSmokeat line 347:} catch (e) { const message = e instanceof Error ? e.message : String(e) updateTaskStatus(taskId, "error") addAction(taskId, { type: "error", content: message }) setLastError(message) if (!pipeline) setStatus("error") return { ok: false, error: message } } - return { ok: false, error: "Smoke run did not complete." } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/RunbookPanel.tsx` at line 262, The return statement after the try-catch is unreachable and should be removed: in RunbookPanel.tsx remove the trailing "return { ok: false, error: 'Paper2Code did not complete.' }" that's after the try/catch in the Paper2Code flow (the function containing the try that already returns on lines corresponding to the inner success/failure and the catch returns on error). Do the same cleanup in the runSmoke function (remove the unreachable return after its try/catch). Ensure each function still has consistent return types by relying on the existing returns inside the try and the catch rather than the dead tail return.web/src/components/studio/FilesPanel.tsx-169-188 (1)
169-188:⚠️ Potential issue | 🟡 MinorSilent failure on non-OK response leaves stale file index.
On line 174, if
res.okis false, the function returns early without clearingfileIndex. If there was a previously loaded index, the user will see stale file data from a different project or directory, with no error indication. Consider clearing the index or showing an error state.🛡️ Proposed fix
try { const res = await fetch(`/api/runbook/files?project_dir=${encodeURIComponent(projectDir)}&recursive=true`) - if (!res.ok) return + if (!res.ok) { + setFileIndex([]) + return + } const data = (await res.json()) as FileIndexResponse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 169 - 188, In refreshIndex, the early return on a non-OK fetch response leaves a previously loaded file index visible; change the non-OK branch to clear state and surface an error instead of returning silently — call setFileIndex([]) and setExpandedDirs(new Set()) (and optionally set an error state or log) when res.ok is false before exiting so stale data is not shown; keep the existing finally block that sets setLoadingIndex(false).web/src/app/papers/[id]/page.tsx-36-40 (1)
36-40:⚠️ Potential issue | 🟡 MinorLong abstracts may exceed URL length limits.
paper.abstractis passed as a query parameter. Some abstracts can be very long; after URI encoding, this could approach or exceed browser/server URL length limits (~2KB–8KB depending on the stack). Consider passing paper data via a shared store, session storage, or a shorter identifier instead of embedding the full abstract in the URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/papers/`[id]/page.tsx around lines 36 - 40, Summary: Passing paper.abstract in the query string (encodeURIComponent(paper.abstract)) can create very long URLs; instead persist the full paper payload elsewhere and send only a short identifier in the URL. Fix: change the Run Reproduction link so it does not embed paper.abstract — e.g. have the Button/Link use only a short param (paper.id or encoded title) and, on click, store the full paper object (paper.abstract, title, id) into sessionStorage/localStorage or a shared client store before navigating to /studio; update the /studio loader to read the paper from that storage by the same key (and remove encodeURIComponent(paper.abstract) usage). Ensure you update symbols Button, Link, and the navigation target (/studio) and any code that currently reads the abstract from the query to instead read from the chosen storage key.web/src/components/studio/PaperCard.tsx-52-66 (1)
52-66:⚠️ Potential issue | 🟡 Minor
formatRelativeTimedoesn't guard against invalid date strings.If
dateStris malformed,new Date(dateStr)returns anInvalid Date, causingdiffto beNaN. All comparisons will befalse, falling through todate.toLocaleDateString()which returns"Invalid Date". This is a minor UX issue — consider a guard:function formatRelativeTime(dateStr: string): string { const date = new Date(dateStr) + if (isNaN(date.getTime())) return dateStr || 'Unknown' const now = new Date()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/PaperCard.tsx` around lines 52 - 66, formatRelativeTime currently doesn't handle malformed date strings and will produce "Invalid Date"; update the function (formatRelativeTime) to validate the parsed Date (e.g., check isNaN(date.getTime()) or isNaN(diff)) early and return a user-friendly fallback like "Unknown date" (or an empty string) when the input is invalid; keep the existing relative-time logic for valid dates and also guard against null/undefined inputs before parsing.
| async def _run_step_async( | ||
| run_id: str, step_name: str, workdir: Path, commands: List[str], body: StepRequest | ||
| ) -> None: | ||
| """Generic async step runner - follows _run_smoke_async pattern but parameterized by step_name.""" | ||
| logger = get_execution_logger() | ||
| monitor = get_resource_monitor() | ||
|
|
||
| try: | ||
| loop = asyncio.get_running_loop() | ||
| if body.executor == "docker": | ||
| result = await loop.run_in_executor( | ||
| None, _run_docker_step_blocking, run_id, step_name, workdir, commands, body | ||
| ) | ||
| elif body.executor == "e2b": | ||
| result = await loop.run_in_executor( | ||
| None, _run_e2b_step_blocking, run_id, step_name, workdir, commands, body | ||
| ) | ||
| else: | ||
| result = await loop.run_in_executor( | ||
| None, _run_openai_ci_step_blocking, run_id, step_name, workdir, commands, body | ||
| ) | ||
|
|
||
| status = "success" if result.success else ("failed" if result.status != "error" else "error") | ||
|
|
||
| with _provider.session() as session: | ||
| run = session.get(AgentRunModel, run_id) | ||
| if run is not None: | ||
| run.status = status | ||
| run.ended_at = datetime.now(timezone.utc) | ||
| meta = run.get_metadata() | ||
| meta["exit_code"] = result.exit_code | ||
| if result.error: | ||
| meta["error"] = result.error | ||
| run.set_metadata(meta) | ||
|
|
||
| step = ( | ||
| session.query(RunbookStepModel) | ||
| .filter(RunbookStepModel.run_id == run_id, RunbookStepModel.step_name == step_name) | ||
| .order_by(RunbookStepModel.id.desc()) | ||
| .first() | ||
| ) | ||
| if step is not None: | ||
| step.status = status | ||
| step.ended_at = datetime.now(timezone.utc) | ||
| step.exit_code = result.exit_code | ||
| step.error = result.error | ||
| session.commit() | ||
|
|
||
| if result.success: | ||
| logger.log(run_id, "info", f"{step_name.title()} completed successfully (exit_code={result.exit_code})", source="system") | ||
| else: | ||
| if result.error: | ||
| logger.log(run_id, "error", f"{step_name.title()} failed: {result.error}", source="system") | ||
| logger.log(run_id, "error", f"{step_name.title()} failed (exit_code={result.exit_code})", source="system") | ||
| except Exception as e: | ||
| with _provider.session() as session: | ||
| run = session.get(AgentRunModel, run_id) | ||
| if run is not None: | ||
| run.status = "error" | ||
| run.ended_at = datetime.now(timezone.utc) | ||
| meta = run.get_metadata() | ||
| meta["error"] = str(e) | ||
| run.set_metadata(meta) | ||
| step = ( | ||
| session.query(RunbookStepModel) | ||
| .filter(RunbookStepModel.run_id == run_id, RunbookStepModel.step_name == step_name) | ||
| .order_by(RunbookStepModel.id.desc()) | ||
| .first() | ||
| ) | ||
| if step is not None: | ||
| step.status = "error" | ||
| step.ended_at = datetime.now(timezone.utc) | ||
| step.exit_code = 1 | ||
| step.error = str(e) | ||
| session.commit() | ||
| logger.log(run_id, "error", f"{step_name.title()} runner crashed: {e}", source="system") | ||
| finally: | ||
| try: | ||
| with _provider.session() as session: | ||
| run = session.get(AgentRunModel, run_id) | ||
| status = run.status if run is not None else "completed" | ||
| monitor.stop_run(run_id, status=status) | ||
| except Exception: | ||
| pass | ||
| try: | ||
| logger.stop_run(run_id) | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the required imports exist in the full file
echo "=== Checking imports in runbook.py ==="
fd 'runbook.py' src/paperbot/api/routes/ --exec head -60 {}
echo ""
echo "=== Checking for asyncio import ==="
rg -n '^\s*import asyncio' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Checking for time import ==="
rg -n '^\s*import time' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Checking for threading import ==="
rg -n '^\s*import threading' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Checking for RunbookStepModel import ==="
rg -n 'RunbookStepModel' src/paperbot/api/routes/runbook.py | head -5
echo ""
echo "=== Checking for get_execution_logger import ==="
rg -n 'get_execution_logger' src/paperbot/api/routes/runbook.py | head -5
echo ""
echo "=== Checking for get_resource_monitor import ==="
rg -n 'get_resource_monitor' src/paperbot/api/routes/runbook.py | head -5Repository: jerry609/PaperBot
Length of output: 2902
🏁 Script executed:
#!/bin/bash
# Search for import statements for the referenced items
echo "=== Full imports section (first 100 lines) ==="
head -100 src/paperbot/api/routes/runbook.py | tail -50
echo ""
echo "=== Search for RunbookStepModel import ==="
rg 'from .* import.*RunbookStepModel' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for get_execution_logger import ==="
rg 'from .* import.*get_execution_logger|import.*get_execution_logger' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for get_resource_monitor import ==="
rg 'from .* import.*get_resource_monitor|import.*get_resource_monitor' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Check if asyncio is actually used in the _run_step_async function ==="
rg -A 5 'asyncio\.get_running_loop' src/paperbot/api/routes/runbook.py | head -20Repository: jerry609/PaperBot
Length of output: 3083
🏁 Script executed:
#!/bin/bash
# Check the complete imports section at the top of the file
echo "=== Complete first 50 lines (all imports) ==="
head -50 src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for all lines containing 'import' in first 100 lines ==="
head -100 src/paperbot/api/routes/runbook.py | grep -n 'import\|from'Repository: jerry609/PaperBot
Length of output: 2753
🏁 Script executed:
#!/bin/bash
# Search the entire file for where these items are imported or defined
echo "=== Search for asyncio in entire file ==="
rg 'asyncio' src/paperbot/api/routes/runbook.py | head -20
echo ""
echo "=== Check if RunbookStepModel is in the imported models ==="
rg 'from paperbot.infrastructure.stores.models' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for where get_execution_logger and get_resource_monitor come from ==="
rg 'get_execution_logger|get_resource_monitor' src/paperbot/api/routes/runbook.py -B 2 | head -30
echo ""
echo "=== Check if there are additional imports later in the file ==="
grep -n '^import\|^from' src/paperbot/api/routes/runbook.py | tail -20Repository: jerry609/PaperBot
Length of output: 2259
🏁 Script executed:
#!/bin/bash
# Check if these functions/classes are imported conditionally or from other modules
echo "=== Search for any 'from' statement that might import these ==="
rg 'from.*import.*(' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for Literal import (used in ExecutorType) ==="
rg 'Literal' src/paperbot/api/routes/runbook.py | head -5
echo ""
echo "=== Check if get_execution_logger is defined in this file ==="
rg '^def get_execution_logger|^get_execution_logger' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Check if get_resource_monitor is defined in this file ==="
rg '^def get_resource_monitor|^get_resource_monitor' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Search for all function definitions in the file ==="
rg '^def ' src/paperbot/api/routes/runbook.py | head -20
echo ""
echo "=== Check the entire file size to understand scope ==="
wc -l src/paperbot/api/routes/runbook.pyRepository: jerry609/PaperBot
Length of output: 1817
🏁 Script executed:
#!/bin/bash
# Let me check if there's a missing import statement or if these are typos
echo "=== Check file structure around line 1205 area where the function starts ==="
sed -n '1200,1215p' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Look for any function/class that returns ExecutionResult ==="
rg 'def.*ExecutionResult' src/paperbot/api/routes/runbook.py | head -10
echo ""
echo "=== Check what modules these functions might be from ==="
find src/paperbot -name '*.py' -type f | xargs grep -l 'def get_execution_logger' 2>/dev/null | head -5
echo ""
echo "=== Check where RunbookStepModel should be imported from ==="
find src/paperbot -name '*.py' -type f | xargs grep -l 'class RunbookStepModel' 2>/dev/null | head -5
echo ""
echo "=== Search for typing imports ==="
sed -n '1,50p' src/paperbot/api/routes/runbook.py | grep -i 'literal\|typing'Repository: jerry609/PaperBot
Length of output: 1375
🏁 Script executed:
#!/bin/bash
# Check for get_resource_monitor location and verify all missing imports
echo "=== Find where get_resource_monitor is defined ==="
find src/paperbot -name '*.py' -type f | xargs grep -l 'def get_resource_monitor' 2>/dev/null
echo ""
echo "=== Verify Literal import is missing ==="
grep "from typing import" src/paperbot/api/routes/runbook.py
echo ""
echo "=== Check if asyncio is imported anywhere ==="
grep "import asyncio" src/paperbot/api/routes/runbook.py
echo ""
echo "=== Verify the exact import statements needed ==="
head -30 src/paperbot/api/routes/runbook.py | grep -E 'from typing|import asyncio|from.*import.*Literal'
echo ""
echo "=== Check execution_logger module for exports ==="
head -30 src/paperbot/infrastructure/logging/execution_logger.py | grep -E 'def get_execution_logger|def get_resource_monitor'Repository: jerry609/PaperBot
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Final verification - check the exact imports that are missing
echo "=== Check execution_logger.py for get_execution_logger function ==="
grep -n "def get_execution_logger" src/paperbot/infrastructure/logging/execution_logger.py
echo ""
echo "=== Check resource_monitor.py for get_resource_monitor function ==="
grep -n "def get_resource_monitor" src/paperbot/infrastructure/monitoring/resource_monitor.py
echo ""
echo "=== Verify RunbookStepModel is NOT in current imports ==="
grep "RunbookStepModel" src/paperbot/infrastructure/stores/models.py | head -1
echo ""
echo "=== List ALL current imports to confirm status ==="
sed -n '10,35p' src/paperbot/api/routes/runbook.py
echo ""
echo "=== Verify asyncio usage on lines around 1209 ==="
sed -n '1208,1215p' src/paperbot/api/routes/runbook.pyRepository: jerry609/PaperBot
Length of output: 1685
Add missing imports to enable function execution.
The following imports are used but not imported in the file:
asyncio(used at line 1212 in_run_step_async)Literalfromtyping(used at line 34 forExecutorType)RunbookStepModelfrompaperbot.infrastructure.stores.models(used at lines 188, 335, 1096+)get_execution_loggerfrompaperbot.infrastructure.logging.execution_logger(used at lines 203, 350, 1125, 1209, 1434)get_resource_monitorfrompaperbot.infrastructure.monitoring.resource_monitor(used at lines 208, 355, 1126, 1210)
These missing imports will cause NameError at runtime when the code executes.
🧰 Tools
🪛 Ruff (0.15.1)
[error] 1209-1209: Undefined name get_execution_logger
(F821)
[error] 1210-1210: Undefined name get_resource_monitor
(F821)
[error] 1213-1213: Undefined name asyncio
(F821)
[error] 1241-1241: Undefined name RunbookStepModel
(F821)
[error] 1242-1242: Undefined name RunbookStepModel
(F821)
[error] 1242-1242: Undefined name RunbookStepModel
(F821)
[error] 1243-1243: Undefined name RunbookStepModel
(F821)
[warning] 1259-1259: Do not catch blind exception: Exception
(BLE001)
[error] 1269-1269: Undefined name RunbookStepModel
(F821)
[error] 1270-1270: Undefined name RunbookStepModel
(F821)
[error] 1270-1270: Undefined name RunbookStepModel
(F821)
[error] 1271-1271: Undefined name RunbookStepModel
(F821)
[error] 1287-1288: try-except-pass detected, consider logging the exception
(S110)
[warning] 1287-1287: Do not catch blind exception: Exception
(BLE001)
[error] 1291-1292: try-except-pass detected, consider logging the exception
(S110)
[warning] 1291-1291: Do not catch blind exception: Exception
(BLE001)
🤖 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 1205 - 1293, Add the missing
imports at the top of the file so the referenced symbols resolve: import
asyncio; from typing import Literal (for ExecutorType); from
paperbot.infrastructure.stores.models import RunbookStepModel; from
paperbot.infrastructure.logging.execution_logger import get_execution_logger;
and from paperbot.infrastructure.monitoring.resource_monitor import
get_resource_monitor. Ensure these imports are placed alongside the other module
imports so functions like _run_step_async, ExecutorType, and code referencing
RunbookStepModel, get_execution_logger, and get_resource_monitor run without
NameError.
|
|
||
| export function LayoutShell({ children }: { children: React.ReactNode }) { | ||
| const [collapsed, setCollapsed] = useState(false) | ||
| const [collapsed, setCollapsed] = useState(true) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test files that reference LayoutShell
rg -rn "LayoutShell" --type ts --type tsx -g "*.test.*" -g "*.spec.*"
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts | xargs grep -l "LayoutShell" 2>/dev/nullRepository: jerry609/PaperBot
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for test files containing LayoutShell
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs grep -l "LayoutShell" 2>/dev/null || echo "No test files found mentioning LayoutShell"
# Also check for test files in the components directory
fd -type f \( -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" -o -name "*.spec.tsx" \) web/src/components/layout/ 2>/dev/null || echo "No test files found in layout directory"Repository: jerry609/PaperBot
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check if LayoutShell.tsx exists and examine the area around line 8
cat -n web/src/components/layout/LayoutShell.tsx | head -20Repository: jerry609/PaperBot
Length of output: 900
Add or update tests for the changed default collapsed state
The behavior of the initial render (collapsed sidebar, w-14 / md:pl-14) differs from the previous default, and no existing tests cover LayoutShell. Per coding guidelines, behavior changes should be accompanied by added or updated tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/layout/LayoutShell.tsx` at line 8, The LayoutShell
component default state was changed to collapsed (const [collapsed,
setCollapsed] = useState(true)), so add or update unit tests that assert the
initial render uses the collapsed layout (e.g., has the w-14 / md:pl-14 classes
and collapsed sidebar markup) and that toggling via setCollapsed or the sidebar
toggle control updates the DOM accordingly; locate tests referencing LayoutShell
(e.g., LayoutShell.test or render/LayoutShell tests) and update expectations
from the old default to verify collapsed is true on first render and interaction
tests still behave as expected.
- Add studio_chat router to main API - Remove openai_ci_executor.py - Simplify runbook.py to file management only - Update ReproductionLog with 2-button UI + Claude chat input - Add directory selection dialog to FilesPanel - Remove unused executor code from sandbox.py and models Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/paperbot/api/routes/runbook.py (3)
397-417:⚠️ Potential issue | 🟠 Major
WriteFileRequest.contenthas no size limit — arbitrary large file writes possible
content: straccepts arbitrarily large strings. FastAPI/Starlette imposes no default request body size cap, so a caller can POST a multi-gigabyte payload, exhausting memory and disk space. Add amax_length(ormax_bytes) constraint consistent with themax_bytesceiling used inread_project_file(20 MB).🛡️ Proposed fix
class WriteFileRequest(BaseModel): project_dir: str path: str - content: str + content: str = Field(max_length=20_000_000) # 20 MB cap, mirrors read_project_file🤖 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 397 - 417, WriteFileRequest currently allows arbitrarily large request bodies via content: str; add the same 20 MB cap used by read_project_file by constraining the field (or validating in the write_project_file handler) so the content length/bytes cannot exceed 20 * 1024 * 1024; update the WriteFileRequest model (content) or add an explicit size check in write_project_file before writing to disk and return HTTP 413/400 on too-large payloads to prevent memory/disk exhaustion.
112-140:⚠️ Potential issue | 🟡 Minor
total_byteslimit not strictly enforced across directory boundaries increate_snapshotLine 128 breaks the inner
for name in filenamesloop when the projected total would exceedmax_total_bytes, but does not updatetotal_bytes. The outeros.walkloop's guard at Lines 139–140 (if total_bytes >= max_total_bytes) therefore remainsFalse, so the walk continues into the next directory and can add more files. The snapshot may end up larger thanmax_total_bytes.🐛 Proposed fix
if total_bytes + size > body.max_total_bytes: - break + total_bytes = body.max_total_bytes # force outer-loop guard to fire + break🤖 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 112 - 140, The loop in create_snapshot stops the inner filenames iteration when adding the next file would exceed the quota but doesn't update total_bytes or signal the outer os.walk to stop, allowing later directories to be processed; fix this by setting total_bytes = body.max_total_bytes (or a boolean flag like done = True) right before breaking out of the inner loop and then check that flag (or the updated total_bytes) in the outer loop so os.walk stops immediately and no additional files are added; refer to total_bytes, body.max_total_bytes and the inner filenames loop in your changes.
159-202:⚠️ Potential issue | 🟡 MinorSnapshot file write and DB artifact commit are not atomic
The sequence is:
- Line 174 —
session.commit()persists theAgentRunModel.- Line 177 — snapshot JSON written to disk.
- Line 194 —
session.commit()persists theArtifactModel.If step 2 throws (e.g., disk full), the run record exists in the DB with no snapshot file and no artifact — leaking an orphaned run. If step 3 throws, the snapshot file exists but is un-indexed, so
_load_snapshotcan never find it.Consider writing the file and adding both DB records in a single
try/exceptblock, cleaning up the file on failure:♻️ Proposed fix (sketch)
- session.merge(run) - session.commit() - - snapshot_path = _snapshot_root() / f"{run_id}.json" - snapshot_path.write_text(...) - - artifact = ArtifactModel(...) - session.add(artifact) - session.commit() + snapshot_path = _snapshot_root() / f"{run_id}.json" + snapshot_path.write_text(json.dumps(snapshot_payload, ensure_ascii=False), encoding="utf-8") + try: + session.merge(run) + artifact = ArtifactModel(...) + session.add(artifact) + session.commit() + except Exception: + snapshot_path.unlink(missing_ok=True) + raise🤖 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 159 - 202, The run and artifact persistence is not atomic: write the snapshot to disk before committing any DB rows, using _snapshot_root() to create a temp file (or .tmp then atomic rename) for snapshot_payload; then in a single DB transaction (use _provider.session() and create AgentRunModel and ArtifactModel together) call session.add/merge and session.commit() once so both records are persisted together; wrap the file write + DB work in try/except and on any exception remove the temp/target snapshot file to avoid orphaned files (references: new_run_id(), AgentRunModel, ArtifactModel, _snapshot_root(), _provider.session()).
🧹 Nitpick comments (3)
web/src/components/studio/ReproductionLog.tsx (1)
40-57:run_commandis mapped inactionIconsbut absent fromactionColors.Actions of type
run_commandwill silently fall back to the generictextcolors (line 73). If that's intentional, a brief comment would help; otherwise, add a matching entry.Suggested fix
const actionColors: Record<string, { bg: string; text: string; border: string }> = { thinking: { bg: "bg-purple-50 dark:bg-purple-950/30", text: "text-purple-600 dark:text-purple-400", border: "border-purple-200 dark:border-purple-800" }, file_change: { bg: "bg-blue-50 dark:bg-blue-950/30", text: "text-blue-600 dark:text-blue-400", border: "border-blue-200 dark:border-blue-800" }, function_call: { bg: "bg-orange-50 dark:bg-orange-950/30", text: "text-orange-600 dark:text-orange-400", border: "border-orange-200 dark:border-orange-800" }, error: { bg: "bg-red-50 dark:bg-red-950/30", text: "text-red-600 dark:text-red-400", border: "border-red-200 dark:border-red-800" }, complete: { bg: "bg-green-50 dark:bg-green-950/30", text: "text-green-600 dark:text-green-400", border: "border-green-200 dark:border-green-800" }, + run_command: { bg: "bg-gray-50 dark:bg-gray-950/30", text: "text-gray-600 dark:text-gray-400", border: "border-gray-200 dark:border-gray-800" }, text: { bg: "bg-muted/50", text: "text-foreground", border: "border-border" }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/ReproductionLog.tsx` around lines 40 - 57, The actionIcons map includes "run_command" but actionColors does not, causing run_command to fall back to text colors; update the actionColors object to add a "run_command" key with appropriate bg/text/border values consistent with the other entries (or if intentional, add a clarifying comment near actionColors indicating run_command should use the "text" theme), referencing the actionIcons and actionColors identifiers so the change is applied to the same component.web/src/components/studio/FilesPanel.tsx (1)
186-213: Consider wrappingrefreshIndexinuseCallbackto avoid stale-closure suppression
refreshIndexis recreated on every render and excluded from theuseEffectdependency array via theeslint-disablecomment. This is functionally safe (sinceprojectDir— the only external value it captures — is listed in the deps array), but the suppression hides future dep drift. Wrapping inuseCallback([projectDir])removes the need for the suppression and makes the deps explicit.♻️ Proposed refactor
- const refreshIndex = async () => { + const refreshIndex = useCallback(async () => { if (!projectDir) return setLoadingIndex(true) try { ... } finally { setLoadingIndex(false) } - } + }, [projectDir]) useEffect(() => { setFileIndex([]) setQuery("") setExpandedDirs(new Set()) if (projectDir) refreshIndex() - // eslint-disable-next-line react-hooks/exhaustive-deps }, [projectDir, refreshIndex])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/studio/FilesPanel.tsx` around lines 186 - 213, The refreshIndex function is recreated each render and the effect suppresses linting; wrap refreshIndex in useCallback with [projectDir] so its dependencies are explicit, then reference that memoized refreshIndex from the useEffect deps array (remove the eslint-disable-next-line comment) and ensure refreshIndex still sets state (setLoadingIndex, setFileIndex, setExpandedDirs) and catches errors as before; this uses the same function name refreshIndex and the same state setters so locating and replacing the existing refreshIndex declaration and the useEffect hook is straightforward.src/paperbot/api/routes/runbook.py (1)
91-661: New endpoints lack test coverageAll new file-management endpoints (
create_snapshot,diff_file,revert_file,list_project_files,read_project_file,write_project_file,list_changes,delete_file,revert_project,revert_hunks) are untested. The path-traversal guards (_resolve_under_root,_allowed_workdir) and the hunk-revert matching logic in particular warrant targeted tests. As per coding guidelines, behavior changes should be accompanied by updated or new 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 91 - 661, Add unit/integration tests covering the new runbook file-management endpoints: create_snapshot, diff_file, revert_file, list_project_files, read_project_file, write_project_file, list_changes, delete_file, revert_project, and revert_hunks; specifically include tests that validate path-traversal/permission guards by exercising _resolve_under_root and _allowed_workdir with allowed and disallowed project_dir/path combinations, and add focused tests for the hunk-revert logic (RevertHunksRequest) to verify matching via context vs core, uniqueness behavior, applied/failed counts, and edge cases (empty before/after, overlapping hunks, non-unique patterns). Ensure tests assert proper HTTP status codes and side effects (file writes/deletes, snapshot artifact creation) for functions create_snapshot, revert_project, delete_file, write_project_file, and that snapshots loaded via _load_snapshot are validated for allowed snapshot paths.
🤖 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/components/studio/ReproductionLog.tsx`:
- Around line 499-505: The Install button is currently disabled when !projectDir
which prevents runInstall's workspace-setup branch from ever running; decide
whether to (A) allow Install to trigger workspace setup by removing !projectDir
from the button's disabled prop (leave disabled={isBusy}) so runInstall can open
the setup dialog when projectDir is falsy, or (B) remove the now-dead
workspace-setup code inside runInstall (the branch that checks !projectDir) if
Install should only be clickable after code generation; update either the
button's disabled condition or the runInstall logic (referencing projectDir and
runInstall) accordingly to keep behavior consistent.
- Around line 158-687: Add tests for ReproductionLog to cover its core
behaviors: write unit/interaction tests that mount the ReproductionLog component
(with mocked useStudioStore and project context) and assert the workspace-setup
flows (handleWorkspaceConfirm triggers updatePaper and executes pendingAction),
saving behavior (saveActiveFile posts correct body and shows errors), CLI flows
that use SSE (runPaper2CodeWithDir, runInstallWithDir, handleSendMessageWithDir)
by mocking fetch and readSSE to emit progress/result/error events and asserting
calls to addAction/updateTaskStatus/setLastGenCodeResult/updatePaper and status
transitions, and UI interactions (Generate code button opens WorkspaceSetup when
no projectDir, Code-mode chat blocks when no projectDir, model/mode selectors
update state). Mock fetch/readSSE and the store hooks (functions like addTask,
addAction, updateTaskStatus, updatePaper, setLastGenCodeResult) and assert
expected side-effects and rendered UI (error banner, task actions, DiffModal
open) for each scenario. Ensure tests include success and error SSE paths and
simulate keyboard send (Enter) and Save button flows.
- Around line 248-319: The UI can remain stuck in "running" if the SSE loop
exits without emitting result/error; update runPaper2CodeWithDir by adding a
finally guard after the try/catch that ensures setStatus is cleared (e.g., call
setStatus("idle") or setStatus("") ) when the function exits if it still equals
"running", and apply the same pattern to runInstallWithDir and
handleSendMessageWithDir so none of these handlers leave the component stuck in
the running state.
- Around line 664-672: The DiffModal's onApply currently only closes the modal
(setDiffAction(null)) but doesn't persist the change; update onApply to call the
project save/update routine before clearing diffAction: obtain the project
context (useProjectContext/updateFile or the appropriate save API), compute the
content to write (prefer diffAction.metadata.newContent or apply
diffAction.metadata.diff to oldContent if needed), call updateFile/save with
diffAction.metadata.filename and the new content, await success (handle errors),
then call setDiffAction(null); keep onReject as-is to only close without saving.
Use the existing symbols DiffModal, diffAction, setDiffAction, and
updateFile/useProjectContext to locate where to add the save logic.
- Line 185: The UI currently exposes an invalid "Opus 4.5" model option which
will cause API errors; in the ReproductionLog component update the model choices
so none map to the nonexistent Anthropic ID (alternatively update the backend
mapping). Specifically, inside the ReproductionLog component change the default
useState value (model / setModel) and remove or replace the option whose value
corresponds to "Opus 4.5" (the option shown in the model selection list and any
JSX option value like the Opus 4.5 string) so the UI only presents valid model
IDs (e.g., keep Sonnet/Haiku 4.5 or use the correct Opus 4.6 ID if desired).
Ensure the component's model state and any option value strings are consistent
with the backend-supported IDs.
---
Outside diff comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 397-417: WriteFileRequest currently allows arbitrarily large
request bodies via content: str; add the same 20 MB cap used by
read_project_file by constraining the field (or validating in the
write_project_file handler) so the content length/bytes cannot exceed 20 * 1024
* 1024; update the WriteFileRequest model (content) or add an explicit size
check in write_project_file before writing to disk and return HTTP 413/400 on
too-large payloads to prevent memory/disk exhaustion.
- Around line 112-140: The loop in create_snapshot stops the inner filenames
iteration when adding the next file would exceed the quota but doesn't update
total_bytes or signal the outer os.walk to stop, allowing later directories to
be processed; fix this by setting total_bytes = body.max_total_bytes (or a
boolean flag like done = True) right before breaking out of the inner loop and
then check that flag (or the updated total_bytes) in the outer loop so os.walk
stops immediately and no additional files are added; refer to total_bytes,
body.max_total_bytes and the inner filenames loop in your changes.
- Around line 159-202: The run and artifact persistence is not atomic: write the
snapshot to disk before committing any DB rows, using _snapshot_root() to create
a temp file (or .tmp then atomic rename) for snapshot_payload; then in a single
DB transaction (use _provider.session() and create AgentRunModel and
ArtifactModel together) call session.add/merge and session.commit() once so both
records are persisted together; wrap the file write + DB work in try/except and
on any exception remove the temp/target snapshot file to avoid orphaned files
(references: new_run_id(), AgentRunModel, ArtifactModel, _snapshot_root(),
_provider.session()).
---
Duplicate comments:
In `@web/src/components/studio/FilesPanel.tsx`:
- Around line 160-205: Update the Next.js dependency from 16.1.0 to 16.1.6 to
address the flagged CVEs: edit package.json to set "next": "16.1.6", then
reinstall to update your lockfile (package-lock.json / yarn.lock /
pnpm-lock.yaml), run the app build and test suite (and CI) to ensure FilesPanel
(export function FilesPanel) and other components still work, and commit both
package.json and the updated lockfile; if any breaking issues appear, roll
forward appropriate minor fixes or pin transitive deps as needed.
In `@web/src/components/studio/ReproductionLog.tsx`:
- Around line 263-310: The SSE streams leak because fetch calls in
runPaper2CodeWithDir, runInstallWithDir, and handleSendMessageWithDir never
receive an AbortSignal; add a component-level AbortController ref (e.g.,
sseAbortRef) that you replace/abort before starting any new request, pass
sseAbortRef.current.signal to fetch, abort the controller in useEffect cleanup
(on unmount) and before starting a new run, and update each function's
catch/loop to treat an aborted fetch (signal.aborted / DOMException name
"AbortError") as a no-op (don’t call updateTaskStatus, setState, or addAction on
abort) while still handling real errors normally.
---
Nitpick comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 91-661: Add unit/integration tests covering the new runbook
file-management endpoints: create_snapshot, diff_file, revert_file,
list_project_files, read_project_file, write_project_file, list_changes,
delete_file, revert_project, and revert_hunks; specifically include tests that
validate path-traversal/permission guards by exercising _resolve_under_root and
_allowed_workdir with allowed and disallowed project_dir/path combinations, and
add focused tests for the hunk-revert logic (RevertHunksRequest) to verify
matching via context vs core, uniqueness behavior, applied/failed counts, and
edge cases (empty before/after, overlapping hunks, non-unique patterns). Ensure
tests assert proper HTTP status codes and side effects (file writes/deletes,
snapshot artifact creation) for functions create_snapshot, revert_project,
delete_file, write_project_file, and that snapshots loaded via _load_snapshot
are validated for allowed snapshot paths.
In `@web/src/components/studio/FilesPanel.tsx`:
- Around line 186-213: The refreshIndex function is recreated each render and
the effect suppresses linting; wrap refreshIndex in useCallback with
[projectDir] so its dependencies are explicit, then reference that memoized
refreshIndex from the useEffect deps array (remove the eslint-disable-next-line
comment) and ensure refreshIndex still sets state (setLoadingIndex,
setFileIndex, setExpandedDirs) and catches errors as before; this uses the same
function name refreshIndex and the same state setters so locating and replacing
the existing refreshIndex declaration and the useEffect hook is straightforward.
In `@web/src/components/studio/ReproductionLog.tsx`:
- Around line 40-57: The actionIcons map includes "run_command" but actionColors
does not, causing run_command to fall back to text colors; update the
actionColors object to add a "run_command" key with appropriate bg/text/border
values consistent with the other entries (or if intentional, add a clarifying
comment near actionColors indicating run_command should use the "text" theme),
referencing the actionIcons and actionColors identifiers so the change is
applied to the same component.
| export function ReproductionLog() { | ||
| const { theme } = useTheme() | ||
| const { | ||
| papers, | ||
| tasks, | ||
| activeTaskId, | ||
| selectedPaperId, | ||
| lastGenCodeResult, | ||
| addTask, | ||
| addAction, | ||
| updateTaskStatus, | ||
| setLastGenCodeResult, | ||
| updatePaper, | ||
| selectPaper, | ||
| } = useStudioStore() | ||
|
|
||
| const { files, activeFile, updateFile, setActiveFile } = useProjectContext() | ||
| const activeFileData = activeFile ? files[activeFile] : null | ||
|
|
||
| const selectedPaper = useMemo(() => | ||
| selectedPaperId ? papers.find(p => p.id === selectedPaperId) ?? null : null, | ||
| [papers, selectedPaperId] | ||
| ) | ||
|
|
||
| const [status, setStatus] = useState<StepStatus>("idle") | ||
| const runIdRef = useRef<string | null>(null) | ||
| const [mode, setMode] = useState<Mode>("Code") | ||
| const [model, setModel] = useState("claude-sonnet-4-5") | ||
| const [lastError, setLastError] = useState<string | null>(null) | ||
| const [diffAction, setDiffAction] = useState<AgentAction | null>(null) | ||
| const [saving, setSaving] = useState(false) | ||
| const [messageInput, setMessageInput] = useState("") | ||
| const [showWorkspaceSetup, setShowWorkspaceSetup] = useState(false) | ||
| const [pendingAction, setPendingAction] = useState<"generate" | "install" | "chat" | null>(null) | ||
|
|
||
| const activeTask = tasks.find(t => t.id === activeTaskId) | ||
| const projectDir = selectedPaper?.outputDir || lastGenCodeResult?.outputDir || null | ||
| const canRun = (selectedPaper?.title.trim().length ?? 0) > 0 && (selectedPaper?.abstract.trim().length ?? 0) > 0 | ||
| const isBusy = status === "running" | ||
|
|
||
| const saveActiveFile = async () => { | ||
| if (!projectDir || !activeFile || !activeFileData) return | ||
| setSaving(true) | ||
| setLastError(null) | ||
| try { | ||
| const res = await fetch(`/api/runbook/file`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ project_dir: projectDir, path: activeFileData.name, content: activeFileData.content }), | ||
| }) | ||
| if (!res.ok) { | ||
| const text = await res.text() | ||
| throw new Error(`Failed to save (${res.status}): ${text}`) | ||
| } | ||
| } catch (e) { | ||
| setLastError(e instanceof Error ? e.message : String(e)) | ||
| } finally { | ||
| setSaving(false) | ||
| } | ||
| } | ||
|
|
||
| const handleWorkspaceConfirm = (directory: string) => { | ||
| setShowWorkspaceSetup(false) | ||
| if (selectedPaperId) { | ||
| updatePaper(selectedPaperId, { outputDir: directory }) | ||
| } | ||
| // Execute the pending action | ||
| if (pendingAction === "generate") { | ||
| runPaper2CodeWithDir(directory) | ||
| } else if (pendingAction === "install") { | ||
| runInstallWithDir(directory) | ||
| } else if (pendingAction === "chat") { | ||
| runChatWithDir(directory) | ||
| } | ||
| setPendingAction(null) | ||
| } | ||
|
|
||
| const runPaper2Code = async () => { | ||
| if (!selectedPaper || !canRun || isBusy) return | ||
|
|
||
| // If no project directory, show workspace setup dialog | ||
| if (!projectDir) { | ||
| setPendingAction("generate") | ||
| setShowWorkspaceSetup(true) | ||
| return | ||
| } | ||
|
|
||
| runPaper2CodeWithDir(projectDir) | ||
| } | ||
|
|
||
| const runPaper2CodeWithDir = async (targetDir: string) => { | ||
| if (!selectedPaper || !canRun) return | ||
|
|
||
| setStatus("running") | ||
| setLastError(null) | ||
|
|
||
| if (selectedPaperId) { | ||
| updatePaper(selectedPaperId, { status: 'generating' }) | ||
| } | ||
|
|
||
| const taskId = addTask(`Paper2Code — ${selectedPaper.title.slice(0, 40)}${selectedPaper.title.length > 40 ? "…" : ""}`) | ||
| addAction(taskId, { type: "thinking", content: "Starting Paper2Code run…" }) | ||
| runIdRef.current = null | ||
| let outputDir: string | undefined | ||
|
|
||
| try { | ||
| const res = await fetch("/api/gen-code", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| title: selectedPaper.title, | ||
| abstract: selectedPaper.abstract, | ||
| method_section: selectedPaper.methodSection || undefined, | ||
| use_orchestrator: true, | ||
| use_rag: true, | ||
| }), | ||
| }) | ||
|
|
||
| if (!res.ok || !res.body) { | ||
| throw new Error(`Failed to start run (${res.status})`) | ||
| } | ||
|
|
||
| updateTaskStatus(taskId, "running") | ||
|
|
||
| for await (const evt of readSSE(res.body)) { | ||
| if (evt?.type === "progress") { | ||
| const data = (evt.data ?? {}) as { phase?: string; message?: string; run_id?: string } | ||
| if (data.run_id && !runIdRef.current) { | ||
| runIdRef.current = data.run_id | ||
| } | ||
| addAction(taskId, { | ||
| type: "thinking", | ||
| content: `${data.phase ? `[${data.phase}] ` : ""}${data.message || "Working…"}`, | ||
| }) | ||
| } else if (evt?.type === "result") { | ||
| const result = evt.data as GenCodeResult | ||
| outputDir = result?.outputDir | ||
| setLastGenCodeResult(result) | ||
| addAction(taskId, { type: "complete", content: "Run completed" }) | ||
| updateTaskStatus(taskId, "completed") | ||
| if (selectedPaperId && outputDir) { | ||
| updatePaper(selectedPaperId, { status: 'ready', outputDir }) | ||
| } | ||
| setStatus("success") | ||
| } else if (evt?.type === "error") { | ||
| addAction(taskId, { type: "error", content: evt.message || "Run failed" }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(evt.message || "Run failed") | ||
| setStatus("error") | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| const message = e instanceof Error ? e.message : String(e) | ||
| addAction(taskId, { type: "error", content: message }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(message) | ||
| setStatus("error") | ||
| } | ||
| } | ||
|
|
||
| const runInstall = async () => { | ||
| if (isBusy) return | ||
|
|
||
| // If no project directory, show workspace setup dialog | ||
| if (!projectDir) { | ||
| if (!selectedPaper) { | ||
| setLastError("Select or create a paper first.") | ||
| return | ||
| } | ||
| setPendingAction("install") | ||
| setShowWorkspaceSetup(true) | ||
| return | ||
| } | ||
|
|
||
| runInstallWithDir(projectDir) | ||
| } | ||
|
|
||
| const runInstallWithDir = async (targetDir: string) => { | ||
| if (!targetDir) return | ||
|
|
||
| setStatus("running") | ||
| setLastError(null) | ||
|
|
||
| const taskId = addTask(`Install — ${targetDir.split("/").pop()}`) | ||
| addAction(taskId, { type: "thinking", content: "Installing dependencies via Claude..." }) | ||
|
|
||
| try { | ||
| // Use Claude chat to run the install command | ||
| const res = await fetch("/api/studio/chat", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| message: "Please install the project dependencies. Look for requirements.txt, setup.py, or pyproject.toml and run the appropriate pip install command.", | ||
| mode: "Code", | ||
| model, | ||
| paper: selectedPaper ? { | ||
| title: selectedPaper.title, | ||
| abstract: selectedPaper.abstract, | ||
| method_section: selectedPaper.methodSection, | ||
| } : undefined, | ||
| project_dir: targetDir, | ||
| }), | ||
| }) | ||
|
|
||
| if (!res.ok || !res.body) { | ||
| throw new Error(`Failed to start install (${res.status})`) | ||
| } | ||
|
|
||
| updateTaskStatus(taskId, "running") | ||
|
|
||
| for await (const evt of readSSE(res.body)) { | ||
| if (evt?.type === "progress") { | ||
| const data = (evt.data ?? {}) as { delta?: string; content?: string; phase?: string; message?: string } | ||
| if (data.delta) { | ||
| addAction(taskId, { type: "text", content: data.delta }) | ||
| } else if (data.message) { | ||
| addAction(taskId, { type: "thinking", content: data.message }) | ||
| } | ||
| } else if (evt?.type === "result") { | ||
| addAction(taskId, { type: "complete", content: "Install completed" }) | ||
| updateTaskStatus(taskId, "completed") | ||
| setStatus("success") | ||
| } else if (evt?.type === "error") { | ||
| addAction(taskId, { type: "error", content: evt.message || "Install failed" }) | ||
| updateTaskStatus(taskId, "error") | ||
| setLastError(evt.message || "Install failed") | ||
| setStatus("error") | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| const message = e instanceof Error ? e.message : String(e) | ||
| addAction(taskId, { type: "error", content: message }) | ||
| updateTaskStatus(taskId, "error") | ||
| setLastError(message) | ||
| setStatus("error") | ||
| } | ||
| } | ||
|
|
||
| const runChatWithDir = async (targetDir: string) => { | ||
| // Chat with specified directory - called after workspace setup | ||
| const message = messageInput.trim() | ||
| if (!message) return | ||
| setMessageInput("") | ||
| await handleSendMessageWithDir(message, targetDir) | ||
| } | ||
|
|
||
| const handleSendMessage = async () => { | ||
| if (!messageInput.trim() || isBusy) return | ||
|
|
||
| // For Code mode, require a project directory | ||
| if (mode === "Code" && !projectDir) { | ||
| if (!selectedPaper) { | ||
| setLastError("Select or create a paper first.") | ||
| return | ||
| } | ||
| setPendingAction("chat") | ||
| setShowWorkspaceSetup(true) | ||
| return | ||
| } | ||
|
|
||
| const message = messageInput.trim() | ||
| setMessageInput("") | ||
| await handleSendMessageWithDir(message, projectDir || undefined) | ||
| } | ||
|
|
||
| const handleSendMessageWithDir = async (message: string, targetDir?: string) => { | ||
| setStatus("running") | ||
| setLastError(null) | ||
|
|
||
| const taskId = addTask(`Chat — ${message.slice(0, 30)}${message.length > 30 ? "…" : ""}`) | ||
| addAction(taskId, { type: "thinking", content: `[${mode}] Sending to Claude...` }) | ||
|
|
||
| try { | ||
| const res = await fetch("/api/studio/chat", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| message, | ||
| mode, | ||
| model, | ||
| paper: selectedPaper ? { | ||
| title: selectedPaper.title, | ||
| abstract: selectedPaper.abstract, | ||
| method_section: selectedPaper.methodSection, | ||
| } : undefined, | ||
| project_dir: targetDir, | ||
| }), | ||
| }) | ||
|
|
||
| if (!res.ok || !res.body) { | ||
| throw new Error(`Failed to send message (${res.status})`) | ||
| } | ||
|
|
||
| updateTaskStatus(taskId, "running") | ||
|
|
||
| for await (const evt of readSSE(res.body)) { | ||
| if (evt?.type === "progress") { | ||
| const data = (evt.data ?? {}) as { delta?: string; content?: string; phase?: string; message?: string } | ||
| if (data.delta) { | ||
| // Streaming text update | ||
| addAction(taskId, { type: "text", content: data.delta }) | ||
| } else if (data.message) { | ||
| addAction(taskId, { type: "thinking", content: data.message }) | ||
| } | ||
| } else if (evt?.type === "result") { | ||
| addAction(taskId, { type: "complete", content: "Response complete" }) | ||
| updateTaskStatus(taskId, "completed") | ||
| setStatus("success") | ||
| } else if (evt?.type === "error") { | ||
| addAction(taskId, { type: "error", content: evt.message || "Chat failed" }) | ||
| updateTaskStatus(taskId, "error") | ||
| setLastError(evt.message || "Chat failed") | ||
| setStatus("error") | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| const msg = e instanceof Error ? e.message : String(e) | ||
| addAction(taskId, { type: "error", content: msg }) | ||
| updateTaskStatus(taskId, "error") | ||
| setLastError(msg) | ||
| setStatus("error") | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <div className="h-full flex flex-col min-w-0 min-h-0 bg-background"> | ||
| {/* Simplified Action Bar */} | ||
| <div className="px-4 py-2 flex items-center gap-2 shrink-0 border-b"> | ||
| <button | ||
| onClick={runPaper2Code} | ||
| disabled={!canRun || isBusy} | ||
| className="px-4 py-2 text-sm font-medium rounded-lg bg-primary text-primary-foreground hover:bg-primary/90 disabled:opacity-50 disabled:cursor-not-allowed transition-colors inline-flex items-center gap-2" | ||
| > | ||
| <Sparkles className="h-4 w-4" /> | ||
| Generate code | ||
| </button> | ||
| <button | ||
| onClick={runInstall} | ||
| disabled={!projectDir || isBusy} | ||
| className="px-4 py-2 text-sm font-medium rounded-lg hover:bg-muted disabled:opacity-50 disabled:cursor-not-allowed transition-colors" | ||
| > | ||
| Install | ||
| </button> | ||
| </div> | ||
|
|
||
| {/* Error banner */} | ||
| {lastError && ( | ||
| <div className="px-4 py-2 flex items-start gap-2 shrink-0 bg-red-50 dark:bg-red-950/30 text-red-600 dark:text-red-400"> | ||
| <AlertCircle className="h-4 w-4 mt-0.5 shrink-0" /> | ||
| <span className="text-xs">{lastError}</span> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Main content area */} | ||
| <div className="flex-1 min-h-0 overflow-hidden"> | ||
| {activeFileData ? ( | ||
| /* File Viewer */ | ||
| <div className="h-full flex flex-col"> | ||
| <div className="px-4 py-2 border-b flex items-center justify-between bg-muted/30 shrink-0"> | ||
| <div className="flex items-center gap-2 text-sm"> | ||
| <FileCode className="h-4 w-4 text-muted-foreground" /> | ||
| <span className="font-medium">{activeFileData.name}</span> | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| <Button | ||
| variant="default" | ||
| size="sm" | ||
| className="h-7 text-xs" | ||
| onClick={saveActiveFile} | ||
| disabled={!projectDir || saving} | ||
| > | ||
| <Save className="h-3.5 w-3.5 mr-1" /> | ||
| {saving ? "Saving..." : "Save"} | ||
| </Button> | ||
| <button | ||
| onClick={() => setActiveFile("")} | ||
| className="p-1.5 rounded hover:bg-muted transition-colors" | ||
| title="Close" | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <div className="flex-1 min-h-0 overflow-hidden"> | ||
| <Editor | ||
| height="100%" | ||
| language={activeFileData.language} | ||
| value={activeFileData.content} | ||
| theme={theme === "dark" ? "vs-dark" : "light"} | ||
| onChange={(value) => updateFile(activeFileData.name, value || "")} | ||
| options={{ | ||
| minimap: { enabled: false }, | ||
| fontSize: 13, | ||
| lineNumbers: "on", | ||
| scrollBeyondLastLine: false, | ||
| automaticLayout: true, | ||
| padding: { top: 12, bottom: 12 }, | ||
| fontFamily: "'JetBrains Mono', 'Menlo', 'Monaco', 'Courier New', monospace", | ||
| }} | ||
| /> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| /* Timeline */ | ||
| <ScrollArea className="h-full"> | ||
| <div className="p-4"> | ||
| {!activeTask || activeTask.actions.length === 0 ? ( | ||
| <div className="flex flex-col items-center justify-center text-muted-foreground py-20 space-y-4"> | ||
| <div className="w-16 h-16 rounded-full bg-muted/50 flex items-center justify-center"> | ||
| <Sparkles className="h-8 w-8 opacity-30" /> | ||
| </div> | ||
| <div className="text-center space-y-2"> | ||
| <p className="font-medium">Ready to reproduce</p> | ||
| <p className="text-xs max-w-[280px]"> | ||
| {selectedPaper | ||
| ? "Click Generate code to generate code from the selected paper" | ||
| : "Select or create a paper to get started"} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="space-y-0"> | ||
| {activeTask.actions.map((action, index) => ( | ||
| <ActionItem | ||
| key={action.id} | ||
| action={action} | ||
| onViewDiff={setDiffAction} | ||
| isLast={index === activeTask.actions.length - 1} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </ScrollArea> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Rich Input Area - CodePilot Style */} | ||
| <div className="border-t p-4 shrink-0"> | ||
| <div className="border rounded-xl bg-muted/30 overflow-hidden"> | ||
| <Textarea | ||
| value={messageInput} | ||
| onChange={(e) => setMessageInput(e.target.value)} | ||
| placeholder="Message Claude..." | ||
| className="border-0 bg-transparent resize-none min-h-[60px] focus-visible:ring-0 px-4 py-3" | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| handleSendMessage() | ||
| } | ||
| }} | ||
| /> | ||
| <div className="px-3 py-2 flex items-center justify-between border-t bg-background/50"> | ||
| <div className="flex items-center gap-2"> | ||
| {/* Paper attachment indicator */} | ||
| {selectedPaper && ( | ||
| <div className="flex items-center gap-1.5 px-2 py-1 bg-muted rounded-md text-xs text-muted-foreground"> | ||
| <FileText className="h-3.5 w-3.5" /> | ||
| <span className="max-w-[150px] truncate">{selectedPaper.title}</span> | ||
| </div> | ||
| )} | ||
| {/* Mode selector */} | ||
| <Select value={mode} onValueChange={(v) => setMode(v as Mode)}> | ||
| <SelectTrigger className="h-7 w-[90px] text-xs border-0 bg-muted"> | ||
| <Code className="h-3.5 w-3.5 mr-1" /> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="Code">Code</SelectItem> | ||
| <SelectItem value="Plan">Plan</SelectItem> | ||
| <SelectItem value="Ask">Ask</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| {/* Model selector */} | ||
| <Select value={model} onValueChange={setModel}> | ||
| <SelectTrigger className="h-7 w-[130px] text-xs border-0 bg-muted"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="claude-sonnet-4-5">Sonnet 4.5</SelectItem> | ||
| <SelectItem value="claude-opus-4-5">Opus 4.5</SelectItem> | ||
| <SelectItem value="claude-haiku-4-5">Haiku 4.5</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| {/* Send button */} | ||
| <Button | ||
| size="icon" | ||
| className="h-8 w-8 rounded-full" | ||
| onClick={handleSendMessage} | ||
| disabled={!messageInput.trim() || isBusy} | ||
| > | ||
| <Send className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Diff Modal */} | ||
| <DiffModal | ||
| isOpen={!!diffAction} | ||
| oldValue={diffAction?.metadata?.oldContent || '// Original file content'} | ||
| newValue={diffAction?.metadata?.newContent || diffAction?.metadata?.diff || '// Modified file content'} | ||
| filename={diffAction?.metadata?.filename} | ||
| onClose={() => setDiffAction(null)} | ||
| onApply={() => setDiffAction(null)} | ||
| onReject={() => setDiffAction(null)} | ||
| /> | ||
|
|
||
| {/* Workspace Setup Dialog */} | ||
| {selectedPaper && ( | ||
| <WorkspaceSetupDialog | ||
| paper={selectedPaper} | ||
| open={showWorkspaceSetup} | ||
| onConfirm={handleWorkspaceConfirm} | ||
| onCancel={() => { | ||
| setShowWorkspaceSetup(false) | ||
| setPendingAction(null) | ||
| }} | ||
| /> | ||
| )} | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No tests accompany this new component.
ReproductionLog contains substantial logic — SSE streaming, file saving, workspace setup flow, mode/model selection — none of which is covered by tests in this PR.
As per coding guidelines, "If behavior changes, add or update tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` around lines 158 - 687, Add
tests for ReproductionLog to cover its core behaviors: write unit/interaction
tests that mount the ReproductionLog component (with mocked useStudioStore and
project context) and assert the workspace-setup flows (handleWorkspaceConfirm
triggers updatePaper and executes pendingAction), saving behavior
(saveActiveFile posts correct body and shows errors), CLI flows that use SSE
(runPaper2CodeWithDir, runInstallWithDir, handleSendMessageWithDir) by mocking
fetch and readSSE to emit progress/result/error events and asserting calls to
addAction/updateTaskStatus/setLastGenCodeResult/updatePaper and status
transitions, and UI interactions (Generate code button opens WorkspaceSetup when
no projectDir, Code-mode chat blocks when no projectDir, model/mode selectors
update state). Mock fetch/readSSE and the store hooks (functions like addTask,
addAction, updateTaskStatus, updatePaper, setLastGenCodeResult) and assert
expected side-effects and rendered UI (error banner, task actions, DiffModal
open) for each scenario. Ensure tests include success and error SSE paths and
simulate keyboard send (Enter) and Save button flows.
| const [status, setStatus] = useState<StepStatus>("idle") | ||
| const runIdRef = useRef<string | null>(null) | ||
| const [mode, setMode] = useState<Mode>("Code") | ||
| const [model, setModel] = useState("claude-sonnet-4-5") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Anthropic Claude API model identifiers 2025 2026 latest
💡 Result:
Latest Claude API model identifiers (Anthropic) as of Feb 22, 2026
Current “latest generation” (Claude 4.6)
- Claude Opus 4.6:
claude-opus-4-6[1] - Claude Sonnet 4.6:
claude-sonnet-4-6[1]
Common 2025-era identifiers you’ll still see (snapshots / aliases)
- Claude Sonnet 4.5 (snapshot):
claude-sonnet-4-5-20250929(alias:claude-sonnet-4-5) [2] - Claude Haiku 4.5 (snapshot):
claude-haiku-4-5-20251001(alias:claude-haiku-4-5) [2] - Claude Sonnet 4 (snapshot):
claude-sonnet-4-20250514[3] - Claude Sonnet 3.7 (snapshot):
claude-3-7-sonnet-20250219[3]
How to confirm what’s “latest” in your account right now
Call the Models API: GET /v1/models (newest models are listed first). [4]
Sources:
[1] What’s new in Claude 4.6 (Claude API Docs) (platform.claude.com)
[2] Models overview (Claude API Docs) (platform.claude.com)
[3] Claude on Amazon Bedrock (Claude API Docs; includes snapshot IDs) (docs.anthropic.com)
[4] List Models endpoint (Claude API Reference) (docs.claude.com)
Citations:
- 1: https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-6?utm_source=openai
- 2: https://platform.claude.com/docs/zh-CN/about-claude/models/overview?utm_source=openai
- 3: https://docs.anthropic.com/en/api/claude-on-amazon-bedrock
- 4: https://docs.claude.com/en/api/models-list
🏁 Script executed:
# Search for references to the model identifiers in the code
rg "claude-opus-4-5|claude-sonnet-4-5|claude-haiku-4-5" web/src/Repository: jerry609/PaperBot
Length of output: 592
🏁 Script executed:
# Find where the model state is used (sent to backend, used in API calls, etc.)
rg -A 5 -B 5 "setModel|model," web/src/components/studio/ReproductionLog.tsx | head -100Repository: jerry609/PaperBot
Length of output: 2381
🏁 Script executed:
# Search for backend code that handles the model parameter
find . -type f -name "*.py" | xargs rg -l "claude-opus|claude-sonnet|claude-haiku|model" | grep -E "(api|backend|server)" | head -5Repository: jerry609/PaperBot
Length of output: 416
🏁 Script executed:
# Check the studio_chat.py route to see how the model parameter is handled
cat -n src/paperbot/api/routes/studio_chat.pyRepository: jerry609/PaperBot
Length of output: 16010
"Opus 4.5" model option will fail at runtime due to invalid API model ID.
The UI offers "Opus 4.5" (line 646), but the backend maps it to claude-opus-4-5-20250514 (src/paperbot/api/routes/studio_chat.py, line 98), which is not a valid Anthropic model ID as of Feb 2026. Current Anthropic API only provides claude-opus-4-6. Selecting "Opus 4.5" will cause an API error. Either update the backend mapping to claude-opus-4-6-20250514 or remove the "Opus 4.5" option from the UI if it's not yet available. "Sonnet 4.5" and "Haiku 4.5" are valid and work correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` at line 185, The UI currently
exposes an invalid "Opus 4.5" model option which will cause API errors; in the
ReproductionLog component update the model choices so none map to the
nonexistent Anthropic ID (alternatively update the backend mapping).
Specifically, inside the ReproductionLog component change the default useState
value (model / setModel) and remove or replace the option whose value
corresponds to "Opus 4.5" (the option shown in the model selection list and any
JSX option value like the Opus 4.5 string) so the UI only presents valid model
IDs (e.g., keep Sonnet/Haiku 4.5 or use the correct Opus 4.6 ID if desired).
Ensure the component's model state and any option value strings are consistent
with the backend-supported IDs.
| const runPaper2CodeWithDir = async (targetDir: string) => { | ||
| if (!selectedPaper || !canRun) return | ||
|
|
||
| setStatus("running") | ||
| setLastError(null) | ||
|
|
||
| if (selectedPaperId) { | ||
| updatePaper(selectedPaperId, { status: 'generating' }) | ||
| } | ||
|
|
||
| const taskId = addTask(`Paper2Code — ${selectedPaper.title.slice(0, 40)}${selectedPaper.title.length > 40 ? "…" : ""}`) | ||
| addAction(taskId, { type: "thinking", content: "Starting Paper2Code run…" }) | ||
| runIdRef.current = null | ||
| let outputDir: string | undefined | ||
|
|
||
| try { | ||
| const res = await fetch("/api/gen-code", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| title: selectedPaper.title, | ||
| abstract: selectedPaper.abstract, | ||
| method_section: selectedPaper.methodSection || undefined, | ||
| use_orchestrator: true, | ||
| use_rag: true, | ||
| }), | ||
| }) | ||
|
|
||
| if (!res.ok || !res.body) { | ||
| throw new Error(`Failed to start run (${res.status})`) | ||
| } | ||
|
|
||
| updateTaskStatus(taskId, "running") | ||
|
|
||
| for await (const evt of readSSE(res.body)) { | ||
| if (evt?.type === "progress") { | ||
| const data = (evt.data ?? {}) as { phase?: string; message?: string; run_id?: string } | ||
| if (data.run_id && !runIdRef.current) { | ||
| runIdRef.current = data.run_id | ||
| } | ||
| addAction(taskId, { | ||
| type: "thinking", | ||
| content: `${data.phase ? `[${data.phase}] ` : ""}${data.message || "Working…"}`, | ||
| }) | ||
| } else if (evt?.type === "result") { | ||
| const result = evt.data as GenCodeResult | ||
| outputDir = result?.outputDir | ||
| setLastGenCodeResult(result) | ||
| addAction(taskId, { type: "complete", content: "Run completed" }) | ||
| updateTaskStatus(taskId, "completed") | ||
| if (selectedPaperId && outputDir) { | ||
| updatePaper(selectedPaperId, { status: 'ready', outputDir }) | ||
| } | ||
| setStatus("success") | ||
| } else if (evt?.type === "error") { | ||
| addAction(taskId, { type: "error", content: evt.message || "Run failed" }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(evt.message || "Run failed") | ||
| setStatus("error") | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| const message = e instanceof Error ? e.message : String(e) | ||
| addAction(taskId, { type: "error", content: message }) | ||
| updateTaskStatus(taskId, "error") | ||
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | ||
| setLastError(message) | ||
| setStatus("error") | ||
| } | ||
| } |
There was a problem hiding this comment.
UI can get permanently stuck in "running" if the SSE stream ends without a result or error event.
setStatus("running") is called at the top (line 251), but status is only updated to "success" inside the result event handler (line 301) or "error" in the catch/error handlers. If the stream terminates cleanly without emitting either event (e.g., server closes the connection, network drop caught gracefully), the for await loop simply exits and isBusy stays true permanently — disabling all buttons.
The same pattern repeats in runInstallWithDir (line 341) and handleSendMessageWithDir (line 428).
Suggested fix — add a finally guard
Add a finally block after the try/catch (or at the end of the try block, after the loop) to ensure status is never left as "running":
} catch (e) {
const message = e instanceof Error ? e.message : String(e)
addAction(taskId, { type: "error", content: message })
updateTaskStatus(taskId, "error")
if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' })
setLastError(message)
setStatus("error")
+ } finally {
+ // Guard: ensure we never leave the UI in a perpetual "running" state
+ setStatus((prev) => (prev === "running" ? "error" : prev))
}Apply the same pattern to runInstallWithDir and handleSendMessageWithDir.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const runPaper2CodeWithDir = async (targetDir: string) => { | |
| if (!selectedPaper || !canRun) return | |
| setStatus("running") | |
| setLastError(null) | |
| if (selectedPaperId) { | |
| updatePaper(selectedPaperId, { status: 'generating' }) | |
| } | |
| const taskId = addTask(`Paper2Code — ${selectedPaper.title.slice(0, 40)}${selectedPaper.title.length > 40 ? "…" : ""}`) | |
| addAction(taskId, { type: "thinking", content: "Starting Paper2Code run…" }) | |
| runIdRef.current = null | |
| let outputDir: string | undefined | |
| try { | |
| const res = await fetch("/api/gen-code", { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| title: selectedPaper.title, | |
| abstract: selectedPaper.abstract, | |
| method_section: selectedPaper.methodSection || undefined, | |
| use_orchestrator: true, | |
| use_rag: true, | |
| }), | |
| }) | |
| if (!res.ok || !res.body) { | |
| throw new Error(`Failed to start run (${res.status})`) | |
| } | |
| updateTaskStatus(taskId, "running") | |
| for await (const evt of readSSE(res.body)) { | |
| if (evt?.type === "progress") { | |
| const data = (evt.data ?? {}) as { phase?: string; message?: string; run_id?: string } | |
| if (data.run_id && !runIdRef.current) { | |
| runIdRef.current = data.run_id | |
| } | |
| addAction(taskId, { | |
| type: "thinking", | |
| content: `${data.phase ? `[${data.phase}] ` : ""}${data.message || "Working…"}`, | |
| }) | |
| } else if (evt?.type === "result") { | |
| const result = evt.data as GenCodeResult | |
| outputDir = result?.outputDir | |
| setLastGenCodeResult(result) | |
| addAction(taskId, { type: "complete", content: "Run completed" }) | |
| updateTaskStatus(taskId, "completed") | |
| if (selectedPaperId && outputDir) { | |
| updatePaper(selectedPaperId, { status: 'ready', outputDir }) | |
| } | |
| setStatus("success") | |
| } else if (evt?.type === "error") { | |
| addAction(taskId, { type: "error", content: evt.message || "Run failed" }) | |
| updateTaskStatus(taskId, "error") | |
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | |
| setLastError(evt.message || "Run failed") | |
| setStatus("error") | |
| return | |
| } | |
| } | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : String(e) | |
| addAction(taskId, { type: "error", content: message }) | |
| updateTaskStatus(taskId, "error") | |
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | |
| setLastError(message) | |
| setStatus("error") | |
| } | |
| } | |
| const runPaper2CodeWithDir = async (targetDir: string) => { | |
| if (!selectedPaper || !canRun) return | |
| setStatus("running") | |
| setLastError(null) | |
| if (selectedPaperId) { | |
| updatePaper(selectedPaperId, { status: 'generating' }) | |
| } | |
| const taskId = addTask(`Paper2Code — ${selectedPaper.title.slice(0, 40)}${selectedPaper.title.length > 40 ? "…" : ""}`) | |
| addAction(taskId, { type: "thinking", content: "Starting Paper2Code run…" }) | |
| runIdRef.current = null | |
| let outputDir: string | undefined | |
| try { | |
| const res = await fetch("/api/gen-code", { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| title: selectedPaper.title, | |
| abstract: selectedPaper.abstract, | |
| method_section: selectedPaper.methodSection || undefined, | |
| use_orchestrator: true, | |
| use_rag: true, | |
| }), | |
| }) | |
| if (!res.ok || !res.body) { | |
| throw new Error(`Failed to start run (${res.status})`) | |
| } | |
| updateTaskStatus(taskId, "running") | |
| for await (const evt of readSSE(res.body)) { | |
| if (evt?.type === "progress") { | |
| const data = (evt.data ?? {}) as { phase?: string; message?: string; run_id?: string } | |
| if (data.run_id && !runIdRef.current) { | |
| runIdRef.current = data.run_id | |
| } | |
| addAction(taskId, { | |
| type: "thinking", | |
| content: `${data.phase ? `[${data.phase}] ` : ""}${data.message || "Working…"}`, | |
| }) | |
| } else if (evt?.type === "result") { | |
| const result = evt.data as GenCodeResult | |
| outputDir = result?.outputDir | |
| setLastGenCodeResult(result) | |
| addAction(taskId, { type: "complete", content: "Run completed" }) | |
| updateTaskStatus(taskId, "completed") | |
| if (selectedPaperId && outputDir) { | |
| updatePaper(selectedPaperId, { status: 'ready', outputDir }) | |
| } | |
| setStatus("success") | |
| } else if (evt?.type === "error") { | |
| addAction(taskId, { type: "error", content: evt.message || "Run failed" }) | |
| updateTaskStatus(taskId, "error") | |
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | |
| setLastError(evt.message || "Run failed") | |
| setStatus("error") | |
| return | |
| } | |
| } | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : String(e) | |
| addAction(taskId, { type: "error", content: message }) | |
| updateTaskStatus(taskId, "error") | |
| if (selectedPaperId) updatePaper(selectedPaperId, { status: 'error' }) | |
| setLastError(message) | |
| setStatus("error") | |
| } finally { | |
| // Guard: ensure we never leave the UI in a perpetual "running" state | |
| setStatus((prev) => (prev === "running" ? "error" : prev)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` around lines 248 - 319, The UI
can remain stuck in "running" if the SSE loop exits without emitting
result/error; update runPaper2CodeWithDir by adding a finally guard after the
try/catch that ensures setStatus is cleared (e.g., call setStatus("idle") or
setStatus("") ) when the function exits if it still equals "running", and apply
the same pattern to runInstallWithDir and handleSendMessageWithDir so none of
these handlers leave the component stuck in the running state.
| <button | ||
| onClick={runInstall} | ||
| disabled={!projectDir || isBusy} | ||
| className="px-4 py-2 text-sm font-medium rounded-lg hover:bg-muted disabled:opacity-50 disabled:cursor-not-allowed transition-colors" | ||
| > | ||
| Install | ||
| </button> |
There was a problem hiding this comment.
Install button's disabled condition makes the workspace-setup path in runInstall unreachable.
Line 501 disables the Install button when !projectDir, but runInstall (lines 325-332) contains logic to show the workspace-setup dialog when !projectDir — which can now never execute because the button is already disabled.
If the intent is to let users install before generating code (by choosing a workspace first), relax the disabled condition. If install should only be available after code generation, remove the dead workspace-setup code from runInstall.
Option A — allow Install to trigger workspace setup
<button
onClick={runInstall}
- disabled={!projectDir || isBusy}
+ disabled={isBusy}
className="px-4 py-2 text-sm font-medium rounded-lg hover:bg-muted disabled:opacity-50 disabled:cursor-not-allowed transition-colors"
>
Install
</button>📝 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.
| <button | |
| onClick={runInstall} | |
| disabled={!projectDir || isBusy} | |
| className="px-4 py-2 text-sm font-medium rounded-lg hover:bg-muted disabled:opacity-50 disabled:cursor-not-allowed transition-colors" | |
| > | |
| Install | |
| </button> | |
| <button | |
| onClick={runInstall} | |
| disabled={isBusy} | |
| className="px-4 py-2 text-sm font-medium rounded-lg hover:bg-muted disabled:opacity-50 disabled:cursor-not-allowed transition-colors" | |
| > | |
| Install | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` around lines 499 - 505, The
Install button is currently disabled when !projectDir which prevents
runInstall's workspace-setup branch from ever running; decide whether to (A)
allow Install to trigger workspace setup by removing !projectDir from the
button's disabled prop (leave disabled={isBusy}) so runInstall can open the
setup dialog when projectDir is falsy, or (B) remove the now-dead
workspace-setup code inside runInstall (the branch that checks !projectDir) if
Install should only be clickable after code generation; update either the
button's disabled condition or the runInstall logic (referencing projectDir and
runInstall) accordingly to keep behavior consistent.
| <DiffModal | ||
| isOpen={!!diffAction} | ||
| oldValue={diffAction?.metadata?.oldContent || '// Original file content'} | ||
| newValue={diffAction?.metadata?.newContent || diffAction?.metadata?.diff || '// Modified file content'} | ||
| filename={diffAction?.metadata?.filename} | ||
| onClose={() => setDiffAction(null)} | ||
| onApply={() => setDiffAction(null)} | ||
| onReject={() => setDiffAction(null)} | ||
| /> |
There was a problem hiding this comment.
onApply on DiffModal only closes the modal — it does not apply the diff.
Both onApply and onReject perform the same action: setDiffAction(null). The user clicking "Apply" would expect the new content to be written back to the project file (e.g., via updateFile from useProjectContext and/or a server-side save), but nothing happens.
Suggested approach
<DiffModal
isOpen={!!diffAction}
oldValue={diffAction?.metadata?.oldContent || '// Original file content'}
newValue={diffAction?.metadata?.newContent || diffAction?.metadata?.diff || '// Modified file content'}
filename={diffAction?.metadata?.filename}
onClose={() => setDiffAction(null)}
- onApply={() => setDiffAction(null)}
+ onApply={() => {
+ if (diffAction?.metadata?.filename && diffAction?.metadata?.newContent) {
+ updateFile(diffAction.metadata.filename, diffAction.metadata.newContent)
+ }
+ setDiffAction(null)
+ }}
onReject={() => setDiffAction(null)}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/ReproductionLog.tsx` around lines 664 - 672, The
DiffModal's onApply currently only closes the modal (setDiffAction(null)) but
doesn't persist the change; update onApply to call the project save/update
routine before clearing diffAction: obtain the project context
(useProjectContext/updateFile or the appropriate save API), compute the content
to write (prefer diffAction.metadata.newContent or apply
diffAction.metadata.diff to oldContent if needed), call updateFile/save with
diffAction.metadata.filename and the new content, await success (handle errors),
then call setDiffAction(null); keep onReject as-is to only close without saving.
Use the existing symbols DiffModal, diffAction, setDiffAction, and
updateFile/useProjectContext to locate where to add the save logic.
1 participant
Closes #134
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
UI Components