docs(decisions): Phase 02a packet 1 — foundation decisions (ADR 0023/0024/0028) - #5
Merged
Merged
Conversation
…0024/0028)
Moves the three Phase-02a-blocking drafts from Open ADR Drafts to Accepted:
- ADR-0023 — Strongly-Typed ID Source Generator → Vogen. Chosen for
broader value-object support (Email / Slug / LocaleCode use the same
[ValueObject<Guid>] annotation as IDs), pre-baked EF + JSON + ASP.NET
binder + OpenAPI schema-filter emitters, and active maintenance
velocity. PrivateAssets="all" reference in SharedKernel; no runtime
bleed into module assemblies. UUIDv7 minting split DB-side
(gen_uuid_v7 per ADR-0031) for high-volume tables, app-side
(Guid.CreateVersion7) for aggregates needing the id before flush.
- ADR-0024 — API Versioning Policy. Codifies what Standards 04 already
half-said: URL-based /v{N}/, no minor versions in the URL, 6-month
deprecation window (vs Stripe's 12, Twilio's 6 — we sit with Twilio).
Every deprecated endpoint carries RFC 8594 Sunset + Deprecation +
Link rel=successor-version headers plus OpenAPI deprecated + x-sunset
+ x-successor + x-migration-guide extensions for SDK codegen.
410 Gone with RFC 7807 Problem Details at sunset. /healthz exempted.
Hub /api/internal/* surface explicitly out of scope (governed by
ADR-0019).
- ADR-0028 — audit_log Partition Management → Hangfire recurring job
(learnstack:audit:partition-management). pg_partman rejected for
air-gapped-deployment story (no PG extension to ship) + single-
source-of-truth posture (retention rules already in Standards 18, not
duplicated in partman config). Daily 02:15 UTC; create-ahead 2
months; drop only at platform-max (10y) horizon; row-level retention
purge separate (separate Hangfire job operating inside still-attached
partitions).
Cross-link updates:
- docs/standards/02-backend-coding.md § Strongly-Typed Identifiers cites
ADR-0023 with the Vogen [ValueObject<Guid>(...)] declaration shape.
- docs/standards/04-api-design.md § Versioning cites ADR-0024 with the
6-month + RFC 8594 + OpenAPI extension set.
- docs/decisions/0002-initial-architecture.md, 0016-audit-log-subsystem.md,
0031-postgresql-major-version.md: stale "ADR-XXXX draft" references
updated to point at the now-Accepted files.
- docs/decisions/README.md: 3 rows added to Active ADRs, removed from
Open ADR Drafts.
- docs/roadmap/phase-02a-kernel-tenancy.md: Packet 1 marked ✅; the ADR
commitments table at the bottom updated to "Accepted" state.
- README.md: Status line names the three new ADRs and points at Packet 2
(Shared Kernel core) as next.
No code in this packet; ADR-0023's compile-time Vogen wiring + the
Entity<TId> base + Result<T> already-in-SharedKernel work belongs in
Packet 2. Packet 0 was the kickoff; Packet 1 unblocks Packets 2+.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughPhase 02a Packet 1 (Foundation decisions) ships with three newly documented architecture decisions: Vogen for strongly-typed IDs, URL-based API versioning with 6-month deprecation windows, and Hangfire-managed audit log partitioning. Cross-references in existing ADRs, standards documentation, and roadmap tracking are updated to reflect these acceptances. ChangesPhase 02a Packet 1 Architecture Decisions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request formalizes several architectural decisions for Phase 02a, including the adoption of Vogen for strongly-typed IDs and value objects (ADR-0023), the establishment of a URL-based API versioning policy with a six-month deprecation window (ADR-0024), and the selection of Hangfire recurring jobs for audit log partition management (ADR-0028). Corresponding updates have been made to the project roadmap, coding standards, and existing ADRs to reflect these decisions. Feedback was provided regarding the format of the Deprecation header to ensure alignment with IETF drafts and to correct a typo in the RFC reference for the Sunset header.
Comment on lines
+100
to
+101
| - `Deprecation: @<unix-timestamp>` HTTP response header on every call | ||
| (per `draft-ietf-httpapi-deprecation-header-06`). |
There was a problem hiding this comment.
The Deprecation header format @<unix-timestamp> does not align with the cited draft-ietf-httpapi-deprecation-header-06. That draft specifies a boolean value (Deprecation: true), while later drafts specify an HTTP-date. Using a Unix timestamp appears to be a custom implementation that deviates from the standard you're aiming to follow.
To better adhere to web standards, consider aligning with one of the draft versions. For example:
- Use a boolean value and a separate
Linkheader withrel="deprecation"as per older drafts. - Use an HTTP-date as per the latest draft.
Collaborator
Author
There was a problem hiding this comment.
Applied in 9df097d. Verified against the upstream spec: RFC 9745 (March 2025) superseded the draft and uses sf-date per RFC 9651 § 3.3.7, format @<unix-timestamp>. The ADR now cites RFC 9745 + RFC 9651 directly in the Lifecycle section and the References block.
Comment on lines
+102
to
+103
| - `Sunset: <RFC 9651 HTTP-date>` HTTP response header pointing 6 months | ||
| into the future (per RFC 8594). |
There was a problem hiding this comment.
There seems to be a typo in the RFC number. RFC 9651 does not exist. The Sunset header is defined in RFC 8594, and its value should be an HTTP-date as defined in RFC 7231. It would be clearer to use <HTTP-date> as a placeholder.
Suggested change
| - `Sunset: <RFC 9651 HTTP-date>` HTTP response header pointing 6 months | |
| into the future (per RFC 8594). | |
| - `Sunset: <HTTP-date>` HTTP response header pointing 6 months | |
| into the future (per RFC 8594). |
Collaborator
Author
There was a problem hiding this comment.
Applied in 9df097d. RFC 9651 IS a real RFC (Structured Field Values for HTTP, used by the Deprecation header) but was the wrong citation for Sunset. Replaced with HTTP-date per RFC 9110 § 5.6.7, which is what RFC 8594 § 3 actually requires (example: Sun, 15 Jan 2027 00:00:00 GMT).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/decisions/0016-audit-log-subsystem.md`:
- Around line 295-296: The paragraph in 0016-audit-log-subsystem.md conflicts
with ADR-0028 by implying tenant retention purges drop partitions; update the
text to explicitly follow ADR-0028: state that partition management (including
monthly partition drops via the Hangfire recurring job) is handled at the
platform horizon per ADR-0028, while tenant-specific retention is enforced via
row-level purge and does not drop partitions, and remove or reword any sentence
that says retention purge drops partitions by tenant windows to avoid the
contradiction.
In `@docs/decisions/0023-strongly-typed-id-source-generator.md`:
- Around line 190-192: The inline code span for the PackageVersion XML element
is malformed (extra backtick) in the sentence mentioning
Directory.Packages.props; fix the markdown so the full element is a single
inline code span (e.g. `<PackageVersion Include="Vogen" Version="..." />`) and
ensure the following `<PackageReference Include="Vogen" PrivateAssets="all" />`
remains properly formatted; update the text around Directory.Packages.props and
LearnStack.SharedKernel.csproj to use that single correct inline code span.
In `@docs/standards/02-backend-coding.md`:
- Around line 52-63: Update the standards file header to include ADR-0023 in the
"Derives from:" metadata so the backend coding standard traces its source to
ADR-0023 (which defines the shared Vogen-based value object convention); locate
the "Derives from" header in docs/standards/02-backend-coding.md and add
"ADR-0023" alongside the existing ADR-0002/0006 entries.
In `@docs/standards/04-api-design.md`:
- Around line 34-56: The file's metadata header is missing ADR-0024 as a
provenance source; update the document header that lists "Derives from" (the
top-of-file metadata block) to include "ADR-0024" (or the full ADR identifier
"ADR-0024: API versioning policy") so the Versioning standard explicitly cites
ADR-0024 as its source; ensure the header format matches other standards files
(the existing "Derives from" field) and add the ADR identifier only to that
header without changing the body text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57cca3fa-6606-4575-8b1d-0a073327819b
📒 Files selected for processing (11)
README.mddocs/decisions/0002-initial-architecture.mddocs/decisions/0016-audit-log-subsystem.mddocs/decisions/0023-strongly-typed-id-source-generator.mddocs/decisions/0024-api-versioning-policy.mddocs/decisions/0028-audit-log-partition-management.mddocs/decisions/0031-postgresql-major-version.mddocs/decisions/README.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/02-backend-coding.mddocs/standards/04-api-design.md
…ix + 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>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 02a Packet 1 ships the three Phase-02a-blocking ADRs as Accepted, unblocking every code-bearing packet that follows.
Email/Slug/LocaleCodeuse the same[ValueObject<Guid>]annotation as IDs), pre-baked EF + JSON + ASP.NET binder + OpenAPI emitters, active maintenance. UUIDv7 minting split: DB-sidegen_uuid_v7()for high-volume tables (per ADR-0031), app-sideGuid.CreateVersion7()for aggregates needing the ID before flush./v{N}/, 6-month deprecation window, RFC 8594Sunset+Deprecation+Link rel=successor-versionheaders, OpenAPIdeprecated: true+x-sunset/x-successor/x-migration-guideextensions for SDK codegen, 410 Gone with RFC 7807 Problem Details at sunset./healthzexempted. Hub/api/internal/*out of scope (ADR-0019 governs).audit_logPartition Management → Hangfire recurring job (learnstack:audit:partition-management).pg_partmanrejected for SelfHostedAirGapped story + single-source-of-truth posture (retention rules already live in Standards 18). Daily 02:15 UTC; create-ahead 2 months; drop only at platform-max (10y) horizon; row-level retention purge separate.Docs-only. No code. Compile-time Vogen wiring +
Entity<TId>base + remaining SharedKernel primitives ship in Packet 2.Cross-link updates
IStronglyTypedId<TKey>marker interface stays.Test plan
docs/analysis/residual scan on changed files: 0 hits.IConnectionMultiplexer/KafkaProducer/VaultClientdirect injection mentioned.Subject_Constraint):Aggregate_Roots_Use_StronglyTypedId,Every_Deprecated_Endpoint_Has_Sunset_And_Successor,Every_Endpoint_Is_Under_Versioned_Route,Partition_Manager_Job_Is_Registered_At_Startup. Each carries a "lands in Packet N" note; catalogue entries follow when the tests do (per Packet 10's plan).dotnet build— N/A (docs-only).Notes for the reviewer
security-eventrows.🤖 Generated with Claude Code
Summary by CodeRabbit