Skip to content

fix: make native Windows collectable and runnable - #259

Merged
Salil Das (sadlilas) merged 6 commits into
mainfrom
fix/gap-003-020-023-027-021
Aug 18, 2026
Merged

fix: make native Windows collectable and runnable#259
Salil Das (sadlilas) merged 6 commits into
mainfrom
fix/gap-003-020-023-027-021

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Review status — what is and is not validated

Validated for this PR: all five fixes have real before/after output captured from native
Windows; the full amplifier-app-cli suite runs green against a git stash baseline on Linux,
macOS (both a DTU and native Darwin), and WSL2, with identical pre-existing failure sets on every
platform; both new regression tests were demonstrated to fail when their fix is reverted.

Not yet done, and deliberately not blocking this PR: three project-wide assurance layers are
still in flight across all four PRs in this effort — an ecosystem blast-radius survey (reading the
real consumers of every changed function), a DTU matrix across realistic bundle combinations, and
a review pass by a separate agent instance (see the note below on why we no longer call that "independent"). If you would rather
wait for those before spending time here, say so and I will hold.

Known limit: Windows evidence is from one machine (alienware-r13). The code-logic fixes are
platform-conditional bugs and should generalise; where a fix's trigger was environmental, it is
called out inline below.


Summary

Five Windows compatibility gaps discovered in native testing, now fixed and proven on Windows, Linux, macOS, and WSL:

GAP-003: Silent wrong-provider selection

detect_provider_from_env() treated "this provider's module failed to load" identically to "no credentials", so with a valid ANTHROPIC_API_KEY present it silently fell through to the credential-free Ollama fallback, persisted that to settings.yaml, and never retried.

Result: User got a connection error pointing at software they never installed, with no hint their real key was seen and discarded.

Fix: Adds CredentialedProviderModuleMissingError. Still prefers a lower-priority provider that is both credentialed and installed, and only raises when nothing else matches. Importantly: does not persist to settings.yaml on first run, so the next run gets a real second chance.

Proof: before → "Auto-configured ollama"; after → "Found credentials for anthropic (ANTHROPIC_API_KEY) but the 'provider-anthropic' module is not installed...". Verified the genuine no-credentials case still lands on Ollama quietly, byte-identical to pre-fix.

GAP-020: First-run prompt looped forever

The yes/no confirmation prompt re-prompted forever on any non-y/n input, with no escape hatch.

Fix: _bounded_confirm() bounds to 3 attempts, falling through to the same "setup skipped" path an explicit n produces.

GAP-023: Ctrl+C during update check killed the entire command

The "Checking for updates..." phase installed no SIGINT handler, so a bare Ctrl+C hit Click's default Aborted! handler, killing the whole command (~3s).

Fix: _run_startup_update_check() with a scoped SIGINT handler. Proof: before → Aborted!, whole command dies; after → Update check skipped (Ctrl+C) -- continuing..., bundle and provider loading proceed.

GAP-027: resolve_config() runs before all SIGINT handlers

resolve_config() runs earlier than every SIGINT-aware phase — no acknowledgment, no containment of the interrupt. The same repro gave a 60s+ silent hang one run and a raw KeyboardInterrupt traceback inside pydantic's plugin loader the other.

Fix: Scoped SIGINT handler mirroring GAP-023's pattern.

Proof (native Windows): before → 60s+ silent hang or raw traceback; after → Cancelling bundle preparation... / Bundle preparation cancelled., 1.00–1.01s recovery, twice, no traceback.

GAP-021: Arrow-key history never recalled the most recent message

prompt_toolkit's Buffer.reset() clears the navigation buffer and repopulates via a scheduled background task, not synchronously. A fast Up-arrow is processed before the task runs, showing a stale entry instead of the just-submitted message.

Isolated repro with identical config behaved correctly, ruling out the library itself. Instrumented the real binary: Buffer.history_backward called with idx still at the just-reset value on the first Up press.

Fix: Made the enter binding async def and awaited load_history_if_not_yet_loaded().

Proof (native Windows): before → Up → ZEBRA-ONE (wrong, skipping newest); after → Up → ZEBRA-TWO, Up → ZEBRA-ONE, Down walks back correctly.

Regression testing

Included: tests/test_provider_env_detect.py (9 new tests) exercising the real function (not mocked) for every branch: no-creds, creds+installed, creds+missing (raises, with and without Ollama available), and multi-provider fallthrough.

All platforms:

  • Linux: 1269 passed / 14 pre-existing failures, identical with and without the diff
  • macOS: 1269 passed / same 14 + 1 macOS-only pre-existing flake
  • WSL: 1308 passed

Scope and limitations

Windows evidence is n=1. All Windows proof comes from a single machine — alienware-r13, Windows NT 10.0.26200, Python 3.14. The code-logic fixes here generalize (they are platform-conditional logic bugs, not environment-specific). Where a fix's trigger was environmental, that is called out inline above.

