Skip to content

Simplify TTY detection via testing.Testing() and OS-level detachment#1029

Merged
gtrrz-victor merged 8 commits intomainfrom
reduce-tty-complexity
Apr 28, 2026
Merged

Simplify TTY detection via testing.Testing() and OS-level detachment#1029
gtrrz-victor merged 8 commits intomainfrom
reduce-tty-complexity

Conversation

@gtrrz-victor
Copy link
Copy Markdown
Contributor

@gtrrz-victor gtrrz-victor commented Apr 24, 2026

https://entire.io/gh/entireio/cli/trails/1f88227b85b8

Summary

  • Replace ENTIRE_TEST_TTY=0 env-var signalling with real OS-level state where possible.
  • Add testing.Testing() short-circuit so in-process go test runs default to non-interactive without per-test env plumbing (~20 t.Setenv calls deleted).
  • Introduce cmd/entire/cli/execx/ package with Interactive / NonInteractive subprocess constructors. NonInteractive uses Setsid on Unix to detach the child from the controlling terminal, making /dev/tty probe fail naturally. Windows is a no-op (probe already fails there).
  • ENTIRE_TEST_TTY=1 survives only as the narrow force-interactive knob — can't be synthesized via process attributes.

Precedence in CanPromptInteractively (first match wins)

  1. ENTIRE_TEST_TTY=1 → force interactive ON.
  2. ENTIRE_TEST_TTY=<other> → force interactive OFF.
  3. testing.Testing() → OFF (in-process tests).
  4. Agent sentinels (GEMINI_CLI, COPILOT_CLI, PI_CODING_AGENT, GIT_TERMINAL_PROMPT=0) → OFF.
  5. CI=<non-empty-non-false> → OFF.
  6. /dev/tty probe.

Subprocess spawn sites converted

  • e2e/entire/entire.go (run / runErr / runOutputEnv)
  • e2e/testutil/repo.go (Git helper)
  • e2e/vogon/main.go (fireHook)
  • e2e/tests/resume_test.go (commit spawn — kept GIT_EDITOR=true)
  • cmd/entire/cli/integration_test/testenv.go (RunCLI, RunCLIWithStdin, agent-simulation branches)
  • cmd/entire/cli/integration_test/login_test.go

Test plan

  • `mise run fmt`
  • `mise run test` (unit)
  • `mise run test:integration` (122s)
  • `mise run test:e2e:canary` (4/4)
  • `mise run lint` — pre-existing unrelated `ireturn` failure in `manual_commit_condensation.go` (not part of this PR).

🤖 Generated with Claude Code


Note

Low Risk
Low risk: behavior changes are limited to test runs (testing.Testing() short-circuit) and test/e2e subprocess spawning; production TTY detection remains /dev/tty-based with the same env/CI/agent guards.

Overview
Simplifies non-interactive behavior in tests by using real TTY state instead of env var plumbing. interactive.CanPromptInteractively() now defaults to non-interactive when running under go test (unless ENTIRE_TEST_TTY=1 forces interactive), reducing reliance on ENTIRE_TEST_TTY=0 across unit tests.

Introduces execx with Interactive/NonInteractive constructors; on Unix, NonInteractive detaches child processes from the controlling TTY via Setsid so /dev/tty probes fail naturally (Windows is a no-op). Integration/e2e helpers and git-hook simulations are updated to spawn the CLI (and some git commands) using execx.NonInteractive, and many tests drop explicit ENTIRE_TEST_TTY=0 setup; askConfirmTTY also treats testing.Testing() as “don’t touch real TTY” to avoid hangs.

Reviewed by Cursor Bugbot for commit f89decd. Configure here.

Replaces env-var signalling for the "subprocess is non-interactive" case
with real OS state, and lets Go's testing harness handle in-process tests
automatically — dropping ~20 `ENTIRE_TEST_TTY=0` setenv calls.

Changes:
- `interactive.CanPromptInteractively` now short-circuits on
  `testing.Testing()`, so in-process tests default to non-interactive
  without per-test env var plumbing.
- New `cmd/entire/cli/execx/` package provides `Interactive`/`NonInteractive`
  subprocess constructors. `NonInteractive` uses `Setsid` on Unix to put
  the child in a new session with no controlling terminal; the child's
  `/dev/tty` probe then fails naturally. Windows is a no-op (probe already
  fails). Replaces `ENTIRE_TEST_TTY=0` at e2e/integration spawn sites.
