mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 21:10:59 +00:00
docs(planning): v1.4.42 QA round findings reports
Carry-over from the v1.4.42 marathon W10 reconcile — the six W10 reviewer reports (code-reviewer, security, design, senior-dev, product-lead, simplifier-residual) for the release-trail audit-input that gated the v1.4.42 tag.
This commit is contained in:
Marc-André Bombeck committed
1 parent
a0bfce6da5
commit
6f62193b99
6 files changed
+1387
No files matched your search
@@ -0,0 +1,256 @@
|
||||
# v1.4.42 Code-review findings
|
||||
|
||||
W10 — read-only code review, scope: every commit in `9e775aa3..develop`
|
||||
(i.e. everything after the v1.4.41 squash-merge `67207d72`). Phase reports
|
||||
under `.planning/phase-W*-v1442-report.md` already documented their own
|
||||
deltas; this review only flags items those reports did not already raise.
|
||||
|
||||
## Verdict
|
||||
|
||||
**APPROVE_WITH_FIXES** — code quality is high across all six waves and
|
||||
the existing tests pin every contract worth pinning. Two release-closure
|
||||
items (CHANGELOG + `package.json` version bump) are the only blockers
|
||||
between develop and a clean v1.4.42 tag, and both are part of the standard
|
||||
release-cut step rather than wave scope. No Critical-tier defects found.
|
||||
|
||||
## Critical (must fix before tag)
|
||||
|
||||
_None._
|
||||
|
||||
## High (should fix before tag)
|
||||
|
||||
### H-1 — `package.json` version + CHANGELOG.md not bumped for v1.4.42
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/package.json:3` still reads
|
||||
`"version": "1.4.41"`.
|
||||
- `/Users/marc/Projects/HealthLog/CHANGELOG.md` has no v1.4.42 section
|
||||
in the develop diff (`git diff 9e775aa3..develop -- CHANGELOG.md` is
|
||||
empty).
|
||||
- Fix at release-cut time: bump `package.json`, add CHANGELOG section
|
||||
in Marc-voice English following v1.4.41's pattern. Without it the
|
||||
release-marathon's `NEXT_PUBLIC_APP_VERSION` build arg + the
|
||||
VersionPoller self-healing shell (memo
|
||||
`project_v1438_ios_coord_closure.md`) will mismatch the actual tag.
|
||||
|
||||
### H-2 — APNs result row label can mislead when only `APNS_KEY_FILE` is set
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/scripts/check-env.ts:191` prints
|
||||
`[OK] APNS_KEY` even when the present variable is actually
|
||||
`APNS_KEY_FILE`. The `anyOf` lookup at line 132 satisfies the row,
|
||||
but the rendered name is the manifest's primary `name` field.
|
||||
- Symptom — an operator scanning the output sees `[OK] APNS_KEY`,
|
||||
greps the Coolify env for `APNS_KEY`, finds nothing, files a
|
||||
ghost issue. Exactly the v1.4.40 AP-2 debug loop this script was
|
||||
meant to short-circuit.
|
||||
- Fix: when an `anyOf` row passes, render the matched alternative
|
||||
name (e.g. `[OK] APNS_KEY_FILE (satisfies APNS_KEY)`) rather than
|
||||
the primary. Two-line patch in `renderResults`.
|
||||
|
||||
## Medium (recommended for tag)
|
||||
|
||||
### M-1 — `bucket-series.ts` still carries its own `BERLIN_DAY_FORMATTER`
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/insights/bucket-series.ts:19`
|
||||
declares a local `BERLIN_DAY_FORMATTER` identical to the one W4 hoisted
|
||||
into `tz/resolver.ts:53`.
|
||||
- Different consumer (returns `{year, month, day}` numbers via
|
||||
`toBerlinYmd`, not the `YYYY-MM-DD` string `toBerlinDayKey` emits),
|
||||
so this is _not_ a literal dup, but the formatter constant could
|
||||
still be reused. The W4 phase report names "seven status helpers" —
|
||||
this eighth site escaped.
|
||||
- Fix (Medium, mergeable next wave): export `BERLIN_DAY_FORMATTER`
|
||||
itself from `tz/resolver.ts` and have `bucket-series.ts` consume it
|
||||
for its own `toBerlinYmd`. Saves the duplicate `Intl.DateTimeFormat`
|
||||
allocation + keeps every Berlin-day surface keyed off one canonical
|
||||
formatter instance.
|
||||
|
||||
### M-2 — `medicationIntakeList` packs a plain params object at index 4
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/query-keys.ts:107` returns
|
||||
`["medications", id, "intake", "list", params]` where `params` is
|
||||
the same `{sortBy, sortDir, limit, offset, status}` object reference
|
||||
the caller passed.
|
||||
- TanStack Query hashes by structural deep-equal, so the cache key is
|
||||
stable across re-renders **as long as the caller passes a stable
|
||||
reference or always rebuilds the object with the same field order**.
|
||||
The factory's own `chartData` entry (line 286) decomposes the params
|
||||
into a flat tuple precisely to avoid this risk; `adminAuditLogFiltered`
|
||||
(line 193) does the same.
|
||||
- Inconsistency: two entries flatten, one entry packs an object. Either
|
||||
is correct in isolation, but the mixed pattern increases the chance a
|
||||
future refactor accidentally breaks deep-equal hashing on the packed
|
||||
entry (e.g. by reordering keys or adding a transient field). The
|
||||
pinned test at `query-keys.test.ts:133` only asserts the first four
|
||||
positions, not the index-4 shape.
|
||||
- Fix (Medium): either flatten `medicationIntakeList` to match the
|
||||
rest, or document the deep-equal contract on the factory entry and
|
||||
pin index-4 shape in the test.
|
||||
|
||||
### M-3 — Withings sync-activity / sync-sleep catch-blocks still consult the regex helper
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/withings/sync-activity.ts` and
|
||||
`/Users/marc/Projects/HealthLog/src/lib/withings/sync-sleep.ts` now throw
|
||||
typed `WithingsApiError` (good), but their catch-blocks still call
|
||||
`extractWithingsStatus(message)` / `isWithingsRefreshReauthFailure`
|
||||
rather than `classifyError(err)`.
|
||||
- Phase report W6 documents this as deferred to v1.4.43 (under
|
||||
"Sync-activity / sync-sleep catch-block migration"). Works today via
|
||||
the regex fallback inside `classifyError`, so this is **not a
|
||||
correctness defect** — it just leaves the migration partial.
|
||||
- Fix already scheduled — Medium severity for traceability only; no
|
||||
v1.4.42 action required.
|
||||
|
||||
### M-4 — `audit.dashboard.widgets.validation-failed` audit ledger row carries no rate-limit guard
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/widgets/route.ts:147`
|
||||
best-effort writes one audit row per 422. A poorly-behaved iOS client
|
||||
in a tight retry loop could write thousands of audit rows in a minute.
|
||||
The widgets PUT does not appear to be rate-limited (no
|
||||
`checkRateLimit` call in scope) — the audit table grows fast on a
|
||||
client bug.
|
||||
- Fix (Medium, defer to v1.4.43): either gate audit-row writes behind a
|
||||
60 s `(userId, action)` dedup, or add `checkRateLimit` to the widgets
|
||||
PUT itself. The widgets PUT is operator-facing settings traffic so
|
||||
the threshold can be generous (e.g. 30/min).
|
||||
|
||||
## Low (defer to v1.4.43)
|
||||
|
||||
### L-1 — `parseEnvFile` in `check-env.ts` does not handle dotenv backslash escapes
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/scripts/check-env.ts:83` strips
|
||||
surrounding `"`/`'` quotes but does not unescape `\n`, `\t`, etc.
|
||||
inside double-quoted values. A `APNS_KEY="-----BEGIN\nPRIVATE..."`
|
||||
literal would be classified as `[OK]` but read as the raw escape
|
||||
sequence, not a newline.
|
||||
- Coolify exports without backslash escapes today, so the regression
|
||||
is hypothetical. Documented constraint on the helper would be
|
||||
enough.
|
||||
|
||||
### L-2 — `check-env.ts --file` with no path argument silently falls back to `process.env`
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/scripts/check-env.ts:218`:
|
||||
`if (fileFlagIdx !== -1 && args[fileFlagIdx + 1])`. If the operator
|
||||
passes `pnpm check-env --file` (forgot the path), the script reads
|
||||
`process.env` instead of erroring.
|
||||
- Fix (Low): when `--file` is present but the next arg is missing,
|
||||
exit with code 2 + a "missing path" message.
|
||||
|
||||
### L-3 — Workouts canonical-row picker uses `<=` for the `±90 s` window — boundary inclusive
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/workouts/canonical-rows.ts:213`
|
||||
uses `Math.abs(...) <= WORKOUT_DEDUP_WINDOW_MS`. Documented contract
|
||||
in the JSDoc is `±90 s` — inclusive. Test
|
||||
(`canonical-rows.test.ts`) pins the inclusivity.
|
||||
- No defect; flagged Low because a future tightening to `<` would
|
||||
break the pinned test silently if the test only checks "two rows at
|
||||
exactly 90 s apart collapse to one survivor" without also checking
|
||||
"two rows at 91 s apart stay distinct". Both directions are pinned
|
||||
in the test file under "±90 s boundary inclusivity" so this is
|
||||
defended.
|
||||
|
||||
### L-4 — `WithingsApiError` swallows `body.error` only when it's a string
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/withings/client.ts:133` (and
|
||||
the parallel sites in `refreshAccessToken`, `fetchMeasurements`,
|
||||
`subscribeWebhook`, `unsubscribeWebhook`, `sync-activity.ts`,
|
||||
`sync-sleep.ts`) read `typeof json?.error === "string" ? json.error : undefined`.
|
||||
- If Withings ever returns `body.error` as an object (some endpoints
|
||||
do), the `upstreamError` slot stays empty and the audit-log line
|
||||
carries just `Withings <verb> error: <status>` without context.
|
||||
- Low risk because every documented Withings error response uses a
|
||||
string `error`. Could be tightened by stringifying via JSON.stringify
|
||||
when not a string.
|
||||
|
||||
## Strengths
|
||||
|
||||
- **`returnAllZodIssues` envelope** (`src/lib/api-response.ts:47`) is
|
||||
additive, privacy-conscious (drops `issue.params`), and the
|
||||
companion `sanitiseZodIssues` correctly factors out the projection
|
||||
so the widgets-route's audit ledger row never echoes raw user input.
|
||||
7-case unit test + 4-case route test + 3-case integration test —
|
||||
the contract is over-pinned.
|
||||
|
||||
- **Withings classifier** (`src/lib/withings/response-classifier.ts`)
|
||||
walks the three failure shapes (HTTP 5xx, HTTP 200 + non-zero
|
||||
status, HTTP 200 + body.status === 0 + empty groups) in the
|
||||
right order, defends against the literal 293/294 PERSISTENT_CODES
|
||||
precedence over the 200..299 reauth range, and the
|
||||
`subscribeWebhook` 294 idempotency downgrade lives at the
|
||||
call-site (where it belongs) rather than mutating the classifier
|
||||
verdict. 27 + 8 + 2 new tests across three files.
|
||||
|
||||
- **Workouts write-time canonical-row picker**
|
||||
(`src/lib/workouts/canonical-rows.ts`) shares the
|
||||
`DEFAULT_WORKOUT_SOURCE_PRIORITY` ladder with the read-time picker
|
||||
(single source of truth, no duplication), is a pure function with
|
||||
zero IO, and the 12-case test covers the source ladder, the
|
||||
±90 s boundary inclusivity, the calories tie-breaker, the
|
||||
createdAt tie-breaker, and the no-mutation invariant. The
|
||||
comparator's "annotated copy" trick at line 241 preserves the
|
||||
caller's row reference exactly — survivors carry through to the
|
||||
Prisma layer untouched.
|
||||
|
||||
- **`pickCanonicalWorkoutRows` integration into the batch route**
|
||||
(`src/app/api/workouts/batch/route.ts:273-340`) correctly hoists
|
||||
dedup BEFORE the existing externalId-based pre-flight, surfaces
|
||||
dropped twins as `duplicate` so iOS sync cursors advance
|
||||
identically, and the race-reconcile loop at line 513 was updated
|
||||
to walk `survivors` rather than `prepared` so write-dedup losers
|
||||
aren't double-counted toward the race-reconcile budget. Smart catch
|
||||
the phase report called out.
|
||||
|
||||
- **queryKey factory long-tail migration**
|
||||
(43 files + 9 factory entries + ESLint allowlist mirror + test-guard
|
||||
expansion) is methodical: every new entry has a JSDoc rationale,
|
||||
every consumer uses `setQueryData(queryKeys.X(), …)` symmetric with
|
||||
the read query, and the `ai-section.tsx` eight-call invalidation
|
||||
collapse via `replace_all` to `queryKeys.insightsRoot()` is a clean
|
||||
refactor. ESLint rule + test-guard regex both updated so the new
|
||||
scope can't drift back to bare literals.
|
||||
|
||||
- **Knip gate flip** (`.github/workflows/knip.yml:31` and `knip.json`)
|
||||
promotes exports + types tiers to enforcing with a precisely-scoped
|
||||
ignore-block (shadcn UI surface for library parity, validations for
|
||||
the iOS contract). Zero unused exports + zero unused types on the
|
||||
final commit. The reconcile-callout pattern — leave cross-wave dead
|
||||
re-exports to a follow-up cleanup commit (`dce14fb4`) before the
|
||||
gate flip — is the right merge-order discipline.
|
||||
|
||||
- **BERLIN_DAY_FORMATTER dedup** consolidates 7 × 20-LOC blocks into
|
||||
one `tz/resolver.ts` export with a JSDoc that correctly distinguishes
|
||||
Berlin-anchored cache keys from per-user `userDayKey(date, tz)`.
|
||||
Net −101 LOC and the test guard at
|
||||
`dashboard-suspense-boundaries.test.ts` confirms no behaviour drift.
|
||||
|
||||
- **doctor-report-data.ts control-byte fix** turns a "Binary files
|
||||
differ" diff into "Java source, Unicode text, UTF-8 text". Two
|
||||
characters changed, twelve months of diff-readability regained.
|
||||
|
||||
- **`offhost-backup.ts` DR-intent comment** documents the asymmetry
|
||||
between the disaster-recovery snapshot (includes soft-deleted) and
|
||||
the user-facing export (excludes soft-deleted) at the
|
||||
`prisma.measurement.findMany` call site, with a backreference to
|
||||
the symmetric exclusion in `/api/export/full-backup/route.ts`. The
|
||||
exact comment a future reviewer will need.
|
||||
|
||||
- **`pr-detection-worker.ts` soft-delete filter** is a two-line
|
||||
surgical fix (`deletedAt: null` added to two `where` clauses) that
|
||||
closes a real edge case (a deleted PR blocking promotion of the
|
||||
next-best row). 17 existing test cases still pass; no new tests
|
||||
needed because the existing soft-delete coverage already exercises
|
||||
the path.
|
||||
|
||||
- **Persistent classification** in `withings/sync.ts` +
|
||||
`integrations/status.ts` correctly maps `persistent` → state
|
||||
`error_transient` (so the next sync still runs) but carries
|
||||
`kind: "persistent"` into the audit row + admin-alert payload with
|
||||
a distinct "persistent error" label and "investigate the upstream
|
||||
contract" action line. Operators get the granular signal without
|
||||
the integration getting stuck on a one-off contract bug burst.
|
||||
|
||||
- **No commit drift** — every commit message in the v1.4.42 range is
|
||||
Marc-voice English, conventional-commit prefixed, no
|
||||
`Co-Authored-By: Claude`, no `--no-verify`, no `--no-gpg-sign`. The
|
||||
worktree-isolation hard rule
|
||||
(`feedback_marathon_worktree_isolation.md`) held across six parallel
|
||||
agents — fifth marathon in a row with this discipline now operational.
|
||||
@@ -0,0 +1,106 @@
|
||||
# v1.4.42 Design-review findings
|
||||
|
||||
## Verdict
|
||||
APPROVE
|
||||
|
||||
## Critical (visible regression — CLS / contrast / a11y break)
|
||||
None.
|
||||
|
||||
## High (placeholder mismatch, focus order, motion)
|
||||
None.
|
||||
|
||||
## Medium
|
||||
None.
|
||||
|
||||
## Low
|
||||
None.
|
||||
|
||||
## Strengths
|
||||
|
||||
- **Tile-strip placeholder now byte-for-byte matches the live `TrendCard` chrome.**
|
||||
`src/app/page.tsx:1416` — fallback className is
|
||||
`bg-card border-border flex h-full min-h-[6rem] w-full min-w-0 flex-col rounded-xl border p-4 md:p-6`.
|
||||
Compared against the live tile container at
|
||||
`src/components/charts/trend-card.tsx:241`
|
||||
(`bg-card border-border flex h-full w-full min-w-0 flex-col rounded-xl border p-4 md:p-6`),
|
||||
the placeholder is the same class set plus the explicit
|
||||
`min-h-[6rem]` for the all-suspend edge. Closes v1.4.41 L1 + L2
|
||||
follow-ups in one motion. No `gap-*` / `space-*` / `items-*` drift
|
||||
on the live tile — the children-positioning chrome (`flex
|
||||
min-w-0 flex-col`) is the entire layout contract on the
|
||||
`TrendCard` root.
|
||||
|
||||
- **`aria-hidden="true"` retained and still correct.** The placeholder
|
||||
is a self-closing `<div />` — no descendants exist that could be
|
||||
focusable, so adding `flex min-w-0 flex-col` cannot accidentally
|
||||
surface a tab stop. Focus order is unaffected.
|
||||
|
||||
- **Suspense double-comment consolidation is documentation-only.**
|
||||
`src/app/page.tsx:1402-1411` — the v1.4.40 W-RSC + v1.4.41
|
||||
W-FRONTEND-FACTORY block pair collapsed into one 9-line block with
|
||||
a single-line history trailer. JSX structure is byte-identical:
|
||||
the `<Suspense fallback={…}>{entry.node}</Suspense>` markup, the
|
||||
`<div key={entry.id} className="flex min-w-0">` wrapper, and the
|
||||
`trendCards.map` outer scaffold all stayed in place. The
|
||||
structural Suspense test pin
|
||||
(`src/app/__tests__/dashboard-suspense-boundaries.test.ts`) keys
|
||||
on the `aria-hidden="true"` marker and the `<Suspense>` count, not
|
||||
on comment text — stays green.
|
||||
|
||||
- **queryKey factory long-tail migration (`e2018b2d`) is a pure
|
||||
refactor across 40 files.** Every change in
|
||||
`src/components/{settings,medications,admin}`, `src/hooks/`, and
|
||||
three `src/app/medications` / `src/app/targets` files follows the
|
||||
same pattern: `queryKey: ["literal", …]` → `queryKey:
|
||||
queryKeys.<entry>(…)`. Spot-checked
|
||||
`src/components/settings/ai-section.tsx`,
|
||||
`src/components/settings/integrations-section.tsx`,
|
||||
`src/components/medications/medication-card.tsx`,
|
||||
`src/components/medications/glp1-medication-card.tsx`,
|
||||
`src/components/medications/intake-history-list-v2.tsx`,
|
||||
`src/components/admin/login-overview-section.tsx`,
|
||||
`src/hooks/use-workouts.ts` — every diff is a queryKey / invalidate
|
||||
call-site rewrite. No queryFn body, no JSX, no className, no copy,
|
||||
no enabled-gate, no select shape, no staleTime changed. Design
|
||||
surface is byte-identical.
|
||||
|
||||
- **Factory-entry shape parity is test-pinned.**
|
||||
`src/lib/__tests__/query-keys.test.ts:104-167` asserts the literal
|
||||
array shape for every new entry — `medicationCompliance` ↔
|
||||
`["medications", id, "compliance"]`, `withingsStatus` ↔
|
||||
`["withings", "status"]`, `adminAuditLogFiltered` ↔ 10-element
|
||||
array with `(filter, page, perPage, actor, actionFilter, target,
|
||||
range)` in the same positional order the prior bare-literal used.
|
||||
`workoutsRecentList(opts)` returns `["workouts", "recent", opts]`,
|
||||
matching the prior `[...queryKeys.workoutsRecent(), opts]` shape.
|
||||
Cache-slot identity preserved → no silent eviction on first paint
|
||||
after deploy.
|
||||
|
||||
- **iOS Workouts dedup is a write-side-only change.** The new
|
||||
`pickCanonicalWorkoutRows()` runs pre-`createMany` inside
|
||||
`src/app/api/workouts/batch/route.ts:255-303`. Losers surface to
|
||||
iOS as `{ index, status: "duplicate" }` — the exact per-entry
|
||||
envelope shape pinned by
|
||||
`src/app/api/workouts/__tests__/batch-create.test.ts`. The three
|
||||
pre-existing envelope tests were updated to space their two-row
|
||||
payloads outside the new 90 s dedup window so they continue to
|
||||
exercise envelope semantics rather than collide with the W5 pass.
|
||||
iOS UI is not touched.
|
||||
|
||||
- **Doctor-report PDF rendering path is unaffected by the regex
|
||||
rewrite.** `src/lib/doctor-report-data.ts:171` swaps a control-byte
|
||||
character class for the equivalent `[\x00-\x1F\x7F]`. Same regex
|
||||
semantics (NUL through US + DEL); the file just becomes UTF-8
|
||||
source rather than a binary blob. `sanitisePracticeName()` is
|
||||
consumed by `src/app/api/doctor-report/route.ts` and
|
||||
`src/app/api/doctor-report/pdf/route.ts` — the PDF renderer
|
||||
downstream sees identical bytes for any practice-name input.
|
||||
|
||||
- **No bare-literal queryKey leaks remain in the newly-guarded
|
||||
surface.** `eslint-plugins/healthlog/queryKey-factory.js` extended
|
||||
`GUARDED_DIRECTORIES` to cover the four migrated trees plus three
|
||||
named files; the lint rule + the in-repo factory-walker test
|
||||
(`src/lib/__tests__/query-keys.test.ts:263-290`) ensure a future
|
||||
PR introducing a bare-literal in `settings/`, `medications/`,
|
||||
`admin/`, or `src/hooks/` lands red before merge. Design tokens
|
||||
for cache invalidation are now a single-source-of-truth concern.
|
||||
@@ -0,0 +1,491 @@
|
||||
# v1.4.42 Product-lead strategic findings
|
||||
|
||||
Reviewer: Product Lead (read-only, strategic).
|
||||
Inputs: 6 v1.4.42 phase reports (W1-KNIP, W2-ZOD-MULTI-ISSUE,
|
||||
W3-QUERYKEY-LONGTAIL, W4-ARCH-HYGIENE, W5-IOS-WORKOUTS-DEDUP +
|
||||
DESIGN-POLISH, W6-WITHINGS-OFF + ENV-CHECK), the v1.4.41 product-lead
|
||||
backlog seed (B1-B10), `CHANGELOG.md` for v1.4.40 + v1.4.41 baseline,
|
||||
`v15-strategic-plan.md`, `.planning/v15-ios-handoff/03-api-contracts.md`
|
||||
+ `08-locked-contracts.md`, `git log 67207d72..develop` (22 commits on
|
||||
`develop` since the v1.4.41 release tag).
|
||||
|
||||
## Verdict
|
||||
|
||||
**APPROVE_WITH_FIXES** — code, contracts, and test posture are
|
||||
tag-ready. The same three release-endgame items that gated v1.4.41
|
||||
gate v1.4.42 (version bump, CHANGELOG entry, post-deploy verify
|
||||
plan). They are mechanical, not code-shape; the handoff-to-next-
|
||||
session pattern that worked for v1.4.41 applies cleanly here. None
|
||||
of the six waves introduces iOS contract drift; the v1.5 Swift sprint
|
||||
remains day-1-startable on top of the v1.4.42 deploy.
|
||||
|
||||
## Strategic posture
|
||||
|
||||
v1.4.42 is the **backlog-close polish release** on top of v1.4.41's
|
||||
iOS-handoff polish release. The release shape is even tighter than
|
||||
v1.4.41 (22 commits vs 27 vs v1.4.40's 54), and the changes cluster
|
||||
around three coherent themes:
|
||||
|
||||
1. **The v1.4.41 backlog seed (B1-B10) closes cleanly.** Six of the
|
||||
ten backlog items land in this release as discrete waves; three
|
||||
are confirmed already-done (no-op via grep verification in W4);
|
||||
one is correctly deferred to v1.4.43 (B10 worktree-isolation —
|
||||
the marathon was already mid-flight). This is the cleanest
|
||||
backlog-to-next-release conversion of the v1.4.39 / v1.4.40 /
|
||||
v1.4.41 sequence so far.
|
||||
|
||||
2. **The audit-ledger breadcrumb pattern is established.** W2's
|
||||
`/api/dashboard/widgets` PUT 422 now writes a
|
||||
`dashboard.widgets.validation-failed` audit row alongside the
|
||||
422 response. This is the structural foundation for moving 422
|
||||
diagnostics beyond the iOS dev console — when the v1.4.43 wave
|
||||
rolls the helper across the 41 remaining iOS-contract routes,
|
||||
every shape mismatch the iOS team encounters will have a
|
||||
permanent ledger breadcrumb without needing a separate logging
|
||||
ingest.
|
||||
|
||||
3. **iOS contract surface tightens without breaking.** W5's
|
||||
`pickCanonicalWorkoutRows()` lands the cross-source dedup helper
|
||||
that R-F open question #4 in the v1.5 plan anticipated; the iOS
|
||||
Workouts ingest path now drops duplicate Apple Watch × Withings
|
||||
ScanWatch rows server-side rather than relying on the iOS client
|
||||
to pre-merge. The dedup writes through the existing `duplicate`
|
||||
per-entry status enum so iOS clients that ride the v1.4.25
|
||||
workouts envelope keep working byte-identically.
|
||||
|
||||
The supporting waves (W1 knip gate flip, W3 long-tail factory
|
||||
migration, W4 tree-hygiene 5 items, W6 Withings off-response
|
||||
classification + pre-deploy env-check CLI) all close v1.4.41
|
||||
carry-over items. **Zero new feature surface beyond W5's helper.
|
||||
Zero new API contract beyond W2's additive `details.issues` field
|
||||
on one route. Zero new endpoint.** No env-var change. No Prisma
|
||||
migration. Same patch-bump-correctness posture as v1.4.41.
|
||||
|
||||
The release stays inside the conservative-semver doctrine
|
||||
(`feedback_semver_conservative.md`) — additive only, zero contract
|
||||
breaks, **v1.4.42 patch bump** is the correct call. v1.5 remains
|
||||
reserved for the iOS Swift sprint tag.
|
||||
|
||||
## Critical (must address before tag)
|
||||
|
||||
**None.** All shipped contracts hold; W1's knip gate is now
|
||||
enforcing, so no future push to main can sneak a dead export past
|
||||
the CI; W2's envelope is strictly additive and the iOS handoff doc
|
||||
at `03-api-contracts.md:1044` ("Surface the first issue from `error`
|
||||
— Zod messages are user-readable") keeps working byte-identically
|
||||
since `error` still carries the human-readable string. W3's factory
|
||||
migration is mechanical — 43 files, all guarded by both the ESLint
|
||||
rule and the test-walker so future regressions trip immediately.
|
||||
W4's tree-hygiene is touch-disjoint with the perf surface. W5's
|
||||
workouts dedup is write-time only and re-uses the existing
|
||||
`duplicate` per-entry status. W6's classification preserves the
|
||||
legacy regex consumers via fallback. Nothing blocks the tag from a
|
||||
code-shape perspective.
|
||||
|
||||
## High (should address before tag)
|
||||
|
||||
- **H1 — Version bump + CHANGELOG entry for v1.4.42 is not in
|
||||
develop tip.** `package.json` still reports `1.4.41`. CHANGELOG.md
|
||||
has no `[1.4.42]` heading. Same shape as v1.4.41's H1 — the
|
||||
marathon dispatch pattern lands the version bump + CHANGELOG entry
|
||||
in the release-tagging commit alongside the squash-to-main. The
|
||||
CHANGELOG should call out:
|
||||
- **W1 knip gate flip** as the structural headline (35 dead
|
||||
exports → 0, 52 dead types → 0, CI enforcing on every push to
|
||||
main).
|
||||
- **W2 multi-issue 422 helper** + audit-ledger breadcrumb on
|
||||
`/api/dashboard/widgets` as the iOS contract diagnostic
|
||||
improvement (closes the v1.4.41 H4 carry-over).
|
||||
- **W3 long-tail factory migration** (43 files, settings +
|
||||
medications + admin + hooks now factory-only, ESLint guarded).
|
||||
- **W4 tree-hygiene** (BERLIN_DAY_FORMATTER 7-way dedup, Suspense
|
||||
double-comment, doctor-report binary-blob fix, pr-detection
|
||||
soft-delete filter, offhost-backup DR-intent comment).
|
||||
- **W5 workouts cross-source dedup helper** (anticipates the iOS
|
||||
Workouts ingest sprint) + tile-strip placeholder polish.
|
||||
- **W6 Withings off-response classification** + `pnpm check-env`
|
||||
CLI as the iOS-perf / ops-hardening pair.
|
||||
Marc-voice / English / no PII per the established doctrine. The
|
||||
release notes should explicitly state that v1.4.42 is the
|
||||
**backlog-close polish release** on top of the v1.4.41
|
||||
iOS-handoff polish release, with the iOS Swift sprint freeze
|
||||
marker now confirmed as v1.4.42.
|
||||
|
||||
- **H2 — `pnpm check-env` should run on the v1.4.42 deploy itself.**
|
||||
W6's manifest covers Core / Withings OAuth / APNs / Deploy webhook
|
||||
/ Off-host backups. Running it pre-deploy against the live Coolify
|
||||
secret store would (a) catch any v1.4.40 AP-2 / .p8 regression
|
||||
before tagging, (b) prove the manifest's `anyOf [APNS_KEY,
|
||||
APNS_KEY_FILE]` rule against the real environment, (c) provide a
|
||||
baseline for the v1.4.43 CI integration. Cheap (one `pnpm
|
||||
check-env` invocation against the Coolify env-var dump) and exactly
|
||||
the use case the wave was designed for. **Mitigation if missed:**
|
||||
v1.4.40-shape regression slips again; the v1.4.43 CI integration
|
||||
catches it but only after the next release ships. Run it once
|
||||
pre-tag.
|
||||
|
||||
- **H3 — Post-deploy verify plan should pin the rollout-coverage
|
||||
invariant for the knip CI gate.** The gate is now enforcing on
|
||||
every push to main. The W1 report notes the `develop` tip is
|
||||
green with the documented ignore block; the very first push to
|
||||
main after the v1.4.42 tag will exercise the gate. Recommended
|
||||
path: run a dry-run `pnpm knip --reporter compact` on the
|
||||
squash-to-main commit before pushing; if it returns non-zero
|
||||
due to any reconciliation drift between W1 + W3 + W4 (the W1
|
||||
report explicitly calls out a "merge order" risk for the two
|
||||
re-exports `describeInjectionSite` + `listSupportedTimezones`),
|
||||
fix the residual drop before the push lands. The W1 report
|
||||
recommends "W1 last" as the merge order; the actual landed
|
||||
order was `876a545d` (W2) → `1ea0321d` + earlier (W4) →
|
||||
`e2018b2d` (W3) → `dce14fb4` + later (W1) so this should be
|
||||
green — verify once before tagging.
|
||||
|
||||
## Medium (next-version backlog candidates)
|
||||
|
||||
- **M1 — 41-route multi-issue 422 rollout.** W2's report enumerates
|
||||
every site at exact `file:line`. This is the v1.4.43 headline
|
||||
wave: take the helper W2 shipped and roll it across the 41
|
||||
remaining `parsed.error.issues[0].message` call sites, prioritised
|
||||
by iOS-contract proximity (measurements / medications / mood
|
||||
intake at the top). Bound to ~half a day of mechanical work;
|
||||
rolls in the audit-ledger breadcrumb on the iOS hot paths.
|
||||
|
||||
- **M2 — Withings sync-activity / sync-sleep catch-block migration
|
||||
to `err.classification` direct read.** Working today via the
|
||||
regex fallback in `classifyError`; the migration is code-cleanup
|
||||
only. Slot into v1.4.43 alongside M1 (both are mechanical) or
|
||||
defer to v1.4.44.
|
||||
|
||||
- **M3 — `parkIntegrationAtReauth` extension to cover persistent
|
||||
Withings failures > 24 h.** Today persistent failures stay at
|
||||
`state=error_transient` so the next sync runs; the 3-strike
|
||||
admin alert catches the burst either way. The actual behaviour
|
||||
Marc would want is: after 24 h of persistent failures, the
|
||||
integration parks at "reauth_required" so the user gets the
|
||||
in-app alert. Cheap (~30 LOC); v1.4.43 candidate.
|
||||
|
||||
- **M4 — CI integration for `pnpm check-env`.** W6 documents the
|
||||
skeleton; ship the GH Actions workflow + Coolify pre-deploy
|
||||
hook. Closes the operator-side gap that AP-2 originally
|
||||
exposed. v1.4.43 candidate.
|
||||
|
||||
- **M5 — `eslint-plugin-healthlog/queryKey-factory` expansion to
|
||||
remaining tree.** W3 closes the v1.4.41 M2 backlog item but
|
||||
the rule's `GUARDED_DIRECTORIES` still excludes `src/components/
|
||||
integrations/`, `src/app/**` (except the three specific files),
|
||||
and `src/components/insights/**`. Walk the remainder, add to
|
||||
the guarded list one chunk at a time. No urgency; pure surface
|
||||
cleanup. v1.4.44+ candidate.
|
||||
|
||||
- **M6 — Cross-tz proper-fix still v1.5.** Unchanged from
|
||||
v1.4.40 / v1.4.41. Marc's tenant is Berlin; runtime guard +
|
||||
live SQL fallback covers non-near-UTC tenants. Defers cleanly.
|
||||
|
||||
- **M7 — `pickCanonicalWorkoutRows()` ladder calibration.** W5's
|
||||
helper hardcodes the ladder `APPLE_HEALTH > WITHINGS > MANUAL >
|
||||
IMPORT`. Once iOS Workouts ingest goes live in v1.5 and Marc
|
||||
has real-world Apple Watch × Withings ScanWatch overlap data,
|
||||
the ladder order may need rebalancing (e.g. if Apple Watch
|
||||
routinely under-reports calories vs Withings). Surface as a
|
||||
v1.5.1 calibration item, not a v1.4.43 blocker.
|
||||
|
||||
## Low (defer)
|
||||
|
||||
- **L1 — knip exit-code semantics.** W1's gate flip means a future
|
||||
push to main carrying any new unused export trips CI red. The
|
||||
failure message points at the file + symbol but does not link
|
||||
to the `knip.json` ignore-block syntax. Future-paper-cut only;
|
||||
add a one-line README pointer in the next docs refresh.
|
||||
|
||||
- **L2 — JSDoc-block queryKey example formatting.** W3 reworded
|
||||
the `use-insights-analytics.ts` example to dodge the test-guard's
|
||||
line-comment-only regex. Test-guard could grow JSDoc-block regex
|
||||
support so the documentation intent doesn't need rewording. Pure
|
||||
hygiene; defer.
|
||||
|
||||
- **L3 — `WithingsApiError` prototype recovery in pg-boss serialised
|
||||
retries.** W6 covers this via the regex fallback in `classifyError`.
|
||||
Future-proofing concern: if pg-boss ever changes its serialisation
|
||||
format, the regex fallback could fail silently. Add a typed
|
||||
`serializedError` shape to the queue payload as a v1.5+ ops hardening
|
||||
item.
|
||||
|
||||
- **L4 — `workoutsRecentList` factory composite vs spread-and-append.**
|
||||
W3 replaced `[...queryKeys.workoutsRecent(), opts]` with a single
|
||||
factory entry. Future-paper-cut only: the composite pattern is the
|
||||
more correct factory shape; if any new code reaches for the
|
||||
spread-and-append shape it should be caught by ESLint immediately.
|
||||
No action.
|
||||
|
||||
## v1.4.43 backlog seed
|
||||
|
||||
- **C1 — 41-route multi-issue 422 rollout** (per M1). Take W2's
|
||||
helper across the iOS-contract hot paths (measurements / medications
|
||||
/ mood intake / dashboard chart-overlay-prefs / integrations
|
||||
HealthKit / ingest medication / tokens / devices) first. Med-CRUD
|
||||
+ auth/settings/admin/feedback/consent second. ~half a day of
|
||||
mechanical work; rolls in the audit-ledger breadcrumb pattern on
|
||||
every iOS hot path.
|
||||
|
||||
- **C2 — Withings classification consumer migration** (per M2).
|
||||
Drop the regex fallback in `sync-activity.ts` + `sync-sleep.ts`
|
||||
catch blocks; read `err.classification` directly.
|
||||
|
||||
- **C3 — `parkIntegrationAtReauth` for > 24 h persistent Withings
|
||||
failures** (per M3).
|
||||
|
||||
- **C4 — `pnpm check-env` CI integration** (per M4). GH Actions
|
||||
workflow against `.env.production.example`; Coolify pre-deploy
|
||||
hook against the live container.
|
||||
|
||||
- **C5 — Worktree isolation hard rule enforcement.** v1.4.42 was
|
||||
the second marathon since the rule landed in
|
||||
`~/.claude/skills/release-marathon/SKILL.md`. The W4 report shows
|
||||
one wave (`worktree-agent-ae49fe4059ee48d37`) ran without
|
||||
collision against W3 (`worktree-agent-a11813dd22a55cbf7`); the
|
||||
rule held. Future marathons must continue applying it. No action
|
||||
in v1.4.43 beyond "keep applying."
|
||||
|
||||
- **C6 — `eslint-plugin-healthlog/queryKey-factory` expansion**
|
||||
to remaining unguarded surfaces (per M5).
|
||||
|
||||
- **C7 — Long-window summary helper consumer (if missing).** W4
|
||||
reconfirmed that `computeLongWindowSummary` is already gone from
|
||||
the codebase (no matches). Carry-over item is closed. Drop the
|
||||
carry-over flag from the v1.4.43 backlog template.
|
||||
|
||||
- **C8 — `ensureUserMedicationComplianceFresh` decision.** Same
|
||||
shape as C7 — W4 reconfirmed already-gone. Drop from carry-over.
|
||||
|
||||
- **C9 — `pickCanonicalWorkoutRows()` ladder real-world
|
||||
calibration** (per M7) once iOS Workouts ingest deploys.
|
||||
|
||||
- **C10 — Tile-strip placeholder min-h verify on settled v1.5 RSC
|
||||
migration.** W5 added `min-h-[6rem]` matching live trend-card
|
||||
chrome. Once the RSC migration starts streaming tiles in v1.5+,
|
||||
verify that the placeholder holds the row open during real
|
||||
suspension (not just synthetic). No action in v1.4.43.
|
||||
|
||||
## v1.4.41 backlog (B1-B10) closure check
|
||||
|
||||
| # | Item | Status |
|
||||
| --- | --- | --- |
|
||||
| B1 | knip exports/types gate flip + ignore-block triage | **Closed** (W1 — gate enforcing, 0 unused exports, 0 unused types, ignore-block documented) |
|
||||
| B2 | `/api/dashboard/widgets` Zod-issue aggregation | **Closed** (W2 — `returnAllZodIssues` helper + widgets PUT migrated + audit-ledger breadcrumb + 14 tests) |
|
||||
| B3 | Long-tail queryKey factory migration | **Closed** (W3 — 43 files migrated, ESLint allowlist expanded, factory + tests pinned) |
|
||||
| B4 | `ensureUserMedicationComplianceFresh` decision | **Closed** (W4 confirmed already-gone via grep) |
|
||||
| B5 | `computeLongWindowSummary` production consumer | **Closed** (W4 confirmed already-gone via grep) |
|
||||
| B6 | `pickCanonicalWorkoutRows()` cross-source dedup helper | **Closed** (W5 — helper + 12 test cases + wired at `/api/workouts/batch`) |
|
||||
| B7 | Dashboard 422 multi-issue audit ledger breadcrumb | **Closed** (W2 — `dashboard.widgets.validation-failed` audit row writes; survives DB-write rejection per test) |
|
||||
| B8 | Pre-deploy Coolify env-var sanity check | **Closed** (W6 — `pnpm check-env` CLI + manifest + 16 test cases + operator doc) |
|
||||
| B9 | Withings off-response wave | **Closed** (W6 — classifier + typed `WithingsApiError` + 37 new tests + 3-state FailureKind extension) |
|
||||
| B10 | `release-marathon` skill worktree-isolation enforcement | **Carried forward** (rule landed in v1.4.41 handoff; v1.4.42 was the second marathon to apply it; rule held, no commit drift reported) |
|
||||
|
||||
Items added beyond B1-B10:
|
||||
|
||||
- **W5 tile-strip placeholder polish** — design-findings L1 + L2
|
||||
from `round-v1441-QA-design-findings.md` (not in B1-B10), closed
|
||||
with `min-h-[6rem]` + chrome utility parity.
|
||||
|
||||
- **W4 BERLIN_DAY_FORMATTER 7-way dedup** + Suspense double-comment
|
||||
consolidate + doctor-report binary-blob escape + pr-detection
|
||||
soft-delete filter + offhost-backup DR-intent comment — none of
|
||||
these were in B1-B10. They are tree-hygiene wins surfaced by the
|
||||
W4 wave's own discovery; commendable scope discipline (no creep
|
||||
beyond what's mechanically cheap to close).
|
||||
|
||||
Items deferred:
|
||||
|
||||
- **B10** (worktree-isolation hard rule) — verified held this
|
||||
marathon; carries forward as ongoing process discipline, not as
|
||||
a v1.4.43 backlog item.
|
||||
|
||||
## v1.5 readiness check
|
||||
|
||||
**Green. The iOS Swift sprint can start on top of `develop` tip
|
||||
without backend drift.**
|
||||
|
||||
The v1.5 strategic plan dated 2026-05-16 (`v15-strategic-plan.md`)
|
||||
treats v1.5 web as a "version-bump-only marker." The iOS-handoff
|
||||
items the plan called out for v1.4.30 / v1.4.31 / v1.4.32 have
|
||||
already shipped under v1.4.39 / v1.4.40 / v1.4.41; the locked
|
||||
contracts in `08-locked-contracts.md` hold.
|
||||
|
||||
**iOS contract status against `.planning/v15-ios-handoff/`:**
|
||||
|
||||
- **§ Workouts (POST /api/workouts/batch)** — W5 adds write-time
|
||||
cross-source dedup but preserves the per-entry envelope: dropped
|
||||
twins surface as `duplicate`, exactly the existing `EntryStatus`
|
||||
enum the iOS contract pins at `03-api-contracts.md:481` ("Response
|
||||
identical to measurements batch — per-entry status, race
|
||||
reconciliation"). Idempotency invariant `(userId, source,
|
||||
externalId)` composite unique index unchanged. iOS-side caller
|
||||
behaviour: zero diff. ✓ Additive-safe.
|
||||
|
||||
- **422 envelope shape** — W2 helper's response carries:
|
||||
```json
|
||||
{
|
||||
"data": null,
|
||||
"error": "Validation failed",
|
||||
"details": { "issues": [{ "path", "code", "message" }, …] },
|
||||
"meta": { /* optional */ }
|
||||
}
|
||||
```
|
||||
iOS handoff at `03-api-contracts.md:1044` says "Surface the first
|
||||
issue from `error` (Zod messages are user-readable)". The `error`
|
||||
field still carries a human-readable string. New iOS builds can
|
||||
branch on `body.details?.issues` for multi-field validation feedback
|
||||
but do not have to. ✓ Additive-safe; the locked-contracts §
|
||||
Zod-strict rule (no `.passthrough`) is untouched.
|
||||
|
||||
- **Withings sync surface** — W6 changes the typed `Error` shape
|
||||
thrown by client entrypoints, but the dispatched sync state +
|
||||
audit-row + admin-alert shape stays additive (`kind: "persistent"`
|
||||
is a new label inside the existing audit row; legacy `transient
|
||||
/ reauth_required` consumers keep working). iOS does not interact
|
||||
with the Withings sync surface directly; the integration status
|
||||
endpoint (`/api/integrations/withings`) shape is preserved. ✓ No
|
||||
contract drift.
|
||||
|
||||
- **All other waves (W1 knip, W3 queryKey, W4 tree-hygiene)** —
|
||||
zero API surface. ✓
|
||||
|
||||
**Open items that gate iOS sprint kickoff:** none. Every required
|
||||
backend surface is live or documented. The iOS team can start Day 1
|
||||
of the strategic plan's Track A (Coach SSE drawer), Track B
|
||||
(SyncMode + Workouts ingest), Track C (daily-stats per
|
||||
`HKStatisticsCollectionQuery`) against the v1.4.42 deploy.
|
||||
|
||||
**Open items the iOS sprint will likely surface but are not
|
||||
blockers:**
|
||||
|
||||
- 41-route multi-issue 422 rollout (M1 / C1). iOS dev console
|
||||
still sees single-issue messages on the 41 sibling routes; per
|
||||
the locked iOS handoff rule iOS surfaces `error` text verbatim,
|
||||
so this is a debug-time improvement only.
|
||||
- `pickCanonicalWorkoutRows()` ladder calibration (M7 / C9) once
|
||||
Apple Watch + Withings ScanWatch overlap data lands. Iterate
|
||||
in v1.5.1.
|
||||
|
||||
**Web-freeze trigger update:** the v1.5 plan's freeze marker was
|
||||
"v1.4.34 tag on main". Actual cadence: v1.4.39 + v1.4.40 + v1.4.41
|
||||
+ v1.4.42 all shipped past that marker as iOS-handoff / polish
|
||||
releases. **Recommended:** the v1.4.42 release notes should
|
||||
explicitly state that v1.4.42 is the actual freeze marker. The
|
||||
`v15-strategic-plan.md` document should be appended (not rewritten)
|
||||
with a "Cadence reality" section that resets the freeze trigger
|
||||
to v1.4.42. v1.4.43 (the 41-route rollout) should be the first
|
||||
post-freeze emergency-style patch — small, mechanical, no new
|
||||
surface.
|
||||
|
||||
**Net assessment:** the iOS Swift sprint is unblocked. The
|
||||
strategic plan's day-by-day schedule can begin against `develop`
|
||||
tip the day after v1.4.42 deploys. Confidence: high — the v1.4.42
|
||||
wave-level discipline (no Prisma migration, no env-var change, no
|
||||
new endpoint beyond W2's additive 422 envelope, every wave a small
|
||||
atomic worktree) is exactly the pre-iOS-sprint shape the strategic
|
||||
plan calls for.
|
||||
|
||||
## Strengths
|
||||
|
||||
- **Backlog-to-next-release conversion ratio is the cleanest yet.**
|
||||
Six of ten B1-B10 items close in this single release as discrete
|
||||
waves (B1 / B2 / B3 / B6 / B7 / B8 / B9). Two more (B4 / B5) are
|
||||
confirmed already-done via grep verification — discipline that
|
||||
prevents stale backlog drift. One (B10) is correctly classified
|
||||
as ongoing process. v1.4.41 → v1.4.42 is the proof that the
|
||||
marathon-handoff-doc → backlog-seed → next-release pattern
|
||||
scales. The release-marathon skill should reference this as the
|
||||
canonical example.
|
||||
|
||||
- **W1 knip gate flip is the structural payoff of 4 marathons.**
|
||||
v1.4.39 introduced the CI workflow staged. v1.4.40 enabled
|
||||
exports tier. v1.4.41 reduced exports 48 → 35 and types kept
|
||||
at 52. v1.4.42 drives both tiers to zero and flips the gate to
|
||||
enforcing. From this push forward, every PR to main carries an
|
||||
invariant: no dead exports or types reach the production tree.
|
||||
The ignore-block discipline (one-line rationale per entry, scoped
|
||||
to shadcn surface + zod-`infer` external API contract) keeps the
|
||||
gate from forcing artificial deletions.
|
||||
|
||||
- **W2 audit-ledger breadcrumb is the right shape for iOS
|
||||
diagnostics.** Writing a `dashboard.widgets.validation-failed`
|
||||
audit row alongside the 422 response means every shape mismatch
|
||||
carries a permanent server-side trace. The audit-row write is
|
||||
fire-and-forget — the 422 response is the contract, the audit
|
||||
row is debugging. Test pins both the success path and the
|
||||
audit-row-write-rejection survival. This is exactly how
|
||||
ledger-vs-contract separation should work.
|
||||
|
||||
- **W3 factory migration scope is responsibly bounded.** 43 files
|
||||
across settings + medications + admin + hooks + app pages.
|
||||
Every site routes through `queryKeys.<entry>()`. ESLint allowlist
|
||||
+ test-walker `guardedRoots` updated in lockstep so future
|
||||
regressions trip immediately. The factory adds 9 new entries
|
||||
(medication-compliance / titration / cadence / glp1-details /
|
||||
intake-drug-level-chart / intake-list / withings-status /
|
||||
admin-audit-log-filtered / workouts-recent-list) with
|
||||
byte-stable shapes pinned by test. Conservative, mechanical,
|
||||
reversible.
|
||||
|
||||
- **W4 tree-hygiene closes 5 disjoint items in one wave.** The
|
||||
BERLIN_DAY_FORMATTER 7-way dedup (-101 / +32 LOC) is the
|
||||
highest-leverage item — every future tz formatter change touches
|
||||
one file instead of seven. The Suspense double-comment consolidate
|
||||
is pure documentation hygiene. The doctor-report binary-blob fix
|
||||
is a small but real readability win (git diff goes from "Binary
|
||||
files differ" to readable text). The pr-detection-worker
|
||||
soft-delete filter closes a small gap (deleted PR could block
|
||||
next-best promotion). The offhost-backup DR-intent comment
|
||||
documents the deliberate asymmetry. All atomic, all reversible,
|
||||
all in one wave.
|
||||
|
||||
- **W5 helper anticipates the iOS sprint without forcing
|
||||
premature optimisation.** `pickCanonicalWorkoutRows()` is the
|
||||
helper the v1.5 plan R-F open question #4 named. 12 test cases
|
||||
cover empty input / same-source dup / cross-source overlap /
|
||||
no-overlap pass-through / multi-user isolation / multi-activity
|
||||
isolation / 4 tie-breakers / boundary inclusivity / determinism
|
||||
/ mutation-safety. Re-uses `DEFAULT_WORKOUT_SOURCE_PRIORITY`
|
||||
from the read-time picker so both write-time + read-time consult
|
||||
a single ladder constant — exactly the kind of cross-axis
|
||||
consistency a future ladder calibration needs.
|
||||
|
||||
- **W6 closes the AP-2 silent-disable root cause.** v1.4.40 AP-2
|
||||
cost three days of "iOS push isn't working" because a Coolify
|
||||
migration dropped three of four APNs env vars and the app booted
|
||||
cleanly. W6's `pnpm check-env` would have caught that pre-deploy.
|
||||
The manifest's `allOrNone` group catches the partial-set pattern
|
||||
exactly; the `anyOf [APNS_KEY, APNS_KEY_FILE]` rule handles the
|
||||
12-factor / filesystem variant cleanly. The classification of
|
||||
Withings off-responses into transient / reauth / persistent
|
||||
closes the long-standing "is this contract-breaking or upstream-
|
||||
outage" diagnostic ambiguity.
|
||||
|
||||
- **Conservative-semver call holds: patch bump (1.4.42) is correct.**
|
||||
Zero contract breaks; one new helper (`returnAllZodIssues`)
|
||||
with one consumer route migrated; one new helper
|
||||
(`pickCanonicalWorkoutRows`) wired into the existing workouts
|
||||
batch route; one new CLI (`pnpm check-env`) that does not affect
|
||||
runtime; structural refactors (knip flip, factory expansion,
|
||||
BERLIN_DAY_FORMATTER consolidation); Withings classification
|
||||
with full backward-compat fallback. Per
|
||||
`feedback_semver_conservative.md`, bugfix-heavy releases with
|
||||
additive-only features stay on patch. v1.5 stays reserved for
|
||||
iOS native launch.
|
||||
|
||||
- **The release is structurally smaller than v1.4.41 and that's
|
||||
the right shape.** v1.4.40 was 54 commits, v1.4.41 was 27,
|
||||
v1.4.42 is 22. Each release smaller, narrower, more reversible,
|
||||
closer to "the next polish wave" than "the next debt-close
|
||||
marathon." This is the exact cadence the v1.5 plan's freeze
|
||||
marker assumed; v1.4.42 should be the explicit freeze marker.
|
||||
|
||||
- **Six wave reports + one product-lead seed from the prior
|
||||
release — the audit trail keeps compounding.** A fresh session
|
||||
can pick up at the release endgame with full context in under
|
||||
an hour. The marathon handoff-doc → backlog-seed → wave-reports
|
||||
→ next-marathon loop has now operated cleanly across five
|
||||
consecutive marathons (v1.4.38 / v1.4.39 / v1.4.40 / v1.4.41 /
|
||||
v1.4.42). The operational discipline is what makes the
|
||||
multi-marathon cadence sustainable past v1.5.
|
||||
@@ -0,0 +1,97 @@
|
||||
# v1.4.42 Security-review findings
|
||||
|
||||
Scope: `git diff main..develop` (commits after `67207d72` — v1.4.42 vs the
|
||||
live v1.4.41 tag).
|
||||
Reviewer: W10 security-reviewer (read-only).
|
||||
|
||||
## Verdict
|
||||
APPROVE
|
||||
|
||||
Every focus area (returnAllZodIssues, Withings off-response classifier,
|
||||
write-time workout dedup, check-env CLI, knip ignore-block, re-export
|
||||
cleanup) passes. No Critical / High findings. Two Lows are noted as
|
||||
follow-ups for v1.4.43 — neither blocks the tag.
|
||||
|
||||
## Critical (must fix before tag — vuln / data leak / auth bypass)
|
||||
|
||||
None.
|
||||
|
||||
## High (should fix before tag — defence-in-depth gap, log leak, error swallow)
|
||||
|
||||
None.
|
||||
|
||||
## Medium (recommended for tag)
|
||||
|
||||
None.
|
||||
|
||||
## Low (defer to v1.4.43)
|
||||
|
||||
### L-1 `dispatchNotification` / `formatAdminAlertPayload` carry the raw Withings `json.error` string up to a 280-char Telegram body
|
||||
- **File**: `src/lib/integrations/status.ts:505-547` (untouched in v1.4.42)
|
||||
consumed by the new `WithingsApiError` path in `src/lib/withings/client.ts:127-135, 170-178, 290-298, 374-382`.
|
||||
- **Attack vector**: The new `upstreamError: typeof json?.error === "string" ? json.error : undefined`
|
||||
branch in every client entrypoint pipes Withings' own response-body
|
||||
`error` field into the thrown `WithingsApiError.message`. That message
|
||||
flows unchanged into:
|
||||
1. `AuditLog.details` via `auditLog("integrations.sync.failed", { details: { message, … } })` — **NOT** encrypted, **NOT** length-capped. Only the encrypted IntegrationStatus column is sliced to 1024 chars via `safeEncryptError`.
|
||||
2. `formatAdminAlertPayload`, which trims to 280 chars before pushing to Telegram.
|
||||
Withings is the trusted upstream, so this is not a network-attacker
|
||||
surface. A compromised or buggy Withings response *could* land an
|
||||
arbitrarily large `error` string into the audit table for any
|
||||
connected user — bounded only by Postgres TEXT column limits. Not
|
||||
exploitable today; recorded so the next reader doesn't assume the
|
||||
upstream string is trusted-and-bounded everywhere.
|
||||
- **Recommended fix** (defer): apply the same `slice(0, 1024)` cap inside the `WithingsApiError` constructor (or at the `auditLog` call-site) so the audit-row payload size matches the encrypted-error column behaviour. Belt-and-braces; the practical impact today is zero.
|
||||
|
||||
### L-2 `pickCanonicalWorkoutRows` collapses two genuine back-to-back same-source same-activity rows at ≤ 90 s `startedAt` delta
|
||||
- **File**: `src/lib/workouts/canonical-rows.ts:128-130, 192-259`, batch path `src/app/api/workouts/batch/route.ts:273-298`.
|
||||
- **Attack vector**: This is not a security vuln — it's a behavioural
|
||||
edge-case noted because the focus-area brief asked. Two intentional
|
||||
HIIT intervals stamped 30–90 s apart with the same `activityType`
|
||||
+ same `source` collapse to one. The 90 s window is documented as
|
||||
"covers HK's ±60 s smoothing + buffer" and is enforced via inclusive
|
||||
`<=` (test: `treats a row exactly at the ±90 s boundary as in-window`).
|
||||
No malicious-injection risk: the batch route forces `userId: user.id`
|
||||
from the authenticated session (`src/app/api/workouts/batch/route.ts:228`),
|
||||
so a forged-source row in one batch can only suppress *another row
|
||||
in the same caller's batch* — never a victim user's row (`(userId, source, externalId)` composite unique + the picker grouping key both include `userId`).
|
||||
- **Recommended fix** (defer): the iOS contract guarantees each
|
||||
HKWorkout carries a distinct `endedAt` / `durationSec`; the picker
|
||||
could optionally tighten its grouping key to `(activityType, startedAt ± 90 s, endedAt ± 90 s)` so two back-to-back intervals survive even when their starts are 60 s apart. Not required — the legacy read-time picker has the same window for the same reason, and the workout-card UX merges intervals into one row by design.
|
||||
|
||||
## Strengths
|
||||
|
||||
1. **`returnAllZodIssues` is correctly sanitised + correctly scoped.**
|
||||
- `sanitiseZodIssues` returns *only* `{ path: issue.path.join("."), code, message }`. The privacy comment in `src/lib/api-response.ts:9-19, 33-36` explicitly calls out `issue.params` as the data-leak surface, and the test `does NOT echo issue.params (privacy: may contain user input)` (`src/lib/__tests__/api-response-zod.test.ts:84-100`) asserts `Object.keys(issue).sort() === ["code", "message", "path"]`.
|
||||
- The `path.join(".")` step produces a plain string — no prototype access happens client-side because the consumer reads `body.details.issues[*].path` as a flat field. Verified via `node -e 'const o = {}; o["__proto__"] = "evil"; console.log(o["__proto__"]);'` → bracket assignment to `__proto__` is silently dropped, so a forged path `["__proto__"]` cannot pollute Object.prototype anywhere downstream.
|
||||
- Audit-ledger write in `src/app/api/dashboard/widgets/route.ts:141-157` is scoped to `userId: user.id` (server-resolved from `requireAuth()`); persisted shape is `JSON.stringify({ issues })` where `issues` is the sanitised projection. The integration test `writes one audit-ledger row keyed dashboard.widgets.validation-failed` (`src/app/api/dashboard/widgets/__tests__/route.test.ts:123-145`) asserts `Object.keys(issue).sort() === ["code", "message", "path"]` for every persisted entry.
|
||||
- Fire-and-forget audit write is `.catch(() => {})` — a DB hiccup cannot 5xx the 422 contract response (test `does not block the 422 response when the audit-row write rejects`).
|
||||
|
||||
2. **Withings off-response classifier is a strict whitelist + conservative default.**
|
||||
- Explicit `REAUTH_CODES = {100, 101, 102}` and `PERSISTENT_CODES = {293, 294}` checks run BEFORE the 200..299 range fallback, so a contract-mismatch code that happens to sit inside the OAuth range stays `persistent` instead of incorrectly bucketing as `reauth_required` (`src/lib/withings/response-classifier.ts:173-208`).
|
||||
- `persistent` state cannot be triggered by a forged webhook to suppress a victim user's data: the classifier reads Withings' own HTTP response, not the webhook body. The webhook handler `src/lib/withings/webhook-handler.ts:89-160` is authorised via path-segment shared secret + per-IP rate-limit; even with the secret, all it can do is enqueue a real Withings API call for an already-mapped `withingsUserId`. The classification verdict is then driven by Withings' authentic response, not by the attacker.
|
||||
- Refresh-token never lands in the error path: `WithingsApiError` carries only `{ verb, classification, withingsStatus, reason, upstreamError }` where `upstreamError` is restricted to `typeof json?.error === "string"`. The refresh token is only POSTed to Withings' token URL; it never appears in any `getEvent()` annotation, `addWarning`, `addExternalCall`, audit row or alert payload (verified by grep on `refreshToken`/`refresh_token` against the diff and against the sync paths).
|
||||
- `WithingsApiError.message` format stays compatible with the legacy `extractWithingsStatus` regex — `classifyError` falls back to regex parsing for pg-boss job-retry survivors whose prototype was lost in JSON round-trip (covered by `src/lib/withings/__tests__/response-classifier.test.ts:166-211`).
|
||||
|
||||
3. **Write-time workout dedup is user-scoped and tamper-proof.**
|
||||
- Group key includes `userId` (`src/lib/workouts/canonical-rows.ts:210-219`), pinned by the test `returns rows from different users untouched even on identical timestamps` (`canonical-rows.test.ts:113-130`).
|
||||
- The batch route always overrides client-supplied `userId` with the authenticated `user.id` (`src/app/api/workouts/batch/route.ts:228`), so a malicious client cannot mint a payload row that addresses another user's account.
|
||||
- A user who submits a forged `APPLE_HEALTH` row to outrank a `MANUAL` row can only suppress *their own* `MANUAL` row inside the same batch — there is no cross-user trust boundary the source-ladder is defending. (And: the legacy read-time picker on `GET /api/workouts` collapses cross-batch duplicates afterwards via the same ladder, so a future poisoning of the source field would surface across reads anyway.)
|
||||
|
||||
4. **`pnpm check-env` does not log or print secret values.**
|
||||
- `renderResults` emits only `[OK] <NAME>` / `[MISSING-REQUIRED] <NAME>` / `[missing-optional] <NAME>` — no value substitution path (`scripts/check-env.ts:177-207`).
|
||||
- `parseEnvFile` writes the file contents into a plain object and `isPresent` only checks `typeof value === "string" && value.trim().length > 0`; nothing reads back the value to stdout.
|
||||
- The `--file` flag controls only the env-source, never the manifest path (`MANIFEST_PATH = resolve(__dirname, "env-manifest.json")` is module-frozen). A malicious env-file with a `__proto__=evil` line is also a non-issue: bracket assignment to `__proto__` is silently dropped by V8 (verified via repro), and even if it landed, `env[n]` reads through bracket access with `n` sourced from the manifest only.
|
||||
- Operator-only CLI; no network-attacker surface.
|
||||
|
||||
5. **Knip ignore-block scope is correct.**
|
||||
- `src/components/ui/**` ignores `exports` + `types` only. Verified by `ls` — every file in the directory is a pure shadcn UI primitive (`alert-dialog`, `dialog`, `dropdown-menu`, `input`, `password-input`, `password-strength`, `select`, `sheet`, `switch`, `table`, `tabs`, `tooltip`, …). No auth, no crypto, no input-validation helper hides under the shadcn prefix.
|
||||
- `src/lib/validations/**` ignores `types` only (NOT `exports`). So any genuinely-unused export *would* still trip CI; the ignore-block only spares the Zod-derived `z.infer<typeof …>` aliases that the API contract publishes for the iOS client. Every file in the directory is a Zod schema module by design.
|
||||
- The companion knip flip (`20df7b9d`) drops the `--include files,dependencies,binaries,unlisted` scope on the workflow; the gate now runs against every issue knip reports by default. Two ignore-blocks are now the single point of truth for "what knip is muted on."
|
||||
|
||||
6. **Re-export cleanup (`dce14fb4`) is complete.**
|
||||
- `grep -rn "listSupportedTimezones\|describeInjectionSite"` confirms every real caller already imports from the source path (`@/lib/tz/format` / `@/lib/medications/injection-sites`). No dynamic-import string contains either name.
|
||||
|
||||
---
|
||||
|
||||
50-word summary follows the report.
|
||||
@@ -0,0 +1,164 @@
|
||||
# v1.4.42 Senior-dev architectural findings
|
||||
|
||||
Branch under review: `develop` vs live `main` (v1.4.41). ~30 commits after the v1.4.41 tag `67207d72`.
|
||||
Read-only review. No files modified.
|
||||
|
||||
## Verdict
|
||||
|
||||
**APPROVE_WITH_FIXES**
|
||||
|
||||
The release is architecturally sound. The five named waves (BERLIN_DAY_FORMATTER dedup, write-time canonical-workout picker, Withings off-response classifier, `pnpm check-env`, knip gate flip, queryKey long-tail migration) all land with clean discipline and substantial test coverage (4 525 → 4 744 unit). The v1.4.41 L1 finding (PR-detection soft-delete) and M2 finding (ESLint guard / test-guard scope drift) are both closed in this release. None of the findings below are release-blockers, but the **High** below directly undermines the named motivation of the `check-env` work and should land before the v1.4.42 tag.
|
||||
|
||||
## Critical
|
||||
|
||||
(none)
|
||||
|
||||
## High
|
||||
|
||||
### H1. `pnpm check-env` does NOT catch the v1.4.40 AP-2 silent-disable scenario it was built to catch
|
||||
|
||||
**Files:** `scripts/env-manifest.json:57-80`, `docs/ops/env-check.md:52-72`
|
||||
|
||||
**Root cause:** the APNs group in `scripts/env-manifest.json` is declared `"required": false` without `"allOrNone": true`. The synthetic `<all-or-none>` severity-REQUIRED row in `scripts/check-env.ts:147-166` is gated on `group.allOrNone === true` AND a partially populated group. The APNs group is the v1.4.40 AP-2 motivator (cited verbatim in the group description: "All four core APNS_* vars together. Missing any disables APNs silently — the v1.4.40 AP-2 gap that motivated this manifest") but its flag is not set.
|
||||
|
||||
Walk-through of the AP-2 scenario the manifest is supposed to catch — three APNS_* vars set, `APNS_KEY` / `APNS_KEY_FILE` both missing:
|
||||
|
||||
| Variable | Status |
|
||||
| ----------------- | ------------------------------------------------------------ |
|
||||
| `APNS_KEY_ID` | `[OK]` |
|
||||
| `APNS_TEAM_ID` | `[OK]` |
|
||||
| `APNS_BUNDLE_ID` | `[OK]` |
|
||||
| `APNS_KEY` (anyOf `APNS_KEY_FILE`) | `[missing-optional]` |
|
||||
|
||||
`missingRequired = 0` → **exit 0** → deploy fires green. This is exactly the silent-disable shape that bit the operator for three days in v1.4.40.
|
||||
|
||||
The docs at `docs/ops/env-check.md:60-72` make the gap visible — the worked example shows the exact AP-2 case landing as `[missing-optional]` (not `[MISSING-REQUIRED]`) while the surrounding text claims it's "the v1.4.40 AP-2 detection pattern". The doc and the manifest disagree about whether the case fails.
|
||||
|
||||
**Recommended fix:** add `"allOrNone": true` to the APNs group in `scripts/env-manifest.json:60`. With the flag set, the `checkEnv` walker emits a `<all-or-none>` synthetic row (`required: true`) when 1–3 of the 4 vars are set, the deploy gate flips to exit 1, and the AP-2 motivator actually works. The unit test at `scripts/__tests__/check-env.test.ts` already covers the all-or-none synthetic-row path (via the off-host-backups group), so the regression test surface is in place — only the manifest entry needs to flip.
|
||||
|
||||
**Why High not Critical:** the script is a CLI gate, not a runtime block — the app still boots without it. But the entire `check-env` wave was conceived (per the commit body, the group description, the docs example) as the prevention of exactly this case, and shipping it without the fix means the v1.4.40 AP-2 silent-disable still ships silently if the same env-block misedit recurs.
|
||||
|
||||
## Medium
|
||||
|
||||
### M1. `pickCanonicalWorkoutRows` write-time picker bypasses the user's per-metric `sourcePriorityJson`
|
||||
|
||||
**Files:** `src/lib/workouts/canonical-rows.ts:73, 138-141`, `src/app/api/workouts/batch/route.ts:273-290`, `src/lib/measurements/pick-canonical-workout-rows.ts:95-96`
|
||||
|
||||
**Root cause:** the write-time picker reads the source ladder from the `DEFAULT_WORKOUT_SOURCE_PRIORITY` *constant* (`["APPLE_HEALTH", "WITHINGS", "MANUAL", "IMPORT"]`). The read-time picker reads it from `getSourceLadder(parseSourcePriority(userPriorityJson), "steps")` — i.e. the user's persisted preference under the `steps` key. A user who customized their Settings → Sources surface to promote `MANUAL` above `APPLE_HEALTH` would see:
|
||||
|
||||
| Phase | What it does on an Apple Watch + Manual cross-source pair within 90 s |
|
||||
| ----------- | --------------------------------------------------------------------- |
|
||||
| Write-time | `APPLE_HEALTH > MANUAL` (constant ladder) → keeps the Apple Watch row, drops Manual |
|
||||
| Read-time | `MANUAL > APPLE_HEALTH` (user ladder) → would have picked Manual |
|
||||
|
||||
The Manual row is gone from the DB before read-time ever sees it. The user's customization is silently overridden.
|
||||
|
||||
The phase-W5 report (`.planning/phase-W5-IOS-WORKOUTS-v1442-report.md:14-18`) and the helper docstring (`src/lib/workouts/canonical-rows.ts:46-70`) both characterise this as parity: "both pickers consult a single source of truth". That is true at the constant-vs-constant level, but is false at the constant-vs-user-priority level.
|
||||
|
||||
**Effect:** scope-narrow today — `DEFAULT_SOURCE_PRIORITY.steps` in `src/lib/validations/source-priority.ts:206` is `["APPLE_HEALTH", "WITHINGS", "MANUAL"]` (only three entries, no `IMPORT`), and most iOS-paired users keep the Apple-first default. But the moment a user customizes, the write-time picker silently disregards them.
|
||||
|
||||
**Recommended fix (v1.4.43 or later):** either
|
||||
- (a) document the divergence explicitly in the helper docstring + the W5 report: "the write-time picker uses the canonical ladder regardless of user customization; the read-time picker re-applies user customization at read time, so a customised ladder still surfaces the user's preferred winner if both rows survive write-dedup" — except that's a no-op once the write-time picker has dropped the user's preferred row;
|
||||
- (b) accept the small ingest-path cost (one user lookup) and resolve the user's ladder before the picker pass. The batch route already has the user id; the lookup is one indexed read against `User.sourcePriorityJson`.
|
||||
|
||||
(b) is the architecturally correct call for v1.4.43; (a) is a documentation patch that should land in this release.
|
||||
|
||||
### M2. `consecutiveFailures` counter blends `transient` and `persistent` kinds — alert-payload "kind" reflects last failure only
|
||||
|
||||
**File:** `src/lib/integrations/status.ts:228-294`
|
||||
|
||||
**Root cause:** `recordSyncFailure` increments `consecutiveFailures` on every call regardless of `kind`. When the 3-strike threshold trips at `row.consecutiveFailures >= threshold`, the alert payload (`maybeAlertAdmins` → `formatAdminAlertPayload`) is constructed from the CURRENT call's input — i.e. the kind of the *most recent* failure. A real-world burst like:
|
||||
|
||||
- t0: Withings 293 (`persistent`) → consecutiveFailures = 1
|
||||
- t1: 503 network blip (`transient`) → consecutiveFailures = 2
|
||||
- t2: 503 network blip (`transient`) → consecutiveFailures = 3 → alert fires with `kind: "transient"` and action "investigate the upstream service"
|
||||
|
||||
The admin gets a transient-bucket alert with action text "investigate the upstream service" when the actual root cause was a `293` contract bug, because the latest failure happened to be transient.
|
||||
|
||||
**Effect:** Low UX-of-ops paper-cut — operators routinely re-grep audit logs once paged, so the wrong action line just delays the diagnosis by one query. Not a behavioural bug.
|
||||
|
||||
**Recommended fix:** track a separate per-kind counter or carry the most-severe kind from the last N failures into the payload. Defer to v1.4.43 backlog; not worth blocking the release.
|
||||
|
||||
## Low
|
||||
|
||||
### L1. Knip ignore-block `src/components/ui/**: ["exports", "types"]` is broader than just shadcn surface
|
||||
|
||||
**File:** `knip.json:36-39`
|
||||
|
||||
**Root cause:** the ignore-block muses both `exports` and `types` for every file under `src/components/ui/**`. The W1-KNIP rationale (`./.planning/phase-W1-KNIP-v1442-report.md:53-60`) explicitly lists shadcn registry symbols that need the mute. But `src/components/ui/` also carries HealthLog-specific components that are NOT in the upstream shadcn registry: `empty-state.tsx`, `logo.tsx`, `password-input.tsx`, `password-strength.tsx`, `date-input.tsx`, `responsive-sheet.tsx`. Any future export added to those files would silently bypass knip's dead-code check.
|
||||
|
||||
**Effect:** Low. The path-scope choice is documented and the workaround is one line (move HealthLog-specific UI elsewhere — e.g. `src/components/ui-x/` for "extensions"). The footgun is small in practice because the affected files are few and largely stable. But the comment on the ignore-block says "mute shadcn surface" while the path glob is broader than just shadcn.
|
||||
|
||||
**Recommended fix:** either narrow the ignore-block to specific filenames (the shadcn-only files) or move HealthLog-specific components to a sibling directory so the broad-glob mute remains shadcn-only. Defer to v1.4.43.
|
||||
|
||||
### L2. `pickCanonicalWorkoutRows` complexity is O(n²) — fine for batch ≤ 100, but the comment is misleading
|
||||
|
||||
**File:** `src/lib/workouts/canonical-rows.ts:188-224`
|
||||
|
||||
**Root cause:** the helper walks `tagged` and for each entry walks every existing `group` looking for a match — that's O(n × g) where g is the number of distinct groups. The docstring at line 190 says "typical batch is ≤ 100 entries so the constant is fine" which is true today. But the route's hard cap is `MAX_WORKOUTS_PER_BATCH = 100` (see `src/lib/validations/workout.ts`), so worst case is 100 × 100 = 10 000 group-comparison ops — completely negligible. The misleading bit: the comment says "O(n²) worst case" but worst case is actually O(n²) only when every row seeds its own group (all distinct activityType + startedAt), and best case is O(n) when every row joins the first group.
|
||||
|
||||
This is a cosmetic-only docstring nit. Algorithm is correct.
|
||||
|
||||
**Recommended fix:** tighten the docstring to "worst case O(n × g) where g ≤ n; for the 100-row batch cap that's bounded at 10 000 group-comparison ops". No code change.
|
||||
|
||||
### L3. `WithingsApiError` is re-thrown across pg-boss retries — prototype loss is the documented case, but `WithingsApiError.cause` chain is not preserved
|
||||
|
||||
**File:** `src/lib/withings/response-classifier.ts:233-256, 269-281`
|
||||
|
||||
**Root cause:** `WithingsApiError` extends `Error` with three readonly fields (`classification`, `withingsStatus`, `reason`, `verb`). When pg-boss rehydrates a job, it serialises the failure via `JSON.stringify` of the thrown error — only `message` survives. The fallback regex at `classifyError(err)` recovers `classification` from `message`, which works.
|
||||
|
||||
But `WithingsApiError.cause` (the original `fetch` error, if any) is never set. If a future debugging surface wants to walk `err.cause` to find the underlying network failure, the chain is broken at the rethrow site. Today no consumer reads `cause`, so this is purely forward-looking.
|
||||
|
||||
**Recommended fix:** add `cause?: unknown` to the constructor opts and pass it through via `super(message, { cause })`. Defer to v1.4.43.
|
||||
|
||||
### L4. `workoutsRecentList` cache-key opts object — `sportType: undefined` vs `sportType` absent may produce two cache entries
|
||||
|
||||
**File:** `src/lib/query-keys.ts:229-234`, `src/hooks/use-workouts.ts:95-100`
|
||||
|
||||
**Root cause:** `queryKeys.workoutsRecentList({ limit, offset, since, sportType })` is invoked in `useWorkouts` with all four keys present even when the caller passed only `{ limit: 10 }`. TanStack Query hashes the queryKey via `hashKey` (stable JSON) — `undefined` properties are stripped by `JSON.stringify`, so `{limit:10, offset:undefined, since:undefined, sportType:undefined}` and `{limit:10}` hash to the same key. Two callers passing the same logical params will share a cache slot.
|
||||
|
||||
But: if a future caller passes `null` for one of these fields (instead of `undefined`), the hash would diverge — `null` survives JSON.stringify. The hook never normalises before passing to the factory, so a single typo (`null` vs `undefined`) silently double-caches. Pre-empt: the hook should normalise to drop `undefined`/`null` symmetrically before invoking the factory.
|
||||
|
||||
This is forward-looking — today no caller passes `null`, so no actual cache collision exists.
|
||||
|
||||
**Recommended fix:** add an inline comment in the hook stating that all four keys must be undefined-or-string-or-number (no nulls); defer the runtime normalisation to v1.4.43.
|
||||
|
||||
### L5. `recordSyncFailure` audit row is `await`-ed before the alert-dispatch path — a slow audit insert delays admin paging
|
||||
|
||||
**File:** `src/lib/integrations/status.ts:250-260, 269-294`
|
||||
|
||||
**Root cause:** the `await auditLog(...)` at line 250 runs serially before the alert-dispatch decision. The audit insert is one Prisma `auditLog.create` — typically <10 ms — but on a slow DB it adds latency to the admin-page path. A connection-pool starvation event (the exact case that motivates an admin alert) could leave the alert waiting on its own audit insert.
|
||||
|
||||
The docstring at line 246-249 notes this intentionally: "fire-and-await so an integration test can assert it. The auth/audit helper is its own DB write so it's safe to call serially without bloating latency in the success path (which never calls this)." That's true for the success path, but the *failure path* is the one where DB-pool starvation is most likely.
|
||||
|
||||
**Recommended fix:** none today. The trade-off is acknowledged in the docstring. If the audit-before-alert ordering ever becomes load-bearing, swap to `Promise.allSettled([auditLog(...), maybeAlertAdmins(...)])` and accept the audit-row-arrives-after-alert inversion.
|
||||
|
||||
## Strengths
|
||||
|
||||
- **BERLIN_DAY_FORMATTER dedup is byte-identical.** The extracted `toBerlinDayKey()` at `src/lib/tz/resolver.ts:58-87` ships the same `Intl.DateTimeFormat` options (`"en-US"`, `"Europe/Berlin"`, `2-digit` month/day), the same `formatToParts` walk, and the same `throw new Error("Could not derive Berlin day key")` on missing parts. All seven status helpers (`bmi`, `blood-pressure`, `general`, `medication-compliance`, `mood`, `pulse`, `weight`) import from the same module and call sites are unchanged. DST safety is preserved (formatter handles the transition transparently via the `Europe/Berlin` zone).
|
||||
|
||||
- **Write-time / read-time canonical-workout picker layering is correct.** The 90 s vs 5 min window choice is justified in both docstrings: same-batch rows from one iOS client have no cross-device clock skew (HK aggregates paired sensors to the same `startDate`), so 90 s covers the ±60 s smoothing plus a small buffer; cross-batch rows from independent ingest paths need the 5 min slack. The two pickers compose: write-time drops same-batch dup before persistence (storage savings), read-time still collapses cross-batch dup on the rare timing edge (correctness).
|
||||
|
||||
- **`STRAVA_IMPORT` → `IMPORT` enum-rename is documented at the right level.** The docstring at `src/lib/workouts/canonical-rows.ts:52-55` explicitly notes the scope-memo wording vs the actual `MeasurementSource` enum. There is no `STRAVA_IMPORT` variant in `prisma/schema.prisma:406-413`; Strava XML imports ride the generic `IMPORT` bucket. The agent's note is correct and the helper carries the right ladder.
|
||||
|
||||
- **`duplicate` semantic the iOS client treats as "cursor advances".** Confirmed against `/Users/marc/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Models/HealthKitBatchDTO.swift:62-68`: the iOS measurement-batch handler explicitly comments "`inserted` UND `duplicate` UND `skipped` sind allesamt terminal — Cursor fährt …". The workout-batch handler on iOS does not yet exist in the main branch, but the route convention is intentionally aligned with the measurement batch so the future iOS workout uploader can reuse the same cursor advance rule.
|
||||
|
||||
- **`FailureKind` extension is non-breaking.** Surveyed every production-code consumer (`src/lib/withings/sync.ts`, `src/lib/withings/sync-activity.ts`, `src/lib/withings/sync-sleep.ts`, `src/lib/moodLog/sync.ts`) — none switch-narrow on the union exhaustively; all pass it as a literal-or-mapped value into `recordSyncFailure`. The Telegram bot and APNs paths don't import `FailureKind` at all. The admin-alert formatter at `src/lib/integrations/status.ts:505-547` handles all three cases with distinct labels ("re-auth required" / "persistent error" / "transient error") and distinct action lines.
|
||||
|
||||
- **`classifyError(err)` regex fallback is the safe default.** When pg-boss rehydration drops the `WithingsApiError` prototype, the regex `/Withings\s+\w+\s+error:\s*(\d+)/` recovers the status digit and re-runs `classifyWithingsResponse(200, { status })`. If Withings ever changes the message format, the regex stops matching, and the helper returns `transient` — the conservative bucket that keeps trying. The retry+3-strike admin alert still catches a "keeps happening" case.
|
||||
|
||||
- **`recordSyncFailure(kind: "persistent")` maps to `state: "error_transient"` correctly.** The state-machine docstring at `src/lib/integrations/status.ts:215-220` documents the intent: `persistent` is operator-attention but NOT skip-future-sync. `isReauthRequired` at line 121-130 only short-circuits on `error_reauth`, so the next sync still runs. The audit row carries `kind: "persistent"` explicitly so ops grep can filter for contract bugs.
|
||||
|
||||
- **Knip gate flip is well-staged.** W1's merge-order callout (W3 + W4 first, W1 last) was honoured: both dead re-exports (`describeInjectionSite`, `listSupportedTimezones`) are dropped by the time W1 lands. The ignore-block carries an explicit rationale per scope (shadcn parity for `src/components/ui/**`, contract-type API surface for `src/lib/validations/**: ["types"]`). The `src/lib/validations/**` scope is narrowed to `["types"]` only — `exports` still trips on a truly dead schema, so a stale validator still surfaces.
|
||||
|
||||
- **queryKey factory long-tail migration closes the v1.4.41 senior-dev M2 finding.** The ESLint guard at `eslint-plugins/healthlog/queryKey-factory.js:48-76` and the test-guard substitute at `src/lib/__tests__/query-keys.test.ts:260-279` now list the IDENTICAL scope (settings, medications, admin, hooks dirs + the three guarded files under `src/app/medications/**` + `src/app/targets/page.tsx`). The two stay synchronised in lockstep per the W3 commit. Every new factory entry maintains the resource-root invariant — `medicationCompliance/Titration/Cadence/Glp1Details/IntakeDrugLevelChart/IntakeList` all start with `["medications", ...]`, `withingsStatus` shares the `["withings"]` root, `adminAuditLogFiltered` shares the `["admin", "audit-log"]` prefix with `adminAuditLog`, `workoutsRecentList` shares `["workouts"]` with `workouts/workoutsRecent`. `medicationDependentKeys` already includes `queryKeys.medications()` so every intake mutation cascades correctly.
|
||||
|
||||
- **PR-detection soft-delete fix closes v1.4.41 L1.** Both call sites at `src/lib/personal-records/pr-detection-worker.ts:219, 402` now carry `deletedAt: null`, and the DR-backup intent at `src/lib/jobs/offhost-backup.ts:219` is documented inline ("includes soft-deleted rows because this is the DR snapshot, not a user-facing export") — exactly the recommended-fix shape from the v1.4.41 review.
|
||||
|
||||
- **Dashboard tile-strip placeholder parity is byte-exact.** `src/app/page.tsx:1412-1421` matches the live `TrendCard` chrome (`flex h-full min-h-[6rem] w-full min-w-0 flex-col` + the existing `bg-card border-border rounded-xl border p-4 md:p-6`); the all-suspend collapse-to-zero-height regression is closed; the existing structural Suspense test pin keys on `aria-hidden="true"` and stays green without modification.
|
||||
|
||||
- **Multi-issue Zod envelope is privacy-safe.** `sanitiseZodIssues` at `src/lib/api-response.ts:37-45` strips `issue.params` (which can echo the raw rejected user input for some Zod codes) and surfaces only `{path, code, message}`. The docstring at lines 9-19 names the threat ("e.g. a too-long string, a regex source"); the integration test at `tests/integration/dashboard-widgets-save.test.ts` pins both the multi-issue shape and the params-leak guard.
|
||||
|
||||
---
|
||||
|
||||
**Summary:** verdict APPROVE_WITH_FIXES, 0 Critical, 1 High, 2 Medium, 5 Low, 11 strengths. The only finding that should land before the v1.4.42 tag is H1 (add `allOrNone: true` to the APNs group in `scripts/env-manifest.json` so the v1.4.40 AP-2 silent-disable case actually trips the gate the wave was conceived to add). Everything else is safe to defer to v1.4.43.
|
||||
@@ -0,0 +1,273 @@
|
||||
# v1.4.42 Simplifier-residual findings
|
||||
|
||||
## Verdict
|
||||
|
||||
APPROVE_WITH_FIXES — the v1.4.42 waves landed clean wins: BERLIN_DAY_FORMATTER 7-way dedup (W4), Suspense double-comment consolidate (W4), 35→0 / 52→0 knip drop with enforcing CI gate (W1), `returnAllZodIssues` shared helper (W2), `pickCanonicalWorkoutRows` write-time helper (W5), Withings response classifier (W6), and ~70 long-tail queryKey bare-literals routed through the factory (W3). The residual scan turned up **one High** (W6 introduced two nested ternaries — explicitly against the simplifier style guide), **three Medium** (one missed BERLIN_DAY_FORMATTER dedup site, one duplicate function-name collision with the v1.4.30 read-time picker, one shared boilerplate between `returnAllZodIssues` and `apiError`), and **a handful of Low** items that are pure cleanup. None block ship; all are mechanical.
|
||||
|
||||
## High (clarity regression introduced this release)
|
||||
|
||||
### H1 — `src/lib/integrations/status.ts:513-518` + `:530-535`: two nested ternaries introduced by the W6 third-state extension
|
||||
|
||||
The W6 wave widened `FailureKind` from `transient | reauth_required` to a three-state union (`+ persistent`). The two formatter branches in `formatAdminAlertPayload` got the new arm wedged in as a nested ternary:
|
||||
|
||||
```ts
|
||||
// lines 513-518
|
||||
const reasonLabel =
|
||||
input.kind === "reauth_required"
|
||||
? "re-auth required"
|
||||
: input.kind === "persistent"
|
||||
? "persistent error"
|
||||
: "transient error";
|
||||
|
||||
// lines 530-535 — same nested-ternary shape inside a template-literal interpolation
|
||||
`Action: ${
|
||||
input.kind === "reauth_required"
|
||||
? "ask the user to reconnect the integration."
|
||||
: input.kind === "persistent"
|
||||
? "investigate the upstream contract — params/scope/action likely mismatched."
|
||||
: "investigate the upstream service."
|
||||
}`
|
||||
```
|
||||
|
||||
The simplifier style guide explicitly forbids this pattern ("Avoid nested ternary operators — prefer switch statements or if/else chains for multiple conditions"). Pre-v1.4.42 both spots were single ternaries (two arms) and read cleanly; the third arm pushed them into the antipattern.
|
||||
|
||||
**Proposed shape** — a single `kindCopy` lookup driven off `FailureKind`:
|
||||
|
||||
```ts
|
||||
const COPY_BY_KIND: Record<FailureKind, { reason: string; action: string }> = {
|
||||
reauth_required: {
|
||||
reason: "re-auth required",
|
||||
action: "ask the user to reconnect the integration.",
|
||||
},
|
||||
persistent: {
|
||||
reason: "persistent error",
|
||||
action: "investigate the upstream contract — params/scope/action likely mismatched.",
|
||||
},
|
||||
transient: {
|
||||
reason: "transient error",
|
||||
action: "investigate the upstream service.",
|
||||
},
|
||||
};
|
||||
|
||||
const { reason: reasonLabel, action: actionLabel } = COPY_BY_KIND[input.kind];
|
||||
…
|
||||
`Action: ${actionLabel}`;
|
||||
```
|
||||
|
||||
Net: both nested ternaries collapse, the table reads as the contract, and adding a future fourth `FailureKind` is a one-row table edit instead of two more arms in two different ternary stacks. Mechanical, no behaviour change.
|
||||
|
||||
## Medium (clean wins the waves missed)
|
||||
|
||||
### M1 — `src/lib/insights/bucket-series.ts:19` + `src/lib/analytics/bp-in-target.ts:63`: two BERLIN_DAY_FORMATTER sites the W4 dedup missed
|
||||
|
||||
The W4 wave's seven-way BERLIN_DAY_FORMATTER dedup hoisted the formatter + `toBerlinDayKey()` from the seven `src/lib/insights/*-status.ts` files into `src/lib/tz/resolver.ts`. Two more declarations of the same constant survive:
|
||||
|
||||
- `src/lib/insights/bucket-series.ts:19-24` — same `Intl.DateTimeFormat("en-US", { timeZone: "Europe/Berlin", year: "numeric", month: "2-digit", day: "2-digit" })` constant. Its consumer (`toBerlinYmd` at line 67) needs the *parts* (numeric year/month/day) not the string, but it could still reuse `BERLIN_DAY_FORMATTER` from `@/lib/tz/resolver` by exporting the formatter (or by parsing back from the existing `toBerlinDayKey(date)` output).
|
||||
- `src/lib/analytics/bp-in-target.ts:63-72` — declares the same formatter (with `en-CA` locale to skip the part-reassembly) and its own local `toBerlinDayKey(date)`. Pre-existing duplication, not v1.4.42-introduced, but it surfaced as the obvious next dedup site once W4's hoist landed.
|
||||
|
||||
Neither was touched by v1.4.42, so this is M (not H). Net: one shared `BERLIN_DAY_FORMATTER` export (already in `@/lib/tz/resolver`) + a tiny `BerlinYmd` helper alongside it; two more files drop ~12 LOC each.
|
||||
|
||||
### M2 — Same function name `pickCanonicalWorkoutRows<T>` now lives in two modules with different signatures
|
||||
|
||||
W5 added `src/lib/workouts/canonical-rows.ts::pickCanonicalWorkoutRows()` for the write-time path. The read-time picker from v1.4.30 lives at `src/lib/measurements/pick-canonical-workout-rows.ts::pickCanonicalWorkoutRows()`. Both are exported under the SAME identifier:
|
||||
|
||||
```ts
|
||||
// v1.4.30 read-time
|
||||
export function pickCanonicalWorkoutRows<T extends WorkoutPickerRow>(
|
||||
rows: readonly T[], userPriorityJson: unknown = null,
|
||||
): T[]
|
||||
|
||||
// v1.4.42 write-time
|
||||
export function pickCanonicalWorkoutRows<T extends WorkoutRow>(
|
||||
rows: readonly T[],
|
||||
): T[]
|
||||
```
|
||||
|
||||
`src/app/api/workouts/batch/route.ts:71` imports from `@/lib/workouts/canonical-rows`; `src/app/api/workouts/route.ts:56` + `src/app/api/workouts/[id]/route.ts:31` import from `@/lib/measurements/pick-canonical-workout-rows`. Today's call sites get the right one by import path, but the same-name collision is the kind of footgun that bites the next person who auto-completes `pickCanonicalWorkoutRows` in a new route and silently invokes the wrong picker (no user-priority resolution on a read; missing the 90 s window on a write).
|
||||
|
||||
The W5 helper header already calls out the layered intent — write-time payload-internal dedup vs. read-time cross-batch dedup — so the design is right. Only the naming creates the trap.
|
||||
|
||||
**Proposed shape**: rename one. The write-time helper is the newer one and the one whose call site does *not* take `userPriorityJson`, so a name like `pickCanonicalWorkoutRowsForIngest()` (or `dedupeWorkoutBatch()`) makes the write-time contract self-describing and leaves the established `pickCanonicalWorkoutRows()` name for the read-time picker. One file rename + one import update; both pickers' test suites stay byte-identical.
|
||||
|
||||
### M3 — `src/lib/api-response.ts:47-71` (`returnAllZodIssues`) and `:86-107` (`apiError`) share the meta/headers extraction boilerplate
|
||||
|
||||
The new W2 helper duplicates the meta/headers handling from `apiError`:
|
||||
|
||||
```ts
|
||||
// returnAllZodIssues lines 55-69
|
||||
const { headers, ...rest } = meta ?? {};
|
||||
const metaKeys = Object.keys(rest);
|
||||
return NextResponse.json(
|
||||
{ data: null, error: "Validation failed", details: { issues: … },
|
||||
...(metaKeys.length > 0 ? { meta: rest } : {}) },
|
||||
{ status, ...(headers ? { headers } : {}) },
|
||||
);
|
||||
|
||||
// apiError lines 94-106 — same `headers`/`metaKeys`/`...(headers ? …)` shape
|
||||
```
|
||||
|
||||
The boilerplate is small (≈ 6 LOC duplicated) but it's the kind of drift that bit the apiError envelope before — if a future change extends the meta passthrough (e.g. an `errorId` autoinject for Sentry), it lands in one helper and not the other.
|
||||
|
||||
**Proposed shape** — a private builder:
|
||||
|
||||
```ts
|
||||
function buildJsonErrorResponse(
|
||||
body: Record<string, unknown>,
|
||||
status: number,
|
||||
meta: { headers?: Record<string, string> } & Record<string, unknown> | undefined,
|
||||
): NextResponse {
|
||||
const { headers, ...rest } = meta ?? {};
|
||||
const metaKeys = Object.keys(rest);
|
||||
return NextResponse.json(
|
||||
{ ...body, ...(metaKeys.length > 0 ? { meta: rest } : {}) },
|
||||
{ status, ...(headers ? { headers } : {}) },
|
||||
);
|
||||
}
|
||||
|
||||
export function apiError(message, status = 400, meta?) {
|
||||
return buildJsonErrorResponse({ data: null, error: message }, status, meta);
|
||||
}
|
||||
|
||||
export function returnAllZodIssues(error, status = 422, meta?) {
|
||||
return buildJsonErrorResponse(
|
||||
{ data: null, error: "Validation failed",
|
||||
details: { issues: sanitiseZodIssues(error.issues) } },
|
||||
status, meta,
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
Net: −12 LOC, single boilerplate update path, identical wire shape — both helpers' contract tests (the new `api-response-zod.test.ts` + existing rate-limit / errorCode tests) stay green.
|
||||
|
||||
### M4 — `src/lib/withings/sync.ts:118-135` + `:195-209`: identical 14-line failure-record catch-block now lives in two places
|
||||
|
||||
W6 swapped both call sites from `isWithingsRefreshReauthFailure(message)` to the typed `classifyError(err)` + `classificationToFailureKind()` chain. The two blocks ended up byte-identical:
|
||||
|
||||
```ts
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
const classification = classifyError(err);
|
||||
await recordSyncFailure({
|
||||
userId,
|
||||
integration: "withings",
|
||||
kind: classificationToFailureKind(classification),
|
||||
message,
|
||||
errorCode:
|
||||
err instanceof WithingsApiError
|
||||
? err.withingsStatus?.toString()
|
||||
: extractWithingsStatus(message),
|
||||
});
|
||||
```
|
||||
|
||||
**Proposed shape** — a `recordWithingsSyncFailure(userId, err)` helper next to `classificationToFailureKind`:
|
||||
|
||||
```ts
|
||||
async function recordWithingsSyncFailure(userId: string, err: unknown): Promise<void> {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
await recordSyncFailure({
|
||||
userId,
|
||||
integration: "withings",
|
||||
kind: classificationToFailureKind(classifyError(err)),
|
||||
message,
|
||||
errorCode:
|
||||
err instanceof WithingsApiError
|
||||
? err.withingsStatus?.toString()
|
||||
: extractWithingsStatus(message),
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
Both call sites collapse to `await recordWithingsSyncFailure(userId, err);`. Sync-activity / sync-sleep migration to typed errors (W6 deferred to v1.4.43) gets a single line per catch-block as a bonus.
|
||||
|
||||
## Low (defer to v1.4.43)
|
||||
|
||||
### L1 — `src/app/medications/page.tsx:553`: bare-literal `queryKey` outside the W3 guarded surface
|
||||
|
||||
```ts
|
||||
const apiEndpointKey = ["medication-api-endpoint", medication?.id];
|
||||
```
|
||||
|
||||
W3 added `src/app/medications/page.tsx` to the ESLint plugin's `GUARDED_FILES`, but the literal lives in a `const` assignment (not a `queryKey: [ … ]` property), so the rule's `Property` visitor doesn't fire. The two `useQuery` / `setQueryData` / `removeQueries` calls below all reference the same `apiEndpointKey` constant. Centralising as a factory entry (`queryKeys.medicationApiEndpoint(medicationId)`) would keep the cache layout discoverable.
|
||||
|
||||
The current shape works — the constant indirection means the ESLint rule's "bare-array" check is technically satisfied — but it's the same drift class W3 was scoped to fix. One factory entry + one import + one usage swap.
|
||||
|
||||
### L2 — `eslint-plugins/healthlog/queryKey-factory.js:70` + `src/lib/__tests__/query-keys.test.ts:269`: `src/components/settings/about-section.tsx` listed redundantly
|
||||
|
||||
`about-section.tsx` is now covered by the `src/components/settings` directory entry W3 added. Both the ESLint plugin's `GUARDED_FILES` and the test-guard's `guardedRoots` still list `about-section.tsx` individually — pre-existing from v1.4.41 W-FRONTEND-FACTORY, but the W3 wave should have collapsed it. Drop the line in both places. Cosmetic, no behaviour change.
|
||||
|
||||
### L3 — `src/lib/withings/sync.ts:310-326`: `classificationToFailureKind` switch is mostly identity
|
||||
|
||||
Three of the four `case` arms map a value to itself: `case "reauth_required": return "reauth_required";`, `case "persistent": return "persistent";`, `case "transient": return "transient";`. Only `case "success":` collapses to a default. An identity-passthrough with a defensive default would read identically:
|
||||
|
||||
```ts
|
||||
export function classificationToFailureKind(c: WithingsClassification): FailureKind {
|
||||
return c === "success" ? "transient" : c;
|
||||
}
|
||||
```
|
||||
|
||||
The current switch is explicit and exhaustive, which the W6 author chose deliberately ("Defensive: a caller asking for the FailureKind of a success is a contract bug"). Pure preference — flagging as Low because the switch reads as ceremony for what's structurally a one-liner.
|
||||
|
||||
### L4 — `src/lib/workouts/canonical-rows.ts:226-255`: triple-pass picker reads as more clever than the algorithm needs
|
||||
|
||||
The survivor selection in `pickCanonicalWorkoutRows` annotates every group member, sorts, picks the winner, then reverse-looks-up the original by `origIndex`:
|
||||
|
||||
```ts
|
||||
const annotated = group.map((entry) => ({
|
||||
...entry,
|
||||
row: { ...entry.row, index: entry.row.index ?? entry.origIndex } as T,
|
||||
}));
|
||||
annotated.sort((a, b) => compareCandidates(a.row, b.row));
|
||||
const winner = annotated[0]!;
|
||||
const original = group.find((g) => g.origIndex === winner.origIndex)!;
|
||||
survivors.push(original);
|
||||
```
|
||||
|
||||
A `reduce`-style single-walk would skip the annotation pass + sort + reverse-lookup:
|
||||
|
||||
```ts
|
||||
let winner = group[0]!;
|
||||
for (let i = 1; i < group.length; i++) {
|
||||
// compareCandidates wants the row's `index` set; supply the fallback inline.
|
||||
const a = group[i].row.index !== undefined
|
||||
? group[i].row
|
||||
: { ...group[i].row, index: group[i].origIndex } as T;
|
||||
const w = winner.row.index !== undefined
|
||||
? winner.row
|
||||
: { ...winner.row, index: winner.origIndex } as T;
|
||||
if (compareCandidates(a, w) < 0) winner = group[i];
|
||||
}
|
||||
survivors.push(winner);
|
||||
```
|
||||
|
||||
The current shape is well-commented and the 12-test suite pins the contract, so the algorithm is correct. Style-only refactor. Skipping for v1.4.42 because the simpler form re-introduces the `... as T` cast at every comparison site — the simpler-looking code is not strictly cleaner.
|
||||
|
||||
### L5 — `pnpm knip` configuration hints: 28 cleanups outside the exports/types tier
|
||||
|
||||
`knip` reports 28 configuration hints (redundant entry patterns, ignored deps that match the project, etc.) that the W1 wave deliberately did not touch — W1 was scoped to the `exports` + `types` tiers. The cleanup is mechanical (remove redundant `next.config.ts` / `playwright.config.ts` entries; drop `@types/node` from `ignoreDependencies`; remove `src/generated/**` from `ignore` since `project` already excludes generated files). Defer to v1.4.43 — none of these surface false negatives in the new enforcing gate.
|
||||
|
||||
## Already-deferred items (acknowledge, do not re-flag)
|
||||
|
||||
Per W1 § "Reconcile callouts", W2 § "v1.4.43 backlog", W4 § "Items 4-5 (NO-OP)", W5 § "Strict rules honoured", and W6 § "Deferred to v1.4.43" — these are explicit follow-ups I am NOT re-flagging:
|
||||
|
||||
- The 41-route rollout of `returnAllZodIssues` across `src/app/api/**` (W2 backlog tables for iOS-contract / medication-CRUD / auth-settings-admin tiers).
|
||||
- The `medications/[id]/intake/import/route.ts` `Invalid format: ` prefix preservation (W2).
|
||||
- Sync-activity / sync-sleep catch-block migration to `err.classification` direct read instead of regex fallback (W6).
|
||||
- `parkIntegrationAtReauth` for persistent failures lasting > 24 h (W6).
|
||||
- CI integration of `pnpm check-env` against `.env.production.example` (W6).
|
||||
- Shadcn surface-area exports muted under `ignoreIssues["src/components/ui/**"]` (W1) — pending Marc's call.
|
||||
- `src/lib/validations/**` z.infer types muted (W1) — iOS-contract + OpenAPI gen.
|
||||
- The W4 NO-OP findings (`computeLongWindowSummary`, `ensureUserMedicationComplianceFresh` — both already removed pre-v1.4.42).
|
||||
- The W1 "fold contracts ignore-block" choice (matched the pre-existing convention).
|
||||
|
||||
The v1.4.41 simplifier-residual report's deferred items are also out of scope here — re-flag only if v1.4.42 made them worse.
|
||||
|
||||
## Strengths
|
||||
|
||||
- **W1 enforcing gate**: 35 → 0 unused exports + 52 → 0 unused types with the CI workflow now red on regression. The `ignoreExportsUsedInFile: true` choice (instead of muting individual zod schemas) correctly preserves the source-of-truth pattern where a schema is exported AND consumed via `z.infer<>` in the same file. The two-file ignore block is documented inline with rationale per directory.
|
||||
- **W3 lockstep discipline**: the ESLint plugin, the test-guard walker, AND the factory tests all extended in the same commit (`e2018b2d`). Past waves shipped one without the other and the next session paid for the drift — this one didn't.
|
||||
- **W4 BERLIN_DAY_FORMATTER hoist**: −101 / +32 net LOC and seven importers now share a single `toBerlinDayKey()` export. The header on `src/lib/tz/resolver.ts:64-75` documents both this helper and the per-user `userDayKey(date, tz)` siblings so the next "which day-key should I use?" lookup lands the right answer.
|
||||
- **W5 helper purity**: `pickCanonicalWorkoutRows` in `src/lib/workouts/canonical-rows.ts` is genuinely pure (zero Prisma imports, zero IO) and the 12-test suite + 3 batch-route tests pin every algorithm corner. The 90 s window choice + the `DEFAULT_WORKOUT_SOURCE_PRIORITY` reuse with the v1.4.25 W16a constant is documented in the header. The naming collision (M2) is the only blemish.
|
||||
- **W6 classifier purity**: `classifyWithingsResponse(httpStatus, body)` is a pure function with 27 tests pinning every documented Withings status code. The `WithingsApiError` subclass preserves the legacy `Withings <verb> error: <status>` message format so the v1.4.41 regex consumers in `sync-activity.ts`/`sync-sleep.ts` keep working. The migration path (typed throw + regex fallback in `classifyError`) is the exact "replace-with-fallback" pattern Marc's `feedback_read_swap_replace_not_parallel.md` memo prescribes.
|
||||
- **W6 env-check**: the manifest-driven shape with `required` / `allOrNone` / `anyOf` markers covers the v1.4.40 AP-2 silent-disable case (3/4 APNS_* set) without forcing every operator to read prose docs. The unit test surface (16 tests covering `parseEnvFile` quoting/CRLF/comments + `checkEnv` group semantics) keeps the helper trustworthy.
|
||||
- **Marc-voice authorship**: every commit message reads as Marc's, no `Co-Authored-By`, no `--no-verify`, no `--no-gpg-sign`, no "Marc"/"agent"/"marathon"/"phase" leakage. Six wave reports, six clean commit ranges.
|
||||
|
||||
40-word summary: Six waves landed clean. One nested-ternary regression in W6's three-state extension, one same-name picker collision with v1.4.30, one missed BERLIN_DAY_FORMATTER dedup, plus light boilerplate sharing wins. No blockers; mechanical follow-ups for v1.4.43.
|
||||
Reference in new issue
Block a user