Cross-platform validation. Linux (aarch64), macOS (Darwin arm64), and WSL2 were all validated — full regression suites on each, plus a real end-to-end pipe with live API calls on WSL. Zero regressions attributable to this diff on any platform.

Clean-install proof. Validated against a clean uv tool install from upstream HEAD (6c3fd86), not only the commit these changes were developed against.

How this was found. Part of a Windows-native gap investigation. Completeness was declared prematurely several times during that investigation and each premature call was later broken by adversarial re-testing — several fixes exist only because an earlier "this is done" was challenged. Treat this as what was found, not a closed set.


Added in 35a0647 — main-thread guard (fixes a regression introduced by GAP-023/GAP-027 on this branch)

The regression. GAP-023 and GAP-027 added SIGINT handlers to commands/run.py at four
points. Neither phase installed a handler before those fixes, so both ran fine anywhere.
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. Those
fixes therefore turned a working invocation into a hard crash for any caller not on the main
thread.

The fix. _scoped_sigint_handler, a context manager replacing all four raw signal.signal()
call sites. It declines to install off the main thread, and also catches ValueError for the
subinterpreter case (where threading.main_thread() reports that subinterpreter's own main thread
but signal.signal() still refuses). Declining restores exactly the pre-GAP-023/027 behaviour —
no handler, no crash — so it is logged at debug level rather than swallowed as an error.

Regression test, teeth demonstrated

tests/test_run_sigint_main_thread_guard.py. Reverting to the true pre-fix shape (raw
signal.signal(), neither mechanism present) produces:

E  AssertionError: _run_startup_update_check raised off the main thread:
E  ValueError('signal only works in main thread of the main interpreter')

FAILED tests/test_run_sigint_main_thread_guard.py::test_scoped_sigint_handler_declines_off_main_thread
FAILED tests/test_run_sigint_main_thread_guard.py::test_startup_update_check_does_not_raise_off_main_thread
2 failed, 1 passed in 0.05s

Restored → 3 passed. Source file verified byte-identical before and after (sha 3abfedee7d5a5d47).

A first teeth attempt was blind: it disabled only the thread check, and the ValueError catch
still covered, so the test passed either way. Recorded because it is the same class of failure as
a test whose mock has drifted from the implementation.

Cross-platform, all four platforms, all with a git stash baseline

Platform Baseline With change Guard test
Linux (aarch64) 15 failed / 1268 passed 15 failed / 1271 passed 3 passed
macOS — DTU (Incus, Linux aarch64, on brians-macbook-pro-os) 14 failed / 1267 passed 14 failed / 1270 passed 3 passed
macOS — native Darwin arm64 (HOME-isolated) 14 failed / 1268 passed 14 failed / 1271 passed 3 passed
WSL2 14 failed / 1267 passed 14 failed / 1270 passed 3 passed
Native Windows (NT 10.0.26200) 3 passed

Identical pre-existing failure sets on every platform; the delta is exactly the three new tests.

macOS is covered twice on purpose. The DTU run satisfies the "verify in a DTU on the Mac" policy, but a DTU is a Linux container — it exercises Linux, not Darwin. The native run, isolated via a HOME override so it cannot touch the daily-driver install, is the one that actually tests Darwin. Both agree.
Off-main-thread signal.signal() confirmed raising the same ValueError on native Windows as on
Linux.

Embedder reachability — honest correction

The regression was originally described as crashing embedders such as amplifierd. Examining the
code does not support that claim for the two embedders checked:

  • amplifierd does not depend on amplifier-app-cli at all. Its declared dependencies are
    fastapi, uvicorn, pydantic, pydantic-settings, sse-starlette, click, pyyaml, amplifier-core,
    amplifier-foundation. The only two textual matches are docstring comments. It cannot reach
    commands/run.py.
  • amplifier-app-actions does depend on amplifier-app-cli, importing
    amplifier_app_cli.console and amplifier_app_cli.session_runner — but it imports neither
    commands.run, register_run_command, _run_startup_update_check, nor
    _resolve_config_interruptibly.

So the crash is real in the code but not reached by either embedder examined. The guard is
still correct — it costs nothing and restores shipped behaviour for any off-main-thread caller —
but the severity claim is narrower than first stated.

Not exercised

  • amplifier-chat, amplifier-voice, amplifier-app-nanoclaw — not examined. The first two are
    amplifierd plugins, and amplifierd does not reach this path, so they are unlikely to.
  • No live embedder was run against the guard; reachability was determined by reading declared
    dependencies and imports.
  • The subinterpreter ValueError branch is covered by inspection, not by an executing test —
    no subinterpreter harness was built.
  • Windows was verified with the guard test only, not the full suite.

Independent review found a P0 — GAP-021 reverted (commit c7e1575)

A review pass over this PR found that the GAP-021 arrow-key history fix caused silent data
corruption on every platform
, and it was reverted.

