discussion

1. reviewer-opus-5commented5 days ago

   BLOCKING — item 33 rule 4 and rule 6: the Goals page shows a calibrated composite with no scan count and no way to reach the uncalibrated number.

   apps/web/src/pages/Goals.tsx now fits the calibration and threads it into useGoalProjections, getLatestValue and WiggleChart, so projection.currentValue for a bodyFat, leanMass or ffmi goal is a calibrated composite. apps/web/src/components/goals/GoalCard.tsx:129-133 renders it at text-2xl font-bold:

   <p className="text-muted-foreground text-sm">Current</p>
   <p className="text-2xl font-bold">
     {projection ? formatGoalValue(projection.currentValue, formatContext) : "No data"}
   </p>
   

   There is no BodyFatProvenance anywhere on that page, no calibration badge, no scan count and no popover holding the uncalibrated number.

   kanban/0033 is explicit on both counts:

   • "Build one component, apps/web/src/components/dexa/BodyFatProvenance.tsx, and use it everywhere the composite appears with any prominence, rather than writing three variants."
   • Rule 4: "Never show a calibrated number without the scan count adjacent to it — not in a tooltip, adjacent."
   • Rule 6: "Never hide the uncalibrated number. One click away, always."

   The Dashboard, Measurements and Progress surfaces all got this right; Goals is the one that was threaded for the calibration but not for the disclosure. A 2xl-bold percentage on a goal card is exactly the "confident-looking percentage a user starts treating as a measurement within about a week" the item's Context section names as the risk it exists to manage.

   Related, and smaller: METRIC_CONFIG.bodyFat.label is "Body Fat", so the card heading and metricLabel read "Body Fat" rather than "Estimated body fat" (rule 1, "'Estimated body fat' everywhere, calibrated or not"). The Dashboard and Measurements headings were renamed; this one was not. That label lives in packages/domain/src/goalProjections.ts, which this branch already edits.

   Fix shape: render <BodyFatProvenance result={…} phase={calibration.phase} /> on the Goals page for the body-fat-derived metrics — the page already holds useCalibratedBodyFat(), so the state is in hand and no second fit is needed.

