[build] surface failures from archive rules

[titusfortner]

Ran into an issue where the build failed but the error message wasn't helpful, this digs into it.

💥 What does this PR do?

Makes the macOS browser-fetch repository rules (dmg_archive, pkg_archive) and convert_dmg.sh report real failures.

• repository_ctx.execute() return codes are now checked; failures call fail() with the captured stdout/stderr. Previously a failing convert_dmg.sh or pkgutil --expand-full surfaced three steps downstream as the cryptic Archive path … does not exist.
• convert_dmg.sh runs with set -euo pipefail, validates that hdiutil attach produced a mount point, and uses a trap to detach the volume and remove its scratch file on every exit path.
• The scratch zip moves from a shared /tmp/firefox.zip (which raced between concurrent mac_firefox / mac_beta_firefox fetches and could pick up stale state) into the repo's working directory.
• Intermediates (.dmg, converted .zip, .pkg.download, expanded .pkg/ tree) are deleted after extract — matching what deb_archive.bzl already does and avoiding ~hundreds of MB of dead weight per cached snapshot.

🔧 Implementation Notes

• I chose to duplicate _execute_or_fail helper in both dmg_archive.bzl and pkg_archive.bzl rather than extract to a separate file.
• deb_archive.bzl is unchanged — it was already correct (uses download_and_extract + repository_ctx.delete and never shells out).

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: Diagnosis of silent-failure paths and the patch across all three files.
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Surface failures from macOS archive rules with improved error handling

🐞 Bug fix

Walkthroughs

Description

• Add error handling to macOS archive rules with proper failure reporting
• Replace shared /tmp/firefox.zip with repository-local scratch file
• Add cleanup trap and validation to convert_dmg.sh script
• Delete intermediate artifacts after extraction to reduce cache size

Diagram

flowchart LR
  A["Archive Rules<br/>dmg_archive.bzl<br/>pkg_archive.bzl"] -->|"Add _execute_or_fail<br/>helper"| B["Error Handling<br/>Check return codes<br/>Report stdout/stderr"]
  A -->|"Delete intermediates<br/>after extract"| C["Cleanup<br/>Remove .dmg, .zip<br/>Remove .pkg files"]
  D["convert_dmg.sh"] -->|"Add set -euo pipefail<br/>Local scratch file"| E["Robustness<br/>Validate mount point<br/>Cleanup on exit"]
  D -->|"Replace /tmp/firefox.zip<br/>with repo-local path"| F["Concurrency<br/>Avoid race conditions<br/>between fetches"]

Loading

File Changes

1. common/private/dmg_archive.bzl 🐞 Bug fix +24/-5

Add error handling and cleanup to dmg_archive

• Add _execute_or_fail helper function to check return codes and report errors
• Wrap convert_dmg.sh execution with error handling
• Delete intermediate .dmg and .zip files after extraction

common/private/dmg_archive.bzl

---

2. common/private/pkg_archive.bzl 🐞 Bug fix +32/-8

Add error handling and cleanup to pkg_archive

• Add _execute_or_fail helper function to check return codes and report errors
• Wrap pkgutil --expand-full and mv commands with error handling
• Delete intermediate .download and expanded .pkg/ directory after processing
• Extract download_name variable for clarity

common/private/pkg_archive.bzl

---

3. common/private/convert_dmg.sh 🐞 Bug fix +29/-11

Improve error handling and concurrency safety in convert_dmg.sh

• Add set -euo pipefail for strict error handling
• Replace shared /tmp/firefox.zip with repository-local scratch file to prevent race conditions
• Add validation that hdiutil attach produced a valid mount point
• Add trap cleanup EXIT to ensure volume detach and scratch file removal on all exit paths
• Convert relative output path to absolute path for consistency

common/private/convert_dmg.sh

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2)

1. pkg_archive deletes extracted tree ✓ Resolved 🐞 Bug ≡ Correctness

Description

_pkg_archive_impl unconditionally deletes pkg_name after the (optional) move loop, so when
move is empty the rule removes the only extracted payload and leaves the repository with no usable
contents. This is a functional regression because move is not a mandatory attribute.

Code

common/private/pkg_archive.bzl[R57-58]

Evidence

The code deletes pkg_name regardless of whether any moves ran, and move is not mandatory, so an
empty move causes the extracted directory to be deleted with no replacement outputs.

