From 19a474626c56e88e877dd661bc85050188c2977d Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 16:13:15 +0100 Subject: [PATCH 1/4] feat: migrate agent skills to the root skills/ folder and the skills CLI Purview.BuildSdk 1.0.6 removes skill packaging and the build-time .agents/ mirror, so the skills CLI cloning the git repository is the only delivery route. Skills are authored at skills//SKILL.md and nothing packs them. - rename every skill to a purview--* prefix; skills install into one flat directory per agent, so an unprefixed name collides with somebody else's and the loser is whichever installed second - validate them on every test run with Purview.Skills.Testing 1.0.6, through a Repository.UnitTests project whose [InheritsTests] attribute is load-bearing - raise the TUnit floor to 1.75.0, which Purview.Skills.Testing depends on and central transitive pinning would otherwise pin back down - rewrite the Agent Enablement and Agent Skills Distribution wiki pages, which documented the removed mirror as the design - bump to 0.1.0-prerelease.2 --- AGENTS.md | 32 +- Directory.Packages.props | 6 +- docs/wiki/Adopting-This-At-Your-Scale.md | 2 +- docs/wiki/Agent-Enablement.md | 120 ++++--- docs/wiki/Agent-Skills-Distribution.md | 293 ++++++++---------- docs/wiki/Engineering-Principles.md | 2 +- docs/wiki/Getting-Started.md | 5 +- docs/wiki/Repository-Shape.md | 3 +- global.json | 2 +- package.json | 2 +- .../purview-engineering-principles/SKILL.md | 171 ++++++++++ skills/purview-testing-conventions/SKILL.md | 255 +++++++++++++++ src/EngineeringPlaybook.slnx | 1 + .../Repository.UnitTests.csproj | 12 + src/tests/Repository.UnitTests/SkillTests.cs | 64 ++++ 15 files changed, 732 insertions(+), 238 deletions(-) create mode 100644 skills/purview-engineering-principles/SKILL.md create mode 100644 skills/purview-testing-conventions/SKILL.md create mode 100644 src/tests/Repository.UnitTests/Repository.UnitTests.csproj create mode 100644 src/tests/Repository.UnitTests/SkillTests.cs diff --git a/AGENTS.md b/AGENTS.md index 7639dd4..3393977 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,12 +67,34 @@ demonstrate it. ## Skills -Authored skills live in `skills//SKILL.md` at the repository root — not in `.agents/skills/`, which -is the gitignored mirror that packages write into. The root folder is the one the skills CLI can discover -in a git clone. A skill states when it applies in its `description`, keeps the workflow in `SKILL.md`, and -pushes detail into `references/`. See [Agent Skills Distribution](docs/wiki/Agent-Skills-Distribution.md). +Authored skills live in `skills//SKILL.md` at the repository root. `.agents/skills/` is the skills +CLI's install location for skills from **other** repositories and is gitignored; this repository's own +skills are never installed into it, because two copies drift apart. A skill states when it applies in its +`description`, keeps the workflow in `SKILL.md`, and pushes detail into `references/`. See +[Agent Skills Distribution](docs/wiki/Agent-Skills-Distribution.md). -This repository ships no package, so its skills travel by the git route only. +Nothing packs a skill into a NuGet package - `Purview.BuildSdk` 1.0.6 removed skill packaging and the +build-time `.agents/` mirror outright. The skills CLI clones the git repository, and that is the only route. + +| Skill | Answers | +| -- | -- | +| `skills/purview-engineering-principles/` | the ten repository conventions, what each prevents, and whether a build or a reviewer enforces it | +| `skills/purview-architecture/` | how many projects, which layers, where validation lives | +| `skills/purview-architecture-testing/` | turning a boundary into a failing build | +| `skills/purview-testing-conventions/` | TUnit, AAA, `TestBuilder`, substitutes, tiers | +| `skills/purview-persistence/` | storage approach, EF Core abstraction, GUID key strategy | +| `skills/purview-telemetry/` | logging, tracing, metrics, and when a project is "too small" for them | + +Every skill carries the `purview-` prefix: skills install into one flat directory per agent, shared with +every skill a developer has installed from anywhere, so an unprefixed name collides and the loser is +whichever installed second. + +`src/tests/Repository.UnitTests/SkillTests.cs` validates them on every test run through +`Purview.Skills.Testing`. The `[InheritsTests]` attribute there is load-bearing - TUnit does not walk base +types during discovery, so without it the run is green and nothing is checked. + +A relative link from one `SKILL.md` into another skill's folder is a rule violation: only a skill's own +folder is installed with it. Name the other skill in prose instead. ## Code conventions diff --git a/Directory.Packages.props b/Directory.Packages.props index 48dd3b7..1e84f24 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -26,7 +26,9 @@ 5.0.3 - 1.72.4 + + 1.75.0 @@ -60,5 +62,7 @@ + + diff --git a/docs/wiki/Adopting-This-At-Your-Scale.md b/docs/wiki/Adopting-This-At-Your-Scale.md index 9c52a84..b67ed97 100644 --- a/docs/wiki/Adopting-This-At-Your-Scale.md +++ b/docs/wiki/Adopting-This-At-Your-Scale.md @@ -20,7 +20,7 @@ leave the rest — with the knowledge that the rest is there when the repository | **Consumer verification** | No | No | Yes | | **Wiki** | `index`, `Home`, `_Sidebar`, one page | One page per public concern | One page per subsystem, plus a hub | | **`AGENTS.md`** | Yes | Yes | Yes | -| **Agent skills** | The SDK's mirrored set | Plus one per repeated review comment | Plus your own packaged skills | +| **Agent skills** | Install what you need with `npx skills add` | Plus one per repeated review comment | Plus your own, published from the repository that owns the convention | ## Small — one project diff --git a/docs/wiki/Agent-Enablement.md b/docs/wiki/Agent-Enablement.md index a30731e..1cda014 100644 --- a/docs/wiki/Agent-Enablement.md +++ b/docs/wiki/Agent-Enablement.md @@ -5,13 +5,14 @@ A repository tells an agent what it is, how it is laid out, and what its conventions are — in the repository, in files an agent reads without being pointed at them. -Three files do the work: +Four paths do the work: | Path | What it is | | -- | -- | | `AGENTS.md` | The repository-wide source of truth for agents. Purpose, authority, layout, conventions, local commands, completion checklist. | | `.github/copilot-instructions.md` | One job: defer to `AGENTS.md`. | -| `.agents/` | Agent content: skills, sub-agent definitions and prompts. Part-authored, part mirrored from the SDK. | +| `skills/` | The skills this repository authors and publishes. Tracked. | +| `.agents/skills/` | Skills installed from other repositories by the skills CLI. Gitignored; `skills-lock.json` is the tracked record. | The rule has a corollary that is easy to miss: **nothing important may be discoverable only by reading the code.** If the answer to "which of these two patterns is the intended one?" lives in the commit history or @@ -56,106 +57,97 @@ rule the humans have not written down, and a rule that only the wiki states is o ## The `.agents/` folder ```text -skills/ authored skills, tracked in git +skills/ skills this repository authors, tracked in git └── / ├── SKILL.md the skill itself └── references/ optional deeper material the skill links to .agents/ -├── .gitignore generated; lists the mirrored files -├── agents/ sub-agent definitions -├── prompts/ reusable prompts -└── skills/ mirrored from packages; gitignored +└── skills/ skills installed from other repositories, by the skills CLI; gitignored └── / + +skills-lock.json the committed record of what the CLI installed ``` -Note the split. A skill you **author** lives at the repository root in `skills/`, where it is tracked -and where a cloning installer can find it. A skill you **receive** from a package arrives in -`.agents/skills/`, where it is gitignored. The same folder cannot do both jobs, and -[Agent Skills Distribution](Agent-Skills-Distribution.md) explains why. +Note the split, and that it is a **single-writer** split. A skill you **author** lives at the +repository root in `skills/`, where it is tracked and where a cloning installer can find it. A skill +you **receive** from somewhere else arrives in `.agents/skills/`, written by the CLI and gitignored. + +The same folder cannot do both jobs. `.agents/skills/` is the CLI's universal install location, so a +repository that also mirrored its own skills there would have two writers on one set of paths — and +the build would silently overwrite a pinned install. -A **skill** is a self-contained instruction set for one kind of task. It opens with YAML frontmatter whose -`description` is the trigger — it is what tells an agent that this skill applies: +Link to a skill by its **authored** path, `skills//SKILL.md`. A link into `.agents/skills/` +points at a gitignored file and is broken in a fresh clone. + +A **skill** is a self-contained instruction set for one kind of task. It opens with YAML frontmatter +whose `description` is the trigger — it is what tells an agent that this skill applies: ```markdown --- -name: sdk-engineering-principles -description: "Use when creating or rationalising a repository that uses Purview.BuildSdk and you need the - policy-level conventions for project placement, naming, namespace identity, test categories, and - large-suite readability." +name: purview-persistence +description: >- + Use when a .NET project actually persists data and you are choosing a storage approach, deciding + whether to abstract EF Core, or selecting a GUID primary-key strategy. --- ``` -Writing a good trigger is most of writing a good skill. "Use when X, and you need Y" is far more useful -than a topic label, because the agent is matching a situation, not searching an index. +Writing a good trigger is most of writing a good skill. "Use when X, and you need Y" is far more +useful than a topic label, because the agent is matching a situation, not searching an index. -A **prompt** is a reusable instruction for a recurring task; an **agent** is a definition of a specialised -sub-agent with its own remit. Both are plain Markdown, and both are read the same way. +Prefer a skill to a prompt, and a skill to a shipped sub-agent definition. A prompt or a mirrored +agent file is inert: it lands somewhere no agent reads natively, so somebody has to already know it +exists. A skill carries a `description` an agent matches against the task, so it fires when it is +relevant. Anything task-shaped and reusable belongs in `skills/`. -## The mirrored half +And guidance about working *in this repository* belongs in `AGENTS.md`, not in a skill — a skill +ships to somebody else. -The SDK ships its own agent content and mirrors it into `.agents/` on build: skills for the SDK's own -conventions, prompts for diagnosing the agent folder itself, and sub-agent definitions for repository -rationalisation. You get the SDK's guidance without writing it. +## Getting somebody else's guidance -The mirror is change-aware — unchanged files are never rewritten — and it writes a generated `.gitignore` -that lists exactly the files it owns: +Install it, explicitly: -```text -# Generated by Purview.BuildSdk - do not edit. -# -# These files are mirrored into this folder from NuGet packages on build. Ignoring them -# keeps an SDK or package upgrade from showing up as local modifications. Files you -# author in this folder are not listed here and are tracked as normal. -/skills/sdk-engineering-principles/SKILL.md -... +```bash +npx skills add purview-dev/build-sdk +npx skills add purview-dev/engineering-playbook#v2.1.0 # pinned ``` -That generated file is the whole design, and it is worth copying. It means: +The CLI writes into each agent's own directories and records the source and ref in +`skills-lock.json`, which is committed. Everything it writes is gitignored, because it is all +reproducible from the lockfile with `npx skills experimental_install`. -- **An SDK upgrade is not a diff.** The mirrored files are ignored, so the working tree stays clean. -- **Your own content is tracked normally.** Anything you author is absent from the list and committed. -- **The boundary is explicit.** A file's presence in the list is a statement that the SDK owns it and an - edit will be overwritten. +> An earlier arrangement had the SDK mirror a package's agent content into `.agents/` on build. It +> was removed in `Purview.BuildSdk` 1.0.6: it duplicated the CLI, wrote to the same paths, and made +> an SDK upgrade capable of replacing a pinned install. +> [Agent Skills Distribution](Agent-Skills-Distribution.md) has the reasoning. -The SDK also keeps its state in `.purview/`, whose `.gitignore` is a single `*` — so the sync cache never -appears in a status output. +The SDK still keeps its own state in `.purview/`, whose `.gitignore` is a single `*` — so a cache +never appears in a status output. ## How it is enforced | Rule | Enforced by | | -- | -- | -| The SDK's own guidance is present and current | The build (change-aware mirror into `.agents/`) | -| Mirrored content is not mistaken for local content | The build (generated `.agents/.gitignore`) | +| An authored skill is well formed, correctly named, and its links resolve | `ValidatePurviewSkills` at pack time, and `PurviewSkillTests` on every test run | +| An empty `skills/` folder is reported rather than passing vacuously | `PurviewSkillTests` discovery rule | +| A build installs nothing into an agent directory | Test asserts no lockfile and no agent directory after a full build | +| Installed skills are reproducible | `skills-lock.json`, committed; everything the CLI writes is gitignored | | The SDK's cache never appears as a change | `.purview/.gitignore` | | `AGENTS.md` exists, is accurate, and is specific | Review | | `copilot-instructions.md` defers to `AGENTS.md` | Review | ## At your scale -- **Any repository.** Write `AGENTS.md`. It is the cheapest thing on this page and the one with the largest - effect on an agent's first ten minutes. -- **A repository with conventions worth repeating.** Add a skill. The test is whether you have written the - same explanation twice in review comments. -- **A repository with several task shapes.** Add a prompt per shape, and consider a sub-agent definition for - the one that needs a different remit rather than more instructions. -- **A repository that others copy from.** Ship your skills in a package so the guidance travels with the - code, in the same way the SDK does — the mirror mechanism is the pattern, not the SDK. See +- **Any repository.** Write `AGENTS.md`. It is the cheapest thing on this page and the one with the + largest effect on an agent's first ten minutes. +- **A repository with conventions worth repeating.** Add a skill. The test is whether you have + written the same explanation twice in review comments. +- **A repository others depend on.** Publish your skills from it, with a vendor-and-product name + prefix, and validate them in the test suite so a broken one fails where it was written. See [Agent Skills Distribution](Agent-Skills-Distribution.md). - -## The gap the mirror does not close - -`.agents/` is a convention this playbook and the SDK share. It is **not** a directory any agent reads -natively — Claude Code, Codex, Copilot and the rest each look somewhere else. Mirroring a skill into -`.agents/` therefore makes it present, not active. - -Closing that gap is a separate, explicit step, delegated to a tool that tracks every agent's discovery -paths so the SDK does not have to. That is the subject of -[Agent Skills Distribution](Agent-Skills-Distribution.md). - ## See also - [Agent Skills Distribution](Agent-Skills-Distribution.md) - [Documentation Convention](Documentation-Convention.md) - [Repository Shape](Repository-Shape.md) -- [build-sdk: Agent Folder](https://github.com/purview-dev/build-sdk/blob/main/docs/wiki/Agent-Folder.md) +- [Agent Skills specification](https://agentskills.io/specification) diff --git a/docs/wiki/Agent-Skills-Distribution.md b/docs/wiki/Agent-Skills-Distribution.md index fdb36c3..d10c39e 100644 --- a/docs/wiki/Agent-Skills-Distribution.md +++ b/docs/wiki/Agent-Skills-Distribution.md @@ -3,41 +3,48 @@ ## The rule A skill is published **once**, at the repository root in `skills//SKILL.md`, and travels to its -consumers by two routes that share that single copy: +consumers by **one** route: the skills CLI clones the git repository and installs into each agent's +own directories. -| Route | Who it serves | Mechanism | -| -- | -- | -- | -| NuGet package → `.agents/skills/` | A .NET repository that references the package | Mirrored on build by `Purview.BuildSdk` | -| Git repository → agent directories | Any agent, any language, via `npx skills` | Installed on request by the skills CLI | - -**Installation into an agent's own directories is always explicit.** It is never a side effect of -`dotnet restore`, `dotnet build`, a design-time build in an IDE, or a test run. +**Installation is always explicit.** It is never a side effect of `dotnet restore`, `dotnet build`, a +design-time build in an IDE, or a test run. -Version compatibility is resolved from metadata the package already carries — the skills it ships and -the commit it was built from — so there is no hand-maintained package-to-skill manifest. +Nothing packs a skill into a NuGet package. `Purview.BuildSdk` 1.0.6 removed both the packing and the +build-time `.agents/` mirror that delivered it — see [Why one route](#why-one-route). ## Why -A skill is guidance that goes stale against the code it describes. Three failures follow from that, -and the design answers each one. +A skill is guidance that goes stale against the code it describes. Three failures follow, and the +design answers each one. **Guidance that does not travel with the code.** If a library's conventions live only in its own repository's wiki, every consumer re-derives them, and an agent in a consuming repository has no -access to them at all. Shipping the skill inside the package means a `PackageReference` is enough. +access to them at all. Publishing the skill from the library's repository means one `npx skills add` +is enough. **Guidance that does not match the version in use.** A project pinned to an older release that -receives the newest skill is told about APIs it does not have. The nuspec records the commit the -package was built from, so the skill resolved is the skill *that version* shipped. +receives the newest skill is told about APIs it does not have. The CLI installs from a **ref**, and +records it, so a repository can pin a skill to the commit or tag that matches the version it uses. **An agent directory modified by a build.** A build that writes into `.claude/`, `.codex/` or a -peer's equivalent is a build with an invisible side effect: it mutates a developer's tooling, races -between parallel projects, and makes a restore non-reproducible. Resolution is cheap and automatic; -installation is a decision a person makes. +peer's equivalent has an invisible side effect: it mutates a developer's tooling, races between +parallel projects, and makes a restore non-reproducible. Installation is a decision a person makes. + +## Why one route -There is a fourth, subtler reason for the root `skills/` folder. The mirrored `.agents/` folder is -gitignored in every consuming repository — that is what stops a package upgrade appearing as a local -modification. A gitignored folder is absent from a git clone, so it cannot also be the source a -cloning installer reads. The authored copy has to live somewhere tracked. +There used to be two: the CLI, and a NuGet package whose `.agents/skills/` content the SDK mirrored +into the consuming repository on build. + +The second was removed because it was **a second writer on the first one's paths**. The CLI's +universal install location *is* `.agents/skills/`. So a build could silently overwrite a pinned +install, and the SDK's own drift check would then compare a lockfile ref against content the mirror +had already replaced. Duplication was not neutral here; it was a correctness problem. + +It also never closed the gap it appeared to close. `.agents/` is a convention, not a directory any +agent reads natively — Claude Code, Codex, Copilot and the rest each look somewhere else. Mirroring a +skill into `.agents/` made it *present*, not *active*. The CLI knows where each agent actually looks, +and that knowledge changes on its own schedule, so it belongs in a tool that tracks it rather than in +a build. ## Skill format @@ -62,12 +69,33 @@ description: >- --- ``` +The specification defines six fields and no more: `name`, `description`, `license`, `compatibility`, +`metadata` and `allowed-tools`. Anything else belongs under `metadata`. `name` is 1–64 characters of +lowercase `a-z0-9` and single hyphens, and **must match the folder name** — the CLI installs by the +frontmatter name while a human reads the folder, so a mismatch installs under a name nobody expects. + Use progressive disclosure. The frontmatter says *when* the skill applies; `SKILL.md` carries the -workflow; `references/` holds the detail that only some tasks need. A skill that inlines everything -costs context on every activation, whether or not the detail was relevant. +workflow; `references/` holds the detail only some tasks need. A skill that inlines everything costs +context on every activation, whether or not the detail was relevant — keep `SKILL.md` under 500 +lines. -Link to the authoritative documentation rather than restating it. A skill that paraphrases a library's -own wiki is a second source of truth, and it is the one that will drift. +Link to the authoritative documentation rather than restating it. A skill that paraphrases a +library's own wiki is a second source of truth, and it is the one that will drift. + +**Never link from one skill into another skill's folder.** Only a skill's own folder is installed +with it, so a relative path that escapes it works in the authoring repository and nowhere else. Name +the other skill in prose. + +## Naming + +Skills install into **one flat directory per agent**, shared with every skill a developer has +installed from anywhere. `sdk-configuration`, `project-placement`, `dotnet-tunit` and +`git-conventional-commits` are all names somebody else will also pick, and the loser of a collision +is whichever installed second. + +So every published skill carries a vendor-and-product prefix: `purview-value-objects-efcore`, +`purview-sourcegenerator-codewriter`, `purview-telemetry-diagnostics`. The prefix is not decoration; +it is the only thing keeping two unrelated skills from being the same file. ## Publication and distribution @@ -76,60 +104,35 @@ own wiki is a second source of truth, and it is the one that will drift. ```text flowchart TD A["skills/<name>/SKILL.md
the single authored copy"] - A --> G["git: tracked at the repository root"] - A --> P["IncludePurviewSkillsInPackage"] - - P --> N[".nupkg
.agents/skills/<name>/
+ generated .gitignore
+ nuspec repository url and commit"] - N --> F["NuGet feed"] - G --> C["skills CLI
clone at ref"] - F --> M["Purview.BuildSdk mirror
reads the restore graph"] - C --> AD["Agent directories
.claude/, .codex/, ..."] - M --> DA[".agents/skills/
gitignored mirror"] + C --> L["skills-lock.json
source + ref, committed"] ``` -The nuspec's `` element is written by SourceLink for every package built by the SDK. It -is what lets a consumer get from a restored package back to the exact source commit. - -## Package-to-skill discovery +The CLI discovers skills **only** in a root `skills/` folder. It does not look in nested or dotted +folders, even with `--full-depth`. -No custom metadata file is introduced, because the package already carries both halves of the mapping. - -**Diagram K — package-to-skill discovery and Build SDK resolution.** - -```text -flowchart TD - A["project.assets.json
every restored package: id, version, path"] --> B{"package has
.agents/skills/ ?"} - B -- no --> X["Not a skill source"] - B -- yes --> C["Skill names = folders containing a SKILL.md"] - C --> D["Read the package nuspec"] - D --> E{"repository url is GitHub
and commit present?"} - E -- yes --> F["PurviewSkillSource
owner/repo#commit + skill names"] - E -- no --> G["PurviewUnresolvableSkillSource
PRSKL0002, nothing installed"] -``` - -A folder under `.agents/skills/` counts as a skill only when it contains a `SKILL.md`; a `references/` -or `scripts/` folder beside one is part of that skill, not another. +`skills-lock.json` is the committed record of what a repository installed, so the same set can be +restored later with `npx skills experimental_install`. Everything the CLI writes under +`.agents/skills/` and the per-agent directories is gitignored — it is all reproducible from the +lockfile. ## Responsibilities The separation matters more than any single piece of it. -**Diagram K (detail) — installer and Build SDK responsibilities.** - ```text flowchart LR - subgraph Package["Skill metadata - the package itself"] - M1[".agents/skills/*"] - M2["nuspec repository element"] + subgraph Repo["The authoring repository"] + S["skills/<name>/SKILL.md"] + V["ValidatePurviewSkills (pack-time)
PurviewSkillTests (test-time)"] end subgraph SDK["Purview.BuildSdk"] - R["Skill resolver
ResolvePurviewSkillSources
decides nothing about agents"] T["Explicit targets
ListPurviewSkills
InstallPurviewSkills"] D["CLI detection
DetectPurviewSkillsCli"] + R["Repository-root resolution
FindPurviewRepositoryRoot"] end subgraph CLI["Skill installer - npx skills"] @@ -138,159 +141,127 @@ flowchart LR I3["skills-lock.json"] end - M1 --> R - M2 --> R + S --> V R --> T D --> T T -->|"invokes, never installs itself"| CLI ``` -The SDK does not maintain a copy of each skill per supported agent. Agent discovery paths differ, are -numerous, and change on their own schedule; the CLI tracks them, so the SDK does not have to. +The SDK keeps no copy of a skill per supported agent. Agent discovery paths differ, are numerous, and +change on their own schedule; the CLI tracks them, so the SDK does not have to. + +Two invariants govern how the SDK launches it: + +- **Any package runner, defaulting to `npx`.** `PurviewSkillsRunner` selects one (`npx`, `bunx`, + `bun x`, `pnpm dlx`, `yarn dlx`, `deno run -A npm:`); an unknown runner falls through to + ` skills`. Detection probes `PATH` for `skills` first, then the configured runner, then the + candidates — honouring `PATHEXT` on Windows. `PATH` is read directly rather than by spawning a + version check: deterministic, no process, and it cannot hang on a cold download. +- **Always the repository root.** `skills-lock.json` and the agent directories belong to the + repository, not to a project, so every skill target runs from one resolved root. There is + deliberately no project-directory fallback — that would scatter a lockfile and an agent directory + through the tree, one per project. ## How it is enforced | Rule | Enforced by | | -- | -- | -| A package's skills are discoverable from the restore graph | `ResolvePurviewSkillSources`, tested in `PurviewSkillResolutionTests` | -| The resolved skill matches the restored package version | The nuspec commit is the ref; tested | -| A package that cannot be pinned is reported, not guessed | `PRSKL0002` warning; tested | -| Resolution without a restore gives an actionable error | `PRSKL0001` error | +| A skill's frontmatter is valid, its name matches its folder and is unique | `ValidatePurviewSkills` before `Pack` (`PRSKL0005`) | +| The same rules hold on every test run | `PurviewSkillTests` from `Purview.Skills.Testing`, inherited by one derived class per repository | +| Every `references/...` link in a skill resolves, and resolves *inside* the skill | Both of the above | +| `SKILL.md` stays within the recommended size | `PurviewSkillTests` | +| An empty `skills/` folder is reported rather than passing vacuously | `PurviewSkillTests` discovery rule | | An ordinary restore-and-build installs nothing | Test asserts no lockfile and no agent directory after a full build | -| Repeated installation is idempotent | The skills CLI skips a skill already installed from the same source and ref | -| Root `skills/` reaches the package's `.agents/skills/` | `AgentPackFolderTests`, asserted against the built `.nupkg` | -| A packaged skill does not show as a local modification | Generated per-skill `.gitignore`, asserted in the same test | -| A skill's frontmatter is valid, its name matches its folder and is unique | `ValidatePurviewSkills`, which `IncludePurviewSkillsInPackage` depends on — so a malformed skill fails the pack (`PRSKL0005`) | -| Every `references/...` link in a skill resolves | The same target | +| Repeated installation is idempotent | The CLI skips a skill already installed from the same source and ref | | The guidance in a skill is good, and an agent follows it | **Review** | The last row is a review obligation and stays one. A build can check that a skill is well formed; it cannot check that prose is correct or that an agent obeyed it. See [Agent Guidance Skills](Agent-Guidance-Skills.md) for the scenarios used to review that by hand. -## Version resolution - -**Diagram K (detail) — version resolution and compatibility diagnostics.** - -```text -flowchart TD - A["Referenced package version
from project.assets.json"] --> B{"nuspec repository
commit present?"} - B -- no --> W["PRSKL0002 warning
nothing installed
the package's own mirror still delivers it"] - B -- yes --> C["owner/repo#commit
pinned and reproducible"] - C --> D["skills CLI records source + ref
in skills-lock.json"] - D --> E["npx skills experimental_install
restores the same set"] -``` - -Pinning to a commit rather than a tag is deliberate: a tag can be moved, and a package can be built -from a commit that was never tagged — a pre-release, for instance. The commit is the only identifier -guaranteed to correspond to the restored package. - -Where exact compatibility cannot be determined, the SDK warns and installs nothing. Installing the -default branch's skill against an older package is the failure this is designed to avoid. +**`[InheritsTests]` is load-bearing** on the derived test class. TUnit does not walk base types during +discovery, so without it the class compiles, the run is green, and nothing is validated — the worst +failure this arrangement could have. ## Using it ```bash -dotnet restore # resolution reads the restore graph -dotnet msbuild -t:ListPurviewSkills # diagnostics; installs nothing -dotnet msbuild -t:InstallPurviewSkills # explicit install, via npx skills +npx skills add purview-dev/engineering-playbook # install +npx skills add purview-dev/engineering-playbook#v2.1.0 # pinned +npx skills list # what is installed +npx skills update # newest published +npx skills remove +npx skills experimental_install # restore from skills-lock.json ``` -| Property | Default | Purpose | -| -- | -- | -- | -| `PurviewSkillsSourceFolder` | `/skills` | Where authored skills live | -| `PurviewPackRepositorySkills` | `false` | Opts a project into packing them | -| `PurviewSkillsToPack` | *(empty — all)* | The subset of skills *this* project ships | -| `PurviewSkillsAgents` | `*` | Which agents to install to | -| `PurviewSkillsCommand` | *(detected)* | Overrides the CLI launcher | -| `PurviewSkillsSource` | *(empty)* | Install from an explicit git source instead of from packages | - -### Detecting the installer - -The installer is Node tooling, and a machine that can build .NET need not have it. The SDK probes -`PATH` for `skills` first (a global or local install — no download, works offline), then `npx`, -honouring `PATHEXT` on Windows where both are `.cmd` shims. `PATH` is read directly rather than by -spawning a version check: deterministic, no process, and it cannot hang on a cold `npx` download. - -When `npx` is the fallback it is pinned to a major (`skills@1`), so a future breaking release of the -CLI cannot silently change what the target does. - -Neither found is an actionable error naming the fix, not a raw `'npx' is not recognized`. And -`ListPurviewSkills` only *reports* availability — resolving and reporting needs no Node tooling, so -the diagnostic path always works. - -You can also install from a named repository, bypassing package resolution entirely: +Through the SDK, so a repository exercises the path it ships to consumers: ```bash +dotnet msbuild -t:ListPurviewSkills # diagnostics; installs nothing dotnet msbuild -t:InstallPurviewSkills -p:PurviewSkillsSource=purview-dev/engineering-playbook ``` -That is the escape hatch for a repository that wants a Purview skill it has no `PackageReference` -for. The caller owns the ref — append `#` to pin it — and no restore is required. +| Property | Default | Purpose | +| -- | -- | -- | +| `PurviewSkillsSource` | *(empty)* | Install from an explicit git source; append `#` to pin | +| `PurviewSkillsAgents` | `*` | Which agents to install to | +| `PurviewSkillsRunner` | `npx` | Which package runner launches the CLI | +| `PurviewSkillsCommand` | *(detected)* | Overrides the launcher entirely, skipping detection | +| `PurviewSkillsInstallRoot` | *(resolved)* | Forces the repository root | -### One repository, several packages +### One repository, several skills -A repository usually produces more than one package, and a skill belongs to the one whose library it -describes — `Purview.Results.AspNetCore`'s HTTP-mapping skill has no business in `Purview.Results`. -`PurviewSkillsToPack` names that subset per project: +A repository publishes the whole of its `skills/` folder — there is no per-project subset any more, +because there is no package to subset. Scope is expressed in the **names**: ```text -skills/ ← one authored copy of each -├── purview-results-core ─┐ -├── purview-results-union-errors ─┴─► Purview.Results -├── purview-results-http-mapping ───► Purview.Results.AspNetCore -├── purview-results-zodsharp-validation ───► Purview.Results.ZodSharp -└── purview-results-zodsharp-problems ───► Purview.Results.ZodSharp.AspNetCore +skills/ +├── purview-results-core +├── purview-results-union-errors +├── purview-results-http-mapping +├── purview-results-zodsharp-validation +└── purview-results-zodsharp-problems ``` -A skill left out of every `PurviewSkillsToPack` is still installable from the git source and ships in no -package — which is how a repository keeps skills about *working in that repository* (test runners, commit -conventions) separate from the guidance it publishes to consumers. - -A name in `PurviewSkillsToPack` with no matching folder is `PRSKL0003`, so a renamed or deleted skill -fails the pack instead of silently vanishing from the package. - -Updating and removing are the CLI's, not the SDK's: `npx skills update`, `npx skills remove`, -`npx skills list`. +Guidance about working *in this repository* — its test runner, its commit conventions — does not +belong in `skills/` at all. A skill here ships to somebody else; put repository-local guidance in +`AGENTS.md`. ### Troubleshooting | Symptom | Cause | | -- | -- | -| `PRSKL0001` | No `project.assets.json`. Run `dotnet restore`; resolution never restores for you. | -| `PRSKL0002` | The package's nuspec has no GitHub repository URL or no commit — usually SourceLink disabled at pack time. | -| `PRSKL0003` | `PurviewSkillsToPack` names a skill folder that does not exist — usually a rename. | +| `PRSKL0004` | Neither `skills` nor a package runner is on `PATH`. Install Node.js, or `npm install -g skills`. `ListPurviewSkills` still works without it. | | `PRSKL0005` | A skill is malformed: no `SKILL.md`, no frontmatter, no `name`/`description`, a name that does not match its folder, or a broken `references/` link. | -| `PRSKL0004` | Neither `skills` nor `npx` is on `PATH`. Install Node.js, or `npm install -g skills`. `ListPurviewSkills` still works. | -| "No skills found" from the CLI | The source repository has no root `skills/` folder. The CLI does not discover skills in nested or dotted folders, even with `--full-depth`. | -| Skills appear in `.agents/` but no agent sees them | `.agents/` is the mirror, not an agent directory. Run the install target. | -| `npx` not available | The install target needs Node (`PRSKL0004` names the fix). Resolution and `ListPurviewSkills` do not. | +| `PRSKL0003` | The repository root could not be resolved for an install. Pass `PurviewSkillsInstallRoot`. | +| "No skills found" from the CLI | The source repository has no **root** `skills/` folder. Nested and dotted folders are not discovered. | +| A skill installed but the agent ignores it | Check the `description`. It is the trigger, and a topic label does not match a situation. | +| `PRSKL0010`–`PRSKL0014` | Recommendation diagnostics raised against packages built by an older SDK that still carried `.agents/skills/` metadata. | ## Publishing skills for a new library 1. Author the skill at the repository root: `skills//SKILL.md`. -2. Set `PurviewPackRepositorySkills=true` on the project that ships it, and name it in that project's - `PurviewSkillsToPack` when the repository produces more than one package. -3. Leave SourceLink enabled, so the nuspec records the repository and commit. +2. Give it a vendor-and-product prefix. +3. Add `[InheritsTests] public sealed class SkillTests : PurviewSkillTests;` to a test project + referencing `Purview.Skills.Testing`, so the rule set runs on every build. 4. Verify with `npx skills add / -l` — if it reports no skills, the folder is in the wrong place. -5. Verify the package with `dotnet pack` and inspect `.agents/skills/` in the `.nupkg` — that is what - existing consumers receive, and it should not change for skills you did not touch. ## At your scale - **One repository, one agent.** You do not need any of this. Put `SKILL.md` files in `skills/` and run `npx skills add .` when they change. The format is the part worth adopting first. -- **Several repositories sharing conventions.** Publish skills from the repository that owns the - convention and install them with the CLI. Still no SDK required. -- **A library with consumers.** Ship skills in the package. The guidance arrives with the dependency, - and it arrives at the version the consumer is actually on. -- **A platform with many packages and agents.** The full split above: metadata in the package, - resolution in the build, installation in a tool that owns agent paths. The thing to copy is the - separation, not the implementation. +- **Several repositories sharing conventions.** Publish from the repository that owns the convention + and install with the CLI. Pin the ref; commit `skills-lock.json`. +- **A library with consumers.** Same mechanism, plus a named prefix and the conformance test, so a + broken skill fails in the repository that owns it rather than in somebody else's agent. +- **A platform with many repositories.** The thing to copy is the **separation**: authoring and + validation in the owning repository, installation in a tool that owns agent paths, and exactly one + writer per path. ## See also - [Agent Enablement](Agent-Enablement.md) — `AGENTS.md`, `.agents/`, and what makes a good skill - [Documentation Convention](Documentation-Convention.md) - [skills CLI](https://github.com/vercel-labs/skills) +- [Agent Skills specification](https://agentskills.io/specification) diff --git a/docs/wiki/Engineering-Principles.md b/docs/wiki/Engineering-Principles.md index b85e438..b20e975 100644 --- a/docs/wiki/Engineering-Principles.md +++ b/docs/wiki/Engineering-Principles.md @@ -137,7 +137,7 @@ the same guidance the build uses. Nothing important is discoverable only by read memory, no intuition about which of two plausible patterns is the intended one. The guidance that makes the repository obvious to an agent is the same guidance that makes it obvious to a new joiner. -**How it is enforced.** Review, plus the SDK mirroring its own skills into `.agents/` so the agent-facing +**How it is enforced.** Review, plus the SDK publishing its own skills so the agent-facing guidance tracks the SDK version rather than the repository's memory of it. See [Agent Enablement](Agent-Enablement.md). diff --git a/docs/wiki/Getting-Started.md b/docs/wiki/Getting-Started.md index 5b20778..2a8636f 100644 --- a/docs/wiki/Getting-Started.md +++ b/docs/wiki/Getting-Started.md @@ -349,8 +349,9 @@ Then `docs/wiki/index.md`, `docs/wiki/Home.md` and `docs/wiki/_Sidebar.md`, and ## Step 11 — Make the repository legible to an agent Add `AGENTS.md` at the root: purpose, layout table, conventions, completion checklist, local commands. -The SDK also mirrors a set of SDK skills into `.agents/` on build, so an agent gets the SDK's own -guidance without you writing it. See [Agent Enablement](Agent-Enablement.md). +Then install the SDK's own skills with `npx skills add purview-dev/build-sdk`, so an agent gets that +guidance without you writing it. Installation is explicit - no build writes into an agent directory. +See [Agent Enablement](Agent-Enablement.md). ## Step 12 — Add the local tools and the commands that run them diff --git a/docs/wiki/Repository-Shape.md b/docs/wiki/Repository-Shape.md index f9d1be9..cd1ba13 100644 --- a/docs/wiki/Repository-Shape.md +++ b/docs/wiki/Repository-Shape.md @@ -12,7 +12,8 @@ Repository/ ├── .github/ │ ├── workflows/ pr.yml, release.yml, any repository-specific workflow │ └── copilot-instructions.md defers to AGENTS.md -├── .agents/ SDK-mirrored agent content (see Agent Enablement) +├── skills/ skills this repository publishes (see Agent Enablement) +├── .agents/skills/ skills installed from elsewhere by the skills CLI; gitignored ├── .purview/ SDK state; gitignored ├── docs/ │ └── wiki/ the wiki; docs_dir in mkdocs.yml diff --git a/global.json b/global.json index ff3df1e..3d7a1e6 100644 --- a/global.json +++ b/global.json @@ -3,7 +3,7 @@ "allowPrerelease": false }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.5" + "Purview.BuildSdk": "1.0.6" }, "test": { "runner": "Microsoft.Testing.Platform" diff --git a/package.json b/package.json index 81d0a1b..c7d0a86 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "engineering-playbook", - "version": "0.1.0-prerelease.1", + "version": "0.1.0-prerelease.2", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/skills/purview-engineering-principles/SKILL.md b/skills/purview-engineering-principles/SKILL.md new file mode 100644 index 0000000..ba4dd9b --- /dev/null +++ b/skills/purview-engineering-principles/SKILL.md @@ -0,0 +1,171 @@ +--- +name: purview-engineering-principles +description: >- + Use when setting up, reviewing or rationalising a .NET repository against the Purview engineering + conventions - convention over configuration, derived identity, architecture as tests, warnings as + errors, proving consumer-facing claims, documentation ownership, test data builders, agent + legibility, and tiering verification by risk. Each rule names the failure it prevents and whether a + build or a reviewer enforces it. +metadata: + product: Purview engineering playbook + repository: purview-dev/engineering-playbook +--- + +# Purview engineering principles + +Ten rules, each with the failure it prevents and the mechanism that makes it true. The distinction +that matters throughout: **a convention a build can check is checked by the build; a convention only +a reviewer can check is labelled a review obligation and never described as enforced.** A rule that +claims enforcement it does not have is worse than no rule, because people stop looking. + +## 1. Convention over configuration + +A repository declares as little as possible. One MSBuild SDK supplies the defaults, and a project +file is short because the SDK already knows what a project of that shape needs. + +Configuration copied between repositories diverges. When the rule lives in one package, fixing it +once fixes it everywhere; when it lives in eleven project files, it is fixed eleven times — or, more +often, once. + +*Enforced by:* `` in a single +`Directory.Build.props`. The SDK detects project type from the filename and the `Sdk` attribute. + +*At your scale:* a single-project repository still benefits. The import is two lines and removes the +target framework, language version, nullable context, analyzer set and package metadata. + +## 2. Identity is derived, never repeated + +`RootNamespace` comes from `$(NamespacePrefix).$(ProjectName)`, and `AssemblyName` follows the +resolved `RootNamespace`. A name appears once — in the folder and file name — and everything else is +computed. + +A hand-typed namespace drifts from the assembly it lives in, and the drift is invisible until +something reflects over types: an architecture test, a serializer, a DI scan. + +*Enforced by:* the SDK during `.props` evaluation, plus a build-time check +(`PurviewProjectFileNameMismatch`) when a `.csproj` filename does not match its parent directory. + +*At your scale:* always. It costs nothing and removes a class of bug. + +## 3. Architecture is a test, not a diagram + +The dependencies a layer may have are asserted by tests that read the compiled assemblies. A +`ProjectReference` pointing the wrong way fails the build. + +A diagram is a claim about the past. Dependency direction erodes one convenient reference at a time, +and every such reference looks reasonable in isolation. + +*Enforced by:* a test project that references every layer and inspects their metadata references. + +*At your scale:* small repositories can skip it. Adopt it the moment a **second layer** exists — +that is when the wrong direction first becomes possible. + +## 4. The build is the only authority + +Style is enforced by analyzers and `EnforceCodeStyleInBuild`, and the SDK's own style policy is +re-validated on every compile, so a repository cannot quietly downgrade a rule. + +*At your scale:* always. If a rule matters and cannot be checked, say so plainly. + +## 5. Warnings are not acceptable output + +A clean build has no warnings. One that will never be fixed is either fixed, suppressed *where it +occurs* with a stated justification, or turned off deliberately. + +Warnings are a budget. A build that emits twenty is a build where the twenty-first is invisible — and +the twenty-first is the one that mattered. + +*Enforced by:* severity, partly. The SDK's shipped `.editorconfig` sets many rules to `error`. The +rest is review, backed by the pipeline running `dotnet build` with the repository's configuration. + +*At your scale:* a small repository can tolerate `warning`; a large one cannot. Decide deliberately +rather than by accumulation. + +## 6. Every claim about a consumer is proved by a consumer + +When a package documents how it must be used — a target framework, an asset, a guard, a workaround — +something in the repository builds that documented consumer and asserts the documented outcome. + +Consumer-facing documentation is the part of a library least likely to be exercised by its own tests, +because **the library's own build is not a consumer**. It drifts silently and fails in somebody +else's repository. + +*Enforced by:* a verification script that packs, then builds throwaway consumer projects against the +produced packages, asserting the happy path *and* the documented guard errors. `purview-dev/containers` +is the worked example (`just verify-consumers`), deliberately excluded from the default unit filter +because it restores from nuget.org. + +*At your scale:* only once a package is consumed outside its own repository. Before that, the +repository's own tests already are the consumer. + +## 7. Documentation is a build artefact of the repository + +The wiki lives in the repository it describes, in `docs/wiki`, and the repository owns its +`mkdocs.yml`, its navigation and its `edit_uri`. + +Documentation that lives elsewhere is updated elsewhere. A page with no owner and no review path is +wrong within one release. + +*Enforced by:* review, plus the navigation file — a page absent from `_Sidebar.md` is not published. + +*At your scale:* always. Even a repository with one page should own that page. + +## 8. Everything a test needs is built by one builder + +Domain content in a test comes from `TestBuilder`. A test never constructs a domain object inline, +and it overrides **only** the value it is proving. + +Inline construction encodes a type's shape into every test that touches it, so one constructor change +becomes a repository-wide edit. It also hides which values the test actually depends on, because +every value looks deliberate. + +*Enforced by:* review. + +*At your scale:* adopt from the first test. Retrofitting is a mechanical edit across every test file, +and exactly the kind of edit that is postponed forever. + +## 9. An agent is a reader with no memory + +A repository states its purpose, its layout and its conventions in `AGENTS.md`. **Nothing important +is discoverable only by reading the code.** + +A coding agent has the repository and nothing else — no colleague to ask, no institutional memory, no +intuition about which of two plausible patterns is intended. It will confidently generalise from +whatever it finds first. That is also every new-joiner failure mode; the agent just makes the cost +visible sooner, because an agent does not ask. + +*Enforced by:* review, plus publishing skills so agent-facing guidance tracks the library version +rather than a repository's memory of it. + +*At your scale:* always. `AGENTS.md` is cheaper than the questions it prevents. + +## 10. A change is verified in proportion to its risk + +Checks are tiered, and a repository declares which tiers its pipeline runs. A pull request runs the +fast tier; the slow tier runs deliberately, locally or on a dedicated runner. + +A pipeline that runs everything on every pull request gets made optional. One that runs too little +gets ignored. Tiering is what keeps the gate credible. + +*Enforced by:* the pipeline's `TestFilter` and the test project's own **name** — the SDK categorises +a `*UnitTests` project as `Unit`, so the tier is a property of the project rather than of a +hand-written attribute. + +*At your scale:* one tier is fine until the slow tests exist. Then split by project name, not by +attribute. + +## Applying these in review + +Three questions catch most of it: + +1. **Is this configuration, or is it a default the SDK should own?** If two repositories would copy + it, it belongs in the SDK. +2. **Is this rule enforced, or merely stated?** If stated, say so where it is stated. +3. **Could an agent — or a new joiner — find this without being told?** If not, it goes in + `AGENTS.md` or the wiki. + +## Related skills + +`purview-architecture` for project-shape decisions, `purview-architecture-testing` for turning a +boundary into a failing build, `purview-telemetry` and `purview-persistence` for the per-concern +guidance. diff --git a/skills/purview-testing-conventions/SKILL.md b/skills/purview-testing-conventions/SKILL.md new file mode 100644 index 0000000..078f85f --- /dev/null +++ b/skills/purview-testing-conventions/SKILL.md @@ -0,0 +1,255 @@ +--- +name: purview-testing-conventions +description: >- + Use when writing or reviewing tests in a .NET repository following the Purview conventions - TUnit + and TUnit.Mocks, the AAA structure and naming, placing a test in the namespace of the class under + test, CreateXXX construction helpers, building domain content through TestBuilder with Bogus, the + assertion surface, parameterised tests, and which tier a test project belongs to. +metadata: + product: Purview engineering playbook + repository: purview-dev/engineering-playbook +--- + +# Testing conventions + +| Concern | The rule | +| --- | --- | +| Framework | **TUnit**. Not xUnit, NUnit or MSTest. | +| Substitutes | **TUnit.Mocks**. Not NSubstitute or Moq. | +| Test data | **Bogus**, reached through `TestBuilder`. | +| Structure | Explicit `// Arrange`, `// Act`, `// Assert`. | +| Naming | `{SubjectUnderTest}_{Scenario}_{Expectation}`, and `async Task`. | +| Placement | One `{Class}Tests` class per class under test, **in the namespace of the class under test**. | +| Construction | Dependencies and the system under test come from `CreateXXX` helpers with optional parameters. | +| Cancellation | Pass a `CancellationToken` wherever a downstream method accepts one. | +| Domain content | Built through `TestBuilder`. Never inline. | + +## Why these, specifically + +**One framework.** The SDK wires TUnit into any project whose name ends in a test suffix — that +wiring is why a test project file is empty. A second framework means every project file grows a +conditional and the pipeline grows a second discovery path. + +**Same namespace as the subject.** The test reads as part of the thing it describes, needs no `using` +to reach the type, and **cannot silently test a different type with the same name**. + +**`CreateXXX` helpers.** A test body that constructs its own dependencies buries the assertion under +setup. A helper with optional parameters says what the test changed and defaults everything else: + +```csharp +static ConventionCatalogue CreateCatalogue(IConventionRepository? repository = null) => + new(repository ?? CreateRepository().Object); +``` + +**`TestBuilder`.** Inline construction encodes a domain type's shape into every test that touches it, +so one constructor change becomes a repository-wide edit. It also destroys the reader's signal about +which values the test depends on, because every literal looks deliberate. + +## A test + +```csharp +using Acme.Builders; + +namespace Acme.Domain; + +public sealed class CustomerIdTests +{ + [Test] + public async Task Create_GivenLowercaseSlug_ReturnsTheValue() + { + // Arrange + var id = TestBuilder.CustomerId("build-enforced"); + + // Act + var value = id.Value; + + // Assert + await Assert.That(value).IsEqualTo("build-enforced"); + } +} +``` + +Two deliberate details: + +- **The empty `// Arrange` block stays.** The comments are what makes a three-line test readable next + to a thirty-line one, and a convention applied "when needed" is applied inconsistently. +- **Assertions are awaited.** `Assert.That(...)` is terminal and returns a value, so + `await Assert.That(result).IsEqualTo(expected)` is the form. **An un-awaited assertion is a test + that cannot fail.** + +## Assertions + +| Form | Use | +| --- | --- | +| `await Assert.That(x).IsEqualTo(y)` / `.IsNotEqualTo(y)` | value equality | +| `await Assert.That(x).IsTrue()` / `.IsFalse()` | booleans | +| `await Assert.That(x).IsEmpty()` | collections — the workhorse of architecture rules | +| `await Assert.That(x).Count().IsEqualTo(2)` | collection size | +| `await Assert.That(x).Contains(y)` / `.DoesNotContain(y)` | membership | +| `await Assert.That(act).Throws()` | a synchronous delegate that should throw | +| `await Assert.That(async () => await f()).Throws()` | an async one | +| `await Assert.That(x).IsTypeOf()` | exact runtime type | +| `await Assert.That(x).IsEquivalentTo(sequence)` | order-independent sequence equality | + +For an exception test the two shapes are `Action` and `Func`. The compiler will not infer +either, so the explicit type is required and is **not** a style violation: + +```csharp +Action act = () => CustomerId.Create(""); + +await Assert.That(act).Throws(); +``` + +## Parameterised tests + +`[Arguments]` puts the interesting values in the attribute rather than a loop, so a failure names the +case: + +```csharp +[Test] +[Arguments("")] +[Arguments(" ")] +[Arguments("Has Spaces")] +[Arguments("has_underscore")] +public async Task Create_GivenSlugThatIsNotALowercaseSlug_Throws(string value) +{ + // Arrange + // Act + Action act = () => CustomerId.Create(value); + + // Assert + await Assert.That(act).Throws(); +} +``` + +## Substitutes + +TUnit.Mocks is globally imported by the SDK, so no `using` is needed: + +```csharp +[Test] +public async Task RegisterAsync_GivenAnAlreadyRegisteredConvention_DoesNotStoreIt(CancellationToken cancellationToken) +{ + // Arrange + var repository = CreateRepository(); + repository.ExistsAsync(Any(), Any()).Returns(true); + var catalogue = CreateCatalogue(repository.Object); + + // Act + await Assert + .That(async () => await catalogue.RegisterAsync(TestBuilder.EnforcedConvention(), cancellationToken)) + .Throws(); + + // Assert + repository.AddAsync(Any(), Any()).WasNeverCalled(); +} +``` + +- `Mock.Of()` produces a `Mock`; `.Object` is the instance you pass in. +- `Any()` matches any argument, `Is(value)` a specific one, `AnyArgs()` the whole call. +- `.Returns(value)` sets the result. For a `Task`-returning method, return the `T` — the task is + unwrapped for you. +- `.WasCalled(Times.Once)` and `.WasNeverCalled()` are the assertions. + +The `// Assert` after a `Throws` is not a mistake: that test's point is that the exception was thrown +**and** that nothing was written, and both are assertions about the same act. + +## `TestBuilder` + +It lives in the shared testing project and is the single means by which a test creates domain +content: + +```csharp +public static class TestBuilder +{ + public const int DefaultCount = 3; + + internal static Faker Faker { get; } = new(); + + public static Domain.ConventionId ConventionId(string? value = null) => + Domain.ConventionId.Create(value ?? $"{Slug()}-{Faker.Random.String2(6, LowercaseAlphabet)}"); + + public static Domain.Convention Convention( + Domain.ConventionId? id = null, + string? title = null, + VerificationTier? tier = null, + bool? enforcedByBuild = null) => + Domain.Convention.Create( + id ?? ConventionId(), + title ?? Faker.Lorem.Sentence(4), + tier ?? Faker.Random.Enum(), + enforcedByBuild ?? Faker.Random.Bool()); +} +``` + +Four rules make it work: + +- **Omitted arguments are generated; supplied arguments are used exactly.** A test overrides only the + value it is proving, and everything else is realistic without the test saying so. +- **A collection factory comes with a companion.** `Conventions(int count = DefaultCount)` generates a + distinct set, so a test needing "some" gets "some" without its own loop. +- **Bogus is a global `using`.** `Faker` is available with no import — which is also why an explicit + `using Bogus;` in a shared testing project is reported as unnecessary. +- **A value-object test is the one exception.** A test proving what `Create` and `Hydrate` accept and + reject has to name the rejected input itself, so it constructs the invalid value directly. + Everything valid still goes through the builder. + +## Two constraints that shaped the builder + +Both are consequences of SDK rules, not preferences, and both were found by building it. + +**The factories are flat, not grouped.** The natural shape is a nested class per feature area +(`TestBuilder.Catalogue.Convention(...)`), and it cannot compile: the SDK ships +`dotnet_diagnostic.CA1034.severity = error`, so a publicly visible nested type fails the build, and +`PRSGD0008` refuses to let a repository add `CA1034` to `NoWarn`. Making the group `internal` does +not help either — the SDK's `InternalsVisibleTo` target runs on non-test projects and grants access +*to* test and shared-testing projects rather than *from* them, so a shared testing project's +internals are not visible to the tests beside it. An area that outgrows a flat builder gets its own +top-level builder class. + +**`var` is the house style, with one carve-out.** `csharp_style_var_elsewhere = true:warning` makes +an explicit type on a declaration initialised from a method call a warning (`IDE0007`). The carve-out +is object creation: the `PDS0003` analyzer prefers an explicit type with target-typed `new()`: + +```csharp +var id = TestBuilder.ConventionId("build-enforced"); // method call -> var +string title = new('a', Convention.MaxTitleLength); // new() -> explicit type +``` + +## What is enforced, and what is review + +| Rule | Enforced by | +| --- | --- | +| TUnit is the only framework | the build — the SDK adds it; a second has to be added by hand | +| Test projects are discovered and categorised | the build (name-suffix detection) | +| A shared testing project is referenced, not executed | the build (`IsSharedTestingProject`) | +| Style, including the `var` carve-out | the build (analyzers, `warning` or above) | +| AAA structure, naming, `CreateXXX` helpers, `TestBuilder` usage | **review** | +| A `CancellationToken` is passed through | **review** | + +## Tiers + +A test project's **name** carries its tier — the SDK categorises `*UnitTests` as `Unit` — so the +pipeline's `TestFilter` selects a tier without a hand-written attribute. Split a slow suite into its +own project rather than tagging individual tests. + +Filtering uses TUnit's `--treenode-filter`, not `--filter`: these repositories run on +Microsoft.Testing.Platform, where `dotnet test --filter` reports "Zero tests ran". + +## At your scale + +- **One test project.** TUnit, AAA, the naming convention, `TestBuilder`. That is the whole of it. +- **A shared fixture.** Move it into `SharedTestingInfra` and make the class public — a shared + testing project is a library, so its fixtures are legitimately public API and `CA1515` does not + apply. +- **A database or a container.** `Purview.Containers` gives throwaway Linux containers behind one + API, over WSL Containers or Docker. Give the suite its own project so the tier is in the name, and + make it skip itself when no backend is reachable. It is **experimental and prerelease** — pin the + version you adopt. +- **A package with a documented contract.** Add a consumer-verification script; see + `purview-engineering-principles`. + +## Related skills + +`purview-engineering-principles` for the rules this implements, and `purview-architecture-testing` +for turning an architectural boundary into a failing build. diff --git a/src/EngineeringPlaybook.slnx b/src/EngineeringPlaybook.slnx index 54e15ff..9652864 100644 --- a/src/EngineeringPlaybook.slnx +++ b/src/EngineeringPlaybook.slnx @@ -20,5 +20,6 @@ + diff --git a/src/tests/Repository.UnitTests/Repository.UnitTests.csproj b/src/tests/Repository.UnitTests/Repository.UnitTests.csproj new file mode 100644 index 0000000..1307e1d --- /dev/null +++ b/src/tests/Repository.UnitTests/Repository.UnitTests.csproj @@ -0,0 +1,12 @@ + + + + + + + diff --git a/src/tests/Repository.UnitTests/SkillTests.cs b/src/tests/Repository.UnitTests/SkillTests.cs new file mode 100644 index 0000000..022f247 --- /dev/null +++ b/src/tests/Repository.UnitTests/SkillTests.cs @@ -0,0 +1,64 @@ +using Purview.Skills.Testing; + +namespace Purview.EngineeringPlaybook; + +/// +/// Runs the Agent Skills conformance rule set against this repository's authored skills in +/// skills/. +/// +/// +/// +/// The playbook states the rule that a published skill is well formed and correctly named; this is +/// the test that makes the statement true of the playbook's own skills. +/// +/// +/// [InheritsTests] is load-bearing. TUnit does not walk base types when it discovers +/// tests, so without it this class compiles, the run is green, and nothing is validated. +/// +/// +/// There is no RepositoryRoot override: the default is the SDK's repository-root marker walk, +/// the same resolution InstallPurviewSkills uses, so an ordinary test run exercises it. +/// is what catches a walk that landed elsewhere - +/// every inherited rule passes vacuously against a catalogue that was never found. +/// +/// +[InheritsTests] +public sealed class SkillTests : PurviewSkillTests +{ + [Test] + public async Task SkillsFolder_ResolvesToThisRepository() + { + var catalogue = Evaluator.Catalogue; + + await Assert + .That(catalogue.FolderExists) + .IsTrue() + .Because($"the marker walk resolved '{catalogue.RepositoryRoot}', whose skills folder does not exist"); + + await Assert + .That(File.Exists(Path.Combine(catalogue.RepositoryRoot, "AGENTS.md"))) + .IsTrue() + .Because($"'{catalogue.RepositoryRoot}' does not look like this repository"); + + await Assert.That(catalogue.Skills).IsNotEmpty(); + } + + [Test] + public async Task EverySkill_IsNamedForThisOrganisation() + { + // Skills install into one flat directory per agent, shared with every skill a developer has + // installed from anywhere. An unprefixed name collides with somebody else's, and the loser is + // whichever installed second. + var unprefixed = Evaluator + .Catalogue.Skills.Select(skill => skill.FolderName) + .Where(name => !name.StartsWith("purview-", StringComparison.Ordinal)) + .ToArray(); + + await Assert + .That(unprefixed) + .IsEmpty() + .Because( + "every skill this repository publishes must carry a vendor prefix: " + string.Join(", ", unprefixed) + ); + } +} From 722802c377c1f1ec98f7008b247ea9d1a162a68a Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 16:17:47 +0100 Subject: [PATCH 2/4] chore: run CSharpier on save in Visual Studio A tracked solution-level setting, so format-on-save matches the lint check the pipeline runs rather than being set per developer. --- src/settings.VisualStudio.json | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 src/settings.VisualStudio.json diff --git a/src/settings.VisualStudio.json b/src/settings.VisualStudio.json new file mode 100644 index 0000000..194ba35 --- /dev/null +++ b/src/settings.VisualStudio.json @@ -0,0 +1,4 @@ +/* Visual Studio Settings File */ +{ + "csharpier.general.runonsave": true +} From 905c05f6020ae8b898e7ece064164009f026b9a2 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 23:43:26 +0100 Subject: [PATCH 3/4] chore: structure --- Directory.Packages.props | 7 ++----- src/src/Application/ConventionCatalogue.cs | 17 +++++++---------- src/src/Domain/Convention.cs | 3 ++- src/src/Domain/ConventionId.cs | 3 ++- .../InMemoryConventionRepository.cs | 3 ++- .../ConventionCatalogueTests.cs | 12 ++++++------ .../Policy/ArchitecturePolicy.cs | 1 + .../Policy/ProjectGraph.cs | 4 ++-- src/tests/Domain.UnitTests/ConventionIdTests.cs | 12 ++++++------ src/tests/Domain.UnitTests/ConventionTests.cs | 12 ++++++------ src/tests/Repository.UnitTests/SkillTests.cs | 2 +- .../SharedTestingInfra/Builders/TestBuilder.cs | 10 +++++----- 12 files changed, 42 insertions(+), 44 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 1e84f24..4f74483 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -23,11 +23,9 @@ 10.10.0 - 5.0.3 + 5.0.4 - 1.75.0 @@ -54,7 +52,7 @@ - + @@ -62,7 +60,6 @@ - diff --git a/src/src/Application/ConventionCatalogue.cs b/src/src/Application/ConventionCatalogue.cs index 0ae3227..da151b1 100644 --- a/src/src/Application/ConventionCatalogue.cs +++ b/src/src/Application/ConventionCatalogue.cs @@ -9,17 +9,14 @@ namespace Purview.EngineeringPlaybook.Application; /// The class holds no infrastructure of its own: it takes the port, /// which is what makes it testable without a database and what keeps the dependency direction one-way. /// -public sealed class ConventionCatalogue +/// +/// Initialises the service. +/// +/// The persistence port. +/// is . +public sealed class ConventionCatalogue(IConventionRepository repository) { - readonly IConventionRepository _repository; - - /// - /// Initialises the service. - /// - /// The persistence port. - /// is . - public ConventionCatalogue(IConventionRepository repository) => - _repository = repository ?? throw new ArgumentNullException(nameof(repository)); + readonly IConventionRepository _repository = repository ?? throw new ArgumentNullException(nameof(repository)); /// /// Registers a convention, rejecting an identifier that is already in use. diff --git a/src/src/Domain/Convention.cs b/src/src/Domain/Convention.cs index b4a895a..0f7fc08 100644 --- a/src/src/Domain/Convention.cs +++ b/src/src/Domain/Convention.cs @@ -61,7 +61,8 @@ public static Convention Create(ConventionId id, string title, VerificationTier ); } - return new Convention(id, normalised, tier, enforcedByBuild); + // The title is valid and normalised, so construct the convention. + return new(id, normalised, tier, enforcedByBuild); } /// diff --git a/src/src/Domain/ConventionId.cs b/src/src/Domain/ConventionId.cs index f52ba1f..dcd93d8 100644 --- a/src/src/Domain/ConventionId.cs +++ b/src/src/Domain/ConventionId.cs @@ -80,7 +80,8 @@ public static ConventionId Hydrate(string value) throw new ArgumentException("A hydrated convention identifier cannot be empty.", nameof(value)); } - return new ConventionId(value); + // The value is already known to be valid, so we don't need to validate it again. + return new(value); } /// diff --git a/src/src/Infrastructure/InMemoryConventionRepository.cs b/src/src/Infrastructure/InMemoryConventionRepository.cs index 6e9807d..1666318 100644 --- a/src/src/Infrastructure/InMemoryConventionRepository.cs +++ b/src/src/Infrastructure/InMemoryConventionRepository.cs @@ -1,6 +1,6 @@ -using System.Collections.Concurrent; using Purview.EngineeringPlaybook.Application; using Purview.EngineeringPlaybook.Domain; +using System.Collections.Concurrent; namespace Purview.EngineeringPlaybook.Infrastructure; @@ -36,6 +36,7 @@ public Task AddAsync(Convention convention, CancellationToken cancellationToken) throw new InvalidOperationException($"Convention '{convention.Id}' is already registered."); } + // The repository is in-memory, so there's no actual async work to do. The method is async to satisfy the interface contract. return Task.CompletedTask; } diff --git a/src/tests/Application.UnitTests/ConventionCatalogueTests.cs b/src/tests/Application.UnitTests/ConventionCatalogueTests.cs index 2219823..d577b7d 100644 --- a/src/tests/Application.UnitTests/ConventionCatalogueTests.cs +++ b/src/tests/Application.UnitTests/ConventionCatalogueTests.cs @@ -37,10 +37,10 @@ public async Task RegisterAsync_GivenAnAlreadyRegisteredConvention_Throws(Cancel var catalogue = CreateCatalogue(repository.Object); // Act - Func act = () => catalogue.RegisterAsync(TestBuilder.EnforcedConvention(), cancellationToken); + Task Act() => catalogue.RegisterAsync(TestBuilder.EnforcedConvention(), cancellationToken); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -69,10 +69,10 @@ public async Task RegisterAsync_GivenNoConvention_Throws(CancellationToken cance var catalogue = CreateCatalogue(); // Act - Func act = () => catalogue.RegisterAsync(null!, cancellationToken); + Task Act() => catalogue.RegisterAsync(null!, cancellationToken); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -123,10 +123,10 @@ public async Task ConventionCatalogue_GivenNoRepository_Throws() { // Arrange // Act - Action act = () => _ = new ConventionCatalogue(null!); + static void Act() => _ = new ConventionCatalogue(null!); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } /// Creates the repository substitute. diff --git a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs index c6f41bc..5cff6b9 100644 --- a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs +++ b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs @@ -50,6 +50,7 @@ public static ArchitecturePolicy Load() if (!File.Exists(path)) throw new FileNotFoundException($"The architecture policy was not found at '{path}'.", path); + // The policy is a single source of truth, so deserialising to null is a failure. return JsonSerializer.Deserialize(File.ReadAllText(path), SerializerOptions) ?? throw new InvalidOperationException($"The architecture policy at '{path}' deserialised to null."); } diff --git a/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs b/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs index 58c0af4..c858153 100644 --- a/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs +++ b/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs @@ -107,8 +107,8 @@ static ProjectNode Read(string projectPath) /// public static IReadOnlyList FindCycles(IReadOnlyDictionary projects) { - List cycles = new(); - Dictionary state = new(); + List cycles = []; + Dictionary state = []; foreach (var project in projects.Keys) Visit(project, []); diff --git a/src/tests/Domain.UnitTests/ConventionIdTests.cs b/src/tests/Domain.UnitTests/ConventionIdTests.cs index 919d93c..ad74f4f 100644 --- a/src/tests/Domain.UnitTests/ConventionIdTests.cs +++ b/src/tests/Domain.UnitTests/ConventionIdTests.cs @@ -51,10 +51,10 @@ public async Task Create_GivenSlugThatIsNotALowercaseSlug_Throws(string value) { // Arrange // Act - Action act = () => ConventionId.Create(value); + void Act() => ConventionId.Create(value); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -64,10 +64,10 @@ public async Task Create_GivenSlugLongerThanTheMaximum_Throws() string slug = new('a', ConventionId.MaxLength + 1); // Act - Action act = () => ConventionId.Create(slug); + void Act() => ConventionId.Create(slug); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -86,10 +86,10 @@ public async Task Hydrate_GivenAnEmptyValue_Throws() { // Arrange // Act - Action act = () => ConventionId.Hydrate(" "); + static void Act() => ConventionId.Hydrate(" "); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] diff --git a/src/tests/Domain.UnitTests/ConventionTests.cs b/src/tests/Domain.UnitTests/ConventionTests.cs index ef13e03..9e72314 100644 --- a/src/tests/Domain.UnitTests/ConventionTests.cs +++ b/src/tests/Domain.UnitTests/ConventionTests.cs @@ -50,10 +50,10 @@ public async Task Create_GivenNoTitle_Throws(string title) var id = TestBuilder.ConventionId(); // Act - Action act = () => Convention.Create(id, title, VerificationTier.Unit, enforcedByBuild: true); + void Act() => Convention.Create(id, title, VerificationTier.Unit, enforcedByBuild: true); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -64,10 +64,10 @@ public async Task Create_GivenATitleLongerThanTheMaximum_Throws() string title = new('a', Convention.MaxTitleLength + 1); // Act - Action act = () => Convention.Create(id, title, VerificationTier.Unit, enforcedByBuild: true); + void Act() => Convention.Create(id, title, VerificationTier.Unit, enforcedByBuild: true); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] @@ -75,10 +75,10 @@ public async Task Create_GivenNoId_Throws() { // Arrange // Act - Action act = () => Convention.Create(null!, "Architecture is a test", VerificationTier.Unit, true); + static void Act() => Convention.Create(null!, "Architecture is a test", VerificationTier.Unit, true); // Assert - await Assert.That(act).Throws(); + await Assert.That(Act).Throws(); } [Test] diff --git a/src/tests/Repository.UnitTests/SkillTests.cs b/src/tests/Repository.UnitTests/SkillTests.cs index 022f247..5173650 100644 --- a/src/tests/Repository.UnitTests/SkillTests.cs +++ b/src/tests/Repository.UnitTests/SkillTests.cs @@ -1,6 +1,6 @@ using Purview.Skills.Testing; -namespace Purview.EngineeringPlaybook; +namespace Purview.EngineeringPlaybook.Repository; /// /// Runs the Agent Skills conformance rule set against this repository's authored skills in diff --git a/src/tests/SharedTestingInfra/Builders/TestBuilder.cs b/src/tests/SharedTestingInfra/Builders/TestBuilder.cs index 38e6f73..318154b 100644 --- a/src/tests/SharedTestingInfra/Builders/TestBuilder.cs +++ b/src/tests/SharedTestingInfra/Builders/TestBuilder.cs @@ -48,7 +48,7 @@ public static class TestBuilder /// /// The slug to use; omitted generates a unique one. /// The identifier. - public static Domain.ConventionId ConventionId(string? value = null) => + public static ConventionId ConventionId(string? value = null) => Domain.ConventionId.Create(value ?? $"{Slug()}-{Faker.Random.String2(6, LowercaseAlphabet)}"); /// @@ -59,8 +59,8 @@ public static Domain.ConventionId ConventionId(string? value = null) => /// The verification tier; omitted generates one. /// Whether the build enforces it; omitted generates one. /// The convention. - public static Domain.Convention Convention( - Domain.ConventionId? id = null, + public static Convention Convention( + ConventionId? id = null, string? title = null, VerificationTier? tier = null, bool? enforcedByBuild = null @@ -77,7 +77,7 @@ public static Domain.Convention Convention( /// /// The identifier; omitted generates one. /// The convention. - public static Domain.Convention EnforcedConvention(Domain.ConventionId? id = null) => + public static Convention EnforcedConvention(ConventionId? id = null) => Convention(id: id, tier: VerificationTier.Unit, enforcedByBuild: true); /// @@ -85,7 +85,7 @@ public static Domain.Convention EnforcedConvention(Domain.ConventionId? id = nul /// /// How many to create; defaults to . /// The conventions. - public static IReadOnlyList Conventions(int count = DefaultCount) => + public static IReadOnlyList Conventions(int count = DefaultCount) => [.. Enumerable.Range(0, count).Select(_ => Convention())]; /// From 0fbce82fc08db5496ca159a725f926fc17868b79 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Fri, 16 Oct 2026 00:24:54 +0100 Subject: [PATCH 4/4] refactor: updated rules from build-sdk --- .config/architecture-policy.json | 3 +- .config/architecture-policy.schema.json | 161 --------- AGENTS.md | 7 +- Directory.Packages.props | 22 +- README.md | 9 +- docs/wiki/Adopting-This-At-Your-Scale.md | 34 +- docs/wiki/Agent-Guidance-Skills.md | 7 +- docs/wiki/Architecture-As-Tests.md | 7 + docs/wiki/Contracts-And-Data-Exchange.md | 2 +- docs/wiki/Contributing.md | 30 +- docs/wiki/Documentation-Convention.md | 17 +- docs/wiki/Engineering-Principles.md | 193 ++--------- docs/wiki/Getting-Started.md | 102 ++++-- docs/wiki/Home.md | 6 +- docs/wiki/Project-Responsibilities.md | 63 ++-- docs/wiki/Repository-Shape.md | 3 +- docs/wiki/Worked-Examples.md | 55 ++- skills/purview-architecture-testing/SKILL.md | 90 +++-- .../InMemoryConventionRepository.cs | 2 +- .../Architecture.UnitTests.csproj | 8 +- .../ArchitectureTests.cs | 27 ++ .../Policy/ArchitecturePolicy.cs | 196 ----------- .../Policy/ProjectGraph.cs | 148 --------- .../PolicyDocumentationTests.cs | 28 +- .../PolicySchemaTests.cs | 186 ++--------- .../ProjectDependencyPolicyTests.cs | 313 ------------------ .../DocumentationTests.cs | 179 ++++++++++ templates/Directory.Packages.props | 23 +- templates/README.md | 10 +- templates/architecture-policy.json | 70 ++++ templates/dotnet-tools.json | 7 + templates/global.json | 2 +- 32 files changed, 704 insertions(+), 1306 deletions(-) delete mode 100644 .config/architecture-policy.schema.json create mode 100644 src/tests/Architecture.UnitTests/ArchitectureTests.cs delete mode 100644 src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs delete mode 100644 src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs delete mode 100644 src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs create mode 100644 src/tests/Repository.UnitTests/DocumentationTests.cs create mode 100644 templates/architecture-policy.json diff --git a/.config/architecture-policy.json b/.config/architecture-policy.json index 879d816..b289a14 100644 --- a/.config/architecture-policy.json +++ b/.config/architecture-policy.json @@ -1,7 +1,8 @@ { - "$schema": "https://raw.githubusercontent.com/purview-dev/engineering-playbook/main/.config/architecture-policy.schema.json", + "$schema": "https://raw.githubusercontent.com/purview-dev/build-sdk/main/architecture-policy.schema.json", "version": 1, "description": "The authoritative dependency and naming policy. The architecture tests read this file, and a test asserts the wiki tables agree with it, so there is one definition of each rule rather than one in prose and one in code.", + "sourceRoots": ["src"], "conventions": { "qualifiedNameSeparator": ".", "dependencyArrowMeaning": "A -> B means A depends on B at compile time." diff --git a/.config/architecture-policy.schema.json b/.config/architecture-policy.schema.json deleted file mode 100644 index daeda82..0000000 --- a/.config/architecture-policy.schema.json +++ /dev/null @@ -1,161 +0,0 @@ -{ - "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": "https://raw.githubusercontent.com/purview-dev/engineering-playbook/main/.config/architecture-policy.schema.json", - "title": "Purview architecture policy", - "description": "The authoritative project responsibility, naming and dependency policy. The architecture tests read this document; PolicyDocumentationTests asserts the wiki agrees with it. See docs/wiki/Project-Responsibilities.md.", - "type": "object", - "required": ["version", "responsibilities"], - "additionalProperties": false, - "properties": { - "$schema": { - "type": "string", - "description": "Pointer to this schema, so an editor validates the file as it is written." - }, - "version": { - "type": "integer", - "minimum": 1, - "description": "Schema version of this document. Increment when a change would break a reader that understood the previous shape." - }, - "description": { - "type": "string" - }, - "conventions": { - "type": "object", - "additionalProperties": false, - "properties": { - "qualifiedNameSeparator": { - "type": "string", - "minLength": 1, - "maxLength": 1, - "description": "Separates the responsibility from its implementation qualifier, as in Persistence.SqlServer." - }, - "dependencyArrowMeaning": { - "type": "string", - "description": "States what an arrow means, so compile-time references are not confused with runtime calls." - } - } - }, - "responsibilities": { - "type": "array", - "minItems": 1, - "description": "The responsibility catalogue. A project name is [.]; only the first segment is classified.", - "items": { "$ref": "#/$defs/responsibility" } - }, - "discouragedProjectNames": { - "type": "object", - "required": ["names", "reason"], - "additionalProperties": false, - "description": "Names that describe a category rather than a responsibility, so nothing can be excluded from them.", - "properties": { - "names": { - "type": "array", - "minItems": 1, - "items": { "$ref": "#/$defs/projectNameSegment" } - }, - "reason": { "type": "string", "minLength": 20 } - } - }, - "rules": { - "type": "object", - "additionalProperties": false, - "description": "Repository-wide rules that are not expressed per responsibility.", - "properties": { - "noCycles": { "type": "boolean" }, - "noProductionToTestReferences": { "type": "boolean" }, - "implementationsMustNotReferenceComposition": { "type": "boolean" }, - "siblingImplementationsIndependent": { "type": "boolean" } - } - }, - "waivers": { - "type": "array", - "description": "Deliberate, reviewed departures. A waiver excuses the narrowest thing possible - usually the name - and mapsTo keeps the project classified so every dependency rule still applies.", - "items": { "$ref": "#/$defs/waiver" } - } - }, - "$defs": { - "projectNameSegment": { - "type": "string", - "pattern": "^[A-Za-z][A-Za-z0-9]*$", - "description": "A single PascalCase segment, with no dots: the responsibility, not a qualified project name." - }, - "responsibility": { - "type": "object", - "required": ["name", "category", "purpose", "mayDependOn", "mustNotDependOn"], - "additionalProperties": false, - "properties": { - "name": { "$ref": "#/$defs/projectNameSegment" }, - "aliases": { - "type": "array", - "description": "Recognised alternative names. An established project using one of these is not renamed.", - "items": { "$ref": "#/$defs/projectNameSegment" } - }, - "category": { - "type": "string", - "enum": ["domain", "application", "infrastructure", "composition", "host", "shared"] - }, - "purpose": { "type": "string", "minLength": 20 }, - "mayDependOn": { - "type": "array", - "description": "Responsibilities this one may reference at compile time. An empty array means it depends on nothing.", - "items": { "$ref": "#/$defs/projectNameSegment" }, - "uniqueItems": true - }, - "mustNotDependOn": { - "type": "array", - "description": "Responsibilities this one must never reference. Must not overlap mayDependOn.", - "items": { "$ref": "#/$defs/projectNameSegment" }, - "uniqueItems": true - }, - "forbiddenPackagePrefixes": { - "type": "array", - "description": "NuGet identifier prefixes that must not appear in this responsibility, such as Microsoft.EntityFrameworkCore in Core.", - "items": { "type": "string", "minLength": 1 }, - "uniqueItems": true - }, - "siblingIndependence": { - "type": "boolean", - "description": "When true, qualified implementations of this responsibility must not reference one another.", - "default": false - }, - "separateProjectWhen": { "type": "string", "minLength": 10 }, - "mayRemainCombinedWhen": { "type": "string", "minLength": 10 } - } - }, - "waiver": { - "type": "object", - "required": ["project", "rules", "reason"], - "additionalProperties": false, - "properties": { - "project": { - "type": "string", - "minLength": 1, - "description": "The project name the waiver applies to, exactly as it appears on disk." - }, - "mapsTo": { - "$ref": "#/$defs/projectNameSegment", - "description": "The responsibility this project actually has. Keeps every dependency rule in force while excusing the name." - }, - "rules": { - "type": "array", - "minItems": 1, - "description": "The rules this waiver excuses. A waiver that excuses nothing does nothing.", - "items": { - "type": "string", - "enum": ["naming", "discouragedName"] - }, - "uniqueItems": true - }, - "reason": { - "type": "string", - "minLength": 40, - "description": "Why the departure is acceptable. A blanket suppression with no reason makes the policy unfalsifiable." - }, - "reviewedOn": { - "type": "string", - "format": "date", - "pattern": "^\\d{4}-\\d{2}-\\d{2}$" - } - } - } - } -} diff --git a/AGENTS.md b/AGENTS.md index 3393977..90e4e2b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,12 +28,14 @@ implementation whose only job is to prove the conventions compile, run, and fail | `src/src/Infrastructure` | The in-memory adapter that implements the port | | `src/tests/Domain.UnitTests` | Value-object behaviour; the value-object exception to `TestBuilder` | | `src/tests/Application.UnitTests` | The service, against a TUnit.Mocks substitute | -| `src/tests/Architecture.UnitTests` | Dependency-direction rules; a wrong `ProjectReference` fails the build | +| `src/tests/Architecture.UnitTests` | Assembly dependency-direction rules and the architecture policy; a wrong `ProjectReference` fails the build | +| `src/tests/Repository.UnitTests` | The repository itself: skill conformance and documentation checks | | `src/tests/SharedTestingInfra` | `TestBuilder`; referenced automatically by every test project | | `skills/` | Authored agent skills, installable with `npx skills add purview-dev/engineering-playbook` | | `templates/` | Ready-to-copy starter files for a repository adopting the playbook | | `src/Directory.Build.props` | `NamespacePrefix`, the SDK import, and `NamespaceRemoveSuffix` | | `.config/purview-build.json` | Pipeline configuration: solution, test discovery, filter | +| `.config/architecture-policy.json` | The dependency and naming policy the architecture tests read | | `Justfile` | Supported local commands | | `package.json` | Authoritative version; the release reads it and nothing else | @@ -54,7 +56,8 @@ demonstrate it. - **One page per concern**, in `docs/wiki/.md`. Title case, hyphens between words. - **Four sections, in order:** the rule, why, how it is enforced, at your scale. A rule a build cannot check - is labelled a review obligation — never described as enforced. + is labelled a review obligation — never described as enforced. `Home.md` and `index.md` are navigation, + and `Engineering-Principles.md` is an index that links to the page stating each rule in full. - **A new page goes in `_Sidebar.md` and in the table in `Home.md`.** A page in neither does not exist. - **Link to the authority rather than paraphrasing it.** The SDK's own wiki is authoritative on the SDK's behaviour; a paraphrase here is a second source of truth that will drift. diff --git a/Directory.Packages.props b/Directory.Packages.props index 4f74483..c69a11e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -24,6 +24,11 @@ 5.0.4 + + 1.0.6 1.75.0 @@ -41,15 +46,6 @@ - - - - @@ -60,6 +56,12 @@ - + + + diff --git a/README.md b/README.md index b72b26c..d996a42 100644 --- a/README.md +++ b/README.md @@ -12,15 +12,22 @@ coding agents are enabled to work in a codebase without hand-holding. | Area | Question it answers | | -- | -- | -| [Engineering principles](docs/wiki/Engineering-Principles.md) | What are the non-negotiable rules, and who owns them? | +| [Getting started](docs/wiki/Getting-Started.md) | What is the smallest repository that satisfies every rule, in the order you build it? | +| [Engineering principles](docs/wiki/Engineering-Principles.md) | What are the non-negotiable rules, and where is each one stated in full? | | [Repository shape](docs/wiki/Repository-Shape.md) | Where does a file go, and what is it called? | +| [Project responsibilities](docs/wiki/Project-Responsibilities.md) | What is each project for, how is it named, and what may it depend on? | | [Architecture as tests](docs/wiki/Architecture-As-Tests.md) | How does the build refuse a dependency that points the wrong way? | | [Verification model](docs/wiki/Verification-Model.md) | Which checks run, in which tier, for which change? | | [Contracts and data exchange](docs/wiki/Contracts-And-Data-Exchange.md) | How do value objects, schemas and results cross a boundary? | | [Testing and test data](docs/wiki/Testing-And-Test-Data.md) | How are tests written, and where does their data come from? | | [Agent enablement](docs/wiki/Agent-Enablement.md) | How is a repository made legible to a coding agent? | +| [Agent skills distribution](docs/wiki/Agent-Skills-Distribution.md) | How is a skill published once and reach an agent? | +| [Agent guidance skills](docs/wiki/Agent-Guidance-Skills.md) | Which shared guidance ships as a skill, and what context comes first? | | [Documentation convention](docs/wiki/Documentation-Convention.md) | How is the wiki written, structured and published? | +| [Worked examples](docs/wiki/Worked-Examples.md) | What does the reference implementation under `src/` actually demonstrate? | | [Adopting this at your scale](docs/wiki/Adopting-This-At-Your-Scale.md) | Which parts apply to a small, medium or large repository? | +| [Contributing](docs/wiki/Contributing.md) | How do I change the playbook itself? | +| [Release flow](docs/wiki/Release-Flow.md) | How does a change become a version, and what does a release mean here? | ## Start here diff --git a/docs/wiki/Adopting-This-At-Your-Scale.md b/docs/wiki/Adopting-This-At-Your-Scale.md index b67ed97..26bca92 100644 --- a/docs/wiki/Adopting-This-At-Your-Scale.md +++ b/docs/wiki/Adopting-This-At-Your-Scale.md @@ -1,12 +1,30 @@ # Adopting This at Your Scale +## The rule + Every rule in this playbook came from a repository that needed it. That is the argument for the rule and also the reason not to take all of them: a rule adopted before the problem it prevents exists is pure cost. -This page is the short version of that decision. Find the profile that matches, take what it lists, and -leave the rest — with the knowledge that the rest is there when the repository grows into it. +Find the profile that matches, take what it lists, and leave the rest — with the knowledge that the rest is +there when the repository grows into it. + +## Why + +Structure is a cost paid now against a failure expected later. A boundary added in anticipation of a second +caller that never arrives is maintained forever; a boundary added when the second caller appears is cheap. +The same is true of a CI tier, a pack manifest or an agent skill: taken early it is ceremony, taken late it +is a migration. Naming the smaller version explicitly is what lets a reader take the part that fits instead +of cargo-culting the whole playbook. + +## How it is enforced + +This page is guidance rather than a rule, so nothing here is enforced by a build. The rules it chooses +between are enforced as their own pages say; what this page adds is the decision about which of them to +adopt, and that decision is a review obligation. + +## At your scale -## The three profiles +### The three profiles | | Small | Medium | Large | | -- | -- | -- | -- | @@ -22,7 +40,7 @@ leave the rest — with the knowledge that the rest is there when the repository | **`AGENTS.md`** | Yes | Yes | Yes | | **Agent skills** | Install what you need with `npx skills add` | Plus one per repeated review comment | Plus your own, published from the repository that owns the convention | -## Small — one project +### Small — one project Adopt: @@ -43,7 +61,7 @@ Skip: - Pack validation, consumer verification, release channels. All three need a published package to mean anything. -## Medium — a few layers +### Medium — a few layers Everything from Small, plus: @@ -59,7 +77,7 @@ Everything from Small, plus: This is the profile the reference implementation in this repository is built to. It is the smallest shape where every mechanism in the playbook is visible. -## Large — several packages +### Large — several packages Everything from Medium, plus: @@ -74,7 +92,7 @@ Everything from Medium, plus: - **Architecture rules promoted into the build.** At this size a rule that can be skipped is a rule that will be, so move it from a test into an analyzer or an MSBuild check that runs before compile. -## What not to adopt early +### What not to adopt early Four things that are more expensive than they look when taken out of order. @@ -92,7 +110,7 @@ the wrong thing to discover while adding the first package. Turn on pack validat **Consumer verification before there is a consumer.** The script exists to keep a published contract honest. With no published contract it is a slow test that restores from nuget.org and proves nothing. -## Migrating an existing repository +### Migrating an existing repository The order matters, because each step makes the next one cheaper. diff --git a/docs/wiki/Agent-Guidance-Skills.md b/docs/wiki/Agent-Guidance-Skills.md index 46c65e3..7373d26 100644 --- a/docs/wiki/Agent-Guidance-Skills.md +++ b/docs/wiki/Agent-Guidance-Skills.md @@ -2,12 +2,15 @@ ## The rule -Three skills carry the guidance that is not specific to any one Purview library, and they are the -only place that guidance is written: +The skills this repository publishes carry the guidance that is not specific to any one Purview +library, and they are the only place that guidance is written: | Skill | Answers | | -- | -- | +| `purview-engineering-principles` | What are the ten repository conventions, what does each prevent, and is it enforced by a build or by a reviewer? | | `purview-architecture` | How much structure does this project actually need, and what do I need to know before saying? | +| `purview-architecture-testing` | How does a boundary become a failing build rather than a review comment? | +| `purview-testing-conventions` | How are tests written and fed — framework, structure, naming, and `TestBuilder`? | | `purview-telemetry` | What instrumentation does a project of this size get? (Answer: always some.) | | `purview-persistence` | How is data stored, and — only when it applies — how are GUID keys generated? | diff --git a/docs/wiki/Architecture-As-Tests.md b/docs/wiki/Architecture-As-Tests.md index 31a2bd0..10b6930 100644 --- a/docs/wiki/Architecture-As-Tests.md +++ b/docs/wiki/Architecture-As-Tests.md @@ -162,6 +162,13 @@ The rules are ordinary tests in a project named `*UnitTests`, so the pipeline ru request as part of the fast tier. No extra workflow, no extra step, no separate tool: an architecture violation is a failing test with the same status as any other. +These four rules are hand-written here because they read **compiled assemblies**, which is what lets +them catch a dependency that arrived indirectly rather than one that was declared. The broader +**policy** rules — the dependency matrix, naming, sibling independence, the composition boundary, +forbidden packages, cycles, test references and waivers — ship in `Purview.Architecture.Testing`, and +are adopted with a policy file and one derived test class rather than with C#. See +[Project Responsibilities](Project-Responsibilities.md). + ## At your scale - **One project.** Nothing to assert. Skip it. diff --git a/docs/wiki/Contracts-And-Data-Exchange.md b/docs/wiki/Contracts-And-Data-Exchange.md index 4aab245..3a23339 100644 --- a/docs/wiki/Contracts-And-Data-Exchange.md +++ b/docs/wiki/Contracts-And-Data-Exchange.md @@ -1,6 +1,6 @@ # Contracts and Data Exchange -## The rules +## The rule | Rule | Statement | | -- | -- | diff --git a/docs/wiki/Contributing.md b/docs/wiki/Contributing.md index 87606e2..5fa649c 100644 --- a/docs/wiki/Contributing.md +++ b/docs/wiki/Contributing.md @@ -1,9 +1,11 @@ # Contributing +## The rule + This repository is documentation with a small reference implementation attached. The documentation is the product; the reference implementation exists to prove the documentation is not guesswork. -## What belongs here +### What belongs here A rule belongs in the playbook when all four of these are true: @@ -19,6 +21,14 @@ What does not belong: anything that names a product, a customer, or a repository playbook is a published artefact, and a rule that can only be justified by something private cannot be reviewed by the people it is published to. +## Why + +The four criteria are the whole value of the playbook. A rule without a failure behind it is an opinion, and +opinions accumulate until the useful rules are indistinguishable from the noise. A rule that only works at +one size gets cargo-culted at another. A rule that restates the SDK's own documentation is a second source of +truth that will drift. And a rule that claims enforcement it does not have is worse than no rule at all, +because people rely on the guarantee and stop looking. Each criterion removes one of those failure modes. + ## Adding a page 1. **Write the page** in `docs/wiki/.md`. Title case, hyphens between words. @@ -54,7 +64,11 @@ Changing a rule is a bigger change than adding one, because pages link to it and is not — a published package contract, a persistence layer, a second CI tier — say so on the page and point at the public repository that does demonstrate it. -## Commit messages +## How it is enforced + +Three mechanisms, and they are the ones a change to this repository actually passes through. + +### Commit messages Conventional Commits, enforced by a `commit-msg` hook. The allowed types are in `commitlint.config.mts`: @@ -66,7 +80,7 @@ The header is limited to 100 characters, the subject must not be sentence case, A documentation-only change is `docs:`; a change to the reference implementation is `feat:` or `fix:`; a change to the pipeline configuration is `ci:` or `build:`. -## Validation before handing work back +### Validation before handing work back ```text just lint-check # CSharpier check @@ -81,7 +95,7 @@ risk — an unstated gap is the one that gets discovered in production. For a documentation-only change, the checks that matter are the ones that catch a broken reference: the navigation in `_Sidebar.md`, the table in `Home.md`, and any relative link you added. -## Review expectations +### Review expectations A reviewer should be able to answer these from the pull request alone: @@ -91,6 +105,14 @@ A reviewer should be able to answer these from the pull request alone: - If the rule is the SDK's, why is it restated here rather than linked? - Does `src/` still demonstrate the conventions, or has it drifted? +## At your scale + +This page is for changing the playbook itself. A repository that is only *borrowing* from the playbook does +not need any of it: take the pages and templates that fit and leave the rest, as [Adopting This at Your +Scale](Adopting-This-At-Your-Scale.md) describes. The smaller version of this process, for a +documentation-only change, is: edit the page, keep `_Sidebar.md` and `Home.md` in step, and run the reference +checks in [Validation before handing work back](#validation-before-handing-work-back). + ## See also - [Documentation Convention](Documentation-Convention.md) diff --git a/docs/wiki/Documentation-Convention.md b/docs/wiki/Documentation-Convention.md index b04e32f..a70c74c 100644 --- a/docs/wiki/Documentation-Convention.md +++ b/docs/wiki/Documentation-Convention.md @@ -70,7 +70,7 @@ Three of those lines are load-bearing: ## How to write a page -Every page in this playbook uses the same four sections, in this order: +Every content page uses the same four sections, in this order: | Section | Answers | | -- | -- | @@ -79,6 +79,11 @@ Every page in this playbook uses the same four sections, in this order: | **How it is enforced** | The mechanism that makes it true — or an explicit statement that it is a review obligation. | | **At your scale** | What to do when the repository is smaller or larger than the one the rule came from. | +Three pages are deliberately not content pages. `Home.md` is the hub and `index.md` is the landing +page, so both are navigation rather than rules. `Engineering-Principles.md` is an index: it states each +of the ten rules in one line and links to the page that states it in full, so a rule is never written +twice. + The shape is not decoration. It forces three questions that a page written as prose usually skips: *is this actually a rule, or an opinion?*; *what breaks if we drop it?*; and *is it enforced, or are we asking a reviewer to remember?* @@ -120,13 +125,15 @@ and it is the fastest way for a reader to decide whether the rule applies to the | Rule | Enforced by | | -- | -- | | The site configuration is present and points at `docs/wiki` | Review | -| A page is in `_Sidebar.md` | The aggregating portal — an unlisted page is not published in the navigation | +| Every content page is in `_Sidebar.md` and the `Home.md` table, and their links resolve | `DocumentationTests` | | Pages follow the four-section shape | Review | -| Fences declare a language | Review | +| Fences declare a language | `DocumentationTests` | | `README.md` and `AGENTS.md` point in rather than duplicate | Review | -The navigation is the only part a tool can check, and it is the part that matters most: a page nobody can -reach is a page that does not exist. +The navigation and the fences are the two conventions a machine can check, and they are the two that +break silently: a page nobody can reach is a page that does not exist, and a bare fence renders as an +untyped block. Both are enforced by `DocumentationTests` in `src/tests/Repository.UnitTests`. The rest +is a review obligation, and this page says so rather than implying otherwise. ## At your scale diff --git a/docs/wiki/Engineering-Principles.md b/docs/wiki/Engineering-Principles.md index b20e975..0ca0f46 100644 --- a/docs/wiki/Engineering-Principles.md +++ b/docs/wiki/Engineering-Principles.md @@ -1,162 +1,38 @@ # Engineering Principles -These are the rules the Purview repositories are held to, each with the failure it prevents and the -mechanism that makes it true. Most of them are not stated here for the first time: the authoritative -statement of the SDK's own behaviour is the [build-sdk wiki][build-sdk], and this page links to it rather -than paraphrasing it, because a paraphrase is a second source of truth that drifts. - -## 1. Convention over configuration - -**The rule.** A repository declares as little as possible. Defaults come from one MSBuild SDK, and a -project file is short because the SDK already knows what a project of that shape needs. - -**Why.** Configuration that is copied between repositories diverges. When the rule lives in one package, -fixing it once fixes it everywhere; when it lives in eleven project files, it is fixed eleven times or, -more often, once. - -**How it is enforced.** `` in a single -`Directory.Build.props`. The SDK detects project type from the filename and the `Sdk` attribute, and -applies the corresponding defaults. See [build-sdk: Project Type -Detection](https://github.com/purview-dev/build-sdk/blob/main/docs/wiki/Project-Type-Detection.md). - -**At your scale.** A single-project repository still benefits: the SDK import is two lines and removes -the target framework, language version, nullable context, analyzer set and package metadata from the -project file. - -## 2. Identity is derived, never repeated - -**The rule.** `RootNamespace` comes from `$(NamespacePrefix).$(ProjectName)`, and `AssemblyName` follows -the resolved `RootNamespace`. A name appears once, in the folder and file name, and everything else is -computed from it. - -**Why.** A namespace that is typed by hand drifts from the assembly it lives in, and the drift is -invisible until something reflects over types — an architecture test, a serializer, a DI scan. - -**How it is enforced.** The SDK derives both during `.props` evaluation, and a build-time check -(`PurviewProjectFileNameMismatch`) fails when a `.csproj` filename does not match its parent directory. - -**At your scale.** Always. This rule costs nothing and removes a class of bug. - -## 3. Architecture is a test, not a diagram - -**The rule.** The dependencies a layer is allowed to have are asserted by tests that read the compiled -assemblies. If a `ProjectReference` points the wrong way, the build fails. - -**Why.** A diagram is a claim about the past. Dependency direction erodes one convenient reference at a -time, and every such reference looks reasonable in isolation. - -**How it is enforced.** A test project that references every layer and inspects their metadata -references. See [Architecture as Tests](Architecture-As-Tests.md), and the running example in -`src/tests/Architecture.UnitTests`. - -**At your scale.** Small repositories can skip it. Adopt it the moment a second layer exists, because -that is when the wrong direction first becomes possible. - -## 4. The build is the only authority - -**The rule.** A convention that a build can check is checked by the build. A convention that only a -reviewer can check is labelled as a review obligation and never described as enforced. - -**Why.** A rule that claims to be enforced when it is not is worse than no rule: people rely on the -guarantee and stop looking. The distinction has to be visible in the documentation, not inferred. - -**How it is enforced.** Style is enforced by analyzers and `EnforceCodeStyleInBuild`; the SDK's own style -policy is re-validated on every compile, so a repository cannot quietly downgrade a rule. See -[build-sdk: SDK-Shipped -Analyzers](https://github.com/purview-dev/build-sdk/blob/main/docs/wiki/Analyzers.md). - -**At your scale.** Always. If a rule matters and cannot be checked, say so plainly. - -## 5. Warnings are not acceptable output - -**The rule.** A clean build has no warnings. A warning that will never be fixed is either fixed, or -suppressed where it occurs with a stated justification, or the rule is turned off deliberately. - -**Why.** Warnings are a budget. A build that emits twenty is a build where the twenty-first is invisible, -and the twenty-first is the one that mattered. - -**How it is enforced.** Partly by severity: the SDK's shipped `.editorconfig` sets many rules to `error`, -so the important ones cannot be ignored. The rest is a review obligation, backed by the pipeline running -`dotnet build` with the repository's configuration. - -**At your scale.** A small repository can tolerate `warning`; a large one cannot. Either way, decide -deliberately rather than by accumulation. - -## 6. Every claim about a consumer is proved by a consumer - -**The rule.** When a package documents how it must be used — a target framework, an asset, a guard, a -workaround — something in the repository builds that documented consumer and asserts the documented -outcome. - -**Why.** Consumer-facing documentation is the part of a library least likely to be exercised by its own -tests, because the library's own build is not a consumer. It drifts silently and fails in the consumer's -repository rather than ours. - -**How it is enforced.** A dedicated verification script that packs, then builds throwaway consumer -projects against the produced packages, asserting both the happy path and the documented guard errors. -[containers](https://github.com/purview-dev/containers) is the worked example: `just verify-consumers` -asserts every claim in its consumer-requirements page, and the script is deliberately excluded from the -default unit filter because it restores from nuget.org. - -**At your scale.** Only worth it once a package is consumed outside its own repository. Before that, the -repository's own tests already are the consumer. - -## 7. Documentation is a build artefact of the repository - -**The rule.** The wiki lives in the repository it describes, in `docs/wiki`, and the repository owns its -`mkdocs.yml`, its navigation and its `edit_uri`. - -**Why.** Documentation that lives elsewhere is updated elsewhere. A page with no owner and no review path -is wrong within one release. - -**How it is enforced.** Review, plus the navigation file: a page that is not in `_Sidebar.md` is not -published by the aggregating site. See [Documentation Convention](Documentation-Convention.md). - -**At your scale.** Always. Even a repository with one page should own that page. - -## 8. Everything a test needs is built by one builder - -**The rule.** Domain content in a test comes from `TestBuilder`. A test never constructs a domain object -inline, and it overrides only the value it is proving. - -**Why.** Inline construction encodes the shape of a type into every test that touches it, so one -constructor change becomes a repository-wide edit. It also hides which values the test actually depends -on, because every value looks deliberate. - -**How it is enforced.** Review. See [Testing and Test Data](Testing-And-Test-Data.md). - -**At your scale.** Adopt from the first test. Retrofitting it is a mechanical edit across every test file, -and it is the kind of edit that is postponed forever. - -## 9. An agent is a reader with no memory - -**The rule.** A repository states its purpose, its layout and its conventions in `AGENTS.md`, and ships -the same guidance the build uses. Nothing important is discoverable only by reading the code. - -**Why.** A coding agent has the repository and nothing else — no colleague to ask, no institutional -memory, no intuition about which of two plausible patterns is the intended one. The guidance that makes -the repository obvious to an agent is the same guidance that makes it obvious to a new joiner. - -**How it is enforced.** Review, plus the SDK publishing its own skills so the agent-facing -guidance tracks the SDK version rather than the repository's memory of it. See [Agent -Enablement](Agent-Enablement.md). - -**At your scale.** Always. `AGENTS.md` is cheaper than the questions it prevents. - -## 10. A change is verified in proportion to its risk - -**The rule.** Checks are tiered, and a repository declares which tiers its pipeline runs. A pull request -runs the fast tier; the slow tier is run deliberately, locally or on a dedicated runner. - -**Why.** A pipeline that runs everything on every pull request gets made optional. A pipeline that runs -too little gets ignored. The tiering is what keeps the gate credible. - -**How it is enforced.** The pipeline's `TestFilter`, and the test project's own name — the SDK -categorises a `*UnitTests` project as `Unit`, so the tier is a property of the project rather than of a -hand-written attribute. See [Verification Model](Verification-Model.md). - -**At your scale.** One tier is fine until the slow tests exist. Then split by project name, not by -attribute. +These are the ten rules the Purview repositories are held to. This page is an index: each rule is +stated once, in full, on the page named beside it. That page carries the failure the rule prevents, the +mechanism that makes it true, and what a smaller repository does instead — so a rule is never written +twice and the two copies cannot disagree. Where a rule has no dedicated page, the line here is its full +statement. + +The distinction that runs through all ten: **a convention a build can check is checked by the build; a +convention only a reviewer can check is labelled a review obligation and never described as enforced.** +A rule that claims enforcement it does not have is worse than no rule, because people stop looking. + +| # | Principle | In one line | Detail | +| -- | -- | -- | -- | +| 1 | Convention over configuration | A repository declares as little as possible; one MSBuild SDK supplies the defaults, so a project file is short. | [Repository Shape](Repository-Shape.md) | +| 2 | Identity is derived, never repeated | A name appears once, in the folder and file name; `RootNamespace` and `AssemblyName` are computed from it. | [Repository Shape](Repository-Shape.md) | +| 3 | Architecture is a test, not a diagram | The dependencies a layer may have are asserted by tests, so a wrong `ProjectReference` fails the build. | [Architecture as Tests](Architecture-As-Tests.md), [Project Responsibilities](Project-Responsibilities.md) | +| 4 | The build is the only authority | A convention a build can check is checked by the build; the rest is labelled a review obligation, never described as enforced. | [Documentation Convention](Documentation-Convention.md) | +| 5 | Warnings are not acceptable output | A clean build has no warnings: fix one, suppress it where it occurs with a justification, or turn the rule off deliberately. | [Testing and Test Data](Testing-And-Test-Data.md) | +| 6 | Every claim about a consumer is proved by a consumer | A package that documents how it must be used ships something that builds that documented consumer and asserts the documented outcome. | [Contracts and Data Exchange](Contracts-And-Data-Exchange.md) | +| 7 | Documentation is a build artefact of the repository | The wiki lives in the repository it describes, and the repository owns its `mkdocs.yml`, navigation and `edit_uri`. | [Documentation Convention](Documentation-Convention.md) | +| 8 | Everything a test needs is built by one builder | Domain content in a test comes from `TestBuilder`; a test overrides only the value it is proving. | [Testing and Test Data](Testing-And-Test-Data.md) | +| 9 | An agent is a reader with no memory | A repository states its purpose, layout and conventions in `AGENTS.md`; nothing important is discoverable only from the code. | [Agent Enablement](Agent-Enablement.md) | +| 10 | A change is verified in proportion to its risk | Checks are tiered; a pull request runs the fast tier, and the slow tier is run deliberately. | [Verification Model](Verification-Model.md) | + +Enforcement is stated per rule on the linked page. Three are enforced by the build in a way worth +knowing at a glance: identity (rule 2) by `PurviewProjectFileNameMismatch` and the SDK's derivation, +architecture (rule 3) by the architecture tests, and the verification tiers (rule 10) by the pipeline's +`TestFilter`. The rest are enforced partly by the build and partly by review, and each page says which. + +## At your scale + +Every rule has a smaller version, and taking one before the problem it prevents exists is pure cost. +[Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) decides which rules apply to a small, +medium or large repository, and in what order to adopt them. ## See also @@ -164,5 +40,4 @@ attribute. - [Architecture as Tests](Architecture-As-Tests.md) - [Verification Model](Verification-Model.md) - [Testing and Test Data](Testing-And-Test-Data.md) - -[build-sdk]: https://github.com/purview-dev/build-sdk +- [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) diff --git a/docs/wiki/Getting-Started.md b/docs/wiki/Getting-Started.md index 2a8636f..312c6c3 100644 --- a/docs/wiki/Getting-Started.md +++ b/docs/wiki/Getting-Started.md @@ -1,15 +1,25 @@ # Getting Started -This page builds the smallest repository that satisfies every rule in the playbook, in the order you -would actually create the files. Each step says what it buys you, so you can stop when you have taken -what you need — [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) is the short version of -that decision. +## The rule -The finished result is a repository whose defaults come from one MSBuild SDK, whose test projects are -wired by their filenames, whose architecture is asserted by a test, and whose documentation is a wiki -the build already knows how to find. +Build the smallest repository that satisfies every rule in the playbook, in the order you would +actually create the files. Each step says what it buys you, so you can stop when you have taken what +you need. The finished result is a repository whose defaults come from one MSBuild SDK, whose test +projects are wired by their filenames, whose architecture is asserted by a test, and whose +documentation is a wiki the build already knows how to find. -## Step 1 — Pin the SDK in `global.json` +## Why + +A convention adopted by copying somebody else's finished repository is a convention you have not read. +Building the repository in the order the rules depend on each other means every file has a reason by +the time you write it, and the build tells you immediately when a step is wrong. The order is not +arbitrary: the SDK import must come before anything that derives from it, the package versions must +exist before the first project restores, and the solution must exist before the pipeline can be pointed +at it. + +## The steps + +### Step 1 — Pin the SDK in `global.json` There are **two** versions in play here, and the file pins both: @@ -21,7 +31,7 @@ There are **two** versions in play here, and the file pins both: "allowPrerelease": false }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.4" + "Purview.BuildSdk": "1.0.6" }, "test": { "runner": "Microsoft.Testing.Platform" @@ -43,10 +53,13 @@ latest-wins, and the build that passes locally is not necessarily the build CI r `global.json` bootstrapping](https://github.com/purview-dev/build-sdk/blob/main/docs/wiki/Repository-Bootstrap.md#globaljson-bootstrapping). -## Step 2 — Declare the versions the SDK injects, in `Directory.Packages.props` +### Step 2 — Declare the versions the SDK injects, in `Directory.Packages.props` Central Package Management is on by default, so every `PackageReference` the SDK adds has to have a -matching `PackageVersion` or restore fails with `NU1010`: +matching `PackageVersion` or restore fails with `NU1010`. The versions below are the ones this +repository builds with today — check them against the SDK's current release before you commit, as +[`templates/README.md`](https://github.com/purview-dev/engineering-playbook/blob/main/templates/README.md) +says. ```xml @@ -59,10 +72,10 @@ matching `PackageVersion` or restore fails with `NU1010`: 10.10.0 - 5.0.1 + 5.0.4 - 1.72.4 + 1.75.0 @@ -73,7 +86,7 @@ matching `PackageVersion` or restore fails with `NU1010`: - + @@ -114,7 +127,7 @@ Two rules keep it from becoming indirection for its own sake: This is a convention, not something the build checks: nothing fails if you inline every version. It is on the reviewer, and it is cheap to keep because the file is read far more often than it is written. -## Step 3 — Import the SDK, and name the namespace prefix +### Step 3 — Import the SDK, and name the namespace prefix `src/Directory.Build.props`: @@ -146,7 +159,7 @@ declaring any of them. See [Repository Shape](Repository-Shape.md). > repositories put it at `src/`, which keeps the root for repository-level configuration only. The SDK's > own repository-root discovery still resolves correctly from there, because it asks `git`. -## Step 4 — Add the solution +### Step 4 — Add the solution `src/Acme.slnx`: @@ -174,7 +187,7 @@ declaring any of them. See [Repository Shape](Repository-Shape.md). The `Solution Items` folder is the list of files a reader of the solution needs and that a build would otherwise not surface. `.slnx` is the XML solution format; `.gitattributes` pins it to LF. -## Step 5 — Create a project with a two-line project file +### Step 5 — Create a project with a two-line project file `src/src/Domain/Domain.csproj`: @@ -220,7 +233,7 @@ Exchange](Contracts-And-Data-Exchange.md). > > ``` -## Step 6 — Add a test project with an even shorter project file +### Step 6 — Add a test project with an even shorter project file `src/tests/Domain.UnitTests/Domain.UnitTests.csproj`: @@ -252,7 +265,7 @@ public sealed class CustomerIdTests } ``` -## Step 7 — Add the shared testing project +### Step 7 — Add the shared testing project `src/tests/SharedTestingInfra/SharedTestingInfra.csproj`: @@ -268,7 +281,7 @@ The name is one of a recognised set, so the SDK treats it as a shared testing pr packages but not the runner, stays a library, and is referenced automatically by the test projects beside it. This is where `TestBuilder` lives — see [Testing and Test Data](Testing-And-Test-Data.md). -## Step 8 — Point the shared pipeline at the solution +### Step 8 — Point the shared pipeline at the solution `.config/purview-build.json`: @@ -279,7 +292,9 @@ beside it. This is where `TestBuilder` lives — see [Testing and Test Data](Tes "Solution": "src/Acme.slnx", "TestRoot": "src/tests", "TestPatterns": "*Tests.csproj", - "TestFilter": "/*/*/*/*[Category=Unit]" + "TestFilter": "/*/*/*/*[Category=Unit]", + "RunPack": false, + "ValidatePack": false }, "Release": { "Mode": "None" @@ -290,6 +305,10 @@ beside it. This is where `TestBuilder` lives — see [Testing and Test Data](Tes Paths are anchored to the **repository root**, not to the configuration file, so moving the file into `.config/` does not change what it points at. +`RunPack` and `ValidatePack` are off because this repository ships no package. The shared workflow +defaults both to `true`, so leaving them unset would give a green local build and a failing pipeline. +See [Verification Model](Verification-Model.md). + `TestPatterns` is left at its default, `*Tests.csproj`, on purpose. Discovery and tiering are two mechanisms, and only one of them should carry the tier: `TestPatterns` chooses which project files are discovered, `TestFilter` chooses which discovered tests run. Leave discovery wide and the day you add @@ -297,7 +316,7 @@ discovered, `TestFilter` chooses which discovered tests run. Leave discovery wid edit. Narrow `TestPatterns` and you have to remember both. See [Verification Model](Verification-Model.md). -## Step 9 — Add the caller workflow +### Step 9 — Add the caller workflow `.github/workflows/pr.yml`: @@ -318,14 +337,18 @@ jobs: name: Build and test uses: purview-dev/build/.github/workflows/purview-build.yml@main with: + run-pack: false + validate-pack: false test-filter: "/*/*/*/*[Category=Unit]" secrets: inherit ``` -The workflow is a caller; the pipeline itself lives in one shared repository. See [Verification -Model](Verification-Model.md) and [Release Flow](Release-Flow.md). +The two pack switches repeat `Build:RunPack` and `Build:ValidatePack` from the configuration, so the +workflow and the JSON agree about what a green build means. The workflow is a caller; the pipeline +itself lives in one shared repository. See [Verification Model](Verification-Model.md) and +[Release Flow](Release-Flow.md). -## Step 10 — Add the wiki +### Step 10 — Add the wiki `mkdocs.yml`: @@ -346,14 +369,14 @@ Then `docs/wiki/index.md`, `docs/wiki/Home.md` and `docs/wiki/_Sidebar.md`, and `_Sidebar.md` is the navigation list, so a new page that is not in it is a page nobody finds. See [Documentation Convention](Documentation-Convention.md). -## Step 11 — Make the repository legible to an agent +### Step 11 — Make the repository legible to an agent Add `AGENTS.md` at the root: purpose, layout table, conventions, completion checklist, local commands. Then install the SDK's own skills with `npx skills add purview-dev/build-sdk`, so an agent gets that guidance without you writing it. Installation is explicit - no build writes into an agent directory. See [Agent Enablement](Agent-Enablement.md). -## Step 12 — Add the local tools and the commands that run them +### Step 12 — Add the local tools and the commands that run them Step 13 runs `just`, and `just lint-check` runs CSharpier. Neither exists yet, so create both now. @@ -371,7 +394,7 @@ installed globally per machine: "rollForward": true }, "purview.build": { - "version": "0.5.2", + "version": "0.5.3", "commands": ["purview-build"], "rollForward": false } @@ -421,7 +444,7 @@ pipeline-pr *args: There is deliberately no `Justfile` in `templates/`: the recipes are the list of things *your* repository actually does, and a copied list of someone else's is noise. The five above are the floor, not the shape. -## Step 13 — Prove it +### Step 13 — Prove it ```text just build # dotnet build (Debug) @@ -431,10 +454,10 @@ just pipeline-pr # restore, build, lint, test through the shared Purview.Buil ``` Then break a rule on purpose — add a `ProjectReference` from the domain to the layer above it, or rename -a file so it no longer matches its folder — and confirm the build refuses. A convention you have not -watched fail is a convention you are guessing about. +a project file so it no longer matches its folder — and confirm the build refuses. A convention you have +not watched fail is a convention you are guessing about. -## Step 14 — Or copy the templates +### Step 14 — Or copy the templates Most of the files in steps 1–12 have a copy-paste version in [`templates/`](https://github.com/purview-dev/engineering-playbook/tree/main/templates), with `Acme` standing @@ -446,7 +469,10 @@ Four things in the steps above are deliberately **not** templated, because a cop on arrival: the solution file (its paths depend on your project list), the two `.csproj` files (they are empty by design), and the `Justfile` (see step 12). `templates/README.md` says which, and why. -## What you have now +## How it is enforced + +Step 13 proved it by breaking a rule on purpose. The table below is the same statement for the whole +repository: which concern lives where, and what refuses a change that breaks it. | Concern | Where it lives | Enforced by | | -- | -- | -- | @@ -456,9 +482,15 @@ empty by design), and the `Justfile` (see step 12). `templates/README.md` says w | Code style | Bootstrapped `.editorconfig` | The build, with the style policy validated | | Test wiring | The `.csproj` filename | The build | | Test data | `TestBuilder` in `SharedTestingInfra` | Review | -| Architecture | Dependency-direction tests | The build | +| Architecture | Dependency-direction tests + `.config/architecture-policy.json` | The build | | Verification tiers | `TestFilter` in `purview-build.json` | The pipeline | -| Documentation | `docs/wiki` + `_Sidebar.md` | Review | +| Documentation | `docs/wiki` + `_Sidebar.md` | The build (`DocumentationTests`) for navigation and fences; review for the rest | + +## At your scale + +This page is the floor, not the whole playbook. [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) +decides which parts to take for a small, medium or large repository, and [Worked +Examples](Worked-Examples.md) walks the reference implementation that results from taking all of them. ## See also diff --git a/docs/wiki/Home.md b/docs/wiki/Home.md index f86fca2..03c3591 100644 --- a/docs/wiki/Home.md +++ b/docs/wiki/Home.md @@ -11,7 +11,7 @@ scaled so that a repository adopting them can take the part that fits. ## How to read a page -Every page follows the same shape: +Every content page follows the same shape: | Section | Answers | | -- | -- | @@ -24,6 +24,10 @@ Rules a build can enforce are labelled as such. Rules only a reviewer can enforc review obligations. A convention that claims to be enforced when it is not is worse than no convention at all, so the two are never blurred. +This page and [`index.md`](index.md) are navigation rather than rules, and +[Engineering Principles](Engineering-Principles.md) is an index that links to the page stating each +rule in full. + ## The playbook | Page | Covers | diff --git a/docs/wiki/Project-Responsibilities.md b/docs/wiki/Project-Responsibilities.md index 985bf70..3b6fe25 100644 --- a/docs/wiki/Project-Responsibilities.md +++ b/docs/wiki/Project-Responsibilities.md @@ -17,7 +17,8 @@ it has one project carrying three responsibilities, which is usually correct. Th what each responsibility *is* so that you can tell when splitting one out would buy something. The authoritative definition is [`.config/architecture-policy.json`](https://github.com/purview-dev/engineering-playbook/blob/main/.config/architecture-policy.json), -described by [`architecture-policy.schema.json`](https://github.com/purview-dev/engineering-playbook/blob/main/.config/architecture-policy.schema.json). +described by the JSON Schema the SDK publishes at +[`architecture-policy.schema.json`](https://github.com/purview-dev/build-sdk/blob/main/architecture-policy.schema.json). The tables on this page are checked against it — see [How it is enforced](#how-it-is-enforced). ## Why @@ -306,37 +307,48 @@ flowchart TD | Rule | Enforced by | | -- | -- | -| Every production project maps to a responsibility or a recorded waiver | `ProjectDependencyPolicyTests` | -| Declared project references respect the dependency matrix | `ProjectDependencyPolicyTests` | -| Sibling implementations stay independent | `ProjectDependencyPolicyTests` | -| Nothing depends on Composition except hosts | `ProjectDependencyPolicyTests` | -| Forbidden framework packages stay out of a responsibility | `ProjectDependencyPolicyTests` | -| The project graph has no cycles | `ProjectDependencyPolicyTests` | -| Catch-all project names are rejected | `ProjectDependencyPolicyTests` | -| Production code does not reference a test project | `ProjectDependencyPolicyTests` | -| Every waiver names a real project, a rule, and a substantive reason | `ProjectDependencyPolicyTests` | +| Every production project maps to a responsibility or a recorded waiver | `ArchitectureTests` | +| Declared project references respect the dependency matrix | `ArchitectureTests` | +| Sibling implementations stay independent | `ArchitectureTests` | +| Nothing depends on Composition except hosts | `ArchitectureTests` | +| Forbidden framework packages stay out of a responsibility | `ArchitectureTests` | +| The project graph has no cycles | `ArchitectureTests` | +| Catch-all project names are rejected | `ArchitectureTests` | +| Production code does not reference a test project | `ArchitectureTests` | +| Every waiver names a real project, a rule, and a substantive reason | `ArchitectureTests` | | This page's tables match the policy file | `PolicyDocumentationTests` | -| The policy matches its JSON Schema, and the schema can actually reject | `PolicySchemaTests` | +| The policy matches its JSON Schema, and the schema can actually reject | `PurviewArchitectureTests`, `PolicySchemaTests` | | A responsibility is split only when it is justified | **Review** | -The tests read `.config/architecture-policy.json`. Nothing on this page is a second source of truth — -a consistency test fails if the tables drift from the policy. +The rules themselves are not written here: they ship in **`Purview.Architecture.Testing`**, and +`ArchitectureTests` derives from the package's `PurviewArchitectureTests` base class, so the matrix, +naming, sibling-independence, composition, forbidden-package, cycle, test-reference and waiver rules +are defined once and shared. The tests read `.config/architecture-policy.json`. Nothing on this page +is a second source of truth — a consistency test fails if the tables drift from the policy. ### The policy format -`.config/architecture-policy.json` is described by a JSON Schema (draft 2020-12) published beside it, +`.config/architecture-policy.json` is described by a JSON Schema (draft 2020-12) that the SDK +publishes at +[`architecture-policy.schema.json`](https://github.com/purview-dev/build-sdk/blob/main/architecture-policy.schema.json), and the file points at it so an editor validates as you type: ```json { - "$schema": "https://raw.githubusercontent.com/purview-dev/engineering-playbook/main/.config/architecture-policy.schema.json", - "version": 1 + "$schema": "https://raw.githubusercontent.com/purview-dev/build-sdk/main/architecture-policy.schema.json", + "version": 1, + "sourceRoots": ["src"] } ``` +The schema lives in the SDK's repository rather than in a copy beside the policy, because a second +copy is a second source of truth and the two drift. It is also embedded in +`Purview.Architecture.Testing`, so validation needs no network and cannot disagree with the reader. + | Key | Purpose | | -- | -- | | `version` | Format version. Increment when a change would break a reader that understood the previous shape. | +| `sourceRoots` | Repo-relative folders holding the projects; defaults to `["src"]`. An empty array scans the repository root. A wrong value finds no projects, which the graph-discovery rule reports rather than letting every other rule pass vacuously. | | `responsibilities[]` | `name`, `aliases`, `category`, `purpose`, `mayDependOn`, `mustNotDependOn`, `forbiddenPackagePrefixes`, `siblingIndependence`, and the split/combine guidance | | `discouragedProjectNames` | Category names rejected as project names, with the reason | | `rules` | Repository-wide switches not expressed per responsibility | @@ -350,13 +362,10 @@ Two schema choices do real work: - **`minLength` on a waiver's `reason`.** A waiver with a token justification is a suppression wearing a reason's clothes. -The schema is enforced, not decorative: `PolicySchemaTests` validates the policy against it, asserts -the two files agree on the published `$id`, and — importantly — asserts the schema *rejects* a -document missing a required field, a waiver with a token reason, and a misspelled key. A schema that -cannot fail proves nothing. - -Validation uses `JsonSchema.Net`, the same library `Purview.Build` uses for `purview-build.json`, so -the ecosystem has one JSON Schema implementation rather than two that disagree at the edges. +The schema is enforced, not decorative: `PurviewArchitectureTests` validates the policy against the +embedded schema on every run, and `PolicySchemaTests` asserts it *rejects* a document missing a +required field, a waiver with a token reason, and a misspelled key. A schema that cannot fail proves +nothing. ### Adopting this in another repository @@ -370,6 +379,7 @@ validates it as you type: { "$schema": "https://raw.githubusercontent.com/purview-dev/build-sdk/main/architecture-policy.schema.json", "version": 1, + "sourceRoots": ["src"], "responsibilities": [ ... ] } ``` @@ -396,9 +406,10 @@ Two optional overrides: | `RepositoryRoot` | The policy does not sit above the test binary — the default probes upward for `.config/architecture-policy.json` | | `SkipRule(rule)` | A rule the repository has not adopted yet. Prefer a waiver: it is per project, carries a reason, and is itself checked | -For bespoke rules, use the engine directly rather than the base class — -`ArchitecturePolicyEvaluator.ForRepository()` returns typed `ArchitectureViolation` lists per rule, -and takes an in-memory `ProjectNode` graph so a rule can be tested without a repository on disk. +For bespoke rules, use the package's engine directly rather than the base class — +`ArchitecturePolicyEvaluator` returns typed `ArchitectureViolation` lists per rule, and takes an +in-memory `ProjectNode` graph so a rule can be tested without a repository on disk. The package's own +skill, `purview-buildsdk-architecture-tests`, is authoritative on that API and on the rule ids. ### Waivers diff --git a/docs/wiki/Repository-Shape.md b/docs/wiki/Repository-Shape.md index cd1ba13..c791939 100644 --- a/docs/wiki/Repository-Shape.md +++ b/docs/wiki/Repository-Shape.md @@ -8,7 +8,7 @@ where something lives without being told. ```text Repository/ -├── .config/ dotnet-tools.json, lefthook.yml, purview-build.json +├── .config/ dotnet-tools.json, lefthook.yml, purview-build.json, architecture-policy.json ├── .github/ │ ├── workflows/ pr.yml, release.yml, any repository-specific workflow │ └── copilot-instructions.md defers to AGENTS.md @@ -50,6 +50,7 @@ knowing before you follow it expecting a finished repository: | -- | -- | | `.editorconfig` | Written by the SDK on the first build | | `.gitignore`, `.gitattributes` | The SDK's own `templates/` folder, so they stay in step with it | +| `.config/architecture-policy.json` | Optional; adopted when the repository wants its architecture policy enforced — see [Project Responsibilities](Project-Responsibilities.md) | | `commitlint.config.mts`, `nuget.config`, `LICENSE.md`, `package.json` | Repository-level choices; not covered by Getting Started | | Everything else | Getting Started steps 1–12, or [`templates/`](https://github.com/purview-dev/engineering-playbook/tree/main/templates) | diff --git a/docs/wiki/Worked-Examples.md b/docs/wiki/Worked-Examples.md index 4d0bf2e..ca37f46 100644 --- a/docs/wiki/Worked-Examples.md +++ b/docs/wiki/Worked-Examples.md @@ -1,13 +1,23 @@ # Worked Examples +## The rule + `src/` in this repository is a deliberately small layered repository that is built, linted and tested by the same pipeline every Purview repository uses. It exists so that the rules in this playbook are not just prose: the dependency-direction rules fail the build when they are broken, and the test-data conventions are shown in the form they are meant to be copied. -It is small on purpose. There is one aggregate, one port, one adapter and three test projects — enough to +It is small on purpose. There is one aggregate, one port, one adapter and four test projects — enough to show every mechanism, and little enough that you can read all of it in ten minutes. +## Why + +A rule you have not watched fail is a rule you are guessing about. Prose can be read and agreed with; a +reference implementation can be built, run, and broken on purpose to see the failure. It is also the one +place this playbook can prove that its own conventions compile and pass — the pipeline that runs here is the +pipeline every Purview repository uses, so a convention that cannot survive its own reference implementation +is a convention that will not survive contact with a real one. + ## The shape ```text @@ -29,7 +39,8 @@ src/ ├── SharedTestingInfra/ TestBuilder ├── Domain.UnitTests/ value-object behaviour, direct construction ├── Application.UnitTests/ the service, against a TUnit.Mocks substitute - └── Architecture.UnitTests/ the dependency-direction rules + ├── Architecture.UnitTests/ the dependency-direction rules, and the architecture policy + └── Repository.UnitTests/ the repository itself: its skills and its wiki ``` ## The one architectural decision @@ -174,7 +185,7 @@ interface is explicitly `public` because `public` is not the default there. ## The tests -Three projects, each demonstrating a different thing. +Four projects, each demonstrating a different thing. **`Domain.UnitTests`** is the value-object exception to the "build everything through `TestBuilder`" rule. A test that proves what `Create` rejects has to name the rejected input itself: @@ -227,9 +238,19 @@ public async Task RegisterAsync_GivenAnAlreadyRegisteredConvention_DoesNotStoreI } ``` -**`Architecture.UnitTests`** is the rules from [Architecture as Tests](Architecture-As-Tests.md), running -against the three assemblies above. It is the reason the `Application` project file carries a comment -explaining that a reference to `Infrastructure` would fail the build. +**`Architecture.UnitTests`** runs two kinds of rule. `DependencyDirectionTests` is the four rules from +[Architecture as Tests](Architecture-As-Tests.md), reading the compiled assemblies; it is the reason the +`Application` project file carries a comment explaining that a reference to `Infrastructure` would fail +the build. `ArchitectureTests` derives from `PurviewArchitectureTests` and enforces the policy in +`.config/architecture-policy.json`, so the dependency matrix, naming, sibling-independence, composition, +forbidden-package, cycle, test-reference and waiver rules come from `Purview.Architecture.Testing` +rather than from C# written here. `PolicyDocumentationTests` keeps the wiki's tables in step with that +policy. + +**`Repository.UnitTests`** turns the repository itself into the subject. `SkillTests` derives from the +skill-conformance rule set the SDK ships, and `DocumentationTests` asserts that every wiki page is in +the navigation and that every fence declares a language. A convention that has to hold across the whole +repository, and that a reviewer would otherwise have to remember, belongs in a test here. ## The test data builder @@ -253,7 +274,10 @@ public static Domain.Convention Convention( An omitted argument is generated; a supplied argument is used exactly. A test therefore reads as a list of the values it actually cares about, and everything else is realistic without the test saying so. -## Running it +## How it is enforced + +The reference implementation is built, linted and tested by the same pipeline every Purview repository +uses, so a broken convention fails here first. ```text just build # dotnet build (Debug) @@ -270,23 +294,23 @@ Build succeeded. 0 Error(s) Test run summary: Passed! - total: 37 + total: 71 failed: 0 - succeeded: 37 + succeeded: 71 ``` The zero-warning result is worth noticing, because it took work: the telemetry source generator's public types, the SDK's `var` preference, and the flat-factory constraint all surfaced as build diagnostics rather than as documentation. Each is written down on the page where it belongs. -## Break it on purpose +### Break it on purpose The fastest way to understand a rule is to watch it fail. Three edits, each of which should fail the build: 1. **Add `` to `Application.csproj`.** `Application_GivenItsReferences_DependsOnTheDomainAndNotOnInfrastructure` fails. -2. **Rename `src/src/Domain/Convention.cs` to `ConventionAggregate.cs`.** `PurviewProjectFileNameMismatch` - fails, because the filename no longer matches its directory. +2. **Rename `src/src/Domain/Domain.csproj` to `DomainAggregate.csproj`.** `PurviewProjectFileNameMismatch` + fails, because the project filename no longer matches its directory. 3. **Move a `Convention` type into a namespace outside the repository's prefix, such as `Acme.Elsewhere`.** The namespace-identity rule fails and names the type. @@ -302,6 +326,13 @@ The fastest way to understand a rule is to watch it fail. Three edits, each of w would be a worse copy. - **No integration tier.** Every test is deterministic, so the pipeline needs no second job. +## At your scale + +This is the **Medium** profile: a few layers, a shared testing project, and architecture tests. A small +repository takes the SDK import, `Directory.Packages.props`, `AGENTS.md` and `TestBuilder` and stops there; +a large one adds pack validation, consumer verification and a release gate. [Adopting This at Your +Scale](Adopting-This-At-Your-Scale.md) is the page that decides. + ## See also - [Getting Started](Getting-Started.md) diff --git a/skills/purview-architecture-testing/SKILL.md b/skills/purview-architecture-testing/SKILL.md index 9fb3f11..de41062 100644 --- a/skills/purview-architecture-testing/SKILL.md +++ b/skills/purview-architecture-testing/SKILL.md @@ -1,14 +1,14 @@ --- name: purview-architecture-testing description: >- - Use when adding, changing or debugging architecture tests in a .NET solution — dependency-direction - rules, forbidden framework references, project naming checks, cycle detection — or when a boundary - keeps being broken in review and should fail the build instead. Writes TUnit tests driven by a - single policy definition rather than hand-maintained lists. + Use when adding, changing or debugging architecture tests in a .NET solution - dependency-direction + rules, forbidden framework references, project naming checks, cycle detection - or when a boundary + keeps being broken in review and should fail the build instead. Adopts Purview.Architecture.Testing + and a single policy file rather than hand-maintained lists. license: MIT metadata: author: Purview-Dev - version: "1.0" + version: "2.0" --- # Purview architecture testing @@ -26,28 +26,37 @@ Before writing anything, answer: **has this rule been broken, or is it certain t A test for a dependency nobody would plausibly add is cost with no benefit. Good candidates: - A direction that has already been violated once. -- A boundary that is load-bearing — Core staying free of EF Core, Composition being referenced by +- A boundary that is load-bearing - Core staying free of EF Core, Composition being referenced by nothing. - A rule a newcomer cannot infer from the code. Bad candidates: anything the compiler already enforces, and style preferences dressed up as architecture. -## 2. Define the policy once +## 2. Adopt the package; define the policy once -The failure mode of architecture tests is a dependency matrix written twice — once in a wiki table -and once in assertions — which then disagree. The prose is the copy that drifts, because nothing -fails when it is wrong. +The failure mode of architecture tests is a dependency matrix written twice - once in a wiki table and +once in assertions - which then disagree. The prose is the copy that drifts, because nothing fails +when it is wrong. -So: put the matrix in **one** machine-readable place and have the tests read it. A JSON file beside -the solution is enough; it needs no framework. +So do not write the rule engine. `Purview.Architecture.Testing` ships it, so the matrix, naming, +sibling-independence, composition, forbidden-package, cycle, test-reference and waiver rules are +defined once and shared. Adoption is two files. + +**A policy** at `.config/architecture-policy.json`, pointing at the published schema so an editor +validates it as you type: ```json { + "$schema": "https://raw.githubusercontent.com/purview-dev/build-sdk/main/architecture-policy.schema.json", + "version": 1, + "sourceRoots": ["src"], "responsibilities": [ { "name": "Core", "aliases": ["Domain"], + "category": "domain", + "purpose": "The domain model, its invariants and its rules.", "mayDependOn": ["Shared"], "mustNotDependOn": ["Application", "Persistence", "Composition"], "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Microsoft.AspNetCore"] @@ -57,6 +66,21 @@ the solution is enough; it needs no framework. } ``` +**One derived test class:** + +```csharp +[InheritsTests] +public sealed class ArchitectureTests : PurviewArchitectureTests; +``` + +That is the whole of the adoption. Each rule is a separate test, so a failure names the rule rather +than collapsing the set into one assertion, and the package validates the policy against its schema +first - a policy that does not match the schema may be silently missing a rule. + +The build-sdk ships a skill for the package itself, `purview-buildsdk-architecture-tests`, which is +authoritative on its API and rule ids. Read it rather than this page when you are extending or +debugging the package. + Then add a test that the documentation matches the policy, so the table in the wiki is falsifiable too. Full worked example: [Project Responsibilities](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Project-Responsibilities.md). @@ -70,10 +94,14 @@ Two sources, because they catch different things: | `.csproj` `ProjectReference` | The reference a reviewer actually added | A dependency arriving transitively | | Compiled assembly references | A dependency that arrived indirectly | Intent — it cannot tell direct from transitive | -Read the project files for the declared-intent rules (the matrix, naming, cycles) and the assemblies -for leak detection. Classify a project by the **first segment** of its name, so -`Persistence.SqlServer` and `Persistence` are both Persistence — qualification is a naming choice, -never a different rule. +The package's rules read the **project files**, because a declared `ProjectReference` is what a +reviewer adds and what the policy governs. Add a small hand-written test that reads the **compiled +assemblies** for the leak a project file cannot show - a namespace from the wrong layer reaching a +public type. + +Classify a project by the **first segment** of its name, so `Persistence.SqlServer` and `Persistence` +are both Persistence - qualification is a naming choice, never a different rule. The package does +this for you; a hand-written assembly test must do it too. ## 4. The rules worth having @@ -87,6 +115,8 @@ never a different rule. | Catch-all names | `Infrastructure`, `Services`, `Helpers` appearing as project names | | No production → test reference | A test helper leaking into shipped code | +The package provides all of these. Do not re-implement one; change the policy instead. + ## 5. Write failures someone can act on This is what decides whether the suite survives. A failure must name: @@ -103,13 +133,14 @@ must not depend on Persistence. Invert it: Persistence may depend on Application an abstraction that Application owns and Persistence implements. ``` -Not `Expected collection to be empty`. Collect every violation and report them as a numbered list — -one test run should show all of them, not the first. +Not `Expected collection to be empty`. Collect every violation and report them as a numbered list - +one test run should show all of them, not the first. The package already reports this way; keep the +same standard in any rule you add. ## 6. Waivers, not suppressions A repository that predates the policy will violate it. Deleting the test or adding a blanket -`#pragma` makes the policy a lie. Record the departure instead: +`#pragma` makes the policy a lie. Record the departure in the policy instead: ```json { @@ -121,29 +152,32 @@ A repository that predates the policy will violate it. Deleting the test or addi } ``` -Then test the waivers themselves: each must name a project that still exists, a rule, and a +The package tests the waivers themselves: each must name a project that still exists, a rule, and a substantive reason. A waiver for a deleted project is dead weight that hides the next one. -Critically, a waiver should excuse **the narrowest thing possible** — usually the name. `mapsTo` -keeps the project classified so every dependency rule still applies to it. +Critically, a waiver excuses **the narrowest thing possible** - usually the name. `mapsTo` keeps the +project classified so every dependency rule still applies to it. Prefer a waiver to +`SkipRule`, which removes a rule for the whole repository with no record. ## 7. Conventions -- TUnit, matching the repository's other tests. No separate architecture-testing framework or NuGet - package unless reuse across repositories actually justifies one. +- TUnit, matching the repository's other tests, adopted through `PurviewArchitectureTests` with + `[InheritsTests]`. - Arrange/Act/Assert, one behaviour per test. -- Find the repository root by probing upward for a marker file — never by counting `../` from the - test binary, which differs between a local build and CI. -- Keep the policy reader free of MSBuild and test-framework types so it can be exercised directly. +- Let the package find the repository root - it probes upward for the policy and stops at the `.git` + boundary. Override `RepositoryRoot` only when the policy does not sit above the test binary. +- Keep the policy file free of anything a rule cannot read; a field no rule consults is a rule that + looks enforced and is not. ## Checks before you answer - Is every rule you added one that has been, or will be, broken? -- Is the matrix defined once, with the documentation checked against it? +- Is the matrix defined once, in the policy, with the documentation checked against it? - Does each failure name the fix, not just the fact? - Did you record a waiver rather than weaken a rule for existing code? ## See also - `purview-architecture` — what the projects are for, and how they are named +- `purview-buildsdk-architecture-tests` — the package's own skill, authoritative on its API - [Architecture as Tests](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Architecture-As-Tests.md) diff --git a/src/src/Infrastructure/InMemoryConventionRepository.cs b/src/src/Infrastructure/InMemoryConventionRepository.cs index 1666318..c98736e 100644 --- a/src/src/Infrastructure/InMemoryConventionRepository.cs +++ b/src/src/Infrastructure/InMemoryConventionRepository.cs @@ -1,6 +1,6 @@ +using System.Collections.Concurrent; using Purview.EngineeringPlaybook.Application; using Purview.EngineeringPlaybook.Domain; -using System.Collections.Concurrent; namespace Purview.EngineeringPlaybook.Infrastructure; diff --git a/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj b/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj index 49b8259..f5c22b7 100644 --- a/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj +++ b/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj @@ -10,8 +10,12 @@ - + - + diff --git a/src/tests/Architecture.UnitTests/ArchitectureTests.cs b/src/tests/Architecture.UnitTests/ArchitectureTests.cs new file mode 100644 index 0000000..8fae65a --- /dev/null +++ b/src/tests/Architecture.UnitTests/ArchitectureTests.cs @@ -0,0 +1,27 @@ +using Purview.Architecture.Testing; + +namespace Purview.EngineeringPlaybook.Architecture; + +/// +/// Enforces the repository's architecture policy as tests. +/// +/// +/// +/// The rule set lives in Purview.Architecture.Testing, so the dependency matrix, naming, +/// sibling-independence, composition, forbidden-package, cycle, test-reference and waiver rules are +/// defined once rather than copied here. This class is the whole of the adoption: the base class +/// reads .config/architecture-policy.json and derives every rule from it. +/// +/// +/// [InheritsTests] is load-bearing. TUnit does not walk base types when it discovers +/// tests, so without it this class compiles, the run is green and nothing is enforced. TUnit's +/// analyzer reports TUnit0030; treat that as an error rather than a suggestion. +/// +/// +/// stays hand-written and separate: it reads compiled assembly +/// metadata, which catches a dependency that arrived indirectly and which the policy graph - built +/// from declared ProjectReferences - cannot see. See docs/wiki/Architecture-As-Tests.md. +/// +/// +[InheritsTests] +public sealed class ArchitectureTests : PurviewArchitectureTests; diff --git a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs deleted file mode 100644 index 5cff6b9..0000000 --- a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs +++ /dev/null @@ -1,196 +0,0 @@ -using System.Text.Json; -using System.Text.Json.Serialization; - -namespace Purview.EngineeringPlaybook.Architecture.Policy; - -/// -/// The dependency and naming policy, read from .config/architecture-policy.json. -/// -/// -/// The policy file is the single definition of each rule. The tests read it, and -/// PolicyDocumentationTests asserts the wiki agrees with it, so a rule cannot be true in prose -/// and false in code. See docs/wiki/Project-Responsibilities.md. -/// -public sealed class ArchitecturePolicy -{ - static readonly JsonSerializerOptions SerializerOptions = new() - { - PropertyNameCaseInsensitive = true, - ReadCommentHandling = JsonCommentHandling.Skip, - }; - - [JsonPropertyName("version")] - public int Version { get; init; } - - [JsonPropertyName("responsibilities")] - public IReadOnlyList Responsibilities { get; init; } = []; - - [JsonPropertyName("discouragedProjectNames")] - public DiscouragedNames DiscouragedProjectNames { get; init; } = new(); - - [JsonPropertyName("rules")] - public PolicyRules Rules { get; init; } = new(); - - [JsonPropertyName("waivers")] - public IReadOnlyList Waivers { get; init; } = []; - - /// Finds the documented waiver for a project, if one exists. - /// The project name. - /// The waiver, or . - public PolicyWaiver? WaiverFor(string projectName) => - Waivers.FirstOrDefault(waiver => - string.Equals(waiver.Project, projectName, StringComparison.OrdinalIgnoreCase) - ); - - /// Reads the policy from the repository's configuration folder. - /// The policy. - public static ArchitecturePolicy Load() - { - var path = Path.Combine(RepositoryRoot(), ".config", "architecture-policy.json"); - if (!File.Exists(path)) - throw new FileNotFoundException($"The architecture policy was not found at '{path}'.", path); - - // The policy is a single source of truth, so deserialising to null is a failure. - return JsonSerializer.Deserialize(File.ReadAllText(path), SerializerOptions) - ?? throw new InvalidOperationException($"The architecture policy at '{path}' deserialised to null."); - } - - /// Finds the repository root by walking up to the folder holding the policy. - /// The repository root path. - /// - /// The test binary runs from bin/, and the depth of that path differs between a local build - /// and CI, so the root is found by probing for a marker rather than by counting parents. - /// - public static string RepositoryRoot() - { - DirectoryInfo? directory = new(AppContext.BaseDirectory); - while (directory is not null) - { - if (File.Exists(Path.Combine(directory.FullName, ".config", "architecture-policy.json"))) - return directory.FullName; - - directory = directory.Parent; - } - - throw new InvalidOperationException( - $"Could not locate the repository root above '{AppContext.BaseDirectory}'. Expected a '.config/architecture-policy.json'." - ); - } - - /// Finds the responsibility a project name maps to, honouring aliases. - /// The project name, which may be implementation-qualified. - /// The responsibility, or when the name is not recognised. - /// - /// A project name is <Responsibility>[.<Implementation>], so only the first - /// segment is classified: Persistence.SqlServer and Persistence are both Persistence. - /// Qualification is a naming choice, never a different set of rules. - /// - public Responsibility? Classify(string projectName) - { - // A documented exception maps an established project onto the responsibility it actually has, - // so the dependency rules still apply in full - only the name is excused. - var waiver = WaiverFor(projectName); - if (waiver is not null && !string.IsNullOrEmpty(waiver.MapsTo)) - { - return Responsibilities.FirstOrDefault(responsibility => - string.Equals(responsibility.Name, waiver.MapsTo, StringComparison.OrdinalIgnoreCase) - ); - } - - var segment = projectName.Split('.')[0]; - - return Responsibilities.FirstOrDefault(responsibility => - string.Equals(responsibility.Name, segment, StringComparison.OrdinalIgnoreCase) - || responsibility.Aliases.Any(alias => string.Equals(alias, segment, StringComparison.OrdinalIgnoreCase)) - ); - } -} - -/// A single architectural responsibility and the dependencies it permits. -public sealed class Responsibility -{ - [JsonPropertyName("name")] - public string Name { get; init; } = string.Empty; - - [JsonPropertyName("aliases")] - public IReadOnlyList Aliases { get; init; } = []; - - [JsonPropertyName("category")] - public string Category { get; init; } = string.Empty; - - [JsonPropertyName("purpose")] - public string Purpose { get; init; } = string.Empty; - - [JsonPropertyName("mayDependOn")] - public IReadOnlyList MayDependOn { get; init; } = []; - - [JsonPropertyName("mustNotDependOn")] - public IReadOnlyList MustNotDependOn { get; init; } = []; - - [JsonPropertyName("forbiddenPackagePrefixes")] - public IReadOnlyList ForbiddenPackagePrefixes { get; init; } = []; - - [JsonPropertyName("siblingIndependence")] - public bool SiblingIndependence { get; init; } - - [JsonPropertyName("separateProjectWhen")] - public string SeparateProjectWhen { get; init; } = string.Empty; - - [JsonPropertyName("mayRemainCombinedWhen")] - public string MayRemainCombinedWhen { get; init; } = string.Empty; -} - -/// Project names that describe a category rather than a responsibility. -public sealed class DiscouragedNames -{ - [JsonPropertyName("names")] - public IReadOnlyList Names { get; init; } = []; - - [JsonPropertyName("reason")] - public string Reason { get; init; } = string.Empty; -} - -/// A deliberate, reviewed departure from the policy for one project. -/// -/// An exception is how a rule stays enforceable in a repository that predates it. Suppressing a rule -/// silently would make the policy a lie; recording it here keeps the departure reviewable and keeps -/// every other rule in force for that project. -/// -public sealed class PolicyWaiver -{ - [JsonPropertyName("project")] - public string Project { get; init; } = string.Empty; - - [JsonPropertyName("mapsTo")] - public string MapsTo { get; init; } = string.Empty; - - [JsonPropertyName("rules")] - public IReadOnlyList Rules { get; init; } = []; - - [JsonPropertyName("reason")] - public string Reason { get; init; } = string.Empty; - - [JsonPropertyName("reviewedOn")] - public string ReviewedOn { get; init; } = string.Empty; - - /// Reports whether this exception excuses a named rule. - /// The rule identifier. - /// when the rule is excused. - public bool Excuses(string rule) => Rules.Contains(rule, StringComparer.OrdinalIgnoreCase); -} - -/// Repository-wide rules that are not expressed per responsibility. -public sealed class PolicyRules -{ - [JsonPropertyName("noCycles")] - public bool NoCycles { get; init; } - - [JsonPropertyName("noProductionToTestReferences")] - public bool NoProductionToTestReferences { get; init; } - - [JsonPropertyName("implementationsMustNotReferenceComposition")] - public bool ImplementationsMustNotReferenceComposition { get; init; } - - [JsonPropertyName("siblingImplementationsIndependent")] - public bool SiblingImplementationsIndependent { get; init; } -} diff --git a/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs b/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs deleted file mode 100644 index c858153..0000000 --- a/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs +++ /dev/null @@ -1,148 +0,0 @@ -using System.Xml.Linq; - -namespace Purview.EngineeringPlaybook.Architecture.Policy; - -/// A project in the solution, with the references it declares. -/// The project name, without extension. -/// The absolute path to the project file. -/// The names of the projects it references directly. -/// The package identifiers it references directly. -/// Whether the project is a test project. -public sealed record ProjectNode( - string Name, - string Path, - IReadOnlyList ProjectReferences, - IReadOnlyList PackageReferences, - bool IsTestProject -); - -/// -/// Reads the solution's projects and their declared references straight from the project files. -/// -/// -/// Project files are read rather than compiled assemblies because a declared -/// ProjectReference is the thing a reviewer adds and the thing the policy governs. Compiled -/// assembly references are checked separately - they catch a dependency that arrived indirectly, -/// which the project file does not show. -/// -public static class ProjectGraph -{ - /// Loads every project beneath the repository's source folder. - /// The repository root. - /// The projects, keyed by name. - public static IReadOnlyDictionary Load(string repositoryRoot) - { - var sourceRoot = Path.Combine(repositoryRoot, "src"); - Dictionary projects = new(StringComparer.OrdinalIgnoreCase); - - if (!Directory.Exists(sourceRoot)) - return projects; - - foreach (var projectPath in Directory.EnumerateFiles(sourceRoot, "*.csproj", SearchOption.AllDirectories)) - { - // bin/ and obj/ can hold copies of project files during a build; they are not the source. - if ( - projectPath.Contains( - $"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", - StringComparison.Ordinal - ) - || projectPath.Contains( - $"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", - StringComparison.Ordinal - ) - ) - { - continue; - } - - var node = Read(projectPath); - projects[node.Name] = node; - } - - return projects; - } - - /// Reads a single project file. - /// The project file path. - /// The project node. - static ProjectNode Read(string projectPath) - { - var document = XDocument.Load(projectPath); - var name = Path.GetFileNameWithoutExtension(projectPath); - - var projectReferences = document - .Descendants("ProjectReference") - .Select(element => (string?)element.Attribute("Include")) - .Where(include => !string.IsNullOrWhiteSpace(include)) - .Select(include => Path.GetFileNameWithoutExtension(include!.Replace('\\', Path.DirectorySeparatorChar))) - .Where(reference => !string.IsNullOrWhiteSpace(reference)) - .Distinct(StringComparer.OrdinalIgnoreCase) - .ToArray(); - - var packageReferences = document - .Descendants("PackageReference") - .Select(element => (string?)element.Attribute("Include") ?? (string?)element.Attribute("Update")) - .Where(include => !string.IsNullOrWhiteSpace(include)) - .Select(include => include!) - .Distinct(StringComparer.OrdinalIgnoreCase) - .ToArray(); - - // The SDK derives IsTestProject from the folder, so the path is the reliable signal here. - var isTestProject = - name.EndsWith("Tests", StringComparison.OrdinalIgnoreCase) - || projectPath.Contains( - $"{Path.DirectorySeparatorChar}tests{Path.DirectorySeparatorChar}", - StringComparison.OrdinalIgnoreCase - ); - - return new ProjectNode(name, projectPath, projectReferences, packageReferences, isTestProject); - } - - /// Finds every dependency cycle in the project graph. - /// The projects to inspect. - /// Each cycle, rendered as the path that closes it. - /// - /// MSBuild itself rejects a cycle, so this is a guard that reports the cycle readably rather than - /// as a build error naming two projects. It also covers a graph assembled in a test fixture. - /// - public static IReadOnlyList FindCycles(IReadOnlyDictionary projects) - { - List cycles = []; - Dictionary state = []; - - foreach (var project in projects.Keys) - Visit(project, []); - - return cycles; - - void Visit(string current, List path) - { - // 1 = on the current path, 2 = fully explored and clean. - if (state.TryGetValue(current, out var visited)) - { - if (visited == 1) - { - var start = path.FindIndex(entry => - string.Equals(entry, current, StringComparison.OrdinalIgnoreCase) - ); - if (start >= 0) - cycles.Add(string.Join(" -> ", path[start..].Append(current))); - } - - return; - } - - state[current] = 1; - path.Add(current); - - if (projects.TryGetValue(current, out var node)) - { - foreach (var reference in node.ProjectReferences) - Visit(reference, path); - } - - path.RemoveAt(path.Count - 1); - state[current] = 2; - } - } -} diff --git a/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs b/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs index 463013c..9de3e23 100644 --- a/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs +++ b/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs @@ -1,5 +1,5 @@ using System.Text.RegularExpressions; -using Purview.EngineeringPlaybook.Architecture.Policy; +using Purview.Architecture.Testing; namespace Purview.EngineeringPlaybook.Architecture; @@ -164,30 +164,4 @@ await Assert .IsEmpty() .Because($"these waivers are not explained on the page: {string.Join(", ", undocumented)}"); } - - [Test] - public async Task Documentation_GivenItsFencedBlocks_AlwaysDeclaresALanguage(CancellationToken cancellationToken) - { - // Arrange - // AGENTS.md: Mermaid is not enabled in mkdocs.yml, so Mermaid source lives in a text fence. - // A bare fence renders as an untyped block and is the easiest convention to break by accident. - var lines = await File.ReadAllLinesAsync(DocumentationPath, cancellationToken); - - // Act - List offenders = []; - var inFence = false; - for (var index = 0; index < lines.Length; index++) - { - if (!lines[index].StartsWith("```", StringComparison.Ordinal)) - continue; - - if (!inFence && lines[index].Trim() == "```") - offenders.Add($"line {index + 1}: opening fence declares no language"); - - inFence = !inFence; - } - - // Assert - await Assert.That(offenders).IsEmpty().Because(string.Join("; ", offenders)); - } } diff --git a/src/tests/Architecture.UnitTests/PolicySchemaTests.cs b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs index 4df381c..e632c9f 100644 --- a/src/tests/Architecture.UnitTests/PolicySchemaTests.cs +++ b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs @@ -1,53 +1,25 @@ -using System.Text.Json; using System.Text.Json.Nodes; -using Json.Schema; -using Purview.EngineeringPlaybook.Architecture.Policy; +using Purview.Architecture.Testing; namespace Purview.EngineeringPlaybook.Architecture; /// -/// Validates .config/architecture-policy.json against its JSON Schema. +/// Validates .config/architecture-policy.json against the schema the package ships. /// /// -/// A schema nothing checks is decoration: it drifts from the document it claims to describe and -/// becomes a second, wrong account of the format. These tests make the schema load-bearing, so an -/// editor's red squiggle and the build agree. Same library the Purview.Build tool uses for -/// purview-build.json, so the ecosystem has one JSON Schema implementation. +/// The schema is embedded in Purview.Architecture.Testing and published at +/// for editors, so the policy is described in one place +/// and validation needs no network. A schema nothing checks is decoration: it drifts from the +/// document it claims to describe and becomes a second, wrong account of the format. The rejection +/// tests below are the point - a schema that cannot fail proves nothing. /// public sealed class PolicySchemaTests { - const string SchemaId = - "https://raw.githubusercontent.com/purview-dev/engineering-playbook/main/.config/architecture-policy.schema.json"; - - /// - /// The schema, parsed once. - /// - /// - /// JsonSchema.FromText registers a schema globally by its $id, and a second - /// registration of the same identifier throws "Overwriting registered schemas is not permitted". - /// Parsing per test therefore fails as soon as two tests run. - /// - static readonly Lazy PolicySchema = new(() => JsonSchema.FromText(File.ReadAllText(SchemaPath))); - - static string ConfigDirectory => Path.Combine(ArchitecturePolicy.RepositoryRoot(), ".config"); - - static string SchemaPath => Path.Combine(ConfigDirectory, "architecture-policy.schema.json"); - - static string PolicyPath => Path.Combine(ConfigDirectory, "architecture-policy.json"); + static string PolicyPath => + Path.Combine(ArchitecturePolicy.RepositoryRoot(), ".config", "architecture-policy.json"); [Test] - public async Task Schema_GivenTheRepository_Exists() - { - // A '$schema' pointing at a file nobody wrote is worse than none: an editor reports no error, - // so the format looks validated when it is not. - await Assert - .That(File.Exists(SchemaPath)) - .IsTrue() - .Because($"the policy declares a $schema, so '{SchemaPath}' must exist"); - } - - [Test] - public async Task Policy_GivenItsSchemaPointer_ReferencesTheSchemaByItsPublishedIdentifier( + public async Task Policy_GivenItsSchemaPointer_ReferencesThePublishedSchemaIdentifier( CancellationToken cancellationToken ) { @@ -55,79 +27,36 @@ CancellationToken cancellationToken var policyText = await File.ReadAllTextAsync(PolicyPath, cancellationToken); // Act - var document = JsonNode.Parse(policyText); - var declared = document?["$schema"]?.GetValue(); - - // Assert // An absolute identifier is what an editor can actually fetch; a relative path only works for // someone who already has the repository open, which is the case that needs help least. - await Assert - .That(declared) - .IsEqualTo(SchemaId) - .Because( - "the policy must point at the published schema identifier, matching the purview-build.json convention" - ); - } - - [Test] - public async Task Schema_GivenItsContent_DeclaresTheMatchingIdentifier(CancellationToken cancellationToken) - { - // Arrange - var schemaText = await File.ReadAllTextAsync(SchemaPath, cancellationToken); - - // Act - var document = JsonNode.Parse(schemaText); + var declared = JsonNode.Parse(policyText)?["$schema"]?.GetValue(); // Assert - await Assert.That(document?["$id"]?.GetValue()).IsEqualTo(SchemaId); - await Assert - .That(document?["$schema"]?.GetValue()) - .IsEqualTo("https://json-schema.org/draft/2020-12/schema"); + await Assert.That(declared).IsEqualTo(PolicySchemaValidator.SchemaId); } [Test] - public async Task Policy_GivenTheSchema_Validates(CancellationToken cancellationToken) + public async Task Policy_GivenTheSchema_Validates() { - // Arrange - var schema = PolicySchema.Value; - using var policy = JsonDocument.Parse(await File.ReadAllTextAsync(PolicyPath, cancellationToken)); - // Act - var result = schema.Evaluate( - policy.RootElement, - new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical, RequireFormatValidation = true } - ); + var failures = PolicySchemaValidator.Validate(PolicyPath); // Assert - await Assert.That(result.IsValid).IsTrue().Because(Describe(result)); + await Assert.That(failures).IsEmpty().Because(string.Join(Environment.NewLine, failures)); } [Test] public async Task Schema_GivenADocumentMissingARequiredField_RejectsIt() { - // Arrange // The schema must be able to fail, or "the policy validates" says nothing. - var schema = PolicySchema.Value; - using var invalid = JsonDocument.Parse("""{ "version": 1 }"""); - - // Act - var result = schema.Evaluate( - invalid.RootElement, - new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical } - ); - - // Assert - await Assert.That(result.IsValid).IsFalse().Because("a policy with no responsibilities must not validate"); + await AssertRejectedAsync("""{ "version": 1 }"""); } [Test] public async Task Schema_GivenAWaiverWithNoReason_RejectsIt() { - // Arrange - // The minLength on 'reason' is the rule that stops a waiver becoming a silent suppression, so - // it is worth pinning rather than trusting. - var schema = PolicySchema.Value; - using var invalid = JsonDocument.Parse( + // The minLength on 'reason' is the rule that stops a waiver becoming a silent suppression. + await AssertRejectedAsync( """ { "version": 1, @@ -144,25 +73,14 @@ public async Task Schema_GivenAWaiverWithNoReason_RejectsIt() } """ ); - - // Act - var result = schema.Evaluate( - invalid.RootElement, - new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical } - ); - - // Assert - await Assert.That(result.IsValid).IsFalse().Because("a waiver with a token reason must not validate"); } [Test] public async Task Schema_GivenAnUnknownResponsibilityField_RejectsIt() { - // Arrange - // additionalProperties:false is what catches a typo'd key, which would otherwise be read as - // an absent rule - the most dangerous kind of mistake in a policy file. - var schema = PolicySchema.Value; - using var invalid = JsonDocument.Parse( + // additionalProperties:false is what catches a typo'd key, which would otherwise be read as an + // absent rule - the most dangerous kind of mistake in a policy file. + await AssertRejectedAsync( """ { "version": 1, @@ -179,63 +97,25 @@ public async Task Schema_GivenAnUnknownResponsibilityField_RejectsIt() } """ ); - - // Act - var result = schema.Evaluate( - invalid.RootElement, - new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical } - ); - - // Assert - await Assert.That(result.IsValid).IsFalse().Because("a misspelled key must not be silently ignored"); } - /// Renders schema evaluation errors with the instance location that failed. - /// The evaluation result. - /// The rendered failures. - static string Describe(EvaluationResults result) + /// Writes a candidate policy, validates it, and asserts the schema rejected it. + /// The malformed policy. + /// A task that completes when the assertion has run. + static async Task AssertRejectedAsync(string json) { - if (result.IsValid) - return "valid"; - - var failures = Leaves(result) - .SelectMany(leaf => leaf.Errors?.Select(error => $" {Location(leaf)}: {error.Value}") ?? []); - - return Environment.NewLine + string.Join(Environment.NewLine, failures); - } - - /// - /// The failing results that have no failing children, so a container failure reports the key that - /// caused it rather than a generic message about the object. - /// - /// The evaluation results to walk. - /// The failing leaves. - static IEnumerable Leaves(EvaluationResults results) - { - var children = (results.Details ?? []).Where(detail => !detail.IsValid).ToList(); - - if (children.Count == 0) + var path = Path.Combine(Path.GetTempPath(), $"architecture-policy-{Guid.NewGuid():N}.json"); + try { - if (!results.IsValid) - yield return results; + await File.WriteAllTextAsync(path, json); - yield break; - } + var failures = PolicySchemaValidator.Validate(path); - foreach (var child in children) + await Assert.That(failures).IsNotEmpty().Because("the schema accepted a document it must reject"); + } + finally { - foreach (var leaf in Leaves(child)) - yield return leaf; + File.Delete(path); } } - - /// Renders the instance location of a failure as a readable path. - /// The failing result. - /// The location. - static string Location(EvaluationResults result) - { - var pointer = result.InstanceLocation.ToString(); - - return string.IsNullOrEmpty(pointer) ? "(root)" : pointer.TrimStart('/').Replace('/', '.'); - } } diff --git a/src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs b/src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs deleted file mode 100644 index 036cf97..0000000 --- a/src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs +++ /dev/null @@ -1,313 +0,0 @@ -using Purview.EngineeringPlaybook.Architecture.Policy; - -namespace Purview.EngineeringPlaybook.Architecture; - -/// -/// Evaluates the declared project references in this repository against the architecture policy. -/// -/// -/// These tests read .config/architecture-policy.json rather than restating its rules, so the -/// policy has one definition. A failure names the project, the rule, the offending reference and the -/// direction the dependency should run - enough to act on without opening the policy. -/// See docs/wiki/Project-Responsibilities.md. -/// -public sealed class ProjectDependencyPolicyTests -{ - static readonly ArchitecturePolicy Policy = ArchitecturePolicy.Load(); - static readonly IReadOnlyDictionary Projects = ProjectGraph.Load( - ArchitecturePolicy.RepositoryRoot() - ); - - static IEnumerable ProductionProjects => Projects.Values.Where(project => !project.IsTestProject); - - [Test] - public async Task Solution_GivenItsProjects_AreAllClassifiableOrDeliberatelyNamed() - { - // Act - IReadOnlyList offenders = - [ - .. ProductionProjects - .Where(project => - Policy.Classify(project.Name) is null && Policy.WaiverFor(project.Name)?.Excuses("naming") != true - ) - .Select(project => - $"'{project.Name}' does not map to a known responsibility or alias. " - + "Rename it to '[.]', or record a waiver in the policy. " - + $"Known responsibilities: {string.Join(", ", Policy.Responsibilities.Select(responsibility => responsibility.Name))}." - ), - ]; - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task ProductionProjects_GivenTheirReferences_RespectTheDependencyMatrix() - { - // Act - List offenders = []; - foreach (var project in ProductionProjects) - { - var responsibility = Policy.Classify(project.Name); - if (responsibility is null) - continue; - - foreach (var reference in project.ProjectReferences) - { - var referenced = Policy.Classify(reference); - if (referenced is null) - continue; - - // A qualified sibling of the same responsibility is the same box on the diagram, so it - // is governed by sibling independence rather than by the matrix. - if (string.Equals(referenced.Name, responsibility.Name, StringComparison.OrdinalIgnoreCase)) - continue; - - if (responsibility.MustNotDependOn.Contains(referenced.Name, StringComparer.OrdinalIgnoreCase)) - { - offenders.Add( - $"'{project.Name}' ({responsibility.Name}) references '{reference}' ({referenced.Name}), " - + $"but {responsibility.Name} must not depend on {referenced.Name}. " - + $"Invert it: {referenced.Name} may depend on {responsibility.Name}, or introduce an abstraction " - + $"that {responsibility.Name} owns and {referenced.Name} implements." - ); - } - else if (!responsibility.MayDependOn.Contains(referenced.Name, StringComparer.OrdinalIgnoreCase)) - { - offenders.Add( - $"'{project.Name}' ({responsibility.Name}) references '{reference}' ({referenced.Name}), " - + "which the policy neither permits nor forbids. " - + $"{responsibility.Name} may depend on: {string.Join(", ", responsibility.MayDependOn)}. " - + $"Add {referenced.Name} to mayDependOn in .config/architecture-policy.json if this is intended." - ); - } - } - } - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task SiblingImplementations_GivenTheirReferences_DoNotDependOnEachOther() - { - // Act - List offenders = []; - foreach (var project in ProductionProjects) - { - var responsibility = Policy.Classify(project.Name); - if (responsibility?.SiblingIndependence != true || !project.Name.Contains('.', StringComparison.Ordinal)) - continue; - - foreach (var reference in project.ProjectReferences) - { - // Only a *qualified* sibling is a peer; the unqualified project is shared ground. - var isQualifiedSibling = - reference.Contains('.', StringComparison.Ordinal) - && !string.Equals(reference, project.Name, StringComparison.OrdinalIgnoreCase) - && string.Equals( - Policy.Classify(reference)?.Name, - responsibility.Name, - StringComparison.OrdinalIgnoreCase - ); - - if (isQualifiedSibling) - { - offenders.Add( - $"'{project.Name}' references its sibling '{reference}'. Sibling {responsibility.Name} implementations " - + $"must stay independent; move the shared part into the unqualified '{responsibility.Name}' project " - + "or into an abstraction both can depend on." - ); - } - } - } - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task Implementations_GivenTheirReferences_DoNotDependOnComposition() - { - // Act - IReadOnlyList offenders = - [ - .. ProductionProjects - .Where(project => - !string.Equals( - Policy.Classify(project.Name)?.Name, - "Composition", - StringComparison.OrdinalIgnoreCase - ) - && !string.Equals( - Policy.Classify(project.Name)?.Category, - "host", - StringComparison.OrdinalIgnoreCase - ) - && project.ProjectReferences.Any(reference => - string.Equals( - Policy.Classify(reference)?.Name, - "Composition", - StringComparison.OrdinalIgnoreCase - ) - ) - ) - .Select(project => - $"'{project.Name}' references Composition. Composition selects implementations, so it depends on them - " - + "never the other way round, which would make the choice circular." - ), - ]; - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task ProductionProjects_GivenTheirPackages_AvoidForbiddenFrameworkDependencies() - { - // Act - List offenders = []; - foreach (var project in ProductionProjects) - { - var responsibility = Policy.Classify(project.Name); - if (responsibility is null) - continue; - - foreach (var package in project.PackageReferences) - { - var forbidden = responsibility.ForbiddenPackagePrefixes.FirstOrDefault(prefix => - package.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) - ); - - if (forbidden is not null) - { - offenders.Add( - $"'{project.Name}' ({responsibility.Name}) references package '{package}', which matches the forbidden " - + $"prefix '{forbidden}' for {responsibility.Name}. {responsibility.Purpose} " - + "Move the dependency to the responsibility that owns that technology." - ); - } - } - } - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task ProjectGraph_GivenItsReferences_HasNoCycles() - { - // Act - var cycles = ProjectGraph.FindCycles(Projects); - - // Assert - await Assert.That(cycles).IsEmpty().Because(Report(cycles)); - } - - [Test] - public async Task ProductionProjects_GivenTheirNames_AvoidCatchAllNames() - { - // Arrange - var discouraged = Policy.DiscouragedProjectNames.Names; - - // Act - IReadOnlyList offenders = - [ - .. ProductionProjects - .Where(project => - discouraged.Contains(project.Name.Split('.')[0], StringComparer.OrdinalIgnoreCase) - && Policy.WaiverFor(project.Name)?.Excuses("discouragedName") != true - ) - .Select(project => $"'{project.Name}': {Policy.DiscouragedProjectNames.Reason}"), - ]; - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task ProductionProjects_GivenTheirReferences_DoNotReferenceATestProject() - { - // Act - IReadOnlyList offenders = - [ - .. ProductionProjects.SelectMany(project => - project - .ProjectReferences.Where(reference => - Projects.TryGetValue(reference, out var target) && target.IsTestProject - ) - .Select(reference => $"'{project.Name}' references the test project '{reference}'.") - ), - ]; - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task PolicyWaivers_GivenTheirDefinitions_AreJustifiedAndStillApplyToAProject() - { - // Act - List offenders = []; - foreach (var waiver in Policy.Waivers) - { - // A waiver for a project that no longer exists is dead weight that hides the next one. - if (!Projects.ContainsKey(waiver.Project)) - offenders.Add($"The waiver for '{waiver.Project}' names a project that does not exist. Remove it."); - - if (waiver.Rules.Count == 0) - offenders.Add($"The waiver for '{waiver.Project}' excuses no named rule, so it does nothing."); - - // A blanket suppression would make the policy unfalsifiable; a reason keeps it reviewable. - if (waiver.Reason.Length < 40) - offenders.Add($"The waiver for '{waiver.Project}' has no substantive reason recorded."); - - if ( - waiver.MapsTo.Length > 0 - && !Policy.Responsibilities.Any(responsibility => - string.Equals(responsibility.Name, waiver.MapsTo, StringComparison.OrdinalIgnoreCase) - ) - ) - { - offenders.Add($"The waiver for '{waiver.Project}' maps to unknown responsibility '{waiver.MapsTo}'."); - } - } - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - [Test] - public async Task WaivedProjects_GivenTheirReferences_AreStillGovernedByTheDependencyMatrix() - { - // Arrange - // A waiver excuses the *name*, never the direction. Infrastructure maps to Adapters, so it is - // bound by the Adapters rules like any other project. - var waived = Policy.Waivers.Where(waiver => waiver.MapsTo.Length > 0).Select(waiver => waiver.Project); - - // Act - IReadOnlyList offenders = - [ - .. waived - .Where(name => Policy.Classify(name) is null) - .Select(name => - $"'{name}' has a waiver but still does not classify, so no dependency rule applies to it." - ), - ]; - - // Assert - await Assert.That(offenders).IsEmpty().Because(Report(offenders)); - } - - /// Renders policy violations as a numbered list for the assertion message. - /// The violations found. - /// The rendered report. - static string Report(IReadOnlyList violations) => - violations.Count == 0 - ? "no violations" - : Environment.NewLine - + string.Join( - Environment.NewLine, - violations.Select((violation, index) => $" {index + 1}. {violation}") - ); -} diff --git a/src/tests/Repository.UnitTests/DocumentationTests.cs b/src/tests/Repository.UnitTests/DocumentationTests.cs new file mode 100644 index 0000000..832bde4 --- /dev/null +++ b/src/tests/Repository.UnitTests/DocumentationTests.cs @@ -0,0 +1,179 @@ +using System.Text.RegularExpressions; + +namespace Purview.EngineeringPlaybook.Repository; + +/// +/// Asserts that the wiki obeys the conventions in docs/wiki/Documentation-Convention.md. +/// +/// +/// +/// Two documentation rules are machine-checkable and both break silently: a page that is not in the +/// navigation is a page nobody finds, and a bare code fence renders as an untyped block. The +/// convention page states both; these tests make the statement true rather than aspirational. +/// +/// +/// index.md is the landing page rather than a content page, so the navigation checks exclude +/// it; the fence rule applies to every page, including the landing page. +/// +/// +public sealed partial class DocumentationTests +{ + [GeneratedRegex(@"\]\((?[^)]+)\)")] + private static partial Regex LinkPattern(); + + static readonly string RepositoryRoot = FindRepositoryRoot(); + + static string WikiDirectory => Path.Combine(RepositoryRoot, "docs", "wiki"); + + [Test] + public async Task Pages_GivenTheSidebar_AreAllReachable(CancellationToken cancellationToken) + { + // Arrange + var sidebar = await File.ReadAllTextAsync(Path.Combine(WikiDirectory, "_Sidebar.md"), cancellationToken); + + // Act + var missing = ContentPages() + .Where(page => !sidebar.Contains($"({page}.md)", StringComparison.Ordinal)) + .ToArray(); + + // Assert + await Assert + .That(missing) + .IsEmpty() + .Because($"these pages are not in _Sidebar.md: {string.Join(", ", missing)}"); + } + + [Test] + public async Task Pages_GivenTheHomeTable_AreAllDiscoverable(CancellationToken cancellationToken) + { + // Arrange + var home = await File.ReadAllTextAsync(Path.Combine(WikiDirectory, "Home.md"), cancellationToken); + + // Act + // Home is the hub itself, so it is the one page the hub's table does not list. + var missing = ContentPages() + .Where(page => page != "Home" && !home.Contains($"({page}.md)", StringComparison.Ordinal)) + .ToArray(); + + // Assert + await Assert + .That(missing) + .IsEmpty() + .Because($"these pages are not linked from the Home.md table: {string.Join(", ", missing)}"); + } + + [Test] + public async Task Navigation_GivenItsLinks_AllResolve(CancellationToken cancellationToken) + { + // Arrange + List offenders = []; + + // Act + foreach (var file in new[] { "_Sidebar.md", "Home.md" }) + { + var lines = await File.ReadAllLinesAsync(Path.Combine(WikiDirectory, file), cancellationToken); + for (var index = 0; index < lines.Length; index++) + { + foreach (var target in RelativeLinkTargets(lines[index])) + { + if (!File.Exists(Path.Combine(WikiDirectory, target))) + offenders.Add($"{file}:{index + 1}: '{target}' does not exist"); + } + } + } + + // Assert + await Assert.That(offenders).IsEmpty().Because(string.Join("; ", offenders)); + } + + [Test] + public async Task Pages_GivenTheirFencedBlocks_AlwaysDeclareALanguage(CancellationToken cancellationToken) + { + // Arrange + // AGENTS.md: Mermaid is not enabled in mkdocs.yml, so Mermaid source lives in a text fence. A + // bare fence renders as an untyped block and is the easiest convention to break by accident. + List offenders = []; + + // Act + foreach (var path in Directory.EnumerateFiles(WikiDirectory, "*.md")) + { + var lines = await File.ReadAllLinesAsync(path, cancellationToken); + var inFence = false; + for (var index = 0; index < lines.Length; index++) + { + if (!lines[index].StartsWith("```", StringComparison.Ordinal)) + continue; + + if (!inFence && lines[index].Trim() == "```") + offenders.Add($"{Path.GetFileName(path)}:{index + 1}: opening fence declares no language"); + + inFence = !inFence; + } + } + + // Assert + await Assert.That(offenders).IsEmpty().Because(string.Join("; ", offenders)); + } + + /// The content pages: every wiki file except the navigation file and the landing page. + /// The page names, without extension. + static IReadOnlyList ContentPages() => + [ + .. Directory + .EnumerateFiles(WikiDirectory, "*.md") + .Select(path => Path.GetFileNameWithoutExtension(path) ?? string.Empty) + .Where(name => name is not "_Sidebar" and not "index") + .Order(StringComparer.Ordinal), + ]; + + /// Reads the relative link targets on a line, ignoring anchors and absolute URLs. + /// The markdown line. + /// The relative file targets, with any fragment removed. + static List RelativeLinkTargets(string line) + { + List targets = []; + foreach (Match match in LinkPattern().Matches(line)) + { + var target = match.Groups["target"].Value.Trim(); + if ( + target.Length == 0 + || target.StartsWith("http", StringComparison.OrdinalIgnoreCase) + || target.StartsWith('#') + || target.StartsWith("mailto:", StringComparison.OrdinalIgnoreCase) + ) + { + continue; + } + + var file = target.Split('#')[0]; + if (file.Length > 0) + targets.Add(file); + } + + return targets; + } + + /// Walks up from the test binary to the repository root. + /// The repository root path. + static string FindRepositoryRoot() + { + DirectoryInfo? directory = new(AppContext.BaseDirectory); + while (directory is not null) + { + if ( + File.Exists(Path.Combine(directory.FullName, "AGENTS.md")) + && Directory.Exists(Path.Combine(directory.FullName, "docs", "wiki")) + ) + { + return directory.FullName; + } + + directory = directory.Parent; + } + + throw new InvalidOperationException( + $"Could not locate the repository root above '{AppContext.BaseDirectory}'. " + + "Expected an AGENTS.md and a docs/wiki folder." + ); + } +} diff --git a/templates/Directory.Packages.props b/templates/Directory.Packages.props index 7992c84..fbb2cf8 100644 --- a/templates/Directory.Packages.props +++ b/templates/Directory.Packages.props @@ -23,8 +23,9 @@ Group with Label so the file reads as a short list of decisions rather than one long alphabetical block. Delete the groups you do not need: the ones below are illustrative, not a required set. - Check these versions against the SDK's current release before you commit — they are the versions - this playbook builds with today, not a promise about tomorrow. + Check these versions against the SDK's current release before you commit. They are the versions + this playbook builds with today, plus one illustrative package family (Purview.Containers) that + you delete if no suite needs a container. --> true @@ -35,7 +36,12 @@ 10.10.0 - 5.0.3 + 5.0.4 + + 1.0.6 1.0.0-prerelease.6 - 1.72.4 + 1.75.0 @@ -58,7 +64,7 @@ - + + + diff --git a/templates/README.md b/templates/README.md index 5758db6..08c251f 100644 --- a/templates/README.md +++ b/templates/README.md @@ -16,6 +16,7 @@ already exists in this repository, with `Acme` standing in for your own namespac | [`AGENTS.md`](AGENTS.md) | `AGENTS.md` | All | | [`mkdocs.yml`](mkdocs.yml) | `mkdocs.yml` | All | | [`TestBuilder.cs`](TestBuilder.cs) | `src/tests/SharedTestingInfra/Builders/TestBuilder.cs` | Medium and up | +| [`architecture-policy.json`](architecture-policy.json) | `.config/architecture-policy.json` | Medium and up | ## How to use them @@ -23,7 +24,8 @@ already exists in this repository, with `Acme` standing in for your own namespac 2. **Replace `Acme`.** It appears in `Directory.Build.props` (as `NamespacePrefix`), in the names in `AGENTS.md`, and in the paths in `purview-build.json`. 3. **Pin the versions.** The SDK version in `global.json` and the package versions in - `Directory.Packages.props` are the ones this repository builds with today. Check them against the SDK's + `Directory.Packages.props` are the ones this repository builds with today, plus one illustrative + package family (`Purview.Containers`) you delete if you do not use it. Check them against the SDK's current release before you commit. 4. **Read the comment in each file.** Every template explains the one thing about it that is easy to get wrong — the order of the `NamespacePrefix` and the import, the `NU1010` trap, why the workflow repeats the @@ -62,8 +64,10 @@ Everything in Small, plus: - `TestBuilder.cs` in `src/tests/SharedTestingInfra/Builders/`. - One test project per layer, named `*.UnitTests`. -- `src/tests/Architecture.UnitTests/` with the rules from - [Architecture as Tests](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Architecture-As-Tests.md). +- `architecture-policy.json` in `.config/`, and one test class deriving from `PurviewArchitectureTests` + (the `Purview.Architecture.Testing` package) so a wrong `ProjectReference` fails the build — see + [Project Responsibilities](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Project-Responsibilities.md) + and [Architecture as Tests](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Architecture-As-Tests.md). - A second CI job for any suite that needs a real resource. ### Large — several packages diff --git a/templates/architecture-policy.json b/templates/architecture-policy.json new file mode 100644 index 0000000..e698801 --- /dev/null +++ b/templates/architecture-policy.json @@ -0,0 +1,70 @@ +{ + // COPY TO: .config/architecture-policy.json + // + // The authoritative dependency and naming policy. The architecture tests read this file, and a test + // you add alongside it can assert your wiki agrees with it, so each rule is defined once rather + // than once in prose and once in code. + // + // Adopt it with Purview.Architecture.Testing: add the package and one derived test class. + // + // [InheritsTests] + // public sealed class ArchitectureTests : PurviewArchitectureTests; + // + // `[InheritsTests]` is required - TUnit does not walk base types during discovery, so without it + // the class compiles, the run is green, and nothing is enforced. See + // docs/wiki/Project-Responsibilities.md. + // + // Delete the responsibilities you do not have. A small application with `Core` and a host is a + // complete and correct architecture; the catalogue is not a list of projects to create. + "$schema": "https://raw.githubusercontent.com/purview-dev/build-sdk/main/architecture-policy.schema.json", + "version": 1, + // The folders holding your projects, relative to the repository root. An empty array scans the + // repository root, which suits a flat layout. Getting this wrong finds no projects - which the + // graph-discovery rule reports rather than letting every other rule pass vacuously. + "sourceRoots": ["src"], + "responsibilities": [ + { + "name": "Core", + "aliases": ["Domain"], + "category": "domain", + "purpose": "The domain model: entities, value objects, invariants and domain rules.", + "mayDependOn": ["Shared"], + "mustNotDependOn": ["Application", "Abstractions", "Contracts", "Persistence", "Providers", "Adapters", "Composition"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Microsoft.AspNetCore", "Aspire.Hosting"], + "separateProjectWhen": "A domain model exists at all.", + "mayRemainCombinedWhen": "The application is a thin utility with no domain rules worth isolating." + }, + { + "name": "Application", + "aliases": ["UseCases"], + "category": "application", + "purpose": "Coordinates use cases and workflows across domain and capability boundaries.", + "mayDependOn": ["Core", "Abstractions", "Contracts", "Shared"], + "mustNotDependOn": ["Persistence", "Providers", "Adapters", "Composition"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Npgsql", "Microsoft.Data.SqlClient", "Aspire.Hosting"], + "separateProjectWhen": "Workflows are reused by more than one host, or coordinate several capabilities.", + "mayRemainCombinedWhen": "It would only forward CRUD without adding behaviour." + }, + { + "name": "Shared", + "aliases": [], + "category": "shared", + "purpose": "Small, broadly reusable, domain-independent utilities.", + "mayDependOn": [], + "mustNotDependOn": ["Core", "Abstractions", "Application", "Contracts", "Persistence", "Providers", "Adapters", "Composition"], + "separateProjectWhen": "A domain-independent primitive is used by two or more responsibilities.", + "mayRemainCombinedWhen": "Almost always. Shared is the easiest project to create and the hardest to keep coherent." + } + ], + "discouragedProjectNames": { + "names": ["Infrastructure", "Services", "Managers", "Models", "Helpers", "Common", "Utils", "Utilities"], + "reason": "These name a category, not a responsibility, so nothing can be excluded from them and they accumulate. Infrastructure is an architectural category covering Persistence, Providers and Adapters - not a project." + }, + "rules": { + "noCycles": true, + "noProductionToTestReferences": true, + "implementationsMustNotReferenceComposition": true, + "siblingImplementationsIndependent": true + }, + "waivers": [] +} diff --git a/templates/dotnet-tools.json b/templates/dotnet-tools.json index e405545..372873b 100644 --- a/templates/dotnet-tools.json +++ b/templates/dotnet-tools.json @@ -14,6 +14,13 @@ "version": "1.3.0", "commands": ["csharpier"], "rollForward": true + }, + // `just pipeline-pr` runs `dotnet purview-build`. Pinning it here means the shared pipeline + // version is restored by `dotnet tool restore` rather than installed globally per machine. + "purview.build": { + "version": "0.5.3", + "commands": ["purview-build"], + "rollForward": false } } } diff --git a/templates/global.json b/templates/global.json index 4b079bb..865d9ea 100644 --- a/templates/global.json +++ b/templates/global.json @@ -9,7 +9,7 @@ "allowPrerelease": false }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.4" + "Purview.BuildSdk": "1.0.6" }, "test": { "runner": "Microsoft.Testing.Platform"