[sagnik11]

Why

Backend PR Autter-dev/autter-monorepo#513 (merged Oct 17) made the server answer a refused CLI refresh token from memory. That cut invalid_grant refusals from about 2,000–3,600 to about 110–180 per 6 hours. The rest come from about 5 older installs (CLI 1.6.9–2.2.0). Their daemon still sends a dead refresh token on every telemetry flush, so those developers stay logged out and their AI-authorship data never reaches the server.

The loop on the client: the daemon flushes every 3 s. Each flush builds several ApiContext::new(), and each one calls try_load_auth_token(), which refreshes with the stored token. When the refresh fails, the CLI keeps the dead credentials and tries again on the next call. The server sees bursts of 2–10 refreshes every 8–47 s per install.

The server's refusal now (HTTP 400):

{"error":"invalid_grant","error_description":"Refresh token is not valid. Run `autter login` to sign in again.","error_code":"refresh_token_invalid","login_required":true}

error_code is one of refresh_token_invalid, device_revoked, refresh_token_expired or refresh_token_superseded. Older servers send only error: "invalid_grant".

What changed (the four follow-ups from #513)

1. Stop retrying a dead token. When the refresh grant returns a 4xx with error == "invalid_grant" (or login_required: true) for a device refresh token, the CLI saves the refusal with the credentials (refresh_failed: time, error, error_code, error_description). Every process then returns AuthLoadIssue::LoginRequired without a network call until autter login writes new credentials (which never carry the field). If that write fails (disk full, keychain locked), the process remembers the refusal in memory, so the loop can't restart.
2. Tell the user once. The first interactive command after the refusal prints the server's error_description through auth::notice. This happens right away, even inside the 24-hour reminder window, and only once per refusal (a stamp file holds the refusal's timestamp). The usual 24-hour "cloud sync is paused" reminder now names the server's reason. whoami, doctor and debug show a new LoginRefused state. autter login no longer says "already logged in" for refused credentials. The daemon logs the refusal once per process instead of once per queue per flush.
3. Compare-and-set store writes. After a refresh, CredentialStore::store_if_current overwrites the stored credentials only if they still hold the refresh token that was sent. Marking a token refused uses the same check. The wait for credentials.lock goes from 2 s to 10 s before the CLI refreshes without it. This closes the last-writer-wins race when three or more processes refresh at once: the late writers no longer put back an older rotation, and a sibling's refresh_token_superseded doesn't mark a newer, live session as refused.
4. Keep the OAuth error typed. exchange_token and refresh_access_token return TokenRequestError::{Transport, Refused { status, error: OAuthError }} instead of a string. OAuthError now parses error_code and login_required. PAT and install-nonce sign-in keep their string errors for their callers.

Transient failures are retried as before: network errors, non-OAuth error pages (e.g. a proxy 502), any 5xx, temporarily_unavailable and access_denied. An explicit login_required: false makes a refusal non-terminal.

Left as before on purpose: refused PATs. A PAT uses the same refresh grant, but the brief limited the terminal state to device refresh tokens, and the per-org PAT exchange can refuse for reasons that don't mean the PAT is dead.

Tests

New unit tests (all passing):

• OAuth error parsing: the current body, each error_code, the old invalid_grant-only body, login_required true, false and null, transient errors, 4xx versus 5xx, and non-OAuth pages counted as transport errors.
• Dead-token state: one refresh call, then none across repeated loads until a login replaces the credentials, then refreshing resumes. Also covered: old-server refusals, valid access tokens on refused credentials, PATs left non-terminal, and the in-memory fallback when persisting fails.
• Compare-and-set: a newer sibling rotation is kept, a logout isn't brought back, a login landing during a refresh isn't overwritten, a superseded refusal after a sibling rotation uses the sibling's session, and the refusal mark only applies to the refused token.
• One-time notice: announced once per refusal and again for a new one. The daemon logs once per refusal, and per-flush queue warnings skip LoginRequired.

Checks run locally:

• cargo fmt: clean.
• cargo build: clean.
• Full suite as task test runs it (daemon mode, 12 threads): 1,919 lib unit tests passed. Integration: 3,109 passed; 21 timed out waiting on the shared test daemon while the machine was at load average ~57 on 12 cores. All 21 are in utf8_filenames, windsurf and worktrees, and the same harness timeout hit each. Re-running those three modules alone: 150/150 passed. daemon_mode, async_mode and the other test binaries all passed.
• cargo clippy --all-targets -- -D warnings fails on 12 lints from stable 1.99 that are already on main, none in this diff. CI only runs cargo check --all-targets and the Windows installer job.

