Marketplace gh login: tear down the container login on every failure

A lost stream or a failed Enter write returned without killing the
in-container gh, leaving it polling with a temp GH_CONFIG_DIR that would
receive the token. The output loop is now drive_login (generic over the
stream, writer and emitter, so it is unit-tested without Docker), and
its result goes through cleanup_on_error, so every ending except a token
read back runs the pkill. Neutral wording for the shared ANSI stripper's
overflow warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

app/src-tauri/src/commands/auth_token_commands.rs

@@ -644,7 +644,7 @@ impl AnsiStripper {
         // fresh chunk, which re-enters here.
         if self.carry.len() > MAX_ANSI_CARRY {
             log::warn!(
-                "`claude setup-token` emitted an unterminated control sequence \
+                "the command emitted an unterminated control sequence \
                  longer than {} bytes — treating it as text",
                 MAX_ANSI_CARRY
             );

app/src-tauri/src/marketplace/gh_login.rs

@@ -5,9 +5,10 @@
 
 use std::time::Duration;
 
-use futures_util::StreamExt;
+use bollard::container::LogOutput;
+use futures_util::{Stream, StreamExt};
 use tauri::{AppHandle, Emitter};
-use tokio::io::AsyncWriteExt;
+use tokio::io::{AsyncWrite, AsyncWriteExt};
 use tokio::sync::oneshot;
 
 use crate::commands::auth_token_commands::{push_capped_tail, AnsiStripper, SUBMIT_ENTER_DELAY};
@@ -134,43 +135,49 @@ fn failure_tail(transcript: &str) -> String {
     lines[lines.len().saturating_sub(5)..].join("\n")
 }
 
-/// Pre-flight N9: stop the in-container login after a cancel or timeout.
+/// Pre-flight N9 / review fix 1: stop the in-container login after any
+/// failed attempt.
 async fn kill_container_login(container_id: &str) {
     let cmd = CANCEL_PKILL.iter().map(|s| s.to_string()).collect();
     let _ = exec_oneshot_as(container_id, "claude", cmd, vec![]).await;
 }
 
-/// Run `gh auth login --web` in the container and return the token it minted.
-pub async fn run_gh_container_login(
-    app: &AppHandle,
-    account_id: &str,
-    container_id: &str,
-    host: &str,
-    mut cancel: oneshot::Receiver<()>,
-) -> Result<String, String> {
-    if !valid_host(host) {
-        return Err(format!("{host:?} is not a valid host name."));
+/// Hand `result` back, running `cleanup` first when it is a failure.
+///
+/// Review fix 1: a tty exec keeps running after its attach is dropped, so any
+/// login that ends without a token — cancel, timeout, a lost stream, a failed
+/// write, gh exiting without one — must stop the in-container login, or gh
+/// keeps polling and its temp `GH_CONFIG_DIR` (which receives the token if the
+/// user finishes in the browser) outlives the attempt.
+async fn cleanup_on_error<T, C, Fut>(result: Result<T, String>, cleanup: C) -> Result<T, String>
+where
+    C: FnOnce() -> Fut,
+    Fut: std::future::Future<Output = ()>,
+{
+    if result.is_err() {
+        cleanup().await;
     }
-    let AttachedExec {
-        exec_id,
-        mut output,
-        mut input,
-    } = create_attached_exec_as(
-        container_id,
-        vec![
-            "sh".to_string(),
-            "-c".to_string(),
-            GH_LOGIN_SCRIPT.to_string(),
-            "triple-c-gh-login".to_string(),
-            host.to_string(),
-        ],
-        true,
-        "claude",
-        "/home/claude",
-    )
-    .await?;
+    result
+}
 
-    let deadline = tokio::time::Instant::now() + LOGIN_TIMEOUT;
+/// Pump gh's output until the exec ends, emitting code and display events and
+/// pressing Enter at gh's prompt. `Ok` is the transcript of a stream that ended
+/// normally; every other ending is `Err`. Takes the attach halves by value, so
+/// they are closed by the time this returns.
+async fn drive_login<S, W, E>(
+    mut output: S,
+    mut input: W,
+    cancel: &mut oneshot::Receiver<()>,
+    deadline: tokio::time::Instant,
+    account_id: &str,
+    host: &str,
+    mut emit: E,
+) -> Result<String, String>
+where
+    S: Stream<Item = Result<LogOutput, bollard::errors::Error>> + Unpin,
+    W: AsyncWrite + Unpin,
+    E: FnMut(&'static str, serde_json::Value),
+{
     let mut stripper = AnsiStripper::default();
     let mut transcript = String::new();
     let mut pending = String::new();
@@ -179,18 +186,12 @@ pub async fn run_gh_container_login(
 
     loop {
         let next = tokio::select! {
-            _ = &mut cancel => {
-                drop(output);
-                drop(input);
-                kill_container_login(container_id).await;
+            _ = &mut *cancel => {
                 return Err("GitHub sign-in cancelled. Nothing was stored.".to_string());
             }
             next = tokio::time::timeout_at(deadline, output.next()) => match next {
                 Ok(next) => next,
                 Err(_) => {
-                    drop(output);
-                    drop(input);
-                    kill_container_login(container_id).await;
                     return Err(format!(
                         "Timed out after {} minutes waiting for the GitHub sign-in. Nothing was stored.",
                         LOGIN_TIMEOUT.as_secs() / 60
@@ -205,21 +206,21 @@ pub async fn run_gh_container_login(
                     "Lost the connection to gh: {e}. Nothing was stored."
                 ))
             }
-            None => break,
+            None => return Ok(transcript),
         };
         let text = stripper.push(&frame.into_bytes());
         push_capped_tail(&mut transcript, &text, MAX_TRANSCRIPT);
 
         let shown = take_display_lines(&mut pending, &text);
         if !shown.is_empty() {
-            let _ = app.emit(
+            emit(
                 OUTPUT_EVENT,
                 serde_json::json!({ "account_id": account_id, "chunk": shown }),
             );
         }
         if !code_sent {
             if let Some((code, url)) = parse_device_prompt(&transcript, host) {
-                let _ = app.emit(
+                emit(
                     CODE_EVENT,
                     serde_json::json!({ "account_id": account_id, "code": code, "url": url }),
                 );
@@ -238,18 +239,70 @@ pub async fn run_gh_container_login(
             enter_sent = true;
         }
     }
+}
 
-    let status = wait_for_exec_exit(&exec_id).await;
-    if let Some(token) = extract_token(&transcript) {
-        return Ok(token);
+/// Run `gh auth login --web` in the container and return the token it minted.
+///
+/// Once the exec exists there is exactly one way out: the result of the inner
+/// block goes through [`cleanup_on_error`], so only a token read back skips
+/// the in-container kill.
+pub async fn run_gh_container_login(
+    app: &AppHandle,
+    account_id: &str,
+    container_id: &str,
+    host: &str,
+    mut cancel: oneshot::Receiver<()>,
+) -> Result<String, String> {
+    if !valid_host(host) {
+        return Err(format!("{host:?} is not a valid host name."));
     }
-    Err(format!(
-        "gh did not complete the sign-in (exit status {}). Nothing was stored.\n{}",
-        status
-            .map(|c| c.to_string())
-            .unwrap_or_else(|| "unknown".to_string()),
-        failure_tail(&transcript)
-    ))
+    let AttachedExec {
+        exec_id,
+        output,
+        input,
+    } = create_attached_exec_as(
+        container_id,
+        vec![
+            "sh".to_string(),
+            "-c".to_string(),
+            GH_LOGIN_SCRIPT.to_string(),
+            "triple-c-gh-login".to_string(),
+            host.to_string(),
+        ],
+        true,
+        "claude",
+        "/home/claude",
+    )
+    .await?;
+
+    let deadline = tokio::time::Instant::now() + LOGIN_TIMEOUT;
+    let result = async {
+        let transcript = drive_login(
+            output,
+            input,
+            &mut cancel,
+            deadline,
+            account_id,
+            host,
+            |event, payload| {
+                let _ = app.emit(event, payload);
+            },
+        )
+        .await?;
+        if let Some(token) = extract_token(&transcript) {
+            return Ok(token);
+        }
+        let status = wait_for_exec_exit(&exec_id).await;
+        Err(format!(
+            "gh did not complete the sign-in (exit status {}). Nothing was stored.\n{}",
+            status
+                .map(|c| c.to_string())
+                .unwrap_or_else(|| "unknown".to_string()),
+            failure_tail(&transcript)
+        ))
+    }
+    .await;
+    cleanup_on_error(result, || kill_container_login(container_id)).await
 }
 
 #[cfg(test)]
@@ -378,6 +431,173 @@ mod tests {
         assert!(GH_LOGIN_SCRIPT.contains(CANCEL_PKILL[2]));
     }
 
+    /// Review fix 1: every failed login tears the container side down, and a
+    /// successful one does not.
+    mod teardown {
+        use super::super::*;
+        use bollard::container::LogOutput;
+        use futures_util::stream;
+        use std::pin::Pin;
+        use std::sync::{Arc, Mutex};
+        use std::task::{Context, Poll};
+
+        type Frame = Result<LogOutput, bollard::errors::Error>;
+
+        fn out(s: &'static str) -> Frame {
+            Ok(LogOutput::StdOut { message: s.into() })
+        }
+
+        fn lost() -> Frame {
+            Err(bollard::errors::Error::DockerResponseServerError {
+                status_code: 500,
+                message: "connection reset".to_string(),
+            })
+        }
+
+        /// Records every write separately; or fails every write.
+        #[derive(Clone, Default)]
+        struct Keys {
+            writes: Arc<Mutex<Vec<Vec<u8>>>>,
+            broken: bool,
+        }
+
+        impl tokio::io::AsyncWrite for Keys {
+            fn poll_write(
+                self: Pin<&mut Self>,
+                _: &mut Context<'_>,
+                buf: &[u8],
+            ) -> Poll<std::io::Result<usize>> {
+                if self.broken {
+                    return Poll::Ready(Err(std::io::Error::other("pipe closed")));
+                }
+                self.writes.lock().unwrap().push(buf.to_vec());
+                Poll::Ready(Ok(buf.len()))
+            }
+            fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<std::io::Result<()>> {
+                Poll::Ready(Ok(()))
+            }
+            fn poll_shutdown(
+                self: Pin<&mut Self>,
+                _: &mut Context<'_>,
+            ) -> Poll<std::io::Result<()>> {
+                Poll::Ready(Ok(()))
+            }
+        }
+
+        const PROMPT: &str = "! First copy your one-time code: 4F2A-9C1B\r\nPress Enter to open github.com in your browser... ";
+
+        async fn drive<S>(
+            frames: S,
+            keys: Keys,
+            cancel: &mut oneshot::Receiver<()>,
+            deadline: tokio::time::Instant,
+        ) -> (
+            Result<String, String>,
+            Vec<(&'static str, serde_json::Value)>,
+        )
+        where
+            S: futures_util::Stream<Item = Frame> + Unpin,
+        {
+            let mut events = Vec::new();
+            let r = drive_login(
+                frames,
+                keys,
+                cancel,
+                deadline,
+                "acct-1",
+                "github.com",
+                |e, p| events.push((e, p)),
+            )
+            .await;
+            (r, events)
+        }
+
+        fn far() -> tokio::time::Instant {
+            tokio::time::Instant::now() + LOGIN_TIMEOUT
+        }
+
+        #[tokio::test]
+        async fn cleanup_runs_on_every_failure_and_never_on_success() {
+            let runs = Arc::new(Mutex::new(0));
+            let count = || {
+                let runs = runs.clone();
+                async move { *runs.lock().unwrap() += 1 }
+            };
+            let ok: Result<String, String> = Ok("test-token-not-real".into());
+            assert!(cleanup_on_error(ok, count).await.is_ok());
+            assert_eq!(*runs.lock().unwrap(), 0);
+            let err: Result<String, String> = Err("boom".into());
+            assert_eq!(cleanup_on_error(err, count).await, Err("boom".into()));
+            assert_eq!(*runs.lock().unwrap(), 1);
+        }
+
+        #[tokio::test(start_paused = true)]
+        async fn a_complete_login_returns_the_transcript_and_presses_enter_alone() {
+            let keys = Keys::default();
+            let (_tx, mut cancel) = oneshot::channel();
+            let frames = stream::iter(vec![
+                out(PROMPT),
+                out("\r\n\u{2713} Logged in\r\n"),
+                out("__TRIPLEC_TOKEN_BEGIN__test-token-"),
+                out("not-real__TRIPLEC_TOKEN_END__\r\n"),
+            ]);
+            let (r, events) = drive(frames, keys.clone(), &mut cancel, far()).await;
+            let transcript = r.unwrap();
+            assert_eq!(
+                extract_token(&transcript),
+                Some("test-token-not-real".into())
+            );
+            assert_eq!(*keys.writes.lock().unwrap(), vec![b"\r".to_vec()]);
+            assert!(events.contains(&(
+                CODE_EVENT,
+                serde_json::json!({
+                    "account_id": "acct-1",
+                    "code": "4F2A-9C1B",
+                    "url": "https://github.com/login/device"
+                })
+            )));
+            for (_, payload) in &events {
+                assert!(!payload.to_string().contains("test-token-not-real"));
+            }
+        }
+
+        #[tokio::test]
+        async fn a_lost_stream_is_a_failure() {
+            let (_tx, mut cancel) = oneshot::channel();
+            let frames = stream::iter(vec![out(PROMPT), lost()]);
+            let (r, _) = drive(frames, Keys::default(), &mut cancel, far()).await;
+            assert!(r.unwrap_err().contains("Lost the connection"));
+        }
+
+        #[tokio::test(start_paused = true)]
+        async fn a_failed_enter_is_a_failure() {
+            let keys = Keys {
+                broken: true,
+                ..Default::default()
+            };
+            let (_tx, mut cancel) = oneshot::channel();
+            let frames = stream::iter(vec![out(PROMPT)]);
+            let (r, _) = drive(frames, keys, &mut cancel, far()).await;
+            assert!(r.unwrap_err().contains("Could not answer"));
+        }
+
+        #[tokio::test]
+        async fn a_cancel_is_a_failure() {
+            let (tx, mut cancel) = oneshot::channel();
+            tx.send(()).unwrap();
+            let (r, _) = drive(stream::pending(), Keys::default(), &mut cancel, far()).await;
+            assert!(r.unwrap_err().contains("cancelled"));
+        }
+
+        #[tokio::test(start_paused = true)]
+        async fn a_timeout_is_a_failure() {
+            let (_tx, mut cancel) = oneshot::channel();
+            let deadline = tokio::time::Instant::now() + Duration::from_secs(1);
+            let (r, _) = drive(stream::pending(), Keys::default(), &mut cancel, deadline).await;
+            assert!(r.unwrap_err().contains("Timed out"));
+        }
+    }
+
     /// The script end to end against a stand-in `gh`, as a login would run it
     /// inside the container (minus Docker).
     #[cfg(unix)]