Skip to content

fix: restore gzip artifact uploads - #1727

Merged
thymikee merged 2 commits into
mainfrom
fix/gzip-artifact-uploads
Aug 11, 2026
Merged

fix: restore gzip artifact uploads#1727
thymikee merged 2 commits into
mainfrom
fix/gzip-artifact-uploads

Conversation

@thymikee

@thymikee thymikee commented Aug 11, 2026

Copy link
Copy Markdown
Member

Summary

Restore remote app-bundle uploads by detecting gzip from the uploaded archive bytes before safe extraction. The artifact-ingestion security pass replaced system tar auto-detection with an extractor configured unconditionally for plain tar, so CLI-produced .tar.gz bundles were parsed without decompression.

This applies at the shared archive boundary used by both legacy upload and resumable finalize flows, while preserving plain tar support and the hardened manifest checks.

Closes #1726

Validation

Both legacy upload and the primary resumable finalize path now have gzip app-bundle regressions. Each test was proven red without the production fix (Unexpected end of data) and green with it. pnpm check:affected --run passes format, lint, typecheck, layering, fallow, build, and 62 related unit/provider-integration tests.

Touched 3 files; scope did not expand beyond daemon artifact ingestion. Docs and skills were not updated because the public upload contract and CLI behavior are unchanged.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.13 MB 2.13 MB -891 B
JS gzip 695.8 kB 695.5 kB -252 B
npm tarball 827.1 kB 826.9 kB -276 B
npm unpacked 2.88 MB 2.88 MB -1.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.5 ms 27.2 ms -0.3 ms
CLI --help 64.3 ms 65.0 ms +0.8 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/internal/daemon.js +227 B +105 B
dist/src/prepare-kind.js -337 B -53 B
dist/src/sdk-batch-runner.js +492 B +52 B
dist/src/screenshot-result.js -179 B -39 B
dist/src/runtime.js -146 B -22 B

Copy link
Copy Markdown
Member Author

Review

Correct, minimal, and well-targeted. The root cause and the fix location both check out.

Root cause confirmed. #1692 replaced runCmd('tar', ['xf', …]) — where system tar auto-detects compression — with the hardened tar-stream extractor called as type: 'tar' unconditionally. The CLI always produces gzip (createGzipTarArchive shells out to tar czf, src/remote/upload-client-artifact.ts:56), so every app-bundle upload was parsed without decompression.

Verified locally:

  • The new test fails on 3a089a5^ with Unexpected end of data and passes with the fix — matches the reported symptom exactly, so it isn't a vacuous test.
  • I wrote a throwaway test driving the resumable path (beginResumableUploadreceiveResumableUploadChunkfinalizeResumableUpload with artifactType: 'app-bundle'). Same failure before, correct extraction after — so the shared-boundary placement genuinely covers both flows.
  • Detection logic is sound: 2-byte gzip magic, short-read guarded, handle closed in finally, and it runs after the payload is fully written in both call sites (the resumable one under the per-entry exclusive lock), so there's no TOCTOU.
  • Sniffing rather than trusting content-type is the right call — the table in Remote daemon rejects gzipped artifact uploads — every install fails with "Invalid tar header … needs to be gunzipped" #1726 shows an uncompressed tar labeled application/gzip was accepted, and the daemon writes the payload to artifact.tar regardless, so extension-based archiveTypeFromPath couldn't help here.
  • The hardening from fix: harden artifact ingestion boundaries #1692 is untouched: symlink rejection, root-name confinement, and ArchiveBudget still apply to gzip input, since detection only selects the decompressor.

Suggestions (none blocking)

1. The test covers the fallback path, not the one installs actually take. uploadArtifact goes preflight → direct upload → /upload/finalizematerializeFinalArtifact (src/remote/upload-client.ts:52), with POST /upload only as a fallback. Every test in resumable-upload.test.ts uses artifactType: 'file', so the app-bundle finalize path has no gzip coverage at all. The fix does cover it (verified above), but nothing pins it — an app-bundle finalize case there, or a direct unit test on extractTarInstallableArtifact, is cheap insurance.

2. The same bug still lives behind another door. src/utils/archive-extraction.ts already owns type resolution via archiveTypeFromPath, and install-source-archive.ts:89 resolves the type purely from a downloaded filename that comes from content-disposition or the URL basename (install-source-download.ts:162). A remote build served as app.tar but gzipped hits this identical failure; one with no usable extension gets Unsupported archive. Putting the magic-byte detector next to archiveTypeFromPath — and letting extractArchiveSafely fall back to sniffing when the extension lies — would fix both with one helper.

3. Narrower than pre-#1692 behavior. tar xf also auto-detected bzip2/xz/zstd; this restores gzip only. Fine in practice since the CLI always gzips, and the smaller surface is arguably the better default — worth a line on the helper so it reads as a decision rather than an oversight.

4. Nit: a payload that is neither gzip nor tar still surfaces the raw tar-stream string ("Invalid tar header. Maybe the tar is corrupted or it needs to be gunzipped?"), which is what sent the reporter chasing the client. Since the daemon now reads the header anyway, rejecting unknown magic up front with an explicit AppError would be a clearer failure.

One note on re-enabling decompression in a path that was just hardened: ArchiveBudget's declared-size preflight plus per-byte charging bounds expansion at MAX_ARCHIVE_EXPANDED_BYTES (4 GiB), so a compression bomb can't blow past the byte ceiling. The residual is CPU spent decompressing filler past the tar end-marker — equally true of the old tar xf, so not a regression. I didn't test bomb behavior directly.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the actionable primary-path coverage suggestion in bdd4a34: resumable-upload.test.ts now drives app-bundle preflight state through chunk receipt and finalize, and verifies the extracted payload. I proved it red without the production fix (Unexpected end of data, test 3) and green after restoration.

I kept the implementation scoped to uploaded CLI bundles:

  • Downloaded archives with misleading or missing filename extensions are a separate input contract and would expand this outage fix into install-source inference.
  • bzip2/xz/zstd are intentionally not restored; the CLI emits gzip, and accepting more decompression formats would enlarge the ingestion surface without helping affected users.
  • I did not add an eager “unknown tar” magic check because tar identification is format-variant-sensitive and could turn clearer errors into false rejections. The safe extractor remains the validator.

The earlier iOS smoke failure was unrelated (automation-longpress visibility in the fixture); this push gives it a clean rerun. All affected local gates pass.

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head review at bdd4a34dc: clean / ready-for-human. #1726’s gzip bytes are detected at the shared app-bundle materialization boundary, so both legacy upload and resumable finalize use the safe tgz extractor without trusting content type or filename. Manifest/root/link and archive-budget protections remain in force. Both real gzip regressions are load-bearing against the old plain-tar selection, and all authoritative checks are green. No device-specific evidence applies to this daemon archive-decoder fix. No findings.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 11, 2026
@thymikee
thymikee merged commit 191d49c into main Aug 11, 2026
31 checks passed
@thymikee
thymikee deleted the fix/gzip-artifact-uploads branch August 11, 2026 08:07
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-11 08:07 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote daemon rejects gzipped artifact uploads — every install fails with "Invalid tar header … needs to be gunzipped"

1 participant