Catcrafts/Crafter.Review
Watch
2
0
Fork
You've already forked Crafter.Review
 0
Code Issues 7 Pull requests Projects Releases 3 Packages Wiki Activity Actions

fix: what an audit of the documented behaviour found

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Jorijn van der Graaf 2026-10-31 16:45:53 +01:00
commit 32a0de1a07
Signed by: jorijnvdgraaf
GPG key ID: 2937E59CDCC1BCFB
7 changed files with 114 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

13
implementations/Crafter.Review-Config.cpp
View file

@ -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<std::string>().starts_with('/')) {
std::error_code error;
const fs::path file(command.get<std::string>());
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<std::string>()),
Platform::InSandbox() ? " (inside the Flatpak /usr is the Sdk's: name it `npx`, the bundled one)" : ""));
}
}
return out;
}

27
implementations/Crafter.Review-Driver.cpp
View file

@ -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<std::string>& 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<std::string> 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));
}

25
implementations/Crafter.Review-Run.cpp
View file

@ -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;
}
}

8
implementations/Crafter.Review-Seats.cpp
View file

@ -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<std::int64_t>(length) - std::min<std::int64_t>(20, static_cast<std::int64_t>(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;
}