fix: silence zip output when creating source zips

[ndhoule]

This code path is the cause of a bunch of SWAR errors. Logs don't
provide enough information for us to debug this right now, so this PR
does two things:

• Adds the -q flag to zip to quiet its output. (We pass this flag
  everywhere else we call out to zip.) I have a weak suspicion the
  root cause here could be a too-much-output error (but that seems
  unlikely).
• Improves the error output when we hit this error. This should let us
  debug the issue if it comes up again in the future.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b5676a89-0771-4f4c-ae9c-e2f6aff40ef0

📥 Commits

Reviewing files that changed from the base of the PR and between 10bd0ec and 6da3ec8.

📒 Files selected for processing (1)

• src/utils/deploy/upload-source-zip.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• netlify/blueprints (manual)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/utils/deploy/upload-source-zip.ts

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved source-zip creation for deploy uploads by using the system zip command for more consistent results.
  • Enhanced zip-failure error messages to include clearer failure details.
  • Improved consistency of temporary source-zip cleanup after zip operations.
• Tests
  • Updated unit tests to reflect the new zip execution approach.
  • Confirmed the correct zip arguments, working directory behavior, and error propagation are handled correctly.

Walkthrough

upload-source-zip.ts now runs zip through execa, handles failures with a message built from ExecaError.shortMessage and baseErr.all, and imports cleanup helpers from node:fs/promises. The unit tests now mock execa instead of child_process.execFile and assert the updated zip arguments, failure path, cleanup behavior, and subdirectory filename handling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: silencing zip output while creating source zips.
Description check	✅ Passed	The description is directly related, describing both the zip quieting change and the improved error output.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch nathanhoule/ex-2434-intermittent-zip-r-failure-when-deploying-swar-projects

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

📊 Benchmark results

Comparing with 137fbf0

• Dependency count: 1,127 (no change)
• Package size: 379 MB ⬆️ 0.00% increase vs. 137fbf0
• Number of ts-expect-error directives: 359 ⬆️ 1.95% increase vs. 137fbf0

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/unit/utils/deploy/upload-source-zip.test.ts (1)

335-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent mock return type across tests.

These two execa mocks return a synchronous {} as ExecaReturnValue, whereas the other tests (Lines 64, 127, 286) return Promise.resolve(...). The awaited call tolerates both, but aligning them avoids confusion about what execa resolves to.

♻️ Optional alignment

-    vi.mocked(mockExeca.default).mockImplementation((..._args) => {
-      return {} as ExecaReturnValue
-    })
+    vi.mocked(mockExeca.default).mockImplementation((..._args) => {
+      return Promise.resolve({} as ExecaReturnValue)
+    })

Also applies to: 374-377

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/utils/deploy/upload-source-zip.test.ts` around lines 335 - 338,
The execa mocks in the upload-source-zip tests are inconsistent with the other
cases, returning a synchronous cast instead of a resolved promise. Update the
mockImplementation blocks for the affected execa stubs to match the existing
Promise.resolve pattern used elsewhere in upload-source-zip.test.ts, so the
mocked behavior is consistent across all tests and easier to understand.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Around line 76-89: The zip upload error handling in upload-source-zip should
use the combined execa output instead of stdout alone, since zip -q often leaves
stdout empty. Update the catch block around the execa call to build the thrown
Error message from the ExecaError details using the interleaved output first,
then stderr as fallback, and keep the existing cause chaining. Use the existing
execa invocation and ExecaError handling in upload-source-zip.ts as the place to
make this change.

---

Nitpick comments:
In `@tests/unit/utils/deploy/upload-source-zip.test.ts`:
- Around line 335-338: The execa mocks in the upload-source-zip tests are
inconsistent with the other cases, returning a synchronous cast instead of a
resolved promise. Update the mockImplementation blocks for the affected execa
stubs to match the existing Promise.resolve pattern used elsewhere in
upload-source-zip.test.ts, so the mocked behavior is consistent across all tests
and easier to understand.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e33820fe-c445-48ee-b472-90460211264a

📥 Commits

Reviewing files that changed from the base of the PR and between 137fbf0 and b9049ab.

📒 Files selected for processing (2)

• src/utils/deploy/upload-source-zip.ts
• tests/unit/utils/deploy/upload-source-zip.test.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• netlify/blueprints (manual)

[davbree]

Claude:

One thing on the error output, since the whole point is making this debuggable: the message appends baseErr.stdout, but zip writes its errors/warnings to stderr, and -q empties stdout further — so stdout will usually be blank, and execa's shortMessage is just Command failed with exit code N: zip … (no stderr). Net result: the actual reason (zip error: Nothing to do!, an I/O error, the exit code, …) still won't show up.

Since you already set all: true, it's a one-word fix to capture it:

message = `${baseErr.shortMessage}\n\n${baseErr.all}`   // or baseErr.stderr

Also worth noting: the error-case test rejects with a plain new Error('zip command failed') (no .command), so it skips the ExecaError branch entirely — the enrichment isn't actually exercised. A test that rejects with a realistic ExecaError shape (command, shortMessage, stderr/all) would lock it in.

Context on the urgency: per Humio this is hitting the agent-runner create flow at ~900+ sessions/day over the last few days, and the swallowed reason is exactly what's been blocking diagnosis — so getting the real zip stderr into the message is the high-value part here.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/utils/deploy/upload-source-zip.ts (1)

82-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fix undefined all causing lint failure and add stderr fallback.

baseErr.all is typed string | undefined (only populated when all: true, which is set, but the type still allows undefined for edge cases like early spawn failures), triggering the ESLint restrict-template-expressions error. Also, per the PR's own rationale, stderr should be the fallback when all isn't populated.

🐛 Proposed fix

-      message = `${baseErr.shortMessage}\n\n${baseErr.all}`
+      message = `${baseErr.shortMessage}\n\n${baseErr.all ?? baseErr.stderr ?? ''}`

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/deploy/upload-source-zip.ts` around lines 82 - 89, The error
handling in upload-source-zip.ts’s catch block should avoid interpolating the
possibly undefined ExecaError.all value and should fall back to stderr when all
is unavailable. Update the logic around the _baseErr / baseErr path so the
message is built from shortMessage plus a defined output source, using
baseErr.all when present and baseErr.stderr otherwise, while preserving the
existing throw new Error(..., { cause: _baseErr }) behavior.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Around line 82-89: The error handling in upload-source-zip.ts’s catch block
should avoid interpolating the possibly undefined ExecaError.all value and
should fall back to stderr when all is unavailable. Update the logic around the
_baseErr / baseErr path so the message is built from shortMessage plus a defined
output source, using baseErr.all when present and baseErr.stderr otherwise,
while preserving the existing throw new Error(..., { cause: _baseErr })
behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7b6f7d3f-de3b-4033-880f-a7f0d1380d81

📥 Commits

Reviewing files that changed from the base of the PR and between b9049ab and 10bd0ec.

📒 Files selected for processing (1)

• src/utils/deploy/upload-source-zip.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• netlify/blueprints (manual)