Skip to content

test: cover google keychain metadata#158

Merged
rianjs merged 1 commit into
mainfrom
fix/157-keychain-metadata
Jun 20, 2026
Merged

test: cover google keychain metadata#158
rianjs merged 1 commit into
mainfrom
fix/157-keychain-metadata

Conversation

@rianjs

@rianjs rianjs commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bump cli-common to the immutable pseudo-version containing test: cover macOS keychain metadata cli-common#64
  • add a Darwin+cgo gated empirical test for gro Keychain metadata
  • verify oauth_token metadata on fresh write and stale-metadata overwrite repair

Verification

  • go list -m github.com/open-cli-collective/cli-common
  • go mod tidy -diff
  • go test ./internal/keychain
  • GRO_KEYCHAIN_METADATA_TEST=1 go test ./internal/keychain -run TestKeychainMetadataGated -count=1 -v
  • go test ./...
  • make test
  • make verify

Closes #157

@rianjs rianjs force-pushed the fix/157-keychain-metadata branch from aea355f to 49e1d6b Compare June 20, 2026 12:42
@rianjs

rianjs commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Findings

  • Major: internal/keychain/keychain_metadata_darwin_test.go:57 only checks metadata after SetToken; it never asserts the token itself round-trips from the real Keychain backend. A broken implementation that writes the right label/description but stores a corrupted or empty oauth_token payload would still pass. Add a st.Token() assertion after the fresh write and after the repair write.
  • Minor: internal/keychain/keychain_metadata_darwin_test.go:66 seeds the stale item with empty label/description and only verifies it differs from the target. That proves overwrite updates metadata from "blank", but it does not reproduce the concrete stale non-empty metadata shape described by the ticket. If the goal is to prove the cli-common regression fix, seed deliberate wrong non-empty metadata and assert those exact stale values before overwrite.
  • Minor: internal/keychain/keychain_metadata_darwin_test.go:89 does not check that GetMetadata returns no secret bytes. The test could still pass if metadata access started leaking Data, which is a useful property to keep pinned for this backend path.

@monit-reviewer

Copy link
Copy Markdown

PR review failed and the daemon has stopped retrying for this commit. I am removing my pending review request to stop the automated review cycle. Re-request @monit-reviewer to retry, or push a new commit and re-request.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: 49e1d6b

Manually approved via CLI.

@rianjs rianjs merged commit 2c4ef57 into main Jun 20, 2026
11 checks passed
@rianjs rianjs deleted the fix/157-keychain-metadata branch June 20, 2026 12:44
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.

Fix macOS Keychain metadata for credstore items

2 participants