fix(reports): remove dead attachmentsNote override and refactor ReportWizardPage to useReducer

.claude/agent-memory/product-architect/MEMORY.md

@@ -2,7 +2,7 @@
 
 ## Topic Files
 
-- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952)
+- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988)
 - [Dual-rail aggregation](dual-rail-aggregation.md) — Rail A/B tagged-deposit invariants (#1891/PR #1894), residual-denominator rule, isSplit UNION
 - [Source-report split inference](source-report-split-inference.md) — budgetLines[]/deposits[] are this-source-scoped, so †/‡ classification is a proxy; proposed `splitKind`; pdfmake `'2*'` width trap
 - [Story reviews](story-reviews.md) — per-story and per-PR review log

.claude/agent-memory/product-architect/recurring-patterns.md

@@ -708,8 +708,8 @@ discriminates. Good pattern to reuse; also a reminder that a length assertion al
 ### A bumped submodule ref is not a pushed wiki commit (PR #1987)
 
 PR #1987 had the parent ref bumped to a wiki commit that was **never pushed** — the wiki remote was two
-commits behind. `git -C wiki log --oneline` shows the commit as HEAD, so the wiki *looks* published, and
-`git ls-tree HEAD wiki` matches it, so the ref *looks* correct. Anyone cloning the branch and running
+commits behind. `git -C wiki log --oneline` shows the commit as HEAD, so the wiki _looks_ published, and
+`git ls-tree HEAD wiki` matches it, so the ref _looks_ correct. Anyone cloning the branch and running
 `git submodule update` would fail on an unresolvable ref.
 
 Verify with `git -C wiki ls-remote origin master` compared against `git ls-tree HEAD wiki` — those are the
@@ -730,5 +730,59 @@ and nothing else. A scoped ref-bump commit does not disturb an implementer mid-e
 ### Shell heredocs: a bare `cat >> file` with no redirect hangs the tool
 
 `cat >> a.md 2>/dev/null || true` followed by a second `cat >> b.md <<'EOF'` — the heredoc binds to the
-*second* cat, so the first reads stdin and blocks until the 120s timeout. Prefer the Edit/Write tools for
+_second_ cat, so the first reads stdin and blocks until the 120s timeout. Prefer the Edit/Write tools for
 appending to memory files; if you must use bash, one heredoc per command and never a redirect-less `cat`.
+
+### `Pick<State, 'a'|'b'>` is not a forcing function (#1947 action-set review)
+
+A reducer "tier factory" typed `function freshTier(): Pick<State, 'a'|'b'|'c'>` claims to make
+"what does this transition clear" a compile-time decision. It does not: adding a field to `State`
+produces **no error** — the key union just doesn't mention it, the spread leaves it untouched, and it
+silently defaults to _kept_. The key union is a second hand-maintained list, i.e. the very thing being
+replaced. The working version is a **named tier type** (`interface ReportTier {...}`) whose factory has an
+**explicit return-type annotation** and returns a total object literal — missing property = compile error.
+The annotation is load-bearing: an inferred return type re-derives the shape from the literal and the
+error vanishes. Partition state as a flat intersection of tiers, not nested objects (nesting churns every
+read site). Generalises to any "exhaustive mapping" claim made with `Pick`/`Omit`/`Record<keyof …>`.
+
+### Caller-supplied monotonic seq in an action payload reintroduces the ref it replaces (#1947)
+
+`dispatch({type:'SELECT_SOURCE', payload:{ newReportSeq }})` asks the caller to produce a value that must
+stay **in sync with reducer-owned state** — only achievable with an out-of-reducer counter ref, so the
+"staleness is enforced in the reducer" claim is false. Fix: **opaque nullable token** (`requestId: string |
+null`) used as identity, never ordering — caller generates via a module-level `nextRequestId()`, echoes it
+back in the completion action, reducer no-ops on mismatch. `null` then means "nothing in flight, discard
+every outstanding response", so a reset invalidates in-flight work with no bump arithmetic. Monotonicity is
+never needed when nothing compares generations for order. Corollary: an in-flight **boolean flag**
+(`isGeneratingAi`) alongside such a token must be **derived** (`token !== null`), never stored — the two
+disagreeing is exactly the bug class the token exists to kill.
+
+### A refactor's cascade table smuggles behaviour changes (#1947)
+
+Diff every row of a proposed reset/cascade table against the actual handler line-by-line. Two of three rows
+in #1947's table cleared `aiError` where the code does not: one handler never clears it, and the other
+clears it only inside `if (isGeneratingAi)`. Both were reachable, user-visible, and would have landed inside
+a PR whose stated AC was "no user-visible change". Also watch for **generic setter actions**
+(`SET_MAX_STEP`) — a setter wearing an action's clothes preserves the ad-hoc call it was meant to replace
+and names nothing about what it invalidates. And check whether the _unfixed_ instances of the same race
+exist elsewhere in the file (#1947 had a third, unguarded, in the Step-2 fan-out fetch).
+
+### The neutralised trigger left in the code (#1947 `deepLinkAppliedRef`)
+
+When a defect's trigger condition is _neutralised by a new guard_ rather than removed, the guarantee lives
+in a comment. `if (… && !report && !appliedRef.current)` — `!report` was the AC8 trigger, kept alive behind
+a ref and a nine-line comment. Removing the redundant condition also removes `report` from the effect's
+dep array, making "clearing report cannot re-fire this" structural. Look for this shape in any fix that
+_added_ a guard without deleting what it guards against.
+
+### A total-object tier factory only forces a decision in the cases that spread it (PR #1988 review)
+
+Follow-up to the `Pick<>` entry above: getting the factory right is necessary but not sufficient. A named
+tier type + annotated total-literal factory produces the compile error, but **any reducer case that
+hand-lists that tier's fields instead of spreading the factory keeps the hole** — the new field silently
+defaults to _kept_ there. PR #1988 had `freshContentTier()` correct and then bypassed it in `SELECT_SOURCE`
+and `DISCARD_EDITS`, the two cases that clear content state, because each needed one field _preserved_
+(`aiError`). Reviewing a tier-factory design: grep every case for the tier's field names appearing as
+literal keys; each hit is an unenforced case. The fix is always the same shape — spread the factory, then
+name the exception on the next line (`...freshContentTier(), aiError: state.aiError`), which is
+behaviour-identical and makes the KEEP the thing that is written down rather than the CLEAR.

.claude/agent-memory/product-architect/story-reviews.md

@@ -688,7 +688,7 @@ the empty-fallback makes it incapable of emptying a valid field), prompt rule 4
 amended not deleted with the submodule ref bumped on-branch (AC 4.1). 193/193 + 187/187 green locally.
 
 Non-blocking: `futureDateStr` now triplicated (`timeline.test.ts:151` + 2 copies) while
-`server/src/test-helpers/` exists — and the two new copies dropped the JSDoc that carries the *reason*
+`server/src/test-helpers/` exists — and the two new copies dropped the JSDoc that carries the _reason_
 (CPM today-floor on `not_started`), i.e. exactly the knowledge #1913 was filed to preserve.
 
 ### Round 2 (`857fcedd`) — APPROVED
@@ -705,3 +705,30 @@ Accepted residuals, recorded so they are not rediscovered as bugs: `<word …>`
 stripped (INFO-1, AC 2.4's `Beträge < 500 EUR` safe via the space-after-`<` guard); a genuine German date
 list of ≥2 lines (`15. Mai: …\n16. Mai: …`) still loses its numbers, which is arguably correct (INFO-2).
 `futureDateStr` extraction to `server/src/test-helpers/dates.ts` deferred as a follow-up.
+
+## PR #1988 (#1967 dead `attachmentsNote` override + #1947 `ReportWizardPage` → `useReducer`)
+
+### Round 1 (`b503e496`) — CHANGES_REQUIRED, one HIGH
+
+The tier design I specified in the #1947 action-set review landed correctly (named tier interfaces,
+annotated total-literal factories, opaque nullable request tokens, `isGeneratingAi` derived not stored,
+the Step-2 fan-out race tokenized, `deepLinkAppliedRef` holding the applied id with `report` out of the
+dep array). H1 was that `freshContentTier()` was **bypassed** in `SELECT_SOURCE` and `DISCARD_EDITS` —
+the two cascades — because each needed `aiError` preserved, so a future 5th `ContentTier` field would
+silently default to _kept_ in exactly the handler that produced #1943 and M2. See the recurring-patterns
+entry; the fix shape is spread-the-factory-then-name-the-exception.
+
+Method note: for a behaviour-preserving refactor, **green CI is necessary but not the review**. What
+settled AC3 here was walking each silently-changed semantic and proving it unreachable — `GO_TO_STEP`
+now bumping `maxReachedStep` at every call site (no-op: step-1 Next only renders once `useCase` is set,
+and `WizardStepper` gates clickability on `maxReachedStep`), the new `Math.min` step clamps (use case is
+only selectable at step 1, source at step 2), `freshContentTier()` in `SELECT_USE_CASE` (no-op because
+`isDirty` covers all three content fields, and the dirty path dispatches `DISCARD_EDITS` first).
+
+### Round 2 (`01d8ff12`) — APPROVED
+
+Both cascades now spread the factory and override `aiError` back; the M-I test passes unmodified, which is
+the pin that matters. 57 tests, 100% on all four metrics. Also confirmed the one-file Prettier fix did not
+drag repo-wide drift with it. Carried forward non-blocking: whole-`wizardState` in a `useCallback` dep
+array, `REPORT_REFRESHED` as the last untokenized async write, `reportStatus` duplicating the page-local
+`PageStatus` union, and a comma-operator exhaustiveness guard that invites deletion.

.claude/agent-memory/qa-integration-tester/story-1947-wizardreducer.md

@@ -0,0 +1,21 @@
+---
+name: story-1947-wizardreducer
+description: wizardReducer.ts pure unit test patterns — tier factories, staleness guards, M-I/M-J regression tests, exhaustiveness guard
+metadata:
+  type: project
+---
+
+New pure unit test file `client/src/pages/ReportWizardPage/wizardReducer.test.ts` (57 tests, 100% coverage on wizardReducer.ts).
+
+**Why:** Story #1947 extracted inline reducer logic from ReportWizardPage into a pure module; QA owns the unit tests.
+
+**Key patterns used:**
+
+- `makeState(overrides?)` helper spreads `createInitialWizardState(null)` — keeps tests minimal and focused.
+- Staleness guards tested with `toBe(state)` (same object reference) — proves the reducer short-circuits, not just "returns equivalent state".
+- M-I regression (SELECT_SOURCE must NOT clear `aiError`): tested with a comment that the guard must fail if someone adds `aiError: ''` to that case.
+- M-J regression (REPORT_REFRESHED no-op when report=null): `toBe(state)` reference check.
+- Exhaustiveness guard (line 282, `action satisfies never`): cast to `any` to hit default branch.
+- `nextRequestId` exported function: test that two consecutive calls produce strings where the second is +1 of the first (counter monotonically increases).
+
+**How to apply:** For future pure reducer modules: use the same `makeState()` pattern, test staleness guards with `toBe`, test exhaustiveness with `any` cast.

client/src/lib/reportContent/applyOverrides.test.ts

@@ -4,7 +4,7 @@
  * applyOverrides is a pure function: given a baseline ReportContent and a flat
  * ReportContentOverrides map, it returns a NEW ReportContent with the recognized override keys
  * applied, without mutating the input. Recognized keys: coverLetter.{sender,recipient,reference,
- * subject,body} and row.<invoiceId>.{usageText,attachmentsNote}. Unknown keys are silently
+ * subject,body,signature} and row.<invoiceId>.usageText. Unknown keys are silently
  * ignored. Overriding coverLetter.sender recomputes coverLetter.signature.
  */
 import { describe, it, expect } from '@jest/globals';