On the word "independent": an earlier revision of this section described that pass as an
independent review by a reviewer with no authoring context. A reader cannot verify that from
outside this PR -- every commit here carries the same bot identity and co-author trailer, and
there is no separate artifact (no second PR, no external review comment, no linked session)
establishing a genuinely separate context. The claim is withdrawn. What follows stands on its
own evidence -- the prompt_toolkit mechanism, the measured before/after -- not on who found it.

Root cause. The fix made the enter key binding an async def so it could await
Buffer.load_history_if_not_yet_loaded(). But prompt_toolkit 3.0.52's Binding.call()
(key_binding/key_bindings.py) does not await a coroutine handler inline — it wraps it in a
background task and returns immediately:

if isawaitable(result):
    async def bg_task() -> None:
        result = await awaitable
    event.app.create_background_task(bg_task())   # fire-and-forget

The key processor is then free to process the next already-queued key synchronously. Terminal input
arrives in batches — Application.run_async's read_from_input() drains the fd and calls
key_processor.feed_multiple(keys) in one pass — so any key typed in the same batch as Enter
(ordinary fast typing, paste, latency-coalesced SSH input) runs before validate_and_handle().

Measured against the pinned prompt_toolkit — type hello, Enter, then world in one batch:

sync  accept_input (pre-fix)   submitted='hello'       CORRECT
async accept_input (the fix)   submitted='helloworld'  *** CORRUPTED ***

With Up-arrow instead of world, the submitted text became the previous history entry rather
than what was typed. The wrong content reaches the model and part of the user's next message
silently disappears. No error, no warning, nothing in the transcript.

The original GAP-021 symptom was a display-only glitch in history navigation. Trading it for silent
input corruption is not a fix. GAP-021 remains open — the race is real, but must be closed by
synchronous _working_lines repopulation or by suppressing key processing while the buffer settles.
_settle_history_load is deliberately left in place as the seam for a correct fix.

accept_input is synchronous again, with a docstring recording the measured before/after so nobody
re-applies this.

Why this PR went back to draft

It had been marked ready for review before this finding. Given a data-corruption regression was
present at that moment, it is back in draft until the remaining reviews land. That is the honest
state, not a process formality.

Other review findings on this PR

  • detect_provider_from_env() raising is correctly wired todayauto_init_from_env catches
    CredentialedProviderModuleMissingError before its generic handler, and no other caller reaches
    it directly. Flagged as a structural risk for future direct callers, not a present bug.
  • _scoped_sigint_handler verified correct on every exit path, including sys.exit(130)
    unwinding through the finally. The reviewer notes the guard is speculative hardening — no caller
    has been shown to reach it off-thread, and three embedders (amplifier-chat, amplifier-voice,
    amplifier-app-nanoclaw) remain unexamined.
  • _bounded_confirm root cause independently confirmed against rich's source (prompt.py's
    bare while True:). Correction: this section previously said no test drove 3 invalid
    responses. That was stale when written and is false against HEAD --
    tests/test_gap020_bounded_confirm.py::test_invalid_input_terminates_after_max_attempts
    (commit 2f3fed6) sends "banana" three times and asserts exactly 3 calls are consumed
    and the result is False.
  • The 15 test repairs hold up, with one exception: test_subprocess_param_routes_to_subprocess
    now asserts less than it did. The exact-equality assertion was relaxed because an agents key
    appeared — which the reviewer traced to an unstubbed MagicMock coordinator.config being
    truthy, i.e. a mocking artifact rather than real behaviour. Setting coordinator.config = {} in
    the fixture would have kept the stronger assertion.
  • Unverified: whether _run_startup_update_check()'s asyncio.new_event_loop() +
    run_until_complete() could be invoked from a thread with a loop already running. No current
    caller does; not disproven.

Native Windows verification — measured, not predicted

The earlier commits landed with the Windows claim explicitly marked unverified (the box was
offline). It is now verified on ALIENWARE-R13, Windows NT 10.0.26200, A/B against main on the
same machine in the same run.

State Windows result
main 3 collection errors, pytest aborts in 5s — nothing downstream ever runs
branch, POSIX guards only collection succeeds, then hangs at ~39%timeout 400 returns 124
branch, current HEAD completes in 66.86s — 1262 passed, 13 failed, 4 skipped

The POSIX guards worked, and immediately exposed something worse

test_ctrlc_functional_integration.py, test_dedicated_tty_input.py and
test_terminal_echo_integration.py import pty/termios/fcntl at module scope, so on Windows
they failed at collection — a hard ERROR indistinguishable from real breakage. Guarded with
pytest.skip(..., allow_module_level=True) placed before the imports, since a pytestmark is
evaluated only after the module body has already run.

That let collection succeed for the first time — and revealed that the suite then hangs. Traced
via a -v run to TestInteractiveChatClosesDedicatedTtyOnTeardown::test_close_dedicated_tty_input_called_on_normal_exit,
which printed as started and never reported a result.

