close
Skip to content

fix(test): skip POSIX executable-resolution ACP tests on Windows - #2073

Merged
iamwhatever merged 1 commit into
mainfrom
fix/acp-posix-only-windows-skips
Aug 7, 2026
Merged

fix(test): skip POSIX executable-resolution ACP tests on Windows#2073
iamwhatever merged 1 commit into
mainfrom
fix/acp-posix-only-windows-skips

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

5 tests in test/test_acp_client.py fail on the Windows backend lane whenever the
suite is sliced by the reduced-scope selector — which is what a frontend-only
PR gets. Observed on #2056, whose diff contains zero Python files:

Select test scope: ONLY_FRONTEND: true
                   Reduced scope: 234 cross-surface backend file(s).
...
5 failed, 2891 passed, 123 skipped

FAILED TestResolveClaudeAcpBin::test_path_lookup                       - assert None is not None
FAILED TestResolveClaudeAcpBin::test_mise_which_preferred              - assert None == ['C:\\...\\claude-agent-acp']
FAILED TestResolveClaudeAcpBin::test_mise_glob_fallback                - assert None == [...]
FAILED TestResolveClaudeAcpBin::test_mise_installed_script_resolves_node
                                          - assert ['C:\\Program...index.js'] == ['C:\\Users\\...index.js']
FAILED TestResolveClaudeCodeExecutable::test_path_lookup
                                          - assert 'D:\\home\\u\\...\\bin\\claude' == '/home/u/.toolbox/bin/claude'

Correcting an earlier version of this description: I first wrote that these
fail on main too. They do not. main pushes run ONLY_FRONTEND: false → the
full backend suite, and the Windows lane is green there (8/8 recent runs).
These 5 are also not on test/windows-expected-failures.txt (239 entries,
zero in test_acp_client.py), so the burn-down allowlist is not what is hiding
them. The divergence is real and worth stating plainly:

the same 5 tests pass under the full suite and fail under the 234-file subset,
on the same OS and the same install.

That is a test-isolation artifact, not a stable platform fact. Numerous modules
patch sys.platform / platform_compat.IS_WINDOWS (test_platform_compat,
test_kiro_prerequisite, test_cli_desktop, test_acp_client itself, others),
so which tests share an xdist worker decides whether such a patch is in effect
when these five run. I have not pinned down the exact leaking patch and am not
claiming to.

Why it still needs fixing

"Green on main" here is not evidence of Windows correctness — it is evidence
that these assertions are only satisfied when something else has already made the
process look non-Windows. Two consequences:

  1. Any PR taking the reduced-scope path — i.e. every frontend-only PR — gets a
    red Windows lane for reasons unrelated to its diff. That is how this surfaced.
  2. The Windows coverage these five appear to provide on main is illusory. In a
    genuinely Windows-shaped process they cannot pass, for the reasons below.

Root cause of the assertions themselves

Independent of the ordering effect, the fixtures encode POSIX
executable-resolution semantics with no Windows equivalent.

TestResolveClaudeAcpBin (4) create an extensionless claude-agent-acp and
make it runnable with chmod(0o755). Windows shutil.which resolves candidates
through PATHEXT, so an extensionless file is never found and the resolver
correctly returns None; chmod executable bits are also near-meaningless there.
test_mise_installed_script_resolves_node fails a variant — it resolves the real
system node under C:\Program Files instead of the fixture's.

TestResolveClaudeCodeExecutable::test_path_lookup stubs shutil.which to
return /home/u/.toolbox/bin/claude and asserts the resolver echoes it back. The
resolver ends in _normalize_exe_casing(), which calls os.path.realpath() when
platform_compat.IS_WINDOWS — and realpath anchors a drive-relative /... path
to the current drive, producing D:\home\u\.toolbox\bin\claude.

That normalizer is not a bug and should not be "fixed": it exists so a
PATHEXT-derived ...\kiro-cli.EXE reaches a case-sensitive launcher shim under
its true on-disk .exe name. Spawned with the wrong casing, such a launcher can
fail to dispatch and break the ACP pipe.

Fix

A _POSIX_EXEC_PATHS_ONLY skipif on those 5 methods — making explicit the
POSIX-only status they currently hold by accident.

Per-method, not per-class. The same two classes also hold
test_env_override_wins, test_env_override_ignored_when_missing and
test_mise_preferred_over_path, which are Windows-safe by construction (absolute
override paths, fully mocked which()). Marking the classes would trade 5
failures for 3 lost assertions.

A separate marker, not the existing _POSIX_ONLY. That one's stated reason is
"POSIX process tree APIs only" (os.killpg, ps, /proc) — accurate for what
#2044 guarded, wrong for these.

Relationship to #2044

#2044 guarded the process-tree half of this same file (12 tests), motivated by
fork runners. These 5 fail for an unrelated reason and were outside its scope.
Together the two cover the file.

Tests

No new tests — this changes only when existing tests run. Verified:

  • On Linux all five still run and pass: 473 passed, 0 skipped for the whole
    file, so the skip is Windows-only and no Linux coverage is lost.
  • isort --check-only and flake8 clean on the changed file.
  • Same sys.platform == "win32" construct as the existing _POSIX_ONLY, already
    demonstrated to work: a branch picking up fix(test): skip POSIX-only ACP tests on Windows fork runners #2044 saw its 12 failures become skips.

Manual verification

Measured on #2056's Windows lane across three states of that branch:

branch state Windows failures
before #2044 was in the base 17
rebased onto main with #2044 5
with this change 0 (all 4 shards green)

