fix: Windows CI CRLF baseline and WebSocket shutdown thread-join (closes #2233)#2237
fix: Windows CI CRLF baseline and WebSocket shutdown thread-join (closes #2233)#2237danielmeppiel wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR targets two Windows CI reliability issues: ensuring ratchet baseline JSON is written with stable LF line endings on Windows, and making the WebSocket test server fixture shut down cleanly by joining its background server thread.
Changes:
- Update
scripts/ratchet_baseline.py::write_baseline()to write canonical JSON as UTF-8 bytes (avoids Windows text-mode newline translation). - Add a new unit test that simulates Windows newline translation to lock in LF-only baseline output.
- Join the
serve_foreverthread during WebSocket test server teardown and add a regression test asserting the thread terminates.
Show a summary per file
| File | Description |
|---|---|
| scripts/ratchet_baseline.py | Switch baseline writer from write_text to write_bytes to enforce platform-independent LF output. |
| tests/unit/scripts/test_ratchet_baseline.py | New regression test to prove baseline writing stays LF-only even when write_text would translate \n -> \r\n. |
| tests/unit/integration/test_copilot_app_ws.py | Join server thread on shutdown and add a test ensuring the thread is no longer alive after context exit. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| with contextlib.suppress(Exception): | ||
| self._server.shutdown() | ||
| if self._thread is not None: | ||
| self._thread.join(timeout=1.0) |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean single-owner fix; write_bytes bypasses text-mode CRLF correctly, thread join is bounded. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output, logging, or diagnostic rendering code was touched. |
| DevX UX Expert | 0 | 0 | 0 | No CLI command surface, help text, error wording, install/init/run flow, or first-run experience is affected. |
| Supply Chain Security Expert | 0 | 0 | 0 | No dependency download, token, lockfile, registry, auth, path-traversal, or integrity surface touched. |
| OSS Growth Hacker | 0 | 0 | 0 | CI-only fix; no conversion surface touched. |
| Test Coverage Expert | 0 | 0 | 0 | Both regression tests exercise the exact bug-fix paths and mutation-break checks validated the assertions. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Merge as-is. Zero required findings across active panelists, mutation-break evidence validates both guards, and the change is scoped entirely to CI/test infrastructure with no user-facing surface risk.
Regression-trap evidence (mutation-break gate)
tests/unit/scripts/test_ratchet_baseline.py::test_write_baseline_uses_lf_bytes_when_text_mode_translates_newlines-- deletedscripts/ratchet_baseline.py write_bytes(content.encode("utf-8")); test FAILED as expected; guard restored.tests/unit/integration/test_copilot_app_ws.py::test_server_context_joins_serve_forever_thread-- deletedself._thread.join(timeout=1.0); test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both passed with no diagnostics.
CI
gh pr checks 2237 --repo microsoft/apm --watch reported all required checks passing on the latest head (CI run https://github.com/microsoft/apm/actions/runs/29460953043; sync skipped) after 0 CI fix iteration(s).
Mergeability status
Captured from gh pr view 2237 --repo microsoft/apm --json mergeable,mergeStateStatus,statusCheckRollup immediately after CI went green.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2237 | df6e741 |
ship_now | 1 | 0 | 0 | 1 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
1 outer iteration; 1 Copilot round. Final panel stance: ship_now.
Ready for maintainer review.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Canonical-owner consolidation is structurally sound; shepherd-driver standalone git resolution is a justified portability exception; WinError catch is narrowly scoped with full regression coverage. |
| CLI Logging Expert | 0 | 0 | 2 | No interactive CLI output surface touched; script diagnostics and CI workflow changes are sound. Two nits on diagnostic polish. |
| DevX UX Expert | 0 | 1 | 2 | No CLI command-surface regression; surfaces systemic bare-git-argv risk in 25+ production CLI call sites that the PR's fix pattern reveals but does not yet cover. |
| Supply Chain Security Expert | 0 | 0 | 1 | No supply-chain or subprocess security regression; git resolution, subprocess safety, CI permissions, and atomic-write delegation are clean. |
| OSS Growth Hacker | 0 | 1 | 1 | CHANGELOG omits a release-notes-ready Windows contributor-experience improvement; story angle is real but not blocking. |
| Test Coverage Expert | 0 | 0 | 0 | All 4 Windows-only production fixes have regression-trap tests that would catch a revert; CI workflow contract pinned by 7 mutation-break-proof tests; ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Oss Growth Hacker + Doc Writer] Add CHANGELOG.md [Unreleased] entries for the 4-class Windows fix and the new Windows Compatibility Gate CI job. -- Three independent personas converged on this. Repo convention (.github/instructions/changelog.instructions.md) requires an entry for every merged PR that changes code/tests. The fix is contributor-experience-relevant and the new CI gate is a structural addition to the merge contract -- both are changelog-worthy.
- [Devx Ux Expert] File a follow-up issue to migrate ~25 bare-git-argv subprocess calls in src/apm_cli/ to get_git_executable(). -- The PR fixes this bug class in tests/scripts but grep confirms the identical Windows CreateProcess failure pattern exists in production code (github_downloader.py, git_file_transport.py, commands/_helpers.py, version.py, etc.). This is a latent defect on the portability-by-manifest surface. Scoping it as a tracked issue makes the work visible and contributable.
- [Devx Ux Expert] Fix 3 production .as_posix() sites in validation.py and packer.py that emit Windows backslash paths in diagnostics. -- Same bug class this PR fixes in scripts/; low-effort follow-up that extends the cross-platform fix pattern to user-facing output.
- [Cli Logging Expert + Supply Chain Security Expert] Narrow the CI failure-diagnostics env dump from blanket Get-ChildItem Env: to targeted PATH/PYTHON*/UV_/GIT_ vars. -- Two personas independently flagged this. Risk is low (secrets masked, if:failure guarded, contents:read permissions), but a targeted dump is more useful for debugging and reduces noise.
- [Cli Logging Expert + Devx Ux Expert] Add a remediation hint to the GateError message when git is not found on PATH. -- Two personas independently flagged the missing "next action" in the error message. Low-stakes (CI infra script, not user-facing CLI) but consistent with CLI logging rule 4 ("include the fix").
Architecture
classDiagram
direction LR
class atomic_write_text {
<<CanonicalOwner>>
+atomic_write_text(path, data) None
+normalize_crlf_to_lf(data) str
+write_text_lf(path, data) None
}
class ratchet_baseline {
<<Script>>
+write_baseline(path, payload, label) None
}
class run_mutation_pilot {
<<Script>>
+_write_report(path, payload) None
}
class git_env {
<<CanonicalOwner>>
+get_git_executable() str
+git_subprocess_env() dict
+reset_git_cache() None
}
class test_shepherd_gate {
<<TestHelper>>
+_git(repo, args) str
}
class owner_touch_gate {
<<StandaloneSkill>>
+_git_executable() str
+_git(repo_root, args) str or bytes
}
class _Server {
<<TestHarness>>
+_run_serve_forever() None
+__enter__()
+__exit__()
}
class _FakeSyncServer {
<<TestDouble>>
+serve_forever() None
}
ratchet_baseline ..> atomic_write_text : delegates
run_mutation_pilot ..> atomic_write_text : delegates
test_shepherd_gate ..> git_env : delegates
_Server *-- _FakeSyncServer : test injects
note for atomic_write_text "Single canonical atomic-write primitive.\nLF normalization + os.fdopen(newline='') +\nos.replace atomic rename."
note for owner_touch_gate "Standalone portable skill artifact:\nshutil.which('git') self-contained cache.\nNo apm_cli dependency by design."
note for _Server "Adapter: wraps serve_forever() to\nabsorb win32 WinError 10038 only"
class ratchet_baseline:::touched
class run_mutation_pilot:::touched
class test_shepherd_gate:::touched
class owner_touch_gate:::touched
class _Server:::touched
class _FakeSyncServer:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
subgraph CRLF_FIX["CRLF baseline fix"]
RB["ratchet_baseline::write_baseline()"] --> AWT["[I/O] atomic_write_text(path, json_content)"]
RMP["run_mutation_pilot::_write_report()"] --> MK["[FS] path.parent.mkdir(parents=True)"]
MK --> AWT
AWT --> NORM["normalize_crlf_to_lf(data)"]
NORM --> TMP["[FS] tempfile.mkstemp(dir=path.parent)"]
TMP --> FD["[I/O] os.fdopen(fd, 'w', newline='')"]
FD --> WR["[I/O] fh.write(normalized)"]
WR --> REP["[FS] os.replace(tmp, path)"]
end
subgraph GIT_RESOLVE["Git executable resolution"]
TSG["test_shepherd_gate::_git(repo)"] --> GGE["git_env.get_git_executable()"]
GGE --> SW1["shutil.which('git') -- cached"]
TSG --> GSE["git_subprocess_env()"]
GSE --> EPE["external_process_env() minus _STRIP_GIT_VARS"]
TSG --> EXEC1["[EXEC] subprocess.run(resolved_git, args)"]
OTG["owner_touch_gate::_git(repo_root)"] --> GE2["_git_executable()"]
GE2 --> SW2["shutil.which('git') -- self-contained cache"]
GE2 -->|None| ERR["raise GateError"]
OTG --> EXEC2["[EXEC] subprocess.run(resolved_git, args)"]
end
subgraph WS_SHUTDOWN["WebSocket shutdown race fix"]
ENTER["_Server.__enter__()"] --> THR["threading.Thread(target=_run_serve_forever)"]
THR --> SF["[NET] _server.serve_forever()"]
SF --> CHK{OSError raised?}
CHK -- No --> CLEAN["Clean return"]
CHK -- Yes --> PLAT{win32 AND winerror==10038?}
PLAT -- Yes --> ABSORB["Absorb: return"]
PLAT -- No --> RERAISE["Re-raise OSError"]
EXIT["_Server.__exit__()"] --> SD["[NET] _server.shutdown()"]
SD --> JOIN["_thread.join(timeout=1.0)"]
end
Recommendation
Merge after adding the CHANGELOG entry (two lines, three independent personas converged on this). The systemic bare-git-argv follow-up should be filed as a tracked issue linked to #2233 immediately post-merge -- it is real defect-class debt on the portability surface but correctly out of scope for this PR. All other follow-ups are nit-tier polish. The PR is structurally sound, fully tested (19,253+ tests green across all CI scopes, 5x flake-free re-runs on thread-sensitive files), security-audited, and architecturally clean.
Full per-persona findings
Python Architect
- [nit] Design patterns assessment for this PR
Canonical Owner delegation (atomic_write_text), Adapter (_run_serve_forever narrow WinError absorption), Standalone Leaf isolation (owner_touch_gate.py's own shutil.which cache, justified by no-apm_cli-dependency portability constraint, documented via cross-reference comment). No changes suggested -- current shape is the simplest correct design at this scope; extracting a micro-package for a single shutil.which call would be over-engineering.
CLI Logging Expert
- [nit] GateError message for missing git could include a fix hint at
packages/shepherd-driver/scripts/owner_touch_gate.py:47
CLI logging rule 4 ('Include the fix'): current message names the problem but gives no next step. Low-stakes since this is a CI infra script, not apm's user-facing CLI.
Suggested: raise GateError("git executable not found on PATH; install git or add its directory to PATH") - [nit] Failure diagnostics step dumps all env vars -- noisy for the stated debugging goal at
.github/workflows/ci.yml:146
Get-ChildItem Env: dumps 200+ vars when only PATH/PYTHON*/UV_/GIT_/COMSPEC/SystemRoot matter. Low risk (secrets masked, if:failure() guarded) but a narrower dump is more useful.
Suggested: Replace blanket dump with targeted var echoes.
DevX UX Expert
- [recommended] Production CLI (src/apm_cli/) has ~25 bare ["git", ...] subprocess calls sharing the identical Windows CreateProcess bug this PR fixes in tests/scripts. at
src/apm_cli/deps/github_downloader.py:1094
grep across src/apm_cli/ finds the same bug class in github_downloader.py, git_file_transport.py, commands/_helpers.py, version.py, marketplace/ref_resolver.py, core/token_manager.py, commands/marketplace/doctor.py, install/validation.py, deps/transport_selection.py, install/pipeline.py, policy/discovery.py -- none use the existing get_git_executable(). These would raise FileNotFoundError: [WinError 2] on Windows wherever PATH lacks an extension-qualified git.
Suggested: File a follow-up issue linked to [BUG] Windows CI fails due to CRLF baseline output, missing ratchet baseline authority detection, and WebSocket shutdown warning #2233 to mechanically migrate all production bare-git-argv call sites to get_git_executable(). - [nit] Two production files interpolate path.relative_to() without .as_posix() -- same Windows backslash bug class this PR fixes in scripts/. at
src/apm_cli/models/validation.py:769
src/apm_cli/models/validation.py:769,773 and src/apm_cli/bundle/packer.py:296 format Windows paths with backslashes into user-facing diagnostics.
Suggested: Chain .as_posix() at these 3 sites in a follow-up. - [nit] GateError message lacks a concrete remediation step for a Windows contributor. at
packages/shepherd-driver/scripts/owner_touch_gate.py:47
DevX recovery rule: name what/why/next-action. Current message omits the fix action.
Suggested: raise GateError("git executable not found on PATH; install Git (https://git-scm.com) and ensure 'git' is available in your PATH")
Supply Chain Security Expert
- [nit] Full environment dump in CI diagnostics step is broader than necessary. at
.github/workflows/ci.yml
Get-ChildItem Env: dumps the full runner environment on failure. GitHub masks secrets/GITHUB_TOKEN and the job has minimal contents:read permissions, so risk is low, but a targeted list would narrow the surface.
Suggested: Replace with targeted PATH/PYTHON_VERSION/platform vars, or gate behind a debug flag.
Audit trail (verified, no findings beyond the nit above): (a) owner_touch_gate.py's shutil.which('git') is functionally equivalent to get_git_executable() for non-frozen distributions -- the PyInstaller LD_LIBRARY_PATH/DYLD_* guard only applies when sys.frozen, irrelevant to this standalone script, no security gap. (b) subprocess.run uses shell=False, check=False, capture_output=True, fully-trusted argv -- no shell injection vector. (c) new windows-compat-gate job declares minimal permissions (contents: read), pinned major-tag actions, no secrets referenced. (d) atomic_write_text's os.replace() has identical non-symlink-following rename semantics to the old code -- no TOCTOU or symlink regression.
OSS Growth Hacker
- [recommended] CHANGELOG.md [Unreleased] omits both the 4-class Windows fix and the new PR-time Windows CI gate. at
CHANGELOG.md:8
This PR removes a real contributor-friction class (Windows devs whose PRs pass Linux-only CI but silently break the Windows-only post-merge build) and adds a new required CI gate -- both changelog-worthy per repo convention. grep confirms no [BUG] Windows CI fails due to CRLF baseline output, missing ratchet baseline authority detection, and WebSocket shutdown warning #2233/fix: Windows CI CRLF baseline and WebSocket shutdown thread-join (closes #2233) #2237 mention in [Unreleased].
Suggested: Add Fixed: "Four classes of Windows-only CI failures (CRLF baseline leakage, backslash path diagnostics, bare-git-argv subprocess resolution, WebSocket shutdown race) no longer slip through Linux-only PR CI. (closes [BUG] Windows CI fails due to CRLF baseline output, missing ratchet baseline authority detection, and WebSocket shutdown warning #2233)" and Added: "A focused Windows Compatibility Gate now runs at PR time. ([BUG] Windows CI fails due to CRLF baseline output, missing ratchet baseline authority detection, and WebSocket shutdown warning #2233)" - [nit] Strong release-narrative angle: "we found a structural blind spot in CI and fixed it, not just the symptoms" -- worth mining for the next release beat.
Cross-platform reliability is a table-stakes signal for enterprise adopters on mixed-OS teams evaluating APM against pip/uv/cargo.
Auth Expert -- inactive
PR touches only CI workflows, build/test scripts (owner_touch_gate, ratchet_baseline, run_mutation_pilot, check_test_contract_authorities), and test files -- no auth, token, credential, or host-classification surface is affected.
Doc Writer -- inactive
PR touches only CI workflow YAML and test/script infrastructure, no docs/src/content/docs/**, README.md, MANIFESTO.md, or agent/skill/instruction prose. (Independently confirmed the missing CHANGELOG entry -- folded into the OSS Growth Hacker finding above and the Top 5 follow-ups.)
Test Coverage Expert
No findings.
Performance Expert -- inactive
PR touches CI YAML, scripts/ (CRLF/.as_posix() portability), packages/shepherd-driver (git-argv fix), and tests/ only -- no cache, transport, resolver, install pipeline, or algorithmic hot-path code is changed. (Independently verified git-executable caching is correctly implemented -- single shutil.which, no per-call re-resolution -- and confirmed the new Windows CI gate's 225-test/15m scope is well-bounded and contract-guarded against creep.)
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
…ual-packages fixes)
…se + PR-time gate
Root causes (post-main-merge, current CI run 29485676569 / job 87579479487):
1. scripts/ratchet_baseline.py and scripts/run_mutation_pilot.py each hand-rolled
their own atomic write (.tmp + write_text/write_bytes + .replace()), so CRLF
translation via text-mode Windows writes leaked into canonical JSON baselines
and mutation reports. Both now delegate to the single canonical owner,
apm_cli.utils.atomic_io.atomic_write_text, which already guarantees LF-only,
UTF-8/ASCII-safe, atomic output. No behavior changes for the JSON payloads
themselves (json.dumps(..., ensure_ascii=True) output is pure ASCII, a
strict UTF-8 subset).
2. scripts/check_test_contract_authorities.py interpolated Path objects
(BINARY_OWNER, PARITY_OWNER, TEST_FILE_INVENTORY_OWNER,
RATCHET_BASELINE_OWNER, and relative/owner loop variables) directly into
diagnostic f-strings. str(Path) renders backslashes on Windows, so every
affected diagnostic and authority-comparison string silently mismatched its
POSIX-style owner declarations there -- breaking authority detection only
on win32. Fixed by routing all of these (and _parse()'s relative_to(root)
error message) through .as_posix(), matching the pattern already used
elsewhere in the same module.
3. tests/unit/test_shepherd_owner_touch_gate.py and the production script it
exercises, packages/shepherd-driver/scripts/owner_touch_gate.py, both called
subprocess.run(["git", ...], ...) with a bare "git" argv. With shell=False,
Windows CreateProcess does not reliably resolve bare executable names via
PATH the way POSIX exec does, raising FileNotFoundError: [WinError 2]. The
test file now resolves git via the existing canonical owner,
apm_cli.utils.git_env.get_git_executable()/git_subprocess_env(). The
packages/shepherd-driver script is a standalone, portable APM skill
artifact (governed by its own apm.yml, no pyproject.toml, and no other
file under packages/*/scripts/ imports apm_cli) so it gets its own
self-contained fix instead: a cached shutil.which("git") lookup with a
clear GateError on failure, preserving portability for consumers who
install only this skill.
4. tests/unit/integration/test_copilot_app_ws.py's WebSocket test harness ran
Server.serve_forever() directly as a thread target. On Windows,
websockets.sync.server skips the self-pipe shutdown signal POSIX relies on
(upstream guards it with sys.platform != "win32"), so a shutdown() call
that lands mid-poll can raise OSError: [WinError 10038] one level above the
accept-loop's own `except OSError: break`. Added a narrow
_run_serve_forever() wrapper that catches *only*
OSError with winerror == 10038 on win32 and re-raises everything else --
no broad exception swallowing.
Regression tests (new, mutation-break-proven where practical):
- test_run_mutation_pilot.py: replace-failure test now patches the true
os.replace() call site inside atomic_io.
- test_check_test_contract_authorities.py: static-source guards asserting
the four owner-path constants and relative_to(root) are never
interpolated raw (bare {CONSTANT}) in the checker's own source --
verified to fail when the fix is reverted.
- test_shepherd_owner_touch_gate.py: 3 new tests proving the production
gate script never uses a bare ["git", ...] argv, that git-path
resolution is cached (shutil.which called once), and that a missing git
binary fails closed with a clear GateError -- verified to fail when the
fix is reverted.
- test_copilot_app_ws.py: 4 new tests proving the WinError 10038 catch is
narrow (only that error, only on win32) and that normal completion /
other errors are unaffected.
CI hardening -- Windows PR-time gate (closes the "Windows only runs
post-merge on main" gap):
- Added windows-compat-gate job ("Windows Compatibility Gate") to
ci.yml, runs-on windows-latest, timeout-minutes: 15, executing the 10
test files that constitute the load-bearing cross-platform contract
family (ratchet baseline, mutation pilot, authority checker, shepherd
owner-touch gate, WS shutdown, atomic_io, git_env, paths) -- 225 tests,
~18s locally on this host. Verified against tests/quality/
test_ci_topology.py's ratchet-scope caller-uniqueness constraint (none
of the newly-added files carry RATCHET_TEST_SCOPE = "repository", so
this doesn't collide with the existing test-architecture job).
- Registered "Windows Compatibility Gate" in merge-gate.yml's
EXPECTED_CHECKS for both the pull_request/workflow_dispatch and
merge_group branches, so it's enforced as a required PR check without
duplicating the full Linux suite.
- New tests/unit/test_windows_compat_gate_workflow.py (7 tests) pins the
job's shape (name/runs-on/timeout), the exact pytest command, absence
of full-suite-root duplication, and both EXPECTED_CHECKS registrations
-- including 2 mutation-break-proof tests.
Verification:
- ruff check/format (src/ tests/): clean.
- pylint R0801 duplication guard (src/apm_cli/): 10.00/10.
- scripts/lint-auth-signals.sh, scripts/lint-architecture-boundaries.sh:
clean.
- YAML I/O safety guard, file-length guardrail, raw
str(path.relative_to()) guard (src/apm_cli/ scope, as in ci.yml): clean.
- actionlint on ci.yml + merge-gate.yml: clean.
- tests/unit tests/test_console.py tests/red_team (build-and-test-shard
scope): 19253 passed, 2 skipped, 21 xfailed.
- test-architecture job scope (5 files): 52 passed.
- New Windows Compatibility Gate's exact 10-file list: 225 passed.
- tests/quality/ + workflow-contract tests: 50 passed.
- tests/acceptance + tests/spec_conformance: 156 passed, 2 skipped.
- WS shutdown + shepherd owner-touch-gate files run 5x consecutively:
45/45 passed every run (thread-safety/determinism check).
Refs: #2233, #2237
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 09f373aa-49b9-487b-86e0-fc095e47a00c
Docs sync advisoryVerdict: no_change * Pages affected: 0 * LLM calls: 1/15 * Took: ~30s SummaryThis PR touches only internal CI workflow configuration, build/test scripts, a standalone skill package script, and test files -- no user-facing CLI command, flag, schema, public API, or documented behavior changed. The classifier's L0 fast-path could not short-circuit because No documentation changes needed for this PR. L0 did not short-circuit because packages/shepherd-driver/scripts/owner_touch_gate.py is outside the current no_impact_paths map, even though the rest of the diff is workflows, scripts, tests, and CHANGELOG-class meta surfaces. L1 then found no user-observable APM CLI command, flag, schema, public API, or documented error-surface change: the added workflow uses generic tooling flags, the script changes are internal Windows/path/atomic-write fixes, the package script change is an internal git-resolution fix, and the remaining edits are tests. The PR touches no docs/src/content/docs/** pages, so the docs corpus map yields no candidate pages. Note: the CHANGELOG.md How this advisory was produced
This is an advisory comment from the Re-run by removing and re-applying the |
Per apm-review-panel synthesis on PR #2237 (3 independent personas -- oss-growth-hacker, doc-writer, devx-ux-expert -- converged on this finding): add the Unreleased CHANGELOG entries required by .github/instructions/changelog.instructions.md for the 4-class Windows-only CI fix (closes #2233) and the new Windows Compatibility Gate CI job introduced in this PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09f373aa-49b9-487b-86e0-fc095e47a00c
The Windows git-executable fix in packages/shepherd-driver/scripts/ owner_touch_gate.py (the canonical source) had drifted from its deployed copy at .agents/skills/shepherd-driver/scripts/ owner_touch_gate.py, which this repo's own apm.lock.yaml tracks and apm audit --ci verifies on every PR. Ran 'apm install' to re-materialize the deployed skill copy and refresh the lockfile's content hash for the single changed file. This is what the 'APM Self-Check' CI job's drift check was correctly catching (self-dogfooding: apm-cli audits its own dependency deployments). Verified locally: 'uv run apm audit --ci' now reports 'All 9 check(s) passed' with the lockfile diff limited to the one file's hash plus the generated_at timestamp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09f373aa-49b9-487b-86e0-fc095e47a00c
Context
Closes #2233 and hardens the underlying CI/test-architecture defect class, not just the two originally-reported symptoms.
Main's Windows job (CI run 29485676569, job 87579479487) was failing on 4 distinct signatures, all sharing one root cause: Windows-only regressions are invisible at PR time because PR CI only runs on Linux; the Windows build only executes post-merge on
main.test_write_report_is_deterministic_atomic_and_printable_asciiwrites CRLFscripts/ratchet_baseline.pyandscripts/run_mutation_pilot.pyhand-rolled their own write-then-replace logic instead of using the existing atomic writerapm_cli.utils.atomic_io.atomic_write_text(LF-normalizing, atomicos.replace-based)scripts/check_test_contract_authorities.pyinterpolated rawPathobjects into f-strings, emitting backslash paths on Windows and breaking string-based authority matching.as_posix()before formattingtests/unit/test_shepherd_owner_touch_gate.py-- WindowsFileNotFoundError: [WinError 2]packages/shepherd-driver/scripts/owner_touch_gate.pyinvoked bare["git", ...]argv; WindowsCreateProcessdoes not PATH-search extension-less names whenshell=Falseapm_cli.utils.git_env.get_git_executable()/git_subprocess_env().owner_touch_gate.pyis a standalone distributable skill script with noapm_clidependency, so it gets a small self-containedshutil.which-based cache instead (documented cross-reference to the canonical owner)websockets.sync.serverskips its self-pipe shutdown signal on win32, so a concurrentserve_forever()+shutdown()can raiseOSError: [WinError 10038]thread.join(timeout=1.0)CI hardening: new PR-time Windows gate
Added a Windows Compatibility Gate job (
.github/workflows/ci.yml,runs-on: windows-latest,timeout-minutes: 15) that runs a focused 10-file / 225-test cross-platform contract subset at PR time, registered in.github/workflows/merge-gate.yml'sEXPECTED_CHECKS(both branches). This is deliberately NOT a full-suite duplicate -- a dedicated contract test (tests/unit/test_windows_compat_gate_workflow.py) pins the job's scope, timeout, and asserts it never grows to include the full-suite roots, so it can't silently become the new bottleneck.Architecture impact
This is a genuine dual-authority consolidation, not a one-off patch set: two scripts that had drifted into hand-rolled write logic now reuse
atomic_write_text; the authority-checker and shepherd git-helper bugs are both instances of the same "raw platform-native value crossed a text boundary" defect class, fixed at their respective canonical owners (atomic_io,git_env, plus a documented standalone exception for the portableshepherd-driverpackage).Regression tests
tests/unit/integration/test_copilot_app_ws.py)tests/unit/scripts/test_check_test_contract_authorities.py) -- mutation-break-verified: reverting.as_posix()makes the new test fail with a clear messagetests/unit/test_shepherd_owner_touch_gate.py), including a static-source guard against reintroducing baregitargv, and a test proving theshutil.whichcache is populated exactly oncetests/unit/test_windows_compat_gate_workflow.py), including 2 mutation-break-proof testsHow to test
Verification evidence (this session)
relative_toguard, actionlint (scoped to the 2 edited workflow files) -- all cleanbuild-and-test-shardscope: 19253 passed, 2 skipped, 21 xfailedtest-architecturescope: 52 passedtests/quality: 50 passedtests/acceptance+tests/spec_conformance: 156 passed, 2 skippedtest_copilot_app_ws.py,test_shepherd_owner_touch_gate.py) re-run 5x consecutively: 45/45 passing every run, zero flakinessapm-review-panel(9 personas + CEO synthesis) anddocs-syncadvisory panels both run; findings posted as PR comments. No blocking findings from any panelist. Top follow-up (missing CHANGELOG entry) already folded into this PR.Follow-ups filed as future work (not blocking this PR)
devx-ux-expertsurfaced ~25 productionsrc/apm_cli/call sites sharing the identical bare-git-argv Windows bug this PR fixes in tests/scripts -- worth a dedicated migration issue linked to [BUG] Windows CI fails due to CRLF baseline output, missing ratchet baseline authority detection, and WebSocket shutdown warning #2233..as_posix()sites (src/apm_cli/models/validation.py:769,773,src/apm_cli/bundle/packer.py:296) share the backslash-path diagnostic bug class..github/workflows/ci.yml) could be narrowed from a blanketGet-ChildItem Env:to targeted vars (2 personas flagged, low risk).Out-of-scope, pre-existing issues observed (not fixed, noted for visibility)
tests/perf/conftest.py'spytest_collection_modifyitemshook applies itsPYTEST_PERFopt-in skip marker to the entire pytest session (no path-scoping), so a barepytest tests/skip-marks everything. Only manifests whentests/perfis collected alongside other dirs; CI's actual scoped invocations never do this, so it never affects real CI.tests/integration/hangs when run broadly (likely real network/git-clone fixtures); untouched by this PR, belongs toci-integration.yml's domain.closes #2233