Both concerns I raised in my previous review have been genuinely addressed.
Logout revocation — this was the most important fix, and it was done properly rather than just documented. revokeIdentitySession now calls TokenForIdentity first to get a live access token (fast-path if the cached token is still fresh, automatic refresh via /auth/refresh if it isn't), then re-reads the identity from disk to get the refresh token that is actually current (not the one a concurrent refresh may have just spent), and then calls RevokeRefreshToken with a valid bearer. RevokeRefreshToken now also returns an error instead of nil when accessToken == "", which closes the silent-false-success bug CodeRabbit also caught. The new test in logout_revoke_test.go is properly synchronized via a mutex-backed revokeCapture struct and covers both the happy path (correct bearer, correct refresh token) and the failure path (server 500 must not block local logout).
Error message for sourceless identity — the author's pushback was correct. GetIdentitySource does fall back to c.sourcePath for identities in the main config, so main-config token identities work fine. The "" case is genuinely only for in-memory configs that were never loaded from disk (e.g., DecodeConfig on a raw byte slice with no sourcePath), where there's nowhere to write rotated tokens. The new error message ("identity %q has no config file on disk to store refreshed tokens in; run 'miren login' to save it") is clear and actionable. I was wrong on the premise, and I'm satisfied the edge case is now documented rather than silently broken.
The TOCTOU note from my first review (two reads inside the locked section) is still present but remains a minor paper concern — both reads hold the lock, so the only risk is from a writer that deliberately ignores the lock, and persistIdentityTokens needs the full file contents to preserve other fields anyway.
This is ready to merge.
Posted to the PR's comment threads when you submit.
GetIdentitySource now, it clearly falls back to c.sourcePath for main-config identities — the "" case is only for configs that were never loaded from disk at all. The new error message is clear and actionable. Resolved.RevokeRefreshToken also now returns an error for a missing access token instead of silently claiming success. Resolved.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 968,
"verdict": "ready",
"event": "approve",
"summary": "Both concerns I raised in my previous review have been genuinely addressed.\n\n**Logout revocation** — this was the most important fix, and it was done properly rather than just documented. `revokeIdentitySession` now calls `TokenForIdentity` first to get a live access token (fast-path if the cached token is still fresh, automatic refresh via `/auth/refresh` if it isn't), then re-reads the identity from disk to get the refresh token that is actually current (not the one a concurrent refresh may have just spent), and then calls `RevokeRefreshToken` with a valid bearer. `RevokeRefreshToken` now also returns an error instead of `nil` when `accessToken == \"\"`, which closes the silent-false-success bug CodeRabbit also caught. The new test in `logout_revoke_test.go` is properly synchronized via a mutex-backed `revokeCapture` struct and covers both the happy path (correct bearer, correct refresh token) and the failure path (server 500 must not block local logout).\n\n**Error message for sourceless identity** — the author's pushback was correct. `GetIdentitySource` does fall back to `c.sourcePath` for identities in the main config, so main-config token identities work fine. The `\"\"` case is genuinely only for in-memory configs that were never loaded from disk (e.g., `DecodeConfig` on a raw byte slice with no `sourcePath`), where there's nowhere to write rotated tokens. The new error message (`\"identity %q has no config file on disk to store refreshed tokens in; run 'miren login' to save it\"`) is clear and actionable. I was wrong on the premise, and I'm satisfied the edge case is now documented rather than silently broken.\n\nThe TOCTOU note from my first review (two reads inside the locked section) is still present but remains a minor paper concern — both reads hold the lock, so the only risk is from a writer that deliberately ignores the lock, and `persistIdentityTokens` needs the full file contents to preserve other fields anyway.\n\nThis is ready to merge.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6TaodA",
"body": "You're right, I was wrong on the premise. Looking at `GetIdentitySource` now, it clearly falls back to `c.sourcePath` for main-config identities — the `\"\"` case is only for configs that were never loaded from disk at all. The new error message is clear and actionable. Resolved.",
"resolve": true
},
{
"thread_id": "PRRT_kwDONZ__ss6TaodB",
"body": "This is a solid fix — much better than just documenting it. The refresh-before-revoke path works correctly: fast-path for a still-fresh token, proper network refresh otherwise, and then a re-read to get the current refresh token (not one that may already be spent). `RevokeRefreshToken` also now returns an error for a missing access token instead of silently claiming success. Resolved.",
"resolve": true
}
],
"posted_to_pr": true
}miren login ephemeral by defaultThis is a well-designed change that introduces a new "token" identity type with automatic JWT refresh, cross-process locking, and atomic file writes. The core design decisions are sound, the test suite is genuinely good (the subprocess-based concurrency test for the single-use refresh token is particularly useful), and the code comments explain the less obvious choices clearly. That said, there are a couple of concrete issues worth knowing about before merging.
persistIdentityTokens reads-then-writes under the lock, but readIdentityFromFile is a separate passIn token.go, tokenForTokenIdentity holds the flock, re-reads the identity via readIdentityFromFile, and then calls persistIdentityTokens — which reads the file a second time before writing. That's three disk reads total per locked refresh, and between read 2 (inside readIdentityFromFile) and read 3 (inside persistIdentityTokens), a concurrent writer that isn't using the lock (e.g. miren login --force) could change the file. The comment says "the caller must hold the lock", but that invariant is only partially enforced. Minor, but worth noting — the locked re-read already validated freshness, so persistIdentityTokens only needs a write, not another full unmarshal-then-marshal round-trip. Not a blocker by itself, but it's a genuine TOCTOU window inside the locked section.
tokenForTokenIdentity calls c.GetIdentitySource(name) and returns an error if it gets "". GetIdentitySource returns "" for identities that live directly in the main config file (not a leaf). The typical install path (saveTokenIdentityToConfig) always writes token identities as leaf configs in clientconfig.d/, so in practice this won't be hit — but a user who manually edits their clientconfig.yaml or who still has a main-config identity from an old version will see cannot locate config file for identity "cloud" on the first token refresh instead of the real auth error. The error message at token.go line 218 doesn't explain this, so it would be confusing.
RevokeRefreshToken called with a possibly-expired access tokenIn logout.go line 119, identity.Token is passed as the bearer credential. That's the cached access token, which may be expired by the time the user logs out. The server-side revoke endpoint is documented as best-effort and the logout continues regardless, but if the endpoint requires a valid bearer, the revoke will silently fail on every real-world logout (tokens expire). It would be more robust to attempt a refresh before revoking, or to note in the comment that an expired access token is expected here.
tmp.Chmod may not be portableatomicWriteFile calls tmp.Chmod(perm) after writing but before renaming. On Linux this is fine. On macOS it's also fine. But (*os.File).Chmod is not guaranteed by the Go spec to be the same as os.Chmod(path, perm) on all platforms (Windows). This is a CLI tool that presumably runs only on Unix, so it's low risk, but using os.Chmod(tmpPath, perm) would be clearer and portable.
tokenExpiryBuffer constant is a named, tested value rather than a magic number.TestTokenForIdentityConcurrentRefresh test covering both goroutines and subprocesses is exactly what you want for a single-use rotating refresh token.ErrLoginRequired (401 / definitive) and transient errors (5xx / network) is correct and tested.I'm marking this CAVEATS rather than NOT_READY because the main-config identity issue is an edge case that won't affect the default install path, and the access-token-expiry on logout is best-effort by design. But both are worth addressing before shipping.
clientconfig.yaml (not in a leaf under clientconfig.d/), GetIdentitySource returns "" and this error fires. The error message "cannot locate config file for identity" gives no actionable hint. Consider checking c.sourcePath as a fallback, or updating the message to explain that the identity must be in a clientconfig.d/ leaf file for token refresh to work.identity.Token is the cached access token, which may well be expired by the time a user runs miren logout. If the revoke endpoint enforces a valid bearer, this will silently fail on every real-world logout. Since the caller already treats revocation as best-effort and continues anyway, the failure won't block logout — but it means the refresh token is never actually revoked. Consider adding a note in the comment, or attempting a quick in-memory refresh before revoking so the revoke call has a fighting chance to succeed.Verdict: caveats