Close the in-report estimate path, gate AI capabilities, and declare the address family for erasure - #301
Merged
important-new merged 11 commits intoAug 7, 2026
Conversation
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.
Closes the remaining piece of erasure-freetext Task 3 — the plan reduced it to a script header, and the document itself landed in 225d33e. 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/.
The last four columns portal InspectorHub#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.
…d the money
`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.
…ristic 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.
…a list
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.
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
… 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
The service-layer refusal in 08c5115 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
Both branches sit on the same base and touch disjoint files -- the containment/AI work is in branding, ai and the report service; the erasure work is in the compliance manifest and its gate. Merged so the full suite runs once over the whole batch, and so lint:erasure sees the manifest changes for the first time in a real full run: it is not part of the pre-commit set, only of CI and the full lint.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent pieces of work that happen to share a release.
1. In-report repair estimates are closed, on both sides
tenant_configs.is_estimates_showngated per-defect price badges, the repairpanel prices and the cost tables in the published report. The values come from
platform-seeded pricing, so a tenant who never edited them published our figures
under their own licence number. There was no UI control for the flag, but the
capability was reachable:
POST /api/admin/brandingaccepted the field from anyowner or manager and the service spread the request body into the config write
with no whitelist.
Estimates are being rebuilt as a standalone deliverable rather than a section of
the signed report, so:
updateBrandingrefuses to set the flag totrue, using the sameUnprocessableEntityshape as the cancellation-clause guard beside it.falsestill passes, so anything already enabled can be turned off.getReportDatano longer reads the column at all, so all threesurfaces go dark regardless of the stored value. The column survives: still
writable to false, still carried by the branding endpoint, so the per-tenant
setting stays visible and reversible.
Verified there is no second read path, including implicit ones: unprojected
select()s overtenantConfigswere 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, not separate reads.
The e2e case that asserted the field was writable is inverted to assert the 422,
with the turn-it-off case left as the control -- otherwise a route that rejected
every branding write would satisfy it too.
2. An AI capability gate at the one chokepoint
callGeminiresolved credentials, called the model and metered the call. Nothingasked whether the product offers this capability on this credential source, so a
platform-key path was closed only by nobody having provisioned a key -- starved
rather than refused.
lib/ai/capability-policy.tsis a sync, pure table over(capability, source),consulted before any content leaves the process. Refusal is a typed throw, never
a silent skip. Behaviour is unchanged today, which is the point: an operational
accident becomes a stated refusal that survives a key being configured later.
The source comes from
resolveRuntimeAiSourceon the same credential literalbuildAiMeterreads, so the source the gate judges and the source a usage rowrecords cannot disagree. No second resolver, no second counter.
Prompts move to
lib/ai/prompts.tsunder stable version tokens, verbatim --skeletons compared byte-for-byte after newline normalisation, including two lines
that carry trailing spaces.
Also fixes a real swallow found on the way:
suggestCommentwrapped the call incatch { return [] }, so a refusal reached the user as an empty popover. Thecatch now rethrows
AI_NOT_CONFIGURED.3. The erasure manifest can now see a property address
inspectionshad no erasure rule of any kind whileinspections.property_addressholds, for a residential inspection, the client's home address -- and the
manifest's PII heuristic had no
addressterm, so the gap was invisible to thegate.
inspectionsaddress columns andinspection_requests.property_addresscarry
retainrules with a stated basis and a bounded period taken from thetenant's existing retention setting rather than a second number. One entry per
column, no wildcard: a wildcard hides what was actually considered.
retainrule that declares a retention period but noenforcementStatusnowfails the gate. "Unenforced" is never what happens when nobody says
anything.
PENDING_ENFORCEMENTis a checked-in list validated both ways -- a pending rulemissing from it fails, and a stale entry whose rule is gone fails too, so the
list cannot decay into a blanket permit.
ERASURE_OUT_OF_SCOPEmoves to its own module (the manifest was at 389 of 400lines). The gate concatenates both sources before parsing either, so splitting
the register cannot halve what it sees.
Two justifications in the manifest were checked against the code and found false:
reports.titledescribed a column "a human writes" that is system-written with noAPI able to edit it, and
repair_requests.created_by_refwas documented as anopaque id while the code stores an email address in it. The rules are unchanged
and correct; the reasoning was not.
reports.titleis corrected with an amendmenthistory rather than a silent overwrite, and the file header now warns that a
premise stated in a comment is not evidence.
Also
Two CodeQL findings on the QuickBooks OAuth spec (
js/incomplete-url-substring-sanitization)fixed by matching the token endpoint on hostname rather than substring, and the
refund path reaches QuickBooks keyed on the ledger row that moved the money.
Verification
Full suite serially, no pipes:
lintexit 0 (including the erasure gate at 44rules / 57 out-of-scope declarations),
test:unit5066 passed,test:web2182passed. Every behavioural change was proven red first by removing the fix and
observing the specific failure.
🤖 Generated with Claude Code