Merge RB-33 — delete unwrapOk, the unadopted test value-object helper
ADR-C-011: unwrapOk had zero consumers anywhere in apps/ or libs/ since it shipped, and the one candidate call site (submit-change-request.spec.ts's inline parse-and-throw guard) already satisfies ADR-0006 section 3's real requirement, never a cast. Manufacturing a first caller to justify keeping the helper would remove no real duplication. Deleted rather than adopted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -101,7 +101,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
16-row "Compliance review required" list, carries it — regardless of priority.
|
||||
|
||||
| ID | Module | Category | Description | Baseline metric improved | Effort | Risk | Priority | CD batch # | Depends on | Compliance | Status |
|
||||
| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | -------- |
|
||||
| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | ----------- |
|
||||
| **RB-01** | backend/Program.cs + Data | security | Add an owner/capability check to `GET /uploads/{id}/content` and `/uploads/status`; 404 not 403 | §3c Data 75.5% branch vs 99.0% line (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-02** | backend/Program.cs + Data | privacy | Stop concatenating the BSN into `AuthzAudit.Resource`; assert on **values** in the test | §3c Data 75.5% branch (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-03** | backend/Contracts | privacy | `MaskTail(a.Owner, 3)` in `ToAdminSummaryDto` — both cross-owner lists inherit it | §3a bhp/behandeling 91.6%/81.5%; §7 Mapping row | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** |
|
||||
@@ -134,7 +134,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
| **RB-30** | backend/Data + Domain | testability | Extract 5 brief guards into `Domain/Letters/BriefRules.cs`; add `tests/Domain/BriefRuleTests.cs` | §3c Data **75.5% branch** (BL-005); §4b `BriefStore.cs` CC 17, `ToDto` CC 16 | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** |
|
||||
| **RB-31** | 4 app contexts (specs only) | ADR conform. | Replace hand-rolled state literals with `given(reduce, initial)` replays in 4 machine specs | §7 Elm machines 9 (1 has a `*.testing.ts`); §3a herreg 67.8% / brief 68.8% branch | M | Low | P2 | 6 | — | — | open |
|
||||
| **RB-32** | libs/shared/docs | ADR conform. | Add the missing `language-switcher` row to the CIBG gap register (9 markers vs 8 rows) | §2 libs/shared 86 files / 5 194 lines; §6 layout Ca 22 | S | Low | P3 | 6 | — | — | open |
|
||||
| **RB-33** | libs/shared/testing | ADR conform. | Adopt `unwrapOk` at its one call site — **or delete it**; both satisfy ADR-0006 §3 | BL-004; §3a libs/shared/testing 3 files, 100% line | S | Low | P3 | 6 | — | — | open |
|
||||
| **RB-33** | libs/shared/testing | ADR conform. | Adopt `unwrapOk` at its one call site — **or delete it**; both satisfy ADR-0006 §3 | BL-004; §3a libs/shared/testing 3 files, 100% line | S | Low | P3 | 6 | — | — | implemented |
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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.
|
||||
@@ -89,11 +89,11 @@ can only ever be a state the real reducer actually produces:
|
||||
const atStep3 = givenIntake(Start(), SetUren('1200'), Next(), SetDiplomaHerkomst('NL'), Next());
|
||||
```
|
||||
|
||||
The same rule extends to value objects (`unwrapOk(parseX(raw))` instead of a cast) and to
|
||||
`RemoteData` (`loading()` / `success(v)` / `failure(e)` in
|
||||
`libs/shared/src/testing/{value-object,remote-data}.ts` instead of a redefined-per-file
|
||||
literal). **Never** a `.withX().withY()` builder over an open constructor — that just
|
||||
re-opens whatever illegal state the domain closed.
|
||||
The same rule extends to value objects — call the real `parse*` and check `.ok` before use,
|
||||
never a cast — and to `RemoteData` (`loading()` / `success(v)` / `failure(e)` in
|
||||
`libs/shared/src/testing/remote-data.ts` instead of a redefined-per-file literal). **Never**
|
||||
a `.withX().withY()` builder over an open constructor — that just re-opens whatever illegal
|
||||
state the domain closed.
|
||||
|
||||
## UI = Storybook, not heavy component tests
|
||||
|
||||
|
||||
@@ -1,14 +0,0 @@
|
||||
import { Result } from '@shared/kernel/fp';
|
||||
|
||||
/**
|
||||
* Unwrap a `Result` produced by a REAL `parse*` value-object parser, throwing
|
||||
* if it isn't `ok`. This is the only sanctioned way for a spec to obtain a
|
||||
* branded value-object type — it closes off the `'garbage' as Postcode` cast
|
||||
* route, since the only door to the branded type is the parser itself.
|
||||
*/
|
||||
export function unwrapOk<E, T>(result: Result<E, T>): T {
|
||||
if (!result.ok) {
|
||||
throw new Error(`unwrapOk: expected ok, got error: ${JSON.stringify(result.error)}`);
|
||||
}
|
||||
return result.value;
|
||||
}
|
||||
Reference in New Issue
Block a user