test(windows): pin the real-process branch and the Arguments binding contract (#512) - #588
Closed
localai-bot wants to merge 4 commits into
Closed
test(windows): pin the real-process branch and the Arguments binding contract (#512)#588localai-bot wants to merge 4 commits into
localai-bot wants to merge 4 commits into
Conversation
…o-arg test uses (#512) `windows-msvc-cpu` has failed on every pull request, ~21 minutes in, with zero compile diagnostics: Cannot bind argument to parameter 'Arguments' because it is an empty array. `Invoke-Checked` declared `Arguments` as `[Parameter(Mandatory)][string[]]`. PowerShell's `Mandatory` validation treats an empty collection as "not supplied", so every call that runs a test executable taking no arguments died at parameter binding before the process was ever started. There are six such production call sites, not four -- the two forced-CPU-tier invocations (`VT_CPU_MATMUL_TIER=portable` / `avx2`) are on the same path and would have failed next. The fix is `[AllowEmptyCollection()]` alongside `Mandatory`, not a `= @()` default. The existing intent is that a caller must state its argument list; `AllowEmptyCollection` keeps the omission an error while permitting an explicitly empty list, whereas a default would silently accept a call that forgot the parameter entirely. The contract step ran green all along because the suite never executed an empty-argument invocation, which is why a 21-minute build step caught what a seconds-long contract step should have. `Invoke-CheckedContractTests` closes that: it injects a recording runner -- mirroring the `DumpbinRunner` and unsupported-tier-probe seams already in this file -- and asserts the empty and non-empty argument lists are forwarded verbatim and that a nonzero status still throws on both. `Invoke-Checked` gains the matching optional `-Runner` seam. RED-first, run under pwsh 7.6.4: the new contract test fails with the exact CI message above before `[AllowEmptyCollection()]` is applied, and passes after. Five mutations of the claimed guarantees are each caught by their intended assertion (drop `AllowEmptyCollection`; swallow the argument list; never throw on nonzero; forward the wrong program; truncate the arguments), and the real `& $Program @Arguments` path -- not just the fake runner -- was exercised against `/bin/true`, `/bin/echo a b`, and `/bin/false`. The pre-existing unsupported-tier contract assertion is untouched and proven still non-vacuous: making the probe send an empty, a wrong, or a two-element argument list each still trips "did not receive one exact filter argument". That empty-args mutation also shows the fake-runner scriptblock parameters need no `AllowEmptyCollection` of their own -- being non-mandatory, they bind `@()` and fail on the assertion rather than on binding. This is a distinct defect from #514, the POSIX `setenv`/`unsetenv` C3861 error that fails `windows-msvc-vulkan`; that one is fixed on its own branch and `windows-msvc-vulkan` stays red here until it lands. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…#512) FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…contract (#512) A fresh review of #583 found two guarantees that nothing pins. Both are tightening, not repair: the behavior is already correct on the PR head, and this change only makes an edit that breaks it go red. The real-process branch of `Invoke-Checked` -- `$null -eq $Runner`, the one the release gate actually runs -- had no coverage at all, because a fake runner never executes `& $Program @Arguments`. Setting `$exitCode = 0` unconditionally there survived the whole contract suite, which is the failure class this repo keeps paying for: a Windows gate reporting success for tests that failed. It now drives the real branch for a success, an expected throw carrying the child's own nonzero status, argv distinctness, and an explicitly empty argument list. The program it drives is the PowerShell host executing the script, resolved from the running process. That is the one executable guaranteed to exist wherever this script can run, so the identical assertions execute on the Windows runners and on POSIX developer boxes. A `cmd.exe`/`/bin/sh` pair or a platform guard would give the Windows and POSIX runners different arms, and an arm that silently no-ops on one platform is its own version of the bug being fixed here. `Arguments` is `Mandatory` + `[AllowEmptyCollection()]` rather than defaulted to `@()` precisely so that omission stays a hard error while an explicitly empty list binds -- the argument the fix form rests on. Dropping `Mandatory` and adding `[AllowNull()]` both survived. An explicit `$null` now has to be rejected, and so does omission. Omission is asserted in an API runspace rather than in-process. An omitted mandatory parameter PROMPTS in an interactive console host: asserted directly, it hangs a developer's terminal on `Arguments[0]:` forever, and only reaches the binding error in CI where stdin is not a tty. A runspace host cannot prompt and reports the error instead, so the assertion means the same thing in both places. The function under test is rebuilt from the live definition's own source text, so it tracks any edit to the real parameter block. Mutation results, contract suite only, `pwsh` 7.6.4 on Linux. All eleven red, against four that survived on the #583 head (3768bd3): drop AllowEmptyCollection RED (was RED) runner forwards @() instead of $Arguments RED (was RED) exit-status check -> if ($false) RED (was RED) runner forwards "wrong.exe" RED (was RED) runner forwards only the first element RED (was RED) invert the $null -eq $Runner guard RED (was RED) runner called with no args argument at all RED (was RED) real branch sets $exitCode = 0 RED (was GREEN) real branch drops the @ splat RED (was GREEN) drop Mandatory from Arguments RED (was GREEN) add [AllowNull()] to Arguments RED (was GREEN) One caveat is worth recording rather than glossing. For a native executable, `& $Program $Arguments` and `& $Program @Arguments` are indistinguishable -- verified identical for a `[string[]]` at 0, 1 and 3 elements under all three `$PSNativeCommandArgumentPassing` modes, including `Windows`, which is the runner default. The splat only becomes observable when the program is a PowerShell script, where an empty list otherwise arrives as one array argument instead of no arguments. That is what the empty-argument probe catches, and it is the #512 contract stated exactly. Each new arm was also shown to catch a defect on its own, so none is dead weight: joining argv red-lines the distinctness probe, truncating argv red-lines it too, and dropping `Mandatory` with the null assertion deleted still red-lines the omission arm. Verified locally: contract suite green with stdin closed and under a pty (no prompt hang), `check-windows-portability.py` rc=0, `agent-preflight.sh --staged` all green. `windows-msvc-cpu` cannot go green regardless -- #512 unmasked a `STATUS_STACK_BUFFER_OVERRUN` in `test_openai_api_server.exe` (#584), which dies before doctest prints a summary line -- and `windows-msvc-vulkan` stays red on #514. The identical missing `AllowEmptyCollection` on `Assert-CrtPolicy` is #585 and is out of scope here. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
localai-bot
changed the base branch from
row/ENG-RELEASE-WINDOWS-EMPTY-ARGS
to
main
August 13, 2026 06:16
Collaborator
Author
|
Superseded — squash-merging #583 orphaned this stacked branch ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repairs the two findings from the fresh review of #583. That review's verdict was PASS — this is a tightening pass, not a repair. The behavior was already correct on the #583 head (
3768bd31); nothing pinned it.Stacked on
row/ENG-RELEASE-WINDOWS-EMPTY-ARGS, so this diff is only the new coverage. Land #583 first; GitHub retargets this tomainautomatically.origin/main(65625f34a, spec-only) is merged in.Issue: #512.
Finding 1 (MEDIUM) — the real-process branch was unguarded
Invoke-Checked's$null -eq $Runnerbranch is the one the release gate actually runs, and it had no coverage at all: a fake runner never executes& $Program @Arguments. Setting$exitCode = 0unconditionally there survived the entire contract suite — a future edit could go green while the Windows gate reported success for failing tests.Added a real-process arm covering a success, an expected throw carrying the child's own nonzero status, argv distinctness, and an explicitly empty argument list.
Platform-portability choice
The program driven is the PowerShell host executing the script, resolved from the running process (
(Get-Process -Id $PID).Path).It is the one executable guaranteed to exist wherever this script can run — it is what is running it — so the identical arm executes on the Windows runners and on POSIX developer boxes, with no platform branch. A
cmd.exe//bin/shpair or an$IsWindowsguard would give the two platforms different arms, and an arm that silently no-ops on one platform is its own version of the bug being fixed here. It also keepscmd.exeout of the tree, whichcheck-windows-portability.pyalready treats as a smell elsewhere.Finding 2 (MEDIUM-LOW) — the property justifying the fix form was untested
ArgumentsisMandatory+[AllowEmptyCollection()]rather than defaulted to@()precisely so omission stays a hard error while an explicitly empty list binds. DroppingMandatoryand adding[AllowNull()]both survived. Both are now expected-throws.Omission is asserted in an API runspace, not in-process. An omitted mandatory parameter prompts in an interactive console host: asserted directly it hangs a developer's terminal on
Arguments[0]:forever (verified — a direct assertion had to be killed at 25s under a pty) and only reaches the binding error in CI where stdin is not a tty. A runspace host cannot prompt and reports the error instead, so the assertion means the same thing in both places. The function under test is rebuilt from the live definition's own source text, so it tracks any edit to the real parameter block.Mutation table
Contract suite only,
pwsh7.6.4 on Linux. Baseline column re-measured on the #583 head (3768bd31) with the same harness.AllowEmptyCollection@()instead of$Argumentsif ($exitCode -ne 0)→if ($false)"wrong.exe"$null -eq $Runnerguard$exitCode = 0@splatMandatoryfromArguments[AllowNull()]toArgumentsM11 is unweakened: the runner arm still distinguishes "forwarded an empty array" from "forwarded nothing".
One caveat, recorded rather than glossed
For a native executable,
& $Program $Argumentsand& $Program @Argumentsare indistinguishable — verified identical for a[string[]]at 0, 1 and 3 elements (including an element containing a space) under all three$PSNativeCommandArgumentPassingmodes:Standard,Legacy, andWindows, the runner default.The splat only becomes observable when the program is a PowerShell script, where an empty list otherwise arrives as one array argument rather than no arguments. That is what the empty-argument probe catches (
exited with status 23), and it is the #512 contract stated exactly. So the@-drop mutation is RED, but for a narrower reason than "argv got mangled", and a reviewer should know that.Every new arm earns its place
Each was shown to catch a defect on its own:
Mandatoryand delete the null-binding assertionVerified locally vs. what only CI can show
Verified locally (the whole
-ContractTestpath runs on Linux):pwsh -File scripts/build-windows-release.ps1 -ContractTest→ rc=0 with stdin closed and under a pty (no prompt hang)python3 scripts/check-windows-portability.py --root .→ rc=0scripts/agent-preflight.sh --staged→ all gates green/tmp/vllm-cpp-checked-*absent after the run)Only CI can show: that
pwshon a Windows runner resolves the host path and spawns the child identically. The design deliberately has no platform branch, so there is no Windows-only code path to be uncovered; theWindowsargument-passing mode was exercised locally by setting$PSNativeCommandArgumentPassing.Not in scope
windows-msvc-cpucannot go green: Windows release runner rejects empty process arguments #512 unmaskedSTATUS_STACK_BUFFER_OVERRUNintest_openai_api_server.exe(test_openai_api_server crashes on Windows with STATUS_STACK_BUFFER_OVERRUN (0xC0000409), unmasked by the #512 fix #584), which dies before doctest prints either summary line — absence of a summary is the signature, not a passing grep.windows-msvc-vulkanstays red on Windows Vulkan release test uses POSIX environment APIs #514.AllowEmptyCollectiononAssert-CrtPolicyis Assert-CrtPolicy has the same empty-array binding defect as #512, latent only because dumpbin always prints a banner #585.No workflow was disabled, skipped, or set
continue-on-error. Onlyscripts/build-windows-release.ps1changed (+109 / -0).🤖 Generated with Claude Code