From 4fc15fe1c2339e21f11dcb3c790c2796e7605027 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Sat, 31 Oct 2026 21:31:48 +0100 Subject: [PATCH] feat(run): the repository under review is the freeze record's, not the working directory's A directory can review repositories other than itself: a workbench whose config, env file and records serve several, freezing each with --repo and --out. The run took the working directory for the repository under review: the prompt named it, the calibration was kept under it, and --trees fell back to it. The run now reads the repository from the artifact's freeze.json - the prompt names it and the calibration is kept under it - and --trees defaults to what the record says: the worktree, else the repository when it is not the working directory, else `.`, the same rule that builds the printed run line. The working directory keeps its roles: the config, the env file, the slots, the lens processes' cwd and every logged path. A damaged record stops the run at preflight. --- AGENTS.md | 1 + README.md | 4 ++ implementations/Crafter.Review-Freeze.cpp | 33 +++++++++++++---- implementations/Crafter.Review-Run.cpp | 19 +++++++--- interfaces/Crafter.Review-Freeze.cppm | 12 ++++++ interfaces/Crafter.Review-Run.cppm | 7 ++++ process/prepush.md | 4 +- tests/Freeze/main.cpp | 22 +++++++++++ tests/Runs/main.cpp | 45 +++++++++++++++++++++++ 9 files changed, 133 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 55e3e69..6053e0f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -98,6 +98,7 @@ Conventions: - **Messages that point at a pid.** In the sandbox, the run-lock message says "wait for it or stop it (` stop `)", and `seats` adds a line naming `stop`; natively it says "wait for it or kill it". The pid inside a sandbox ignores SIGTERM. - **The dump's `driver` field and the first log line** read `crafter-review (prompt v1)`. Nothing reads them. - **`freeze.json` records `gitVersion`.** When the hashes differ, `freeze --verify`'s MISMATCH names a git version change as the likely cause (Freeze). +- **The repository under review is the freeze record's**, not the working directory's: `Run::reviewed` is named in the prompt and keys the calibration, and `--trees` defaults to the record's worktree, else its repository when that is not the working directory, else `.` (`Freeze::ReviewedBy`, `TreeOf`). The working directory stays `Run::repo`: the config, `.env`, the slots, the lens processes' cwd and every logged path. That is how a workbench reviews other repositories with `freeze --repo` (Freeze, Runs). - **`config` reports an absolute `functional.mcp.command` that is no program here**, such as `/usr/bin/npx` inside the Flatpak, whose `/usr` is the Sdk's. `config` then exits 1, and a run stops at preflight (Config). - **JSON edge cases.** - `NaN` and `Infinity` are refused when read: a claude stream, opencode or seat-probe line that holds one is skipped. They are written as `null`. diff --git a/README.md b/README.md index 2d6937b..7bd66d6 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,10 @@ crafter-review run --artifact .review-gates/push// --hash \ The dump with every finding, note and coverage entry is `gate-state/commits--run-.json`. +### Reviewing another repository + +A directory can review repositories other than itself: a workbench whose config, `.env` and records serve several. From it, `crafter-review freeze --repo --out --worktree`, then the printed run line. The run reads the repository under review from the artifact's `freeze.json`: the prompt names it, the calibration is kept under it, and `--trees` defaults to the recorded worktree. The config, the env file and the slots are the working directory's, the lenses run in it, and `--context-paths` are relative to it. + To stop a run, run `crafter-review stop `, or `crafter-review stop --hash ` for every live run of a hash. Its running passes are killed, no later gate starts and the dump is written. A run takes its passes with it however it ends, SIGKILL included: each pass runs under a supervisor that kills its whole tree, the MCP server and Chromium included. `flatpak kill net.catcrafts.CrafterReview` is the hard stop. | Command | What it does | diff --git a/implementations/Crafter.Review-Freeze.cpp b/implementations/Crafter.Review-Freeze.cpp index 8cfbd14..e111ddc 100644 --- a/implementations/Crafter.Review-Freeze.cpp +++ b/implementations/Crafter.Review-Freeze.cpp @@ -423,6 +423,30 @@ namespace Crafter::Review::Freeze { Dyn::Str(Dyn::At(report, "base")), trees, Dyn::JoinPath(Dyn::JoinPath(outRel, "gate-state"), "dispositions.json"), Dyn::Str(Dyn::At(report, "date"))); } + std::string TreeOf(const Json& record, const fs::path& cwd) { + const Json& worktree = Dyn::Get(record, "worktree"); + if (Dyn::Truthy(worktree)) return Dyn::Str(worktree); + const Json& repo = Dyn::Get(record, "repo"); + if (!Dyn::Truthy(repo)) return "."; + std::error_code error; + return fs::weakly_canonical(fs::path(Dyn::Str(repo)), error) == fs::weakly_canonical(cwd, error) ? std::string(".") : Dyn::Str(repo); + } + + Reviewed ReviewedBy(const fs::path& artifact, const fs::path& cwd) { + const fs::path file = artifact / "freeze.json"; + std::error_code error; + if (!fs::exists(file, error)) return {cwd, "."}; + Json record; + try { + record = Dyn::LoadJsonFile(file); + } catch (const std::exception& e) { + throw std::runtime_error(std::format("{}: not a freeze record ({})", file.string(), e.what())); + } + if (!record.is_object()) throw std::runtime_error(std::format("{}: not a freeze record", file.string())); + const Json& repo = Dyn::Get(record, "repo"); + return {Dyn::Truthy(repo) ? fs::path(Dyn::AbsPath(Dyn::Str(repo))) : cwd, TreeOf(record, cwd)}; + } + std::int32_t Main(const Arguments& arguments, const Json& config) { const std::string cwd = fs::current_path().string(); const std::string repo = Dyn::AbsPath(arguments.repo && !arguments.repo->empty() ? *arguments.repo : cwd); @@ -467,14 +491,7 @@ namespace Crafter::Review::Freeze { std::println("frozen: {} commit(s) {}..{} (base from {}) -> {}/ ({})", files.size(), Head12(report.at("base").get()), Head12(report.at("head").get()), report.at("baseFrom").get(), rel, Dyn::Join(files, ", ")); std::println("hash: {}", report.at("hash").get()); - std::string tree; - if (Dyn::Truthy(Dyn::Get(report, "worktree"))) { - tree = report.at("worktree").get(); - } else { - std::error_code error; - tree = fs::weakly_canonical(repo, error) == fs::weakly_canonical(cwd, error) ? std::string(".") : report.at("repo").get(); - } - const std::string line = RunLine(report, rel, tree); + const std::string line = RunLine(report, rel, TreeOf(report, cwd)); std::ofstream(fs::path(report.at("out").get()) / "run-line.txt", std::ios::binary) << line << "\n"; std::println("run (from the repository root, in a detached background shell; also in {}):\n {}", Dyn::JoinPath(rel, "run-line.txt"), line); return 0; diff --git a/implementations/Crafter.Review-Run.cpp b/implementations/Crafter.Review-Run.cpp index 0b610b2..431d016 100644 --- a/implementations/Crafter.Review-Run.cpp +++ b/implementations/Crafter.Review-Run.cpp @@ -238,7 +238,7 @@ namespace Crafter::Review::Run { return {0, {}}; } const fs::path root = scratchRoot && !scratchRoot->empty() ? *scratchRoot : fs::path(ScratchRoot(std::nullopt)); - const Memory::Calibration calib(root / "memory.json", repo, DefaultCosts()); + const Memory::Calibration calib(root / "memory.json", artifact ? Freeze::ReviewedBy(*artifact, repo).repo : repo, DefaultCosts()); std::optional best; for (const std::string& e : appEnvs) { Memory::Calibration::Cost cost = calib.CostOf(e); @@ -307,7 +307,7 @@ namespace Crafter::Review::Run { } // ---- the Run --------------------------------------------------------------------------- - Run::Run() : repo(fs::current_path()), t0(Platform::Now()) { + Run::Run() : repo(fs::current_path()), reviewed(repo), t0(Platform::Now()) { log = [this](const std::string& message) { Message(message); }; appReady = [](std::string_view url, std::string_view path) { return AppReady(url, path); }; probe = [](std::string_view url) { return PgWire::Probe(url); }; @@ -483,6 +483,8 @@ namespace Crafter::Review::Run { } logFile = scratch / "log.txt"; log(std::format("crafter-review {} prompt v{}: commits {} @ {}", Version(), PromptVersion, Dyn::RelPath(a.artifact, repo.string()), hash)); + std::error_code canonical; + if (fs::weakly_canonical(reviewed, canonical) != fs::weakly_canonical(repo, canonical)) log(std::format("repository under review: {} (read at {})", reviewed.string(), trees.front())); log(std::format("config: {}", Dyn::Truthy(Dyn::Get(cfg, "_path")) ? Dyn::Str(cfg.at("_path")) : std::string("defaults (no .review-gates/config.json)"))); for (const Json& w : Dyn::Or(Dyn::Get(cfg, "_warnings"), Dyn::EmptyList())) log(std::format("config: warning: {}", Dyn::Str(w))); std::vector lensParts; @@ -560,7 +562,7 @@ namespace Crafter::Review::Run { systemText = Prompt::SystemText(); systemPaths.clear(); systemByLens.clear(); - runLayer = Prompt::RunLayer(Dyn::RelPath(a.artifact, repo.string()), a.base, a.date, trees, hash, lines, units, contextPaths, Basename(repo)); + runLayer = Prompt::RunLayer(Dyn::RelPath(a.artifact, repo.string()), a.base, a.date, trees, hash, lines, units, contextPaths, Basename(reviewed)); // The application under test: the login fixture, the TOTP helper. const Json& fcfg = cfg.at("functional"); loginFixture = Config::LoginFixture(cfg, repo); @@ -570,7 +572,7 @@ namespace Crafter::Review::Run { schemas = {{"block", Schema::SchemaJson("block")}, {"whole", Schema::SchemaJson("whole")}}; // Memory: every pass's tree measured, its peak recorded per repository // and environment; the budget per gate. - calibration = std::make_unique(scratchRoot / "memory.json", repo, DefaultCosts()); + calibration = std::make_unique(scratchRoot / "memory.json", reviewed, DefaultCosts()); if (!a.dryRun) { // A pass runs under the supervisor: its tree is claude's, the // supervisor itself left out, so a peak measures claude's tree @@ -1685,7 +1687,14 @@ namespace Crafter::Review::Run { // ---- the run ------------------------------------------------------------------------------ std::int32_t Run::Execute() { - trees = CommaList(a.trees.value_or(".")); + Freeze::Reviewed frozen; + try { + frozen = Freeze::ReviewedBy(a.artifact, repo); + } catch (const std::runtime_error& e) { + Die(e.what()); + } + reviewed = frozen.repo; + trees = CommaList(a.trees.value_or(frozen.tree)); contextPaths = CommaList(a.contextPaths); for (const std::string& c : contextPaths) { if (!fs::exists(c)) Die(std::format("--context-paths: {} does not exist", c)); diff --git a/interfaces/Crafter.Review-Freeze.cppm b/interfaces/Crafter.Review-Freeze.cppm index fb69cb6..d95d1eb 100644 --- a/interfaces/Crafter.Review-Freeze.cppm +++ b/interfaces/Crafter.Review-Freeze.cppm @@ -76,6 +76,18 @@ export namespace Crafter::Review::Freeze { // The exact `run` command for a freeze report: built from its fields, never // retyped (2026-09-29). std::string RunLine(const Json& report, std::string_view outRel, std::string_view trees); + // The tree the lenses read for a frozen artifact, as its record says: the + // worktree (relative to the directory freeze ran from), else the + // repository when it is not `cwd`, else `.`. + std::string TreeOf(const Json& record, const std::filesystem::path& cwd); + // What an artifact's freeze.json says it reviews: the repository, absolute + // (`cwd` when the artifact has no record, or its record names none), and + // the tree (TreeOf). A record that is not JSON throws. + struct Reviewed { + std::filesystem::path repo; + std::string tree; + }; + Reviewed ReviewedBy(const std::filesystem::path& artifact, const std::filesystem::path& cwd); struct Arguments { std::optional repo; diff --git a/interfaces/Crafter.Review-Run.cppm b/interfaces/Crafter.Review-Run.cppm index 24f10a7..030cb64 100644 --- a/interfaces/Crafter.Review-Run.cppm +++ b/interfaces/Crafter.Review-Run.cppm @@ -175,7 +175,14 @@ export namespace Crafter::Review::Run { // {lens file: environment}, in the config's lens order: the dump, the // resume file and the scratch dir's marker write it in that order. Json environments = Json::object(); + // The working directory: the config, the env file and the slots are + // its, the lens processes run in it, and every path logged is relative + // to it. std::filesystem::path repo; + // The repository under review, from the artifact's freeze record (the + // working directory when it has none): the prompt names it, and the + // calibration is kept under it. + std::filesystem::path reviewed; std::vector logs; double t0 = 0; std::optional logFile; diff --git a/process/prepush.md b/process/prepush.md index d0a55bb..0f2ad2f 100644 --- a/process/prepush.md +++ b/process/prepush.md @@ -27,7 +27,9 @@ hash at Gate 5. - The repository at the range HEAD: the checkout itself (`--trees .`; the driver verifies HEAD and a clean tree before every launch) or a detached worktree under `.review-gates/trees/-/` (`freeze --worktree`; - gitignored) — content, history and blame; the state before the range via + gitignored; `--trees` defaults to what the freeze record says, so a + workbench that froze another repository with `--repo` reads its worktree) + — content, history and blame; the state before the range via `git show :`. - `--context-paths` for the evidence dirs the commit messages' claims rest on. - The lens charter (`process/lenses/repo-*.md`, system text diff --git a/tests/Freeze/main.cpp b/tests/Freeze/main.cpp index d2f8dd5..9bbe53e 100644 --- a/tests/Freeze/main.cpp +++ b/tests/Freeze/main.cpp @@ -170,6 +170,28 @@ int main() { g({"merge", "-q", "--no-ff", "-m", "merge side", "side"}); const Json r3 = freeze({.base = base, .replace = true}); Check("merge in the range refused", !r3.at("ok").get() && AnyContains(r3.at("failures"), "merge commit")); + // What a run reads from the record: the repository under review and the + // tree the lenses read (a workbench freezing another repository with + // --repo: the record names it, not the working directory). + const Json withTree = {{"repo", repo.string()}, {"worktree", "x/y"}}; + const Json repoOnly = {{"repo", repo.string()}}; + Check("TreeOf: the record's worktree, else the repository when it is not the working directory, else `.`", + Freeze::TreeOf(withTree, td) == "x/y" && Freeze::TreeOf(repoOnly, td) == repo.string() && Freeze::TreeOf(repoOnly, repo) == "." && Freeze::TreeOf(Json::object(), td) == "."); + const Freeze::Reviewed byRecord = Freeze::ReviewedBy(out, td); + const fs::path none = td / "no-record"; + fs::create_directories(none); + const Freeze::Reviewed byNone = Freeze::ReviewedBy(none, td); + std::ofstream(none / "freeze.json", std::ios::binary) << "{broken"; + bool damaged = false; + try { + Freeze::ReviewedBy(none, td); + } catch (const std::runtime_error& e) { + damaged = std::string(e.what()).contains("not a freeze record"); + } + Check("ReviewedBy: the record's repository and its tree; no record: the working directory and `.`; a damaged record throws, saying so", + fs::weakly_canonical(byRecord.repo) == fs::weakly_canonical(repo) && byRecord.tree == Dyn::AbsPath(repo.string()) && Freeze::ReviewedBy(out, repo).tree == "." && byNone.repo == td + && byNone.tree == "." && damaged, + std::format("{} {} {}", byRecord.repo.string(), byRecord.tree, byNone.tree)); g({"worktree", "remove", "--force", wt.string()}); fs::remove_all(td); return Failures ? 1 : 0; diff --git a/tests/Runs/main.cpp b/tests/Runs/main.cpp index 01163cf..2f0a7d1 100644 --- a/tests/Runs/main.cpp +++ b/tests/Runs/main.cpp @@ -448,6 +448,51 @@ echo "{\"type\":\"result\",\"subtype\":\"success\",\"is_error\":false,\"result\" std::format("exit {}, {} call(s), {}", truth.returnCode, calls() - before, dumpT.is_object() ? Text(dumpT.at("overall")) : truth.out + truth.err)); } + // A workbench (2026-10-31): a directory that reviews another + // repository - its own config and env file, the artifact wherever + // --out says, the lenses' tree the frozen worktree. The run reads the + // repository under review from the freeze record: the prompt names + // it, not the working directory, and --trees defaults to the + // recorded worktree. + { + const fs::path bench = base / "bench"; + fs::create_directories(bench / ".review-gates"); + Write(bench / ".review-gates" / "config.json", R"({"lenses": {"repo-bug-prover": {"run": true}}})"); + const fs::path out = bench / "journal" / "push" / "y"; + const Process::Captured benchFreeze = Process::Run({Platform::SelfExe().string(), "freeze", "--repo", repo.string(), "--out", out.string(), "--worktree", "--base", baseSha}, + bench, std::chrono::seconds(60), {}, Env); + Json benchRecord; + try { + benchRecord = Dyn::LoadJsonFile(out / "freeze.json"); + } catch (const std::exception&) { + } + const std::string benchHash = benchRecord.is_object() ? Text(benchRecord.at("hash")) : std::string(); + const std::string benchTree = benchRecord.is_object() ? Text(Dyn::Get(benchRecord, "worktree")) : std::string(); + Check("a workbench freezes another repository: the record names it, and the worktree is under the workbench's .review-gates/trees/", + benchFreeze.returnCode == 0 && benchRecord.is_object() && Text(benchRecord.at("repo")) == Dyn::AbsPath(repo.string()) + && benchTree == std::format(".review-gates/trees/repo-{}", benchHash.substr(0, 8)) && fs::is_directory(bench / benchTree), + benchFreeze.out + benchFreeze.err); + const fs::path benchScratch = base / "bench-scratch"; + const Process::Captured benchRun = Process::Run({Platform::SelfExe().string(), "run", "--artifact", "journal/push/y", "--hash", benchHash, "--base", baseSha, "--dispositions", + "journal/push/y/gate-state/dispositions.json", "--date", Text(freezeJson.at("date")), "--claude-bin", fake.string(), "--scratch", + benchScratch.string(), "--seats", "local", "--no-probe", "--dry-run"}, + bench, std::chrono::seconds(60), {}, Env); + const fs::path benchDry = benchScratch / std::format("{}-run-1-dry", benchHash.substr(0, 8)); + // The run layer is in the system prompts (system-.md). + std::string systems; + std::error_code benchError; + for (fs::directory_iterator it(benchDry, benchError), end; !benchError && it != end; it.increment(benchError)) { + const std::string name = it->path().filename().string(); + if (name.starts_with("system-") && name.ends_with(".md")) systems += Platform::ReadFile(it->path()).value_or(""); + } + const std::string benchLog = Platform::ReadFile(benchDry / "log.txt").value_or(""); + Check("...and a dry run from the workbench without --trees (exit 2, as every dry run) reads the recorded worktree, names the repository under review in the run layer, not the workbench, and logs it", + benchRun.returnCode == 2 && !systems.empty() + && systems.contains(std::format("Repository under review (repo), checked out at the range HEAD (the last commit of the artifact): {} -", benchTree)) && !systems.contains("(bench)") + && benchLog.contains(std::format("repository under review: {} (read at {})", Dyn::AbsPath(repo.string()), benchTree)), + std::format("exit {}, {} bytes of system prompt; {}{}", benchRun.returnCode, systems.size(), benchRun.out, benchRun.err)); + } + // A stop between two gates: the next gate's pool starts aborted and // runs nothing of it. {