Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 22 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]
# requires-python = ">=3.11" in pyproject.toml -- cover the floor
# and the latest supported minor.
python-version: ["3.11", "3.12"]
Expand Down Expand Up @@ -49,6 +49,25 @@ jobs:
strategy:
fail-fast: false
matrix:
# No windows-latest here, deliberately. Every test this job selects
# (`-m integration`) forks a real child process and, in most of the
# files, allocates a real pty pair via the POSIX-only `pty`/`termios`
# stdlib modules -- there is no Windows equivalent of either
# mechanism. `test_ctrlc_functional_integration.py` and
# `test_terminal_echo_integration.py` already `pytest.skip(...,
# allow_module_level=True)` on win32 (they import `pty`/`termios` at
# module scope, so skipping is the only way to avoid a collection
# error). `test_stdout_offload_freeze_integration.py` calls
# `os.fork()` directly with no platform guard at all, and `os.fork`
# simply does not exist on Windows -- it fails with
# `AttributeError: module 'os' has no attribute 'fork'`
# (confirmed on a real windows-latest run, job id 94301234220).
# A Windows leg of this job would therefore either run zero tests
# (all skipped) or hard-fail on the one file missing a guard -- pure
# CI theatre, burning runner minutes to report a "green" (or "red")
# that says nothing about Windows support. If a genuinely
# cross-platform integration test is ever added to this job, add
# windows-latest back for it specifically.
os: [ubuntu-latest, macos-latest]
steps:
- uses: actions/checkout@v4
Expand All @@ -66,5 +85,6 @@ jobs:
# -- the ones that fork a real pty child and probe real termios state
# -- are skipped by default and, before this job existed, ran nowhere.
# They are exactly the tests that guard the dedicated-tty-input
# mechanism, so they get their own job on both platforms.
# mechanism, so they get their own job (POSIX only -- see the
# `matrix.os` comment above for why Windows is excluded).
run: uv run pytest -m integration -q
29 changes: 29 additions & 0 deletions amplifier_app_cli/lib/bundle_loader/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import asyncio
import logging
import os
import re
from importlib import metadata
from pathlib import Path
from typing import Any
Expand All @@ -27,6 +28,17 @@

logger = logging.getLogger(__name__)

# The two absolute path forms Windows actually has: a drive letter
# (``C:\...`` or ``C:/...``) and a UNC share (``\\server\share``). Used to tell
# a local path from a package name in ``_parse_source`` -- see the comment at
# that call site for why a Windows path otherwise gets misread as a PyPI
# distribution name.
#
# Evaluated on every platform, which is safe: no legitimate package name
# contains a backslash or an ``X:`` drive prefix, and on POSIX the ``/`` and
# ``.`` prefixes short-circuit before this is ever reached.
_WINDOWS_ABSOLUTE_PATH_RE = re.compile(r"^(?:[A-Za-z]:[\\/]|\\\\)")


class ModuleResolutionError(Exception):
"""Error during module resolution."""
Expand Down Expand Up @@ -321,6 +333,23 @@ def _parse_source(
source.startswith("file://")
or source.startswith("/")
or source.startswith(".")
# A Windows absolute path matches none of the prefixes above -- not
# "file://", not "/", not "." -- so it used to fall through to the
# package-name branch below. A user putting a local path override in
# settings.yaml on Windows (`C:\src\my-module`) had it silently
# treated as a PyPI distribution name, and the resulting failure
# named a package that was never mentioned anywhere.
#
# Matches the two absolute forms Windows actually has: a drive
# letter (`C:\...` or `C:/...`) and a UNC share (`\\server\share`).
# A bare leading backslash (`\foo`, drive-relative) is deliberately
# NOT matched -- it is not absolute, and treating it as a path would
# be a guess.
#
# POSIX is unaffected: `/` and `.` already short-circuit before
# this, and no legitimate package name contains a backslash or a
# `X:` drive prefix.
or _WINDOWS_ABSOLUTE_PATH_RE.match(source) is not None
):
return FoundationFileSource(source)
# Assume package name
Expand Down
181 changes: 154 additions & 27 deletions amplifier_app_cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@
from prompt_toolkit.key_binding import KeyBindings
from rich.panel import Panel

