From c6b4d782456bf011a7fa9ada925a3e9052591cd4 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Mon, 2 Nov 2026 13:57:20 +0100 Subject: [PATCH] feat(freeze): no authors in the config, no author check A repository that takes commits from people it cannot list has no way to freeze a range today: a null `authors` falls back to `git config user.email`, so every foreign author or committer fails. Null, or no key, now means neither is checked; the freeze warns that it did not check them and records `authors` as null. A list is checked as before. An empty list, which no commit could pass, is a config error that points at null. Released as 1.1.0. --- README.md | 2 +- implementations/Crafter.Review-Config.cpp | 26 +++++++------------ implementations/Crafter.Review-Freeze.cpp | 23 ++++++++-------- interfaces/Crafter.Review-Config.cppm | 7 +++-- interfaces/Crafter.Review-Freeze.cppm | 3 ++- .../net.catcrafts.CrafterReview.metainfo.xml | 8 ++++++ process/prepush.md | 7 ++--- process/skill/prepush/SKILL.md | 2 +- tests/Config/main.cpp | 11 ++++---- tests/Freeze/main.cpp | 12 ++++++--- 10 files changed, 52 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 1669236..4c4b8e8 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ crafter-review config # edit .review-gates/config.json until it prints no `.review-gates/config.json` is committed. It says: -- what `freeze` checks: `authors`, `aiTrailers`, `requireSigned`, and the branch a range is measured against (`base`, `rangeFrom`); +- what `freeze` checks: `authors` (null: not checked), `aiTrailers`, `requireSigned`, and the branch a range is measured against (`base`, `rangeFrom`); - `modes.`: the kind of artifact, the lenses it runs and their effort. Every lens is off until a mode lists it; `run --mode` picks one; - `lenses.`: the environment a lens gets (`none`, `browser`, `supabase` or `browser+supabase`) and, if the repository has one, its `section` - a file of this repository's own appended to the lens's charter; - `functional`, `slots`, `supabase`: how the application is started and reached, one stack or several, and where its database answers; diff --git a/implementations/Crafter.Review-Config.cpp b/implementations/Crafter.Review-Config.cpp index f2b539b..a766e84 100644 --- a/implementations/Crafter.Review-Config.cpp +++ b/implementations/Crafter.Review-Config.cpp @@ -9,7 +9,6 @@ import :Core; import :Plan; import :Tools; import :PgWire; -import :Process; import :Platform; import :Prompt; import std; @@ -151,8 +150,8 @@ namespace Crafter::Review::Config { // when HEAD tracks nothing; base: always merge-base HEAD // ..HEAD, the whole pull request. {"rangeFrom", "upstream"}, - // Author and committer emails the freeze accepts; null: the - // repository's `git config user.email`. + // Author and committer emails the freeze accepts; null: neither + // is checked. {"authors", nullptr}, // forbid | allow: Assisted-by / Co-Authored-By trailers in the // range. @@ -266,6 +265,8 @@ namespace Crafter::Review::Config { return a.is_string() && a.get_ref().contains('@'); }))) { out.emplace_back("authors must be null or a list of email addresses"); + } else if (authors.is_array() && authors.empty()) { + out.emplace_back("authors is an empty list, which no commit passes - null leaves author and committer unchecked"); } for (const std::string_view key : {"allowDirty", "gate0"}) { if (!AllStrings(Dyn::At(config, key))) out.push_back(std::format("{} must be a list of strings", key)); @@ -453,21 +454,12 @@ namespace Crafter::Review::Config { return copy; } - std::string GitUserEmail(const fs::path& repo) { - const Process::Captured run = Process::Run({"git", "-C", repo.string(), "config", "user.email"}); - return run.returnCode == 0 ? Dyn::Strip(Dyn::UniversalNewlines(Dyn::DecodeReplace(run.out))) : std::string(); - } - - std::vector Authors(const Json& config, const fs::path& repo) { + std::optional> Authors(const Json& config) { const Json& authors = Dyn::Get(config, "authors"); - if (Dyn::Truthy(authors)) { - std::vector out; - for (const Json& author : authors) out.push_back(Dyn::Str(author)); - return out; - } - const std::string email = GitUserEmail(repo); - if (email.empty()) return {}; - return {email}; + if (!authors.is_array()) return std::nullopt; + std::vector out; + for (const Json& author : authors) out.push_back(Dyn::Str(author)); + return out; } std::map LensSectionsOf(const Json& config, const fs::path& repo) { diff --git a/implementations/Crafter.Review-Freeze.cpp b/implementations/Crafter.Review-Freeze.cpp index e111ddc..a1dc4b5 100644 --- a/implementations/Crafter.Review-Freeze.cpp +++ b/implementations/Crafter.Review-Freeze.cpp @@ -211,20 +211,18 @@ namespace Crafter::Review::Freeze { fail(std::format("{} is not the toplevel of a git checkout", repo)); return report; } - std::vector authors = options.authors; - if (authors.empty()) authors = Config::Authors({{"authors", nullptr}}, repo); - if (authors.empty()) { - fail("no author identity: set `authors` in .review-gates/config.json or git config user.email"); - return report; - } + const std::optional>& authors = options.authors; const Base base = ResolveBase(repo, options.base, options.configBase, options.rangeFrom); if (!base.failure.empty()) { fail(base.failure); return report; } const std::string head = Artifact::TreeHead(repo); - Json authorList = Json::array(); - for (const std::string& author : authors) authorList.push_back(author); + Json authorList; + if (authors) { + authorList = Json::array(); + for (const std::string& author : *authors) authorList.push_back(author); + } report["base"] = base.sha; report["baseFrom"] = base.how; report["upstream"] = base.upstream ? Json(*base.upstream) : Json(); @@ -261,11 +259,12 @@ namespace Crafter::Review::Freeze { commitList.push_back({{"sha", c.sha}, {"subject", c.subject}, {"author", c.author}, {"committer", c.committer}, {"sig", c.sig}}); } report["commits"] = commitList; - const std::string authorText = Dyn::Join(authors, ", "); + if (!authors) warn("no authors in .review-gates/config.json: author and committer not checked"); + const std::string authorText = authors ? Dyn::Join(*authors, ", ") : std::string(); for (const Commit& c : commits) { const std::string who = std::format("{} {}", Head12(c.sha), Dyn::Repr(Dyn::Slice(c.subject, 0, 50))); - if (!std::ranges::contains(authors, c.author)) fail(std::format("{}: author {} not in {}", who, c.author, authorText)); - if (!std::ranges::contains(authors, c.committer)) fail(std::format("{}: committer {} not in {}", who, c.committer, authorText)); + if (authors && !std::ranges::contains(*authors, c.author)) fail(std::format("{}: author {} not in {}", who, c.author, authorText)); + if (authors && !std::ranges::contains(*authors, c.committer)) fail(std::format("{}: committer {} not in {}", who, c.committer, authorText)); if (const std::optional trailer = AiTrailer(c.body)) { const bool forbid = options.aiTrailers == "forbid"; const std::string message = std::format("{}: AI attribution trailer {}{}", who, Dyn::Repr(Dyn::Strip(*trailer)), @@ -472,7 +471,7 @@ namespace Crafter::Review::Freeze { options.replace = arguments.replace; options.worktree = arguments.worktree; options.requireSigned = arguments.requireSigned || Dyn::Truthy(Dyn::Get(config, "requireSigned")); - options.authors = Config::Authors(config, repo); + options.authors = Config::Authors(config); options.aiTrailers = Dyn::Str(Dyn::Get(config, "aiTrailers")); options.allowDirty = allowDirty; const Json& configBase = Dyn::Get(config, "base"); diff --git a/interfaces/Crafter.Review-Config.cppm b/interfaces/Crafter.Review-Config.cppm index d51bbe2..461905a 100644 --- a/interfaces/Crafter.Review-Config.cppm +++ b/interfaces/Crafter.Review-Config.cppm @@ -36,10 +36,9 @@ export namespace Crafter::Review::Config { // The config for a dump: the database URL with its password masked, no // `_` bookkeeping keys. Json Redacted(const Json& config); - // The emails the freeze accepts: the configured list, else the - // repository's git identity. - std::vector Authors(const Json& config, const std::filesystem::path& repo); - std::string GitUserEmail(const std::filesystem::path& repo); + // The emails the freeze accepts as author and committer: the configured + // list; none (null, or no `authors` key) - the freeze checks neither. + std::optional> Authors(const Json& config); // The lens files no mode lists (what runs nowhere: the slots and init). std::vector LensesOff(const Json& config); // {lens file: environment} for every lens of the table, as the config diff --git a/interfaces/Crafter.Review-Freeze.cppm b/interfaces/Crafter.Review-Freeze.cppm index d95d1eb..f4f5faa 100644 --- a/interfaces/Crafter.Review-Freeze.cppm +++ b/interfaces/Crafter.Review-Freeze.cppm @@ -56,7 +56,8 @@ export namespace Crafter::Review::Freeze { bool replace = false; bool worktree = false; bool requireSigned = false; - std::vector authors; + // The emails author and committer must be among; none: not checked. + std::optional> authors; std::string aiTrailers = "forbid"; std::vector allowDirty; std::optional configBase; diff --git a/packaging/flatpak/net.catcrafts.CrafterReview.metainfo.xml b/packaging/flatpak/net.catcrafts.CrafterReview.metainfo.xml index 8e249d7..7d66405 100644 --- a/packaging/flatpak/net.catcrafts.CrafterReview.metainfo.xml +++ b/packaging/flatpak/net.catcrafts.CrafterReview.metainfo.xml @@ -27,6 +27,14 @@ + + +

