feat(DeepStudio): 重构DeepStudio页面，删除local docker和E2B，更换成codex 沙箱#135
Merged
Conversation
Related to 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. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces a new OpenAI Code Interpreter executor backend, redesigns the studio UI from a complex multi-panel layout to a streamlined 3-panel architecture featuring Papers, Reproduction Log, and Files panels, adds comprehensive paper lifecycle management with localStorage persistence, provides new API endpoints for executor status detection and command auto-detection, and restructures scholar subscription configuration to use per-scholar mappings with enhanced settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Web Frontend
participant Backend as Backend API
participant OpenAI as OpenAI API
participant Workspace as Project Workspace
User->>Frontend: Click "Run Reproduction"
Frontend->>Frontend: Navigate to /studio with paper params
Frontend->>Backend: POST /api/paper2code (generate implementation)
Backend->>Backend: Parse paper & generate spec
Backend-->>Frontend: Return generated code result
Frontend->>Frontend: Update ReproductionLog, show status
User->>Frontend: Click "Run" button (execute)
Frontend->>Backend: POST /api/runbook/smoke (with openai_ci executor)
Backend->>Backend: Prepare project environment
Backend->>OpenAI: POST /v1/files/upload (project zip)
OpenAI-->>Backend: File ID
Backend->>OpenAI: POST /v1/assistants (create with code interpreter)
OpenAI-->>Backend: Assistant ID
Backend->>OpenAI: POST /v1/threads (create execution thread)
OpenAI-->>Backend: Thread ID
Backend->>OpenAI: POST /v1/threads/:id/messages (send execution script)
OpenAI->>Workspace: Execute Python code in sandbox
Workspace-->>OpenAI: Code output & logs
OpenAI-->>Backend: Message with execution results
Backend->>Backend: Parse results & collect logs
Backend-->>Frontend: Stream execution logs via SSE
Frontend->>Frontend: Display in ReproductionLog timeline
Backend->>OpenAI: Cleanup (delete assistant/thread)
Backend-->>Frontend: Execution complete
sequenceDiagram
participant User
participant Frontend as Web Frontend
participant Store as Zustand Store
participant Backend as Backend API
participant Storage as localStorage
User->>Frontend: Load studio page
Frontend->>Store: Call loadPapers()
Store->>Storage: Read papers from localStorage
Storage-->>Store: Return persisted papers
Store->>Frontend: Update papers state
Frontend->>Frontend: Render PapersPanel with papers list
User->>Frontend: Click "Add Paper" button
Frontend->>Frontend: Open NewPaperModal
User->>Frontend: Select paper from library or enter manual details
Frontend->>Backend: GET /api/papers/library (if library mode)
Backend-->>Frontend: Return paper list
User->>Frontend: Confirm paper selection
Frontend->>Store: Call addPaper(paper)
Store->>Store: Create StudioPaper with id/timestamps/taskIds
Store->>Storage: Save papers to localStorage
Storage-->>Store: Persisted
Store->>Frontend: Update papers state
Frontend->>Frontend: Render new paper in PapersPanel
User->>Frontend: Select paper & run reproduction
Frontend->>Store: Call selectPaper(paperId)
Store->>Frontend: Update selectedPaperId, populate draft
Frontend->>Frontend: Run Paper2Code workflow
Frontend->>Backend: POST /api/paper2code
Backend-->>Frontend: Return result
Frontend->>Store: Call setLastGenCodeResult(result)
Store->>Store: Update selected paper lastGenCodeResult & status
Store->>Storage: Persist updated paper
Storage-->>Store: Saved
Store->>Frontend: Update state
Frontend->>Frontend: Display result in ReproductionLog & update PapersPanel status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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! 此拉取请求对 DeepStudio 功能进行了重大升级，通过将代码执行后端从本地 Docker 和 E2B 切换到 OpenAI Code Interpreter 沙箱，显著提升了执行环境的灵活性和安全性。同时，前端界面也进行了彻底的重新设计，采用了更直观的三面板布局，并集成了更完善的论文管理和代码复现流程，为用户提供了更流畅、高效的体验。 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
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperbot/api/routes/runbook.py (1)
91-408:⚠️ Potential issue | 🟡 MinorAdd tests for the expanded Runbook API.
New endpoints and execution flows (install/data/train/eval, detect-commands, executor-status) represent behavior changes and require API test coverage per the coding guidelines for behavior changes.
🤖 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 - 408, Add pytest API tests for the new Runbook endpoints: cover POST /runbook/install (start_install), /runbook/data (start_data), /runbook/train (start_train), /runbook/eval (start_eval), GET /runbook/detect-commands (detect_commands) and GET /runbook/executor-status (get_executor_status). For each POST endpoint assert 200 and response contains run_id/status, verify a RunbookStepModel and AgentRunModel were created (mock or inspect _provider.session), that _start_step was invoked (or that logger.start_run/monitor.start_run were called via monkeypatch), and that correct commands are constructed by the respective _build_install_commands/_build_data_commands/_build_train_commands/_build_eval_commands given a temporary project_dir fixture; for detect-commands assert returned dict includes expected keys (install/data/train/eval) for a sample project layout; for executor-status add tests that mock docker/e2b/openai imports and environment/model endpoints to assert available/error branches. Use FastAPI test client, pytest tmp_path for project dir fixtures, monkeypatch to stub external SDKs, and to capture DB/session side-effects so tests don't hit real services.
🧹 Nitpick comments (10)
web/src/components/studio/PaperCard.tsx (1)
52-66: DuplicateformatRelativeTimehelper exists in TasksPanel.tsx.This function duplicates the implementation in
TasksPanel.tsx(lines 19-30). Consider extracting to a shared utility in@/lib/utilsor a dedicated@/lib/formatmodule to maintain DRY principles.♻️ Extract to shared utility
Create a shared helper in
web/src/lib/format.ts:export function formatRelativeTime(date: Date | string): string { const d = typeof date === 'string' ? new Date(date) : date const now = new Date() const diff = now.getTime() - d.getTime() const seconds = Math.floor(diff / 1000) const minutes = Math.floor(seconds / 60) const hours = Math.floor(minutes / 60) const days = Math.floor(hours / 24) if (seconds < 60) return 'Just now' if (minutes < 60) return `${minutes}m ago` if (hours < 24) return `${hours}h ago` if (days < 7) return `${days}d ago` return d.toLocaleDateString() }Then import from both components.
🤖 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, The formatRelativeTime function is duplicated (present in PaperCard.tsx and TasksPanel.tsx); extract it into a shared utility (e.g., export function formatRelativeTime(date: Date | string) in a new module like web/src/lib/format.ts or `@/lib/format`), update the implementation to accept Date | string and use a local variable d for the parsed Date, then import and use formatRelativeTime in both PaperCard (replace the local formatRelativeTime) and TasksPanel to remove the duplicate implementation.web/next.config.ts (1)
5-12: Consider environment-based backend URL for flexibility.The hardcoded
localhost:8000works for local development but may cause issues in other environments (staging, production, CI). Consider using an environment variable:♻️ Suggested refactor for environment flexibility
const nextConfig: NextConfig = { reactCompiler: true, async rewrites() { + const backendUrl = process.env.BACKEND_URL || 'http://localhost:8000' return [ { source: '/api/:path*', - destination: 'http://localhost:8000/api/:path*', + destination: `${backendUrl}/api/:path*`, }, ] }, };🤖 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 rewrites() config currently hardcodes 'http://localhost:8000' as the backend; update the rewrites function to read the backend base URL from an environment variable (e.g., process.env.BACKEND_URL or NEXT_PUBLIC_BACKEND_URL) with a sensible local fallback (like http://localhost:8000) and use that variable when constructing the destination for the '/api/:path*' rule so the destination becomes "<envBase>/api/:path*"; ensure the env var includes the protocol or validate/normalize it before use and keep the rewrites function and the :path* placeholder unchanged.web/src/components/studio/PapersPanel.tsx (1)
73-76: DuplicateloadPapers()call.
loadPapers()is already called instudio/page.tsxon mount. Calling it again here is redundant since both components mount together. Consider removing this call or centralizing the initialization logic.🤖 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 73 - 76, PapersPanel.tsx currently calls loadPapers() in a local useEffect even though studio/page.tsx already initializes loadPapers() on mount, causing a duplicate call; remove the useEffect block that invokes loadPapers() from PapersPanel.tsx (or alternatively refactor so PapersPanel receives the loaded data via props or a shared hook/store) and ensure only the central initializer (studio/page.tsx or a shared loader hook) triggers loadPapers(), keeping PapersPanel rendering purely from the provided state or context.web/src/lib/store/studio-store.ts (1)
81-92: Consider adding schema validation for localStorage data.
JSON.parseon potentially corrupted localStorage data could cause runtime errors or inject malformed objects into state. Consider validating the parsed data structure before using it.🛡️ Example validation approach
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)) { + // Basic validation: ensure each item has required fields + return parsed.filter(p => + typeof p === 'object' && p !== null && + typeof p.id === 'string' && + typeof p.title === 'string' + ) as StudioPaper[] + } } } 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 81 - 92, The loadPapersFromStorage function currently JSON.parses localStorage[STORAGE_KEY] without validating shape; modify loadPapersFromStorage to validate the parsed value before returning it (use a schema validator like zod or a small manual check) so only properly formed StudioPaper[] is accepted; on parse success, run the validator against the result (checking required StudioPaper fields/types and that it's an array), return the validated value, and on validation failure log a warning including STORAGE_KEY and return [] (avoid throwing so app still boots).web/src/components/studio/ReproductionLog.tsx (5)
50-67: Missing color definition forrun_commandaction type.
actionIconsincludesrun_command(Line 57) butactionColorsdoesn't have a corresponding entry. This will cause the action to fall back toactionColors.text, which may be intentional but worth confirming.💡 Add consistent color for run_command
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" }, text: { bg: "bg-muted/50", text: "text-foreground", border: "border-border" }, + run_command: { bg: "bg-slate-50 dark:bg-slate-950/30", text: "text-slate-600 dark:text-slate-400", border: "border-slate-200 dark:border-slate-800" }, }🤖 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 50 - 67, actionIcons defines a "run_command" type but actionColors lacks a matching entry, causing run_command to fall back to the "text" styling; add a new actionColors["run_command"] entry (same shape: { bg: string; text: string; border: string }) to provide consistent styling for the Terminal/run_command action (pick appropriate Tailwind classes consistent with other entries, e.g., a blue/teal or neutral background + matching text and border classes) so components that use actionColors for run_command render with the intended colors.
222-228: Unused state:stepStatusesis written but never read.The
setStepStatusessetter is called throughout the component, but the state value is never consumed. Either use it in the UI or remove it.♻️ Options
Option 1: Remove if not needed
- const [, setStepStatuses] = useState<Record<string, StepStatus>>({ - install: "idle", - data: "idle", - train: "idle", - eval: "idle", - report: "idle", - })And remove all
setStepStatusescalls throughout the file.Option 2: Use the state (if you plan to show step progress)
- const [, setStepStatuses] = useState<Record<string, StepStatus>>({ + const [stepStatuses, setStepStatuses] = useState<Record<string, StepStatus>>({Then render step indicators in the UI.
🤖 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 222 - 228, The state value stepStatuses returned from useState (type Record<string, StepStatus>) is never read; remove the unused state by deleting the const [, setStepStatuses] = useState<...> declaration and then remove all invocations of setStepStatuses throughout ReproductionLog.tsx (or alternatively, if you intend to display progress, replace the unused state with an active state variable and render it in the UI — references: stepStatuses, setStepStatuses, StepStatus, useState).
697-706: Usenullinstead of empty string to close file.Line 700 passes
""tosetActiveFile, but the store usesnullto represent no active file. While""is falsy and works, usingnullis semantically clearer and matches the store's type.💡 Consistency fix
<button onClick={() => { // Close view: deselect file (file stays in Files panel) - setActiveFile("") + setActiveFile(null as unknown as string) // or update store type to accept null }}Or better, update
setActiveFilein project-context to acceptstring | 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 697 - 706, The close button currently calls setActiveFile("") but the store represents no active file as null; update the onClick handler in the ReproductionLog component so it calls setActiveFile(null) instead of setActiveFile(""), or alternatively update the setActiveFile signature to accept string | null across the project to keep types consistent (refer to the setActiveFile setter used in this component).
275-293: Consider adding abort handling for SSE stream.If the component unmounts while streaming logs, the stream continues processing. This could cause state updates on an unmounted component.
💡 Optional: Add AbortController support
- const streamRunLogsToTimeline = async (runId: string, taskId: string) => { + const streamRunLogsToTimeline = async (runId: string, taskId: string, signal?: AbortSignal) => { const res = await fetch(`/api/sandbox/runs/${encodeURIComponent(runId)}/logs/stream`, { headers: { Accept: "text/event-stream" }, + signal, })Then pass an
AbortController.signalfrom callers and abort on cleanup.🤖 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 275 - 293, The streamRunLogsToTimeline function currently doesn't support aborting the SSE stream, causing potential state updates after unmount; update streamRunLogsToTimeline to accept an optional AbortSignal parameter, pass that signal to fetch, and break/return from the for-await loop if signal.aborted (or handle an AbortError thrown by readSSE); ensure you close or cancel the stream reader if provided and catch the abort to avoid calling addAction after abort. Update callers to create an AbortController, pass controller.signal into streamRunLogsToTimeline, and call controller.abort() in the component cleanup to stop the stream when unmounting.
845-853:onApplyandonRejecthave identical behavior.Both handlers just close the modal. If the diff modal is meant to allow accepting/rejecting changes, these should have different implementations.
💡 Future enhancement
If diff actions should be applied to files:
- onApply={() => setDiffAction(null)} - onReject={() => setDiffAction(null)} + onApply={() => { + // Apply the change to the file + 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 845 - 853, The DiffModal currently uses identical handlers for onApply and onReject (both just call setDiffAction(null)), so accepting vs rejecting a diff does nothing different; update the onApply handler to perform the "apply" logic (e.g., call the function that updates the file content or dispatches the apply-diff action using diffAction.metadata, such as applyDiffAction(diffAction) or an existing file update method) and then call setDiffAction(null), and update the onReject handler to perform the "reject" logic (e.g., call a rejectDiffAction(diffAction) or mark the action as rejected) and then call setDiffAction(null); ensure you reference DiffModal, onApply, onReject, diffAction, and setDiffAction when making these changes so the code applies the diff or records rejection before closing the modal.web/src/components/studio/FilesPanel.tsx (1)
198-208: Consider adding user feedback for file loading errors.The empty catch block (Lines 205-207) silently swallows errors. Users won't know if a file failed to load.
💡 Optional: Add error feedback
const openFile = async (path: string) => { if (!projectDir) return try { const res = await fetch(`/api/runbook/file?project_dir=${encodeURIComponent(projectDir)}&path=${encodeURIComponent(path)}`) - if (!res.ok) return + if (!res.ok) { + console.error(`Failed to load file: ${path} (${res.status})`) + return + } const data = (await res.json()) as { path: string; content: string } addFile(data.path, data.content, languageForPath(data.path)) - } catch { - // ignore + } catch (e) { + console.error(`Failed to load file: ${path}`, e) } }🤖 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 198 - 208, The openFile function currently swallows errors in its catch block, leaving users unaware when a file fails to load; update the catch to both log the error (e.g., console.error with the caught error and the path/projectDir) and surface user-visible feedback—either call the app's existing toast/snackbar helper or set a local error state in FilesPanel to render an error message—so failures when calling the file fetch (and before calling addFile) are visible to the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/paperbot/api/routes/gen_code.py`:
- Around line 150-175: Add unit tests for the /gen-code endpoint that assert the
new result event fields: create test cases that call the gen_code route (or the
handler that yields StreamEvent) and verify the emitted StreamEvent.data
includes "blueprint" and "verificationPassed"; one test should supply a result
object with result.blueprint (with attributes architecture_type and domain) and
assert blueprint_info maps to "architectureType" and "domain", another should
supply verification_results (list of objects with .passed booleans) to assert
verificationPassed is true only when all passed, and a fallback test should omit
verification_results but set result.verification = {"all_passed": True/False} to
validate the fallback logic; ensure tests also check "success", "outputDir", and
"files" keys exist to cover payload structure consistency and reference the
StreamEvent emitted by gen_code.py to locate the handler under test.
- Around line 150-175: Add unit tests that assert the streaming response event
includes the new nested blueprint structure and verification flag: create tests
that call the route/handler that yields StreamEvent (the code building
blueprint_info and verification_passed in gen_code.py), simulate result objects
with/without result.blueprint, result.spec.model_type,
result.verification_results, and result.verification, and assert the yielded
event.data contains "blueprint" with keys "architectureType" and "domain"
(matching blueprint_info) and a boolean "verificationPassed". Target the handler
that yields StreamEvent and include cases for completed vs non-completed results
and for multiple verification paths (verification_results list and verification
dict) to cover all branches.
In `@src/paperbot/api/routes/sandbox.py`:
- Around line 392-444: The settings file is created without restrictive
permissions — update _save_settings to enforce directory and file modes: after
ensuring SETTINGS_FILE.parent exists, set its permissions to 0o700 (use chmod to
override umask) and after writing SETTINGS_FILE, set its permissions to 0o600;
reference the _save_settings function and SETTINGS_FILE/SETTINGS_FILE.parent so
the change is applied immediately after mkdir and after write (call chmod on
both to harden permissions).
- Around line 465-549: The start_docker route
(router.post("/sandbox/docker/start") -> start_docker) is unprotected and runs
system-level commands; require authentication by adding a
Depends(get_current_user) parameter to the endpoint signature so the request is
authenticated/authorized before running any subprocess/systemctl calls, update
the import if missing, and apply the same pattern to other sandbox routes
(verify every function under the sandbox router and add
Depends(get_current_user) or equivalent authorization checks) to prevent
unauthenticated remote command execution.
- Around line 370-460: Add unit tests for the new E2B settings endpoints: cover
get_e2b_settings, save_e2b_settings, and delete_e2b_settings; verify that GET
returns configured=True with proper masked_key when E2B_API_KEY is set in the
environment and that environment variable takes precedence over persisted
settings (_load_settings/_save_settings); test POST persists the key (via
_save_settings) and sets os.environ["E2B_API_KEY"] and returns the correctly
masked_key string; test DELETE removes the key from persisted settings and from
os.environ; use a temporary SETTINGS_FILE or monkeypatch
_load_settings/_save_settings and ensure tests clean up/reset environment
variables between runs.
In `@src/paperbot/repro/e2b_executor.py`:
- Around line 190-201: The run() and run_code() functions currently instantiate
sandboxes directly (via Sandbox.create(...) or Sandbox(...)) and always
close/kill them in their finally blocks, which ignores the reuse logic in
_get_sandbox() and makes keep_alive ineffective; change both methods to obtain
the sandbox by calling _get_sandbox() (passing through
self.api_key/self.timeout_sandbox as needed) instead of creating one inline, and
modify the finally blocks to only close/kill the sandbox when keep_alive is
False (leave it open when keep_alive is True), preserving the existing fallback
logic inside _get_sandbox() and keeping references to Sandbox.create and
Sandbox(...) removal from these methods.
In `@src/paperbot/repro/openai_ci_executor.py`:
- Around line 205-208: Replace the sensitive api_key fragment in the log inside
the try block that calls _openai_client(): do not log api_key[:20]; instead log
only non-sensitive info (e.g., base_url and a constant placeholder like
"api_key=<REDACTED>" or a safe key identifier if available). Update the
logger.log call that currently references api_key to use the redacted
placeholder (keep run_id, "info", base_url and source="system" intact) so no
secret material is written to logs.
- Around line 166-186: Add unit tests covering the new
OpenAICodeInterpreterExecutor class: exercise the __init__ and run methods
(including use of model and max_zip_bytes), and add test cases for the key edge
behaviors—timeout handling (simulate long-running execution to assert
timeout_sec triggers), upload failures (mock the file/upload step to raise or
return an error and assert run returns a failure result and logs appropriately),
and exit-code parsing (feed mocked execution outputs with various exit-code
formats and assert ExecutionResult captures correct exit code and
stdout/stderr). Use mocks for the external OpenAI/Assistants API interactions
and for file zipping to simulate size limits and upload errors, and add
assertions on ExecutionLogger calls to verify proper logging in each scenario.
- Around line 96-107: The code temporarily pops and restores OPENAI_BASE_URL
around OpenAI(...) which races across threads; remove the os.environ.pop/restore
sequence and rely on the explicit base_url parameter when creating the client
(i.e., eliminate env mutation in _openai_client / the client creation block
referencing env_base_url and OpenAI(api_key=..., base_url=...)); if you must
keep the env mutation instead, serialize the pop/restore with a module-level
threading.Lock used around the pop, OpenAI(...) call, and restore to ensure the
sequence is atomic.
In `@web/src/app/studio/page.tsx`:
- Around line 27-52: The effect races with async paper loading and also
mismatches URL paper_id with generated IDs; modify the useEffect that references
hasProcessedParams, papers, selectPaper, addPaper and router.replace to first
wait until papers are loaded (introduce or use an isPapersLoaded boolean/ref
that becomes true after loadPapers completes and include it in the effect
dependencies), only run the lookup once isPapersLoaded is true, and move
hasProcessedParams.current = true to after processing; additionally, if a
paper_id is present but not found, do NOT create a new paper that will get a
generated ID (instead either ignore the paper_id and only create a new paper
when title/abstract are provided, or add a helper like addPaperWithId / accept
an id param to addPaper to preserve the URL id), then call
router.replace("/studio", { scroll: false }) after handling.
In `@web/src/components/layout/LayoutShell.tsx`:
- Line 8: Add unit tests for the LayoutShell component to cover the new default
collapsed state: create a test (e.g., LayoutShell.test.tsx) that renders
<LayoutShell /> with no props and asserts the sidebar element has the collapsed
width class "w-14" and the main content has the collapsed padding class
"md:pl-14"; also include a test that toggles the collapse (calling the same
handler or simulating a button click exposed by LayoutShell) to verify classes
change when collapsed is false. Locate the component by the LayoutShell
component name and target its sidebar/main DOM nodes (by test-id, role, or
class) to make deterministic assertions about "w-14" and "md:pl-14".
In `@web/src/components/studio/NewPaperModal.tsx`:
- Around line 110-127: The current deduplication in handleImportSelected uses
paper IDs (existingIds from papers) but studio papers get new IDs via
addPaper/generateId so the match never succeeds; to fix, change the dedupe to
compare a stable property (e.g., title and abstract) or preserve the library ID
as a sourceId when calling addPaper. Concretely, in handleImportSelected compute
existing keys from papers (e.g., `${title}||${abstract}` or a sourceId field)
and for each selectedPaperId look up libraryPapers, build the same key for the
candidate and skip adding when the key exists; alternatively modify addPaper
calls to accept and persist a sourceId from the library paper so existingIds can
be based on paper.sourceId instead of generated IDs, then call resetAndClose as
before.
- Around line 148-149: studioPaperIds is built from studio-generated IDs (using
papers.map(p => p.id) ) but the import check uses library paper IDs, so
isInStudio always returns false; update NewPaperModal to build the lookup using
the library ID or other consistent identifier (e.g., use papers.map(p =>
p.libraryPaperId) or match on normalized title) and then use that set in the
isInStudio check so existing papers are correctly detected; ensure you reference
the studioPaperIds variable and the isInStudio usage when making this change.
In `@web/src/components/studio/PapersPanel.tsx`:
- Around line 112-137: handleConfirmDelete currently swallows failures from the
/api/runbook/delete call and proceeds to call deletePaper, which can leave
orphaned files; change the flow so that the file-deletion error is surfaced to
the user and only call deletePaper when the API call succeeds (or if the paper
has no outputDir). Concretely, in handleConfirmDelete wrap the fetch in a
try/catch that on error sets an error state or shows a user-facing notification
(e.g., via existing toast/alert mechanism), avoid calling deletePaper if the
fetch failed, and ensure setDeleting, setDeleteConfirmOpen and setPaperToDelete
are updated appropriately in both success and failure paths; reference
functions/state: handleConfirmDelete, deletePaper, setDeleting,
setDeleteConfirmOpen, setPaperToDelete and the /api/runbook/delete fetch.
In `@web/src/components/studio/ReproductionLog.tsx`:
- Around line 367-371: The current logic sets a step to success even when the
status fetch returned a non-OK response; change the branch handling the non-OK
response in the function using setStepStatuses and updateTaskStatus (the block
around setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) and
updateTaskStatus(taskId, "completed")) so that if the fetch/response is not ok
you do not mark the step as "success" — instead mark it "unknown" or "retry",
log/record the response error, and return { ok: false } (or trigger a retry
path) so callers know the check failed rather than treating it as completed.
In `@web/src/components/studio/RunbookPanel.tsx`:
- Around line 150-170: The current polling block treats a non-ok response from
fetch(`/api/runbook/runs/${encodeURIComponent(data.run_id)}`) as success; change
that to mark the step as failed/unknown instead: when statusRes.ok is false,
call setStepStatuses(prev => ({ ...prev, [stepName]: "error" })) (or "unknown"
per desired semantics), call updateTaskStatus(taskId, "error"), call
addAction(taskId, { type: "error", content: `Status fetch failed for
${stepName}: ${statusRes.status} ${statusRes.statusText}` }) and setLastError
with a clear message, then return { ok: false, error: message }; keep the
existing success path for info.status === "success" unchanged and ensure you
include statusRes status/text in the error message for debugging.
- Around line 373-394: After runPaper2Code completes and pipelineProjectDir is
set, the code currently uses the potentially stale detectedCommands from state
to decide whether to run the "data" step; re-fetch/refresh detection immediately
using the same detection routine (or an explicit API call) with
pipelineProjectDir and await its result, then use that freshDetectedCommands to
decide whether to run runStep("data", ...) (and to call
setStepStatuses/addAction accordingly) instead of the existing detectedCommands
state; place this refresh right after determining pipelineProjectDir (after
const pipelineProjectDir = ...) and before the if
(detectedCommands?.data?.detected) branch, referencing runPaper2Code,
pipelineProjectDir, detectedCommands, runStep, setStepStatuses, addAction, and
taskId.
In `@web/src/lib/store/studio-store.ts`:
- Around line 254-269: The addTask flow risks a race because it calls set()
twice and reads state.papers via get() between them; consolidate into a single
atomic update by using the functional set updater once (e.g., inside the addTask
function’s set(currentState => { ... })) to update both tasks and papers
together (referencing task creation logic, state.papers / papers mapping, and
taskIds update) and then call savePapersToStorage(get().papers) (or better, pass
the updated papers returned from the single set) after the updater so
persistence uses the consistent papers array.
---
Outside diff comments:
In `@src/paperbot/api/routes/runbook.py`:
- Around line 91-408: Add pytest API tests for the new Runbook endpoints: cover
POST /runbook/install (start_install), /runbook/data (start_data),
/runbook/train (start_train), /runbook/eval (start_eval), GET
/runbook/detect-commands (detect_commands) and GET /runbook/executor-status
(get_executor_status). For each POST endpoint assert 200 and response contains
run_id/status, verify a RunbookStepModel and AgentRunModel were created (mock or
inspect _provider.session), that _start_step was invoked (or that
logger.start_run/monitor.start_run were called via monkeypatch), and that
correct commands are constructed by the respective
_build_install_commands/_build_data_commands/_build_train_commands/_build_eval_commands
given a temporary project_dir fixture; for detect-commands assert returned dict
includes expected keys (install/data/train/eval) for a sample project layout;
for executor-status add tests that mock docker/e2b/openai imports and
environment/model endpoints to assert available/error branches. Use FastAPI test
client, pytest tmp_path for project dir fixtures, monkeypatch to stub external
SDKs, and to capture DB/session side-effects so tests don't hit real services.
---
Nitpick comments:
In `@web/next.config.ts`:
- Around line 5-12: The rewrites() config currently hardcodes
'http://localhost:8000' as the backend; update the rewrites function to read the
backend base URL from an environment variable (e.g., process.env.BACKEND_URL or
NEXT_PUBLIC_BACKEND_URL) with a sensible local fallback (like
http://localhost:8000) and use that variable when constructing the destination
for the '/api/:path*' rule so the destination becomes "<envBase>/api/:path*";
ensure the env var includes the protocol or validate/normalize it before use and
keep the rewrites function and the :path* placeholder unchanged.
In `@web/src/components/studio/FilesPanel.tsx`:
- Around line 198-208: The openFile function currently swallows errors in its
catch block, leaving users unaware when a file fails to load; update the catch
to both log the error (e.g., console.error with the caught error and the
path/projectDir) and surface user-visible feedback—either call the app's
existing toast/snackbar helper or set a local error state in FilesPanel to
render an error message—so failures when calling the file fetch (and before
calling addFile) are visible to the user.
In `@web/src/components/studio/PaperCard.tsx`:
- Around line 52-66: The formatRelativeTime function is duplicated (present in
PaperCard.tsx and TasksPanel.tsx); extract it into a shared utility (e.g.,
export function formatRelativeTime(date: Date | string) in a new module like
web/src/lib/format.ts or `@/lib/format`), update the implementation to accept Date
| string and use a local variable d for the parsed Date, then import and use
formatRelativeTime in both PaperCard (replace the local formatRelativeTime) and
TasksPanel to remove the duplicate implementation.
In `@web/src/components/studio/PapersPanel.tsx`:
- Around line 73-76: PapersPanel.tsx currently calls loadPapers() in a local
useEffect even though studio/page.tsx already initializes loadPapers() on mount,
causing a duplicate call; remove the useEffect block that invokes loadPapers()
from PapersPanel.tsx (or alternatively refactor so PapersPanel receives the
loaded data via props or a shared hook/store) and ensure only the central
initializer (studio/page.tsx or a shared loader hook) triggers loadPapers(),
keeping PapersPanel rendering purely from the provided state or context.
In `@web/src/components/studio/ReproductionLog.tsx`:
- Around line 50-67: actionIcons defines a "run_command" type but actionColors
lacks a matching entry, causing run_command to fall back to the "text" styling;
add a new actionColors["run_command"] entry (same shape: { bg: string; text:
string; border: string }) to provide consistent styling for the
Terminal/run_command action (pick appropriate Tailwind classes consistent with
other entries, e.g., a blue/teal or neutral background + matching text and
border classes) so components that use actionColors for run_command render with
the intended colors.
- Around line 222-228: The state value stepStatuses returned from useState (type
Record<string, StepStatus>) is never read; remove the unused state by deleting
the const [, setStepStatuses] = useState<...> declaration and then remove all
invocations of setStepStatuses throughout ReproductionLog.tsx (or alternatively,
if you intend to display progress, replace the unused state with an active state
variable and render it in the UI — references: stepStatuses, setStepStatuses,
StepStatus, useState).
- Around line 697-706: The close button currently calls setActiveFile("") but
the store represents no active file as null; update the onClick handler in the
ReproductionLog component so it calls setActiveFile(null) instead of
setActiveFile(""), or alternatively update the setActiveFile signature to accept
string | null across the project to keep types consistent (refer to the
setActiveFile setter used in this component).
- Around line 275-293: The streamRunLogsToTimeline function currently doesn't
support aborting the SSE stream, causing potential state updates after unmount;
update streamRunLogsToTimeline to accept an optional AbortSignal parameter, pass
that signal to fetch, and break/return from the for-await loop if signal.aborted
(or handle an AbortError thrown by readSSE); ensure you close or cancel the
stream reader if provided and catch the abort to avoid calling addAction after
abort. Update callers to create an AbortController, pass controller.signal into
streamRunLogsToTimeline, and call controller.abort() in the component cleanup to
stop the stream when unmounting.
- Around line 845-853: The DiffModal currently uses identical handlers for
onApply and onReject (both just call setDiffAction(null)), so accepting vs
rejecting a diff does nothing different; update the onApply handler to perform
the "apply" logic (e.g., call the function that updates the file content or
dispatches the apply-diff action using diffAction.metadata, such as
applyDiffAction(diffAction) or an existing file update method) and then call
setDiffAction(null), and update the onReject handler to perform the "reject"
logic (e.g., call a rejectDiffAction(diffAction) or mark the action as rejected)
and then call setDiffAction(null); ensure you reference DiffModal, onApply,
onReject, diffAction, and setDiffAction when making these changes so the code
applies the diff or records rejection before closing the modal.
In `@web/src/lib/store/studio-store.ts`:
- Around line 81-92: The loadPapersFromStorage function currently JSON.parses
localStorage[STORAGE_KEY] without validating shape; modify loadPapersFromStorage
to validate the parsed value before returning it (use a schema validator like
zod or a small manual check) so only properly formed StudioPaper[] is accepted;
on parse success, run the validator against the result (checking required
StudioPaper fields/types and that it's an array), return the validated value,
and on validation failure log a warning including STORAGE_KEY and return []
(avoid throwing so app still boots).
Comment on lines
+150
to
175
| # 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 | ||
|
|
||
| # Check verification status | ||
| verification_passed = False | ||
| if result.verification_results: | ||
| verification_passed = all(v.passed for v in result.verification_results) | ||
| elif result.verification: | ||
| verification_passed = result.verification.get("all_passed", False) | ||
|
|
||
| yield StreamEvent( | ||
| type="result", | ||
| data={ | ||
| "success": result.status.value == "completed", | ||
| "success": result.status.value == "completed" or len(result.generated_files) > 0, | ||
| "outputDir": str(output_dir), | ||
| "files": files, | ||
| "blueprint": { | ||
| "architectureType": ( | ||
| result.blueprint.architecture_type if result.blueprint else "unknown" | ||
| ), | ||
| "domain": result.blueprint.domain if result.blueprint else "unknown", | ||
| }, | ||
| "verificationPassed": ( | ||
| len(result.verification_results) > 0 | ||
| and all(v.passed for v in result.verification_results) | ||
| if result.verification_results | ||
| else False | ||
| ), | ||
| "blueprint": blueprint_info, | ||
| "verificationPassed": verification_passed, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(gen_code|test)" | head -20Repository: jerry609/PaperBot
Length of output: 854
🏁 Script executed:
fd -e py | grep -i "gen_code" | head -20Repository: jerry609/PaperBot
Length of output: 96
🏁 Script executed:
fd -e py | grep -i "test" | head -30Repository: jerry609/PaperBot
Length of output: 1174
🏁 Script executed:
wc -l src/paperbot/api/routes/gen_code.pyRepository: jerry609/PaperBot
Length of output: 100
🏁 Script executed:
cat -n src/paperbot/api/routes/gen_code.py | head -200Repository: jerry609/PaperBot
Length of output: 7819
🏁 Script executed:
rg -l "gen_code|StreamEvent" --type py tests/Repository: jerry609/PaperBot
Length of output: 98
🏁 Script executed:
cat -n tests/unit/test_streaming_envelope.pyRepository: jerry609/PaperBot
Length of output: 1770
🏁 Script executed:
rg -n "gen.code|gen_code|result.*type" tests/ --type py -C 2 | head -80Repository: jerry609/PaperBot
Length of output: 5560
🏁 Script executed:
git log --oneline src/paperbot/api/routes/gen_code.py | head -10Repository: jerry609/PaperBot
Length of output: 148
🏁 Script executed:
git diff HEAD~5 src/paperbot/api/routes/gen_code.py 2>/dev/null | head -100Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
fd "test" -e py | xargs rg "gen.code|gen_code" 2>/dev/null | grep -v ".pyc"Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
find tests -name "*.py" -type f | xargs rg "gen.code\|gen_code" 2>/dev/nullRepository: jerry609/PaperBot
Length of output: 45
🏁 Script executed:
find tests -name "*api*" -type fRepository: jerry609/PaperBot
Length of output: 106
🏁 Script executed:
cat -n tests/e2e/test_api_track_fullstack_offline.py | head -100Repository: jerry609/PaperBot
Length of output: 4368
🏁 Script executed:
git show HEAD:src/paperbot/api/routes/gen_code.py 2>/dev/null | tail -60Repository: jerry609/PaperBot
Length of output: 2235
🏁 Script executed:
find tests -name "*.py" -type f -exec grep -l "blueprint\|verification" {} \;Repository: jerry609/PaperBot
Length of output: 259
🏁 Script executed:
rg "blueprint|verificationPassed" --type py -A 2 -B 2Repository: jerry609/PaperBot
Length of output: 34474
🏁 Script executed:
ls -la tests/unit/test_gen_code* 2>/dev/null || echo "No test_gen_code files found"Repository: jerry609/PaperBot
Length of output: 89
🏁 Script executed:
rg "gen.code|generate_code" tests/ --type py -lRepository: jerry609/PaperBot
Length of output: 86
🏁 Script executed:
cat -n tests/test_repro_agent.py | head -100Repository: jerry609/PaperBot
Length of output: 3012
🏁 Script executed:
rg "blueprint|verification|result.*type.*data" tests/test_repro_agent.py -A 3 -B 1Repository: jerry609/PaperBot
Length of output: 588
Add tests for the gen_code endpoint's new result payload fields.
The /gen-code endpoint response was modified to include blueprint and verificationPassed fields in the result event. Per coding guidelines, behavior changes must have test coverage. Create tests that validate:
blueprint_infoextraction fromresult.blueprint(architectureType and domain fields)verificationPassedlogic (usingverification_resultsor fallback toverification.get("all_passed"))- Result payload structure consistency
🤖 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 - 175, Add unit tests
for the /gen-code endpoint that assert the new result event fields: create test
cases that call the gen_code route (or the handler that yields StreamEvent) and
verify the emitted StreamEvent.data includes "blueprint" and
"verificationPassed"; one test should supply a result object with
result.blueprint (with attributes architecture_type and domain) and assert
blueprint_info maps to "architectureType" and "domain", another should supply
verification_results (list of objects with .passed booleans) to assert
verificationPassed is true only when all passed, and a fallback test should omit
verification_results but set result.verification = {"all_passed": True/False} to
validate the fallback logic; ensure tests also check "success", "outputDir", and
"files" keys exist to cover payload structure consistency and reference the
StreamEvent emitted by gen_code.py to locate the handler under test.
🧩 Analysis chain
🏁 Script executed:
rg -n "blueprint|verificationPassed|verification" -g "*.py" -g "*.ts" -g "*.tsx" -C2 --max-count=50Repository: jerry609/PaperBot
Length of output: 50373
🏁 Script executed:
find . -name "*test*.py" -o -name "*spec*.ts" -o -name "*spec*.tsx" | head -20Repository: jerry609/PaperBot
Length of output: 934
🏁 Script executed:
rg -n "gen_code|gen-code" -g "*.py" -g "*.ts" -g "*.tsx" -C2 --max-count=30Repository: jerry609/PaperBot
Length of output: 6263
🏁 Script executed:
cat -n src/paperbot/api/routes/gen_code.py | sed -n '140,180p'Repository: jerry609/PaperBot
Length of output: 1894
🏁 Script executed:
rg -n "verificationPassed|blueprint.*info" tests/ -A2 -B2 --max-count=20Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
cat -n src/paperbot/repro/models.py | grep -A10 -B5 "class ReproductionResult"Repository: jerry609/PaperBot
Length of output: 603
🏁 Script executed:
fd "test*gen_code*" tests/ -type fRepository: jerry609/PaperBot
Length of output: 231
🏁 Script executed:
rg -n "class StreamEvent" src/ -A5Repository: jerry609/PaperBot
Length of output: 463
🏁 Script executed:
rg -n "verification_results|verification" src/paperbot/repro/models.py -A2 -B2 | head -50Repository: jerry609/PaperBot
Length of output: 1344
🏁 Script executed:
find tests -name "*gen_code*" -o -name "*gencode*" 2>/dev/nullRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
rg -n "StreamEvent.*result.*blueprint|verificationPassed" tests/ -C2Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
rg -n "blueprint|verificationPassed" web/src/components/ -C2 | head -60Repository: jerry609/PaperBot
Length of output: 997
🏁 Script executed:
cat -n src/paperbot/api/routes/gen_code.py | sed -n '1,50p'Repository: jerry609/PaperBot
Length of output: 1651
🏁 Script executed:
cat -n web/src/components/studio/RunbookPanel.tsx | sed -n '200,230p'Repository: jerry609/PaperBot
Length of output: 1313
🏁 Script executed:
cat -n web/src/components/studio/RunbookPanel.tsx | sed -n '230,280p'Repository: jerry609/PaperBot
Length of output: 2520
🏁 Script executed:
rg -n "GenCodeResult" web/src -B5 -A10 | head -80Repository: jerry609/PaperBot
Length of output: 5319
🏁 Script executed:
rg -n "test.*gen_code|def test.*gen" tests/ --type py | head -30Repository: jerry609/PaperBot
Length of output: 1033
🏁 Script executed:
cat -n src/paperbot/api/routes/gen_code.py | sed -n '140,180p'Repository: jerry609/PaperBot
Length of output: 1894
Add test coverage for response schema changes.
The payload structure has intentionally changed (blueprint → nested blueprint_info, new verificationPassed field), and the frontend is correctly updated to consume these fields. However, this behavior change lacks test coverage per the coding guidelines requiring tests for all behavior changes in src/**/*.py files. Add tests to validate the response schema structure and field presence.
🤖 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 - 175, Add unit tests
that assert the streaming response event includes the new nested blueprint
structure and verification flag: create tests that call the route/handler that
yields StreamEvent (the code building blueprint_info and verification_passed in
gen_code.py), simulate result objects with/without result.blueprint,
result.spec.model_type, result.verification_results, and result.verification,
and assert the yielded event.data contains "blueprint" with keys
"architectureType" and "domain" (matching blueprint_info) and a boolean
"verificationPassed". Target the handler that yields StreamEvent and include
cases for completed vs non-completed results and for multiple verification paths
(verification_results list and verification dict) to cover all branches.
Comment on lines
+370
to
+460
| class E2BSettingsRequest(BaseModel): | ||
| """Request body for saving E2B API key""" | ||
| api_key: str | ||
|
|
||
|
|
||
| class E2BSettingsResponse(BaseModel): | ||
| """Response for E2B settings""" | ||
| configured: bool | ||
| masked_key: Optional[str] = None | ||
|
|
||
|
|
||
| def _load_settings() -> Dict[str, Any]: | ||
| """Load settings from file.""" | ||
| import json | ||
| if SETTINGS_FILE.exists(): | ||
| try: | ||
| return json.loads(SETTINGS_FILE.read_text()) | ||
| except Exception: | ||
| return {} | ||
| return {} | ||
|
|
||
|
|
||
| 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)) | ||
|
|
||
|
|
||
| @router.get("/sandbox/settings/e2b") | ||
| async def get_e2b_settings(http_request: Request) -> E2BSettingsResponse: | ||
| """ | ||
| Get E2B configuration status. | ||
|
|
||
| Returns whether E2B is configured and a masked version of the key. | ||
| """ | ||
| # Check environment variable first | ||
| env_key = os.getenv("E2B_API_KEY") | ||
| if env_key: | ||
| return E2BSettingsResponse( | ||
| configured=True, | ||
| masked_key=f"{env_key[:4]}...{env_key[-4:]}" if len(env_key) > 8 else "****" | ||
| ) | ||
|
|
||
| # Check persisted settings | ||
| settings = _load_settings() | ||
| api_key = settings.get("e2b_api_key") | ||
| if api_key: | ||
| return E2BSettingsResponse( | ||
| configured=True, | ||
| masked_key=f"{api_key[:4]}...{api_key[-4:]}" if len(api_key) > 8 else "****" | ||
| ) | ||
|
|
||
| return E2BSettingsResponse(configured=False) | ||
|
|
||
|
|
||
| @router.post("/sandbox/settings/e2b") | ||
| async def save_e2b_settings(body: E2BSettingsRequest, http_request: Request): | ||
| """ | ||
| Save E2B API key. | ||
|
|
||
| Persists the key to ~/.paperbot/sandbox_settings.json and sets it in the environment. | ||
| """ | ||
| settings = _load_settings() | ||
| settings["e2b_api_key"] = body.api_key | ||
| _save_settings(settings) | ||
|
|
||
| # Also set in environment for current session | ||
| os.environ["E2B_API_KEY"] = body.api_key | ||
|
|
||
| return { | ||
| "status": "saved", | ||
| "message": "E2B API key saved successfully", | ||
| "masked_key": f"{body.api_key[:4]}...{body.api_key[-4:]}" if len(body.api_key) > 8 else "****" | ||
| } | ||
|
|
||
|
|
||
| @router.delete("/sandbox/settings/e2b") | ||
| async def delete_e2b_settings(http_request: Request): | ||
| """Remove saved E2B API key.""" | ||
| settings = _load_settings() | ||
| if "e2b_api_key" in settings: | ||
| del settings["e2b_api_key"] | ||
| _save_settings(settings) | ||
|
|
||
| # Remove from environment | ||
| if "E2B_API_KEY" in os.environ: | ||
| del os.environ["E2B_API_KEY"] | ||
|
|
||
| return {"status": "deleted", "message": "E2B API key removed"} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for test files related to sandbox
fd -t f -e py -g "*test*" | grep -i sandboxRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
# Search for existing tests that might cover E2BSettings or sandbox settings
rg -n "sandbox/settings|E2BSettings|e2b_api_key" -g "*test*" -A2Repository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
# Look at test structure - find all test files
fd -t f -e py -g "*test*" | head -20Repository: jerry609/PaperBot
Length of output: 894
🏁 Script executed:
# Check if there's a test directory and its structure
ls -la tests/ 2>/dev/null || echo "No tests directory found"Repository: jerry609/PaperBot
Length of output: 1377
🏁 Script executed:
# Search for sandbox in the entire codebase
rg -n "sandbox" -g "*.py" | head -30Repository: jerry609/PaperBot
Length of output: 2809
🏁 Script executed:
# Look for routes test files
fd -t f -e py "test.*route" -o "route.*test"Repository: jerry609/PaperBot
Length of output: 202
🏁 Script executed:
# Check if there's a unit/routes test directory
find tests -type d -name "*route*" 2>/dev/nullRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
# Search for api route tests
rg -n "router\." tests/ -g "*.py" | head -20Repository: jerry609/PaperBot
Length of output: 1037
🏁 Script executed:
# Confirm the sandbox.py file exists
find src -name "sandbox.py" -type fRepository: jerry609/PaperBot
Length of output: 95
🏁 Script executed:
# Look for API integration tests that might test routes
rg -n "@router\.(get|post|delete)" src/paperbot/api/routes/ -lRepository: jerry609/PaperBot
Length of output: 588
🏁 Script executed:
# Check if there are tests for the routes directory
ls -la tests/integration/ | grep -i routeRepository: jerry609/PaperBot
Length of output: 140
🏁 Script executed:
# Search for any test file that imports from the routes module
rg -n "from.*routes" tests/ -g "*.py" | head -10Repository: jerry609/PaperBot
Length of output: 1110
🏁 Script executed:
# Check for any existing sandbox route tests
find tests -name "*sandbox*" -type fRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
# Search for E2BSettings in entire codebase (tests and src)
rg -n "E2BSettings" . -g "*.py"Repository: jerry609/PaperBot
Length of output: 695
🏁 Script executed:
# Look at the test_research_track_routes.py to understand the test pattern
cat tests/integration/test_research_track_routes.pyRepository: jerry609/PaperBot
Length of output: 5933
Add tests for the new E2B settings endpoints.
The new GET/POST/DELETE /sandbox/settings/e2b routes and key masking behavior are untested. Per coding guidelines, behavior changes in {src,web/src}/**/*.{py,ts,tsx} require tests. Consider adding test coverage for configuration retrieval, key persistence, environment variable handling, and key deletion.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 387-387: Do not catch blind exception: Exception
(BLE001)
[warning] 400-400: Unused function argument: http_request
(ARG001)
[warning] 427-427: Unused function argument: http_request
(ARG001)
[warning] 448-448: Unused function argument: http_request
(ARG001)
🤖 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 370 - 460, Add unit tests
for the new E2B settings endpoints: cover get_e2b_settings, save_e2b_settings,
and delete_e2b_settings; verify that GET returns configured=True with proper
masked_key when E2B_API_KEY is set in the environment and that environment
variable takes precedence over persisted settings
(_load_settings/_save_settings); test POST persists the key (via _save_settings)
and sets os.environ["E2B_API_KEY"] and returns the correctly masked_key string;
test DELETE removes the key from persisted settings and from os.environ; use a
temporary SETTINGS_FILE or monkeypatch _load_settings/_save_settings and ensure
tests clean up/reset environment variables between runs.
Comment on lines
+392
to
+444
| 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)) | ||
|
|
||
|
|
||
| @router.get("/sandbox/settings/e2b") | ||
| async def get_e2b_settings(http_request: Request) -> E2BSettingsResponse: | ||
| """ | ||
| Get E2B configuration status. | ||
|
|
||
| Returns whether E2B is configured and a masked version of the key. | ||
| """ | ||
| # Check environment variable first | ||
| env_key = os.getenv("E2B_API_KEY") | ||
| if env_key: | ||
| return E2BSettingsResponse( | ||
| configured=True, | ||
| masked_key=f"{env_key[:4]}...{env_key[-4:]}" if len(env_key) > 8 else "****" | ||
| ) | ||
|
|
||
| # Check persisted settings | ||
| settings = _load_settings() | ||
| api_key = settings.get("e2b_api_key") | ||
| if api_key: | ||
| return E2BSettingsResponse( | ||
| configured=True, | ||
| masked_key=f"{api_key[:4]}...{api_key[-4:]}" if len(api_key) > 8 else "****" | ||
| ) | ||
|
|
||
| return E2BSettingsResponse(configured=False) | ||
|
|
||
|
|
||
| @router.post("/sandbox/settings/e2b") | ||
| async def save_e2b_settings(body: E2BSettingsRequest, http_request: Request): | ||
| """ | ||
| Save E2B API key. | ||
|
|
||
| Persists the key to ~/.paperbot/sandbox_settings.json and sets it in the environment. | ||
| """ | ||
| settings = _load_settings() | ||
| settings["e2b_api_key"] = body.api_key | ||
| _save_settings(settings) | ||
|
|
||
| # Also set in environment for current session | ||
| os.environ["E2B_API_KEY"] = body.api_key | ||
|
|
||
| return { | ||
| "status": "saved", | ||
| "message": "E2B API key saved successfully", | ||
| "masked_key": f"{body.api_key[:4]}...{body.api_key[-4:]}" if len(body.api_key) > 8 else "****" | ||
| } |
There was a problem hiding this comment.
Persisted API key should be written with restrictive permissions.
~/.paperbot/sandbox_settings.json is written without chmod; on multi-user systems the key can be readable by others. Consider setting file and directory permissions (600 / 700) after write.
🔐 Suggested permission hardening
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))
+ SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True)
+ SETTINGS_FILE.write_text(json.dumps(settings, indent=2))
+ try:
+ SETTINGS_FILE.chmod(0o600)
+ SETTINGS_FILE.parent.chmod(0o700)
+ except Exception:
+ pass🧰 Tools
🪛 Ruff (0.15.1)
[warning] 400-400: Unused function argument: http_request
(ARG001)
[warning] 427-427: Unused function argument: http_request
(ARG001)
🤖 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 392 - 444, The settings file
is created without restrictive permissions — update _save_settings to enforce
directory and file modes: after ensuring SETTINGS_FILE.parent exists, set its
permissions to 0o700 (use chmod to override umask) and after writing
SETTINGS_FILE, set its permissions to 0o600; reference the _save_settings
function and SETTINGS_FILE/SETTINGS_FILE.parent so the change is applied
immediately after mkdir and after write (call chmod on both to harden
permissions).
Comment on lines
+465
to
+549
| @router.post("/sandbox/docker/start") | ||
| async def start_docker(http_request: Request): | ||
| """ | ||
| Attempt to start Docker Desktop (macOS/Windows only). | ||
|
|
||
| Returns status of the start attempt. | ||
| """ | ||
| system = platform.system() | ||
|
|
||
| if system == "Darwin": # macOS | ||
| try: | ||
| subprocess.Popen( | ||
| ["open", "-a", "Docker"], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| return { | ||
| "status": "starting", | ||
| "message": "Docker Desktop is starting. Please wait a moment.", | ||
| "platform": "macOS", | ||
| } | ||
| except Exception as e: | ||
| return { | ||
| "status": "error", | ||
| "message": f"Failed to start Docker: {e}", | ||
| "platform": "macOS", | ||
| } | ||
|
|
||
| elif system == "Windows": | ||
| try: | ||
| # Try common Docker Desktop paths | ||
| docker_paths = [ | ||
| r"C:\Program Files\Docker\Docker\Docker Desktop.exe", | ||
| r"C:\Program Files (x86)\Docker\Docker\Docker Desktop.exe", | ||
| ] | ||
| for path in docker_paths: | ||
| if Path(path).exists(): | ||
| subprocess.Popen( | ||
| [path], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| return { | ||
| "status": "starting", | ||
| "message": "Docker Desktop is starting. Please wait a moment.", | ||
| "platform": "Windows", | ||
| } | ||
| return { | ||
| "status": "not_found", | ||
| "message": "Docker Desktop not found. Please install it from docker.com", | ||
| "platform": "Windows", | ||
| } | ||
| except Exception as e: | ||
| return { | ||
| "status": "error", | ||
| "message": f"Failed to start Docker: {e}", | ||
| "platform": "Windows", | ||
| } | ||
|
|
||
| elif system == "Linux": | ||
| # On Linux, Docker daemon is typically managed by systemd | ||
| try: | ||
| result = subprocess.run( | ||
| ["systemctl", "start", "docker"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode == 0: | ||
| return { | ||
| "status": "starting", | ||
| "message": "Docker daemon is starting.", | ||
| "platform": "Linux", | ||
| } | ||
| else: | ||
| return { | ||
| "status": "error", | ||
| "message": f"Failed to start Docker: {result.stderr}. Try 'sudo systemctl start docker'", | ||
| "platform": "Linux", | ||
| } | ||
| except Exception as e: | ||
| return { | ||
| "status": "error", | ||
| "message": f"Failed to start Docker: {e}. Try 'sudo systemctl start docker'", | ||
| "platform": "Linux", | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the sandbox router is mounted and check for auth middleware
rg -n "include_router" src/paperbot -C3Repository: jerry609/PaperBot
Length of output: 1741
🏁 Script executed:
#!/bin/bash
# Check for auth decorators or dependencies on the endpoint itself
rg -n "@router.post.*sandbox.*docker" src/paperbot -B5 -A10Repository: jerry609/PaperBot
Length of output: 998
🏁 Script executed:
#!/bin/bash
# Search for auth/role patterns in the API routes
rg -n "Depends\(|from.*auth|authorization|security" src/paperbot/api -C2Repository: jerry609/PaperBot
Length of output: 2131
🏁 Script executed:
#!/bin/bash
# Check the main app file to see how routers are configured
find src/paperbot -name "*.py" -type f | xargs rg -l "include_router.*sandbox"Repository: jerry609/PaperBot
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Look at the top of sandbox.py to see router definition and imports
head -100 src/paperbot/api/routes/sandbox.pyRepository: jerry609/PaperBot
Length of output: 2492
🏁 Script executed:
#!/bin/bash
# Check main.py for any middleware or auth setup
cat -n src/paperbot/api/main.py | head -80Repository: jerry609/PaperBot
Length of output: 3312
🏁 Script executed:
#!/bin/bash
# Search for any Depends patterns in sandbox.py specifically
rg -n "Depends" src/paperbot/api/routes/sandbox.pyRepository: jerry609/PaperBot
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's any auth module or middleware being imported/used
rg -n "auth|middleware|security" src/paperbot/api/main.py -iRepository: jerry609/PaperBot
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Check for README or documentation
find src/paperbot -name "README*" -o -name "*.md" | head -5Repository: jerry609/PaperBot
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Search for any environment-based or feature-flag protection in sandbox.py
rg -n "os.environ|getenv|feature|flag|debug|development" src/paperbot/api/routes/sandbox.py -iRepository: jerry609/PaperBot
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check if there are any comments about auth/security in sandbox.py
rg -n "TODO|FIXME|auth|security|protection|mvp" src/paperbot/api/routes/sandbox.py -iRepository: jerry609/PaperBot
Length of output: 43
Protect /api/sandbox/docker/start with authentication.
This endpoint executes dangerous system-level commands (Docker Desktop launch, systemctl on Linux) without any authentication or authorization checks. Combined with CORS allowing all origins, this creates a critical security vulnerability where unauthorized remote users could execute arbitrary system commands. Add authentication (e.g., Depends(get_current_user)) to this endpoint and verify all other sandbox endpoints are similarly protected.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 466-466: Unused function argument: http_request
(ARG001)
[error] 477-477: Starting a process with a partial executable path
(S607)
[warning] 481-485: Consider moving this statement to an else block
(TRY300)
[warning] 486-486: Do not catch blind exception: Exception
(BLE001)
[error] 502-502: subprocess call: check for execution of untrusted input
(S603)
[warning] 512-516: Consider moving this statement to an else block
(TRY300)
[warning] 517-517: Do not catch blind exception: Exception
(BLE001)
[error] 528-528: Starting a process with a partial executable path
(S607)
[warning] 544-544: 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/sandbox.py` around lines 465 - 549, The start_docker
route (router.post("/sandbox/docker/start") -> start_docker) is unprotected and
runs system-level commands; require authentication by adding a
Depends(get_current_user) parameter to the endpoint signature so the request is
authenticated/authorized before running any subprocess/systemctl calls, update
the import if missing, and apply the same pattern to other sandbox routes
(verify every function under the sandbox router and add
Depends(get_current_user) or equivalent authorization checks) to prevent
unauthenticated remote command execution.
Comment on lines
190
to
+201
| try: | ||
| # Create sandbox context | ||
| with Sandbox(api_key=self.api_key, timeout=self.timeout_sandbox) as sandbox: | ||
| # Set API key in environment (required for newer SDK versions) | ||
| if self.api_key: | ||
| os.environ["E2B_API_KEY"] = self.api_key | ||
|
|
||
| # Create sandbox using new API (Sandbox.create()) | ||
| try: | ||
| sandbox = Sandbox.create(timeout=self.timeout_sandbox) | ||
| except (TypeError, AttributeError): | ||
| # Fall back to old API | ||
| sandbox = Sandbox(api_key=self.api_key, timeout=self.timeout_sandbox) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
ls -lah src/paperbot/repro/e2b_executor.py
wc -l src/paperbot/repro/e2b_executor.pyRepository: jerry609/PaperBot
Length of output: 182
🏁 Script executed:
# Read the entire file to understand the implementation
cat -n src/paperbot/repro/e2b_executor.pyRepository: jerry609/PaperBot
Length of output: 15934
keep_alive is ineffective in run() and run_code() because both methods bypass _get_sandbox() and unconditionally destroy sandboxes.
The run() method (lines 190–201) and run_code() method (lines 316–320) create sandboxes directly instead of using _get_sandbox(), which contains the reuse logic. Both methods then unconditionally kill/close the sandbox in their finally blocks (lines 264–272 and 357–365), making the keep_alive parameter useless.
To honor keep_alive, call _get_sandbox() instead of creating sandboxes inline, and conditionally clean up only when keep_alive is false:
Suggested adjustment to honor keep_alive
- try:
- sandbox = Sandbox.create(timeout=self.timeout_sandbox)
- except (TypeError, AttributeError):
- sandbox = Sandbox(api_key=self.api_key, timeout=self.timeout_sandbox)
+ sandbox = self._get_sandbox()
+ if sandbox is None:
+ raise RuntimeError("Failed to initialize E2B sandbox")
...
- finally:
- # Clean up sandbox
- try:
- if hasattr(sandbox, 'kill'):
- sandbox.kill()
- elif hasattr(sandbox, 'close'):
- sandbox.close()
- except Exception:
- pass
+ finally:
+ if not self.keep_alive:
+ self.cleanup()
@@
- def cleanup(self):
+ def cleanup(self):
"""Clean up sandbox resources."""
if self._sandbox is not None:
try:
- self._sandbox.kill()
+ if hasattr(self._sandbox, "kill"):
+ self._sandbox.kill()
+ elif hasattr(self._sandbox, "close"):
+ self._sandbox.close()
except Exception:
pass
self._sandbox = NoneAlso applies to: 311–321, 357–365
🤖 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 - 201, The run() and
run_code() functions currently instantiate sandboxes directly (via
Sandbox.create(...) or Sandbox(...)) and always close/kill them in their finally
blocks, which ignores the reuse logic in _get_sandbox() and makes keep_alive
ineffective; change both methods to obtain the sandbox by calling _get_sandbox()
(passing through self.api_key/self.timeout_sandbox as needed) instead of
creating one inline, and modify the finally blocks to only close/kill the
sandbox when keep_alive is False (leave it open when keep_alive is True),
preserving the existing fallback logic inside _get_sandbox() and keeping
references to Sandbox.create and Sandbox(...) removal from these methods.
Comment on lines
+112
to
+137
| const handleConfirmDelete = async () => { | ||
| if (!paperToDelete) return | ||
|
|
||
| setDeleting(true) | ||
| try { | ||
| // Delete generated files if outputDir exists | ||
| if (paperToDelete.outputDir) { | ||
| try { | ||
| await fetch('/api/runbook/delete', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ project_dir: paperToDelete.outputDir }), | ||
| }) | ||
| } catch (e) { | ||
| console.error('Failed to delete project files:', e) | ||
| } | ||
| } | ||
|
|
||
| // Delete paper from store | ||
| deletePaper(paperToDelete.id) | ||
| } finally { | ||
| setDeleting(false) | ||
| setDeleteConfirmOpen(false) | ||
| setPaperToDelete(null) | ||
| } | ||
| } |
There was a problem hiding this comment.
Delete flow error handling could leave inconsistent state.
The delete handler has a nested try/catch that silently continues if the API call to /api/runbook/delete fails. If the API fails but deletePaper() succeeds, the paper is removed from the UI but orphaned files remain on disk. Consider providing user feedback when file deletion fails.
🔧 Proposed improvement with user feedback
+ const [deleteError, setDeleteError] = useState<string | null>(null)
+
const handleConfirmDelete = async () => {
if (!paperToDelete) return
setDeleting(true)
+ setDeleteError(null)
+ let fileDeleteFailed = false
+
try {
// Delete generated files if outputDir exists
if (paperToDelete.outputDir) {
try {
await fetch('/api/runbook/delete', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ project_dir: paperToDelete.outputDir }),
})
} catch (e) {
console.error('Failed to delete project files:', e)
+ fileDeleteFailed = true
}
}
// Delete paper from store
deletePaper(paperToDelete.id)
+
+ if (fileDeleteFailed) {
+ // Could show a toast notification here
+ console.warn('Paper removed but some files may not have been deleted')
+ }
} finally {
setDeleting(false)
setDeleteConfirmOpen(false)
setPaperToDelete(null)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/studio/PapersPanel.tsx` around lines 112 - 137,
handleConfirmDelete currently swallows failures from the /api/runbook/delete
call and proceeds to call deletePaper, which can leave orphaned files; change
the flow so that the file-deletion error is surfaced to the user and only call
deletePaper when the API call succeeds (or if the paper has no outputDir).
Concretely, in handleConfirmDelete wrap the fetch in a try/catch that on error
sets an error state or shows a user-facing notification (e.g., via existing
toast/alert mechanism), avoid calling deletePaper if the fetch failed, and
ensure setDeleting, setDeleteConfirmOpen and setPaperToDelete are updated
appropriately in both success and failure paths; reference functions/state:
handleConfirmDelete, deletePaper, setDeleting, setDeleteConfirmOpen,
setPaperToDelete and the /api/runbook/delete fetch.
Comment on lines
+367
to
+371
| } else { | ||
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | ||
| updateTaskStatus(taskId, "completed") | ||
| return { ok: true } | ||
| } |
There was a problem hiding this comment.
Optimistic success when status check fails.
When the status endpoint returns a non-OK response (Line 367), the step is marked as success anyway. This could mask real failures.
🐛 Proposed fix: Mark as unknown or retry
} else {
- setStepStatuses((prev) => ({ ...prev, [stepName]: "success" }))
- updateTaskStatus(taskId, "completed")
- return { ok: true }
+ // Status check failed - mark as unknown rather than success
+ const message = `Could not verify ${stepName} status (${statusRes.status})`
+ addAction(taskId, { type: "text", content: message })
+ updateTaskStatus(taskId, "completed")
+ return { ok: true } // Proceed optimistically but log the uncertainty
}🤖 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 367 - 371, The
current logic sets a step to success even when the status fetch returned a
non-OK response; change the branch handling the non-OK response in the function
using setStepStatuses and updateTaskStatus (the block around
setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) and
updateTaskStatus(taskId, "completed")) so that if the fetch/response is not ok
you do not mark the step as "success" — instead mark it "unknown" or "retry",
log/record the response error, and return { ok: false } (or trigger a retry
path) so callers know the check failed rather than treating it as completed.
Comment on lines
+150
to
+170
| // Poll final status | ||
| const statusRes = await fetch(`/api/runbook/runs/${encodeURIComponent(data.run_id)}`) | ||
| if (statusRes.ok) { | ||
| const info = (await statusRes.json()) as { status: string; exit_code?: number; error?: string } | ||
| if (info.status === "success") { | ||
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | ||
| updateTaskStatus(taskId, "completed") | ||
| addAction(taskId, { type: "complete", content: `${stepName} succeeded (exit_code=${info.exit_code ?? 0})` }) | ||
| return { ok: true } | ||
| } else { | ||
| setStepStatuses((prev) => ({ ...prev, [stepName]: "error" })) | ||
| updateTaskStatus(taskId, "error") | ||
| addAction(taskId, { type: "error", content: info.error || `${stepName} finished with status: ${info.status}` }) | ||
| const message = info.error || `${stepName} finished with status: ${info.status}` | ||
| setLastError(message) | ||
| return { ok: false, error: message } | ||
| } | ||
| } else { | ||
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | ||
| updateTaskStatus(taskId, "completed") | ||
| return { ok: true } |
There was a problem hiding this comment.
Status fetch failure is treated as success.
If /runbook/runs/{id} fails, the step is marked successful, which can mask real failures. Treat this as error/unknown instead.
🐛 Suggested handling on status fetch failure
- } else {
- setStepStatuses((prev) => ({ ...prev, [stepName]: "success" }))
- updateTaskStatus(taskId, "completed")
- return { ok: true }
- }
+ } else {
+ const message = `Failed to fetch final status for ${stepName}`
+ setStepStatuses((prev) => ({ ...prev, [stepName]: "error" }))
+ updateTaskStatus(taskId, "error")
+ addAction(taskId, { type: "error", content: message })
+ setLastError(message)
+ return { ok: false, error: message }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Poll final status | |
| const statusRes = await fetch(`/api/runbook/runs/${encodeURIComponent(data.run_id)}`) | |
| if (statusRes.ok) { | |
| const info = (await statusRes.json()) as { status: string; exit_code?: number; error?: string } | |
| if (info.status === "success") { | |
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | |
| updateTaskStatus(taskId, "completed") | |
| addAction(taskId, { type: "complete", content: `${stepName} succeeded (exit_code=${info.exit_code ?? 0})` }) | |
| return { ok: true } | |
| } else { | |
| setStepStatuses((prev) => ({ ...prev, [stepName]: "error" })) | |
| updateTaskStatus(taskId, "error") | |
| addAction(taskId, { type: "error", content: info.error || `${stepName} finished with status: ${info.status}` }) | |
| const message = info.error || `${stepName} finished with status: ${info.status}` | |
| setLastError(message) | |
| return { ok: false, error: message } | |
| } | |
| } else { | |
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | |
| updateTaskStatus(taskId, "completed") | |
| return { ok: true } | |
| // Poll final status | |
| const statusRes = await fetch(`/api/runbook/runs/${encodeURIComponent(data.run_id)}`) | |
| if (statusRes.ok) { | |
| const info = (await statusRes.json()) as { status: string; exit_code?: number; error?: string } | |
| if (info.status === "success") { | |
| setStepStatuses((prev) => ({ ...prev, [stepName]: "success" })) | |
| updateTaskStatus(taskId, "completed") | |
| addAction(taskId, { type: "complete", content: `${stepName} succeeded (exit_code=${info.exit_code ?? 0})` }) | |
| return { ok: true } | |
| } else { | |
| setStepStatuses((prev) => ({ ...prev, [stepName]: "error" })) | |
| updateTaskStatus(taskId, "error") | |
| addAction(taskId, { type: "error", content: info.error || `${stepName} finished with status: ${info.status}` }) | |
| const message = info.error || `${stepName} finished with status: ${info.status}` | |
| setLastError(message) | |
| return { ok: false, error: message } | |
| } | |
| } else { | |
| const message = `Failed to fetch final status for ${stepName}` | |
| 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 150 - 170, The
current polling block treats a non-ok response from
fetch(`/api/runbook/runs/${encodeURIComponent(data.run_id)}`) as success; change
that to mark the step as failed/unknown instead: when statusRes.ok is false,
call setStepStatuses(prev => ({ ...prev, [stepName]: "error" })) (or "unknown"
per desired semantics), call updateTaskStatus(taskId, "error"), call
addAction(taskId, { type: "error", content: `Status fetch failed for
${stepName}: ${statusRes.status} ${statusRes.statusText}` }) and setLastError
with a clear message, then return { ok: false, error: message }; keep the
existing success path for info.status === "success" unchanged and ensure you
include statusRes status/text in the error message for debugging.
Comment on lines
+373
to
+394
| try { | ||
| const paperResult = await runPaper2Code({ pipeline: true }) | ||
| if (!paperResult.ok) throw new Error(paperResult.error || "Paper2Code failed") | ||
| const pipelineProjectDir = paperResult.outputDir || projectDir | ||
| if (!pipelineProjectDir) throw new Error("Paper2Code did not return a project directory.") | ||
|
|
||
| addAction(taskId, { type: "thinking", content: "Paper2Code complete. Running smoke..." }) | ||
| const smokeResult = await runSmoke({ pipeline: true, projectDir: pipelineProjectDir }) | ||
| if (!smokeResult.ok) throw new Error(smokeResult.error || "Smoke failed") | ||
|
|
||
| addAction(taskId, { type: "thinking", content: "Running install..." }) | ||
| const installResult = await runStep("install", { pip_cache: true }, { pipeline: true, projectDir: pipelineProjectDir }) | ||
| if (!installResult.ok) throw new Error(installResult.error || "Install failed") | ||
|
|
||
| if (detectedCommands?.data?.detected) { | ||
| addAction(taskId, { type: "thinking", content: "Running data step..." }) | ||
| const dataResult = await runStep("data", {}, { pipeline: true, projectDir: pipelineProjectDir }) | ||
| if (!dataResult.ok) throw new Error(dataResult.error || "Data step failed") | ||
| } else { | ||
| setStepStatuses((prev) => ({ ...prev, data: "success" })) | ||
| addAction(taskId, { type: "thinking", content: "Data step skipped (no script detected)." }) | ||
| } |
There was a problem hiding this comment.
Pipeline may use stale detectedCommands after Paper2Code.
detectedCommands updates asynchronously via useEffect, so the pipeline can decide to skip the data step using stale info. Consider re-fetching immediately after outputDir is known.
✅ Suggested refresh before data step decision
- if (detectedCommands?.data?.detected) {
+ const refreshedCommands = await fetch(`/api/runbook/detect-commands?project_dir=${encodeURIComponent(pipelineProjectDir)}`)
+ .then((res) => (res.ok ? res.json() : null))
+ .catch(() => null)
+ if (refreshedCommands) setDetectedCommands(refreshedCommands)
+
+ if (refreshedCommands?.data?.detected) {
addAction(taskId, { type: "thinking", content: "Running data step..." })
const dataResult = await runStep("data", {}, { pipeline: true, projectDir: pipelineProjectDir })
if (!dataResult.ok) throw new Error(dataResult.error || "Data step failed")🤖 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 373 - 394, After
runPaper2Code completes and pipelineProjectDir is set, the code currently uses
the potentially stale detectedCommands from state to decide whether to run the
"data" step; re-fetch/refresh detection immediately using the same detection
routine (or an explicit API call) with pipelineProjectDir and await its result,
then use that freshDetectedCommands to decide whether to run runStep("data",
...) (and to call setStepStatuses/addAction accordingly) instead of the existing
detectedCommands state; place this refresh right after determining
pipelineProjectDir (after const pipelineProjectDir = ...) and before the if
(detectedCommands?.data?.detected) branch, referencing runPaper2Code,
pipelineProjectDir, detectedCommands, runStep, setStepStatuses, addAction, and
taskId.
Comment on lines
+254
to
+269
|
|
||
| // Link task to paper if one is selected | ||
| if (paperId) { | ||
| const paper = state.papers.find(p => p.id === paperId) | ||
| if (paper) { | ||
| set(currentState => ({ | ||
| papers: currentState.papers.map(p => | ||
| p.id === paperId | ||
| ? { ...p, taskIds: [...p.taskIds, id], updatedAt: new Date().toISOString() } | ||
| : p | ||
| ) | ||
| })) | ||
| // Persist updated papers | ||
| savePapersToStorage(get().papers) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition in task-paper linking.
The addTask function calls set() twice with a get() in between. The second set() (lines 259-265) uses state.papers but this could be stale if another update occurred between the two set() calls. Consider consolidating into a single atomic set() call.
🔧 Proposed fix to consolidate state updates
addTask: (name) => {
const state = get()
const id = `task-${Date.now()}`
const paperId = state.selectedPaperId
- set(currentState => ({
- tasks: [...currentState.tasks, {
+
+ set(currentState => {
+ const newTask = {
id,
name,
status: 'running',
actions: [],
createdAt: new Date(),
paperId: paperId || undefined,
- }],
- activeTaskId: id
- }))
-
- // Link task to paper if one is selected
- if (paperId) {
- const paper = state.papers.find(p => p.id === paperId)
- if (paper) {
- set(currentState => ({
- papers: currentState.papers.map(p =>
+ }
+
+ let newPapers = currentState.papers
+ if (paperId) {
+ newPapers = currentState.papers.map(p =>
p.id === paperId
? { ...p, taskIds: [...p.taskIds, id], updatedAt: new Date().toISOString() }
: p
- )
- }))
- // Persist updated papers
- savePapersToStorage(get().papers)
+ )
+ savePapersToStorage(newPapers)
}
- }
+
+ return {
+ tasks: [...currentState.tasks, newTask],
+ activeTaskId: id,
+ papers: newPapers,
+ }
+ })
+
return id
},🤖 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 254 - 269, The addTask flow
risks a race because it calls set() twice and reads state.papers via get()
between them; consolidate into a single atomic update by using the functional
set updater once (e.g., inside the addTask function’s set(currentState => { ...
})) to update both tasks and papers together (referencing task creation logic,
state.papers / papers mapping, and taskIds update) and then call
savePapersToStorage(get().papers) (or better, pass the updated papers returned
from the single set) after the updater so persistence uses the consistent papers
array.
This was referenced Feb 22, 2026
This was referenced Mar 13, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #134
Summary by CodeRabbit
New Features
UI/UX Enhancements