ci: run the test suite on Windows as well as Linux

[Frank Li (franklixuefei)]

Fixes#385.

The gap

CI ran ubuntu-latest only for Lint, Type Check and Test. Conductor has a substantial amount of Windows-specific code that nothing executed:

• cli/pid.py — an entire OpenProcess / GetExitCodeProcess ctypes implementation
• cli/bg_runner.py — CREATE_NEW_PROCESS_GROUP, DETACHED_PROCESS, job-object breakaway and its access-denied fallback
• sys.platform == "win32" branches in cli/app.py and cli/update.py

Three bugs found in one week of Windows use all reached a release: #342 (JSON truncation via a if WINDOWS chunked-write path), #344 (conductor stop orphaning runs because CTRL_BREAK_EVENT needs a shared console), #382 (MarkupError through the console path).

The gap is bidirectional, which is the part I would stress

It is tempting to read this as "Linux CI can't catch Windows bugs". It is worse than that — Linux-only CI also lets Windows-passing tests hide Linux breakage, because the existing tests fake the platform.

Both directions bit me while fixing #344:

• A test I wrote patched conductor.cli.app.os.kill. os is a single shared module object, so on POSIX that also patched cli/pid.py's liveness probe and turned a DEAD result into UNKNOWN. It passed on Windows and would have failed on Ubuntu CI. Caught by review, not by a test run.
• Conversely, _liveness_windows is covered everywhere by monkey-patching a _kernel32 mock. That is deliberate and good — the logic is exercised on every platform — but it means nothing validates the real ctypes argtypes/restype. An ABI mistake would be CI-green and fail only on a user's machine.

So the current setup cannot catch either direction, and the mocked-platform tests give a false sense of coverage for the one platform where the code actually differs.

The change

Windows added to the test matrix on one Python version rather than a full 2×2:

matrix:
os: [ubuntu-latest]python-version: ["3.12", "3.13"]include:
- os: windows-latestpython-version: "3.13"

The platform is the variable here, not the interpreter — the Windows-specific code is not version-sensitive. And the Install Scripts job already provisions a Windows runner on every PR (its own comment says it runs there because "the failure modes actually live" there), so this is incremental cost on infrastructure that is already paid for, not a new platform commitment.

Two things this required beyond the matrix:

• The "Remove bundled Copilot CLI binary" step used find, which is not a native Windows command — the step would have failed outright. It now runs under shell: bash (Git Bash is preinstalled on the runner) and also matches the Windows venv layout, where scripts live under Scripts/ and the binary carries a .exe suffix.
• Coverage upload is now gated on ubuntu-latest as well as 3.12, so a second runner cannot upload a partial report for the same commit and skew the totals.

I expect the first Windows run to be red

I would rather say that up front than have it look like a surprise. Likely causes, in order:

1. Tests that fake a platform.patch("conductor.cli.pid.sys.platform", "win32") and the _kernel32 mocks are written to run on Linux; some will double up confusingly on real Windows. There is already a correct pattern for this in the repo — TestIsProcessAliveWindowsReal guards with skipif.
2. Path and encoding assumptions. Tests asserting on /tmp/... string literals, and anything comparing rendered console output.

I am happy to triage that first red run and push the fixes onto this branch — I have a Windows environment and have already been through these modules for #381, #383 and #387. Equally happy for you to prefer a narrower first step (e.g. Windows restricted to tests/test_cli/test_pid.py, test_stop.py, test_bg_runner.py, where essentially all the platform-specific code lives) — say the word and I will rework it.

Related

#342 / #381, #344 / #383, #382 / #387 — every one of them is a bug this job would plausibly have caught before release.

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 88.00000% with 6 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0ad105c). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/conductor/registry/config.py	78.57%	6 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #390 +/- ##
=======================================
Coverage ? 91.78% =======================================
Files ? 112 Lines ? 18278 Branches ? 0 =======================================
Hits ? 16777 Misses ? 1501 Partials ? 0 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Frank Li (franklixuefei)]

First Windows run: 30 failed, 5,076 passed (99.4%)

Better than I expected, and more useful than a green run would have been — every failure is a real portability bug that was invisible before.

All 30 are test-level assumptions, not conductor defects. I checked each class against the product code; nothing here is a bug a user would hit.

file	count	class
test_registry/test_integration.py	10	path separators
test_mcp/test_manager_truncation.py	5	POSIX file modes
test_skills/test_path_entries.py	3	path separators
test_providers/test_claude_agent_sdk.py	2	path separators
test_providers/test_pydantic_ai_mcp.py	2	path separators
test_skills/test_registry.py	2	path separators
6 others	1 each	mixed

