fix(windows): document MotW unblock, surface config errors, fix taskkill quoting

[ShauryaaSharma]

Fixes #697

What does this PR do?

Addresses all three root causes reported in #697, where install.ps1 reports success but leaves the MCP server unconfigured on Windows.

1. Mark-of-the-Web (README)
Invoke-WebRequest stamps downloaded files with a Zone Identifier that causes PowerShell to refuse execution. The README Windows install steps now include an explicit Unblock-File .\install.ps1 step (both the quick-install
block and the manual-install block), with a note on -ExecutionPolicy Bypass as a fallback.

2. Swallowed config errors (install.ps1)
The try/catch around & $Dest install -y does not catch non-terminating exits. A non-zero exit code was silently swallowed and the script printed "Done!" regardless. The block now checks $LASTEXITCODE after the call and, if non-zero, prints a red error message and exits 1 so the failure is visible.

3. taskkill /FI quoting (compat_fs.c)
cbm_exec_no_shell on Windows used _spawnvp, whose MinGW CRT implementation does not quote arguments that contain spaces when building the CreateProcess command line. The filter value "IMAGENAME eq codebase-memory-mcp.exe" was passed as three bare tokens, causing taskkill to print ERROR: Invalid argument/option - 'eq' on every invocation.

cbm_exec_no_shell now uses CreateProcess directly with a new cbm_build_cmdline helper that produces a properly-quoted command line following the MSVC/Windows CRT convention (arguments with spaces, tabs, or embedded double-quotes are wrapped in "…", with backslashes before a closing quote doubled). The POSIX path (fork/execvp) is unchanged.

CreateProcess replaces _spawnvp at the same call site with the same security surface, no new subprocess capability is introduced, and no update to scripts/security-allowlist.txt is required.

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (make -f Makefile.cbm test), full suite requires ASan + 64-bit toolchain unavailable in this environment; the cbm_build_cmdline quoting logic and cbm_exec_no_shell live-exec paths were verified with a targeted harness compiled under MinGW GCC 6.3 (7 quoting cases + 3 live CreateProcess exec cases, all passing)
• Lint passes (make -f Makefile.cbm lint-ci) — clang-tidy unavailable in this environment; please run in CI
• New behavior is covered by a test (reproduce-first for bug fixes) no test added to tests/; the fix is in the installer script and a Windows-only code path not currently covered by the test suite.

[DeusData]

Huge thanks for opening this PR and for the work you put into it.

The maintainer shop is currently full, so this may sit for a bit before it gets a proper review. We will come back to this as soon as possible with real feedback; I wanted to make sure it did not sit unacknowledged in the meantime.

[DeusData]

Thanks for the clear Windows fix. Before this can move forward, could you add a regression guard for the Windows quoting/config failure, or explain why it cannot live in the suite? This touches install.ps1 and Windows process spawning, so we’ll also need CI/maintainer verification around those paths.

[ShauryaaSharma]

Hey, Thanks for the steer!

So, you were right to call out my "can't really test this" note. I was wrong, I'd figured the bug only really shows itself once taskkill actually chokes on the args, which would've meant spinning up a live Windows process to catch it. But when I went back and actually looked, the real reason is just the command-line building in cbm_build_cmdline, and that thing is pure string logic, no OS calls, totally deterministic. So there was nothing stopping me from testing it directly. Sorry about that.

Pushed an update just now, rebased onto main first (went through clean), then added the guard.

[ShauryaaSharma]

The one thing I didn't write a test for is the install.ps1 config-error fix.

There's no Pester setup anywhere in the repo and install.ps1 never actually runs in CI (it just gets zipped for release), so testing it properly would mean a whole new CI job plus reworking the installer so the config step can be driven in isolation.

I'm glad to pick it up as a follow-up if you'd like. I did check the exit 1 behavior on the config failure by hand for now.

Let me know what you think!

[ShauryaaSharma]

Rest, here is what i have done as of now.

I exposed the command-line builder (cbm_build_cmdline) through a new compat_fs_internal.h, the same trick system_info_internal.h already uses to make internal helpers reachable from tests, and dropped a few tests into test_security.c. The main test nails the exact #697 case:

{"taskkill","/FI","IMAGENAME eq codebase-memory-mcp.exe"}
  ->  taskkill /FI "IMAGENAME eq codebase-memory-mcp.exe"

so that the IMAGENAME filter has to survive as one quoted token instead of the three bare words that had taskkill tripping over eq. I threw in the fiddly quoting cases too (empty args, embedded quotes, trailing backslashes). And while I was in test_security.c I noticed the cbm_exec_no_shell tests were all behind #ifndef _WIN32, so the Windows spawn path had no coverage at all, I filled that gap with a couple of live CreateProcessW runs. All of these tests ride on the windows-latest job that's already in _test.yml, so there's nothing new needed on the CI side.