- `ENTIRE_TEST_TTY=1` survives only as the narrow force-interactive knob
  (cannot be synthesized via process attributes).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 06eb11ddec12
Copilot AI review requested due to automatic review settings April 24, 2026 16:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates how Entire CLI determines whether it can prompt interactively, aiming to make tests and automation reliably non-interactive by default while reducing env-var plumbing.

Changes:

  • Add cmd/entire/cli/execx helpers to spawn subprocesses in interactive vs non-interactive (TTY-detached) modes.
  • Update interactive.CanPromptInteractively() to default OFF under go test via testing.Testing(), and keep ENTIRE_TEST_TTY=1 as the force-interactive override.
  • Convert multiple E2E/integration subprocess spawn sites to execx.NonInteractive and remove most ENTIRE_TEST_TTY=0 usage in tests.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
e2e/vogon/main.go Run vogon hooks via execx.NonInteractive instead of env-var TTY forcing.
e2e/testutil/repo.go Make git helper spawn non-interactive subprocesses via execx.
e2e/tests/resume_test.go Detach git commit subprocess from TTY while preserving GIT_EDITOR=true.
e2e/entire/entire.go Run entire E2E helpers via execx.NonInteractive and drop ENTIRE_TEST_TTY=0.
cmd/entire/cli/versioncheck/versioncheck_test.go Remove per-test ENTIRE_TEST_TTY=0 now that in-process tests default non-interactive.
cmd/entire/cli/versioncheck/autoupdate_test.go Keep explicit env override only where needed to counter a fixture default.
cmd/entire/cli/strategy/rewind_test.go Remove ENTIRE_TEST_TTY=0 from a non-interactive prompt test.
cmd/entire/cli/strategy/preparecommitmsg_bench_test.go Remove benchmark ENTIRE_TEST_TTY=0 plumbing; rely on testing default.
cmd/entire/cli/strategy/manual_commit_hooks.go Short-circuit /dev/tty prompting during tests via testing.Testing().
cmd/entire/cli/strategy/condense_skip_test.go Remove ENTIRE_TEST_TTY=0 from several strategy tests.
cmd/entire/cli/setup_test.go Delete many ENTIRE_TEST_TTY=0 calls and rely on test-mode default non-interactive behavior.
cmd/entire/cli/setup_github_test.go Remove ENTIRE_TEST_TTY=0 from GitHub bootstrap tests.
cmd/entire/cli/interactive/interactive_test.go Add a test for testing.Testing() behavior; adjust agent-guard tests.
cmd/entire/cli/interactive/interactive.go Implement testing.Testing() short-circuit and update precedence documentation.
cmd/entire/cli/integration_test/testenv.go Use execx.NonInteractive for CLI subprocesses and hook simulations; remove ENTIRE_TEST_TTY=0 from baseline env.
cmd/entire/cli/integration_test/login_test.go Spawn entire login subprocess detached from TTY.
cmd/entire/cli/explain_test.go Remove ENTIRE_TEST_TTY=0 from explain tests (now default non-interactive).
cmd/entire/cli/explain_summary_provider_test.go Remove ENTIRE_TEST_TTY=0 from summary provider tests.
cmd/entire/cli/execx/execx.go New package providing Interactive/NonInteractive constructors.
cmd/entire/cli/execx/execx_unix.go Unix implementation uses Setsid to detach from controlling TTY.
cmd/entire/cli/execx/execx_windows.go Windows implementation is a no-op (no /dev/tty).
cmd/entire/cli/execx/execx_test.go Adds basic tests for execx subprocess attribute behavior.
Comments suppressed due to low confidence (1)

cmd/entire/cli/interactive/interactive_test.go:60

  • TestCanPromptInteractively_AgentEnvGuards no longer exercises the agent-sentinel branch: when ENTIRE_TEST_TTY is unset, CanPromptInteractively() returns false due to testing.Testing() before it reaches the agent-env checks, so the assertions will pass even if the agent guard logic is broken. Consider refactoring CanPromptInteractively to make the agent-env predicate testable (e.g., a helper that accepts an isTesting flag / env accessor), or moving this verification to a subprocess where testing.Testing() is false.