Four distinct causes, all mechanical:

1. Hardcoded POSIX separators (~20 of 30)

assert parsed["data"]["path"] == "/some/path"
E AssertionError: assert '\\some\\path' == '/some/path'

The product normalises correctly; the test compares against a literal.

2. POSIX file modes (5, all in test_manager_truncation.py)

assert stat.S_IMODE(spill_file.stat().st_mode) == 0o600
E AssertionError: assert 438 == 384

0o600 is not expressible on Windows — chmod there sets only the read-only bit, so the mode reads back 0o666. The spill file is created correctly; the permission assertion simply cannot hold. Needs a skipif, or an assertion on POSIX only.

3. Line endings (1)

assert output.stdout == "from-args\nfrom-stdin"
E AssertionError: assert 'from-args\r\nfrom-stdin' == ...

4. POSIX-shaped fixture paths (1)

ExecutionError: Agent 'worker': working_dir 'C:\Users\runneradmin\proj' does not exist

The test builds a path the product then correctly rejects.

How would you like this split?

I do not think 30 test fixes belong in the same PR as "add a CI job" — it makes the review much harder and mixes two concerns. Three options, and I am happy with any:

1. Land the job as continue-on-error: true now, fix the 30 in a follow-up, then flip it to blocking. Gets the signal immediately without gating anyone, at the risk that an advisory job gets ignored.
2. Hold this PR, I push the 30 fixes onto this branch, and it lands green and blocking in one go.
3. Narrow the first step to the platform-sensitive modules (test_cli/test_pid.py, test_stop.py, test_bg_runner.py) which are already green here, and widen once the rest are fixed.

My preference is 2 — the fixes are mechanical and a blocking job from day one is worth more than an advisory one — but it is your call on review load, and 1 is the lower-risk way to start.

One thing worth noting either way: the platform-specific modules this job exists for — test_pid.py, test_stop.py, test_bg_runner.py, test_stop_ladder.py — all passed on Windows. So the ctypes layer is genuinely sound; the failures are entirely in tests that were written POSIX-first.

[Frank Li (franklixuefei)]

Correction, and a result: the Windows job found a product bug on its first run

When I opened this I reported the first Windows run as "30 failures, all test-level POSIX
assumptions, zero product defects."That was wrong, and I'd rather correct it here than let it
stand.

Ten of the thirty were one real defect in conductor/registry/config.py.

The bug

_format_toml built TOML by interpolating values directly into basic strings:

lines.append(f'source = "{entry.source}"')

TOML basic strings process backslash escapes. A path registry pointing at a Windows directory
therefore serialises as:

source = "C:\Users\alice\AppData\Local\workflows"

\U is a Unicode escape, so reading it back fails:

RegistryError: Failed to parse ...registries.toml: Invalid hex value (at line 5, column 15)

conductor registry add writes a config file that can never be read back — every subsequent
registry command on that machine fails.

This is not Windows-only. It is only reliably reachable on Windows because paths there start
with a drive and contain backslashes. On Linux, any source containing \ or " corrupts the file
the same way, and \t/\n corrupt it silently rather than erroring, which is worse.

Fixed by escaping per the TOML v1.0 spec.

Why the existing tests missed it

The round-trip test used a POSIX path with nothing to escape:

"dev": RegistryEntry(type=RegistryType.path, source="/dev/workflows"),

Added a backslash round-trip plus a parametrised case per metacharacter: \U (invalid hex),
\t/\n (silent corruption), \x, an embedded quote, and a trailing backslash. I verified these
actually bite — reverting _toml_escape turns exactly those 6 red and nothing else.

The other 29

Genuinely test-level, grouped by cause in the commit message: POSIX mode bits (S_IMODE reports
0o666 on Windows regardless), path separators, ~ resolving from USERPROFILE rather than HOME,
\r\n, Path("/x") not being absolute without a drive, chmod(0o000) blocking neither a Windows
owner nor root, and a case-insensitive filesystem making a mis-cased Skill.md unreachable.

One of these is now a better test on both platforms:
test_spill_os_error_falls_back_to_marker_without_path simulated failure with mkdir(mode=0o500),
which is a no-op on Windows — the spill quietly succeeded and produced the wrong marker. It now
patches os.open to raise, exercising the same except (OSError, ValueError) branch
deterministically everywhere, instead of depending on a permission side-effect.

On the earlier question about how to split this

I asked which of three split options you'd prefer, including landing the job continue-on-error.
Withdrawing that: a non-blocking CI job is one nobody looks at, which defeats the point of #385. The
job stays blocking and the suite is now green under it.

