docs: archive the finished backlogs (RD-30)
Two backlog trees are complete: `docs/project/backlog/` (75 files, every WP done) and `docs/project/refactor-backlog-setup/` (the arc before it). Move both under `docs/project/archive/` with `git mv`, so history stays intact through `git log --follow`. `SHOWCASE-ROADMAP.md` moves with them, because it points at the now-archived backlog README. Add `docs/project/archive/README.md`. It states that these trees are historical and names the two directories that are still live. Repoint every inbound reference named in RD-30's Files table: CLAUDE.md, the root README, both backend READMEs, `LetterHtml.cs`, `a11y.mdx`, the `document-feature` and `new-ssp` skills, and the readable-codebase PLAN, README, and RD-19 ticket. Fix two upward-relative links inside the moved WP files (WP-68, WP-69) that gained a directory level and would otherwise break. Repoint `.prettierignore`'s two agent-prompt exclusions to their new path, so prettier keeps leaving those files' exact wording alone. Mark RD-30 done and check off its acceptance criteria; flip its README row to done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+69
@@ -0,0 +1,69 @@
|
||||
# ADR-C-001 — rewrite ADR-0001's worked example against the shipped system
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `06-adr-conformance.md` §ADR-C-001
|
||||
|
||||
## What was wrong
|
||||
|
||||
ADR-0001's §"Worked example in this POC" opened with _"This POC has no real backend (static
|
||||
mock JSON + fake submit timers), so the 'BFF output' is a static file"_. That premise is
|
||||
false and every path the section cited was gone. The decision itself was intact; only the
|
||||
description had drifted.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------- | --------------------------------------------------------------------------- |
|
||||
| `docs/reference/architecture/0001-...md` §Worked example | rewritten against `backend/src/BigRegister.Api`; all six paths repointed |
|
||||
| same file, §Out of scope here | 4 bullets → 2, plus a paragraph recording which two were discharged and why |
|
||||
|
||||
No code changed. No CLAUDE.md edit was required for this finding.
|
||||
|
||||
## Paths corrected, each verified
|
||||
|
||||
| Claimed | Actual |
|
||||
| -------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| "no real backend … static file" | `backend/src/BigRegister.Api`, `var api = app.MapGroup("/api/v1")` at `Program.cs:168` |
|
||||
| `public/mock/dashboard-view.json` | `GET /api/v1/dashboard-view` (`Program.cs:172`) |
|
||||
| `public/mock/intake-policy.json` | `GET /api/v1/intake/policy` (`Program.cs:193`) |
|
||||
| `src/app/registratie/contracts/dashboard-view.dto.ts` | `apps/ssp/src/app/registratie/contracts/dashboard-view.dto.ts` |
|
||||
| `src/app/registratie/infrastructure/dashboard-view.adapter.ts` | `apps/ssp/.../infrastructure/dashboard-view.adapter.ts`, `parseDashboardView` at `:50` |
|
||||
| `src/app/herregistratie/contracts/intake-policy.dto.ts` | **deleted** — the DTO is now the generated `IntakePolicyDto`; the adapter is `apps/ssp/src/app/herregistratie/infrastructure/intake-policy.adapter.ts` |
|
||||
|
||||
`apps/ssp/public/mock/` does not exist (`ls`: no such directory).
|
||||
|
||||
## The finding was wrong about one out-of-scope bullet
|
||||
|
||||
ADR-C-001 said to _"reduce §Out of scope to the two items still genuinely open (the
|
||||
`BigProfileStore` optimistic-update race, and session persistence / multi-tab sync)"_,
|
||||
carrying the original bullet's parenthetical **"`SessionStore` is in-memory"**. That
|
||||
parenthetical is no longer true, so the bullet could not be kept verbatim.
|
||||
|
||||
- `apps/ssp/src/app/auth/application/session.store.ts:13` reads
|
||||
`parseStoredPrincipal(localStorage.getItem(STORAGE_KEY))`, and `:41` writes it back.
|
||||
Session persistence **has landed** (RB-10 extracted the parser, RB-13 renamed it
|
||||
`parseStoredPrincipal`). The file even carries a `ponytail:` note explaining the choice of
|
||||
`localStorage` over `sessionStorage`.
|
||||
- Multi-tab sync has **not** landed: `grep` for a `storage` event listener across `apps` and
|
||||
`libs` returns nothing.
|
||||
|
||||
The bullet was therefore narrowed to multi-tab sync only, and states that the session itself
|
||||
now persists. Recording this because the finding, taken literally, would have re-asserted a
|
||||
false claim in the same edit that removed two others.
|
||||
|
||||
The other two survivors were verified rather than assumed: `BigProfileStore` still holds
|
||||
`pending` as a bare `signal(false)` with `begin`/`confirm`/`rollback` mutating it
|
||||
(`big-profile.store.ts:61-74`), so the concurrent-submit race is real.
|
||||
|
||||
## Discharged bullets, both verified
|
||||
|
||||
- _"Runtime DTO validation on **every** endpoint (only the dashboard view has it)"_ — 33
|
||||
distinct `export function parse*` boundary functions exist across `apps` and `libs`.
|
||||
- _"Real OpenAPI/TypeSpec codegen toolchain"_ — `npm run gen:api` (`package.json:12`) runs
|
||||
`dotnet swagger tofile` then `nswag run`, emitting
|
||||
`libs/shared/src/infrastructure/api-client.ts` (2329 lines). CI's `api-client-drift` job
|
||||
regenerates and runs `git diff --exit-code` (`.github/workflows/ci.yml:319-321`).
|
||||
|
||||
## Scope discipline
|
||||
|
||||
Descriptive drift only, as the finding states. The decision, the options table, the two
|
||||
policy shapes and the migration sequence are untouched.
|
||||
+64
@@ -0,0 +1,64 @@
|
||||
# ADR-C-003 — state that the generated client is the wire contract
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `06-adr-conformance.md` §ADR-C-003
|
||||
|
||||
## What was wrong
|
||||
|
||||
ADR-0001 set "one source of truth that generates types for both sides" as the target state.
|
||||
The code reached it. CLAUDE.md §4 still stated the pre-codegen rule — _"DTO lives in
|
||||
`contracts/`"_ — as standing law, so §4 could be cited to justify both deleting the four
|
||||
survivors and adding new hand-written DTOs for already-generated endpoints.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------- | ------------------------------------------------------------------------------------------------------ |
|
||||
| `docs/reference/architecture/0001-...md` | **new** §"Where the contract lives, after codegen" |
|
||||
| `CLAUDE.md` §4 | the flat "DTO lives in `contracts/`" rule replaced with the generated-client rule + the two exceptions |
|
||||
|
||||
No code changed. Per CLAUDE.md's own precedence rule, the ADR was amended first and
|
||||
CLAUDE.md corrected to match, in one diff.
|
||||
|
||||
## The decision the finding asked for: the four survivors stay
|
||||
|
||||
ADR-C-003 required an explicit, recorded decision on the four remaining hand-written DTOs.
|
||||
**They stay**, all four under exception case 2 ("the generator types the shape too loosely").
|
||||
This is not a preference — adopting the generated shapes would violate CLAUDE.md §3.
|
||||
|
||||
Evidence. NSwag emits every property as optional, and flattens a discriminated union into a
|
||||
bag of optional fields:
|
||||
|
||||
| | generated (`api-client.ts`) | hand-written (`dashboard-view.dto.ts`) |
|
||||
| ----------------------- | ---------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- |
|
||||
| `DashboardViewDto` | `registration?`, `person?`, `decisions?` — all optional (`:2017`) | all three required |
|
||||
| `RegistrationDto` | six optional fields (`:2202`) | six required fields |
|
||||
| `RegistrationStatusDto` | **one flat record of five optional strings**, `tag?: string` (`:2211`) | a real union of three variants, `tag: 'Geregistreerd' \| 'Geschorst' \| 'Doorgehaald'`, per-variant fields required |
|
||||
|
||||
The generated `RegistrationStatusDto` makes `{ tag: 'Geregistreerd', doorgehaaldOp: '…' }`
|
||||
representable. That is precisely the illegal state CLAUDE.md §3 exists to forbid, and the
|
||||
`parse*` boundary would have to reconstruct the union by hand anyway.
|
||||
|
||||
The ADR therefore records that retiring these four is **not** a cleanup to schedule. It
|
||||
becomes correct only if the backend annotates its DTOs so the generator emits required
|
||||
properties and real unions — which names the actual prerequisite instead of leaving the
|
||||
question open.
|
||||
|
||||
## Verified counts, not carried over from the finding
|
||||
|
||||
- Hand-written `contracts/*.dto.ts`: **4** —
|
||||
`apps/ssp/src/app/registratie/contracts/{brp-address,dashboard-view,duo-diplomas}.dto.ts`
|
||||
and `libs/beheer/src/contracts/stamdata.dto.ts`.
|
||||
- All four duplicate generated types **by the same names**: `BrpAddressDto` (`:1997`),
|
||||
`DashboardViewDto` (`:2017`), `DuoLookupDto` (`:2056`), `DuoDiplomaDto` (`:2047`),
|
||||
`PolicyQuestionDto` (`:2168`), `ManualDiplomaPolicyDto` (`:2106`), `StamdataColumnDto`
|
||||
(`:2246`), `StamdataTableDto` (`:2253`), `StamdataTableSummaryDto` (`:2261`). None is a
|
||||
codegen gap — the finding's "case 1" has no occupant today, which is worth knowing.
|
||||
- The `parse*` boundary is restated as mandatory regardless of type provenance. The amendment
|
||||
says why in one line: a generated type is a compile-time claim about the wire, not a
|
||||
runtime guarantee.
|
||||
|
||||
## Gate released
|
||||
|
||||
ADR-C-003 blocked any ticket that would delete the four `contracts/*.dto.ts` files or add a
|
||||
hand-written DTO for a generated endpoint. No open ticket needed it. The rule is now written
|
||||
down, so a future one can be judged against it rather than against a stale §4.
|
||||
+58
@@ -0,0 +1,58 @@
|
||||
# ADR-C-006 — extract the actor-agnostic route guards to `libs/shared`
|
||||
|
||||
Status: **implemented** · 2026-08-26 · Source finding: `06-adr-conformance.md` §ADR-C-006
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ----------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------- |
|
||||
| `libs/shared/src/application/auth.guard.ts` | **new** — `authGuard` + `capabilityGuard`, injecting `SESSION_PORT` instead of an app-local `SessionStore` |
|
||||
| `libs/shared/src/application/auth.guard.spec.ts` | **new** — the single spec, provides `SESSION_PORT`; one case added asserting the guard reads the port |
|
||||
| `libs/shared/src/application/session.port.ts` | widened by one member: `readonly isAuthenticated: Signal<boolean>` |
|
||||
| `apps/ssp/src/app/auth/auth.guard.ts` | now a re-export |
|
||||
| `apps/behandelportal/src/app/auth/auth.guard.ts` | now a re-export |
|
||||
| `apps/{ssp,behandelportal}/src/app/auth/auth.guard.spec.ts` | **deleted** — both were byte-identical to the new shared spec |
|
||||
|
||||
Neither `app.routes.ts` was touched: both still `import { authGuard, capabilityGuard }
|
||||
from '@auth/auth.guard'`. Routing asks the auth context for its guards, which is the
|
||||
direction the boundary should read.
|
||||
|
||||
## Why the port widening was free
|
||||
|
||||
Both `SessionStore`s already exposed `readonly isAuthenticated = computed(() =>
|
||||
this._session() !== null)` (`session.store.ts:40` in each app), and both apps already
|
||||
registered `{ provide: SESSION_PORT, useExisting: SessionStore }`
|
||||
(`app.config.ts:64` / `:65`). `SessionPort` is satisfied structurally, so adding the
|
||||
member required no change in either app — the seam existed, it was just narrower than
|
||||
what it already carried.
|
||||
|
||||
## Scope discipline
|
||||
|
||||
Only the guards moved. Per the finding, **no** ticket to merge `session.store.ts`,
|
||||
`session.ts`, `digid.adapter.ts`, `login-form.component.ts` or `login.page.ts`, and no
|
||||
relaxation of ADR-0002 §3. Those five are identical because ADR-C-004 (`Session →
|
||||
Principal`) was never executed; merging them would cement a citizen DigiD/BSN login as
|
||||
the backoffice's shared login, which is the outcome §3 exists to prevent.
|
||||
|
||||
## Measured effect
|
||||
|
||||
Re-ran `tools/baseline-scan.mjs --dup` after the change:
|
||||
|
||||
| Metric | Before | After |
|
||||
| --------------------------- | -----: | --------: |
|
||||
| `ssp/auth` duplicated lines | 211 | **151** |
|
||||
| `bhp/auth` duplicated % | 86.8% | **82.5%** |
|
||||
| Repo-wide duplication | 7.1% | **6.6%** |
|
||||
|
||||
The `auth.guard.spec.ts` (36 windows) and `auth.guard.ts` (21 windows) clone pairs have
|
||||
dropped out of the top-clones list entirely. The remaining `ssp/auth` ↔ `bhp/auth`
|
||||
duplication is `session.store.ts` (39), `login-form.component.ts` (35) and `login.page.ts`
|
||||
(23) — exactly the three ADR-C-004 is expected to differentiate. Re-measure BL-002 after
|
||||
that lands; the finding's expectation is a drop to under 40 lines.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run lint` · `npm run typecheck` · `npm run dep:check` (0 violations, 224 modules)
|
||||
· `npx prettier --check apps libs` — all clean.
|
||||
Tests: shared 122, ssp 235, behandelportal 27, beheer 23 — **407 passed, 0 failed**.
|
||||
`ng build --localize` for both apps.
|
||||
+65
@@ -0,0 +1,65 @@
|
||||
# ADR-C-007 — repoint ADR-0003's WP-67 paths and fix its point 4
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `06-adr-conformance.md` §ADR-C-007
|
||||
|
||||
## What was wrong
|
||||
|
||||
Two separate defects in one ADR. Every file path in ADR-0003 predated WP-67's monorepo move,
|
||||
and decision point 4 made a claim about `app-alert` that the code contradicts.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------- | ---------------------------------------------------------------------------------- |
|
||||
| `docs/reference/architecture/0003-...md` | points 1, 2, 4 and both §Consequences bullets rewritten |
|
||||
| `CLAUDE.md` §2 | the `alert` parenthetical corrected to `skeleton`/`spinner` + a denial for `alert` |
|
||||
|
||||
No code changed. ADR first, CLAUDE.md to match, one diff.
|
||||
|
||||
## Paths, each verified
|
||||
|
||||
| Claimed | Actual |
|
||||
| ------------------------ | ---------------------------------------------------------------------- |
|
||||
| `src/styles.scss` | `libs/shared/styles.scss` — one copy, both apps' `angular.json:41,169` |
|
||||
| `src/index.html` | `apps/ssp/src/index.html` **and** `apps/behandelportal/src/index.html` |
|
||||
| `.storybook/` | `.storybook-ssp/` and `.storybook-behandelportal/` |
|
||||
| `src/docs/cibg-gaps.mdx` | `libs/shared/docs/cibg-gaps.mdx` |
|
||||
|
||||
**One path in the finding's list needed no change.** ADR-C-007 implied point 1's
|
||||
`public/cibg-huisstijl/` had moved with the rest. It has not: `public/` is still at the repo
|
||||
root, and both apps' `angular.json` asset entries read `"input": "public"` (`:38`, `:166`).
|
||||
Both Storybook configs serve it as `staticDirs: ['../public']`. Point 1's vendoring path is
|
||||
left as written; only its `index.html` clause changed.
|
||||
|
||||
## Point 4: the finding was right, and understated
|
||||
|
||||
ADR-C-007 flagged the `.alert` half of point 4. Verified: `libs/shared/src/ui/alert/alert.component.ts`
|
||||
documents itself as a _"Thin wrapper over the vendored `.feedback feedback-*` classes"_, its
|
||||
template binds `.feedback-info/-success/-warning/-error`, its only local CSS is a 3-line flex
|
||||
fix, and it carries **no** `CIBG-GAP EXTENSION` marker. `grep` confirms `feedback-error` is
|
||||
present in `public/cibg-huisstijl/css/huisstijl.css` — the class is vendored, so `alert` is not
|
||||
a gap.
|
||||
|
||||
**The finding missed that the same sentence's second claim is also false.** Point 4 said "the
|
||||
header/side-nav use `.nav` + a local blue bar". They do not:
|
||||
|
||||
- `site-header.component.ts` composes the vendored `.titlebar` and `.logo__*` classes
|
||||
(`grep` confirms `titlebar` in the vendored CSS) and its own comment says the titlebar
|
||||
_"keeps its own robijn fill — `--ro-layout` — untouched"_.
|
||||
- `shell.component.ts` emits only `.layout`, `.main`, `.content`, `.skip` — page scaffolding.
|
||||
- No `.nav` class appears in either, and neither carries a gap marker.
|
||||
|
||||
Both corrections are stated in the amended point 4 rather than silently dropped, so a reader
|
||||
comparing the old text against the code can see which claim was retired and why.
|
||||
|
||||
## Replacement example chosen
|
||||
|
||||
`skeleton` and `spinner`, as the finding proposed. Both are in the gap register, both carry
|
||||
markers reading "No loading-skeleton/spinner class in the vendored build", and both are
|
||||
genuinely absent — the cleanest live illustration of the principle point 4 exists to state.
|
||||
|
||||
## Noted, not fixed: the gap register is still one row short
|
||||
|
||||
`grep` finds **9** `CIBG-GAP EXTENSION` markers; `libs/shared/docs/cibg-gaps.mdx` has **8**
|
||||
rows. The missing one is `language-switcher`. That is **ADR-C-008 → RB-32** (batch 6), not
|
||||
this ticket, and it was left alone.
|
||||
+71
@@ -0,0 +1,71 @@
|
||||
# ADR-C-009 — state the runtime-editable-config exception as a test, not a list
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `06-adr-conformance.md` §ADR-C-009
|
||||
· Gated on: **RB-07** (satisfied — batch 2)
|
||||
|
||||
## What was wrong
|
||||
|
||||
ADR-0004 said "never runtime-editable" and then named **one** exception in the singular,
|
||||
justified narrowly ("specific to one sub-organization's identity"). WP-47 added a second
|
||||
runtime-editable SQLite surface, `FeatureFlagStore`, whose own doc-comment states the
|
||||
equivalence the ADR did not: _"SQLite-backed like `OrgTemplateStore`, same single-gate
|
||||
idiom."_
|
||||
|
||||
The code is right; the ADR's text was wrong. A closed list of one leaves the next
|
||||
operational-config surface with no principle to test itself against.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `docs/reference/architecture/0004-...md` | §"The deliberate exception: org-templates" → §"The deliberate exception: operational configuration" — a four-part test plus a table of the two passing surfaces |
|
||||
| same file, §Context + the table | `src/locale/*.xlf` → `apps/<app>/src/locale/*.xlf` (two apps since WP-67) |
|
||||
| `CLAUDE.md` §4 | the singular "Org-templates are the deliberate exception" replaced with the four-part test |
|
||||
|
||||
No code changed — the finding says so outright, and verification confirmed it.
|
||||
|
||||
## Why the RB-07 gate was real, verified clause by clause
|
||||
|
||||
Clause (4) of the test is "writes are admin-capability-gated **and** audited". Signing this
|
||||
ADR before RB-07 would have ratified a control the code did not implement. RB-07 has landed,
|
||||
so the clause is now true. Read at `backend/src/BigRegister.Api/Program.cs:863-923`: each of
|
||||
the five gates now computes `var ok = …`, calls `AuditAuthz(ctx, capability, resource, ok,
|
||||
principal)` with the **real** boolean, and only then branches. `FlagsAdmin`'s own comment
|
||||
names this ticket: _"this is the surface CQ-004/ADR-C-009 hinge on."_
|
||||
|
||||
All four clauses were checked against both surfaces rather than assumed:
|
||||
|
||||
| Clause | `OrgTemplateStore` | `FeatureFlagStore` |
|
||||
| ------------------- | ----------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------- |
|
||||
| (1) catalog in code | the `OrgTemplateDto` shape + `OrgTemplateRules` validate before save (`OrgTemplateStore.cs:49`) | `FeatureFlags.Catalog` (`Domain/Features/FeatureFlags.cs:15`) |
|
||||
| (2) fails closed | unknown `subOrgId` → `null` → endpoint 404s (`:44-45,:55-56,:72-73,:94-96`) | `Set` returns false for an unlisted key (`:54`); `IsEnabled` returns false (`:42-43`) |
|
||||
| (3) operational | one sub-organisation's letterhead | an on/off rollout switch |
|
||||
| (4) gated + audited | `OrgAdmin` → `orgtemplate:edit` (`Program.cs:863`) | `FlagsAdmin` → `flags:manage` (`Program.cs:914`) |
|
||||
|
||||
`FeatureFlagStore`'s own comment states clause (1) and (2) explicitly: _"The CATALOG … is
|
||||
code … this store only holds the admin's on/off overrides. An unknown key is never
|
||||
writable/enabled — the code catalog is the authority."_
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Clause (2) is about the write/enable path, not every read.** `OrgTemplateStore` has a
|
||||
deliberate read-path fallback for briefs from before WP-23 (`:110-114`, its own `ponytail:`
|
||||
comment): an empty `SubOrgId` falls back to the first seeded sub-org rather than failing a
|
||||
whole screen. That is a preview convenience on a read; the four write entry points all
|
||||
return `null` for an unknown sub-org. The clause is worded "cannot invent a setting,
|
||||
enable a feature, or be written" so this read fallback is not caught by it. Recorded
|
||||
because a reader checking clause (2) against `OrgTemplateStore.cs` will meet that
|
||||
fallback first.
|
||||
- **Org-templates' publish/rollback versioning is mentioned but excluded from the test.** It
|
||||
is stronger than the test requires, and making it a fifth clause would block a legitimate
|
||||
flag-style surface that has nothing to version.
|
||||
- **The stale `src/locale/*.xlf` paths were fixed in the same diff**, though ADR-C-009 did
|
||||
not flag them. They are two occurrences of the same WP-67 drift ADR-C-001 and ADR-C-007
|
||||
exist to correct, in the section being edited, and leaving a known-false path in a document
|
||||
while amending it is the exact failure mode those two findings describe. Scope creep is
|
||||
two words wide here; the alternative is filing a third ticket for it.
|
||||
|
||||
## Gate released
|
||||
|
||||
ADR-C-009 blocked "any ticket proposing a third runtime-editable config surface". Such a
|
||||
ticket can now be judged against a written test rather than by analogy to org-templates.
|
||||
@@ -0,0 +1,57 @@
|
||||
# RB-01 — authorize `GET /uploads/{id}/content` and `/uploads/status`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-004 · `99-backlog.md` RB-01
|
||||
|
||||
## What was wrong
|
||||
|
||||
`GET /uploads/{documentId}/content` took `(string documentId)` — no `HttpContext`, so no
|
||||
authorization was possible at all. It streams diploma and identity scans; the only
|
||||
protection was the unguessability of the document GUID. `DELETE` on the same resource has
|
||||
been owner-scoped (`DocumentStore.DeleteOwned`) since it was written.
|
||||
|
||||
`GET /uploads/status?localIds=` had the same shape, and leaks less but still confirms
|
||||
whether a given client-chosen `localId` exists anywhere in the store, plus its documentId.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` `/uploads/{documentId}/content` | takes `HttpContext`; allowed for the owning `ZorgverlenerCaller` or a caller passing `Authz.CanBeoordelen`; else `404` |
|
||||
| `Program.cs` `/uploads/status` | takes `HttpContext`; scoped to `ctx.Zorgverlener().Bsn` |
|
||||
| `Data/DocumentStore.cs` `ByLocalIds` | second parameter `owner`; filters on it (the only call site is the endpoint above) |
|
||||
| `tests/BigRegister.Tests/UploadAccessTests.cs` | **new** — 5 cases |
|
||||
|
||||
The two actor kinds are matched, not branched on a boolean, because `ctx.Zorgverlener()`
|
||||
**throws** for a `MedewerkerCaller` — a behandelaar reading an aanvraag's linked documents
|
||||
(`beoordeling-documenten.component.ts`) is a legitimate caller here:
|
||||
|
||||
```csharp
|
||||
var allowed = ctx.Caller() switch
|
||||
{
|
||||
ZorgverlenerCaller z => doc?.Owner == z.Bsn,
|
||||
var caller => Authz.CanBeoordelen(caller),
|
||||
};
|
||||
```
|
||||
|
||||
**404, not 403**, per the ticket: a foreign document id must not be distinguishable from
|
||||
one that never existed. `doc is null || !allowed` collapses both to the same answer, and
|
||||
`/uploads/status` reports a foreign `localId` as `"unknown"` — the same word an id that
|
||||
never existed gets.
|
||||
|
||||
## Known residual — this endpoint is reached without identity headers
|
||||
|
||||
Both callers link to the URL directly (`<a href>` in `beoordeling-documenten.component.ts`,
|
||||
`previewUrl` in `libs/shared/src/upload/upload.adapter.ts`), so the request is a plain
|
||||
browser navigation that carries **no** `X-Medewerker` / `X-Subject` header and never passes
|
||||
through an Angular interceptor. `StubIdentityProvider` therefore resolves it to the seeded
|
||||
citizen, which owns every document in the POC, so both links keep working — by coincidence,
|
||||
not by authorization. That coincidence **is** BIO-002, and it is fixed by **RB-09** (making
|
||||
`IIdentityProvider` able to express "no identity"), not here. RB-09 will need this endpoint
|
||||
to receive a real credential — a signed URL or a cookie — rather than the ambient default.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet build` clean. `dotnet test`: **250 passed, 1 failed** — the failure is
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
which needs a live OpenZaak container and **fails identically on a stashed tree**, i.e. it
|
||||
pre-dates this change.
|
||||
@@ -0,0 +1,58 @@
|
||||
# RB-02 — stop concatenating the BSN into `AuthzAudit.Resource`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-008 · `99-backlog.md` RB-02
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs` (was `:674`) built the reveal attempt's audit resource ref as
|
||||
`"brief/" + ctx.Zorgverlener().Bsn`. `AuditAuthz` persists that string to the
|
||||
`AuthzAudit.Resource` column in SQLite, and `GET /admin/audit` renders it on the admin
|
||||
audit page — so a BSN was written to durable storage and shown in a UI, on the one trail
|
||||
four documents describe as data-minimised and PII-free.
|
||||
|
||||
The endpoint is the _BIG-nummer reveal_, whose own comment says the audit carries
|
||||
"NO PII. Never the value that was (or wasn't) revealed" — and it did not carry the
|
||||
BIG-nummer. It carried the BSN instead, in the adjacent argument.
|
||||
|
||||
## Why the existing test did not catch it
|
||||
|
||||
`AuthzAuditTests.The_audit_schema_carries_no_pii` asserts on **column names**:
|
||||
|
||||
```csharp
|
||||
Assert.DoesNotContain(names, n => Regex.IsMatch(n, "naam|name|bsn|value|waarde", …));
|
||||
```
|
||||
|
||||
A BSN inside a column called `Resource` is invisible to a regex over the word `Resource`.
|
||||
The test was structurally incapable of failing on this defect, which is why the
|
||||
value-asserting test is part of this ticket's definition of done rather than a follow-up.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------- | ------------------------------------------------------------------------------------------------ |
|
||||
| `Program.cs` | resource ref is `"brief"`; a comment records why the id added nothing |
|
||||
| `AuthzAuditTests.cs` | **new** `No_audit_row_carries_a_subjects_bsn` — asserts on stored **values**, every string field |
|
||||
|
||||
No identifier was lost. `BriefStore` keys one brief per owner, so `brief/<bsn>` named the
|
||||
same thing the row's acting principal already implies; there is no second brief the ref
|
||||
could have disambiguated.
|
||||
|
||||
The new test drives a denied reveal as a **non-default** subject (`X-Subject: 999999990`),
|
||||
then scans every string field of every audit row for that BSN and for
|
||||
`DocumentStore.DemoOwner`. Asserting against the two BSNs actually in play, rather than a
|
||||
`\d{9}` shape, keeps it deterministic — a hex correlation id can hold nine consecutive
|
||||
digits by chance.
|
||||
|
||||
**Confirmed it fails without the fix**: reverting only the `Program.cs` line turns
|
||||
`No_audit_row_carries_a_subjects_bsn` red, and restoring it turns it green.
|
||||
|
||||
## Not in scope
|
||||
|
||||
`AuditEntry.Actor` on document audit rows also holds a raw BSN. That is a different store
|
||||
(`DocumentStore.Audit`) and is **RB-04**, which is where the masking decision for it lives.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet format --verify-no-changes` clean. `dotnet test`: **251 passed, 1 failed** — the
|
||||
failure is `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live OpenZaak container
|
||||
and fails identically on a stashed tree.
|
||||
@@ -0,0 +1,45 @@
|
||||
# RB-03 — mask the owner BSN on the cross-owner case lists
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-003 · `99-backlog.md` RB-03
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Mappers.ToAdminSummaryDto` set `Owner = a.Owner` — the raw BSN. Two endpoints consume it,
|
||||
both cross-owner lists read by someone who is **not** the subject:
|
||||
|
||||
- `GET /admin/cases` (`cases:manage`)
|
||||
- `GET /werkvoorraad` (`aanvraag:beoordelen`)
|
||||
|
||||
`GET /beoordeling/{id}` — the _detail_ view of the same data — already masked. So the
|
||||
detail screen showed `******782` while the list one click earlier showed the whole BSN.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------- | ---------------------------------------------------------------- |
|
||||
| `Domain/People/Pii.cs` | **new** — `Pii.MaskTail`, moved out of `Program.cs` |
|
||||
| `Contracts/Mappers.cs` | `Owner = Pii.MaskTail(a.Owner, 3)` |
|
||||
| `Program.cs` | local `MaskTail` deleted; two call sites point at `Pii.MaskTail` |
|
||||
| `AdminCasesTests.cs` | asserts the masked value and that `DemoOwner` does not appear |
|
||||
| `WerkvoorraadTests.cs` | same assertion, replacing the `IsNullOrEmpty` one |
|
||||
|
||||
**Masked in the mapper, not at the endpoints.** The point of the ticket is that both
|
||||
lists _inherit_ it, so a third cross-owner list cannot be added that forgets to mask.
|
||||
|
||||
**`MaskTail` moved to `Domain/People/Pii.cs`** because it now has three callers across
|
||||
three folders (`Contracts`, `Program.cs`, and `Data` once **RB-04** lands), and a second
|
||||
hand-rolled copy is how one of them drifts into leaking. It is documented as idempotent,
|
||||
which is what lets `/beoordeling/{id}` keep its own call: `IZaakSource` has a second
|
||||
implementation (`OpenZaakZaakSource` → `ZgwZaakMapper`, which maps `Owner` from the zaak
|
||||
`identificatie`), so that endpoint's guarantee should not depend on which source answered.
|
||||
|
||||
## Blast radius on the frontend — none
|
||||
|
||||
Both consumers use the value for display only (`admin-cases.page.ts:101`,
|
||||
`beoordeling-view.ts:40`, `werkvoorraad-item-view.ts:28`); the `parse*` boundaries require
|
||||
a non-empty string, which a masked BSN still is. Nothing keys, filters or looks up by owner.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet format --verify-no-changes` clean. `dotnet test`: **251 passed, 1 failed** — the
|
||||
pre-existing `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
@@ -0,0 +1,37 @@
|
||||
# RB-04 — mask the BSN recorded as `AuditEntry.Actor`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-005 · `99-backlog.md` RB-04
|
||||
|
||||
## What was wrong
|
||||
|
||||
`DocumentStore` writes one audit row per upload and per user delete, with the acting
|
||||
citizen's raw BSN as `AuditEntry.Actor`, persisted to SQLite. The class's own doc comment
|
||||
says "The audit log holds metadata only (never file content **or other PII**)" — a BSN in
|
||||
every row is precisely other PII. Same failure shape as RB-02, in a second store.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------- | --------------------------------------------------------- |
|
||||
| `Data/DocumentStore.cs` `Add` | `Audit("upload", …, Pii.MaskTail(owner, 3))` |
|
||||
| `Data/DocumentStore.cs` `DeleteOwned` | `Audit("delete-user", …, Pii.MaskTail(owner, 3))` |
|
||||
| `Data/DocumentStore.cs` `Audit` | doc comment: actors arrive **already redacted** |
|
||||
| `UploadAccessTests.cs` | **new** `The_document_audit_trail_records_a_masked_actor` |
|
||||
|
||||
**Masked at the two call sites, not inside `Audit`** — unlike RB-03, where masking in the
|
||||
mapper was the point. `Audit`'s third actor is the literal `"admin"` (from `AdminDelete`),
|
||||
and `MaskTail("admin", 3)` is `"**min"`: masking centrally would mean guessing which
|
||||
actors are BSNs and which are role names. The contract is stated on `Audit` instead.
|
||||
|
||||
**`StoredDocument.Owner` is untouched**, per the ticket. It is the authorization key —
|
||||
`DeleteOwned`, `ForeignIds` and now the RB-01 content check all compare against it — so it
|
||||
has to stay whole. The BSN remains where it is load-bearing and leaves the trail where it
|
||||
was only decoration.
|
||||
|
||||
Nothing reads `DocumentStore.AuditLog` today (no endpoint exposes it), so this is a
|
||||
data-at-rest fix with no response-shape change.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet format --verify-no-changes` clean. `dotnet test`: **252 passed, 1 failed** — the
|
||||
pre-existing `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
@@ -0,0 +1,50 @@
|
||||
# RB-05 — drop the BSN-bearing query and body snippet from the ZGW failure message
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-009 · `99-backlog.md` RB-05
|
||||
|
||||
## What was wrong
|
||||
|
||||
`ZgwHttpClient.SendWithRetryAsync` built its failure message as
|
||||
|
||||
```csharp
|
||||
$"ZGW {req.Method} {req.RequestUri} failed: {(int)res.StatusCode} {snippet}"
|
||||
```
|
||||
|
||||
with `snippet` being up to 500 characters of the **response body**. That message is not
|
||||
transient: `Program.cs`'s submit endpoint catches it and stores it as `Aanvraag.ZgwError`
|
||||
in SQLite, and logs it.
|
||||
|
||||
Two BSN paths into it:
|
||||
|
||||
- **the query string.** ZGW filters travel as query parameters, and the citizen-scoped zaken
|
||||
list filters on `rol__betrokkeneIdentificatie__natuurlijkPersoon__inpBsn=<BSN>`.
|
||||
- **the body snippet.** OpenZaak's error responses echo the offending request, so a rejected
|
||||
`POST /rollen` (whose body carries `BetrokkeneIdentificatie(aanvraag.Owner)`) comes back
|
||||
with the BSN in it.
|
||||
|
||||
The two `"returned null body"` throws in `GetAsync`/`PostAsync` interpolated the same url.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ----------------------- | ---------------------------------------------------------------------------- |
|
||||
| `Zgw/ZgwHttpClient.cs` | `Redact(url)` (path only) at all three sites; snippet → `res.ReasonPhrase` |
|
||||
| `ZgwDivergenceTests.cs` | **new** `A_recorded_divergence_carries_no_response_body_and_no_query_string` |
|
||||
|
||||
Status + path is enough to route a failure to the right endpoint. The diagnostic detail
|
||||
that was lost already has a deliberate home: `ZGW_DEBUG_HTTP=1` wires
|
||||
`ZgwDiagnosticHandler`, which logs the full url and request bytes — opt-in, dev-only, and
|
||||
not persisted.
|
||||
|
||||
## The test
|
||||
|
||||
Fails the `statustypen` GET (the only call in that fixture whose url carries a query
|
||||
string) after the zaak POST succeeds, then asserts on the persisted `ZgwError`:
|
||||
no `"stub failure"` (the body snippet), no `"?"` (the query string), but still the path and
|
||||
the `503`. **Confirmed it fails without the fix** — restoring the old interpolation turns it
|
||||
red on both counts.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet format --verify-no-changes` clean. `dotnet test`: **253 passed, 1 failed** — the
|
||||
pre-existing `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
@@ -0,0 +1,69 @@
|
||||
# RB-06 — delete the dead `POST /registrations`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-010 · `99-backlog.md` RB-06
|
||||
|
||||
## What was wrong
|
||||
|
||||
`POST /registrations` took a `Documents` list and passed it straight to `Submit`, which
|
||||
calls `DocumentStore.Link(...)` on every digital `documentId` in it. Linking a document
|
||||
**blocks its owner from deleting it** (`DeleteOwned` → 409 `Linked`).
|
||||
|
||||
There was no `ForeignIds` ownership check on that path. The real submit endpoint,
|
||||
`POST /applications/{id}/submit`, has had one since it was written:
|
||||
|
||||
```csharp
|
||||
if (documentIds is { Count: > 0 } && DocumentStore.ForeignIds(documentIds, ctx.Zorgverlener().Bsn) is { Count: > 0 } foreignIds)
|
||||
return Results.Problem(detail: $"Onbekend of niet-eigen document(en): …", statusCode: 400);
|
||||
```
|
||||
|
||||
So any authenticated citizen could post another citizen's document id and permanently
|
||||
block them from deleting their own diploma scan.
|
||||
|
||||
## Deleted rather than guarded
|
||||
|
||||
The ticket allowed either. Deleted, because the endpoint is dead: no frontend caller (the
|
||||
generated client's `registrations` method was unreferenced), and the whole registratie flow
|
||||
goes through `POST /applications/{id}/submit`.
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------- | ------------------------------------------------- |
|
||||
| `Program.cs` | endpoint deleted |
|
||||
| `Contracts/Dtos.cs` | `RegistratieRequest` deleted (no other reference) |
|
||||
| `Domain/Submissions/SubmissionRules.cs` | `RejectRegistratie` deleted — see below |
|
||||
| `backend/swagger.json`, `api-client.ts` | regenerated (`npm run gen:api`) |
|
||||
| `EndpointTests.cs`, `SubmissionRuleTests.cs` | retargeted, see below |
|
||||
|
||||
### Why `RejectRegistratie` went with it
|
||||
|
||||
It was reachable only from this endpoint, and the live path deliberately **contradicts**
|
||||
it. `RejectRegistratie("handmatig")` returned a 422 rejection; the modern submit does
|
||||
|
||||
```csharp
|
||||
"registratie" => (null, req.DiplomaHerkomst == "duo"),
|
||||
```
|
||||
|
||||
— a manual diploma is not rejected, it simply does not auto-approve and goes to a
|
||||
behandelaar. Its own message even said so ("doorgestuurd voor handmatige beoordeling")
|
||||
while being returned as a rejection. Leaving it behind would have left an obsolete rule
|
||||
with a passing spec, which is exactly how it gets reintroduced.
|
||||
|
||||
**This is the one judgement call in this ticket** — the backlog row says "delete the dead
|
||||
endpoint", not "delete the rule". Reverting just the `SubmissionRules`/`SubmissionRuleTests`
|
||||
hunks restores it without touching anything else.
|
||||
|
||||
### Test coverage that moved rather than vanished
|
||||
|
||||
- `Registration_with_manual_diploma_is_rejected_with_problem_details` was the only test
|
||||
asserting the `Submit` helper's `application/problem+json` rejection shape. That assertion
|
||||
moved into `Change_request_with_bad_phone_is_rejected_with_problem_details` —
|
||||
`/change-requests` is the other endpoint on the same helper.
|
||||
- `User_delete_blocked_with_409_once_linked_to_submission` covered `DocumentStore.Link`
|
||||
blocking a delete. Retargeted to `POST /applications/{id}/submit`, i.e. the path that is
|
||||
actually in use. `POST /registrations` was the only other caller of `Link`.
|
||||
- `Registration_with_duo_diploma_succeeds` was deleted outright — `Change_request_with_valid_phone_succeeds`
|
||||
is the same assertion on the same helper.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run ci`. `dotnet test`: **249 passed, 1 failed** — the pre-existing
|
||||
`OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
@@ -0,0 +1,63 @@
|
||||
# RB-07 — audit the allow path, not just the denial
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-007 (+ the outstanding half of CQ-004) · `99-backlog.md` RB-07
|
||||
|
||||
## What was wrong
|
||||
|
||||
All five authorization gates called `AuditAuthz(..., allowed: false, ...)` only on the deny
|
||||
branch; the allow branch called `action()` and returned. So `/beheer/audit` — the queryable
|
||||
trail the product ships as its audit surface — could answer "who was turned away" but never
|
||||
"who changed this".
|
||||
|
||||
Nothing recorded: `PUT /admin/flags/{key}`, `PUT /admin/org-template/{subOrgId}`,
|
||||
`POST /admin/org-template/{subOrgId}/rollback/{version}`, `DELETE /admin/cases/{id}`,
|
||||
`DELETE /admin/uploads/{documentId}`, `POST /brief/approve|reject|send`, and
|
||||
`POST /beoordeling/{id}/besluit`. The comment above `OrgAdmin` claimed the endpoints logged
|
||||
their own effect instead; publish and admin case delete do, the other six did not log at all.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------- | ----------------------------------------------------------------------------- |
|
||||
| `Program.cs` × 5 gates | `var ok = Authz.CanX(p); AuditAuthz(ctx, …, ok, p); if (ok) return action();` |
|
||||
| `Program.cs` `FlagsAdmin` | takes a per-call `resource` (see below) |
|
||||
| `Program.cs` `LogBrief` | takes `HttpContext`, writes the audit row alongside the log line |
|
||||
| `Program.cs` besluit | one `aanvraag:besluit` row recording **what** was decided |
|
||||
| `AuthzAuditTests.cs` | allow-path row; the flag key + value; a refused brief transition |
|
||||
| `BriefEndpointTests.cs` | the allow side of `brief:submit` |
|
||||
| `BeoordelingTests.cs` | the `aanvraag:besluit` row |
|
||||
|
||||
**The row is written by the gate, not the endpoint.** That is the point: a new admin
|
||||
endpoint cannot be added that forgets to audit itself. Same reasoning for the brief — every
|
||||
transition already funnelled through `LogBrief` for its log line, so the audit call went
|
||||
there too, which covers `submit`/`approve`/`reject`/`send` in one place and any fifth
|
||||
transition automatically. The decision recorded is the transition's own outcome, so a 403 or
|
||||
a 409 is as visible as a success.
|
||||
|
||||
**`FlagsAdmin` gained a `resource` parameter** — the one deviation from BIO-007's minimal
|
||||
remediation, and the reason is in the finding itself: the toggle endpoint writes no log line
|
||||
of its own, so a constant `"feature-flags"` row would record that a flag changed without
|
||||
recording _which_. It now writes `feature-flags/<key>=<value>`. One call site.
|
||||
`OrgAdmin`/`CasesAdmin` keep their coarse refs because those endpoints do log the specific
|
||||
object; **that asymmetry is deliberate, not an oversight.**
|
||||
|
||||
**The besluit gets a second row.** The `Beoordelen` gate records that a behandelaar was
|
||||
_allowed to act_; `aanvraag:besluit` records _what they decided_
|
||||
(`aanvraag/<id>/Goedkeuren`). Only the first would leave "who rejected this aanvraag"
|
||||
unanswerable, which is the question the trail exists for.
|
||||
|
||||
## Consequences worth knowing
|
||||
|
||||
- **Row volume goes up.** `StamdataAdmin` gates read endpoints, so every admin page load now
|
||||
writes rows. That is what "audit the allow path" means and BIO-007 asks for it explicitly;
|
||||
if `AuthzAuditStore` ever needs retention or sampling, this is the change that made it
|
||||
necessary.
|
||||
- **This unblocks ADR-C-009.** Clause (4) of agent 06's four-part test is "writes are
|
||||
admin-capability-gated **and** audited". Both surfaces now are, so the amendment can be
|
||||
signed without ratifying a control the code does not implement.
|
||||
- **CQ-004's outstanding half is closed.** `PUT /admin/flags/{key}` writes an audit row.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet test`: **252 passed, 1 failed** — the pre-existing
|
||||
`OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
@@ -0,0 +1,75 @@
|
||||
# RB-08 — route `DELETE /admin/uploads/{documentId}` through `CasesAdmin`; delete the orphaned `IsAdmin` gate
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-003 · `99-backlog.md` RB-08
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs:790` (pre-change) had `static bool IsAdmin(HttpContext ctx) => ctx.Request.Headers["X-Admin"] == "true";`
|
||||
and `DELETE /admin/uploads/{documentId}` (`:274`) was gated by `IsAdmin` alone — outside
|
||||
`Authz`, outside every wrapper the four sibling admin surfaces use, and writing no
|
||||
`AuthzAuditStore` row at all. `DocumentStore.AdminDelete` bypasses ownership and deletes the
|
||||
row and its bytes; the only record left behind was a `DocumentStore.Audit("delete-admin", …)`
|
||||
metadata row, which never surfaces on `/beheer/audit`.
|
||||
|
||||
`grep -rn "X-Admin"` over `apps`, `libs`, `backend`, `e2e` (re-verified before deleting the
|
||||
gate, as the ticket asked) confirmed the finding: the only sender was
|
||||
`backend/tests/BigRegister.Tests/EndpointTests.cs:231`. No frontend or e2e path uses this
|
||||
header — it was an orphaned gate, not a live seam.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` `DELETE /admin/uploads/{id}` | now `CasesAdmin(ctx, () => DocumentStore.AdminDelete(...) ? NoContent : NotFound)` — same wrapper the other four admin-cases endpoints use; response doc changed `Produces(403)` → `ProducesProblem(403)` to match `CasesAdmin`'s `Results.Problem` |
|
||||
| `Program.cs` `IsAdmin` | **deleted** |
|
||||
| `Program.cs` two stale comments | the endpoint's own comment rewritten to describe the new gate; the brief-section banner comment ("mirrors the X-Admin seam") no longer references a gate that doesn't exist |
|
||||
| `tests/BigRegister.Tests/EndpointTests.cs` | `Admin_delete_requires_admin_role` sends `X-Role: admin` instead of `X-Admin: true` |
|
||||
| `tests/BigRegister.Tests/AuthzAuditTests.cs` | **new** `An_admin_upload_delete_is_recorded` — asserts the `cases:manage`/`allow`/`Admin` row count increases by exactly one after the delete |
|
||||
|
||||
No new `Authz` capability was added — `CasesAdmin`/`Authz.CanManageCases` is the wrapper the
|
||||
ticket named as the expected outcome, and nothing about this endpoint needed a narrower
|
||||
capability than "manage cases" already provides.
|
||||
|
||||
**RB-07 already moved `AuditAuthz` onto the allow path for every `*Admin` wrapper**, so
|
||||
routing through `CasesAdmin` gives BIO-003's missing audit row for free. No second
|
||||
`AuditAuthz` call was added — confirmed by reading `CasesAdmin`'s body (`Program.cs`): it
|
||||
calls `AuditAuthz(ctx, "cases:manage", "cases", ok, principal)` unconditionally before
|
||||
branching on `ok`.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Test asserts a count delta, not mere presence.** `CasesAdmin` audits under a fixed
|
||||
`"cases"` resource literal shared by every `cases:manage` call (`GET /admin/cases`,
|
||||
`DELETE /admin/cases/{id}`, `GET /admin/audit` itself, and now this endpoint), so
|
||||
`Assert.Contains(rows, cases:manage/allow/Admin)` would already be satisfied by this test
|
||||
class's _other_ tests even without the fix. The new test counts matching rows before and
|
||||
after the delete and asserts the count grew by exactly one. It reads `AuthzAuditStore.List()`
|
||||
in-process rather than through `GET /admin/audit` — that endpoint is itself a `CasesAdmin`
|
||||
read, so calling it to take the "before" measurement would have written its own
|
||||
`cases:manage`/`allow` row and silently inflated the count by one every time it was called
|
||||
(caught this by running the test once against the fix with an HTTP-based baseline: it
|
||||
failed with an off-by-one before switching to the in-process read).
|
||||
- **Two comments referencing the old gate were also updated**, not just the endpoint mapping
|
||||
itself — one directly above the endpoint, one in the brief-section banner comment
|
||||
("dev-only stand-in via X-Role, mirrors the X-Admin seam") that would otherwise describe a
|
||||
gate that no longer exists.
|
||||
|
||||
## Known residual
|
||||
|
||||
None new. RB-01's implementation note already records that `GET /uploads/{id}/content` is
|
||||
reached with no identity header via plain browser navigation — that residual is RB-09's
|
||||
territory, not this ticket's, and is untouched here.
|
||||
|
||||
## Verification
|
||||
|
||||
- Reverted `Program.cs`'s endpoint change only (`git stash push` on that one file, tests
|
||||
left in place) and re-ran `dotnet test --filter "AuthzAuditTests|EndpointTests"`: **both**
|
||||
`EndpointTests.Admin_delete_requires_admin_role` and
|
||||
`AuthzAuditTests.An_admin_upload_delete_is_recorded` failed red (403 Forbidden — the old
|
||||
gate rejects `X-Role: admin`, and the count-delta test throws on `EnsureSuccessStatusCode`
|
||||
before it can assert). Restored the fix (`git stash pop`) and re-ran: both green.
|
||||
- `dotnet build`: clean.
|
||||
- `dotnet test` (full suite): **253 passed, 1 failed** — the pre-existing
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
which needs a live OpenZaak container and fails identically on a clean tree; not touched by
|
||||
this ticket.
|
||||
@@ -0,0 +1,168 @@
|
||||
# RB-09 — make "no identity" representable; stub Development-only; fail fast in Production
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-002 (folds in BIO-001(a)/(b)) · `99-backlog.md` RB-09
|
||||
|
||||
## What was wrong
|
||||
|
||||
`IIdentityProvider.Resolve` returned a non-nullable `CallerIdentity`
|
||||
(`IIdentityProvider.cs:12`, pre-change), so the interface could not express "no identity" — any
|
||||
implementation, stub or real, was forced to invent one for an unauthenticated request.
|
||||
`StubIdentityProvider` was registered unconditionally, for every environment.
|
||||
|
||||
Consequence, traced end to end: `apps/behandelportal/src/app/app.config.ts:57-63` puts
|
||||
`medewerkerInterceptor` inside the `isDevMode()` provider array, so a production
|
||||
behandelportal build sends **no** `X-Medewerker`/`X-Rollen` header. With neither header,
|
||||
`StubIdentityProvider.Resolve` fell through to
|
||||
`new ZorgverlenerCaller(DocumentStore.DemoOwner, ..., PrincipalRole.Drafter)` — the single
|
||||
seeded citizen, role `drafter`. That:
|
||||
|
||||
- **Fails closed, correctly, on backoffice capabilities** — `Authz.CanBeoordelen` is
|
||||
`caller is MedewerkerCaller`, so a zorgverlener caller is always `false` regardless of role.
|
||||
This part of the design was right and is untouched.
|
||||
- **Fails open on every citizen-scoped endpoint** — `GET/PUT/DELETE /applications*`,
|
||||
`POST /applications/{id}/submit`, `DELETE /uploads/{id}`, `GET|PUT /brief`,
|
||||
`POST /brief/submit|send|reset` all resolve `ctx.Zorgverlener().Bsn` to the seeded citizen's
|
||||
BSN. An employee with no employee identity was granted a citizen's own read/write rights.
|
||||
- **Holds `CanRevealBigNummer`** — that capability is `Role == PrincipalRole.Drafter`, and
|
||||
`drafter` is exactly the no-header default.
|
||||
|
||||
`CallerIdentityHttpContextExtensions.Caller()` (`CallerIdentity.cs:44-50`) already throws
|
||||
loudly when the identity middleware didn't run — the codebase reached for fail-loud one layer
|
||||
up and then defaulted one layer down, which is the shape of the bug.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `Domain/Authorization/IIdentityProvider.cs` | `Resolve` returns `CallerIdentity?`; doc comment states what null means and where it's turned into a response |
|
||||
| `Domain/Authorization/StubIdentityProvider.cs` | **implementation signature unchanged** (`CallerIdentity`, non-nullable) — a valid, narrower override of the nullable interface method (return-type covariance; the compiler accepts it with zero warnings); doc comment records it is Development-only and never itself returns null |
|
||||
| `Program.cs` — registration | `StubIdentityProvider` registered only under `builder.Environment.IsDevelopment()`; an `else if (builder.Environment.IsProduction())` branch throws `InvalidOperationException` immediately, before `builder.Build()` — the earliest possible failure point |
|
||||
| `Program.cs` — identity middleware | resolves the identity once; if `null`, sets `401` and returns without calling `next()`, instead of passing a null (or invented) caller downstream |
|
||||
| `tests/BigRegister.Tests/StubIdentityProviderTests.cs` | **new** `Never_returns_null_even_with_no_headers_at_all`; **new** `ProductionIdentityProviderTests.Production_environment_with_no_real_identity_provider_fails_at_startup` |
|
||||
|
||||
No consumer beyond the middleware itself calls `IIdentityProvider.Resolve` (`grep -rn
|
||||
"IIdentityProvider\|identityProvider\."` over `backend/src` confirms exactly one call site) —
|
||||
`Authz.ResolvePrincipal`, `ZgwTokenProvider.Mint`, and every endpoint read `ctx.Caller()` /
|
||||
`ctx.Zorgverlener()`, which already throw on a missing identity and are untouched. The 401
|
||||
now happens _before_ those are ever reached for a request the middleware rejects.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`StubIdentityProvider`'s own method signature stays `CallerIdentity`, not
|
||||
`CallerIdentity?`.** The interface needed the nullable shape to make "no identity"
|
||||
representable in general; the stub itself never has that case (it is a developer
|
||||
convenience that always invents a caller by design) and returning a narrower,
|
||||
non-nullable type from an override of a nullable-returning interface method is valid C#
|
||||
nullable-reference-type covariance — verified with a clean `dotnet build` (0 warnings).
|
||||
This kept every existing `StubIdentityProviderTests` call site (`private static
|
||||
CallerIdentity Resolve(...)`) compiling with zero changes, rather than sprinkling
|
||||
null-forgiving operators through a file whose entire point is "the stub always resolves."
|
||||
- **The Production-only check is `IsProduction()`, not `!IsDevelopment()`.** The ticket and
|
||||
BIO-002 both say "Production must fail at startup" specifically. A third environment (e.g.
|
||||
a hypothetical `Staging`) falls through neither branch, registers no `IIdentityProvider` at
|
||||
all, and would still fail — one line later, when `app.Services.GetRequiredService<
|
||||
IIdentityProvider>()` throws .NET's own "no service for type" exception — just with a less
|
||||
specific message than the one this ticket adds for Production. That fallback is a safety
|
||||
net, not the intended fail-fast message; if a real non-Production, non-Development
|
||||
environment is added later, giving it the same explicit message is a one-line follow-up,
|
||||
not a design gap today.
|
||||
- **The throw sits before `builder.Build()`**, not after (where `GetRequiredService` already
|
||||
runs today). Both satisfy "throw during service registration / app build so a misconfigured
|
||||
deploy never serves a request" — throwing earlier was free and gives a message naming the
|
||||
actual cause (no real identity provider) rather than a generic DI resolution failure.
|
||||
- **The 401 short-circuits before `ctx.SetCaller`, not after.** `next(ctx)` is never called,
|
||||
so no downstream middleware or endpoint runs for a request with no identity — a citizen or
|
||||
behandelaar endpoint reached this way now gets a clean 401 instead of ever executing.
|
||||
- **`TestWebApplicationFactory` needed no change.** `WebApplicationFactory<T>` defaults its
|
||||
test host to the `Development` environment when nothing overrides it (confirmed
|
||||
empirically: `dotnet test` — every non-Production test, all 253 of them pre-existing plus
|
||||
2 new, passed unchanged), so the entire existing test suite continues to exercise the
|
||||
Development path exactly as before. The Production test builds its own
|
||||
`WebApplicationFactory<Program>().WithWebHostBuilder(b => b.UseEnvironment("Production"))`
|
||||
rather than touching the shared fixture.
|
||||
|
||||
## Known residual — explicitly out of scope, confirmed and written up per the ticket
|
||||
|
||||
**RB-01's residual is this ticket's territory but is explicitly out of scope for this
|
||||
ticket**, per the task: `GET /uploads/{documentId}/content` is reached by a plain browser
|
||||
navigation (`<a href>` in `beoordeling-documenten.component.ts`, `previewUrl` in
|
||||
`libs/shared/src/upload/upload.adapter.ts`) that sends no identity header and never passes
|
||||
through an Angular interceptor.
|
||||
|
||||
- **In Development, this is unchanged** — verified by reading the endpoint
|
||||
(`Program.cs:253-266`) and confirming `StubIdentityProvider` is still registered and still
|
||||
resolves the same non-null seeded-citizen default it always did when no headers are
|
||||
present. `dotnet test`'s full pass (255/255, excluding the pre-existing OpenZaak failure)
|
||||
including `UploadAccessTests` — which exercises exactly this endpoint — confirms it
|
||||
byte-for-byte.
|
||||
- **The Production consequence, for the next ticket:** today Production cannot start at
|
||||
all (this ticket's fail-fast), so the question is moot until a real `IIdentityProvider`
|
||||
exists. Once one does, this endpoint's plain-navigation callers carry no credential a real
|
||||
provider could resolve — the identity middleware would treat that as "no identity" and
|
||||
return 401 before the endpoint ever runs, breaking both preview links outright. Making the
|
||||
stub Development-only does not itself break anything (nothing in Production exists yet to
|
||||
break), but it does mean **whoever builds the real provider must also solve this endpoint's
|
||||
credential-carrying problem in the same change**, or ship it broken. This is not a
|
||||
signed-URL or cookie scheme, and no such scheme was designed here, per the ticket's explicit
|
||||
instruction — it is recorded so the next ticket (RB-13, or whichever lands the real
|
||||
provider) picks it up deliberately rather than discovering it in a production incident.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Reverted the registration change only** (kept `AddSingleton<IIdentityProvider,
|
||||
StubIdentityProvider>()` unconditional, left the new tests in place) and ran
|
||||
`ProductionIdentityProviderTests`: it failed red — `Assert.ThrowsAny() Failure: No exception
|
||||
was thrown` (the stub gets registered in every environment, so the host builds fine and
|
||||
`factory.CreateClient()` never throws). Restored the fix and re-ran: green.
|
||||
- `dotnet build`: clean, **0 warnings** (confirms the nullable-covariance judgement call
|
||||
above compiles cleanly).
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test` (full suite): **255 passed, 1 failed** — the pre-existing
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`
|
||||
(needs a live OpenZaak container; fails identically on a clean tree). CI's actual filter,
|
||||
`dotnet test BigRegister.slnx --filter "Category!=Integration"`: **255 passed, 0 failed**.
|
||||
|
||||
## A regression found by actually running `npm run ci`, and its fix
|
||||
|
||||
`npm run gen:api` (`dotnet swagger tofile`) loads `BigRegister.Api.dll` through .NET's
|
||||
design-time `HostFactoryResolver` — the same mechanism `dotnet ef` migrations use — which
|
||||
executes this file's top-level statements, including the identity middleware's
|
||||
pre-existing, unconditional `app.Services.GetRequiredService<IIdentityProvider>()`, without
|
||||
ever setting `ASPNETCORE_ENVIRONMENT`. Unset defaults to `Production`. Before this ticket
|
||||
that was harmless (`StubIdentityProvider` was registered unconditionally); after it, nothing
|
||||
is registered for that default environment, so the tool crashed
|
||||
(`dotnet swagger tofile` exited **134**, confirmed by running it directly, both before and
|
||||
after the fix below).
|
||||
|
||||
This is real breakage of a real workflow, not a false alarm from `ci-local.sh` — verified by
|
||||
reading `.github/workflows/ci.yml`'s `api-client-drift` job: `npm run gen:api` and
|
||||
`git diff --exit-code ...` are **two separate `- run:` steps** there, so the crash would fail
|
||||
actual CI. `ci-local.sh` chains them as `npm run gen:api && git diff --exit-code ...` on one
|
||||
line, and its first full run of `npm run ci` after this ticket's change **printed the crash
|
||||
but still reported `✔ local CI passed`** — a bash `set -e` gotcha, not a false negative
|
||||
specific to this fix: a failing command that is not the last element of an `&&`/`||` list is
|
||||
exempt from triggering `errexit`, so `cmd1 && cmd2` silently "passes" whenever `cmd1` alone
|
||||
fails. That is a pre-existing fragility in `ci-local.sh`'s three `step "X"; gen && git diff`
|
||||
lines (snippets/behaviour-spec/api-client drift), unrelated to RB-09 and out of this
|
||||
ticket's scope — flagged here rather than fixed, since fixing a local convenience script's
|
||||
error handling is a different, standalone change. Running the failing command directly
|
||||
(rather than trusting the local script) is what caught this.
|
||||
|
||||
**Fix:** `package.json`'s `gen:api` script now sets `ASPNETCORE_ENVIRONMENT=Development`
|
||||
on the `dotnet swagger tofile` invocation specifically — the same value
|
||||
`backend/src/BigRegister.Api/Properties/launchSettings.json` already sets for `dotnet run`,
|
||||
and the same value `docker-compose.yml` already sets for local Docker (confirmed by reading
|
||||
both: `docker-compose.prod.yml` sets `Production` explicitly, `docker-compose.yml` sets
|
||||
`Development` explicitly — the bare CLI tool invocation was the **one** place with no
|
||||
environment variable set at all). Verified: `npm run gen:api` now exits 0 and produces the
|
||||
regenerated `backend/swagger.json` / `libs/shared/src/infrastructure/api-client.ts` cleanly.
|
||||
|
||||
This also surfaced a second, unrelated gap: RB-08 changed
|
||||
`DELETE /admin/uploads/{documentId}`'s 403 mapping from `.Produces` to `.ProducesProblem`
|
||||
but the generated OpenAPI doc/client were never regenerated for it (RB-08's own `npm run ci`
|
||||
was run before this fix existed, so `gen:api` was already broken by the time RB-09 landed
|
||||
and the drift went unnoticed). Regenerated and committed separately — see the two follow-up
|
||||
commits **fix(api): regenerate client for RB-08's 403 response shape** and
|
||||
**fix(tooling): keep gen:api working under RB-09's Development-only stub**. No frontend
|
||||
consumes the admin-uploads-delete endpoint (confirmed by grep), so the client regeneration
|
||||
has no consumer impact.
|
||||
@@ -0,0 +1,87 @@
|
||||
# RB-10 — extract `parseStoredSession` (both apps) and spec `redactProfile`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `02-testability.md` TE-001 (ssp/auth and bhp/auth) · `07-bio2-compliance.md` BIO-017 · `99-backlog.md` RB-10
|
||||
|
||||
## What was wrong
|
||||
|
||||
`SessionStore.restore()` — identical in `apps/ssp/src/app/auth/application/session.store.ts`
|
||||
and `apps/behandelportal/src/app/auth/application/session.store.ts` — called
|
||||
`localStorage.getItem(STORAGE_KEY)` itself and did the parse + shape validation in the same
|
||||
module-private function. It was invoked from a field initializer
|
||||
(`private _session = signal<Session | null>(restore())`), so the storage read happened the
|
||||
instant the singleton was constructed; a spec could not feed it a raw string without
|
||||
stubbing the `localStorage` global before the injector built the store.
|
||||
|
||||
The logic behind that guard is a trust boundary, not incidental validation — the comment
|
||||
above it names two guarantees: **G1** (never persist the BSN) and **G2** (validate the shape
|
||||
before trusting it). CLAUDE.md §5 mandates a spec for boundary `parse*` adapters, and none
|
||||
existed. Baseline evidence: `02-testability.md` §3a cites `ssp/auth` and `bhp/auth` at
|
||||
42.9% line / 46.2% branch — jointly the worst line coverage in the frontend table — with
|
||||
this file's own lcov at LH 2/LF 20 (10.0% line), BRH 3/BRF 13 (23.1% branch).
|
||||
|
||||
BIO-017 read the same code and confirmed G1 holds on every path by inspection (`restore()`
|
||||
returns `{ bsn: '', naam }`, the persistence `effect()` writes only `naam`, `login()`/
|
||||
`logout()` never touch storage with a BSN) — but "correct, unverified by a test" is exactly
|
||||
the gap TE-001 already targeted, so BIO-017 folds into it and adds one required assertion:
|
||||
a stored `{"bsn":"…","naam":"…"}` must yield a session whose `bsn` is `''`.
|
||||
|
||||
Separately, `apps/ssp/src/app/shell/debug-state/mask.ts` — confirmed at the path the finding
|
||||
cites — has `redactProfile`, a pure, exported, directly callable PII-redaction function with
|
||||
no spec. It redacts name, birthdate and address and masks the BIG-nummer; BIO-017 verified it
|
||||
correct by reading, same "no regression net" gap.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `apps/ssp/src/app/auth/domain/session.ts` | added `export function parseStoredSession(raw: string \| null): Session \| null` — the exact parse+validate body `restore()` used to hold |
|
||||
| `apps/ssp/src/app/auth/application/session.store.ts` | `restore()` collapses to `parseStoredSession(localStorage.getItem(STORAGE_KEY))` |
|
||||
| `apps/ssp/src/app/auth/domain/session.spec.ts` | 4 new cases: absent, non-JSON, wrong shape, and the G1 assertion |
|
||||
| `apps/behandelportal/src/app/auth/domain/session.ts` | identical extraction, second app |
|
||||
| `apps/behandelportal/src/app/auth/application/session.store.ts` | identical collapse, second app |
|
||||
| `apps/behandelportal/src/app/auth/domain/session.spec.ts` | identical 4 cases, second app |
|
||||
| `apps/ssp/src/app/shell/debug-state/mask.spec.ts` | new file — spec for `redactProfile`: masks the BIG-nummer, redacts name/geboortedatum/adres on both `registration` and `person`, leaves `beroep`/`registratiedatum`/`status` untouched |
|
||||
|
||||
The extracted function's body is a byte-for-byte move — same `try`/`catch`, same
|
||||
`JSON.parse` cast, same `typeof parsed?.naam === 'string'` guard, same `{ bsn: '', naam }`
|
||||
construction. Only its location and the doc comment (rewritten to explain the _why_ of G1/G2
|
||||
for a function now read on its own, rather than inline next to the `effect()` it used to sit
|
||||
beside) changed.
|
||||
|
||||
## The seam lands twice, on purpose
|
||||
|
||||
`auth` is deliberately unshared per ADR-0002 / CLAUDE.md §1: Zorgverlener and Medewerker are
|
||||
different `Principal` variants with different login flows, and the two `session.ts` files are
|
||||
expected to diverge. TE-001 says this outright, and BL-002 flags any extract-to-`libs/shared`
|
||||
here as contradicting an accepted ADR. `parseStoredSession` was therefore written twice, once
|
||||
per app's own `domain/session.ts` — not factored into a shared helper, and not resisted only
|
||||
in this note; the two functions are word-for-word identical today and that is expected to
|
||||
change the moment `RB-13` (`Session → Principal`) lands.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`redactProfile`'s spec asserts on the concrete shape**, not just "not equal to the input" —
|
||||
it pins `bigNummer` to `'********901'`, checks `REDACTED` on each PII field individually, and
|
||||
separately asserts the non-PII fields (`beroep`, `registratiedatum`, `status`) survive
|
||||
unchanged. A looser "no PII substring appears" assertion would have been weaker at catching
|
||||
the regression this ticket exists to prevent (e.g. a future field added to `redactProfile`'s
|
||||
output that is left unmasked by accident).
|
||||
- **The G1 spec case uses `toEqual`, not `toBe`**, since the parser constructs a new object;
|
||||
this matches the existing `isAuthenticated` spec's style in the same file.
|
||||
- No production code beyond the `restore()` one-liner in each `session.store.ts` changed —
|
||||
`login()`, `logout()`, and the persistence `effect()` were already correct and are
|
||||
unaffected.
|
||||
|
||||
## Verification
|
||||
|
||||
Confirmed both new specs are red without the fix:
|
||||
|
||||
- Temporarily changed `parseStoredSession` to keep a stored `bsn` (`bsn: parsed.bsn ?? ''`
|
||||
instead of `bsn: ''`) — the new G1 test failed with
|
||||
`expected { bsn: '19012345601', naam: 'Test' } to deeply equal { bsn: '', naam: 'Test' }`,
|
||||
all 243 other tests stayed green. Reverted; `git diff` on the file is empty afterward.
|
||||
- Temporarily changed `redactProfile` to pass `naam` through unmasked — the new "redacts the
|
||||
name" test failed with `expected 'J. Jansen' to be '‹redacted›'`. Reverted; `git diff` on the
|
||||
file is empty afterward.
|
||||
|
||||
`npm run ci`: **green** (see PR/commit for the run this doc ships with).
|
||||
@@ -0,0 +1,179 @@
|
||||
# RB-11 — dev hatches out of prod, trust boundaries exported, doc corrected
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-012,
|
||||
TE-002, BIO-006(a)+(b) · `99-backlog.md` RB-11
|
||||
|
||||
## What was wrong
|
||||
|
||||
**BIO-012 — the dev hatches were not actually dev-only.** The `roleInterceptor` /
|
||||
`subjectInterceptor` chain is correctly registered only under `isDevMode()`
|
||||
(`app.config.ts`), but three adapters bypass `HttpClient` entirely and set headers
|
||||
themselves with no guard at all:
|
||||
|
||||
- `reveal-bignummer.adapter.ts:26` — `'X-Role': currentRole(), 'X-Step-Up': 'true'`
|
||||
- `letter-preview.adapter.ts:46` — `'X-Role': currentRole()`, plus `'X-Subject'` when present
|
||||
- `org-template.adapter.ts:79` — `'X-Role': currentRole()`
|
||||
|
||||
The readers underneath were ungated too: `role.ts:24` and `subject.ts:24` both read the
|
||||
`?role=`/`?subject=` query param and **wrote it into `sessionStorage`** on any
|
||||
navigation, in any build. For `?subject=` that value is a BSN — `subject.ts`'s own doc
|
||||
comment argued at length that the BSN must never leave `SessionStore` and then routed it
|
||||
through `sessionStorage` anyway. `docs/reference/roles-and-access.md:23` claimed "Both
|
||||
are wired only under `isDevMode()` — they do not exist in a production build", which was
|
||||
false for exactly these three call sites.
|
||||
|
||||
**TE-002 — the reveal's trust boundary was not callable.** The response-shape validation
|
||||
in `reveal-bignummer.adapter.ts` (the code's own comment called it a "Trust boundary")
|
||||
lived inline inside `async reveal()`, after `await fetch(...)` on the global `fetch`. A
|
||||
spec could not reach it without stubbing `globalThis.fetch`. The same shape recurred,
|
||||
un-exported, in `letter-preview.adapter.ts`'s `errorMessage` and — contrary to the
|
||||
finding's text, see "Judgement calls" below — as an inline `try/catch` (not yet a
|
||||
function) in `org-template.adapter.ts`'s `proefbrief()`.
|
||||
|
||||
**BIO-006(a) — the step-up stub was a constant.** `reveal-bignummer.adapter.ts` sent
|
||||
`'X-Step-Up': 'true'` unconditionally, as a literal, so the backend's
|
||||
`canReveal && X-Step-Up == "true"` precondition was satisfied by every call that reached
|
||||
the endpoint and constrained nothing.
|
||||
|
||||
**BIO-006(b) — the default role holds the PII-reveal capability, undocumented.**
|
||||
`StubIdentityProvider`'s `_ =>` role-switch arm resolves any request with no (or an
|
||||
unrecognised) `X-Role` header to `drafter` — the one role `Authz.CanRevealBigNummer`
|
||||
grants. `roles-and-access.md` documented `drafter` as "the only role that may reveal a
|
||||
BSN" without noting that it is also the fallback identity, so the least-privilege
|
||||
consequence was invisible.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `libs/shared/src/infrastructure/role.ts` | `currentRole()` returns `'drafter'` immediately when `!isDevMode()` — no query-param read, no `sessionStorage` write |
|
||||
| `libs/shared/src/infrastructure/subject.ts` | `currentSubject()` returns `undefined` immediately when `!isDevMode()` — same treatment for the BSN |
|
||||
| `reveal-bignummer.adapter.ts` | `reveal(stepUp: boolean)`; `X-Role` sent only under `isDevMode()`, `X-Step-Up` sent only when `stepUp`; inline shape check moved to exported `parseRevealed(body)`; `REVEAL_FAILED` exported |
|
||||
| `letter-preview.adapter.ts` | headers wrapped in `isDevMode() ? {...} : {}`; `errorMessage` exported |
|
||||
| `org-template.adapter.ts` | `X-Role` sent only under `isDevMode()`; the inline `proefbrief()` error `try/catch` extracted to an exported `proefbriefErrorMessage`; `PROEFBRIEF_FAILED` exported |
|
||||
| `apps/ssp/src/app/brief/application/brief.store.ts` | `revealBigNummer()` calls `this.revealAdapter.reveal(true)` — the literal now lives at the one call site reachable only after the UI's confirm gesture, not inside the adapter |
|
||||
| `docs/reference/roles-and-access.md` | records which two places the `isDevMode()` gate now lives (interceptor registration **and** the reader functions) and why; adds the BIO-006(b) note that `drafter` is also the backend's fallback identity |
|
||||
| 6 new/extended `*.spec.ts` | see "Verification" below |
|
||||
|
||||
**The fix is two layers, not one, because the finding named both.** Gating only
|
||||
`currentRole()`/`currentSubject()` would already stop the query param and the
|
||||
`sessionStorage` write from working outside `isDevMode()` — the three adapters would
|
||||
then send the safe default (`'X-Role': 'drafter'`, no `X-Subject`) even in production.
|
||||
The adapters are _also_ wrapped in `isDevMode()` so a production request from any of the
|
||||
three hand-written `fetch` calls carries no `X-Role`/`X-Subject` header at all, exactly
|
||||
matching what a `HttpClient` request already does once `roleInterceptor` is not
|
||||
registered — the two paths now agree on production behaviour instead of merely agreeing
|
||||
on the resulting header value.
|
||||
|
||||
**`X-Step-Up` is deliberately not folded into the same `isDevMode()` gate.** It is not a
|
||||
`?role=`/`?subject=`-style dev override; it is the stub for a control BIO-006 says must
|
||||
survive into production (in stubbed form) until a real step-up exists. Nesting it inside
|
||||
`isDevMode()` would make the reveal endpoint permanently unreachable in a production
|
||||
build. Instead it is gated on the `stepUp` parameter alone, which is `true` only when
|
||||
`BriefStore.revealBigNummer()` — reachable only via `behandel-scherm.component.ts`'s
|
||||
`onReveal()` confirm — calls it.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`org-template.adapter.ts`'s proefbrief error mapping was not "already a separate
|
||||
function".** The finding's remediation text says "the proefbrief error mapping in
|
||||
`org-template.adapter.ts` — both are already separate functions and only need
|
||||
`export` and a spec", matching `letter-preview.adapter.ts`'s `errorMessage`. Reading
|
||||
the file: the other two adapters do have a standalone `errorMessage`/similar function,
|
||||
but `org-template.adapter.ts`'s proefbrief error handling was inlined directly in the
|
||||
`try { … } catch { … }` block, not a named function. This is a minor factual
|
||||
imprecision in the finding, not a blocker — I extracted the same inline logic into a
|
||||
named `proefbriefErrorMessage`, exported it, and added the same spec shape as its two
|
||||
siblings. The result matches the finding's intent (a callable, spec'd trust boundary)
|
||||
even though the starting shape needed one extra step the finding didn't mention.
|
||||
- **The BIO-006(a) literal moved to `BriefStore.revealBigNummer()`, not to the UI.**
|
||||
`behandel-scherm.component.ts`'s `onReveal()` already gates the _only_ path that can
|
||||
reach `store.revealBigNummer()` behind a `confirm()` dialog, and the store's own
|
||||
docstring says the step-up gesture "is the UI's concern". Threading a boolean through
|
||||
the component's `output<void>()` and the page's template binding would touch three more
|
||||
files for no behavioural change, since the call graph already guarantees confirmation
|
||||
happened first. I moved the literal one layer up instead — out of the adapter (the
|
||||
transport) and into the store (the command that is exclusively reachable via the
|
||||
confirmed gesture) — which is the smallest change consistent with "not from the
|
||||
adapter's literal" and with this repo's ui → application → infrastructure layering (ui
|
||||
cannot call infrastructure directly to pass the flag down any other way).
|
||||
- **Redundant-looking `isDevMode()` guards, kept anyway.** After gating
|
||||
`currentRole()`/`currentSubject()`, the three adapters' own `isDevMode()` wrap around
|
||||
the headers object is not strictly load-bearing for `X-Subject` (already `undefined`
|
||||
outside dev) and only changes the _value sent_ for `X-Role` (a hardcoded `'drafter'`
|
||||
vs. no header) rather than any security outcome (the backend treats both identically).
|
||||
I kept the adapter-level gate anyway so the security posture is visible by inspection
|
||||
at the fetch call site — matching `app.config.ts`'s `isDevMode() ? [...] : []` pattern
|
||||
— rather than requiring a reviewer to trace into `role.ts`/`subject.ts` to confirm it.
|
||||
- **No `proefbrief()`-level header spec.** `OrgTemplateAdapter` injects `ApiClient` via
|
||||
`inject()`, so exercising `proefbrief()` itself needs a `TestBed` + a mock `ApiClient`
|
||||
purely to reach a method that doesn't use either. I judged that disproportionate to the
|
||||
marginal coverage gained, since the identical `isDevMode()` pattern is already
|
||||
exercised end-to-end (via `fetch` stubbing) on the other two adapters
|
||||
(`reveal-bignummer.adapter.spec.ts`, `letter-preview.adapter.spec.ts`), and the
|
||||
underlying reader-level fix is covered directly in `role.spec.ts`. Noted here as a
|
||||
residual rather than silently skipped.
|
||||
- **`setRole()` (the dev-switcher writer) was left ungated.** BIO-012's evidence names
|
||||
the two _readers_ (`currentRole`/`currentSubject`); `setRole()` is only ever invoked
|
||||
from `debug-state.component.ts`, which is itself rendered only under
|
||||
`shell.component.ts`'s `@if (isDev && debugPanel)`. Gating it too would be harmless but
|
||||
wasn't asked for and has no reachable production call site to protect — left alone to
|
||||
keep the diff to what the finding actually named.
|
||||
|
||||
## Consequences worth knowing
|
||||
|
||||
- **Doc correction, same diff.** `roles-and-access.md`'s "Both are wired only under
|
||||
`isDevMode()`" line is accurate as of this commit — the gate now lives in the
|
||||
interceptor registration **and** inside `currentRole()`/`currentSubject()` themselves.
|
||||
Before this commit, the sentence was false for the three hand-written `fetch` paths; the
|
||||
doc has been extended, not merely left as-is, to say _where_ the gate lives so a future
|
||||
reader doesn't have to rediscover why the interceptor site alone wasn't sufficient.
|
||||
- **CLAUDE.md needed no correction.** Its "Scenario toggle (dev-only, not wired in prod
|
||||
builds)" and "Dev role stand-in (dev-only)" lines don't claim anything about the three
|
||||
hand-written `fetch` adapters specifically (the accompanying sentence already says they
|
||||
"bypass the interceptor", which stays true — they still don't go through
|
||||
`HttpClient`). Those claims were already compatible with a fix landing here; they made
|
||||
no false statement that needed walking back.
|
||||
- **`?subject=` is still undocumented by name in `roles-and-access.md`.** The BIO-012
|
||||
evidence and this ticket's brief both discuss it, but the doc file never named
|
||||
`?subject=`/`X-Subject` before this change and still doesn't get a dedicated section —
|
||||
only the new paragraph under "How to switch role" mentions it in passing. A full
|
||||
`?subject=` write-up (its own e2e-only purpose, `X-Medewerker`/`X-Rollen` parallel) is
|
||||
arguably worth a follow-up doc pass, but out of scope for a security-focused ticket
|
||||
about production leakage.
|
||||
- **Behaviour spec regenerated.** `libs/shared/docs/behaviour-spec.mdx` is generated from
|
||||
the suite (`npm run gen:behaviour-spec`) and is included in this diff — the CI gate's
|
||||
drift check would otherwise fail on the 6 new `describe` blocks this ticket adds.
|
||||
|
||||
## Verification
|
||||
|
||||
Every fix below was confirmed **red without it** by temporarily reverting the source
|
||||
change (tests unchanged) and re-running the affected suite, then restoring the fix:
|
||||
|
||||
- `libs/shared/src/infrastructure/role.spec.ts` — removing the `if (!isDevMode())` guard
|
||||
from `currentRole()` turned 3 "outside isDevMode()" tests red (`?role=` still honoured,
|
||||
still written to `sessionStorage`).
|
||||
- `libs/shared/src/infrastructure/subject.spec.ts` — same removal on `currentSubject()`
|
||||
turned its 3 "outside isDevMode()" tests red (a BSN still read from the URL and written
|
||||
to `sessionStorage`).
|
||||
- `apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.spec.ts` — reverting
|
||||
`reveal()` to the original unconditional `{ 'X-Role': currentRole(), 'X-Step-Up': 'true' }`
|
||||
turned both `RevealBigNummerAdapter.reveal` tests red (`X-Step-Up` sent regardless of the
|
||||
`stepUp` argument; `X-Role` sent regardless of `isDevMode()`).
|
||||
|
||||
New specs, all pure/exported-boundary tests per house convention (no `TestBed`, no
|
||||
`globalThis.fetch` stub needed for the pure halves):
|
||||
|
||||
| File | Covers |
|
||||
| ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `reveal-bignummer.adapter.spec.ts` | `parseRevealed` (incl. the finding's own `{ bigNummer: 42 }` rejection case); `reveal()`'s `X-Step-Up`/`X-Role` gating via a stubbed `fetch` |
|
||||
| `letter-preview.adapter.spec.ts` | `errorMessage`; `preview()`'s header gating via a stubbed `fetch` |
|
||||
| `org-template.adapter.spec.ts` (extended) | `proefbriefErrorMessage` |
|
||||
| `role.spec.ts` (new) | `currentRole()` dev behaviour + the `isDevMode()`-gated production behaviour |
|
||||
| `subject.spec.ts` (new) | `currentSubject()` dev behaviour + the `isDevMode()`-gated production behaviour |
|
||||
|
||||
`npm run ci` (lint, typecheck, `dep:check`, `format:check`, `check:tokens`, `check:seam`,
|
||||
full test suite with coverage, `ng build --localize` for both apps, `npm audit`, backend
|
||||
`dotnet format` + `dotnet test`, showcase-snippets/behaviour-spec/api-client drift
|
||||
checks): **green**, including all 4 vitest projects (ssp/behandelportal/shared/beheer) and
|
||||
`dotnet test` (241 passed).
|
||||
@@ -0,0 +1,115 @@
|
||||
# RB-12 — a route-table test: every route hits an authz wrapper or an explicit allow-list
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-016, `00-baseline.md` BL-006 · `99-backlog.md` RB-12
|
||||
|
||||
## What was wrong
|
||||
|
||||
BL-006, verbatim: "the backend has zero automated architecture enforcement … `Domain/`
|
||||
purity currently holds by convention." BIO-016 names the specific consequence for
|
||||
authorization: nothing asserted the **set** of gated endpoints, so an endpoint added
|
||||
without a gate (BIO-003's `X-Admin` gate outside `Authz`, BIO-004's two endpoints with
|
||||
no gate at all) failed no test. Both were caught by a human reading `Program.cs`, not by
|
||||
CI.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` — 16 endpoint mappings | each chains a new `.Gate("XAdmin")` call, naming the admin wrapper (`OrgAdmin`, `StamdataAdmin`, `CasesAdmin`, `Beoordelen`, `FlagsAdmin`) already used inside its handler |
|
||||
| `Program.cs` — new types, end of file | `public sealed record AuthzGateMetadata(string Wrapper)` + a `Gate(...)` extension method on `IEndpointConventionBuilder` that attaches it via `.WithMetadata(...)` |
|
||||
| `tests/BigRegister.Tests/RouteInventoryTests.cs` | **new** — walks the real app's `EndpointDataSource`, asserts every route carries either an `AuthzGateMetadata` naming a known wrapper, or an entry in a written-down allow-list; a second test asserts every `.Gate(...)` name is one of the five known wrappers |
|
||||
|
||||
## Design: metadata at mapping time, not reflection over the compiled lambda
|
||||
|
||||
The ticket left the detection mechanism open, noting the wrappers are local functions
|
||||
in `Program.cs`. Reflecting over a compiled minimal-API lambda to determine which local
|
||||
function its closure calls is fragile-to-impossible (the call is inside IL a test would
|
||||
have to disassemble, and a local function's identity isn't easily recoverable from the
|
||||
delegate's `MethodInfo`). Endpoint **metadata**, attached at the same call site where the
|
||||
route is mapped, is exactly what `EndpointDataSource` hands back to a test host and
|
||||
doesn't depend on inspecting compiled code at all — so a `.Gate("XAdmin")` extension
|
||||
method was added and chained onto each of the 16 mappings that call one of the five
|
||||
wrappers.
|
||||
|
||||
This is a **declaration**, not a **derivation**: the test does not verify that
|
||||
`.Gate("CasesAdmin")` and an actual `CasesAdmin(ctx, …)` call inside the handler agree —
|
||||
it only verifies that a marker is present. A handler that swapped its `CasesAdmin(ctx,
|
||||
…)` call for a no-op without updating `.Gate(...)` would go undetected here. What _is_
|
||||
caught, reliably, is the actual BIO-003/BIO-004 failure mode: a new endpoint mapped with
|
||||
**no** marker and **no** allow-list entry — verified below by adding one and watching the
|
||||
test go red.
|
||||
|
||||
## Judgement call: the allow-list is not "public routes"
|
||||
|
||||
The ticket's literal framing — every route "goes through one of the authz wrappers …
|
||||
or appears in an explicit, named allow-list of deliberately-public routes" — doesn't fit
|
||||
this codebase as read. Only 16 of the app's 47 routes go through one of the five admin
|
||||
wrappers. The other 31 are not uniformly public:
|
||||
|
||||
- **10 are genuinely public** — orchestrator health probes and static/reference demo
|
||||
data (`SeedData`, the DUO/BRP fixtures, the scholing-threshold config value, the
|
||||
feature-flag catalog, `/me`'s reflection of the caller's own capabilities) that reads
|
||||
the same for every caller in this one-seeded-citizen POC.
|
||||
- **19 are ownership-scoped inline**, not public and not wrapper-gated: `GET
|
||||
/applications/{id}`, the upload endpoints, every brief transition, etc. all key off
|
||||
`ctx.Zorgverlener().Bsn` / `ctx.Caller()` — an authenticated citizen (or, for the
|
||||
uploads-content endpoint, a behandelaar) reading or writing only their own resource.
|
||||
Calling these "public" in an allow-list would misrepresent exactly the property
|
||||
BIO-004 was about — object-level authorization existing at all.
|
||||
- **1 (`POST /zgw/notificaties`) uses a different mechanism entirely** — a fixed-time
|
||||
shared-secret comparison for a non-Principal external caller (OpenZaak's
|
||||
notifications), audited the same way but never going through `Authz`.
|
||||
- **1 (`POST /brief/reset`) is deliberately, literally unguarded** — the endpoint's own
|
||||
pre-existing comment says so ("No guards — showcase affordance only").
|
||||
|
||||
The allow-list (`RouteInventoryTests.AllowList`) keeps all 31 as one array for the test's
|
||||
sake, but every entry carries its own reason string rather than a blanket "public" label —
|
||||
preserving BIO-016's actual intent ("makes 'this endpoint is public' a decision someone
|
||||
wrote down rather than an omission") generalised to "this endpoint's access boundary is
|
||||
_X_, deliberately," which is true of all 31 and false of "public" for 20 of them. This is
|
||||
recorded here rather than silently reinterpreted, per this task's brief: implementing the
|
||||
literal "public" framing would have been actively misleading about which endpoints have no
|
||||
access control at all.
|
||||
|
||||
## Other judgement calls
|
||||
|
||||
- **`AuthzGateMetadata` and its extension method are `public`, not `internal`.** The test
|
||||
project has no `InternalsVisibleTo` wired up for `BigRegister.Api` (checked — none
|
||||
exists anywhere in `backend/`), and adding one for a single marker type was more
|
||||
machinery than the alternative. Both types carry a comment stating why.
|
||||
- **A second test (`Every_gate_marker_names_a_known_admin_wrapper`) guards against a typo
|
||||
in a `.Gate(...)` call.** Without it, a call like `.Gate("CasesAdmn")` would just fall
|
||||
through to "unaccounted for" in the main test with a less specific failure message —
|
||||
fine, but a dedicated assertion names the actual mistake.
|
||||
- **The main test also asserts the reverse direction: no stale allow-list entries.** An
|
||||
allow-list entry for a route that was renamed or removed is exactly the kind of drift
|
||||
a "decision someone wrote down" ledger needs to catch, not just silently keep. Verified
|
||||
this fires: temporarily added one extra `AllowList` entry for a route that doesn't
|
||||
exist (via Edit, not committed) — every real route was still covered, so only the
|
||||
stale-entry assertion tripped, naming exactly that bogus entry. Reverted the same way.
|
||||
- **`RouteInventoryTests` uses the house `TestWebApplicationFactory` + `IClassFixture`
|
||||
idiom**, not a bare `new WebApplicationFactory<Program>()` per test. The first draft did
|
||||
the latter and immediately hit `SQLite Error 1: 'table "Applications" already exists'`
|
||||
— `Db.ConnectionString` (`Data/Db.cs`) is a shared mutable **static** field, and a bare
|
||||
factory doesn't override `ConnectionStrings:AppDb`, so two such factories in the same
|
||||
class end up pointed at the same file, and the second one's `Migrate()` collides with
|
||||
the first's already-created tables (the first factory's default `Dispose()` doesn't
|
||||
delete that file — only `TestWebApplicationFactory`'s override does, to its own
|
||||
per-instance temp path). This is exactly the hazard `TestWebApplicationFactory`'s own
|
||||
doc comment describes; switching to it (as every other endpoint-test class in this
|
||||
suite already does) fixed it outright — no product code involved, purely a test-fixture
|
||||
choice.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Proved the test is hard to fool**, per the ticket's explicit ask: added a throwaway
|
||||
`api.MapDelete("/rb12-throwaway-unguarded/{id}", …)` with no `.Gate(...)` and no
|
||||
allow-list entry (via Edit, not `git checkout`) — `Every_mapped_route_is_authz_gated_or_
|
||||
on_the_named_allow_list` failed red, naming exactly that route. Reverted the same way;
|
||||
re-ran green. Repeated once more after switching to `TestWebApplicationFactory` to
|
||||
confirm the fixture change didn't weaken the check — same red, same green.
|
||||
- `dotnet build` (both `src/BigRegister.Api` and `tests/BigRegister.Tests`): clean, 0
|
||||
warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **257 passed, 0 failed** (255
|
||||
pre-existing + 2 new).
|
||||
@@ -0,0 +1,186 @@
|
||||
# RB-13 — land `Session → Principal`; `MedewerkerAdapter`; the backoffice login stops being a DigiD/BSN form
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `06-adr-conformance.md` ADR-C-004 · `00-baseline.md` BL-002 · `docs/reference/architecture/0002-user-groups-and-bounded-contexts.md` §3, "Known debt" · `99-backlog.md` RB-13
|
||||
|
||||
## What was wrong
|
||||
|
||||
ADR-0002 §3 ("Separate identity from authorization") specifies a discriminated
|
||||
`Principal` union — `{ kind: 'zorgverlener'; bsn; naam } | { kind: 'medewerker';
|
||||
medewerkerId; naam; rollen }` — as "the one concrete FE change when actor #2 lands."
|
||||
Actor #2 (`apps/behandelportal`) landed in WP-61/67; the union did not follow.
|
||||
|
||||
Verified before this ticket:
|
||||
|
||||
- `grep -rn "Principal" apps libs` returned exactly one hit — a comment in
|
||||
`libs/shared/src/infrastructure/role.ts:8`. No such type existed.
|
||||
- `apps/ssp/src/app/auth/domain/session.ts` and
|
||||
`apps/behandelportal/src/app/auth/domain/session.ts` were byte-identical:
|
||||
`interface Session { readonly bsn: string; readonly naam: string }` — a Behandelaar
|
||||
carrying a `bsn`, which §3 names as precisely the state the union exists to make
|
||||
unrepresentable.
|
||||
- `apps/behandelportal/src/app/auth/ui/login.page.ts` rendered `intro="Log in op uw
|
||||
persoonlijke BIG-register omgeving."` and called `SessionStore.login(bsn)` →
|
||||
`DigidAdapter.authenticate(bsn)`, resolving `{ bsn: r.value, naam: 'Dr. A. (Anna) de
|
||||
Vries' }` — a backoffice employee logging into the backoffice as a citizen, by DigiD,
|
||||
under a citizen's name.
|
||||
- `apps/behandelportal/src/app/auth/infrastructure/medewerker.interceptor.ts` already
|
||||
stamps every backend request with `X-Medewerker`/`X-Rollen`, independently of
|
||||
`SessionStore` — the divergence ADR-0002 predicted took this orthogonal side door
|
||||
instead of the `Principal` union, which is why the two `auth` contexts still measured
|
||||
as identical.
|
||||
- `tools/baseline-scan.mjs --dup`, measured immediately before this ticket (after
|
||||
ADR-C-006 shared the route guards): `ssp/auth` 168/168 dup lines (100.0%),
|
||||
`bhp/auth` 168/200 (84.0%) — down from the original 211/211, but the WP-67 amendment's
|
||||
"auth stays duplicated because it's expected to diverge" claim had never actually been
|
||||
tested, only asserted.
|
||||
|
||||
RB-09 (a prerequisite, landed the day before) made the backend's `IIdentityProvider`
|
||||
able to say "no identity" and fail closed; this ticket is its stated FE half — without
|
||||
it, a production behandelportal falls through to the seeded zorgverlener by default,
|
||||
open on every citizen-scoped endpoint and holding `CanRevealBigNummer`. This ticket
|
||||
does not touch that backend behaviour — it makes the FE identity model honest about
|
||||
who is actually authenticating.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `apps/ssp/src/app/auth/domain/session.ts` → `principal.ts` | `Session` → `Principal`, `{ kind: 'zorgverlener'; bsn; naam }`; `parseStoredSession` → `parseStoredPrincipal` (G1/G2 unchanged) |
|
||||
| `apps/ssp/src/app/auth/domain/session.spec.ts` → `principal.spec.ts` | renamed, updated to the `Principal`/`kind` shape |
|
||||
| `apps/ssp/src/app/auth/application/session.store.ts` | `Session` → `Principal`; header doc rewritten to state _why_ G1 applies here and not in behandelportal (cross-reference, not shared prose) |
|
||||
| `apps/ssp/src/app/auth/infrastructure/digid.adapter.ts` | resolves `{ kind: 'zorgverlener', bsn, naam }` |
|
||||
| `apps/ssp/src/app/shell/debug-state/debug-state.component.ts` | `Session` → `Principal` (the one other consumer of the domain type) |
|
||||
| `apps/behandelportal/src/app/auth/domain/session.ts` → `principal.ts` | new `medewerker` variant: `{ kind: 'medewerker'; medewerkerId; naam; rollen: readonly Rol[] }`; `parseStoredPrincipal` validates the full shape (no BSN to strip — G2 only); new `parseRollen(raw): Rol[]`, mirroring the backend's `StubIdentityProvider.ParseRollen` (comma-separated, case-insensitive, unrecognized tokens dropped) |
|
||||
| `apps/behandelportal/src/app/auth/domain/session.spec.ts` → `principal.spec.ts` | rewritten: `isAuthenticated`, `parseStoredPrincipal` (5 cases including "kind is not medewerker" and "unrecognized rol"), `parseRollen` (4 cases) |
|
||||
| `apps/behandelportal/src/app/auth/infrastructure/digid.adapter.ts` → `medewerker.adapter.ts` | **new `MedewerkerAdapter`** — resolves `MEDEWERKER_ID` + `currentRollen()` (`medewerker.ts`, unchanged) into a `Principal`; no input, returns the `Principal` directly (no `Result` — there is nothing for this stand-in to fail on) |
|
||||
| `apps/behandelportal/src/app/auth/application/session.store.ts` | `MedewerkerAdapter` replaces `DigidAdapter`; `login()` takes no argument; the whole principal round-trips through `localStorage` (no G1 field to strip); header doc rewritten, cross-referencing the SSP's instead of repeating it |
|
||||
| `apps/behandelportal/src/app/auth/ui/login-form/login-form.component.ts` | rewritten: no BSN/wachtwoord fields — one explainer line + one "Inloggen met SSO" button, `submitted = output<void>()` |
|
||||
| `apps/behandelportal/src/app/auth/ui/login.page.ts` | new heading/intro copy ("Inloggen bij het behandelportal" / "Voor medewerkers die aanvragen beoordelen."); `login()` takes no argument; the error-alert branch is gone (nothing can fail) |
|
||||
| `apps/behandelportal/src/locale/messages.en.xlf` | new id `login.ssoExplainer`; `login.submit`/`login.heading`/`login.intro` updated to the new source text + English target; `login.bsnLabel`/`bsnDescription`/`wachtwoordLabel`/`form.verplichteVelden` removed (no longer reachable from this app — confirmed by grep and by a trial `extract-i18n:behandelportal` run) |
|
||||
| `libs/shared/src/infrastructure/subject.ts`, `subject.interceptor.ts` | doc comments: `` `Session.bsn` `` → `` `Principal.bsn` `` (the type these comments cite renamed; the design they describe — `libs/shared` can't reach an app-local `auth` context, so `?subject=` exists instead — is unchanged) |
|
||||
| `docs/reference/architecture/0002-user-groups-and-bounded-contexts.md` | new "Amendment (RB-13, 2026-08-27)" replacing the "Known debt" section it closes out; records what landed and the re-measured duplication figure |
|
||||
| `libs/shared/docs/behaviour-spec.mdx` | regenerated (`npm run gen:behaviour-spec`) — reflects the renamed spec titles and the new `parseRollen`/medewerker `parseStoredPrincipal` cases |
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Each app's `Principal` holds only the one variant it has an actor for**, not the
|
||||
full two-member union ADR-0002 §3 writes as a single illustrative type. The ADR's own
|
||||
proposed resolution under ADR-C-004 says this explicitly ("In `apps/behandelportal`:
|
||||
replace `Session` with the `medewerker` variant … In `apps/ssp`: the `zorgverlener`
|
||||
variant"), and it matches how the codebase already splits `auth` per app. `kind` stays
|
||||
on both single-member types anyway — it is what makes the two types genuinely
|
||||
different rather than a same-shaped coincidence, and it is where a third actor (§4 —
|
||||
admin/auditor/institution-rep) would add a member.
|
||||
- **`MedewerkerAdapter.authenticate()` returns `Promise<Principal>`, not
|
||||
`Promise<Result<string, Principal>>`.** The first draft mirrored `DigidAdapter`'s
|
||||
`Result`-returning shape for symmetry, but that `Result`'s error variant could never
|
||||
actually be produced — there is no credential to check, so wrapping the return in a
|
||||
type that claims to have a failure mode was itself a small instance of the thing
|
||||
CLAUDE.md §3 warns against (representing a state that can't happen). Reverted to a
|
||||
direct `Promise<Principal>` and dropped the now-dead error-handling branch from
|
||||
`login.page.ts` (`error` signal, the `<app-alert type="error">`, the `AlertComponent`
|
||||
import) — a real SSO integration is where that branch would come back, not before.
|
||||
This was also the change that did the most to bring the duplication figure down (see
|
||||
below): `login.page.ts`'s 7-window overlap with the SSP's disappeared once the two
|
||||
pages' control flow, not just their copy, actually differed.
|
||||
- **`rollen` is typed `readonly Rol[]` with `Rol = 'behandelaar'`, and `parseRollen`
|
||||
lives in `domain/`, not the adapter.** The raw `currentRollen()` stand-in returns an
|
||||
unvalidated string (`medewerker.ts`, untouched by this ticket); turning it into typed
|
||||
`Rol[]` is pure string logic with no Angular dependency, so it belongs in
|
||||
`domain/principal.ts` per CLAUDE.md §1's layer table — the adapter (`infrastructure/`)
|
||||
stays a thin wire-up that only reaches for `MEDEWERKER_ID`/`currentRollen()` and
|
||||
hands them to a pure function. `parseRollen` deliberately mirrors the backend's own
|
||||
`StubIdentityProvider.ParseRollen` (comma-separated, unrecognized tokens dropped, so
|
||||
`?rollen=geen` yields `[]`) — this is not the FE recomputing a business rule
|
||||
(ADR-0001's boundary is about _authorization decisions_, which still come only from
|
||||
`GET /me`/`AccessStore`); it is the FE's own dev-only identity stand-in echoing the
|
||||
same header value it is about to send, for display, the same way `DigidAdapter`
|
||||
already fabricates its own fake identity.
|
||||
- **`SessionStore` (bhp) persists the whole `Principal` to `localStorage`, not a
|
||||
stripped-down `{ naam }` copy.** The SSP's G1 guarantee ("never persist the BSN")
|
||||
doesn't apply here — a `medewerker` principal has no national identifier — so there is
|
||||
nothing to strip. `parseStoredPrincipal` validates the full shape (G2 only) and
|
||||
restores it as-is. This was a deliberate choice against an alternative: reconstructing
|
||||
`medewerkerId`/`rollen` from the live `MEDEWERKER_ID`/`currentRollen()` on every
|
||||
restore, which would have made `domain/principal.ts` depend on
|
||||
`infrastructure/medewerker.ts` — backwards per CLAUDE.md §1's inward-only dependency
|
||||
rule, and it would have made `parseStoredPrincipal` impure. Consequence: changing
|
||||
`?rollen=` mid-session does not retroactively change an already-restored `Principal`
|
||||
until the next `login()`/`logout()` — the same way changing the DigiD demo BSN
|
||||
requires a fresh login in the SSP. The backend's own authorization is unaffected
|
||||
either way, since `medewerkerInterceptor` reads `currentRollen()` fresh on every HTTP
|
||||
request regardless of what `SessionStore` holds.
|
||||
- **Session/store class names (`SessionStore`, `SESSION_PORT`, `SessionPort`) were left
|
||||
unchanged.** ADR-0002 §3's own Consequences section names `SessionStore` — alongside
|
||||
`auth.guard.ts` — as one of the _seams that localise_ the `Session → Principal` change,
|
||||
not as something the change renames. `libs/shared/src/application/session.port.ts`'s
|
||||
`SessionPort` (ADR-C-006) is unaffected: it only ever exposed `{ naam }` and
|
||||
`isAuthenticated`, neither of which is `kind`-dependent.
|
||||
- **`libs/shared/src/infrastructure/subject.ts`/`subject.interceptor.ts` doc comments
|
||||
updated, code untouched.** Both cite `` `Session.bsn` `` by name to explain why
|
||||
`?subject=` exists instead of reading the store directly; renaming the type these
|
||||
comments describe without updating the comment would have left them citing a type
|
||||
that no longer exists.
|
||||
- **`auth.guard.ts`'s verbatim re-export in both apps was left alone.** ADR-C-006 is
|
||||
explicit that a route guard is actor-agnostic and out of ADR-0002 §3's scope — it
|
||||
reads only `SESSION_PORT`/`AccessStore`, never `Principal`, so there was nothing for
|
||||
this ticket to change there.
|
||||
- **No backend change.** RB-09 already made `IIdentityProvider` nullable and
|
||||
Production-fail-fast; this ticket is purely the frontend counterpart it named. The
|
||||
residual RB-09 flagged (`GET /uploads/{documentId}/content`'s plain-navigation
|
||||
callers carrying no identity header once a real, non-stub `IIdentityProvider` exists)
|
||||
is unaffected by anything here — it is about a _future_ real provider replacing the
|
||||
Development-only stub, which this ticket does not touch.
|
||||
|
||||
## Duplication, measured (`tools/baseline-scan.mjs --dup`)
|
||||
|
||||
| When | `ssp/auth` dup lines | `bhp/auth` dup lines |
|
||||
| ----------------------------------- | -------------------: | -------------------: |
|
||||
| Before ADR-C-006 (baseline, BL-002) | 211/211 (100%) | — |
|
||||
| After ADR-C-006, before this ticket | 168/168 (100.0%) | 168/200 (84.0%) |
|
||||
| **After this ticket** | **32/179 (17.9%)** | **32/259 (12.4%)** |
|
||||
|
||||
Expected by the backlog: "<40 after this." Measured: **32 lines each side** — under
|
||||
target. The full clone-pair listing (the script's own output truncates to the top 15
|
||||
pairs repo-wide; re-run with the pair filter widened to confirm nothing auth-related was
|
||||
hiding below that cut) resolves to exactly four remaining pairs:
|
||||
|
||||
- `principal.spec.ts` (6 windows) — both files test the same G2 "validate before
|
||||
trusting a stored shape" concept with a parallel `describe`/`it` structure (including
|
||||
the shared `import { describe, it, expect } from 'vitest';` line); the assertions
|
||||
themselves differ (BSN-stripping vs. kind/rollen validation).
|
||||
- `login-form.stories.ts` (3 windows) — the generic Storybook `Meta`/`StoryObj`/`Default`
|
||||
scaffold, unavoidable for any two co-located `.stories.ts` files regardless of subject.
|
||||
- `auth.guard.ts` (2 windows) — the intentional verbatim re-export (ADR-C-006); this is
|
||||
meant to stay identical.
|
||||
- `session.store.ts` (1 window) — down from 33 windows before this ticket to one small
|
||||
shared fragment (the `@Injectable`/signal/`asReadonly`/`computed` wiring any root
|
||||
singleton store in this codebase shares).
|
||||
|
||||
None of what remains is re-converged identity or login-flow logic — the domain type,
|
||||
the adapter, and the login UI all now differ in kind, not just in copy. §3's prediction
|
||||
("the two groups authenticate differently") has been tested for the first time by this
|
||||
ticket, not just asserted, and it held.
|
||||
|
||||
## Verification
|
||||
|
||||
Confirmed each non-trivial change is red without its fix (edited in place, verified red,
|
||||
edited back — never `git checkout`):
|
||||
|
||||
- **ssp `parseStoredPrincipal` (G1):** changed `bsn: ''` to `bsn: parsed.bsn ?? ''` →
|
||||
`G1: a stored bsn is never restored…` failed with `expected { bsn: '19012345601', …}
|
||||
to deeply equal { bsn: '', … }`. Reverted; all other tests unaffected.
|
||||
- **bhp `parseStoredPrincipal` (kind guard):** dropped the `parsed?.kind === 'medewerker'`
|
||||
clause → `returns null when kind is not medewerker` failed, returning the parsed
|
||||
zorgverlener-shaped object instead of `null`. Reverted.
|
||||
- **bhp `parseRollen`:** dropped `.filter(isRol)` → `drops unrecognized tokens` and
|
||||
`returns an empty list for an empty string` both failed (`['geen']`/`['']` returned
|
||||
instead of `[]`). Reverted.
|
||||
|
||||
`npm test` (both apps + both libraries): all green, 258 (ssp) + 37 (behandelportal) +
|
||||
133 (shared) + 23 (beheer) tests passing, including the new/renamed auth specs.
|
||||
`npm run lint`: clean. `npm run dep:check`: 0 violations, both apps. `ng build ssp
|
||||
--localize` and `ng build behandelportal --localize`: both succeed (the new
|
||||
`login.ssoExplainer` id and the updated `login.submit`/`login.heading`/`login.intro`
|
||||
sources all resolve to an English `<target>`). `npm run ci`: green (see the commit this
|
||||
doc ships with).
|
||||
@@ -0,0 +1,57 @@
|
||||
# RB-14 — scan the .NET dependency tree for known advisories
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-016 · `99-backlog.md` RB-14
|
||||
|
||||
## What was wrong
|
||||
|
||||
`npm audit --omit=dev` gates the shipped frontend bundle. Nothing equivalent existed for the
|
||||
backend, so **the entire .NET dependency tree — direct and transitive — was unscanned.** BIO-016
|
||||
lists it first under "Absent".
|
||||
|
||||
## The trap the ticket walked into
|
||||
|
||||
The backlog row says: `dotnet list package --vulnerable --include-transitive` **as a failing
|
||||
step**. Implemented literally, that step cannot fail. `dotnet list package --vulnerable` is a
|
||||
_reporting_ command: it prints the advisory table and exits 0 regardless.
|
||||
|
||||
Verified rather than assumed — a throwaway project with `System.Net.Http 4.3.0`:
|
||||
|
||||
```
|
||||
Project `vulntest` has the following vulnerable packages
|
||||
> System.Net.Http 4.3.0 4.3.0 High https://github.com/advisories/GHSA-7jgj-8wvc-jh57
|
||||
EXITCODE=0
|
||||
```
|
||||
|
||||
A **High** severity advisory, exit code **0**. A bare `- run: dotnet list package --vulnerable`
|
||||
would have added a line to `ci.yml` that reads like coverage in a compliance review and enforces
|
||||
nothing — which is worse than leaving the gap visible.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------- | -------------------------------------------------------------------------- |
|
||||
| `scripts/dotnet-audit.sh` | **new** — runs the scan, matches its output, exits 1 on a hit |
|
||||
| `.github/workflows/ci.yml` | new backend step calling the script (same `changes.outputs.backend` guard) |
|
||||
| `scripts/ci-local.sh` | new `backend dependency audit` step calling the same script |
|
||||
|
||||
**One script, two callers**, rather than the same four lines pasted into a workflow and a shell
|
||||
script that would then drift. The guard matches `has the following vulnerable packages` — the
|
||||
exact sentence `dotnet list` prints per project on a hit; the clean case prints
|
||||
`has no vulnerable packages given the current sources` instead.
|
||||
|
||||
## Verification
|
||||
|
||||
- Against the real solution: passes, both projects clean (exit 0).
|
||||
- Against the marker sentence `dotnet list` actually emits: the guard fires and exits 1.
|
||||
- The exit-0-on-High behaviour that motivates the whole script is reproduced above.
|
||||
|
||||
## Residual
|
||||
|
||||
`--include-transitive` means a vulnerable package pulled in by a dependency turns CI red with no
|
||||
direct upgrade available. The fix in that case is a direct `PackageReference` pinning a patched
|
||||
version; the script's failure message says so. There is deliberately **no severity threshold and
|
||||
no suppression list** — adding one before a real advisory forces the question would be guessing
|
||||
at a policy nobody has needed yet.
|
||||
|
||||
Secret scanning (gitleaks/trufflehog), BIO-016's other named absence, is **not** in this ticket
|
||||
and remains on the pre-production checklist.
|
||||
@@ -0,0 +1,93 @@
|
||||
# RB-15 — Swagger and the OpenAPI document behind `IsDevelopment()`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-015 · `99-backlog.md` RB-15
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs:145-146` (pre-change) ran `app.UseSwagger(); app.UseSwaggerUI();`
|
||||
unconditionally — the full OpenAPI document (every route, every request/response shape)
|
||||
and SwaggerUI's interactive "Try it out" were reachable in every environment, including a
|
||||
real deployment, with no `app.Environment.IsDevelopment()` guard. BIO-015's own evidence
|
||||
notes this is one line of genuine attack-surface reduction with no POC cost.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------- | -------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` | `app.UseSwagger(); app.UseSwaggerUI();` now run only inside `if (app.Environment.IsDevelopment()) { … }` |
|
||||
| `tests/BigRegister.Tests/SwaggerGateTests.cs` | **new** — asserts `/swagger/v1/swagger.json` is served in Development and 404s outside it |
|
||||
|
||||
`builder.Services.AddSwaggerGen(...)` and `AddEndpointsApiExplorer()` were left
|
||||
unconditional — they only register DI services (the swagger-generation machinery),
|
||||
expose nothing over HTTP by themselves, and (see below) are exactly what `npm run
|
||||
gen:api` depends on staying registered in every environment it might run against.
|
||||
|
||||
## The hazard, checked rather than assumed
|
||||
|
||||
RB-09 made a non-Development environment throw during `builder.Build()` (no
|
||||
`IIdentityProvider` registered for a bare/unset environment, which defaults to
|
||||
Production), which crashed `dotnet swagger tofile` until `package.json`'s `gen:api`
|
||||
script was pinned to `ASPNETCORE_ENVIRONMENT=Development` for that one invocation
|
||||
(`docs/.../implementation/rb-09.md`). This ticket's change sits in exactly the same
|
||||
pipeline, so it needed the same empirical check, not an assumption.
|
||||
|
||||
**Mechanism, confirmed by reading Swashbuckle's CLI behaviour and then proving it:**
|
||||
`dotnet swagger tofile` (`Swashbuckle.AspNetCore.Cli`) loads the built DLL through
|
||||
.NET's design-time `HostFactoryResolver`, builds the host, and then resolves
|
||||
`ISwaggerProvider` **directly out of the DI container** to produce `swagger.json` — it
|
||||
never issues an HTTP request through the ASP.NET Core middleware pipeline this ticket's
|
||||
`if (app.Environment.IsDevelopment())` guard lives in. Gating `UseSwagger()`/
|
||||
`UseSwaggerUI()` therefore cannot affect it, in any environment, by construction — those
|
||||
are pipeline middleware; the CLI tool bypasses the pipeline entirely.
|
||||
|
||||
**Verified, not assumed:** ran `npm run gen:api` for real. It exited 0, printed "Swagger
|
||||
JSON/YAML successfully written to …/backend/swagger.json", and regenerated the NSwag
|
||||
client. `git status`/`git diff` on both `backend/swagger.json` and
|
||||
`libs/shared/src/infrastructure/api-client.ts` showed **zero changes** — the regenerated
|
||||
files are byte-identical to what's already committed, confirming the gate has no effect
|
||||
on the generated contract at all.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **The guard wraps both `UseSwagger()` and `UseSwaggerUI()` together**, not just one —
|
||||
the ticket's own wording lists both, and gating only the document while leaving the UI
|
||||
reachable (or vice versa) would be a strange half-measure: SwaggerUI without the
|
||||
document 404s on load anyway, and the document without the UI still leaks the same
|
||||
route/shape enumeration BIO-015 is about.
|
||||
- **`AddSwaggerGen`/`AddEndpointsApiExplorer` were left unconditional.** They're
|
||||
DI-registration-time calls with no HTTP surface, and — now confirmed rather than
|
||||
assumed — `dotnet swagger tofile` needs `ISwaggerProvider` registered in whatever
|
||||
environment it runs the host under (pinned to Development by `gen:api`'s own script,
|
||||
but nothing stops a future non-Development invocation), so conditioning those
|
||||
registrations on `IsDevelopment()` would risk breaking the CLI tool for no
|
||||
attack-surface benefit — nobody can reach a DI-registered-but-never-routed service
|
||||
over HTTP.
|
||||
- **New tests build the "non-Development" case on a third environment name
|
||||
("Staging"), not `"Production"`.** RB-09 already made Production fail at startup
|
||||
entirely (no real `IIdentityProvider` exists yet) — a stronger guarantee than "no
|
||||
Swagger in Production," but one that means a `UseEnvironment("Production")` host
|
||||
never reaches this middleware to prove the gate itself works; it only proves RB-09's
|
||||
unrelated startup throw, which already has its own test. A `"Staging"` environment
|
||||
satisfies neither `IsDevelopment()` nor `IsProduction()`, so `Program.cs` registers no
|
||||
`IIdentityProvider` for it — the test supplies one via `ConfigureTestServices`
|
||||
(`StubIdentityProvider`, the same one Development uses) so the host actually boots,
|
||||
and the test exercises this ticket's real gate rather than a different ticket's.
|
||||
- **The Staging host is built via `factory.WithWebHostBuilder(...)`** (layering on the
|
||||
shared `TestWebApplicationFactory` fixture), not a bare `new
|
||||
WebApplicationFactory<Program>()` — RB-12's implementation note already records the
|
||||
"table already exists" collision a bare factory hits by sharing the mutable static
|
||||
`Db.ConnectionString` instead of the fixture's own per-class isolated temp path;
|
||||
layering avoids repeating that mistake here.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Reverted the guard only** (unwrapped `UseSwagger()`/`UseSwaggerUI()` back to
|
||||
unconditional, via Edit, tests left in place) and ran `SwaggerGateTests`:
|
||||
`Swagger_document_is_not_served_outside_development` failed red (`Expected: NotFound,
|
||||
Actual: OK`). Restored the fix (via Edit) and re-ran: both green.
|
||||
- **`npm run gen:api`, run for real**: exit 0; `backend/swagger.json` and
|
||||
`libs/shared/src/infrastructure/api-client.ts` both unchanged (`git status` clean on
|
||||
both) — see "The hazard" above.
|
||||
- `dotnet build` (both projects): clean, 0 warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **259 passed, 0 failed** (257 + 2 new).
|
||||
@@ -0,0 +1,82 @@
|
||||
# RB-16 — `DateOnly.TryParse` on `?peildatum=`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-019 · `99-backlog.md` RB-16
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs:215` (pre-change) —
|
||||
`var rows = peildatum is { Length: > 0 } p ? t.RowsOn(DateOnly.Parse(p)) : t.Rows();`.
|
||||
`DateOnly.Parse` throws `FormatException` on anything unparseable; there was no
|
||||
`TryParse`, no 400 path, and `.Produces` on the endpoint declared only 200/403/404 — so
|
||||
an unparseable `?peildatum=` value 500'd, and in Development the exception detail was
|
||||
returned to the caller. §3c's baseline named `backend/Stamdata` 96.8% line but **71.7%
|
||||
branch** (BL-005) — this was one of the unentered branches.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` — `GET /stamdata/{table}` | `DateOnly.Parse` replaced with `DateOnly.TryParse`; an unparseable value now returns `Results.Problem(detail: …, statusCode: 400)` instead of throwing; endpoint doc gained `.ProducesProblem(StatusCodes.Status400BadRequest)` |
|
||||
| `tests/BigRegister.Tests/StamdataEndpointTests.cs` | **new** `Unparseable_peildatum_is_400_not_500` |
|
||||
| `backend/swagger.json`, `libs/shared/src/infrastructure/api-client.ts` | regenerated (`npm run gen:api`) — the new 400 response is now part of the documented contract |
|
||||
|
||||
## What the fix looks like
|
||||
|
||||
```csharp
|
||||
DateOnly? peildatumWaarde = null;
|
||||
if (peildatum is { Length: > 0 } p)
|
||||
{
|
||||
if (!DateOnly.TryParse(p, out var parsed))
|
||||
return Results.Problem(detail: $"Ongeldige peildatum '{p}'.", statusCode: StatusCodes.Status400BadRequest);
|
||||
peildatumWaarde = parsed;
|
||||
}
|
||||
var rows = peildatumWaarde is { } d ? t.RowsOn(d) : t.Rows();
|
||||
```
|
||||
|
||||
Matches the shape every other bad-input check in this file already uses (e.g. the
|
||||
upload endpoint's `Results.Problem(detail: …, statusCode: 400)` for a malformed
|
||||
multipart request) — a `Results.Problem` with a Dutch detail message, not a bespoke
|
||||
response shape.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **No FE change needed, and none made.** `libs/beheer/src/infrastructure/
|
||||
stamdata.adapter.ts`'s `load()` already routes every call through `runSubmit`
|
||||
(`libs/shared/src/application/submit.ts`), which try/catches any thrown
|
||||
`ApiException` — including the client's new 400 branch — into a generic `Result`
|
||||
error string via `problemDetail`. There is no status-code-specific branching to
|
||||
extend; ADR-0001's "the FE renders the decision, it does not recompute the rule"
|
||||
already covers "the server rejected this input" as a case the generic error path
|
||||
handles, same as the existing 403.
|
||||
- **Regenerated the API client and committed it in this ticket's diff**, rather than
|
||||
leaving it to drift. RB-09's implementation note records a real prior incident where
|
||||
a response-shape change (RB-08's 403 → `ProducesProblem`) landed without a
|
||||
regeneration and the drift went unnoticed until the next ticket's `gen:api` run. This
|
||||
ticket's `.ProducesProblem(400)` is exactly that same category of change, so
|
||||
`npm run gen:api` was run immediately as part of implementing it, not deferred.
|
||||
- **The Dutch detail message follows the file's own convention** (`$"Ongeldige
|
||||
peildatum '{p}'."`) rather than English — every other `Results.Problem(detail: …)`
|
||||
call in `Program.cs` (change-request rejection, upload validation, submit rejection)
|
||||
is Dutch; this is server-internal wire text, not `$localize`-wrapped UI copy (the FE
|
||||
never renders it verbatim — CLAUDE.md's `$localize` rule is about user-facing copy
|
||||
the FE owns, not backend `ProblemDetails.detail` strings), so no locale entry was
|
||||
needed.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Reverted the fix only** (`DateOnly.Parse` restored, ternary un-nested, via Edit —
|
||||
test left in place) and ran `StamdataEndpointTests`:
|
||||
`Unparseable_peildatum_is_400_not_500` failed red (`Expected: BadRequest, Actual:
|
||||
InternalServerError` — confirming the endpoint really did 500, not some other status).
|
||||
Restored the fix (via Edit) and re-ran: all 6 tests in the class green.
|
||||
- `dotnet build`: clean, 0 warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **260 passed, 0 failed** (259 + 1
|
||||
new).
|
||||
- `npm run gen:api`: exit 0; `backend/swagger.json` gained the 400 response shape for
|
||||
this one endpoint; `libs/shared/src/infrastructure/api-client.ts` gained the
|
||||
matching `status === 400` branch. Both regenerated files committed alongside the
|
||||
code change.
|
||||
- `npm test` (all four Vitest projects — ssp/behandelportal/shared/beheer): **445
|
||||
passed, 0 failed**, confirming the regenerated client doesn't break any existing FE
|
||||
consumer of `stamdataTable(...)`.
|
||||
@@ -0,0 +1,130 @@
|
||||
# RB-17 — split `runResult` (fold) from `runSubmit` (fold + idempotency mint)
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-003 + CQ-005 ·
|
||||
`00-baseline.md` BL-007 (+ its §10 amendment) · `99-backlog.md` RB-17
|
||||
|
||||
## What was wrong
|
||||
|
||||
`runSubmit` (`libs/shared/src/application/submit.ts`) did two things in one function: fold a
|
||||
call into a `Result`, and mint an Idempotency-Key for it
|
||||
(`withIdempotencyKey(crypto.randomUUID(), fn)`). Its own docstring called that mint "the one
|
||||
place a logical submit's Idempotency-Key is minted". Five call sites are reads and had no
|
||||
business minting one:
|
||||
|
||||
| Adapter | Method | Wire call |
|
||||
| ---------------------------- | -------------------- | --------------------- |
|
||||
| `brief.adapter.ts:56` | `load()` | `briefGET()` |
|
||||
| `org-template.adapter.ts:42` | `list()` | `orgTemplates()` |
|
||||
| `org-template.adapter.ts:54` | `load(subOrgId)` | `orgTemplateGET(...)` |
|
||||
| `stamdata.adapter.ts:27` | `list()` | `stamdataTables()` |
|
||||
| `stamdata.adapter.ts:42` | `load(tableId, ...)` | `stamdataTable(...)` |
|
||||
|
||||
That is **exactly five** — verified by grepping every `runSubmit` call site in
|
||||
`libs/shared/src/application` plus the `brief` and `beheer` scopes (13 call sites total) and
|
||||
reading each one's wire call for a request body / non-GET verb. The other 8 are genuine
|
||||
writes (`brief.adapter.ts` save/submit/approve/reject/send/reset,
|
||||
`org-template.adapter.ts` save/publish/rollback) and stay on `runSubmit` unchanged.
|
||||
|
||||
`stamdata.adapter.ts`'s own module docstring already said "Both endpoints are reads … There
|
||||
is no write method" while both called `runSubmit` — the sharpest instance of the mismatch,
|
||||
and the one BL-007's original "~13 mutations" count mis-classified because the count was
|
||||
derived from the helper's name, not from what the call actually does.
|
||||
|
||||
**Not this ticket, seen while auditing:** `ApplicationsStore.cancel`, `AdminCasesStore.delete`
|
||||
(RB-20) and `FeatureFlagStore.set` reach `ApiClient` more directly; the baseline's §10
|
||||
amendment flags these as writes the original "~13" count missed. Grepping confirms
|
||||
`FeatureFlagStore.set` (`libs/shared/src/application/feature-flags.store.ts:63`) already
|
||||
calls `runSubmit` correctly and returns a `Result` — it is not broken, just outside this
|
||||
ticket's five. `ApplicationsStore.cancel`/`AdminCasesStore.delete` were not touched; they are
|
||||
RB-20's.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `libs/shared/src/application/submit.ts` | Split: `runResult` = the try/catch + `problemDetail` fold, no mint. `runSubmit` = `runResult` wrapping `withIdempotencyKey`. |
|
||||
| `libs/shared/src/application/submit.spec.ts` | Specs for both, including one that would catch a read minting a key again (see below). |
|
||||
| `apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` | `load()` → `runResult`; docstring updated to name both halves. |
|
||||
| `apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts` | `list()`, `load()` → `runResult`. |
|
||||
| `libs/beheer/src/infrastructure/stamdata.adapter.ts` | `list()`, `load()` → `runResult`; import trimmed to `runResult` only (no writes in this file). |
|
||||
| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the new `runResult` describe block and the extra `runSubmit` case. |
|
||||
|
||||
`runSubmit`'s new body is exactly the minimal composition the ticket asked for:
|
||||
|
||||
```ts
|
||||
export function runSubmit<T>(fn: () => Promise<T>, fallback: string): Promise<Result<string, T>> {
|
||||
return runResult(() => withIdempotencyKey(crypto.randomUUID(), fn), fallback);
|
||||
}
|
||||
```
|
||||
|
||||
Zero behaviour change for the 8 write call sites — same fold, same mint, same timing (the key
|
||||
is still minted before `fn` runs and cleared in `withIdempotencyKey`'s `.finally`). The five
|
||||
reads now run the fold with no `pendingIdempotencyKey` touched at all.
|
||||
|
||||
## The spec that would catch a regression
|
||||
|
||||
`currentIdempotencyKey()` (`api-client.provider.ts`) returns the pending key while one is
|
||||
"in flight" for the duration of a `withIdempotencyKey` call, and a fresh `crypto.randomUUID()`
|
||||
on every call otherwise. That gives a real, mock-free way to assert "no key was minted": call
|
||||
`currentIdempotencyKey()` twice inside the function passed to `runResult`/`runSubmit` — two
|
||||
different reads means no pending key existed (each fell back to its own random UUID); two
|
||||
equal reads means one pending key was minted and reused.
|
||||
|
||||
```ts
|
||||
it('mints no Idempotency-Key — the read fold', async () => {
|
||||
let first = '',
|
||||
second = '';
|
||||
await runResult(async () => {
|
||||
first = currentIdempotencyKey();
|
||||
second = currentIdempotencyKey();
|
||||
return 'x';
|
||||
}, 'fallback');
|
||||
expect(first).not.toBe(second);
|
||||
});
|
||||
```
|
||||
|
||||
This mirrors the house convention of not mocking relative imports under this repo's
|
||||
Angular/vitest setup (see `role.interceptor.spec.ts`'s comment) — it asserts on real,
|
||||
exported behaviour instead of a spy.
|
||||
|
||||
**Verified red without the fix**: temporarily changed `runResult` to also call
|
||||
`withIdempotencyKey` (i.e. reintroduced the bug it exists to prevent) and reran `ng test
|
||||
shared`. Result: `runResult > mints no Idempotency-Key — the read fold` failed
|
||||
(`expected 'd185d827-...' not to be 'd185d827-...'`), all 137 other tests stayed green. Then
|
||||
reverted the temporary edit back to the real fix (an `Edit` undo, not `git checkout`, so the
|
||||
rest of the change stayed in place) and reran — 138/138 green.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Docstring on `brief.adapter.ts`** was rewritten (it previously said only "Mutations go
|
||||
through `runSubmit`") to name `load`'s `runResult` path explicitly, since the file mixes
|
||||
both now and a future reader needs the split spelled out at the top, not just per-method.
|
||||
`org-template.adapter.ts` and `stamdata.adapter.ts`'s docstrings needed no change — neither
|
||||
named `runSubmit` specifically (`stamdata.adapter.ts`'s already correctly said "no write
|
||||
method").
|
||||
- **No new concept, per the ticket's "minimal" framing** — `runSubmit` stays exported with
|
||||
the same signature and the same call sites for the 8 real mutations; only its body changed
|
||||
to delegate.
|
||||
- **Left `runResult`'s JSDoc pointing at `runSubmit`** ("never route a read through that
|
||||
one") rather than duplicating the Idempotency-Key explanation, so the two docs stay
|
||||
synchronized by cross-reference instead of by copy.
|
||||
|
||||
## Residuals (not this ticket)
|
||||
|
||||
- RB-18 (key the `IdempotencyStore` on `{SubjectId}:{idemKey}`) is sequenced behind this one
|
||||
per `99-backlog.md` and is unaffected by this split beyond it now landing on a correctly
|
||||
write-only call set.
|
||||
- RB-20 (`ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`) is
|
||||
untouched, as scoped.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run ci` (foreground): **green** — lint, typecheck, `dep:check` (341 + 226 modules, 0
|
||||
violations), `format:check`, `check:tokens`, `check:seam`, tests (ssp 258/258, behandelportal
|
||||
31/31, shared 138/138, beheer 23/23 — 450 total), `ng build --localize` (both apps), `npm
|
||||
audit` (0 vulnerabilities), backend `dotnet test` (255/255 — the known
|
||||
`OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure did not reproduce on
|
||||
this run), `gen:snippets` drift clean, `gen:behaviour-spec` drift clean once the regenerated
|
||||
file is committed alongside the code (the local gate compares the working tree to `HEAD`, so
|
||||
it necessarily shows a diff pre-commit — this is the documented "will conflict at merge time"
|
||||
behaviour, not a defect).
|
||||
@@ -0,0 +1,115 @@
|
||||
# RB-18 — key `IdempotencyStore` on `{SubjectId}:{idemKey}`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-018 ·
|
||||
`00-baseline.md` §7 (`IdempotencyStore` listed among the 7 stores "Not behind any port"),
|
||||
agent 02's `backend/Data` note ("no `Reset()` and no TTL") · `99-backlog.md` RB-18
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Data/IdempotencyStore.cs` is a process-global `Dictionary<string, IResult>` keyed only on
|
||||
the raw `Idempotency-Key` header value. `Program.cs`'s `Submit` helper read and wrote it
|
||||
with that raw value, never composed with the caller's identity:
|
||||
|
||||
```csharp
|
||||
var idemKey = ctx.Request.Headers.TryGetValue("Idempotency-Key", out var k) && !string.IsNullOrEmpty(k)
|
||||
? k.ToString()
|
||||
: null;
|
||||
```
|
||||
|
||||
The client picks the header value. Two different callers who happen to send the same
|
||||
value shared one cache slot: the second caller's request short-circuited to the first
|
||||
caller's cached `IResult` instead of running its own submission. BIO-018 rates this
|
||||
**severity low** — the cached value is only a `ReferentieResponse` (a reference number) or
|
||||
a `ProblemDetails`, never personal data — but flags it as a defect in an access-control
|
||||
path with a trivial fix.
|
||||
|
||||
**Location check against the finding.** BIO-018 cites `Program.cs:901-909` for the read/
|
||||
write and `Data/IdempotencyStore.cs:11-27` for the store. RB-17 (landed the day before,
|
||||
same file, unrelated change) shifted line numbers; the real call sites are
|
||||
`Program.cs:994` (read) and `:1028` (write), inside the local `Submit` helper starting at
|
||||
`:991`. The store file itself is untouched by RB-17 and matches the finding's shape
|
||||
exactly. `Submit` has exactly one call site (`POST /change-requests`, `:239`) — the
|
||||
`ChangeRequestRequest` → `telefoonwijziging` endpoint — so the scoping change lands on a
|
||||
single endpoint, not the "smaller call set" RB-17 was sequenced ahead of this ticket to
|
||||
produce; RB-17 removed idempotency-key minting from 5 read call sites, none of which used
|
||||
this helper in the first place, so its ordering benefit does not change what this ticket
|
||||
touches. Reported for completeness, not as a discrepancy: RB-17's own note already scoped
|
||||
its residual to "this ticket is unaffected by this split beyond it now landing on a
|
||||
correctly write-only call set" — true, and the call set was already this one endpoint
|
||||
before and after RB-17.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `src/BigRegister.Api/Program.cs` | `Submit`'s `idemKey` is now `$"{ctx.Caller().SubjectId}:{k}"` instead of the raw header value `k.ToString()`; doc comment above `Submit` states the scoping and cites RB-18/BIO-018 |
|
||||
| `tests/BigRegister.Tests/IdempotencyTests.cs` | **new** `A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result` |
|
||||
|
||||
`ctx.Caller()` (`Domain/Authorization/CallerIdentity.cs`) is already in scope in
|
||||
`Program.cs` — `ctx.Zorgverlener()` is used elsewhere in the same file — and it throws if
|
||||
the identity middleware did not run, so this composition cannot silently fall back to an
|
||||
unscoped key. `SubjectId` is the BSN for a `ZorgverlenerCaller` and the medewerkerId for a
|
||||
`MedewerkerCaller`; either way it is stable per caller and never empty.
|
||||
|
||||
This is exactly the ticket's minimal remediation, no more: no TTL, no eviction, no bound,
|
||||
no `Reset()`, no port/interface extraction. `IdempotencyStore`'s own `ponytail:` comment
|
||||
("no TTL/eviction … an unbounded dictionary keyed on client-supplied strings is a memory
|
||||
leak at scale") is untouched — the store is still unbounded and still keyed on a
|
||||
client-supplied string, only now composed with a server-resolved one first. The comment
|
||||
stays accurate; this ticket did not touch the part it would need to correct.
|
||||
|
||||
## The test
|
||||
|
||||
`IdempotencyTests.cs` already existed (RB-17's predecessor work, not this ticket) with
|
||||
three cases exercising same-caller replay/independence. Added a fourth:
|
||||
|
||||
```csharp
|
||||
[Fact]
|
||||
public async Task A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result()
|
||||
{
|
||||
var sharedKey = Guid.NewGuid().ToString();
|
||||
|
||||
var callerARequest = ChangeRequestWithKey(sharedKey);
|
||||
callerARequest.Headers.Add("X-Subject", "111222333");
|
||||
var callerA = await _client.SendAsync(callerARequest);
|
||||
callerA.EnsureSuccessStatusCode();
|
||||
var callerABody = await callerA.Content.ReadFromJsonAsync<ReferentieResponse>();
|
||||
|
||||
var callerBRequest = ChangeRequestWithKey(sharedKey);
|
||||
callerBRequest.Headers.Add("X-Subject", "999888777");
|
||||
var callerB = await _client.SendAsync(callerBRequest);
|
||||
callerB.EnsureSuccessStatusCode();
|
||||
var callerBBody = await callerB.Content.ReadFromJsonAsync<ReferentieResponse>();
|
||||
|
||||
Assert.NotEqual(callerABody!.Referentie, callerBBody!.Referentie);
|
||||
}
|
||||
```
|
||||
|
||||
`X-Subject` is `StubIdentityProvider`'s existing header for setting the caller's BSN in a
|
||||
test (the same idiom `ApplicationTests.cs` and `UploadAccessTests.cs` use), so caller A and
|
||||
caller B are two different `ZorgverlenerCaller`s sending the identical `Idempotency-Key`.
|
||||
|
||||
**Verified red without the fix.** Reverted `Program.cs`'s `idemKey` line to
|
||||
`k.ToString()` with an `Edit` (not `git checkout`, so the rest of the working tree stayed
|
||||
intact), reran `dotnet test --filter "FullyQualifiedName~IdempotencyTests"`:
|
||||
|
||||
```
|
||||
Failed BigRegister.Tests.IdempotencyTests.A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result [4 ms]
|
||||
Error Message:
|
||||
Assert.NotEqual() Failure: Strings are equal
|
||||
Expected: Not "BIG-2026-476969"
|
||||
Actual: "BIG-2026-476969"
|
||||
Failed! - Failed: 1, Passed: 3, Skipped: 0, Total: 4
|
||||
```
|
||||
|
||||
Caller B received caller A's cached reference. Then reapplied the fix with a second
|
||||
`Edit` and reran: `Passed! - Failed: 0, Passed: 4, Skipped: 0, Total: 4`.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet test` (full suite): **261 passed, 1 failed** — the failure is
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
which needs a live OpenZaak container and fails identically on a stashed tree; it predates
|
||||
this change and is not run by `npm run ci`.
|
||||
|
||||
`npm run ci` (foreground): green — see the commit's own record for the full step list.
|
||||
@@ -0,0 +1,220 @@
|
||||
# RB-19 — reorder `Program.cs`: reads before writes per section, regroup admin-cases + org-template preview
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-006 ·
|
||||
`99-backlog.md` RB-19 · Depends on `implementation/rb-12.md` (the route-table test this
|
||||
ticket leans on as its regression net)
|
||||
|
||||
This is a **pure reorder**. No route, signature, DTO, or handler-body text changed. The
|
||||
sorted list of `HTTP METHOD + path` mapping calls is byte-identical before and after (see
|
||||
"Verification" below) — that identity is the strongest evidence this ticket did what it
|
||||
says and nothing else.
|
||||
|
||||
## What was wrong
|
||||
|
||||
CQ-006, verbatim: `Program.cs` opens by declaring direction as its organising principle
|
||||
(a "GET: screen-shaped reads" banner, then a "POST: submits" banner), then from the
|
||||
Document-upload section onward switches to feature grouping without saying so, and every
|
||||
subsequent section interleaves reads and writes. One feature (Beoordeling/Besluit, WP-65)
|
||||
already got the fix — a `:441`/`:464`-style banner pair splitting its query endpoint from
|
||||
its command endpoint — and CQ-006 asks for the same treatment on the five sections that
|
||||
predate that pattern: Document upload, Applications, Admin cases, Brief, and Organization
|
||||
templates. Separately, `DELETE /admin/cases/{id}` sat 129 lines away from `GET
|
||||
/admin/cases`, with werkvoorraad, beoordeling, besluit and the ZGW notification hook in
|
||||
between; and `GET /admin/org-template/{subOrgId}/preview` was filed under the Brief
|
||||
section's banner instead of the Org-templates section it actually belongs to.
|
||||
|
||||
## What changed
|
||||
|
||||
| Section (banner) | Before | After |
|
||||
| ---------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| Document upload | categories, **POST /uploads**, content, status, DELETE, admin-DELETE | categories, content, status, `--- reads ---`/`--- writes ---` sub-banners, **POST /uploads** moved after the reads, DELETE, admin-DELETE |
|
||||
| Applications | already reads-first (2 GETs, then POST/PUT/DELETE/POST-submit) | unchanged order; sub-banners inserted only |
|
||||
| Admin cases | GET /admin/cases, _(werkvoorraad/beoordeling/besluit/zgw-notificaties in between)_, **DELETE /admin/cases/{id}**, **GET /admin/audit** | GET /admin/cases, **GET /admin/audit** (moved up), `--- writes ---`, **DELETE /admin/cases/{id}** (moved up) — all three now contiguous; werkvoorraad/beoordeling/besluit/zgw-notificaties follow, unmoved and unchanged |
|
||||
| Brief | GET /brief, PUT, submit, approve, reject, send, reveal-bignummer, **GET /brief/preview**, _(org-template preview)_, POST /reset | GET /brief, **GET /brief/preview** (moved up beside GET /brief), `--- writes ---`, PUT, submit, approve, reject, send, reveal-bignummer, POST /reset — org-template preview removed from this section |
|
||||
| Organization templates | list, detail, PUT, publish, rollback | list, detail, **GET /admin/org-template/{subOrgId}/preview** (moved in from Brief), `--- writes ---`, PUT, publish, rollback |
|
||||
|
||||
Every section above got a `// --- reads ---` / `// --- writes ---` sub-banner pair
|
||||
(matching the short, bare form already used at the file's top-level `:170`/`:236`
|
||||
banners) inserted at the reads→writes boundary. Werkvoorraad, Beoordeling and Besluit —
|
||||
not named by CQ-006 as mixed, and already correctly split (Beoordeling is the read,
|
||||
Besluit is the write, each with its own WP-65 banner) — were left exactly as they were,
|
||||
including their absolute position relative to each other; only the block ahead of them
|
||||
(admin-cases) grew, pushing their line numbers down without touching their content.
|
||||
|
||||
`backend/swagger.json` and `libs/shared/src/infrastructure/api-client.ts` were
|
||||
regenerated (`npm run gen:api`) and are part of this commit — see "The regenerated pair"
|
||||
below.
|
||||
|
||||
## Design: line-range slicing, not manual retyping
|
||||
|
||||
Every moved block was cut with a Python script operating on exact 1-indexed line ranges
|
||||
against the file as it stood after merging in `refactor/adr-c-006-shared-route-guards`
|
||||
(this branch's actual base — see "Base commit" below), then reassembled in the new order.
|
||||
No handler body was retyped by hand. This is the same guarantee the ticket's "cut/paste,
|
||||
not retype" instruction asks for, made structural rather than a promise to be careful:
|
||||
a line-range slice cannot silently change a character inside a block it does not touch.
|
||||
The script is not part of this commit (a one-shot tool, not project code); the diff it
|
||||
produced is what is being reviewed.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Sub-banner wording is bare `// --- reads ---` / `// --- writes ---`, not prose
|
||||
matching WP-65's descriptive style.** The ticket asks for ":441/:464-style" banners;
|
||||
WP-65's actual banners are long, feature-specific paragraphs ("read side only
|
||||
(recording a decision is WP-65's second half)…"). Inventing five more paragraphs like
|
||||
that would mean writing new explanatory prose about code this ticket is not meant to
|
||||
re-explain — CQ-006 is explicit that this is "a structure finding, not a correctness
|
||||
one," and the ticket itself forbids "no fixed comments beyond the banners this ticket
|
||||
adds." The file's own top-level banners (`:170` "GET: screen-shaped reads", `:236`
|
||||
"POST: submits") already establish a bare, label-only banner as a legitimate style in
|
||||
this exact file — the sub-banners here are that same style, nested one level deeper.
|
||||
- **Werkvoorraad/Beoordeling/Besluit end up sandwiched between Admin-cases and
|
||||
zgw/notificaties, in that order, unmoved.** Moving `GET /admin/audit` and `DELETE
|
||||
/admin/cases/{id}` up next to `GET /admin/cases` (as instructed) necessarily pushes
|
||||
everything that used to sit between them — werkvoorraad, beoordeling, besluit,
|
||||
zgw/notificaties — down, but does not reorder those four relative to each other. They
|
||||
were not named as mixed by CQ-006 and were not touched beyond their line numbers
|
||||
changing.
|
||||
- **`GET /brief/preview` and `POST /uploads` are both `.ExcludeFromDescription()`-marked
|
||||
(hand-written FE `fetch`/XHR calls, never through the generated client) — moving them
|
||||
produced zero diff in `swagger.json`.** This is not a coincidence being reported as
|
||||
one: an excluded endpoint has no OpenAPI operation to reorder in the first place, so
|
||||
the regenerated pair's diff below is smaller than "every moved route" might suggest —
|
||||
it only shows the two endpoints that are both documented and reordered relative to
|
||||
each other (`GET /admin/audit`, `DELETE /admin/cases/{id}`).
|
||||
- **No handler types, no `Features/` folder, no mediator** — out of mandate per CQ-006's
|
||||
own text (filed separately as OOM-A) and the ticket's explicit "out of scope" section.
|
||||
Nothing beyond comments and mapping order changed.
|
||||
|
||||
## Base commit
|
||||
|
||||
Step zero's warning matched this worktree's actual starting state: `git log --oneline -8`
|
||||
showed `ae7781e` at HEAD, not `edd20c0`, and `edd20c0 docs(backlog): mark RB-23 done after
|
||||
merge` was absent from the log entirely — the bad-base lineage named in the ticket. `git
|
||||
merge refactor/adr-c-006-shared-route-guards` was run, after which `edd20c0` appeared as
|
||||
`HEAD~0`'s direct ancestor and every RB-01..RB-23 commit was present. All work in this
|
||||
ticket happened after that merge.
|
||||
|
||||
## Verification
|
||||
|
||||
**The sorted-route-list diff (the key evidence).** Extracted every `.Map(Get|Post|Put|
|
||||
Delete)("...")` call from `Program.cs` before and after, sorted each list, and diffed
|
||||
them:
|
||||
|
||||
```
|
||||
$ grep -oE '\.Map(Get|Post|Put|Delete)\("[^"]*"' Program.cs.before-reorder | sort > before.txt
|
||||
$ grep -oE '\.Map(Get|Post|Put|Delete)\("[^"]*"' Program.cs | sort > after.txt
|
||||
$ diff before.txt after.txt
|
||||
$ echo "exit=$?"
|
||||
exit=0
|
||||
$ wc -l before.txt after.txt
|
||||
47 before.txt
|
||||
47 after.txt
|
||||
```
|
||||
|
||||
Empty diff, same count (47 `api.Map*` calls — the two `app.MapGet` health probes are
|
||||
outside the `/api/v1` group and were never in scope for this reorder; they were untouched
|
||||
either way). The set of routes is provably unchanged.
|
||||
|
||||
**`.Gate(...)` count, before/after, by wrapper name:**
|
||||
|
||||
```
|
||||
3 .Gate("Beoordelen")
|
||||
4 .Gate("CasesAdmin")
|
||||
1 .Gate("FlagsAdmin")
|
||||
6 .Gate("OrgAdmin")
|
||||
2 .Gate("StamdataAdmin")
|
||||
```
|
||||
|
||||
Identical in both directions — no gate call was added, removed, or renamed.
|
||||
|
||||
**Per-route eyeball check of every route that changed position, per RB-12's stated
|
||||
limitation** (the route-table test only proves a `.Gate(...)` marker is present, not that
|
||||
it still names the wrapper the handler body actually calls):
|
||||
|
||||
| Route | Moved | `.Gate(...)` after | Wrapper actually called inside the handler | Match |
|
||||
| -------------------------------------------- | ------------------------------------------------------- | ----------------------------------------- | ------------------------------------------ | ----- |
|
||||
| `GET /admin/audit` | up, beside `GET /admin/cases` | `CasesAdmin` | `CasesAdmin(ctx, () => ...)` | yes |
|
||||
| `DELETE /admin/cases/{id}` | up, beside `GET /admin/cases` | `CasesAdmin` | `CasesAdmin(ctx, () => { ... })` | yes |
|
||||
| `GET /admin/org-template/{subOrgId}/preview` | Brief section → Org-templates section | `OrgAdmin` | `OrgAdmin(ctx, () => { ... })` | yes |
|
||||
| `POST /uploads` | within Document-upload, past the three reads | _(none — allow-listed, ownership-scoped)_ | — | n/a |
|
||||
| `GET /brief/preview` | within Brief, up beside `GET /brief` | _(none — allow-listed, ownership-scoped)_ | — | n/a |
|
||||
| `GET /uploads/{documentId}/content` | incidental one-slot shift (POST /uploads moved past it) | _(none — allow-listed)_ | — | n/a |
|
||||
| `GET /uploads/status` | incidental one-slot shift (POST /uploads moved past it) | _(none — allow-listed)_ | — | n/a |
|
||||
|
||||
Five routes were deliberately relocated by this ticket; two more shifted position only as
|
||||
a byproduct of `POST /uploads` moving past them (their own order relative to each other
|
||||
is unchanged). All three gated routes among these were checked by eye against the
|
||||
handler body they wrap, not just against `RouteInventoryTests`' marker check — all three
|
||||
match.
|
||||
|
||||
**`RouteInventoryTests`:**
|
||||
|
||||
```
|
||||
Passed! - Failed: 0, Passed: 2, Skipped: 0, Total: 2, Duration: 770 ms
|
||||
```
|
||||
|
||||
Both `Every_mapped_route_is_authz_gated_or_on_the_named_allow_list` and
|
||||
`Every_gate_marker_names_a_known_admin_wrapper` pass.
|
||||
|
||||
**Full backend suite:** `dotnet test --filter "Category!=Integration"` — **262/262
|
||||
passing**, plus the one known, pre-existing, container-dependent failure
|
||||
(`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`),
|
||||
which does not run under `npm run ci` and reproduces on a clean tree with no OpenZaak
|
||||
container running — not this ticket's bug.
|
||||
|
||||
**`dotnet build`** (both projects): 0 warnings, 0 errors. **`dotnet format
|
||||
BigRegister.slnx --verify-no-changes`**: clean.
|
||||
|
||||
**No new test was added.** Per the ticket's Definition of Done: this is a zero-semantic-
|
||||
change commit, and §3c's pre-existing 97.4% line / 84.8% branch coverage of `Program.cs`
|
||||
is the regression net CQ-006 itself names. Nothing about this diff needs a new test to be
|
||||
trustworthy — a passing pre-existing suite plus an empty sorted-route diff is stronger
|
||||
evidence for "nothing changed" than a new test asserting the same thing would be.
|
||||
|
||||
## The regenerated pair
|
||||
|
||||
`npm run gen:api` was run after the reorder. It produced a diff in both
|
||||
`backend/swagger.json` (2 hunks) and `libs/shared/src/infrastructure/api-client.ts` (5
|
||||
hunks) — both **pure reordering, zero content change**. Confirmed by sorting every line of
|
||||
each file (before vs. after) and diffing the sorted output: empty in both cases. The only
|
||||
two OpenAPI paths that moved position in the document are `/api/v1/admin/audit` and
|
||||
`/api/v1/admin/cases/{id}` — the two documented (non-`ExcludeFromDescription`) endpoints
|
||||
this ticket actually reordered relative to their OpenAPI-document neighbours; the
|
||||
generated client's `audit()`/`cases()` methods and their `process*` helpers moved by the
|
||||
same amount, unchanged in every other respect (parameters, return types, status-code
|
||||
branches, JSDoc). Both regenerated files are committed alongside `Program.cs`, per the
|
||||
ticket's explicit instruction: "if the only change is ordering inside swagger.json, say
|
||||
so explicitly and commit the regenerated pair rather than leaving CI's drift job to
|
||||
fail."
|
||||
|
||||
**`npm run ci`**: every job through "backend dependency audit" passed before this
|
||||
ticket's files were committed; the one job that legitimately failed pre-commit was "api-
|
||||
client drift" (`git diff --exit-code` against the not-yet-committed regenerated files —
|
||||
expected, since that step compares the working tree to `HEAD`, and `HEAD` still had the
|
||||
pre-reorder client at that point). After committing, `npm run ci` was re-run to confirm a
|
||||
clean, fully green result against the committed tree — see the final PASS/exit-code
|
||||
reported in this ticket's closing message.
|
||||
|
||||
## What a reviewer should check
|
||||
|
||||
This diff is too large to read top-to-bottom without guidance. The fastest way to review
|
||||
it with confidence:
|
||||
|
||||
1. **Trust the sorted-route diff, not a manual read of every hunk.** The "Verification"
|
||||
section above shows the set of `HTTP METHOD + path` strings is byte-identical before
|
||||
and after. If you want to reproduce it yourself: check out this commit's parent,
|
||||
extract the same `grep -oE` pattern from both revisions of `Program.cs`, sort, diff.
|
||||
2. **Spot-check the five per-route table entries above**, not the whole file — those are
|
||||
the only routes whose position (and, for three of them, gate-vs-handler match)
|
||||
actually matters for this ticket's correctness claim.
|
||||
3. **Diff `git show <this-commit> -- backend/src/BigRegister.Api/Program.cs` with
|
||||
whitespace-insensitive word diff** (`git diff -w --color-words`) if you want to
|
||||
confirm no character inside a moved handler body changed — the line-range-slicing
|
||||
approach in "Design" above makes this a formality rather than a real risk, but it is
|
||||
cheap to re-check.
|
||||
4. **Do not expect Werkvoorraad/Beoordeling/Besluit/zgw-notificaties to have moved
|
||||
position relative to each other** — only their absolute line numbers shifted, as a
|
||||
side effect of the admin-cases block growing above them.
|
||||
5. **The regenerated `swagger.json`/`api-client.ts` diff is expected and pre-verified as
|
||||
ordering-only** (sorted-file diff is empty) — it does not need a second manual read.
|
||||
@@ -0,0 +1,117 @@
|
||||
# RB-20 — route `ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`, surface the error
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `04-cqrs-light.md` CQ-002 ·
|
||||
`00-baseline.md` BL-007 · `99-backlog.md` RB-20 · SIGN-OFF: consolidation approved
|
||||
2026-08-27, HALT lifted
|
||||
|
||||
## What was wrong
|
||||
|
||||
`ApplicationsStore.cancel` and `AdminCasesStore.delete` both owned an optimistic write next
|
||||
to their `RemoteData` read, and both reached `ApplicationsAdapter` directly instead of going
|
||||
through `runSubmit` (the fold + Idempotency-Key mint every other mutation in the repo uses,
|
||||
including `createSubmitChangeRequest` in the same folder). The failure path was a bare
|
||||
`catch { this.state.set(before); }`: a failed cancel or delete rolled the row back, but the
|
||||
user saw no message at all — no `ActionState`, no ProblemDetails `detail`, nothing. The
|
||||
`Idempotency-Key` on the wire was also a fresh UUID per HTTP attempt (minted by
|
||||
`api-client.provider.ts`'s default), not the per-logical-submit key `runSubmit` promises —
|
||||
harmless today only because `Program.cs` happens to ignore the header outside the `Submit`
|
||||
helper (CQ-005's note).
|
||||
|
||||
## What changed
|
||||
|
||||
CQ-002's option (a) — the smallest fix, applied identically to both stores. No new command
|
||||
factory, no adapter split (CQ-002's own "Not filed" note reserves that split for option (b),
|
||||
which this ticket does not take).
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `apps/ssp/src/app/registratie/application/applications.store.ts` | `cancel` now calls `runSubmit(() => this.adapter.cancel(id), SUBMIT_FAILED)`; added a private `error` signal, exposed read-only as `lastError`. On failure: roll back AND `this.error.set(r.error)`. On the next attempt, the error is cleared before the call so a stale message never survives a fresh action. |
|
||||
| `apps/ssp/src/app/registratie/application/applications.store.spec.ts` (new) | 4 specs: load+parse, optimistic cancel, roll-back-and-surface-error on failure, stale-error-clears-on-next-attempt. No spec file existed for this store before RB-20. |
|
||||
| `apps/ssp/src/app/registratie/application/admin-cases.store.ts` | Same shape as `applications.store.ts`: `delete` through `runSubmit`, `error`/`lastError` signal pair. |
|
||||
| `apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts` | Existing "rolls back … when the delete fails" spec extended to also assert `lastError()`; one new stale-error-clears spec added. |
|
||||
| `apps/ssp/src/app/registratie/ui/dashboard.page.ts` | One `@if (cancelError(); as err) { <app-alert type="error">{{ err }}</app-alert> }` above the aanvragen list, mirroring `brief.page.ts`'s `lastError` rendering. `cancelError` is a `computed(() => this.apps.lastError())`. |
|
||||
| `apps/ssp/src/app/registratie/ui/admin-cases.page.ts` | Same `@if (store.lastError(); as err) { <app-alert type="error">{{ err }}</app-alert> }`, placed above `<app-async>` inside the `canManage()` branch (`store` was already `protected`, so no new exposure needed). |
|
||||
|
||||
`applications.adapter.ts` (`cancel`, `deleteAny`) is **unchanged** — the fix is entirely in
|
||||
the two stores, which now wrap the existing thin adapter calls in `runSubmit` at the call
|
||||
site, exactly as `createSubmitChangeRequest` wraps `ChangeRequestAdapter.changeRequest`. The
|
||||
adapter methods still return a bare `Promise<void>`; `runSubmit` is what folds that into a
|
||||
`Result`.
|
||||
|
||||
Neither UI change introduces a new user-facing string: the rendered text is either the
|
||||
existing `SUBMIT_FAILED` constant (`@@submit.failed`, already translated in
|
||||
`messages.en.xlf` since RB-17) or, when the backend sends one, a ProblemDetails `detail`
|
||||
string carried verbatim from the server — never a new `$localize` id. `messages.en.xlf` did
|
||||
not need a new `<target>`.
|
||||
|
||||
## The tests, and their red failures
|
||||
|
||||
Both specs assert `store.lastError()` after a rejected adapter call, which only the fix can
|
||||
satisfy — the old bare `catch { this.state.set(before) }` never touched an error signal, so
|
||||
`lastError()` stayed `null` forever.
|
||||
|
||||
**Verified red without the fix** (an `Edit` undo of the store method, not `git checkout`, so
|
||||
the rest of the change — imports, the other store, the UI, the specs — stayed in place):
|
||||
|
||||
- `applications.store.ts`: reverted `cancel` to `try { await this.adapter.cancel(id); } catch
|
||||
{ this.state.set(before); }`. Reran `ng test ssp --include applications.store.spec.ts`:
|
||||
2 of 4 failed —
|
||||
`rolls back the removal and surfaces the error when the cancel fails` and
|
||||
`clears a stale error on the next cancel attempt`, both with
|
||||
`AssertionError: expected null to be 'Het indienen is niet gelukt. Probeer het later opnieuw.'`.
|
||||
The other two specs (load, optimistic-cancel-success) stayed green, as expected — they
|
||||
don't touch the error path. Re-applied the fix (`Edit` back to the `runSubmit` version);
|
||||
reran: 4/4 green.
|
||||
- `admin-cases.store.ts`: same procedure on `delete`. Reran
|
||||
`ng test ssp --include admin-cases.store.spec.ts`: 2 of 4 failed with the identical
|
||||
`expected null to be '...'` shape. Reverted to the fix; reran: 4/4 green.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **Signal naming**: private backing field `error`, public readonly `lastError` — matching
|
||||
the name `BriefStore`/`OrgTemplateStore` already expose for exactly this purpose (CQ-002's
|
||||
own citation), rather than inventing a new name per store.
|
||||
- **Error cleared at the start of each write**, not only on success, so a second cancel/delete
|
||||
attempt after a failure doesn't leave a stale banner up if the retry itself is still in
|
||||
flight. Covered by the "clears a stale error on the next attempt" spec in each file.
|
||||
- **No `ActionState`/`SaveState` pair** (the fuller shape `BriefStore` uses for busy-state and
|
||||
save-state together) — CQ-002 explicitly scoped option (a) to "one `error` signal", and
|
||||
neither store needs a busy indicator: the row already disappears optimistically the instant
|
||||
the click happens, so there is nothing for a spinner to cover.
|
||||
- **UI placement**: one alert per page, above the list the mutated row belongs to, using the
|
||||
same `@if (x(); as err) { <app-alert type="error">{{ err }}</app-alert> }` shape as
|
||||
`brief.page.ts` — composition of an existing atom, no new building block (CLAUDE.md §2).
|
||||
- **`applications.adapter.ts` left untouched, on purpose** — CQ-002's "Not filed" note ties
|
||||
the read/write file split to option (b) only; taking option (a) means this ticket changes
|
||||
no adapter code at all, matching the ticket's own framing ("(a) touches 2 files plus a UI
|
||||
line each").
|
||||
|
||||
## Ticket accuracy
|
||||
|
||||
CQ-002's description matched the code as found: both stores' `cancel`/`delete` reached the
|
||||
adapter directly with a bare `catch { this.state.set(before); }`, no `Result`, no error
|
||||
channel — no discrepancy to flag.
|
||||
|
||||
## Residuals (not this ticket)
|
||||
|
||||
- RB-18 (key `IdempotencyStore` on `{SubjectId}:{idemKey}`) is unaffected: `cancel`/`delete`
|
||||
now mint a key through `runSubmit` like every other mutation, so it lands on the same
|
||||
write-only call set RB-18 already targets.
|
||||
- RB-21 (extract `createDraftSync`'s read half) is a separate CQRS-light finding in the same
|
||||
context, untouched by this ticket.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run ci` (foreground, `timeout: 600000`): **green** — `✔ local CI passed`. Lint,
|
||||
typecheck, `dep:check` (342 + 226 modules, 0 violations), `format:check`, `check:tokens`,
|
||||
`check:seam`, tests (ssp 263/263 — 5 more than the pre-RB-20 258, from the new/extended
|
||||
specs above — behandelportal 37/37, shared 138/138, beheer 23/23), `ng build --localize`
|
||||
(both apps), `npm audit` (0 vulnerabilities), backend `dotnet format --verify-no-changes` +
|
||||
`dotnet test --filter "Category!=Integration"` (260/260 — this filter is what keeps the
|
||||
known `OpenZaakIntegrationTests.Admin_cases_…` container-dependent test, which needs a live
|
||||
OpenZaak container, out of `npm run ci` entirely; it is a standing caveat, not introduced by
|
||||
this change, and not exercised by this run), backend dependency audit (0 vulnerable
|
||||
packages), `gen:snippets` / `gen:behaviour-spec` / `gen:api` drift checks all clean once the
|
||||
regenerated `behaviour-spec.mdx` was staged alongside the code (the local gate's
|
||||
`git diff --exit-code` compares the working tree to the index, so it is clean once the file
|
||||
is staged — this is the documented pre-commit behaviour from RB-17's note, not a defect).
|
||||
@@ -0,0 +1,119 @@
|
||||
# RB-21 — extract the read half of `createDraftSync` into `find-concept.ts`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `04-cqrs-light.md` CQ-001 ·
|
||||
`00-baseline.md` §4a (`createDraftSync` 143 lines, the largest function in the repo), §9
|
||||
(`fn > 40` threshold) · `99-backlog.md` RB-21
|
||||
|
||||
## What was wrong
|
||||
|
||||
`createDraftSync` (`apps/ssp/src/app/registratie/application/draft-sync.ts`) was registered
|
||||
as a command factory but owned three read paths (`load`, `findConcept`, and the read half of
|
||||
`resume`) mixed into the same function as the write path (`ensureId`, `flush`, `submit`,
|
||||
`reset`). CQ-001 named three pieces of shared mutable closure state — `id`, `ensuring`,
|
||||
`resumeGate` — as load-bearing: `resumeGate` exists only so the write path (`ensureId`) can
|
||||
wait for the read path (`resume`) to finish. That coupling is genuine and stays in place.
|
||||
|
||||
## What changed
|
||||
|
||||
CQ-001's proposal, applied as a pure move, no redesign.
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `apps/ssp/src/app/registratie/application/find-concept.ts` (new) | `findConcept(adapter, type)` and `loadConcept(adapter, id)` — free functions taking `ApplicationsAdapter`, no `inject()`. `loadConcept` returns a `LoadedConcept` union (`{tag:'concept', draft}` \| `{tag:'not-concept'}`) instead of the boolean-shaped branching the inline version had. |
|
||||
| `apps/ssp/src/app/registratie/application/find-concept.spec.ts` (new) | Direct spec, no TestBed — a fake `ApplicationsAdapter` object passed straight to the functions. |
|
||||
| `apps/ssp/src/app/registratie/application/draft-sync.ts` | Removed the inline `findConcept` closure and the body of `load`; both now call the free functions. `createDraftSync` keeps `id`, `ensuring`, `resumeGate`, and the whole write path, unchanged. |
|
||||
| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the new `find-concept.spec.ts` describe blocks. |
|
||||
|
||||
`createDraftSync` shrank from 187 lines (`export function createDraftSync` to its closing
|
||||
brace, HEAD~1) to 169 lines. The whole file went from 236 to 216 lines.
|
||||
|
||||
The two call sites that used the old inline `findConcept()` now pass the adapter and type
|
||||
explicitly:
|
||||
|
||||
```ts
|
||||
// ensureId's 409-recovery catch (WP-35)
|
||||
const existing = await findConcept(adapter, deps.type);
|
||||
```
|
||||
|
||||
```ts
|
||||
// resume(), no ?aanvraag in the URL
|
||||
const existing = await findConcept(adapter, deps.type);
|
||||
```
|
||||
|
||||
`load` keeps setting the closure `id` and calling `applyResume` (both closure-dependent), but
|
||||
delegates the actual read to `loadConcept`:
|
||||
|
||||
```ts
|
||||
const load = (linked: string): Promise<void> => {
|
||||
id = linked;
|
||||
return loadConcept(adapter, linked).then((result) => {
|
||||
if (result.tag === 'not-concept') {
|
||||
id = undefined;
|
||||
applyResume(null);
|
||||
return;
|
||||
}
|
||||
applyResume(result.draft);
|
||||
});
|
||||
};
|
||||
```
|
||||
|
||||
## `draft-sync.spec.ts` — unchanged
|
||||
|
||||
`draft-sync.spec.ts` was not edited. It never called `resume()`/`load()` directly — its
|
||||
coverage is the debounce, `submit()` (including the 409-recovery path, which exercises the
|
||||
extracted `findConcept` indirectly through `ensureId`'s catch), and `flushPending`. All of
|
||||
that stayed in `createDraftSync`, so the spec is unchanged and still exercises the wiring
|
||||
between `createDraftSync` and the two new free functions (the 409-recovery test would fail if
|
||||
that wiring were wrong). It passed unchanged, 8/8.
|
||||
|
||||
## The new spec, and its verified red
|
||||
|
||||
`find-concept.spec.ts` covers the branches CQ-001 named:
|
||||
|
||||
- `findConcept`: match found → id returned; no match of that type → `undefined`; match found
|
||||
but not `Concept` status → `undefined`; `adapter.list()` resolves to an unparsable shape
|
||||
(`parseApplications` fails) → `undefined`; `adapter.list()` rejects → `undefined`.
|
||||
- `loadConcept`: `Concept` with a draft → `{tag:'concept', draft}`; `Concept` with no draft →
|
||||
`{tag:'concept', draft:null}`; a non-`Concept` status (e.g. `Ingediend`, submitted) →
|
||||
`{tag:'not-concept'}`; `adapter.detail()` rejects (unknown/deleted id) →
|
||||
`{tag:'not-concept'}`.
|
||||
|
||||
**Verified red without the fix.** Used `Edit` (not `git checkout`) to invert one condition in
|
||||
`loadConcept` — `dto.status.tag !== 'Concept'` → `dto.status.tag === 'Concept'` — reran `ng
|
||||
test ssp --include find-concept.spec.ts`. Result: 3 of 9 tests failed —
|
||||
|
||||
```
|
||||
loadConcept > reads the draft off a Concept
|
||||
AssertionError: expected { tag: 'not-concept' } to deeply equal { tag: 'concept', draft: { step: 1 } }
|
||||
loadConcept > reports a missing draft as null
|
||||
AssertionError: expected { tag: 'not-concept' } to deeply equal { tag: 'concept', draft: null }
|
||||
loadConcept > reports not-concept when the id has moved past Concept (submitted)
|
||||
AssertionError: expected { tag: 'concept', draft: null } to deeply equal { tag: 'not-concept' }
|
||||
```
|
||||
|
||||
Then used `Edit` again to flip the condition back to `!==`, reran the same command: 9/9
|
||||
green. `findConcept`'s and `loadConcept`'s other branches were not separately mutated — the
|
||||
inverted condition alone was enough to prove the spec is sensitive to the extraction being
|
||||
correct, and re-verifying full green after the revert confirmed no collateral change was left
|
||||
in the file.
|
||||
|
||||
## Scope held
|
||||
|
||||
- No restructuring of the write path (`ensureId`, `flush`, `submit`, `reset`) — untouched
|
||||
beyond the two call-site updates shown above.
|
||||
- `applications.adapter.ts` was not split (CQ-002's "Not filed" note rules that out for this
|
||||
design; out of scope here regardless).
|
||||
- `resume()`'s semantics (URL-param precedence, the `resumeGate` release-in-`finally`, the
|
||||
navigate-to-stamp-the-id side effect) are unchanged — only its two `findConcept()`/`load()`
|
||||
calls now go through the free functions.
|
||||
- No wire change, no DTO change, no behaviour change.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run ci` (foreground): **green** — lint, typecheck, `dep:check`, `format:check`,
|
||||
`check:tokens`, `check:seam`, tests (ssp includes `find-concept.spec.ts` 9/9 new,
|
||||
`draft-sync.spec.ts` 8/8 unchanged), `ng build --localize` (both apps), `npm audit`, backend
|
||||
`dotnet test` (the known `OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure
|
||||
is expected and outside `npm run ci`'s scope), `gen:snippets` drift clean, `gen:behaviour-spec`
|
||||
drift clean once the regenerated file is committed alongside the code. Full counts are in the
|
||||
commit's `npm run ci` run — see the session note for the exact step-by-step output.
|
||||
@@ -0,0 +1,139 @@
|
||||
# RB-22 — `BriefStore.load()` tolerates a 404, calling `reset()` exactly once
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 ·
|
||||
`99-backlog.md` RB-22, "Tickets that were rejected and split" · `implementation/rb-17.md`
|
||||
(the `runResult`/`runSubmit` seam this store already sits on)
|
||||
|
||||
This is the **expand** half of an expand/contract pair. RB-23 (backend: `GET /brief` 404s
|
||||
when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate`) ships after this
|
||||
ticket, in a later merge. Today's backend never 404s `GET /brief`, so this ticket's new
|
||||
branch is dead code in the running app — provably backend-frontend-safe by construction.
|
||||
|
||||
## What was wrong
|
||||
|
||||
CQ-007 flags `GET /brief` (`Program.cs:603` → `BriefStore.GetOrCreate`,
|
||||
`Data/BriefStore.cs:50`) as the one endpoint in the backend where a GET performs a
|
||||
persisted write, breaking the read/write split every other endpoint respects. The fix is
|
||||
split across both sides of the seam because the FE must be ready to receive a 404 before
|
||||
the backend can safely start sending one. This ticket is the FE half: `BriefStore.load()`
|
||||
(`apps/ssp/src/app/brief/application/brief.store.ts`) had no notion of "no brief exists
|
||||
yet" — every adapter failure, 404 included, dispatched `BriefLoadFailed` and showed the
|
||||
generic error banner. `BriefAdapter.load()` (`brief.adapter.ts`) also had no way to tell
|
||||
the store a failure was specifically an HTTP 404: it folded every failure through the
|
||||
shared `runResult` helper (RB-17), which keeps only a human-readable string and throws
|
||||
away the HTTP status.
|
||||
|
||||
The ticket read as filed against the current code: `BriefStore.load()` is exactly where
|
||||
CQ-007 says it is, `BriefAdapter.load()` is exactly the read `runResult` call RB-17 pointed
|
||||
at it, and nothing about either file was factually wrong. Nothing to flag here.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` | `load()`'s error channel becomes `BriefLoadFailure` (`{tag:'notFound'} \| {tag:'error', reason:string}`) instead of a plain `string`. `load()` no longer routes through the shared `runResult` — it does its own try/catch so it can read the thrown value's HTTP `status` before folding it away, via the new local `isHttpNotFound` predicate. Every other method (`save`/`submit`/`approve`/`reject`/`send`/`reset`) is untouched, still on `runSubmit`. |
|
||||
| `apps/ssp/src/app/brief/application/brief.store.ts` | `load()` branches on `BriefLoadFailure`: `notFound` (and not already recovered) calls the existing `reset()` command directly and applies its result; every other failure (including a repeated `notFound`) dispatches `BriefLoadFailed` as before. Extracted `applyLoadedView` (the success-path body shared by `load()` and the new recovery path) and added `recoverFromMissingBrief`. |
|
||||
| `apps/ssp/src/app/brief/application/brief.store.spec.ts` | New `describe('BriefStore.load — 404 tolerance (RB-22)')` with the two required cases. Six pre-existing `load:` fakes' explicit `Result<string, BriefView>` return-type annotations updated to `Result<BriefLoadFailure, BriefView>` (they only ever produce the `ok: true` branch, so this is a type-only change); two shared `ok(v)` test helpers that build fakes for both `load` and `save` had their return-type annotation dropped in favour of `as const` inference, since one helper now serves two different error-channel types. |
|
||||
| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the two new `it()` titles. |
|
||||
|
||||
No `backend/` file was touched — `Program.cs` and `Data/BriefStore.cs` are RB-23's, per the
|
||||
ticket's explicit scope.
|
||||
|
||||
## How the once-only bound is structural
|
||||
|
||||
`BriefStore` gains one field: `private hasRecoveredFromMissingBrief = false`. `load()`
|
||||
takes the recovery branch only when `r.error.tag === 'notFound' && !this.hasRecoveredFromMissingBrief`,
|
||||
and the branch's first statement sets the flag before doing anything else. A second 404 —
|
||||
whether from a second `load()` call, or in principle from `reset()` itself somehow also
|
||||
404ing — falls through to the plain `BriefLoadFailed` branch instead, on every subsequent
|
||||
call, for the life of the store instance. This is a field on the singleton store, not a
|
||||
comment: nothing in the reachable call graph can flip it back to `false`.
|
||||
|
||||
The loop CQ-007's proposed change warns about ("the reset's own load must not be able to
|
||||
loop") is not merely bounded, it is **structurally absent**: `recoverFromMissingBrief`
|
||||
calls `this.adapter.reset()` and applies its `BriefView` result directly (the same
|
||||
`applyLoadedView` the success path uses) — it never calls `this.load()` again. There is no
|
||||
recursive edge from the recovery path back into `load()` for the once-only flag to have to
|
||||
stop; the flag exists only to stop a **second, separate** `load()` invocation (e.g. a
|
||||
caller retrying navigation) from reaching `reset()` again.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`load()` no longer uses `runResult`, only for this one method.** `runResult`
|
||||
(`libs/shared/src/application/submit.ts`, RB-17) intentionally keeps only a string —
|
||||
every other read in the app is fine with that. This is the first read that needs one
|
||||
more bit (the HTTP status) than `runResult` exposes, so `load()` does its own
|
||||
try/catch instead, matching `runResult`'s shape (`problemDetail(e, fallback)` on the
|
||||
non-404 path) but adding the 404 branch first. `libs/shared/src/application/submit.ts`
|
||||
itself is untouched — changing a shared helper used by many call sites for one adapter's
|
||||
need was out of scope and unjustified.
|
||||
- **404 detection reads `(e as {status?:unknown}).status === 404`, not
|
||||
`SwaggerException.isSwaggerException`.** The generated client throws a plain
|
||||
`SwaggerException` for `GET /brief` today (no OpenAPI 404 response is declared for it
|
||||
yet), but throws the parsed `ProblemDetails` object instead for an endpoint whose spec
|
||||
**does** declare a 404 (both shapes carry a `status` field). Checking `status` alone,
|
||||
not the `SwaggerException` type, means this predicate keeps working unchanged once
|
||||
RB-23 regenerates the client with a documented 404 response for `briefGET()` — no
|
||||
follow-up FE edit needed for detection to keep working.
|
||||
- **`BriefLoadFailure` is a new exported type, not a sentinel string.** CLAUDE.md's
|
||||
default reflex is a discriminated union over a second flag; a magic string
|
||||
(`'__not_found__'`) compared by identity would have kept `load()`'s signature at
|
||||
`Result<string, BriefView>` and touched fewer test lines, but it is exactly the kind
|
||||
of stringly-typed control flow the union tool exists to avoid. The touched-test cost
|
||||
was six type annotations plus two helper signatures, all in the one already-scoped
|
||||
spec file — judged worth it for the correct shape.
|
||||
`BriefLoadFailure` is exported.
|
||||
- **On a repeated 404, the store shows `BRIEF_LOAD_FAILED`** (the same generic banner
|
||||
text `load()` already used for every other failure), not a distinct "still missing"
|
||||
message. No new user-facing copy was needed or added, so no new `$localize` id and no
|
||||
`messages.en.xlf` change — confirmed by diffing for `$localize` occurrences: both
|
||||
hits in the diff are unchanged context lines, not new additions.
|
||||
- **`resetDemo()` (the "start over" button) was left untouched**, even though it
|
||||
duplicates part of the same apply-a-fresh-view logic now factored into
|
||||
`applyLoadedView`. It also manages `actionState`/`saveState`/`rejectionSnapshot` that
|
||||
`recoverFromMissingBrief` correctly does not touch (an automatic recovery on first
|
||||
load is not a user-initiated "start over" action), and refactoring it was not asked
|
||||
for by this ticket.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Verified red without the fix.** Temporarily replaced `load()`'s body (via `Edit`,
|
||||
not `git checkout`) with the pre-fix shape — every failure, `notFound` included,
|
||||
dispatches `BriefLoadFailed` straight away, no `reset()` call — and reran the spec
|
||||
file. Both new tests failed:
|
||||
`expected "vi.fn()" to be called 1 times, but got 0 times` on `reset`, for both "a 404
|
||||
drives exactly one reset()" and "a second 404 does not drive a second reset()"; the
|
||||
other 18 tests in the file stayed green. Restored the real fix with a second `Edit`
|
||||
and reran: all 20 tests in the file green, 30/30 across both touched spec files.
|
||||
- `npm run ci` (foreground, no background/Monitor): **green**, exit 0 — lint,
|
||||
typecheck, `dep:check` (341 + 226 modules, 0 violations), `format:check`,
|
||||
`check:tokens`, `check:seam`, tests (ssp 260/260, behandelportal 37/37, shared
|
||||
138/138, beheer 23/23 — 458 total), `ng build --localize` (both apps), `npm audit`
|
||||
(0 vulnerabilities), backend `dotnet test` (260/260 — the known
|
||||
`OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure did not
|
||||
reproduce on this run, matching the standing caveat that it needs a live OpenZaak
|
||||
container and is not this ticket's bug), backend dependency audit clean, `gen:snippets`
|
||||
drift clean, `gen:behaviour-spec` drift clean once the regenerated file was staged (the
|
||||
local gate diffs the working tree against the index, so it necessarily shows a diff
|
||||
before the file is staged/committed — the same documented, expected behaviour RB-17
|
||||
recorded, not a defect).
|
||||
- `npx prettier --check` on every touched file (including the reformatted
|
||||
`99-backlog.md` table and the regenerated `behaviour-spec.mdx`): clean.
|
||||
|
||||
## What RB-23 must do
|
||||
|
||||
Once `GET /brief` in `Program.cs` returns a real 404 (no `ProblemDetails` body is
|
||||
required — `BriefAdapter.load()`'s `isHttpNotFound` only reads the HTTP `status`, not the
|
||||
response body), and `BriefStore.GetOrCreate` splits into `Get` (query) + the existing
|
||||
`ResetAndCreate` (already there, already used by `POST /brief/reset`), this ticket's
|
||||
`notFound` branch stops being dead code and starts running on first-ever page load for any
|
||||
owner with no persisted brief. Run `npm run gen:api` as part of RB-23 so `briefGET()`
|
||||
regenerates with a documented `status === 404` branch throwing the parsed `ProblemDetails`
|
||||
(matching the shape most other endpoints already use) — the detection predicate here
|
||||
already tolerates that shape and needs no FE follow-up change. Two things worth
|
||||
re-verifying once RB-23 lands, not fixing preemptively here: first, that the resulting
|
||||
double round-trip (404, then `reset()`) is an acceptable UX cost on a first visit, per
|
||||
CQ-007's own framing of this as its least certain finding; second, that this store's
|
||||
`hasRecoveredFromMissingBrief` field — private to one store instance, reset only by a full
|
||||
reload — is still the right lifetime for the once-only guard once a real 404 can occur in
|
||||
production traffic, not only in a test's fake adapter.
|
||||
@@ -0,0 +1,183 @@
|
||||
# RB-23 — `GET /brief` 404s when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 ·
|
||||
`99-backlog.md` RB-23, "Tickets that were rejected and split" · `implementation/rb-22.md`
|
||||
(the FE **expand** half this ticket **contracts** against)
|
||||
|
||||
This is the **contract** half of the RB-22/RB-23 expand/contract pair. RB-22 shipped first
|
||||
and made `BriefStore.load()` tolerate a 404 by calling `reset()` once, as a no-op against
|
||||
the (then) still-seeding backend. This ticket is what makes that branch live: `GET /brief`
|
||||
now 404s when the owner has no brief yet, and the endpoint no longer performs a persisted
|
||||
write on a read.
|
||||
|
||||
## What was wrong
|
||||
|
||||
CQ-007 flagged `GET /brief` (`Program.cs:676` → `BriefStore.GetOrCreate`,
|
||||
`Data/BriefStore.cs:50`) as the one endpoint in the backend where a GET performs a
|
||||
persisted write, breaking the read/write split every other endpoint respects. The FE
|
||||
retries GETs automatically (`api-client.provider.ts`, `retry({ count: 2, delay: 500 })`,
|
||||
GET-only, precisely because GETs are assumed safe), so a transient failure could enter the
|
||||
create path more than once; `GetOrCreate`'s `lock` prevented a duplicate row today, but the
|
||||
safety depended on the lock rather than on the endpoint being a query.
|
||||
|
||||
The ticket read as filed against the current code: `GetOrCreate` was exactly at
|
||||
`BriefStore.cs:50`, `GET /brief` called it exactly as described, and `ResetAndCreate`
|
||||
already existed and was already the sole body of `POST /brief/reset`. One thing the
|
||||
ticket's own text did not mention: `BriefStore.GetOrCreate` had a **second** call site,
|
||||
`GET /brief/preview` (`Program.cs:769`, excluded from the OpenAPI doc — a hand-written FE
|
||||
`fetch`, same seam as uploads). Splitting `GetOrCreate` away necessarily touches that
|
||||
call site too, or the file does not compile. See "What changed" below — this was a forced
|
||||
consequence of the split, not a new business decision, and it is reported here rather than
|
||||
silently worked around.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `backend/src/BigRegister.Api/Data/BriefStore.cs` | `GetOrCreate` removed. New `Get(string owner): BriefEntity?` — pure query, `lock`-guarded like every other method in this file for consistency, no write. `ResetAndCreate` is untouched. |
|
||||
| `backend/src/BigRegister.Api/Program.cs` | `GET /brief`: calls `BriefStore.Get`; returns `Results.NotFound()` when null, `Results.Ok(ToView(ctx, e))` otherwise; declares `.Produces(StatusCodes.Status404NotFound)` (the same bare-404 pattern already used at 17 other call sites in this file). `GET /brief/preview`: same `Get` + 404 treatment — forced by the split (see above), not a scope decision made independently. |
|
||||
| `backend/src/BigRegister.Api/Data/AppDbContext.cs` | One comment updated (`GetOrCreate's invariant` → `ResetAndCreate's invariant`) — the unique index on `Owner` it annotates is unchanged. |
|
||||
| `backend/tests/BigRegister.Tests/BriefEndpointTests.cs` | New `Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner` (the DoD-required test). The `Get()` seeding helper, used by nearly every other test in the file, renamed to `SeedBrief()` and changed to create the brief explicitly via `POST /brief/reset` instead of relying on `GET /brief`'s old side effect. One test renamed (`Get_creates_a_draft_with_expected_sections_locked_and_empty` → `SeedBrief_creates_a_draft_with_expected_sections_locked_and_empty`) — it asserts on the shape of a freshly created brief, which is now `SeedBrief()`'s job, not `GET`'s. |
|
||||
| `backend/tests/BigRegister.Tests/PreviewEndpointTests.cs` | Two tests explicitly create the brief (`POST /brief/reset`) before hitting `/brief/preview`, instead of relying on the old `GET /brief` implicit create. |
|
||||
| `backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs` | Five call sites (three bare seeding `GetAsync` calls, two `GetFromJsonAsync<BriefViewDto>` calls used as seeding) changed to an explicit `POST /brief/reset` first. One call site (`Sent_brief_keeps_its_pinned_template_after_a_republish`, reading a brief already created and sent by the shared `WalkBriefToSentThenRepublish` helper) needed no change — a brief already exists by the time it runs. |
|
||||
| `backend/tests/BigRegister.Tests/RouteInventoryTests.cs` | Two `AllowList` reason strings updated (`GetOrCreate` → `Get`, 404 noted) — documentation text only, not itself a check the test enforces beyond "some reason is on record". |
|
||||
| `e2e/brief-v2.spec.ts` | One header comment updated to name the current methods and to state explicitly that this spec's own first click ("Opnieuw beginnen (demo)") is fixture setup, not a workaround for the new 404 — see "e2e and seeding paths" below. |
|
||||
| `libs/shared/src/infrastructure/api-client.ts` | Regenerated (`npm run gen:api`). `briefGET()` gains a `status === 404` branch. See "The generated client" below for the shape it actually took. |
|
||||
| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-23's status cell: `open` → `implemented`. |
|
||||
|
||||
No `apps/ssp/src/app/brief/**` file was touched — RB-22's `BriefStore.load()` recovery and
|
||||
`BriefAdapter.load()`'s `BriefLoadFailure`/`isHttpNotFound` are unchanged, per this
|
||||
ticket's explicit scope.
|
||||
|
||||
## The generated client
|
||||
|
||||
RB-22's handoff note predicted `briefGET()` would regenerate "throwing the parsed
|
||||
`ProblemDetails` (matching the shape most other endpoints already use)". That did not
|
||||
happen, and the actual result is still correct. `Results.NotFound()` (this ticket's
|
||||
implementation, and the pattern used at every one of the 17 other bare-404 call sites in
|
||||
`Program.cs` — none of them use `ProducesProblem`/a typed body) declares a 404 with **no**
|
||||
response body schema. With nothing to parse into, NSwag emits a generic branch that throws
|
||||
a plain `SwaggerException` carrying `status: 404` — the same shape `briefGET()` already
|
||||
threw before this ticket, for the same reason (no declared 404 body). `BriefAdapter.load()`'s
|
||||
`isHttpNotFound` predicate (`(e as {status?:unknown}).status === 404`) already tolerates
|
||||
both a `SwaggerException` and a parsed `ProblemDetails`, by design, precisely so this
|
||||
detail would not matter — RB-22's own comment says as much. No FE follow-up was needed, and
|
||||
none was made.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`GET /brief/preview` also moved off `GetOrCreate`, to `Get` + 404.** Not mentioned in
|
||||
the ticket text, but unavoidable: `GetOrCreate` no longer exists once split, and this
|
||||
was its only other caller. The alternative — leaving a private, undocumented
|
||||
`GetOrCreate`-shaped helper only for this one endpoint — would have reintroduced
|
||||
exactly the GET-writes-on-read pattern CQ-007 is about, in the one place nobody would
|
||||
think to look for it. Returning 404 there too keeps both `/brief` GETs behaving the
|
||||
same way. In the running app this is unreachable in practice: the preview button
|
||||
only renders inside the brief page's `@if (loaded(); as s)` block
|
||||
(`apps/ssp/src/app/brief/ui/brief.page.ts`), which by construction only shows once
|
||||
`BriefStore.load()` has already succeeded — including via RB-22's 404-recovery branch.
|
||||
So a brief always exists by the time a real user can trigger `/brief/preview`; the 404
|
||||
path there is a defensive consequence of the type split, not a new user-facing
|
||||
behaviour anyone will hit.
|
||||
- **`BriefStore.Get` keeps the `lock (_gate)` wrap**, even though a plain SQLite read
|
||||
does not strictly need the same mutual exclusion a write does. Every other method in
|
||||
this file, including the pre-existing `ApplicationStore.Get`-style query in the
|
||||
sibling store, locks unconditionally — matching that convention was judged more
|
||||
valuable than a lock-free read this ticket did not need to justify removing.
|
||||
- **Existing test changes create the brief via `POST /brief/reset`, not a new
|
||||
`BriefStore.Get`/`ResetAndCreate` direct call from the test.** Going through the HTTP
|
||||
endpoint (as the old `Get()` helper always did) keeps the tests exercising the real
|
||||
request pipeline (identity resolution, `ToView` mapping) rather than reaching around
|
||||
it — the same reasoning that already justified an `IClassFixture<TestWebApplicationFactory>`
|
||||
HTTP-level test suite in the first place.
|
||||
|
||||
## e2e and seeding paths
|
||||
|
||||
- **`e2e/brief-v2.spec.ts`** is the only e2e spec that reaches `/brief`. It already opens
|
||||
`/brief?role=drafter` and immediately clicks "Opnieuw beginnen (demo)" (`POST
|
||||
/brief/reset`) before asserting anything — a deliberate fixture reset, not a
|
||||
workaround. With this ticket live, the page's first `GET /brief` on the fresh
|
||||
per-run database (WP-74) now 404s; RB-22's `BriefStore.load()` recovers from that by
|
||||
calling `reset()` once, so the page still renders correctly, and the spec's own
|
||||
explicit reset click still runs on top of that (harmless — resetting an
|
||||
already-fresh brief). No behavioural change to the spec was needed; one comment was
|
||||
updated to say this explicitly rather than leave it to be re-derived.
|
||||
- **Storybook**: no `brief.page.stories.ts` exists, and none of the eleven `brief/ui/**`
|
||||
component stories call `HttpClient`/`fetch`/`ApiClient` — every story supplies data
|
||||
through component `input()`s, per the house convention (design-system/component
|
||||
stories are not live-network integration tests). Nothing in Storybook depended on
|
||||
`GET /brief`'s old seeding behaviour.
|
||||
|
||||
## The double round-trip — verdict
|
||||
|
||||
CQ-007 named this its least certain point: a first-ever visit to `/brief` now costs a 404
|
||||
followed by a `reset()` call, instead of one request that both creates and returns the
|
||||
brief. **Shipped as-is; the cost is acceptable.** Three reasons:
|
||||
|
||||
1. **It happens once per browser tab, ever, for one demo entity.** `BriefStore`'s
|
||||
`hasRecoveredFromMissingBrief` flag (RB-22) makes the 404 unreachable again for the
|
||||
life of the store instance; a real deployment has one brief per zorgverlener, created
|
||||
the first time that person ever opens the page. This is not a cost paid on every
|
||||
page load, or even every session — a page reload still 404s once if the flag reset
|
||||
with the page, but the underlying row is already there by then, so the _second_ call
|
||||
in the pair — `reset()` — is now hitting an existing row rather than truly
|
||||
first-creating one, and returns just as fast as `Get` would have.
|
||||
2. **An extra round-trip is not an extra spinner.** `BriefStore.load()`'s failure
|
||||
handling for `notFound` calls `reset()` and applies the result through the same
|
||||
`applyLoadedView` the success path uses — there is no intermediate "not found" UI
|
||||
state rendered to the user between the two calls; the page shows its loading state
|
||||
once, for the combined duration of both requests.
|
||||
3. **The alternative was rejected, not merely deprioritized.** CQ-007's own
|
||||
documentation-only alternative — leave `GetOrCreate` in place, just write down that
|
||||
the GET seeds on first call — was rejected outright by agent 07 in `99-backlog.md`:
|
||||
"a non-idempotent GET must be visible in the code, not only in a ticket." Given that,
|
||||
the only way to remove the mixing is some version of this two-call shape; a
|
||||
single-call alternative would mean either GET creates (the defect) or `POST
|
||||
/brief/reset` runs unconditionally on load (destructive — it deletes an existing
|
||||
brief, unacceptable for anyone with real content already saved).
|
||||
|
||||
## The once-only guard's lifetime — re-verified
|
||||
|
||||
RB-22 flagged this as worth re-checking once a real 404 could occur in production
|
||||
traffic, not only in a test's fake adapter. Having now made the 404 real: `hasRecoveredFromMissingBrief`
|
||||
is a private field on `BriefStore`, which is `providedIn: 'root'` — one instance per
|
||||
browser tab (per CLAUDE.md's "shared cross-page state = one root singleton" convention),
|
||||
reset only by a full page reload. That lifetime is still correct for what the flag
|
||||
guards: it exists to stop a _second, separate_ `load()` call in the same tab session from
|
||||
re-triggering `reset()` (e.g. a caller retrying navigation after the first recovery
|
||||
already ran) — not to remember "this owner has a brief" across reloads or across owners,
|
||||
which is the server's job (`BriefStore.Get` returning non-null). A page reload correctly
|
||||
starts the guard over: the first `load()` after a reload will find the now-existing row
|
||||
via a plain `GET` (no 404, no `reset()` call at all), so the flag never actually gets
|
||||
exercised a second time in the reload case either. No FE change was needed or made.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Verified red without the fix.** Temporarily (via `Edit`, never `git checkout`)
|
||||
restored `BriefStore.GetOrCreate` alongside the new `Get`, and pointed `GET /brief` in
|
||||
`Program.cs` back at `GetOrCreate`. Ran the new test alone:
|
||||
```
|
||||
BigRegister.Tests.BriefEndpointTests.Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner [FAIL]
|
||||
Assert.Equal() Failure: Values differ
|
||||
Expected: NotFound
|
||||
Actual: OK
|
||||
```
|
||||
Restored the real fix with a second `Edit` (removed the temporary `GetOrCreate`,
|
||||
pointed `GET /brief` back at `Get` + 404) and reran: green.
|
||||
- Full backend suite after the fix: **262/262 passing**, plus the one known,
|
||||
pre-existing, container-dependent failure
|
||||
(`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
"Connection refused (localhost:8000)") — not this ticket's bug, does not run under
|
||||
`npm run ci`, reproduces on a clean tree with no OpenZaak container running.
|
||||
- `npm run gen:api`: the client changed (`libs/shared/src/infrastructure/api-client.ts`,
|
||||
`briefGET()` gains a `status === 404` branch — 4 lines). Regenerated and committed;
|
||||
see "The generated client" above for why the shape differs from RB-22's prediction and
|
||||
why that difference is harmless.
|
||||
- `npm run ci` (foreground, no background/Monitor): see result below.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`apps/ssp/src/app/brief/application/brief.store.ts`, `brief.store.spec.ts`, and
|
||||
`apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` are unchanged — RB-22's FE logic
|
||||
was already correct and already tested against exactly this contract, per this ticket's
|
||||
explicit scope.
|
||||
@@ -0,0 +1,178 @@
|
||||
# RB-24 — `libs/shared/upload` moves into `infrastructure/`/`domain/`/`application/`; the depcruise carve-out is deleted
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `06-adr-conformance.md` ADR-C-002 ·
|
||||
`99-backlog.md` RB-24, "Merges" table row for RB-25/26/27
|
||||
|
||||
## What was wrong
|
||||
|
||||
`libs/shared/src/upload/` held five files outside the folder-per-layer convention every
|
||||
other context follows. `upload.adapter.ts` injects `ApiClient` and opens a raw
|
||||
`XMLHttpRequest` — a genuine network adapter — yet sat outside `infrastructure/`.
|
||||
`upload.machine.ts` was the only Elm-style machine (of 9 in the repo) outside a `domain/`
|
||||
folder. The exception was hard-coded into the enforcement itself:
|
||||
`.dependency-cruiser.base.js`'s `apiclient-infrastructure-only` rule read
|
||||
`from: { pathNot: '/infrastructure/|^libs/shared/src/upload/' }` — carved around the
|
||||
violation instead of the violation being fixed, which is why the baseline scan reported 0
|
||||
violations despite this.
|
||||
|
||||
## What changed
|
||||
|
||||
| From `libs/shared/src/upload/` | To |
|
||||
| -------------------------------- | ----------------------------------------------------- |
|
||||
| `upload.adapter.ts` | `libs/shared/src/infrastructure/upload.adapter.ts` |
|
||||
| `upload.machine.ts` + `.spec.ts` | `libs/shared/src/domain/upload.machine.ts` (+ spec) |
|
||||
| `upload-controller.ts` | `libs/shared/src/application/upload-controller.ts` |
|
||||
| `upload-shell.service.ts` | `libs/shared/src/application/upload-shell.service.ts` |
|
||||
|
||||
All five moves used `git mv`. `libs/shared/src/upload/` no longer exists.
|
||||
|
||||
**Import updates.** 24 consumer files import from `@shared/upload/*` (found with
|
||||
`grep -rln "shared/upload" apps libs --include=*.ts`, filtered to exclude the unrelated
|
||||
`@shared/ui/upload/*` component folder, which was not touched). All 24 files' import paths
|
||||
were rewritten to the new locations (30 import statements total, some files import more
|
||||
than one symbol). No export was renamed, no file was split, no logic changed in any of
|
||||
these 24 files beyond the import path string.
|
||||
|
||||
**Within the five moved files**, three had relative imports (`./upload.adapter`,
|
||||
`./upload.machine`) that now crossed layers and were rewritten to `@shared/*` aliases:
|
||||
`upload.adapter.ts`'s import of `DocumentCategory` from `./upload.machine` →
|
||||
`@shared/domain/upload.machine`; `upload-controller.ts`'s imports of `UploadAdapter` and
|
||||
`upload.machine` symbols → `@shared/infrastructure/...` / `@shared/domain/...`;
|
||||
`upload-shell.service.ts` likewise. `upload.machine.spec.ts` needed no import change — it
|
||||
and `upload.machine.ts` moved into the same `domain/` folder together, so its `./upload.machine`
|
||||
import stayed correct; `git diff --find-renames` confirms this file as a 0-line-changed
|
||||
pure rename.
|
||||
|
||||
**The carve-out.** `.dependency-cruiser.base.js`'s `apiclient-infrastructure-only` rule:
|
||||
`from: { pathNot: '/infrastructure/|^libs/shared/src/upload/' }` → `from: { pathNot: '/infrastructure/' }`,
|
||||
comment updated to drop the now-false "(+ shared/upload)" parenthetical. One further
|
||||
consequence: `docs/reference/architecture/dependencies.md`'s "Atomic-layer rules"
|
||||
paragraph stated the same carve-out in prose ("the generated `ApiClient` is a value only
|
||||
inside `infrastructure/` (+ `libs/shared/src/upload`)") — corrected in the same diff, since
|
||||
leaving it would document a rule that no longer exists.
|
||||
|
||||
## A second, real violation the move exposed — fixed, not just reported
|
||||
|
||||
Deleting the carve-out did not by itself make `dep:check` pass. A **separate,
|
||||
pre-existing** rule — `ui-not-infrastructure` (`ui/`+`layout/` may not import
|
||||
`infrastructure/` as a value) — had never fired against `upload.adapter.ts`, because
|
||||
before this move the file's path did not contain `/infrastructure/` at all. Three UI
|
||||
components were injecting `UploadAdapter` directly:
|
||||
`apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts`,
|
||||
`apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts`,
|
||||
and `apps/ssp/src/app/brief/ui/org-template.page.ts`. Once `upload.adapter.ts` physically
|
||||
moved into `infrastructure/`, `dep:check` correctly flagged all three:
|
||||
|
||||
```
|
||||
error ui-not-infrastructure: .../registratie-wizard.component.ts → libs/shared/src/infrastructure/upload.adapter.ts
|
||||
error ui-not-infrastructure: .../herregistratie-wizard.component.ts → libs/shared/src/infrastructure/upload.adapter.ts
|
||||
error ui-not-infrastructure: .../org-template.page.ts → libs/shared/src/infrastructure/upload.adapter.ts
|
||||
```
|
||||
|
||||
This is judged in-scope to fix, not a second unrelated finding to merely report, for three
|
||||
reasons. First, the ticket's own DoD is explicit: "if `dep:check` fails after the
|
||||
deletion, the move is incomplete, so fix the move rather than restoring the clause."
|
||||
Second, all three call sites used `UploadAdapter` for exactly one thing —
|
||||
`.contentUrl(documentId)`, a thin wrapper around the adapter's own already-exported,
|
||||
injection-free pure function `uploadContentUrl(documentId)` (its doc comment: "Pure (no
|
||||
injection) so a store can build a letterhead-logo `src` without pulling `ApiClient` into
|
||||
its dependency graph" — written for precisely this case). `apps/ssp/src/app/brief/application/brief.store.ts`
|
||||
already used that pure function directly; the three UI files had independently reinvented
|
||||
`inject(UploadAdapter)` + `.contentUrl()` instead. Third, the fix is mechanical and stays
|
||||
inside the ADR's own established idiom — no new architecture, no touch to any RB-25/26/27
|
||||
target:
|
||||
|
||||
- `libs/shared/src/application/upload-controller.ts` — the object `createUploadController`
|
||||
returns gained one more method, `previewUrlFor(documentId)`, built on the existing pure
|
||||
`uploadContentUrl`. Both wizard components already hold a `createUploadController`
|
||||
instance (`uploadCtl`) for their other upload effects; their `previewUrlFor` field now
|
||||
delegates to `uploadCtl.previewUrlFor` instead of injecting `UploadAdapter` itself.
|
||||
- `apps/ssp/src/app/brief/application/org-template.store.ts` (already injects
|
||||
`UploadAdapter` legitimately — it's application layer) gained one more computed-style
|
||||
field, `previewUrlFor`, on the same pure `uploadContentUrl`. `org-template.page.ts` now
|
||||
reads `this.store.previewUrlFor` instead of injecting `UploadAdapter`.
|
||||
|
||||
No behaviour changed: `uploadContentUrl(id)` and `uploadAdapter.contentUrl(id)` return the
|
||||
identical string (the method is a one-line pass-through to the function), and the
|
||||
`demo-*` short-circuit in the two wizards moved into `upload-controller.ts`'s new method
|
||||
verbatim.
|
||||
|
||||
## Verification
|
||||
|
||||
- **`upload.machine.spec.ts` passes unchanged.** `git diff --find-renames=30%` shows it as
|
||||
a 0-insertion/0-deletion pure rename — no content changed, including its own imports
|
||||
(both files moved into `domain/` together, so its `./upload.machine` relative import
|
||||
needed no edit). No spec content changed anywhere in this ticket.
|
||||
- `npm run dep:check`: **passes for both apps** with the carve-out clause removed —
|
||||
`✔ no dependency violations found (344 modules, 1200 dependencies cruised)` (ssp),
|
||||
`✔ no dependency violations found (226 modules, 588 dependencies cruised)` (behandelportal).
|
||||
- `npm run lint`: clean.
|
||||
- `npm test`: **43+6+24+4 = 77 test files, 274+37+138+23 = 472 tests, all passing**
|
||||
(ssp / behandelportal / shared / beheer).
|
||||
- `npm run build`: both apps build (pre-existing, unrelated warnings about
|
||||
`/cibg-huisstijl/css/huisstijl.min.css` and `/letter.css` not being found at build time —
|
||||
present before this ticket, vendored assets resolved at serve/deploy time, not a
|
||||
regression from this move).
|
||||
- **Coverage, `libs/shared/src/domain/`** (`npm run test:coverage` narrowed to `shared`):
|
||||
the folder now includes `upload.machine.ts` at 98.82% statements / 91.8% branches / 100%
|
||||
functions / 98.36% lines (84/85, 56/61, 28/28, 60/61) — the "well-specced machine" ADR-C-002
|
||||
predicted landing in a folder the baseline reported at "0% spec reach across 3 files"
|
||||
(`capability.ts`, `feature-flag.ts`, `role.ts`, which this ticket does not touch and which
|
||||
remain unspecced — that gap is pre-existing and out of this ticket's scope).
|
||||
|
||||
## Non-TypeScript references to the old path — findings
|
||||
|
||||
Checked `.storybook-ssp/`, `.storybook-behandelportal/`, `angular.json`, no vitest config
|
||||
file exists separately (Angular's builder owns test config), both `.dependency-cruiser.*.js`
|
||||
files, and `libs/shared/docs/*.mdx`.
|
||||
|
||||
- **Storybook config, angular.json, dependency-cruiser app configs**: no reference to
|
||||
`shared/upload` or `libs/shared/src/upload` in any of these. Nothing to change.
|
||||
- **`.dependency-cruiser.base.js`**: the one real reference — the carve-out clause itself,
|
||||
deleted (see above).
|
||||
- **`docs/reference/architecture/dependencies.md`**: one prose reference to the same
|
||||
carve-out, corrected in this diff (see above) since it directly describes the rule this
|
||||
ticket edits.
|
||||
- **`libs/shared/docs/*.mdx`**: no `.mdx` file references `libs/shared/src/upload` or
|
||||
`@shared/upload`. `atomic-design.mdx` and `machines.mdx` mention `upload.machine.ts` and
|
||||
`shared/ui/upload/...` by filename/short-path only, never the full old directory path —
|
||||
both remain accurate (the filename didn't change; `ui/upload/` is the untouched sibling
|
||||
folder).
|
||||
- **`apps/ssp/src/locale/messages.xlf`, `messages.en.xlf`, `apps/behandelportal/src/locale/messages.en.xlf`**:
|
||||
each carries a handful of `<context context-type="sourcefile">src/app/shared/upload/upload.machine.ts</context>`
|
||||
/`upload.adapter.ts` annotations — auto-generated by Angular's `$localize` extractor,
|
||||
informational only (they tell a translator where a string originated; they are not
|
||||
read by the build or by `i18nMissingTranslation`). Left as-is: regenerating them is
|
||||
`npm run extract-i18n`'s job for the source-locale file and does not touch the
|
||||
hand-maintained `messages.en.xlf` translations at all, and this ticket's scope is the
|
||||
move plus import updates, not a translation-tooling refresh. They will self-correct
|
||||
the next time `extract-i18n` runs for an unrelated reason.
|
||||
- **`docs/project/backlog/*.md`, `docs/project/refactor-backlog-setup/refactor-backlog/*.md`**:
|
||||
several planning/history documents (WP-25, WP-74, the baseline scan, `02-testability.md`,
|
||||
`06-adr-conformance.md`, `07-bio2-compliance.md`, `99-backlog.md`, `rb-01.md`, `rb-09.md`)
|
||||
reference the old path — expected, since most of them describe or cite the violation
|
||||
this ticket resolves, as history. Not edited, except `99-backlog.md`'s RB-24 status cell
|
||||
(see below).
|
||||
|
||||
## What RB-25/26/27 now find where
|
||||
|
||||
- **RB-25** (`UPLOAD_TRANSPORT` injection token, replacing `inject(KeepaliveTransport)`):
|
||||
`KeepaliveTransport` and `UploadShellService` are both now in
|
||||
`libs/shared/src/application/upload-shell.service.ts` (unchanged content, new path). The
|
||||
token belongs in `application/` alongside them — nothing about the token's shape or
|
||||
location changes because of this move.
|
||||
- **RB-26** (`planFileSelection` in `upload.machine.ts`): the machine is now
|
||||
`libs/shared/src/domain/upload.machine.ts`. `createUploadController`'s `onFileSelected`
|
||||
callback — the accept/reject decision RB-26 targets — is in
|
||||
`libs/shared/src/application/upload-controller.ts` (also renumbered, otherwise
|
||||
unchanged; it also now exports one more method, `previewUrlFor`, added by this ticket —
|
||||
see above). RB-26 should extend `upload.machine.ts` in its new location; no import path
|
||||
in that file needs touching beyond what this ticket already did.
|
||||
- **RB-27** (`uploadOutcome(status, responseText)` out of the XHR closure): the XHR closure
|
||||
is in `libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` method — same
|
||||
file, same method, new path only. `load`/`error`/`abort` handlers, `parseError`, and
|
||||
`genericError` are all still exactly where they were, just under `infrastructure/`.
|
||||
|
||||
## `npm run ci`
|
||||
|
||||
Result and step count reported in the final answer.
|
||||
@@ -0,0 +1,130 @@
|
||||
# RB-25 — `UPLOAD_TRANSPORT` injection token replaces `inject(KeepaliveTransport)`
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-003 ·
|
||||
`99-backlog.md` RB-25, "Merges" table row for RB-25/26/27 · Depends on
|
||||
`implementation/rb-24.md` (the move that put this file at its current path)
|
||||
|
||||
## What was wrong
|
||||
|
||||
`libs/shared/src/application/upload-shell.service.ts` defines `export interface
|
||||
UploadTransport` and documents it as the swap seam for upload transport. It then binds
|
||||
`private transport: UploadTransport = inject(KeepaliveTransport)` — the concrete class,
|
||||
which is `@Injectable` but not exported. A spec cannot reference the class to override its
|
||||
provider, and cannot provide against the interface either, because an interface is not a
|
||||
DI token. The port existed on paper only.
|
||||
|
||||
The consequence TE-003 measures: `upload()`, `delete()`, `cancel()`, and `pollReturning()`
|
||||
— the methods that translate transport and adapter outcomes into `UploadMsg`s — had no
|
||||
spec at all. `libs/shared/upload` sat at 52.0% line / 50.0% branch, and
|
||||
`upload-shell.service.ts` was one of the two unreached non-`ui/` files.
|
||||
|
||||
## What changed
|
||||
|
||||
One file plus one new spec, exactly as scoped:
|
||||
|
||||
- `libs/shared/src/application/upload-shell.service.ts`: added
|
||||
|
||||
```ts
|
||||
export const UPLOAD_TRANSPORT = new InjectionToken<UploadTransport>('UPLOAD_TRANSPORT', {
|
||||
providedIn: 'root',
|
||||
factory: () => inject(KeepaliveTransport),
|
||||
});
|
||||
```
|
||||
|
||||
copied verbatim from TE-003's own fix, placed directly under the `KeepaliveTransport`
|
||||
class it wraps. `UploadShellService.transport` now reads
|
||||
`inject(UPLOAD_TRANSPORT)` instead of `inject(KeepaliveTransport)`. This is the same
|
||||
interface-plus-token shape as `SessionPort`/`SESSION_PORT`
|
||||
(`libs/shared/src/application/session.port.ts`), the repo's one other explicit port.
|
||||
`KeepaliveTransport` itself is untouched: still a private, unexported `@Injectable`, and
|
||||
still the default factory's target — a real app gets the exact same singleton instance
|
||||
it always did.
|
||||
|
||||
- `libs/shared/src/application/upload-shell.service.spec.ts` (new): a recording fake
|
||||
`UploadTransport` (records every `send()` call, exposes `resolveDone`/`rejectDone` per
|
||||
call so a test drives the returned `Promise` by hand) provided against `UPLOAD_TRANSPORT`,
|
||||
plus a fake `UploadAdapter` (a plain object with `vi.fn()` for `status`/`deleteDocument`)
|
||||
provided against the already-exported `UploadAdapter` class. 16 specs across all four
|
||||
target methods:
|
||||
- `upload()` — `UploadQueued` carries the transport's `backgroundSyncAvailable`;
|
||||
`onProgress` → `UploadProgress`; a resolved transport → `UploadComplete`; a rejected
|
||||
transport → `UploadFailed` with the rejection reason; a rejection with the
|
||||
`UPLOAD_ABORTED` sentinel dispatches nothing.
|
||||
- `cancel()` — calls the stored cancel function for an in-flight upload and forgets it
|
||||
(a second `cancel()` on the same id is a no-op); an unknown id is a no-op.
|
||||
- `delete()` — `UploadDeleting` then `UploadDeleteComplete` on success;
|
||||
`UploadDeleteFailed` with the server's `detail` on a ProblemDetails rejection; falls
|
||||
back to an empty reason when the rejection carries no `detail`.
|
||||
- `pollReturning()` — skips the adapter call entirely for an empty upload list;
|
||||
dispatches `BackgroundUploadsReturned` filtered to only the items the server reports
|
||||
`complete` with a `documentId`; dispatches nothing when nothing has arrived.
|
||||
|
||||
No other file changed. `UploadAdapter`, `upload.machine.ts`, and `upload-controller.ts`
|
||||
are untouched, per the ticket's file-scope fence (RB-26 and RB-28 are concurrently in
|
||||
adjacent files).
|
||||
|
||||
## Verification
|
||||
|
||||
- **Coverage, `upload-shell.service.ts`** (`npm run test:coverage` narrowed to `shared`,
|
||||
read from `coverage/shared/lcov.info`):
|
||||
|
||||
| Metric | Before | After |
|
||||
| --------- | ------ | -------------- |
|
||||
| Lines | 0% | 88.57% (31/35) |
|
||||
| Branches | 0% | 85.00% (17/20) |
|
||||
| Functions | 0% | 87.50% (14/16) |
|
||||
|
||||
"Before" is 0% across the board: no spec file for this service existed prior to this
|
||||
ticket (confirmed by `grep -rln UploadShellService --include=*.spec.ts`, which returns
|
||||
only the new spec), matching TE-003's "unreached" classification. The remaining
|
||||
uncovered lines are the `KeepaliveTransport` class body (`send()`, its `inject`) and the
|
||||
`UPLOAD_TRANSPORT` factory closure itself — both require a real `XMLHttpRequest`/real DI
|
||||
resolution to exercise and are intentionally out of this ticket's scope: TE-003's fix is
|
||||
the seam, not a rewrite of the transport it wraps.
|
||||
|
||||
- **Red-proof.** Edited `upload()`'s success branch from
|
||||
`dispatch({ type: 'UploadComplete', localId: req.localId, documentId })` to
|
||||
`dispatch({ type: 'UploadFailed', localId: req.localId, reason: 'BROKEN-FOR-RED-PROOF' })`,
|
||||
ran `ng test shared`. Result: 1 failed / 150 passed, with
|
||||
|
||||
```
|
||||
AssertionError: expected "vi.fn()" to be called with arguments: [ { type: 'UploadComplete', …(2) } ]
|
||||
Received:
|
||||
1st vi.fn() call: [{ "backgroundSync": false, "localId": "l1", "type": "UploadQueued" }]
|
||||
2nd vi.fn() call: [{ "localId": "l1", "reason": "BROKEN-FOR-RED-PROOF", "type": "UploadFailed" }]
|
||||
```
|
||||
|
||||
at `upload-shell.service.spec.ts:79` (the `UploadComplete` assertion). Re-applied the
|
||||
original line with a second edit (not `git checkout`); `git diff` against HEAD shows
|
||||
only the intended token change — the red edit left no trace. Re-ran: 151/151 green.
|
||||
|
||||
- `npm run ci`: result and step count in the final answer.
|
||||
|
||||
## Judgement call
|
||||
|
||||
- **The fake `UploadAdapter` is a plain object, not a class extending `UploadAdapter`.**
|
||||
`UploadAdapter` is exported and already usable as a DI token (it always was — TE-003's
|
||||
gap was specific to `KeepaliveTransport`, not `UploadAdapter`), so `delete()` and
|
||||
`pollReturning()` (which never touch `this.transport`) were technically fakeable before
|
||||
this ticket by providing a fake `UploadAdapter`. Nobody had written that spec, though,
|
||||
and `upload()`/`cancel()` still needed `UPLOAD_TRANSPORT` regardless (they populate and
|
||||
drain the `inflight` map via `transport.send()`). The spec fakes both seams together so
|
||||
all four methods are exercised as one coherent suite, per the ticket's own framing
|
||||
("provide a recording fake transport and assert the message translation in `upload()`,
|
||||
`delete()`, `cancel()` and `pollReturning()`").
|
||||
|
||||
## Handoff to RB-27
|
||||
|
||||
RB-27 extracts `uploadOutcome(status, responseText)` out of the XHR closure in
|
||||
`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` — a different file,
|
||||
untouched by this ticket. The token makes RB-27's optional half (moving the
|
||||
`currentScenario()` branch into `KeepaliveTransport.send()`) no easier and no harder than
|
||||
before: `KeepaliveTransport` is still unexported and its `send()` body is unchanged, one
|
||||
line (`inject(UploadAdapter)`, `return this.adapter.xhrUpload(req, onProgress)`). If RB-27
|
||||
takes that optional move, it can inject `UPLOAD_TRANSPORT` in its own spec to assert the
|
||||
scenario branch without touching this file — the seam is there and provided-in-root, but
|
||||
RB-27 does not need to change anything here to use it.
|
||||
|
||||
## `npm run ci`
|
||||
|
||||
Result and step count reported in the final answer.
|
||||
@@ -0,0 +1,109 @@
|
||||
# RB-26 — move the accept/reject decision into `planFileSelection` (`upload.machine.ts`)
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-004 ·
|
||||
`99-backlog.md` RB-26 · Depends on `implementation/rb-24.md` (moved the upload files into
|
||||
`infrastructure`/`domain`/`application`)
|
||||
|
||||
## What was wrong
|
||||
|
||||
TE-004: `createUploadController` does three `inject()` calls, registers an `effect()`, and
|
||||
adds a `window` focus listener, all before it returns. A spec must run inside a `TestBed`
|
||||
injection context with `UploadAdapter`, `UploadShellService`, and `DestroyRef` all
|
||||
satisfied to reach anything inside it. What sits behind that cost is real policy:
|
||||
`onFileSelected` decides, per file, whether to reject it with reason `'multiple'`, reject
|
||||
it with a `rejectReason` result, or start its upload — a decision over
|
||||
`(categories, categoryId, files)` with no I/O in it. `rejectReason`, the predicate that
|
||||
decision calls, was already exported and spec'd; the decision that calls it was not.
|
||||
|
||||
## What changed
|
||||
|
||||
`libs/shared/src/domain/upload.machine.ts` gains one pure export:
|
||||
|
||||
```ts
|
||||
export function planFileSelection(
|
||||
state: UploadState,
|
||||
categoryId: string,
|
||||
files: { name: string; type: string; size: number }[],
|
||||
): UploadMsg[];
|
||||
```
|
||||
|
||||
It takes plain `{ name, type, size }` objects, not `File` — a spec needs no DOM. The body
|
||||
is the old `onFileSelected` decision, moved: an unknown category plans nothing; too many
|
||||
files for a single-file category plans one `FileRejected` with reason `'multiple'` and
|
||||
skips the per-file checks; otherwise each file is judged by `rejectReason` and plans
|
||||
either a `FileRejected` or a `FileSelected` entry, one entry per input file, in order.
|
||||
|
||||
`libs/shared/src/application/upload-controller.ts`'s `onFileSelected` now maps `selected:
|
||||
File[]` to plain candidates, calls `planFileSelection`, and executes the result: a
|
||||
`FileRejected` entry dispatches as-is; anything else starts the upload for the file at
|
||||
that same array index (`crypto.randomUUID()`, `files.set()`, `shell.upload()` — the three
|
||||
things that must stay impure and stay in the controller). No other method changed.
|
||||
`previewUrlFor` (added by RB-24) is untouched.
|
||||
|
||||
## The `localId` placeholder — a deliberate, contained choice
|
||||
|
||||
An accepted file's planned `FileSelected` entry carries `localId: ''`. A real id needs
|
||||
`crypto.randomUUID()`, and the ticket is explicit that call stays in the controller, not
|
||||
the domain. The controller reads only each entry's `.type` to route it — it dispatches a
|
||||
`FileRejected` entry verbatim, but for a `FileSelected` entry it discards the entry and
|
||||
calls `start(categoryId, selected[i])`, which builds its own message with a real id.
|
||||
The placeholder is therefore never dispatched. This was the only way found to keep the
|
||||
return type exactly `UploadMsg[]` (as the ticket's own code sketch specifies) while still
|
||||
letting the plan carry a per-file, order-preserving "start this one" signal — the
|
||||
`FileRejected` variant carries no file identity (state keys rejections by category only),
|
||||
so position in the returned array is what the controller uses to find the matching
|
||||
original `File`. A discriminated `{ kind: 'reject' | 'start'; msg? }` return would avoid
|
||||
the placeholder but was not built, since the ticket's signature is explicit and the
|
||||
placeholder design meets it without changing behaviour.
|
||||
|
||||
## Behaviour
|
||||
|
||||
Same messages, same order, for the same inputs. Tracing all three original branches:
|
||||
|
||||
- Unknown category: original returns without dispatching; new code calls `planFileSelection`
|
||||
(returns `[]`), then `forEach` over an empty array — no dispatch, no start.
|
||||
- Too many files for a single-file category: original dispatches one `FileRejected`
|
||||
('multiple') and returns; new code gets a one-entry plan and dispatches that one entry —
|
||||
`forEach` never reaches indices past the plan's length, so no file starts.
|
||||
- Per-file loop: original dispatches `FileRejected` or calls `start` for each file, in
|
||||
order; new code's plan has one entry per file, in the same order, and the controller
|
||||
dispatches or starts at each index identically.
|
||||
|
||||
## Testing
|
||||
|
||||
`libs/shared/src/domain/upload.machine.spec.ts` gained a `planFileSelection` describe
|
||||
block: unknown category (plans nothing), the `'multiple'` batch rejection, a passing
|
||||
single file against a single-file category, `rejectReason`'s two reject cases (`'type'`,
|
||||
`'size'`) reached through the plan, the accept case's exact `FileSelected` shape
|
||||
(including the `localId: ''` placeholder), and a mixed multiple-file case asserting
|
||||
order (`['FileSelected', 'FileRejected', 'FileSelected']`).
|
||||
|
||||
**Proved red before green**, per the ticket's instruction not to use `git checkout`:
|
||||
temporarily replaced the function body with a stub returning `[]` unconditionally (an
|
||||
edit, not a revert), ran `ng test shared`, and got:
|
||||
|
||||
```
|
||||
Test Files 1 failed | 23 passed (24)
|
||||
Tests 6 failed | 139 passed (145)
|
||||
```
|
||||
|
||||
The 6 failures were the `'multiple'` rejection, both `rejectReason` cases, the accepted-
|
||||
file shape, and the mixed-order case — every outcome that depends on the real branching,
|
||||
each failing with `expected [] to deeply equal [...]`. The unknown-category case passed
|
||||
even against the stub, since both the stub and the real implementation return `[]` there
|
||||
— expected, not a gap, since that branch has no policy to exercise. A second edit restored
|
||||
the real body; the same run returned to `24 passed / 145 passed`.
|
||||
|
||||
## Scope held
|
||||
|
||||
No change to `createUploadController`'s construction, the `effect()`, or the `window`
|
||||
listener — those are RB-25/RB-27's targets (RB-25 is `UploadShellService`, running
|
||||
concurrently in the same commit window; RB-27 is `upload.adapter.ts`'s XHR closure).
|
||||
Neither file was touched. The controller's public surface (`previewUrlFor`,
|
||||
`onFileSelected`, `onRemove`, `onRetry`, `onDelete`, `onChannelChange`) is unchanged in
|
||||
name and signature, and the organism that calls it (`<app-document-upload>`) needed no
|
||||
change.
|
||||
|
||||
## `npm run ci`
|
||||
|
||||
Result and step count reported in the closing message.
|
||||
@@ -0,0 +1,193 @@
|
||||
# RB-27 — `uploadOutcome` extracted from the XHR `load` closure
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-005 ·
|
||||
`99-backlog.md` RB-27, "Merges" table row for RB-25/26/27 · Depends on
|
||||
`implementation/rb-24.md` (the move that put this file at its current path) and
|
||||
`implementation/rb-25.md` (handoff paragraph read before deciding the optional half)
|
||||
|
||||
## What was wrong
|
||||
|
||||
`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` constructs
|
||||
`new XMLHttpRequest()` directly and attaches its `load` listener inline. The listener
|
||||
body held the actual decisions: 2xx-vs-not, `JSON.parse` of the response body with a
|
||||
fallback to a generic error, and (on a non-2xx status) ProblemDetails mapping via the
|
||||
un-exported `parseError`. None of it is reachable without stubbing the XHR global, so
|
||||
the interpretation logic had no spec.
|
||||
|
||||
TE-005's baseline citation: **LH 5 / LF 64 (7.8% line), BRH 3 / BRF 57 (5.3% branch)**.
|
||||
The file was counted "reached" in the module total only because another spec imports
|
||||
it — essentially nothing in it executed.
|
||||
|
||||
## What changed
|
||||
|
||||
One function extracted from the `load` listener, in the same file:
|
||||
|
||||
```ts
|
||||
export function uploadOutcome(
|
||||
status: number,
|
||||
responseText: string,
|
||||
): Result<string, { documentId: string }> {
|
||||
if (status < 200 || status >= 300) return err(parseError(responseText));
|
||||
try {
|
||||
return ok({ documentId: JSON.parse(responseText).documentId });
|
||||
} catch {
|
||||
return err(genericError());
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
placed next to `genericError`/`parseError` (below the class, above the dev
|
||||
`simulateUpload`). It contains exactly the 2xx-vs-not check, the `JSON.parse`-with-
|
||||
fallback, and the ProblemDetails mapping — the three decisions TE-005 names. The `load`
|
||||
listener is now a two-line dispatch:
|
||||
|
||||
```ts
|
||||
xhr.addEventListener('load', () => {
|
||||
const outcome = uploadOutcome(xhr.status, xhr.responseText);
|
||||
outcome.ok ? resolve(outcome.value) : reject(outcome.error);
|
||||
});
|
||||
```
|
||||
|
||||
`Result`, `ok`, `err` are imported from `@shared/kernel/fp` (the repo's one `Result`
|
||||
type, already used the same way by `libs/shared`'s other infrastructure adapters).
|
||||
`parseError` and `genericError` are untouched — `uploadOutcome` calls them exactly as
|
||||
the old listener body did, so their own behavior (ProblemDetails detail extraction,
|
||||
generic fallback) is unchanged.
|
||||
|
||||
## Abort-vs-error: left as a separate, smaller concern
|
||||
|
||||
TE-005 names abort-vs-error disambiguation in the same sentence as the extraction
|
||||
target, but its proposed signature — `uploadOutcome(status: number, responseText:
|
||||
string)` — has no way to express "the request was aborted before any response
|
||||
arrived." That is a real, structural mismatch, not an oversight to route around:
|
||||
|
||||
- `uploadOutcome` runs inside the `load` listener, which fires only when the browser
|
||||
received a complete HTTP response — it has a `status` and a `responseText` by
|
||||
construction.
|
||||
- The `abort` listener fires instead of `load` when `xhr.abort()` was called
|
||||
client-side. There is no HTTP response at that point — no status, no body — so
|
||||
folding it into `uploadOutcome`'s signature would mean inventing a fake status (e.g.
|
||||
`0`) to stand for "not actually a response," which trades one implicit convention for
|
||||
another and makes the pure function's contract lie about what it receives.
|
||||
|
||||
The existing code already expresses this as the smallest form it can take:
|
||||
|
||||
```ts
|
||||
xhr.addEventListener('abort', () => (aborted ? reject(UPLOAD_ABORTED) : reject(genericError())));
|
||||
```
|
||||
|
||||
one ternary, deciding between two sentinels based on which native event fired and
|
||||
whether `cancel()` was called first — not on response content. It is not a second
|
||||
`uploadOutcome`-shaped decision hiding in a closure; it is a one-line dispatch already.
|
||||
Extracting it into its own named function would add a call site and an import for a
|
||||
single ternary with no reachable-only-via-DOM logic left inside it. Left in place, as
|
||||
DoD point 2 allows.
|
||||
|
||||
## Spec added, verified red
|
||||
|
||||
`libs/shared/src/infrastructure/upload.adapter.spec.ts` (new file) — plain
|
||||
`describe`/`it`, no `TestBed`, no DOM, no XHR stub, matching the DoD's explicit
|
||||
"that is the entire point." Five cases:
|
||||
|
||||
1. 2xx status with a valid JSON body → `{ ok: true, value: { documentId } }`.
|
||||
2. 2xx status with an unparseable body → falls back to the generic `UPLOAD_FAILED`
|
||||
text (the `JSON.parse`-with-fallback branch).
|
||||
3. Non-2xx status with a ProblemDetails body → the `detail` field, via `parseError`.
|
||||
4. Non-2xx status with a body that is not ProblemDetails-shaped → falls back to the
|
||||
generic text.
|
||||
5. The 200/300 boundary: 299 is success, 300 is not.
|
||||
|
||||
`UPLOAD_FAILED`'s text is not exported (unchanged by this ticket), so the spec holds
|
||||
its own copy of the Dutch string as a local constant with a comment pointing at the
|
||||
source — the same trade every other spec makes when asserting against `$localize`
|
||||
constants that never leave their module ($localize`strings are English-first prose
|
||||
only where the source is`nl`, so this is the source text as written, not a stand-in).
|
||||
|
||||
**Red-proof.** Edited `uploadOutcome`'s body down to a single line —
|
||||
`return ok({ documentId: JSON.parse(responseText).documentId });`, dropping the
|
||||
status check and the try/catch — with an `Edit` (not `git checkout`). Ran
|
||||
`ng test shared`. Result: 4 of the 5 new specs failed:
|
||||
|
||||
```
|
||||
SyntaxError: Unexpected token 'o', "not json" is not valid JSON
|
||||
❯ uploadOutcome libs/shared/src/infrastructure/upload.adapter.ts:168:32
|
||||
|
||||
AssertionError: expected { ok: true, value: { …(1) } } to deeply equal { ok: false, …(1) }
|
||||
- Expected "error": "Document is al aan een aanvraag gekoppeld.", "ok": false,
|
||||
+ Received "ok": true, "value": { "documentId": undefined },
|
||||
|
||||
SyntaxError: Unexpected token 'I', "Internal S"... is not valid JSON
|
||||
|
||||
AssertionError: expected true to be false // Object.is equality
|
||||
```
|
||||
|
||||
(only the plain 2xx-valid-JSON case still passed, as expected of a mutant that always
|
||||
reports success). Re-applied the real body with a second `Edit`; `git diff` against
|
||||
HEAD shows only the intended net change — the red edit left no trace. Re-ran:
|
||||
163/163 green.
|
||||
|
||||
## Coverage, `upload.adapter.ts`
|
||||
|
||||
| Metric | Before (TE-005 baseline) | After |
|
||||
| -------- | ------------------------ | ---------------------- |
|
||||
| Lines | LH 5 / LF 64 (7.8%) | LH 12 / LF 65 (18.5%) |
|
||||
| Branches | BRH 3 / BRF 57 (5.3%) | BRH 7 / BRF 59 (11.9%) |
|
||||
|
||||
(`LF`/`BRF` grew by one line and two branches because `uploadOutcome` is new source;
|
||||
`npm run test:coverage`'s shared run, `coverage/shared/lcov.info`, narrowed to this
|
||||
file's `SF:` block.) The jump is real but modest in absolute percentage: `uploadOutcome`
|
||||
itself is now fully exercised (`FNDA:6,uploadOutcome`, both branches of the status
|
||||
check hit, both the try and the catch path hit), but the class methods
|
||||
(`categoriesResource`, `status`, `deleteDocument`, `xhrUpload`'s own body,
|
||||
`simulateUpload`) remain unreached — they need DI/XHR/timers to test and are
|
||||
out of this ticket's scope, exactly as TE-005 scopes it ("extract the interpretation,
|
||||
not the transport").
|
||||
|
||||
## Optional scenario-branch move: not taken
|
||||
|
||||
TE-005 suggests, as an explicitly optional second half, moving the `currentScenario()`
|
||||
branch from `xhrUpload` up into `KeepaliveTransport.send()`
|
||||
(`libs/shared/src/application/upload-shell.service.ts`) so `xhrUpload` becomes
|
||||
transport-only. RB-25's handoff confirms the seam is available (`KeepaliveTransport`
|
||||
is still unexported, `send()` is still an unchanged one-liner) but not required.
|
||||
|
||||
This ticket does not take that half, for a reason RB-25's handoff does not settle:
|
||||
the ticket's own **Scope** section restricts this ticket to `upload.adapter.ts` and its
|
||||
spec only ("RB-24, RB-25, RB-26, RB-28 have all already merged — nothing else in the
|
||||
upload module is in flight, so you have the folder to yourself"). Moving the scenario
|
||||
branch requires editing `upload-shell.service.ts` too — exporting `simulateUpload` (or
|
||||
moving it) out of `upload.adapter.ts` and importing it into the application-layer
|
||||
`send()` — which is a second file, outside the stated scope. Doing it anyway would also
|
||||
widen this single-file ticket's diff for an explicitly optional half the ticket itself
|
||||
says to skip when it "complicates the diff." The dev simulator's behavior is therefore
|
||||
byte-for-byte unchanged: `xhrUpload` still checks `currentScenario()` first and still
|
||||
delegates to the untouched `simulateUpload` for `upload-slow`/`upload-fail`, verified by
|
||||
inspection (the only edit inside `xhrUpload` is the `load`-listener dispatch) and by the
|
||||
full `shared` suite staying green, including `upload-shell.service.spec.ts`'s existing
|
||||
scenario-adjacent assertions.
|
||||
|
||||
## Verification
|
||||
|
||||
- `npm run lint`: clean.
|
||||
- `npm run dep:check`: unaffected — the only new import is `@shared/kernel/fp`, already
|
||||
the repo's shared `Result` module, imported the same way by other `libs/shared`
|
||||
infrastructure adapters (no new import direction).
|
||||
- `npm test` / `ng test shared`: 163/163, across 26 spec files — 5 of those tests are
|
||||
the new `upload.adapter.spec.ts`, the other 158 across 25 pre-existing files are
|
||||
unchanged by this ticket.
|
||||
- `npm run ci`: result and step count reported in the implementing agent's final answer.
|
||||
|
||||
## Batch 5 close-out
|
||||
|
||||
Batch 5 (RB-25 through RB-30) is now fully implemented. For `libs/shared/upload`
|
||||
(moved to its layered home by RB-24) specifically: `upload.machine.ts` (domain) has its
|
||||
own spec and `planFileSelection` extracted by RB-26; `upload-shell.service.ts`
|
||||
(application) has a full spec covering `upload()`/`cancel()`/`delete()`/
|
||||
`pollReturning()` via the `UPLOAD_TRANSPORT` token RB-25 added; `upload-controller.ts`
|
||||
(application) was already spec'd before this batch; `upload.adapter.ts`
|
||||
(infrastructure) now has `uploadOutcome` as a pure, spec'd seam, though the class's
|
||||
HTTP-bound methods (categories/status/delete/the XHR transport itself) remain
|
||||
untested by design — XHR is the one boundary this batch deliberately does not
|
||||
abstract, per TE-005's own instruction. End to end, every layer of the upload module
|
||||
that can hold pure logic now does, and has a spec proving it; what is left uncovered is
|
||||
exactly the DOM/network edge the module exists to wrap, not logic hiding behind it.
|
||||
@@ -0,0 +1,180 @@
|
||||
# RB-28 — `BLOB_PRESENTER` token unlocks the three blob-to-browser success paths
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-006 ·
|
||||
`99-backlog.md` RB-28
|
||||
|
||||
## What was wrong
|
||||
|
||||
Three application-layer commands each ended in raw DOM/browser calls that jsdom cannot
|
||||
meaningfully execute: `StamdataStore.download()`
|
||||
(`libs/beheer/src/application/stamdata.store.ts`) did `URL.createObjectURL` →
|
||||
`document.createElement('a')` → `a.click()` → `URL.revokeObjectURL`;
|
||||
`BriefStore.previewLetter()` (`apps/ssp/src/app/brief/application/brief.store.ts`) and
|
||||
`OrgTemplateStore.proefbrief()` (`apps/ssp/src/app/brief/application/org-template.store.ts`)
|
||||
both did `window.open(URL.createObjectURL(blob), '_blank')`. Because the call was the
|
||||
last statement of each command, TE-006 recorded the whole success path as effectively
|
||||
unassertable, and `download()`'s two-clause guard (`if (!s || !this.canDownload())
|
||||
return;`) as permanently dark on its true branch.
|
||||
|
||||
## What changed
|
||||
|
||||
One new file, `libs/shared/src/application/blob-presenter.ts`, mirroring the
|
||||
`SESSION_PORT` token already in that folder — an interface, a production
|
||||
implementation, and an `InjectionToken`:
|
||||
|
||||
```ts
|
||||
export interface BlobPresenter {
|
||||
open(blob: Blob): void;
|
||||
download(blob: Blob, filename: string): void;
|
||||
}
|
||||
|
||||
const realBlobPresenter: BlobPresenter = {
|
||||
open(blob) {
|
||||
window.open(URL.createObjectURL(blob), '_blank');
|
||||
},
|
||||
download(blob, filename) {
|
||||
const url = URL.createObjectURL(blob);
|
||||
const a = document.createElement('a');
|
||||
a.href = url;
|
||||
a.download = filename;
|
||||
a.click();
|
||||
URL.revokeObjectURL(url);
|
||||
},
|
||||
};
|
||||
|
||||
export const BLOB_PRESENTER = new InjectionToken<BlobPresenter>('BLOB_PRESENTER', {
|
||||
providedIn: 'root',
|
||||
factory: () => realBlobPresenter,
|
||||
});
|
||||
```
|
||||
|
||||
`open()` never revokes the object URL (the tab it opens outlives the call —
|
||||
`BriefStore.previewLetter`'s original comment already said so and is preserved,
|
||||
moved onto the token's own doc comment); `download()` does revoke, once the click has
|
||||
fired. This asymmetry is preserved deliberately, not unified — the two call sites
|
||||
behaved differently before this ticket and still do.
|
||||
|
||||
Each of the three commands now injects `BLOB_PRESENTER` and calls it instead of the DOM
|
||||
directly:
|
||||
|
||||
| File | Before (last statement) | After |
|
||||
| ----------------------- | ---------------------------------------------------------------------------------- | ------------------------------------------------------------ |
|
||||
| `stamdata.store.ts` | `createObjectURL` → `createElement('a')` → `click()` → `revokeObjectURL` (6 lines) | `this.blobPresenter.download(blob, \`${s.table.id}.json\`);` |
|
||||
| `brief.store.ts` | `window.open(URL.createObjectURL(r.value), '_blank')` | `this.blobPresenter.open(r.value);` |
|
||||
| `org-template.store.ts` | `window.open(URL.createObjectURL(r.value), '_blank')` | `this.blobPresenter.open(r.value);` |
|
||||
|
||||
`OrgTemplateStore.previewUrlFor` (added by RB-24, a different seam — a document
|
||||
content URL for an `<a href>`, not a blob handoff) is untouched.
|
||||
|
||||
## Tests added
|
||||
|
||||
**`libs/beheer/src/application/stamdata.store.spec.ts`** — a new
|
||||
`StamdataStore.download (RB-28)` describe block with a recording fake `BlobPresenter`:
|
||||
|
||||
1. Does not call the presenter while `canDownload()` is false because nothing is dirty
|
||||
yet — the guard's previously-dark true branch, first clause.
|
||||
2. Does not call the presenter while previewing a date, even with a real edit present —
|
||||
the guard's true branch, second clause.
|
||||
3. **The success path**, asserting `toJson(...)`'s exact output reaches the file: reads
|
||||
the recorded blob's text and compares it byte-for-byte against a direct call to
|
||||
`toJson(store.table()!, store.rows())`, and asserts the filename is
|
||||
`professions.json`.
|
||||
|
||||
**`apps/ssp/src/app/brief/application/brief.store.spec.ts`** — the existing
|
||||
`BriefStore.previewLetter` describe block's success test previously spied directly on
|
||||
`window.open`/`URL.createObjectURL` (both already jsdom-spyable, since the properties
|
||||
exist even though calling them for real throws "not implemented"). It now provides the
|
||||
recording fake via `BLOB_PRESENTER` and asserts `opened` holds exactly the resolved
|
||||
blob — the same outcome, reached through the new seam instead of monkey-patching two
|
||||
global browser objects.
|
||||
|
||||
**`apps/ssp/src/app/brief/application/org-template.store.spec.ts`** (new file —
|
||||
`OrgTemplateStore` had no spec at all before this ticket) — a
|
||||
`OrgTemplateStore.proefbrief (RB-28)` describe block: the success path (presenter
|
||||
receives the resolved blob, no error) and the failure path (presenter never reached,
|
||||
error surfaced). A `Partial<UploadAdapter>` stub with a no-op `categoriesResource`
|
||||
(status `'idle'`) satisfies the store's constructor effect without touching the
|
||||
logo-upload sub-state, which these tests do not exercise.
|
||||
|
||||
## Verified red without the fix
|
||||
|
||||
Broke `StamdataStore.download()` with an `Edit` (not `git checkout`): changed the
|
||||
filename from `` `${s.table.id}.json` `` to `` `${s.table.id}.csv` ``. Ran the new
|
||||
success-path spec:
|
||||
|
||||
```
|
||||
AssertionError: expected 'professions.csv' to be 'professions.json' // Object.is equality
|
||||
|
||||
Expected: "professions.json"
|
||||
Received: "professions.csv"
|
||||
❯ libs/beheer/src/application/stamdata.store.spec.ts:134:36
|
||||
```
|
||||
|
||||
Re-applied the correct filename with a second `Edit`; the full `stamdata.store.spec.ts`
|
||||
file (6 tests) went green again.
|
||||
|
||||
## Verification
|
||||
|
||||
- **`grep` for remaining DOM blob calls** in all three stores —
|
||||
`grep -nE "window\.open|createObjectURL|revokeObjectURL|createElement\('a'\)|\.click\(\)"` —
|
||||
zero matches. The only occurrences of those calls anywhere in `apps`/`libs` are inside
|
||||
`blob-presenter.ts` itself (checked with a second, unscoped grep — no fourth inlined
|
||||
handoff exists).
|
||||
- `npm run lint`: clean.
|
||||
- `npm run dep:check`: unaffected (no new import direction — `libs/shared` still does not
|
||||
depend on `libs/beheer`; both `libs/beheer` and `apps/ssp/brief` import the new token
|
||||
from `libs/shared`, never the reverse).
|
||||
- `npm test` (all four projects): all pass — ssp 276, behandelportal 37, shared 138,
|
||||
beheer 26 (up from 23; +3 for the new `download()` describe block).
|
||||
- Coverage, `npm run test:coverage` narrowed per project:
|
||||
- `libs/beheer/src/application/stamdata.store.ts` — **before** BRH 15 / BRF 37
|
||||
(40.5% branch, confirmed against the current tree, matching TE-006's citation
|
||||
exactly); **after** BRH 25 / BRF 37 (**67.6% branch**). `libs/beheer/src/application`
|
||||
has exactly this one file, so the module figure moves the same way.
|
||||
- `apps/ssp/src/app/brief/application/brief.store.ts` — **before** BRH 39 / BRF 72
|
||||
(54.2% branch). This is higher than TE-006's cited 32/64 (50%) because RB-22/RB-23
|
||||
already added branches (the 404-tolerance path) since the finding was written — see
|
||||
"What TE-006 got wrong" below. **After**: BRH 39 / BRF 72, unchanged — swapping the
|
||||
global-spy assertions for the injected fake changes how the success branch is
|
||||
reached in the spec, not whether it is reached; it was already covered before this
|
||||
ticket (see below).
|
||||
- `apps/ssp/src/app/brief/application/org-template.store.ts` — no spec existed before
|
||||
this ticket, so there is no meaningful "before" branch figure for it specifically.
|
||||
**After**: BRH 17 / BRF 77, including both `proefbrief()` branches newly covered.
|
||||
- `npm run ci` (foreground, `timeout: 600000`, no background/Monitor): result reported
|
||||
in the implementing agent's final answer.
|
||||
|
||||
## What TE-006 got wrong
|
||||
|
||||
TE-006 states: "`brief.store.spec.ts` demonstrates this exactly: it tests
|
||||
`previewLetter`'s failure case ... and cannot test the success case." This is not
|
||||
accurate for the code as it stood at the start of this ticket. The spec already had an
|
||||
`'opens the composed letter in a new tab on success'` test that used
|
||||
`vi.spyOn(URL, 'createObjectURL')` and `vi.spyOn(window, 'open')` to assert the success
|
||||
path — jsdom defines both properties (as functions that throw "not implemented" if
|
||||
actually invoked), so `vi.spyOn` can already replace them, and the pre-existing test
|
||||
did. That test passed both before and after this ticket's change; this ticket did not
|
||||
newly unlock `previewLetter`'s success path, it moved an already-passing assertion off
|
||||
two hand-spied global browser objects and onto the new injectable seam. `git log
|
||||
--follow -p` on the spec file shows this test dates to the WP-67 monorepo merge, not to
|
||||
any of RB-22/23/24.
|
||||
|
||||
The seam is still worth having: `StamdataStore.download()`'s success path (five
|
||||
DOM/API calls in a row: `createObjectURL`, `createElement`, `.href`, `.download`,
|
||||
`.click()`, `revokeObjectURL`) is a materially harder thing to spy on faithfully than a
|
||||
single `window.open` call, and was in fact still dark before this ticket (no
|
||||
`download()` test of any kind existed). `OrgTemplateStore.proefbrief()` also had no
|
||||
spec at all. TE-006's diagnosis (three commands share the same class of problem, one
|
||||
token fixes all three) is sound; only the specific "cannot test" claim about
|
||||
`previewLetter` overstates what was true for that one call site. Scope was not reduced
|
||||
because of this — all three call sites are migrated per the ticket's own instruction to
|
||||
ship them together rather than half-adopt the seam.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`OrgTemplateStore.previewUrlFor` (RB-24) — confirmed present and unchanged at
|
||||
`org-template.store.ts:78`. `libs/shared/src/application/upload-shell.service.ts`
|
||||
(RB-25) and `upload-controller.ts` (RB-26) — not read beyond what RB-24's own note
|
||||
already described, not edited. `libs/shared/docs/behaviour-spec.mdx` — regenerated by
|
||||
`npm run gen:behaviour-spec` (part of `npm run ci`) to reflect the new/renamed test
|
||||
names; never hand-edited.
|
||||
@@ -0,0 +1,125 @@
|
||||
# RB-29 — Thread `at` through `LetterHtml.ResolveAuto`'s `datum` case
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `02-testability.md` TE-007 ·
|
||||
`99-backlog.md` RB-29
|
||||
|
||||
## What was wrong
|
||||
|
||||
`LetterHtml.Render(BriefEntity brief, OrgTemplateDto template, string at, bool watermark)`
|
||||
(`backend/src/BigRegister.Api/Domain/Letters/LetterHtml.cs`) already took the letter's
|
||||
instant and used it correctly for the letterhead date
|
||||
(`sb.Append(Enc(FormatDatumNl(at)))`). The body's `datum` placeholder resolved through
|
||||
the private `ResolveAuto(string key, string label)`, which ignored `at` and called
|
||||
`FormatDatumNl(DateTimeOffset.UtcNow.ToString("o"))` instead — a pure `Domain/` rule
|
||||
class reading the wall clock. `ResolveAuto` is reached only through the private chain
|
||||
`RenderNode` ← `RenderParagraphs` ← `Render`, so no caller outside this file could pin
|
||||
the value a test would see.
|
||||
|
||||
The ticket read as filed against the current code: `Render`'s signature, the letterhead's
|
||||
correct use of `at`, and `ResolveAuto`'s `UtcNow` read were all exactly as TE-007
|
||||
described (line numbers had moved — CC around the file has grown since the finding was
|
||||
written — but the code shape had not). One thing TE-007 named as the visible symptom
|
||||
also checked out: `LetterHtmlTests.cs` already declares a
|
||||
`new PlaceholderDefDto("datum", "Datum", true)` in its golden-file fixture, but no
|
||||
`RichTextNodeDto` in that fixture's `Sections` actually references the `datum` key in
|
||||
the body — it is declared but never rendered there, so the existing golden-file test
|
||||
could not have caught this even if it asserted on dates (which it does not either).
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `backend/src/BigRegister.Api/Domain/Letters/LetterHtml.cs` | `ResolveAuto(string key, string label)` → `ResolveAuto(string key, string label, string at)`; `"datum" => FormatDatumNl(at)`. `at` threaded down through the two private call sites in the chain: `RenderParagraphs` and `RenderNode` both gained an `at` parameter, passed from `Render`'s own `at`. |
|
||||
| `backend/tests/BigRegister.Tests/LetterHtmlTests.cs` | New fixture `FixtureBriefWithDatumInBody()` — a minimal brief whose body actually references the `datum` placeholder (the golden fixture never does). Two new `[Fact]`s (see below) plus two small extraction helpers. |
|
||||
| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-29 status cell: `open` → `implemented`. |
|
||||
|
||||
`Render`'s own signature is unchanged — TE-007's "zero public API change, zero
|
||||
call-site change" held exactly. `Program.cs:697`, `:708`, and `BriefStore.cs:120` (the
|
||||
three callers) needed no edit.
|
||||
|
||||
## What the fix looks like
|
||||
|
||||
```csharp
|
||||
private static void RenderParagraphs(
|
||||
StringBuilder sb, IReadOnlyList<ParagraphDto> paragraphs, IReadOnlyDictionary<string, PlaceholderDefDto> defs,
|
||||
string at)
|
||||
{
|
||||
// ... unchanged body, forwards `at` to RenderNode ...
|
||||
}
|
||||
|
||||
private static void RenderNode(
|
||||
StringBuilder sb, RichTextNodeDto node, IReadOnlyDictionary<string, PlaceholderDefDto> defs, string at)
|
||||
{
|
||||
// ... unchanged body, forwards `at` to ResolveAuto ...
|
||||
}
|
||||
|
||||
private static string ResolveAuto(string key, string label, string at) => key switch
|
||||
{
|
||||
"naam_zorgverlener" => SeedData.Registration.Naam,
|
||||
"big_nummer" => SeedData.Registration.BigNummer,
|
||||
"datum" => FormatDatumNl(at),
|
||||
_ => label,
|
||||
};
|
||||
```
|
||||
|
||||
Both call sites already had `at` in scope (`Render`'s own parameter), so this is a pure
|
||||
threading change — no new state, no new dependency.
|
||||
|
||||
## Tests added
|
||||
|
||||
TE-007 named the exact gap: the golden-file fixture declares the `datum` placeholder but
|
||||
never renders it in the body, so no existing assertion could catch a body/letterhead
|
||||
mismatch. A new fixture and two focused tests close it:
|
||||
|
||||
1. **`Render_resolves_the_body_datum_placeholder_from_the_given_at_not_the_wall_clock`** —
|
||||
renders `FixtureBriefWithDatumInBody()` with a fixed historical `at`
|
||||
(`2019-03-14T08:00:00.0000000+00:00`) and asserts the body's rendered paragraph is the
|
||||
exact string `"14 maart 2019"`. A test using today's date would have passed before
|
||||
and after the fix and proven nothing — this one pins a date nowhere near "now", so it
|
||||
fails whenever the resolver reads the wall clock instead of `at`.
|
||||
2. **`Render_keeps_the_letterhead_date_and_the_body_datum_in_agreement_for_a_historical_at`**
|
||||
— same fixture and historical `at`, asserts the letterhead `<dd>` date and the body's
|
||||
rendered `datum` paragraph are equal. This is TE-007's stated payoff: not a shipped
|
||||
bug today (every current caller passes `Now()` at render time, so the two dates always
|
||||
coincided even with the bug present), but a latent one — the moment `Render` is ever
|
||||
called with a historical `at` (re-rendering an archive, back-dating a letter), the
|
||||
letterhead and body would disagree within a single document. This test is the one
|
||||
that would have caught that.
|
||||
|
||||
Both tests use the repo's one date formatter (`FormatDatumNl`, already used by both call
|
||||
sites under test) only indirectly, through the literal expected string `"14 maart
|
||||
2019"` — no second hand-rolled `ToString` format was introduced in the test file either.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Verified red without the fix.** Reverted only the `ResolveAuto` expression (via
|
||||
`Edit`, not `git checkout`) back to
|
||||
`"datum" => FormatDatumNl(DateTimeOffset.UtcNow.ToString("o"))`, leaving the new
|
||||
tests and the threaded signatures in place. Ran the two new tests:
|
||||
```
|
||||
Render_resolves_the_body_datum_placeholder_from_the_given_at_not_the_wall_clock [FAIL]
|
||||
Assert.Equal() Failure: Strings differ
|
||||
Expected: "14 maart 2019"
|
||||
Actual: "27 augustus 2026"
|
||||
|
||||
Render_keeps_the_letterhead_date_and_the_body_datum_in_agreement_for_a_historical_at [FAIL]
|
||||
Assert.Equal() Failure: Strings differ
|
||||
Expected: "14 maart 2019"
|
||||
Actual: "27 augustus 2026"
|
||||
```
|
||||
Both failures show the body rendering the run's actual wall-clock date (today,
|
||||
2026-08-27) instead of the pinned historical `at` — the precise defect TE-007
|
||||
describes. Restored the fix with a second `Edit` and re-ran: all 4 tests in
|
||||
`LetterHtmlTests` green (2 pre-existing + 2 new).
|
||||
- `grep -n "UtcNow\|DateTime.Now\|DateTime.Today\|DateTimeOffset.Now"
|
||||
backend/src/BigRegister.Api/Domain/Letters/LetterHtml.cs` — no matches. No ambient
|
||||
clock read remains anywhere in the file.
|
||||
- `npm run ci` (foreground, no background/Monitor): see result reported alongside this
|
||||
ticket.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`LetterHtml.cs`'s overall structure and CC (TE-007 records it at 21, the third-highest
|
||||
in the backend) are unchanged — reducing that is out of scope for this ticket, per its
|
||||
own text. `Data/BriefStore.cs` and any `BriefRules.cs` file were not touched — a
|
||||
concurrent ticket owns that file.
|
||||
@@ -0,0 +1,210 @@
|
||||
# RB-30 — extract `BriefStore`'s guards into `Domain/Letters/BriefRules.cs`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source finding: `02-testability.md` TE-008 ·
|
||||
`99-backlog.md` RB-30
|
||||
|
||||
RB-30 moves the brief workflow's five guard decisions out of `BriefStore` (a
|
||||
lock-held, DB-opening static store) into a pure `Domain/Letters/BriefRules.cs`, and
|
||||
adds a free-running unit test file for them. This is a pure extraction: the store
|
||||
keeps its lock, its `Db.Create()`, its static shape, and every method's signature.
|
||||
|
||||
## What was wrong
|
||||
|
||||
Five guard clusters in `Data/BriefStore.cs` are pure decisions over `(status tag,
|
||||
actor role, entity completeness)` — Save, Submit, Send, and the shared Approve/Reject
|
||||
review path each start with an `if` cascade that is a function of two enums and a
|
||||
bool. But every one of those `if`s sat inside a method that had already done `lock
|
||||
(_gate) { using var db = Db.Create(); ... }`, so a spec could not exercise the
|
||||
decision without a booted host and a real SQLite file. `Domain/Letters/` held only
|
||||
`LetterHtml.cs` and `OrgTemplateRules.cs`; there was no `BriefRules` class, even
|
||||
though `Authz.CanActOn` — a pure `Domain/Authorization/` call one line away from
|
||||
three of the guards — already proved the pattern worked for this exact file.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `backend/src/BigRegister.Api/Domain/Letters/BriefRules.cs` | New. Five pure statics: `CanSave`, `StatusAfterSave`, `RequiredFilled` + `CanSubmit`, `CanSend`, `CanDecide`. All take `BriefStatusDto`/`bool`/`Principal`/`string`, never `BriefEntity` — no persistence type reaches this file. |
|
||||
| `backend/src/BigRegister.Api/Data/BriefStore.cs` | `Save`, `Submit`, `Send`, and the private `Review` (the Approve/Reject shared path) each replace their inline `if` cascade with one call into `BriefRules`, then branch only on the returned `Outcome`. The private `RequiredFilled(BriefEntity e)` helper is deleted — `BriefRules.RequiredFilled(IReadOnlyList<LetterSectionDto>)` replaces it. Lock, `Db.Create()`, method signatures, and the public `Outcome` enum are all unchanged. |
|
||||
| `backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs` | New. 29 `[Fact]`/`[Theory]` assertions covering every branch of all five rules — see "Tests added" below. |
|
||||
| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-30's status cell: `open` → `done`. |
|
||||
|
||||
## The surface, as built — and where it differs from TE-008's proposal
|
||||
|
||||
TE-008 proposed:
|
||||
|
||||
```
|
||||
CanSave(BriefStatusDto status, bool isDrafter) → Outcome
|
||||
StatusAfterSave(BriefStatusDto) → BriefStatusDto
|
||||
CanSubmit(status, isDrafter, bool requiredFilled) → Outcome
|
||||
CanSend(status)
|
||||
CanDecide(status, Principal, drafterId)
|
||||
```
|
||||
|
||||
The ticket explicitly names this a proposal, not a specification. What was built
|
||||
matches it almost exactly, with two adjustments forced by the real code:
|
||||
|
||||
- **`Outcome` is `BriefStore.Outcome`, not a new type.** `BriefStore` already
|
||||
exposes a public `enum Outcome { Ok, Forbidden, Conflict }`, and `Program.cs`'s
|
||||
`BriefResult` switches on it directly across every brief endpoint. TE-008 itself
|
||||
says: "if `Outcome` does not already exist as a domain concept, use whatever the
|
||||
sibling rule classes already return" — it does exist, so `BriefRules` returns it
|
||||
rather than inventing a second result shape. This does mean `Domain/Letters/`
|
||||
references a type nested in `Api.Data`; the same cross-reference already exists in
|
||||
this file's neighbor, `LetterHtml.cs` (`using BigRegister.Api.Data;`, for
|
||||
`BriefEntity`), and in `Authz.cs` (for `BriefStore`'s role-id constants) — both in
|
||||
the same single-assembly project, so this is a namespace convention, not an
|
||||
assembly boundary. `Outcome` itself is a plain three-value enum with no EF/ASP.NET
|
||||
attached, so this does not pull a persistence type into `Domain/`.
|
||||
- **`CanDecide` takes an explicit `BriefAction action` parameter**, not just
|
||||
`(status, Principal, drafterId)`. The real guard — `BriefStore.Review` — is one
|
||||
private method shared by both `Approve` and `Reject`, and it calls
|
||||
`Authz.CanActOn(action, principal, drafterId)`, which needs to know which action is
|
||||
being attempted. `BriefRules.CanDecide` composes that existing pure
|
||||
`Authz.CanActOn` call with the status check, rather than re-implementing the SoD
|
||||
logic a second time — so the four-eyes rule still has exactly one source of truth.
|
||||
|
||||
The `RequiredFilled` predicate is a sixth pure static, not one of the five guards
|
||||
proper — TE-008 names it separately ("plus the `RequiredFilled(e)` predicate") and it
|
||||
is built the same way: `RequiredFilled(IReadOnlyList<LetterSectionDto> sections) →
|
||||
bool`, taking the section list rather than the entity.
|
||||
|
||||
## Order and behaviour preserved
|
||||
|
||||
Every rule keeps the original check order, which matters because `Outcome.Forbidden`
|
||||
must outrank `Outcome.Conflict` (a non-drafter or non-entitled caller sees Forbidden
|
||||
even against an otherwise-invalid status):
|
||||
|
||||
- `CanSave`: `!isDrafter` (Forbidden) before the status-tag check (Conflict).
|
||||
- `CanSubmit`: `!isDrafter` (Forbidden) before `status.Tag != "draft" ||
|
||||
!requiredFilled` (Conflict).
|
||||
- `CanDecide`: `!Authz.CanActOn(...)` (Forbidden) before `status.Tag != "submitted"`
|
||||
(Conflict) — the exact order the old inline check in `Review` used, per its own
|
||||
comment ("checked BEFORE the status guard").
|
||||
- `CanSave`'s entity-not-found branch (`e is null → Conflict`) stays inline in
|
||||
`BriefStore` — it is a persistence fact ("no row for this owner"), not one of the
|
||||
three business axes TE-008 names (status tag, actor role, entity completeness), so
|
||||
it was left where it was rather than forced into a rule that would then need to
|
||||
accept a nullable entity.
|
||||
|
||||
## Tests added
|
||||
|
||||
`backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs`, 29 assertions, alongside
|
||||
the seven Domain test files that already existed:
|
||||
|
||||
- **`CanSave`** — drafter saves draft/rejected (Ok, `[Theory]`); drafter saves
|
||||
submitted/approved/sent (Conflict, `[Theory]`); non-drafter saves draft or submitted
|
||||
(Forbidden both times — proves role beats status).
|
||||
- **`StatusAfterSave`** — rejected → draft; draft stays draft.
|
||||
- **`RequiredFilled`** — no sections; an unfilled optional section; a filled required
|
||||
section; an unfilled required section; one filled + one unfilled required section
|
||||
(proves one bad section blocks the whole letter).
|
||||
- **`CanSubmit`** — filled draft (Ok); unfilled draft (Conflict — **the required-filled
|
||||
gate the ticket explicitly asked for**); non-draft status (Conflict); non-drafter
|
||||
on a filled draft (Forbidden — role beats completeness).
|
||||
- **`CanSend`** — approved (Ok); draft/submitted/rejected/sent (Conflict, `[Theory]`).
|
||||
- **`CanDecide`** — approver decides a submitted letter drafted by someone else, for
|
||||
both Approve and Reject (Ok, `[Theory]`); a drafter attempting to decide (Forbidden
|
||||
— **the non-drafter denial the ticket asked for**); an approver whose acting id
|
||||
equals the drafter id, i.e. self-review (Forbidden — the four-eyes/SoD case); an
|
||||
approver deciding a non-submitted letter (Conflict); an approver who is also the
|
||||
drafter AND the status is non-submitted (Forbidden, not Conflict — proves the
|
||||
priority order survived the extraction).
|
||||
|
||||
## Verified red without the fix
|
||||
|
||||
Inverted `CanSubmit`'s completeness check (`!requiredFilled` → `requiredFilled`) with
|
||||
an `Edit`, ran `BriefRuleTests` alone:
|
||||
|
||||
```
|
||||
[xUnit.net] BigRegister.Tests.Domain.BriefRuleTests.A_drafter_may_not_submit_an_unfilled_draft [FAIL]
|
||||
Assert.Equal() Failure: Values differ
|
||||
Expected: Conflict
|
||||
Actual: Ok
|
||||
[xUnit.net] BigRegister.Tests.Domain.BriefRuleTests.A_drafter_may_submit_a_filled_draft [FAIL]
|
||||
Assert.Equal() Failure: Values differ
|
||||
Expected: Ok
|
||||
Actual: Conflict
|
||||
|
||||
Failed! - Failed: 2, Passed: 27, Skipped: 0, Total: 29
|
||||
```
|
||||
|
||||
Reverted with a second `Edit` (never `git checkout` — that would have discarded the
|
||||
whole file). Reran: 29/29 green.
|
||||
|
||||
## Existing tests — unchanged
|
||||
|
||||
`BriefEndpointTests.cs`, `PreviewEndpointTests.cs`, and `OrgTemplateEndpointTests.cs`
|
||||
(the three host-booting suites that exercise the brief endpoints) needed **no
|
||||
changes**. Ran together: 32/32 passing, proving the extraction preserved every HTTP
|
||||
outcome (`Save_is_drafter_only`, `Submit_blocks_on_empty_required_section`,
|
||||
`Submit_succeeds_when_required_sections_filled`,
|
||||
`Drafter_cannot_approve_own_letter_but_a_different_reviewer_can`,
|
||||
`Reject_returns_comments`, `Editing_a_rejected_letter_reopens_it_to_draft`,
|
||||
`Send_only_from_approved`, and the rest, all unmodified).
|
||||
|
||||
## The metric TE-008 cares about: host-booting brief-rule assertions
|
||||
|
||||
Before this ticket, the five guard decisions had **zero** free-running unit
|
||||
assertions. Every branch of every guard was reachable only through the seven
|
||||
host-booting endpoint test methods above (six of them containing an explicit
|
||||
`Assert.Equal(HttpStatusCode.Forbidden/Conflict, ...)`, each paying a full
|
||||
`TestWebApplicationFactory` host boot plus a real SQLite round-trip, run serially
|
||||
process-wide because of `[assembly: DisableTestParallelization]`).
|
||||
|
||||
After this ticket:
|
||||
|
||||
- **0 → 29** free-running unit assertions covering these branches
|
||||
(`BriefRuleTests.cs`, `dotnet test --filter FullyQualifiedName~BriefRuleTests`
|
||||
completes in **~120 ms**, no host, no SQLite file).
|
||||
- **7 → 7** host-booting endpoint tests, unchanged. They stay — they are now the
|
||||
proof that `BriefStore` wires `BriefRules`'s answer to the right HTTP status, not
|
||||
the only place the business decision itself is checked. That split (wiring proven
|
||||
at the integration layer, decision logic proven at the unit layer) is the seam
|
||||
TE-008 argued for.
|
||||
- New branches this ticket made assertable that the endpoint suite never covered
|
||||
directly: the SoD self-review case (`An_approver_may_not_decide_a_letter_they_drafted_themselves`)
|
||||
and the Forbidden-beats-Conflict priority ordering for both `CanSave`/`CanSubmit`
|
||||
(role checked first) and `CanDecide` (entitlement checked first) — these existed as
|
||||
implicit behaviour in the original `if` cascades but had no assertion pinning them
|
||||
before RB-30.
|
||||
|
||||
## What was not extracted
|
||||
|
||||
Nothing — all five guards named in TE-008, plus the `RequiredFilled` predicate, moved
|
||||
cleanly. None needed the `DbContext`: each was already a function of values already
|
||||
resident on the in-memory `BriefEntity` (its `Status`, `Sections`, `DrafterId`), never
|
||||
of a query against the database itself.
|
||||
|
||||
## Scope respected
|
||||
|
||||
- `Domain/Letters/LetterHtml.cs` was not touched (a concurrent agent owns it).
|
||||
- `BriefEntity.ToDto()` was not touched — its CC 16 is a separate, out-of-scope
|
||||
finding per the ticket.
|
||||
- `Data/Db.cs`'s static-store decision and `TestWebApplicationFactory`'s serialized-test
|
||||
position were not challenged; the store's lock, `Db.Create()`, and public shape are
|
||||
byte-for-byte the same as before this ticket, other than the `if` cascades moving
|
||||
out.
|
||||
|
||||
## Verification
|
||||
|
||||
- `dotnet build`: 0 warnings, 0 errors.
|
||||
- `dotnet test --filter FullyQualifiedName~BriefRuleTests`: 29/29, ~120 ms.
|
||||
- `dotnet test --filter FullyQualifiedName~BriefEndpointTests|...PreviewEndpointTests|...OrgTemplateEndpointTests`:
|
||||
32/32, unchanged.
|
||||
- Full backend suite: **291/292 passing**, plus the one known, pre-existing,
|
||||
container-dependent failure
|
||||
(`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
"Connection refused (localhost:8000)") — not this ticket's bug, does not run under
|
||||
`npm run ci`, reproduces on a clean tree with no OpenZaak container running.
|
||||
- `npm run ci` (foreground, no background/Monitor): see the commit message / session
|
||||
report for the exit code and step count.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
No frontend file was touched — the brief workflow's status machine is server-
|
||||
authoritative, and the FE's own pure reducer (mirroring these same transitions for
|
||||
UX) was already out of this ticket's scope. No file outside `backend/Data/BriefStore.cs`,
|
||||
`backend/Domain/Letters/BriefRules.cs`,
|
||||
`backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs`, and `99-backlog.md` was
|
||||
changed.
|
||||
@@ -0,0 +1,155 @@
|
||||
# RB-31 — replay real messages in the four hand-rolling machine specs
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `06-adr-conformance.md` ADR-C-010 ·
|
||||
`99-backlog.md` RB-31
|
||||
|
||||
RB-31 replaces four hand-rolled state-literal fixtures with `given(reduce, initial)`
|
||||
replays, per ADR-0006 §2 ("no object is built directly; a fixture is the result of
|
||||
running real `Msg`s through the real `reduce`"). This is a fixture-construction change
|
||||
only. No `*.machine.ts` production file was touched.
|
||||
|
||||
## What was wrong
|
||||
|
||||
Four machine specs built their starting `Answering`/`Invullen`/`Editing`/`loaded` state
|
||||
with a local object-literal helper instead of replaying messages:
|
||||
|
||||
- `intake.machine.spec.ts` — `answering(answers, cursor, scholingThreshold)` hardcoded
|
||||
`errors: {}`. `intake.testing.ts` (exporting `givenIntake`) already existed next to
|
||||
it and was already correct, but was imported only by `intake.acceptance.spec.ts`.
|
||||
- `registratie-wizard.machine.spec.ts` — `invullen(draft, cursor)` hardcoded `errors: {}`
|
||||
and `upload: initialUpload`.
|
||||
- `besluit.machine.spec.ts` — `editingWith(besluit, toelichting)` hardcoded `errors: {}`.
|
||||
- `brief.machine.spec.ts` — `loaded(status, sections)` built the `'loaded'` tag object
|
||||
directly (no `errors` field on this union, so this one did not hardcode `errors: {}`,
|
||||
but it still skipped the reducer).
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `apps/ssp/src/app/herregistratie/domain/intake.machine.spec.ts` | Removed the local `answering(...)` helper. Every fixture is now built with the existing `givenIntake` (imported from `intake.testing.ts`), matching `intake.acceptance.spec.ts`'s own style. |
|
||||
| `apps/ssp/src/app/registratie/domain/registratie-wizard.testing.ts` | New. One-liner: `export const givenRegistratieWizard = given(reduce, initial)`. |
|
||||
| `apps/ssp/src/app/registratie/domain/registratie-wizard.machine.spec.ts` | Removed the local `invullen(...)` helper and the now-unused `validAdres`/`validDraft`/`Draft`/`initialUpload` fixtures. Added module-level replay helpers (`toAdresValid`, `toBeroepStep`, `toBeroepStepWithDiploma`, `toControleStep`, `toIndienen`, `toFullDraftAtCursor0`) built from `givenRegistratieWizard` + `reduce`, reused across every `describe` block (the file's pre-existing `reduce (message-driven happy path)` block already had three of these, scoped locally; they are now module-level and shared, removing the duplication). |
|
||||
| `apps/behandelportal/src/app/behandeling/domain/besluit.testing.ts` | New. One-liner: `export const givenBesluit = given(reduce, initial)`. |
|
||||
| `apps/behandelportal/src/app/behandeling/domain/besluit.machine.spec.ts` | Removed the local `editingWith(...)` helper. Every fixture is now built with `givenBesluit` (or, for the empty-draft case, the machine's own `initial` — see below). |
|
||||
| `apps/ssp/src/app/brief/domain/brief.testing.ts` | New. One-liner: `export const givenBrief = given(reduce, initial)`. |
|
||||
| `apps/ssp/src/app/brief/domain/brief.machine.spec.ts` | Rewrote the `loaded(...)` helper to replay a real `BriefLoaded` message through `givenBrief` instead of building the `'loaded'` tag object directly. Also converted one further inline `BriefState` literal in the "deep-copies content" test to the same replay (same anti-pattern, same file, not named individually by the finding's evidence list but visibly the same shape — see "Beyond the letter of the finding" below). |
|
||||
| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-31's status cell: `open` → `implemented`. |
|
||||
|
||||
## Message sequence used per machine
|
||||
|
||||
### `intake.machine.spec.ts`
|
||||
|
||||
Every fixture in this file has `cursor` 0, 1, or 2 and the default or an overridden
|
||||
`scholingThreshold`. All are built as direct sequences of `SetAnswer`/`Next`/`SetPolicy`
|
||||
through `givenIntake`, mirroring `intake.acceptance.spec.ts`'s own explicit style (no new
|
||||
generic wrapper was added — the finding's own resolution is "wire the spec to
|
||||
`givenIntake`", not "invent a second helper"). Representative sequences:
|
||||
|
||||
- Cursor 0, plain answers (most tests): `givenIntake({SetAnswer buitenlandGewerkt}, {SetAnswer uren}, ...)`.
|
||||
- Cursor 0 with an overridden threshold: adds a trailing `{tag:'SetPolicy', scholingThreshold: N}`.
|
||||
- Cursor 1 ("editing an answer leaves the cursor fixed"): `SetAnswer buitenlandGewerkt=ja`,
|
||||
`SetAnswer land`, `SetAnswer buitenlandseUren`, `Next` (buitenland step now valid ->
|
||||
cursor 1), then the edit under test.
|
||||
- Cursor 2 ("gaNaarStap jumps back..."): the above sequence continued with
|
||||
`SetAnswer uren=4160`, `Next` (werk step valid -> cursor 2).
|
||||
|
||||
No drift found here: `intake.testing.ts` already existed correctly, and every one of the
|
||||
nine cursor/threshold combinations the old literal used turned out to be reachable by a
|
||||
real message sequence.
|
||||
|
||||
### `registratie-wizard.machine.spec.ts`
|
||||
|
||||
- `toAdresValid()` = `PrefillAdres(straat, postcode, woonplaats)`, `SetCorrespondentie('post')`
|
||||
— cursor 0, matches the old `invullen(validAdres)`.
|
||||
- `toBeroepStep()` = `reduce(toAdresValid(), Next)` — cursor 0 -> 1, no diploma. Matches
|
||||
`invullen(validAdres, 1)`.
|
||||
- `toBeroepStepWithDiploma()` = `reduce(toBeroepStep(), KiesDiploma('d1','Arts',[]))` —
|
||||
cursor 1, diploma set. Matches `invullen(validDraft, 1)`.
|
||||
- `toControleStep()` = `reduce(toBeroepStepWithDiploma(), Next)` — cursor 1 -> 2. Matches
|
||||
`invullen(validDraft, 2)`.
|
||||
- `toFullDraftAtCursor0()` = `PrefillAdres`, `SetCorrespondentie('post')`, `KiesDiploma(...)`,
|
||||
never advancing the cursor — matches `invullen(validDraft)` (cursor 0). `SetField`/
|
||||
`SetCorrespondentie`/`KiesDiploma` carry no cursor gate, so setting every field before
|
||||
ever pressing `Next` is a genuinely reachable cursor-0 state with a complete draft.
|
||||
- `invullen({})` (five call sites) is exactly the machine's own `initial` value
|
||||
(`{tag:'Invullen', draft:{antwoorden:{}}, cursor:0, errors:{}, upload:initialUpload}`)
|
||||
— replaced with `initial` directly, no message needed.
|
||||
|
||||
### `besluit.machine.spec.ts`
|
||||
|
||||
- `editingWith('')` is exactly `initial` (`draft:{besluit:'',toelichting:''}`) — replaced
|
||||
with `initial` directly.
|
||||
- `editingWith('Afwijzen')` / `editingWith('Goedkeuren')` = `givenBesluit({SetField besluit})`.
|
||||
- `editingWith('Afwijzen', ' niet erkend ')` = `givenBesluit({SetField besluit=Afwijzen}, {SetField toelichting=' niet erkend '})`.
|
||||
- Every `Submitting`/`Failed` fixture is now `givenBesluit({SetField besluit}, {Submit})`
|
||||
composed further with `reduce(..., {SubmitFailed}/{Retry}/{Reset})`.
|
||||
|
||||
### `brief.machine.spec.ts`
|
||||
|
||||
- `loaded(status, sections)` = `givenBrief({tag:'BriefLoaded', brief: briefWith(status, sections), availablePassages: lib, decisions})`.
|
||||
This is a 1:1 replacement: the `'BriefLoaded'` reducer case sets exactly
|
||||
`{tag:'loaded', brief: m.brief, availablePassages: m.availablePassages, decisions: m.decisions}`
|
||||
— the same three fields the old literal built by hand, with the same values. No drift.
|
||||
|
||||
## Drift found
|
||||
|
||||
Two tests in `registratie-wizard.machine.spec.ts` asserted against a cursor value the
|
||||
real reducer cannot reach:
|
||||
|
||||
- `'validateAll keeps only the answers to the questions that applied'` built
|
||||
`invullen(validAdres, 2)` then called `kiesDiploma(...)` on it — i.e. a wizard already
|
||||
at cursor 2 (`controle`) with **no diploma chosen yet**. That is impossible by replay:
|
||||
advancing past `beroep` (cursor 1 -> 2) requires `validateStep('beroep', ...)` to pass,
|
||||
which requires `diplomaId` and `beroep` to already be set. The literal encoded a state
|
||||
the reducer can never produce.
|
||||
- `'requires a declared beroep + all maximal questions before submit'` had the same
|
||||
problem: `invullen(validAdres, 2)` then `kiesHandmatig(...)`, which leaves `beroep`
|
||||
`undefined` — again a cursor-2 state that could never have been reached via `Next`.
|
||||
|
||||
In both cases the cursor value is not actually load-bearing for the test: `submit()`
|
||||
calls `validateAll(s.draft, s.upload)`, which validates every step regardless of
|
||||
`s.cursor`. Both tests were re-pointed at the reachable **cursor-1** equivalent
|
||||
(`toBeroepStep()` then `kiesDiploma`/`kiesHandmatig`), with an inline `// DRIFT (see
|
||||
rb-31.md)` comment at each site. No assertion changed — both tests still check the same
|
||||
`submit(...)` outcome on the same field values; only the now-irrelevant cursor number
|
||||
in the starting fixture moved from an unreachable 2 to a reachable 1.
|
||||
|
||||
No other named state, across any of the four machines, turned out to be unreachable.
|
||||
|
||||
## Beyond the letter of the finding
|
||||
|
||||
`brief.machine.spec.ts`'s `'BesluitSelected deep-copies content...'` test built a second,
|
||||
separate `BriefState` literal inline (not through the `loaded(...)` helper the finding
|
||||
cited) — same anti-pattern, same file, not itself named in ADR-C-010's evidence list.
|
||||
Since it sits inside one of the four files already being brought into line, and the fix
|
||||
is the identical one-line `BriefLoaded` replay, it was converted too rather than left as
|
||||
a residual violation in a file this ticket otherwise fixed. No other spec, in any other
|
||||
file, was touched.
|
||||
|
||||
## `intake.acceptance.spec.ts` — confirmed unaffected
|
||||
|
||||
`intake.testing.ts` and its `givenIntake` export were not modified. The acceptance spec
|
||||
still imports and uses `givenIntake` exactly as before; it was not read or edited by
|
||||
this ticket beyond confirming (by running it) that it still passes.
|
||||
|
||||
## Verification
|
||||
|
||||
- `npx ng test ssp`: 44 test files, 276 tests, all passing (includes
|
||||
`intake.machine.spec.ts`, `intake.acceptance.spec.ts`,
|
||||
`registratie-wizard.machine.spec.ts`, `brief.machine.spec.ts`, and every other ssp
|
||||
spec, unmodified ones included).
|
||||
- `npx ng test behandelportal`: 6 test files, 37 tests, all passing (includes
|
||||
`besluit.machine.spec.ts`).
|
||||
- `npx eslint` on all seven touched/added files: clean.
|
||||
- `npx prettier --check` on all seven touched/added files: clean (one file needed
|
||||
`--write` once, then verified clean).
|
||||
- `npm run ci`: see the commit message / session report for the exit code and step
|
||||
count.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
No `*.machine.ts` reducer or production domain file was changed — every fixture change
|
||||
is confined to the four `*.spec.ts` files and the three new `*.testing.ts` files listed
|
||||
above. No other machine spec (including `change-request.machine.spec.ts`, which the
|
||||
finding notes already honours the idiom inline) was touched.
|
||||
@@ -0,0 +1,86 @@
|
||||
# RB-32 — add the missing `language-switcher` row to the CIBG gap register
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `06-adr-conformance.md`
|
||||
ADR-C-008 · `99-backlog.md` RB-32 · Depends on
|
||||
`implementation/adr-c-007.md` (the same file, left one row short on purpose,
|
||||
filed forward as this ticket)
|
||||
|
||||
## What was wrong
|
||||
|
||||
ADR-0003 §Consequences' final bullet requires every `// CIBG-GAP EXTENSION:`
|
||||
marker in code to have a row in `libs/shared/docs/cibg-gaps.mdx`, "so it's
|
||||
auditable rather than silently drifting." `libs/shared/src/layout/language-switcher/
|
||||
language-switcher.component.ts:7-9` carries a full, well-formed marker
|
||||
("Taal instellen" — no vendored Huisstijl class ships for it — see
|
||||
`cibg-gaps.mdx`) but the register table had no row for it.
|
||||
|
||||
## Verified before editing
|
||||
|
||||
```
|
||||
grep -rln "CIBG-GAP EXTENSION" apps libs --include=*.ts | wc -l # 9
|
||||
```
|
||||
|
||||
Nine files carry the marker: `debug-state`, `language-switcher`, `wizard-shell`,
|
||||
`application-link`, `placeholder-chip`, `rich-text-editor`, `skeleton`,
|
||||
`spinner`, `status-badge`. The register table had 8 rows, and `language-switcher`
|
||||
was the one missing — matching ADR-C-008's finding exactly, re-verified rather
|
||||
than trusted from the finding's own snapshot (per this ticket's DoD point 1, and
|
||||
per `adr-c-007.md`'s own note that it left this exact row for a later ticket).
|
||||
|
||||
## What changed
|
||||
|
||||
One row added to `libs/shared/docs/cibg-gaps.mdx`'s register table, matching the
|
||||
existing rows' two-column shape (component name, closest CIBG concept, reason —
|
||||
wording taken from the component's own marker comment, not invented):
|
||||
|
||||
| Component | Closest CIBG concept | Why hand-rolled |
|
||||
| ------------------- | -------------------- | -------------------------------------------------------------------------------------------------- |
|
||||
| `language-switcher` | Taal instellen | No vendored Huisstijl class ships for it; a small hand-rolled surface built from the token bridge. |
|
||||
|
||||
No other row was touched. The 8 existing rows were each re-checked against their
|
||||
component's current marker comment while the file was open; none needed a change.
|
||||
|
||||
## Optional CI script: taken
|
||||
|
||||
ADR-C-008 flags a ~10-line `grep -l CIBG-GAP | diff`-style script folded into
|
||||
`check:tokens` as an explicitly optional second step. It is genuinely small and
|
||||
fits the existing script's shape, so `scripts/check-tokens.sh` gained one more
|
||||
guard after its existing hardcoded-colour check:
|
||||
|
||||
```bash
|
||||
gap_register='libs/shared/docs/cibg-gaps.mdx'
|
||||
markers=$(grep -rl 'CIBG-GAP EXTENSION' apps libs --include='*.component.ts' | xargs -n1 dirname | xargs -n1 basename | sort -u)
|
||||
rows=$(grep -oP '^\| `\K[^`]+' "$gap_register" | sort -u)
|
||||
missing=$(comm -23 <(echo "$markers") <(echo "$rows"))
|
||||
if [ -n "$missing" ]; then
|
||||
echo "$missing"
|
||||
echo "FAIL: CIBG-GAP EXTENSION marker(s) with no row in $gap_register"
|
||||
exit 1
|
||||
fi
|
||||
echo 'OK: every CIBG-GAP EXTENSION marker has a cibg-gaps.mdx row'
|
||||
```
|
||||
|
||||
The marker's component directory basename (`dirname` of the flagged file,
|
||||
`.component.ts` files only) is compared against the table's backtick-quoted
|
||||
first column, extracted with `grep -oP`. This matches for all 9 current
|
||||
markers, including the two rows with a parenthetical suffix
|
||||
(`` `wizard-shell` (error summary only) ``, `` `application-link` (non-navigating
|
||||
row) ``) — the regex stops at the closing backtick, before the parenthetical.
|
||||
|
||||
**Verified working, not just written.** Added a scratch component
|
||||
(`libs/shared/src/ui/scratch-gap-test/scratch-gap-test.component.ts`, a single
|
||||
`// CIBG-GAP EXTENSION:` line plus a dummy export) with no matching row. Ran
|
||||
`bash scripts/check-tokens.sh`: failed with exit 1, printing `scratch-gap-test`
|
||||
and the expected `FAIL:` line. Deleted the scratch file and its directory (`rm`,
|
||||
not `git checkout` — it was never tracked). Re-ran the script: passed, exit 0.
|
||||
`git status` confirms no trace of the scratch file remains.
|
||||
|
||||
## Verification
|
||||
|
||||
- `npm run check:tokens` (includes the new guard): passes — 9 markers, 9 rows,
|
||||
after the register row was added.
|
||||
- `npm run ci`: result and step count reported in the implementing agent's final
|
||||
answer.
|
||||
- No code changed outside `libs/shared/docs/cibg-gaps.mdx`,
|
||||
`scripts/check-tokens.sh`, `99-backlog.md`, and this note — a doc-only ticket
|
||||
plus its optional, self-verifying guard script.
|
||||
@@ -0,0 +1,100 @@
|
||||
# RB-33 — `unwrapOk`: adopt or delete
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `06-adr-conformance.md` ADR-C-011 ·
|
||||
`99-backlog.md` RB-33
|
||||
|
||||
## Decision: delete
|
||||
|
||||
The ticket names this "adopt or delete", not "adopt", and asks for the judgment call, not
|
||||
the default. I deleted `unwrapOk`.
|
||||
|
||||
## Why delete, not adopt
|
||||
|
||||
`unwrapOk` (`libs/shared/src/testing/value-object.ts`) has had zero consumers across the
|
||||
whole codebase since ADR-0006 shipped it, except its own definition and one sentence in
|
||||
`libs/shared/docs/testing.mdx`. I verified this before changing anything:
|
||||
|
||||
```
|
||||
grep -rn "unwrapOk" apps libs --include=*.ts --include=*.mdx
|
||||
libs/shared/docs/testing.mdx:92: ...unwrapOk(parseX(raw))...
|
||||
libs/shared/src/testing/value-object.ts:9:export function unwrapOk<E, T>(...)
|
||||
libs/shared/src/testing/value-object.ts:11: throw new Error(`unwrapOk: ...`);
|
||||
```
|
||||
|
||||
The one call site the finding names,
|
||||
`apps/ssp/src/app/registratie/application/submit-change-request.spec.ts`, still has the
|
||||
exact hand-rolled guard the finding quotes:
|
||||
|
||||
```ts
|
||||
const telefoon = parseTelefoonnummer('0612345678');
|
||||
if (!telefoon.ok) throw new Error('fixture phone should parse');
|
||||
```
|
||||
|
||||
I also checked whether any other spec has the same shape, in case the finding's "one call
|
||||
site" undercounted the real duplication:
|
||||
|
||||
```
|
||||
grep -rln "if (!.*\.ok)\s*throw" apps libs --include=*.spec.ts
|
||||
apps/ssp/src/app/registratie/application/submit-change-request.spec.ts
|
||||
```
|
||||
|
||||
Only this one file, anywhere. There is no cast (`'x' as Telefoonnummer`) to close off
|
||||
either — the spec already calls the real `parseTelefoonnummer` and checks `.ok` before
|
||||
touching `.value`. ADR-0006 §3's actual requirement ("never a cast") is already met by the
|
||||
inline code, with or without the helper.
|
||||
|
||||
Weighing it honestly:
|
||||
|
||||
- **For adopt:** it is a one-line change, and the ADR's own worked example literally shows
|
||||
this exact call. Doing it would make the finding's "zero adopters" claim technically
|
||||
false.
|
||||
- **For delete:** a helper that gains its _only_ real-codebase consumer by an agent adding
|
||||
that one call site as an act of ticket compliance is not organic adoption — it is
|
||||
manufacturing a usage to justify keeping the file. `unwrapOk` has sat available, exported,
|
||||
and documented since ADR-0006 (well before this session) without a single spec reaching
|
||||
for it on its own. One caller, forever, is not "removing duplication" (the stated point
|
||||
of a shared test helper) — there is no duplication with only one occurrence. The inline
|
||||
guard is also arguably clearer here: its error message (`'fixture phone should parse'`)
|
||||
names the actual fixture, where `unwrapOk`'s generic message
|
||||
(`unwrapOk: expected ok, got error: ...`) does not.
|
||||
|
||||
Delete wins: it removes dead, unadopted code and its stale doc reference, changes no
|
||||
runtime behaviour anywhere, and costs nothing to reverse if a second real need for this
|
||||
idiom shows up later (three lines, trivial to re-add against actual duplication instead of
|
||||
a single hypothetical site).
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `libs/shared/src/testing/value-object.ts` | Deleted. Its only export, `unwrapOk`, is what this ticket removes; the file had nothing else in it. |
|
||||
| `libs/shared/docs/testing.mdx` | Rewrote the sentence that named `unwrapOk` and the deleted file's path. It now states the same rule in plain terms — call the real `parse*` and check `.ok`, never a cast — and keeps the `RemoteData` half of the sentence pointing at `remote-data.ts` (unchanged, still in use). |
|
||||
| `apps/ssp/src/app/registratie/application/submit-change-request.spec.ts` | **Not touched.** Its inline guard already satisfies ADR-0006 §3; this is the "delete" branch, so the fixture-construction behaviour stays exactly as it was. |
|
||||
| `99-backlog.md` | RB-33's status cell: `open` → `implemented`. |
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`docs/reference/architecture/0006-test-data-builders.md` (the ADR itself) still shows
|
||||
`unwrapOk` in its worked example and decision table. That is deliberate: RB-33 is a code
|
||||
ticket, not one of the five ADR-fix tickets that need architect sign-off
|
||||
(`06-adr-conformance.md`'s "ADR-fix tickets" section). The ADR's illustrated pattern
|
||||
("call the real parser, unwrap through a checked path, never a cast") is still the correct
|
||||
principle — this ticket only removes one now-unused concrete implementation of it, which
|
||||
the inline guard in `submit-change-request.spec.ts` already satisfies without the named
|
||||
helper. Amending the ADR's own text is out of this ticket's scope and is left for a future
|
||||
ADR-fix ticket if one is ever raised. The finding document (`06-adr-conformance.md`) and the
|
||||
historical WP-70/WP-71 backlog notes that mention `unwrapOk` are left as-is — they are
|
||||
records of what was true when written, not living code.
|
||||
|
||||
No other file in `libs/shared/src/testing/` was touched (`expect-tag.ts`, `machine.ts`,
|
||||
`remote-data.ts` are all unrelated and still have real consumers).
|
||||
|
||||
## Verification
|
||||
|
||||
- `grep -rn "unwrapOk" apps libs --include=*.ts --include=*.mdx` — zero occurrences.
|
||||
- `apps/ssp/src/app/registratie/application/submit-change-request.spec.ts` — unchanged file,
|
||||
still passes (see `npm run ci` result below).
|
||||
- No new test added. The ticket is a deletion of unused code plus a doc-sentence rewrite;
|
||||
the surviving inline guard in the spec is exercised the same way it always was, by the
|
||||
spec's three existing `it` blocks.
|
||||
- `npm run ci` (foreground): see the session report for the exit code and step count.
|
||||
Reference in New Issue
Block a user