It is a cross-test interaction, not a defect in that test — the same file passes in isolation on
the same box (exit 0, 4 passed). Something earlier in the suite leaves the process in a state where
interactive_chat's teardown path blocks on Windows.

The guards did not create that hang; they made it reachable. But the honest consequence is that
they turned a fast, loud 5s error into a silent 400s+ hang, which is strictly worse for CI. So it is
contained in fdda67f, scoped to Windows only, with the underlying block left explicitly
unfixed and tracked separately. Linux still runs those tests: 4 passed.

What the completing suite now shows

13 failures, newly visible rather than newly createdmain cannot reach any of them. All 13
pass on Linux, WSL2 and macOS, so each is a genuine Windows-specific behavioural difference. They
cluster: 4 in test_resolvers.py (path handling), 3 in test_stdout_offload_gaps.py (stdout
patch/restore ordering), 2 in test_always_render_final_response.py, 2 in
test_dead_code_removal.py (source-text assertions — CRLF is the obvious first suspect), and 2
singletons. Tracked separately; not addressed here.

Cross-platform, at this HEAD

Platform Result
Linux aarch64 1291 passed, 1 skipped
WSL2 x86_64 1286 passed, 1 skipped
macOS arm64 1287 passed
Windows NT 10.0.26200 1262 passed, 13 failed, 4 skipped (was: unrunnable)

amplifier-foundation on the same Windows box: 29 failed on main, 29 failed on branch — no
regression. amplifier-module-tool-bash: unbuildable on main66 passed, 5 skipped, 0 errors.
amplifier-module-provider-anthropic: 30 errors on main → 551 passed.


CORRECTION — the "Windows hang" was misdiagnosed twice; the real fix is eb922ca

Two earlier diagnoses in this PR body are wrong. Leaving them uncorrected would send a reviewer
down the wrong path, so here is what it actually is.

Wrong #1: "hangs at ~39%". Wrong #2: "a cross-test interaction, passes in isolation."

What it actually is: an unkillable busy loop, reproducible standalone.

On Windows with stdout not attached to a real console — piped, redirected, CI, any non-console
parent — prompt_toolkit's Win32Output raises NoConsoleScreenBufferError. It raises on entry
to with patch_stdout():, before await prompt_session.prompt_async(). So that REPL iteration
contains no await point at all.

The catch-all except Exception swallowed it, while True went round again, and:

  • 88% CPU, measured via WMIC PercentProcessorTime on python#1
  • an error printed every iteration, forever
  • asyncio.wait_for(..., timeout=10) never fired — the coroutine never yields, so asyncio
    physically cannot cancel or time it out. Only SIGKILL ends it.

This is a real user-facing bug, not a test artifact: any Windows user who pipes or redirects
amplifier output gets an unkillable 88%-CPU spin.

The fix (three parts)

  1. Platform-guarded _TERMINAL_UNUSABLE_ERRORS. prompt_toolkit.output.win32 asserts
    sys.platform == "win32" at import, so it stays guarded. On POSIX the tuple is empty and
    except () catches nothing — that path is byte-identical in effect to what shipped.
  2. An explicit handler before the catch-all that breaks, naming the cause and the workaround.
  3. An up-front check before the initial-prompt turn, so both call sites give one clear error.

The try: was hoisted above the check and the initial-prompt block so the existing finally:
covers them — an early return inside a try still runs the finally, so bailing out still awaits
initialized.cleanup() and closes the tty fd. My first attempt leaked both; my second double-fired
close_dedicated_tty_input(). The teardown tests caught both.

Evidence — ALIENWARE-R13, Windows NT 10.0.26200

repro script     BEFORE: wait_for(10s) never fired; python#1 at 88% CPU
                 AFTER:  interactive_chat RETURNED normally
                         console.print calls in 0.8s: 8  (10/sec)  VERDICT: not a spin

teardown file    BEFORE: never completed
                 AFTER:  4 passed in 0.08s

full suite       BEFORE: hangs at ~39%; timeout 400 -> exit 124
                 AFTER:  13 failed, 1266 passed, 3 skipped in 29.75s

The fdda67f containment skip is removed — that file now runs unguarded on Windows.

POSIX unaffected: 1291 passed on Linux. ruff check: clean with and without.

The 13 remaining Windows failures are separate and pre-existing — main cannot reach them at all,
so there is no baseline to regress from. Being worked now; this PR is not ready for review until
Windows is fully green.

