fix(build,platform): paths with spaces survive both include channels and cmd.exe

Two independent places assumed no path ever contains a space — which on
Windows means assuming nobody installs under C:\Program Files and no user
name has a space in it.

include dirs (#331): the same manifest include_dirs reaches the compiler
through the global blob in flags.cppm and through ninja_backend's per-TU
$local_includes. Only the first shell-quoted; the second applied ninja's $
escaping alone, so ninja un-escaped the space and handed the shell a word
that split. Both now go through mcpp::build::include_token — a shared helper
rather than a second copy of the quoting, because a third channel is exactly
how this happened. That also fixes the plain half of local_include_flags
hardcoding -I while its after-dirs half honoured the dialect.

cmd.exe: argv[0] was emitted raw to survive cmd's /c quote stripping, which
traded 'the path gets mangled' for 'the path is cut at the first space'.
Give cmd the outer quote pair it insists on consuming and the inner quoting
arrives intact, so every token can be quoted. run_exec keeps inheriting
stdio — sealing its stdin would break interactive `mcpp run`.

The Windows command-line shape is now built by host-independent functions so
Linux CI exercises it; that branch is not even compiled on the platforms mcpp
is developed on, which is how the unquoted argv[0] survived this long.

Author: Sunrisepeak

src/build/flags.cppm

@@ -82,6 +82,26 @@ std::string atomic_link_flag(const std::vector<std::filesystem::path>& linkDirs,
 // escaped as `\"`) — cmd.exe/CreateProcess argv convention.
 std::string shell_quote_arg(std::string_view arg);
 
+// One include-directory token, fully prepared for a ninja command line:
+// dialect prefix, ninja `$` escaping, and shell quoting — in that order.
+//
+// #331: the same manifest `[build] include_dirs` reaches the compiler through
+// two channels — the global blob assembled below, and the per-translation-unit
+// `$local_includes` emitted by ninja_backend. Only the first one quoted, so an
+// include dir containing a space (`C:\Program Files\...`, or `/home/my dir` on
+// Linux) survived one path and split into separate shell words on the other.
+// Both channels call this now; adding a third one and forgetting to quote is
+// how the bug happened, and a shared helper is the only fix that also covers
+// the fourth.
+//
+// `prefixOverride` replaces `d.includePrefix` for the callers that need a
+// different flag for the same kind of path (`-idirafter` for #249's
+// after-dirs, plain `-I` for NASM units which would parse `-idirafter<p>` as
+// `-i dirafter<p>`).
+std::string include_token(const mcpp::toolchain::CommandDialect& d,
+                          const std::filesystem::path& dir,
+                          std::string_view prefixOverride = {});
+
 }  // namespace mcpp::build
 
 namespace mcpp::build {
@@ -139,6 +159,19 @@ std::string atomic_link_flag(const std::vector<std::filesystem::path>& linkDirs,
     return {};
 }
 
+std::string include_token(const mcpp::toolchain::CommandDialect& d,
+                          const std::filesystem::path& dir,
+                          std::string_view prefixOverride) {
+    std::string_view prefix =
+        prefixOverride.empty() ? d.includePrefix : prefixOverride;
+    // Prefix first, then escape+quote the whole token: the prefix and the
+    // path are ONE argv word, so quoting them separately would put the
+    // opening quote in the wrong place and re-split exactly what we came to
+    // join.
+    return shell_quote_arg(
+        escape_path(std::filesystem::path(std::string(prefix) + dir.string())));
+}
+
 std::string shell_quote_arg(std::string_view arg) {
     // Characters that split/alter a word when unquoted in POSIX sh or
     // cmd.exe: whitespace plus the common shell metacharacters. Anything
@@ -222,7 +255,7 @@ CompileFlags compute_flags(const BuildPlan& plan) {
     std::vector<std::string> includeTokens;
     for (auto& inc : plan.manifest.buildConfig.includeDirs) {
         std::filesystem::path p = inc.has_root_path() ? inc : (plan.projectRoot / inc);
-        includeTokens.push_back(std::string(d.includePrefix) + p.string());
+        includeTokens.push_back(include_token(d, p));
     }
     // #249: `[build] include_dirs_after` — searched AFTER the toolchain's
     // system dirs via -idirafter (gcc+clang), so entries can't shadow
@@ -233,12 +266,13 @@ CompileFlags compute_flags(const BuildPlan& plan) {
     for (auto& inc : plan.manifest.buildConfig.includeDirsAfter) {
         std::filesystem::path ip(inc);
         std::filesystem::path p = ip.has_root_path() ? ip : (plan.projectRoot / ip);
-        includeTokens.push_back((msvcInclude ? "/I" : "-idirafter") + p.string());
+        includeTokens.push_back(
+            include_token(d, p, msvcInclude ? "/I" : "-idirafter"));
     }
     std::string include_flags;
     for (auto& t : includeTokens) {
         include_flags += ' ';
-        include_flags += shell_quote_arg(escape_path(std::filesystem::path(t)));
+        include_flags += t;   // already prefixed, escaped and quoted
     }
 
     // Sysroot / payload paths — resolved ONCE by the toolchain link model

src/build/ninja_backend.cppm

@@ -105,12 +105,20 @@ bool is_nasm_source(const std::filesystem::path& src) {
     return src.extension() == ".asm";
 }
 
-std::string local_include_flags(const CompileUnit& cu, bool msvcDialect) {
-    const bool nasmUnit = is_nasm_source(cu.source);
+std::string local_include_flags(const CompileUnit& cu,
+                                const mcpp::toolchain::CommandDialect& d) {
+    const bool nasmUnit    = is_nasm_source(cu.source);
+    const bool msvcDialect = d.includePrefix == std::string_view("/I");
     std::string flags;
     for (auto const& inc : cu.localIncludeDirs) {
-        flags += " -I";
-        flags += escape_flag_path(inc);
+        // #331: this used to hardcode `-I` and apply only ninja's `$`
+        // escaping — no shell quoting — while the global channel in
+        // flags.cppm quoted properly. Same manifest include_dirs, two
+        // derivations, and a directory with a space in it split into
+        // separate shell words on this path only. Both channels now go
+        // through mcpp::build::include_token.
+        flags += ' ';
+        flags += mcpp::build::include_token(d, inc);
     }
     // #249: after-dirs are searched AFTER the toolchain's system dirs
     // (-idirafter, gcc+clang), so a dep source root that contains a file
@@ -126,8 +134,10 @@ std::string local_include_flags(const CompileUnit& cu, bool msvcDialect) {
     //     `-idirafter<p>` as its `-i` option with value `dirafter<p>` —
     //     a silently wrong search dir — so nasm units get plain -I.
     for (auto const& inc : cu.localIncludeDirsAfter) {
-        flags += nasmUnit ? " -I" : (msvcDialect ? " /I" : " -idirafter");
-        flags += escape_flag_path(inc);
+        std::string_view pfx =
+            nasmUnit ? "-I" : (msvcDialect ? "/I" : "-idirafter");
+        flags += ' ';
+        flags += mcpp::build::include_token(d, inc, pfx);
     }
     return flags;
 }
@@ -994,7 +1004,7 @@ std::string emit_ninja_string(const BuildPlan& plan) {
             append(std::format("build {} : cxx_scan {}{}\n", escape_ninja_path(ddi),
                                escape_ninja_path(cu.source), stagedOrderOnly));
             append(std::format("  compile_target = {}\n", escape_ninja_path(cu.object)));
-            if (auto includes = local_include_flags(cu, msvcDeps); !includes.empty())
+            if (auto includes = local_include_flags(cu, dial); !includes.empty())
                 append(std::format("  local_includes ={}\n", includes));
             if (auto flags = join_flags(cu.packageCxxflags); !flags.empty())
                 append(std::format("  unit_cxxflags ={}\n", flags));
@@ -1076,7 +1086,7 @@ std::string emit_ninja_string(const BuildPlan& plan) {
             } else {
                 out_line += stagedOrderOnly + "\n";
             }
-            if (auto includes = local_include_flags(cu, msvcDeps); !includes.empty())
+            if (auto includes = local_include_flags(cu, dial); !includes.empty())
                 out_line += "  local_includes =" + includes + "\n";
             if (is_gas_source(cu.source) || is_nasm_source(cu.source)) {
                 if (auto flags = join_flags(asm_unit_flags(cu)); !flags.empty())
@@ -1128,7 +1138,7 @@ std::string emit_ninja_string(const BuildPlan& plan) {
                 out_line += " |" + implicit;
             out_line += stagedOrderOnly;
             out_line += "\n";
-            if (auto includes = local_include_flags(cu, msvcDeps); !includes.empty())
+            if (auto includes = local_include_flags(cu, dial); !includes.empty())
                 out_line += "  local_includes =" + includes + "\n";
             if (is_gas_source(cu.source) || is_nasm_source(cu.source)) {
                 if (auto flags = join_flags(asm_unit_flags(cu)); !flags.empty())

src/platform/process.cppm

@@ -128,12 +128,48 @@ int run_passthrough(std::string_view command,
 // a wait-status word requiring WIFEXITED/WEXITSTATUS unwrapping.
 int extract_exit_code(int raw_status);
 
+// ─── Windows command-line shaping (host-independent, for testing) ─────────
+//
+// `cmd.exe /c <string>` applies a quote rule that silently mangles most
+// command lines (`cmd /?`, /C section): unless the whole string is exactly
+// one quoted executable name, cmd removes the FIRST character and the LAST
+// quote character, then runs the remainder. A correctly quoted line like
+//
+//     "C:\Program Files\gcc\g++.exe" -c "main.cpp"
+//
+// therefore arrives as
+//
+//     C:\Program Files\gcc\g++.exe" -c "main.cpp
+//
+// The fix is to hand cmd an outer pair to consume. These two functions build
+// exactly that shape and are compiled on every platform so the rule can be
+// unit-tested from Linux/macOS — the Windows branch below is otherwise
+// unreachable in every environment mcpp is normally developed on, which is
+// how the unquoted-argv[0] bug survived.
+std::string windows_command_from_argv(const std::vector<std::string>& argv);
+std::string windows_wrap_for_cmd_c(std::string_view cmd);
+
 } // namespace mcpp::platform::process
 
 // ─── Implementation ──────────────────────────────────────────────────────
 
 namespace mcpp::platform::process {
 
+// Host-independent (see the declarations): always the Windows shape.
+std::string windows_command_from_argv(const std::vector<std::string>& argv) {
+    if (argv.empty()) return "";
+    std::string cmd = mcpp::platform::shell::quote_windows(argv[0]);
+    for (std::size_t i = 1; i < argv.size(); ++i) {
+        cmd += ' ';
+        cmd += mcpp::platform::shell::quote_windows(argv[i]);
+    }
+    return cmd;
+}
+
+std::string windows_wrap_for_cmd_c(std::string_view cmd) {
+    return "\"" + std::string(cmd) + "\"";
+}
+
 namespace {
 
 // Append a non-interactive stdin redirect to prevent child processes from
@@ -151,6 +187,44 @@ std::string seal_stdin(std::string_view cmd) {
 #endif
 }
 
+// Everything that reaches _popen / std::system on Windows is run by
+// `cmd.exe /c <string>`, and cmd applies a quote rule that mangles any
+// command line carrying more than one pair of quotes (`cmd /?`, the /C
+// section): unless the whole string is exactly one quoted executable name,
+// cmd strips the FIRST character and the LAST quote character and runs what
+// is left. So
+//
+//     "C:\Program Files\gcc\g++.exe" -c "main.cpp"
+//
+// becomes
+//
+//     C:\Program Files\gcc\g++.exe" -c "main.cpp
+//
+// which is why command_from_argv used to leave argv[0] unquoted — the
+// program path then survived, at the cost of breaking as soon as it
+// contained a space, which every default install path does
+// (`C:\Program Files\...`, or any user whose account name has a space).
+//
+// The documented fix is to give cmd an outer pair to eat, so the inner
+// quoting arrives intact. Applied at the single point where a command
+// string becomes a child process, so no caller has to remember it, and the
+// redirects appended by seal_stdin / silent_redirect stay inside the wrap
+// where cmd still parses them after stripping.
+std::string wrap_for_cmd_c(std::string_view cmd) {
+#if defined(_WIN32)
+    return windows_wrap_for_cmd_c(cmd);
+#else
+    return std::string(cmd);
+#endif
+}
+
+// Seal stdin AND wrap. Kept separate from wrap_for_cmd_c because run_exec
+// deliberately inherits stdio — `mcpp run` hands the terminal to the program
+// being run, and sealing its stdin would break every interactive one.
+std::string finalize_shell_command(std::string_view cmd) {
+    return wrap_for_cmd_c(seal_stdin(cmd));
+}
+
 int normalize_exit_code(int rc) {
 #if defined(_WIN32)
     return rc;
@@ -227,22 +301,27 @@ std::string spawn_failure(std::string_view program, int error) {
 }
 #else
 // Build a shell command line from an argv vector (Windows + residual non-POSIX
-// fallback only; Linux/macOS exec directly, #248). The first token (program)
-// is kept RAW on Windows — quoting it would make cmd.exe's `/c "..."` strip the
-// outer quotes and mangle the path (see platform.shell) — and shell-quoted
-// otherwise. Remaining args are always shell-quoted.
+// fallback only; Linux/macOS exec directly, #248). EVERY token is shell-quoted,
+// including the program — a payload under `C:\Program Files\...` or a home
+// directory with a space in the user name is otherwise cut at the first space
+// and reported as `'C:\Program' is not recognized`.
+//
+// argv[0] used to be left raw here to survive cmd.exe's /c quote stripping.
+// That traded one bug for another; finalize_shell_command now feeds cmd the
+// outer quote pair it insists on eating, so the quoting below arrives intact.
 std::string command_from_argv(const std::vector<std::string>& argv) {
-    if (argv.empty()) return "";
 #if defined(_WIN32)
-    std::string cmd = argv[0];
+    // One derivation: the tested, host-independent shaper above.
+    return windows_command_from_argv(argv);
 #else
+    if (argv.empty()) return "";
     std::string cmd = mcpp::platform::shell::quote(argv[0]);
-#endif
     for (std::size_t i = 1; i < argv.size(); ++i) {
         cmd += ' ';
         cmd += mcpp::platform::shell::quote(argv[i]);
     }
     return cmd;
+#endif
 }
 #endif
 
@@ -253,7 +332,7 @@ int extract_exit_code(int raw_status) {
 }
 
 RunResult capture(std::string_view command) {
-    auto cmd = seal_stdin(command);
+    auto cmd = finalize_shell_command(command);
     RunResult result;
 
     std::FILE* fp = ::popen(cmd.c_str(), "r");
@@ -306,14 +385,14 @@ RunResult capture_with_env(
 }
 
 int run_silent(std::string_view command) {
-    auto cmd = seal_stdin(command);
+    auto cmd = finalize_shell_command(command);
     return normalize_exit_code(std::system(cmd.c_str()));
 }
 
 int run_streaming(std::string_view command,
                   std::function<void(std::string_view line)> on_line)
 {
-    auto cmd = seal_stdin(command);
+    auto cmd = finalize_shell_command(command);
     std::FILE* fp = ::popen(cmd.c_str(), "r");
     if (!fp) return -1;
 
@@ -342,7 +421,7 @@ int run_streaming(std::string_view command,
 }
 
 int run_passthrough(std::string_view command, std::string* output) {
-    auto cmd = seal_stdin(command);
+    auto cmd = finalize_shell_command(command);
     std::FILE* fp = ::popen(cmd.c_str(), "r");
     if (!fp) return -1;
 
@@ -397,7 +476,8 @@ int run_exec(const std::vector<std::string>& argv,
     return normalize_exit_code(status);
 #else
     std::string prefix = mcpp::platform::env::build_env_prefix(extraEnv);
-    std::string cmd = prefix + command_from_argv(argv);
+    // wrap only — run_exec inherits stdio on purpose (see finalize_shell_command).
+    std::string cmd = wrap_for_cmd_c(prefix + command_from_argv(argv));
     return normalize_exit_code(std::system(cmd.c_str()));
 #endif
 }

src/platform/shell.cppm

@@ -20,6 +20,13 @@ export namespace mcpp::platform::shell {
 // Platform-aware shell argument quoting.
 std::string quote(std::string_view s);
 
+// The two halves of `quote`, callable regardless of host. Exposed so the
+// Windows command-line shape can be built and unit-tested from any platform
+// — the cmd.exe quoting rules are the easiest thing in mcpp to get wrong and
+// the hardest to notice, since a Linux/macOS run never executes that code.
+std::string quote_windows(std::string_view s);
+std::string quote_posix(std::string_view s);
+
 // Silent redirect — stdout + stderr → /dev/null (or NUL on Windows).
 // stdin is NOT touched here; that's the responsibility of
 // mcpp::platform::process::seal_stdin, which is auto-applied by capture /
@@ -36,25 +43,36 @@ constexpr std::string_view silent_redirect = ">/dev/null 2>&1";
 
 namespace mcpp::platform::shell {
 
-std::string quote(std::string_view s) {
+std::string quote_windows(std::string_view s) {
     std::string out;
     out.reserve(s.size() + 2);
-#if defined(_WIN32)
     out.push_back('"');
     for (char c : s) {
         if (c == '"') out += "\\\"";
         else out.push_back(c);
     }
     out.push_back('"');
-#else
+    return out;
+}
+
+std::string quote_posix(std::string_view s) {
+    std::string out;
+    out.reserve(s.size() + 2);
     out.push_back('\'');
     for (char c : s) {
         if (c == '\'') out += "'\\''";
         else out.push_back(c);
     }
     out.push_back('\'');
-#endif
     return out;
 }
 
+std::string quote(std::string_view s) {
+#if defined(_WIN32)
+    return quote_windows(s);
+#else
+    return quote_posix(s);
+#endif
+}
+
 } // namespace mcpp::platform::shell