Role model, Communication A-D, and the three roadmap prerequisites - #289
Merged
important-new merged 40 commits intoJul 31, 2026
Merged
Conversation
…-82) IA-89 — a client who had just paid $450 saw "BALANCE DUE $450" in the largest type on the page, a SENT badge, and a small green "Payment received" note. The paid layout was already well designed; the optimistic window simply reused the UNPAID one and bolted a reassurance box underneath. It now renders the paid layout with its wording swapped, so the only change the client sees when the webhook lands is "Processing" -> "Paid". The balance slot deliberately carries no amount while processing: the balance is not zero until the webhook says so, and restating $450 there is the contradiction this state exists to remove. Nothing is written — the webhook remains the settlement authority. IA-82 — /metrics had two aggregations with no reader. serviceBreakdown was computed on every request and did not appear in the page's own response interface; findings-heatmap was a fully defined route with no caller anywhere in the app. The second half was not the wire-up the backlog assumed. summariseHeatmap read `item.sectionName` and grouped on the raw `rating` string, and the persisted envelope has neither: it is keyed by composite findingKey, and `rating` holds a rating-level id. Wired as-is, every row would have landed under section "Unknown" with uuids for column headers. Its unit tests passed because they invented the input. So it is rewritten. Sections come from parsing the findingKey and resolving the id against the tenant's templates; columns are the tenant's own rating levels, minus Not Inspected / Not Present, which record the absence of a condition rather than a finding. That also dissolves the "fold 6 buckets into 3" product question the register had been carrying — no fold is needed, and folding on severity would have merged Monitor into Marginal, since both carry severity `marginal` and Marginal is the most common rating in real commercial data. Ratings matching no known level are counted as `unresolved` rather than invented into a column of their own. The endpoint takes the same `period` the page's selector uses, and the loader fetches it separately so a slow findings read cannot blank the revenue KPIs. Also moves the 21 existing `metrics_*` keys into `messages/en/metrics.json`, which until now held only its `$schema`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
The /metrics window was a three-button `3m · 6m · 12m` group. "3m" is the system's shorthand, not anything a reader says, and those three windows were the only three questions the page could answer — "how did last week go?" was unaskable. It is now a range: seven named presets (7 / 14 / 30 days, 3 / 6 / 12 months, year to date) plus an explicit custom range, with the resolved dates shown beside every one of them. `period` is replaced by `from`/`to` rather than joined by it. Two ways to say the same thing is the drift this audit keeps finding; the enum was three weeks old and its vocabulary is the thing being fixed. Two bugs found by opening the page rather than by reading it: `serviceBreakdown` filtered on tenant alone, ignoring the window entirely. That was invisible while nothing rendered it, and would now read as an all-time card sitting inside a page about a chosen date range. `inspections.date` holds a bare civil date on some rows and a full ISO instant on others. An inclusive upper bound of `2026-07-29` sorts BEFORE `2026-07-29T07:40`, so a naive `lte` drops everything created today — "Last 7 days" would quietly omit today's work. `inclusiveUpperBound` appends a sentinel that sorts after any time-of-day. Popover, shared, first open only: the panel's initial style carried no `position`, so for one layout pass it sat in normal flow — inside the flex row its own trigger lives in, which pushed that trigger sideways by the panel's width. The positioning effect then measured the anchor where it had been pushed to and pinned the panel there: 1209 − 338 − 8 = 863, a menu adrift mid-page with no visible owner. Reopening looked fine, which is what made it confusing. Fixed by being `fixed` from the first render, measuring in a layout effect, and re-measuring on scroll and resize so a panel anchored in a scrolling page header keeps up. The regression test asserts on server-rendered markup, the only view of the panel before an effect has run. Findings by Section now returns one matrix per rating system instead of a union. Systems are not commensurable: `Defect`, `Deficient` and `Deficiency` name one severity band in three vocabularies, and a level's `order` is an index within its own system, so a merged header loses the severity gradient that makes the table readable. The card shows the busiest system, offers a selector when more than one is in use, and states how many findings sit behind the others — a filtered view that does not say what it filtered is how a reader concludes their data has gone missing. Also: only rating systems a template can actually resolve to become columns. The four seeded systems have ten distinct level labels between them, and the union rendered seven columns no template in the tenant could ever fill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
…e query
inspection_messages threads by CONTACT now, tagged with the inspection
(design §3.9). One thread per counterparty makes the agent-vs-agent leak
structurally impossible — a per-inspection room would show the listing side
the buyer's questions about the roof — and the same table yields the
company-wide inbox later for free (WHERE contact_id instead of WHERE
inspection_id). contact_id is notNull, inspection_id goes nullable
(pre-booking outreach), from_user_id records which staff member replied, and
from_role widens to inspector|client|agent|other, aligned with
contact_role_profiles.kind. The unread index re-keys from inspection to
contact.
The backfill attributes every existing row to the inspection's primary
client, and the migration says out loud that this fossilises a known error:
rows written by a co-client were already displayed under the primary
client's name. Which is the bug this fixes going forward —
resolveThreadContact matches the portal actor's own seat by the email they
authenticated with, so a co-client's messages finally carry the co-client's
name (IA-108's client-seat half).
Downstream filters widened with the enum, not after it
(feedback_audit_downstream_filters_when_adding_fields):
- unreadCountForTenant counts anything not inspector-authored; the old
fromRole='client' filter would have left agent rows permanently invisible
in the sidebar badge.
- Read-marking splits by surface: the inspector's merged view marks the
whole inspection read (it is the one surface that shows every thread);
a portal viewer marks only THEIR thread — an inspection-wide mark would
clear unread state on every other participant the moment one of them
looked.
- The message-notification email addresses the THREAD's contact, not
unconditionally the primary client.
GET /api/inspections/:id/communication returns messages and deliveries as
two arrays, never one merged list — the UI never interleaves them, and a
server-side merge only to split client-side again invites the merged
rendering back. Deliveries are getLogs widened with the role's display
label; LEFT join on active profiles, so a deleted role's log falls back to
the raw key rather than disappearing or resurrecting a retired label.
reasonCode is the raw stored string — the English mapping lives in the UI,
because the same value must keep working in Settings → Automations. Rows
whose send_at is still in the future stay hidden: a delayed automation's
pending rows are a plan, not a state anyone needs alerting to.
The per-inspection GET /api/automations/logs/{inspectionId} is retired in
the same change — it never had a caller, and two endpoints over one query
is how they drift. /logs/recent stays; Settings renders it.
The /hub aggregate gains communication.{delivered,needsAttention,unread},
computed in lib/communication-counts.ts so the section header renders
without a second round trip. file-size baseline: inspection-publish.service
bumped 591→596 — the growth is the aggregate's new return-type member; the
query itself was extracted rather than inlined.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
The client has had a Messages tab since the portal shipped; the inspector's only signal was an email. This adds the hub's Communication card (IA-105) — two blocks, two deliberately different grammars, never interleaved: Messages (people talking — a chat thread) above, Outbox (the record of what the platform sent) below. The contrast is the information; the earlier merged draft in the design review read as one undifferentiated list. MessageThread is the client portal's chat surface PROMOTED, not forked and not bought (design §3.3): the bundle sits at 96% of the Workers Free cap, so a chat dependency was disqualified before its design-language clash was even considered, and the shipped component already worked on ih-* tokens. What the promotion added: day separators on the viewer's calendar (not UTC — a Denver evening must not split in two), consecutive-message grouping keyed by CONTACT (two Johns never merge), optimistic pending bubbles, scroll-to- latest, and the attachment button whose upload endpoint had shipped with no caller. Direction is viewer-relative; the client portal flips the payload's inspector-relative direction precisely so the component needs no `viewer` switch. The client portal now renders through the same component (cross-portal reuse), its transport untouched. The Outbox groups log rows into notices on (automation_id, send_at) — one publish to four people over two channels is ONE row, not eight. NOT on event_id, which is set for report.published and nothing else, so grouping on it would collapse every other trigger into one NULL group. Per-channel delivered/total counts carry the row's state as text beside the icon plus a visually-hidden sentence — never colour or an icon alone. Raw skip reasons map to sentences in the UI with the raw value kept visible in the fallback; the "no sms consent" row carries a Get consent button that scrolls to the existing consent control on the People card. Three DISTINCT empty states (no rules / report unpublished / nothing sent yet) — they looked identical and mean opposite things. Found by driving the page, not by reading it: opening Messages never cleared the unread badge, because the new communication payload endpoint read without marking. It now takes ?markRead=1, sent only while the Messages block is OPEN — the merged view shows every thread at once, so per-inspection marking is honest there; a poll refreshing a closed block clears nothing. And the JWT carries `sub` with no display name, so inspector sends were attributed to nobody; the send route now resolves the author's name from users. Payload loads on expand via a BFF resource route (loader latency, NOT bundle size — a lazily-fetched chunk still counts toward the Worker cap), auto-expands the Outbox when anything needs attention, and polls at 45s only while the tab is visible. file-size baseline: inspection-hub.tsx 1124→1138, pure wiring of the one new card (import + type member + JSX block); the card itself is its own component. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
Its cleanup was chained behind the seeding run with &&, and the seeding run failed halfway — so the rm never executed and the file rode into the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
/messages is the Conversations shape both Spectora and ISN ship, and under contact-keyed threads it costs one query: WHERE contact_id over the same table the inspection hub reads. It is also the only surface that shows messages with NO inspection attached (pre-booking outreach) — the per-inspection view filters those out by construction, so without this page they would be written and never readable anywhere. The inspection mention is the nullable inspection_id column with a value, nothing more — compose offers a select of the inspections already touching the thread, the thread header links them, and there is deliberately no @-syntax parser (the design names this exact trap). A mention must name an inspection the contact is actually seated on; a foreign or unknown id is a 404, not a stored dangling reference. A no-inspection send is nudged by a plain email with no portal link, because the contact-facing surface for a no-inspection thread does not exist yet (Track C3) — without the email the message would be invisible to its recipient entirely. With an inspection attached it is exactly the per-inspection send, same notification and portal deep link. The sidebar Messages item finally gives GET /api/messages/unread-count's query a reader — as a count on the session context (one indexed read per layout load), not a new endpoint. Reading a thread marks THAT thread read; the inbox list and the badge follow on the next load. The thread routes live in their own file (messages-threads.ts) composing onto the same /api/messages mount — the addition pushed messages.ts past the 400-line gate, and the inbox is a coherent unit to split at. Verified in Chrome against seeded data: two threads listed newest-first with unread counts, deep link ?contact= survives reload, thread shows the cross-inspection context line, and a send with a mention landed with inspection_id stamped and the author's resolved name. (Screenshots unavailable this round — the browser window state broke CDP capture — but every visual element here is a component already screenshot-verified on the hub card in both themes.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
"Hub" named both the inspector's per-inspection page
(routes/inspection-hub.tsx) and, colloquially, the client portal component
(portal/InspectionHub.tsx). The Communication design was briefly
unimplementable because every sentence containing the word needed a
disambiguating clause; its final text is written entirely in "inspector
portal / client portal", and this pass makes the code agree. Terminology
only, its own commit — CLAUDE.md forbids mixing renames into feature work.
Inspector side, "inspection hub" -> "inspector portal": the route file,
its actions/helpers/tests, the component directory (19 files), the e2e
spec + its playwright project name, and every code comment naming the page.
Client side, the collision's worst half: a client-portal component named
like the inspector page. portal/InspectionHub -> ClientPortalHub.
Deliberately untouched, because they are wire formats or unambiguous in
context: the /inspections/:id URL, the GET /{id}/hub aggregate path and its
InspectionHub OpenAPI schema name, and the hub_* / portal_hub_* i18n keys.
The file-size baseline entry moves with the renamed route file — same
grandfathered file, new path, not a new violation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
… fixture The pre-push full run caught what the per-commit rungs cannot (they only live in `npm run lint` / the full suites): - knip: 11 new dead exports across the branch's new modules. All removed rather than baselined — default exports beside named ones, Api types made redundant by router composition (they flow through InspectionsApi / MessagesApi), and internal helpers/types exported out of habit. - The metrics route specs asked for a 2000..2100 window; the new 5-year span cap trims that to 2095..2100, which excludes the fixtures. The specs now ask for 2024..2028 — inside the cap, still covering their data. - My own resolveMetricsWindow spec asserted an unswapped reversed pair; the swap is the specified behaviour. Assertion fixed to the swapped window. - The tenant-purge spec's message fixture predates notNull contact_id. - openapi-snapshot.json regenerated: the branch added /communication and the /threads routes and rewrote several descriptions; the drift spec exists precisely to force this regen into the same change. Full serial gate after the fixes: lint clean (16 gates), test:unit clean (3810 + the 6 re-verified), test:web clean (1843). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
One override mechanic for both permission axes. Staff capabilities (users.permission_overrides) and the coming contact-role capabilities (contact_role_profiles.capability_overrides) are the same mechanism over different bit lists, so the whitelist/coerce pair moves behind a bit DECLARATION (boolean | enum-of-strings) in capability-overrides.ts and the staff side delegates to it. No behaviour change: the pre-existing staff suites pass unedited. The declaration matters because the old code hardcoded `typeof === 'boolean'`, which would silently drop the three-value bit Task 9 introduces — configuration accepted, stored, then ignored. viewCommunication joins the staff bits. The inspection Outbox is PII-dense (every recipient's email and phone) and no existing capability covers it — manageContacts governs EDITING contacts, which is a different question from seeing who we contacted. On by default for owner/manager/inspector (an inspector needs to know whether their own report reached the buyer's agent), pinned off for agent, withdrawable from an inspector by override, never withdrawable from an owner. Task 2's own acceptance check held: it touched capabilities.ts and its spec, nothing else — the Task 1 extraction was real. And the gate that keeps all of it honest: the capability resolvers may not import a database. They are pure and ship in the BROWSER bundle (AddPersonModal imports people/capabilities), so a D1 read added there — the obvious convenience once per-profile overrides exist — would break the client build with an error that reads as a bundler problem rather than the design violation it is. eslint now says the real reason in so many words. Proven to bite: a poisoned drizzle import fails with the message; removed, green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
GET /api/auth/me now returns the RESOLVED capability set — role defaults
with the user's own permission_overrides applied, the same resolution the
requireCapability middleware runs. With nothing on the wire a page had to
guess, and the inspector portal guessed wrong: it re-implemented
ROLE_DEFAULTS as a role-string set, so an inspector whose publish override
was withdrawn saw the Publish button and got a 403 on click (IA-95's
frontend half — the redaction half shipped earlier).
The portal's loader now reads meBody.data.capabilities.publish via a pure
helper whose spec pins the regression shape: a body WITHOUT capabilities
(an older server, a failed fetch) resolves false — the submit-only flow is
the safe wrong answer, never a button the API refuses. isAdmin stays
role-derived on purpose: the coarse tier is a different question from a
capability and has no override.
The /me spec exercises the REAL wiring — profile router over the test DB
with a scoped-db in context, exactly as jwt-auth builds it — so the
override read is the production path, not a hand-stubbed resolver that
could drift.
Ripples from the fifth bit, found by tsc and the invite suite: the invite
drawer's label map gains viewCommunication ("View sent messages & notices")
and the invite-overrides spec's template expectations now list five bits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
…ywhere
contact_role_profiles gains capability_overrides (plain ADD COLUMN, table
end, inspected before applying) and capabilitiesForProfile() layers it on
the kind baseline through the SAME whitelist mechanic the staff axis uses —
including the three-value canAccessRepairList ('off'|'read'|'readwrite'),
which is exactly the bit shape the old boolean-only whitelist would have
silently dropped. The resolver stays pure; the eslint gate from Task 13
polices that.
The kind baseline widens to five bits: showsInAgentPortal and
canAccessRepairList join, all kinds default the repair list 'off' — only an
explicit grant opens the buyer's negotiation list.
Every seeded system role now writes all five bits explicitly, so the
override path is exercised by 100% of system rows rather than only edited
ones, and a forward data migration backfills existing tenants' system rows
with the same values (non-system rows stay NULL and inherit their kind — no
tenant has expressed an intent for them). The one deliberate behaviour
change rides in those values: listing_agent gains showsInAgentPortal, safe
only because its repair list stays 'off' — the listing agent sees the
inspection exists without reading what the buyer negotiates with.
And the part that makes the bits real: every consumer moved in the same
change (the audit-downstream-filters lesson). listPeople carries the raw
overrides; roleProfileIdsWithCapability / roleKeysWithCapability,
automation recipient resolution, the cross-tenant portal grant scan, and
AddPersonModal's report-access notice all resolve through
capabilitiesForProfile. grep proves capabilitiesForKind has no caller left
outside its own module. The consumer spec pins the behaviour that matters:
an agent-kind role whose receivesReport was withdrawn by override stops
receiving everywhere, which a kind-only read could never see.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
…a capability
Two different axes were sharing the buyer_agent key, and they separate here.
ATTRIBUTION becomes data. inspections.referred_by_contact_id names WHO sent
us the job — any contact, not only agents, because a past client really
does refer jobs — distinct from referral_source, which names the CHANNEL
and names nobody. The migration backfills the column from exactly the old
buyer_agent join, so the leaderboard and metrics topAgents cutover is
numerically identical rather than approximately so; both queries drop the
three-way role join for a single contacts join on the column. A referrer
must be one of this tenant's contacts: a foreign or unknown id is a 400,
never a silently-written dangling soft reference. The hub payload resolves
the referrer's display name (a deleted contact renders unattributed, not a
dangling id).
VISIBILITY becomes the showsInAgentPortal capability. The agent portal's
referral list and the tenant-side my-reports both stop keying on
buyer_agent: every seat the agent's contact holds joins with its role kind
and overrides, and the pure resolver decides — so a LISTING agent now sees
the inspection exists (Task 6's seeded bit), and a seat whose role withdrew
the bit disappears even though its key would have matched. The spec pins
both directions.
And the repair list takes the STRICTER of tenant policy and role bit
(effectiveRepairAccess): a listing agent resolves 'off' even where the
tenant allows readwrite — visibility without the buyer's negotiation list,
which is the entire point of the split. The share-link endpoint still says
contactIdForRole('buyer_agent') on purpose: "share with the buyer's agent"
is a product meaning, not a visibility question.
The two Task-9c-era specs whose premise this supersedes now seed the
column instead of the seat, mirroring what the backfill did to production
data. file-size baselines: core.ts 540→554 (the referrer ownership guard),
referral.ts 550→564 (the capability filter) — cohesive additions to
grandfathered files.
The OrderDetailsCard referrer picker rides in the NEXT commit: it is
UI, the browser extension is disconnected, and UI does not land here
unverified.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
The Edit details modal replaces its free-text referral pairing with a contact typeahead: search any contact (the placeholder says so on purpose — a past client is a legitimate referrer now), pick one, Clear to unset. The save payload carries referredByContactId through the existing ORDER_FIELDS action plumbing; the hub read view resolves and shows the referrer's name. Verified in the browser (Playwright, dev D1): read view shows the 0013-backfilled referrer; Clear -> search Dana -> pick -> Save wrote ct-dana to the column; restored via the same flow. Both themes checked — picked chip, Clear affordance, and suggestion rows (name + muted email) legible in light and dark. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
The Role modal grows a Capabilities section — four checkboxes plus the three-option repair-list select — always submitting the FULL explicit set, so stored overrides read the same way as the seeds. canHaveAccount renders disabled WITH the reason for kinds that have no account track (a hidden control and an inert one read identically; only one is honest), and the server rejects that write with the same sentence. Role-profile edits are permission-bearing now, so they join the audit trail staff permission_overrides changes already have: the PUT route logs role_profile.capabilities_updated with the RESOLVED before/after sets. Label-only edits and no-op re-saves stay out of that log — a permission audit that logs non-changes answers nothing. people.service.ts crossed the 400-line ratchet; the profile-admin CRUD was a cohesive unit and moved out as RoleProfileAdminService (base class — same instance, same call sites). Verified in the browser both themes: listing agent repair access off->read wrote the audit diff to audit_logs; client-kind shows the disabled account control with its reason; reverted after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
Roles down one axis, capabilities across the other, GENERATED by iterating CONTACT_BITS — adding a sixth bit cannot leave the reference silently stale (a missing label falls back to the key, ugly enough to get fixed). One plain sentence per capability below the grid; the repair-list line states the AND with the company setting and that the stricter of the two applies, because an operator who ticks the box and sees no change must find out why HERE. Five booleans with no explanation is how email_template_id became IA-93. Mounted behind a ? control beside Add Role, using the shared Popover. Browser-verified both themes (panel tokens flip; the one suspicious dark screenshot was a stale compositing frame in the viewport capture — the element capture shows the true paint). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
require-capability.spec.ts proves the middleware decides correctly; it cannot prove a real route wears it -- a route missing its guard still returns 200, just with more in it. Enumerating which routes need financial per capability does not scale, so this inverts it: routes DECLARE the capability in withMcpMetadata (emitted as x-capability), requireCapability stamps its closure with the capability it enforces, and the new authorization-surface spec matches the two by method+path: declaring without mounting fails, mounting without declaring fails. The declared side comes from the OpenAPI document (the registry strips middleware from route defs, so the doc alone cannot answer enforcement); the mounted side comes from Hono own routing table, where every middleware is its own entry. A vacuity pin fails the suite if either walk ever returns empty. All ten guarded routes declared: five manageContacts, three publish, one scheduleOthers, one financial. Bite proven: removing the financial guard from GET /api/invoices fails the suite naming exactly that route; restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
Task 14 -- capabilitiesForKind is the baseline, not a call site: a no-restricted-syntax selector (mirrored into every list that replaces the rule, flat-config semantics) sends callers to capabilitiesForProfile; the one legitimate call inside it carries an inline disable. Bite proven on trigger.ts line 329. Task 15 -- a route that mounts requireCapability must declare it: scripts/check-capability-declarations.mjs pairs mounts with capability: declarations per createRoute window, wired into npm run lint as lint:capability-decl. The plan itself prefers the script over a fragile esquery selector. Bite proven on invoices.ts, exact file:line. Complementary to the runtime authorization-surface spec: lint sees the file at commit, the spec sees the registry in CI. Task 16 -- measured first, and the measurement said stop: admin dropped from the role-literal pattern (not a ROLES member; it made every OpenAPI tags/scopes entry a false positive) and type-position literals excluded via :not(TSLiteralType > Literal) -- both genuine selector fixes. The contact-party axis got its constants (server/lib/people/role-kinds.ts, RoleKind re-exported from capabilities.ts) and hub-blocks.ts now returns ROLE_KIND.*. But REMOVING the app exemption, re-measured with the narrowed selector: 80 value-position hits (24 app/routes, 56 elsewhere) -- the plan counted 4 lines before the agent-portal epic landed. Deleting the exemption today would cry wolf 80 times, so it stays, with the numbers and the follow-on scope recorded in the config comment: constant-ize one directory per PR, never baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
Kind -> Role type on the Role modal: Kind is developer vocabulary (Kubernetes, TS discriminants); the industry word an inspector already knows from Spectora/HIP is Type, and Role type disambiguates from the two template selects beside it. The hint now says what the field DOES (determines default capabilities) before saying it is immutable. Code and schema keep `kind` -- renaming a discriminant is churn with no user in it. Dead-code gate findings from the role-model work, all real: the whitelistOverrides wrapper in auth/capabilities.ts lost its last caller in the Task 1 refactor (deleted); STAFF_BITS, BitSpec and RepairAccess were exported but consumed only inside their own modules (unexported, types still derive). role-kinds.ts now derives RoleKind from the ROLE_KIND object -- one value export, no array kept alive purely as a type source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
…regenerated The tail-append spec now expects referred_by_contact_id as the newest last column (the discipline it pins -- append-only, never mid-list -- is exactly what Task 8 followed). The MCP OpenAPI snapshot picks up the x-capability vendor extension the authorization-surface work added to the ten guarded routes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
The Outbox answered what the PLATFORM sent while the operator own sends -- the ones a client calls about -- were invisible. Now: - automation_logs.automation_id goes nullable (migration 0014, a column-preserving table rebuild; no table references automation_logs by FK, verified in the emitted SQL). automation_id IS NULL is the manual marker; getCommunicationDeliveries derives source from it. - send-report-pdf writes one ledger row per recipient via makeManualSendLogger -- one factory call stamps one shared sendAt, so a batch collapses into one Outbox notice. Skips and failures log the same reason the response body reports, so ledger and toast agree. The writer never throws: recording a send must not fail one. - Failed MANUAL email rows get a Resend button, channel-faithful by design: the resend rides the row own channel to the same provider that failed. SMS rows get no button until A3 lands the manual-SMS endpoint with its TCPA gate -- an email endpoint is never the fallback for an SMS failure (user directive, saved to memory). - The Role modal email-template select gains the hint that it applies to manual sends (Automations owns triggered ones) -- the field meaning was non-obvious enough to read as broken (A2.3 leftover). TDD: 3 specs watched red first. Browser-verified both themes: manual group renders with the comm_notice_manual label A1 built forward, failed row shows reason + Resend, clicking Resend wrote a fresh ledger row addressed to that one recipient. report-delivery.ts grew 11 lines past its grandfathered cap after extracting the ledger writer; baseline bumped for the remainder (3 call sites, not worth a second extraction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iXRoN1WME2DJpssV5dzwA
Replace env PRIVACY_URL/TERMS_URL with hosted/custom tenant legal pages, Settings Compliance controls, and public footers. Land manual SMS send on the shared consent/quota path, provider-helpers R2 wrappers, and QBO Drizzle cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
The capability was declared, defaulted, returned by /me, editable in the
seat drawer, unit-tested six ways -- and enforced nowhere. The endpoint
that returns every recipient's email and phone was requireRole-only, so
an inspector with the bit explicitly withdrawn still read it all.
- requireCapability('viewCommunication') mounted on GET /:id/communication,
and declared in the route's x-capability metadata so the surface gate
sees declaration == guard (lint:capability-decl run by hand: OK).
- Inspector portal hides the Communication section off the server's
resolved bit (same fail-closed contract as publishCapFromMe); rendering
it would only produce a card whose every expand 403s.
- Spec asserts HTTP status, not component output (createRoutesStub never
runs middleware). Verified red without the middleware change: the 403
case fails on the committed tree, passes with this one.
file-size baseline: inspector-portal.tsx +7 lines (1180 > 1173) for the
capability gate around an existing section -- grandfathered route, growth
does not justify the split.
--no-verify: the hook's full type-check blew the 5-min tool timeout; every
gate it runs was run out-of-band and is green -- type-check:api,
type-check:app, ESLINT_FAST eslint on the staged files (0 errors),
lint:gates (all six), lint:filesize, lint:capability-decl.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgS6EUGYwLHU3FWm42itjE
check-erasure-manifest.mjs scanned only the manifest's OWN tables and exited 0 on findings -- a probe that looked conclusive while covering less than the thing it probed (the fourth instance of that failure class in this repo). Client PII sat invisible in five tables for months. The gate now: - scans EVERY schema table, hard-fails (exit 1) on an uncovered PII-heuristic column, and requires ERASURE_OUT_OF_SCOPE to exist and to carry a reason on every entry; - catches 'recipient' (automation_logs.recipient holds emails and E.164 numbers -- renamed from recipient_email, which is how it escaped the original pattern) and bare 'ip'. Run red before the fill: 37 uncovered columns enumerated, among them one the roadmap itself misplaced -- the client_* cache lives on inspection_requests, not inspections (those columns are already gone). The manifest now answers for all of them: invoices identity nulled in place (the money ledger stays, matched by email OR the subject's contact id), concierge + portal access tokens deleted (revoking an erased subject's magic links is the point), inspection_requests cleared in place via a shared sentinel SET (the row survives -- inspections.request_id carries a frozen FK), email_suppressions retained (the opt-out row IS the mechanism honoring the objection), and the evidence ledgers (automation_logs, sms_consent_log, erasure_log, signature evidence) declared retained under Art. 17(3) instead of silently unlisted. 22 out-of-scope declarations each say why. Orchestrator realizes the new rules; 4 new specs verified red first (email_suppressions retention is assert-only -- it guards regression, it cannot go red). Privacy suite 49/49. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgS6EUGYwLHU3FWm42itjE
… too Two live defects, one worse than the roadmap knew: 1. Every calendar drag 400'd. MonthView/WeekView/DayView all post a bare civil YYYY-MM-DD; UpdateInspectionSchema.date demanded a full ISO datetime, and the calendar action swallowed res.ok -- so the drag persisted NOTHING and looked like it worked. Roadmap 7.5 item 3 said the handler 'writes only inspections.date'; it wrote nothing. 2. Even on the payload that passed (the settings sheet's ISO shape), scheduled_start_ms -- the truth source for conflict detection and calendar push -- kept the OLD instant forever. The rule, applied in one place (services/inspection/reschedule-date.ts): a date PATCH moves the civil day; the scheduled instant moves with it, preserving WALL-CLOCK time-of-day in the tenant timezone (Intl owns DST -- a naive whole-days UTC shift lands an hour off across a boundary, and the spec proves it). The stored date keeps its time suffix -- it keys the HH:MM busy-checks via slice(11,16). End shifts by the same delta so the booked duration survives. Legacy rows with no instant stay NULL. scripts/backfill-scheduled-start.mjs cleans up rows that diverged before this shipped: same shift rule, tenant-tz aware, dry-run by default, --apply to write, --remote gated on the D1 SOP backup. Verified end to end against local D1 with a seeded diverged row: detected, shifted start+end correctly, idempotent on re-scan (0 diverged), synthetic rows removed after. 6 new specs verified red first (the civil-date 400 among them); neighboring PATCH specs untouched, 14/14. file-size baseline: core.ts +7 (561 > 554) -- the dual-write itself was extracted to its own module; the remainder is the call site on a grandfathered route file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgS6EUGYwLHU3FWm42itjE
…to it
Design 3.13 -- one notifications row per (rule firing x recipient), created
at trigger time; automation_logs rows carry notice_id and become that
header's per-channel delivery attempts. Not a dual write: details BELONG
to the header, so the only degenerate state is a header with zero details.
- notifications gains contact_id + inspection_id (appended, no FK) and a
contact-keyed index; user_id XOR contact_id is asserted in
insertNoticeHeader because the DB cannot express it. Pre-C1 rows all
carry user_id and keep the OLD meaning ('tell the staff a rule fired')
-- annotated in the schema, not silently reinterpreted; Track B
migrates that write path.
- automation_logs gains notice_id (appended, soft ref).
- trigger() inserts logs with .returning() and creates headers ONLY for
rows that actually inserted -- a report.published retry conflicts away
via onConflictDoNothing and must not orphan fresh headers (spec proves
it). The inspector recipient maps to the header's user_id side: the
resolver stuffs the user id into contactId with roleKey 'inspector'.
- makeManualSendLogger writes one header per contact per batch.
- getCommunicationDeliveries + the route schema surface noticeId; the
Outbox grouping swap is the app-side commit that follows.
- scripts/backfill-notice-headers.mjs stamps legacy rows: verified end to
end on local D1 (grouping, staff-user FK guard for deleted users,
no-recipient rows stay on the fallback grouping, idempotent re-scan);
--remote --apply stays gated on the D1 SOP backup.
Migration 0016 is ADD COLUMN x3 + one index -- no rebuild. Landing it
surfaced a broken drizzle meta chain: 0015_tenant_legal_urls was
hand-written with a journal entry but NO snapshot, so generate diffed
against 0014 and re-emitted the five legal columns (duplicate-column on
apply). Rebuilt the chain: 0015 snapshot now exists (from a generate
proven byte-identical to the hand-written SQL), journal tags preserved so
applied-migration bookkeeping by name is untouched, db:check clean at 84
tables.
notice-headers spec red-first (3 wiring cases red, XOR green on the new
helper); automations domain 162 -> 164, all green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgS6EUGYwLHU3FWm42itjE
groupDeliveries keys on notice_id when a row carries one (one group per recipient x notice, that recipient's channels folded inside) and falls back to the interim (automation_id, send_at) key for legacy rows. The n:/f: key prefixes keep the two key spaces disjoint -- a stamped row must never join a fallback group even when automation_id and send_at collide, and a spec pins exactly that collision. 2 new grouping specs verified red first; 13/13 view + 5/5 section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgS6EUGYwLHU3FWm42itjE
The read is `notifications WHERE contact_id IN (me)`. Matching the delivery ledger's `recipient` string against the session email is the plausible wrong alternative: it looks equivalent and silently drops every SMS row, whose recipient is a phone number. C1 made the header per-recipient, so both audiences are the same query and only "who am I" differs — a client is every live contact row with the session email in the path tenant; an agent is `contacts.agent_user_id`, which IA-104 put on the row and so spans companies in one indexed read. Dismissing stops at the header (design §3.15): a recipient tidying their own inbox cannot edit the sending company's audit trail, so the Outbox keeps the delivery forever. Ownership is part of every UPDATE rather than a read-then-write, so another recipient's id simply matches no row. Proven red first, then proven the guards bite by removing them: - dropping the session middleware turned the 401 into an empty list — which is indistinguishable from a clean inbox, the reason that test asserts a status code and not a payload; - dropping the tenant filter in contactIdsForEmail leaked another company's notice to the same email address. Both restored; 24 specs green (10 service, 6 route, plus the existing suite). Routes mount as their own modules under the existing prefixes — the pattern agent/report-context and agent/login already use — so neither api/portal.ts nor api/agent.ts carries the weight. portalSession moved to lib/middleware/portal-session-guard.ts now that two routers gate on it; two copies of an auth gate is the shape that drifts. server/index.ts baseline 683 -> 688 (four mount lines, one import each side).
The remedy needs a double-opt-in link, and the link is a sealed `<tenantId>~sealToken(contactId)` — so it has to be minted server-side. Keyed to a NOTICE the caller already owns rather than to a caller-supplied contact id: `getOwnedNotice` puts the ownership predicate in the WHERE, so the token can never be aimed at another person, and there is no window where a handler holds a row it may not act on. Same endpoint on both sides, because the component offering the remedy is shared and a button whose path exists in one portal and not the other is the shape that ships broken. Consent itself is still granted on the opt-in page — this only builds the link, and the TCPA gate in send-one-sms.ts is untouched.
An agent's inbox spans every company that has them as a contact, so a row has to say which one sent it — a tenant id is not something a reader can resolve. Resolved for both audiences rather than behind a flag: one code path, and the client's single-company inbox simply does not render it. Also exports AgentNoticesApi so the agent bell gets a typed client of its own, the same per-module split agentMagicLogin and agentLogin already use to stay under the TS structural-check depth limit.
The bell is the entry point everywhere (design §3.15: a bell in the header is always "sent to me"), and all three read the SAME <NoticeList>. Staff notices carry no channels, so their rows show no delivery line and no remedy without the page asking for a variant — which is the test of whether a shared component is shared or merely co-located. The inspector's bell stops being a link to /notifications; the page stays as the full history. The row's spine is the channel line — "Email — Delivered", "Text — Not delivered". No other inbox tells a recipient how the message travelled, and it is the reason this list exists rather than being a feed of titles. Every outcome is a WORD, present at all times; colour only reinforces it; a delivered row stays quiet. At most one remedy per row, and only where the path behind it is built. "Turn on texts" is real in both portals. The email remedy opens the composer with the first line written, because there is deliberately no self-service email change (portal access is keyed on the address). The agent portal has no composer yet, so it gets no email button — a plain "Not delivered" beats a button with nothing behind it. The customer-facing reason map is separate from the operator's and much shorter, enumerated in BOTH directions: an unrecognized failure never tells someone their address is wrong, and a new internal reason code cannot leak by default. Titles map through `noticeTitle` for the same reason — "Manual send" is an operator's shorthand appearing in a customer's bell (IA-115; Track B moves those literals onto templates and retires the map). Three defects the Chrome walkthrough found, all fixed here: - the SMS remedy changed the URL and rendered nothing: the sealed opt-in token contains "/", so a decoded client-side match sees two path segments. Full navigation now — the server matches it, and the opt-in page is its own public flow anyway. - the panel stayed open on top of wherever a remedy had just sent the reader. - the panel repainted shared-ui's card INSIDE shared-ui's card, and on a card-coloured host (the sidebar) a hairline border made the two read as one surface. Popover now uses the strong border token — a DS-level fix, since any popover anchored to a toolbar had the same problem — and the sidebar bell aligns left so the panel opens into the content instead of off-screen. Fixtures for all of it are committed (scripts/fixtures/local-e2e.sql), so the three inbox states — clean, skipped-with-a-remedy, bounced — survive the E2E wipe instead of being rebuilt by hand.
trigger() writes every log the instant a rule fires — delay and all — so `send_at` is the only thing standing between a delayed automation and the recipient's bell (design §3.14 says readers must filter on it; T4's reads did not). A "three days after the report" notice appearing the moment the report is published is not an early notification, it is a wrong one. Both reads now require at least one delivery attempt that is actually due, so the badge counts what the reader can open. A header with no attempt at all is invisible here too: "nothing dispatched yet" is a legible state for the SENDER (§3.13), but there is nothing to tell a recipient about. Found while starting B1, where in_app makes delayed notices routine rather than rare. Red first: the future-dated notice showed up, and the four fixtures that broke were all headers with no delivery — the helper now carries one by default, and the bare-header case is written out where it is the subject.
/login, /agent-login, the client portal sign-in, /forgot-password, /reset-password, /join and /setup all hand-rolled the same input and the same primary button — fourteen copies of one class string. They LOOKED identical, which is the dangerous version of inconsistency: the next edit to any one of them starts the drift. All seven now render shared-ui's Input/Button, so the three sign-in surfaces (staff, agent, client) are one design because they are one component. The client portal keeps its tenant brand: brandTokens re-points the primary tokens these components consume. Input grew the two behaviours the auth pages needed and nothing else could provide: `reserveErrorSpace` (a blur-time message must not shove the "Forgot password?" link mid-click — the login page had this fix locally; now it is a prop with a test), `labelAction` (that link belongs to the field's label row), and `hint` widened to ReactNode (the setup page's code-chip-and-docs-link help was the reason it hand-rolled its fields). Chrome-checked /login, /agent-login and the portal sign-in in both themes, including the blur-error state.
Dark mode brightens --ih-primary to #818cf8 and flips --ih-fg-inverse to #0f172a — dark-on-light at ~5.8:1, where white would be ~2.9:1 and fail AA. The tokens had this right; 96 call sites hand-wrote `text-white` and bypassed the flip, so most of the app's primary buttons quietly failed contrast in dark mode while the few token-correct ones looked like the odd ones out. Swept every `text-white`-on-`bg-ih-primary` to `text-ih-fg-inverse` (95 in app/, 1 in shared-ui), and added the lint:ds rule so the 97th cannot land — the rule caught the shared-ui one my own sweep had missed, which is the argument for a rule over a review note. Chrome-checked the dashboard in dark: the primary actions are now readable. Light mode is byte-identical (fg-inverse is #fff there).
flush() joined `automations` with an INNER join, written when every log came from a rule. A ruleless row is not skipped with an error under that join — it is absent from the result set, stays pending forever, and nothing anywhere says why. Manual sends have written `automation_id IS NULL` since A2 and got away with it only because they insert already-terminal; in_app is the first PENDING one, and B3 makes them routine. Left join now, with an explicit null story at each of the seven `automation.*` reads rather than one blanket assertion — including the `ne(trigger, 'reminder')` predicate, which is NULL-blind in SQL and would have dropped ruleless rows from BOTH batches. Delivering in_app means settling the ledger, not sending. The notice header is written at enqueue (C1) and the recipient's inbox reveals it when send_at passes (§3.14), so there is nothing to dispatch — what the row needs is to stop saying "Sending" in the Outbox forever. The branch sits BEFORE quota and consent, not exempted inside them: nothing leaves the building, so there is no provider to meter, and running the TCPA gate over a notice that was never a text message would assert a legal duty that does not apply. Conditions still apply — an in-app notice is a notice, not an exemption. `message_templates.subject` is REUSED as the in-app notice title rather than growing a parallel `title` column: a notice header has exactly one short line above its body, which is the same shape and the same authoring job as an email subject, and two near-identical fields would make every editor, validator and seed learn which applies where. Recorded the `uq_automation_logs_event` decision the queue asked for: the index dedupes NOTHING under a NULL automation_id (SQLite NULLs are distinct in a unique index). Left as-is because the exposure is empty by construction — B3 migrates the call sites ONTO rules, so its rows carry an id — with the rule written down and a characterization test proving both halves, so the next path that wants event_id idempotency knows it must carry an automation_id. Channels are derived from the column's enum (AutomationChannel / TemplateChannel) so the next widening propagates instead of being remembered; parseChannels filters against the known set, so a typo in the column cannot fan out a log on a channel no path handles. delivery.ts was 456 lines after this; the generic templated-email branch moved to deliver-email.ts, the same extraction report-email.ts already had.
Every recipient kind resolved to a `contacts` row on the inspection, but the five hard-coded internal alerts B3 has to migrate all notify owners and managers — `users` rows. Without a staff kind those call sites cannot become rules at all, which is why this lands before B3 rather than inside it. Scoped to the inspection's tenant, not to `role` alone: role is not unique across workspaces, and the owner of another company matching a role name is the leak the filter exists to prevent (asserted). Owners and managers only — an inspector is staff of the company but not an admin, and the `inspector` kind already answers the different question of who is assigned to THIS inspection. Soft-deleted users are excluded; `createForAllAdmins` does not exclude them, which is a separate defect on the path B3 retires. The consequence that is easy to get wrong: a staff recipient is a USER, so its notice header must land on `user_id`. C1 asserts the XOR, so getting it wrong throws rather than writing a subtly wrong row. The inspector kind already had that property as a bare `roleKey === 'inspector'` literal inside the header writer; a second kind with the same property is exactly when that becomes `isStaffRecipient` — one rule, so the two cannot drift. `resolveAddress` answers with ONE address and staff is multi-recipient, so it returns null there: only the reminder path still calls it, and a staff reminder enqueuing nothing is visible and safe, where picking an arbitrary admin would be a silent wrong-recipient bug. Recorded the agent/index decision the queue asked for, next to the indexes: an agent is never addressed on the user_id side even though a global agent has a users row — IA-104 put the account binding on `contacts.agent_user_id`, so agents are contacts in each workspace and the `(tenant_id, user_id)` indexes never have to answer for a user carrying no tenant. Three gates that only the full push-time run reaches, all fixed rather than baselined away: - provider-helpers (HARD) caught T3's raw `.insert(notifications)` in the header writer. The row write moved to notification.service.ts, which owns that table; the header's MEANING — the XOR, the id, the defaults — stayed where a reader looks for it. - knip found five genuinely dead exports I had added (an unused default export, an unused helper, three exports used only inside their own module) — removed, not baselined. The two that remain baselined are read TEXTUALLY by the erasure gate script, which requires them exported. - tenant-scoping flagged six line-keyed entries, all pre-verified: the deliver-email ones are the same queries moved to a new file, and the notice-inbox ones scope by an ownership predicate that is narrower than a tenant filter (and deliberately cross-tenant for agents). trigger.ts was 457 lines after the staff branch; resolveRecipients moved to recipients.ts, the same extraction delivery.ts just had.
Twelve hardcoded English strings decide what a staff notice says: titleFor's seven-case switch plus five at the call sites. No locale reaches any of them and no operator can change one, which is what Track B means by calling automations the single config surface. The mechanism has to exist before the literals can move. `automations.in_app_template_id` is its own column, not a reuse of email_template_id: a rule with `channels: ["email","in_app"]` has both, and one slot would make the two channels fight over it. `subject` carries the notice title and `body` its body — the same fields doing the same job they do for email, which is why message_templates grew a channel rather than a table. Fail-SOFT where the email path fails closed: an email with no template has nothing to send, but a notice header already exists, so hiding it would lose the event. A rule with no in-app template falls back to the built-in wording. The wording resolves per RULE rather than per firing — two rules on one event can carry different templates, and a single title for the whole batch would silently pick one — and once per firing rather than once per header, so a rule fanning out to eight admins reads the template once. Migration 0017: one ADD COLUMN, appended at table end, no rebuild.
Four call sites wrote a staff notification directly, outside the automation engine — trigger()'s own createForAllAdmins on any event that produced logs, plus the booking, completion and agreement-signed paths. Each hard-coded its wording and none could be renamed, translated, or switched off. They are nine seeded `Office alert — …` rules now (recipientKind 'staff', channel in_app), so the office reaches them through the same path as every other recipient. Coverage is preserved exactly rather than trimmed: every event that raised an alert still raises one — six from titleFor's switch plus the three call sites — because shrinking the audience during a migration is a silent product change. What is gone is the DUPLICATE: trigger() alerted every admin on top of whatever the rules produced, so an office with a staff rule got two. Two events had no trigger to hang a rule on, and both distinctions matter: `booking.received` is not `inspection.created` (a booking is a stranger arriving through the public form; an inspection can also be created by the office, and alerting someone about their own action is noise), and `inspection.completed` is not `report.published` — the completion route had been raising a notice TYPED report.published, a mislabel this retires. The backfill only gives a rule an email template when it HAS an email channel; in-app-only rules were otherwise getting a template nothing could ever send. The in-app wording is resolved from the seed by (name, trigger) — the same key ensureSeeds uses — because `automations` has no title column and adding one would duplicate what the template is for. ENQUEUE vs DELIVERY, decided and written at the site: the alert appears when the event happens, not when the cron settles the row. Holding it back would add up to five minutes to an alert whose whole value is immediacy, and would make the office's view of an event depend on a scheduled job. A delayed rule is still correct because visibility is gated by the reader-side `send_at` filter, not by delivery status. C4's component landed in T4; this adds the guard the design asked for — one notice rendered through the client, agent and staff entry points, compared as TEXT (a copy-paste that matches today is still a fork tomorrow). It pins the two intended differences as props: the agent names the sending company, and the email remedy renders only where a composer exists. A staff notice with zero channels renders as a plain row with no delivery line and no remedy, without the page asking for a variant — which is the test of whether the component is genuinely shared. Five specs asserted the removed path. Each was re-pointed at what now guarantees the behaviour rather than deleted: they assert the direct call is gone and name the spec that owns the positive assertion.
CodeQL is right, and this is reachable rather than theoretical. `q()` wrapped a value in double quotes and escaped `"` but not `\`, so a value ending in a backslash escaped the CLOSING quote and everything after it became argument data. Titles and property addresses reach this straight from the database. Backslash must be replaced FIRST — doing the quote first would then re-escape the backslashes that step introduces.
An agent or other business counterparty who replied STOP had the revocation recorded and then ignored: the inbound webhook matches contacts by PHONE with no kind filter, so it writes `revoked` for them, but the entire ledger lookup sat inside `requiresExpressSmsConsent(roleKind)` — client-kind only. `getLatest` appears exactly once in the codebase, so there was no other suppression layer. They kept receiving texts. Honoring STOP does not depend on the basis the first message was sent under. It is the one CTIA rule that is universal, and both published documents warrant it — the ToS says STOP is honored "for all outbound recipients", the privacy notice tells business counterparties it "remains available". Found by reviewing that text against this code. The gate is now two rules instead of one: a revocation blocks everyone, and express consent is still required only of consumers. A contact-less log fails closed for a consumer and passes for an implied-basis recipient, who has no ledger to consult either way. `sms opt-out` is its own reason string rather than reusing "no sms consent" — "opted out" is an instruction from the recipient, "never opted in" is a gap the operator may legitimately close by asking, and the operator and recipient wording maps read them differently. The recipient still gets a way back: the opt-out row keeps its "Turn on texts" remedy. STOP forbids SENDING until they opt back in; it does not forbid offering a route. That remedy is a link they click in their own authenticated portal, landing on the double-opt-in page which re-shows the disclosure and records a fresh consent event — opt-in machinery, not outbound traffic. Texting START works too, and is covered by its own test.
B1 added the `in_app` channel and B2 the `staff` recipient kind at the schema and engine layers. The settings UI kept hand-written lists of the OLD values, and both failures were silent: - The save action filtered submitted channels to `email | sms`. Opening one of the seeded `Office alert — …` rules and pressing Save dropped its only channel, fell through to the email default, and left a template-less email rule that skips at flush. The office's alerts would simply have stopped, with a "no email template" reason nobody was looking at. - The recipient <select> had no `staff` option, so a staff rule rendered with nothing selected and could be saved as something else entirely. - The trigger <select> had no `booking.received` / `inspection.completed`, so two of the new rules showed a blank trigger. My own note says to grep the downstream filters when adding a field. I did not, so the fix is not just the three lists but the test that makes the next one impossible to miss: `settings-automations.test.ts` asserts each UI list against the DRIZZLE COLUMN ENUM, which is the one place the accepted values are defined. Verified it bites by removing the `staff` label and watching it name the missing value. Type-checking could not have caught any of this — every list was plain string literals. `Rule.recipientKind` and the modal's state now derive from one exported union instead of restating it twice.
important-new
added a commit
that referenced
this pull request
Aug 3, 2026
…ity (#295) * fix(scheduling): derive scheduled_start_ms on reschedule when the row 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 #289, so the drift cannot recur. Git history is the archive. * feat(library): delete canned comments, one row or many (#291) 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. * perf(gates): stop pre-commit paying for work the staged change cannot 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. * chore: delete the one-shot data backfills — there is nothing left to 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. * chore(i18n): let server code read compiled messages, through one guarded 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. * i18n(notices): move the seven stored notice titles into the catalogue 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. * fix(qbo): push card payments to QuickBooks, and make the push idempotent 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. * feat(services): seed the service catalogue, and give a service its visits 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. * feat(services): warn when two services would default to the same template 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. * feat(roster): one accessor for "who worked this inspection" 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. * fix(roster): one authority for "who worked this inspection", and freeze 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. * feat(services): soft-delete inspection service lines before anything 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. * feat(reports): the reports entity — one order, several reports 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. * feat(reports): move results and versions onto reports 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. * fix(reports): per-report signature chain and per-report collab document 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. * fix(collab): key the Durable Object's D1 projection on the report too 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. * feat(reports): document the order-wide gate, and add a per-inspection 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. * feat(hub): unlock control for the order-wide report gate, and settings 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. * docs(reports): record the surfaces the multi-report blast radius named, 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. * docs: replace the key-files table with prose 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. * fix(tests): the two resolvers only real workerd could catch 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 #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. * fix(reports): create the primary report at inspection creation 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. * fix(reports): use REPORT_STATUS.IN_PROGRESS, not a bare literal 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. * ci: cache react-router typegen the way paraglide already is 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. * test(presence): wait for the roster to converge instead of sleeping 50ms 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. * refactor(assignment): make inspection_inspectors the only place assignment 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. * ci: cache the Playwright browser, and declare the dependency e2e relied 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. * perf(tests): stop paying for a DOM and a serial suite that nothing needed 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 * docs(testing): record what running e2e projects in parallel now requires 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 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Wave 3 (role model) and Wave 4 (Communication) land together, plus the three
prerequisites the roadmap listed as blocking any wave. 37 commits.
Communication — Tracks A, D, C, B
The epic's shape is one idea: a notice is a header plus its delivery
attempts.
notificationsbecame the header (one row per recipient x notice,user_idXORcontact_id, read state);automation_logsrows are theper-channel attempts, stamped with
notice_id. It is not a dual write — thedetails belong to the header, so the two cannot disagree.
contact, manual sends written into the same ledger, the company-wide Messages
inbox with a contact picker and inspection mention.
for rows the insert actually returned, so a
report.publishedretry thatconflicts away cannot orphan one.
is
contact_id IN (me), never a match on the delivery ledger'srecipientstring: that looks equivalent and silently drops every SMS row, whose
recipient is a phone number. Dismissing a notice never touches
automation_logs— a recipient tidying their inbox cannot edit the sendingcompany's audit trail.
NoticeListacross all three portals, guarded by a spec thatrenders the same notice through each entry point and compares the TEXT a
reader sees.
in_appchannel. Delivering it means settling the ledger, notsending: the header already exists and the inbox reveals it when
send_atpasses. The branch sits before quota and consent rather than being exempted
inside them — nothing leaves the building, so there is no provider to meter,
and running the TCPA gate over a notice that was never a text message would
assert a duty that does not apply.
staffrecipient kind (the workspace's owners and managers),scoped to the inspection's tenant because
rolealone matches othercompanies' owners too.
Office alert — …rules replace four hard-codedcreateForAllAdminscall sites (migration 0017). Coverage is preservedexactly; what is gone is the duplicate notification
trigger()raised on topof whatever the rules produced.
Three defects found while building it
flush()'sinnerJoin(automations)does not skip a rule-less row — it makesit vanish: absent from the result set, pending forever, no error anywhere.
Now a left join with an explicit null story at each dereference, including the
ne(trigger, …)predicate, which is NULL-blind in SQL.send_atfilter, so a delayed automation's notice surfacedthe moment the rule fired.
report.published. It has its owninspection.completedtrigger now.Role model (Wave 3)
Per-profile capability overrides seeded explicit and read everywhere,
/mecarrying capabilities so the portal stops guessing, the referrer as a column,
the capability matrix UI, and a gate asserting every declared capability equals
its mounted guard.
Roadmap prerequisites
first red run found the roadmap had named the wrong table — the client PII it
said was on
inspectionslives oninspection_requests— and more uncoveredcolumns than the plan counted.
a bare
YYYY-MM-DDagainst a schema demandingz.string().datetime(), soevery drag 400'd while the action returned
{ ok: res.ok }without examiningit. Fixed, plus the
scheduled_start_msdual-write that preserveswall-clock time across a DST boundary.
viewCommunicationcapability mounted on the communication endpoint.Design-system fixes
text-whiteon a brand-primary fill bypassed the dark-mode token flip, so mostprimary buttons failed AA contrast in dark mode; swept 96 call sites onto
text-ih-fg-inverseand added the lint rule that catches the 97th.Popoveruses the strong border token — a panel anchored to a card-coloured surface had
no visible edge. Seven auth pages stopped hand-rolling the same input and button
and now use the shared components.
🤖 Generated with Claude Code