Conversation
The "Export All Evidence" button only bundled automation runs — user-uploaded files were missing. This blocked a customer acquisition deal (CS-279) where the prospect required a full evidence bundle with attachments. Extend /v1/tasks/:id/evidence/export and /v1/evidence-export/all to also include task attachments, and swap adm-zip for archiver so bundles stream to the response instead of buffering the whole ZIP in RAM. - Task-entity attachments streamed from S3 into 01-attachments/ per task, placed before automation PDFs (human evidence first, system proof second). - Missing S3 objects produce _MISSING_<name>.txt placeholders so the bundle stays auditable instead of 500ing mid-stream. - Filename collisions disambiguated with numeric suffixes. - Org-wide export now includes tasks that have only attachments (previously required at least one automation run). - Summary PDF shows attachment count. Scope confirmed with Dustin: tasks YES; vendor/risk/comment attachments, findings, risk register, KB docs — NO (fast follow via optional toggles). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four issues from the PR review, all legitimate:
- Attachment streamer treated every S3 failure as a missing object. Now only
NoSuchKey / HTTP 404 produce a `_MISSING_*.txt` placeholder. Network,
permission, and throttling errors rethrow so the archive aborts and the
user sees a real failure instead of a silently-incomplete bundle.
- `streamOrganizationEvidenceZip` was throwing NotFoundException from the
async populate task, which fired after headers were already sent — the
client got a broken stream instead of a 404. Hoisted the empty-content
check to pre-flight so it becomes a proper HTTP 404.
- Controllers now listen for client disconnect (`req.on('close')`) and abort
the archive so cancelled downloads stop consuming backend resources
(S3 fetches, background populate task).
- Org populate no longer buffers all task summaries into an array before
writing. Each task is streamed into the archive as it's processed, and
only a lightweight manifest (id / title / counts) is accumulated. Manifest
is written last.
Tests: 31 passing (was 29) — added AccessDenied rethrow, client-disconnect
abort. Pre-flight 404 test now asserts the throw is synchronous and no
archive is created.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…or codes The blanket `httpStatusCode === 404` fallback in `isS3MissingObjectError` misclassified `NoSuchBucket` (bucket misconfigured) as a per-attachment missing object. That would have produced an export that completed "successfully" with nothing but `_MISSING_*.txt` placeholders — worse than failing outright, because the customer would have no idea their bundle contains none of the actual evidence. Now we only treat the specific error codes `NoSuchKey` and `NotFound` as missing; everything else (including other 404s like `NoSuchBucket`) rethrows and aborts the archive. Added a regression test asserting NoSuchBucket aborts the archive rather than producing a placeholder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chments feat(evidence-export): include task attachments and stream large ZIPs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/tasks/evidence-export/evidence-attachment-streamer.ts">
<violation number="1" location="apps/api/src/tasks/evidence-export/evidence-attachment-streamer.ts:132">
P2: Placeholder filenames are deduplicated before `_MISSING_... .txt` wrapping, which can still generate duplicate final ZIP entry names.</violation>
</file>
<file name="apps/api/src/tasks/evidence-export/evidence-export.controller.ts">
<violation number="1" location="apps/api/src/tasks/evidence-export/evidence-export.controller.ts:299">
P1: Using `req.once('close')` for disconnect detection can abort successful exports on Node >=16/18, because request `close` also fires on normal request completion.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Add explicit regression tests for the three scenarios that aren't covered by the attachment-focused happy path: - Per-task export with automations but no attachments: verifies the 01-attachments/ folder is NOT created, no S3 calls are made, and the summary PDF is rendered with attachmentsCount=0 (line omitted). - Per-task export with neither automations nor attachments: verifies the ZIP contains only the summary PDF. - Org-wide export where no task has attachments (classic pre-PR scenario): verifies manifest shows totalAttachments=0 and no 01-attachments/ folders appear anywhere. Matches exact shape of old behaviour. - Org-wide export mixing an automation-only task with an attachment-only task: verifies both appear in the same ZIP with correct contents. All 36 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues cubic flagged on the production deploy PR (#2640): 1. Client-disconnect detection was listening on `req.once('close')`. In Node 16+ `req.close` fires on both disconnect AND normal request completion, which could falsely abort a successful export. Now we listen only on `res.close` and distinguish normal completion via `res.writableFinished` (true only after the full response is flushed, unlike `writableEnded` which only reflects that `.end()` was called). 2. Filename collisions were deduplicated against the raw attachment name, then wrapped into `_MISSING_<name>.txt` for placeholders. A legitimate upload named `_MISSING_foo.txt` plus a missing upload named `foo` could therefore both land at the same final ZIP path. Now the tracker sees the full final name (including any `_MISSING_` wrapping) so the collision is resolved on what actually ends up in the ZIP. Added two regression tests: normal-completion does not abort, and success/placeholder names don't collide in the final ZIP path. 38 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
…edback fix(evidence-export): address two follow-up review findings from prod deploy PR
Contributor
|
@cubic-dev-ai review it |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/tasks/evidence-export/evidence-export.service.ts">
<violation number="1" location="apps/api/src/tasks/evidence-export/evidence-export.service.ts:597">
P2: Attachment-only task IDs are added without verifying the task still exists, so stale attachment rows can bypass preflight and produce a misleading empty org export ZIP.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
claudfuen
pushed a commit
that referenced
this pull request
Apr 22, 2026
# [3.29.0](v3.28.0...v3.29.0) (2026-04-22) ### Bug Fixes * **evidence-export:** address review feedback on streaming export ([3814e33](3814e33)) * **evidence-export:** address two follow-up review findings ([752ed24](752ed24)), closes [#2640](#2640) * **evidence-export:** tighten S3 missing-object check to specific error codes ([317d407](317d407)) ### Features * **evidence-export:** include task attachments and stream large ZIPs ([1bc86d8](1bc86d8))
Contributor
|
🎉 This PR is included in version 3.29.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
2 participants
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.
This is an automated pull request to release the candidate branch into production, which will trigger a deployment.
It was created by the [Production PR] action.
Summary by cubic
Evidence exports now include task attachments and stream ZIPs via
archiverfor lower memory use and larger downloads. Org-wide exports also include tasks with only attachments and handle empty exports safely.New Features
01-attachments/, before automation PDFs.archiver; controllers pipe live archives and set headers._MISSING_*.txt).Bug Fixes
NoSuchKey/NotFoundcreate_MISSING_*.txt; other S3 errors abort the archive.Written for commit 0ac9c80. Summary will update on new commits.