Skip to content

Commit 9df097d

Browse files
cemililikclaude
andcommitted
fix(decisions): PR #5 review — Vogen API + RFC citations + route prefix + partition shape
Addresses two review-agent passes on PR #5. Each finding was verified against current code / upstream docs / corpus before fixing; the trivial-prose Minors (steel-man overlap, in-place edits to Accepted ADRs' Drivers vs dated Amendments) were left alone with explicit reasoning. Blockers / Majors fixed: - ADR-0023 Vogen API claims. Verified against https://github.com/SteveDunn/Vogen/blob/main/src/Vogen.SharedTypes/Conversions.cs: `Conversions.AspNetCoreRouteParameter` and `Conversions.SwaggerSchemaFilter` are fabricated names. Real members used: `EfCoreValueConverter`, `SystemTextJson`, `TypeConverter` (TypeConverter is what carries ASP.NET Core route binding). OpenAPI schema mapping is wired separately via either assembly-level `[VogenDefaults(openApiSchemaCustomizations: ...)]` or a custom `IOpenApiSchemaTransformer` in `LearnStack.Api` (Packet 4 picks one). EF registration is explicit, not convention-scanned — `LearnStack.SharedKernel.Infrastructure.ModelConfigurationBuilder.RegisterVogenIds(Assembly[])` helper reflects Domain assemblies and calls `.HaveConversion<...>()`. Acknowledged `Vogen.SharedTypes.dll` runtime dependency (~10KB, MIT). PrivateAssets="all" must be per-project: SharedKernel for cross-cutting VOs + each `LearnStack.Modules.<X>.Domain` for aggregate IDs; centralised via `Directory.Build.props` under `backend/src/Modules/`. - ADR-0024 route prefix `/v1/*` → `/api/v{N}/*` end-to-end. Verified against `infra/apisix/apisix.yaml` (routes `/api/v*/**`) and `docs/architecture/30-api-gateway.md` (uses `/api/v*/**`). Fixed Decision, Lifecycle, Per-deployment-mode, OpenAPI examples, Punted-on, Drivers (the "first endpoint already ships" claim was factually wrong — the scaffold exposes `/healthz` unversioned, not `/v1/healthz`). - ADR-0024 Deprecation header citation. Verified RFC 9745 (March 2025, "The Deprecation HTTP Response Header Field") supersedes `draft-ietf-httpapi-deprecation-header`. Replaced all three citation sites (Drivers, Lifecycle, References). Format `@<unix-timestamp>` is correct per RFC 9745 § 2 (sf-date per RFC 9651 § 3.3.7). - ADR-0024 Sunset HTTP-date citation. RFC 9651 is Structured Field Values (sf-date, used by Deprecation), NOT HTTP-date. Sunset uses HTTP-date per RFC 9110 § 5.6.7 (was RFC 7231 § 7.1.1.1). Fixed example placeholder + References block. - ADR-0024 Problem Details example host. Was `learnstack.dev/problems/...`; Standards 04 § Errors uses `errors.learnstack.dev/...`. Aligned to the Standards 04 convention. - ADR-0024 closed-set enum rule. Was only in prose below the matrix; promoted to a matrix row (Breaking: "Adding a value to a closed-set enum (default)"). Open-set rule stays on the non-breaking side with the `x-extensible-enum: true` hint. - ADR-0028 partition shape. Verified ADR-0016 + architecture/31 use `PARTITION BY RANGE (timestamp)`. My LIST claim was inconsistent; fixed to RANGE(timestamp) and noted this ADR owns *lifecycle*, not table layout (which stays under ADR-0016). Column-name occurrences `occurred_at` → `timestamp` to match the authoritative ADR. (Standards 18 still uses `occurred_at` — a pre-existing corpus inconsistency, out of this PR's scope.) - ADR-0028 anchor "§ Background Jobs". Verified architecture/31 has no such heading; the cited content lives under "§ 8 Retention" (lines 432-438). Fixed both occurrences (Drivers + References). - ADR-0028 architecture-test name `Partition_Manager_Job_Is_Registered_At_Startup` → `Partition_Manager_Job_Is_Registered_AtStartup` to match the catalogue's existing `IExceptionHandler_Registered_AtStartup` single-token convention. - ADR-0016 retention-purge text contradicted ADR-0028 ("dropping partitions older than the tenant's configured retention window"). Reworded to follow ADR-0028's split: partition lifecycle = platform-max horizon via `learnstack:audit:partition-management`; per-tenant retention = row-level delete via `learnstack:audit:retention-purge` inside still-attached partitions. Cross-link / standards-hygiene fixes: - Standards 02 `**Derives from:**` adds ADR-0023 alongside ADR-0002 / ADR-0006. - Standards 04 `**Derives from:**` adds ADR-0024 alongside ADR-0002 / ADR-0003. - Standards 04 § Versioning shrunk from a duplicate of ADR-0024 to a one-paragraph summary + "for the full policy, see ADR-0024" pointer (single-source-of-truth per Standards 13). - Standards 04 URL examples (7 occurrences) updated from `/v1/...` to `/api/v1/...` to match the canonical APISIX route shape. The `/openapi/v1.json` spec download URL is intentionally left as-is (that's a spec download surface, not an API endpoint). Skipped with reasoning: - "Why Hangfire over pg_partman + steel-man overlap" (ADR-0028 § Context) — reviewer marked as defensible-as-is; the steel-man section earns its keep for a 2027 reader who might wonder if we should have chosen pg_partman. - "In-place edits to Accepted ADRs 0002 / 0016 / 0031 push slightly past typo grade" — reviewer marked as "Acceptable for this PR"; switching to dated Amendments retroactively would itself be an Accepted-ADR edit. The previous packet's commit body already noted the pattern. - ADR-0023 inline code span (malformed extra backtick) — already collateral-fixed in the Implementation Notes § Package reference rewrite in this same commit; the malformed `<PackageVersion Include="Vogen" Version="..."` /> span is now a single clean inline span. Validation: - Broken-link sweep over 6 changed files: 0 broken. - docs/analysis/ residual scan: 0 hits. - Conversions enum members in ADR-0023 verified against upstream source. - Standards 04 has no remaining bare `/v1/` API-surface examples. - ADR-0028 has no remaining LIST / At_Startup / Background Jobs references. - ADR-0024 cites RFC 9745 + RFC 8594 + RFC 9651 + RFC 9110 + RFC 7807 with correct role per header. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent da1d37e commit 9df097d

