test: raise non-UI unit coverage from 71.9% to 99.5% statements

.claude/TODO/ci-broken-ajv-lockfile-drift.md

@@ -0,0 +1,81 @@
+# TODO: CI is red on `main` — `npm ci` fails on `ajv` lockfile drift
+
+**Created:** 2026-08-21
+**Severity:** High — **every** CI run on `main` and on every branch fails at the install step. No PR can be verified by CI until this is fixed.
+**Status:** Open. Pre-existing; discovered while pushing unit-test coverage (PR #71), unrelated to that work.
+
+## Symptom
+
+`npm ci` fails in ~8s on GitHub Actions, before any test runs:
+
+```
+npm error `npm ci` can only install packages when your package.json and
+npm error package-lock.json or npm-shrinkwrap.json are in sync.
+npm error Invalid: lock file's ajv@6.15.0 does not satisfy ajv@8.20.0
+npm error Missing: ajv@6.15.0 from lock file
+npm error Missing: fast-uri@3.1.5 from lock file
+npm error Invalid: lock file's json-schema-traverse@0.4.1 does not satisfy json-schema-traverse@1.0.0
+npm error Missing: json-schema-traverse@0.4.1 from lock file
+```
+
+## When it started
+
+Introduced by `64f18f0` ("Package Update Cleanup"). The run immediately before it
+(`32470332071`, merge of PR #70) was green; `32472743169` on `64f18f0` is red with this
+error, and every run since has failed identically.
+
+Verified byte-identical between the `main` run and PR #71's run, and PR #71 touches neither
+`package.json` nor `package-lock.json` — so this is not branch-specific.
+
+## Cause
+
+Two packages want different `ajv` majors:
+
+| Package | Requires |
+|---|---|
+| `eslint` | `ajv@^6.14.0` |
+| `@hookform/resolvers` | `ajv@^8` |
+
+`package-lock.json` contains exactly **one** `node_modules/ajv` entry, pinned to `6.15.0`
+(line ~5242). The nested `ajv@8.x` entry that `@hookform/resolvers` needs is absent, along
+with its `fast-uri@3.1.5` and `json-schema-traverse@1.0.0` subtree.
+
+This is the same class of failure as `.claude/TODO/investigate-emnapi-lockfile-drift.md`: a
+Windows `npm install` / `npm dedupe` pruned nested entries out of the lockfile, and `npm ci`
+on Linux then refuses to proceed. `ajv` is a different victim, same mechanism.
+
+## Fix
+
+Regenerate the lockfile without touching `node_modules`, then confirm both `ajv` trees survive:
+
+```bash
+npm install --package-lock-only
+git diff package-lock.json    # expect a nested ajv@8.x under @hookform/resolvers
+```
+
+**Do this in WSL, a Linux container, or with `--os=linux --cpu=x64`.** A bare
+`npm install` on Windows is what caused this, and the emnapi TODO documents it re-pruning
+Linux-only optional entries — fixing `ajv` on Windows risks reintroducing that drift in the
+same commit.
+
+Then verify the way CI does, on Linux:
+
+```bash
+rm -rf node_modules && npm ci && npm run test:run
+```
+
+## Worth doing alongside
+
+Both incidents share one root cause: lockfiles are generated on Windows and consumed on
+Linux. Options in `.claude/TODO/investigate-emnapi-lockfile-drift.md` §"Things to try" apply
+verbatim here — in particular a CI guard or pre-commit hook that runs `npm ci --dry-run`
+before a lockfile change can reach `main`. That would have caught both incidents at the
+commit that introduced them rather than one merge later.
+
+The `/audit-deps` skill is the natural home for this check.
+
+## Related
+
+- `.claude/TODO/investigate-emnapi-lockfile-drift.md` — same mechanism, different packages
+- `64f18f0` — the commit that introduced it
+- Failing run on `main`: https://github.com/MinistryPlatform-Community/MPNext/actions/runs/32472743169

.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md

@@ -0,0 +1,49 @@
+# TODO: Contact-log actions authenticate but never authorize
+
+**Created:** 2026-08-21
+**Severity:** Medium-High — needs a policy decision before it can be called a bug or a feature.
+**Status:** Open. Documented during the test-coverage push. Requires a product decision, not just code.
+
+## Symptom
+
+In `src/components/contact-logs/actions.ts`, `updateContactLog`, `deleteContactLog`,
+`getContactLogById`, and `getContactLogsByContactId` all confirm that *a* valid session exists, then
+act on whatever ID they are handed. Nothing verifies that:
+
+- the contact log belongs to the caller, or
+- the caller is permitted to touch that contact at all.
+
+`deleteContactLog` is the sharpest edge. Unlike create/update it does not even resolve `userGuid`:
+
+```ts
+const session = await auth.api.getSession({ headers: await headers() });
+if (!session?.user?.id) throw new Error("Authentication required");
+if (!contactLogId || contactLogId <= 0) throw new Error("Valid Contact Log ID is required");
+await contactLogService.deleteContactLog(contactLogId);
+```
+
+Any authenticated session can delete **any** contact log in the domain by ID. Given CLAUDE.md's
+stance on MP write safety ("Ministry Platform is a shared production database containing real church
+member data"), this needs an explicit decision rather than an implicit one.
+
+## The decision to make
+
+Either:
+
+**(a)** "Any authenticated staff user may read, edit, and delete any contact log" is the intended
+policy — MP itself is a staff-facing system and this may well match how the church operates. If so,
+document it in `.claude/references/auth.md` and add a test that *encodes* the decision, so a future
+reader knows it was chosen rather than overlooked.
+
+**(b)** Ownership or role gating is required. Then `deleteContactLog` and `updateContactLog` should
+load the log first, compare `Made_By` against the acting `User_ID`, and reject mismatches unless the
+caller holds a supervisory role.
+
+Option (a) is plausible and cheap. What is not acceptable is leaving it ambiguous — the current tests
+mirror the code's assumptions exactly (`deleteContactLog(42)` asserts the service was called with
+`42`), so they keep passing under either policy and encode nothing.
+
+## Related
+
+- `.claude/TODO/mp-filter-injection-numeric-ids.md` — the same entry points, different defect
+- `.claude/docs/TestCoverage.md` §7.5

.claude/TODO/contact-log-actions-bypass-session-context-service.md

@@ -0,0 +1,79 @@
+# TODO: Contact-log actions re-implement User_ID resolution instead of using `SessionContextService`
+
+**Created:** 2026-08-21
+**Severity:** Medium — redundant MP round-trips per write, duplicated logic, and it contradicts the
+project's own decided policy on unattributed writes.
+**Status:** Open. Refactor, not a bug — behavior is currently correct-ish but the wrong shape.
+
+## Symptom
+
+`src/components/contact-logs/actions.ts` resolves the acting user's MP `User_ID` inline, and does it
+**twice** — once in `createContactLog` and again, byte-for-byte, in `updateContactLog`:
+
+```ts
+const { MPHelper } = await import("@/lib/providers/ministry-platform");
+const mp = new MPHelper();
+const users = await mp.getTableRecords<{ User_ID: number }>({
+  table: "dp_Users",
+  filter: `User_GUID = '${sanitizeGuid(userGuid)}'`,
+  select: "User_ID",
+  top: 1
+});
+if (!users || users.length === 0 || !users[0].User_ID) {
+  throw new Error("Unable to determine user User_ID");
+}
+```
+
+Three separate things are wrong with this:
+
+### 1. The work is already done
+
+`src/lib/auth.ts` has `resolveMpUserId`, which performs exactly this `dp_Users` lookup, caches it
+process-wide by `User_GUID`, and bakes the result into the session as `session.user.userId` via
+`customSession`. The actions ignore that and pay an uncached MP round-trip on **every single write**.
+
+### 2. `SessionContextService` exists for precisely this call site
+
+`src/services/sessionContextService.ts` is documented as the canonical way to get the acting user for
+a write:
+
+> "Use this — not `getCurrentUserId` — at every MP write boundary."
+
+`getActingUserIdForWrite({ table, operation })` reads `session.user.userId` (free, already resolved)
+and emits a structured `mp.write.non_user` warning when it comes back null. It is fully tested at
+100%. Nothing in `contact-logs/actions.ts` calls it.
+
+### 3. Throwing on an unresolved user contradicts the decided policy
+
+The actions throw `"Unable to determine user User_ID"` and abandon the write. `SessionContextService`
+was built on the opposite premise — anonymous writes are legitimate and should be *logged*, not
+*blocked*, so the unattributed write is visible in production logs and can be investigated. Right now
+a user whose `dp_Users` row is missing or whose lookup transiently fails simply cannot save a contact
+log at all.
+
+## Fix
+
+Replace both inline blocks with:
+
+```ts
+import { sessionContextService } from "@/services/sessionContextService";
+...
+const userId = await sessionContextService.getActingUserIdForWrite({
+  table: "Contact_Log",
+  operation: "create",   // or "update"
+});
+const logDataWithUser = { ...contactLogData, Made_By: userId };
+```
+
+Then delete the now-unused `getUserGuid` helper and the `MPHelper`/`sanitizeGuid` imports if nothing
+else in the file needs them.
+
+Confirm before doing this that `Made_By` accepts null on the MP side. If it does not, the graceful
+path is to omit the field rather than to fail the write — decide and document which.
+
+## Test impact
+
+`src/components/contact-logs/actions.test.ts` mocks `MPHelper.getTableRecords` to satisfy the inline
+lookup; those mocks get replaced with a `sessionContextService` mock. Add a case asserting that a
+null acting user still performs the write and emits `mp.write.non_user`, which is the behavior change
+this refactor is really about.

.claude/TODO/contact-logs-component-untested.md

@@ -0,0 +1,44 @@
+# TODO: `contact-logs.tsx` — 602 lines driving MP writes, 0% coverage
+
+**Created:** 2026-08-21
+**Severity:** Medium — the largest untested file in the app, on the app's only write path.
+**Status:** Open. Explicitly **out of scope** for the >90% non-UI coverage target (this is a React
+component), but it is the highest-value remaining test gap in the repo and should not get lost.
+
+## Symptom
+
+`src/components/contact-logs/contact-logs.tsx` is 602 lines at 0% coverage. It is the component that
+drives contact-log **create / update / delete** against Ministry Platform. It owns:
+
+- form state and client-side validation
+- the delete-confirmation gate
+- optimistic updates and rollback
+- error handling and user-facing error surfaces
+
+The server actions beneath it sit at 97.8% coverage — but the actions are the easy half. The form
+logic, the confirmation gate, and the error handling are where a regression silently corrupts or
+deletes real member data.
+
+## Why it matters more than the coverage number suggests
+
+CLAUDE.md is unambiguous about MP write safety. The only interactive path a user has to mutate MP
+data in this app goes through code that no test has ever executed. A regression that, say, fires
+delete before the confirmation resolves would not be caught by anything in the suite.
+
+## Suggested approach — targeted, not exhaustive
+
+`@testing-library/react` is already installed and `components/layout/auth-wrapper.test.tsx` proves
+the harness works. Do not chase full render coverage. Three tests, in priority order:
+
+1. **The delete-confirmation gate.** Clicking delete does *not* call `deleteContactLog` until the
+   confirmation is accepted; cancelling calls nothing.
+2. **Form validation before submit.** Missing `Contact_Date` or `Notes` does not reach
+   `createContactLog` (the action throws on these, but the component should never send them).
+3. **Action failure surfaces to the user** and does not leave an optimistic row in place.
+
+Even these three beat zero by a wide margin.
+
+## Related
+
+- `.claude/docs/TestCoverage.md` §6.3
+- `.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md`

.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md

@@ -0,0 +1,54 @@
+# TODO: `MinistryPlatformClient` discards `expires_in` and caps every token at 5 minutes
+
+**Created:** 2026-08-21
+**Severity:** Low — wasteful, and the code contradicts its own comment.
+**Status:** Open.
+
+## Symptom
+
+`src/lib/providers/ministry-platform/client.ts`:
+
+```ts
+// Token refresh interval - refresh 5 minutes before actual expiration for safety
+const TOKEN_LIFE = 5 * 60 * 1000; // 5 minutes
+...
+const creds = await getClientCredentialsToken();
+this.token = creds.access_token;
+// Set expiration time with safety buffer (TOKEN_LIFE before actual expiration)
+this.expiresAt = new Date(Date.now() + TOKEN_LIFE);
+```
+
+The comments describe subtracting a safety buffer from the real expiry. The code instead sets every
+token's usable life to exactly 5 minutes, discarding the `expires_in` value that MP returns in the
+token response.
+
+MP client-credentials tokens are typically valid for an hour, so this means roughly 12x more token
+requests than necessary. Behavior is correct — just wasteful, and the stated intent and the actual
+behavior disagree, which is the kind of gap that bites whoever edits it next.
+
+## Fix
+
+```ts
+const creds = await getClientCredentialsToken();
+this.token = creds.access_token;
+const lifetimeMs = (Number(creds.expires_in) || 3600) * 1000;
+const SAFETY_MARGIN = 5 * 60 * 1000;
+this.expiresAt = new Date(Date.now() + Math.max(lifetimeMs - SAFETY_MARGIN, 30_000));
+```
+
+Rename `TOKEN_LIFE` to `TOKEN_SAFETY_MARGIN` so the constant says what it is. The `max(..., 30s)`
+floor keeps a pathologically short `expires_in` from causing a refresh storm.
+
+## Test to add alongside the fix
+
+- `expires_in: 3600` -> `expiresAt` is ~55 minutes out
+- `expires_in` missing -> falls back to the 1-hour default
+- `expires_in: 60` -> clamped to the 30s floor rather than going negative
+
+Note for whoever writes these: `client.test.ts` already exercises the refresh path, and the current
+behavior is not pinned by any assertion on `expiresAt` — so the fix will not break existing tests,
+which is precisely the problem.
+
+## Related
+
+- `.claude/docs/TestCoverage.md` §7.7

.claude/TODO/mp-filter-injection-numeric-ids.md

@@ -0,0 +1,68 @@
+# TODO: Numeric IDs are interpolated into MP filters without sanitization
+
+**Created:** 2026-08-21
+**Severity:** High — confirmed filter injection reachable from a public server action.
+**Status:** Open. Documented during the test-coverage push; deliberately **not** fixed, because the fix changes runtime behavior on read paths and deserves its own review.
+
+## Symptom
+
+`ContactLogService` interpolates caller-supplied IDs straight into the MP `$filter` string:
+
+- `src/services/contactLogService.ts:101` — `filter: \`Contact_Log_ID = ${contactLogId}\``
+- `src/services/contactLogService.ts:118` — `filter: \`Contact_ID = ${contactId}\``
+- `src/services/contactLogService.ts:83` — same shape via `searchContactLogs`
+- `src/services/userService.ts:75` and `:80` — `User_ID = ${profile.User_ID}` (lower risk; the value originates from MP, not from a caller)
+
+The codebase has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid` in
+`src/lib/providers/ministry-platform/utils/filter-sanitize.ts`, and applies them faithfully to every
+**string** parameter. There is no equivalent for numeric IDs, and the TypeScript `number` annotation
+is erased at runtime.
+
+## Why the action-level guard does not stop it
+
+`src/components/contact-logs/actions.ts` guards with `if (!contactLogId || contactLogId <= 0)`.
+For `contactLogId = "1 OR 1=1"`:
+
+```
+!id      -> false   (a non-empty string is truthy)
+id <= 0  -> false   (string/number comparison does not reject it)
+guard passes
+```
+
+Server actions compile to callable POST endpoints. A caller controls the payload *shape*, not just
+its values, so a string arriving where the signature says `number` is entirely reachable.
+
+## Reproduction
+
+Verified empirically against the real service with a mocked `MPHelper` (probe test since removed):
+
+```
+getContactLogById("1 OR 1=1")  ->  filter: "Contact_Log_ID = 1 OR 1=1"
+searchContactLogs("5; DROP")   ->  filter: "Contact_ID = 5; DROP"
+```
+
+Both reach the MP API. `Contact_Log_ID = 1 OR 1=1` widens a single-record read into a full-table read.
+
+## Proposed fix
+
+1. Add to `filter-sanitize.ts`:
+
+```ts
+export function sanitizeNumericId(value: unknown): number {
+  const n = typeof value === 'number' ? value : Number(value);
+  if (!Number.isInteger(n) || n <= 0) {
+    throw new Error('Invalid numeric ID');
+  }
+  return n;
+}
+```
+
+2. Apply it at all five interpolation sites listed above.
+3. Test the rejection set: `'1 OR 1=1'`, `'5; DROP'`, `NaN`, `Infinity`, `1.5`, `-1`, `0`, `null`,
+   `undefined`, `'  7  '` (decide whether whitespace-padded numerics are accepted or rejected).
+
+## Why the existing tests missed it
+
+`contactLogService.ts` is at 100% statement coverage. No test passes a non-numeric value to any of
+these methods, so every line executes and the defect survives. This is the clearest example in the
+repo of coverage measuring the wrong thing — see `.claude/docs/TestCoverage.md` §7.