common/private/pkg_archive.bzl[50-59]
common/private/pkg_archive.bzl[60-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pkg_archive` always deletes the expanded package directory (`pkg_name`) at the end. Since `move` is optional, a caller can legally omit it, in which case nothing is moved out and the extracted tree is deleted, leaving an empty external repository.

### Issue Context
The rule’s attrs define `move` as a non-mandatory `attr.string_dict()`, so an empty dict is a valid configuration.

### Fix Focus Areas
- common/private/pkg_archive.bzl[50-59]
- common/private/pkg_archive.bzl[60-71]

### Suggested fix
- Always delete the downloaded archive (`download_name`).
- Only delete the expanded directory (`pkg_name`) if `move` is non-empty (i.e., you actually moved desired artifacts out), **or** make `move` mandatory / explicitly `fail()` when `move` is empty so the rule can’t silently delete everything.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. zip lacks .app precheck 📘 Rule violation ☼ Reliability ⭐ New

Description

convert_dmg.sh invokes zip on ./*.app without first validating that the DMG actually contains
any .app bundle(s). When the glob matches nothing, this can fail with a less actionable zip
error instead of a deterministic, clear failure message.

Code

common/private/convert_dmg.sh[54]

Evidence

PR Compliance ID 10 requires validating required artifacts/command outputs and failing explicitly
with actionable errors. The updated script still directly zips ./*.app without checking that the
expected .app artifact exists, which can lead to non-actionable failures when the DMG contents
differ.

common/private/convert_dmg.sh[54-54]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`convert_dmg.sh` runs `zip -r "$SCRATCH" ./*.app` without validating that any `.app` entries exist in the mounted DMG. If none exist, the script can fail with a confusing `zip`/glob-related error rather than an explicit message about missing expected artifacts.

## Issue Context
This script is part of repository fetch rules where failures should be deterministic and actionable (explicitly validating external command outputs/artifacts).

## Fix Focus Areas
- common/private/convert_dmg.sh[54-54]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Mount parse uses stderr 🐞 Bug ☼ Reliability ⭐ New

Description

convert_dmg.sh captures hdiutil attach stdout+stderr into ATTACH_OUTPUT and later derives the mount
path from the last line, so any stderr emitted after the mount table can be parsed as the “mount
point” and trigger a false failure. The added validation will surface a clearer error, but it can
still fail even when the DMG mounted successfully.

Code

common/private/convert_dmg.sh[R42-46]

Evidence

The script explicitly merges stderr into the stream it later uses to compute the mount directory,
and then validates that derived path is a directory; this makes mount detection dependent on stderr
ordering/content.

common/private/convert_dmg.sh[42-52]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`hdiutil attach` output is captured with `2>&1` and then (elsewhere in the script) the mount point is derived from the *last line* of that combined stream. If `hdiutil` prints any stderr after the mount table, the last line may not be the tab-separated mount row, causing the script to fail even though the mount succeeded.

### Issue Context
Keep stderr for diagnostics, but do not let it influence mount-point parsing.

### Fix Focus Areas
- common/private/convert_dmg.sh[42-55]

### Suggested fix
- Capture stdout and stderr separately (e.g., redirect stderr to a temp file) and parse the mount point only from stdout.
- On failure (non-zero exit), print both captured stdout/stderr to aid debugging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. convert_dmg.sh missing arg checks 📘 Rule violation ☼ Reliability

Description

convert_dmg.sh enables set -euo pipefail but does not validate required positional args (or that
the input DMG exists), so callers can get a terse shell error instead of a deterministic, actionable
failure message. This reduces debuggability and can mask the real root cause when inputs are missing
or incorrect.

Code

common/private/convert_dmg.sh[R18-22]

Evidence

PR Compliance ID 10 requires scripts/automation to validate required inputs and fail with explicit
errors. The updated script turns on strict mode (set -euo pipefail) but still directly assigns
DMGFILE=$1 and OUTFILE=$2 without any explicit precondition checks (arg count, file existence),
which can produce non-actionable failures when invoked incorrectly.

common/private/convert_dmg.sh[18-22]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`common/private/convert_dmg.sh` uses `set -euo pipefail` and immediately reads `$1`/`$2`, but it does not explicitly validate argument count or that the DMG input exists. Per the automation/script reliability guidance, the script should fail fast with an explicit usage/error message when preconditions are not met.

## Issue Context
This script is invoked by Bazel repository rules; when inputs are missing/incorrect, failures should be deterministic and explain exactly what is wrong (e.g., missing arg, missing DMG file).

## Fix Focus Areas
- common/private/convert_dmg.sh[18-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Duplicated execute helper 🐞 Bug ⚙ Maintainability

Description

_execute_or_fail is duplicated in both dmg_archive.bzl and pkg_archive.bzl, which increases
drift risk if its formatting/behavior ever needs to change. This is minor, but it creates
unnecessary maintenance overhead.

Code

common/private/pkg_archive.bzl[R1-11]

Evidence

Both files contain the same helper implementation, indicating duplication that could drift over
time.

common/private/dmg_archive.bzl[1-12]
common/private/pkg_archive.bzl[1-12]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `_execute_or_fail` helper is duplicated in two separate .bzl files, which can lead to inconsistent behavior over time.

### Issue Context
Both implementations are currently identical.

### Fix Focus Areas
- common/private/dmg_archive.bzl[1-12]
- common/private/pkg_archive.bzl[1-12]

### Suggested fix
Create a small shared Starlark file (e.g., `common/private/exec_utils.bzl`) that defines `_execute_or_fail` (or `execute_or_fail`), and `load()` it from both archive rules.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit d3f051d

Results up to commit 020658c

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. pkg_archive deletes extracted tree ✓ Resolved 🐞 Bug ≡ Correctness

Description

_pkg_archive_impl unconditionally deletes pkg_name after the (optional) move loop, so when
move is empty the rule removes the only extracted payload and leaves the repository with no usable
contents. This is a functional regression because move is not a mandatory attribute.

Code

common/private/pkg_archive.bzl[R57-58]

Evidence

The code deletes pkg_name regardless of whether any moves ran, and move is not mandatory, so an
empty move causes the extracted directory to be deleted with no replacement outputs.

common/private/pkg_archive.bzl[50-59]
common/private/pkg_archive.bzl[60-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pkg_archive` always deletes the expanded package directory (`pkg_name`) at the end. Since `move` is optional, a caller can legally omit it, in which case nothing is moved out and the extracted tree is deleted, leaving an empty external repository.

### Issue Context
The rule’s attrs define `move` as a non-mandatory `attr.string_dict()`, so an empty dict is a valid configuration.

### Fix Focus Areas
- common/private/pkg_archive.bzl[50-59]
- common/private/pkg_archive.bzl[60-71]

### Suggested fix
- Always delete the downloaded archive (`download_name`).
- Only delete the expanded directory (`pkg_name`) if `move` is non-empty (i.e., you actually moved desired artifacts out), **or** make `move` mandatory / explicitly `fail()` when `move` is empty so the rule can’t silently delete everything.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. convert_dmg.sh missing arg checks 📘 Rule violation ☼ Reliability

Description

convert_dmg.sh enables set -euo pipefail but does not validate required positional args (or that
the input DMG exists), so callers can get a terse shell error instead of a deterministic, actionable
failure message. This reduces debuggability and can mask the real root cause when inputs are missing
or incorrect.

Code

common/private/convert_dmg.sh[R18-22]

Evidence

PR Compliance ID 10 requires scripts/automation to validate required inputs and fail with explicit
errors. The updated script turns on strict mode (set -euo pipefail) but still directly assigns
DMGFILE=$1 and OUTFILE=$2 without any explicit precondition checks (arg count, file existence),
which can produce non-actionable failures when invoked incorrectly.

common/private/convert_dmg.sh[18-22]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`common/private/convert_dmg.sh` uses `set -euo pipefail` and immediately reads `$1`/`$2`, but it does not explicitly validate argument count or that the DMG input exists. Per the automation/script reliability guidance, the script should fail fast with an explicit usage/error message when preconditions are not met.

## Issue Context
This script is invoked by Bazel repository rules; when inputs are missing/incorrect, failures should be deterministic and explain exactly what is wrong (e.g., missing arg, missing DMG file).

## Fix Focus Areas
- common/private/convert_dmg.sh[18-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Duplicated execute helper 🐞 Bug ⚙ Maintainability

Description

_execute_or_fail is duplicated in both dmg_archive.bzl and pkg_archive.bzl, which increases
drift risk if its formatting/behavior ever needs to change. This is minor, but it creates
unnecessary maintenance overhead.

Code

common/private/pkg_archive.bzl[R1-11]

Evidence

Both files contain the same helper implementation, indicating duplication that could drift over
time.

common/private/dmg_archive.bzl[1-12]
common/private/pkg_archive.bzl[1-12]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `_execute_or_fail` helper is duplicated in two separate .bzl files, which can lead to inconsistent behavior over time.

### Issue Context
Both implementations are currently identical.

### Fix Focus Areas
- common/private/dmg_archive.bzl[1-12]
- common/private/pkg_archive.bzl[1-12]

### Suggested fix
Create a small shared Starlark file (e.g., `common/private/exec_utils.bzl`) that defines `_execute_or_fail` (or `execute_or_fail`), and `load()` it from both archive rules.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[Copilot]

Pull request overview

This PR improves diagnosability and cleanup behavior for macOS-focused Bazel repository rules (dmg_archive, pkg_archive) by surfacing underlying command failures (with stdout/stderr) instead of later failing with a generic “archive path does not exist” message.

Changes:

• Add an _execute_or_fail helper to dmg_archive.bzl and pkg_archive.bzl to check repository_ctx.execute() return codes and fail() with captured output on errors.
• Harden convert_dmg.sh with set -euo pipefail, robust cleanup via trap, and a per-invocation scratch zip path to avoid /tmp races.
• Delete large intermediate artifacts after extraction (.dmg, converted .zip, .pkg.download, expanded .pkg/ tree) to reduce cached repository size.

Note: after these changes, run ./go format to avoid CI failures from formatter checks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
common/private/pkg_archive.bzl	Check pkgutil/mv execution results and delete .pkg intermediates after moving payload files.
common/private/dmg_archive.bzl	Check DMG conversion execution results and delete .dmg/.zip intermediates after extraction.
common/private/convert_dmg.sh	Make DMG attach/zip/detach more reliable with strict mode, cleanup trap, and non-/tmp scratch output.

[qodo-code-review]

Code review by qodo was updated up to the latest commit d3f051d