skill: make the spool the way lessons are added, and guard it

.claude/skills/ci-gates/SKILL.md

@@ -14,6 +14,34 @@ the innermost one was right.
 
 ---
 
+## Adding a lesson to this file
+
+**Do not append to this file directly.** Use the spool:
+
+```
+tri skill add "<the section title>"     # writes incoming/<date>-<slug>.md, unnumbered
+```
+
+then write the lesson into that file. `tri skill fold` appends every spooled lesson to
+SKILL.md and assigns the numbers **at that moment**, against the file as it then stands.
+
+The reason is measured, not stylistic. Appending `## N.` from a branch chooses the number
+against **that branch's base**. Two branches doing it merge into a duplicate number, and
+that has happened twice in two passes here — after which the two *repairs* collided with
+each other as well. No check on the branch can catch it: `tri skill check` passes on both
+sides and fails only on the merged result, so there is nothing for a hook to look at, and
+a local `renumber` before pushing still races anything that merges in between.
+
+A spooled lesson has a unique path and no number, so two branches write two paths and
+there is nothing to conflict on. This is exactly what `docs/now/` does, and for the same
+reason — its gate script records that entries used to be prepended to one file and "the
+races were resolved by hand". 548 entry files later they do not conflict.
+
+`cat >> SKILL.md` is the habit this replaces. It is easy, it works on one branch, and it
+is how every collision so far was made.
+
+---
+
 ## 1. A gate that cannot fail reads as coverage, and is worse than none
 
 `docs/BRANCH-PROTECTION.md` named five required checks. Two had a body of exactly one

.github/workflows/cli-tri.yml

@@ -142,6 +142,27 @@ jobs:
       - name: No two skill sections share a number
         run: ./target/debug/tri skill check
 
+      # The check above finds a duplicate number. It cannot find the PRACTICE
+      # that makes one, because the collision exists in neither branch: two
+      # branches each append `## N.` numbered from their own base, `tri skill
+      # check` passes on both sides, and the duplicate appears only in the merge
+      # result. It happened twice in two passes, and the two repairs then raced
+      # each other as well.
+      #
+      # `tri skill add` / `tri skill fold` remove it by giving each lesson a
+      # unique path and assigning the number after the merge. That is a habit,
+      # and a habit is exactly what failed here the first time -- so this asks
+      # the question a diff CAN answer: did every section this branch adds come
+      # from a spool file that was on the base?
+      #
+      # The fetch is not decoration. This checkout is shallow, so `origin/master`
+      # does not resolve, every file would read as absent-on-base, the population
+      # would empty, and a gate that never ran would print a pass.
+      - name: Every new skill section came through the spool
+        run: |
+          git fetch --depth=1 origin master
+          ./target/debug/tri skill spooled --base origin/master --gate
+
       # A census that moves silently is a number nobody re-read. Measured over
       # the 39 most recent transitions on master with one fixed instrument:
       # 8 commits moved a census and only 4 said so -- and one of the silent

cli/tri/src/main.rs

@@ -383,6 +383,19 @@ enum SkillAction {
         #[arg(long)]
         gate: bool,
     },
+    /// Whether every section this branch adds came through the spool.
+    ///
+    /// The practice check for `tri skill add`. The collision it prevents is
+    /// invisible on a branch, but a direct append is not: `fold` deletes one
+    /// spool file per section it writes, and `cat >>` deletes nothing.
+    Spooled {
+        /// The branch this one is measured against.
+        #[arg(long, default_value = "origin/master")]
+        base: String,
+        /// Exit 1 when a section arrived without a spool file.
+        #[arg(long)]
+        gate: bool,
+    },
     /// Every cross-reference in the skills, and whether it resolves.
     Refs {
         /// Print every reference counted, not only the ones that dangle.
@@ -1001,6 +1014,12 @@ fn main() -> Result<()> {
                         gate: *gate,
                     })?
                 }
+                SkillAction::Spooled { base, gate } => {
+                    skillnum::run(&skillnum::SkillCmd::Spooled {
+                        base: base.clone(),
+                        gate: *gate,
+                    })?
+                }
                 SkillAction::Refs { list } => {
                     skillnum::run(&skillnum::SkillCmd::Refs { list: *list })?
                 }