Verification

Run locally on Windows (Python 3.14): 4,717 passed. The 3 remaining failures
(test_aca_runner ×2, test_agent_iteration_limits ×1) reproduce identically with this commit's
production change reverted, so they are pre-existing and environmental rather than introduced
here. ruff check and ruff format --check clean.

Happy to split the product fix into its own PR ahead of the CI change if you'd prefer them
separate — though they are coupled in the sense that the job cannot go green without the fix.

[Frank Li (franklixuefei)]

✅ Green — 10/10, Windows included

Test (Python 3.13, windows-latest): 5,106 passed, 45 skipped, 0 failed. The job is
blocking, not continue-on-error.

The 30 → 0 walk, and what it cost to get there:

2 product defects	TOML escaping (10 failures), bundled-CLI detection on Windows (1)
19 test-level	POSIX mode bits, separators, \r\n, USERPROFILE vs HOME, driveless roots, chmod(0o000), case-insensitive FS, JSON-escaped paths
1 test made stronger	permission-based failure simulation → deterministic os.open patch

The second product defect

validate_connection carries a comment saying to keep it in sync with the SDK's _find_cli. It had
drifted twice:

• The bundled probe looked only for _bundled/claude. The SDK names it per-platform —
  cli_name = "claude.exe" if platform.system() == "Windows" else "claude" — so conductor reported
  "Claude CLI not found" on Windows with the bundled binary sitting right there.
• The fallback list probed POSIX paths on every platform. Path("/usr/local/bin/claude") is rooted
  but driveless, so on Windows it resolves against the current drive — C:\usr\local\bin\claude
  — a path any unprivileged local user can create. A planted file there would make conductor report
  the CLI as available. The SDK refuses these on Windows and names the reason in its own source: a
  binary-planting probe. Conductor now matches, probing only ~/.local/bin/claude.exe.

Scope note: conductor uses this for a boolean availability check, not to select what it spawns — so
this is a false-positive signal rather than direct execution. Worth fixing regardless, and worth not
diverging from the SDK on a security-shaped decision it made deliberately.

On being wrong

I opened this PR asserting "all 30 are test-level POSIX assumptions, zero product defects." Two of
them were product defects, one security-shaped. Thirty failures was too many to have actually checked
before saying that, and the confident phrasing is what made it worth correcting rather than quietly
amending.

That is also the argument for the job existing: it found two real bugs on its first two runs, in code
paths that ship to Windows users today and were invisible to a Linux-only matrix.

Verification

Local Windows run (Python 3.14): 4,716 passed. Three failures (test_aca_runner ×2,
test_agent_iteration_limits) reproduce identically with the production changes reverted — I
checked rather than inferred it from "those files look unrelated". test_performance is
pytestmark = pytest.mark.performance and is deselected in CI. ruff check / ruff format --check
clean. Regression tests for the TOML fix were verified to fail on revert — all 6, and nothing else.

Ready for review. Happy to split the two product fixes into their own PR ahead of the CI change if
you'd rather review them separately — though the job cannot go green without them, so they'd need to
land first either way.

[Jason Robert (jrob5756)]

Thanks for doing the triage and digging in! The Windows leg earns its place: it found a real cross-platform bug in _format_toml on its first run, and it makes TestIsProcessAliveWindowsReal execute for the first time since it was written.

Most of my comments are about the two production fixes that came along for the ride, not the CI change itself

The one I can't leave inline: neither production fix has a CHANGELOG entry. The registry TOML bug hit every platform, not just Windows, so it belongs under Unreleased > Fixed alongside the Claude CLI detection change.

I pulled both claude-agent-sdk wheels to check the SDK claims. Details in the comment on claude_agent_sdk.py.

[Jason Robert (jrob5756)]

I pulled both wheels to check this. claude-agent-sdk 0.2.87, which is what uv.lock pins and what CI installs, has no platform branch in _find_cli at all. It probes all six POSIX paths on every OS, Windows included. The branch you're mirroring shows up much later: 0.2.134 has it, with almost this exact binary-planting wording.

Two consequences. The comment tells a future maintainer the hole is closed upstream, and at the pinned version it isn't, since the SDK still probes and still spawns a planted C:\usr\local\bin\claude. And on Windows, a user whose CLI sits at ~/.claude/local/claude (where Claude Code's local installer puts it) now gets validate_connection() == False even though the SDK would find and run it. What they see is "Failed to connect... Check your credentials and network connection", plus a suggestion to npm-install a CLI they already have.

