fix(GATE-WINDOWS-WARNING-POLICY): /WX- is not /WX, and OBJCXX is not the C++ compile (#774) - #795
Merged
Merged
Conversation
…n the C/C++ compile (#774) `check-windows-portability.py:1710` asserts the policy with `token in warnings`. `"/WX" in "/WX-"` is True, and `/WX-` is MSVC's spelling for DISABLE warnings-as-errors, so the gate is blind to its own inversion. Measured on PR #640 commit `74ba3823f` (reverted in `15aa963a6` after review): it shipped `/WX-` on the CXX arm while the only bare `/WX` left was on `$<COMPILE_LANGUAGE:OBJCXX>` -- Objective-C++, the Metal backend, which never compiles under MSVC. Two more blindnesses fall out of the same `in`: `/W44996` answers for `/W4`, and `CMakeLists.txt:30`'s `#` comment satisfies the whole policy on its own. The spec fixes the shape of the repair before the code exists: token boundaries, language scoping, comment stripping, and the negating spellings of the two flags the policy names. It also decides the two sibling evasions #640 used and argues both rather than widening silently -- `COMPILE_WARNING_AS_ERROR OFF` cannot cancel a literal `/WX` and stays out; `/wd####` narrows what `/W4` reports rather than inverting it and stays out; `/W0` and `/w` are the disable spellings of `/W4` itself and come in. Spec before code, per AGENTS.md. No checker or test changes here. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
…the C++ compile (#774) `check-windows-portability.py:1710` asserted the MSVC warning policy with `token in warnings`. `"/WX" in "/WX-"` is True, and `/WX-` is MSVC's spelling for DISABLE warnings-as-errors: the gate could not see its own inversion. Nor did it ask which language the flag reaches, so a `/WX` confined to `$<COMPILE_LANGUAGE:OBJCXX>` -- the Metal backend, never compiled by MSVC -- satisfied it. And `CMakeLists.txt:30` says `/W4 /WX` in a `#` comment, which satisfied the whole policy by itself. `msvc_cxx_flag_text()` reduces the CMake text to what can reach an MSVC C/C++ translation unit: comments removed, and every generator expression whose COMPILE_LANGUAGE names only non-C/C++ languages blanked in place (spans from a `$<`/`>` stack scan, blanked rather than cut so offsets survive, as `without_set_source_properties` does). A genex naming no language at all is KEPT -- it does reach C/C++ -- so the failure direction is never "silently excluded". `has_msvc_flag()` matches with boundaries on both sides, case-sensitively because `cl` is. The assertion then requires /W4 and /WX as tokens on that text and refuses the disable spellings of the same two flags -- /WX-, /W0, /w -- as a separate error, because absent and present-and-cancelled are different repairs. `/wd####` and COMPILE_WARNING_AS_ERROR are argued OUT in the spec and pinned as accepted shapes by the inverse test. RED before, same harness both sides: the six new cases fail against the base checker `af026e524` with "AssertionError: 0 == 0 : Windows portability contract OK" -- the gate reporting OK on a tree whose C++ arm says `/WX-` -- and pass against this one. The seventh, the inverse pin, passes on both. `cmake/CompilerWarnings.cmake` is deliberately untouched: the tree already satisfies the repaired policy, and a checker repair that also moved the thing it checks could not show that. Spec: .agents/specs/windows-msvc-warning-policy-tokens.md FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
…nction since #583 (#680) `test_real_unsupported_tier_helper_is_structurally_scoped` mutates the real `build-windows-release.ps1` and asserts the checker rejects each mutation. The "fake runner call recording" case did `script.replace("$calls.Add(", "$calls.Append(", 1)`. The checker reads `$calls.Add(` in exactly one place: `good_runner`, the `$good` scriptblock inside `Invoke-UnsupportedTierContractTests` (`check-windows-portability.py:1372`). `replace(..., 1)` takes the FIRST occurrence in the file, and since `e8a9e74e1` (#512/#583) added `Invoke-CheckedContractTests` above it, that first occurrence is in a different function the checker never looks at. So the mutation left the governed text intact, the checker correctly stayed green, and the assertion read that as a defect. This is the "one fake-runner case" of #680, RED on `main` at `af026e524` -- verified on a pristine base worktree, 71 tests, 1 failure, message `AssertionError: 0 == 0 : Windows portability contract OK`. Anchor the mutation to the governed occurrence and assert that occurrence is unique rather than assuming it, which is the property that silently changed underneath this test. No assertion is deleted or widened: the same rejection is still required, and it now actually exercises the rule it names -- the mutation is rejected by BOTH the base and the current checker, so this is a fixture repair, not a semantics change. It has to land here because `check-pr-size.py`'s executable-evidence contract runs the WHOLE recognized module and requires it green at HEAD, so this stale anchor blocks every change to `scripts/check-windows-portability.py`. The second half of #680 -- that no workflow invokes this suite -- is NOT addressed here and the issue stays open for it. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
Five commits landed on main after this branch's base `af026e524`. None touch `scripts/check-windows-portability.py`, its test module, or the MSVC warning flags in `cmake/CompilerWarnings.cmake`; the only shared surface is the `.agents/roadmap_v1.md` issue table, where #777 was inserted mid-table and #774 appended, so both keys survive and no unrelated key moved. Merged so the gates run against the tree this branch will actually land on. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
Picks up #782 (GATE-FORK-ANCESTRY), which repairs the range computation in check-pr-size.py and check-commit-trailers.py. Both of this PR's red non-baseline checks -- pr-size and agent-record -- failed on "base must be an ancestor of head", which is exactly that defect and predates its fix. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] # Conflicts: # .agents/roadmap_v1.md
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
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.
Closes #774.
Spec:
.agents/specs/windows-msvc-warning-policy-tokens.mdRoadmap issue table:
.agents/roadmap_v1.md,#774/ENG-RELEASE-WINDOWS.Root cause
scripts/check-windows-portability.py:1710asserted the MSVC warning policywith a substring test:
"/WX" in "/WX-"isTrue, and/WX-is MSVC's spelling for disablewarnings-as-errors. Two more blindnesses fall out of the same
in, bothconfirmed against this tree:
"/W4" in "/W44996"isTrue—/W44996sets warning C4996 to level 4, itdoes not raise the warning level.
CMakeLists.txt:30contains the literal/W4 /WXinside a#comment, sothe prose alone satisfied the whole policy. Deleting every real flag in
cmake/CompilerWarnings.cmakewould still have passed.And the assertion never asked which language the flag reaches. The measured
demonstration is PR #640 commit
74ba3823f(reverted in15aa963a6afterreview), where the only bare
/WXleft was on$<COMPILE_LANGUAGE:OBJCXX>—Objective-C++, the Metal backend, which never compiles under MSVC — while the
arm that matters got
/WX-.RED before
Same harness both sides: the HEAD test file run against the BASE checker
(
af026e524) and against the HEAD checker, differing in nothing else.The base failure message is the point: the checker reports
Windows portability contract OKon a tree whose C++ arm says/WX-.The one base test that passes is the inverse pin — an honest tree still passes
both before and after, so this is not "stricter about everything".
The fix
msvc_cxx_flag_text()reduces the CMake text to what can reach an MSVC C/C++translation unit:
#comments removed, and every generator expression whoseCOMPILE_LANGUAGEnames only non-C/C++ languages blanked (spans from a$</>stack scan, blanked in place so offsets survive, matchingwithout_set_source_properties). A genex naming no language is kept, becauseit does reach C/C++, so the failure direction is never "silently excluded".
has_msvc_flag()matches with(?<![A-Za-z0-9_-])/(?![A-Za-z0-9_-])boundaries, case-sensitively —
cltreats/wand/W4as different flags.The assertion then requires
/W4and/WXas tokens on that text, and refusesthe disable spellings of the same two flags —
/WX-,/W0,/w— as aseparate error, because "absent" and "present and cancelled" are different
repairs.
cmake/CompilerWarnings.cmakeis deliberately not edited: the tree alreadysatisfies the repaired policy, and a checker repair that also moved the thing it
checks could not show that.
The sibling evasions #640 also used — decided, not silently widened
COMPILE_WARNING_AS_ERROR OFF: out of scope. CMake uses that propertyonly to decide whether it adds a warnings-as-errors flag; nothing about it
removes a literal flag from
target_compile_options. Under the VS generatorit lowers to MSBuild
TreatWarningAsError=false, whose/WX-precedesAdditionalOptions, and MSVC takes the last of the pair — our explicit/WXstill wins. The converse already fails closed: a tree that dropped the literal
/WXin favour of the property would go RED on the missing token. Asserting aproperty value is a new policy about spelling, which check-windows-portability's /WX assertion is satisfied by /WX-, so the gate is blind to its own inversion #774 does not describe.
/wd####: out of scope, and pinned as a deliberate non-goal bytest_accepts_the_policy_on_every_shape_that_reaches_cxx. A per-warningsuppression narrows what
/W4reports; it does not invert/W4or/WX.Refusing it wholesale would forbid the legitimate targeted suppression (the
tree has none today, so nothing is grandfathered in) and "how many is too
many" is an undecided threshold. Any future
/wdis a review judgement on itsown PR.
/W0and/w: in scope, unlike the two above, because they are thedisable spellings of
/W4itself. Closing/WX-while leaving its exact twinone character away would be the same defect check-windows-portability's /WX assertion is satisfied by /WX-, so the gate is blind to its own inversion #774 reports.
One pre-existing red had to be cleared to land this
check-pr-size.py's executable-evidence contract runs the whole recognizedmodule and requires it green at HEAD.
tests/scripts/test_check_windows_portability.pyhas been RED on
mainsincee8a9e74e1— verified on a pristine base worktreeat
af026e524: 71 tests, 1 failure. That is the "one fake-runner case" of #680,so no change to
scripts/check-windows-portability.pycould satisfy the gate.It is a stale mutation anchor, not a checker gap. The checker reads
$calls.Add(in exactly one place —good_runner, the$goodscriptblock ofInvoke-UnsupportedTierContractTests(check-windows-portability.py:1372) —and
script.replace("$calls.Add(", ..., 1)takes the first occurrence in thefile, which since #512/#583 added
Invoke-CheckedContractTestsabove it livesin a function the checker never looks at. The mutation left the governed text
intact, the checker correctly stayed green, and the assertion read that as a
defect.
The third commit anchors the mutation to the governed occurrence and asserts
that occurrence is unique. No assertion is deleted or widened, and the mutation
is now rejected by the base checker and this one — a fixture repair, not a
semantics change. #680 stays open for its other half: no workflow invokes
this suite at all.
Gates
scripts/agent-preflight.sh— green (70 ok, 0 failed).python3 scripts/check-pr-size.py --base af026e524 --head HEAD— OK. Thatis the gate's own re-execution of the red-before/green-after evidence in an
isolated worktree, so
base_failedandhead_passedare proven by the gate,not asserted by me.
python3 scripts/check-commit-trailers.py --range af026e524..HEAD— OK.python3 -m unittest tests.scripts.test_check_windows_portabilityundercheck-pr-size's exact sanitized environment — 78 tests, OK.
python3 -m pytest tests/scripts/ --ignore=tests/scripts/test_cpu_kernel_bench.py— 8 failed, 1368 passed, 3 skipped, 1541 subtests passed (18:51). All 8
are pre-existing; the same 8 reproduce on a pristine
origin/mainworktreeunder the same names.
Baseline to subtract
Run on a
/dev/shmworktree detached atorigin/main, no diff applied:test_now_renderreads.agents/specs/now-derived.md, not the spec added here.Separately,
tests/scripts/test_cpu_x86_llamacpp_floor.pyfailed on an earlierrun of this branch and on the pristine base tree (
af026e524: 2 failures,including
test_a_contended_leg_is_discarded_and_never_summarised, at loadavg~29 with many agents on the box) and passed on the run above. It is
contention-sensitive, not attributable to this change.
windows-msvc-*arePR-only and red on every PR (#584).
CPU-only; no GPU, no benchmark.