cli/tri/src/skillnum.rs

@@ -94,6 +94,21 @@ pub enum SkillCmd {
         #[arg(long)]
         gate: bool,
     },
+    /// Whether every section this branch adds came through the spool.
+    ///
+    /// The guard `tri skill add` needs in order to survive a change of context.
+    /// The rule it enforces is written at the top of SKILL.md, and a rule that
+    /// lives only in prose is the same class of thing that already failed here:
+    /// the next pass reads what is convenient and reaches for `cat >>`.
+    Spooled {
+        /// The branch this one is measured against.
+        #[arg(long, default_value = "origin/master")]
+        base: String,
+        /// Exit 1 when a section arrived without a spool file. Off by default so
+        /// the command can be read before it is enforced.
+        #[arg(long)]
+        gate: bool,
+    },
     /// Every cross-reference in the skills, and whether it resolves.
     Refs {
         /// Print every reference counted, not only the ones that dangle.
@@ -171,6 +186,38 @@ pub fn sections(text: &str) -> Vec<(usize, String)> {
     out
 }
 
+/// The sections a branch adds to SKILL.md that did NOT come through the spool.
+///
+/// `tri skill add` removes a collision that no branch-side check can see: two
+/// branches each append `## N.` numbered from their OWN base, both merge, and
+/// the number appears twice. The COLLISION is invisible here -- but the practice
+/// that causes it is not. `tri skill fold` deletes one spool file for every
+/// section it appends; `cat >> SKILL.md` deletes nothing.
+///
+/// Compared by TITLE, and not by number and not by diff line:
+///   - `tri skill renumber` rewrites every number and keeps every title, so a
+///     number-set comparison would report the whole file as new.
+///   - a `+## N. ` diff line cannot be told from a heading QUOTED inside a fence.
+///     3 of the 518 `## N. ` lines on master are quotations, and miscounting one
+///     of those has already cost a real section here.
+///
+/// Returns the new titles and whether the branch is clean. A fold of K lessons
+/// deletes K spool files, so K new titles are allowed; a direct append allows 0.
+pub fn unspooled(
+    base: &[(usize, String)],
+    head: &[(usize, String)],
+    folded: usize,
+) -> (Vec<String>, bool) {
+    let was: std::collections::BTreeSet<&str> = base.iter().map(|(_, t)| t.as_str()).collect();
+    let new: Vec<String> = head
+        .iter()
+        .filter(|(_, t)| !was.contains(t.as_str()))
+        .map(|(_, t)| t.clone())
+        .collect();
+    let ok = new.len() <= folded;
+    (new, ok)
+}
+
 /// Every complaint about one file. Empty means the numbering holds.
 pub fn problems(secs: &[(usize, String)]) -> Vec<String> {
     let mut bad = Vec::new();
@@ -226,9 +273,26 @@ fn skill_files(root: &std::path::Path) -> Vec<PathBuf> {
         return out;
     };
     for e in rd.flatten() {
-        let p = e.path().join("SKILL.md");
-        if p.is_file() {
-            out.push(p);
+        // Read the entry that is THERE rather than joining a name and asking
+        // whether it exists. 5 skill files are tracked here and 2 are spelled
+        // `skill.md`, so joining the uppercase name reads 3 of 5 -- and on a
+        // case-insensitive filesystem `join("SKILL.md").is_file()` is true for a
+        // file actually called `skill.md`, handing back a path git has never
+        // heard of. `git show origin/master:<that>` then fails and a tracked
+        // file reads as NEW, which is the quiet direction to be wrong in.
+        //
+        // Neither lowercase file carries a numbered heading today, so the
+        // population that was missing is empty and no past check gave a wrong
+        // answer. That is luck: the next numbered heading added to one of them
+        // would have been unguarded.
+        let Ok(inner) = std::fs::read_dir(e.path()) else {
+            continue;
+        };
+        for f in inner.flatten() {
+            if f.file_name().eq_ignore_ascii_case("SKILL.md") && f.path().is_file() {
+                out.push(f.path());
+                break;
+            }
         }
     }
     out.sort();
@@ -736,8 +800,118 @@ fn fold(check: bool, skill: &str) -> Result<()> {
     Ok(())
 }
 
+/// Read a file at a rev, distinguishing "absent there" from "git could not run".
+fn at_opt(rev: &str, path: &str, root: &std::path::Path) -> Option<String> {
+    let out = std::process::Command::new("git")
+        .args(["show", &format!("{rev}:{path}")])
+        .current_dir(root)
+        .output()
+        .ok()?;
+    out.status
+        .success()
+        .then(|| String::from_utf8_lossy(&out.stdout).to_string())
+}
+
+/// Spool files this branch deleted -- which is what folding one looks like.
+fn folded_here(base: &str, dir: &str, root: &std::path::Path) -> usize {
+    let out = std::process::Command::new("git")
+        .args([
+            "diff",
+            "--name-status",
+            "--diff-filter=D",
+            base,
+            "--",
+            &format!("{dir}/incoming/"),
+        ])
+        .current_dir(root)
+        .output();
+    match out {
+        Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).lines().count(),
+        _ => 0,
+    }
+}
+
+fn spooled(base: &str, gate: bool) -> Result<()> {
+    let root = repo_root()?;
+    let files = skill_files(&root);
+    println!("base: {base}   files: {}", files.len());
+    let mut offenders = 0usize;
+    let mut checked = 0usize;
+    for f in &files {
+        let path = rel(&root, f);
+        let Some(before) = at_opt(base, &path, &root) else {
+            // A SKILL.md this branch CREATES cannot collide with a base that has
+            // no such file, so this is not an offence -- but say so, because a
+            // population that quietly shrinks is how a clean bill of health gets
+            // printed over nothing.
+            println!("  NEW      {path} -- absent on {base}, nothing to collide with");
+            continue;
+        };
+        let head = std::fs::read_to_string(f).unwrap_or_default();
+        let dir = std::path::Path::new(&path)
+            .parent()
+            .map(|d| d.display().to_string())
+            .unwrap_or_default();
+        let folded = folded_here(base, &dir, &root);
+        let (new, ok) = unspooled(&sections(&before), &sections(&head), folded);
+        checked += 1;
+        if new.is_empty() {
+            println!("  ok       {path} -- adds no section");
+            continue;
+        }
+        if ok {
+            println!(
+                "  ok       {path} -- {} new section(s), {folded} spool file(s) folded",
+                new.len()
+            );
+            continue;
+        }
+        offenders += 1;
+        println!(
+            "  UNSPOOLED {path} -- {} new section(s) but {folded} spool file(s) folded",
+            new.len()
+        );
+        for t in new.iter().take(10) {
+            println!("             {t}");
+        }
+        if new.len() > 10 {
+            println!("             ... and {} more", new.len() - 10);
+        }
+    }
+    println!("checked {checked} file(s) that exist on {base}; {offenders} unspooled");
+    // A shallow checkout has no `origin/master`, so every file reads NEW, the
+    // population empties, and a gate that never ran prints a pass. That is the
+    // failure this whole skill is about, so it is spelled COULD NOT RUN and
+    // given its own exit code rather than folded into the clean one.
+    if checked == 0 && !files.is_empty() {
+        println!();
+        println!("COULD NOT RUN: no skill file could be read on `{base}`.");
+        println!("  The ref is probably missing -- a shallow clone has no remote-tracking");
+        println!("  branches. Fetch it first:  git fetch --depth=1 origin master");
+        if gate {
+            std::process::exit(2);
+        }
+        return Ok(());
+    }
+    if offenders > 0 {
+        println!();
+        println!("A section was appended to SKILL.md directly. Two branches doing that");
+        println!("choose the same number and the duplicate appears only after the merge,");
+        println!("where no branch-side check can see it. Use the spool instead:");
+        println!();
+        println!("    tri skill add \"<title>\"      # writes incoming/<date>-<slug>.md");
+        println!("    tri skill fold               # appends and numbers, on one branch");
+        println!();
+        if gate {
+            std::process::exit(1);
+        }
+    }
+    Ok(())
+}
+
 pub fn run(cmd: &SkillCmd) -> Result<()> {
     let show_gaps = match cmd {
+        SkillCmd::Spooled { base, gate } => return spooled(base, *gate),
         SkillCmd::Refs { list } => return refs(*list),
         SkillCmd::Claims {
             list,
@@ -833,6 +1007,88 @@ pub fn run(cmd: &SkillCmd) -> Result<()> {
 #[cfg(test)]
 mod tests {
 
+    fn secs(v: &[(usize, &str)]) -> Vec<(usize, String)> {
+        v.iter().map(|(n, t)| (*n, t.to_string())).collect()
+    }
+
+    #[test]
+    fn a_direct_append_is_unspooled() {
+        use super::unspooled;
+        let base = secs(&[(1, "one"), (2, "two")]);
+        let head = secs(&[(1, "one"), (2, "two"), (3, "appended by hand")]);
+        let (new, ok) = unspooled(&base, &head, 0);
+        assert_eq!(new, vec!["appended by hand".to_string()]);
+        assert!(!ok, "a section with no spool file behind it must not pass");
+    }
+
+    #[test]
+    fn a_fold_of_one_spooled_lesson_passes() {
+        use super::unspooled;
+        let base = secs(&[(1, "one")]);
+        let head = secs(&[(1, "one"), (2, "folded")]);
+        let (new, ok) = unspooled(&base, &head, 1);
+        assert_eq!(new.len(), 1);
+        assert!(
+            ok,
+            "one new section against one deleted spool file is a fold"
+        );
+    }
+
+    #[test]
+    fn a_renumber_moves_every_number_and_adds_no_work() {
+        use super::unspooled;
+        // Why the comparison is by TITLE. `tri skill renumber` rewrites every
+        // heading, so comparing NUMBERS would call the whole file new -- and the
+        // guard would fire hardest on the one command whose job is to avoid
+        // collisions.
+        let base = secs(&[(1, "one"), (2, "two"), (3, "three")]);
+        let head = secs(&[(7, "one"), (8, "two"), (9, "three")]);
+        let (new, ok) = unspooled(&base, &head, 0);
+        assert!(new.is_empty(), "renumber adds no title, got {new:?}");
+        assert!(ok);
+    }
+
+    #[test]
+    fn a_withdrawal_removes_and_never_fires() {
+        use super::unspooled;
+        let base = secs(&[(1, "one"), (2, "two")]);
+        let head = secs(&[(1, "one")]);
+        let (new, ok) = unspooled(&base, &head, 0);
+        assert!(new.is_empty());
+        assert!(ok);
+    }
+
+    #[test]
+    fn folding_two_lessons_needs_two_spool_files() {
+        use super::unspooled;
+        let base = secs(&[(1, "one")]);
+        let head = secs(&[(1, "one"), (2, "a"), (3, "b")]);
+        assert!(
+            !unspooled(&base, &head, 1).1,
+            "two sections, one spool file"
+        );
+        assert!(unspooled(&base, &head, 2).1);
+    }
+
+    #[test]
+    fn a_heading_quoted_in_a_fence_is_not_a_new_section() {
+        use super::{sections, unspooled};
+        // A diff-line matcher counts this as an appended section and demands a
+        // spool file for it. 3 of the 518 `## N. ` lines on master have exactly
+        // this shape, and miscounting one has already destroyed a real section.
+        let base_text = "## 1. one\n\nbody\n";
+        let quoted = "## 1. one\n\nbody\n\n```text\n## 2. quoted, not a section\n```\n";
+        let (new, ok) = unspooled(&sections(base_text), &sections(quoted), 0);
+        assert!(new.is_empty(), "a quotation is not a section, got {new:?}");
+        assert!(ok);
+        // The control, without which the assertion above passes for the wrong
+        // reason: the SAME line OUTSIDE a fence must be counted.
+        let real = format!("{base_text}\n## 2. quoted, not a section\n");
+        let (new, ok) = unspooled(&sections(base_text), &sections(&real), 0);
+        assert_eq!(new.len(), 1, "outside a fence this IS a section");
+        assert!(!ok);
+    }
+
     #[test]
     fn a_spool_path_is_unique_per_lesson() {
         use super::spool_name;