# Errors that mean the terminal can never satisfy the REPL, so retrying is
# pointless. See the REPL loop's handler for the full story: on Windows without
# a real console, prompt_toolkit's Win32Output raises before the loop reaches
# any await, so the "keep going" catch-all turns into an unkillable busy spin.
#
# `prompt_toolkit.output.win32` asserts `sys.platform == "win32"` at import, so
# this must stay guarded. On POSIX the tuple is empty and `except ()` catches
# nothing -- the POSIX path is byte-identical in effect to what shipped.
if sys.platform == "win32": # pragma: no cover - platform-specific
from prompt_toolkit.output.win32 import NoConsoleScreenBufferError

_TERMINAL_UNUSABLE_ERRORS: tuple[type[BaseException], ...] = (
NoConsoleScreenBufferError,
)
else:
_TERMINAL_UNUSABLE_ERRORS = ()


from .commands.agents import agents as agents_group
from .commands.allowed_dirs import allowed_dirs as allowed_dirs_group
from .commands.bundle import bundle as bundle_group
Expand Down Expand Up @@ -66,6 +84,36 @@
from .utils.error_format import escape_markup
from .utils.version import get_core_version, get_version


def _report_terminal_unusable(exc: BaseException, *, verbose: bool = False) -> None:
"""Explain an unusable terminal in terms the user can act on.

Shared by every site that can hit ``_TERMINAL_UNUSABLE_ERRORS`` so the
message stays identical no matter where the failure surfaces. Without this,
a user piping or redirecting ``amplifier`` on Windows got a raw
prompt_toolkit traceback naming ``Win32Output`` -- accurate, but it points
at a library internal rather than at what they did or what to do instead.
"""
# Local imports: this helper is defined above main.py's own import block so
# it can sit next to the _TERMINAL_UNUSABLE_ERRORS tuple it belongs to.
from .console import console as _console
from .utils.error_format import escape_markup as _escape

_console.print(f"[red]Cannot run an interactive session:[/red] {_escape(exc)}")
_console.print(
"[yellow]The terminal has no console screen buffer. This happens when "
"output is piped or redirected, or when running without a real "
"console.[/yellow]"
)
_console.print(
"Run interactively in a real terminal (Windows Terminal, conhost, or "
"cmd.exe), or use a non-interactive command such as "
"[cyan]amplifier run[/cyan]."
)
if verbose:
_console.print_exception()


logger = logging.getLogger(__name__)

# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -3476,13 +3524,30 @@ async def interactive_chat(
)
)

# Create prompt session for history and advanced editing
prompt_session = _create_prompt_session(
get_active_mode=lambda: command_processor.session.coordinator.session_state.get(
"active_mode"
),
get_pinned_provider=lambda: _pinned_provider_name(command_processor.session),
)
# Create prompt session for history and advanced editing.
#
# This is the FIRST place an interactive session touches the terminal, and
# on Windows it is where an unusable terminal actually surfaces: building
# the prompt_toolkit Application resolves `get_app().output`, which
# constructs Win32Output, which raises NoConsoleScreenBufferError whenever
# stdout is not a real console (piped, redirected, CI, non-console parent).
#
# Guarding here rather than only at the REPL loop matters: measured on
# Windows, an unguarded `amplifier` with piped stdout died with a raw
# prompt_toolkit traceback out of this call, never reaching the loop. Unit
# tests miss it because they mock _create_prompt_session.
try:
prompt_session = _create_prompt_session(
get_active_mode=lambda: command_processor.session.coordinator.session_state.get(
"active_mode"
),
get_pinned_provider=lambda: _pinned_provider_name(command_processor.session),
)
except _TERMINAL_UNUSABLE_ERRORS as e:
_report_terminal_unusable(e, verbose=verbose)
await initialized.cleanup()
close_dedicated_tty_input()
return

# Helper to extract model name from config
def _extract_model_name() -> str:
Expand Down Expand Up @@ -3806,24 +3871,50 @@ def sigint_handler(signum, frame):
):
_streaming_hooks_instance.set_composing_source(None)

