Download latest artifacts as a ZIP

[brynary]

Summary

• add a Download all action to the run artifacts page
• stream a ZIP that contains the latest version of each visible artifact path
• validate archive paths and avoid buffering artifact contents in memory
• document the endpoint and regenerate the API client

Test plan

• cargo nextest run -p fabro-server
• cargo nextest run -p fabro-store
• cargo nextest run -p fabro-server --features test-support all_spec_routes_are_routable
• cargo +nightly-2026-04-14 clippy -p fabro-store -p fabro-server --all-targets -- -D warnings
• cargo +nightly-2026-04-14 fmt --check --all
• cd apps/fabro-web && bun run test
• cd apps/fabro-web && bun run typecheck
• cd apps/fabro-web && bun run build
• cd lib/packages/fabro-api-client && bun run typecheck

[Copilot]

Pull request overview

This PR adds an end-to-end “Download all” flow for run artifacts: a new server endpoint streams a ZIP containing the latest version of each artifact path (matching the artifacts page’s “latest” semantics), updates the web UI to expose the action, and documents/regenerates the OpenAPI + TypeScript client surface.

Changes:

• Add GET /api/v1/runs/{id}/artifacts/download to stream a ZIP of latest artifacts without buffering full contents in memory.
• Extend fabro-store with a streaming artifact read API (get_stream) and add server-side ZIP path validation.
• Add a “Download all” action in the run artifacts page, and update OpenAPI + generated TS client.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
lib/apps/fabro-server/src/server/handler/artifacts.rs	Implements ZIP streaming endpoint, latest-artifact selection, and ZIP path validation.
lib/components/fabro-store/src/artifact_store.rs	Adds streaming read support (get_stream) and a shared relative-path validator helper.
lib/apps/fabro-server/src/server.rs	Excludes application/zip responses from response compression.
lib/apps/fabro-server/src/server/handler/mod.rs	Wires the new route in demo mode as not_implemented.
lib/apps/fabro-server/src/server/tests.rs	Adds integration test asserting ZIP contents/headers and “latest” selection behavior.
docs/public/api-reference/fabro-api.yaml	Documents the new download endpoint and response shape.
lib/packages/fabro-api-client/src/api/run-internals-api.ts	Regenerates API client bindings for downloadRunArtifacts.
apps/fabro-web/app/routes/run-artifacts.tsx	Adds “Download all” link in the artifacts UI header.
apps/fabro-web/app/lib/api-client.ts	Adds runArtifactsDownloadUrl() helper for the download href.
apps/fabro-web/app/lib/api-client.test.ts	Adds unit test for runArtifactsDownloadUrl() escaping.
lib/apps/fabro-server/Cargo.toml	Adds async_zip dependency for ZIP generation.
Cargo.toml	Adds workspace dependency entry for async_zip.
Cargo.lock	Locks the new async_zip dependency.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[brynary]

Ran a simplification pass over this branch (three parallel reviews: reuse, quality, efficiency).

Path safety moved to one place. The NUL-byte and drive-letter checks were server-only, so uploads could still store C:/x or a path with a NUL — only the ZIP read path caught them. They now live in the store's validate_filename_segments, which both writes and reads go through. validate_artifact_archive_path is gone. The download still re-validates, since artifacts stored before the rule existed can carry a bad path, but it now skips that one artifact instead of failing the whole archive with a 500.

is_boundary_stage promoted to RunProjection, replacing three byte-identical private copies (billing.rs, run_state.rs, billing_rollup.rs). The new code matched on the node name ("start" | "exit") rather than the handler type, so a working node named start would have had its artifacts silently dropped.

The archive is now compressed. Entries were Compression::Stored and the response was excluded from transfer compression, so text logs moved at full size. Enabled async_zip's deflate feature — async-compression and flate2 were already in the lock file, so no new dependencies.

Archive failures always log. if error_sender.send(...).is_ok() skipped the warn! exactly when the client had already disconnected, which is when the log is the only record. Now logs first, then signals, and renders the full error chain.

Smaller: collapsed the two duplicate 500 arms; dropped the dead stage-ID tiebreaker and the cached order from the selection map; aria-label now contains the visible "Download all" text (WCAG 2.5.3); shared the run URL prefix between the two download href builders; dropped the download attribute that duplicated the server's Content-Disposition; split the 404 case into its own test and removed the debug panic scaffolding; documented in the OpenAPI description that a mid-stream failure truncates rather than returning 500.