Follow-up worth filing separately

The ordering sensitivity is the deeper issue and this PR does not fix it: a test
whose outcome depends on which xdist worker it lands on is a latent flake even
where it currently passes. The durable fix is for the modules patching
sys.platform / IS_WINDOWS to guarantee restoration, or for these assertions to
be platform-parameterised rather than hardcoded POSIX strings. Happy to file that
separately if maintainers want it tracked.

Five tests in test_acp_client.py fail on every Windows CI run because their
fixtures encode POSIX executable-resolution semantics that have no Windows
equivalent:

  - TestResolveClaudeAcpBin builds an extensionless `claude-agent-acp` and makes
    it runnable with chmod(0o755). On Windows shutil.which resolves candidates
    through PATHEXT, so an extensionless file is never found and the resolver
    correctly returns None. test_mise_installed_script_resolves_node fails a
    variant: it resolves the real system node under Program Files rather than
    the fixture's.
  - TestResolveClaudeCodeExecutable::test_path_lookup stubs shutil.which to
    return "/home/u/.toolbox/bin/claude" and asserts the resolver echoes it. The
    resolver ends in _normalize_exe_casing, which calls os.path.realpath on
    Windows; realpath anchors a drive-relative "/..." path to the current drive,
    yielding "D:\home\u\.toolbox\bin\claude".

The production resolvers behave correctly on Windows in all five cases —
_normalize_exe_casing exists precisely so a PATHEXT-derived ".EXE" reaches a
case-sensitive launcher shim under its true on-disk name. Only the fixtures are
POSIX-shaped, so these are test bugs rather than a Windows defect.

Marked per-method rather than per-class: the same two classes also hold
test_env_override_wins, test_env_override_ignored_when_missing and
test_mise_preferred_over_path, which pass on Windows today because they use
absolute override paths and a fully mocked which(). Marking the classes would
trade five failures for three silently lost assertions.

The marker is separate from the existing _POSIX_ONLY because that one's stated
reason is process-tree APIs (os.killpg, ps, /proc), which is not why these fail.

Verified on Linux: all five still run and pass (473 passed, 0 skipped in the
file), so the skip is Windows-only and no coverage is lost.
@kyleseaman
kyleseaman requested a review from a team as a code owner August 7, 2026 19:32
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Opus 5 Review — ✅ no blocking findings

Reviewed 3a4bdb8014cb443feebc09aa774c1298cb694db5 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 3a4bdb8

Verdict parsed from the review's SHA-scoped output markers for commit 3a4bdb8014cb443feebc09aa774c1298cb694db5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3a4bdb8014cb443feebc09aa774c1298cb694db5: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 3a4bdb8014cb443feebc09aa774c1298cb694db5 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Test-only skip of POSIX-shaped fixtures, correctly scoped per-method with a distinct, accurate marker; alternatives were weighed and the tradeoff is documented.

Suggestions

  • The Windows resolution path (PATHEXT lookup, _normalize_exe_casing) now has zero test coverage on the platform where it does the most work; a follow-up adding Windows-shaped fixtures (.cmd/.exe in tmp_path) would close that gap rather than leaving it skip-only.

[DESIGN-REVIEWED] 3a4bdb8

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3a4bdb8014cb443feebc09aa774c1298cb694db5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3a4bdb8

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3a4bdb8014cb443feebc09aa774c1298cb694db5: <one-sentence reason>

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision labels Aug 7, 2026
@iamwhatever
iamwhatever merged commit e97b626 into main Aug 7, 2026
54 checks passed
@iamwhatever
iamwhatever deleted the fix/acp-posix-only-windows-skips branch August 7, 2026 21:20
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 7, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…odotdev#2073)

Five tests in test_acp_client.py fail on every Windows CI run because their
fixtures encode POSIX executable-resolution semantics that have no Windows
equivalent:

  - TestResolveClaudeAcpBin builds an extensionless `claude-agent-acp` and makes
    it runnable with chmod(0o755). On Windows shutil.which resolves candidates
    through PATHEXT, so an extensionless file is never found and the resolver
    correctly returns None. test_mise_installed_script_resolves_node fails a
    variant: it resolves the real system node under Program Files rather than
    the fixture's.
  - TestResolveClaudeCodeExecutable::test_path_lookup stubs shutil.which to
    return "/home/u/.toolbox/bin/claude" and asserts the resolver echoes it. The
    resolver ends in _normalize_exe_casing, which calls os.path.realpath on
    Windows; realpath anchors a drive-relative "/..." path to the current drive,
    yielding "D:\home\u\.toolbox\bin\claude".

The production resolvers behave correctly on Windows in all five cases —
_normalize_exe_casing exists precisely so a PATHEXT-derived ".EXE" reaches a
case-sensitive launcher shim under its true on-disk name. Only the fixtures are
POSIX-shaped, so these are test bugs rather than a Windows defect.

Marked per-method rather than per-class: the same two classes also hold
test_env_override_wins, test_env_override_ignored_when_missing and
test_mise_preferred_over_path, which pass on Windows today because they use
absolute override paths and a fully mocked which(). Marking the classes would
trade five failures for three silently lost assertions.

The marker is separate from the existing _POSIX_ONLY because that one's stated
reason is process-tree APIs (os.killpg, ps, /proc), which is not why these fail.

Verified on Linux: all five still run and pass (473 passed, 0 skipped in the
file), so the skip is Windows-only and no coverage is lost.

Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.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.

2 participants