Rollout: the 5 stuck installs only stop after they update

The fix is client-side, so an install stops sending dead refreshes only once it runs a CLI with this change. Those users still need to run autter login once, which the new CLI tells them to do. Until then the server keeps answering them from #513's refusal cache.

How the release would go (not part of this PR):

1. Bump version in Cargo.toml and Cargo.lock (e.g. 2.2.0 → 2.2.1) in a PR and merge it to main.
2. tag-release.yml tags vX.Y.Z on that merge commit, and release.yml builds the binaries and publishes @autter/cli to npm.
3. Installs pick it up through autter upgrade or the background update check. That check installs automatically unless auto-updates are off. In older versions it only runs on git push. Not verified: whether 1.6.9–2.2.0 restart the daemon on the new binary right after a background upgrade.
4. Optionally, raise the releases payload's min_version to the fixed release so older CLIs show the "below minimum version" warning on interactive commands.
5. After rollout, the #513 Logs Insights query (stats count(*) by tokenHashPrefix, cliVersion, installId) should show refusals only from versions older than the fix.

Not verified

• Not tested against a live server. The tests inject the refresh call with the exact body above.
• Not checked: the interactive notice's terminal output, and the daemon's single log line in a real daemon (task dev).
• Multi-process compare-and-set is covered by in-process tests that simulate a sibling write during a refresh, not by real concurrent processes.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary

Summary generated by Autter.

Why this change

A refresh token that has expired cannot renew the login, but refresh handling needs to recognize that state instead of continuing to treat the credentials as refreshable.

What changes for users

This change adds explicit refresh-token expiry and failure handling, and surfaces login-required status across authentication checks and sync reporting so users can identify when they need to log in again.

Implementation and review notes

• Review the credential and token-response changes in src/auth/types.rs and src/auth/credentials.rs, including compatibility with stored credentials.
• Check how refresh failures propagate through src/api/client.rs and src/auth/client.rs.
• Review the updated auth status and user-facing guidance in doctor, whoami, debug, and sync notices.
• The PR adds focused tests for token and credential behavior.

Changes

• Track refresh-token expiry and distinguish refresh failures that require the user to log in again.
• Propagate the updated auth state through API requests, credential handling, and sync reporting; improve auth status and guidance in CLI commands.
• Breaking changes: no public API or CLI contract change is identified in the supplied diff.

Acceptance Criteria

• Expired refresh credentials are recognized as requiring login rather than being treated as refreshable.
• Auth status and sync guidance reflect the login-required state in the affected CLI flows.
• Existing credential and token handling tests pass.

Test Plan

• Run task test and confirm the auth-related tests pass.
• With credentials whose refresh token is expired, exercise an authenticated CLI operation and confirm it does not repeatedly attempt refresh and reports that login is required.
• Run autter whoami, autter doctor, and autter debug with the expired credentials and check that their auth status is consistent.
• Complete a fresh login and verify that authenticated operations can proceed with the new credentials.

Rollback Plan

Revert this PR to restore the previous refresh and auth-status handling. The supplied diff does not indicate a database schema migration requiring a separate rollback.

Related Issues

• CE-20

^{Written for commit 9fcde40. Summary will update on new commits.}

[autter-dev]

⏳ Still checking: nothing blocks merging so far

The code review is done. Autter is now running the build and tests and trying the change like a user.

• 🔄 Re-run review

What we checked

⚠️ Code review: Found 5 things worth fixing. Nothing that blocks merging.
⏳ Build and tests: Waiting to run.
⏳ Tried it like a user: Running now.

What to fix

1. Should fix: Missing security-team review on sensitive path (src/auth/credentials.rs:4)
   This path is in a security-sensitive area (matches auth/) but no approving review came from the configured security team.
2. Should fix: Possible non-atomic read-modify-write (src/auth/credentials.rs:191)
   mark\_refresh\_failed loads the stored credentials, checks that the refresh token still matches, then writes back the modified copy.
3. Should fix: Sensitive data exposure (src/commands/whoami.rs:58)
   Sensitive data (credential, internal id, or user secret) flows into a client-bound response or log line without redaction.
4. Should fix: Missing security-team review on sensitive path (src/auth/client.rs:2)
   This path is in a security-sensitive area (matches auth/) but no approving review came from the configured security team.
