fix(auth): continue login when browser launch fails by cloud9c · Pull Request #38 · wvdsh/cli

cloud9c · 2026-06-25T18:23:12Z

Summary

wavedash auth login no longer aborts when the OS browser launcher fails in headless/SSH environments. The CLI now prints a manual URL and keeps the local callback server running so auth can finish from another browser.

Before:

Opening browser for authentication...
Auth URL: https://wavedash.com/auth/cli?callback_uri=...&state=...
Authentication failed: Launcher "gio" "open" "..." failed with ExitStatus(unix_wait_status(512))

After browser-launch failure:

Opening browser for authentication...
Couldn't open a browser automatically.
Open this URL to continue: https://wavedash.com/auth/cli?callback_uri=...&state=...
Waiting for authorization...

After the user completes the browser flow, login continues normally:

✓ Successfully authenticated!

Verification

rustfmt --check src/auth.rs
doppler run --project cli --config dev -- cargo test
Headless Ubuntu Docker E2E: shimmed Linux browser launchers to fail, confirmed the fallback URL printed once, callback server stayed up, callback completed, and credentials were written.

claude · 2026-06-25T18:24:36Z

Claude finished @cloud9c's task in 1m 24s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types
Post review

This is a clean, well-targeted fix. The login flow no longer aborts when open::that fails (the common headless/SSH case) — it prints a fallback URL and keeps the local callback server running. 👍

What's good

The refactor into login_with_browser_using<F> with an injected open_auth_url closure is a nice, minimal seam for testing without pulling in a mocking framework.
The closure signature FnOnce(&str) -> std::io::Result<()> matches open::that exactly, so the production path stays a straight delegation.
The test is genuinely solid: it correctly handles the startup race (the closure fires before Server::http binds) via the retry-with-deadline loop in send_callback_request, and asserts both the 302 redirect and the returned credentials.

Minor / optional (left as an inline comment on src/auth.rs:150-155)

