From ba8b3a19f9bd2b8b29a4fcff96c0bd7a280332a8 Mon Sep 17 00:00:00 2001 From: typebasedio Date: Wed, 22 Apr 2026 19:03:17 -0400 Subject: [PATCH] fix: only remove provisional workspace when child exits non-zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a child-exit-code guard to handleProvisional so a `ctask new` workspace is reclaimed only when the agent was actually canceled before real work (trust prompt rejected, Esc during startup, Ctrl+C mid-launch — all confirmed empirically as exit code 1). A zero exit means the user entered the agent and exited cleanly, which is a legitimate workflow that must preserve the workspace even with an empty manifest diff — for example when the user wants the workspace directory established so they can populate context/ before resuming. The exit code reaches handleProvisional via a new childExitCode helper that unwraps *exec.ExitError from cmd.Run's return. Non-exit errors (agent not found, OS-level failure) map to -1 so the workspace is still cleaned up — the child never actually ran. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/session/run.go | 26 +++++++++++++++---- internal/session/run_provisional.go | 20 ++++++++++++--- internal/session/run_provisional_test.go | 32 ++++++++++++++++++------ 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/internal/session/run.go b/internal/session/run.go index 5e7727f..41022ae 100644 --- a/internal/session/run.go +++ b/internal/session/run.go @@ -3,6 +3,7 @@ package session import ( "fmt" "os" + "os/exec" "path/filepath" "time" @@ -153,11 +154,12 @@ func Run(opts LaunchOpts) error { } // ---- Provisional-workspace cleanup ---- - // If `ctask new` launched this session and the child exited without - // changing any files, remove the workspace entirely and skip finalize. - // Nothing to log, nothing to summarize, and the .ctask state files die - // with the directory. - if handleProvisional(opts, startManifest) { + // If `ctask new` launched this session, the child exited non-zero + // (canceled before real work), and no files changed, remove the workspace + // entirely and skip finalize. A zero child exit means the user entered + // the agent and exited normally — those workspaces are preserved even + // with an empty diff. + if handleProvisional(opts, startManifest, childExitCode(childErr)) { return childErr } @@ -173,6 +175,20 @@ func Run(opts LaunchOpts) error { return childErr } +// childExitCode extracts the exit code from the error returned by cmd.Run(). +// Returns 0 when err is nil (clean exit). Returns the reported code for +// *exec.ExitError. Returns -1 for any other error (agent not found, OS-level +// failure) so provisional cleanup still kicks in — the child never ran. +func childExitCode(err error) int { + if err == nil { + return 0 + } + if ee, ok := err.(*exec.ExitError); ok { + return ee.ExitCode() + } + return -1 +} + // finalize runs all end-of-session metadata writes under one write-lock // acquisition: session log append (best-effort), last-session-summary.json // (best-effort), lease removal (best-effort, if we owned it), and diff --git a/internal/session/run_provisional.go b/internal/session/run_provisional.go index b89f557..ca1611c 100644 --- a/internal/session/run_provisional.go +++ b/internal/session/run_provisional.go @@ -6,9 +6,15 @@ import ( ) // handleProvisional removes a workspace that was created by this `ctask new` -// invocation but saw no real work. "No real work" is defined strictly as an -// empty manifest diff — zero added, zero modified, zero deleted. No other -// heuristic is used. +// invocation but was canceled before producing any real work. All three +// conditions must hold: +// +// - NewlyCreated is true (so the workspace is ours to reclaim) +// - the child process exited non-zero (trust prompt rejected, Esc during +// startup, Ctrl+C mid-launch — confirmed empirically; a zero exit means +// the user entered the agent and exited cleanly, which is a legitimate +// workflow that must preserve the workspace even with no file changes) +// - the manifest diff is empty (nothing to preserve) // // Removing the workspace directory also removes every v0.4 state file inside // .ctask/ (session.json, manifest-start.json, last-session-summary.json, @@ -16,10 +22,16 @@ import ( // // Returns true iff the workspace was removed (the caller must then skip the // normal finalize path, since there is nothing to log or summarize). -func handleProvisional(opts LaunchOpts, startManifest *Manifest) bool { +func handleProvisional(opts LaunchOpts, startManifest *Manifest, childExitCode int) bool { if !opts.NewlyCreated { return false } + // A clean child exit (code 0) means the user reached the agent and exited + // normally — they may want the workspace preserved for context files even + // if the session produced no manifest changes. Keep it. + if childExitCode == 0 { + return false + } // If the start manifest failed to capture we cannot prove the diff is // empty, so err on the side of keeping the workspace. if startManifest == nil { diff --git a/internal/session/run_provisional_test.go b/internal/session/run_provisional_test.go index 7aa3c7f..d87319c 100644 --- a/internal/session/run_provisional_test.go +++ b/internal/session/run_provisional_test.go @@ -25,11 +25,11 @@ func newWorkspace(t *testing.T) (string, *Manifest) { return wsDir, m } -func TestHandleProvisionalRemovesWhenNewlyCreatedAndEmptyDiff(t *testing.T) { +func TestHandleProvisionalRemovesWhenNewlyCreatedAndEmptyDiffAndExitNonZero(t *testing.T) { wsDir, startManifest := newWorkspace(t) opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true} - if !handleProvisional(opts, startManifest) { + if !handleProvisional(opts, startManifest, 1) { t.Fatal("expected handleProvisional to return true (workspace removed)") } if _, err := os.Stat(wsDir); !errors.Is(err, os.ErrNotExist) { @@ -41,7 +41,7 @@ func TestHandleProvisionalKeepsWhenNotNewlyCreated(t *testing.T) { wsDir, startManifest := newWorkspace(t) opts := LaunchOpts{WsDir: wsDir, NewlyCreated: false} - if handleProvisional(opts, startManifest) { + if handleProvisional(opts, startManifest, 1) { t.Fatal("expected handleProvisional to return false when NewlyCreated is false") } if _, err := os.Stat(wsDir); err != nil { @@ -58,7 +58,7 @@ func TestHandleProvisionalKeepsWhenDiffNonEmpty(t *testing.T) { } opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true} - if handleProvisional(opts, startManifest) { + if handleProvisional(opts, startManifest, 1) { t.Fatal("expected handleProvisional to return false when diff has changes") } if _, err := os.Stat(wsDir); err != nil { @@ -73,7 +73,7 @@ func TestHandleProvisionalKeepsWhenStartManifestNil(t *testing.T) { } opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true} - if handleProvisional(opts, nil) { + if handleProvisional(opts, nil, 1) { t.Fatal("expected handleProvisional to return false when startManifest is nil") } if _, err := os.Stat(wsDir); err != nil { @@ -81,13 +81,29 @@ func TestHandleProvisionalKeepsWhenStartManifestNil(t *testing.T) { } } +// A zero exit code means the user accepted the trust prompt and exited the +// agent normally — a legitimate workflow (e.g. the user wanted the workspace +// directory established to populate context/ later). The workspace must be +// preserved even when the manifest diff is empty. +func TestHandleProvisionalKeepsWhenExitCodeZero(t *testing.T) { + wsDir, startManifest := newWorkspace(t) + + opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true} + if handleProvisional(opts, startManifest, 0) { + t.Fatal("expected handleProvisional to return false when child exited zero") + } + if _, err := os.Stat(wsDir); err != nil { + t.Errorf("expected wsDir to still exist, got err=%v", err) + } +} + func TestHandleProvisionalRemovesShellSession(t *testing.T) { // The fix is not agent-specific: a --shell launch with no file changes - // must also be cleaned up. + // and a non-zero exit (Ctrl+C during shell startup) must be cleaned up. wsDir, startManifest := newWorkspace(t) opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true, Shell: true} - if !handleProvisional(opts, startManifest) { + if !handleProvisional(opts, startManifest, 1) { t.Fatal("expected handleProvisional to remove workspace for shell session with no changes") } if _, err := os.Stat(wsDir); !errors.Is(err, os.ErrNotExist) { @@ -118,7 +134,7 @@ func TestHandleProvisionalRemovalWipesV04State(t *testing.T) { } opts := LaunchOpts{WsDir: wsDir, NewlyCreated: true} - if !handleProvisional(opts, startManifest) { + if !handleProvisional(opts, startManifest, 1) { t.Fatal("expected handleProvisional to remove workspace") } for _, name := range stateFiles {