func TestCanPromptInteractively_AgentEnvGuards(t *testing.T) {
	// ENTIRE_TEST_TTY=1 bypasses testing.Testing() so we can exercise the
	// agent-env guards — they must still force non-interactive regardless.
	t.Setenv("ENTIRE_TEST_TTY", "1")

	cases := []struct {
		name, key, val string
	}{
		{"gemini", "GEMINI_CLI", "1"},
		{"copilot", "COPILOT_CLI", "1"},
		{"pi", "PI_CODING_AGENT", "true"},
		{"git-terminal-prompt-off", "GIT_TERMINAL_PROMPT", "0"},
	}
	for _, c := range cases {
		t.Run(c.name, func(t *testing.T) {
			// ENTIRE_TEST_TTY=1 wins by design; to test agent guards we must
			// clear the override and let testing.Testing() be bypassed by the
			// agent check coming next. Use empty override so the env precedence
			// check falls through to the agent guards.
			t.Setenv("ENTIRE_TEST_TTY", "")
			_ = os.Unsetenv("ENTIRE_TEST_TTY")
			t.Setenv(c.key, c.val)
			// testing.Testing() returns true here too, so this test only
			// asserts that the gate returns false (which both paths satisfy).
			if CanPromptInteractively() {
				t.Errorf("CanPromptInteractively() = true; want false when %s=%s", c.key, c.val)
			}
		})

Comment thread cmd/entire/cli/execx/execx_test.go Outdated
Comment thread cmd/entire/cli/interactive/interactive.go Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit f89decd. Configure here.

Comment thread cmd/entire/cli/interactive/interactive_test.go Outdated
gtrrz-victor and others added 5 commits April 27, 2026 14:15
Address review findings on top of f89decd:

- Strengthen Windows execx with DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP
  (matching the now-deleted integration_test/procattr_windows.go).
- Delete duplicate cmd/entire/cli/integration_test/procattr_{unix,windows}.go;
  RunResume now uses execx.NonInteractive directly.
- Add interactive.UnderTest() helper. manual_commit_hooks.askConfirmTTY drops
  its inline testing.Testing()/getenv duplication and calls UnderTest instead.
- Extract isAgentSubprocessEnv() and unit-test it directly — the previous
  AgentEnvGuards test was vacuous because testing.Testing() short-circuits
  before the guards run.
- Extract testenv.prepareCommitMsgCmd helper so both gitCommit{,Staged}WithShadowHooks
  share the simulateTTY branching.
- Export EnvTestTTY constant; reuse in tests.
- Drop redundant os.Unsetenv after t.Setenv("", "").

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 2a66a73ea393
TestDefaultRunInteractiveDispatch_DoesNotUseAltScreen and
TestDefaultRunInteractiveDispatch_ClearsLoadingCardBeforeReturn both
mutate the package-level newDispatchProgram. With t.Parallel() the
race detector flags the concurrent writes, cascading into 24+
"race detected during execution" failures across cmd/entire/cli.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 879ccafeb9bd
Adds a section under "Git in Tests" explaining the new precedence chain
(EnvTestTTY → testing.Testing() → agent sentinels → CI → /dev/tty probe),
the execx.NonInteractive helper for OS-level TTY detachment, and the
interactive.UnderTest() helper for code that needs to skip /dev/tty
reads even when CanPromptInteractively returns true.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: ce53e26a8ca7
@gtrrz-victor gtrrz-victor marked this pull request as ready for review April 27, 2026 13:43
@gtrrz-victor gtrrz-victor requested a review from a team as a code owner April 27, 2026 13:43
Soph
Soph previously approved these changes Apr 27, 2026
Comment thread cmd/entire/cli/versioncheck/versioncheck_test.go Outdated
# Conflicts:
#	cmd/entire/cli/dispatch_tui_test.go
@gtrrz-victor gtrrz-victor requested a review from Soph April 28, 2026 10:08
@gtrrz-victor gtrrz-victor enabled auto-merge April 28, 2026 10:14
@gtrrz-victor gtrrz-victor merged commit a72a26b into main Apr 28, 2026
9 checks passed
@gtrrz-victor gtrrz-victor deleted the reduce-tty-complexity branch April 28, 2026 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants