v1.37.27 — the checks that were green because they were not checking - #837
Merged
Conversation
DELETE /api/user/thresholds without ?metric= erased the account's whole
override set on an omitted query parameter alone: no confirmation, no dry
run, and no rate limit of its own while the PUT beside it had one.
The wide form now wants { "confirm": "RESET_THRESHOLDS" } in the body — the
same typed-confirmation shape /api/settings/account and /api/settings/data
already use — and refuses 422 without writing anything. The ?metric= form
is untouched: it resets one band the person can retype, and the targets
sheet fires it in a loop. Both forms now run the PUT's 30 / 5 min limit.
The settings card's "reset all" control already opened a confirm dialog;
it now sends that answer to the server too. Its token is compared against
the route's declaration in the tests, so the two ends cannot drift apart
into a permanent 422.
DELETE /api/auth/passkeys/{id} took a plain cookie session or a wildcard
Bearer, while DELETE /api/auth/me/mfa/webauthn/{id} beside it demanded a
fresh factor proof. The softer gate sat on the primary sign-in credential:
the last-method check stops an account locking itself out, but nothing
stopped a hijacked session stripping the credentials the owner signs in
with.
The route now goes through requireMfaManagementAuth, the same gate the
security-key removal uses, in a new "if-enrolled" mode. An account with a
second factor pays the full price on either transport — a freshly verified
cookie session, or a factor-proved X-Step-Up elevation on Bearer, so the
native client keeps the capability rather than being sent to the website.
An account with no second factor keeps the contract it had, because a gate
it can never satisfy is not a safeguard but a locked door in front of its
own credential list.
The elevation is spent immediately before the delete, so a foreign id or a
refused last credential does not burn a proof that then has to be minted
again. The route is added to the elevation allowlist with its reason, and
the either-factor enrolment test that was written out three times in the
API kit is now one function.
A restore's first act is to delete the class of records it is about to rebuild, so a payload whose documents section is simply absent did not "restore everything else". It emptied the vault, reported success, and left the operator to find the hole later. The whole file is refused instead, above the transaction that wipes the account, and the refusal names the section and says nothing was changed — an operator holding the only copy of an account cannot act on "restore failed". The sections ride the error envelope as meta.errorCode backup.section.missing, and the admin console holds them in a panel beside the skip report rather than a toast that outlives nothing. The manifest is the discriminator, and only the manifest. A portable export leaves the mental-health screeners and the consent receipts out on purpose and declares both as omitted; those files still restore. So does a file whose section is present and empty, which is what an account with no documents honestly writes, and so does every backup written before the manifest existed. The integration test proves the distinction with the same builder and the same deleted keys, differing only in what the manifest said. The route is deliberately outside the published contract — requireAdmin is cookie-only — so the OpenAPI document is unchanged.
Four surfaces assembled the correlation-discovery matrix and three carried a different channel set. The Coach `get_correlations` tool had neither the environmental-exposure channels nor the custom-metric ones, so asking it whether air pressure relates to your sleep met a matrix the pair had never been in. Its own file header said the opposite - "the SAME full-matrix scan the route runs" - and had said it since the weather channels arrived in v1.25. The period narrative was missing those two families and the compliance / symptom pair on top. `src/lib/insights/discovery-matrix.ts` is now the one place a matrix is built. v1.30.3 hoisted the fetch discipline into a shared module for exactly this reason and left the assembly duplicated behind; this finishes it. The route, the per-metric card, the Coach tool and the period narrative all call `assembleDiscoveryMatrix` and get the same families. What still differs is declared as an option with its reason at the declaration: the measurement read mode (a read-cost choice, left per surface), the extra metric types the narrative's delta beat needs, and the RATED mood factors. Mood factors stay on the narrative alone. That capture kind is frozen, the two surfaces that persist a discovered pair write its channel key into a plaintext column, and a custom factor's key is an opaque id - so admitting them elsewhere is a decision about what to ask, not a parity fix. Restore the per-metric filter on the assessment card's relations. It was written in place of the pattern-dismissal filter rather than chained with it, so a weight card's prompt had been receiving every surviving pair in the record while the function name, the docstring and the early-return guard all still said one metric. `discovery-matrix-guard.test.ts` freezes who may fetch a channel, who may construct one, and who may scan. Each matcher asserts a non-zero match count, so a matcher that has stopped matching fails rather than agreeing with the allowlist. The statistics are untouched - same Pearson, same exact p-value, same Benjamini-Hochberg, same floors. The Coach's family is larger now, which is the point: it is corrected over the same family the insights page is.
The gate landed with a softer mode for accounts holding no second factor, on the premise that they could not clear it. That premise was wrong. A passkey is a possession factor here and the codebase already says so in three places: a primary passkey login stamps Session.mfaVerifiedAt, passkey sits in FRESH_FACTOR_METHODS so a passkey-proved elevation reaches the fresh-factor routes, and the enrolment policy counts a primary passkey as satisfying it outright. The only thing refusing such an account was the reachability pre-check inside requireFreshMfa, which counts second factors and nothing else. So the mode is gone and the route asks for a full freshFactor: true with proofSource: "any-possession" — a second knob that widens which credentials count as "could ever prove this", and changes nothing about the gate. A passkey-only account now gets a real gate with the re-provable auth.stepup.required rather than no gate at all, and the dead-end auth.stepup.mfa_not_enrolled is reserved for an account holding nothing. hasSecondFactorEnrolled is deliberately NOT widened. It answers a different question, and requireFreshMfaIfEnrolled asks it before deciding whether account deletion, the data reset, the password change, the encrypted export and key rotation gate at all — counting a passkey there would change five destructive actions in passing. The two predicates sit beside each other with docblocks saying why they differ, and collapsing them turns eight existing tests red across three files. Recovery is named rather than assumed. On Bearer: mint an elevation with method "passkey". On the web: sign in again, which is what stamps the session — there is no in-page re-proof for a cookie session because the mint endpoint is Bearer-only by design. That limit is written into the route docblock instead of left to be discovered, and the removal's refusal now renders its own sentence in all six locales rather than the server's English "second-factor" prose, which was wrong for the account this card mostly serves.
…as written in The normative-claim bank held English and German words. Insight recommendations are generated in the reader's locale, so for French, Spanish, Italian and Polish every recommendation scored non-normative: the annotation reported zero normative claims and zero uncited ones, and the admin quality dashboard read that as full coverage rather than as no measurement at all. A check that cannot fail is indistinguishable from a check that passes. The bank now has one arm per shipped locale and the caller passes the generation locale; the reader's arm and the English one both run, because a fallback provider answers a non-English reader in English. Matching folds both sides through the shared label fold and anchors the leading word boundary, so inflections still hit while the everyday Italian "soprattutto" no longer counts as a comparative. These words are the model's prose, not app labels, so they are not in the message bundles and cannot be derived from them. The structural test refuses an empty arm instead: a seventh language fails the suite rather than quietly scoring nothing. An empty arm also used to compile to an alternation that matched every string, which would have flipped the same blindness to the opposite extreme.
The gate is "looks off-topic AND does not look health", and both halves were English/German word lists. Two things followed. An off-topic ask in French, Spanish, Italian or Polish was never recognised. And a genuine health question in those languages that happened to brush an English deny word was refused outright: "Ho una serie di misurazioni strane" trips "serie", and nothing on the allow side knew "misurazioni", so a question about the reader's own data came back as a refusal. The allow side is no longer transcribed. Every metric label key and the health nav labels already name these concepts in all six bundles, and those labels are the nouns people type, so the vocabulary is read from `messages/<locale>.json` and a seventh language is understood the day its bundle lands. Matching is whole-word over a shared fold applied to both sides. A subtractive list keeps label scaffolding out; French "bien-être" alone would have contributed "bien" and "etre" and marked nearly every French sentence as health. The deny side stays hand-written — these are not app labels — and its new arms match the folded message. A pattern anchored with \b cannot match a word starting with "ż", because neither the preceding space nor the letter is an ASCII word character, so there is no boundary between them. The prompt-injection bank is untouched. It reads the person's own message and the foreign-text path is fenced elsewhere; this is defence in depth, not an open door.
The bot's real commands are slash commands, and `telegram.helpBody` documents exactly those in all six bundles, so nothing a user is TOLD to type depended on their language. The undocumented courtesies did: the bare word "help" and "hilfe" opened the help text and five greetings were answered, everything else fell through to the intake keyword, failed it, and returned without sending anything. Someone typing "aide" or "pomoc" got silence, which is the worst possible answer to a request for help. Both lists now cover the six shipped languages and are compared against the folded message, so "cześć" and "czesc" are one entry and a capital cannot miss. The match stays anchored to the whole message: "help me log my weight" is prose the later paths should still see. These are ordinary words rather than app labels, so there is nothing in the bundles to derive them from.
The grid draws its group headers in the reader's language and then searched only the English Lucide names and the English keyword aids. A German reader who typed back the header they had just read — "Gesundheit" — got an empty grid and "no icons found", with nothing to say the query was fine and the language was not. The filter now also matches the group header the picker is currently rendering, folded on both sides so an accent or a capital cannot make someone miss a word they can see. The labels come from the bundle at the call site, so a seventh language needs no edit here. This is a partial close and the test says so. The per-icon aids stay English, so "Herz" and "kawa" still find nothing; translating roughly eighty icons times three aids times six languages is a content decision, not a filter one.
…ten in The grounding tokenizer read English unit words and English or German month names, while the reply is written in the reader's language. Two silent failures came out of that, pointing in opposite directions. A missing unit loosens the guard. "7,4 Stunden", "45 minuti" and "8000 pasos" came back unkinded, and an unkinded token clears the kind gate against every ledger entry, so a fabricated night's sleep could ground itself on a glucose reading that happened to share its magnitude. Kind scoping is the thing that stops a weight grounding a blood-pressure claim; for five languages it was not running. A missing month corrupts correct prose. "Le 21 juillet" left the day untyped, the plain-magnitude pass graded it, nothing matched, and the soft-strip replaced it — the reader was shown "Le […] juillet". Spanish and Italian also put a particle between day and month, so the pass admits one. The unit vocabulary had been written out three times and the copies had already drifted; only one of them knew about "%". One list now feeds the classifier, the range pass and the measurement pass. Polish months are listed in the genitive, which is what a date uses and also the only form a trailing word boundary can match, since that boundary is defined over ASCII and "ń" is not part of it.
The classification map held three values. Apple's set is larger, so a real verdict was being thrown away on English devices: "Heart Rate Over 120" and "Heart Rate Under 50" both became null, and the column cannot tell that apart from a recording that carries no classification at all. The wordings are observed rather than remembered. The CSV writes a display string and it does not match the framework's case names — the case documented as InconclusiveHighHeartRate reaches the file as "Heart Rate Over 120". Across a corpus of 128 real exports the field held five distinct values; three of them mapped and two were dropped. The threshold belongs to the watch generation, so the two heart-rate arms match the number instead of pinning it, and the rest of the map takes its wording from Apple's ECG instructions for use. Classification is now split from the column write so the parser can tell a verdict it has never heard of from one it knows it cannot represent. Unrecognized is the second kind: the device did not fail to classify the waveform, it received a verdict it does not know, so recording it as "could not classify" would assert something Apple never said. Both still store null, because that is the column's whole vocabulary, and no column is added for a distinction nothing reads yet. The old table pinned one row for "Localized or unknown verdict". That assertion is still true, but it fused three different facts — a localised verdict, an unknown one, and the two documented verdicts being silently dropped beside them — and it read as "everything else is unknown" when two of those were verdicts Apple documents. It is split into the four outcomes. The language half stays open and is now named at the mapping. A German export writes the KEY in German too, so a non-English verdict never reaches this function; it fails the parse earlier.
The newer resolver tracks each re-export as its own reference site, so a barrel line that nothing imports is now reported instead of being counted as used whenever the underlying symbol was used anywhere. The gate goes from zero findings to 206 on unchanged code.
The barrel mirrored every export under src/lib/insights/derived. Of the 124 lines, 101 had no importer: consumers and per-metric tests reach for the concrete module (./sleep-score, ./trajectory, …) directly. Keep the cross-cutting surface — registry, dispatcher, baseline loader, and the value shapes outside consumers read — and say in the header that the barrel is not a mirror, so the next addition needs a consumer first. Definitions are untouched; only the re-export lines go.
The barrel mirrored the tool, resource, prompt, write and scope registries; its only consumer is the /mcp transport route, which needs three of them. The registries are imported from their concrete module by ./server and by their own tests, so the other 31 lines had no importer.
Same shape as the derived and MCP barrels: the line in the barrel had no importer because the consumer imports the concrete module. Affected: cycle (15), coach tools (9), record-settings (8), links (4), data-list, workout-detail and analytics/score (1 each). analytics/score also carried CompositeValue on an explicit export line above an export * of the same module, so the name stays exported either way. Definitions are untouched.
Eighteen modules carried a convenience re-export of a symbol defined elsewhere. In each case the callers the comment describes now import the concrete module directly, so the line had no importer left. Where the whole re-export block went, the comment explaining it went with it — doctor-report-pdf-core, trend-card and medication-extract-prompt each claimed to be a single import surface that nothing was using. Definitions are untouched; imports that existed only to feed a removed re-export go too.
SENSITIVE_LEAVES and COACH_SUGGESTED_ACTION_TYPES each aliased a constant defined elsewhere and were never imported. Neither is a re-export the way the barrel lines were — the definition itself is the dead thing, and both doc comments describe a consumer that does not exist: the purity guard reads SENSITIVE_LEAF_IDS from ./catalogue directly, and nothing mirrors the suggested-action allowlist. The imports that fed them go with them.
CadenceKind and WeekdayToken were imported only to be re-exported. The re-export went in the previous commit but the import survived, because the doc comment above it still named both types at the time.
ignoreExportsUsedInFile did not only exempt exports used inside their own module; until 6.28.0 it also marked every re-export as referenced in its own file, so a barrel line nothing imported was never reported. The upgrade took this repo from 0 findings to 206 on unchanged code. Write it down next to the setting, because a green gate on any earlier knip proved nothing about re-exports. Also correct the production-pass file count, which had drifted to 21.
…s with Every real export begins with a bare `Name` when the name field is blank — Apple omits the trailing comma rather than writing an empty value. The parser required a comma on every header row, so it rejected the file on line 1, before it had read anything. A valueless header row is now skipped. It contributes no metadata entry rather than an empty-string one, because `Lead` and `Unit` presence is what switches the parser into the single-column waveform mode and a key with no value must not be able to trip that switch. The relaxation is confined to the header. Inside the single-column waveform a comma-less row is a sample and is read as one; inside the paired waveform every row is lead and voltage, so a missing comma there is still malformed.
A watch set to a comma-decimal region writes its samples as "-180,596". The single-column reader rejected any row containing a comma outright, so every one of those files failed on its first sample row. The two waveform layouts disagree about what a comma means, which is why this cannot be decided per line. The parser now names the section it is in — header, single-column, paired — and both crossings are commented at the point they happen: the paired layout announces itself with a `Lead,Voltage` column header, while the single-column one begins at the first blank line AFTER Lead and Unit have been declared, because real exports also carry blank lines mid-header. Scalars in both waveforms now read either dialect through one helper, so "-180.596" and "-180,596" give the same microvolts and neither is privileged. A number carrying thousands grouping is refused rather than guessed at, since resolving it needs the file's region and the file never states it. The one genuinely ambiguous shape is a separator followed by exactly three digits; it is read as a decimal, because observed waveforms put two- and three-digit fractions in the same file and grouping is always three, and because a machine writing one bare value per row has no reason to group at all.
The rate matcher demanded "Hz". Observed exports spell the unit out and lower-case it — "511,422 hertz" — and the German ones separate it from the number with a no-break space. The fixtures had only ever used "512 Hz", so the mismatch never showed. Rate and heart rate now split into a magnitude and a unit word, with the magnitude read through the same decimal helper the waveform uses. "512 Hz", "512 hertz" and "511,562 Hertz" all resolve; a missing unit, the wrong unit, a grouped number and a rate past the bound are all still refused. The unit word is matched loosely and the magnitude strictly. That is the right way round: the unit is presentation and varies by region, while the number is the measurement.
The CSV is written in the device's language, and the keys are translated along with the values: a German watch writes `Aufzeichnungsdatum` and `Klassifizierung`, not `Recorded Date` and `Classification`. Indexing on the English keys found none of them, so the parser never located the waveform and rejected the recording outright — a lost import, not a missing field. Two alias maps close it, both taken from real exports rather than from a dictionary: nine metadata keys and two verdicts, consistent across every German file observed. Adding a language is a data addition to those maps and nothing else. The verdict map is an indirection onto the English wording rather than a second verdict-to-enum table, so there stays exactly one place that decides what a verdict means. The other four shipped languages are absent on purpose. A wrong key silently mis-files a value and a wrong clinical verdict is worse than none, so an export this parser cannot place is refused — which an operator can see — rather than read into the wrong column, which they cannot. Two gaps are written down at the map: no observed file carries an average-heart-rate row in any language, and the paired layout's column header has only ever been seen in English.
# Conflicts: # src/lib/apple-health/ecg-csv.ts
The two halves of the ECG fix were built apart. One taught the parser Apple's full English verdict set; the other taught it the metadata keys and verdict wording a German watch writes. Each was correct alone, and each left "Sinusrhythmus" unresolved for a different reason. Together they meet: the alias table translates the wording onto Apple's own English, and the classifier decides what it means. One place still owns the meaning, which is what stops a per-language verdict table from drifting the first time Apple adds a case. The test that expected null was pinning the gap between the branches rather than a decision about German. It now expects NOT_DETECTED, and a French verdict takes over the null case, because four of the six languages genuinely have no observed key map yet.
The authenticated shell wraps every route and read one destination predicate off the `@/lib/record-settings` barrel. That barrel also re-exports the managed settings-patch schemas, so Zod, the module registry and three validation modules rode into the eager client bundle of every authenticated page for a function that inspects a URL slug. The shell imports `record-settings/classification` now; the barrel loses the re-export nobody else used. Measured over all 585 client-rendered routes: none grew, 100 fell, by 4.7 KB gz at the median and 8.1 KB at the most. /insights had been sitting on its 445 KB ceiling for three releases and went over it on this branch's CI run; it measures 437 now, /measurements 435, / 450, /insights/mood 450. The whole-build sum moved the other way, 3451 to 3476, because the aggregate counts each emitted chunk once: a graph that leaves the chunk group every route shares gets counted again wherever it lands, here inside the deferred settings chunks that use it. Bytes on disk rose, bytes a reader downloads only fell. The aggregate cap moves to 3500 with the same 24 KB headroom the four earlier re-measurements kept, and the reasoning sits next to the number.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five branches. Five of the fixes have the same shape underneath: a check that was green because it was not checking.
Apple Health ECG import. Run against 128 real Apple Watch exports, none of them parsed. Four causes, only one about language: the file opens with a header row that has no value after it, waveform samples carry a decimal comma, the sample rate reads
511,422 hertz, and a German watch writes the column names in German. The parser had been written against the test fixture and the fixture is US-style throughout. Both dialects are accepted now and pinned side by side, because the corpus is European and no US export was available to check against. Separately,Heart Rate Over 120andHeart Rate Under 50are what the export writes for two of Apple's own English verdicts, matching neither the framework's case names nor Apple's published wording, so both were dropped as unknown.Correlation matrix. Four surfaces assembled it independently and three carried different channel sets, so the Coach could not report a single environmental correlation. Its own file header claimed the opposite. One assembler now; the divergences that were deliberate stay, as options with their reasons written down. Found on the way: a metric card was handed every surviving correlation in the record because the metric filter had been replaced by the dismissal filter rather than chained with it.
Language matchers. Citation coverage read full coverage for four of six languages. The Coach's topic gate refused Italian and let off-topic Italian through. A unit word it could not place carried no kind, and an unkinded value clears the gate against every ledger entry, so a sleep figure could ground itself on a glucose result.
Data envelope. Deleting every threshold override asks first. Removing a passkey needs the same fresh proof that removing a security key has always needed, satisfiable by the passkey itself so nobody is locked out of their own credential list. A portable backup claiming a section it does not carry is refused whole and names the section; a declared omission still restores.
Performance. Measured against a three-year record: 29,565 measurements, 1,095 mood entries, 5,475 intakes, 20,688 rollup rows. Heaviest surface is 103 statements and 77 KB; page load does not depend on record size. Budgets pin query counts, payload sizes and which rollup arm ran, not wall-clock, because counts were identical across runs while wall-clock moved sevenfold on one surface. One N+1 fixed: the source-priority ladder resolved 25 times on the dashboard and 17 on analytics.
Dead-code gate. It had been blind to every re-export since knip 6.16. The tool changed at 6.28, and the old silence was an artefact of one setting rather than a clean tree: with that setting off, old and new agree exactly. Two dead constants and 204 unimported re-exports removed.
Worth knowing before merge
The aggregate bundle budget passes at 3451 of 3452 KB gz, and
/insightsat 444 of 445. One kilobyte. The next change that adds client-side copy will fail this gate, which is the gate working, but it needs a deliberate answer rather than a raised number.Gate: typecheck, lint, format, knip, openapi:check, 21,991 tests, build, bundle-check.