Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d01aaa7818
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+1117
to
+1118
| std::thread::spawn(move || { | ||
| for line in stdin_rx.iter() { |
There was a problem hiding this comment.
When the subprocess exits while the caller retains a FixStdinWriter, this detached thread remains blocked in stdin_rx.iter() and therefore keeps the receiver open. A subsequent send_line after the execution future has completed returns Ok; only that queued write discovers EPIPE and eventually closes the channel, contrary to the public promise that sending after completion returns Err. It also leaves one parked OS thread per completed fix until the writer is dropped or another line is sent. Coordinate child completion with this worker so the receiver is closed as soon as the process exits.
Useful? React with 👍 / 👎.
matt2e
force-pushed
the
log-in-code
branch
2 times, most recently
from
September 2, 2026 04:04
625ab08 to
29d68c7
Compare
Fix commands run through the streaming executor always inherited the host process's stdin. In a GUI host that stdin is never writable, so an interactive fix like `claude-agent-acp --cli auth login` — which prints an OAuth URL and then blocks reading the auth code — hangs forever (block/berd#99). Add an opt-in pipe the host can feed: - New public types `FixStdin` / `FixStdinWriter`: `FixStdin::pipe()` returns a cloneable line writer (`send_line`, which appends `\n` and flushes) plus the `FixStdin` to place in the options. Lines sent before spawn are buffered; dropping every writer delivers EOF. - `ExecuteFixOptions` gains `pub stdin: Option<FixStdin>` and a `with_stdin` builder; the existing `Debug`/`Clone`/`Default` derives are preserved via an `Arc<Mutex<Option<Receiver>>>` around the non-cloneable channel receiver. - The option threads through `execute_fix_streaming_with_env_options` → `run_command_streaming` → `run_command_streaming_blocking`, which only then sets `Stdio::piped()` on stdin and feeds the child from a detached writer thread. The thread is deliberately never joined — a writer outliving the child would park it in `rx.iter()` and hang the fix; it exits on channel close or on the post-exit EPIPE write error. - `stdin: None` keeps today's inherited-stdin behavior byte-for-byte, so terminal hosts with legitimately interactive fixes are untouched. Tests: cat echo round-trip (write + EOF + pre-spawn buffering), `read -r` prompt shape (the paste-an-auth-code flow), and no-hang-when-writer-outlives-child including post-exit `send_line` erroring instead of panicking. Existing streaming tests cover the default path. Verified with `cargo test` in crates/doctor (118 passed) plus `cargo fmt` and `cargo clippy --all-targets`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`ExecuteFixOptions` and `FixStdin` are both `Clone`, so a host that caches its options struct and retries a failed login got a second child whose stdin was EOF'd at spawn, while `send_line` calls either vanished or landed in the *first* child's stdin. The retry hung or died with nothing in the output explaining why — block/berd#99 re-created by the mechanism meant to fix it. Claim the receiver before anything is launched and turn the already-consumed case into an error: - `run_command_streaming_blocking` hoists `take_receiver` to the top of the body, ahead of `build_shell_command`, and maps `None` to "FixStdin already consumed by a previous fix execution". Pre-spawn matters beyond tidiness: returning `Err` after `spawn` would drop the `Child`, and `std::process::Child`'s `Drop` neither kills nor reaps, so the doomed login subprocess would keep running and become a zombie. - `Stdio::piped()` now keys off the claimed receiver, and the post-spawn block collapses to the writer thread — no `match`, no `drop(child_stdin)` EOF arm. - `stdin: None` is untouched byte-for-byte; terminal hosts with legitimately interactive fixes keep inheriting stdin. A spawn failure now consumes the receiver even though no child got the pipe, so a retry with the same cached options reports the reuse rather than the underlying spawn error. That is strictly better than a hang, and a `restore_receiver` escape hatch would add a branch no test in this crate can exercise (the shell path isn't injectable). Docs for `FixStdin`, `ExecuteFixOptions::stdin`, `with_stdin`, and `take_receiver` are corrected to the single-use contract they now have. Tests: new reuse case asserts the second run errors naming the reuse and that nothing reaches `on_line`, which pins the pre-spawn ordering — `run_command_streaming` emits no preamble of its own, so any output would mean the child ran. Verified with `cargo test` in crates/doctor (119 passed) plus `cargo fmt`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri, which is workspace-excluded and so never built by crate-local commands. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`run_command_streaming_blocking` could park forever in two places, and the two were entangled: the detached stdin writer thread's only "am I done?" signal is the main loop finishing, and the main loop can't finish while the child is alive. Fixing either alone leaves the other in place. Reclaim the writer thread: - The thread polls `recv_timeout(STDIN_WRITER_POLL_INTERVAL)` (250 ms) against a shared `AtomicBool` instead of parking in `stdin_rx.iter()`, so a host that holds its `FixStdinWriter` and simply stops sending no longer leaks an OS thread and a `ChildStdin` fd per fix run. - A `FixFinishedFlag` drop guard sets that flag on every exit path — including the `wait` error return and a panic in `on_line` — so worst-case thread lifetime is "fix completion + one poll interval" regardless of caller discipline. Breaking out still drops `child_stdin`, delivering EOF. - Dropping the receiver on the way out makes `send_line` fail deterministically from then on, which lets the writer-outlives-child test replace its 100 x 10 ms polling loop with a fixed wait. Give the fix itself a deadline: - New `FixTimeout` (`Standard` / `After` / `Unbounded`) plus `DEFAULT_FIX_TIMEOUT = 600s`, added to `ExecuteFixOptions` with a `with_timeout` builder and threaded through `execute_fix_streaming_with_env_options` -> `run_command_streaming` -> `run_command_streaming_blocking` exactly as `stdin` was. An enum rather than `Option<Duration>` because `None` reads as both "use the default" and "no timeout"; `Unbounded` also keeps the plain `rx.recv()` path, sidestepping the `recv_timeout(Duration::MAX)` instant-overflow hazard. - 10 minutes clears a cold-cache `npm install -g` behind the corporate proxy and a human doing SSO in a browser. The 10s/15s probe timeouts in `command.rs` are wildly wrong for this path. An idle timeout would suit installs better but is precisely wrong for `auth login`, which prints its URL and then goes deliberately silent; `FixTimeout` can gain `Idle` later. - Enforcement replaces `rx.iter()` with `recv_timeout` against the deadline and `child.wait()` with `wait_timeout` — a process can close both pipes and keep running. On expiry: drain queued lines with `try_recv` so the last real output survives, emit one greppable notice through `on_line`, kill, reap, and return `Err`. The reader threads are deliberately *not* joined, because a descendant that escaped the process group can hold the inherited stdout open indefinitely (`command.rs`'s module docs; the escaped-descendant test). - `kill_child_process_group_or_child` is promoted to `pub(crate)`, and the child gets `process_group(0)` only when `stdin.is_some()`. Group-kill is available precisely when doctor owns the child's stdin and so the child can't touch the tty; setting it unconditionally would give a terminal host's tty-reading fix a SIGTTIN stop. `stdin: None` stays byte-for-byte unchanged. The in-crate `ExecuteFixOptions` literals switch to `..Default::default()` so the next option field is genuinely additive. Tests: `After(100ms)` vs `sleep 60` returns `Err` naming the timeout and the command in well under a second, with the notice line visible to `on_line`; the group kill takes a backgrounded grandchild with it (verified non-vacuous by disabling `process_group(0)`, which makes it fail); the timeout returns promptly even when a `setsid`-escaped descendant holds the pipes open; the writer thread has retired a few poll intervals after the fix returns; and a guard test pins `DEFAULT_FIX_TIMEOUT` at 600s, `FixTimeout::Standard` as the `ExecuteFixOptions` default, and the default at >= 30x probe scale. Verified with `cargo test` in crates/doctor (123 passed) plus `cargo fmt` and `cargo clippy --all-targets`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
… fields `apps/staged/src-tauri` is in the workspace `exclude` list, so `cargo check` under `crates/` never compiles it — while `staged-ci.yml` triggers on `crates/**`. Its exhaustive `ExecuteFixOptions` literal therefore turns every new doctor option into a CI-only breakage, twice now: `stdin`, then `timeout`. Switch it to `..Default::default()`. Staged's fixes are non-interactive, so the defaults are what it wanted anyway — inherited stdin and the standard 600s fix timeout, both far more than any install or login this runs needs. Verified with `cargo check` from apps/staged/src-tauri. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…livered
`send_line` was documented as returning `Err` "when the fix has already
finished (its stdin pipe is closed)" and could not honour it: it returned
`mpsc::Sender::send`'s result, which fails only once the receiver drops, and
the receiver lived in the detached writer thread. So the first post-exit
`send_line` — plus anything queued behind it — returned `Ok` and was then
discarded when the thread's write hit EPIPE. For berd#99 that is the
difference between an error and a hang: the login subprocess dies, the user
pastes the auth code a beat later, `send_line` says `Ok`, the code goes
nowhere, and a host treating `Ok` as delivery waits forever with nothing in
the log to explain it.
Replace the channel and the thread with a three-state pipe shared behind the
`Arc<Mutex<…>>` `FixStdin` already carried, written inline by the caller:
- `FixStdinState` is `Buffered { lines, eof, claimed }` -> `Live(ChildStdin)`
-> `Closed`. `send_line` queues on `Buffered`, writes through on `Live` and
reports the real `io::Error`, and fails fast on `Closed` — which is latched,
so a dead pipe is discovered once. `ChildStdin: Debug`, so the public
`Debug`/`Clone` derives on `FixStdin` and `ExecuteFixOptions` survive.
- EOF on last-writer-drop, previously free from the mpsc disconnect, comes
from a `Drop` on a `FixStdinWriterInner` behind an `Arc`, keeping
`FixStdinWriter: Clone`. The `Buffered { eof: true }` arm is load-bearing:
a host may queue a line, drop the writer, and only then start the fix, and
those lines must still be replayed before the pipe closes.
- The runner claims pre-spawn exactly as before (`claim` replaces
`take_receiver`), then `attach`es the child's stdin *after* the reader
threads start — the replay writes inline on the runner thread, so a queue
bigger than the pipe buffer would otherwise deadlock against a child whose
output nobody is draining.
- `FixStdinCloser` replaces `FixFinishedFlag`, closing the pipe on every exit
path (return, error, timeout, spawn failure, a panic in `on_line`). EPIPE
cannot carry this alone: a probe confirms that with the runner's
`zsh -l -c` shape a backgrounded grandchild inherits stdin and keeps the
read end open, so writes into a finished fix's pipe still succeed.
Two review comments dissolve rather than getting documented around: there is
no detached thread to leak an OS thread and a `ChildStdin` fd for, so
`STDIN_WRITER_POLL_INTERVAL` and its worst-case-lag caveat are both gone, and
the fd is now reclaimed at fix completion instead of one poll interval later.
Public API is unchanged — `FixStdin::pipe`, `send_line`, `ExecuteFixOptions::
stdin`/`with_stdin` keep their signatures, and `stdin: None` is byte-for-byte
untouched. One new semantic: `send_line` performs blocking I/O under a mutex
on the calling thread. For a one-line auth code against a ~64KB pipe buffer
that is instantaneous, but the doc now says it can block if the fix isn't
reading, and a host sending anything bulkier should keep it off its async
runtime. Docs also state the hazard that predates and survives this change:
a fix reading *to EOF* won't exit until every writer clone drops.
Tests: the writer-outlives-child test drops its 100x10ms poll loop and
asserts the *first* post-fix `send_line` is `Err` naming closed input; a new
grandchild test pins the explicit close (verified non-vacuous — disabling
`close()` fails it, while the no-grandchild case passes on EPIPE alone); the
prompt-style test now sends from inside `on_line`, on the fix's own thread in
response to the fix's own prompt, so its `Ok` is the delivery guarantee
rather than the pre-spawn queueing one; the `cat` round-trip test is
unchanged as the `Buffered { eof: true }` regression test, and was confirmed
to hang without that arm. Verified with `cargo test` in crates/doctor (124
passed) plus `cargo fmt`, `cargo clippy --all-targets`, and `cargo check` in
apps/staged/src-tauri.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
`execute_fix_streaming_with_env_options` resolves the fix command and `?`s out when the lookup misses. That return drops `opts.stdin`, and `FixStdin` has no `Drop`, so the shared state stayed `Buffered` forever: a host still holding a `FixStdinWriter` kept getting `Ok` from `send_line` for a fix that would never spawn — the same "`Ok` didn't mean delivered" hole c29da8f closed at the fix's end, at its spawn failure, and on its timeout. Low severity on its own (the awaited call returns a self-explanatory `Err("Unknown check …")`, and reaching the arm needs a caller bug), but it bites exactly the host shape berd#99's UI wants: a code-entry box feeding input from a different task than the one awaiting the fix. Worth making the contract uniform. Neither obvious fix works. Claiming before the lookup closes nothing — `claim` only sets `Buffered { claimed: true }` and `send_line` returns `Ok` on any `Buffered` state, so the writer still gets bogus `Ok`s from the run that failed. A bare `close()` in the failure arm is strictly worse than the bug: `FixStdin` is `Clone` and clones share one `Arc<Mutex<FixStdinState>>`, so closing a stale clone would EOF a running login the user is mid-way through. `impl Drop for FixStdin` is not a substitute either — per-handle `Drop` reintroduces that trap, and last-handle `Drop` leaves the reported case unfixed, since a host that *retains* its `FixStdin` keeps the count above zero. Make the ownership test explicit instead — the claim *is* the test: - `FixStdin::close_if_unclaimed` closes only a pipe no run has reserved, so a stale clone whose first run is still live is never EOF'd out from under it. - `UnlaunchedFixStdinCloser` hangs that off a scope guard, armed ahead of the lookup in `execute_fix_streaming_with_env_options`, covering every exit that never reaches the runner: an unresolved command, a panic in the `on_line` preamble, a blocking task dropped before it ran. Once the runner claims, it is a no-op and the existing `FixStdinCloser` owns the close. The two guards cannot be merged — `FixStdinCloser` needs the unconditional `close()`, which `close_if_unclaimed` would refuse for the very pipe that run claimed. The guard borrows `opts.stdin`, so the hand-off to `run_command_streaming` clones it — an `Arc` bump. `stdin: None` stays byte-for-byte unchanged (the guard is `None`), both non-streaming entry points inherit the fix by funnelling through here, and direct `run_command_streaming` callers keep relying on the inner closer. `send_line`'s docs drop "if the fix never spawns they are dropped" for what now happens, and name the one `Ok` that still isn't a delivery guarantee: a send in the legitimate pre-spawn window. Tests: an `UpdateMain` with no `command_override` against a real check id — the honest reachable miss, since `lookup_fix_command` returns `None` for both `Update*` variants — errors, captures nothing through `on_line` (no `$ command` preamble means the fix never ran), and makes the *first* following `send_line` fail; and the guard's non-vacuity case runs `cat` against a live pipe, then hands a *clone* to a failing execution and asserts the live run still takes a line and finishes `Ok`. Both were confirmed non-vacuous: disarming the guard fails the first, and swapping `close_if_unclaimed` for `close` fails the second. The live fix runs on a plain thread rather than through `run_command_streaming`, because blocking on `recv_timeout` would starve `#[tokio::test]`'s current-thread runtime before a spawned task reached its `spawn_blocking`. Verified with `cargo test` in crates/doctor (126 passed), `cargo fmt --check`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… stdin `run_command_streaming_blocking` only called `process_group(0)` when the caller supplied a `FixStdin`, so the `FixTimeout` tree kill worked on the path nothing uses yet and did something weaker — and unpredictable — on the path every real fix takes. Staged reaches this through `execute_fix_with_env_options` with `stdin: None`, where, verified locally: - the group kill is a no-op (a child that never called `setpgid` shares the host's pgid, so `kill(-childpid)` fails with `ESRCH`) and `kill_child_process_group_or_child` falls back to `child.kill()`; - the fallback's reach depends on the user's dotfiles: zsh exec-optimizes `zsh -l -c 'sleep 4'` down to `sleep` itself, so a plain `npm install -g` does get killed — but a `TRAPEXIT`/`zshexit` hook in a `.zprofile`, or any pipeline, keeps zsh alive with the fix as a grandchild that survives; - descendants always survive, keep the inherited stdout open, and may finish an install the user was told was terminated; - and `doctor: fix timed out after … — terminating` claimed a tree kill in exactly those cases. The old gate's premise was narrower than the gate: SIGTTIN needs an actual terminal on fd 0. This runner always pipes stdout/stderr, so fd 0 is the only tty descriptor a fix can inherit, and if it is `/dev/null`, a pipe, or closed — the GUI-host case that is the whole reason `FixStdin` exists — nothing the child does with it can raise SIGTTIN, whatever group it is in. The probe runner in `command.rs` already treats "no tty on stdin" as licence to own the group. - New `FixProcessGroup` (`Own` / `Inherited`) with `for_fix(stdin, stdin_is_terminal)`: `Own` when stdin is piped *or* fd 0 is not a terminal, `Inherited` only for a terminal host on inherited stdin. `run_command_streaming` computes it once from `std::io::stdin().is_terminal()` before `stdin` moves into the closure, so the one production call site changes and the eleven test call sites don't. The decision is a parameter of the blocking runner, not a re-read of fd 0 inside it, because cargo passes the terminal through to test binaries — a tree-kill test that trusted auto-detection would exercise `Own` in CI and `Inherited` on a laptop, silently. - `kill_child_process_group_or_child` returns a `KillReach` instead of discarding it, and the timeout arm kills *before* phrasing its notice: `Own` → "killed the fix and its child processes", `Inherited` → "killed the fix process; anything it started may still be running", with the same caveat appended to the `Err` for a host that only logs the string. The `Inherited` arm calls `child.kill()` directly, skipping a negative-pid `kill` whose failure is known by construction. The `doctor: fix timed out after ` prefix and the "names the timeout and the command" shape of the error both survive. - `stdin: None` in a terminal host is byte-for-byte unchanged. `setsid`-wrapping was rejected (no `setsid` binary on macOS, and a new session loses the controlling terminal, breaking a `/dev/tty` prompt like `sudo`'s), as was a new `ExecuteFixOptions` field (its default has to be wrong for someone; worth adding later purely as an override). One thing the plan did not anticipate: the two tree-kill tests were vacuous as written. A *login* shell with real dotfiles takes ~1.6s to start here, so a 300ms deadline fired before the payload had forked anything and the marker file was absent no matter what the kill reached — including the pre-existing piped-stdin test, whose non-vacuity check would have been fooled the same way. Both now share a helper that gives the shell a throwaway `HOME` (~0.2s of startup, and the same on any machine), backgrounds `sleep 300` recording its pid, and asserts on that pid's liveness after the deadline — with the pid file's absence a loud failure rather than a silent pass. Tests: the decision matrix (`for_fix` over both stdin shapes x both tty states, pinning `None` + non-tty as the fix and `None` + tty as the preserved behavior); tree kills through the blocking runner for both `stdin: None` and piped stdin, each confirmed non-vacuous by forcing `Inherited` and watching it fail; and two message tests pinning each notice/`Err` phrasing, likewise confirmed by swapping the decision. Verified with `cargo test` in crates/doctor (130 passed) and again under `cargo test < /dev/null` to prove the new tests are tty-independent, plus `cargo fmt --check`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…e mutex `FixTimeout` promises a wall-clock bound on every fix, and since c29da8f the `FixStdin` pipe holds the child's `ChildStdin` inside an `Arc<Mutex<…>>` so `send_line` can write inline and report real delivery. The two collide: every write happens *while holding the state mutex*, and a write into a full pipe blocks for an unbounded time. Two paths escaped the bound. **The replay.** `FixStdin::attach` took the mutex, went `Live`, and replayed every pre-spawn queued line with an inline `write_all`, all under the guard — and before the deadline was computed. Queue more than a pipe holds ahead of a fix that never reads stdin but keeps the read end open (a backgrounded descendant is enough) and the runner parked in `write_all` with no deadline armed and nothing able to interrupt it. Arming the deadline earlier fixes nothing: it is only ever *consulted*, in the recv loop and `wait_timeout`. - New `MAX_QUEUED_FIX_STDIN_BYTES` (4096) caps the pre-spawn queue, charged as `line.len() + 1` for the newline `send_line` appends and tracked in a new `Buffered { queued_bytes }`. A replay into a *virgin* pipe cannot block while the total fits its capacity; 4 KiB is the one-page floor on any platform doctor runs on, measured at 65536 here (macOS grows the pipe on first write — `lsof`'s initial 16384 is not the ceiling), and ~40x the longest credential a fix prompts for. Over the cap, `send_line` returns `Err`: the honest answer, and what c29da8f established — not delivered, and not silently queued for a replay that would wedge the runner. **The return path.** `FixStdinCloser::drop` → `close()` set `Closed` under the same mutex, so a host parked in a `Live` write kept the runner from returning at all — the timeout fired, the notice reached `on_line`, the tree was killed, and then the function that guarantees a return sat on a lock. - `FixStdinShared { state: Mutex<FixStdinState>, closed: AtomicBool }` replaces the bare `Arc<Mutex<…>>` in both `FixStdin` and `FixStdinWriterInner`. Both fields are `Debug`, so the public `Debug`/`Clone` derives on `FixStdin` and `ExecuteFixOptions` — load-bearing since d01aaa7 — survive. - The invariant is enforced by construction rather than by remembering: a `FixStdinGuard` wrapper (replacing the free `lock_fix_stdin_state`, keeping its poison recovery and rationale) applies the latch on release, so whichever lock holder is last to leave performs the `Closed` transition. - `close()` stores the latch, then `try_lock`s once. That arm is not best-effort: it is what reclaims the `ChildStdin` fd in the ordinary case, where nobody will lock again. `WouldBlock` returns immediately and leaves the transition to the holder's guard drop. - `send_line` checks the latch *before* the mutex, which the mutex-only design could not offer: after the fix ends a host is never even queued behind another clone's parked write. `FixStdinWriterInner::drop` takes the same fast path but must not skip a contended lock — dropping the last writer *is* the EOF. - `let limit`/`let deadline` move up to the spawn, so the fix's wall clock measures the fix rather than silently excluding setup. Deliberately not fixed: a host's own `Live` write into a full pipe still blocks without bound. Bounding it needs a non-blocking fd plus a `poll(POLLOUT)` loop, and changes `send_line`'s contract for the worse — a write abandoned partway leaves a truncated line in the child's stdin, which for an auth code is poison. What it gets instead is documentation: such a write delays other clones and the last-writer EOF, but no longer the fix's own completion or `Err`. Tests: the cap's `Err` and its exact budget (plus an oversized single line refused rather than sliced); a compile-time guard on the cap fitting a pipe's floor, since a cap that can wedge the runner should not build; a full queue replaying and finishing promptly; and the hazard-B regression — a `setsid` descendant the group kill can't reach holds the pipe while a host thread parks in a 1 MB write, driven on a plain thread with a `recv_timeout` window between the deadline and that descendant's exit so a regression *fails* instead of hanging the suite, plus a second thread proving a fresh `send_line` answers from the latch while the mutex is still held. Both new hazard tests were confirmed non-vacuous by reverting each half of the fix; the replay test by raising the cap to 400 KiB, which turns it into a 30s `Ok` that the elapsed bound catches. One find along the way: the backgrounded-descendant tests need the dotfile-free `HOME` (now a shared `dotfile_free_env` helper) for a second reason beyond startup time — under a real `$HOME` the shell's startup leaks a descriptor into its children, so a descendant with both output streams redirected still held the fix's stderr open and kept the reader threads alive for its whole lifetime. Verified with `cargo test` in crates/doctor (134 passed), again under `cargo test < /dev/null`, plus `cargo fmt --check`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The code-entry UI added with the authentication recovery controls could never appear. It was gated on `isAuthCodePrompt`, and the Claude CLI's prompt is `Paste code here if prompted > ` with no trailing newline — doctor's reader threads use `BufReader::lines()`, so that fragment is only delivered when stdout closes at exit. The two lines that do arrive live (`Opening browser to sign in…` and `If the browser didn't open, visit: <authorize url>`) match neither regex. Net effect: the input never rendered, and the login only succeeded if the CLI's own `open` plus its localhost callback completed — the manual-code path this branch exists for was unreachable. Stop inferring the prompt and show the entry for as long as the login runs: - `isAuthCodePrompt` is deleted rather than widened. Nothing in the stream can announce the prompt, and the CLI re-prompts (again newline-less) on a code it rejects locally, so hiding the box after a send left a mistyped paste with no way to retry. `submitAgentLoginCode` now clears only the sent text. - New `agentLogin.svelte.ts` owns the login as shared state: the check id, the running flag, the sign-in URL, a 40-line output tail, the error, and the code being typed. One record, because doctor allows one login per check and both UI entry points drive the same fix; a second `startAgentLogin` is refused rather than taking the record from a running login. - New `AgentLoginPrompt.svelte` renders the URL (selectable, plus an `Open` button), the code input, and the output tail. Surfacing the URL closes the other half of berd#99: the CLI's `open` can fail silently inside the Tauri process, and a user driving Staged through the web server never had a browser on the host — previously that was the same dead end with a spinner. - The listener is registered with the check id captured once instead of read from `session?.provider` at callback time (the pane is reused across opens, so switching sessions mid-login made every event fail the filter, stranding `loginRunning` true and the unlisten uncalled until destroy), and the fix is started from `onEstablished` — registration is asynchronous, and a spawn failure's immediate `done` lost in that gap stranded it the same way. `onEstablished` fires on every web-socket reconnect, so the start is latched. - The pane no longer unlistens on destroy: the subprocess outlives the pane, and the record is what the Doctor panel — or the pane's next open — feeds a code. Both entry points now behave the same. `run_doctor_fix` routes `FixType::Auth` into the same piped, streamed helper as `start_doctor_login` (`claim_login` + `run_login_fix`, the latter awaited here and spawned there), so the Doctor panel's own `Fix` on an unauthenticated agent is no longer a login with `/dev/null` on stdin that printed its URL to a log nobody reads and died at the 600s `FixTimeout`; `DoctorCheckRow` drives it through the shared record and renders the same prompt inside its confirmation dialog. Two smaller things from the same review: - `send_doctor_login_code` is `async` and does the write on a blocking thread. `send_line` writes into the pipe inline under the pipe's state mutex, and under Tauri 2 a non-`async` command body runs on the main thread — the worst place to discover a full pipe. - `SessionChatPane` runs the doctor checks itself the first time it displays an authentication failure with no report. `doctorState.report` was otherwise populated only by opening the Doctor settings panel, so on a fresh launch `canLogin` was false for every auth-failed session and only `Fix` rendered — the primary action appeared only after a detour through settings. - `startDoctorLogin`'s doc named a `doctor-fix-output` event; the backend emits `doctor-login-output`. Tests: a new suite for the store covers the URL extraction against the real authorize line, that nothing starts before `onEstablished` and only once across reconnects, that another check's `done` doesn't finish this login, the failure and never-spawned paths, that a send keeps the box open and a finished login refuses one, and the record's scoping and clearing. The `isAuthCodePrompt` cases are gone with the function. Verified with `vitest run` in apps/staged (74 files, 918 passed), `svelte-check --fail-on-warnings` (0 errors), `prettier --check`, and `cargo check`, `cargo clippy` and `cargo fmt --check` in apps/staged/src-tauri. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
`run_command_streaming_blocking` owns the fix's `Child`, and until now nothing outside it could end a run early: a fix finished on its own or `FixTimeout` killed it, ten minutes later by default. The review of a190fa9 proposed aborting an abandoned login by dropping the `FixStdinWriter` so the CLI reads EOF and exits. That does not work for the CLI this branch exists for: `claude-agent-acp --cli auth login` prints its URL and then waits on its localhost callback server, ignoring a closed stdin (Experiment B in the stdin-vs-TTY notes ran it with `< /dev/null` and killed it after 25s, still waiting). So a user who closes the sign-in tab and wants to start over has no way out — Staged's login slot stays taken until the 600s deadline — and the host cannot help, because it has no handle on the process. This was deferred from the original plan as "kill-on-cancel for abandoned logins"; the a190fa9 review makes it required. Add the handle, shaped like `FixStdin`: - `FixCancellation::token()` returns a `FixCancelHandle` for the host to keep and the `FixCancellation` to place in the new `ExecuteFixOptions::cancellation` (with a `with_cancellation` builder). The handle is `Clone + Send + Sync`; `cancel()` is infallible and idempotent — an `AtomicBool` store, so a UI thread never waits on a runner that is mid-`write_all` under the stdin mutex — and `is_cancelled()` lets a host holding the run's `Err` tell a cancellation it asked for from a failure without parsing the message. The runner's error type is unchanged, so goose-internal's older entry points are untouched. Both types are `Debug`, so the derives on `ExecuteFixOptions` survive. - Before the spawn, the runner takes one last look after the stdin claim and the `FixStdinCloser` are in place: a cancelled token refuses the spawn with an `Err` naming it, nothing reaches `on_line`, the pipe is closed by the existing closer, and there is no child to kill or reap — an `Err` after `spawn` would drop a `Child` whose `Drop` neither kills nor reaps. - Mid-run, a `FixStop` enum (`TimedOut` / `Cancelled`) replaces the `expired` bool, and both reasons share one abort path: drain what the readers already queued with `try_recv`, kill through the existing `kill_child_process_group_or_child` / `FixProcessGroup` machinery, reap, drain the lines the kill shook loose, then emit one notice and one `Err` phrased for the reason — `doctor: fix cancelled — killed the fix and its child processes`, or the same `KillReach` hedge the timeout uses when doctor did not own the group. One reason per run by construction, so a cancel landing on a fix that is timing out cannot produce two notices. - After the run has returned, `cancel()` sets a flag nothing reads. The fix's own `Result` stands. - The `wait_timeout` crate computes its poll timeout in whole milliseconds and can return `None` a hair early; the reap now re-checks the deadline before calling a slice expired rather than trusting the `None`, a small strengthening that costs one extra `wait_timeout(~0)` in theory. The wake-up is bounded polling on the atomic, not a sentinel in the stream channel. `cancel_requested()` is checked on every pass through the recv loop and before every reap slice, and each blocking wait is cut to `min(remaining, FIX_CANCEL_POLL_INTERVAL)` (100ms) — including under `FixTimeout::Unbounded`, whose plain `recv()` and `wait()` nothing else would ever wake. Why not the sentinel: it only reaches the recv loop, while the reap phase reads no channel and would need polling regardless, so it is a second mechanism next to polling rather than a replacement for it; it needs the runner to publish a `Sender` into shared state under a lock once the channel exists (post-spawn), clear it on every exit path, and still carry the flag for the pre-spawn case and `is_cancelled()` — a Buffered/Live/Closed state machine for what is otherwise one atomic; and `StreamLine` would gain a variant that is not a line, which every drain loop then has to filter. What polling costs is ten wake-ups a second on a blocking thread, only while a cancellable fix runs and only while it is silent — nothing next to a login idling for minutes — and a latency bound of one interval plus a kill. The per-pass check, not the tick, is what bounds a fix that never goes quiet: continuous output never lets `recv_timeout` time out, and a tick-only design would be uncancellable for as long as the fix kept talking. `cancellation: None` maps to no slicing at all — `fix_wait_slice(None, None)` keeps the blocking calls — so that path is byte-for-byte unchanged, as `stdin: None` and `FixTimeout::Standard` were. The race with a normal exit is settled by observation order, not luck. The flag is consulted at fixed points, so if the runner sees it before it has observed the exit the run is cancelled — one notice, one `Err`, the line that was being consumed already delivered — and once `wait` has handed back a status the cancel is late and changes nothing. What is lost on a cancel is what the timeout already loses: a line a reader had not yet handed over when the kill landed, because the readers are deliberately not joined (an escaped descendant can hold the pipes open indefinitely). A cancel in the instant between the fix's exit and the runner's reap is reported as a cancellation; the window is the runner's own latency, milliseconds, not a human's. Deliberately not done: a `try_wait` in the abort path to hand back the fix's own result when it had already exited, which would make the outcome scheduling-dependent for a window nobody can hit and diverge from the timeout arm; a new error variant, which existing callers would have to absorb; and the Staged side — a `cancel_doctor_login` command and an enabled `Cancel` in the login dialog — which is the next commit. Docs on `FixStdin::pipe`, `FixStdinWriter`, `send_line` and `ExecuteFixOptions` now say plainly that EOF is "no more input", not "stop", and point at `FixCancellation`. Tests, each confirmed non-vacuous by disabling the half of the fix it pins: a mid-run cancel of a piped-stdin fix that backgrounds `sleep 300` (the pid file and `dotfile_free_env` helpers, the liveness poll factored out of the timeout tree-kill helper) returns promptly with `Err` naming the cancellation and the command, one notice promising the tree is gone, a dead grandchild and a closed pipe — fails with `Inherited` forced; a pre-spawn cancel spawns nothing, emits nothing, and closes the pipe — fails without the pre-spawn check; a cancel after completion through the public entry point and builder leaves the `Ok` and records the request; a cancel on the fix's last line yields exactly one notice; `Unbounded` plus a cancel from another thread during a silent stretch returns within a second — fails with polling disabled; a fix that prints without pause is cancelled between lines — fails with the flag checked only on ticks, which the final-line test alone did not catch because the reap-phase check masked it; a cancel while both pipes are closed but the child lives, for both the bounded and the unbounded reap, returns within a second — the only two tests that fail with the reap-phase check removed; the `Inherited` cancel hedges in both the notice and the `Err`; and a contract test pins the handle as shareable, `None` as the default, and the interval under 250ms. The two tests that cancel from another thread after a marker run their shell with the `dotfile_free_env` `HOME` for the same reason the timeout tree-kill tests do, plus one more: under a load average above 60 (parallel sessions building on this machine) a real-dotfile login shell took over ten seconds to print anything, which tripped the marker wait here and, in the same runs, two pre-existing tests that use the real `$HOME` — both green again once the load dropped, and untouched by this change. Verified with `cargo test` in crates/doctor (144 passed), again under `cargo test < /dev/null`, plus `cargo fmt --check`, `cargo clippy --all-targets`, and `cargo check` in apps/staged/src-tauri, whose `ExecuteFixOptions` literal already spells `..Default::default()`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The a190fa9 review left three gaps around a login the UI loses sight of, and 473c3f2 gave doctor the handle needed to close the first of them. **Cancel.** Nothing could end a login early. The CLI ignores EOF on stdin once it is waiting on its browser callback (Experiment B in the stdin-vs-TTY notes), so dropping the `FixStdinWriter` — the review's suggestion — would have left the check's slot held for the full 600s `FixTimeout` with nothing to show for it. The `ACTIVE_LOGINS` entry now holds a `FixCancelHandle` from `FixCancellation::token()` beside the writer, and a new `cancel_doctor_login` command calls `cancel()` on it: idempotent, harmless when nothing is running, and it reports whether a run was found. The `done` event gains `cancelled` (from `is_cancelled()` on a run that ended `Err`) with `error` left `None`, so every connected client renders a cancelled login as a neutral end rather than a failure; a cancel that lands after the fix finished on its own changes nothing, as doctor documents. The frontend record settles a cancelled `done` — its own or another client's — by clearing itself: no error, code box gone, `agentLoginFor` null. `AgentLoginPrompt` grows a Cancel beside the code entry (with a `cancellable` prop so a host with its own chrome doesn't show two), and `DoctorCheckRow` enables the dialog's Cancel for an `auth` fix while it runs. Its Escape and click-outside paths now report through `onOpenChange` to the same `cancelFix`, so leaving the dialog kills the login instead of stranding `fixing` and the slot until the timeout; bits-ui fires that callback only for user-driven closes, so the dialog's own programmatic close on completion does not re-enter it. Install and update fixes keep their disabled Cancel — cancelling those is a possible follow-up now that the runner supports it. **Re-attach.** `start_doctor_login` answered a login already running for the check with an error string, which `startAgentLogin` recorded as a failure: the CLI was alive and waiting for exactly the code the UI then refused to offer. It now returns a structured `LoginStart` (`started` / `alreadyRunning`), and the frontend treats the latter as a re-attach: `running` stays true, the listener stays, and the new `doctor_login_status` query — `running` plus a 40-line output tail, the size the frontend keeps — replays what was missed, restoring the URL and code box. A same-check `startAgentLogin` joins the running attempt instead of rejecting, so both entry points follow one run. Lines carry a sequence number to make the replay exact. The runner's `on_line` records each line in the entry's `LoginOutputTail` before emitting it, so a snapshot taken between the two covers the line whose event is about to follow; the frontend registers its listener first, then queries, and merges by `seq`: a live line below the snapshot's `next_seq` is in it (or older than the tail), one at or above arrived after. Content matching would have been a heuristic; this is a comparison. **Attach on open.** A client whose record is idle while the backend has a login running — a web refresh, a second client, a reloaded webview, a login started from the other entry point — now asks. `attachAgentLogin` probes without touching the record (nothing may be running, and a UI must not show a login that doesn't exist), adopts the run and replays its tail when the backend confirms one, and resolves `null` otherwise. The session pane probes once per open when it shows an authentication failure, not gated on `canLogin` since that needs the doctor report; the Doctor panel's fix dialog probes when it opens for an `auth` check, so a login started from the pane shows its URL and code box there too instead of a Run the backend would answer "already running". A `Log in` click during a probe takes over from it. The same sync runs on every web-socket reconnect after the first `onEstablished`, and when a cancel finds nothing running, so a `done` missed across a gap can't leave the record running forever; a login the backend no longer has settles as `completed`, and the checks the callers re-run report whether it actually signed in. **Slot release.** The `ACTIVE_LOGINS` entry was removed on the straight-line path only; a panic in `doctor_env_vars().await` or the emit closure, or a dropped future, left the check refusing logins until restart. A `LoginSlot` drop guard makes the release structural. It is dropped *before* the `done` goes out, deliberately: a client attaches by registering, then asking `doctor_login_status`, so a snapshot that says `running` is guaranteed to have been taken before the `done` was emitted, with that `done` still ahead of the listener; the other order lets a snapshot report a run whose end had already passed. The entry's doc now says why the writer is held for the whole run and what that means for a fix that reads stdin to EOF. `run_doctor_fix`'s `auth` arm still errors on an already-running login: it awaits the fix to its end, so there is no run to hand that answer to. Events are filtered by check id as before; a `done` from one run reaching a client whose fresh start for the same check landed in the microseconds between the slot release and that emit is a pre-existing window this change narrows but does not close. Tests. Rust: the tail numbers lines from zero and keeps the newest forty with a snapshot whose `output` sits exactly below `next_seq`; `login_done_event` tells a cancellation from a failure and leaves a late cancel alone; the slot lifecycle end to end — second claim `None` not `Err`, status and cancel reach the run through the entry, drop releases, the retry gets a fresh token; and a check with no auth command is refused. Store suite, in the existing style: cancel ends the record without an error and a second click asks nothing more; a cancelled `done` from another client is the same end; a cancel the backend can't find re-syncs; already-running re-attaches and the replayed tail restores the URL; a re-attach whose login ended first settles; a line delivered while the snapshot was in flight shows once and a redelivery is dropped; attach-on-open adopts a running login and leaves the record alone otherwise; a start takes over an unanswered probe; and a reconnect replays the gap and settles a login that ended in it. There is no component test infrastructure for `DoctorCheckRow` (vitest runs without the Svelte plugin), so its Cancel/Escape path rests on the store's cancel tests and on bits-ui's `onOpenChange` semantics, checked in its source. Verified with `vitest run` in apps/staged (74 files, 928 passed), `svelte-check --fail-on-warnings` (0 errors, 0 warnings), `prettier --check`, and `cargo check`, `cargo clippy`, `cargo fmt --check` and `cargo test doctor::tests` in apps/staged/src-tauri. crates/doctor is untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…red token The `Log in` action in the session alert was gated on doctor reporting `notAuthenticated`, which `agents.rs` derives from the exit code of `claude-agent-acp --cli auth status`. Measured against crafted credentials in an isolated config dir (the `auth status` probe note), that exit code is 0 for an expired token with a dead refresh token, for an expired token with no refresh token, and for a well-formed but bogus token: the CLI exits `loggedIn ? 0 : 1`, and `loggedIn` for the claude.ai branch is "a credentials record with an access token and the right scopes exists" — no expiry comparison, no network call, and no field in the status JSON to key on instead. So in the scenario that brings a user to this UI — the berd#99 route — the alert rendered only `Fix`, and the Doctor panel that `Fix` opens showed Claude Code passing with nothing to click. On a machine whose login shell exports `ANTHROPIC_API_KEY`, the probe reports authenticated regardless of OAuth state and the gate could never open at all. This closes the `authRecovery.ts` comment from the a190fa9 review. Doctor gains the static capability the gate needs: - `DoctorCheck::login_command` is filled from `AgentCheckInfo::auth_command` on every branch where the binary resolved — Goose's three probe arms and its timeout builder included — and is `None` for a missing binary, for a present agent whose bridge is missing (the actionable problem there is the bridge), and for providers without a login command. `fix_type` / `fix_command` are untouched: still set only in the `NotAuthenticated` arm, so the Doctor panel's `Fix` semantics don't move and Pi and Goose never grow a button. Serde camelCase (`loginCommand`), `#[serde(default)]`, and additive on the wire: a consumer pinning doctor by git rev reads an older payload's absence as `None`, and an older consumer ignores the new key. Every in-repo constructor is updated — 26 struct literals plus the `TimeoutCheck` builder — including the node-runtime check in `apps/staged/src-tauri`, which is workspace-excluded and so was `cargo check`ed explicitly. Staged reads it and stops treating the probe's positive verdict as truth: - `canOfferLogin` requires the check, its `loginCommand`, and an `authStatus` other than `unknown`. `authenticated` is no longer an exclusion — a fresh authentication failure from this session's own agent process outranks a probe that verifiably does not look at expiry — and `notApplicable` (Copilot: a login command with no status probe) qualifies. `unknown` stays excluded for the original reason: the binary wasn't on the login shell's PATH or the probe never ran, and a login through the same binary would fail the same way. The gate reads three things and stores none — the live error, a compile-time constant in doctor, and the probe used only negatively — so Staged still keeps no authentication memory of its own and the vendor remains the sole source of truth; a future CLI that checks expiry needs no change here. - In the session alert `Log in`, when offered, is now the primary action and leads; `Fix` becomes the outlined secondary. The failure came from the agent itself, and the panel behind `Fix` may well call the check passing. - Deliberately not done: a `Log in` button on a passing Doctor row. Without a live error nothing says the passing verdict is wrong, and a login button on a green row would be a standing invitation to re-authenticate a working install. The row keeps offering `Fix` exactly when the probe positively reported a signed-out agent. Tests. Rust, in `agents.rs`: a fake `claude-agent-acp` run through a dotfile-free login shell pins the field across the probe's exit 0 (`Authenticated`, `Pass`, no fix, command present — the expired-token shape), exit 1 (`NotAuthenticated`, an `Auth` fix equal to the command), and exit 127 (`Unknown`, command present, no fix — withholding on `unknown` is the host's policy, not doctor's); Copilot's `NotApplicable` carries its command without shelling out; Pi (both binaries resolved) and Goose (spawn-failure arm) have none; an uninstalled Claude and a bridge-less Amp have none; and a serde round-trip checks the camelCase key and that a payload without it parses to `None`. Frontend, `authRecovery.test.ts`: the full matrix — signed out, authenticated with a login command, `notApplicable`, fix fields empty, `unknown`, no check, and Pi/Goose with no login command — replacing the single three-line case. Verified with `cargo test` in crates/doctor (151 passed), `cargo fmt --check` and `cargo clippy --all-targets`; `vitest run` in apps/staged (74 files, 934 passed), `svelte-check --tsconfig ./tsconfig.app.json --fail-on-warnings` (0 errors, 0 warnings) and `prettier --check`; and `cargo check`, `cargo clippy` and `cargo fmt --check` in apps/staged/src-tauri. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
16912ea left one window open and said so: the backend releases a check's login slot *before* it emits the run's `done`, so a fresh `start_doctor_login` for the same check can be claimed between the two, and that client — correlating events by check id — then takes the earlier run's `done` as its own end. The record settles, the code box vanishes, and the CLI that just printed its URL is left running with nobody to type into it. The same identity gap let a code typed for a login that had ended be delivered to whichever login now held the slot, and a cancel aimed at one run kill another client's newer one. The slot release could not move: it is what guarantees a late attacher that a `running` snapshot was taken ahead of the `done` still to come. Give each run an identity the check id never had: - `claim_login` mints a UUID v4 `run_id` for the entry it inserts. A UUID rather than a counter because a web client's record can outlive a backend restart, and a restarted counter would hand a new run an id a stale record still holds. `claim_login` now returns a `LoginClaim` — `Claimed(login)` or `AlreadyRunning { run_id }` — so the "already running" answer can name the run it found. - `start_doctor_login` returns `LoginStart::Started { run_id }` or `AlreadyRunning { run_id }`, serialized as a tagged object (`{ outcome, runId }`) — one type on the wire, the same shape both ways. `doctor_login_status` carries `run_id: Option<String>`, `None` when idle. Every `doctor-login-output` event carries `run_id`; `seq` stays per run, which is what it always counted. - `send_doctor_login_code` and `cancel_doctor_login` take `(check_id, run_id)`. `find_login_run` resolves the pair against the check-keyed map and tells `Ended` (nothing running) from `Superseded` (a different run holds the slot). A code for either is refused — the refusal names which, and for `Superseded` says the code was not delivered to the newer login. A cancel for either answers `false` and leaves the newer run alone: it is someone else's login, and `false` is the caller's cue that its own is gone. - `LoginSlot` carries its `run_id` and releases the entry only while the entry is still that run's. Call order already made this true — a later claim needs the entry gone first — but the release is now keyed to the run by construction rather than by sequence. The frontend record follows the run, not the check: - `AgentLoginState.runId` and `Attempt.runId`, null until the backend names the run: the start's answer for a start (both outcomes), the status snapshot for an attach. `handleEvent` keeps the check id as a cheap pre-filter and drops any event whose `runId` is not the followed run's. `syncFromBackend` treats a status naming a *different* run the same as nothing running — the followed run's `done` was missed, and the run that replaced it is not this record's to show — and settles `completed` as before. - Events that arrive before the run is named are held, not judged by check id, because that window is precisely where the earlier run's `done` lands. `adoptRun` sets the id and replays what was held through the same filter, so the stale `done` is dropped and the new run's own first lines are shown. A start's answer normally beats the fix's first line (the reply goes out before the spawned task has even resolved the shell env), but the hold makes it exact rather than likely. Bounded to `MAX_OUTPUT_LINES`. - `submitAgentLoginCode` and `cancelAgentLogin` name the run. A cancel clicked before the start has answered waits on `runIdKnown` rather than being dropped: cancelling "whatever runs for the check" could hit another client's newer login, and a dropped cancel is the failure 16912ea fixed — the CLI holding the slot until doctor's fix timeout. `finish` resolves that promise with null so a waiter is released when the attempt ends unnamed. The `probing`-time `done` → `abandon` branch in `handleEvent` is gone: a probing attempt has no run id yet, so its events are held, and the probe's own answer decides — which also stops an earlier run's late `done` from abandoning a probe that would have found the current run running. `AgentLoginPrompt`, `DoctorCheckRow` and `SessionChatPane` are untouched: they read `checkId` and `running` and call the store's functions, and the run id stays inside the record. `web_server.rs` reads the new `runId` argument for the two commands. crates/doctor is untouched. Tests. Rust, in `doctor::tests`: `find_login_run` over a local map tells `Ended` from `Superseded` and pins both refusals; run ids are unique; a stale `LoginSlot`'s drop leaves a live run's entry alone; `LoginStart`, the status and the `done` event serialize `runId` in camelCase with the tagged outcome; and the slot lifecycle test now checks that a second claim names the running run, that a code and a cancel for another run id are refused without reaching the run while the right id reaches it, that a code for a run that has ended says so, and that the retry gets a different id. Store suite: a `done` for the same check from an earlier run — failed, cancelled, or plain — does not settle the current run; an earlier run's line is neither shown nor counted; the exact race — the earlier run's `done` then the new run's first line, both landing before the start's answer — leaves the new run running with its line shown; a code for a run the backend has replaced is refused with the backend's message and the following cancel re-syncs and settles without adopting the newer run's tail; a cancel before the start is answered waits for the run id; re-attach and attach-on-open take the run id from the start answer and the status and send the code under it; a re-attach whose run the backend has replaced settles; events held during a probe are filtered to the found run; and every existing case sends and cancels with the run id. Verified with `vitest run` in apps/staged (74 files, 941 passed), `svelte-check --tsconfig ./tsconfig.app.json --fail-on-warnings` (0 errors, 0 warnings), `prettier --check src`, and `cargo test doctor::tests` (15 passed), `cargo check`, `cargo clippy` and `cargo fmt --check` in apps/staged/src-tauri. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…iccups The review of ffffa30 found two ways the UI could end, or misreport, a login that was alive and being watched, one way it could drop lines, and one false positive in the widened `Log in` gate worth writing down. Frontend only; crates/doctor and src-tauri are untouched. **The fix dialog's close cancelled logins it did not start.** `cancelFix` killed whatever login the shared record showed running for the check, and since 16912ea `promptFix` *attaches* on open to a login already running — one the user began from the session pane and is still watching there. Opening the Doctor row's Fix dialog for a look and pressing Escape, or clicking outside, killed that login out from under the pane. The doc comment's premise, "a login nobody is watching", was true only for the run `confirmFix` started. - `loginStartedHere` is set in `confirmFix` and cleared in `promptFix`, and the close decision is a pure helper, `closingFixDialogCancelsLogin` in the new `fixDialog.ts`: cancel a running login this dialog started, detach from one it only attached to. There is no component test infrastructure (vitest runs without the Svelte plugin), so the helper is what carries the test. - Detaching makes a path reachable that the unconditional cancel never left open: close the dialog on an attached login, re-open it, and it attaches again — two followers of one login, and `onFixed` twice when it completes, which is two full doctor scans since `runChecks` has no concurrency guard. `followLogin` now notes which open of the dialog it belongs to and stands down if the dialog has been re-opened since, so the end is acted on once. `fixing` is cleared explicitly on the two paths that still own it instead of in a `finally` a stale follower would also run. **A reconnect during an unanswered start dropped its lines.** `resync` returned early while `attempt.answered` was false, on the reasoning that the answer still to come covered the gap — true of an `alreadyRunning` answer, which syncs, and of a probe's, which is a snapshot, but not of a `started` answer, which only replays the events this client itself received. Lines the backend emitted while the socket was down — the fix's first ones, so possibly `If the browser didn't open, visit: …` — were gone until the next reconnect or a cancel. - `resync` now records the gap on the attempt (`gapBeforeAnswer`) instead of dropping it, and a `started` answer catches up from the backend when it is set, through the same `syncFromBackend` the `alreadyRunning` branch uses. - Not unconditional, though the review allowed it and the snapshot does merge by `seq`. `syncFromBackend` settles the record as `completed` when the status finds the run not running — right for a run whose `done` is gone, wrong for one whose `done` is merely behind the status answer. A fix that fails fast (an unresolvable command, a spawn failure) can have released its slot before the status is served, and an unconditional sync would then swallow the `done` and the error it carries for a blank `completed`. With no gap there is nothing to fetch: this listener was live before the fix existed, so every line is already on its way. The suite's default status mock answers "not running", which is the racing shape, and thirteen existing tests fail under the unconditional variant. **A failed post-adopt sync failed a live login.** The `.catch` on the start chain covered the `alreadyRunning` branch's awaited sync as well, so a status rejection right after the backend confirmed the run alive tore down the listener and showed an error for a login that was still waiting for its code. `resync` already treated the same failure as a warning. - The start uses a two-argument `then`, so only the start's own rejection is the login's failure. The sync goes through a new `catchUp`, which `resync` now shares: warn and keep following, since the record still describes a login the backend confirmed and the next event or reconnect catches up. **The gate's known false positive is now on record.** `canOfferLogin`'s doc names the one `authenticated` shape that is wrong in the other direction: a login shell exporting `ANTHROPIC_API_KEY`, on which the Claude CLI reports itself logged in whatever the OAuth state, while an OAuth login changes nothing about the exported key the agent's next session starts with. It stays offered because `authenticated` cannot be split — the exit code is all doctor reports, and it is the same 0 for the expired token the widening was for (the `auth status` probe note) — and the comment says what a future fix keys on: a doctor-side credential-source field lifted from the probe's JSON, which does tell the two apart (`apiKeySource`, `authMethod`), rather than re-excluding `authenticated` wholesale. No behaviour change. Tests, each of the store's three confirmed non-vacuous by reverting its half of the fix: a reconnect while the start is unanswered asks nothing at the time, and the `started` answer then fetches the tail, restoring the URL, with later lines merging by `seq`; a start answered with no gap asks nothing, and a fast failure's `done` keeps its error — this is the test the unconditional variant fails; a rejected catch-up after `alreadyRunning` leaves the record running with its run id and listener, warns, still shows lines and sends a code, and the next reconnect fetches what the failed one could not. The helper's suite pins started-here as cancel, attached as detach, and nothing running as nothing to do. Verified with `vitest run` in apps/staged (75 files, 947 passed), `svelte-check --tsconfig ./tsconfig.app.json --fail-on-warnings` (0 errors, 0 warnings), and `prettier --check src`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…catch up after a probe gap The review of 0d4b08a left two warnings, a suggestion and a note it called cheap to close. All four are frontend; crates/doctor and src-tauri are untouched. **The dialog called a login its own before the backend had said so.** `loginStartedHere` was set in `confirmFix`, ahead of `startDoctorLogin`'s answer, and that answer can be `alreadyRunning`: the dialog's probe on open found nothing, someone else — another client, or the session pane on a web client elsewhere — started a login for the check before Run was clicked, and the click re-attached to theirs. The store hides that difference on purpose (a re-attach is not a failure), so the dialog showed the URL and code box marked started-here, and Escape killed a login someone else was watching: the bug 0d4b08a fixed, through a narrower door. - The record gains `origin` (`started` / `attached` / null), set in the two answer handlers — the start's from its outcome, the probe's always `attached` — and null until an answer lands or when nothing is followed. Set before `adoptRun`, since the held-events replay can end the attempt. A finished login keeps it, like the rest of what it leaves behind; a cancelled one is reset with the record. - The dialog's flag becomes `loginRequestedHere`: only what the dialog knows on its own, that its Run was confirmed. `closingFixDialogCancelsLogin` reads it together with the record's `origin` — `running && requestedHere && origin !== 'attached'` — at close time rather than answer time, which the dialog has no hook for: it awaits the login's *end*. The dialog's own flag cannot go, because the pane's start reads `started` on the shared record and a dialog that attached on open to that login has to know it did not make it. - A request the backend has yet to answer is cancelled on close. It is this dialog's, and the store's cancel already waits for the answer to name the run. Should that answer be "already running", the kill lands on a login the user asked to start a moment ago and then left — the review's ambiguous shape, where a reload whose probe failed makes it their own lost login. Resolved for cancelling: the other reading leaves a slot the user has walked away from held until doctor's fix timeout, and the window is the backend's answer, not a human's. **A follower whose dialog was left still refreshed the panel.** The re-open stand-down in `followLogin` covers a follower superseded by a newer open of the dialog. A dialog closed by *detach* and never re-opened left its follower current, so when the pane's login completed it fired `onFixed` (`runChecksAndRefresh`) beside the pane's own `runChecks`: two full doctor scans at once, through a dialog the user had already left. Newly reachable, because detach no longer kills the login. - `followLogin` stands down when `showFixDialog` is already false at resolution: a dialog that was left has handed the login back to whoever is still watching it, and that watcher refreshes. Chosen over firing `onFixed` only for a login this dialog started because an attached login that completes while the dialog is *open* still closes the dialog, and closing it onto a row that then stays stale — another client's login, no pane here to refresh — would be the worse end; the open-dialog overlap with the pane is pre-existing and the review's acknowledged trade. The `fixDialogOpens` guard stays, since a re-opened dialog is open at resolution for both of its followers. Re-traced: detach then re-open, a cancel pending across a re-open, and a stale probe from an earlier open all still act on the end once. For a login this dialog started and then left, leaving cancelled it and a cancelled end refreshed nothing anyway — unless the cancel found nothing or failed, in which case the closed dialog no longer refreshes a row the next open re-attaches to or the user re-runs. **The probe's answer ignored an observed gap.** `resync` recorded a reconnect during an unanswered attach in `gapBeforeAnswer`, whose doc said a probe's answer "is a snapshot and needs nothing more". That holds only if the backend took the snapshot after the re-subscribe. In web mode `doctorLoginStatus` is an HTTP fetch and events ride the socket, so the snapshot can be taken, the socket drop and come back, and the response land last; lines emitted between the snapshot and the reconnect are in neither, and the next live line moves `nextSeq` past them for good. - After `applySnapshot`, the probe's handler catches up when a gap was seen — the one-liner the `started` branch got in 0d4b08a. Redundant when the snapshot post-dates the gap (the merge is by `seq`), and the fast-failure trade is the one the `alreadyRunning` branch already accepts for a run the backend just confirmed alive. The field's doc now says what both answers do with it and why a snapshot alone is not enough. **The note that was cheap to close.** The two-argument `then` from 0d4b08a keeps a catch-up rejection out of `fail`, but also let a synchronous throw in the success handler — the held-events replay — escape as an unhandled rejection with `running` left true. The handler's body is now in a `try` whose `catch` calls `fail`, as the probe's `.catch` already does. Tests. Store suite: the probe's answer after a reconnect while the status was in flight asks the backend again and shows the line the first snapshot predates, and the no-gap attach test now pins a single ask; `origin` is null until the start's answer, `started` after it and kept by the finished login, null again on the next start and `attached` once that one is answered "already running", `attached` for an attach on open, and null after a cancelled end; and a fault-injection case has the record refuse the run id the answer hands it, which without the guard is a 5s test timeout plus an "Unhandled Rejection" from vitest, and with it a rejected start and `running` false. Both behavioural additions were confirmed non-vacuous by disabling their fix. Helper suite: the started case, the pending case, the request answered "already running", the attached case over every origin, and nothing running over every combination. There is no component test infrastructure for `DoctorCheckRow` (vitest runs without the Svelte plugin), so the stand-down rests on the tracing above, as the Cancel/Escape path did. Verified with `vitest run` in apps/staged (75 files, 952 passed), `svelte-check --tsconfig ./tsconfig.app.json --fail-on-warnings` (0 errors, 0 warnings), and `prettier --check src`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds opt-in piped stdin to the doctor crate's streaming fix execution, so a host can feed input (e.g. pasting an auth code into a login flow) to a running fix subprocess.
Changes
FixStdin::pipe()returns a connected(FixStdinWriter, FixStdin)pair. The caller keeps the cloneable writer and puts theFixStdininExecuteFixOptions::stdin(or uses.with_stdin(..)).FixStdinWriter::send_linequeues a line (trailing newline appended, pipe flushed) and returnsErronce the fix's stdin is closed. Lines sent before the child spawns are buffered and delivered on spawn; dropping every writer clone closes the child's stdin (EOF).stdin: Nonethe child keeps inheriting the host process's stdin, so interactive fixes in terminal hosts are unchanged.execute_fix_optionsfor the new field; its fixes are non-interactive, so they keep inherited stdin.Testing
Three new tests cover the round trip through
cat(including pre-spawn buffering and EOF-on-drop), the prompt-styleread -rshape fed while the fix runs, and the no-hang path when a writer outlives the child. Fulljust app staged ci(739 Rust + 669 frontend tests) and the crates fmt/lint/test suites pass.