Bump github.com/basecamp/cli for the per-probe keyring item - #666
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Updates github.com/basecamp/cli to include the concurrent keyring-probe fix and improved fallback diagnostics.
Changes:
- Pins the dependency containing per-probe keyring entries.
- Refreshes Go checksums and the Nix vendor hash.
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 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
go.mod |
Updates the github.com/basecamp/cli pseudo-version. |
go.sum |
Refreshes dependency checksums. |
nix/package.nix |
Updates the vendored dependency hash. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review round converged — no code changes, head stays Both findings this round were the same issue: the red Trivy Security Scan check was unexplained in the PR body. Verified and documented:
Re-ran |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Pulls in basecamp/cli#70: the keyring availability probe now writes a per-probe keychain item (service credstore.probe.<name>, account __probe__.<pid>.<n>), so concurrent CLI processes no longer share one probe entry and race each other's read-back. With the previous pin, 20 parallel invocations against the real macOS keychain returned 19 file-store fallbacks reading stale or missing credentials; with this pin all 20 read the keyring. FallbackWarning() now carries the probe failure reason, so the fallback warning added in #664 names its cause. vendorHash recomputed and the Nix build verified via make update-nix-hash.
513b383 to
0c138bf
Compare
Bumps the
github.com/basecamp/clipin from844e9f9(cli#69) to767413f, picking up basecamp/cli#70: the keyring availability probe now writes a per-probe keychain item (servicecredstore.probe.<name>, account__probe__.<pid>.<n>), so concurrent CLI processes never share a probe entry and can't race each other's read-back.FallbackWarning()now carries the probe failure reason, so the fallback warning this repo added in #664 names its cause instead of just stating the fallback;ProbeError()is also newly available (not yet consumed here — see follow-up).This is the follow-up promised in #664 ("Bump credstore once basecamp/cli#70 merges").
Why
cli#69 stopped the lost probe-write race from being misread as an unavailable keyring, but all concurrent processes still shared one probe item, so parallel invocations kept degrading to the file store and reading stale credentials there. Under agent workloads (many
basecampprocesses at once) that showed up as intermittentexpired: true/ "Not authenticated" from processes that lost the race.Verification on a real macOS keychain
20 parallel invocations, same machine, same keychain, same load:
auth status -jauthenticated: false)user_id"3",source"oauth"auth status --profile clawdito -jme --profile clawdito -jauth_required("Not authenticated. Run: basecamp auth login")identity.id28142355, 10 × HTTP 429rate_limit("Too many concurrent requests"), 0 auth failuresme --profile clawdito -j, 10-wayauth_requiredThe 429s in the 20-way
merun are the API's per-token concurrency cap — those requests carried valid credentials (the old binary never got far enough to be rate-limited). At 10-way, below the cap, it's 10/10.Also checked: no probe items accumulate in the keychain after ~100 invocations — the per-probe entry is cleaned up synchronously on darwin (the one
credstore.probe.*item present predates this run and belongs to areviewtestservice, notbasecamp).Checks
make fmt-check vet lint lint-actions— pass (lint under mise Go 1.26.7; the Homebrew golangci-lint panics against a Go 1.27 toolchain regardless of this change)make test— passes except 10 terminal-detection tests (TestIsInteractive*,TestInteractiveStdio/Prompt,TestPredicatesDisagreeOnTheStreamTheyAskAbout,TestDeleteConfirmableFollowsTheAudienceNotTheDevice,TestBareBasecampNeverReportsASetupError,TestExplicitSetupStillRefuses,TestIsInteractiveRequiresTerminalStdio) that fail identically on untouchedmainin this PTY-less sandbox — environmental, not from this change. Full suite minus those: 30/30 packages ok.make test-e2e— 430/430 pass, run from this branch on an idle Linux host (the local rush-parallel bats harness wedged under a load-average-27 macOS machine before any test of substance ran — also unrelated to this change)make check-naming check-surface check-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage provenance-check tidy-check replace-check— passmake update-nix-hash— vendorHash recomputed and the Nix build verified in Dockergolang.org/x/crypto v0.54.0(fixed in v0.55.0), published between main's last green Security run (18:10 UTC) and the base commit's run — the same workflow is red onmainat this PR's baseaf381b34(run 33219046926, 23:02 UTC, before this PR's 23:49 UTC run). This diff doesn't touch x/crypto (the go.sum delta is cli-only). The check isn't in the main-gate required set. The fix is a separatego get golang.org/x/crypto@v0.55.0 && go mod tidy+ vendorHash recompute — see Declined.Declined
ProbeError()now (e.g. surfacing the probe diagnostic inbasecamp doctor). Separate feature; this PR is the pin bump promised in Warn about the keyring fallback on the first credential read, not only on login #664, and the reason already reaches stderr throughFallbackWarning().credstoreand is covered by its own tests in basecamp/cli (TestProbeUsesIsolatedPerProbeEntry,TestProbeDirectUsesPerProbeEntry); reproducing it here would mean hammering the real OS keychain from CI, which the suite deliberately avoids (BASECAMP_NO_KEYRING=1). The end-to-end proof above is the verification.golang.org/x/crypto v0.55.0into this diff would mix an independent dependency bump (and another vendorHash recompute) into a change that's otherwise a verified single-purpose pin. It belongs in its own PR againstmain, where it also clears the red check for every other open PR.767413fis basecamp/climain.Release follow-up
Not released here. Per RELEASING.md:
make release VERSION=0.9.2(pre-1.0 convention: patch bump for fixes) from a clean, syncedmainonce merged.