Conversation
…or skip and remedy
…nd migrate comment
github-actions
Bot
added
commands
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Authentication storage behavior depends on the still-unmerged basecamp/cli#79 concurrency and timeout implementation.
Review effort: Balanced
Findings: None
What changed in this PR
Bounds Linux keyring operations to prevent stalled D-Bus handshakes from hanging the CLI, with improved diagnostics, migration handling, documentation, and regression coverage.
Changes:
- Applies 10-second Linux keyring probe/operation limits through
basecamp/cli#79. - Preserves explicit fallback/error semantics across authentication, doctor, and migration.
- Documents and tests
BASECAMP_NO_KEYRING.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
SECURITY.md |
Documents keyring deadlines and plaintext fallback risks. |
nix/package.nix |
Updates the dependency vendor hash. |
go.mod |
Pins the reviewed credential-store implementation. |
go.sum |
Updates dependency checksums. |
internal/auth/keyring.go |
Configures Linux timeouts and improves timeout remedies. |
internal/auth/keyring_test.go |
Tests timeout configuration and errors. |
internal/auth/issue800_keyring_linux_test.go |
Reproduces the stalled D-Bus handshake. |
internal/commands/auth.go |
Adds credential-storage help. |
internal/commands/auth_status_agent_test.go |
Adapts agent diagnostic coverage. |
internal/commands/doctor.go |
Distinguishes unreadable credentials and bounds legacy lookup. |
internal/commands/doctor_test.go |
Covers doctor timeout and diagnostic behavior. |
internal/commands/migrate.go |
Bounds legacy keyring migration and honors bypass. |
internal/commands/migrate_test.go |
Tests migration timeout and bypass behavior. |
internal/cli/help.go |
Adds root credential-storage guidance. |
internal/cli/help_test.go |
Verifies override discoverability. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+174
to
+179
| } else if unreadable { | ||
| checks = append(checks, Check{ | ||
| Name: "Authentication", | ||
| Status: "skip", | ||
| Message: "Skipped (credentials could not be read)", | ||
| }) |
robzolkos
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Linux commands now return instead of hanging indefinitely on a stalled keyring. A failed initial probe retains the existing warned file fallback; a later keyring timeout returns an error without silently switching storage or suggesting another login.
doctorand legacy migration also bound their keyring calls. Unreadable credentials get no login hint or breadcrumb; genuinely missing/broken agent credentials keep their client-credentials remedy.BASECAMP_NO_KEYRING=1is documented in root/auth/doctor help and SECURITY.md. Migration honors it without marking skipped keyring entries as migrated.Why
A local Unix socket that stalls the real D-Bus authentication path reproduces the indefinite desktop wait. Subsequent keyring operations and legacy probes needed bounds too, otherwise fixing startup alone leaves other commands able to hang.
Dependency: review and merge basecamp/cli#79 first. This PR pins its reviewed immutable commit
86242a69567b; the Nix vendor hash was regenerated and its Docker Nix build passed. Neither PR has been merged or given auto-merge.Testing
make checkpasses via fullbin/cion final HEAD21451b89e88623a20aac18978773a301f16211eb.01b43a2cprecedes fix6ab6743a: desktop credential loading stays blocked after 12 seconds before the fix, and returns after approximately 10 seconds afterward. Headless and explicit-bypass controls pass.auth status --jsonagainst a stalled socket returns after approximately 10.03 seconds with warned file fallback; explicit bypass returns in approximately 0.03 seconds.5f3c7a54precedes breadcrumb fix297efe71. Real doctor checks now distinguish an unreadable store from missing credentials and preserve the agent login remedy.Blast radius: no API/SDK change. Fresh Fizzy full tests and command race tests pass against the shared library in a disposable workspace; its zero-value timeout behavior is unchanged. HEY owns a separate credential store. Windows command-test compilation has an unrelated failure on unchanged main too (
newOperatorFixture/connectSetupPath); all shipped binary targets compile.Review corrections: the original change missed direct legacy migration calls, doctor's follow-on recovery/skip paths, timeout remedies on an already-active file backend, and completion markers after bypassed migration. Those paths now have fixes and regression coverage. The credential-subprocess test also has a wider timing margin.
Limits: a started write/delete cannot be canceled and may complete after the timeout. The shared store refuses further operations rather than claiming a safe retry. An unusually slow desktop unlock can hit the initial 10-second bound and trigger the warned fallback. Later-operation timeouts are covered with injected-provider tests, not a complete fake Secret Service. The specific GNOME Keyring crash relationship has not been reproduced.
Related: #800
Earlier automated review and reproduction receipts
Intent
Fix #800. The red subprocess test was committed first (01b43a2), followed by the Linux fix (6ab6743); preserve that history. Linux GUI/TTY and headless credential stores use 10-second ProbeTimeout and OperationTimeout via basecamp/cli#79, pinned to immutable reviewed commit 86242a69567b. Zero/default behavior of other library consumers and interactive macOS/Windows remains unchanged. Keep warned initial-probe plaintext fallback; an established keyring operation timeout is an error with BASECAMP_NO_KEYRING=1 as the explicit plaintext remedy, not a missing login or automatic backend switch. Cover reads/writes/deletes/migration, doctor's bounded best-effort legacy lookup and credential read errors, preserving kind-aware agent remedies. Document the env override in actual root/auth/doctor help and SECURITY.md. No SDK bump or API changes. Nix hash was recomputed and a Docker Nix build passed. The full bin/ci is GREEN with repo-pinned zizmor 1.30.0 and Ruby 3.3 explicitly ahead of ~/.local/bin on PATH; use GOWORK=off to avoid the unrelated parent workspace. All five shipped binaries compile; Windows command TEST compilation fails on unchanged main too (missing newOperatorFixture/connectSetupPath), do not change unrelated connector tests. Fresh Fizzy main's full tests pass against the new shared library in a disposable go.work; HEY has its own store and no shared credstore dependency. Perform scoped review, tests, lint and docs; open an unmerged PR, include dependency ordering (shared PR79 first), blast radius, red-before-fix evidence and checks. Do NOT merge, enable auto-merge, close issue800, squash, or drop prior commits. CI watcher is explicitly skipped only because the local gh wrapper pollutes JSON; actual hosted GitHub checks will be verified separately by the outer agent, not skipped. Basecamp full local bin/ci ran green in this session.
What Changed
Refs #800. Depends on basecamp/cli#79. This branch pins
github.com/basecamp/clito the reviewed commit86242a69567b, so #79 needs to land first. The Nix vendor hash is updated to match.internal/auth): On Linux, the keyring availability probe and every later read, write, delete and migrate now have a 10-second limit, including GUI and TTY sessions. On macOS and Windows, only headless sessions bound the probe, as before. If the first probe times out, the CLI still falls back to plaintext file storage with a warning. If an operation times out after the keyring is already in use, the CLI returns an error instead of reporting a missing login or switching backends. That error suggestsBASECAMP_NO_KEYRING=1as the explicit plaintext option, and only while the keyring is the active store. A subprocess test reproduces the stalled D-Bus handshake. It was committed ahead of the fix so it fails first.doctorandmigrate(internal/commands):doctornow reports "Could not read stored credentials" and skips Authentication with "credentials could not be read", not "no credentials" plus a login hint. Agent credentials that are stored but unusable keep their kind-specific remedy.doctor's legacybcq::*keyring lookup is best-effort and shares one 10-second budget on Linux.basecamp migrateskips keyring migration whenBASECAMP_NO_KEYRINGis set. On Linux, each legacy keyring call gives up after 10 seconds, and once one call stalls, the rest fail immediately.auth --help,doctor --helpand SECURITY.md now documentBASECAMP_NO_KEYRING, the Linux timeouts, and the fact that a timed-out write may still complete.Risk Assessment
✅ Low: The follow-up commit fixes all four round-1 findings that called for a change, and the no-op tradeoff note needs none. Doctor now reports an unreadable store as a read error, not a missing login.
basecamp migratenow honours BASECAMP_NO_KEYRING and bounds its Linux keyring calls. The plaintext-storage remedy is added only while the keyring is in use, and the subprocess test has a wider timing margin. No new defects or intent conflicts turned up.Testing
Red before fix, green after. The committed subprocess testTestIssue800DesktopKeyringHandshakeMustBeBoundedfails on 01b43a2's code: the desktop read is still blocked after 12s. At HEAD it passes, with the desktop read bounded at about 10s. Focused unit tests all pass. They cover: - the Linux probe and operation timeouts, with the non-Linux interactive default unchanged; - a keyring operation timeout returned as an error that namesBASECAMP_NO_KEYRING=1, without that remedy when the store has already fallen back to the file; - doctor treating an unreadable store differently from a missing login, and its bounded legacy-install lookup; - the agent-specific login remedy; - bounded migrate and migrate skipping the keyring when bypassed; - the help text. The pinned library'scredstoretests pass, and the binary cross-compiles for every platform checked. End to end with the real binaries against a fake desktop D-Bus whose Secret Service handshake never finishes: - Before the fix,auth status,doctorandmigrateall hang until killed at 40s. - After the fix,auth statusreturns in 10s with the plaintext-fallback warning and reads the stored token.doctorfinishes in 20s: 10s for the credential probe and 10s for the separate legacy-install lookup.migratereturns in 10s with an error that namesBASECAMP_NO_KEYRING=1. - WithBASECAMP_NO_KEYRING=1, all three return immediately and print no warning. Help and docs. Root, auth and doctor help render the documented override, and SECURITY.md documents it. Not covered end to end. A timeout after a successful probe would need a working fake Secret Service, so only the unit tests cover it. Scope and cleanup. I did not run the full suite or linters, as this phase requires. Temporary binaries and the extracted base tree were removed; the worktree is clean.Evidence: Red before fix: subprocess test fails on 01b43a2
Evidence: Green after fix: same subprocess test passes at HEAD
Evidence: CLI transcript: auth status and doctor against a stalled D-Bus, before and after the fix and with BASECAMP_NO_KEYRING=1
Evidence: CLI transcript: migrate against a stalled D-Bus, before and after the fix and with BASECAMP_NO_KEYRING=1
Evidence: Rendered root, auth and doctor help showing BASECAMP_NO_KEYRING
Evidence: SECURITY.md documentation diff
Evidence: Focused unit test results
Evidence: Fake stalled Secret Service D-Bus server
Evidence: Script for the auth status / doctor end-to-end check
Evidence: Script for the migrate end-to-end check
Evidence: Before vs after summary
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed ✅
internal/commands/doctor.go:178- When checkCredentials fails because the keyring read timed out (or the store is otherwise unreadable), runDoctorChecks still adds an Authentication entry reading "Skipped (no credentials)" with Hint app.Auth.LoginHint(). So after a Linux keyring timeout, doctor tells the user they have no credentials and should run an OAuth login. The intent says a timeout is "an error with BASECAMP_NO_KEYRING=1 as the explicit plaintext remedy, not a missing login", and the new checkCredentials branch deliberately leaves Hint empty for this case, but the next check puts the login remedy back. Fix: keep the login-hinted skip only for a real missing or unusable credential (the !authenticated or CodeAuth cases). For a read error, skip with a message such as "Skipped (credentials could not be read)" and no login hint.internal/commands/migrate.go:160- Sibling path left unbounded.basecamp migratecalls go-keyring directly through keyringOps.get/set/delete, with no Linux timeout, and it runs even when BASECAMP_NO_KEYRING is set. On a desktop Linux session with the auth login/status/doctor hang forever (no timeout) when keyring's D-Bus handshake stalls; BASECAMP_NO_KEYRING=1 works but is undocumented #800 D-Bus stall it still hangs forever. That contradicts the new root help ("Set BASECAMP_NO_KEYRING=1 to bypass the system keyring") and SECURITY.md ("Any non-empty BASECAMP_NO_KEYRING value bypasses the keyring"). Doctor's legacy check can also recommendbasecamp migrateafter its own bounded lookup gives up, if bcq cache or theme directories exist. Suggested fix: skip migrateKeyring when BASECAMP_NO_KEYRING is set, and on Linux run it under the same budget as legacyKeyringEntryExists. This goes somewhat beyond the stated scope, so confirm with the author.internal/auth/keyring.go:282- keyringOperationError decides by errors.Is(DeadlineExceeded) plus a "keyring" substring. After an initial probe timeout, credstore's fallback file Load wraps probeErr ("keyring probe timed out…: context deadline exceeded") into every file error. A non-missing file failure, such as a corrupt credentials.json or permission denied, therefore gets "to use plaintext credential storage explicitly, set BASECAMP_NO_KEYRING=1" appended, even though the store is already on plaintext and the env var would not fix the problem. Consider adding the remedy only when s.ensure().UsingKeyring() is true.internal/auth/keyring.go:141- Tradeoff note. Linux Secret Service backends (gnome-keyring, KWallet) also show unlock prompts for locked collections. Under the 10s probe and operation bounds, a user who takes longer than 10s to unlock will get either the warned plaintext fallback (probe) or an error that blocks keyring use for the rest of the process (operation). The intent explicitly authorizes 10s bounds for Linux GUI/TTY sessions, so no change is needed, but SECURITY.md and the code comment could mention that a slow unlock prompt on Linux is also cut off.internal/auth/issue800_keyring_linux_test.go:93- The subprocess test gives only a 2s margin over the 10s probe for process startup, the test-framework init and the file load. It is also sequential and adds about 20s of wall time togo test ./internal/authon Linux. release.yml runs the whole suite with -race across parallel packages, where a loaded runner could go over the 12s budget and fail the test spuriously. A slightly larger margin, such as +5s, would still catch the unbounded hang and lower that risk.🔧 Fix: Bound migrate keyring calls, fix doctor read-error skip and remedy
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
Red before fix: temporarily restored 01b43a2'sinternal/auth/,go.modandgo.sumin the worktree and ranGOWORK=off go test ./internal/auth/ -run TestIssue800 -count=1 -v. The desktop subtest FAILED: the read was still blocked in D-Bus authentication after 12s. The headless and no-keyring controls passed. Files were then restored from HEAD.Green after fix:GOWORK=off go test ./internal/auth/ -run TestIssue800 -count=1 -vat HEAD. The desktop subtest passed in 10.03s, and so did the headless and no-keyring controls.GOWORK=off go test ./internal/auth/ -run 'TestEnsureBoundsLinuxKeyringAndHeadlessProbes|TestKeyringTimeoutIsAnErrorWithAnExplicitFileStorageRemedy|TestFileStoreErrorsAfterAProbeTimeoutOmitTheFileStorageRemedy' -vGOWORK=off go test ./internal/commands/ -run 'TestDoctorDoesNotCallAnUnreadableStoreAMissingLogin|TestDoctorLegacyKeyringProbe|TestMigrateKeyring_StalledKeyringIsBounded|TestMigrateSkipsKeyringWhenNoKeyring|TestAuthStatus.*Agent|TestDoctorOffersTheAgentLoginForABrokenAgent' -vGOWORK=off go test ./internal/cli/ -run TestKeyringBypassIsDiscoverableInHelp -v, which covers root, auth and doctor helpBroader targeted packages:go test ./internal/auth/ -run 'Ensure|Keyring|FileStore|Store',go test ./internal/cli/ -run 'Help',go test ./internal/commands/ -run 'Doctor|Migrate|Credential|AuthStatus|LegacyInstall'GOWORK=off go test github.com/basecamp/cli/credstoreagainst the pinned 86242a69567b libraryManual CLI check: built the base (c0b5896) and HEAD binaries and ranauth statusanddoctorin an isolated HOME/XDG setup with a seeded credentials.json,WAYLAND_DISPLAYset, andDBUS_SESSION_BUS_ADDRESSpointed atstalled_dbus.py. Each was run with and withoutBASECAMP_NO_KEYRING=1(script:drive_stalled_bus.sh).Manual CLI check:migratewith a legacy bcq config, before and after the fix, and withBASECAMP_NO_KEYRING=1(script:drive_migrate.sh)Renderedbasecamp --help,basecamp auth --helpandbasecamp doctor --helpfrom the HEAD binary and captured theBASECAMP_NO_KEYRINGsections. Captured the SECURITY.md diff.Cross-compiled./cmd/basecampwith CGO_ENABLED=0 for darwin, linux and windows on amd64 and arm64, plus freebsd/amd64 and openbsd/arm64. All built.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by cubic
Fixes the CLI hanging indefinitely when the Linux keyring's Secret Service D-Bus handshake stalls (#800) by bounding keyring probes and operations with a 10-second deadline.
Behavior
BASECAMP_NO_KEYRING=1, never a missing-login message or automatic backend switch.doctorreports an unreadable store as a read error and skips authentication and login breadcrumbs instead of prompting for a login; its legacybcq::*lookup is best-effort and shares the 10-second budget.basecamp migratebounds its Linux keyring calls, skips the keyring whenBASECAMP_NO_KEYRINGis set, and skips the completion marker in that case so leftoverbcqentries are still found on a later run.BASECAMP_NO_KEYRING, the Linux timeouts, and the timed-out-write caveat in root/auth/doctor help andSECURITY.md.Adoption
github.com/basecamp/clito fix(credstore): support bounded keyring operations cli#79 (commit86242a69567b), which must land first; the Nix vendor hash is updated to match.Written for commit 21451b8. Summary will update on new commits.