Skip to content

fix: make amplifier-agent work on native Windows (stdio encoding, getsid, fcntl, git prerequisite) - #126

Merged
Salil Das (sadlilas) merged 8 commits into
mainfrom
fix/windows-support
Aug 18, 2026
Merged

fix: make amplifier-agent work on native Windows (stdio encoding, getsid, fcntl, git prerequisite)#126
Salil Das (sadlilas) merged 8 commits into
mainfrom
fix/windows-support

Conversation

@DavidKoleczek

@DavidKoleczek David Koleczek (DavidKoleczek) commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Makes amplifier-agent actually run on a bare native Windows host. Three independent code defects, each on its own commit, plus the documentation gap that made the failure unrecoverable for a new user.

What this fixes

1. 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 tested). 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/stderr to UTF-8 before dispatch, honoring an explicit PYTHONIOENCODING and skipping streams already UTF-8 or not reconfigurable.

Not gated on platform: the trigger is a stdio encoding that cannot represent the payload, not the OS. POSIX under LC_ALL=C PYTHONCOERCECLOCALE=0 PYTHONUTF8=0 fails identically. On a normal UTF-8 host this is a no-op.

Two corrections to the original report, confirmed while fixing it. First, the exposed surface is exactly one line — single_turn.py's text-mode reply write. The JSON envelope paths are ASCII by construction via json.dumps(ensure_ascii=True), jsonrpc.write_message encodes to UTF-8 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. What actually crashes is a character with no cp1252 mapping, such as U+2713 or CJK.

2. AttributeError: module 'os' has no attribute 'getsid'.

The session-leader step at the top of run called a POSIX-only API unconditionally.

3. fcntl imported at module scope in migration.py.

fcntl is Unix-only, and the module says so in its own docstring — the defect was expressing that scope as a bare import, which broke the entire CLI rather than the one command that cannot work there. fcntl is now imported optionally and file_lock raises MigrationUnsupportedError when it is absent. amplifier-agent migrate on such a platform 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.

4. git was a silent, undocumented requirement.

install.sh 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 twice: while priming the cache during install, and every subsequent time a bundle is mounted at run time. On a bare Windows host, where git is not present by default, this surfaced as an opaque clone failure naming neither the missing dependency nor the remedy.

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, and the distinction is worth stating plainly: 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 at all necessarily already has git, and an installer-side check can never fire for them. install.sh did also gain an up-front check with a per-platform install hint, but its real coverage is the environments where bash is present without git: minimal Linux containers, slim CI images, and fresh WSL distributions.

Scope: what this deliberately does NOT do

An earlier revision of this PR also swapped the shell tool on Windows, mounting tool-pwsh (from amplifier-bundle-windows-shell) instead of tool-bash. That commit has been reverted and is no longer part of this PR.

Why it was pulled out:

  • It was a feature riding inside a bugfix PR, and much the largest and riskiest part of the diff.
  • It depended on amplifier-bundle-windows-shell, a single-commit proof-of-concept repo with no tags and no CI, pinned by commit SHA.
  • It changed the tool name the model sees (bashpwsh) on one platform. That is a behavioral fork with real blast radius: anything matching the shell by literal tool name has to name both, and the shipped plan and brainstorm mode tool policies had to be edited or the shell would have been unpoliced on Windows.
  • It cost every POSIX user an extra clone during cold-prepare, because the manifest is static and its sha256 is the prepared-cache key, so there is no conditional form — both tools had to be declared and installed.

The underlying Windows shell problem is real, and is being fixed in tool-bash itself, where it belongs:

Combined with the git prerequisite documented here — Git for Windows supplies the bash that tool-bash looks for — a Windows user gets a working shell without a platform-forked tool name.

Verification

The three code defects were each reproduced on a Windows 11 guest before the fix and confirmed after; the per-commit messages record the console code page, the failing traceback, and the post-fix behavior. The installer's git check was exercised directly by running install.sh under a PATH with git removed, confirming it refuses with the per-platform hint and exits 1. main has been merged into the branch and it is conflict-free.

`amplifier_agent_lib/migration.py` imported `fcntl` unconditionally, and it
sits on the CLI's eager import path (`__main__.py` -> `admin/migrate.py` ->
here). `__main__.py` registers all 15 commands up front, so a module needed by
exactly one subcommand took down every subcommand, `--version` included, with
`ModuleNotFoundError: No module named 'fcntl'`.

