fix(windows): preserve NOMINMAX checker coordinates (#462)

Use one comment-and-literal-inert structural source view for both local directive parsing and the windows.h location. This keeps line and byte coordinates aligned while preventing string literals from masquerading as active preprocessor definitions.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true

AI-Assisted: true

Assisted-by: Codex:GPT-5 [Codex]

Author: mudler

.agents/specs/windows-msvc-central-nominmax.md

@@ -156,3 +156,15 @@ containing directive decoys must pass. Removing the central definition with no
 fallbacks must reject all five sources. The central CMake definition, all four
 source removals, `/W4 /WX`, guarded standalone fallbacks, and the release
 invocation remain unchanged.
+
+The recognized RED run reproduced both defects: the independent guarded-before
+case falsely rejected MiniMax-H3 sharded loading, filesystem offload, and the
+CPU threadpool, and raw/ordinary string decoys were falsely treated as active
+definitions. Line and block comment decoys already passed. The repair reuses
+`without_cpp_comments_and_literals`, whose space-for-byte replacement preserves
+newlines and offsets, and passes that single structural view to both the
+directive state machine and the `windows.h` search. The complete focused matrix
+then passes: guarded-before independently in all five sources, late and
+unguarded definitions rejected in all five, all four inert decoys accepted, and
+central removal without fallbacks rejected in all five. The direct production
+checker and the complete 68-test Windows portability suite pass.

scripts/check-windows-portability.py

@@ -60,11 +60,11 @@ def _has_central_msvc_nominmax(cmake: str) -> bool:
     )
 
 
-def _local_nominmax_definitions(source: str) -> list[tuple[int, bool]]:
-    """Return local NOMINMAX definition lines and whether absence-guarded."""
+def _local_nominmax_definitions(active_source: str) -> list[tuple[int, bool]]:
+    """Return active NOMINMAX definition lines and whether absence-guarded."""
     definitions: list[tuple[int, bool]] = []
     absence_guards: list[bool] = []
-    for number, line in enumerate(without_cpp_comments(source).splitlines(), 1):
+    for number, line in enumerate(active_source.splitlines(), 1):
         match = re.match(r"\s*#\s*(\w+)(.*)$", line)
         if match is None:
             continue
