Skip to content

fix(setup): surface codesign failures, verify signatures, exit non-zero on Apple Silicon (closes #1254)#1525

Open
mvanhorn wants to merge 1 commit into
garrytan:mainfrom
mvanhorn:fix/1254-gstack-setup-codesign-silent-failure
Open

fix(setup): surface codesign failures, verify signatures, exit non-zero on Apple Silicon (closes #1254)#1525
mvanhorn wants to merge 1 commit into
garrytan:mainfrom
mvanhorn:fix/1254-gstack-setup-codesign-silent-failure

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

Summary

Setup's Apple Silicon codesign loop now surfaces codesign's stderr, verifies each signature actually applied, and exits non-zero when any binary fails to sign. The previous wrapper swallowed every error and exited 0 even when 5 of 5 binaries were left unsigned, so users hit SIGKILL minutes later with no breadcrumb.

Why this matters

After gstack setup (fresh install or upgrade), Bun-compiled binaries in ~/.claude/skills/gstack/{browse,design,make-pdf,bin}/ are killed by macOS Gatekeeper with SIGKILL (exit status 137) on first invocation. The setup itself reports success, so the failure surfaces later when a skill tries to call $B, $D, etc.

That's @lucascaro on #1254. The reporter pinpointed the codesign loop in setup lines 247-264 (now 288-303 after subsequent edits), reproduced 5/5 binaries unsigned despite a clean setup exit, and confirmed the manual recipe (codesign --remove-signature && codesign -s - -f) works on the same machine. The codesign machinery itself is fine; only the wrapper was lying.

The original codesign loop landed in #1056 / v0.18.4.0 to fix #997 (the SIGKILL class). It was correct on the happy path but its three failure modes were all silent: 2>/dev/null swallowed diagnostics, log "warning: ..." returned 0, and there was no verification step to catch a signature that didn't take.

Changes

  • setup lines 288-303 (the Darwin+arm64 codesign block) now:
    • Captures both codesign --remove-signature and codesign -s - -f stderr to a tempfile and prints it via the existing log helper if non-empty. Users see exactly what codesign said.
    • Adds codesign --verify --strict "$_bin_path" after signing. Catches the silent-corruption case the reporter hit (signature applied but doesn't actually verify).
    • Maintains a _codesign_failures counter. Both signing failures and verification failures bump it.
    • Exits 1 with a clear message when the counter is non-zero. No more "setup succeeded, SIGKILL surfaces later" footgun.
  • The Darwin+arm64 guard is unchanged. Other platforms see no behavior change.

Testing

bun test test/setup-codesign.test.ts: 6 pass / 0 fail. Old assertions about "warning, not fatal" replaced with assertions for the new fail-loud structure. The set -e smoke test still passes (the codesign block is syntactically valid bash). Existing for-loop coverage expanded to assert all 5 binaries the setup loop iterates (the test was only checking 4; setup itself already had 5).

Fixes #1254.

AI was used for assistance.

…ro on Apple Silicon

Closes garrytan#1254.

Three changes to the codesign loop in setup (lines 288-303):

1. Capture codesign stderr to a tempfile and print it via the log helper. The previous `2>/dev/null` swallowed every diagnostic, so users hit SIGKILL minutes later with no breadcrumb explaining what went wrong.

2. Run `codesign --verify --strict` after signing to catch silent corruption. The reporter's repro had 5/5 binaries unsigned despite a clean setup exit; the verify step makes that case visible at setup time.

3. Track failures and exit 1 when any binary fails to sign or verify. The previous `log "warning: ..."` returned 0 so setup silently succeeded; now it fails loud per the CLAUDE.md "never silent failures" guidance.

The Darwin+arm64 guard is unchanged. Other platforms see no behavior change. The codesign machinery itself is fine (the reporter's manual `codesign --remove-signature && codesign -s - -f` recipe works on the same machine); only the wrapper needed hardening.

test/setup-codesign.test.ts: 6 pass / 0 fail. Old assertions about 'warning, not fatal' replaced with assertions for the new fail-loud behavior. Existing for-loop coverage expanded to assert all 5 binaries (the setup loop already had 5; the test only checked 4).

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

Labels

None yet

Projects

None yet

1 participant