diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index e89a9e3..2197486 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -17,7 +17,7 @@ First read `CLAUDE.md` and every **accepted** ADR in `docs/adr/` that relates to **ADR compliance** - ADR-0002: no `catch (Exception)` or bare `catch`. Expected failures (HTTP status, payload, network, timeout) are returned as `OpenUrzednikResult` errors. Only caller cancellation, argument errors and fatal errors are thrown; in every public method of a public type that returns a `Task` (provider clients, Core's extensions on `Task<…>`), a `null` argument or an undefined enum value throws synchronously, before the task is returned (flag a public `async` method that checks them, or a `ValidationError` for them), while a non-null value of the right type that fails a format or range check is a returned `ValidationError`. Timeouts are distinguished from caller cancellation. Errors implement `CreateException()` returning `OpenUrzednikException` subtypes; exceptions keep their error in `Error`/`Errors`. Several validation errors are thrown as one `OpenUrzednikValidationException` listing all of them, never `AggregateException`; several errors of other kinds as the first error's exception with all of them in `Errors`. Resilience-handler rejections are translated in DI packages, not caught in providers. - ADR-0003: no `ILogger`/`ActivitySource` in Core or provider packages, except Core's internal `Telemetry/Activities` (the .NET fallback trace source, compiled on .NET only). Members of those types that other packages call through `InternalsVisibleTo` are a binary contract: flag a change or removal outside a major. Span and tag naming; HTTP tags use the OpenTelemetry names (`http.request.method`, `url.path`, `http.response.status_code`); errors set `error.type`. Activity sources are named `OpenUrzednik.`. Every log call passes an event id; a new log event has a new id in `RestLogEvents` (HTTP, 1xxx) or the provider's `LogEvents` (2xxx); flag a renumbered, reused or missing id, and a `RestLogEvents` entry not mirrored in `LogEvents`. Flag any change to `IOpenUrzednikLogger`, `IOpenUrzednikTraceSource` or `IOpenUrzednikSpan` after 1.0 (frozen; new capabilities are separate interfaces), and any change to a stable name (source, span, documented tag, `error.type`, logger category, event id) outside a major. `IsEnabled` guards. No secrets (API keys, session ids, tokens) or personal identifiers (PESEL, NIP, REGON, bank account numbers) in logs, span names or tags, error and exception messages, error metadata or `Exception.Data`; at most the last 4 characters. -- ADR-0004: no new `PackageReference` in Core or provider projects, apart from netstandard2.0-only official Microsoft BCL packages (currently `System.Text.Json`, `System.Net.Http.Json`, `Microsoft.Bcl.TimeProvider`, `Microsoft.Bcl.HashCode`; any other one needs approval). `System.Diagnostics.DiagnosticSource` is allowed only in `OpenUrzednik.Diagnostics`. No `Version=` on package references (central package management). A dependency of a published library moves only by patch unless the PR says why a minor or major raise is needed; the DI package's `Microsoft.Extensions.*` keep the `net8.0` override (8.0.x), and no package gets two conditional `Include` rows. DI packages don't add a resilience handler themselves (the app chains `AddStandardResilienceHandler()`), and they translate its rejection exceptions into errors. +- ADR-0004: no new `PackageReference` in Core or provider projects, apart from netstandard2.0-only official Microsoft BCL packages (currently `System.Text.Json`, `System.Net.Http.Json`, `Microsoft.Bcl.TimeProvider`, `Microsoft.Bcl.HashCode`; any other one needs approval). `System.Diagnostics.DiagnosticSource` is allowed only in `OpenUrzednik.Diagnostics`. The build enforces these lists (`CheckPackageReferenceAllowList` in `src/Directory.Build.targets`, `OpenUrzednikAllowedPackageReferences` in a csproj): flag any change to a list or to the check as needing an ADR or the maintainer's approval. No `Version=` on package references (central package management). A dependency of a published library moves only by patch unless the PR says why a minor or major raise is needed; the DI package's `Microsoft.Extensions.*` keep the `net8.0` override (8.0.x), and no package gets two conditional `Include` rows. DI packages don't add a resilience handler themselves (the app chains `AddStandardResilienceHandler()`), and they translate its rejection exceptions into errors. - ADR-0005: code builds for netstandard2.0 and passes `test (net472)`. Package validation stays on with no suppressions: flag a `CompatibilitySuppressions.xml`, a `CPxxxx` in `NoWarn`, or `EnablePackageValidation` turned off (the only exception is ADR-0011's CP0006 for a new client-interface method). `OpenUrzednik.snk`, `SignAssembly`, `PublicKey` and `assemblyVersion.precision` don't change; a project doesn't turn signing off; a third-party `InternalsVisibleTo` friend has its own `Key`. `#if` kept inside helpers (polyfills in `src/Polyfills`, `src/Http/Polyfills`, `Nbp/Polyfills`, `Extensions.Logging/Polyfills`). Every build has the same public signatures for what it shares: flag a public `DateOnly` (parameter, property or return type) and any interface member that exists only on .NET; public dates are `DateTime`, with `DateOnly` overloads only as .NET-only extension methods. Internally, dates are written as `DateOnly` (an alias for `DateTime` on netstandard2.0) and compared by `DayNumber`. A .NET target that isn't the lowest one may be removed in a minor version once Microsoft ends its support; the lowest .NET target and netstandard2.0 are removed only in a major. Targets are netstandard2.0 plus the .NET LTS releases in support. - ADR-0006: REST/JSON provider HTTP goes through `RestRequestExecutor` (shared source in `src/Http`); those providers never call `HttpClient.SendAsync` (a SOAP or other transport follows its provider ADR), and a request URI (path or query) carrying a personal identifier must not reach `url.path`, logs, error messages or `ErrorResponseContext.RequestPath` (display path; a 400's server message must be masked or dropped), and no public signature uses a type from `OpenUrzednik.Http.Infrastructure`. Request and response are disposed. - ADR-0007: public types follow the namespace layout (clients, interfaces and options in the provider root namespace, models in area namespaces, no public `.Extensions`/`.Options` namespaces in Core or providers; `OpenUrzednik.Extensions.Logging` and internal namespaces are exempt); the default constructor stays simple; nothing that shapes requests (URL builders, path enums) is public or a public constructor parameter: customization goes through `NbpOptions` and `DelegatingHandler`s (auth that needs protocol calls, or credentials passed as API arguments, belongs in the provider, per its provider ADR); `CancellationToken cancellationToken = default` comes last in both interface and implementation. diff --git a/CLAUDE.md b/CLAUDE.md index ab1944b..39aba94 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,7 +41,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and **Errors ([ADR-0002](docs/adr/0002-result-pattern-and-error-handling.md))** - Public async APIs return `Task>`. Failures that can happen during normal execution are returned, not thrown: validation, every non-success HTTP status, bad payloads, network errors, timeouts. - Throw only for caller cancellation (`OperationCanceledException` when the caller's token is cancelled), programmer errors (`ArgumentNullException`, invalid options) and fatal errors. In every public method of a public type that returns a `Task`, a `null` argument and an undefined enum value (`ArgumentOutOfRangeException`) throw **synchronously**: the method isn't `async` and checks them before it creates the task (typically it returns the task of a private `…CoreAsync` method). A non-null value of the right type that fails a format or range check is a returned `ValidationError`. -- **Never write `catch (Exception)` or a bare `catch`.** Catch specific types and convert them to errors. +- **Never write `catch (Exception)` or a bare `catch`.** Catch specific types and convert them to errors. CA1031 flags it in `src/` (`src/.editorconfig`). - A timeout is an `OperationCanceledException` while `cancellationToken.IsCancellationRequested == false`. Return it as an error. - New error types derive from `OpenUrzednikError`, define `public const string ErrorCode`, and implement `CreateException()` returning an `OpenUrzednikException` subtype. The public `ToException()` calls it and attaches the error (`OpenUrzednikException.Error`/`Errors`). - Several errors (only validation produces them) are thrown by `EnsureSuccess()` as one `OpenUrzednikValidationException` listing all of them, never as `AggregateException`. Several errors of other kinds: the first error's exception, with all of them in `Errors`. @@ -55,7 +55,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and - Versions live in `Directory.Packages.props` (central package management). Never put `Version=` on a `PackageReference`. - Version choice applies to the **dependencies of the published libraries** only; tests, build tooling and anything used only inside the repository take the latest version. For those dependencies: **abstractions** (API contracts under semantic versioning, e.g. `Microsoft.Extensions.Logging.Abstractions`) use the **lowest** version: the first release (`x.0.0`) of the major that matches our oldest .NET target (currently 8.0.0), because it is a minimum for every app. Move to a later patch only if that version has a known vulnerability; the minimum rises when the oldest .NET target is removed (ADR-0005). **Implementations** (code that runs, e.g. `System.Text.Json`, `Microsoft.Extensions.Http`) use the **latest patch** of their chosen minor, to get bug and security fixes; a minor or major raise is a deliberate change. `Directory.Packages.props` follows the split, and Dependabot ignores version updates of `*.Abstractions`, so watch Dependabot alerts for them. The DI package's `Microsoft.Extensions.Http` and `Microsoft.Extensions.Options` take the latest patch of the major matching the target: a normal row with 10.0.x (`net10.0`, `netstandard2.0`) and a `net8.0`-only `PackageVersion Update` override with 8.0.x, so a .NET 8 app stays on `Microsoft.Extensions.*` 8.x. Don't put two conditional `Include` rows for one package: Dependabot skips such packages. Dependabot proposes only patch updates for the published libraries' dependencies, in one group (listed in `.github/dependabot.yml`); the `net8.0` overrides are patched by hand. - **If a change raises an abstraction above its minimum** (typically a Dependabot PR), stop and ask the maintainer explicitly what to do, naming the package, both versions and the rule. Don't merge, approve, rebase onto or build on that change until they answer explicitly. -- Do not add a `PackageReference` to a `src/` project without an ADR or explicit approval. +- Do not add a `PackageReference` to a `src/` project without an ADR or explicit approval. The build fails on a package the project's list doesn't name (`CheckPackageReferenceAllowList` in `src/Directory.Build.targets`; the integration packages and Diagnostics set `OpenUrzednikAllowedPackageReferences` in their csproj). Never extend a list to make a build pass. **Telemetry ([ADR-0003](docs/adr/0003-telemetry-abstractions.md))** - Use `IOpenUrzednikLogger`, `IOpenUrzednikTraceSource` and `IOpenUrzednikSpan` from Core. Don't use `ILogger` or `ActivitySource` in Core or provider packages. The one exception is the fallback trace source: on the .NET targets a client without a trace source uses the shared `OpenUrzednik.` source, through Core's internal `Telemetry/Activities` (compiled into Core on .NET only, linked into `OpenUrzednik.Diagnostics` for netstandard2.0; Core grants `InternalsVisibleTo` on .NET only, and a new REST provider must be added to it). netstandard2.0 falls back to `NullOpenUrzednikTraceSource`. Those Core internals are called across packages, and an app may pair a newer Core with older packages: within a major, add to them, never change or remove. @@ -113,7 +113,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and - Large clients are split into `partial` files by area (`NbpCurrencyExchangeRateClient.BuySell.cs`). - Messages and logs are in English. Format dates invariantly (`yyyy-MM-dd`). - No parallel API variants: one public method per operation, returning a result; the exception style comes only through `EnsureSuccess()` / `EnsureSuccessAsync()`. -- Library code (`src/`) awaits with `ConfigureAwait(false)`. +- Library code (`src/`) awaits with `ConfigureAwait(false)`; CA2007 flags a missing one. ## Testing conventions diff --git a/docs/adr/0002-result-pattern-and-error-handling.md b/docs/adr/0002-result-pattern-and-error-handling.md index 1a60551..d757dea 100644 --- a/docs/adr/0002-result-pattern-and-error-handling.md +++ b/docs/adr/0002-result-pattern-and-error-handling.md @@ -84,7 +84,7 @@ The metadata keys are public constants next to the errors that set them: `OpenUr ### Confirmation * WireMock tests cover every error path for each client, asserting the error type: 400 with the server's message, 401 and 403, 404, 429 with `Retry-After`, 5xx, a 200 with an empty array (`NotFoundError`, for endpoints that return an array), malformed JSON, a timeout and a connection failure. This is the canonical list; `CLAUDE.md` and the `/new-endpoint` skill point to it. Unit tests cover the status mapping in the request executor. -* `reviewer` agent and code review reject `catch (Exception)`. +* The analyzer rule CA1031 (do not catch general exception types) is a warning in `src/` (`src/.editorconfig`), which CI turns into an error; the `reviewer` agent and code review reject `catch (Exception)` too. ## More Information @@ -115,3 +115,5 @@ These are breaking changes for preview users. **2026-10-30 change: masked values in validation errors** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). `ValueValidator.GetValidationErrorResult` stores the validated value under `ValueMetadataKey`, so a validator for a personal identifier would copy the identifier into every error, exception and log that shows the metadata. A validator for a personal identifier must pass a masked value, under the rule in [ADR-0003](0003-telemetry-abstractions.md). NBP validates no such value, so nothing changes there. **2026-10-30 change: synchronous argument errors in every task-returning method** ([#159](https://github.com/milten89/OpenUrzednik.NET/issues/159)). The rule from #89 covered only the provider clients. Core's extensions on `Task<…>` (`EnsureSuccessAsync`, `MapAsync`, `BindAsync`) checked their arguments inside an `async` method, so a `null` task or delegate came back as a faulted task. They now check their arguments, then return the task of a private `async` method, and the rule covers every public method of a public type that returns a task. It is a breaking change for callers who observed the exception on the task. + +**2026-10-30 change: CA1031 enforces the catch rule** ([#110](https://github.com/milten89/OpenUrzednik.NET/issues/110)). The rule against `catch (Exception)` and a bare `catch` relied on review. CA1031 is now a warning in `src/`, and an error in CI; `src/` had no violations. Tests aren't covered, since they may catch broadly on purpose. diff --git a/docs/adr/0004-dependency-policy.md b/docs/adr/0004-dependency-policy.md index 25baae7..ed5821f 100644 --- a/docs/adr/0004-dependency-policy.md +++ b/docs/adr/0004-dependency-policy.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-29 +date: 2026-10-30 decision-makers: milten89 --- @@ -53,7 +53,7 @@ Chosen option: "No dependencies in Core/providers; DI and integrations in separa ### Confirmation -`dotnet list package --include-transitive` for each `src/` provider/Core project shows no non-BCL dependencies; the `reviewer` agent flags new `PackageReference`s in Core/provider projects. +The build checks every `src/` project's direct `PackageReference`s against an allow-list (`CheckPackageReferenceAllowList` in `src/Directory.Build.targets`). The lists hold the packages approved so far, a subset of what the table above allows: Core and the providers only `System.Text.Json`, `System.Net.Http.Json`, `Microsoft.Bcl.TimeProvider` and `Microsoft.Bcl.HashCode`, and only on netstandard2.0; the Diagnostics adapter and the integration packages name theirs in their csproj (`OpenUrzednikAllowedPackageReferences`). Any other reference fails the build; a package the table allows still needs approval and a change to the list. `dotnet list package --include-transitive` for each `src/` provider/Core project shows no non-BCL dependencies; the `reviewer` agent flags new `PackageReference`s in Core/provider projects. ## More Information @@ -87,3 +87,5 @@ Chosen option: "No dependencies in Core/providers; DI and integrations in separa **2026-10-29 note: `OpenUrzednik.Http` is shared source** ([#85](https://github.com/milten89/OpenUrzednik.NET/issues/85)). The HTTP layer is no longer a package but internal source compiled into each REST provider ([ADR-0006](0006-shared-http-layer.md)). The "Shared infrastructure" row of the table became "Shared source", and `OpenUrzednik.Nbp` now references `System.Net.Http.Json` on netstandard2.0 itself. **2026-10-29 note: marked rejections** ([#90](https://github.com/milten89/OpenUrzednik.NET/issues/90)). Network failures became a `ConnectionFailedError` ([ADR-0002](0002-result-pattern-and-error-handling.md)), so an `HttpRequestException` alone no longer means `ServiceUnavailableError`. The DI package's handler marks the circuit-breaker and rate-limiter rejections with an `Exception.Data` entry, and the executor returns a `ServiceUnavailableError` for a marked exception (or one that wraps it, as .NET Framework's `HttpClient` may). A marker type would have needed `InternalsVisibleTo` from the provider to the DI package, which also exposes the provider's internal polyfills and makes them clash with the DI package's own copy on netstandard2.0. + +**2026-10-30 note: the build checks the rule** ([#110](https://github.com/milten89/OpenUrzednik.NET/issues/110)). Until now only the `reviewer` agent and code review caught a new dependency. A target in `src/Directory.Build.targets` now fails the build of any `src/` project that references a package its list doesn't name: the default list is the Core and provider row of the table, and the Diagnostics adapter and the integration packages set theirs in their csproj. References the SDK adds itself and build-only ones (`PrivateAssets="all"`, such as Nerdbank.GitVersioning) aren't dependencies of the package and aren't checked. Adding a package now means changing its project's list, which a reviewer sees. A runtime library referenced with `PrivateAssets="all"` would pass the check (and break the package at runtime); code review still catches that. diff --git a/src/.editorconfig b/src/.editorconfig new file mode 100644 index 0000000..cd4d181 --- /dev/null +++ b/src/.editorconfig @@ -0,0 +1,7 @@ +# Library code only (src/): rules that would be noise in tests. CI turns these warnings into errors +# (TreatWarningsAsErrors in the root Directory.Build.props). + +[*.cs] + +dotnet_diagnostic.CA1031.severity = warning # no catch (Exception) or bare catch (ADR-0002) +dotnet_diagnostic.CA2007.severity = warning # ConfigureAwait(false) on every await (CLAUDE.md) diff --git a/src/Directory.Build.targets b/src/Directory.Build.targets index 9617d3d..9926eec 100644 --- a/src/Directory.Build.targets +++ b/src/Directory.Build.targets @@ -10,4 +10,23 @@ + + + System.Text.Json;System.Net.Http.Json;Microsoft.Bcl.TimeProvider;Microsoft.Bcl.HashCode + + + + + <_OpenUrzednikAllowedPackage Include="$(OpenUrzednikAllowedPackageReferences)" /> + <_OpenUrzednikPackageDependency Include="@(PackageReference)" + Condition="'%(PackageReference.IsImplicitlyDefined)' != 'true' and '%(PackageReference.PrivateAssets)' != 'all'" /> + <_OpenUrzednikDisallowedPackage Include="@(_OpenUrzednikPackageDependency)" Exclude="@(_OpenUrzednikAllowedPackage)" /> + + + + diff --git a/src/OpenUrzednik.Diagnostics/OpenUrzednik.Diagnostics.csproj b/src/OpenUrzednik.Diagnostics/OpenUrzednik.Diagnostics.csproj index 6e270c1..2b0d1f2 100644 --- a/src/OpenUrzednik.Diagnostics/OpenUrzednik.Diagnostics.csproj +++ b/src/OpenUrzednik.Diagnostics/OpenUrzednik.Diagnostics.csproj @@ -5,6 +5,11 @@ Tracing;ActivitySource;OpenTelemetry;API;Poland;Gov + + + System.Diagnostics.DiagnosticSource + + diff --git a/src/OpenUrzednik.Extensions.Logging/OpenUrzednik.Extensions.Logging.csproj b/src/OpenUrzednik.Extensions.Logging/OpenUrzednik.Extensions.Logging.csproj index 6deec14..3cf71e2 100644 --- a/src/OpenUrzednik.Extensions.Logging/OpenUrzednik.Extensions.Logging.csproj +++ b/src/OpenUrzednik.Extensions.Logging/OpenUrzednik.Extensions.Logging.csproj @@ -3,6 +3,8 @@ Unofficial OpenUrzednik.NET adapter for Microsoft.Extensions.Logging: sends the clients' structured logs to ILogger. Logging;ILogger;API;Poland;Gov + + Microsoft.Extensions.Logging.Abstractions diff --git a/src/OpenUrzednik.Nbp.DependencyInjection/OpenUrzednik.Nbp.DependencyInjection.csproj b/src/OpenUrzednik.Nbp.DependencyInjection/OpenUrzednik.Nbp.DependencyInjection.csproj index 03cdadd..dfca804 100644 --- a/src/OpenUrzednik.Nbp.DependencyInjection/OpenUrzednik.Nbp.DependencyInjection.csproj +++ b/src/OpenUrzednik.Nbp.DependencyInjection/OpenUrzednik.Nbp.DependencyInjection.csproj @@ -4,6 +4,8 @@ Unofficial Microsoft.Extensions.DependencyInjection registration for the OpenUrzednik.NET NBP clients: typed HttpClients, validated options, logging and tracing; ready for AddStandardResilienceHandler() on .NET 8 and later. NBP;DependencyInjection;HttpClientFactory;API;Poland;Gov icon-nbp.png + + Microsoft.Extensions.Http;Microsoft.Extensions.Options;Polly.Core