@@ -1726,18 +1726,19 @@ def check(root: Path, build_dir: Path | None = None,
 
     central_nominmax = _has_central_msvc_nominmax(cmake)
     for relative, source in texts.items():
-        definitions = _local_nominmax_definitions(source)
+        active_source = without_cpp_comments_and_literals(source)
+        definitions = _local_nominmax_definitions(active_source)
         for line, guarded in definitions:
             if not guarded:
                 errors.append(
                     f"{relative}:{line}: unguarded source-local NOMINMAX is forbidden"
                 )
         windows_header = re.search(
-            r"(?m)^\s*#\s*include\s*<windows\.h>", without_cpp_comments(source)
+            r"(?m)^\s*#\s*include\s*<windows\.h>", active_source
         )
         if windows_header is None or central_nominmax:
             continue
-        header_line = source.count("\n", 0, windows_header.start()) + 1
+        header_line = active_source.count("\n", 0, windows_header.start()) + 1
         if not any(guarded and line < header_line for line, guarded in definitions):
             errors.append(
                 f"{relative}: NOMINMAX must be defined centrally or by an "

tests/scripts/test_check_windows_portability.py

@@ -24,6 +24,13 @@
 UNSUPPORTED_TIER_FILTER = (
     "--test-case=elementwise CPU GEMM: the forced tier is the tier that actually ran"
 )
+NOMINMAX_REAL_CLOSURE = (
+    "src/vllm/model_executor/models/minimax_h3_sharded.cpp",
+    "src/vllm/v1/kv_offload/fs_io.cpp",
+    "src/vt/cpu/cpu_threadpool.cpp",
+    "src/vllm/platform/console_shutdown.cpp",
+    "src/vllm/platform/process.cpp",
+)
 
 
 SAFE_FILES = {
@@ -267,6 +274,27 @@ def assert_rejected(self, relative: str, content: str, reason: str) -> None:
         self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr)
         self.assertIn(reason, result.stdout + result.stderr)
 
+    @staticmethod
+    def nominmax_real_sources() -> dict[str, str]:
+        return {
+            relative: (REPO / relative).read_text(encoding="utf-8")
+            for relative in NOMINMAX_REAL_CLOSURE
+        }
+
+    @staticmethod
+    def with_nominmax_fallback(source: str, *, before: bool,
+                               guarded: bool = True) -> str:
+        header = "#include <windows.h>"
+        if source.count(header) != 1:
+            raise AssertionError("expected exactly one windows.h include")
+        definition = "#define NOMINMAX\n"
+        if guarded:
+            definition = "#ifndef NOMINMAX\n#define NOMINMAX\n#endif\n"
+        return source.replace(
+            header,
+            definition + header if before else header + "\n" + definition,
+        )
+
     def test_accepts_complete_guarded_contract(self) -> None:
         result = self.run_checker(self.make_tree())
         self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
@@ -704,6 +732,115 @@ def test_nominmax_contract_follows_central_compile_definition(self) -> None:
             "unguarded source-local NOMINMAX",
         )
 
+    def test_nominmax_real_closure_accepts_guarded_fallbacks_before_headers(
+            self) -> None:
+        cmake = textwrap.dedent(SAFE_FILES["CMakeLists.txt"])
+        without_central = cmake.replace(
+            "add_compile_definitions(NOMINMAX _CRT_SECURE_NO_WARNINGS)", ""
+        )
+        safe_fs = textwrap.dedent(
+            SAFE_FILES["src/vllm/v1/kv_offload/fs_io.cpp"]
+        )
+        real_sources = self.nominmax_real_sources()
+        for relative, source in real_sources.items():
+            with self.subTest(relative=relative):
+                result = self.run_checker(self.make_tree({
+                    "CMakeLists.txt": without_central,
+                    "src/vllm/v1/kv_offload/fs_io.cpp":
+                        self.with_nominmax_fallback(safe_fs, before=True),
+                    relative: self.with_nominmax_fallback(
+                        source, before=True
+                    ),
+                }))
+                self.assertEqual(
+                    result.returncode, 0, result.stdout + result.stderr
+                )
+
+    def test_nominmax_real_closure_rejects_late_and_unguarded_fallbacks(
+            self) -> None:
+        cmake = textwrap.dedent(SAFE_FILES["CMakeLists.txt"])
+        without_central = cmake.replace(
+            "add_compile_definitions(NOMINMAX _CRT_SECURE_NO_WARNINGS)", ""
+        )
+        real_sources = self.nominmax_real_sources()
+        all_guarded = {
+            relative: self.with_nominmax_fallback(source, before=True)
+            for relative, source in real_sources.items()
+        }
+        for relative, source in real_sources.items():
+            with self.subTest(relative=relative, shape="late"):
+                mutations = dict(all_guarded)
+                mutations[relative] = self.with_nominmax_fallback(
+                    source, before=False
+                )
+                result = self.run_checker(self.make_tree({
+                    "CMakeLists.txt": without_central,
+                    **mutations,
+                }))
+                self.assertNotEqual(
+                    result.returncode, 0, result.stdout + result.stderr
+                )
+                self.assertIn(
+                    f"{relative}: NOMINMAX must be defined centrally",
+                    result.stdout + result.stderr,
+                )
+
+            with self.subTest(relative=relative, shape="unguarded"):
+                mutations = dict(real_sources)
+                mutations[relative] = self.with_nominmax_fallback(
+                    source, before=True, guarded=False
+                )
+                result = self.run_checker(self.make_tree(mutations))
+                self.assertNotEqual(
+                    result.returncode, 0, result.stdout + result.stderr
+                )
+                self.assertRegex(
+                    result.stdout + result.stderr,
+                    rf"{re.escape(relative)}:\d+: unguarded source-local "
+                    r"NOMINMAX is forbidden",
+                )
+
+    def test_nominmax_real_closure_ignores_comment_and_literal_decoys(
+            self) -> None:
+        real_sources = self.nominmax_real_sources()
+        target = "src/vllm/platform/process.cpp"
+        source = real_sources[target]
+        decoys = {
+            "raw string": 'constexpr auto kRaw = R"TAG(\n#define NOMINMAX\n)TAG";\n',
+            "ordinary string": 'constexpr auto kString = "\\\n#define NOMINMAX";\n',
+            "line comment": "// #define NOMINMAX\n",
+            "block comment": "/*\n#define NOMINMAX\n*/\n",
+        }
+        for shape, decoy in decoys.items():
+            with self.subTest(shape=shape):
+                mutations = dict(real_sources)
+                mutations[target] = source.replace(
+                    "#include <windows.h>",
+                    decoy + "#include <windows.h>",
+                )
+                result = self.run_checker(self.make_tree(mutations))
+                self.assertEqual(
+                    result.returncode, 0, result.stdout + result.stderr
+                )
+
+    def test_nominmax_real_closure_without_contract_rejects_all_five_sources(
+            self) -> None:
+        cmake = textwrap.dedent(SAFE_FILES["CMakeLists.txt"])
+        without_central = cmake.replace(
+            "add_compile_definitions(NOMINMAX _CRT_SECURE_NO_WARNINGS)", ""
+        )
+        result = self.run_checker(self.make_tree({
+            "CMakeLists.txt": without_central,
+            **self.nominmax_real_sources(),
+        }))
+        self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr)
+        output = result.stdout + result.stderr
+        for relative in NOMINMAX_REAL_CLOSURE:
+            with self.subTest(relative=relative):
+                self.assertIn(
+                    f"{relative}: NOMINMAX must be defined centrally", output
+                )
+
     def test_pins_peer_close_invalidation(self) -> None:
         source = textwrap.dedent(
             SAFE_FILES["src/vllm/v1/kv_offload/lmcache/remote_client.cpp"]