Considered and left alone: the PollSender/SinkWriter chain is not simplified to tokio::io::duplex — a duplex ends the body with a clean EOF, which would hand callers a truncated ZIP that looks complete. Added a comment saying so. Also left get_stream in place rather than buffering whole artifacts.

Not done, worth a follow-up: the "latest version wins" rule is implemented twice, here and in run-artifacts/group.ts. The OpenAPI description promises the ZIP matches the artifacts page, so that is a drift hazard. Fixing it means the list endpoint exposing stage order or a latest flag — a contract change beyond this PR.

Verified: cargo nextest run -p fabro-server -p fabro-store -p fabro-types -p fabro-workflow (2767 passed), all_spec_routes_are_routable, nightly clippy and fmt clean, both TS clients typecheck. The 13 failing apps/fabro-web tests are pre-existing — same 787/13 on the branch before these changes.

[brynary]

Follow-up from the efficiency review, pushed as b834e0b:

Batched the archive writes. Deflate flushes output in 8 KiB blocks, and CopyToBytes::start_send allocates and memcpys per call, so every 8 KiB was becoming its own allocation, channel send, and HTTP body frame. A 64 KiB BufWriter in front of the sink cuts all three by 8x. Per-request buffer memory goes from ~64 KiB to ~512 KiB, which is still small.

A vanished artifact no longer aborts the archive. If an object disappears between the LIST and its GET — realistically delete_for_run racing a download — the handler used to fail the whole stream, giving the caller a truncated ZIP missing everything after that point. It now logs and skips, so the caller gets a usable archive missing one file. ArtifactArchiveError::MissingArtifact is gone with it.

One thing I did not do, and I think it needs a decision. There is no cap on archive size, artifact count, or time. Per-artifact uploads are capped at 10 MB and multipart requests at 50 MB, but a run accumulates artifacts across every stage and retry, so the total is unbounded — one authenticated GET turns into unbounded object-store reads plus egress, and the router has no timeout or concurrency limit. Deflate adds CPU per in-flight download on top.

The fix is cheap because NodeArtifact.size is already in hand from list_for_run: sum it after latest_run_artifacts and return 413 above a threshold, no extra I/O. I left it out because picking the threshold is a product call — too low and large runs simply cannot be downloaded — and it adds a documented 413 to the endpoint contract. Happy to add it with whatever limit you want.

Also confirmed as non-issues, so nobody re-investigates: list_for_run is a single LIST, not N+1 (ObjectMeta carries size). The channel capacity of 8 gives real backpressure — PollSender::poll_ready returns Pending when full, which suspends the read loop. The spawned writer aborts within one chunk of a client disconnect, so no orphaned task. The compression predicate short-circuits and only affects this route. Prefetching the next artifact is not worth it: it would pipeline request latency only, and holding an unread S3 body open risks idle-read timeouts.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

docs/public/api-reference/fabro-api.yaml:3293

• The spec text says “start and exit control nodes”, but the implementation excludes boundary stages by handler type (not node ID). This description is ambiguous for workflows where the control node IDs aren’t literally start/exit (or where a real stage happens to be named start).

        Streams a ZIP archive with the latest captured version of each artifact path.
        Stage order and retry number determine the latest version, matching the artifacts page.
        Captures from the `start` and `exit` control nodes are excluded.

lib/apps/fabro-server/src/server/handler/artifacts.rs:224

• latest_run_artifacts breaks ties only on (stage_order, retry). When those are equal (e.g. unknown stages where stage_order is None), the chosen “latest” depends on iteration order rather than matching the artifacts UI’s additional stage-id tie-breaker, which can make ZIP contents surprising/non-deterministic.

        .iter_stages()
        .enumerate()
        .map(|(order, (stage_id, _))| (stage_id.clone(), order))
        .collect::<HashMap<_, _>>();

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

lib/components/fabro-store/src/artifact_store.rs:538

