mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(init): don't block on confirmation for 'init --here' without a TTY (#3236)
* fix(init): don't block on confirmation for 'init --here' without a TTY When 'specify init --here' targets a non-empty directory without --force, it called typer.confirm() unconditionally. In a non-interactive session (no TTY -- CI, piped, agent) there is no input, so the prompt reads EOF and aborts unhelpfully (or blocks), with no actionable message. The named-project path already fails fast and points to --force; --here was the inconsistent outlier. Guard the confirmation with the existing _stdin_is_interactive() helper: when non-interactive, print a clear 'directory not empty; re-run with --force' error and exit 1 instead of prompting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped confirmation for 'init --here'; only fail-fast on empty stdin The first version of this fix short-circuited on '_stdin_is_interactive()' (isatty) before typer.confirm, which broke 'init --here' when confirmation is piped (e.g. 'echo y | specify init --here' / CliRunner input='y\n') -- a non-TTY pipe with valid input was wrongly rejected, regressing test_init_here_without_force_preserves_shared_infra. Instead, call typer.confirm normally (piped 'y'/'n' is honored) and catch the Abort/EOFError it raises only when stdin is empty, converting that to the actionable '--force' guidance. This keeps the UX win for the no-input case without rejecting piped input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): distinguish interactive cancel from no-input; defer merge warning Address Copilot review on the --here non-empty path: (1) treat typer.Abort during an interactive confirm (e.g. Ctrl+C) as a normal cancellation (exit 0), and only emit the '--force' guidance + exit 1 when there is no TTY (empty stdin / EOF) -- no longer conflating the two; (2) move the 'will be merged / may overwrite' warning so it only shows when actually proceeding (force) or folded into the confirmation prompt, not on the fail-fast path where nothing is merged. Piped confirmation (e.g. 'echo y | specify init --here') is still honored, which is why the prompt is attempted rather than refused outright when non-interactive -- the existing test_init_here_without_force_preserves_shared_infra pipes 'y' and must succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): fail fast on non-interactive --here instead of prompting Per Copilot review: do not call typer.confirm when stdin is not a TTY -- an open-but-idle non-TTY stdin (CI/agent) could block on the prompt. When the directory is non-empty and --force is not given, fail fast with '--force' guidance unless an interactive terminal is present. Interactive confirm still offers the merge-but-preserve path (distinct from --force, which overwrites); a Ctrl+C there is treated as a normal cancellation (exit 0). The merge/overwrite warning is only printed when actually proceeding, not on the fail-fast path. Updated the preserve-merge E2E test to simulate an interactive terminal so it exercises the confirm path (non-interactive sessions now require --force). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped y/n for 'init --here', error only on no-input Per maintainer review: restore the second-revision shape. Calling typer.confirm normally keeps 'echo y | specify init --here' reaching the non-destructive preserve-merge path (and piped 'n' cancels with exit 0). Only when no confirmation input is available at all (closed/empty stdin -> typer.Abort/EOFError) is it converted into the actionable error that points at --force. This drops the _stdin_is_interactive fail-fast that broke the common piped-confirm idiom and made preserve-merge interactive-only. The preserve test no longer needs to monkeypatch _stdin_is_interactive - it passes on the real contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(init): preserve interactive-cancel semantics; fold merge risk into the prompt Two review-driven refinements to the 'init --here' non-empty confirm, keeping the maintainer-endorsed control flow (piped y/n honored; non-interactive EOF → actionable --force error): 1. typer.confirm raises typer.Abort for BOTH an interactive Ctrl+C and an EOF on closed/empty stdin. Catching it unconditionally reported 'no confirmation input available, use --force' and exited 1 even when the user cancelled at a real TTY. Branch on _stdin_is_interactive(): a TTY cancel is a normal exit 0 ('Operation cancelled'); only non-interactive EOF becomes the --force error. 2. Fold the merge-risk warning into the confirmation question instead of printing it unconditionally beforehand, so the EOF/no-input path (which exits without changing anything) no longer prints a misleading 'will be merged' line first. Adds test_init_here_interactive_cancel_exits_zero (fails before: exit 1 with --force; passes after: exit 0, 'cancelled', pre-existing file untouched). The non-interactive EOF and piped-y preserve-merge tests are unchanged and still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -220,16 +220,45 @@ def register(app: typer.Typer) -> None:
|
||||
console.print(
|
||||
f"[yellow]Warning:[/yellow] Current directory is not empty ({len(existing_items)} items)"
|
||||
)
|
||||
console.print(
|
||||
"[yellow]Template files will be merged with existing content and may overwrite existing files[/yellow]"
|
||||
)
|
||||
if force:
|
||||
# Proceeding: the merge/overwrite warning is accurate here.
|
||||
console.print(
|
||||
"[yellow]Template files will be merged with existing content and may overwrite existing files[/yellow]"
|
||||
)
|
||||
console.print(
|
||||
"[cyan]--force supplied: skipping confirmation and proceeding with merge[/cyan]"
|
||||
)
|
||||
else:
|
||||
response = typer.confirm("Do you want to continue?")
|
||||
if not response:
|
||||
# Fold the merge risk into the confirmation prompt rather than
|
||||
# printing it unconditionally first: on the EOF/no-input path
|
||||
# below the command exits without changing anything, so a
|
||||
# standalone "will be merged" line would mislead. Interactive
|
||||
# users still see the risk as part of the question.
|
||||
#
|
||||
# Call typer.confirm normally so piped y/n is honored — e.g.
|
||||
# `echo y | specify init --here` keeps reaching the
|
||||
# non-destructive preserve-merge path.
|
||||
try:
|
||||
proceed = typer.confirm(
|
||||
"Template files will be merged with existing content "
|
||||
"and may overwrite existing files. Do you want to continue?"
|
||||
)
|
||||
except (typer.Abort, EOFError):
|
||||
# typer.confirm raises Abort for BOTH an interactive Ctrl+C
|
||||
# and an EOF on closed/empty stdin. Distinguish them: a real
|
||||
# TTY cancellation is a normal exit (0, "cancelled"), while a
|
||||
# missing-input EOF (non-interactive) becomes an actionable
|
||||
# error pointing at --force.
|
||||
if _stdin_is_interactive():
|
||||
console.print("[yellow]Operation cancelled[/yellow]")
|
||||
raise typer.Exit(0) from None
|
||||
console.print(
|
||||
"[red]Error:[/red] Current directory is not empty and no "
|
||||
"confirmation input is available. Re-run with "
|
||||
"[bold]--force[/bold] to merge into it."
|
||||
)
|
||||
raise typer.Exit(1) from None
|
||||
if not proceed:
|
||||
console.print("[yellow]Operation cancelled[/yellow]")
|
||||
raise typer.Exit(0)
|
||||
else:
|
||||
|
||||
@@ -115,6 +115,63 @@ class TestInitIntegrationFlag:
|
||||
data = json.loads((project / ".specify" / "integration.json").read_text(encoding="utf-8"))
|
||||
assert data["integration"] == specify_cli.DEFAULT_INIT_INTEGRATION
|
||||
|
||||
def test_init_here_nonempty_noninteractive_errors_with_force_guidance(self, tmp_path):
|
||||
"""`init --here` on a non-empty directory with no confirmation input (empty
|
||||
stdin) must fail fast with guidance to use --force, instead of the bare
|
||||
'Aborted.' from an EOF on typer.confirm. CliRunner with no `input=` provides
|
||||
empty stdin, so typer.confirm raises Abort, which the command converts to the
|
||||
actionable error."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
project = tmp_path / "nonempty-here"
|
||||
project.mkdir()
|
||||
(project / "existing.txt").write_text("keep me", encoding="utf-8")
|
||||
old_cwd = os.getcwd()
|
||||
try:
|
||||
os.chdir(project)
|
||||
result = CliRunner().invoke(app, [
|
||||
"init", "--here", "--integration", "copilot", "--script", "sh", "--ignore-agent-tools",
|
||||
], catch_exceptions=False)
|
||||
finally:
|
||||
os.chdir(old_cwd)
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "--force" in result.output
|
||||
# Aborted before scaffolding: the pre-existing file is untouched.
|
||||
assert (project / "existing.txt").read_text(encoding="utf-8") == "keep me"
|
||||
|
||||
def test_init_here_interactive_cancel_exits_zero(self, tmp_path, monkeypatch):
|
||||
"""An interactive Ctrl+C at the merge confirmation (typer.Abort on a TTY)
|
||||
is a normal cancellation — exit 0, "cancelled" — NOT the missing-input
|
||||
--force error, which is reserved for non-interactive EOF. Guards the
|
||||
regression where Abort was caught unconditionally and every cancel became
|
||||
an exit-1 --force error."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
import specify_cli.commands.init as init_mod
|
||||
|
||||
# Simulate an interactive terminal so the Abort is treated as a cancel.
|
||||
monkeypatch.setattr(init_mod, "_stdin_is_interactive", lambda: True)
|
||||
|
||||
project = tmp_path / "cancel-here"
|
||||
project.mkdir()
|
||||
(project / "existing.txt").write_text("keep me", encoding="utf-8")
|
||||
old_cwd = os.getcwd()
|
||||
try:
|
||||
os.chdir(project)
|
||||
# No input → typer.confirm raises Abort (stands in for Ctrl+C).
|
||||
result = CliRunner().invoke(app, [
|
||||
"init", "--here", "--integration", "copilot", "--script", "sh", "--ignore-agent-tools",
|
||||
], catch_exceptions=False)
|
||||
finally:
|
||||
os.chdir(old_cwd)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "cancelled" in result.output.lower()
|
||||
assert "--force" not in result.output # not the missing-input error
|
||||
assert (project / "existing.txt").read_text(encoding="utf-8") == "keep me"
|
||||
|
||||
def test_integration_copilot_auto_promotes(self, tmp_path):
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
@@ -835,7 +892,8 @@ class TestInitIntegrationFlag:
|
||||
assert (scripts_dir / "common.sh").read_text(encoding="utf-8") != custom_content
|
||||
|
||||
def test_init_here_without_force_preserves_shared_infra(self, tmp_path):
|
||||
"""E2E: specify init --here (no --force) preserves existing shared infra files."""
|
||||
"""E2E: confirming the merge with piped "y" (no --force) preserves
|
||||
existing shared infra files (unlike --force, which overwrites them)."""
|
||||
from typer.testing import CliRunner
|
||||
from specify_cli import app
|
||||
|
||||
|
||||
Reference in New Issue
Block a user