Clear the fast-uri advisory, and the gates and skips that were not holding - #297
Merged
important-new merged 8 commits intoAug 5, 2026
Merged
Conversation
A new advisory surfaced after InspectorHub#296 merged — host confusion via a backslash authority introducer, high, runtime scope. fast-uri arrives transitively through @modelcontextprotocol/sdk -> ajv, so it needs an override rather than a direct bump. Pinned `^3.1.5`, not `>=3.1.5`: the open range resolved to 4.1.2, and forcing a major on a transitive that ajv 8.20 expects at 3.x is a way to break schema validation while fixing a URL parser. The MCP suite (85 tests) passes on 3.1.5. Lockfile updated in place; linux entries still 346. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
The knip baseline held exactly two entries, and they were the same thing twice: `ERASURE_OUT_OF_SCOPE` and its `ErasureOutOfScopeEntry` type. Neither is dead. `scripts/check-erasure-manifest.mjs` is a plain .mjs gate against a TypeScript manifest, so it reads the declaration out of the SOURCE TEXT (`arrayBody(src, "ERASURE_OUT_OF_SCOPE")`) instead of importing it — a consumption no module-graph analyzer can see. A baseline entry says "this is dead and we tolerate it". Only "a tool consumes this" was true, so it is now said that way: the two exports carry a `@gateConsumed` JSDoc tag, wired through knip `tags: ["-gateConsumed"]`, with the reason written at the declaration. Chose the tag over adding the file to `entry`: `entry` would have exempted the whole file, so a future dead export in the manifest would go unreported. Verified by canary — a throwaway unused export added to that same file still fails the gate (exit 1), and removing it returns exit 0. scripts/knip-baseline.json is now `[]`, and the gate docstring says it must stay that way, with the three legitimate ways to declare a new finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
All 8 server-side entries in the status-literal baseline are gone; the baseline drops 13 -> 5 and the 5 that remain are the app/ render branches, left alone deliberately. They were three different state machines, not one: - REPORT axis (server/api/inspections/publish.ts) — the three transition responses now return REPORT_STATUS.SUBMITTED / .IN_PROGRESS. - INSPECTION axis (server/services/concierge.service.ts) — the confirm-by- client write now sets INSPECTION_STATUS.CONFIRMED. The file already imported the constant and used it four lines earlier. - EVENT axis (server/services/event.service.ts, server/lib/google-calendar.ts) — a visit, not the order: a radon drop-off is `completed` while its inspection is still `confirmed`, and `results_received` has no counterpart on the order at all. These derive from EVENT_STATUS, NOT from INSPECTION_STATUS, even where the word is identical. event.service.ts also carried a hand-written `export type EventStatus` union duplicating the canonical one in lib/status/event-status.ts — deleted, nothing imported it. server/portal/outbox.service.ts was the fourth axis and the interesting one: its `published` means "handed to the queue", while a report's `published` means "delivered to the client". Collapsing them into REPORT_STATUS would have been worse than the literal. The column already declared its enum inline, so that enum is now named — lib/status/sync-outbox-status.ts, the same shape as the three existing axes — the schema derives from it (type-layer only; db:check confirms zero DDL drift) and the service's pending/published/failed literals all reference it. Also fixed one bare literal the gate CANNOT see, found while working: server/services/automation/conditions.ts compared inspection.status against 'cancelled' and 'completed' in a `||` chain. The gate's union-type guard (`/^\s*\|/`, meant for `status: 'a' | 'b'` type declarations) matches the `|` of a `||` too, so every comparison in an or-chain except the last is silently dropped. Reported separately — not fixed here, since tightening the guard surfaces new hits and this commit is about shrinking the baseline. test:unit 669 files / 4581 tests passed (1 file, 1 test skipped). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
…ails globalSetup wrapped seedFixtures() in a try/catch that only console.warn'd. SEED_E2E=1 means the caller ASKED for the seed and every spec downstream depends on the rows it writes, so swallowing the failure did not make the run robust — it made it lie: the specs then failed at a login or a missing inspection id, which reads as a broken feature rather than a broken fixture. Five separate defects in tests/seed-fixtures.ts survived months behind that warning (wrong database name, missing -c, embedded newlines cmd.exe rejects, a password-hash format verifyPassword can never match, and a tenant id standalone login cannot resolve). The seed now throws, and a seedRequested flag carries the throw past the outer catch — which exists to tolerate a missing local D1 and would otherwise re-swallow it as the same soft warning. The un-requested (SEED_E2E unset) path still warns exactly as before. Verified both directions by running globalSetup directly with a temporary throw planted in seedFixtures: SEED_E2E=1 threw, SEED_E2E unset returned normally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
… them
Six skipped tests targeted Alpine-era source the React Router migration
deleted, so they could never be unskipped — they only inflated the suite's
skip count with tests that can never run.
- standalone-browser: UI-11 / UI-NOTIFY / UI-WIDGET. Empty bodies
(async () => {}) keyed to #agreementsList, #notifyUnreadBadge and
[data-widget-embed]. Placeholders, not tests — nothing is lost.
- standalone-mobile: M-05, same empty-body shape (Alpine message FAB).
- booking-date-input.spec.ts: the whole file was one describe.skip driving
the Alpine booking form. File deleted.
- sprint2-regression.spec.ts: both describe.skip blocks read
src/templates/pages/rating-systems.tsx, src/templates/layouts/
main-layout.tsx and public/js/auth.js off disk — none of which exist.
File deleted.
Deleting the last two files empties their projects, so the sprint2-regression
and booking-date-input entries go from playwright.config.ts as well: a project
whose testMatch resolves to nothing is a new way to report green over zero
tests.
Verified the survivors: browser + mobile projects, 33 passed / 0 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
…what is missing The spec said it was "skipped pending the dual-server harness" and pointed at tests/global-setup.ts as the gap. That gap has been closed since the multi-user seed landed: SEED_E2E=1 + tests/seed-fixtures.ts exists and works, and subsystem-D/E now run on it. Anyone reading the old note would go looking for something that is already there. The real blocker is a run environment this repo cannot supply: the portal worker on 8787 AND this worker on 8789 AND `stripe listen` forwarding real signed events, all at once. Playwright's webServer starts one server and globalSetup seeds one D1. Stays skipped, now with a TODO that names what would unblock it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
…ship today
Nine tests that had been skipped since the Alpine build now run and pass, on a
config that gives them a real home instead of an env var nobody sets.
SEED (tests/seed-fixtures.ts)
- inspector-half@seed.test, exported through SEED_EMAILS. The name says
"half" of the INSPECTION, not of a seat quota: the only spec that logs in as
it opens seed-half-done-inspection and asserts the publish pre-flight gates
fail, and nothing anywhere touches seats. It now owns that inspection.
- seed-empty-inspection and seed-republished-inspection are seeded
propertyType 'commercial'. This is load-bearing, not cosmetic: the editor
renders the units surface only when propertyType === 'commercial'
(showUnitsSurface), so without it the unit flows have nothing to drive.
- one inspector_credentials row, so a published report has a badge to render.
- SEED_INSPECTIONS / SEED_TENANT_SLUG exported so specs stop hardcoding ids.
WHAT CHANGED IN THE ASSERTIONS (all: the old contract no longer exists)
- D P1+P2: no Alpine [title="Add building"] + window.prompt. Units are the
UnitsManager drawer; scope selection is the BreadcrumbDropdown, and only in
per-unit mode. "selectedUnitId mirrors the click" was a trailing comment and
is now an assertion.
- D P7+P9: the republish summary prompt is NOT in the editor's PublishModal —
that modal has no summary field and never had one. It is the hub's
PublishReportModal (/inspections/:id), shown when the next publish would be
an amendment. "Send All" exists nowhere in this repo; the submit is
"Publish report".
- D P8: the page is /version-diff/:id?n=&from= — /inspections/:id/versions/:n/diff
is the API path and was never a page. Headings are "Version N Changes" +
Field/Before/After, not "v1 -> v2" / "Items changed".
- E P1: there is no publish modal with a disabled "Send All" and a gate
checklist; [data-test=publish-send-all] appears nowhere. The five-gate
aggregator shipped as GET /:id/preflight with NO frontend consumer, so the
test asserts it there.
- E P2: neither tab id existed. Live keys are all|active|requested|to_review|
awaiting_payment|published|cancelled — no "drafts", and snake_case. The
active tab is the DS token class text-ih-primary, not .bg-indigo-600.
- E P6: /settings/integrations (no -grid), owner-only (inspector gets
AccessDenied), and the six cards are QBO/GCal/Places/Resend/Zapier/Gemini —
Stripe Connect is its own panel now, not a card.
- E P7: charts are div bars, not svg polyline, and the findings surface is
"Findings by Section". The date range has to be stated or the card honestly
renders "No data in this date range" and the test asserts a heading over
nothing.
- E P8: /report-view/:tenant/:id, and it is NOT anonymously readable (the
public report API 404s without a recipient token or owner session). "Inspected
by" is now "Inspector: {name}"; there is no hardcoded NACHI badge — the
credential comes from the inspector's own row.
Also: P2-P8 of subsystem E had no login at all and were driving ANONYMOUS
pages, because Playwright gives each test a fresh context. Each logs in now, at
the role its surface requires.
RUN HOME (playwright.seeded.config.ts + npm run test:e2e:seeded + a CI step)
The seed writes users into TENANT_A, which IS the standalone workspace, and the
default run's api project asserts POST /api/auth/setup returns a fresh 200 —
which 409s the moment any user exists. The two are mutually exclusive in one
D1, so they are two runs sharing one worker, not two projects. The alternative,
skipping these nine unless SEED_E2E is set, was rejected: it puts them straight
back in the permanently-skipped column this work exists to empty.
Verified: 9 passed. Then falsified — reverting the commercial propertyType and
the credential row reds exactly P1+P2 and E-P8 and nothing else; dropping the
inspector-half row aborts the whole run through Task 1's new fatal seed path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
`verify` is the job branch protection keys off, and e2e was missing from its needs list. A red E2E run therefore left verify green — the suite ran, reported, and could not block a merge. Nobody decided E2E should be advisory; it just was. Costs verify the wait for e2e, which is the longest job at roughly five minutes. That is the price of the job meaning what its name says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125yvQL7kgEym1oR6hiqUAQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #296: one security advisory, and the gates and test suite that #296's review exposed as weaker than they looked.
The advisory
fast-uri— host confusion via a backslash authority introducer, high, runtime scope — surfaced after #296 merged. It arrives transitively through@modelcontextprotocol/sdk→ajv, so it takes an override. Pinned^3.1.5rather than>=3.1.5: the open range resolved to 4.1.2, and forcing a major on a transitive thatajv@8.20expects at 3.x is a way to break schema validation while fixing a URL parser.Gates that were not ratcheting
Dead-code baseline is now empty. Its two entries were the same declaration twice —
ERASURE_OUT_OF_SCOPEand its type, whichscripts/check-erasure-manifest.mjsreads out of the source text rather than importing, so knip was right that no module consumes it and wrong that it is dead. Marked per-export with a@gateConsumedtag rather than exempting the whole file, so a genuinely dead export in that manifest still gets reported.Status literals: 13 → 5. All eight server-side entries now reference the enum their column already declares. They turned out to be four different state machines, kept separate rather than collapsed — a visit is not the order it belongs to, and an outbox row's
published("handed to the queue") is not a report'spublished("delivered to the client"). The remaining five areapp/render branches.That gate also has a blind spot worth knowing about: its union-type guard
/^\s*\|/matches the|of a||too, so instatus === 'x' || status === 'y'every comparison but the last is silently dropped. Probing for it found one genuinely unseen literal, now fixed. The guard itself is left alone here — tightening it surfaces new hits, which is a separate change.verifydid not depend one2e. It is the job branch protection keys off, so a red Playwright run left it green: the suite ran, reported, and could not block a merge. Nobody decided E2E should be advisory. It costs verify the wait for the longest job, which is the price of the name being true.The skipped tests were hiding something worse than debt
41 skipped → 21. The nine that were unskipped had all been waiting on a "multi-user seed harness" that already existed. Once they ran, every one asserted a contract that does not ship:
[title="Add building"]control and awindow.promptfrom the Alpine build; the surface is a drawer/inspections/:id/versions/:n/diffas a page, when it is and always was an API pathGET /:id/preflightwith no frontend consumerThey would have been red the day they were written. The skip was presenting "we did not build this" as "this test cannot run yet." Also fixed: those specs never logged in at all, so they were driving anonymous pages at routes that bounce to
/login.Eleven more tests were deleted rather than repaired — they target Alpine-era files the React Router migration removed, and four had empty bodies, so they were placeholders inflating a count. Where that emptied a spec file, its
playwright.config.tsproject entry went too: a project whosetestMatchresolves to nothing is another way to report green over zero tests.A requested seed now fails loudly.
SEED_E2E=1seeding was wrapped in aconsole.warncatch, which is exactly what hid five separate defects in the seed fixtures until this week. When seeding is explicitly asked for, its failure aborts the run.The nine unskipped specs run under a second config (
npm run test:e2e:seeded) because the seed writes into the standalone tenant and the default run asserts a coldPOST /api/auth/setupreturns 200 — mutually exclusive in one D1. The alternative, skipping them unless an env var is set, would have put them straight back in the column this change exists to empty.Counts: 180 → 189 passing, 41 → 21 skipped, 11 dead tests removed.
🤖 Generated with Claude Code