Skip to content

Make keyring availability checks read-only - #165

Open
code-monger-givenall wants to merge 1 commit into
basecamp:mainfrom
code-monger-givenall:codex/keyring-concurrency
Open

Make keyring availability checks read-only#165
code-monger-givenall wants to merge 1 commit into
basecamp:mainfrom
code-monger-givenall:codex/keyring-concurrency

Conversation

@code-monger-givenall

@code-monger-givenall code-monger-givenall commented Aug 18, 2026

Copy link
Copy Markdown

What changed

Credential-store initialization now checks Keychain availability with a read-only lookup. A missing availability entry means the keyring is working. Other errors keep the existing plaintext-file fallback.

The keyring interface lets the tests verify that initialization calls Get once and never calls Set or Delete.

Why

Every CLI process previously wrote and deleted the same hey::test item before loading the real credential. Parallel processes could make that probe fail. The affected process then switched to an empty plaintext store and reported not logged in, even though valid credentials were still in Keychain.

I reproduced this with 60 read-only hey auth status --json calls at concurrency 6. Before the change, 25 printed the keyring warning and reported unauthenticated. After the change, all 60 completed without either failure.

Checks

  • env GOWORK=off mise x golangci-lint@2.10.1 -- make check
  • env GOWORK=off mise x -- go test -race -count=1 ./internal/auth
  • 60 concurrent installed CLI auth checks, with zero keyring warnings and zero false unauthenticated results

Summary by cubic

Makes keyring availability checks read-only to remove concurrency races that caused false plaintext fallbacks. Previously the CLI wrote/deleted "hey::test"; now it does a single Get on "hey::availability" where ErrNotFound means the keyring is available, and only other errors fall back to the file store.

  • Introduces a credentialKeyring interface with a default systemCredentialKeyring wrapper around github.com/zalando/go-keyring; Store now depends on this interface so tests assert exactly one Get and zero Set/Delete during init.
  • Replaces direct calls to github.com/zalando/go-keyring with s.keyring across load/save/delete paths. External behavior is unchanged except fewer keyring warnings and no spurious unauthenticated states under concurrency.

Written for commit f36ec0f. Summary will update on new commits.

Review in cubic

@code-monger-givenall
code-monger-givenall marked this pull request as ready for review August 19, 2026 17:35
Copilot AI balanced review requested due to automatic review settings August 19, 2026 17:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Makes credential-store initialization read-only, preventing concurrent keyring probes from causing false plaintext fallbacks.

Changes:

  • Adds an injectable keyring interface.
  • Replaces write/delete availability probes with one read.
  • Tests read-only probing and fallback behavior.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/auth/store.go Implements read-only keyring availability checks.
internal/auth/store_test.go Verifies probe calls and fallback behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants