Skip to content

fix(tests): skip install.sh safety tests when POSIX bash is unavailable#1719

Merged
danielmeppiel merged 1 commit into
mainfrom
danielmeppiel/fix-windows-ci-tests
Jun 9, 2026
Merged

fix(tests): skip install.sh safety tests when POSIX bash is unavailable#1719
danielmeppiel merged 1 commit into
mainfrom
danielmeppiel/fix-windows-ci-tests

Conversation

@danielmeppiel

Copy link
Copy Markdown
Collaborator

TL;DR

The Build & Test (windows-latest) CI job was failing with 30 test failures in tests/unit/install/test_install_safety.py, all with a uniform assert 1 == <expected>. These tests shell out to bash to exercise the Unix installer's safety validator; on windows-latest runners the ambient bash is the WSL stub and exits 1 for every call. This PR skips the bash-dependent tests where no usable POSIX bash exists.

Problem (WHY)

tests/unit/install/test_install_safety.py sources the apm_lib_dir_validate() block out of install.sh and runs it under bash to assert exit codes (0, 11, 12, 14). On windows-latest GitHub runners, the bash on PATH resolves to the WSL stub (C:\Windows\System32\bash.exe). With no Linux distribution installed, that stub exits 1 for every invocation — so all 30 bash-dependent tests failed with the same assert 1 == 0/11/12/14, while the 3 pure-Python sentinel tests passed.

install.sh is the Unix installer; Windows installs via install.ps1. There is no value in running these shell-validator tests on a Windows runner that has no POSIX bash.

Linux (x64/arm64) and macOS jobs were green — the failure was Windows-only.

Approach (WHAT)

Detect whether a real POSIX bash is invocable, and skip the bash-dependent test classes when it is not — instead of letting them fail with a misleading uniform exit code. The pure-Python sentinel-invariant tests keep running on every platform.

Implementation (HOW)

In tests/unit/install/test_install_safety.py:

  • Added _usable_bash(): probes bash -c "printf ok" and returns true only when it exits 0 and prints ok. Any OSError / SubprocessError / non-zero exit (the WSL-stub case) yields False.
  • Added a module-level requires_bash = pytest.mark.skipif(...) and applied it to the six bash-dependent classes (TestAbsolutePathGuard, TestSuffixGuard, TestBlocklistGuard, TestMarkerFileGuard, TestUserLocalInstall, TestReportedIncident). TestSentinelInvariants is pure-Python and stays unmarked.
  • Set MSYS_NO_PATHCONV=1 / MSYS2_ARG_CONV_EXCL=* on the subprocess env so a real Git Bash (MSYS2) runner does not rewrite POSIX path arguments like /usr/local/lib/apm into Windows paths.

Trade-offs

  • On a Windows runner with no POSIX bash, these validator tests are skipped rather than executed. That is correct: the code under test (install.sh) never runs on Windows, and the existing CRLF-normalization plus sentinel tests still guard the script's testability invariants. Linux and macOS continue to execute the full validator matrix.

Validation evidence

  • macOS (bash usable): 43 passed — unchanged from baseline.
  • Simulated WSL stub (a fake bash on PATH that exits 1): 3 passed, 40 skipped — the bash-dependent tests skip cleanly and the pure-Python sentinel tests still run.
  • ruff check and ruff format --check on the changed file: clean.

How to test

# Normal (POSIX bash present): all tests run
uv run --extra dev pytest tests/unit/install/test_install_safety.py -q

# Simulate the windows-latest WSL stub locally:
mkdir -p /tmp/fakebash && printf '#!/bin/sh\nexit 1\n' > /tmp/fakebash/bash
chmod +x /tmp/fakebash/bash
PATH="/tmp/fakebash:$PATH" uv run --extra dev pytest tests/unit/install/test_install_safety.py -q
# expect: 3 passed, 40 skipped
rm -rf /tmp/fakebash

Failing run for reference: https://github.com/microsoft/apm/actions/runs/27216959181/job/80361090943

On windows-latest GitHub runners the ambient bash on PATH is the WSL
stub (C:\Windows\System32\bash.exe), which exits 1 for every
invocation when no distribution is installed. That made all 30
bash-dependent install-safety tests fail with a uniform exit code of 1
(assert 1 == 0/11/12/14) even though install.sh is the Unix installer
(Windows uses install.ps1).

Probe for a usable POSIX bash and skip the bash-dependent test classes
when none exists, keeping the pure-Python sentinel tests running. Also
set MSYS_NO_PATHCONV/MSYS2_ARG_CONV_EXCL so a real Git Bash does not
mangle POSIX path arguments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 9, 2026 18:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves cross-platform reliability of the Unix-installer safety validator unit tests by skipping bash-dependent test classes when a usable POSIX bash is not available (notably on windows-latest, where bash may resolve to the WSL stub and fail uniformly).

