Payments, scheduling and retention - #299
Merged
important-new merged 78 commits intoAug 6, 2026
Merged
Conversation
Probes the primitive the cap's concurrency guarantee rests on, before any code relies on it: a conditional upsert whose WHERE holds a correlated subquery over another table. Settles three things: - `excluded.tenant_id` IS in scope inside `DO UPDATE ... WHERE`, and resolves to the calling tenant (a sibling tenant's rows do not cap it). No bound-parameter fallback is needed. - `changes === 0` is reported when that subquery is false. - The plain `INSERT ... VALUES` form leaves the INSERT branch UNGATED: a conflict-free insert never reaches DO UPDATE, so a tenant already over the cap but with no counter row yet gets a free pass. The `INSERT ... SELECT ... WHERE` form gates both branches with the same predicate and is what the guard will use.
The free-tier cap read a monotonic counter, so a tenant who deleted an inspection never got the allowance back. Found in production 2026-08-05: one tenant had 1 inspection and 4 of 5 consumed; another was capped at 5 with 3 rows. Five counters were corrected by hand that day — this makes that correction unnecessary rather than repeatable. The gate now counts inspection rows. `usage_counters.value` for this metric degrades from source-of-truth to a self-healing display cache. Still one statement, so `meta.changes === 0` stays the authoritative "at cap" answer with no read-then-write window inside the guard. Two things the probe (previous commit) forced: - `INSERT ... SELECT ... WHERE` rather than VALUES, so the INSERT branch is gated too. A conflict-free INSERT never reaches DO UPDATE, so the VALUES form waves through a tenant who has rows but no counter row. - `consumeInspection(tenantId, count)`. Because the gate counts rows the caller inserts only afterwards, N looped calls all read the same count and all pass — a 3-sub inspection request took a tenant from 3 inspections to 6, deterministically. InspectionRequestService.create now consumes the batch as one unit; it is admitted whole or not at all. What counting rows gives up, unavoidably: two creates that overlap before either row lands both pass. The cap becomes a steady-state invariant rather than a serialized claim — the overshoot is bounded by in-flight concurrency and self-corrects, since the next create counts what exists. That is the better trade: the old design bought serialization with a counter that could only climb, which is what cost real tenants their allowance permanently. sms/email are untouched — consumed events with no rows to count. Converting them to a row count would silently uncap them.
…rrency The unit suite runs the guard against better-sqlite3, which is synchronous — Promise.all there overlaps nothing, so it can only assert the statement's logic. Two things are testable only under workerd: - That D1 accepts the statement at all. `INSERT ... SELECT ... WHERE` with an `ON CONFLICT ... DO UPDATE ... WHERE` whose predicate is a correlated subquery over another table, plus `excluded.` inside that predicate, is not a shape D1 code usually reaches for, and `meta.changes === 0` on the no-rows-selected path is the signal the whole gate reads. Confirmed. - What concurrent callers actually observe. At the cap, six genuinely overlapping consumes ALL fail — no phantom pass. Below the cap, two that overlap before either row lands both pass, which pins the bound recorded in guard.ts as real-engine behaviour rather than a test-driver artefact.
Retires the 2026-08-05 manual production correction rather than leaving it to be repeated. guard.ts header now records that `usage_counters.value` for `inspections` is a cache and not the gate, so the next person who finds a value higher than the row count does not "fix" it. Deleting the row is worse than leaving it, and the header says why. sms/email are called out as the opposite case: consumed events whose counters ARE the source of truth. /api/usage decision (plan Task 4 Step 2), taken rather than left open: a tenant whose inspections are capped now gets the live row count — the number shown against a cap has to be the number the cap is enforced against, or a tenant who deletes three inspections is told "5 of 5 used" while creating works, which is the visible half of the defect. Uncapped tenants keep the cumulative lifetime counter: with `caps: null` it is measured against nothing, and silently redefining it from "ever created" to "currently have" would change an analytics figure nobody asked to change.
lint:provider-helpers hard-fails on `drizzle(c.env.DB)` in an API route; route handlers go through the helper.
recordPayment stamped TxnDate with the push date (UTC today), discarding the ledger row's occurred_at — an inspector recording Tuesday's cash on Thursday booked it into the wrong accounting period. TxnDate is a bare calendar date, so it is now derived from occurred_at in the TENANT's timezone (epochMsToWallClockYmd + resolveTenantTimeZone): a 6pm Pacific payment is the next day in UTC, a real one-day period error at month end. All three push sites (mark-paid, offline recording, Stripe webhook) pass the appended ledger row's occurredAt. The check-tz-safety header no longer names QBO payment TxnDate as a legitimate UTC-today use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
removeInspectionService soft-deleted unconditionally. The deferral written at the function came due when the reports table landed: with no FKs by design, nothing else surfaces a report left pointing at a line that is no longer on the invoice. Removal now returns 409 Conflict naming the blocking report (in_progress and published both block — both are work the line paid for); reports on OTHER lines do not block. The pay-split clause stays deferred to the pay-splits plan as its own explicit step, since inspection_service_pay_splits still does not exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
…er console Per-tier tenant counts bucketed managed/byo/unconfigured by the SAME resolveAi call the runtime meters with — resolveRuntimeAiSource is extracted from buildAiMeter so the still-false entitlement literal lives in exactly one place and the console can never disagree with the resolver. Wire contract pinned by portal's narrowAiProvisioning: a tier with no tenants is absent, never zeroed; all three counts finite. The /usage handler moves verbatim to server/portal/usage-report.ts — the file-size ratchet asked for an extracted unit, not a baseline bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
A mutating route with no idempotency story looks identical to a guarded
one at the call site — the duplicate row only appears when a customer's
network retries a POST. This freezes today's surface as a burn-down
ledger and fails on any new mutating route that arrives without one.
Ported from portal's equivalent gate rather than written fresh, keeping
its four-category ledger (verified / pending / uncoveredByDesign /
knownUnreachable), its fail-closed behaviour when zero routes are
discovered, and its stale-entry check — a ratchet that only ever grows
lies about progress. Discovery is rewritten for OI's layout: the walk
starts at server/index.ts's .route() mounts and follows sub-routers,
alias chains, ident statements and router-taking helpers recursively.
Evidence files diverge from portal's: only `*-replay.spec.ts` under
tests/unit/idempotency/ and `*-idempotency.test.ts` under app/ count.
The mechanism's own unit specs live in that directory and quote real
route paths as sample input — scanning them would mark routes verified
on the strength of a hash test that never calls them.
Seen red both ways before wiring: a throwaway
`testHooks.post('/x-throwaway')` was named and exited 1, and a hand-added
`POST /api/gone-away` pending entry failed as stale.
302 of 312 declared mutating routes resolve; 287 pending, 14 by design,
1 verified.
Config-only, in its own commit: package.json in a diff escalates the pre-commit type-check to the full tier, so it does not ride along with code. Registered in `lint` AND `lint:gates-full` — the two chains are hand-duplicated, and a gate added to one drifts silently out of the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
Portal's tier console owns the numbers and records who set them; core needed somewhere to put them and a command case to receive them. Storage is the existing tenant_configs.integration_config blob under a reserved key, so this lands with no schema change. Two properties of that column make sharing it safe, and both are load-bearing, so both are asserted rather than described: the tenant-facing writer MERGES over the stored object (a Settings save cannot drop a key it does not know about), and the tenant-facing route validates against a closed Zod object that STRIPS unknown keys (a tenant cannot write their own allowance). No cap constant appears anywhere. An absent key, an absent tier, an absent metric and an unparseable blob all mean the same thing — unconfigured, so unenforced. Caps resolve through a loader rather than at guard construction, so the per-request site pays no read and no tenant's caps can bind to another tenant's check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
The guard could already read an allowance; nothing gave it one. All seven production construction sites now pass tenantAiCapsLoader, and checkAiQuota resolves it at the moment of the check. A loader rather than a value, because two of those seven sites make an eager read wrong rather than merely wasteful: the DI middleware constructs a guard on every authenticated request, and the cron tick reuses one guard across every tenant — where a single resolved value would bind the first tenant's caps to everybody else's check. Tests keep passing an object, which is what makes a configured-cap control cheap to write. managedEntitled is still a literal false, so no cap is reachable in production yet; this is the path being ready, not switched on. No cap constant is introduced anywhere: an absent loader, an absent tier and an absent metric all mean unenforced, and FREE_TIER_CAPS is asserted to carry no AI entry so that "no managed allowance" cannot silently inherit one. Seen RED at "promise resolved undefined instead of rejecting" with the resolution removed — while the fourteen object-shaped cases stayed green, which is precisely why the loader case had to be added. Baseline +1 on sms.ts and di.ts: one import line each into files already grandfathered at 844 and 433. Splitting either is a refactor this change does not justify. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
Tier 1 of the burn-down starts where a duplicate costs money. Creating an
invoice twice does not just duplicate a record — it doubles what the tenant
believes is owed, and pushes the duplicate into their QuickBooks where a
human has to unpick it.
The route is tenant-authenticated, so the global mount already sits in front
of it. What the mount does not tell you is whether the endpoint's side
effects all happen INSIDE the guard's span: the QBO upsert is scheduled
through executionCtx.waitUntil from the handler, and anything scheduled
outside the guarded window would repeat on a replay while the invoice row
correctly did not. So the spec drives the real router behind the real
middleware over a real in-memory D1 and counts rows and provider calls.
Seen RED with the guard removed from the harness, on all five containment
cases — "expected [ {...}, {...} ] to have a length of 1 but got 2",
"expected vi.fn() to be called 1 times, but got 2 times", and "expected 201
to be 422" for the changed-payload case.
Baseline: 287 -> 286 pending, 2 verified by replay spec.
…wice
Covers POST /api/invoices/{id}/payments and POST /api/invoices/{id}/mark-paid.
Both fan out past the invoice row — the report's payment gate is opened and
the movement is pushed to QuickBooks — so the 201 is not the thing to assert
on. The offline ledger is append-only BY DESIGN (a correction is a new row,
never an edit), so a retried POST conflicts with nothing and looks like an
error from no angle. It just books the same cash twice.
Two findings the red run forced into the spec:
- Written around a FULL payment the duplicate is invisible: the overpayment
refusal fires first, so the second row never lands and the guard cannot be
seen to do anything. The duplicate proof therefore uses a partial deposit,
which nothing refuses. The settling case is kept as its own test for what
the guard actually changes there — the operator gets the original receipt
back instead of a balance error about a cheque they have already banked.
- mark-paid is append-once on its own (markPaid returns nothing when the
ledger already covers the balance, and the QBO push is conditional on that
return), so those two assertions pass unguarded. They are kept, labelled
CHARACTERIZATION, so nobody later reads them as evidence. The route still
needed the guard: the payment-gate call is unconditional.
Six of eight cases seen RED with the guard removed from the harness —
"expected [ {...}, {...} ] to have a length of 1 but got 2", "expected
vi.fn() to be called 1 times, but got 2 times" (QBO push, and the payment
gate on mark-paid), "expected 422 to be 201" (settling retry refused instead
of replayed), and "expected 201 to be 422" (changed payload under one key).
The two survivors are the fresh-key control and the characterization test.
Baseline: 286 -> 284 pending, 4 verified by replay spec.
POST /api/admin/agreements/send. An email that has left cannot be recalled,
and this one carries a per-signer signing link — a retried send puts a second
"please sign" in the client's inbox with a link that also works, and nobody
can tell afterwards which one they used.
The envelope is NOT the exposure: AgreementService.findOrCreate is
find-or-create, so an unguarded duplicate returns the same requestId and
leaves one envelope. That is kept as a labelled CHARACTERIZATION test so the
requestId assertion is never mistaken for the containment proof. What
actually repeats is the tail — one outbound email per signer, and the
request.sent entry in the tamper-evident audit chain, which is supposed to be
the record of how many times the envelope was mailed.
The changed-payload case turned out to be worse than a wasted email:
unguarded, a retry carrying a different signer list is MERGED into the live
envelope ("findOrCreate merged signers", added: 1), adding a party to an
agreement already out for signature. The spec now asserts the signer rows,
not just that no mail went out.
Seen RED with the guard removed: "expected vi.fn() to be called 2 times, but
got 4 times", "expected [ [...], [...] ] to have a length of 1 but got 2"
(audit chain), "expected null to be 'true'" (no replay flag), and "expected
200 to be 422" (changed signer list accepted under the used key).
Baseline: 284 -> 283 pending, 5 verified by replay spec.
POST /api/inspections/{id}/send-sms.
The ledger's claim was checked before writing anything, and it holds: Task 4's
title said "email and SMS" but its Files list only ever named the email
builder, so there is NO service-level dedupe on the SMS path — nothing
resembling buildEmailDedupe exists under server/lib/sms/ or in
send-one-sms.ts. Reading the handler end to end confirms it: every call mints
a fresh automation_logs id, inserts a pending row, and hands it to sendOneSms
without consulting whether this message already went out.
The claim needs one refinement, because the two halves are easy to conflate.
The HTTP route IS tenant-authenticated, so the global mount does span it — the
guard contains a retried REQUEST. What the missing service-level dedupe means
is that anything reaching sendOneSms by another path (the automation flush, an
Outbox resend) is still unprotected. This commit closes the first, not the
second.
A duplicate here costs twice: the carrier charges per segment and the send
meters against the tenant's quota. Unlike email it also lands on a phone.
Seen RED with the guard removed: "expected vi.fn() to be called 1 times, but
got 2 times" (the Twilio seam), "expected [ {...}, {...} ] to have a length of
1 but got 2" (the SMS ledger the Outbox and metering read), "expected null to
be 'true'", and "expected 200 to be 422" for a changed recipient under a used
key. The survivor is the fresh-key control.
Baseline: 283 -> 282 pending, 6 verified by replay spec.
…n replay
POST /api/inspections/{id}/agreement-requests — the sibling of the admin send,
and the reason it gets its own commit rather than a footnote: the inspection
workspace's "Send agreement" button posts HERE, not to /api/admin, and the two
share emailSignersTheirLinks precisely because they had already drifted into
mailing different links once (IA-65).
A shared helper is not shared containment. Verifying one route and calling the
behaviour covered would have left the button most inspectors actually press
outside the ledger, with the gate reporting progress for it.
Seen RED with the guard removed: "expected [ 'jane@example.com', ...(1) ] to
deeply equal [ 'jane@example.com' ]", "expected null to be 'true'", and
"expected 200 to be 422" — the last one again logging "findOrCreate merged
signers, added: 1", i.e. a mis-keyed retry adding a party to an agreement
already out for signature.
Baseline: 282 -> 281 pending, 7 verified by replay spec.
… client
POST /api/invoices/request-payment — the hub's "Request payment" button. The
endpoint resolves or CREATES the inspection's invoice, marks it sent, mints
the recipient's portal token, and emails a working link to the public payment
page.
Two of those are self-limiting and are asserted as CHARACTERIZATION rather
than as evidence: the invoice is reused when one exists, and issueToken is
deliberately idempotent so older copies of the email keep working. The email
is not self-limiting — a retry sends the client a second "please pay for your
inspection", the message in this system most likely to be read as a second
bill.
The red run turned up one effect that was not obvious from reading the
handler: unguarded, the replay reruns markSent and the returned sentAt MOVES.
A retry nobody made rewrites the invoice's own record of when it was sent, so
that assertion is now explicit rather than incidental to a deep-equal.
Seen RED with the guard removed: "expected vi.fn() to be called 1 times, but
got 2 times", the sentAt drift above ("2026-08-06T00:52:52.722Z" vs
"...52.753Z"), and "expected 200 to be 422" — that last one meaning a key
reused against a DIFFERENT inspection raised an invoice on the wrong job.
Baseline: 281 -> 280 pending, 8 verified by replay spec.
The concurrency case fired its second request immediately after the first and
assumed the first had already claimed the key. It usually had. Under load it
sometimes had not — and then BOTH requests claim, both enter the handler, and
both park on a gate that is only released after the second returns. The
deadlock surfaces as a 5s timeout, which reads like a slow test rather than
the ordering bug it is.
Observed for real: adding six replay specs to this directory raised the
parallel load enough to trip it ("Test timed out in 5000ms" on
tests/unit/idempotency/middleware.spec.ts:113), while the file on its own
passed every time. Found by running the directory, not the file.
The fix waits for the handler to be ENTERED, which is strictly after the
claim, so the overlap is ordered rather than hoped for. It does not weaken the
test: `ran` becoming 2 still fails, and a missing in-flight branch still parks
the second request and times out.
Splits are RECORDS, not a calculation: a tenant rule populates a row once,
then only a deliberate edit moves it. A derived split would rewrite what
someone was already paid whenever the rule changed.
Grain is the billing line (`inspection_services.id`), the same key
`reports.inspection_service_id` uses, so "what did this line earn and what
did it produce" stays joinable.
Two partial unique indexes rather than the obvious plain ones:
- `service_pay_rules` — SQLite treats NULLs as distinct, so a single
unique over (tenant, service, user) silently accepts TWO default rules
for one service and the populate step would then pick one arbitrarily.
- `inspection_service_pay_splits` — one PRIMARY split per (line, user),
but correction rows against a locked split must be insertable, and an
unconditional unique makes that path impossible to write.
Both tables are payroll records about staff, so they are declared in
ERASURE_OUT_OF_SCOPE with a reason: a client's erasure request does not
reach them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
Rules populate a split row; after that only an explicit edit moves it.
`populateSplits` is additive — an existing (line, user) pair is skipped
whatever the rules now say — and `refreshSplits` is the single, explicit
path that re-derives, behind `previewRefresh` so nobody's pay moves
without someone deciding it should.
Three rules that are easy to get wrong and are pinned by tests:
- The computed amount is DIVIDED by the number of inspectors eligible
for that line. Without it, attaching a second inspector pays out
120% of the service.
- Reads filter `inspection_services.is_active`. A line declined at the
door survives because a report or a split may point at it; paying
against it anyway is what that column exists to prevent. A split
whose line is deactivated later surfaces as an orphan, the same
treatment a removed inspector's split gets.
- `percent_after_deduction` takes the deduction off the top BEFORE the
percentage, so it is not restatable as a smaller percentage.
The roster is read through `getInspectionRoster` only, and assignment
writes now go through `syncAssignmentsAndSplits` rather than
`syncInspectionAssignments` directly — the roster write and the pay
reconciliation move together so a new assignment path cannot record who
worked the job while leaving the money on whoever worked it before. The
reconciliation is deliberately quiet: a misconfigured pay rule must not
make saving an assignment fail.
Once payroll exports a split it locks, and a later adjustment is a new
row carrying the delta. An in-place edit after money has moved
desynchronises the books from what was actually paid with nothing
surfacing the divergence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
…alls lint:deadcode exits 1 on it, and knip-baseline.json is empty on purpose — an export with no consumer is a failure, not an allow-list entry. Its only caller is refreshSplits in the same file; Task 3 gives it a route and can export it then. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
…nspectorHub#278) An inspector may see their own pay and nobody else's. That is a THIRD state — `financial: false` AND `subject = self` — and no boolean permission expresses it, which is exactly why Housecall Pro and Jobber do not offer it: their pay permissions are binary. So it is implemented as query scoping, not a capability and not a redactor exemption. financial: true -> every split on the inspection, editable financial: false -> only rows where user_id = the caller, read-only `server/lib/auth/money-redaction.ts` is deliberately untouched. That redactor stops an endpoint leaking the COMPANY's money; an endpoint that structurally returns only the caller's own row is not leaking, so there is nothing to exempt, and the single shared security-sensitive file stays as it was. No `payroll` capability was added. The line is the existing `financial`. Mounted under the inspections aggregator rather than server/index.ts, which sits at 703 of a baselined 704 — a top-level mount costs two lines and hard- fails lint:filesize. The payroll export is company-level, so it went to the team router (staff administration) instead of inventing a home for it. The route definitions are NAMED CONSTS, and that is not style. `check-idempotency-coverage.mjs` discovers routes by resolving `.openapi(IDENT)` to a `const X = createRoute(...)`; a route written inline as `.openapi(createRoute(...), handler)` is INVISIBLE to it and is never asked for a retry story. Written inline first, all four money routes passed the gate by not existing to it. That is also why `POST /api/inspections/{id}/services` (inline, shipped earlier) is absent from the baseline while its named-const siblings are there — a pre-existing hole, reported not fixed here. All four mutating routes are VERIFIED by a replay spec, not parked: - POST .../corrections writes a NEW row carrying a delta, so an unguarded retry pays the delta twice and the ledger stays internally consistent while doing it. - POST /api/team/payroll-export LOCKS what it returns, so a replayed handler hands the operator an EMPTY run and the money reads as unowed. - PATCH and refresh are contained on their own; asserted as characterization and labelled, so nobody reads them as evidence for the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
Authentication is not authorization, and this router had only the first. It verified the JWT and ran the handler — no role, no capability — so any signed-in inspector could call POST /disconnect, which revokes the Intuit refresh token and deletes the tenant's whole qbo_entity_map. That table is the OI-invoice to QBO-invoice correspondence; reconnecting does not restore it, so the next push writes duplicate invoices against the same DocNumbers. Listing a Stripe webhook log, in the router next door, already required owner or manager. Neither authorization gate could have caught this. check-capability-declarations scans for createRoute(withMcpMetadata( windows and this file is a hand-rolled Hono router with none, so it passed vacuously; the authorization-surface spec is the same inversion against the live registry. Both ask whether declaration and enforcement agree — a route that declares nothing and mounts nothing looks correct to both. So the assertion is an explicit HTTP-level spec instead. Seen RED at "expected 200 to be 403": an inspector was getting 200 on disconnect. The owner/manager control passed before the fix too, which is why it is there — without it the four refusal cases would also pass against a router that refuses everyone. Applied router-wide rather than per route, since /status exposes the connected realm, company name and sync errors, and a uniform surface gives a future edit no per-route reasoning to get wrong. It also refuses a caller with no role at all, which is what an agent JWT is: it satisfies the verifier and carries no tenant by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
…s basis (InspectorHub#278) Two thirds of this already shipped, so this is a rewrite of `byInspector` rather than an introduction. Every field of the old row is gone, and each for its own reason: count -> ledCount + assistedCount revenue -> payCents + attributedRevenueCents avgTurnaroundDays -> medianTurnaroundDays + turnaroundBasis The lead-only grouping the old query used was a deliberate anti-double-count — its own comment said so. Widening it to the whole roster is right for COUNTS (a helper was there) and wrong for the company's revenue, so the count widens and that figure does not follow. What follows instead is `attributedRevenueCents`, which is an attribution and is NOT additive down the column: two inspectors on one job are each credited with the work they were on. It is eligible-line scoped by the SAME rule pay splits use, so the two columns sitting side by side are comparable. Pay is not attributed revenue. Housecall Pro names the worker's money `Commission Cost`, which is safe there because only an owner opens that report. Our inspector opens this page, so it is Pay, and there is no column called "revenue" holding both. Median, not mean: one delayed report on a complex property drags a mean and misrepresents the person it is attached to. Turnaround anchors, both corrected: - END is `reports.published_at` — per deliverable, nullable — NOT `report_versions.published_at`, which is NOT NULL and fires per version AND per amendment. The old query also joined report_versions on version_number = 1 scoped by inspection_id alone, so an order delivering several reports scored an arbitrary one. - START is MAX(inspection_events.completed_at), which has no frontend writer yet. So the metric reports `turnaroundBasis: 'no_data'` and renders "no field-completion times have been recorded" rather than substituting a booking-confirmation clock, which measures how fast the office confirms bookings under this metric's name. Referrals: `referred_by_contact_id is not null` was silently dropping every row whose only attribution is free text — and for a one-person firm those are usually the only rows there are. They come back as a second bucket tagged `kind: 'source'`, listed after the contact-keyed rows and never merged into them; they are different kinds of answer. The route widens to `inspector` with scoping instead of a capability, matching the pay-split surface: without `financial` the caller gets their own row and the company figures as NULL, not zero. Zero is a claim about the business. Verified in Chrome, light and dark: the By Inspector table renders Led / Assisted / Pay / Attributed revenue / Median turnaround with the basis line under the title, legible in both themes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
…y state
Two independent defects, either of which alone kept the QuickBooks connect
flow from ever running. Neither is visible from the settings page, which is
why the integration has never completed a handshake.
First, unreachable. workers/app.ts forwards an explicit prefix allow-list to
the Hono app; /settings/** is not on it, so the connect and callback routes
went to React Router, which has no such route. The webhook next door lives
under /api/* and works — that asymmetry was the tell. /connect and /callback
move to /api/integrations/qbo, beside the webhook. The siblings (/status,
/pause, /sync, /errors/:id/retry, /contacts/:contactId/link) stay where they
are: the settings page reaches those through the in-process API_WORKER
binding, and they work today.
Second, the session cookie cannot arrive. __Host-inspector_token is
SameSite=Strict, and Intuit returns the user by a cross-site top-level
navigation — the exact case Strict withholds a cookie on, as the portal
cookie right below it in auth-helpers.ts already documents. Making the route
reachable alone would have turned a 404 into a 401.
So /callback is unauthenticated and authorized by state instead: the value
stored under qbo_oauth_state:${state} is now the tenantId rather than the
placeholder, and the callback resolves the tenant from it. The state is a
server-generated UUID, single-use, 600s TTL, and only ever issued to a caller
who has just passed the owner/manager guard on /connect — which /connect
keeps. Declared public in jwt-auth.ts alongside the QBO webhook, for the same
reason: neither caller can hold a session.
Also found while tracing, not in the brief: the client id and secret can be a
per-tenant secret, and integrationSecretsMiddleware only merges those once a
tenant is known — which on this request it is not, in saas mode. The callback
loads them for the tenant its state names, using the same helper and the same
precedence rule.
redirectUri was built from the same literal in two places. Both now call
qboRedirectUri(), and the mount plus the jwt-auth entry come from the same
constants, so the path cannot move without the URI moving with it. Intuit
compares that string byte-for-byte against the registered value.
The load-bearing test sends what Intuit sends — a bare GET with no Cookie
header — and asserts it completes against the right tenant. Proven red first:
"expected 401 to be 302".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
The accounting host was compiled in — quickbooks.api.intuit.com, in services/qbo/api-base.ts and again in the OAuth callback's companyinfo lookup. Sandbox lives at sandbox-quickbooks.api.intuit.com, and the two are not reachable with the same credentials: Intuit Development keys authenticate only against sandbox companies, Production keys only against real ones. A build that can only address production therefore cannot be exercised against a sandbox at all, which is a large part of why this integration has never run end to end. QBO_ENV names the host. It fails closed — unset or unrecognised raises before any request leaves the worker, and the OAuth callback refuses to store a connection rather than saving a working token against an API the worker will then decline to call. No fallback constant, because either default is wrong half the time and both failure modes read as credential problems: pointed at production with Development keys you get an auth error, and pointed at sandbox with Production keys a paying customer's books quietly receive nothing. Resolved lazily off the constructor rather than eagerly, since the service is built for every request that touches an invoice and a deployment with no QuickBooks connection should not fail on a setting it never uses. The token and revoke endpoints are shared by both environments and are unchanged. Asserted through apiCall — what matters is the URL that actually leaves the worker, not a helper in isolation. Proven red first: "expected 'https://quickbooks.api.intuit.com/...' to be 'https://sandbox-quickbooks.api.intuit.com/...'", and the two fail-closed cases resolved instead of rejecting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
refreshToken treated any non-2xx from Intuit's token endpoint as "reauthorize required" and DELETEd the connection row. Intuit rotates the refresh token every 24-26 hours, so that path runs on every connected tenant every day, forever — which means one Intuit 5xx, or one rate limit, permanently severed a paying customer's integration and sent an owner back through the whole OAuth flow. Their books stop syncing in the meantime, with nothing recorded that says why. Only an explicit refusal of the grant is terminal now. 400 (invalid_grant) and 401 are the answers that actually say the token is dead; 429, 5xx and network errors say nothing about its validity, so the row is left intact and the next attempt uses the same token. The two cases also raise distinct messages, so a log line no longer has to be guessed at. Intuit has begun returning a field on this response that dates the refresh token's hard expiry. It is deliberately not read: the exact key could not be verified from the published documentation, and keying a destructive delete off a guessed field name would reintroduce exactly the failure this removes. The status code already separates the two cases; the field would only allow disconnecting EARLIER, which is not the useful direction. Proven red first: at 500 and at 429, "expected undefined to be truthy" — the connection row was gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFkgupms2h36Mmf11ojFan
The spec reads its SQL out of docs/developers/multilingual-demand-signal.md by matching a fence followed immediately by \n. A Windows checkout has CRLF there, so the pattern matched nothing, every block was silently missed, and the failure surfaced as "doc has no -- Query D block" — pointing at the document rather than at the parser that had read none of it. CI is Linux, so this passed there while being unrunnable on the machine the document is edited on. Also two glossary violations in the new metrics translations, which only the full lint chain can see. "tus" is ruled out for second-person possessive. And English "Pay" now has a declared divergence: it is the verb on the client checkout (Pagar) and a column header naming what an inspector earned (Pago). Pagar as a header reads as an instruction to pay someone. The header is deliberately "Pay" rather than "Cost" because the inspector reads this column, and calling their earnings a cost states the company's view of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bc5q8eunHDbkqDHtYvEkv
`{ type, value }` is the shape this was specified in and not the shape it
ships in. A bare `value` on a money field has no unit — 50 is half the
price under one type and fifty cents under another — which is the exact
correction `CancellationFee` and `service_pay_rules` each already made.
The fields name their own units. It is one refined object rather than the
discriminated union that reasoning would otherwise produce, because
`service.schema.ts` records a MEASURED type-check heap death from union
members crossing hono/client on these same services routes.
The order rule is the part worth reading twice. A workspace default
applies ONCE to an order, not once per line, or a flat $100 deposit
becomes $300 on a three-service booking. But applying it to the whole
order total instead re-charges the lines that said `{ type: 'none' }` —
and opting out is the only reason tier 2 exists. So the default covers
what no service has spoken for, and the services that price their own
deposit are added to it. Both failures are asserted.
`none` is a value, not an absence: NULL already means inherit.
Tier 3 gets a flag rather than one clever column. `deposit_required_cents`
alone cannot tell a computed snapshot from a figure an operator agreed on
the phone, so a later re-resolve would overwrite the second in silence.
All four columns append at table END — `inspections`, `services` and
`tenant_configs` are all FK-referenced, and a mid-table insert makes
drizzle rebuild the table, which loses tables on remote D1 without
`db:check` saying a word.
Two files had no room for any of this and neither baseline moved. The
discount codes were interleaved through `ServiceService` rather than
grouped, so they are now one file like qualification and pay-rules
already are; the three soft references an inspection PATCH can dangle
were three look-alike blocks in a route handler and are now one unit,
where whoever adds the fourth will find them.
`extractSettledPayment` returned null for any intent without
`metadata.invoiceId`, and a booking deposit has no invoice by
definition. So a settled deposit made the handler log `received`, ACK,
and stop: money in Stripe, nothing in the ledger, and nothing anywhere
saying so. The fix is a discriminated `metadata.kind` — the id follows
from the kind, rather than the kind being inferred from which id turned
up — so a future purpose adds an arm and the webhook's branch stops
compiling until it is handled.
Three properties the tests pin, because each was a way to lose money:
- The deposit row is written on WEBHOOK CONFIRMATION and nowhere else.
`POST /book` makes no Stripe call at all; it freezes what is owed
and returns it. A declined card therefore leaves a real appointment
with an unpaid deposit the tenant can see — a decline that also
loses the booking is worse than having no deposit feature.
- A deposit does not call `markPaid` and does not call
`markPaymentReceived`. $90 of a $450 job is not payment in full, and
the second of those is the gate the public report reads.
- `amount_received`, not `amount`. The ledger records what arrived.
The new public route is new because it had to be: the existing
pay-intent is gated on `resolveClientActor`, and an anonymous booker
thirty seconds after submitting has no portal grant and no invoice to
charge. What authorises it is the inspection id from their own booking
response, and the header says so plainly along with what that does and
does not permit. Every refusal is the same 404, so it cannot double as a
probe for which ids exist.
Retry safety is Stripe's own idempotency key rather than the mounted
guard, which never engages: the caller is a payment panel that sends no
`Idempotency-Key`. The key carries the OUTSTANDING amount, so a partial
deposit gets a fresh intent for the remainder instead of replaying a
stale one for money already collected.
The deposit basis is the summed catalogue price, read at booking and
snapshotted — NOT tier 2. The booking path writes no
`inspection_services` rows, and wiring `writeInspectionServiceSnapshots`
into it changes invoice totals for every booking-created order. That is
a change for someone to make deliberately; the comment at the snapshot
field says so, because the next reader will otherwise "fix" it.
Two ratchets went red and neither baseline moved. The public booking
PROFILE reads are now their own file: they answer "who are you and what
do you sell" while everything left answers "what times are free", and
they were the only two raw `.get()`s interleaved among the OpenAPI
routes — a shape difference that reads as an accident when mixed in and
as a fact about their age when grouped. The deposit route mounts through
that same aggregator instead of `server/index.ts`, which needs no line
at all for it and keeps the external path identical.
… shut Raising an invoice is where a held deposit stops being a liability and becomes money against that invoice. It is the ONE exception to the ledger's append-only rule — `invoice_id` written once onto rows that predate the invoice — and nothing else about the row moves. The re-sync of the report gate is the line that looks wrong and is not. This function can only ever ADD money, so downgrading a gate reads as backwards until you notice what the gate actually caches: "some unvoided invoice on this order is paid". Applying a deposit moves an order from "no invoice" to "an invoice with a partial payment", and the re-sync is what asserts IN CODE that $90 against $450 leaves the report locked. The test sets `payment_status = 'paid'` first so it fails loudly if the call ever goes away — deleting it turns green to red with 'paid' to be 'unpaid', which is the sentence a reader needs to see. Idempotent by construction rather than by a flag: it claims only rows whose `invoice_id` IS NULL, so a second invoice on the same order finds nothing to claim and the client is not credited twice. `createInvoice` now returns `amountPaidCents` / `partialPaidAt`. The hand-built row it answered with carried neither — they are written by `recomputeInvoicePaymentState` — so a caller reading a deposit-bearing invoice got `undefined` and reported it as having received nothing. Also asserted: the held-deposit count on the QBO Books health card falls to zero when the deposit is applied. That figure is the tenant's "record these manually" list, and a list that only ever grows is one nobody reads.
…own writer The seam O5's ladder and this plan meet at. `resolveCancellation` caps every fee at `paidCents`, and a booking deposit sat outside that number because it has no invoice. So the exact case the deposit exists for — a no-show on a job nobody invoiced — quoted zero collected, charged nothing, refunded nothing, and left the money held with no surface saying so. The feature was inert precisely where it was supposed to bite. It counts now. Counting it alone would have been WORSE than leaving it out, and that is why the second writer is in the same commit. `applyCancellationRefund` needed an invoice; a quote that promised $90 back against a write that returned null is a promise the product does not keep. Deleting the routing turns the spec red with "expected null not to be null", which is the sentence describing that failure exactly. `refundPartial` was NOT bent to take a null invoice. Its body is "load the invoice, seed its ledger from its own record, check what IT received, append against it, recompute ITS cache" — four of those five steps mean nothing without an invoice, and a flag would have put two functions behind one name with the guard-skipping one handling money nobody has billed for yet. `refundHeldDeposit` is a sibling in the same file, and its header states what is absent and why. The two pools are normally disjoint — raising an invoice backfills `invoice_id` onto the deposit rows — and overlap only when a webhook lands after the invoice was raised. That case drains the INVOICE first: its `amount_paid_cents` is a figure a human reads off a screen, and leaving it overstated while the refund came out of an invisible pool is the same "cash in one place and not the other" failure in miniature. The retained-Stripe-fee quote moved to order scope too. A deposit is the likeliest card payment on a job cancelled before invoicing, so the invoice-scoped lookup quoted a $0 processing loss on exactly the cancellation where Stripe has kept its fee.
Two separate jobs, and putting them in the wrong order is how a deposit
becomes a chargeback.
BEFORE: the services step and the confirm summary QUOTE the amount, from
the same pure resolver the server runs, and both say it comes off the
total rather than sitting on top of it. A client who discovers a charge
after clicking Book writes a review about it.
AFTER: the payment panel renders only once the booking exists and only
when the SERVER says money is owed. The client-side quote can never
conjure one — the panel charges `depositRequiredCents` off the booking
response, which is what was frozen on the order. A test pins that by
having the server answer $50 where the form guessed $90.
The panel's most important line is the small grey one: "Your appointment
is already booked. Paying the deposit secures the slot." A decline shows
"Your appointment is still booked", and a 503 from an unconfigured
workspace says the same thing in the same place. Nowhere does this
surface imply payment is what confirms the booking, because it is not.
Money renders through `formatCurrency`, never `toLocale*`.
Walked in Chrome against a real local workspace on 20%, both themes,
which is where two things showed up. A stale paraglide module in the dev
server crashed `ServicesStep` outright ("m.booking_deposit_quote_note is
not a function") — a hard reload, not a code defect, but the crash is
what a stale build looks like and it took the whole page. And the
opt-out arithmetic held in the browser exactly as the unit test claims:
adding Radon (`{ type: 'none' }`) moved the total $250 → $400 and left
the deposit at $50.00. Submitting wrote 5000 on the primary inspection,
0 on the sibling, and nothing at all in `order_payments` — owed, not
collected, which is the whole invariant.
The embed widget gets NO deposit and now says why at the call site: it
collects no service, so there is no price for a percentage to resolve
against and the order it creates carries `price: 0`. Giving it a deposit
means giving it service selection first.
…right The append-at-end ratchet on `inspections` fired on `deposit_required_cents` + `is_deposit_overridden`. Exactly its job: the columns ARE at the tail, so the list grows by two and the comment says which change put them there. A mid-list insert would have made db:generate emit a table rebuild instead of ALTER ADD COLUMN, and on remote D1 that loses tables without db:check saying anything. Tenant-scope flagged two post-insert read-backs — the new deposit-aware `createInvoice` re-read, and the discount-code read-back that moved file in the extraction. Both are provably safe (a primary key this function just generated inside a tenant-scoped insert), which is precisely the case the gate lets you baseline. Scoped them instead: the filter is free, and a baseline entry is a judgement someone has to re-derive later. That also retired a stale entry, so the ratchet tightened by one rather than staying put. Knip flagged two exported types nothing imports. `PublicDepositIntentApi` was cargo-culted from the sibling router that IS consumed by name — this one mounts inside the bookings aggregator, so `BookingsApi` already carries its RPC shape. `PaymentPurpose` is narrowed structurally off `settled.purpose.kind` and never named. Deleted both rather than baselining; a name nothing imports is surface a reader has to account for.
The booking deposit shipped with an API that accepts a policy at all
three tiers and a booking flow that quotes and charges it, and no
control anywhere -- so turning it on meant writing the PATCH yourself.
This is tier 1: the company-wide default, in the panel that already
holds the other booking policies.
A deposit is not a checkbox, so it is not a fourth row of them; it is a
segmented control under the same Save, because "clients must sign" and
"clients must pay something up front" are the same kind of decision and
splitting them across two panels is how an admin ends up looking for a
page that does not exist.
Three things here are load-bearing.
OFF IS A STATE. `deposit_policy` is NULL for every existing company, and
NULL means no deposit. The control renders that as a selected "No
deposit" with a sentence saying so, not as an empty box that reads as
half-saved. Turning the deposit off clears the column rather than
storing an opt-out of itself: at company scope there is no third answer.
THE UNIT IS NOT THE PAY-RULE UNIT. A pay rate goes on the wire as basis
points and PayRuleWidget multiplies by 100 to get there. A deposit
percent is a whole percent (`z.number().min(0).max(100)`), so nothing
multiplies it; the only x100 in this path is dollars -> cents inside the
shared MoneyInput, beside the "$". Sending 2000 for 20% would ask a
client for twenty times the price, and only the schema's max(100) would
notice.
ZERO IS REFUSED. The API accepts `{ percent: 0 }`. A policy that reads
as configured and charges nothing is the state the control exists to
prevent, so the panel refuses it and says to choose No deposit instead.
Two seams worth knowing about. The default is written through branding
(`POST /api/admin/branding`) while the rest of the panel writes through
tenant-config, so one Save touches two endpoints; the deposit half is
sent only when the form carried it, because an absent key must leave a
configured deposit alone. And the read comes from branding too --
`GET /api/admin/tenant-config` does not project the column, though both
live on the same row.
The SegmentedControl carries no hidden input. Its value reaches the
server only because handleSave sends the state it owns; the test asserts
the submitted body rather than the DOM for that reason.
Verified in Chrome, light and dark: set 25%, saw
`{"type":"percent","percent":25}` land in D1, and saw the public booking
page quote it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
Tier 2, on the row of the service it prices. The column has accepted a
per-service policy since the feature shipped; nothing offered one, so a
company that wanted 20% everywhere except on a $150 add-on had no way to
say so except by hand.
It is the third widget on that row and deliberately the twin of the
other two. QualificationWidget answers "who MAY run this",
PayRuleWidget answers "what they EARN running it", and this answers
"what the client PAYS UP FRONT" -- three adjacent questions about one
service, so the disclosure, the inline panel and the tokens are copied
on purpose. A third visual language there would read as a different
kind of setting.
WHAT THE PICKER HAS TO SPELL OUT is that NULL and `{ type: 'none' }` are
different answers. NULL inherits the company default; `none` refuses it.
A blank that could be either is how every excused add-on quietly starts
charging again, so the options say "Company default" and "No deposit" in
words, and the collapsed row says which one is in force -- naming the
inherited value ("Company default (25% of the price)"), because
"inherits" on its own tells an admin nothing about what a client will be
asked for.
WHAT IT DOES NOT COPY FROM ITS TWIN is the arithmetic. PayRuleWidget
multiplies its percent by 100 because pay rates are basis points; a
deposit percent is a whole percent, so nothing here multiplies. Two
widgets that look identical and disagree about units is exactly the
trap, hence the note at the top of the file and a test that pins 35 ->
35.
Both tiers share `~/lib/deposit-policy-form` for the choice -> wire
conversion, so there is one place where "inherit" becomes null and
"none" becomes an object.
Verified in Chrome, light and dark, through to the public booking page:
Sewer Scope ($250) fixed at $150, Radon opted out, Mold inheriting 25%.
Selecting all three quotes $237.50 -- $150 + nothing + 25% of $350 --
which is the three tiers resolving against each other correctly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
Routing by distance needs two coordinates, and this schema had neither: no
office anchor anywhere under schema/tenant, and no per-inspector origin at
all. Load-balancing needs a load, and booking rules need somewhere to put a
lead time. One migration lays all of it down.
inspector_service_areas which ZIPs an inspector will travel to; zero rows
means everywhere, mirroring service_inspectors
tenant_configs booking_routing_strategy / booking_min_lead_hours /
booking_same_day_cutoff_time, plus company_lat/lng
for the address the workspace already types into
Settings and only ever used as PDF footer text
users service_origin_address/lat/lng — NULL inherits the
company coordinates, which is what makes `closest`
work for a single-office workspace with no setup
Every column is appended at its table's END; both tenant_configs and users are
FK-referenced, and a mid-table insert would have drizzle rebuild the table
without db:check saying a word. The generated migration is 14 ALTER/CREATE
statements and no rebuild.
An inspector's service origin can be their home address. That is personal
data, but not a CONSUMER data subject's, so it lands in ERASURE_OUT_OF_SCOPE
beside users.email and users.phone with a stated reason and no DSAR path —
the settled position at the top of that block. It would not have been caught
by the PII heuristic either way, which is exactly why it is written down.
inline-ddl.ts was proved RED first (missing 6 tenant_configs columns) and is
fixed in the same commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
…id nothing
Each of these strategies has an input on which it returns a perfectly
plausible inspector while having computed nothing — and in this codebase that
input was not an edge case, it was the only case:
least_loaded every load ties at 0, so the tiebreak IS first_available.
Counted off inspections.date, not scheduled_start_ms, which
has no non-NULL rows in production.
closest nothing is geocoded, so every distance is undefined and the
tiebreak decides again.
zip filter "empty ZIP -> degrade gracefully" was 100% of traffic, because
no public booking ever carried a ZIP.
So routing returns a DECISION, not an id: {requested, applied, reason,
candidateCount}. A substitution is logged as booking.routing.substituted and
written to audit_logs as booking.routing.applied on the inspection. The slot
pipeline reports geoSkipped when the ZIP filter could not run, and
outsideServiceArea — a distinct answer from "that time is taken" — when it ran
and excluded everyone. Every degenerate branch has a test asserting the
REPORTED reason; each was proved red by deleting the guard.
A missing geocode is never a distance. An unanchored candidate is removed from
`closest` rather than sorted to the bottom, because a sort position is a claim
about a distance nobody measured.
Also landing, because they are the same feature seen from the other end:
- lib/places/geocode.ts. The Places details fetch existed and was CORRECT, and
lived inside one JWT-gated route handler, which is exactly why nothing else
could reach it. Booking fulfilment now resolves the submitted placeId to the
property's coordinates and writes address_zip/lat/lng — columns the wizard
had populated for a year and the public form never touched.
- inspector_service_areas CRUD (PUT is replace-by-value, so an unguarded retry
converges; proved, not asserted) and the ZIP filter ahead of the slot union.
- Lead time and same-day cutoff, in the office's wall clock. server/lib/booking
is now inside check-tz-safety.mjs SCOPE — a UTC-day bucket shipped green
until this commit and was caught by a test, one gate later than it should be.
Two extractions paid for the room: slot-arbitration (booking.service.ts had 20
lines of headroom against a hard 400) and the public geocode sub-router
(bookings.ts was at its 477 baseline exactly).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
The server can now route three ways and report when it could not. Nothing
could choose between them, and — the reason the geographic half was dead in
the first place — nothing captured a property ZIP.
Public booking page. The address field is now an autocomplete against
`/api/public/geocode`, the public rate-limited endpoint that has returned a
ZIP and a placeId for as long as the booking page has existed and that the
page never called. A picked suggestion sends `addressZip` + `addressPlaceId`;
a typed one sends neither, and the server says so rather than pretending the
filter ran. Deliberately NOT the dashboard's AddressAutocomplete: that goes
through a session-gated BFF and returns an empty list to a signed-out visitor,
silently, which would have read as "Places is not configured".
Settings → Online Booking gains two panels:
- Routing & booking rules. Three strategies with the readiness of each stated
NEXT TO the option — "needs a Google Places key", "no address has been
located", "only one inspector has a start address". A radio that silently
does nothing is the exact failure this programme keeps producing, so the
panel refuses to present one as live. Company geocoding is an explicit
button with the matched address shown back, not a side effect of saving a
colour, and a failed lookup names its reason.
- Inspector territories. ZIP list + start address per inspector, with the
empty state stated: no ZIPs means all areas, an empty start address means
the company one.
Both hang off a new `/api/admin/booking-routing` sub-router rather than
admin-settings.ts, which sits exactly on its 754-line baseline and has no
business making outbound Google calls from a generic config PATCH.
Chrome walkthrough, light and dark. Three things it caught that no gate did:
the blocker used invented `ih-warn-*` classes Tailwind dropped silently
(now the DS `Banner tone="warn"`, which also carries role="alert");
`lint:ds` does not catch a token that does not exist. Live, with the tenant
in UTC-4, a 24h lead time made 2026-08-07 bookable from 13:00 local exactly —
the tenant zone, not UTC. Clearing the cutoff restored all 18 slots.
settings-booking.tsx crossed 400 lines, so the read and write halves were
extracted to ~/lib/settings/booking-routing-{data,actions} rather than the
baseline being raised.
en + es-419 in this commit; FALLBACK_ALLOW stays empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
`test:workers` went red on `table users has no column named
service_origin_address`, in four cmd-consumer / cmd-fixtures specs, after
lint + test:unit + test:web had all gone green.
The reasoning that let it through was wrong in a specific and reusable way:
"`db.insert(users).values({ id, email, … })` only binds the columns you pass".
It does not. Drizzle emits EVERY column of the table and nulls the rest, so a
partial insert is exactly as exposed to hand-written-DDL drift as a full one —
which is why `applyAdminCredential`, a five-field insert, parked on three
columns it never mentions.
The guard existed and pointed at the wrong tables. `inline-ddl-schema-sync`
covered tenant_configs and inspection_results; `users` had its DDL copy-pasted
into two workers specs with no assertion over either. Now there is one source
(`USERS_TEST_DDL`), both specs import it, and the sync spec asserts coverage —
proved RED first, naming exactly the three missing columns.
`test:workers` is not in the pre-push three-suite run, so without this the
next person to add a users column finds out from CI too. This is the third
table to teach the same lesson and the first one to get the assertion in the
same commit as the lesson.
Also: two pinned `getTenantSlots` call-shape assertions in
bookings-company-endpoints.spec.ts updated for the propertyZip argument, with
the reason `null` and not `undefined` is passed written down at the assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
calendar_external_links answers "does this thing already exist on that person's calendar, under which id" — the fact a push needs to update instead of duplicate, a cancel needs to delete the remote copy, and an import needs to recognise its own events and skip them. entity_type is 'inspection' | 'calendar_block'. Inspection events are deliberately out of v1. inspection_events.gcal_event_id already holds that mapping, and the obvious move is to backfill it and freeze the column — but it cannot be done correctly. user_id here is NOT NULL and names whose calendar holds the event, and the push that wrote gcal_event_id sent every tenant event to whichever user pressed the button without recording who that was. A backfill would have to invent the one fact this table exists to record, and the delete path would then issue DELETEs against the wrong person's calendar. One writer or none. No .references() — the legacy FKs on availability_overrides are frozen, not a pattern to copy.
pushEvent/deleteEvent existed and nothing invoked them. google-export.ts
is the wiring, and it owns the three things the primitives cannot decide:
whose calendar (the lead in inspection_inspectors, via getInspectionRoster
— never inspections.inspector_id), which instant, and create-vs-update
(calendar_external_links, so a reschedule MOVES the entry already on the
inspector's phone).
Added to the provider contract: patchEvent, an exported
CalendarPushEventInput, and a timeZone that actually reaches Google —
the instant fixes the moment, the zone fixes how the entry renders.
ExternalEventGoneError lets a hand-deleted event be repaired by a fresh
create instead of failing forever.
Production holds no rows with a non-NULL scheduled_start_ms, so the
fallback rung is the one that runs: an HH:MM suffix on inspections.date
read in the tenant zone. A bare civil date is skipped as NO_START_TIME.
A wrong time on someone's phone is worse than an absent entry, so no
08:00 is invented.
Retires POST /api/calendar/sync-events, syncEventsToGcal and
createCalendarEvent. Not for double-creation — sync-events was id-tracked
— but for pushing every tenant event to whoever pressed the button, never
propagating a reschedule or a cancel, and guessing 30 minutes.
inspection_events.gcal_event_id is frozen -- DEAD.
Fixes a second live tz bug found on the way: booking-confirmation composed
both the calendar push and the customer .ics invite as `${date}T${time}:00Z`,
a wall clock labelled UTC. Both now read the stamped instant, and
server/services/booking joins the lint:tz SCOPE so they stay that way.
Rules 4 and 5 already shipped inside syncGoogleBusyOverrides. What was
missing is everything that needs to know WHICH event a busy block is:
rule 2 — skip events OI pushed itself. Without it the calendar
round-trips: we write an inspection to Google, read it back as busy,
and the inspector is unavailable for the job they are booked on.
rule 3 — skip instances of a recurring series. singleEvents=true
expands a weekly standup into dozens of separate busy blocks.
rule 6 — no historical backfill. Connecting a calendar must not
retroactively block already-accepted work. An event counts as new if
EITHER created or updated is at/after connected_at.
All three need the per-event externalId, so the sync route stops calling
mergeBusyIntervals — it unions ranges into anonymous blocks and throws
the id away, leaving the upsert keyed on a synthesised range string that
churns rows on every sync. mergeBusyIntervals had no callers left and is
deleted rather than kept as a second way to reduce busy blocks.
Window stays at the shipped 30 days. The plan said 90; that triples
provider cost and override churn for time inspectors are rarely booked
into. Pinned by SYNC_WINDOW_DAYS and a test, so it is a decision.
recurringEventId/created/updated now travel from the Google parser
through BusyBlock, with a wire-level test — a rule test that builds its
own literals proves the rule, not that the provider supplies what it
reads.
The button and the coming cron sweep call one function
(importBusyForConnection), so "Sync now" cannot drift from automatic sync.
The busy feed had shipped with two defects:
1. toUtcStamp composed `new Date(`${day}T${time}:00Z`)` — a wall clock
labelled UTC. An 08:00 appointment in America/New_York was published
to every subscriber as 20260601T080000Z. The correct instant is
20260601T120000Z. Not a formatting difference: subscribers were told
the inspector was busy four hours before they are. Proven by a test
asserting the instant, run against the old code first.
2. It filtered on inspections.inspector_id — the frozen legacy column —
while every assignment write goes to inspection_inspectors. The
fixture leaves the column NULL, exactly as real rows do, and a version
that reads it returns an empty calendar for an inspector with work.
Now reads the link table, lead role only.
ics.service.ts joins the lint:tz SCOPE in this commit and not before —
adding it earlier would have turned `npm run lint` red for everything
else in flight. The gate's own comment claimed every real bug lives in
its scope while this file sat outside it.
New: /api/ics/inspector/:token — the inspector's own schedule WITH
property addresses. Addressed by a sealed token, never the slug: the
/inspector/ surface is unauthenticated and a slug is a name, so a
guessable URL would publish someone's daily route. Deterministic, so no
migration; the cost is that revocation is secret-wide, stated at the
definition.
IcsSubscribePanel replaces the ICS prose in ScheduleLinksPanel with the
three feeds, each labelled with its audience and marked when the link is
private. ManageOthersPicker extracted so settings-schedule.tsx went DOWN
(437 -> 411), and the ics-links route lives in its own file so
calendar.ts went down too (502 -> 471) rather than through its ratchet.
Chrome walkthrough, light and dark, caught one more: the endpoint
composed absolute URLs with getBaseUrl(c), which behind the in-process
API mount resolves to the API worker's host — the copied link read
http://127.0.0.1:8787/... and was dead on arrival. It returns paths now
and the browser supplies its own origin. Verified end to end: valid
token 200 VCALENDAR, tampered token 404.
The cron half calls importBusyForConnection — the same function the
"Sync now" button calls. A sweep with its own copy of the import would
drift, and synced-automatically would quietly become a different feature
from synced-when-I-ask.
last_sync_error is the only new column (last_synced_at already exists as
last_sync_at). It earns its place because the freshness badge cannot tell
"nothing changed" from "nobody has reached Google in three days" — both
look like an old timestamp. The usual cause is a revoked token and the
inspector is the only one who can fix it, so the reason is surfaced in
the connect panel next to the reconnect button.
Two invariants the tests pin by deletion:
- a FAILED sync must not advance last_sync_at. That column vouches for
data we hold, and a failed attempt refreshed nothing; the badge stays
stale AND gains a reason, which is the honest pair.
- a SUCCESSFUL sync must clear last_sync_error, or a recovered
connection keeps prompting for the failure it already survived.
Stalest-first with a per-tick cap is the fairness mechanism: it spreads
work across tenants without a per-tenant loop, because a tenant swept
this tick sorts to the back of the next one. Both are pinned by tests.
No new router. The plan called for calendar-sync.ts, but POST
/api/calendar/sync already triggers a sync and /status already reports
connection state — a second router would have been a second way to do
one thing, which is the failure this whole phase keeps working around.
The sweep body lives in lib/calendar/sync-sweep.ts and scheduled.ts gains
ten lines.
settings-schedule.tsx crossed its cap again on the way, so the loader's
envelope-unwrapping moved to lib/settings/calendar-section.server.ts;
the route is 369 lines now, down from 437 when this batch started.
Extends the existing calendar-connect project rather than adding a spec file — one file is one playwright project here, and a new one would have meant a new project for four tests. The Google push and import are NOT asserted here: exercising them means calling Google, and the provider interface is exactly where they are stubbed in tests/unit/calendar. What a real worker adds is the part mocks cannot vouch for — that the sealed schedule token actually opens against the running crypto, that a tampered one is indistinguishable from a missing one (404, not 403), that ics-links hands back paths rather than the in-process API host, and that status carries the freshness pair the panel branches on. 29 passed, 2 skipped: the OAuth redirect needs GOOGLE_CLIENT_ID, and the busy feed is slug-addressed while the seed admin has no slug. Both name their blocker.
…ot run Pre-commit runs a subset; these only surface in the push-time chain, and three of the four are real: - tenant-scope: the booking-confirmation read of `inspections` filtered by id alone. The id does arrive from a tenant-scoped path, but a by-id-only read is a cross-tenant vector the moment anyone reuses the helper. Filtered, not baselined. - status-literals: google-export compared `status === 'cancelled'` instead of INSPECTION_STATUS.CANCELLED. - knip: getLinksByEntityIds had exactly one caller — its own test. That is the habit this whole phase exists to undo (a server primitive built and never called), so it is deleted rather than baselined. PushSkipReason unexported; it is reachable through PushOutcome. - i18n-glossary: the new es-419 copy used tú forms. This catalog is formal usted.
cmd.tenant.update carries adminPasswordHash on password-change commands, and BOTH parking paths stored the message: the raw string on a parse failure and JSON.stringify(env) on an unknown type/version. So a malformed password-change command wrote an admin credential into parked_cmd_events -- a table nothing pruned, no erasure rule covered, and no PII heuristic flagged, because `envelope` and `reason` look like nothing. The row now holds a fingerprint: type, dataschema, command id, tenantseq, byte count, a SHA-256 of the exact bytes, and (on a parse failure) the names of the envelope fields that failed validation. Those answer the only question the table exists for -- portal and core disagree about a command shape -- and none of them is the payload. The fields are an allow-list read through primitive type guards, so a field added to `data` later is dropped because nothing reads it, not because someone remembered to name it. Rows parked before this change still hold whatever they held, so a data-only migration clears the payload out of them while keeping id/reason/received_at. Production had 0 parked rows; this closes the exposure rather than reporting it closed for new rows only. parked_cmd_events is also registered in ERASURE_OUT_OF_SCOPE with the history named, because an entry that only says "no PII here" invites restoring raw parking as a debugging convenience. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
audit_logs.metadata is a free-form JSON blob written with no redaction, absent from the erasure manifest, and invisible to the PII heuristic -- the same gap portal closed in audit_logs.details after counsel ruled that retaining such a column through an erasure is an incomplete DSAR. OI applied neither half of that ruling. Real content today includes recipient emails, an SMS recipient phone, and the property address on four inspection events. Write side: both insert sites now redact. The primary filter is on the VALUE -- a string that IS an email, a phone or an IP is removed wherever it appears and whatever the key is called, so a field added later is caught by what it holds rather than by someone having named it. A short key list covers the identifiers that have no detectable value shape (address, client/contact/recipient/signer name); it is knowingly incomplete, which is why the manifest rule and not the redactor is what makes the column safe to keep. Deliberately NOT portal's key list: matching `name`, `token` and a bare `ip` here would redact the tag/template/rating-system names that ARE the audit value of half these events, the rotation forensics in previousTokenHash, and every key containing the letters "ip" (zipPrefixes, description). Erasure side: a manifest rule plus the shared ANONYMIZE_AUDIT_PII SET the orchestrator executes, so historical rows and prose no pattern can see are scrubbed wholesale on a DSAR. The structured event -- action, entity_type, entity_id -- survives, which is what the row exists for. ip_address stays: it is the staff-action security trail, already declared out of scope. The orchestrator's repeated inspection-id lookup is memoized in the same pass; three steps now need it, and the file is at its size cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185QebqzFLviQKtnEjLkC3H
OI InspectorHub#271 Task 1. An assessment written after the implementation is a justification for it, so this one is written first and is allowed to come out negative. It does, in part. Purpose and necessity pass for the narrow question ("was the deliverable received"), with no marketing or profiling purpose claimed -- which is why the engagement-analytics features are absent rather than merely unbuilt. The balancing test splits. It passes for a counter that records what the server actually observed. It FAILS for the shape that resolves every open to the primary report: the public renderer has no report identity today (app/routes.ts:49 is keyed on the inspection id, public-report.ts:80 documents the param as "Inspection id.", and report-view-props.ts:51 sets `reportId = data.inspectionId ?? ""`), so attributing an open to a specific deliverable manufactures a false statement about an identified person, for every open, by design. Section 3.4(b) says so and offers the honest alternative: key the row on the order and say so. The eight conditions in section 4 are conditions, not reassurances. Condition 3 is currently unmet, so the feature is not yet covered by its own assessment. Also recorded: no Art. 21 objection mechanism is designed, and the accuracy of the scanner filter is a heuristic the inspector-facing UI has to compensate for. Public repo because self-hosters run the same code and are controllers in their own right.
… example erasure-freetext-pii Task 3. The gate is green today (31 rules, 48 out-of-scope declarations) and that sentence is easy to read as "erasure covers the schema". It means only that no column whose NAME the gate was told to look for is unruled. The worked example is current, not hypothetical. `address` is absent from PII_HEURISTIC (check-erasure-manifest.mjs:52), so the gate never asks about inspections.property_address (schema/inspection/core.ts:13) or its nine geocoded siblings -- and `inspections` has no manifest rule, no out-of-scope entry, and none of runErasure's fourteen steps. Two tables away reports.title IS declared and IS executed. The failure mode is not an under-report. runErasure returns status:'completed' whenever no step threw (erasure-orchestrator.ts:453), writes it to erasure_log, and admin.service.ts:227 hands it straight to the caller. A gap nobody declared is indistinguishable from a gap that does not exist, and the accountability log records it as done. Also corrected: the manifest's justification for the reports.title rule says the title is human-written free text carrying the address. It is not -- there is no API that edits it, and it is machine-written from the service catalogue. The rule that got written is the one whose justification someone imagined. Deliberately NOT fixed. The address ruling is a compliance decision awaiting a human, and widening the regex first would red the gate on twelve columns and invite twelve reasonless out-of-scope entries -- which is worse than the gap. The document says it is open and unowned, and names the twelve. Held up as the behaviour to copy: users.service_origin_address, declared out of scope although the heuristic never asked.
important-new
changed the title
This was referenced Aug 7, 2026
Closed
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.
78 commits. Seven migrations (
0040–0046), all additive except one data-only cleanup.Payments — inspector pay splits with a settings surface; booking deposits at three tiers (company default / per service / per booking) collected through Stripe and applied to the invoice without unlocking the report; a cancellation ladder with an attestation gate, and the product's first partial-refund writer.
Scheduling — three routing strategies that return a decision with a named reason rather than an id, so a strategy that cannot apply says why instead of silently degrading; geographic anchors (company coordinates, per-inspector service origins, address capture on the public booking form); two-way Google Calendar sync and per-inspector iCal feeds.
Security and retention — the command dead-letter table stored raw message payloads, and a message can carry an admin password hash. Parked rows now hold an allow-listed fingerprint instead, and pre-existing rows are cleared by a data-only migration. Audit metadata is redacted at write and scrubbed on erasure.
Three live timezone bugs fixed — the inspector iCal feed, the calendar push and the customer
.icsinvite each composed a wall-clock time as UTC, publishing an 08:00 America/New_York appointment as 08:00Z.lint:tzcould not see any of those files; its scope now covers them.Deliberately not included: Apple CalDAV (untestable here), the per-booking deposit override (its only host has one line of file-size headroom), and the erasure ruling on
inspections.property_address(a compliance decision, documented indocs/compliance/erasure-heuristic-limits.mdrather than decided unilaterally).🤖 Generated with Claude Code