The underlying browser-launch error is discarded. Surfacing it in the stderr message (Couldn't open a browser automatically ({err}).) would help diagnose odd environments without harming the UX.
Auth URL: {} (printed unconditionally) and Open this URL to continue: {} (on failure) print the same URL twice in the failure path — slightly redundant.

No correctness or security concerns. Nice change.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:25:54Z

    println!("Opening browser for authentication...");
    println!("Auth URL: {}", auth_url);
-    open::that(&auth_url)?;
+    if open_auth_url(&auth_url).is_err() {
+        eprintln!("Couldn't open a browser automatically.");
+        eprintln!("Open this URL to continue: {}", auth_url);
+    }


Optional: two things worth considering here.

The underlying error is fully discarded. The original failure detail (e.g. Launcher "gio" "open" ... failed with ExitStatus(...)) can be useful when diagnosing why a browser won't open in unusual environments. Consider logging it to stderr at a lower priority so the friendly message stays primary but the cause is still recoverable:

Suggested change

println!("Opening browser for authentication...");

println!("Auth URL: {}", auth_url);

open::that(&auth_url)?;

if open_auth_url(&auth_url).is_err() {

eprintln!("Couldn't open a browser automatically.");

eprintln!("Open this URL to continue: {}", auth_url);

}

println!("Opening browser for authentication...");

if let Err(err) = open_auth_url(&auth_url) {

eprintln!("Couldn't open a browser automatically ({err}).");

eprintln!("Open this URL to continue: {}", auth_url);

}

The Auth URL: {} line (printed unconditionally just above) and the Open this URL to continue: {} fallback now print the same URL twice on failure. Dropping the unconditional Auth URL: print — as in the suggestion above — removes the redundancy while keeping the URL visible in the success path via the browser itself. Adjust to taste if the always-on Auth URL: line is intentional.

claude · 2026-06-25T18:26:16Z

Claude finished @cloud9c's task in 1m 13s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types (open = "5.0", open::that → io::Result<()>)
Post review

The change is correct and well-targeted. Login no longer aborts with ? when open::that fails (the common headless/SSH case) — it now prints a fallback URL and proceeds to start the local callback server, so the user can complete auth by opening the URL manually. The control flow into Server::http and Waiting for authorization... is unaffected on the success path.

Note: this revision is the minimal fallback (if open::that(...).is_err()) rather than the closure-injected login_with_browser_using<F> + test that an earlier review thread described. For a fix this small the minimal version is reasonable and more readable — though it does mean the behavior isn't covered by a test.

Minor / optional (left inline at src/auth.rs:144-148):

The underlying launch error is dropped by .is_err(). Surfacing it (Couldn't open a browser automatically ({err}).) aids diagnosing unusual environments without harming UX.
Auth URL: (line 144, unconditional) and Open this URL to continue: (line 147) print the same URL twice on the failure path — slightly redundant.

No correctness or security concerns. 👍
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:27:28Z

    println!("Auth URL: {}", auth_url);
-    open::that(&auth_url)?;
+    if open::that(&auth_url).is_err() {
+        eprintln!("Couldn't open a browser automatically.");
+        eprintln!("Open this URL to continue: {}", auth_url);
+    }


Optional (non-blocking) — the fix is correct as-is. Two small refinements:

The launch error is discarded by .is_err(). The original detail (e.g. Launcher "gio" "open" ... ExitStatus(...)) is useful when diagnosing odd environments. Capturing it keeps the friendly message primary while preserving the cause.

The URL prints twice on the failure path — Auth URL: (line 144, unconditional) and Open this URL to continue: (line 147). Folding the unconditional print into the success/failure branches removes the redundancy.

Suggested change

println!("Auth URL: {}", auth_url);

open::that(&auth_url)?;

if open::that(&auth_url).is_err() {

eprintln!("Couldn't open a browser automatically.");

eprintln!("Open this URL to continue: {}", auth_url);

}

println!("Opening browser for authentication...");

if let Err(err) = open::that(&auth_url) {

eprintln!("Couldn't open a browser automatically ({err}).");

eprintln!("Open this URL to continue: {}", auth_url);

}

claude · 2026-06-25T18:28:48Z

Claude finished @cloud9c's task in 1m 13s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types (open::that → std::io::Result<()>)
Post review

The change is correct and well-targeted. Login no longer aborts via ? when open::that fails — the common headless/SSH case — and instead prints a fallback URL and proceeds to start the local callback server (Server::http → Waiting for authorization...). The success path into incoming_requests() is unaffected.

Resolved since the earlier reviews 👍

The double-print is gone: the unconditional Auth URL: {} line was removed, so the URL now prints exactly once, on the failure path.

Minor / optional (one inline comment at src/auth.rs:144)

.is_err() drops the underlying launch error. Surfacing it (Couldn't open a browser automatically ({err}).) aids diagnosing unusual environments without harming UX. Fix this →

Note: This is the minimal if open::that(...).is_err() form rather than a closure-injected, unit-tested seam. For a fix this small the minimal version is reasonable and more readable, but the new fallback branch isn't covered by a test.

No correctness or security concerns. Nice change.

Note: I couldn't run cargo check/clippy here — Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:29:59Z

    println!("Opening browser for authentication...");
-    println!("Auth URL: {}", auth_url);
-    open::that(&auth_url)?;
+    if open::that(&auth_url).is_err() {


Optional (non-blocking): .is_err() discards the underlying launch error (e.g. Launcher "gio" "open" ... ExitStatus(...)), which is useful for diagnosing odd headless/SSH environments. Capturing it keeps the friendly message primary while preserving the cause:

Suggested change

if open::that(&auth_url).is_err() {

if let Err(err) = open::that(&auth_url) {

eprintln!("Couldn't open a browser automatically ({err}).");

eprintln!("Open this URL to continue: {}", auth_url);

}

claude · 2026-06-25T18:37:25Z

Claude finished @cloud9c's task in 1m 4s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types (open = "5.0", open::that → std::io::Result<()>)
Post review

The change is correct and well-targeted, and the latest revision has incorporated the feedback from the earlier review threads. 👍

println!("Opening browser for authentication...");
if let Err(error) = open::that(&auth_url) {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("  {}", error);
    eprintln!("Open this URL to continue: {}", auth_url);
}

What's good

Login no longer aborts via ? when open::that fails (the common headless/SSH case). The control flow falls through to Server::http → Waiting for authorization..., so the user can complete auth by opening the printed URL manually. Success path is unaffected.
Underlying error is now surfaced (eprintln!(" {}", error)) — the earlier reviews flagged that .is_err() discarded the cause; this revision preserves it on its own indented line while keeping the friendly message primary. error is an io::Error, so Display is appropriate here.
Double-print is resolved — the unconditional Auth URL: {} line is gone, so the URL now prints exactly once, only on the failure path.

Minor / optional (non-blocking)

The diagnostics go to stderr (eprintln!) while Opening browser… / Waiting for authorization… go to stdout (println!). That's a reasonable split (errors on stderr), just noting the streams differ in case output ordering matters when piped.
The new fallback branch still isn't covered by a test. For a fix this small that's a fair trade-off, but a closure-injected seam (as discussed in an earlier thread) would let you assert the "browser fails → server still starts" path if you want regression coverage later.

No correctness or security concerns. Nice change.

Note: I couldn't run cargo check/clippy here — the Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

cursor · 2026-06-25T18:41:15Z

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

claude · 2026-06-25T18:41:31Z

Claude finished @cloud9c's task in 1m 11s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types (open = "5.0", open::that → std::io::Result<()>)
Post review

The core fix is correct and well-targeted. Login no longer aborts via ? when open::that fails (the common headless/SSH case) — it falls through to Server::http and keeps the local callback server running, so the user can finish auth by opening the printed URL manually. The earlier review feedback is all incorporated: the underlying error is now surfaced on its own line, and the redundant double-print of the URL is gone.

println!("Opening browser for authentication...");
if let Err(error) = open::that(&auth_url) {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("  {}", error);
    eprintln!("Open this URL to continue: {}", auth_url);
}

⚠️ One thing to flag — likely unintentional regression

The diff also removed the println!("Waiting for authorization...") line that previously sat just before the for request in server.incoming_requests() loop:

     let server = Server::http(format!("127.0.0.1:{}", port))
         .map_err(|e| anyhow::anyhow!("Failed to start server: {}", e))?;
-    println!("Waiting for authorization...");

After this change the CLI prints nothing while it blocks waiting for the callback — the user is left at a frozen prompt with no feedback. This is especially relevant in the exact scenario this PR targets (browser didn't open → user has to go open the URL by hand → they really need to know the CLI is still waiting for them).

It also contradicts the PR's own After example, which still lists Waiting for authorization... as expected output. That strongly suggests the removal was accidental. I left an inline suggestion at src/auth.rs:148-150 to restore it. Fix this →

Minor / optional (non-blocking)

Diagnostics go to stderr (eprintln!) while Opening browser… goes to stdout (println!). Reasonable split (errors on stderr), just noting the streams differ if output ordering matters when piped.
The new fallback branch isn't covered by a test. For a fix this small that's a fair trade-off; a closure-injected seam would let you assert "browser fails → server still starts" if you want regression coverage later.

No correctness or security concerns with the fallback logic itself. The only real action item is restoring the Waiting for authorization... message.

Note: I couldn't run cargo check/clippy here — the Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:42:39Z

+    }

    let server = Server::http(format!("127.0.0.1:{}", port))


This diff also removed println!("Waiting for authorization...") that previously sat just before the for request in server.incoming_requests() loop. After this change the CLI prints nothing while it blocks on the local callback server — the user is left staring at a frozen prompt with no indication anything is happening.

Notably, the PR description's own After example still lists Waiting for authorization... as expected output, so this removal looks unintentional. Suggest restoring it after the server binds:

Suggested change

}

let server = Server::http(format!("127.0.0.1:{}", port))

}

let server = Server::http(format!("127.0.0.1:{}", port))

.map_err(|e| anyhow::anyhow!("Failed to start server: {}", e))?;

println!("Waiting for authorization...");

claude · 2026-06-25T18:49:18Z

Claude finished @cloud9c's task in 56s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code
Verify dependencies & types (open = "5.0", open::that → std::io::Result<()>)
Post review

The change is correct and well-targeted, and this revision has folded in the feedback from every earlier review thread. 👍

println!("Opening browser for authentication...");
if let Err(error) = open::that(&auth_url) {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("  {}", error);
    eprintln!("Open this URL to continue: {}", auth_url);
}

let server = Server::http(format!("127.0.0.1:{}", port))
    .map_err(|e| anyhow::anyhow!("Failed to start server: {}", e))?;
println!("Waiting for authorization...");

What's good

Login no longer aborts via ? when open::that fails (the common headless/SSH case). Control falls through to Server::http → Waiting for authorization..., so the user can complete auth by opening the printed URL manually. The success path is unaffected.
Underlying error is surfaced (eprintln!(" {}", error)) on its own indented line, keeping the friendly message primary while preserving the diagnostic cause. error is an io::Error, so Display is appropriate.
Double-print is resolved — the unconditional Auth URL: {} line is gone, so the URL now prints exactly once, only on the failure path.

On the earlier "regression" flag — resolved/not present ✅

An earlier review thread (18:42Z) flagged that println!("Waiting for authorization...") had been removed. In the current HEAD (949e562) that line is still present at src/auth.rs:152, right after the server binds and before the incoming_requests() loop — confirmed against the actual diff (git show 949e562). So there's no missing-feedback issue here; the user is informed the CLI is still waiting. Nothing to do.

Minor / optional (non-blocking)

The diagnostics go to stderr (eprintln!) while Opening browser… / Waiting for authorization… go to stdout (println!). That's a sensible split (errors on stderr) — just noting the streams differ in case output ordering matters when piped.
The new fallback branch isn't covered by a test. For a fix this small that's a fair trade-off; a closure-injected seam (as floated in an earlier thread) would let you assert "browser fails → server still starts" if you want regression coverage later.

No correctness or security concerns. Nice change. 🚢

Note: I couldn't run cargo check/clippy here — the Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:51:48Z

Claude finished @cloud9c's task in 1m 19s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code (src/auth.rs)
Verify dependencies & types (open = "5.0", open::that → std::io::Result<()>)
Post review

The core fix is correct and well-targeted. Login no longer aborts via ? when open::that fails — the common headless/SSH case. Control falls through to Server::http → Waiting for authorization..., so the user can complete auth by opening the printed URL manually. The success path is unaffected.

println!("Opening browser for authentication...");
if open::that(&auth_url).is_err() {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("Open this URL to continue: {}", auth_url);
}

let server = Server::http(format!("127.0.0.1:{}", port))
    .map_err(|e| anyhow::anyhow!("Failed to start server: {}", e))?;
println!("Waiting for authorization...");

What's good

The redundant unconditional Auth URL: {} print is gone — the URL now appears exactly once, only on the failure path.
Waiting for authorization... (src/auth.rs:151) is present after the server binds, so the user still gets feedback while the callback server blocks. The earlier "missing Waiting…" regression flagged in a prior review thread is not present in the current HEAD (084a43b).

One thing to flag — diverges from the PR's own "After" example ⚠️

The current code uses .is_err(), which discards the underlying launch error. So it prints just two lines, whereas the PR description's After block shows the cause on its own indented line:
```
Couldn't open a browser automatically.
  Launcher "xdg-open" "https://..." failed with ExitStatus(unix_wait_status(512))   ← missing in current code
Open this URL to continue: https://...
```
Surfacing the error keeps the friendly message primary while preserving the diagnostic cause (helpful for unusual environments) and makes the output match the PR description. Inline suggestion left at src/auth.rs:144-147. Fix this →

Minor / optional (non-blocking)

Diagnostics go to stderr (eprintln!) while Opening browser… / Waiting for authorization… go to stdout (println!). A sensible split (errors on stderr) — just noting the streams differ in case output ordering matters when piped.
The new fallback branch isn't covered by a test. For a fix this small that's a fair trade-off; a closure-injected seam would let you assert "browser fails → server still starts" if you want regression coverage later.

No correctness or security concerns. Nice change. 🚢

Note: I couldn't run cargo check/clippy here — the Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job, and git fetch is restricted. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T18:52:58Z

+    if open::that(&auth_url).is_err() {
+        eprintln!("Couldn't open a browser automatically.");
+        eprintln!("Open this URL to continue: {}", auth_url);
+    }


.is_err() discards the underlying launch error, so the current code prints:

Couldn't open a browser automatically. Open this URL to continue: https://...

But the PR's own After example shows the error cause on its own indented line:

Couldn't open a browser automatically. Launcher "xdg-open" "https://..." failed with ExitStatus(unix_wait_status(512)) Open this URL to continue: https://...

The indented cause line is missing because .is_err() drops the error. Surfacing it aids diagnosing odd headless/SSH environments and matches the PR description:

Suggested change

if open::that(&auth_url).is_err() {

eprintln!("Couldn't open a browser automatically.");

eprintln!("Open this URL to continue: {}", auth_url);

}

if let Err(error) = open::that(&auth_url) {

eprintln!("Couldn't open a browser automatically.");

eprintln!(" {}", error);

eprintln!("Open this URL to continue: {}", auth_url);

}

claude · 2026-06-25T21:46:11Z

Claude finished @cloud9c's task in 2m 38s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code (all 9 files)
Verify dependencies & types
Post review

This PR is broader than its title — beyond the auth fallback it lands the "agent-friendly commands" commit (--json everywhere, --no-color, --no-update-check, headless detection, scripted init, --token-stdin). The earlier review threads only looked at src/auth.rs; this pass covers the rest.

The work is solid and consistent: every mutating command grows a --json flag that short-circuits to serde_json::to_string_pretty and suppresses the update check, progress output is correctly silenced in JSON mode (src/uploader.rs makes the ProgressReporter optional — clean), and the README documents the automation surface well.

Findings

1. handle_stat_update now parses the response on the default path — src/stats.rs:92 (inline comment)
let stat: Stat = resp.json().await? runs unconditionally, before the json_output check. Previously this command only called check_api_response. If the update endpoint returns an empty/different body, the non-JSON path now fails where it used to succeed. Suggest moving the parse inside the if json_output branch (as stat_create is unaffected since it already parsed). Fix this →

2. Headless bail vs. the new auth fallback layering — src/main.rs:620 (inline comment)
is_browser_login_unavailable() hard-bails before login_with_browser() runs in CI/SSH/headless-Linux. So the new "print manual URL + keep callback server up" fallback in auth.rs is only reachable when the heuristic says a browser is available but open::that still fails (the broken-gio-with-DISPLAY case from the description). Reasonable split — just flagging the title describes the narrower path, and the fallback can't be exercised by setting CI=1.

3. src/auth.rs:144 — .is_err() drops the launch error (already raised across the earlier threads)
The current code prints two lines; the PR description's After block shows the underlying cause on its own indented line. Surfacing it aids diagnosing odd environments and matches the description:

if let Err(error) = open::that(&auth_url) {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("  {}", error);
    eprintln!("Open this URL to continue: {}", auth_url);
}

Non-blocking; the fix is correct as-is. Waiting for authorization... is present after the server binds, so the user still gets feedback.

Minor / optional (non-blocking)

InitArgs::is_interactive() returns false if any flag is set, including --json or --force alone. So wavedash init --force (wanting an interactive overwrite) routes to the scripted path and bails requiring --team-*/--game-*. Probably fine, just slightly surprising.
No test covers the new fallback branch or the JSON outputs. For a fix this size that's a fair trade-off; a closure-injected seam (floated earlier) would let you assert "browser fails → server still starts."

No correctness or security concerns beyond the stat_update regression in finding #1, which is the only real action item.

Note: I couldn't run cargo check/clippy — the Doppler env (per CLAUDE.md, doppler run -- cargo ...) isn't available in this job. Findings are from static review of the diff.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T21:46:31Z

Claude finished @cloud9c's task in 59s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code (src/auth.rs)
Verify dependencies & types (open = "5.0", open::that → std::io::Result<()>)
Post review

The fix is correct and well-targeted. Replacing open::that(&auth_url)? with a non-fatal if open::that(&auth_url).is_err() means login no longer aborts when the OS browser launcher fails — the common headless/SSH case. Control falls through to Server::http → Waiting for authorization..., so the user can finish auth by opening the printed URL manually. The success path is unchanged.

println!("Opening browser for authentication...");
if open::that(&auth_url).is_err() {
    eprintln!("Couldn't open a browser automatically.");
    eprintln!("Open this URL to continue: {}", auth_url);
}

let server = Server::http(format!("127.0.0.1:{}", port))
    .map_err(|e| anyhow::anyhow!("Failed to start server: {}", e))?;
println!("Waiting for authorization...");

What's good

The behavior matches the PR's After example exactly — three lines on failure, then Waiting for authorization....
The redundant unconditional Auth URL: {} print was removed, so the URL now shows exactly once, only on the failure path.
Waiting for authorization... (src/auth.rs:151) is still present after the server binds, so the user gets feedback while the callback server blocks. (An earlier review thread flagged this as accidentally removed — it is present in the current HEAD 084a43b, so there's no regression.)

Minor / optional (non-blocking)

.is_err() discards the underlying launch error (e.g. Launcher "gio" "open" … ExitStatus(…)). Surfacing it on its own indented line would aid diagnosing unusual environments without harming UX — but the current two-line output already matches the PR description, so this is purely a taste call. Fix this →
Diagnostics go to stderr (eprintln!) while Opening browser… / Waiting for authorization… go to stdout (println!). A sensible split (errors on stderr) — just noting the streams differ if output ordering matters when piped.
The new fallback branch isn't covered by a test. For a fix this small that's a fair trade-off; a closure-injected seam (as floated in earlier threads) would let you assert "browser fails → server still starts" if you want regression coverage later.

No correctness or security concerns. Nice change. 🚢

Note: I couldn't run cargo check/clippy here — the Doppler env (per CLAUDE.md, doppler run -- cargo …) isn't available in this job. The diff is type-trivial (open::that returns std::io::Result<()>), so this is low-risk.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

cursor · 2026-06-25T21:47:44Z

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

claude · 2026-06-25T21:47:55Z

Claude finished @cloud9c's task in 2m 45s —— View job

Code Review — `fix(auth): continue login when browser launch fails`

Read PR diff and surrounding code (auth fix + new agent-friendly commit, 8 files)
Verify dependencies, types & build (build commands restricted in this job — see note)
Post review

This PR now contains two distinct commits: the original small auth.rs browser-fallback fix (084a43b), and a large new agent-friendly commit (da291d9, +802/-138 across 8 files) that adds --json output to most commands, non-interactive init, --token-stdin, --no-color, --no-update-check, and headless-environment detection. Reviewing both.

🔴 Worth a decision — the two commits overlap and partly conflict

The auth.rs fix (commit 1) makes browser login non-fatal: if open::that fails it prints a fallback URL and keeps the local callback server running so the user can finish auth from a browser. But commit 2 adds a guard that runs before login_with_browser() is ever called (src/main.rs:620-624):

if is_browser_login_unavailable() {
    anyhow::bail!("Browser login requires a local desktop session. In headless, SSH, CI, ...");
}
match login_with_browser() { ... }

is_browser_login_unavailable() returns true for CI, WAVEDASH_HEADLESS, any SSH session (SSH_CONNECTION/SSH_CLIENT/SSH_TTY), and Linux-without-DISPLAY/WAYLAND_DISPLAY. That set is exactly the headless/SSH scenario the commit-1 fallback was written for — so in those environments the fallback-URL path is now unreachable (we bail before reaching it). The two changes were authored ~4 hours apart and target the same problem from opposite directions.

This is likely intentional and arguably better for true remote sessions: the callback server binds 127.0.0.1, so a URL opened in the user's local browser would hit their loopback, not the remote host's — the fallback can't actually complete a remote SSH login, whereas the bail message points the user at --token-stdin/WAVEDASH_TOKEN, which do work. So this isn't a bug, but it's worth deciding consciously:

Is the commit-1 auth.rs fallback still needed, or is it now dead in practice? It still helps the local "Linux box with no working default browser but a real display"… except that box has DISPLAY set, so it isn't caught by the guard — so yes, the fallback still earns its place. Good.
The SSH_CONNECTION check is a broad heuristic: SSHing into your own desktop with X11/Wayland forwarding (where a browser could open) is now blocked from even trying. Consider whether WAVEDASH_HEADLESS/CI plus the Linux-no-display check is enough, and whether SSH-with-a-display should be allowed to fall through to the (now non-fatal) browser attempt. At minimum, documenting the precedence would help.

🟠 Possible regression — `handle_stat_update` now deserializes the response unconditionally

src/stats.rs — the update path changed from not parsing the response to parsing it as Stat on every call, even in non-JSON mode:

let resp = config::check_api_response(resp).await?;
let stat: Stat = resp.json().await?;   // NEW — runs even when json_output == false
if json_output { /* print stat */ return Ok(()); }
println!("✓ Updated stat {}", stat_id);   // stat is otherwise unused

If the PATCH /stats endpoint returns anything that doesn't deserialize into { _id, identifier, displayName } (an empty body, 204 No Content, or a wrapped/different shape), this now errors where it previously succeeded — and it regresses the non-JSON path, not just --json. The create endpoint already returned a full Stat, so that one is fine; but update was previously parse-free, so this is a new assumption about the response body. Worth confirming the update endpoint actually returns a full Stat. If unsure, gate the parse behind if json_output. Fix this →

🟡 Minor / optional

auth.rs fallback still drops the launch error. As flagged in several earlier threads, if open::that(&auth_url).is_err() discards the cause (e.g. Launcher "gio" "open" … ExitStatus(…)). Surfacing it (if let Err(error) = … { eprintln!(" {error}"); }) aids diagnosing odd environments and matches the PR's own After example. Non-blocking — current two-line output is acceptable.
handle_init_scripted creates remote resources before writing the file. If --team-name/--game-title are given, the team/game are created via API before std::fs::write(&config_path, …). A write failure (or a race where the file appeared after the early exists() check) leaves orphaned remote resources. Same shape as the interactive flow, so this is pre-existing behavior — just noting it now runs unattended in scripts where partial failure is harder to notice.
--json stream consistency. JSON goes to stdout via println!, which is correct for machine parsing. Good. Just ensure no stray println! diagnostics land on stdout ahead of the JSON object in any of the handlers (the update-check is correctly suppressed via command_outputs_json, 👍).
No test coverage for the new non-interactive init, headless detection, or --json serializers. These are the higher-risk additions in this commit; a couple of unit tests around is_browser_login_unavailable() (env-var matrix) and validate_scripted_init_args would be cheap regression insurance.

What's good

--json plumbing is consistent and the update-check + progress bar are correctly suppressed in JSON mode (verbose && !json, command_outputs_json).
uploader.rs cleanly makes the progress reporter optional (Option<&ProgressReporter>) rather than faking one — nice.
--no-color honoring both the flag and NO_COLOR is the right convention.
--token-stdin with conflicts_with = "token" is a good, scriptable auth path.

No security concerns. The headline items are the commit-1/commit-2 overlap (decision, not necessarily a fix) and the handle_stat_update unconditional deserialize (verify the endpoint contract).

Note: I couldn't run cargo check/clippy — both the Doppler wrapper (per CLAUDE.md) and bare cargo are restricted in this job, and there's no merge-base with origin/main (shallow clone), so I reviewed the two PR commits directly. Findings are from static analysis.
• kire5030/wvdsh-1604-cli-wavedash-auth-login-aborts-in-headlessssh-environments

claude · 2026-06-25T21:48:24Z


-    config::check_api_response(resp).await?;
+    let resp = config::check_api_response(resp).await?;
+    let stat: Stat = resp.json().await?;


Behavior change on the default (non-JSON) path. Previously handle_stat_update only called check_api_response(resp).await?; now it unconditionally parses the body as a Stat (let stat: Stat = resp.json().await?) before the json_output branch. This runs even when --json is not passed, so if the update endpoint returns an empty body or a shape that isn't a Stat, the command will now fail where it previously succeeded.

If the parse is only needed for JSON output, gate it:

config::check_api_response(resp).await?; if json_output { let stat: Stat = resp.json().await?; println!("{}", serde_json::to_string_pretty(&stat)?); return Ok(()); } println!("✓ Updated stat {}", stat_id);

(Note: check_api_response consumes/returns resp, so you'd parse from the returned value as the diff already does — just move it inside the if.) handle_stat_create already had the parse, so it's unaffected; this is specific to update.

claude · 2026-06-25T21:48:30Z

+                        if is_browser_login_unavailable() {
+                            anyhow::bail!(
+                                "Browser login requires a local desktop session. In headless, SSH, CI, or cloud-agent environments, set WAVEDASH_TOKEN or pipe a token into `wavedash auth login --token-stdin`."
+                            );
+                        }


Worth noting the interaction with the src/auth.rs fallback this PR adds. is_browser_login_unavailable() hard-bails before login_with_browser() runs in CI / WAVEDASH_HEADLESS / SSH / Linux-without-DISPLAY. So the new "print a manual URL and keep the callback server running" fallback in auth.rs is only reachable when the heuristic says a browser is available but open::that still fails (e.g. a Linux desktop with DISPLAY set but a broken gio/xdg-open — the exact case in the PR description). That's a reasonable split, but two consequences:

In a true headless SSH session the user never sees the manual-URL fallback — they get the bail message instead. That's arguably better UX, just flagging that the PR title ("continue login when browser launch fails") describes the narrower path.

The fallback path can't be exercised by simply setting CI=1 in a test, since the bail fires first.

No change required — just confirming the layering is intentional.