feat(cli): precompiled, signed & notarized macOS keychain helper packages#2458
Open
WcaleNieWolny wants to merge 17 commits into
Open
feat(cli): precompiled, signed & notarized macOS keychain helper packages#2458WcaleNieWolny wants to merge 17 commits into
WcaleNieWolny wants to merge 17 commits into
Conversation
…er for Keychain ACL persistence
New cli-helper/ dir: generic 'helper' binary (Swift, subcommand dispatch) with
the keychain-export subcommand moved+renamed from cli/. Per-arch npm packages
@capgo/cli-keychain-darwin-{arm64,x64} (files:[helper]), build/sign-notarize/
prepare-publish scripts (stable --identifier app.capgo.cli.helper), and
SECURITY.md threat model. Anti-footgun gate (--invoked-by handshake + non-TTY)
emits FORBIDDEN_CALLER.
…wiftc path resolveHelperBinary resolves the arch-matching @capgo/cli-keychain-darwin-* optional dep and verifies its Developer ID + Capgo Team ID codesign requirement before exec; hard-errors (no compile fallback). exportP12FromKeychain invokes 'helper keychain-export … --invoked-by capgo-cli'. Removes the runtime swiftc compile path, tmp cache, and the compiling-helper onboarding step. Dev-only CAPGO_KEYCHAIN_HELPER_PATH override is dead-code-eliminated from release builds.
publish_cli_helper.yml: manual workflow_dispatch(version) — build, codesign (hardened runtime), notarize, verify, smoke+gate test, npm publish with provenance, then create the cli-helper-<version> tag + release. publish_cli.yml fails the CLI release if CAPGO_KEYCHAIN_HELPER_PATH leaks into the bundle.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates the CLI from runtime-compiling a Swift keychain helper to using per-arch, Developer-ID-signed, notarized Capgo.app bundles resolved at runtime; adds signing/notarization and publish automation; removes the compile-helper onboarding step; and updates build wiring, tests, README, and security docs. ChangesKeychain Helper Precompilation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36bb529bf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resolveSpecifier = options.resolve ?? createRequire(import.meta.url).resolve | ||
| let packageJsonPath: string | ||
| try { | ||
| packageJsonPath = resolveSpecifier(`${packageName}/package.json`) |
There was a problem hiding this comment.
When a user installs the published @capgo/cli normally, this resolver now requires @capgo/cli-keychain-darwin-*, but I checked cli/package.json and bun.lock and the new helper packages are not declared as optionalDependencies anywhere. Because cli/build.mjs also marks them external, npm will not install either helper with the CLI, so the macOS import-existing flow will always hit the “not installed” error unless users manually install the arch package. Add the two helper packages to the CLI package manifest (and lockfile) so the platform-matching optional package is present at runtime.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish_cli_helper.yml:
- Around line 79-93: Tests hardcode the helper binary name
(./cli-helper/dist/helper-arm64) which breaks on x86 runners; change the runtime
invocation to select the binary using the machine architecture (uname -m) and
use that to build the path (e.g., ./cli-helper/dist/helper-<arch>) wherever
helper-arm64 is referenced in the workflow steps that run helper invocations so
both the gate-test and keychain-export test call the correct binary for the
runner architecture.
- Around line 25-31: The workflow uses floating action tags and leaves checkout
credentials enabled; update the job steps using actions/checkout,
actions/setup-node, and softprops/action-gh-release to pinned commit SHAs
instead of version tags (replace uses: actions/checkout@v6, uses:
actions/setup-node@v6, and softprops/action-gh-release@v2 with their specific
commit SHA refs) and add persist-credentials: false to the actions/checkout step
(and optionally set fetch-depth: 1) so the workflow does not retain privileged
tokens during signing/notarization and publish steps.
In @.github/workflows/publish_cli.yml:
- Around line 39-43: The release check can be silently bypassed if the bundle
file cli/dist/index.js is missing; update the run block so it first verifies the
file exists and fails explicitly if not, then performs the grep for
CAPGO_KEYCHAIN_HELPER_PATH. Concretely, add an existence test (e.g. [ -f
"cli/dist/index.js" ] || { echo "::error::cli/dist/index.js missing"; exit 1; })
before the grep command that searches for CAPGO_KEYCHAIN_HELPER_PATH to ensure
the gate runs reliably.
In `@cli-helper/npm/darwin-arm64/package.json`:
- Line 10: Update the package.json "license" field to use the SPDX identifier;
replace the current value "Apache 2.0" with "Apache-2.0" so the "license" key in
the package.json contains the valid SPDX string.
In `@cli-helper/npm/darwin-x64/package.json`:
- Line 10: The package.json's "license" field currently uses an invalid SPDX
string; update the "license" property in package.json to the canonical SPDX
identifier "Apache-2.0" (replace "Apache 2.0" with "Apache-2.0") so tooling and
metadata correctly recognize the license.
In `@cli-helper/scripts/sign-and-notarize.sh`:
- Around line 15-18: The script currently trusts CAPGO_APPLE_TEAM_ID (used in
REQUIREMENT) but runtime verification in macos-signing.ts only accepts
UVTJ336J2D; add an explicit guard that validates CAPGO_APPLE_TEAM_ID equals the
expected team ID (e.g., EXPECTED_TEAM_ID='UVTJ336J2D') or read that
single-source-of-truth constant used at build/release time, and if it does not
match, fail fast with a clear error and non-zero exit (before any signing
steps). Update the script to reference CAPGO_APPLE_TEAM_ID and REQUIREMENT so
the check runs prior to using REQUIREMENT.
In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md`:
- Around line 347-350: Several fenced code blocks (e.g., the blocks containing
"dist/helper-arm64: Mach-O 64-bit executable arm64" and "dist/helper-x64:
Mach-O 64-bit executable x86_64" as well as the similar blocks later) are
missing language identifiers; update each triple-backtick fence around those
output snippets to include an appropriate language tag (for example `text` or
`bash`) so markdownlint MD040 is satisfied — locate the fences that wrap the
"dist/helper-*" lines and the other occurrences (the blocks around lines showing
similar Mach-O output) and change ``` to ```text (or ```bash) accordingly.
- Line 1354: Update the customer-facing CLI example that currently uses the
string "npx `@capgo/cli` build init" to use the scoped latest tag by changing it
to "npx `@capgo/cli`@latest build init"; locate the example text in the same
paragraph that also mentions "iOS → import existing" and replace the command
string so it follows the docs rule for customer-facing Capgo CLI usage.
In `@docs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md`:
- Around line 49-63: The fenced code blocks showing the repository tree
(starting with "cli-helper/" and the later block at 128-134) lack language
identifiers which triggers markdownlint MD040; update both triple-backtick
fences to include a language tag (e.g., ```text or ```bash) so the tree output
is recognized as plain text by the linter—edit the block that contains
"cli-helper/" and the other fenced block referenced to add the language
identifiers.
- Line 396: The prose example contains incorrect platform capitalization: change
the fragment "Capgo.app/Contents/{MacOS/capgo, Info.plist,
Resources/Capgo.icns}" by replacing "MacOS/capgo" with "macOS/capgo" so the
string becomes "Capgo.app/Contents/{macOS/capgo, Info.plist,
Resources/Capgo.icns}" (update the occurrence of `MacOS` in that brace-enclosed
path).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66cf2dc8-2368-4167-b2c0-d4920d9d5fc5
📒 Files selected for processing (20)
.github/workflows/publish_cli.yml.github/workflows/publish_cli_helper.yml.gitignorecli-helper/README.mdcli-helper/SECURITY.mdcli-helper/npm/darwin-arm64/package.jsoncli-helper/npm/darwin-x64/package.jsoncli-helper/scripts/build.shcli-helper/scripts/prepare-publish.mjscli-helper/scripts/sign-and-notarize.shcli-helper/src/helper.swiftcli/build.mjscli/src/build/onboarding/error-categories.tscli/src/build/onboarding/macos-signing.tscli/src/build/onboarding/types.tscli/src/build/onboarding/ui/app.tsxcli/src/build/onboarding/ui/steps/ios-import.tsxcli/test/test-macos-signing.mjsdocs/superpowers/plans/2026-06-06-keychain-helper-precompile.mddocs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md
💤 Files with no reviewable changes (3)
- cli/src/build/onboarding/error-categories.ts
- cli/src/build/onboarding/ui/steps/ios-import.tsx
- cli/src/build/onboarding/types.ts
| reset everyone's "Always Allow" once. | ||
|
|
||
| **When built, this would add:** a bundle-assembly step in `build.sh` | ||
| (`Capgo.app/Contents/{MacOS/capgo, Info.plist, Resources/Capgo.icns}`), bundle |
Contributor
There was a problem hiding this comment.
Fix platform spelling: use macOS
Please replace MacOS/capgo with macOS/capgo in the prose example text.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~396-~396: The operating system from Apple is written “macOS”.
Context: ...* a bundle-assembly step in build.sh (`Capgo.app/Contents/{MacOS/capgo, Info.plist, Resources/Capgo.icns...
(MAC_OS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md` at
line 396, The prose example contains incorrect platform capitalization: change
the fragment "Capgo.app/Contents/{MacOS/capgo, Info.plist,
Resources/Capgo.icns}" by replacing "MacOS/capgo" with "macOS/capgo" so the
string becomes "Capgo.app/Contents/{macOS/capgo, Info.plist,
Resources/Capgo.icns}" (update the occurrence of `MacOS` in that brace-enclosed
path).
Source: Linters/SAST tools
The runtime swiftc compile path was deleted in this PR, but tests/onboarding-error-categories.unit.test.ts still asserted the now-gone `import-compiling-helper` step maps to `keychain_helper_compile_failed`, failing the "Run tests" suite. Remove the obsolete case and the orphaned `keychain_helper_compile_failed` category (no longer produced anywhere). Also address valid CodeRabbit review comments: - publish_cli_helper.yml: select helper binary by `uname -m` in the smoke + gate tests instead of hardcoding helper-arm64 (portable to x64 runners). - publish_cli.yml: fail explicitly if cli/dist/index.js is missing before the env-override grep gate, so the release check can't be silently bypassed. - cli-helper npm manifests: use valid SPDX identifier `Apache-2.0`. - sign-and-notarize.sh: fail fast if CAPGO_APPLE_TEAM_ID drifts from the team ID the CLI runtime verifier enforces (single source of truth).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Wrap the keychain helper in a signed, notarized, stapled Capgo.app (LSUIElement = no Dock icon) so the macOS Keychain export prompts show the Capgo name + icon, and CFBundleIdentifier app.capgo.cli.helper keys the 'Always Allow' grant across releases. Per-arch packages ship Capgo.app (files:[Capgo.app]); the CLI verifies the bundle signature and execs Contents/MacOS/capgo. build.sh assembles the bundle (Info.plist + Capgo.icns from the iOS app icon) and bakes the version before signing; sign-and-notarize staples the ticket. Resolver + tests + CI updated for the bundle layout.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli-helper/scripts/prepare-publish.mjs (1)
15-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten semver validation to match npm-compatible prerelease syntax.
The current pattern allows invalid prerelease tokens (for example underscores), so malformed versions can pass this gate and fail later during publish.
Suggested patch
-if (!version || !/^\d+\.\d+\.\d+(-[\w.]+)?$/.test(version)) { +if (!version || !/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(version)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli-helper/scripts/prepare-publish.mjs` around lines 15 - 17, The regex guarding the version check around the variable "version" in prepare-publish.mjs is too permissive for prerelease tokens (it uses \w and allows underscores); replace that regex in the if (!version || !/.../.test(version)) condition with an npm-compatible semver pattern that only permits numeric MAJOR.MINOR.PATCH and prerelease/build identifiers made of [0-9A-Za-z-] separated by dots (e.g. use a pattern equivalent to: digits.digits.digits optionally followed by -prerelease (dot-separated [0-9A-Za-z-] identifiers) and optionally +build (dot-separated [0-9A-Za-z-] identifiers)); update the test to use that stricter regex so malformed prerelease tokens are rejected early.
♻️ Duplicate comments (1)
docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md (1)
1362-1362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
@latestin the customer-facing CLI example.This still shows
npx@capgo/clibuild init; per docs rules it should benpx@capgo/cli@latest build init.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md` at line 1362, The CLI example uses the package without a version tag ("npx `@capgo/cli` build init"); update the customer-facing command to pin to latest by changing occurrences of the command string to "npx `@capgo/cli`@latest build init" (e.g., in the sentence starting with "Then run the real onboarding export flow once") so docs follow the required `@latest` convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli-helper/scripts/sign-and-notarize.sh`:
- Line 51: The spctl check in sign-and-notarize.sh currently appends "|| true"
which suppresses Gatekeeper verification failures; remove the "|| true" so that
the spctl command (spctl -a -t exec -vv "$app" 2>&1 | head -3) will return its
non-zero exit status and cause the script to fail under set -euo pipefail, or
alternatively capture the output into a variable, print it, and explicitly exit
with the spctl exit code; update the line invoking spctl in sign-and-notarize.sh
accordingly so Gatekeeper failures are not masked.
In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md`:
- Around line 13-19: The plan still references bare helper binaries (strings
like "helper", "dist/helper-*", resolver paths to "/helper" and `files:
["helper"]`) even though the amendment changed the artifact to a packaged
Capgo.app (inner exec at Contents/MacOS/capgo) produced by build.sh which now
bakes Info.plist and signs/staples; update every concrete step that mentions the
old bare-binary paths to point to the app bundle or its inner executable (e.g.,
replace "helper" and "dist/helper-*" with "Capgo.app" or
"Capgo.app/Contents/MacOS/capgo", update any `files: ["helper"]` to `files:
["Capgo.app"]`, and change resolvers that return `/helper` to return the bundle
path), and ensure build.sh and signing/stapling steps referenced in
cli-helper/README.md are used consistently wherever the helper is referenced.
In `@docs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md`:
- Around line 28-39: The doc contains conflicting states about the helper
packaging: the AMENDMENT paragraph says the helper now ships as a hidden
Capgo.app bundle (inner exec at Capgo.app/Contents/MacOS/capgo, package files =
["Capgo.app"], CI signs+notarizes+staples) but later sections still describe a
bare helper binary as current. Update the spec so all sections consistently
describe the bundle model: replace references to the "bare `helper`" with
"bundle's inner executable at Capgo.app/Contents/MacOS/capgo", update
packaging/CI/resolver guidance to use package `files = ["Capgo.app"]`, ensure
Keychain/CFBundleIdentifier notes reference app.capgo.cli.helper, and remove or
mark obsolete notes about a bare helper or future `.app` migration; also add a
single pointer to cli-helper/README.md for implementation details.
---
Outside diff comments:
In `@cli-helper/scripts/prepare-publish.mjs`:
- Around line 15-17: The regex guarding the version check around the variable
"version" in prepare-publish.mjs is too permissive for prerelease tokens (it
uses \w and allows underscores); replace that regex in the if (!version ||
!/.../.test(version)) condition with an npm-compatible semver pattern that only
permits numeric MAJOR.MINOR.PATCH and prerelease/build identifiers made of
[0-9A-Za-z-] separated by dots (e.g. use a pattern equivalent to:
digits.digits.digits optionally followed by -prerelease (dot-separated
[0-9A-Za-z-] identifiers) and optionally +build (dot-separated [0-9A-Za-z-]
identifiers)); update the test to use that stricter regex so malformed
prerelease tokens are rejected early.
---
Duplicate comments:
In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md`:
- Line 1362: The CLI example uses the package without a version tag ("npx
`@capgo/cli` build init"); update the customer-facing command to pin to latest by
changing occurrences of the command string to "npx `@capgo/cli`@latest build init"
(e.g., in the sentence starting with "Then run the real onboarding export flow
once") so docs follow the required `@latest` convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e22f1c5-a56e-4acf-94f2-68843486388b
📒 Files selected for processing (14)
.github/workflows/publish_cli_helper.yml.gitignorecli-helper/README.mdcli-helper/assets/Capgo.icnscli-helper/assets/Info.plist.templatecli-helper/npm/darwin-arm64/package.jsoncli-helper/npm/darwin-x64/package.jsoncli-helper/scripts/build.shcli-helper/scripts/prepare-publish.mjscli-helper/scripts/sign-and-notarize.shcli/src/build/onboarding/macos-signing.tscli/test/test-macos-signing.mjsdocs/superpowers/plans/2026-06-06-keychain-helper-precompile.mddocs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md
Comment on lines
+13
to
+19
| > **AMENDMENT (implemented):** the helper now ships inside a hidden `Capgo.app` | ||
| > bundle (`LSUIElement`), not a bare `helper` binary. Tasks below that say | ||
| > `files: ["helper"]`, copy/resolve a bare `helper`, or sign a bare binary now | ||
| > operate on `Capgo.app` (inner exec `Contents/MacOS/capgo`); `build.sh` takes a | ||
| > version arg and bakes Info.plist; signing also staples. The branded "Capgo" | ||
| > Keychain prompt + icon is the payoff. See `cli-helper/README.md` and the spec | ||
| > amendment for the current shape. |
Contributor
There was a problem hiding this comment.
Amendment is not fully propagated to executable steps.
This amendment says “operate on Capgo.app”, but the plan still includes many concrete old-path commands (helper, dist/helper-*, files: ["helper"], resolver to /helper). Please update those concrete steps to bundle paths so the runbook is executable without interpretation gaps.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: The operating system from Apple is written “macOS”.
Context: ...ow > operate on Capgo.app (inner exec Contents/MacOS/capgo); build.sh takes a > version a...
(MAC_OS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md` around lines
13 - 19, The plan still references bare helper binaries (strings like "helper",
"dist/helper-*", resolver paths to "/helper" and `files: ["helper"]`) even
though the amendment changed the artifact to a packaged Capgo.app (inner exec at
Contents/MacOS/capgo) produced by build.sh which now bakes Info.plist and
signs/staples; update every concrete step that mentions the old bare-binary
paths to point to the app bundle or its inner executable (e.g., replace "helper"
and "dist/helper-*" with "Capgo.app" or "Capgo.app/Contents/MacOS/capgo", update
any `files: ["helper"]` to `files: ["Capgo.app"]`, and change resolvers that
return `/helper` to return the bundle path), and ensure build.sh and
signing/stapling steps referenced in cli-helper/README.md are used consistently
wherever the helper is referenced.
- publish_cli_helper.yml: persist-credentials: false on checkout (privileged signing/publish workflow) - sign-and-notarize.sh: guard CAPGO_APPLE_TEAM_ID == the CLI verifier's UVTJ336J2D (catches secret drift at release, not user runtime); stop masking Gatekeeper (spctl) failures with || true - docs: @latest in customer-facing CLI example; language tags on fenced blocks; mark the .app bundle as implemented in the Future section (Contents/MacOS/ path kept — it's the real bundle dir, not a macOS typo)
The tag + GitHub release authenticate via PERSONAL_ACCESS_TOKEN, npm publish via NPM_TOKEN, and provenance via id-token. The automatic GITHUB_TOKEN is only used by checkout, which needs read. Tighten contents: write → read (least privilege).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish_cli_helper.yml (1)
103-107: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert non-zero exit in the gate test too.
Line 103-107 checks the JSON envelope but not the exit status. Add the same non-zero assertion used in the smoke test so
FORBIDDEN_CALLERexit-code regressions are caught.Suggested patch
- name: Gate test — keychain-export without handshake is FORBIDDEN_CALLER run: | arch="$(uname -m)" case "$arch" in arm64) helper="./cli-helper/dist/arm64/Capgo.app/Contents/MacOS/capgo" ;; x86_64) helper="./cli-helper/dist/x64/Capgo.app/Contents/MacOS/capgo" ;; *) echo "::error::unsupported runner arch: $arch"; exit 1 ;; esac set +e out=$("$helper" keychain-export --sha1 "$(printf 'a%.0s' {1..40})" --output /tmp/x.p12 --passphrase p | cat) + code=$? set -e + [ "$code" -ne 0 ] || { echo "::error::expected non-zero exit"; exit 1; } echo "$out" | jq -e '.ok == false and .errorCode == "FORBIDDEN_CALLER"' > /dev/null \ || { echo "::error::gate did not reject missing handshake: $out"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish_cli_helper.yml around lines 103 - 107, The gate test captures output in out from "$helper" keychain-export but doesn't assert the command exited non-zero; update the block that sets out (the subshell calling keychain-export) to also assert the command's exit code is non-zero using the captured PIPESTATUS (e.g. test "${PIPESTATUS[0]}" -ne 0) and fail with an ::error:: message if it was zero, keeping the existing jq check for '.ok == false and .errorCode == "FORBIDDEN_CALLER"'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md`:
- Around line 354-358: The fenced code block that contains the lines starting
with "dist/helper-arm64: Mach-O 64-bit executable arm64" is missing blank lines
around the fence and triggers markdownlint MD031; add an empty line immediately
before the opening ``` and an empty line immediately after the closing ``` so
the block is separated by blank lines from surrounding text, ensuring the fenced
block around the "dist/helper-arm64" / "dist/helper-x64" output meets MD031.
---
Outside diff comments:
In @.github/workflows/publish_cli_helper.yml:
- Around line 103-107: The gate test captures output in out from "$helper"
keychain-export but doesn't assert the command exited non-zero; update the block
that sets out (the subshell calling keychain-export) to also assert the
command's exit code is non-zero using the captured PIPESTATUS (e.g. test
"${PIPESTATUS[0]}" -ne 0) and fail with an ::error:: message if it was zero,
keeping the existing jq check for '.ok == false and .errorCode ==
"FORBIDDEN_CALLER"'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fb381c6-05c0-4426-8118-de629e4f3a46
📒 Files selected for processing (4)
.github/workflows/publish_cli_helper.ymlcli-helper/scripts/sign-and-notarize.shdocs/superpowers/plans/2026-06-06-keychain-helper-precompile.mddocs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md
Comment on lines
+354
to
+358
| Expected output ends with: | ||
| ```text | ||
| dist/helper-arm64: Mach-O 64-bit executable arm64 | ||
| dist/helper-x64: Mach-O 64-bit executable x86_64 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add blank lines around this fenced block (MD031).
This block still trips markdownlint’s blanks-around-fences.
Suggested patch
Run: `bash cli-helper/scripts/build.sh`
Expected output ends with:
+
```text
dist/helper-arm64: Mach-O 64-bit executable arm64
dist/helper-x64: Mach-O 64-bit executable x86_64</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 355-355: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md around lines
354 - 358, The fenced code block that contains the lines starting with
"dist/helper-arm64: Mach-O 64-bit executable arm64" is missing blank lines
around the fence and triggers markdownlint MD031; add an empty line immediately
before the opening and an empty line immediately after the closing so
the block is separated by blank lines from surrounding text, ensuring the fenced
block around the "dist/helper-arm64" / "dist/helper-x64" output meets MD031.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- cr-comment:v1:e5b92efa4279e29f85cc3384 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/publish_cli_helper.yml (2)
83-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the helper’s documented exit codes in these release gates.
These steps currently validate the JSON envelope, but not the exit-code contract.
cli-helper/src/helper.swiftassignsINVALID_ARGSto exit2andFORBIDDEN_CALLERto exit5; the| catin the gate test also hides the helper’s real status. A regression in exit handling would still pass this workflow.Suggested fix
- name: Smoke test signed binary run: | arch="$(uname -m)" case "$arch" in arm64) helper="./cli-helper/dist/arm64/Capgo.app/Contents/MacOS/capgo" ;; x86_64) helper="./cli-helper/dist/x64/Capgo.app/Contents/MacOS/capgo" ;; *) echo "::error::unsupported runner arch: $arch"; exit 1 ;; esac set +e out=$("$helper") code=$? set -e - [ "$code" -ne 0 ] || { echo "::error::expected non-zero exit"; exit 1; } + [ "$code" -eq 2 ] || { echo "::error::expected INVALID_ARGS exit code 2, got $code"; exit 1; } echo "$out" | jq -e '.ok == false and .errorCode == "INVALID_ARGS"' > /dev/null \ || { echo "::error::unexpected helper output: $out"; exit 1; } - name: Gate test — keychain-export without handshake is FORBIDDEN_CALLER run: | arch="$(uname -m)" case "$arch" in arm64) helper="./cli-helper/dist/arm64/Capgo.app/Contents/MacOS/capgo" ;; x86_64) helper="./cli-helper/dist/x64/Capgo.app/Contents/MacOS/capgo" ;; *) echo "::error::unsupported runner arch: $arch"; exit 1 ;; esac set +e - out=$("$helper" keychain-export --sha1 "$(printf 'a%.0s' {1..40})" --output /tmp/x.p12 --passphrase p | cat) + out=$("$helper" keychain-export --sha1 "$(printf 'a%.0s' {1..40})" --output /tmp/x.p12 --passphrase p) + code=$? set -e + [ "$code" -eq 5 ] || { echo "::error::expected FORBIDDEN_CALLER exit code 5, got $code"; exit 1; } echo "$out" | jq -e '.ok == false and .errorCode == "FORBIDDEN_CALLER"' > /dev/null \ || { echo "::error::gate did not reject missing handshake: $out"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish_cli_helper.yml around lines 83 - 109, The workflow steps "Smoke test signed binary" and "Gate test — keychain-export without handshake is FORBIDDEN_CALLER" currently only validate JSON output and hide the helper's true exit code (the latter uses a trailing "| cat"); update each step to capture and assert the helper process exit status explicitly: run the helper command without piping to cat, save its exit code (e.g., code=$?), and assert code equals the documented values from cli-helper/src/helper.swift (INVALID_ARGS -> exit 2 for the smoke test; FORBIDDEN_CALLER -> exit 5 for the gate test) in addition to validating the JSON envelope; ensure you reference the same helper invocation logic (the helper path selection logic and variable name like helper) so the change is localized to those two steps.
3-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable auto-cancel for this release workflow.
npm publishis irreversible. Withcancel-in-progress: trueand a concurrency group keyed only bygithub.ref, a second manual dispatch from the same branch can kill the first run after one architecture is published but before the other, leaving a half-released version.Suggested fix
concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + cancel-in-progress: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish_cli_helper.yml around lines 3 - 5, The workflow's concurrency block uses "group: ${{ github.workflow }}-${{ github.ref }}" together with "cancel-in-progress: true", which can abort an in-progress release (leaving partially published artifacts); change the concurrency configuration to disable auto-cancel by setting "cancel-in-progress: false" (or remove the "cancel-in-progress" key entirely) so that concurrent manual dispatches for the same branch do not cancel earlier runs — update the "concurrency" block that contains group: ${{ github.workflow }}-${{ github.ref }} accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/publish_cli_helper.yml:
- Around line 83-109: The workflow steps "Smoke test signed binary" and "Gate
test — keychain-export without handshake is FORBIDDEN_CALLER" currently only
validate JSON output and hide the helper's true exit code (the latter uses a
trailing "| cat"); update each step to capture and assert the helper process
exit status explicitly: run the helper command without piping to cat, save its
exit code (e.g., code=$?), and assert code equals the documented values from
cli-helper/src/helper.swift (INVALID_ARGS -> exit 2 for the smoke test;
FORBIDDEN_CALLER -> exit 5 for the gate test) in addition to validating the JSON
envelope; ensure you reference the same helper invocation logic (the helper path
selection logic and variable name like helper) so the change is localized to
those two steps.
- Around line 3-5: The workflow's concurrency block uses "group: ${{
github.workflow }}-${{ github.ref }}" together with "cancel-in-progress: true",
which can abort an in-progress release (leaving partially published artifacts);
change the concurrency configuration to disable auto-cancel by setting
"cancel-in-progress: false" (or remove the "cancel-in-progress" key entirely) so
that concurrent manual dispatches for the same branch do not cancel earlier runs
— update the "concurrency" block that contains group: ${{ github.workflow }}-${{
github.ref }} accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7851239c-b7e0-46d2-ba3b-55e2c80bc3f6
📒 Files selected for processing (1)
.github/workflows/publish_cli_helper.yml
workflow_dispatch can't run from a non-default branch, so add a push:tags trigger — pushing a cli-helper-X.Y.Z tag runs the pipeline from that commit, letting the whole thing be tested on a PR branch without merging. Version comes from the dispatch input or the tag name. The tag+release are created with the built-in GITHUB_TOKEN (loop-safe: GITHUB_TOKEN tags don't re-trigger push:tags), so contents: write returns and the PAT is dropped.
…atest') While the packages are new/unproven, publish both with --tag rc so they never become the default 'latest' install. Does not affect @capgo/cli: its optionalDependencies (^1.0.0) resolve against published versions, not dist-tags.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ships the macOS keychain-export helper as precompiled, Developer-ID-signed, notarized per-arch npm packages instead of compiling Swift on the user's machine at runtime. The CLI resolves the arch-matching package, verifies its code signature (Developer ID + Capgo Team ID) before executing it, and hard-errors with install guidance if anything is missing — the runtime
swiftccompile path is deleted.Drops the Xcode Command Line Tools requirement, removes the first-run compile delay and the "compiling helper" onboarding step, and (via a stable signing identifier) lets macOS Keychain "Always Allow" persist across CLI upgrades.
Spec + plan:
docs/superpowers/specs/2026-06-06-keychain-helper-precompile-design.md,docs/superpowers/plans/2026-06-06-keychain-helper-precompile.md.What's in this PR
New
cli-helper/package (feat(cli-helper))src/helper.swift— generichelperbinary with subcommand dispatch (helper keychain-export …); moved+renamed fromcli/. Future helpers are new subcommands of the same signed binary.@capgo/cli-keychain-darwin-{arm64,x64}(os/cpufiltered,files: ["helper"]).scripts/build.sh(arm64→macOS 11, x64→macOS 10.15),prepare-publish.mjs,sign-and-notarize.sh(pins stable--identifier app.capgo.cli.helper).SECURITY.mdthreat model +README.md.CLI integration (
feat(cli))resolveHelperBinaryresolves the optional dep,accessSyncX_OK, thenverifyHelperSignature(codesign --verify --strict -R '<Developer ID + team>') before exec. No compile fallback.exportP12FromKeychaininvokeshelper keychain-export … --invoked-by capgo-cli.compileSwiftHelper/ensureSwiftHelper/tmp cache/precompileSwiftHelper/isHelperCached+ the compiling-helper onboarding step.CAPGO_KEYCHAIN_HELPER_PATHoverride is dead-code-eliminated from release builds (__CAPGO_ALLOW_HELPER_ENV_OVERRIDE__define).CI (
ci)publish_cli_helper.yml:workflow_dispatch(version)— build → codesign (hardened runtime) → notarize → verify → smoke + gate test → npm publish--provenance→ createcli-helper-<version>tag + release. (Deliberate button, not auto-tag — releases are rare and notarization is a flaky external dependency.)publish_cli.yml: fails the CLI release ifCAPGO_KEYCHAIN_HELPER_PATHleaks into the bundle.Security posture
The macOS Keychain ACL prompt is the boundary (OS-enforced against our binary's signature). The
--invoked-by capgo-cli+ non-TTY anti-footgun gate (FORBIDDEN_CALLER) is explicitly not a security boundary —SECURITY.mddocuments why caller auth is infeasible (node isn't Capgo-signed) and that invoking the helper grants a local attacker nothing they can't already do via Apple's own APIs. This pre-empts the predictable low-effort bug report.Verification (all green locally)
bun run build(release) ✅ ·dist/index.jshas 0CAPGO_KEYCHAIN_HELPER_PATH✅ · dev build retains it ✅bun run typecheck✅ ·bun run lint✅bun test/test-macos-signing.mjs→ 47 pass (resolution order, signature verify pass/fail, missing-package, unsupported-arch, env-override gated) ✅cli-helper/scripts/build.sh→ both Mach-O arches (minos 11.0 / 10.15) ✅FORBIDDEN_CALLERexit 5; no subcommand →INVALID_ARGSexit 2 ✅test-onboarding-progress,test-build-complete-exit,test-min-size-gate) ✅Implemented via a multi-agent workflow with a 4-lens adversarial review (security/correctness, build-integrity, package-correctness, spec-conformance) — all pass, only cosmetic findings (fixed).
CAPGO_APPLE_TEAM_ID/HELPER_IDENTIFIER— currentlyUVTJ336J2D(from existing test fixtures). Update if the Developer ID cert'ssubject.OUdiffers.DEVELOPER_ID_CERT_BASE64,DEVELOPER_ID_CERT_PASSWORD,APPLE_TEAM_ID; do one local sign+notarize dry run.gh workflow run publish_cli_helper.yml -f version=1.0.0to publish the packages.cli/package.jsonoptionalDependencies(+bun install). Must merge only after 1.0.0 is on npm, elsebun install --frozen-lockfilebreaks CI. Until then the CLI's export flow errors with actionable install guidance (no crash).🤖 Implemented with multi-agent orchestration + adversarial review.
Summary by CodeRabbit