Skip to content

fix(win): deliver non-ASCII CLI paths end-to-end (wide argv + CreateProcessW)#891

Merged
DeusData merged 1 commit into
DeusData:mainfrom
Flipper1994:fix/win-utf8-argv
Jul 5, 2026
Merged

fix(win): deliver non-ASCII CLI paths end-to-end (wide argv + CreateProcessW)#891
DeusData merged 1 commit into
DeusData:mainfrom
Flipper1994:fix/win-utf8-argv

Conversation

@Flipper1994

Copy link
Copy Markdown
Contributor

Summary

Closes #423, #20 -- the still-open Windows narrow-argv bug flagged in the #639 review as "genuinely still present". A non-ASCII repo path passed to codebase-memory-mcp cli index_repository "<json>" was mangled on Windows and the command failed with "repo_path is required" instead of indexing the real directory. POSIX is unaffected (argv is already UTF-8).

The bug has TWO boundaries (both fixed)

A self-review caught that fixing only argv is insufficient — the default runtime re-mangles the path a second time:

  1. argv read. main() took only the narrow int main(int argc, char **argv), so the CRT handed it argv in the active ANSI code page. On Windows main() now rebuilds argv from the wide command line (GetCommandLineW + CommandLineToArgvW) and converts each element to UTF-8 (cbm_wide_to_utf8).
  2. worker spawn. With the supervisor at its default (enabled) setting, the worker is spawned via CreateProcessA, which re-interprets the UTF-8 command line through the ANSI code page — re-mangling the path at the parent→worker boundary even after (1). Both spawn sites (subprocess.c cbm_run_win and the UI http_server.c index spawn) now build a wide command line (cbm_utf8_to_wide) and use CreateProcessW.

Without (2) the default path stays broken while only CBM_INDEX_SUPERVISOR=0 (in-process) works — so this is the complete end-to-end fix.

Test — reproduce-first, promoted guard, exercises the DEFAULT path

tests/windows/test_cli_non_ascii_arg.py (the opt-in known-red from #639) is promoted to a green guard ($knownReds$guards, gated by test-windows-guards). Critically it now drops the suite's CBM_INDEX_SUPERVISOR=0, so it crosses the real supervisor→worker spawn boundary — otherwise a green guard would mask the spawn half of the bug.

Verified on native Windows (Win11, CLANG64):

  • RED before the fix; GREEN after (a café_日本語_repo indexes, nodes>0) on the supervised path.
  • Full -GuardsOnly suite green.

Also: -lshell32 added explicitly (main.c now depends on CommandLineToArgvW), and RED_TEST_ANALYSIS.md refreshed from known-red to green-guard.

Closes #423, #20.

@DeusData

DeusData commented Jul 5, 2026

Copy link
Copy Markdown
Owner

One housekeeping ask before we merge, @Flipper1994: could you drop tests/windows/RED_TEST_ANALYSIS.md from this PR (git rm it — deleting it from the repo is fine)? We're keeping analysis/tracking markdown out of the tree as a policy. The code fix and the promoted test_cli_non_ascii_arg.py guard are exactly what we want — just the .md needs to go.

The fix itself looks great: clean wide-argv main() (GetCommandLineW → CommandLineToArgvW → UTF-8, with fallback) + CreateProcessW at both spawn sites, and -lshell32 wired in. Once the .md is removed I'll re-diff and merge. Thanks! 🙏

…rocessW)

Closes DeusData#423, DeusData#20. A non-ASCII repo path passed to
`codebase-memory-mcp cli index_repository "<json>"` was mangled on Windows, so the
command failed with "repo_path is required" instead of indexing the real directory.
POSIX is unaffected (argv is already UTF-8). Two boundaries had to be fixed:

1. argv read: main() took only the narrow `int main(int argc, char **argv)`, so the
   CRT handed it argv in the active ANSI code page. On Windows main() now rebuilds
   argv from the wide command line (GetCommandLineW + CommandLineToArgvW) and
   converts each element to UTF-8 (cbm_wide_to_utf8).

2. worker spawn: the index supervisor (and the UI index thread) spawned the worker
   via CreateProcessA, which re-interpreted the UTF-8 command line through the ANSI
   code page -- re-mangling the path at the parent->worker boundary even after (1).
   Both spawns now build a wide command line (cbm_utf8_to_wide) and use CreateProcessW.

Without (2) the default (supervisor-enabled) path stayed broken while only the
in-process path worked, so this is the complete end-to-end fix, not just argv.

Promotes tests/windows/test_cli_non_ascii_arg.py from an opt-in known-red to a green
Windows guard (scripts/test-windows.ps1 + the test-windows-guards CI job). The guard
now exercises the DEFAULT supervised path (it drops the suite's CBM_INDEX_SUPERVISOR=0
so it actually crosses the worker-spawn boundary). Verified on native Windows: RED
before the fix, GREEN after (a non-ASCII repo indexes, nodes>0). Also adds -lshell32
explicitly (main.c now uses CommandLineToArgvW) and refreshes RED_TEST_ANALYSIS.md.

Signed-off-by: Flipper <jacobphilipp@ymail.com>
@Flipper1994 Flipper1994 marked this pull request as ready for review July 5, 2026 16:51
@Flipper1994 Flipper1994 requested a review from DeusData as a code owner July 5, 2026 16:51
@Flipper1994

Copy link
Copy Markdown
Contributor Author

Done -- git rm'd tests/windows/RED_TEST_ANALYSIS.md and updated the single reference to it in test-windows.ps1 so nothing dangles. Just the code fix + the promoted test_cli_non_ascii_arg.py guard remain now. Pushed -- ready for your re-diff. Thanks! 🙏

@DeusData DeusData merged commit a5351d3 into DeusData:main Jul 5, 2026
10 checks passed
@DeusData

DeusData commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Merged (a5351d3a) — thank you! Solid end-to-end non-ASCII CLI fix (wide argv via GetCommandLineW→CommandLineToArgvW + CreateProcessW at both spawn sites), and thanks for dropping the .md and the clean commit message. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codebase-memory-mcp cli index_repository asking for path but path is specified (Windows)

2 participants