diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 37ade01..0fffe20 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,6 +88,17 @@ jobs: python-version: "3.14" - run: python3 meta/scripts/check_go_work.py + adr-numbers-check: + name: ADR number uniqueness check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + # renovate: datasource=python-version depName=python + python-version: "3.14" + - run: python3 meta/scripts/check_adr_numbers.py + secrets-check: name: Secrets check runs-on: ubuntu-latest diff --git a/.vscode/settings.json b/.vscode/settings.json index 59b7c2a..ed22bb7 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -52,6 +52,9 @@ "check: go work": [ "**/go.mod", "go.work" + ], + "check: adr numbers": [ + "**/docs/adr/*.md" ] }, "triggerTaskOnSave.showStatusBarToggle": true, diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 213e46f..4d2e486 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -62,6 +62,33 @@ } } }, + { + "label": "check: adr numbers", + "type": "shell", + "command": "python3", + "args": ["${workspaceFolder}/meta/scripts/check_adr_numbers.py"], + "presentation": { + "reveal": "silent", + "panel": "dedicated", + "showReuseMessage": false, + "clear": true, + "revealProblems": "onProblem" + }, + "problemMatcher": { + "owner": "check-adr-numbers", + "source": "check_adr_numbers", + "fileLocation": ["relative", "${workspaceFolder}"], + "severity": "error", + "pattern": { + "regexp": "^(\\S+):(\\d+):(\\d+)-(\\d+):\\s+(.+)$", + "file": 1, + "line": 2, + "column": 3, + "endColumn": 4, + "message": 5 + } + } + }, { "label": "Bazel Build all", "detail": "bazel build //...", diff --git a/CONTEXT-MAP.md b/CONTEXT-MAP.md index c388579..ebaf12a 100644 --- a/CONTEXT-MAP.md +++ b/CONTEXT-MAP.md @@ -5,7 +5,8 @@ directories in the [README](README.md), not a directory under `src/`. See [`docs/agents/domain.md`](docs/agents/domain.md) for how the engineering skills consume this file. Each context owns its own `CONTEXT.md` (its glossary) and its own `docs/adr/` (decisions scoped to -it). Repo-wide decisions live in `docs/adr/`. +it). Repo-wide decisions live in `docs/adr/`. ADR *numbers* are unique across all of those +directories rather than per directory — see [`docs/agents/domain.md`](docs/agents/domain.md). ## Contexts @@ -27,8 +28,7 @@ it). Repo-wide decisions live in `docs/adr/`. Treat a request for anything under "planned" as unimplemented: a redundancy check that matched a request against this domain language would otherwise close a real one as already-done. -- **Repo meta** — `meta/` - _(`CONTEXT.md` not yet written)_ +- **Repo meta** — `meta/` ([`CONTEXT.md`](meta/CONTEXT.md)) The monorepo's own automation: `meta/scripts/` (pre-commit checks, changed-path classification, Renovate proposal ratification and manual-job triggering, base-image pin sync) and diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 7c5c608..9a8548e 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -10,6 +10,28 @@ codebase. - **`docs/adr/`**: system-wide architectural decisions. - **`/docs/adr/`**: decisions scoped to a single context. +**ADR numbers are unique repo-wide, not per directory.** A new ADR takes the next number across +*every* `docs/adr/` directory in the repo, whichever one it lands in — so `docs/adr/0001-…` and +`meta/docs/adr/0002-…` is correct and a second `0001` anywhere is not. This deliberately diverges +from the `/domain-modeling` skill's `ADR-FORMAT.md`, which scans a single directory; follow this +file, and note that a per-directory scan will hand you a number that is already taken. The +directory still says whose decision it is: repo-wide, or one context's. + +**To get the next number, ask the check — don't search the repo:** + +``` +python3 meta/scripts/check_adr_numbers.py --next +``` + +It prints the number alone (e.g. `0003`) so it can be substituted straight into a filename, and it +answers even while a collision exists, since that is when you most need it. The number is the +highest in use plus one, never a gap left by a deleted ADR — reusing one would break every +reference to the original. + +`meta/scripts/check_adr_numbers.py` with no arguments enforces both the uniqueness and the +`NNNN-kebab-slug.md` filename shape, as the `ADR number uniqueness check` CI job. A duplicate is +reported with a free number to move to, so a collision carries its own fix. + If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved. diff --git a/meta/CONTEXT.md b/meta/CONTEXT.md new file mode 100644 index 0000000..4ff5e27 --- /dev/null +++ b/meta/CONTEXT.md @@ -0,0 +1,60 @@ +# Repo meta + +The monorepo's own automation: the repo-health checks that enforce cross-cutting invariants, and +the shared devcontainer base image that Syndic repos build their development environments on. It +operates on the other contexts rather than shipping anything a user runs. + +## Language + +### Devcontainer base image + +**Base image**: +The devcontainer image this context assembles and publishes for Syndic repos to build on. It +carries the container half of the plumbing and nothing else — no language toolchains, no features. +_Avoid_: shared image, common image + +**Consumer**: +A repo whose devcontainer Dockerfile `FROM`s the base image. This repo is one; `Syndic/.dotfiles` +is the other. +_Avoid_: client, downstream, dependent + +**Plumbing**: +The bridging of host state into a container — the git common directory, the host timezone, the +material a signed commit needs. Not git's plumbing/porcelain sense: nothing here is a low-level git +command. +_Avoid_: bootstrap, host integration + +**Host stub**: +The `initializeCommand` script a consumer keeps, which runs on the developer's host before any +container exists. Its job is to present host state in the shape the container consumes, without +dictating where the host keeps anything. +_Avoid_: initialize script, host script + +**Dispatcher**: +The single command a consumer's lifecycle hooks call to apply every shared plumbing step for a +phase. Being one command rather than a library is what lets a new step reach every consumer on its +next base image bump, with no edit on their side. + +**Contract**: +The interface between a consumer's host stub and the base image: the file names, symlink names and +values the stub must present, plus the destinations the plumbing writes to. Conforming to it is +what adoption means. + +### Repo health + +**Check**: +A guard that enforces an invariant no single language toolchain owns. Whether it blocks depends on +where it runs — CI, pre-commit, or on-save in the editor — not on what it is. +_Avoid_: gate, guard, linter, validator + +**Derived file**: +A checked-in file that is reproducible from other checked-in sources, so it is regenerated rather +than hand-edited and its staleness is a defect a check can find. Distinct from a dependency, whose +value comes from outside the repo and is bumped rather than derived — the same artifact can be one +in this repo and the other in a consumer. +_Avoid_: generated file, lockfile + +**Marker comment**: +A comment that tells Renovate which version string a line carries, where the file's own syntax +can't. A marker no pattern claims is invisible rather than broken, which is the failure mode it is +prone to. diff --git a/meta/docs/adr/0002-the-plumbing-contract-includes-the-host-presentation-shape.md b/meta/docs/adr/0002-the-plumbing-contract-includes-the-host-presentation-shape.md new file mode 100644 index 0000000..3c203c2 --- /dev/null +++ b/meta/docs/adr/0002-the-plumbing-contract-includes-the-host-presentation-shape.md @@ -0,0 +1,42 @@ +# The plumbing contract includes the host-presentation shape + +The shared devcontainer base image's contract with a consumer is not merely "call the dispatcher". +It includes the shape the host stub must present: fixed file names under `.git-plumbing/`, fixed +symlink names beside it, and the destinations the plumbing writes to. A consumer conforms to that +layout. What stays free is where on the host each value is read from, which is what the symlink +indirection buys. + +## Considered options + +### The steps only — rejected + +Under this reading the image publishes behaviour, and each consumer presents host state however it +likes. It is what `meta/devcontainer-base/README.md` implied by saying the stub "does not constrain +where the host keeps anything". + +The code never matched it. `plumbing_apply_git_common` reads `$PLUMBING_DIR/host-git-common-path` +by name and links it to `/host-git-common`; `plumbing_apply_all` reads `host-timezone` by name. +Those are contract terms whether or not they are written down as such. Leaving them undocumented +means the next consumer discovers them by breakage rather than by reading, and `Syndic/.dotfiles` +demonstrates the cost: it presents `known_hosts` and `allowed_signers` as snapshot files where this +repo presents them as bind-mounted symlinks, so its copy of the plumbing diverged in kind rather +than in detail. + +### The steps plus the shape — accepted + +The contract names the files, the symlinks and the destinations, and the README documents them as +an interface. The host stub's remaining freedom is real but bounded: it chooses what each symlink +points at, so a developer's `known_hosts` can live anywhere, but it does not choose the name the +container looks for. + +## Consequences + +- Adoption is conformance, not integration. `Syndic/.dotfiles` moves to the documented shape rather + than keeping its own; its snapshot-based handling is an artefact of age, not a variant to support. +- A consumer that wants a different layout has one supported answer — change the symlink target — + and one unsupported one. That is deliberate: configurability on this axis would reintroduce the + divergence the shared image exists to remove. +- The contract is versioned by the image digest like everything else, so widening it is a base + image change that reaches consumers on their next bump. +- What the image cannot carry stays outside the contract and unsolved: the host stub itself, the + two bind mounts, and `containerEnv`. Tracked in #248. diff --git a/meta/scripts/BUILD.bazel b/meta/scripts/BUILD.bazel index 31b1102..aaad66d 100644 --- a/meta/scripts/BUILD.bazel +++ b/meta/scripts/BUILD.bazel @@ -38,6 +38,30 @@ py_test( ], ) +py_library( + name = "check_adr_numbers_lib", + srcs = ["check_adr_numbers.py"], + deps = [":_workspace_lib"], +) + +py_binary( + name = "check_adr_numbers", + srcs = ["check_adr_numbers.py"], + main = "check_adr_numbers.py", + deps = [":_workspace_lib"], +) + +py_test( + name = "test_check_adr_numbers", + size = "small", + srcs = ["test_check_adr_numbers.py"], + main = "test_check_adr_numbers.py", + deps = [ + ":_workspace_lib", + ":check_adr_numbers_lib", + ], +) + py_library( name = "check_modules_lib", srcs = ["check_modules.py"], diff --git a/meta/scripts/README.md b/meta/scripts/README.md index af96aca..525cdc5 100644 --- a/meta/scripts/README.md +++ b/meta/scripts/README.md @@ -10,9 +10,16 @@ without blocking). | `check_modules.py` | Go module matrix/config and Python workspace/lock invariants are consistent | `ci.yml`, `security.yml` | — | `check: modules` | | `check_go_work.py` | Every Go module in the repo is registered in `go.work` | `ci.yml` | — | `check: go work` | | `check_no_cgo.py` | No `import "C"` in our Go source and no transitive deps that compile C/C++/cgo/SWIG | `ci.yml` | — | — | +| `check_adr_numbers.py` | ADR numbers are unique repo-wide and filenames are `NNNN-kebab-slug.md` | `ci.yml` | — | `check: adr numbers` | | `check_secrets_dir.py` | `secrets/` contains no committed files other than `secrets.md` | `ci.yml` | `check-secrets-dir` | — | -`_workspace.py` is a private shared helper for the four guards above (Bazel workspace discovery, +`check_adr_numbers.py` also answers `--next`, which prints the next free ADR number and nothing +else. That is the supported way to pick one — the alternative is a repo-wide search, since the +numbering is global while the directories are per-context. It deliberately has no counter file to +read: the numbers already live in the filenames, and a second copy would be a derived file needing +its own freshness check, and a merge conflict on every concurrent ADR. + +`_workspace.py` is a private shared helper for the five guards above (Bazel workspace discovery, module enumeration). The leading underscore signals it's not a public API; `test__workspace.py` covers it directly. diff --git a/meta/scripts/check_adr_numbers.py b/meta/scripts/check_adr_numbers.py new file mode 100644 index 0000000..4d8cebf --- /dev/null +++ b/meta/scripts/check_adr_numbers.py @@ -0,0 +1,123 @@ +#!/usr/bin/env python3 +""" +Verifies that ADR numbers are unique across every decision-record directory in the repo: + - Every file in a docs/adr/ directory is named NNNN-kebab-slug.md (README.md excepted) + - No two ADRs anywhere in the repo share a number + +Numbering is repo-global, not per-directory: a context's ADR takes the next number across +*all* of docs/adr/ and /docs/adr/. See docs/agents/domain.md. + +Usage: ./meta/scripts/check_adr_numbers.py # check + ./meta/scripts/check_adr_numbers.py --next # print the next free number +""" + +import re +import sys +from collections import defaultdict +from pathlib import Path + +# When invoked as `python3 meta/scripts/check_adr_numbers.py` (the form used in CI and by the +# editor task), the workspace root is not on sys.path, so `from meta.scripts.X` would fail. +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from meta.scripts._workspace import find_files, workspace_root + +# Four digits so a plain sort orders them; kebab slug so filenames stay greppable and +# case-insensitive filesystems can't collide two ADRs onto one path. +_ADR_NAME_RE = re.compile(r"^(\d{4})-[a-z0-9]+(?:-[a-z0-9]+)*\.md$") + +# A directory-level readme is not a decision record. Nothing else earns an exemption: +# an unrecognised file here is either a misnamed ADR or something in the wrong place. +_EXEMPT = frozenset(["README.md"]) + + +def adr_files(root: Path) -> list[Path]: + """Every markdown file under a docs/adr/ directory, sorted, relative to root.""" + return sorted( + p.relative_to(root) + for p in find_files(root, "*.md") + if p.parent.name == "adr" and p.parent.parent.name == "docs" + ) + + +def next_number(files: list[Path]) -> str: + """The lowest number no ADR has taken yet: highest in use plus one, or 0001 if none are. + + Deliberately max+1 rather than count+1, so a gap left by a deleted or renamed ADR is never + handed out again -- a reused number would break every reference to the original. + """ + used = [int(n) for n in _numbers_by_path(files)] + return f"{(max(used) + 1) if used else 1:04d}" + + +def _numbers_by_path(files: list[Path]) -> dict[str, list[Path]]: + """Number -> the ADR paths claiming it. Malformed and exempt names are absent.""" + by_number: dict[str, list[Path]] = defaultdict(list) + for path in files: + if path.name in _EXEMPT: + continue + match = _ADR_NAME_RE.match(path.name) + if match is not None: + by_number[match.group(1)].append(path) + return by_number + + +def violations(files: list[Path]) -> list[tuple[Path, str]]: + """(path, message) for every malformed name and every reused number.""" + found: list[tuple[Path, str]] = [] + by_number = _numbers_by_path(files) + + for path in files: + if path.name not in _EXEMPT and _ADR_NAME_RE.match(path.name) is None: + found.append( + (path, f"not a valid ADR filename: expected NNNN-kebab-slug.md, got {path.name}") + ) + + # Named in the message so a collision carries its own fix. "or later" because several + # duplicates all see the same free number and only the first can take it. + free = next_number(files) + for number, paths in by_number.items(): + if len(paths) == 1: + continue + first, *rest = paths + for path in rest: + found.append( + ( + path, + f"duplicate ADR number {number}: already used by {first}; " + f"renumber to {free} or later", + ) + ) + + return sorted(found) + + +def run(root: Path, next_only: bool) -> int: + files = adr_files(root) + + if next_only: + # A query, not a check: prints the number and nothing else, so it can be substituted + # into a filename. Reports a number even when the tree has violations -- `--next` is + # how you get out of a collision, so refusing to answer during one would be backwards. + print(next_number(files)) + return 0 + + found = violations(files) + + for path, message in found: + # `path:line:startCol-endCol: message`, the format the editor's problem matcher reads. + # The offence is the filename, so there is no interesting position inside the file. + print(f"{path}:1:1-2: {message}") + + if not found: + print(f"ADR numbers are unique across {len(files)} decision record(s).") + + return len(found) + + +def main() -> int: + return run(workspace_root(), "--next" in sys.argv[1:]) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/meta/scripts/test_check_adr_numbers.py b/meta/scripts/test_check_adr_numbers.py new file mode 100644 index 0000000..a4a2156 --- /dev/null +++ b/meta/scripts/test_check_adr_numbers.py @@ -0,0 +1,176 @@ +#!/usr/bin/env python3 +"""Unit tests for check_adr_numbers.py.""" + +import contextlib +import io +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from meta.scripts.check_adr_numbers import adr_files, next_number, run, violations + + +def _paths(*names: str) -> list[Path]: + return [Path(n) for n in names] + + +class TestAdrFiles(unittest.TestCase): + """Discovery has to span every docs/adr/ in the tree, and nothing else.""" + + def _tree(self, *rel: str) -> Path: + root = Path(tempfile.mkdtemp()) + for r in rel: + p = root / r + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text("# x\n") + return root + + def test_finds_root_and_context_directories(self): + root = self._tree("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md") + self.assertEqual(adr_files(root), _paths("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md")) + + def test_ignores_markdown_outside_an_adr_directory(self): + # A check that swept every *.md would flag the repo's ordinary docs. + root = self._tree("docs/adr/0001-a.md", "docs/agents/domain.md", "README.md") + self.assertEqual(adr_files(root), _paths("docs/adr/0001-a.md")) + + def test_ignores_an_adr_directory_not_under_docs(self): + root = self._tree("adr/0001-a.md", "docs/adr/0002-b.md") + self.assertEqual(adr_files(root), _paths("docs/adr/0002-b.md")) + + def test_skips_build_output_and_vcs_directories(self): + # find_files' skip list is what keeps a bazel-* symlink from doubling every ADR. + root = self._tree("docs/adr/0001-a.md", "bazel-out/docs/adr/0001-a.md") + self.assertEqual(adr_files(root), _paths("docs/adr/0001-a.md")) + + +class TestViolations(unittest.TestCase): + def test_unique_numbers_pass(self): + self.assertEqual(violations(_paths("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md")), []) + + def test_duplicate_across_directories_fails(self): + # The case global numbering exists to prevent: per-directory numbering produces this. + found = violations(_paths("docs/adr/0001-a.md", "meta/docs/adr/0001-b.md")) + self.assertEqual(len(found), 1) + path, message = found[0] + self.assertEqual(path, Path("meta/docs/adr/0001-b.md")) + self.assertIn("duplicate ADR number 0001", message) + self.assertIn("docs/adr/0001-a.md", message) + + def test_duplicate_message_names_a_free_number(self): + # The failure should carry its own fix rather than sending the reader back to --next. + found = violations(_paths("docs/adr/0001-a.md", "docs/adr/0001-b.md")) + self.assertIn("renumber to 0002 or later", found[0][1]) + + def test_duplicate_within_one_directory_fails(self): + found = violations(_paths("docs/adr/0001-a.md", "docs/adr/0001-b.md")) + self.assertEqual(len(found), 1) + + def test_a_third_use_is_reported_too(self): + # Reporting only the first collision would let a fix land and still leave a duplicate. + found = violations(_paths("docs/adr/0001-a.md", "docs/adr/0001-b.md", "docs/adr/0001-c.md")) + self.assertEqual(len(found), 2) + + def test_readme_is_exempt(self): + self.assertEqual(violations(_paths("docs/adr/README.md", "docs/adr/0001-a.md")), []) + + def test_malformed_names_are_reported(self): + for name in ( + "1-a.md", + "00001-a.md", + "0001a.md", + "0001-.md", + "0001-Mixed-Case.md", + "notes.md", + ): + with self.subTest(name=name): + found = violations(_paths(f"docs/adr/{name}")) + self.assertEqual(len(found), 1, f"{name} should be rejected") + self.assertIn("not a valid ADR filename", found[0][1]) + + def test_malformed_name_is_not_also_counted_as_a_number(self): + # A file that fails the shape has no number to collide with; reporting both would + # double-count one mistake. + found = violations(_paths("docs/adr/0001-a.md", "docs/adr/0001a.md")) + self.assertEqual(len(found), 1) + self.assertIn("not a valid ADR filename", found[0][1]) + + def test_valid_slugs_with_digits_are_accepted(self): + self.assertEqual(violations(_paths("docs/adr/0007-use-oauth2-for-sso.md")), []) + + +class TestNextNumber(unittest.TestCase): + def test_empty_repo_starts_at_one(self): + self.assertEqual(next_number([]), "0001") + + def test_is_max_plus_one_not_count_plus_one(self): + # A gap from a deleted ADR must not be handed out again: 0002 may still be cited. + self.assertEqual(next_number(_paths("docs/adr/0001-a.md", "docs/adr/0003-c.md")), "0004") + + def test_spans_every_directory(self): + self.assertEqual( + next_number(_paths("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md")), "0003" + ) + + def test_ignores_malformed_and_exempt_names(self): + self.assertEqual( + next_number(_paths("docs/adr/0001-a.md", "docs/adr/README.md", "docs/adr/9999x.md")), + "0002", + ) + + def test_answers_during_a_collision(self): + # --next is how you get out of a duplicate, so it has to work while one exists. + self.assertEqual(next_number(_paths("docs/adr/0001-a.md", "docs/adr/0001-b.md")), "0002") + + def test_pads_to_four_digits(self): + self.assertEqual(next_number(_paths("docs/adr/0009-i.md")), "0010") + + +class TestRun(unittest.TestCase): + """The CLI's two modes, driven without shelling out.""" + + def _tree(self, *rel: str) -> Path: + root = Path(tempfile.mkdtemp()) + for r in rel: + p = root / r + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text("# x\n") + return root + + def _capture(self, root: Path, next_only: bool) -> tuple[int, str]: + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + code = run(root, next_only) + return code, buf.getvalue() + + def test_next_prints_only_the_number(self): + # Printed bare so it can be substituted straight into a filename. + root = self._tree("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md") + code, out = self._capture(root, next_only=True) + self.assertEqual(code, 0) + self.assertEqual(out, "0003\n") + + def test_next_succeeds_even_when_the_tree_is_invalid(self): + root = self._tree("docs/adr/0001-a.md", "docs/adr/0001-b.md") + code, out = self._capture(root, next_only=True) + self.assertEqual(code, 0) + self.assertEqual(out, "0002\n") + + def test_check_mode_reports_and_exits_nonzero(self): + root = self._tree("docs/adr/0001-a.md", "meta/docs/adr/0001-b.md") + code, out = self._capture(root, next_only=False) + self.assertEqual(code, 1) + self.assertIn("meta/docs/adr/0001-b.md:1:1-2:", out) + + def test_check_mode_is_quiet_and_zero_when_clean(self): + root = self._tree("docs/adr/0001-a.md", "meta/docs/adr/0002-b.md") + code, out = self._capture(root, next_only=False) + self.assertEqual(code, 0) + self.assertIn("unique across 2", out) + + +if __name__ == "__main__": + unittest.main()