docs: restructure the roadmap around the one-way-door test, and close 151 audit findings - #9
Merged
Merged
Conversation
A four-report audit of the corpus surfaced three mechanisms that do not work as written, all inside the two subsystems the project calls non-negotiable, plus a sequencing problem: the control plane was being built before the product it controls. Correctness, moved earlier: - ADR-0003 Amendment 3 corrects the RLS policy template. It issued two permissive policies, which PostgreSQL combines with OR, so every tenant-wide row (organization_id IS NULL) was visible to every tenant. The corrected template is one AND-ed policy with FORCE ROW LEVEL SECURITY, an explicit WITH CHECK, and a four-role model. It now lives in exactly one place -- Standards 05 -- instead of four divergent copies. Isolation tests run as learnstack_app, because a test that connects as the table owner passes against inert policies. - ADR-0033 supersedes ADR-0016. MUST-class audit is written as a durable intent inside the business transaction and fails closed; SHOULD/MAY stays best-effort. This resolves the contradiction between Standards 18 and ADR-0016, and prevents the corrected RLS policy from silently rejecting every audit insert. Also corrects the audit_log DDL, which declared a primary key twice. - ADR-0034 replaces the "closed at four endpoints" rule with two enforceable invariants. The count was never true, and protecting it drove TLS private keys into the cached entitlement payload. Host resolution never calls the Hub. Additive infrastructure, moved later: - ADR-0035 codifies the one-way-door test. Dapr, Kafka, APISIX, Vault, the Hub integration, signed licence keys, custom-domain TLS automation and audit_log partitioning each ship as a port plus a working default now, and a vendor adapter on a written trigger in Phase 11. - Standards 00 gains the test itself, plus a rule that an uncontracted deployment mode may not decide a technical question. Proof, moved earlier: - Phase 02d is new: a two-tenant walking skeleton that renders two different education sites from one binary. The second tenant moves out of Phase 10 into Phase 02a Packet 7, where two tenants already exist for isolation testing, so genericity is tested continuously rather than at the end. - Phase 02a packets 4-10 are re-scoped and a Packet 3b repairs what Packets 2 and 3 left behind. Packets 0-3 are shipped; their records are unchanged. Every phase now carries a Phase Exit Decision -- nine were missing. The Hub repository owns its own roadmap; phase-02c keeps only LearnStack's side of the boundary, and phase-09b and phase-12 become pointers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent pass over both repositories found no blockers and seven substantive gaps, all of them places where a rule the restructure reversed survived in a file the restructure did not own. - The mechanical review gate still enforced the old rules. `standards-check` required Dapr, APISIX and the entitlement projection in Phase 02a and asserted a four-endpoint Hub surface -- so the gate that `implement-task` and `code-review` dispatch would have rejected correct Packet 5 work. It now carries the one-way-door test and ADR-0034's two invariants. - Two Accepted ADRs scheduled demand-gated infrastructure in Phase 02a in their Implementation notes: ADR-0015's APISIX route table and ADR-0030's Dapr cache adapter. Both move to Phase 11 with their trigger; the decisions themselves are untouched. - `05-mvp-scope.md` -- required reading for what is in, out and deferred -- still placed the second tenant at MVP exit and named a coding bootcamp with `CodeChallenge` lesson items as the genericity proof. The second tenant has existed since Phase 02a Packet 7, and the bootcamp candidate was dropped precisely because running submitted code sits outside the ADR-0018 genericity boundary. Choosing it would have proven the opposite point. - The operator portal is `operator-portal`, not `learnstack-hub-web`. The Hub repository's own architecture test asserts the former. Renamed across 11 core files including the glossary, which is the terminology source of truth. `phase-01-repository-tooling.md` keeps the old name: the phase is frozen. - Hub still asserted the four-endpoint rule in its glossary, its designated contract pointer doc, three review skills, the PR template and `.env.example` -- one glossary per repository, stating opposite facts. - The Co-Authored-By trailer was fixed in LearnStack but not in Hub. - Hub's `tenant-lifecycle.md` deferred the hard-delete flow to "a later packet" without naming it. It is P02c-4. Also fixes two pre-existing broken relative links in `add-provider-adapter` and `update-glossary`, which CI could not catch because the link audit only walks files a pull request changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A reader who opens Phase 00, Phase 01, or Phase 02a's shipped-packet records now hits statements that were true when written and are no longer the plan. The records themselves stay untouched -- they are dated delivery history -- so each gains a dated block saying what superseded it and which packet owns the correction. Phase 01 gains a "What changed after this phase closed" block covering six stale claims (the operator portal's name; `make seed` seeding two tenants, which it never did; a green CI whose frontend job asserts nothing; the 14-service compose stack; and the phases the OpenAPI-diff, Lighthouse and integration-test CI placeholders now activate in), the five defects the phase shipped with and where Packet 3b fixes each, and the two conventions introduced afterwards -- the mandatory Phase Exit Decision section this document predates, and Phase 02d's insertion into the order. Phase 00's historical preamble gains two corrections: the second tenant no longer waits for Phase 10, and its own "don't hardcode code-challenge" risk now has an explicit answer -- running submitted code is not expressible as tenant data at all, it is a plan-gated platform feature. Phase 02a's restructure note now names, before the re-scoped packets, the two things in the frozen Packet 2 and Packet 3 records that are known wrong: the `TenantContextBehavior` TODO points at a connection interceptor, which cannot work for a transaction-local setting, and the Shared Kernel's `Unit` collision, missing `[MemberNotNullWhen]`, and boxing `Entity<TId>` equality. Both are Packet 3b's job. It also flags that ADR-0032's resilience registration example does not compile while the shipped code is correct, so nobody "fixes" working code to match a broken snippet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The annotation added to Phase 01 says the OpenAPI-diff and Lighthouse CI placeholders now activate in Phase 02d rather than in Phases 03 and 04. That was true as a consequence of the reordering but nothing owned it, so the pointer resolved to a phase with no matching deliverable. Phase 02d now claims both explicitly, with the reason each moves: its two read endpoints are the first real `/v1/*` surface, and its catalog and lesson pages are the first content-bearing public pages -- and the right ones to hold a performance budget against, since they are what a visitor loads. Also corrects Phase 02d's own attribution for the `--passWithNoTests` removal, which lands in Packet 3b rather than Packet 5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The restructure corrected the Row Level Security template in four documents and recorded the correction in ADR-0003 Amendment 3. It did not correct the copies inside the skills an agent runs when it adds a tenant-owned table, or the checklist the review gate walks. So the next agent dispatched to `add-tenant-owned-entity` would have written the superseded two-policy shape into a real migration, and `standards-check` would have passed it. That is the same failure the original audit found -- one template, several copies, drifting -- reproduced at the only copies that execute. - `add-tenant-owned-entity` and `add-ef-migration` now carry the canonical template: one policy with an AND-ed predicate, `ENABLE` **and** `FORCE ROW LEVEL SECURITY`, an explicit `WITH CHECK`, and `current_setting` called with its missing-OK argument. Each says which three lines to drop for a tenant-wide table, names Standards 05 as canonical, and states that the runtime connects as `learnstack_app` while migrations own the table. - `standards-check` stopped asking for the defect. It required "RLS policy on `app.tenant_id`" plus "additionally ... RLS policy on `app.organization_id`" -- literally the two-policy shape, which would pass a leaking migration and could flag a correct one. It now requires exactly one policy, FORCE, and WITH CHECK, and requires isolation tests to connect as `learnstack_app`. - `12-localization.md`'s `tenant_template_library` template was still the unforced single-policy form. It is org-scoped, so its organization term belongs inside the one policy; Phase 04 and Phase 08a copy this table. - ADR-0016's `audit_log` DDL keeps its historical shape -- superseded ADRs are not rewritten -- but now carries an inline pointer to the binding template and to the double-primary-key defect ADR-0033 corrects. Every live document and skill that creates a tenant-owned table now declares FORCE ROW LEVEL SECURITY. The only remaining unforced copy is ADR-0016's, and it is marked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An aggressive nine-lens review of the restructure found three faults in the template ADR-0003 Amendment 3 introduced. All three are engine-level, all three fail silently, and all three would have shipped in the first migration. **The tenant-scope hatch widened writes, not just reads.** The template's `app.scope = 'tenant'` term exists so cross-organization reporting can read across organizations. But `USING` also decides which rows an `UPDATE` may target, and for `DELETE` it is the only gate -- PostgreSQL has no `WITH CHECK` for `DELETE`. A tenant-scope session could therefore delete another organization's rows, or reassign them to itself. Fixed with two `AS RESTRICTIVE` policies, `FOR UPDATE` and `FOR DELETE`; restrictive policies combine with `AND` and cannot widen. **`current_setting(..., true)` does not return NULL on a pooled connection.** A dotted GUC becomes a session placeholder the first time it is assigned and its reset value is the empty string, not "undefined". So the exact case the template reasons about -- a pooled connection whose previous transaction set `app.tenant_id` and whose next one does not -- evaluates `''::uuid` and *raises* instead of filtering the row out. The named completion criterion `Unsetting_tenant_context_returns_zero_rows_through_RLS` could not have passed. Every read is now wrapped in `NULLIF(current_setting(...), '')`, which yields NULL, and a NULL policy result is false for both `USING` and `WITH CHECK`. **Referential integrity bypasses Row Level Security.** PostgreSQL evaluates FK checks as a security-restricted operation on behalf of the table owner, so a single-column `lessons.course_id -> courses.id` lets one tenant's row reference another tenant's row: the child's `WITH CHECK` passes because its own `tenant_id` is right, and the FK check passes because no policy runs. Every foreign key between tenant-owned tables is now composite on `tenant_id`, and parents carry `UNIQUE (tenant_id, id)` to be referenceable that way. Phase 02d creates exactly this relationship and now says so. Propagated to every mirror: Standards 05 (canonical), ADR-0003 Amendment 3, `12-localization.md`, and the two skills that write migrations. Also sweeps the `/v1/` vs `/api/v1/` route prefix. ADR-0024 exists precisely to fix that inconsistency, and the restructure reproduced it in the packet whose job is to establish API conventions -- including in Phase 02d, which contradicted itself two sections apart. Under the documented APISIX route (`/api/v*/**`) the bare form is not merely inconsistent, it is unroutable. Phase 02d's frontend test also promised a "public / authenticated route split" in a phase that has no authentication. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…veChanges
Wave 1 of the fix plan. Closes 14 findings, two of them blockers.
ADR-0033 said MUST-class audit rows are "enrolled in the same
`DbContext.SaveChanges` as the business write". Four documents then gave three
different answers to the question that phrasing raises — which component
actually writes the row — and every downstream contradiction followed from
that gap.
A first attempt at a fix ("behavior decides, interceptor completes") was
rejected during review because it loses audit on the ordinary path. The
interceptor marked its intent consumed inside `SavingChangesAsync`, but the
commit happens later, in `TransactionBehavior`. A handler that calls
`SaveChanges` and then returns `Result.Fail` — every business-rule rejection in
the system — rolls the audit row back while the consumed flag survives in the
request scope, so the standalone write is skipped and both the business change
and its audit record disappear. Consumed is not committed.
The settled model is **decide, write, reconcile**:
- `AuditLogBehavior` (step 3) classifies and, for MUST-class, parks an intent
in the scoped capture. It opens no transaction and touches no `DbContext`.
- `TransactionBehavior` (step 6) calls `IAuditStore.WritePendingAsync` on the
ambient transaction immediately before `COMMIT`, then reports whether the
commit actually succeeded.
- `AuditLogBehavior` writes the row standalone on the way out whenever the
transaction did not commit, or when there was no business write at all —
denied outcomes, read-sensitive reads, non-mutating security events.
Atomicity now comes from the transaction rather than from `SaveChanges`, which
dissolves three problems at once:
- The write carries an `AuditEntryDraft` record from SharedKernel, not the
`AuditEntry` aggregate, so nothing maps a module's Domain type into every
other module's `DbContext`. That would have required SharedKernel to
reference `Modules.Audit.Domain` — a circular project reference — and would
have broken `Modules_Do_Not_Write_AuditLog_Directly` in every module at once.
- Writing immediately before commit means every flush has already happened, so
the snapshots are complete even when a handler calls `SaveChanges` twice.
- Nothing ever updates a committed audit row, so the append-only rule holds.
Three further defects surfaced while settling it:
- `ClassifyAsync` runs at step 3, before any transaction exists, and reads
`audit_config`, which is RLS-protected. The policy could not match, so the
read returned zero rows rather than raising — "this tenant has no overrides"
was indistinguishable from "RLS filtered everything". Classification now
reads the in-process catalogue plus a cached projection whose loader opens
its own transaction and sets the GUC itself.
- `AuditEntry_Is_AppendOnly` forbade every `UPDATE` and `DELETE` on
`audit_log`, which killed not only GDPR redaction but the retention purge —
both shipped by design. The rule now names the two sanctioned paths and the
role each runs as.
- The redaction SQL was independently broken: `jsonb_set(before_state,
'{redacted}', 'true')` adds a flag and leaves the PII in place, and raises
on a scalar or array snapshot. Payload redaction was already delegated three
lines later, so the clauses were wrong and redundant.
Findings closed: audit-durable-intent-owner-unresolved,
audit-complete-violates-append-only, audit-must-class-without-a-business-write,
audit-cross-dbcontext-savechanges, audit-outcome-boolean-cannot-express-denied,
fail-closed-caller-experience-unspecified, cc-33-stale-audit-contract,
standards18-storage-asserts-old-contract, add-audit-coverage-skill-predates-adr0033,
arch31-behavior-sample-not-buildable, standards18-payload-contract-self-inconsistent,
audit-classification-not-decidable-when-it-is-decided,
audit-tests-named-but-unregistered, partitioning-is-not-additive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…grant matrix Waves 2 and 3 of the fix plan, committed together because both rewrite docs/standards/05-database.md and splitting the file's hunks would leave neither commit internally consistent. Wave 2 — course-lesson-slug-violates-adr0008 (blocker). Phase 02d put title, summary and slug on the Course and Lesson parents, which ADR-0008 names explicitly among the side-translation-table entities, and the architecture doc's own DDL declared a unique index whose columns are a proper superset of the translation table's primary key — an index that can reject no row the table would otherwise accept, so two courses in one tenant could both hold /en/courses/beginner. The satellite is now a tenant-owned table in its own right: real tenant_id, mirrored organization_id, its own ENABLE + FORCE and full policy set, composite FK on (tenant_id, entity_id). Slug uniqueness is UNIQUE (tenant_id, locale, slug), flat across organizations — organization_id is nullable, and a nullable column in a standard UNIQUE constrains none of the rows where it is null, while repairing that with NULLS NOT DISTINCT still leaves a tenant-wide row and an org-scoped row competing for one URL that a host resolving to (tenant_id, organization_id) serves from both tiers. The locale parameter on the two read endpoints is required, and slug lookup is exact: fallback resolves display fields, never a slug. Wave 3 — template-applied-to-tables-that-cannot-take-it (blocker). "Every one of these tables is created with the corrected template" cannot run: tenants has no tenant_id column, and platform_host_to_tenant is read in order to determine the tenant, so a tenant-keyed predicate returns zero rows and no tenant ever resolves. Three table classes are now enumerated — tenant-owned, tenant-owned self-keyed, platform-scoped — with the test for the third stated (read before the tenant is known; exactly one table qualifies, and platform_entitlement_cache does not despite its name). Row security is enabled and forced on all nine. "Writes reserved to the provisioning path" is replaced by a real role × table × privilege matrix, because a GRANT names a role and every handler in the API process runs as the same one. BYPASSRLS bypasses policies, not GRANTs, so grant scope is the only bound on the two bypass roles: no PUBLIC grant, no ALTER DEFAULT PRIVILEGES, and the outbox dispatcher holds SELECT plus a column-level UPDATE and nothing else. EnterPlatformAdminScope reaches learnstack_platform by a second credentialed connection, never SET ROLE — membership is a standing capability, a plain SET ROLE survives COMMIT on a transaction-pooled connection, and per-role statement_timeout does not follow a role switch. app.resolving_host joins the canonical session-variable set as the fourth and last member, set by the resolver alone inside its own short transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…umber for The harvest round. 81 of the verified findings carried unnumbered "cluster notes" — real defects observed while verifying something else and never promoted to a finding of their own. This closes them, plus the 8 findings I raised by hand that never entered verification. Every one was re-checked against the file on this branch and anchored on quoted text, because six numbered findings already cite line numbers that moved under 1e7114c. The blocker of the set is in the Hub repository and lands in the same wave: the CI link audit's sibling-repo skip pattern was lowercase-only, bash `=~` is case-sensitive, and eb85c2b/f105d07 had just recapitalised all 215 cross-repo links to `../LearnStack/...`. Every one of them now reached an existence check that cannot resolve outside the Actions checkout. It had to land before any later wave adds a cross-repo link. The executable skills carried the same class of defect the RLS template did — the copies nobody re-read: - add-architecture-test asserted `current_setting('app.tenant_id')`, which fails against every migration written to the canonical template and passes against the superseded one-argument form. It also checked ENABLE and never FORCE, so it was green against the inert-policy failure mode ADR-0003 Amendment 3 exists to catch. - add-ef-migration's data-backfill loop issued SET LOCAL outside a transaction (discarded with a warning), interpolated the tenant id into SQL (banned by 05-database.md § Forbidden), and restated the RLS predicate in the WHERE clause — a second copy of the policy that can drift from the first. - add-tenant-owned-entity and add-ef-migration both omitted the composite UNIQUE (tenant_id, id) and the composite foreign keys, and standards-check's list did not ask for them, for NULLIF, or for the two restrictive guards. Also: ADR-0018's two architecture-test spellings mapped to their canonical names in the catalogue rather than by editing an Accepted ADR; seven registered-but-uncatalogued tests written up; `is_success` replaced by the three-valued `outcome` in the domain model's AuditEntry row; ADR-0033's DDL corrected to the unpartitioned table Packet 9 actually ships, with the reason the composite key is still right; the demand-gated provider defaults corrected in the extension model and in infra/dapr, where the previous text had Phase 02a shipping Dapr adapters ADR-0035 moved to Phase 11; and Phase 02a's exit decision restated as something a reviewer can run rather than four adjectives. Two findings did not apply. 18-audit-coverage's append-only exemption was already written by wave 1 and written better — the harvest text allowed GDPR redaction but forbade DELETE outright, which would have made the retention purge unimplementable. Recorded, not applied. Registered in Packet 3b: eight shipped source comments still schedule the Dapr secret provider to "Packet 5". The seam is correct; only the phase pointer is stale, and it is declared work rather than a silent edit to Packet 3's shipped code. verify.sh gains a ninth check. A replacement in this wave dropped a closing ``` in ADR-0033 and all eight existing checks stayed green — the table check reads an unclosed fence as "still inside code" and simply stops inspecting the rest of the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ration columns, and settle one auth chain for both directions Wave 4 of the fix plan — the last two blockers. entitlement-cache-missing-columns. The only CREATE TABLE platform_entitlement_cache in the corpus declared eight columns and neither `grace_until` nor `generation`. Both are load-bearing elsewhere and both had no storage: the grace window is ADR-0034's headline entitlement change and is asserted by 26-hybrid-license-model's reference Evaluate(), by the glossary, and by Phase 02c's completion criteria; the generation comparison — accept a push only when received >= stored — is a Hub-side completion criterion in p02c-3 and p02c-7, and a comparison against a column that does not exist cannot be written. Phase 02a Packet 6 ships this table, so the wrong DDL would have become the migration. `generation` defaults to 1 so tenant provisioning needs no special case; `grace_until` stays nullable because the Hub sends null for a tenant not in grace. The wire-to-column mapping (`tier` -> `plan_code`, `expires_at` -> `valid_until`) is written down rather than left to be rediscovered, and the write path is corrected: RefreshAsync from the HTTP push, with the Dapr event as the invalidation signal it actually is. ls-to-hub-auth-chain-contradiction. 24-learnstack-hub.md stated two different authentication schemes for the LearnStack -> Hub direction twenty lines apart — "every one of these carries the same auth chain: mTLS ... JWT ... HMAC", then "API key per LearnStack instance, stored in Vault" under a heading reading "applies to every endpoint above". ADR- 0034 meanwhile changed that direction's auth while filing it under "what we explicitly did not change", and the Hub repository's P02c-2 was already specified against the strengthened chain. Resolved in favour of the chain, not the key: a bearer credential on a path that returns a tenant's whole entitlement set has no replay protection and no per-request integrity. ADR-0034 gains § One auth chain, both directions, which owns the change instead of denying it, and its "did not change" section now claims only what is true. ADR-0019's body is untouched; the supersession banner I added in this restructure is the thing corrected. The fix spec named three remaining API-key carriers. Sweeping found six: the three plus 04-technical-architecture, 30-api-gateway, 03-module- boundaries mermaid edges, the Vault path list in 29-dapr-integration, and the Helm `apiKeyRef` in 25-deployment-models. This is the third time in this fix plan that the finding under-counted its own carriers, so verify.sh check 7 now fails on a live Hub API-key claim the way it already does on the four-endpoint claim. The chain's cost is real and is now paid for: a SelfHostedOnline instance needs a client certificate, and nothing said who issues it. Hub P02c-6 gains it as a deliverable — issued with the .lic bundle, same license_id, same re-issue command, same revocation, so a revoked instance loses the transport before it loses the entitlement — and a completion criterion asserting the handshake refuses a missing, expired or revoked one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e the role model runnable Wave 5 — the eight remaining RLS and isolation majors. Session-variable placement was described four different ways after 11-security.md declared itself the single authority on it and listed two corrections. The corpus's own pipeline deep dive (33-cross-cutting- concerns) still had step 4 setting the GUCs through a DbConnection- Interceptor — the mechanism 11-security rules out by name three paragraphs above its own claim; ADR-0032's canonical eight-step diagram annotated step 4 the same way; Phase 02a's live scope list said "TenantContextBehavior (asserts resolved + sets RLS GUCs)"; and 09-tenant-isolation's sequence diagram had middleware issuing SET LOCAL before any transaction existed — a fourth placement, earlier than all three. SET LOCAL is transaction-local and step 4 runs before step 6 opens the transaction, so every one of those discards the value before the query it protects. All four now say what 11-security says, whose corrections table grew from two rows to six. ADR-0032's Decision section is untouched; the correction is item 3 of its existing dated Amendment 2. The Packet 3 code TODO is deliberately left alone — Packet 3b owns it, and rewriting it here would desynchronise the roadmap record from the code. The four-role model could not be provisioned. Since PostgreSQL 15 the public schema grants CREATE to nobody but its owner, so the shown CREATE ROLE learnstack_migration LOGIN NOBYPASSRLS cannot create a table, the first dotnet ef database update fails with "permission denied for schema public", and the shortest way out — make it superuser or database owner — reinstates precisely the ownership arrangement FORCE ROW LEVEL SECURITY exists to defeat. The provisioning block now grants CONNECT and USAGE, CREATE ON SCHEMA public, gives each role its own password, and drops the ALTER TABLE ... OWNER TO line, whose presence in a migration means the migration is running as the wrong role. No ALTER DEFAULT PRIVILEGES: the wave-3 grant matrix already decided a table nobody granted should fail loudly rather than silently inherit DML from a bypass role. Phase 02a Packet 6 gains the script and the split connection strings, and says plainly that until it lands local dev runs one superuser and the isolation layer is inert. Also: the checkout interceptor that could not see what it was checking — checkout precedes the transaction carrying SET LOCAL, so it throws universally, or under PgBouncer transaction pooling reads a previous tenant's leftover and passes falsely. Replaced by a DbCommandInterceptor reading an in-process marker TransactionBehavior stamps, so the check costs no round trip. outbox_messages.correlation_id becomes NOT NULL: ADR-0032 § 12 and Standards 10 both make it the full W3C traceparent and a registered test asserts non-null on every row, while only the canonical DDL — the one Packet 6's migration is written from — said nullable, on the strength of an APISIX behaviour ADR-0035 demand-gates to Phase 11. The remaining bare current_setting casts in 31-audit-subsystem now wrap in NULLIF like the template they claim to be built from; the two architecture-test descriptions in 09-tenant-isolation no longer say "at least one RLS policy", which licensed exactly the two-permissive-policy shape the standard forbids (identifiers left alone — their rename is Packet 10's, and the catalogue still registers the old spellings); ADR-0016's banner names the abolished audit_admin role its DDL comment still invokes; and the TenantTemplateLibrary Studio editor, which Phase 08a assigned to Phase 06 and Phase 06's ownership table assigned back to 08a while neither listed it as a deliverable, lands in 08a — the only phase where the aggregate it edits exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ocation list off the contract surface Wave 6 — Hub contract and entitlement terminology. The glossary rewrite made `Entitlement` mean one thing: a Hub-authored, tenant-subject projection of what a plan permits. The learner-subject aggregate it displaced was never renamed, so Phase 07 still owned "the `Entitlement` aggregate (id, tenant, **user**, scope, source)", the permission keys were `enrollment.entitlement.read|write`, and the domain model's Enrollment subgraph carried an `Entitlement` node twelve lines below the Hub's. Two aggregates in two modules with two subjects and one name — and `add-permission`'s worked example, the file an agent copies verbatim, minted the old key on every use. The learner aggregate becomes `CourseAccess`, the name the glossary already gives it. Applied as enumerated line edits across phase-07, phase-09, 19-permissions, 02-domain-model and the skill — never as a global replace: `entitlement` is correct in roughly sixty other places (`IEntitlementProvider`, `platform_entitlement_cache`, the projection, the feature-flag surface), and sweeping it would corrupt the corpus in the opposite direction. Stateful entitlement was owned three ways. Phase 09 split a credit-pack ledger across three phases — purchase here, balance in Phase 07, decrement in Phase 08b — while Phase 07 and Phase 10 both say no phase builds one, and Phase 08b never acknowledged the work assigned to it. Phase 07 wins: the ledger needs its own ADR and its own release. Phase 09 keeps the purchase, which is genuinely a `Product`. The exit gate went with it — it still required that "a credit pack's balance survives a purchase, a booking and a cancellation without disagreeing with itself", which would have gated the phase on a capability the same document now says nobody builds. `GET /api/v1/internal/license/revocations` was a fifth LearnStack -> Hub path, published in architecture 26 § 7 and embedded in the licence payload, absent from ADR-0034's enumerated set. It is now a signed static artefact at `/.well-known/learnstack/revocation-list.signed.json`, off `/api/` entirely — which is also the only form an air-gapped customer can consume, and is unauthenticated on purpose: the signature is the authentication and the contents are opaque licence ids, so a reader learns how many licences were revoked, not whose. The daily fetch is attributed to `SignedLicenseKeyEntitlementProvider` so no unnamed type holds a Hub reference. The air-gapped local file path is untouched — the two are different things and both survive. Also, found while reconciling: architecture 24's "authoritative list" of Hub-side architecture tests spelled `Hub_NeverStores_TenantContent` where the catalogue, ADR-0019, ADR-0034, Standards 20 and two roadmap phases all say `Hub_NeverStores_TenantData`, and it was missing `Hub_Client_Referenced_Only_By_Named_Adapters`, which ADR-0034 adds as the enforcement of its own second invariant. Both fixed, and the list now says why `LearnStack_Modules_DoNotReference_Hub` is not in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ling Dapr to a phase it left Wave 7 — ADR-0035 and its carriers. Two of ADR-0035's five decision drivers stated verifiable facts that were false against the branch they were written on. The package-declaration driver listed Serilog, Polly and Sentry among the libraries the backend cannot call — all three are declared and wired, by Packet 3, which is the packet this ADR's own gated set carves out. And "the ports already exist and already isolate the choice" named six ports in SharedKernel when two are there: ISecretProvider and IProviderResilience. The rest land in Packet 5. It then cited "ADR-0014 § 9" for a "three-class swap in one release" quotation; ADR-0014 has no § 9 and contains no such sentence. Both drivers are rewritten to what is true, and the second now argues the thing that actually carries the decision — a port is three files and a registration, an adapter is not, and that asymmetry is what makes the deferral reversible. ADR-0014 still scheduled all three Dapr adapters to "Phase 02 — Platform kernel". Its Decision is untouched and its vendor choice is not reversed; a dated Amendments section records that only the schedule moved, with the per-adapter trigger table, and states plainly that the amendment does not withdraw Dapr — the failure mode a future reader would otherwise reach. 29-dapr-integration.md was the one infrastructure architecture document the restructure never opened. It now carries the same "target design, not running" banner Standards 20 does, and its ICacheService reference implementation is annotated where it is unimplementable: RemoveByPrefixAsync enumerates an instance-local _trackedKeys dictionary, so keys written by another pod are never evicted — it under-invalidates the moment a second instance runs, which is the exact condition its own cross-instance invalidation section assumes. The generation-counter replacement is named at both sites. EnvironmentSecretProvider does not exist. The shipped class is ConfigurationSecretProvider and it reads IConfiguration, not process environment variables — so the name was wrong in 15 documents and the behaviour description in 29-dapr was wrong too. Swept everywhere except ADR-0014's body, where the new Amendment discharges it. The shipped class's own XML comment, which promised Packet 5 would swap it, now names ADR-0035 and Phase 11. Two ownership gaps closed. ITenantSearch had a demand-gated Meilisearch adapter and a "PostgreSQL full-text" default that no phase built, while Phase 09 asserted search had existed since Phase 04 — Phase 04 now ships the port and the tsvector default, with the tenant predicate composed inside the port and a cross-tenant isolation test from that phase. IVideoTranscoder was gated in Phase 11's prose with no row in ADR-0035's table at all; it has one now, and Phase 11 stops claiming every gated item has a Packet 5 default, because three of them have no port and this one's default is Phase 04's. Finally, host-mappings had two owners: 27-custom-domain-tls § 11 puts the handler in Phase 02c, Phase 11 claimed the endpoint. Phase 02c wins — host resolution is a one-way door, the automation that populates the mapping is additive — and Phase 11 keeps the edge half. The Hub's blocking table said P02c-5 waits on Phase 11 for the handler; it now waits on Phase 02c for the handler and explicitly does not wait on Phase 11 for the edge. Phase 11 also stopped accusing 25-deployment-models of presenting RLS as failure isolation. That document draws the correctness/contention split correctly and has since the restructure touched it; the paragraph now cites it rather than arguing with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…odules a phase Wave 8, part 1 — roadmap ownership. Eight of the sixteen modules the architecture declares have no project files and no phase that creates them. Phase 01 scaffolded seven; Phases 07, 08a, 08b, 08c and 09 all write handlers, aggregates and migrations into assemblies that do not exist, and never list the assembly itself as a deliverable. Each of those phases now opens its Deliverables with the scaffold bullet in the shape Phase 01 used — the four projects, the IModule registration, the module's audit.md, and its permission-catalogue rows — which is exactly what the add-backend-module skill emits, so the executor has a mechanical path rather than an inference. Phase 07's bullet also records that Enrollment covers progress, so nobody scaffolds a Progress assembly the architecture does not have. TenantScoringRule and TenantCompletionRule were storage nobody creates. Packet 8 fixed their body column as opaque text plus a dialect discriminator and listed "evaluation" as Phase 05's — so four documents described a column shape for tables no phase migrates. Split along the line that already existed: Packet 8 owns the storage decision, because the column type is the part that cannot change later without a migration; Phase 05 owns the aggregates, the tables and the runtime, arriving with their first consumer. Packet 8 stays at two aggregates, which is what owner decision 1 reduced it to. The customization key shape could not express a version. Invariant 6 of the customization model required UNIQUE (tenant_id, key) on every customization table, while § 4 of the same document shows vocabulary-card at schema_version 1 and 2 in tenant_content_types — the constraint would reject the second revision, and with it the first breaking change ADR-0013 requires. It is now the versioned pair: UNIQUE (tenant_id, key, schema_version) for the revision, and the partial index UNIQUE (tenant_id, key) WHERE status = 'active' for the live definition. Packet 8 carries it from the first migration, because version history cannot be retrofitted onto the single constraint. Phase 02d listed a TenantContentType column in its two-tenant comparison table with nothing in the phase producing it. The lesson body's field set is now driven by the tenant's own TenantContentType, so the two tenants' lesson pages differ in shape rather than only in copy — which is the proof the phase exists to produce. Deliberately no ContentEntry aggregate and no authoring surface: those are Phase 04, and the phase's own top risk is the slice growing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ory blockquote Wave 8, part 2 — a structural move of one file, no reworded sentences. phase-02a-kernel-tenancy.md opened with a 761-line blockquote. The first 360 lines were shipped history (Packets 0-3); the next 390 were the forward plan for the seven unshipped packets — quoted as though they too were a record, above `## Goal`, above `## Scope`. An agent opening the file to find out what Packet 6 builds read four hundred lines of what already happened first, and then found the same work described a second time in `## Scope`, because the plan was in two places and neither claimed authority. Three moves, all mechanical: - The status block becomes a ten-row packet table with a state column and a link per packet, and says which of the two sections to read for what. - Packets 0-3 and the 2026-08-08 restructure annotation move verbatim to `## Delivery Record (Packets 0-3)` at the end of the file. Verified byte-identical: 410 non-blank lines in, 410 out, no diff. They were written when they sat at the top, so where they say "above" and "below" they mean the original layout — the new section header says so rather than editing frozen text to match its new position. - Packets 3b-10 become `## Packet Sequence`, unquoted, immediately after `## Goal` and before `## Scope`, with a header stating the division of labour: Scope is the authority on a subsystem's shape, Packet Sequence on order and gating. The Packet 3b entry pointed at "their records above"; that pointer now names the section rather than a direction. verify.sh's frozen check had to change with it. It compared the first diff hunk's line number against a hard-coded 366 — a boundary this move erases. It now extracts the Packet 0-3 record block by content from both refs and requires them byte-identical, so the check follows the records wherever they sit and catches a reworded line the line-number version could not see. No corpus anchor pointed into this file (grep for `phase-02a-kernel-tenancy.md#` across both repositories and .claude returns nothing outside docs/analysis), so the move breaks no link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esolution path
Waves 9 and 10, LearnStack half — combined into one commit because six of
the eighteen findings were already closed by earlier waves and the
remainder share no file.
03-module-boundaries listed "direct HTTPS calls to Hub from anywhere
except IEntitlementProvider / IHostToTenantResolver" as the forbidden
column — naming the resolver as a *sanctioned* Hub caller. ADR-0034
deleted that call: the resolver reads platform_host_to_tenant and nothing
else, because putting the Hub on the hot path of an anonymous public page
load is the defect 27-custom-domain-tls was corrected for. The row now
names the three real adapters and says the resolver is not one of them.
Nothing wrote the row that makes a new tenant reachable. The Hub creates
a tenant, LearnStack inserts tenants, organizations and the entitlement
cache, and the Hub redirects the operator to {slug}.learnstack.app — a
host with no platform_host_to_tenant row, which Standards 20 turns into a
404 rather than a lookup. The tenant-create handler now seeds its own
platform subdomain in the same transaction. Deliberately not over the
host-mappings push: the platform subdomain needs no DNS verification and
rides the wildcard certificate, so routing it through the custom-domain
path would couple every tenant provision to P02c-5.
The custom-domain submission hop was a fifth caller — Admin Studio to
LearnStack to `POST /api/v1/tenants/{id}/custom-domains`, labelled
"internal API", through no named adapter, in the very document ADR-0034
was written to correct. Kept and enumerated rather than removed: a direct
Studio-to-Hub call is impossible under ADR-0004's two-realm boundary,
because the Hub rejects a `learnstack` realm token. It is now
`/api/v1/internal/tenants/{id}/custom-domains` behind IHubTenantSync, in
ADR-0034's table and in Standards 20's.
Tenant-defined custom fields attached to `users` — a global table with no
tenant_id, no query filter and no policy. A tenant-authored column there
is a cross-tenant read by construction, and Phase 03 already says
attributes whose value depends on which tenant is asking live on the
membership. `target_entity = "User"` now resolves to membership_profiles,
the DDL example alters membership_profiles rather than users, the helper
extension methods hang off MembershipProfile, and pii_category joins the
table definition, which Phase 03 requires with no default and the
customization model omitted.
Erasure was one workflow where Phase 03 has two. As written, a
tenant-scoped request published UserAnonymisationRequestedV1 and a module
invalidated the Keycloak user — so a tenant admin closing an account in
their tenant logged the person out of every other tenant they belong to.
Split by authority: tenant-scoped erasure removes the membership and
touches no identity provider; global closure is platform-scoped, runs
through EnterPlatformAdminScope, and is the only path that invalidates
Keycloak.
Also: ADR-0022's banner now names both superseded delivery mechanisms
(Amendment 1 steps 3-4 and the Option B amendment) rather than only the
first, and its SaaS/Dedicated bullet stops routing private keys through
the entitlement push; the SignedLicenseKeyEntitlementProvider skeleton,
which Phase 02c assigned to Phase 11 and Phase 11 disclaimed, is Hub
P02c-6's coordinated PR; the frontend --passWithNoTests tolerance is
removed with the first test that satisfies it rather than seven packets
before it, so the required check never sits red across a boundary; 05-mvp-
scope and add-provider-adapter stop enumerating four Hub endpoints; the
architecture-test catalogue gains the four Phase 02b entries the roadmap
registers but the catalogue never listed; and README states plainly that
only Development and SaaS are wired, the other three modes being prepared
seams.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last two waves, committed together in each repository: 48 minors and
18 polish items, none of which shares a file with another in a way that
made two commits more readable than one. Six of the 66 were already
closed by earlier waves and are recorded in the execution log rather than
re-applied.
The ones that were more than cosmetic:
- Data migrations could not migrate data. "Per-tenant data migrations use
the tenant role" named a role that is not one of the four, and
learnstack_migration is NOBYPASSRLS against FORCE ROW LEVEL SECURITY —
so a backfill that forgets SET LOCAL matches zero rows and reports
success. The rule now names the real role, requires the organization
variable too on org-scoped tables, and says why BYPASSRLS is not the
fix.
- The canonical DDL contradicted its own index rules: a standalone index
on tenant_id duplicated the leading column of two existing unique
b-trees, and a partial index on organization_id excluded exactly the
tenant-wide rows the policy's IS NULL branch matches. One composite
index, deliberately not partial.
- outbox_messages had no partition_key column, while ordering is promised
per partition key and an architecture test asserts every event declares
one.
- IEntitlementProvider_Implementations_Are_Three asserted "exactly three
exist" and was therefore red from Packet 9 until P02c-6. It now asserts
that none exists outside the named three — green throughout, tightening
as each lands.
- ADR-0035's LiveKit row failed the four-element test the ADR states
three lines above it, and its trigger ("the classroom phase begins")
was circular. Both LiveKit and audit_log partitioning are now named as
deliberate exceptions with the reason each is one.
- The Hub's "6-step vs 8-step" pipeline framing was off by one: it said
two steps drop out and listed one, because LearnStack's eight-step list
counts the Handler. Both sides now count behaviors — seven and six —
and say so.
- Phase 02a Packet 6 and ADR-0003 disagreed on when the role model lands.
Packet 6, with the policies: FORCE ROW LEVEL SECURITY has no effect
worth having while the connecting role is still the owner.
Wave 12 is formatting and provenance: phase-08b re-wrapped to the corpus
width (verified lossless — identical after whitespace normalisation, and
Markdown links kept atomic so none splits across a line), the 88-column
rule written into Documentation Standards where it had been convention
only, two anchor targets in the customization model pointing at a section
that is not the one they name, the ADR index's "pick the next available
number" replaced by the rule that accounts for reserved numbers, and
Status banners on the four Hub packet documents that opened straight into
Goal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tually have Wave 8 rewrote 222 Hub-side cross-repo links from sibling-relative paths to absolute URLs, and used `github.com/cemililik/LearnStack` — the owner CLAUDE.md and README.md both name. Both repositories have since moved to the HodeTech organisation, and `git remote get-url origin` says so in each of them. The old URLs are not broken today: GitHub answers them with a 301 to HodeTech. That is precisely why this is worth fixing before the links multiply. A rename redirect is not a permanent alias — it survives only while nothing exists at the old path, and `cemililik` is a live personal account. The day anything is created at `cemililik/LearnStack`, 250 links stop redirecting and start resolving to a different repository, silently. A 404 would be the kinder failure. Scoped deliberately. `cemililik/leakwatch` (Go module path), `cemililik/tap/leakwatch` (Homebrew tap) and `@cemililik` (Hub CODEOWNERS) are left exactly as they are. Leakwatch moved to HodeTech too, but a Go module's canonical import path is fixed by the `module` directive in its own `go.mod`, so rewriting the install command can break `go install` outright rather than redirect it — and a CODEOWNERS entry names a person, not a repository. Same trap as the `entitlement` rename: the token appears in more than one namespace and only one of them is this change's business. verify.sh gains the check, so the owner cannot drift back. Co-Authored-By: Claude Opus 5 (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, your pull request is larger than the review limit of 150000 diff characters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR revises architecture, standards, ADRs, roadmap documents, skills, and repository guidance. It updates tenant isolation, audit durability, Hub contracts, demand-gated infrastructure, customization boundaries, and phase sequencing. ChangesArchitecture and repository guidance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRestructures the Phase 02a roadmap and related docs around the one-way-door test, corrects the tenant isolation and audit durability models, wires cross-repo contracts and architecture-test catalogue, and incorporates the results of a 151-finding audit into the documentation and standards without changing runtime behavior. Sequence diagram for the corrected audit durability pipelinesequenceDiagram
actor User
participant Handler as CommandHandler
participant TxBehavior as TransactionBehavior
participant AuditBehavior as AuditLogBehavior
participant Db as DbContext
participant Interceptor as AuditChangeTrackerInterceptor
participant State as IAuditStateCapture
participant AuditStore as IAuditStore
User->>AuditBehavior: Handle(request)
AuditBehavior->>State: Declare(intent) [MUST-class]
AuditBehavior->>TxBehavior: next()
TxBehavior->>Db: BeginTransactionAsync()
TxBehavior->>Db: SetTenantContextAsync()
TxBehavior->>Handler: next()
Handler->>Db: SaveChangesAsync()
Db->>Interceptor: ISaveChangesInterceptor.On...()
Interceptor->>State: Add(CapturedEntityChange)
TxBehavior->>AuditStore: WritePendingAsync(unitOfWork)
TxBehavior->>Db: CommitAsync()
TxBehavior->>State: MarkCommitted()
TxBehavior-->>AuditBehavior: response
AuditBehavior->>AuditStore: WriteStandaloneAsync(...) [if State != Committed]
AuditBehavior->>AuditStore: WriteBestEffortAsync(...) [SHOULD/MAY]
AuditBehavior->>State: Clear()
AuditBehavior-->>User: response
Sequence diagram for the updated outbox claim and dispatch protocolsequenceDiagram
participant Processor as OutboxProcessor
participant Db as OutboxDbContext
participant Outbox as outbox_messages
participant Bus as IEventBus
participant Consumer as IIntegrationEventHandler
participant Inbox as IInboxGuard
loop Poll pending messages
Processor->>Db: BeginTransactionAsync()
Processor->>Outbox: UPDATE ... SET locked_by, locked_until, attempts+1
Processor->>Outbox: RETURNING *
Processor->>Db: CommitAsync() [lease persists]
alt batch has rows
loop For each OutboxMessage
Processor->>Db: BeginTransactionAsync()
Processor->>Bus: PublishAsync(event, partitionKey)
Bus-->>Consumer: HandleAsync(event)
Consumer->>Inbox: IsAlreadyProcessedAsync(event.EventId)
alt not processed
Consumer->>Consumer: BusinessLogic
Consumer->>Inbox: MarkAsProcessed(event.EventId,...)
end
Consumer->>Db: SaveChangesAsync()
Processor->>Outbox: UPDATE processed_at, locked_by=NULL
Processor->>Db: CommitAsync()
end
else No rows
Processor-->>Processor: Sleep(PollInterval)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/standards/20-infrastructure-stack.md (1)
159-176: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winResolve the host-cache L1 TTL conflict.
The general rule sets the hot-path host-to-tenant L1 TTL to 60 seconds. The cache table sets
hub:host:{host}L1 TTL to two minutes. Both values describe the same layer.Choose one value or document the host mapping as an explicit exception. Otherwise, deployments can use different stale-mapping windows after a failed invalidation.
🤖 Prompt for 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. In `@docs/standards/20-infrastructure-stack.md` around lines 159 - 176, Resolve the conflicting host-to-tenant L1 TTL guidance by making the general hot-path rule and the hub:host:{host} entry in the cache layer cheat sheet agree. Choose a single TTL, or explicitly document the two-minute host mapping as an exception in the surrounding standards text.
🟡 Minor comments (14)
docs/architecture/32-tenant-customization-model.md-263-295 (1)
263-295: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWire the renamed identifiers into the active registries.
COMPOSITE_KEYSomitssubmission-shell, soresolveRendererKey("submission-shell")returnsnull. Add the renderer key before using it in tenant data. AddFeatureKeys.SandboxedEvaluationwith valueassessment.sandboxed_evaluationto the typed feature catalog and entitlement wiring. The old identifiers occur only in the historical ADR amendment.🤖 Prompt for 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. In `@docs/architecture/32-tenant-customization-model.md` around lines 263 - 295, The active renderer registry must recognize submission-shell, and the renamed evaluation capability must be wired into the typed feature catalog and entitlement configuration. Add submission-shell to COMPOSITE_KEYS so resolveRendererKey returns it, then define FeatureKeys.SandboxedEvaluation with value assessment.sandboxed_evaluation and include it in the corresponding entitlement wiring; leave historical ADR references unchanged.docs/architecture/26-hybrid-license-model.md-368-370 (1)
368-370: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced code block.
Use
textfor the revocation-list URL block.🤖 Prompt for 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. In `@docs/architecture/26-hybrid-license-model.md` around lines 368 - 370, Update the fenced code block containing the revocation-list URL to specify the text language, without changing the URL or surrounding documentation.Source: Linters/SAST tools
docs/architecture/24-learnstack-hub.md-47-48 (1)
47-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd text fallbacks for all changed Mermaid diagrams.
Each changed diagram needs a short textual description for readers who cannot render Mermaid.
docs/architecture/24-learnstack-hub.md#L47-L48: describe both internal authentication directions.docs/architecture/27-custom-domain-tls.md#L43-L47: describe certificate replication, route reload, and mapping delivery.docs/architecture/30-api-gateway.md#L58-L59: describe the LearnStack and Hub traffic paths.As per coding guidelines, Markdown files must include readable text fallbacks for Mermaid diagrams.
🤖 Prompt for 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. In `@docs/architecture/24-learnstack-hub.md` around lines 47 - 48, Add concise readable text fallbacks for each changed Mermaid diagram: in docs/architecture/24-learnstack-hub.md lines 47-48, describe both internal authentication directions; in docs/architecture/27-custom-domain-tls.md lines 43-47, describe certificate replication, route reload, and mapping delivery; and in docs/architecture/30-api-gateway.md lines 58-59, describe the LearnStack and Hub traffic paths.Source: Coding guidelines
docs/architecture/27-custom-domain-tls.md-229-235 (1)
229-235: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine canonical host normalization before lookup.
The documented
Request.Host.Hostcaller removes the port but preserves casing and trailing dots. Since the resolver useshostverbatim for both the cache key andm.Host == host, equivalent hosts can miss mappings and create separate cache entries. Normalize at the boundary, or define and enforce a canonical-host contract for every caller.🤖 Prompt for 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. In `@docs/architecture/27-custom-domain-tls.md` around lines 229 - 235, The ResolveAsync method must use a canonical host value before both cache-key construction and the m.Host lookup. Normalize the incoming host by handling casing and trailing dots at the resolver boundary, then use that normalized value consistently while preserving the existing active-mapping query behavior.docs/architecture/30-api-gateway.md-265-267 (1)
265-267: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the unsupported CORS wildcard.
APISIX 3.16.0 accepts exact
scheme://host:portentries inallow_origins;*.learnstack.appis not a subdomain wildcard and fails the plugin schema. Use anchored patterns withallow_origins_by_regex, or enumerate exact origins. Apply this to all three occurrences at lines 265, 277, and 296.🤖 Prompt for 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. In `@docs/architecture/30-api-gateway.md` around lines 265 - 267, Replace the unsupported *.learnstack.app entry in all three API gateway CORS configurations with valid anchored patterns via allow_origins_by_regex, or enumerate the exact allowed origins. Update each occurrence near the upstream definitions while preserving the existing CORS and rate-limit settings.docs/standards/21-architecture-tests-catalogue.md-10-13 (1)
10-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the catalogue scope accurately.
The introduction says the catalogue covers rules enforced at build time. This file also contains runtime Testcontainers and HTTP integration proofs.
Replace that wording with “tracks and enforces” or state that the catalogue includes structural, compile-time, and runtime checks.
🤖 Prompt for 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. In `@docs/standards/21-architecture-tests-catalogue.md` around lines 10 - 13, Update the catalogue introduction to accurately include runtime Testcontainers and HTTP integration checks, replacing the build-time-only scope statement with wording that says it tracks and enforces structural, compile-time, and runtime checks.docs/roadmap/phase-02c-hub-foundation.md-12-18 (1)
12-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the sibling-repository paths.
This file is under
docs/roadmap. The path../LearnStack-Hub/...resolves below this repository'sdocsdirectory. It does not resolve to a sibling repository at repository root.Use
../../LearnStack-Hub/...if the sibling layout is intended.Proposed path fix
- `../LearnStack-Hub/docs/roadmap/` + `../../LearnStack-Hub/docs/roadmap/`Also applies to: 50-54, 73-76
🤖 Prompt for 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. In `@docs/roadmap/phase-02c-hub-foundation.md` around lines 12 - 18, Update the sibling-repository references in this roadmap document, including the status text and the sections around the other affected references, from ../LearnStack-Hub/... to ../../LearnStack-Hub/... so they resolve from docs/roadmap to the repository sibling at the workspace root.docs/roadmap/phase-03-identity-admin.md-133-151 (1)
133-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one valid PII category contract.
Paymentis not in the closed category set, but the completion criteria refer to a definition with categoryPayment. State that an unsupported category such asPaymentis rejected, or addPaymentto the set and define its retention rules.Also applies to: 388-390
🤖 Prompt for 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. In `@docs/roadmap/phase-03-identity-admin.md` around lines 133 - 151, Align the PII category contract in the “PII classification on JSONB” section: either remove `Payment` from completion criteria and state that unsupported categories are rejected, or add `Payment` to the closed set with explicit retention rules. Ensure the save-time rejection behavior and all references use the same valid category contract.docs/roadmap/phase-04-cms-media-pages.md-433-434 (1)
433-434: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNarrow the
ContentTypeexit criterion.This document and the architecture-test catalogue retain
ContentTypeas a historical or superseded spelling. Therefore,ContentTypecannot disappear from the entire corpus as written.Change the criterion to require that no active domain model, schema, permission, or code symbol uses
ContentType. Permit historical references that explain its removal.🤖 Prompt for 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. In `@docs/roadmap/phase-04-cms-media-pages.md` around lines 433 - 434, Revise the ContentType exit criterion in the Phase 04 CMS media pages roadmap to require that no active domain model, schema, permission, or code symbol uses ContentType, while explicitly allowing historical or superseded references documenting its removal. Keep TenantContentType as the required replacement.docs/roadmap/phase-02b-events-auth.md-83-89 (1)
83-89: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the audit correlation contract with
traceparent.The audit payload uses a trace-ID-shaped value and requires
correlationIdto match the trace ID, while the event, observability, and Problem Details contracts use the full W3Ctraceparent. Define one value or document the explicit mapping before implementation.🤖 Prompt for 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. In `@docs/roadmap/phase-02b-events-auth.md` around lines 83 - 89, Clarify the audit correlation contract in the roadmap by choosing a single canonical value or explicitly documenting the mapping between the trace-ID-shaped audit correlationId and the full W3C traceparent used by integration events, observability, and Problem Details. Update the audit payload requirements and related references consistently so consumers know which value to store and propagate.docs/architecture/02-domain-model.md-409-416 (1)
409-416: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse one audit timestamp name across the schema documentation.
ADR-0033 and
architecture/31-audit-subsystem.mddefineaudit_log.timestampand(id, timestamp), but.claude/skills/add-ef-migration/SKILL.mdusesoccurred_atfor the same table. Update the migration guidance or document an explicit mapping before implementation.🤖 Prompt for 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. In `@docs/architecture/02-domain-model.md` around lines 409 - 416, Align the migration guidance in add-ef-migration with the audit schema’s canonical timestamp name, timestamp, used by ADR-0033 and architecture/31-audit-subsystem.md. Replace occurred_at for audit_log references, or explicitly document a mapping if the migration layer must retain that name..claude/skills/add-permission/SKILL.md-61-61 (1)
61-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
course accessterminology in the permission descriptions.The keys now use
enrollment.course_access.*, but the descriptions still say “entitlements”.CourseAccessand Hub planEntitlementare separate concepts. Update both descriptions and the nearby matrix label so the permission catalogue names the capability correctly.Also applies to: 86-91
🤖 Prompt for 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. In @.claude/skills/add-permission/SKILL.md at line 61, Update the permission descriptions and nearby permission matrix label for the enrollment.course_access.* entries to use “course access” terminology instead of “entitlements,” while keeping the existing permission keys unchanged.docs/architecture/04-technical-architecture.md-150-150 (1)
150-150: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSynchronize API route examples with the
/api/v1/...convention.The application has no global
/apiprefix. Update both.claudeskill examples from[Route("v1/enrollments")]to[Route("api/v1/enrollments")].🤖 Prompt for 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. In `@docs/architecture/04-technical-architecture.md` at line 150, Update both .claude skill examples containing [Route("v1/enrollments")] to use the [Route("api/v1/enrollments")] convention, keeping the documented API versioning examples consistent with the architecture guidance..github/workflows/ci.yml-166-166 (1)
166-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the remaining phase text for both jobs.
The job names now say
Phase 02d, but the OpenAPI placeholder at Line 170 still saysPhase 03. The Lighthouse heading and placeholder at Lines 172 and 178 still sayPhase 04. This gives conflicting activation instructions. Change those remaining references toPhase 02d.Proposed wording update
- - run: echo "Placeholder — Phase 03 wires oasdiff against the prior main spec." + - run: echo "Placeholder — Phase 02d wires oasdiff against the prior main spec." - # ─── Lighthouse budget (deferred — Phase 04) ────────────────────────── + # ─── Lighthouse budget (deferred — Phase 02d) ───────────────────────── - - run: echo "Placeholder — Phase 04 wires LHCI against the built Next.js app." + - run: echo "Placeholder — Phase 02d wires LHCI against the built Next.js app."Also applies to: 172-174
🤖 Prompt for 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. In @.github/workflows/ci.yml at line 166, Update the remaining phase references in the OpenAPI and Lighthouse job headings/placeholders to say “Phase 02d,” including the text around the OpenAPI placeholder and Lighthouse heading and placeholder; leave the job behavior unchanged.
🧹 Nitpick comments (1)
docs/roadmap/phase-02b-events-auth.md (1)
319-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the topic-test status with the catalogue.
This phase says
Dapr_PubSub_TopicNames_FollowConventionapplies and is enforced here. The infrastructure standard and architecture catalogue state that automated enforcement lands with the Dapr adapter in Phase 11 and is reviewer-enforced before then.State the Phase 02b status as reviewer-enforced, or add a transport-independent automated test now.
🤖 Prompt for 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. In `@docs/roadmap/phase-02b-events-auth.md` around lines 319 - 322, Update the Phase 02b topic-test status around Dapr_PubSub_TopicNames_FollowConvention to align with the catalogue: describe the convention as reviewer-enforced until Phase 11, unless a transport-independent automated test is added in this phase. Keep the existing Phase 11 Dapr adapter enforcement reference accurate.
🤖 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.
Outside diff comments:
In `@docs/standards/20-infrastructure-stack.md`:
- Around line 159-176: Resolve the conflicting host-to-tenant L1 TTL guidance by
making the general hot-path rule and the hub:host:{host} entry in the cache
layer cheat sheet agree. Choose a single TTL, or explicitly document the
two-minute host mapping as an exception in the surrounding standards text.
---
Major comments:
In @.claude/skills/add-architecture-test/SKILL.md:
- Around line 157-165: Update the migration validation around the FORCE and
tenant-predicate assertions to parse executable SQL rather than scanning the raw
file content. Strip SQL comments and isolate the relevant ALTER TABLE and CREATE
POLICY statements, then apply the existing checks to those statements so
comments or unrelated strings cannot satisfy them.
In @.claude/skills/add-audit-coverage/SKILL.md:
- Around line 27-30: Update the pipeline description around AuditLogBehavior and
TransactionBehavior to state that rollback recovery inserts a separate
failed-outcome audit row rather than re-writing the original row. Define that
the new row correlates with the original parked intent, and apply the same
terminology consistently in the referenced rollback and append-only guidance.
- Around line 19-31: Revise the guidance around AuditLogBehavior and
TransactionBehavior to describe decide → write → reconcile as a future contract,
not an active pipeline. Remove claims that either behavior classifies, writes,
reconciles, commits, or enlists audit records; state that the current Phase 02a
shells only log exceptions or delegate to next(). Do not present the durability
model as binding unless the implementation and tests are added.
In @.claude/skills/add-ef-migration/SKILL.md:
- Around line 163-169: Update the executable migration example in the section
describing learnstack_migration and learnstack_app to include the required grant
on the newly created table to learnstack_app. Keep the grant in the migration
SQL so copied migrations preserve application access.
- Around line 106-108: Update the migration SQL template so the
ix_<name_plural>_organization_id index is generated only for organization-scoped
tables. Move that CREATE INDEX statement into the organization-scoped section,
while keeping the tenant_id index available for tenant-only tables.
- Around line 95-104: Update the foreign-key guidance in the migration skill to
include organization_id in composite keys for organization-scoped
child-to-parent relationships, referencing matching organization_id columns on
the parent. If any cross-organization references are intentionally allowed,
document that rule and specify how it is enforced rather than presenting
tenant_id alone as universally sufficient.
- Around line 157-161: Replace the full RLS SQL template in the migration skill
with a link to docs/standards/05-database.md, retaining only migration-specific
instructions. Remove duplicated policy details so Database Standards remains the
sole canonical source, and preserve the ADR-0003 Amendment 3 reference where
relevant.
- Around line 244-267: Update the backfill flow around the transaction and
UPDATE statement to iterate through every organization, creating a transaction
per organization and setting app.organization_id within it before the update.
Execute the existing UPDATE under that organization context, verify its
affected-row count against the expected result, and commit only after the
assertion succeeds; do not rely on app.scope or a single tenant context to widen
RLS access.
- Around line 141-153: Update the CREATE POLICY definitions for
<name_plural>_org_write_guard and <name_plural>_org_delete_guard so
organization_id IS NULL is permitted only when current_setting('app.scope',
true) equals 'tenant'; retain the existing organization_id match for scoped
sessions and add equivalent WITH CHECK protection for updates so rows cannot be
changed to organization_id = NULL unintentionally.
In @.claude/skills/add-tenant-owned-entity/SKILL.md:
- Around line 138-146: Remove the embedded SQL/RLS template from the
instructions around the generated migration section and the repeated blocks at
the referenced ranges. Replace each with a concise checklist directing readers
to the canonical “Tenant-Owned and Organization-Scoped Tables” section in
docs/standards/05-database.md, preserving the requirement for one RLS policy
without duplicating executable SQL.
- Around line 159-163: Update the generated entity’s EF configuration to map
IOptimisticConcurrency.Version to the row_version column as a concurrency token,
and wire the shared SaveChangesInterceptor to increment the version on every
update. Keep DEFAULT 0 only for insert initialization, and ensure the change
applies to the entity-generation flow in add-tenant-owned-entity.
In @.claude/skills/code-review/SKILL.md:
- Around line 265-266: Update the Hub HTTPS contract guidance near the “named
adapter” wording to explicitly allow only IEntitlementProvider, IUsageReporter,
and IHubTenantSync. Add that IHostToTenantResolver reads local
platform_host_to_tenant and must never call the Hub, removing the broader
named-adapter allowance.
In @.claude/skills/standards-check/SKILL.md:
- Around line 191-211: The RLS checklist contradicts itself about organization
predicates and policy counts. Update the checklist so tenant isolation applies
to every [TenantOwned] table, while the organization predicate and nullable
organization_id apply only to [OrganizationScoped] tables; clarify that each
table has one AND-combined isolation policy, with the required restrictive
UPDATE and DELETE write guards treated as separate guards rather than additional
isolation policies.
In `@backend/src/LearnStack.SharedKernel/Secrets/ConfigurationSecretProvider.cs`:
- Around line 8-10: Update the composition-root guidance comment in
CrossCuttingFoundationExtensions so it no longer references the superseded Phase
02a Packet 5 rollout or limits ConfigurationSecretProvider to Development; align
its provider schedule and deployment-mode guidance with the Phase 11 rollout
documented by ConfigurationSecretProvider.
In `@docs/architecture/03-module-boundaries.md`:
- Line 124: Update the Hub-to-LearnStack license-verification contract label to
require a signed JWT alongside mTLS and HMAC, and document the corresponding JWT
validation requirements in the associated ADR or contract definition.
In `@docs/architecture/06-extension-model.md`:
- Around line 156-165: Update the Dapr deployment guidance in
04-technical-architecture.md and 05-mvp-scope.md to identify Dapr-backed Kafka,
Valkey, and Vault as Phase 11 targets rather than current MVP infrastructure.
State consistently that the in-process providers are the current default in
every deployment mode until Phase 11, matching ADR-0035, ADR-0014, and the
extension-model guidance.
In `@docs/architecture/15-event-and-outbox.md`:
- Around line 307-316: Restructure the per-message flow around the foreach loop
so PublishAsync executes outside any database transaction. Begin the short
transaction only after publishing, then perform the lease-guarded MarkProcessed
success update (and corresponding failure update) before committing, preserving
the existing poisoned-message isolation and lease-loss no-op behavior.
- Around line 433-447: Update InProcessEventBus.PublishAsync to capture the
current tenant context before tenantAccessor.Set, then wrap handler execution in
a finally block that restores the captured context, including when handler
processing fails.
In `@docs/architecture/21-feature-flags.md`:
- Around line 133-146: Update the generation acceptance rule in the documented
tenant projection flow to prevent equal-generation pushes from overwriting
stored data: require received.generation to be strictly greater than the stored
value, or explicitly verify payload identity before allowing equality. Document
the first-write exception for generation 1 if the POST /api/internal/tenants
provisioning insert depends on it, and ensure replays remain no-ops.
In `@docs/architecture/23-data-protection.md`:
- Around line 63-71: Update the “Tenant-scoped erasure” flow to explicitly
handle removal of the user’s last membership: transition to the global closure
process or define the required identity-PII anonymisation and Keycloak
invalidation steps. Clarify that the users row remains unchanged only when
memberships still exist elsewhere.
In `@docs/architecture/24-learnstack-hub.md`:
- Line 217: Define one authoritative host-mapping payload schema and reference
it consistently across docs/architecture/24-learnstack-hub.md:217-217,
docs/architecture/27-custom-domain-tls.md:274-290, and
docs/architecture/28-platform-tenant-organization.md:269-269. Update the Hub
endpoint to use the shared schema instead of “tuple only”; retain
certificate_ref only if included in that schema and describe it as a path
reference, while keeping certificate and private-key material out of HTTP
payloads. Use identical schema and terminology in the tenant/organization
document.
In `@docs/architecture/25-deployment-models.md`:
- Around line 237-242: Make prepared deployment modes fail closed: in
docs/architecture/25-deployment-models.md:237-242, remove the statement that
prepared modes use the Development defaults while preserving the distinction
between a prepared seam and a supported mode; in
docs/architecture/26-hybrid-license-model.md:474-475, retain and enforce the
outside-Development registration check so unsupported non-development modes fail
startup or use a fail-closed entitlement provider.
In `@docs/architecture/26-hybrid-license-model.md`:
- Around line 257-290: The entitlement-checking flow around GetAsync must either
catch and map failures from cache, durable-store, write, and Evaluate operations
according to the documented policy, or narrow its stated guarantee to Hub
transport failures. Update the surrounding documentation and implementation
consistently, preserving cancellation behavior and ensuring storage failures
cannot contradict any exception-free claim.
- Around line 437-439: Update the revocation flow described in the “Revoked
license still cached” architecture entry to prevent stale L1/L2 values from
being served during asynchronous invalidation. Specify an atomic revocation
generation/version check or fail-closed invalidation protocol that rejects
cached entitlements once revocation begins, while preserving the required
invalidation order and durable-row behavior.
In `@docs/architecture/27-custom-domain-tls.md`:
- Around line 229-238: Update ResolveAsync’s CacheOptions so both L1Ttl and
L2Ttl use the documented 60-second fallback limit, preserving the existing
host-mapping lookup and cache behavior.
In `@docs/architecture/29-dapr-integration.md`:
- Around line 167-169: Update the “Development” secret-provider documentation to
state that the composition root always uses ConfigurationSecretProvider and that
dapr-file selection via Deployment:Secrets:Provider is not currently
implemented; label the Dapr file provider as a future option so operators are
not misled.
- Around line 221-223: Complete the migration away from prefix invalidation in
DaprCacheService: remove RemoveByPrefixAsync, its per-instance key tracking, and
the old invalidation topic, then update every caller to use the generation-key
design instead. Ensure no supported deployment can still invoke the legacy
prefix-invalidation path.
In `@docs/architecture/31-audit-subsystem.md`:
- Around line 825-836: Replace the raw audit_log UPDATE in the redaction handler
with the appropriate IAuditStore operation, using the platform-admin connection
required for audit-log mutations. Keep the existing redacted actor_email,
ip_address, user_agent, UserId, and TenantId behavior, and leave the per-module
userReferenceLocators processing unchanged.
- Around line 838-853: The redaction flow must not finalize the inbox event
before the MUST audit is durable. Update the handler around
inboxGuard.MarkAsProcessed, db.SaveChangesAsync, and
auditStore.WriteStandaloneAsync so redaction, inbox completion, and
audit.redaction.apply commit atomically, or retain a retryable pending state
when the audit write fails; ensure retries can still write the audit row.
- Around line 301-303: Update the MUST fallback write in the non-Committed
branch around BuildDraft and WriteStandaloneAsync to use a non-cancelable or
independently bounded reconciliation token instead of the request token ct,
ensuring the required audit row is persisted after rollback or an indeterminate
commit while leaving cancellation behavior for SHOULD/MAY writes unchanged.
- Around line 184-200: The AuditStateCapture implementation must support the
full IAuditStateCapture lifecycle, not only change collection. Add storage and
implementations for Intent, State, Declare, MarkWrittenInTransaction,
MarkCommitted, MarkRolledBack, and MarkIndeterminate, ensuring each transition
records the appropriate state and indeterminate cause; update Clear() to clear
changes and reset intent and state to None so requests cannot retain prior audit
data.
- Around line 34-38: Define a canonical representation for the four BuildDraft
outcomes—Denied, Failed, Indeterminate, and Success—and use it consistently
across AuditEntry, audit_log, persistence paths, queries, and redaction rules.
Replace or supplement the schema’s is_success-only storage so each outcome
remains distinguishable, including grouping indeterminate duplicates.
- Around line 734-738: Resolve the conflicting audit_config read-failure policy
by aligning the statement near ClassifyAsync with Section 5 and the architecture
test: read failures must fall back to the in-process catalogue and allow
classification to complete. Update the MUST-floor description only as needed to
preserve the existing override behavior, and keep the documentation consistent
across all referenced sections.
In `@docs/architecture/32-tenant-customization-model.md`:
- Around line 319-331: Make the User custom-field contract consistent by
prohibiting User as a target: remove the membership_profiles alias wording,
update the helper example using User.CustomFields to reference the tenant-owned
membership profile, and ensure the target_entity documentation consistently
lists User as invalid.
- Around line 460-462: Update the cold-start query bound in the
TenantLevelTaxonomy caching discussion to account for one query per distinct
taxonomy key used by the request, rather than asserting an unconditional maximum
of four. Express the bound as the fixed base query count plus the number of
taxonomy keys, while preserving the existing per-definition-set behavior.
- Around line 522-546: Choose a single canonical identifier for the HTML
embedding primitive, aligning the registry key with the `embed-html` references
in this section or vice versa. Update all related exact-key lookups, schema
extensions, lint rules, and audit references consistently so no
`embed_html`/`embed-html` divergence remains.
- Around line 413-418: Update the validation timing table to assign each §8.3
limit to the write operation that owns the relevant object: keep schema limits
on schema saves, move content-entry limits to entry saves, page limits to page
saves, and import limits to import writes. Preserve the existing failure-mode
details and validation entries that are not affected.
- Around line 444-458: The schema identity used by the uniqueness rules and
compiled-validator cache must include every immutable revision and tenant
component. Update the cache key and related prose around the generation-counter
and compiled-validator rules to use a complete identity such as tenant_id,
content_type_key, schema_version, and schema_revision, or a single immutable
version-row identifier; apply the same correction to the additionally referenced
section.
- Around line 415-418: Clarify the write-time validation rule for dynamic
x-language values in the json_schema contract. Define bounded, template-aware
handling for the "{language}" placeholder that still validates against the
allowed language registry, or replace the worked example with a fixed language
so every x-language extension resolves when the schema is saved.
- Around line 507-513: Update the array limit contract in the tenant
customization validation flow: select and name the JSON Schema validator, then
either reject schemas lacking maxItems or inject maxItems: 200 into the
effective schema before validating schemas and entries, matching Phase 05. Add
coverage proving arrays exceeding 200 items are rejected, and ensure the
documented limit reflects the enforced behavior.
- Around line 507-510: Update the schema limits section near the `$ref`/`$defs`
nesting and reference-resolution depth entries to define save-time detection and
rejection of cyclic `$ref` and `$dynamicRef` graphs, or another explicit
termination rule. Add a JSON Schema validator wall-clock or step budget with the
required behavior when exceeded, separately from the entry reference-resolution
depth limit.
- Around line 539-548: Replace the vague embed-html sanitisation rules in the
architecture table with one explicit, executable policy defining allowed
elements, attributes, and positive URL schemes, including the resolved treatment
of data:image/* URLs. Specify exact iframe host matching, mutually exclusive
sandbox-token selection, and required CSP directives; then update the
architecture, roadmap, security standard, and save/render tests to reference
this shared policy contract.
In `@docs/decisions/0014-adopt-dapr.md`:
- Around line 255-268: Align the DaprEventBus decision with the OutboxProcessor
deployment topology: either register DaprEventBus when integration-event
consumers run in LearnStack.Api, or document that the outbox worker and
consumers share one process and remove the conflicting transport statements from
the adapter table and surrounding implementation notes.
In `@docs/decisions/0028-audit-log-partition-management.md`:
- Around line 249-253: Update the remaining Phase 02a partitioning guidance in
this ADR: revise the first-migration/partitioned-parent note, architecture-test
phase, and DDL references so Phase 02a creates a plain audit_log table and Phase
11 performs partitioning and retention based on measured growth. Remove or
update stale ADR-0016 strategy references while preserving the new
plain-table-then-Phase-11 conversion contract.
In `@docs/decisions/0032-exception-handling-logging-and-observability.md`:
- Around line 29-39: Update the canonical pipeline list so TenantContextBehavior
only asserts and carries the resolved tenant context, while TransactionBehavior
performs the RLS GUC assignment as its first statement inside the transaction.
Remove the conflicting pre-transaction SET LOCAL/set_config wording and preserve
the existing pipeline order.
- Around line 16-22: Replace the non-compiling services.Decorate<TPort,
ResilientProviderAdapter<TPort>>() sample in sub-decision 5 with the shipped
AddSingleton<IProviderResilience<TPort>> collaborator registration, or remove
the sample and link to LearnStack.Infrastructure.Resilience; do not show
decoration of the port type.
In `@docs/decisions/0033-audit-durability-model.md`:
- Around line 76-85: The reconciliation flow described in TransactionBehavior
must make Indeterminate outcomes idempotent: reuse a stable idempotency key for
the original audit intent and perform the standalone retry with a duplicate-safe
insert so an already-committed in-transaction row is not duplicated. Add
coverage for CommitAsync throwing after the server commits, verifying only one
audit row exists.
In `@docs/glossary.md`:
- Around line 278-280: Align the demand-gated definitions with ADR-0035: in
docs/glossary.md lines 278-280, exclude schema-internal audit_log partitioning
and scheduled LiveKit from adapter-backed demand-gated blocks, while including
managed video transcoding where applicable; in docs/roadmap/README.md lines
30-34, scope the four-field port/default/owner/trigger rule to adapter-backed
blocks; in docs/roadmap/README.md line 114, remove audit_log partitioning from
the adapter classification; and in docs/standards/00-principles.md lines
118-123, document the schema-internal exception instead of requiring an
interface and default implementation.
In `@docs/roadmap/phase-02b-events-auth.md`:
- Around line 355-359: Update docs/roadmap/phase-02b-events-auth.md lines
355-359 to promise at-least-once outbox dispatch with one committed business
effect enforced through IInboxGuard; revise lines 404-409 to describe explicit
duplicate-delivery handling through the inbox guard instead of “no duplicate
handling”; update docs/standards/21-architecture-tests-catalogue.md lines
1033-1043 to test concurrent claim protection and idempotent consumption, not
exactly-once dispatch.
- Around line 158-163: Update the event retry design around inbox_messages to
choose one durable model: add per-(event, consumer) attempt and dead_lettered
state to the inbox schema, or consistently define and use a separate dead-letter
store instead of leaving inbox rows unprocessed. Specify that failure-state
updates occur independently from the handler transaction, while successful
business writes and marking the inbox message processed remain atomic.
In `@docs/roadmap/phase-03-identity-admin.md`:
- Around line 174-182: Update the custom-field target allowlist in the
tenant_custom_field_defs roadmap section to support Phase 04 ContentEntry
values, including ContentEntry as a Phase 04-owned target and documenting its
applicable PII and audit rules; otherwise remove or defer the Phase 04
custom-field deliverable.
In `@docs/roadmap/phase-04-cms-media-pages.md`:
- Around line 111-125: Clarify the schema revision contract in the roadmap:
state that additive revisions are the only validation-contract changes allowed
within a schema_version, and define how entries select the applicable
schema_revision, including whether they always use the latest compatible
revision. Ensure validation and rendering behavior for existing entries is
explicitly determined.
In `@docs/roadmap/phase-06-renderer-admin-studio.md`:
- Around line 193-206: Update the Phase 06 E2E compose diagnosis to state that
volumes: !reset [] removes the PostgreSQL init-script and SeaweedFS
identity-file mounts, the init script creates only the keycloak database, and
Valkey retains valkey-data:/data so state persists. Ensure the referenced E2E
overlay restores the PostgreSQL and SeaweedFS mounts and resets Valkey state
between runs.
In `@docs/roadmap/phase-08a-assessment-notifications.md`:
- Around line 155-159: The notification flow must make external effects
idempotent despite at-least-once delivery: define a durable notification record,
use a stable provider idempotency key, and document crash recovery around
IEmailProvider calls and IInboxGuard. In
docs/roadmap/phase-08a-assessment-notifications.md lines 155-159 and 205-206,
update the “sends one message” criteria to require durable, retry-safe
idempotent delivery. In docs/roadmap/phase-11-production-hardening.md lines
458-460, replace “consumed exactly once” with at-least-once delivery and an
idempotent consumer effect.
In `@docs/roadmap/phase-09-billing-integrations-analytics.md`:
- Around line 289-295: Update the Phase 09 “Completion Criteria” section to
remove the credit-pack balance decrement and cancellation refund requirement.
Limit the criterion to purchasing a credit-pack product and emitting
OrderPaidV1, without specifying ledger, consumption, or refund behavior.
In `@docs/roadmap/phase-11-production-hardening.md`:
- Around line 350-355: Align the LiveKit production policy between the Phase 08c
and Phase 11 roadmap gates: choose either LiveKit Cloud or self-hosted LiveKit
as the production posture, then update both phases’ requirements and exit
criteria consistently. Ensure the self-hosted switch and validation requirements
match across the two phase gates so one phase cannot approve a state rejected by
the next.
In `@docs/standards/05-database.md`:
- Around line 47-51: Rewrite the opening tenant-table rule to explicitly exempt
self-keyed tables such as tenants from requiring tenant_id, and distinguish the
single permissive RLS policy from any additional restrictive guards used by
organization-scoped tables. Keep the existing EF-filter requirement and
organization_id guidance consistent with the canonical exceptions.
In `@docs/standards/11-security.md`:
- Around line 129-136: Update the introduction of the APISIX security section to
make tenant-facing traffic requirements conditional on APISIX being active,
consistent with the demand-gated Phase 11 rollout and ASP.NET middleware
responsibilities before activation. Preserve the existing security requirements
once APISIX is in front of production traffic.
In `@docs/standards/README.md`:
- Around line 73-77: Make the standards status authoritative in a single
location: either update all individual document headers to match the table in
README.md, or remove their conflicting status values and keep the table as the
sole source. Eliminate the statement that headers still declare Active once the
competing source is resolved.
- Around line 43-66: Add an ADR citation to docs/standards/README.md covering
the Active and Adopted definitions, the no-implementing-code rule, and the
Adopted-to-Active promotion rule. If the decision record does not yet exist,
create it and link it from the relevant standards section.
In `@infra/dapr/README.md`:
- Around line 19-31: Update the phase-ownership statements in the sections
describing Dapr adapters and client packages to assign DaprEventBus,
DaprCacheService, and DaprSecretProvider to Phase 11. Remove the conflicting
Phase 02a claims while preserving the surrounding implementation guidance.
---
Minor comments:
In @.claude/skills/add-permission/SKILL.md:
- Line 61: Update the permission descriptions and nearby permission matrix label
for the enrollment.course_access.* entries to use “course access” terminology
instead of “entitlements,” while keeping the existing permission keys unchanged.
In @.github/workflows/ci.yml:
- Line 166: Update the remaining phase references in the OpenAPI and Lighthouse
job headings/placeholders to say “Phase 02d,” including the text around the
OpenAPI placeholder and Lighthouse heading and placeholder; leave the job
behavior unchanged.
In `@docs/architecture/02-domain-model.md`:
- Around line 409-416: Align the migration guidance in add-ef-migration with the
audit schema’s canonical timestamp name, timestamp, used by ADR-0033 and
architecture/31-audit-subsystem.md. Replace occurred_at for audit_log
references, or explicitly document a mapping if the migration layer must retain
that name.
In `@docs/architecture/04-technical-architecture.md`:
- Line 150: Update both .claude skill examples containing
[Route("v1/enrollments")] to use the [Route("api/v1/enrollments")] convention,
keeping the documented API versioning examples consistent with the architecture
guidance.
In `@docs/architecture/24-learnstack-hub.md`:
- Around line 47-48: Add concise readable text fallbacks for each changed
Mermaid diagram: in docs/architecture/24-learnstack-hub.md lines 47-48, describe
both internal authentication directions; in
docs/architecture/27-custom-domain-tls.md lines 43-47, describe certificate
replication, route reload, and mapping delivery; and in
docs/architecture/30-api-gateway.md lines 58-59, describe the LearnStack and Hub
traffic paths.
In `@docs/architecture/26-hybrid-license-model.md`:
- Around line 368-370: Update the fenced code block containing the
revocation-list URL to specify the text language, without changing the URL or
surrounding documentation.
In `@docs/architecture/27-custom-domain-tls.md`:
- Around line 229-235: The ResolveAsync method must use a canonical host value
before both cache-key construction and the m.Host lookup. Normalize the incoming
host by handling casing and trailing dots at the resolver boundary, then use
that normalized value consistently while preserving the existing active-mapping
query behavior.
In `@docs/architecture/30-api-gateway.md`:
- Around line 265-267: Replace the unsupported *.learnstack.app entry in all
three API gateway CORS configurations with valid anchored patterns via
allow_origins_by_regex, or enumerate the exact allowed origins. Update each
occurrence near the upstream definitions while preserving the existing CORS and
rate-limit settings.
In `@docs/architecture/32-tenant-customization-model.md`:
- Around line 263-295: The active renderer registry must recognize
submission-shell, and the renamed evaluation capability must be wired into the
typed feature catalog and entitlement configuration. Add submission-shell to
COMPOSITE_KEYS so resolveRendererKey returns it, then define
FeatureKeys.SandboxedEvaluation with value assessment.sandboxed_evaluation and
include it in the corresponding entitlement wiring; leave historical ADR
references unchanged.
In `@docs/roadmap/phase-02b-events-auth.md`:
- Around line 83-89: Clarify the audit correlation contract in the roadmap by
choosing a single canonical value or explicitly documenting the mapping between
the trace-ID-shaped audit correlationId and the full W3C traceparent used by
integration events, observability, and Problem Details. Update the audit payload
requirements and related references consistently so consumers know which value
to store and propagate.
In `@docs/roadmap/phase-02c-hub-foundation.md`:
- Around line 12-18: Update the sibling-repository references in this roadmap
document, including the status text and the sections around the other affected
references, from ../LearnStack-Hub/... to ../../LearnStack-Hub/... so they
resolve from docs/roadmap to the repository sibling at the workspace root.
In `@docs/roadmap/phase-03-identity-admin.md`:
- Around line 133-151: Align the PII category contract in the “PII
classification on JSONB” section: either remove `Payment` from completion
criteria and state that unsupported categories are rejected, or add `Payment` to
the closed set with explicit retention rules. Ensure the save-time rejection
behavior and all references use the same valid category contract.
In `@docs/roadmap/phase-04-cms-media-pages.md`:
- Around line 433-434: Revise the ContentType exit criterion in the Phase 04 CMS
media pages roadmap to require that no active domain model, schema, permission,
or code symbol uses ContentType, while explicitly allowing historical or
superseded references documenting its removal. Keep TenantContentType as the
required replacement.
In `@docs/standards/21-architecture-tests-catalogue.md`:
- Around line 10-13: Update the catalogue introduction to accurately include
runtime Testcontainers and HTTP integration checks, replacing the
build-time-only scope statement with wording that says it tracks and enforces
structural, compile-time, and runtime checks.
---
Nitpick comments:
In `@docs/roadmap/phase-02b-events-auth.md`:
- Around line 319-322: Update the Phase 02b topic-test status around
Dapr_PubSub_TopicNames_FollowConvention to align with the catalogue: describe
the convention as reviewer-enforced until Phase 11, unless a
transport-independent automated test is added in this phase. Keep the existing
Phase 11 Dapr adapter enforcement reference accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 370e3362-db86-483a-8055-fe2b7c36c529
📒 Files selected for processing (96)
.claude/skills/add-architecture-test/SKILL.md.claude/skills/add-audit-coverage/SKILL.md.claude/skills/add-ef-migration/SKILL.md.claude/skills/add-feature-key/SKILL.md.claude/skills/add-frontend-route/SKILL.md.claude/skills/add-mediatr-handler/SKILL.md.claude/skills/add-permission/SKILL.md.claude/skills/add-provider-adapter/SKILL.md.claude/skills/add-tenant-owned-entity/SKILL.md.claude/skills/code-review/SKILL.md.claude/skills/commit-and-pr/SKILL.md.claude/skills/local-dev-setup/SKILL.md.claude/skills/seed-tenant/SKILL.md.claude/skills/standards-check/SKILL.md.claude/skills/update-glossary/SKILL.md.claude/skills/wire-cross-cutting-foundation/SKILL.md.github/CONTRIBUTING.md.github/workflows/ci.ymlAGENTS.mdCLAUDE.mdREADME.mdbackend/analyzers/LearnStack.Analyzers/DomainExceptionThrowAnalyzer.csbackend/src/LearnStack.SharedKernel/Secrets/ConfigurationSecretProvider.csdocs/architecture/01-platform-vision.mddocs/architecture/02-domain-model.mddocs/architecture/03-module-boundaries.mddocs/architecture/04-technical-architecture.mddocs/architecture/05-mvp-scope.mddocs/architecture/06-extension-model.mddocs/architecture/09-tenant-isolation.mddocs/architecture/12-localization.mddocs/architecture/14-frontend-architecture.mddocs/architecture/15-event-and-outbox.mddocs/architecture/21-feature-flags.mddocs/architecture/23-data-protection.mddocs/architecture/24-learnstack-hub.mddocs/architecture/25-deployment-models.mddocs/architecture/26-hybrid-license-model.mddocs/architecture/27-custom-domain-tls.mddocs/architecture/28-platform-tenant-organization.mddocs/architecture/29-dapr-integration.mddocs/architecture/30-api-gateway.mddocs/architecture/31-audit-subsystem.mddocs/architecture/32-tenant-customization-model.mddocs/architecture/33-cross-cutting-concerns.mddocs/decisions/0003-tenant-isolation-defense-in-depth.mddocs/decisions/0014-adopt-dapr.mddocs/decisions/0015-api-gateway-apisix.mddocs/decisions/0016-audit-log-subsystem.mddocs/decisions/0017-tenant-organization-hierarchy.mddocs/decisions/0018-tenant-driven-customization-model.mddocs/decisions/0019-learnstack-hub.mddocs/decisions/0022-custom-domain-tls.mddocs/decisions/0028-audit-log-partition-management.mddocs/decisions/0030-redis-compatible-store-valkey.mddocs/decisions/0032-exception-handling-logging-and-observability.mddocs/decisions/0033-audit-durability-model.mddocs/decisions/0034-hub-contract-surface-invariant.mddocs/decisions/0035-demand-gated-infrastructure.mddocs/decisions/README.mddocs/glossary.mddocs/roadmap/README.mddocs/roadmap/phase-00-product-architecture.mddocs/roadmap/phase-01-repository-tooling.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/roadmap/phase-02b-events-auth.mddocs/roadmap/phase-02c-hub-foundation.mddocs/roadmap/phase-02d-walking-skeleton.mddocs/roadmap/phase-03-identity-admin.mddocs/roadmap/phase-04-cms-media-pages.mddocs/roadmap/phase-05-education-learning-content.mddocs/roadmap/phase-06-renderer-admin-studio.mddocs/roadmap/phase-07-enrollment-learner-portal.mddocs/roadmap/phase-08a-assessment-notifications.mddocs/roadmap/phase-08b-scheduling.mddocs/roadmap/phase-08c-classroom.mddocs/roadmap/phase-09-billing-integrations-analytics.mddocs/roadmap/phase-09b-hub-billing.mddocs/roadmap/phase-10-english-learning-mvp.mddocs/roadmap/phase-11-production-hardening.mddocs/roadmap/phase-12-hub-marketplace.mddocs/standards/00-principles.mddocs/standards/01-architecture-standards.mddocs/standards/02-backend-coding.mddocs/standards/05-database.mddocs/standards/07-frontend-architecture.mddocs/standards/08-localization.mddocs/standards/11-security.mddocs/standards/13-documentation.mddocs/standards/14-git-workflow.mddocs/standards/18-audit-coverage.mddocs/standards/19-permissions.mddocs/standards/20-infrastructure-stack.mddocs/standards/21-architecture-tests-catalogue.mddocs/standards/README.mdinfra/dapr/README.md
…ject Found by the adversarial pre-PR review, and it is this project's signature failure mode one more time: the wave-1 commit settled the audit model in ADR-0033 and in 31-audit-subsystem § 5, and left four carriers of the superseded draft standing. ADR-0033 § Fail-closed, stated precisely names exactly two failures that reject an operation: one the catalogue does not classify at all, and a MUST-class row that cannot be written durably. A failure to read a tenant *override* is deliberately not one of them — the in-process catalogue still supplies the MUST floor, so nothing proceeds unaudited, and rejecting every request platform-wide because a cache is unavailable is a worse compliance outcome than losing one tenant's voluntary SHOULD→MUST elevation. Four documents said the opposite: - 31-audit-subsystem § 7 told the reader a read failure "rejects the operation rather than defaulting it" — and cited § 5 as its authority, which says the reverse three hundred lines earlier. § 7 is the section someone opens while writing the audit_config loader, so this was the copy most likely to be implemented. - The same file's pipeline annotation read "fails closed on config failure", which reads as endorsing it. - CLAUDE.md carried it as a hard never-do rule. That file is read first by every agent, so in practice it outranks the ADR. - Phase 02a repeated it twice, in the Packet Sequence and in Scope. The same file already records, at § 13, that `Audit_Config_Failure_Rejects_Operation` was **withdrawn** precisely because it "would have locked in a platform-wide denial of service triggered by a cache outage" — so the corpus had already reasoned its way to the right answer and simply failed to sweep the passages that said otherwise. No decision changes here. ADR-0033 and Standards 18 were already correct; this deletes the residue that contradicted them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paired with the Hub repository's merge commit. The Hub side merged P02c-1 after a review found its 221 files implement none of what ADR-0033, ADR-0034 or ADR-0035 changed — no hosted endpoint, no LearnStackApiClient, an audit behavior that is a shell, and an entitlement wire shape that already carries grace_until and generation with the wire names ADR-0034 fixed. This repository's two claims about that state are corrected. The phase banner said the branch "exists and is not merged"; the Risks list carried "the frozen Hub branch rots", mitigated by treating it as "a reference implementation to be re-landed ... not as a branch to merge unchanged". Both now say what is true: P02c-1 shipped, and the freeze applies from P02c-2 onward — which is where the contract surface ADR-0034 redrew actually gets built, and the packet the ADR-0035 trigger genuinely gates. The rot risk is marked discharged rather than deleted, because the reason it was discharged is the reasoning that decided the merge, and the SharedKernel reconciliation against Packet 3b it left behind is real and still grows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**The RLS template was copied back into three files.** This is the defect ADR-0003 Amendment 3 exists to prevent, and CLAUDE.md lists it under Things to never do: the pre-2026-08-08 template lived in four documents and was wrong in all four, two PERMISSIVE policies OR-ed together, leaking every tenant-wide row across tenants. Commit 1e7114c corrected the copies in add-tenant-owned-entity and add-ef-migration instead of removing them, and 12-localization carried a complete org-scoped instance for tenant_template_library. All three now point at docs/standards/05-database.md and carry a checklist of the load-bearing properties instead of the SQL — enough to tell a correct paste from a wrong one without maintaining a second copy. The disclaimer the skills carried is worth quoting, because it is the whole argument against copies: "The block below mirrors it so this skill is executable without a second file open. If the two ever disagree, the standard wins and this skill is the bug." A note predicting the drift does not prevent it. verify.sh gains the guard. It matches `AS RESTRICTIVE FOR` — the DDL form — rather than the prose mention, so a document may still say "plus two AS RESTRICTIVE write guards" while a copy of the policy fails the build. It also asserts no .claude skill contains CREATE POLICY at all. **30-api-gateway claimed the backend rejects unauthenticated requests.** It does not. `grep AddAuthentication|UseAuthentication|JwtBearer|[Authorize]` over backend/src returns two hits and both are comments; `AuthorizationBehavior.Handle` is `return next()` with a Phase 03 TODO. The paragraph used that false claim to downgrade an open gateway from "an open door" to "a defense-in-depth gap" — the most load-bearing sentence in the section, and the one that was wrong. It now says neither layer authenticates today, names the phases that change that, and says why it is survivable: no tenant-owned table and no protected endpoint exists yet. **Architecture 24 dropped an endpoint.** ADR-0034 and Standards 20 both enumerate four LearnStack -> Hub paths; architecture 24 carried three, missing POST /api/v1/internal/tenants/{id}/custom-domains — in the document ADR-0034's Context singles out as the one that previously mis-declared the surface. The endpoint set is a cross-repository contract that changes by decision record in both repositories or not at all, so a third copy silently diverging is the exact failure the ADR was written against. The fourth blocker, the audit_config read-failure contradiction, was fixed in ca1c06e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…and-gating claims
Second pass on the pre-PR review — eleven majors and minors.
**The endpoint set gained a fourth outbound path and three carriers kept
the old count.** Wave 9 added POST /api/v1/internal/tenants/{id}/custom-domains
to ADR-0034 and Standards 20; the glossary still said "six inbound plus
three outbound", architecture 24's scope line still listed three purposes,
and ADR-0024 still said "internal mTLS at four endpoints". The glossary now
states no count at all and points at ADR-0034 — a count in a glossary entry
is a second thing to keep in step, which is the failure this ADR exists to
stop.
**`ALTER TABLE … OWNER TO` was simultaneously required and forbidden.**
ADR-0003 Amendment 3 listed it as DDL belonging in the same migration as
the policies; 05-database said no such statement is needed and that seeing
one means the migration is running as the wrong role. The standard is
right — migrations connect *as* `learnstack_migration`, so every table it
creates is already owned by it. The ADR (restructure-authored, editable
pre-merge) now says the same and cites the standard.
**ADR-0035's LiveKit row promised an exception that was never written.**
Wave 11 changed the cell to "scheduled, not gated — see the exception
below" and the paragraph below never arrived. It exists now, and names both
exceptions to the four-element rule: `audit_log` partitioning is
schema-internal so it has no port, and LiveKit has no default because its
absence is a missing product feature rather than a missing implementation.
Phase 08c stopped quoting the circular trigger ("the classroom phase
begins"), and the glossary stopped listing LiveKit as an ordinary member of
the gated set.
**Three documents claimed implementations that do not exist.**
06-extension-model and 29-dapr-integration both said `InProcessEventBus`
and `InMemoryCacheService` are "registered today". They are not — only
`ConfigurationSecretProvider` shipped, in Packet 3; the other two land with
their ports in Packet 5. This is the same class of claim the wave-7 sweep
corrected in infra/dapr/README and missed in these two.
**ADR-0035 said its defaults are the only registrations "until Phase 11"**
while its own rule ten lines later forbids `NullEntitlementProvider`
outside Development once Phase 02c lands. The sentence now says each
default holds until its own trigger fires, and names the one that differs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third pass on the pre-PR review. **Three checks were described as enforcing rules they do not enforce.** - `docs/standards/README.md` said the integration suite runs in CI. It does not: the backend job filters it out with `FullyQualifiedName!~LearnStack.Tests.Integration`, and its own job is `if: false` until Packet 7 lands the first isolation test. Standards 21 names this exact failure — claiming a rule is enforced when the check is registered but not implemented — so the standards index asserting it was the worst place for it. - `30-api-gateway` said the route-priority invariant "is asserted in CI: the route-table lint fails when…". There is no route-table lint anywhere in the repository. The lint ships with APISIX in Phase 11; the sentence now says so in the future tense and cites Standards 21 for why the distinction matters. - `ci.yml`'s banner comments and placeholder `echo`s still said Phase 03 and Phase 04 while the job names, the file header and CONTRIBUTING all say Phase 02d. Same file, four sites, disagreeing with itself. CONTRIBUTING's deferred-check list also carried the pre-rename job names, which matters more than it looks: GitHub matches required checks **by name**, so a rename that is not mirrored into branch protection silently stops the check being required. That warning is now written where the rename happens. **Packet 3b's own inventory was wrong in three ways.** It said "eight shipped source comments"; there are nine, across four files. It attributed five sites to `CrossCuttingFoundationExtensions.cs` correctly but listed `ConfigurationSecretProvider.cs` as needing repair — the restructure had already fixed that file while sweeping the provider's name, so the packet claimed work that was done. The inventory now gives the per-file counts and says explicitly which file is already corrected and why. **Packet 7 promised an audit row before the audit table exists.** `EnterPlatformAdminScope` was described as writing a `SecurityEvent` row; `audit_log` and `IAuditStore` land in Packet 9, two packets later. Packet 7 now logs at `Warning` with the reason, caller and sentinel tenant id, and Packet 9 replaces that with the durable row. A log line that is honestly a log line beats an audit row that does not exist. **The restructure annotation credited the broken RLS template to the wrong ADR** — ADR-0017 Amendment 1, whose only amendment is a terminology note containing no SQL at all. It was ADR-0003 Amendment 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review caught this restructure breaking the rule it enforces most loudly. CLAUDE.md lists "Edit an Accepted ADR's decision section" first under Things to never do, and this branch did it twice. - **ADR-0019's Decision section** had `learnstack-hub-web` renamed to `operator-portal` in place. The app really was renamed, but a rename is not a decision ADR-0019 made, and its Decision is a record of what was decided in May. Reverted; the supersession banner now carries the rename, including the detail that `learnstack-hub-web` survives as the Keycloak OIDC client id and is a different identifier entirely. - **ADR-0022's 2026-05-19 Option B amendment** had its SaaS / Dedicated bullet rewritten to the post-ADR-0034 mechanism, which left the status banner declaring that bullet superseded while the bullet described the current design. Reverted to the original wording; the banner already says what replaces it. **The withdrawn `SaveChanges` formulation was still in six places**, including CLAUDE.md's never-do list and the glossary's *Durable Audit Intent* entry — the two files agents read first and treat as definitive. ADR-0033 says plainly: *"The same `DbContext.SaveChanges` as the business write" was the wrong formulation and is withdrawn. The guarantee is "the same transaction".* All six now say transaction, and name the two behaviors that make it true. The outbox and inbox uses of `SaveChanges` are left alone — those are correct and are a different mechanism. Also: CLAUDE.md claimed every phase doc carries the same six sections while the roadmap index now declares three exceptions; ADR-0032's Amendment 2 said "Neither changes a sub-decision; both correct text" under a heading reading "Three corrections"; and three pointers into 31-audit-subsystem cited section numbers that shifted when the audit rewrite inserted a new § 1 — § 8 Retention is § 9, and Append-only enforcement is a subsection of § 7 rather than a section of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… copies Final pass on the pre-PR review. **`audit_log`'s DDL still declared `is_success boolean`.** Wave 1 replaced that with a three-valued `outcome` precisely because a boolean cannot express `denied` — and then updated the prose, the domain model and the capture pipeline while leaving the CREATE TABLE behind. The column is now `outcome text NOT NULL` with the three values named, and `reason` joins it, which `EnterPlatformAdminScope(reason)` and every denial already assume. **Two documents described the Phase 11 partition conversion as additive.** 31-audit-subsystem said "Phase 11 adds PARTITION BY RANGE (timestamp)" and `add-ef-migration` shipped a partitioned `audit_log` in its own template. PostgreSQL has no `ALTER TABLE … PARTITION BY`: Phase 11 creates a partitioned parent, attaches the table, and recreates the indexes and the policy under a lock. The skill now ships the unpartitioned table with the composite key that makes that conversion a data operation rather than a key migration, and says why the key looks redundant today. **Three executable copies had drifted from their source.** `add-tenant-owned-entity` still emitted the two-index form — a standalone index on `tenant_id` duplicating the leading column of two UNIQUE constraints, and a partial index on `organization_id` excluding exactly the tenant-wide rows the policy's `IS NULL` branch matches. The standard moved to one composite, non-partial index; the skill now matches and explains the non-partiality. **20-search opened by asserting Meilisearch is what LearnStack uses.** It is demand-gated: the port and a PostgreSQL full-text default ship in Phase 04, the adapter in Phase 09 on its trigger. The document keeps its Meilisearch detail behind a banner saying which parts describe the target and which rule — the tenant predicate composed inside the port — applies to both implementations. Also: an outbox code comment said "RLS bypassed by grant" in a document whose own § explains that `BYPASSRLS` bypasses policies while the GRANT is what *bounds* the bypass; Phase 02c named the persisted column `valid_until` where the wire field is `expires_at`; and the Hub's contract pointer said the Hub tests are "still under to be backfilled" when that section now says every identifier has been folded into the catalogue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/decisions/0035-demand-gated-infrastructure.md (2)
173-181: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake the
ISecretProvidertrigger consistent with its registration plan.The table at Line 87 says that a non-dev deployment itself triggers the Vault adapter. The support claim at Line 109 says SaaS is wired before Phase 11, while the implementation note at Line 177 keeps
ConfigurationSecretProviderregistered until Phase 11.The trigger is already true while the default remains active. Narrow the trigger to a concrete secret-rotation requirement, or move the adapter before the first supported non-dev deployment.
Based on learnings, demand-gated infrastructure must name a port, a working default implementation, an owning phase, and a trigger condition.
🤖 Prompt for 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. In `@docs/decisions/0035-demand-gated-infrastructure.md` around lines 173 - 181, Align the ISecretProvider trigger and registration timeline across the decision document: update the table, support claim, and implementation note so they consistently describe either a concrete secret-rotation trigger while ConfigurationSecretProvider remains active, or move Vault adapter ownership before the first supported non-development deployment. Preserve the required port, working default, owning phase, and trigger condition.Source: Learnings
82-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign APISIX activation with Phase 11.
Phase 03 uses ASP.NET JWT middleware; it only commits the corrected APISIX route configuration. Move
openid-connectactivation and gateway JWT validation to Phase 11, and apply the same phase and trigger in both documents.🤖 Prompt for 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. In `@docs/decisions/0035-demand-gated-infrastructure.md` around lines 82 - 95, Update the APISIX entry in the demand-gated infrastructure decision table so openid-connect activation and gateway JWT validation are assigned to Phase 11, matching the corresponding Phase 11 trigger and configuration in the other document. Keep Phase 03 limited to ASP.NET JWT middleware and corrected APISIX route configuration.Source: Learnings
docs/architecture/30-api-gateway.md (2)
274-275: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
allow_origins_by_regexfor tenant subdomain CORS.If tenant subdomains must call the API, replace
*.learnstack.appwith an anchoredallow_origins_by_regexpattern or exact origins at lines 274, 286, and 305.allow_originsdoes not expand this wildcard form.🤖 Prompt for 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. In `@docs/architecture/30-api-gateway.md` around lines 274 - 275, Update the CORS configurations near the visible allow_origins entries to stop using the unsupported *.learnstack.app wildcard. Use anchored allow_origins_by_regex patterns for tenant subdomains, or list exact origins, consistently at all three referenced configuration blocks.Source: MCP tools
250-256: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd
basic-authto the APISIX plugin inventory. The custompluginslist omitsbasic-auth, but the documented Hangfire route uses it.limit-countis already listed.🤖 Prompt for 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. In `@docs/architecture/30-api-gateway.md` around lines 250 - 256, Add basic-auth to the documented APISIX plugin inventory, alongside the existing limit-count entry, so it matches the plugin used by the Hangfire route configuration.Source: MCP tools
.github/CONTRIBUTING.md (1)
54-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument runtime-specific commit attribution.
This example hard-codes
Claude Opus 5as the contributor identity. State the exact trailer forClaude CodeandOpenAI Codex, and require one trailer for each materially contributing agent.As per coding guidelines, AI-assisted commits carry the specified
Co-Authored-Bytrailer. Based on learnings, the trailer identifies the contributing runtime and includes one trailer per materially contributing agent.🤖 Prompt for 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. In @.github/CONTRIBUTING.md around lines 54 - 55, Update the commit attribution guidance around the Co-Authored-By example to document the exact trailers for Claude Code and OpenAI Codex instead of hard-coding Claude Opus 5. Require one trailer for each materially contributing agent, identifying the runtime used.Sources: Coding guidelines, Learnings
.claude/skills/add-ef-migration/SKILL.md (1)
228-251: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the migration role for tenant-aware data migrations.
Use
learnstack_migration(NOBYPASSRLS) withFORCE ROW LEVEL SECURITYon the target table. Update the example’slearnstack_appstatement to match the database standard. KeepBEGIN,set_config('app.tenant_id', ..., true),UPDATE, andCOMMITin that order.🤖 Prompt for 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. In @.claude/skills/add-ef-migration/SKILL.md around lines 228 - 251, Update the tenant-aware migration example around the transaction and ExecuteAsync calls to use the learnstack_migration role, with FORCE ROW LEVEL SECURITY enabled on the target table. Preserve the required BEGIN, set_config('app.tenant_id', ..., true), UPDATE, and COMMIT order, and remove the learnstack_app reference.
🤖 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 @.claude/skills/add-ef-migration/SKILL.md:
- Around line 122-127: The checklist in the migration guidance must distinguish
tenant-only tables from [OrganizationScoped] tables. Define a policy containing
only the tenant predicate for tenant-only tables, and require the organization
predicate plus FOR UPDATE and FOR DELETE restrictive guards only for
[OrganizationScoped] tables; retain the existing RLS, FORCE RLS, explicit WITH
CHECK, and NULLIF GUC requirements.
- Around line 162-165: Update the audit_log table definition in the migration
example to use the column name timestamp instead of occurred_at, and change
audit_log_pkey to PRIMARY KEY (id, timestamp). Keep the surrounding audit
architecture, indexes, and partitioning guidance unchanged.
In @.github/CONTRIBUTING.md:
- Around line 32-34: Update the contributing guidance around activating deferred
jobs to explicitly require changing the job’s name in .github/workflows/ci.yml,
the corresponding required-check list, and the live branch-protection setting
together; do not imply that changing only the if condition is sufficient.
In @.github/workflows/ci.yml:
- Line 168: Replace the constant false conditions for the OpenAPI diff and
Lighthouse budget jobs with GitHub Actions expressions checking
vars.ENABLE_OPENAPI_DIFF and vars.ENABLE_LIGHTHOUSE_BUDGET equal to 'true',
respectively, preserving their disabled-by-default behavior when unset. Also
update the constant condition near the workflow’s existing Line 112 equivalent
if required for actionlint to pass.
---
Outside diff comments:
In @.claude/skills/add-ef-migration/SKILL.md:
- Around line 228-251: Update the tenant-aware migration example around the
transaction and ExecuteAsync calls to use the learnstack_migration role, with
FORCE ROW LEVEL SECURITY enabled on the target table. Preserve the required
BEGIN, set_config('app.tenant_id', ..., true), UPDATE, and COMMIT order, and
remove the learnstack_app reference.
In @.github/CONTRIBUTING.md:
- Around line 54-55: Update the commit attribution guidance around the
Co-Authored-By example to document the exact trailers for Claude Code and OpenAI
Codex instead of hard-coding Claude Opus 5. Require one trailer for each
materially contributing agent, identifying the runtime used.
In `@docs/architecture/30-api-gateway.md`:
- Around line 274-275: Update the CORS configurations near the visible
allow_origins entries to stop using the unsupported *.learnstack.app wildcard.
Use anchored allow_origins_by_regex patterns for tenant subdomains, or list
exact origins, consistently at all three referenced configuration blocks.
- Around line 250-256: Add basic-auth to the documented APISIX plugin inventory,
alongside the existing limit-count entry, so it matches the plugin used by the
Hangfire route configuration.
In `@docs/decisions/0035-demand-gated-infrastructure.md`:
- Around line 173-181: Align the ISecretProvider trigger and registration
timeline across the decision document: update the table, support claim, and
implementation note so they consistently describe either a concrete
secret-rotation trigger while ConfigurationSecretProvider remains active, or
move Vault adapter ownership before the first supported non-development
deployment. Preserve the required port, working default, owning phase, and
trigger condition.
- Around line 82-95: Update the APISIX entry in the demand-gated infrastructure
decision table so openid-connect activation and gateway JWT validation are
assigned to Phase 11, matching the corresponding Phase 11 trigger and
configuration in the other document. Keep Phase 03 limited to ASP.NET JWT
middleware and corrected APISIX route configuration.
🪄 Autofix
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: 6ae5455a-cf06-4b0d-8c71-325579fc2733
📒 Files selected for processing (27)
.claude/skills/add-ef-migration/SKILL.md.claude/skills/add-tenant-owned-entity/SKILL.md.github/CONTRIBUTING.md.github/workflows/ci.ymlCLAUDE.mddocs/architecture/06-extension-model.mddocs/architecture/12-localization.mddocs/architecture/15-event-and-outbox.mddocs/architecture/20-search.mddocs/architecture/24-learnstack-hub.mddocs/architecture/29-dapr-integration.mddocs/architecture/30-api-gateway.mddocs/architecture/31-audit-subsystem.mddocs/decisions/0003-tenant-isolation-defense-in-depth.mddocs/decisions/0019-learnstack-hub.mddocs/decisions/0022-custom-domain-tls.mddocs/decisions/0024-api-versioning-policy.mddocs/decisions/0028-audit-log-partition-management.mddocs/decisions/0032-exception-handling-logging-and-observability.mddocs/decisions/0035-demand-gated-infrastructure.mddocs/glossary.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/roadmap/phase-02c-hub-foundation.mddocs/roadmap/phase-03-identity-admin.mddocs/roadmap/phase-08c-classroom.mddocs/standards/18-audit-coverage.mddocs/standards/README.md
🚧 Files skipped from review as they are similar to previous changes (18)
- docs/architecture/12-localization.md
- docs/decisions/0032-exception-handling-logging-and-observability.md
- docs/architecture/06-extension-model.md
- docs/standards/README.md
- docs/standards/18-audit-coverage.md
- docs/decisions/0028-audit-log-partition-management.md
- docs/decisions/0022-custom-domain-tls.md
- docs/architecture/24-learnstack-hub.md
- docs/architecture/29-dapr-integration.md
- CLAUDE.md
- docs/glossary.md
- docs/roadmap/phase-03-identity-admin.md
- docs/architecture/15-event-and-outbox.md
- docs/roadmap/phase-02c-hub-foundation.md
- docs/decisions/0003-tenant-isolation-defense-in-depth.md
- docs/roadmap/phase-08c-classroom.md
- .claude/skills/add-tenant-owned-entity/SKILL.md
- docs/architecture/31-audit-subsystem.md
Each verified against the current tree first; two were rejected and are listed at the end with reasons. **`add-ef-migration` had drifted from the standard in four ways**, and it is an executable skill, so each one ships into a real migration: - Its `audit_log` example used `occurred_at` and keyed on `(id, occurred_at)`. The canonical DDL in ADR-0033 and 31-audit-subsystem uses `timestamp`. `occurred_at` is `outbox_messages`' column, so the two were genuinely swapped; the note now says which is which. - It still emitted the two-index form — a standalone index on `tenant_id` duplicating the leading column of the UNIQUE constraints, plus a partial index on `organization_id` that excludes exactly the tenant-wide rows the policy's `IS NULL` branch matches. Corrected to the standard's single composite, non-partial index. (The sibling skill was fixed in b28763c; this one was missed — same carrier, one file over.) - Its RLS checklist described one shape for all tables. A tenant-only table takes a policy with the tenant predicate alone and **no** restrictive guards — there is no second scope to widen. The checklist now splits the two cases and says which properties are common to both. - Step 7's data-migration example said the connection runs as `learnstack_app`. 05-database says per-tenant data migrations run as `learnstack_migration`, and the consequence is worth stating: that role is `NOBYPASSRLS` against a `FORCE ROW LEVEL SECURITY` table, so skipping `set_config` matches zero rows **and still reports success**. It also still carried "this is a mirror; if they disagree the standard wins" after f3e2628 removed the mirror it described. **Three CORS blocks used `*.learnstack.app` as an `allow_origins` value.** APISIX's `cors` plugin takes exact origins or `**`; there is no `*.domain` form, so the pattern matches nothing and silently blocks every tenant subdomain — a functional defect, not a style one. All three now use anchored `allow_origins_by_regex`. The wildcard at the TLS-certificate SAN is left alone, where it is correct. **`basic-auth` was used by the Hangfire route but absent from both plugin inventories.** APISIX rejects a route referencing a plugin the deployment does not declare, so the documented config would not load. **`openid-connect` activation was assigned to Phase 03** while ADR-0035 demand-gates the whole gateway to Phase 11. Phase 03 keeps authorization in ASP.NET middleware and corrects the route table; it does not stand APISIX up. **The Vault trigger was already satisfied by its own wording.** It read "or a non-dev deployment exists", and the same ADR says `SaaS` is wired end to end before Phase 11 — so the trigger fired the moment SaaS shipped, while the row said Phase 11. Rewritten to the conditions that actually matter, in all three carriers, with the bounded position stated plainly: reading secrets from `IConfiguration` is defensible while one operator holds them and a redeploy is an acceptable way to change them. **CONTRIBUTING** now says activating a deferred job is three edits in one PR — guard, job name, and both the list and the live branch-protection setting — rather than implying the guard alone. Not applied, with reasons: - **`if: false` → `vars.ENABLE_*` gating.** This is a design change, not a defect. Repo-variable gating lets a required check be switched off from Settings with no PR and no review, which is strictly worse for a check that gates `main`; and it does not address the real hazard, which is the rename. `actionlint` does not object to `if: false`. - **Documenting per-assistant `Co-Authored-By` trailers.** CLAUDE.md § Commit conventions is the single source for the trailer and specifies one exact form; CONTRIBUTING already defers to it and says to substitute the model. Enumerating other runtimes here would fork the convention into two places, which is the duplication the corpus is being cleaned of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/standards/20-infrastructure-stack.md (2)
11-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCite the audit durability and partition-management ADRs.
audit_logpartitioning and retention are governed by ADR-0028. ADR-0033 governs the audit durability contract and delegates partition lifecycle to ADR-0028. Add both ADRs to the standard’sDerives fromandReferenceslists, or document this ownership boundary explicitly.🤖 Prompt for 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. In `@docs/standards/20-infrastructure-stack.md` around lines 11 - 13, Update the infrastructure standard’s ADR references to include ADR-0028 for audit_log partitioning and retention and ADR-0033 for the audit durability contract. Add both to the appropriate “Derives from” and “References” lists, or explicitly document that ADR-0033 owns durability while ADR-0028 owns partition lifecycle.Source: Coding guidelines
298-301: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconcile the mTLS termination point and failure response.
The repository gives two incompatible topologies: the security and infrastructure standards place
/api/internal/*behind APISIX mTLS, while the API gateway architecture says the internal listener bypasses APISIX. Define one topology. For direct TLS termination, document handshake rejection without an HTTP response. If APISIX terminates mTLS, document its401mapping and reserve application-level401responses for JWT and HMAC failures.🤖 Prompt for 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. In `@docs/standards/20-infrastructure-stack.md` around lines 298 - 301, Reconcile the `/api/internal/*` topology across the infrastructure and security standards with the API gateway architecture by choosing either direct TLS termination or APISIX termination. Update the endpoint authentication and failure-response language accordingly: direct termination must specify TLS handshake rejection without an HTTP response, while APISIX termination must specify its `401` mapping and reserve application-level `401` responses for JWT and HMAC failures.
🧹 Nitpick comments (1)
docs/standards/20-infrastructure-stack.md (1)
136-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep automated topic validation for
InProcessEventBus.
InProcessEventBusis registered today at Line 42, and the topic convention applies to it. DeferringDapr_PubSub_TopicNames_FollowConventionuntil Phase 11 leaves the active transport with reviewer-only enforcement.Add a transport-independent test now, or add a test that covers
InProcessEventBusand retain the Dapr-specific test for Phase 11.🤖 Prompt for 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. In `@docs/standards/20-infrastructure-stack.md` around lines 136 - 141, Add automated topic-name convention validation covering the currently registered InProcessEventBus, using a transport-independent test or an InProcessEventBus-specific test. Keep Dapr_PubSub_TopicNames_FollowConvention scheduled for Phase 11 while ensuring the active transport is validated now.
🤖 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/standards/20-infrastructure-stack.md`:
- Line 45: Update the infrastructure standards entry around SelectSecretProvider
to explicitly state that Development and SaaS use startup-only configuration
values, including the Sentry DSN, until the Vault adapter is implemented;
alternatively document the required configuration reload and options-rebinding
path if refresh is intended before then.
---
Outside diff comments:
In `@docs/standards/20-infrastructure-stack.md`:
- Around line 11-13: Update the infrastructure standard’s ADR references to
include ADR-0028 for audit_log partitioning and retention and ADR-0033 for the
audit durability contract. Add both to the appropriate “Derives from” and
“References” lists, or explicitly document that ADR-0033 owns durability while
ADR-0028 owns partition lifecycle.
- Around line 298-301: Reconcile the `/api/internal/*` topology across the
infrastructure and security standards with the API gateway architecture by
choosing either direct TLS termination or APISIX termination. Update the
endpoint authentication and failure-response language accordingly: direct
termination must specify TLS handshake rejection without an HTTP response, while
APISIX termination must specify its `401` mapping and reserve application-level
`401` responses for JWT and HMAC failures.
---
Nitpick comments:
In `@docs/standards/20-infrastructure-stack.md`:
- Around line 136-141: Add automated topic-name convention validation covering
the currently registered InProcessEventBus, using a transport-independent test
or an InProcessEventBus-specific test. Keep
Dapr_PubSub_TopicNames_FollowConvention scheduled for Phase 11 while ensuring
the active transport is validated now.
🪄 Autofix
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: 688687f6-5a6f-4ccb-9b72-6149691bb3db
📒 Files selected for processing (6)
.claude/skills/add-ef-migration/SKILL.md.github/CONTRIBUTING.mddocs/architecture/30-api-gateway.mddocs/decisions/0014-adopt-dapr.mddocs/decisions/0035-demand-gated-infrastructure.mddocs/standards/20-infrastructure-stack.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/decisions/0035-demand-gated-infrastructure.md
- .github/CONTRIBUTING.md
- .claude/skills/add-ef-migration/SKILL.md
- docs/decisions/0014-adopt-dapr.md
- docs/architecture/30-api-gateway.md
…alse Reversing a call I made in 37f959c, because the reason I gave for it was wrong. I skipped this finding partly on the claim that "actionlint does not object to `if: false`". It does — three times: ci.yml:112:9: constant expression "false" in condition. remove the if: section [if-cond] I had no way to check that when I wrote it and asserted it anyway. This repository does not run actionlint in CI, which is why three lint errors have sat in the workflow since Phase 01 with nothing to report them. My other objection — that a `vars.` gate lets a required check be switched off from Settings without a PR — does not survive contact with the actual branch protection. The required contexts are backend, frontend, meta and secret scan. None of the three deferred jobs is among them, so there is no required check to disable. And the failure direction is safe anyway: a required job skipped by an unset variable never reports, so the PR blocks rather than merging green. All three now gate on `vars.ENABLE_*`, unset by default — an unset variable is the empty string, so the condition is false until someone sets it to 'true'. actionlint is clean. The activation procedure grew a step and CONTRIBUTING says so: set the variable, replace the placeholder step, rename the job, and add the new name to both the required-check list and the live branch-protection setting. Setting the variable alone leaves a job that runs and gates nothing. Three live documents that described the old mechanism are updated. Phase 01's delivery record still says `if: false` and stays that way — it is an accurate record of what Phase 01 shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
phase-02c said 'the Hub-side P02c-1 branch unfreezes with it'. P02c-1 merged on 2026-08-09 and is neither a branch nor frozen — the same file's status block says so. The frozen track resumes at P02c-2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…no refresh yet Four findings on Standards 20, each verified against the tree first. **`401` was promised for an mTLS failure that can never produce one.** Standards 20 and Standards 11 both said "failure of any returns `401` with no detail leak" for the three-layer chain. `/api/internal/*` is bound to its own mTLS listener and is never proxied by APISIX (30-api-gateway § Topology; APISIX returns 404 for that path). A missing, expired or untrusted client certificate is therefore rejected during the TLS handshake — no HTTP request reaches the application, so there is no status code and no body. Only the JWT and HMAC checks can return `401`. Both carriers now say so, and Standards 11 adds the consequence: a handler returning `401` for a certificate failure is unreachable, and writing one implies a listener that terminates TLS without requiring the client cert. **The secret path has no refresh mechanism, and the standard implied it did.** It described startup binding plus "runtime re-fetches via `IOptionsMonitor<T>` with a Vault-driven refresh". Both `Development` and `SaaS` run on `ConfigurationSecretProvider`, which resolves through `IConfiguration` at composition time — including the Sentry DSN, which `ErrorTrackingRegistration` reads once during registration. No reload, no rebinding: every secret is fixed for the process lifetime and changing one is a redeploy. Stated plainly, because it is what makes the Vault trigger legible — "must rotate without a redeploy" is a condition this arrangement cannot satisfy by construction. **The topic-name convention was unasserted against the transport that is actually registered.** The standard says the convention applies to `InProcessEventBus` too, then deferred its only test to Phase 11 with the Dapr adapter — leaving the live transport reviewer-enforced, which is the gap the catalogue exists to close. Split in two: `Integration_Event_TopicNames_FollowConvention` is transport-independent, reads the event declarations rather than a broker, and lands with `InProcessEventBus` in Packet 5; the Dapr test keeps Phase 11 and narrows to what only it can check — that the component bindings agree with the declared topics. **Two audit ADRs were referenced without being placed.** Standards 20 records that `audit_log` partitioning is demand-gated and that modules never write `audit_log` directly, which are ADR-0028's and ADR-0033's territory. Both are now named in the header with the ownership split spelled out — ADR-0033 owns the durability contract, ADR-0028 owns the partition lifecycle — because those two are the easy pair to conflate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The LearnStack half of a two-repository roadmap restructure.
Paired PR: HodeTech/LearnStack-Hub#2 at
c2db5e80a7f82f8dd0fa36e55f340d5300756a65— opened first, perthat repo's cross-repo protocol. Both merge in the same session; either side
alone leaves the contract dangling.
Documentation only. One source comment and one
.csXML doc changed; nobehaviour, no schema, no migration.
Why
Four independent audits of the corpus, then a restructure of everything not yet
shipped, then a verification pass that produced 151 findings and closed all of
them. Completed phases and the packets inside them were not touched: where a
finding affected shipped work, it became declared work in a later packet rather
than a rewrite of history.
The audit found one failure mode running through the whole corpus, and it is the
thing this PR is really about: the corpus kept fixing the document it opened
and leaving the untouched carriers of the same fact stale. The clearest case is
the Row Level Security template — it lived in four documents and was wrong in all
four, in the same way, because each was corrected in isolation. Three findings in
this PR under-counted their own carriers by half or more; one named three and had
six.
The three things that would have shipped broken
Every tenant-wide row was visible to every tenant. The published RLS template
declared two permissive policies — one for tenant, one for organization.
PostgreSQL combines permissive policies with
OR, so a row withorganization_id IS NULLsatisfied the organization half on its own and becamereadable across tenants. One
AND-ed policy now, plus twoAS RESTRICTIVEwrite guards, because
USINGis the only gate aDELETEhas and is also whatselects the rows an
UPDATEmay target.ENABLEandFORCE, anexplicit
WITH CHECK, andNULLIF(current_setting(…), '')throughout — acustomized GUC's reset value is the empty string, not undefined, and
''::uuidraises instead of filtering on a pooled connection.ADR-0003 Amendment 3;
canonical SQL in exactly one file,
05-database.md.
Audit rows were lost on rollback. The model wrote MUST-class audit in
SaveChangesand marked the intent consumed — butTransactionBehaviorcommits later, and every business-rule rejection that returns
Result.Failrolls back. The audit row went with it while the flag survived, so both the state
change and its record vanished.
ADR-0033 settles it on the
transaction: decide at step 3, write on the ambient transaction immediately
before
COMMIT, reconcile after rollback. Atomicity comes from the transaction,not from
SaveChanges— which also dissolves the cross-DbContextproblem.The first migration could not run. "Every one of these tables is created with
the corrected template" is not implementable:
tenantshas notenant_idcolumn, and
platform_host_to_tenantis read in order to determine thetenant, so a tenant-keyed predicate returns zero rows and no tenant ever resolves.
Three table classes now, with the test for the third stated. And the four-role
model could not be provisioned at all — since PostgreSQL 15 the
publicschemagrants
CREATEto nobody but its owner, sodotnet ef database updatefailswith
permission denied for schema public, and the obvious fix (make the role asuperuser) reinstates exactly the ownership arrangement
FORCE ROW LEVEL SECURITYexists to defeat.What else changed
proper superset of the translation table's primary key, so it could reject no
row the table would otherwise accept — two courses in one tenant could both
hold
/en/courses/beginner. The tempting repair, addingorganization_id,makes it worse: a nullable column in a standard
UNIQUEdoes not constrainthe rows where it is null, which is most of them.
24-learnstack-hub.mdstated twodifferent schemes for the same three endpoints, twenty lines apart, the second
under a heading reading "applies to every endpoint above". Resolved toward the
chain; the cost — a client certificate for
SelfHostedOnline— is paid for inthe paired Hub PR.
Entitlementmeant two things. The glossary reserved it for theHub-authored, tenant-subject projection; Phase 07 still owned an
Entitlementaggregate keyed on a user, twelve lines below the Hub's inthe same domain-model diagram. Renamed to
CourseAccessby enumerated lineedits, never a global replace —
entitlementis correct in roughly sixtyother places.
03-module-boundarieslisted
IHostToTenantResolveras a sanctioned Hub caller. Host resolutionreads
platform_host_to_tenantand nothing else, or a Hub outage becomes atenant-marketing-site outage.
handlers and migrations into assemblies nobody scaffolds. Each now opens its
Deliverables with the scaffold bullet, in the shape Phase 01 used.
Polly and Sentry among libraries the backend cannot call (all three are wired),
claimed six ports already exist (two do), and cited an ADR-0014 § 9 that does
not exist.
shipped Packet 0–3 records moved verbatim to a
## Delivery Recordsection —verified byte-identical, 410 lines in, 410 out — and the seven unshipped packets
became a
## Packet Sequencebefore## Scope.two-tenant walking skeleton: the first milestone whose output someone who does
not read C# can evaluate.
New decision records
ADR-0033 Audit Durability Model ·
ADR-0034 Hub Contract Surface Invariant ·
ADR-0035 Demand-Gated Infrastructure
ADR-0034 replaces "closed at four endpoints" — a rule that was never true, and
that had already damaged the design: ADR-0022 routed TLS private keys through
the entitlement payload specifically to avoid declaring a fifth endpoint. Two
enforceable invariants instead.
ADR-0035 is the one-way-door test: if I add this in six months, will I have to
touch code that is already written? Irreversible now, additive on a named
trigger with a port, a default, an owning phase and a written condition.
Verification
Nine checks after every wave, across both repositories, returning zero each time —
link resolution (case-sensitive, because macOS APFS hides the casing bug that
breaks Linux CI), frozen-record byte-identity, section conformance, mermaid,
Markdown tables, no committed file citing gitignored
docs/analysis/, retiredclaims, sibling-path casing, balanced code fences.
Three of those were added during execution, each after something got through:
a dropped closing fence passed all eight prior checks; a retired claim's carriers
were under-counted a fourth time; and the frozen-record check compared line
numbers against a hard-coded boundary the Phase 02a move erased.
Review notes
records. The dated 2026-08-08 annotation blocks added to them are in scope
and are the only change;
git show 5df5ca6:docs/roadmap/phase-02a-kernel-tenancy.mdagainst HEAD confirms no record line was reworded.
supersession banners this restructure authored. ADR-0014 and ADR-0015 keep
their vendor decisions; only the schedule moved.
wave had written the same section better — one of them (
08-localization.md)would have reintroduced the nullable-
UNIQUEdefect this PR fixes.ICacheService.RemoveByPrefixAsyncleaves theport or becomes a generation-key pattern. ADR-0035 permits both and Packet 5
owns it; closing it here would be a design decision outside this scope.
🤖 Generated with Claude Code
Summary by Sourcery
Restructure and update the Phase 02a kernel-tenancy roadmap and related standards/architecture docs around the corrected RLS template, audit durability, demand-gated infrastructure, and the one-way-door test, ensuring tenant isolation, Hub contract, entitlement, and marketplace guidance are accurate, and that the roadmap sequencing and phase responsibilities reflect the latest decisions.
Enhancements:
audit_logpartitioning, and signed licence keys to Phase 11 with explicit triggers while keeping ports and defaults in Phase 02a.Documentation:
Summary by CodeRabbit