5. Should fix: Missing security-team review on sensitive path (src/auth/state.rs:10)
   This path is in a security-sensitive area (matches auth/) but no approving review came from the configured security team.

_{Plus 9 minor suggestions (style, conventions, hardening) in the sections below. They don't block merging.}

---

🧭 Trying it like a user: running

Autter is trying this change the way a user would. This section updates when it finishes.

💡 More findings (13): suggestions and ones Autter couldn't confirm

🧭 PR hygiene & process suggestions

Autter has 1 suggestion(s) about the shape of this PR (size, scope, reviewability). These are process guidance — not code defects — so they are consolidated here instead of posted as inline comments on individual files.

🟡 Change not tied to any stated requirement — Risk: 25/100

No stated requirement asked for this: The credentials.lock wait is lengthened from about 2 s (10 × 200 ms) to 10 s with 100 ms polling and a deadline. The intent does not ask for this timing change, though it is related to the refresh race.. Confirm it belongs in this PR rather than a separate change.

🛠 AI fix prompt (copy & paste into your coding agent)

Confirm this change is in scope for the PR's stated intent. If it is unrelated, split it into its own PR; if it is required, note why in the PR description.

_{Flagged by Autter PR-hygiene checks.}

---

💡 2 minor suggestions: conventions, hardening and tidiness (not bugs)

Comments on the code are kept for real bugs. These are things worth knowing that the change doesn't prove wrong: a missing safety layer, a convention the repo usually follows, a file that usually changes together with one you touched. Take what fits and ignore the rest. They don't block merging.

• 🟠 src/commands/autter_handlers.rs usually changes with this file (risk 51/100) — src/commands/debug.rs:337
• 🟡 Refresh retry orchestration is dense (complexity ~14 in refresh_stored_credentials) (risk 45/100) — src/api/client.rs:126

❔ 10 possible problems Autter couldn't confirm

Autter's checks flagged these, but a second look couldn't confirm them from the code it could see, so they are listed here instead of on the code. Check the ones that look real to you. They don't block merging.

• 🟡 Missing security-team review on sensitive path (risk 40/100) — src/auth/client.rs:2 — This is a process/policy finding about whether the configured security team approved the PR. The provided evidence contains no review/approval metadata, so neither presence nor absence of a security review can be verified from code. The path does match auth/, and the change touches token refresh/exchange logic.
• 🟡 Missing security-team review on sensitive path (risk 30/100) — src/auth/notice.rs:18 — This is a process/policy check. The provided evidence does not show whether a security-team approval exists. The changed code in src/auth/notice.rs is user-facing stderr messaging: it reads refresh_failed from the stored credentials and prints the refusal message. That limits the actual security sensitivity, but the review status still cannot be confirmed or ruled out.
• 🟡 Source changes without matching tests (risk 35/100) — src/api/client.rs:1 — _The detector only looks for changes under separate test files such as tests/config_fresh_test.rs. This Rust codebase keeps unit tests in in-file #[cfg(test)] modules. The changed-symbol list includes what look like test helpers (make_credentials in src/auth/types.rs, test_fallback_path in src/auth/credentials.rs), which suggests in-file tests were added or updated in this PR. The test _
• 🟡 Log injection (risk 35/100) — src/api/client.rs:245 — The value is logged as a structured tracing field from failure.message(), which may carry the server's error_description. Whether control characters can forge log lines depends on RefreshFailure::message() and the tracing formatter, neither of which is in the provided code. The server is also the trusted OAuth endpoint, so exploitability is doubtful, but I cannot prove it either way.
• 🟡 Missing security-team review on sensitive path (risk 40/100) — src/auth/state.rs:10 — This is a process/policy finding: it claims no approving review came from the configured security team. Review approval state is not part of the provided evidence. The path does sit under auth/ and the change alters login-state handling. Whether a security-team approval exists cannot be verified from the code.
• 🟡 Missing security-team review on sensitive path (risk 49/100) — src/auth/credentials.rs:4 — This is a process-policy finding about which reviews the PR received. Review and approval state is not part of the provided evidence, so I cannot verify whether a security-team approval exists. The changed file is under auth/, so the policy match itself is plausible.
• 🟡 Possible non-atomic read-modify-write (risk 49/100) — src/auth/credentials.rs:191 — The method has a load/check/store sequence, but the provided excerpts do not show its callers or establish that a concurrent login can write between those operations without a coordinating lock. The concurrency claim therefore cannot be confirmed from the supplied evidence.
• 🟡 Source changes without matching tests (risk 30/100) — src/file_changes/mod.rs:107 — The excerpt only shows one changed line in src/file_changes/mod.rs: auth_issue() was replaced with auth_issue_to_log(), and the result is used purely for log output. The PR touches 12 files, so tests may well have been added or updated in files not shown here, and the claim that no test changed cannot be verified from this evidence. The blast-radius list is also overstated for this hunk, since
• 🟡 Missing security-team review on sensitive path (risk 30/100) — src/auth/types.rs:16 — This is a process/policy check about who approved the PR. Neither the diff nor the provided context shows review approvals, so it cannot be confirmed or refuted from the code. The change itself only adds a non-secret refresh_failed marker, and Debug keeps the tokens redacted, so a risk of 100 is overstated.
• 🟡 Code correctness issue (risk 10/100) — src/auth/types.rs:54 — The LOGGED_REFUSAL_AT and notice-stamp logic that supposedly dedups by the at timestamp is not in the provided code, so the dedup mechanism cannot be verified. The scenario is also implausible in practice. A new login goes through the interactive device flow (run_device_login). The new token would then have to be refused within the same wall-clock second as the previous refusal. That makes t

🔎 8 low-signal finding(s) (test-file conventions, hardening advice on internal routes, packages the registry knows) were kept off this PR — see the Autter dashboard.

🚦 Pre-merge checks: 8 worth a look, 155 passed

Check	Status	Explanation
Too many lines changed	⚠️ Worth a look	This PR changes 1220 lines (limit 500 lowered from 1000). Splitting into smaller, focused PRs makes review more reliable. This PR touches high-impact code whose failure cascades downstream: src/api/client.rs (blast 66, fan-in 25), so the bar for review is lower.
Log injection	⚠️ Worth a look	1 potential issue(s) detected (max risk 67/100): src/api/client.rs:245.
Possible non-atomic read-modify-write	⚠️ Worth a look	1 potential issue(s) detected (max risk 49/100): src/auth/credentials.rs:191.
Missing security-team review on sensitive path	⚠️ Worth a look	7 root-cause issue(s) detected (max risk 100/100): src/auth/credentials.rs:4, src/auth/client.rs:2, src/auth/notice.rs:18, src/auth/state.rs:10, src/auth/types.rs:16.
Sensitive data exposure	⚠️ Worth a look	2 root-cause issue(s) detected (max risk 49/100): src/commands/whoami.rs:58.
Code correctness issue	⚠️ Worth a look	1 finding(s) on changed lines.
Co-change coupling	⚠️ Worth a look	1 file(s) changed without a partner they usually change with.
Complexity Guard	⚠️ Worth a look	1 finding(s) on changed lines.

✅ 155 passed

Too many files changed · Too many unrelated chapters · Generated files hiding real changes · Missing PR context · Mixed concerns (refactor + behavior change) · Migration + app logic + UI combined in one PR · Sensitive data in logs · Missing audit logging · Removed observability · Silent exception swallowing · Unhandled promise rejection · Circuit breaker not detected · Stack trace leakage · Multi-write without detected transaction · Possible TOCTOU in critical path · Idempotency key not detected · Optimistic locking not detected · Rate limiting not detected · Rate limiting removed · Batch size limit not detected · Pagination not detected · Prompt injection risk · LLM output used in a dangerous sink · Sensitive data in prompt or system-prompt leakage · Over-privileged LLM tool / excessive agency · Missing validation on an LLM-driven decision · Unbounded LLM usage (denial-of-wallet) · Table exposed without row-level security · Over-broad row-level security policy · Code path that bypasses row-level security · Privileged database credential reachable from the client · Privileged query without row-level scoping · Row-level security enabled with no policy · Template-default gradient styling · Interchangeable AI marketing copy · Placeholder content shipped to users · Emoji standing in for an icon system · Call-to-action that goes nowhere · Templated page composition · Color outside the brand palette · Hardcoded style bypassing design tokens · Typography outside the brand type system · One-off UI instead of the shared component · Copy that does not match brand voice · Screen does not match the rest of the product · Merge-blocking marker left in the change · Known-defect marker shipped in code · Untracked TODO without an issue reference · Test disabled or left pending · PII in logs · PII or internals leaked in error response · PII stored without application-level encryption · User data stored without retention controls · PII sent to external / cross-border destination · Lockfile resolution / integrity tampered · Dependency runs install-time lifecycle script · Possible dependency-confusion attack · Lockfile resolves a dependency the manifest does not declare · Checked-in build artefact modified without source change · Dockerfile build-step is insecure · External artefact pulled in without integrity pinning · Changed export, importer not updated · Missing linked tracker issue · Missing CODEOWNERS reviewer approval · Migration missing rollback / down step · Frontend importing database client directly · Route handler bypassing service layer · Backend service importing UI module · Cross-context internals import · Workspace package rule violation · Inconsistent logging pattern · Inconsistent error handling · Endpoint missing input validation · Multi-write without transaction wrapper · New feature shipped without feature flag · Module placed in the wrong workspace package · Hallucinated import (package not installed) · Nonexistent package (not found in registry) · Call to function that does not exist · Generic placeholder identifier in production logic · Repetitive boilerplate (duplicated block) · Overbroad try/catch swallowing all exceptions · TODO / FIXME on critical path · Comment contradicts or fabricates code behaviour · Abstraction defined but never used · Code style differs from rest of codebase · Established pattern ignored · Unhandled edge case (null / empty / zero / boundary) · Insecure training-data / doc-example pattern · Dead code (defined but never referenced) · Deprecated API call · API pattern from wrong library version · API endpoint removed · HTTP method changed (GET ↔ POST etc.) · New required field added to request · Field removed from response schema · Response field type changed · HTTP status code changed · Auth requirement added / removed / changed · Error response shape changed · Pagination behaviour changed · Outbound webhook payload schema changed · GraphQL field removed without deprecation · GraphQL enum value removed · Connected repositories · Vendor API consumer · SQL injection · Cross-site scripting (XSS) · Path traversal · Command injection · Insecure deserialization · Weak cryptography · Hardcoded secret · Insecure randomness for security material · Unsafe file upload · Missing input validation · Unsafe CORS configuration · Unsafe / open redirect · Missing CSRF protection · Unsafe cookie / session settings · Invisible Unicode / Trojan Source characters · API key in source · Access token in source · Private key in source · Database connection URL with embedded credentials · Cloud credential in source · Webhook signing secret in source · OAuth client secret in source · JWT signing secret in source · Hardcoded password · Auth middleware removed from route · Route protection changed (protected → public) · Permission / RBAC check removed · Required role weakened · Admin-only route exposed to lower privilege · Token validation skipped in middleware chain · JWT verification weakened or changed · Session expiration / TTL changed · Password reset flow changed · Webhook endpoint missing signature verification · Public route touches private/PII data · Runtime error risk · Resource leak risk · Data integrity risk · Maintainability issue · Redundant alias / duplicate import · Redundant type construct · Unnecessary type assertion · Module smell · Dead export (no callers) · Code duplication / DRY violation · Scanner suppressions added · Scanner settings changed · Release Notes Curator · Intent vs. implementation

🤖 Release Notes Curator

Release impact: patch. This is a bug fix: Autter stops retrying a refresh token that has already expired, and users with a dead login get a clear prompt to sign in again instead of repeated failed refresh attempts. No public interface is removed or changed incompatibly, and no new capability is added.

Changelog entry:

Fixed: Autter no longer keeps trying to refresh a login that can no longer be renewed. When your session has expired, you'll now see a clear message asking you to sign in again with autter login, and cloud sync will pause until you do.

_{Custom agent · runs after review · configured in Autter}

📄 What this PR changes

This change adds explicit refresh-token expiry and failure handling, and surfaces login-required status across authentication checks and sync reporting so users can identify when they need to log in again.

• Review the credential and token-response changes in src/auth/types.rs and src/auth/credentials.rs, including compatibility with stored credentials.
• Check how refresh failures propagate through src/api/client.rs and src/auth/client.rs.
• Review the updated auth status and user-facing guidance in doctor, whoami, debug, and sync notices.
• The PR adds focused tests for token and credential behavior.

📖 What these words mean

• Must fix: Blocks merging. Autter is confident this is a real problem in this PR.
• Should fix: Doesn't block merging, but is likely a real problem worth fixing.
• Minor: Style, conventions or hardening ideas. Fine to skip.
• Simulated users: Autter runs your app, package or CLI the way people would, each with a different habit (follows the docs, mistypes a command, …).
• Already broken before this PR: The same failure happens on the code before this PR, so this PR didn't cause it.
• We couldn't tell: Some runs failed, but not clearly enough to blame this PR.
• Doesn't block merging: Shown for your information; the merge check ignores it.

_{View this PR in Autter · Checked commit 9fcde40 · Tick Re-run review to check this commit again}