diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b77070f3cc..488b301ff0 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -46,6 +46,11 @@ /backend/internal/system/jose/ @hwupathum +## architectural decision records + +/docs/adrs/ @darshanasbg @madurangasiriwardena + + ## scripts /backend/dbscripts/ @rajithacharith @darshanasbg diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 181ba19526..d00231aa48 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -29,8 +29,14 @@ _How should users update their code/configuration to adapt to the breaking chang ### Related PRs - N/A +### Related ADRs + +- N/A + ### Checklist - [ ] Followed the contribution guidelines. +- [ ] Checked whether this change requires an [ADR](https://github.com/thunder-id/thunderid/blob/main/docs/adrs/README.md#when-an-adr-is-required); if so, it is linked under Related ADRs. - [ ] Manual test round performed and verified. - [ ] Documentation provided. (Add links if there are any) - [ ] Ran Vale and fixed all errors and warnings diff --git a/.github/workflows/adr-lint.yml b/.github/workflows/adr-lint.yml new file mode 100644 index 0000000000..edcf639703 --- /dev/null +++ b/.github/workflows/adr-lint.yml @@ -0,0 +1,99 @@ +name: ๐Ÿ“ ADR Lint + +on: + pull_request: + paths: + - 'docs/adrs/**' + merge_group: + workflow_dispatch: + +permissions: + contents: read + +jobs: + adr-checks: + name: ๐Ÿ“ Validate Decision Records + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: ๐Ÿ“ฅ Checkout Code + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: false + + # The root package.json pins pnpm through `devEngines`, which makes npm + # refuse to run `npx` anywhere in this repository. + - name: โš™๏ธ Set up Node.js and pnpm + uses: ./.github/actions/setup-pnpm + with: + cache-store: 'false' + + - name: ๐Ÿงน Run markdownlint + run: > + pnpm dlx markdownlint-cli2@0.14.0 + --config docs/adrs/.markdownlint.yml + "docs/adrs/*.md" + + - name: ๐Ÿ” Check ADR Conventions + run: | + set -euo pipefail + + shopt -s nullglob + records=(docs/adrs/[0-9]*.md) + shopt -u nullglob + + if [ ${#records[@]} -eq 0 ]; then + echo "No decision records found." + exit 0 + fi + + fail=0 + + for f in "${records[@]}"; do + base=$(basename "$f") + + # Filename must be NNNN-lowercase-with-dashes.md + if ! [[ "$base" =~ ^[0-9]{4}-[a-z0-9]+(-[a-z0-9]+)*\.md$ ]]; then + echo "::error file=$f::filename must match NNNN-title-with-dashes.md" + fail=1 + fi + + # A status field must be present and hold a recognised value. + status=$(awk '/^---$/{n++; next} n==1 && /^status:/{print; exit}' "$f" || true) + if [ -z "$status" ]; then + echo "::error file=$f::missing status in YAML front matter" + fail=1 + elif ! echo "$status" | grep -Eq '^status: *"(proposed|rejected|accepted|deprecated|superseded by ADR-[0-9]{4})"'; then + echo "::error file=$f::unrecognised status value: $status" + fail=1 + fi + + # Every record must appear in the index table, and the status column + # must agree with the record's front matter. + # Restricted to table rows: a passing mention in the README prose is + # not an index entry. + row=$(awk -v needle="($base)" \ + 'substr($0, 1, 1) == "|" && index($0, needle)' docs/adrs/README.md) + if [ -z "$row" ]; then + echo "::error file=$f::not listed in docs/adrs/README.md index" + fail=1 + elif [ -n "$status" ]; then + declared=$(echo "$status" | sed -E 's/^status:[[:space:]]*"?//; s/"?[[:space:]]*$//') + indexed=$(echo "$row" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print $4}') + if [ "$declared" != "$indexed" ]; then + echo "::error file=$f::index status \"$indexed\" does not match front matter status \"$declared\"" + fail=1 + fi + fi + done + + # Numbers must be unique. + dupes=$(printf '%s\n' "${records[@]}" \ + | xargs -n1 basename \ + | cut -c1-4 | sort | uniq -d) + if [ -n "$dupes" ]; then + echo "::error::duplicate ADR numbers: $dupes" + fail=1 + fi + + exit $fail diff --git a/.github/workflows/pr-builder.yml b/.github/workflows/pr-builder.yml index d94549a679..4182326ebe 100644 --- a/.github/workflows/pr-builder.yml +++ b/.github/workflows/pr-builder.yml @@ -66,6 +66,7 @@ jobs: filters: | docs: - 'docs/**' + - '!docs/adrs/**' - 'package.json' - 'pnpm-lock.yaml' - 'pnpm-workspace.yaml' diff --git a/.vale.ini b/.vale.ini index c4a46ccfe1..2a7d3f3bbe 100644 --- a/.vale.ini +++ b/.vale.ini @@ -34,6 +34,9 @@ Vale.PassiveVoice = NO Vale.Readability = NO TokenIgnores = (<[A-Z][^>]*>),(]*>),(<(?!\!--)[^>]+>),(\!\[[^\]]*\]\([^)]+\)),(\[[^\]]*\]\([^)]+\)),(\[[^\]]*\]\[[^\]]+\]),(\[[^\]]+\]:\s+\S+),(@[a-zA-Z0-9_-]+\/[a-zA-Z0-9._/-]+),(\{`[^`]*`\}) +[**/adrs/*.md] +BasedOnStyles = + [**/SKILL.md] BasedOnStyles = diff --git a/AGENTS.md b/AGENTS.md index aa2e7cbb8b..a3646b7c2a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,7 @@ Load only the guidance the task needs: | Backend Go code | [backend/AGENTS.md](backend/AGENTS.md) | | Frontend React code | [frontend/AGENTS.md](frontend/AGENTS.md) | | Documentation | [docs/AGENTS.md](docs/AGENTS.md) | +| A decision that needs an architectural record | [docs/adrs/README.md](docs/adrs/README.md) | | Database schema, queries, or stores | [.agent/skills/db/SKILL.md](.agent/skills/db/SKILL.md) | | Browser automation / Console UI verification | [.agent/skills/console/SKILL.md](.agent/skills/console/SKILL.md) | diff --git a/docs/adrs/.markdownlint.yml b/docs/adrs/.markdownlint.yml new file mode 100644 index 0000000000..4866b550d7 --- /dev/null +++ b/docs/adrs/.markdownlint.yml @@ -0,0 +1,22 @@ +# markdownlint configuration for architectural decision records. +# Based on the configuration shipped with MADR 4.0.0. + +# MD013/line-length: disabled. Records use one sentence per line, so lines vary +# in length by design and wrapping them would make diffs harder to review. +MD013: false + +# MD024/no-duplicate-heading: allow the same heading text under different parents. +# "Consequences" appears under Decision Outcome and option headings repeat under +# "Pros and Cons of the Options". +MD024: + siblings_only: true + +# MD004/ul-style: MADR uses asterisks as list markers. +MD004: + style: asterisk + +# MD025/single-title: one H1 per file, which is the record title. +MD025: true + +# MD034/no-bare-urls: keep URLs in angle brackets or link syntax. +MD034: true diff --git a/docs/adrs/0000-use-madr.md b/docs/adrs/0000-use-madr.md new file mode 100644 index 0000000000..6d69d13614 --- /dev/null +++ b/docs/adrs/0000-use-madr.md @@ -0,0 +1,113 @@ +--- +status: "accepted" +date: "2026-09-01" +decision-makers: "@madurangasiriwardena @darshanasbg" +--- + +# Use MADR for architectural decision records + +## Context and Problem Statement + +ThunderID has no decision log, in the sense of a maintained collection of records that each capture one architectural decision (AD) and its rationale. +Design reasoning currently lives in GitHub Discussions, pull request threads, and, for decisions inherited from earlier work, internal mail threads that external contributors cannot read at all. + +This has three costs that grow as the project does. +Contributors cannot tell whether an approach was already considered and rejected, so the same proposals resurface. +Decisions that were deliberate deviations from a specification are indistinguishable from oversights, which matters disproportionately for software in the identity domain. +And the reasoning behind a design is reconstructed from a thread whose conclusion is often implicit, if it survives at all. + +The project is governed under the Open Wallet Foundation, and external contributors build production systems on it. +Those contributors need the same access to design rationale that maintainers have. + +How should decisions that address an architecturally significant requirement (ASR) be recorded, so that the reasoning survives the thread it was formed in? + +## Decision Drivers + +* Records must be readable and writable by external contributors with no access to internal systems. +* The format must add little enough friction that it is actually used under delivery pressure. +* Records must be reviewable through the mechanism the project already uses for changes: pull requests. +* Decision history must be versioned alongside the code it describes. +* The scheme must survive the project outliving any individual maintainer. + +## Considered Options + +* Continue with GitHub Discussions only +* Michael Nygard's original ADR format +* MADR 4.0.0 +* Y-statements +* A wiki or Confluence space + +## Decision Outcome + +Chosen option: "MADR 4.0.0", because it is the only option that keeps records in the repository under normal pull request review, offers enough structure to make rejected options explicit, and remains lean enough to write in one sitting. + +### Consequences + +* Good, because decision history is versioned with the code and travels with any fork or clone of the repository. +* Good, because proposing a decision uses the same pull request mechanism external contributors already use, giving them a legitimate path to influence architecture rather than only implementation. +* Good, because the log is complete with respect to the code in this repository: a decision affecting code here is recorded here, so a reader of the repository is never missing rationale that exists elsewhere. +* Good, because the structure forces rejected options to be written down, which is the part reconstruction from a thread always loses. +* Neutral, because MADR is a template rather than a tool; no tooling dependency is added, and none is provided either. +* Bad, because writing a record is real work that competes with delivery, and the practice will decay unless the trigger conditions and review integration in `README.md` are enforced. +* Bad, because a partially adopted log is arguably worse than none. Readers may reasonably infer that an undocumented decision was never deliberately made. + +### Confirmation + +Adoption is enforced by three mechanisms rather than by intent: + +* A `CODEOWNERS` entry on `docs/adrs/` requiring architectural review on every record. +* A checkbox in the pull request template asking whether the change requires an ADR. +* An `adr-lint` workflow that fails on malformed records and on records missing from the index. + +Whether the log is actually being *read* is confirmed by a softer signal: architectural objections in code review citing ADR numbers. +If that has not started happening within two release cycles, this decision should be revisited rather than quietly ignored. + +## Pros and Cons of the Options + +### Continue with GitHub Discussions only + +* Good, because it costs nothing and is already in use. +* Good, because it captures the full debate, including the arguments that were abandoned. +* Neutral, because discussions are public and therefore already accessible to external contributors. +* Bad, because a discussion has no canonical conclusion. The outcome is implicit in the last few comments, or absent. +* Bad, because there is no status, so a superseded decision looks identical to a current one. +* Bad, because discussions are not versioned with the code and do not travel with a fork. + +### Michael Nygard's original ADR format + +* Good, because it is the most widely recognized ADR format and the origin of the term. +* Good, because it is extremely short: context, decision, status, consequences. +* Bad, because it has no dedicated section for options considered, which is the section that most often prevents a settled question from being reopened. +* Neutral, because MADR is a direct descendant and the two are close enough that migration in either direction is mechanical. + +### MADR 4.0.0 + +* Good, because only four sections are mandatory, so a minor record can be fifteen lines. +* Good, because "Pros and Cons of the Options" makes the comparison explicit, with Good, Neutral and Bad prefixes. +* Good, because the "Confirmation" subsection asks how compliance will be verified, which maps directly onto conformance suites for protocol decisions. +* Good, because it ships markdownlint configuration and templates in four levels of verbosity. +* Neutral, because the format has changed across major versions, with section names differing between 3.x and 4.x, so records copied from other projects may need adjusting. +* Bad, because the fuller template is verbose enough that contributors may skip it entirely rather than delete the optional sections; the minimal template variants mitigate this. + +### Y-statements + +* Good, because a decision compresses to a single structured sentence, making it very cheap to write. +* Bad, because that compression discards the option comparison and the consequences, which is most of the value for a long-lived project. +* Neutral, because a Y-statement works well as a summary line *inside* a MADR record. + +### A wiki or Confluence space + +* Good, because it is easy to write in and easy to reorganize. +* Bad, because pages are mutable by default, so there is no reliable history of what was decided when. +* Bad, because it is not reviewable through pull requests. +* Bad, because access for external contributors is an administrative problem that recurs with every new contributor, and is fundamentally at odds with an Open Wallet Foundation project. + +## More Information + +* Architectural decision terminology, including AD and ASR: +* MADR project: +* MADR 4.0.0 release: +* Nygard's original article: +* Trigger conditions, process, and repository integration: [README.md](README.md) + +This decision should be revisited if the log falls out of use, or if the project adopts documentation tooling that makes a different format cheaper to maintain. diff --git a/docs/adrs/README.md b/docs/adrs/README.md new file mode 100644 index 0000000000..a2d66e78b4 --- /dev/null +++ b/docs/adrs/README.md @@ -0,0 +1,162 @@ +# Architectural Decision Records + +This directory holds the architectural decision log for ThunderID. + +The format is [MADR 4.0.0](https://adr.github.io/madr/). +See [ADR-0000](0000-use-madr.md) for why. + +## Index + + + +| ADR | Title | Status | Date | +| --- | --- | --- | --- | +| [0000](0000-use-madr.md) | Use MADR for architectural decision records | accepted | 2026-09-01 | + +## Terminology + +These definitions come from [adr.github.io](https://adr.github.io/). + +* An **architectural decision (AD)** is a justified design choice that addresses a functional or non-functional requirement that is architecturally significant. +* An **architecturally significant requirement (ASR)** is a requirement that has a measurable effect on the architecture and quality of a system. +* An **architectural decision record (ADR)** captures a single AD and its rationale. +* The collection of ADRs maintained in a project constitutes its **decision log**, which is what this directory is. + +Two rules follow from those definitions, and they are the two people get wrong most often. +A record captures one AD, which is why [Granularity](#granularity) insists on one decision per record. +A record is warranted only when the requirement behind the decision is an ASR, which is what the next section tests for. + +## When an ADR is required + +An ADR is warranted when the requirement behind the change is architecturally significant. +That is hard to judge in the abstract, so the list below is the working test for it. +It follows [Nygard's original categories](https://cognitect.com/blog/2011/11/15/documenting-architecture-decisions), which are structure, non-functional characteristics, dependencies, interfaces, and construction techniques, narrowed to the ones that recur in this project. + +Write an ADR when a change does any of the following: + +* Alters a **wire-visible contract**: token or claim shape, endpoint semantics, error responses, or the choice of a spec profile. +* Introduces a **deliberate deviation** from a specification ThunderID claims to implement, including a decision not to implement an optional feature. +* Changes the **shape of the system**: a new or removed component, a responsibility that changes hands, or a pattern other components are expected to follow, such as an extension point, a flow model, or a concurrency or transaction convention. +* Trades off a **quality attribute** across the product: a security posture, a latency or throughput budget, an availability target, or what an operator has to do to run it. +* Changes a **storage strategy that spans components**: the split of data across stores, an encryption boundary, or a retention and revocation model. +* Adds, replaces, or removes a **runtime dependency** or an external system ThunderID talks to. +* Changes how the product is **built, packaged, or deployed** in a way that operators or downstream consumers have to follow. + +Two questions settle most of the cases the list does not. +Would reversing this decision later be expensive, and does it rule out an alternative someone might reasonably propose again? +A choice that is cheap to reverse, or that is confined to a single component and binds nobody outside it, is not an ADR however much debate it took. + +Everything else is a pull request description. +A bug fix or a refactor that preserves behavior does not need a record. + +If you are unsure, open the discussion first. +The need for an ADR usually becomes obvious once someone disagrees. + +### Granularity + +One decision per record. +A related cluster of decisions becomes a small set of ADRs that cross-reference each other, not one large one. + +A useful test is whether the choices could be reversed independently. +If they could, they are separate records that cite each other. +A choice that follows automatically from one already made, such as the column type used to store a structure whose storage split is already decided, belongs under that record's Consequences rather than in a record of its own. + +### Scope + +This log covers decisions affecting code in this repository. + +A decision that changes code here is recorded here, whatever motivated it. +Records state the constraint being satisfied, not who asked for it. +An ADR naming a specific organization, customer or downstream project is usually recording a circumstance rather than a reason, and circumstances change while the design they produced does not. + +Decisions confined to a downstream distribution's own tooling are that distribution's to record, and are out of scope here. +If a decision seems to need recording in two places at once, that is worth raising rather than resolving by splitting the record. + +This section is deliberately not an ADR. +It describes where records go rather than making an architectural choice, and it needs to stay editable as the project's distribution arrangements change. + +## Process + +1. **Discuss.** Design debate happens in a GitHub Discussion or an issue, as it does today. An ADR is the distillation of that debate, not a transcript of it. +2. **Propose.** Open a pull request adding `NNNN-title-with-dashes.md` with `status: "proposed"`, using the next free number. Link the discussion in the `## More Information` section. +3. **Review.** Reviewers argue in the PR. Amend the record until it reflects what was actually decided, including options that were rejected and why. +4. **Accept.** Change `status:` to `"accepted"`, set `date:`, add the row to the index table, and merge. **The merge is the decision.** + +A record that is rejected is still merged, with `status: "rejected"`. +Knowing that an option was considered and turned down is as useful as knowing what was chosen, and a rejected record stops the same proposal arriving again in six months. + +### Accepted records are immutable + +Do not edit the body of an accepted ADR. +Typos and broken links are fine to fix; reasoning and outcomes are not. + +To change a decision, write a new ADR and then edit exactly one line in the old one: + +```yaml +status: "superseded by ADR-0014" +``` + +The status field carries the identifier only, not a link. This is a MADR 4.0 convention. + +If in-place editing is allowed, the log stops being a decision history and becomes a design document that silently drifts away from what was actually agreed. + +### Numbering + +Numbers are allocated in the order records are **proposed**, not accepted, and are never reused. +Two open PRs claiming the same number is a trivial rebase; two merged ADRs sharing a number is not. + +The log is flat. +MADR supports categorizing records into subdirectories, but that makes numbers unique only within a category, which breaks cross-references and any number an external contributor has already cited. +Revisit this only if the log becomes genuinely hard to navigate. + +## Writing a record + +Copy [`adr-template.md`](adr-template.md) to `NNNN-title-with-dashes.md`. + +Four sections are mandatory: the title, Context and Problem Statement, Considered Options, and Decision Outcome. +A `status` field in the front matter is mandatory too, because the lint rejects a record without one. +Everything else in the template is optional, so delete the sections you do not need rather than filling them with filler. + +A few conventions that keep diffs readable and the linter quiet: + +* One sentence per line. Reviewers can then comment on a single sentence and rewording produces a one-line diff. +* Asterisks (`*`) as list markers. +* Titles state both the problem and the chosen solution: "Store refresh tokens hashed rather than encrypted", not "Refresh token storage". + +### Confirmation + +The `### Confirmation` subsection is optional in MADR but should be filled in for anything spec-related. +For a decision about protocol behavior, name the conformance profile or test suite that proves the implementation matches the record. +This is what stops an ADR from decaying into an aspiration. + +## Backfill + +The log starts mostly empty, and reconstructing every past decision is not worth the effort. +Backfill only where a newcomer currently has to read a mail thread or a GitHub Discussion to understand why the code looks the way it does. + +Backfilled records carry the original decision date and say plainly, in `## More Information`, that they were written after the fact. +Their "Considered Options" sections will be thinner than a live record's, and that is honest rather than a defect. + +## Repository integration + +[`.github/CODEOWNERS`](../../.github/CODEOWNERS) carries an entry for this directory, so architectural review is enforced by the platform rather than by memory: + +```text +/docs/adrs/ @darshanasbg @madurangasiriwardena +``` + +[`.github/pull_request_template.md`](../../.github/pull_request_template.md) carries two additions: a `### Related ADRs` section alongside the existing Related Issues and Related PRs sections, and a checklist item. + +The checklist item is phrased as *checked whether* an ADR is required rather than *wrote an ADR*, so a contributor whose change needs no record can still tick it truthfully. +A box that can only be ticked by some contributors is a box everyone learns to ignore. + +[`.github/workflows/adr-lint.yml`](../../.github/workflows/adr-lint.yml) runs on any change under this directory. +It checks formatting with markdownlint, using the configuration in [`.markdownlint.yml`](.markdownlint.yml), and then enforces the conventions on this page: the `NNNN-title-with-dashes.md` filename, a `status` holding one of the recognized values, an index row whose Status column matches the record's front matter, and unique record numbers. + +## Reviewing against the log + +The log only pays for itself if it is read. +The mechanism is review: when objecting to an architectural choice in a pull request, cite the ADR number that the change contradicts, or say that no record covers it and one is needed. + +An objection grounded in a merged record is a different conversation from an objection grounded in a reviewer's preference, and that difference matters most with contributors who are not in your timezone or your organization. diff --git a/docs/adrs/adr-template.md b/docs/adrs/adr-template.md new file mode 100644 index 0000000000..acd6f3b50a --- /dev/null +++ b/docs/adrs/adr-template.md @@ -0,0 +1,77 @@ +--- +# status is required. One of: +# proposed | rejected | accepted | deprecated | superseded by ADR-0123 +status: "proposed" +# +# Everything below is optional. Remove what you do not use. +# date: {YYYY-MM-DD when the decision was last updated} +# decision-makers: {GitHub handles of everyone involved in the decision} +# consulted: {handles of subject-matter experts consulted; two-way communication} +# informed: {handles of everyone kept up to date; one-way communication} +--- + +# {Short title, stating both the problem and the chosen solution} + +## Context and Problem Statement + +{Two or three sentences, or a short illustrative story. +State what forced the decision. Phrasing the problem as a question often works well. +Link the GitHub Discussion or issue where this was debated.} + + +## Decision Drivers + +* {driver 1: a force, a constraint, a concern} +* {driver 2} + +## Considered Options + +* {option 1} +* {option 2} +* {option 3} + +## Decision Outcome + +Chosen option: "{option 1}", because {justification: it is the only option meeting a +knock-out criterion, or it resolves a specific force, or it comes out best against the +drivers above}. + + +### Consequences + +* Good, because {positive consequence} +* Neutral, because {consequence that weighs neither way but is worth recording} +* Bad, because {negative consequence accepted as part of this decision} + + +### Confirmation + +{How will compliance with this decision be verified? +Name the conformance profile, test suite, integration test, or review gate that proves +the implementation matches this record. "Reviewed by the architecture group" is a weak +answer; an executable check is a strong one.} + + +## Pros and Cons of the Options + +### {option 1} + +{Optional description, example, or pointer to more information.} + +* Good, because {argument} +* Neutral, because {argument} +* Bad, because {argument} + +### {option 2} + +{Optional description, example, or pointer to more information.} + +* Good, because {argument} +* Bad, because {argument} + + +## More Information + +{Links to the originating discussion, related ADRs, relevant specifications and RFCs. +Note here if this record was written retroactively. +Record when the decision should be revisited, if that is knowable.} diff --git a/scripts/docs-lint.sh b/scripts/docs-lint.sh index 4fd01b1f33..25216a43aa 100755 --- a/scripts/docs-lint.sh +++ b/scripts/docs-lint.sh @@ -40,11 +40,13 @@ if [[ $# -eq 0 ]]; then else for f in "$@"; do [[ "$f" == /* ]] || f="$PWD/$f" - # Skip agent skill definitions and repo meta-docs (AGENTS.md, README.md, - # ARCHITECTURE.md) even if passed explicitly โ€” they're not docs/content pages - # and don't follow its conventions (frontmatter, , etc.). + # Skip agent skill definitions, architectural decision records, and repo + # meta-docs (AGENTS.md, README.md, ARCHITECTURE.md) even if passed + # explicitly โ€” they're not docs/content pages and don't follow its + # conventions (frontmatter, , etc.). case "$f" in "$REPO_ROOT"/.agent/skills/*|"$REPO_ROOT"/.claude/skills/*) continue ;; + "$REPO_ROOT"/docs/adrs/*) continue ;; */AGENTS.md|*/README.md|*/ARCHITECTURE.md) continue ;; esac FILES+=("$f")