fix(cmux-sync): stop SecurityAgent prompt storm on go install builds

[henkyermontero]

Closes #106

What this fixes

Three compounding bugs caused agentcookie cmux-sync enable to trigger 10+ SecurityAgent Keychain prompts in rapid succession on go install builds (ad-hoc signed, no team ID). See #106 for the full repro and root-cause analysis.

Changes

• internal/chrome/keychain.go (U1): SafeStoragePasswordFor now checks BinaryTeamID(os.Executable()) before the CGO path. When it returns "" (ad-hoc/unsigned binary), it skips safeStoragePasswordViaKeybaseFor (SecItemCopyMatching) entirely and goes straight to the security CLI. Unsigned binaries always prompt on the CGO path; skipping it avoids both the initial SecurityAgent dialog and the goroutine leaked after the 3s timeout. Signed binaries are unchanged. Adds IsKeychainAccessError to classify Keychain-grant failures vs. operational failures. The CGO call is now behind a safeStorageViaKeybaseRunner seam for testability.

• internal/cli/cmux_sync.go (U2): In --watch mode, when SafeStoragePasswordFor returns a Keychain access error, the agent prints the remediation and exits 0 instead of returning the error — so launchd's KeepAlive does not restart it into a prompt loop. Operational errors (cmux down, Chrome DB locked, etc.) still exit non-zero so launchd retries them as before. --once mode is unchanged (still returns the error).

• internal/cli/cmux_sync_enable.go (U3): enableCmuxLoop runs a Keychain pre-flight before installing the persistent launch agent and aborts with wizard set-keychain-access remediation if it fails — so the restart loop never starts. The pre-flight runs after the cmux-not-found no-op, so an absent cmux never triggers a prompt. With U1 in place, on a granted machine the pre-flight and the agent's own read both go through the security CLI with no prompt; on an ungranted machine the pre-flight aborts and the agent is never installed.

Tests

• internal/chrome/keychain_test.go: CGO path is skipped for unsigned binaries, used for signed binaries, and a BinaryTeamID error is treated as unsigned; IsKeychainAccessError boundary cases.
• internal/cli/cmux_sync_test.go: --watch exits 0 on Keychain access error, returns error on non-Keychain errors, and --once returns the error (no exit-0).
• internal/cli/cmux_sync_enable_test.go: enable aborts (no mode change, no agent install) when pre-flight fails, proceeds when it passes, and skips the pre-flight entirely when cmux is absent.

go test ./... passes.

Notes / scope

• BinaryTeamID shells out to codesign; it runs once per process in these call paths, so the added latency is negligible (not cached). Worth a sync.Once only if SafeStoragePasswordFor ever becomes hot.
• This targets the ad-hoc-signed (go install) case, which is the storm in cmux-sync enable triggers 10+ SecurityAgent Keychain prompts in rapid succession (go install builds) #106. A Developer-ID-signed binary that loses its grant (e.g., after a macOS upgrade) would still take the CGO path; the underlying goroutine-abandonment in safeStoragePasswordViaKeybaseFor is out of scope here and could be hardened separately.

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

Three compounding bugs caused repeated SecurityAgent Keychain prompts on go install builds; this PR addresses all three. On unsigned/ad-hoc binaries the CGO (SecItemCopyMatching) path is now skipped entirely, --watch mode exits 0 (not non-zero) on a missing-grant error so launchd's KeepAlive doesn't restart into a loop, and a Keychain pre-flight in cmux-sync enable aborts before the persistent launch agent is ever installed.

• keychain.go: BinaryTeamID gates the CGO path; unsigned binaries go straight to the security CLI. IsKeychainAccessError classifies missing-grant failures and explicitly excludes transient -25308 locked-keychain errors (which should still exit non-zero so launchd retries).
• cmux_sync.go: --watch calls cmuxExitFunc(0) on IsKeychainAccessError; non-Keychain errors and --once mode are unchanged.
• cmux_sync_enable.go: Keychain pre-flight is placed after the ErrNotFound early-return, so a never-launched cmux still produces the "launch cmux once" hint rather than a spurious Keychain message.

Confidence Score: 4/5

Safe to merge for the core prompt-storm fix; one ordering issue in the enable flow leaves cmux.json in allowAll on a failed pre-flight.

The fix for unsigned binaries, the --watch exit-0 path, and IsKeychainAccessError are all correctly implemented and well-tested. The one real issue is that cmuxSyncSetMode writes allowAll to cmux.json before the Keychain pre-flight can abort, leaving a partially-mutated config when the pre-flight fails — no agent is installed and the user isn't told to restart cmux, but the file change is persistent and the disable command doesn't revert it.

internal/cli/cmux_sync_enable.go — the ordering of cmuxSyncSetMode vs the Keychain pre-flight.

Important Files Changed