# Execute initial prompt if provided
if initial_prompt:
console.print(
f"\n[bold cyan]>[/bold cyan] {initial_prompt[:100]}{'...' if len(initial_prompt) > 100 else ''}"
)
console.print("\n[dim]Processing... (Ctrl+C to cancel)[/dim]")

# Process runtime @mentions in initial prompt
initial_prompt = await process_runtime_mentions(session, initial_prompt)
# NOTE: the /goal auto-continue loop lives in the orchestrator
# (loop-streaming's execute()), so the REPL calls the plain
# `_execute_with_interrupt` here -- the orchestrator drives
# auto-continuation internally via session_state["goal"]. See
# docs/GOAL_COMMAND.md.
await _execute_with_interrupt(initial_prompt)

# === REPL LOOP ===
# === REPL LOOP (and everything that must run under its cleanup) ===
#
# The try/finally starts HERE rather than at the loop, so the terminal
# check and the initial-prompt turn are both covered by the finally's
# teardown. An early `return` from inside a try still runs the finally,
# so bailing on an unusable terminal still awaits initialized.cleanup()
# and closes the dedicated tty fd -- leaking those was a real bug caught
# by test_interactive_chat_teardown_does_not_raise_when_fd_never_opened.
try:
# An interactive session needs a terminal prompt_toolkit can actually
# drive. Check ONCE, here, before any turn runs -- both the initial-prompt
# path below and the REPL loop wrap their work in `patch_stdout()`, and on
# Windows without a real console that raises NoConsoleScreenBufferError
# from Win32Output. Checking up front means one clear message instead of
# the same failure surfacing differently from two call sites.
if _TERMINAL_UNUSABLE_ERRORS:
try:
with patch_stdout():
pass
except _TERMINAL_UNUSABLE_ERRORS as e:
_report_terminal_unusable(e, verbose=verbose)
# No explicit teardown here: this `return` is inside the try,
# so the finally below runs and does the whole teardown --
# cleanup(), close_dedicated_tty_input(), the hook emits.
# Calling close_dedicated_tty_input() here as well double-fired
# it, which the teardown tests correctly caught.
return

# Execute initial prompt if provided
if initial_prompt:
console.print(
f"\n[bold cyan]>[/bold cyan] {initial_prompt[:100]}{'...' if len(initial_prompt) > 100 else ''}"
)
console.print("\n[dim]Processing... (Ctrl+C to cancel)[/dim]")

# Process runtime @mentions in initial prompt
initial_prompt = await process_runtime_mentions(session, initial_prompt)
# NOTE: the /goal auto-continue loop lives in the orchestrator
# (loop-streaming's execute()), so the REPL calls the plain
# `_execute_with_interrupt` here -- the orchestrator drives
# auto-continuation internally via session_state["goal"]. See
# docs/GOAL_COMMAND.md.
await _execute_with_interrupt(initial_prompt)

while True:
try:
# Get user input with history, editing, and paste support.
Expand Down Expand Up @@ -3923,6 +4014,44 @@ def sigint_handler(signum, frame):
except LLMError as e:
display_llm_error(console, e, verbose=verbose)

except _TERMINAL_UNUSABLE_ERRORS as e:
# MUST precede the catch-all below, and MUST break.
#
# On Windows with stdout not attached to a real console (piped,
# redirected, CI, a non-console parent), prompt_toolkit's
# Win32Output raises NoConsoleScreenBufferError. Critically it
# raises on ENTRY to `with patch_stdout():` -- before
# `await prompt_session.prompt_async()` -- so this loop
# iteration contains NO await point at all.
#
# Falling into the generic handler below therefore produced an
# infinite BUSY loop: raise, print, loop, raise... measured at
# 88% CPU on ALIENWARE-R13. And because the coroutine never
# yields, asyncio cannot interrupt it -- an
# `asyncio.wait_for(..., timeout=10)` around the whole call
# never fired. Not cancellable, not timeout-able; only SIGKILL
# ends it.
#
# A terminal that is not a console will not become one by
# trying again, so this is fatal to the REPL by definition.
# Fail loud and leave, rather than spin in a lesser state.
console.print(
f"[red]Cannot run an interactive session:[/red] {escape_markup(e)}"
)
console.print(
"[yellow]The terminal has no console screen buffer. This "
"happens when output is piped or redirected, or when "
"running without a real console.[/yellow]"
)
console.print(
"Run interactively in a real terminal (Windows Terminal, "
"conhost, or cmd.exe), or use a non-interactive command "
"such as [cyan]amplifier run[/cyan]."
)
if verbose:
console.print_exception()
break

