[gontzess]

Why

Post-merge review of #160 surfaced a real bug (adjudicated from the CI reviewer thread): discoverStores's Darwin branch discarded brew --prefix's error and unconditionally added the macos-brew store. On a self-hosted mac without Homebrew (or without the brewed ca-certificates formula), the resolved path doesn't exist → openssl fails to load the -CAfile → an ERROR row → the action fails closed in both report and enforce mode — a job-blocker on brew-less macs, where the store should simply not be observed (macos-brew is residual-not-gating, so skipping it never weakens the enforce gate).

What

• probe.go: macos-etc-ssl-cert-pem is always observed; macos-brew is added only when brew --prefix succeeds, returns a non-empty prefix, and the resolved bundle exists on disk (mirrors the existing certifi guard on the Linux branch).
• TestDiscoverStoresDarwin: five cases (brew unavailable, brew error with output, empty prefix, bundle present with the correct resolved path, bundle absent) using a temporary Homebrew-shaped prefix and real filesystem checks.
• README residual list: macos-brew is observed only when Homebrew and its ca-certificates bundle are present.

Verification

• go vet + full guard Go suite green incl. the new test (0.34s); image-lock gate green.
• Not run: native macOS (no local runtime); the canary macs all have Homebrew, which is why this only bites self-hosted brew-less runners.

[github-actions]

Superseded — see the current review report for commit c1f0d396733c

General PR Review: ca-trust-guard: observe the Homebrew bundle store only when it exists

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base e9b91a2dad02.
Review mode: full
View review run

Review Summary

On Darwin, discoverStores (cmd/ca-trust-guard/probe.go:39-46) now always adds macos-etc-ssl-cert-pem. It adds macos-brew only when all three hold: brew --prefix succeeds, it prints a non-empty prefix, and <prefix>/etc/ca-certificates/cert.pem exists according to os.Stat. This follows the existing python-certifi guard on the Linux branch. Before this change, the brew error was thrown away and a store at /etc/ca-certificates/cert.pem was still added, which produced an openssl ERROR row on Macs without Homebrew. The new TestDiscoverStoresDarwin covers all five branches using a fake command runner and a real temp-dir bundle. The README residual note now describes the conditional observation. I scanned the full PR diff for security and correctness and found no new issues. Specific checks:

• filepath/os/errors/reflect imports are all present.
• The test's expected paths use the same filepath.Join, so it is portable across OSes.
• No other code depends on macos-brew always being present: the only other reference is a fixture in summarize_test.go.
• macos-brew is residual and not gating, as the PR states, so skipping it does not weaken enforce mode.

No repo-local criteria loaded, so only the base criteria were applied. Not verified: behavior on a native macOS runner, which the author also says was not run.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

General PR Review: ca-trust-guard: observe the Homebrew bundle store only when it exists

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base e9b91a2dad02.
Review mode: full
View review run

Review Summary

On Darwin, discoverStores (cmd/ca-trust-guard/probe.go:39-46) now always adds macos-etc-ssl-cert-pem. It adds macos-brew only when all three hold: brew --prefix succeeds, it prints a non-empty prefix, and <prefix>/etc/ca-certificates/cert.pem exists according to os.Stat. This follows the existing python-certifi guard on the Linux branch. Before this change, the brew error was thrown away and a store at /etc/ca-certificates/cert.pem was still added, which produced an openssl ERROR row on Macs without Homebrew. The new TestDiscoverStoresDarwin covers all five branches using a fake command runner and a real temp-dir bundle. The README residual note now describes the conditional observation. I scanned the full PR diff for security and correctness and found no new issues. Specific checks:

• filepath/os/errors/reflect imports are all present.
• The test's expected paths use the same filepath.Join, so it is portable across OSes.
• No other code depends on macos-brew always being present: the only other reference is a fixture in summarize_test.go.
• macos-brew is residual and not gating, as the PR states, so skipping it does not weaken enforce mode.

No repo-local criteria loaded, so only the base criteria were applied. Not verified: behavior on a native macOS runner, which the author also says was not run.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: c1f0d396733c

[github-actions]

No blocking issues found — see the full review report