Skip to content

fix: don't kill sibling sessions when sessions start runs without a TTY #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andreakiro wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: don't kill sibling sessions when sessions start runs without a TTY #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/cmd/agents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
2 changes: 2 additions & 0 deletions internal/cmd/agents_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
27 changes: 23 additions & 4 deletions internal/cmd/confirm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}
Comment on lines 114 to 122

@greptile-apps greptile-apps Bot May 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Explicit stop commands silently no-op with exit 0 in non-TTY

The non-TTY guard added to ConfirmStop means notte sessions stop <id> and notte agents stop <id> called from a CI script without --yes now print "Cancelled." and return exit 0 without stopping anything. Before this PR, EOF on stdin caused the [Y/n] default to resolve to true, so explicit stops worked as expected in scripts.

The confirmReplaceSession/confirmReplaceAgent guards are correct — those are the functions on the start-command path that caused sibling sessions to be killed. But ConfirmStop is called from the explicit stop subcommand, where the user has already identified the target ID. A CI cleanup step like notte sessions stop $SESSION_ID silently leaks the session, and a caller checking only exit code gets no signal that the stop was skipped.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: internal/cmd/confirm.go
Line: 114-122

Comment:
**Explicit `stop` commands silently no-op with exit 0 in non-TTY**

The non-TTY guard added to `ConfirmStop` means `notte sessions stop <id>` and `notte agents stop <id>` called from a CI script without `--yes` now print "Cancelled." and return exit 0 without stopping anything. Before this PR, EOF on stdin caused the `[Y/n]` default to resolve to `true`, so explicit stops worked as expected in scripts.

The `confirmReplaceSession`/`confirmReplaceAgent` guards are correct — those are the functions on the start-command path that caused sibling sessions to be killed. But `ConfirmStop` is called from the explicit `stop` subcommand, where the user has already identified the target ID. A CI cleanup step like `notte sessions stop $SESSION_ID` silently leaks the session, and a caller checking only exit code gets no signal that the stop was skipped.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code


Expand Down
86 changes: 86 additions & 0 deletions internal/cmd/confirm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
8 changes: 6 additions & 2 deletions internal/cmd/sessions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
134 changes: 134 additions & 0 deletions internal/cmd/sessions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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")

Expand Down
Loading