except Exception as e:
console.print(f"[red]Error:[/red] {escape_markup(e)}")
if verbose:
Expand Down Expand Up @@ -4119,9 +4248,7 @@ async def execute_single(
# condition is re-sent to the evaluator model every turn;
# without this it would see the literal "@file" token
# forever instead of the file's content.
goal_condition = await process_runtime_mentions(
session, goal_condition
)
goal_condition = await process_runtime_mentions(session, goal_condition)

session.coordinator.session_state["goal"] = {
"condition": goal_condition,
Expand Down
13 changes: 10 additions & 3 deletions tests/lib/mention_loading/test_deduplicator.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,16 @@ def test_deduplicator_duplicate_content():
ctx_file = files[0]
assert ctx_file.content == content
assert len(ctx_file.paths) == 3
assert Path("/path1/file.md") in ctx_file.paths
assert Path("/path2/file.md") in ctx_file.paths
assert Path("/path3/file.md") in ctx_file.paths
# Compare against RESOLVED paths. add_file() deliberately calls
# path.resolve() so a relative and an absolute reference to the same file
# deduplicate. On POSIX "/path1/file.md" is already canonical so the
# unresolved form happened to match; on Windows resolve() prepends the
# current drive, giving WindowsPath("C:/path1/file.md") and the bare
# comparison failed. Resolving both sides tests the actual contract --
# "the path I added is tracked" -- instead of an accident of POSIX.
assert Path("/path1/file.md").resolve() in ctx_file.paths
assert Path("/path2/file.md").resolve() in ctx_file.paths
assert Path("/path3/file.md").resolve() in ctx_file.paths


def test_deduplicator_same_path_twice():
Expand Down
28 changes: 25 additions & 3 deletions tests/test_always_render_final_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,41 @@
GREEN phase: Once the gate is removed and _streaming_overlay_active is deleted
the calls go through and both assertions pass.
"""

from __future__ import annotations

import sys
from pathlib import Path
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
from prompt_toolkit.application import create_app_session
from prompt_toolkit.output import DummyOutput

_MODULE = "amplifier_app_cli.main"


@pytest.fixture(autouse=True)
def _dummy_app_session():
"""Give prompt_toolkit an output it can use without a real console.

These tests drive the REAL ``interactive_chat``, whose REPL wraps each turn
in ``patch_stdout()``. That reaches ``get_app().output``; with no app
session it builds a platform Output, and on Windows
``Win32Output.__init__`` raises ``NoConsoleScreenBufferError`` whenever
stdout is not a real console (piped, redirected, CI). ``interactive_chat``
then correctly declines to start an interactive session, so no turn runs
and ``render_message`` is never called -- the tests failed for a reason
that has nothing to do with the always-render contract they exist to guard.

A ``DummyOutput`` app session removes that incidental dependency on the
host terminal. The assertions are unchanged and now hold on every platform
rather than only where a console happens to be attached.
"""
with create_app_session(output=DummyOutput()):
yield


# ---------------------------------------------------------------------------
# Session / initialized mock helpers (mirrors test_overlay_active_detection.py)
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -143,9 +167,7 @@ async def test_render_message_called_even_when_overlay_would_be_active(
mock_render.assert_called_once()

@pytest.mark.asyncio
async def test_render_message_called_when_no_streaming_config(
self, tmp_path: Path
):
async def test_render_message_called_when_no_streaming_config(self, tmp_path: Path):
"""render_message IS called when no streaming-ui hook is configured.

Sanity-check: the non-streaming path must also always render.
Expand Down
Loading
Loading