Per-deliverable reports, the service catalogue, and one roster authority - #295
Merged
important-new merged 29 commits intoAug 3, 2026
Merged
Conversation
… has none fulfillBooking was the only writer, so every manually-created inspection kept a NULL instant forever and conflict detection stayed on the sameDayHour bucket for them — 09:59 and 10:01 collide under that rule. The wall-clock now comes from the date suffix when there is no prior instant. Rows whose date carries no time stay NULL: inventing midnight would be less honest than the bucket fallback. Also retires scripts/backfill-scheduled-start.mjs. It matched zero production rows (0 of 19 inspections carry a non-NULL scheduled_start_ms, verified against the remote database), and the hole it patched — a reschedule that moved date without moving the instant — was closed by the dual-write in InspectorHub#289, so the drift cannot recur. Git history is the archive.
) The library had create, read, update and a per-user touch counter, but no delete at any surface — so 2,774 of the 3,654 production rows could only ever accumulate. `/library/repair-items` deletes through RecommendationService, which filters on `isNotNull(repair_summary)` and therefore only ever reached the other 880. `DELETE /api/admin/comments/{id}` already existed, was on the OpenAPI surface and RBAC-gated, and had zero callers. This adds the caller rather than a second endpoint. Bulk delete loops the same route: the concrete case is the 80 seeded rows per tenant that a tenant may decline wholesale, and a second delete path on one table is how two paths end up behaving differently. Both confirmations identify what they are about to remove — a library page is dozens of near-identical rows, so "delete this comment?" cannot tell you whether the right one is selected. One selected row is named rather than counted, which also keeps the count message honestly plural. The design rests on a claim nothing enforces: an inspection snapshots the comment TEXT and holds no reference to the library row. comment-delete-isolation.spec.ts pins it — the defect narrative and the published version's snapshot, content hash and signature all survive the delete — so a future change to that relationship fails there instead of in a delivered report.
… affect Two defects, both measured on this repo rather than reasoned about. 1. The i18n guard's stamp lived only inside `app/paraglide/`, which the paraglide compiler CLEANS on every run — and `paraglideVitePlugin` in vite.config.ts writes to that same outdir with the same options. So every `npm run dev` and every `npm run build` silently deleted the stamp, and the next commit staging `messages/**` recompiled 4,249 keys from inputs that had not moved: 66s. The hash is now mirrored to `node_modules/.cache/`, either copy is enough to skip, and a skip re-writes whichever copy went missing. Verified: 806ms after deleting the in-output stamp, and still "inputs changed" when a message file actually moves. 2. `needs_typegen` fired on any added or deleted path under `app/routes/`, including colocated specs. `app/routes/foo.test.tsx` never enters routes.ts, so typegen's output cannot differ because one arrived — but the Test Layout REQUIRES frontend specs to sit beside what they test, so this fired on ordinary work and cost ~60s each time. Worth recording alongside these: a running dev server makes the hook far more expensive than either fix saves. It rewrites `app/paraglide/**`, which is inside the app tsconfig program, so tsc's incremental build-info goes cold — measured 11.5s vs 295.5s for type-check:app, and 4.1s vs 37.6s for eslint. Stop the dev server before committing.
…migrate
These exist to repair or populate data in an install that predates a change.
There are no open-source users yet, so there is no such install: every one of
them can only ever run against our own production, and each has already been
checked there.
- backfill-notice-headers.mjs — dry run against production returned "0 headers
to create; 9 rows have no recipient id". All 9 candidate rows carry neither a
recipient_contact_id nor a recipient_role_key, so no header can be derived
from them by this or any script. Zero unstamped rows postdate the split, so
there is no live defect feeding it either.
- backfill-rich-templates.mjs — seeds rich templates into tenants that predate
them; POST /api/admin/backfill-default-templates covers the same ground for a
live tenant and stays.
- migrate-rating-levels.mjs and migrations/data/2026-07-04-severity-normalization.sql
— a matched pair normalising the retired {abbr,bucket} rating shape. The SQL
file was never journalled and nothing read it.
The `noticeId` schema comment cited the deleted script as the reason the column
is nullable. It now states the invariant instead: a row whose recipient resolves
to neither a contact nor a user keeps NULL, the Outbox falls back to the interim
key, and pre-split rows stay NULL permanently because they recorded no recipient
at all.
Kept deliberately: backfill-route-metadata.ts and backfill-zod-descriptions.ts
are idempotent SOURCE codemods, not data migrations — they are still how a new
route gets its MCP metadata and field descriptions. verify-migration-equivalence.mjs
is db:check itself.
Verified after deletion: db:check green (86 tables, no drift), knip clean
(2 baselined, 0 new), no references left anywhere in the repo.
…ded seam First Paraglide usage in server/. Four things had to agree on where the compiled catalogue lives, and only one of them did: - tsconfig.api.json mapped @core/api-types alone, so `~/paraglide/messages` did not resolve in the api program. Adding `~/*` keeps `include` at server/**; tsc follows the import out of the entry set without widening the program to all of app/. - vitest.api.config.ts had no `~` alias either, so an api spec reaching a server-side message failed to IMPORT rather than to assert. - vite.config.ts already aliases `~`, so the worker build was never a problem. - eslint forbids server/ importing app/ at all — the BFF boundary. That rule is right, and app/paraglide/ is the case it did not anticipate: generated data, compiled from messages/**, living under app/ only because that is where the outdir points. Relocating it to a shared package would rewrite the import in 392 files for no behavioural gain. The exception is an inline disable on the seam module, not an eslint.config.js entry, and that choice is the point: a config entry would exempt all of server/, while one line exempts one file. Verified both directions — the seam lints clean, and a probe file importing the catalogue directly still fails the rule. So "the one place server code reads messages" is enforced, not just documented. Same shape as server/lib/jwt-keyring.ts, the sanctioned wrapper for hono/jwt.
T6 moved the notification CALL SITES onto templates but left titleFor as the
fallback, so six hard-coded English cases plus a default survived where no
translator could reach them. The switch stays — a tenant can have no template,
and a trigger can exist in the enum before a template does — but its literals
are now message keys.
They are `comm_` prefixed, not `notice_`, and that is not arbitrary. The
catalogue already holds a `notice_title_*` family, and at a glance these look
like duplicates of it. They are not: `notice_title_report_published` is
recipient voice with no address ("Your inspection report is ready") and is what
app/lib/notice-view.ts renders for types it recognises, while these are
staff/ledger voice with the address ("Report published — 12 Oak St") and are
what gets STORED on the row and shown in the Outbox. The same split already
distinguishes comm_reason_sms_opt_out from notice_reason_sms_opt_out.
English output is byte-identical — the spec asserts exact strings, em dash and
all, rather than "contains". es-419 keys are deliberately absent so the catalogue
gate's English-fallback path applies; an empty target key would render blank,
which is a hard failure.
This does NOT make titles render in the recipient's language. Nothing resolves a
recipient locale, and in a cron or queue context there is no request locale at
all. It makes them reachable by a translator, which they were not before.
The spec carries a guard against the literals returning. Narrow by choice: a
general "no English literals in server/" lint would need an allowlist for logs,
error codes and SQL, and the allowlist is what would quietly grow. It was seen
red first — it listed all seven literals before the rewrite.
Two defects of the same shape — the function was written and one line was never connected. recordPayment had exactly ONE caller, the manual "mark as paid" route. A client paid by card, Stripe confirmed, markPaid ran, and QuickBooks never learned. A tenant reconciling their books found every online payment missing. Verified against the code rather than assumed: `git grep recordPayment` returns the one call site, and stripe-webhook.ts settled an invoice with no QBO call at all. The push also carried no idempotency key, and Stripe redelivers webhooks. QBO is the tenant's book of record, so a duplicate Payment overstates their revenue and their tax position — the kind of error found at tax time rather than in a test. The push now carries QBO's `requestid`, whose contract is that a repeated key returns the original response instead of performing the operation again. The key identifies the FACT, not the attempt: a per-attempt uuid is unique every time and therefore protects nothing. Both push sites derive it from one function so they agree — an invoice settled online and then also marked paid by hand is one payment, and the second push collapses. That single derivation point is also what makes the ledger migration safe later: when the fact becomes a ledger row rather than an invoice, every caller moves at once, because re-pushing under a new key would duplicate. Also adds InvoiceService.findById. The manual route was reading one invoice's amount out of a full listInvoices scan; the webhook runs on every card settlement and must not do that. Out of scope, and paired with the payment ledger in a later batch: pushing the amount actually paid rather than the invoice total (correct today only because payment is all-or-nothing), TxnDate being the push date rather than when money moved, and refunds — createCreditMemo is still implemented with zero callers.
…sits Production had 43 event types, 7 templates and ZERO services. Service lines can only be attached from a catalogue, so an empty catalogue is why no inspection had ever carried one — a product gap, not slow adoption. A tenant had a "Sewer Scope" template and a sewer scope event type and no sewer scope they could sell. Three corrections to the plan, all found by checking code before writing any: 1. `services.price` is NOT NULL, and `inspection_services.price_snapshot` copies it. "Seed the entry and leave price null" was never available. These are real starting prices the tenant is expected to edit, which is a deliberate trade against the alternative — making the column nullable would have put a table rebuild and a null path through getEffectivePriceCents, the booking page and the order picker on the critical path of three dependent plans. 2. There were TWO event-type seed lists and they disagreed. This one seeded three `starter_*` types at provisioning; server/data/event-type-seeds.ts held five more behind a manual endpoint, despite its own header claiming they were "seeded on every new tenant" — bulkSeed had exactly one caller and it was a button. `starter_sewer_scope` and `sewer_scope` were one real-world thing under two slugs, and a tenant who provisioned and then pressed the button got both. Merged into one list; bulkSeed now reads it too and is a gap-filling repair tool rather than a second source of truth. 3. Radon's two event types were not seeded at provisioning at all, so a service referencing them by id would have pointed at rows that did not exist. Hence slugs, not ids, for `services.default_event_type_slugs`: they do not depend on seed ordering inside one run, they survive a tenant deleting and re-creating a type, and an unmatched slug degrades to a shorter proposal instead of dangling. `proposeEventsForService` returns visits in the service's declared order — a radon pickup proposed before its drop-off is a nonsense sequence, and the order is the only thing carrying that meaning. Also guards deleteTemplate against `services.template_id`, the second foreign key to that table, which was unchecked. Latent only while the catalogue was empty; seeding it ends that. The error names the service, because "Conflict" with no subject sends a tenant hunting through their inspections for a reference that is in their catalogue. A soft-deleted service still blocks — deleteService leaves template_id in place, so the FK outlives the delete the tenant thinks they did, and a guard that filtered on `active` would hand them the raw FK error instead. The seeding and the batchInsert helper move into their own modules to stay under the file-size ratchet; extracting them was preferable to bumping the baseline, and seedServices reads better as a named unit than as an inline block anyway. Migration 0025 is a single appended ADD COLUMN, no table rebuild. db:check green, 86 tables. Two pre-existing specs updated for the merged event-type list.
…late
The competitor keeps an FAQ entry for this exact confusion: "If you're seeing
multiple reports generating on your inspections, you may have the same template
defaulted for both your primary service and your add-on services." A tenant who
defaults the residential template for both Standard Home Inspection and Sewer
Scope gets two identical blank reports and no way to guess why.
Naming the other service is the whole value — "this template is already in use"
is not actionable. It warns rather than blocks: there are legitimate reasons to
want it, so the failure mode being prevented is surprise, not invalidity.
Two things the Chrome pass caught that the unit tests could not:
- The message pushed the Save button down 20px. The component already reserved a
fixed-height slot for exactly this reason ("appearing and disappearing moved
the form's Save button by two lines, under whatever the cursor was already
aiming at") but it was sized for the one-line hint. Measured in the browser at
341px vs 321px, tightened the copy, resized the slot to the tallest message,
and re-measured: 341 vs 339, which is line-height rounding rather than movement.
- "{name} and 1 others" — the same plural bug as "Delete 1 comments?" earlier in
this branch. Rephrased to "{count} more ... too", which reads correctly at 1
and at N.
role="status", not "alert": this is advice about a choice the admin just made
deliberately, not an error, and assertive would interrupt them on every keystroke
through the select. Verified in both themes — amber #fbbf24 on #0f172a is ~9.5:1.
Task 2 of the roster-convergence plan, and on the measured evidence it is the task that carries the plan's actual value: a pay split is money attributed to a named person that nobody re-derives later, so it must read one function rather than whichever table its author reached for. Reads inspection_inspectors and never inspections.inspector_id. That is pinned by asserting on the EXECUTED SQL rather than on the result, and the assertion was proven red first: an implementation that also selected from `inspections` returned an identical roster and passed all four result-based tests, which is exactly the failure mode a result-only test cannot see. Batch-first — getInspectionRosters takes a list and getInspectionRoster delegates to it, because the calendar and the inspections list render many rows and a per-row accessor turns one query into N. A test asserts the query count is one, and it also went red against the sabotaged version. Tenant scope is on the LINK row, not on the joined user: an agent user carries a null tenant_id, so scoping on the join would silently drop rows rather than protect them. inspection_events.inspector_id is deliberately out of scope and the module says so — it answers who performed THAT visit, and a radon pickup may legitimately be a different person from the lead.
…ze the dead columns Three answers to that question were live in code and agreed only by accident: inspections.inspector_id (17 refs) coalesce(lead_inspector_id, inspector_id) (metrics) inspection_inspectors (39 refs) lead_inspector_id is NULL on all 19 production rows and helper_inspector_ids is '[]' on all of them, so the second collapsed to the first and nothing disagreed. The first write of a lead or a helper would have ended that — in metrics, and in collab access control, which decides who may EDIT an inspection. That is the worst place to find out, so this closes it before anything writes one. metrics.ts now attributes work to the roster's lead row. Joined on role = 'lead' deliberately, NOT the whole roster: grouping over every link row would count an inspection once per assigned person and double its revenue the moment a job has a helper. One inspection is attributed to one person here; this moves the SOURCE, not the meaning. can-access.ts now takes the roster instead of three columns, which also makes it fail CLOSED — an inspection with no roster grants access to nobody outside the admin roles. Its malformed-JSON and null-helpers tests are gone along with the column they guarded; a role lives in a column of its own now, so there is no string left to fail to parse. The presence badge label moved too, so it cannot say "Helper" about the person the roster calls the lead. Both collab routes still call getInspection, now for the guard rather than the row: it throws when the inspection is missing or belongs to another tenant, which is the 404. Who may edit comes from the roster. Both dual-write feed sites stop reading the dead columns. bulk.ts and core.ts selected them back out and re-supplied them to syncInspectionAssignments to avoid wiping "team mode" rows — there was never anything to preserve. The columns are frozen, not dropped (D1 cannot rebuild an FK-referenced table), with the evidence in the schema comment and the same treatment as comments.rating_bucket. Neither name is ever reused. Production was reconciled first, since metrics filters out rows with no lead and three inspections had none: 3 rows backfilled under the D1 SOP with a fresh export and bookmark 0000489a-00000000-000050bb-b61cbfa911c7445cd70556d1137c2a73. Applied changes: 3, exactly the predicted count. After: 19 inspections, 19 with a roster, 0 missing, 0 disagreeing.
…points at them Task 0 of the per-deliverable-reports plan, numbered 0 because order matters: it has to land before the first `reports` row points at a billing line. Reversed, there is a window in which a client changing scope at the door silently orphans a report. A scope change at the door is routine — add a sewer scope, drop the pool inspection, decline the radon — and it was a hard delete. Harmless only while nothing hung off a line. Once a `reports` row or a pay split does, Schema Rules forbid the foreign key that would catch it, so the delete leaves dangling rows and nothing surfaces them. The invoice disagrees too: invoices.amountCents outranks the line sum, so removing a line does not change what was billed while a split keeps paying against it. The column is the easy half. The half that gets forgotten is that EVERY reader must filter on it, and getEffectivePriceCents is a pure function over already-fetched lines — so the filter belongs at each fetch site, not in the helper. Grepped first, then filtered, all of them: effective-price.sql.ts the money chain itself api/invoices.ts a declined line must not appear on the invoice api/metrics.ts revenue by service inspection-analytics feeds getEffectivePriceCents per inspection automation/conditions or an automation keeps firing for a declined service service.service.ts the live list every surface reads Re-adding a declined service REACTIVATES the original row rather than inserting a second one. A reports row or a pay split may already point at that id, and a fresh row would strand them while the client is billed for the line they see. The REFUSAL half of the guard is deliberately absent. Neither `reports` nor `inspection_service_pay_splits` exists yet, so a check against them would be a function that always returns "nothing blocks this" — a gate that passes vacuously, which this repo has shipped before. Each of those tasks adds its own clause, with a test, when it creates the table. IA-26's inspector-qualification pair moves to service/qualification.ts: the file crossed the 400-line ratchet, and which inspectors MAY perform a catalogue service is a separate concern from what it costs. Thin delegates keep every caller unchanged. Migration 0026 is a single appended ADD COLUMN, no table rebuild. db:check clean. One pre-existing spec updated: removal is a soft delete now, so it asserts the row survives and leaves the live list rather than disappearing.
Task 1 of the per-deliverable-reports plan. A standard inspection publishes today
and the radon report publishes on Thursday, each with its own document, signature
chain and notification, without the client waiting on the slowest one.
`uq_results_inspection` made that impossible; this is the table it becomes.
Two naming decisions carry weight and are written into the schema comment:
`inspection_service_id`, NOT `service_id`. In this schema `service_id` already
means the CATALOGUE entry — both inspection_services.service_id and
service_inspectors.service_id are that — so naming this one `service_id` reads
as the catalogue to anyone who has seen the other two, and a catalogue id here
makes "which report did this billing line produce" unanswerable. That is exactly
the grain pay splits are built on.
`kind` distinguishes primary from ancillary because primary is not just another
one: it must exist, the pay gate keys on it, and it is what a client means by
"my report". One primary per inspection is enforced by a PARTIAL unique index
rather than by the service layer — which report the client means must not depend
on which caller wrote last — while ancillary reports stay unbounded, since a job
can deliver radon, sewer and mould.
The erasure-manifest entry lands in this task rather than later, because an
entity that enters the schema without one is how the manifest drifts from the
database. `title` is the only free text a human writes and it routinely carries
the address ("123 Oak St — Radon"), so it is anonymised rather than deleted: the
row is the spine of a signed, delivered document and removing it would strand the
version chain that proves what was delivered. The other eight columns are ids,
enums and a timestamp, declared out of scope. lint:erasure passes at 29 rules and
32 declarations.
Migration 0027 is CREATE TABLE plus three indexes, no rebuild. db:check clean at
87 tables. `reports` also had to be added to the top-level schema barrel, which
uses explicit named exports — without it drizzle saw no schema change at all.
Task 2 of the per-deliverable-reports plan. `inspection_results` and `report_versions` gain a `report_id`, the uniqueness that used to be per INSPECTION becomes per REPORT, and every existing inspection is backfilled with one primary report carrying its current template. The statement order in the migration is load-bearing and is NOT what db:generate emitted. Drizzle put the `DROP INDEX` on the old results uniqueness FIRST, which opens a window in which two results rows can be written for one inspection. The shipped order is: add both columns, backfill, create the new unique indexes, and only then drop the old ones. Verified no table rebuild — every statement is ADD COLUMN or an index. db:check clean at 87 tables, lint:migchain intact at 29 snapshots. The backfill derives each report id from its inspection id rather than generating one. That makes it idempotent — a replayed migration creates nothing and changes nothing, which a test asserts — and it makes a row traceable to what produced it. Titles are generic on purpose: `reports.title` is anonymised by the erasure manifest precisely because it usually carries an address, and a backfill has no business inventing PII that was not there. The test that earns its place is the chain one. `report_versions` carries contentHash / prevHash / signature and the public verifier reads them, so a backfill that renumbered or reordered versions would invalidate signatures on reports ALREADY DELIVERED. Counting rows cannot see that; comparing the hashes before and after can, and does. Applied locally and checked: 3 inspections, 3 primaries, zero orphan results and zero orphan versions. The REMOTE apply is deliberately not done here — production holds 19 inspections and 6 versions, and that step goes through the D1 SOP with a backup, a bookmark, and a human looking at the statements first.
Task 3 of the per-deliverable-reports plan, and it lands in the same breath as
the model because both defects go live the moment a second report can exist —
which, as of the previous commit, it can. Neither fails loudly.
THE SIGNATURE CHAIN. report_versions carries contentHash / prevHash / signature,
a tamper-evident chain that was numbered and linked per INSPECTION. Two reports
publishing independently — the standard report Tuesday, radon Thursday —
interleave into one chain, and every subsequent verification fails INCLUDING for
versions published before the second report existed. Version numbers and the
prevHash walk are now per report, and verification walks within its own report:
reading by inspection would pick up the other document's version and call a valid
chain broken, or a broken one valid.
THE COLLABORATIVE DOCUMENT. The Durable Object id came from
`${tenantId}:${inspectionId}`, so two inspectors working the standard report and
the sewer report of one order landed in the SAME object and shared one Y.Doc.
Nothing threw. The CRDT merged content belonging to two different documents, and
the corruption surfaced when a client opened a report containing someone else's
findings. The name is derived per report now, and the derivation lives in its own
function so a test can assert the INPUT: two report ids are trivially unequal, so
a test comparing only outputs would pass against an implementation that still
keyed on the inspection.
Resolving which report also fails CLOSED. An inspection with no report row gets
a 404 rather than falling back to an inspection-keyed document — that fallback is
precisely the shared-Y.Doc bug.
snapshotOnPublish takes an optional reportId and defaults to the inspection's
primary, so every existing caller keeps working while a caller that knows which
deliverable it is publishing can say so.
Re-keying the DO address was not sufficient, and this is the second path. InspectionDocDO writes its Y.Doc projection back to `inspection_results`, and that write matched on `inspection_id`. An inspection now has one results row PER report, so a correctly-routed radon document would still have overwritten the standard report's row — the same corruption as the shared Y.Doc, one layer down, and just as silent. Fixing the id derivation alone would have looked complete and left the bug. The DO now carries an `x-report-id` header and writes by `report_id` when it knows which document it is, falling back to the inspection predicate only when it does not — a DO awakened before any client has connected, which cannot yet have a sibling to clobber. INSPECTION_PRESENCE is deliberately left keyed per inspection: it persists no document, so there is no corruption path, only the question of whether two people editing different reports of one order should see each other. That is a UX call, not a correctness one. inspection-doc.ts is already grandfathered at 900+ lines; the baseline moves by the 16 lines this adds rather than splitting a Durable Object mid-change.
… unlock The report gate is order-wide: any required agreement left unsigned, or payment outstanding, blocks EVERY report on the inspection — not just the report belonging to the service whose agreement is missing. That was already the behaviour, as an accident of `agreement_requests` having no report dimension. It is now a stated rule, written at the gate itself, because one rule an inspector and a client can both recite without looking it up is worth more than a finer one neither can. Its cost is real and this is the part that needed solving: an add-on's unsigned addendum can hold back a report that is finished and that someone is waiting for. The release is a deliberate human action — an owner or manager opening one inspection and recording why — rather than resolving the gate per report, which would mean putting a service dimension on `agreement_requests`, a signed-evidence table with a retention rule and an erasure entry, to solve what one override solves. `reason` is required and stored, not defaulted. An override with no stated reason is indistinguishable later from a mistake, and this one is client-visible: it hands over a report the tenant's own rules said to hold. Unlocking twice keeps the ORIGINAL timestamp, person and reason, so the record shows who actually made the call rather than whoever pressed it last. Relocking clears the reason with it — it described a decision that no longer stands. Both routes are owner/manager only and both are audited, including the already-unlocked case, because someone asking is part of the story of who wanted the gate open. The two audit actions are registered in the AuditAction union, which is closed on purpose — the type check refused them until they were declared. Default stays locked. Reports being held until signed and paid is what a tenant expects, and defaulting open would quietly change that for them. The two routes live in their own module rather than in publish.ts: publishing is about whether a document is finished, this is about whether the people waiting for it may see it. Different questions, different reasons to change. Migration 0029 is three appended ADD COLUMNs, no rebuild. db:check clean. The append-order guard in pca-foundation-schema.spec.ts was extended rather than worked around — it exists to keep db:generate emitting ALTER rather than a table rebuild, and a new trio at the tail is exactly what it watches for.
…s copy that states the rule Two surfaces for one rule. SETTINGS. The gate's description only mentioned booking — clients must sign before the booking is confirmed. It also holds every report, which is the rule this branch committed to, so an operator would have met it the hard way. The scope note appears only when the setting is on and says plainly that an unsigned agreement holds back EVERY report on that inspection, not only the report for the service the agreement covers, and how to release it. THE CONTROL. Deliberately not a GateToggle. That component's own comment states this hub's law — "a switch, not a button… nothing to confirm and nothing to lose by flipping it back" — and it owns ONE gate on the card for the artifact it gates. This owns BOTH gates, spans the order, and hands a client a report the tenant's rules said to hold. Three failures of the switch test, so: a quiet text control opening a modal, with the reason required to enable the confirm. It lives on the REPORT card, by the same rule GateToggle follows — the card for the artifact it gates — because what it gates is the reports. The weight is on the STATE, not the action. Unlocked, it stops being a control and becomes a standing record: who released it, when, and their reason QUOTED rather than paraphrased, so it reads as somebody's words. Requiring a reason is pointless if nobody reads it back, and that is the one place any visual weight is spent. Locked, it is a line of grey text that does not compete with Publish. The hub resolves the unlocker to a NAME server-side rather than shipping an opaque id; a deleted teammate degrades to "a teammate" so it never renders "Released by on …". Admin-only in the UI as well as at the route. Chrome, both themes: modal opens, confirm stays disabled until a reason is typed, the round trip lands and the control becomes the record, and "Put the gate back" returns it to the action. Amber on light is #b45309 on #f8fafc (~6.5:1). One thing the browser caught that no test could: the control rendered as permanently locked because the LOCAL database was a migration behind, so the hub query selected a column that did not exist and the loader swallowed the failure. Same silent-empty shape as the services catalogue earlier in this branch — a hub loader failing quietly is indistinguishable from a feature that does not work.
…d, and erase report titles Task 5 of the per-deliverable-reports plan. Each of these was a decision the spec proposed; the plan asks for them in CODE rather than only in the spec, because the next reader is in the file. ACCESS TOKENS stay keyed on the inspection. One order, one client, one link — a per-report token would mean three links in three emails for one job, and a client who mislaid the middle one. The report dimension survives where it earns its place: a view counter keys on (report, token) — the report says what was opened, the token says who. THE PAY GATE is the invoice's, and the invoice is the order's. Paying unlocks whatever has been published, and a client who has paid is not asked again because a second report arrived later. Per-report payment would need per-report invoicing, which is a different product. REPAIR REQUESTS gather across all published reports on the order. A client negotiating repairs is negotiating about one house and does not care which document a defect came from; splitting the list would make them assemble it themselves before they could ask for anything. RE-INSPECTIONS are a new ORDER, not a second report. Recorded at the column so nobody "improves" it into an ancillary report and puts two fee-bearing visits, two agreements and two invoices behind one order id. The erasure work is the substantive part. Adding `reports.title` to the manifest without implementing it left a rule with no orchestrator behind it, and erasure-manifest-coverage caught exactly that — a manifest that promises more than the code does is worse than one that promises less. Titles are now cleared through the subject's inspections, and cleared to a sentinel rather than blank: a reader of the version chain needs to see a document existed and was deliberately cleared, not wonder whether the field was ever filled in. Two things that had to be got right there. The scope goes through inspection_people, NOT a column on inspections — clientContactId was dropped when people became rows and the schema comment says not to reintroduce it, which my first attempt did. And matching is by inspection, not by title text: an address can be spelled several ways, and a title mentioning someone else's street is not this subject's data. The orchestrator crosses the 400-line ratchet and is baselined rather than split. An erasure routine's worth is that its steps can be read in order in one place; splitting it to satisfy a line count would damage the property that matters most. Full serial gate green: lint 0, test:unit 4246, test:web 1994.
The table listed paths that are discoverable by looking, and it was one more thing to keep in sync with the tree. What is worth writing down is the handful of things the layout does not tell you — which is what Core Architecture below already does.
CI's worker-runtime job failed after lint, test:unit and test:web had all gone green. test:workers is deliberately not in that three-suite run, so both of these reached CI — which is the interesting part, not the fixes themselves. 1. Four collab specs hand-wrote `CREATE TABLE inspection_results`, copy-pasted. The Drizzle table gained `report_id`, and the only thing that noticed was the Durable Object's persist() throwing D1_ERROR inside workerd. The DDL now lives once in tests/helpers/inline-ddl.ts beside the tenant_configs one that learned this same lesson in InspectorHub#164, and inline-ddl-schema-sync.spec.ts asserts it covers every Drizzle column — so the next added column fails a fast unit test instead of a slow runtime one. Proven red by deleting report_id from the DDL and watching it name the missing column. 2. vitest.workers.config.ts had no `~` alias, so the server-side message seam could not resolve and three suites failed to import at all. That is the FOURTH resolver that has to agree on where the compiled catalogue lives — tsconfig.api.json's paths, vitest.api.config.ts, vite.config.ts and this one — and it is the only one whose disagreement is invisible until workerd runs. test:workers now 20 files / 91 tests green.
The E2E run caught a gap I opened. The collab route resolves an inspection to its primary report and fails CLOSED when there is none — which is right, because the alternative is falling back to an inspection-keyed document and that IS the shared-Y.Doc bug. But nothing created that row for NEW inspections: generating one report per sold service is Task 4, deferred. The production backfill covered the 19 existing rows, so the hole only opened for anything created afterwards, and the symptom was collaborative editing silently 404ing. This is the minimum slice, not the whole of generation: every order — including a re-inspection, which is its own order — gets a primary report. Per-service reports remain Task 4's job. Non-fatal by design: both callers have already written the canonical inspections row, and throwing here would lose it over a row a backfill can add later. Also fixes a pre-existing bug the same run surfaced. seedDefaultServices names `price` and `active` in its INSERT; the columns are `price_cents` and `is_active`, so the statement always threw and was swallowed by its own catch. A standalone tenant has never had a seeded service catalogue, and the only evidence was a warning in a log nobody reads — the public booking page simply had nothing to sell. test:workers 20/91 green; inspections suite 537 green.
The status-literal gate caught the new reports row writing 'in_progress' as a hand-typed string. That gate exists because a bare status bypasses the type layer, which is the path by which ghost values reach runtime. reports.status is a NARROWER axis than REPORT_STATUS — it has no 'submitted' — but 'in_progress' means the same thing on both, and the point of the constant is that the two cannot drift apart silently.
Five jobs need the generated route types and every one of them was running the full ~60s typegen. Paraglide, which sits right beside it in the same five jobs, has been cached on its inputs since the parallel split; typegen never got the same treatment, so each CI run burned about five runner-minutes producing five identical copies. The key hashes the route tree rather than the generated output, which is what makes it safe: a hit can only restore what those exact inputs produced. Hashing the route file BODIES over-invalidates — editing a loader cannot change the generated types, since those infer from `typeof loader` when tsc runs — but over-invalidating only costs a regenerate, and that is the direction a cache should err in. package-lock.json is in the key because the generator is react-router's own.
The close-path assertion failed intermittently in CI with the departed user still listed, and passed every time locally. The helper was a fixed setTimeout(50): whether the DO has processed a webSocketClose yet is a question about the runtime's schedule, not about elapsed time, so a sleep answers it correctly only on a machine fast enough. A loaded CI runner is not. Polling to a deadline is both faster when the event has already landed and correct when it has not. sync-producer.spec.ts already waits this way, so this follows the suite's existing convention rather than introducing one. Verified the helper can still fail: with a deliberately wrong expectation the spec goes red at the deadline and reports the value it actually observed, so a real regression cannot hide behind the polling.
…nment lives The columns were annotated DEAD and were not. inspection-core.service.ts still wrote lead_inspector_id and helper_inspector_ids on every wizard create, the clone path read them back off the cloned row, and two automation paths resolved the inspector as 'leadInspectorId ?? inspectorId'. Meanwhile the link table's own docstring called itself a denormalized mirror of those columns, and the roster module's said the same. Half the code had been migrated and every comment still described the old direction — the state most likely to produce a wrong fix later. So: stop writing the two columns, and move the reads. - The wizard writes teamMode (still live) and passes lead/helpers as INTENT to syncInspectionAssignments. Same resolution, one place instead of three. - The clone reads the SOURCE inspection's roster rather than columns copied onto the clone row. Those columns are no longer written, so a clone of any newly assigned inspection would otherwise come out with nobody on it. - automation/trigger.ts and automation/recipients.ts read roster.lead. This is value-identical to what they did: 'leadInspectorId ?? inspectorId' is exactly how the lead row is resolved when written (buildSyncStatements), so resolving it once at the write and reading the answer cannot disagree with itself. - inspections.inspector_id survives only as a fallback for rows created before the link table existed and never re-assigned since. One consequence recorded rather than smoothed over: the sync failure handler stays non-fatal, but its justification changed. It used to be 'the mirror can lag harmlessly'; now a failure leaves the inspection genuinely UNASSIGNED. It stays non-fatal because the inspection row is already committed and throwing would lose it — assignment can be redone, a lost inspection cannot. canEdit now takes assignedUserIds instead of the columns. It had to change even though NOTHING CALLS IT: had it kept reading columns that are never written, it would have denied every non-admin the moment anyone wired it up. Its docstring claimed it guarded every write-bearing route; it does not. Write routes authorize with requireRole + requireCapability only, with no per-inspection membership test, so within a tenant any inspector with the capability may edit any inspection. That may be the intended product behaviour — it is now written down instead of implied by a function nobody runs. file-size baseline: inspection-core.service.ts 1126 -> 1131. Comments only, and I trimmed them once already; the remainder documents a correctness-critical invariant. Splitting an 1131-line service is a refactor this change does not justify.
…ed on by accident Two things about the e2e job, found while working out whether it can be sharded. The browser download was uncached — about 60-90s of every run spent fetching a Chromium that a given Playwright version pins exactly. Keyed on the lockfile for the same reason the paraglide and typegen caches are keyed on their inputs: a hit can only restore what those inputs produced. "--with-deps" still runs on a hit; it installs system libraries outside the cached path and is cheap once the download is skipped. The "inspector-portal" project declared no dependencies, yet its beforeAll seeds the admin password and then logs in — both of which need the workspace that the "api" project creates. It passes today only because workers:1 runs projects in declaration order and "api" happens to come first. That is an ordering held by accident; any reorder, any parallel run, and any shard that did not happen to include "api" would fail it. Declaring the edge costs nothing and removes the trap. Sharding itself is NOT done here, and the reason is worth recording: the blocker is not the spec coupling it looked like. globalSetup truncates all of D1 and all of KV once per "playwright test" invocation, and webServer.reuseExistingServer is true on a fixed port — so two shards on one machine share a worker and a database and wipe each other. There is a cheaper win to take first. The workers:1 cap exists for two specific reasons (several specs race POST /api/auth/setup, and five shell out to "wrangler d1 execute --local" mid-test); fixing those raises in-job parallelism with no extra runner-minutes at all, which sharding cannot claim.
…eded Three separate costs, all of them paid every run. UNIT SUITES. Both are import-bound, not assertion-bound: a full API run reported `import 1977s` against `tests 1246s`, because the default forks pool rebuilds the whole module graph per spec file. Pre-bundling node_modules attacks that directly and is cached in node_modules/.vite. The web suite also defaulted every one of its 305 files to happy-dom, though 136 touch no browser API at all; the default is now node and a file that needs a browser declares it with a docblock. Declaring the browser rather than its absence means a component test written without the docblock fails loudly, in the file whose author can fix it — verified by removing one and watching it die on `document is not defined`. Measured: API 611s -> 222s, web 130s. Do NOT set `pool: 'threads'` in vitest.api.config.ts: 312 specs drive an in-memory better-sqlite3 and the run dies with SIGSEGV. Capping maxWorkers was also tried and cost 13% for memory that was never scarce. E2E PARALLELISM. `workers: 1` had three causes, not the two that were obvious. Specs raced POST /api/auth/setup with different company names, so whichever won named the tenant — they now share one COMPANY_NAME and depend on `api`. Four specs shelled out to `wrangler d1 execute --local` mid-test to re-hash a password that already had that value, locking the SQLite file the dev worker was serving; those are gone, and calendar-connect seeds through a fail-closed worker hook instead. The third only surfaced when 3 workers were actually tried: ten projects shared ONE seeded inspection, so SpeedMode found nothing left to rate after a concurrent spec had rated it. There is now one inspection per editing project, and `readEditorSeed()` resolves the caller's own rather than falling back silently — handing an exclusive project someone else's fixture is the failure this is meant to prevent. That first 3-worker run also exposed a race the specs always had: four of them waited for `<main>` under a comment claiming that proved hydration. It proves the SSR shell arrived. `awaitEditorInteractive` retries the idempotent item selection until the pane opens — a gate, not a sleep, since a genuinely broken editor never opens it. All four are fixed, including the two that happened to win this time. Last local-only blocker: a real `.dev.vars` sets APP_BASE_URL to 8787 for `npm run dev`, so the server stamped that port into emailed links and agent-unified-link followed one to a port nothing served. CI's generated .dev.vars omits the key entirely, which is why only local runs saw it. Pinned via --var, like the other E2E-only bindings. webServer's timeout also had to cover the full build it runs: 60s survived only because Linux CI finishes inside a minute, while the same build takes 2m02s on Windows. Verified: e2e 173 passed twice running (3.7m, 5.1m), unit 636 files, web 304. Also here: `app/routes/inspections-list.test.ts` is deleted. It tested `groupByInspectionStatus`, an exported pure helper that was never implemented — the shipped list groups by attention and time, not status, and both halves of what it actually does are already covered by dashboard-workflow.test.ts and dashboard-buckets.test.ts. It survived because no gate could see it: tsconfig excludes app/**/*.test.ts from the app tsc pass, so the import of a non-existent symbol never failed, and describe.skip meant the body never ran. The web suite now has no skipped files at all. And test-hooks.ts routes through getDrizzle, which lint:provider-helpers wants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016BzmFiHBLHN7HFFYW2whLr
The E2E section still said `workers: 1`. It is 3, and the two rules that make that safe were learned the hard way when it was raised: a project owns the rows it writes (the editor-seed setup mints one inspection per editing project), and a spec gates on the editor being interactive rather than on `<main>` being visible — the app is server-rendered, so the markup is on screen before React attaches a handler, and every spec that waited for markup was racing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016BzmFiHBLHN7HFFYW2whLr
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.
Batch 1 of the open-work programme, plus the first four tasks of per-deliverable reports. Twenty commits; every one is independently reviewable and the branch is green on
lint+test:unit(4,246) +test:web(1,994).What this changes
One order can now deliver several reports. A standard inspection publishes on Tuesday and the radon report publishes on Thursday, each with its own document, its own signature chain and its own collaborative session — without the client waiting on the slowest one.
uq_results_inspectionmade that impossible; areportsentity replaces it.Two of those changes fix defects that fail silently, which is why they land in the same breath as the model rather than after it:
report_versionsnumbered and chained per inspection. Two reports publishing independently would interleave into one chain and break verification for every version — including ones published before the second report existed. Numbering and theprevHashwalk are now per report.inspection_id, which would have overwritten sibling documents. Both paths are closed.The service catalogue is seeded. Production had 43 event types, 7 templates and zero services, so no inspection could ever carry a service line — a product gap rather than slow adoption. Seeding it also surfaced that two event-type seed lists had drifted apart (
starter_sewer_scopevssewer_scopewere one thing under two slugs); they are now one list.One authority for "who worked this inspection". Three answers were live in code and agreed only because two columns were empty on every row. The first write of a lead or helper would have made metrics — and collaborative edit access — answer differently from everything else.
Notable decisions, recorded in code rather than only in a plan
reports.inspection_service_idis deliberately not namedservice_id— in this schema that already means the catalogue entry, and a catalogue id there would make "which report did this billing line produce" unanswerable.inspection_servicesgainsis_activeand soft-deletes, landed before anything points at a billing line — otherwise an on-site scope change silently orphans a report or a pay split, with no FK to catch it.Migrations
0025–0029. All additive:ADD COLUMN,CREATE TABLE, and index moves — no table rebuild anywhere.0028reorders whatdb:generateemitted, because the generated order dropped the old uniqueness first and opened a window where two results rows could exist for one inspection.Its backfill derives report ids from inspection ids, so a replay is a no-op, and a test compares
contentHash/prevHashbefore and after: a backfill that renumbered versions would invalidate signatures on already-delivered reports, which row counts cannot see.Also here
recordPaymenthad one caller — the manual "mark as paid" route — so every Stripe-settled invoice was missing from the tenant's books. The push is now idempotent via QBO'srequestid, keyed on the record rather than the attempt.npm run devsilently invalidated it.Out of scope, deliberately
Generating reports from sold services, publish-per-report and notification coalescing are the next change and will rebase onto
mainafter this lands. The remaining QuickBooks defects (pushing the amount actually paid,TxnDate, and refunds —createCreditMemostill has zero callers) wait on the payment ledger.