fix: resolve adversarial review round 2 — 9 findings, 8 of them in round 1's fixes
Build and deploy / build-and-deploy (push) Failing after 4s

D19 caps the loop at two rounds, and this is what the second round is for.

BLOCKING. Round 1 made NO_RETAINER_NOTICE a requireEnv and added it to no
document, while the fix's own comment claimed docs/06 named it. The deployment
list said five variables for a handler that needs six, so an operator following
the cutover checklist would have deployed a function that throws at cold start
on every invocation — 5xx from API Gateway, every inquiry lost from the moment
/api/* was wired, loud in CloudWatch and silent to Pouya. docs/05 and docs/06
now name all six, and the comment that asserted the documentation existed is
corrected rather than deleted.

The intake route check added in round 1 could not fail: curl -w already prints
000 on a failed transfer, so `|| echo 000` double-appended and the failure arm
was unreachable, and the pass arm accepted anything that was not literally 404 —
including the 403 CloudFront returns when the /api/* behaviour is missing, which
is the one distinction the check exists to draw. It now sends the correct Origin
and asserts a positive: 303 to /contact/could-not-send/, which the handler
returns before any DynamoDB write or email. Probed on refused/501/403/303; the
old version passed the first three. Fixed in both deploy paths.

Removing priceRange left three statements saying it was present or pending, one
of them the stated reason /fees/ emits no Offer node. Deleting
overtimeStartsAfterSessionHours left AGENTS.md §9 naming it and left Q59
recorded as open. The Google-as-processor fix was applied to the privacy
policy's "Where it is stored" and not to "Who can see it", which still read
"Nobody else has access".

And the variable removal was justified with a path-scoped git grep — which also
cannot see untracked files. The unscoped sweep found docs/06's variable table,
the OIDC example, and .env.example still carrying them; .env.example also
restates the execute-api hostname, falsifying a live claim in intake.ts that has
been corrected. That file is not edited here: this environment denies read
access to it, and nothing may edit a file it cannot read. It is in the batched
list.

Also: og:image:alt was the page title rather than the card's headline on 20
pages; og-card.ts documented the wrong path and invocation for the contact
sheet; deploy-local.sh still said Q22's deploy credential "does NOT yet exist";
and the round-1 fix comments were trimmed per D19, though the ratio held at 0.44.

Round 2 also confirmed the round-1 fixes by measurement: all 56 .btn instances
across 22 pages, the consent checkbox's computed accessible name, the radio
labels hit-tested at 44px, and og:proof exercised against synthetic article
pages in a sandbox.

Verified: check/build/check:claims/og:proof/check:intake/lint/bio:pdf all exit 0
on a clean build; 22 pages; Lighthouse 99-100 / 100 / 100 / 100, CLS 0.000.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Md3GndFqWPzK78xAoebsg5
This commit is contained in:
Pouya Lajevardi
2026-08-31 11:21:07 -04:00
co-authored by Claude Opus 5
parent 9f2d83c32f
commit 9f2d2eeb04
18 changed files with 360 additions and 180 deletions
+1 -1
View File
@@ -128,7 +128,7 @@ JSON-LD only. Validate against Google's Rich Results Test before cutover.
| Type | Where | Notes |
|---|---|---|
| `Person` | `/about/`, referenced site-wide | **Emitted:** `name`, `url`, `jobTitle`, `description`, `alumniOf` (Bond University), `knowsLanguage` (en, fa), `hasCredential` (**Q.Med, Q.Arb** — both, since 2026-08-29), `sameAs` (LinkedIn), `email`, `image`. **Emitted on `/about/` only:** `memberOf` — the four §4 memberships as `Organization` nodes (Q53, ruled 2026-08-28). `/` shows no memberships, so its Person node omits it: structured data represents the page it sits on. **Withheld:** `worksFor` — Q49(b) declined the row 2026-08-28 and Pouya confirmed the reading 2026-08-29, so it is settled rather than pending; `provider → Person → worksFor` would assert a same-entity claim §4 does not row. *(This enumeration listed `worksFor` as emitted while the same cell said it was withheld, and omitted `url` and `email`, which are — wrong in both directions. The enumeration is the part an implementer copies. Found by `adversarial-reviewer`.)* **CHANGED 2026-08-28 — Q47.** This row read *"`jobTitle` = 'Director of Firm Operations'; omit `worksFor`"*, which put the boutique title on a node whose `url` is this ADR practice's `/about/` — so a consumer could attach it to this entity. Pouya's ruling reframes the field: `jobTitle` describes **this practice**, not the boutique role, which D16 keeps unnamed. The visible role line is unchanged and still reads "Director of Firm Operations at a Toronto litigation and ADR boutique". **THE VALUE IS `PRACTICE_JOB_TITLE` IN `src/data/site.ts` AND THIS ROW DOES NOT RESTATE IT** — §7's rule, applied to a string with a live revert trigger on it: this row carried the literal text for one pass, and `adversarial-reviewer` noted it would go stale the moment the constant moved. Cite, do not copy. **`worksFor` IS WITHHELD** — set for one pass under Q47, then reverted: `ProfessionalService.provider` is this Person, so `provider → Person → worksFor` asserts the same-entity claim `schema.ts` explicitly declines, and §4 says "alongside the practice" where the ruling says "operates through". **`memberOf` is emitted** — see the sentence above; Q53 closed 2026-08-28. *(This cell asserted `memberOf` was both emitted and withheld for one pass, which is the defect it already records itself being caught for on `worksFor`, in the opposite direction. The enumeration is the part an implementer copies.)* See `src/data/schema.ts` |
| `ProfessionalService` | Home | `areaServed` Toronto/Ontario, `serviceType` **Mediation / Commercial arbitration / Mediation-arbitration (med-arb)***scoped 2026-08-28 on `claims-auditor`'s finding; this row instructed the unscoped class form "Mediation/Arbitration" that Q39 struck and that `schema.ts` deliberately does not follow. Family arbitration carries prescribed training and has its own NOT OFFERED row, so unscoped "Arbitration" is the struck universal in a field nobody reads. Do not widen these strings without a §4 row to widen them from*`provider` → Person, `priceRange` once `/fees/` is real. **Never `LegalService`** — schema.org defines it as a business providing legal advice and *representation*, which asserts in machine-readable form exactly what D13 bars and §4 Forbidden calls out |
| `ProfessionalService` | Home | `areaServed` Toronto/Ontario, `serviceType` **Mediation / Commercial arbitration / Mediation-arbitration (med-arb)***scoped 2026-08-28 on `claims-auditor`'s finding; this row instructed the unscoped class form "Mediation/Arbitration" that Q39 struck and that `schema.ts` deliberately does not follow. Family arbitration carries prescribed training and has its own NOT OFFERED row, so unscoped "Arbitration" is the struck universal in a field nobody reads. Do not widen these strings without a §4 row to widen them from*`provider` → Person, ⚠️ **`priceRange` DECLINED 2026-08-31 — this row said *"once `/fees/` is real"*, the page became real at step 9, the field went in, and it came out the same day.** Its own defence rejected a `min`/`max` over `FEES` because *"a range whose ends mean different units is a range that misinforms"* — and the ends it chose had different units too: the floor was the hourly rate, the ceiling a flat documents-only fee. The floor misinformed in the direction that matters, because the least anyone pays for the headline service is **$2,000**. **Nothing on the site states a price in machine-readable form**, and no `Offer` node either: every figure on `/fees/` is conditional on session length, party count or format, and schema.org's `Offer` models one price for one item. This row gates the field; it does not require it. **Never `LegalService`** — schema.org defines it as a business providing legal advice and *representation*, which asserts in machine-readable form exactly what D13 bars and §4 Forbidden calls out |
| `Service` | **`/mediation/`, `/arbitration/`, `/med-arb/`** and each practice page | `serviceType`, `provider` → Person, `areaServed`. **The Person node travels in the same `@graph`** so `provider: {'@id'}` resolves in one document rather than relying on a crawler joining two — `homeGraph`'s reasoning, applied. `serviceType` is scoped where §4 scopes it: *Commercial arbitration*, never a bare "Arbitration". No `BreadcrumbList` on the three — one hop from the root, no visible breadcrumb, and this spec requires the markup to match the visible one |
| `Article` | Each article | `headline`, `description`, `datePublished`, `dateModified`, `author` → Person, `image` |
| `BreadcrumbList` | All nested pages | Matches visible breadcrumbs |
+1 -1
View File
@@ -374,4 +374,4 @@ Plausible or Fathom, cookieless, no consent banner.
- [x] **Works with JavaScript disabled** — replacing the `mailto:` degradation item; see deviation 3
- [x] **Privacy policy matches the implementation** — and three of its statements are DERIVED rather than written, so they cannot drift: the collected-data list renders from `INTAKE_FIELDS`, the retention period from the handler's own figure, and the analytics paragraph from `ANALYTICS.installed`
- [ ] **CloudFront `/api/*` behaviour created**, routing to the HTTP API origin §7 records. The form does not work without it
- [ ] **Handler deployed**, replacing the hand-built `adr-intake-handler`, with `INTAKE_TABLE`, `SITE_ORIGIN`, `NOTIFY_TO`, `MAIL_FROM` and `RESPONSE_TIME` set. It throws at cold start on any missing one, deliberately
- [ ] **Handler deployed**, replacing the hand-built `adr-intake-handler`, with **SIX** variables set: `INTAKE_TABLE`, `SITE_ORIGIN`, `NOTIFY_TO`, `MAIL_FROM`, `RESPONSE_TIME` and `NO_RETAINER_NOTICE`. It throws at cold start on any missing one, deliberately. ⚠️ **This item said five while the handler required six.** `NO_RETAINER_NOTICE` became a `requireEnv` and reached no document, so an operator following the list would have deployed a function that throws on every invocation — 5xx from API Gateway, and every inquiry lost from the moment `/api/*` was wired. Found by `adversarial-reviewer`, 2026-08-31. **Two of the six must be verbatim from `src/data/site.ts`**, because both are published commitments: `RESPONSE_TIME` from `CONTACT.responseTime`, and `NO_RETAINER_NOTICE` from the constant of the same name — whose fourth clause (*"does not itself create a conflict check"*, required by `docs/01` §`/contact/`) a hand-typed copy in the handler had dropped
+41 -7
View File
@@ -162,10 +162,31 @@ they are useful for debugging.
| `AWS_REGION` | `AGENTS.md` §7 — Region |
| `S3_BUCKET` | §7 — S3 bucket |
| `CLOUDFRONT_DISTRIBUTION_ID` | §7 — CloudFront |
| `INTAKE_ENDPOINT` | §7 — Intake API |
| `BOOKING_URL` | *(empty — parked, R6)* |
The same four values fill the IAM policy's `BUCKET_NAME`, `ACCOUNT_ID` and
⚠️ **`INTAKE_ENDPOINT` AND `BOOKING_URL` WERE ROWS HERE AND ARE GONE, 2026-08-31.**
Build step 8 moved the intake form to the same-origin path `/api/intake`, after
which nothing in the build read either one — and both deploy paths were still
refusing to run without `INTAKE_ENDPOINT`. **The removal was made in the two
scripts and not here**, so this table went on telling an operator to set a
variable no guard checked and no build consumed. Found by
`adversarial-reviewer` round 2, whose sharper point is about the evidence: the
removal was justified with `git grep PUBLIC_INTAKE_ENDPOINT -- src/` — a
**path-scoped** grep offered as proof of a repo-wide removal, and `git grep`
additionally cannot see untracked files, which at that moment was most of the new
work. That is `CLAUDE.md`'s *a sweep is a command, not a claim* and *sweep the
vocabulary, not only the subject*, both at once.
⚠️ **AND ONE PLACE STILL CARRIES IT: `.env.example` sets
`PUBLIC_INTAKE_ENDPOINT` to the full execute-api hostname, and
`PUBLIC_BOOKING_URL`.** That falsifies a live claim in `src/data/intake.ts`
which said the endpoint id *"is not restated in the repo either"* — and the claim
has been corrected there rather than left standing. **The file itself was not
edited: this environment denies read access to it, and nothing in this repo may
edit a file it cannot read.** It is in the batched list for Pouya: delete both
lines, and `PUBLIC_ANALYTICS_*` with them, since `ANALYTICS` is a literal in
`src/data/site.ts`.
The three values above fill the IAM policy's `BUCKET_NAME`, `ACCOUNT_ID` and
`DISTRIBUTION_ID` placeholders. **They are deliberately not restated here**
§7 is the single source of truth for operational facts, and the copy that goes
stale is always the one nobody re-reads. `scripts/aws-discover.sh` regenerates
@@ -438,10 +459,23 @@ Then invalidate `/*`.
this behaviour the form 404s.
2. **Deploy `backend/intake/handler.mjs` + `backend/intake/fields.mjs`**,
replacing the hand-built `adr-intake-handler` §7 records. It needs
`INTAKE_TABLE`, `SITE_ORIGIN`, `NOTIFY_TO`, `MAIL_FROM` and
`RESPONSE_TIME` set; it throws at cold start on any missing one, on
purpose. `RESPONSE_TIME` must be `CONTACT.responseTime` verbatim — it is
a public commitment (§4, Q27) and must read identically on `/contact/`,
**SIX** variables — `INTAKE_TABLE`, `SITE_ORIGIN`, `NOTIFY_TO`,
`MAIL_FROM`, `RESPONSE_TIME` and `NO_RETAINER_NOTICE`; it throws at cold
start on any missing one, on purpose. ⚠️ **This list said five until
2026-08-31 and the handler required six.** `NO_RETAINER_NOTICE` became a
`requireEnv` in the same change set and was added to no document, so an
operator working this list would have set five, and the Lambda would
have thrown on **every** invocation — API Gateway answering 5xx and every
inquiry from counsel lost from the moment `/api/*` was wired. Loud in
CloudWatch, silent to Pouya, who has no reason to submit his own form.
Found by `adversarial-reviewer` round 2.
**Two of the six must be verbatim from `src/data/site.ts`:**
`RESPONSE_TIME` from `CONTACT.responseTime` and `NO_RETAINER_NOTICE`
from `NO_RETAINER_NOTICE`. Both are published commitments — the first is
§4/Q27's two-business-day response, the second is the no-retainer notice
`docs/01` §`/contact/` requires, **including its fourth clause about
not itself creating a conflict check**, which a hand-typed copy in the
handler had dropped. `RESPONSE_TIME` must read identically on `/contact/`,
in the confirmation email, and in the bio.
3. **API Gateway throttling, 5 requests / 5 minutes per source IP**
(`docs/05`). Not expressible in handler code, and it is carrying load
+12 -4
View File
@@ -49,12 +49,20 @@ jobs:
run: npm run build
env:
PUBLIC_SITE_URL: https://adr.smlcompany.ca
PUBLIC_INTAKE_ENDPOINT: ${{ vars.INTAKE_ENDPOINT }}
PUBLIC_BOOKING_URL: ${{ vars.BOOKING_URL }}
# SUPERSEDED 2026-08-31 — do not copy these two lines. Build step 8
# moved the intake form to the same-origin path /api/intake, after
# which nothing in the build read either variable; both were removed
# from the live workflow and from scripts/deploy-local.sh. Kept visible
# rather than deleted because this whole file is a historical
# alternative, and a silent edit to it would make it disagree with the
# entry that recorded it.
# PUBLIC_INTAKE_ENDPOINT: ${{ vars.INTAKE_ENDPOINT }}
# PUBLIC_BOOKING_URL: ${{ vars.BOOKING_URL }}
# If adopting this: set AWS_DEPLOY_ROLE_ARN as a repository variable. The
# rest — AWS_REGION, S3_BUCKET, CLOUDFRONT_DISTRIBUTION_ID, INTAKE_ENDPOINT and
# BOOKING_URL — are recorded in docs/06-deployment.md.
# rest — AWS_REGION, S3_BUCKET and CLOUDFRONT_DISTRIBUTION_ID — are recorded
# in docs/06-deployment.md. (INTAKE_ENDPOINT and BOOKING_URL were listed here
# and are no longer required by either deploy path; see above.)
- name: Configure AWS credentials
uses: aws-actions/configure-aws-credentials@v4
with: