From e7c1fd4db180a84e9966100a9ca75651c79295df Mon Sep 17 00:00:00 2001 From: Andrea Pinto Date: Fri, 22 May 2026 10:45:48 -0700 Subject: [PATCH] fix: don't kill sibling sessions when sessions start runs without a TTY When `notte sessions start` is invoked from a subprocess, CI, or parallel script, stdin is EOF. The replace-existing-session prompt treats EOF as "Enter" and defaults to YES, so the CLI silently issues DELETE /sessions/{id}/stop on whatever ID happens to be in the shared ~/.notte/cli/current_session file - which may belong to a sibling worker's live session. Two changes: 1. confirmReplaceSession, confirmReplaceAgent and ConfirmStop now return false when stdin is not a terminal (unless --yes was passed). Matches the convention used by apt, git, gh. 2. runSessionsStart and runAgentsStart skip writing the global current_session / current_agent / current_session_expiry / current_viewer_url files when stdin is not a terminal. This stops scripted invocations from mutating state the interactive shell relies on, and removes the race where two parallel workers overwrite each other's current_session. Scripted callers should thread the session ID explicitly via --session-id or NOTTE_SESSION_ID; this PR makes that the only safe option for non-interactive use. --- internal/cmd/agents.go | 6 +- internal/cmd/agents_test.go | 2 + internal/cmd/confirm.go | 27 ++++++- internal/cmd/confirm_test.go | 86 ++++++++++++++++++++++ internal/cmd/sessions.go | 8 +- internal/cmd/sessions_test.go | 134 ++++++++++++++++++++++++++++++++++ 6 files changed, 255 insertions(+), 8 deletions(-) diff --git a/internal/cmd/agents.go b/internal/cmd/agents.go index 6029275..18e36db 100644 --- a/internal/cmd/agents.go +++ b/internal/cmd/agents.go @@ -266,8 +266,10 @@ func runAgentsStart(cmd *cobra.Command, args []string) error { return err } - // Save agent ID as current agent - if resp.JSON200 != nil { + // Save agent ID as current agent, but only when running interactively. + // In non-TTY contexts (scripts, CI, parallel workers) writing to the shared + // ~/.notte/cli/current_agent file races with sibling processes. + if resp.JSON200 != nil && isInteractiveStdin() { if err := setCurrentAgent(resp.JSON200.AgentId); err != nil { PrintInfo(fmt.Sprintf("Warning: could not save current agent: %v", err)) } diff --git a/internal/cmd/agents_test.go b/internal/cmd/agents_test.go index 752e433..ab9d85d 100644 --- a/internal/cmd/agents_test.go +++ b/internal/cmd/agents_test.go @@ -546,6 +546,8 @@ func TestRequireAgentID_FromFile(t *testing.T) { } func TestAgentsStart_SetsCurrentAgent(t *testing.T) { + withInteractiveStdinForTest(t) + env := testutil.SetupTestEnv(t) env.SetEnv("NOTTE_API_KEY", "test-key") diff --git a/internal/cmd/confirm.go b/internal/cmd/confirm.go index fda2cb3..5b9daea 100644 --- a/internal/cmd/confirm.go +++ b/internal/cmd/confirm.go @@ -7,11 +7,19 @@ import ( "io" "os" "strings" + + "golang.org/x/term" ) // skipConfirmation is set by --yes flag to skip prompts var skipConfirmation bool +// isInteractiveStdin reports whether stdin is connected to a terminal. +// Indirected through a variable so tests can override it. +var isInteractiveStdin = func() bool { + return term.IsTerminal(int(os.Stdin.Fd())) +} + // ConfirmAction prompts the user to confirm a destructive action. // Returns true if confirmed, false otherwise. func ConfirmAction(resource, id string) (bool, error) { @@ -38,11 +46,15 @@ func ConfirmActionWithIO(in io.Reader, out io.Writer, resource, id string) (bool } // confirmReplaceSession prompts the user to confirm stopping an existing session before starting a new one. -// Defaults to "yes" if user just presses Enter. +// Defaults to "yes" if the user just presses Enter on an interactive terminal. +// Returns false (do nothing destructive) when stdin is not a terminal, unless --yes was passed. func confirmReplaceSession(id string) (bool, error) { if skipConfirmation { return true, nil } + if !isInteractiveStdin() { + return false, nil + } return confirmReplaceSessionWithIO(os.Stdin, os.Stderr, id) } @@ -63,11 +75,15 @@ func confirmReplaceSessionWithIO(in io.Reader, out io.Writer, id string) (bool, } // confirmReplaceAgent prompts the user to confirm stopping an existing agent before starting a new one. -// Defaults to "yes" if user just presses Enter. +// Defaults to "yes" if the user just presses Enter on an interactive terminal. +// Returns false (do nothing destructive) when stdin is not a terminal, unless --yes was passed. func confirmReplaceAgent(id string) (bool, error) { if skipConfirmation { return true, nil } + if !isInteractiveStdin() { + return false, nil + } return confirmReplaceAgentWithIO(os.Stdin, os.Stderr, id) } @@ -93,12 +109,15 @@ func SetSkipConfirmation(skip bool) { } // ConfirmStop prompts the user to confirm stopping a resource. -// Defaults to "yes" if user just presses Enter. -// Returns true if confirmed, false otherwise. +// Defaults to "yes" if the user just presses Enter on an interactive terminal. +// Returns false (do nothing destructive) when stdin is not a terminal, unless --yes was passed. func ConfirmStop(resource, id string) (bool, error) { if skipConfirmation { return true, nil } + if !isInteractiveStdin() { + return false, nil + } return ConfirmStopWithIO(os.Stdin, os.Stderr, resource, id) } diff --git a/internal/cmd/confirm_test.go b/internal/cmd/confirm_test.go index 1deb8b7..8accab1 100644 --- a/internal/cmd/confirm_test.go +++ b/internal/cmd/confirm_test.go @@ -181,3 +181,89 @@ func TestConfirmReplaceAgentWithIO_Errors(t *testing.T) { t.Fatal("expected read error") } } + +// withNonInteractiveStdin temporarily overrides isInteractiveStdin to report false. +func withNonInteractiveStdin(t *testing.T) { + t.Helper() + prev := isInteractiveStdin + isInteractiveStdin = func() bool { return false } + t.Cleanup(func() { isInteractiveStdin = prev }) +} + +func TestConfirmReplaceSession_NonTTYDefaultsToFalse(t *testing.T) { + withNonInteractiveStdin(t) + + ok, err := confirmReplaceSession("sess_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected non-TTY confirm to default to false") + } +} + +func TestConfirmReplaceSession_NonTTYSkipConfirmationStillReturnsTrue(t *testing.T) { + withNonInteractiveStdin(t) + SetSkipConfirmation(true) + t.Cleanup(func() { SetSkipConfirmation(false) }) + + ok, err := confirmReplaceSession("sess_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected --yes to still return true even on non-TTY stdin") + } +} + +func TestConfirmReplaceAgent_NonTTYDefaultsToFalse(t *testing.T) { + withNonInteractiveStdin(t) + + ok, err := confirmReplaceAgent("agent_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected non-TTY confirm to default to false") + } +} + +func TestConfirmReplaceAgent_NonTTYSkipConfirmationStillReturnsTrue(t *testing.T) { + withNonInteractiveStdin(t) + SetSkipConfirmation(true) + t.Cleanup(func() { SetSkipConfirmation(false) }) + + ok, err := confirmReplaceAgent("agent_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected --yes to still return true even on non-TTY stdin") + } +} + +func TestConfirmStop_NonTTYDefaultsToFalse(t *testing.T) { + withNonInteractiveStdin(t) + + ok, err := ConfirmStop("session", "sess_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected non-TTY confirm to default to false") + } +} + +func TestConfirmStop_NonTTYSkipConfirmationStillReturnsTrue(t *testing.T) { + withNonInteractiveStdin(t) + SetSkipConfirmation(true) + t.Cleanup(func() { SetSkipConfirmation(false) }) + + ok, err := ConfirmStop("session", "sess_123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected --yes to still return true even on non-TTY stdin") + } +} diff --git a/internal/cmd/sessions.go b/internal/cmd/sessions.go index c657efc..2b04b97 100644 --- a/internal/cmd/sessions.go +++ b/internal/cmd/sessions.go @@ -588,8 +588,12 @@ func runSessionsStart(cmd *cobra.Command, args []string) error { return err } - // Save session ID as current session - if resp.JSON200 != nil { + // Save session ID as current session, but only when running interactively. + // In non-TTY contexts (scripts, CI, parallel workers) writing to the shared + // ~/.notte/cli/current_session file is the root of a race where sibling + // processes can read each other's IDs and stop them. Scripted callers should + // thread the session ID explicitly via --session-id or NOTTE_SESSION_ID. + if resp.JSON200 != nil && isInteractiveStdin() { if err := setCurrentSession(resp.JSON200.SessionId); err != nil { PrintInfo(fmt.Sprintf("Warning: could not save current session: %v", err)) } diff --git a/internal/cmd/sessions_test.go b/internal/cmd/sessions_test.go index d009e72..eb348a8 100644 --- a/internal/cmd/sessions_test.go +++ b/internal/cmd/sessions_test.go @@ -819,6 +819,132 @@ func TestRunSessionWorkflowCode(t *testing.T) { } } +// withNonInteractiveStdinForTest forces isInteractiveStdin to report non-TTY for the test duration. +func withNonInteractiveStdinForTest(t *testing.T) { + t.Helper() + prev := isInteractiveStdin + isInteractiveStdin = func() bool { return false } + t.Cleanup(func() { isInteractiveStdin = prev }) +} + +// withInteractiveStdinForTest forces isInteractiveStdin to report TTY for the test duration. +// Use when verifying behaviors that are gated on interactive mode (e.g. writing to +// the global current_session/current_agent file). +func withInteractiveStdinForTest(t *testing.T) { + t.Helper() + prev := isInteractiveStdin + isInteractiveStdin = func() bool { return true } + t.Cleanup(func() { isInteractiveStdin = prev }) +} + +// TestRunSessionsStart_NonTTYDoesNotStopExistingSession verifies the core bug fix: +// when stdin is not a terminal, runSessionsStart must not stop the session +// referenced by ~/.notte/cli/current_session, since that ID may belong to a +// concurrent worker process. Without the fix, the EOF stdin causes the +// confirmation prompt to default to "yes" and issues a DELETE on a sibling +// process's live session. +func TestRunSessionsStart_NonTTYDoesNotStopExistingSession(t *testing.T) { + env := testutil.SetupTestEnv(t) + env.SetEnv("NOTTE_API_KEY", "test-key") + + server := testutil.NewMockServer() + defer server.Close() + env.SetEnv("NOTTE_API_URL", server.URL()) + + server.AddResponse("/sessions/start", 200, `{"session_id":"sess_new","status":"ACTIVE","created_at":"2020-01-01T00:00:00Z","last_accessed_at":"2020-01-01T00:00:00Z","timeout_minutes":5}`) + // If the bug regresses, the CLI will call this endpoint with sess_existing. + server.AddResponse("/sessions/sess_existing/stop", 200, `{"session_id":"sess_existing","status":"CLOSED","created_at":"2020-01-01T00:00:00Z","last_accessed_at":"2020-01-01T00:00:00Z","timeout_minutes":0}`) + + tmpDir := setupSessionFileTest(t) + configDir := filepath.Join(tmpDir, config.ConfigDirName) + if err := os.MkdirAll(configDir, 0o700); err != nil { + t.Fatalf("failed to create config dir: %v", err) + } + sessionFile := filepath.Join(configDir, config.CurrentSessionFile) + if err := os.WriteFile(sessionFile, []byte("sess_existing"), 0o600); err != nil { + t.Fatalf("failed to seed current_session: %v", err) + } + + withNonInteractiveStdinForTest(t) + + origID := sessionID + sessionID = "" + t.Cleanup(func() { sessionID = origID }) + + origFormat := outputFormat + outputFormat = "json" + t.Cleanup(func() { outputFormat = origFormat }) + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + var runErr error + _, _ = testutil.CaptureOutput(func() { + runErr = runSessionsStart(cmd, nil) + }) + if runErr != nil { + t.Fatalf("unexpected error: %v", runErr) + } + + if reqs := server.Requests("/sessions/sess_existing/stop"); len(reqs) > 0 { + t.Fatalf("non-TTY runSessionsStart must not stop the existing session, but called %d times", len(reqs)) + } + + // Current-session file should NOT have been overwritten with the new ID, + // otherwise a concurrent sibling process could later read it and target sess_new. + data, err := os.ReadFile(sessionFile) + if err != nil { + t.Fatalf("failed to read current_session file: %v", err) + } + if got := strings.TrimSpace(string(data)); got != "sess_existing" { + t.Fatalf("non-TTY runSessionsStart must not overwrite current_session; got %q, want %q", got, "sess_existing") + } +} + +// TestRunSessionsStart_NonTTYNewSessionDoesNotWriteFile verifies that on a clean +// machine (no current_session) a non-TTY runSessionsStart does not write the +// global state file - so a sibling process in a separate worker doesn't later +// pick up the ID and stop it. +func TestRunSessionsStart_NonTTYNewSessionDoesNotWriteFile(t *testing.T) { + env := testutil.SetupTestEnv(t) + env.SetEnv("NOTTE_API_KEY", "test-key") + + server := testutil.NewMockServer() + defer server.Close() + env.SetEnv("NOTTE_API_URL", server.URL()) + + server.AddResponse("/sessions/start", 200, `{"session_id":"sess_new","status":"ACTIVE","created_at":"2020-01-01T00:00:00Z","last_accessed_at":"2020-01-01T00:00:00Z","timeout_minutes":5}`) + + tmpDir := setupSessionFileTest(t) + configDir := filepath.Join(tmpDir, config.ConfigDirName) + sessionFile := filepath.Join(configDir, config.CurrentSessionFile) + + withNonInteractiveStdinForTest(t) + + origID := sessionID + sessionID = "" + t.Cleanup(func() { sessionID = origID }) + + origFormat := outputFormat + outputFormat = "json" + t.Cleanup(func() { outputFormat = origFormat }) + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + var runErr error + _, _ = testutil.CaptureOutput(func() { + runErr = runSessionsStart(cmd, nil) + }) + if runErr != nil { + t.Fatalf("unexpected error: %v", runErr) + } + + if _, err := os.Stat(sessionFile); !os.IsNotExist(err) { + t.Fatalf("non-TTY runSessionsStart must not write current_session; stat err=%v", err) + } +} + // Tests for session ID resolution (file-based tracking) func setupSessionFileTest(t *testing.T) string { @@ -1032,6 +1158,8 @@ func TestRequireSessionID_FromFile(t *testing.T) { } func TestSessionsStart_SetsCurrentSession(t *testing.T) { + withInteractiveStdinForTest(t) + env := testutil.SetupTestEnv(t) env.SetEnv("NOTTE_API_KEY", "test-key") @@ -1238,6 +1366,8 @@ func TestClearCurrentSessionExpiry_NoFile(t *testing.T) { } func TestSessionsStart_SavesExpiry(t *testing.T) { + withInteractiveStdinForTest(t) + env := testutil.SetupTestEnv(t) env.SetEnv("NOTTE_API_KEY", "test-key") @@ -1289,6 +1419,8 @@ func TestSessionsStart_SavesExpiry(t *testing.T) { } func TestSessionsStart_AutoClearsExpiredSession(t *testing.T) { + withInteractiveStdinForTest(t) + env := testutil.SetupTestEnv(t) env.SetEnv("NOTTE_API_KEY", "test-key") @@ -1354,6 +1486,8 @@ func TestSessionsStart_AutoClearsExpiredSession(t *testing.T) { } func TestSessionsStart_DoesNotAutoClearNonExpiredSession(t *testing.T) { + withInteractiveStdinForTest(t) + env := testutil.SetupTestEnv(t) env.SetEnv("NOTTE_API_KEY", "test-key")