From 7c9ed6e6048ec291413f7c4a4e3b4f5c8b6de429 Mon Sep 17 00:00:00 2001 From: CJ Pais Date: Fri, 3 Jul 2026 13:19:34 +0800 Subject: [PATCH 1/3] fix utf-8 file paths on windows --- bindings/python/tests/test_utf8_paths.py | 99 ++++++++++++++++ include/transcribe.h | 8 ++ src/transcribe-bin-loader.cpp | 23 +--- src/transcribe-load-common.cpp | 5 +- src/transcribe-loader.cpp | 47 +------- src/transcribe-path.h | 67 +++++++++++ src/transcribe.cpp | 35 ++---- tests/CMakeLists.txt | 32 +++++ tests/utf8_path_unit.cpp | 141 +++++++++++++++++++++++ 9 files changed, 368 insertions(+), 89 deletions(-) create mode 100644 bindings/python/tests/test_utf8_paths.py create mode 100644 src/transcribe-path.h create mode 100644 tests/utf8_path_unit.cpp diff --git a/bindings/python/tests/test_utf8_paths.py b/bindings/python/tests/test_utf8_paths.py new file mode 100644 index 00000000..436c4c9c --- /dev/null +++ b/bindings/python/tests/test_utf8_paths.py @@ -0,0 +1,99 @@ +"""Non-ASCII (UTF-8) path handling through the C ABI, end to end. + +Regression coverage for Handy issue #1585: on Windows, model loading and +backend artifact-dir validation failed for any path containing a +non-ASCII character (e.g. ``C:\\Users\\Jerôme\\...``) because the +library's narrow ``::stat()`` / ``ifstream`` calls read the UTF-8 path +bytes in the process ANSI code page. src/transcribe-path.h now routes +all release-path file access through an explicit UTF-8 -> wide +conversion on Windows. + +The C++ counterpart (tests/utf8_path_unit.cpp) pins the same contract +white-box, but ctest only runs on Linux/macOS in CI. THIS file is what +executes on a real Windows runner — the wheel lanes run the full pytest +suite via scripts/ci/wheel_smoke.py — so it is the test that actually +exercises the conversion on the platform the bug lives on. + +Two tiers, mirroring the suite convention: + + - Model-free tests (junk bytes / missing file / init_backends) always + run. The junk-file case is the exact #1585 failure shape: the file + EXISTS at the non-ASCII path, so anything but "found and rejected + on content" is a path-encoding bug. + - The full-load test needs a real GGUF and skips when absent + (``TRANSCRIBE_SMOKE_MODEL``, set by the wheel lanes). +""" + +from __future__ import annotations + +import os +import shutil +from pathlib import Path + +import pytest + +import transcribe_cpp as t +from transcribe_cpp import errors + +# Same characters as the C++ counterpart's directory name. +NON_ASCII_DIR = "transcribe-utf8-Jerôme-日本語" + + +def _nonascii_dir(tmp_path: Path) -> Path: + d = tmp_path / NON_ASCII_DIR + try: + d.mkdir() + except OSError as e: # filesystem that cannot represent the name + pytest.skip(f"cannot create non-ASCII directory: {e}") + return d + + +def test_missing_file_in_nonascii_dir_raises_file_not_found(tmp_path): + # The not-found classification must survive the wide-path port: a + # genuinely absent file is FILE_NOT_FOUND, not some generic error. + d = _nonascii_dir(tmp_path) + with pytest.raises(t.ModelFileNotFound) as ei: + t.Model(d / "definitely-missing.gguf") + assert ei.value.status == errors.ERR_FILE_NOT_FOUND + + +def test_junk_file_in_nonascii_dir_is_found_then_rejected(tmp_path): + # THE #1585 shape: the file exists at the non-ASCII path. Pre-fix + # Windows raised ModelFileNotFound here (ANSI-code-page stat could + # not see the file). ModelLoadError — a sibling class, so this + # cannot pass via FILE_NOT_FOUND — proves the file was found, + # opened, and rejected on content. + d = _nonascii_dir(tmp_path) + junk = d / "junk.gguf" + junk.write_bytes(b"this is not a gguf file" * 64) + with pytest.raises(t.ModelLoadError): + t.Model(junk) + + +def test_smoke_model_loads_from_nonascii_dir(model_path, tmp_path): + # Full successful load through the non-ASCII path: existence + # pre-check, magic sniff, gguf parse, and the tensor-data streaming + # reopen all consume the path. Constructing the Model is the + # assertion. + d = _nonascii_dir(tmp_path) + local = d / model_path.name + shutil.copyfile(model_path, local) + with t.Model(local): + pass + + +def test_init_backends_nonascii_dirs(tmp_path): + # Multiple init_backends calls are tolerated (test_backends.py + # already relies on this post-import). The existing non-ASCII dir + # must not be misread as absent; FILE_NOT_FOUND specifically is the + # #1585 encoding bug ("...is not an existing directory"). We do not + # assert OK: a module-less dir may legitimately report ERR_BACKEND + # depending on build posture. + lib = t._lib + d = _nonascii_dir(tmp_path) + st = lib.transcribe_init_backends(os.fspath(d).encode("utf-8")) + assert st != errors.ERR_FILE_NOT_FOUND + + missing = d / "missing-subdir" + st = lib.transcribe_init_backends(os.fspath(missing).encode("utf-8")) + assert st == errors.ERR_FILE_NOT_FOUND diff --git a/include/transcribe.h b/include/transcribe.h index 15bf1d67..ea0f27d3 100644 --- a/include/transcribe.h +++ b/include/transcribe.h @@ -719,6 +719,9 @@ typedef enum { * is automatic under the supported usage (call once, before the first * model load). * + * artifact_dir is UTF-8, like every path crossing this API; on Windows + * it is converted to a wide path internally. + * * Returns: * TRANSCRIBE_ERR_INVALID_ARG artifact_dir is NULL or empty. * TRANSCRIBE_ERR_FILE_NOT_FOUND artifact_dir is not an existing directory. @@ -1335,6 +1338,11 @@ TRANSCRIBE_API const char * transcribe_model_meta_val_str(const struct transcrib /* * Load a GGUF model from disk. * + * path is UTF-8, like every path crossing this API. On Windows it is + * converted to a wide path internally, so paths containing non-ASCII + * characters (e.g. a user profile such as C:\Users\Jerôme\...) load + * correctly regardless of the process ANSI code page. + * * params may be NULL for all library defaults. To customize, initialize * a struct with transcribe_model_load_params_init() and set fields. A * struct with struct_size == 0 (including `{0}`) is rejected with diff --git a/src/transcribe-bin-loader.cpp b/src/transcribe-bin-loader.cpp index e2a40b42..bd95d43e 100644 --- a/src/transcribe-bin-loader.cpp +++ b/src/transcribe-bin-loader.cpp @@ -5,10 +5,8 @@ #include "ggml-backend.h" #include "ggml.h" #include "transcribe-log.h" +#include "transcribe-path.h" -#include - -#include #include #include #include @@ -20,21 +18,6 @@ namespace { constexpr const char * kTag = "whisper-bin"; -// Same path-existence check transcribe-loader uses for GGUFs: ENOENT / -// ENOTDIR are conclusive "file is not at this path"; anything else -// means we couldn't reach the path and the open below should surface -// the underlying error. -bool path_is_present(const char * path) { - struct stat st{}; - if (::stat(path, &st) == 0) { - return true; - } - if (errno == ENOENT || errno == ENOTDIR) { - return false; - } - return true; -} - // Read sizeof(T) bytes into `out`. Returns false on short read or // stream failure. template bool read_pod(std::ifstream & fin, T & out) { @@ -158,7 +141,7 @@ transcribe_status parse_whisper_bin(const char * path, WhisperBinModel & out) { } out.path = path; - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to open %s", kTag, path); return TRANSCRIBE_ERR_GGUF; @@ -395,7 +378,7 @@ transcribe_status parse_whisper_bin(const char * path, WhisperBinModel & out) { transcribe_status stream_tensor_data_from_bin(const std::string & path, const std::vector & slots, const char * error_tag) { - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to reopen %s for tensor data", error_tag, path.c_str()); return TRANSCRIBE_ERR_GGUF; diff --git a/src/transcribe-load-common.cpp b/src/transcribe-load-common.cpp index fbaf1be5..f1205905 100644 --- a/src/transcribe-load-common.cpp +++ b/src/transcribe-load-common.cpp @@ -11,6 +11,7 @@ #include "gguf.h" #include "transcribe-backend.h" #include "transcribe-log.h" +#include "transcribe-path.h" #include #include @@ -382,7 +383,7 @@ transcribe_status stream_tensor_data(const std::string & path, // time: ifstream::seekg takes a streamoff (signed 64-bit on every // platform we target), so multi-GB tensor offsets work without // #ifdef'ing fseeko vs _fseeki64. - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to reopen %s for tensor data", error_tag, path.c_str()); return TRANSCRIBE_ERR_GGUF; @@ -574,7 +575,7 @@ ReadF32Result read_f32_tensor_checked(gguf_context * gguf_ctx, const size_t data_off = gguf_get_data_offset(gguf_ctx); const size_t t_off = gguf_get_tensor_offset(gguf_ctx, idx); - std::ifstream fin(gguf_path, std::ios::binary); + std::ifstream fin(path_from_utf8(gguf_path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to open %s for tensor \"%s\"", error_tag, gguf_path.c_str(), tensor_name); diff --git a/src/transcribe-loader.cpp b/src/transcribe-loader.cpp index 6973783f..b3c74887 100644 --- a/src/transcribe-loader.cpp +++ b/src/transcribe-loader.cpp @@ -9,10 +9,7 @@ #include "gguf.h" #include "transcribe-meta.h" - -#include - -#include +#include "transcribe-path.h" namespace transcribe { @@ -29,44 +26,6 @@ gguf_context * Loader::release_gguf() { return out; } -namespace { - -// Narrow stat-based pre-check whose ONLY job is to distinguish -// "the file is not at this path" from every other reason -// gguf_init_from_file might return nullptr. -// -// We deliberately do not check S_ISREG here. If stat() succeeds and the -// path points at a directory (or a fifo, or a socket), the file IS at -// that path — it's just not a usable GGUF. fopen() inside ggml will -// return EISDIR / etc. and we will surface that as TRANSCRIBE_ERR_GGUF, -// which is strictly more accurate than collapsing it into FILE_NOT_FOUND. -// -// We also deliberately only short-circuit on ENOENT / ENOTDIR. Other -// stat() failures (EACCES, ENAMETOOLONG, ELOOP, EIO, ...) do NOT -// conclusively prove the file is missing — they prove we can't reach -// it. Returning FILE_NOT_FOUND for those would be wrong. We let -// gguf_init_from_file try and surface whatever it returns as ERR_GGUF. -// -// Returns true if the path either exists OR if we cannot prove it -// doesn't (in which case the caller should hand off to -// gguf_init_from_file unchanged). -bool path_is_present(const char * path) { - struct stat st{}; - if (::stat(path, &st) == 0) { - return true; - } - // ENOENT: the named file does not exist. - // ENOTDIR: a non-final component of the path is not a directory, - // which is the same observable failure mode as "the file - // is not at this path." - if (errno == ENOENT || errno == ENOTDIR) { - return false; - } - return true; -} - -} // namespace - transcribe_status Loader::open(const char * path) { if (path == nullptr) { return TRANSCRIBE_ERR_INVALID_ARG; @@ -74,6 +33,10 @@ transcribe_status Loader::open(const char * path) { path_ = path; + // Pre-check whose ONLY job is to distinguish "the file is not at + // this path" (-> FILE_NOT_FOUND) from every other reason + // gguf_init_from_file might return nullptr (-> ERR_GGUF). See + // transcribe-path.h for the exact semantics. if (!path_is_present(path)) { return TRANSCRIBE_ERR_FILE_NOT_FOUND; } diff --git a/src/transcribe-path.h b/src/transcribe-path.h new file mode 100644 index 00000000..a38624d1 --- /dev/null +++ b/src/transcribe-path.h @@ -0,0 +1,67 @@ +// transcribe-path.h - UTF-8 path handling shared by every file-access site. +// +// Contract: every path crossing the public C ABI is UTF-8 (see +// include/transcribe.h; that is what the FFI bindings send, and what +// path_for_c_api() in transcribe.cpp produces). On POSIX the narrow +// char* file APIs consume UTF-8 bytes directly, so plain std::string +// paths work. On Windows the narrow APIs (::stat, fopen, +// ifstream(const std::string &)) interpret narrow strings in the +// process ANSI code page, NOT UTF-8 — a path containing e.g. "ô" +// fails with ENOENT even though the file exists (Handy issue #1585). +// Every stat/open on the release model-load path must therefore go +// through the helpers here, which route file access through +// std::filesystem::path with an explicit UTF-8 -> native-wide +// conversion on Windows. +// +// Deliberately NOT converted: dev-only I/O — the dump writers in +// transcribe-debug.cpp and the TRANSCRIBE_ENABLE_VALIDATION_HOOKS +// sidecar readers in src/arch/*/model.cpp. Those read/write paths that +// never cross the public ABI. Don't copy file-access idioms from those +// files into release-path code; anything a user-supplied path reaches +// goes through these helpers. + +#pragma once + +#include +#include +#include + +namespace transcribe { + +// Build a std::filesystem::path from UTF-8 bytes. On Windows this +// converts to the native wide encoding; elsewhere the narrow encoding +// IS UTF-8 and this is a passthrough. (fs::u8path is the C++17 +// spelling; it is deprecated-but-present under C++20.) +inline std::filesystem::path path_from_utf8(const char * utf8) { +#if defined(_WIN32) + return std::filesystem::u8path(utf8); +#else + return std::filesystem::path(utf8); +#endif +} + +inline std::filesystem::path path_from_utf8(const std::string & utf8) { + return path_from_utf8(utf8.c_str()); +} + +// Existence pre-check whose ONLY job is to distinguish "the file is +// not at this path" from every other reason a subsequent open might +// fail. +// +// We deliberately do not check for a regular file. If the path points +// at a directory (or a fifo, or a socket), the file IS at that path — +// it's just not usable, and the caller's open will surface that as a +// more accurate error than FILE_NOT_FOUND. +// +// We also deliberately only report absence on the not-found outcome +// (POSIX ENOENT / ENOTDIR and their Windows equivalents, which +// fs::status maps to file_type::not_found). Other failures (EACCES, +// ENAMETOOLONG, ELOOP, EIO, ...) do NOT conclusively prove the file +// is missing — they prove we can't reach it — so they return true and +// the caller's open reports the real error. +inline bool path_is_present(const char * utf8_path) { + std::error_code ec; + return std::filesystem::status(path_from_utf8(utf8_path), ec).type() != std::filesystem::file_type::not_found; +} + +} // namespace transcribe diff --git a/src/transcribe.cpp b/src/transcribe.cpp index 9b203c03..6e254815 100644 --- a/src/transcribe.cpp +++ b/src/transcribe.cpp @@ -24,6 +24,7 @@ #include "transcribe-loader.h" #include "transcribe-log.h" #include "transcribe-model.h" +#include "transcribe-path.h" #include "transcribe-session.h" #include "transcribe-tokenizer.h" #include "transcribe/whisper.h" @@ -40,17 +41,8 @@ # include #endif -#include - -// MSVC's has the _S_IFDIR mode bits but not the POSIX -// S_ISDIR() macro. -#if defined(_WIN32) && !defined(S_ISDIR) -# define S_ISDIR(m) (((m) & _S_IFMT) == _S_IFDIR) -#endif - #include #include -#include #include #include #include @@ -864,8 +856,8 @@ static transcribe_status transcribe_init_backends_impl(const char * artifact_dir return TRANSCRIBE_ERR_INVALID_ARG; } { - struct stat st{}; - if (::stat(artifact_dir, &st) != 0 || !S_ISDIR(st.st_mode)) { + std::error_code ec; + if (!std::filesystem::is_directory(transcribe::path_from_utf8(artifact_dir), ec)) { transcribe::log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "transcribe_init_backends: %s is not an existing directory", artifact_dir); return TRANSCRIBE_ERR_FILE_NOT_FOUND; @@ -880,8 +872,8 @@ static transcribe_status transcribe_init_backends_impl(const char * artifact_dir std::lock_guard lock(s_mutex); std::error_code ec; - std::filesystem::path canonical = std::filesystem::weakly_canonical(artifact_dir, ec); - const std::string key = ec ? std::string(artifact_dir) : canonical.string(); + std::filesystem::path canonical = std::filesystem::weakly_canonical(transcribe::path_from_utf8(artifact_dir), ec); + const std::string key = ec ? std::string(artifact_dir) : canonical.u8string(); if (s_loaded_dirs.find(key) == s_loaded_dirs.end()) { ggml_backend_load_all_from_path(artifact_dir); s_loaded_dirs.insert(key); @@ -1419,22 +1411,15 @@ static transcribe_status transcribe_model_load_file_impl(const char * // which HF/users mislabel): GGUF magic 0x46554747 is the canonical path, // ggml magic 0x67676d6c is a legacy whisper.cpp .bin. Public contract: // missing path -> ERR_FILE_NOT_FOUND; every other failure -> ERR_GGUF. - { - struct stat st{}; - if (::stat(path, &st) != 0) { - if (errno == ENOENT || errno == ENOTDIR) { - return TRANSCRIBE_ERR_FILE_NOT_FOUND; - } - // Other stat() failures (EACCES, etc.) are not "not found"; - // the open below will surface them as ERR_GGUF. - } + if (!transcribe::path_is_present(path)) { + return TRANSCRIBE_ERR_FILE_NOT_FOUND; } uint32_t magic = 0; { - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(transcribe::path_from_utf8(path), std::ios::binary); if (!fin) { - // stat() said the path is reachable but ifstream couldn't - // open it — permissions / type issue. Treat as a generic + // The existence pre-check said the path is reachable but + // the open failed — permissions / type issue. Treat as a generic // load failure rather than FILE_NOT_FOUND. return TRANSCRIBE_ERR_GGUF; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ce23f4ef..9287ef4c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -392,6 +392,38 @@ if(TRANSCRIBE_UV) SKIP_RETURN_CODE 77) endif() +# ----------------------------------------------------------------------------- +# Non-ASCII (UTF-8) path handling through the public C ABI +# ----------------------------------------------------------------------------- +# +# Regression test for Handy issue #1585: on Windows, paths containing +# non-ASCII characters (e.g. C:\Users\Jerôme\...) failed model load and +# backend-dir validation because narrow stat()/ifstream calls read the +# UTF-8 bytes in the ANSI code page. Copies two fixtures into a +# non-ASCII-named temp directory and drives transcribe_model_load_file / +# transcribe_init_backends through it. On POSIX this pins the UTF-8 +# contract; on Windows it exercises the wide-path conversion in +# src/transcribe-path.h. Public-API only. Fixture-dependent, so gated +# on uv like loader_smoke. + +if(TRANSCRIBE_UV) + add_executable(transcribe_utf8_path_unit + utf8_path_unit.cpp) + + target_link_libraries(transcribe_utf8_path_unit PRIVATE transcribe) + + target_compile_definitions(transcribe_utf8_path_unit PRIVATE + "TRANSCRIBE_TEST_FIXTURES_DIR=\"${CMAKE_CURRENT_SOURCE_DIR}/fixtures\"") + + transcribe_apply_warnings(transcribe_utf8_path_unit) + + add_dependencies(transcribe_utf8_path_unit fixtures) + + add_test(NAME transcribe_utf8_path_unit COMMAND transcribe_utf8_path_unit) + set_tests_properties(transcribe_utf8_path_unit PROPERTIES + SKIP_RETURN_CODE 77) +endif() + # ----------------------------------------------------------------------------- # Tokenizer ingest smoke (success path through the public ABI) # ----------------------------------------------------------------------------- diff --git a/tests/utf8_path_unit.cpp b/tests/utf8_path_unit.cpp new file mode 100644 index 00000000..b7687367 --- /dev/null +++ b/tests/utf8_path_unit.cpp @@ -0,0 +1,141 @@ +// utf8_path_unit.cpp - non-ASCII (UTF-8) path handling through the +// public C ABI. +// +// Regression test for Handy issue #1585: on Windows, model loading and +// backend artifact-dir validation failed for any path containing a +// non-ASCII character (e.g. C:\Users\Jerôme\...), because the library's +// ::stat() / ifstream(const std::string &) calls interpreted the UTF-8 +// path bytes in the process ANSI code page. The contract +// (include/transcribe.h) is that every path crossing the C ABI is +// UTF-8; src/transcribe-path.h now routes all file access through an +// explicit UTF-8 -> wide conversion on Windows. +// +// The test copies two build-time fixtures into a directory whose name +// contains non-ASCII characters and asserts, via the public API only: +// +// 1. A complete minimal model at the non-ASCII path loads with +// TRANSCRIBE_OK — covering the existence pre-check, the +// magic-bytes sniff, gguf_init_from_file, and the tensor-data +// streaming reopen. +// 2. arch_unknown.gguf at the non-ASCII path returns +// ERR_UNSUPPORTED_ARCH, not FILE_NOT_FOUND: the file was found +// and parsed; only the architecture is unknown. +// 3. A missing file inside the non-ASCII directory still returns +// FILE_NOT_FOUND — the not-found distinction survives the +// std::filesystem port. +// 4. transcribe_init_backends accepts the existing non-ASCII +// directory (anything but FILE_NOT_FOUND) and still rejects a +// missing non-ASCII directory with FILE_NOT_FOUND. +// +// On POSIX, narrow paths are UTF-8 natively, so cases 1-4 pin the +// contract; on Windows they exercise the actual conversion. +// +// The directory name is spelled in escaped UTF-8 bytes, not literal +// characters, so the test is immune to source-charset differences +// across compilers ("Jerôme-日本語"). + +#include "transcribe.h" + +#include +#include +#include +#include +#include + +#ifndef TRANSCRIBE_TEST_FIXTURES_DIR +# error "TRANSCRIBE_TEST_FIXTURES_DIR must be defined by the build system" +#endif + +namespace fs = std::filesystem; + +namespace { + +// "transcribe-utf8-Jerôme-日本語" in escaped UTF-8 bytes. +constexpr const char * kDirName = + "transcribe-utf8-Jer\xC3\xB4me-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; + +int g_failures = 0; + +#define CHECK(cond) \ + do { \ + if (!(cond)) { \ + std::fprintf(stderr, "FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + ++g_failures; \ + } \ + } while (0) + +void check_load(const std::string & utf8_path, transcribe_status expected, const char * label) { + transcribe_model_load_params mp; + transcribe_model_load_params_init(&mp); + struct transcribe_model * m = nullptr; + + const transcribe_status st = transcribe_model_load_file(utf8_path.c_str(), &mp, &m); + + if (st != expected) { + std::fprintf(stderr, "FAIL %s: expected %s, got %s\n", label, transcribe_status_string(expected), + transcribe_status_string(st)); + ++g_failures; + } + transcribe_model_free(m); +} + +} // namespace + +int main() { + const fs::path fixtures = fs::u8path(TRANSCRIBE_TEST_FIXTURES_DIR); + const fs::path src_ok = fixtures / "tokenizer_minimal.gguf"; + const fs::path src_bad = fixtures / "arch_unknown.gguf"; + + // Backstop, same rationale as loader_smoke: the build dependency on + // the `fixtures` target guarantees these exist; skip cleanly (77) + // if someone deleted them between build and test. + std::error_code ec; + if (!fs::exists(src_ok, ec) || !fs::exists(src_bad, ec)) { + std::fprintf(stderr, + "utf8_path_unit: fixtures missing; regenerate with:\n" + " cmake --build build --target fixtures\n"); + return 77; + } + + const fs::path dir = fs::temp_directory_path() / fs::u8path(kDirName); + fs::remove_all(dir, ec); // leftovers from a previous crashed run + fs::create_directories(dir); + fs::copy_file(src_ok, dir / "tokenizer_minimal.gguf", fs::copy_options::overwrite_existing); + fs::copy_file(src_bad, dir / "arch_unknown.gguf", fs::copy_options::overwrite_existing); + + // C-API strings must be UTF-8 regardless of platform; u8string() + // guarantees that on Windows where string() would be ANSI. + const std::string dir_u8 = dir.u8string(); + + // 1. Full successful load through the non-ASCII path. This is the + // exact failure mode from Handy #1585 (FILE_NOT_FOUND despite the + // file existing). + check_load(dir_u8 + "/tokenizer_minimal.gguf", TRANSCRIBE_OK, "minimal-model"); + + // 2. The file must be FOUND and parsed; only its arch is unknown. + // A path-encoding regression turns this into FILE_NOT_FOUND. + check_load(dir_u8 + "/arch_unknown.gguf", TRANSCRIBE_ERR_UNSUPPORTED_ARCH, "unknown-arch"); + + // 3. Genuinely missing file inside the non-ASCII dir: the not-found + // classification must survive. + check_load(dir_u8 + "/__missing__.gguf", TRANSCRIBE_ERR_FILE_NOT_FOUND, "missing-file"); + + // 4. Backend artifact-dir validation. The directory exists, so + // FILE_NOT_FOUND specifically is a path-encoding bug ("...is not + // an existing directory" from Handy #1585). We do not assert OK: + // a dynamic-backend build pointed at a module-less directory + // legitimately returns ERR_BACKEND. + const transcribe_status init_st = transcribe_init_backends(dir_u8.c_str()); + CHECK(init_st != TRANSCRIBE_ERR_FILE_NOT_FOUND); + + CHECK(transcribe_init_backends((dir_u8 + "/__missing_dir__").c_str()) == TRANSCRIBE_ERR_FILE_NOT_FOUND); + + fs::remove_all(dir, ec); + + if (g_failures > 0) { + std::fprintf(stderr, "utf8_path_unit: %d failures\n", g_failures); + return EXIT_FAILURE; + } + std::fprintf(stdout, "utf8_path_unit: ok\n"); + return EXIT_SUCCESS; +} From 92dc08df79cddab3f624517c153316029f51c81a Mon Sep 17 00:00:00 2001 From: CJ Pais Date: Fri, 3 Jul 2026 13:35:58 +0800 Subject: [PATCH 2/3] clean up comments --- bindings/python/tests/test_utf8_paths.py | 56 ++++++++++-------------- include/transcribe.h | 5 +-- src/transcribe-loader.cpp | 7 ++- src/transcribe-path.h | 46 ++++++++----------- tests/CMakeLists.txt | 17 ++++--- tests/utf8_path_unit.cpp | 19 ++++---- 6 files changed, 62 insertions(+), 88 deletions(-) diff --git a/bindings/python/tests/test_utf8_paths.py b/bindings/python/tests/test_utf8_paths.py index 436c4c9c..ea09bc44 100644 --- a/bindings/python/tests/test_utf8_paths.py +++ b/bindings/python/tests/test_utf8_paths.py @@ -1,27 +1,18 @@ -"""Non-ASCII (UTF-8) path handling through the C ABI, end to end. - -Regression coverage for Handy issue #1585: on Windows, model loading and -backend artifact-dir validation failed for any path containing a -non-ASCII character (e.g. ``C:\\Users\\Jerôme\\...``) because the -library's narrow ``::stat()`` / ``ifstream`` calls read the UTF-8 path -bytes in the process ANSI code page. src/transcribe-path.h now routes -all release-path file access through an explicit UTF-8 -> wide -conversion on Windows. - -The C++ counterpart (tests/utf8_path_unit.cpp) pins the same contract -white-box, but ctest only runs on Linux/macOS in CI. THIS file is what -executes on a real Windows runner — the wheel lanes run the full pytest -suite via scripts/ci/wheel_smoke.py — so it is the test that actually -exercises the conversion on the platform the bug lives on. - -Two tiers, mirroring the suite convention: - - - Model-free tests (junk bytes / missing file / init_backends) always - run. The junk-file case is the exact #1585 failure shape: the file - EXISTS at the non-ASCII path, so anything but "found and rejected - on content" is a path-encoding bug. - - The full-load test needs a real GGUF and skips when absent - (``TRANSCRIBE_SMOKE_MODEL``, set by the wheel lanes). +"""Non-ASCII (UTF-8) path handling through the C ABI. + +Regression coverage for Handy issue #1585: on Windows, model loading +and backend artifact-dir validation failed for non-ASCII paths because +the library's narrow ``::stat()`` / ``ifstream`` calls read the UTF-8 +path bytes in the process ANSI code page. src/transcribe-path.h now +converts UTF-8 to wide paths on Windows. + +The C++ counterpart (tests/utf8_path_unit.cpp) pins the same contract, +but ctest only runs on Linux/macOS in CI — this file is what executes +on a real Windows runner (the wheel lanes run the pytest suite via +scripts/ci/wheel_smoke.py). + +Model-free tests always run; the full-load test skips without a model +(``TRANSCRIBE_SMOKE_MODEL``, set by the wheel lanes). """ from __future__ import annotations @@ -36,7 +27,7 @@ from transcribe_cpp import errors # Same characters as the C++ counterpart's directory name. -NON_ASCII_DIR = "transcribe-utf8-Jerôme-日本語" +NON_ASCII_DIR = "transcribe-utf8-café-日本語" def _nonascii_dir(tmp_path: Path) -> Path: @@ -58,11 +49,10 @@ def test_missing_file_in_nonascii_dir_raises_file_not_found(tmp_path): def test_junk_file_in_nonascii_dir_is_found_then_rejected(tmp_path): - # THE #1585 shape: the file exists at the non-ASCII path. Pre-fix - # Windows raised ModelFileNotFound here (ANSI-code-page stat could - # not see the file). ModelLoadError — a sibling class, so this - # cannot pass via FILE_NOT_FOUND — proves the file was found, - # opened, and rejected on content. + # The #1585 failure shape: the file EXISTS at the non-ASCII path, + # but pre-fix Windows raised ModelFileNotFound (the ANSI-code-page + # stat could not see it). ModelLoadError is a sibling class, so + # this cannot pass via FILE_NOT_FOUND. d = _nonascii_dir(tmp_path) junk = d / "junk.gguf" junk.write_bytes(b"this is not a gguf file" * 64) @@ -83,10 +73,8 @@ def test_smoke_model_loads_from_nonascii_dir(model_path, tmp_path): def test_init_backends_nonascii_dirs(tmp_path): - # Multiple init_backends calls are tolerated (test_backends.py - # already relies on this post-import). The existing non-ASCII dir - # must not be misread as absent; FILE_NOT_FOUND specifically is the - # #1585 encoding bug ("...is not an existing directory"). We do not + # The existing non-ASCII dir must not be misread as absent; + # FILE_NOT_FOUND specifically is the #1585 encoding bug. We do not # assert OK: a module-less dir may legitimately report ERR_BACKEND # depending on build posture. lib = t._lib diff --git a/include/transcribe.h b/include/transcribe.h index ea0f27d3..27cc3f03 100644 --- a/include/transcribe.h +++ b/include/transcribe.h @@ -1338,9 +1338,8 @@ TRANSCRIBE_API const char * transcribe_model_meta_val_str(const struct transcrib /* * Load a GGUF model from disk. * - * path is UTF-8, like every path crossing this API. On Windows it is - * converted to a wide path internally, so paths containing non-ASCII - * characters (e.g. a user profile such as C:\Users\Jerôme\...) load + * path is UTF-8, like every path crossing this API; on Windows it is + * converted to a wide path internally, so non-ASCII paths load * correctly regardless of the process ANSI code page. * * params may be NULL for all library defaults. To customize, initialize diff --git a/src/transcribe-loader.cpp b/src/transcribe-loader.cpp index b3c74887..94603687 100644 --- a/src/transcribe-loader.cpp +++ b/src/transcribe-loader.cpp @@ -33,10 +33,9 @@ transcribe_status Loader::open(const char * path) { path_ = path; - // Pre-check whose ONLY job is to distinguish "the file is not at - // this path" (-> FILE_NOT_FOUND) from every other reason - // gguf_init_from_file might return nullptr (-> ERR_GGUF). See - // transcribe-path.h for the exact semantics. + // Distinguishes "the file is not at this path" (-> FILE_NOT_FOUND) + // from every other reason gguf_init_from_file might return nullptr + // (-> ERR_GGUF). Exact semantics in transcribe-path.h. if (!path_is_present(path)) { return TRANSCRIBE_ERR_FILE_NOT_FOUND; } diff --git a/src/transcribe-path.h b/src/transcribe-path.h index a38624d1..8faec1b3 100644 --- a/src/transcribe-path.h +++ b/src/transcribe-path.h @@ -1,24 +1,16 @@ -// transcribe-path.h - UTF-8 path handling shared by every file-access site. +// transcribe-path.h - UTF-8 path handling for user-facing file access. // // Contract: every path crossing the public C ABI is UTF-8 (see -// include/transcribe.h; that is what the FFI bindings send, and what -// path_for_c_api() in transcribe.cpp produces). On POSIX the narrow -// char* file APIs consume UTF-8 bytes directly, so plain std::string -// paths work. On Windows the narrow APIs (::stat, fopen, -// ifstream(const std::string &)) interpret narrow strings in the -// process ANSI code page, NOT UTF-8 — a path containing e.g. "ô" -// fails with ENOENT even though the file exists (Handy issue #1585). -// Every stat/open on the release model-load path must therefore go -// through the helpers here, which route file access through -// std::filesystem::path with an explicit UTF-8 -> native-wide -// conversion on Windows. +// include/transcribe.h). On POSIX the narrow file APIs consume UTF-8 +// bytes directly. On Windows they interpret narrow strings in the +// process ANSI code page, so a non-ASCII path fails with ENOENT even +// though the file exists (Handy issue #1585). Every stat/open a +// user-supplied path can reach must therefore go through these +// helpers, which convert UTF-8 to the native wide encoding on Windows. // -// Deliberately NOT converted: dev-only I/O — the dump writers in -// transcribe-debug.cpp and the TRANSCRIBE_ENABLE_VALIDATION_HOOKS -// sidecar readers in src/arch/*/model.cpp. Those read/write paths that -// never cross the public ABI. Don't copy file-access idioms from those -// files into release-path code; anything a user-supplied path reaches -// goes through these helpers. +// Deliberately not converted: dev-only I/O (the transcribe-debug.cpp +// dump writers, the TRANSCRIBE_ENABLE_VALIDATION_HOOKS sidecar +// readers) — those paths never cross the public ABI. #pragma once @@ -48,17 +40,15 @@ inline std::filesystem::path path_from_utf8(const std::string & utf8) { // not at this path" from every other reason a subsequent open might // fail. // -// We deliberately do not check for a regular file. If the path points -// at a directory (or a fifo, or a socket), the file IS at that path — -// it's just not usable, and the caller's open will surface that as a -// more accurate error than FILE_NOT_FOUND. +// Deliberately no regular-file check: if the path names a directory +// (or a fifo, or a socket), the file IS at that path — the caller's +// open will surface a more accurate error than FILE_NOT_FOUND. // -// We also deliberately only report absence on the not-found outcome -// (POSIX ENOENT / ENOTDIR and their Windows equivalents, which -// fs::status maps to file_type::not_found). Other failures (EACCES, -// ENAMETOOLONG, ELOOP, EIO, ...) do NOT conclusively prove the file -// is missing — they prove we can't reach it — so they return true and -// the caller's open reports the real error. +// Deliberately only the not-found outcome (ENOENT / ENOTDIR and their +// Windows equivalents, which fs::status maps to file_type::not_found) +// reports absence. Other failures (EACCES, ELOOP, EIO, ...) don't +// prove the file is missing — only that we can't reach it — so they +// return true and the caller's open reports the real error. inline bool path_is_present(const char * utf8_path) { std::error_code ec; return std::filesystem::status(path_from_utf8(utf8_path), ec).type() != std::filesystem::file_type::not_found; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9287ef4c..b35bb291 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -396,15 +396,14 @@ endif() # Non-ASCII (UTF-8) path handling through the public C ABI # ----------------------------------------------------------------------------- # -# Regression test for Handy issue #1585: on Windows, paths containing -# non-ASCII characters (e.g. C:\Users\Jerôme\...) failed model load and -# backend-dir validation because narrow stat()/ifstream calls read the -# UTF-8 bytes in the ANSI code page. Copies two fixtures into a -# non-ASCII-named temp directory and drives transcribe_model_load_file / -# transcribe_init_backends through it. On POSIX this pins the UTF-8 -# contract; on Windows it exercises the wide-path conversion in -# src/transcribe-path.h. Public-API only. Fixture-dependent, so gated -# on uv like loader_smoke. +# Regression test for Handy issue #1585: on Windows, non-ASCII paths +# failed model load and backend-dir validation because narrow +# stat()/ifstream calls read the UTF-8 bytes in the ANSI code page. +# Copies two fixtures into a non-ASCII-named temp directory and drives +# transcribe_model_load_file / transcribe_init_backends through it. On +# POSIX this pins the UTF-8 contract; on Windows it exercises the +# wide-path conversion in src/transcribe-path.h. Public-API only. +# Fixture-dependent, so gated on uv like loader_smoke. if(TRANSCRIBE_UV) add_executable(transcribe_utf8_path_unit diff --git a/tests/utf8_path_unit.cpp b/tests/utf8_path_unit.cpp index b7687367..7c94560b 100644 --- a/tests/utf8_path_unit.cpp +++ b/tests/utf8_path_unit.cpp @@ -2,13 +2,12 @@ // public C ABI. // // Regression test for Handy issue #1585: on Windows, model loading and -// backend artifact-dir validation failed for any path containing a -// non-ASCII character (e.g. C:\Users\Jerôme\...), because the library's -// ::stat() / ifstream(const std::string &) calls interpreted the UTF-8 -// path bytes in the process ANSI code page. The contract -// (include/transcribe.h) is that every path crossing the C ABI is -// UTF-8; src/transcribe-path.h now routes all file access through an -// explicit UTF-8 -> wide conversion on Windows. +// backend artifact-dir validation failed for any non-ASCII path, +// because the library's ::stat() / ifstream(const std::string &) calls +// interpreted the UTF-8 path bytes in the process ANSI code page. The +// contract (include/transcribe.h) is that every path crossing the C +// ABI is UTF-8; src/transcribe-path.h now routes all file access +// through an explicit UTF-8 -> wide conversion on Windows. // // The test copies two build-time fixtures into a directory whose name // contains non-ASCII characters and asserts, via the public API only: @@ -32,7 +31,7 @@ // // The directory name is spelled in escaped UTF-8 bytes, not literal // characters, so the test is immune to source-charset differences -// across compilers ("Jerôme-日本語"). +// across compilers ("café-日本語"). #include "transcribe.h" @@ -50,9 +49,9 @@ namespace fs = std::filesystem; namespace { -// "transcribe-utf8-Jerôme-日本語" in escaped UTF-8 bytes. +// "transcribe-utf8-café-日本語" in escaped UTF-8 bytes. constexpr const char * kDirName = - "transcribe-utf8-Jer\xC3\xB4me-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; + "transcribe-utf8-caf\xC3\xA9-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; int g_failures = 0; From f1baf04258457a261d6ccae65ccce50f46201b86 Mon Sep 17 00:00:00 2001 From: CJ Pais Date: Fri, 3 Jul 2026 13:38:54 +0800 Subject: [PATCH 3/3] clang format --- tests/utf8_path_unit.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/utf8_path_unit.cpp b/tests/utf8_path_unit.cpp index 7c94560b..f20461a0 100644 --- a/tests/utf8_path_unit.cpp +++ b/tests/utf8_path_unit.cpp @@ -50,8 +50,7 @@ namespace fs = std::filesystem; namespace { // "transcribe-utf8-café-日本語" in escaped UTF-8 bytes. -constexpr const char * kDirName = - "transcribe-utf8-caf\xC3\xA9-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; +constexpr const char * kDirName = "transcribe-utf8-caf\xC3\xA9-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; int g_failures = 0;