fix(debug-files): exit non-zero when files are dropped for size#1146
fix(debug-files): exit non-zero when files are dropped for size#1146BYK wants to merge 3 commits into
Conversation
A partial size-drop during upload silently exited 0. The all-dropped cases already fail loudly (filterBySize throws ValidationError; the scan-time path exits via doNothingToUpload), but a partial drop did not: oversized files left no result entry, so they were never counted as failures. - uploadDebugFiles now returns oversized (filterBySize-dropped) files as `error` results — most relevant for in-memory --include-sources bundles, which bypass the scan-time gate. - doUpload now receives oversizedCount/maxFileSize and exits non-zero when scan-time oversized files were dropped, matching doNothingToUpload. Surfaces Warden finding 9LL-87A on #1140.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 40f8047. Configure here.
| if (params.oversizedCount > 0) { | ||
| setExitCode(1); | ||
| return { | ||
| hint: `Uploaded ${results.length} debug file(s) to ${params.org}/${params.project}, but ${params.oversizedCount} file(s) were skipped for exceeding the maximum file size (${params.maxFileSize} bytes).`, |
There was a problem hiding this comment.
require-all hint skipped after upload
Medium Severity
When some files upload successfully but scan-time size skips occur alongside missing --id values and --require-all, doUpload returns the oversize hint and never evaluates the missing-id branch. doNothingToUpload documents that --require-all takes precedence over oversize messaging; the partial-upload path breaks that contract even though exit code stays non-zero.
Reviewed by Cursor Bugbot for commit 40f8047. Configure here.
| filesUploaded: results.length, | ||
| }); | ||
|
|
||
| // Scan-time oversized files were dropped before the queue was built, so they |
There was a problem hiding this comment.
filesUploaded counts size-drop errors
Low Severity
filesUploaded is set to results.length, but uploadDebugFiles now appends upload-time oversize drops as error results. Those entries were never assembled or uploaded, so the JSON field overstates successful uploads when --include-sources (or similar) causes a partial upload-time size drop.
Reviewed by Cursor Bugbot for commit 40f8047. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| filesUploaded: results.length, | ||
| }); | ||
|
|
||
| // Scan-time oversized files were dropped before the queue was built, so they | ||
| // never appear in `results`. Note them on any failure hint and treat them as | ||
| // a failure in their own right below. | ||
| const scanOversize = | ||
| params.oversizedCount > 0 | ||
| ? ` ${params.oversizedCount} file(s) were skipped for exceeding the maximum file size (${params.maxFileSize} bytes).` | ||
| : ""; |
There was a problem hiding this comment.
Bug: The filesUploaded count in JSON output is inflated by including oversized source bundles that were filtered out and never uploaded when using --include-sources.
Severity: MEDIUM
Suggested Fix
Calculate filesUploaded by filtering the results array to only include files with a successful upload state, rather than using results.length directly. This ensures the count accurately reflects the number of files that were actually uploaded to the server.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/debug-files/upload.ts#L409-L418
Potential issue: When using the `--include-sources` flag, source bundles are generated
in-memory, bypassing the initial file size scan. If a generated bundle exceeds the
server's maximum file size, it is filtered out during the upload process. However, the
`filesUploaded` count in the JSON output is calculated from `results.length`, which
includes both successfully uploaded files and error entries for these oversized,
non-uploaded bundles. This results in an inaccurate count in the machine-readable
output, even though the human-readable summary and exit code are correct.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Results 📊✅ Patch coverage is 90.91%. Project has 5113 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.53% 81.53% —%
==========================================
Files 397 397 —
Lines 27679 27686 +7
Branches 17966 17970 +4
==========================================
+ Hits 22566 22573 +7
- Misses 5113 5113 —
- Partials 1868 1867 -1Generated by Codecov Action |
| // Scan-time oversized files were dropped before the queue was built, so they | ||
| // never appear in `results`. Note them on any failure hint and treat them as | ||
| // a failure in their own right below. | ||
| const scanOversize = |
There was a problem hiding this comment.
Bug: The filesUploaded count is inflated because it includes oversized files that were dropped during the upload process, leading to a misleading summary for both human and JSON outputs.
Severity: MEDIUM
Suggested Fix
The calculation for filesUploaded should be adjusted to count only successful uploads. Instead of using results.length, filter the results array to include only items that do not have an error status before calculating the length. This will ensure both the filesUploaded field and the user-facing output accurately reflect the number of successfully uploaded files.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/debug-files/upload.ts#L415
Potential issue: The `doUpload` function calculates the number of uploaded files based
on the length of the `results` array. This array, returned by `uploadDebugFiles`,
contains entries for both successfully uploaded files and files that were dropped due to
being oversized. As a result, the `filesUploaded` field in the JSON output and the count
displayed in the terminal are inflated, incorrectly including files that failed to
upload. For instance, if one file is successfully uploaded and another is dropped for
being too large, the output will misleadingly report that two files were uploaded.
1 participant
Summary
Follow-up to #1140, addressing Warden finding 9LL-87A (a comment we missed before merge).
A partial size-drop during
debug-files uploadsilently exited0and printed "Uploaded N debug file(s)". The all-dropped cases already fail loudly —filterBySizethrowsValidationError, and the scan-time all-dropped path exits viadoNothingToUpload(... oversizedCount > 0 → exit 1)— but a partial drop did not honor that same "oversized ⇒ non-zero exit" contract. Oversized files left no result entry, sodoUploadnever counted them as failures.Two disjoint drop sites both had the gap:
filterBySize) — most relevant for in-memory--include-sourcessource bundles, which bypass the scan-time size gate and can only be caught here.uploadDebugFilesnow returns these aserrorresults so the existingdoUploadfailure path sets exit 1 and lists them.prepareDifs→oversizedCount) — regular oversized files are dropped before the upload queue is built.doUploadnow receivesoversizedCount/maxFileSizeand exits non-zero on a partial drop, matchingdoNothingToUpload.Accepted (in-cap) files are still uploaded; only the exit code and hint change so a partial drop is no longer mistaken for a clean success.
Warden findings on #1140
prepareFileDifnow gates onpeeked.size(fromfd.stat()) beforereadFile, so oversized files are never buffered.Tests
test/lib/api/debug-files.test.ts: the partial-drop test now asserts the oversized file is not assembled but is returned as anerrorresult.test/commands/debug-files/upload.test.ts: new test — a partial scan-time size-drop still uploads the in-cap file but exits1.pnpm run typecheck,biome check, and both affected test files (40 tests) pass.