[DeusData]

Thank you — this fixes all three root causes of #697 rather than documenting around them: MotW genuinely can only be documented (a blocked script can't unblock itself), the $LASTEXITCODE check is the correct diagnosis of why the old catch never fired, and the CreateProcessW quoting rewrite addresses a real MinGW _spawnvp bug that's still on main. We hand-verified the two-pass cmdline size accounting (embedded quotes, trailing backslashes) — no overrun — and the live Windows-CI-executed tests make the green genuinely meaningful. One ask before merge: the byte-widening loop treats UTF-8 as Latin-1, so non-ASCII args (e.g. a download destination under a non-ASCII %USERPROFILE%) become mojibake — not a regression vs _spawnvp, but the repo convention is cbm_utf8_to_wide and the helper already exists; could you switch the widening to it? Small change, then this merges. Thanks for the careful work on a sensitive surface.

---

Update: to keep momentum on the bug backlog, we're going to carry the changes above over the line ourselves shortly — a distilled follow-up on current main implementing the notes in this thread, with you credited as Co-authored-by on the commit, and this PR closed referencing it. If you'd prefer to push the update yourself, just reply within the next couple of days and we'll gladly take yours instead. Thanks again for the contribution!

[ShauryaaSharma]

Thanks for the heads-up, and I completely understand wanting to keep the backlog moving. No need to redo it on your end though. I pushed the update addressing the cbm_utf8_to_wide note, so this PR should be good to take the rest of the way as-is.

I'll stay on top of it and I'm around anytime for any further changes or tweaks you'd like, happy to turn things around quickly.

[ShauryaaSharma]

What changed (commits 577c769 + 2bb0401, already on this branch):

• The fix (compat_fs.c): cbm_build_cmdline previously widened each argument byte with (wchar_t)(unsigned char)*p, which zero-extends bytes and so decodes UTF-8 as Latin-1, turning a non-ASCII arg (e.g. a destination under a non-ASCII %USERPROFILE%) into mojibake. It now assembles the quoted command line in UTF-8 and converts the whole buffer once via the existing cbm_utf8_to_wide helper. The byte-level quoting logic is unchanged and safe, since every character it acts on (space, tab, ", \) is ASCII and, by UTF-8's design, never appears inside a multibyte sequence, so multibyte bytes pass through untouched. The two-pass byte-size accounting you hand-verified is preserved exactly.

• The test (test_security.c): added cmdline_utf8_arg_is_widened_not_latin1, which pins that "café dir" (bytes C3 A9) collapses to the single code point U+00E9, not U+00C3 U+00A9. It fails against the old byte-widening loop and runs on the existing windows-latest CI job.

How I tested it:

• ASCII output is byte-for-byte identical to before, so all existing Windows: install.ps1 reports success but leaves MCP server unregistered (MotW + swallowed config errors + malformed taskkill) #697 quoting regression tests are unaffected.
• Ran 20 targeted cases: every existing suite case plus tab-quoting, backslash/quote combinations, and 2-byte (é), 3-byte (€), and 4-byte (😀 → surrogate pair D83D DE00) UTF-8, including non-ASCII at the start of argv[0] and backslashes adjacent to multibyte characters, all pass.
• Ran a 200,000-iteration round-trip fuzz: quote randomized argv arrays (ASCII + whitespace + metacharacters + multibyte), parse the result back with Windows' own CommandLineToArgvW, and assert each argument survives identically — 0 failures. This validates both quoting correctness and the absence of any buffer overrun or truncation.
• Both compat_fs.c and test_security.c compile cleanly under the Windows toolchain, and every commit is signed off (DCO).

Happy to make any adjustments you'd like before merge.

[ShauryaaSharma]

@DeusData
(Small note: the rebase force-pushed new SHAs, so the Actions run is now sitting in "awaiting approval".
Whenever you get a moment, a quick "Approve and run workflows" will kick CI off against the update. Thanks!)

[DeusData]

Thank you — this fixed all three root causes of #697 rather than documenting around them, and your CreateProcessW quoting rewrite addressed a real MinGW _spawnvp bug. Special thanks for pushing the UTF-8 completion yourself after the review — we verified it line-by-line (the two-pass byte sizing balances exactly) and carried the whole thing over the line with one extra mixed-multibyte round-trip test, keeping you credited as co-author. Merged as 9fbdac2 (PR #813), closing #697. The lint failures that had blocked your branch were repo-side queue/format noise, nothing about your code. Closing this in favor of the distill — thanks again for careful work on a sensitive surface!