fix(win): escape spawned command-line args so the index worker gets valid argv

[Flipper1994]

Summary

Fixes the Windows-only smoke-windows (standard + ui) failure in the final release dry run, where index_repository reported outcome=exit_nonzero and "Indexing worker crashed on a file" on big_templated.hpp.

The failure is not the #424 allocator class and not big_templated.hpp. It is a command-line quoting bug in the Windows index-worker spawn. The file parses fine; the worker never reaches it.

Relationship to #838

This is the Windows residual of #838. #838 fixed the CLI parser to accept the supervisor's raw-JSON-positional + --response-out flag layout — verified there with a PowerShell repro, where PowerShell passes the JSON as one intact argument. On current main a correctly-delivered JSON arg does parse (confirmed: running the worker directly indexes clean).

But the real supervisor spawns the worker via CreateProcessA, not PowerShell. Its command-line re-parse strips the JSON's quotes, so on Windows the now-fixed parser still receives corrupted JSON. #838 was closed against the parser; this PR fixes the spawn-side quoting that #838's repro never exercised. (See also #423 — the open Windows "repo_path is required, path is specified" report.)

Root cause

The supervisor spawns:

<self> cli --index-worker index_repository <args_json> --response-out <file>

args_json is the raw tool JSON, e.g. {"repo_path":"C:/r"} — it contains double-quotes. On Windows, cbm_run_win built the CreateProcess command line by wrapping each argv element in bare quotes without escaping:

snprintf(cmdline, ..., "%s\"%s\"", (i ? " " : ""), argv[i]);  // no escaping

Windows re-parses that single string back into argv and strips the inner quotes, so the child received {repo_path:C:/r} — invalid JSON → repo_path is required → exit 1. The supervisor classified exit_nonzero and blamed the last-marked file (big_templated.hpp), producing the misleading crash message.

Proof: a standalone CreateProcessA reproducer built with the exact cbm_run_win construction made an argv-printer receive argv[4]=<{repo_path:C:/r}>; feeding the worker that exact mangled JSON reproduced the failure signature byte-for-byte.

Why POSIX is unaffected: cbm_run_posix passes the argv array straight to execv — no command-line re-parse. That is why Linux/macOS index the same fixture fine and only smoke-windows was red.

Fix

cbm_build_win_cmdline() applies the Microsoft C runtime quoting rules (quote-wrap + escape embedded quotes and their preceding backslash runs) so the child re-parses byte-identical argv. It is defined unconditionally (pure string logic, no Windows headers) so the quoting contract is unit-tested on Linux/macOS CI too, and both spawn sites share one tested implementation.

Also in this PR

• ui/http_server.c — the UI index spawn had a byte-for-byte duplicate of the same bug (its own "%s" cmdline for the JSON arg). Now routed through cbm_build_win_cmdline.
• mcp/index_supervisor.c (observability) — log the worker's raw exit_code and keep its log on any failure (delete only on a clean run). Previously the log was always deleted and only outcome+signal were logged, so a non-zero worker left nothing to diagnose — the CI blind spot that hid this bug.

Scope

Kept atomic to the CreateProcess argv-quoting bug class. A Windows quoting audit of the tree also found pipeline/artifact.c's two single-quoted, unvalidated git -C '%s' shell-outs — but that is a different bug class (shell/popen command-string quoting, not CreateProcess argv re-parse), so per review it is split into a follow-up PR with its own guard test. The other CreateProcess sites are safe (argv-array escaping via cbm_exec_no_shell, or "-rejecting validation).

Test

tests/test_subprocess.c gains a Layer-3 guard for cbm_build_win_cmdline, verified by round-trip: a reference implementation of the Windows CommandLineToArgvW rules (the inverse of the builder) must re-parse the emitted line back into the exact original argv. Cases cover the failing JSON, spaces/tabs, embedded quotes, backslash runs, trailing backslashes, backslash-before-quote, real Windows paths, and overflow. Testing the invariant (not a hand-computed escaped string) keeps the guard honest and runs on every platform.

Verification (CLANG64 — the toolchain CI ships)

• Before: supervised big_templated.hpp → index.supervisor.reap outcome=exit_nonzero, exit 1.
• After: index.supervisor.reap outcome=clean exit_code=0 signal=0, status:indexed (9005 nodes), exit 0.
• Also confirmed on the native Windows smoke-invariants leg: PASS: index-cli (nodes=29, rc=0) — the supervised index_repository that returns nodes=0 without the fix.
• subprocess suite: 11 passed / 5 skipped (POSIX-only spawn tests).

Refs #838, #423.

[DeusData]

Thank you — this is genuinely excellent work. The diagnosis is spot-on (spawn-side CreateProcessA quoting, not the allocator/big_templated.hpp), the cbm_build_win_cmdline escaping matches the canonical MS-CRT rules exactly (and lines up byte-for-byte with our existing cbm_build_cmdline), both vulnerable spawn sites are covered, and the round-trip test is a real, cross-platform guard. We want to land it.

Two small things before we merge, then it's good to go:

1. Please drop the AI attribution from the commit. Our public repos keep commit messages human-authored — no Co-Authored-By: Claude … / claude co-author. Could you git commit --amend to remove that trailer (keep your own Signed-off-by: — DCO needs it, and it already matches your author email 👍), then git push --force-with-lease?

2. Please split out the artifact.c change into its own follow-up PR. It's a good hardening, but it's a different bug class — shell/popen command-string quoting rather than the CreateProcess argv re-parse this PR fixes — and it lands without a test. We keep PRs atomic + reproduce-first, so pulling it into a separate PR (with a small guard test) keeps this one focused on the argv-quoting fix. Happy to fast-track that follow-up. The subprocess.c + http_server.c fix, the test, and the index_supervisor.c observability are all great to keep here.

Once those two are done we'll merge it. Heads up: we're landing it right alongside a smoke-CI fix that makes the native windows-latest smoke leg actually run and gate (it wasn't, due to a matrix bug) — so your fix will get validated on native Windows in CI, not just the emulated leg. Really appreciate you jumping on this. 🙏

[DeusData]

Merged — thank you, @Flipper1994. Sharp diagnosis and a clean fix: the crash was never the allocator or big_templated.hpp at all, but the Windows CreateProcessA spawn wrapping the JSON arg in bare quotes, so the re-parse stripped them and the worker got invalid JSON. The MS-CRT quoting in cbm_build_win_cmdline (shared across both spawn sites), the round-trip test, and the exit-code observability are all excellent, and the artifact.c split into #886 was exactly right. This unblocks the release. Really appreciate you jumping on it. 🙏

[Flipper1994]

Thanks again for the fast merge! 🙏

One honest post-review note: during a self-review after this was already merged, I spotted a small contract inaccuracy in cbm_build_win_cmdline — on the overflow path it return falses without NUL-terminating buf, although the header comment says "buf then holds a truncated string". No live bug — both call sites (cbm_run_win, the http_server spawn) ignore buf when the function returns false — but a future caller trusting the doc could OOB-read. I have a tiny tidy ready that adds buf[0] = '\0' on that path, corrects that comment plus one stale comment on cbm_cmdline_put, and bounds-guards the reference parser in the round-trip test.

Happy to open it as a small atomic follow-up, or leave main as-is if it's not worth the round-trip — your call. It's pure polish, not a blocker.

[DeusData]

Thanks for the honest post-merge self-review, @Flipper1994 — I verified it and you're right on all three points. subprocess.h promises "buf then holds a truncated string" on overflow, but the overflow path (if (ovf) return false;) skips the buf[pos] = '\0' that only runs on the success path, so buf is left unterminated. Both call sites (cbm_run_win, http_server index spawn) ignore buf on false, so it's latent — no live bug — but the contract is wrong and a future caller could OOB-read.

Yes please, open it as a small atomic follow-up. Two asks to keep it in line with our conventions:

• Make the overflow path NUL-terminate — buf[0] = '\0' (an empty truncated string is safest, since pos can reach cap), so the code matches the doc rather than loosening the doc to "contents unspecified".
• Include a reproduce-first test: one that is RED on current main (feed an argv that overflows a small cap, then assert buf is NUL-terminated / the reference round-trip parser stays in bounds) and turns green with the fix. The bounds-guarded round-trip test you mentioned sounds exactly right.

Keep the two comment corrections (cbm_build_win_cmdline + cbm_cmdline_put) in the same PR — they're part of the same contract fix. Appreciate the diligence 🙏