• If ':' is rejected in artifact paths for Windows safety, it should be covered by this validation test set (e.g. logs:output.txt or file.txt:ads) to prevent regressions.

        for filename in [
            "",
            "/escape.txt",
            "../escape.txt",
            "logs//output.txt",
            "logs/./output.txt",
            r"logs\output.txt",
            "C:/escape.txt",
            "c:escape.txt",
            "bad\0name.txt",
        ] {

lib/apps/fabro-server/src/server/handler/artifacts.rs:311

• This logs a warning and tries to poison the body even when the failure is just the client disconnecting mid-download (the sink maps receiver drop to BrokenPipe). That can create noisy/misleading warnings in normal usage (canceled downloads). Consider treating BrokenPipe as a normal termination and skipping the warn/error signalling in that case (similar to how sandbox stdin handling treats BrokenPipe as non-error).

    tokio::spawn(async move {
        if let Err(error) = write_artifact_archive(writer, artifact_store, run_id, artifacts).await
        {
            // Log before signalling: the send fails when the caller has already
            // gone away, and that is exactly when this log is the only record

lib/components/fabro-store/src/artifact_store.rs:272

• validate_filename_segments still permits ':' in non-drive-letter positions (e.g. logs:output.txt or file.txt:ads). On Windows/NTFS this can be an invalid filename and can also be interpreted as an Alternate Data Stream, so allowing it undermines the stated “portable/safe on Windows” guarantee and could create surprising extraction behavior.

This issue also appears on line 528 of the same file.

    let bytes = filename.as_bytes();
    if bytes.first().is_some_and(u8::is_ascii_alphabetic) && bytes.get(1) == Some(&b':') {
        return Err(Error::Other(
            "artifact filename must not start with a drive letter".to_string(),
        ));
    }
    let segments = filename.split('/').collect::<Vec<_>>();

[brynary]

Worked through the Copilot review. All three findings were valid — two of them against changes I made in the simplify pass, not the original PR. Fixed in 8b767c6.

Tie-break ordering (inline thread, now resolved). The original &artifact.node tie-breaker disagreed with the artifacts page. My cleanup commit removed it as unreachable, which was wrong — two artifacts can share a filename and retry while both come from stages absent from the projection. That just moved the non-determinism to "whichever the object store listed first". The tie-break is now the serialized node@visit string, matching what group.ts:86 compares. Deliberately not StageId's own Ord, which compares the visit numerically and would disagree, so unknown@2 beats unknown@10 in both places now. Pinned by a test.

Spec wording (suppressed comment on fabro-api.yaml:3293). Correct, and it was my regression: I changed the exclusion from node name to handler type but left the spec saying "the start and exit control nodes". It now says boundary nodes are identified by handler type rather than by name, so a node named start that does real work keeps its artifacts. Client docs regenerated.

Non-deterministic ties (suppressed comment on artifacts.rs:224). Same root cause as the first, fixed by the same change.

Copilot cannot resolve its own suppressed comments, so I'm answering them here rather than leaving them looking unaddressed.

Two things Copilot did not raise that are still open, both from the earlier review pass and unchanged:

• No cap on archive size or artifact count. Detailed in the comment above — the fix is cheap, but the threshold is a product call I'd rather you make.
• The "latest version wins" rule is still implemented twice, here and in run-artifacts/group.ts. The byte-vs-localeCompare caveat on the new tie-break is a symptom of that. Fixing it properly means the list endpoint exposing stage order or a latest flag.

Verified after the change: 2767 tests pass across the four touched crates, nightly clippy and fmt clean, both generated clients regenerated and typechecking.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/packages/fabro-api-client/src/api/run-internals-api.ts:170

• downloadRunArtifacts returns an AxiosPromise<File> for application/zip, but the generated request options don't set an Axios responseType. With Axios defaults, this will be treated as text/JSON and can corrupt the ZIP payload unless every caller remembers to override options.responseType manually.

            const localVarRequestOptions = { method: 'GET', ...baseOptions, ...options};
            const localVarHeaderParameter = {} as any;
            const localVarQueryParameter = {} as any;

lib/apps/fabro-server/src/server/tests.rs:10607

• This enum literal has extra spacing (actor: None) that will cause cargo fmt --check to fail (and diverges from rustfmt output elsewhere).

        workflow_event::Event::RunRunnable {
            source: fabro_types::RunRunnableSource::StartRequested,
            actor:  None,
        },