fix(windows): launch canonicalized batch shims - #293
Conversation
717f6d2 to
4ac445c
Compare
|
Investigated the macOS and Linux Docker E2E failures from run 33819255950. They are not caused by this Windows-only patch:
I do not have permission to rerun upstream Actions jobs directly. I force-updated the one-commit branch without changing its tree ( |
|
Follow-up from the fresh run (
Because the failures occur before plugin loading and reproduce on the exact base commit, I am not adding unrelated E2E/test-timing changes to this Windows shim-launch PR. An upstream maintainer will need to rerun the failed jobs once the OpenCode startup issue clears. |
|
Agreed on the diagnosis, from our side of the wall: main's own push run (33810981315) failed the same two E2E legs on the same first-scenario shape - OpenCode 1.18.25 never reached aimock (0 turns), empty plugin log, later scenarios healthy - 40 minutes after an identical suite went green on 33806638998 with the same OpenCode build. That is a first-launch startup stall on the runner, not this patch (which is cfg(windows) and only crosses into cmd.exe at the batch launch boundary). The Windows libtest failure is ours too: tier1_worker_panic_delivers_failed_to_waiter used a 250 ms soft deadline for a test about panic delivery, not timing; fixed on main in 5258861. Review of the change itself: the boundary is the right one (canonical paths stay internal, conversion only where cmd.exe sees the path), the UNC host+share guard and the refusal to strip \?\Volume{GUID}\ are the parts I would have asked for, and the canonicalized npm-shim test exercises real cmd.exe. One note for us, not for this PR: AFT already carries two private windows_non_verbatim_path copies (inspect/job.rs, oxc_engine/resolver.rs) that strip the prefix unconditionally; your stricter form is the one they should converge on, and I will unify them in a follow-up rather than ask you to widen this patch. Merging once the E2E rerun on main confirms the stall has cleared, so the merge lands on a green base. |
Summary
cmd.exe-compatible spellings only at the shared.cmd/.batlaunch boundary\\?\...paths internally and leave native.exelaunches unchanged%characters, UNC paths, and unsupported verbatim namespacesProblem
aft-pi0.55.1 successfully installsyaml-language-server, and the LSP resolver correctly selects its Windows shim fromlsp_paths_extra:However,
lsp_paths_extradirectories are canonicalized withstd::fs::canonicalize, which yields extended-length paths on Windows. AFT then passed that\\?\...\.cmdpath unchanged tocmd.exe.cmd.exeand npm's%~dp0shim logic do not reliably handle that namespace, so the server disconnected during initialization with:A standalone
spawn("yaml-language-server")experiment also returnsENOENT, but that is not AFT's execution path: AFT already probes.cmd,.exe, and.batin configured directories before falling back to the extensionless file. Changing PATH fallback resolution would therefore not fix this failure.Why this boundary is correct
The canonical path remains useful for internal identity and filesystem operations. The compatibility conversion belongs specifically where AFT crosses into
cmd.exe:\\?\C:\...becomesC:\...\\?\UNC\server\share\...becomes\\server\share\...\\?\Volume{GUID}\...are preserved rather than accidentally converted into relative paths.exeexecution does not use this batch launcher and is unaffectedThis also fixes formatter and other npm batch shims that use the same shared launcher.
Reproduction and proof
I exercised the actual installed
yaml-language-server@1.24.0through AFT's NDJSONconfigure+lsp_inspectpath on Windows.With the released v0.55.1 binary:
With this branch, using the same project, cache directory, server package, and resolved
binary_path:The unchanged resolved path plus changed launch result isolates the failure to the
cmd.exeboundary and verifies the fix without changing resolver precedence.Tests
cargo test -p agent-file-tools --lib windows_command— 6 passedcargo test -p agent-file-tools --lib lsp::registry::tests— 42 passedcargo test -p agent-file-tools --lib lsp::client::tests— 9 passedcargo test -p agent-file-tools --lib run_external_tool_invokes_npm_style_batch_shim— passedcargo check -p agent-file-tools --lib— passedcargo fmt --check— passedA complete
cargo test -p agent-file-tools --librun reached 2,829 passing tests. Five unrelated background-registry failures caused by parallel shared-artifact collisions all passed when rerun individually; the remaining three failures require Windows symlink privileges unavailable in this environment.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes Windows batch shim launches failing with "The system cannot find the path specified." when the shim path comes from a canonicalized directory, because
cmd.exeand npm's%~dp0logic do not handle\\?\extended-length paths.Bug Fixes
\\?\C:\...and\\?\UNC\...paths tocmd.exe-compatible spellings only at the batch launch boundary..exelaunches and internal canonical paths unchanged.%, UNC paths, and unsupported verbatim namespaces.Written for commit 4ac445c. Summary will update on new commits.
Greptile Summary
This PR converts canonical Windows DOS and UNC paths into
cmd.exe-compatible spellings at the shared batch-launch boundary while preserving internal canonical paths and unsupported namespaces..cmdand.batfiles.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(windows): launch canonicalized batch..." | Re-trigger Greptile