From 4fcab67c5fea5d791fdddc242d1a0459a4b7775b Mon Sep 17 00:00:00 2001 From: NCHILLC Date: Tue, 13 Oct 2026 13:54:24 -0400 Subject: [PATCH 1/2] fix(agreements): fill merge fields at send and refuse a body that still has placeholders (QA sweep DELIV-01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The starter Pre-Inspection Agreement could be sent and signed with every merge field still in brackets and the admin-only "review before sending" notice inside the permanent content_snapshot. findOrCreate now fills [INSPECTOR_NAME], [CUSTOMER_NAME], [PROPERTY_ADDRESS], [INSPECTION_DATE] and $[AMOUNT] from the inspection (amount via the P-4 authority chain) before the snapshot is frozen, and throws 422 naming any bracketed token that is left or the starter notice, creating no envelope. The hub's Send dialog shows that refusal inside itself; the page-level line it replaces sat under the modal scrim, so Send looked like it did nothing. Chrome: /inspections/:id — sent the unedited starter agreement from the Send for signing dialog on a local empty-D1 build, and the refusal about unfilled placeholders and the review-before-sending notice reads in red inside the dialog above Cancel and Send, dark theme at 1024 px; not seen: light theme, a phone width, the filled-in success path in the pane Co-Authored-By: Claude Sonnet 5.5 --- .../agreements/SendAgreementModal.test.tsx | 32 ++++++ .../agreements/SendAgreementModal.tsx | 5 +- app/routes/inspector-portal.tsx | 4 +- docs/develop/fork-log.md | 1 + server/lib/agreement-tokens.ts | 61 ++++++++++++ server/services/agreement/envelope-legacy.ts | 45 ++++++++- .../agreements/agreement-send-tokens.spec.ts | 97 +++++++++++++++++++ 7 files changed, 237 insertions(+), 8 deletions(-) create mode 100644 app/components/agreements/SendAgreementModal.test.tsx create mode 100644 server/lib/agreement-tokens.ts create mode 100644 tests/unit/agreements/agreement-send-tokens.spec.ts diff --git a/app/components/agreements/SendAgreementModal.test.tsx b/app/components/agreements/SendAgreementModal.test.tsx new file mode 100644 index 00000000..1a3e8fad --- /dev/null +++ b/app/components/agreements/SendAgreementModal.test.tsx @@ -0,0 +1,32 @@ +// @vitest-environment happy-dom +/** + * DELIV-01 — the API now refuses to send an agreement that still carries + * placeholders. That refusal used to land in a page-level line underneath the + * modal's scrim, so Send looked like it did nothing. It must read inside the + * dialog the inspector is looking at. + */ +import { describe, it, expect } from "vitest"; +import { render, screen } from "@testing-library/react"; + +import { SendAgreementModal } from "./SendAgreementModal"; + +const base = { + open: true, + onSend: () => {}, + onClose: () => {}, + initialSigners: [{ name: "Pat Rivera", email: "pat@example.com", role: "client" as const }], +}; + +describe("SendAgreementModal serverError", () => { + it("shows the API's refusal as an alert inside the dialog", () => { + render(); + const alert = screen.getByRole("alert"); + expect(alert.textContent).toContain("cannot be sent yet"); + expect(alert.closest('[role="dialog"]')).not.toBeNull(); + }); + + it("shows nothing when there is no refusal", () => { + render(); + expect(screen.queryByRole("alert")).toBeNull(); + }); +}); diff --git a/app/components/agreements/SendAgreementModal.tsx b/app/components/agreements/SendAgreementModal.tsx index 9e0ffe24..97189af8 100644 --- a/app/components/agreements/SendAgreementModal.tsx +++ b/app/components/agreements/SendAgreementModal.tsx @@ -101,6 +101,7 @@ export function SendAgreementModal({ busy, initialSigners, templates, + serverError, }: { open: boolean; onSend: (payload: SendAgreementPayload) => void; @@ -109,6 +110,8 @@ export function SendAgreementModal({ initialSigners?: SignerDraft[]; /** Agreement templates to choose from. Omit to hide the picker entirely. */ templates?: AgreementTemplateOption[]; + /** The API's refusal of the last send. Shown inside the dialog: a page-level line sits under the modal's scrim and reads as "Send did nothing". */ + serverError?: string | null; }) { const seed = (): SignerDraftRow[] => { if (initialSigners && initialSigners.length > 0) { @@ -272,7 +275,7 @@ export function SendAgreementModal({ - {error &&

{error}

} + {(error ?? serverError) &&

{error ?? serverError}

} ); } diff --git a/app/routes/inspector-portal.tsx b/app/routes/inspector-portal.tsx index 81ee9e89..344a51ca 100644 --- a/app/routes/inspector-portal.tsx +++ b/app/routes/inspector-portal.tsx @@ -1274,12 +1274,10 @@ export default function InspectionHubPage() { : undefined } busy={agreementModal.busy} + serverError={agreementModal.error} onSend={submitSendAgreement} onClose={() => agreementModal.setOpen(false)} /> - {agreementModal.error && ( -

{agreementModal.error}

- )} {/* Inspector pre-sign — the envelope-level signature an inspector applies before the client sees it. Moved off the Library page with the rest of diff --git a/docs/develop/fork-log.md b/docs/develop/fork-log.md index c88e8f4f..9f740821 100644 --- a/docs/develop/fork-log.md +++ b/docs/develop/fork-log.md @@ -331,3 +331,4 @@ deliberate departure rather than work upstream simply has not done yet. - Record skips as explicitly as takes. "We looked and said no" and "we never looked" are the same silence otherwise. - Update the snapshot when you fetch, not when you remember. +| `agreement` merge fields filled at send, placeholders block the send (2026-10-13 QA sweep DELIV-01, Blocker; no upstream fix) | `AgreementService.findOrCreate` (`server/services/agreement/envelope-legacy.ts`) runs the template through `fillAgreementTokens` (`server/lib/agreement-tokens.ts`) before it freezes `content_snapshot`: `[INSPECTOR_NAME]` (assigned inspector, else the company's legal or company name), `[CUSTOMER_NAME]`, `[PROPERTY_ADDRESS]`, `[INSPECTION_DATE]` (long civil day) and `$[AMOUNT]` (`getEffectivePriceCents`: invoice, then service lines, then `inspections.price`); `[CLIENT_SIGNATURE_BLOCK]` and `[SIGN_DATE]` become "(signature captured below)" and "(recorded at signing)", values HTML-escaped. If any other bracketed ALL-CAPS token remains, or the starter's "Review before sending to real customers" notice is still in the text, it throws 422 naming them and creates no envelope. `SendAgreementModal` takes `serverError` and shows it inside the dialog (the page-level line it replaces sat under the scrim, so Send looked dead). Specs: `tests/unit/agreements/agreement-send-tokens.spec.ts`, `app/components/agreements/SendAgreementModal.test.tsx` | The finding: a fresh company's only template could be sent and signed with every merge field still in brackets and the admin caveat in the permanent record. `git log main..upstream/main` by keyword (placeholder, merge field, starter agreement, agreement) and on the fixture file finds nothing; the upstream plan lists it under its PR 5 but upstream has not landed it. The starter text is unchanged, so an untouched starter is refused (notice and the liability clause both stay) until the company edits it in Settings; that is the intended state of a legal document, not a gap. **Known ceilings:** the match is `[A-Z][A-Z0-9_]{2,}`-style brackets only, so a custom template with lowercase or `{{x}}` fields is sent as written; the check runs only when an envelope is created (reusing a live envelope does not re-check); an inspection with no price blocks on `[AMOUNT]` rather than printing $0.00; the signing page itself still shows what the snapshot holds, so envelopes sent before this change keep their placeholders | diff --git a/server/lib/agreement-tokens.ts b/server/lib/agreement-tokens.ts new file mode 100644 index 00000000..c82c8219 --- /dev/null +++ b/server/lib/agreement-tokens.ts @@ -0,0 +1,61 @@ +/** + * Send-time merge fields for an agreement template. + * + * The starter template ships `[INSPECTOR_NAME]`, `[CUSTOMER_NAME]`, + * `[PROPERTY_ADDRESS]`, `[INSPECTION_DATE]` and `$[AMOUNT]`. They are filled + * from the inspection when the envelope is created, because the envelope's + * `content_snapshot` is the permanent signed record: whatever is still in + * brackets here is what the client signs and what a dispute produces. + * + * Anything bracketed that is NOT filled, and the starter's "review before + * sending" notice, block the send instead (`blockingAgreementIssues`). + */ + +export interface AgreementTokenValues { + inspector?: string | null | undefined; + client?: string | null | undefined; + address?: string | null; + /** Civil day, `YYYY-MM-DD` (an `T…Z` suffix is ignored). */ + date?: string | null; + amountCents?: number | null; +} + +/** Substring of the starter template's admin-only caveat paragraph. */ +export const STARTER_NOTICE_MARK = 'Review before sending to real customers'; + +const esc = (s: string): string => + s.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"'); + +const longDay = (d: string): string | null => { + const t = new Date(`${d.slice(0, 10)}T12:00:00Z`); + return Number.isNaN(t.getTime()) ? null : t.toLocaleDateString('en-US', { timeZone: 'UTC', dateStyle: 'long' }); +}; + +export function fillAgreementTokens(html: string, v: AgreementTokenValues): string { + const day = v.date ? longDay(v.date) : null; + const map: Record = { + INSPECTOR_NAME: v.inspector?.trim() || null, + CUSTOMER_NAME: v.client?.trim() || null, + PROPERTY_ADDRESS: v.address?.trim() || null, + INSPECTION_DATE: day, + AMOUNT: v.amountCents != null && v.amountCents > 0 ? (v.amountCents / 100).toFixed(2) : null, + // Not known at send time. The signature itself and its timestamp are + // recorded with the signer row; say so rather than leave a bracket. + CLIENT_SIGNATURE_BLOCK: '(signature captured below)', + SIGN_DATE: '(recorded at signing)', + }; + return html.replace(/\[([A-Z_]+)\]/g, (whole, key: string) => { + const val = map[key]; + return val ? esc(val) : whole; + }); +} + +/** Human-readable reasons this body must not be sent as it stands. */ +export function blockingAgreementIssues(html: string): string[] { + const issues: string[] = []; + const tokens = [...new Set(html.match(/\[[A-Z][A-Z0-9_]{2,}\b[^\]]*\]/g) ?? [])] + .map((t) => (t.length > 40 ? `${t.slice(0, 37)}...]` : t)); + if (tokens.length > 0) issues.push(`unfilled placeholders: ${tokens.join(', ')}`); + if (html.includes(STARTER_NOTICE_MARK)) issues.push('the "review before sending" notice is still in the text'); + return issues; +} diff --git a/server/services/agreement/envelope-legacy.ts b/server/services/agreement/envelope-legacy.ts index c7ade5a3..2f90e7c1 100644 --- a/server/services/agreement/envelope-legacy.ts +++ b/server/services/agreement/envelope-legacy.ts @@ -1,5 +1,7 @@ import { eq, and, inArray, lt, sql, desc, asc } from 'drizzle-orm'; -import { agreements, agreementRequests, agreementSigners, inspections } from '../../lib/db/schema'; +import { agreements, agreementRequests, agreementSigners, inspections, inspectionServices, invoices, users } from '../../lib/db/schema'; +import { getEffectivePriceCents } from '../../lib/effective-price'; +import { fillAgreementTokens, blockingAgreementIssues } from '../../lib/agreement-tokens'; import { Errors } from '../../lib/errors'; import { logger } from '../../lib/logger'; import { mintToken, hashToken } from '../../lib/token-hash'; @@ -131,7 +133,10 @@ export function EnvelopeLegacyMixin 0) { + throw Errors.UnprocessableEntity( + `This agreement cannot be sent yet: ${issues.join('; ')}. Edit the template in Settings, then send again.`, + ); + } const contentHash = await sha256Hex(contentSnapshot); // Spec 2026-08-04 section 3 — freeze the contracting identity onto // the envelope. Renaming the company must not retroactively rewrite // which entity a past agreement was signed with. `legalName` arrives // ALREADY resolved from getBrand; no fallback is re-applied here. - const brand = await new BrandingService(this.db).getBrand(tenantId); const newRow = { id: requestId, diff --git a/tests/unit/agreements/agreement-send-tokens.spec.ts b/tests/unit/agreements/agreement-send-tokens.spec.ts new file mode 100644 index 00000000..7d59d7cd --- /dev/null +++ b/tests/unit/agreements/agreement-send-tokens.spec.ts @@ -0,0 +1,97 @@ +/** + * DELIV-01 — the starter agreement used to be sendable and signable with every + * merge field still in brackets and the admin-only "review before sending" + * notice inside the permanent signed snapshot. Fields are now filled at send + * time and a body that still carries placeholders or the notice is refused. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { eq } from 'drizzle-orm'; +import { AgreementService } from '../../../server/services/agreement.service'; +import { PeopleService } from '../../../server/services/people.service'; +import { seedRoleProfiles } from '../../../server/services/seed/seed-role-profiles'; +import { AGREEMENT_TEMPLATE } from '../../../server/services/starter-content/fixtures/agreement-template'; +import { fillAgreementTokens, blockingAgreementIssues } from '../../../server/lib/agreement-tokens'; +import { createTestDb, setupSchema } from '../db'; +import { asD1Db } from '../helpers/test-db'; +import * as schema from '../../../server/lib/db/schema'; +import type { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; + +vi.mock('drizzle-orm/d1', () => ({ drizzle: vi.fn() })); +import { drizzle as mockDrizzle } from 'drizzle-orm/d1'; + +const TENANT = '00000000-0000-0000-0000-000000000001'; +const CLIENT = 'contact-client-1'; +const INSP_ID = '550e8400-e29b-41d4-a716-446655440000'; +const AGR_ID = '00000000-0000-0000-0000-000000000020'; +const USER = 'user-inspector-1'; + +const withoutNotice = AGREEMENT_TEMPLATE.content.replace(/^

⚠️.*?<\/p>/, ''); + +describe('fillAgreementTokens / blockingAgreementIssues', () => { + it('fills the starter fields and leaves nothing blocking', () => { + const out = fillAgreementTokens(withoutNotice.replace(/

4\..*?<\/p>/, ''), { + inspector: 'Quinn ', client: 'Pat Rivera', address: '742 Ivy Lane', date: '2026-10-20T09:00:00Z', amountCents: 40000, + }); + expect(out).toContain('Quinn <Assurance> ("Inspector")'); + expect(out).toContain('Pat Rivera ("Client")'); + expect(out).toContain('October 20, 2026'); + expect(out).toContain('Inspection fee: $400.00.'); + expect(blockingAgreementIssues(out)).toEqual([]); + }); + + it('a missing value stays bracketed and blocks', () => { + const out = fillAgreementTokens('

[CUSTOMER_NAME] pays $[AMOUNT]

', { client: 'Pat', amountCents: 0 }); + expect(out).toBe('

Pat pays $[AMOUNT]

'); + expect(blockingAgreementIssues(out)[0]).toContain('[AMOUNT]'); + }); +}); + +describe('AgreementService.findOrCreate — starter agreement merge fields (DELIV-01)', () => { + let db: BetterSQLite3Database; + let svc: AgreementService; + + const seedAgreement = (content: string) => + db.update(schema.agreements).set({ content }).where(eq(schema.agreements.id, AGR_ID)); + + beforeEach(async () => { + const fixture = createTestDb(); + db = fixture.db; + await setupSchema(fixture.sqlite); + (mockDrizzle as unknown as ReturnType).mockReturnValue(db); + await db.insert(schema.tenants).values({ + id: TENANT, slug: 'acme', status: 'active', deploymentMode: 'shared', tier: 'free', createdAt: new Date(), + }); + await db.insert(schema.users).values({ + id: USER, tenantId: TENANT, email: 'q@example.com', passwordHash: 'x', name: 'Quinn Assurance', role: 'owner', createdAt: new Date(), + } as typeof schema.users.$inferInsert); + await db.insert(schema.agreements).values({ + id: AGR_ID, tenantId: TENANT, name: AGREEMENT_TEMPLATE.name, content: AGREEMENT_TEMPLATE.content, version: 1, createdAt: new Date(), + }); + await seedRoleProfiles(asD1Db(db), TENANT, new Date(1)); + await db.insert(schema.contacts).values({ + id: CLIENT, tenantId: TENANT, type: 'client', name: 'Pat Rivera', email: 'pat@example.com', phone: '+15551234567', createdAt: new Date(), + }); + await db.insert(schema.inspections).values({ + id: INSP_ID, tenantId: TENANT, inspectorId: USER, propertyAddress: '742 Ivy Lane, Charlotte, NC', date: '2026-10-20', + status: 'requested', paymentStatus: 'unpaid', price: 40000, agreementRequired: true, paymentRequired: false, createdAt: new Date(), + }); + await new PeopleService({ DB: {} as D1Database }).addPerson(TENANT, INSP_ID, CLIENT, `crp_${TENANT}_client`); + svc = new AgreementService({} as D1Database, { jwtSecret: 'test-secret' }); + }); + + it('the unedited starter template is refused and creates no envelope', async () => { + await expect(svc.findOrCreate(TENANT, INSP_ID)).rejects.toMatchObject({ status: 422 }); + expect(await db.select().from(schema.agreementRequests).all()).toHaveLength(0); + }); + + it('once the notice and liability placeholder are edited out, the snapshot carries the real values', async () => { + await seedAgreement(withoutNotice.replace(/

4\..*?<\/p>/, '')); + const r = await svc.findOrCreate(TENANT, INSP_ID); + const env = await db.select().from(schema.agreementRequests).where(eq(schema.agreementRequests.id, r.requestId)).get(); + expect(env?.contentSnapshot).toContain('Quinn Assurance ("Inspector")'); + expect(env?.contentSnapshot).toContain('Pat Rivera ("Client")'); + expect(env?.contentSnapshot).toContain('742 Ivy Lane, Charlotte, NC on October 20, 2026'); + expect(env?.contentSnapshot).toContain('$400.00'); + expect(env?.contentSnapshot).not.toMatch(/\[[A-Z_]{3,}/); + }); +}); From 3cb3a68384ef953e335cbefd5eef26c793133370 Mon Sep 17 00:00:00 2001 From: NCHILLC Date: Tue, 13 Oct 2026 14:19:15 -0400 Subject: [PATCH 2/2] fix(agreements): satisfy the CI-only dead-code and i18n gates (DELIV-01) STARTER_NOTICE_MARK was exported but only used in-file; the long-date formatter is English-only on purpose (the body is frozen into content_snapshot), so it carries an i18n-lint-ok note. Co-Authored-By: Claude Sonnet 5.5 --- server/lib/agreement-tokens.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/lib/agreement-tokens.ts b/server/lib/agreement-tokens.ts index c82c8219..85af2d3b 100644 --- a/server/lib/agreement-tokens.ts +++ b/server/lib/agreement-tokens.ts @@ -21,14 +21,14 @@ export interface AgreementTokenValues { } /** Substring of the starter template's admin-only caveat paragraph. */ -export const STARTER_NOTICE_MARK = 'Review before sending to real customers'; +const STARTER_NOTICE_MARK = 'Review before sending to real customers'; const esc = (s: string): string => s.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"'); const longDay = (d: string): string | null => { const t = new Date(`${d.slice(0, 10)}T12:00:00Z`); - return Number.isNaN(t.getTime()) ? null : t.toLocaleDateString('en-US', { timeZone: 'UTC', dateStyle: 'long' }); + return Number.isNaN(t.getTime()) ? null : t.toLocaleDateString('en-US', { timeZone: 'UTC', dateStyle: 'long' }); // i18n-lint-ok: the agreement body is English-only and frozen into content_snapshot }; export function fillAgreementTokens(html: string, v: AgreementTokenValues): string {