mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 20:42:43 +00:00
fix(ops): refuse an account whose rows are not the thousandfold class
The provenance criterion proves where a row came from, not which build imported it. An account re-imported on the fixed build carries the same `EXPORT_XML_SOURCE_MAX` stamp and the same `stats:` external id with the right numbers and no audit row, so it is selected exactly like a broken one — and its rest days, under 200 m because the phone stayed at home, survive the plausibility check after another thousandfold multiply. So one candidate row that would leave the range now refuses the whole account: nothing written, no audit row, the offending rows and a printed reason instead. The idempotency guard only ever held against the script's own prior run; this holds against the case it could not see. The runbook says the same in words — a re-imported account must not be repaired, the re-import already did it — and says what the dry run is for: it is where the decision is made, not a formality in front of one already taken. Integration coverage for the re-imported account: correct rows, an `--apply` run, nothing changes and no audit row is left behind. Review finding H1.
This commit is contained in:
1 parent
65e9ecbde6
commit
361e577864
4 files changed
+189
-18
No files matched your search
@@ -25,6 +25,16 @@ build rewrites every affected day with the right number, needs no flags, and
|
||||
also repairs rows this script deliberately leaves alone (see the criterion
|
||||
below). The script exists for accounts that no longer have the archive.
|
||||
|
||||
**An account that has been re-imported must not then be repaired.** The
|
||||
re-import already fixed it. Its rows keep the same `EXPORT_XML_SOURCE_MAX`
|
||||
stamp and the same `stats:` external id, and they carry no audit row, so the
|
||||
criterion selects them exactly as it selects a broken account — the stamp
|
||||
proves where a row came from, not which build imported it. Multiplying them
|
||||
again turns a 2.484 km day into 2484 km. The script refuses such an account on
|
||||
its own (any day whose value would leave the plausible range after the
|
||||
multiply stops the whole account), but that guard fires on the big days: keep
|
||||
the two apart yourself before you type `--apply`.
|
||||
|
||||
## Which rows it touches
|
||||
|
||||
Only rows carrying the stamp the archive importer itself writes:
|
||||
@@ -51,7 +61,13 @@ Deliberately out of scope:
|
||||
|
||||
## Run a dry-run first
|
||||
|
||||
Always. The default mode reports and never writes:
|
||||
Always, and read it. The dry run is where the decision is made, not a
|
||||
formality in front of a decision already taken: it is the only place the
|
||||
script shows you real numbers from the account before it changes them. If the
|
||||
worked example does not match what that person's Health app shows for that day,
|
||||
the run is wrong and `--apply` will make it permanent.
|
||||
|
||||
The default mode reports and never writes:
|
||||
|
||||
```bash
|
||||
# from a source checkout, with DATABASE_URL pointing at the instance
|
||||
@@ -94,9 +110,12 @@ clients pick the rows up on their next delta sync, and an audit row
|
||||
and the row count. The DAY/WEEK/MONTH/YEAR rollups and the cached status
|
||||
insights for the touched days are recomputed afterwards.
|
||||
|
||||
A row whose repaired value would leave the plausible range (0–200 000 m) is
|
||||
reported and never written — several of those in one account mean the rows are
|
||||
not the 1000x class and the run should be stopped.
|
||||
A row whose repaired value would leave the plausible range (0–200 000 m)
|
||||
refuses the **whole account**: nothing is written, the offending rows and the
|
||||
reason are printed, and the run moves on to the next account. One such row is
|
||||
evidence the account is not the 1000x class — a re-imported account looks
|
||||
exactly like this — and repairing the rest of it would multiply days that were
|
||||
already right.
|
||||
|
||||
## It runs once per account
|
||||
|
||||
|
||||
@@ -24,8 +24,10 @@
|
||||
* - `--apply` writes, one transaction per account, and records an audit row
|
||||
* in that same transaction. An account carrying that row is skipped, so a
|
||||
* second run finds nothing.
|
||||
* - A row whose repaired value would leave the plausibility range is
|
||||
* reported and never written.
|
||||
* - A row whose repaired value would leave the plausibility range refuses
|
||||
* the WHOLE account: one such row is evidence the rows are not the 1000x
|
||||
* class (an account re-imported on the fixed build looks exactly like
|
||||
* this), and its rest days would otherwise be multiplied again.
|
||||
* - `--unit=mi` for an imperial archive (default `km`). The stored row keeps
|
||||
* no record of the archive's unit — that is the defect — so the operator
|
||||
* names it; a mismatched unit is why the flag exists rather than a guess.
|
||||
@@ -76,6 +78,7 @@ async function main(): Promise<void> {
|
||||
let totalRepairable = 0;
|
||||
let totalUpdated = 0;
|
||||
let totalSkipped = 0;
|
||||
let refused = 0;
|
||||
|
||||
for (const plan of plans) {
|
||||
if (plan.alreadyRepairedAt) {
|
||||
@@ -107,7 +110,12 @@ async function main(): Promise<void> {
|
||||
for (const row of plan.outOfRange) {
|
||||
console.log(
|
||||
` ! ${row.id} (${row.measuredAt.toISOString().slice(0, 10)}): ` +
|
||||
`${row.value} m x ${factor} leaves the plausible range — not written`,
|
||||
`${row.value} m x ${factor} leaves the plausible range`,
|
||||
);
|
||||
}
|
||||
if (plan.outOfRange.length > 0) {
|
||||
console.log(
|
||||
" ! this account will be REFUSED — see the reason under --apply",
|
||||
);
|
||||
}
|
||||
|
||||
@@ -117,6 +125,11 @@ async function main(): Promise<void> {
|
||||
});
|
||||
totalUpdated += outcome.updated;
|
||||
totalSkipped += outcome.skipped;
|
||||
if (outcome.refusedReason) {
|
||||
refused += 1;
|
||||
console.log(` ✗ refused: ${outcome.refusedReason}`);
|
||||
continue;
|
||||
}
|
||||
console.log(
|
||||
` ✓ repaired ${outcome.updated} row(s), skipped ${outcome.skipped}`,
|
||||
);
|
||||
@@ -124,9 +137,11 @@ async function main(): Promise<void> {
|
||||
|
||||
console.log(
|
||||
apply
|
||||
? `\nDone. ${totalUpdated} row(s) repaired, ${totalSkipped} skipped.`
|
||||
? `\nDone. ${totalUpdated} row(s) repaired, ${totalSkipped} skipped, ` +
|
||||
`${refused} account(s) refused.`
|
||||
: `\nDry run: ${totalRepairable} row(s) would be repaired. ` +
|
||||
"Re-run with --apply to write.",
|
||||
"Read the worked examples above and decide before re-running with " +
|
||||
"--apply.",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -35,6 +35,16 @@
|
||||
* archive on the fixed build repairs those, and is the authoritative
|
||||
* repair for any account that still has its `export.zip`.
|
||||
*
|
||||
* ── The refusal ────────────────────────────────────────────────────
|
||||
*
|
||||
* The criterion proves an archive ORIGIN. It cannot prove the archive was
|
||||
* imported on the broken build — an account re-imported since carries the
|
||||
* same stamp with the right numbers, and its rest days still pass the
|
||||
* plausibility check after another x1000. So any candidate whose repaired
|
||||
* value would leave the range refuses the whole account: nothing is
|
||||
* written, the rows are reported, and the operator decides. Re-importing
|
||||
* the archive is the authoritative repair and needs no script.
|
||||
*
|
||||
* ── Idempotency ────────────────────────────────────────────────────
|
||||
*
|
||||
* Applying the repair writes one `AuditLog` row per account inside the
|
||||
@@ -81,8 +91,8 @@ export interface AccountRepairPlan {
|
||||
repairable: RepairCandidateRow[];
|
||||
/**
|
||||
* Candidates whose repaired value would leave the plausibility range
|
||||
* (`validateMeasurementRange`) — reported, never written. A row here is
|
||||
* a sign the account's rows are NOT the 1000x class.
|
||||
* (`validateMeasurementRange`). One row here refuses the whole account:
|
||||
* it is evidence the rows are NOT the 1000x class.
|
||||
*/
|
||||
outOfRange: RepairCandidateRow[];
|
||||
/** Non-null when this account already carries the repair's audit row. */
|
||||
@@ -178,6 +188,35 @@ export interface RepairOutcome {
|
||||
userId: string;
|
||||
updated: number;
|
||||
skipped: number;
|
||||
/**
|
||||
* Why the account was left alone, when it was. Null on a run that wrote.
|
||||
* The script prints it verbatim.
|
||||
*/
|
||||
refusedReason: string | null;
|
||||
/** The out-of-range candidates — what a refusal was decided on. */
|
||||
skippedRows: RepairCandidateRow[];
|
||||
}
|
||||
|
||||
/**
|
||||
* The refusal an out-of-range candidate earns.
|
||||
*
|
||||
* The provenance criterion proves an archive origin; it cannot prove the
|
||||
* archive was imported on the BROKEN build. An account that has since been
|
||||
* re-imported on the fixed build carries the same stamp with the right
|
||||
* numbers, and its rest days — under 200 m, phone left at home — pass the
|
||||
* plausibility check after another x1000 and would be written. So a single
|
||||
* row that would leave the range is treated as what it is: evidence the
|
||||
* account is not the 1000x class. The whole account stops.
|
||||
*/
|
||||
function outOfRangeRefusal(plan: AccountRepairPlan, factor: number): string {
|
||||
return (
|
||||
`${plan.outOfRange.length} of ${plan.candidateCount} candidate row(s) ` +
|
||||
`leave the plausible range after x${factor}, so these rows are not the ` +
|
||||
"1000x class — the account was most likely re-imported on the fixed " +
|
||||
"build, or its archive was never in the affected unit. Nothing written. " +
|
||||
"Check the dry run's worked examples against the account's Health app " +
|
||||
"before forcing anything."
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -192,8 +231,24 @@ export async function applyAppleHealthDistanceRepair(
|
||||
): Promise<RepairOutcome> {
|
||||
const factor = repairFactorFor(options.archiveUnit);
|
||||
const skipped = plan.outOfRange.length;
|
||||
if (plan.alreadyRepairedAt || plan.repairable.length === 0) {
|
||||
return { userId: plan.userId, updated: 0, skipped };
|
||||
const base = {
|
||||
userId: plan.userId,
|
||||
updated: 0,
|
||||
skipped,
|
||||
skippedRows: plan.outOfRange,
|
||||
};
|
||||
if (plan.alreadyRepairedAt) {
|
||||
return {
|
||||
...base,
|
||||
refusedReason: `already repaired on ${plan.alreadyRepairedAt.toISOString()}`,
|
||||
};
|
||||
}
|
||||
// Refuse the ACCOUNT, not just the row: see `outOfRangeRefusal`.
|
||||
if (plan.outOfRange.length > 0) {
|
||||
return { ...base, refusedReason: outOfRangeRefusal(plan, factor) };
|
||||
}
|
||||
if (plan.repairable.length === 0) {
|
||||
return { ...base, refusedReason: "no repairable rows" };
|
||||
}
|
||||
|
||||
const ids = plan.repairable.map((row) => row.id);
|
||||
@@ -239,7 +294,13 @@ export async function applyAppleHealthDistanceRepair(
|
||||
"repair-apple-health-distance",
|
||||
);
|
||||
|
||||
return { userId: plan.userId, updated: ids.length, skipped };
|
||||
return {
|
||||
userId: plan.userId,
|
||||
updated: ids.length,
|
||||
skipped,
|
||||
refusedReason: null,
|
||||
skippedRows: plan.outOfRange,
|
||||
};
|
||||
}
|
||||
|
||||
/** Narrowing helper for the script's `--unit` flag. */
|
||||
|
||||
@@ -187,25 +187,101 @@ describe("Apple Health export distance repair — integration", () => {
|
||||
expect((await readRow(prisma, rowId)).syncVersion).toBe(2);
|
||||
});
|
||||
|
||||
it("uses the archive's own unit and never writes an implausible value", async () => {
|
||||
it("uses the archive's own unit", async () => {
|
||||
const prisma = getPrismaClient();
|
||||
const userId = await makeUser(prisma, "mi");
|
||||
const mile = await seedRow(prisma, userId, { value: 1.543 });
|
||||
const absurd = await seedRow(prisma, userId, { value: 250 });
|
||||
|
||||
const plans = await planAppleHealthDistanceRepair(prisma, {
|
||||
archiveUnit: "mi",
|
||||
});
|
||||
expect(plans[0].repairable.map((row) => row.id)).toEqual([mile]);
|
||||
|
||||
const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], {
|
||||
archiveUnit: "mi",
|
||||
});
|
||||
await settleBackgroundTasks();
|
||||
expect(outcome).toMatchObject({ updated: 1, skipped: 0 });
|
||||
expect((await readRow(prisma, mile)).value).toBeCloseTo(2483.217792, 6);
|
||||
});
|
||||
|
||||
it("refuses the whole account when one row would leave the range", async () => {
|
||||
const prisma = getPrismaClient();
|
||||
const userId = await makeUser(prisma, "mixed");
|
||||
const plausible = await seedRow(prisma, userId, { value: 1.543 });
|
||||
const absurd = await seedRow(prisma, userId, { value: 250 });
|
||||
|
||||
const plans = await planAppleHealthDistanceRepair(prisma, {
|
||||
archiveUnit: "mi",
|
||||
});
|
||||
expect(plans[0].repairable.map((row) => row.id)).toEqual([plausible]);
|
||||
expect(plans[0].outOfRange.map((row) => row.id)).toEqual([absurd]);
|
||||
|
||||
const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], {
|
||||
archiveUnit: "mi",
|
||||
});
|
||||
await settleBackgroundTasks();
|
||||
expect(outcome).toMatchObject({ updated: 1, skipped: 1 });
|
||||
expect((await readRow(prisma, mile)).value).toBeCloseTo(2483.217792, 6);
|
||||
expect(outcome.updated).toBe(0);
|
||||
expect(outcome.skipped).toBe(1);
|
||||
expect(outcome.skippedRows.map((row) => row.id)).toEqual([absurd]);
|
||||
expect(outcome.refusedReason).toMatch(/leave the plausible range/);
|
||||
|
||||
// Nothing written at all — not the "repairable" row, not the audit row
|
||||
// that would make a later, correct run a no-op.
|
||||
expect((await readRow(prisma, plausible)).value).toBe(1.543);
|
||||
expect((await readRow(prisma, plausible)).syncVersion).toBe(1);
|
||||
expect((await readRow(prisma, absurd)).value).toBe(250);
|
||||
expect(
|
||||
await prisma.auditLog.count({
|
||||
where: { action: APPLE_HEALTH_DISTANCE_REPAIR_ACTION, userId },
|
||||
}),
|
||||
).toBe(0);
|
||||
});
|
||||
|
||||
it("leaves an account re-imported on the fixed build untouched", async () => {
|
||||
// The scenario the criterion cannot see: the operator re-imported
|
||||
// `export.zip` on the fixed build, so the rows are already right. They
|
||||
// still carry `EXPORT_XML_SOURCE_MAX` + the `stats:` external id and
|
||||
// still have no audit row, so they are selected exactly like a broken
|
||||
// account's rows. The rest day survives another x1000; the ordinary days
|
||||
// do not, and that is what has to stop the account.
|
||||
const prisma = getPrismaClient();
|
||||
const userId = await makeUser(prisma, "reimported");
|
||||
const ordinary = await seedRow(prisma, userId, { value: 2484 });
|
||||
const longWalk = await seedRow(prisma, userId, { value: 7020 });
|
||||
const restDay = await seedRow(prisma, userId, { value: 150 });
|
||||
|
||||
const plans = await planAppleHealthDistanceRepair(prisma, {
|
||||
archiveUnit: "km",
|
||||
});
|
||||
expect(plans).toHaveLength(1);
|
||||
// The rest day would pass the plausibility check on its own.
|
||||
expect(plans[0].repairable.map((row) => row.id)).toEqual([restDay]);
|
||||
expect(plans[0].outOfRange.map((row) => row.id).sort()).toEqual(
|
||||
[ordinary, longWalk].sort(),
|
||||
);
|
||||
|
||||
const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], {
|
||||
archiveUnit: "km",
|
||||
});
|
||||
await settleBackgroundTasks();
|
||||
expect(outcome.updated).toBe(0);
|
||||
expect(outcome.refusedReason).not.toBeNull();
|
||||
|
||||
for (const [id, value] of [
|
||||
[ordinary, 2484],
|
||||
[longWalk, 7020],
|
||||
[restDay, 150],
|
||||
] as const) {
|
||||
const row = await readRow(prisma, id);
|
||||
expect(row.value).toBe(value);
|
||||
expect(row.syncVersion).toBe(1);
|
||||
}
|
||||
expect(
|
||||
await prisma.auditLog.count({
|
||||
where: { action: APPLE_HEALTH_DISTANCE_REPAIR_ACTION, userId },
|
||||
}),
|
||||
).toBe(0);
|
||||
});
|
||||
|
||||
it("plans one entry per account", async () => {
|
||||
|
||||
Reference in new issue
Block a user