review fixes: the tail prefilter, the scan pass's options, the xlings upgrade, the GCC map

- A `**/` also matches no directory, so the literal tail of `**/name` does not
  start with its slash; a root-level match of `**/main.cpp` was dropped.
- Under -k the scans run in the main pass; the scan pass takes
  MCPP_NINJA_DEBUG, and -j precedes its goal.
- The versions of the vendored xlings's candidate sources are read through the
  memo, and an upgrade copies beside the binary and renames over it, so a
  failed copy keeps the old binary.
- The GCC module map also lists a name a unit imports that no unit provides,
  at GCC's own path; the clang and MSVC paths use native separators.

Author: speak-agent

.agents/docs/2026-09-30-build-wall-time-progress-count-and-hang-plan.md

@@ -972,6 +972,23 @@ without a new staleness class.
 - Revert probes: the W1 property test, e2e 846 and e2e 847 each fail on
   2026.9.30.1.
 
+### 10.5 Review of the implementation
+
+An independent review of the pull request's diff found the following, each
+resolved as stated.
+
+| Finding | Resolution |
+|---|---|
+| W8: the literal-tail prefilter dropped a root-level match of `**/name`, since `**/` also matches no directory | The `/` after a `**` is not part of the tail; `Scanner.ADoubleStarSlashTailMatchesAtTheRoot` |
+| W2: the scan pass took neither `-k` nor `MCPP_NINJA_DEBUG`, and put `-j` after its goal | Under `-k` the scans run in the main pass; `-d` is passed; `-j` precedes the goal |
+| W3: the candidates' versions were asked without the memo on the path where the vendored binary is behind the pin | They are read through the memo |
+| W3: a failed copy during an upgrade removed the working binary | The copy is written beside the binary and renamed over it; a failure keeps the old one |
+| W10: the GCC map listed only names a unit provides | A name a unit of the package imports that no unit provides is listed at GCC's own path |
+| W10: the clang and MSVC paths mixed separators | Native separators |
+| W10: the GCC map's relative path in the compile database | Not a defect: the database's directory is the build directory (`compile_commands.cppm:484`) |
+| W2: the phase is one value for the whole command, so two configurations built at once show the phase of the pass that began last | Recorded; cosmetic |
+| W8: a file symlink whose target is deleted during planning stays listed | Recorded; the per-pattern walk had the same window between its walk and its use |
+
 ## 8. Appendix: readings
 
 - **Clean builds, wall time (s).**

src/build/ninja_backend.cppm

