docs(architecture): identify records by GitHub tracking number - #90
Merged
Conversation
Adopt the identification model decided in the main repository (exelearning/exelearning#2232, ADR-2232-01): a record is identified by the GitHub number of the change it belongs to, not by a global counter. The old rule was "IDs are zero-padded, monotonic and never reused; the next ID is max(existing) + 1". max(existing) was evaluated against whatever tree the author had checked out, nothing coordinated that across branches, and git cannot detect the collision because it manifests as two differently named files that merge cleanly. Issues are disabled on this repository, so every tracking number here is a pull request number. Cross-repository links -- the main eXeLearning repository, the Moodle plugin -- come from other sequences and are now kept as full URLs in external_refs instead of being written as bare numbers. ADRs (git mv, so git log --follow still resolves the history): ADR-0001 -> ADR-68-01-resolve-obsolete-hashes-via-attachment-post-meta ADR-0002 -> ADR-72-01-bundle-editor-exclusively-in-release-packages ADR-0003 -> ADR-86-01-make-distignore-single-source-of-truth Design documents move out of sdd/ into per-change directories, whole: SDD-0001 -> changes/68-stale-content-url-redirects/design.md SDD-0002 -> changes/86-unify-release-packaging/design.md SDD-0003 -> changes/88-testable-editor-bundle-paths/design.md SDD-0004 -> changes/88-editor-bootstrap-view-returns-html/design.md SDD-0005 -> changes/89-block-api-version-3/design.md #88 delivered two independent designs, so it owns two change directories. The alternative -- demoting one of them to spec.md -- would misrepresent what it is. Every record keeps its content and its ai_assistance provenance. Only the identifiers, the H1, the cross-references and the duplicated "## Status" section changed; status now lives in the frontmatter only. Statuses were case-folded to the change vocabulary but not otherwise "corrected". The hand-maintained records.md indexes are removed. The index is generated from frontmatter on demand and never committed: a generated file in git conflicts on every concurrent branch. docs/architecture/migration-map.md records every old -> new mapping and the deviations this repository makes from the reference model.
Comment, documentation and test references still named the retired identifiers,
which no longer resolve to a file.
ADR-0002 -> ADR-72-01 (Makefile, README.md, readme.txt, docs/HOOKS.md,
admin/class-admin-settings.php,
admin/views/editor-bootstrap.php,
includes/class-editor-bundle.php,
includes/class-upgrader.php,
AdminSettingsTest, UpgraderTest)
ADR-0003 -> ADR-86-01 (AGENTS.md)
SDD-0001 -> ADR-68-01 / changes/68-stale-content-url-redirects/
SDD-0003 -> changes/88-testable-editor-bundle-paths/
Design documents no longer have an identifier of their own, so the references
that pointed at one now name the ADR that carries the decision, or the change
directory that holds the design -- whichever the sentence actually meant.
AGENTS.md, CONVENTIONS.md, CLAUDE.md and README.md drop the "SDD" vocabulary
and the retired "IDs are monotonic and never reused" / "the index is maintained
by hand" rules, and describe the tracking-number model instead.
Comments only. No behaviour changes.
The tracking-number identification model is only worth adopting if something
enforces it. bin/architecture-records.php discovers the ADRs and the change
directories, validates their identifiers, metadata and cross-references, and
generates the two indexes.
php bin/architecture-records.php check # non-zero on failure
php bin/architecture-records.php list # print the index
It reports: filenames that do not match ADR-<number>-<NN>-<slug>.md (naming the
migration when the old form is used), frontmatter that disagrees with the
filename, missing or invalid metadata, statuses outside the vocabulary, an H1
that is not "<id>: <title>", duplicate ids and reused local sequences,
cross-references that do not resolve, one-sided supersession, implementation_prs
declared outside the canonical document, a committed index, and any retired
ADR-NNNN / SDD-NNNN identifier anywhere in the tree.
PHP rather than the reference implementation's Bun/TypeScript: this repository
already has PHP and Composer in every developer environment and in CI, and has
no Bun in the lint_and_test job. Adding a second runtime to lint documentation
would be a new dependency for no gain. bin/ is the established home for
standalone CLI tooling here, alongside validate-translations.php.
Two adaptations to this repository's conventions, both recorded in the
migration map:
- `deciders` is not required. AGENTS.md forbids people's names in
frontmatter, so requiring the field would force contributors to fabricate
provenance. The policy stands; the requirement is dropped.
- `external_refs` carries cross-repository links as full URLs, because
related.prs holds bare numbers of this repository and reusing it for
another repository's numbers would silently re-point them here.
Wired into `make check` via a new `make architecture-check`, and into a separate
`Architecture records` workflow. Separate because ci.yml carries
paths-ignore: ['**.md', 'docs/**'] and paths-ignore has no negation, so under CI
a pull request that only changes architecture records runs nothing at all --
which is exactly the pull request this validator exists for. The job is a
checkout plus one PHP script with no Composer packages, so running it unfiltered
costs seconds.
tests/unit/ArchitectureRecordsTest.php covers the parser, the discovery, every
validation rule and the index rendering against throwaway fixture trees, plus a
guard that this repository's own records pass.
The change-document template declares `related_changes`, and two migrated designs populate it: #88 owns two change directories, so a design there names its sibling by bare directory name. The validator read `related_adrs` but never `related_changes`, so a typo or a later directory rename passed silently — the one cross-reference a tracking-number identifier cannot make self-evident. Resolve every entry against the discovered change directories and reject a change that references itself, mirroring what the ADR side already does for `related.changes`.
Contributor
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
=========================================
Coverage 96.87% 96.87%
Complexity 864 864
=========================================
Files 39 39
Lines 4323 4323
=========================================
Hits 4188 4188
Misses 135 135
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PHPMD, whose findings surface as GitHub Advanced Security alerts, reported eight real problems in bin/architecture-records.php: validate() 242 lines, cyclomatic 65, NPath 2.4e14 parse_frontmatter() cyclomatic 21, NPath 10380 find_legacy_references() cyclomatic 18, NPath 3745 the class overall complexity 188 (threshold 100) None was a false positive. The file had grown into a single class doing frontmatter parsing, discovery, validation, scanning, rendering and the CLI. - `validate()` becomes an orchestrator over six focused rule methods, with the repeated date / status / tracking-number checks shared instead of inlined three times each. - `parse_frontmatter()` keeps its parser state in one structure and dispatches per line, so each branch is readable on its own. - `find_legacy_references()` separates allowlisting, file reading and the per-file scan. - Frontmatter parsing and validation move into `ExeLearning_Architecture_Frontmatter` and `ExeLearning_Architecture_Validator`. Decomposing alone does not lower class complexity -- it raised it to 198 -- so the concerns had to actually separate. `ExeLearning_Architecture_Records` keeps its documented public surface as delegating methods, so callers and tests are untouched. Behaviour is unchanged: no rule added, removed or altered. phpmd bin/architecture-records.php phpmd.xml clean (was 8 findings) make test OK, 984 tests / 2177 assertions make lint exit 0 make architecture-check OK, 3 ADRs, 5 changes npm run test:js exit 0, 95.7% lines
…ed script Four repositories had four implementations of the same rules — roughly 3,800 lines of validator and 2,700 of tests, in two languages — and they had already started to diverge. A differential test over the same adversarial fixtures found the PHP validators accepting a record with no `deciders` that the Python one rejected. There is now one implementation, byte-identical everywhere: architecture_records.py one file, standard library only architecture_records_test.py 48 tests, unittest, no dependency Python because it is the only runtime guaranteed on every CI image here, and because two of the four were already written in it. Only paths and the record prefix are configurable, in `architecture-records.json`. Every rule and every frontmatter key is identical in every repository, so the divergence cannot come back. Two things had to be reconciled to get there: - `deciders` is optional. Two repositories legitimately have records without it; inventing authorship would be worse than not recording it. - `related.prs` holds bare integers of this repository; cross-repository references move to `external_refs` as full URLs, which is what the omeka policy already said and its validator did not enforce. `0` is added as a tracking-number sentinel for records that predate tracking. GitHub numbers issues and pull requests from 1, so 0 can never collide with a real one.
…on port The validator is now one TypeScript file, byte-identical in every repository. It runs under Bun in core, where `bun test` covers it, and under the Node that ships on the CI image in the plugins, which need no setup step and no dependency. This replaces the Python port from the previous commit. Python was chosen when the alternative looked like installing Bun in four PHP pipelines. It is not: `setup-bun` appears only in release workflows that never run on a pull request, but Node is preinstalled on the runner image, and one source file runs under both runtimes once it avoids Bun-specific APIs. That keeps the tests where the scaffolding already is. Core keeps its 47 `bun test` cases with `expect()`; the plugins run the file itself on every pull request, which is what would surface a Node-specific regression. Repository-specific values live in `architecture-records.json` — paths and the record prefix only. Every rule is identical everywhere. bun run scripts/architecture-records.mts check OK node scripts/architecture-records.mts check OK (same output)
…sing Making `deciders` optional was the wrong reconciliation. It weakened the rule in every repository to accommodate five records that simply had not recorded who decided — a gap to fill, not a rule to relax. The field is required again, and the five records in wp-exelearning and omeka-s-exelearning now carry `@erseco` and `claude-code`, matching the pattern moodle-mod_exelearning already used for the same work. No model had to be inferred: `ai_assistance` was already present and consistent with each record's date (claude-fable-5 in July, claude-opus-5 in August). wp, omeka, nextcloud, moodle, core architecture-check OK core bun test 47 pass
Five documents still pointed at `scripts/architecture-records.ts`, which no longer exists: the file became `.mts` so the same source runs under Bun in core and under Node in the plugin repositories. The script's own usage header said `bun run` unconditionally, which is wrong in the four repositories that carry a copy and run it with Node. It now states both, and why the file must avoid runtime-specific APIs. core architecture-check OK, bun test 47 pass, lint OK all 5 byte-identical copies, architecture-check OK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Architecture records here were numbered from a globally sequential counter —
ADR-0001,SDD-0005— with the next ID computed asmax(existing) + 1and the index (records.md) maintained by hand.That counter is unsafe on parallel branches.
max(existing)is evaluated against whatever tree the author happens to have checked out, and nothing coordinates it across branches. Two branches opened the same week both pickADR-0004, and git cannot detect the collision: it manifests as two differently named files that merge cleanly. The hand-maintained index makes it worse —records.mdis a single table every branch appends to, so it conflicts on essentially every concurrent PR.The identifier also carried no information.
SDD-0003told a reader nothing about which change it belonged to.The main repository solved this in
exelearning/exelearning#2232(ADR-2232-01). This PR adopts the same model here.Identification model
A record is identified by the GitHub tracking number of the change it belongs to — the issue when there is one, otherwise the pull request. GitHub allocates issue and PR numbers from one repository-wide sequence, so they can never collide, and the number is allocated by GitHub rather than computed from a checkout.
<NN>is a two-digit sequence scoped to that tracking number alone, starting at01, present even for a single ADR.idmust equalADR-<number>-<NN>,tracking_issuemust hold the number, and the H1 must be exactly# <id>: <title>. CI enforces all three.## Statussections are gone.legacy_idkeeps the retired identifier on every migrated record, so provenance is not lost.Issues are disabled on this repository, so every tracking number here is a pull request number:
Each number was established from the squash-merge commit subject of the PR that introduced the record (
git log --follow --diff-filter=A), cross-checked withgh pr view. No issue was opened for this migration, and none could have been.Old to new mapping
Architecture Decision Records
ADR-0001-obsolete-hash-alias-storageADR-68-01-resolve-obsolete-hashes-via-attachment-post-meta7fad3beADR-0002-bundle-editor-exclusively-in-release-packagesADR-72-01-bundle-editor-exclusively-in-release-packages3a49e65ADR-0003-distignore-single-source-of-truthADR-86-01-make-distignore-single-source-of-trutha1b875fTwo slugs were rewritten because the original named the mechanism or the topic rather than the decision.
ADR-0002already named a decision, so its slug is unchanged.Design documents to change directories
docs/architecture/sdd/no longer exists. Each design moved whole intodesign.mdunder a per-change directory; all five were already complete designs, and splitting a shipped record acrossproposal.md/spec.md/design.mdwould rewrite history for no benefit.SDD-0001-stale-content-url-redirectschanges/68-stale-content-url-redirects/design.md7fad3beSDD-0002-unify-release-packagingchanges/86-unify-release-packaging/design.mda1b875fSDD-0003-testable-editor-bundle-pathschanges/88-testable-editor-bundle-paths/design.mdeaa9baeSDD-0004-editor-bootstrap-view-returns-htmlchanges/88-editor-bootstrap-view-returns-html/design.mdeaa9baeSDD-0005-block-api-version-3changes/89-block-api-version-3/design.md88346fcSupporting files
adr/template.mdadr/template.mdsdd/template.mdchanges/template.mdsdd/README.mdchanges/README.mdadr/records.md,sdd/records.mdmigration-map.mdNumbers
git mv.git diff --find-renamesreportsR088–R097, andgit log --followresolves the full pre-rename history on every one.ai_assistanceblock verbatim. Only identifiers, the H1, cross-references and the duplicated## Statussection changed. Nodeciders,authorsorreviewersfield was dropped — this repository never had one (see the deviations).Makefile,README.md,readme.txt,AGENTS.md,CONVENTIONS.md,CLAUDE.md,docs/HOOKS.md, 6 PHP source files and 7 test files. All of them are comments or docblocks — there is no behaviour change in this PR.The index is generated, not committed
docs/architecture/adr/records.mdanddocs/architecture/sdd/records.mdare deleted. The index is derived entirely from frontmatter and printed on demand:$ make architecture-records # or: composer architecture-recordsA committed index is a guaranteed merge conflict on every concurrent branch, and it is contributor-facing rather than published documentation. The validator now treats its presence as an error, so it cannot creep back. CI prints it into the job summary, so a reviewer can read the index without a checkout.
What CI now validates
New tool
bin/architecture-records.php(check/list), wired intomake check,make architecture-check,composer architecture-check, and a dedicated workflow.It validates:
idmatches the filename;tracking_issuematches the filename's number.# <id>: <title>.YYYY-MM-DDcalendar dates, and the two status vocabularies (Proposed|Accepted|Rejected|Supersededfor ADRs, lowercasedraft|in-review|accepted|implemented|superseded|abandonedfor changes).related.adrs,related.changes,related_adrs,related_changes,supersedes/superseded_by— including reciprocity (a superseded ADR must both name its successor and carrystatus: Superseded) and self-reference rejection.related.prs/implementation_prsare bare integers of this repository;external_refsare fullhttp(s)URLs.implementation_prs.<number>-<slug>and contain at least one recognised document.ADR-NNNN/SDD-NNNNidentifiers in every tracked and untracked file — not just Markdown, since they land in PHP comments and tests just as often. Allowlisted: the migration map,legacy_id:lines, a document naming its own former identifier, and the validator's own test fixtures..github/workflows/architecture-records.ymlis a separate workflow with no path filters, deliberately.ci.ymlcarriespaths-ignore: ['**.md', 'docs/**', '.github/*.md']and GitHub'spaths-ignorehas no negation syntax — so underci.ymla docs-only PR runs nothing at all, which is exactly the PR this validator exists for. The job is a checkout plus one PHP script with no Composer packages, so running it unfiltered costs seconds.Commands run, with results
php bin/architecture-records.php check(against pre-migration tree)php bin/architecture-records.php check(final)Architecture records OK — 3 ADRs, 5 changes.make architecture-records./vendor/bin/phpcs --standard=.phpcs.xml.dist -q .(the CI command)composer phpcs34 / 34 (100%)composer phpcbfmake testmake test FILE=tests/unit/ArchitectureRecordsTest.phpnpm run test:jsmake check-pluginSuccess: Checks complete. No errors found.make check-translationsTranslations are up to date and deterministic.; tree cleanThe validator was additionally mutation-tested rather than merely run: a broken H1, a legacy identifier planted in a PHP file, a committed
records.md, an id/filename mismatch, a danglingrelated_adrs, a danglingrelated_changes, a cross-repository number smuggled intorelated.prs, and a duplicated local sequence were each injected in turn. Every one produced a non-zero exit with a specific message; the tree was restored clean after each. The dangling-related_changescase is what produced the last commit on this branch — the field was declared in the template and populated by two records, but never resolved.Every relative Markdown link in
docs/,README.md,AGENTS.md,CONVENTIONS.mdandCLAUDE.mdwas resolved against the filesystem. The only two that do not resolve are illustrative examples inside inline-code spans, annotated "(adjust the relative path)".Deviations from the reference implementation
All five are documented in
docs/architecture/migration-map.md.exelearning/exelearning#2150) come from a different sequence and cannot identify a record here.deciders/reviewersrequirement. The reference validator requiresdeciders;AGENTS.mdstates "Use issue/PR links for attribution — no people's names in frontmatter or templates." No record here has ever carried one. Dropping the requirement beats fabricating provenance. Covered by a test.external_refsholds cross-repository links as full URLs, becauserelated.prsmust stay bare integers of this repository —moodle-mod_exelearning#106written as106would silently re-point here.spec.md, which misrepresents what it is. The reference model already anticipates this — it keys uniqueness on the directory name and sorts by number then slug.lint_and_test; Bun does not.bin/is the established home for standalone CLI tooling (bin/validate-translations.php), and is excluded from PHPCS, from the plugin check and from the release ZIP via.distignore.Known limitations
migration-map.md,adr/README.mdandchanges/README.md, citingexelearning/exelearning#2232/ADR-2232-01as the source decision. Follow-up now that this PR has a number: adddocs/architecture/adr/ADR-<pr>-01-adopt-tracking-number-identifiers.mdwithtracking_issue: <pr>andexternal_refs: [https://github.com/exelearning/exelearning/issues/2232], capturing the five deviations above.make architecture-checkwill validate it.Acceptedtoaccepted,Drafttodraft,Implementedtoimplemented. The values are preserved as they were:ADR-86-01/86-unify-release-packagingstayProposed/drafteven though Make .distignore the single source of truth for the release ZIP #86 merged. Whether those are still accurate is an editorial question, deliberately not folded into a renumbering.supersedes/superseded_byon change documents are declared but not resolved. No change uses them, and the reference implementation does not validate them either. The ADR side is fully validated, including reciprocity.migration-map.mdis the lookup table, andlegacy_idkeeps each record self-describing.docs/architecture/*/records.mdis not.gitignored. The validator errors if either is committed, andlistwrites only to stdout, so a stray index cannot land silently — but a.gitignorerule would be belt-and-braces.