test(client): stop the suite writing to the developer's real Keychain

[omarshahine]

Client authentication tests currently write synthetic OAuth tokens to the developer's persistent credential stores. On CGO-enabled macOS builds, those entries reach Keychain and remain after the test process exits.

Install in-memory primary and fallback stores in the client package's TestMain, covering every test that reaches authentication. Production authentication and storage behavior stay unchanged. The README also clarifies the existing build distinction: published macOS binaries disable CGO and use the file cache, while CGO-enabled source builds prefer Keychain.

Verification:

• Full uncached test suites on macOS with Go 1.26.8 and Go 1.26.7; core coverage is 86.1%.
• Guarded compiled-test reproduction: removing TestMain reaches a persistent opener and fails; the same authentication test passes with TestMain. Guards stop access before opening any real credential store.
• Built the real CLI and ran --version and --help; formatting and golangci-lint pass.
• Final branch Codex autoreview found no actionable findings through P2; CI is green on head c12effd: https://github.com/steipete/eightctl/actions/runs/34660657496.

Contributor: @omarshahine. The changelog entry is carried by the final dependency/notes PR to keep sibling branches independent.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 11, 2026, 8:15 PM ET / September 12, 2026, 00:15 UTC (Revision 2).

ClawSweeper review

What this changes

Redirects client authentication tests to in-memory credential stores and clarifies how macOS builds choose token storage.

Merge readiness

✅ Ready for maintainer review

The fix remains necessary on current main, and no actionable defect was found. Maintainer verification directly covers the credential-store isolation; this PR is ready for ordinary merge review.

Priority: P2
Reviewed head: c12effd959914721c5e95fedab7ffff95ecee07f

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused, maintainable fix with relevant owner verification and no actionable findings.
Proof confidence	🌊 off-meta tidepool	Not applicable: The collaborator-authored PR is exempt from the external-contributor gate. The owner nevertheless reports exact-head macOS compiled-test verification showing TestMain prevents entry into guarded persistent openers; help/version output is not relied on for that claim.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The collaborator-authored PR is exempt from the external-contributor gate. The owner nevertheless reports exact-head macOS compiled-test verification showing TestMain prevents entry into guarded persistent openers; help/version output is not relied on for that claim.
Evidence reviewed	8 items	Introduced scope and review continuity: The pinned main-to-head diff contains only README wording and the 40-line client TestMain. Comparison with the previously reviewed head shows only the README changed; the prior review recorded no findings. Existing storage boundary: Successful authentication calls tokencache.Save. Both existing opener hooks are replaced before m.Run and restored afterward. Current client tests contain no parallel-test calls or competing opener overrides. Still necessary on main and latest release: The client trees at fetched main and v0.2.5 have no main_test.go. Main retains the authentication-to-persistent-cache path, and the supplied related PR does not replace this fix.
Findings	None	None.
Security	None	None.

How this fits together

Eightctl’s client authenticates with Eight Sleep and caches tokens for subsequent commands. Its authentication tests exercise that same cache path; the new test entrypoint substitutes memory-backed stores before the tests run.

flowchart TD
 A[Client test suite] --> B[Install test credential stores]
 B --> C[Authentication tests]
 C --> D[Client token cache]
 D --> E[Primary memory store]
 D --> F[Fallback memory store]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Keep credential-store substitution at the client test boundary while preserving the production cache and its separate backend tests.

Do we have a high-confidence way to reproduce the issue?

Yes. Current source connects the authentication test to persistent cache openers, and the owner reports a guarded before/after compiled-test reproduction on macOS; this read-only review did not execute tests.

Is this the best way to solve the issue?

Yes. Reusing both existing opener hooks in TestMain covers the package without altering production storage or duplicating backend implementation.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against db84b936e0ba.

Labels

Label justifications:

• P2: Prevents authentication tests from polluting developers' persistent credential stores without changing shipped runtime behavior.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor gate. The owner nevertheless reports exact-head macOS compiled-test verification showing TestMain prevents entry into guarded persistent openers; help/version output is not relied on for that claim.

Evidence

What I checked:

• Introduced scope and review continuity: The pinned main-to-head diff contains only README wording and the 40-line client TestMain. Comparison with the previously reviewed head shows only the README changed; the prior review recorded no findings. (internal/client/main_test.go:24, c12effd95991)
• Existing storage boundary: Successful authentication calls tokencache.Save. Both existing opener hooks are replaced before m.Run and restored afterward. Current client tests contain no parallel-test calls or competing opener overrides. (internal/client/eightsleep.go:171, c12effd95991)
• Still necessary on main and latest release: The client trees at fetched main and v0.2.5 have no main_test.go. Main retains the authentication-to-persistent-cache path, and the supplied related PR does not replace this fix. (internal/client/eightsleep_test.go:81, db84b936e0ba)
• Exact-head maintainer verification: The owner reports full uncached macOS arm64 suites on Go 1.26.7 and 1.26.8, 86.1% core coverage, and a guarded compiled-test comparison: omitting TestMain reaches the persistent opener and exits 2; including it passes with exit 0. The guards prevent real credential access. CLI help/version checks are separate supplemental evidence. Source: test(client): stop the suite writing to the developer's real Keychain #88 (comment). (internal/client/main_test.go:24, c12effd95991)
• Release build configuration and dependency signal: GoReleaser sets CGO_ENABLED=0, while go.mod pins github.com/99designs/keyring v1.2.2. The new test directly uses that dependency's memory backend, and the README's macOS distinction depends on its build constraints. (.goreleaser.yaml:15, c12effd95991)
• Pinned dependency contract: The dependency's v1.2.2 Keychain source requires darwin and cgo. Its array backend stores items in memory and is intended for tests, supporting both the documentation correction and chosen substitution. (keychain.go:1)

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• omarshahine: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-09T02:59:18.007Z sha a58c5f4 :: needs maintainer review before merge. :: none

[steipete]

Maintainer verification completed on head c12effd959914721c5e95fedab7ffff95ecee07f.

• CGO_ENABLED=1 GOTOOLCHAIN=go1.26.8 go test -count=1 ./... and the full uncached suite with GOTOOLCHAIN=go1.26.7 pass on macOS arm64.
• The existing core coverage gate reports 86.1%; golangci-lint reports zero issues and gofumpt is clean.
• Built the real CLI with Go 1.26.8 and ran --version and --help successfully.
• Reproduced the unsafe call path with compiled test binaries using a temporary Go overlay that makes either persistent opener panic before accessing storage. With TestMain omitted, TestAuthTokenEndpoint_FormEncoded fails at the primary opener (exit 2). With this PR's TestMain, the same test passes (exit 0). This confirms the new package boundary prevents the call; the check does not touch or delete real credentials.
• Final branch autoreview against origin/main is scoped-clean through P2.
• CI is green on this exact head, including the minimum-Go test, install smoke, coverage, release checks, and artifact smoke.

The added README correction documents the existing CGO distinction; it does not change token-storage policy. Thanks @omarshahine for the fix. Prepared for squash merge; the final dependency/notes PR carries the changelog entry.