Worth noting only /usr/local/bin/claude is driveless. The other five are Path.home()-anchored, so the planting argument doesn't cover them. Dropping just that one entry on Windows would give you the hardening without the false negative.

If you keep the narrowing, name the version you audited. This file already does that well at line 1274 ("audited against claude-agent-sdk 0.2.87").

Suggested change

	# The SDK refuses these on Windows for exactly this reason.
	# Note this narrows Conductor's report only: claude-agent-sdk 0.2.87 (the
	# locked version) still probes these unconditionally in _find_cli, so a
	# planted binary can be spawned even when this returns False. Later SDKs
	# (>= 0.2.13x) refuse them; this matches that behaviour ahead of the pin.

[Jason Robert (jrob5756)]

Line 805 returns first here. On Windows shutil.which("claude") already applies PATHEXT, so a claude.exe on PATH gets found there and this never runs.

0.2.134 makes the equivalent probe reachable by vetting the hit with _is_windows_native_exe first, so an npm claude.cmd shim doesn't short-circuit it. Without that vetting the branch is dead. Either port the vetting or drop the branch and its comment.

[Jason Robert (jrob5756)]

Every other Windows check in src/ uses sys.platform == "win32". There are 11 of them, and cli/pid.py:44 calls it out as the house idiom.

Beyond consistency it costs testability: the repo fakes Windows by patching sys.platform (test_script.py:472 does exactly this), and nothing patches platform.system. Switching would let you exercise the new branch from Linux.

Needs import sys at module scope, which this file doesn't currently have.

Suggested change

	is_windows=platform.system()=="Windows"
	is_windows=sys.platform=="win32"

[Jason Robert (jrob5756)]

The values are escaped now, but the table key still isn't, and _validate_registry_name only rejects / and \. I ran it: my.team, has"quote, with space, a=b and a#b all write successfully and then never load.

That's a worse version of the bug you're fixing, because add_registry, remove_registry and get_registry all call load_config() first. Once the file is written the user can't remove the entry that broke it, and unrelated registries go down with it. conductor registry add my.team reports success.

Names also become cache directory names (registry/cache.py:93), and ", :, *, ? are illegal in Windows filenames, so a charset check at write time covers both problems. Something like ^[A-Za-z0-9._-]+$ in _validate_registry_name would subsume the existing / and \ cases.

If you'd rather escape than reject:

Suggested change

	lines.append(f"[registries.{name}]")
	lines.append(f'[registries."{_toml_escape(name)}"]')

[Jason Robert (jrob5756)]

The new Windows branch in validate_connection has no test behind it. On Linux is_windows is always False, so it's dead code. On the Windows runner the bundled claude.exe ships in the wheel, so the probe returns True at line 801 and the fallback block never runs there either. You could delete the whole branch and all three CI legs stay green.

It's testable from Linux, since platform is imported inside the function:

withpatch("platform.system", return_value="Windows"):
...

Asserting the probe set per platform would have caught the SDK version mismatch while you were writing it, because you'd have had to put ~/.claude/local/claude -> False in the test as a literal expectation.

[Jason Robert (jrob5756)]

This reports PASSED on Windows having asserted nothing about the mode of a file that holds raw tool output. Same shape at line 263, at test_pydantic_ai_mcp.py:349 and :411, and at test_claude_agent_sdk.py:1826, where the file holds resolved env values and Authorization headers.

You already have the honest mechanism a few lines up. Splitting the mode assertion into its own @requires_posix_perms test makes the report say SKIPPED rather than PASSED, which is the difference between a known gap and an invisible one.

[Jason Robert (jrob5756)]

The reason is accurate, but it describes the fixture rather than the behaviour. expand_skills_root reports any immediate subdirectory without a SKILL.md; the mis-cased filename is only one way to trigger it. A file that isn't a skill manifest keeps the assertion running everywhere:

(root/"oops"/"notes.md").write_text("a file that is not SKILL.md")

This one matters more than most, because mis-casing SKILL.md is a Windows-authoring mistake specifically: it works locally and then vanishes for Linux teammates. Windows is where you'd most want the diagnostic covered. Keeping the mis-cased variant as a separate POSIX-only test would preserve both.

[Jason Robert (jrob5756)]

The binary ships inside the package on both platforms (copilot/bin/copilot on Linux, copilot/bin/copilot.exe in the win_amd64 wheel), and there's no console-script entry point, so it never lands in Scripts/. The pattern above already matches the Windows layout: the leading */ absorbs Lib/site-packages/ and the trailing * absorbs .exe. I checked against both wheels.

So line 138 is an unanchored -iname sweep of the whole venv that deletes nothing the line above missed.