6 files changed

Lines changed: 179 additions & 135 deletions

‎docs/decisions/0016-audit-log-subsystem.md‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -315,8 +315,14 @@ Three blocker-level architecture tests are added in Phase 02:
315315
`Value` field so JSON is readable.
316316
- The `Changes` JSON shape: array of `{ entityType, entityId, field, old, new }` for
317317
multi-entity commands; flat object `{ field, old, new }` for single-entity commands.
318-
- Retention purge: `LearnStackJob` running monthly, dropping partitions older than the
319-
tenant's configured retention window (default 7y / 2y by operation class).
318+
- Retention follows the split defined in
319+
[ADR-0028](0028-audit-log-partition-management.md):
320+
partition lifecycle (creating monthly partitions, dropping them only at the
321+
platform-max horizon) is owned by the `learnstack:audit:partition-management`
322+
Hangfire job; per-tenant retention enforcement (default 7y / 2y by operation
323+
class) is a separate row-level delete job
324+
(`learnstack:audit:retention-purge`) operating *inside* the still-attached
325+
partitions — never by partition drop.
320326
- PII redaction handler: subscribes to `UserGdprDeletedIntegrationEvent`, runs a
321327
parameterised UPDATE on audit rows containing the matching user reference, replaces PII
322328
fields with `[REDACTED]` placeholder constant.

‎docs/decisions/0023-strongly-typed-id-source-generator.md‎

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -63,21 +63,33 @@ value objects.
6363

