diff --git a/.agents/skills/purview-architecture-testing/SKILL.md b/.agents/skills/purview-architecture-testing/SKILL.md new file mode 100644 index 0000000..0619225 --- /dev/null +++ b/.agents/skills/purview-architecture-testing/SKILL.md @@ -0,0 +1,183 @@ +--- +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. Adopts Purview.Architecture.Testing + and a single policy file rather than hand-maintained lists. +license: MIT +metadata: + author: Purview-Dev + version: "2.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. Adopt the package; define the policy once + +The failure mode of architecture tests is a dependency matrix written twice - once in a wiki table and +once in assertions - which then disagree. The prose is the copy that drifts, because nothing fails +when it is wrong. + +So do not write the rule engine. `Purview.Architecture.Testing` ships it, so the matrix, naming, +sibling-independence, composition, forbidden-package, cycle, test-reference and waiver rules are +defined once and shared. Adoption is two files. + +**A policy** at `.config/architecture-policy.json`, pointing at the published schema so an editor +validates it as you type: + +```json +{ + "$schema": "https://raw.githubusercontent.com/purview-dev/architests/main/architecture-policy.schema.json", + "version": 2, + "sourceRoots": ["src"], + "responsibilities": [ + { + "name": "Core", + "aliases": ["Domain"], + "category": "domain", + "purpose": "The domain model, its invariants and its rules.", + "mayDependOn": ["Shared"], + "mustNotDependOn": ["Application", "Persistence", "Composition"], + "forbiddenPackagePrefixes": ["Microsoft.EntityFrameworkCore", "Microsoft.AspNetCore"] + } + ], + "waivers": [] +} +``` + +**One derived test class:** + +```csharp +[InheritsTests] +public sealed class ArchitectureTests : PurviewArchitectureTests; +``` + +That is the whole of the adoption. Each rule is a separate test, so a failure names the rule rather +than collapsing the set into one assertion, and the package validates the policy against its schema +first - a policy that does not match the schema may be silently missing a rule. + +The build-sdk ships a skill for the package itself, `purview-buildsdk-architecture-tests`, which is +authoritative on its API and rule ids. Read it rather than this page when you are extending or +debugging the package. + +Then add a test that the documentation matches the policy, so the table in the wiki is falsifiable +too. Full worked example: +[Project Responsibilities](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Project-Responsibilities.md). + +## 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 | + +The package's rules read the **project files**, because a declared `ProjectReference` is what a +reviewer adds and what the policy governs. Add a small hand-written test that reads the **compiled +assemblies** for the leak a project file cannot show - a namespace from the wrong layer reaching a +public type. + +Classify a project by the **first segment** of its name, so `Persistence.SqlServer` and `Persistence` +are both Persistence - qualification is a naming choice, never a different rule. The package does +this for you; a hand-written assembly test must do it too. + +## 4. The rules worth having + +| 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 | + +The package provides all of these. Do not re-implement one; change the policy instead. + +## 5. Write failures someone can act on + +This is what decides whether the suite survives. A failure must name: + +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. The package already reports this way; keep the +same standard in any rule you add. + +## 6. Waivers, not suppressions + +A repository that predates the policy will violate it. Deleting the test or adding a blanket +`#pragma` makes the policy a lie. Record the departure in the policy 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" +} +``` + +The package tests the waivers themselves: each must name a project that still exists, a rule, and a +substantive reason. A waiver for a deleted project is dead weight that hides the next one. + +Critically, a waiver excuses **the narrowest thing possible** - usually the name. `mapsTo` keeps the +project classified so every dependency rule still applies to it. Prefer a waiver to +`SkipRule`, which removes a rule for the whole repository with no record. + +## 7. Conventions + +- TUnit, matching the repository's other tests, adopted through `PurviewArchitectureTests` with + `[InheritsTests]`. +- Arrange/Act/Assert, one behaviour per test. +- Let the package find the repository root - it probes upward for the policy and stops at the `.git` + boundary. Override `RepositoryRoot` only when the policy does not sit above the test binary. +- Keep the policy file free of anything a rule cannot read; a field no rule consults is a rule that + looks enforced and is not. + +## Checks before you answer + +- Is every rule you added one that has been, or will be, broken? +- Is the matrix defined once, in the policy, with the documentation checked against it? +- Does each failure name the fix, not just the fact? +- Did you record a waiver rather than weaken a rule for existing code? + +## See also + +- `purview-architecture` — what the projects are for, and how they are named +- `purview-buildsdk-architecture-tests` — the package's own skill, authoritative on its API +- [Architecture as Tests](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Architecture-As-Tests.md) diff --git a/.agents/skills/purview-architecture/SKILL.md b/.agents/skills/purview-architecture/SKILL.md new file mode 100644 index 0000000..c04f3d6 --- /dev/null +++ b/.agents/skills/purview-architecture/SKILL.md @@ -0,0 +1,176 @@ +--- +name: purview-architecture +description: >- + Use when deciding the shape of a .NET solution that uses Purview libraries — how many projects, + whether to add a repository or CQRS layer, where validation lives, how an API and its clients share + a contract (Contracts, OpenAPI or Hybrid) and when a BFF is earned — or before generating + substantial implementation code. Establishes project context first, then applies DRY, KISS and + architecture proportional to the actual requirements rather than to an enterprise reference diagram. +license: MIT +metadata: + author: Purview-Dev + version: "1.0" +--- + +# Purview architecture + +The job of this skill is to stop two failures that cost far more than they look: + +- **Architecture bought in advance.** Four projects, a repository interface and a mapping layer for an + application that has one table and one caller. +- **A recommendation made without context.** Advice about database-side GUID generation given to a + stateless CLI, or a Clean Architecture split proposed for a 300-line utility. + +So: establish context, then choose proportionally. Both steps are mandatory, and the first one is the +one that gets skipped. + +## 1. Establish context before deciding + +Before recommending a structure, adding a project, or generating substantial code, you must know four +things. **Read them out of the repository first** — asking a user a question the code already answers +is the most common way this skill is applied badly. + +| Dimension | Read it from | Ask only if | +| -- | -- | -- | +| Complexity (small / medium / large) | Number of functional areas, integrations and external consumers — not file count | The requirements are genuinely not written down anywhere | +| Project type | SDK attribute, `OutputType`, `Microsoft.NET.Sdk.Web`, an AppHost project, a `Main` | The repository is empty | +| Persistence | `PackageReference` to an EF Core provider, `DbContext`, a connection string, a `Purview.Containers` resource | Persistence is being introduced now | +| Telemetry | `Purview.Telemetry.SourceGenerator`, `ILogger` injection, `ActivitySource`, an OTLP exporter | Nothing is configured and you are about to add it | + +Identifier generation is a **conditional** dimension: resolve it only when persistence exists *and* +GUID keys are in play. See `purview-persistence`. + +Judge complexity on requirements, not size. A 2,000-line CLI with one output format is small. A +900-line service that reconciles three upstream systems and owns money movement is not. + +**Do not re-ask.** A dimension answered by repository configuration, existing source, a project file, +an earlier turn in the conversation, or a recorded architectural decision is answered. Ask concise, +targeted questions only for the unknowns that change your recommendation — and if nothing changes, +ask nothing. + +Full decision procedure: `references/project-context-discovery.md`. + +## 2. Choose proportionally + +| Scale | Shape | What you still do not add | +| -- | -- | -- | +| Small | One project, or two when a test project needs a non-trivial subject. Direct implementations. | Repository over `DbContext`, CQRS, a mapping layer, speculative interfaces | +| Medium | Explicit boundaries where complexity already hurts — usually a domain project and a host | Event sourcing, a generic infrastructure framework, a second infrastructure implementation nobody asked for | +| Large | Modularity, independent infrastructure implementations, enforceable boundaries (see the playbook's Architecture-As-Tests page) | Patterns adopted for symmetry rather than for a named problem | + +Architecture is allowed to arrive late. A boundary added when the second caller appears is cheap; a +boundary added in anticipation of a second caller that never appears is permanent. + +**Never add a project for symmetry.** "Domain, Application, Infrastructure, API" is a response to a +problem, not a starting position. If you cannot name the failure a layer prevents in this codebase, +do not add the layer. + +Full guidance and the per-pattern justification tests: `references/proportional-architecture.md`. + +## 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. Choose how an API and its clients share a contract + +When there is an HTTP API and at least one client, choose **Contracts**, **OpenAPI** or **Hybrid** +from the clients that **exist today**. A planned client is noted; it does not count. + +| Clients today | Strategy | Failure it prevents | +| -- | -- | -- | +| All .NET, same solution, shipped in lockstep | **Contracts** — a library of request/response types on the domain's value objects, referenced by the server and every .NET client | Rules drifting, and a weaker second copy of them in the UI | +| At least one non-.NET client, or one that ships independently | **Hybrid** — Contracts is the source; the OpenAPI document and its schemas are generated from it | Non-.NET clients hand-writing shapes, and .NET clients losing their value objects to a generated client | +| No .NET client — partners or the public only | **OpenAPI**, generated from the server's types; no Contracts project | A project with no consumer but the server | + +The rules that do not bend: + +- **OpenAPI is an output, never an input**, while Contracts exists. Never regenerate a C# client from + your own API's document when that client is in the same solution. +- **Never hand-maintain a parallel set of models**, and **the server always validates** — a shared + validator is for user experience, not trust. +- **Contracts is browser-safe** when a WebAssembly client references it: architecture-policy bans, + the trim and AOT analyzers and the browser platform analyzer check it, not a reviewer. +- **A BFF is earned by keeping tokens out of the browser.** It holds the tokens, gives the browser an + `HttpOnly` cookie, forwards with the user's token, and requires an anti-forgery header on every + forwarded call. In Blazor it is the host that serves the WebAssembly client, and a BFF that only + forwards does not reference Contracts. The policy does not define a BFF yet, so these are review + obligations until a future Build SDK release adds Blazor and BFF support. + +Full procedure, version-skew rules, BFF guidance and worked examples: `references/contract-strategy.md`. + +## 6. DRY, applied where it pays + +Reuse the Purview libraries when they remove a real source of truth, not for coverage: + +| Concern | Library | Use it when | +| -- | -- | -- | +| Operation outcomes without exceptions for control flow | `Purview.Results` | Failures are expected and the caller must handle them | +| Primitive obsession, validated identity types | `Purview.ValueObjects` | A value has rules that would otherwise be re-checked at each boundary | +| Schema shared with a TypeScript client | `Purview.ZodSharp` | A contract is duplicated between C# and TypeScript today | +| Local dependencies for dev and integration tests | `Purview.Containers` | Tests need a real database, queue or emulator | +| Logging, tracing and metrics from one declaration | `Purview.Telemetry.SourceGenerator` | Always — see `purview-telemetry` | + +Duplication that matters: validation repeated per entry point, a domain rule restated in the UI, +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. + +## 7. 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`. + +## Checks before you answer + +- Did you read the four context dimensions rather than ask for them? +- Can you name, for each project and each abstraction you are proposing, the failure it prevents here? +- Is anything in your proposal there only because a reference architecture has it? +- If there is an API with clients, did you choose its contract strategy from the clients that exist + today? +- Did you include telemetry? + +## See also + +- `purview-telemetry` — instrumentation at every scale +- `purview-persistence` — storage and GUID-key guidance, only when persistence is real +- [Engineering Principles](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Engineering-Principles.md) +- [Adopting This At Your Scale](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Adopting-This-At-Your-Scale.md) diff --git a/.agents/skills/purview-architecture/references/contract-strategy.md b/.agents/skills/purview-architecture/references/contract-strategy.md new file mode 100644 index 0000000..e281a41 --- /dev/null +++ b/.agents/skills/purview-architecture/references/contract-strategy.md @@ -0,0 +1,236 @@ +# Contract strategy: Contracts, OpenAPI or Hybrid + +When a solution has an HTTP API and one or more clients, something has to define the shapes that cross +between them. There are three ways to do it, and the wrong one fails in one of two directions: + +- **Rules drift.** The client holds a second, weaker copy of the domain rules, and a request the UI + accepted is rejected by the server — or a break only surfaces at runtime. +- **Decoupling bought in advance.** A .NET client in the same solution regenerates its models from + the API's own OpenAPI document, and throws away every value object and rule to buy an independence + the solution does not have. + +The strategy is chosen from the clients that **exist today**. A planned client is noted; it does not +count. "Later" and "in case" do not justify a strategy any more than they justify a project — see +`proportional-architecture.md`. + +## The three strategies + +### Contracts + +A .NET class library holding the request and response types, built on the domain's value objects and +carrying `[ZodSchema]`. The server references it, and so does every .NET client — including a Blazor +WebAssembly client. + +There is one copy of every rule, error code and normalisation, and both sides run the same generated +validators. + +### OpenAPI + +The server publishes an OpenAPI document, and clients generate their models and calls from it. It is +language-neutral and versions independently. + +Generated models are primitives. Value objects, custom ZodSharp rules, error codes, and trimming and +casing are lost; only what JSON Schema can express survives. + +### Hybrid + +Contracts is the **single source of truth**. The OpenAPI document is **generated from it**, with its +component schemas taken from ZodSharp's export — the generated `{T}Schema.BuildJsonSchema()` is built +from the same metadata as the validator, so the document and the validators cannot drift. + +.NET clients reference Contracts. Non-.NET clients generate from the document and take their +validation rules from the exported JSON Schema — for TypeScript, `z.fromJSONSchema` (Zod 4) reads it +back as Zod schemas. The `purview-zodsharp-json` and `purview-zodsharp-source-generator` skills are +authoritative on the export. + +Three facts about the export that a Hybrid design has to plan for: + +- **ZodSharp ships no OpenAPI integration.** It produces JSON Schema; putting that into the OpenAPI + document — for example with a schema transformer in `Microsoft.AspNetCore.OpenApi` — is the API + host's job. Without it, ASP.NET Core describes a value object from its CLR shape, not from its rules, + and the document and the validators drift after all. +- **A non-.NET client still gets only what JSON Schema can express.** A value object exports as its + underlying value, and a rule the exporter cannot represent is listed under `x-zodsharp-unsupported` + rather than dropped. The contract test should fail when that list grows. +- **Exported objects set `additionalProperties: false`.** A client that validates a *response* against + that schema rejects a field the server has added — so "additive changes only" is not safe until + clients tolerate unknown response fields. Validate requests strictly and responses leniently, or the + versioning rule below protects nobody. (The reverse also holds: System.Text.Json ignores unknown + members, so the server accepts a request with an extra key that the schema rejects.) + +## Decision procedure + +1. **List the clients that exist today.** For each, record: + - its language and runtime; + - who owns it; + - whether it lives in the same repository and solution; + - whether it ships **in lockstep** with the server or on its own cadence. + + A planned client is noted, but it does not count yet. +2. **Every client is .NET, in the same solution, and ships in lockstep → Contracts.** This prevents: + - rules drifting between client and server; + - a second, weaker copy of the domain rules in the UI; + - contract breaks that only surface at runtime. + + Publish OpenAPI anyway if documentation or tooling wants it, but generate it from Contracts. It is + an output, never an input. +3. **At least one client is not .NET, or ships independently → Hybrid.** This prevents: + - non-.NET clients hand-writing shapes and rules; + - the .NET clients losing their value objects and rules to a generated client. + + It requires the independent-versioning rules under [Version skew](#version-skew). +4. **No .NET client exists, or the API is a surface whose clients the team does not own (public or + partner) → OpenAPI**, still generated from the server's types. A separate Contracts project is not + earned when the server is its only consumer. +5. **A .NET client in another repository with its own release cadence** prefers Contracts shipped as + a package, if that team accepts the dependency. Otherwise treat it as step 3. + +```text +flowchart TD + A["List the clients that exist today"] --> C{"Every client .NET,
same solution, lockstep?"} + C -- yes --> K["Contracts
OpenAPI only as an output"] + C -- no --> B{"Any .NET client
at all?"} + B -- no --> O["OpenAPI
generated from the server's types"] + B -- yes --> D{"The only other clients are .NET
in another repository,
and they accept a package?"} + D -- yes --> P["Contracts, shipped as a package"] + D -- no --> H["Hybrid
Contracts is the source, OpenAPI generated from it"] +``` + +Steps 3 and 4 do not overlap: a .NET client in the solution alongside a partner or non-.NET client is +step 3, because the .NET client still has value objects and rules to lose. + +## Rules that follow + +- **Never regenerate a C# client from your own API's OpenAPI document when that client is in the same + solution.** It throws away value objects and rules to buy decoupling the solution does not have. +- **Never hand-maintain a parallel set of models** on either side. Two definitions of one wire shape + disagree eventually, and nothing fails when they start to. +- **The server always validates.** Sharing validators with a client is for user experience and + consistency. It is never a substitute for validating on the server, because the client is untrusted. +- **Moving from Contracts to Hybrid must be additive.** Because Contracts is already the source, adding + a non-.NET client means turning on generation of the OpenAPI document and a contract test. Nothing in + Contracts changes. Design for that, but do not build it until the client exists. + +## Contracts project constraints + +- **References only the domain library (value objects) and the runtime Purview packages** — + `Purview.ValueObjects`, `Purview.ZodSharp`, `Purview.Results`. Never the server, persistence, HTTP + clients or hosting, and never an ASP.NET Core bridge such as `Purview.ZodSharp.AspNetCore`; those + belong to the API host. A Contracts project that references the server is a server that every client + now ships. +- **Browser-safe when a WebAssembly client references it.** No file, network or process I/O, safe to + trim, and no reflection-dependent behaviour. The same applies transitively to the domain library, + because the client downloads it too. Make the build enforce it, not a reviewer — three mechanisms, + each catching what the others cannot: + - **Your repository's architecture policy** forbids `Microsoft.AspNetCore` packages + (`forbiddenPackagePrefixes`) and the `Microsoft.AspNetCore.App` framework reference + (`forbiddenFrameworkReferences`) in `Contracts` and `Core`. The playbook's own policy forbids the + packages in `Core` only; the rest arrives with the Build SDK's planned Blazor support, so add it + yourself until then. See `purview-architecture-testing`. + - **`true`** on both projects turns on the trim and AOT analyzers, + so reflection a trimmed client would break on is a warning at build time. `Purview.ValueObjects` + and `Purview.ZodSharp` already set it. + - **``** makes the platform-compatibility analyzer (`CA1416`) + flag an API that is unsupported in the browser, such as starting a process. + + The policy rules fail a test run. The two analyzers produce warnings, and the SDK does not turn on + `TreatWarningsAsErrors` for an ordinary project — set it on `Contracts` and `Core`, or the "clean + build has no warnings" rule is the only thing standing behind them. No analyzer proves the absence of + every kind of I/O; what they miss is a review obligation, and the client's own tests running in the + browser are the last check. +- **Deserialisation mode is a deliberate choice per direction.** The server reads requests through + `Create` (strict), because the client is untrusted. Record what the client uses for responses, and + why — strict catches a server that has drifted, and `Hydrate` trusts a server the client cannot + verify. + +## Version skew + +| Shipping | Strategy | Failure it prevents | What to do | +| -- | -- | -- | -- | +| Lockstep | Contracts, internal apps | A silent failure that looks like a bug, when an old cached client talks to a new server | The server stamps its build on responses; the client prompts the user to reload when the stamps differ | +| Independent | Hybrid, OpenAPI | A deployed client breaking without warning | Additive changes only; a breaking change gets a new route version; a contract test compares the generated document against the last published one and fails on a breaking difference | + +## Backend-for-Frontend + +A BFF is a host, not a layer, and it is held to the same justification test as anything else. +Microsoft's [Blazor Web App with OIDC and the BFF pattern](https://learn.microsoft.com/aspnet/core/blazor/security/blazor-web-app-with-oidc?pivots=with-bff-pattern) +is the authority on the mechanics; what follows is the rules and the failures behind them. + +### What earns it + +- **Keeping tokens out of the browser.** The BFF signs the user in (OpenID Connect), holds the access + and refresh tokens, and gives the browser only an authentication cookie. Script in the page — a + compromised dependency, an XSS hole — cannot read a token that never reached it. This prevents tokens + being stolen from browser storage. +- **It forwards with the user's access token, never its own identity.** The API validates that token + exactly as it would any other, so authorisation and the audit trail stay per-user. A BFF calling with + a client-credentials token turns every action into the BFF's and moves authorisation into the BFF. + +### In a Blazor solution, the BFF is the host + +The project that serves the WebAssembly client is the natural BFF: it already runs on the server, at +the client's origin. Do not add a separate BFF in front of it. + +- **It references the client project** (`Bff → WebApp`) because it serves the client's assets. The + client never references it; they talk over HTTP with the cookie. +- **Same origin is the point.** The client calls the BFF's own routes, the BFF forwards them to the API + (with YARP's `MapForwarder`, for example), and there is no CORS configuration and no third-party + cookie. +- **The client shares Contracts with the API directly.** The WebAssembly client references Contracts, + sends Contracts types, and runs the generated validators. The BFF in the middle passes the bytes + through untouched. + +### Rules that come with a cookie + +Moving the credential into a cookie trades token theft for a different attack, and the BFF has to +close it: + +- **Every forwarded call requires an anti-forgery signal.** The browser attaches the cookie to any + request to the BFF's origin, including one a hostile page triggers. Require a custom header the + client always sends, which a cross-site form cannot set, and reject calls without it — Duende.BFF's + `X-CSRF: 1` is the established form. `SameSite` narrows the attack; it does not replace the check. + This prevents cross-site request forgery. +- **The cookie is `HttpOnly`, `Secure` and `SameSite=Strict` or `Lax`.** `HttpOnly` is what keeps + script from reading it; without it the BFF protects nothing. +- **Data Protection keys are persisted and shared** when the BFF runs on more than one instance or + survives a restart. The cookie is encrypted with them, so per-instance keys sign users out at random. +- **Refresh happens on the server.** The BFF refreshes the access token before it expires and signs + the user out when refresh fails. The client never sees either token. +- **A large session moves to a server-side store.** Tokens kept inside the cookie make it large enough + to be split across several cookies, and the session cannot be revoked server-side. Use a ticket store + when either matters. + +### What it may reference + +- **A BFF that only forwards does not reference Contracts** and never reads payloads. Reading a payload + it does not own makes it a second consumer that has to change in lockstep for no benefit. +- **Prerendering is the common exception.** If the BFF prerenders client components that load data, + it must run those data calls on the server, and it references Contracts to do so. That is a real + reason; turning prerendering off for those components is the alternative, and the choice should be + written down. +- **A BFF that combines several API calls into one response owns its own contract** for those + responses, chosen with this same procedure. That is a separate justification. The forwarding-only + test — "a service layer that forwards" is not earned — still applies to everything else it does. +- **One BFF per client kind, not one per screen.** Web and mobile may each get one when their sign-in + or response needs differ. + +**None of this is enforced yet.** The architecture policy has no BFF responsibility; Blazor and BFF +support is planned for a future Build SDK release. Until then every rule in this section is a review +obligation, and a repository that adds a BFF project records it in its own policy — see +`project-responsibilities.md`. Even once it is in the policy, "Contracts only when it prerenders or +combines" stays a review obligation, because a dependency matrix cannot express a condition. + +## Worked examples + +| Clients today | Strategy | +| -- | -- | +| Blazor WebAssembly app, same solution, lockstep, signed in through a BFF | Contracts, referenced by the API and the WebAssembly client. The BFF is the host that serves the client, holds the tokens and forwards with the user's token; it references Contracts only if it prerenders data. OpenAPI generated for documentation. | +| Blazor WebAssembly app plus a React Native mobile app on its own release cadence | Hybrid. Mobile generates from the document and takes Zod rules from the exported schema. Additive-only versioning. A mobile BFF if sign-in differs. | +| Partner integrations only | OpenAPI, generated from the server's types. No Contracts project. | + +## See also + +- [Contracts and Data Exchange](https://github.com/purview-dev/engineering-playbook/blob/main/docs/wiki/Contracts-And-Data-Exchange.md) + — `Create` and `Hydrate`, and the schema as the source of truth for a wire shape +- [zodsharp](https://github.com/purview-dev/zodsharp) — authoritative on `[ZodSchema]` and JSON Schema + export diff --git a/.agents/skills/purview-architecture/references/project-context-discovery.md b/.agents/skills/purview-architecture/references/project-context-discovery.md new file mode 100644 index 0000000..d40569a --- /dev/null +++ b/.agents/skills/purview-architecture/references/project-context-discovery.md @@ -0,0 +1,115 @@ +# Project context discovery + +The procedure for establishing enough context to make an architectural recommendation, and — just as +importantly — for knowing when to stop asking. + +## The principle + +Every question you ask a user is a question they have usually already answered in the repository. +Reading is free and silent; asking is slow and, when the answer was already in a project file, it +reads as not having looked. So the order is fixed: + +1. Read the repository. +2. Read the conversation so far. +3. Ask only about what remains *and* changes your answer. + +A dimension whose answer would not change your recommendation is not worth a question. If a project +is a stateless CLI, the database GUID strategy is not an unknown — it is irrelevant, and raising it +is noise. + +## Dimension 1 — complexity + +Assess from requirements, never from line or project count. + +| Signal | Suggests | +| -- | -- | +| One functional area, one consumer, no integrations | Small | +| A handful of features, one or two integrations, a single team | Medium | +| Several bounded areas, multiple services or teams, operational obligations (SLOs, auditability, money) | Large | + +Where to look: `README.md`, the wiki, issue or ticket references, the solution file's project list, +`PackageReference` entries naming external systems, the number of distinct hosts. + +Traps: + +- **A big repository is not a complex one.** A monorepo of ten tiny tools is ten small projects. +- **A small repository can be complex.** Few files, hard invariants. +- **Generated code inflates everything.** Discount it. + +## Dimension 2 — project type + +Read it; do not ask it. + +| Evidence | Type | +| -- | -- | +| `Microsoft.NET.Sdk.Web` | Web application or HTTP API — distinguish by whether views/pages or endpoints dominate | +| `OutputType` of `Exe` plus an argument parser | CLI | +| `IsPackable`, no entry point | Reusable library | +| `IHostedService` / `BackgroundService`, `Worker` SDK | Background worker | +| An AppHost project, `Aspire.Hosting` | Distributed application | +| WPF/WinForms/Avalonia/MAUI SDK or `UseWPF` | Desktop application | + +A reusable library carries the strongest constraint on the rest of your advice: it must not configure +global telemetry providers, must not read configuration from the ambient environment, and must not +decide a persistence technology for its consumer. + +## Dimension 3 — persistence + +| Question | Read it from | +| -- | -- | +| Is persistence required at all? | Any data-access `PackageReference`, a `DbContext`, a connection string, a repository type, an Aspire resource | +| Which technology? | The EF Core provider package (`Microsoft.EntityFrameworkCore.SqlServer`, `Npgsql.EntityFrameworkCore.PostgreSQL`), the client package, or the `Purview.Containers` resource in the AppHost | +| Is more than one implementation required? | Two providers referenced, a provider abstraction already present, or an explicitly stated requirement | +| EF Core or something else? | Presence of `Microsoft.EntityFrameworkCore` versus Dapper, a raw client, or a document store SDK | + +"No persistence" is a complete and common answer. When it is the answer, every storage-shaped +recommendation — repositories, migrations, GUID strategies, connection resilience — is out of scope. + +## Dimension 4 — telemetry + +| Question | Read it from | +| -- | -- | +| Is telemetry configured? | `ILogger` injection, `ActivitySource`/`Meter` construction, `AddOpenTelemetry`, a `ServiceDefaults` project | +| Is Purview Telemetry available? | A `Purview.Telemetry.SourceGenerator` `PackageReference`, or interfaces attributed `[Logger]`, `[ActivitySource]`, `[Meter]` | +| Which signals are relevant? | Logs always; traces once a call crosses a process or a dependency; metrics once someone would alert on a rate or a duration | +| Is there an exporter? | An OTLP endpoint in configuration, a console exporter, an Aspire dashboard, an APM package | + +Existing instrumentation is a constraint, not a starting point to replace. Preserve it, extend it, +and never register the same source twice. + +## Conditional dimension — identifier generation + +Resolve **only** when persistence exists and GUIDs are, or are about to be, primary keys. + +- Are GUIDs primary keys, or just surface identifiers? +- Is the value generated in the application or by the database? +- Which provider and version? (This changes the answer — see `purview-persistence`.) +- Does index locality matter at the expected write volume? +- Must the identifier exist before the entity is persisted (outbox, correlation, client-side graph)? + +The last question usually decides it: if the application needs the value before `SaveChanges`, the +database cannot be the generator. + +## Asking well + +When you must ask, ask few and ask concretely. Batch the unknowns into one short set, offer the +likely answers, and state what you will do if the user does not care. + +Good: + +> Two things I can't tell from the repository: is this service expected to own its own database, or +> read from the existing one? And should failures be returned to the caller or thrown? I'll assume +> its own database and `Purview.Results` for expected failures unless you say otherwise. + +Bad: + +> What is the project complexity? What is the project type? Is persistence required? Which database? +> Is telemetry configured? Do you use GUID primary keys? + +The second asks six questions, five of which the repository answers, and one of which is irrelevant. + +## Recording the answer + +When a dimension is resolved by conversation rather than by the repository, that is a decision the +next agent will not be able to read. Write it down — in `AGENTS.md`, a wiki page, or an ADR — so the +question is asked once and never again. diff --git a/.agents/skills/purview-architecture/references/project-responsibilities.md b/.agents/skills/purview-architecture/references/project-responsibilities.md new file mode 100644 index 0000000..d74d2cf --- /dev/null +++ b/.agents/skills/purview-architecture/references/project-responsibilities.md @@ -0,0 +1,119 @@ +# 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; browser-safe when a WebAssembly client references it | 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, including a Blazor WebAssembly client that references Contracts | 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. + +### Backend-for-Frontend: guidance, not yet policy + +A BFF is a host responsibility that the policy does **not** define yet; Blazor and BFF support is +planned for a future Build SDK release. Until then it is guidance, and every rule below is a review +obligation: + +| Owns | May depend on | Split it out when | Leave it combined when | +| -- | -- | -- | -- | +| Sign-in and token custody for a browser or mobile client; forwards with the user's token. In Blazor, the host that serves the WebAssembly client | The WebAssembly client project, ServiceDefaults, Shared — and Contracts only when it prerenders data or combines responses | Tokens must be kept out of the browser, or several API calls must become one response | It would only forward calls that the client could make with a token it already holds safely | + +A repository that adds a BFF project today needs its own responsibility entry or a waiver in its +policy, or the architecture tests reject the project as unclassified. See `contract-strategy.md`. + +## 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` | The contract strategy is Contracts or Hybrid — see `contract-strategy.md` | The API host is the only consumer, which is the OpenAPI strategy || `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. diff --git a/.agents/skills/purview-architecture/references/proportional-architecture.md b/.agents/skills/purview-architecture/references/proportional-architecture.md new file mode 100644 index 0000000..3ed7f55 --- /dev/null +++ b/.agents/skills/purview-architecture/references/proportional-architecture.md @@ -0,0 +1,92 @@ +# Proportional architecture + +Structure is a cost paid now against a failure expected later. Proportional architecture is the +practice of only paying when the failure is named, present, and more expensive than the structure. + +## The justification test + +Before adding a project, an interface, a layer or a pattern, answer in one sentence: + +> **What failure does this prevent in *this* codebase, and has it happened or is it certain to?** + +If the sentence needs the words "in case", "later", "when we scale", or "it's good practice", the +structure is not yet justified. Write the simpler thing. The simpler thing is also the thing that is +cheapest to change when the failure finally does arrive. + +This is not an argument against structure. It is an argument against *unfalsifiable* structure. + +## By scale + +### Small — a utility, CLI, prototype, simple API, limited-scope application + +- One project. A second only when there is a test project, or when a genuinely reusable library + falls out. +- Call the database, the API or the file system directly from the code that needs it. +- Validate at the edge, once. +- Logging from the first commit; traces and metrics when there is something to correlate. + +Do not add: a repository interface over `DbContext`, a service layer that forwards, a mapping layer +between two near-identical shapes, an interface with one implementation and no test double need +(`Purview.Containers` and TUnit.Mocks remove most of that need), a `Domain`/`Application`/ +`Infrastructure` split. + +### Medium — several functional areas, some integrations, moderate domain complexity + +- Introduce a boundary where you can name what it separates. Usually that is domain rules from the + host, so a domain project and an application host. +- An abstraction over an external dependency earns its place when the dependency is genuinely + swappable, or when testing against it is otherwise painful. "Swappable in principle" is not + swappable. +- Dependency instrumentation, trace propagation across the integrations, metrics on the operations + someone would be paged about. + +Do not add: CQRS unless reads and writes have actually diverged in shape, lifetime or scaling; +event sourcing unless the history *is* the product; a second infrastructure implementation that has +no second consumer. + +### Large — complex domain, multiple services or teams, substantial operational requirements + +- Modularity along ownership lines, because the real constraint is people, not code. +- Independent infrastructure implementations where they are genuinely independent. +- Boundaries made enforceable rather than documented — a dependency-direction rule that a wrong + `ProjectReference` fails the build. See the playbook's Architecture-As-Tests page. +- End-to-end tracing, standardised resource attributes, SLIs, retention and sampling policy. + +Even here, the justification test holds per decision. "Large" licenses the *option* to add +structure, not the obligation to add all of it on day one. + +## Patterns and the conditions that earn them + +| Pattern | Earned when | Not earned by | +| -- | -- | -- | +| Repository over EF Core | You must swap the store, or the `DbContext` API genuinely leaks into domain decisions | Testability — EF Core's in-memory and container-backed testing already covers it | +| CQRS | Read and write models have diverged in shape or scaling needs | A desire for symmetry with a reference architecture | +| Event sourcing | The sequence of changes is the product, or audit is a hard requirement | "We might want history" | +| Mediator / in-process bus | Many handlers, genuinely decoupled registration | Three call sites that could be method calls | +| Mapping layer | Two shapes that must diverge and are owned separately | Two shapes that are identical today | +| Extra DI indirection | A real second implementation, or a composition root that must vary | Inversion for its own sake | +| Generic infrastructure framework | Three or more concrete uses exist now | One use and an expectation of more | + +## Evolving incrementally + +The honest sequence is: write the direct implementation; let the second caller arrive; extract the +boundary the second caller revealed. Each step is small, reversible and informed. + +The dishonest sequence is: predict the second caller; build the boundary; never get the second +caller; maintain the boundary forever. The cost is invisible because it is spread across every +future change. + +When you do extract, extract along the seam the code showed you — not the seam the diagram +predicted. + +## What this does not license + +Proportional is not an excuse for: + +- Skipping tests, or skipping telemetry. +- Leaving validation duplicated across entry points. +- Letting domain rules live in the UI or in the database. +- Two sources of truth for the same fact. + +Those are not structure bought in advance; they are defects. Simplicity means fewer moving parts, +not fewer guarantees. diff --git a/.agents/skills/purview-buildsdk-adoption/SKILL.md b/.agents/skills/purview-buildsdk-adoption/SKILL.md new file mode 100644 index 0000000..b1f222c --- /dev/null +++ b/.agents/skills/purview-buildsdk-adoption/SKILL.md @@ -0,0 +1,114 @@ +--- +name: purview-buildsdk-adoption +description: Adopt Purview.BuildSdk in a repository, or troubleshoot an adoption that is not behaving. Use when wiring the SDK into Directory.Build.props for the first time, when a property seems to have no effect, when the resolved version or namespace is wrong, or when a consuming repository builds but the SDK's automatic behaviour is not appearing. Covers the import chain, pre-import property ordering, and version detection from package.json. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Adopting Purview.BuildSdk + +The goal is a working adoption that does not break existing build behaviour. Prefer minimal, +targeted property changes over rewriting a repository's `Directory.Build.props`. + +For the full property list use `purview-buildsdk-configuration`; for auditing an existing +repository's shape, `purview-buildsdk-repository-review`. + +## 1. The import chain + +```xml + + + + + Acme.Sales + + + + +``` + +```xml + + + + +``` + +Both halves are required. A repository with only `Sdk.props` imported gets the evaluation-time +defaults — naming, identity, framework selection — but none of the targets, so validation, +packaging behaviour and the generated files never run. That combination usually presents as "the +properties look right but nothing happens". + +The SDK version comes from `global.json`, which the SDK itself bootstraps. + +## 2. Pre-import ordering is the first thing to check + +**A property that must affect `Sdk.props` evaluation has to be set before the import.** Setting it +in a `.csproj`, or below the import, is too late and silently does nothing — no error, no warning. +This is the single most common adoption failure. + +Set before the import: + +- `NamespacePrefix` +- `UsePackageJsonVersion`, `RootPackageJson` +- `EnableAssemblyNameGeneration` +- `AutoIncludeUsings` +- `PurviewSharedTestingOutputType`, `PurviewTestContextNoWarn` +- any repo-wide testing-framework selection you want every project to inherit + +Everything else is read in `Sdk.targets` and can be set in a project file. + +When a property appears to have no effect, confirm what MSBuild actually resolved before changing +anything else: + +```bash +dotnet msbuild -getProperty:NamespacePrefix -getProperty:RootNamespace +``` + +## 3. Version detection + +Versions come from `package.json`. Resolution order: + +1. An explicit `RootPackageJson`. +2. CI workspace variables. +3. The `.git` root. +4. A nearby `package.json`. + +On success both `Version` and `PackageVersion` are set from the `version` field. + +If the version looks wrong, do not guess at the discovery — make it fail loudly instead: + +```xml +Strict +``` + +`Strict` fails the build when discovery or resolution cannot succeed, rather than silently skipping +it and leaving the default `1.0.0`. Set `VersionDetectionLogEnabled=true` to see what it found. + +## 4. Naming and placement + +Most of the SDK's value is inferred from project names and locations, so an adoption that keeps a +bespoke structure gets correspondingly little. Confirm the repository follows the conventions rather +than introducing new structure to work around them — see `purview-buildsdk-project-layout`, and +`purview-buildsdk-repository-review` for auditing an existing layout. + +The highest-value single check: every test project is named `.Tests`. **The dot is +required.** A project named plainly `UnitTests` is not detected as a test project at all, and +silently gets no runner, no coverage, no `InternalsVisibleTo` and no category. + +## 5. Agent skills are a separate, explicit step + +The SDK does not install agent skills, and nothing it does during a build writes to an agent +directory. It reads which skills each referenced package ships and recommends the command that +installs them; the skills CLI does the installing. See `purview-buildsdk-skill-installation`. + +## Constraints + +- Prefer minimal, targeted property changes over broad `Directory.Build.props` rewrites. +- Do not disable `PurviewAutoSdkPack` unless the consumer explicitly asks to opt out. +- Do not restate an SDK default in a project file. Hand-configuration that merely repeats a default + is pure maintenance cost, and it hides the fact that the default later changed. +- Re-run the build — or the repository's canonical build command — after each change, and confirm + the fix before moving to the next one. diff --git a/.agents/skills/purview-buildsdk-configuration/SKILL.md b/.agents/skills/purview-buildsdk-configuration/SKILL.md new file mode 100644 index 0000000..d82768b --- /dev/null +++ b/.agents/skills/purview-buildsdk-configuration/SKILL.md @@ -0,0 +1,115 @@ +--- +name: purview-buildsdk-configuration +description: Configure Purview.BuildSdk from Directory.Build.props or a .csproj. Use when setting NamespacePrefix, choosing a testing or mocking framework, turning telemetry or SourceLink off, controlling version detection from package.json, tuning the repository bootstrap, making a library browser-safe for a Blazor WebAssembly client, configuring a BFF host, or working out whether a property must be set before the SDK import. Also use when the SDK "did something automatically" and you need the property that controls it. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Purview.BuildSdk configuration + +Use this skill when the question is "what property controls this, and where do I set it?" + +For *why* the SDK classified a project a particular way, use `purview-buildsdk-project-detection` +instead. For where projects and tests should live, use `purview-buildsdk-project-layout`. + +## First rule: know where a property must be set + +A property that changes `Sdk.props` evaluation must be set **before** the SDK import — in +`Directory.Build.props`, above the import. Setting it in a `.csproj` is too late and silently has no +effect. + +Pre-import properties include: + +- `NamespacePrefix` +- `UsePackageJsonVersion`, `RootPackageJson` +- `EnableAssemblyNameGeneration` +- `PurviewSharedTestingOutputType`, `PurviewTestContextNoWarn` +- `AutoIncludeUsings` +- `DisablePurviewBlazorWebAssemblyDefaults` +- repo-wide testing-framework selection you want every project to inherit + +Everything else is read in `Sdk.targets` and can be set in a project file. Prefer repo-wide defaults +in `Directory.Build.props` unless the setting is deliberately project-specific. + +## The properties you will reach for most + +| Property | Default | What it does | +| -- | -- | -- | +| `NamespacePrefix` | *(required)* | The root identity. Required unless `DisableNamespacePrefixCheck=true` | +| `TestingFramework` | `TUnit` | `TUnit`, `Xunit` or `None` | +| `SubstituteFramework` | `TUnitMocks` | `TUnitMocks`, `NSubstitute` or `None` | +| `TestDataFramework` | `Bogus` | `Bogus` or `None` | +| `TargetFramework` | `net10.0` | `netstandard2.0` when `IsRoslynComponent=true` | +| `IsPackable` | `false` | Opts a project into the packable defaults | +| `UsePackageJsonVersion` | `true` | `true`, `false` or `Strict` — version from `package.json` | +| `ExcludePurviewTelemetry` | `false` | Drops the telemetry source generator | +| `DisableSourceLink` | `false` | Drops SourceLink from packable projects | +| `EnableAssemblyNameGeneration` | `true` | `AssemblyName`/`PackageId` follow the resolved `RootNamespace` | +| `PurviewBrowserSafe` | *(unset)* | On a class library a WebAssembly client downloads: trim, AOT and browser-unsupported APIs fail the build | +| `IsBffProject` | name ends `Bff` | Classifies a BFF host; `false` opts out | +| `PurviewBrowserSafeReferenceCheckAsError` | `false` | Makes `PRBLZ0001` (a client reference that is not browser-safe) an error | + +The full set, grouped by area and with the behaviour each one changes, is in +[references/properties.md](references/properties.md). Read it rather than guessing a property name — +several of these have near-miss names that do nothing. + +## Derived properties to inspect, not re-derive + +When explaining what the SDK did, read the value it computed rather than reasoning from the folder +name: + +`PurviewLogicalProjectName`, `PurviewNamespacePrefix`, `PurviewProjectShortName`, `PurviewTestType`, +`RootNamespace`, `AssemblyName`, `PackageVersion`, `TestingType`, `TargetProjectName`, `RepoRoot`, +`RootPackageJson`, `IsBlazorHostProject`, `IsBffProject`, `PurviewBffExposesApi`, `IsAotCompatible`, +`WarningsAsErrors`. + +Ask MSBuild for one directly: + +```bash +dotnet msbuild -getProperty:RootNamespace +dotnet msbuild -getProperty:TestingType -getProperty:TargetProjectName +``` + +## Compiler-visible properties + +The SDK exports its properties to analyzers and generators as `build_property.`. When +authoring an analyzer, read the exported property instead of re-deriving SDK behaviour — the export +is the SDK's answer, and it stays correct when the rules change. The exported set is listed in +[references/properties.md](references/properties.md). + +## Version detection + +1. If `RootPackageJson` is set, that file is used. +2. Otherwise the repo root is discovered from CI variables, `.git`, or a nearby `package.json`. +3. On success, both `Version` and `PackageVersion` come from the `version` field. +4. `UsePackageJsonVersion=Strict` turns a failed discovery into a build failure instead of a silent + fallback. + +## Repository bootstrap + +The SDK bootstraps two repository-wide files from the package: `.editorconfig` and `global.json`. + +- `PurviewRepoBootstrapMode` — `IfMissing` (default), `Always`, `WarnOnDrift`, `Never` +- `DisableAutoCopySdkFiles` — the master off switch +- `PurviewRepoBootstrapCopyRetries` / `...RetryDelayMilliseconds` / `...FailureAsError` + +Every project in a solution writes those same destinations, so the write is race-tolerant by +design: an existing file is never overwritten unless asked, each write is staged into a temporary +file and renamed into place, retries are quiet, and only a failure that survives every retry is +reported. + +**The SDK writes nothing into an agent directory.** It used to mirror a package's `.agents/**` into +the repository; that is gone, along with `EnableAgentFolderInPackage`, `AgentPackDestinationFolder` +and the `PurviewAgentFolder*` properties. Skills have exactly one installer, the skills CLI — see +`purview-buildsdk-skill-installation`. + +## Guidance when changing SDK configuration + +1. Preserve existing defaults unless the task explicitly changes product behaviour. +2. Keep `README.md`, the property declaration, its validation and the shipped skills aligned — a + property documented in one place and absent from another is the failure mode this repository + keeps hitting. +3. A new property that affects import-time behaviour must say so in its documentation. +4. Add the property to the `CompilerVisibleProperty` list if an analyzer would benefit from it. diff --git a/.agents/skills/purview-buildsdk-configuration/references/properties.md b/.agents/skills/purview-buildsdk-configuration/references/properties.md new file mode 100644 index 0000000..1fb1f81 --- /dev/null +++ b/.agents/skills/purview-buildsdk-configuration/references/properties.md @@ -0,0 +1,168 @@ +# Purview.BuildSdk property reference + +Every user-facing property, grouped by what it changes. Defaults are what the SDK applies when the +consuming project supplies nothing. + +## Identity and naming + +| Property | Default | Behaviour | +| -- | -- | -- | +| `NamespacePrefix` | *(required)* | The root identity source. Required unless the check is disabled. Set before the SDK import | +| `DisableNamespacePrefixCheck` | `false` | Stops the build failing when `NamespacePrefix` is unset | +| `EnableAssemblyNameGeneration` | `true` | `AssemblyName` and the default `PackageId` follow the fully evaluated `RootNamespace`, or the full logical project name when suffix-stripping removed a segment (`Shared`, `ServiceDefaults`). Set `false` before the SDK import for standard project-name behaviour | +| `AssemblyIdentifierClassName` | *(derived)* | Renames the generated marker class | +| `DisableGenerateAssemblyIdentifierClass` | `false` (forced `true` for test and shared-testing projects) | Drops the empty marker class (`Assembly`, global namespace) that architecture tests use to pin an assembly by `typeof` rather than by name | +| `DisableGenerateAssemblyInfoClass` | `false` | Drops the generated `AssemblyInfo` | +| `DisableAutoInternalsVisibleTo` | `false` | Drops automatic friend-assembly generation | +| `DisableProjectFileNamingConventionCheck` | `false` | Stops validating that the `.csproj` filename matches its directory | +| `AutoIncludeUsings` | `true` | Controls the SDK-added global usings. Set before the SDK import | + +## Target framework and language + +| Property | Default | Behaviour | +| -- | -- | -- | +| `TargetFramework` | `net10.0` | Applied when neither `TargetFramework` nor `TargetFrameworks` is set. A project that explicitly declares `IsRoslynComponent=true` defaults to `netstandard2.0` | +| `IsRoslynComponent` | *(unset)* | When explicitly `true`: a single `netstandard2.0` target, `LangVersion=latest`, `Nullable=enable`, `TreatWarningsAsErrors=true`, `Deterministic=true`, extended analyzer rules, SourceLink with `EmbedUntrackedSources=true`, no dependency file, telemetry exclusion, and `PrivateAssets=all` on `Microsoft.CodeAnalysis.*` references | +| `IsRoslynComponentOnly` | `true` for Roslyn components | Analyzer-only package: `IncludeBuildOutput=false`, `IncludeSymbols=false`, portable PDB packed beside the analyzer under `analyzers/dotnet/cs/`. Set `false` for a dual-role component using normal library symbol packaging | +| `DisableRoslynCompilerDefaultsValidation` | `false` | Opts out of `ValidateRoslynComponentCompilerSettings`, which fails the pack of a packable Roslyn component missing `LangVersion`, `Nullable`, `TreatWarningsAsErrors` or `EnforceExtendedAnalyzerRules` | +| `PackProjectReferencedSourceGenerators` | `true` | Packable projects include analyzer `ProjectReference` outputs under `analyzers/dotnet/cs/`. Use `Pack="false"` on one reference to opt that one out | + +## Style policy + +| Property | Default | Behaviour | +| -- | -- | -- | +| `DisablePurviewStylePolicyValidation` | `false` | Stops the build failing (`PRSGD0006`–`PRSGD0009`) when the repository `.editorconfig` overrides the modifier policy — `dotnet_style_require_accessibility_modifiers` other than `omit_if_default`, hiding `IDE0040`/`IDE1006`, disabling the Style category in bulk, weakening the `_camelCase` private-instance-field rule, or adding the accessibility rules to `NoWarn`. Entries the SDK injects itself are ignored | +| `PurviewTestContextNoWarn` | `CA1002;CA1012;CA1034;CA1047;CA1050;CA1051;CA1062;CA1064;CA1515;CA1707` | The production API-surface rules exempted in test and shared-testing projects. Test projects keep the strict style contract (`IDE0040`, field naming, formatting, `IDE1006`) but are context aware: public test classes, `Method_Scenario_Expectation` names, exposed fields and unvalidated helper parameters are allowed. Set before the SDK import | +| `DisablePurviewTestContextRuleSet` | `false` | Makes test and shared-testing projects enforce the production API-surface rules too | +| `PurviewPolicyExemptNoWarn` | *(derived)* | The SDK-injected `NoWarn` entries `ValidatePurviewStylePolicy` accepts | + +## Testing + +| Property | Default | Behaviour | +| -- | -- | -- | +| `TestingFramework` | `TUnit` | `TUnit`, `Xunit`, `None` | +| `SubstituteFramework` | `TUnitMocks` | `TUnitMocks`, `NSubstitute`, `None` | +| `TestDataFramework` | `Bogus` | `Bogus`, `None` | +| `CollectCoverage` | `true` for detected test projects | | +| `EnableStaticNativeInstrumentation` | `false` for test projects | | +| `EnableDynamicNativeInstrumentation` | `false` for test projects | | +| `TestingPlatformDotnetTestSupport`, `UseMicrosoftTestingPlatformRunner`, `EnableMicrosoftTestingPlatform` | enabled for TUnit test projects | Microsoft.Testing.Platform wiring | +| `PurviewSharedTestingOutputType` | `Library` | Forced onto `IsSharedTestingProject` projects, with `IsTestProject`/`IsTestingPlatformApplication` cleared, because the test packages otherwise flip them into an executable test host. Set `Exe` before the SDK import to keep the package-driven shape | + +## Packaging + +| Property | Default | Behaviour | +| -- | -- | -- | +| `IsPackable` | `false` | | +| `PurviewAutoSdkPack` | `true` | Packs `Sdk/**/*` into the package at the right root-level paths | +| `PurviewSkillsSourceFolder` | `/skills` | Where authored skills live | + +Packable-project defaults, applied only when the project supplied nothing: +`GenerateDocumentationFile=true`, `IncludeSymbols=true`, `SymbolPackageFormat=snupkg`, +`PublishRepositoryUrl=true`, `EmbedUntrackedSources=true`, `DebugType=portable`. Portable PDBs ship +in the `.snupkg`; the `.nupkg` gets none unless the project opts in. Roslyn-component-only packages +instead default `IncludeSymbols=false` and ship their PDB inside `analyzers/dotnet/cs/`. + +When the repository root is discoverable, the root `README.md` is packed automatically and +registered as `PackageReadmeFile`, unless one is configured already. + +Non-packable projects, including web apps, default `WarnOnPackingNonPackableProject=false` so a +solution-wide pack skips them silently. + +`ContinuousIntegrationBuild` is set only by real CI environment variables. Packability alone never +forces SourceLink's CI-mode dirty-repository checks. + +## Telemetry and SourceLink + +| Property | Default | Behaviour | +| -- | -- | -- | +| `ExcludePurviewTelemetry` | `false` | Removes `Purview.Telemetry.SourceGenerator` | +| `ExcludeMSTelemetryExtension` | `false` | Removes `Microsoft.Extensions.Telemetry.Abstractions`. Only relevant when `ExcludePurviewTelemetry` is also `false` — the whole group is skipped otherwise | +| `SourceLinkPackageName` | `Microsoft.SourceLink.GitHub` | | +| `DisableSourceLink` | `false` | | + +## Blazor WebAssembly, browser-safe libraries and BFF hosts + +| Property | Default | Behaviour | +| -- | -- | -- | +| `IsBffProject` | `true` for a web SDK project named `*Bff`, else `false` | Classifies a BFF host. `false` opts a `*Bff` project out; `true` opts another web project in | +| `IsBlazorHostProject` | *(computed)* | `true` for a web SDK project that references a WebAssembly client. Set `false` to opt out. Never implies `IsBffProject` | +| `PurviewAutoBlazorClientReference` | `true` | A host named `` references its `.Client` WebAssembly client automatically. Skipped when the reference is already declared | +| `DisablePurviewBlazorWebAssemblyDefaults` | `false` | Keeps the generic defaults on a WebAssembly client: packable when asked, Dockerfile container detection, the automatic `Shared` reference. Set before the SDK import | +| `PurviewBrowserSafe` | *(unset)* | `true` on a class library applies `IsAotCompatible`, `SupportedPlatform browser` and the browser-safe `WarningsAsErrors`, each only where the project set nothing | +| `PurviewBrowserSafeWarningsAsErrors` | `true` | `false` keeps the trim and AOT diagnostics as warnings; `CA1416` then falls back to the repository's severity (`suggestion` in the shipped `.editorconfig`) | +| `PurviewBrowserSafeWarningCodes` | `CA1416`, `IL2001`-`IL2128`, `IL3000`-`IL3005`, `IL3050`-`IL3059` | The ids a browser-safe library adds to `WarningsAsErrors` | +| `DisablePurviewBrowserSafeReferenceCheck` | `false` | Turns off `PRBLZ0001`, the check that every project a client (or browser-safe library) references sets `PurviewBrowserSafe` | +| `PurviewBrowserSafeReferenceCheckAsError` | `false` | Reports `PRBLZ0001` as an error | +| `PurviewBffExposesApi` | `true` when the BFF references `Microsoft.AspNetCore.OpenApi`, else `false` | Whether a BFF keeps the `Microsoft.AspNetCore.OpenApi.Generated` interceptor namespace | + +`PurviewBrowserSafe="true"` is also accepted as `ProjectReference` metadata, recording that a +reference was judged browser-safe when it cannot say so itself. + +## Version detection + +| Property | Default | Behaviour | +| -- | -- | -- | +| `UsePackageJsonVersion` | `true` | `true`, `false`, `Strict` | +| `RootPackageJson` | *(discovered)* | Explicit path to the `package.json` to read | +| `EnableVersionDetectionCache` | `true` | | +| `VersionDetectionCacheFile` | *(derived)* | | +| `VersionDetectionLogEnabled` | `false` | Logs the detected version | + +## Repository bootstrap + +| Property | Default | Behaviour | +| -- | -- | -- | +| `DisableAutoCopySdkFiles` | `false` | Master switch for SDK-managed repository file writes | +| `PurviewRepoBootstrapMode` | `IfMissing` | Also `Always`, `WarnOnDrift`, `Never` | +| `BootstrapEditorConfigToRepoRoot` | `true` | | +| `RepositoryEditorConfigFilePath` | *(derived)* | Overrides the destination `.editorconfig` | +| `BootstrapGlobalJsonToRepoRoot` | `true` | | +| `RepositoryGlobalJsonFilePath` | *(derived)* | Overrides the destination `global.json` | +| `PurviewBuildSdkVersionForGlobalJson` | restored package version, else the version it was packed as | | +| `PurviewRepoBootstrapCopyRetries` | `3` | | +| `PurviewRepoBootstrapCopyRetryDelayMilliseconds` | `500` | | +| `PurviewRepoBootstrapCopyFailureAsError` | `true` | `false` downgrades a persistent write failure to a warning | +| `PurviewSuppressCopyRetryWarnings` | `true` | Demotes the built-in copy task's `MSB3026` retry notice to a message | +| `PurviewRepositoryRootMarkers` | `.git/;.github/;.gitattributes;LICENSE.md;AGENTS.md;.agents/;CLAUDE.md;.claude/;package.json` | Ordered markers identifying a repository root, most authoritative first | + +There is no agent-folder mirror. `EnableAgentFolderInPackage`, `AgentPackDestinationFolder`, +`PurviewAgentFolderSourcePath`, `PurviewAgentFolderCopy*`, `PurviewAgentSyncManifestPath` and +`DisableMirroredFileTrackingCheck` were removed: skills are installed by the skills CLI, and +nothing else was left for the mirror to deliver. + +## Skills + +Covered in full by `purview-buildsdk-skill-installation`: `PurviewSkillsRunner`, +`PurviewSkillsRunnerCandidates`, `PurviewSkillsCommand`, `PurviewSkillsPackage`, +`PurviewSkillsAgents`, `PurviewSkillsSource`, `PurviewSkillsInstallRoot`, `PurviewSkillsRepoRoot`, +`EnablePurviewSkillRecommendations`, `PurviewSkillRecommendationsInCI`. + +## Compiler-visible properties + +Exported to analyzers and generators as `build_property.`: + +- `UsePackageJsonVersion`, `RootPackageJson`, `RepoRoot`, `Version`, `PackageVersion` +- `NamespacePrefix`, `DisableNamespacePrefixCheck` +- `TestingFramework`, `SubstituteFramework`, `TestDataFramework` +- `ExcludePurviewTelemetry`, `ExcludeMSTelemetryExtension` +- `EnableAssemblyNameGeneration`, `DisableAutoInternalsVisibleTo`, `DisableGenerateAssemblyInfoClass`, + `DisableGenerateAssemblyIdentifierClass`, `AssemblyIdentifierClassName` +- `IsCSharpProject`, `IsTestProject`, `IsSharedTestingProject`, `IsSharedProject` +- `TestingType`, `TargetProjectName` +- `IsContainerProject`, `IsSdkProject`, `SdkProjectName`, `IsWebProject`, `IsWebSdkProject`, + `IsWorkerSdkProject`, `IsAspireHostProject`, `IsCLIProject` +- `IsBlazorWebAssemblyProject`, `IsRazorClassLibraryProject`, `IsBlazorHostProject`, `IsBffProject` +- `PurviewBrowserSafe`, `PurviewBrowserSafeWarningsAsErrors`, `DisablePurviewBrowserSafeReferenceCheck`, + `PurviewBrowserSafeReferenceCheckAsError`, `DisablePurviewBlazorWebAssemblyDefaults`, + `PurviewAutoBlazorClientReference`, `PurviewBffExposesApi` +- `EditorConfigFilePath`, `RepositoryEditorConfigFilePath`, `BootstrapEditorConfigToRepoRoot` +- `RepositoryGlobalJsonFilePath`, `BootstrapGlobalJsonToRepoRoot`, `DisableAutoCopySdkFiles` +- `PurviewRepoBootstrapMode`, `PurviewRepoBootstrapCopyRetries`, + `PurviewRepoBootstrapCopyRetryDelayMilliseconds`, `PurviewRepoBootstrapCopyFailureAsError` +- `PurviewSuppressCopyRetryWarnings` +- `PurviewSkillsSourceFolder`, + `PurviewSkillsAgents`, `PurviewSkillsInstallRoot`, `PurviewRepositoryRootMarkers`, + `PurviewSkillsRunner` +- `PurviewBuildSdkVersionForGlobalJson`, `CurrentYear`, `AutoGeneratedAssemblyInfoFile`, + `AutoGeneratedAssemblyIdentifierFile` diff --git a/.agents/skills/purview-buildsdk-project-detection/SKILL.md b/.agents/skills/purview-buildsdk-project-detection/SKILL.md new file mode 100644 index 0000000..8573ca4 --- /dev/null +++ b/.agents/skills/purview-buildsdk-project-detection/SKILL.md @@ -0,0 +1,157 @@ +--- +name: purview-buildsdk-project-detection +description: Explain why Purview.BuildSdk classified a project as test, shared, shared-testing, CLI, web, worker, Aspire host, container, Roslyn component, Blazor WebAssembly client, Razor class library, Blazor host or BFF, and why it added a package, a project reference, a namespace or an analyzer automatically. Use when SDK behaviour looks surprising, when a test project gets no runner or no InternalsVisibleTo, or when reasoning about RootNamespace, AssemblyName and TargetProjectName. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Why Purview.BuildSdk did that + +Use this skill when the question is **why** the SDK applied something automatically. + +For where a project should live, use `purview-buildsdk-project-layout`. For the property that turns +a behaviour off, use `purview-buildsdk-configuration`. + +## Diagnose from computed properties, never from the folder name + +Read what the SDK actually computed: + +```bash +dotnet msbuild -getProperty:PurviewLogicalProjectName -getProperty:RootNamespace \ + -getProperty:TestingType -getProperty:TargetProjectName -getProperty:IsTestProject +``` + +The values worth inspecting first: `MSBuildProjectName`, `NamespacePrefix`, +`PurviewLogicalProjectName`, `RootNamespace`, `TestingType`, `TargetProjectName`, `SdkProjectName`, +`IsTestProject`, `IsSharedProject`, `IsSharedTestingProject`, `IsContainerProject`, `IsCLIProject`, +`IsWebSdkProject`, `IsAspireHostProject`, `IsBlazorWebAssemblyProject`, `IsRazorClassLibraryProject`, +`IsBlazorHostProject`, `IsBffProject`. + +## Detection rules + +### Test projects + +Name matches `^.*\.([\w|_]\w*)Tests?$` — a target name, a dot, then any word before `Test`/`Tests`. +The captured word becomes `TestingType`. + +This is a pattern, **not** a lookup. `MyProject.GoldenTests` is detected exactly as readily as +`MyProject.UnitTests`, with `TestingType=Golden` and `Category=Golden`, even though `Golden` is not a +name the SDK ships. + +**The dot is required.** A project named plainly `UnitTests` does not match, and silently gets no +runner, no coverage, no `InternalsVisibleTo` and no category. This is the single most common +surprise. + +Derived: `IsTestProject=true`, `TestingType=`, `PurviewTestType=Tests`, +`TargetProjectName=`. + +The SDK's explicit `TestType` item group governs **`InternalsVisibleTo` generation**, not detection: +a non-test project grants access to `{AssemblyName}.{TestType}Tests` for each name in the group. A +suffix outside it still builds, runs and gets a category — it just cannot see `internal` members. +The 53 names are in [references/test-types.md](references/test-types.md). + +### Shared and shared-testing projects + +Recognised by **exact name**, never by substring. The two lists are in +[references/detection.md](references/detection.md). Derived: `IsSharedProject`, +`IsSharedTestingProject`. + +### SDK and content based + +| Signal | Result | +| -- | -- | +| the project's `Sdk="..."` declaration | `IsSdkProject`, `SdkProjectName` | +| `Microsoft.NET.Sdk.Web` | `IsWebSdkProject=true` | +| `Microsoft.NET.Sdk.Worker` | `IsWorkerSdkProject=true` | +| SDK starts with `Aspire.Sdk.Host` or `Aspire.AppHost.Sdk` | `IsAspireHostProject=true` | +| `Dockerfile`, `dockerfile` or `Dockerfile.dev` in the project directory | `IsContainerProject=true` | +| name ends with `CLI`, `Console`, `CommandLine`, `QuickStart`, `QuickStarts` | `IsCLIProject=true` | +| `Microsoft.NET.Sdk.BlazorWebAssembly` | `IsBlazorWebAssemblyProject=true` | +| `Microsoft.NET.Sdk.Razor` | `IsRazorClassLibraryProject=true` | +| a web SDK project whose name ends with `Bff` (case-sensitive, like `CLI`), or that sets `IsBffProject=true` | `IsBffProject=true` | +| a web SDK project that references an `IsBlazorWebAssemblyProject` project | `IsBlazorHostProject=true` | + +`IsBffProject=false` opts a `*Bff` project out. `IsBlazorHostProject` is **never** inferred into +`IsBffProject`: a host that keeps no tokens is not a BFF, and BFF rules do not describe it. + +`IsBlazorHostProject` is known at evaluation for the ``/`.Client` pairing. A host +referencing a client under any other name is resolved during the build, so ask for it with the +target that resolves it: + +```bash +dotnet msbuild -t:ResolvePurviewBlazorHostProject -getProperty:IsBlazorHostProject +``` + +What each Blazor and BFF classification applies - and the browser-safe library check - is in +[references/blazor.md](references/blazor.md). + +## Namespace and identity + +1. `PurviewLogicalProjectName` is `NamespacePrefix` plus the project name, de-duplicated when the + project name already starts with the prefix's tail. +2. `RootNamespace` defaults to `PurviewLogicalProjectName`. +3. Known suffixes are stripped from `RootNamespace` — shared and shared-testing names, and segments + such as `Core`, `EF`, `Shared`, `ClientShared`, `ServiceDefaults`. +4. Test suffixes are stripped too, so `Acme.Api.UnitTests` maps back to `Acme.Api`. +5. `AssemblyName` and `PackageId` default to the fully evaluated `RootNamespace` — except where + stripping removed a segment, where they use the full `PurviewLogicalProjectName` so the artifact + stays distinct from its parent. Test and shared-testing projects keep their detected suffix. + Explicit `AssemblyName`/`PackageId`/`RootNamespace` always win. +6. The naming defaults are applied during `Sdk.props`, before the Microsoft SDK computes + `TargetName`, so the compiled output name always matches `AssemblyName`. + +## Automatic packages and references + +- **Non-test C# projects**: SourceLink (unless `DisableSourceLink`), the Purview telemetry packages + (unless `ExcludePurviewTelemetry`), `GenerateDocumentationFile=true`, and `InternalsVisibleTo` + generation (unless `DisableAutoInternalsVisibleTo`). +- **Test and shared-testing projects**: test-friendly `NoWarn` defaults, not packable or + publishable, and the packages selected by `TestingFramework`, `SubstituteFramework` and + `TestDataFramework`. TUnit test projects get Microsoft.Testing.Platform wiring automatically. + Shared-testing projects skip the runnable test package. +- **Packable projects**, **Roslyn components** and **special project types** (CLI, container, web, + Aspire host): see [references/detection.md](references/detection.md). +- **Blazor WebAssembly clients, Blazor hosts, BFFs and browser-safe libraries**: see + [references/blazor.md](references/blazor.md). + +Project-reference probing is layout-driven and covered by `purview-buildsdk-project-layout`. + +## The shipped analyzers + +The SDK adds `Purview.BuildSdk.Analyzers` as an `` item to every C# project, so its rules +surface in command-line builds and in Visual Studio. The code-fix assembly ships beside it. + +| Rule | What it enforces | +| -- | -- | +| `PDS0002` | Extensions classes live under `Extensions//` | +| `PDS0003` | Explicit types with target-typed `new()` | +| `PDS0004` | Correct acronym capitalisation | +| `PDS0008` | A BFF's authentication cookie is not set `HttpOnly = false` | +| `PDS0009` | A BFF's authentication cookie is not set `SameSite = None` | + +`PDS0004` follows .NET naming guidance: well-known framework spellings (`Sql`, `Guid`, `Uuid`, +`Url`, `Dns`, `Tcp`, `Http`, `Xml`, `Db`, …) are exempt — `Db` among them, so `DbContext`, +`DbConnection` and `DbSet` are never flagged. A small set is still uppercased: `Api`, `Ai`, `Ui`, +`Io`, `Os`, `Cpu`, `Gpu`, `Cli`, `Gui`, `Ram`, `Ssh`. Members mandated by a contract — interface +implementations and base-class overrides — are never renamed. + +Customise through `dotnet_analyzer_configuration.pds0004.allowed_words` (exempt segments), +`.acronym_map` (segment renames, e.g. `Sql:SQL`, or `Db:DB` to re-enable it) and +`.allowed_identifiers` (brand names, exact or word-boundary prefix, first match wins — `CosmosDb` +covers `CosmosDbServer`). All three merge with and override the shipped defaults. + +The shipped `.editorconfig` marks generated content (`Migrations/`, `*.g.cs`, `Generated/`, +`*.Designer.cs`, `obj/`, `bin/`) as `generated_code = true`, and suppresses namespace-conflict +diagnostics under `Extensions/`, so no `#pragma` suppressions are needed there. + +Two Visual Studio refactorings ship with them, both described in +[references/detection.md](references/detection.md). + +## When changing a detection rule + +1. Keep the `.csproj` filename equal to its directory name. +2. Update `README.md` and the shipped skills in the same change — a rule true in code and false in + prose is this repository's recurring failure. +3. Add or update the integration test in `src/tests/BuildSdk.IntegrationTests/` that pins it. diff --git a/.agents/skills/purview-buildsdk-project-detection/references/blazor.md b/.agents/skills/purview-buildsdk-project-detection/references/blazor.md new file mode 100644 index 0000000..c65d153 --- /dev/null +++ b/.agents/skills/purview-buildsdk-project-detection/references/blazor.md @@ -0,0 +1,105 @@ +# Blazor WebAssembly, browser-safe libraries and BFF hosts + +Every behaviour below applies only to the project type it names, and each one names the failure it +prevents. Nothing here changes a project that is not a WebAssembly client, a Blazor host, a BFF or a +`PurviewBrowserSafe` library. + +## Blazor WebAssembly client (`IsBlazorWebAssemblyProject`) + +| Applied | Failure it prevents | Opt out | +| -- | -- | -- | +| `IsPackable=false` | packing an app's `wwwroot` as a package nobody can consume | `IsPackable=true` in the project | +| `IsContainerProject=false`, so no `PublishAot`, `InvariantGlobalization` or Docker defaults | `PublishAot` on a client is a publish that fails, and the Blazor SDK owns globalisation | `DisablePurviewBlazorWebAssemblyDefaults=true` before the SDK import | +| no automatic `../Shared*/Shared*.csproj` reference | a server-side library joining the browser download because of a folder name | `DisablePurviewBlazorWebAssemblyDefaults=true` before the SDK import | +| the browser-safe reference check (`PRBLZ0001`) | a library joining the download without anyone deciding it was safe there | `DisablePurviewBrowserSafeReferenceCheck=true` | + +Globalisation, trimming mode and the Blazor runtime settings are deliberately left alone: the Blazor +SDK owns them, and a second default would compete with it. + +### The automatic packages + +The SDK adds `Purview.Telemetry.SourceGenerator` and `Microsoft.Extensions.Telemetry.Abstractions` +to every non-test project, a client included. Both were checked rather than assumed: + +- the generator is analyzer-only (`PrivateAssets=all`), so nothing of it is downloaded; +- its generated `ActivitySource`, `ILogger` and `Meter` code, and `Telemetry.Abstractions` itself, + publish with no trim warnings and run on the WebAssembly runtime + (`BlazorWebAppTests.AutomaticTelemetryPackages_RunInsideTheWebAssemblyRuntime` boots it); +- the SDK adds no exporter. An OTLP exporter cannot work from a browser, which is exactly why the SDK + never adds one - that is the application's choice. + +Nothing is therefore excluded from a client by default. `ExcludePurviewTelemetry` and +`ExcludeMSTelemetryExtension` still remove them. + +## Blazor host (`IsBlazorHostProject`) + +A web SDK project named `` beside a WebAssembly client named `.Client` is the Blazor Web +App pairing. The host gets an automatic `ProjectReference` to the client - probed the same way a test +project's target is, and skipped when the project already references it - so the client's framework +files are served. Opt out with `PurviewAutoBlazorClientReference=false`. + +`IsBlazorHostProject` says nothing about `IsBffProject`. In a Blazor Web App the host is the BFF only +if it keeps the tokens. + +## BFF host (`IsBffProject`) + +- **No packages are added.** YARP, OpenID Connect and any BFF library are the application's choice, + and Duende.BFF carries a commercial licence. +- The `Microsoft.AspNetCore.OpenApi.Generated` interceptor namespace every other web project gets is + removed, because a BFF that only forwards has no API document to generate. A BFF referencing + `Microsoft.AspNetCore.OpenApi` keeps it; `PurviewBffExposesApi` decides explicitly. +- `PDS0008` reports `HttpOnly = false` on the `CookieAuthenticationOptions` cookie (script could read + the session). `PDS0009` reports `SameSite = None` on it (cross-site requests would carry it). Only a + constant assigned to that cookie is reported - never the antiforgery cookie, which script may + legitimately read, nor the OpenID Connect correlation and nonce cookies, which need + `SameSite=None`. + +Deliberately **not** checked, because a build cannot check them precisely: + +- **a persisted Data Protection key ring.** The call that persists it (`PersistKeysTo*`) is routinely + made in a shared ServiceDefaults project, which the BFF's compilation cannot see, so the rule would + fire on correctly configured BFFs - and a noisy rule gets suppressed wholesale. +- **anti-forgery on forwarded routes.** It stays a review obligation. + +## Browser-safe library (`PurviewBrowserSafe=true`) + +Applies to class libraries only, and only what the project has not set: + +- `IsAotCompatible=true` (on a TFM that supports it), which turns on `IsTrimmable` and the trim, + single-file and AOT analyzers; +- ``, so `CA1416` reports an API the browser does not support; +- `WarningsAsErrors` for every trim (`IL2001`-`IL2128`), single-file (`IL3000`-`IL3005`) and AOT + (`IL3050`-`IL3059`) diagnostic and `CA1416`, listed in `PurviewBrowserSafeWarningCodes`. + `TreatWarningsAsErrors` is not touched. + +`CA1416` is `suggestion` in the shipped `.editorconfig`. The compiler's `/warnaserror+:CA1416` +outranks an `.editorconfig` severity, so the error still fires without the repository's setting +changing. The consequence of `PurviewBrowserSafeWarningsAsErrors=false` is that the trim and AOT +diagnostics stay warnings while `CA1416` falls back to the repository's configured severity. + +What it cannot catch: file and network I/O. `File` works against the browser's in-memory file system +and `HttpClient` works through `fetch`, so neither is annotated as unsupported, and no analyzer reports +them. Reviewing for them stays a human obligation. + +### The reference check (`PRBLZ0001`) + +A WebAssembly client asks each `ProjectReference` - through an MSBuild call on the referenced project, +never by guessing from its name - for its `PurviewBrowserSafe` value, and warns for each one that does +not set it. A browser-safe library runs the same check over its own references, so the guarantee +covers the whole download, not only the first level. + +- `PurviewBrowserSafe="true"` metadata on the `ProjectReference` records the decision for a project + that does not use this SDK. +- References with `ReferenceOutputAssembly="false"` or an `OutputItemType` are not downloaded and are + not checked. Package references are not checked either. +- `PurviewBrowserSafeReferenceCheckAsError=true` makes it an error. + +### Purview packages a Contracts library builds on + +Checked against a `PurviewBrowserSafe` library, then run on the WebAssembly runtime: + +| Package | Result | +| -- | -- | +| `Purview.Results` 1.0.4 | clean | +| `Purview.ZodSharp` 2.4.2 | clean, including `[ZodSchema]` | +| `Purview.ValueObjects` 1.2.3 | the runtime assembly is AOT-compatible, but the JSON converter the generator emits for a scalar calls the reflection-based `JsonSerializer.Serialize`/`Deserialize(..., options)` overloads, which is `IL2026` and `IL3050` - errors in a browser-safe library. Set `GenerateJsonConverter = false` on the scalar until the generator writes the value directly. | diff --git a/.agents/skills/purview-buildsdk-project-detection/references/detection.md b/.agents/skills/purview-buildsdk-project-detection/references/detection.md new file mode 100644 index 0000000..85fde09 --- /dev/null +++ b/.agents/skills/purview-buildsdk-project-detection/references/detection.md @@ -0,0 +1,83 @@ +# Detection detail + +## Shared project names (exact) + +`Shared`, `SharedFramework`, `SharedInfrastructure`, `SharedInfra`, `SharedUtilities`, +`SharedUtils`, `SharedLibrary`, `SharedLib`, `SharedHelpers` + +## Shared testing project names (exact) + +`SharedTestingFramework`, `SharedTestingInfrastructure`, `SharedTestingInfra`, +`SharedTestingUtilities`, `SharedTestingUtils`, `SharedTestingLibrary`, `SharedTestingLib`, +`SharedTestingHelpers` + +These are matched whole, not as substrings. `SharedModels` is not a shared project. + +A shared-testing project is forced to `PurviewSharedTestingOutputType` (default `Library`), with +`IsTestProject` and `IsTestingPlatformApplication` cleared, because the test packages otherwise flip +it into an executable test host. Set `PurviewSharedTestingOutputType=Exe` before the SDK import to +keep that package-driven shape. + +## Packable projects + +Defaults applied only where the project supplied nothing: `GenerateDocumentationFile`, +`IncludeSymbols`, `SymbolPackageFormat=snupkg`, `PublishRepositoryUrl`, `EmbedUntrackedSources`, +`IncludeSource`, `DebugType=portable`. + +- Portable PDBs ship in the `.snupkg`. The `.nupkg` receives none unless the project opts in. +- The repository-root `README.md` is packed and registered as `PackageReadmeFile` when it exists and + nothing else is already being packed as one. +- Non-packable projects, web apps included, default `WarnOnPackingNonPackableProject=false`, so a + solution-wide `dotnet pack` skips them silently. +- `ContinuousIntegrationBuild` is set only by real CI environment variables. Packability alone never + forces SourceLink's CI-mode dirty-repository checks. + +## Roslyn components + +A project that explicitly declares `IsRoslynComponent=true` gets: + +- a single `netstandard2.0` target, `LangVersion=latest`, `Nullable=enable`, + `TreatWarningsAsErrors=true`, `Deterministic=true`, extended analyzer rules +- SourceLink with `EmbedUntrackedSources=true`, no dependency file, telemetry excluded +- compiler-generated output under the framework-specific intermediate directory +- `PrivateAssets=all` on `Microsoft.CodeAnalysis.*` and `Microsoft.CodeAnalysis.Analyzers`, so they + never leak into the packed nuspec + +`IsRoslynComponentOnly` defaults to `true`: `IncludeBuildOutput=false`, `IncludeSymbols=false`, no +`.symbols.nupkg` or `.snupkg`, and the analyzer PDB shipped inside the main `.nupkg` under +`analyzers/dotnet/cs/` beside the assembly (`PurviewPackAnalyzerPdb=true`). Set it to `false` for a +dual-role component that wants normal library symbol packaging. + +`ValidateRoslynComponentCompilerSettings` fails the pack of a packable Roslyn component missing +`LangVersion`, `Nullable`, `TreatWarningsAsErrors` or `EnforceExtendedAnalyzerRules`. Opt out with +`DisableRoslynCompilerDefaultsValidation=true`. + +`PackProjectReferencedSourceGenerators` (default `true`) makes a packable project include analyzer +`ProjectReference` outputs and their runtime dependencies under `analyzers/dotnet/cs/`. Set it +`false` globally, or `Pack="false"` on one analyzer reference. + +## Special project types + +| Type | Applied | +| -- | -- | +| CLI | `OutputType=Exe`, `appsettings*.json` included as content | +| Container | `InvariantGlobalization`, `PublishAot`, Linux Docker defaults, container tooling packages | +| Web SDK | `Microsoft.AspNetCore.OpenApi.Generated` added to `InterceptorsNamespaces`, unless marked as a separate web-project mode | +| Aspire host | `OutputType=Exe` | +| Blazor WebAssembly client | not packable, no container defaults, no automatic `Shared` reference, the browser-safe reference check - see [blazor.md](blazor.md) | +| Blazor host | an automatic reference to its `.Client` - see [blazor.md](blazor.md) | +| BFF | no packages, no OpenAPI interceptor unless it exposes an API, `PDS0008`/`PDS0009` - see [blazor.md](blazor.md) | +| `PurviewBrowserSafe` library | `IsAotCompatible`, `SupportedPlatform browser`, trim/AOT/`CA1416` as errors - see [blazor.md](blazor.md) | + +## The Visual Studio refactorings + +Two ship with the code-fix assembly, both serving the `PDS0002` convention: + +- **Split extensions class into one class per receiver type** — offered on a static extensions class + targeting several receiver types. It produces one `Extensions` class per receiver + (a generic `this TBuilder where TBuilder : IHostApplicationBuilder` receiver becomes + `HostApplicationBuilderExtensions`) and places each under `Extensions//`. +- **Move extensions class to conventional location** — offered on a misplaced single-receiver + extensions class. It re-paths the file to `Extensions//`, fixes the namespace + to the receiver's, and updates `using` directives in other referencing documents so the move + compiles. diff --git a/.agents/skills/purview-buildsdk-project-detection/references/test-types.md b/.agents/skills/purview-buildsdk-project-detection/references/test-types.md new file mode 100644 index 0000000..dace44f --- /dev/null +++ b/.agents/skills/purview-buildsdk-project-detection/references/test-types.md @@ -0,0 +1,35 @@ +# The shipped `TestType` group + +These 53 names drive **`InternalsVisibleTo` generation**, not test detection. A non-test project +grants friend access to `{AssemblyName}.{TestType}Tests` for each name here. + +Detection is the pattern `^.*\.([\w|_]\w*)Tests?$`, so a suffix outside this group is still detected, +still runs and still gets its category — it simply cannot reach the target project's `internal` +members. Prefer a name from this group unless the test type genuinely needs its own word. + +```text +Unit Integration E2E EndToEnd Acceptance +Functional Performance Load Smoke Stress +Regression Security Chaos Scenario System +Threat BlackBox WhiteBox GrayBox GreyBox +Accessibility Usability Visual Interactive Localization +Globalization Environment Configuration Deployment Architecture +Contract Component Sanity Property Fuzz +Mutation Snapshot Approval Benchmark Capacity +Concurrency Endurance Soak Spike Failover +Recovery Resilience Compatibility Interoperability Migration +Upgrade Penetration Compliance +``` + +## The five to reach for first + +`Unit`, `Integration`, `E2E`, `Functional`, `Contract`. Everything else is available, but a reader +has to ask what it means — use one only when the test type itself carries operational weight. + +## What a suffix buys you + +| | detected as a test project | gets a category | gets a runner and coverage | sees `internal` members | +| -- | -- | -- | -- | -- | +| `Api.UnitTests` | yes | `Unit` | yes | yes | +| `Api.GoldenTests` | yes | `Golden` | yes | **no** | +| `UnitTests` *(no dot)* | **no** | no | **no** | no | diff --git a/.agents/skills/purview-buildsdk-project-layout/SKILL.md b/.agents/skills/purview-buildsdk-project-layout/SKILL.md new file mode 100644 index 0000000..6c19c3d --- /dev/null +++ b/.agents/skills/purview-buildsdk-project-layout/SKILL.md @@ -0,0 +1,161 @@ +--- +name: purview-buildsdk-project-layout +description: Decide where a project goes and what to call it in a repository that uses Purview.BuildSdk. Use when creating, moving, splitting or renaming a project or test project, choosing a test suffix, aligning namespaces and assembly identity with NamespacePrefix, laying out a Blazor Web App host, WebAssembly client and browser-safe Contracts library, or relying on the SDK's automatic project references. Also use when rationalising a repository's shape so the SDK's conventions work without overrides. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Project layout and naming for Purview.BuildSdk + +Use this skill when physically adding, moving, splitting or renaming a project. + +For *why* the SDK classified an existing project a particular way, use +`purview-buildsdk-project-detection`. For the property that changes a behaviour, use +`purview-buildsdk-configuration`. + +## First principle + +**Treat naming and placement as configuration, not decoration.** + +The SDK infers namespaces, assembly identity, test categories, package wiring and project references +from a small set of conventions. The closer a repository sits to them, the fewer overrides it needs. + +The corollary matters just as much: **preserve the host repository's existing layout first.** Only +introduce new structure when no established pattern exists. + +## Canonical layout + +```text +/ + package.json version source + .editorconfig bootstrapped by the SDK + skills/ authored agent skills, if any + src/ + .slnx + src//.csproj + tests/.Tests/.Tests.csproj +``` + +- The `.csproj` filename must equal its containing directory name, unless + `DisableProjectFileNamingConventionCheck=true`. +- Source projects under `src/`, test projects under `tests/`. +- Place a new project beside similar ones — same language, layer and test type. +- One test type per project by default. + +A repository that already uses a different shape keeps it; add to it rather than converting it. + +## Naming and identity + +`NamespacePrefix` is the root identity source. Keep project names short and let the SDK apply the +prefix: + +| `NamespacePrefix` | Project | `RootNamespace` | +| -- | -- | -- | +| `Aspire` | `Hosting` | `Aspire.Hosting` | +| `Acme.Sales.RegionalPipeline` | `Identity.API` | `Acme.Sales.RegionalPipeline.Identity.API` | +| `Acme.Sales.RegionalPipeline` | `Identity.Core` | `Acme.Sales.RegionalPipeline.Identity` | + +`AssemblyName` and `PackageId` follow the resolved `RootNamespace` — except where suffix-stripping +removed a segment (`Core`, `Shared`, `ServiceDefaults`), in which case they keep the fuller logical +name so the artifacts stay distinct from their parent. Explicit values always win, and +`EnableAssemblyNameGeneration=false` (before the SDK import) restores standard behaviour. + +Do not hand-author an alternate namespace scheme when one is already in use. + +## Test project naming + +Test detection is a **pattern**, not a list: `^.*\.([\w|_]\w*)Tests?$`. A target name, a dot, then +any word before `Test` or `Tests`. The captured word becomes `TestingType` and the baseline category. + +**The dot is required.** A project named plainly `UnitTests`, with no target in front of it, is not +detected as a test project at all — no runner, no coverage, no `InternalsVisibleTo`, no category. +Always name a test project `.Tests`. + +Prefer these by default: `UnitTests`, `IntegrationTests`, `E2ETests`, `FunctionalTests`, +`ContractTests`. Use another suffix only when the test type itself carries operational meaning. + +A suffix outside the SDK's shipped `TestType` group still builds, runs and gets its category — it +just cannot reach the target project's `internal` members, because that group drives +`InternalsVisibleTo` generation. The full list is in `purview-buildsdk-project-detection`. + +## Shared projects: exact names only + +The SDK recognises these names exactly — a near miss gets none of the behaviour. + +Shared: `Shared`, `SharedFramework`, `SharedInfrastructure`, `SharedInfra`, `SharedUtilities`, +`SharedUtils`, `SharedLibrary`, `SharedLib`, `SharedHelpers`. + +Shared testing: `SharedTestingFramework`, `SharedTestingInfrastructure`, `SharedTestingInfra`, +`SharedTestingUtilities`, `SharedTestingUtils`, `SharedTestingLibrary`, `SharedTestingLib`, +`SharedTestingHelpers`. + +## Automatic project references to preserve + +Layout is what makes these work. Move a project off the convention and you own the reference. + +Test projects probe for their target, in order: + +- `../$(TargetProjectName)/$(TargetProjectName).csproj` +- `../../$(TargetProjectName)/$(TargetProjectName).csproj` +- `../src/$(TargetProjectName)/$(TargetProjectName).csproj` +- `../../src/$(TargetProjectName)/$(TargetProjectName).csproj` + +Non-test projects pick up sibling shared projects via `../Shared*/Shared*.csproj`; test projects +pick up `../SharedTesting*/SharedTesting*.csproj`. A Blazor WebAssembly client is the exception: it +does not pick up `Shared*`, because everything it references is downloaded to the browser. + +## Blazor Web App: host and client + +```text +src/src/ + Shop/Shop.csproj Microsoft.NET.Sdk.Web - the host + Shop.Client/Shop.Client.csproj Microsoft.NET.Sdk.BlazorWebAssembly - the client + Shop.ClientShared/Shop.ClientShared.csproj a PurviewBrowserSafe library both use + Shop.Bff/Shop.Bff.csproj a BFF, when tokens are kept server-side in a separate host +src/tests/ + Shop.Client.UnitTests/ TargetProjectName = Shop.Client +``` + +- A web host named `` beside a WebAssembly client named `.Client` is the pairing the SDK + recognises: the host gets `IsBlazorHostProject=true` and a reference to the client automatically + (skipped when declared; `PurviewAutoBlazorClientReference=false` opts out). +- Identities with `NamespacePrefix=Acme`: `Shop.Client` is `Acme.Shop.Client`; `Shop.Bff` is + `Acme.Shop.Bff`; `ClientShared` is a stripped segment, so `Shop.ClientShared` has namespace + `Acme.Shop` and assembly `Acme.Shop.ClientShared`. +- Every library the client references must set `true`, or the + build reports `PRBLZ0001`. Keep server-only code (file, process, database access) out of anything + the client references. +- Name a BFF host `*Bff` (case-sensitive) for `IsBffProject=true`. Being the client's host does not + make a project a BFF. + +## Creating a new project + +1. Match the SDK and style of sibling projects. +2. Use Central Package Management if the repository has it. +3. Add only the dependencies the project's scope needs — the SDK supplies the testing stack, + SourceLink and telemetry by default. +4. Add the project to the solution. +5. Keep target frameworks, nullability, analyzers and warning settings consistent with neighbours. + +## Moving or splitting + +1. Create the destination using the established layout. +2. Move the files. +3. Update namespaces, usings and references for the destination. +4. Verify the destination still produces the intended `TestingType`, `TargetProjectName` and + `RootNamespace` — ask MSBuild rather than assuming: + `dotnet msbuild -getProperty:RootNamespace -getProperty:TargetProjectName`. +5. Remove dependencies the source no longer needs. +6. Update solution membership and project references. +7. Build, then run the affected tests. + +## Guardrails + +- Prefer minimal, targeted diffs; avoid cross-cutting renames unrelated to the move. +- Keep test intent unchanged while relocating. +- When the structure is ambiguous, infer from the nearest sibling projects and say which assumption + you made. +- When in doubt, preserve compatibility with the SDK's automatic naming, namespace and + project-reference behaviour. diff --git a/.agents/skills/purview-buildsdk-repository-review/SKILL.md b/.agents/skills/purview-buildsdk-repository-review/SKILL.md new file mode 100644 index 0000000..ce1fd29 --- /dev/null +++ b/.agents/skills/purview-buildsdk-repository-review/SKILL.md @@ -0,0 +1,111 @@ +--- +name: purview-buildsdk-repository-review +description: Review a repository that uses Purview.BuildSdk for naming, placement and test-structure alignment, and report what the SDK can and cannot do automatically as a result. Use when asked to audit, rationalise or health-check a repo against the SDK's conventions, when onboarding an existing codebase onto Purview.BuildSdk, or when deciding whether a layout quirk is worth changing. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Reviewing a repository against the SDK's conventions + +This is an audit, not a refactor. The deliverable is a prioritised list of findings and the cost of +each, so somebody else can decide what to change. + +For the conventions themselves use `purview-buildsdk-project-layout`; for why one project was +classified as it was, `purview-buildsdk-project-detection`. + +## The governing judgement + +**Deviation is only a finding when it costs the repository something.** The SDK infers behaviour +from naming and placement, so a deviation is interesting precisely to the extent that it turns an +automatic behaviour off. A layout that differs from the canonical one but still gets every automatic +behaviour is not debt, and reporting it as such wastes the reader's attention. + +Sort every finding into one of three buckets, and say which: + +| Bucket | Meaning | +| -- | -- | +| **Acceptable variance** | Differs from the canonical layout, costs nothing. Preserve it. | +| **Structural debt** | Blocks an automatic behaviour the repository would benefit from. | +| **Incremental opportunity** | Low-risk tidying worth doing when the area is touched anyway. | + +## 1. Read before concluding + +Inspect, in this order, before forming any opinion: + +- `Directory.Build.props` and `Directory.Build.targets` — where the SDK is imported, and what is set + before the import +- the solution entry point, for what is actually in the build +- `Directory.Packages.props`, for Central Package Management +- the project layout on disk + +## 2. Check the structural signals + +- Source projects under `src/`, test projects under `tests/`. +- `.csproj` filename equals its containing directory name — unless + `DisableProjectFileNamingConventionCheck=true`, which is itself a finding worth surfacing. +- Short project names, with `NamespacePrefix` carrying the repository identity. +- Test project names match `^.*\.([\w|_]\w*)Tests?$`. **The dot is required**; a project named + plainly `UnitTests` is not detected at all and silently gets no runner, no coverage, no + `InternalsVisibleTo` and no category. This is the single highest-value thing to check. +- Test suffixes come from the SDK's shipped `TestType` group where `internal` access is wanted — a + suffix outside it still runs, it just cannot see `internal` members. +- Shared and shared-testing projects use the **exact** recognised names. A near miss gets none of + the behaviour. + +## 3. Verify with MSBuild, do not infer from folder names + +A review that reasons from directory names will be wrong about at least one project. Ask: + +```bash +dotnet msbuild -getProperty:RootNamespace -getProperty:AssemblyName \ + -getProperty:IsTestProject -getProperty:TestingType -getProperty:TargetProjectName +``` + +Quote the computed values in the findings. "`Api.Tests` resolves `TestingType=` empty, so it has no +runner" is actionable; "`Api.Tests` looks like it might not be detected" is not. + +## 4. Review test readability + +- Subject-based suites: `{SubjectName}Tests`, with + `{SubjectOrMemberUnderTest}_{Scenario}_{Expectation}` methods. +- Non-subject-based suites — build, generator, workflow, package, full-system — named clearly for + their broader role. A forced `{Subject}Tests` on one of these is worse, not better. +- TUnit categories and display names used where a suite is large enough to need navigating. + +See `purview-buildsdk-testing`. + +## 5. Check the test dependencies are earned + +The SDK already supplies `TUnit`, `TUnit.Mocks`, `Bogus` and the Microsoft.Testing.Platform wiring +to every detected test project. Anything beyond that was a deliberate choice, and deliberate choices +go stale: + +- `TUnit.Aspire` and `Purview.Containers` are **not** universal defaults. Confirm each reference is + genuinely needed by that project rather than copied from a sibling. +- Where a project references `Testcontainers` directly, check whether a `Purview.Containers` typed + module now covers it — the module set grows, and a direct reference that was once necessary often + is not any more. +- A package the SDK already adds, restated in a project file, is maintenance cost with no effect. + +## 6. Explain consequences, not just mismatches + +For each finding, name the specific automatic behaviour that is lost: `RootNamespace`, +`AssemblyName`, `PackageId`, `TargetProjectName`, the automatic target-project reference, the +sibling `Shared*` reference, `InternalsVisibleTo` generation, the test runner and coverage wiring. + +"Project is not under `src/`" is not a finding. "Project is not under `src/`, so the test project's +four probe paths all miss and the `ProjectReference` has to be maintained by hand" is. + +## Reporting the result + +1. One sentence on whether the repository broadly fits the SDK's conventions. +2. The findings, ordered by impact, each with its bucket and the behaviour it costs. +3. The low-risk rationalisation steps, as concrete renames or moves. +4. An explicit split of what is **already automatic** versus what this repository has configured by + hand — that list is usually the most useful part of the review, because hand-configuration that + merely restates a default is pure maintenance cost. + +Do not propose a cross-cutting rename as a first step. Name the one change with the best +cost-to-benefit ratio and say what it unlocks. diff --git a/.agents/skills/purview-buildsdk-skill-installation/SKILL.md b/.agents/skills/purview-buildsdk-skill-installation/SKILL.md new file mode 100644 index 0000000..66c20a3 --- /dev/null +++ b/.agents/skills/purview-buildsdk-skill-installation/SKILL.md @@ -0,0 +1,166 @@ +--- +name: purview-buildsdk-skill-installation +description: Install, list and diagnose the agent skills a Purview.BuildSdk package ships. Use for the InstallPurviewSkills, ListPurviewSkills and RecommendPurviewSkills MSBuild targets, for any PRSKL0001-PRSKL0014 diagnostic, for choosing a package runner (npx, pnpm dlx, bunx, yarn dlx, deno), for skills-lock.json, or when a package ships skills but no agent can see them. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Installing the skills a package ships + +For *authoring* skills, use `purview-buildsdk-skill-tests`. + +## The one thing to understand first + +**The SDK writes nothing into an agent directory.** A package *ships* `.agents/skills//`, but +as metadata — it is what tells the SDK which skills the package offers and at which commit. Nothing +copies it out. + +Skills are installed by the skills CLI and by nothing else. That is a deliberate single-writer rule. +Earlier versions of this SDK mirrored a package's whole `.agents/**` folder into the consuming +repository, and the CLI's own universal install location is `.agents/skills/` — the same paths. The +mirror would silently overwrite a pinned install on the next build, while `PRSKL0011` compared a +lockfile ref against content the mirror had already replaced. The mirror was removed outright rather +than fenced off, because the CLI covers the whole job. + +Installing is therefore a separate, deliberately **explicit** step. It is never a side effect of a +restore, a build, a design-time build or a test run. + +```bash +dotnet msbuild -t:ListPurviewSkills # diagnostics only, installs nothing +dotnet msbuild -t:InstallPurviewSkills # delegates to the skills CLI +``` + +Neither target has `BeforeTargets` or `AfterTargets`. If skills are appearing or changing on their +own, something has been wired up that should not be. + +## How a skill is resolved, and why the version is always right + +There is deliberately **no custom manifest**. A package that ships skills already carries both +halves of the mapping, and `ResolvePurviewSkillSources` reads them: + +- `.agents/skills//` in the restored package says *which* skills it ships. +- The nuspec's ``, written by SourceLink for every package this + SDK builds, says which git repository and which **exact commit** produced them. + +So the resolved ref is the commit the *restored package version* was built from. An older +`PackageReference` gets that version's skills, not whatever is on the default branch. Compatibility +is exact by construction rather than by convention. + +## Installing from an explicit source + +For a skill you want but have no `PackageReference` for: + +```bash +dotnet msbuild -t:InstallPurviewSkills -p:PurviewSkillsSource=purview-dev/build-sdk +``` + +The caller owns the ref here — append `#` to pin it. This path needs no restore, so +`PRSKL0001` does not apply. + +## Diagnostics + +| Code | Meaning | Fix | +| -- | -- | -- | +| `PRSKL0001` | `project.assets.json` is missing | Restore. Resolution reads the restore graph and deliberately does not trigger a restore of its own | +| `PRSKL0002` | The package's nuspec has no GitHub repository URL, or no commit | The package cannot be pinned. It is reported rather than installed from a guessed branch | +| `PRSKL0003` | No repository root resolved | Pass `-p:PurviewSkillsInstallRoot=` | +| `PRSKL0004` | No skills CLI and no package runner found | Install Node, or set `PurviewSkillsCommand` | +| `PRSKL0005` | An authored skill is malformed | See `purview-buildsdk-skill-tests` | +| `PRSKL0010` | A skill a referenced package ships is missing | Warning. `npx skills add ...` | +| `PRSKL0011` | An installed skill is pinned to a different ref than the referenced package | Warning. Install the package's own commit | +| `PRSKL0012` | A skill cannot be verified | Message | +| `PRSKL0013` | An orphaned lockfile entry | Message | +| `PRSKL0014` | Recorded in the lockfile but never restored | Message | + +## Recommendations run during an ordinary build + +This is the **one** skill feature that does. `RecommendPurviewSkillsOnBuild` runs after `Build` and +delegates to `RecommendPurviewSkills`, which can also be invoked by name: + +```bash +dotnet msbuild -t:RecommendPurviewSkills +``` + +`EvaluatePurviewSkillState` joins the skills each restored package ships against the CLI's own +`skills-lock.json`. Both inputs are local files, so it stays offline and spawns no process. + +Four things keep it quiet, and all of them are load-bearing: + +- Design-time builds, CI builds, non-default TFM passes and + `EnablePurviewSkillRecommendations=false` are skipped outright. + `PurviewSkillRecommendationsInCI=true` re-enables CI. +- A fingerprint stamp at `/.purview/skill-recommendations.cache` means a given state is + reported once per repository, not once per project and again on every rebuild. The explicit target + ignores the stamp. +- Missing and drifted are **warnings** (fixable in one command); unverifiable and orphaned are + **messages**. No amount of skill advice can break a build using `TreatWarningsAsErrors`. +- A correctly installed set produces no output at all. + +Orphan detection is **owner-scoped on purpose**: a lockfile cannot record who asked for an entry, so +only an entry under an owner the project references — but from a repository it no longer references +— is reported. A skill from any other owner is the developer's own, and is never mentioned. + +### Drift never recommends `npx skills update` + +`update` installs the *newest* published skill, which is only correct when the newest skill happens +to describe the referenced package version. `PRSKL0011` names the referenced package's own commit in +a pinned `npx skills add` instead. + +## Package runners + +The CLI is published to npm, and every runner spells "run this npm package" differently. +`PurviewSkillsRunner` selects one, defaulting to `npx` because npx ships with Node itself. + +| Runner | Invocation | +| -- | -- | +| `npx` | `npx -y skills@1` | +| `bunx` | `bunx skills@1` | +| `bun` | `bun x skills@1` | +| `pnpm` | `pnpm dlx skills@1` | +| `yarn` | `yarn dlx skills@1` — Yarn 2+ only | +| `deno` | `deno run -A npm:skills@1` | + +An unknown runner falls through to ` $(PurviewSkillsPackage)`, so it still works. + +`DetectPurviewSkillsCli` probes `PATH` for `skills` first, then `$(PurviewSkillsRunner)`, then +`$(PurviewSkillsRunnerCandidates)`, honouring `PATHEXT` on Windows where these are `.cmd` shims. +`PATH` is read directly rather than by spawning a version check: deterministic, no process, and it +cannot hang on a cold download. **The runner actually found wins over the configured preference.** +`PurviewSkillsCommand` overrides the launcher entirely and skips detection. + +`ListPurviewSkills` reports runner availability but **never** errors on its absence — resolving and +reporting needs no Node tooling at all. + +## Always the repository root + +`skills-lock.json` and the agent directories belong to the repository, not to a project, so every +skill target runs from one resolved root: + +1. `PurviewSkillsInstallRoot` — an explicit override +2. `PurviewSkillsRepoRoot` — only when the **user** set it +3. the `PurviewRepositoryRootMarkers` walk +4. `PurviewSkillsRepoRoot` / `RepoRoot` as the SDK guessed them +5. CI workspace variables +6. SourceLink's git `SourceRoot` + +There is deliberately **no project-directory fallback**. That would scatter a lockfile and an agent +directory through the tree, one per project. An unresolved root is `PRSKL0003` on install, and +silence for recommendations — a build never fails over advice. + +The marker walk is described in [references/repository-root.md](references/repository-root.md); its +two-pass behaviour and its home-directory bound are both load-bearing. + +## Why the CLI rather than a reimplementation + +The CLI owns the agent-specific destinations and their discovery conventions, which differ per agent +and change independently of this SDK. It also owns `skills-lock.json`, so a repository can restore +the same set later with `npx skills experimental_install`. + +`PurviewSkillsAgents` (default `*`) narrows the target agents. + +The lockfile is read **directly** rather than by parsing `npx skills list --json`, because that +listing omits the `ref` — and the ref is the whole compatibility question. Reading a file also needs +no Node tooling, which is what keeps recommendations working on a machine that can build .NET but +has no Node. There is deliberately no competing lockfile. diff --git a/.agents/skills/purview-buildsdk-skill-installation/references/repository-root.md b/.agents/skills/purview-buildsdk-skill-installation/references/repository-root.md new file mode 100644 index 0000000..e90b695 --- /dev/null +++ b/.agents/skills/purview-buildsdk-skill-installation/references/repository-root.md @@ -0,0 +1,55 @@ +# The repository-root marker walk + +`FindPurviewRepositoryRoot` is an inline MSBuild task driven by `PurviewRepositoryRootMarkers`: + +```text +.git/;.github/;.gitattributes;LICENSE.md;AGENTS.md;.agents/;CLAUDE.md;.claude/;package.json +``` + +A trailing slash marks a directory. A directory marker still matches a **file** of that name, +because `.git` is a directory in a clone but a file in a worktree or a submodule, and both identify +a repository root. + +## Three things that are load-bearing + +### It cannot be an MSBuild intrinsic + +`GetDirectoryNameOfFileAbove` matches **files only** and returns nothing for `.git/`, `.github/`, +`.agents/` or `.claude/` — four of the nine markers. Verified, not assumed. + +### Two passes + +- **Pass 1** — the first marker in the list is *definitive* and is searched all the way up. `.git/` + *is* the repository root, so a monorepo sub-package with its own `package.json` does not win. +- **Pass 2** — every other marker resolves nearest-ancestor-first, with list order breaking ties + inside a single directory. Searching them all to the top would let a stray `.claude/` in a + developer's home directory outrank the real root. + +### The home directory is never a candidate + +The home directory and everything above it are excluded. Without that bound, a project outside any +repository resolves to `~` and installs into it — and a `.claude/` or a `.github/` very commonly +sits there. A repository *below* the home directory is unaffected, because it is below it. + +## Why the marker walk outranks the evaluation-time guess + +`PurviewSkillsRepoRoot` is computed during evaluation from `RepoRoot` and an `AGENTS.md` walk-up. +An `AGENTS.md` can legitimately sit in a subdirectory, and resolving to it would put a lockfile and +an agent directory somewhere that is not the repository. + +`_PurviewSkillsRepoRootWasSpecified` is captured in `Sdk.props` *before* defaulting, so a value the +**user** set still outranks the walk while a value the SDK guessed does not. + +## A target's Condition is evaluated before its DependsOnTargets + +A property a dependency computes is still empty in the depending target's `Condition`. That is why +the unresolved-root guard sits on the `EvaluatePurviewSkillState` task rather than on +`RecommendPurviewSkills` — and equally why the build hook's CI and design-time gating works at all. + +## The managed port + +`Purview.Skills.Testing` ships `SkillRepositoryRoot`, a managed port of the same walk with the same +default markers, so a test that validates a repository's skills resolves the same root the build +installs them into. The two are kept in step by `SkillRepositoryRootTests`, including an assertion +on the default marker list itself — both are defaults nobody sets, so nothing else would notice them +drifting apart. diff --git a/.agents/skills/purview-buildsdk-skill-tests/SKILL.md b/.agents/skills/purview-buildsdk-skill-tests/SKILL.md new file mode 100644 index 0000000..4043ac4 --- /dev/null +++ b/.agents/skills/purview-buildsdk-skill-tests/SKILL.md @@ -0,0 +1,131 @@ +--- +name: purview-buildsdk-skill-tests +description: Author agent skills in a repository that uses Purview.BuildSdk, and validate them with Purview.Skills.Testing or the SDK's pack-time ValidatePurviewSkills target. Use when adding or renaming a skill, writing SKILL.md frontmatter, splitting content into references/ or scripts/, fixing a PRSKL0005 pack failure or a skill-name, skill-description or skill-bundled-resources test failure, or deciding where skills belong in a repository. +license: MIT +metadata: + package: Purview.Skills.Testing + repository: purview-dev/build-sdk +--- + +# Authoring and validating agent skills + +For installing skills a package ships, use `purview-buildsdk-skill-installation`. + +## Where skills live + +**At the repository root, in `skills//SKILL.md`.** Always. + +The skills CLI only discovers skills at a root `skills/` folder, and it reaches a repository by +The skills CLI only discovers skills at a root `skills/` folder, and it reaches a repository by +cloning it, so nothing else is visible to it. Nothing is packed into a NuGet package — a package +carries .NET artifacts, and skills are delivered by the CLI from git. + +```text +skills/ + my-skill/ + SKILL.md required + references/ loaded on demand + scripts/ executable, run on demand + assets/ templates and static resources +``` + +Link to a skill by its **authored** path, `./skills//SKILL.md`. A link into `.agents/skills/` +points at a gitignored file that does not exist in a fresh clone. + +## `SKILL.md` format + +```markdown +--- +name: my-skill +description: What it does, and when to use it. Include the words a task would contain. +license: MIT +metadata: + package: My.Package +--- + +# My skill + +Instructions. +``` + +| Field | Required | Constraint | +| -- | -- | -- | +| `name` | yes | 1–64 chars, lowercase `a-z0-9` and single hyphens, no leading, trailing or consecutive hyphen, **must equal the folder name** | +| `description` | yes | 1–1024 chars, non-empty | +| `license` | no | A licence name, or the name of a bundled licence file | +| `compatibility` | no | ≤500 chars, only when the skill has real environment requirements | +| `metadata` | no | A map of string to string. Anything not defined above belongs here | +| `allowed-tools` | no | Space-separated pre-approved tools. Experimental | + +**There are no other top-level fields.** `category`, `roles` and `tags` are common inventions and a +strict client rejects them; put them under `metadata`. + +The full specification is at . +[references/authoring.md](references/authoring.md) covers description writing, naming for a shared +namespace, and progressive disclosure. + +## Progressive disclosure + +An agent loads a skill in three stages: the `name` and `description` at startup, the whole +`SKILL.md` body once the skill activates, and `references/`, `scripts/` or `assets/` only when the +task reaches them. + +So: keep `SKILL.md` **under 500 lines**, and move detail into `references/`. The body is a cost paid +on every activation; a reference file is paid only when it is needed. + +Keep file references one level deep from `SKILL.md`, and relative to the skill root. A path that +climbs out of the skill folder is broken everywhere except the authoring repository, because only +the skill's own folder is installed. + +## Validating + +### In tests — `Purview.Skills.Testing` + +```csharp +[InheritsTests] +public sealed class SkillTests : PurviewSkillTests; +``` + +**`[InheritsTests]` is load-bearing.** TUnit does not walk base types during discovery, so without +it the class compiles, the run is green and nothing is validated. TUnit reports `TUnit0030`; treat +it as an error. + +| Rule id | What it checks | +| -- | -- | +| `skills-discovery` | The folder, when present, holds at least one skill | +| `skill-structure` | Every folder has a `SKILL.md` whose frontmatter parses | +| `skill-name` | Spec-legal, unique, and equal to the folder name | +| `skill-description` | Present, non-empty, within 1024 characters | +| `skill-optional-fields` | Only spec-defined fields, each within its limit | +| `skill-bundled-resources` | Every linked file exists, inside the skill folder | +| `skill-body-size` | `SKILL.md` stays under the recommended 500 lines | + +A repository with **no** `skills/` folder passes every rule — authoring skills is optional. An +**empty** folder fails `skills-discovery`, because every other rule is vacuous against an empty +catalogue. + +Overrides: `RepositoryRoot` (default: the SDK's repository-root marker walk), +`SkillsFolderName` (default `skills`), and `SkipRule(string)`. + +The evaluator takes a `SkillCatalogue`, so a rule can be exercised against a fixture folder rather +than a real repository — the same three-layer shape this repository uses throughout: a reader, an +evaluator over an in-memory model, and a thin test base class. + +### At pack time — `ValidatePurviewSkills` + +It runs `BeforeTargets="Pack"`, so a malformed skill fails in the repository that owns it rather +than reaching the CLI, which would silently skip it. + +```bash +dotnet msbuild -t:ValidatePurviewSkills +``` + +Failures are reported as `PRSKL0005`, one per problem. Every authored skill is checked. + +The two checks overlap deliberately: the pack-time one runs without a test host, the test one +covers more of the specification. + +## What is deliberately not checked + +Prose quality, and whether an agent actually obeys the skill. A build can only check that a skill is +well formed. Treat a green run as "installable", not "good". diff --git a/.agents/skills/purview-buildsdk-skill-tests/references/authoring.md b/.agents/skills/purview-buildsdk-skill-tests/references/authoring.md new file mode 100644 index 0000000..69d4ee4 --- /dev/null +++ b/.agents/skills/purview-buildsdk-skill-tests/references/authoring.md @@ -0,0 +1,79 @@ +# Authoring a skill that fires, and keeps firing + +## The description is the whole activation decision + +`name` and `description` are the only things loaded at startup. Everything else — the body, the +references, the scripts — is invisible until an agent has already decided to activate the skill on +the strength of those two fields. + +So write the description for a *matcher*, not for a reader: + +- Say **what it does** and **when to use it**, in that order. +- Include the words a real task would contain: the tool names, the file names, the error codes, the + property names, the phrases a person would type. +- Name the thing the skill is *not*, when a neighbouring skill could be confused with it. + +| | | +| -- | -- | +| Poor | `Helps with the build SDK.` | +| Better | `Configure Purview.BuildSdk from Directory.Build.props or a .csproj. Use when setting NamespacePrefix, choosing a testing framework, or working out whether a property must be set before the SDK import.` | + +The limit is 1024 characters. Most of that budget is better spent on trigger words than on prose. + +## Naming for a shared namespace + +Skills install into one flat directory per agent, shared with every other skill a developer has +installed from anywhere. A generic name is a collision waiting to happen, and the loser is whichever +one installed second. + +- Prefix with the vendor and the product: `purview-buildsdk-configuration`, not `sdk-configuration`. +- `sdk-*`, `project-*`, `test-*`, `build-*` are all too common to use unprefixed. +- The name must be 1–64 characters, lowercase `a-z0-9` with single hyphens, no leading, trailing or + consecutive hyphen — and it must equal the folder name, because the CLI installs by the + frontmatter name while a human reads the folder. + +Renaming a skill means renaming the folder and the frontmatter together, and re-running the install. + +## Progressive disclosure in practice + +| Stage | Loaded | Budget | +| -- | -- | -- | +| Metadata | `name`, `description` | ~100 tokens, always | +| Instructions | the whole `SKILL.md` body | under 5000 tokens recommended; keep the file under 500 lines | +| Resources | `references/`, `scripts/`, `assets/` | only when the task reaches them | + +What belongs where: + +- **`SKILL.md`** — the decisions, the rules, the things that are wrong by default, and a pointer to + everything else. If a reader needs it to make the *first* correct move, it goes here. +- **`references/`** — long tables, full option lists, grammars, enumerations. Keep each file + focused; an agent loads the whole file. +- **`scripts/`** — executable code. Self-contained, or documenting its dependencies clearly, with + helpful error messages. +- **`assets/`** — templates, schemas, images, lookup data. + +Keep references one level deep. Avoid a chain where one reference sends an agent to another. + +## Links + +Use relative paths from the skill root: + +```markdown +See [the reference](references/REFERENCE.md) for detail. +Run `scripts/extract.py`. +``` + +Only the skill's own folder is installed, so a link that climbs out of it (`../other-skill/...`) is +broken on every machine except the one that authored it. To point at another skill, name it in prose +and let the agent find it by name. + +## Keeping a skill true + +A skill that describes a product goes stale the moment the product changes, and nothing about a +stale skill looks broken — it just gives confidently wrong advice. + +- Change the skill in the **same commit** as the behaviour it documents. +- Prefer stating the property name, the diagnostic code or the target name over paraphrasing the + behaviour; those are greppable when the code changes. +- Where a number or a list is authoritative somewhere else — a default, an enumeration — say where + it comes from, so a reader can check it. diff --git a/.agents/skills/purview-buildsdk-testing/SKILL.md b/.agents/skills/purview-buildsdk-testing/SKILL.md new file mode 100644 index 0000000..5a5a7d5 --- /dev/null +++ b/.agents/skills/purview-buildsdk-testing/SKILL.md @@ -0,0 +1,121 @@ +--- +name: purview-buildsdk-testing +description: Write and run tests in a repository that uses Purview.BuildSdk. Use when authoring or refactoring TUnit tests (AAA structure, Subject_Scenario_Expectation naming, awaited assertions, CancellationToken), when choosing a test project's framework packages, and when running or filtering tests - the SDK wires Microsoft.Testing.Platform, so `dotnet test --filter` reports "Zero tests ran" and `--treenode-filter` is the one that works. +license: MIT +metadata: + package: Purview.BuildSdk + repository: purview-dev/build-sdk +--- + +# Testing in a Purview.BuildSdk repository + +Two things to know before anything else: + +1. A test project is detected by **name**, and detection is what wires the runner. See + `purview-buildsdk-project-layout` if the project is new or being moved. +2. The runner is **Microsoft.Testing.Platform (MTP)**, not VSTest. `--filter` does nothing useful. + +## What a test project gets automatically + +With the defaults (`TestingFramework=TUnit`, `SubstituteFramework=TUnitMocks`, +`TestDataFramework=Bogus`), a detected test project receives: + +- `TUnit`, `TUnit.Mocks`, `Bogus` +- Microsoft.Testing.Platform integration +- `CollectCoverage=true` +- test-friendly `NoWarn` defaults (`PurviewTestContextNoWarn`), while keeping the strict style + contract — `IDE0040`, field naming, formatting, `IDE1006` +- `IsPackable=false`, `IsPublishable=false` + +Add nothing else unless the project's purpose demands it. Two that stay explicit: + +- `TUnit.Aspire` — Aspire lifecycle and AppHost-backed integration tests +- `Purview.Containers` — container-backed integration tests. One API over Microsoft WSL Containers + (no Docker install needed on Windows) or Docker, with typed modules for SQL Server, PostgreSQL, + Redis, MySQL, RabbitMQ, Azurite and NATS. Experimental and prerelease, so pin the version. + Reference `Testcontainers` directly only where no typed module exists yet. + +## Authoring rules + +- Use TUnit (`TUnit.Core`, `TUnit.Assertions`). Do not introduce xUnit, NUnit, MSTest or + FluentAssertions unless explicitly asked. +- Every test method carries `[Test]`. +- **Every assertion is awaited**: `await Assert.That(actual).IsEqualTo(expected);`. An un-awaited + assertion silently passes. +- Arrange / Act / Assert, with the `// Arrange`, `// Act`, `// Assert` comments present. +- Test classes are `{SubjectName}Tests`, in the **same namespace** as the subject. +- Test methods are `{SubjectOrMemberUnderTest}_{Scenario}_{Expectation}`. Use `ctor` as the subject + for constructor tests. Methods, constructors, properties, operators, conversions and validation + hooks are all valid subjects. +- When any method called in the test body accepts a `CancellationToken`, take + `CancellationToken cancellationToken` as the **last parameter** and pass it through. + +```csharp +[Test] +public async Task ProcessAsync_GivenAnEmptyBatch_ReturnsZero(CancellationToken cancellationToken) +{ + // Arrange + var sut = new BatchProcessor(); + + // Act + var result = await sut.ProcessAsync([], cancellationToken); + + // Assert + await Assert.That(result).IsEqualTo(0); +} +``` + +For a suite that is not subject-based — a build, generator, workflow, package or full-system suite — +a broader, more truthful class name is better than a forced `{Subject}Tests`. Use TUnit display +names and categories to keep it navigable. + +When refactoring existing tests, bring them into compliance with all of the above while preserving +test intent. + +## Categories + +The detected test type becomes the baseline category automatically — `Api.IntegrationTests` yields +`Category=Integration`. Add further categories when they make a large suite easier to navigate; +they are what `--treenode-filter` property predicates select on. + +## Running tests + +```bash +dotnet test # the whole suite +dotnet test --treenode-filter "/*/*/LoginTests/*" # one class +dotnet test --treenode-filter "/*/*/*/*[Category=Unit]" +``` + +Prefer the repository's own task (`just test`, or equivalent) for a full run. + +The filter path is `////`, with `*` wildcards, `=`/`!=` property +predicates, `&`/`|` inside a single bracket group, and `**` only at the end. The full grammar, the +`--` separator rules across SDK versions, and the zero-tests checklist are in +[references/treenode-filter.md](references/treenode-filter.md). + +### "Zero tests ran" + +In order: + +1. Are you using `--treenode-filter` and not `--filter`? This is the cause most of the time. +2. Try `dotnet test --treenode-filter "..."`; on an older SDK use `dotnet test -- --treenode-filter "..."`. +3. Broaden to `/*/*/*/*`, then narrow one segment at a time. +4. Does the project actually contain `[Test]` methods? +5. Is the project **detected** as a test project? A name without the dot — `UnitTests` rather than + `Api.UnitTests` — gets no runner at all. Check with + `dotnet msbuild -getProperty:IsTestProject`. +6. Are all property predicates inside one bracket group, `[(A=1)&(B=2)]`? + +## Cross-platform requirement + +CI runs the full suite on Linux, so a test that only passes on Windows is a failing change. + +- Never hardcode a platform-specific path — no `C:\...`, no backslash-only separators, no + case-insensitive path assumptions. +- Build paths with `Path.Combine`, `Path.GetTempPath()`, `Path.DirectorySeparatorChar`, + `Path.GetFullPath()`. +- Feed production path-math code only native-format paths. Normalise a fixed Windows-style literal + to the current platform before passing it in. +- Do not rely on a case-insensitive filesystem, a `C:` drive, or trailing-separator behaviour. +- Keep `SyntaxTree` file paths and `build_property.*` values consistent and native when writing + analyzer tests. diff --git a/.agents/skills/purview-buildsdk-testing/references/treenode-filter.md b/.agents/skills/purview-buildsdk-testing/references/treenode-filter.md new file mode 100644 index 0000000..35d032b --- /dev/null +++ b/.agents/skills/purview-buildsdk-testing/references/treenode-filter.md @@ -0,0 +1,107 @@ +# `--treenode-filter` reference + +TUnit runs on Microsoft.Testing.Platform, not VSTest, so the VSTest `--filter` syntax does not +apply. Narrowing is done with `--treenode-filter`. + +## The path + +```text +//// +``` + +Each segment maps to one level of the tree: + +| Position | Matches | +| -- | -- | +| 1 | Assembly | +| 2 | Namespace | +| 3 | Class | +| 4 | Test name | + +```text +/*/*/LoginTests/* every test in LoginTests +/*/*/*/AcceptCookiesTest one test by name +/*/MyProject.Tests.Integration/*/* every test in a namespace +/*/*/*/* everything +``` + +## Operators + +### `*` — wildcard + +Matches a whole segment or part of one. + +```text +/*/*/LoginTests*/* +/*/*/MyProject.Tests.Api*/* +``` + +### `=` and `!=` — property predicates + +In the last segment, inside `[...]`. + +```text +/*/*/*/*[Category=Unit] +/*/*/*/*[Category!=Slow] +/*/*/*/*[Owner=*Team-Backend*] +``` + +### `&` and `|` — combining + +Inside a **single** bracket group, or a single parenthesised segment. + +```text +/*/*/*/*[(Category=Unit)&(Priority=High)] +/**[(Category=Unit)|(Priority=High)] +/*/*/(LoginTests)|(SignupTests)/* +``` + +### `**` — match all + +Any depth, but **only at the end** of the path. + +```text +/** +/MyAssembly/** +``` + +`/**/Path` is not valid. + +## Rules that catch people out + +- Only **one** property group `[...]` per path segment. `[Category=Smoke]|[Priority=High]` is not + valid; write `[(Category=Smoke)|(Priority=High)]`. +- `**` must be last. +- A segment left empty is not a wildcard — write `*`. + +## The `--` separator + +```bash +dotnet test --treenode-filter "/*/*/*/*[Category=Unit]" # preferred +dotnet test -- --treenode-filter "/*/*/*/*[Category=Unit]" # older SDKs +``` + +Newer .NET SDKs forward unrecognised arguments to the MTP runner directly. On an older SDK the `--` +separator is required, and omitting it produces an argument error rather than a filter. + +## Common selections + +| Intent | Filter | +| -- | -- | +| Everything | `/*/*/*/*` | +| All unit tests | `/*/*/*/*[Category=Unit]` | +| High-priority unit tests | `/*/*/*/*[(Category=Unit)&(Priority=High)]` | +| One class | `/*/*/MyFeatureTests/*` | +| A name pattern | `/*/*/*/MyScenario*` | +| Critical integration tests | `/*/MyProject.Tests.Integration/*/*[Priority=Critical]` | + +## Zero tests ran + +1. `--treenode-filter`, not `--filter`. +2. Try without the `--` separator first, then with it. +3. Broaden to `/*/*/*/*`, then narrow one segment at a time. +4. Confirm the project has `[Test]` methods. +5. Confirm the project is **detected** as a test project — + `dotnet msbuild -getProperty:IsTestProject`. A name without the dot, such as + `UnitTests` rather than `Api.UnitTests`, is not detected and gets no runner. +6. Confirm every property predicate sits in one bracket group. diff --git a/.agents/skills/purview-engineering-principles/SKILL.md b/.agents/skills/purview-engineering-principles/SKILL.md new file mode 100644 index 0000000..ba4dd9b --- /dev/null +++ b/.agents/skills/purview-engineering-principles/SKILL.md @@ -0,0 +1,171 @@ +--- +name: purview-engineering-principles +description: >- + Use when setting up, reviewing or rationalising a .NET repository against the Purview engineering + conventions - convention over configuration, derived identity, architecture as tests, warnings as + errors, proving consumer-facing claims, documentation ownership, test data builders, agent + legibility, and tiering verification by risk. Each rule names the failure it prevents and whether a + build or a reviewer enforces it. +metadata: + product: Purview engineering playbook + repository: purview-dev/engineering-playbook +--- + +# Purview engineering principles + +Ten rules, each with the failure it prevents and the mechanism that makes it true. The distinction +that matters throughout: **a convention a build can check is checked by the build; a convention only +a reviewer can check is labelled a review obligation and never described as enforced.** A rule that +claims enforcement it does not have is worse than no rule, because people stop looking. + +## 1. Convention over configuration + +A repository declares as little as possible. One MSBuild SDK supplies the defaults, and a project +file is short because the SDK already knows what a project of that shape needs. + +Configuration copied between repositories diverges. When the rule lives in one package, fixing it +once fixes it everywhere; when it lives in eleven project files, it is fixed eleven times — or, more +often, once. + +*Enforced by:* `` in a single +`Directory.Build.props`. The SDK detects project type from the filename and the `Sdk` attribute. + +*At your scale:* a single-project repository still benefits. The import is two lines and removes the +target framework, language version, nullable context, analyzer set and package metadata. + +## 2. Identity is derived, never repeated + +`RootNamespace` comes from `$(NamespacePrefix).$(ProjectName)`, and `AssemblyName` follows the +resolved `RootNamespace`. A name appears once — in the folder and file name — and everything else is +computed. + +A hand-typed namespace drifts from the assembly it lives in, and the drift is invisible until +something reflects over types: an architecture test, a serializer, a DI scan. + +*Enforced by:* the SDK during `.props` evaluation, plus a build-time check +(`PurviewProjectFileNameMismatch`) when a `.csproj` filename does not match its parent directory. + +*At your scale:* always. It costs nothing and removes a class of bug. + +## 3. Architecture is a test, not a diagram + +The dependencies a layer may have are asserted by tests that read the compiled assemblies. A +`ProjectReference` pointing the wrong way fails the build. + +A diagram is a claim about the past. Dependency direction erodes one convenient reference at a time, +and every such reference looks reasonable in isolation. + +*Enforced by:* a test project that references every layer and inspects their metadata references. + +*At your scale:* small repositories can skip it. Adopt it the moment a **second layer** exists — +that is when the wrong direction first becomes possible. + +## 4. The build is the only authority + +Style is enforced by analyzers and `EnforceCodeStyleInBuild`, and the SDK's own style policy is +re-validated on every compile, so a repository cannot quietly downgrade a rule. + +*At your scale:* always. If a rule matters and cannot be checked, say so plainly. + +## 5. Warnings are not acceptable output + +A clean build has no warnings. One that will never be fixed is either fixed, suppressed *where it +occurs* with a stated justification, or turned off deliberately. + +Warnings are a budget. A build that emits twenty is a build where the twenty-first is invisible — and +the twenty-first is the one that mattered. + +*Enforced by:* severity, partly. The SDK's shipped `.editorconfig` sets many rules to `error`. The +rest is review, backed by the pipeline running `dotnet build` with the repository's configuration. + +*At your scale:* a small repository can tolerate `warning`; a large one cannot. Decide deliberately +rather than by accumulation. + +## 6. Every claim about a consumer is proved by a consumer + +When a package documents how it must be used — a target framework, an asset, a guard, a workaround — +something in the repository builds that documented consumer and asserts the documented outcome. + +Consumer-facing documentation is the part of a library least likely to be exercised by its own tests, +because **the library's own build is not a consumer**. It drifts silently and fails in somebody +else's repository. + +*Enforced by:* a verification script that packs, then builds throwaway consumer projects against the +produced packages, asserting the happy path *and* the documented guard errors. `purview-dev/containers` +is the worked example (`just verify-consumers`), deliberately excluded from the default unit filter +because it restores from nuget.org. + +*At your scale:* only once a package is consumed outside its own repository. Before that, the +repository's own tests already are the consumer. + +## 7. Documentation is a build artefact of the repository + +The wiki lives in the repository it describes, in `docs/wiki`, and the repository owns its +`mkdocs.yml`, its navigation and its `edit_uri`. + +Documentation that lives elsewhere is updated elsewhere. A page with no owner and no review path is +wrong within one release. + +*Enforced by:* review, plus the navigation file — a page absent from `_Sidebar.md` is not published. + +*At your scale:* always. Even a repository with one page should own that page. + +## 8. Everything a test needs is built by one builder + +Domain content in a test comes from `TestBuilder`. A test never constructs a domain object inline, +and it overrides **only** the value it is proving. + +Inline construction encodes a type's shape into every test that touches it, so one constructor change +becomes a repository-wide edit. It also hides which values the test actually depends on, because +every value looks deliberate. + +*Enforced by:* review. + +*At your scale:* adopt from the first test. Retrofitting is a mechanical edit across every test file, +and exactly the kind of edit that is postponed forever. + +## 9. An agent is a reader with no memory + +A repository states its purpose, its layout and its conventions in `AGENTS.md`. **Nothing important +is discoverable only by reading the code.** + +A coding agent has the repository and nothing else — no colleague to ask, no institutional memory, no +intuition about which of two plausible patterns is intended. It will confidently generalise from +whatever it finds first. That is also every new-joiner failure mode; the agent just makes the cost +visible sooner, because an agent does not ask. + +*Enforced by:* review, plus publishing skills so agent-facing guidance tracks the library version +rather than a repository's memory of it. + +*At your scale:* always. `AGENTS.md` is cheaper than the questions it prevents. + +## 10. A change is verified in proportion to its risk + +Checks are tiered, and a repository declares which tiers its pipeline runs. A pull request runs the +fast tier; the slow tier runs deliberately, locally or on a dedicated runner. + +A pipeline that runs everything on every pull request gets made optional. One that runs too little +gets ignored. Tiering is what keeps the gate credible. + +*Enforced by:* the pipeline's `TestFilter` and the test project's own **name** — the SDK categorises +a `*UnitTests` project as `Unit`, so the tier is a property of the project rather than of a +hand-written attribute. + +*At your scale:* one tier is fine until the slow tests exist. Then split by project name, not by +attribute. + +## Applying these in review + +Three questions catch most of it: + +1. **Is this configuration, or is it a default the SDK should own?** If two repositories would copy + it, it belongs in the SDK. +2. **Is this rule enforced, or merely stated?** If stated, say so where it is stated. +3. **Could an agent — or a new joiner — find this without being told?** If not, it goes in + `AGENTS.md` or the wiki. + +## Related skills + +`purview-architecture` for project-shape decisions, `purview-architecture-testing` for turning a +boundary into a failing build, `purview-telemetry` and `purview-persistence` for the per-concern +guidance. diff --git a/.agents/skills/purview-persistence/SKILL.md b/.agents/skills/purview-persistence/SKILL.md new file mode 100644 index 0000000..4667fa2 --- /dev/null +++ b/.agents/skills/purview-persistence/SKILL.md @@ -0,0 +1,106 @@ +--- +name: purview-persistence +description: >- + Use when a .NET project actually persists data and you are choosing a storage approach, deciding + whether to abstract EF Core, or selecting a GUID primary-key strategy. Gives provider-specific + guidance — SQL Server and PostgreSQL differ and must not be treated alike — and stays silent for + projects that do not persist anything. +license: MIT +metadata: + author: Purview-Dev + version: "1.0" +--- + +# Purview persistence + +**Activation condition:** this skill applies only when persistence is real. If the project is a +stateless CLI, a pure library, or a service that calls an API and stores nothing, stop here — none of +this is relevant, and raising a database GUID strategy with that project is noise. + +Confirm persistence from the repository before proceeding (see `purview-architecture`): a data-access +`PackageReference`, a `DbContext`, a connection string, or an explicit requirement to store state. + +## 1. Shape the data access proportionally + +| Scale | Shape | +| -- | -- | +| Small | `DbContext` used directly from the code that needs it. No repository, no unit-of-work wrapper — `DbContext` already is both. | +| Medium | A boundary when domain rules must not know about EF Core, or when a query is complex enough to deserve a name. Often that is a few focused query types, not a generic `IRepository`. | +| Large | Independent infrastructure implementations where there genuinely are several, with the dependency direction enforced by a test. | + +**Do not wrap EF Core in a repository for testability.** `Purview.Containers` gives you a real +database in tests, which is both more faithful and less code than an abstraction built to be mocked. +A repository earns its place when you must swap the store or when `DbContext` is genuinely leaking +into domain decisions — not before. + +## 2. Local dependencies + +`Purview.Containers` provides Aspire resource integrations for the dependencies you need in +development and integration tests — including `MsSql`, `PostgreSql`, `MySql`, `Redis`, `Nats`, +`RabbitMq`, `Azurite` and an Azure Key Vault emulator. Prefer a real container over an in-memory +provider: the in-memory provider does not enforce the constraints that break you in production. + +## 3. GUID keys — only if GUIDs are keys + +Resolve these before recommending anything; most are answered by the repository: + +1. Are GUIDs primary keys, or merely external identifiers over an integer key? +2. Is the value generated by the application or by the database? +3. Which provider, and which version? +4. Does index locality matter at the expected write volume? +5. Must the identifier exist before the row is persisted? + +Question 5 usually decides it. If the application needs the identifier before `SaveChanges` — for an +outbox row, a correlation value, or to build an object graph client-side — the database cannot be the +generator, and the remaining questions are about *which* client-side strategy. + +### The rule that matters most + +**SQL Server and PostgreSQL do not benefit from the same sequential GUID implementation.** Their +index structures and their GUID comparison semantics differ, and a "sequential GUID" that helps one +can be no better than random on the other. Never carry a strategy from one to the other without +re-deriving it. + +- SQL Server specifics: `references/sqlserver.md` +- PostgreSQL specifics: `references/postgresql.md` +- Choosing between them, and when a custom generator is justified: `references/guid-generation.md` + +### Prefer built-in capability + +Use the framework or database capability when it is sufficient — `Guid.CreateVersion7()` on .NET 9+, +`NEWSEQUENTIALID()`, EF Core's own value generators, a provider's native UUIDv7 function. A custom +generator is a maintenance obligation and a correctness risk; introduce one only against a measured +requirement. + +**There is no Purview GUID generation package.** Do not reference one, and do not create one without +first establishing a concrete, measured need that the built-in options cannot meet. If such a +capability is added later, this skill is where the guidance for recommending it belongs. + +## 4. Value objects as keys + +`Purview.ValueObjects` gives strongly-typed identifiers that stop an `OrderId` being passed where a +`CustomerId` belongs. They need an EF Core value converter; configure it once in the model rather +than per property. + +This is about type safety at the boundary, not about the generation strategy — the two decisions are +independent. + +## 5. Migrations and schema + +- Migrations are code: reviewed, tested against a real container, and applied by a deliberate step — + not by the application at startup in production. +- Expect the schema to be ahead of or behind the code during a deployment; design changes to be + compatible in both directions. +- Keep the connection string out of source. Keep it out of telemetry too. + +## Checks before you answer + +- Have you confirmed persistence is actually required? +- If you are recommending a GUID strategy, do you know the provider *and* its version? +- Are you proposing an abstraction you can justify by something other than testability? +- Have you preferred a built-in capability over a custom generator? + +## See also + +- `purview-architecture` — context discovery and proportional structure +- `purview-telemetry` — instrument the data access too; it is where the latency is diff --git a/.agents/skills/purview-persistence/references/guid-generation.md b/.agents/skills/purview-persistence/references/guid-generation.md new file mode 100644 index 0000000..432053a --- /dev/null +++ b/.agents/skills/purview-persistence/references/guid-generation.md @@ -0,0 +1,93 @@ +# Choosing a GUID generation strategy + +The cross-provider decision procedure. Provider detail lives in `sqlserver.md` and `postgresql.md`. + +## Step 0 — is this question in scope? + +Ask nothing and recommend nothing unless **both** are true: + +- The project persists data, and +- GUIDs are (or are about to be) primary keys. + +A stateless CLI, a pure library, a service with an integer key, or a project using GUIDs only as +external correlation identifiers does not need this decision. Raising it anyway is the failure this +page is written to prevent. + +## Step 1 — must the identifier exist before persistence? + +This question decides more than any other. + +**Yes** — the application needs the value before `SaveChanges` for an outbox row, a correlation +identifier, an object graph built client-side, or an identifier returned before commit. The database +cannot be the generator. Go to step 2 with client-side options only. + +**No** — the database may generate it, and database generation is available to you. + +## Step 2 — which provider, and which version? + +Do not skip the version. It changes the available options materially: + +| Provider | Version matters because | +| -- | -- | +| SQL Server | `NEWSEQUENTIALID()` is long-standing; the comparison semantics that make byte-ordered UUIDs useless have never changed | +| PostgreSQL | Native `uuidv7()` exists only from 18; `gen_random_uuid()` is built in from 13 | +| .NET | `Guid.CreateVersion7()` exists from .NET 9 | + +**The two databases 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 that is +optimal on one is unremarkable or wrong on the other. + +## Step 3 — does locality actually matter? + +Sequential keys exist to reduce index fragmentation and page splits under write pressure. If the +table takes tens of inserts a minute, this is not your problem and a random GUID is the simpler, +more private choice. + +Reach for a sequential strategy when you can point at write volume, measured fragmentation, page +splits, or buffer-pool pressure. "It's best practice" is not a measurement. + +Note also that on SQL Server the problem is often better solved by making the clustered index +something other than the GUID key — see `sqlserver.md`. + +## Step 4 — is unpredictability required? + +Sequential and time-ordered identifiers are guessable, and UUIDv7 discloses creation time to the +millisecond. Where an identifier appears in a URL, is handed to a third party, or acts as a +capability, use a random UUID — or keep the ordered value internal and expose a random surrogate. + +Never use `NEWSEQUENTIALID()` output as a secret. + +## Decision summary + +| Provider | Needs value before insert | Locality matters | Recommendation | +| -- | -- | -- | -- | +| SQL Server | No | Yes | `NEWSEQUENTIALID()` (EF Core's default for `Guid` keys) | +| SQL Server | Yes | Yes | EF Core `SequentialGuidValueGenerator` | +| SQL Server | Either | No | `Guid.NewGuid()` | +| PostgreSQL | Yes | Yes | `Guid.CreateVersion7()` with `ValueGeneratedNever()` | +| PostgreSQL | No | Yes | `uuidv7()` on 18+, otherwise generate v7 in the application | +| PostgreSQL | Either | No | `Guid.NewGuid()` or `gen_random_uuid()` | +| Any | Either | Either, but must be unpredictable | Random v4 | + +## Prefer built-in capability + +In order of preference: + +1. A framework capability — `Guid.CreateVersion7()`, EF Core's value generators. +2. A database capability — `NEWSEQUENTIALID()`, `uuidv7()`, `gen_random_uuid()`. +3. A reviewed, tested implementation of a published standard, when neither of the above is available + on your target versions. +4. A bespoke generator — only against a measured requirement the first three cannot meet. + +A hand-written sequential GUID generator is easy to get subtly wrong — endianness, clock regression, +counter rollover within a millisecond, and thread safety are all live hazards, and the failure mode +is duplicate or badly-ordered keys found in production. + +## There is no Purview GUID package + +No Purview library provides GUID generation today. Do not reference one and do not invent one. + +If a concrete need emerges that the built-in options cannot meet — established by measurement, not by +anticipation — that is the point at which creating one is worth discussing. Until then this guidance +is the deliverable, and this page is where a recommendation for such a package would be documented. diff --git a/.agents/skills/purview-persistence/references/postgresql.md b/.agents/skills/purview-persistence/references/postgresql.md new file mode 100644 index 0000000..cd557ec --- /dev/null +++ b/.agents/skills/purview-persistence/references/postgresql.md @@ -0,0 +1,98 @@ +# PostgreSQL UUID keys + +Read this only when the provider is PostgreSQL and UUIDs are primary keys. The SQL Server answer is +different — see `sqlserver.md`. + +## Why PostgreSQL differs from SQL Server + +Two differences drive everything on this page: + +1. **PostgreSQL has no clustered index.** Tables are heaps; the primary key is a separate B-tree. A + random UUID therefore does not cause row-level page splits the way it does on a clustered SQL + Server table. The cost is in the index: random inserts touch scattered index pages, which hurts + cache locality and WAL volume, but it is a smaller and differently-shaped problem. +2. **`uuid` compares in byte order.** Unlike SQL Server, PostgreSQL orders UUIDs by their bytes. A + UUID that is sequential in byte layout *is* sequential to the index. + +The consequence: **UUIDv7 works here.** It is the natural answer on PostgreSQL and the wrong answer +on SQL Server. Never carry a sequential-GUID choice across the two. + +## The options + +### `Guid.CreateVersion7()` — .NET 9+, application-generated + +The default recommendation for new PostgreSQL schemas with UUID keys. + +```csharp +public sealed class Order +{ + public Guid Id { get; init; } = Guid.CreateVersion7(); +} +``` + +- Time-ordered in byte layout, so index locality is good. +- The value exists before persistence, so outbox rows, correlation and client-side graphs all work. +- Standard (RFC 9562), so it is portable and readable by other systems. +- Embeds a millisecond timestamp — which is also its disclosure caveat. + +Tell EF Core the application supplies it: + +```csharp +builder.Property(o => o.Id).ValueGeneratedNever(); +``` + +Without that, EF Core may treat the key as store-generated and skip your value. + +### Database-generated UUIDv7 — PostgreSQL 18+ + +PostgreSQL 18 added a native `uuidv7()` function. Earlier versions do not have it; on 13–17 you have +`gen_random_uuid()` (v4, from the built-in pgcrypto-derived function) and nothing native for v7. + +**Check the server version before recommending `uuidv7()`.** It is the single most common way this +guidance goes wrong — the function simply does not exist on the version most deployments are running. + +```sql +-- PostgreSQL 18 and later only +ALTER TABLE orders ALTER COLUMN id SET DEFAULT uuidv7(); +``` + +Prefer application-side generation anyway unless rows are inserted by paths that bypass the +application, because the value-before-persistence property is worth more than the marginal +difference. + +### `gen_random_uuid()` — database-generated v4 + +Available without an extension from PostgreSQL 13. Random, so index locality is poor, but +unpredictable and simple. + +### `Guid.NewGuid()` — application-generated v4 + +Random and unpredictable. Fine at low to moderate write volume, which covers most applications. + +## Npgsql and EF Core notes + +- `Guid` maps to `uuid` natively. Do not store UUIDs as `text` or `varchar` — it costs space, loses + index efficiency and defeats comparison. +- Npgsql's `uuid` mapping is byte-order faithful, so a v7 value sorts in the index as you expect. +- If the application generates the key, set `ValueGeneratedNever()` so EF Core does not fetch a + store-generated value that does not exist. +- A legacy database may have UUIDs in `text` columns. Migrating to `uuid` is usually worth it, but it + is a data migration, not a model change. + +## Choosing + +| Situation | Use | +| -- | -- | +| New schema, .NET 9+, UUID keys | `Guid.CreateVersion7()`, `ValueGeneratedNever()` | +| .NET 8 or earlier, high write volume | A reviewed UUIDv7 implementation, or accept v4 after measuring | +| Rows inserted outside the application, PostgreSQL 18+ | `uuidv7()` as a column default | +| Rows inserted outside the application, PostgreSQL 13–17 | `gen_random_uuid()` | +| Key must be unpredictable | `Guid.NewGuid()` or `gen_random_uuid()` — v7 leaks creation time | +| Low or moderate volume | v4 — the locality difference will not be your bottleneck | + +## The timestamp caveat + +A UUIDv7 exposes when the row was created, to the millisecond. Where identifiers are public, that can +leak signup times, order rates or activity patterns — and it makes identifiers partially guessable by +time window. Where that matters, use a random UUID for the public identifier and keep the v7 value +internal, or use a surrogate. diff --git a/.agents/skills/purview-persistence/references/sqlserver.md b/.agents/skills/purview-persistence/references/sqlserver.md new file mode 100644 index 0000000..c728295 --- /dev/null +++ b/.agents/skills/purview-persistence/references/sqlserver.md @@ -0,0 +1,101 @@ +# SQL Server GUID keys + +Read this only when the provider is SQL Server and GUIDs are primary keys. The PostgreSQL answer is +different — see `postgresql.md`. + +## Why SQL Server is a special case + +A clustered index determines physical row order. A `uniqueidentifier` primary key is clustered by +default, so random GUIDs insert into the middle of the index, causing page splits, fragmentation and +a larger buffer-pool working set. On a write-heavy table this is measurable. + +The complication is that **SQL Server does not compare `uniqueidentifier` values the way the bytes +are laid out.** Its ordering weights the last six bytes (node field) most significantly, then earlier +groups. So a GUID that is sequential in big-endian byte order — including a standard UUIDv7 — is +**not** sequential under SQL Server's comparison. This is precisely why the SQL Server and PostgreSQL +answers diverge. + +## The options + +### `NEWSEQUENTIALID()` — database-generated + +A column default that produces values ascending in SQL Server's own ordering. Best index locality +available. + +```sql +ALTER TABLE [Orders] ADD CONSTRAINT [DF_Orders_Id] + DEFAULT NEWSEQUENTIALID() FOR [Id]; +``` + +EF Core, for a `Guid` key on SQL Server, configures this by default — `ValueGeneratedOnAdd` maps to +`NEWSEQUENTIALID()` through the default value-generation strategy. + +Trade-offs: + +- The value does not exist until the row is inserted. EF Core retrieves it, but you cannot build an + object graph around the key beforehand. +- Values are **not** unpredictable: the sequence restarts on service restart but is otherwise + guessable. Never use one as a security token or capability URL. +- It reveals rough insert ordering, which can be an information-disclosure concern on a public + surface. + +### EF Core's `SequentialGuidValueGenerator` — client-generated + +EF Core ships a value generator that produces GUIDs ordered for SQL Server's comparison semantics, +generated in the application. + +```csharp +builder.Property(o => o.Id) + .HasValueGenerator() + .ValueGeneratedOnAdd(); +``` + +Use this when the identifier must exist before `SaveChanges`. It keeps most of the index locality +while letting the application own the value. + +### `Guid.NewGuid()` — random + +Correct, portable and unpredictable; poor index locality. + +Appropriate when write volume is low (the fragmentation never matters), when the key is +non-clustered, or when unpredictability is a requirement. Do not dismiss it by default — for most +applications it is fine, and "most applications" includes more than people expect. + +### `Guid.CreateVersion7()` — .NET 9+ + +Ordered in byte layout, but **not** ordered under SQL Server's comparison. On SQL Server it behaves +roughly like a random GUID for clustered-index purposes. It is the right answer on PostgreSQL and the +wrong answer here, which is the trap this page exists to prevent. + +Use it on SQL Server only when you want the embedded timestamp for application reasons and accept +random-like insert locality. + +## Decoupling the key from the index + +Often the best answer is not a different GUID but a different index. Keep the GUID as the primary key +and make the clustered index something naturally ascending: + +```csharp +builder.HasKey(o => o.Id).IsClustered(false); +builder.HasIndex(o => o.CreatedAt).IsClustered(); +``` + +This removes the page-split problem entirely and leaves the key unpredictable. Consider it before +reaching for a sequential generator. + +## Choosing + +| Situation | Use | +| -- | -- | +| High insert volume, key may be database-generated | `NEWSEQUENTIALID()` | +| High insert volume, identifier needed before persisting | EF Core `SequentialGuidValueGenerator` | +| Key must be unpredictable (public surface, security boundary) | `Guid.NewGuid()`, with a non-clustered PK | +| Low or moderate volume | `Guid.NewGuid()` — do not optimise what does not hurt | +| You want a sortable timestamp in the key and accept the locality cost | `Guid.CreateVersion7()` | + +## Measure before optimising + +Index fragmentation from random GUIDs is real but frequently irrelevant. Before changing a key +strategy, confirm it matters: check page splits, fragmentation and buffer-pool pressure on the actual +table at the actual volume. Changing a primary key strategy on a live table is expensive; do it +against evidence. diff --git a/.agents/skills/purview-telemetry/SKILL.md b/.agents/skills/purview-telemetry/SKILL.md new file mode 100644 index 0000000..6d83173 --- /dev/null +++ b/.agents/skills/purview-telemetry/SKILL.md @@ -0,0 +1,123 @@ +--- +name: purview-telemetry +description: >- + Use when adding, reviewing or scaling observability in a .NET project — logging, tracing, metrics, + OpenTelemetry wiring — or when deciding whether a project is "too small for telemetry". Recommends + Purview.Telemetry.SourceGenerator at every project size, keeps libraries free of global provider + configuration, and scales the export infrastructure rather than the instrumentation. +license: MIT +metadata: + author: Purview-Dev + version: "1.0" +--- + +# Purview telemetry + +**Telemetry is a baseline, not an advanced topic.** Instrument from the first commit at every project +size. What changes with scale is the *export and operations* story — collectors, dashboards, sampling +— not whether the code emits anything. + +A project that defers telemetry until it becomes distributed acquires its observability at exactly +the moment it can least afford the refactor. + +## Instrumentation versus export + +Keep these separate; conflating them is what produces "we're too small for telemetry". + +| | What it is | Who owns it | +| -- | -- | -- | +| **Instrumentation** | The log, span and measurement the code emits | Every project, every size, from day one | +| **Export** | Where signals go: console, OTLP, a backend, a dashboard | The application host, sized to the operation | + +A small CLI is fully instrumented and exports to the console. That is not a reduced form of +telemetry; it is the correctly sized export for a CLI. + +## Use the generator + +`Purview.Telemetry.SourceGenerator` turns an attributed interface into logging, tracing and metrics +implementations plus a DI registration extension. One declaration, three signals, no hand-written +`ILogger` message templates or `ActivitySource` plumbing. + +Do **not** reinvent its API here. The three shipped skills are authoritative and travel with the +package: + +- `telemetry-sourcegenerator-quickstart` — adding the package, the first interface, DI registration, + and publishing meter/activity-source names to the host via `TelemetryNames` +- `telemetry-sourcegenerator-design` — designing telemetry interfaces well +- `telemetry-sourcegenerator-migration` — moving existing `ILogger`/`ActivitySource`/metrics code onto + the generator + +Recommend the generator whenever the project targets a compatible TFM and the team is willing to take +an analyzer-only package reference. Where it is not compatible, fall back to the standard .NET +abstractions below — never to a bespoke logging wrapper. + +## Libraries versus hosts + +This is the rule most often broken, and it is not negotiable. + +**A reusable library:** + +- Emits through `ILogger`, `ActivitySource` and `Meter` — or through generated telemetry + interfaces, which produce exactly those. +- Publishes its activity-source and meter names so a host can subscribe. The generator does this for + you through assembly metadata. +- **Never** calls `AddOpenTelemetry`, configures an exporter, sets a sampler, or requires a specific + backend. +- Never reads an OTLP endpoint from configuration itself. + +**The application host:** + +- Owns provider configuration, exporters, sampling and resource attributes. +- Subscribes to the sources its libraries publish — with the generator, by opting into the + `TelemetryNames` class and passing its arrays to the OpenTelemetry builder. + +A library that configures a global provider breaks every consumer that already configured one, and +silently wins or loses depending on initialisation order. + +## Sizing the export + +| Scale | Export and operations | +| -- | -- | +| **Small** | Console or a single OTLP endpoint. Structured logs, exceptions recorded on the span. No collector, no dashboards, no custom exporter, no distributed tracing infrastructure. | +| **Medium** | OTLP export, dependency instrumentation (HTTP, EF Core), trace propagation across the integrations, application metrics, health checks, a sampling decision made deliberately. | +| **Large** | End-to-end distributed tracing, standardised resource attributes, operational metrics, dashboards, alerting, SLIs, documented retention and sampling policy. | + +Full detail per level: `references/observability-levels.md`. + +Instrumentation is identical across all three rows. Only the right-hand column grows. + +## Baseline for any application + +- Structured logging — parameters as fields, never interpolated into the message. +- Errors and exceptions recorded as telemetry, with the exception attached to the span. +- Operation correlation, so one unit of work is one trace. +- OpenTelemetry-compatible instrumentation, so the backend stays a choice. +- Log level configurable at runtime. +- Export behaviour configurable per environment. +- Sensitive values — credentials, tokens, connection strings, personal data — never logged or placed + on span attributes or baggage. + +## Before you change anything + +1. **Inspect what exists.** Find current `ILogger` use, `ActivitySource`/`Meter` instances, any + `AddOpenTelemetry` call, a `ServiceDefaults` project. +2. **Preserve it.** Extend existing instrumentation; do not replace working telemetry to adopt a + library. +3. **Do not duplicate registrations.** One registration per source; adding a second is a silent + double-count on every metric. +4. **Size the configuration to the project**, using the table above. +5. **Check for leaking secrets** in anything you add. +6. **Keep instrumentation and export distinct** in what you recommend. + +## Do not + +- Omit telemetry because the application is small. +- Invent Purview telemetry APIs — read the three shipped skills instead. +- Configure a global provider from a library. +- Recommend a collector, dashboard stack or custom exporter for a small project. +- Put a user identifier, token or connection string into a log field, span attribute or baggage item. + +## See also + +- `purview-architecture` — establishing project context and scale before deciding +- [Purview.Telemetry.SourceGenerator](https://github.com/purview-dev/purview-telemetry-sourcegenerator) diff --git a/.agents/skills/purview-telemetry/references/observability-levels.md b/.agents/skills/purview-telemetry/references/observability-levels.md new file mode 100644 index 0000000..df97d78 --- /dev/null +++ b/.agents/skills/purview-telemetry/references/observability-levels.md @@ -0,0 +1,93 @@ +# Observability levels + +Instrumentation is constant across these levels. Export, infrastructure and operational practice are +what scale. Read this after establishing project complexity — see `purview-architecture`. + +## Small + +*A utility, CLI, prototype, simple API or limited-scope application.* + +**Do:** + +- Structured logging with parameters as fields. +- Exceptions recorded with enough context to reproduce. +- An `ActivitySource` around the unit of work, so a failure has a shape and a duration. +- Console export, or a single OTLP endpoint when one already exists. +- Log level from configuration, so a user can raise it without a rebuild. + +**Do not:** + +- Stand up a collector. +- Adopt a centralised monitoring platform. +- Build distributed tracing infrastructure for a single process. +- Create dashboards nobody reads. +- Write a custom exporter. + +The test: a user reports a failure and you can ask for one log output that tells you what happened. + +## Medium + +*Several functional areas, integrations, moderate domain complexity.* + +Everything above, plus: + +- **Dependency instrumentation** — HTTP clients, EF Core, messaging. These are where latency and + failure actually live, and the instrumentation is a package reference, not code. +- **Trace propagation** across every integration boundary, so a trace survives the process hop. W3C + `traceparent` by default; do not invent a correlation header. +- **Application metrics** for the operations someone would notice: request rate, error rate, duration, + queue depth. Prefer histograms over averages. +- **OTLP export** to a real backend. +- **Health checks**, distinguishing liveness from readiness. +- **A deliberate sampling decision.** Head sampling at a fixed ratio is fine; the point is that it was + chosen rather than defaulted into. + +Common mistake at this level: adding metrics for everything measurable instead of the few that drive +a decision. A metric nobody alerts or decides on is cost without signal. + +## Large + +*Complex domain, multiple services or teams, substantial operational requirements.* + +Everything above, plus: + +- **End-to-end distributed tracing**, including asynchronous hops — message producers and consumers + linked, not orphaned. +- **Standardised resource attributes** — `service.name`, `service.version`, `deployment.environment` + — applied uniformly so cross-service queries work at all. Follow OpenTelemetry semantic conventions + rather than inventing names. +- **Operational metrics** beyond application ones: saturation, dependency health, cost drivers. +- **Dashboards** owned by the team that gets paged. +- **Alerting** on symptoms users feel, not on causes. Alert on error rate and latency; investigate + with CPU and queue depth. +- **Service-level indicators** derived from the telemetry, with objectives agreed with whoever cares. +- **Retention and sampling policy**, documented and costed. Tail sampling if error traces must always + survive. + +## Signal selection + +| Signal | Add when | Warning | +| -- | -- | -- | +| Logs | Always | Unstructured logs are not telemetry; they are strings | +| Traces | Work crosses a process, a dependency or an async boundary | A span per method produces noise, not insight | +| Metrics | Someone would alert on, or decide from, a rate or duration | High-cardinality attributes (user id, request id) are a cost incident | + +Put high-cardinality context on spans and logs, never on metric dimensions. + +## Sensitive information + +At every level, and without exception: + +- No credentials, tokens, keys or connection strings in messages, attributes or baggage. +- No personal data beyond what the system is entitled to process — and where an identifier is + necessary, prefer a pseudonymous one. +- Remember that **baggage propagates across service boundaries**; treat it as the most public place + in your telemetry. +- Redact at the point of emission. A redaction step in the pipeline is a backstop, not a control. + +## Export configuration + +The host owns this. Keep the endpoint, protocol, sampling ratio and resource attributes in +configuration, not in code, so the same build runs in every environment. + +Libraries contribute names and signals; they never contribute configuration. diff --git a/.agents/skills/purview-testing-conventions/SKILL.md b/.agents/skills/purview-testing-conventions/SKILL.md new file mode 100644 index 0000000..1fe3232 --- /dev/null +++ b/.agents/skills/purview-testing-conventions/SKILL.md @@ -0,0 +1,357 @@ +--- +name: purview-testing-conventions +description: >- + Use when writing or reviewing tests in a .NET repository following the Purview conventions - TUnit + and TUnit.Mocks, the AAA structure and naming, placing a test in the namespace of the class under + test, CreateXXX construction helpers, building domain content through TestBuilder with Bogus, the + assertion surface, parameterised tests, which tier a test project belongs to, BDTest for + acceptance tests, and Purview.Containers for a suite that needs a real database or queue. +metadata: + product: Purview engineering playbook + repository: purview-dev/engineering-playbook +--- + +# Testing conventions + +| Concern | The rule | +| --- | --- | +| Framework | **TUnit**. Not xUnit, NUnit or MSTest. | +| Substitutes | **TUnit.Mocks**. Not NSubstitute or Moq. | +| Test data | **Bogus**, reached through `TestBuilder`. | +| Acceptance tests | **BDTest** over TUnit, via `BDTest.TUnit`. Not Gherkin, SpecFlow or Reqnroll. | +| A real dependency | **`Purview.Containers`**. Not Testcontainers directly, never a shared instance. | +| Structure | Explicit `// Arrange`, `// Act`, `// Assert` — Given/When/Then in an acceptance test. | +| Naming | `{SubjectUnderTest}_{Scenario}_{Expectation}`, and `async Task`. | +| Placement | One `{Class}Tests` class per class under test, **in the namespace of the class under test**. | +| Construction | Dependencies and the system under test come from `CreateXXX` helpers with optional parameters. | +| Async | The async overload wherever the API offers one — in the test **and in every helper it calls**. Convert a sync helper, or state why it cannot be. | +| Cancellation | If any method the test calls accepts a `CancellationToken`, the test's **last parameter** is one, forwarded to every call that accepts one. | +| Domain content | Built through `TestBuilder`. Never inline. | + +## Why these, specifically + +**One framework.** The SDK wires TUnit into any project whose name ends in a test suffix — that +wiring is why a test project file is empty. A second framework means every project file grows a +conditional and the pipeline grows a second discovery path. That is a rule about the **runner**, not +the style of a test — `BDTest.TUnit` is an adapter *over* TUnit, so an acceptance suite does not break +it. + +**Same namespace as the subject.** The test reads as part of the thing it describes, needs no `using` +to reach the type, and **cannot silently test a different type with the same name**. + +**`CreateXXX` helpers.** A test body that constructs its own dependencies buries the assertion under +setup. A helper with optional parameters says what the test changed and defaults everything else: + +```csharp +static ConventionCatalogue CreateCatalogue(IConventionRepository? repository = null) => + new(repository ?? CreateRepository().Object); +``` + +**`TestBuilder`.** Inline construction encodes a domain type's shape into every test that touches it, +so one constructor change becomes a repository-wide edit. It also destroys the reader's signal about +which values the test depends on, because every literal looks deliberate. + +## A test + +```csharp +using Acme.Builders; + +namespace Acme.Domain; + +public sealed class CustomerIdTests +{ + [Test] + public async Task Create_GivenLowercaseSlug_ReturnsTheValue() + { + // Arrange + var id = TestBuilder.CustomerId("build-enforced"); + + // Act + var value = id.Value; + + // Assert + await Assert.That(value).IsEqualTo("build-enforced"); + } +} +``` + +Two deliberate details: + +- **The empty `// Arrange` block stays.** The comments are what makes a three-line test readable next + to a thirty-line one, and a convention applied "when needed" is applied inconsistently. +- **Assertions are awaited.** `Assert.That(...)` is terminal and returns a value, so + `await Assert.That(result).IsEqualTo(expected)` is the form. **An un-awaited assertion is a test + that cannot fail.** + +## Assertions + +| Form | Use | +| --- | --- | +| `await Assert.That(x).IsEqualTo(y)` / `.IsNotEqualTo(y)` | value equality | +| `await Assert.That(x).IsTrue()` / `.IsFalse()` | booleans | +| `await Assert.That(x).IsEmpty()` | collections — the workhorse of architecture rules | +| `await Assert.That(x).Count().IsEqualTo(2)` | collection size | +| `await Assert.That(x).Contains(y)` / `.DoesNotContain(y)` | membership | +| `await Assert.That(act).Throws()` | a synchronous delegate that should throw | +| `await Assert.That(async () => await f()).Throws()` | an async one | +| `await Assert.That(x).IsTypeOf()` | exact runtime type | +| `await Assert.That(x).IsEquivalentTo(sequence)` | order-independent sequence equality | + +For an exception test the two shapes are `Action` and `Func`. The compiler will not infer +either, so the explicit type is required and is **not** a style violation: + +```csharp +Action act = () => CustomerId.Create(""); + +await Assert.That(act).Throws(); +``` + +## Parameterised tests + +`[Arguments]` puts the interesting values in the attribute rather than a loop, so a failure names the +case: + +```csharp +[Test] +[Arguments("")] +[Arguments(" ")] +[Arguments("Has Spaces")] +[Arguments("has_underscore")] +public async Task Create_GivenSlugThatIsNotALowercaseSlug_Throws(string value) +{ + // Arrange + // Act + Action act = () => CustomerId.Create(value); + + // Assert + await Assert.That(act).Throws(); +} +``` + +## Async all the way down + +**Where an API offers an async overload, use it** — `File.ReadAllTextAsync`, not `File.ReadAllText`. +**And the rule reaches through test helpers:** a helper calling such an API is itself converted. Being +a helper is not an exemption. + +The build enforces the first half and cannot enforce the second, which is why the second needs saying: + +- `VSTHRD103` is an **`error`** and flags a sync call with an `Async`-suffixed equivalent **when the + call sits inside a `Task`-returning method**. In an `async Task` test, `File.ReadAllText` fails the + build. +- A **sync helper is not `Task`-returning**, so the identical call inside it is invisible to the + analyzer. Pushing the call one level down turns a build error into nothing. + +It also compounds with the cancellation rule, which keys off whether anything in the chain accepts a +`CancellationToken`: a sync helper accepts none, so it **silently removes the trigger** and the test is +uncancellable while looking compliant. (TUnit also runs tests in parallel, so a blocking read holds a +thread-pool thread an async read would release.) + +Converting — three things change together, and the first two are not optional: + +```csharp +// Before — invisible to VSTHRD103, and the chain accepts no token +static string Documentation => File.ReadAllText(DocumentationPath); + +// After +static async Task ReadDocumentationAsync(CancellationToken cancellationToken) => + await File.ReadAllTextAsync(DocumentationPath, cancellationToken); +``` + +- It returns a `Task`, bringing every call inside it back under `VSTHRD103`. +- It gains the **`Async` suffix** — `VSTHRD200` is an `error`, so the rename is enforced. +- It takes a trailing `CancellationToken`, because the chain now accepts one; its callers declare and + forward one. + +**A property cannot be `async`, so it becomes a method** — that is the conversion, not an exemption. +A read-only property hiding IO is the most common form of this violation. + +It genuinely cannot be converted when **no async overload exists** (`File.Exists`, `File.Delete`, +`Directory.EnumerateFiles`; likewise `ArchitecturePolicy.Load()` and `PolicySchemaValidator.Validate()`, +which `Purview.Architecture.Testing` ships synchronous-only) or when **a base type or interface fixes +the signature**. State the reason at the call. Never fake it: `Task.Run` around a blocking call, +`Task.FromResult` around a sync result, or `.Result` / `.GetAwaiter().GetResult()` to flatten a task — +the last two are `VSTHRD002`, an `error`. + +## Cancellation + +Three parts, triggered by the **call chain** rather than by the test: + +1. **If any method the test calls accepts a `CancellationToken`, the test declares one.** Not only the + method under test — anything in the chain, including a `File.ReadAllTextAsync` or the delegate handed + to `Assert.That`. +2. **It is the last parameter**, and TUnit supplies the value. With `[Arguments]`, the data binds to the + leading parameters and the token is supplied on top, so the attribute's positional order still matches + the signature: + + ```csharp + [Test] + [Arguments("build-enforced", true)] + public async Task ListEnforcedByBuildAsync_GivenAConvention_FiltersIt( + string id, + bool enforcedByBuild, + CancellationToken cancellationToken + ) + ``` + +3. **It is forwarded to every call that accepts one.** Never `CancellationToken.None`, never `default`, + never a token the test creates. TUnit's token is the *test's* token — cancelled on timeout or on an + aborted run — so a call given `None` cannot be stopped, and **one hop that drops it makes the whole + chain uncancellable**, which is why partial forwarding is a failure and not a lesser version of the + rule. It is also the only place that proves the production code threads the token through at all. + +A test whose chain accepts no token takes no parameter; an unused token is noise, not compliance. + +## Substitutes + +TUnit.Mocks is globally imported by the SDK, so no `using` is needed: + +```csharp +[Test] +public async Task RegisterAsync_GivenAnAlreadyRegisteredConvention_DoesNotStoreIt(CancellationToken cancellationToken) +{ + // Arrange + var repository = CreateRepository(); + repository.ExistsAsync(Any(), Any()).Returns(true); + var catalogue = CreateCatalogue(repository.Object); + + // Act + await Assert + .That(async () => await catalogue.RegisterAsync(TestBuilder.EnforcedConvention(), cancellationToken)) + .Throws(); + + // Assert + repository.AddAsync(Any(), Any()).WasNeverCalled(); +} +``` + +- `Mock.Of()` produces a `Mock`; `.Object` is the instance you pass in. +- `Any()` matches any argument, `Is(value)` a specific one, `AnyArgs()` the whole call. +- `.Returns(value)` sets the result. For a `Task`-returning method, return the `T` — the task is + unwrapped for you. +- `.WasCalled(Times.Once)` and `.WasNeverCalled()` are the assertions. + +The `// Assert` after a `Throws` is not a mistake: that test's point is that the exception was thrown +**and** that nothing was written, and both are assertions about the same act. + +## `TestBuilder` + +It lives in the shared testing project and is the single means by which a test creates domain +content: + +```csharp +public static class TestBuilder +{ + public const int DefaultCount = 3; + + internal static Faker Faker { get; } = new(); + + public static Domain.ConventionId ConventionId(string? value = null) => + Domain.ConventionId.Create(value ?? $"{Slug()}-{Faker.Random.String2(6, LowercaseAlphabet)}"); + + public static Domain.Convention Convention( + Domain.ConventionId? id = null, + string? title = null, + VerificationTier? tier = null, + bool? enforcedByBuild = null) => + Domain.Convention.Create( + id ?? ConventionId(), + title ?? Faker.Lorem.Sentence(4), + tier ?? Faker.Random.Enum(), + enforcedByBuild ?? Faker.Random.Bool()); +} +``` + +Four rules make it work: + +- **Omitted arguments are generated; supplied arguments are used exactly.** A test overrides only the + value it is proving, and everything else is realistic without the test saying so. +- **A collection factory comes with a companion.** `Conventions(int count = DefaultCount)` generates a + distinct set, so a test needing "some" gets "some" without its own loop. +- **Bogus is a global `using`.** `Faker` is available with no import — which is also why an explicit + `using Bogus;` in a shared testing project is reported as unnecessary. +- **A value-object test is the one exception.** A test proving what `Create` and `Hydrate` accept and + reject has to name the rejected input itself, so it constructs the invalid value directly. + Everything valid still goes through the builder. + +## Two constraints that shaped the builder + +Both are consequences of SDK rules, not preferences, and both were found by building it. + +**The factories are flat, not grouped.** The natural shape is a nested class per feature area +(`TestBuilder.Catalogue.Convention(...)`), and it cannot compile: the SDK ships +`dotnet_diagnostic.CA1034.severity = error`, so a publicly visible nested type fails the build, and +`PRSGD0008` refuses to let a repository add `CA1034` to `NoWarn`. Making the group `internal` does +not help either — the SDK's `InternalsVisibleTo` target runs on non-test projects and grants access +*to* test and shared-testing projects rather than *from* them, so a shared testing project's +internals are not visible to the tests beside it. An area that outgrows a flat builder gets its own +top-level builder class. + +**`var` is the house style, with one carve-out.** `csharp_style_var_elsewhere = true:warning` makes +an explicit type on a declaration initialised from a method call a warning (`IDE0007`). The carve-out +is object creation: the `PDS0003` analyzer prefers an explicit type with target-typed `new()`: + +```csharp +var id = TestBuilder.ConventionId("build-enforced"); // method call -> var +string title = new('a', Convention.MaxTitleLength); // new() -> explicit type +``` + +## What is enforced, and what is review + +| Rule | Enforced by | +| --- | --- | +| TUnit is the only framework | the build — the SDK adds it; a second has to be added by hand | +| Test projects are discovered and categorised | the build (name-suffix detection) | +| A shared testing project is referenced, not executed | the build (`IsSharedTestingProject`) | +| Style, including the `var` carve-out | the build (analyzers, `warning` or above) | +| An acceptance suite is categorised `Acceptance` and kept out of the fast tier | the build (name-suffix detection) and the pipeline's `TestFilter` | +| A container-backed suite fails rather than passes when its backend is missing in CI | the pipeline, when the backend is pinned with `PURVIEW_CONTAINERS_BACKEND` | +| AAA structure, naming, `CreateXXX` helpers, `TestBuilder` usage | **review** | +| Given/When/Then chains end in `.BDTestAsync()` | **review** | +| A real dependency comes from `Purview.Containers`, not a shared instance | **review** | +| An async overload is used inside an `async Task` test | the build — `VSTHRD103` is `error` | +| A converted helper carries the `Async` suffix | the build — `VSTHRD200` is `error` | +| A **sync helper** calling an API with an async overload is converted | **review** — a non-`Task`-returning method is outside `VSTHRD103`'s scope | +| A declared `CancellationToken` is the last parameter | the build — `CA1068` is `warning`, and a clean build has no warnings | +| A test whose chain accepts a token declares one and forwards it everywhere | **review** | + +## Acceptance tests and container-backed tests + +An acceptance test — a behaviour somebody outside the team asked for, in their words — is written with +**BDTest over TUnit** (`BDTest.TUnit`) in a `.AcceptanceTests` project, where Given/When/Then +replaces the AAA comments. A suite that needs a real database, queue or emulator gets a throwaway +container from **`Purview.Containers`** in its own `.IntegrationTests` project, skipping +locally when no backend is usable and failing in CI. + +Read `references/acceptance-and-container-tests.md` before writing either: it carries the scenario and +container shapes, and the two traps — a chain that never calls `.BDTestAsync()` runs no steps and still +passes, and a suite that skips in CI turns a missing runtime into a green job. + +## Tiers + +A test project's **name** carries its tier — the SDK categorises `*UnitTests` as `Unit`, and takes the +word before `Tests` as a pattern, so `*AcceptanceTests` gives `Acceptance` with nothing added to the +pipeline. The `TestFilter` then selects a tier without a hand-written attribute. Split a slow suite +into its own project rather than tagging individual tests. + +Filtering uses TUnit's `--treenode-filter`, not `--filter`: these repositories run on +Microsoft.Testing.Platform, where `dotnet test --filter` reports "Zero tests ran". + +## At your scale + +- **One test project.** TUnit, AAA, the naming convention, `TestBuilder`. That is the whole of it. +- **A shared fixture.** Move it into `SharedTestingInfra` and make the class public — a shared + testing project is a library, so its fixtures are legitimately public API and `CA1515` does not + apply. +- **A database or a container.** One `.IntegrationTests` project and one `Purview.Containers` + module. Do not build a fixture framework around it until a second suite needs the same container. +- **A behaviour somebody outside the team asked for.** That, and not before, is when BDTest earns its + place: one `.AcceptanceTests` project with one story. Reach for the HTML report only once + somebody will read it. +- **A package with a documented contract.** Add a consumer-verification script; see + `purview-engineering-principles`. + +## Related skills + +`purview-engineering-principles` for the rules this implements, and `purview-architecture-testing` +for turning an architectural boundary into a failing build. diff --git a/.agents/skills/purview-testing-conventions/references/acceptance-and-container-tests.md b/.agents/skills/purview-testing-conventions/references/acceptance-and-container-tests.md new file mode 100644 index 0000000..e8b9110 --- /dev/null +++ b/.agents/skills/purview-testing-conventions/references/acceptance-and-container-tests.md @@ -0,0 +1,117 @@ +# Acceptance tests and container-backed tests + +Two shapes that sit outside the unit tier. Both keep TUnit as the runner, and both live in their own +test project so the tier comes from the project name rather than an attribute. + +## Acceptance tests: BDTest + +An acceptance test states a behaviour somebody outside the team asked for, in their words. Use +**BDTest over TUnit through the `BDTest.TUnit` adapter**, in a `.AcceptanceTests` project +(the SDK emits `Category=Acceptance` for it). + +Not Gherkin, SpecFlow or Reqnroll, for two reasons: + +- **BDTest is code-only.** No `.feature` file and no generated test body, so there is no second + artefact that can drift from the step implementing it. That drift is the failure that makes a BDD + suite stop being trusted. +- **One runner stays one runner.** `BDTest.TUnit` is an adapter over TUnit, so the SDK's wiring, the + emitted category and `--treenode-filter` are all unchanged. A BDD framework with its own runner is + exactly what the one-framework rule refuses. + +```csharp +[Story(AsA = "account holder", IWant = "to withdraw money", SoThat = "I can use my savings")] +public class WithdrawalTests : TUnitBDTestBase +{ + AccountSteps Account => new(Context); + + [Test] + [ScenarioText("Withdrawing money reduces the balance")] + public async Task WithdrawMoney() => + await Given(() => Account.CreateAnAccount(100m)) + .When(() => Account.Withdraw(25m)) + .Then(() => Account.TheBalanceIs(75m)) + .BDTestAsync(); +} + +public class AccountSteps(AccountContext context) +{ + [StepText("I have an account with a balance of {0}")] + public void CreateAnAccount(decimal balance) => context.Balance = balance; + + [StepText("I withdraw {0}")] + public void Withdraw(decimal amount) => context.Balance -= amount; + + [StepText("the account balance is {0}")] + public async Task TheBalanceIs(decimal expected) => await Assert.That(context.Balance).IsEqualTo(expected); +} +``` + +Five rules, and the first is the one that bites: + +- **`.BDTestAsync()` runs the scenario.** A chain stopping at `.Then(...)` compiles, passes, and + executes none of its steps — the same failure mode as an un-awaited `Assert.That`. +- **Given/When/Then replaces Arrange/Act/Assert, and only here.** The chain already names the three + phases. Everywhere else the AAA comments stay. +- **`Context` is the only state.** Steps take it through the constructor and hold no fields of their + own; BDTest gives each test its own context, which is what keeps parallel and repeated runs honest. +- **`[StepText]` is written for the report's reader.** `{0}` is replaced with the argument's actual + value at run time, so the report states what the scenario did, not what it was called. +- **Domain content still comes from `TestBuilder`.** The vocabulary changes at the step; the test-data + rule does not. + +Assertions inside a step are ordinary awaited TUnit assertions. `BDTest.Reporting` produces the +single-file HTML report — publish it as a build artifact from the job that ran the suite. + +## A test that needs a real dependency: `Purview.Containers` + +A database, queue, blob store or emulator comes from `Purview.Containers`: a throwaway Linux container +behind one API over Microsoft WSL Containers or Docker, with typed modules for SQL Server, PostgreSQL, +Redis, MySQL, RabbitMQ, Azurite and NATS. + +The two refused alternatives: + +- **A shared instance** (a local SQL Server, a team database) makes tests order-dependent and failures + unreproducible — the state a test found was left by whatever ran before it, on whatever machine. +- **Testcontainers directly** ties the suite to a Docker installation, which on Windows is an install + and a licensing question per developer and per runner. The WSL backend removes it, and the same test + code runs on either. + +```csharp +await using var postgres = new PostgreSqlBuilder() + .WithDatabase("tests") + .WithUsername("postgres") + .WithPassword("postgres") + .Build(); + +await postgres.StartAsync(); + +await using var connection = new NpgsqlConnection(postgres.GetConnectionString()); +await connection.OpenAsync(); +``` + +`Build()` validates the configuration, `StartAsync()` returns only once every wait strategy is +satisfied — there is no sleep to tune — and `await using` disposes the container at end of scope. + +Three rules: + +- **Its own `.IntegrationTests` project**, so the tier is in the name and the suite stays out + of the pull-request filter without anybody remembering an attribute. +- **Skip locally when no backend is usable; fail in CI.** Two different situations, so never one + outcome. Locally, probe and skip. In CI, pin the backend with `PURVIEW_CONTAINERS_BACKEND` — a named + backend never falls back, so a missing runtime fails loudly instead of letting a green job mean + nothing ran: + + ```csharp + [Before(Class)] + public static async Task EnsureABackendIsAvailable() + { + var backends = await ContainerBackends.ProbeAllAsync(); + + Skip.Unless(backends.Any(backend => backend.IsUsable), "No container backend is available."); + } + ``` + + Left unpinned, automatic selection probes in priority order and prefers WSL Containers over Docker. +- **Pin the version exactly.** `Purview.Containers` is **experimental and prerelease** — API, defaults + and packaging rules can change between releases. The packages are a family, so one property in + `Directory.Packages.props` governs all of them and a bump cannot be applied to half. diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index f8a4785..8080adf 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -17,7 +17,7 @@ "rollForward": false }, "purview.build": { - "version": "0.7.1", + "version": "0.8.0", "commands": [ "purview-build" ], diff --git a/.editorconfig b/.editorconfig index b511789..ea03058 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,4 +1,4 @@ -# Purview.BuildSdk 1.1.0 - managed file, do not edit +# Purview.BuildSdk 1.2.0 - managed file, do not edit # # This file is written by Purview.BuildSdk and is replaced whenever the SDK version changes. The version # on the first line is how the SDK recognises a file it owns: a .editorconfig without that marker is diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6aac13e..4389112 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -15,5 +15,5 @@ Keep product, architecture, and general engineering standards centralized in `AG - Apply the `AGENTS.md` testing bar strictly (TUnit/TUnit.Mocks, AAA comments, naming, cancellation token rule). - Treat work as incomplete until relevant tests pass. -- Consult the repository `.agents/` folder for additional skills/workflows that may improve execution quality. +- Before specialist work, load the relevant installed skill from `.agents/skills/` (committed, with `skills-lock.json`); `AGENTS.md` ("Installed skills") lists each one, when to load it, and how to add or update them. Install only into `.agents/` (`-a universal`), never into `.claude/` or another per-agent folder. - Keep edits minimal, focused, and aligned with existing SDK and repository conventions. diff --git a/.gitignore b/.gitignore index a047b9b..ef0ee01 100644 --- a/.gitignore +++ b/.gitignore @@ -662,3 +662,7 @@ BenchmarkDotNet.Artifacts/ !build/ .tools/ + +# Agent skills are installed into the universal .agents/skills/ folder only, and that folder and +# skills-lock.json are committed. Per-agent folders (and Claude Code's local settings) stay out. +/.claude/ diff --git a/AGENTS.md b/AGENTS.md index 32ede14..4b0a778 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,19 +10,57 @@ Do **not** suppress or mute compiler, analyser, or build warnings by adding `/SKILL.md`, every installed folder is in the lockfile, no +authored `skills/` skill is copied into `.agents/skills/`, no `.claude/skills/` copy exists, and no +`CLAUDE.md` is present. + +## Authoring skills Authored skills live in the repository-root `skills/` folder. **Nothing packs them into a NuGet package.** `Purview.BuildSdk` 1.0.6 removed skill packaging outright — the skills CLI clones the *git* repository and only discovers a root `skills/` folder, so that is the one delivery route. `PurviewSkillsToPack`, the `IncludeRepositorySkillsInPackage` shim in `src/Directory.Build.targets`, -and the `.agents/**/*` entries in `.config/purview-build.json` went with it. - -`.agents/skills/` is the CLI's install location for skills from **other** repositories, and is -gitignored. This repository's own skills are never installed into it — that would leave two copies to -drift apart. Read them at their authored path, `skills//SKILL.md`. +and the `.agents/**/*` entries in `.config/purview-build.json` went with it. Read them at their +authored path, `skills//SKILL.md`. -Install them into your agent's own directories with: +Consumers install them with: ```bash npx skills add purview-dev/sourcegenerator-framework diff --git a/CHANGELOG.md b/CHANGELOG.md index b7af299..daf8376 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,24 @@ that version to nuget.org. This file was reconstructed from the conventional-commit history at 1.1.1, so entries before that release summarise what the commits recorded rather than what was written at the time. +## [1.2.2] - 2026-10-27 + +### Fixed + +- `Purview.SourceGeneratorFramework.dll` leaked into application output. The build targets declared + the framework assembly as copy-to-output content so a dependent component's bin folder could load + the unmerged generator, but copy-to-output content flows transitively through every + `ProjectReference` — including `OutputItemType="Analyzer"` / `ReferenceOutputAssembly="false"` + ones. A library that referenced a generator project as an analyzer, and every application that + referenced that library by project, received the compiler-host DLL in its build output, + `deps.json`, publish output, single-file bundle and Blazor WebAssembly `_framework` folder. Native + AOT and trimmed publishes shipped it, and the WebAssembly AOT compiler failed with "Could not load + Microsoft.CodeAnalysis". `DeclarePurviewFrameworkOutputCopy` now copies the assembly into the + component's own output with a plain `Copy` (recorded in `FileWrites`); a dependent component or + test project that references the component as an assembly still receives it beside its own output + through assembly-reference resolution, and an analyzer-only reference never does. Package consumers + were not affected: the package ships only the merged analyzer, which is unchanged. + ## [1.2.0] - 2026-10-21 ### Fixed diff --git a/Directory.Packages.props b/Directory.Packages.props index 05cce36..e84e3b8 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -23,6 +23,6 @@ - + diff --git a/docs/wiki/Getting-Started.md b/docs/wiki/Getting-Started.md index e32cbd1..c9ea74f 100644 --- a/docs/wiki/Getting-Started.md +++ b/docs/wiki/Getting-Started.md @@ -52,8 +52,9 @@ unmerged assembly + loose framework DLL shape is required (see [Packaging.md](Pa A code-fix component can reference the generator component normally (`ProjectReference`, `ReferenceOutputAssembly` not `false`) when it needs the generator's internal diagnostic identity. The generator's **analyzer artifact** is the merged, self-contained assembly, while its **bin output** -stays unmerged; the framework assembly is copied beside that bin output and flows transitively -through `ProjectReference`, so the dependent component's bin folder is self-sufficient. The dependent +stays unmerged; the framework assembly is copied beside that bin output, and assembly-reference +resolution copies it beside the dependent component's bin output, so that folder is self-sufficient. +A project that references a component only as an analyzer never receives it. The dependent component's analyzer closure includes the referenced component's merged artifact — it is never IL-merged a second time, which would duplicate its types. See [Packaging.md](Packaging.md#a-component-that-references-another-component). diff --git a/docs/wiki/Packaging.md b/docs/wiki/Packaging.md index c5427f0..96a8b33 100644 --- a/docs/wiki/Packaging.md +++ b/docs/wiki/Packaging.md @@ -115,10 +115,16 @@ framework. self-contained assembly from `obj/.../purview-merged//`; its **bin output** stays unmerged and references `Purview.SourceGeneratorFramework`. - Because a package consumer's framework `lib/` asset is not copied to output, the framework - assembly is declared as copy-to-output content beside the generator's bin output. Content with - `CopyToOutputDirectory` flows transitively through `ProjectReference`, so the dependent code-fix - component's bin folder is self-sufficient too — that is what lets Visual Studio load the code-fix + assembly is copied beside the generator's bin output by a plain copy step. The dependent code-fix + component references the generator's assembly, so assembly-reference resolution copies the + framework assembly beside its bin output too — that is what lets Visual Studio load the code-fix provider from its own bin without a `FileNotFoundException` for the framework assembly. +- The copy is deliberately **not** copy-to-output content. Content with `CopyToOutputDirectory` + flows transitively through every `ProjectReference`, including `OutputItemType="Analyzer"` / + `ReferenceOutputAssembly="false"` ones, so before 1.2.2 every application that referenced a + generator's library by project received `Purview.SourceGeneratorFramework.dll` in its build output, + `deps.json` and publish output, breaking Native AOT and Blazor WebAssembly AOT publishes. A project + that references a component only as an analyzer now never receives it. - The dependent component's **analyzer closure** (what `GetSourceGeneratorAnalyzerFiles` returns) includes the referenced component's analyzer artifact. The referenced component is returned as its own merged assembly and is **never** IL-merged into the dependent component, which would duplicate diff --git a/global.json b/global.json index 8b388e3..c9e8fce 100644 --- a/global.json +++ b/global.json @@ -5,7 +5,7 @@ "allowPrerelease": true }, "msbuild-sdks": { - "Purview.BuildSdk": "1.1.0" + "Purview.BuildSdk": "1.2.0" }, "test": { "runner": "Microsoft.Testing.Platform" diff --git a/package.json b/package.json index 7dcc6d3..a6ce1bb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sourcegenerator-framework", - "version": "1.2.1", + "version": "1.2.2", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/skills-lock.json b/skills-lock.json new file mode 100644 index 0000000..8c8eb26 --- /dev/null +++ b/skills-lock.json @@ -0,0 +1,89 @@ +{ + "version": 1, + "skills": { + "purview-architecture": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-architecture/SKILL.md", + "computedHash": "d194bd0f30091ffab6ef35c29582e56511d2daca32fdbb1942c50134198f9270" + }, + "purview-architecture-testing": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-architecture-testing/SKILL.md", + "computedHash": "5907c6bf28788b755b752041e2f0cd88222604a949988d487c25940b6284fce4" + }, + "purview-buildsdk-adoption": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-adoption/SKILL.md", + "computedHash": "ca2e13fc8074281f8ca2b510b742ae99db28f97d78f5fb36a6f27936a91941fc" + }, + "purview-buildsdk-configuration": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-configuration/SKILL.md", + "computedHash": "88b4a655efdc058b9a10adb1b5cbcd44e89bff6ed8c858e066092e5be0ca3e0b" + }, + "purview-buildsdk-project-detection": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-project-detection/SKILL.md", + "computedHash": "7c30062fe592105b7a2539384843759521c5a3f90ace8f7c60f0e6ae2405b0b1" + }, + "purview-buildsdk-project-layout": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-project-layout/SKILL.md", + "computedHash": "101f09c92cf97e93387b926c48adab9598a0cf0a64a4932d41085ab0a0078cfa" + }, + "purview-buildsdk-repository-review": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-repository-review/SKILL.md", + "computedHash": "d366821995ff410caa436ccb996eaa6429f1ba51ba985b210d74ec3963f6c06f" + }, + "purview-buildsdk-skill-installation": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-skill-installation/SKILL.md", + "computedHash": "b61cce9f0f213469c3acab4dd5ebaec450ee2be730680b166c9718c901aedffe" + }, + "purview-buildsdk-skill-tests": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-skill-tests/SKILL.md", + "computedHash": "807b64466de0a871e5f8e678e7686c48a06d8bb9d42a03448f604a35d40d3935" + }, + "purview-buildsdk-testing": { + "source": "purview-dev/build-sdk", + "sourceType": "github", + "skillPath": "skills/purview-buildsdk-testing/SKILL.md", + "computedHash": "d6a2ac1167faf68e8c27493de81c2e5b0d8183e523de4411aca5f5b9249c8a9c" + }, + "purview-engineering-principles": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-engineering-principles/SKILL.md", + "computedHash": "fd38d374b816ecf77964c41486e7c734ea34998c3f2a5538c90a2ba793905994" + }, + "purview-persistence": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-persistence/SKILL.md", + "computedHash": "0523ecc210bf3571e2fccb859128a499fd80694e9d4089b377a8d39d73c028f4" + }, + "purview-telemetry": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-telemetry/SKILL.md", + "computedHash": "08ba26e1379dfa2bb826ddfd011e95ac84cd583b865f0b89673d7302b2c2c27d" + }, + "purview-testing-conventions": { + "source": "purview-dev/engineering-playbook", + "sourceType": "github", + "skillPath": "skills/purview-testing-conventions/SKILL.md", + "computedHash": "388b0a79b565ed45a8513d77e8ed6e3d4491f96b43328cb09933cec7811c10f6" + } + } +} diff --git a/skills/purview-sourcegenerator-packaging/SKILL.md b/skills/purview-sourcegenerator-packaging/SKILL.md index 92350db..a0e7f80 100644 --- a/skills/purview-sourcegenerator-packaging/SKILL.md +++ b/skills/purview-sourcegenerator-packaging/SKILL.md @@ -76,10 +76,14 @@ identity. Both are `IsRoslynComponent`; only the generator references the framew - The referenced component is returned as **its own merged assembly** and is never IL-merged into the dependent one — that would duplicate its types, including `InternalsVisibleTo`-visible internals, inside a second analyzer in the same host. -- The framework assembly is declared as copy-to-output content beside the generator's `bin`, and - content with `CopyToOutputDirectory` flows transitively through `ProjectReference`. That is what - lets Visual Studio load the dependent code-fix provider from its own `bin` without a - `FileNotFoundException` for the framework assembly. +- The framework assembly is copied beside the generator's `bin` (a plain copy, not copy-to-output + content). A dependent component that references the generator as an assembly gets it copied + beside its own `bin` by ordinary assembly-reference resolution, which is what lets Visual Studio + load the dependent code-fix provider without a `FileNotFoundException` for the framework assembly. +- A project that references a component only as an analyzer (`ReferenceOutputAssembly="false"`) + never receives the framework assembly, and neither does any application downstream of it. Since + 1.2.2 it no longer leaks into app build output, `deps.json`, publish output, single-file bundles or + Blazor WebAssembly `_framework`, where it broke Native AOT and WebAssembly AOT publishing. - Visual Studio's project system resolves an `OutputItemType=Analyzer` project reference to the referenced project's **unmerged** `bin` assembly and adds it to the compiler's analyzers — the command-line build does not. `Purview.BuildSdk` removes that item before adding the resolved diff --git a/src/Directory.Build.props b/src/Directory.Build.props index dbd0908..37e40a0 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -6,6 +6,11 @@ Purview.SourceGeneratorFramework true https://github.com/purview-dev/sourcegenerator-framework + + universal diff --git a/src/src/SourceGeneratorFramework/Sdk/README.md b/src/src/SourceGeneratorFramework/Sdk/README.md index 97a3f57..825e59e 100644 --- a/src/src/SourceGeneratorFramework/Sdk/README.md +++ b/src/src/SourceGeneratorFramework/Sdk/README.md @@ -33,9 +33,13 @@ Only the generator references the framework, so: - the generator merges to a **merged, self-contained analyzer artifact** while its **bin output stays unmerged**; -- the framework assembly is copied beside the generator's bin output and flows transitively through - `ProjectReference`, so the dependent component's bin folder is self-sufficient — the IDE can load - the code-fix provider without a `FileNotFoundException` for the framework assembly; and +- the framework assembly is copied beside the generator's bin output, and assembly-reference + resolution copies it beside the dependent component's bin output, so that folder is + self-sufficient — the IDE can load the code-fix provider without a `FileNotFoundException` for the + framework assembly; +- a project that references a component only as an analyzer (`ReferenceOutputAssembly="false"`), and + any application downstream of it, never receives the framework assembly in its build or publish + output; and - the dependent component's analyzer closure includes the referenced component's merged artifact (it is never IL-merged a second time, which would duplicate its types). diff --git a/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets b/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets index fc6eeae..a19ef44 100644 --- a/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets +++ b/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets @@ -75,31 +75,56 @@ lib assets). A dependent component that references this component normally resolves the unmerged bin copy at runtime, so the framework assembly must sit beside it. - Declare the resolved framework assembly as copy-to-output content. Content with - CopyToOutputDirectory flows transitively through ProjectReference, so a dependent component's - bin folder becomes self-sufficient without any consumer-side configuration, while the merged - analyzer artifact stays the only thing returned to Roslyn. + The framework assembly is copied into this component's own output directory by a plain Copy + task. It is deliberately NOT declared as copy-to-output content (None/Content with + CopyToOutputDirectory), which is what earlier versions did. Copy-to-output content flows + transitively through *every* ProjectReference - GetCopyToOutputDirectoryItems ignores + ReferenceOutputAssembly="false" and OutputItemType="Analyzer" - so a library that referenced a + component as an analyzer, and every application that referenced that library, received the + compiler-host framework DLL in its build output, deps.json, publish output, single-file + bundle and Blazor WebAssembly _framework folder. That broke Native AOT, trimming and the + WebAssembly AOT compiler ("Could not load Microsoft.CodeAnalysis"). CopyToPublishDirectory="Never" + would only keep it out of publish - it would still land in every downstream build output - + and the receiving projects never import these targets, so the flow cannot be filtered there. + + A dependent project still finds the framework beside it through ordinary assembly + reference resolution instead: the unmerged component assembly references + Purview.SourceGeneratorFramework, and ResolveAssemblyReferences resolves (and copies local) a + primary reference's dependencies from the primary reference's own directory. So: + + * a dependent Roslyn component, or an in-repo test project, that references this component + as an assembly (a normal ProjectReference) gets the DLL copied beside its own output; and + * a project that references this component only as an analyzer + (ReferenceOutputAssembly="false") never resolves the component's assembly, so it - and + everything downstream of it - never receives the framework DLL at all. + + The copy runs after CopyFilesToOutputDirectory, so it is in place before any referencing + project runs ResolveAssemblyReferences, and it is recorded in FileWrites so IncrementalClean + keeps it and Clean removes it. This is deliberately *not* a SourceGeneratorRuntimeDependency: that item feeds the analyzer closure and the package, where the loose framework DLL must never appear. --> - - - + + <_PurviewFrameworkOutputCopyDestination>$([System.IO.Path]::GetFullPath('$(OutDir)Purview.SourceGeneratorFramework.dll')) + + + + + + Exe + + net$(BundledNETCoreAppTargetFrameworkVersion) + Fixture.Application + Fixture.Application + false + + true + + false + + + + + + diff --git a/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Application/Program.cs b/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Application/Program.cs new file mode 100644 index 0000000..4ead7c2 --- /dev/null +++ b/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Application/Program.cs @@ -0,0 +1 @@ +System.Console.WriteLine(Fixture.Consumer.FixtureConsumerType.Marker); diff --git a/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Generator/.editorconfig b/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Generator/.editorconfig index b511789..ea03058 100644 --- a/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Generator/.editorconfig +++ b/src/tests/Fixtures/Purview.ComponentClosureFixture/Fixture.Generator/.editorconfig @@ -1,4 +1,4 @@ -# Purview.BuildSdk 1.1.0 - managed file, do not edit +# Purview.BuildSdk 1.2.0 - managed file, do not edit # # This file is written by Purview.BuildSdk and is replaced whenever the SDK version changes. The version # on the first line is how the SDK recognises a file it owns: a .editorconfig without that marker is diff --git a/src/tests/Repository.UnitTests/InstalledSkillTests.cs b/src/tests/Repository.UnitTests/InstalledSkillTests.cs new file mode 100644 index 0000000..44339a1 --- /dev/null +++ b/src/tests/Repository.UnitTests/InstalledSkillTests.cs @@ -0,0 +1,170 @@ +using System.Text.Json; + +namespace Purview.SourceGeneratorFramework.Repository; + +/// +/// Guards the skills installed from other repositories. They are installed by the skills CLI into the +/// universal .agents/skills/ folder only, and that folder and the CLI's skills-lock.json +/// are committed, so the two must agree and no per-agent copy or instruction file may appear beside +/// them. +/// +/// +/// SkillTests validates this repository's own skills in skills/; these are someone else's, +/// so their content is not checked here, only that the installed set is the recorded set. +/// +public sealed class InstalledSkillTests +{ + static readonly Lazy RepositoryRoot = new(FindRepositoryRoot); + + [Test] + public async Task Lockfile_GivenEveryEntry_HasItsSkillInstalledUnderAgents(CancellationToken cancellationToken) + { + // Arrange + var recorded = await ReadLockfileSkillsAsync(cancellationToken); + + // Act + var missing = recorded + .Where(static name => !File.Exists(Path.Combine(AgentsSkillsFolder(), name, "SKILL.md"))) + .ToArray(); + + // Assert + await Assert.That(recorded).IsNotEmpty().Because("skills-lock.json records no installed skills"); + await Assert + .That(missing) + .IsEmpty() + .Because( + "skills-lock.json records skills that are not installed in .agents/skills/. Restore them with " + + $"`npx -y skills@1 experimental_install`: {string.Join(", ", missing)}" + ); + } + + [Test] + public async Task AgentsSkills_GivenEveryInstalledSkill_IsRecordedInTheLockfile(CancellationToken cancellationToken) + { + // Arrange + var recorded = await ReadLockfileSkillsAsync(cancellationToken); + + // Act + var unrecorded = Directory + .GetDirectories(AgentsSkillsFolder()) + .Select(static path => Path.GetFileName(path)) + .Where(name => !recorded.Contains(name, StringComparer.Ordinal)) + .ToArray(); + + // Assert + await Assert + .That(unrecorded) + .IsEmpty() + .Because( + "every folder in .agents/skills/ must be written by the skills CLI, which records it in " + + $"skills-lock.json: {string.Join(", ", unrecorded)}" + ); + } + + [Test] + public async Task AgentsSkills_GivenTheAuthoredSkills_HoldsNoCopyOfThem(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + var authored = Directory + .GetDirectories(Path.Combine(RepositoryRoot.Value, "skills")) + .Select(static path => Path.GetFileName(path)) + .ToArray(); + + // Act + var copied = authored + .Where(static name => Directory.Exists(Path.Combine(AgentsSkillsFolder(), name))) + .ToArray(); + + // Assert + await Assert + .That(copied) + .IsEmpty() + .Because( + "this repository's own skills live in skills/ only; a copy in .agents/skills/ would drift from " + + $"the authored one: {string.Join(", ", copied)}" + ); + } + + [Test] + public async Task ClaudeFolder_GivenTheInstalledSkills_HoldsNoCopy(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + var claudeSkills = Path.Combine(RepositoryRoot.Value, ".claude", "skills"); + + // Act + var exists = Directory.Exists(claudeSkills); + + // Assert + await Assert + .That(exists) + .IsFalse() + .Because( + "skills are installed into .agents/skills/ only. A .claude/skills/ folder means the CLI ran without " + + "`-a universal`; delete it and reinstall with `-a universal`" + ); + } + + [Test] + public async Task Repository_GivenAgentInstructions_HasNoClaudeMd(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + string[] candidates = + [ + Path.Combine(RepositoryRoot.Value, "CLAUDE.md"), + Path.Combine(RepositoryRoot.Value, ".github", "CLAUDE.md"), + ]; + + // Act + var present = candidates.Where(File.Exists).ToArray(); + + // Assert + await Assert + .That(present) + .IsEmpty() + .Because( + "this repository is not agent-specific: AGENTS.md is its one instruction file, so a CLAUDE.md " + + $"must not be added: {string.Join(", ", present)}" + ); + } + + static string AgentsSkillsFolder() => Path.Combine(RepositoryRoot.Value, ".agents", "skills"); + + static async Task ReadLockfileSkillsAsync(CancellationToken cancellationToken) + { + await using var stream = File.OpenRead(Path.Combine(RepositoryRoot.Value, "skills-lock.json")); + using var lockfile = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken); + + return + [ + .. lockfile + .RootElement.GetProperty("skills") + .EnumerateObject() + .Select(static property => property.Name) + .Order(StringComparer.Ordinal), + ]; + } + + static string FindRepositoryRoot() + { + for ( + var directory = new DirectoryInfo(AppContext.BaseDirectory); + directory is not null; + directory = directory.Parent + ) + { + if ( + File.Exists(Path.Combine(directory.FullName, "AGENTS.md")) + && File.Exists(Path.Combine(directory.FullName, "skills-lock.json")) + && Directory.Exists(Path.Combine(directory.FullName, "src", "src")) + ) + return directory.FullName; + } + + throw new InvalidOperationException( + $"No repository root above '{AppContext.BaseDirectory}' carries AGENTS.md, skills-lock.json and src/src." + ); + } +} diff --git a/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs b/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs index d6e472d..7bc0e26 100644 --- a/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs +++ b/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using System.IO.Compression; using System.Reflection.Metadata; using System.Reflection.PortableExecutable; using System.Text.Json; @@ -14,6 +15,10 @@ namespace Purview.SourceGeneratorFramework; /// the analyzer closure returned by GetSourceGeneratorAnalyzerFiles has no unresolvable /// assembly reference. /// +/// It also asserts that the framework assembly, which exists beside a component's bin output only +/// for a dependent component or test project to load, never reaches a project that references a +/// component as an analyzer, nor any application downstream of one (build output, deps.json or +/// publish output), and that the packed analyzer stays the self-contained merged artifact. /// [NotInParallel(nameof(AnalyzerClosureTests))] public sealed class AnalyzerClosureTests @@ -97,6 +102,187 @@ public async Task ComponentClosure_ContainsNoUnresolvableAssemblyReferences(Canc await Assert.That(string.Join(Environment.NewLine, problems)).IsEmpty(); } + [Test] + public async Task AnalyzerReferenceConsumers_BuildOutput_ExcludesFrameworkAssembly( + CancellationToken cancellationToken + ) + { + // Arrange + var appProject = ProjectPath("Fixture.Application"); + + // Act + await BuildAsync(appProject, cancellationToken); + + // Assert + List problems = []; + AddFrameworkLeakProblems(problems, "Fixture.Consumer (analyzer reference)", BinDirectory("Fixture.Consumer")); + AddFrameworkLeakProblems(problems, "Fixture.Application (references Fixture.Consumer)", AppBinDirectory()); + + await Assert.That(string.Join(Environment.NewLine, problems)).IsEmpty(); + } + + [Test] + public async Task Application_PublishOutput_ExcludesFrameworkAssembly(CancellationToken cancellationToken) + { + // Arrange + var appProject = ProjectPath("Fixture.Application"); + var publishDirectory = CreateScratchDirectory("publish"); + + try + { + // Act + await RunAsync( + "dotnet", + ["publish", appProject, "-c", Configuration, "-v:minimal", "-o", publishDirectory], + Path.GetDirectoryName(appProject)!, + cancellationToken + ); + var resolvedFilesToPublish = await GetItemIdentitiesAsync( + appProject, + "ComputeResolvedFilesToPublishList", + "ResolvedFileToPublish", + cancellationToken + ); + + // Assert + List problems = []; + if (!File.Exists(Path.Combine(publishDirectory, "Fixture.Application.dll"))) + problems.Add($"The publish directory '{publishDirectory}' does not contain the application."); + + AddFrameworkLeakProblems(problems, "Fixture.Application publish output", publishDirectory); + + var leakedPublishItems = resolvedFilesToPublish.Where(static path => IsFrameworkAssembly(path)).ToArray(); + if (leakedPublishItems.Length > 0) + { + // ResolvedFileToPublish is also the input to single-file bundling and to the Blazor + // WebAssembly _framework layout, so this covers both. + problems.Add( + $"ResolvedFileToPublish contains '{FrameworkAssemblyFileName}': " + + string.Join(", ", leakedPublishItems) + ); + } + + await Assert.That(string.Join(Environment.NewLine, problems)).IsEmpty(); + } + finally + { + TryDeleteDirectory(publishDirectory); + } + } + + [Test] + public async Task ComponentPackage_AnalyzerAsset_IsTheSelfContainedMergedArtifact( + CancellationToken cancellationToken + ) + { + // Arrange + var generatorProject = ProjectPath("Fixture.Generator"); + var packDirectory = CreateScratchDirectory("pack"); + + try + { + // Act + await RunAsync( + "dotnet", + [ + "pack", + generatorProject, + "-c", + Configuration, + "-v:minimal", + "-p:IsPackable=true", + "-o", + packDirectory, + ], + Path.GetDirectoryName(generatorProject)!, + cancellationToken + ); + // Resolved with the same global properties as the pack, so both see the same build. + var mergedClosure = await ResolveAnalyzerClosureAsync( + generatorProject, + cancellationToken, + "-p:IsPackable=true" + ); + + // Assert + List problems = []; + var package = Directory.GetFiles(packDirectory, "*.nupkg").SingleOrDefault(); + if (package is null) + { + problems.Add($"'dotnet pack' did not produce a package in '{packDirectory}'."); + } + else + { + await using var packageStream = File.OpenRead(package); + using ZipArchive archive = new(packageStream, ZipArchiveMode.Read); + var entries = archive.Entries.Select(static entry => entry.FullName).ToArray(); + + var looseFramework = entries.Where(static entry => IsFrameworkAssembly(entry)).ToArray(); + if (looseFramework.Length > 0) + problems.Add( + $"The package contains the loose framework assembly: {string.Join(", ", looseFramework)}" + ); + + var analyzerEntry = archive.GetEntry("analyzers/dotnet/cs/Fixture.Generator.dll"); + if (analyzerEntry is null) + { + problems.Add( + "The package does not contain analyzers/dotnet/cs/Fixture.Generator.dll. Entries: " + + string.Join(", ", entries) + ); + } + else + { + var packedAnalyzer = Path.Combine(packDirectory, "packed-Fixture.Generator.dll"); + await using (var entryStream = +#if NET10_0_OR_GREATER + await analyzerEntry.OpenAsync(cancellationToken) +#else + analyzerEntry.Open() +#endif + ) + await using (var fileStream = File.Create(packedAnalyzer)) + await entryStream.CopyToAsync(fileStream, cancellationToken); + + var references = ReadAssemblyReferenceNames(packedAnalyzer); + if (references.Contains(Path.GetFileNameWithoutExtension(FrameworkAssemblyFileName))) + { + problems.Add( + "The packed analyzer references Purview.SourceGeneratorFramework, so it is not the " + + "self-contained merged artifact." + ); + } + + var merged = mergedClosure.SingleOrDefault(static path => + path.EndsWith("Fixture.Generator.dll", StringComparison.OrdinalIgnoreCase) + ); + if (merged is null) + { + problems.Add("The analyzer closure did not return the merged Fixture.Generator.dll."); + } + else if ( + !AreSameBytes( + await File.ReadAllBytesAsync(packedAnalyzer, cancellationToken), + await File.ReadAllBytesAsync(merged, cancellationToken) + ) + ) + { + problems.Add( + $"The packed analyzer differs from the merged artifact '{merged}' that the analyzer " + + "closure returns." + ); + } + } + } + + await Assert.That(string.Join(Environment.NewLine, problems)).IsEmpty(); + } + finally + { + TryDeleteDirectory(packDirectory); + } + } + [Test] public async Task ProjectReferenceConsumer_ReceivesBuildTransitiveCompilerVisibleProperties( CancellationToken cancellationToken @@ -210,6 +396,99 @@ public async Task ConsumerAnalyzerSet_ExcludesUnmergedComponentAnalyzer(Cancella await Assert.That(string.Join(Environment.NewLine, problems)).IsEmpty(); } + static bool AreSameBytes(byte[] left, byte[] right) => left.AsSpan().SequenceEqual(right); + + static bool IsFrameworkAssembly(string path) => + Path.GetFileName(path).Equals(FrameworkAssemblyFileName, StringComparison.OrdinalIgnoreCase); + + static void AddFrameworkLeakProblems(List problems, string description, string directory) + { + if (!Directory.Exists(directory)) + { + problems.Add($"{description}: the output directory '{directory}' does not exist."); + return; + } + + foreach (var file in Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories)) + { + if (IsFrameworkAssembly(file)) + { + problems.Add( + $"{description}: '{file}' is the compiler-host framework assembly, which must never " + + "reach a project that only references a component as an analyzer." + ); + } + else if ( + file.EndsWith(".deps.json", StringComparison.OrdinalIgnoreCase) + && File.ReadAllText(file).Contains(FrameworkAssemblyFileName, StringComparison.OrdinalIgnoreCase) + ) + { + problems.Add($"{description}: '{file}' lists '{FrameworkAssemblyFileName}'."); + } + } + } + + static async Task> GetItemIdentitiesAsync( + string projectPath, + string target, + string itemType, + CancellationToken cancellationToken + ) + { + var output = await RunAsync( + "dotnet", + [ + "msbuild", + projectPath, + $"-t:{target}", + $"-getItem:{itemType}", + $"-p:Configuration={Configuration}", + "-nologo", + ], + Path.GetDirectoryName(projectPath)!, + cancellationToken + ); + + var start = output.IndexOf('{', StringComparison.Ordinal); + if (start < 0) + { + throw new InvalidOperationException( + $"Unable to locate the item JSON in the msbuild output:{Environment.NewLine}{output}" + ); + } + + using var document = JsonDocument.Parse(output[start..]); + List identities = []; + foreach (var item in document.RootElement.GetProperty("Items").GetProperty(itemType).EnumerateArray()) + identities.Add(item.GetProperty("Identity").GetString()!); + + return identities; + } + + static string AppBinDirectory() + { + var bin = Path.Combine(FixtureRoot, "Fixture.Application", "bin", Configuration); + return Directory.GetDirectories(bin, "net*").Single(); + } + + static string CreateScratchDirectory(string purpose) + { + var directory = Path.Combine(Path.GetTempPath(), $"purview-closure-{purpose}-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + return directory; + } + + static void TryDeleteDirectory(string directory) + { + try + { + if (Directory.Exists(directory)) + Directory.Delete(directory, recursive: true); + } + catch (IOException) { } + catch (UnauthorizedAccessException) { } + } + static bool IsPermittedReference(string name) => name.StartsWith("Microsoft.CodeAnalysis", StringComparison.Ordinal) || name.StartsWith("System.Composition", StringComparison.Ordinal) @@ -233,7 +512,11 @@ static List ReadAssemblyReferenceNames(string path) return names; } - static async Task> ResolveAnalyzerClosureAsync(string projectPath, CancellationToken cancellationToken) + static async Task> ResolveAnalyzerClosureAsync( + string projectPath, + CancellationToken cancellationToken, + params string[] extraArguments + ) { var output = await RunAsync( "dotnet", @@ -244,6 +527,7 @@ static async Task> ResolveAnalyzerClosureAsync(string projectPath, "-getTargetResult:GetSourceGeneratorAnalyzerFiles", $"-p:Configuration={Configuration}", "-nologo", + .. extraArguments, ], Path.GetDirectoryName(projectPath)!, cancellationToken