diff --git a/CHANGELOG.md b/CHANGELOG.md index fe69708f..93c7b74b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,83 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 since local servers commonly need none. Both are environment-only; the persisted credentials file is not consulted for this provider. Default model is `default`. +### Fixed + +- **`git` was a silent, undocumented requirement.** The installer checked for + `uv` and `curl` but not `git`, and neither `README.md` nor `docs/INSTALL.md` + listed it. `git` is not a build-time nicety: bundles and modules are fetched + by cloning git repositories, so a machine without `git` on `PATH` fails both + while priming the cache during install and every subsequent time a bundle is + mounted. On a bare Windows host -- where `git` is not present by default -- + this surfaced as an opaque clone failure with no statement of the missing + dependency. + + `README.md` and `docs/INSTALL.md` now list `git` alongside `uv` and `curl`, + noting that it is needed at run time and that Git for Windows supplies both + `git` and the `bash` the shell tool looks for. The documentation is what + carries this on Windows: `install.sh` is a bash script, and on Windows bash + is itself supplied by Git for Windows, so a user who can run the installer + necessarily already has `git`. `install.sh` also gained an up-front check + with a per-platform install hint, which covers the environments where bash + is present without `git` -- minimal Linux containers, slim CI images, and + fresh WSL distributions. + +- **`amplifier-agent run` crashed with `UnicodeEncodeError` after the turn had + already completed.** Python picks the console code page for stdio, which on + Windows is a legacy single-byte encoding (cp1252 on the guests we test), so + writing a reply containing any character outside that page raised at the + final write -- after the model call had run and been billed. `main()` now + reconfigures stdout and stderr to UTF-8 before dispatch, honoring an explicit + `PYTHONIOENCODING` and skipping streams that are already UTF-8 or cannot be + reconfigured. + + Not gated on the platform, because the trigger is a stdio encoding that + cannot represent the payload rather than the OS: POSIX under + `LC_ALL=C PYTHONCOERCECLOCALE=0 PYTHONUTF8=0` selects ASCII and fails + identically. On a normal UTF-8 host this is a no-op. + + Two corrections to the original Windows report while confirming this. First, + the exposed surface is exactly one line, `single_turn.py`'s text-mode reply + write: the JSON envelope paths go through `json.dumps`, whose default + `ensure_ascii=True` makes them ASCII by construction; `jsonrpc.write_message` + encodes to UTF-8 bytes itself; and `click.echo` does its own encoding and + never raises. Second, an em dash is *not* the trigger -- U+2014 is + representable in cp1252. Em dashes in `--help` and in skill descriptions + degrade to a replacement character rather than crashing. What actually + crashes is a character with no cp1252 mapping, such as U+2713 or CJK. + +- **`amplifier-agent run` crashed on Windows with `AttributeError: module 'os' + has no attribute 'getsid'`.** The SC-B session-leader step at the top of the + `run` callback called `os.getsid`/`os.setsid` unconditionally, and + `AttributeError` was not in its `except` tuple. Neither name exists on + Windows, so every `run` died before reaching any engine code. The step is now + gated on `hasattr(os, "setsid")` and the debug SIDLOG branch reports `n/a` + instead of raising. On POSIX the behavior is unchanged: the engine still + becomes session leader, verified by `AMPLIFIER_AGENT_DEBUG_SIDLOG` reporting + `sid == pid`. + + This does not give Windows an equivalent guarantee, and pretending otherwise + would be worse than the crash. Windows has no session groups; the containment + primitive is a Job Object, a different mechanism on both sides of the wrapper + boundary. Until that is built, cancellation on Windows reaches the engine but + MCP children may outlive it. Recorded in `docs/spec/wrapper-contract.md`. + +- **Every CLI command failed on Windows with `ModuleNotFoundError: No module + named 'fcntl'`**, `--version` included. `amplifier_agent_lib/migration.py` + imported `fcntl` unconditionally, and it sits on the CLI's eager import path + (`__main__.py` -> `admin/migrate.py` -> here), so a module that only one + subcommand needs took down every subcommand. The module was always scoped to + Unix and says so in its own docstring; the defect was expressing that scope + as a bare import. `fcntl` is now imported optionally and `file_lock` raises + `MigrationUnsupportedError` when it is absent, so the limitation is enforced + at call time and stays scoped to the one command that cannot work there. + `amplifier-agent migrate` on such a platform now exits 1 with + `{"error": "migration-unsupported: ..."}` rather than crashing the CLI. + Refusing rather than locking as a no-op is deliberate: the migrations move + user data and re-check their preconditions under the lock, so running + unlocked would trade a documented platform limitation for a data-loss race. + No behavior change on Linux or macOS. + ## [0.12.0] — 2026-07-29 ### Added diff --git a/README.md b/README.md index e6f82a79..b482b6fd 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ Public integrations run opencode, paperclip, and NanoClaw on it: see [who has in curl -fsSL https://raw.githubusercontent.com/microsoft/amplifier-agent/main/install.sh | bash ``` -Installs the latest release and primes the bundle cache so your first run is instant. Requires [`uv`](https://docs.astral.sh/uv/) and `curl`; the installer tells you what is missing rather than bootstrapping silently. +Installs the latest release and primes the bundle cache so your first run is instant. Requires [`uv`](https://docs.astral.sh/uv/), `curl`, and `git`; the installer tells you what is missing rather than bootstrapping silently. `git` is a runtime dependency too -- bundles and modules are fetched by cloning git repositories. To review the script first, pin a version, install without the script, or uninstall, see [`docs/INSTALL.md`](docs/INSTALL.md). diff --git a/docs/INSTALL.md b/docs/INSTALL.md index a6cfdee5..991c6f30 100644 --- a/docs/INSTALL.md +++ b/docs/INSTALL.md @@ -2,7 +2,9 @@ `amplifier-agent` is a Python tool installed with [`uv`](https://docs.astral.sh/uv/). The installer resolves the latest tagged release and installs from it. -**Prerequisites:** `uv` and `curl`. The installer tells you exactly what to install if either is missing. It will not bootstrap them silently. +**Prerequisites:** `uv`, `curl`, and `git`. The installer tells you exactly what to install if any is missing. It will not bootstrap them silently. + +`git` is needed at run time, not just at install time: bundles and modules are fetched by cloning git repositories, so a machine without `git` on `PATH` can neither prime the cache nor mount a bundle. On Windows, installing [Git for Windows](https://git-scm.com/download/win) satisfies this and also provides the `bash` that the shell tool looks for. ## Recommended diff --git a/docs/spec/cli.md b/docs/spec/cli.md index 66eb3fa7..8e88eb88 100644 --- a/docs/spec/cli.md +++ b/docs/spec/cli.md @@ -130,6 +130,9 @@ migrate [--output text|json] ~/.amplifier-agent). Idempotent. Exit 1 if either migration raises, else 0. JSON payload: {"sessions_migration": {migrated, skipped, collided}, "xdg_migration": {migrated, skipped, collided, from_xdg}} + Unix only. On a platform without flock it refuses at CALL time, exit 1 (see + storage-and-workspace.md). Its unavailability is scoped to this one subcommand: + every other subcommand, including `--version`, still dispatches normally there. version [--json] Plain: `amplifier-agent (wire )`. diff --git a/docs/spec/storage-and-workspace.md b/docs/spec/storage-and-workspace.md index a3935bd0..a5ff80e2 100644 --- a/docs/spec/storage-and-workspace.md +++ b/docs/spec/storage-and-workspace.md @@ -207,6 +207,12 @@ observe the completed state. A lock file is created if absent, and it is release is killed, so a crashed run never strands it. Migration is supported on Unix only; Windows is out of scope for both. +That scope is enforced at CALL time, not at import time. Where `flock` is unavailable, both +migrations refuse rather than run unlocked: these migrations move user data and re-check their +preconditions under the lock, so an unlocked run would trade a documented platform limitation for a +data-loss race. Refusing is also what keeps the limitation scoped to this one command instead of +breaking CLI dispatch for every other subcommand. + Sessions migration: ``` @@ -251,6 +257,10 @@ Exit 0 on success, including the nothing-to-do case. Exit 1 on either migration `{"error": "sessions-migration-failed: ..."}` or `{"error": "xdg-migration-failed: ..."}` in JSON mode and a message on stderr in text mode. +Exit 1 with `{"error": "migration-unsupported: ..."}` where the platform has no `flock`. A distinct +key from the two above because nothing failed and nothing was moved: the command is unavailable +here, and reporting it as a failed migration would suggest a partial one. + Logging cadence: ``` diff --git a/docs/spec/wrapper-contract.md b/docs/spec/wrapper-contract.md index b795800e..305bf1d1 100644 --- a/docs/spec/wrapper-contract.md +++ b/docs/spec/wrapper-contract.md @@ -274,6 +274,13 @@ Cancellation is idempotent, and dispose is an alias for it. Signaling only the engine PID would orphan the engine's MCP children, holding file descriptors, ports, and sockets open until the OS reaps them minutes later. +POSIX only, on both sides. Windows has no session groups and provides neither `os.getsid` nor +`os.setsid`, so the engine skips the session-leader step there, and a negative-PID group signal is +not a Windows operation. The containment primitive on Windows is a Job Object, a different +mechanism on both sides of the boundary rather than a flag on this one. Until that is built, the +guarantee above does not hold on Windows: cancelling reaches the engine, and MCP children may +outlive it. Engine startup and single-turn `run` are unaffected. + ## Non-goals - **The wrapper never sees or configures the bundle.** No mount plan crosses the boundary, no diff --git a/install.sh b/install.sh index a10b2efc..654fa412 100755 --- a/install.sh +++ b/install.sh @@ -125,6 +125,21 @@ if ! command -v curl > /dev/null 2>&1; then exit 1 fi +# Require git -- the bundle system clones module and bundle repositories, both +# while priming the cache below and every time a bundle is mounted at run time. +if ! command -v git > /dev/null 2>&1; then + printf 'error: git is required but not found on PATH.\n\n' >&2 + printf 'amplifier-agent fetches modules and bundles by cloning git repositories,\n' >&2 + printf 'both while priming the cache during this install and every time a bundle\n' >&2 + printf 'is mounted at run time. It is a runtime dependency, not just a build one.\n\n' >&2 + printf 'Install git:\n' >&2 + printf ' macOS: xcode-select --install\n' >&2 + printf ' Linux: your package manager, e.g. apt install git\n' >&2 + printf ' Windows: https://git-scm.com/download/win (Git for Windows)\n\n' >&2 + printf 'Then re-run this script.\n' >&2 + exit 1 +fi + # Require uv — never silently bootstrap it; direct the user instead. if ! command -v uv > /dev/null 2>&1; then printf 'error: uv is required but not found on PATH.\n\n' >&2 diff --git a/src/amplifier_agent_cli/__main__.py b/src/amplifier_agent_cli/__main__.py index 71303ac4..0d8aa0d3 100644 --- a/src/amplifier_agent_cli/__main__.py +++ b/src/amplifier_agent_cli/__main__.py @@ -27,6 +27,7 @@ from __future__ import annotations +import os import sys import click @@ -75,8 +76,45 @@ def cli(ctx: click.Context) -> None: cli.add_command(_providers_group, name="providers") +def _force_utf8_io() -> None: + """Make stdout and stderr UTF-8 unless the operator picked an encoding. + + Python selects the console code page for stdio, which on Windows is a + legacy single-byte encoding (cp1252 on the guests we test). Any non-ASCII + character then raises UnicodeEncodeError at write time -- after the turn has + run and been paid for, which is the worst possible moment to fail. This is + not an edge case: em dashes appear in this CLI's own --help text and in + bundled skill descriptions, so a reply quoting either one is enough. + + Not Windows-specific, and not gated on the platform. POSIX under LC_ALL=C + selects ASCII for the same reason and fails the same way; the trigger is a + stdio encoding that cannot represent the payload, not the OS. + + PYTHONIOENCODING is honored when set. An operator who named an encoding + outranks this default, including when they name a narrow one deliberately. + + Best-effort by construction: a replaced or non-reconfigurable stream is + skipped rather than fought, because failing here would break the CLI in + exactly the environments this is meant to protect. + """ + if os.environ.get("PYTHONIOENCODING"): + return + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is None: + continue + current = (getattr(stream, "encoding", "") or "").lower().replace("-", "_") + if current in ("utf_8", "utf8"): + continue + try: + reconfigure(encoding="utf-8") + except (OSError, ValueError): # pragma: no cover - stream refused + pass + + def main() -> None: """Entry point referenced by pyproject.toml [project.scripts].""" + _force_utf8_io() try: cli(standalone_mode=True) except KeyboardInterrupt: diff --git a/src/amplifier_agent_cli/admin/migrate.py b/src/amplifier_agent_cli/admin/migrate.py index ca27e26e..77c18ab6 100644 --- a/src/amplifier_agent_cli/admin/migrate.py +++ b/src/amplifier_agent_cli/admin/migrate.py @@ -12,10 +12,12 @@ import json import sys +from typing import NoReturn import click from amplifier_agent_lib.migration import ( + MigrationUnsupportedError, maybe_migrate_legacy_xdg_storage, migrate_legacy_sessions_if_needed, ) @@ -23,6 +25,20 @@ __all__ = ["migrate_command"] +def _exit_unsupported(exc: MigrationUnsupportedError, output: str) -> NoReturn: + """Report an unsupported platform and exit non-zero. + + Distinct from the generic failure path: nothing went wrong and nothing was + moved, so the message says the command is unavailable here rather than + implying a partial or failed migration. + """ + if output == "json": + click.echo(json.dumps({"error": f"migration-unsupported: {exc}"})) + else: + click.echo(f"Error: {exc}", err=True) + sys.exit(1) + + @click.command(name="migrate") @click.option( "--output", @@ -35,6 +51,8 @@ def migrate_command(output: str) -> None: """Migrate legacy storage layouts to current. Idempotent; safe to run multiple times. Reports what was moved.""" try: sessions = migrate_legacy_sessions_if_needed() + except MigrationUnsupportedError as exc: + _exit_unsupported(exc, output) except Exception as exc: if output == "json": click.echo(json.dumps({"error": f"sessions-migration-failed: {exc}"})) @@ -44,6 +62,8 @@ def migrate_command(output: str) -> None: try: xdg = maybe_migrate_legacy_xdg_storage() + except MigrationUnsupportedError as exc: + _exit_unsupported(exc, output) except Exception as exc: if output == "json": click.echo(json.dumps({"error": f"xdg-migration-failed: {exc}"})) diff --git a/src/amplifier_agent_cli/modes/single_turn.py b/src/amplifier_agent_cli/modes/single_turn.py index 74a1bc65..3d18a600 100644 --- a/src/amplifier_agent_cli/modes/single_turn.py +++ b/src/amplifier_agent_cli/modes/single_turn.py @@ -665,16 +665,25 @@ def run( # SC-B — engine becomes session leader so MCP child processes spawned via # tool-mcp.mount() inherit a shared session group. The wrapper kills the # group on cancel so children die with the parent. - try: - if os.getsid(0) != os.getpid(): - os.setsid() - except (OSError, PermissionError): - # Best-effort — running under a debugger or test harness that already - # owns a session may make setsid() fail; tolerate. - pass + # + # POSIX only. Windows has no session groups and provides neither os.getsid + # nor os.setsid, so this is skipped there rather than guessed at: the + # equivalent containment primitive is a Job Object, which is a different + # mechanism on both sides of the wrapper boundary. The consequence is + # recorded in docs/spec/wrapper-contract.md — on Windows, MCP children are + # not guaranteed to die with the engine on cancel. + if hasattr(os, "setsid"): + try: + if os.getsid(0) != os.getpid(): + os.setsid() + except (OSError, PermissionError): + # Best-effort — running under a debugger or test harness that already + # owns a session may make setsid() fail; tolerate. + pass if os.environ.get("AMPLIFIER_AGENT_DEBUG_SIDLOG"): try: - sys.stderr.write(f"engine-sid-ok pid={os.getpid()} sid={os.getsid(0)}\n") + sid = os.getsid(0) if hasattr(os, "getsid") else "n/a" + sys.stderr.write(f"engine-sid-ok pid={os.getpid()} sid={sid}\n") except OSError: pass diff --git a/src/amplifier_agent_lib/migration.py b/src/amplifier_agent_lib/migration.py index 7dffee50..7ab0cb2e 100644 --- a/src/amplifier_agent_lib/migration.py +++ b/src/amplifier_agent_lib/migration.py @@ -14,12 +14,18 @@ Idempotent via sentinel file at /.migrated_from_xdg. Unix-only (fcntl.flock). AAA targets Linux/macOS; Windows is out of scope. + +That scope is enforced at CALL time, not at import time. This module sits on +the CLI's eager import path (__main__.py -> admin/migrate.py -> here), so an +unguarded ``import fcntl`` would take the entire CLI down on Windows rather +than just the one command that cannot work there. The import is therefore +optional, and ``file_lock`` raises MigrationUnsupportedError when locking is +unavailable. """ from __future__ import annotations import contextlib -import fcntl import logging import os import shutil @@ -30,11 +36,28 @@ from amplifier_agent_lib.persistence import _home, amplifier_agent_home, state_root +try: + import fcntl +except ImportError: # pragma: no cover - Windows; see module docstring + fcntl = None # type: ignore[assignment] + logger = logging.getLogger(__name__) LEGACY_WORKSPACE = "_legacy" +class MigrationUnsupportedError(RuntimeError): + """Raised when migration cannot run safely on this platform. + + Migration moves user data between storage roots, and both migrations + re-check their preconditions under an exclusive lock precisely because a + concurrent process could otherwise move the same tree twice. Without + ``fcntl.flock`` there is no lock, so the correct behavior is to refuse + rather than to run unguarded: a silent no-op lock would turn a documented + platform limitation into a data-loss race. + """ + + @dataclass class MigrationResult: """Outcome of a migration attempt.""" @@ -52,7 +75,16 @@ def file_lock(lock_path: Path) -> Iterator[None]: The lock file is created if absent. The kernel releases the lock when the file descriptor closes (on context exit or process death), so a killed process never strands the lock. + + Raises: + MigrationUnsupportedError: if ``fcntl`` is unavailable (Windows). """ + if fcntl is None: + raise MigrationUnsupportedError( + "Storage migration requires file locking (fcntl.flock), which is " + "not available on this platform. amplifier-agent migrate is " + "supported on Linux and macOS only." + ) lock_path.parent.mkdir(parents=True, exist_ok=True) fd = open(lock_path, "w") try: