fix: don't import broken agents#1278
Conversation
📝 WalkthroughWalkthroughAdds UI rendering for structured import errors, changes backend import endpoints to return JSON error payloads with Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client(UI)
participant API as Server(API)
participant App as AppRunner
participant WF as wf_project
UI->>API: POST /import (zip)
API->>App: import_zip(stream)
App->>WF: read_files(main_py_dir) -- validate files
alt validation fails
WF-->>App: raises ValueError (details)
App-->>API: return 400 {"summary": "Invalid archive contents", "details": "..."}
API-->>UI: 400 JSON error
UI->>UI: populate importErrorSummary/importErrorDetails
else validation passes
App-->>App: replace files
App-->>API: 200 success
API-->>UI: 200 success
UI->>UI: hide modal, show success toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/writer/serve.py (1)
272-287:⚠️ Potential issue | 🟠 MajorEnsure temp file cleanup on failed imports.
If
app_runner.import_zipraises, the temporary file is not removed. Repeated failures can leak disk space.🧹 Suggested fix
- try: - with tempfile.NamedTemporaryFile(delete=False) as tmp: + tmp_path = None + try: + with tempfile.NamedTemporaryFile(delete=False) as tmp: # Stream file to disk to avoid memory issues size = 0 while chunk := await file.read(8192): size += len(chunk) if size > MAX_FILE_SIZE: tmp.close() os.unlink(tmp.name) raise HTTPException(status_code=413, detail={"summary": f"File too large. Max file size: {MAX_FILE_SIZE}"}) tmp.write(chunk) tmp_path = tmp.name await app_runner.import_zip(tmp_path) - os.remove(tmp_path) + finally: + if tmp_path and os.path.exists(tmp_path): + os.remove(tmp_path) except ValueError: raise HTTPException(status_code=400, detail={"summary": "Invalid archive contents", "details": traceback.format_exc()})
🤖 Fix all issues with AI agents
In `@src/ui/src/builder/BuilderHeaderMoreDropdown.vue`:
- Around line 85-86: importErrorSummary and importErrorDetails are retained
between modal opens causing stale errors to show; reset them to empty strings
before starting a new import attempt and when cancelling/closing the modal.
Locate the refs importErrorSummary and importErrorDetails in
BuilderHeaderMoreDropdown.vue and (a) clear both (set to "") at the start of the
import handler (before kicking off the import) and (b) clear them in the
cancel/close handler for the modal; optionally extract this into a small helper
like resetImportErrors() and call it from both the import-start and cancel/close
methods to avoid duplication.
In `@src/writer/serve.py`:
- Around line 286-287: The except block that currently reads "except
ValueError:" and raises HTTPException should preserve the original exception
context; change it to "except ValueError as e:" and re-raise the HTTPException
using exception chaining ("raise HTTPException(...) from e") so the original
ValueError is linked into the traceback while keeping the existing detail
payload (traceback.format_exc()) in the HTTPException.
HackerOne Code Security Review🟢 Scan Complete: 4 Issue(s) *We want to surface issues only when necessary and actionable. If we didn't get something right, or if there's more context we should take into account, reply to the comment so we'll know for the future. Here's how the code changes were interpreted and info about the tools used for scanning. 📖 Summary of ChangesThe changes focus on enhancing error handling and import processes across multiple components. Modifications include adding detailed error reporting in the UI, improving file import validation in the project runner, and refining the API endpoint's error response mechanism. These updates aim to provide more comprehensive error information during import operations.
ℹ️ Issues DetectedNOTE: These may not require action! Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem. How will I know if something is a problem?
🧰 Analysis tools
⏱️ Latest scan covered changes up to commit 5ec2d9b (latest) |
HackerOne Code Security Review🟢 Scan Complete: 4 Issue(s) Here's how the code changes were interpreted and info about the tools used for scanning. 📖 Summary of ChangesThe changes focus on enhancing error handling and reporting for the import process across multiple components. Improvements include adding detailed error references in the UI, implementing file parsing before import in the backend, and expanding error response structures to provide more comprehensive information when file uploads or processing encounters issues.
ℹ️ Issues DetectedNOTE: These may not require action! Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem. How will I know if something is a problem?
🧰 Analysis tools
⏱️ Latest scan covered changes up to commit 3bbbafa (latest) |
|
✅ Graham C reviewed all the included code changes and associated automation findings and determined that there were no immediately actionable security flaws. Note that they will continue to be notified of any new commits or comments and follow up as needed throughout the duration of this pull request's lifecycle.
Reviewed with ❤️ by PullRequest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/writer/serve.py`:
- Around line 278-281: The error currently reports MAX_FILE_SIZE as raw bytes;
update the HTTPException detail in the branch where size > MAX_FILE_SIZE to
include a human-readable size string instead of the raw number by converting
MAX_FILE_SIZE (and/or size) to a readable format (e.g., "200 MB") before
building the detail message; add or call a small helper like
human_readable_size(value) and use it in the f-string passed to HTTPException
(referencing MAX_FILE_SIZE, tmp, and the existing exception construction) so the
summary reads "File too large. Max file size: 200 MB".
🧹 Nitpick comments (2)
src/ui/src/builder/BuilderHeaderMoreDropdown.vue (1)
147-152: Consider more defensive JSON parsing.If the server returns a non-JSON error response (e.g., a 500 with an HTML body),
response.json()will throw, causing the catch block to show a generic toast while the modal error block remains empty. This is acceptable fallback behavior, but you could make it more explicit:💡 Optional: More defensive parsing
if (!response.ok) { - const parsed = await response.json(); - importErrorSummary.value = parsed?.detail?.summary; - importErrorDetails.value = parsed?.detail?.details; + try { + const parsed = await response.json(); + importErrorSummary.value = parsed?.detail?.summary ?? "Unknown error"; + importErrorDetails.value = parsed?.detail?.details; + } catch { + importErrorSummary.value = "Failed to parse server error response"; + } throw new Error("Failed to connect to import API"); }src/writer/serve.py (1)
250-253: Note: export endpoint retains plain string error format.The
export_zipendpoint at line 253 still usesdetail="Invalid mode."whileimport_zipnow uses structured errors. This is acceptable since the export flow doesn't use the same error display modal, but consider aligning for consistency if error handling is unified in the future.
|
Francisco P has submitted feedback.
Reviewed with ❤️ by PullRequest |
|
Francisco P has submitted feedback.
Reviewed with ❤️ by PullRequest |
|
Francisco P has submitted feedback.
Reviewed with ❤️ by PullRequest |
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
| except ValueError: | ||
| raise HTTPException(status_code=400, detail="Invalid upload.") | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail={"summary": "Invalid archive contents", "details": traceback.format_exc()}) from e |
There was a problem hiding this comment.
The full stack trace is included in the HTTP response which can potentially expose sensitive system details.
Risk
Exposing stack traces in API responses can:
- Aid attackers in understanding internal code structure
- Reveal sensitive configuration or environment details
- Increase the effectiveness of targeted attacks and vulnerability exploitation
References:
CWE-532: Insertion of Sensitive Information into Log File
[OWASP Error Handling Guidelines]
Recommendation:
🔺 Vulnerability (Error)
1 participant
Summary by CodeRabbit
New Features
Improvements