@claude review

Claude finished @dubadub's task in 2m 26s —— View job

Code Review

• Gather context and understand the request
• Read src/auth/device_flow.rs
• Read src/auth/device_flow_test.rs
• Read src/auth/mod.rs changes
• Read src/main.rs changes
• Read spec doc
• Analyze and post review feedback

Overall this is a clean RFC 8628 implementation. The separation of the pure interpret_token_response interpreter from the networked poll_for_token loop is good design, and the prefer_device_flow_from testability seam mirrors the repo's existing patterns well. Below are the issues I found, ordered by severity.

Robustness

1. Unbounded interval growth on slow_down — src/auth/device_flow.rs:156

PollOutcome::SlowDown => interval += Duration::from_secs(5),

RFC 8628 says to increase the interval by 5s per slow_down, but doesn't cap it. If a server sends repeated slow_down responses (buggy server or during heavy load), the polling interval could grow to minutes before the code expires, leaving the user staring at dots. A ceiling of, say, 30s is standard practice:

PollOutcome::SlowDown => interval = (interval + Duration::from_secs(5)).min(Duration::from_secs(30)),

Fix this →

2. Zero-interval guard — src/auth/device_flow.rs:137

If the server returns interval: 0, tokio::time::sleep(Duration::from_secs(0)) yields immediately, creating a tight loop for the full expires_in window. Legitimate servers won't do this, but it's inexpensive to guard:

let actual_interval = interval.max(Duration::from_secs(5));
_ = tokio::time::sleep(actual_interval) => {}

Fix this →

Design / duplication

3. /.dockerenv check duplicated in two places

prefer_device_flow() in main.rs:480 and server_host_label() in device_flow.rs:186 both independently call std::path::Path::new("/.dockerenv").exists(). If the detection heuristic ever changes (e.g., checking DOCKER_* env vars as a fallback), it needs updating in two places. One of them should call through to the other, or extract a shared is_running_in_docker() function.

4. Fresh reqwest::Client bypasses CookApi configuration — src/auth/mod.rs:291

let client = reqwest::Client::new();

CookApi has its own client (presumably configured at construction). A fresh client with no custom TLS settings, proxy config, or user-agent loses those. Since the device endpoints are at the site root (not under /api), using a separate client may be intentional, but the inconsistency is worth documenting or, ideally, exposing CookApi::client() / CookApi::base_site_url() and threading it through.

Tests

5. Missing test: --headless flag on non-Linux — src/main.rs:1025

The test headless_flag_always_prefers_device_flow is named "always" but only covers is_linux = true. The flag should also force device flow on macOS/Windows (is_linux = false, has_display = true):

#[test]
fn headless_flag_works_on_non_linux() {
    assert!(prefer_device_flow_from(true, false, false, true));
}

Fix this →

Minor / nits

6. Unreachable None branch — src/auth/mod.rs:359

match self.get_session() {
    Some(session) => println!("Logged in as {}", session.email.unwrap_or(session.user_id)),
    None => println!("Logged in"),   // logically unreachable: set_session just succeeded
}

set_session stores the session before returning; get_session immediately after will always return Some. The None arm is dead code. Just print the success message unconditionally, or call set_session with a ? and then directly display the known email from the SecureSession it just built.

7. Redundant dot_handle.abort() — src/auth/mod.rs:353

cancel.cancel() is called one line earlier (line 352), which causes the dot task to exit via its cancel.cancelled() select arm. The subsequent abort() is harmless but redundant.

Summary

The RFC 8628 state machine is correct, the test coverage is solid for the interpreter and flow-selection matrix, and the Ctrl+C cancellation is idiomatic. The items worth addressing before merge are #1 (unbounded slow_down interval), #3 (duplicated Docker detection), and #5 (incomplete test coverage for the headless flag on non-Linux). The rest are low-priority polish.