From 32a0de1a073854b7989723c576561a03e86780bf Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Sat, 31 Oct 2026 16:45:53 +0100 Subject: [PATCH] fix: what an audit of the documented behaviour found An independent check of every behaviour AGENTS.md was to describe: - `--units ,` and `--only ,` skipped their empty items and so had no filter left: a typo started a run of every unit. They are usage errors now (exit 3). - Two fixes the plan lists were missing: a dry run now clears the prompts a dry run before it left in the dir they share (the report counted them), and `config` reports an absolute functional.mcp.command that is no program here - /usr/bin/npx inside the Flatpak, whose /usr is the Sdk's. - A patch that is not UTF-8 stops `run` at preflight (exit 3), as the plan has it; it exited 1. - An exception no command caught aborted the process (134), e.g. a home that holds no lenses: it ends with its message and exit 1. An error escaping `run` or `freeze` now also clears the signal handler's run and sweeps what is left below the subreaper. - `config -h` printed "unrecognized arguments" (exit 3): it prints the usage, exit 0. - An API seat's .claude.json is linked into place, so a second writer never replaces the first's file and what claude wrote to it since. Co-Authored-By: Claude Opus 5.5 --- implementations/Crafter.Review-Config.cpp | 13 ++++++++ implementations/Crafter.Review-Driver.cpp | 27 ++++++++++++++--- implementations/Crafter.Review-Run.cpp | 25 ++++++++++++--- implementations/Crafter.Review-Seats.cpp | 8 +++-- tests/Commands/main.cpp | 9 ++++++ tests/Config/main.cpp | 6 ++++ tests/Runs/main.cpp | 37 +++++++++++++++++++++++ 7 files changed, 114 insertions(+), 11 deletions(-) diff --git a/implementations/Crafter.Review-Config.cpp b/implementations/Crafter.Review-Config.cpp index f66eab2..ff988c1 100644 --- a/implementations/Crafter.Review-Config.cpp +++ b/implementations/Crafter.Review-Config.cpp @@ -10,6 +10,7 @@ import :Plan; import :Tools; import :PgWire; import :Process; +import :Platform; import std; namespace fs = std::filesystem; @@ -321,6 +322,18 @@ namespace Crafter::Review::Config { } const Json& args = Dyn::Get(Dyn::At(functional, "mcp"), "args"); if (!args.is_null() && !AllStrings(args)) out.emplace_back("functional.mcp.args must be null or a list of strings"); + // A path found on the host, such as /usr/bin/npx, is not there inside + // the Flatpak, whose /usr is the Sdk's. + const Json& command = Dyn::Get(Dyn::At(functional, "mcp"), "command"); + if (command.is_string() && command.get().starts_with('/')) { + std::error_code error; + const fs::path file(command.get()); + const bool executable = fs::is_regular_file(file, error) && (fs::status(file, error).permissions() & (fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec)) != fs::perms::none; + if (!executable) { + out.emplace_back(std::format("functional.mcp.command {} is no program here{}", Dyn::Repr(command.get()), + Platform::InSandbox() ? " (inside the Flatpak /usr is the Sdk's: name it `npx`, the bundled one)" : "")); + } + } return out; } diff --git a/implementations/Crafter.Review-Driver.cpp b/implementations/Crafter.Review-Driver.cpp index 62c851e..3321046 100644 --- a/implementations/Crafter.Review-Driver.cpp +++ b/implementations/Crafter.Review-Driver.cpp @@ -347,6 +347,13 @@ inventory unit, against a frozen commit range.)"; if (std::string id = Dyn::Strip(part); !id.empty()) a.units->insert(std::move(id)); } } + // Empty items are skipped, so a typo such as `--units ,` must not + // leave no filter at all: that is a full run. + auto namesNothing = [](const std::optional& list) { + return list && !list->empty() && std::ranges::all_of(Dyn::Split(*list, ","), [](const std::string& part) { return Dyn::Strip(part).empty(); }); + }; + if (namesNothing(a.only)) return UsageError(UsageLine(Prog, specs), Prog, std::format("argument --only: names no lens file ({})", Dyn::Repr(*a.only))); + if (namesNothing(parsed.Value("units"))) return UsageError(UsageLine(Prog, specs), Prog, std::format("argument --units: names no unit id ({})", Dyn::Repr(*parsed.Value("units")))); a.warm = parsed.Flag("warm"); if (const std::optional n = parsed.Value("concurrency")) a.concurrency = *Dyn::ParseInt(*n); a.effort = parsed.Value("effort").value_or(""); @@ -436,9 +443,12 @@ inventory unit, against a frozen commit range.)"; // `%G?` starts gpg, which can leave a daemon behind: as a subreaper // this process inherits it, and takes it down on the way out. Process::BecomeSubreaper(); - const std::int32_t code = Freeze::Main(arguments, *config); - Process::KillDescendants(); - return code; + struct Sweep { + ~Sweep() { + Process::KillDescendants(); + } + } const sweep; + return Freeze::Main(arguments, *config); } // ---- seats: the report, the ledger and the board beside it ------------------ @@ -929,7 +939,12 @@ inventory unit, against a frozen commit range.)"; if (command == "totp") return Totp::Main(after); if (command == "inventory" && !configPath) return Inventory::Main(after); if (command == "config") { - if (!after.empty()) return UsageError(Usage, "crafter-review", std::format("unrecognized arguments: {}", after.front())); + const Parsed parsed = ParseOptions(after, {}); + if (parsed.help) { + std::print("{}", Help("crafter-review config", "print the merged consumer config and its problems", {})); + return 0; + } + if (!parsed.error.empty()) return UsageError(Usage, "crafter-review", parsed.error); return Config::Main(fs::current_path(), configPath ? fs::path(*configPath) : fs::path()); } if (command == "freeze") return FreezeCommand(after, configPath); @@ -944,6 +959,10 @@ inventory unit, against a frozen commit range.)"; // An error no command catches: its message, exit 1. std::println(std::cerr, "crafter-review: {}: {}", error.Kind(), error.what()); return 1; + } catch (const std::exception& error) { + // Anything else that escapes a command, as a traceback would. + std::println(std::cerr, "crafter-review: {}", error.what()); + return 1; } return UsageError(Usage, "crafter-review", std::format("argument cmd: invalid choice: '{}'", command)); } diff --git a/implementations/Crafter.Review-Run.cpp b/implementations/Crafter.Review-Run.cpp index 679b5d8..b2795fe 100644 --- a/implementations/Crafter.Review-Run.cpp +++ b/implementations/Crafter.Review-Run.cpp @@ -390,6 +390,11 @@ namespace Crafter::Review::Run { built = Inventory::Build("commits", a.artifact); } catch (const Inventory::InvariantError& e) { Die(e.what()); + } catch (const Dyn::Error& e) { + // A patch that is not UTF-8 is no artifact to review: a preflight + // failure. + if (e.Kind() != "UnicodeDecodeError") throw; + Die(std::format("the artifact's patches are not UTF-8 ({}) - re-freeze", e.what())); } units = built.out.at("units"); lines = built.lines; @@ -449,6 +454,12 @@ namespace Crafter::Review::Run { } scratch = scratchRoot / std::format("{}-run-{}{}", h8, runN, a.dryRun ? "-dry" : ""); fs::create_directories(scratch); + if (a.dryRun) { + // Every dry run on a hash shares this dir: the report counts + // this run's prompts, not the last one's too. + std::error_code error; + fs::remove_all(scratch / "prompts", error); + } if (!a.dryRun) { // A stop meant for a run this dir held before is not this run's. std::error_code error; @@ -2161,6 +2172,16 @@ namespace Crafter::Review::Run { Process::BecomeSubreaper(); Run run(std::move(a), cfg); Current = &run; + // However this ends - a return, or an exception on its way to Main's + // handler - the signal handler loses its run, and whatever is left + // below this subreaper (a daemon a git call started) goes with it; + // never a process it did not start. + struct Cleanup { + ~Cleanup() { + Current = nullptr; + Process::KillDescendants(); + } + } const cleanup; // `crafter-review stop`: the file is looked at every 2 s, and a stop // is handled exactly like SIGTERM. std::jthread watcher([&run](const std::stop_token& token) { @@ -2189,10 +2210,6 @@ namespace Crafter::Review::Run { } watcher.request_stop(); watcher.join(); - Current = nullptr; - // Whatever is left below this subreaper - a daemon a git call - // started - goes with it; never a process it did not start. - Process::KillDescendants(); return code; } } diff --git a/implementations/Crafter.Review-Seats.cpp b/implementations/Crafter.Review-Seats.cpp index 78840bf..a84da78 100644 --- a/implementations/Crafter.Review-Seats.cpp +++ b/implementations/Crafter.Review-Seats.cpp @@ -441,8 +441,10 @@ namespace Crafter::Review::Seats { const std::size_t length = Dyn::Length(key); const std::string tail = Dyn::Slice(key, static_cast(length) - std::min(20, static_cast(length))); const Json approval = {{"hasCompletedOnboarding", true}, {"customApiKeyResponses", {{"approved", {tail}}, {"rejected", Json::array()}}}}; - // O_EXCL: two passes of a fresh seat write it in the same instant; the - // contents are the same, so the one that loses leaves the other's. + // Two passes of a fresh seat write it in the same instant. Written + // whole under another name, then linked into place, which fails when + // the file is there: the one that loses leaves the other's, and with + // it whatever claude has written to it since. const fs::path temp = std::format("{}.{}.{}.tmp", path.string(), Platform::PublishedPid(), TokenHex(4)); { std::ofstream out(temp, std::ios::binary); @@ -450,7 +452,7 @@ namespace Crafter::Review::Seats { } fs::permissions(temp, fs::perms::owner_read | fs::perms::owner_write, fs::perm_options::replace); std::error_code error; - if (!fs::exists(path)) fs::rename(temp, path, error); + link(temp.c_str(), path.c_str()); fs::remove(temp, error); return path; } diff --git a/tests/Commands/main.cpp b/tests/Commands/main.cpp index b54575d..786a4af 100644 --- a/tests/Commands/main.cpp +++ b/tests/Commands/main.cpp @@ -220,6 +220,15 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"ok","total && badFile.err.contains("UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte"), none.out + noFile.err + badFile.err); + // What escapes a command ends it with exit 1, never an abort; config's + // help. + const Process::Captured noHome = Self({"doc", "prepush"}, base, {{"CRAFTER_REVIEW_HOME", (base / "nowhere").string()}}); + const Process::Captured configHelp = Self({"config", "-h"}, base); + Check("an error no command catches ends it with exit 1 and its message, never an abort; config -h prints its usage, exit 0", + noHome.returnCode == 1 && noHome.err.starts_with("crafter-review: ") && noHome.err.contains("holds no process/lenses") && configHelp.returnCode == 0 + && configHelp.out.starts_with("usage: crafter-review config [-h]"), + noHome.err + configHelp.out + configHelp.err); + // browser: its usage (an install fetches from npm). const Process::Captured noAction = Self({"browser"}, base); const Process::Captured badAction = Self({"browser", "remove"}, base); diff --git a/tests/Config/main.cpp b/tests/Config/main.cpp index 7b1a994..f12f88d 100644 --- a/tests/Config/main.cpp +++ b/tests/Config/main.cpp @@ -64,6 +64,12 @@ int main() { && Any(problems, "aiTrailers") && Any(problems, "authors"), Show(problems)); problems = bad({{"functional", {{"extraTools", {"rm -rf /"}}, {"mcp", {{"args", "x"}}}}}}); Check("extraTools must be Bash(:*) or mcp__...; mcp.args a list", problems.size() == 2, Show(problems)); + // A path found on the host is not there inside the Flatpak. + const std::vector missingCommand = bad({{"functional", {{"mcp", {{"command", "/nonexistent/bin/npx"}}}}}}); + Check("functional.mcp.command: an absolute path that is no program here is a problem; a name, or a program that is there, is not", + missingCommand.size() == 1 && missingCommand[0].starts_with("functional.mcp.command '/nonexistent/bin/npx' is no program here") + && bad({{"functional", {{"mcp", {{"command", "/bin/sh"}}}}}}).empty() && bad({{"functional", {{"mcp", {{"command", "npx"}}}}}}).empty(), + Show(missingCommand)); std::ofstream(p, std::ios::binary) << "{not json"; Check("not JSON -> one problem", load().problems.size() == 1); Check("--config to a missing file is a problem", Any(Config::Load(td, td / "none.json").problems, "no such file")); diff --git a/tests/Runs/main.cpp b/tests/Runs/main.cpp index b9eb94e..82333da 100644 --- a/tests/Runs/main.cpp +++ b/tests/Runs/main.cpp @@ -264,6 +264,16 @@ echo "{\"type\":\"result\",\"subtype\":\"success\",\"is_error\":false,\"result\" dry.returnCode == 2 && all > gate1 && gate1 > 1 && !securityUnit.empty() && !proverUnit.empty() && calls() == 0 && Count(planned, "lens", "repo-functional") == 0, dry.out.substr(dry.out.size() > 2000 ? dry.out.size() - 2000 : 0) + dry.err); + // Dry runs with no full run between them share one dir: the next + // drops the last one's prompts. + const fs::path dryDir = scratchRoot / std::format("{}-run-1-dry", h8); + Write(dryDir / "prompts" / "stale.txt", "a prompt of another range"); + const Process::Captured again = run({"--dry-run"}); + std::int64_t prompts = 0; + for (const fs::directory_entry& entry : fs::directory_iterator(dryDir / "prompts")) prompts += entry.path().extension() == ".txt" ? 1 : 0; + Check("dry run: a second dry run on a hash counts its own prompts, the one before's gone", again.returnCode == 2 && !fs::exists(dryDir / "prompts" / "stale.txt") && prompts == all, + std::to_string(prompts)); + // Killed in its last gate: one security pass never answers, and the // driver is SIGKILLed once every other pass has. const fs::path scratchA = scratchRoot / std::format("{}-run-1", h8); @@ -364,6 +374,33 @@ echo "{\"type\":\"result\",\"subtype\":\"success\",\"is_error\":false,\"result\" Check("preflight: an env file that is not UTF-8 stops the run, exit 3, with the decoder's own words", badEnv.returnCode == 3 && badEnv.err.contains("preflight: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte"), badEnv.err); + // A filter that names nothing is refused: skipping its empty items + // must not leave no filter, which is a full run. + const Process::Captured noUnits = run({"--units", ","}); + const Process::Captured noLenses = run({"--only", " , "}); + Check("--units and --only that name nothing are usage errors (exit 3) - never a full run", + noUnits.returnCode == 3 && noUnits.err.contains("argument --units: names no unit id (',')") && noLenses.returnCode == 3 + && noLenses.err.contains("argument --only: names no lens file (' , ')"), + noUnits.err + noLenses.err); + + // A patch that is not UTF-8 is no artifact: a preflight stop. + const fs::path badArtifact = repo / ".review-gates" / "push" / "bad"; + fs::copy(artifact, badArtifact, fs::copy_options::recursive); + for (const fs::directory_entry& entry : fs::directory_iterator(badArtifact)) { + if (entry.path().extension() == ".patch") std::ofstream(entry.path(), std::ios::binary | std::ios::app) << std::string("\xff\n", 2); + } + const Process::Captured badPatch = Process::Run({Platform::SelfExe().string(), "run", "--artifact", ".review-gates/push/bad", "--base", baseSha, "--trees", ".", "--dispositions", + ".review-gates/push/bad/gate-state/dispositions.json", "--claude-bin", fake.string(), "--scratch", scratchRoot.string(), "--dry-run"}, + repo, std::chrono::seconds(60), {}, Env); + Check("preflight: a patch that is not UTF-8 stops the run, exit 3, saying so", + badPatch.returnCode == 3 && badPatch.err.contains("the artifact's patches are not UTF-8 ('utf-8' codec can't decode byte 0xff"), badPatch.err); + + // An error that escapes a run ends it with its message, exit 1, + // through the run's cleanup. + const Process::Captured badReserve = run({"--no-reuse", "--no-salvage"}, {{"REVIEW_GATES_MEM_RESERVE_MB", "x"}}); + Check("an error escaping a full run (a reserve that is no number) ends it with its ValueError, exit 1", + badReserve.returnCode == 1 && badReserve.err.contains("crafter-review: ValueError: could not convert string to float: 'x'"), badReserve.err); + // A stop between two gates: the next gate's pool starts aborted and // runs nothing of it. {