ci(sandbox): add macOS runner for darwin-specific tar behavior test

[rh-hemartin]

Adding a macOS runner to test behavior related to tar in macOS.

Closes #2032

[github-actions]

Site preview

Preview: https://ec5f6e47-site.fullsend-ai.workers.dev

Commit: db0772f02be0ad0f6b9503e358f27d1449b704c4

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:40 AM UTC · Completed 9:49 AM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR links to issue macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip #2032 and explains the rationale for adding a macOS CI runner for darwin-specific bsdtar behavior testing. Human approval is always required for protected-path changes, regardless of context.

---

Labels: PR already has appropriate labels (component/ci, component/sandbox).

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR links to issue macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip #2032 and explains the rationale for adding a macOS CI runner. Human approval is always required for protected-path changes, regardless of context.

• [architectural-fit] .github/workflows/lint.yml:54 — Adding a macOS runner for darwin-specific tests incurs significant CI cost (macOS runners are ~10x more expensive than Linux). The existing unit test at sandbox_test.go:418 verifies COPYFILE_DISABLE=1 is set via a fake tar shim. The new integration test validates real bsdtar behavior on macOS, which is orthogonal coverage. Consider whether this should run on every PR or on a schedule to reduce cost.

Low

• [test-adequacy] internal/sandbox/sandbox_darwin_test.go:82 — The subject assertion (assert.False(t, hasAppleDouble(subjectMembers))) can pass trivially when the negative control also produces no ._* files. If the xattr does not trigger AppleDouble generation on the CI runner, the test provides zero signal. The negative control only emits t.Log (warning) rather than t.Skip, making the no-signal condition invisible in CI output.
  Remediation: Replace t.Log("warning: ...") with t.Skip("control tar without COPYFILE_DISABLE produced no ._* files — skipping") so CI clearly reports when the test was not exercised.

• [test-naming-convention] internal/sandbox/sandbox_darwin_test.go:20 — Test name TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble deviates from the dominant TestFunctionName_Scenario pattern in sandbox_test.go (e.g., TestUploadDir_TarIncludesCopyfileDisable). The deviation is partially justified since this test exercises raw bsdtar behavior rather than a specific exported function.

• [tier-mismatch] .github/workflows/lint.yml — PR title uses fix(sandbox) but the actual COPYFILE_DISABLE=1 fix already exists in sandbox.go:525. This PR adds test infrastructure (macOS CI runner + darwin-specific integration test), which is more accurately a test or ci type change.

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR links to issue macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip #2032 and explains the rationale for adding a macOS CI runner. Human approval is always required for protected-path changes, regardless of context.

Low

• [naming-clarity] .github/workflows/lint.yml:41 — The job name test-macos suggests a general macOS test suite, but it runs only go test -race ./internal/sandbox/.... The existing test job runs make test (full suite) on Ubuntu. The name asymmetry may cause confusion as the job scope grows or stays narrow.
  Remediation: Rename the job to test-sandbox-darwin or test-sandbox-macos to accurately reflect that it tests only darwin-specific sandbox behavior.
