fix: don't kill sibling sessions when sessions start runs without a TTY#43
fix: don't kill sibling sessions when sessions start runs without a TTY#43andreakiro wants to merge 1 commit into
sessions start runs without a TTY#43Conversation
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.
|
| Filename | Overview |
|---|---|
| internal/cmd/confirm.go | Adds isInteractiveStdin var and non-TTY guards to confirmReplaceSession, confirmReplaceAgent, and ConfirmStop — correctly fixes the auto-replace bug, but ConfirmStop guard also silently blocks explicit stop commands in non-TTY |
| internal/cmd/sessions.go | Skips writing current_session/expiry/viewer_url in non-TTY; correct fix for the race condition root cause |
| internal/cmd/agents.go | Mirrors sessions.go fix: skips writing current_agent file in non-TTY mode |
| internal/cmd/confirm_test.go | Good coverage of non-TTY defaults and --yes override for all three confirm functions; defines withNonInteractiveStdin helper |
| internal/cmd/sessions_test.go | Adds two targeted regression tests for the core bug fix; correctly adds withInteractiveStdinForTest to existing tests that test the interactive contract |
| internal/cmd/agents_test.go | Adds withInteractiveStdinForTest to TestAgentsStart_SetsCurrentAgent to correctly model the interactive-only write behavior |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
internal/cmd/confirm.go:114-122
**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.
### Issue 2 of 2
internal/cmd/confirm_test.go:185-186
**Duplicate non-TTY helper with inconsistent name**
`withNonInteractiveStdin` here and `withNonInteractiveStdinForTest` in `sessions_test.go` are identical functions in the same package. Consolidating to one name avoids confusion about which helper to use when adding future tests.
```suggestion
// withNonInteractiveStdinForTest temporarily overrides isInteractiveStdin to report false.
func withNonInteractiveStdinForTest(t *testing.T) {
```
Reviews (2): Last reviewed commit: "fix: don't kill sibling sessions when se..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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) | ||
| } |
There was a problem hiding this comment.
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.
1 participant
Summary
Fixes a correctness bug where
notte sessions startinvoked from a non-TTY context (subprocess, CI, parallel script) silently kills whatever session ID happens to be in~/.notte/cli/current_session. In parallel workloads, workers race to overwrite that shared file and then stop each other's live sessions, surfacing later asNotteApiSessionTimeoutError: Session ... has been closedon unrelated page calls.Root cause is three behaviors combined:
~/.notte/cli/current_sessionis global, file-based, unsynchronized.runSessionsStartauto-stops whatever ID it finds there, with no ownership check.confirmReplaceSessionreads stdin, gets EOF, swallows the error, and treats empty input as the[Y/n]default → YES.What's in this PR
confirm.go—confirmReplaceSession,confirmReplaceAgent, andConfirmStopnow returnfalsewhenstdinis not a terminal (unless--yeswas passed). Matches the convention inapt,git,ghof refusing destructive defaults in non-interactive mode. The*WithIOhelpers are unchanged so existing tests keep their interactive contract.sessions.go/agents.go—runSessionsStartandrunAgentsStartskip writingcurrent_session/current_agent/current_session_expiry/current_viewer_urlwhen stdin is not a terminal. This stops scripted invocations from clobbering each other's state and from mutating state the user's interactive shell depends on. Scripted callers should pass--session-idorNOTTE_SESSION_IDexplicitly.isInteractiveStdinis indirected through a package-levelvarso tests can flip it.golang.org/x/termis already a dependency (used byinternal/update).Behavior changes
[Enter]n--yescurrent_session(race)current_session(unchanged)Scripts that relied on the auto-stop behavior (unlikely; it was a footgun) need to pass
--yesexplicitly. The README/CHANGELOG should mention this in the next release notes.Test plan
go test ./...passes locallyconfirm_test.gotests cover non-TTY default for replace session/agent and stop, plus--yesoverridesessions_test.gotests verify (a) non-TTY start does not call/sessions/{id}/stopon a pre-populatedcurrent_session, and (b) non-TTY start does not writecurrent_sessionTestSessionsStart_SetsCurrentSession/_SavesExpiry/_AutoClearsExpiredSession/_DoesNotAutoClearNonExpiredSession/TestAgentsStart_SetsCurrentAgentnow opt intowithInteractiveStdinForTest(they were always testing the interactive contract;go testruns without a TTY by default)Follow-ups (not in this PR)
current_sessionby API-key hash so different accounts on the same machine never collide interactively. Suggested in the PRD as additional hardening on top of (A) + skip-write-if-not-TTY.-y.