From eb5080a780d54c793b31e9174727eb91b900f337 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 09:46:35 +0100 Subject: [PATCH 1/4] feat: add architecture policy, enforcement tests and guidance skills The project responsibility catalogue, naming convention and dependency matrix are defined once, in .config/architecture-policy.json. The architecture tests read it rather than restating its rules, and PolicyDocumentationTests asserts the wiki tables match it, so a rule cannot be true in prose and false in code. Enforced: the dependency matrix, sibling independence, nothing depending on Composition, forbidden framework packages per responsibility, cycles, catch-all project names, and production code referencing a test project. Failures name the project, the rule, the offending reference and the direction it should run. Waivers record a deliberate departure instead of suppressing a rule silently. This repository's own Infrastructure project is the worked example: it maps to Adapters, so the name is excused and every dependency rule still applies. Adds the Project-Responsibilities and Agent-Guidance-Skills pages with Mermaid diagrams A-N, all verified to render with mermaid-cli. --- .config/architecture-policy.json | 310 +++++++++++++++ docs/wiki/Agent-Guidance-Skills.md | 194 +++++++++ docs/wiki/Agent-Skills-Distribution.md | 133 ++++--- docs/wiki/Home.md | 2 + docs/wiki/Project-Responsibilities.md | 373 ++++++++++++++++++ docs/wiki/_Sidebar.md | 2 + .../Policy/ArchitecturePolicy.cs | 195 +++++++++ .../Policy/ProjectGraph.cs | 148 +++++++ .../PolicyDocumentationTests.cs | 193 +++++++++ .../ProjectDependencyPolicyTests.cs | 313 +++++++++++++++ 10 files changed, 1796 insertions(+), 67 deletions(-) create mode 100644 .config/architecture-policy.json create mode 100644 docs/wiki/Agent-Guidance-Skills.md create mode 100644 docs/wiki/Project-Responsibilities.md create mode 100644 src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs create mode 100644 src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs create mode 100644 src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs create mode 100644 src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs diff --git a/.config/architecture-policy.json b/.config/architecture-policy.json new file mode 100644 index 0000000..034d841 --- /dev/null +++ b/.config/architecture-policy.json @@ -0,0 +1,310 @@ +{ + "$schema": "./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.", + "conventions": { + "qualifiedNameSeparator": ".", + "dependencyArrowMeaning": "A -> B means A depends on B at compile time." + }, + "responsibilities": [ + { + "name": "Core", + "aliases": ["Domain"], + "category": "domain", + "purpose": "The fundamental domain model: entities, value objects, invariants, domain events and domain errors.", + "mayDependOn": ["Shared"], + "mustNotDependOn": [ + "Application", + "Abstractions", + "Contracts", + "Persistence", + "Providers", + "Adapters", + "Composition", + "API", + "Worker", + "Migrator", + "AppHost", + "ServiceDefaults", + "WebApp" + ], + "forbiddenPackagePrefixes": [ + "Microsoft.EntityFrameworkCore", + "Microsoft.AspNetCore", + "Npgsql", + "Microsoft.Data.SqlClient", + "Aspire.Hosting" + ], + "separateProjectWhen": "Always, when a domain model exists at all; it is the one responsibility that earns its own project early.", + "mayRemainCombinedWhen": "The application is a thin utility with no domain rules worth isolating." + }, + { + "name": "Abstractions", + "aliases": [], + "category": "domain", + "purpose": "Stable contracts for capabilities needed across architectural boundaries.", + "mayDependOn": ["Core", "Shared"], + "mustNotDependOn": [ + "Application", + "Persistence", + "Providers", + "Adapters", + "Composition", + "API", + "Worker", + "Migrator", + "AppHost", + "ServiceDefaults", + "WebApp" + ], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Microsoft.AspNetCore", "Aspire.Hosting"], + "separateProjectWhen": "A contract has more than one implementation, or an implementation must not be referenced by its consumer.", + "mayRemainCombinedWhen": "An interface has a single implementation and no cross-boundary consumer; keep it beside that implementation." + }, + { + "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", + "API", + "Worker", + "Migrator", + "AppHost", + "ServiceDefaults", + "WebApp" + ], + "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 trivial CRUD to persistence without adding behaviour." + }, + { + "name": "Contracts", + "aliases": [], + "category": "application", + "purpose": "Externally exposed contracts, particularly API request and response models.", + "mayDependOn": ["Core", "Shared"], + "mustNotDependOn": [ + "Application", + "Persistence", + "Providers", + "Adapters", + "Composition", + "API", + "Worker", + "Migrator", + "AppHost", + "ServiceDefaults", + "WebApp" + ], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Npgsql", "Microsoft.Data.SqlClient"], + "separateProjectWhen": "A client - a WebApp, an SDK or another service - must compile against the contract without taking the server.", + "mayRemainCombinedWhen": "The only consumer is the API host itself." + }, + { + "name": "Persistence", + "aliases": [], + "category": "infrastructure", + "purpose": "Durable data access: DbContext, mappings, migrations and persistence-specific configuration.", + "mayDependOn": ["Core", "Abstractions", "Application", "Shared"], + "mustNotDependOn": ["Composition", "API", "Worker", "Migrator", "AppHost", "WebApp", "Providers", "Adapters"], + "forbiddenPackagePrefixes": ["Microsoft.AspNetCore.Mvc", "Aspire.Hosting"], + "separateProjectWhen": "Always, when persistence exists; it is what Core and Application must not reference.", + "mayRemainCombinedWhen": "A single-project utility where the storage is a file and there is no provider to isolate." + }, + { + "name": "Providers", + "aliases": ["Integrations"], + "category": "infrastructure", + "purpose": "Integrations with named external platforms or services.", + "mayDependOn": ["Core", "Abstractions", "Application", "Shared"], + "mustNotDependOn": ["Composition", "API", "Worker", "Migrator", "AppHost", "WebApp", "Persistence", "Adapters"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Aspire.Hosting"], + "siblingIndependence": true, + "separateProjectWhen": "Two or more platforms are integrated and their SDKs or lifecycles differ.", + "mayRemainCombinedWhen": "One platform, or several small clients sharing a lifecycle and dependency set." + }, + { + "name": "Adapters", + "aliases": [], + "category": "infrastructure", + "purpose": "Concrete implementations of application-defined technical capabilities.", + "mayDependOn": ["Core", "Abstractions", "Application", "Shared"], + "mustNotDependOn": ["Composition", "API", "Worker", "Migrator", "AppHost", "WebApp", "Persistence", "Providers"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Aspire.Hosting"], + "siblingIndependence": true, + "separateProjectWhen": "A capability has more than one technology behind it, or the technology drags in heavy dependencies.", + "mayRemainCombinedWhen": "One capability, one technology, small surface." + }, + { + "name": "Composition", + "aliases": [], + "category": "composition", + "purpose": "Dependency injection registration and concrete implementation selection.", + "mayDependOn": [ + "Core", + "Abstractions", + "Application", + "Contracts", + "Persistence", + "Providers", + "Adapters", + "Shared" + ], + "mustNotDependOn": ["API", "Worker", "Migrator", "AppHost", "WebApp"], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "More than one host composes the same implementations.", + "mayRemainCombinedWhen": "There is one host; compose in the host." + }, + { + "name": "API", + "aliases": [], + "category": "host", + "purpose": "Executable HTTP application host.", + "mayDependOn": [ + "Core", + "Abstractions", + "Application", + "Contracts", + "Composition", + "ServiceDefaults", + "Shared", + "Persistence", + "Providers", + "Adapters" + ], + "mustNotDependOn": ["Worker", "Migrator", "AppHost", "WebApp"], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "An HTTP surface exists.", + "mayRemainCombinedWhen": "Never combined with domain or persistence; it is the host." + }, + { + "name": "Worker", + "aliases": [], + "category": "host", + "purpose": "Executable background-processing host.", + "mayDependOn": [ + "Core", + "Abstractions", + "Application", + "Contracts", + "Composition", + "ServiceDefaults", + "Shared", + "Persistence", + "Providers", + "Adapters" + ], + "mustNotDependOn": ["API", "Migrator", "AppHost", "WebApp"], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "Background processing is deployed or scaled separately from the API.", + "mayRemainCombinedWhen": "A hosted service inside the API is sufficient and they scale together." + }, + { + "name": "Migrator", + "aliases": [], + "category": "host", + "purpose": "Executable migration and controlled seeding host. Persistence owns migration definitions; Migrator owns their execution.", + "mayDependOn": ["Core", "Abstractions", "Persistence", "Composition", "ServiceDefaults", "Shared"], + "mustNotDependOn": ["API", "Worker", "AppHost", "WebApp"], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "Schema changes are applied by a deliberate deployment step rather than at API startup.", + "mayRemainCombinedWhen": "Development-only, where the host applies migrations on start." + }, + { + "name": "AppHost", + "aliases": [], + "category": "host", + "purpose": "Aspire orchestration host. Orchestration is not a compile-time dependency between the services it orchestrates.", + "mayDependOn": ["API", "Worker", "Migrator", "WebApp", "Shared"], + "mustNotDependOn": ["Core", "Application", "Persistence", "Providers", "Adapters"], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "Aspire is used.", + "mayRemainCombinedWhen": "Never; it is its own host." + }, + { + "name": "ServiceDefaults", + "aliases": [], + "category": "host", + "purpose": "Common host configuration: telemetry, health checks, resilience and service discovery defaults.", + "mayDependOn": ["Shared"], + "mustNotDependOn": [ + "Core", + "Application", + "Persistence", + "Providers", + "Adapters", + "Composition", + "API", + "Worker", + "Migrator", + "AppHost", + "WebApp" + ], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "More than one host shares operational defaults.", + "mayRemainCombinedWhen": "There is one host." + }, + { + "name": "WebApp", + "aliases": [], + "category": "host", + "purpose": "Frontend application.", + "mayDependOn": ["Contracts", "ServiceDefaults", "Shared"], + "mustNotDependOn": ["Core", "Application", "Persistence", "Providers", "Adapters", "Composition", "API", "Worker", "Migrator"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Npgsql", "Microsoft.Data.SqlClient"], + "separateProjectWhen": "A UI exists.", + "mayRemainCombinedWhen": "Server-rendered pages inside the API host." + }, + { + "name": "Shared", + "aliases": [], + "category": "shared", + "purpose": "Small, broadly reusable, domain-independent utilities.", + "mayDependOn": [], + "mustNotDependOn": [ + "Core", + "Abstractions", + "Application", + "Contracts", + "Persistence", + "Providers", + "Adapters", + "Composition", + "API", + "Worker", + "Migrator", + "AppHost", + "ServiceDefaults", + "WebApp" + ], + "forbiddenPackagePrefixes": [], + "separateProjectWhen": "A genuinely 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 + }, + "exceptions": [ + { + "project": "Infrastructure", + "mapsTo": "Adapters", + "rules": ["naming", "discouragedName"], + "reason": "This repository's reference implementation has used 'Infrastructure' since before the responsibility catalogue existed, and it holds exactly one adapter - the in-memory IConventionRepository - so it is not the catch-all the discouraged-name rule is aimed at. The policy's own guidance is not to rename an established project merely to match canonical terminology, and the wiki, AGENTS.md and NamespaceRemoveSuffix all name it. It is classified as Adapters for every dependency rule, so the direction is still enforced.", + "reviewedOn": "2026-10-15" + } + ] +} diff --git a/docs/wiki/Agent-Guidance-Skills.md b/docs/wiki/Agent-Guidance-Skills.md new file mode 100644 index 0000000..cb44772 --- /dev/null +++ b/docs/wiki/Agent-Guidance-Skills.md @@ -0,0 +1,194 @@ +# Agent Guidance Skills + +## 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: + +| Skill | Answers | +| -- | -- | +| `purview-architecture` | How much structure does this project actually need, and what do I need to know before saying? | +| `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? | + +Two obligations fall out of that, and both are enforced only by review: + +- **Context before recommendation.** An agent must establish project complexity, project type, + persistence and telemetry before making a decision that depends on them — and must read those from + the repository rather than asking. +- **Guidance is written once.** A library skill describes its library. It does not restate the + architecture, telemetry or persistence advice; it links to it. + +## Why + +A coding agent will give an answer whether or not it has the context to give a good one. Asked to +"add a service", it will produce whatever shape its training makes most available — which is usually +the shape of a large enterprise codebase, because that is what gets written about. + +The result is a four-project Clean Architecture solution with a repository interface and a mapping +layer, wrapped around one table and one caller. Nothing in it is wrong in isolation. All of it is +cost that will be paid at every future change, for a failure that will never happen. + +The opposite failure is quieter and more common: telemetry deferred because the application is +"small", and then never added, because by the time it is needed the code has no seams for it. + +And the third failure is irrelevant advice — asking a stateless CLI which database GUID strategy it +wants. That trains the user to stop reading what the agent says. + +## Project-context discovery + +**Diagram L — project-context discovery and skill activation.** + +```text +flowchart TD + A["Request touching architecture
or substantial code generation"] --> B["Read the repository"] + B --> C["Complexity: small / medium / large
from requirements, not file count"] + B --> D["Project type
SDK attribute, OutputType, AppHost"] + B --> E["Persistence
provider package, DbContext, connection string"] + B --> F["Telemetry
ILogger, ActivitySource, AddOpenTelemetry"] + + C --> G{"Any dimension still
unknown AND material?"} + D --> G + E --> G + F --> G + + G -- no --> H["Decide - ask nothing"] + G -- yes --> I["One concise batched question
with a stated default"] + I --> H + + H --> J["purview-architecture
always"] + H --> K["purview-telemetry
always, at every size"] + E --> L{"Persistence real?"} + L -- yes --> M["purview-persistence"] + L -- no --> N["purview-persistence stays silent"] +``` + +The branch that matters most is `G`. A dimension answered by a project file, existing source, or an +earlier turn is answered; re-asking it is the single most common way this guidance is applied badly. + +## Telemetry + +Instrumentation is constant across project sizes. What scales is the export and operations story. +Conflating the two is what produces "we are too small for telemetry". + +**Diagram M — telemetry recommendation workflow.** + +```text +flowchart TD + A["Project needs telemetry advice"] --> B["Inspect existing instrumentation"] + B --> C{"Telemetry already
configured?"} + C -- yes --> D["Preserve and extend
never duplicate a registration"] + C -- no --> E{"Reusable library
or application host?"} + + E -- library --> F["ILogger, ActivitySource, Meter
publish source names
NEVER configure a global provider"] + E -- host --> G{"Project size?"} + + G -- small --> H["Console or single OTLP endpoint
no collector, no dashboards"] + G -- medium --> I["OTLP, dependency instrumentation
trace propagation, metrics, health checks"] + G -- large --> J["End-to-end tracing, resource attributes
dashboards, alerting, SLIs, sampling policy"] + + D --> K["Recommend Purview.Telemetry.SourceGenerator
where the TFM is compatible"] + F --> K + H --> K + I --> K + J --> K +``` + +Note that no path reaches "no telemetry". The small branch reaches *small* telemetry. + +The library branch is the one most often got wrong: a library that calls `AddOpenTelemetry` breaks +every consumer that already configured a provider, and wins or loses silently depending on +initialisation order. + +## Persistence and GUID keys + +This guidance activates only when persistence is real and GUIDs are keys. For a stateless CLI or a +pure library, the correct output is silence. + +**Diagram N — persistence-specific GUID strategy selection.** + +```text +flowchart TD + A["Persistence confirmed?"] -- no --> Z["Out of scope - ask nothing"] + A -- yes --> B{"GUIDs used as
primary keys?"} + B -- no --> Z + B -- yes --> C{"Identifier needed
before SaveChanges?"} + + C -- yes --> D["Application must generate it"] + C -- no --> E["Database may generate it"] + + D --> F{"Provider?"} + E --> G{"Provider?"} + + F -- "SQL Server" --> H["EF Core SequentialGuidValueGenerator"] + F -- "PostgreSQL" --> I["Guid.CreateVersion7 on .NET 9+
ValueGeneratedNever"] + + G -- "SQL Server" --> J["NEWSEQUENTIALID()"] + G -- "PostgreSQL" --> K{"Server version 18+?"} + K -- yes --> L["uuidv7()"] + K -- no --> M["gen_random_uuid(), or generate v7 in the app"] + + H --> N{"Must be
unpredictable?"} + I --> N + J --> N + L --> N + M --> N + N -- yes --> O["Random v4 - ordered keys leak creation time"] + N -- no --> P["Keep the ordered choice"] +``` + +**The two providers do not share an answer.** SQL Server compares `uniqueidentifier` with the last +six bytes most significant, so a byte-ordered UUIDv7 is effectively random to its clustered index. +PostgreSQL compares `uuid` in byte order, so UUIDv7 is genuinely sequential there. A strategy carried +across without re-deriving it is the failure this diagram exists to prevent. + +There is no Purview GUID generation package. Prefer a framework or database capability; a custom +generator is a correctness risk — endianness, clock regression, counter rollover and thread safety +are all live hazards, and the failure shows up as duplicate keys in production. + +## How it is enforced + +| Rule | Enforced by | +| -- | -- | +| Every `SKILL.md` has valid frontmatter, a unique name matching its folder, and existing references | `ValidatePurviewSkills` (fails the pack) | +| A skill is reachable without loading every skill | The `description` frontmatter is the activation trigger | +| Context is established before a recommendation | **Review** | +| Telemetry is recommended at every project size | **Review** | +| GUID guidance stays silent without persistence | **Review** | + +The last three are review obligations and this page does not pretend otherwise. A build can check +that a skill is well-formed; it cannot check that an agent followed prose. See +[Validation scenarios](#validation-scenarios) for how they are reviewed instead. + +## Validation scenarios + +These are the scenarios used to review the guidance by hand. They are **not** automated tests, and +passing them does not prove an agent will always comply — it demonstrates the guidance says the right +thing when read. + +| Scenario | Expected | +| -- | -- | +| "Add a command to this CLI" (no persistence) | No new projects, no repository, no DI indirection. Telemetry recommended. **No GUID question asked.** | +| "Build a small HTTP API" | Single project. Structured logging and an `ActivitySource` from the first commit. Console or one OTLP endpoint — no collector, no dashboards. | +| "Add telemetry to this reusable library" | `ILogger` / `ActivitySource` / `Meter`, names published for the host. **No `AddOpenTelemetry`, no exporter, no backend requirement.** | +| "Use GUID keys on SQL Server, high write volume" | `NEWSEQUENTIALID()`, or `SequentialGuidValueGenerator` if the value is needed before insert. **Not** UUIDv7. | +| "Use GUID keys on PostgreSQL, high write volume" | `Guid.CreateVersion7()` with `ValueGeneratedNever()`. `uuidv7()` only if the server is 18+. | +| Repository already has `AddOpenTelemetry` | Existing setup extended, not replaced. No duplicate registration. | +| "Should I add a repository over EF Core?" | Only with a named reason other than testability — `Purview.Containers` gives a real database in tests. | +| Project file already states the type and persistence | No questions asked at all. | + +## At your scale + +- **Any project.** The context habit is the whole value: read the repository before recommending. + It costs nothing and removes most bad answers. +- **A repository with conventions worth repeating.** Write the architecture guidance down once and + link to it, rather than restating it in each library's skill. +- **A platform with several libraries.** Keep shared guidance in shared skills, as here, so a change + to the telemetry position is one edit rather than one per library. + +## See also + +- [Agent Skills Distribution](Agent-Skills-Distribution.md) — how these skills reach a consumer +- [Agent Enablement](Agent-Enablement.md) +- [Engineering Principles](Engineering-Principles.md) +- [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) diff --git a/docs/wiki/Agent-Skills-Distribution.md b/docs/wiki/Agent-Skills-Distribution.md index 1ed6c94..fdb36c3 100644 --- a/docs/wiki/Agent-Skills-Distribution.md +++ b/docs/wiki/Agent-Skills-Distribution.md @@ -71,27 +71,23 @@ own wiki is a second source of truth, and it is the one that will drift. ## Publication and distribution +**Diagram J — skill publication and distribution.** + ```text -┌──────────────────────────────────────────────────────────────┐ -│ Library repository │ -│ │ -│ skills//SKILL.md ← the single authored copy │ -│ │ │ -│ ├──────────────┐ │ -│ ▼ ▼ │ -│ git (tracked) IncludePurviewSkillsInPackage │ -│ │ │ │ -└────────┼──────────────┼──────────────────────────────────────┘ - │ ▼ - │ .nupkg .agents/skills// - │ │ + generated .gitignore - │ │ + nuspec - │ ▼ - │ NuGet feed - │ │ - ▼ ▼ - skills CLI Purview.BuildSdk mirror - (clone + ref) (restore graph) +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"] ``` The nuspec's `` element is written by SourceLink for every package built by the SDK. It @@ -101,20 +97,17 @@ is what lets a consumer get from a restored package back to the exact source com 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 -project.assets.json - │ every restored package: id, version, on-disk path - ▼ -/.agents/skills/*/SKILL.md ──► which skills this package ships - │ -/*.nuspec - └─ - │ - ▼ - owner/repo# + [skill names] - │ - ├─ has GitHub URL and commit ──► PurviewSkillSource (installable) - └─ missing either ──► PurviewUnresolvableSkillSource (PRSKL0002) +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/` @@ -124,22 +117,32 @@ or `scripts/` folder beside one is part of that skill, not another. The separation matters more than any single piece of it. +**Diagram K (detail) — installer and Build SDK responsibilities.** + ```text -┌───────────────────┬──────────────────────────────────────────────┐ -│ Skill metadata │ The package itself: .agents/skills/* and the │ -│ │ nuspec element. Nothing custom. │ -├───────────────────┼──────────────────────────────────────────────┤ -│ Skill resolver │ ResolvePurviewSkillSources. Reads the restore │ -│ │ graph, emits source + ref + skill names. │ -│ │ Decides nothing about agents or paths. │ -├───────────────────┼──────────────────────────────────────────────┤ -│ Skill installer │ The skills CLI. Owns every agent-specific │ -│ │ destination, the symlink-or-copy decision and │ -│ │ skills-lock.json. │ -├───────────────────┼──────────────────────────────────────────────┤ -│ Build SDK │ Discovers packages, exposes explicit targets, │ -│ │ invokes the installer. Never installs itself. │ -└───────────────────┴──────────────────────────────────────────────┘ +flowchart LR + subgraph Package["Skill metadata - the package itself"] + M1[".agents/skills/*"] + M2["nuspec repository element"] + end + + subgraph SDK["Purview.BuildSdk"] + R["Skill resolver
ResolvePurviewSkillSources
decides nothing about agents"] + T["Explicit targets
ListPurviewSkills
InstallPurviewSkills"] + D["CLI detection
DetectPurviewSkillsCli"] + end + + subgraph CLI["Skill installer - npx skills"] + I1["Agent-specific destinations"] + I2["symlink or copy"] + I3["skills-lock.json"] + end + + M1 --> R + M2 --> R + 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 @@ -157,30 +160,25 @@ numerous, and change on their own schedule; the CLI tracks them, so the SDK does | 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 and its name unique | Review, plus the CLI's own parse on install — **not** a build check | +| 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 | +| The guidance in a skill is good, and an agent follows it | **Review** | -The last row is a review obligation. The build does not validate skill frontmatter, and this page does -not claim it does. +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 - referenced package version (from project.assets.json) - │ - ▼ - nuspec present? - │ │ - yes no - │ │ - ▼ ▼ - owner/repo# PRSKL0002 warning: - pinned, reproducible not installed; the package's - │ own mirror still delivers it - ▼ - skills CLI records source + ref in skills-lock.json - │ - ▼ - npx skills experimental_install restores the same set +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 @@ -262,6 +260,7 @@ Updating and removing are the CLI's, not the SDK's: `npx skills update`, `npx sk | `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. | +| `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. | diff --git a/docs/wiki/Home.md b/docs/wiki/Home.md index 65720e7..f86fca2 100644 --- a/docs/wiki/Home.md +++ b/docs/wiki/Home.md @@ -31,12 +31,14 @@ at all, so the two are never blurred. | [Getting Started](Getting-Started.md) | The smallest repository that satisfies every rule, in the order you would build it. | | [Engineering Principles](Engineering-Principles.md) | The non-negotiable rules, and where the authoritative statement of each one lives. | | [Repository Shape](Repository-Shape.md) | Where a file goes, what it is called, and how a project's name becomes its namespace. | +| [Project Responsibilities](Project-Responsibilities.md) | What each project is for, how it is named, and what it may depend on. | | [Architecture as Tests](Architecture-As-Tests.md) | How the build refuses a dependency that points the wrong way. | | [Verification Model](Verification-Model.md) | Which checks run, in which tier, for which change. | | [Contracts and Data Exchange](Contracts-And-Data-Exchange.md) | How value objects, schemas and results cross a boundary. | | [Testing and Test Data](Testing-And-Test-Data.md) | How tests are written, and where their data comes from. | | [Agent Enablement](Agent-Enablement.md) | How a repository is made legible to a coding agent. | | [Agent Skills Distribution](Agent-Skills-Distribution.md) | How a skill is published once and reaches both a package's consumers and an agent's own directories. | +| [Agent Guidance Skills](Agent-Guidance-Skills.md) | The shared architecture, telemetry and persistence guidance, and the context an agent must establish first. | | [Documentation Convention](Documentation-Convention.md) | How the wiki is written, structured and published. | | [Worked Examples](Worked-Examples.md) | The reference implementation in this repository, walked end to end. | | [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) | Which parts apply to a small, medium or large repository. | diff --git a/docs/wiki/Project-Responsibilities.md b/docs/wiki/Project-Responsibilities.md new file mode 100644 index 0000000..8e882c9 --- /dev/null +++ b/docs/wiki/Project-Responsibilities.md @@ -0,0 +1,373 @@ +# Project Responsibilities + +## The rule + +A project name states a **responsibility**, optionally qualified by an implementation: + +```text +[.] +``` + +`Persistence`, `Persistence.SqlServer`, `Providers.GitHub`, `Adapters.Email` are all well-formed. The +responsibility decides the rules; the qualifier is a naming choice and never changes them. + +**This is a catalogue of responsibilities, not a list of projects you must create.** A small +application that combines Core, Application and Persistence in one assembly has not broken the rule — +it has one project carrying three responsibilities, which is usually correct. The catalogue tells you +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). +The tables on this page are checked against it — see [How it is enforced](#how-it-is-enforced). + +## Why + +Two failures, equally common and opposite. + +**The catch-all project.** `Infrastructure`, `Services`, `Helpers`, `Common`. These name a *category*, +so nothing can be excluded from them. Every technical class is plausibly infrastructure, every +coordinating class is plausibly a service, and both grow until no one can say what the project is for. +A responsibility name can be violated; a category name cannot, which is exactly why it is useless. + +**Architecture bought in advance.** Four projects, a repository interface and a mapping layer around +one table and one caller. Nothing in it is wrong in isolation; all of it is cost paid at every future +change against a failure that never arrives. + +A catalogue addresses both: it gives each project a name that can be wrong, and it makes "do I need +this project?" a question with an answer rather than a matter of taste. + +## The catalogue + +`A → B` means **A depends on B at compile time**. It is not a runtime call, a DI registration, an +HTTP request or an Aspire orchestration edge. + +| Responsibility | Category | Also known as | May depend on | +| -- | -- | -- | -- | +| `Core` | domain | `Domain` | Shared | +| `Abstractions` | domain | — | Core, Shared | +| `Application` | application | `UseCases` | Core, Abstractions, Contracts, Shared | +| `Contracts` | application | — | Core, Shared | +| `Persistence` | infrastructure | — | Core, Abstractions, Application, Shared | +| `Providers` | infrastructure | `Integrations` | Core, Abstractions, Application, Shared | +| `Adapters` | infrastructure | — | Core, Abstractions, Application, Shared | +| `Composition` | composition | — | Core, Abstractions, Application, Contracts, Persistence, Providers, Adapters, Shared | +| `API` | host | — | Core, Abstractions, Application, Contracts, Composition, ServiceDefaults, Shared, Persistence, Providers, Adapters | +| `Worker` | host | — | Core, Abstractions, Application, Contracts, Composition, ServiceDefaults, Shared, Persistence, Providers, Adapters | +| `Migrator` | host | — | Core, Abstractions, Persistence, Composition, ServiceDefaults, Shared | +| `AppHost` | host | — | API, Worker, Migrator, WebApp, Shared | +| `ServiceDefaults` | host | — | Shared | +| `WebApp` | host | — | Contracts, ServiceDefaults, Shared | +| `Shared` | shared | — | *(nothing)* | + +### When each one earns its own project + +| Responsibility | Split it out when | Leave it combined when | +| -- | -- | -- | +| `Core` | A domain model exists at all | The app is a thin utility with no domain rules | +| `Abstractions` | A contract has two implementations, or a consumer must not see the implementation | One implementation, no cross-boundary consumer — keep the interface beside it | +| `Application` | Workflows are reused by more than one host | It would only forward CRUD without adding behaviour | +| `Contracts` | A client must compile against the contract without taking the server | The only consumer is the API host | +| `Persistence` | Persistence exists — it is what Core must not reference | Single-project utility writing to a file | +| `Providers` | Two platforms with different SDKs or lifecycles | One platform, or small clients sharing a lifecycle | +| `Adapters` | A capability has more than one technology behind it | One capability, one technology | +| `Composition` | More than one host composes the same implementations | One host — compose in the host | +| `Migrator` | Schema changes are applied by a deliberate deployment step | Development-only, migrate on startup | +| `ServiceDefaults` | More than one host shares operational defaults | One host | +| `Shared` | A domain-independent primitive is used by two or more responsibilities | Almost always — it is the easiest project to create and the hardest to keep coherent | + +### Diagram A — the responsibility catalogue + +Grouped by category. **No project here is mandatory.** + +```text +flowchart TB + subgraph domain["Domain"] + Core["Core
(aka Domain)"] + Abstractions["Abstractions"] + end + subgraph application["Application"] + Application["Application
(aka UseCases)"] + Contracts["Contracts"] + end + subgraph infrastructure["Infrastructure - a category, not a project"] + Persistence["Persistence"] + Providers["Providers
(aka Integrations)"] + Adapters["Adapters"] + end + subgraph composition["Composition"] + Composition["Composition"] + end + subgraph hosts["Hosts"] + API["API"] + Worker["Worker"] + Migrator["Migrator"] + AppHost["AppHost"] + ServiceDefaults["ServiceDefaults"] + WebApp["WebApp"] + end + subgraph shared["Shared"] + Shared["Shared"] + end +``` + +### Diagram B — compile-time dependency direction + +Every arrow means *depends on*. Note that nothing points into Core except by permission, and nothing +points at Composition at all. + +```text +flowchart RL + Abstractions --> Core + Application --> Core + Application --> Abstractions + Application --> Contracts + Contracts --> Core + + Persistence --> Core + Persistence --> Abstractions + Persistence --> Application + Providers --> Abstractions + Adapters --> Abstractions + + Composition --> Application + Composition --> Persistence + Composition --> Providers + Composition --> Adapters + + API --> Composition + API --> Contracts + Worker --> Composition + Migrator --> Persistence + WebApp --> Contracts + + AppHost -.->|"orchestrates, not a reference"| API + AppHost -.->|"orchestrates, not a reference"| Worker +``` + +## Naming + +### Diagram H — generic or qualified? + +```text +flowchart TD + A["New implementation of a responsibility"] --> B{"More than one
implementation?"} + B -- no --> C["Use the generic name
Persistence, Providers, Adapters"] + B -- yes --> D{"Do they differ in
dependencies, lifecycle
or deployment?"} + D -- no --> E["One project, several implementations inside
still the generic name"] + D -- yes --> F["Qualify each one
Persistence.SqlServer, Persistence.PostgreSql"] + F --> G{"Is there genuinely
shared code?"} + G -- yes --> H["Add the unqualified project too
Persistence + Persistence.SqlServer"] + G -- no --> I["Qualified projects only
no empty parent"] +``` + +Three rules fall out of that tree, and all three are departures from what people usually assume: + +- **Do not qualify a single implementation.** `Persistence` is correct even though it happens to use + SQL Server. Rename it when a second provider arrives, not in anticipation. +- **Do not create an empty parent.** `Persistence.SqlServer` and `Persistence.PostgreSql` with no + shared code need no `Persistence` project. +- **Multiple implementations do not require multiple assemblies.** Two small adapters with the same + dependencies can live in one project. + +### Diagram C — single implementation + +```text +flowchart RL + Persistence["Persistence
(SQL Server inside - not in the name)"] --> Core + Providers["Providers
(one platform)"] --> Abstractions + Adapters["Adapters"] --> Abstractions + Abstractions --> Core + Composition --> Persistence + Composition --> Providers + Composition --> Adapters + API --> Composition +``` + +### Diagram D — multiple implementations + +The unqualified `Persistence` appears only because there is shared mapping code. `Providers` has no +unqualified project, because `GitHub` and `AzureDevOps` share nothing. + +```text +flowchart RL + PersistenceShared["Persistence
shared mappings"] --> Core + PersistenceSql["Persistence.SqlServer"] --> PersistenceShared + PersistencePg["Persistence.PostgreSql"] --> PersistenceShared + + ProvidersGitHub["Providers.GitHub"] --> Abstractions + ProvidersAzdo["Providers.AzureDevOps"] --> Abstractions + Abstractions --> Core + + Composition --> PersistenceSql + Composition --> PersistencePg + Composition --> ProvidersGitHub + Composition --> ProvidersAzdo + API --> Composition + + ProvidersGitHub -. "forbidden: siblings stay independent" .-x ProvidersAzdo +``` + +### Alternative names + +`Domain` for Core, `UseCases` for Application and `Integrations` for Providers are recognised and +need no change. **Do not rename an established project merely to match canonical terminology** — the +rename costs more than the consistency is worth. + +These names are discouraged as project names, because each names a category rather than a +responsibility: `Infrastructure`, `Services`, `Managers`, `Models`, `Helpers`, `Common`, `Utils`, +`Utilities`. + +`Infrastructure` is the one worth calling out. It is a useful *category* — it covers Persistence, +Providers and Adapters — and a poor *project*, because everything technical qualifies. + +## Providers versus Adapters + +The distinction is the one people get wrong most often, and it is not about the vendor. + +- A **Provider** integrates a *named external platform*: GitHub, Azure DevOps. The platform is part of + the requirement — "sync issues from GitHub". +- An **Adapter** implements an *application-defined capability* using some technology: send email, + store a blob. The capability is the requirement; the technology is interchangeable. + +The same vendor can be either. Azure Blob Storage behind `IFileStore` is an Adapter. The Azure DevOps +work-item API, where the product explicitly integrates Azure DevOps, is a Provider. + +### Diagram E — Providers versus Adapters + +```text +flowchart TD + subgraph req["What the requirement says"] + R1["'Sync issues from GitHub'
the platform is named"] + R2["'Send the user an email'
the capability is named"] + end + + R1 --> P["Provider
Providers.GitHub"] + R2 --> A["Adapter
Adapters.Email implements IEmailSender"] + + P --> PA["Owns: client, auth, rate limits,
provider models, error translation"] + A --> AA["Owns: one technology behind
an application-defined interface"] + + PA --> N1["Swapping it changes the product"] + AA --> N2["Swapping it changes nothing the user sees"] +``` + +## Composition and runtime + +Composition is the only project allowed to know every implementation, and nothing is allowed to +depend on it. That is what keeps implementation choice in one place. + +### Diagram F — composition and runtime + +Solid arrows are compile-time references. Dashed arrows are runtime resolution, which runs in the +opposite direction and is the whole point of the arrangement. + +```text +flowchart TD + Abstractions["Abstractions
IConventionRepository"] + Application["Application"] + PersistenceSql["Persistence.SqlServer"] + Composition["Composition"] + API["API"] + + Application -->|depends on| Abstractions + PersistenceSql -->|implements| Abstractions + Composition -->|references| PersistenceSql + Composition -->|references| Application + API -->|references| Composition + + Composition -.->|"registers at runtime"| Resolve["IConventionRepository resolves
to the SQL Server implementation"] + Application -.->|"receives at runtime"| Resolve +``` + +Application never references `Persistence.SqlServer`. It receives an implementation it cannot name. + +## Persistence and migration + +Persistence **owns** migration definitions. Migrator **executes** them. Splitting those two is what +stops an API startup path from mutating a production schema. + +### Diagram G — persistence and migration + +```text +flowchart TD + Persistence["Persistence
DbContext, mappings, migration definitions"] + Migrator["Migrator
executes migrations, controlled seeding"] + API["API"] + DB[("Database")] + + Persistence -->|"migrations live here"| Migrator + Migrator -->|"applies schema, deliberate step"| DB + API -->|"reads and writes only"| DB + + API -.->|"must NOT apply migrations at startup in production"| DB +``` + +## How it is enforced + +| 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` | +| This page's tables match the policy file | `PolicyDocumentationTests` | +| 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. + +### Waivers + +A repository that predates the policy will violate it, and suppressing that silently would make the +policy a lie. A waiver records the departure instead: + +```json +{ + "project": "Infrastructure", + "mapsTo": "Adapters", + "rules": ["naming", "discouragedName"], + "reason": "...", + "reviewedOn": "2026-10-15" +} +``` + +A waiver excuses the **name only**. `mapsTo` keeps the project classified, so every dependency rule +still applies to it. This repository's own `Infrastructure` project is the worked example: it holds +one adapter, so it is not the catch-all the rule is aimed at, and renaming it would churn the wiki, +`AGENTS.md` and `NamespaceRemoveSuffix` for no gain. + +### Diagram I — architecture enforcement + +```text +flowchart TD + A["Discover *.csproj under src/"] --> B["Read ProjectReference and PackageReference"] + B --> C["Classify by name
responsibility, alias, or waiver mapsTo"] + C --> D["Load .config/architecture-policy.json"] + D --> E["Evaluate rules
matrix, siblings, composition, packages, cycles, names"] + E --> F{"Violations?"} + F -- yes --> G["Fail with project, rule, offending reference
and the direction it should run"] + F -- no --> H["Pass"] + D --> I["PolicyDocumentationTests
wiki tables match the policy"] +``` + +## At your scale + +- **A small application.** One or two projects. Use the catalogue to *name* them, not to multiply + them. `Core` plus a host is a complete and correct architecture. +- **A medium application.** Split out the responsibility whose boundary you keep tripping over — + usually Persistence, so Core and Application stop seeing EF Core. +- **A large application.** The full catalogue earns its place, and the tests stop the matrix drifting + as teams change. +- **Any size.** Copy the waiver mechanism before you copy the rules. A policy with no honest escape + hatch gets suppressed wholesale the first time it is inconvenient. + +## See also + +- [Architecture as Tests](Architecture-As-Tests.md) — how a dependency rule becomes a failing build +- [Repository Shape](Repository-Shape.md) +- [Engineering Principles](Engineering-Principles.md) +- [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) diff --git a/docs/wiki/_Sidebar.md b/docs/wiki/_Sidebar.md index fdd182e..f9fe831 100644 --- a/docs/wiki/_Sidebar.md +++ b/docs/wiki/_Sidebar.md @@ -2,12 +2,14 @@ - [Getting Started](Getting-Started.md) - [Engineering Principles](Engineering-Principles.md) - [Repository Shape](Repository-Shape.md) +- [Project Responsibilities](Project-Responsibilities.md) - [Architecture as Tests](Architecture-As-Tests.md) - [Verification Model](Verification-Model.md) - [Contracts and Data Exchange](Contracts-And-Data-Exchange.md) - [Testing and Test Data](Testing-And-Test-Data.md) - [Agent Enablement](Agent-Enablement.md) - [Agent Skills Distribution](Agent-Skills-Distribution.md) +- [Agent Guidance Skills](Agent-Guidance-Skills.md) - [Documentation Convention](Documentation-Convention.md) - [Worked Examples](Worked-Examples.md) - [Adopting This at Your Scale](Adopting-This-At-Your-Scale.md) diff --git a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs new file mode 100644 index 0000000..afb3de1 --- /dev/null +++ b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs @@ -0,0 +1,195 @@ +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("exceptions")] + 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); + + 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 new file mode 100644 index 0000000..58c0af4 --- /dev/null +++ b/src/tests/Architecture.UnitTests/Policy/ProjectGraph.cs @@ -0,0 +1,148 @@ +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 = new(); + Dictionary state = new(); + + 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 new file mode 100644 index 0000000..463013c --- /dev/null +++ b/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs @@ -0,0 +1,193 @@ +using System.Text.RegularExpressions; +using Purview.EngineeringPlaybook.Architecture.Policy; + +namespace Purview.EngineeringPlaybook.Architecture; + +/// +/// Asserts that the wiki agrees with the architecture policy. +/// +/// +/// The policy is stated twice by necessity - once as JSON the tests evaluate, once as a table a human +/// reads. Two statements of one rule drift, and the prose is the copy that drifts silently, because +/// nothing fails when it is wrong. These tests make the documentation falsifiable. +/// See docs/wiki/Project-Responsibilities.md. +/// +public sealed partial class PolicyDocumentationTests +{ + static readonly ArchitecturePolicy Policy = ArchitecturePolicy.Load(); + + static string DocumentationPath => + Path.Combine(ArchitecturePolicy.RepositoryRoot(), "docs", "wiki", "Project-Responsibilities.md"); + + static string Documentation => File.ReadAllText(DocumentationPath); + + [GeneratedRegex( + @"^\| `(?[A-Za-z]+)` \| (?[a-z]+) \| (?[^|]+) \| (?[^|]+) \|$", + RegexOptions.Multiline + )] + private static partial Regex CatalogueRowPattern(); + + [Test] + public async Task Documentation_GivenTheCatalogueTable_ListsEveryResponsibility() + { + // Arrange + var documented = CatalogueRowPattern() + .Matches(Documentation) + .Select(match => match.Groups["name"].Value) + .ToArray(); + + // Act + IReadOnlyList missing = + [ + .. Policy + .Responsibilities.Select(responsibility => responsibility.Name) + .Where(name => !documented.Contains(name, StringComparer.Ordinal)), + ]; + + // Assert + await Assert.That(missing).IsEmpty().Because($"the catalogue table omits: {string.Join(", ", missing)}"); + } + + [Test] + public async Task Documentation_GivenTheCatalogueTable_DoesNotInventResponsibilities() + { + // Arrange + var known = Policy.Responsibilities.Select(responsibility => responsibility.Name).ToArray(); + + // Act + IReadOnlyList invented = + [ + .. CatalogueRowPattern() + .Matches(Documentation) + .Select(match => match.Groups["name"].Value) + .Where(name => !known.Contains(name, StringComparer.Ordinal)) + .Distinct(StringComparer.Ordinal), + ]; + + // Assert + await Assert + .That(invented) + .IsEmpty() + .Because( + $"the catalogue table names responsibilities the policy does not define: {string.Join(", ", invented)}" + ); + } + + [Test] + public async Task Documentation_GivenTheCatalogueTable_MatchesThePolicyDependencies() + { + // Act + List mismatches = []; + foreach (Match row in CatalogueRowPattern().Matches(Documentation)) + { + var name = row.Groups["name"].Value; + var responsibility = Policy.Responsibilities.FirstOrDefault(candidate => + string.Equals(candidate.Name, name, StringComparison.Ordinal) + ); + + if (responsibility is null) + continue; + + var documented = row.Groups["depends"].Value.Trim(); + var expected = + responsibility.MayDependOn.Count == 0 ? "*(nothing)*" : string.Join(", ", responsibility.MayDependOn); + + if (!string.Equals(documented, expected, StringComparison.Ordinal)) + { + mismatches.Add( + $"{name}: the page says '{documented}' but the policy says '{expected}'. " + + "Update docs/wiki/Project-Responsibilities.md, or change the policy if the rule moved." + ); + } + + var documentedCategory = row.Groups["category"].Value.Trim(); + if (!string.Equals(documentedCategory, responsibility.Category, StringComparison.Ordinal)) + mismatches.Add( + $"{name}: category '{documentedCategory}' but the policy says '{responsibility.Category}'." + ); + + var documentedAliases = row.Groups["aliases"] + .Value.Trim() + .Replace("`", string.Empty, StringComparison.Ordinal); + var expectedAliases = responsibility.Aliases.Count == 0 ? "—" : string.Join(", ", responsibility.Aliases); + + if (!string.Equals(documentedAliases, expectedAliases, StringComparison.Ordinal)) + mismatches.Add($"{name}: aliases '{documentedAliases}' but the policy says '{expectedAliases}'."); + } + + // Assert + await Assert + .That(mismatches) + .IsEmpty() + .Because(Environment.NewLine + string.Join(Environment.NewLine, mismatches)); + } + + [Test] + public async Task Documentation_GivenTheDiscouragedNames_ListsEveryOne() + { + // Arrange + var text = Documentation; + + // Act + IReadOnlyList missing = + [ + .. Policy.DiscouragedProjectNames.Names.Where(name => + !text.Contains($"`{name}`", StringComparison.Ordinal) + ), + ]; + + // Assert + await Assert + .That(missing) + .IsEmpty() + .Because($"the page does not mention these discouraged names: {string.Join(", ", missing)}"); + } + + [Test] + public async Task Documentation_GivenEveryWaiver_ExplainsIt() + { + // Arrange + var text = Documentation; + + // Act + // A waiver that nobody wrote down is indistinguishable from a rule that was never enforced. + IReadOnlyList undocumented = + [ + .. Policy + .Waivers.Where(waiver => !text.Contains(waiver.Project, StringComparison.Ordinal)) + .Select(waiver => waiver.Project), + ]; + + // Assert + await Assert + .That(undocumented) + .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/ProjectDependencyPolicyTests.cs b/src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs new file mode 100644 index 0000000..036cf97 --- /dev/null +++ b/src/tests/Architecture.UnitTests/ProjectDependencyPolicyTests.cs @@ -0,0 +1,313 @@ +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}") + ); +} From 37708024db5187f5a49d3900d0ec6f2cf17acf5c Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 09:50:40 +0100 Subject: [PATCH 2/4] feat: add architecture naming guidance and the architecture-testing skill purview-architecture now covers naming ([.]) and dependency direction, pointing at the policy rather than restating it. purview-architecture-testing is new: how to turn a rule a reviewer keeps repeating into a build failure without a brittle suite. It insists the matrix is defined once, that failures name the fix rather than the fact, and that existing code gets a recorded waiver instead of a weakened rule. Extends the review scenarios to cover modular monoliths, multiple persistence implementations, and a repository that already has a catch-all project name. --- docs/wiki/Agent-Guidance-Skills.md | 4 + skills/purview-architecture-testing/SKILL.md | 149 ++++++++++++++++++ skills/purview-architecture/SKILL.md | 48 +++++- .../references/project-responsibilities.md | 107 +++++++++++++ 4 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 skills/purview-architecture-testing/SKILL.md create mode 100644 skills/purview-architecture/references/project-responsibilities.md diff --git a/docs/wiki/Agent-Guidance-Skills.md b/docs/wiki/Agent-Guidance-Skills.md index cb44772..46c65e3 100644 --- a/docs/wiki/Agent-Guidance-Skills.md +++ b/docs/wiki/Agent-Guidance-Skills.md @@ -176,6 +176,10 @@ thing when read. | Repository already has `AddOpenTelemetry` | Existing setup extended, not replaced. No duplicate registration. | | "Should I add a repository over EF Core?" | Only with a named reason other than testability — `Purview.Containers` gives a real database in tests. | | Project file already states the type and persistence | No questions asked at all. | +| "Scaffold a modular monolith with GitHub and Azure DevOps integrations" | `Providers.GitHub` and `Providers.AzureDevOps`, independent of each other. Shared code goes to an abstraction or the unqualified project, never sibling-to-sibling. | +| "Distributed app, SQL Server and PostgreSQL" | `Persistence.SqlServer` and `Persistence.PostgreSql`. An unqualified `Persistence` **only** if there is genuinely shared mapping code. GUID guidance differs per provider. | +| "Add a second adapter for file storage" | Same project as the first if dependencies and lifecycle match; a separate `Adapters.*` project only if they diverge. | +| Repository names a project `Infrastructure` | Flag the catch-all risk, but **do not propose a rename**. Fix wrong dependency directions; record a waiver for the name. | ## At your scale diff --git a/skills/purview-architecture-testing/SKILL.md b/skills/purview-architecture-testing/SKILL.md new file mode 100644 index 0000000..9fb3f11 --- /dev/null +++ b/skills/purview-architecture-testing/SKILL.md @@ -0,0 +1,149 @@ +--- +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. +license: MIT +metadata: + author: Purview-Dev + version: "1.0" +--- + +# Purview architecture testing + +A boundary that only review enforces is a boundary that erodes. The job of this skill is to turn a +rule a reviewer keeps repeating into a test that fails the build, **without** producing a wall of +brittle assertions that everyone disables six months later. + +Prerequisite: know what the projects are *for*. See `purview-architecture`. + +## 1. Only test a rule that is real + +Before writing anything, answer: **has this rule been broken, or is it certain to be?** + +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 + 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 + +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. + +```json +{ + "responsibilities": [ + { + "name": "Core", + "aliases": ["Domain"], + "mayDependOn": ["Shared"], + "mustNotDependOn": ["Application", "Persistence", "Composition"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Microsoft.AspNetCore"] + } + ], + "waivers": [] +} +``` + +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). + +## 3. Read project files, and assemblies + +Two sources, because they catch different things: + +| Source | Catches | Misses | +| -- | -- | -- | +| `.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. + +## 4. The rules worth having + +| Rule | What it catches | +| -- | -- | +| Dependency matrix | Core referencing Persistence; Application referencing a concrete provider | +| Nothing references Composition | A circular implementation choice | +| Sibling independence | `Providers.GitHub` reaching into `Providers.AzureDevOps` | +| Forbidden package prefixes | EF Core in Core; ASP.NET hosting in a domain project | +| Cycle detection | A graph that MSBuild rejects with an unreadable message | +| Catch-all names | `Infrastructure`, `Services`, `Helpers` appearing as project names | +| No production → test reference | A test helper leaking into shipped code | + +## 5. Write failures someone can act on + +This is what decides whether the suite survives. A failure must name: + +1. The offending project. +2. The rule violated. +3. The specific reference that broke it. +4. The direction the dependency *should* run. +5. The likely fix. + +```text +'Application' (Application) references 'Persistence.SqlServer' (Persistence), but Application +must not depend on Persistence. Invert it: Persistence may depend on Application, or introduce +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. + +## 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: + +```json +{ + "project": "Infrastructure", + "mapsTo": "Adapters", + "rules": ["naming"], + "reason": "Holds one adapter, predates the catalogue; renaming churns the wiki and docs for no gain.", + "reviewedOn": "2026-10-15" +} +``` + +Then test 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. + +## 7. Conventions + +- TUnit, matching the repository's other tests. No separate architecture-testing framework or NuGet + package unless reuse across repositories actually justifies one. +- 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. + +## 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? +- 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 +- [Architecture as Tests](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Architecture-As-Tests.md) diff --git a/skills/purview-architecture/SKILL.md b/skills/purview-architecture/SKILL.md index eee48ea..8d4b5a8 100644 --- a/skills/purview-architecture/SKILL.md +++ b/skills/purview-architecture/SKILL.md @@ -66,7 +66,51 @@ do not add the layer. Full guidance and the per-pattern justification tests: `references/proportional-architecture.md`. -## 3. DRY, applied where it pays +## 3. Name what you do create + +A project name states a **responsibility**, optionally qualified by an implementation: + +```text +[.] +``` + +The responsibility decides the rules; the qualifier never changes them. Three rules people usually +get wrong: + +- **Do not qualify a single implementation.** `Persistence` is correct even when it uses SQL Server. + Rename when a second provider arrives, not in anticipation. +- **Do not create an empty parent.** `Persistence.SqlServer` + `Persistence.PostgreSql` with nothing + shared need no `Persistence` project. +- **Multiple implementations do not require multiple assemblies.** Two small adapters with the same + dependencies belong in one project. + +Never use a *category* as a project name — `Infrastructure`, `Services`, `Managers`, `Models`, +`Helpers`, `Common`. Nothing can be excluded from a category, so it grows without limit. And never +rename an established project just to match canonical terminology; `Domain`, `UseCases` and +`Integrations` are recognised aliases. + +Responsibility catalogue, dependency matrix and the naming decision tree: +`references/project-responsibilities.md`. + +## 4. Check the direction + +`A → B` means **A depends on B at compile time** — not a runtime call, a DI registration, an HTTP +request or an Aspire orchestration edge. Confusing those is the most common source of a wrong answer +about whether a dependency is allowed. + +The rules that matter most: + +- Core depends on nothing but `Shared`. +- Application never references a concrete `Persistence`, `Providers` or `Adapters` project. +- Composition references implementations; **nothing references Composition** except a host. +- Sibling implementations (`Providers.GitHub`, `Providers.AzureDevOps`) stay independent. +- Persistence owns migration *definitions*; `Migrator` owns their *execution*. Production API startup + does not apply schema migrations. + +When a boundary is worth having, make it fail the build rather than the review — see +`purview-architecture-testing`. + +## 5. DRY, applied where it pays Reuse the Purview libraries when they remove a real source of truth, not for coverage: @@ -82,7 +126,7 @@ Duplication that matters: validation repeated per entry point, a domain rule res configuration bound in two places, two types that both claim to be the truth. Duplication that does not: two similar-looking methods that answer to different owners. -## 4. Telemetry is not an advanced topic +## 6. Telemetry is not an advanced topic Recommend instrumentation from the first commit, at every project size. A small application gets small telemetry — it does not get none. See `purview-telemetry`. diff --git a/skills/purview-architecture/references/project-responsibilities.md b/skills/purview-architecture/references/project-responsibilities.md new file mode 100644 index 0000000..1bf0a1a --- /dev/null +++ b/skills/purview-architecture/references/project-responsibilities.md @@ -0,0 +1,107 @@ +# Project responsibilities, naming and dependency direction + +The catalogue in brief. The authoritative definition is +[`.config/architecture-policy.json`](https://github.com/purview-dev/engineering-playbook/blob/main/.config/architecture-policy.json), +and the full page — with diagrams — is +[Project Responsibilities](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Project-Responsibilities.md). +Do not restate either here or in another skill; link to them. + +## The catalogue + +`A → B` means A depends on B **at compile time**. + +| Responsibility | Also known as | Owns | May depend on | +| -- | -- | -- | -- | +| `Core` | `Domain` | Entities, value objects, invariants, domain events and errors | Shared | +| `Abstractions` | — | Cross-boundary capability contracts | Core, Shared | +| `Application` | `UseCases` | Use cases, workflows, orchestration | Core, Abstractions, Contracts, Shared | +| `Contracts` | — | Externally exposed request/response types | Core, Shared | +| `Persistence` | — | DbContext, mappings, migration *definitions* | Core, Abstractions, Application, Shared | +| `Providers` | `Integrations` | Integration with a *named external platform* | Core, Abstractions, Application, Shared | +| `Adapters` | — | An *application-defined capability* via one technology | Core, Abstractions, Application, Shared | +| `Composition` | — | DI registration and implementation selection | Everything above | +| `API` | — | HTTP host | Composition, Contracts, ServiceDefaults, … | +| `Worker` | — | Background-processing host | Composition, ServiceDefaults, … | +| `Migrator` | — | *Executes* migrations and controlled seeding | Persistence, Composition, ServiceDefaults | +| `AppHost` | — | Aspire orchestration | The hosts it orchestrates | +| `ServiceDefaults` | — | Telemetry, health, resilience, discovery defaults | Shared | +| `WebApp` | — | Frontend | Contracts, ServiceDefaults, Shared | +| `Shared` | — | Small domain-independent primitives | *nothing* | + +**None of these is mandatory.** A small application with `Core` and one host has a complete and +correct architecture. The catalogue names responsibilities so you can tell when splitting one out +buys something — it is not a list of projects to create. + +## When to split + +| Responsibility | Split it out when | Leave it combined when | +| -- | -- | -- | +| `Core` | A domain model exists | The app is a thin utility | +| `Abstractions` | Two implementations, or the consumer must not see the implementation | One implementation — keep the interface beside it | +| `Application` | Workflows are reused by more than one host | It would only forward CRUD | +| `Contracts` | A client compiles against it without taking the server | The API host is the only consumer | +| `Persistence` | Persistence exists at all | A single-project utility writing a file | +| `Providers` | Two platforms with different SDKs or lifecycles | One platform | +| `Adapters` | One capability, several technologies | One capability, one technology | +| `Composition` | Two hosts compose the same implementations | One host — compose in the host | +| `Migrator` | Schema changes are a deliberate deployment step | Development-only startup migration | +| `Shared` | A primitive is used by two or more responsibilities | Almost always | + +## Naming + +```text +[.] +``` + +| Situation | Name | +| -- | -- | +| One implementation | `Persistence` — do **not** qualify it just because it is SQL Server | +| Several, same dependencies and lifecycle | `Persistence` — one project, several classes | +| Several, genuinely different | `Persistence.SqlServer`, `Persistence.PostgreSql` | +| Several, with shared code | Add `Persistence` alongside the qualified projects | +| Several, nothing shared | Qualified projects only — no empty parent | + +Discouraged as project names, because each names a category rather than a responsibility: +`Infrastructure`, `Services`, `Managers`, `Models`, `Helpers`, `Common`, `Utils`, `Utilities`. + +`Infrastructure` is the common case. It is a useful *category* — covering Persistence, Providers and +Adapters — and a poor *project*, because every technical class qualifies for it. + +**Do not rename an established project** merely to match canonical terminology. A rename costs more +than the consistency is worth, and `Domain`/`UseCases`/`Integrations` are recognised aliases. + +## Providers versus Adapters + +Not a vendor distinction: + +- **Provider** — the requirement names the platform: "sync issues from GitHub". Swapping it changes + the product. +- **Adapter** — the requirement names the capability: "send an email". Swapping the technology + changes nothing the user sees. + +Azure Blob behind `IFileStore` is an Adapter. The Azure DevOps work-item API in a product that +integrates Azure DevOps is a Provider. + +## Dependency rules that catch people out + +1. **Composition is referenced by nothing** except a host. It selects implementations, so it depends + on them; the reverse would make the choice circular. +2. **Application never references a concrete infrastructure project.** It depends on an abstraction + and receives an implementation it cannot name. +3. **Sibling implementations stay independent.** If `Providers.GitHub` needs something from + `Providers.AzureDevOps`, that shared thing belongs in the unqualified project or an abstraction. +4. **Persistence owns migration definitions; Migrator executes them.** Production API startup does + not mutate a schema. +5. **Runtime is not compile time.** DI registration, an HTTP call and Aspire orchestration are not + assembly references. A Worker and an API can share Application without referencing each other. + +## Recommending this to a repository that predates it + +Do not propose a mass rename. Suggest, in order: + +1. Fix dependency *directions* that are actually wrong — those are bugs. +2. Split a project only where a named problem justifies it. +3. Record a waiver for an established name rather than renaming it, so the rule stays enforceable + without churn. + +A waiver excuses the name only; the project still classifies, so every dependency rule still applies. From 7e71ba8ab57cd378cba87dee9e36e0cf671fa01d Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 10:17:10 +0100 Subject: [PATCH 3/4] fix: add the architecture policy schema the file points at The policy declared "$schema": "./architecture-policy.schema.json" and no such file existed. A dangling pointer is worse than none: an editor reports no error, so the format looks validated when nothing checks it. Adds the schema (draft 2020-12), published by $id at the raw GitHub URL so editors can fetch it, matching the purview-build.json convention. Renames the policy's "exceptions" key to "waivers" so the file, the schema and the C# agree. The schema is enforced rather than decorative. PolicySchemaTests validates the policy against it, checks both files agree on the published identifier, and 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. additionalProperties:false throughout, because a misspelled key reads as an absent rule rather than an error, which is the most dangerous mistake a policy file can carry. Validation uses JsonSchema.Net, the same library Purview.Build uses. --- .config/architecture-policy.json | 4 +- .config/architecture-policy.schema.json | 161 ++++++++++++ Directory.Packages.props | 9 + docs/wiki/Project-Responsibilities.md | 40 ++- .../Architecture.UnitTests.csproj | 5 + .../Policy/ArchitecturePolicy.cs | 2 +- .../PolicySchemaTests.cs | 241 ++++++++++++++++++ 7 files changed, 458 insertions(+), 4 deletions(-) create mode 100644 .config/architecture-policy.schema.json create mode 100644 src/tests/Architecture.UnitTests/PolicySchemaTests.cs diff --git a/.config/architecture-policy.json b/.config/architecture-policy.json index 034d841..879d816 100644 --- a/.config/architecture-policy.json +++ b/.config/architecture-policy.json @@ -1,5 +1,5 @@ { - "$schema": "./architecture-policy.schema.json", + "$schema": "https://raw.githubusercontent.com/purview-dev/engineering-playbook/main/.config/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.", "conventions": { @@ -298,7 +298,7 @@ "implementationsMustNotReferenceComposition": true, "siblingImplementationsIndependent": true }, - "exceptions": [ + "waivers": [ { "project": "Infrastructure", "mapsTo": "Adapters", diff --git a/.config/architecture-policy.schema.json b/.config/architecture-policy.schema.json new file mode 100644 index 0000000..daeda82 --- /dev/null +++ b/.config/architecture-policy.schema.json @@ -0,0 +1,161 @@ +{ + "$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/Directory.Packages.props b/Directory.Packages.props index 63357c6..48dd3b7 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -41,6 +41,15 @@ + + + + diff --git a/docs/wiki/Project-Responsibilities.md b/docs/wiki/Project-Responsibilities.md index 8e882c9..37f98b7 100644 --- a/docs/wiki/Project-Responsibilities.md +++ b/docs/wiki/Project-Responsibilities.md @@ -16,7 +16,8 @@ application that combines Core, Application and Persistence in one assembly has it has one project carrying three responsibilities, which is usually correct. The catalogue tells you 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). +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). The tables on this page are checked against it — see [How it is enforced](#how-it-is-enforced). ## Why @@ -315,11 +316,48 @@ flowchart TD | Production code does not reference a test project | `ProjectDependencyPolicyTests` | | Every waiver names a real project, a rule, and a substantive reason | `ProjectDependencyPolicyTests` | | This page's tables match the policy file | `PolicyDocumentationTests` | +| The policy matches its JSON Schema, and the schema can actually reject | `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 policy format + +`.config/architecture-policy.json` is described by a JSON Schema (draft 2020-12) published beside it, +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 +} +``` + +| Key | Purpose | +| -- | -- | +| `version` | Format version. Increment when a change would break a reader that understood the previous shape. | +| `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 | +| `waivers[]` | Deliberate departures — see below | + +Two schema choices do real work: + +- **`additionalProperties: false` everywhere.** A misspelled key is the most dangerous mistake in a + policy file, because it reads as an *absent* rule rather than an error. `mustNotDependOnn` must + fail, not silently disable a boundary. +- **`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. + ### Waivers A repository that predates the policy will violate it, and suppressing that silently would make the diff --git a/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj b/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj index 1df81f1..49b8259 100644 --- a/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj +++ b/src/tests/Architecture.UnitTests/Architecture.UnitTests.csproj @@ -9,4 +9,9 @@ + + + + + diff --git a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs index afb3de1..c6f41bc 100644 --- a/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs +++ b/src/tests/Architecture.UnitTests/Policy/ArchitecturePolicy.cs @@ -31,7 +31,7 @@ public sealed class ArchitecturePolicy [JsonPropertyName("rules")] public PolicyRules Rules { get; init; } = new(); - [JsonPropertyName("exceptions")] + [JsonPropertyName("waivers")] public IReadOnlyList Waivers { get; init; } = []; /// Finds the documented waiver for a project, if one exists. diff --git a/src/tests/Architecture.UnitTests/PolicySchemaTests.cs b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs new file mode 100644 index 0000000..4df381c --- /dev/null +++ b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs @@ -0,0 +1,241 @@ +using System.Text.Json; +using System.Text.Json.Nodes; +using Json.Schema; +using Purview.EngineeringPlaybook.Architecture.Policy; + +namespace Purview.EngineeringPlaybook.Architecture; + +/// +/// Validates .config/architecture-policy.json against its JSON Schema. +/// +/// +/// 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. +/// +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"); + + [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( + CancellationToken cancellationToken + ) + { + // Arrange + 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); + + // Assert + await Assert.That(document?["$id"]?.GetValue()).IsEqualTo(SchemaId); + await Assert + .That(document?["$schema"]?.GetValue()) + .IsEqualTo("https://json-schema.org/draft/2020-12/schema"); + } + + [Test] + public async Task Policy_GivenTheSchema_Validates(CancellationToken cancellationToken) + { + // 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 } + ); + + // Assert + await Assert.That(result.IsValid).IsTrue().Because(Describe(result)); + } + + [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"); + } + + [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( + """ + { + "version": 1, + "responsibilities": [ + { + "name": "Core", + "category": "domain", + "purpose": "The fundamental domain model and its rules.", + "mayDependOn": [], + "mustNotDependOn": [] + } + ], + "waivers": [ { "project": "Infrastructure", "rules": ["naming"], "reason": "because" } ] + } + """ + ); + + // 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( + """ + { + "version": 1, + "responsibilities": [ + { + "name": "Core", + "category": "domain", + "purpose": "The fundamental domain model and its rules.", + "mayDependOn": [], + "mustNotDependOn": [], + "mustNotDependOnn": ["Application"] + } + ] + } + """ + ); + + // 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) + { + 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) + { + if (!results.IsValid) + yield return results; + + yield break; + } + + foreach (var child in children) + { + foreach (var leaf in Leaves(child)) + yield return leaf; + } + } + + /// 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('/', '.'); + } +} From af16861b3a3b71db64161e80c4ed387ca80aaf63 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Thu, 15 Oct 2026 10:44:44 +0100 Subject: [PATCH 4/4] docs: document adopting the architecture policy in another repository The rule engine now ships as Purview.Architecture.Testing, so adoption is a policy file and one derived test class rather than copying the C#. Calls out that [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. --- docs/wiki/Project-Responsibilities.md | 42 +++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/docs/wiki/Project-Responsibilities.md b/docs/wiki/Project-Responsibilities.md index 37f98b7..985bf70 100644 --- a/docs/wiki/Project-Responsibilities.md +++ b/docs/wiki/Project-Responsibilities.md @@ -358,6 +358,48 @@ 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. +### Adopting this in another repository + +The rule engine ships as **`Purview.Architecture.Testing`**, so a repository does not copy the C#. +Adoption is two files: + +**1. 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, + "responsibilities": [ ... ] +} +``` + +**2. One test class:** + +```csharp +[InheritsTests] +public sealed class ArchitectureTests : PurviewArchitectureTests; +``` + +That is the whole adoption. The base class contributes the full rule set — schema validation, +dependency matrix, sibling independence, composition boundary, forbidden packages, cycles, naming, +test references and waiver integrity — each as a separate test, so a failure names the rule. + +> **`[InheritsTests]` is required.** TUnit does not walk base types during discovery. Without the +> attribute the class compiles, the run is green, and **nothing is enforced**. TUnit's analyzer +> reports `TUnit0030` when it is missing; treat that as an error, not a suggestion. + +Two optional overrides: + +| Override | Use when | +| -- | -- | +| `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. + ### Waivers A repository that predates the policy will violate it, and suppressing that silently would make the