Filename	Overview
internal/chrome/keychain.go	Adds safeStorageViaKeybaseRunner seam and gates the CGO path behind a BinaryTeamID check; unsigned/ad-hoc binaries skip straight to the security CLI. Adds IsKeychainAccessError that correctly excludes transient -25308 locked-keychain errors.
internal/cli/cmux_sync.go	Adds cmuxSyncPasswordFor and cmuxExitFunc seams; --watch mode exits 0 on IsKeychainAccessError so launchd's KeepAlive does not restart into a prompt loop.
internal/cli/cmux_sync_enable.go	Adds cmuxSyncKeychainCheck seam and Keychain pre-flight before agent install; correctly placed after the ErrNotFound early-return so a never-launched cmux still shows the "launch cmux once" hint. Pre-flight runs after cmuxSyncSetMode has already written allowAll to cmux.json.
internal/chrome/keychain_test.go	Adds table-driven tests for IsKeychainAccessError (including explicit -25308 exclusion) and three behavioural tests for the unsigned/signed/error-treated-as-unsigned paths of SafeStoragePasswordFor.
internal/cli/cmux_sync_enable_test.go	Extends enable tests with four new cases: pre-flight absent when cmux.json is missing, aborts (no agent install) on failure, proceeds on success, and no pre-flight when cmux is absent.
internal/cli/cmux_sync_test.go	Adds three tests for the new --watch exit-0 path: exits 0 on Keychain access error in watch mode, returns error on non-Keychain errors in watch mode, returns error in --once mode.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[SafeStoragePasswordFor called] --> B{os.Executable + BinaryTeamID}
    B -- teamID != empty signed binary --> C[safeStorageViaKeybaseRunner CGO path]
    C -- success --> D[return password]
    C -- error --> E[fall through to CLI]
    B -- teamID == empty unsigned/ad-hoc --> E
    E[security find-generic-password CLI] --> F{outcome}
    F -- timeout DeadlineExceeded --> G[return: not yet in Safe Storage partition]
    F -- non-zero exit --> H[return: did you grant access?]
    F -- success --> I[return password]

    subgraph watch[--watch mode runCmuxSync]
    J[cmuxSyncPasswordFor] --> K{IsKeychainAccessError?}
    K -- yes --> L[cmuxExitFunc 0 - no launchd restart]
    K -- no --> M[return err - launchd retries]
    end

    subgraph enable[enableCmuxLoop]
    N[cmuxSyncSetMode allowAll] --> O{ErrNotFound?}
    O -- yes --> P[no-op: launch cmux once]
    O -- no --> Q[Keychain pre-flight cmuxSyncKeychainCheck]
    Q -- fail --> R[abort: run wizard set-keychain-access]
    Q -- pass --> S[cmuxSyncInstallAgent --watch agent]
    end

    G --> K
    H --> K

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[SafeStoragePasswordFor called] --> B{os.Executable + BinaryTeamID}
    B -- teamID != empty signed binary --> C[safeStorageViaKeybaseRunner CGO path]
    C -- success --> D[return password]
    C -- error --> E[fall through to CLI]
    B -- teamID == empty unsigned/ad-hoc --> E
    E[security find-generic-password CLI] --> F{outcome}
    F -- timeout DeadlineExceeded --> G[return: not yet in Safe Storage partition]
    F -- non-zero exit --> H[return: did you grant access?]
    F -- success --> I[return password]

    subgraph watch[--watch mode runCmuxSync]
    J[cmuxSyncPasswordFor] --> K{IsKeychainAccessError?}
    K -- yes --> L[cmuxExitFunc 0 - no launchd restart]
    K -- no --> M[return err - launchd retries]
    end

    subgraph enable[enableCmuxLoop]
    N[cmuxSyncSetMode allowAll] --> O{ErrNotFound?}
    O -- yes --> P[no-op: launch cmux once]
    O -- no --> Q[Keychain pre-flight cmuxSyncKeychainCheck]
    Q -- fail --> R[abort: run wizard set-keychain-access]
    Q -- pass --> S[cmuxSyncInstallAgent --watch agent]
    end

    G --> K
    H --> K

Loading

Comments Outside Diff (1)

1. internal/cli/cmux_sync_enable.go, line 106-126 (link)

   socketControlMode set to allowAll before pre-flight can abort

   cmuxSyncSetMode writes allowAll to cmux.json at line 106 before the Keychain pre-flight runs at line 122. When the pre-flight fails and enableCmuxLoop returns an error, the user's cmux.json is left permanently in allowAll mode even though no agent was installed and the user was never told to restart cmux. The mode doesn't take effect until the user restarts cmux, so the practical window of exposure is narrow — but re-running enable after fixing Keychain access will write allowAll a second time (creating another .bak), and running disable only removes the agent, not the allowAll mode (the disable path tells the user to revert it manually). Moving the pre-flight before cmuxSyncSetMode would prevent the partial-state write entirely, though that requires handling the ErrNotFound hint separately.

   

_{Reviews (2): Last reviewed commit: "fix(cmux-sync): address review — locked-..." | Re-trigger Greptile}