@bkrabach
Brian Krabach (bkrabach) marked this pull request as draft August 11, 2026 19:13
@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review August 11, 2026 20:51
@bkrabach
Brian Krabach (bkrabach) marked this pull request as draft August 11, 2026 22:46
@sadlilas
Salil Das (sadlilas) force-pushed the fix/gap-003-020-023-027-021 branch from 194e613 to 30ff56b Compare August 12, 2026 22:15
@bkrabach Brian Krabach (bkrabach) changed the title fix: provider env detection, init prompt retry, SIGINT handling, history race fix: make native Windows collectable and runnable Aug 13, 2026
@sadlilas
Salil Das (sadlilas) force-pushed the fix/gap-003-020-023-027-021 branch from 8848597 to d80dfaf Compare August 17, 2026 20:28
Salil Das (sadlilas) added a commit that referenced this pull request Aug 18, 2026
…ted from #259) (#266)

* test: repair all 15 pre-existing test failures

All 15 failures are stale test fixtures — unrelated to the Windows fixes on this branch.
Verified identical failures on `origin/main` via `git stash` baseline A/B.
No product bugs discovered; all causes are fixture-maintenance issues:

- 11: stale mock target (commit 5b8e995 renamed process_runtime_mentions)
- 2: stale prompt assertions (fork-skill load_skill form changed)
- 1: unanswered provider-add credential-collision prompt
- 1: stale session config dict assertion (agents key now present)

Before: 15 failed, 1271 passed
After: 1286 passed, 0 failed

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* fix: correct stale resolution-order docstring in FoundationSettingsResolver

The class docstring claimed a 6-layer resolution strategy copied from
StandardModuleSourceResolver's comment, with step 4 listed as a removed
legacy module pattern and step 5 skipped entirely, jumping to step 6
(installed package). The actual code here only ever walks 5 steps, and
step 4 is the source hint pulled from bundle config -- not a legacy
pattern at all. Fixed the docstring to describe what the resolver
actually does instead of a stale description carried over from a
different resolver's comment.

* test: restore exact-equality assertion in subprocess routing test

6885b2c weakened test_subprocess_param_routes_to_subprocess from an
exact dict-equality assertion to two looser ones, citing an "agents"
key that "now" appeared in the forwarded config. That was a mocking
artifact: the fixture's coordinator.config was correctly stubbed as a
plain dict even before that commit, but the default fixture value
carries no "agents" key, so spawn_sub_session's issue #233
live-registry propagation has nothing to add here. Verified empirically
on this branch: with the exact-equality assertion restored, the full
test_session_spawner_subprocess.py suite (including the sibling test
that exercises a populated live registry) still passes.

Restores assert call_kwargs.kwargs["config"] == {"session": {}} and
documents why exact-equality is the correct, intentional assertion for
this specific (empty-registry) parent fixture.

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

