diff --git a/.claude/skills/llp-create/SKILL.md b/.claude/skills/llp-create/SKILL.md index fd2a9799..1aa0e27a 100644 --- a/.claude/skills/llp-create/SKILL.md +++ b/.claude/skills/llp-create/SKILL.md @@ -21,9 +21,19 @@ Invoke as `/llp-create ` with an optional title, or `/llp-create` and the ### 1. Determine the next LLP number -Scan all files in `llp/` (including subdirectories and `llp/tombstones/`) that match the pattern `NNNN-*.md`. Extract the numeric prefix of each. The next number is `max(existing) + 1`, zero-padded to four digits. +Run `node scripts/llp-numbers.js next` and use what it prints. -If another LLP tree exists in a non-standard location (some projects use `docs/site/content/llp/` or similar), include it in the scan. Check the project's root LLP or `CLAUDE.md` / `AGENTS.md` for any documented LLP locations. +**Do not read the number off the tree you have checked out.** The branch you are on is not the corpus: numbers are minted on every branch at once, and a document minted on a branch you cannot see claims its number just as firmly as one on the default branch. Three branches cut from the same master each took `max(llp/) + 1`, each got the same answer, and git reported no conflict because their slugs differed ([issue #907](https://github.com/hyparam/hypaware/issues/907)). LLP 0156 settles the rule: a fresh number sits above the highest claimed **anywhere**, including branches without an open PR, and including `llp/tombstones/`, because a retired number is never reused. + +`scripts/llp-numbers.js` is that rule: + +- `next` prints the next free number across every ref that could still merge and the working tree, zero-padded. A document you created a minute ago and have not committed claims its number too, so two calls in one session do not collide. +- `check` exits nonzero when a number this branch mints is already claimed elsewhere. It runs in CI (`.github/workflows/llp-check.yml`) and in `npm test`. +- `survey` lists every collision across every ref, including the settled ones stale branches still carry. + +Fetch first or the scan only sees the refs you already had: `git fetch --prune --unshallow` on a shallow clone, `git fetch --no-tags --prune origin '+refs/heads/*:refs/remotes/origin/*'` on a complete one (`--unshallow` aborts there). The script warns when it can tell that it is reading a shallow or single-branch checkout, and `check` refuses outright rather than passing on a corpus it never saw. + +Fall back to `max(existing) + 1` over `llp/`, **and say that you did so the number is known to be a guess**, in the three cases the script cannot cover: the repository has no `scripts/llp-numbers.js` (it is a HypAware file, not part of LLP itself), the repository is not a git checkout, or LLP documents live somewhere the script does not look. The script scans `llp/` only. If another LLP tree exists in a non-standard location (some projects use `docs/site/content/llp/` or similar), scan it by hand and take the higher of the two answers. Check the project's root LLP or `CLAUDE.md` / `AGENTS.md` for any documented LLP locations. ### 2. Ask the user for the metadata if not provided diff --git a/.claude/skills/ref-check/SKILL.md b/.claude/skills/ref-check/SKILL.md index 3c970029..92846fc5 100644 --- a/.claude/skills/ref-check/SKILL.md +++ b/.claude/skills/ref-check/SKILL.md @@ -111,7 +111,9 @@ Scan `llp/` (and any other configured LLP trees) for all documents. For each, ex Build a map from `(LLP number) → (file path, title, {anchor: heading text})`. -Also record where the same number is claimed by **more than one** file. A duplicate number makes `@ref LLP NNNN#anchor` ambiguous, and a checker that keeps only the last file it walked will report every reference aimed at the other one as broken. Resolve an anchor against **any** claimant, and report the duplicate itself as a `WARNING` on the corpus. +Also record where the same number is claimed by **more than one** file. A duplicate number makes `@ref LLP NNNN#anchor` ambiguous, and a checker that keeps only the last file it walked will report every reference aimed at the other one as broken. Resolve an anchor against **any** claimant, so that the references keep resolving, and report the duplicate itself as `BROKEN` on the corpus: one number resolving to two documents is a defect in its own right, not a hint, and it is repaired by renumbering the later claimant (LLP 0156). + +**The tree you are scanning is one branch of the corpus.** Two branches can each mint the same number cleanly, and nothing in a single tree can see it: the collision appears only when the second one merges (issue #907). `node scripts/llp-numbers.js survey` reports duplicates across every ref, `check` fails when the branch you are on mints a number claimed elsewhere, and `next` gives the number a new document should take. Run `git fetch --prune` first, or the scan only sees the refs you already had. ### 4. Validate each reference @@ -183,6 +185,7 @@ For scripting (this skill can be invoked from CI): - Exit 0 if no broken references - Exit 1 if any broken references +- Exit 1 if any LLP number resolves to more than one document - Warnings and hints do not cause a non-zero exit code ## Output formats diff --git a/.github/workflows/llp-check.yml b/.github/workflows/llp-check.yml index 2b3c3c50..a5e6f54a 100644 --- a/.github/workflows/llp-check.yml +++ b/.github/workflows/llp-check.yml @@ -21,3 +21,22 @@ jobs: for n in $dups; do find llp -path llp/reviews -prune -o -name "$n-*.md" -print >&2; done echo "Fix: renumber the doc that landed second to the next free number across all branches (tombstones count - numbers are never reused), and update the @refs that meant it." >&2 exit 1 + + # The in-tree job above only sees a collision once both documents are in one + # tree, which is after the second merge. Three branches minting the same number + # in parallel is not visible from any one of them (issue #907), so this job + # fetches every branch and asks whether the number this one mints is already + # claimed somewhere else. It reports only the numbers this branch adds, so a + # collision already settled on master, still carried by a stale branch that + # predates the repair, does not redden pull requests party to none of it. + cross-branch-numbers: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + - name: Fetch every branch that could merge + run: git fetch --no-tags --prune origin '+refs/heads/*:refs/remotes/origin/*' + - name: No newly minted LLP number is claimed elsewhere + run: node scripts/llp-numbers.js check diff --git a/AGENTS.md b/AGENTS.md index 5e249054..4dc9a289 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,6 +37,14 @@ actually shipped in V1. rationale-order view; `/llp-create <title>` scaffolds a new doc; `/llp-list` surveys the corpus; `/llp-grill` stress-tests a plan against the LLP corpus before you write code. +- **A new number comes from `node scripts/llp-numbers.js next`**, after a + `git fetch --prune`. Numbers are minted on every branch at once, so the tree + you have checked out is not the corpus: three branches each read + `max(llp/) + 1` and each got the same answer (issue #907). The same script + gates it: `check` in the `cross-branch-numbers` CI job, which fetches every + branch first, and in `npm test` wherever the clone carries them (a shallow or + single-branch checkout skips it and says so). `survey` shows every collision + across every ref. ## Code Style diff --git a/scripts/llp-numbers.js b/scripts/llp-numbers.js new file mode 100755 index 00000000..4a49da1d --- /dev/null +++ b/scripts/llp-numbers.js @@ -0,0 +1,474 @@ +#!/usr/bin/env node +// @ts-check + +// The minting rule of LLP 0156, made executable. A number is free only when no +// document claims it on *any* ref that could still merge, nor in the working tree +// alongside them, so the tree you happen to have checked out is the wrong +// denominator: three branches cut from the same +// master each read `max(llp/) + 1`, each got the same answer, and git reported no +// conflict because their slugs differed (issue #907). +// +// node scripts/llp-numbers.js next the next free number, four digits +// node scripts/llp-numbers.js check exit 1 if this branch mints a taken number +// node scripts/llp-numbers.js survey every collision across every ref +// +// @ref LLP 0156#renumber [implements]: a fresh number sits above the highest claimed anywhere, including branches without an open PR +// @ref LLP 0001#tooling [implements]: the corpus gate that has to hold on every push, as a script rather than a prompt + +import { execFileSync } from 'node:child_process' +import path from 'node:path' +import process from 'node:process' +import { pathToFileURL } from 'node:url' + +/** Documents live under this directory, tombstones and subdirectories included. */ +const LLP_DIR = 'llp' + +/** + * Per-document review artifacts are not documents and claim no number. Mirrors + * the exclusion in `.github/workflows/llp-check.yml`. + */ +const EXCLUDED_DIRS = new Set(['reviews']) + +/** `NNNN-slug.type.md`, the filename convention of LLP 0000. */ +const DOC_PATTERN = /^(\d{4})-.*\.md$/ + +/** How many refs a claimant line names before it summarises the rest. */ +const MAX_REFS_SHOWN = 6 + +/** The label the working tree carries when it claims a number alongside the refs. */ +export const WORKTREE = 'the working tree' + +/** What `partialScan` answers when the path is not a git checkout at all. */ +export const NOT_A_CHECKOUT = 'not a git checkout, so there is no corpus to scan' + +/** What `partialScan` answers when the clone carries truncated history. */ +export const SHALLOW_CLONE = 'a shallow clone, so the history it carries is not the corpus' + +/** What `partialScan` answers when the clone carries no branch to compare against. */ +export const NO_BASE_REF = 'a checkout with no default-branch ref, so the branches it carries are not the corpus' + +/** + * The fetch that turns each partial scan into a whole one, named per case + * because the two are not interchangeable: `--unshallow` aborts with + * `fatal: --unshallow on a complete repository does not make sense` on a + * single-branch clone, which is the other case entirely. + */ +const REMEDY = new Map([ + [SHALLOW_CLONE, 'git fetch --prune --unshallow'], + [NO_BASE_REF, "git fetch --no-tags --prune origin '+refs/heads/*:refs/remotes/origin/*'"], +]) + +/** Where the default branch is looked for, first hit wins. */ +const BASE_CANDIDATES = ['refs/remotes/origin/master', 'refs/remotes/origin/main', 'refs/heads/master', 'refs/heads/main'] + +/** + * The number a path claims, or null when the path is not an LLP document. + * + * @param {string} filePath repo-relative, forward-slashed as git reports it + * @returns {number | null} + */ +export function llpNumberOf(filePath) { + const parts = filePath.split('/') + if (parts[0] !== LLP_DIR) return null + if (parts.slice(1, -1).some(dir => EXCLUDED_DIRS.has(dir))) return null + const matched = DOC_PATTERN.exec(parts[parts.length - 1]) + return matched === null ? null : Number(matched[1]) +} + +/** + * Index every claim across a set of refs: which document basenames claim each + * number, and which refs carry each basename. Keyed on the basename rather than + * the full path so that a document that only moved into `llp/tombstones/` on one + * branch is one claimant, not two. + * + * @param {Map<string, string[]>} refFiles ref name to the paths it carries + * @returns {Map<number, Map<string, string[]>>} number to basename to the refs claiming it + */ +export function claimsByNumber(refFiles) { + /** @type {Map<number, Map<string, string[]>>} */ + const claims = new Map() + for (const [ref, files] of refFiles) { + for (const file of files) { + const number = llpNumberOf(file) + if (number === null) continue + const byName = claims.get(number) ?? new Map() + const name = basename(file) + const refs = byName.get(name) ?? [] + if (!refs.includes(ref)) refs.push(ref) + byName.set(name, refs) + claims.set(number, byName) + } + } + return claims +} + +/** + * The next number a new document may take: above the highest claimed anywhere, + * never the lowest unused. The corpus is deliberately sparse, because a number + * retired with its document is never reused, so a gap is not free. + * + * @param {Map<string, string[]>} refFiles + * @returns {number} + */ +export function nextFreeNumber(refFiles) { + let highest = 0 + for (const number of claimsByNumber(refFiles).keys()) { + if (number > highest) highest = number + } + return highest + 1 +} + +/** + * Numbers claimed by two or more different documents across the given refs. + * + * `only` narrows the report to a set of numbers. Reporting every collision would + * make this unusable as a gate: a branch cut before a repair still carries the + * pre-repair filename, so the corpus's already-settled collisions live on in + * every stale branch and would redden pull requests that are party to none of + * them. What a pull request is answerable for is the number it *mints*. + * + * @param {Map<string, string[]>} refFiles + * @param {Set<number> | null} only + * @returns {{ number: number, claimants: { file: string, refs: string[] }[] }[]} + */ +export function collisions(refFiles, only = null) { + /** @type {{ number: number, claimants: { file: string, refs: string[] }[] }[]} */ + const found = [] + for (const [number, byName] of claimsByNumber(refFiles)) { + if (byName.size < 2) continue + if (only !== null && !only.has(number)) continue + const claimants = [...byName].map(([file, refs]) => ({ file, refs })) + found.push({ number, claimants: claimants.sort((a, b) => a.file.localeCompare(b.file)) }) + } + return found.sort((a, b) => a.number - b.number) +} + +/** + * Distinct documents per number in one tree, by basename so that a document the + * branch only moved between directories (into `llp/tombstones/`, say) stays one + * document. + * + * @param {string[]} files + * @returns {Map<number, Set<string>>} + */ +function docsByNumber(files) { + /** @type {Map<number, Set<string>>} */ + const byNumber = new Map() + for (const file of files) { + const number = llpNumberOf(file) + if (number === null) continue + const names = byNumber.get(number) ?? new Set() + names.add(basename(file)) + byNumber.set(number, names) + } + return byNumber +} + +/** + * The numbers a branch is answerable for: those that more documents claim at its + * tip than at the point it left the default branch. Counted rather than matched + * by name, because changing a document's slug leaves its number exactly as + * claimed as it already was (0265-a to 0265-b mints nothing), while adding a + * second document at a number the base already carries does mint it. A renumber, + * the repair LLP 0156 prescribes, mints the number it moves to and is checked + * there, which is the point: the number it moves to has to be free too. + * + * @param {string[]} baseFiles paths at the merge base + * @param {string[]} headFiles paths at the tip + * @returns {Set<number>} + */ +export function mintedAgainst(baseFiles, headFiles) { + const before = docsByNumber(baseFiles) + /** @type {Set<number>} */ + const minted = new Set() + for (const [number, names] of docsByNumber(headFiles)) { + if (names.size > (before.get(number)?.size ?? 0)) minted.add(number) + } + return minted +} + +/** + * The base branch's own documents that are sitting in this tree: inherited, not + * minted here. The merge base alone is the wrong floor whenever the tree carries + * base-branch content the fork point did not have (an interrupted `git merge`, + * a `git checkout origin/master -- llp/`). Every such document then reads as + * newly minted and the branch is told to renumber one it never wrote. Matching + * by basename, and only where the document is actually here, keeps the count + * comparison honest: a slug this branch rewrote is not in the base tip under its + * new name, so it stays inherited at one document, and a number the base branch + * claims under a name this branch does not carry stays mintable, which is how a + * rival document landing on master first is still caught. + * + * @param {string[]} baseFiles paths at the base branch's tip + * @param {string[]} tipPaths paths this branch would merge + * @returns {string[]} + */ +function inheritedFrom(baseFiles, tipPaths) { + const here = new Set(tipPaths.map(basename)) + return baseFiles.filter(file => here.has(basename(file))) +} + +/** + * Refs whose tip is reachable from HEAD. They hold no claim of their own: every + * document they carry is either in this tree too or was removed on the way here, + * so a claim only they hold is a prior version of this branch rather than a + * rival. Without this, renaming the slug of a number you minted and pushed fails + * the gate against your own pre-rename filename, still sitting on + * `refs/remotes/origin/<branch>`, and prescribes a renumber for a collision with + * yourself. + * + * @param {string} repoRoot + * @returns {Set<string>} + */ +export function supersededRefs(repoRoot) { + const out = tryGit(repoRoot, ['for-each-ref', '--format=%(refname)', '--merged', 'HEAD', 'refs/heads', 'refs/remotes']) + return new Set((out ?? '').split('\n').filter(line => line !== '')) +} + +/** + * Every ref whose content could still reach the default branch: local branches, + * remote tracking branches, and the working tree's own ref. Tags are excluded (a + * tag is history that already merged or never will), and so is a remote's + * symbolic `HEAD`, an alias for a branch already listed. + * + * @param {string} repoRoot + * @returns {string[]} + */ +export function mergeableRefs(repoRoot) { + const out = tryGit(repoRoot, ['for-each-ref', '--format=%(refname)', 'refs/heads', 'refs/remotes']) + const refs = (out ?? '').split('\n').filter(line => line !== '' && !/\/HEAD$/.test(line)) + refs.push('HEAD') + return refs +} + +/** + * The LLP documents each ref carries. A ref that cannot be read (a remote pruned + * mid-run, a shallow clone missing the object) contributes nothing rather than + * aborting the scan: a partial index still catches collisions, an aborted one + * catches none. + * + * @param {string} repoRoot + * @param {string[]} refs + * @returns {Map<string, string[]>} + */ +export function refFilesFromGit(repoRoot, refs) { + /** @type {Map<string, string[]>} */ + const refFiles = new Map() + for (const ref of refs) { + const listed = tryGit(repoRoot, ['ls-tree', '-r', '--name-only', ref, '--', LLP_DIR]) + if (listed === null) continue + refFiles.set(ref, listed.split('\n').filter(line => line !== '')) + } + return refFiles +} + +/** + * The LLP documents on disk, tracked and untracked alike. A document written but + * not yet committed claims its number: two `/llp-create` calls in one session + * are the ordinary case, and a scan of committed trees only would hand both the + * same number, which is the defect this script exists to prevent. + * + * @param {string} repoRoot + * @returns {string[]} + */ +export function worktreeFiles(repoRoot) { + const listed = tryGit(repoRoot, ['ls-files', '--cached', '--others', '--exclude-standard', '--', LLP_DIR]) + return listed === null ? [] : [...new Set(listed.split('\n').filter(line => line !== ''))] +} + +/** + * What this branch would merge: the working tree where there is one, the HEAD + * tree otherwise (a bare checkout, or a repo with no `llp/` on disk). + * + * @param {string} repoRoot + * @returns {string[]} + */ +export function tipFiles(repoRoot) { + const worktree = worktreeFiles(repoRoot) + return worktree.length > 0 ? worktree : refFilesFromGit(repoRoot, ['HEAD']).get('HEAD') ?? [] +} + +/** + * The claim index every mode reads: every mergeable ref plus the working tree. + * + * @param {string} repoRoot + * @returns {Map<string, string[]>} + */ +export function scanRefFiles(repoRoot) { + const refFiles = refFilesFromGit(repoRoot, mergeableRefs(repoRoot)) + const worktree = worktreeFiles(repoRoot) + if (worktree.length > 0) refFiles.set(WORKTREE, worktree) + return refFiles +} + +/** + * The numbers the working ref mints against the default branch. Empty on the + * default branch itself, which is what keeps the gate quiet on merges. + * + * A merge base that cannot be found (a truncated clone, an unrelated history) + * means the answer is unknown, reported as a null `mergeBase`. Treating it as an + * empty base instead would count the whole corpus at HEAD as newly minted and + * blame this branch for every collision already settled in it. + * + * @param {string} repoRoot + * @returns {{ base: string | null, mergeBase: string | null, numbers: Set<number> }} + */ +export function mintedNumbers(repoRoot) { + const base = BASE_CANDIDATES.find(ref => tryGit(repoRoot, ['rev-parse', '--verify', '--quiet', ref]) !== null) ?? null + if (base === null) return { base: null, mergeBase: null, numbers: new Set() } + const mergeBase = tryGit(repoRoot, ['merge-base', 'HEAD', base]) + if (mergeBase === null) return { base, mergeBase: null, numbers: new Set() } + const tip = tipFiles(repoRoot) + const inherited = inheritedFrom(refFilesFromGit(repoRoot, [base]).get(base) ?? [], tip) + const floor = [...refFilesFromGit(repoRoot, [mergeBase]).get(mergeBase) ?? [], ...inherited] + return { base, mergeBase, numbers: mintedAgainst(floor, tip) } +} + +/** + * Why this checkout cannot answer the question, or null when it can. A clone + * fetched shallow or with a single branch (what `actions/checkout` hands a job + * that does not ask for more) carries one ref and no default branch, so every + * mode reads a corpus of one: `next` hands back a number other branches already + * took, and `check` finds no base to diff against and passes. That is issue + * #907 wearing this script's own clothes, so it is reported rather than answered + * silently. + * + * @param {string} repoRoot + * @returns {string | null} + */ +export function partialScan(repoRoot) { + if (tryGit(repoRoot, ['rev-parse', '--git-dir']) === null) return NOT_A_CHECKOUT + if (tryGit(repoRoot, ['rev-parse', '--is-shallow-repository']) === 'true') return SHALLOW_CLONE + if (!BASE_CANDIDATES.some(ref => tryGit(repoRoot, ['rev-parse', '--verify', '--quiet', ref]) !== null)) return NO_BASE_REF + return null +} + +/** + * @param {string} filePath + * @returns {string} + */ +function basename(filePath) { + return filePath.split('/').pop() ?? filePath +} + +/** + * @param {string} repoRoot + * @param {string[]} args + * @returns {string | null} null when git failed or said nothing + */ +function tryGit(repoRoot, args) { + try { + const out = execFileSync('git', args, { cwd: repoRoot, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trimEnd() + return out === '' ? null : out + } catch { + return null + } +} + +/** + * @param {number} number + * @returns {string} + */ +export function padNumber(number) { + return String(number).padStart(4, '0') +} + +/** + * A claimant is carried by every branch cut since it landed, which on this repo + * is over a hundred refs for one document. Naming them all buries the report the + * reader has to act on, so the line names enough to locate it and counts the rest. + * + * @param {string[]} refs + * @returns {string} + */ +function describeRefs(refs) { + if (refs.length <= MAX_REFS_SHOWN) return refs.join(', ') + return `${refs.slice(0, MAX_REFS_SHOWN).join(', ')} and ${refs.length - MAX_REFS_SHOWN} more` +} + +/** + * @param {{ number: number, claimants: { file: string, refs: string[] }[] }[]} found + * @param {number} next the number a repair should move to + * @returns {string} + */ +export function formatCollisions(found, next) { + const lines = [`${found.length} LLP number${found.length === 1 ? '' : 's'} claimed by more than one document:`] + for (const { number, claimants } of found) { + lines.push(` LLP ${padNumber(number)}`) + for (const claimant of claimants) lines.push(` ${claimant.file} on ${describeRefs(claimant.refs)}`) + } + lines.push('') + lines.push(`Renumber the later claimant to ${padNumber(next)} or above (LLP 0156), and sweep`) + lines.push('every @ref, doc link and Related: header that meant it.') + return lines.join('\n') +} + +/** + * @param {string[]} argv + * @param {string} repoRoot + * @param {(text: string) => void} write + * @param {(text: string) => void} writeError + * @returns {number} process exit code + */ +export function run(argv, repoRoot, write, writeError) { + const mode = argv[0] + if (mode !== 'next' && mode !== 'check' && mode !== 'survey') { + writeError('usage: llp-numbers.js next | check | survey\n') + return 2 + } + const partial = partialScan(repoRoot) + if (partial === NOT_A_CHECKOUT) { + writeError(`${repoRoot} is ${partial}. Mint by hand from max(llp/) + 1, and say that you did.\n`) + return 2 + } + if (partial !== null) { + const remedy = REMEDY.get(partial) ?? 'git fetch --prune' + // `check` refuses rather than warns: a gate whose failure mode is a silent + // pass is the defect of issue #907 again, one layer up. A job that loses its + // `fetch-depth: 0` would go green having compared one ref against itself. + if (mode === 'check') { + writeError(`this is ${partial}. The check would pass without looking, so it refuses instead. Run \`${remedy}\`, or run it where the whole corpus is.\n`) + return 2 + } + writeError(`warning: this is ${partial}. Run \`${remedy}\`, or this answer is a guess.\n`) + } + const refFiles = scanRefFiles(repoRoot) + const next = nextFreeNumber(refFiles) + if (mode === 'next') { + write(`${padNumber(next)}\n`) + return 0 + } + if (mode === 'survey') { + const all = collisions(refFiles) + write(all.length === 0 ? 'no LLP number is claimed by two documents\n' : `${formatCollisions(all, next)}\n`) + return 0 + } + const minted = mintedNumbers(repoRoot) + if (minted.base === null) { + writeError('no default branch to compare against, so nothing is minted here\n') + return 0 + } + if (minted.mergeBase === null) { + writeError(`no common ancestor with ${minted.base}, so what this branch mints cannot be told from what it inherited\n`) + return 0 + } + const superseded = supersededRefs(repoRoot) + const rivals = new Map([...refFiles].filter(([ref]) => !superseded.has(ref))) + const found = collisions(rivals, minted.numbers) + if (found.length === 0) { + write(`${minted.numbers.size} LLP number${minted.numbers.size === 1 ? '' : 's'} minted against ${minted.base}, no collision\n`) + return 0 + } + writeError(`${formatCollisions(found, next)}\n`) + return 1 +} + +if (process.argv[1] !== undefined && import.meta.url === pathToFileURL(process.argv[1]).href) { + const root = path.resolve(path.dirname(process.argv[1]), '..') + process.exit(run( + process.argv.slice(2), + root, + text => process.stdout.write(text), + text => process.stderr.write(text), + )) +} diff --git a/test/core/llp-number-minting.test.js b/test/core/llp-number-minting.test.js new file mode 100644 index 00000000..5184f9f6 --- /dev/null +++ b/test/core/llp-number-minting.test.js @@ -0,0 +1,457 @@ +// @ts-check + +// Issue #907: three branches cut from the same master each minted `llp/0266-*`. +// Each read the highest number in the tree it had checked out, each got 0265, +// and each picked 0266. Their slugs differed, so git reported no conflict and +// the second merge would simply have added a second document numbered 0266. +// +// The fixtures below are that history, rebuilt in temp repos, and they are the +// whole reason this file exists: the rule under test is only wrong when more +// than one branch is in play, so a check that reads the working tree cannot see +// the defect it is meant to catch. +// +// @ref LLP 0156#renumber [tests]: a fresh number sits above the highest claimed anywhere, so a per-branch max is not the rule + +import test from 'node:test' +import assert from 'node:assert/strict' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { execFileSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' +import { + claimsByNumber, + collisions, + formatCollisions, + llpNumberOf, + mergeableRefs, + mintedAgainst, + mintedNumbers, + nextFreeNumber, + NOT_A_CHECKOUT, + partialScan, + refFilesFromGit, + scanRefFiles, + supersededRefs, + run, +} from '../../scripts/llp-numbers.js' + +/** + * @import { TestContext } from 'node:test' + */ + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..') + +/** The three documents of issue #907, one per branch, all numbered 0266. */ +const COLLIDED = [ + ['fix/issue-836', 'llp/0266-core-command-argument-validation.decision.md'], + ['fix/issue-884', 'llp/0266-prune-asks-every-client-not-the-run.decision.md'], + ['update/icebird-squirreling', 'llp/0266-native-prepared-batches.decision.md'], +] + +/** + * @param {string} repo + * @param {string[]} args + * @returns {string} + */ +function git(repo, args) { + return execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trimEnd() +} + +/** + * @param {string} repo + * @param {string} message + */ +function commit(repo, message) { + git(repo, ['add', '-A']) + git(repo, ['-c', 'user.name=t', '-c', 'user.email=t@example.com', 'commit', '-q', '-m', message]) +} + +/** + * @param {string} repo + * @param {string} file + */ +function writeDoc(repo, file) { + const full = path.join(repo, file) + fs.mkdirSync(path.dirname(full), { recursive: true }) + fs.writeFileSync(full, `# ${file}\n`) +} + +/** + * @param {TestContext} t + * @returns {string} an empty repo with a master branch + */ +function emptyRepo(t) { + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'llp-numbers-')) + t.after(() => fs.rmSync(repo, { recursive: true, force: true })) + git(repo, ['init', '-q', '-b', 'master']) + return repo +} + +/** + * A master at LLP 0265 with the three colliding branches hanging off it, each + * also mirrored under `refs/remotes/origin/` so the scan sees what a fetched + * clone would see. + * + * @param {TestContext} t + * @returns {string} the repo path + */ +function collidedRepo(t) { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0264-grep-search.decision.md') + writeDoc(repo, 'llp/0265-grep-search-implementation.plan.md') + writeDoc(repo, 'llp/tombstones/0018-retired.decision.md') + commit(repo, 'the corpus up to LLP 0265') + for (const [branch, file] of COLLIDED) { + git(repo, ['checkout', '-q', '-b', branch, 'master']) + writeDoc(repo, file) + commit(repo, `mint 0266 on ${branch}`) + git(repo, ['update-ref', `refs/remotes/origin/${branch}`, 'HEAD']) + } + git(repo, ['checkout', '-q', 'master']) + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + return repo +} + +test('a path claims a number only when it is an LLP document', () => { + assert.equal(llpNumberOf('llp/0266-prune-asks-every-client.decision.md'), 266) + // A retired number is never reused, so a tombstone still claims its number. + assert.equal(llpNumberOf('llp/tombstones/0018-retired.decision.md'), 18) + assert.equal(llpNumberOf('llp/0000-hypaware.explainer.md'), 0) + assert.equal(llpNumberOf('llp/reviews/0266-round-1.claude.md'), null) + assert.equal(llpNumberOf('llp/README.md'), null) + assert.equal(llpNumberOf('llp/266-unpadded.decision.md'), null) + assert.equal(llpNumberOf('notes-archive/0266-not-an-llp.md'), null) +}) + +// A document is tombstoned on one branch and not on another all the time, and +// that is one document at one number, not two claimants of it. +test('a document that only moved between directories is one claimant', () => { + const refFiles = new Map([ + ['refs/heads/master', ['llp/0100-a.spec.md']], + ['refs/heads/topic', ['llp/tombstones/0100-a.spec.md']], + ]) + const byName = claimsByNumber(refFiles).get(100) ?? new Map() + assert.deepEqual([...byName.keys()], ['0100-a.spec.md']) + assert.deepEqual(byName.get('0100-a.spec.md'), ['refs/heads/master', 'refs/heads/topic']) + assert.deepEqual(collisions(refFiles), []) +}) + +// A branch mints a number when it puts a document at one that had fewer, which +// is not the same as adding a filename. Slugs get rewritten during review all the +// time, and a slug rewrite claims nothing the branch did not already own. +test('a branch mints a number only when it adds a document at it', () => { + assert.deepEqual([...mintedAgainst(['llp/0100-a.spec.md'], ['llp/tombstones/0100-a.spec.md'])], []) + assert.deepEqual([...mintedAgainst(['llp/0100-a.spec.md'], ['llp/0100-a-renamed.spec.md'])], []) + assert.deepEqual([...mintedAgainst(['llp/0100-a.spec.md'], ['llp/0100-a.spec.md', 'llp/0101-b.spec.md'])], [101]) + // A second document at a number the base already carries is a mint of it. + assert.deepEqual([...mintedAgainst(['llp/0100-a.spec.md'], ['llp/0100-a.spec.md', 'llp/0100-b.spec.md'])], [100]) + // The LLP 0156 repair mints the number it moves to, and is checked there. + assert.deepEqual([...mintedAgainst(['llp/0100-a.spec.md'], ['llp/0281-a.spec.md'])], [281]) + assert.deepEqual([...mintedAgainst([], ['llp/README.md'])], []) +}) + +// The reproduction. Reading one branch at a time is exactly what each of the +// three workers did, and it is why all three answered 0266. +test('the next free number is the highest claimed on any ref, not on the checked-out one', t => { + const repo = collidedRepo(t) + const refs = ['refs/heads/master', ...COLLIDED.map(([branch]) => `refs/heads/${branch}`)] + const perRef = refFilesFromGit(repo, refs) + // Master alone is the tree all three workers read, and it answers 0266. + assert.equal(nextFreeNumber(new Map([['refs/heads/master', perRef.get('refs/heads/master') ?? []]])), 266) + // Across every ref that could still merge, 0266 is taken and 0267 is free. + assert.equal(nextFreeNumber(refFilesFromGit(repo, mergeableRefs(repo))), 267) +}) + +test('a number claimed by two documents on different refs is a collision', t => { + const repo = collidedRepo(t) + const found = collisions(refFilesFromGit(repo, mergeableRefs(repo))) + assert.equal(found.length, 1) + assert.equal(found[0].number, 266) + assert.deepEqual( + found[0].claimants.map(c => c.file).sort(), + COLLIDED.map(([, file]) => String(file.split('/').pop())).sort(), + ) +}) + +test('the check fails on the branch that minted the taken number, and names every claimant', t => { + const repo = collidedRepo(t) + git(repo, ['checkout', '-q', COLLIDED[1][0]]) + assert.deepEqual([...mintedNumbers(repo).numbers], [266]) + /** @type {string[]} */ + const err = [] + const code = run(['check'], repo, () => {}, text => err.push(text)) + assert.equal(code, 1) + const report = err.join('') + assert.match(report, /LLP 0266/) + for (const [, file] of COLLIDED) assert.ok(report.includes(String(file.split('/').pop())), report) + // And it says where to move: above everything claimed anywhere, per LLP 0156. + assert.match(report, /0267 or above \(LLP 0156\)/) +}) + +test('the check passes on the branch that mints nothing', t => { + const repo = collidedRepo(t) + assert.deepEqual([...mintedNumbers(repo).numbers], []) + /** @type {string[]} */ + const out = [] + assert.equal(run(['check'], repo, text => out.push(text), () => {}), 0) + assert.match(out.join(''), /no collision/) +}) + +// The gate has to be quiet about collisions it is not party to, or it stops +// being read. A branch cut before a repair still carries the pre-repair +// filename, so every already-settled collision in the corpus lives on in stale +// branches forever. +test('a settled collision surviving on a stale branch does not fail an unrelated branch', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0100-a.spec.md') + commit(repo, 'before the repair') + git(repo, ['branch', 'stale']) + git(repo, ['mv', 'llp/0100-a.spec.md', 'llp/0101-a.spec.md']) + writeDoc(repo, 'llp/0100-b.spec.md') + commit(repo, 'the repair of LLP 0156: the later claimant is renumbered') + git(repo, ['checkout', '-q', '-b', 'topic']) + writeDoc(repo, 'llp/0102-c.spec.md') + commit(repo, 'an unrelated branch minting the next free number') + + // Across refs, 0100 really is claimed twice, and a survey says so. + const refFiles = refFilesFromGit(repo, mergeableRefs(repo)) + assert.deepEqual(collisions(refFiles).map(c => c.number), [100]) + /** @type {string[]} */ + const surveyed = [] + assert.equal(run(['survey'], repo, text => surveyed.push(text), () => {}), 0) + assert.match(surveyed.join(''), /LLP 0100/) + + // The gate stays quiet: 0102 is all this branch minted. + assert.deepEqual([...mintedNumbers(repo).numbers], [102]) + assert.equal(run(['check'], repo, () => {}, () => {}), 0) +}) + +test('the next mode prints the free number zero-padded', t => { + const repo = collidedRepo(t) + /** @type {string[]} */ + const out = [] + assert.equal(run(['next'], repo, text => out.push(text), () => {}), 0) + assert.equal(out.join(''), '0267\n') +}) + +test('an unknown mode is a usage error, not a silent pass', t => { + const repo = collidedRepo(t) + assert.equal(run([], repo, () => {}, () => {}), 2) + assert.equal(run(['fix'], repo, () => {}, () => {}), 2) +}) + +// A document exists before it is committed, and two `/llp-create` calls in one +// session are the ordinary case. Reading committed trees only would hand the +// second one the number the first already took, which is issue #907 again with +// both claimants on the same machine. +test('a document written but not committed already claims its number', t => { + const repo = collidedRepo(t) + assert.equal(run(['next'], repo, () => {}, () => {}), 0) + + writeDoc(repo, 'llp/0300-still-a-draft.decision.md') + /** @type {string[]} */ + const out = [] + assert.equal(run(['next'], repo, text => out.push(text), () => {}), 0) + assert.equal(out.join(''), '0301\n') + + // And the gate sees it: an uncommitted doc at a taken number fails the check. + writeDoc(repo, 'llp/0266-also-a-draft.decision.md') + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 1) + assert.match(err.join(''), /LLP 0266/) +}) + +// Slugs get rewritten during review. A rewrite claims nothing the branch did not +// already own, so blaming it for a collision the corpus already had (and telling +// it to renumber) is a false positive with the wrong remedy attached. +test('changing a slug without changing the number is not a mint', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0100-a.spec.md') + commit(repo, 'the document that reached master first') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '-b', 'stale', 'master']) + writeDoc(repo, 'llp/0100-b.spec.md') + commit(repo, 'a settled collision at 0100 that a stale branch still carries') + + git(repo, ['checkout', '-q', '-b', 'topic', 'master']) + git(repo, ['mv', 'llp/0100-a.spec.md', 'llp/0100-a-clearer-slug.spec.md']) + commit(repo, 'reword the slug, keep the number') + + assert.deepEqual([...mintedNumbers(repo).numbers], []) + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 0, err.join('')) +}) + +// A truncated clone resolves the base ref but not a merge base. Reading that as +// an empty base counts the whole corpus as newly minted and blames the branch for +// every collision already settled in it. +test('a merge base that cannot be found is unknown, not an empty base', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0100-a.spec.md') + writeDoc(repo, 'llp/tombstones/0100-a-twin.spec.md') + commit(repo, 'a corpus that already carries a settled collision') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '--orphan', 'unrelated']) + commit(repo, 'a history with no common ancestor') + + const minted = mintedNumbers(repo) + assert.equal(minted.base, 'refs/remotes/origin/master') + assert.equal(minted.mergeBase, null) + assert.deepEqual([...minted.numbers], []) + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 0) + assert.match(err.join(''), /no common ancestor/) +}) + +// One document is carried by every branch cut since it landed, which on this +// repository is over a hundred refs. Printing them all buries the report. +test('a claimant line names enough refs to locate it and counts the rest', () => { + const refs = Array.from({ length: 30 }, (_, i) => `refs/heads/branch-${i}`) + const report = formatCollisions([{ number: 100, claimants: [{ file: '0100-a.spec.md', refs }] }], 281) + assert.match(report, /refs\/heads\/branch-0/) + assert.match(report, /and 24 more/) + assert.ok(!report.includes('refs/heads/branch-29'), report) +}) + +// A checkout that carries one branch and no default branch answers every mode +// from a corpus of one, which is the defect of issue #907 rather than a pass. +// `actions/checkout` hands exactly that to any job that does not ask for more, +// so the condition is detected rather than assumed away. +test('a checkout that cannot see the corpus says so instead of answering', t => { + assert.equal(partialScan(collidedRepo(t)), null) + + const alone = emptyRepo(t) + writeDoc(alone, 'llp/0100-a.spec.md') + commit(alone, 'one branch, no default branch, no remote') + git(alone, ['checkout', '-q', '-b', 'topic']) + git(alone, ['branch', '-q', '-D', 'master']) + assert.match(String(partialScan(alone)), /no default-branch ref/) + /** @type {string[]} */ + const err = [] + assert.equal(run(['next'], alone, () => {}, text => err.push(text)), 0) + assert.match(err.join(''), /^warning: /) + + assert.equal(partialScan(path.join(os.tmpdir(), 'llp-numbers-no-such-checkout')), NOT_A_CHECKOUT) +}) + +// The tip is the working tree, so it can carry base-branch content the fork +// point never had: an interrupted `git merge origin/master`, a +// `git checkout origin/master -- llp/`. Read against the merge base alone, every +// such document is newly minted here, and the branch is told to renumber one it +// never wrote. +test('a base-branch document sitting in this tree is inherited, not minted here', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0100-a.spec.md') + commit(repo, 'the fork point') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '-b', 'topic']) + writeDoc(repo, 'llp/0102-c.spec.md') + commit(repo, 'this branch mints 0102') + + // Master lands 0101, and a branch cut before that landing claims it too. + git(repo, ['checkout', '-q', 'master']) + writeDoc(repo, 'llp/0101-winner.spec.md') + commit(repo, 'a document that reached master while topic was out') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '-b', 'stale', 'master~1']) + writeDoc(repo, 'llp/0101-loser.spec.md') + commit(repo, 'a settled collision a stale branch still carries') + git(repo, ['update-ref', 'refs/remotes/origin/stale', 'stale']) + + // Topic pulls master in and the merge stops for a conflict elsewhere, so + // master's 0101 is on disk with nothing committed. + git(repo, ['checkout', '-q', 'topic']) + git(repo, ['checkout', 'origin/master', '--', 'llp/0101-winner.spec.md']) + + assert.deepEqual([...mintedNumbers(repo).numbers], [102]) + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 0, err.join('')) +}) + +// A rival that lands on the base branch first is still a collision, and the +// inheritance rule above must not swallow it: the base branch claims the number +// under a name this branch does not carry, so the branch really did mint it. +test('a rival document landing on the base branch first still fails the gate', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0265-a.plan.md') + commit(repo, 'the fork point') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '-b', 'topic']) + writeDoc(repo, 'llp/0266-mine.decision.md') + commit(repo, 'this branch mints 0266') + git(repo, ['checkout', '-q', 'master']) + writeDoc(repo, 'llp/0266-winner.decision.md') + commit(repo, 'a different 0266 reaches master first') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', 'topic']) + + assert.deepEqual([...mintedNumbers(repo).numbers], [266]) + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 1) + assert.match(err.join(''), /0266-winner\.decision\.md/) +}) + +// Slugs get reworded after review, and the pre-rename filename lives on at +// `refs/remotes/origin/<branch>` until the rename is pushed. That ref is +// reachable from HEAD, so it is a prior version of this branch, not a rival, and +// failing the gate against it prescribes renumbering a collision with yourself. +test('renaming the slug of a number this branch minted is not a collision with itself', t => { + const repo = emptyRepo(t) + writeDoc(repo, 'llp/0100-a.spec.md') + commit(repo, 'the fork point') + git(repo, ['update-ref', 'refs/remotes/origin/master', 'master']) + git(repo, ['checkout', '-q', '-b', 'topic']) + writeDoc(repo, 'llp/0101-first-slug.decision.md') + commit(repo, 'mint 0101') + git(repo, ['update-ref', 'refs/remotes/origin/topic', 'topic']) + git(repo, ['mv', 'llp/0101-first-slug.decision.md', 'llp/0101-clearer-slug.decision.md']) + commit(repo, 'reword the slug of the number this branch minted') + + assert.deepEqual([...supersededRefs(repo)].includes('refs/remotes/origin/topic'), true) + assert.deepEqual([...mintedNumbers(repo).numbers], [101]) + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], repo, () => {}, text => err.push(text)), 0, err.join('')) +}) + +// A gate whose failure mode is a silent pass is issue #907 one layer up: a job +// that lost its `fetch-depth: 0` would go green having compared one ref with +// itself. And the remedy has to be the one that works here: `--unshallow` aborts +// on a complete single-branch clone. +test('the check refuses a checkout that cannot see the corpus, and names a fetch that works', t => { + const alone = emptyRepo(t) + writeDoc(alone, 'llp/0100-a.spec.md') + commit(alone, 'one branch, no default branch, no remote') + git(alone, ['checkout', '-q', '-b', 'topic']) + git(alone, ['branch', '-q', '-D', 'master']) + + /** @type {string[]} */ + const err = [] + assert.equal(run(['check'], alone, () => {}, text => err.push(text)), 2) + const report = err.join('') + assert.match(report, /no default-branch ref/) + assert.match(report, /refs\/heads\/\*:refs\/remotes\/origin\/\*/) + assert.ok(!report.includes('--unshallow'), report) +}) + +// The in-suite half of the gate: what this branch mints has to be free in this +// repository, not only in a fixture. It can only answer that where the clone +// carries the other branches, so it says why it is skipping rather than passing +// green on a shallow or single-branch checkout that never looked. +test('this branch mints no number another ref already claims', t => { + const partial = partialScan(REPO_ROOT) + if (partial !== null) { + t.skip(`this is ${partial}: fetch every branch to run this check`) + return + } + const refFiles = scanRefFiles(REPO_ROOT) + assert.ok(refFiles.size > 0, 'expected at least one readable ref') + const found = collisions(refFiles, mintedNumbers(REPO_ROOT).numbers) + assert.deepEqual(found.map(c => `LLP ${c.number}: ${c.claimants.map(x => x.file).join(', ')}`), []) +})