fix: close 3 high-severity findings, gate lint + types in CI

.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md

@@ -6,7 +6,7 @@ area: components
 files: [src/components/address-labels/actions.ts]
 discovered: 2026-09-13
 discovered_by: coverage-agent-address-labels
-status: open
+status: resolved
 ---
 
 ## Problem
@@ -56,3 +56,31 @@ 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.

.claude/TODO/2026-09-13-page-logs-raw-error-object.md

@@ -6,7 +6,7 @@ area: components
 files: [src/app/(web)/tools/addeditfamily/page.tsx]
 discovered: 2026-09-13
 discovered_by: coverage-agent-addeditfamily
-status: open
+status: resolved
 ---
 
 ## Problem
@@ -60,3 +60,29 @@ 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.

.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md

@@ -6,7 +6,7 @@ area: components
 files: [src/components/field-management/use-field-order-state.ts]
 discovered: 2026-09-13
 discovered_by: coverage-agent-field-management
-status: open
+status: resolved
 ---
 
 ## Problem
@@ -87,3 +87,43 @@ 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).

.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md

@@ -6,7 +6,7 @@ area: testing
 files: [.github/workflows/test.yml, package.json]
 discovered: 2026-09-13
 discovered_by: coverage-review-orchestrator
-status: open
+status: resolved
 ---
 
 ## Problem
@@ -83,3 +83,37 @@ 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.

.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md

@@ -6,7 +6,7 @@ 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: open
+status: resolved
 ---
 
 ## Problem
@@ -75,3 +75,48 @@ 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.

.claude/TODO/INDEX.md

@@ -22,14 +22,19 @@ Severity tiers:
 - **medium**: doc drift, missing test, refactor with real cost
 - **low**: nits, minor doc fixes, stylistic improvements
 
-Total: **9 open TODOs**.
+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. Details in each
-> file.
+> 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`.
 
 ---
 
@@ -38,18 +43,12 @@ Total: **9 open TODOs**.
 ### Critical (0)
 _none open_
 
-### High (3)
-| Area | Tags | Title | File |
-|---|---|---|---|
-| services | security, bug | Unvalidated client-supplied numeric fields interpolated into MP `$filter` strings in FamilyService | [→](2026-09-13-unvalidated-envelope-donor-ids-in-filter.md) |
-| components | bug | `removeGroup` drops a non-empty group's fields from the save payload | [→](2026-09-13-removegroup-order-guard-mismatch.md) |
-| components | security, bug | `mergeTemplate` console.errors the raw docxtemplater error, which can carry household addresses | [→](2026-09-13-mergetemplate-logs-address-pii-on-error.md) |
+### High (0)
+_none open_
 
-### Medium (4)
+### Medium (2)
 | Area | Tags | Title | File |
 |---|---|---|---|
-| testing | bug, testing, drift | Add a type-check gate to CI (stale fixtures had silently broken `npm run build`) | [→](2026-09-13-testing-no-typecheck-gate-in-ci.md) |
-| components | security, drift | `AddEditFamilyPage` logs the raw error object instead of an identifier | [→](2026-09-13-page-logs-raw-error-object.md) |
 | 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) |
 
@@ -63,23 +62,15 @@ _none open_
 
 ## By tag
 
-### security (3)
-- unvalidated-envelope-donor-ids-in-filter — high
-- mergetemplate-logs-address-pii-on-error — high
-- page-logs-raw-error-object — medium
+### security (0)
+_none open_
 
-### bug (6)
-- unvalidated-envelope-donor-ids-in-filter — high
-- removegroup-order-guard-mismatch — high
-- mergetemplate-logs-address-pii-on-error — high
-- testing-no-typecheck-gate-in-ci — medium
+### bug (3)
 - components-template-editor-no-mp-persistence — medium
 - components-template-editor-merge-token-resolver — medium
 - search-empty-state-never-renders — low
 
-### drift (3)
-- testing-no-typecheck-gate-in-ci — medium
-- page-logs-raw-error-object — medium
+### drift (1)
 - components-template-editor-no-mp-persistence — medium
 
 ### missing-test (1)
@@ -89,8 +80,8 @@ _none open_
 - components-template-editor-merge-token-resolver — medium
 - dead-empty-fields-branch-handlenext — low
 
-### testing (1)
-- testing-no-typecheck-gate-in-ci — medium
+### testing (0)
+_none open_
 
 ### doc (0)
 _none open_
@@ -104,9 +95,9 @@ _none open_
 
 | Area | Count |
 |---|---|
-| components | 6 |
-| testing | 1 |
-| services | 1 |
+| components | 4 |
+| testing | 0 |
+| services | 0 |
 | auth | 0 |
 | mp-provider | 0 |
 | utils | 0 |
@@ -126,6 +117,11 @@ _none open_
 | 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) |
 
 ---