Found while reviewing #677 (the #664 Windows fix). Not blocking that PR — it adopts an existing spelling byte-for-byte rather than inventing a seventh variant, and extracting a shared header is a strictly larger change than an emergency unblock should carry.
The state
Six copies of the same std::string → std::filesystem::path conversion, each in its own anonymous namespace:
| site |
name |
throws? |
src/vllm/v1/kv_offload/fs_io.cpp:31 |
NativePath |
no |
src/vllm/model_executor/models/minimax_h3_sharded.cpp:55 |
NativePath |
YES — three throw std::invalid_argument sites |
src/vllm/model_executor/model_loader/gguf_reader.cpp:22 |
Utf8Path |
no |
src/vllm/model_executor/model_loader/safetensors_reader.cpp:29 |
Utf8Path |
no |
src/vllm/entrypoints/openai/server_main.cpp:115 |
NativeUtf8Path |
no |
src/vllm/multimodal/video_engine.cpp:65 |
NativePath |
no |
minimax_h3_sharded.cpp:55-70 under _WIN32 calls MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, …) and throws on oversize and twice on malformed UTF-8. The other five do not.
Why it matters, beyond tidiness
The hazard is not the duplication, it is the semantic divergence. Two functions with the same name, in the same codebase, on the same seam, differ on whether a malformed-UTF-8 path escapes as an exception. That is exactly the drift AGENTS.md's "never hand-roll a parallel path" rule exists to prevent, and it already had a consequence: #677 had to reason explicitly about which NativePath to copy, and chose fs_io.cpp's precisely because minimax_h3_sharded.cpp's would have reintroduced on Windows the escaping exception the fix removes on POSIX.
A future author picking the nearest copy has no way to know that.
Related, same seam, same defect class
The reviewer also found the throwing std::filesystem overloads still reachable one file away: src/vllm/multimodal/minimax_h3_video.cpp:308 uses std::filesystem::is_directory(params.dit_path) without an error_code. Reachable only after detection has already stat'd the path, so it is TOCTOU-only today — but it is the identical defect in the sibling family loader behind the same registry. Same shape at src/vt/cuda/nvfp4_persistent_cache.cpp:677, parakeet_transcription.cpp:72, fs_io.cpp:391/409/450. Re-derive each anchor at HEAD before acting — line numbers in this tree go stale within a single PR.
What closing this looks like
One shared helper on a real seam, with one documented policy on malformed UTF-8, and the six call sites routed through it. If any site genuinely needs the throwing behaviour, that is a second named function with the reason attached, not a copy that happens to differ.
Do not assert absence from a failed grep when sweeping for further copies — enumerate the conversions that exist and diff against the list above, with a positive control in the same command.
Related: #664, #677, #680.
Found while reviewing #677 (the #664 Windows fix). Not blocking that PR — it adopts an existing spelling byte-for-byte rather than inventing a seventh variant, and extracting a shared header is a strictly larger change than an emergency unblock should carry.
The state
Six copies of the same
std::string→std::filesystem::pathconversion, each in its own anonymous namespace:src/vllm/v1/kv_offload/fs_io.cpp:31NativePathsrc/vllm/model_executor/models/minimax_h3_sharded.cpp:55NativePaththrow std::invalid_argumentsitessrc/vllm/model_executor/model_loader/gguf_reader.cpp:22Utf8Pathsrc/vllm/model_executor/model_loader/safetensors_reader.cpp:29Utf8Pathsrc/vllm/entrypoints/openai/server_main.cpp:115NativeUtf8Pathsrc/vllm/multimodal/video_engine.cpp:65NativePathminimax_h3_sharded.cpp:55-70under_WIN32callsMultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, …)and throws on oversize and twice on malformed UTF-8. The other five do not.Why it matters, beyond tidiness
The hazard is not the duplication, it is the semantic divergence. Two functions with the same name, in the same codebase, on the same seam, differ on whether a malformed-UTF-8 path escapes as an exception. That is exactly the drift AGENTS.md's "never hand-roll a parallel path" rule exists to prevent, and it already had a consequence: #677 had to reason explicitly about which
NativePathto copy, and chosefs_io.cpp's precisely becauseminimax_h3_sharded.cpp's would have reintroduced on Windows the escaping exception the fix removes on POSIX.A future author picking the nearest copy has no way to know that.
Related, same seam, same defect class
The reviewer also found the throwing
std::filesystemoverloads still reachable one file away:src/vllm/multimodal/minimax_h3_video.cpp:308usesstd::filesystem::is_directory(params.dit_path)without anerror_code. Reachable only after detection has already stat'd the path, so it is TOCTOU-only today — but it is the identical defect in the sibling family loader behind the same registry. Same shape atsrc/vt/cuda/nvfp4_persistent_cache.cpp:677,parakeet_transcription.cpp:72,fs_io.cpp:391/409/450. Re-derive each anchor at HEAD before acting — line numbers in this tree go stale within a single PR.What closing this looks like
One shared helper on a real seam, with one documented policy on malformed UTF-8, and the six call sites routed through it. If any site genuinely needs the throwing behaviour, that is a second named function with the reason attached, not a copy that happens to differ.
Do not assert absence from a failed grep when sweeping for further copies — enumerate the conversions that exist and diff against the list above, with a positive control in the same command.
Related: #664, #677, #680.