@@ -291,20 +291,6 @@ describe('applyOverrides — row overrides', () => {
     expect(result.rows.find((r) => r.invoiceId === 'inv-b')!.usageText).toBe('B baseline');
   });
 
-  it('overrides attachmentsNote with a non-empty string', () => {
-    const row = makeRow({ attachmentsNote: '1 attachment: Invoice' });
-    const content = makeContent({ rows: [row] });
-    const result = applyOverrides(content, { 'row.inv-1.attachmentsNote': 'Edited note' });
-    expect(result.rows[0]!.attachmentsNote).toBe('Edited note');
-  });
-
-  it('overriding attachmentsNote with an empty string coerces it to null', () => {
-    const row = makeRow({ attachmentsNote: '1 attachment: Invoice' });
-    const content = makeContent({ rows: [row] });
-    const result = applyOverrides(content, { 'row.inv-1.attachmentsNote': '' });
-    expect(result.rows[0]!.attachmentsNote).toBeNull();
-  });
-
   it('overriding usageText with an empty string coerces it to an empty string (never null)', () => {
     const content = makeContent();
     const result = applyOverrides(content, { 'row.inv-1.usageText': '' });
@@ -318,15 +304,23 @@ describe('applyOverrides — row overrides', () => {
     expect(result.rows[0]!.usageText).toBe(content.rows[0]!.usageText);
   });
 
-  it('applies both usageText and attachmentsNote overrides for the same row together', () => {
-    const row = makeRow({ attachmentsNote: 'baseline note' });
-    const content = makeContent({ rows: [row] });
-    const result = applyOverrides(content, {
-      'row.inv-1.usageText': 'Edited usage',
-      'row.inv-1.attachmentsNote': 'Edited note',
-    });
-    expect(result.rows[0]!.usageText).toBe('Edited usage');
-    expect(result.rows[0]!.attachmentsNote).toBe('Edited note');
+  it('silently ignores the row.attachmentsNote key (dead since #1959)', () => {
+    // Arrange: content with an invoice row where attachmentsNote is null (the default)
+    const content = makeContent();
+    // Act: call applyOverrides with the attachmentsNote key — no `in`-check exists for it in
+    // applyOverrides.ts, so it cannot update any row field
+    const result = applyOverrides(content, { 'row.inv-1.attachmentsNote': 'some-override' });
+    // Assert: attachmentsNote is still null — the key is silently ignored
+    expect(result.rows[0]!.attachmentsNote).toBeNull();
+  });
+
+  it('still applies usageText override (positive control for remaining field coverage)', () => {
+    // Arrange: content with an invoice row
+    const content = makeContent();
+    // Act: apply the usageText key — it IS in the `in`-check inside applyOverrides.ts
+    const result = applyOverrides(content, { 'row.inv-1.usageText': 'positive-control' });
+    // Assert: the field was updated — proves the row-loop is still wired up correctly
+    expect(result.rows[0]!.usageText).toBe('positive-control');
   });
 });
 

client/src/lib/reportContent/applyOverrides.ts

@@ -1,7 +1,7 @@
 /**
  * Apply user overrides to baseline ReportContent.
  * Pure function: returns a new ReportContent without mutating the input.
- * Recognized override keys: coverLetter.{sender,recipient,reference,subject,body,signature}, row.<id>.{usageText,attachmentsNote}
+ * Recognized override keys: coverLetter.{sender,recipient,reference,subject,body,signature}, row.<id>.usageText
  * Unknown keys are silently ignored.
  * When sender is overridden, signature is recomputed from it UNLESS signature has itself been
  * explicitly overridden — an explicit signature override always wins (AC 2.6).
@@ -86,9 +86,6 @@ export function applyOverrides(
     if (rowKeys.usageText in overrides) {
       row.usageText = overrides[rowKeys.usageText] || '';
     }
-    if (rowKeys.attachmentsNote in overrides) {
-      row.attachmentsNote = overrides[rowKeys.attachmentsNote] || null;
-    }
   }
 
   return result;

client/src/lib/reportContent/overrideKeys.test.ts

@@ -4,7 +4,7 @@
  * `overrideKey` is a pure, side-effect-free builder for override map keys, decoupling
  * applyOverrides.ts and ReportContentEditor.tsx from manually-constructed string literals.
  * `overrideKey.coverLetter` is a fixed set of literal string constants; `overrideKey.row(id)` is a
- * factory that interpolates a given invoiceId into two field-specific keys.
+ * factory that interpolates a given invoiceId into one field-specific key.
  */
 import { describe, it, expect } from '@jest/globals';
 import { overrideKey } from './overrideKeys.js';
@@ -41,10 +41,9 @@ describe('overrideKey.coverLetter — fixed literal keys', () => {
 });
 
 describe('overrideKey.row(invoiceId) — interpolated per-row keys', () => {
-  it('interpolates a simple invoiceId into both the usageText and attachmentsNote keys', () => {
+  it('interpolates a simple invoiceId into the usageText key', () => {
     expect(overrideKey.row('inv-1')).toEqual({
       usageText: 'row.inv-1.usageText',
-      attachmentsNote: 'row.inv-1.attachmentsNote',
     });
   });
 
@@ -58,11 +57,10 @@ describe('overrideKey.row(invoiceId) — interpolated per-row keys', () => {
 
   it('correctly interpolates an invoiceId that itself contains a literal "." with no key ambiguity', () => {
     // A UUID-like or namespaced invoiceId containing dots must not be confused with the key's own
-    // "row." / ".usageText" / ".attachmentsNote" structural dot-separators — the whole id is used
-    // verbatim as the middle segment, however many dots it contains.
+    // "row." / ".usageText" structural dot-separators — the whole id is used verbatim as the
+    // middle segment, however many dots it contains.
     const keys = overrideKey.row('src.2026.inv-42');
     expect(keys.usageText).toBe('row.src.2026.inv-42.usageText');
-    expect(keys.attachmentsNote).toBe('row.src.2026.inv-42.attachmentsNote');
 
     // Splitting on '.' yields more than 3 segments (proving the id's own dots survived verbatim,
     // rather than being collapsed/stripped), and the first/last segments are still the fixed
@@ -77,8 +75,6 @@ describe('overrideKey.row(invoiceId) — interpolated per-row keys', () => {
     const keys = overrideKey.row('any-id-123');
     expect(keys.usageText.startsWith('row.')).toBe(true);
     expect(keys.usageText.endsWith('.usageText')).toBe(true);
-    expect(keys.attachmentsNote.startsWith('row.')).toBe(true);
-    expect(keys.attachmentsNote.endsWith('.attachmentsNote')).toBe(true);
   });
 
   it('returns a fresh object on each call (not a shared/mutated singleton)', () => {

client/src/lib/reportContent/overrideKeys.ts

@@ -14,6 +14,5 @@ export const overrideKey = {
   },
   row: (invoiceId: string) => ({
     usageText: `row.${invoiceId}.usageText`,
-    attachmentsNote: `row.${invoiceId}.attachmentsNote`,
   }),
 } as const;