@@ -4338,16 +4338,25 @@ std::expected<BuildResult, BuildError> NinjaBackend::build(const BuildPlan& plan
     std::string out;
     int ninjaExit = 0;
     bool scanFailed = false;
-    if (goalArg.empty()
+    // Under `-k` the build goes on past a failure and states every one, so
+    // the scans run in the main pass there: a failed scan pass would end the
+    // build at its first failure, and running both would state a failed scan
+    // twice.
+    if (goalArg.empty() && !opts.keepGoing
         && manifest.find("\nbuild " + std::string(kScannedGoal) + " : phony") != std::string::npos) {
         const auto scanDeadline =
             std::chrono::milliseconds(static_cast<long long>(opts.buildTimeoutSecs) * 1000);
+        // The main pass's options, and its goal last.
         std::vector<std::string> scan{ninjaProgram};
         if (!opts.verbose && !opts.progress) scan.push_back("--quiet");
         scan.insert(scan.end(), {std::string("-C"), plan.outputDir.string()});
         if (opts.verbose) scan.push_back("-v");
-        scan.push_back(std::string(kScannedGoal));
+        if (const char* topics = std::getenv("MCPP_NINJA_DEBUG"); topics && *topics) {
+            scan.push_back("-d");
+            scan.push_back(topics);
+        }
         if (opts.parallelJobs) scan.push_back(std::format("-j{}", opts.parallelJobs));
+        scan.push_back(std::string(kScannedGoal));
         if (opts.progress) {
             auto run = run_ninja_reporting(scan, nenv, scanDeadline, *opts.progress, opts.verbose,
                                            command_prefixes(flags, plan),

src/build/plan.cppm

@@ -3224,12 +3224,23 @@ make_plan(const mcpp::manifest::Manifest&         manifest,
                     if (collided.contains(name)) bound.emplace_back(name, path);
                 }
                 if (bound.empty()) continue;   // sees no collided name
-                // GCC's mapper answers only what it lists: the standard
-                // library modules are listed where GCC's own mapper puts them.
+                // GCC's mapper answers only what it lists. The standard
+                // library modules, and any name a unit of the package imports
+                // that no unit of the graph provides (a BMI placed by other
+                // means), are listed where GCC's own mapper puts them.
                 if (gcc) {
-                    entries.emplace_back("std", std::string(traits.bmiDir) + "/" + basename("std"));
-                    entries.emplace_back("std.compat",
-                                         std::string(traits.bmiDir) + "/" + basename("std.compat"));
+                    std::set<std::string, std::less<>> listed;
+                    for (auto const& [name, path] : entries) listed.insert(name);
+                    auto flat = [&](const std::string& name) {
+                        if (listed.insert(name).second)
+                            entries.emplace_back(name, std::string(traits.bmiDir) + "/" + basename(name));
+                    };
+                    flat("std");
+                    flat("std.compat");
+                    for (auto const& cu : plan.compileUnits)
+                        if (cu.packageName == pkg)
+                            for (auto const& imp : cu.imports)
+                                if (!graph.providersOf.contains(imp)) flat(imp);
                 }
                 std::sort(entries.begin(), entries.end());
                 ModuleScope scope;
@@ -3251,7 +3262,9 @@ make_plan(const mcpp::manifest::Manifest&         manifest,
                     const bool separate = !prefix.empty() && prefix.back() == ' ';
                     if (separate) prefix.remove_suffix(1);
                     for (auto const& [name, path] : bound) {
-                        const auto value = name + "=" + (outputDir / path).string();
+                        auto bmi = outputDir / std::filesystem::path(path);
+                        bmi.make_preferred();
+                        const auto value = name + "=" + bmi.string();
                         if (separate) {
                             flags.push_back(std::string(prefix));
                             flags.push_back(mcpp::manifest::flag_element(value));

src/fallback/xlings_binary.cppm

@@ -56,8 +56,10 @@ struct XlingsSource {
 std::optional<XlingsSource> choose_xlings_source(std::optional<XlingsSource> override_,
                                                  std::optional<XlingsSource> released,
                                                  std::optional<XlingsSource> onPath);
-// The same choice over this process's sources.
-std::optional<XlingsSource> select_xlings_source(const std::filesystem::path& destBin = {});
+// The same choice over this process's sources. Their versions are read
+// through `versionMemo` (known_xlings_version).
+std::optional<XlingsSource> select_xlings_source(const std::filesystem::path& destBin = {},
+                                                 const std::filesystem::path& versionMemo = {});
 
 // The version `bin` answers, asked at most once per process. With `memoFile`,
 // the answer is also kept across processes, keyed by the binary's path, size
@@ -120,7 +122,7 @@ acquire_xlings_binary(const std::filesystem::path& destBin, bool quiet = false,
         // re-acquired, which replaced 2026.8.2.1 with the system's 0.4.51 --
         // older still, and equally missing the feature the check exists to
         // restore. Look before leaping.
-        auto candidate = select_xlings_source(destBin);
+        auto candidate = select_xlings_source(destBin, versionMemo);
         if (!candidate || candidate->version.empty()
             || !version_is_older(have, candidate->version)) {
             // stderr, not stdout. This is a remark about the environment,
@@ -138,15 +140,21 @@ acquire_xlings_binary(const std::filesystem::path& destBin, bool quiet = false,
             settled.insert(destBin);
             return destBin;
         }
+        // Beside the binary first and renamed over it, so a copy that fails
+        // (a full disk, a running binary) leaves the old one in place.
         std::error_code rec;
-        std::filesystem::copy_file(candidate->path, destBin,
+        auto staged = destBin;
+        staged += ".new";
+        std::filesystem::copy_file(candidate->path, staged,
             std::filesystem::copy_options::overwrite_existing, rec);
-        if (!rec) {
-            std::filesystem::permissions(destBin,
+        if (!rec)
+            std::filesystem::permissions(staged,
                 std::filesystem::perms::owner_exec
               | std::filesystem::perms::group_exec
               | std::filesystem::perms::others_exec,
               std::filesystem::perm_options::add, rec);
+        if (!rec) std::filesystem::rename(staged, destBin, rec);
+        if (!rec) {
             if (!quiet && !updated)
                 std::println(stderr,
                              "{:>12} vendored xlings {} -> {} from {} (pinned {})",
@@ -158,8 +166,11 @@ acquire_xlings_binary(const std::filesystem::path& destBin, bool quiet = false,
             settled.insert(destBin);
             return destBin;
         }
-        // The copy failed; the chain below tries again from the start.
-        std::filesystem::remove(destBin, rec);
+        // The copy failed: the old binary stays, as it would with no source.
+        std::error_code sec;
+        std::filesystem::remove(staged, sec);
+        settled.insert(destBin);
+        return destBin;
     }
 
     std::error_code ec;
@@ -177,7 +188,7 @@ acquire_xlings_binary(const std::filesystem::path& destBin, bool quiet = false,
     // The first acquisition takes the source a replacement would take
     // (select_xlings_source): the override, otherwise the newer of the
     // released copy and the PATH copy.
-    if (auto src = select_xlings_source(destBin)) {
+    if (auto src = select_xlings_source(destBin, versionMemo)) {
         std::filesystem::copy_file(src->path, destBin,
             std::filesystem::copy_options::overwrite_existing, ec);
         if (!ec) {
@@ -290,22 +301,24 @@ std::optional<XlingsSource> choose_xlings_source(std::optional<XlingsSource> ove
     return released;
 }
 
-std::optional<XlingsSource> select_xlings_source(const std::filesystem::path& destBin) {
+std::optional<XlingsSource> select_xlings_source(const std::filesystem::path& destBin,
+                                                 const std::filesystem::path& versionMemo) {
     std::optional<XlingsSource> override_, released, onPath;
     std::error_code ec;
     if (const char* e = std::getenv("MCPP_VENDORED_XLINGS"); e && *e) {
         std::filesystem::path p{e};
         if (std::filesystem::exists(p, ec))
-            override_ = XlingsSource{p, vendored_xlings_version(p), "MCPP_VENDORED_XLINGS"};
+            override_ = XlingsSource{p, known_xlings_version(p, versionMemo), "MCPP_VENDORED_XLINGS"};
     }
     if (!override_) {
         if (auto r = released_xlings_source(destBin); !r.empty())
-            released = XlingsSource{r, vendored_xlings_version(r), "the release of this mcpp"};
+            released = XlingsSource{r, known_xlings_version(r, versionMemo),
+                                    "the release of this mcpp"};
         if (auto sys = mcpp::platform::fs::which(
                 std::string("xlings") + std::string(mcpp::platform::exe_suffix))) {
             const bool isDest = !destBin.empty() && std::filesystem::equivalent(*sys, destBin, ec);
             if (!isDest && std::filesystem::exists(*sys, ec))
-                onPath = XlingsSource{*sys, vendored_xlings_version(*sys), "PATH"};
+                onPath = XlingsSource{*sys, known_xlings_version(*sys, versionMemo), "PATH"};
         }
     }
     return choose_xlings_source(std::move(override_), std::move(released), std::move(onPath));

src/modgraph/scanner.cppm

@@ -692,9 +692,14 @@ std::vector<std::filesystem::path> expand_glob_one(const std::filesystem::path&
     if (!fs::exists(start, startEc)) return out;
 
     // Every match ends with the text after the glob's last `*`, which the
-    // matcher reads literally: a cheap test that turns most entries away.
+    // matcher reads literally: a cheap test that turns most entries away. A
+    // `**/` also matches no directory at all (`**/main.cpp` matches a root
+    // `main.cpp`), so the `/` after a `**` is not part of the tail.
     const auto star = glob.find_last_of('*');
-    const std::string_view tail = star == std::string_view::npos ? glob : glob.substr(star + 1);
+    std::string_view tail = star == std::string_view::npos ? glob : glob.substr(star + 1);
+    if (star != std::string_view::npos && star > 0 && glob[star - 1] == '*'
+        && tail.starts_with('/'))
+        tail.remove_prefix(1);
     auto listing = tree_listing(root, start);
     for (auto const& f : listing->files) {
         if (!f.relative) continue;

tests/unit/test_modgraph.cpp

@@ -1433,3 +1433,19 @@ TEST(ModuleResolution, OneFileReachedTwiceIsNamedAsSuchWhateverItsSpelling) {
     EXPECT_NE(errors.find("one file is reached as two packages"), std::string::npos) << errors;
     std::filesystem::remove_all(dir);
 }
+
+// A kept walk matched by the literal tail of a glob: `**/` also matches no
+// directory, so `**/main.cpp` matches a `main.cpp` at the root as well as one
+// below it, as the per-pattern walk did.
+TEST(Scanner, ADoubleStarSlashTailMatchesAtTheRoot) {
+    auto dir = make_tempdir("mcpp-glob-tail");
+    write(dir / "main.cpp", "int main() {}\n");
+    write(dir / "sub" / "main.cpp", "int main() {}\n");
+    write(dir / "sub" / "other.cpp", "int x;\n");
+    auto files = expand_glob(dir, "**/main.cpp");
+    ASSERT_EQ(files.size(), 2u);
+    EXPECT_EQ(files[0], dir / "main.cpp");
+    EXPECT_EQ(files[1], dir / "sub" / "main.cpp");
+    std::filesystem::remove_all(dir);
+}
+