diff --git a/README.md b/README.md index c70dbd6..edb670f 100644 --- a/README.md +++ b/README.md @@ -45,10 +45,10 @@ crafter-review config # edit .review-gates/config.json until it prints no `init` does four things: - it writes `.review-gates/config.json` from the template when there is none - the three artifact kinds as modes with every lens of their kind: trim each to what this repository runs; - it adds `.review-gates/push/` and `.review-gates/trees/` to `.gitignore`; -- it writes the `/prepush` skill to `.claude/skills/prepush/SKILL.md`, which a later `init` refreshes (the line after its frontmatter marks it as init's); +- it writes the `/prepush` skill to `.claude/skills/prepush/SKILL.md`, which a later `init` refreshes (the line after its frontmatter marks it as init's), with a `.gitignore` of `*` beside it: the skill is generated from the shipped text, so it stays out of git and every checkout runs `init` once (a tracked copy would lag the installed binary); a skill the repository wrote itself, without that line, is left alone and not ignored, unless `--force`; - it prints the next steps. -It writes nothing in a subdirectory, in your home directory (`--force` when that is the repository) or over a `SKILL.md` it did not write. +It writes nothing in a subdirectory, in your home directory or over a `SKILL.md`, a `.gitignore` beside it or a link there that it did not make (the old link into the review-gates submodule aside, which it removes); `--force` allows all but the first. `.review-gates/config.json` is committed. It says what the pipeline must know about the repository: - who may author its commits, and which branch a range is measured against; @@ -66,6 +66,7 @@ It writes nothing in a subdirectory, in your home directory (`--force` when that 1. Re-freeze every review in flight with crafter-review. The Flatpak's git may be another version than the one that froze it, and a different git version gives a different hash. The old `run-line.txt` also points into the submodule. 2. `git submodule deinit -f review-gates && git rm review-gates` 3. `crafter-review init`. If `.claude/skills/prepush` is the old link into `review-gates/`, it removes the link, never writing through it, and makes a real directory with the skill. +4. If a copy of the skill that an earlier `init` wrote is tracked, `git rm --cached .claude/skills/prepush/SKILL.md` (init says so when it sees one): the skill is generated, and the `.gitignore` init writes beside it keeps it out of git from then on. An existing `config.json` needs one change since 2026-11-01: `lenses..run` is retired, so list the lenses in `modes.commits.lenses` (`config` names what is missing, and `freeze`, `run` and `slots` refuse until it is there). The reuse keys keep working: a range reviewed before the move is reused, not bought again. diff --git a/implementations/Crafter.Review-Driver.cpp b/implementations/Crafter.Review-Driver.cpp index 73ed6f1..cddb026 100644 --- a/implementations/Crafter.Review-Driver.cpp +++ b/implementations/Crafter.Review-Driver.cpp @@ -804,7 +804,7 @@ inventory unit, against a frozen commit range.)"; // ---- init: a repository set up for the gates --------------------------------------------- constexpr std::array InitOptions = {{ - {"--force", false, "", "replace a SKILL.md or a link init did not make, and run from your home directory"}, + {"--force", false, "", "replace a SKILL.md, the .gitignore beside it or a link init did not make, and run from your home directory"}, }}; // After the skill's frontmatter: what tells a later `init` the file is // its own to refresh. @@ -818,12 +818,15 @@ inventory unit, against a frozen commit range.)"; } return std::format("{}{}", marker, skill); } + // The first line of the .gitignore init writes beside the skill it + // owns: what tells a later `init` that file is its own. + constexpr std::string_view IgnoreMarker = "# written by crafter-review init"; std::int32_t InitCommand(std::span args) { constexpr std::string_view Prog = "crafter-review init"; const Parsed parsed = ParseOptions(args, InitOptions); if (parsed.help) { - std::print("{}", Help(Prog, "set this repository up for the pre-push gates: .review-gates/config.json from the template, the artifact dirs ignored, the prepush skill", InitOptions)); + std::print("{}", Help(Prog, "set this repository up for the pre-push gates: .review-gates/config.json from the template, the artifact dirs ignored, the prepush skill with the .gitignore that keeps it out of git", InitOptions)); return 0; } if (!parsed.error.empty()) return UsageError(UsageLine(Prog, InitOptions), Prog, parsed.error); @@ -846,20 +849,37 @@ inventory unit, against a frozen commit range.)"; // The skill's place. The old setup linked .claude/skills/prepush // into the review-gates submodule: the link goes, never written // through, which would change the submodule's tracked file and - // fail the next freeze on a dirty tree. Any other link stays - // unless --force - checked before anything is written. + // fail the next freeze on a dirty tree. Any other link - the + // directory, SKILL.md or the .gitignore init writes beside it, + // dangling or not - stays unless --force, and nothing is written + // through it: checked before anything is written. const fs::path skillDir = cwd / ".claude" / "skills" / "prepush"; const fs::path skill = skillDir / "SKILL.md"; + const fs::path skillIgnore = skillDir / ".gitignore"; std::vector> links; - for (const fs::path& link : {skillDir, skill}) { + for (const fs::path& link : {skillDir, skill, skillIgnore}) { if (!fs::is_symlink(fs::symlink_status(link, error))) continue; const fs::path target = fs::read_symlink(link, error); - if (!force && !target.string().contains("review-gates")) { + // The old setup linked the skill's directory or its SKILL.md - + // never the .gitignore - into the review-gates submodule at the + // toplevel: a link that resolves under it, not one that merely + // names it (the pipeline's own .review-gates/ does too). + const fs::path resolved = (link.parent_path() / target).lexically_normal(); + const fs::path submodule = (cwd / "review-gates").lexically_normal(); + const bool oldSetup = link != skillIgnore && std::mismatch(submodule.begin(), submodule.end(), resolved.begin(), resolved.end()).first == submodule.end(); + if (!force && !oldSetup) { return refuse(std::format("{} is a link to {}, not into the review-gates submodule: --force replaces it", link.lexically_relative(cwd).string(), target.string())); } links.emplace_back(link, target); } std::println("init: {}", cwd.string()); + // The links go first, before anything is written: one init cannot + // remove refuses, and nothing is written through it. + for (const auto& [link, target] : links) { + fs::remove(link, error); + if (error) return refuse(std::format("cannot remove the link {}: {}", link.lexically_relative(cwd).string(), error.message())); + std::println(" removed the link {} -> {}", link.lexically_relative(cwd).string(), target.string()); + } // The config, from the template, only when there is none. Written // fresh, not copied: the Flatpak's files are read-only. const fs::path config = cwd / ".review-gates" / "config.json"; @@ -889,20 +909,45 @@ inventory unit, against a frozen commit range.)"; std::println(" added {} to .gitignore", Dyn::Join(added, " and ")); } // The skill. - for (const auto& [link, target] : links) { - fs::remove(link, error); - std::println(" removed the link {} -> {}", link.lexically_relative(cwd).string(), target.string()); - } fs::create_directories(skillDir); const std::optional shipped = Platform::ReadFile(Home() / "process" / "skill" / "prepush" / "SKILL.md"); if (!shipped) return refuse(std::format("cannot read the skill under {}", Home().string())); const std::optional existing = Platform::ReadFile(skill); - if (existing && !existing->contains(InitMarker) && !force) { + const bool owned = !existing || existing->contains(InitMarker) || force; + if (!owned) { std::println(" kept .claude/skills/prepush/SKILL.md (init did not write it: --force replaces it)"); } else { std::ofstream(skill, std::ios::binary) << WithMarker(*shipped); std::println(" {} .claude/skills/prepush/SKILL.md", existing ? "refreshed" : "wrote "); } + // The skill init owns stays out of git (Jorijn, 2026-11-01: every + // consumer leaves it untracked, one init per checkout; generated + // and refreshed by every init, a tracked copy lags the installed + // binary). Its directory ignores itself, as a generated directory + // does - Python's venv, pytest's cache: a .gitignore of `*` beside + // the skill, that file included, on every checkout that runs + // init, whatever the repository's own rules or one machine's say, + // and nothing of the repository's is edited for it. A tracked + // copy init wrote gets the step that untracks it. A skill the + // repository wrote itself is its own and stays visible: the + // ignore file an earlier init wrote beside it goes. An ignore + // file init did not write is kept, unless --force. + const std::optional ignoring = Platform::ReadFile(skillIgnore); + const bool initIgnores = ignoring && ignoring->contains(IgnoreMarker); + if (owned) { + if (ignoring && !initIgnores && !force) { + std::println(" kept .claude/skills/prepush/.gitignore (init did not write it: --force replaces it)"); + } else { + std::ofstream(skillIgnore, std::ios::binary) << std::format("{}: the generated /prepush skill stays out of git; `init` refreshes this file while this line is in it\n*\n", IgnoreMarker); + std::println(" {} .claude/skills/prepush/.gitignore (the generated skill ignores itself)", ignoring ? "refreshed" : "wrote "); + } + if (Process::Run({"git", "-C", cwd.string(), "ls-files", "--error-unmatch", ".claude/skills/prepush/SKILL.md"}).returnCode == 0) { + std::println(" note .claude/skills/prepush/SKILL.md is tracked - `git rm --cached .claude/skills/prepush/SKILL.md` untracks it (the skill is generated; init refreshes it)"); + } + } else if (initIgnores) { + fs::remove(skillIgnore, error); + std::println(" removed .claude/skills/prepush/.gitignore (an earlier init's: the skill is the repository's own)"); + } // What comes next. const Config::Loaded loaded = Config::Load(cwd); const std::vector off = Config::LensesOff(loaded.config); diff --git a/process/prepush.md b/process/prepush.md index 2bdb72a..5ac75bf 100644 --- a/process/prepush.md +++ b/process/prepush.md @@ -666,3 +666,12 @@ repository. smoke config, this file, the shipped skill and the manifest template: every reader of a setting must agree (2026-10-12), and the run lock still keyed on the mode's name where the gate-state is the kind's. +- 2026-11-01 — **the generated skill stays untracked** (Jorijn, as + Crafter.Build was set up: a consumer that ignored `.claude` wholesale + never saw the skill in git, and whether it should was the question): + every consumer leaves `.claude/skills/prepush/SKILL.md` out of git, one + `init` per checkout. `init` writes a `.gitignore` of `*` beside the + skill, as a generated directory does, and edits nothing of the + repository's for it: the first self-review's gate on the repository's + own ignore rules was reworked twice in one range (a tracked path, one + machine's rule) before the gate itself went (2026-10-31's rule). diff --git a/tests/Commands/main.cpp b/tests/Commands/main.cpp index bf27778..c416bb8 100644 --- a/tests/Commands/main.cpp +++ b/tests/Commands/main.cpp @@ -85,18 +85,23 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"ok","total }; const std::string shipped = Read(checkout / "process" / "skill" / "prepush" / "SKILL.md"); const std::string marker = "\nold text\n", marker)); const Process::Captured again = Self({"init"}, r); - Check("init again: the config and an entry already ignored are kept, the missing one added once, the skill it wrote refreshed", + Check("init again: the config and an entry already ignored are kept, the missing one added once, the skill it wrote refreshed, and the .gitignore beside it", again.returnCode == 0 && Read(r / ".review-gates" / "config.json") == R"({"base": "origin/dev"})" && Read(r / ".gitignore") == "node_modules/\n/.review-gates/push\n.review-gates/trees/\n" && Read(r / ".claude" / "skills" / "prepush" / "SKILL.md") == skill - && again.out.contains("refreshed .claude/skills/prepush/SKILL.md"), + && again.out.contains("refreshed .claude/skills/prepush/SKILL.md") && again.out.contains("refreshed .claude/skills/prepush/.gitignore") + && Read(r / ".claude" / "skills" / "prepush" / ".gitignore") == skillIgnore, again.out + again.err); + // The repository's own .gitignore is never edited for the skill: one + // that ignores .claude wholesale (Crafter.Build) is kept as it is. + Write(r / ".gitignore", "node_modules/\n/.review-gates/push\n.review-gates/trees/\n.claude\n"); + const Process::Captured wholesale = Self({"init"}, r); + Check("init: a repository that ignores .claude wholesale keeps its .gitignore as it is, and the skill its own .gitignore", + wholesale.returnCode == 0 && Read(r / ".gitignore") == "node_modules/\n/.review-gates/push\n.review-gates/trees/\n.claude\n" && wholesale.out.contains("kept .gitignore") + && Read(r / ".claude" / "skills" / "prepush" / ".gitignore") == skillIgnore, + wholesale.out + wholesale.err); + // A copy init wrote that the repository tracks (a base-era init + // ignored nothing): the note says how to untrack it, on every init. + // A hand-written tracked skill is kept and not ignored - the ignore + // file the earlier init wrote beside it goes - with no note. + const fs::path tracked = repo("tracked"); + Self({"init"}, tracked); + Process::Run({"git", "-C", tracked.string(), "add", "-f", ".claude/skills/prepush/SKILL.md", ".gitignore"}, std::nullopt, std::chrono::seconds(60), {}, Env); + Process::Run({"git", "-C", tracked.string(), "-c", "user.email=me@example.com", "-c", "user.name=Me", "commit", "-q", "-m", "track the skill"}, std::nullopt, std::chrono::seconds(60), {}, Env); + const Process::Captured trackedInit = Self({"init"}, tracked); + const Process::Captured trackedAgain = Self({"init"}, tracked); + Write(tracked / ".claude" / "skills" / "prepush" / "SKILL.md", "my own skill\n"); + Process::Run({"git", "-C", tracked.string(), "-c", "user.email=me@example.com", "-c", "user.name=Me", "commit", "-q", "-am", "my own skill"}, std::nullopt, std::chrono::seconds(60), {}, Env); + const Process::Captured handWritten = Self({"init"}, tracked); + const bool stillIgnored = + Process::Run({"git", "-C", tracked.string(), "check-ignore", "--no-index", "-q", ".claude/skills/prepush/SKILL.md"}, std::nullopt, std::chrono::seconds(60), {}, Env).returnCode == 0; + Check("init on a tracked skill: one init wrote gets the note with `git rm --cached` on every init; one the repository wrote is kept and no rule ignores it any more - the earlier init's .gitignore beside it removed - and no note", + trackedInit.returnCode == 0 && trackedInit.out.contains("git rm --cached .claude/skills/prepush/SKILL.md") && trackedAgain.returnCode == 0 && trackedAgain.out.contains("git rm --cached") + && handWritten.returnCode == 0 && handWritten.out.contains("kept .claude/skills/prepush/SKILL.md") && handWritten.out.contains("removed .claude/skills/prepush/.gitignore") + && !fs::exists(tracked / ".claude" / "skills" / "prepush" / ".gitignore") && !stillIgnored && !handWritten.out.contains("git rm --cached"), + std::format("exits {} {} {}, ignored {}; {}{}{}{}", trackedInit.returnCode, trackedAgain.returnCode, handWritten.returnCode, stillIgnored, trackedInit.out, trackedAgain.out, handWritten.out, + handWritten.err)); Write(r / ".claude" / "skills" / "prepush" / "SKILL.md", "my own skill\n"); const Process::Captured kept = Self({"init"}, r); const std::string mine = Read(r / ".claude" / "skills" / "prepush" / "SKILL.md"); + const bool keptIgnoreFile = fs::exists(r / ".claude" / "skills" / "prepush" / ".gitignore"); const Process::Captured forced = Self({"init", "--force"}, r); - Check("init: a SKILL.md init did not write is kept; --force replaces it", - kept.returnCode == 0 && mine == "my own skill\n" && kept.out.contains("--force replaces it") && forced.returnCode == 0 && Read(r / ".claude" / "skills" / "prepush" / "SKILL.md") == skill, + Check("init: a SKILL.md init did not write is kept, and the .gitignore init wrote beside it goes; --force replaces the skill and writes the .gitignore again", + kept.returnCode == 0 && mine == "my own skill\n" && kept.out.contains("--force replaces it") && !keptIgnoreFile && forced.returnCode == 0 + && Read(r / ".claude" / "skills" / "prepush" / "SKILL.md") == skill && Read(r / ".claude" / "skills" / "prepush" / ".gitignore") == skillIgnore, kept.out + forced.out); + // A .gitignore beside the skill that init did not write is kept; + // --force replaces it. + Write(r / ".claude" / "skills" / "prepush" / ".gitignore", "my own rules\n"); + const Process::Captured foreignIgnore = Self({"init"}, r); + const std::string ownRules = Read(r / ".claude" / "skills" / "prepush" / ".gitignore"); + const Process::Captured forcedIgnore = Self({"init", "--force"}, r); + Check("init: a .gitignore beside the skill that init did not write is kept, saying so; --force replaces it", + foreignIgnore.returnCode == 0 && ownRules == "my own rules\n" && foreignIgnore.out.contains("kept .claude/skills/prepush/.gitignore (init did not write it: --force replaces it)") + && forcedIgnore.returnCode == 0 && Read(r / ".claude" / "skills" / "prepush" / ".gitignore") == skillIgnore, + foreignIgnore.out + forcedIgnore.out); // The old setup: the skill dir a link into the submodule. const fs::path old = repo("old"); @@ -158,6 +205,69 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"ok","total && atHome.err.contains("is your home directory") && homeUntouched && atHomeForced.returnCode == 0 && plain.returnCode == 3 && plain.err.contains("is not in a git checkout"), refused.err + sub.err + atHome.err + plain.err); + // A .gitignore beside the skill that is a link - to a file outside + // the skill's directory, or dangling - is a link like the skill's + // others: refused before anything is written, never written through; + // --force replaces the link with init's file and leaves its target as + // it was. + const fs::path linked = repo("linked"); + Write(linked / "outside.ignore", "keep me\n"); + fs::create_directories(linked / ".claude" / "skills" / "prepush"); + fs::create_symlink("../../../outside.ignore", linked / ".claude" / "skills" / "prepush" / ".gitignore"); + const Process::Captured linkRefused = Self({"init"}, linked); + const bool linkUntouched = !fs::exists(linked / ".review-gates") && Read(linked / "outside.ignore") == "keep me\n"; + const Process::Captured linkForced = Self({"init", "--force"}, linked); + std::error_code linkError; + const bool replaced = !fs::is_symlink(fs::symlink_status(linked / ".claude" / "skills" / "prepush" / ".gitignore", linkError)) + && Read(linked / ".claude" / "skills" / "prepush" / ".gitignore") == skillIgnore && Read(linked / "outside.ignore") == "keep me\n"; + const fs::path dangling = repo("dangling"); + fs::create_directories(dangling / "shared"); + fs::create_directories(dangling / ".claude" / "skills" / "prepush"); + fs::create_symlink("../../../shared/skill.ignore", dangling / ".claude" / "skills" / "prepush" / ".gitignore"); + const Process::Captured danglingRefused = Self({"init"}, dangling); + Check("init: a .gitignore beside the skill that is a link is refused before anything is written and never written through, dangling or not; --force replaces the link with init's file, " + "its target untouched", + linkRefused.returnCode == 3 && linkRefused.err.contains(".claude/skills/prepush/.gitignore is a link to ../../../outside.ignore") && linkUntouched && linkForced.returnCode == 0 + && replaced && danglingRefused.returnCode == 3 && !fs::exists(dangling / "shared" / "skill.ignore") && !fs::exists(dangling / ".review-gates"), + linkRefused.err + linkForced.out + linkForced.err + danglingRefused.err); + // A .gitignore link whose target names review-gates is the + // repository's too - the old setup linked the skill's directory, never + // its .gitignore - so it is refused like any other. A link init cannot + // remove (its directory not writable, where permissions bind) refuses + // under --force before anything is written, nothing written through it. + const fs::path named = repo("named"); + Write(named / ".review-gates" / "skill.gitignore", "mine\n"); + fs::create_directories(named / ".claude" / "skills" / "prepush"); + fs::create_symlink("../../../.review-gates/skill.gitignore", named / ".claude" / "skills" / "prepush" / ".gitignore"); + const Process::Captured namedRefused = Self({"init"}, named); + // A SKILL.md link into the pipeline's own .review-gates/ names the + // submodule but does not resolve into it: refused and kept too. + const fs::path namedSkill = repo("named-skill"); + Write(namedSkill / ".review-gates" / "skill.md", "mine\n"); + fs::create_directories(namedSkill / ".claude" / "skills" / "prepush"); + fs::create_symlink("../../../.review-gates/skill.md", namedSkill / ".claude" / "skills" / "prepush" / "SKILL.md"); + const Process::Captured namedSkillRefused = Self({"init"}, namedSkill); + const bool namedSkillKept = fs::is_symlink(fs::symlink_status(namedSkill / ".claude" / "skills" / "prepush" / "SKILL.md", linkError)) + && Read(namedSkill / ".review-gates" / "skill.md") == "mine\n" && !fs::exists(namedSkill / ".review-gates" / "config.json"); + const bool namedKept = fs::is_symlink(fs::symlink_status(named / ".claude" / "skills" / "prepush" / ".gitignore", linkError)) + && Read(named / ".review-gates" / "skill.gitignore") == "mine\n" && !fs::exists(named / ".review-gates" / "config.json"); + const fs::path stuck = repo("stuck"); + Write(stuck / "outside.ignore", "keep me\n"); + fs::create_directories(stuck / ".claude" / "skills" / "prepush"); + fs::create_symlink("../../../outside.ignore", stuck / ".claude" / "skills" / "prepush" / ".gitignore"); + fs::permissions(stuck / ".claude" / "skills" / "prepush", fs::perms::owner_read | fs::perms::owner_exec); + const bool writable = static_cast(std::ofstream(stuck / ".claude" / "skills" / "prepush" / ".probe")); + fs::remove(stuck / ".claude" / "skills" / "prepush" / ".probe", linkError); + const Process::Captured stuckForced = Self({"init", "--force"}, stuck); + fs::permissions(stuck / ".claude" / "skills" / "prepush", fs::perms::owner_all); + Check("init: a .gitignore or SKILL.md link into a review-gates path that is not the submodule is refused and kept; a link init cannot remove refuses under --force, writing nothing (where " + "permissions bind)", + namedRefused.returnCode == 3 && namedRefused.err.contains(".claude/skills/prepush/.gitignore is a link to ../../../.review-gates/skill.gitignore") && namedKept + && namedSkillRefused.returnCode == 3 && namedSkillRefused.err.contains(".claude/skills/prepush/SKILL.md is a link to ../../../.review-gates/skill.md") && namedSkillKept + && (writable + || (stuckForced.returnCode == 3 && stuckForced.err.contains("cannot remove the link .claude/skills/prepush/.gitignore") && Read(stuck / "outside.ignore") == "keep me\n" + && !fs::exists(stuck / ".review-gates"))), + namedRefused.err + namedSkillRefused.err + stuckForced.out + stuckForced.err); // doc: the texts, and where they are. const Process::Captured prepush = Self({"doc", "prepush"}, base);