Changes:

  • Add a _usable_bash() probe and module-level requires_bash skip marker for bash-dependent test classes.
  • Ensure MSYS2/Git Bash does not rewrite POSIX-looking path arguments by setting MSYS_NO_PATHCONV / MSYS2_ARG_CONV_EXCL in the subprocess environment.
Show a summary per file
File Description
tests/unit/install/test_install_safety.py Adds a bash usability probe + skip marker for bash-driven tests and hardens subprocess env against MSYS path conversion.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +50 to +55
proc = subprocess.run(
["bash", "-c", "printf ok"],
capture_output=True,
text=True,
timeout=10,
)
@danielmeppiel danielmeppiel merged commit c5bb1db into main Jun 9, 2026
21 checks passed
@danielmeppiel danielmeppiel deleted the danielmeppiel/fix-windows-ci-tests branch June 9, 2026 19:31
danielmeppiel added a commit that referenced this pull request Jun 9, 2026
Merge origin/main (PRs #1714, #1717, #1718, #1719) into the release
branch. Only #1718 is user-facing (src/apm_cli/install drift +
target-complete lockfile manifest); its entry was authored on main and
now sits in the [0.19.0] Fixed block with the PR number appended.
#1714, #1717, #1719 are maintainer-toolkit / test-only and dropped per
the entry-sanitizer DROP list. Lint mirror green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Jun 9, 2026
* chore: release v0.19.0

Bump pyproject.toml + uv.lock to 0.19.0 and move the [Unreleased]
CHANGELOG block to [0.19.0] - 2026-06-09 with one 'so what' entry per
merged PR. Lint mirror green locally (ruff check + format, pylint
R0801, auth-signals).

Post-merge: tag v0.19.0 to trigger the release workflow.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* chore: fold #1718 into v0.19.0 changelog after main merge

Merge origin/main (PRs #1714, #1717, #1718, #1719) into the release
branch. Only #1718 is user-facing (src/apm_cli/install drift +
target-complete lockfile manifest); its entry was authored on main and
now sits in the [0.19.0] Fixed block with the PR number appended.
#1714, #1717, #1719 are maintainer-toolkit / test-only and dropped per
the entry-sanitizer DROP list. Lint mirror green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(self-check): reconcile stale apm-review-panel SKILL.md hash in lockfile

The APM Self-Check (apm audit --ci) content-integrity gate failed:
apm.lock.yaml recorded a stale deployed_file_hashes entry
(fe6a73...) for .agents/skills/apm-review-panel/SKILL.md while the
deployed file and its packages/** source are both fc63e25... #1714
updated the skill content but deliberately left apm.lock.yaml
untouched; #1718 then turned the drift gate live (dropped --no-drift),
exposing the mismatch. apm install reconciles the recorded hash (and
refreshes apm_version to 0.19.0). All 9 audit checks now pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam pushed a commit that referenced this pull request Jun 9, 2026
…e integrity)

Sync with main through v0.19.0 (#1715, #1714, #1718, #1719, #1717).

Conflicts resolved:
- CHANGELOG.md: main cut the v0.19.0 release, moving prior Unreleased
  entries into a dated section. Took main's restructure; relocated the
  #1681 "Tightened Stage 2 code-complexity thresholds" entry to the new
  ## [Unreleased] section (released sections stay immutable per Keep a
  Changelog). Took main's #1702/#1710 close-ref for the lockfile entry.
- src/apm_cli/install/services.py: #1718 added per-file skill-bundle
  integrity recording inline at a site my refactor had extracted into
  _log_skill_result. Kept the new _skill_bundle_file_entries helper in
  services.py (sibling of _deployed_path_entry); dropped main's duplicate
  inline _log_hook_display_payloads (already relocated to
  services_integrate.py and re-exported in my #1700 port); accepted the
  _log_skill_result delegator at the call site.
- src/apm_cli/install/services_integrate.py: ported #1718's
  deployed.extend(_skill_bundle_file_entries(tp, ...)) into
  _log_skill_result's deployed-path loop (deployed aliases
  result["deployed_files"]), with a lazy import of the helper.

Shadow gate: no markers, all src <=800 lines, ruff check + format clean,
pylint R0801 EXIT=0, auth-signals clean; 11144 tests pass across the
install/integration suites plus the #1718, #1700, and #1709 targeted sets.

Co-authored-by: Copilot <223556219+Copilot@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.

2 participants