6464
- Every aggregate's ID type is declared as a partial `record struct`
6565
annotated with `[ValueObject<Guid>(conversions: Conversions.EfCoreValueConverter |
66-
Conversions.SystemTextJson | Conversions.AspNetCoreRouteParameter |
67-
Conversions.TypeConverter | Conversions.SwaggerSchemaFilter)]`.
68-
- Vogen lives in `LearnStack.SharedKernel` as a `PrivateAssets="all"` build-time
69-
package reference — the generator emits compile-time code; no runtime dependency
70-
on the Vogen assembly bleeds into module assemblies.
71-
- Value objects with invariants (`Email`, `Slug`, `LocaleCode`, `Money`, …) follow
72-
the same annotation pattern, with a `Validate` static method enforcing the
73-
invariant.
66+
Conversions.SystemTextJson | Conversions.TypeConverter)]`. The
67+
`TypeConverter` flag is what makes ASP.NET Core route-parameter binding
68+
work — Vogen does not ship a separate `AspNetCoreRouteParameter` flag.
69+
OpenAPI schema customisation is wired separately (see Implementation Notes
70+
§ OpenAPI).
71+
- The Vogen **build-time** generator (`Vogen` package) is referenced via
72+
`PrivateAssets="all"` on **each project that hosts `[ValueObject<...>]`
73+
declarations** — `LearnStack.SharedKernel` for cross-cutting value objects
74+
(`Email`, `Slug`, `LocaleCode`, `Money`), and **each
75+
`LearnStack.Modules.<X>.Domain`** for the module's aggregate-root IDs. The
76+
reference is centralised via `Directory.Build.props` so adding a new module
77+
picks the generator up automatically. The **runtime** assembly
78+
`Vogen.SharedTypes` (which carries the `Conversions` enum and a handful of
79+
helper types the generated code calls into) flows transitively to
80+
consumers — this is a small (~10 KB) MIT dependency, not a heavyweight
81+
runtime.
82+
- Value objects with invariants (`Email`, `Slug`, `LocaleCode`, `Money`, …)
83+
follow the same annotation pattern, with a `Validate` static method
84+
enforcing the invariant.
7485
- The `IStronglyTypedId<TKey>` marker interface ([Standards 02
75-
§ Strongly-Typed Identifiers](../standards/02-backend-coding.md)) is implemented
76-
by every Vogen-emitted ID struct. The interface stays; Vogen is just the body.
86+
§ Strongly-Typed Identifiers](../standards/02-backend-coding.md)) is
87+
implemented by every Vogen-emitted ID struct. The interface stays; Vogen
88+
is just the body.
7789

7890
The choice covers the four-artefact emission requirement, the value-object case, the
7991
PostgreSQL 18 DB-side UUIDv7 path (Vogen can wrap any `Guid`, including those minted
80-
by `gen_uuid_v7()`), and the `[Description]`/`[ReadOnly]` annotation surface Roslyn
92+
by `gen_uuid_v7()`), and a `[Description]`/`[ReadOnly]` annotation surface Roslyn
8193
analyzers can read for additional compile-time rules.
8294

8395
## Context
@@ -181,27 +193,39 @@ Three things outweighed the appeal:
181193

182194
- The `IStronglyTypedId<TKey>` interface in `LearnStack.SharedKernel` stays as a
183195
type-system contract; Vogen-generated structs implement it.
184-
- The `LearnStack.SharedKernel` project's `PrivateAssets="all"` Vogen reference
185-
means consumer modules don't transitively reference the Vogen assembly — they only
186-
consume the emitted code.
196+
- Consumer projects gain a small (~10 KB) **runtime** dependency on
197+
`Vogen.SharedTypes` (which carries the `Conversions` enum and a handful of
198+
helper types referenced by generated code). The Vogen generator package
199+
itself (`Vogen`) stays build-time-only via `PrivateAssets="all"`.
187200

188201
## Implementation Notes
189202

190-
- **Package reference (single place):** `Directory.Packages.props` adds
191-
`<PackageVersion Include="Vogen" Version="..."` />`. `LearnStack.SharedKernel.csproj`
192-
consumes it with `<PackageReference Include="Vogen" PrivateAssets="all" />`. No
193-
other project references Vogen directly.
203+
- **Package references:** `Directory.Packages.props` pins `<PackageVersion Include="Vogen" Version="..." />`. **Every project that hosts `[ValueObject<>]` declarations** — `LearnStack.SharedKernel` (for cross-cutting value objects: `Email`, `Slug`, `LocaleCode`, `Money`) and **each `LearnStack.Modules.<X>.Domain`** (for its aggregate-root IDs) — adds `<PackageReference Include="Vogen" PrivateAssets="all" />`. A `Directory.Build.props` rule under `backend/src/Modules/` keeps the per-module addition automatic when a new module is scaffolded. Source generators only run on projects that reference the generator package; transitive references do **not** carry the generator (this is a `PrivateAssets="all"` semantics constraint, not a Vogen quirk).
194204
- **Naming convention (per Standards 02):** ID type names end in `Id`
195205
(`TenantId`, `OrganizationId`, `CourseId`, …); value object types are named
196206
for the concept (`Email`, not `EmailValueObject`).
197207
- **Default conversions enum:** every ID + value object opts into the same
198-
`Conversions` mask (`EfCoreValueConverter | SystemTextJson |
199-
AspNetCoreRouteParameter | TypeConverter | SwaggerSchemaFilter`). A
200-
`LearnStack.SharedKernel.VogenDefaults` const captures the mask so the
208+
`Conversions` mask: `EfCoreValueConverter | SystemTextJson | TypeConverter`. The
209+
`TypeConverter` member carries ASP.NET Core minimal-API + MVC route-parameter
210+
binding (Vogen does not expose a separate `AspNetCoreRouteParameter` flag).
211+
A `LearnStack.SharedKernel.VogenDefaults` const captures the mask so the
201212
annotation reads `[ValueObject<Guid>(LearnStackVogenDefaults.IdMask)]`.
202-
- **EF Core registration:** `ModelBuilder.UseValueConverterForType<TId>()` is not
203-
needed — Vogen's `EfCoreValueConverter` conversion is picked up by the EF Core
204-
convention scanner when the entity property type is a Vogen-emitted struct.
213+
- **OpenAPI schema mapping:** Vogen does **not** ship a `Conversions.SwaggerSchemaFilter`-style
214+
flag. Schema customisation is wired one of two ways: (a) an assembly-level
215+
`[VogenDefaults(openApiSchemaCustomizations: ...)]` attribute in
216+
`LearnStack.SharedKernel` so every emitted type advertises its primitive
217+
shape, or (b) a custom `IOpenApiSchemaTransformer` in `LearnStack.Api`
218+
(Microsoft.AspNetCore.OpenApi) that detects Vogen-generated wrappers via the
219+
generated `IVogenValueObject<T>`-marker interface and emits the underlying
220+
primitive (`format: uuid`, `format: int64`, …). Packet 4 picks one when API
221+
conventions wiring lands; both paths are documented in Vogen's upstream docs.
222+
- **EF Core registration:** the `EfCoreValueConverter` Vogen flag **generates**
223+
the converter type per ID; it does not auto-register it. Each module's
224+
`DbContext.OnConfiguring` (or a shared `IModelCustomizer`) calls
225+
`configurationBuilder.Properties<TId>().HaveConversion<TId.EfCoreValueConverter>()`
226+
for each Vogen-emitted ID. A `LearnStack.SharedKernel.Infrastructure`
227+
helper `ModelConfigurationBuilder.RegisterVogenIds(Assembly[])` reflects
228+
over the Domain assemblies and applies the registration in one call.
205229
- **Architecture test (lands in Phase 02a Packet 2):**
206230
`Aggregate_Roots_Use_StronglyTypedId` — every type implementing
207231
`IAggregateRoot<TId>` has `TId : IStronglyTypedId<Guid>`, and every such

0 commit comments

Comments
 (0)