• [test-file-organization] internal/sandbox/sandbox_darwin_test.go — This PR introduces the first platform-specific test file (with //go:build darwin). No other *_darwin.go or *_linux.go files exist in the codebase. However, using Go build tags for platform-specific tests is idiomatic and preferable to runtime OS checks when the test requires platform-specific tooling (xattr, real bsdtar). No action strictly required.

Info

• [architectural-efficiency] .github/workflows/lint.yml:41 — The narrowed scope (from full suite to ./internal/sandbox/...) resolves prior duplication concerns. The existing TestUploadDir_TarIncludesCopyfileDisable validates that COPYFILE_DISABLE=1 is passed to tar's environment but uses a fake tar shim — it does not exercise real bsdtar behavior. The new darwin test provides genuine end-to-end validation that real macOS bsdtar suppresses AppleDouble files.
• [test-naming-convention] internal/sandbox/sandbox_darwin_test.go:16 — The test name TestUploadDir_SuppressesAppleDoubleInTarball follows the established positive-assertion naming pattern used throughout sandbox_test.go.

Previous run (3)

Review

Findings

High

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR has no linked issue and the description provides only a brief rationale for modifying CI infrastructure. Human approval is always required for protected-path changes; a linked issue documenting why a macOS CI runner is needed would provide sufficient context.
  Remediation: Create a GitHub issue documenting the need for macOS-specific CI testing and link it in the PR description.

Medium

• [architectural-consistency] .github/workflows/lint.yml:41 — The new test-macos job runs make go-test (go test -race -cover ./...), executing the full Go test suite on macOS. Only TestUploadDir_NoAppleDoubleInTarball requires macOS (gated by runtime.GOOS != "darwin" skip). This duplicates ~99% of the testing already performed on ubuntu-latest and increases CI cost (macOS runners are significantly more expensive).
  Remediation: Run only sandbox tests on macOS: go test -race ./internal/sandbox/...
• [edge-case] internal/sandbox/sandbox_test.go:497 — The negative control sets controlCmd.Env = os.Environ() without filtering out a pre-existing COPYFILE_DISABLE environment variable. If the macOS CI runner already has COPYFILE_DISABLE set, the control tar will not produce AppleDouble files and the test will hard-fail via t.Fatalf (not t.Skip), causing a CI-blocking failure rather than a graceful degradation.
  Remediation: Either filter COPYFILE_DISABLE out of the control command's environment, or change the t.Fatalf to t.Skip so the test degrades gracefully when the control cannot produce AppleDouble files.

Low

• [logic-error] internal/sandbox/sandbox_test.go:481 — hasAppleDouble uses strings.Contains(filepath.Base(m), "._") which matches ._ anywhere in the filename, not just as a prefix. AppleDouble files always start with ._, so strings.HasPrefix would be more precise. In practice, the risk is minimal within this test's controlled inputs, but using HasPrefix is semantically correct.

Info

• [test-naming-convention] internal/sandbox/sandbox_test.go:424 — The test name TestUploadDir_NoAppleDoubleInTarball uses a negative assertion, while nearby tests use positive descriptions (e.g., TestSanitizeDownload_RemovesAppleDoubleInGitDir). Consider TestUploadDir_SuppressesAppleDoubleInTarball for consistency.

Previous run (4)

Review

Findings

High

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR has no linked issue and the body provides only a brief description without detailed justification for modifying CI infrastructure. Human approval is always required for protected-path changes; a linked issue documenting the rationale (why macOS-specific CI is needed, cost/benefit analysis) would provide sufficient context.
  Remediation: Create a GitHub issue documenting why a macOS runner is needed and link it in the PR description.

Medium

• [architectural-consistency] .github/workflows/lint.yml:41 — The new test-macos job runs make go-test (go test -race -cover ./...), executing the full Go test suite on macOS. Only TestUploadDir_NoAppleDoubleInTarball requires macOS (gated by runtime.GOOS != "darwin" skip). This duplicates ~99% of the testing already performed on ubuntu-latest and increases CI cost (macOS runners are significantly more expensive).
  Remediation: Run only macOS-specific tests, e.g., go test -run TestUploadDir_NoAppleDoubleInTarball ./internal/sandbox/.

Low

• [logic-error] internal/sandbox/sandbox_test.go:481 — hasAppleDouble uses strings.Contains(filepath.Base(m), "._") which matches ._ anywhere in the filename, not just as a prefix. AppleDouble files always start with ._, so strings.HasPrefix would be more precise. In practice, the risk is minimal within this test's controlled inputs, but using HasPrefix is semantically correct.
• [edge-case] internal/sandbox/sandbox_test.go:489 — The negative control sets controlCmd.Env = os.Environ() without filtering out a pre-existing COPYFILE_DISABLE. If the CI runner's environment already has this variable set, the control will not produce AppleDouble files and the test will skip — providing no signal. The test correctly skips rather than giving a false pass, so this is a robustness concern rather than a correctness bug.

Info

• [commit-message-convention] .github/workflows/lint.yml — The PR title "Add macOS runner due to some specialized tests" does not follow Conventional Commits format required by CLAUDE.md. Suggested: ci: add macOS runner for darwin-specific tar tests.

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR links to issue macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip #2032 and explains the rationale for adding a macOS CI runner. Human approval is always required for protected-path changes, regardless of context.

• [architectural-fit] .github/workflows/lint.yml:54 — Adding a macOS runner for darwin-specific tests incurs significant CI cost (macOS runners are ~10x more expensive than Linux). The existing unit test at sandbox_test.go:418 verifies COPYFILE_DISABLE=1 is set via a fake tar shim. The new integration test validates real bsdtar behavior on macOS, which is orthogonal coverage. Consider whether this should run on every PR or on a schedule to reduce cost.

Low

• [test-adequacy] internal/sandbox/sandbox_darwin_test.go:82 — The subject assertion (assert.False(t, hasAppleDouble(subjectMembers))) can pass trivially when the negative control also produces no ._* files. If the xattr does not trigger AppleDouble generation on the CI runner, the test provides zero signal. The negative control only emits t.Log (warning) rather than t.Skip, making the no-signal condition invisible in CI output.
  Remediation: Replace t.Log("warning: ...") with t.Skip("control tar without COPYFILE_DISABLE produced no ._* files — skipping") so CI clearly reports when the test was not exercised.

• [test-naming-convention] internal/sandbox/sandbox_darwin_test.go:20 — Test name TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble deviates from the dominant TestFunctionName_Scenario pattern in sandbox_test.go (e.g., TestUploadDir_TarIncludesCopyfileDisable). The deviation is partially justified since this test exercises raw bsdtar behavior rather than a specific exported function.

• [tier-mismatch] .github/workflows/lint.yml — PR title uses fix(sandbox) but the actual COPYFILE_DISABLE=1 fix already exists in sandbox.go:525. This PR adds test infrastructure (macOS CI runner + darwin-specific integration test), which is more accurately a test or ci type change.

---

Labels: PR modifies CI workflow and sandbox test infrastructure.

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR links to issue macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip #2032 and explains the rationale for adding a macOS CI runner. Human approval is always required for protected-path changes, regardless of context.

Low

• [naming-clarity] .github/workflows/lint.yml:41 — The job name test-macos suggests a general macOS test suite, but it runs only go test -race ./internal/sandbox/.... The existing test job runs make test (full suite) on Ubuntu. The name asymmetry may cause confusion as the job scope grows or stays narrow.
  Remediation: Rename the job to test-sandbox-darwin or test-sandbox-macos to accurately reflect that it tests only darwin-specific sandbox behavior.
• [test-file-organization] internal/sandbox/sandbox_darwin_test.go — This PR introduces the first platform-specific test file (with //go:build darwin). No other *_darwin.go or *_linux.go files exist in the codebase. However, using Go build tags for platform-specific tests is idiomatic and preferable to runtime OS checks when the test requires platform-specific tooling (xattr, real bsdtar). No action strictly required.

Info

• [architectural-efficiency] .github/workflows/lint.yml:41 — The narrowed scope (from full suite to ./internal/sandbox/...) resolves prior duplication concerns. The existing TestUploadDir_TarIncludesCopyfileDisable validates that COPYFILE_DISABLE=1 is passed to tar's environment but uses a fake tar shim — it does not exercise real bsdtar behavior. The new darwin test provides genuine end-to-end validation that real macOS bsdtar suppresses AppleDouble files.
• [test-naming-convention] internal/sandbox/sandbox_darwin_test.go:16 — The test name TestUploadDir_SuppressesAppleDoubleInTarball follows the established positive-assertion naming pattern used throughout sandbox_test.go.

Previous run (3)

Review

Findings

High

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR has no linked issue and the description provides only a brief rationale for modifying CI infrastructure. Human approval is always required for protected-path changes; a linked issue documenting why a macOS CI runner is needed would provide sufficient context.
  Remediation: Create a GitHub issue documenting the need for macOS-specific CI testing and link it in the PR description.

Medium

• [architectural-consistency] .github/workflows/lint.yml:41 — The new test-macos job runs make go-test (go test -race -cover ./...), executing the full Go test suite on macOS. Only TestUploadDir_NoAppleDoubleInTarball requires macOS (gated by runtime.GOOS != "darwin" skip). This duplicates ~99% of the testing already performed on ubuntu-latest and increases CI cost (macOS runners are significantly more expensive).
  Remediation: Run only sandbox tests on macOS: go test -race ./internal/sandbox/...
• [edge-case] internal/sandbox/sandbox_test.go:497 — The negative control sets controlCmd.Env = os.Environ() without filtering out a pre-existing COPYFILE_DISABLE environment variable. If the macOS CI runner already has COPYFILE_DISABLE set, the control tar will not produce AppleDouble files and the test will hard-fail via t.Fatalf (not t.Skip), causing a CI-blocking failure rather than a graceful degradation.
  Remediation: Either filter COPYFILE_DISABLE out of the control command's environment, or change the t.Fatalf to t.Skip so the test degrades gracefully when the control cannot produce AppleDouble files.

Low

• [logic-error] internal/sandbox/sandbox_test.go:481 — hasAppleDouble uses strings.Contains(filepath.Base(m), "._") which matches ._ anywhere in the filename, not just as a prefix. AppleDouble files always start with ._, so strings.HasPrefix would be more precise. In practice, the risk is minimal within this test's controlled inputs, but using HasPrefix is semantically correct.

Info

• [test-naming-convention] internal/sandbox/sandbox_test.go:424 — The test name TestUploadDir_NoAppleDoubleInTarball uses a negative assertion, while nearby tests use positive descriptions (e.g., TestSanitizeDownload_RemovesAppleDoubleInGitDir). Consider TestUploadDir_SuppressesAppleDoubleInTarball for consistency.

Previous run (4)

Review

Findings

High

• [protected-path] .github/workflows/lint.yml — This file is under the .github/ protected path. The PR has no linked issue and the body provides only a brief description without detailed justification for modifying CI infrastructure. Human approval is always required for protected-path changes; a linked issue documenting the rationale (why macOS-specific CI is needed, cost/benefit analysis) would provide sufficient context.
  Remediation: Create a GitHub issue documenting why a macOS runner is needed and link it in the PR description.

Medium

• [architectural-consistency] .github/workflows/lint.yml:41 — The new test-macos job runs make go-test (go test -race -cover ./...), executing the full Go test suite on macOS. Only TestUploadDir_NoAppleDoubleInTarball requires macOS (gated by runtime.GOOS != "darwin" skip). This duplicates ~99% of the testing already performed on ubuntu-latest and increases CI cost (macOS runners are significantly more expensive).
  Remediation: Run only macOS-specific tests, e.g., go test -run TestUploadDir_NoAppleDoubleInTarball ./internal/sandbox/.

Low

• [logic-error] internal/sandbox/sandbox_test.go:481 — hasAppleDouble uses strings.Contains(filepath.Base(m), "._") which matches ._ anywhere in the filename, not just as a prefix. AppleDouble files always start with ._, so strings.HasPrefix would be more precise. In practice, the risk is minimal within this test's controlled inputs, but using HasPrefix is semantically correct.
• [edge-case] internal/sandbox/sandbox_test.go:489 — The negative control sets controlCmd.Env = os.Environ() without filtering out a pre-existing COPYFILE_DISABLE. If the CI runner's environment already has this variable set, the control will not produce AppleDouble files and the test will skip — providing no signal. The test correctly skips rather than giving a false pass, so this is a robustness concern rather than a correctness bug.

Info

• [commit-message-convention] .github/workflows/lint.yml — The PR title "Add macOS runner due to some specialized tests" does not follow Conventional Commits format required by CLAUDE.md. Suggested: ci: add macOS runner for darwin-specific tar tests.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:53 AM UTC · Completed 10:03 AM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:41 AM UTC · Completed 10:49 AM UTC
Commit: ba204cb · View workflow run →

[waynesun09]

Review Squad (5 agents: Claude x3, Gemini, Codex) — 6 new findings (2 HIGH, 4 MEDIUM)

HIGH — Wrong Conventional Commit prefix
PR title uses fix(sandbox): but this PR adds CI infrastructure and a test file — no user-visible bug fix. fix feeds into release notes under "Bug Fixes" via GoReleaser. Suggest: ci(sandbox): add macOS runner for darwin-specific tar behavior test or test(sandbox): ...

Flagged by 3/5 review agents (consensus).

[qodo-code-review]

PR Summary by Qodo

Add macOS CI job to validate COPYFILE_DISABLE tar behavior
🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Description

• Add a macOS GitHub Actions job to run sandbox tests on Darwin.
• Introduce a darwin-only integration test for bsdtar AppleDouble suppression.
• Validate COPYFILE_DISABLE=1 prevents ._* entries in generated tarballs.

Diagram

graph TD
  A[".github/workflows/lint.yml"] --> B["Job: test-sandbox-darwin"] --> C["go test ./internal/sandbox/..."] --> D["darwin test: sandbox_darwin_test.go"] --> E{{"macOS tools: xattr + tar"}} --> F("tar.gz artifact") --> G["Go reads tar: gzip+tar"]
  subgraph Legend
    direction LR
    _wf["Workflow/Job"] ~~~ _ext{{"External command"}} ~~~ _art("Artifact")
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Linux-only test using libarchive/bsdtar

• ➕ Avoids macOS runner cost and scheduling variability
• ➕ Keeps CI homogeneous and faster to provision
• ➖ Does not reliably reproduce macOS AppleDouble generation behavior
• ➖ May miss regressions tied to macOS tar/xattr semantics

2. Unit test only: assert env var is set (no external tar)

• ➕ Fast and deterministic; no reliance on OS tools
• ➕ Works on all CI platforms
• ➖ Only validates implementation intent, not OS-level outcome
• ➖ Could still regress if toolchain behavior changes on macOS

Recommendation: Keep the macOS runner + OS-level integration test. The risk being addressed is platform-specific behavior (AppleDouble emission by macOS tar when xattrs exist), which is hard to emulate faithfully on non-Darwin runners. The added negative control (tar without COPYFILE_DISABLE) is a good guardrail to ensure the test can actually detect the presence of ._* entries.

Files changed (2) +110 / -0

Tests (1) +94 / -0

sandbox_darwin_test.goDarwin integration test for AppleDouble suppression +94/-0

Darwin integration test for AppleDouble suppression

• Adds a darwin-only test that applies an xattr to a file, creates tarballs with and without COPYFILE_DISABLE=1, and inspects the tar members using Go's gzip/tar readers. Asserts that COPYFILE_DISABLE=1 prevents ._* AppleDouble entries, with a negative control to confirm detectability.

internal/sandbox/sandbox_darwin_test.go

Other (1) +16 / -0

lint.ymlAdd macOS sandbox test job +16/-0

Add macOS sandbox test job

• Introduces a new GitHub Actions job that runs Go sandbox tests on macOS. This ensures darwin-specific behavior is exercised in CI (including tar/AppleDouble handling).

.github/workflows/lint.yml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 macOS tar includes ._* AppleDouble files in UploadDir, corrupting .git on round-trip

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Weak negative control check 🐞 Bug ⚙ Maintainability

Description

The darwin test only logs when the control tarball lacks ._* members, so it may silently run in an
environment where AppleDouble generation isn’t being exercised. This weakens the test’s diagnostic
value because it won’t clearly indicate when the prerequisite behavior (AppleDouble without
COPYFILE_DISABLE) isn’t present.

Code

internal/sandbox/sandbox_darwin_test.go[R89-93]

+	// Bonus: verify the negative control produced ._* files, proving the test can detect regressions.
+	controlMembers := listTarMembers(controlTar)
+	if !hasAppleDouble(controlMembers) {
+		t.Log("warning: control tar without COPYFILE_DISABLE produced no ._* files — xattr may not have applied")
+	}

Relevance

⭐⭐ Medium

No clear precedent on failing vs logging for unmet negative controls in tests; only general
test-hardening history.

PR-#2040
PR-#2182

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly chooses to only log (not fail/skip) when the negative control condition is not
met.

internal/sandbox/sandbox_darwin_test.go[89-93]

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 test’s negative control is not enforced; it only logs if `. _*` files are missing from the control tarball. That reduces confidence the environment is actually exercising AppleDouble behavior.

### Issue Context
The main assertion (subject tarball has no `._*` with `COPYFILE_DISABLE=1`) still runs, but if the runner never generates AppleDouble files in the first place, the test provides less signal.

### Fix Focus Areas
- internal/sandbox/sandbox_darwin_test.go[89-93]

### Suggested fix
- Replace the log-only block with either:
 - `require.True(t, hasAppleDouble(controlMembers), "...")` if you want to hard-enforce the prerequisite, or
 - `if !hasAppleDouble(controlMembers) { t.Skip("...") }` to avoid false failures while still ensuring the test is meaningful when it runs.

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

---

2. Tar binary not pinned 🐞 Bug ☼ Reliability

Description

TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble intends to validate macOS bsdtar behavior, but
it executes "tar" via PATH, so the test may run a different tar implementation (or wrapper)
depending on environment. This can make the test non-deterministic and reduce confidence it’s
exercising the intended macOS-specific behavior.

Code

internal/sandbox/sandbox_darwin_test.go[R71-82]

+	controlTar := filepath.Join(t.TempDir(), "control.tar.gz")
+	controlCmd := exec.Command("tar", "-czf", controlTar, "-C", srcDir, ".")
+	controlCmd.Env = controlEnv
+	if out, err := controlCmd.CombinedOutput(); err != nil {
+		t.Fatalf("control tar failed: %v: %s", err, out)
+	}
+	// Subject: tar WITH COPYFILE_DISABLE=1 (matching UploadDir) must produce no ._* files.
+	// Run unconditionally — this is the actual assertion under test.
+	subjectTar := filepath.Join(t.TempDir(), "subject.tar.gz")
+	subjectCmd := exec.Command("tar", "-czf", subjectTar, "-C", srcDir, ".")
+	subjectCmd.Env = append(controlEnv, "COPYFILE_DISABLE=1")
+	out, err := subjectCmd.CombinedOutput()

Relevance

⭐ Low

Repo relies on PATH tar; PR#2040 tests override PATH with fake tar to inspect env.

PR-#2040
PR-#1079

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test’s own comment says it exercises macOS bsdtar, but the implementation uses PATH-resolved
tar. Elsewhere in the sandbox tests, PATH is intentionally manipulated to intercept tar,
demonstrating this codebase already relies on PATH resolution behavior.

internal/sandbox/sandbox_darwin_test.go[19-22]
internal/sandbox/sandbox_darwin_test.go[71-83]
internal/sandbox/sandbox_test.go[425-434]

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 darwin-specific test claims to exercise macOS `/usr/bin/tar` (bsdtar), but currently invokes `tar` via PATH. If PATH precedence changes (e.g., a different `tar` is installed earlier in PATH), the test may validate the wrong behavior.

### Issue Context
This test is meant to validate OS-level behavior specific to macOS bsdtar and AppleDouble generation/suppression.

### Fix Focus Areas
- internal/sandbox/sandbox_darwin_test.go[71-82]

### Suggested fix
- Invoke `/usr/bin/tar` explicitly for both control and subject commands (and optionally `/usr/bin/xattr` too), or detect/verify the tar implementation (e.g., via `tar --version`) and `t.Skip` if it is not the expected macOS tar.

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

---

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 6:04 AM UTC · Ended 6:17 AM UTC
Commit: 4e21a60 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] architectural-fit

Adding a macOS runner for darwin-specific tests incurs significant CI cost (macOS runners are ~10x more expensive than Linux). The existing unit test at sandbox_test.go:418 verifies COPYFILE_DISABLE=1 is set via a fake tar shim. The new integration test validates real bsdtar behavior on macOS, which is orthogonal coverage. Consider whether this should run on every PR or on a schedule to reduce cost.

[fullsend-ai-review]

[low] test-adequacy

The subject assertion can pass trivially when the negative control also produces no ._* files. If the xattr does not trigger AppleDouble generation on the CI runner, the test provides zero signal. The negative control only emits t.Log (warning) rather than t.Skip, making the no-signal condition invisible in CI output.

Suggested fix: Replace t.Log("warning: ...") with t.Skip("control tar without COPYFILE_DISABLE produced no ._* files — skipping") so CI clearly reports when the test was not exercised.

[fullsend-ai-review]

[low] test-naming-convention

Test name TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble deviates from the dominant TestFunctionName_Scenario pattern in sandbox_test.go. The deviation is partially justified since this test exercises raw bsdtar behavior rather than a specific exported function.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:04 AM UTC · Completed 6:17 AM UTC
Commit: 9a9f810 · View workflow run →

[waynesun09]

LGTM — prior review findings addressed. One remaining nit: PR title uses fix(sandbox): but should be ci(sandbox): or test(sandbox): since this adds CI infrastructure and a test, not a user-visible bug fix. This will affect release note categorization via GoReleaser.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:53 AM UTC · Ended 6:58 AM UTC
Commit: ff8ec85 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:01 AM UTC · Completed 7:13 AM UTC
Commit: db0772f · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:23 AM UTC · Completed 7:29 AM UTC
Commit: db0772f · View workflow run →

[fullsend-ai-retro]

Retro: PR #2063 — macOS CI runner for darwin-specific tar test

Timeline: Human-authored PR opened Jun 9, merged Jun 25 (16 days). The review bot ran 5+ times and flagged protected-path, architectural scope, and minor code issues. However, a human "Review Squad" (5 agents) on Jun 22 caught 6 additional findings — including two HIGH-severity issues — that the review bot missed: stale GitHub Actions versions (checkout v6 vs v7 in the same repo) and missing token isolation (GH_TOKEN/GITHUB_TOKEN blanking present in sibling jobs but absent from the new job). The author addressed all findings and the final version was significantly improved.

What went well:

• The review bot correctly flagged protected-path, over-broad test scope (make go-test vs targeted go test -run), and minor code issues (strings.Contains vs HasPrefix).
• The final merged version addressed all review findings comprehensively (Go stdlib instead of python3, narrowed CI scope, token isolation, updated action versions, renamed test).
• Multiple review perspectives (bot + human squad) produced a thorough review.

Key gap: The review bot failed to detect inconsistent configuration between sibling CI jobs in the same workflow file — specifically missing security env blanking and outdated action versions compared to adjacent jobs. These are pattern-consistency checks that don't require external knowledge.

Proposals: 1 filed (sibling-job consistency checks). Skipped proposals for redundant review deduplication (covered by #1418, #1422, #1014), protected-path tuning (covered by #2614, #1551, #2588), and review bot contradiction handling (covered by #2484).

Proposals filed

• Review agent: detect inconsistent config between sibling CI workflow jobs (in fullsend-ai/fullsend)