feat(minimald): handle git pushes natively - #347

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimald/src/exec.rs (2)

771-785: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stop on .git probe/init failures before continuing.

try_exists errors are still ignored, a non-zero git init exit still falls through to receive-pack, and channel.close().await.unwrap() can panic in the error path. That leaves this branch able to continue with an invalid repo state or turn a recoverable failure into a task panic.

Suggested fix

-        if let Ok(false) = tokio::fs::try_exists(&dotgit_dir).await {
-            let res = tokio::process::Command::new("git")
-                .arg("init")
-                .current_dir(workspace.as_utf8_path())
-                .output()
-                .await;
-            if let Err(e) = res {
-                tracing::warn!(error = %e, "git init failed");
-                channel.close().await.unwrap();
-                return;
-            }
-        }
+        match tokio::fs::try_exists(&dotgit_dir).await {
+            Ok(false) => match tokio::process::Command::new("git")
+                .arg("init")
+                .current_dir(workspace.as_utf8_path())
+                .output()
+                .await
+            {
+                Ok(out) if out.status.success() => {}
+                Ok(out) => {
+                    tracing::warn!(status = %out.status, stderr = %String::from_utf8_lossy(&out.stderr), "git init failed");
+                    let _ = channel.close().await;
+                    return;
+                }
+                Err(e) => {
+                    tracing::warn!(error = %e, "git init failed");
+                    let _ = channel.close().await;
+                    return;
+                }
+            },
+            Ok(true) => {}
+            Err(e) => {
+                tracing::warn!(error = %e, "failed to check .git existence");
+                let _ = channel.close().await;
+                return;
+            }
+        }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/exec.rs` around lines 771 - 785, The probe/init branch
currently ignores errors from tokio::fs::try_exists and a non-zero git init
result, and it calls channel.close().await.unwrap() which can panic; change the
logic in the block around tokio::fs::try_exists,
Command::new("git").arg("init").output().await and the subsequent error handling
so that: if try_exists returns Err treat it as a hard failure (log and stop
processing), when .output().await returns Ok check output.status.success() and
treat non-success as an error (log stderr/stdout and stop), and replace
channel.close().await.unwrap() with a non-panicking close (e.g. .await.ok() or
handle the Result) before returning instead of falling through to receive-pack.
Ensure logs include the command output for debugging.

---

646-648: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply the PTY guard before routing git-receive-pack.

This early return still bypasses the unsupported-PTY rejection at Lines 650-654, so git pushes can take a path normal execs explicitly reject.

Suggested fix

 if let Some(ident) = argv.strip_prefix("git-receive-pack min://") {
+    if config.pty.is_some() {
+        tracing::warn!("channel {id}: pty requested but not yet supported");
+        session.channel_failure(id)?;
+        return Ok(());
+    }
     return handle_git_receive(ident, serv, conn, id, session, channel, config).await;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/exec.rs` around lines 646 - 648, The early return for
git-receive-pack bypasses the PTY rejection guard; move or invoke the PTY check
that's currently executed after this block (the unsupported-PTY rejection)
before the match that tests argv.strip_prefix("git-receive-pack min://"), so
that when `if let Some(ident) = argv.strip_prefix("git-receive-pack min://")`
triggers the code still runs the PTY guard first; ensure
`handle_git_receive(...)` is only called after the PTY validation passes (reuse
the same guard logic/condition used later to reject unsupported PTY).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimald/src/test_harness.rs (2)

115-116: ⚡ Quick win

Document the socket path precondition or handle cleanup.

The UnixListener::bind will panic if sock already exists (e.g., from a previous test run that didn't clean up). Consider either documenting that the caller must ensure the path is clean, or adding explicit cleanup:

let _ = std::fs::remove_file(sock); // Ignore error if not present
let listener = UnixListener::bind(sock).unwrap();

This helps prevent test flakes when socket files persist between runs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/test_harness.rs` around lines 115 - 116, The
listen_on_uds function currently calls UnixListener::bind(sock) which will panic
if the socket path already exists; update listen_on_uds to either document that
callers must ensure sock is removed before calling or proactively remove any
existing socket file before binding (e.g., call std::fs::remove_file(sock) and
ignore NotFound errors) so UnixListener::bind(sock) succeeds reliably and avoids
test flakes.

---

119-130: 💤 Low value

Consider returning the outer JoinHandle for test control.

The spawned accept loop is fire-and-forget, which limits test flexibility:

• No way to await listener shutdown
• Accept errors are silently discarded (while let Ok(...) stops on first failure)
• No mechanism to verify the listener is ready before clients connect

For more robust test infrastructure, consider returning the JoinHandle and logging accept errors:

♻️ Suggested enhancement

-pub(crate) async fn listen_on_uds(&self, sock: &Path) {
+pub(crate) async fn listen_on_uds(&self, sock: &Path) -> tokio::task::JoinHandle<()> {
     let listener = UnixListener::bind(sock).unwrap();
     let russh_config = self.russh_config.clone();
     let state = self.state.clone();
-    tokio::spawn(async move {
+    tokio::spawn(async move {
-        while let Ok((socket, _)) = listener.accept().await {
+        loop {
+            let (socket, _) = match listener.accept().await {
+                Ok(conn) => conn,
+                Err(e) => {
+                    eprintln!("UDS accept failed: {e}");
+                    break;
+                }
+            };
             let russh_config = russh_config.clone();
             let state = state.clone();
             tokio::spawn(async move {

This allows tests to join the accept loop if needed and makes failures visible.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/test_harness.rs` around lines 119 - 130, The accept loop
currently spawned with tokio::spawn is fire-and-forget which drops control and
silences accept errors; change the code that creates the listener accept loop to
return its outer JoinHandle (instead of discarding it) so callers/tests can
await shutdown or inspect panics, replace the `while let Ok((socket, _)) =
listener.accept().await` pattern with an explicit loop that logs accept errors
via the crate logger (or eprintln!) and breaks/continues as appropriate, and
keep the inner per-connection spawn that calls Connection::from_socket and
awaits session_fut; also consider adding a readiness signal (oneshot) from where
the JoinHandle is created so tests can wait until the listener is ready before
connecting.