The module was always scoped to Unix and says so in its own docstring. The
defect was expressing that scope as a bare import rather than a runtime check.

`fcntl` is now imported optionally and `file_lock` raises the new
`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 exits 1 with
`{"error": "migration-unsupported: ..."}` in JSON mode and a message on stderr
in text mode. That key is distinct from the two existing failure keys because
nothing failed and nothing was moved; reporting it as a failed migration would
suggest a partial one.

Refusing rather than locking as a no-op is deliberate. Both migrations move
user data and re-check their preconditions under the lock precisely because a
concurrent process could otherwise move the same tree twice, so running
unlocked would trade a documented platform limitation for a data-loss race.

Verified on a Windows 11 guest running the same ref: unpatched upstream
reproduces the three-hop traceback and exits 1; with this change `--version`,
`doctor` (11/11), and `verify` all exit 0, and `migrate` refuses cleanly in
both output formats. No behavior change on Linux or macOS.

This bug class has no automated regression coverage. The e2e harness
provisions Linux, where `fcntl` imports fine, so any e2e case asserting
`--version` exit 0 is green both before and after this change.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
`run` called `os.getsid(0)` and `os.setsid()` unconditionally at the top of its
callback, guarded by `except (OSError, PermissionError)`. Neither name exists on
Windows and the raised `AttributeError` was not in that tuple, so every `run`
died before reaching any engine code:

    AttributeError: module 'os' has no attribute 'getsid'

The step is now gated on `hasattr(os, "setsid")`, and the
`AMPLIFIER_AGENT_DEBUG_SIDLOG` branch reports `n/a` rather than raising from the
same missing name. The capability is checked rather than the platform, because
what the code needs is the call, not the OS.

On POSIX nothing changes: the engine still becomes session leader, confirmed by
`AMPLIFIER_AGENT_DEBUG_SIDLOG` reporting `sid == pid`.

This deliberately does not give Windows an equivalent guarantee. Windows has no
session groups, and the containment primitive is a Job Object, a different
mechanism on both sides of the wrapper boundary rather than a flag on this one.
Swallowing the error silently would leave a documented contract reading as
satisfied on a platform that cannot satisfy it, so the gap is written down
instead: cancellation on Windows reaches the engine, and MCP children may
outlive it. See docs/spec/wrapper-contract.md.

Verified on a Windows 11 guest: unpatched reproduces the AttributeError at
single_turn.py:669 and exits 1; with this change `run` completes a real LLM
turn and exits 0. Linux e2e `run` suite passes.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Python selects the console code page for stdio. On Windows that 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:

    _real_stdout.write(result.get("reply", "") + "\n")
      File ".../encodings/cp1252.py", line 19, in encode
    UnicodeEncodeError: 'charmap' codec can't encode character '\u2713'

The failure lands after the model call has run and been billed, which is the
worst available moment: the work is done, paid for, and then discarded.

`main()` now reconfigures stdout and stderr to UTF-8 before dispatch. An
explicit PYTHONIOENCODING wins, because an operator who named an encoding
outranks this default even when the one they named is narrow. Streams that are
already UTF-8, or that cannot be reconfigured, are skipped rather than fought.

Not gated on the platform. The trigger is a stdio encoding that cannot
represent the payload, not the OS: POSIX under
`LC_ALL=C PYTHONCOERCECLOCALE=0 PYTHONUTF8=0` selects ASCII and fails
identically. On a normal UTF-8 host the call is a no-op.

Two corrections to the original Windows report, found while confirming this.

The exposed surface is one line, not the general stdout path. The JSON envelope
writes go through `json.dumps`, whose default `ensure_ascii=True` makes them
ASCII by construction; `jsonrpc.write_message` encodes to UTF-8 bytes itself;
`click.echo` does its own encoding and never raises. Only the text-mode reply
write is exposed.

An em dash is not the trigger. U+2014 is representable in cp1252, so em dashes
in --help and in skill descriptions degrade to a replacement character rather
than crashing. Reproducing the reported failure requires a character with no
cp1252 mapping, such as U+2713 or CJK.

Verified on a Windows 11 guest, console code page 437, Python stdout cp1252:
unpatched reproduces the UnicodeEncodeError at the reply write and exits 1;
with this change the same prompt returns `DONE <U+2713> OK` and exits 0. Linux
`make check` clean and e2e `run` + `modes` suites pass (16).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
`tool-bash` cannot find a shell on Windows. It resolves via
`shutil.which("bash")`, and Git for Windows installs `bash.exe` into
`C:\Program Files\Git\bin` while adding only `C:\Program Files\Git\cmd` to
PATH. The lookup returns None and commands fall through to a branch that execs
the command name as a binary. Driving the tool directly on a Windows guest:

    shutil.which("bash") -> None
    tool-bash.execute({"command": "pwd"})
      success = False
      output  = '[WinError 2] The system cannot find the file specified'

No mention of bash, no remedy, nothing the model can act on. The original report
described the model retrying and concluding the tool was broken. What we
measured is worse: given that error the model did not report a failure at all,
it fabricated a plausible working directory and presented it as command output.
An unactionable error does not reliably surface as a visible failure; it can
surface as a confident wrong answer, which is the more expensive outcome.

bundle.md now declares both shell modules and `shell_tool.select_shell_tool`
drops the wrong one at the bundle-prep seam -- one place, reached by both the
CLI and HTTP faces, running before the host-config merge so every later step and
the kernel see the final roster.

Declaring both is what makes this expressible. The manifest is static and its
sha256 is the prepared-cache key, so there is no conditional form to hang a
platform predicate on. Both are therefore installed during cold-prepare, costing
one extra clone of a zero-dependency package on POSIX.

The swap is not a rename, and that is deliberate. The tool is named `pwsh`
because the tool name is a strong prior on the syntax the model emits; a `bash`
tool that runs PowerShell would invite bash syntax and fail differently. The
cost is that anything matching the shell by literal tool name must name both, so
the shipped `plan` and `brainstorm` modes now list `bash` and `pwsh` in their
tool policies. Without that the shell would be entirely unpoliced on Windows,
which is a worse failure than the one being fixed. Sub-agents need no change:
they inherit the already-filtered parent roster.

`tool-pwsh` is pinned to a commit rather than `@main`.
amplifier-bundle-windows-shell is a single-commit proof-of-concept with no tags
and no CI, so `@main` would let an upstream force-push change what users install
with no signal here.

Verified on a Windows 11 guest: cold-prepare installs tool-pwsh and primes the
cache; `select_shell_tool` reports `dropped: tool-bash, roster: ['tool-pwsh',
...]`; driving tool-pwsh directly returns success=True with real stdout; and an
agent turn shows `[tool/started] pwsh` returning the true working directory.
PowerShell 7 is absent on that guest and the documented 5.1 fallback is what
runs, so no PATH repair or extra install is required.

Linux: `make check` clean; full e2e 44 passed. The 11 github_copilot failures
are the harness's own documented consequence of GITHUB_TOKEN being unset, and
the single shadowing error was a port-9098 collision that passes 6/6 on re-run.
The DTU log confirms the filter live: `shell tool: kept tool-bash, dropped
tool-pwsh (platform=linux)`.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
git was a silent, undocumented requirement. install.sh 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 twice: while
priming the bundle cache during install, and every subsequent time a bundle
is mounted at run time. On a bare Windows host, where git is not present by
default, this surfaced as an opaque clone failure that named neither the
missing dependency nor the remedy.

- install.sh now refuses up front when git is absent, with a per-platform
  install hint (xcode-select / package manager / Git for Windows).
- README.md and docs/INSTALL.md list git alongside uv and curl.
- docs/INSTALL.md notes that git is needed at run time, and that Git for
  Windows supplies both git and the bash the shell tool looks for.

Co-Authored-By: Amplifier <amplifier@microsoft.com>
@sadlilas Salil Das (sadlilas) changed the title fix(engine): make amplifier-agent work on native Windows fix: make amplifier-agent work on native Windows (stdio encoding, getsid, fcntl, git prerequisite) Aug 18, 2026
…k covers

The prior wording juxtaposed the bare-Windows failure with the new
install.sh check, implying the installer is what rescues that user. It
cannot be: install.sh is a bash script, and on Windows bash is supplied
by Git for Windows, which also supplies git -- so anyone able to run the
installer already has git and the check never fires for them.

The README and INSTALL.md entries are what carry this on Windows. The
install.sh check covers the environments that genuinely have bash without
git: minimal Linux containers, slim CI images, and fresh WSL distributions.

Co-Authored-By: Amplifier <amplifier@microsoft.com>
@sadlilas
Salil Das (sadlilas) merged commit aac2ff7 into main Aug 18, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants