[thebrandonlucas]

Stack 6 of 6. Base: feat/cli-403-credential-source (#66).

Closes #48.

Problem

voltage login refused any saved name, and only after the browser step: the user opened the URL, confirmed the code, and then got Credential NAME already exists; log it out first. Signing in again as the same user, for example after the login expired, needed voltage logout --local --account NAME first. The expired-login hint said so.

Behavior

Saved under the name	New login	Result
A login of the same user at the same auth URL	any	Replaced under the credential lock. The replaced refresh token is revoked. --json reports "replaced": true.
A login of another user, or a login saved without a user ID	any	Refused with the same error. The saved login is kept, and the new session is revoked.
A login whose secret cannot be read (locked keychain, damaged or missing file)	any	Refused as a name in use, with a hint naming voltage logout --local --account NAME. With --account NAME, this happens before the browser step.
An API key, or a login at another auth URL	--account NAME	Refused before the browser step.
Nothing	any	Saved, as before ("replaced": false).

• Credential store: --credential-store is now optional. Without it, a replaced login keeps its saved store, and a new login uses the keychain, as before.
• Saving: Settings::put_account saves an account under the credential lock. It replaces a saved one only when replaceable accepts the saved account and then same accepts its credential. The secret is read only for a replaceable account, so add_account (and so auth import-key) refuses a used name with name_in_use without reading it.
• Failure and rollback: a failed secret write or config save may already have replaced its file (for example, when the directory sync after the rename fails). So every failure runs restore. It writes the old secret back, or removes the new one when nothing was replaced, and saves the config again. A rollback that fails is reported in the error's hint: voltage login --account NAME when the old secret is lost, and voltage logout --local --account NAME when the config cannot be put back. A test-only hook in atomic_write injects failures before or after the rename.
• Ctrl-C: after the new login is saved, Ctrl-C while the replaced session is being revoked says "the new login is saved", and names account global signout for the old session.
• logout --local: removes an account whose secret is already gone. When the secret cannot be removed, it removes the account and prints a warning. A dropped secret delete during a store change also warns now, where it used to fail silently.
• Hints: the expired or revoked login hint, and the 401 hint for a saved login, are now only voltage login --account NAME. shell_word moves from auth to config, so config no longer imports auth.
• Docs: CHANGELOG, login-and-profiles.md, troubleshooting.md and exit-codes.md are updated, including the replaced field in --json output.
• Untested: a replacement that changes store (keychain↔file), and the keychain arm of delete_secret, because the tests cannot use an OS keychain. Code comments say so.

Checks

On head c66d2a3:

• The gate passes in nix develop: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (118 unit, 103 integration), and python3 scripts/check-coverage.py.
• CRAP: 0 of 555 measured functions are at or above 20. The highest is wait_for_payment at 19.60.
• Full diff against feat(cli): name VOLTAGE_API_KEY and the organization source on a 403 #66: 984 changed lines.
• New tests:
  • a_login_of_the_same_user_replaces_the_saved_one_and_revokes_it: replaces a file-stored login without --credential-store, and checks that the store stays file. The tokens are replaced, replaced is true, and the old refresh token is revoked. When revocation fails, the login still succeeds and names the signout step.
  • interrupt_while_revoking_a_replaced_login_says_the_new_one_is_saved: SIGINT during the delayed revoke. Checks the message and the saved new tokens.
  • a_replacement_that_fails_after_a_rename_restores_the_saved_login (unit): a secret write failing after its rename, a write and its rollback both failing before the rename, and a config save failing after its rename. Each case reloads from disk and checks that the old secret and account are still saved.
  • a_failed_rollback_names_the_lost_login and a_new_account_that_fails_after_a_rename_leaves_nothing_saved (unit): the rollback hints, and that a new account leaves no account and no secret behind.
  • a_secret_that_cannot_be_removed_is_discarded_with_a_warning (unit): the logout --local path.
  • The earlier tests stay: another user, another auth URL, no user ID, an unreadable saved login, the unit refusal tests, and the read-only configuration directory.
• I reviewed the diff with voltage-pr-review and the Rust style checklist. An independent review of the rollback found one bug, a config left on disk after a failed save, which is fixed in this head.
• Every commit is signed G blu@blu.cx, with no trailers.
• Not run: a login against staging, and a real keychain replacement.

[thebrandonlucas]

Review at 2c776c6 (Aurelius). 2 low findings. Nothing blocking.

The main path is right:

• put_account reloads under the lock, writes the new secret, saves, and returns the old credential for revocation.
• On a failed save, restore puts the old account and secret back. a_replacement_that_cannot_be_saved_restores_the_saved_login really exercises this: the new secret goes into the 0700 credentials/ directory, so only save() fails under the 0500 parent.
• Another user and another auth URL are refused. The other-URL case is refused before any request.

1. Low: src/config.rs:605: put_account reads the saved secret before replaces decides, and a read failure returns early. This changes add_account, whose predicate always refuses. If a saved name's secret is missing, unreadable, or in a locked keychain, auth import-key --account NAME now fails with "Cannot read OS credential store…", an I/O error, or "Invalid saved credential". Before, it failed with name_in_use. For login, the same failure comes after the browser step: the new session is revoked, and the error is about the old secret, not the name. Fix: decide on the account first, and read the secret only when the account could be replaced. For example, pass is_login_at separately, or have add_account refuse when the name exists without reading. Map a failed read of the old secret to name_in_use, so the user is told to log out first. Add a test that removes a saved login's credential file and then runs add_account, or a same-user login.

2. Low (tests): new branches with no test:

• src/auth.rs:724: a saved login with no user_id is refused after the browser step.
• src/config.rs:621 and :638 (restore): a replacement into a different --credential-store. On success this deletes the old store's secret; on a failed save it deletes the new one.

Please add at least the user_id: None case. The store-change case needs a keychain, so a unit test that drives put_account from File into a stubbed store, or a comment saying why it is untested, would do.

Checks on 2c776c6:

• Gate in nix develop: fmt, clippy, machete, tests (112 unit, 100 integration) and coverage pass, and the tree is clean.
• CRAP: 0 functions at or above 20. The highest is wait_for_payment at 19.60. login is 11.22.
• Signed G blu@blu.cx, with no trailers.
• Not run: a login against staging, and a keychain-backed replacement.

[thebrandonlucas]

First review at 2c776c6, against b7a3e92. This is not an approval. The main flow is right:

• The --account precheck refuses an API key or another auth URL before the device request, and a test checks that no request is sent.
• put_account takes the lock, reloads, and asks replaces with the saved account and credential. The user check needs old.user_id.is_some(), so a login without a user ID is never replaced.
• A different user's new session is revoked by the existing login error path.
• I checked the auth service (codex/cli-device-login fd88993, src/device_oauth.rs:436): revoking a token it does not know returns 204. So replacing an expired or revoked login does not print the "Could not revoke" warning.

I found 1 medium problem, 1 low problem and 1 nit.

1. Medium: an unreadable saved secret now blocks login after the browser step, in a loop.

• src/config.rs:605: put_account reads the old credential with ? before it asks replaces.
• When a keychain login's secret cannot be read (locked or missing keychain, or a removed entry), voltage login --account NAME runs the whole browser step and then fails with "Cannot read OS credential store; unlock it or log in with --credential-store file". It also revokes the new session.
• Following that advice (--credential-store file) fails the same way, because the old account still names the keychain. troubleshooting.md:95 gives the same advice. An unparseable credential file fails the same way with "Invalid saved credential".
• Before this PR, --account NAME was refused with name_in_use before the browser step, and the expired-login hint named voltage logout --local. That hint is now gone.
• No test covers it.

Fix: when the old credential cannot be read, refuse with name_in_use and a hint to run voltage logout --local --account NAME first. Do this before the browser step when --account is given, and in put_account otherwise. Add a test with an unreadable credential file, for example invalid JSON.

2. Low: the store-change cleanup is untested.

• src/config.rs:621-623 deletes the replaced secret when the new login uses another credential store.
• :637-639 removes the new secret on restore.
• Coverage shows both branches at 0 hits. Replacing a keychain login with --credential-store file, or the reverse, is the only way to reach them.
• The repository has no keychain mock, so this may need one, or a store-agnostic seam for delete_secret. If you leave them untested, say so in the PR description.

3. Nit: CHANGELOG.md:25 adds a blank line inside the "Changed" list, which makes it a loose list. Remove it.

Checks on 2c776c6:

• Gate in nix develop: fmt, clippy, machete, tests (112 unit, 100 integration) and coverage pass, and the tree is clean afterwards.
• CRAP, with the rustup toolchain: 0 of 537 measured functions are at or above 20. The highest is wait_for_payment at 19.60. login is at 11.22, put_account at 9.00, and restore at 3.00.
• The commit is signed G blu@blu.cx.
• Not run: a staging login, and a real keychain replacement.

[thebrandonlucas]

Re-review at be91ef5 (Aurelius). Both of my findings are fixed:

• put_account checks replaceable before it reads the secret, so add_account and import-key refuse with name_in_use and read nothing.
• An unreadable replaceable secret is refused with a logout --local hint, before the browser step when --account is given. logout --local now tolerates a missing secret.
• user_id: None, a missing or corrupt file, and the in-lock path are tested. The store-change gap is stated in the code and in the description.

Nit: src/config.rs:9 now imports auth::shell_word, while auth imports config. Rust allows the cycle, but it puts a lower-level module under the higher one. Consider moving shell_word into config, or into a small shared module, and importing it from auth. This does not block.

Checks on be91ef5:

• Gate in nix develop: fmt, clippy, machete, tests (113 unit, 102 integration) and coverage pass, and the tree is clean.
• CRAP: 0 functions at or above 20. The highest is wait_for_payment at 19.60. login is 13.21.
• Signed G blu@blu.cx, with no trailers.
• Not run: a staging login, and a keychain replacement.

[thebrandonlucas]

Re-review at be91ef5. This is not an approval. My medium finding, my low finding and my nit are fixed:

• put_account checks replaceable before it reads the secret. A replaceable secret that cannot be read is refused as name_in_use with a removal hint.
• With --account, login checks this before the device request, and the integration test checks that no request is sent.
• delete_secret treats a missing file or NoEntry as already removed, so the hint's logout --local works for a missing or damaged file. The test checks that.
• The store-change branches are untested, and the code comments and the PR description now say so.
• The blank line in the CHANGELOG is gone.

I found 1 low problem and 1 nit.

1. Low: a keychain that is unavailable, not just locked, still has no way out.

• unreadable_name (src/config.rs:744) tells a keychain user to "Unlock the OS credential store, or remove it with voltage logout --local --account NAME".
• When the store cannot be reached, as on headless Linux without a Secret Service (the case troubleshooting.md gives for this error), delete_credential returns a platform error, not NoEntry.
• So logout --local fails with "Cannot remove saved credential" at src/auth.rs:971, before it removes the account, and the name stays in use.

Fix: either add "or sign in under another name with --account" to the keychain hint, or let logout --local remove the account with a warning when only the keychain delete fails. The hint is the smaller change.

2. Nit: config now imports auth::shell_word (src/config.rs:9). That makes config depend on auth, which already depends on config. Consider moving shell_word to a module both can use, such as output or a small shell module.

Checks on be91ef5:

• Gate in nix develop: fmt, clippy, machete, tests (113 unit, 102 integration) and coverage pass, and the tree is clean afterwards.
• CRAP, with the rustup toolchain: 0 of 539 measured functions are at or above 20. The highest is wait_for_payment at 19.60, and login is at 13.21.
• The commit is signed G blu@blu.cx.
• Not run: a staging login, and a real keychain, locked or unavailable.

[tee8z]

Reviewed at be91ef5. CRAP on this head: 0 of 539 functions at or above 20, highest wait_for_payment at 19.60 (login 13.21). There is one bug I'd fix before merge (inline on src/auth.rs, store: flags.credential_store), a couple of smaller issues, and some suggestions that flatten the nested matches in config.rs.

• --json output gains a replaced field. It deserves a line in the CHANGELOG and docs, since agents read that output.
• config.rs now imports auth::shell_word, while auth already depends on config. Moving shell_word to a shared module would avoid the cycle.
• The PR description ends with a "Generated with Claude Code" line; AGENTS.md says to leave tool attribution out of PR descriptions.

I applied the suggestions on this head and checked them: cargo clippy --all-targets -- -D warnings is clean and the unit and integration tests pass. I applied all of them together.

[tee8z]

Requesting changes on be91ef5. Three correctness findings remain:

1. Credential replacement can leave a revoked new token on disk and lose the previous local token. The new inline finding covers reproduced post-rename and rollback failures.
2. Omitting --credential-store during replacement selects keychain for a file-stored login. Preserve the saved store unless the caller explicitly chooses another. Reproduced with an unavailable Secret Service: exit 3 after authentication.
3. Ctrl-C while revoking the replaced session says the new login was not saved, although it is already persisted. Track saved-login state separately from old-session cleanup. Reproduced: exit 130 with new credentials on disk.

Validation: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked, and python3 scripts/check-coverage.py passed after one terminal-test timeout was rerun successfully. The instrumented suite also passed. CRAP: 539 measured functions, zero violations, highest wait_for_payment 19.04; 150 test functions unmeasured. A working OS keychain and live services were not exercised.

[thebrandonlucas]

@tee8z All items from both reviews are addressed in c66d2a3. The inline replies give the details, and the description reports the checks on that head.

• replaced in --json: now in the CHANGELOG and login-and-profiles.md.
• shell_word: moved to config.rs. auth and api import it from there, so config no longer imports auth.
• "Generated with" line: it is not in the description.

[tee8z]

Approved 762363e against feat/cli-403-credential-source (#66 0a22d4c). This head has the same tree as c66d2a3, which I verified. My three correctness findings on be91ef5 are fixed:

1. Partial writes: put_account sends every secret-write or config-save failure through restore. That includes a write that already renamed its file. restore writes the old secret back. If that fails, it checks whether the old secret is still stored, and then saves the config again. A lost login or config is reported with a hint: voltage login --account NAME or voltage logout --local --account NAME. The write_faults tests cover failures before and after the rename for the secret, for the rollback, and for the config. The put_account lock is the only await, so Ctrl-C cannot stop it between the writes.
2. Store default: --credential-store is now Option. Without it, a replaced login keeps saved.store, and a new login uses the keychain. The integration test replaces a file-stored login without the flag, and checks that it stays file.
3. Ctrl-C: after put_account, a replacement sets SessionChange::RevokeReplaced, so an interrupt during the old session's revoke says that the new login is saved. This is tested with SIGINT during a delayed revoke.

My nits are also fixed: replaced is in the CHANGELOG and login-and-profiles.md, shell_word is now in config with no config → auth import, the description has no attribution line, and the inline suggestions are applied. logout --local warns through discard_secret.

Nonblocking:

• atomic_write holds a #[cfg(test)] fault hook. It doesn't affect release builds, but a seam passed in by the caller would keep test code out of the write path.
• After a config save that fails after its rename, followed by a failed retry, the disk keeps the new account's entry with the old secret. For the same user at the same auth URL, that entry differs only by store, which saved_store_matches checks.

Validation on this tree, in nix develop: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (118 unit, 103 integration) and python3 scripts/check-coverage.py passed. CRAP: 555 measured functions, none at or above 20, highest wait_for_payment at 19.60; 152 functions were unmeasured. CI is green. A working OS keychain and live auth services were not exercised.