fix(keychain): close CGO dialog leak, lock misclassification, and enable side-effect (follow-up to #107)

[mvanhorn]

Follow-up to #107. That PR stopped the cmux-sync enable prompt storm; this closes three residual defects the storm fix left in the same keychain-failure subsystem.

What this fixes

1. CGO dialog/goroutine leak. safeStoragePasswordViaKeybaseFor ran SecItemCopyMatching in a goroutine abandoned on a 3s timeout; on a signed binary that lost its grant the abandoned call held a SecurityAgent dialog open and the security CLI fallback opened a second one. The read now disables keychain user interaction (SecKeychainSetUserInteractionAllowed) around a direct SecItem query, so it returns errSecInteractionNotAllowed immediately instead of prompting — making the call synchronous, with no goroutine to abandon and no orphaned dialog. (kSecUseAuthenticationUIFail alone does not suppress this prompt: the Chrome Safe Storage dialog is a legacy keychain ACL confirmation, not a LocalAuthentication UI.)

2. Lock vs missing-grant misclassification. SafeStoragePasswordFor wrapped every non-timeout security-CLI failure as "did you grant access?" and IsKeychainAccessError string-matched that wrapper, so a transient locked keychain was misread as a permanent missing grant — and --watch exited 0 (no launchd retry), permanently stopping the sync. The raw--25308 exclusion fix(cmux-sync): stop SecurityAgent prompt storm on go install builds #107 added never fired because the live path discards the raw error and re-wraps it. Now there are typed sentinels (ErrKeychainNoGrant / ErrKeychainLocked / ErrKeychainTimeout), classification happens at the source (including a SecKeychainGetStatus lock check before the CLI so a locked keychain can't hang it), and the predicates use errors.Is.

3. socketControlMode side-effect on aborted enable. enableCmuxLoop wrote socketControlMode=allowAll before the Keychain pre-flight could abort, leaving cmux.json mutated on a failed enable. The pre-flight now runs before the write (gated by a stubbable cmux.json existence check), so an aborted enable leaves cmux.json untouched.

Testing

• go build ./..., go vet, and the full suite pass; go test -race ./internal/chrome/ ./internal/cli/ is clean.
• New coverage: no-hang regression guard on the CGO read; locked-keychain short-circuits before the CLI; locked-vs-missing-grant classification survives extra wrap layers (the fix(cmux-sync): stop SecurityAgent prompt storm on go install builds #107 gap); --watch exits non-zero on a locked keychain; aborted enable leaves cmux.json unmutated.
• Both changes were adversarially reviewed (CGO memory safety + classification correctness); the lock-before-CLI check and the interaction-flip mutex came out of that review.

Manual check on a Developer-ID build whose grant was invalidated: cmux-sync --once fails fast with the no-grant remediation and zero SecurityAgent dialogs; with the login keychain locked, cmux-sync --watch exits non-zero (launchd retries) rather than exit 0.

Post-Deploy Monitoring & Validation

• Watch: ~/.agentcookie/logs/ for the cmux-sync agent. Healthy = silent sync; on a missing grant, a single "Keychain not accessible; exiting cleanly" line and no restart loop; on a locked keychain, non-zero exit with launchd retry that recovers after unlock.
• Failure signal / rollback trigger: any recurrence of repeated SecurityAgent prompts, or a --watch agent that stops permanently after a sleep-wake. Mitigation: launchctl unload ~/Library/LaunchAgents/dev.agentcookie.cmux-sync.plist then agentcookie wizard set-keychain-access; revert is a clean git revert of this range (no migrations, no persisted state).
• Validation window: next real cmux-sync enable + one sleep-wake cycle.

[greptile-apps]

Greptile Summary

This follow-up to #107 closes three residual bugs in the keychain-failure subsystem: it eliminates the CGO dialog/goroutine leak by disabling SecKeychainSetUserInteractionAllowed around a synchronous SecItemCopyMatching call, introduces typed sentinel errors (ErrKeychainNoGrant / ErrKeychainLocked / ErrKeychainTimeout) to replace fragile string matching, and moves the Keychain pre-flight ahead of the cmux.json write so a failed enable leaves no orphaned socketControlMode=allowAll.

• CGO path (keychain_keybase.go): the goroutine+timeout pattern is removed entirely; a process-global interaction-disable mutex ensures SecItemCopyMatching is synchronous and non-prompting, with NULL guards for all CF allocations that address the previous review's crash paths.
• Classification (keychain.go): IsKeychainAccessError / IsKeychainLocked are rewritten around errors.Is on the new sentinels, preventing a locked keychain wrapped in "did you grant access?" prose from causing a permanent exit-0 in --watch mode.
• Enable ordering (cmux_sync_enable.go): a stubbable cmuxSyncConfigExists seam lets tests control the no-op branch; the Keychain pre-flight now runs before any mutation, with a TOCTOU fallback retained on the write path.

Confidence Score: 5/5

Safe to merge. All three targeted defects are correctly fixed, error classification paths are clean, and the CGO memory management is sound.

The CGO rewrite replaces the goroutine+timeout with a synchronous, mutex-serialized SecItemCopyMatching that can never open a SecurityAgent dialog. NULL guards for CFStringCreateWithBytes and CFDictionaryCreate close the crash paths flagged in the previous review. The sentinel-based classification correctly distinguishes locked (transient) from missing-grant (permanent) even across multiple wrap layers. The pre-flight ordering change in enableCmuxLoop is minimal and correct, with a TOCTOU fallback preserved. Test coverage is targeted: regression guards on no-hang, locked short-circuit, classification wrap-through, and cmux.json immutability on abort.

No files require special attention.

Important Files Changed

Filename	Overview
internal/chrome/keychain_keybase.go	Replaces goroutine+timeout pattern with a synchronous SecItemCopyMatching call guarded by SecKeychainSetUserInteractionAllowed; adds interactionMu mutex, NULL guards for CFStringCreateWithBytes and CFDictionaryCreate (fixing previous review flags), and keychainDefaultLocked via SecKeychainGetStatus. Memory ownership is correct throughout.
internal/chrome/keychain.go	Introduces ErrKeychainNoGrant/ErrKeychainLocked/ErrKeychainTimeout sentinels, a stubbable securityCLIRead var, a keychainLockedCheck pre-flight, and rewrites IsKeychainAccessError/IsKeychainLocked to use errors.Is. Classification is correct: locked errors are excluded from IsKeychainAccessError so --watch exits non-zero for retry.
internal/chrome/keychain_keybase_stub.go	Adds keychainDefaultLocked stub for non-darwin/cgo builds; returns an error so callers treat lock state as unknown and do not short-circuit — correct fallback behavior.
internal/chrome/keychain_keybase_test.go	New regression test verifies safeStoragePasswordViaKeybaseFor returns within 5 seconds (no-hang guard for the #106 goroutine-leak bug), covers both the granted and interaction-required outcomes.
internal/chrome/keychain_test.go	Adds LockedKeychainShortCircuitsBeforeCLI and UnknownLockStateFallsThroughToCLI table-driven tests; updates IsKeychainAccessError cases to use sentinel-wrapped errors and adds the #107 regression guard (locked wrapped in grant prose must not match).
internal/chrome/keychain_items_test.go	Extends IsKeychainLocked table with sentinel-based cases to verify the authoritative sentinel path works independently of the legacy string fallback.
internal/cli/cmux_sync_enable.go	Adds cmuxSyncConfigExists seam, moves Keychain pre-flight before cmuxSyncSetMode, and retains a TOCTOU ErrNotFound fallback after the write. An aborted enable now leaves cmux.json untouched.
internal/cli/cmux_sync_enable_test.go	Updates all seam-restore tuples to include cmuxSyncConfigExists; adds setCalls assertion in NoAgentWhenCmuxConfigMissing and AbortsWhenKeychainPreflightFails to verify no socketControlMode write on abort.
internal/cli/cmux_sync_test.go	Converts keychainAccessErr constant to errKeychainAccess var wrapping ErrKeychainNoGrant sentinel; adds TestRunCmuxSync_ReturnsErrorOnLockedKeychainInWatchMode to verify locked errors are not treated as access errors in --watch mode.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant C as Caller
    participant S as SafeStoragePasswordFor
    participant CGO as safeStorageViaKeybaseRunner
    participant LC as keychainLockedCheck
    participant CLI as securityCLIRead

    C->>S: SafeStoragePasswordFor(browser)
    alt signed binary
        S->>CGO: safeStoragePasswordViaKeybaseFor(service, account)
        note over CGO: interactionMu.Lock()<br/>SecKeychainSetUserInteractionAllowed(false)<br/>SecItemCopyMatching
        alt success
            CGO-->>S: password, nil
            S-->>C: password, nil
        else errSecInteractionNotAllowed
            CGO-->>S: ErrKeychainInteractionRequired
        end
    end
    S->>LC: keychainLockedCheck()
    alt "locked == true"
        LC-->>S: true, nil
        S-->>C: ErrKeychainLocked → launchd retry
    else unknown or unlocked
        LC-->>S: false or error
        S->>CLI: securityCLIRead(ctx 10s, account, service)
        alt timeout
            CLI-->>S: DeadlineExceeded
            S-->>C: ErrKeychainTimeout → exit 0
        else stderr -25308
            CLI-->>S: "stderr=-25308"
            S-->>C: ErrKeychainLocked → launchd retry
        else other error
            CLI-->>S: err
            S-->>C: ErrKeychainNoGrant → exit 0
        else success
            CLI-->>S: stdout
            S-->>C: password, nil
        end
    end

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"}}}%%
sequenceDiagram
    participant C as Caller
    participant S as SafeStoragePasswordFor
    participant CGO as safeStorageViaKeybaseRunner
    participant LC as keychainLockedCheck
    participant CLI as securityCLIRead

    C->>S: SafeStoragePasswordFor(browser)
    alt signed binary
        S->>CGO: safeStoragePasswordViaKeybaseFor(service, account)
        note over CGO: interactionMu.Lock()<br/>SecKeychainSetUserInteractionAllowed(false)<br/>SecItemCopyMatching
        alt success
            CGO-->>S: password, nil
            S-->>C: password, nil
        else errSecInteractionNotAllowed
            CGO-->>S: ErrKeychainInteractionRequired
        end
    end
    S->>LC: keychainLockedCheck()
    alt "locked == true"
        LC-->>S: true, nil
        S-->>C: ErrKeychainLocked → launchd retry
    else unknown or unlocked
        LC-->>S: false or error
        S->>CLI: securityCLIRead(ctx 10s, account, service)
        alt timeout
            CLI-->>S: DeadlineExceeded
            S-->>C: ErrKeychainTimeout → exit 0
        else stderr -25308
            CLI-->>S: "stderr=-25308"
            S-->>C: ErrKeychainLocked → launchd retry
        else other error
            CLI-->>S: err
            S-->>C: ErrKeychainNoGrant → exit 0
        else success
            CLI-->>S: stdout
            S-->>C: password, nil
        end
    end

Loading

_{Reviews (2): Last reviewed commit: "test(cli): rename keychainAccessErr to e..." | Re-trigger Greptile}