New

+
    +
  • Leaving authors out of the config turns the freeze's author and committer check off, where it used to check git config user.email. An empty list is a config error.
    • +
+ +

Fixes

diff --git a/process/prepush.md b/process/prepush.md index 5ac75bf..f571fad 100644 --- a/process/prepush.md +++ b/process/prepush.md @@ -136,7 +136,7 @@ delta `@{upstream}..HEAD`, the rule for a repository that pushes to its own trunk; `base`: always the merge-base with `base`, the whole pull request for a repository whose branches become PRs — their commits must be linear, rebased, never a merge of the integration branch), `authors` (the emails the -freeze accepts; null = `git config user.email`), `aiTrailers` (`forbid` or +freeze accepts; null = neither author nor committer is checked), `aiTrailers` (`forbid` or `allow`), `requireSigned`, `allowDirty` (tracked files the local stack rewrites in place while the lenses run), `gate0` (the commands Gate 0 runs by hand, for the skill), `modes` (`modes.`: the @@ -209,8 +209,9 @@ default), `notes`. output where a commit message's "tested" claim can point at it (`--context-paths`). 2. **Freeze** — `freeze` checks, all of them, before it - exports anything: author AND committer are in `authors`; AI attribution - trailers (`Assisted-by:`, `Co-Authored-By:`) by the `aiTrailers` rule; the + exports anything: author AND committer are in `authors` (when it is + set); AI attribution trailers (`Assisted-by:`, `Co-Authored-By:`) by the + `aiTrailers` rule; the tracked tree is clean (the `allowDirty` paths aside, warned); no merge commit in the range (`git format-patch` drops them silently, so the artifact would not be the push); the range is a fast-forward of diff --git a/process/skill/prepush/SKILL.md b/process/skill/prepush/SKILL.md index dd4a925..84d28b7 100644 --- a/process/skill/prepush/SKILL.md +++ b/process/skill/prepush/SKILL.md @@ -1,6 +1,6 @@ --- name: prepush -description: Pre-push gates for a repository set up for crafter-review (`crafter-review init`). Runs the pipeline's procedure end to end — Gate 0 build/tests + freeze (identity, trailers by the config's rule, clean tree, no merges, fast-forward), the application and the database under test brought up from the frozen tree, the driver run of the lenses the config turns on - every lens is off until it does (repo-better-solution → repo-functional with the Playwright MCP → repo-bug-prover and repo-security side by side → repo-facts, the commit truth) via crafter-review, the fix/rerun loop, the owner's approval of the hash, verify, push. Nothing is pushed unreviewed or unapproved. +description: Pre-push gates for a repository set up for crafter-review (`crafter-review init`). Runs the pipeline's procedure end to end — Gate 0 build/tests + freeze (identity when `authors` is set, trailers by the config's rule, clean tree, no merges, fast-forward), the application and the database under test brought up from the frozen tree, the driver run of the lenses the config turns on - every lens is off until it does (repo-better-solution → repo-functional with the Playwright MCP → repo-bug-prover and repo-security side by side → repo-facts, the commit truth) via crafter-review, the fix/rerun loop, the owner's approval of the hash, verify, push. Nothing is pushed unreviewed or unapproved. --- Follow the pipeline's procedure, `prepush.md`, EXACTLY — read it now, in diff --git a/tests/Config/main.cpp b/tests/Config/main.cpp index 853d815..6590592 100644 --- a/tests/Config/main.cpp +++ b/tests/Config/main.cpp @@ -233,12 +233,11 @@ int main() { && ex.at("modes").at("commits").at("lenses").size() == 5 && ex.at("modes").at("series").at("lenses").size() == 10 && ex.at("modes").at("reply").at("lenses").size() == 5 && ex.at("modes").at("commits").at("effort") == "xhigh" && ex.at("modes").at("series").at("effort") == "max" && ex.at("modes").at("reply").at("effort") == "max" && std::ranges::none_of(ex.at("lenses"), [](const Json& entry) { return entry.contains("run"); }), Show(Config::Validate(ex))); - const fs::path repo = td / "r"; - fs::create_directories(repo); - Process::Run({"git", "-C", repo.string(), "init", "-q"}); - Process::Run({"git", "-C", repo.string(), "config", "user.email", "me@example.com"}); - Check("authors: the configured list, else git config user.email", Config::Authors({{"authors", {"a@b.c"}}}, repo) == std::vector{"a@b.c"} - && Config::Authors({{"authors", nullptr}}, repo) == std::vector{"me@example.com"}); + Check("authors: the configured list; null or no key - none, the freeze checks neither author nor committer (2026-11-02, not the git identity)", + Config::Authors({{"authors", {"a@b.c"}}}) == std::vector{"a@b.c"} && !Config::Authors({{"authors", nullptr}}) && !Config::Authors(Json::object()) + && !Config::Authors(Config::Defaults())); + problems = bad({{"authors", Json::array()}}); + Check("an empty authors list is the problem that says null turns the check off", problems.size() == 1 && problems[0].contains("empty list") && problems[0].contains("null"), Show(problems)); std::ofstream(td / "u.json", std::ios::binary) << "{}"; Check("login_fixture: the first candidate that exists", Config::LoginFixture({{"functional", {{"login", {{"fixtures", {"nope.json", "u.json"}}}}}}}, td) == "u.json" && !Config::LoginFixture({{"functional", {{"login", {{"fixtures", Json::array()}}}}}}, td)); diff --git a/tests/Freeze/main.cpp b/tests/Freeze/main.cpp index 9bbe53e..930b959 100644 --- a/tests/Freeze/main.cpp +++ b/tests/Freeze/main.cpp @@ -79,14 +79,18 @@ int main() { if (options.date.empty()) options.date = "2026-10-31"; return Freeze::Run(repo, out, options); }; - Json r = freeze({.base = base}); + Json r = freeze({.base = base, .authors = std::vector {me}}); std::string failures = Joined(r.at("failures")); - Check("trailer (forbid) + foreign author both named, nothing exported; the author list came from git config", !r.at("ok").get() && failures.contains("Co-Authored-By") + Check("trailer (forbid) + foreign author both named, nothing exported", !r.at("ok").get() && failures.contains("Co-Authored-By") && failures.contains("catbot@bot.local") && !fs::exists(out) && r.at("authors") == Json::array({me}), failures); - r = freeze({.base = base, .aiTrailers = "allow"}); + r = freeze({.base = base, .authors = std::vector {me}, .aiTrailers = "allow"}); Check("trailer (allow): a warning, the foreign author still fails", !r.at("ok").get() && Joined(r.at("failures")).contains("catbot") && !Joined(r.at("failures")).contains("Co-Authored-By") && AnyContains(r.at("warnings"), "Co-Authored-By")); - r = freeze({.base = base, .authors = {me, "catbot@bot.local"}, .aiTrailers = "allow"}); + r = freeze({.base = base}); + Check("no authors (null in the config): author and committer unchecked, said in a warning - only the trailer fails", !r.at("ok").get() + && Joined(r.at("failures")).contains("Co-Authored-By") && !Joined(r.at("failures")).contains("catbot") && r.at("authors").is_null() + && AnyContains(r.at("warnings"), "author and committer not checked"), Joined(r.at("failures"))); + r = freeze({.base = base, .authors = std::vector {me, "catbot@bot.local"}, .aiTrailers = "allow"}); Check("both emails in authors + allow -> the range freezes (3 commits)", r.at("ok").get() && r.at("files").size() == 3, Joined(r.at("failures"))); g({"reset", "-q", "--hard", clean}); r = freeze({.base = base, .replace = true});