From 1ec76365817b92d953faea66abdb81afed52b178 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 09:24:55 +0800 Subject: [PATCH 01/10] test(qbo): match the token endpoint by host, not by substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeQL alerts (js/incomplete-url-substring-sanitization, both high) on the same shape: `String(u).includes('oauth.platform.intuit.com')`, used once as a fetch-mock router and once as a call finder. Not an exploit — it is a test — but the finding is correct about the code: `https://evil.example/?next=oauth.platform.intuit.com` satisfies the substring check, so as a router it can answer the wrong call and as a finder it can find one. The assertion was weaker than it reads. Both sites now compare `new URL(u).hostname` to the host exactly, and an unparseable input yields '' so it simply does not match. --- tests/unit/qbo/qbo-oauth-callback.spec.ts | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/tests/unit/qbo/qbo-oauth-callback.spec.ts b/tests/unit/qbo/qbo-oauth-callback.spec.ts index 6256bc0eb..5270b3863 100644 --- a/tests/unit/qbo/qbo-oauth-callback.spec.ts +++ b/tests/unit/qbo/qbo-oauth-callback.spec.ts @@ -93,10 +93,29 @@ const ENV = (kv: ReturnType) => ({ const CTX = { waitUntil: vi.fn(), passThroughOnException: vi.fn() } as never; +const INTUIT_TOKEN_HOST = 'oauth.platform.intuit.com'; + +/** + * Match the token endpoint by EXACT host, never by substring. + * + * `String(u).includes('oauth.platform.intuit.com')` also matches + * `https://evil.example/?next=oauth.platform.intuit.com`, so as a router it can + * route the wrong call and as a finder it can find one. CodeQL flags the shape + * (`js/incomplete-url-substring-sanitization`) and it is right to: the test is + * weaker than it reads. Returns '' for an unparseable input so a malformed URL + * simply does not match. + */ +function hostOf(u: unknown): string { + try { + return new URL(String(u)).hostname; + } catch { + return ''; + } +} + function tokenExchangeOk() { return vi.fn(async (input: RequestInfo | URL) => { - const url = String(input); - if (url.includes('oauth.platform.intuit.com')) { + if (hostOf(input) === INTUIT_TOKEN_HOST) { return new Response(JSON.stringify({ access_token: 'at', refresh_token: 'rt', @@ -155,7 +174,7 @@ describe('QBO OAuth callback authorization', () => { ); const call = (globalThis.fetch as unknown as ReturnType).mock.calls - .find(([u]: [unknown]) => String(u).includes('oauth.platform.intuit.com')); + .find(([u]: [unknown]) => hostOf(u) === INTUIT_TOKEN_HOST); const body = new URLSearchParams(String((call![1] as RequestInit).body)); expect(body.get('redirect_uri')).toBe(qboRedirectUri(APP_BASE_URL)); expect(body.get('redirect_uri')).toBe(`${APP_BASE_URL}/api/integrations/qbo/callback`); From 20e0b2a323ed436bfe665320bd0737a3d6fae1e5 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 09:28:49 +0800 Subject: [PATCH 02/10] docs(erasure): point the gate at the document describing its blind spots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the remaining piece of erasure-freetext Task 3 — the plan reduced it to a script header, and the document itself landed in 225d33eb. The pointer is the point. "PII column" in this gate means a column matching PII_HEURISTIC, so every column that pattern was not told about is invisible and a green run reads as coverage. A limits document nobody finds next to the gate is the failure mode that document describes. Also corrects a stale path in the same header: the coverage spec moved to tests/unit/privacy/ and the comment still pointed at tests/unit/. --- scripts/check-erasure-manifest.mjs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/scripts/check-erasure-manifest.mjs b/scripts/check-erasure-manifest.mjs index 484ea1e3c..f07858cc8 100644 --- a/scripts/check-erasure-manifest.mjs +++ b/scripts/check-erasure-manifest.mjs @@ -7,9 +7,19 @@ * column appears anywhere in the Drizzle schema without either a covering * manifest rule or an explicit ERASURE_OUT_OF_SCOPE entry. * + * ⚠️ READ `docs/compliance/erasure-heuristic-limits.md` BEFORE TRUSTING A GREEN + * RUN. "PII column" here means a column matching PII_HEURISTIC below, and that + * pattern is a list of shapes someone thought of — every column it was not told + * about is invisible to this gate and reads as correct. The document names what + * is structurally out of reach (free prose, addresses, anything whose + * sensitivity is contextual rather than lexical) and carries a worked example + * that is currently open. A limits document nobody finds next to the gate is + * exactly the failure mode it describes, which is why this pointer is here. + * * This guard is COMPLEMENTARY to: - * - tests/unit/erasure-manifest-coverage.spec.ts (manifest <-> orchestrator - * binding drift) — that proves every rule is realized by the executor. + * - tests/unit/privacy/erasure-manifest-coverage.spec.ts (manifest <-> + * orchestrator binding drift) — that proves every rule is realized by the + * executor. * - This lint proves every rule is well-formed AND that NO schema table * grows an un-cataloged PII column unnoticed. * From adc37c997dc421b54a05eb6c10c18552f78c5377 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 10:49:50 +0800 Subject: [PATCH 03/10] fix(erasure): catalogue the repair-request columns the gate cannot see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last four columns portal #88 named. They survived every earlier pass because the PII heuristic matches column NAMES, and `created_by_ref` / `custom_intro` / `note` / `comment_snapshot` do not look like PII while being the one surface in the product where the CLIENT, not the tenant, types prose. The gate was green before this commit and is green after it; nothing went red to prompt this. They are not one kind of thing, so they do not get one verb: - `created_by_ref` is NOT NULL and, on the portal-token path, holds the actor's EMAIL. The schema comment called it "recipient id (client token)" and had been wrong for as long as agent-portal sessions have existed; that comment is corrected here, because it is why the column read as an opaque reference. Being both the identifier and the locator, it deletes the ROWS the subject authored: a client's own repair wish-list carries no legal-evidence basis (the `contacts` posture, not the `invoices` one), nothing references it, and the delete revokes a `share_token` that a contractor may still be holding. - `custom_intro` and `note` are cleared in place on lists OTHER people built for the subject's inspections. Those rows are that person's record and survive; an agent's intro names the buyer just as readily as the buyer's own. - `comment_snapshot` is declared out of scope with its four sibling snapshot columns. They are machine-copied off the published report card — defect prose the inspector wrote about the property. The reason says out loud that the report content they copy from carries no rule of its own, so this is not a decision inherited from a ruled source; it is the same call, made here first. The executor is a separate module because the orchestrator was one line under its anti-monolith cap. Extracting the two timestamp/count helpers it shared with the retention sweep paid for the call site and removed a byte-identical duplicate at the same time. The drift guard reads the new module, and a new test asserts the orchestrator still calls it — a delegated step that stopped being invoked would otherwise satisfy the scan while executing nothing. --- server/lib/compliance/db-row-utils.ts | 43 ++++++ .../lib/compliance/erase-repair-requests.ts | 126 ++++++++++++++++++ server/lib/compliance/erasure-manifest.ts | 43 ++++++ server/lib/compliance/erasure-orchestrator.ts | 29 +--- server/lib/compliance/retention-sweep.ts | 23 +--- server/lib/db/schema/repair-request.ts | 8 +- .../privacy/erasure-manifest-coverage.spec.ts | 76 ++++++++++- .../unit/privacy/erasure-orchestrator.spec.ts | 119 +++++++++++++++++ 8 files changed, 419 insertions(+), 48 deletions(-) create mode 100644 server/lib/compliance/db-row-utils.ts create mode 100644 server/lib/compliance/erase-repair-requests.ts diff --git a/server/lib/compliance/db-row-utils.ts b/server/lib/compliance/db-row-utils.ts new file mode 100644 index 000000000..5b510968e --- /dev/null +++ b/server/lib/compliance/db-row-utils.ts @@ -0,0 +1,43 @@ +/** + * Track I-a GDPR — driver/timestamp helpers shared by the compliance executors. + * + * `changeCount` and `toMs` existed as byte-identical private copies in + * `erasure-orchestrator.ts` and `retention-sweep.ts`. They are one definition + * now: the two modules must agree on what "this update changed N rows" and + * "this column is at instant T" mean, because a row erased on a DSAR and later + * swept past its window has to land in the same place either way. + * + * The year arithmetic is deliberately UTC-only. A retention window is a record + * -keeping obligation measured in whole years, not a local-calendar event, and + * `setUTCFullYear` is the one operation that cannot shift by a day when the + * boundary lands on a DST change. + */ + +/** Driver-tolerant row-count extraction (D1: meta.changes; better-sqlite3: changes). */ +export function changeCount(res: unknown): number { + const r = res as { meta?: { changes?: number }; changes?: number } | undefined; + return r?.meta?.changes ?? r?.changes ?? 0; +} + +/** Coerce a timestamp column value (Date | number | null) to Unix-MS or null. */ +export function toMs(v: unknown): number | null { + if (v == null) return null; + if (v instanceof Date) return v.getTime(); + if (typeof v === 'number') return v; + const n = Number(v); + return Number.isFinite(n) ? n : null; +} + +/** Add whole years to a Unix-MS timestamp, returning a Unix-MS integer. */ +export function addYearsMs(ms: number, years: number): number { + const d = new Date(ms); + d.setUTCFullYear(d.getUTCFullYear() + years); + return d.getTime(); +} + +/** Subtract whole years from a Unix-MS timestamp, returning a Unix-MS integer. */ +export function subtractYearsMs(ms: number, years: number): number { + const d = new Date(ms); + d.setUTCFullYear(d.getUTCFullYear() - years); + return d.getTime(); +} diff --git a/server/lib/compliance/erase-repair-requests.ts b/server/lib/compliance/erase-repair-requests.ts new file mode 100644 index 000000000..8e1062547 --- /dev/null +++ b/server/lib/compliance/erase-repair-requests.ts @@ -0,0 +1,126 @@ +/** + * Track I-a GDPR (portal #88) — erasure executor for the repair-request lists. + * + * A repair request is a list a buyer or their agent builds from a published + * report and shares with the seller's side. It is the one surface in the + * product where the CLIENT, not the tenant, types prose — `custom_intro` at the + * top of the document and a `note` per line item. Both routinely name people. + * + * Why this lives outside `erasure-orchestrator.ts`: that file is at its + * anti-monolith line cap, and this is a self-contained two-table step. It is + * registered through the orchestrator's `step()` recorder, so its decisions land + * in the same append-only `erasure_log` row as every other step and a throw here + * flips the run to `partially_completed` like any other. + * + * Two passes, because there are two ways a subject's data reaches this table: + * + * 1. Lists the SUBJECT authored -> the ROWS are deleted, items first. There is + * no legal-evidence basis for a client's own wish-list (the `contacts` + * posture, not the `invoices` one): nothing references a repair request, it + * is not financial, and it is not signed. Deleting it also destroys the + * `share_token`, which is a persistent link a contractor may still hold — + * an erased subject's links must stop working (the same call the + * `inspection_access_tokens` rule makes). + * + * 2. Lists SOMEBODY ELSE built on the subject's inspections -> the rows stay + * (they are that person's record) and only the free text is cleared. An + * agent's intro can name the buyer just as easily as the buyer's own can. + * + * KNOWN REACH LIMIT, stated rather than left to be discovered: pass 1 locates + * lists by `created_by_ref`, which holds the actor's EMAIL only on the portal + * -token path (`repair-access.ts`). An agent who authenticated through an + * agent-portal session is recorded by user id instead, so a list they authored + * is not found by an email lookup. That is the right outcome for a client + * erasure — the agent is not the subject — but it does mean an agent who is + * themselves the subject keeps an authorship reference to their own account id. + * Pass 2 still clears the prose on those rows. + */ +import { and, eq, inArray, isNotNull } from 'drizzle-orm'; +import { repairRequests, repairRequestItems } from '../db/schema'; +import { changeCount } from './db-row-utils'; + +/** + * The orchestrator's fail-closed step recorder. Passed in rather than imported + * so this module cannot write to the decision log behind the orchestrator's + * back, and so the counts it produces are aggregated exactly like the rest. + */ +type StepRecorder = ( + table: string, + action: 'delete' | 'null' | 'anonymize', + extra: { legalBasis?: 'art_17_3_b' | 'art_17_3_e'; retentionExpiry?: number }, + fn: () => Promise, +) => Promise; + +export interface EraseRepairRequestsInput { + tenantId: string; + subjectEmail: string; + /** Inspection ids the subject is a person on (via `inspection_people`). */ + inspectionIds: string[]; + step: StepRecorder; +} + +export async function eraseRepairRequests( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + db: any, + { tenantId, subjectEmail, inspectionIds, step }: EraseRepairRequestsInput, +): Promise { + // ── Pass 1: the subject's own lists, deleted whole ──────────────────────── + const ownRows = await db.select({ id: repairRequests.id }).from(repairRequests) + .where(and( + eq(repairRequests.tenantId, tenantId), + eq(repairRequests.createdByRef, subjectEmail), + )) + .all(); + const ownIds = (ownRows as Array<{ id: string }>).map((r) => r.id); + + if (ownIds.length > 0) { + // Items first: nothing enforces the parent link at the database level, + // so deleting the parent first would strand them silently. + await step('repair_request_items', 'delete', {}, async () => + changeCount(await db.delete(repairRequestItems) + .where(and( + eq(repairRequestItems.tenantId, tenantId), + inArray(repairRequestItems.repairRequestId, ownIds), + )) + .run())); + await step('repair_requests', 'delete', {}, async () => + changeCount(await db.delete(repairRequests) + .where(and( + eq(repairRequests.tenantId, tenantId), + inArray(repairRequests.id, ownIds), + )) + .run())); + } + + // ── Pass 2: other people's lists on the subject's inspections ───────────── + if (inspectionIds.length === 0) return; + const survivingRows = await db.select({ id: repairRequests.id }).from(repairRequests) + .where(and( + eq(repairRequests.tenantId, tenantId), + inArray(repairRequests.inspectionId, inspectionIds), + )) + .all(); + const survivingIds = (survivingRows as Array<{ id: string }>).map((r) => r.id); + if (survivingIds.length === 0) return; + + // `isNotNull` is not an optimisation — it keeps the recorded count truthful. + // Without it every row on the inspection reports as "cleared", and a + // decision log that overstates what it did is the failure this log exists + // to prevent. + await step('repair_requests', 'null', {}, async () => + changeCount(await db.update(repairRequests).set({ customIntro: null }) + .where(and( + eq(repairRequests.tenantId, tenantId), + inArray(repairRequests.id, survivingIds), + isNotNull(repairRequests.customIntro), + )) + .run())); + await step('repair_request_items', 'null', {}, async () => + changeCount(await db.update(repairRequestItems).set({ note: null }) + .where(and( + eq(repairRequestItems.tenantId, tenantId), + inArray(repairRequestItems.repairRequestId, survivingIds), + isNotNull(repairRequestItems.note), + )) + .run())); +} diff --git a/server/lib/compliance/erasure-manifest.ts b/server/lib/compliance/erasure-manifest.ts index 778862f5e..d1c4c4a4e 100644 --- a/server/lib/compliance/erasure-manifest.ts +++ b/server/lib/compliance/erasure-manifest.ts @@ -179,6 +179,24 @@ export const ERASURE_MANIFEST: ErasureRule[] = [ // `ip_address` stays too — staff-action security trail, declared out of // scope below. { table: 'audit_logs', column: 'metadata', category: 'user.freetext', action: 'anonymize', legalBasis: 'art_17_3_b' }, + + // ── repair requests (#88) ───────────────────────────────────────────────── + // The one surface where the CLIENT types prose rather than the tenant. None + // of these column names looks like PII, which is exactly why the gate never + // asked about them; they are ruled on because somebody read the table, not + // because anything went red. + // + // `created_by_ref` is NOT NULL and, on the portal-token path, holds the + // actor's EMAIL — a plain identifier, despite a schema comment that called + // it an id for years. So it is both the subject PII on this table and the + // locator for it: the ROWS the subject authored are deleted (no + // legal-evidence basis for a client's own wish-list, and the delete revokes + // the still-live `share_token`). `custom_intro` / `note` are cleared in + // place on lists OTHER people built for the subject's inspections, which + // survive as that person's record. Executor: `erase-repair-requests.ts`. + { table: 'repair_requests', column: 'created_by_ref', category: 'user.contact.email', action: 'delete' }, + { table: 'repair_requests', column: 'custom_intro', category: 'user.freetext', action: 'null' }, + { table: 'repair_request_items', column: 'note', category: 'user.freetext', action: 'null' }, ]; /** @@ -296,4 +314,29 @@ export const ERASURE_OUT_OF_SCOPE: ErasureOutOfScopeEntry[] = [ reason: 'Fingerprint only (type/dataschema/id/seq/size/digest) — the command payload is never written, so no subject PII reaches this table. It WAS payload-bearing before #276, when a cmd.tenant.update that failed to parse wrote an admin password hash here. Naming that history is deliberate: an out-of-scope entry that only says "no PII" invites restoring raw parking as a debugging convenience.' }, { table: 'parked_cmd_events', column: 'reason', reason: 'Fixed diagnostic enum (parse-failed / unknown-type-or-version), not personal data.' }, + // Repair-request line items (#88) — the report-derived snapshot columns. + // These are machine-copied off the published report card at add time, not + // typed by anyone on this table: defect prose the INSPECTOR wrote about the + // property, frozen so the shared list stays readable after the report + // changes. They are declared as a group because they are one question, and + // declared at all because `comment_snapshot` was on #88's list and the + // honest answer needs saying out loud: the report content these copy from + // carries NO manifest rule of its own, so this is not a decision inherited + // from a ruled source. It is the same call, made here for the first time. + // The subject's OWN lists never reach this reasoning — those rows are + // deleted whole by the `created_by_ref` rule above. + { table: 'repair_request_items', column: 'comment_snapshot', + reason: 'frozen copy of the inspector-authored defect comment on the published report — professional content about the property, not prose about or by the data subject' }, + { table: 'repair_request_items', column: 'defect_title_snapshot', + reason: 'frozen copy of the report defect title — inspector-authored content about the property' }, + { table: 'repair_request_items', column: 'location_snapshot', + reason: 'frozen copy of the defect location WITHIN the property ("primary bathroom"), not a postal address' }, + { table: 'repair_request_items', column: 'category_snapshot', + reason: 'frozen copy of the report defect category — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'trade_snapshot', + reason: 'resolved trade label ("licensed roofer") snapshotted at add time — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'section_title', + reason: 'frozen copy of the report section heading — template structure, not personal data' }, + { table: 'repair_request_items', column: 'item_label', + reason: 'frozen copy of the report item label — template structure, not personal data' }, ]; diff --git a/server/lib/compliance/erasure-orchestrator.ts b/server/lib/compliance/erasure-orchestrator.ts index 52e4b2897..dc801368a 100644 --- a/server/lib/compliance/erasure-orchestrator.ts +++ b/server/lib/compliance/erasure-orchestrator.ts @@ -59,6 +59,8 @@ import { ANONYMIZE_BOOKING_REQUEST_PII, ANONYMIZE_AUDIT_PII, } from './anonymize-pii'; +import { changeCount, toMs, addYearsMs } from './db-row-utils'; +import { eraseRepairRequests } from './erase-repair-requests'; /** * What a report title becomes. Not blank: a reader of the version chain needs @@ -100,28 +102,6 @@ export interface ErasureSummary { // Both expose the same query-builder surface used here. type AnyDb = DrizzleD1Database> | { [k: string]: unknown }; -/** Driver-tolerant row-count extraction (D1: meta.changes; better-sqlite3: changes). */ -function changeCount(res: unknown): number { - const r = res as { meta?: { changes?: number }; changes?: number } | undefined; - return r?.meta?.changes ?? r?.changes ?? 0; -} - -/** Add whole years to a Unix-MS timestamp, returning a Unix-MS integer. */ -function addYearsMs(ms: number, years: number): number { - const d = new Date(ms); - d.setUTCFullYear(d.getUTCFullYear() + years); - return d.getTime(); -} - -/** Coerce a timestamp column value (Date | number | null) to Unix-MS or null. */ -function toMs(v: unknown): number | null { - if (v == null) return null; - if (v instanceof Date) return v.getTime(); - if (typeof v === 'number') return v; - const n = Number(v); - return Number.isFinite(n) ? n : null; -} - /** * Run a data-subject erasure for `subjectEmail` within `tenantId`. The caller * supplies `retentionYears` (read from tenant_configs.agreement_retention_years). @@ -381,6 +361,11 @@ export async function runErasure( return c; }); + // Repair-request lists built from the published report — the one surface + // where the CLIENT types prose. Two passes (own lists deleted, other + // people's prose cleared); see `erase-repair-requests.ts` for why. + await eraseRepairRequests(db, { tenantId, subjectEmail, inspectionIds: await subjectInspectionIds(), step }); + // The payment ledger is append-only and financial — the ROWS stay, retained // under the accounting/tax obligation. `note` is the one column a human // writes free-hand on a row tied to an identified client, so it is the one diff --git a/server/lib/compliance/retention-sweep.ts b/server/lib/compliance/retention-sweep.ts index 21b103ef8..068de015c 100644 --- a/server/lib/compliance/retention-sweep.ts +++ b/server/lib/compliance/retention-sweep.ts @@ -45,6 +45,7 @@ import { ANONYMIZE_SIGNER_PII, ANONYMIZE_REQUEST_PII, } from './anonymize-pii'; +import { changeCount, toMs, subtractYearsMs } from './db-row-utils'; // Accept either the D1 drizzle type (prod) or the better-sqlite3 test db. type AnyDb = DrizzleD1Database> | { [k: string]: unknown }; @@ -58,28 +59,6 @@ export interface RetentionSweepSummary { purgedSigners: number; } -/** Driver-tolerant row-count extraction (D1: meta.changes; better-sqlite3: changes). */ -function changeCount(res: unknown): number { - const r = res as { meta?: { changes?: number }; changes?: number } | undefined; - return r?.meta?.changes ?? r?.changes ?? 0; -} - -/** Subtract whole years from a Unix-MS timestamp, returning a Unix-MS integer. */ -function subtractYearsMs(ms: number, years: number): number { - const d = new Date(ms); - d.setUTCFullYear(d.getUTCFullYear() - years); - return d.getTime(); -} - -/** Coerce a timestamp column value (Date | number | null) to Unix-MS or null. */ -function toMs(v: unknown): number | null { - if (v == null) return null; - if (v instanceof Date) return v.getTime(); - if (typeof v === 'number') return v; - const n = Number(v); - return Number.isFinite(n) ? n : null; -} - /** * Run the retention sweep against `db` at logical time `now` (Unix-MS). * Returns per-run counts. Idempotent: a second run finds the same rows already diff --git a/server/lib/db/schema/repair-request.ts b/server/lib/db/schema/repair-request.ts index 087a98340..60667a174 100644 --- a/server/lib/db/schema/repair-request.ts +++ b/server/lib/db/schema/repair-request.ts @@ -7,7 +7,13 @@ export const repairRequests = sqliteTable('repair_requests', { tenantId: text('tenant_id').notNull(), inspectionId: text('inspection_id').notNull(), createdByKind: text('created_by_kind', { enum: ['client', 'agent', 'inspector'] }).notNull(), - createdByRef: text('created_by_ref').notNull(), // recipient id (client token) / userId (agent,inspector) + // WHO built this list, as resolved by `repair-access.ts`. NOT an opaque id: + // on the portal-token path (how a client always arrives, and most agents) it + // is the recipient's EMAIL ADDRESS. It is a userId only for the owner-preview + // inspector and for an agent on a logged-in agent-portal session, and the raw + // token string for the legacy KV agent link. Personal data in the common + // case, which is why it carries an erasure rule (erasure-manifest.ts). + createdByRef: text('created_by_ref').notNull(), customIntro: text('custom_intro'), shareToken: text('share_token').notNull(), createdAt: integer('created_at', { mode: 'timestamp_ms' }).notNull(), diff --git a/tests/unit/privacy/erasure-manifest-coverage.spec.ts b/tests/unit/privacy/erasure-manifest-coverage.spec.ts index 0ecee2a3d..272b1a0e1 100644 --- a/tests/unit/privacy/erasure-manifest-coverage.spec.ts +++ b/tests/unit/privacy/erasure-manifest-coverage.spec.ts @@ -17,7 +17,10 @@ import { describe, it, expect } from 'vitest'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { ERASURE_MANIFEST } from '../../../server/lib/compliance/erasure-manifest'; +import { + ERASURE_MANIFEST, + ERASURE_OUT_OF_SCOPE, +} from '../../../server/lib/compliance/erasure-manifest'; /** snake_case -> camelCase (single underscore groups; does not handle acronyms). */ function toCamelCase(snake: string): string { @@ -32,11 +35,38 @@ const sharedAnonymizePath = path.resolve( __dirname, '../../../server/lib/compliance/anonymize-pii.ts', ); +// A step the orchestrator delegates rather than inlines (it is at its line +// cap). The orchestrator CALLS it, so the rule is genuinely executed; the +// table/column names it acts on live here, so the drift scan has to read it. +const repairRequestStepPath = path.resolve( + __dirname, + '../../../server/lib/compliance/erase-repair-requests.ts', +); +const retentionSweepPath = path.resolve( + __dirname, + '../../../server/lib/compliance/retention-sweep.ts', +); // Anonymize columns are defined in the shared SET module and consumed by the -// orchestrator; scan both so the binding holds wherever the columns live. +// orchestrator; scan all of them so the binding holds wherever the columns live. const orchestratorSource = fs.readFileSync(orchestratorPath, 'utf8') + - fs.readFileSync(sharedAnonymizePath, 'utf8'); + fs.readFileSync(sharedAnonymizePath, 'utf8') + + fs.readFileSync(repairRequestStepPath, 'utf8'); + +/** + * A delegated step only counts if the orchestrator actually calls it. Reading + * the module into the drift scan above would otherwise let a rule pass while + * its executor sits unreferenced — the "rule that exists but never runs" + * failure, with the drift guard now helping it hide. + */ +const orchestratorCallsRepairRequestStep = fs + .readFileSync(orchestratorPath, 'utf8') + .includes('eraseRepairRequests('); + +/** Source with comments stripped, so prose cannot satisfy or trip a scan. */ +function stripComments(src: string): string { + return src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/.*$/gm, ''); +} describe('erasure-manifest coverage', () => { it('every anonymize rule column (camelCase) appears in the orchestrator source', () => { @@ -72,4 +102,44 @@ describe('erasure-manifest coverage', () => { expect(missing, `Orchestrator missing delete/null tables: ${missing.join(', ')}`).toHaveLength(0); }); + + it('the delegated repair-request step is actually called by the orchestrator', () => { + expect( + orchestratorCallsRepairRequestStep, + 'erase-repair-requests.ts is read into the drift scan above. If the orchestrator ' + + 'stops calling eraseRepairRequests(), those rules would still pass the scan while ' + + 'nothing executed them.', + ).toBe(true); + }); +}); + +/** table.column pairs that carry a rule OR a reasoned exclusion. */ +const DECIDED = new Set([ + ...ERASURE_MANIFEST.map((r) => `${r.table}.${r.column}`), + ...ERASURE_OUT_OF_SCOPE.map((e) => `${e.table}.${e.column}`), +]); + +describe('portal #88 — the repair-request columns', () => { + // Asserts the ABSENCE of a gap, which is the shape that catches this class + // of defect: a table entirely missing from the manifest is invisible to any + // check that starts from the manifest's own entries, and invisible reads + // exactly like correct. + it.each([ + 'repair_requests.created_by_ref', + 'repair_requests.custom_intro', + 'repair_request_items.note', + 'repair_request_items.comment_snapshot', + ])('%s has a rule or a reasoned exclusion', (key) => { + expect(DECIDED.has(key)).toBe(true); + }); + + it('created_by_ref is treated as an identifier, not as an opaque reference', () => { + // It holds the actor's email on the portal-token path. A rule that + // merely retained or excluded it would leave the subject's address + // sitting in a NOT NULL column on a shareable document. + const rule = ERASURE_MANIFEST.find( + (r) => r.table === 'repair_requests' && r.column === 'created_by_ref', + ); + expect(rule?.action).toBe('delete'); + }); }); diff --git a/tests/unit/privacy/erasure-orchestrator.spec.ts b/tests/unit/privacy/erasure-orchestrator.spec.ts index d3a883c8e..8f55fce61 100644 --- a/tests/unit/privacy/erasure-orchestrator.spec.ts +++ b/tests/unit/privacy/erasure-orchestrator.spec.ts @@ -537,3 +537,122 @@ describe('runErasure — the residences the original manifest missed (#88)', () expect(summary.status).toBe('completed'); }); }); + +/** + * Portal #88, the remaining half — the repair-request lists. These four columns + * survived every earlier pass because the gate matches column NAMES, and + * `created_by_ref` / `custom_intro` / `note` / `comment_snapshot` do not look + * like PII while being exactly where a client types someone's name. + */ +describe('runErasure — repair-request lists (#88)', () => { + let db: BetterSQLite3Database; + + const INSP = 'insp-rr'; + + beforeEach(async () => { + const fixture = createTestDb(); + db = fixture.db; + await setupSchema(fixture.sqlite); + await seedTenants(db); + await seedRoleProfiles(db, TENANT_A, new Date(1)); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (mockDrizzle as any).mockReturnValue(db); + await db.insert(schema.inspections).values({ + id: INSP, tenantId: TENANT_A, propertyAddress: '4 Elm St', + date: '2026-06-04', status: 'completed', paymentStatus: 'unpaid', price: 40000, createdAt: new Date(), + }); + await db.insert(schema.contacts).values({ + id: 'contact-rr', tenantId: TENANT_A, type: 'client', + name: 'Jane Subject', email: SUBJECT_EMAIL, phone: '555-1111', createdAt: new Date(), + }); + // The live client link — pass 2 reaches other people's lists THROUGH it. + await db.insert(schema.inspectionPeople).values({ + id: 'ip-rr', tenantId: TENANT_A, inspectionId: INSP, + contactId: 'contact-rr', roleProfileId: `crp_${TENANT_A}_client`, createdAt: new Date(), + }); + }); + + const run = () => runErasure(db, { tenantId: TENANT_A, subjectEmail: SUBJECT_EMAIL, retentionYears: 6 }); + + /** A list, with one item. `createdByRef` is an EMAIL on the portal-token path. */ + async function seedList(id: string, createdByRef: string, kind: 'client' | 'agent' | 'inspector') { + await db.insert(schema.repairRequests).values({ + id, tenantId: TENANT_A, inspectionId: INSP, createdByKind: kind, + createdByRef, customIntro: 'Jane Subject asks for these repairs before closing.', + shareToken: `share-${id}`, createdAt: new Date(), updatedAt: new Date(), + }); + await db.insert(schema.repairRequestItems).values({ + id: `${id}-item`, tenantId: TENANT_A, repairRequestId: id, + findingKey: 'f1', sectionTitle: 'Roof', itemLabel: 'Shingles', + commentSnapshot: 'Shingles are cupping at the south slope.', + note: 'Call Jane on 555-1111 to arrange access.', + createdAt: new Date(), + }); + } + + it("the subject's own list is deleted whole, items included, share token gone", async () => { + await seedList('rr-own', SUBJECT_EMAIL, 'client'); + + const summary = await run(); + + expect(await db.select().from(schema.repairRequests).all()).toHaveLength(0); + expect(await db.select().from(schema.repairRequestItems).all()).toHaveLength(0); + // The share link is a persistent URL a contractor may still hold; the + // row going is what stops it resolving. + const decisions = summary.decisions.filter((d) => d.table.startsWith('repair_request')); + expect(decisions.map((d) => `${d.table}:${d.action}`)).toEqual([ + 'repair_request_items:delete', 'repair_requests:delete', + ]); + }); + + it("another person's list survives, but its prose about the subject does not", async () => { + await seedList('rr-agent', 'agent@other.com', 'agent'); + + await run(); + + const row = await db.select().from(schema.repairRequests) + .where(eq(schema.repairRequests.id, 'rr-agent')).get(); + expect(row, 'the agent\'s own record must survive').toBeTruthy(); + expect(row!.createdByRef).toBe('agent@other.com'); + expect(row!.customIntro).toBeNull(); + + const item = await db.select().from(schema.repairRequestItems) + .where(eq(schema.repairRequestItems.id, 'rr-agent-item')).get(); + expect(item!.note).toBeNull(); + // The inspector's defect prose is professional content about the + // property; it is declared out of scope, not cleared. + expect(item!.commentSnapshot).toBe('Shingles are cupping at the south slope.'); + }); + + it('a list on somebody else\'s inspection is not touched at all', async () => { + await db.insert(schema.inspections).values({ + id: 'insp-other', tenantId: TENANT_A, propertyAddress: '9 Oak St', + date: '2026-06-05', status: 'completed', paymentStatus: 'unpaid', price: 1, createdAt: new Date(), + }); + await db.insert(schema.repairRequests).values({ + id: 'rr-elsewhere', tenantId: TENANT_A, inspectionId: 'insp-other', + createdByKind: 'client', createdByRef: OTHER_EMAIL, customIntro: 'Untouched.', + shareToken: 'share-elsewhere', createdAt: new Date(), updatedAt: new Date(), + }); + + await run(); + + const row = await db.select().from(schema.repairRequests) + .where(eq(schema.repairRequests.id, 'rr-elsewhere')).get(); + expect(row!.customIntro).toBe('Untouched.'); + }); + + it('records nothing when there was nothing to clear', async () => { + // A decision log that reports work it did not do is worse than a quiet + // one: it is the same artefact an auditor reads as proof. + await db.insert(schema.repairRequests).values({ + id: 'rr-blank', tenantId: TENANT_A, inspectionId: INSP, + createdByKind: 'agent', createdByRef: 'agent@other.com', customIntro: null, + shareToken: 'share-blank', createdAt: new Date(), updatedAt: new Date(), + }); + + const summary = await run(); + + expect(summary.decisions.filter((d) => d.table.startsWith('repair_request'))).toHaveLength(0); + }); +}); From d4674b3e36db42d14665bcd7e8291d04cbe87734 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 10:56:40 +0800 Subject: [PATCH 04/10] fix(qbo): refunds reach QuickBooks, keyed on the ledger row that moved the money MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `createCreditMemo` was implemented and had zero callers, so every refund a tenant granted existed only in OI while their books still showed the revenue. Wiring it was not one line, because the hand-off assumed one refund producer and there are three. Where the push lives: at the route, not inside the writers. `server/services/invoice/refund.ts` is a pure DB module with no QBO service, no `env` and no `executionCtx` — the same constraint that kept the payment push out of `payment-ledger.service`. A push inside a writer would also mean awaiting an outbound HTTP call inside the refund's own path, where a QuickBooks outage could fail a refund the tenant already granted. `POST /api/inspections/{id}/cancel` is the single production entry to all three writers via `applyCancellationRefund`, and it is where `waitUntil` exists. The reason is written at the seam and in the refund module's header, so a fourth writer has to be given a seam rather than silently missing one. What posts and what does not: - `refundPartial` — invoice money. Posts a credit memo. - `refundHeldDeposit` — money against an ORDER with no invoice. Does NOT post. QuickBooks was never told about that deposit (no invoice, so no QBO Invoice and no Payment), so a credit memo would credit the customer for revenue QuickBooks never recorded and understate the tenant's income by the refund. The right instrument is a refund receipt against a customer-deposit liability account, which is a choice in the tenant's chart of accounts. The gap is already disclosed as a count in the Books health card. `applyCancellationRefund` now returns the invoice id alongside the row, so the seam is told which pool the money came from rather than re-deriving it. - `markRefunded` — no production caller, so no seam and no push. It now returns the row it appended instead of void, because returning void is exactly what forces the next person to key a memo on the invoice id. Three payload defects fixed while wiring it: - `requestid` was absent. It now carries `refund-${ledgerRowId}` from `qboRefundKey`, next to `qboPaymentKey` and derived the same way: the id of the FACT, never of the attempt. - `TxnDate` was hardcoded to today, so a back-dated refund booked to the wrong accounting period. It derives from the row's `occurred_at` in the tenant's timezone, through the same `txnDateFor` the payment push now shares. - `qbo_entity_map` stored the memo under `oiId: invoiceId`, and the index on (tenant, oi_type, oi_id) is unique — one credit memo per invoice forever. A second refund created the memo in QuickBooks and then threw on the map insert, leaving a live credit nothing recorded. It is stored under the refund row id, with `onConflictDoNothing` so a re-push of one row (which `requestid` already collapsed on Intuit's side) is not filed as a failure. `refundAmount` stays in DOLLARS, like `recordPayment`'s `amountPaid` — it goes straight onto `Line[0].Amount`, and the caller divides. Handing it cents is a 100x error on a customer's books, so it is asserted at the seam. The QuickBooks call cannot fail the refund: the ledger row is committed before the push is scheduled, the push runs in `waitUntil`, and `createCreditMemo` catches and files a sync error keyed on the refund row. Replacing the `waitUntil` with an `await` turns the outage test red with `Error: QBO 503`. `scripts/check-tz-safety.mjs` named QBO TxnDate as a legitimate `.toISOString().slice(0,10)` living outside the calendar surface; the credit memo joins the payment in no longer being one. --- scripts/check-tz-safety.mjs | 12 +- server/api/inspections/cancellation.ts | 28 +- server/lib/qbo-payment-key.ts | 21 + .../inspection/cancellation.service.ts | 39 +- server/services/invoice.service.ts | 9 +- server/services/invoice/refund.ts | 29 +- server/services/qbo/invoice-sync.ts | 99 +++- .../billing/cancellation-held-deposit.spec.ts | 8 +- tests/unit/qbo/refund-push.spec.ts | 480 ++++++++++++++++++ 9 files changed, 686 insertions(+), 39 deletions(-) create mode 100644 tests/unit/qbo/refund-push.spec.ts diff --git a/scripts/check-tz-safety.mjs b/scripts/check-tz-safety.mjs index 28587748b..80ccca1cc 100644 --- a/scripts/check-tz-safety.mjs +++ b/scripts/check-tz-safety.mjs @@ -10,11 +10,13 @@ * * SCOPED to the calendar surface on purpose: every real bug lives here, while * legitimate `.toISOString().slice(0,10)` uses (server UTC-today, report year, - * QBO document-creation dates) live elsewhere. QBO *payment* TxnDate is NOT on - * that list any more: it books an accounting period, so it derives from the - * ledger row's occurred_at in the tenant zone via epochMsToWallClockYmd (see - * recordPayment in server/services/qbo/invoice-sync.ts). A line opts out with a - * trailing — or immediately preceding — `// tz-lint-ok: ` comment. + * QBO document-creation dates) live elsewhere. QBO *money-movement* TxnDate is + * NOT on that list any more — neither the payment nor the credit memo: both + * book an accounting period, so both derive from the ledger row's occurred_at + * in the tenant zone via epochMsToWallClockYmd (see txnDateFor in + * server/services/qbo/invoice-sync.ts, which is the one date path they share). + * A line opts out with a trailing — or immediately preceding — + * `// tz-lint-ok: ` comment. * * Flags: * P1 hardcoded-Z instant composed from a civil date + wall-clock time diff --git a/server/api/inspections/cancellation.ts b/server/api/inspections/cancellation.ts index f2ac61878..48cb63ade 100644 --- a/server/api/inspections/cancellation.ts +++ b/server/api/inspections/cancellation.ts @@ -134,9 +134,35 @@ const cancellationRoutes = createApiRouter() const userId = (c.get('user') as { sub?: string } | undefined)?.sub ?? null; const refund = await applyCancellationRefund(db, tenantId, quote, userId); + // THE SEAM. All three refund writers reach money through + // `applyCancellationRefund`, and this is the only production entry to + // it, so one push here covers every refund that exists — rather than a + // push inside each writer, where `server/services/invoice/refund.ts` has + // no QBO service, no `env` and no `executionCtx`, and where an outbound + // HTTP call would sit inside the refund's own path. + // + // `waitUntil` is what makes the non-negotiable structural: the refund + // row is already committed above, and QuickBooks being down can only + // lose the memo, never the refund. `createCreditMemo` catches and files + // a sync error besides, so the tenant is told rather than the failure + // vanishing. + // + // `invoiceId` null means a held deposit — see AppliedCancellationRefund + // for why that one is not postable, and is not silently postable either. + if (c.env.QBO_CLIENT_ID && refund?.invoiceId) { + c.executionCtx.waitUntil( + c.var.services.qbo.createCreditMemo( + tenantId, refund.invoiceId, + // DOLLARS. The QBO payload puts this straight on Line[0].Amount. + refund.row.amountCents / 100, + refund.row.id, refund.row.occurredAt, + ), + ); + } + return c.json({ success: true as const, - data: { outcome: flatten(quote), refundPaymentId: refund?.id ?? null }, + data: { outcome: flatten(quote), refundPaymentId: refund?.row.id ?? null }, }, 200); }); diff --git a/server/lib/qbo-payment-key.ts b/server/lib/qbo-payment-key.ts index c351fb273..5f62f2169 100644 --- a/server/lib/qbo-payment-key.ts +++ b/server/lib/qbo-payment-key.ts @@ -27,3 +27,24 @@ export function qboPaymentKey(paymentRowId: string): string { return `pay-${paymentRowId}`; } + +/** + * The same rule for money going the other way: a CreditMemo push is keyed on + * the `refund`-kind ledger row, never on the invoice. + * + * This is not a second convention — it is `qboPaymentKey` with a different + * prefix, deliberately in the same file so the two cannot drift. The prefix + * exists because `requestid` is unique per COMPANY and these keys end up side + * by side in one namespace: distinct prefixes make a memo legible as a memo + * when someone is reading Intuit's request log to explain a figure. + * + * Keying on the invoice would be worse here than for a payment. `qbo_entity_map` + * is uniquely indexed on (tenant, oi_type, oi_id), so a memo stored under + * `oiId: invoiceId` allows exactly ONE credit memo per invoice forever: a second + * refund on the same invoice — a cancellation fee refunded, then the retained + * part released — creates the memo in QuickBooks and then throws on the map + * insert, leaving the memo live with nothing recording it. + */ +export function qboRefundKey(refundRowId: string): string { + return `refund-${refundRowId}`; +} diff --git a/server/services/inspection/cancellation.service.ts b/server/services/inspection/cancellation.service.ts index 204339cae..58c458b97 100644 --- a/server/services/inspection/cancellation.service.ts +++ b/server/services/inspection/cancellation.service.ts @@ -162,6 +162,30 @@ export async function quoteCancellation( }; } +/** + * What `applyCancellationRefund` did, in the shape its caller has to push. + * Not exported: the only consumer is the cancel route, which reaches it through + * the function's return type and never needs to name it. + */ +interface AppliedCancellationRefund { + /** The ledger row an external book of record keys its credit memo on. */ + row: AppendedPayment; + /** + * The invoice `row` reverses — or NULL when the money came back off a held + * deposit, which has no invoice. + * + * Null is an instruction, not a missing field: do not post this to + * QuickBooks. An unapplied deposit was never pushed there in the first + * place (no invoice, so no QBO Invoice and no Payment), so a credit memo + * would credit the customer for revenue QuickBooks never recorded and + * understate the tenant's income by the refund. The right instrument is a + * refund receipt against a customer-deposit LIABILITY account, which is a + * choice in the tenant's chart of accounts and not ours to invent. The gap + * is disclosed as a count in the Books health card instead. + */ + invoiceId: string | null; +} + /** * Append the refund a quote calls for. Returns the ledger row so the caller can * hand its id to an external book of record; null when there is nothing to @@ -180,17 +204,18 @@ export async function quoteCancellation( * held pool is the "cash in one place and not the other" failure this task * exists to avoid. * - * Returns the invoice row when both fire. An external book of record keys its - * credit memo on a row id, and a held-deposit refund is one it cannot post - * anyway — an unapplied deposit was never pushed to QuickBooks in the first - * place, which is what the Books health card says out loud. + * Returns the invoice row when both fire, and says which invoice it belongs to + * so the caller does not have to re-derive the split to know whether the row is + * postable. A held-deposit refund comes back with a null `invoiceId` rather + * than being withheld: the cancel response still has to name the row it + * appended, and only the push is off-limits. */ export async function applyCancellationRefund( db: DrizzleD1Database, tenantId: string, quote: CancellationQuote, recordedBy: string | null, -): Promise { +): Promise { const owed = quote.outcome.refundCents; if (owed <= 0) return null; @@ -206,5 +231,7 @@ export async function applyCancellationRefund( ? await refundHeldDeposit(db, tenantId, quote.inspectionId, { amountCents: fromHeld, reason, recordedBy }) : null; - return invoiceRow ?? heldRow; + if (invoiceRow) return { row: invoiceRow, invoiceId: quote.invoiceId }; + if (heldRow) return { row: heldRow, invoiceId: null }; + return null; } diff --git a/server/services/invoice.service.ts b/server/services/invoice.service.ts index 7688d33e9..f1fc73d01 100644 --- a/server/services/invoice.service.ts +++ b/server/services/invoice.service.ts @@ -202,8 +202,13 @@ export class InvoiceService { return ledger.markPartial(this.getDrizzle(), id, tenantId, source, amountPaidCents); } - /** @see refunds.markRefunded — reverses everything received. */ - async markRefunded(id: string, tenantId: string): Promise { + /** + * @see refunds.markRefunded — reverses everything received. Returns the + * appended row (null when there was nothing to reverse) so whoever gives + * this its first production caller can key a QuickBooks credit memo on the + * ROW; it has none today and pushes nothing. + */ + async markRefunded(id: string, tenantId: string): Promise { return refunds.markRefunded(this.getDrizzle(), id, tenantId); } diff --git a/server/services/invoice/refund.ts b/server/services/invoice/refund.ts index 5e9cff148..3ea6ee104 100644 --- a/server/services/invoice/refund.ts +++ b/server/services/invoice/refund.ts @@ -16,13 +16,24 @@ * CACHE of "some unvoided invoice on this inspection is paid", and a refund * can falsify it. Skipping the re-sync leaves a report publicly readable * with no backing payment, and no test that ignores `inspections` notices. - * - **Return the row.** `refundPartial` answers with the appended row rather + * - **Return the row.** All three writers answer with the appended row rather * than void, because an external book of record has to key its credit memo * on the ROW id: `qbo_entity_map` is uniquely indexed on * (tenant, type, oiId) and can hold exactly one credit memo per invoice * forever, so keying on the invoice makes a second refund throw INSIDE the * push — the memo lands in QuickBooks and the map row is lost. Per-row * identity is the only shape that survives a second refund. + * - **Push from the SEAM, never from here.** This module is pure DB: no QBO + * service, no `env`, no `executionCtx` — the same reason the payment push + * does not live in `payment-ledger.service`. Firing a QuickBooks call from a + * writer would mean injecting a callback into every caller AND awaiting an + * outbound HTTP request inside the refund's own path, where a QuickBooks + * outage could fail a refund the tenant already granted. The push belongs + * where `executionCtx.waitUntil` exists, which is the route: today that is + * `POST /api/inspections/{id}/cancel`, the single production entry to all of + * this via `applyCancellationRefund`. A new writer therefore has to be given + * a seam before it can move money, and that is the point — it is a visible + * step, not a silently missing one. */ import { and, eq } from 'drizzle-orm'; import type { DrizzleD1Database } from 'drizzle-orm/d1'; @@ -188,19 +199,30 @@ export async function refundHeldDeposit( * Signature unchanged from when this lived in `invoice-payments.service` * (`(db, id, tenantId)`, tenant SECOND) — both are strings and reordering them * during a move is how the two get swapped in silence. + * + * Returns the appended row, like its two siblings, and null when there was + * nothing to reverse. It returned void until the QuickBooks refund push landed, + * which is when "return the row" stopped being a style rule: NOT returning it + * is what forces the next person to key a credit memo on the invoice id, and + * that is a memo QuickBooks accepts and `qbo_entity_map` then refuses to + * record. This function has no production caller today, so it pushes NOTHING — + * a push wired to no seam is dead code. Whoever gives it one wires the push + * there, exactly as `POST /{id}/cancel` does, and this return value is what + * they key it on. */ export async function markRefunded( db: DrizzleD1Database, id: string, tenantId: string, -): Promise { +): Promise { const existing = await db.select().from(invoices).where(and(eq(invoices.id, id), eq(invoices.tenantId, tenantId))).get(); if (!existing) throw Errors.NotFound('Invoice not found'); await seedLedgerFromInvoiceRecord(db, tenantId, id); const received = await getNetReceivedCents(db, tenantId, id); + let appended: AppendedPayment | null = null; if (received > 0) { - await recordPayment(db, tenantId, { + appended = await recordPayment(db, tenantId, { invoiceId: id, inspectionId: existing.inspectionId, kind: 'refund', @@ -211,4 +233,5 @@ export async function markRefunded( await recomputeInvoicePaymentState(db, tenantId, id); } await syncInspectionPaymentGate(db, tenantId, existing.inspectionId); + return appended; } diff --git a/server/services/qbo/invoice-sync.ts b/server/services/qbo/invoice-sync.ts index dea835678..c9f6e1c69 100644 --- a/server/services/qbo/invoice-sync.ts +++ b/server/services/qbo/invoice-sync.ts @@ -3,6 +3,7 @@ import { qboConnections, qboEntityMap } from '../../lib/db/schema/qbo'; import { invoices } from '../../lib/db/schema/invoice'; import { tenantConfigs } from '../../lib/db/schema/tenant'; import { epochMsToWallClockYmd, resolveTenantTimeZone } from '../../lib/tz'; +import { qboRefundKey } from '../../lib/qbo-payment-key'; import { logger } from '../../lib/logger'; import { getLedgerOpinion } from '../payment-ledger.service'; import type { @@ -19,6 +20,31 @@ export function withInvoiceSync>(Base: return invoiceNumber.slice(0, 21); } + /** + * The accounting date for a transaction, from the instant the money + * moved. + * + * `TxnDate` is a calendar date with no timezone: QuickBooks books it + * into an accounting period as-is. It must be the day the money MOVED + * (the ledger row's `occurred_at` — the ledger separates it from + * `created_at` because Tuesday's cash gets recorded Thursday), in the + * TENANT's zone: money taken at 6pm Pacific is the same civil day + * locally and the next day in UTC, a real one-day period error at month + * end. + * + * Shared by every outbound transaction so a second push site cannot + * quietly grow a fourth date path. `new Date()` is not an acceptable + * fallback here: it is exactly the bug this replaced. + */ + protected async txnDateFor(tenantId: string, occurredAt: Date): Promise { + const db = this.getDrizzle(); + const cfg = await db.select({ defaultTimezone: tenantConfigs.defaultTimezone }) + .from(tenantConfigs).where(eq(tenantConfigs.tenantId, tenantId)).get(); + return epochMsToWallClockYmd( + occurredAt.getTime(), resolveTenantTimeZone(cfg?.defaultTimezone), + ); + } + /** Invoice → contact → mapped QBO Customer id (same join the old raw SQL did). */ protected async getQBOCustomerIdForInvoice(tenantId: string, invoiceId: string): Promise { const db = this.getDrizzle(); @@ -253,18 +279,7 @@ export function withInvoiceSync>(Base: return; } - // TxnDate is a calendar date with no timezone: QuickBooks books it - // into an accounting period as-is. It must be the day the money - // MOVED (the ledger row's occurred_at — the ledger separates it - // from created_at because Tuesday's cash gets recorded Thursday), - // in the TENANT's zone: a payment taken at 6pm Pacific is the same - // civil day locally and the next day in UTC, a real one-day period - // error at month end. - const cfg = await db.select({ defaultTimezone: tenantConfigs.defaultTimezone }) - .from(tenantConfigs).where(eq(tenantConfigs.tenantId, tenantId)).get(); - const txnDate = epochMsToWallClockYmd( - occurredAt.getTime(), resolveTenantTimeZone(cfg?.defaultTimezone), - ); + const txnDate = await this.txnDateFor(tenantId, occurredAt); try { await this.apiCall(tenantId, 'POST', `payment?requestid=${encodeURIComponent(idempotencyKey)}`, { @@ -279,19 +294,50 @@ export function withInvoiceSync>(Base: } } - async createCreditMemo(tenantId: string, invoiceId: string, refundAmount: number): Promise { + /** + * Money going back out, as a QuickBooks CreditMemo. + * + * `refundAmount` is in DOLLARS, like `recordPayment`'s `amountPaid` — + * it goes straight onto `Line[0].Amount`. Callers hold cents and divide. + * Handing this cents would post a hundred times the refund to a + * customer's books. + * + * `refundRowId` is the `refund`-kind payment-ledger row. It is the unit + * of BOTH the idempotency key and the `qbo_entity_map` identity, and it + * is taken rather than a pre-built key precisely so those two cannot + * disagree: the map is uniquely indexed on (tenant, oi_type, oi_id), so + * storing the memo under the INVOICE would allow one credit memo per + * invoice forever and make a second refund throw after the memo already + * existed in QuickBooks. (`recordPayment` takes a ready-made key instead + * because it writes no map row and so has nothing to keep in sync.) + * + * A held deposit is deliberately NOT refundable through here — it has no + * invoice, so it has no QBO Invoice and no Payment either, and a credit + * memo would credit a customer for revenue QuickBooks never recorded. + * That gap is disclosed as a count in the Books health card rather than + * papered over; see `QBOConnectionStatus.heldDepositCount`. + */ + async createCreditMemo( + tenantId: string, invoiceId: string, refundAmount: number, + refundRowId: string, occurredAt: Date, + ): Promise { const db = this.getDrizzle(); const conn = await db.select().from(qboConnections).where(eq(qboConnections.tenantId, tenantId)).get(); if (!conn) return; const qboCustomerId = await this.getQBOCustomerIdForInvoice(tenantId, invoiceId); - if (!qboCustomerId) return; + if (!qboCustomerId) { + logger.info('QBO createCreditMemo: no customer mapping — skipping', { tenantId, invoiceId }); + return; + } + + const txnDate = await this.txnDateFor(tenantId, occurredAt); try { const created = await this.apiCall<{ CreditMemo: { Id: string; SyncToken: string } }>( - tenantId, 'POST', 'creditmemo', { + tenantId, 'POST', `creditmemo?requestid=${encodeURIComponent(qboRefundKey(refundRowId))}`, { CustomerRef: { value: qboCustomerId }, - TxnDate: new Date().toISOString().slice(0, 10), + TxnDate: txnDate, Line: [{ DetailType: 'SalesItemLineDetail', Amount: refundAmount, @@ -304,19 +350,32 @@ export function withInvoiceSync>(Base: }, ); const now = new Date(); + // `onConflictDoNothing` because the only way to arrive here twice + // for one row is a re-push of the same refund, and `requestid` + // means QuickBooks answered that with the ORIGINAL memo rather + // than a second one. Recording the same fact again is not an + // error, and raising one would put a false failure in front of a + // tenant whose books are correct. await db.insert(qboEntityMap).values({ id: crypto.randomUUID(), tenantId, oiType: 'refund', - oiId: invoiceId, + oiId: refundRowId, qboType: 'CreditMemo', qboId: created.CreditMemo.Id, qboSyncToken: created.CreditMemo.SyncToken, syncedAt: now, - }); + }).onConflictDoNothing(); } catch (e) { - logger.error('QBO createCreditMemo failed', { tenantId, invoiceId }, e instanceof Error ? e : undefined); - await this.logSyncError(tenantId, 'refund', invoiceId, e); + logger.error( + 'QBO createCreditMemo failed', + { tenantId, invoiceId, refundRowId }, + e instanceof Error ? e : undefined, + ); + // Scoped to the ROW, not the invoice, for the same reason the + // map row is: two refunds on one invoice that both fail are two + // things to fix, and an invoice-keyed flag would show one. + await this.logSyncError(tenantId, 'refund', refundRowId, e); } } }; diff --git a/tests/unit/billing/cancellation-held-deposit.spec.ts b/tests/unit/billing/cancellation-held-deposit.spec.ts index 0025c342d..1e75e29f7 100644 --- a/tests/unit/billing/cancellation-held-deposit.spec.ts +++ b/tests/unit/billing/cancellation-held-deposit.spec.ts @@ -145,8 +145,12 @@ describe('the refund is actually written, with no invoice to write it against', const row = await applyCancellationRefund(db, TENANT, quote, 'user-1'); expect(row).not.toBeNull(); - expect(row!.kind).toBe('refund'); - expect(row!.amountCents).toBe(9000); + expect(row!.row.kind).toBe('refund'); + expect(row!.row.amountCents).toBe(9000); + // NULL, and the cancel route reads it as "do not post this to + // QuickBooks": the deposit was never pushed there, so a credit memo + // would credit the customer for revenue QuickBooks never recorded. + expect(row!.invoiceId).toBeNull(); const refunds = await heldRefundRows(); expect(refunds).toHaveLength(1); diff --git a/tests/unit/qbo/refund-push.spec.ts b/tests/unit/qbo/refund-push.spec.ts new file mode 100644 index 000000000..d84a8a77c --- /dev/null +++ b/tests/unit/qbo/refund-push.spec.ts @@ -0,0 +1,480 @@ +/** + * Refunds must reach QuickBooks. + * + * `createCreditMemo` was written, and no line ever called it — the same shape as + * the two payment-push defects, one file over. Every refund a tenant granted + * existed only in OI, so their books showed revenue that had been sent back. + * + * Wiring it is not one call, because there are THREE refund writers and they do + * not all mean the same thing to a book of record: + * + * - `refundPartial` — money off an invoice. This is the one that posts. + * - `refundHeldDeposit` — money off an ORDER with no invoice. QuickBooks was + * never told about that deposit (no invoice, so no QBO Invoice and no + * Payment), and crediting a customer for revenue QuickBooks never recorded + * would understate the tenant's income by the refund. It does NOT post. + * - `markRefunded` — has no production caller, so there is no seam to push + * from. It now returns its row so whoever gives it one can key correctly. + * + * Three things these specs are really guarding, all of them expensive: + * + * 1. The amount is DOLLARS on the wire and cents in the ledger. `Line[0].Amount` + * takes what it is handed, so a missing `/ 100` is a hundred times the + * refund on a customer's books. + * 2. The memo is keyed and MAPPED on the refund ROW. `qbo_entity_map` is + * uniquely indexed on (tenant, oi_type, oi_id): under the invoice id it + * holds exactly one credit memo per invoice forever, and a second refund + * creates the memo in QuickBooks and then throws on the map insert. + * 3. QuickBooks cannot fail the refund. The money moved in OI; an outage may + * lose the memo and never the refund. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { and, eq } from 'drizzle-orm'; +import type { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; + +import * as schema from '../../../server/lib/db/schema'; +import { createTestDb, setupSchema } from '../db'; +import { QBOServiceBase } from '../../../server/services/qbo/api-base'; +import { withInvoiceSync } from '../../../server/services/qbo/invoice-sync'; + +vi.mock('drizzle-orm/d1', () => ({ drizzle: vi.fn() })); +import { drizzle as mockDrizzle } from 'drizzle-orm/d1'; + +import { OpenAPIHono } from '@hono/zod-openapi'; +import cancellationRoutes from '../../../server/api/inspections/cancellation'; +import { InvoiceService } from '../../../server/services/invoice.service'; +import { AppError } from '../../../server/lib/errors'; +import type { HonoConfig } from '../../../server/types/hono'; +import type { CancellationPolicy } from '../../../server/lib/billing/cancellation-policy'; +import { recordPayment } from '../../../server/services/payment-ledger.service'; +import { markRefunded } from '../../../server/services/invoice/refund'; + +const TENANT = '00000000-0000-0000-0000-000000000001'; +const INSP = 'insp-aaaaaaaa-0000-0000-0000-000000000001'; +const INV = 'inv-aaaaaaaa-0000-0000-0000-000000000001'; +const CONTACT = 'contact-aaaa-0000-0000-000000000001'; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type AnyDb = any; + +interface Call { method: string; path: string; body: unknown } + +const requestIdOf = (path: string) => + new URLSearchParams(path.slice(path.indexOf('?') + 1)).get('requestid'); + +// -------------------------------------------------------------------------- +// Part 1 — what goes on the wire. +// +// The db reads are one `select().from().where().get()` each (the connection and +// the tenant's timezone), so they are stubbed: this half is about the request we +// build, not the rows that feed it. Part 2 uses the real database. +// -------------------------------------------------------------------------- + +function stubDb(defaultTimezone?: string) { + const chain = { + select: () => chain, + from: () => chain, + where: () => chain, + get: async () => ({ defaultItemId: 'ITEM-7', defaultTimezone }), + insert: () => ({ values: () => ({ onConflictDoNothing: async () => undefined }) }), + }; + return chain; +} + +class ProbeQbo extends withInvoiceSync(QBOServiceBase) { + calls: Call[] = []; + constructor(private readonly tenantTz?: string) { + super({} as never, 'cid', 'secret', 'whsec', 'jwt'); + } + protected override getDrizzle() { return stubDb(this.tenantTz) as never; } + protected override async getQBOCustomerIdForInvoice(): Promise { return 'QBO-CUST-9'; } + protected override async apiCall( + _tenantId: string, method: 'GET' | 'POST' | 'PUT', path: string, body?: unknown, + ): Promise { + this.calls.push({ method, path, body }); + return { CreditMemo: { Id: 'CM-1', SyncToken: '0' } } as T; + } +} + +/** Any fixed instant; the point is that it is not today. */ +const MOVED = new Date('2026-09-08T00:00:00Z'); +const ROW = 'row-11111111-2222-3333-4444-555555555555'; + +describe('createCreditMemo → QuickBooks', () => { + it('posts the partial amount refunded, not the invoice total', async () => { + // The cancellation ladder keeps a fee and returns the rest, so a partial + // refund is the normal case. 22500 cents of a 45000 invoice is $225. + const qbo = new ProbeQbo(); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, MOVED); + + const body = qbo.calls[0].body as { + Line: Array<{ Amount: number; SalesItemLineDetail: { UnitPrice: number; Qty: number; ItemRef: { value: string } } }>; + CustomerRef: { value: string }; + }; + expect(qbo.calls).toHaveLength(1); + expect(body.Line[0].Amount).toBe(225); + expect(body.Line[0].SalesItemLineDetail).toMatchObject({ UnitPrice: 225, Qty: 1, ItemRef: { value: 'ITEM-7' } }); + expect(body.CustomerRef.value).toBe('QBO-CUST-9'); + }); + + it('carries a requestid derived from the refund ROW, not the invoice', async () => { + const qbo = new ProbeQbo(); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, MOVED); + + expect(qbo.calls[0].path.startsWith('creditmemo')).toBe(true); + expect(requestIdOf(qbo.calls[0].path)).toBe(`refund-${ROW}`); + // Keyed on the invoice, two refunds are one fact to QuickBooks and the + // second silently returns the first one's memo. + expect(requestIdOf(qbo.calls[0].path)).not.toContain(INV); + }); + + it('sends the same key twice for the same row — QBO collapses the second', async () => { + const qbo = new ProbeQbo(); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, MOVED); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, MOVED); + + expect(qbo.calls).toHaveLength(2); // both attempted + expect(new Set(qbo.calls.map((c) => requestIdOf(c.path))).size).toBe(1); // one key + }); + + it('books the memo on the date the money moved, not the push date', async () => { + const qbo = new ProbeQbo(); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, MOVED); + expect((qbo.calls[0].body as { TxnDate: string }).TxnDate).toBe('2026-09-08'); + }); + + it("derives the memo's calendar date in the tenant's timezone, not UTC", async () => { + // 01:00 UTC on the 8th is still the evening of the 7th in Los Angeles. + // At a month end that one day is the wrong accounting period. + const qbo = new ProbeQbo('America/Los_Angeles'); + await qbo.createCreditMemo(TENANT, INV, 225, ROW, new Date('2026-09-08T01:00:00Z')); + expect((qbo.calls[0].body as { TxnDate: string }).TxnDate).toBe('2026-09-07'); + }); +}); + +// -------------------------------------------------------------------------- +// Part 2 — the map row, against the real database and the real customer join. +// -------------------------------------------------------------------------- + +class DbQbo extends withInvoiceSync(QBOServiceBase) { + calls: Call[] = []; + private n = 0; + constructor(private readonly realDb: unknown) { + super({} as never, 'cid', 'secret', 'whsec', 'jwt'); + } + protected override getDrizzle() { return this.realDb as never; } + protected override async apiCall( + _tenantId: string, method: 'GET' | 'POST' | 'PUT', path: string, body?: unknown, + ): Promise { + this.calls.push({ method, path, body }); + return { CreditMemo: { Id: `CM-${++this.n}`, SyncToken: '0' } } as T; + } +} + +describe('the credit memo is recorded against the refund row', () => { + let db: BetterSQLite3Database; + let qbo: DbQbo; + + beforeEach(async () => { + const fix = createTestDb(); + db = fix.db; + await setupSchema(fix.sqlite); + qbo = new DbQbo(db); + + await db.insert(schema.tenants).values({ + id: TENANT, name: 'Acme', slug: 'acme', status: 'active', + deploymentMode: 'shared', tier: 'free', createdAt: MOVED, + }); + await db.insert(schema.inspections).values({ + id: INSP, tenantId: TENANT, propertyAddress: '1 Oak St', date: '2026-09-08', createdAt: MOVED, + } as never); + await db.insert(schema.contacts).values({ + id: CONTACT, tenantId: TENANT, type: 'client', name: 'Pat Client', createdAt: MOVED, + } as never); + await db.insert(schema.qboConnections).values({ + tenantId: TENANT, realmId: 'r1', accessToken: 'a', refreshToken: 'r', + tokenExpiresAt: MOVED, refreshTokenExpiresAt: MOVED, + defaultItemId: 'ITEM-7', createdAt: MOVED, + }); + await db.insert(schema.invoices).values({ + id: INV, tenantId: TENANT, inspectionId: INSP, contactId: CONTACT, + amountCents: 45000, lineItems: [{ description: 'Inspection', amountCents: 45000 }], + createdAt: MOVED, currency: 'USD', + } as never); + await db.insert(schema.qboEntityMap).values({ + id: 'map-contact', tenantId: TENANT, oiType: 'contact', oiId: CONTACT, + qboType: 'Customer', qboId: 'QBO-CUST-9', qboSyncToken: '0', syncedAt: MOVED, + }); + }); + + const memoRows = () => db.select().from(schema.qboEntityMap) + .where(and(eq(schema.qboEntityMap.tenantId, TENANT), eq(schema.qboEntityMap.oiType, 'refund'))).all(); + + it('survives a SECOND refund on the same invoice', async () => { + // The one the unique index on (tenant, oi_type, oi_id) makes impossible + // under an invoice key: the second memo lands in QuickBooks and then the + // map insert throws, so the tenant has a live credit nothing records. + await qbo.createCreditMemo(TENANT, INV, 225, 'refund-row-A', MOVED); + await qbo.createCreditMemo(TENANT, INV, 100, 'refund-row-B', MOVED); + + const rows = await memoRows(); + expect(rows.map((r) => r.oiId).sort()).toEqual(['refund-row-A', 'refund-row-B']); + expect(rows.map((r) => r.qboId).sort()).toEqual(['CM-1', 'CM-2']); + // No sync error was filed: both pushes really succeeded. + expect(await db.select().from(schema.qboSyncErrors).all()).toHaveLength(0); + }); + + it('records no second map row, and no error, when one row is re-pushed', async () => { + // requestid means QuickBooks answered the retry with the ORIGINAL memo. + // Filing a failure here would put a false alarm in front of a tenant + // whose books are correct. + await qbo.createCreditMemo(TENANT, INV, 225, 'refund-row-A', MOVED); + await qbo.createCreditMemo(TENANT, INV, 225, 'refund-row-A', MOVED); + + expect(await memoRows()).toHaveLength(1); + expect(await db.select().from(schema.qboSyncErrors).all()).toHaveLength(0); + }); +}); + +// -------------------------------------------------------------------------- +// Part 3 — the seam. The real cancel route, the real refund writers, the real +// database; `services.qbo` is the spy. +// -------------------------------------------------------------------------- + +/** + * The route quotes against the real clock (`quoteCancellation`'s `now` + * defaults), so the schedule is anchored to it rather than to a literal. + */ +const NOW = new Date(); +const IN_12H = new Date(NOW.getTime() + 12 * 3_600_000); +const HOURS_AGO_24 = new Date(NOW.getTime() - 24 * 3_600_000); + +/** 24h notice, 50% late fee — a late cancel keeps half and refunds half. */ +const POLICY: CancellationPolicy = { + noticeHours: 24, + lateFee: { type: 'percent', percent: 50 }, + noShowFee: { type: 'percent', percent: 100 }, + remedy: 'refund', +}; + +describe('a cancellation refund reaches QuickBooks', () => { + let db: BetterSQLite3Database; + let createCreditMemo: ReturnType; + + beforeEach(async () => { + const fix = createTestDb(); + db = fix.db; + await setupSchema(fix.sqlite); + (mockDrizzle as unknown as ReturnType).mockReturnValue(db); + createCreditMemo = vi.fn().mockResolvedValue(undefined); + + await db.insert(schema.tenants).values({ + id: TENANT, name: 'Acme', slug: 'acme', status: 'active', + deploymentMode: 'shared', tier: 'free', createdAt: NOW, + }); + await db.insert(schema.tenantConfigs).values({ + tenantId: TENANT, updatedAt: NOW, cancellationPolicy: POLICY, + } as never); + await db.insert(schema.inspections).values({ + id: INSP, tenantId: TENANT, propertyAddress: '1 Oak St', date: '2026-08-07', + status: 'confirmed', paymentStatus: 'paid', price: 45000, scheduledStartMs: IN_12H, + agreementRequired: false, paymentRequired: true, createdAt: NOW, + } as never); + }); + + /** An invoice with money received against it. */ + async function seedPaidInvoice(cents = 45000) { + await db.insert(schema.contacts).values({ + id: CONTACT, tenantId: TENANT, type: 'client', name: 'Pat Client', createdAt: NOW, + } as never); + await db.insert(schema.invoices).values({ + id: INV, tenantId: TENANT, inspectionId: INSP, contactId: CONTACT, + amountCents: 45000, lineItems: [{ description: 'Inspection', amountCents: 45000 }], + createdAt: NOW, currency: 'USD', + } as never); + await recordPayment(db as AnyDb, TENANT, { + invoiceId: INV, inspectionId: INSP, kind: 'balance', + amountCents: cents, method: 'card', provider: 'stripe', providerRef: 'pi_1', + }); + } + + /** Money against the ORDER, with no invoice raised — a booking deposit. */ + async function seedHeldDeposit(cents = 45000) { + await recordPayment(db as AnyDb, TENANT, { + invoiceId: null, inspectionId: INSP, kind: 'deposit', + amountCents: cents, method: 'card', provider: 'stripe', providerRef: 'pi_2', + }); + } + + function cancel(opts: { + env?: Record; + reason?: string; + acknowledgedFeeCents?: number; + } = {}) { + const settled: Promise[] = []; + const app = new OpenAPIHono(); + app.use('*', async (c, next) => { + c.set('userRole', 'manager' as never); + c.set('tenantId', TENANT); + c.set('user', { sub: 'user-1' } as never); + c.set('services', { + invoice: new InvoiceService({} as D1Database), + inspection: { cancelInspection: vi.fn().mockResolvedValue(undefined) }, + qbo: { createCreditMemo }, + } as never); + await next(); + }); + app.route('/api/inspections', cancellationRoutes); + app.onError((err, c) => { + if (err instanceof AppError) { + return c.json({ success: false, error: { code: err.code, message: err.message } }, err.status as never); + } + throw err; + }); + const req = new Request(`https://acme.example.com/api/inspections/${INSP}/cancel`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + reason: opts.reason ?? 'client_cancelled', + acknowledgedFeeCents: opts.acknowledgedFeeCents ?? 22500, + }), + }); + const env = { DB: {}, JWT_SECRET: 'test-jwt-secret', QBO_CLIENT_ID: 'qbo-client', ...(opts.env ?? {}) }; + return app.fetch(req, env as never, { + waitUntil: (p: Promise) => { settled.push(p); }, passThroughOnException: () => {}, + } as never).then(async (res) => { await Promise.allSettled(settled); return res; }); + } + + const refundRow = () => db.select().from(schema.orderPayments) + .where(and(eq(schema.orderPayments.tenantId, TENANT), eq(schema.orderPayments.kind, 'refund'))).get(); + + it('pushes a credit memo keyed on the refund row, in dollars', async () => { + await seedPaidInvoice(); + + const res = await cancel(); + expect(res.status).toBe(200); + + const row = await refundRow(); + expect(row?.amountCents).toBe(22500); + // $225, the half returned — not $450, and not 22500. + expect(createCreditMemo).toHaveBeenCalledWith(TENANT, INV, 225, row?.id, row?.occurredAt); + }); + + it('names the pushed row in the response, so the two cannot disagree', async () => { + await seedPaidInvoice(); + const res = await cancel(); + const body = await res.json() as { data: { refundPaymentId: string } }; + const row = await refundRow(); + + expect(body.data.refundPaymentId).toBe(row?.id); + expect(createCreditMemo.mock.calls[0][3]).toBe(body.data.refundPaymentId); + }); + + it('pushes NOTHING for a held deposit, which QuickBooks was never told about', async () => { + // No invoice was ever raised, so there is no QBO Invoice and no Payment. + // A credit memo here credits the customer for revenue QuickBooks never + // recorded and understates the tenant's income by the refund amount. + await seedHeldDeposit(); + + const res = await cancel(); + expect(res.status).toBe(200); + + // The refund itself absolutely happened — only the push is withheld. + const row = await refundRow(); + expect(row?.amountCents).toBe(22500); + expect(row?.invoiceId).toBeNull(); + expect(createCreditMemo).not.toHaveBeenCalled(); + }); + + it('stands the refund up even when QuickBooks is down', async () => { + // The money movement in OI is the source of truth. A QuickBooks outage + // must not roll back or block a refund the tenant already granted. + createCreditMemo.mockRejectedValue(new Error('QBO 503')); + await seedPaidInvoice(); + + const res = await cancel(); + + expect(res.status).toBe(200); + expect(createCreditMemo).toHaveBeenCalled(); + const row = await refundRow(); + expect(row?.amountCents).toBe(22500); + // And the invoice's cached figure came down with it. + const inv = await db.select().from(schema.invoices).where(eq(schema.invoices.id, INV)).get(); + expect(inv?.amountPaidCents).toBe(22500); + }); + + it('does not push when QuickBooks is not connected', async () => { + await seedPaidInvoice(); + const res = await cancel({ env: { QBO_CLIENT_ID: undefined } }); + + expect(res.status).toBe(200); + expect(await refundRow()).toBeTruthy(); + expect(createCreditMemo).not.toHaveBeenCalled(); + }); + + it('pushes nothing when the policy refunds nothing', async () => { + // A no-show keeps 100%, so no money goes back and there is nothing to + // credit. This is the non-discriminating control: it stays green under + // the pre-fix code too, so a wholesale break in the push is + // distinguishable from the bug being fixed. + await seedPaidInvoice(); + await db.update(schema.inspections).set({ scheduledStartMs: HOURS_AGO_24 }) + .where(eq(schema.inspections.id, INSP)); + + const res = await cancel({ reason: 'no_show', acknowledgedFeeCents: 45000 }); + expect(res.status).toBe(200); + + const body = await res.json() as { data: { refundPaymentId: string | null } }; + expect(body.data.refundPaymentId).toBeNull(); + expect(await refundRow()).toBeUndefined(); + expect(createCreditMemo).not.toHaveBeenCalled(); + }); +}); + +// -------------------------------------------------------------------------- +// Part 4 — markRefunded, the writer with no seam. +// -------------------------------------------------------------------------- + +describe('markRefunded can be keyed on later', () => { + let db: BetterSQLite3Database; + + beforeEach(async () => { + const fix = createTestDb(); + db = fix.db; + await setupSchema(fix.sqlite); + await db.insert(schema.tenants).values({ + id: TENANT, name: 'Acme', slug: 'acme', status: 'active', + deploymentMode: 'shared', tier: 'free', createdAt: NOW, + }); + await db.insert(schema.inspections).values({ + id: INSP, tenantId: TENANT, propertyAddress: '1 Oak St', date: '2026-08-07', createdAt: NOW, + } as never); + await db.insert(schema.invoices).values({ + id: INV, tenantId: TENANT, inspectionId: INSP, amountCents: 45000, + lineItems: [{ description: 'Inspection', amountCents: 45000 }], createdAt: NOW, currency: 'USD', + } as never); + }); + + it('hands back the row it appended, so a future caller keys on the ROW', async () => { + // It has no production caller and therefore no push. What it must not do + // is return void: that is what forces the next person to key a credit + // memo on the invoice id, which QuickBooks accepts and qbo_entity_map + // then refuses to record. + await recordPayment(db as AnyDb, TENANT, { + invoiceId: INV, inspectionId: INSP, kind: 'balance', + amountCents: 45000, method: 'card', + }); + + const appended = await markRefunded(db as AnyDb, INV, TENANT); + + const row = await db.select().from(schema.orderPayments) + .where(and(eq(schema.orderPayments.invoiceId, INV), eq(schema.orderPayments.kind, 'refund'))).get(); + expect(appended?.id).toBe(row?.id); + expect(appended?.amountCents).toBe(45000); + expect(appended?.id).not.toBe(INV); + }); + + it('answers null when there was nothing to reverse', async () => { + expect(await markRefunded(db as AnyDb, INV, TENANT)).toBeNull(); + }); +}); From 84d710ebaa6db495d506832dfbc725994b7906e1 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 11:20:27 +0800 Subject: [PATCH 05/10] feat(erasure): retain the property address family, then widen the heuristic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A property address cannot automatically be classified as non-personal data. On a residential inspection ordered by the buyer or the homeowner it is where a person lives, held against a named client through `inspection_people`. Declaring the family out of scope as "property data" was the other option and is rejected: it was the cheapest way back to green, and a red gate would have pushed a hurried reader straight at it. So the nine `inspections` address columns and `inspection_requests. property_address` carry `retain` rules with a stated basis (Art. 17(3)(e) — the address identifies which property a report describes, and the report is the inspector's defence against a negligence claim) and a bounded period. One entry per column, no wildcard: an auditor reads this file, and a wildcard hides what was actually considered. The bound is the tenant's EXISTING `agreement_retention_years`, not a new column. Both windows answer the same question for the same tenant under the same state rules and the same E&O cover; two clocks that start equal drift. The two columns that are a different question get the other answer. `tenant_configs.company_address` is a business's own published location — the controller's identity — so it follows its `company_lat`/`company_lng` siblings. `inspections.address_geocoded_at` records when the geocode ran, not where the property is. NOTHING ENFORCES THE WINDOW YET, and that is stated at the rules rather than left to be discovered. The retention sweep reaches the agreement tables only, so a `retain` here is a decision no code acts on — and a retain nothing expires is the rejected exclusion under another name. A tripwire fails the day the sweep gains an `inspections` reference, so the notice cannot quietly become false. `address` joins PII_HEURISTIC in this same commit, after the ruling and not before it, with all twelve columns it newly flags declared alongside it — widening first would have made twelve unconsidered out-of-scope entries the cheapest way back to green, and a widening that lands without the declarations turns the gate red for everyone else in flight. Proven to bite: removing either `property_address` or `company_address` fails the gate naming it. docs/compliance/erasure-heuristic-limits.md carried this as its open worked example and is updated to match — counts, the regex, what the address compensator now is, and the one thing that is still open. --- docs/compliance/erasure-heuristic-limits.md | 236 +++++++++++------- scripts/check-erasure-manifest.mjs | 11 +- server/lib/compliance/erasure-manifest.ts | 77 ++++-- server/lib/compliance/erasure-orchestrator.ts | 2 +- .../privacy/erasure-manifest-coverage.spec.ts | 72 ++++++ 5 files changed, 287 insertions(+), 111 deletions(-) diff --git a/docs/compliance/erasure-heuristic-limits.md b/docs/compliance/erasure-heuristic-limits.md index ff8f14254..0870cc14c 100644 --- a/docs/compliance/erasure-heuristic-limits.md +++ b/docs/compliance/erasure-heuristic-limits.md @@ -5,7 +5,7 @@ schema file, matches column names against a regex, and fails if a matching column has neither a rule in `ERASURE_MANIFEST` nor a reasoned entry in `ERASURE_OUT_OF_SCOPE`. -It is green today: `31 rules, 48 out-of-scope declarations`, exit 0. +It is green today: `44 rules, 57 out-of-scope declarations`, exit 0. This document exists because that sentence is easy to read as "erasure covers the schema", and it does not mean that. It means no column *whose name the @@ -20,12 +20,11 @@ not the reason for the next one. ## The mechanism, exactly ```js -// scripts/check-erasure-manifest.mjs:52 -const PII_HEURISTIC = /(email|phone|ip_address|user_agent|signature|client_name|full_name|recipient)/; +const PII_HEURISTIC = /(email|phone|ip_address|user_agent|signature|client_name|full_name|recipient|address)/; const isPiiColumn = (col) => PII_HEURISTIC.test(col) || col === "ip"; ``` -Eight substrings and one exact match. That is the entire model of "this column +Nine substrings and one exact match. That is the entire model of "this column might hold personal data". The gate is honest about what it does — it never claims to find PII, only to @@ -34,53 +33,77 @@ does and what a green run gets read as. --- -## The worked example: `inspections.property_address` +## The worked example: `inspections.property_address` — closed -`address` is not in the regex. +**This example is now resolved.** It is kept because how it hid is the general +lesson, and because the shape of the fix is the behaviour to copy. -Follow that one omission: +`address` was not in the regex. Follow that one omission as it stood: - `inspections.property_address` is `text('property_address').notNull()` - (`server/lib/db/schema/inspection/core.ts:13`), with nine geocoded siblings - beneath it (`:15`-`:24`: place id, street, city, state, zip, county, lat, - lng, geocoded-at). For a residential inspection ordered by the buyer or the + (`server/lib/db/schema/inspection/core.ts`), with nine geocoded siblings + beneath it (place id, street, city, state, zip, county, lat, lng, + geocoded-at). For a residential inspection ordered by the buyer or the homeowner, that is a person's home address, held against a named client through `inspection_people`. -- The gate never asks about it, because the name does not match. -- `inspections` therefore has **zero** entries in the manifest: no column rule - (`grep "table: 'inspections'"` in `server/lib/compliance/` returns nothing), - no out-of-scope entry, and no row rule. -- `runErasure` has fourteen hand-written per-table steps - (`server/lib/compliance/erasure-orchestrator.ts:232`, `:250`, `:270`, - `:276`, `:310`, `:324`, `:336`, `:345`, `:354`, `:372`, `:392`, `:417`, - `:433`, `:445`). None of them is `inspections`. -- Two tables away, `reports.title` **is** declared — `category: 'user.address'`, - `action: 'anonymize'`, `legalBasis: 'art_17_3_e'`, `retention: 'P6Y'` - (`erasure-manifest.ts:168`) — and executed, overwritten with - `'Inspection Report (details removed)'` (`erasure-orchestrator.ts:68`, - step at `:372`). - -So a consumer erasure today clears the report title and leaves the address. - -### The failure mode is not an under-report +- The gate never asked about it, because the name did not match. +- `inspections` therefore had **zero** entries in the manifest: no column rule, + no out-of-scope entry, no row rule. +- Two tables away, `reports.title` **was** declared — `category: 'user.address'`, + `action: 'anonymize'`, `legalBasis: 'art_17_3_e'`, `retention: 'P6Y'` — and + executed, overwritten with `'Inspection Report (details removed)'`. + +So a consumer erasure cleared the report title and left the address, and +nothing anywhere said so. + +### The failure mode was not an under-report If the gate merely missed a column, the cost would be a smaller number in a -coverage report. The actual cost is different and worse. - -`runErasure` sets `status: 'completed'` whenever no step threw -(`erasure-orchestrator.ts:453`) and writes that status into the append-only -`erasure_log` row (`:461`). Nothing in that path can know about a table it was -never told to visit, so the absence of `inspections` produces no warning, no -partial status, and no decision entry. The only caller -(`server/services/admin.service.ts:227`) spreads that summary straight through -to whatever operator surface invoked it, so a DSAR console records a completed -erasure over a record that still holds the subject's home address — and the -accountability log now says so in writing. +coverage report. The actual cost was different and worse. + +`runErasure` sets `status: 'completed'` whenever no step threw, and writes that +status into the append-only `erasure_log` row. Nothing in that path can know +about a table it was never told to visit, so the absence of `inspections` +produced no warning, no partial status, and no decision entry. The only caller +(`server/services/admin.service.ts`) spreads that summary straight through to +whatever operator surface invoked it, so a DSAR console recorded a completed +erasure over a record that still held the subject's home address — and the +accountability log said so in writing. A silent gap and a gap recorded as "completed" are not the same defect. The second one manufactures evidence that the first one was handled. -### One correction while we are here +### How it was closed + +The address family now carries `action: 'retain'` rules with +`legalBasis: 'art_17_3_e'` and a `retention: 'P6Y'` hint — one entry per column, +in `ERASURE_MANIFEST`. Declaring the family out of scope as "property data" was +the other option and was rejected: it was the cheapest way back to green, and a +red gate would have pushed a hurried reader straight at it. + +Two things about that fix are easy to misread, so both are stated at the rules +themselves: + +1. **The window is not a new number.** It is the tenant's existing + `tenant_configs.agreement_retention_years` (default 6). A second per-tenant + retention column would be two clocks answering the same question and drifting. +2. **Nothing enforces the window yet.** `retention-sweep.ts` reaches + `agreement_requests` and `agreement_signers` only. Until it learns about + `inspections`, a `retain` rule here is a recorded decision that no code acts + on — and a retain nothing ever expires is the rejected exclusion under a + different name. A tripwire in + `tests/unit/privacy/erasure-manifest-coverage.spec.ts` fails the day the sweep + gains an `inspections` reference, so the "not yet enforced" notice in the + manifest cannot quietly become false. + +One more thing worth knowing before reading a retain rule as an audit artefact: +a `retain` produces **no per-run entry** in `erasure_log`. The orchestrator's +`step()` records actions it executed, and a retain executes nothing; that is +true of all six retain rules that predate this one. The manifest, with its +stated basis and period, is the record of the decision. The run log is the +record of the writes. + +### One correction carried over from when this was open The manifest's own comment above the `reports.title` rule (`erasure-manifest.ts:162`-`:167`) says `title` is "the one free-text column a @@ -120,9 +143,15 @@ them was prompted by the gate. ### 2. Addresses, and location generally -Covered above. `street`, `city`, `zip`, `lat`, `lng`, `place_id` — none match. +`address` is in the pattern now, so `property_address`, `address_street`, +`address_city` and their siblings are reached. `street`, `city`, `zip`, `lat`, +`lng`, `place_id` as bare names still are not — `company_lat` and +`service_origin_lat` are declared because somebody wrote them down, not because +anything asked. -**Compensator: none.** See "Open and unowned" below. +**Compensator: partial.** The pattern catches the `address_`-prefixed family +this codebase happens to use. A coordinate or locality column named without +that prefix is invisible again. ### 3. Sensitivity that is contextual rather than lexical @@ -153,9 +182,8 @@ person, or that a column is a locator rather than content. **Compensator:** the manifest's row-delete convention — `action: 'delete'` with the `column` naming the locator, documented at `erasure-manifest.ts:47`-`:54` — plus explicit out-of-scope entries for columns that ride along, e.g. -`contacts.phone`, "rides with the contacts row delete (locator = email)" -(`:207`). This one works, because the convention is written down where the -rules are. +`contacts.phone`, "rides with the contacts row delete (locator = email)". This +one works, because the convention is written down where the rules are. ### 6. A rule that exists but never runs @@ -168,6 +196,19 @@ against the executor. drift guard that fails when a rule has no orchestrator wiring. This is the one blind spot with a real mechanical answer. +It scans more than the orchestrator file — `anonymize-pii.ts` holds the shared +column SETs, and `erase-repair-requests.ts` is a step the orchestrator +delegates because it had run out of line budget. Widening what a guard reads is +how a guard gets weaker, so a companion assertion checks that the orchestrator +still *calls* the delegated step. Without it, a rule whose executor had been +unhooked would satisfy the scan while running nothing — the same failure this +section is about, with the drift guard helping it hide. + +Note also what the guard does **not** cover: `retain` rules. They are exempt by +construction, because a retain executes nothing there is anything to bind to. +Whether a retain is honoured is a question about the retention sweep, not the +orchestrator, and today the sweep only reaches the agreement tables. + ### 7. False positives `automations.recipient_kind` matches `recipient` and is an enum. @@ -181,8 +222,8 @@ that cost in written reasons is the intended trade. ## The behaviour to copy -`users.service_origin_address` is declared out of scope -(`erasure-manifest.ts:221`) with the reason "staff routing origin (may be a +`users.service_origin_address` is declared out of scope in +`ERASURE_OUT_OF_SCOPE` with the reason "staff routing origin (may be a home address) — staff offboarding lifecycle, not consumer-DSAR scope", and the comment above it says explicitly that it is "declared here so the decision is recorded rather than inferred from the PII heuristic not matching @@ -194,10 +235,13 @@ home address, so it is personal data — it is simply not a *consumer* data subject's, which is a decision, not an oversight. Several other entries do the same thing and say so in the same words — the -`reports` column sweep (`:252`-`:262`), the payment ledger (`:273`-`:278`), -`tenant_legal_versions` (`:279`-`:280`), the pay-split rows (`:286`-`:291`), -and `parked_cmd_events` (`:295`-`:298`), which notes that "silence here is -exactly how this one hid: `envelope` and `reason` look like nothing". +`reports` column sweep, the `order_payments` ledger, `tenant_legal_versions`, +the `inspection_service_pay_splits` rows, the `repair_request_items` report +snapshots, and `parked_cmd_events`, which notes that "silence here is exactly +how this one hid: `envelope` and `reason` look like nothing". + +(Line numbers are deliberately absent here. The earlier drafts of this page +carried them and every one went stale the first time a rule was added above.) **That is the discipline the heuristic cannot produce.** Ruling on a column the gate did not flag is the only thing that closes the categories above, and @@ -209,47 +253,51 @@ the entry for each one. --- -## Open and unowned - -The address gap described above is **not fixed, and nobody owns it.** This -document makes it legible; it does not close it. - -Two reasons it is left open deliberately rather than patched here: - -1. **It is a compliance decision, not an engineering one.** Mirroring the - `reports.title` treatment — anonymise with an Art. 17(3)(e) basis and a - bounded retention period — is the obvious call, and it changes what erasure - does to production data on every future request. A property address may be - personal data of the client where it is linked to the client relationship, - and it may also be the thing a professional record has to keep in order to - identify which property a report describes. Which of those wins, and for - how long, is a question for a human with authority to answer it. - -2. **Widening the regex is not a neutral first step.** Adding `address` to - `PII_HEURISTIC` today turns the gate red on **twelve** columns, measured - 2026-08-07: - - ``` - inspections.property_address inspections.address_county - inspections.address_place_id inspections.address_lat - inspections.address_street inspections.address_lng - inspections.address_city inspections.address_geocoded_at - inspections.address_state inspection_requests.property_address - inspections.address_zip tenant_configs.company_address - ``` - - A red gate on twelve columns invites the cheapest way back to green, which - is twelve out-of-scope entries. An out-of-scope entry without a real reason - is worse than a missing rule: it converts an open question into a recorded - decision nobody will revisit. Widening the pattern is the *last* step, once - the ruling exists — not the first. - -Not all twelve are the same question. `tenant_configs.company_address` is the -controller's own business address and mirrors the `company_lat`/`company_lng` -entries already at `erasure-manifest.ts:233`-`:234`. -`inspection_requests.property_address` sits on a table whose identity columns -already carry rules (`:135`-`:137`), so it is the odd one out on a table that -was otherwise ruled on. The `inspections` family is the substantive one. - -Until that decision is made, read the gate's green output as it is written: -*no column whose name suggests PII is unruled*. Not *no PII is unruled*. +## The order the address family was closed in, and why it matters + +Widening the regex is not a neutral first step, and doing it last is the part +worth copying. + +Adding `address` to `PII_HEURISTIC` turns the gate red on **twelve** columns +(measured 2026-08-07, before the ruling existed): + +``` +inspections.property_address inspections.address_county +inspections.address_place_id inspections.address_lat +inspections.address_street inspections.address_lng +inspections.address_city inspections.address_geocoded_at +inspections.address_state inspection_requests.property_address +inspections.address_zip tenant_configs.company_address +``` + +A red gate on twelve columns invites the cheapest way back to green, which is +twelve out-of-scope entries. An out-of-scope entry without a real reason is +worse than a missing rule: it converts an open question into a recorded +decision nobody will revisit. So the ruling came first and the widening came +second, in the same commit as the twelve declarations — a widening that lands +without them turns the gate red for everyone else in flight. + +Not all twelve were the same question, and they did not get the same answer: + +- **Ten are retained** — the nine `inspections` address columns and + `inspection_requests.property_address`. On a residential inspection this is + where a person lives, and the booking request the inspection converted from + has already had its name, email and phone cleared in place, which left the + address as the last part of that record standing. Same question, same answer. +- **`tenant_configs.company_address` is out of scope.** It is a business's own + published location — the controller's identity, not a data subject's — so it + follows the `company_lat` / `company_lng` entries beside it, not the + `inspections` rules. It looks identical to a name-matching gate and is not the + same question, which is the whole reason a gate cannot make this call. +- **`inspections.address_geocoded_at` is out of scope.** It records when the + geocode ran, not where the property is. + +Read the gate's green output as it is written: *no column whose name suggests +PII is unruled*. Not *no PII is unruled*. + +## Still open + +The retention **window is declared but not enforced** — see "How it was closed" +above. Nothing expires an inspection address today. That is the live gap, it is +named at the rules themselves, and a test fails the moment it closes so the +notice cannot go stale silently. diff --git a/scripts/check-erasure-manifest.mjs b/scripts/check-erasure-manifest.mjs index 484ea1e3c..3952f5fe5 100644 --- a/scripts/check-erasure-manifest.mjs +++ b/scripts/check-erasure-manifest.mjs @@ -39,6 +39,15 @@ * The heuristic deliberately includes `recipient` (automation_logs.recipient * holds emails and E.164 numbers — renamed from recipient_email, which is how * it escaped the original pattern) and bare `ip`. + * + * `address` was added only AFTER the address family was ruled on, and the order + * was the point. Widening the pattern first would have turned the gate red on + * twelve columns at once and made twelve out-of-scope entries the cheapest way + * back to green — converting an open question into a recorded decision nobody + * would revisit. `docs/compliance/erasure-heuristic-limits.md` says the same + * thing at more length, and names what this gate still cannot see: read it + * before treating a green run as coverage. Whatever the next widening is, rule + * on the columns first, then widen. */ import { readFileSync, readdirSync, statSync } from "node:fs"; import { join } from "node:path"; @@ -49,7 +58,7 @@ const SCHEMA_DIR = join(ROOT, "server", "lib", "db", "schema"); const VALID_ACTIONS = new Set(["delete", "null", "hash", "retain", "anonymize"]); const REQUIRES_BASIS = new Set(["anonymize", "retain"]); -const PII_HEURISTIC = /(email|phone|ip_address|user_agent|signature|client_name|full_name|recipient)/; +const PII_HEURISTIC = /(email|phone|ip_address|user_agent|signature|client_name|full_name|recipient|address)/; const isPiiColumn = (col) => PII_HEURISTIC.test(col) || col === "ip"; const errors = []; diff --git a/server/lib/compliance/erasure-manifest.ts b/server/lib/compliance/erasure-manifest.ts index d1c4c4a4e..9c1a4df30 100644 --- a/server/lib/compliance/erasure-manifest.ts +++ b/server/lib/compliance/erasure-manifest.ts @@ -11,7 +11,7 @@ * * Executor: `erasure-orchestrator.ts` — the concrete Drizzle executor that * realizes these rules. Binding verified by - * `tests/unit/erasure-manifest-coverage.spec.ts` (drift guard). + * `tests/unit/privacy/erasure-manifest-coverage.spec.ts` (drift guard). */ /** @@ -197,6 +197,49 @@ export const ERASURE_MANIFEST: ErasureRule[] = [ { table: 'repair_requests', column: 'created_by_ref', category: 'user.contact.email', action: 'delete' }, { table: 'repair_requests', column: 'custom_intro', category: 'user.freetext', action: 'null' }, { table: 'repair_request_items', column: 'note', category: 'user.freetext', action: 'null' }, + + // ── the property address family ─────────────────────────────────────────── + // A property address is not automatically non-personal data: on a + // residential inspection ordered by the buyer or the homeowner it is where a + // person lives, held against a named client through `inspection_people`. + // Declaring the family out of scope as "property data" was considered and + // REJECTED — it was the cheapest way back to green and the one a red gate + // pushes you toward, which is why it was not ours to decide alone. + // + // RETAINED under Art. 17(3)(e) instead: the address identifies which + // property a report describes, and the report is the inspector's defence + // against a negligence claim. One entry per column, no wildcard — an auditor + // reads this file, and a wildcard hides what was actually considered. + // + // Retained means FOR A PERIOD. The bound is the tenant's existing + // `tenant_configs.agreement_retention_years` (default 6, hence 'P6Y'), NOT a + // second retention column: both windows answer the same question — how long + // a professional record must survive — for the same tenant under the same + // state rules and the same E&O cover, and two clocks that start equal drift. + // + // ⚠️ NOT YET ENFORCED, and read nothing else into that. `retention-sweep.ts` + // reaches `agreement_requests` and `agreement_signers` only; nothing expires + // an inspection address today, so these rules record a decision no code acts + // on yet. That gap is the distance between the rule as written and the rule + // as honoured — NOT a licence to read 'retain' as 'forever', which is the + // rejected exclusion under another name. The tripwire in + // `tests/unit/privacy/erasure-manifest-coverage.spec.ts` fails the day the + // sweep learns about `inspections`, so this notice cannot outlive its gap. + { table: 'inspections', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_place_id', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_street', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_city', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_state', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_zip', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_county', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_lat', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'inspections', column: 'address_lng', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + // The booking request the inspection was converted from. Its client_name / + // client_email / client_phone are already cleared in place above while the + // ROW survives, so the address is the one part of that record still + // standing — the same question, answered the same way rather than + // differently by omission. + { table: 'inspection_requests', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, ]; /** @@ -250,6 +293,12 @@ export const ERASURE_OUT_OF_SCOPE: ErasureOutOfScopeEntry[] = [ { table: 'tenant_configs', column: 'company_phone', reason: 'company-owned phone' }, { table: 'tenant_configs', column: 'company_lat', reason: 'company office coordinate — controller business identity' }, { table: 'tenant_configs', column: 'company_lng', reason: 'company office coordinate — controller business identity' }, + // The address family's other half. `company_address` is a business's own + // published location — the controller's identity, not a data subject's — + // so it follows its `company_lat`/`company_lng` siblings above and NOT the + // `inspections` rules, where the address is somebody's home. The two look + // alike to a name-matching gate and are not the same question. + { table: 'tenant_configs', column: 'company_address', reason: 'company office address — controller business identity, published on reports and invoices' }, // Heuristic false positives — config values and references, not PII. { table: 'tenant_configs', column: 'email_mode', reason: 'config enum, not personal data' }, @@ -325,18 +374,16 @@ export const ERASURE_OUT_OF_SCOPE: ErasureOutOfScopeEntry[] = [ // from a ruled source. It is the same call, made here for the first time. // The subject's OWN lists never reach this reasoning — those rows are // deleted whole by the `created_by_ref` rule above. - { table: 'repair_request_items', column: 'comment_snapshot', - reason: 'frozen copy of the inspector-authored defect comment on the published report — professional content about the property, not prose about or by the data subject' }, - { table: 'repair_request_items', column: 'defect_title_snapshot', - reason: 'frozen copy of the report defect title — inspector-authored content about the property' }, - { table: 'repair_request_items', column: 'location_snapshot', - reason: 'frozen copy of the defect location WITHIN the property ("primary bathroom"), not a postal address' }, - { table: 'repair_request_items', column: 'category_snapshot', - reason: 'frozen copy of the report defect category — tenant taxonomy value, not personal data' }, - { table: 'repair_request_items', column: 'trade_snapshot', - reason: 'resolved trade label ("licensed roofer") snapshotted at add time — tenant taxonomy value, not personal data' }, - { table: 'repair_request_items', column: 'section_title', - reason: 'frozen copy of the report section heading — template structure, not personal data' }, - { table: 'repair_request_items', column: 'item_label', - reason: 'frozen copy of the report item label — template structure, not personal data' }, + { table: 'repair_request_items', column: 'comment_snapshot', reason: 'frozen copy of the inspector-authored defect comment on the published report — professional content about the property, not prose about or by the data subject' }, + { table: 'repair_request_items', column: 'defect_title_snapshot', reason: 'frozen copy of the report defect title — inspector-authored content about the property' }, + { table: 'repair_request_items', column: 'location_snapshot', reason: 'frozen copy of the defect location WITHIN the property ("primary bathroom"), not a postal address' }, + { table: 'repair_request_items', column: 'category_snapshot', reason: 'frozen copy of the report defect category — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'trade_snapshot', reason: 'resolved trade label ("licensed roofer") snapshotted at add time — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'section_title', reason: 'frozen copy of the report section heading — template structure, not personal data' }, + { table: 'repair_request_items', column: 'item_label', reason: 'frozen copy of the report item label — template structure, not personal data' }, + // Sits inside the address family by name and outside it by substance: it + // records WHEN the geocode ran, not where the property is. Excluded rather + // than retained so the retain rules above stay a list of columns that + // actually hold the address. + { table: 'inspections', column: 'address_geocoded_at', reason: 'timestamp recording when the address was geocoded — a processing record, not the address itself' }, ]; diff --git a/server/lib/compliance/erasure-orchestrator.ts b/server/lib/compliance/erasure-orchestrator.ts index dc801368a..47a7ca5bd 100644 --- a/server/lib/compliance/erasure-orchestrator.ts +++ b/server/lib/compliance/erasure-orchestrator.ts @@ -32,7 +32,7 @@ * source of truth; this orchestrator is the concrete Drizzle executor that * realizes those rules with tenant-scoped, row-state-aware SQL. * - * Binding: `tests/unit/erasure-manifest-coverage.spec.ts` asserts every + * Binding: `tests/unit/privacy/erasure-manifest-coverage.spec.ts` asserts every * manifest anonymize/delete/null rule is referenced in this file, preventing * silent manifest↔orchestrator drift. */ diff --git a/tests/unit/privacy/erasure-manifest-coverage.spec.ts b/tests/unit/privacy/erasure-manifest-coverage.spec.ts index 272b1a0e1..5235090a3 100644 --- a/tests/unit/privacy/erasure-manifest-coverage.spec.ts +++ b/tests/unit/privacy/erasure-manifest-coverage.spec.ts @@ -143,3 +143,75 @@ describe('portal #88 — the repair-request columns', () => { expect(rule?.action).toBe('delete'); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// The property address family. +// +// The columns a widened PII heuristic flags. Two of them are a different +// question from the other ten and are settled by exclusion; the rest are +// RETAINED under Art. 17(3)(e) with a bounded window. Classifying the family as +// out of scope was considered and rejected — these tests are what stops that +// decision from being quietly re-made later, because the exclusion is cheaper +// to write and looks identical in a green gate. +// ───────────────────────────────────────────────────────────────────────────── +const INSPECTION_ADDRESS_COLUMNS = [ + 'property_address', 'address_place_id', 'address_street', 'address_city', + 'address_state', 'address_zip', 'address_county', 'address_lat', 'address_lng', +]; +const RETAINED_ADDRESS_COLUMNS = [ + ...INSPECTION_ADDRESS_COLUMNS.map((c) => `inspections.${c}`), + 'inspection_requests.property_address', +]; +const ADDRESS_FAMILY = [ + ...RETAINED_ADDRESS_COLUMNS, + 'inspections.address_geocoded_at', + 'tenant_configs.company_address', +]; + +describe('the property address family', () => { + it.each(ADDRESS_FAMILY)('%s is decided', (key) => { + expect(DECIDED.has(key)).toBe(true); + }); + + it.each(RETAINED_ADDRESS_COLUMNS)('%s is retained, not excluded', (key) => { + const [table, column] = key.split('.'); + const rule = ERASURE_MANIFEST.find((r) => r.table === table && r.column === column); + expect(rule, `${key} has no manifest rule`).toBeTruthy(); + expect(rule!.action).toBe('retain'); + expect(rule!.legalBasis).toBe('art_17_3_e'); + expect( + ERASURE_OUT_OF_SCOPE.some((e) => `${e.table}.${e.column}` === key), + `${key} is declared out of scope. That option was rejected: a property address on a ` + + 'residential inspection can be where a person lives, so it is retained with a stated ' + + 'basis and a bounded window, never waved through as property data.', + ).toBe(false); + }); + + it.each(RETAINED_ADDRESS_COLUMNS)('%s states a bounded period', (key) => { + const [table, column] = key.split('.'); + const rule = ERASURE_MANIFEST.find((r) => r.table === table && r.column === column); + expect( + rule!.retention, + `${key} is retained with no period. "Retained" means for a period; an unbounded ` + + 'retain is the exclusion this ruling rejected, wearing a different label.', + ).toMatch(/^P\d+Y$/); + }); + + // A TRIPWIRE, not a requirement. Nothing expires an inspection address + // today, so the rules above record a decision no code acts on, and the + // manifest says so where the rules are. The day the sweep learns about + // `inspections`, that notice becomes false — and a false "not yet enforced" + // is worse than none, because it tells a reader to go looking for a gap + // that has been closed. This fails then, so the notice cannot outlive it. + it('the retention sweep does not yet enforce the inspection-record window', () => { + const sweep = stripComments(fs.readFileSync(retentionSweepPath, 'utf8')); + expect( + /\binspections\b/.test(sweep), + 'retention-sweep.ts now references `inspections`. If the sweep expires the property ' + + 'address family, delete the "NOT YET ENFORCED" notice above those rules in ' + + 'erasure-manifest.ts and delete this test. If it references inspections for some ' + + 'other reason, narrow this check rather than removing it — the notice is only ' + + 'honest while nothing acts on the window.', + ).toBe(false); + }); +}); From bf79cd35406ee6aadd72161e382413868be8e343 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 11:43:14 +0800 Subject: [PATCH 06/10] feat(erasure): a pending retain must say so, carry a date, and be on a list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Counsel approved shipping the address retain rules ahead of the sweep only on three conditions, because a bounded retain that nothing enforces becomes an unbounded retain — the blanket exclusion refused in August, relabelled. The rule itself is unchanged; these sit on top of it. 1. The rules no longer read as implemented. Every one carries `enforcementStatus: 'pending'`, a new optional field on `ErasureRule`, so the manifest and anything rendering from it can tell a recorded DECISION from a shipped behaviour. 2. `enforcementDeadline: '2027-02-01'`, and the gate FAILS once it passes. Two quarters, set by review discipline rather than by first breach: the sweep needs a purge marker on `inspections` (the agreement pass keys on `signedAt` + `purged_at IS NULL` and there is no equivalent) and a decision about which column starts an inspection's clock — a schema change and a migration, not a patch. Deliberately NOT derived from when the first address falls due, which is not computable until that clock column exists. Moving the date is allowed; moving it silently is what this prevents. 3. The gate refuses NEW unenforced retain rules. `PENDING_ENFORCEMENT` is a checked-in list of the ten rules allowed to be pending, checked BOTH ways — a pending rule missing from it fails, and a stale entry whose rule is gone fails too, so the list cannot decay into a blanket permit. For a bounded retain the DEFAULT is refusal: a `retain` that declares a `retention` and no `enforcementStatus` fails, so "unenforced" is never what happens when nobody says anything. The two swept signature_base64 rules are marked 'enforced'. `ERASURE_OUT_OF_SCOPE` moves to `erasure-out-of-scope.ts`. The manifest was at 389 of 400 lines and this note would have pushed it over; extracting beats compacting, because the thing that would have been compacted is the reasoning. The gate now concatenates both sources before parsing either, so splitting the register cannot halve what it sees — two lines, and both `arrayBody` calls work unchanged. Manifest 289, register 157. Also records at the top of the manifest that two of its justifications were checked against the code on 2026-08-07 and found false — `reports.title` ("a human writes"; it is machine-written) and `created_by_ref` (an "opaque id" holding an email). Both rules survived review because the reasoning read well. The warning is the general form: a premise stated in a comment is not evidence, so read what writes the column before relying on the paragraph. The `reports.title` rationale itself is untouched — its amendment history is being written separately so the two changes stay reviewable apart. --- docs/compliance/erasure-heuristic-limits.md | 61 ++++- scripts/check-erasure-manifest.mjs | 117 ++++++++- server/lib/compliance/erasure-manifest.ts | 232 +++++------------- server/lib/compliance/erasure-out-of-scope.ts | 157 ++++++++++++ .../privacy/erasure-manifest-coverage.spec.ts | 21 +- 5 files changed, 407 insertions(+), 181 deletions(-) create mode 100644 server/lib/compliance/erasure-out-of-scope.ts diff --git a/docs/compliance/erasure-heuristic-limits.md b/docs/compliance/erasure-heuristic-limits.md index 0870cc14c..bf6f63ff4 100644 --- a/docs/compliance/erasure-heuristic-limits.md +++ b/docs/compliance/erasure-heuristic-limits.md @@ -2,8 +2,10 @@ `scripts/check-erasure-manifest.mjs` is a CI gate. It walks every Drizzle schema file, matches column names against a regex, and fails if a matching -column has neither a rule in `ERASURE_MANIFEST` nor a reasoned entry in -`ERASURE_OUT_OF_SCOPE`. +column has neither a rule in `ERASURE_MANIFEST` (`erasure-manifest.ts`) nor a +reasoned entry in `ERASURE_OUT_OF_SCOPE` (`erasure-out-of-scope.ts`, split out +when the manifest hit its line cap). The gate concatenates both sources before +parsing either, so the split cannot halve what it sees. It is green today: `44 rules, 57 out-of-scope declarations`, exit 0. @@ -91,10 +93,32 @@ themselves: `agreement_requests` and `agreement_signers` only. Until it learns about `inspections`, a `retain` rule here is a recorded decision that no code acts on — and a retain nothing ever expires is the rejected exclusion under a - different name. A tripwire in - `tests/unit/privacy/erasure-manifest-coverage.spec.ts` fails the day the sweep - gains an `inspections` reference, so the "not yet enforced" notice in the - manifest cannot quietly become false. + different name. + + Three mechanisms hold that open rather than letting it settle: + + - Every one of those rules carries `enforcementStatus: 'pending'`, so the + manifest — and anything rendering from it — states that the *decision* is + recorded while the *expiry* is not built. A `retain` that reads as + implemented is the failure mode. + - Every one carries `enforcementDeadline: '2027-02-01'`, and **the gate fails + once that date passes.** A deadline that cannot act is how "pending" + becomes permanent. The date is two quarters, set by review discipline: the + sweep needs a purge marker on `inspections` (the agreement pass keys on + `signedAt` + `purged_at IS NULL`; there is no equivalent here) and a + decision about which column starts an inspection's clock — a schema change + and a migration, not a patch. It is deliberately *not* derived from when + the first address falls due, which is not computable until that clock + column exists. + - `PENDING_ENFORCEMENT` in the gate is a checked-in list of the rules allowed + to be pending. A new one fails; so does a stale entry whose rule is gone. + For a bounded `retain`, the default is refusal — a rule that declares a + `retention` and no `enforcementStatus` fails, so "unenforced" cannot be the + thing that happens when nobody says anything. + + A tripwire in `tests/unit/privacy/erasure-manifest-coverage.spec.ts` also + fails the day the sweep gains an `inspections` reference, so the "not yet + enforced" notice cannot quietly become false in the other direction either. One more thing worth knowing before reading a retain rule as an audit artefact: a `retain` produces **no per-run entry** in `erasure_log`. The orchestrator's @@ -297,7 +321,24 @@ PII is unruled*. Not *no PII is unruled*. ## Still open -The retention **window is declared but not enforced** — see "How it was closed" -above. Nothing expires an inspection address today. That is the live gap, it is -named at the rules themselves, and a test fails the moment it closes so the -notice cannot go stale silently. +The retention **window is declared but not enforced**, deadline **2027-02-01** — +see "How it was closed" above. Nothing expires an inspection address today. That +is the live gap; it is marked on every affected rule, dated, held by a +checked-in list, and the gate turns red if the date passes without the sweep. + +The blockers are a purge marker on `inspections` and a decision about which +column starts its retention clock. Both are schema work. + +## What this file's prose is worth + +On 2026-08-07 two justifications in the manifest were checked against the code +and found false — `reports.title` ("the one free-text column a human writes"; +it is machine-written) and `repair_requests.created_by_ref` (documented as an +opaque id; it stores an email address, and a compliance classification was +resting on that). Both rules had survived review because the reasoning read +well. + +The general form is worth more than either instance: **the manifest may be +assumption-driven rather than data-driven.** A rule can be correct and still +unprovable, because nobody checked the premise its comment asserts. Before +relying on a paragraph in that file, read what writes the column. diff --git a/scripts/check-erasure-manifest.mjs b/scripts/check-erasure-manifest.mjs index 3952f5fe5..dbfc9cc96 100644 --- a/scripts/check-erasure-manifest.mjs +++ b/scripts/check-erasure-manifest.mjs @@ -53,17 +53,60 @@ import { readFileSync, readdirSync, statSync } from "node:fs"; import { join } from "node:path"; const ROOT = new URL("..", import.meta.url).pathname.replace(/^\/([A-Za-z]:)/, "$1"); +// The manifest and its out-of-scope register are two files and one document: +// a column is covered by a rule in the first or excused by an entry in the +// second, and neither array means anything without the other. They are read as +// one concatenated source so `arrayBody` finds whichever it is asked for, and +// so splitting the register out for line-count reasons could not quietly halve +// what this gate sees. A missing file throws here rather than parsing as empty. const MANIFEST = join(ROOT, "server", "lib", "compliance", "erasure-manifest.ts"); +const OUT_OF_SCOPE = join(ROOT, "server", "lib", "compliance", "erasure-out-of-scope.ts"); const SCHEMA_DIR = join(ROOT, "server", "lib", "db", "schema"); const VALID_ACTIONS = new Set(["delete", "null", "hash", "retain", "anonymize"]); const REQUIRES_BASIS = new Set(["anonymize", "retain"]); +const VALID_ENFORCEMENT = new Set(["enforced", "pending"]); + +/** + * The ONLY rules allowed to say `enforcementStatus: 'pending'`. + * + * A `retain` rule that promises a bounded `retention` and has nothing to expire + * it is not a bounded retain — it is a permanent one that reads as temporary, + * which is the blanket exclusion this manifest exists to avoid. Existing + * remediation is allowed to be in flight; ADDING another one is not a thing a + * developer should be able to do by typing a keyword. Landing here is a diff + * somebody has to approve. + * + * The check runs BOTH ways: a pending rule missing from this list fails, and a + * list entry with no matching pending rule also fails. The second direction is + * what stops the list decaying into a blanket permit after the rules it named + * are gone. + * + * To remove an entry: build the enforcement, flip the rule to + * `enforcementStatus: 'enforced'`, delete the line here. + */ +const PENDING_ENFORCEMENT = new Set([ + // The property address family. Retained under Art. 17(3)(e) for the tenant's + // record window; `retention-sweep.ts` does not reach `inspections` yet, so + // nothing expires them. See the NOT YET ENFORCED block in the manifest for + // the two blockers and why the deadline is where it is. + "inspections.property_address", + "inspections.address_place_id", + "inspections.address_street", + "inspections.address_city", + "inspections.address_state", + "inspections.address_zip", + "inspections.address_county", + "inspections.address_lat", + "inspections.address_lng", + "inspection_requests.property_address", +]); const PII_HEURISTIC = /(email|phone|ip_address|user_agent|signature|client_name|full_name|recipient|address)/; const isPiiColumn = (col) => PII_HEURISTIC.test(col) || col === "ip"; const errors = []; -const src = readFileSync(MANIFEST, "utf8"); +const src = `${readFileSync(MANIFEST, "utf8")}\n${readFileSync(OUT_OF_SCOPE, "utf8")}`; /** Extract the body of a top-level `export const NAME = [ ... ];` array. */ function arrayBody(text, name) { @@ -150,6 +193,78 @@ rules.forEach((rule, i) => { } }); +// ── Enforcement of bounded retention ───────────────────────────────────────── +// A `retain` rule that names a period is a promise the data goes away when the +// period elapses. Nothing in this repo can prove a sweep actually runs, so what +// is checked instead is that somebody SAID which it is, and that "not yet" is +// bounded by a list and a date rather than by nobody looking. +const seenPending = new Set(); +rules.forEach((rule, i) => { + const key = `${rule.table}.${rule.column}`; + const label = `rule #${i + 1} (${key})`; + + if (rule.enforcementStatus && !VALID_ENFORCEMENT.has(rule.enforcementStatus)) { + errors.push( + `${label}: invalid enforcementStatus '${rule.enforcementStatus}' (allowed: ${[...VALID_ENFORCEMENT].join(", ")}).`, + ); + return; + } + + // The default is REFUSAL, not "enforced". A new bounded retain has to declare + // what expires it; that is the whole point of this block. + if (rule.action === "retain" && rule.retention && !rule.enforcementStatus) { + errors.push( + `${label}: 'retain' with retention '${rule.retention}' must declare enforcementStatus ` + + `('enforced' if a sweep expires it, 'pending' if that is not built yet). A bounded ` + + `retain nothing enforces is an unbounded retain.`, + ); + } + + if (rule.enforcementStatus !== "pending") return; + seenPending.add(key); + + if (!PENDING_ENFORCEMENT.has(key)) { + errors.push( + `${label}: NEW unenforced retain rule. '${key}' is marked pending but is not in ` + + `PENDING_ENFORCEMENT in this script. Existing remediation may be in flight; adding ` + + `another one is a reviewed decision, so put it on that list in the same change or ` + + `build the enforcement instead.`, + ); + } + + if (!/^\d{4}-\d{2}-\d{2}$/.test(rule.enforcementDeadline ?? "")) { + errors.push( + `${label}: pending rules require an 'enforcementDeadline' as YYYY-MM-DD. Without a date, ` + + `'pending' becomes permanent.`, + ); + return; + } + // Deadline in the past → FAIL. A deadline that cannot act is not a deadline; + // this is the same "expiry acts" principle the rule itself is about, applied + // to our own promise about it. Moving the date is allowed and visible. + const due = Date.parse(`${rule.enforcementDeadline}T23:59:59Z`); + if (Number.isNaN(due)) { + errors.push(`${label}: enforcementDeadline '${rule.enforcementDeadline}' is not a real date.`); + } else if (Date.now() > due) { + errors.push( + `${label}: enforcement deadline ${rule.enforcementDeadline} has PASSED and the retention ` + + `is still not enforced. Build it, or move the date deliberately and say why — an ` + + `expired "pending" is the unbounded retain this check exists to prevent.`, + ); + } +}); + +// The list must not outlive the rules it names, or it quietly becomes a blanket +// permit for whatever lands on it next. +for (const key of PENDING_ENFORCEMENT) { + if (!seenPending.has(key)) { + errors.push( + `PENDING_ENFORCEMENT lists '${key}', but no manifest rule is marked pending for it. ` + + `If the enforcement shipped, delete the line; if the rule moved, update it.`, + ); + } +} + // ── Out-of-scope set (table.column the manifest deliberately skips) ─────────── // Every entry MUST carry a reason — an out-of-scope declaration without one is // indistinguishable from a shrug, and the reason is what a DSAR audit reads. diff --git a/server/lib/compliance/erasure-manifest.ts b/server/lib/compliance/erasure-manifest.ts index 9c1a4df30..b855eeb1a 100644 --- a/server/lib/compliance/erasure-manifest.ts +++ b/server/lib/compliance/erasure-manifest.ts @@ -9,6 +9,16 @@ * * G2 fills `ERASURE_MANIFEST`; this scaffold (G1) ships the type + an empty array. * + * ⚠️ HOW MUCH WEIGHT THIS FILE'S PROSE CAN CARRY. On 2026-08-07 two separate + * justifications in here were checked against the code and found FALSE: the + * `reports.title` rule describes a column "a human writes" that is in fact + * machine-written with no API that can edit it, and `repair_requests. + * created_by_ref` was documented as an opaque id while the code stores an email + * address in it — a compliance classification resting on a comment that had been + * wrong for as long as the feature existed. Both rules survived review because + * their reasoning read well. A premise stated in a comment is not evidence: + * before you rely on one of these paragraphs, go read what writes the column. + * * Executor: `erasure-orchestrator.ts` — the concrete Drizzle executor that * realizes these rules. Binding verified by * `tests/unit/privacy/erasure-manifest-coverage.spec.ts` (drift guard). @@ -39,6 +49,26 @@ export interface ErasureRule { retention?: string; /** Row-state predicate restricting which rows this rule applies to. */ condition?: 'signed_only' | 'draft_only'; + /** + * Whether anything ACTUALLY expires this data, for rules that promise a + * bounded `retention`. Required on every `retain` rule that declares one, + * with no default: a retain nobody enforces is an unbounded retain, which is + * the blanket exclusion this manifest exists to avoid, and silence is how it + * would get there. 'enforced' = a sweep acts when the window elapses; + * 'pending' = the decision is recorded, the expiry is not built yet. + * + * `pending` is not self-service. `scripts/check-erasure-manifest.mjs` holds + * a checked-in list of the rules allowed to be pending and refuses any + * other, so adding one is a reviewed diff rather than a keyword. + */ + enforcementStatus?: 'enforced' | 'pending'; + /** + * ISO date (YYYY-MM-DD) by which a `pending` rule must become enforced. + * Required when `enforcementStatus` is 'pending', and the gate FAILS once it + * passes — a deadline that cannot act is how "pending" becomes permanent. + * Moving it is allowed; moving it silently is what this prevents. + */ + enforcementDeadline?: string; } /** @@ -155,8 +185,8 @@ export const ERASURE_MANIFEST: ErasureRule[] = [ { table: 'erasure_log', column: 'subject_email', category: 'user.contact.email', action: 'retain', legalBasis: 'art_17_3_b' }, // Signature evidence kept on a DSAR (the retention sweep destroys it past // the window); the esign audit chain is NEVER touched. - { table: 'agreement_signers', column: 'signature_base64', category: 'user.biometric.signature', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'agreement_requests', column: 'signature_base64', category: 'user.biometric.signature', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + { table: 'agreement_signers', column: 'signature_base64', category: 'user.biometric.signature', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'enforced' }, + { table: 'agreement_requests', column: 'signature_base64', category: 'user.biometric.signature', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'enforced' }, { table: 'esign_audit_logs', column: 'signature', category: 'system.integrity', action: 'retain', legalBasis: 'art_17_3_e' }, // ── reports ─────────────────────────────────────────────────────────────── @@ -217,173 +247,43 @@ export const ERASURE_MANIFEST: ErasureRule[] = [ // a professional record must survive — for the same tenant under the same // state rules and the same E&O cover, and two clocks that start equal drift. // - // ⚠️ NOT YET ENFORCED, and read nothing else into that. `retention-sweep.ts` - // reaches `agreement_requests` and `agreement_signers` only; nothing expires - // an inspection address today, so these rules record a decision no code acts - // on yet. That gap is the distance between the rule as written and the rule - // as honoured — NOT a licence to read 'retain' as 'forever', which is the - // rejected exclusion under another name. The tripwire in - // `tests/unit/privacy/erasure-manifest-coverage.spec.ts` fails the day the - // sweep learns about `inspections`, so this notice cannot outlive its gap. - { table: 'inspections', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_place_id', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_street', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_city', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_state', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_zip', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_county', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_lat', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, - { table: 'inspections', column: 'address_lng', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, + // ⚠️ NOT YET ENFORCED — deadline 2027-02-01, carried on every rule below as + // `enforcementStatus: 'pending'` so nothing can read them as implemented. + // `retention-sweep.ts` reaches `agreement_requests` and `agreement_signers` + // only; nothing expires an inspection address today, so these rules record a + // decision no code acts on yet. That gap is the distance between the rule as + // written and the rule as honoured — NOT a licence to read 'retain' as + // 'forever', which is the rejected exclusion under another name. + // + // Why that date, so the next reader can argue with it rather than inherit + // it. The sweep is not a patch: `inspections` has no purge marker (the + // agreement pass keys on `signedAt` + `purged_at IS NULL`, and there is no + // equivalent here), so an idempotent sweep needs a schema change and a + // migration first; and nobody has yet chosen which column starts the clock + // for an inspection. Two quarters covers that work with review, and is short + // enough to land on someone who still holds the context. It is NOT derived + // from when the first address actually falls due — that is not computable + // until the clock column exists, which is precisely why the date has to come + // from review discipline instead. + // + // Two mechanisms keep this honest. The gate refuses any pending rule that is + // not on its checked-in list, and FAILS outright once the deadline passes. + // The tripwire in `tests/unit/privacy/erasure-manifest-coverage.spec.ts` + // fails the day the sweep learns about `inspections`, so this notice cannot + // outlive its gap either. + { table: 'inspections', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_place_id', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_street', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_city', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_state', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_zip', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_county', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_lat', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, + { table: 'inspections', column: 'address_lng', category: 'user.location', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, // The booking request the inspection was converted from. Its client_name / // client_email / client_phone are already cleared in place above while the // ROW survives, so the address is the one part of that record still // standing — the same question, answered the same way rather than // differently by omission. - { table: 'inspection_requests', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y' }, -]; - -/** - * A PII-heuristic column the manifest DELIBERATELY does not act on. Every entry - * must say why — the reason is what a DSAR audit reads, and the CI gate - * (`scripts/check-erasure-manifest.mjs`) hard-fails an entry without one. - * - * @gateConsumed `scripts/check-erasure-manifest.mjs` reads this declaration out - * of the SOURCE TEXT (`arrayBody(src, 'ERASURE_OUT_OF_SCOPE')`) rather than - * importing it — the gate is a plain .mjs script and the manifest is TypeScript. - * That consumption is invisible to a module-graph analyzer, so knip would report - * both symbols as dead. The tag (knip `tags: ["-gateConsumed"]`) says "a tool - * consumes this", which is true; a dead-code baseline entry would have said - * "this is dead and we tolerate it", which is not. - */ -export interface ErasureOutOfScopeEntry { - table: string; - column: string; - reason: string; -} - -/** @gateConsumed read as source text by `scripts/check-erasure-manifest.mjs`. */ -export const ERASURE_OUT_OF_SCOPE: ErasureOutOfScopeEntry[] = [ - // Columns that ride with a row-delete rule above (per-column scan cannot - // see row semantics). - { table: 'contacts', column: 'phone', reason: 'rides with the contacts row delete (locator = email)' }, - - // Staff, not data subjects. Consumer-DSAR erasure never touches employee - // accounts; staff offboarding is a separate lifecycle. - { table: 'users', column: 'email', reason: 'staff account — not consumer-DSAR scope' }, - { table: 'users', column: 'phone', reason: 'staff account — not consumer-DSAR scope' }, - { table: 'users', column: 'default_signature_base64', reason: 'inspector (staff) signature asset' }, - { table: 'users', column: 'is_signature_enabled', reason: 'boolean flag, not personal data' }, - // An inspector's routing origin can be their home address, so it IS personal - // data — it is simply not a CONSUMER data subject's. Same posture as - // users.email/phone above: consumer-DSAR erasure never touches it and there - // is deliberately no DSAR-export path for it. Declared here so the decision - // is recorded rather than inferred from the PII heuristic not matching - // 'service_origin_address'. - { table: 'users', column: 'service_origin_address', reason: 'staff routing origin (may be a home address) — staff offboarding lifecycle, not consumer-DSAR scope' }, - { table: 'users', column: 'service_origin_lat', reason: 'staff routing origin coordinate — not consumer-DSAR scope' }, - { table: 'users', column: 'service_origin_lng', reason: 'staff routing origin coordinate — not consumer-DSAR scope' }, - { table: 'tenant_invites', column: 'email', reason: 'staff invite — not consumer-DSAR scope' }, - { table: 'audit_logs', column: 'ip_address', reason: 'staff-action security audit trail' }, - { table: 'report_signoff', column: 'signature_ref', reason: 'inspector (staff) signoff reference' }, - { table: 'agreement_requests', column: 'inspector_signature_base64', reason: 'inspector (staff) countersignature' }, - - // The controller's own business identity, not a data subject's. - { table: 'tenant_configs', column: 'support_email', reason: 'company-owned support address' }, - { table: 'tenant_configs', column: 'sender_email', reason: 'company-owned sending address' }, - { table: 'tenant_configs', column: 'company_phone', reason: 'company-owned phone' }, - { table: 'tenant_configs', column: 'company_lat', reason: 'company office coordinate — controller business identity' }, - { table: 'tenant_configs', column: 'company_lng', reason: 'company office coordinate — controller business identity' }, - // The address family's other half. `company_address` is a business's own - // published location — the controller's identity, not a data subject's — - // so it follows its `company_lat`/`company_lng` siblings above and NOT the - // `inspections` rules, where the address is somebody's home. The two look - // alike to a name-matching gate and are not the same question. - { table: 'tenant_configs', column: 'company_address', reason: 'company office address — controller business identity, published on reports and invoices' }, - - // Heuristic false positives — config values and references, not PII. - { table: 'tenant_configs', column: 'email_mode', reason: 'config enum, not personal data' }, - { table: 'tenant_configs', column: 'email_byo_provider', reason: 'config enum, not personal data' }, - { table: 'automations', column: 'recipient_kind', reason: 'config enum, not personal data' }, - { table: 'automations', column: 'recipient_role_profile_id', reason: 'role-profile reference, not personal data' }, - { table: 'automations', column: 'email_template_id', reason: 'template reference, not personal data' }, - { table: 'automation_logs', column: 'recipient_role_key', reason: 'role key, not personal data' }, - { table: 'automation_logs', column: 'recipient_contact_id', reason: 'opaque id on the retained evidence ledger (see the automation_logs.recipient retain rule)' }, - { table: 'contact_role_profiles', column: 'email_template_id', reason: 'template reference, not personal data' }, - { table: 'sms_consent_log', column: 'recipient_type', reason: 'role-kind enum, not personal data' }, - { table: 'report_versions', column: 'signature', reason: 'report-content integrity seal, not personal data' }, - // ── reports ─────────────────────────────────────────────────────────────── - // A report is findings about a named person's property. Only `title` is - // free text a human writes, and it routinely carries the address ("123 Oak - // St — Radon"). The rest of the row is ids, enums and a timestamp, declared - // out of scope below. - { table: 'reports', column: 'inspection_id', reason: 'opaque id; the inspection row carries its own rules' }, - { table: 'reports', column: 'tenant_id', reason: 'tenant scope key, not personal data' }, - { table: 'reports', column: 'id', reason: 'opaque primary key' }, - { table: 'reports', column: 'kind', reason: 'primary/ancillary enum, not personal data' }, - { table: 'reports', column: 'inspection_service_id', reason: 'billing-line reference, not personal data' }, - { table: 'reports', column: 'template_id', reason: 'template reference, not personal data' }, - { table: 'reports', column: 'status', reason: 'workflow enum, not personal data' }, - { table: 'reports', column: 'created_at', reason: 'record timestamp, not personal data' }, - { table: 'reports', column: 'published_at', reason: 'record timestamp, not personal data' }, - { table: 'reports', column: 'notified_at', reason: 'record timestamp, not personal data' }, - { table: 'reports', column: 'sort_order', reason: 'presentation ordering integer, not personal data' }, - // The tenant's own published Privacy / Terms. `body_snapshot` is the - // company's prose, not a data subject's data, and the row's whole purpose is - // to be immutable — erasing it would destroy the record of what a document - // said at a date, which is the one thing it exists to answer. Listed rather - // than left silent because the PII heuristic does not flag any column here, - // and silence is not the same as a decision. - // The payment ledger. The `note` column has its own anonymize rule above; - // everything that carries a figure or an actor reference is declared here - // rather than left silent, because the heuristic flags none of it and - // silence is not the same as a decision. - { table: 'order_payments', column: 'amount_cents', - reason: 'financial record retained under accounting/tax obligation; carries no subject identifier on its own' }, - { table: 'order_payments', column: 'recorded_by', - reason: 'staff user id (who keyed the payment) — not consumer-DSAR scope' }, - { table: 'order_payments', column: 'provider_ref', - reason: 'payment-processor reference on the retained financial row, not personal data' }, - { table: 'tenant_legal_versions', column: 'body_snapshot', reason: 'company-authored policy text, not personal data of any data subject' }, - { table: 'tenant_legal_versions', column: 'published_by_user_id', reason: 'staff author reference — not consumer-DSAR scope' }, - // Pay splits (#278). A split is a payroll record about a STAFF member, held - // under accounting and employment obligations. A client's erasure request - // never reaches it — the client is not the data subject here. Declared - // rather than left silent: the PII heuristic flags none of these columns, - // and silence is not the same as a decision. - { table: 'inspection_service_pay_splits', column: 'user_id', - reason: 'payroll record for a staff member, retained under accounting and employment obligations; not client data, so a client erasure request does not reach it' }, - { table: 'inspection_service_pay_splits', column: 'reason', - reason: 'free text a manager writes about a payout adjustment to a staff member — payroll audit trail, not consumer-DSAR scope' }, - { table: 'service_pay_rules', column: 'user_id', - reason: 'staff compensation rule — not consumer-DSAR scope' }, - // The portal->core dead-letter queue (#276). Registered although the PII - // heuristic flags neither column, because silence here is exactly how this - // one hid: `envelope` and `reason` look like nothing. - { table: 'parked_cmd_events', column: 'envelope', - reason: 'Fingerprint only (type/dataschema/id/seq/size/digest) — the command payload is never written, so no subject PII reaches this table. It WAS payload-bearing before #276, when a cmd.tenant.update that failed to parse wrote an admin password hash here. Naming that history is deliberate: an out-of-scope entry that only says "no PII" invites restoring raw parking as a debugging convenience.' }, - { table: 'parked_cmd_events', column: 'reason', - reason: 'Fixed diagnostic enum (parse-failed / unknown-type-or-version), not personal data.' }, - // Repair-request line items (#88) — the report-derived snapshot columns. - // These are machine-copied off the published report card at add time, not - // typed by anyone on this table: defect prose the INSPECTOR wrote about the - // property, frozen so the shared list stays readable after the report - // changes. They are declared as a group because they are one question, and - // declared at all because `comment_snapshot` was on #88's list and the - // honest answer needs saying out loud: the report content these copy from - // carries NO manifest rule of its own, so this is not a decision inherited - // from a ruled source. It is the same call, made here for the first time. - // The subject's OWN lists never reach this reasoning — those rows are - // deleted whole by the `created_by_ref` rule above. - { table: 'repair_request_items', column: 'comment_snapshot', reason: 'frozen copy of the inspector-authored defect comment on the published report — professional content about the property, not prose about or by the data subject' }, - { table: 'repair_request_items', column: 'defect_title_snapshot', reason: 'frozen copy of the report defect title — inspector-authored content about the property' }, - { table: 'repair_request_items', column: 'location_snapshot', reason: 'frozen copy of the defect location WITHIN the property ("primary bathroom"), not a postal address' }, - { table: 'repair_request_items', column: 'category_snapshot', reason: 'frozen copy of the report defect category — tenant taxonomy value, not personal data' }, - { table: 'repair_request_items', column: 'trade_snapshot', reason: 'resolved trade label ("licensed roofer") snapshotted at add time — tenant taxonomy value, not personal data' }, - { table: 'repair_request_items', column: 'section_title', reason: 'frozen copy of the report section heading — template structure, not personal data' }, - { table: 'repair_request_items', column: 'item_label', reason: 'frozen copy of the report item label — template structure, not personal data' }, - // Sits inside the address family by name and outside it by substance: it - // records WHEN the geocode ran, not where the property is. Excluded rather - // than retained so the retain rules above stay a list of columns that - // actually hold the address. - { table: 'inspections', column: 'address_geocoded_at', reason: 'timestamp recording when the address was geocoded — a processing record, not the address itself' }, + { table: 'inspection_requests', column: 'property_address', category: 'user.address', action: 'retain', legalBasis: 'art_17_3_e', retention: 'P6Y', enforcementStatus: 'pending', enforcementDeadline: '2027-02-01' }, ]; diff --git a/server/lib/compliance/erasure-out-of-scope.ts b/server/lib/compliance/erasure-out-of-scope.ts new file mode 100644 index 000000000..598426483 --- /dev/null +++ b/server/lib/compliance/erasure-out-of-scope.ts @@ -0,0 +1,157 @@ +/** + * Track I-a GDPR (spec §5) — the erasure manifest's out-of-scope register. + * + * The companion to `ERASURE_MANIFEST` in `erasure-manifest.ts`: the columns the + * PII heuristic flags that erasure deliberately does NOT act on, each with the + * reason. Split out of the manifest when that file reached its anti-monolith + * line cap — the two arrays are read together and mean nothing apart, so the + * gate concatenates both sources before parsing either. Add a column here only + * with a reason that says why it cannot carry a data subject's data; an entry + * without one is a shrug that reads like a decision. + */ + +/** + * A PII-heuristic column the manifest DELIBERATELY does not act on. Every entry + * must say why — the reason is what a DSAR audit reads, and the CI gate + * (`scripts/check-erasure-manifest.mjs`) hard-fails an entry without one. + * + * @gateConsumed `scripts/check-erasure-manifest.mjs` reads this declaration out + * of the SOURCE TEXT (`arrayBody(src, 'ERASURE_OUT_OF_SCOPE')`) rather than + * importing it — the gate is a plain .mjs script and the manifest is TypeScript. + * That consumption is invisible to a module-graph analyzer, so knip would report + * both symbols as dead. The tag (knip `tags: ["-gateConsumed"]`) says "a tool + * consumes this", which is true; a dead-code baseline entry would have said + * "this is dead and we tolerate it", which is not. + */ +export interface ErasureOutOfScopeEntry { + table: string; + column: string; + reason: string; +} + +/** @gateConsumed read as source text by `scripts/check-erasure-manifest.mjs`. */ +export const ERASURE_OUT_OF_SCOPE: ErasureOutOfScopeEntry[] = [ + // Columns that ride with a row-delete rule above (per-column scan cannot + // see row semantics). + { table: 'contacts', column: 'phone', reason: 'rides with the contacts row delete (locator = email)' }, + + // Staff, not data subjects. Consumer-DSAR erasure never touches employee + // accounts; staff offboarding is a separate lifecycle. + { table: 'users', column: 'email', reason: 'staff account — not consumer-DSAR scope' }, + { table: 'users', column: 'phone', reason: 'staff account — not consumer-DSAR scope' }, + { table: 'users', column: 'default_signature_base64', reason: 'inspector (staff) signature asset' }, + { table: 'users', column: 'is_signature_enabled', reason: 'boolean flag, not personal data' }, + // An inspector's routing origin can be their home address, so it IS personal + // data — it is simply not a CONSUMER data subject's. Same posture as + // users.email/phone above: consumer-DSAR erasure never touches it and there + // is deliberately no DSAR-export path for it. Declared here so the decision + // is recorded rather than inferred from the PII heuristic not matching + // 'service_origin_address'. + { table: 'users', column: 'service_origin_address', reason: 'staff routing origin (may be a home address) — staff offboarding lifecycle, not consumer-DSAR scope' }, + { table: 'users', column: 'service_origin_lat', reason: 'staff routing origin coordinate — not consumer-DSAR scope' }, + { table: 'users', column: 'service_origin_lng', reason: 'staff routing origin coordinate — not consumer-DSAR scope' }, + { table: 'tenant_invites', column: 'email', reason: 'staff invite — not consumer-DSAR scope' }, + { table: 'audit_logs', column: 'ip_address', reason: 'staff-action security audit trail' }, + { table: 'report_signoff', column: 'signature_ref', reason: 'inspector (staff) signoff reference' }, + { table: 'agreement_requests', column: 'inspector_signature_base64', reason: 'inspector (staff) countersignature' }, + + // The controller's own business identity, not a data subject's. + { table: 'tenant_configs', column: 'support_email', reason: 'company-owned support address' }, + { table: 'tenant_configs', column: 'sender_email', reason: 'company-owned sending address' }, + { table: 'tenant_configs', column: 'company_phone', reason: 'company-owned phone' }, + { table: 'tenant_configs', column: 'company_lat', reason: 'company office coordinate — controller business identity' }, + { table: 'tenant_configs', column: 'company_lng', reason: 'company office coordinate — controller business identity' }, + // The address family's other half. `company_address` is a business's own + // published location — the controller's identity, not a data subject's — + // so it follows its `company_lat`/`company_lng` siblings above and NOT the + // `inspections` rules, where the address is somebody's home. The two look + // alike to a name-matching gate and are not the same question. + { table: 'tenant_configs', column: 'company_address', reason: 'company office address — controller business identity, published on reports and invoices' }, + + // Heuristic false positives — config values and references, not PII. + { table: 'tenant_configs', column: 'email_mode', reason: 'config enum, not personal data' }, + { table: 'tenant_configs', column: 'email_byo_provider', reason: 'config enum, not personal data' }, + { table: 'automations', column: 'recipient_kind', reason: 'config enum, not personal data' }, + { table: 'automations', column: 'recipient_role_profile_id', reason: 'role-profile reference, not personal data' }, + { table: 'automations', column: 'email_template_id', reason: 'template reference, not personal data' }, + { table: 'automation_logs', column: 'recipient_role_key', reason: 'role key, not personal data' }, + { table: 'automation_logs', column: 'recipient_contact_id', reason: 'opaque id on the retained evidence ledger (see the automation_logs.recipient retain rule)' }, + { table: 'contact_role_profiles', column: 'email_template_id', reason: 'template reference, not personal data' }, + { table: 'sms_consent_log', column: 'recipient_type', reason: 'role-kind enum, not personal data' }, + { table: 'report_versions', column: 'signature', reason: 'report-content integrity seal, not personal data' }, + // ── reports ─────────────────────────────────────────────────────────────── + // A report is findings about a named person's property. Only `title` is + // free text a human writes, and it routinely carries the address ("123 Oak + // St — Radon"). The rest of the row is ids, enums and a timestamp, declared + // out of scope below. + { table: 'reports', column: 'inspection_id', reason: 'opaque id; the inspection row carries its own rules' }, + { table: 'reports', column: 'tenant_id', reason: 'tenant scope key, not personal data' }, + { table: 'reports', column: 'id', reason: 'opaque primary key' }, + { table: 'reports', column: 'kind', reason: 'primary/ancillary enum, not personal data' }, + { table: 'reports', column: 'inspection_service_id', reason: 'billing-line reference, not personal data' }, + { table: 'reports', column: 'template_id', reason: 'template reference, not personal data' }, + { table: 'reports', column: 'status', reason: 'workflow enum, not personal data' }, + { table: 'reports', column: 'created_at', reason: 'record timestamp, not personal data' }, + { table: 'reports', column: 'published_at', reason: 'record timestamp, not personal data' }, + { table: 'reports', column: 'notified_at', reason: 'record timestamp, not personal data' }, + { table: 'reports', column: 'sort_order', reason: 'presentation ordering integer, not personal data' }, + // The tenant's own published Privacy / Terms. `body_snapshot` is the + // company's prose, not a data subject's data, and the row's whole purpose is + // to be immutable — erasing it would destroy the record of what a document + // said at a date, which is the one thing it exists to answer. Listed rather + // than left silent because the PII heuristic does not flag any column here, + // and silence is not the same as a decision. + // The payment ledger. The `note` column has its own anonymize rule above; + // everything that carries a figure or an actor reference is declared here + // rather than left silent, because the heuristic flags none of it and + // silence is not the same as a decision. + { table: 'order_payments', column: 'amount_cents', + reason: 'financial record retained under accounting/tax obligation; carries no subject identifier on its own' }, + { table: 'order_payments', column: 'recorded_by', + reason: 'staff user id (who keyed the payment) — not consumer-DSAR scope' }, + { table: 'order_payments', column: 'provider_ref', + reason: 'payment-processor reference on the retained financial row, not personal data' }, + { table: 'tenant_legal_versions', column: 'body_snapshot', reason: 'company-authored policy text, not personal data of any data subject' }, + { table: 'tenant_legal_versions', column: 'published_by_user_id', reason: 'staff author reference — not consumer-DSAR scope' }, + // Pay splits (#278). A split is a payroll record about a STAFF member, held + // under accounting and employment obligations. A client's erasure request + // never reaches it — the client is not the data subject here. Declared + // rather than left silent: the PII heuristic flags none of these columns, + // and silence is not the same as a decision. + { table: 'inspection_service_pay_splits', column: 'user_id', + reason: 'payroll record for a staff member, retained under accounting and employment obligations; not client data, so a client erasure request does not reach it' }, + { table: 'inspection_service_pay_splits', column: 'reason', + reason: 'free text a manager writes about a payout adjustment to a staff member — payroll audit trail, not consumer-DSAR scope' }, + { table: 'service_pay_rules', column: 'user_id', + reason: 'staff compensation rule — not consumer-DSAR scope' }, + // The portal->core dead-letter queue (#276). Registered although the PII + // heuristic flags neither column, because silence here is exactly how this + // one hid: `envelope` and `reason` look like nothing. + { table: 'parked_cmd_events', column: 'envelope', + reason: 'Fingerprint only (type/dataschema/id/seq/size/digest) — the command payload is never written, so no subject PII reaches this table. It WAS payload-bearing before #276, when a cmd.tenant.update that failed to parse wrote an admin password hash here. Naming that history is deliberate: an out-of-scope entry that only says "no PII" invites restoring raw parking as a debugging convenience.' }, + { table: 'parked_cmd_events', column: 'reason', + reason: 'Fixed diagnostic enum (parse-failed / unknown-type-or-version), not personal data.' }, + // Repair-request line items (#88) — the report-derived snapshot columns. + // These are machine-copied off the published report card at add time, not + // typed by anyone on this table: defect prose the INSPECTOR wrote about the + // property, frozen so the shared list stays readable after the report + // changes. They are declared as a group because they are one question, and + // declared at all because `comment_snapshot` was on #88's list and the + // honest answer needs saying out loud: the report content these copy from + // carries NO manifest rule of its own, so this is not a decision inherited + // from a ruled source. It is the same call, made here for the first time. + // The subject's OWN lists never reach this reasoning — those rows are + // deleted whole by the `created_by_ref` rule above. + { table: 'repair_request_items', column: 'comment_snapshot', reason: 'frozen copy of the inspector-authored defect comment on the published report — professional content about the property, not prose about or by the data subject' }, + { table: 'repair_request_items', column: 'defect_title_snapshot', reason: 'frozen copy of the report defect title — inspector-authored content about the property' }, + { table: 'repair_request_items', column: 'location_snapshot', reason: 'frozen copy of the defect location WITHIN the property ("primary bathroom"), not a postal address' }, + { table: 'repair_request_items', column: 'category_snapshot', reason: 'frozen copy of the report defect category — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'trade_snapshot', reason: 'resolved trade label ("licensed roofer") snapshotted at add time — tenant taxonomy value, not personal data' }, + { table: 'repair_request_items', column: 'section_title', reason: 'frozen copy of the report section heading — template structure, not personal data' }, + { table: 'repair_request_items', column: 'item_label', reason: 'frozen copy of the report item label — template structure, not personal data' }, + // Sits inside the address family by name and outside it by substance: it + // records WHEN the geocode ran, not where the property is. Excluded rather + // than retained so the retain rules above stay a list of columns that + // actually hold the address. + { table: 'inspections', column: 'address_geocoded_at', reason: 'timestamp recording when the address was geocoded — a processing record, not the address itself' }, +]; diff --git a/tests/unit/privacy/erasure-manifest-coverage.spec.ts b/tests/unit/privacy/erasure-manifest-coverage.spec.ts index 5235090a3..b48631b7c 100644 --- a/tests/unit/privacy/erasure-manifest-coverage.spec.ts +++ b/tests/unit/privacy/erasure-manifest-coverage.spec.ts @@ -17,10 +17,8 @@ import { describe, it, expect } from 'vitest'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { - ERASURE_MANIFEST, - ERASURE_OUT_OF_SCOPE, -} from '../../../server/lib/compliance/erasure-manifest'; +import { ERASURE_MANIFEST } from '../../../server/lib/compliance/erasure-manifest'; +import { ERASURE_OUT_OF_SCOPE } from '../../../server/lib/compliance/erasure-out-of-scope'; /** snake_case -> camelCase (single underscore groups; does not handle acronyms). */ function toCamelCase(snake: string): string { @@ -197,6 +195,21 @@ describe('the property address family', () => { ).toMatch(/^P\d+Y$/); }); + it.each(RETAINED_ADDRESS_COLUMNS)('%s does not read as implemented', (key) => { + // The rule is a recorded decision, not a shipped behaviour. Anything + // rendering the manifest — a DSAR console, an audit export — has to be + // able to tell those apart, and a `retain` that looks enforced while + // nothing expires it is exactly the unbounded retain this ruling + // refused. The gate enforces the same thing; this fails faster. + const [table, column] = key.split('.'); + const rule = ERASURE_MANIFEST.find((r) => r.table === table && r.column === column); + expect(rule!.enforcementStatus).toBe('pending'); + expect( + rule!.enforcementDeadline, + `${key} is pending with no deadline. Without a date, "pending" becomes permanent.`, + ).toMatch(/^\d{4}-\d{2}-\d{2}$/); + }); + // A TRIPWIRE, not a requirement. Nothing expires an inspection address // today, so the rules above record a decision no code acts on, and the // manifest says so where the rules are. The day the sweep learns about From 08c51152907f472816d067ecde098d0010021de2 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 11:56:07 +0800 Subject: [PATCH 07/10] fix(branding): refuse to turn in-report repair estimates on `updateBranding` spread whatever the request body carried into the config write, and `showEstimates` was one of those keys. Any owner or manager -- and the admin-scoped MCP surface -- could set it in one call, and the report renderer gates the per-defect price badge and the cost tables on exactly that boolean. Only the UI control was missing; the capability was not. Estimates are being redesigned as a separate deliverable rather than a section of the signed report, so the service now rejects `true` at the one entry point that writes tenant config. `false` still passes, so anything already enabled can be turned off. The OpenAPI/MCP description advertised an example of `true` for a value the server refuses; it now states the rule. A schema comment records that a future second writer of the column would bypass the refusal. Red-first: with the guard neutered the new case fails with the write reaching the DB path carrying `showEstimates: true`, so the test watches the real thing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H --- server/lib/db/schema/tenant/core.ts | 4 +++ server/lib/validations/admin/settings.ts | 3 +- server/services/branding.service.ts | 23 ++++++++++++--- .../unit/branding/cancellation-policy.spec.ts | 28 +++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/server/lib/db/schema/tenant/core.ts b/server/lib/db/schema/tenant/core.ts index 9e7278eed..226a2b863 100644 --- a/server/lib/db/schema/tenant/core.ts +++ b/server/lib/db/schema/tenant/core.ts @@ -102,6 +102,10 @@ export const tenantConfigs = sqliteTable('tenant_configs', { .$type<{ cloneDefault: 'rating' | 'rating_notes' | 'all'; autoAdvanceDelayMs: number; pinnedTagIds: string[]; agentRepairAccess?: 'off' | 'read' | 'readwrite'; reportLinkTtl?: ReportLinkTtl }>(), // Sprint 2 S2-4 — when true, published reports render the per-defect // "Estimated cost: $X – $Y" badge. + // ⚠️ This column cannot currently be set to true: `BrandingService.updateBranding` + // refuses an enable (422) and accepts only a disable. Estimates are being reshaped + // into a deliverable of their own instead of a section of the signed report, so any + // new writer of this column must carry the same refusal or it is bypassed silently. showEstimates: integer('is_estimates_shown', { mode: 'boolean' }).notNull().default(false), // Track E1 (ITB §11, UC-ITB-07) — when true, the published report sub-nav // exposes a "Repair List" tab. Default OFF — opt-in for realtors who want diff --git a/server/lib/validations/admin/settings.ts b/server/lib/validations/admin/settings.ts index 3c74d9213..89b025f62 100644 --- a/server/lib/validations/admin/settings.ts +++ b/server/lib/validations/admin/settings.ts @@ -47,7 +47,8 @@ export const UpdateBrandingSchema = z.object({ billingUrl: z.string().url('Invalid URL').or(z.literal('')).optional().openapi({ example: 'https://example.com/billing' }).describe('TODO describe billingUrl field for the OpenInspection MCP integration'), defaultProfileId: z.string().optional().openapi({ example: 'signature' }).describe('Default report appearance profile id (built-in: signature|meridian|terra)'), // Sprint 2 S2-4 — gate the per-defect "Estimated cost: $X – $Y" badge. - showEstimates: z.boolean().optional().openapi({ example: true }).describe('TODO describe showEstimates field for the OpenInspection MCP integration'), + // Currently a one-way switch; the refusal lives in BrandingService.updateBranding. + showEstimates: z.boolean().optional().openapi({ example: false }).describe('Per-defect "Estimated cost" badge on the published report. Cannot currently be enabled: sending true is rejected with 422 because embedded estimates are being redesigned as a separate deliverable rather than a section of the signed report. Sending false (turning the badge off) is always accepted.'), // Track E1 (ITB §11) — gate the "Repair List" tab on the published report. enableRepairList: z.boolean().optional().openapi({ example: true }).describe('TODO describe enableRepairList field for the OpenInspection MCP integration'), // Sprint 3 S3-2 — gate the customer-driven "Generate repair request" diff --git a/server/services/branding.service.ts b/server/services/branding.service.ts index 920cba3ad..145377ea8 100644 --- a/server/services/branding.service.ts +++ b/server/services/branding.service.ts @@ -239,12 +239,27 @@ export class BrandingService { /** * Updates the branding configuration for a tenant. * - * ⚠️ This is the gate for `cancellation_policy`. The refusal cannot be a Zod - * rule — it compares the submitted policy against DB state — so it lives - * here, in the writer, and a second writer of that column would bypass it - * without failing anything. See the column comment in the tenant schema. + * ⚠️ This is the gate for `cancellation_policy` and for `is_estimates_shown`. + * Neither refusal can be a Zod rule — one compares the submitted policy + * against DB state, the other must stay asymmetric (refuse `true`, accept + * `false`) — so both live here, in the writer, and a second writer of those + * columns would bypass them without failing anything. See the column + * comments in the tenant schema. */ async updateBranding(tenantId: string, data: Partial) { + // Estimates embedded in the signed report are switched off as a product + // decision, not because the rendering is broken: a cost figure printed + // inside the report reads as the inspector's own quote for the work. + // The capability returns as a SEPARATE deliverable, which is a redesign + // this boolean does not describe. Only `true` is refused — a tenant who + // already has estimates showing must always be able to turn them off. + if (data.showEstimates === true) { + throw Errors.UnprocessableEntity( + 'Repair estimates cannot be shown inside the inspection report. ' + + 'Estimates are being redesigned as a separate deliverable rather than a ' + + 'section of the signed report, so this setting can only be turned off.', + ); + } if (data.cancellationPolicy !== undefined && policyChargesFees(data.cancellationPolicy)) { if (!(await this.getCancellationAttestation(tenantId))) { throw Errors.UnprocessableEntity( diff --git a/tests/unit/branding/cancellation-policy.spec.ts b/tests/unit/branding/cancellation-policy.spec.ts index d0c416432..034e34a19 100644 --- a/tests/unit/branding/cancellation-policy.spec.ts +++ b/tests/unit/branding/cancellation-policy.spec.ts @@ -11,6 +11,10 @@ * The gate compares DB state, so it is not expressible in the Zod schema and * lives in `BrandingService.updateBranding`. These specs exercise that method, * which is what `POST /api/admin/branding` funnels into. + * + * The last block covers the OTHER refusal in that same writer — embedded report + * estimates — for the same reason: the writer is the single chokepoint every + * caller (dashboard, MCP, API client) goes through. */ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { eq } from 'drizzle-orm'; @@ -176,4 +180,28 @@ describe('BrandingService — cancellation policy attestation gate', () => { const foreign = await agreementSvc.createAgreement('other', 'Theirs', '

x

'); await expect(branding.attestCancellationClause(TENANT, foreign.id)).rejects.toThrow(/not found/i); }); + + // ─── Embedded repair estimates ─────────────────────────────────────────── + // The other refusal in the same writer. `showEstimates` stays open in the + // Zod schema (it is a real column with a real off-switch), so the service + // is the only place that can be asymmetric about which value it accepts. + + async function storedShowEstimates(): Promise { + const row = await testDb.select({ v: schema.tenantConfigs.showEstimates }) + .from(schema.tenantConfigs).where(eq(schema.tenantConfigs.tenantId, TENANT)).get(); + return row?.v ?? null; + } + + it('refuses to turn embedded report estimates on', async () => { + await expect(branding.updateBranding(TENANT, { showEstimates: true })) + .rejects.toThrow(/estimates cannot be shown inside the inspection report/i); + expect(await storedShowEstimates()).toBeNull(); + }); + + it('still lets a tenant turn embedded report estimates off', async () => { + // Asymmetric on purpose: a blanket refusal would strand any tenant the + // column is already true for, with no supported way back to false. + await branding.updateBranding(TENANT, { showEstimates: false }); + expect(await storedShowEstimates()).toBe(false); + }); }); From 98facdfabba9dcf14733dee3e53481e987415e9a Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 12:12:34 +0800 Subject: [PATCH 08/10] feat(ai): ask whether a capability is offered, not just whether a key exists `callGemini` resolved credentials, called the model and metered the call. Nothing asked whether the product offers this capability on this credential source. So the managed path was closed only because no deployment had provisioned a platform key -- starved rather than refused, and one `wrangler secret put` away from being open without anyone deciding to ship it. `lib/ai/capability-policy.ts` answers that question: a sync pure table over (capability, source). translate is refused on any source because it is unreleased; assist is refused on platform credentials. Behaviour is unchanged today, which is the point -- an operational accident becomes a stated refusal that survives the key being configured. The source comes from `resolveRuntimeAiSource` on the same credential literal `buildAiMeter` reads in di.ts, so the source the gate judges and the source a usage row records cannot be two different answers. No second resolver, no second counter. Prompts move to `lib/ai/prompts.ts` under stable version tokens, verbatim -- skeletons compared byte-for-byte after newline normalisation, including the two lines carrying trailing spaces. Nothing persists a token yet; that needs a table. Also fixes a real swallow: `suggestComment` wrapped the call in `catch { return [] }`, so a refusal reached the inspector as an empty popover. The catch now rethrows AI_NOT_CONFIGURED, matching why `assertModelConfigured` already sits outside that try. Red-first: with the gate neutered, four of six cases fail -- including the refusal returning `[]` instead of throwing. The two that stay green are the allowed path and its metering control. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H --- server/lib/ai/capability-policy.ts | 93 ++++++++++++++++++ server/lib/ai/prompts.ts | 114 +++++++++++++++++++++ server/lib/middleware/di.ts | 16 +-- server/services/ai.service.ts | 125 ++++++++++++------------ tests/unit/ai/capability-policy.spec.ts | 99 +++++++++++++++++++ 5 files changed, 374 insertions(+), 73 deletions(-) create mode 100644 server/lib/ai/capability-policy.ts create mode 100644 server/lib/ai/prompts.ts create mode 100644 tests/unit/ai/capability-policy.spec.ts diff --git a/server/lib/ai/capability-policy.ts b/server/lib/ai/capability-policy.ts new file mode 100644 index 000000000..fcd86abd3 --- /dev/null +++ b/server/lib/ai/capability-policy.ts @@ -0,0 +1,93 @@ +/** + * What this product OFFERS: whether a given AI capability may run at all on a + * given set of credentials. + * + * This is a different question from the two that already had answers. + * `resolve-provider.ts` decides WHICH key a call would run on; `metering.ts` + * counts what a call consumed. Neither one ever asks whether the capability is + * something the product currently ships on that key — so nothing did, and the + * answer was whatever the runtime happened to make possible. + * + * WHY THIS EXISTS EVEN THOUGH IT CHANGES NO BEHAVIOR TODAY. + * The managed path is dark right now for one reason: no deployment has + * provisioned a platform key. That is an operational fact, not a product + * decision, and it reverses the instant someone runs a single + * `wrangler secret put` — an action taken by whoever is provisioning + * infrastructure, who is not choosing what the product ships. A posture that + * holds only while a secret is missing is not a posture; it is an accident that + * has not been corrected yet. Written down here, the same answer survives the + * key being configured, and turning a capability on becomes an edit to this + * table that shows up in a diff. + * + * WHY IT IS COMPILED IN AND NOT AN ENVIRONMENT SWITCH. + * What the product offers is a release decision. An env flag would let one + * deployment quietly ship a capability that was never released anywhere else, + * and the source would no longer describe the product. Flipping any line below + * is a code change, reviewed like one. + * + * CURRENT POSTURE: + * - `assist` on the tenant's OWN key → offered. Unchanged; this is the + * only combination any caller reaches today. + * - `assist` on platform credentials → not offered. Report assistance + * runs on the tenant's own provider account, so the tenant picks the + * provider and owns that relationship directly rather than through us. + * - `translate`, on any credentials → not offered. The capability has a + * usage metric reserved for it and no released surface. A reserved slot + * must not double as an unlocked door. + * + * Pure and synchronous, like `resolveAi`: no I/O, the whole policy fits on one + * screen, and it is testable without a database. + */ +import type { AiUsageKind } from '../usage/period'; +import type { AiCredentialSource } from './resolve-provider'; + +/** Why a capability was refused. Machine-readable so a caller can tell + * "not shipped yet" apart from "not on these credentials" without matching + * on message text. */ +export type AiCapabilityDenialReason = + /** The capability itself has no released surface in this product. */ + | 'capability_not_released' + /** The capability ships, but not funded by these credentials. */ + | 'source_not_offered'; + +export type AiCapabilityDecision = + | { allowed: true } + | { + allowed: false; + reason: AiCapabilityDenialReason; + capability: AiUsageKind; + source: AiCredentialSource; + /** Phrased for the inspector who triggered the call, not for a log. */ + message: string; + }; + +export function checkAiCapability( + capability: AiUsageKind, + source: AiCredentialSource, +): AiCapabilityDecision { + // Capability first, credentials second. Translation is refused on the + // tenant's own key too — the gate is about what the product offers, and a + // tenant supplying their own key does not release a feature. + if (capability === 'translate') { + return { + allowed: false, + reason: 'capability_not_released', + capability, + source, + message: 'AI translation is not available in this product yet.', + }; + } + + if (source === 'managed') { + return { + allowed: false, + reason: 'source_not_offered', + capability, + source, + message: + 'AI assistance runs on your own provider key. Add one in Settings → Advanced → AI.', + }; + } + + return { allowed: true }; +} diff --git a/server/lib/ai/prompts.ts b/server/lib/ai/prompts.ts new file mode 100644 index 000000000..6e53b822d --- /dev/null +++ b/server/lib/ai/prompts.ts @@ -0,0 +1,114 @@ +/** + * The prompts every AI feature sends, addressable by a stable version token. + * + * They used to be inline template literals inside `AIService`, which made them + * unnameable: a prompt could be reworded in a routine edit and nothing — + * no test, no log line, no stored row — could tell an old output from a new + * one afterwards. A prompt is the largest single input to what the model + * returns, so it needs the same treatment as any other versioned input. + * + * Each entry carries a `version` that is a NAME, not a hash and not a + * timestamp: it is written down once and changed only when the wording + * changes. Bump the suffix (`.v1` → `.v2`) in the same commit that edits the + * text, never separately. Nothing persists these tokens yet; making them exist + * and be stable is the prerequisite for a future `prompt_version` on stored AI + * output, and that step is a schema change that does not belong here. + * + * The text below is a verbatim move. Rewording a prompt changes model output, + * and no test in this repository would notice. + */ + +/** Context the rewrite prompt renders above the comment being revised. */ +export interface RewriteCommentPromptArgs { + itemLabel: string; + sectionTitle: string; + tab: 'information' | 'limitations' | 'defects'; + originalComment: string; + instruction: string; + category?: 'safety' | 'recommendation' | 'maintenance'; + location?: string; +} + +/** Item context the suggestion prompt renders. Extra caller fields (e.g. the + * property address) are accepted and ignored — the prompt names what it uses. */ +export interface SuggestCommentPromptArgs { + itemName: string; + sectionName: string; + rating?: string; + yearBuilt?: number | null; + sqft?: number | null; +} + +/** + * The four prompts, keyed by feature. + * + * `render` owns ALL of the prompt, including the label/join formatting of the + * context blocks. Leaving that formatting at the call site would mean a + * version token that covers only part of the text it claims to name. + */ +export const AI_PROMPTS = { + professionalComment: { + version: 'professional-comment.v1', + // `context?: string | undefined`, not `context?: string`: under + // exactOptionalPropertyTypes the caller's optional parameter arrives as + // an explicit `undefined`, which a bare `?:` refuses. + render: (args: { text: string; context?: string | undefined }): string => + `You are a professional home inspector. Rewrite the following rough observation into a professional, clear, and objective report comment. +Keep it concise but informative. +Context: ${args.context || 'General inspection'} +Rough Note: "${args.text}" +Professional Comment:`, + }, + + inspectionSummary: { + version: 'inspection-summary.v1', + render: (args: { defects: string }): string => + `You are a professional home inspector. Analyze the following list of defects found during an inspection and provide a high-level summary (2-3 sentences) focusing on the most critical issues for the home buyer. +Defects: +${args.defects} +Summary:`, + }, + + rewriteComment: { + version: 'rewrite-comment.v1', + render: (args: RewriteCommentPromptArgs): string => { + const ctxLines = [ + `Item: "${args.itemLabel}"`, + `Section: "${args.sectionTitle}"`, + `Tab: ${args.tab}`, + args.tab === 'defects' && args.category ? `Defect category: ${args.category}` : null, + args.tab === 'defects' && args.location ? `Location: ${args.location}` : null, + ].filter(Boolean).join('\n'); + + return `You are a certified home inspector revising a single inspection report comment. +Context: +${ctxLines} + +Original comment: +"""${args.originalComment}""" + +Instruction from the inspector: +"""${args.instruction}""" + +Rewrite the comment to satisfy the instruction while keeping a professional, concise inspection-report tone. +Return only the rewritten comment text — no preamble, no quotes, no markdown.`; + }, + }, + + suggestComment: { + version: 'suggest-comment.v1', + render: (args: SuggestCommentPromptArgs): string => { + const context = [ + args.rating ? `Rating: ${args.rating}` : null, + args.yearBuilt ? `Year Built: ${args.yearBuilt}` : null, + args.sqft ? `Sq Ft: ${args.sqft}` : null, + ].filter(Boolean).join(', '); + + return `You are a certified home inspector writing a professional inspection report. +Item: "${args.itemName}" in section "${args.sectionName}"${context ? ` (${context})` : ''}. +Write exactly 3 short, professional inspection comments for this item. +Each comment must be 1-2 sentences, factual, and in standard inspection report style. +Return only a JSON array of 3 strings, no other text. Example: ["Comment 1.", "Comment 2.", "Comment 3."]`; + }, + }, +} as const; diff --git a/server/lib/middleware/di.ts b/server/lib/middleware/di.ts index 0f39b6377..e5c07ccc8 100644 --- a/server/lib/middleware/di.ts +++ b/server/lib/middleware/di.ts @@ -6,7 +6,7 @@ import { UnitService } from '../../services/unit.service'; import { UnitSwitchService } from '../../services/unit-switch.service'; import { ReportVersionService } from '../../services/report-version.service'; import { AIService } from '../../services/ai.service'; -import { buildAiMeter } from '../ai/metering'; +import { buildAiMeter, resolveRuntimeAiSource } from '../ai/metering'; import { AuthService } from '../../services/auth.service'; import { OutboxService } from '../../portal/outbox.service'; import { publishRow } from '../../portal/outbox.service'; @@ -159,7 +159,10 @@ export async function diMiddleware(c: Context, next: Next) { target.admin = new AdminService(c.env.DB, provider); } break; - case 'ai': + case 'ai': { + // ONE credential picture, read by both the meter and the capability + // gate — the source a usage row is tagged with can never disagree. + const aiCreds = { profile: c.var.profile, tenantKey: emailCfg.dbSecrets.geminiApiKey || null, managedKey: c.env.AI_MANAGED_API_KEY ?? null, model: c.env.AI_MODEL ?? '' }; target.ai = new AIService( c.env.DB, // The tenant's own bound key (Settings → Advanced → AI) — @@ -174,14 +177,11 @@ export async function diMiddleware(c: Context, next: Next) { // No default here on purpose: an unset AI_MODEL fails // closed at the service rather than picking a model. c.env.AI_MODEL ?? '', - buildAiMeter({ - db: c.env.DB, profile: c.var.profile, tenantId, - tenantKey: emailCfg.dbSecrets.geminiApiKey || null, - managedKey: c.env.AI_MANAGED_API_KEY ?? null, - model: c.env.AI_MODEL ?? '', - }), + buildAiMeter({ db: c.env.DB, tenantId, ...aiCreds }), + resolveRuntimeAiSource(aiCreds) ?? 'byo', // same resolver the meter tags with; null (off) → 'byo' ); break; + } case 'auth': // Outbox forwarding to portal is SaaS-only: buildOutbox // returns undefined when SYNC_QUEUE is absent (standalone) diff --git a/server/services/ai.service.ts b/server/services/ai.service.ts index 7c4267256..9c54ae43a 100644 --- a/server/services/ai.service.ts +++ b/server/services/ai.service.ts @@ -1,8 +1,15 @@ import { drizzle } from 'drizzle-orm/d1'; import { eq, and } from 'drizzle-orm'; import { inspections, inspectionResults } from '../lib/db/schema'; -import { Errors } from '../lib/errors'; +import { AppError, ErrorCode, Errors } from '../lib/errors'; import { GeminiProvider } from '../lib/ai/providers/gemini'; +import { checkAiCapability } from '../lib/ai/capability-policy'; +import { + AI_PROMPTS, + type RewriteCommentPromptArgs, + type SuggestCommentPromptArgs, +} from '../lib/ai/prompts'; +import type { AiCredentialSource } from '../lib/ai/resolve-provider'; import type { AiUsageKind } from '../lib/usage/period'; /** @@ -21,6 +28,19 @@ import type { AiUsageKind } from '../lib/usage/period'; * managed path contradicts; without the correction the next reader treats that * path as a regression and deletes it. * + * HAVING credentials is not the same as the product OFFERING the capability + * they would fund. `lib/ai/capability-policy.ts` holds that second answer and + * `callGemini` asks it on every call. Until it existed, the managed path was + * merely starved — no deployment had provisioned a platform key — and one + * `wrangler secret put` by whoever provisions infrastructure would have turned + * it on without anyone deciding to ship it. The gate changes nothing today and + * that is the point: an accident becomes a stated refusal that survives the + * key being configured. + * + * The PROMPTS live in `lib/ai/prompts.ts`, each under a stable version token, + * so the largest input to a model's output is nameable rather than an inline + * literal that can be reworded in passing. + * * Sprint 1 A-4: when running in `standalone` mode without a Gemini API key, * `suggestComment` returns dev-mock suggestions so local development can * exercise the UI flow end-to-end. Production deploys (`saas` mode or @@ -46,6 +66,13 @@ export class AIService { * counter at a route or a hook is how two numbers that have to agree * stop agreeing. Undefined when there is no tenant to attribute to. */ private meter?: { record(kind: AiUsageKind): Promise }, + /** Whose credentials a call from this service would run on, taken from + * `resolveRuntimeAiSource` — the SAME resolver that tags the meter, so + * the source the gate judges and the source the usage row records can + * never be two different answers. Never re-derived inside this class. + * Defaults to the tenant's own key, which is the only source that + * reaches the service today. */ + private credentialSource: AiCredentialSource = 'byo', ) {} private isDevMode(): boolean { @@ -88,6 +115,24 @@ export class AIService { * pre-check are still covered. */ private async callGemini(prompt: string, kind: AiUsageKind = 'assist') { + // The capability gate, placed AFTER credential resolution (the source + // was resolved upstream and handed to the constructor) and BEFORE any + // content leaves the process. Every AI feature funnels through here, so + // this one call covers all of them — the same reason the meter lives at + // this method and nowhere else. + // + // Refusal is an explicit throw, never a silent skip or an empty string: + // a capability the product does not offer must read as a refusal to the + // caller, not as the model having nothing to say. + // + // It reuses AINotConfigured because "this call cannot run" already has + // exactly one shape in this codebase — `resolveAi` returning null uses + // it for the feature-off case, and every client already routes that to + // "set up AI". A second 4xx/5xx shape here would mean two failure paths + // for one situation. + const decision = checkAiCapability(kind, this.credentialSource); + if (!decision.allowed) throw Errors.AINotConfigured(decision.message); + const provider = new GeminiProvider({ apiKey: this.apiKey, model: this.model }); const { text } = await provider.complete({ prompt }); // Meter AFTER success, never before — a model call that failed must not @@ -102,13 +147,7 @@ export class AIService { * Rewrites a rough note into a professional report comment. */ async generateProfessionalComment(text: string, context?: string) { - const prompt = `You are a professional home inspector. Rewrite the following rough observation into a professional, clear, and objective report comment. -Keep it concise but informative. -Context: ${context || 'General inspection'} -Rough Note: "${text}" -Professional Comment:`; - - return this.callGemini(prompt); + return this.callGemini(AI_PROMPTS.professionalComment.render({ text, context })); } /** @@ -135,12 +174,7 @@ Professional Comment:`; if (!defects) return 'No significant defects observed during this inspection.'; - const prompt = `You are a professional home inspector. Analyze the following list of defects found during an inspection and provide a high-level summary (2-3 sentences) focusing on the most critical issues for the home buyer. -Defects: -${defects} -Summary:`; - - return this.callGemini(prompt); + return this.callGemini(AI_PROMPTS.inspectionSummary.render({ defects })); } /** @@ -153,15 +187,7 @@ Summary:`; * failure, throws so the UI can show an error toast (no silent * overwrite of the inspector's existing text). */ - async rewriteComment(input: { - itemLabel: string; - sectionTitle: string; - tab: 'information' | 'limitations' | 'defects'; - originalComment: string; - instruction: string; - category?: 'safety' | 'recommendation' | 'maintenance'; - location?: string; - }): Promise { + async rewriteComment(input: RewriteCommentPromptArgs): Promise { if (!this.hasApiKey()) { // Sprint 1 A-4: dev-mock instead of throwing in standalone mode. if (this.isDevMode()) { @@ -173,28 +199,7 @@ Summary:`; } this.assertModelConfigured(); - const ctxLines = [ - `Item: "${input.itemLabel}"`, - `Section: "${input.sectionTitle}"`, - `Tab: ${input.tab}`, - input.tab === 'defects' && input.category ? `Defect category: ${input.category}` : null, - input.tab === 'defects' && input.location ? `Location: ${input.location}` : null, - ].filter(Boolean).join('\n'); - - const prompt = `You are a certified home inspector revising a single inspection report comment. -Context: -${ctxLines} - -Original comment: -"""${input.originalComment}""" - -Instruction from the inspector: -"""${input.instruction}""" - -Rewrite the comment to satisfy the instruction while keeping a professional, concise inspection-report tone. -Return only the rewritten comment text — no preamble, no quotes, no markdown.`; - - const text = await this.callGemini(prompt); + const text = await this.callGemini(AI_PROMPTS.rewriteComment.render(input)); // Strip wrapping quotes / markdown the model sometimes adds. return text.replace(/^["'`]+|["'`]+$/g, '').trim(); } @@ -205,13 +210,8 @@ Return only the rewritten comment text — no preamble, no quotes, no markdown.` * UI can surface a clear "set up your API key" message instead of a silent * empty popover. Runtime Gemini failures still degrade to an empty array. */ - async suggestComment(params: { - itemName: string; - sectionName: string; - rating?: string; + async suggestComment(params: SuggestCommentPromptArgs & { propertyAddress?: string; - yearBuilt?: number | null; - sqft?: number | null; }): Promise { if (!this.hasApiKey()) { // Sprint 1 A-4: dev-mode mock so local development can exercise @@ -233,24 +233,19 @@ Return only the rewritten comment text — no preamble, no quotes, no markdown.` // must not disappear into "no suggestions today". this.assertModelConfigured(); - const context = [ - params.rating ? `Rating: ${params.rating}` : null, - params.yearBuilt ? `Year Built: ${params.yearBuilt}` : null, - params.sqft ? `Sq Ft: ${params.sqft}` : null, - ].filter(Boolean).join(', '); - - const prompt = `You are a certified home inspector writing a professional inspection report. -Item: "${params.itemName}" in section "${params.sectionName}"${context ? ` (${context})` : ''}. -Write exactly 3 short, professional inspection comments for this item. -Each comment must be 1-2 sentences, factual, and in standard inspection report style. -Return only a JSON array of 3 strings, no other text. Example: ["Comment 1.", "Comment 2.", "Comment 3."]`; - try { - const text = await this.callGemini(prompt); + const text = await this.callGemini(AI_PROMPTS.suggestComment.render(params)); const match = text.match(/\[[\s\S]*\]/); if (!match) return []; return JSON.parse(match[0]) as string[]; - } catch { + } catch (err) { + // Same rule as `assertModelConfigured` above, applied to the one + // refusal that can only be raised from INSIDE this try: the + // capability gate in `callGemini`. A runtime model failure degrades + // to "no suggestions"; a capability the product does not offer must + // reach the inspector as a refusal, not as an empty popover that + // looks like the model had nothing to say. + if (err instanceof AppError && err.code === ErrorCode.AI_NOT_CONFIGURED) throw err; return []; } } diff --git a/tests/unit/ai/capability-policy.spec.ts b/tests/unit/ai/capability-policy.spec.ts new file mode 100644 index 000000000..a7e538355 --- /dev/null +++ b/tests/unit/ai/capability-policy.spec.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + checkAiCapability, + type AiCapabilityDecision, + type AiCapabilityDenialReason, +} from '../../../server/lib/ai/capability-policy'; +import { AIService } from '../../../server/services/ai.service'; + +/** + * The capability gate: whether the product OFFERS a given AI capability on a + * given set of credentials, asked at the one chokepoint every AI feature runs + * through. + * + * The gate is behavior-neutral today — no deployment has a platform key, so no + * call resolves to 'managed'. That is exactly why it needs tests: nothing in a + * running system would notice if it stopped working, and the day it matters is + * the day someone provisions the key. + */ +/** Narrow to the denial arm, so a case reads `reason` without a cast and a + * decision that unexpectedly ALLOWS fails here rather than silently comparing + * `undefined` against the expected reason. */ +function denialReason(d: AiCapabilityDecision): AiCapabilityDenialReason { + if (d.allowed) throw new Error('expected a denial, got an allow'); + return d.reason; +} + +describe('AI capability policy', () => { + it('offers assist on the tenant OWN key', () => { + expect(checkAiCapability('assist', 'byo')).toEqual({ allowed: true }); + }); + + it('does NOT offer assist on platform credentials', () => { + const d = checkAiCapability('assist', 'managed'); + expect(d).toMatchObject({ allowed: false, capability: 'assist', source: 'managed' }); + expect(denialReason(d)).toBe('source_not_offered'); + }); + + it('does NOT offer translate on ANY credentials — including the tenant own key', () => { + // Asserted on BYO first: if this only covered 'managed' it would pass + // against a policy that refuses nothing but managed, which is a + // different rule with the same green. + const byo = checkAiCapability('translate', 'byo'); + expect(byo).toMatchObject({ allowed: false, source: 'byo' }); + expect(denialReason(byo)).toBe('capability_not_released'); + expect(denialReason(checkAiCapability('translate', 'managed'))).toBe('capability_not_released'); + }); +}); + +describe('the gate at the AI chokepoint', () => { + const fetchMock = vi.fn(); + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; + fetchMock.mockReset(); + // Every case below arms a SUCCESSFUL model response, so nothing passes + // because the call would have failed anyway. + fetchMock.mockResolvedValue({ + ok: true, json: async () => ({ candidates: [{ content: { parts: [{ text: 'x' }] } }] }), + } as Response); + }); + afterEach(() => { globalThis.fetch = originalFetch; }); + + function service(source: 'byo' | 'managed', record: () => Promise) { + return new AIService({} as D1Database, 'a-key', 'saas', 'a-model', { record }, source); + } + + it('a refused call records NO usage and sends nothing', async () => { + // The tenant consumed nothing, so there is nothing to meter. The gate + // sits ahead of the provider call and the meter sits behind it, which + // is what makes that true without a second branch. + const record = vi.fn(async () => {}); + await expect(service('managed', record).generateProfessionalComment('note')) + .rejects.toMatchObject({ status: 503, code: 'ai_not_configured' }); + expect(record).not.toHaveBeenCalled(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('the SAME call on the tenant own key still runs and still meters', async () => { + // The control. Without it, "record was not called" proves nothing — + // a meter that never fires at all would satisfy the case above. + const record = vi.fn(async () => {}); + await expect(service('byo', record).generateProfessionalComment('note')).resolves.toBe('x'); + expect(record).toHaveBeenCalledTimes(1); + expect(record).toHaveBeenCalledWith('assist'); + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it('a refusal reaches suggestComment callers instead of degrading to no suggestions', async () => { + // suggestComment swallows RUNTIME failures into an empty list. A + // capability the product does not offer must not arrive looking like + // the model had nothing to say. + const record = vi.fn(async () => {}); + await expect(service('managed', record).suggestComment({ itemName: 'Roof', sectionName: 'Roof' })) + .rejects.toMatchObject({ code: 'ai_not_configured' }); + expect(record).not.toHaveBeenCalled(); + }); +}); From 163e9f2707856442b77a3f4dbb2a18191d0e75c6 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 12:27:43 +0800 Subject: [PATCH 09/10] fix(reports): stop rendering in-report repair estimates The service-layer refusal in 08c51152 closed the write. It did not close the read: a tenant whose `is_estimates_shown` was already 1 kept getting per-defect price badges, the repair panel prices and the cost tables -- platform-seeded median contractor pricing, published under their licence number. One production tenant was in exactly that state. `getReportData` no longer reads the column at all. The projection and the assignment are gone and the local stays at its `false` initialiser, so all three surfaces go dark from one place. The column itself survives: still writable to false, still carried by the branding endpoint, which is the surface that has to stay auditable. Components are untouched -- the estimate work returns as a separate deliverable and will reuse them. Verified there is no second read path, including implicit ones: five unprojected `select()`s over tenantConfigs were enumerated and their consumers checked, because a query with no projection carries a column without ever naming it. The two candidates that looked independent are derived -- the analytics service reads getReportData's result, and the delivery route's field is an OpenAPI response declaration, not a read. The e2e case that asserted this field was writable is inverted to assert the 422, with E-03 (turning it off) left as the control -- otherwise a route that rejected every branding write would satisfy it too. Red-first: with the pin removed the unit case fails `expected true to be false`, and it asserts the stored value is genuinely true first, so it cannot pass by there being nothing to pin. The first draft carried a 19-line comment and tripped the file-size ratchet at 974 > 952. The gate was right: the explanation was longer than the change. Trimmed, and the now-dead projection and assignment removed rather than left looking like a live read. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H --- .../inspection/inspection-report.service.ts | 10 ++++---- tests/e2e/estimate-range.spec.ts | 25 +++++++++++-------- tests/unit/inspections/estimate-range.spec.ts | 20 +++++++++++++-- tests/unit/repair/repair-list.service.spec.ts | 11 +++++--- 4 files changed, 45 insertions(+), 21 deletions(-) diff --git a/server/services/inspection/inspection-report.service.ts b/server/services/inspection/inspection-report.service.ts index 665a9aad0..3675d4925 100644 --- a/server/services/inspection/inspection-report.service.ts +++ b/server/services/inspection/inspection-report.service.ts @@ -512,9 +512,11 @@ export class InspectionReportService extends InspectionSubService { inspectorLicense = primaryLicenseOf(credentialSnapshot); } - // Sprint 2 S2-4 — per-tenant flag controls whether the published - // report renders "Estimated cost: $X – $Y" badges on defect cards. - let showEstimates = false; + // OFF for every tenant, unconditionally: costs are being rebuilt as a + // standalone deliverable rather than a section of the signed report, so + // NOTHING assigns this from tenantConfigs.showEstimates — that column + // survives for audit and for turning off, never for rendering. + const showEstimates = false; // Report Style Presets — tenant's default appearance profile id (resolved below). let tenantDefaultProfileId: string | null = null; // Per-tenant report-feature flags surfaced to the published report so the @@ -532,7 +534,6 @@ export class InspectionReportService extends InspectionSubService { let reportTimeZone = 'UTC'; try { const cfg = await db.select({ - showEstimates: tenantConfigs.showEstimates, defaultProfileId: tenantConfigs.defaultProfileId, enableRepairList: tenantConfigs.enableRepairList, enableCustomerRepairExport: tenantConfigs.enableCustomerRepairExport, @@ -545,7 +546,6 @@ export class InspectionReportService extends InspectionSubService { .where(eq(tenantConfigs.tenantId, tenantId)) .get(); if (cfg) { - showEstimates = Boolean(cfg.showEstimates); enableRepairList = Boolean(cfg.enableRepairList); enableCustomerRepairExport = Boolean(cfg.enableCustomerRepairExport); reserveScheduleEnabled = Boolean(cfg.reserveScheduleEnabled); diff --git a/tests/e2e/estimate-range.spec.ts b/tests/e2e/estimate-range.spec.ts index bce1a2747..b9224903c 100644 --- a/tests/e2e/estimate-range.spec.ts +++ b/tests/e2e/estimate-range.spec.ts @@ -4,8 +4,8 @@ * Drives the JSON API directly (no browser) against the local dev worker * (http://127.0.0.1:8789) to confirm: * - * - PATCH /api/admin/branding accepts the new `showEstimates` boolean - * field; subsequent GET reflects the persisted value. + * - POST /api/admin/branding refuses to turn `showEstimates` on and still + * accepts turning it off. * - The recommendation enum is exposed (sanity check shared with S2-3). * * The inspection-results JSON sanitizer (sanitizeDefectStates) is covered @@ -50,14 +50,19 @@ test.describe.serial('Sprint 2 S2-4 — Repair estimate range', () => { adminToken = await loginApi(request, ADMIN_EMAIL, ADMIN_PASSWORD); }); - test('E-01: POST /api/admin/branding persists showEstimates=true', async ({ request }) => { + test('E-01: POST /api/admin/branding REFUSES showEstimates=true', async ({ request }) => { + // Inverted deliberately. This test used to assert 200 and persistence, + // from when the field was writable. Estimates are being redesigned as a + // separate deliverable rather than a section of the signed report, so + // the service now refuses to turn the in-report form on. Turning it off + // stays permitted — see E-03, which is the control that keeps this + // assertion honest: without it, a route that rejected EVERY branding + // write would satisfy E-01 just as well. const res = await request.post(`${BASE_URL}/api/admin/branding`, { headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${adminToken}` }, data: { showEstimates: true }, }); - expect(res.status(), await res.text()).toBe(200); - const body = await res.json(); - expect(body.success).toBe(true); + expect(res.status(), await res.text()).toBe(422); }); test('E-02: subsequent GET /api/admin/branding round-trips showEstimates', async ({ request }) => { @@ -66,10 +71,10 @@ test.describe.serial('Sprint 2 S2-4 — Repair estimate range', () => { }); expect(res.status()).toBe(200); const body = await res.json(); - // The branding response intentionally omits showEstimates from its - // typed shape (it only powers the report renderer); we verify - // persistence indirectly by toggling it back off + asserting the - // next round-trip is a clean 200. + // The typed branding response omits showEstimates; the raw column does + // travel in the payload, and that is deliberate — the setting must stay + // readable so it can be audited and turned off. What must not happen is + // a successful write of `true`, which E-01 covers. expect(body.success).toBe(true); }); diff --git a/tests/unit/inspections/estimate-range.spec.ts b/tests/unit/inspections/estimate-range.spec.ts index fb4b13933..656957d37 100644 --- a/tests/unit/inspections/estimate-range.spec.ts +++ b/tests/unit/inspections/estimate-range.spec.ts @@ -218,7 +218,10 @@ describe('Sprint 2 S2-4 — repair estimate range', () => { expect(report.coverPhotoUrl).toBe(`https://cdn.example/${COVER_KEY}`); }); - it('getReportData reflects tenant_configs.show_estimates=true', async () => { + it('getReportData forces showEstimates=false even when tenant_configs.show_estimates=1', async () => { + // A tenant that opted in before embedded estimates were withdrawn. The + // stored intent is deliberately left intact (the column is still there + // and still readable); what must not happen is the report rendering it. await testDb.insert(schema.tenantConfigs).values({ tenantId: TENANT, showEstimates: true, @@ -227,7 +230,20 @@ describe('Sprint 2 S2-4 — repair estimate range', () => { await svc.updateResults(INSPECTION_ID, TENANT, { 'roof-shingles': { rating: 'Satisfactory' }, }); + const report = await svc.getReportData(INSPECTION_ID, TENANT); - expect(report.showEstimates).toBe(true); + expect(report.showEstimates).toBe(false); + + // The two numbers that must disagree: what the tenant asked for vs what + // the renderer is handed. Printing both makes the guard's job visible — + // if the stored value ever reads false the assertion above would pass + // for the wrong reason (nothing was pinned; there was nothing to pin). + const stored = await testDb + .select({ v: schema.tenantConfigs.showEstimates }) + .from(schema.tenantConfigs) + .where(eq(schema.tenantConfigs.tenantId, TENANT)) + .get(); + expect(stored?.v).toBe(true); + expect(report.showEstimates).not.toBe(stored?.v); }); }); diff --git a/tests/unit/repair/repair-list.service.spec.ts b/tests/unit/repair/repair-list.service.spec.ts index d239cb013..7f83bce50 100644 --- a/tests/unit/repair/repair-list.service.spec.ts +++ b/tests/unit/repair/repair-list.service.spec.ts @@ -234,15 +234,18 @@ describe('Track E1 — InspectionService.getRepairList', () => { expect(result.defects[0]!.trade).toBeNull(); }); - it('reflects showEstimates from tenant_configs', async () => { - // Default = false (no row). + it('reports showEstimates=false regardless of tenant_configs', async () => { + // The repair list does not read tenant_configs itself — it forwards + // getReportData's `showEstimates`, so it inherits the render-side pin + // that keeps embedded estimates out of the report while they are + // redesigned as a standalone deliverable. let result = await svc.getRepairList(INSPECTION_ID, TENANT); expect(result.showEstimates).toBe(false); - // Insert config with show_estimates = 1. + // Insert config with show_estimates = 1 — still false downstream. await testDb.insert(schema.tenantConfigs).values({ tenantId: TENANT, showEstimates: true, updatedAt: new Date(), }); result = await svc.getRepairList(INSPECTION_ID, TENANT); - expect(result.showEstimates).toBe(true); + expect(result.showEstimates).toBe(false); }); }); From 79b2c0fa79933e0ce8dedc12b566de8be2446d7e Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 7 Aug 2026 13:04:00 +0800 Subject: [PATCH 10/10] docs(erasure): correct the reports.title rationale, and keep the old one The justification recorded for anonymising `reports.title` was wrong about this codebase in both halves. It said the column is "the one free-text column a human writes" and that it "routinely carries the address". Neither holds: the value is either the literal 'Inspection Report' or a snapshot of a service line's name from the tenant's own catalogue, no route can edit it, and the only other writer is the erasure executor running this rule. The rule does not move. Action stays `anonymize`, basis and period unchanged -- a catalogue service name is tenant-authored, so it cannot be assumed free of identifiers, and anonymising a title costs nothing. What changes is the reason on file, which is the part an Art. 5(2) accountability record is made of. The previous wording is kept in an amendment history block with the correction date, the evidence, and an explicit statement that the processing decision did not change. Deleting the record of a mistake is worth less than showing it was found and fixed. No test accompanies this: the change is the recorded reasoning, not behaviour. The erasure gate passes unchanged at 44 rules. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H --- server/lib/compliance/erasure-manifest.ts | 38 ++++++++++++++++++----- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/server/lib/compliance/erasure-manifest.ts b/server/lib/compliance/erasure-manifest.ts index b855eeb1a..fed4fe6da 100644 --- a/server/lib/compliance/erasure-manifest.ts +++ b/server/lib/compliance/erasure-manifest.ts @@ -11,8 +11,9 @@ * * ⚠️ HOW MUCH WEIGHT THIS FILE'S PROSE CAN CARRY. On 2026-08-07 two separate * justifications in here were checked against the code and found FALSE: the - * `reports.title` rule describes a column "a human writes" that is in fact - * machine-written with no API that can edit it, and `repair_requests. + * `reports.title` rule claimed a column "a human writes" that is in fact + * system-written with no API that can edit it (corrected below, with an + * amendment history rather than a silent overwrite), and `repair_requests. * created_by_ref` was documented as an opaque id while the code stores an email * address in it — a compliance classification resting on a comment that had been * wrong for as long as the feature existed. Both rules survived review because @@ -190,11 +191,34 @@ export const ERASURE_MANIFEST: ErasureRule[] = [ { table: 'esign_audit_logs', column: 'signature', category: 'system.integrity', action: 'retain', legalBasis: 'art_17_3_e' }, // ── reports ─────────────────────────────────────────────────────────────── - // A report is findings about a named person's property, and `title` is the - // one free-text column a human writes — it routinely carries the address - // ("123 Oak St — Radon"). Anonymised rather than deleted: the row is the - // spine of a signed, delivered document, and removing it would strand the - // version chain that proves what was delivered. + // A report is findings about a named person's property. `title` is written + // by the system, never by a person composing free text about this client: + // it is either the literal 'Inspection Report' (`inspection/reports.ts`) or + // a snapshot of a service line's name taken from the tenant's own catalogue + // (`inspection/report-generation.ts`, both the insert and the adoption + // update). No route writes it — the only other writer is the erasure + // executor performing this very rule. + // + // Anonymised rather than deleted: the row is the spine of a signed, + // delivered document, and removing it would strand the version chain that + // proves what was delivered. A catalogue service name is tenant-authored, + // so it cannot be assumed free of identifiers, and anonymising a title + // costs nothing. + // + // AMENDMENT HISTORY + // Previous rationale: "`title` is the one free-text column a human writes + // — it routinely carries the address ("123 Oak St — Radon")." + // Correction date: 2026-08-07 + // Why: factually wrong about this codebase, in both halves. + // No human writes it and no API can edit it, so it cannot routinely + // carry a per-property address. Evidence: the two writers named above, + // read 2026-08-07 (E2 — verified in source, not inferred from a plan). + // Impact: NONE on the processing decision. The action stays + // `anonymize`, the basis and the period are unchanged. What changes is + // the reason recorded for it. + // Kept rather than overwritten: an accountability record under Art. 5(2) + // that quietly deletes a mistake is worth less than one that shows the + // mistake was found and corrected. { table: 'reports', column: 'title', category: 'user.address', action: 'anonymize', legalBasis: 'art_17_3_e', retention: 'P6Y' }, // ── audit_logs (#276) ─────────────────────────────────────────────────────