diff --git a/amplifier_app_cli/commands/run.py b/amplifier_app_cli/commands/run.py index f09af52b..687581f9 100644 --- a/amplifier_app_cli/commands/run.py +++ b/amplifier_app_cli/commands/run.py @@ -3,35 +3,231 @@ from __future__ import annotations import asyncio +import contextlib import logging +import signal import sys +import threading import uuid -from collections.abc import Callable +from collections.abc import Callable, Iterator from typing import Any import click - -from rich.panel import Panel - from amplifier_foundation.exceptions import BundleError, BundleValidationError from amplifier_foundation.modules import ModuleActivationError +from rich.panel import Panel from ..console import console -from ..session_store import extract_session_mode -from ..utils.error_format import escape_markup from ..effective_config import get_effective_config_summary from ..lib.settings import AppSettings from ..paths import create_config_manager from ..runtime.config import resolve_config +from ..session_store import extract_session_mode from ..types import ( ExecuteSingleProtocol, InteractiveChatProtocol, SearchPathProviderProtocol, ) +from ..utils.error_format import escape_markup logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _scoped_sigint_handler(handler: Any) -> Iterator[bool]: + """Install a SIGINT handler for the duration of the block, where possible. + + ``signal.signal()`` is only callable from the main thread of the main + interpreter; anywhere else CPython raises + ``ValueError: signal only works in main thread of the main interpreter``. + + Before GAP-023 and GAP-027, the two interruptible phases in this module + (the startup update check and bundle preparation) installed no handler at + all, so both were safe to call from any thread. Adding an unguarded + ``signal.signal()`` call narrowed that contract: it makes those phases + raise for any caller that does not own the main thread. + + No caller that actually reaches these phases off the main thread has been + identified. Two candidate embedders were checked and neither reaches them: + ``amplifierd`` does not depend on this package at all, and + ``amplifier-app-actions`` imports only ``console`` and ``session_runner``, + not this module. So this guard is preventive, not a fix for an observed + field failure -- it restores the "safe to call from any thread" contract + that GAP-023/GAP-027 silently removed, at no cost. + + Declining to install leaves the caller without the Ctrl+C acknowledgment + it never had before those fixes, and changes nothing else. That is a + deliberate restoration of shipped behavior for a context where signal + handling is structurally unavailable, not an error being swallowed -- so + it is reported at debug level and the block runs either way. + + Yields True if the handler was installed, False if it was declined. + """ + if threading.current_thread() is not threading.main_thread(): + logger.debug( + "SIGINT handler not installed: not on the main thread (%s). " + "Interrupt acknowledgment is unavailable in this context.", + threading.current_thread().name, + ) + yield False + return + + try: + original = signal.signal(signal.SIGINT, handler) + except ValueError as exc: + # Reachable on the main thread of a *subinterpreter*, where + # threading.main_thread() reports that subinterpreter's own main + # thread but signal.signal() still refuses. + logger.debug("SIGINT handler not installed: %s", exc) + yield False + return + + try: + yield True + finally: + signal.signal(signal.SIGINT, original) + + +def _run_startup_update_check() -> None: + """Run the startup update check, but let Ctrl+C skip it immediately and + fall through to the user's actual command instead of the whole + invocation dying (GAP-023). + + Before this fix, the pre-REPL "Checking for updates..." phase (this + function's caller, ``asyncio.run(check_and_notify())``) ran with no + SIGINT handling of its own -- ``_execute_with_interrupt``'s handler + (main.py, installed inside the per-turn wrapper) and the headless goal + path's ``_goal_sigint_handler`` (main.py) both install a handler + *around the operation they guard*; this phase runs earlier than either, + so it had no equivalent. A Ctrl+C here fell through to Click's default + top-level (EOFError, KeyboardInterrupt) handler, which prints + "Aborted!" and kills the ENTIRE ``amplifier run`` invocation -- not + just the optional, best-effort update check. Measured on native + Windows: the interrupt was not merely delayed, it destroyed the user's + whole command over an update check they never asked to wait on. + + Fix: run the check on its own event loop with its own SIGINT handler + (same pattern as the two handlers above), so a Ctrl+C here cancels only + this task -- printing a clear message -- and the caller proceeds to run + the user's actual prompt/command normally. + """ + from ..utils.startup_checker import check_and_notify + + interrupted = False + + def _update_check_sigint_handler(signum, frame): + nonlocal interrupted + interrupted = True + for task in asyncio.all_tasks(loop): + task.cancel() + + loop = asyncio.new_event_loop() + with _scoped_sigint_handler(_update_check_sigint_handler): + try: + asyncio.set_event_loop(loop) + task = loop.create_task(check_and_notify()) + try: + loop.run_until_complete(task) + except asyncio.CancelledError: + pass + finally: + asyncio.set_event_loop(None) + loop.close() + + if interrupted: + console.print("[dim]Update check skipped (Ctrl+C) -- continuing...[/dim]") + + +def _resolve_config_interruptibly( + *, + bundle_name: str | None, + app_settings: AppSettings, + console: Any, +) -> tuple[dict[str, Any], Any]: + """Wrap resolve_config() with a scoped SIGINT handler and a clean + cancellation message (GAP-027). + + resolve_config() -- bundle discovery, git clone/fetch, compose, + activate -- runs EARLIER than every other SIGINT-aware phase in this + file: earlier than _run_startup_update_check() (GAP-023's own scoped + handler, above) and earlier than main.py's per-turn + _execute_with_interrupt() handler. Before this fix it had none of its + own, so a Ctrl+C here fell all the way through to Python's raw + default SIGINT handler with: + + 1. No acknowledgment at all -- every OTHER interruptible phase in + the app prints something ("Update check skipped...", "Stopping + after current operation completes...", etc.) the instant Ctrl+C + is pressed. This phase printed nothing, for however long the + unwind took. + + 2. A non-deterministic landing spot. resolve_config() calls through + many layers of third-party library code (git subprocess calls, + pydantic schema validation, importlib.metadata lookups, etc.) + with no exception handling of its own around the interrupt. + Confirmed on native Windows: the IDENTICAL repro (a bundle + source pointed at a black-holed IP so the git clone blocks + deterministically, Ctrl+C sent ~6s after spawn), run twice back + to back, produced two different failure modes from the same + keystroke: once the process ran on completely unaffected for + 60+ seconds (the interrupt seemingly lost), and once it died in + ~2s but with a BARE, unhandled Python traceback dumped straight + to the user's terminal -- ending in a lone "KeyboardInterrupt" + with zero context, landing inside pydantic's + complete_model_class -> create_schema_validator -> + importlib.metadata.entry_points() chain, nothing to do with git + at all. Neither outcome is acceptable, and which one a user gets + is pure timing luck. + + This is a genuinely different location and cause from GAP-014 + (git.py's unbounded wait), GAP-023 (the update-check phase, which + runs AFTER this one), GAP-025 (git.py's BaseException cleanup), and + GAP-026 (subprocess_runner.py's delegation cancellation) -- none of + those touch this call site, and none of them install any + acknowledgment or containment for an interrupt landing here. + + Fix: same established pattern as _run_startup_update_check() -- a + scoped SIGINT handler installed only for the duration of this call, + printing the same "Cancelling..." convention used everywhere else in + the app the moment Ctrl+C is pressed (fixing the missing-feedback + symptom), while still delivering the real interrupt via + signal.default_int_handler so the existing unwind-and-cleanup + machinery (GAP-014/025's process-tree kill, etc.) runs exactly as it + did before this fix. The KeyboardInterrupt is then caught at THIS + single, deliberate point -- regardless of which arbitrary library + frame it actually surfaces in -- and converted into one clean, + actionable message and a normal process exit, instead of a raw + traceback landing wherever the timing happened to put it. + """ + interrupted = False + + def _bundle_prep_sigint_handler(signum: int, frame: Any) -> None: + nonlocal interrupted + if not interrupted: + interrupted = True + console.print( + "\n[yellow]Cancelling bundle preparation... " + "(this may take a moment to unwind cleanly)[/yellow]" + ) + # Still deliver the real interrupt so every existing unwind path + # (git.py's process-tree kill, etc.) behaves exactly as before -- + # this handler only ADDS an acknowledgment and a clean landing + # spot, it does not change what happens once the exception is + # actually raised. + signal.default_int_handler(signum, frame) + + with _scoped_sigint_handler(_bundle_prep_sigint_handler): + try: + return resolve_config( + bundle_name=bundle_name, + app_settings=app_settings, + console=console, + ) + except KeyboardInterrupt: + console.print("[red]Bundle preparation cancelled.[/red]") + sys.exit(130) + + def register_run_command( cli: click.Group, *, @@ -159,9 +355,12 @@ def run( # Track configuration source for display (always bundle mode now) config_source_name = f"bundle:{bundle}" - # Resolve configuration using unified function (single source of truth) + # Resolve configuration using unified function (single source of truth). + # GAP-027: wrapped for scoped SIGINT handling + a clean cancellation + # message instead of a raw traceback landing wherever an interrupt + # happens to surface (see _resolve_config_interruptibly docstring). try: - config_data, prepared_bundle = resolve_config( + config_data, prepared_bundle = _resolve_config_interruptibly( bundle_name=bundle, app_settings=app_settings, console=console, @@ -238,8 +437,7 @@ def run( target_idx = None for i, entry in enumerate(providers_list): if isinstance(entry, dict) and ( - entry.get("id") == provider - or entry.get("instance_id") == provider + entry.get("id") == provider or entry.get("instance_id") == provider ): target_idx = i break @@ -248,7 +446,10 @@ def run( # Pass 2: fallback — module-type match (original behavior). # Preserves single-instance usage: -p anthropic → provider-anthropic. for i, entry in enumerate(providers_list): - if isinstance(entry, dict) and entry.get("module") == provider_module: + if ( + isinstance(entry, dict) + and entry.get("module") == provider_module + ): target_idx = i break @@ -345,9 +546,7 @@ def run( prepared_bundle.mount_plan["providers"] = updated_providers # Run update check (uses unified startup_checker with settings.yaml) - from ..utils.startup_checker import check_and_notify - - asyncio.run(check_and_notify()) + _run_startup_update_check() if mode == "chat": # Interactive mode - supports optional initial_prompt for auto-execution diff --git a/amplifier_app_cli/utils/source_status.py b/amplifier_app_cli/utils/source_status.py index 82d747c8..2d2be017 100644 --- a/amplifier_app_cli/utils/source_status.py +++ b/amplifier_app_cli/utils/source_status.py @@ -4,6 +4,7 @@ Uses existing StandardModuleSourceResolver infrastructure. """ +import asyncio import json import logging import re @@ -205,11 +206,14 @@ async def _check_file_source( local_path = source.path - # Get local git info - local_sha = _get_local_sha(local_path) - remote_url = _get_remote_url(local_path) - uncommitted = _has_uncommitted_changes(local_path) - unpushed = _has_unpushed_commits(local_path) + # Get local git info. These helpers shell out to git synchronously; running + # them off-thread keeps the event loop free so a Ctrl+C during the startup + # update check is honoured at the next await instead of waiting for every + # git invocation to finish first. + local_sha = await asyncio.to_thread(_get_local_sha, local_path) + remote_url = await asyncio.to_thread(_get_remote_url, local_path) + uncommitted = await asyncio.to_thread(_has_uncommitted_changes, local_path) + unpushed = await asyncio.to_thread(_has_unpushed_commits, local_path) status = LocalFileStatus( name=name, @@ -226,7 +230,7 @@ async def _check_file_source( if remote_url and local_sha: try: # Get current branch - current_branch = _get_current_branch(local_path) + current_branch = await asyncio.to_thread(_get_current_branch, local_path) if current_branch: remote_sha = await _get_github_commit_sha( client, remote_url, current_branch @@ -234,7 +238,9 @@ async def _check_file_source( if remote_sha != local_sha: status.remote_sha = remote_sha[:7] - status.commits_behind = _count_commits_behind(local_path) + status.commits_behind = await asyncio.to_thread( + _count_commits_behind, local_path + ) except Exception as e: logger.debug(f"Could not check remote for {name}: {e}") diff --git a/tests/test_run_sigint_main_thread_guard.py b/tests/test_run_sigint_main_thread_guard.py new file mode 100644 index 00000000..3fbc6252 --- /dev/null +++ b/tests/test_run_sigint_main_thread_guard.py @@ -0,0 +1,128 @@ +"""Tests for the main-thread guard around SIGINT handler installation. + +`signal.signal()` is only callable from the main thread of the main +interpreter. Anywhere else CPython raises: + + ValueError: signal only works in main thread of the main interpreter + +Two interruptible phases in `amplifier_app_cli.commands.run` install SIGINT +handlers: the startup update check (GAP-023) and bundle preparation +(GAP-027). Neither installed a handler before those fixes, so both were safe +to call from any thread. Adding an unguarded `signal.signal()` call narrowed +that contract -- it makes those phases raise for any caller that does not own +the main thread. + +No caller that actually reaches these phases off the main thread has been +identified; two candidate embedders were checked and neither reaches them. +These tests therefore guard a contract rather than reproduce an observed +field failure: they pin "safe to call from any thread" so it cannot be +removed again silently. + +They exercise the real `_scoped_sigint_handler` and the real +`_run_startup_update_check`, not mocks of them, from a worker thread. +""" + +from __future__ import annotations + +import signal +import threading +from typing import Any +from unittest.mock import patch + +from amplifier_app_cli.commands.run import ( + _run_startup_update_check, + _scoped_sigint_handler, +) + + +def _noop_handler(signum: int, frame: Any) -> None: # pragma: no cover - never invoked + """Stand-in SIGINT handler. Installed and removed, never fired.""" + + +def _run_in_worker_thread(fn: Any) -> dict[str, Any]: + """Run `fn` on a non-main thread and capture its outcome. + + Returns a dict with either `result` or `exc`, so the caller can assert on + a raised exception rather than having it vanish into the thread. + """ + captured: dict[str, Any] = {} + + def target() -> None: + assert threading.current_thread() is not threading.main_thread() + try: + captured["result"] = fn() + except BaseException as exc: # noqa: BLE001 - deliberately capturing everything + captured["exc"] = exc + + thread = threading.Thread(target=target, name="sigint-guard-test-worker") + thread.start() + thread.join(timeout=30) + assert not thread.is_alive(), "worker thread did not finish" + return captured + + +def test_scoped_sigint_handler_declines_off_main_thread() -> None: + """Off the main thread the guard declines instead of raising ValueError. + + This is the regression. Without the guard, `signal.signal()` raises + `ValueError: signal only works in main thread of the main interpreter` + and the caller dies. + """ + + def use_guard() -> bool: + with _scoped_sigint_handler(_noop_handler) as installed: + return installed + + captured = _run_in_worker_thread(use_guard) + + assert "exc" not in captured, ( + f"guard raised off the main thread: {captured.get('exc')!r} " + "-- the main-thread guard is missing or ineffective" + ) + assert captured["result"] is False, ( + "guard reported a handler was installed off the main thread; " + "signal.signal() cannot succeed there" + ) + + +def test_scoped_sigint_handler_installs_and_restores_on_main_thread() -> None: + """On the main thread the handler is installed and then restored. + + Guards against a fix that works by simply never installing anything. + """ + assert threading.current_thread() is threading.main_thread() + + before = signal.getsignal(signal.SIGINT) + + with _scoped_sigint_handler(_noop_handler) as installed: + assert installed is True + assert signal.getsignal(signal.SIGINT) is _noop_handler + + assert signal.getsignal(signal.SIGINT) is before + + +def test_startup_update_check_does_not_raise_off_main_thread() -> None: + """The real update-check phase survives being run off the main thread. + + Integration-level counterpart to the unit test above: exercises the + actual function an embedder reaches, with only its network-touching + dependency stubbed out. + """ + + async def _fake_check_and_notify() -> None: + return None + + def run_check() -> str: + with patch( + "amplifier_app_cli.utils.startup_checker.check_and_notify", + _fake_check_and_notify, + ): + _run_startup_update_check() + return "completed" + + captured = _run_in_worker_thread(run_check) + + assert "exc" not in captured, ( + f"_run_startup_update_check raised off the main thread: {captured.get('exc')!r}" + ) + assert captured["result"] == "completed"