diff --git a/.claude/TODO/2026-04-17-components-template-editor-merge-token-resolver.md b/.claude/TODO/2026-04-17-components-template-editor-merge-token-resolver.md
deleted file mode 100644
index 224a6d1..0000000
--- a/.claude/TODO/2026-04-17-components-template-editor-merge-token-resolver.md
+++ /dev/null
@@ -1,37 +0,0 @@
----
-title: Merge tokens {{Field_Name}} have no resolver anywhere in-tree
-severity: medium
-tags: [bug, refactor]
-area: components
-files:
- - src/components/template-editor/merge-fields.ts
- - src/components/template-editor/merge-field-picker.tsx
- - src/components/template-editor/actions.ts
-discovered: 2026-04-17
-discovered_by: components-template-editor
-status: open
----
-
-## Problem
-`MergeFieldPicker` inserts tokens of the form `{{First_Name}}`, `{{Email_Address}}`, `{{Unsubscribe_URL}}`, etc., into MJML content. `compileMjml` passes the tokens through as literal text — the HTML output still contains `{{...}}`. Nothing in `src/components/template-editor/`, `src/lib/`, or the services layer replaces these tokens with contact data before send or export. A user generating HTML from this tool and pasting it into a mail client will send literal `{{First_Name}}` to recipients.
-
-## Evidence
-- `src/components/template-editor/merge-fields.ts:11-27` — tokens defined as plain `{{...}}` strings.
-- `src/components/template-editor/merge-field-picker.tsx:27-55` — insertion writes the raw token into component content.
-- `src/components/template-editor/actions.ts:15-38` — `compileMjml` only calls `mjml2html`; no token substitution.
-- `Grep "{{First_Name}}" src/` — only appears in template-editor sources (definition + test fixtures if any); no resolver.
-
-## Proposed fix
-1. Define a token resolver: `resolveMergeTokens(html: string, ctx: { contact, household, congregation, system }): string` — can live in a new `src/lib/merge-tokens.ts` or as a service method.
-2. Invoke it at the point of use:
- - If sending email from this tool → call after `compileMjml` before dispatch.
- - If exporting for downstream MP use → document that MP's mail merge is the resolver and keep tokens literal.
-3. Add a coverage table mapping each `MERGE_FIELDS` entry to its source (MP column / computed value) so drift between the picker list and the resolver is catchable in tests.
-4. Until a resolver lands, either:
- - Put a visible banner in `EditorExportDialog` noting "tokens will not be resolved by this export," or
- - Compile a warning list in `EditorCodeDialog`/`EditorExportDialog` showing unresolved tokens in the current output.
-
-## Impact if not fixed
-- Silent data-quality bug: recipients receive emails with literal `{{First_Name}}`.
-- The "Merge Fields" feature is misleading — the picker implies substitution happens.
-- Adding a resolver later without tests will risk resolver ↔ picker drift (new tokens in the picker, none in the resolver).
diff --git a/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md b/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md
deleted file mode 100644
index 5b184da..0000000
--- a/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md
+++ /dev/null
@@ -1,78 +0,0 @@
----
-title: No tests for src/components/template-editor/
-severity: medium
-tags: [missing-test]
-area: components
-files:
- - src/components/template-editor/template-editor-form.tsx
- - src/components/template-editor/editor-canvas.tsx
- - src/components/template-editor/editor-toolbar.tsx
- - src/components/template-editor/editor-code-dialog.tsx
- - src/components/template-editor/editor-import-dialog.tsx
- - src/components/template-editor/editor-export-dialog.tsx
- - src/components/template-editor/merge-field-picker.tsx
- - src/components/template-editor/merge-fields.ts
- - src/components/template-editor/grapes-config.ts
- - src/components/template-editor/actions.ts
-discovered: 2026-04-17
-discovered_by: components-template-editor
-status: resolved
----
-
-## Problem
-The entire `src/components/template-editor/` directory has zero co-located test files, despite containing a server action (`compileMjml`) with auth-gating and size validation, token helpers (`merge-fields.ts`), a config factory (`grapes-config.ts`), and a non-trivial insertion rule in `MergeFieldPicker`. The baseline facts snapshot lists 37 test files; none are under `template-editor/`.
-
-## Evidence
-- `Glob src/components/template-editor/**/*.test.*` — no matches
-- `.claude/references/_meta/facts/2026-04-17.md` — 37 test files inventory, none under `template-editor/`
-- `src/components/template-editor/actions.ts:15-38` — `compileMjml` has auth + size checks that are untested
-- `src/components/template-editor/merge-fields.ts:34-59` — `getFieldsByCategory`, `registerMergeFieldBlocks` untested
-
-## Proposed fix
-Add at minimum:
-
-1. `src/components/template-editor/actions.test.ts` — mock `@/lib/auth` (`auth.api.getSession`) and `next/headers`; assert `Unauthorized` when no session; assert size-guard rejects empty and >500KB input; mock `mjml` dynamic import and assert result shape.
-2. `src/components/template-editor/merge-fields.test.ts` — assert `MERGE_FIELD_CATEGORIES` dedup order, `getFieldsByCategory` filtering, and that `registerMergeFieldBlocks` calls `editor.Blocks.add` twice with expected ids.
-3. (Optional) `src/components/template-editor/merge-field-picker.test.tsx` — render-with-mocked-editor covering the "selected mj-text" vs "new mj-section" insertion branches.
-
-## Impact if not fixed
-- Regressions in `compileMjml`'s auth gate or size cap can ship unnoticed (security/DoS risk).
-- Merge-field category changes or taxonomy refactors will not surface breakage.
-- Phase 2 of the template editor (adding MP persistence, token resolution) will be harder to land safely without a baseline.
-
----
-
-## Resolution (2026-09-13)
-Closed by the coverage push. Every file in `src/components/template-editor/`
-now has a co-located test, plus the two route files:
-
-| File | Tests | Statements | Lines |
-|---|---|---|---|
-| `merge-fields.ts` | 5 | 100% | 100% |
-| `grapes-config.ts` | 6 | 100% | 100% |
-| `actions.ts` | 7 | 100% | 100% |
-| `merge-field-picker.tsx` | 7 | 100% | 100% |
-| `editor-import-dialog.tsx` | 8 | 95.65% | 100% |
-| `editor-code-dialog.tsx` | 11 | 97.67% | 100% |
-| `editor-export-dialog.tsx` | 11 | 98.08% | 100% |
-| `editor-toolbar.tsx` | 21 | 100% | 100% |
-| `editor-canvas.tsx` | 7 | 100% | 100% |
-| `template-editor-form.tsx` | 3 | 100% | 100% |
-| `templateeditor/template-editor.tsx` | 6 | 100% | 100% |
-| `templateeditor/page.tsx` | 1 | 100% | 100% |
-
-Directory aggregate: 98.8% statements, 100% lines, across 90 tests.
-
-Two mocking notes worth keeping, both learned the hard way:
-- `useEditor()` must be mocked with a **stable object reference** built once via
- `vi.hoisted()`. Several components run `useEffect`s keyed on the editor
- object's identity, so a `useEditor: () => ({...})` factory returns a new
- object on every render and spins an infinite render loop that hangs the runner.
-- `editor-canvas.tsx` imports real `.css`, which this repo's Vite/PostCSS
- config cannot process under test — both stylesheet imports are mocked to
- empty modules.
-
-The two sibling template-editor TODOs (`-no-mp-persistence`,
-`-merge-token-resolver`) were re-verified against the code during this work and
-remain **accurate and open**. The new tests document current behaviour; they do
-not paper over either defect.
diff --git a/.claude/TODO/2026-04-17-components-template-editor-no-mp-persistence.md b/.claude/TODO/2026-04-17-components-template-editor-no-mp-persistence.md
deleted file mode 100644
index 79d338d..0000000
--- a/.claude/TODO/2026-04-17-components-template-editor-no-mp-persistence.md
+++ /dev/null
@@ -1,44 +0,0 @@
----
-title: Template editor ignores pageID/recordID and never saves to MP
-severity: medium
-tags: [bug, drift]
-area: components
-files:
- - src/app/(web)/tools/templateeditor/template-editor.tsx
- - src/components/template-editor/grapes-config.ts
- - src/components/template-editor/template-editor-form.tsx
- - src/components/template-editor/actions.ts
-discovered: 2026-04-17
-discovered_by: components-template-editor
-status: open
----
-
-## Problem
-The `/tools/templateeditor` route accepts `ToolParams` (`pageID`, `recordID`) via `parseToolParams` and displays them in the info popover, but no code loads an MP template record on mount or writes changes back. `grapes-config.ts` hard-codes `storageManager.type = 'local'` with a single shared key `'mp-template-editor'`, so every user and every invocation (regardless of `pageID`/`recordID`) shares the same browser-local draft. The in-doc comment states "Storage: localStorage for Phase 1" — this is drift between the tool's apparent contract (MP-integrated template tool) and its actual behavior (browser-scratchpad).
-
-Users opening the tool from a specific MP page/record will:
-- Not see that template loaded.
-- Overwrite their unrelated local draft if they start editing.
-- Lose all work on browser-data clear or device switch.
-- Have no way to save changes to MP.
-
-## Evidence
-- `src/app/(web)/tools/templateeditor/template-editor.tsx:28-35` — `params.pageID` and `params.recordID` are only rendered as text; no fetch.
-- `src/components/template-editor/grapes-config.ts:23-36` — `storageManager: { type: 'local', ..., options: { local: { key: STORAGE_KEY } } }` with a constant key.
-- `src/components/template-editor/actions.ts` — only exports `compileMjml`; no `loadTemplate`/`saveTemplate` actions.
-- `src/components/template-editor/editor-canvas.tsx:27-35` — seeds `DEFAULT_MJML_TEMPLATE` when localStorage is empty, regardless of route params.
-
-## Proposed fix
-Choose one of:
-
-1. **MP-integrated persistence** (preferred for a "Template Editor" tool):
- - Add server actions `loadTemplate(pageID, recordID)` and `saveTemplate(pageID, recordID, mjml)` in `src/components/template-editor/actions.ts` backed by a new `TemplateService` or an existing MP table (e.g., a `Templates` / `Email_Templates` entity — pick per MP schema).
- - Thread `params` from `TemplateEditor` into `TemplateEditorForm`/`EditorCanvas` so it loads on mount and saves on explicit action (replace or complement autosave).
- - Scope the localStorage key by `pageID`/`recordID` or disable local storage once server persistence is in place.
-
-2. **UI honesty** (minimum): update the info popover + tool copy to say "browser-local drafts only; MP save coming in a later phase," and hide the `pageID`/`recordID` line since they have no effect.
-
-## Impact if not fixed
-- Users lose work unexpectedly.
-- Multiple users on a shared workstation clobber each other's drafts.
-- Expectations set by the tool's info text ("Create and edit email templates") don't match behavior — credibility and support cost.
diff --git a/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md b/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md
deleted file mode 100644
index e39eaba..0000000
--- a/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md
+++ /dev/null
@@ -1,71 +0,0 @@
----
-title: Radix Select fields switched from uncontrolled to controlled on first selection
-severity: medium
-tags: [bug]
-area: components
-files: [src/components/group-wizard/step-attributes.tsx, src/components/group-wizard/step-identity.tsx, src/components/group-wizard/step-meeting.tsx, src/components/group-wizard/step-organization.tsx, src/app/(web)/tools/addeditfamily/add-edit-family.tsx]
-discovered: 2026-09-13
-discovered_by: coverage-review-orchestrator
-status: resolved
----
-
-## Problem
-Fourteen `Select` fields across the group wizard and Add/Edit Family passed
-`undefined` as their value while nothing was selected:
-
-```tsx
-value={field.value ? String(field.value) : undefined}
-```
-
-A Radix `Select` with `value={undefined}` is **uncontrolled**. As soon as the
-user picked an option, `field.value` became truthy and the component received a
-string — switching to **controlled** mid-life. React logs:
-
-```
-Select is changing from uncontrolled to controlled. Components should not
-switch from controlled to uncontrolled (or vice versa). Decide between using
-a controlled or uncontrolled value for the lifetime of the component.
-```
-
-Beyond the console noise, this is a real correctness problem: while the field
-is uncontrolled, React state is not the source of truth for it. Programmatic
-resets — `form.reset()` between wizard runs, or clearing a field back to its
-empty value — are not guaranteed to be reflected in the rendered trigger,
-because the component keeps its own internal value until it becomes controlled.
-
-The falsy check compounds it. `field.value` of `0` is a legitimate lookup id in
-Ministry Platform, and it is falsy, so a genuinely-selected id of 0 would also
-have rendered as "no selection".
-
-## Evidence
-- `src/components/group-wizard/step-meeting.tsx:49,95,123,151`
-- `src/components/group-wizard/step-attributes.tsx:70,98,126,154`
-- `src/components/group-wizard/step-organization.tsx:59,87,162`
-- `src/components/group-wizard/step-identity.tsx:64,128`
-- `src/app/(web)/tools/addeditfamily/add-edit-family.tsx:1168`
- (variant: `value={value > 0 ? String(value) : undefined}`)
-- Surfaced by the new step tests — 7 warnings per `vitest run`, e.g.
- `step-organization.test.tsx > StepOrganization > selects a congregation`.
-
-## Fix applied
-All fourteen sites now pass `""` instead of `undefined`:
-
-```tsx
-value={field.value ? String(field.value) : ""}
-```
-
-`""` is Radix's documented "no selection" value for a controlled `Select`: the
-`SelectValue` placeholder still renders, and the component is controlled for
-its entire lifetime. (`SelectItem` may not use `""` as its own value, which is
-what makes it safe as the sentinel.)
-
-Verified: 1,535 tests pass, `npm run test:run` emits zero warnings, `tsc
---noEmit` is clean, and `npm run build` succeeds.
-
-## Follow-up worth considering
-The `field.value ? ...` truthiness test is still wrong for a legitimate id of
-`0`. No current lookup uses 0 as a real id, so this is latent rather than
-live — but the correct predicate is `field.value != null`, and a future MP
-lookup that does use 0 would fail silently. Left as-is here because changing
-the predicate is a behavioural change beyond the warning fix, and it deserves
-its own review against the actual lookup data.
diff --git a/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md b/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md
deleted file mode 100644
index 643b5f0..0000000
--- a/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md
+++ /dev/null
@@ -1,66 +0,0 @@
----
-title: Unreachable "empty STEP_FIELDS" branch in GroupWizard.handleNext
-severity: low
-tags: [refactor, missing-test]
-area: components
-files: [src/app/(web)/tools/groupwizard/group-wizard.tsx]
-discovered: 2026-09-13
-discovered_by: coverage-agent-group-wizard
-status: open
----
-
-## Problem
-`handleNext` in `group-wizard.tsx` (lines 120-133) special-cases a step whose
-`STEP_FIELDS[currentStep]` entry is missing or empty by skipping validation
-and advancing immediately:
-
-```ts
-const fields = STEP_FIELDS[currentStep];
-if (!fields || fields.length === 0) {
- setCompletedSteps((prev) => new Set(prev).add(currentStep));
- setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1));
- return;
-}
-```
-
-The only entry in `STEP_FIELDS` (see `schema.ts`) with an empty array is
-index `5` (the Review step, `STEP_FIELDS[5] = []`). But `WizardNavigation`
-never renders a "Next" button on the last step (`currentStep === totalSteps - 1`)
-— it renders the Submit/"Save Changes" button instead, which calls
-`handleSubmit`, not `handleNext`. `STEP_FIELDS` is otherwise fully populated
-for indices 0-4, and `currentStep` never exceeds `WIZARD_STEPS.length - 1`
-(5), so this branch is unreachable through the actual UI.
-
-This isn't a security or correctness bug (nothing lets a user skip real
-validation — the branch simply can never execute), but it is 3 statements of
-dead defensive code that both inflates the file's apparent branch count and
-cannot be exercised by any user-facing test, which is why coverage tooling
-flags `group-wizard.tsx` at 94.5% statements instead of ≥95% despite every
-reachable branch being covered.
-
-## Evidence
-- `src/app/(web)/tools/groupwizard/group-wizard.tsx:120-126` — the
- `!fields || fields.length === 0` branch.
-- `src/components/group-wizard/schema.ts:58-65` — `STEP_FIELDS`, only index
- `5` is empty, and index 5 is the last step.
-- `src/components/group-wizard/wizard-navigation.tsx:28,55-75` — `isReviewStep`
- gates rendering Submit vs. Next; the last step never shows the Next button
- that calls `handleNext`.
-- `npx vitest run --coverage` on `group-wizard.tsx` reports lines 123-125
- uncovered even after `src/app/(web)/tools/groupwizard/group-wizard.test.tsx`
- was extended with 12 additional cases covering every other branch/function
- in the file (94.5% statements / 96.25% lines final).
-
-## Proposed fix
-Either:
-1. Remove the `!fields || fields.length === 0` special case entirely (the
- `form.trigger([])` call with an empty array resolves `true` immediately
- in react-hook-form, so simplifying to always call `form.trigger(fields)`
- would behave identically and remove the dead branch), or
-2. If the branch is meant as future-proofing for a step reachable via some
- other entry point, add a code comment explaining that intent so the next
- reader (and coverage tooling) doesn't flag it as untested dead code.
-
-## Impact if not fixed
-None functionally — this is a coverage/clarity issue, not a behavior bug.
-Low priority; safe to leave as documented, known-dead code.
diff --git a/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md b/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md
deleted file mode 100644
index dc9d811..0000000
--- a/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md
+++ /dev/null
@@ -1,86 +0,0 @@
----
-title: mergeTemplate (and sibling actions) console.error the raw docxtemplater error, which can carry household addresses
-severity: high
-tags: [security, bug]
-area: components
-files: [src/components/address-labels/actions.ts]
-discovered: 2026-09-13
-discovered_by: coverage-agent-address-labels
-status: resolved
----
-
-## Problem
-
-CLAUDE.md rule 14 requires that errors log "identifiers and shape" only — never
-record content — "including inside thrown error messages, which travel further
-than logs do." `mergeTemplate` (and its siblings `generateLabelPdf` /
-`generateLabelDocx`) violate this by passing the entire caught `error` object to
-`console.error`, not just `error.message`.
-
-For `mergeTemplate` this is a concrete PII leak, not a theoretical one:
-docxtemplater's scope-parser error path attaches the live merge scope object to
-`err.properties.scope` (see `node_modules/docxtemplater/js/errors.js:593-600`,
-function that throws `"scopeparser_execution_failed"`). In this feature, the
-merge scope is exactly the `addresses` array built in `mergeTemplate` — each
-entry carries `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`,
-`PostalCode` for a real household. A user-uploaded template with a malformed
-tag (e.g. a token that calls a method that throws, or a bad expression) causes
-docxtemplater to throw with that scope attached, and
-`console.error('mergeTemplate error:', error)` writes the full object —
-including the nested `.properties.scope` with every printable household's
-name and address — to server logs.
-
-## Evidence
-
-- `src/components/address-labels/actions.ts:328` — `console.error('mergeTemplate error:', error);` logs the raw `error`, not `error.message`.
-- `src/components/address-labels/actions.ts:189` and `:232` — same pattern for `generateLabelPdf` / `generateLabelDocx` (lower risk here, but same anti-pattern).
-- `node_modules/docxtemplater/js/errors.js:593-601` — `err.properties.scope = scope;` attaches the full render scope (the caller's data) to certain template errors.
-- `mergeTemplate`'s `addresses` array (`actions.ts:286-296`) contains `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`, `PostalCode` — real household PII — which is exactly what would appear in that scope.
-
-## Proposed fix
-
-Log only identifiers/shape, per rule 14: e.g.
-```ts
-console.error('mergeTemplate error:', error instanceof Error ? error.message : 'non-Error thrown');
-```
-or, if the `docxtemplater` error's `.properties.id`/`.name` is useful for
-diagnosing which failure mode occurred, log those specific fields explicitly
-rather than the object itself — never `error` or `error.properties` in bulk.
-Apply the same change to the `console.error` calls in `generateLabelPdf` and
-`generateLabelDocx` for consistency and defense in depth.
-
-## Impact if not fixed
-
-Any user who uploads a mail-merge template with a tag that fails docxtemplater's
-scope parser causes every printable household's name and mailing address for
-that batch to be written to application/server logs (e.g. Vercel function
-logs), which are typically retained, more widely readable, and less access
-controlled than the MP database itself.
-
----
-
-## Resolution (2026-09-13)
-Added a `describeError()` helper in `src/components/address-labels/actions.ts`
-and routed all three `console.error` calls through it —
-`generateLabelPdf`, `generateLabelDocx`, and `mergeTemplate`.
-
-It reduces a caught value to `{ name, message }`, plus docxtemplater's own
-`properties.id` and `properties.explanation` when present. Those two are
-genuinely useful for telling which failure mode occurred and contain no caller
-data. Everything else on `properties` — crucially `scope`, which in this
-feature IS the household list — is dropped. A non-`Error` throw is reported as
-`{ name: 'NonError', type: typeof error }` rather than by value, since a thrown
-string can itself be data-derived.
-
-The user-facing return value still carries `error.message`. That is the
-diagnostic the person fixing their template needs, and docxtemplater's messages
-reference tag names from the uploaded template, not merge data.
-
-### Tests
-`src/components/address-labels/actions.test.ts` — 5 cases driving a
-docxtemplater-shaped error whose `properties.scope` holds a real-looking name
-and address, asserting none of it reaches the log while the identifiers do.
-One asserts on the logged object's KEYS rather than substrings: the word
-"scope" legitimately appears inside the safe identifier
-`scopeparser_execution_failed`, so a naive `not.toContain('scope')` fails for
-the wrong reason.
diff --git a/.claude/TODO/2026-09-13-page-logs-raw-error-object.md b/.claude/TODO/2026-09-13-page-logs-raw-error-object.md
deleted file mode 100644
index 77fac66..0000000
--- a/.claude/TODO/2026-09-13-page-logs-raw-error-object.md
+++ /dev/null
@@ -1,88 +0,0 @@
----
-title: "AddEditFamilyPage logs the raw error object instead of an identifier"
-severity: medium
-tags: [security, drift]
-area: components
-files: [src/app/(web)/tools/addeditfamily/page.tsx]
-discovered: 2026-09-13
-discovered_by: coverage-agent-addeditfamily
-status: resolved
----
-
-## Problem
-`AddEditFamilyPage` swallows a failed `resolveContactIdFromPage` call with:
-
-```ts
-console.warn("Failed to resolve Contact_ID from page record:", error);
-```
-
-CLAUDE.md rule 14 requires errors to log identifiers and shape only — never
-record content, `$filter` strings, or request/response bodies, "including
-inside thrown error messages, which travel further than logs do." Logging the
-raw `error` object prints its `.message` (and, depending on the runtime,
-`.stack`), which can carry the interpolated MP `$filter` string built in
-`FamilyService.resolveContactIdFromPage` (`` `${primaryKey} = ${recordId}` ``)
-or other request detail if the underlying `getTableRecords` call or
-`validateColumnName`/`validatePositiveInt` throws with that detail embedded.
-
-The sibling file `src/lib/tool-params.server.ts` (same call-swallowing shape,
-literally a few lines away in the import graph) already gets this right:
-
-```ts
-// Identifier only. A caller without an MP security role also lands
-// here — the tools layout redirects them to /no-access, and the page
-// simply renders without page metadata in the meantime.
-console.warn('tool_params.page_data_unavailable', { pageID: parsedPageID });
-```
-
-`page.tsx` should follow the same pattern instead of passing `error` through.
-
-## Evidence
-- `src/app/(web)/tools/addeditfamily/page.tsx:29` — `console.warn("Failed to resolve Contact_ID from page record:", error)`
-- `src/lib/tool-params.server.ts:58-60` — the correct pattern, logging an identifier object with no error content
-- `src/services/familyService.ts:140-165` — `resolveContactIdFromPage` builds a `$filter` string from `primaryKey`/`recordId` that could surface in a thrown error's `.message`
-
-## Proposed fix
-Replace the `console.warn` call with an identifier-only log, e.g.:
-
-```ts
-console.warn("addeditfamily.resolve_contact_id_failed", {
- tableName: params.pageData.Table_Name,
- recordID: params.recordID,
-});
-```
-
-and drop `error` from the log call entirely (or log only `error instanceof Error ? error.name : typeof error` if a coarse classification is wanted).
-
-## Impact if not fixed
-Low likelihood, low blast radius — this only logs when the page-record
-resolution has already failed, and only to server logs. But it is the second
-place after `tool-params.server.ts`'s example that this exact swallow-and-warn
-shape appears, and it is the one that gets it wrong; left uncorrected, it is
-an easy pattern to copy into the next new page.
-
----
-
-## Resolution (2026-09-13)
-Fixed alongside the identical anti-pattern in
-`src/components/address-labels/actions.ts` — shipping a redaction fix for one
-file while leaving the same bug in the feature next door would have left the
-codebase with two standards.
-
-```ts
-console.warn("addeditfamily.resolve_contact_id_failed", {
- table: params.pageData.Table_Name,
- name: error instanceof Error ? error.name : "NonError",
-});
-```
-
-The table name is a configuration identifier, not record content, and rule 14
-explicitly allows "table, IDs, HTTP status". The error's `message` is dropped
-because `resolveContactIdFromPage` builds an MP `$filter` from the record id
-and column path, and MP surfaces that filter back inside its error text.
-
-### Tests
-`src/app/(web)/tools/addeditfamily/page.test.tsx` — the existing swallow test
-now asserts the redacted shape, plus two new cases: a rejection whose message
-contains a filter string never reaches the log, and a non-`Error` throw is
-described as `NonError` rather than by value.
diff --git a/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md b/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md
deleted file mode 100644
index 94a32e7..0000000
--- a/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md
+++ /dev/null
@@ -1,129 +0,0 @@
----
-title: removeGroup drops a non-empty group's fields from the save payload
-severity: high
-tags: [bug]
-area: components
-files: [src/components/field-management/use-field-order-state.ts]
-discovered: 2026-09-13
-discovered_by: coverage-agent-field-management
-status: resolved
----
-
-## Problem
-
-`removeGroup(name)` in `use-field-order-state.ts` guards `groupedFields` against
-removing a group that still has fields, but does not apply the same guard to
-`groupOrder`. The two state updates run independently:
-
-```ts
-const removeGroup = useCallback((name: string) => {
- setGroupedFields((prev) => {
- if ((prev[name] || []).length > 0) return prev; // <-- guarded: no-op if non-empty
- const { [name]: _, ...rest } = prev;
- return rest;
- });
- setGroupOrder((prev) => prev.filter((g) => g !== name)); // <-- unconditional
- setIsDirty(true);
-}, []);
-```
-
-If a caller invokes `removeGroup` on a group that still has fields (the UI's
-trash-can button is only rendered for empty groups, but `removeGroup` is a
-plain callback with no such enforcement — any other call site, a future UI
-change, or a test/automation calling the hook directly can hit this), the
-group's fields survive in `groupedFields` but the group name is removed from
-`groupOrder`.
-
-`buildSavePayload()` iterates `groupOrder` (not `Object.keys(groupedFields)`)
-to build the save payload:
-
-```ts
-for (const groupName of groupOrder) {
- const fieldIds = groupedFields[groupName] || [];
- ...
-}
-```
-
-Because the group name is no longer in `groupOrder`, its fields are silently
-skipped — they never appear in the payload sent to
-`savePageFieldOrder`/`api_Tools_...` stored proc. Since this feature writes
-live Ministry Platform page field configuration, this is a silent
-configuration-loss bug: fields simply vanish from the page's field list on
-next save, with no error or warning to the user.
-
-## Evidence
-
-- `src/components/field-management/use-field-order-state.ts:123-130` (removeGroup)
-- `src/components/field-management/use-field-order-state.ts:222-223` (buildSavePayload
- iterates `groupOrder`, using `groupedFields[groupName] || []` as a fallback for
- names no longer present)
-- Reproduced in
- `src/components/field-management/use-field-order-state.test.ts`, describe
- block `useFieldOrderState > removeGroup`, test `'is a no-op on groupedFields
- when the group still has fields, but still removes it from groupOrder'`:
- after calling `removeGroup('1 - First')` on a group with one field, the test
- asserts `groupedFields['1 - First']` still equals `[1]` but
- `buildSavePayload()`'s output has no entry for that field at all.
-
-## Proposed fix
-
-Make the two updates consistent — either:
-1. Only remove from `groupOrder` when the group was actually empty (mirror the
- same guard used for `groupedFields`), or
-2. If `removeGroup` is meant to force-delete a non-empty group (moving its
- fields elsewhere, e.g. into "99 - Other Fields"), do that explicitly instead
- of silently orphaning the fields.
-
-Given the existing UI only calls `removeGroup` for empty groups (see
-`sortable-group.tsx`'s trash button, rendered only when `fieldIds.length ===
-0`), option 1 (guard `groupOrder` the same way) is the minimal, safe fix and
-matches the function's only current call site's expectations.
-
-## Impact if not fixed
-
-Any caller of `removeGroup` on a non-empty group — now or in a future UI
-change — silently drops that group's fields from the next save, with no error
-surfaced to the admin performing the edit. This directly corrupts live
-Ministry Platform page field configuration (view order, group assignment,
-required/hidden/filter settings all lost for the affected fields) until
-someone notices fields missing from the page and manually re-adds them.
-
----
-
-## Resolution (2026-09-13)
-Applied option 1 from the proposed fix: `groupOrder` is now guarded the same
-way `groupedFields` was, so a non-empty group is a complete no-op.
-
-The guard moved OUT of the `setGroupedFields` updater and up to the top of the
-callback, reading the rendered `groupedFields` (added to the dependency array,
-matching how `addGroup` already reads `isFlat`):
-
-```ts
-const removeGroup = useCallback(
- (name: string) => {
- if ((groupedFields[name] || []).length > 0) return;
- setGroupedFields((prev) => { const { [name]: _, ...rest } = prev; return rest; });
- setGroupOrder((prev) => prev.filter((g) => g !== name));
- setIsDirty(true);
- },
- [groupedFields],
-);
-```
-
-### A wrong first attempt, recorded so it is not retried
-The obvious fix — set a `let removed = false` flag inside the
-`setGroupedFields` updater and check it before calling `setGroupOrder` — does
-not work. React runs state updaters during the render phase, not synchronously
-at call time, so the flag is still `false` when the second setter is reached.
-It would have looked correct, passed a casual read, and silently kept the bug.
-The decision has to be made once, up front, from state both setters agree on.
-
-`isDirty` is no longer set when the removal is refused: nothing changed, so
-there is nothing to save.
-
-### Tests
-`src/components/field-management/use-field-order-state.test.ts` — the test that
-documented the broken behaviour was replaced with four that pin the fix: both
-halves of state left intact, fields still present in `buildSavePayload()`,
-`isDirty` untouched on refusal, and a group still removable once emptied (with
-its field surviving).
diff --git a/.claude/TODO/2026-09-13-search-empty-state-never-renders.md b/.claude/TODO/2026-09-13-search-empty-state-never-renders.md
deleted file mode 100644
index 6b6e3d7..0000000
--- a/.claude/TODO/2026-09-13-search-empty-state-never-renders.md
+++ /dev/null
@@ -1,51 +0,0 @@
----
-title: "Add/Edit Family search: 'No contacts found' empty state never renders"
-severity: low
-tags: [bug]
-area: components
-files: [src/app/(web)/tools/addeditfamily/add-edit-family.tsx]
-discovered: 2026-09-13
-discovered_by: coverage-agent-addeditfamily
-status: open
----
-
-## Problem
-`FamilySearchBar` (in `add-edit-family.tsx`) renders a `No contacts
-found.` whenever the query is at least 2 characters and
-`searchContacts` returned zero results. In practice this text never appears in
-the DOM, because `cmdk`'s `CommandEmpty` only renders when the command's
-*global* registered-item count (`filtered.count`, tracked across the whole
-`Command` tree, not just the group it lives in) is zero. The sibling "Or"
-`CommandGroup` always registers a `+ New Family with last name "…"` item as
-soon as the query is >= 2 characters, so that count is never zero, and
-`CommandEmpty` unconditionally returns `null` regardless of our own
-`results.length === 0` check.
-
-Net effect: a user who searches for a name with no matches sees only the
-"+ New Family" suggestion, with no indication that the search itself came up
-empty (they may not notice there's a difference between "no matches" and
-"you haven't typed enough").
-
-## Evidence
-- `src/app/(web)/tools/addeditfamily/add-edit-family.tsx:457-459` — the
- `CommandEmpty` block, gated on local `results`/`query` state.
-- `node_modules/cmdk/dist/index.mjs` — `CommandEmpty` (`Ie`) renders based on
- `useCommandState(v => v.filtered.count === 0)`, and `filtered.count` is
- computed from `shouldFilter === false` as `u.current.size` (total item
- registrations), not scoped to a single `CommandGroup`.
-- `src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx` — the test
- `"finds no CommandEmpty text when the search returns nothing"` documents and
- asserts this actual (broken) behavior instead of the intended message.
-
-## Proposed fix
-Don't rely on `CommandEmpty`'s built-in visibility heuristic here. Replace it
-with an explicit conditional `
` (matching the style already used for the
-"Type at least 2 characters" and "Searching…" states a few lines above) that
-renders whenever `!isSearching && query.trim().length >= 2 && results.length
-=== 0`, instead of wrapping the message in `
`.
-
-## Impact if not fixed
-Cosmetic/UX only — the "+ New Family" path still works, so no data is lost or
-corrupted. A user searching for an existing family that doesn't actually exist
-gets a slightly less clear affordance (no explicit "not found" feedback), but
-can still proceed by creating a new family.
diff --git a/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md b/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md
deleted file mode 100644
index 6e721d0..0000000
--- a/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md
+++ /dev/null
@@ -1,64 +0,0 @@
----
-title: Coverage config omitted `coverage.include`, hiding every untested file from the report
-severity: high
-tags: [bug, testing, drift]
-area: testing
-files: [vitest.config.mts, .github/workflows/test.yml]
-discovered: 2026-09-13
-discovered_by: coverage-review-orchestrator
-status: resolved
----
-
-## Problem
-`vitest.config.mts` (then `vitest.config.ts`) configured v8 coverage without a `coverage.include` glob.
-The v8 provider only instruments files that were actually loaded during the
-test run, so any source file that no test imported was absent from the report
-entirely rather than counted as 0%.
-
-Note for anyone fixing this from memory or from an older guide: the flag that
-used to do this was `coverage.all: true`, and it was **removed in Vitest 5**.
-Setting it now is a TypeScript error (`No overload matches this call`) and has
-no runtime effect — `coverage.include` is the only lever. This was verified
-in-session: adding `all: true` alongside `include` changed nothing but the type
-check, and removing it changed no coverage number.
-
-The reported figure was therefore a percentage of *the code tests already
-touch*, not of the codebase. It read 83.14% statements while true coverage over
-`src/**` was 48.41%. 131 of 176 source files never appeared in the report at
-all, including `src/services/familyService.ts` (756 lines, zero tests) and
-`src/app/(web)/tools/layout.tsx` (the `/tools` authorization gate, zero tests).
-
-CI uploads this report to Codecov, so the inflated number was the project's
-published quality signal.
-
-## Evidence
-- `vitest.config.ts:15-24` (pre-fix, before the rename to `.mts`) — `coverage` block had `provider`,
- `reporter`, `exclude`, but no `all` and no `include`.
-- `npx vitest run --coverage` before fix: `Statements : 83.14% ( 1845/2219 )`
-- Same suite with `include: ['src/**/*.{ts,tsx}']`:
- `Statements : 48.41% ( 1845/3811 )` — identical numerator, denominator up 72%.
-- Files absent from the pre-fix report despite existing: `familyService.ts`,
- `googlePlacesService.ts`, all of `src/components/template-editor/`, all of
- `src/app/(web)/tools/*/page.tsx`.
-
-## Proposed fix
-Applied. `vitest.config.mts` now sets `include: ['src/**/*.{ts,tsx}']`,
-plus explicit exclusions for vendored
-shadcn/ui primitives, generated MP models, build-time scripts, barrel files,
-type-only modules, and declarative `loading.tsx` shells.
-
-A second, related defect was found while applying this: directory exclusions
-were written with a bare trailing slash (`'src/components/ui/'`), which matches
-nothing. Those files stayed in the denominator until the patterns were
-rewritten as `'src/components/ui/**'`. Any directory exclusion added here must
-end in `**`.
-
-Codecov will show a one-time drop from 83% to ~49% on the first push carrying
-this change — before the new tests land on top of it. That is the correction,
-not a regression.
-
-## Impact if not fixed
-Every coverage claim in `CLAUDE.md`, `.claude/references/testing/`, and Codecov
-was overstating the tested share of the codebase by ~35 points. Untested
-files stayed invisible, so no one could see that a 756-line service and a
-route authorization gate had no tests at all.
diff --git a/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md b/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md
deleted file mode 100644
index 2ea39f5..0000000
--- a/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md
+++ /dev/null
@@ -1,119 +0,0 @@
----
-title: Add a type-check gate to CI (stale test fixtures had silently broken `npm run build`)
-severity: medium
-tags: [bug, testing, drift]
-area: testing
-files: [.github/workflows/test.yml, package.json]
-discovered: 2026-09-13
-discovered_by: coverage-review-orchestrator
-status: resolved
----
-
-## Problem
-Two test files that are committed and unmodified on `dev` fail type checking.
-`tsconfig.json` includes `**/*.ts` and `**/*.tsx`, `next.config.ts` sets no
-`typescript.ignoreBuildErrors`, and `npm run build` runs `next build` — which
-type-checks the project. So `npm run build` fails on a clean `dev` checkout.
-
-This is invisible because, as `CLAUDE.md` already notes, CI runs only
-`npm run test:coverage` and never `npm run build`. The tests themselves pass:
-the arguments are wrong *types* but the mocked implementations never inspect
-the missing fields, so the assertions still hold. Type safety is the only thing
-catching it, and nothing automated runs type safety.
-
-The underlying defect is that these tests assert against object shapes the
-production types do not accept — `helper.test.ts` builds `CommunicationInfo`
-and `MessageInfo` values out of raw MP column names (`Author_User_ID`,
-`From_Contact`, `To_Contact_List`) instead of the camelCase DTO fields the
-helper actually requires (`AuthorUserId`, `FromContactId`, `ReplyToContactId`,
-`CommunicationType`, ...). A test that type-checks would have caught the drift;
-these ones silently encode the wrong contract.
-
-## Evidence
-```
-$ git status --porcelain src/lib/providers/ministry-platform/helper.test.ts \
- src/components/address-labels/word-document.test.ts
-(no output — both unmodified)
-
-$ npx tsc --noEmit
-src/components/address-labels/word-document.test.ts(62,46): error TS1355:
- A 'const' assertion can only be applied to references to enum members, or
- string, number, boolean, array, or object literals.
-src/lib/providers/ministry-platform/helper.test.ts(762,57): error TS2345:
- ... is missing the following properties from type 'CommunicationInfo':
- AuthorUserId, FromContactId, ReplyToContactId, CommunicationType, and 4 more.
-src/lib/providers/ministry-platform/helper.test.ts(782,57): error TS2345: (same)
-src/lib/providers/ministry-platform/helper.test.ts(822,49): error TS2345:
- ... is missing the following properties from type 'MessageInfo':
- FromAddress, ToAddresses
-src/lib/providers/ministry-platform/helper.test.ts(841,49): error TS2345: (same)
-src/lib/providers/ministry-platform/helper.test.ts(923,11): error TS2559:
- Type '{ IsDefault: boolean; Description: string; }' has no properties in
- common with type 'FileUploadParams'.
-```
-- `tsconfig.json` — `"include": ["**/*.ts", "**/*.tsx", ...]`, so tests are in
- the program.
-- `next.config.ts` — no `typescript` block, so `ignoreBuildErrors` is false.
-- `.github/workflows/test.yml` — runs `npm run test:coverage` only.
-
-## Fixed in this branch
-The stale fixtures have been corrected. `helper.test.ts` and `provider.test.ts`
-now build real `CommunicationInfo` / `MessageInfo` / `FileUploadParams` /
-`FileUpdateParams` values with explicit type annotations, so the compiler
-enforces the shape, and the `as const` misuse at `word-document.test.ts:62` is
-gone. `npx tsc --noEmit` is clean and all 96 tests in those files still pass.
-
-## Remaining work
-**Add a type-check gate to CI.** Add a `typecheck` script (`tsc --noEmit`) to
-`package.json` and run it in `.github/workflows/test.yml` alongside
-`test:coverage`.
-
-`CLAUDE.md` documents "CI gates tests only ... Type-check locally" as the
-mitigation. This issue is the evidence that the honour system did not hold: two
-committed test files sat on `dev` in a state that broke `npm run build`, and
-nothing caught it because the tests themselves passed — the arguments were the
-wrong *types*, but the mocks never inspected the missing fields.
-
-Note that the new `coverage.thresholds` in `vitest.config.mts` are enforced by
-the existing `test:coverage` CI job, so coverage regressions now fail the build.
-Type regressions still do not.
-
-## Impact if not fixed
-The specific breakage is repaired, but the gap that allowed it is still open. A
-type error reaching `dev` breaks `npm run build` and therefore a Vercel
-production deploy, while CI stays green — so it is found at deploy time by
-whoever is shipping, not at PR time by whoever wrote it.
-
----
-
-## Resolution (2026-09-13)
-Added `"typecheck": "tsc --noEmit"` to `package.json` and wired it into
-`.github/workflows/test.yml`, along with two other gaps found while in there.
-
-**Added as STEPS, not new jobs.** Branch protection on `dev` and `main`
-requires the status check named `test`. Lint and typecheck as separate jobs
-would have been green-but-unrequired until someone also edited the protection
-rules — a gate that does not gate. As steps in the existing `test` job they are
-covered by the rule already in place, with no admin change needed.
-
-Order is install -> lint -> typecheck -> test:coverage, so the cheap checks
-fail first. The Codecov step keeps `if: always()` and `fail_ci_if_error: false`,
-so an early failure that produces no coverage file does not itself fail the
-build.
-
-### Two other CI issues fixed in the same pass
-- **`npm install` -> `npm ci`.** `npm install` resolves fresh versions and
- rewrites `package-lock.json` inside CI, so CI could be testing a different
- dependency tree than any developer had. `npm ci` installs exactly what the
- lockfile pins and fails outright if `package.json` and the lockfile disagree.
- Verified with `npm ci --dry-run` before committing.
-- **No `concurrency` group.** Rapid pushes to a PR branch left superseded runs
- burning minutes. Now cancels in-progress runs for the same ref, except on
- `main` and `dev`, whose runs gate merges and releases and must not be killed.
-
-Also added `permissions: contents: read` — this workflow only reads the repo,
-and Codecov authenticates with its own token rather than `GITHUB_TOKEN`.
-
-`CLAUDE.md` previously documented "CI gates tests only ... Type-check locally"
-as the mitigation. That line should now be read alongside this change: the
-honour system has been replaced by a gate.
diff --git a/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md b/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md
deleted file mode 100644
index d2b2562..0000000
--- a/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md
+++ /dev/null
@@ -1,77 +0,0 @@
----
-title: `vitest.config.ts` was loaded as CommonJS, warned as unsupported by Vite's future default loader
-severity: low
-tags: [drift, testing]
-area: testing
-files: [vitest.config.mts]
-discovered: 2026-09-13
-discovered_by: coverage-review-orchestrator
-status: resolved
----
-
-## Problem
-Every Vitest invocation prints:
-
-```
-(!) Your Vite config uses features that are unsupported by `configLoader: 'native'`,
-which is planned to become the default in a future major version of Vite:
- - ESM syntax in a file loaded as CommonJS (vitest.config.ts:1:1).
- Use a `.mjs` extension or set `"type": "module"` in the closest package.json
-```
-
-`vitest.config.ts` uses ESM syntax (`import`/`export default`) but `package.json`
-has no `"type": "module"`, so Vite loads it through the CommonJS path. That path
-still works today, but Vite plans to make `configLoader: 'native'` the default,
-at which point the config stops loading and the whole suite fails to start.
-
-This is noise on every local run and every CI run today, and a hard break on a
-future Vite major.
-
-## Evidence
-- `vitest.config.ts:1` — `import { defineConfig } from 'vitest/config';`
-- `package.json` — no `"type"` field.
-- Warning reproduces on any `npx vitest run`.
-- Vite 8 / Vitest 6 are the likely forcing versions; neither is adopted here yet.
-
-## Proposed fix
-Lowest-risk option is to rename `vitest.config.ts` -> `vitest.config.mts`. Vitest
-resolves the `.mts` extension automatically, so no script changes are needed, and
-it does not touch how Next.js or the rest of the toolchain treat the package.
-
-Do NOT add `"type": "module"` to `package.json` as the fix — this project also
-contains CommonJS tooling and Next.js build config, and flipping the package type
-has a much wider blast radius than renaming one file.
-
-Verify with `npx vitest run` (warning gone, suite still runs) and
-`npm run test:coverage` (Codecov path `coverage/coverage-final.json` still
-produced).
-
-## Impact if not fixed
-Cosmetic today. On the Vite major that flips `configLoader` to `'native'`, the
-config fails to load and the entire test suite — and therefore the CI `test`
-gate that protects `dev` and `main` — stops running. Cheap to fix now, and the
-dependency-audit process should not have to rediscover it under time pressure.
-
----
-
-## Resolution (2026-09-13)
-Applied the proposed fix: renamed `vitest.config.ts` -> `vitest.config.mts`.
-Vitest resolves the `.mts` extension automatically, so no script or CI changes
-were needed, and `package.json` was left alone (adding `"type": "module"` would
-have had a far wider blast radius, as noted above).
-
-The rename surfaced a second, hidden instance of the same problem. With the
-file loaded as real ESM, Vite immediately flagged:
-
-```
-- `__dirname` (vitest.config.mts:66:25). Use `import.meta.dirname` instead
-```
-
-`__dirname` is a CommonJS global and does not exist in ESM; it had been working
-only because the file was being loaded as CJS. The `@` path alias is now
-resolved with `import.meta.dirname`. Had the rename been done without running
-the suite, the alias would have thrown at config load and every test would have
-failed to start.
-
-Verified: `npm run test:run` produces 17 lines of output with zero warnings,
-116 files / 1,535 tests passing, and `npx tsc --noEmit` is clean.
diff --git a/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md b/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md
deleted file mode 100644
index ae949b0..0000000
--- a/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md
+++ /dev/null
@@ -1,122 +0,0 @@
----
-title: Unvalidated client-supplied numeric fields interpolated into MP $filter strings in FamilyService
-severity: high
-tags: [security, bug]
-area: services
-files: [src/services/familyService.ts, src/app/(web)/tools/addeditfamily/actions.ts, src/lib/dto/family.ts]
-discovered: 2026-09-13
-discovered_by: coverage-agent-services
-status: resolved
----
-
-## Problem
-`saveFamily` (the `"use server"` action in `addeditfamily/actions.ts`) accepts a
-`Household` object straight from the client and passes it directly to
-`FamilyService.saveHousehold()` — it is never parsed through the
-`HouseholdSchema`/`FamilyMemberSchema` Zod schemas that already exist in
-`src/lib/dto/family.ts` for exactly this purpose. TypeScript's compile-time
-`number` type on `FamilyMember.envelopeNo` and `FamilyMember.donorId` is not
-enforced at runtime — a server action is a public POST endpoint, and any JSON
-body can be sent to it, including e.g. `envelopeNo: "1 OR 1=1"`.
-
-Inside `FamilyService.saveHousehold` → `upsertDonor` → `resolveUniqueEnvelopeNo`,
-those two client-controlled values are interpolated directly into an MP
-`$filter` string with no `validatePositiveInt` (or any other) check:
-
-```ts
-private async resolveUniqueEnvelopeNo(
- requested: number,
- excludeDonorId: number | null,
-): Promise<{ envelopeNo: number; bumped: boolean }> {
- let candidate = requested;
- ...
- const filter =
- excludeDonorId && excludeDonorId > 0
- ? `Envelope_No = ${candidate} AND Donor_ID <> ${excludeDonorId}`
- : `Envelope_No = ${candidate}`;
-```
-
-Every other filter built in this file first validates its numeric inputs
-(`validatePositiveInt(contactId)` in `getHousehold`, `validatePositiveInt(recordId)`
-in `resolveContactIdFromPage`), so this is an inconsistency, not the intended
-design. This is the same shape of hole CLAUDE.md rule 11 calls out for strings
-("escape user input in filters") — here the payload is untyped/unvalidated
-numeric input instead of a string, but it reaches a raw `$filter` string the
-same way.
-
-## Evidence
-- `src/services/familyService.ts:687-712` (`resolveUniqueEnvelopeNo`) — `candidate`
- (from `member.envelopeNo`) and `excludeDonorId` (from `member.donorId`) are
- interpolated into the filter with no type/range check.
-- `src/app/(web)/tools/addeditfamily/actions.ts:137-154` (`saveFamily`) — takes
- `household: Household` straight from the client with no `HouseholdSchema.parse()`
- or `.safeParse()` call before it reaches the service.
-- Contrast with `src/services/familyService.ts:150,175` where the same file
- validates `recordId`/`contactId` via `validatePositiveInt` before use in a
- filter.
-- Confirmed via test: `src/services/familyService.test.ts` exercises
- `resolveUniqueEnvelopeNo` only with well-formed numbers (current behavior is
- documented, not exploited, per this task's instructions not to modify source).
-
-## Proposed fix
-1. In `saveFamily` (`actions.ts`), parse the incoming `household` argument with
- `HouseholdSchema.safeParse()` before calling `FamilyService.saveHousehold()`,
- returning an `ActionError` on failure — mirroring the "Validate at API
- boundaries" guidance in CLAUDE.md.
-2. Defense in depth: in `resolveUniqueEnvelopeNo` and `upsertDonor`, call
- `validatePositiveInt` (or an equivalent nullable-safe check) on `requested`/
- `envelopeNo` and `excludeDonorId`/`existingDonorId` before they are
- interpolated into any `$filter` string.
-
-## Impact if not fixed
-An authenticated user with write access to `Households` (the only gate this
-action checks) can submit a crafted `envelopeNo` or `donorId` value that is
-not a plain positive integer. Depending on how the MP OData-style `$filter`
-parser handles the resulting string, this ranges from a confusing 400/500
-error to a filter-injection primitive against the `Donors` table — the same
-class of risk rule 11 exists to close off for string fields.
-
----
-
-## Resolution (2026-09-13)
-Both layers of the proposed fix were applied.
-
-**1. Parse at the action boundary.** `saveFamily` now runs
-`HouseholdSchema.safeParse(household)` before the payload reaches
-`FamilyService`, and returns an `ActionError` on failure. The error reports the
-offending field PATHS only (`members.0.envelopeNo`), never the submitted
-values — rule 14 applies to returned error strings, which travel further than
-logs. Authorization still runs first, so an unauthorized caller learns nothing
-about the schema.
-
-**2. Validate where the filter is built.** `resolveUniqueEnvelopeNo` calls
-`validatePositiveInt` on `requested` and on `excludeDonorId`, and re-validates
-`candidate` on every loop iteration so a future change to
-`getNextEnvelopeNumber()` cannot reintroduce an unchecked value. `upsertDonor`
-validates `existingDonorId` before using it to target the `Donors` update.
-`null` and `0` are preserved as the "no donor to exclude" sentinels rather than
-being rejected.
-
-### Correction to the original severity assessment
-Re-reading this while fixing it: the classic injection string was already
-blocked, but incidentally rather than by design. `upsertDonor` guards with
-`envelopeNo > 0`, and JS coerces `"1 OR 1=1"` to `NaN`, making that comparison
-false — so the crafted string never reached the filter. What *did* get through
-were values that survive numeric coercion but are not positive integers:
-`1.5`, `Infinity`, `1e21` (which interpolates as the malformed `1e+21`), and
-numeric strings. Those produce malformed filters and confusing 400/500s, not a
-filter-injection primitive.
-
-So the practical severity was lower than "high" as originally filed. The fix is
-still correct and worth keeping: relying on an incidental coercion side effect
-to block injection is fragile, and a schema change or a new call path that
-drops the `> 0` guard would turn it into the real thing with nothing to catch
-it. Recorded here so the next reader is not misled by the original framing.
-
-### Tests
-- `src/app/(web)/tools/addeditfamily/actions.test.ts` — 11 cases: injection-shaped
- `envelopeNo`/`donorId`, non-integer/array/object/boolean fields, missing
- top-level fields, path reporting, no value echoed back, authorize-before-validate.
-- `src/services/familyService.test.ts` — 7 cases asserting no malformed value
- ever reaches an MP query, that `donorId: 0` is still accepted as "none", and
- that well-formed input produces the exact expected filter string.
diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md
index e6b6dc6..3ab3e42 100644
--- a/.claude/TODO/INDEX.md
+++ b/.claude/TODO/INDEX.md
@@ -1,20 +1,16 @@
---
title: TODO Index
type: index
-last_updated: 2026-09-13
+last_updated: 2026-10-10
---
-
-
-
-
-
-
-
-
# TODO Index
-Open TODOs from the context-engineering review (2026-04-17) and later additions.
+This folder lists **open work only**. When a TODO is finished, or we decide not
+to do it, its file is **deleted**, not marked resolved. The commit that removes
+it is the record (`git log --diff-filter=D -- .claude/TODO/`). See
+[`SCHEMA.md`](SCHEMA.md) § Lifecycle.
+
Severity tiers:
- **critical**: security hole, data loss, auth bypass
@@ -22,19 +18,7 @@ Severity tiers:
- **medium**: doc drift, missing test, refactor with real cost
- **low**: nits, minor doc fixes, stylistic improvements
-Total: **4 open TODOs**.
-
-> **2026-09-13 — unit-test coverage push.** Statement coverage over authored
-> code went from 49.67% to 98.84% (3,610/3,652), lines to 99.70%, across 1,535
-> tests in 116 files. Most of the items below were opened during that work,
-> found by reading code while writing tests for it. A follow-up pass cleaned
-> the runner output from 1,507 lines to 17 with zero warnings.
->
-> **2026-09-13 (later) — remediation.** All three high-severity items and the
-> CI gap are fixed and closed; see each file's Resolution section. CI now runs
-> lint and `tsc --noEmit` as required steps, and installs with `npm ci`.
-> Everything still open is `medium` or below, and all of it sits in
-> `src/components`.
+Total: **0 open TODOs**.
---
@@ -46,17 +30,11 @@ _none open_
### High (0)
_none open_
-### Medium (2)
-| Area | Tags | Title | File |
-|---|---|---|---|
-| components | bug, drift | Template editor ignores pageID/recordID (no MP persistence) | [→](2026-04-17-components-template-editor-no-mp-persistence.md) |
-| components | bug, refactor | Merge tokens `{{Field_Name}}` have no resolver anywhere | [→](2026-04-17-components-template-editor-merge-token-resolver.md) |
+### Medium (0)
+_none open_
-### Low (2)
-| Area | Tags | Title | File |
-|---|---|---|---|
-| components | bug | Add/Edit Family search: "No contacts found" empty state never renders | [→](2026-09-13-search-empty-state-never-renders.md) |
-| components | refactor, missing-test | Unreachable "empty STEP_FIELDS" branch in `GroupWizard.handleNext` | [→](2026-09-13-dead-empty-fields-branch-handlenext.md) |
+### Low (0)
+_none open_
---
@@ -65,22 +43,16 @@ _none open_
### security (0)
_none open_
-### bug (3)
-- components-template-editor-no-mp-persistence — medium
-- components-template-editor-merge-token-resolver — medium
-- search-empty-state-never-renders — low
-
-### drift (1)
-- components-template-editor-no-mp-persistence — medium
+### bug (0)
+_none open_
-### missing-test (1)
-- dead-empty-fields-branch-handlenext — low
+### drift (0)
+_none open_
-### refactor (2)
-- components-template-editor-merge-token-resolver — medium
-- dead-empty-fields-branch-handlenext — low
+### missing-test (0)
+_none open_
-### testing (0)
+### refactor (0)
_none open_
### doc (0)
@@ -95,33 +67,19 @@ _none open_
| Area | Count |
|---|---|
-| components | 4 |
-| testing | 0 |
+| components | 0 |
+| routing | 0 |
| services | 0 |
| auth | 0 |
+| commands | 0 |
| mp-provider | 0 |
+| testing | 0 |
| utils | 0 |
-| routing | 0 |
| mp-schema | 0 |
| contexts | 0 |
| dto-constants | 0 |
-| doc (cross-cutting) | 0 |
-
----
-
-## Recently resolved
-
-| Date | Title | File |
-|---|---|---|
-| 2026-09-13 | Coverage config omitted `coverage.include`, hiding every untested file | [→](2026-09-13-testing-coverage-report-masked-untested-files.md) |
-| 2026-09-13 | No tests for `src/components/template-editor/` | [→](2026-04-17-components-template-editor-missing-tests.md) |
-| 2026-09-13 | `vitest.config.ts` loaded as CommonJS (renamed to `.mts`) | [→](2026-09-13-testing-vitest-config-loaded-as-cjs.md) |
-| 2026-09-13 | Radix Select fields switched uncontrolled -> controlled | [→](2026-09-13-components-select-uncontrolled-to-controlled.md) |
-| 2026-09-13 | Unvalidated numeric fields reaching MP `$filter` strings | [→](2026-09-13-unvalidated-envelope-donor-ids-in-filter.md) |
-| 2026-09-13 | `removeGroup` dropped a non-empty group's fields from the save payload | [→](2026-09-13-removegroup-order-guard-mismatch.md) |
-| 2026-09-13 | Raw docxtemplater error logged household addresses | [→](2026-09-13-mergetemplate-logs-address-pii-on-error.md) |
-| 2026-09-13 | `AddEditFamilyPage` logged the raw error object | [→](2026-09-13-page-logs-raw-error-object.md) |
-| 2026-09-13 | No type-check gate in CI (also `npm ci`, lint, concurrency) | [→](2026-09-13-testing-no-typecheck-gate-in-ci.md) |
+| data-flow | 0 |
+| security | 0 |
---
diff --git a/.claude/TODO/SCHEMA.md b/.claude/TODO/SCHEMA.md
index b36c378..89df230 100644
--- a/.claude/TODO/SCHEMA.md
+++ b/.claude/TODO/SCHEMA.md
@@ -16,7 +16,7 @@ type: meta
title:
severity: critical | high | medium | low
tags: [security | bug | drift | missing-test | refactor | doc | perf]
-area: auth | mp-provider | services | components | contexts | routing | data-flow | testing | dto-constants | utils | commands
+area: auth | mp-provider | mp-schema | services | components | contexts | routing | data-flow | security | testing | dto-constants | utils | commands
files: [src/path/to/file.ts]
discovered: YYYY-MM-DD
discovered_by:
@@ -37,6 +37,23 @@ Concrete, actionable. Specific file:line edits or command to run.
Who is affected, how badly, when.
```
+## Lifecycle
+This folder holds **open work only**. `status` is always `open`.
+
+- **Done or won't do → delete the file** in the same PR that finishes the work
+ or records the decision. Never leave a file marked `resolved`, `closed`,
+ `won't fix` or `superseded`. The commit message is the record: say what was
+ done or why it was dropped. Find past items with
+ `git log --diff-filter=D -- .claude/TODO/`.
+- **Before deleting**, salvage what outlives the item:
+ - leftover follow-up work → a new TODO file;
+ - a lasting lesson or trap → `.claude/references/GOTCHAS.md`;
+ - a decision → `.claude/references/DECISIONS.md` (ADR).
+- **Remove every inbound link** to the deleted file (`INDEX.md`, references,
+ code comments, other TODOs). A link to a missing TODO is drift.
+- **Update `INDEX.md`** (counts, tables, tags, areas) in the same change.
+ `INDEX.md` has no "resolved" section.
+
## Severity
- **critical**: security hole, data loss risk, auth bypass
- **high**: broken behavior, test failure, convention violation that causes bugs
diff --git a/.claude/commands/newstoredproc.md b/.claude/commands/newstoredproc.md
index c675c8c..9c5fb8a 100644
--- a/.claude/commands/newstoredproc.md
+++ b/.claude/commands/newstoredproc.md
@@ -167,5 +167,5 @@ All INSERT sections use `IF NOT EXISTS` guards so they skip gracefully on re-run
- The unified install script at `_INSTALL/ministryplatform-install.sql` is auto-generated by the build — never edit it manually
- SQL files in `src/lib/providers/ministry-platform/db/` are combined alphabetically by the build script
- Domain_ID is hardcoded to `1` in role grants (matches existing pattern across all deployment scripts)
-- If the procedure needs to be callable from the app, a corresponding service method should be added — see `src/services/` and `.claude/references/services.md`
+- If the procedure needs to be callable from the app, a corresponding service method should be added (it must gate with `AuthorizationService.requireSecurityRole()` and must NOT pass `@DomainID` in the body) — see `src/services/`, `.claude/references/services/README.md`, and add the proc to `.claude/references/mp-schema/required-procs.md`
- **`@DomainID INT` is required by the Ministry Platform API for all `api_*` stored procedures.** The platform passes the current domain ID automatically when calling procedures through the API. If this parameter is missing, the procedure will not be callable via the API even if it is registered in `dp_API_Procedures`.
diff --git a/.claude/commands/newtool.md b/.claude/commands/newtool.md
index 3ce740d..0819537 100644
--- a/.claude/commands/newtool.md
+++ b/.claude/commands/newtool.md
@@ -28,13 +28,18 @@ Create `src/app/(web)/tools/{routeName}/page.tsx`:
```tsx
import { {componentName} } from "./{fileName}";
-import { parseToolParams } from "@/lib/tool-params";
+import { requireToolAccess } from "@/app/(web)/tools/require-tool-access";
+import { parseToolParams } from "@/lib/tool-params.server";
interface {componentName}PageProps {
searchParams: Promise<{ [key: string]: string | string[] | undefined }>;
}
export default async function {componentName}Page({ searchParams }: {componentName}PageProps) {
+ // Self-gating, before ANY param parsing or data call — see require-tool-access.ts.
+ // Pick the MP table/operation this tool actually needs.
+ await requireToolAccess({ table: "dp_Tools", operation: "read" });
+
const params = await parseToolParams(await searchParams);
return <{componentName} params={params} />;
@@ -54,7 +59,7 @@ Create `src/app/(web)/tools/{routeName}/{fileName}.tsx`:
```tsx
"use client";
-import { useState, useEffect } from "react";
+import { useState } from "react";
import { useRouter } from "next/navigation";
import { ToolContainer } from "@/components/tool";
import { Users } from "lucide-react";
@@ -69,19 +74,10 @@ export function {componentName}({ params }: {componentName}Props) {
const [isSaving, setIsSaving] = useState(false);
const isNew = isNewRecord(params);
- useEffect(() => {
- console.log("Tool launched with params:", params);
- console.log("Mode:", isNew ? "Create New" : "Edit Existing");
- if (params.recordDescription) {
- console.log("Editing record:", params.recordDescription);
- }
- }, [params, isNew]);
-
const handleSave = async () => {
setIsSaving(true);
await new Promise((resolve) => setTimeout(resolve, 1000));
setIsSaving(false);
- console.log("Saved!", { params });
};
const handleClose = () => {
@@ -163,6 +159,8 @@ After creating all files, confirm the new tool by listing the created files and
- [ ] Tool component created at `src/app/(web)/tools/{routeName}/{fileName}.tsx`
- [ ] Homepage card added to `src/app/(web)/page.tsx`
- [ ] All placeholder text replaced with the tool's displayName
+- [ ] `page.tsx` calls `requireToolAccess()` before parsing params, and imports `parseToolParams` from `@/lib/tool-params.server` (not `@/lib/tool-params`, which must stay client-safe)
+- [ ] No `console.log` (ESLint `no-console` allows only `warn`/`error`)
- [ ] Component uses named export (no default export) — except `page.tsx` which requires default export per Next.js
## Multiple Tools
@@ -178,5 +176,7 @@ Creates both tools and adds both cards to the homepage.
## Notes
- The `ToolContainer` renders a `DevPanel` above the title bar automatically on localhost in development builds — no manual scaffolding needed.
+- `tools/layout.tsx` only redirects role-less users for UX; it does **not** protect the page. Each page gates itself with `requireToolAccess` (`src/app/(web)/tools/require-tool-access.ts`), and any server action / service the tool adds must gate again with `AuthorizationService.requireSecurityRole()` (CLAUDE.md practice #12).
+- Add co-located tests (`page.test.tsx`, `{fileName}.test.tsx`) — CI enforces coverage thresholds (`vitest.config.mts`).
- The `page.tsx` uses a default export (Next.js requirement), but the tool component uses a named export per project convention
-- Route names should be lowercase with no separators (matching existing pattern: `template`, `groupwizard`, `templateeditor`)
+- Route names should be lowercase with no separators (matching existing pattern: `template`, `groupwizard`, `addresslabels`, `fieldmanagement`, `addeditfamily`)
diff --git a/.claude/commands/release.md b/.claude/commands/release.md
index 23726d1..54d794b 100644
--- a/.claude/commands/release.md
+++ b/.claude/commands/release.md
@@ -20,8 +20,8 @@ Create a GitHub release with auto-generated release notes from merged pull reque
- Run `git log --oneline` (from last release tag to HEAD, or recent commits if first release) to understand what's new
3. **Identify PRs to include:**
- - If this is the first release, run `gh pr list --state merged --limit 20 --json number,title,mergedAt,body,labels` to get recent merged PRs
- - If a previous release exists, find PRs merged since that release using `gh pr list --state merged --search "merged:>YYYY-MM-DD" --json number,title,mergedAt,body,labels`
+ - If this is the first release, run `gh pr list --base dev --state merged --limit 20 --json number,title,mergedAt,body,labels` to get recent merged PRs
+ - If a previous release exists, find PRs merged since that release using `gh pr list --base dev --state merged --search "merged:>YYYY-MM-DD" --json number,title,mergedAt,body,labels` (feature PRs target `dev`; add `--base main` hotfix PRs if any)
- Present the list of PRs to the user and ask which to include (default: all)
- Exclude the `dev` → `main` release PR itself — it is a promotion, not a change
@@ -105,7 +105,7 @@ gh release list --limit 5
git tag --sort=-version:refname | head -5
# 2. Get merged PRs since last release
-gh pr list --state merged --limit 20 --json number,title,mergedAt,body,labels
+gh pr list --base dev --state merged --limit 20 --json number,title,mergedAt,body,labels
# 3. Get repo info for changelog URL
gh repo view --json nameWithOwner --jq .nameWithOwner
diff --git a/.claude/commands/update-deps.md b/.claude/commands/update-deps.md
index dff63a9..a670a91 100644
--- a/.claude/commands/update-deps.md
+++ b/.claude/commands/update-deps.md
@@ -45,7 +45,8 @@ Record these numbers — they are the comparison for everything that follows:
```bash
npm run test:run # test + file counts
npm run lint
-npm run build 2>&1 | grep -cE "error TS" # may be non-zero already
+npm run typecheck 2>&1 | grep -cE "error TS" # may be non-zero already
+npm run build # needs a valid .env.local — the app refuses to boot (even at build) on missing/weak secrets
npm audit
node -v && npm -v
```
@@ -112,8 +113,8 @@ npm view @ peerDependencies engines --json
Then confirm the rest of the tree can satisfy those peers — especially any
wrapper package. A wrapper's peer range on its wrapped library is the usual
-blocker (`@grapesjs/react` → `grapesjs`, `eslint-config-next` →
-`eslint-plugin-react`). Check whether the wrapper's **latest** release supports
+blocker (`eslint-config-next` → `eslint-plugin-react`,
+`typescript-eslint` → `typescript`). Check whether the wrapper's **latest** release supports
the target; if it does not, the upgrade is blocked upstream and no override fixes
it — record it as a hold.
@@ -123,7 +124,8 @@ Then install that **one** package, and verify:
npm install @
npm run test:run
npm run lint
-npm run build 2>&1 | grep -cE "error TS" # compare to baseline
+npm run typecheck 2>&1 | grep -cE "error TS" # compare to baseline
+npm run build && npm run check:prerender
```
- Any new type error is a real break even if tests pass — find the migration
@@ -170,8 +172,7 @@ const all=[...seen.values()];
**Triage every hit before reporting it.** OSV ranges are sometimes malformed —
fetch `https://api.osv.dev/v1/vulns/` and read `affected[].ranges` directly.
A record with `{"introduced":"0"}` and no `fixed` event matches every version and
-is almost always a false positive. Known standing false positive: `grapesjs` /
-GHSA-589f-c66p-hxr4.
+is almost always a false positive.
For richer context on a specific library's breaking changes, `context7` is
available (`resolve-library-id` then `query-docs`).
@@ -208,7 +209,8 @@ adds no install scripts. Otherwise pin to the previous version and note why.
held with clearing conditions**, and any pre-existing issues found but not
fixed.
- Add a row to the index table in `.claude/packages/README.md`.
-- Add a row to the **Dependency Audit History** table in `CLAUDE.md`.
+- Add a row to the **Dependency Audit History** table in `CLAUDE.md`, and update
+ its **Current holds** table to match the new record.
- Commit in the same separable units used above (in-range / removals / each
major), then open a PR targeting `dev`.
diff --git a/.claude/packages/2026-09-30.md b/.claude/packages/2026-09-30.md
new file mode 100644
index 0000000..cba1588
--- /dev/null
+++ b/.claude/packages/2026-09-30.md
@@ -0,0 +1,145 @@
+# Dependency Audit — 2026-09-30
+
+**Branch:** `chore/dependency-audit-2026-09` · **Node:** 24.18.0 · **npm:** 11.16.0
+
+## Result
+
+| | Before | After |
+|---|---|---|
+| `npm audit` | 1 (1 high) | **0** |
+| Direct dependencies | 55 | 55 |
+| Tests | 1563 passing / 116 files | **1563 passing / 116 files** |
+| `eslint .` | clean | clean |
+| `tsc --noEmit` type errors | 0 | 0 |
+| `next build` | passes | passes |
+| OSV.dev sweep | — | 472 packages, 1 hit (known false positive) |
+
+---
+
+## 1. Security
+
+`npm update` alone cleared the one finding.
+
+**`brace-expansion`** (high) — three DoS advisories, all quadratic-time or
+unbounded recursion on crafted brace patterns:
+[GHSA-q2hr-2g5m-vwhr](https://github.com/advisories/GHSA-q2hr-2g5m-vwhr),
+[GHSA-qhr7-859c-m2p7](https://github.com/advisories/GHSA-qhr7-859c-m2p7),
+[GHSA-6j4f-fj2g-mc7p](https://github.com/advisories/GHSA-6j4f-fj2g-mc7p).
+
+| Resolved | Reached us via |
+|---|---|
+| 1.1.18 → 1.1.21 | `eslint` → `minimatch@3` |
+| 5.0.9 → 5.0.12 | `typescript-eslint` → `minimatch@10`; `mjml-cli` → `glob`/`minimatch`; `mjml-core` → `js-beautify` → `editorconfig` → `minimatch` |
+
+**Real exposure was low.** Every path feeds `minimatch` glob patterns that come
+from config files or the repo's own code, never from a request. The only runtime
+path is `mjml` (server action `compileMjml`), where `editorconfig` matches
+`.editorconfig` section headers, not user template content.
+
+### Independent sweep beyond `npm audit`
+
+All **472** resolved packages queried against OSV.dev. **1 hit:**
+`grapesjs@0.22.16` → GHSA-589f-c66p-hxr4 — the standing false positive recorded
+on 2026-09-13. Re-verified: `affected[].ranges` is still
+`[{"introduced":"0"}]` with no `fixed` event. Not re-triaged.
+
+### Supply-chain checks
+
+- No deprecated packages in the resolved tree.
+- Fresh releases (< 2 days) accepted after checking the previous version used the
+ same publish path: `lucide-react@1.49.0` (0.5d), `next@16.3.7` (1.1d),
+ `eslint-config-next@16.3.7` (1.1d), `docx@9.8.1` (1.7d). All four were
+ published by GitHub Actions **with provenance**, exactly as the versions they
+ replace were, and none adds an install script.
+- `dotenv@18.0.4` has **no provenance** — neither did 17.4.2; the maintainer
+ publishes manually. Same publisher (`motdotla`), zero dependencies, no install
+ scripts, 5 days of soak. Accepted.
+- **npm 11 `allowScripts` notice** — `npm install` now warns that `esbuild` and
+ `unrs-resolver` have `postinstall` scripts "not yet covered by allowScripts".
+ **Not new**: both are at the same versions as before this audit and the repo
+ has never had an `allowScripts` config. See §5.
+
+---
+
+## 2. In-range updates
+
+`next` 16.3.5 → 16.3.7, `eslint-config-next` 16.3.5 → 16.3.7, `better-auth`
+1.7.4 → 1.7.6, `zod` 4.6.4 → 4.6.5, `vitest` + `@vitest/coverage-v8` 5.0.0 →
+5.0.2, `jsdom` 30.0.1 → 30.1.1, `docx` 9.7.1 → 9.8.1, `docxtemplater` 3.69.3 →
+3.71.0, `pizzip` 3.2.0 → 3.3.0, `lucide-react` 1.45.0 → 1.49.0,
+`react-hook-form` 7.88.0 → 7.89.0, `tsx` 4.23.13 → 4.23.15, `chalk` 6.0.0 →
+6.0.1, `@types/node` 24.13.4 → 24.19.0 (stays on the 24 line).
+
+Verified: tests, lint and types identical to baseline; `next build` passes;
+`npm run setup:check` exits 0.
+
+---
+
+## 3. Removed — none
+
+Every direct dependency without a direct import is reached another way:
+
+| Package | How it is used |
+|---|---|
+| `react-dom` | Required peer of `next` |
+| `tsx` | Runs 7 `package.json` scripts |
+| `@types/mjml` | Types the dynamic `import('mjml')` in `template-editor/actions.ts`; `mjml@5.4.1` ships no `.d.ts` |
+| `@types/node`, `@types/react`, `@types/react-dom` | Ambient types |
+| `@vitest/coverage-v8` | `provider: 'v8'` in `vitest.config.mts` |
+| `typescript` | Toolchain |
+| `postcss` | Kept deliberately as the root version pin (see 2026-09-13 §2) |
+
+---
+
+## 4. Majors
+
+### Adopted: `dotenv` 17.4.2 → 18.0.4
+
+Used only by the two MP codegen scripts (`generate-types.ts`,
+`generate-storedprocs.ts`), never by the app. Breaking changes are cosmetic: the
+"injected env" notice moved from stdout to **stderr**, and the tips were
+removed. 18 also adds a `dotenv` CLI bin, an opt-in fast parser, and moves the
+entry point to `dist/index.cjs`.
+
+Verified by running both scripts with `--help`: each loaded `.env.local`
+(11 keys) and exited 0. Test/lint/types unchanged; `coverage/coverage-final.json`
+is still written at the path CI uploads.
+
+### Held — all four re-checked, all still blocked
+
+| Package | Re-check result (2026-09-30) | Clears when |
+|---|---|---|
+| `typescript` 6.0.3 → 7.0.2 | `typescript-eslint@8.71.0` still peers `typescript: >=4.8.4 <6.1.0` | TS 7.1 ships a stable API **and** `typescript-eslint` widens its peer |
+| `eslint` 9.39.5 → 10.11.0 | `eslint-plugin-react@7.37.5` is still latest, still peers `eslint ^9.7` at most | `eslint-plugin-react` supports `^10` and `eslint-config-next` picks it up |
+| `grapesjs` 0.22.16 → 0.23.6 | `@grapesjs/react@2.0.0` is still latest, still peers `grapesjs ^0.22.5` | `@grapesjs/react` peers `^0.23` |
+| `@types/node` 24 → 26.6.3 | Vercel still lists **24.x (default)**, 22.x, 20.x — no newer runtime offered | Vercel offers a newer default Node major |
+
+Re-check commands are unchanged from the 2026-09-13 record and CLAUDE.md.
+
+Note: `typescript-eslint` now peers `eslint: ^8.57 || ^9 || ^10`, so it is
+no longer part of the ESLint 10 blocker — `eslint-plugin-react` alone is.
+
+---
+
+## 5. Pre-existing issues — status
+
+### Resolved since the last record
+
+Both items in 2026-09-13 §5 have since been fixed on `dev`, outside a dependency
+audit:
+- The **12 `next build` type errors** in test files are gone (0 today), and CI
+ now runs `npm run typecheck` (#29).
+- `vitest.config.ts` was renamed to **`vitest.config.mts`**; the Vite
+ `configLoader: 'native'` warning no longer appears.
+
+### Found, NOT fixed here
+
+- **npm 11 `allowScripts`.** `esbuild` and `unrs-resolver` run `postinstall`
+ scripts that the repo has never explicitly approved. Today npm only warns; if
+ a future npm makes unapproved scripts fail closed, `npm ci` in CI would skip
+ them and `esbuild` (via Vite/`tsx`) could break. Fix is a reviewed
+ `npm approve-scripts esbuild unrs-resolver`, which writes the allow-list into
+ `package.json`. Out of scope for a version audit — it is a policy decision.
+- **Test performance hint.** Vitest reports jsdom was created 116 times
+ (~73% of tracked time) and suggests `pool: 'vmThreads'`. Not a dependency
+ change; not attempted.
diff --git a/.claude/packages/2026-10-10.md b/.claude/packages/2026-10-10.md
new file mode 100644
index 0000000..ad7995e
--- /dev/null
+++ b/.claude/packages/2026-10-10.md
@@ -0,0 +1,150 @@
+# Dependency Audit — 2026-10-10
+
+**Branch:** `chore/dependency-audit-2026-10` · **Node:** 24.18.0 · **npm:** 11.16.0
+
+## Result
+
+| | Before | After |
+|---|---|---|
+| `npm audit` | 7 high (3 root causes) | **5 high (1 root cause, no fix published)** |
+| Direct dependencies | 57 | 57 |
+| Tests | 2076 passing / 116 files | **2076 passing / 116 files** |
+| `eslint .` | clean | clean |
+| `tsc --noEmit` type errors | 0 | 0 |
+| `next build` | passes | passes |
+| Coverage (`coverage/coverage-final.json`) | written | written; 98.97% stmts / 99.71% lines |
+| OSV.dev sweep | — | 431 packages, 1 hit (`braces`, same as `npm audit`) |
+
+---
+
+## 1. Security
+
+### Fixed by in-range updates
+
+**`next` 16.3.7 → 16.4.0** — six high advisories, all `16.0.0 – 16.3.7`:
+
+| Advisory | Issue | Exposure here |
+|---|---|---|
+| [GHSA-cjq9-62q9-8jv4](https://github.com/advisories/GHSA-cjq9-62q9-8jv4) | SSRF in Image Optimization | Real on any deployment that uses `next/image` |
+| [GHSA-mcj8-r9mp-w47p](https://github.com/advisories/GHSA-mcj8-r9mp-w47p) | SSG/ISR cache poisoning → cross-user content substitution | Low: tool pages are dynamic and auth-gated |
+| [GHSA-4jqv-mc3x-m676](https://github.com/advisories/GHSA-4jqv-mc3x-m676) | SSG/ISR cache poisoning, self-hosted | Not applicable on Vercel |
+| [GHSA-3w37-wq28-93x7](https://github.com/advisories/GHSA-3w37-wq28-93x7) | `use cache` fill leaks Draft Mode content | Not used |
+| [GHSA-f87g-xv8r-7p7x](https://github.com/advisories/GHSA-f87g-xv8r-7p7x) | Metadata image routes, `dynamicParams` bypass | Low |
+| [GHSA-39w2-rjm5-chcv](https://github.com/advisories/GHSA-39w2-rjm5-chcv) | Dev-server MCP endpoint info disclosure | Dev only |
+
+**`source-map-js` 1.2.1** — [GHSA-68fv-2mgg-jv7q](https://github.com/advisories/GHSA-68fv-2mgg-jv7q)
+(event-loop DoS from indexed source-map offsets). Cleared transitively by the
+same `npm update` (via `postcss`, `next`, `jsdom`, Tailwind, Vitest).
+
+### Open, no fix exists: `braces` (accepted, lint-time only)
+
+[GHSA-vfj7-8cjw-p6xm](https://github.com/advisories/GHSA-vfj7-8cjw-p6xm),
+published 2026-09-18: stack-exhaustion DoS from deeply nested brace patterns.
+OSV range is `introduced 0, last_affected 3.0.3`. That is a real range, not the
+malformed `introduced: 0` with no events, and 3.0.3 is the **latest published
+`braces`** (last publish 2024-09). So no patched version exists.
+
+`npm audit` counts it **5 times**, once per node of the chain:
+
+```
+eslint-config-next@16.4.0
+└─ @next/eslint-plugin-next@16.4.0 (pins fast-glob 3.3.1 exactly)
+ └─ fast-glob@3.3.1
+ └─ micromatch@4.0.8
+ └─ braces@3.0.3
+```
+
+The fix `npm audit` offers is `eslint-config-next@14.2.35`. That is a downgrade
+across two majors, so it is not a fix. An `overrides` entry cannot help either,
+because there is no fixed `braces` to override to.
+
+**Exposure is lint-time only.** The only path is the ESLint plugin, which globs
+patterns from the repo's own config. It is not in the app bundle and never sees
+request data. Accepted. It clears when `braces` publishes a fix (or
+`micromatch`/`fast-glob` drops it) **and** `@next/eslint-plugin-next` moves off
+its exact `fast-glob@3.3.1` pin, if the fix doesn't land in 3.3.1's own range:
+
+```bash
+npm view braces version && npm view @next/eslint-plugin-next dependencies
+```
+
+### Supply-chain checks
+
+- No deprecated packages in the resolved tree.
+- Fresh releases (< 2 days), all accepted:
+ - `lucide-react@1.55.0` (0.3d): published by GitHub Actions **with
+ provenance**, as 1.54.0 was. No install scripts.
+ - `dotenv@18.0.7` (0.9d): no provenance, same as every prior release. The same
+ maintainer (`motdotla`) publishes manually. Zero dependencies, no install
+ scripts. Only the codegen scripts use it.
+ - `@types/node@24.19.2` (0.8d): published by the DefinitelyTyped `types` bot,
+ which never attaches provenance. Types only.
+- Every other upgraded direct package was published by GitHub Actions with
+ provenance, matching its previous version, and none adds an install script.
+- **npm 11 `allowScripts`**: still warns on `esbuild` and `unrs-resolver`, both at
+ unchanged versions. Still open, see §5.
+
+---
+
+## 2. In-range updates
+
+| Package | From → To |
+|---|---|
+| `next`, `eslint-config-next` | 16.3.7 → 16.4.0 |
+| `better-auth` | 1.7.6 → 1.7.7 |
+| `docx` | 9.8.1 → 9.9.0 |
+| `dotenv` | 18.0.4 → 18.0.7 |
+| `lucide-react` | 1.49.0 → 1.55.0 |
+| `vitest`, `@vitest/coverage-v8` | 5.0.2 → 5.0.3 |
+| `jsdom` | 30.1.1 → 30.1.2 |
+| `@vitejs/plugin-react` | 6.1.1 → 6.1.2 |
+| `postcss` | 8.5.28 → 8.5.29 |
+| `@inquirer/prompts` | 8.7.2 → 8.7.3 |
+| `@types/node` | 24.19.0 → 24.19.2 (stays on 24) |
+| `@radix-ui/*` (12 packages) | patch/minor bumps. `react-dialog`, `react-popover` 1.1.23 → 1.2.0; `react-slot` 1.3.3 → 1.4.0; `react-tooltip` 1.2.16 → 1.3.0 |
+
+Verified: tests, lint and type-error count are identical to baseline,
+`next build` passes, and `coverage/coverage-final.json` is still written.
+
+---
+
+## 3. Removed: none
+
+The same set as 2026-09-30 has no direct import. Each is still reached another
+way: `react-dom` (peer of `next`), `tsx` (package scripts), `@types/node`,
+`@types/react`, `@types/react-dom` (ambient types), `@vitest/coverage-v8`
+(`provider: 'v8'`), `typescript` (toolchain), `postcss` (deliberate root pin).
+`@types/mjml` was on that list and went with #40.
+
+---
+
+## 4. Majors
+
+### Adopted: none
+
+The only majors available are the three held packages below.
+
+### Held: all re-checked, all still blocked
+
+| Package | Re-check result (2026-10-10) | Clears when |
+|---|---|---|
+| `typescript` 6.0.3 → 7.0.2 | `typescript-eslint` still peers `typescript: >=4.8.4 <6.1.0` | TS 7.1 ships a stable API **and** `typescript-eslint` widens its peer |
+| `eslint` 9.39.5 → 10.12.0 | `eslint-plugin-react@7.37.5` is still latest, still peers `eslint ^9.7` at most. `eslint-config-next@16.4.0` still depends on it | `eslint-plugin-react` supports `^10` and `eslint-config-next` picks it up |
+| `@types/node` 24 → 26.6.5 | Vercel still lists **24.x (default)**, 22.x, 20.x | Vercel offers a newer default Node major |
+
+**Retired hold:** `grapesjs` 0.23. The package was removed in #40, so the
+`@grapesjs/react` peer blocker no longer applies. Its standing OSV false
+positive (GHSA-589f-c66p-hxr4) is also gone from the sweep.
+
+---
+
+## 5. Pre-existing issues: status
+
+Unchanged from 2026-09-30, NOT fixed here:
+
+- **npm 11 `allowScripts`.** `esbuild` and `unrs-resolver` `postinstall`
+ scripts are still unapproved. The fix is a reviewed
+ `npm approve-scripts esbuild unrs-resolver`. That is a policy decision, not a
+ version change.
+- **Test performance hint.** Vitest reports jsdom created 116 times (~58% of
+ tracked time) and suggests `pool: 'vmThreads'`. Not attempted.
diff --git a/.claude/packages/README.md b/.claude/packages/README.md
index b2b2027..25a7c12 100644
--- a/.claude/packages/README.md
+++ b/.claude/packages/README.md
@@ -18,6 +18,8 @@ later audit can re-check in one command instead of re-deriving the analysis.
| Date | Summary |
|---|---|
+| [2026-10-10](2026-10-10.md) | `next` → 16.4.0 cleared 6 high (image-optimizer SSRF, ISR cache poisoning) plus `source-map-js` DoS. `braces` DoS open with no published fix (lint-time only). TS 7 / ESLint 10 / `@types/node` holds re-checked: still blocked. GrapesJS hold retired (#40). |
+| [2026-09-30](2026-09-30.md) | Cleared 1 high (`brace-expansion` DoS) via in-range updates. Adopted dotenv 18. Re-checked all 4 holds: still blocked. |
| [2026-09-13](2026-09-13.md) | Cleared 14 advisories (1 critical). Vitest 5 / jsdom 30 / jest-dom 7 / chalk 6. Dropped 5 unused deps. Held TS 7, ESLint 10, GrapesJS 0.23. |
## Conventions
diff --git a/.claude/references/DECISIONS.md b/.claude/references/DECISIONS.md
index a3ea270..f6a2234 100644
--- a/.claude/references/DECISIONS.md
+++ b/.claude/references/DECISIONS.md
@@ -1,7 +1,7 @@
---
title: Architectural Decisions
type: adr
-last_updated: 2026-04-17
+last_updated: 2026-10-10
---
# Architectural Decisions
@@ -16,6 +16,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
- [ADR-003: `customSession` does name-splitting only; MP profile loaded on client](#adr-003-customsession-does-name-splitting-only-mp-profile-loaded-on-client)
- [ADR-004: Dual-layer route protection — edge proxy + server-component validation](#adr-004-dual-layer-route-protection--edge-proxy--server-component-validation)
- [ADR-005: Route group `(web)` for authenticated pages; `/signin` and `/api/auth` outside the group](#adr-005-route-group-web-for-authenticated-pages-signin-and-apiauth-outside-the-group)
+- [ADR-027: One MP OAuth client for the service account and user sign-in](#adr-027-one-mp-oauth-client-for-the-service-account-and-user-sign-in)
### MP Provider
- [ADR-006: Singleton `MinistryPlatformProvider` with a shared `MinistryPlatformClient`](#adr-006-singleton-ministryplatformprovider-with-a-shared-ministryplatformclient)
@@ -57,7 +58,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** The app authenticates users against Ministry Platform's OIDC provider, which has several non-standard traits (custom `realm` authorization param, MP-specific scope `http://www.thinkministry.com/dataplatform/scopes/all`, dedicated `/oauth/connect/userinfo` endpoint for user lookup). The auth layer must run cleanly on Next.js 16 App Router with Turbopack. `src/lib/auth.ts` still reads `NEXTAUTH_URL` / `NEXTAUTH_SECRET` as fallbacks, evidence that the codebase migrated from NextAuth.
-**Decision:** Use Better Auth 1.5 (`better-auth@^1.5.5`) with the `genericOAuth` plugin, a custom `getUserInfo` that calls `/oauth/connect/userinfo`, and `mapProfileToUser` that stores the OIDC `sub` claim as a user `additionalField` named `userGuid`. Wire Next.js integration with `nextCookies()` plugin and mount `toNextJsHandler(auth)` at `/api/auth/[...all]`.
+**Decision:** Use Better Auth (adopted at 1.5; now `better-auth@^1.7.7`) with the `genericOAuth` plugin, a custom `getUserInfo` that calls `/oauth/connect/userinfo`, and `mapProfileToUser` that stores the OIDC `sub` claim as a user `additionalField` named `userGuid`. Wire Next.js integration with `nextCookies()` plugin and mount `toNextJsHandler(auth)` at `/api/auth/[...all]`.
**Consequences:** Custom `getUserInfo` and `mapProfileToUser` are required for MP's OIDC quirks. `additionalFields` isn't in the `genericOAuth` option types, so a `Record` cast is needed when returning from `mapProfileToUser`. First-class Next.js 16 integration keeps route handlers minimal.
**Alternatives considered:**
- **NextAuth v5** — heavier, less flexible for MP's custom OIDC details; migration evidence suggests it was the previous choice and was replaced.
@@ -71,6 +72,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Context:** The tools suite is an internal app for MP staff. Adding a dedicated Postgres/Redis instance for session storage would be operational overhead disproportionate to the feature set. `auth.api.getSession()` is called on every authenticated server-component render, so the happy path must be fast.
**Decision:** Enable `session.cookieCache` with `strategy: "jwt"` and a 1-hour `maxAge`, plus `account.storeAccountCookie: true` and `storeStateStrategy: "cookie"`. Do not configure a database adapter (`src/lib/auth.ts`).
**Consequences:** Zero infrastructure beyond env vars. `getSession()` verifies the JWT cookie without a DB roundtrip. No server-side session list, no admin revocation, no cross-device logout. Sub-1-hour cookie TTL caps how long a stolen cookie is usable; refresh-token rotation behaviour in pure-cookie mode is unverified in this repo.
+**Amended 2026-09-30 (security Step 3):** the "sub-1-hour" claim was wrong — stateless better-auth defaults `cookieCache.refreshCache` to `true`, so a copied cookie re-signed itself for up to 7 days. Now: `expiresIn` 12 h, `disableSessionRefresh`, `strategy: "jwe"`, `refreshCache: false` (an unbacked cookie dies within 1 h), `storeAccountCookie: false` and no `offline_access`. Still no database. See `auth/sessions.md`.
**Alternatives considered:**
- **DB adapter (Postgres/SQLite via Drizzle)** — requires operating an auth DB and migration pipeline.
- **Redis adapter** — another external dependency and another ops surface.
@@ -80,8 +82,9 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** A tempting design is to enrich the session object inside `customSession` with the user's full MP profile (roles, user groups, Contact_ID, Image_GUID). Every consumer would then read a single object. The downside is that `customSession` runs on every cache miss, and MP profile lookups require extra MP API calls (`dp_Users` + `dp_User_Roles` + `dp_User_User_Groups`).
-**Decision:** `customSession` in `src/lib/auth.ts:398-413` does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the parameterless `getCurrentUserProfile()` server action on mount (the action re-derives the GUID from the session — a GUID parameter would be an IDOR) and exposes `useUser()`.
+**Decision:** `customSession` (callback extracted as `enrichSession` in `src/lib/auth.ts`) does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the parameterless `getCurrentUserProfile()` server action on mount (the action re-derives the GUID from the session — a GUID parameter would be an IDOR) and exposes `useUser()`.
**Consequences:** `getSession()` stays cheap. Sign-in does not break when MP is down. Client components that need roles/groups must mount under `UserProvider`; every page load incurs one extra round-trip. `UserService.getUserProfile()` issues three queries (profile + roles + groups).
+**Note (2026-10-10):** `MPUserProfile` is now display-only (`First_Name`, `Nickname`, `Last_Name`, `Email_Address`) and `getUserProfile()` issues **one** `dp_Users` query; roles and user groups are no longer sent to the client. Security-role checks use `AuthorizationService` server-side. See `services/user-service.md`.
**Alternatives considered:**
- **Enrich in `customSession`** — would hit MP API on every JWT refresh and couple sign-in availability to MP uptime.
- **Load profile server-side in layout** — blocks TTFB on every page render.
@@ -92,10 +95,10 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Status:** Accepted
**Context:** Unauthenticated requests should be redirected as early and cheaply as possible, but stale/forged cookies must still be rejected before any MP API call. Better Auth's full `auth.api.getSession()` is Node-only and too heavy for the edge runtime.
**Decision:** Two-layer gate:
-- **Layer 1 — edge proxy** (`src/proxy.ts`): uses `getSessionCookie()` from `better-auth/cookies` to do a cheap cookie-presence check and redirects to `/signin?callbackUrl=` if absent. Also sets an `x-pathname` header forwarding the original URL.
+- **Layer 1 — edge proxy** (`src/proxy.ts`): uses `getSessionCookie()` from `better-auth/cookies` to do a cheap cookie-presence check and redirects to `/signin?callbackUrl=` if absent. Also sets an `x-pathname` header forwarding the original URL (and, since security Step 6, the per-request CSP nonce).
- **Layer 2 — server component** (`src/components/layout/auth-wrapper.tsx`): calls `auth.api.getSession()` inside the `(web)` route group and redirects using the forwarded `x-pathname` if the session is missing or invalid.
-**Consequences:** Fast happy path at the edge; full validation before any protected render. Auth logic lives in two files that must stay in sync. Public paths (`/api/*`, `/signin`) are whitelisted in the proxy.
+**Consequences:** Fast happy path at the edge; full validation before any protected render. Auth logic lives in two files that must stay in sync. Public paths (`/api/auth`, `/api/auth/*`, `/signin`, `/auth-error` — `isPublicPath()` in `src/proxy.ts`) are allowlisted; every other `/api/*` route needs the session cookie. Since 2026-09-13 the real authorization gate is `AuthorizationService` in the services layer (see ADR-014).
**Alternatives considered:**
- **Single full-validation gate in the proxy** — Better Auth's full session validation isn't cheap enough for edge and has Node-only dependencies.
- **Skip edge check entirely** — every request would render a server component just to redirect, slower and more expensive than a cookie peek.
@@ -105,12 +108,22 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** All authenticated pages need a shared layout with `AuthWrapper` and the client `Providers` (which mounts `UserProvider`). The sign-in page and Better Auth's catch-all handler must render without a valid session.
-**Decision:** Use Next.js App Router route groups. Authenticated pages live under `src/app/(web)/` and inherit its `layout.tsx`. Public routes (`src/app/signin/`, `src/app/api/auth/[...all]/`) sit as siblings outside the group.
+**Decision:** Use Next.js App Router route groups. Authenticated pages live under `src/app/(web)/` and inherit its `layout.tsx`. Public routes (`src/app/signin/`, `src/app/api/auth/[...all]/`, `src/app/auth-error/`, `src/app/session-error/`) sit as siblings outside the group.
**Consequences:** Clean separation of public vs. authenticated surfaces. Adding a new tool is a one-directory drop under `src/app/(web)/tools/`. Easy to accidentally place a public page inside `(web)/`, so reviewers must watch for this.
**Alternatives considered:**
- **Per-page auth checks** — copy-paste prone; easy to forget on new pages.
- **Single top-level layout with conditional branch** — awkward in the App Router model and wastes a layout slot.
+### ADR-027: One MP OAuth client for the service account and user sign-in
+
+**Date:** 2026-10-10
+**Status:** Accepted (reverses security Step 4's dedicated OIDC client)
+**Context:** Step 4 (2026-09-30) added `MP_OIDC_CLIENT_ID` / `MP_OIDC_CLIENT_SECRET` so user sign-in would run as its own MP client, with a fallback to the service account while environments were split. That meant two MP client registrations and two secrets per deployment, plus a fallback path and a warning to maintain.
+**Decision:** The app has exactly one MP OAuth client, `MINISTRY_PLATFORM_CLIENT_ID` / `MINISTRY_PLATFORM_CLIENT_SECRET`, read through `getMpClient()` in `src/lib/env.ts`. It is the client-credentials service account and the OIDC sign-in client; its id is the id_token audience and the sign-out `client_id`. The `MP_OIDC_*` variables, `getOidcClient`, `warnIfSharedOidcClient` and the `auth.oidc.shared_client` event were removed.
+**Consequences:** One registration and one secret per deployment, and no half-configured state. Accepted risk: that one secret is both full MP data access and user sign-in, and an id_token minted for the service account's client is a valid sign-in token. The MP client record must carry the sign-in redirect URIs (`/api/auth/callback/ministryplatform`, post-logout ``) and nothing broader.
+**Alternatives considered:**
+- **Dedicated OIDC client (Step 4)** — narrower blast radius per secret, but double the client management for every deployment and customer clone.
+
---
## MP Provider
@@ -149,11 +162,12 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** MP's OAuth server returns `expires_in: 3600` (1 hour), but tokens have been observed to become invalid earlier — server restarts, credential rotations, and other backend events invalidate tokens without client notice. Trusting `expires_in` causes 401 storms.
-**Decision:** Ignore `expires_in`. `TOKEN_LIFE = 5 * 60 * 1000` is hard-coded in `src/lib/providers/ministry-platform/client.ts:6` and used for both `expiresAt` and `devExpiresAt`. `ensureValidToken()` refreshes whenever `expiresAt < Date.now()`, so at most one 5-minute window is stale.
-**Consequences:** Predictable refresh cadence; worst-case staleness is 5 minutes. OAuth traffic is ~12× per hour per process vs. once. No automatic recovery for sub-5-minute invalidations (no retry-on-401).
+**Decision:** Ignore `expires_in`. `TOKEN_LIFE = 5 * 60 * 1000` is hard-coded in `src/lib/providers/ministry-platform/client.ts:8` and used by each `TokenPipeline` (default and dev). `TokenPipeline.ensureValid()` refreshes whenever `expiresAt < Date.now()`, so at most one 5-minute window is stale.
+**Consequences:** Predictable refresh cadence; worst-case staleness is 5 minutes. OAuth traffic is ~12× per hour per process vs. once.
+**Amended 2026-09-30 (security Step 5):** sub-5-minute invalidations are now recovered — the `HttpClient` 401 hook invalidates the rejected token, refreshes once (single-flight) and retries the request once; a failed refresh is negative-cached for 5–30 s (jittered). See `mp-provider/client.md`.
**Alternatives considered:**
- **Use `expires_in` minus 60-second buffer** — fragile against early invalidation by the MP backend.
-- **Retry-on-401 with single refresh** — complicates error paths and risks infinite-loop bugs.
+- **Retry-on-401 with single refresh** — originally rejected (infinite-loop risk); adopted alongside the 5-minute lifetime in Step 5, bounded to one retry.
- **Heartbeat refresh** — extra cost without clear benefit.
### ADR-009: Two isolated token pipelines (default + dev) with `api_dev_*` routing
@@ -161,7 +175,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** Tools like the Deploy Tool (`DeployToolPanel` → `api_dev_DeployTool`) must run against a non-production MP credential set to avoid accidental prod mutation during local development. Mixing credentials into a single client risks state leakage between requests.
-**Decision:** `MinistryPlatformClient` holds two isolated pipelines — `(token, expiresAt, httpClient)` and `(devToken, devExpiresAt, devHttpClient)`. `getClientCredentialsToken(profile: 'default' | 'dev')` selects which env-var pair to use (`MINISTRY_PLATFORM_CLIENT_ID/SECRET` vs `MINISTRY_PLATFORM_DEV_CLIENT_ID/SECRET`). `ProcedureService` routes only procedures whose name starts with `api_dev_` through the dev pipeline.
+**Decision:** `MinistryPlatformClient` holds two isolated `TokenPipeline` instances — `main` and `dev` — each with its own token, expiry, single-flight refresh and `HttpClient`. `getClientCredentialsToken(profile: 'default' | 'dev')` selects which env-var pair to use (`MINISTRY_PLATFORM_CLIENT_ID/SECRET` vs `MINISTRY_PLATFORM_DEV_CLIENT_ID/SECRET`). `ProcedureService` routes only procedures whose name starts with `api_dev_` through the dev pipeline.
**Consequences:** Clean isolation; dev token refresh cannot touch prod token. Two `HttpClient` instances in memory is negligible. Dev env vars are optional at startup — the dev pipeline throws a clear error only when a caller actually invokes it without the vars set.
**Alternatives considered:**
- **Single pipeline with a pre-call hook swapping credentials** — easy to miss a swap-back, high blast radius.
@@ -194,8 +208,8 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** MP's database has hundreds of tables and stored procedures (current facts baseline: 603 generated model files). Hand-maintaining a catalog would drift constantly.
-**Decision:** `npm run mp:generate:models` produces `src/lib/providers/ministry-platform/models/*.ts` (TypeScript + Zod) using `--clean` to wipe the directory first; `npm run mp:generate:storedprocs` produces `.claude/references/ministryplatform.storedprocs.md`. `required-stored-procs.md` is hand-written for the handful of procs the app actually invokes (facts snapshot lists 8).
-**Consequences:** Schema reference docs stay in sync with MP after a regen. `required-stored-procs.md` must be manually updated whenever a service method wraps a new proc.
+**Decision:** `npm run mp:generate:models` produces `src/lib/providers/ministry-platform/models/*.ts` (TypeScript + Zod) using `--clean` to wipe the directory first; `npm run mp:generate:storedprocs` writes `mp-schema/stored-procs.md`, and the type generator writes `mp-schema/tables.md` (both keep their frontmatter; a `--search` run never overwrites the full catalog). `mp-schema/required-procs.md` is hand-written for the handful of procs the app actually invokes.
+**Consequences:** Schema reference docs stay in sync with MP after a regen (move the output into `mp-schema/`). `mp-schema/required-procs.md` must be manually updated whenever a service method wraps a new proc.
**Alternatives considered:**
- **Hand-maintained catalog** — untenable at MP's scale.
- **On-the-fly MP API lookup during agent work** — latency, no offline grep, and no stable reference.
@@ -208,8 +222,8 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
-**Context:** All five app services (`UserService`, `ToolService`, `AddressLabelService`, `GroupService`, `FieldManagementService`) need a single shared `MPHelper` per process. Construction is currently synchronous but `initialize()` may become async (e.g., to warm a cache). Tests must be able to reset state between cases.
-**Decision:** Each service has a private constructor, a private static `instance` field, and a public static `async getInstance()` that lazily constructs and awaits `initialize()` (see `src/services/userService.ts:29-35`). Tests reset via `(Service as any).instance = undefined` in `beforeEach`.
+**Context:** The MP-backed app services (`UserService`, `ToolService`, `AddressLabelService`, `GroupService`, `FieldManagementService`, `FamilyService`, plus `GooglePlacesService`) need a single shared instance per process. Construction is currently synchronous but `initialize()` may become async (e.g., to warm a cache). Tests must be able to reset state between cases.
+**Decision:** Each service has a private constructor, a private static `instance` field, and a public static `async getInstance()` that lazily constructs and awaits `initialize()` (see `src/services/userService.ts:30-36`). Tests reset via `(Service as any).instance = undefined` in `beforeEach`. Exceptions: `AuthorizationService` and `DomainTimezoneService` expose a **synchronous** `getInstance()`.
**Consequences:** Single shared instance per process. Callers always `await` `getInstance()` even though today's `initialize()` is synchronous — future async setup won't break callers. Test isolation requires explicit reset.
**Alternatives considered:**
- **Module-level `const service = new Service()`** — loses test resettability.
@@ -220,7 +234,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** Components should not know MP URL shapes, `$filter` / `$select` syntax, ambiguous-column prefixing rules, or `$userId` audit wiring. Concentrating these concerns keeps query conventions consistent.
-**Decision:** Server actions always follow: `auth.api.getSession()` → `Service.getInstance()` → `service.method()` → `MPHelper`. Services own query strings, batching, Zod schemas, and audit parameters.
+**Decision:** Server actions always follow: `Service.getInstance()` → `service.method()` → `MPHelper`. Services own query strings, batching, Zod schemas, and audit parameters. **Amended 2026-09-13:** each MP-touching service method gates itself with `AuthorizationService.requireSecurityRole()` (reads and writes) and takes `$userId` from its return value — a bare `auth.api.getSession()` in the action is not authorization (CLAUDE.md practices #12, #13).
**Consequences:** One place to change an ambiguous-column fix or a batching strategy. Clean test seam: per-service mocks of `MPHelper` cover most server-action tests.
**Alternatives considered:**
- **Actions call `MPHelper` directly** — duplicates query strings across features, drifts on ambiguous-column prefixes, and spreads `$userId` wiring.
@@ -233,9 +247,9 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
-**Context:** The Better Auth session contains identity claims (`id`, `email`, `userGuid`, `firstName`, `lastName`). Downstream features need roles, user groups, `Contact_ID`, and profile image — all of which require MP API calls and should not block the auth critical path.
-**Decision:** `useAppSession()` (wraps `authClient.useSession()`) exposes only the lightweight Better Auth session. A separate `UserProvider` (`src/contexts/user-context.tsx`) loads the MP profile lazily by `userGuid` via `getCurrentUserProfile()` and exposes it through `useUser()`. `Providers` (`src/app/providers.tsx`) composes them.
-**Consequences:** Auth-only consumers don't pay MP API latency. MP profile loads once per session on the client; refresh is explicit via `refreshUserProfile()`. Context value and callback are memoized (`useMemo`, `useCallback`).
+**Context:** The Better Auth session contains identity claims (`id`, `email`, `userGuid`, `firstName`, `lastName`). Downstream UI needs the user's MP display profile, which requires an MP API call and should not block the auth critical path.
+**Decision:** `useAppSession()` (wraps `authClient.useSession()`) exposes only the lightweight Better Auth session. A separate `UserProvider` (`src/contexts/user-context.tsx`) loads the MP profile via the parameterless `getCurrentUserProfile()` action and exposes it through `useUser()`. The load is **lazy on demand**: the provider makes no MP call until a component calls `useUser()` (whose effect calls an internal `requestProfile()` from a private context, flipping a `requested` flag), and then only once a `userGuid` is in the session; all consumers share that one load. `Providers` (`src/app/providers.tsx`) mounts `UserProvider`; `useAppSession()` is a standalone hook. Both hooks are kept as a documented **extension point** for tools built on the template — since the header `UserMenu` was removed (#54) nothing outside `src/contexts/` calls them, and with lazy fetch the mounted provider costs no MP request until a tool opts in.
+**Consequences:** Auth-only consumers don't pay MP API latency. Pages with no `useUser()` consumer make no profile request. MP profile loads once per provider lifetime on the client, on first use; refresh is explicit via `refreshUserProfile()`. Context value and callback are memoized (`useMemo`, `useCallback`).
**Alternatives considered:**
- **Single merged provider** — couples auth availability to MP availability.
- **Load profile server-side in layout** — blocks TTFB on every page.
@@ -256,10 +270,10 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** Service code interpolates user input into MP OData `$filter` strings (e.g., `User_GUID = '${guid}'`). MP's API accepts raw filter strings — there is no parameterized query builder. Something must prevent SQL-like injection without forcing every call site to adopt a heavyweight ORM.
-**Decision:** Four throwing helpers in `src/lib/validation.ts` — `validateGuid`, `validatePositiveInt`, `validateColumnName`, `escapeFilterString`. Callers invoke them on input and interpolate the returned value. Violations throw synchronously with a clear message.
+**Decision:** Throwing helpers in `src/lib/validation.ts` — `validateGuid`, `validatePositiveInt`, `validateColumnName`, `validateFkPath`, `validateMailerId`, `escapeFilterString`. Callers invoke them on input and interpolate the returned value. Violations throw synchronously with a clear message.
**Consequences:** Simple, testable (see `src/lib/validation.test.ts`), grep-friendly. Static types cannot enforce "this string was validated" — reviewers must spot missed validation at call sites.
**Alternatives considered:**
-- **Branded/opaque types** — over-engineered for a four-helper API.
+- **Branded/opaque types** — over-engineered for a handful of helpers.
- **Full ORM or query builder** — doesn't integrate with MP's REST surface.
---
@@ -324,7 +338,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
### ADR-023: Client-only GrapesJS via `next/dynamic(ssr: false)`
**Date:** 2026-04-17
-**Status:** Accepted
+**Status:** Superseded (2026-10-10) — the template editor and every GrapesJS/MJML dependency were removed. Kept for numbering; nothing in `src/` follows this decision any more.
**Context:** GrapesJS and `@grapesjs/react` depend on DOM globals and cannot run under Node SSR.
**Decision:** `src/components/template-editor/template-editor-form.tsx:12` wraps the editor canvas in `next/dynamic(..., { ssr: false })` with a `Skeleton` fallback. All editor files carry `"use client"`.
**Consequences:** No SSR for the template editor; initial paint shows a skeleton and hydrates. The rest of the `/tools` route stays server-rendered.
@@ -360,7 +374,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4
**Date:** 2026-04-17
**Status:** Accepted
**Context:** React component tests need a DOM that supports `localStorage`, `window.location`, `Headers`, and the full `@testing-library` API surface.
-**Decision:** `environment: 'jsdom'` in `vitest.config.mts` with `jsdom@^30.0.1`.
+**Decision:** `environment: 'jsdom'` in `vitest.config.mts` with `jsdom@^30.1.2`.
**Consequences:** Slightly slower than happy-dom on pure-DOM tests, but compatibility is higher for edge APIs (`Headers`, `Storage`, `fetch` mocks).
**Alternatives considered:**
- **happy-dom** — faster, but historically has gaps around `Headers`, `fetch`, and `Storage` mocks that affect MP client tests.
diff --git a/.claude/references/GLOSSARY.md b/.claude/references/GLOSSARY.md
index cfb0817..59decf7 100644
--- a/.claude/references/GLOSSARY.md
+++ b/.claude/references/GLOSSARY.md
@@ -1,7 +1,7 @@
---
title: Glossary
type: glossary
-last_updated: 2026-04-17
+last_updated: 2026-10-10
---
# Glossary
@@ -15,15 +15,15 @@ Agent-facing domain ontology for MPNext-Tools. Terms are alphabetized. Aliases a
SQL-side convention: every `api_*` stored procedure MUST declare `@DomainID INT` as its first parameter; the MP REST API injects the authenticated domain automatically so TypeScript callers never pass it.
**Not to be confused with:** `$userId` (app-side audit param) or `Domain_ID` (FK column value).
-**Defined in:** `.claude/commands/newstoredproc.md:171`
+**Defined in:** `.claude/commands/newstoredproc.md:21`
**See:** `mp-schema/required-procs.md`
## $userId (aliases: MP audit user ID)
-Option passed to `MPHelper.createTableRecords` / `updateTableRecords` that stamps MP audit columns with an MP `User_ID`; resolve from `session.user.userGuid` via `UserService.getUserIdByGuid`.
+Option passed to `MPHelper.createTableRecords` / `updateTableRecords` (and procedure/file calls) that stamps MP audit columns with an MP `User_ID`; taken only from the return value of `AuthorizationService.requireSecurityRole()` inside the service, never from a caller (CLAUDE.md practice #13).
**Not to be confused with:** `session.user.id` (Better Auth internal nanoid) or `userGuid` (MP User_GUID).
-**Defined in:** `src/services/groupService.ts:232`
+**Defined in:** `src/services/authorizationService.ts:200` (`requireSecurityRole`); used e.g. `src/services/groupService.ts:369`
**See:** `services/README.md`
## A
@@ -33,7 +33,7 @@ Option passed to `MPHelper.createTableRecords` / `updateTableRecords` that stamp
MP REST error raised when a column name appears in multiple tables joined by `_TABLE` FK traversal; resolved by prefixing with `TableName.Column` or `FKColumn_TABLE.Column`.
**Not to be confused with:** SQL ambiguous-column errors (different error surface).
-**Defined in:** `src/services/addressLabelService.ts:28`
+**Defined in:** `src/services/addressLabelService.ts:30`
**See:** `services/query-patterns.md`
## api_dev_ prefix (aliases: dev procedure, DEV_PROC_PREFIX)
@@ -41,12 +41,12 @@ MP REST error raised when a column name appears in multiple tables joined by `_T
Procedures whose name starts with `api_dev_` (case-insensitive, trailing underscore required) are routed through an isolated dev-credential HttpClient; `api_developer_*` does NOT match.
**Not to be confused with:** regular `api_*` procs (default credential pipeline).
-**Defined in:** `src/lib/providers/ministry-platform/services/procedure.service.ts:6`
+**Defined in:** `src/lib/providers/ministry-platform/services/procedure.service.ts:7`
**See:** `mp-provider/services/procedure.md`
## AuthWrapper
-Server component that runs inside the `(web)` route group, calls `auth.api.getSession()`, and redirects unauthenticated users to `/signin?callbackUrl=`; layer-2 session validation complementing the proxy's layer-1 cookie check.
+Server component that runs inside the `(web)` route group, calls `auth.api.getSession()`, redirects unauthenticated users to `/signin?callbackUrl=` and sessions without a `userGuid` to `/session-error`; layer-2 session validation complementing the proxy's layer-1 cookie check.
**Not to be confused with:** `proxy` (edge-layer cookie check only).
**Defined in:** `src/components/layout/auth-wrapper.tsx:5`
@@ -67,7 +67,7 @@ Server component that runs inside the `(web)` route group, calls `auth.api.getSe
Query-string parameter on `/signin` that captures the originally requested URL; preserved across the OAuth redirect so the user lands back on the exact tool + params after sign-in.
**Not to be confused with:** `post_logout_redirect_uri` (endsession flow).
-**Defined in:** `src/proxy.ts:29`
+**Defined in:** `src/proxy.ts:82`
**See:** `routing/proxy.md`
## client credentials flow (aliases: OAuth2 client credentials, machine-to-machine auth)
@@ -75,15 +75,15 @@ Query-string parameter on `/signin` that captures the originally requested URL;
OAuth2 grant type used by the provider to acquire tokens for server-to-MP calls via `POST {baseUrl}/oauth/connect/token` with `grant_type=client_credentials`; distinct from the Better Auth user-login OAuth flow.
**Not to be confused with:** `genericOAuth` (user-facing OIDC authorization code flow).
-**Defined in:** `src/lib/providers/ministry-platform/auth/client-credentials.ts:3`
+**Defined in:** `src/lib/providers/ministry-platform/auth/client-credentials.ts:27`
**See:** `mp-provider/client.md`
## cookieCache (aliases: JWT cookie cache)
-Better Auth stateless session strategy: 1-hour TTL JWT stored in a cookie with no database — subsequent `getSession()` calls decode the cookie directly.
+Better Auth stateless session strategy: the `session_data` cookie (encrypted JWE, 1-hour `maxAge`, `refreshCache: false`) caches the session with no database — subsequent `getSession()` calls decrypt the cookie directly. The session itself has a hard 12 h ceiling (`expiresIn`). See `auth/sessions.md`.
**Not to be confused with:** server-side session storage (not used here).
-**Defined in:** `src/lib/auth.ts:12`
+**Defined in:** `src/lib/auth.ts:1060`
**See:** `auth/sessions.md`
## CredentialProfile (aliases: default, dev)
@@ -91,7 +91,7 @@ Better Auth stateless session strategy: 1-hour TTL JWT stored in a cookie with n
Union type `'default' | 'dev'` selecting which pair of env vars supplies the client id/secret; `'dev'` is used only by `ProcedureService` when executing `api_dev_*` stored procedures.
**Not to be confused with:** Better Auth's OAuth `providerId`.
-**Defined in:** `src/lib/providers/ministry-platform/auth/client-credentials.ts:1`
+**Defined in:** `src/lib/providers/ministry-platform/auth/client-credentials.ts:6`
**See:** `mp-provider/client.md`
## customSession (aliases: customSession plugin, customSessionClient)
@@ -99,7 +99,7 @@ Union type `'default' | 'dev'` selecting which pair of env vars supplies the cli
Better Auth plugin wrapping the server `auth` config that enriches every `getSession()` response; here it performs name-splitting only (no API calls). Client-side `customSessionClient()` provides type inference but does NOT expose `additionalFields`.
**Not to be confused with:** `UserProvider` (loads the full MP profile).
-**Defined in:** `src/lib/auth.ts:97`
+**Defined in:** `src/lib/auth.ts:1116` (callback: `enrichSession`)
**See:** `auth/sessions.md`
## D
@@ -124,38 +124,30 @@ Hand-written TypeScript interface/type shaping data between layers (e.g. `LabelD
## endsession (aliases: RP-initiated logout, OIDC endsession)
-MP OAuth endpoint at `/oauth/connect/endsession` that ends the SSO session and redirects to a pre-registered `post_logout_redirect_uri`.
+MP OAuth endpoint at `/oauth/connect/endsession` that ends the SSO session and redirects to a pre-registered `post_logout_redirect_uri`. The app always sends `client_id` (the OIDC sign-in client) and adds `id_token_hint` when the process that handled sign-in still holds the id_token.
**Not to be confused with:** Better Auth's local `signOut` (only clears the BA cookie).
-**Defined in:** `src/components/user-menu/actions.ts:18`
-**See:** `components/user-menu.md`
+**Defined in:** `src/components/shared-actions/sign-out.ts` (`handleSignOut`)
+**See:** `auth/oauth-flow.md` § Sign-out flow
## escapeFilterString (aliases: MP filter quote escape)
-Helper that escapes `'` -> `''`, `%` -> `[%]`, `_` -> `[_]` before interpolating user input into an MP `$filter` string; used by `GroupService.searchContacts`/`searchGroups`.
+Helper that escapes `[` -> `[[]`, `%` -> `[%]`, `_` -> `[_]`, `'` -> `''` (and maps quote look-alikes to `_`; throws on control chars or > `MAX_SEARCH_TERM_LENGTH`) before interpolating user input into an MP `$filter` LIKE string; used by `GroupService`, `FamilyService`, and `ToolService`.
**Not to be confused with:** plain quote-doubling (insufficient — misses wildcards).
-**Defined in:** `src/lib/validation.ts:25`
+**Defined in:** `src/lib/validation.ts:127`
**See:** `services/query-patterns.md`
## G
## genericOAuth (aliases: generic OAuth plugin)
-Better Auth plugin wiring arbitrary OIDC providers; configured with `providerId: "ministryplatform"`, OIDC discovery URL, `offline_access` + MP all scope, `pkce: false` (MP advertises `S256` in discovery but rejects the token exchange with `invalid_grant`), `disableIdTokenNonceBinding: true` (MP omits the claim), and `realm=realm` authorization param. As of Better Auth 1.7 it registers providers as first-class **social** providers, so the callback URL is the core `/api/auth/callback/{providerId}` — not the former `/api/auth/oauth2/callback/{providerId}`.
+Better Auth plugin wiring arbitrary OIDC providers; configured with `providerId: "ministryplatform"`, explicit authorize/token/endsession endpoints and **no** `discoveryUrl` (since security Step 4: no MP call at boot; the id_token is verified by the app in `getUserInfo` against lazily loaded discovery + JWKS), the app's single MP client (`MINISTRY_PLATFORM_CLIENT_ID`, shared with the service account), `openid` + MP all scope (no `offline_access`), `pkce: false` (MP advertises `S256` in discovery but rejects the token exchange with `invalid_grant`), no nonce (MP omits the claim), and `realm=realm` authorization param. As of Better Auth 1.7 it registers providers as first-class **social** providers, so the callback URL is the core `/api/auth/callback/{providerId}` — not the former `/api/auth/oauth2/callback/{providerId}`.
**Not to be confused with:** `client credentials flow` (server-to-server, no user).
-**Defined in:** `src/lib/auth.ts:32`
+**Defined in:** `src/lib/auth.ts:699` (`ministryPlatformProviderConfig`)
**See:** `auth/oauth-flow.md`
-## GrapesJS (aliases: grapesjs, @grapesjs/react)
-
-Client-only drag-and-drop visual editor used for the template editor; wrapped via `next/dynamic` with `ssr: false`.
-
-**Not to be confused with:** MJML (the markup GrapesJS edits).
-**Defined in:** `src/components/template-editor/editor-canvas.tsx:4`
-**See:** `components/template-editor.md`
-
## H
## HttpClient
@@ -163,7 +155,7 @@ Client-only drag-and-drop visual editor used for the template editor; wrapped vi
Low-level fetch wrapper for the provider stack; injects `Authorization: Bearer ` via closure, serializes JSON or FormData, URL-encodes query params, throws formatted `Error`s on non-2xx.
**Not to be confused with:** `MinistryPlatformClient` (token lifecycle owner).
-**Defined in:** `src/lib/providers/ministry-platform/utils/http-client.ts:4`
+**Defined in:** `src/lib/providers/ministry-platform/utils/http-client.ts:91`
**See:** `mp-provider/client.md`
## I
@@ -173,7 +165,7 @@ Low-level fetch wrapper for the provider stack; injects `Authorization: Bearer <
USPS 65-bar, 4-state (T/D/A/F) mail barcode encoding a 20-digit tracking block + optional 5/9/11-digit routing code; encoded by the hand-rolled `imbEncode()`.
**Not to be confused with:** POSTNET (legacy tall/short barcode).
-**Defined in:** `src/lib/imb-encoder.ts:14`
+**Defined in:** `src/lib/imb-encoder.ts:320`
**See:** `utils/barcodes.md`
## L
@@ -209,15 +201,15 @@ Secondary tab in `/tools/addresslabels` that lets the user upload a `.docx` temp
USPS-assigned 6- or 9-digit identifier embedded in the IMb tracking block; the serial-number length is derived from Mailer ID length (9 vs 6 digits).
**Not to be confused with:** Service Type ID (3-digit IMb field).
-**Defined in:** `src/lib/barcode-helpers.ts:21`
+**Defined in:** `src/lib/barcode-helpers.ts:22`
**See:** `utils/barcodes.md`
## MinistryPlatformClient
-Token-lifecycle + HttpClient holder; owns two independent pipelines (default + dev) each with its own token cache, `expiresAt`, and `HttpClient`.
+Token-lifecycle + HttpClient holder; owns two independent `TokenPipeline`s (default + dev), each with its own token cache, `expiresAt`, single-flight refresh, 401 retry hook, and `HttpClient`.
**Not to be confused with:** `HttpClient` (low-level fetch wrapper).
-**Defined in:** `src/lib/providers/ministry-platform/client.ts:21`
+**Defined in:** `src/lib/providers/ministry-platform/client.ts:93`
**See:** `mp-provider/client.md`
## MinistryPlatformProvider (aliases: provider singleton)
@@ -228,14 +220,6 @@ Process-singleton orchestrator that owns one `MinistryPlatformClient` and six do
**Defined in:** `src/lib/providers/ministry-platform/provider.ts:44`
**See:** `mp-provider/architecture.md`
-## MJML
-
-Responsive-email markup language; source is edited in GrapesJS and server-compiled to HTML via the `mjml` package in `compileMjml`.
-
-**Not to be confused with:** GrapesJS (the editor that emits MJML).
-**Defined in:** `src/components/template-editor/actions.ts:22`
-**See:** `components/template-editor.md`
-
## mock class (aliases: class mock)
Pattern of mocking a constructable import as a `class { method = mockFn }` literal rather than `vi.fn().mockImplementation()`; required for `MPHelper`.
@@ -249,15 +233,15 @@ Pattern of mocking a constructable import as a `class { method = mockFn }` liter
Public-API facade class for Ministry Platform operations; normalizes friendly param names (`select`, `filter`, `top`) to `$`-prefixed OData params, offers optional Zod validation on create/update, and delegates to the singleton `MinistryPlatformProvider`.
**Not to be confused with:** `MinistryPlatformProvider` (singleton internal).
-**Defined in:** `src/lib/providers/ministry-platform/helper.ts:34`
+**Defined in:** `src/lib/providers/ministry-platform/helper.ts:55`
**See:** `mp-provider/README.md`
## MPUserProfile
-Enriched client-side user profile combining `dp_Users` row + joined Contact fields + `roles[]` + `userGroups[]`; loaded via `UserService.getUserProfile(userGuid)` and cached in `UserContext`.
+Display-only client-side user profile: `First_Name`, `Nickname`, `Last_Name`, `Email_Address` (joined from the `dp_Users` row's Contact) — no IDs, phone, image, roles, or user groups; loaded via the parameterless `getCurrentUserProfile()` → `UserService.getUserProfile(userGuid)` (single query) and cached in `UserContext`.
**Not to be confused with:** Better Auth `session.user` (name/email/id only).
-**Defined in:** `src/lib/providers/ministry-platform/types/user-profile.types.ts:1`
+**Defined in:** `src/lib/providers/ministry-platform/types/user-profile.types.ts:10`
**See:** `contexts/user-provider.md`
## P
@@ -275,7 +259,7 @@ Row describing how one column is presented on an MP page: order, group, required
TS interface describing the first-result-set row returned by `api_Tools_GetPageData`: `Page_ID`, `Display_Name`, `Singular_Name`, `Table_Name`, `Primary_Key`, and optional tool-config fields.
**Not to be confused with:** `PageListItem` (list view) or `PageField` (per-field metadata).
-**Defined in:** `src/lib/tool-params.ts:3`
+**Defined in:** `src/lib/tool-params.ts:14`
**See:** `utils/tool-params.md`
## parseToolParams (aliases: ToolParams)
@@ -283,7 +267,7 @@ TS interface describing the first-result-set row returned by `api_Tools_GetPageD
Single ingestion point for Ministry Platform tool URL query strings (`pageID`, `s`, `sc`, `p`, `q`, `v`, `recordID`, `recordDescription`, `addl`); hydrates `pageData` and returns a typed `ToolParams`.
**Not to be confused with:** framework `searchParams` (raw Next.js input).
-**Defined in:** `src/lib/tool-params.ts:30`
+**Defined in:** `src/lib/tool-params.server.ts:43` (`ToolParams` type: `src/lib/tool-params.ts:28`)
**See:** `utils/tool-params.md`
## POSTNET
@@ -291,15 +275,15 @@ Single ingestion point for Ministry Platform tool URL query strings (`pageID`, `
Legacy USPS tall/short barcode for 5/9/11-digit ZIPs with Mod-10 check digit; used as IMb fallback when Mailer ID is absent or IMb encoding throws.
**Not to be confused with:** IMb (modern 4-state replacement).
-**Defined in:** `src/lib/postnet-encoder.ts:11`
+**Defined in:** `src/lib/postnet-encoder.ts:31`
**See:** `utils/barcodes.md`
## proxy (aliases: proxy.ts, middleware (old name))
-Next.js 16 renamed `middleware.ts`/`middleware()` to `proxy.ts`/`proxy()`; here it performs a session-cookie-presence check via `getSessionCookie()` (no JWT decode) and forwards `pathname + search` as the `x-pathname` header.
+Next.js 16 renamed `middleware.ts`/`middleware()` to `proxy.ts`/`proxy()`; here it performs a session-cookie-presence check via `getSessionCookie()` (no JWT decode) forwards `pathname + search` as the `x-pathname` header, and sets the per-request CSP nonce.
**Not to be confused with:** `AuthWrapper` (server-component layer-2 check).
-**Defined in:** `src/proxy.ts:4`
+**Defined in:** `src/proxy.ts:37`
**See:** `routing/proxy.md`
## R
@@ -319,7 +303,7 @@ Next.js App Router convention: a directory wrapped in parentheses groups routes
Ministry Platform user-scoped record set (identified by `Selection_ID` + `User_ID` + `Page_ID`) fetched via `api_Common_GetSelection`; the "selected records" context passed to cloud tools.
**Not to be confused with:** browser text selection or client-side state.
-**Defined in:** `src/services/toolService.ts:152`
+**Defined in:** `src/services/toolService.ts:162` (`getSelectionRecordIds`)
**See:** `mp-schema/required-procs.md`
## service singleton (aliases: getInstance pattern, lazy async singleton)
@@ -327,7 +311,7 @@ Ministry Platform user-scoped record set (identified by `Selection_ID` + `User_I
Application service class with private constructor and static `getInstance(): Promise` that lazily constructs and initializes a single shared instance holding an `MPHelper`; reset in tests via `(ServiceClass as any).instance = undefined`.
**Not to be confused with:** `MinistryPlatformProvider` singleton (provider stack internal).
-**Defined in:** `src/services/toolService.ts:81`
+**Defined in:** `src/services/toolService.ts:102`
**See:** `services/README.md`
## shadcn/ui (aliases: shadcn)
@@ -343,7 +327,7 @@ Component primitive library installed via the shadcn CLI using `new-york` style,
Test pattern: set `(ServiceClass as any).instance = undefined` inside `beforeEach` to clear cached service singletons between tests.
**Not to be confused with:** `vi.resetModules()` (full module-cache clear).
-**Defined in:** `src/services/toolService.test.ts:22`
+**Defined in:** `src/services/toolService.test.ts:49`
**See:** `testing/mocks.md`
## SkipRecord
@@ -368,8 +352,8 @@ MP REST convention for following a foreign key: suffix the FK column with `_TABL
Constant `5 * 60 * 1000` (ms); `MinistryPlatformClient` refreshes after this hard-coded 5-minute window for safety, ignoring MP's `expires_in`.
-**Not to be confused with:** `cookieCache.maxAge` (Better Auth 1-hour JWT).
-**Defined in:** `src/lib/providers/ministry-platform/client.ts:6`
+**Not to be confused with:** `cookieCache.maxAge` (Better Auth 1-hour JWE cookie cache).
+**Defined in:** `src/lib/providers/ministry-platform/client.ts:8`
**See:** `mp-provider/client.md`
## ToolContainer
@@ -387,7 +371,7 @@ Client-side full-screen flex shell that composes `DevPanel` (conditional), `Tool
Thin React hook that returns only the `data` portion of Better Auth's `authClient.useSession()`; the preferred client-side session accessor (drops `isPending`/`error`/`refetch`).
**Not to be confused with:** `useUser` (loads full MP profile from UserContext).
-**Defined in:** `src/contexts/session-context.tsx:7`
+**Defined in:** `src/contexts/use-app-session.ts:7`
**See:** `contexts/session.md`
## userGuid (aliases: User_GUID, MP User GUID)
@@ -395,7 +379,7 @@ Thin React hook that returns only the `data` portion of Better Auth's `authClien
MP Ministry Platform `User_GUID` (UUID) stored on `session.user` via Better Auth `additionalFields` and populated from the OIDC `sub` claim by `mapProfileToUser`; REQUIRED for every MP API lookup of the signed-in user — `session.user.id` is a Better Auth internal nanoid and is NOT interchangeable.
**Not to be confused with:** `session.user.id` (BA nanoid) or `User_ID` (MP int surrogate).
-**Defined in:** `src/lib/auth.ts:24`
+**Defined in:** `src/lib/auth.ts:353` (`userAdditionalFields.userGuid`)
**See:** `auth/user-identity.md`
## UserProvider
@@ -413,7 +397,7 @@ Client React provider mounted in `src/app/providers.tsx` that watches the Better
Throws if the input does not match `/^[A-Za-z_][A-Za-z0-9_]*$/`; used before interpolating column names into selects.
**Not to be confused with:** `escapeFilterString` (value escaping, not identifier validation).
-**Defined in:** `src/lib/validation.ts:18`
+**Defined in:** `src/lib/validation.ts:40`
**See:** `dto-constants/constants.md`
## validateGuid
@@ -421,7 +405,7 @@ Throws if the input does not match `/^[A-Za-z_][A-Za-z0-9_]*$/`; used before int
Throws on malformed GUID (accepts upper/lower hex); used before interpolating `User_GUID` into MP filters.
**Not to be confused with:** runtime Zod schema validation.
-**Defined in:** `src/lib/validation.ts:4`
+**Defined in:** `src/lib/validation.ts:20`
**See:** `dto-constants/constants.md`
## validatePositiveInt
@@ -429,7 +413,7 @@ Throws on malformed GUID (accepts upper/lower hex); used before interpolating `U
Throws on non-integer, zero, or negative input; strictly positive (`>= 1`).
**Not to be confused with:** non-negative integer checks (this rejects `0`).
-**Defined in:** `src/lib/validation.ts:11`
+**Defined in:** `src/lib/validation.ts:33`
**See:** `dto-constants/constants.md`
## vi.hoisted
@@ -457,5 +441,5 @@ Vitest helper that declares variables which must exist before `vi.mock()` factor
Request header set by `src/proxy.ts` on passthrough responses; carries `pathname + search` so downstream server components (notably `AuthWrapper`) can build a correct `callbackUrl`.
**Not to be confused with:** `NextRequest.nextUrl.pathname` (available only in the proxy layer).
-**Defined in:** `src/proxy.ts:13`
+**Defined in:** `src/proxy.ts:58`
**See:** `routing/proxy.md`
diff --git a/.claude/references/GOTCHAS.md b/.claude/references/GOTCHAS.md
index 97cbe4b..a378b9b 100644
--- a/.claude/references/GOTCHAS.md
+++ b/.claude/references/GOTCHAS.md
@@ -1,7 +1,7 @@
---
title: Gotchas
type: gotchas
-last_updated: 2026-04-17
+last_updated: 2026-10-10
---
# Gotchas
@@ -48,6 +48,8 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
- GOTCHA-027: Next.js errors "sync dynamic API usage" or undefined `params`
- GOTCHA-028: New public route redirects to `/signin` in a loop
- GOTCHA-029: CI runs `next lint` and fails — command unknown
+- GOTCHA-046: `This module cannot be imported from a Client Component module` from `server-only`
+- GOTCHA-047: CI `check:prerender` fails — a route was statically prerendered
### Frontend / components
- GOTCHA-030: `useUser()` throws "useUser must be used within a UserProvider"
@@ -58,8 +60,10 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
- GOTCHA-035: Dev-panel `UserToolsPanel` always shows "NOT AUTHORIZED"
- GOTCHA-036: Edit-mode Review screen shows "ID: <n>" for Primary Contact / Parent Group
- GOTCHA-037: Renaming `Secure_Check-in` breaks compilation in non-obvious places
-- GOTCHA-038: SSR/build error "window is not defined" when loading `/tools/templateeditor`
-- GOTCHA-039: Merge tokens like `{{First_Name}}` appear verbatim in sent emails
+- GOTCHA-038: _Retired 2026-10-10 (template editor removed)_
+- GOTCHA-039: _Retired 2026-10-10 (template editor removed)_
+- GOTCHA-044: A flag set inside one `setState` updater is still `false` when the next setter runs
+- GOTCHA-045: `` never renders even though the search returned nothing
### Utils / labels
- GOTCHA-040: IMb barcode silently becomes POSTNET (or disappears) with no log
@@ -76,39 +80,39 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** You pass `session.user.id` to an MP query expecting `dp_Users.User_GUID`. MP returns 0 rows, joins fail, or audit columns show the wrong user.
**Root cause:** `session.user.id` is Better Auth's internal nanoid-style ID (e.g. `1gYSNMvy6OqAm9q3DdVhtKj3Czkxd0ms`), NOT the MP `User_GUID` (UUID). The MP `sub` claim is stored in a separate `userGuid` additionalField.
**Fix:** Always use `session.user.userGuid` for MP API lookups. Needs a cast at the call site: `const userGuid = (session.user as { userGuid?: string }).userGuid`.
-**Enforced where:** `src/lib/auth.ts:22-30` (additionalFields definition), `src/contexts/user-context.tsx:29` (cast pattern), `src/auth.test.ts:189-207` (regression test).
+**Enforced where:** `src/lib/auth.ts` (`userAdditionalFields`), `src/contexts/user-context.tsx:29` (cast pattern), `src/auth.test.ts:203` (regression test).
**Related:** [auth/user-identity.md](auth/user-identity.md), [services/](services/)
### GOTCHA-002: TypeScript error "Property userGuid does not exist on type..."
**Symptom:** TS complains about `session.user.userGuid` even though the value exists at runtime.
-**Root cause:** Better Auth 1.5's `customSessionClient` does not propagate `additionalFields` into its inferred session type. Runtime is correct, types are not.
+**Root cause:** Better Auth's `customSessionClient` does not propagate `additionalFields` into its inferred session type. Runtime is correct, types are not.
**Fix:** Cast at the read site: `(session.user as { userGuid?: string }).userGuid`. Do not try to extend Better Auth's types globally.
-**Enforced where:** `src/contexts/user-context.tsx:29`, `src/lib/auth.ts:85` (map function also needs a cast).
+**Enforced where:** `src/contexts/user-context.tsx:29`, `src/lib/auth.ts` (`mapProfileToUser` returns `as Record`).
**Related:** [auth/README.md](auth/README.md)
### GOTCHA-003: Proxy lets forged/expired session cookies through
**Symptom:** Proxy does not redirect a broken session to `/signin`; the request reaches a route that then errors or returns empty data.
**Root cause:** `src/proxy.ts` uses `getSessionCookie()` from `better-auth/cookies`, which only checks for cookie presence — it does not decode or validate the JWT. Any forged or expired cookie passes the proxy.
-**Fix:** Every protected page must render inside `(web)/layout.tsx` so `AuthWrapper` runs `auth.api.getSession()` on the server. Protected API/server actions must also call `auth.api.getSession()` themselves. Never rely on the proxy alone for auth.
-**Enforced where:** `src/proxy.ts:24-26` (cookie-presence check), `src/components/layout/auth-wrapper.tsx:5-20` (real session check).
+**Fix:** Every protected page must render inside `(web)/layout.tsx` so `AuthWrapper` runs `auth.api.getSession()` on the server. Server actions and services that touch MP data must call `AuthorizationService.requireSecurityRole()` (CLAUDE.md practice #12). Never rely on the proxy alone for auth.
+**Enforced where:** `src/proxy.ts:91-95` (cookie-presence check), `src/components/layout/auth-wrapper.tsx:5-27` (real session check).
**Related:** [auth/](auth/), [routing/](routing/)
### GOTCHA-004: User lands on tool index instead of specific tool + params after sign-in
**Symptom:** After OAuth round-trip the user is dropped at `/` or the tool index rather than the originally requested URL.
**Root cause:** `callbackUrl` is dropped somewhere in the round-trip. The proxy must forward `x-pathname` on the pass-through request, and `/signin` redirects must set `callbackUrl` to that original path. `AuthWrapper` must read `x-pathname` to build the redirect.
-**Fix:** Preserve both the `x-pathname` header in `src/proxy.ts:13` and the `hdrs.get("x-pathname")` read in `src/components/layout/auth-wrapper.tsx:13`. Do not remove either without updating the other.
-**Enforced where:** `src/proxy.ts:12-15,28-30`, `src/components/layout/auth-wrapper.tsx:13-16`.
+**Fix:** Preserve both the `x-pathname` header in `src/proxy.ts:58` and the `hdrs.get("x-pathname")` read in `src/components/layout/auth-wrapper.tsx:13`. Do not remove either without updating the other.
+**Enforced where:** `src/proxy.ts:57-58` (`forwardedHeaders.set('x-pathname', fullPath)`), `src/components/layout/auth-wrapper.tsx:13-16`.
**Related:** [auth/oauth-flow.md](auth/oauth-flow.md)
### GOTCHA-005: Sign-out redirects to `http://localhost:3000` in production
**Symptom:** Clicking sign-out in prod lands the user at `http://localhost:3000` (or some other wrong base URL).
**Root cause:** `handleSignOut` uses `process.env.BETTER_AUTH_URL || process.env.NEXTAUTH_URL || 'http://localhost:3000'`. If both env vars are unset in prod, the literal fallback fires silently.
-**Fix:** Throw instead of silent fallback (mirror the `MINISTRY_PLATFORM_BASE_URL` check on the line above). TODO: `.claude/TODO/2026-04-17-auth-localhost-fallback-signout.md`.
-**Enforced where:** `src/components/user-menu/actions.ts:18-23` (not currently enforced — open TODO).
+**Fix:** Fixed — `handleSignOut` reads the origin through `getAuthBaseUrl()`, which throws when unset; there is no localhost fallback.
+**Enforced where:** `src/components/shared-actions/sign-out.ts` (`getAuthBaseUrl()` / `getMpBaseUrl()`), `src/lib/env.ts`.
**Related:** [auth/](auth/)
### GOTCHA-006: User appears to sign out but is silently re-authenticated (logout loop)
@@ -116,15 +120,15 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** Clicking sign-out clears the cookie, but the OAuth IDP immediately re-signs-in the user because the redirect back bounces into `/signin` which rehydrates the session.
**Root cause:** The `post_logout_redirect_uri` value sent by `handleSignOut` is not registered on the MP OAuth client's allow-list, so MP ignores it and issues a fresh session.
**Fix:** Register the exact value of `BETTER_AUTH_URL` as a post-logout redirect URI on the MP OAuth client. The value must match byte-for-byte (no trailing slash drift).
-**Enforced where:** `src/components/user-menu/actions.ts:18-23`.
+**Enforced where:** `src/components/shared-actions/sign-out.ts` (`handleSignOut`).
**Related:** [auth/oauth-flow.md](auth/oauth-flow.md)
### GOTCHA-007: MP audit columns show wrong user (or Better Auth nanoid) in created_by/modified_by
**Symptom:** `Created_By`, `Modified_By`, `dp_Audit_Log` rows show the test user, an empty user, or the wrong MP user.
**Root cause:** Caller passed `session.user.id` (the Better Auth internal) as `$userId`. MP expects the numeric `User_ID` (integer), not a GUID and certainly not a nanoid.
-**Fix:** Resolve the numeric MP `User_ID` before the MP call: `const userId = await UserService.getInstance().then(s => s.getUserIdByGuid(session.user.userGuid))`, then pass that integer as `$userId`.
-**Enforced where:** `src/services/groupService.ts:232,248` (correct pattern — uses `userId: number`), `src/services/userService.ts:68-79` (`getUserIdByGuid` helper).
+**Fix:** Take `$userId` from the return value of `AuthorizationService.getInstance().requireSecurityRole(...)` inside the **service** (it resolves the session's `userGuid` to the numeric `dp_Users.User_ID`). Never accept it from an action argument or caller payload (CLAUDE.md practice #13).
+**Enforced where:** `src/services/authorizationService.ts` (`requireSecurityRole` returns the acting `User_ID`); e.g. `src/services/groupService.ts:369,385`, `src/services/toolService.ts` (`deployTool`).
**Related:** [auth/user-identity.md](auth/user-identity.md), [services/](services/)
---
@@ -134,25 +138,25 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
### GOTCHA-008: First API call after cold boot fails with empty `Authorization: Bearer`
**Symptom:** Fresh server boot, first MP call returns 401 with an empty Bearer.
-**Root cause:** Service method forgot to `await this.client.ensureValidToken()` before `getHttpClient().get()`. `HttpClient` has no auto-refresh hook — it just reads whatever `() => this.token` returns.
+**Root cause:** Service method forgot to `await this.client.ensureValidToken()` before `getHttpClient().get()`. Since 2026-09-30 (Step 5) the `HttpClient` 401 hook refreshes and retries once, so this now costs an extra round trip instead of failing — but it is still a bug; do not rely on the hook for a cold client.
**Fix:** Every MP-provider service method that does a network call must start with `await this.client.ensureValidToken()` (or `ensureValidDevToken()` for `api_dev_*` procs).
-**Enforced where:** `src/lib/providers/ministry-platform/client.ts:49-68`. Seen correctly applied in e.g. `src/lib/providers/ministry-platform/services/file.service.ts:48`, `communication.service.ts:20,40`.
+**Enforced where:** `src/lib/providers/ministry-platform/client.ts` (`TokenPipeline.ensureValid`). Seen correctly applied in e.g. `src/lib/providers/ministry-platform/services/file.service.ts:49`, `communication.service.ts:28,51`.
**Related:** [mp-provider/](mp-provider/)
-### GOTCHA-009: N concurrent MP calls on cold server trigger N OAuth token POSTs
+### GOTCHA-009: N concurrent MP calls on cold server trigger N OAuth token POSTs (RESOLVED)
**Symptom:** Log shows many simultaneous `POST /oauth/connect/token` calls on a cold boot under load.
-**Root cause:** `ensureValidToken` has no in-flight promise de-duplication — each caller sees an expired token and each starts its own refresh.
-**Fix:** Cache an in-flight `refreshPromise` inside `MinistryPlatformClient` so concurrent callers all await the same pending refresh. TODO: `.claude/TODO/2026-04-17-mp-provider-token-refresh-no-dedup.md`.
-**Enforced where:** `src/lib/providers/ministry-platform/client.ts:49` (not currently enforced).
+**Root cause (historical):** `ensureValidToken` had no in-flight promise de-duplication — each caller sees an expired token and each starts its own refresh.
+**Fix:** Resolved. Each `TokenPipeline` in `client.ts` shares one in-flight refresh across concurrent callers, and after a failed refresh fails fast for 5–30 s (jittered negative cache) instead of stampeding the token endpoint.
+**Enforced where:** `src/lib/providers/ministry-platform/client.ts` (`TokenPipeline`); pinned by `client.test.ts` "Single-flight refresh and negative cache".
**Related:** [mp-provider/](mp-provider/)
### GOTCHA-010: Callers can't `e instanceof ZodError` after validation failure
**Symptom:** Validation error surfaces as a plain `Error` with a message string; calling code cannot inspect `.issues` to build field-level UI feedback.
**Root cause:** `MPHelper.createTableRecords` / `updateTableRecords` catch the ZodError and re-throw as `new Error("Validation failed for record N: ...")`, losing the structured issue list.
-**Fix:** Subclass `MPValidationError` that carries the original `ZodError`, and throw that instead. TODO: `.claude/TODO/2026-04-17-mp-provider-helper-loses-zod-issue-list.md`.
-**Enforced where:** `src/lib/providers/ministry-platform/helper.ts:185-199,269-281` (not enforced).
+**Fix:** Fixed — the helper throws `MPValidationError`, which carries the original `ZodError` as `.zodError`. Catch that and read `.zodError.issues`.
+**Enforced where:** `src/lib/providers/ministry-platform/helper.ts` (`class MPValidationError`).
**Related:** [mp-provider/](mp-provider/)
### GOTCHA-011: Local edits to files under `src/lib/providers/ministry-platform/models/` disappear
@@ -160,7 +164,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** You hand-edit a model file, run `npm run mp:generate:models`, and the edit is gone.
**Root cause:** `npm run mp:generate:models` uses the `--clean` flag, which deletes every `.ts` file in the output dir before regeneration.
**Fix:** Never hand-edit anything under `models/`. Add runtime types to `src/lib/dto/` or wrap the generated model with a DTO. If the generator output is wrong, fix the generator in `src/lib/providers/ministry-platform/scripts/generate-types.ts`.
-**Enforced where:** `src/lib/providers/ministry-platform/scripts/generate-types.ts:570-581`.
+**Enforced where:** `src/lib/providers/ministry-platform/scripts/generate-types.ts:577-581` (`options.clean`).
**Related:** [mp-provider/](mp-provider/)
### GOTCHA-012: Ambiguous column error when `$filter` joins tables
@@ -168,7 +172,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** MP REST call errors with "ambiguous column name" when querying a table whose columns exist on the base table and a joined table.
**Root cause:** MP REST does not auto-qualify columns that appear in both base and joined tables. The `Contact_ID` on `Contacts` is ambiguous with `Contact_ID` on `Contact_Addresses`, for example.
**Fix:** Prefix every ambiguous column with its table: `Contacts.Contact_ID` instead of bare `Contact_ID`. For FK traversal use `FKColumn_TABLE.Column` (e.g. `Contact_ID_TABLE.First_Name`). For multi-level FK chains use underscores between `_TABLE_` hops and a single dot before the final field: `Building_ID_TABLE_Location_ID_TABLE.Congregation_ID`.
-**Enforced where:** Documented in CLAUDE.md practice #10. Examples: `src/services/userService.ts:85` (FK traversal), `src/services/addressLabelService.ts:83` (multi-level).
+**Enforced where:** Documented in CLAUDE.md practice #10. Examples: `src/services/userService.ts:63` (FK traversal), `src/services/addressLabelService.ts:32-38` (multi-level).
**Related:** [services/query-patterns.md](services/query-patterns.md)
### GOTCHA-013: Passing `@DomainID` in `executeProcedureWithBody` causes parameter-count mismatch
@@ -176,7 +180,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** Stored procedure call errors with "Procedure expected parameter @DomainID which was not supplied" or "procedure has too many arguments".
**Root cause:** The MP REST API injects `@DomainID` automatically from the OAuth token context. Passing it in the body double-binds the parameter.
**Fix:** Never include `@DomainID` in the body passed to `executeProcedureWithBody`. DO still declare `@DomainID INT` as the first parameter in the SP DDL — it is required for MP API exposure. See user's persistent memory: "@DomainID Required — All api_* stored procs MUST have @DomainID INT as first parameter for MP API".
-**Enforced where:** `src/services/toolService.ts:126-129` (comment + correct pattern), `src/services/toolService.ts:185-187`.
+**Enforced where:** `src/services/toolService.ts:133-135` (comment + correct pattern in `getPageData`); every `executeProcedureWithBody` call in `toolService.ts` omits `@DomainID`.
**Related:** [mp-schema/](mp-schema/), [services/](services/)
### GOTCHA-014: Deploy-tool call hits production MP instance by accident
@@ -184,7 +188,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** The deploy-tool flow writes rows into the production `dp_Tools` table.
**Root cause:** Procedures prefixed with `api_dev_` are routed to the dev credentials pipeline by the provider. Without that prefix, `DeployTool` would run under production credentials.
**Fix:** Keep the `api_dev_` prefix on any proc that should stay out of prod. Ensure `MINISTRY_PLATFORM_DEV_CLIENT_ID` / `MINISTRY_PLATFORM_DEV_CLIENT_SECRET` are set in `.env.local`. Keep the dev-panel localhost-only (see GOTCHA-033). Never rename these procs without checking the ProcedureService routing.
-**Enforced where:** `src/services/toolService.ts:242-247`, `src/lib/providers/ministry-platform/client.ts:75-94` (dev pipeline).
+**Enforced where:** `src/services/toolService.ts:266-297` (`deployTool`), `src/lib/providers/ministry-platform/services/procedure.service.ts` (`DEV_PROC_PREFIX`), `src/lib/providers/ministry-platform/client.ts` (`dev` `TokenPipeline`).
**Related:** [mp-provider/client.md](mp-provider/client.md)
### GOTCHA-015: File uploaded but no `$userId` audit trail
@@ -192,7 +196,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** `dp_Files` rows appear after upload but `Modified_By` is null or the system user.
**Root cause:** `FileService.uploadFiles` sends `userId` only as a `$userId` query-string parameter, not as a FormData field. If the caller forgets to pass `params.userId` at all, no audit trail is recorded.
**Fix:** Always pass `params.userId` to `uploadFiles`. Verify MP records the audit by checking `dp_Audit_Log` rows after upload.
-**Enforced where:** `src/lib/providers/ministry-platform/services/file.service.ts:67-77`.
+**Enforced where:** `src/lib/providers/ministry-platform/services/file.service.ts:100` (`$userId` query param in `uploadFiles`).
**Related:** [mp-provider/services/](mp-provider/services/)
### GOTCHA-016: `createCommunication` with empty `[]` attachments sends JSON not multipart
@@ -200,15 +204,15 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** `createCommunication(info, [])` does not exercise the multipart path even though an array was passed.
**Root cause:** Branch condition is `attachments && attachments.length > 0`. An empty array is truthy for `&&` but `length > 0` fails, so it falls through to the JSON path. This is the intended behavior but a footgun for tests that assume "array passed ⇒ multipart used".
**Fix:** Pass `undefined` (or omit the second argument) when there are no attachments. Don't assert `postFormData` was called on an empty-array fixture.
-**Enforced where:** `src/lib/providers/ministry-platform/services/communication.service.ts:22,42`.
+**Enforced where:** `src/lib/providers/ministry-platform/services/communication.service.ts:32,55`.
**Related:** [mp-provider/services/](mp-provider/services/)
### GOTCHA-017: `getTables('Contacts')` returns multiple tables
-**Symptom:** Calling `getTables('Contacts')` returns `Contacts`, `Contact_Addresses`, `Contact_Log`, `Contact_Attributes`, etc. — not just the exact match you wanted.
-**Root cause:** The MP REST `$search` parameter is a substring match, not an exact match.
-**Fix:** Always post-filter: `tables.find((t) => t.Table_Name === exactName)`. Return `tables.find(...) ?? tables[0]` or raise if no exact match found.
-**Enforced where:** `src/services/fieldManagementService.ts:70-71`.
+**Symptom:** `getTables(term)` can return more than the one table you asked for (other tables matching the search term), in no guaranteed order.
+**Root cause:** The MP REST `$search` parameter is a search, not an exact-name lookup. (A live `$search=Contacts` on 2026-10-11 returned a single entry, but callers must not rely on that.)
+**Fix:** Always post-filter on the **`Name`** key (a `/tables` entry has no `Table_Name`): `tables.find((t) => t.Name === exactName) ?? null`. Never fall back to `tables[0]` — that silently uses a different table.
+**Enforced where:** `src/services/fieldManagementService.ts:87`.
**Related:** [services/](services/)
---
@@ -219,16 +223,16 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** User types `O'Brien` into a search box; MP request fails with 400 or — worse — a SQL-like injection token is embedded verbatim into the filter.
**Root cause:** MP `$filter` accepts raw OData-style strings. If user input is interpolated without escaping, single quotes close the literal prematurely.
-**Fix:** Always escape before embedding: `term.replace(/'/g, "''")`. For LIKE patterns with wildcards also escape `%` and `_`: use `escapeFilterString` from `src/lib/validation.ts:25-30`. Documented in CLAUDE.md practice #11.
-**Enforced where:** `src/lib/validation.ts:25-30` (helper), `src/services/toolService.ts:229` (correct inline), `src/services/groupService.ts` passim.
+**Fix:** Always escape before embedding: `term.replace(/'/g, "''")`. For LIKE patterns use `escapeFilterString` from `src/lib/validation.ts:127` (bracket-escapes `[`, `%`, `_`, doubles `'`, rejects control chars and terms over `MAX_SEARCH_TERM_LENGTH`). Documented in CLAUDE.md practice #11.
+**Enforced where:** `src/lib/validation.ts:127` (helper), `src/services/toolService.ts:254`, `src/services/groupService.ts` passim.
**Related:** [services/query-patterns.md](services/query-patterns.md), [dto-constants/](dto-constants/)
### GOTCHA-019: MP filter breaks on non-numeric record ID (NaN interpolation)
**Symptom:** MP returns 400 or empty result because the filter reads `WHERE Record_ID = NaN`.
**Root cause:** `parseInt('abc', 10)` returns `NaN`; if the service doesn't validate before interpolating into `` `Record_ID = ${id}` ``, `NaN` slips through.
-**Fix:** Call `validatePositiveInt(id)` from `src/lib/validation.ts:11-16` before building any `` `ID = ${id}` `` filter. Services like `addressLabelService` batch-validate in loops; copy that pattern.
-**Enforced where:** `src/services/addressLabelService.ts:76` (positive pattern), `src/lib/validation.ts:11-16` (helper).
+**Fix:** Call `validatePositiveInt(id)` from `src/lib/validation.ts:33` before building any `` `ID = ${id}` `` filter. Services like `addressLabelService` batch-validate in loops; copy that pattern.
+**Enforced where:** `src/services/addressLabelService.ts:83` (positive pattern), `src/lib/validation.ts:33` (helper).
**Related:** [services/query-patterns.md](services/query-patterns.md)
### GOTCHA-020: Field-management partial save leaves `dp_Page_Fields` inconsistent
@@ -236,7 +240,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** User edits 20 fields, save fails halfway; some changes are committed and others are not — no rollback.
**Root cause:** `FieldManagementService.updatePageFieldOrder` calls `api_MPNextTools_UpdatePageFieldOrder` once per field, 5 at a time, with no transaction wrapper. A failure in batch 3 leaves batches 1-2 committed.
**Fix:** Caller should wrap in `try/catch` and surface partial-save state. For true atomicity, rewrite `api_MPNextTools_UpdatePageFieldOrder` to accept a table-valued parameter and commit all rows in a single SP transaction.
-**Enforced where:** `src/services/fieldManagementService.ts:74-109`.
+**Enforced where:** `src/services/fieldManagementService.ts:105-130` (`updatePageFieldOrder`, `CONCURRENCY = 5`).
**Related:** [services/](services/), [components/field-management.md](components/field-management.md)
### GOTCHA-021: Negative `Page_Field_ID` values sent to MP if passed through carelessly
@@ -244,7 +248,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** MP rejects update with "PK violation" or silently creates a bogus row because a negative `Page_Field_ID` was sent over the wire.
**Root cause:** `fetchPageFieldData` synthesizes client-only IDs starting at `-1` and decrementing (`nextId--`) for rows that exist in the table metadata but not yet in `dp_Page_Fields`. These negative IDs are sentinel values for "not yet persisted" and must never be sent back to MP.
**Fix:** Never send `Page_Field_ID` to MP. `FieldOrderPayload` intentionally omits it. When you add a new payload type, omit `Page_Field_ID` explicitly.
-**Enforced where:** `src/components/field-management/actions.ts:34-42` (negative ID synthesis).
+**Enforced where:** `src/components/field-management/actions.ts:61-71` (negative ID synthesis).
**Related:** [components/field-management.md](components/field-management.md)
### GOTCHA-022: SkipRecord / FetchAddressLabelsResult indexing returns undefined
@@ -280,7 +284,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** `npm test -- src/services/toolService.test.ts` passes, but `npm run test:run` fails that same file with bogus mock return values from a previous test file.
**Root cause:** The service singleton (`ServiceClass.instance`) is cached across test files. The previous file's mock `MPHelper` is still attached.
**Fix:** Reset the singleton in `beforeEach`: `(ServiceClass as any).instance = undefined`.
-**Enforced where:** `src/services/toolService.test.ts:19-23` (canonical pattern).
+**Enforced where:** `src/services/toolService.test.ts:46-49` (canonical pattern).
**Related:** [testing/mocks.md](testing/mocks.md)
### GOTCHA-026: Server action test crashes with `headers(...).get is not a function`
@@ -288,7 +292,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** Testing a server action: `TypeError: (intermediate value).get is not a function` inside Better Auth.
**Root cause:** `next/headers` was mocked with a plain object (`{}`) or `new Map()`. Better Auth internally calls `.get('cookie')` on a `Headers` instance — plain objects don't implement that interface.
**Fix:** `vi.mock('next/headers', () => ({ headers: vi.fn().mockResolvedValue(new Headers()) }))`. Use a real `Headers` instance.
-**Enforced where:** `src/components/dev-panel/panels/user-tools-actions.test.ts:17-19` (canonical pattern).
+**Enforced where:** `src/components/dev-panel/panels/user-tools-actions.test.ts:16-18` (canonical pattern).
**Related:** [testing/mocks.md](testing/mocks.md)
---
@@ -307,8 +311,8 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** A newly added public route (e.g. `/healthz`) bounces to `/signin` which bounces back.
**Root cause:** The proxy's early-return whitelist was not updated for the new path.
-**Fix:** Add the path to the early-return branch in `src/proxy.ts:18`. The current whitelist is `pathname.startsWith('/api') || pathname === '/signin'`.
-**Enforced where:** `src/proxy.ts:18`.
+**Fix:** Add the path to `isPublicPath()` in `src/proxy.ts:26-35`. The current allowlist is `/api/auth`, `/api/auth/*`, `/signin` and `/auth-error` (exact-segment matches; every other `/api/*` route needs a session cookie).
+**Enforced where:** `src/proxy.ts:26-35` (`isPublicPath`); `src/proxy.test.ts`.
**Related:** [routing/](routing/)
### GOTCHA-029: CI runs `next lint` and fails — command unknown
@@ -316,11 +320,27 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** CI fails with "Unknown command: lint" or similar from a script that invokes `next lint`.
**Root cause:** Next.js 16 removed `next lint`. This project uses ESLint flat config directly via `eslint .`.
**Fix:** Use `npm run lint`. Config is native flat config in `eslint.config.mjs`.
-**Enforced where:** `package.json:9` (`"lint": "eslint ."`).
+**Enforced where:** `package.json:13` (`"lint": "eslint ."`).
**Related:** CLAUDE.md "Next.js 16 Notes"
---
+### GOTCHA-046: `This module cannot be imported from a Client Component module` from `server-only`
+
+**Symptom:** One of three, depending on where the import runs: `next build` fails with *"You're importing a module that depends on "server-only""* and a `[Client Component Browser]` import trace; `npm run mp:generate*` (or a hand-run `tsx` on the codegen scripts) throws from `node_modules/server-only/index.js`; or a new Vitest file throws the same error.
+**Root cause:** Every service, `src/lib/auth.ts`, `src/lib/env.ts`, `src/lib/tool-params.server.ts` and the MP client/credentials modules start with `import 'server-only'`. That package throws unless resolved under the `react-server` export condition, which Next applies to server code only.
+**Fix:** In the build case the guard is doing its job — the trace names the client component that reached a server module; use `import type` or call a server action instead. Codegen scripts must run as `tsx --conditions=react-server …` (the `package.json` scripts already do). Vitest aliases `server-only` to the package's `empty.js` in `vitest.config.mts`; do not remove that alias.
+**Enforced where:** `next build` (CI `Build` step); `vitest.config.mts` `resolve.alias`; `package.json` `mp:generate*` scripts.
+**Related:** [security/README.md](security/README.md), GOTCHA-047
+
+### GOTCHA-047: CI `check:prerender` fails — a route was statically prerendered
+
+**Symptom:** The `Check prerendered routes` CI step lists a route; locally, the build table shows it as `○ (Static)`.
+**Root cause:** The CSP nonce is read from the request at render time. A prerendered page has none, so under the enforced CSP it never hydrates — and it also skips server-side session checks.
+**Fix:** Give the route a dynamic signal: a `headers()`/session read, or `export const dynamic = "force-dynamic"` in a **server** module (segment config is ignored in a `"use client"` file, the same trap as GOTCHA-032). Only Next's `/_not-found` and `/_global-error` may be static.
+**Enforced where:** `scripts/check-prerender.mjs`, run by CI after `npm run build`.
+**Related:** [security/README.md](security/README.md) § Nonces force dynamic rendering
+
## Frontend / components
### GOTCHA-030: `useUser()` throws "useUser must be used within a UserProvider"
@@ -328,15 +348,15 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** Client component crashes on render with `useUser must be used within a UserProvider`.
**Root cause:** Consumer was rendered outside the `` subtree, or inside a server component that never reaches `providers.tsx`.
**Fix:** Ensure the component is a descendant of ``, which is mounted in `src/app/providers.tsx`. Convert the component to a client component (`"use client"`) if it needs `useUser()`.
-**Enforced where:** `src/contexts/user-context.tsx:76-82`.
+**Enforced where:** `src/contexts/user-context.tsx:76-82` (`useUser`).
**Related:** [contexts/](contexts/)
### GOTCHA-031: `userProfile` stays `null` and `isLoading` never resolves when session has no `userGuid`
**Symptom:** Loading spinner spins forever for a user whose session does not have a `userGuid` (e.g. edge-case cookie migration).
**Root cause:** The effect gating `loadUserProfile` only runs when `userGuid` is present AND session is settled; the "settled but no guid" branch does not set `isLoading = false` in the initial render.
-**Fix:** Gate UI on `isLoading` only when a `userGuid` is expected. Handle the missing case explicitly in consumer components. See `user-context.tsx:51-58`.
-**Enforced where:** `src/contexts/user-context.tsx:51-58`.
+**Fix:** Gate UI on `isLoading` only when a `userGuid` is expected. Handle the missing case explicitly in consumer components. See `user-context.tsx:52-60`. (Inside `(web)` this state is unreachable in practice: `AuthWrapper` redirects a session with no `userGuid` to `/session-error`.)
+**Enforced where:** `src/contexts/user-context.tsx:52-60`, `src/components/layout/auth-wrapper.tsx:24-27`.
**Related:** [contexts/](contexts/)
### GOTCHA-032: `AuthWrapper` silently stops authenticating after adding `"use client"`
@@ -344,7 +364,7 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** You add `"use client"` to `auth-wrapper.tsx` for a quick fix; the app compiles but every user is treated as signed in (or the route is never protected).
**Root cause:** `AuthWrapper` must be a server component — it calls `await headers()` and `await auth.api.getSession({ headers })`. These are server-only APIs. Adding `"use client"` silently no-ops the redirect.
**Fix:** Keep `auth-wrapper.tsx` as a server component. Move any interactive state into a child client component.
-**Enforced where:** `src/components/layout/auth-wrapper.tsx:1-20` (no `"use client"`).
+**Enforced where:** `src/components/layout/auth-wrapper.tsx` (no `"use client"`).
**Related:** [components/layout.md](components/layout.md), [auth/](auth/)
### GOTCHA-033: Dev overlay unexpectedly appears in production
@@ -352,15 +372,15 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** The amber dev-panel banner shows up in a deployed environment.
**Root cause:** The self-gating lives inside `DevPanel` (`process.env.NODE_ENV === "development"` AND hostname === localhost) — not in `ToolContainer`. `ToolContainer` always renders `` when `params` is provided.
**Fix:** Trust the DevPanel self-gate. If you need to hide the overlay locally (e.g. for a screenshot), drop the `params` prop from `ToolContainer`. Do NOT remove the self-gate.
-**Enforced where:** `src/components/dev-panel/dev-panel.tsx:20-22,56-58`.
+**Enforced where:** `src/components/dev-panel/dev-panel.tsx:14-22,59-61` (`isLocalhost`, `isDevBuild`).
**Related:** [components/dev-panel.md](components/dev-panel.md)
### GOTCHA-034: Dev-panel server actions callable in production despite UI hidden
**Symptom:** Security scan flags that `deployToolAction` / `listPagesAction` are network-addressable in prod even though the UI is hidden.
**Root cause:** `"use server"` exports create network-addressable RPC endpoints regardless of where (or whether) the UI imports them. Only `deploy-tool-actions.ts` currently has the `requireDevSession` guard that throws on `NODE_ENV === "production"`.
-**Fix:** Every dev-panel server action must call `requireDevSession()` (or an equivalent `NODE_ENV !== "production"` check) before doing any work. Consider extracting a shared helper. TODO: `.claude/TODO/2026-04-17-components-dev-panel-prod-guard-gap.md`.
-**Enforced where:** `src/components/dev-panel/panels/deploy-tool-actions.ts:13-21` (correct pattern, not consistently applied elsewhere).
+**Fix:** Every dev-panel server action must call `requireDevSession()` (or an equivalent `NODE_ENV !== "production"` check) before doing any work. Fixed — every dev-panel action file now calls the shared helper.
+**Enforced where:** `src/components/dev-panel/panels/require-dev-session.ts`, called by `contact-records-actions.ts`, `deploy-tool-actions.ts`, `selection-actions.ts`, `user-tools-actions.ts`.
**Related:** [components/dev-panel.md](components/dev-panel.md)
### GOTCHA-035: Dev-panel `UserToolsPanel` always shows "NOT AUTHORIZED"
@@ -368,40 +388,48 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
**Symptom:** The dev panel's user-tools check shows a red "NOT AUTHORIZED" banner even though the user can obviously access the tool in prod.
**Root cause:** `NEXT_PUBLIC_PROD_URL` is unset (or has a trailing slash mismatch). The check is `path.includes(prodToolUrl)` where `prodToolUrl = ${prodBase}${pathname}`; if `prodBase` is empty, the match can never succeed.
**Fix:** Set `NEXT_PUBLIC_PROD_URL` in `.env.local` to the deployed tool base URL (no trailing slash). The component already renders a helpful amber banner when unset — check for that banner instead of the red one.
-**Enforced where:** `src/components/dev-panel/panels/user-tools-panel.tsx:38-42,112-133` (handles the unset case explicitly).
+**Enforced where:** `src/components/dev-panel/panels/user-tools-panel.tsx:41-44,115` (handles the unset case explicitly).
**Related:** [components/dev-panel.md](components/dev-panel.md)
### GOTCHA-036: Edit-mode Review screen shows "ID: <n>" for Primary Contact / Parent Group
**Symptom:** In the group-wizard edit flow, the Review step shows "ID: 12345" instead of the contact's name next to Primary Contact or Parent Group.
**Root cause:** `fetchGroupRecord` returns numeric IDs only. The display maps (`contactDisplayMap`, `groupDisplayMap`) are only seeded from user interactions during the wizard flow, never from a loaded record.
-**Fix:** Extend `GroupService.getGroup` to return display names alongside IDs and seed the display maps before `form.reset` in the edit-mode effect. TODO: `.claude/TODO/2026-04-17-components-group-wizard-display-map-edit-mode.md`.
-**Enforced where:** `src/app/(web)/tools/groupwizard/group-wizard.tsx:69-84` (not currently enforced).
+**Fix:** Fixed — `fetchGroupRecord` returns `displayNames`, and the edit-mode effect seeds both display maps before `form.reset`. If you see this again, check that a new ID field is included in `displayNames`.
+**Enforced where:** `src/app/(web)/tools/groupwizard/group-wizard.tsx` (edit-mode `useEffect`).
**Related:** [components/group-wizard.md](components/group-wizard.md)
### GOTCHA-037: Renaming `Secure_Check-in` breaks compilation in non-obvious places
**Symptom:** You rename the field in the Zod schema and the form compiles, but the Review step or form submission silently drops the value.
**Root cause:** The field name contains a hyphen and is referenced as a quoted string literal (`'Secure_Check-in'`) across schema, `STEP_FIELDS`, `SwitchRow`, and `step-review`. A refactor tool that only renames symbols will miss the string literals.
-**Fix:** Grep-search for `Secure_Check-in` across `src/components/group-wizard` and rename at all five sites. TypeScript will not help you here — the key is a string literal, not a symbol.
+**Fix:** Grep-search for `Secure_Check-in` across `src/components/group-wizard` and rename at every site (`schema.ts` declaration, `STEP_FIELDS`, defaults; `step-settings.tsx`; two reads in `step-review.tsx`). TypeScript will not help you here — the key is a string literal, not a symbol.
**Enforced where:** `src/components/group-wizard/schema.ts:43` (declaration).
**Related:** [components/group-wizard.md](components/group-wizard.md)
-### GOTCHA-038: SSR/build error "window is not defined" when loading `/tools/templateeditor`
+### GOTCHA-038: _Retired_
+
+Retired 2026-10-10: applied only to the GrapesJS template editor, which was removed along with its dependencies.
-**Symptom:** Route errors during SSR or production build: `ReferenceError: window is not defined`.
-**Root cause:** GrapesJS touches `window` / `document` at import time. Any server-side import of a GrapesJS module blows up.
-**Fix:** Import `EditorCanvas` (or anything that imports `grapesjs`) via `next/dynamic` with `ssr: false`. Keep `"use client"` at the top of every file that imports from `grapesjs`.
-**Enforced where:** `src/components/template-editor/template-editor-form.tsx:6-15` (canonical pattern).
-**Related:** [components/template-editor.md](components/template-editor.md)
+### GOTCHA-039: _Retired_
+
+Retired 2026-10-10: applied only to the template editor's merge tokens; the editor was removed.
+
+### GOTCHA-044: A flag set inside one `setState` updater is still `false` when the next setter runs
+
+**Symptom:** Two state updates that must agree (e.g. remove a group from `groupedFields` *and* `groupOrder`) drift apart. A guard that looked correct never fires.
+**Root cause:** React runs updater functions (`setX(prev => …)`) during the next render, not when you call the setter. Code like `let removed = false; setA(prev => { …; removed = true; … }); if (removed) setB(…)` reads `removed` before the updater has run.
+**Fix:** Make the decision once, up front, from rendered state that both setters agree on (add it to the `useCallback` deps), then call both setters unconditionally.
+**Enforced where:** `src/components/field-management/use-field-order-state.ts` (`removeGroup`) and its tests.
+**Related:** [components/field-management.md](components/field-management.md)
-### GOTCHA-039: Merge tokens like `{{First_Name}}` appear verbatim in sent emails
+### GOTCHA-045: `` never renders even though the search returned nothing
-**Symptom:** A template authored in the editor with `{{First_Name}}` tokens gets sent to recipients with the literal text `{{First_Name}}` visible.
-**Root cause:** The template-editor has no token resolver. Tokens are emitted literally by the MJML compile step; the downstream send pipeline is responsible for substitution but does not yet exist.
-**Fix:** Resolve tokens in the downstream send/render pipeline before emitting the final HTML. TODO: `.claude/TODO/2026-04-17-components-template-editor-merge-token-resolver.md`.
-**Enforced where:** `src/components/template-editor/merge-fields.ts:9-28` (defines tokens but no resolver exists).
-**Related:** [components/template-editor.md](components/template-editor.md)
+**Symptom:** A cmdk combobox with `shouldFilter={false}` gets zero results back, but its `` message never appears, even when it is wrapped in your own `results.length === 0` condition.
+**Root cause:** `CommandEmpty` renders only when the **whole** `Command` has zero registered items (`filtered.count === 0`), not just the group it sits in. A permanent item elsewhere, such as a "+ New …" action, keeps that count above zero.
+**Fix:** Don't use `CommandEmpty` alongside a permanent item. Render a plain `` gated on your own state. Gate it on the term that was actually searched, too, or it flashes during the debounce while `results` still describes the previous term.
+**Enforced where:** `src/app/(web)/tools/addeditfamily/add-edit-family.tsx` (`FamilySearchBar`, `searchedTerm`) and its tests.
+**Related:** [components/](components/)
---
@@ -410,9 +438,9 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
### GOTCHA-040: IMb barcode silently becomes POSTNET (or disappears) with no log
**Symptom:** User configures IMb with a Mailer ID, prints labels, and sees POSTNET (or no barcode) on the output.
-**Root cause:** `preEncodeBarcodes` wraps `imbEncode` in a bare `try {} catch {}` with no logging, falling through to POSTNET. Also, if `config.mailerId` is empty (`''`), the `config.barcodeFormat === 'imb' && config.mailerId` guard skips IMb entirely and falls through silently.
-**Fix:** (a) Users: enter a valid 6- or 9-digit USPS Mailer ID. (b) Code: log the caught IMb error and surface a per-label `barcodeError` field so the UI can warn the user. TODO: `.claude/TODO/2026-04-17-utils-imb-fallback-silent.md`.
-**Enforced where:** `src/lib/barcode-helpers.ts:41-50` (silent fallback), `src/lib/dto/address-label.dto.ts:40` (mailerId not validated at config level).
+**Root cause:** `preEncodeBarcodes` falls back to POSTNET when `imbEncode` throws (originally inside a bare `try {} catch {}` with no logging — now logged, see Fix). Also, if `config.mailerId` is empty (`''`), the `config.barcodeFormat === 'imb' && config.mailerId` guard skips IMb entirely and falls through silently.
+**Fix:** (a) Users: enter a valid 6- or 9-digit USPS Mailer ID. (b) Code: fixed — the fallback is logged (`console.warn`) and the Mailer ID is validated.
+**Enforced where:** `src/lib/barcode-helpers.ts` (logged fallback). Log payload is `{ labelIndex, error:
}` — no label name, address, or routing digits (rule 14) — so locate the row by its index in the request.
**Related:** [utils/](utils/), [components/address-labels.md](components/address-labels.md)
### GOTCHA-041: Avery label cells misaligned after adding new stock
@@ -426,15 +454,15 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs
### GOTCHA-042: Tool page crashes when it assumes `params.pageData` is present
**Symptom:** Tool route throws "Cannot read properties of undefined" on `params.pageData.Table_Name` or similar.
-**Root cause:** `parseToolParams` catches errors from `ToolService.getPageData` and leaves `pageData` as `undefined`. The SP `api_Tools_GetPageData` may not exist yet in the target MP instance (dev/new installs).
+**Root cause:** `parseToolParams` (`src/lib/tool-params.server.ts`) catches errors from `ToolService.getPageData` and leaves `pageData` as `undefined`. The SP `api_Tools_GetPageData` may not exist yet in the target MP instance (dev/new installs).
**Fix:** Always treat `pageData` as optional at consumption sites: guard with `if (params.pageData) { ... }`. Never destructure it without a null check.
-**Enforced where:** `src/lib/tool-params.ts:54-61` (catch + undefined assignment).
+**Enforced where:** `src/lib/tool-params.server.ts:65-76` (catch + undefined assignment).
**Related:** [utils/](utils/)
### GOTCHA-043: `pageID` / `recordID` values of `NaN` appear in downstream state
**Symptom:** Downstream code receives `NaN` as a numeric param and constructs a filter `WHERE Contact_ID = NaN`, producing no rows and a confusing log.
**Root cause:** `parseToolParams` uses `x ? parseInt(x, 10) : undefined`. `parseInt('abc', 10)` returns `NaN` (not undefined), so `NaN` leaks through.
-**Fix:** Filter with `Number.isFinite` after `parseInt`: `const n = parseInt(x, 10); return Number.isFinite(n) ? n : undefined`. TODO: `.claude/TODO/2026-04-17-utils-tool-params-parseint-nan.md`.
-**Enforced where:** `src/lib/tool-params.ts:49,63-70` (not currently enforced).
+**Fix:** Fixed — `parseIntOrUndefined` returns `undefined` unless `Number.isFinite(parsed)`.
+**Enforced where:** `src/lib/tool-params.server.ts` (`parseIntOrUndefined`).
**Related:** [utils/](utils/)
diff --git a/.claude/references/INDEX.md b/.claude/references/INDEX.md
index 6436a51..a53bcf0 100644
--- a/.claude/references/INDEX.md
+++ b/.claude/references/INDEX.md
@@ -1,7 +1,7 @@
---
title: Reference Index
type: index
-last_updated: 2026-04-17
+last_updated: 2026-10-10
---
# Reference Index
@@ -13,6 +13,7 @@ Agent-facing navigation map for MPNext-Tools. For each question pattern, find th
| If you need to... | Read |
|---|---|
| Understand OAuth, sessions, or route protection | [auth/README.md](auth/README.md) |
+| Check who may use a tool, the endpoint surface, CSP/headers, or security findings | [security/README.md](security/README.md) |
| Distinguish `session.user.id` from `userGuid` | [auth/user-identity.md](auth/user-identity.md) |
| Trace the MP provider / API client stack | [mp-provider/README.md](mp-provider/README.md) |
| Look up an MP service (table, procedure, file, etc.) | [mp-provider/services/](mp-provider/services/) |
@@ -20,6 +21,7 @@ Agent-facing navigation map for MPNext-Tools. For each question pattern, find th
| See which MP tables/FKs the app actually touches | [mp-schema/data-model-map.md](mp-schema/data-model-map.md) |
| Understand an app service's role or query patterns | [services/README.md](services/README.md) |
| Avoid MP query pitfalls (ambiguous columns, FK traversal, escaping) | [services/query-patterns.md](services/query-patterns.md) |
+| Send or read an MP date/datetime (`DomainTimezoneService`) | [ministryplatform.datetimehandling.md](ministryplatform.datetimehandling.md) |
| Find a component's location or role | [components/README.md](components/README.md) |
| Understand the Tool framework (ToolContainer, header, footer) | [components/tool-framework.md](components/tool-framework.md) |
| Understand a React context (UserProvider, useAppSession) | [contexts/README.md](contexts/README.md) |
@@ -30,6 +32,9 @@ Agent-facing navigation map for MPNext-Tools. For each question pattern, find th
| Set up Vitest or fix a mock pattern | [testing/setup.md](testing/setup.md), [testing/mocks.md](testing/mocks.md) |
| Find a DTO or shared constant | [dto-constants/README.md](dto-constants/README.md) |
| Look up barcode, label-stock, or tool-param utilities | [utils/README.md](utils/README.md) |
+| Upgrade dependencies / check what is held back | [../packages/README.md](../packages/README.md) (newest dated file), [../commands/update-deps.md](../commands/update-deps.md) |
+| Scaffold a tool or stored proc, or cut a release | [../commands/](../commands/) (`newtool`, `newstoredproc`, `release`, `update-deps`) |
+| Port a past fix into a derived repo | [../playbooks/](../playbooks/) |
| Look up a domain term | [GLOSSARY.md](GLOSSARY.md) |
| Understand why a design choice was made | [DECISIONS.md](DECISIONS.md) |
| Avoid a known trap | [GOTCHAS.md](GOTCHAS.md) |
@@ -42,24 +47,27 @@ Agent-facing navigation map for MPNext-Tools. For each question pattern, find th
| **auth** | [auth/README.md](auth/README.md) | oauth-flow, sessions, route-protection, user-identity |
| **mp-provider** | [mp-provider/README.md](mp-provider/README.md) | architecture, client, type-generation, error-handling + 6 services/ |
| **mp-schema** | [mp-schema/README.md](mp-schema/README.md) | tables, stored-procs, required-procs, data-model-map |
-| **services** | [services/README.md](services/README.md) | query-patterns + 5 per-service docs |
-| **components** | [components/README.md](components/README.md) | tool-framework, layout, ui + 6 feature docs |
+| **services** | [services/README.md](services/README.md) | query-patterns + 6 per-service docs |
+| **components** | [components/README.md](components/README.md) | tool-framework, layout, ui + 4 feature docs (address-labels, dev-panel, field-management, group-wizard) |
| **contexts** | [contexts/README.md](contexts/README.md) | user-provider, session |
| **routing** | [routing/README.md](routing/README.md) | app-router, proxy |
| **data-flow** | [data-flow/README.md](data-flow/README.md) | call-graphs, error-catalog |
| **testing** | [testing/README.md](testing/README.md) | setup, mocks, cookbook, inventory |
| **dto-constants** | [dto-constants/README.md](dto-constants/README.md) | dtos, constants |
| **utils** | [utils/README.md](utils/README.md) | barcodes, label-stock, tool-params |
+| **security** | [security/README.md](security/README.md) | single file: authorization policy, endpoint surface, headers/CSP, findings |
## Cross-cutting
-- [GLOSSARY.md](GLOSSARY.md) — 52 domain terms (alphabetized, one-line definitions)
-- [DECISIONS.md](DECISIONS.md) — 26 architectural decisions (ADRs) grouped by domain
-- [GOTCHAS.md](GOTCHAS.md) — 43 symptom-first traps with fixes
+- [GLOSSARY.md](GLOSSARY.md) — 50 domain terms (alphabetized, one-line definitions)
+- [DECISIONS.md](DECISIONS.md) — 27 architectural decisions (ADR-001–027; ADR-023 superseded) grouped by domain
+- [GOTCHAS.md](GOTCHAS.md) — 47 symptom-first traps (GOTCHA-001–047; 038/039 retired) with fixes
+- [ministryplatform.datetimehandling.md](ministryplatform.datetimehandling.md) — MP date/time boundary rules (`DomainTimezoneService`)
+- [../../.github/SECURITY.md](../../.github/SECURITY.md) — human-facing security policy (vulnerability reporting); agent detail lives in [security/README.md](security/README.md)
## Open TODOs
-- [`../TODO/INDEX.md`](../TODO/INDEX.md) — 39 open TODOs by severity / tag / area (critical → low)
+- [`../TODO/INDEX.md`](../TODO/INDEX.md) — open TODOs by severity / tag / area (open work only; closed items are deleted)
- [`../TODO/SCHEMA.md`](../TODO/SCHEMA.md) — required format for new TODO files
## Meta
@@ -68,7 +76,7 @@ Agent-facing navigation map for MPNext-Tools. For each question pattern, find th
- [`_meta/conventions.md`](_meta/conventions.md) — writing rules for editing these docs
- [`_meta/verification-rules.md`](_meta/verification-rules.md) — how to verify claims
- [`_meta/schemas/`](_meta/schemas/) — templates for every doc type
-- [`_meta/facts/`](_meta/facts/) — frozen facts snapshots per review date (baseline: `2026-04-17.md` at SHA `971c40b`)
+- [`_meta/facts/`](_meta/facts/) — frozen facts snapshots per review date (baseline: `2026-04-17.md` at SHA `971c40b`; read the newest file for current counts)
## How to maintain
diff --git a/.claude/references/_meta/conventions.md b/.claude/references/_meta/conventions.md
index 3d048c6..607cc24 100644
--- a/.claude/references/_meta/conventions.md
+++ b/.claude/references/_meta/conventions.md
@@ -8,7 +8,7 @@ Agent-only. Human readers use `README.md` at the project root.
## Writing rules
- **Terse.** Bullets, tables, frontmatter, code blocks. No narrative prose. No "this document explains..." openings.
-- **Verify every factual claim.** Grep/Read the source or use context7 for framework API claims. Never trust training data for Next.js 16 / React 19 / Zod v4 / Better Auth 1.5 / Vitest 4.
+- **Verify every factual claim.** Grep/Read the source or use context7 for framework API claims. Never trust training data for Next.js 16 / React 19 / Zod v4 / Better Auth 1.7 / Vitest 5.
- **Copy-paste examples from real source.** Do NOT invent code. Pull from actual `src/**` files.
- **Link, don't repeat.** If another doc covers it, link.
- **One responsibility per file.** If a doc is doing two things, split it.
diff --git a/.claude/references/_meta/facts/2026-09-13.md b/.claude/references/_meta/facts/2026-09-13.md
index 88b1914..b9cea56 100644
--- a/.claude/references/_meta/facts/2026-09-13.md
+++ b/.claude/references/_meta/facts/2026-09-13.md
@@ -30,8 +30,9 @@ context-engineering review and should not be edited).
| Functions | 38.35% | **98.81%** (915/926) |
The "before" figures are measured on the same denominator as "after". The
-number the config *reported* before this work was 83.14%, which was wrong — see
-`.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md`.
+number the config *reported* before this work was 83.14%, which was wrong: with no
+`coverage.include`, files no test imported were left out of the denominator (see
+CLAUDE.md § Testing).
### Test files by area
diff --git a/.claude/references/_meta/facts/2026-10-10.md b/.claude/references/_meta/facts/2026-10-10.md
new file mode 100644
index 0000000..b71ce0a
--- /dev/null
+++ b/.claude/references/_meta/facts/2026-10-10.md
@@ -0,0 +1,76 @@
+---
+title: Facts Snapshot 2026-10-10
+type: meta
+git_sha: 61566687ed38c3dd8011a2db47f26b9ab496ad40
+---
+
+## Purpose
+Authoritative counts for the full documentation review on branch
+`docs/full-doc-review`. Supersedes `2026-09-13.md` (kept as history; do not
+edit). All figures below were measured, not carried forward.
+
+## Git baseline
+- **SHA:** `61566687ed38c3dd8011a2db47f26b9ab496ad40` (`feat(no-access): sign-out form for users with no MP security role (#55)`)
+- **Branch:** `docs/full-doc-review` (off `dev`)
+- **Created:** 2026-10-10
+- Changes since `2026-09-13.md`: GrapesJS `template-editor` removed (#40), header `UserMenu` removed and `handleSignOut` moved to `src/components/shared-actions/sign-out.ts` (#54), `/no-access` sign-out form (#55).
+
+## Counts (authoritative)
+
+- **Test files:** 118
+- **Test cases:** 2,185 (from `npx vitest run`, all passing)
+- **Addendum 2026-10-11** (after #57–#60): 120 files / 2,237 cases; the per-area table in `testing/inventory.md` is current.
+- **Vitest:** 5.0.3 installed (`vitest` / `@vitest/coverage-v8` `^5.0.0` in `package.json`); jsdom 30.1.2
+- **Source files in the coverage denominator:** 118 (3,910 statements)
+
+### Coverage (from `vitest run --coverage`)
+
+| Metric | Result |
+|---|---|
+| Statements | **99.13%** (3,876/3,910) |
+| Lines | **99.80%** (3,581/3,588) |
+| Branches | 93.32% (2,490/2,668) |
+| Functions | 99.56% (916/920) |
+
+Enforced thresholds (`vitest.config.mts`): statements 97, lines 98. Branches
+and functions are deliberately unenforced.
+
+### Test files by area
+
+| Area | Files | Cases |
+|---|---|---|
+| `src/app/**` | 22 | 406 |
+| `src/` root (`auth.*` ×10, `proxy`) | 11 | 250 |
+| `src/components/address-labels` | 11 | 126 |
+| `src/components/dev-panel` | 11 | 102 |
+| `src/components/field-management` | 7 | 113 |
+| `src/components/group-wizard` | 12 | 159 |
+| `src/components/{layout,shared-actions,tool}` | 5 | 44 |
+| `src/contexts` | 2 | 13 |
+| `src/lib/providers/ministry-platform` | 14 | 481 |
+| `src/lib/**` (excl. MP provider) | 14 | 255 |
+| `src/services` | 9 | 236 |
+| **Total** | **118** | **2,185** |
+
+Per-file list: `../../testing/inventory.md`.
+
+## Components (`src/components/`)
+
+- **Feature folders (4):** `address-labels` (24 files), `dev-panel` (3 + `panels/` 20), `field-management` (16), `group-wizard` (26)
+- **Framework folders (4):** `layout` (3), `shared-actions` (7 incl. `README.md`), `tool` (5), `ui` (22)
+- **shadcn/ui primitives (22):** `alert`, `alert-dialog`, `avatar`, `badge`, `breadcrumb`, `button`, `card`, `checkbox`, `command`, `dialog`, `drawer`, `dropdown-menu`, `form`, `input`, `label`, `popover`, `radio-group`, `select`, `skeleton`, `switch`, `textarea`, `tooltip`
+- **Shared actions (3):** `getCurrentUserProfile` (`user.ts`), `getMpTimezone` (`domain.ts`), `handleSignOut` (`sign-out.ts`)
+- No `user-menu/` or `template-editor/` folder exists any more.
+
+## Tool routes (`src/app/(web)/tools/`)
+`addeditfamily`, `addresslabels`, `fieldmanagement`, `groupwizard`, `template`
+
+## Contexts (`src/contexts/`)
+`user-context.tsx` (`UserProvider`, `useUser`), `use-app-session.ts`
+(`useAppSession`, `SessionData`), `index.ts` barrel. Neither hook has a
+consumer outside `src/contexts/` at this SHA.
+
+## Services (9)
+`addressLabelService`, `authorizationService`, `domainTimezoneService`,
+`familyService`, `fieldManagementService`, `googlePlacesService`,
+`groupService`, `toolService`, `userService`
diff --git a/.claude/references/_meta/schemas/adr-entry.md b/.claude/references/_meta/schemas/adr-entry.md
index 969a95e..c17a0c0 100644
--- a/.claude/references/_meta/schemas/adr-entry.md
+++ b/.claude/references/_meta/schemas/adr-entry.md
@@ -3,13 +3,13 @@ title: ADR Entry Template
type: meta
---
-Copy this block into `DECISIONS.md`; one section per decision.
+Copy this block into `DECISIONS.md` under the matching `## ` heading (and add a TOC line); one section per decision. Never renumber or delete an ADR — supersede it, or append an `**Amended YYYY-MM-DD:**` line.
```markdown
-## ADR-NNN:
+### ADR-NNN:
**Date:** YYYY-MM-DD
-**Status:** Accepted | Superseded by ADR-XXX
+**Status:** Accepted | Superseded by ADR-XXX | Superseded (YYYY-MM-DD) —
**Context:** why the question came up
**Decision:** what we chose
**Consequences:** tradeoffs / downstream constraints
diff --git a/.claude/references/_meta/schemas/gotcha-entry.md b/.claude/references/_meta/schemas/gotcha-entry.md
index 4865463..9176f6b 100644
--- a/.claude/references/_meta/schemas/gotcha-entry.md
+++ b/.claude/references/_meta/schemas/gotcha-entry.md
@@ -3,10 +3,10 @@ title: GOTCHA Entry Template
type: meta
---
-Copy this block into `GOTCHAS.md`; one section per trap.
+Copy this block into `GOTCHAS.md` under the matching `## ` heading (and add a TOC line); one section per trap. IDs are never reused — a dead entry becomes `### GOTCHA-NNN: _Retired_` with a one-line reason.
```markdown
-## GOTCHA-NNN:
+### GOTCHA-NNN:
**Symptom:** what goes wrong
**Root cause:** underlying reason
diff --git a/.claude/references/_meta/schemas/reference-doc.md b/.claude/references/_meta/schemas/reference-doc.md
index 765531e..ec4adee 100644
--- a/.claude/references/_meta/schemas/reference-doc.md
+++ b/.claude/references/_meta/schemas/reference-doc.md
@@ -8,7 +8,7 @@ Copy this schema when writing any reference doc under a domain subfolder (e.g.,
```markdown
---
title:
-domain: auth | mp-provider | services | components | contexts | routing | data-flow | testing | dto-constants | utils
+domain: auth | mp-provider | mp-schema | services | components | contexts | routing | data-flow | security | testing | dto-constants | utils
type: reference
applies_to: [src/path/to/file.ts]
symbols: [exportName1, exportName2]
diff --git a/.claude/references/_meta/schemas/todo.md b/.claude/references/_meta/schemas/todo.md
index b07492d..d861e53 100644
--- a/.claude/references/_meta/schemas/todo.md
+++ b/.claude/references/_meta/schemas/todo.md
@@ -10,7 +10,7 @@ Used for files under `.claude/TODO/YYYY-MM-DD-.md`. See `.claude/TODO/SCHE
title:
severity: critical | high | medium | low
tags: [security | bug | drift | missing-test | refactor | doc | perf]
-area: auth | mp-provider | services | components | contexts | routing | data-flow | testing | dto-constants | utils | commands
+area: auth | mp-provider | mp-schema | services | components | contexts | routing | data-flow | security | testing | dto-constants | utils | commands
files: [src/path/to/file.ts]
discovered: YYYY-MM-DD
discovered_by:
diff --git a/.claude/references/_meta/verification-rules.md b/.claude/references/_meta/verification-rules.md
index 12feee6..cf680a4 100644
--- a/.claude/references/_meta/verification-rules.md
+++ b/.claude/references/_meta/verification-rules.md
@@ -14,7 +14,7 @@ Applies to every doc under `.claude/references/**/*.md`.
| Line number ref | `Read` with offset — content matches |
| Count (e.g., "22 components", "507 tests") | Generate current count via `Glob`/`Grep` — fix inline if drift; drop TODO if structural |
| Version (package version) | `Read` `package.json` — match |
-| Framework API claim (Next.js 16, React 19, Zod v4, Better Auth 1.5, Vitest 4) | **context7 `query-docs`** — do not trust training |
+| Framework API claim (Next.js 16, React 19, Zod v4, Better Auth 1.7, Vitest 5) | **context7 `query-docs`** — do not trust training |
| Behavior claim | `Read` referenced source — confirm the claim holds |
| Cross-ref to another doc | Target doc exists at that path |
diff --git a/.claude/references/auth/README.md b/.claude/references/auth/README.md
index 6ebb577..8832add 100644
--- a/.claude/references/auth/README.md
+++ b/.claude/references/auth/README.md
@@ -5,13 +5,13 @@ domain: auth
---
## What's in this domain
-Better Auth (`^1.5.5`) wired to Ministry Platform OIDC via the `genericOAuth` plugin, with stateless JWT cookie sessions, a `customSession` name-splitter, and dual-layer route protection (proxy + `AuthWrapper`).
+Better Auth (`1.7.x`) wired to Ministry Platform OIDC via the `genericOAuth` plugin — explicit endpoints (no boot-time discovery), the id_token verified by the app against lazily loaded discovery + JWKS, one MP client for both the service account and sign-in (`MINISTRY_PLATFORM_CLIENT_ID`), and one auth instance per process — with stateless sessions (12 h ceiling, encrypted `session_data` cookie, no user OAuth tokens), boot-time secret/URL refusals, a `customSession` name-splitter, and dual-layer route protection (proxy + `AuthWrapper`).
## File map
| File | Purpose | When to read |
|------|---------|--------------|
-| `oauth-flow.md` | `genericOAuth` config, callback URL, `getUserInfo`, `mapProfileToUser`, OIDC `endsession` logout | Wiring the MP OAuth client, debugging callbacks or logout |
-| `sessions.md` | Stateless JWT cookie strategy, `cookieCache`, `customSession`, session shape, access patterns | Reading/modifying session data, any `getSession()` call |
+| `oauth-flow.md` | `genericOAuth` config, lazy id_token verification, `getUserInfo` + sub binding, the single MP client, log events, `endsession` logout | Wiring the MP OAuth client, debugging callbacks, outages or logout |
+| `sessions.md` | Session lifetime, JWE `cookieCache`, no OAuth tokens, boot refusals (`assertAuthEnvironment`, `src/lib/env.ts`), client-IP headers, `customSession`, session shape | Reading/modifying session data, any `getSession()` call |
| `route-protection.md` | `src/proxy.ts` + `AuthWrapper` dual-layer pattern, `x-pathname` forwarding, `callbackUrl` preservation | Touching route guards or sign-in redirect behavior |
| `user-identity.md` | `session.user.id` (Better Auth internal) vs `session.user.userGuid` (MP `User_GUID`) — gotcha | Before any MP API lookup keyed by the signed-in user |
@@ -19,18 +19,24 @@ Better Auth (`^1.5.5`) wired to Ministry Platform OIDC via the `genericOAuth` pl
| Path | Role |
|------|------|
| `src/lib/auth.ts` | Server Better Auth config (plugins: `genericOAuth`, `customSession`, `nextCookies`) |
-| `src/lib/auth-client.ts` | Client `authClient` (`genericOAuthClient`, `customSessionClient`) |
-| `src/app/api/auth/[...all]/route.ts` | Route handler (`toNextJsHandler(auth)`) |
-| `src/app/signin/page.tsx` | Sign-in page — auto-redirects via `authClient.signIn.oauth2()` |
+| `src/lib/auth-client.ts` | Client `authClient` (`customSessionClient`) |
+| `src/lib/env.ts` | `getMpBaseUrl()`, `getAuthBaseUrl()`, `getMpClient()` — validated env readers |
+| `src/app/api/auth/[...all]/route.ts` | Route handler — deny-by-default allowlist + `/sign-in/social` body filter in front of `toNextJsHandler(auth)` |
+| `src/app/signin/page.tsx` + `sign-in-content.tsx` | Sign-in page (server shell, `force-dynamic`) — the client body auto-redirects via `authClient.signIn.social({ provider: "ministryplatform", callbackURL })` |
| `src/proxy.ts` | Route protection (session-cookie presence check + `x-pathname` forwarding) |
| `src/components/layout/auth-wrapper.tsx` | Server-component guard — redirects to `/signin` with `callbackUrl` |
-| `src/components/user-menu/actions.ts` | `handleSignOut()` — OIDC `endsession` redirect |
+| `src/components/shared-actions/sign-out.ts` | `handleSignOut()` — clears the session, then `endsession` with `client_id` (+ `id_token_hint` when this process has it) |
| `src/contexts/user-context.tsx` | `UserProvider` — loads MP profile client-side from `userGuid` |
-| `src/contexts/session-context.tsx` | `useAppSession()` — thin wrapper around `authClient.useSession()` |
-| `src/auth.test.ts` | Unit tests for `customSession`, `getUserInfo`, `mapProfileToUser` logic |
+| `src/contexts/use-app-session.ts` | `useAppSession()` — thin wrapper around `authClient.useSession()` (returns `data`) |
+| `src/auth.test.ts` | Config pins, `customSession`, `mapProfileToUser` |
+| `src/auth.user-info.test.ts` | `getUserInfo` with genuinely signed id_tokens (verification, alg confusion, sub binding) |
+| `src/auth.oidc-discovery.test.ts` | MP down/hanging at boot and at the callback; lazy verifier units |
+| `src/auth.id-token-sign-in.test.ts` / `src/auth.shared-instance.test.ts` | F12 layers / one instance per process |
+| `src/auth.secret-guard.test.ts`, `src/auth.session-config.test.ts`, `src/auth.session-lifetime.test.ts`, `src/auth.user-oauth-tokens.test.ts`, `src/auth.ip-address.test.ts` | Boot refusals, cookie-cache config, 12 h cap, token stripping, client-IP headers |
+| `src/test-utils/mock-oidc.ts` | Mock MP OIDC provider (per-endpoint failure modes, `signIdToken`) |
| `src/proxy.test.ts` | Proxy route-protection tests |
| `src/components/layout/auth-wrapper.test.tsx` | `AuthWrapper` redirect tests |
-| `src/components/user-menu/actions.test.ts` | `handleSignOut` tests |
+| `src/components/shared-actions/sign-out.test.ts` | `handleSignOut` tests |
## Related domains
- `../contexts/README.md` — `UserProvider` consumes `userGuid` from the session
diff --git a/.claude/references/auth/oauth-flow.md b/.claude/references/auth/oauth-flow.md
index e7af464..d426be3 100644
--- a/.claude/references/auth/oauth-flow.md
+++ b/.claude/references/auth/oauth-flow.md
@@ -1,195 +1,111 @@
---
-title: OAuth Flow (genericOAuth + OIDC endsession)
+title: OAuth Flow (genericOAuth, lazy id_token verification, OIDC endsession)
domain: auth
type: reference
-applies_to: [src/lib/auth.ts, src/app/signin/page.tsx, src/app/api/auth/[...all]/route.ts, src/components/user-menu/actions.ts]
-symbols: [auth, handleSignOut]
-related: [sessions.md, user-identity.md, route-protection.md]
-last_verified: 2026-04-17
+applies_to: [src/lib/auth.ts, src/lib/env.ts, src/app/signin/sign-in-content.tsx, src/app/api/auth/[...all]/route.ts, src/components/shared-actions/sign-out.ts, src/app/session-error/page.tsx]
+symbols: [auth, ministryPlatformProviderConfig, lazyIdTokenVerifier, sharedInstance, getMpClient, handleSignOut]
+related: [sessions.md, user-identity.md, route-protection.md, ../security/README.md]
+last_verified: 2026-10-10
---
## Purpose
-End-to-end OAuth2/OIDC flow against Ministry Platform: sign-in, token exchange, profile mapping, and OIDC-style logout via `endsession`.
+End-to-end OAuth2/OIDC flow against Ministry Platform: sign-in, token exchange,
+id_token verification, profile mapping, and RP-initiated logout via `endsession`.
## Files
-- `src/lib/auth.ts` — `genericOAuth` provider config, exported as `ministryPlatformProviderConfig` (`providerId: "ministryplatform"`)
-- `src/app/signin/page.tsx` — client page that calls `authClient.signIn.oauth2(...)`
-- `src/app/api/auth/[...all]/route.ts` — Better Auth route handler via `toNextJsHandler(auth)`
-- `src/components/user-menu/actions.ts` — `handleSignOut()` server action for OIDC logout
-- `src/auth.test.ts` — unit tests for `getUserInfo`, `mapProfileToUser`
-- `src/components/user-menu/actions.test.ts` — tests for `handleSignOut`
+- `src/lib/auth.ts` — provider config (`ministryPlatformProviderConfig`, `providerId: "ministryplatform"`), `lazyIdTokenVerifier`, `sharedInstance`
+- `src/lib/env.ts` — `getMpBaseUrl()`, `getAuthBaseUrl()`, `getMpClient()` (validated env readers)
+- `src/app/signin/sign-in-content.tsx` — calls `authClient.signIn.social({ provider: "ministryplatform", callbackURL })`
+- `src/app/api/auth/[...all]/route.ts` — deny-by-default allowlist in front of `toNextJsHandler(auth)`
+- `src/components/shared-actions/sign-out.ts` — `handleSignOut()` server action (the `