Separately, find exits 0 when it matches nothing, and 2>/dev/null || true hides -delete failures, which are likelier on Windows against a read-only file. Either way the binary survives, tests taking the real CLI path hang, and the job dies at the 10-minute timeout with an error naming neither the CLI nor this step.

Suggested change

	find .venv -path '*/copilot/bin/copilot*' -delete 2>/dev/null || true
	find .venv -iname 'copilot.exe' -delete 2>/dev/null || true
	found=$(find .venv -path '*/copilot/bin/copilot*' -print -delete)
	if [ -z "$found" ]; then
	echo "::error::No bundled Copilot CLI matched. The SDK layout changed; tests taking the real CLI path will hang until the job timeout."
	exit 1
	fi
	echo "Removed: $found"

[Jason Robert (jrob5756)]

find.exe ships in System32, so a bare find does resolve to a real binary under cmd or pwsh. It's the legacy DOS search tool with unrelated semantics. shell: bash is still the right fix, but "not a native command" would send someone hunting for a missing tool rather than a colliding one.

While you're in here, the header block at line 9 still describes the test job as "Python 3.12 and 3.13" with no platform, and it does name platforms for install-scripts further down.

Suggested change

	# is preinstalled). Without it the step fails on Windows, since `find`
	#is not a native command there.
	# is preinstalled). Without it the step fails on Windows, where a bare
	#`find` resolves to the unrelated DOS search tool in System32.

[Frank Li (franklixuefei)]

Thanks — the SDK finding was the important one and it was right. Verified against the wheels rather than release notes, then fixed in 2b8c26f.

The SDK claim, confirmed

Pulled 0.2.87 and read _internal/transport/subprocess_cli.py. No platform branch in _find_cli at all — it probes all six POSIX paths on every OS:

ifcli:=shutil.which("claude"):
returnclilocations= [
Path.home() /".npm-global/bin/claude",
Path("/usr/local/bin/claude"),
Path.home() /".local/bin/claude",
Path.home() /"node_modules/.bin/claude",
Path.home() /".yarn/bin/claude",
Path.home() /".claude/local/claude",
]

So the comment told a future maintainer the hole was closed upstream when at the pinned version it is not. And your second consequence is the one that would have hurt: only /usr/local/bin/claude is driveless, so dropping the other five made validate_connection() return False on Windows for a CLI at ~/.claude/local/claude — where Claude Code's own installer puts it, and which the SDK would find and run.

Now only the driveless entry is skipped on Windows, the comment names the audited version as you suggested, and it says plainly that this narrows conductor's report only.

(0.2.134 is not on our package proxy — latest available is 0.2.132 — so I confirmed the 0.2.87 half directly and took your reading of the later one.)

The other two on that file

• claude.exe branch dropped. You are right that shutil.which applies PATHEXT, so the line above already finds it and the branch was dead. There is nothing to port from 0.2.87, which has no vetting.
• platform.system() → sys.platform == "win32". House idiom, and it is what makes the branch testable from Linux — the repo fakes Windows by patching sys.platform and nothing patches platform.system.

Registry names — worse than described, and the suggested fix alone was not enough

Reproduced all five. But I found the charset check by itself would not have closed it: the key is written unquoted at config.py:178, so my.team — which ^[A-Za-z0-9._-]+$ allows — becomes [registries.my.team], a nested table. The entry then vanishes silently on load rather than failing loudly, which is arguably worse than the parse error.

So I did both of your suggestions: quote the key and restrict the charset. A round-trip test pins that a dotted name stays one registry key.

Tests

You were right that the Windows branch had no test behind it, and about why: on Linux is_windows is always False, and on the Windows runner the bundled claude.exe short-circuits before the fallbacks — so it ran on no CI leg and could have been deleted with all three staying green.

It now asserts the probe set per platform, which is exactly the thing that forces ~/.claude/local/claude to be written down as a literal expectation.

One honest caveat on discrimination: those two tests fail against the pre-fix code, but partly for a mechanical reason — the old code used platform.system(), so patching sys.platform finds no attribute. They pin the corrected behaviour going forward rather than cleanly isolating the old bug.

CHANGELOG

Added for both production fixes, as you asked — the registry TOML escaping and the Claude CLI detection change, plus the name-validation fix.

Not done here

The skipped-on-Windows file-mode tests (test_manager_truncation.py:83 and :263, test_pydantic_ai_mcp.py:349/:411, test_claude_agent_sdk.py:1826) report PASSED having asserted nothing about files that hold tool output and Authorization headers. That is a real gap and I agree it should not stay, but it is five call sites across three files and unrelated to the CI matrix this PR adds — happy to take it as a follow-up if you would rather keep this PR to its subject.

