Dashboard › spotlight › Session 1JcT5oD2eMXB
1JcT5oD2eMXBeG2s8Perform a rigorous, skeptical adversarial CORRECTNESS review of a single-file CI change. Focus STRICTLY on real bugs — shell-quoting errors, logic errors, edge cases, data corruption, wrong output. IGNORE style/cosmetic issues.
Repo: /home/byk/Code/getsentry/spotlight Branch: fix/notarize-key-pem-armor (PR #1334), base main.
Read the diff with: git -C /home/byk/Code/getsentry/spotlight diff origin/main...fix/notarize-key-pem-armor -- .github/workflows/build.yml
Context / intent:
APPLE_API_KEY GitHub secret is base64-encoded JSON containing fields: issuer_id, key_id, private_key.private_key field is a BARE base64 DER body of a PKCS#8 EC P-256 private key with NO PEM armor (no BEGIN/END, single unwrapped line, ~200 chars). Confirmed empirically.build job and the electron-mac job). Both run under bash (GitHub Actions default shell is bash -e -o pipefail).The new code (both places) is:
KEY_BODY="$(jq -r .private_key /tmp/apple_key.json)"
if printf '%s' "$KEY_BODY" | grep -q 'BEGIN PRIVATE KEY'; then
printf '%s\n' "$KEY_BODY" > /tmp/apple_key.pem
else
{ echo "-----BEGIN PRIVATE KEY-----"; printf '%s' "$KEY_BODY" | tr -d ' \r\n' | fold -w 64; echo; echo "-----END PRIVATE KEY-----"; } > /tmp/apple_key.pem
fi
Rigorously evaluate for REAL correctness bugs. Specifically consider and VERIFY (you may reproduce locally in /tmp/opencode ONLY — generate a throwaway P-256 key with openssl, strip its armor to a bare single-line base64 body, and run the exact snippet against it, then check openssl pkey -in /tmp/opencode/out.pem -noout succeeds):
set -e -o pipefail cause a spurious failure? Note grep -q returns exit 1 when the pattern is absent — but it's the condition of an if, so -e should NOT trip. Confirm. Also consider `printf ... ...I'll perform a rigorous adversarial correctness review. Let me start by reading the diff.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 9150e73d..45168669 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -81,7 +81,14 @@ jobs:
echo "$APPLE_CERT_DATA" | base64 --decode > /tmp/certs.p12
echo 'APPLE_CERT_PATH=/tmp/certs.p12' >> $GITHUB_ENV
echo "$APPLE_API_KEY" | base64 -d > /tmp/apple_key.json
- cat /tmp/apple_key.json | jq .private_key -r > /tmp/apple_key.pem
+ # The stored private_key may be a bare base64 DER body (no PEM armor).
+ # Older notarytool tolerated this; Xcode 26+ notarytool rejects it. Normalize to a proper PEM.
+ KEY_BODY="$(jq -r .private_key /tmp/apple_key.json)"
+ if printf '%s' "$KEY_BODY" | grep -q 'BEGIN PRIVATE KEY'; then
+ printf '%s\n' "$KEY_BODY" > /tmp/apple_key.pem
+ else
+ { echo "-----BEGIN PRIVATE KEY-----"; printf '%s' "$KEY_BODY" | tr -d ' \r\n' | fold -w 64; echo; echo ...
Let me read the full surrounding context of both steps to verify env var names and shell settings.
431 /tmp/opencode/build.yml
<path>/tmp/opencode/build.yml</path>
<type>file</type>
<content>
60: name: Setup pnpm cache
61: with:
62: path: ${{ env.STORE_PATH }}
63: key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
64: restore-keys: |
65: ${{ runner.os }}-pnpm-store-
66:
67: - name: Setup NPM dependencies
68: run: pnpm install
69:
70: - name: Setup Codesign Dependencies
71: env:
72: APPLE_CERT_DATA: ${{ secrets.CSC_LINK }}
73: APPLE_API_KEY: ${{ secrets.APPLE_API_KEY }}
74: run: |
75: curl -L 'https://github.com/indygreg/apple-platform-rs/releases/download/apple-codesign%2F0.29.0/apple-codesign-0.29.0-x86_64-unknown-linux-musl.tar.gz' -o 'rcodesign.tar.gz'
76: echo 'dbe85cedd8ee4217b64e9a0e4c2aef92ab8bcaaa41f20bde99781ff02e600002 rcodesign.tar.gz' | sha256sum -c
77: tar -xzf rcodesign.tar.gz --strip-components=1
78: mv rcodesign ...
<path>/tmp/opencode/build.yml</path> <type>file</type> <content> 370: uses: actions/setup-node@v6 371: with: 372: node-version-file: "package.json" 373: cache: "pnpm" 374: 375: - name: Setup dependencies 376: run: pnpm install 377: 378: - name: Download Electron build 379: uses: actions/download-artifact@v5 380: with: 381: name: electron-build 382: path: packages/spotlight/dist-electron/ 383: 384: - name: Validate Electron build files 385: run: | 386: if [ ! -d "packages/spotlight/dist-electron/main" ] || [ ! ...
Both copies are byte-identical in the snippet. Env var names differ as expected (build job: APPLE_API_KEY_ISSUER_ID; electron-mac job: APPLE_API_ISSUER). Now let me verify the shell default and reproduce empirically.
Let me check whether these steps specify a custom shell (GitHub default for run: on non-Windows is bash --noprofile --norc -eo pipefail {0}).
16: shell: bash 55: shell: bash
<path>/tmp/opencode/build.yml</path>
<type>file</type>
<content>
10: concurrency:
11: group: ${{ github.ref_name || github.sha }}
12: cancel-in-progress: true
13:
14: defaults:
15: run:
16: shell: bash
17:
18: env:
19: TURBO_TELEMETRY_MESSAGE_DISABLED: 1
20:
21: permissions:
22: contents: read
23: packages: write
24: checks: write
25: pull-requests: write # Required for posting/updating PR comments
26:
27: jobs:
28: build:
29: name: Build
(Showing lines 10-29 of 431. Use offset=30 to continue.)
</content>
<path>/tmp/opencode/build.yml</path> <type>file</type> <content> 48: - name: Set up Node 49: uses: actions/setup-node@v6 50: with: 51: node-version-file: "package.json" 52: cache: "pnpm" 53: 54: - name: Get pnpm store directory 55: shell: bash 56: run: | 57: echo "STORE_PATH=$(pnpm store path --silent | tr -d '\n\r')" >> $GITHUB_ENV 58: 59: - uses: actions/cache@v4 (Showing lines 48-59 of 431. Use offset=60 to continue.) </content>
Confirmed: defaults.run.shell: bash. On GitHub Actions, shell: bash maps to bash --noprofile --norc -eo pipefail {0} — so set -e AND pipefail are both active. This is exactly the risky condition to test. Now let me reproduce empirically.
=== real.pem (armored) === -----BEGIN PRIVATE KEY----- MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwT0xEKffxLVOmWJi j/r0tWLGlvwbey10vEVnvdxV51yhRANCAASlqDu6cgMcW9a/5q2uWKweRFPWhLXz seI+Lp0kd+SFFlHIxt6MfaRxbZpICsNqgW7/1DzRsvGznQIoO/9bsh3M -----END PRIVATE KEY----- === bare body length === 184 === jq extract round-trips? === MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0w ...
Now let me run the EXACT snippet under set -eo pipefail with the bare-body input (the else branch), then validate with openssl pkey.
SNIPPET EXIT UNDER set -eo pipefail: 0 === out.pem === -----BEGIN PRIVATE KEY-----$ MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwT0xEKffxLVOmWJi$ j/r0tWLGlvwbey10vEVnvdxV51yhRANCAASlqDu6cgMcW9a/5q2uWKweRFPWhLXz$ seI+Lp0kd+SFFlHIxt6MfaRxbZpICsNqgW7/1DzRsvGznQIoO/9bsh3M$ -----END PRIVATE KEY-----$ === openssl validation === VALID KEY: YES
The else branch works: exit 0 under set -eo pipefail, valid PEM, openssl pkey succeeds. The output byte layout is correct — cat -A confirms each line ends with $ (newline), and there is exactly one END line with no blank line before it.
Now let me verify point 1 more precisely: the grep -q in the if condition when the pattern is ABSENT. I want to confirm set -e/pipefail doesn't trip. My test above already exercised the else branch (pattern absent, exit 0), but let me isolate the pipefail concern explicitly — a failing command in the middle of a pipe used as an if-condition.
--- test A: grep absent as if-condition --- not-matched (else taken, no abort) after: 0 --- test B: prove -e does NOT trip on if-condition even with pipefail --- else-ok still running after if: OK --- test C: what if grep 1 happened OUTSIDE an if (control) ---
The subshell in test C aborted (as designed — proving pipefail would trip outside an if), which is why "SHOULD NOT PRINT" and the trailing echo didn't print. That's the control confirming the mechanism. Tests A and B confirm: set -e/pipefail do NOT abort when the failing pipeline is an if condition — this is guaranteed POSIX/bash behavior. Point 1: safe.
Now let me verify the already-armored passthrough branch (point 5) — multi-line PEM round-trips through $(...) command substitution.
=== stored private_key has embedded newlines? === 6 SNIPPET EXIT: 0 (branch taken: passthrough) === out_armored.pem === -----BEGIN PRIVATE KEY-----$ MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwT0xEKffxLVOmWJi$ j/r0tWLGlvwbey10vEVnvdxV51yhRANCAASlqDu6cgMcW9a/5q2uWKweRFPWhLXz$ seI+Lp0kd+SFFlHIxt6MfaRxbZpICsNqgW7/1DzRsvGznQIoO/9bsh3M$ -----END PRIVATE KEY-----$ === validate === VALID KEY: YES === identical to original real.pem? === IDENTICAL
Passthrough branch preserves embedded newlines and round-trips identically. Point 5: safe. Command substitution strips only the trailing newline; printf '%s\n' restores the single trailing newline the PEM needs.
Now edge cases the review must probe adversarially. Let me test: (a) body length that's an exact multiple of 64 (does the standalone echo produce a spurious blank line?), (b) body with CRLF/spaces (point 4), and (c) the RSA-key case where body >64 chars needs folding — already covered by the 184-char P-256 above. Let me test the exact-multiple-of-64 edge and the whitespace-contaminated input.
=== EDGE 1: body length exact multiple of 64 -> does fold+echo create blank line? === AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA$ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA--- with the trailing standalone echo appended --- AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA$ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA$ (note: fold output for no-trailing-NL input) === EDGE 2: whitespace/CRLF contamination in bare body === MIGHAgEAMB <- after tr -d
Key finding on EDGE 1: fold does NOT emit a trailing newline when its input lacks one. The first line ends with $ (fold's internal wrap newline), but the second/last folded line has NO trailing newline (see how --- runs right up against it). The standalone echo supplies exactly that missing final newline — so the END marker lands on its own line, and there is NO spurious blank line even when the body is an exact multiple of 64. The standalone echo is necessary and correct in all length cases.
EDGE 2 confirms tr -d ' \r\n' cleanly strips spaces and CRLF — base64 has no spaces so this is lossless. Point 4: safe.
Let me confirm point 6 (no secret leak to logs) and verify the exact fold trailing-newline behavior is the same on the macOS runner's BSD fold, since one job runs on Linux and one on macOS.
=== POINT 6: does snippet ever print KEY_BODY to stdout? === grep -q stdout was empty above (rc=1) === fold behavior note (GNU coreutils here) === fold (GNU coreutils) 9.4
Point 6: safe. grep -q produces no output; all printf/echo of key material go into the { ... } > file group redirect, never to stdout. Nothing leaks to CI logs. (Also note: GitHub Actions doesn't run set -x here, and the secret is registered for masking anyway.)
One cross-platform note: the Linux build job (rcodesign) runs on Linux (GNU fold, tested above). The electron-mac job runs on macOS with BSD fold. Let me verify BSD fold shares the "no trailing newline when input lacks one" behavior, since that's the load-bearing assumption for the standalone echo.
BSD fold (from FreeBSD/macOS) does not append a trailing newline to input that lacks one — it only inserts newlines at wrap boundaries, identical to GNU here. But rather than assert, note the crucial point: even if BSD fold did append a trailing newline, the extra standalone echo would then produce one blank line before END — which OpenSSL/notarytool tolerate (blank lines inside PEM armor are ignored). Let me verify that tolerance to bound the worst case.
=== WORST CASE: what if a blank line appears before END (hypothetical BSD fold path)? === Could not read key from worstcase.pem --- with an EXTRA blank line before END --- -----BEGIN PRIVATE KEY-----$ MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwT0xEKffxLVOmWJi$ j/r0tWLGlvwbey10vEVnvdxV51yhRANCAASlqDu6cgMcW9a/5q2uWKweRFPWhLXz$ seI+Lp0kd+SFFlHIxt6MfaRxbZpICsNqgW7/1DzRsvGznQIoO/9bsh3M$ $ -----END PRIVATE KEY-----$ openssl REJECTS blank line: NO
Important adversarial finding: a blank line before END makes OpenSSL reject the key. So the "no spurious blank line" property is load-bearing, and it depends entirely on fold NOT emitting a trailing newline. GNU fold (Linux build job) is confirmed correct. I need to actually verify BSD/macOS fold behavior rather than assume it, because the electron-mac job runs on macOS. Let me check authoritatively.
line count (must be 5 for valid, 6 = blank-line bug): 5 blank lines: 0 --- Also test exact-multiple-of-64 real-ish case with GNU fold in full else branch --- 192-char body -> line count (expect 5, no blank): 5 blank lines: 0
GNU fold: no blank line, in both the normal (184-char) and exact-multiple-of-64 (192-char) cases. Point 2 & 3: safe on Linux.
Now the one genuinely open question my review must flag honestly: the electron-mac job runs on macOS with BSD fold, and I cannot execute BSD fold in this Linux environment. Let me verify BSD fold's documented/actual behavior from an authoritative source rather than guess, since a blank line here would be a BLOCKER.
fold
Skip site
navigation (1)Skip section navigation (2)
Header And Logo
Peripheral Links
.
Donate to FreeBSD
.
Search
Site Navigation
Home
About
Introduction
Features
Advocacy
Marketing
Get FreeBSD
Release Information
Release Engineering
Documentation
FAQ
Handbook
Porter's Handbook
Developer's Handbook
Manual Pages
Documentation Project Primer
All Books and Articles
Community
Mailing Lists
Forums
User Groups
Events
...
The man page confirms BSD fold is a POSIX-conforming line-folding filter. Per POSIX semantics, fold inserts a <newline> when a line reaches width, but it does not append a trailing newline to a final partial line that had none in the input — it emits bytes as read, inserting breaks only at width boundaries. This matches GNU behavior. Both GNU and BSD fold are <newline>-preserving filters, not <newline>-appending ones. So the standalone echo supplies exactly one trailing newline in both cases, and no blank line appears before END on either platform.
That said, I'll flag this as the single point that rests on inference (BSD fold not executed here) rather than direct execution — but it's a well-established, POSIX-mandated behavior, and the empirical macOS notarization outcome will confirm it on the next CI run.
Let me do a final verification: confirm the base job's env var wiring is internally consistent (rcodesign consumes APPLE_API_KEY_P8_PATH) and clean up scratch files.
=== base build job: how is the PEM consumed? (APPLE_API_KEY_P8_PATH) ===
75: curl -L 'https://github.com/indygreg/apple-platform-rs/releases/download/apple-codesign%2F0.29.0/apple-codesign-0.29.0-x86_64-unknown-linux-musl.tar.gz' -o 'rcodesign.tar.gz'
76: echo 'dbe85cedd8ee4217b64e9a0e4c2aef92ab8bcaaa41f20bde99781ff02e600002 rcodesign.tar.gz' | sha256sum -c
77: tar -xzf rcodesign.tar.gz --strip-components=1
78: mv rcodesign /usr/local/bin/rcodesign
79: rm rcodesign.tar.gz
94: echo "APPLE_API_KEY_P8_PATH=/tmp/apple_key.pem" >> $GITHUB_ENV
95: echo 'APPLE_API_KEY_PATH=/tmp/apple_key.json' >> $GITHUB_ENV
=== electron-mac job: how is PEM consumed? (APPLE_API_KEY -> electron-builder notarytool) ===
73: APPLE_API_KEY: ${{ secrets.APPLE_API_KEY }}
83: echo "$APPLE_API_KEY" | base64 -d > /tmp/apple_key.json
93: echo "APPLE_API_KEY_ID=$(cat /tmp/apple_key.json | jq .key_id -r | tr -d '\n\r')" >> ...
Env wiring is internally consistent and unchanged by the edit: the base job exports APPLE_API_KEY_P8_PATH=/tmp/apple_key.pem (line 94) for rcodesign; the electron-mac job exports APPLE_API_KEY=/tmp/apple_key.pem (line 408) plus APPLE_API_ISSUER/APPLE_API_KEY_ID for electron-builder's notarytool. The only lines the diff touched are the PEM-generation lines; the surrounding jq .issuer_id/jq .key_id/$GITHUB_ENV exports are untouched. Now clean up scratch files.
cleaned: wf.bak === confirm no tracked files modified === ?? .opencode/ ?? packages/website/content.config.ts
Scratch files cleaned (the leftover wf.bak is unrelated to this review — not mine to touch). Working tree shows only the known-untracked .opencode/ and content.config.ts; no tracked files modified.
fix/notarize-key-pem-armorBoth edited snippets (build.yml:86-91 and build.yml:400-405) are byte-identical and correct. All seven points verified empirically on GNU/Linux; the one macOS-only path (BSD fold) rests on POSIX-mandated behavior confirmed via the FreeBSD man page.
1. set -e -o pipefail spurious failure — NONE.
Confirmed defaults.run.shell: bash (build.yml:14-16) → GitHub maps this to bash -eo pipefail. Ran the exact snippet under set -eo pipefail: exit 0. A failing pipeline (printf | grep -q, pattern absent → grep exits 1, pipefail rc=1) used as an if condition does NOT trigger -e — this is guaranteed bash/POSIX behavior. Control test (same pipeline outside an if) did abort, proving the mechanism is real and correctly avoided.
2. Armored output parses — NONE. Generated a real P-256 PKCS#8 key, stripped armor to a 184-char bare body, ran the else branch: openssl pkey -in out.pem -noout → VALID KEY: YES. The standalone echo between folded body and END is present and necessary — I verified that a missing newline would merge the last body line into the END marker, and an extra blank line makes OpenSSL reject the key (Could not read key). So the exact one-newline output is load-bearing and correct.
3. fold -w 64 on a no-trailing-newline body — NONE. cat -A confirms fold does not append a trailing newline to a final partial line; the standalone echo supplies exactly the one needed newline. Tested both a normal 184-char body and an exact-multiple-of-64 (192-char) body → 5 lines, 0 blank lines in both. No off-by-one blank-line bug.
**4. ...
<observations> Date: Jul 21, 2026 * 🟡 (11:50) User requested a rigorous, skeptical adversarial CORRECTNESS review (not style/cosmetic) of a single-file CI change in repo /home/byk/Code/getsentry/spotlight, branch fix/notarize-key-pem-armor (PR #1334), base main, file .github/workflows/build.yml. [requested-review] * 🔴 (11:50) User stated GitHub secret APPLE_API_KEY is base64-encoded JSON with f…