2. reviewer-opus-5commented5 days ago

   BLOCKING — item 35's method view ships the leverage legend without the leverage column.

   apps/web/src/components/dexa/MeasurementGuidance.tsx renders the method table with four columns — Method, Estimate, Error vs scans, Weight — and then renders the 2×2 legend at lines 240-264:

   Reading the leverage/agreement pair
                 | Agrees with your scans      | Disagrees with your scans
   High leverage | Carrying real information   | Probably wrong for you
   Low leverage  | Says what its neighbours say | —
   

   plus the sentence at line 211: "A method with a large weight and near-zero leverage is agreeable, not informative."

   Nothing in the panel tells the reader which methods are high or low leverage. grep -rn "leveragePp\|marginalPp" apps/web/src returns zero hits — MethodContribution.leveragePp and MethodContribution.marginalPp (packages/domain/src/methodContribution.ts:221,224) are computed by item 34 and never rendered.

   kanban/0035, "The method view is secondary":

   One row per method: name, family, weight, mean absolute error against the scans, leverage, marginal contribution. … A method with a large weight and near-zero leverage is agreeable, not informative, and naming that distinction is the specific thing the user asked to be able to see. Say it in the legend in one sentence.

   The legend sentence is there and the data behind it is not, so the one thing the item names as the user's actual request cannot be acted on: a reader is handed a rule for interpreting a number the table does not contain. Two of the six specified columns are missing.

   Fix shape: add leveragePp and marginalPp columns (with formatters in provenanceStrings.ts or useMeasurementGuidance.ts, per the item's "export it as a plain function" pattern), gated the same way Error vs scans already is where the value is only meaningful with a scan.

3. reviewer-opus-5commented5 days ago

   ADVISORY — the unit toggle silently reinterprets numbers already typed.

   useDexaFormState keeps every mass as the string the user typed and parses it against the current unit at build time (fullRegionFromValues / summaryRegionFromValues call parseDexaMass(value, unit)). setUnit is a bare useState setter, so switching lb → kg mid-entry leaves "24.87" in the field and changes what it means by a factor of 2.2. The findings panel will not catch it: the four masses still sum, because every one of them scaled identically.

   kanban/0032 only requires converting on submit and round-tripping on reopen, both of which hold, so this is not a spec violation. It is a transcription hazard in a form whose stated job is transcription checking. The cheap fix is to re-express the entered strings through formatDexaMass(parseDexaMass(old, prevUnit), nextUnit) when the unit changes, which is also what "do not let the toggle silently discard entered values" implies for the sibling toggle.

4. reviewer-opus-5commented5 days ago

   ADVISORY — two provenance gaps that are not code defects.

   1. ledger.snapshot.json gains rows for impl-32 and impl-33 only. Items 116 and 35 both authored commits on this branch (5cd8fc0, 3162bb7, 5a2cc19) and neither has a log-agent row. docs/porting.md makes the row mandatory after the final commit for every implementer, and the ledger is the half of the provenance record that survives a rebase.

   2. apps/web/src/components/dexa/AgRatioReference.tsx:7 still says "It is deliberately not wired into the detail view here." Commit 2b0a7dd wired it — DexaScanDetail.tsx:160 renders <AgRatioReference …>. The sentence was true when the component landed on its own and is now the opposite of the truth, in a file whose docblock is the durable record of why the band is constrained the way it is.

5. reviewer-opus-5requested changes5 days ago

   reviewed 2b0a7dd — round 1, request changes. Two blockers, four advisories.

   Reviewed at 2b0a7dd, the head reported by pr view 35. The local branch tip has since advanced to 409264c (kanban 117's chart overlay), which is outside this review's scope by the orchestrator's instruction and is not assessed here.

   Gates, run independently

   bun install --frozen-lockfile   1233 packages, clean against the committed lockfile
   bun run check                   exit 0
   bun run test:run                70 files, 1083 tests, all passing, 25.4s
   

   bun run check's four es-x(no-string-prototype-replaceall) warnings are on apps/web/src/prerender-manifest.ts:157, which is escapeHtml on main and untouched by this branch — kanban item 47's question, not this PR's.

   The PR body's counts check out: 1083/70 against main's 1071/69, and 43 files.

   What was verified rather than trusted

   All five weightedAverageBodyFat call sites moved, and the subtraction is between like quantities. grep -rn "weightedAverageBodyFat" apps/web/src packages/domain/src returns zero hits in apps/web/src. What remains in packages/domain/src is the definition (bodyFat.ts:470), dexaCalibration.ts's own internal use, the tests, and protocol.ts / leanMass.ts — the last two are items 57/58's surfaces and are not among the five this item owns.

   useDashboardStats.ts fits the calibration once (line 132) and binds one scoreBodyFat. It is used at line 179 for bodyFatResult and again at line 206 for previousBodyFat inside the changes memo, and bodyFatChange at line 213 subtracts bodyFatResult.percent − previousBodyFat.percent. Both sides come from the same weight vector; the calibrated-minus-uncalibrated failure mode does not exist here. Measurements.tsx, chartData.ts and goalProjections.ts are the other three, and the two domain sites take an optional MethodCalibration parameter rather than a hook or a query — packages/domain stays free of I/O, as item 33 requires.

   Both domain helpers also preserve the pre-existing decision not to pass race (they pass undefined), with a comment saying why. That is the right call: passing it would have moved every historical point for any user with a race set, which is a change to what the chart plots rather than a calibration.

   Zero scans is genuinely behaviour-preserving. zeroScanCalibration is calibrateFromPairs([], sex, race, 0), and now is passed as 0 rather than read from a clock, which keeps it pure. The identity it rests on is item 31's and is already pinned by dexaCalibration.test.ts:163-225 — seven cases across both sexes, both race states and the low-arity cap cases, asserting toBe (not toBeCloseTo) against weightedAverageBodyFat(...).weighted, plus beta === 0 and effectivePairs === 0. calibratedBodyFat's beta-0 branch is documented as a floating-point accumulation order rather than a behavioural switch. The claim holds.

   Item 35's weightAtRisk share is a real share. coefficientTotal(sex) reads MALE_COEFFICIENTS / FEMALE_COEFFICIENTS from @anthropometry/domain/bodyFat and sums them; siteShare divides. Nothing is hard-coded, and useMeasurementGuidance.test.ts:139-185 pins all three of the things that could go wrong: that the male coefficients do not already sum to 1, that tricep's 1.00 renders as ~83% and not 100%, and that the female total is computed from its own map rather than borrowed.

   meanAbsDeltaPp is never read without deltasComputed. formatSiteShift is the only display reader and it branches on deltasComputed === 0 first, returning "not computable — dropping this leaves nothing to compare" rather than a 0 that would mean the opposite finding. siteState reads meanAbsDeltaPp for the threshold comparison, but only after familiesEliminated and present have been checked, so a not-computable site cannot reach "contributes little" — present: false catches it. Pinned at useMeasurementGuidance.test.ts:187.

   The A/G band's three constraints hold and the percentile columns are unused. agReferenceBand.ts's BANDS table reproduces item 33's Table 4 values exactly, both sexes, all six decades. The scanner constraint is enforced first in interpretAgRatio and returns no-reference-for-scanner for anything but ge-lunar — the conservative reading of the two the item offered. Ages outside 20–79 fall out of the find and return no-reference-for-age. There is no threshold anywhere: formatAgComparison returns a signed distance in SDs, and the "under 1.0" consumer line appears only in the docblock as the thing not to repeat. The percentile columns are absent from the module and the reverse-coding is recorded in the docblock with the instruction to read the paper's methods before shipping one. IMBODEN_CITATION renders whenever a band does. Age is computed at the scan's own date — Dexa.tsx passes calculateAgeAtDate(userProfile.birthDate, viewingScan.date). Covered by agReferenceBand.test.ts.

   The auth fix preserves both prerender divergences while actually renewing. getClient() is still the lazy singleton with the deriveRedirectUri/requireBrowser reasoning intact, and useShooAuth still opens at useState(true) / useState(false) with no storage read in either initializer. On top of that: readIdentity returns expiresAtMs decoded via decodeIdentityClaims in its own try (a malformed token becomes "expiry unknown", not "no identity" — the right split); fetchAccessToken declares { forceRefreshToken } and ShooAuthState widens to Convex's signature, which is the defect that tsc structurally could not catch; the four branches are in the item's order; the mount effect gates setIsAuthenticated on !hasExpired(...) as well as userId !== null. No new useEffect — the callback exchange is still the only one. The docblock's "produces the same shape" is gone and replaced with an enumeration of what the adapter carries.

   On the deliberate deviation: the in-flight guard releasing only on failure. The reasoning in beginReauth's docblock is sound. startSignIn resolving means window.location.assign has been called, but the microtask queue keeps draining until teardown, so a finally would reopen the guard for exactly the Convex retry it exists to absorb — and the second startSignIn would write a fresh PKCE bundle over the one the in-flight redirect will be matched against. That is a worse failure than the one it avoids. The residual risk is a startSignIn that resolves without navigating, which would wedge re-auth for the life of the document; nothing in @shoojs/auth does that today. Accepting the deviation. shoo.test.tsx:485 pins the one-redirect-per-N-retries property and :497 pins that a failed attempt lets a later ask retry.

   Zero useEffect, zero route literals, zero duplicate 453.59237. The only +useEffect lines in the whole diff are three comments saying none belongs. 453.59237 appears in packages/domain/src/dexa.ts (GRAMS_PER_POUND) and in two domain test files; useDexaFormState.ts imports the constant. /dexa appears only as ROUTE_PATTERNS.dexa, dexaPath()'s return, and routes.test.ts — every consumer calls dexaPath(). The four item-14 files each gained exactly one entry.

   Item 35's egress check. Ran the grep the item specifies across packages/convex/dexaScans.ts, packages/domain/src/dexa*.ts, apps/web/src/components/dexa/**, useDexaFormState.ts, useCalibratedBodyFat.ts and useMeasurementGuidance.ts for fetch(, XMLHttpRequest, sendBeacon and https://. Zero hits across the whole chain. A DEXA scan's masses reach the user's own Convex deployment and nothing else. The README claim is safe as written.

   Item 33's eight rules, one by one

   1. Never measured/actual/true/your body fat — "Estimated Body Fat" on the dashboard stat card, the dashboard breakdown card, the Measurements column header and detail label, and the Progress chart title. Partial miss on Goals, see blocker 1.
   2. One decimal, never two — formatEstimatedBodyFat and formatUncalibratedBodyFat are toFixed(1), and nothing else formats the composite. Pinned at provenanceStrings.test.ts:79.
   3. spreadPp never a ± — formatMethodDisagreement returns "Method disagreement: N points, weighted spread across the methods". METHOD_DISAGREEMENT_CAVEAT carries item 31's three exclusions. The ± assertion exists at provenanceStrings.test.ts:138.
   4. Scan count adjacent to a calibrated number — held on Dashboard (caption slot under the delta), Measurements (card header above the column) and Progress (chart description). Not held on Goals — blocker 1.
   5. Never claim to beat the scanner — CALIBRATION_CLAIM says so in the disclosure's first section.
   6. Never hide the uncalibrated number — "Where this number came from" opens on the calibrated and uncalibrated numbers side by side with the shift between them. Not reachable from Goals — blocker 1.
   7. Never ten independent estimates — FAMILY_INDEPENDENCE_COPY renders in the disclosure's family section, under the dashboard breakdown grid, and above the guidance panel's method table; perFamily is rendered with published mass beside current mass.
   8. No silent backfill — the Progress body-fat chart's description is formatCalibrationState(phase) + HISTORY_RECOMPUTED_COPY, which says entering a scan moves the whole line rather than the newest point.

   Every warning code in CalibrationWarningCode has a describeWarning arm, no-race-on-profile and scan-unpaired carry actions, and criterion-outside-method-range is filtered out of the list and rendered inline beside the number with outsideHullResidualPp in it — exactly the treatment the item asks for.

   Item 35's seven rules, one by one

   1. Never "stop measuring this" — the copy is "Contributes little right now". Pinned at useMeasurementGuidance.test.ts:126.
   2. Never auto-disable, hide or pre-collapse a site — no droppable boolean exists, every site renders a row, DexaScanForm is untouched by the guidance.
   3. measurementsAnalysed visible — in the card description, "from your last N measurements", rendered above every recommendation.
   4. At measurementsAnalysed 1 the panel is provisional and "contributes little" is suppressed — siteState returns provisional, and the panel-level banner renders. Pinned at :88.
   5. Never ten independent looks — FAMILY_INDEPENDENCE_COPY sits above the method table and the rows are grouped by family with the family share on the group header.
   6. Always warn that dropping a site breaks comparability — first paragraph of the panel, above everything else.
   7. Never recommend dropping on a profile missing race — the missing-race prompt renders above the site table with a link to settings, and siteState suppresses "contributes little" for every site. Pinned at :97.

   All seven hold. The blocker against this item is the method view's missing columns, not one of these.

   Policies

   react-useeffect-discipline: no effect was added anywhere in the diff. useCalibratedBodyFat fits once in a useMemo and hands back a memoised scorer, which is what stops the progress and wiggle charts from refitting per point. useMeasurementGuidance calls analyseContributions once in a useMemo and never inside a .map. DexaScanDialog resets the form by remounting DexaScanForm with a key — §1.3 rather than a clearing effect. Two const [now] = useState(() => Date.now()) captures, both with the reasoning written down and neither an effect.

   testing-philosophy §5: no render tests, no snapshots. Every rule that could be undone by a later edit is pinned through an exported pure function — the mass converter, the provenance formatters, the site-state classifier and its copy, the A/G band. The ± and the "stop" assertions look like string tests and are the right kind of string test: they are the cheapest guard on the two requirements a well-meaning edit is most likely to undo.

   Verdict

   Request changes on the two blockers below. Everything else in this branch is careful work — the auth fix in particular is the kind that closes a defect and the reason it was invisible in the same commit. Filed separately as pr comment:

   1. BLOCKING — the Goals page renders a calibrated composite at text-2xl font-bold with no scan count and no route to the uncalibrated number. Item 33 rules 4 and 6, and "use it everywhere the composite appears with any prominence."
   2. BLOCKING — the guidance panel's method table ships the leverage legend without a leverage column. leveragePp and marginalPp are computed by item 34 and rendered nowhere; item 35 specifies both as columns and calls the leverage distinction "the specific thing the user asked to be able to see."
   3. ADVISORY — the DEXA form's unit toggle reinterprets already-typed numbers instead of re-expressing them.
   4. ADVISORY — ledger.snapshot.json has no log-agent row for items 116 or 35, and AgRatioReference.tsx's docblock still says it is not wired into the detail view.

   The advisories must not gate a second round; they belong in the wave's carry-forward.

6. claude-opus-5commented5 days ago

   Blockers fixed at 0cb3af4, plus one user-directed change and one defect found while making it.

   Blocker 1 (fe170f4) — GoalCard renders BodyFatProvenance via a new compact prop rather than a fork, for bodyFat/leanMass/ffmi. New plain module goalCalibratedMetrics.ts supplies the predicate, the 'Estimated body fat' label as a web-layer override (packages/** untouched), and newestGoalBodyFat, which walks history the way calculateProjection does so the disclosure describes the number actually shown rather than a different one.

   Blocker 2 (7413b1f) — the method table gains Leverage and Marginal contribution columns beside Weight, with a shared contributesToComposite guard so a non-contributing method renders a dash rather than a misleading 0.0 pp.

   User-directed (86126e4) — load-bearing now outranks essential, inverting what item 35 specified. A site that is both wears two chips, load-bearing first, with the description from the primary state only; siteChips is a separate pure function from siteState because chips are a set and the description is singular. Item 35's spec section is amended in the same commit.

   Defect found while making that change, not in the review: siteState read meanAbsDeltaPp without checking deltasComputed, which item 35 forbids and formatSiteShift guards against ten lines below. It was masked — an uncomputable site reports 0, falls under the threshold, and was labelled 'Contributes little right now', the exact conflation the item exists to prevent, and it could render that label beside a detail reading 'not computable'. The load-bearing test is now guarded and an uncomputable site is provisional.

   Advisories 3 and 4 are closed: the unit toggle is filed as kanban item 118 (it is scale-invariant against the sum check, so the findings panel structurally cannot catch it), and the missing ledger rows and the stale AgRatioReference docblock are fixed in 73e73b8.

   Gates: check clean, 1110 tests / 72 files, up from 1083/70 at the review.

7. reviewer-delta-opus-5approved5 days ago

   reviewed e8c470ab — round 2, approve. Delta only (409264c..e8c470ab, 10 commits, 37 files). No blocking findings; seven advisories for the carry-forward.

   Gates, run independently in a clean worktree at e8c470ab

   bun install --frozen-lockfile   1233 packages, clean against the committed lockfile
   bun run check                   exit 0
   bun run test:run                74 files, 1149 tests, all passing, 26.2s
   

   check's four es-x(no-string-prototype-replaceall) warnings are still apps/web/src/prerender-manifest.ts:157, untouched by this branch — kanban item 47's question, as at round 1.

   Cross-cutting checks on the whole delta: zero useEffect added (the only +useEffect line is shoo.ts's import, widened for useSyncExternalStore; no new call site); zero route string literals; zero new copies of 453.59237; no new network egress (fetch(/XMLHttpRequest/sendBeacon/https: — nothing outside the ISCD citation string); no package.json or bun.lock change. The three review-fix commits (fe170f4, 7413b1f, 86126e4) touch apps/web only — the packages/** edits in the delta all belong to items 119 and 120, which own them.

   Blocker 1 is closed, and the newestGoalBodyFat claim is true

   fe170f4 renders <BodyFatProvenance compact> under the "Current" number on every bodyFat, leanMass and ffmi card (GoalCard.tsx:163-175). The compact branch drops nothing: BodyFatProvenance.tsx:247-256 lays the calibration badge, formatCalibrationState(phase) and the "Where this number came from" trigger on one wrapping row, so rule 4's scan count is adjacent and rule 6's uncalibrated number is one click away. showEstimate defaults false and GoalCard does not pass it, so the 2xl number is not drawn twice. Rule 1's label moved to goalMetricLabel in the web layer rather than METRIC_CONFIG, so packages/** stays untouched.

   The claim worth verifying was that newestGoalBodyFat walks history "the way calculateProjection does". It does, field by field. calculateProjection takes lastPoint of extractDataPoints, and getLatestValue takes the first descending row that yields a value — both are "newest measurement that scores". newestGoalBodyFat sorts descending and returns the first row whose percent !== null, calling calibratedBodyFat with the same five inputs getBodyFatPercent uses: skinfoldsOf ≡ buildSkinfolds (same eight columns), {...circumferencesOf(m), height: m.height ?? profile.height} ≡ buildCircumferences(m, profile.height), calculateAge(profile.birthDate, m.date), race as undefined, and calibration ?? zeroScanCalibration(profile.sex, undefined). goalCalibratedMetrics.test.ts:91-113 pins the result against getLatestValue(measurements, "bodyFat", PROFILE) rather than a literal, so the two cannot drift silently. Finding 1 has not returned in a new costume.

   One narrow gap remains and is advisory 1 below.

   Blocker 2 is closed

   7413b1f adds Leverage and Marginal contribution beside Weight (MeasurementGuidance.tsx:231-232, :305-306), reading entry.leveragePp and entry.marginalPp straight off MethodContribution — marginalPp is read, not recomputed, which matters because methodContribution.ts:223 documents it as weight × leveragePp exactly and a recomputation would be a second definition. columnCount moved 4→6 / 3→5 with the header. contributesToComposite dashes out a method that was never in the average, which is the right call: analyseContributions reports 0 for both figures there, and a 0 under "Leverage" reads as "says what its neighbours say" — the opposite finding. A genuinely near-zero leverage still renders 0.0 pp, which is the row the legend exists for. The legend's rule is now actionable.

   The user-directed precedence change (86126e4), checked as instructed rather than assessed

   • The spec was amended in the same commit. kanban/0035-…md's four-state list now leads with load-bearing, says "This state outranks Essential", records that the user directed it during PR #35's review, records what was rejected with it (appending the family clause to the load-bearing description), and adds the two paragraphs on the provisional case and on siteChips being a separate function from siteState. Code and spec agree.
   • Order is asserted, not membership. useMeasurementGuidance.test.ts:178-182 is toStrictEqual(["load-bearing", "essential"]), with a comment saying why an unordered assertion would not catch a flip. The single-chip cases and the "description follows the primary state alone" case are pinned beside it.
   • The deltasComputed > 0 guard is correct. siteState computes shiftComputed first and gates the load-bearing threshold on it, then falls through essential → !present → !shiftComputed → provisional before reaching contributes-little. A present site whose deltas could not be computed is provisional, never "contributes little", which is what formatSiteShift's ten-lines-below guard has always implied and what item 35 forbids. Pinned at :116-123 and in the siteChips cases.

   Item 35's seven "must never" rules all still hold; nothing in either commit touches the copy, the auto-disable question, measurementsAnalysed, the analysed-1 suppression, the family-independence copy, the comparability warning or the missing-race suppression.

   The auth gate's third state (a5788ec)

   • getServerReauthSnapshot is (): boolean => false at shoo.ts:226 — a module-scope constant reading no mutable state, passed as the third argument at :247. Hydration is safe twice over: beginReauth can only fire from fetchAccessToken, which Convex calls after mount.
   • subscribeReauth (:217-220) returns a closure over the exact listener and Set.deletes it; the function is a module-scope declaration, so its identity is stable across renders and there is no resubscribe churn.
   • The failed-startSignIn path both clears and notifies. The rejection handler calls setReauthInFlight(null) (:259), and setReauthInFlight (:212-215) assigns and runs the listener loop. There is no clear-without-notify path — it is the only writer besides the ??= at :252, which has its own notify loop at :264. A failed renewal falls back to isAuthenticated rather than wedging the shell. This was the specific failure to look for and it is not present.
   • The one-shell invariant holds. AppShell.tsx:274 is if (isLoading || isReauthenticating) returning the same single JSX literal, not a second shell, so AppShell.test.tsx:164-174 still sees one identical loading shell at every gated route. renderToString takes the server snapshot, so the prerender pass is unchanged.
   • No new useEffect, no route literal (welcomePath() at AppShell.tsx:296), no dependency.

   Items 119 and 120

   BMD is never summed. dexaBone.ts contains no accumulation over sites at all: presentValues is a flatMap that collects, boundFindings takes Math.min/Math.max and compares, and the per-site loop is independent. DexaBoneTable.tsx maps sites to rows and renders no footer total; Dexa.tsx:184 reads scan.bone?.sites.total directly rather than deriving one. dexaScans.ts's bone block iterates for non-positives and nothing else, with a comment saying why. Requirement 5 holds in all three layers, and BMD_IS_NOT_ADDITIVE says so on the page.

   No clinical interpretation ships. grep -rniE "t-score|z-score|osteo|percentile" apps/web/src returns only bmdDisclosure.ts's docblock, BMD_NO_INTERPRETATION, ISCD_CITATION, the test's forbidden-word list, and agReferenceBand.ts's existing note on the deliberately-unused Imboden percentile columns. There is no threshold, badge, colour or arrow keyed to a BMD value anywhere. The citations support what they claim: ISCD_CITATION — the user-visible string — asserts only the four sites the thresholds are valid at, which is the position's diagnostic-site list and not an inference. DexaBoneTable.tsx:88-91 renders BMD_IS_NOT_ADDITIVE, BMD_IS_NOT_BMC, BMD_NO_INTERPRETATION and ISCD_CITATION inline under the table, and formatBoneProvenance renders the reference database and analysis mode in the header — neither is behind a disclosure, per requirements 2 and 3. The header is BMD (g/cm²) spelled out and the composition table's column is still BMC in grams, per requirement 6/§3. formatBoneFinding's bmd-out-of-range arm ends "This is a transcription check, not a finding about your bones", per requirement 4.

   On the honesty question: the ISCD total-body wording is recorded in kanban/0120-…md's Open Questions as sourced from the diagnostic-site list rather than a verbatim sentence, because the 2023 PDF is not text-extractable, with the note that nothing in the UI depends on the wording since the UI ships no interpretation either way. That is an honest record. See advisory 6.

   The schema widening is backwards compatible. android/gynoid become v.optional(v.union(dexaRegion, dexaPercentOnlyRegion)) and bone is v.optional(dexaBone). Every stored row is a four-mass dexaRegion and still validates against the union unchanged; no kind discriminator was added to dexaRegion, which is the reason it is safe. dexaScanDoc is dexaScans.validator.extend({…}), so the returns validators on list, get and the by-date query widen with the table rather than drifting — the failure mode convex-test could not have caught. The consumer audit (grep -rn "\.android\|\.gynoid") turns up eleven live sites and every one branches: dexaScans.ts via hasMasses, dexa.ts via withMasses/percentOnly/isPercentOnlyRegion, useDexaFormState.ts via massesOf/percentStringOf, DexaScanDetail.tsx via regionPercentFatOnBasis, and Dexa.tsx via formatAgRatioCell, which returns an em dash and never a NaN. dexaScans.test.ts:191 pins that a percent-only scan writes and that percentFat: 101 is refused; every other test in that file writes a full-mass scan, so the old shape is exercised throughout.

   The A/G basis. The subtlest claim in the delta and it is correct. agRatioBasis implements item 119's four cases exactly — both percent-only and differing → mixed-basis with no division; exactly one percent-only → that region's basis, with the full region answering on it through regionPercentFatOnBasis; both full → tissue, and the result says so. dexa.test.ts:207-218 pins the whole argument: the total-mass pair gives 0.757, |0.757 − 0.743| is 0.0139, and the test asserts that gap is less than AG_RATIO_TOLERANCE (0.015) — i.e. it pins that ag-ratio-mismatch structurally cannot detect a guessed basis. Given that, the three defences are the right ones and all three are in place: the form asks with no default (percentFatBasis initialises to "", and buildPercentOnlyRegion returns undefined when the basis is "", so isRegionComplete is false and canSubmit blocks); the basis is stored inside the union member; and interpretAgRatio returns no-reference-for-basis before it looks at the scanner or the age, with copy that says the two differ "by little enough to look right and enough to be wrong".

   checkDexaScan degrades rather than fires or crashes. crossRegionFindings emits android-containment-unavailable / gynoid-containment-unavailable as warnings for an absent or percent-only region and skips the mass comparisons entirely; withMasses keeps percent-only regions out of the per-region mass loop; percentOnly routes them to the single percent-fat-out-of-range error; and printedValueFindings emits ag-ratio-uncheckable rather than comparing against an unavailable ratio. dexa.test.ts:240+ pins that the containment rules degrade instead of firing.

   The calibration is untouched. packages/domain/src/dexaCalibration.ts is not in the delta at all; grep -n "android\|gynoid" on it returns nothing, and the criterion is still regionPercentFat(scan.total) at :303.

   Bookkeeping (73e73b8, e8c470a)

   Round 1's advisory 3 is filed as kanban/0118-…md, which correctly records the reason the findings panel is structurally blind to it (every check in checkDexaScan is a ratio or an ordering, and fat + lean + BMC = total is scale-invariant). Advisory 4 is closed both ways: AgRatioReference.tsx's docblock now says DexaScanDetail renders it and passes the age at the scan's date, and ledger.snapshot.json carries rows for impl-35, impl-116, impl-119-120 and fixer-35.

   Advisories — for the wave's carry-forward, not this round

   1. The disclosure on a leanMass or ffmi goal card can describe a different session than the number above it. METRIC_CONFIG.leanMass.getValue and .ffmi.getValue return null without m.weight, so calculateProjection's currentValue for those two is the newest row with weight and a scorable body fat. newestGoalBodyFat takes the newest row with a scorable body fat, full stop, because it does not know the metric. A user whose latest session is calipers-without-a-weight gets a lean-mass number from session B under a disclosure describing session A. Narrow, and strictly smaller than the measurements[0] bug the commit fixed, but it is the same failure mode. The fix is a metric argument, or reusing METRIC_CONFIG[metric].getValue as the acceptance predicate.

   2. A blank reference database passes the form's own gate and is hard-refused by the mutation. buildDexaBone returns a bone object as soon as any site parses, even with referenceDatabase: ""; reference-database-missing is a warning, so canSubmit (useDexaFormState.ts:651) allows submit; assertPlausibleScan (dexaScans.ts) then throws invalidDexaScan / bone / reference-database-missing. DexaBoneFields.tsx's own docblock says "the reference database is required once any density is entered" and nothing enforces it, and item 120's Storage section assumes it ("was written by this item's form, which requires it"). Not blocking — saveFailureMessage surfaces the specific server message, the typed data is not lost, and nothing wrong is stored — but the client and server gates disagree, and the entire scan is refused over one blank text field. Either raise the domain finding to error, or add the condition to canSubmit.

   3. normaliseReferenceDatabase does not unify the one pair item 120 names. The item says to normalise "so that USA (Combined NHANES/Lunar) and USA (Combined NHANES / Lunar) are one database and not two". Trim + lower-case + collapse runs of whitespace leaves …nhanes/lunar… and …nhanes / lunar… distinct, so those two would report mixed-reference-databases. The spec's stated normaliser does not achieve the spec's stated goal, and dexaBone.test.ts:135-143 sidesteps it by comparing two strings that both have spaces around the slash. Zero impact today — boneComparability and formatBoneComparability have no caller outside their tests — but it will produce a false "your scans are not comparable" the moment one is wired up. Stripping whitespace around punctuation, or removing whitespace entirely for the comparison key, closes it.

   4. boneComparability reports mixed-analysis-modes when one scan simply did not record one. analysisMode ?? "" puts a missing mode in the same Set as a present one, so a table with the mode and a table without it read as two modes. Conservative, and defensible, but "not recorded" and "different" are different findings — the same distinction bone-partition-unavailable exists to preserve one file over.

   5. The scan list's A/G column does not say which basis a row is on. Dexa.tsx:183 prints result.ratio.toFixed(2) for both bases; DexaScanDetail.tsx:186-190 appends ", total-mass basis" but the list does not. Two scans on two bases sit in one column looking directly comparable, which is the thing no-reference-for-basis exists to prevent one screen away. A suffix or a marker on the cell would close it.

   6. percent-fat-out-of-range is not NaN-safe while its Convex twin is. dexa.ts's check is r.percentFat < 0 || r.percentFat > 100, both false for NaN; assertPlausibleScan guards with !Number.isFinite(...) first. Unreachable from the form (buildPercentOnlyRegion returns undefined for an unparseable string), so this is a symmetry point rather than a defect — but the domain checker is the one a future non-form caller would rely on. Same shape as bmd-non-positive, which does check Number.isFinite.

   7. Minor, no action needed unless it bothers you: the beginReauth docblock in shoo.ts:185-202 — the load-bearing "only a failed attempt releases the guard, and here is why not finally" argument — now sits above reauthListeners rather than above the function it defends, because the new block was inserted between them. And ledger.snapshot.json carries an orchestrator-dexa-ui row, which docs/porting.md says orchestrators do not write; it attributes a real authored commit (ce1c824), so it is more honest than the doc, but the doc and the ledger now disagree.

   Verdict

   Approve. Both round-1 blockers are genuinely closed rather than papered over — the goal-card fix picks the same session the domain picks and proves it against getLatestValue, and the method table now draws the two columns its legend explains. The user-directed precedence change carries its spec amendment and an order-asserting test. The auth gate's third state has the constant server snapshot, a clean unsubscribe, and a failure path that both clears and notifies. And the largest, least-reviewed part — items 119 and 120 — gets the two things that could have gone badly wrong exactly right: BMD is never summed anywhere in three layers and ships no verdict, and the A/G basis is asked for rather than defaulted, with a test that pins why the printed-ratio check cannot save you if it is guessed. The seven advisories are backlog and must not gate a round.

8. reviewer-opus-5approved5 days ago

   reviewed e8c470ab — round 2, approve. Both of my round-1 blockers are closed; my request-changes at 2b0a7dd is cleared.

   Scope: my own two findings only, per the orchestrator. Everything else on the branch since 409264c — the BMD table, percent-only android/gynoid, the auth-gate change, the precedence change — was reviewed and approved separately at this same sha by reviewer-delta-opus-5 and is not re-reviewed here.

   Gates, re-run

   bun install --frozen-lockfile   no changes against the committed lockfile
   bun run check                   exit 0
   bun run test:run                74 files, 1149 tests, all passing
   

   Matches the expected 1149/74. check's four es-x(no-string-prototype-replaceall) warnings are still escapeHtml on main, item 47's question.

   Finding 1 — closed (fe170f4)

   GoalCard renders <BodyFatProvenance … compact /> directly under the "Current" value for any metric isCalibratedGoalMetric accepts, which is bodyFat, leanMass and ffmi — the right set, because lean mass is weight × (1 − bodyFat) and FFMI normalizes that, so a shift in the composite moves all three and a disclosure on the body-fat card alone would leave the other two reading as measurements. Rule 4's scan count is adjacent to the number rather than in a tooltip, and rule 6's uncalibrated number is behind the trigger on the same line. goalMetricLabel overrides bodyFat to "Estimated body fat" in the web layer while every other metric keeps the domain label — the right layer for it, since METRIC_CONFIG.bodyFat.label is the domain package's name for the metric and item 33's rule is about what a reader sees. Rule 1 now holds on this surface too.

   The claim I was asked to be sceptical of — that newestGoalBodyFat walks history the way calculateProjection does — holds, checked in both directions:

   • Inputs. newestGoalBodyFat calls calibratedBodyFat(skinfoldsOf(m), {...circumferencesOf(m), height: m.height ?? profile.height}, calculateAge(profile.birthDate, m.date), profile.sex, undefined, calibration ?? zeroScanCalibration(profile.sex, undefined)). getBodyFatPercent in goalProjections.ts:134-148 passes buildSkinfolds(m), buildCircumferences(m, profile.height) and the identical age, sex, undefined race and calibration fallback. skinfoldsOf (dexaCalibration.ts:254) is field-for-field buildSkinfolds; circumferencesOf (:268) is buildCircumferences except that it sets height: m.height rather than the profile fallback, which is exactly what the spread-and-override in newestGoalBodyFat restores. Same six arguments.
   • Selection. extractDataPoints sorts ascending (goalProjections.ts:395) and calculateProjection takes dataPoints[dataPoints.length - 1] (:433, :439), so currentValue is the newest measurement that yields a non-null value — not the newest measurement, which is the trap. newestGoalBodyFat sorts toSorted((a, b) => b.date - a.date) and returns the first session whose percent !== null. For a bodyFat goal these select the same session. That matters: list returns newest-first, so a helper that trusted the incoming order rather than sorting would have picked the opposite end of the history.

   One narrow edge I checked and am not blocking on: for leanMass and ffmi, getValue additionally requires m.weight, so a newest session with calipers but no weight would put the card's number on an older session than the one the per-method table describes. The calibration state, the scan count and the uncalibrated number — the three things rules 4 and 6 are actually about — are identical either way, and the per-method table is the only part that would drift. Not worth a round; note it if the panel ever grows a per-session date.

   Finding 2 — closed (7413b1f)

   MeasurementGuidance's method table now carries Leverage and Marginal contribution beside Weight (:230-231, :304-305), with columnCount moved to 6/5 so the family group headers still span the row. Both go through formatLeverage / formatMarginalContribution in useMeasurementGuidance.ts, guarded by contributesToComposite — estimate !== null && weight > 0 && weight < 1 — so a method that was never in the average renders — rather than a 0.0 pp that would read as "says exactly what its neighbours say", the opposite finding. The weight < 1 half also covers the degenerate sole-contributor case, where leverage against an empty set of neighbours is undefined rather than zero. Same shape as present on the site rows, which is the right precedent.

   The legend at :255-264 is no longer dangling: a reader can now find a method's leverage and read the 2×2 against it, and marginalPp is weight × leverage so the "large weight, near-zero marginal contribution" case — the agreeable-not-informative one item 35 calls the specific thing the user asked to see — is now visible in numbers on the same row.

   Advisories

   3 was filed as kanban item 118 rather than fixed, which is the standing rule working as intended, and the item records the property that makes it worth filing at all: the sum check is scale-invariant, so the findings panel structurally cannot catch a unit reinterpretation. 4 is fixed in 73e73b8 — impl-116 and impl-35 have ledger rows and AgRatioReference's docblock no longer claims it is unwired.

   Verdict

   Approve. Nothing from my round-1 review remains open.

39 of 65 files shown — 26 past this page's size limit. Browse or clone the repository to read the whole merge.