Gates

2,152 passed across test_registry, test_providers, test_cli and test_executor. ruff check and ty check src both unchanged from this branch's baseline (2 and 68).

[Frank Li (franklixuefei)]

CI update: the Windows leg is now failing, and it is doing its job.

Merged main (631dda9). Test (Python 3.13, windows-latest) fails on 5 tests in tests/test_plugins/test_fetch.py — code that arrived from main with the plugin work (#378/#394), not from this PR.

Confirmed pre-existing rather than assumed. I checked out plain origin/main with no PR changes on a Windows box and ran the same file:

FAILED tests/test_plugins/test_fetch.py::TestPinning::test_pinned_source_resolves_without_the_remote
FAILED tests/test_plugins/test_fetch.py::TestOfflineFallback::test_warns_and_reuses_the_cached_checkout
FAILED tests/test_plugins/test_fetch.py::TestOfflineFallback::test_the_ref_pointer_is_what_makes_the_fallback_possible
FAILED tests/test_plugins/test_fetch.py::TestReadiness::test_a_checkout_without_its_sentinel_is_not_cached
FAILED tests/test_plugins/test_fetch.py::TestMultipleSourceFailures::test_a_duplicated_source_is_fetched_once
5 failed, 35 passed

Identical set, identical on this branch. Nothing in this PR touches src/conductor/plugins/.

There appear to be two distinct problems:

1. PermissionError: [WinError 5] during teardown — shutil.rmtree on source-repo\.git\objects\3a\.... Git marks object files read-only, and on Windows rmtree cannot unlink a read-only file. The two rmtree calls inside plugins/fetch.py pass ignore_errors=True, so this is the test fixture's own cleanup, not production code.
2. PluginSourceError from fetch.py:671 — a fetch failure that surfaces before teardown, so at least some of these are not merely a cleanup artifact.

That is the third real cross-platform bug this leg has found, after _format_toml and the Claude CLI probe — which is a decent argument for the leg itself.

How would you like to sequence it? Three options as I see them:

• Fix it here, and accept that this PR grows a src/conductor/plugins change it did not set out to make.
• Fix it in a separate PR that lands first, and rebase this one onto it — my preference, since the fix belongs with the plugin code and its author has the context.
• Land this with the Windows leg continue-on-error: true and a follow-up issue to remove that, so the matrix is in place while the plugin bug is fixed. Cheapest, but a soft gate has a habit of staying soft.

Happy to take the first or second if you would rather not context-switch; say which and I will do it. I have not guessed, because all three are defensible and it is your PR's scope to spend.

Everything else on this PR is green: Lint, Type Check, both Ubuntu legs, Frontend, both Install Scripts legs, and Validate Example Workflows.

[Frank Li (franklixuefei)]

Update: I stopped asking how to sequence around the Windows failures and fixed them.92ebd97.

I had offered three options for working around the 7 failures on the new Windows leg. That was the wrong question -- a CI leg you have to read past is worse than no CI leg, and this PR exists to make Windows visible. All 7 turned out to be real platform bugs in conductor, not test artefacts, and all 7 are fixed.

What was actually broken

Three distinct defects, all reachable only from a file:// source, all of which put a checkout somewhere other than where the cache key said:

1. Backslashes survived as separators. The key is built from the URL path split on /, so a native Windows path arrived as a single segment with its separators intact. The key kept them and the checkout landed at a drive-absolute location outside the cache root entirely. That is the same escape the .. check exists to prevent, through a door it does not cover.
2. The drive colon survived. An owner of C:_src reads as a drive on Windows, or mid-name as an NTFS alternate data stream.
3. The flattened path was too long. Joining every segment but the last into one owner produced a name long enough that git refused to create .git inside it, at the 260-character cap.

This is what test_a_checkout_without_its_sentinel_is_not_cached was reporting as StopIteration: is_cached() was True while rglob("*.ready") found nothing, because the sentinel was written outside the tree being searched.

Separately, _publish tolerates losing a race to a concurrent fetch, but recognised only ENOTEMPTY/EEXIST. Windows reports a non-empty destination directory as ERROR_ACCESS_DENIED. I checked rather than inferred:

os.replace(dir_a, dir_b) -> errno 13 EACCES, winerror 5

So the tolerance never applied there and a second source resolving to the same SHA failed the whole fetch. Accepting it is safe because the readiness sentinel is written after publishing: a winner that died mid-clone leaves no sentinel, is_cached reports a miss, and the tree is re-fetched rather than read half-written.

And _is_local_path asked pathlib.Path -- the running platform's flavour -- so /srv/plugins was not a local path on Windows. Both conventions are consulted now.

Choices worth challenging

• Substituted, not refused. Unlike .., a Windows path is a legitimate source, so refusing would make file:// unusable there.
• Applied on every platform, not gated on sys.platform. One workflow file must resolve to the same cache layout wherever it runs, and a Windows-gated branch would never execute on the Linux legs -- the same argument the cp1252 tests in fix(cli): emit ASCII-safe JSON so results survive a legacy stdout codec #381 make.
• Digest for an over-long segment. The key's leaf already carries a digest of the full location, so the owner disambiguates nothing on its own; there is a test that two sources differing only mid-path still get distinct keys.
• Test-side:shutil.rmtree of a git repo fails on Windows because git marks objects read-only, so the four sites that delete a checkout now share a helper that clears the bit. That one genuinely is a test bug.

Verification

tests/test_plugins: 365 passed, 2 skipped -- was 7 failed. Full suite locally: 5690 passed. CI's Windows leg was 7 failed, 5632 passed, so this should take it to zero.

New assertions cover containment directly rather than leaving it to whichever test happened to notice. Reverting each fix independently:

reverted	result
backslash folding	caught
colon substitution	caught
length bound	caught

Five tests still fail on my Windows box and do not fail on CI's -- two test_aca_runner server tests, two test_engine/test_limits clock-resolution ones, and a test_skills case that shells out to uv build. I confirmed all five fail identically with my changes stashed, so they are environmental here rather than yours or mine.

[Frank Li (franklixuefei)]

Correction to my last comment, and CI is now green: 5700 passed, 0 failed on the Windows leg (was 7 failed, 5632 passed).

The first attempt regressed it to 23 failures, and the reason is worth recording because it is the same trap twice.

I claimed the containment fix was verified because tests/test_plugins went 7 failed to 365 passed on my machine. It did. But my home directory is C:\Users\xuefl and the runner's is C:\Users\runneradmin, and that difference is enough: the same path came to 264 characters there against a 260 limit, and 240 here. I verified on the only box where the bug could not reproduce.

What that exposed is the more interesting part. The escaped path was short. Landing the checkout at a drive-absolute location outside the cache had been masking a length problem the whole time — so containing it did not cause the second bug, it revealed one that had been there since file:// sources were supported. The Filename too long errors are what the escape was hiding.

Two changes in 6f4178b:

• Segment bound 48 to 24 characters, keeping the tail rather than the head. Sized against CI's actual worst case rather than a guess: 264 at 48, 240 at 24. The tail is also the more useful half — _local/is_honoure0-b60d29bde844/source-repo-ffba0f1a names the directory nearest the repository, where the head is a drive letter and Users.
• core.longpaths=true on every git invocation. git-for-Windows refuses any path over 260 regardless of what Conductor generates, and this is its documented opt-in. Set per-invocation because the depth is a property of the paths Conductor builds, not a preference of the user's. A no-op on POSIX.

Neither alone is enough. The bound keeps Conductor's own paths reasonable; core.longpaths covers a user whose cache legitimately sits somewhere deep, which no bound of mine can fix.

I also A/B'd core.longpaths at a forced breach rather than assuming it would help, and confirmed it is not sufficient on its own — which is why both changes are here.

Final state, all against the runner rather than my box:

leg	before	after
Test (3.13, windows-latest)	7 failed, 5632 passed	5700 passed
Test (3.12 / 3.13, ubuntu-latest)	pass	pass
Lint / Type Check / Install Scripts / Frontend	pass	pass

11 of 11 checks green.

[Jason Robert (jrob5756)]

Pushed the four leftovers onto your branch rather than sending you round again — they were all small and all on the CI change itself. Two commits: 45080b8 and 5036fd4.

The removal step now verifies itself. It could not fail and never checked that it worked: find exits 0 when it matches nothing, so 2>/dev/null || true was masking real errors rather than the no-match case. A layout change would leave the binary in place, hang the tests on auth, and surface ten minutes later as a bare timeout naming neither the CLI nor the step. It now fails immediately with an annotation. I exercised both paths against a real venv — present removes and exits 0, absent annotates and exits 1.

Dropped the second find. Checked both wheels: the binary ships inside the package (copilot/bin/copilot, copilot/bin/copilot.exe) with no console-script entry point, so it never reaches Scripts/. One pattern covers both layouts. Incidentally confirmed when my local venv resynced as 3.13 instead of 3.12 and the pattern still matched.

Two comment corrections.find.exe ships in System32, so it collides with the DOS search tool rather than being absent — a different debugging trail. And the header block listed the test job as Ubuntu-only while naming platforms for install-scripts a few lines down.

Split the skills-root test so the general case runs on Windows. The skip reason was accurate but described the fixture, not the behaviour: expand_skills_root reports any subdirectory without a SKILL.md, and mis-casing is just the one trigger that cannot exist on a case-insensitive filesystem. A plain non-manifest filename keeps the assertion live everywhere; the mis-cased case kept its own test and its skip. Worth having on Windows given mis-casing SKILL.md is a Windows authoring hazard specifically.

Gates from a clean lockfile sync: 5,734 passed, 14 skipped. ruff check and ruff format --check clean.

---

On the file-mode gap — yes, take it as a follow-up. Your scoping argument holds, and there is a second reason to keep it out of here: mcp/manager.py does not re-verify the mode after chmod, so on Windows the 0o700 hardening silently no-ops in production, not just in the tests. Fixing the assertions without first deciding what confidentiality should mean there would just relocate the gap. That is its own change.

---

Three things I want to call out from re-reviewing, because they are the reason this took one round instead of three.

You were too hard on your own tests. You flagged that the two probe-set tests fail against the pre-fix code "partly for a mechanical reason" since the old code used platform.system(). I mutation-tested that: reverting only the fallbacks tuple while keeping sys.platform still fails test_windows_skips_only_the_driveless_path cleanly, with the POSIX one passing. They isolate the bug properly.

Pushing back on my registry suggestion was correct. The charset check alone would have let my.team through, and a nested table makes the entry vanish silently on load — worse than the parse error, and I had not spotted it.

Not taking any of the three workarounds was the right call. "A CI leg you have to read past is worse than no CI leg" is exactly right, and the cache-escape bug underneath was worth finding — a file:// checkout landing outside the cache root is the same escape the .. check exists to stop, through a door it did not cover. I verified the fix independently: hostile inputs still refused including the backslash form, so the fold-then-check ordering holds, and owner-segment collisions stay distinct via the leaf digest.

Approving once CI comes back green.

[Jason Robert (jrob5756)]

Resolved the conflict and got the Windows leg green. MERGEABLE / CLEAN, all 11 checks passing.

The conflict was CHANGELOG.md only — both sides had inserted at the top of Unreleased > Fixed. Everything else auto-merged, including validate_connection, which I checked line by line afterwards. I kept both blocks in place and verified the result is the exact union of the two sides: 99 entries, none dropped from either.

Two test failures on the Windows leg after the merge, both from test_markup_injection.py, which arrived with #408.

The first is the same bug class this branch already fixed, one layer out. The fixture interpolated tmp_path into a YAML double-quoted scalar, which processes backslash escapes — so the runner's C:\Users\runneradmin\... reached the parser as \U and it died with "expected escape sequence of 8 hexdecimal numbers, but found 's'" before the assertion under test ever ran. Same shape as _format_toml: a value with meaning to the serialiser, interpolated as though it were inert. json.dumps fixes it, since YAML is a superset of JSON. Verified against the literal path from the failing run — old form raises ScannerError, new form round-trips exactly.

The second one I want to flag, because the honest answer was a skip and I would rather say why than have it look like giving up. The test puts a rich markup marker inside a directory name, and the CRASHING marker is [/etc/x]. On Windows / is a path separator, so that becomes three nested components — plug[, etc, x] — and the literal string the assertion looks for cannot appear in the path at all. Rich's crashing form is a closing tag, so it needs the slash; there is no Windows-expressible variant to substitute. That makes it a scenario the platform cannot host rather than an assertion to loosen.

I scoped the skip to the one parametrized case rather than the test, so [dim] still runs there — and the crashing form keeps its coverage anyway, since ten other cases in that module put it through error text, agent names, descriptions and manifest messages, none of which touch the filesystem.

That is the fourth and fifth real cross-platform bug this leg has surfaced, after _format_toml, the Claude CLI probe, and the plugin cache key.

Windows: 6,258 passed, 58 skipped. Linux: 6,302 passed, 14 skipped. Lint, format and typecheck clean.

I also ran the removal step against a genuinely fresh uv sync rather than trusting my local venv, since I had already deleted the binary while testing the failure path: present removes and exits 0. Incidentally it resynced as 3.13 rather than 3.12 and the single pattern still matched, which is a nicer proof that it is interpreter-version agnostic than the one I wrote in the commit message.

Nothing outstanding from my side. Approving now — the file-mode gap is yours to take as the follow-up we agreed.

[Jason Robert (jrob5756)]

LGTM. Approved!