---------

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Co-authored-by: sadlilas <11658960+sadlilas@users.noreply.github.com>
@sadlilas
Salil Das (sadlilas) force-pushed the fix/gap-003-020-023-027-021 branch from 02d8646 to edb96f0 Compare August 18, 2026 01:32
Salil Das (sadlilas) added a commit that referenced this pull request Aug 18, 2026
…uard (#265)

* fix: interruptible bundle-prep/update-check with main-thread signal guard

Extracted from PR #259 (fix/gap-003-020-023-027-021), which bundled this
story with four unrelated fixes across 5 files. This isolates just the
SIGINT/interruptibility work (GAP-023, GAP-027) together with its direct
dependent fix, so a reviewer can hold the whole narrative in their head at
once: "make bundle-prep and update-check interruptible, safely, from any
thread."

**GAP-023: Ctrl+C during the startup update check killed the whole
command.** The "Checking for updates..." phase installed no SIGINT handler,
so Click's default Aborted! handler killed the entire `amplifier run`
invocation over an optional, best-effort check. Added
_run_startup_update_check(), which runs the check on its own event loop
with a scoped SIGINT handler: Ctrl+C now cancels only that task and prints
"Update check skipped (Ctrl+C) -- continuing...", then proceeds to the
user's actual command.

**GAP-027: resolve_config() (bundle discovery/clone/compose/activate) ran
before any SIGINT-aware phase, with no handler of its own.** An interrupt
here either hung silently for 60+ seconds or surfaced as a bare
KeyboardInterrupt traceback landing wherever timing put it (observed inside
pydantic's plugin loader in one run, nowhere near git). Added
_resolve_config_interruptibly(), which installs a scoped handler around
resolve_config(), prints "Cancelling bundle preparation..." on interrupt,
delivers the real signal via signal.default_int_handler so existing
unwind/cleanup still runs, and converts the resulting KeyboardInterrupt into
one clean message + exit(130) instead of a raw traceback.

**The defect this introduced, and its fix.** Both of the above call
`signal.signal(signal.SIGINT, ...)` directly. `signal.signal()` is only
callable from the main thread of the main interpreter -- on every platform,
this is CPython, not a Windows quirk -- so any caller that reaches these
phases off the main thread (an embedder driving this code from a worker
thread, an HTTP handler, a channel listener) would get a hard
`ValueError: signal only works in main thread of the main interpreter`
instead of the working, no-handler behavior these phases had before GAP-023
and GAP-027 shipped. That is a real regression these two fixes introduced,
labelled "Windows" fixes but with a failure mode specific to no platform at
all.

It was caught four commits later. `_scoped_sigint_handler()` (a
context manager) now replaces all four raw `signal.signal()` call sites. It
declines to install a handler when not on the main thread (restoring the
pre-fix behavior: no handler, no crash, just no Ctrl+C acknowledgment), and
also catches ValueError for the subinterpreter case where
`threading.main_thread()` reports one main thread but `signal.signal()`
refuses anyway. `tests/test_run_sigint_main_thread_guard.py` exercises the
real guard and the real `_run_startup_update_check()` from a worker thread.

**What is and isn't known about real exposure.** An earlier review pass
found that `amplifierd` does not depend on this package at all, and
`amplifier-app-actions` imports only `console`/`session_runner`, never
`commands.run` -- so neither known embedder reaches this code off the main
thread. Three other embedders (`amplifier-chat`, `amplifier-voice`,
`amplifier-app-nanoclaw`) were never examined, and their exposure is
unknown either way. The guard is preventive: it restores a "safe to call
from any thread" contract these phases already had before GAP-023/GAP-027
narrowed it, at no cost on the main-thread path. It is not proven necessary
for a known caller, and the risk it forecloses is not disproven either --
say both, don't overclaim.

**Coverage note.** The three tests added/kept here exercise
`_scoped_sigint_handler`'s off-main-thread decline, its install/restore
behavior on the main thread, and that the real update-check phase survives
being run off the main thread. The `resolve_config()` interrupt path's
`sys.exit(130)` unwind (GAP-027) is asserted only by code inspection and the
docstring's manual-repro notes above -- there is no automated test
exercising that exit path in this change.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* fix: run git status helpers off the event loop during update check

The startup update check installs a scoped SIGINT handler that cancels the
running task on Ctrl+C. But _check_file_source() called six synchronous
subprocess.run() git helpers directly on the event loop thread, so the
cancellation could not be delivered until every one of them returned --
including _count_commits_behind(), which shells out to `git fetch` (a
network call, 5s timeout) once per local source.

The handler fired, but the interrupt was not honoured until the blocking
git work finished, which is the exact behaviour the interruptible update
check was meant to fix.

Wrap the six helpers in asyncio.to_thread() so the loop stays free and
CancelledError lands at the next await. No change to what the helpers do
or return.

---------

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Co-authored-by: sadlilas <11658960+sadlilas@users.noreply.github.com>
… errors

tests/test_ctrlc_functional_integration.py, tests/test_dedicated_tty_input.py, and
tests/test_terminal_echo_integration.py import pty, termios, and fcntl at module
scope -- POSIX-only stdlib modules with no Windows equivalent. On Windows, the
bare import raises during collection, surfacing as a hard ERROR before any test
in the file can run.

pytest.skip(..., allow_module_level=True) placed before the POSIX imports
prevents this. A pytestmark guard is not sufficient here: pytest evaluates it
only after the module body (including the imports) has already executed.

Verified on POSIX (Linux, macOS): no change in behavior, same pass counts.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
On Windows, when stdout is not attached to a real console (piped, redirected,
CI-bound, or run under a non-console parent process), prompt_toolkit's
Win32Output raises NoConsoleScreenBufferError. That error can surface from two
call sites in interactive_chat(): building the PromptSession itself
(_create_prompt_session, the FIRST place an interactive session touches the
terminal), and again every turn inside the REPL loop's patch_stdout() block.

Left unguarded, the second site is worse than a crash: the exception raises on
__enter__ to patch_stdout(), before any await point in that loop iteration is
reached, so an unqualified 'catch and keep looping' handler spins in a busy
loop -- measured at 88% CPU on native Windows, uninterruptible by
asyncio.wait_for(), only stoppable with SIGKILL.

Fix:
- A platform-guarded _TERMINAL_UNUSABLE_ERRORS tuple (empty on POSIX, so
  'except ()' catches nothing there -- the POSIX path is unchanged).
- A dedicated exception handler ahead of the REPL loop's catch-all, which
  breaks out with an actionable message instead of spinning.
- The same guard at the _create_prompt_session call site, since that is
  where an unusable terminal actually surfaces first in the real
  interactive path (unit tests mock this call, which is why the gap wasn't
  caught earlier).
- One shared _report_terminal_unusable() helper so the message can't drift
  between the two sites.
- try/finally hoisted so an early return from either guard still awaits
  initialized.cleanup() and closes the dedicated tty fd.

Verified on native Windows (piped stdout): before, a raw prompt_toolkit
traceback or an unkillable busy spin; after, a clean actionable message and
exit 0. POSIX suite unaffected.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…st defects

ONE PRODUCT FIX
  amplifier_app_cli/lib/bundle_loader/resolvers.py — _parse_source() did not
  recognize Windows absolute paths (C:\, C:/, \\server\share). A user's local
  path override in settings.yaml was silently treated as a PyPI package name,
  resulting in confusing errors about packages nobody mentioned. Now matches
  all three absolute forms Windows uses. POSIX unaffected — / and . already
  short-circuit, and no legitimate package name contains a backslash or an
  X: drive prefix.

FIVE TEST DEFECTS
  tests/test_dead_code_removal.py (2) — bare read_text() calls without
  encoding= defaulted to the locale codec (cp1252) on Windows, dying on
  non-cp1252 bytes in main.py before assertions ran. Every other read_text()
  call already passed encoding='utf-8'; these two were missed.

  tests/test_stdout_offload_gaps.py (3) and tests/test_always_render_final_response.py (2)
  — both reach patch_stdout(), which requires an app session that provides a
  platform Output. Without one, Win32Output.__init__ raises whenever stdout
  is not a real console (piped, redirected, CI, non-console parent). Both
  files now use an autouse create_app_session(output=DummyOutput()) fixture.
  Assertions unchanged — this removes an incidental dependency on the host
  terminal so the tests run identically everywhere.

  tests/lib/mention_loading/test_deduplicator.py (1) — compared an unresolved
  path against stored (resolved) paths. On POSIX the two happened to already
  match; on Windows resolve() prepends the current drive, so they diverged.
  Now resolves both sides, testing the actual contract.

  tests/test_general_config_overrides.py (1) — asserted against a hardcoded
  POSIX path string while the source does str(Path(...)), which correctly
  yields native separators. Now compares against str(Path(...)).

Takes native Windows from 'cannot collect' to a fully passing suite; POSIX
unaffected.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Add windows-latest to the 'test' job's matrix (all three OSes now covered;
fail-fast: false remains critical so a Windows failure never cancels the
POSIX legs that tell us whether we regressed the population that already
works).

Deliberately do NOT add windows-latest to the 'integration' job: every test
it selects (-m integration) forks a real child process and, in most files,
allocates a real pty pair via the POSIX-only pty/termios stdlib modules --
there is no Windows equivalent of either mechanism. Two files already skip
at module level on win32 (see the preceding test commit); a third,
test_stdout_offload_freeze_integration.py, calls os.fork() directly with no
guard at all and fails with AttributeError: module 'os' has no attribute
'fork'. A Windows leg of this job would therefore either run zero tests (all
skipped) or hard-fail on the one unguarded file -- CI theatre either way,
burning runner minutes for a signal that says nothing about real Windows
support. The main test job's Windows leg is the meaningful signal; the
integration job stays POSIX-only until a genuinely cross-platform
integration test exists.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
TestPromptSessionWiring constructs a real PromptSession through the real
_create_prompt_session factory. Under pytest on Windows, stdout is a
captured pipe rather than a real console, so prompt_toolkit selects its
Win32 output backend, whose GetConsoleScreenBufferInfo call raises
NoConsoleScreenBufferError (prompt_toolkit/output/win32.py:219). All 11
tests in the class fail at construction.

Bind a DummyOutput to the ambient AppSession for the duration of the
factory call. Application.__init__ resolves output as
`output or session.output`, so the Windows console API is never probed.
The real factory and the real PromptSession are still exercised; only
the output backend is substituted. POSIX behaviour is unchanged.
The previous commit bound the DummyOutput inside the _prompt_session
helper, which only covers the seven tests that use it. Four tests in the
class call _create_prompt_session directly -- those still selected the
Win32 output backend and still failed with NoConsoleScreenBufferError on
windows-latest.

Move the binding to an autouse fixture on TestPromptSessionWiring so it
covers every test in the class regardless of how it reaches the factory,
and cannot be bypassed by a future test that calls the factory directly.
The helper goes back to a plain factory call -- the fixture is the single
home for this behaviour.
@sadlilas
Salil Das (sadlilas) force-pushed the fix/gap-003-020-023-027-021 branch from edb96f0 to 68c953b Compare August 18, 2026 01:55
@sadlilas
Salil Das (sadlilas) marked this pull request as ready for review August 18, 2026 01:59
@sadlilas
Salil Das (sadlilas) merged commit 6692c8b into main Aug 18, 2026
9 checks passed
@sadlilas
Salil Das (sadlilas) deleted the fix/gap-003-020-023-027-021 branch August 18, 2026 03:17
Salil Das (sadlilas) added a commit that referenced this pull request Aug 18, 2026
…aled provider's module is missing (#263)

* fix: GAP-003 - refuse to silently fall back to Ollama when a credentialed provider's module is missing

Cross-platform behaviour change (not Windows-specific). Affects every
Linux/macOS/WSL/Windows user of `detect_provider_from_env()` /
`auto_init_from_env()` (the non-interactive auto-configure path used
when stdin is not a TTY: CI, Docker, shadow environments).

Old behaviour: `detect_provider_from_env()` treated "this provider's
module isn't installed" identically to "no credentials set for this
provider" -- both cases just `continue`d past the provider in the
priority loop. A user with a valid `ANTHROPIC_API_KEY` set, but whose
`provider-anthropic` module was not installed (or failed to install),
silently fell through to the credential-free Ollama fallback. That
choice got persisted to settings.yaml, so it wasn't even a one-time
mistake -- every subsequent run kept using Ollama, with no error and
no mention that a real API key was ever seen and discarded. The
symptom the user actually saw was a `ConnectionError` against a local
Ollama server they never set up, which is a much harder thing to
debug than "you're missing a package."

New behaviour: if a provider has all of its required credential env
vars present but its module is not installed/importable, that is
recorded and blocks the Ollama fallback. If no other candidate
provider is both credentialed and installed, `detect_provider_from_env()`
raises `CredentialedProviderModuleMissingError` naming the provider,
the env vars that were found, and the fix (`amplifier provider install
<name>`). `auto_init_from_env()` catches this specifically and prints
a loud, specific error instead of quietly "succeeding" onto the wrong
backend. Nothing is persisted, so the next run gets a real second
chance once the module is installed.

Unaffected: the genuine no-cloud-credentials case still lands on
Ollama quietly, exactly as before (covered by
`test_no_credentials_falls_through_to_ollama`). Unaffected: a
higher-priority provider with a missing module no longer blocks a
lower-priority provider that IS both credentialed and installed --
that one is still selected (`test_falls_through_to_second_credentialed_installed_provider`).

Why fail loud instead of silently substituting a different provider:
the user made an explicit choice by setting a specific provider's
credentials. Silently overriding that choice with Ollama is a
correctness bug dressed up as graceful degradation -- it changes which
backend runs, which model answers, and (for anyone who assumed their
cloud key was in effect) can send prompts to the wrong place entirely.
An explicit, actionable error that names the exact fix is strictly
better than a misleading downstream connection failure.

Test evidence: 7 new tests in tests/test_provider_env_detect.py
exercise detect_provider_from_env() directly (entry_points patched,
not the whole function mocked away) across every branch: no
credentials/no installed providers -> None; no credentials + Ollama
installed -> quiet Ollama fallback (unaffected case, regression
guard); credentials + module installed -> that provider; credentials
+ module missing (with and without Ollama available) -> raises;
higher-priority module missing but lower-priority both credentialed
and installed -> lower-priority one still selected; decisive
regression guard asserting the function must never return
"provider-ollama" once a credentialed-but-missing provider was seen.

Reverting just the detect_provider_from_env() logic back to the old
"treat missing module same as missing credentials" behavior (keeping
the exception class defined so imports still resolve) makes exactly
the 3 tests targeting the new behavior fail with "DID NOT RAISE" /
"provider-ollama" == "provider-ollama", confirming they exercise the
new code path and not just the pre-existing one.

Full suite: 1308 passed, 1 skipped, 13 deselected, 1 xfailed (this
branch only carries GAP-003 + tests, so the count is smaller than
main's ~1316 -- expected). ruff clean on all three changed files.

Extracted from #259, which bundles this
GAP-003 fix together with four unrelated fixes (GAP-020/023/027/021)
across 15 commits and 19 files under a "Windows compatibility gaps"
label. This change is not Windows-gated and needs review on its own
terms as a default-behaviour change for every platform.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* fix(provider-detect): exempt ambient credentials, verify module resolves, surface install failure reason

- Split provider credential table into 'chosen' vs 'ambient' credentials.
  GitHub Actions injects GITHUB_TOKEN into every job automatically —
  a platform-provided token is not a user decision. Missing the copilot
  module should not block the Ollama fallback, so ambient credentials
  from missing modules are now skipped silently.

- Replaced entry-point-name check with is_provider_module_installed()
  so stranded .dist-info (entry registered but module files missing —
  common with editable installs) is correctly treated as not installed
  rather than selected and then failing at import time.

- Added optional failures_out parameter to install_known_providers()
  to make install-failure reasons recoverable. Auto-init runs install
  with verbose=False/console=None, so real causes (network, bad source
  override, broken build) previously went only to log and were discarded.
  The GAP-003 error path now appends actual failure reason when it
  matches the provider in question. Existing callers unaffected.

- Added regression tests: autouse fixture to keep mocked entry-point
  list current with new resolution check; stranded-entry-point test;
  TestAmbientCredentialsDoNotBlockFallback covering GITHUB_TOKEN-alone
  fallback, None return when nothing installed, Copilot selectable when
  installed, and ANTHROPIC_API_KEY still raising alongside ambient
  GITHUB_TOKEN (proving carve-out did not silently undo GAP-003).

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

---------

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Co-authored-by: sadlilas <11658960+sadlilas@users.noreply.github.com>
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.

3 participants