diff --git a/AGENTS.md b/AGENTS.md index bc0110ea0..25e6e9a50 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ Hand-written, deliberately short. The *facts* about this codebase are generated ## What this repo is -The **Ultimate** web framework. Bun-only, Postgres, SolidJS, SCSS tokens. A monorepo of 27 `@ultimat3/*` packages — the `x` CLI among them — plus the unscoped `create-ultimate`. All 28 publish to npm at **1.0.0** in lockstep, `As of 2026-08`. +The **Ultimate** web framework. Bun-only, Postgres, SolidJS, SCSS tokens. A monorepo of 29 `@ultimat3/*` packages — the `x` CLI among them — plus the unscoped `create-ultimate`. All 30 publish to npm at **3.0.0** in lockstep, `As of 2026-08-19`. Run `bun run scripts/list-workspaces.ts` rather than trusting this line; `scripts/release-facts.ts` now fails the gate when it drifts. No ORM dependency, `As of 2026-08`: `entity()` is the one table declaration, and `postgresDriver()` (`packages/entity/src/pg-driver.ts`, `pg-sql.ts`) is hand-written parameterised SQL. Generated apps declare their tables the same way — `packages/db/` holds those declarations plus plain-SQL migrations. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d56f4b3b..a274fa3e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,12 @@ Semver applies from 1.0.0. A breaking change to a documented API needs a major ## [Unreleased] -Five bug sweeps since 3.0.0. The second was six independent auditors over the packages the 3.0.0 +Six bug sweeps since 3.0.0. The second was six independent auditors over the packages the 3.0.0 sweep did not reach; the third added a whole-repo security pass and an architecture review, and found more than the first two combined; the fourth closed the known-gaps backlog; the fifth -typechecked the 966 test files no compiler had ever read and then deleted what nothing calls. -**Twenty-two** of the entries below are breaking changes to documented APIs and one needs a +typechecked the 966 test files no compiler had ever read and then deleted what nothing calls; the +sixth closed the known-gaps backlog and mounted two gates that had been built and never wired. +**Twenty-six** of the entries below are breaking changes to documented APIs and one needs a migration run, so the next release is a major. Four findings are worth reading even if you skip the rest, because each was a guarantee the code @@ -21,6 +22,47 @@ stated in a comment and did not keep: every authenticated websocket carried `act built and handed to nobody, and `references(() => t.id, { onDelete: 'cascade' })` had type-checked since 1.0 while reaching no SQL at all. +### Removed — BREAKING (sixth sweep: the known-gaps backlog) + +- **BREAKING — `CaptureOptions.timeoutMs` and the public `CaptureRequest.timeout` are deleted from + `@ultimat3/scraping`** (#211). The port required it, `page-over-target.ts` threaded it, and **no + driver honoured it** — `cdp-target.ts` reads only `fullPage`, its `pdf` names the parameter + `_options`, and `html-target.ts` ignores it. Deleted rather than honoured for a reason worth + recording: the CDP port's own `screenshot({ fullPage })` has no timeout slot to forward to, and a + generic deadline in `page-over-target.ts` would have to race `ScrapeClock.sleep` — which under + `testClock` advances the clock and resolves on the first microtask, so the deadline would win + every race and **every capture in every test would time out**. A knob no driver honoured takes + nothing away when it goes; a deadline that fires in tests and not in production would. Both + layers went, because leaving the public `CaptureRequest.timeout` behind would make it the new lie. +- **BREAKING — `ScrapeTarget.click`'s `index` parameter is deleted** (#212). The HTML driver + honoured it and the CDP driver dropped it, and they agreed only because the sole caller passed + `0`. The decisive fact: **`index` was unreachable from the public vocabulary** — `ScrapeFrame.click` + takes `(selector, options?: WaitOptions)` and has no index, so no app could set it. A port member + no caller can reach, honoured by one driver and dropped by the other, is a divergence waiting for + an app to find. A three-driver parity test over a multi-match fixture now covers it. +- **BREAKING — `adminResource` no longer pluralises an entity name.** `@ultimat3/admin` carried a + private English pluralizer, and **every entity in both tracked apps is already named plural** — + so `entity('orgs')` was served at `/admin/orgses`, with `/admin/medias` and `/admin/likeses` + queued behind it. The fix is a deletion rather than a better pluralizer: which plural a name + takes is an app's convention, not a mechanism the framework can own (axiom 8), and `path:` was + always the override. Migration: an app relying on the old doubled URL sets `path:` explicitly. + +### Added (sixth sweep) + +- **`X_SCRAPE_YIELD_HISTORY_MISSING`** (#154) — `expect.maxDrop` measures a fall against a trailing + baseline, and with no `history:` store `expect.ts` reads `[]` forever, so `X_SCRAPE_YIELD_COLLAPSED` + could never fire. A silent no-op in a scrape *expectation* is a scrape that cannot fail. Refused + at declaration time, where it is written, before any attempt exists. `expect.minRows` is an + absolute floor checked *before* the history gate and stays legal on its own. +- **`StorageDriver.verifySigned` and `X_STORAGE_URL_UNVERIFIABLE`** (#145) — `acceptSignedUpload` + required a `secret:` that no route could supply, because the signing secret was closed over inside + `localDriver`'s factory. `StorageDriver` already hoisted `signedUrlBase` so "the minting half and + the verifying half cannot state it twice"; the secret was the other half of that pair and was + never hoisted. The new member exposes **verification, never the key** — a member returning the + secret would let any holder of a driver mint a URL for any key, and would put it in every + `JSON.stringify(disk)` a log performs. `acceptSignedUpload` now works with no secret in the call. + The `PUT` route is still unmounted; the seam that blocked it is not. + ### Removed — BREAKING (fifth sweep: delete what nothing calls) - **BREAKING — `PrecacheAsset.critical` is deleted from `@ultimat3/pwa`.** It was declared and read @@ -360,6 +402,63 @@ reach. Five of the entries below are **breaking changes to documented APIs**. - Everything else in PRs #179–#186, each of which names the defect it closes. +## 3.0.1 + +### Added + +- four conventions become build errors, and two were already broken +- typecheck the tests, on a ratchet — 966 files the gate never read (#208) +- canonicalJson, fingerprint and compareDecimalText — one home for three that had two (#191) + +### Fixed + +- the production image ran the dev server, and nothing built it (#215) +- two documented options that no caller could make effective (#207) +- both tracked apps could bake .env.production into their image (#206) +- secrets shipped inside the production image, and four other things x new got wrong (#205) +- delete a knob nothing read, stop echoing request bodies, and check the identity live queries depend on (#203) +- the generated schema told four lies, and drift could see none of them (#202) +- the error-render gate could not see the shape it let through six times (#198) +- the wedge watchdog's abort half was built and never wired (#197) +- the semantic cache defaulted to one store for every tenant (#196) +- qidOf was queryHash written twice, and the two already disagreed (#195) +- every Date collided into one cache key, and two tenants shared one dedupe slot (#194) +- a thrown code named toString made Pipeline.handle reject (#193) +- an allow list sent as a string matched every subject containing it (#192) +- 90 recorded ranges said 1.2.0 and 2.0.0 while every package.json said 3.0.0 (#190) +- deleting every scheduled task and dropping every primary key was a clean x verify (#185) +- the icon name from network data reached a filesystem path and a TypeScript identifier (#186) +- a permanent mail refusal was retried five times, and one document indexed as nine copies of one sentence (#184) +- every authenticated websocket carried actor: null, and the test stub hid it (#183) +- a limiter shed vanished from queue_depth, silencing the HPA exactly when saturated (#182) +- an actor holding a role named "constructor" turned every authz denial into a 500 (#181) +- an app shipping only Spanish served English error pages, and nothing could see it (#180) +- a declared field named toString was unsatisfiable for every input (#179) +- two admin actions could share a name and dispatch to the wrong one (#178) +- an ISR route with a policy served the first actor's HTML to everybody (#177) +- an unreadable TOTP secret verified against a code needing no secret (#176) +- a serialization retry budget of NaN ran the transaction zero times (#175) +- three of these bugs were reachable through a key nobody typed (#174) +- close the four deferred issues — and two of them were not what their issue said (#163) + +### Changed + +- a direct grant was unspellable, and userActor silently dropped it (#218) +- delete two declarations that describe features nothing implements (#217) +- the last 102 test typecheck errors, and RowPatch could not spell the value it refuses (#216) +- 229 more test typecheck errors, and the fetch seam no caller could fill (#214) +- the tests typecheck, and four of the errors were the type being wrong (#210) +- the gap registry said "fixed" about six things that were not, and "open" about fourteen that were (#209) +- replace a stale economy measurement with a reproducible one (#204) +- all 30 packages clear 95%, and six bugs found while getting there (#201) +- ui moves 5 → 4, and the admin → ui exception is deleted (#200) +- three sweeps, twelve breaking changes and one migration — the next release is a major (#199) +- how much code you do not write, and why the bugs matter more than the lines (#189) +- one job per package — its own tests, its own lint, its own coverage bar (#188) +- the second sweep landed five breaking changes, and the next release is a major (#187) +- 3.0.0 is on npm, and the publishers were re-attached, not attached (#162) + + ## 3.0.0 - 2026-08-19 The first release the workflow has published since 1.2.0. Every package has an OIDC trusted diff --git a/README.md b/README.md index 60dfc59d8..4e4041b23 100644 --- a/README.md +++ b/README.md @@ -68,9 +68,21 @@ do with the product. The framework's job is to have already decided those things spends its budget on the feature, and on a floor whose failure modes were found once, here, rather than once per app. +**Roughly 9× — that is the multiplier.** For every line of production code the author of +[`dummy/social-media-clone`](dummy/social-media-clone/) owns, the framework already runs **8.8** on +their behalf: 63,949 code lines across the 17 packages that app executes, against 7,227 of its own. +Counting everything hand-written including tests, against all framework source, it is **11.1 : 1**. + +Read that as *code you never own, never test and never fix* — **not** as "a DIY build of this app +would be nine times bigger". It would not be: a DIY build reaches for libraries too. What the +multiplier measures is how much of the surface has already been decided, and where a bug gets fixed +when one is found. The honest per-feature figure is sharper anyway: **~16 code lines buy a fully +projected endpoint** — input schema, output schema, policy, an OpenAPI operation, an MCP tool and a +typed client, all drift-checked. Write those six by hand once and the ratio stops being abstract. + **A line the app author never writes cannot carry a bug.** So the payoff is measured as how little the author owns. The worked example is [`dummy/social-media-clone`](dummy/social-media-clone/) — the -deployed demo, `As of 2026-08-19`, counted with `git ls-files` + `tokei 14.0.0`. Every figure below +deployed demo, `As of 2026-08-20`, counted with `git ls-files` + `tokei 14.0.0`. Every figure below is **code** lines, never `wc -l`: comments are 21% of this app's raw lines, so a raw count overstates it by a fifth. Where a raw number appears it is labelled raw. @@ -106,15 +118,15 @@ the five handles. The rest of the app holds the shape: | Tests | 45 | 2,482 | | SCSS | 25 | 1,658 | | the `en` catalog | 1 | 397 | -| config, Compose, Dockerfiles, env, shell | 31 | 379 | -| **Total** | **222** | **9,707** | -| production only — total minus tests | 177 | **7,225** | +| config, Compose, Dockerfiles, env, shell | 31 | 381 | +| **Total** | **222** | **9,709** | +| production only — total minus tests | 177 | **7,227** | | Framework : app | Ratio | |---|---| -| the **17 runtime packages this app executes** (63,754 code) : its production code (7,225) | **8.8 : 1** | -| all framework source (106,939 code, 1,073 files) : everything hand-written here (9,707) | **11.0 : 1** | -| the same, plus the framework's 142,922 lines of test code : hand-written (9,707) | 25.7 : 1 — test code on one side only, so read it as reassurance, not leverage | +| the **17 runtime packages this app executes** (63,949 code) : its production code (7,227) | **8.8 : 1** | +| all framework source (107,809 code, 1,080 files) : everything hand-written here (9,709) | **11.1 : 1** | +| the same, plus the framework's 144,046 lines of test code : hand-written (9,709) | 25.9 : 1 — test code on one side only, so read it as reassurance, not leverage | Re-derive all three (`tokei` 14.0.0; `node_modules`, `.x/`, `dist/` and `coverage/` are gitignored, so `git ls-files` never enters them): diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 3951db349..0c2360b8d 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -5,7 +5,7 @@ How Ultimate is built. [`../idea/`](../idea/README.md) answers *what and why*; t | Doc | Hook | |---|---| | [`00-conventions.md`](./00-conventions.md) | File layout, naming, export surface, LOC ceilings — the rules Biome and `x verify` enforce. | -| [`01-package-map.md`](./01-package-map.md) | 28 packages, 6 tiers, one reason to change each. What every package owns and must never do. | +| [`01-package-map.md`](./01-package-map.md) | 29 packages, 6 tiers, one reason to change each. What every package owns and must never do. | | [`02-boundaries.md`](./02-boundaries.md) | Tier violations are build errors. `scripts/boundaries.ts` resolves the transitive chain and prints it. | | [`03-request-lifecycle.md`](./03-request-lifecycle.md) | 17 ordered stages and why each sits where it does. ALS context, so no layer threads `actor`. | | [`04-error-contract.md`](./04-error-contract.md) | `UltimateError`: one object, three renderings. Every error carries an executable `fix:`. | diff --git a/dummy/social-media-clone/openapi.json b/dummy/social-media-clone/openapi.json index 0d847dde2..87c739c4d 100644 --- a/dummy/social-media-clone/openapi.json +++ b/dummy/social-media-clone/openapi.json @@ -605,6 +605,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "createAccount", @@ -664,6 +674,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "runAdminAction", @@ -744,6 +764,16 @@ } }, "description": "X_IDEMPOTENCY_CONFLICT" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Ask another user to be friends", @@ -824,6 +854,16 @@ } }, "description": "X_IDEMPOTENCY_CONFLICT" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Accept or decline a friend request addressed to the caller", @@ -883,6 +923,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Readiness of this process", @@ -942,6 +992,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Send a message to a conversation the actor is already in", @@ -1022,6 +1082,16 @@ } }, "description": "X_IDEMPOTENCY_CONFLICT" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Mark the acting user's notifications as read", @@ -1081,6 +1151,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "createSession", @@ -1140,6 +1220,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "destroySession", @@ -1220,6 +1310,16 @@ } }, "description": "X_IDEMPOTENCY_CONFLICT" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Block a user and decline any friendship between them", @@ -1279,6 +1379,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Lift a block this user placed", diff --git a/examples/dummy/apps/admin/src/index.test.ts b/examples/dummy/apps/admin/src/index.test.ts index e24850658..334aabc4b 100644 --- a/examples/dummy/apps/admin/src/index.test.ts +++ b/examples/dummy/apps/admin/src/index.test.ts @@ -76,32 +76,34 @@ describe('the agent surface is the same dashboard, projected again', () => { }); /** - * Two `@ultimat3/admin` defects, asserted AS THEY ARE so the gate stays honest rather than - * flattering. Both are the framework's to fix; when either is, this test goes red and whoever - * fixed it updates the line deliberately — which is the point of pinning them here. + * The double-pluralisation bug this block used to pin is **fixed**: `adminResource` no longer + * guesses English morphology, so an entity named `orgs` is served at `/admin/orgs`. Which plural + * a name takes is an app's convention, not a mechanism the framework can own (axiom 8), and + * `path:` was always the override. * - * 1. **Double pluralisation.** Every entity name here is already plural, and the route builder - * pluralises again: `orgs` -> `orgses`, `members` -> `memberses`. Every admin URL this app - * would serve is misspelled. - * 2. **The nav omits `basePath`.** `nav` links to `/orgses` while the route is `/admin/orgses`, - * so every navigation link in the dashboard points at a path no route matches. This is the - * one that makes the dashboard unusable rather than merely ugly, and it is invisible to any - * test that checks the nav or the routes alone — it only shows when the two are compared. + * The nav "bug" it also pinned **was never one**, and this is the more useful lesson. The old + * assertion compared `nav` hrefs against route paths and found them disjoint — but hrefs are + * base-relative by design and `layout.tsx:82` composes `${basePath}${item.href}`, which + * `layout.test.ts` has always asserted. Comparing an uncomposed href to a composed path is + * disjoint by construction, so the test passed while proving nothing. It is replaced below by the + * invariant that does matter: every nav href must resolve to a real route AFTER composition. */ -describe('known @ultimat3/admin defects, pinned so they cannot be forgotten', () => { - test('entity names are pluralised twice in every route path', () => { +describe('routes are the entity names, not a guess about English', () => { + test('an already-plural entity name is served once, not twice', () => { const paths = admin.routes.map((route) => route.path); - expect(paths).toContain('/admin/orgses'); - expect(paths).toContain('/admin/memberses'); - expect(paths).not.toContain('/admin/orgs'); + expect(paths).toContain('/admin/orgs'); + expect(paths).toContain('/admin/members'); + expect(paths).not.toContain('/admin/orgses'); }); - test('no nav href matches any route path, because nav drops the base path', () => { - const paths = new Set(admin.routes.map((route) => route.path)); + test('nav hrefs are base-relative, and the layout is what makes them absolute', () => { const hrefs = admin.nav.flatMap((group) => group.items.map((item) => item.href)); + const paths = admin.routes.map((route) => route.path); expect(hrefs.length).toBeGreaterThan(0); - expect(hrefs.filter((href) => paths.has(href))).toEqual([]); - expect(hrefs).toContain('/orgses'); + expect(hrefs).toContain('/orgs'); expect(admin.basePath).toBe('/admin'); + for (const href of hrefs) { + expect(paths).toContain(`${admin.basePath}${href}`); + } }); }); diff --git a/examples/dummy/openapi.json b/examples/dummy/openapi.json index 32aad9c85..e1861760d 100644 --- a/examples/dummy/openapi.json +++ b/examples/dummy/openapi.json @@ -1258,6 +1258,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "grantAvatarUpload", @@ -1317,6 +1327,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "memberAvatar", @@ -1376,6 +1396,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Comment on a post the actor can read", @@ -1438,6 +1468,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Toggle the acting member’s digest subscription", @@ -1499,6 +1539,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Invite a person to the actor’s organisation", @@ -1561,6 +1611,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Move the organisation to a higher plan and return the prorated receipt", @@ -1623,6 +1683,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Create a draft post in the actor’s organisation", @@ -1684,6 +1754,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Like a post on behalf of the acting member", @@ -1746,6 +1826,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Publish a draft post", @@ -1809,6 +1899,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Update the acting member’s locale, timezone, theme and digest opt-in", @@ -1870,6 +1970,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "contactSales", @@ -1932,6 +2042,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Summarise a post into two sentences and up to four tags", @@ -1991,6 +2111,16 @@ } }, "description": "policy denied" + }, + "422": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "X_BODY_INVALID" } }, "summary": "Set the acting member’s theme", diff --git a/framework.manifest.json b/framework.manifest.json index 5173f4a31..a11124ac8 100644 --- a/framework.manifest.json +++ b/framework.manifest.json @@ -1,6 +1,6 @@ { "version": 1, - "buildId": "dbfedc3aa6d6b65e546daffc0d9477c55f27d7bc6502c5ed9d95ea8576e9a0a6", + "buildId": "cb4f65b68b6623b4e3b4aff5012d499c02ea4ae7dbc186d10175d51f82c7222e", "tiers": { "0": [ "core", @@ -252,6 +252,11 @@ "owner": "action", "at": "packages/action/src/errors.ts" }, + { + "code": "X_ACTION_JOB_UNBRIDGED", + "owner": "jobs", + "at": "packages/jobs/src/errors.ts" + }, { "code": "X_ACTION_PATH_DUPLICATE", "owner": "action", @@ -782,6 +787,16 @@ "owner": "scripts", "at": "scripts/doc-commands.ts" }, + { + "code": "X_DOC_FILE_COUNT_STALE", + "owner": "scripts", + "at": "scripts/generator-counts.ts" + }, + { + "code": "X_DOC_FILE_COUNT_UNSCANNED", + "owner": "scripts", + "at": "scripts/generator-counts.ts" + }, { "code": "X_DOC_FIX_UNRUNNABLE", "owner": "scripts", @@ -792,6 +807,16 @@ "owner": "scripts", "at": "scripts/doc-fixes.ts" }, + { + "code": "X_DOC_RELEASE_FACT_STALE", + "owner": "scripts", + "at": "scripts/release-facts.ts" + }, + { + "code": "X_DOC_RELEASE_FACT_UNSCANNED", + "owner": "scripts", + "at": "scripts/release-facts.ts" + }, { "code": "X_DRAINING", "owner": "core", @@ -1132,6 +1157,11 @@ "owner": "jobs", "at": "packages/jobs/src/errors.ts" }, + { + "code": "X_JOB_ROW_STATUS_UNKNOWN", + "owner": "jobs", + "at": "packages/jobs/src/errors.ts" + }, { "code": "X_JOB_SLOT_LOST", "owner": "jobs", @@ -1767,11 +1797,21 @@ "owner": "scripts", "at": "scripts/registry-audit.ts" }, + { + "code": "X_RELEASE_FLAG_UNKNOWN", + "owner": "scripts", + "at": "scripts/release.ts" + }, { "code": "X_RELEASE_VERSION_SKEW", "owner": "cli", "at": "packages/cli/src/error-codes.ts" }, + { + "code": "X_RELEASE_VERSION_UNSTATED", + "owner": "scripts", + "at": "scripts/release.ts" + }, { "code": "X_REPLICATION_FAILED", "owner": "realtime", @@ -2052,6 +2092,11 @@ "owner": "scraping", "at": "packages/scraping/src/errors.ts" }, + { + "code": "X_SCRAPE_YIELD_HISTORY_MISSING", + "owner": "scraping", + "at": "packages/scraping/src/errors.ts" + }, { "code": "X_SECRETS_EDIT_FAILED", "owner": "cli", @@ -2252,6 +2297,11 @@ "owner": "storage", "at": "packages/storage/src/errors.ts" }, + { + "code": "X_STORAGE_URL_UNVERIFIABLE", + "owner": "storage", + "at": "packages/storage/src/errors.ts" + }, { "code": "X_STYLES_GLOBAL_MISSING", "owner": "render", @@ -2327,6 +2377,21 @@ "owner": "cli", "at": "packages/cli/src/error-codes.ts" }, + { + "code": "X_TEST_FIX_PIN_STALE", + "owner": "scripts", + "at": "scripts/test-fix-citations.ts" + }, + { + "code": "X_TEST_FIX_UNRUNNABLE", + "owner": "scripts", + "at": "scripts/test-fix-citations.ts" + }, + { + "code": "X_TEST_FIX_UNSCANNED", + "owner": "scripts", + "at": "scripts/test-fix-citations.ts" + }, { "code": "X_TEST_FIXTURE_UNAVAILABLE", "owner": "testing", @@ -2382,6 +2447,11 @@ "owner": "cli", "at": "packages/cli/src/error-codes.ts" }, + { + "code": "X_TEST_THROW_NOT_THROWN", + "owner": "scripts", + "at": "scripts/to-throw-returns.ts" + }, { "code": "X_TEST_TYPECHECK_PIN_STALE", "owner": "scripts", diff --git a/packages/action/src/http.test.ts b/packages/action/src/http.test.ts index 91a68ba69..ff9a00f67 100644 --- a/packages/action/src/http.test.ts +++ b/packages/action/src/http.test.ts @@ -4,7 +4,7 @@ import { describe, expect, test } from 'bun:test'; import type { Actor } from '@ultimat3/core'; -import { userActor } from '@ultimat3/core'; +import { createContext, isUltimateError, runWithContext, userActor } from '@ultimat3/core'; import type { HttpConfig } from '@ultimat3/http'; import { createServer, defineHttpConfig, setRedirect } from '@ultimat3/http'; import type { Actor as PolicyActor } from '@ultimat3/policy'; @@ -12,7 +12,8 @@ import { allow, can } from '@ultimat3/policy'; import { t } from '@ultimat3/schema'; import type { AnyAction } from './action'; import { action } from './action'; -import { toRoute } from './http'; +import { toOpenApiOperation, toRoute } from './http'; +import { invoke } from './invoke'; const Input = t.object({ postId: t.uuid }); const Output = t.object({ id: t.uuid, published: t.boolean }); @@ -46,13 +47,10 @@ function publisher(authorId: string, evaluations: { count: number }) { */ const oneProcess = (): HttpConfig => defineHttpConfig({ rateLimit: { scope: 'process' } }); -// `permissions` — direct grants, bypassing roles — is a field of POLICY's actor -// (`CoreActor & PolicyActorFields`), which is what `can()` reads through `actorHas`. Core's -// `Actor` has none and cannot: core is tier 0 and knows nothing about grants. -const editor = (id: string): PolicyActor => ({ - ...userActor({ id }), - permissions: ['post:publish'], -}); +// `permissions` — direct grants, bypassing roles — is what `can()` reads through `actorHas`, and +// it is core's field now, so the builder carries it and the actor comes back FROZEN. The spread +// this used to be produced an unfrozen actor, a shape no request ever mints. +const editor = (id: string): PolicyActor => userActor({ id, permissions: ['post:publish'] }); function serve(target: ReturnType, actor: Actor | null) { return createServer({ @@ -238,3 +236,80 @@ describe('an action answering a form post', () => { expect((await call()).status).toBe(303); }); }); + +// Axiom 2, enforced instead of described: the operation an app publishes and the response the +// server sends are two projections of ONE action, so a client written against the document has to +// be right. The expectation is READ OUT OF the published operation rather than written down here +// — a test that hardcoded `400` would pass just as happily if both halves drifted together. +describe('the published operation and the live response are one contract', () => { + const target = publisher('u1', { count: 0 }); + + const badBody = (body: string): Promise => + createServer({ + routes: [toRoute(target)], + config: oneProcess(), + hooks: { authenticate: () => editor('u1') }, + }).fetch( + new Request('http://dev.test/api/posts/publish', { + method: 'POST', + headers: { 'content-type': 'application/json', origin: 'http://dev.test' }, + body, + }), + ); + + // Four shapes of "the body is not what this action declared", because the divergence was not an + // edge case: every one of them answered 422 X_BODY_INVALID while the document promised 400. + // Two DIFFERENT failures, which is the half the original divergence hid. The first three + // parsed and then failed this action's schema; the fourth never became a body at all, and that + // is raised in `bodyRaw()` before a schema exists to fail — so it keeps `X_BODY_INVALID`, and + // the operation publishes both. + const REFUSED: readonly (readonly [string, string, string])[] = [ + ['a field of the wrong type', JSON.stringify({ postId: 'not-a-uuid' }), 'X_INPUT_INVALID'], + ['a missing required field', JSON.stringify({}), 'X_INPUT_INVALID'], + ['a JSON null where an object belongs', 'null', 'X_INPUT_INVALID'], + ['a body that is not JSON at all', '{', 'X_BODY_INVALID'], + ]; + + test('every refusal answers a status the operation actually publishes', async () => { + const published = Object.keys(toOpenApiOperation(target).responses); + + for (const [label, body] of REFUSED) { + const response = await badBody(body); + expect(published, `${label}: ${response.status} is not in the published operation`).toContain( + String(response.status), + ); + } + }); + + test('and it is the code the operation names for that status', async () => { + const responses = toOpenApiOperation(target).responses; + + for (const [label, body, code] of REFUSED) { + const response = await badBody(body); + const problem: unknown = await response.json(); + expect((problem as { readonly code: string }).code, label).toBe(code); + const declared = responses[String(response.status)]; + // The description is `problemResponse(code)`'s own text, so the document names the code. + expect(JSON.stringify(declared), label).toContain(code); + } + }); + + // The half a contract test cannot see, and the reason this is `X_INPUT_INVALID` and not a + // renamed 422: the SAME action invoked without HTTP answers the same code. An action whose + // input error depends on the surface it arrived through is two actions. + test('a direct invocation refuses with the same code the route does', async () => { + const overHttp: unknown = await (await badBody(JSON.stringify({ postId: 'nope' }))).json(); + let direct = 'resolved'; + try { + // In a context, because `invoke` needs one before it validates anything — the point here is + // the CODE the same input produces off the wire, not what an unhosted call does. + await runWithContext(createContext({ actor: editor('u1') }), () => + invoke(target, { postId: 'nope' }, { surface: 'http' }), + ); + } catch (error) { + direct = isUltimateError(error) ? error.code : String(error); + } + expect(direct).toBe((overHttp as { readonly code: string }).code); + expect(direct).toBe('X_INPUT_INVALID'); + }); +}); diff --git a/packages/action/src/http.ts b/packages/action/src/http.ts index e492e6ca3..f177e25f2 100644 --- a/packages/action/src/http.ts +++ b/packages/action/src/http.ts @@ -95,7 +95,17 @@ export function toRoute(target: AnyAction): Route { // stage deciding first would decide from `row: null` — a denial for the row's own // author, from an authz system that never saw the row. enforcedBy: 'handler', - input: def.input, + // `input` stays ABSENT, deliberately, exactly as `@ultimat3/query`'s `toQueryRoute` leaves it. + // Setting it hands the action's schema to the pipeline's `body` stage, which throws + // `bodyInvalid` — so this route answered `422 X_BODY_INVALID` for every malformed body while + // the operation two hundred lines down published `400 X_INPUT_INVALID`, and the SAME action + // called over MCP, over the typed client or directly answered `X_INPUT_INVALID`. One action, + // one input schema, two codes, decided by the surface the call arrived through. + // + // `invoke`'s own `validateInput` is the one parser now, and nothing is lost by dropping this: + // `bodyRaw()` still parses by content-type, caches, and enforces the size cap — the body stage + // only ever added SCHEMA validation on top. `X_BODY_INVALID` keeps its job, which is a body + // failing a plain `route.ts`'s own schema, where no primitive owns the input. cache: { mode: 'no-store', tags: tagKeys(def.cache?.invalidates ?? []) }, tags: [resource], // Name AND numbers. The name alone selected a bucket the limiter's table never held, so @@ -145,7 +155,15 @@ export function toOpenApiOperation(target: AnyAction): OpenApiOperation { description: 'ok', content: { 'application/json': { schema: { $ref: schemaRef(outputSchemaName(name)) } } }, }, + // BOTH, because they are two different failures and this operation published only one of + // them while the route answered only the other. `X_INPUT_INVALID` is the body that parsed + // and failed THIS action's declared schema — the primitive's own code, identical over MCP, + // the typed client and a direct call. `X_BODY_INVALID` is the bytes never becoming a body + // at all: malformed JSON, over `bodyLimitBytes`, an unreadable content type. That one is + // HTTP-only, because only HTTP has bytes, and it is raised in `bodyRaw()` before any schema + // exists to fail. '400': problemResponse('X_INPUT_INVALID'), + '422': problemResponse('X_BODY_INVALID'), '403': problemResponse('policy denied'), ...(idempotent ? { '409': problemResponse('X_IDEMPOTENCY_CONFLICT') } : {}), }, diff --git a/packages/action/src/job-handle.test.ts b/packages/action/src/job-handle.test.ts index a318df539..8dd76c5ce 100644 --- a/packages/action/src/job-handle.test.ts +++ b/packages/action/src/job-handle.test.ts @@ -102,3 +102,31 @@ describe('toJobHandle', () => { expect(ran).toBe(false); }); }); + +// The type is the first refusal, and the one an author actually meets: `.job()` is named `job`, +// so the reach for `.enqueue()` is the natural next keystroke. It is a compile error, and +// `@ts-expect-error` IS the assertion — it stops compiling the day the field appears, which is the +// day someone has quietly made this look enqueueable without seating it through `job()`. +// `registerJobs()` in `@ultimat3/jobs` is the second refusal, for the untyped path. +describe('an action projection cannot be mistaken for a job handle', () => { + test('there is no enqueue on it, at compile time', () => { + const handle = toJobHandle(publishPost()); + // @ts-expect-error `ActionJobHandle` has no `enqueue` — `agentJob()` is what produces one. + const enqueue: unknown = handle.enqueue; + expect(enqueue).toBeUndefined(); + // The literal is deliberately not `'job'`, which is what lets `registerJobs` recognise it + // across a tier boundary it may never import across. + expect(handle.kind).toBe('action-job'); + }); + + test('and nothing on it is a job field the queue would read', () => { + const handle = toJobHandle(publishPost()); + expect(Object.keys(handle).sort()).toEqual([ + 'idempotencyKey', + 'input', + 'invoke', + 'kind', + 'name', + ]); + }); +}); diff --git a/packages/admin/src/nav.test.ts b/packages/admin/src/nav.test.ts index 0966ebafd..ba846a3ac 100644 --- a/packages/admin/src/nav.test.ts +++ b/packages/admin/src/nav.test.ts @@ -66,11 +66,22 @@ describe('adminNav groups the resources', () => { expect(item).toEqual({ key: 'post', labelKey: 'admin.post.title', - href: '/posts', + href: '/post', entity: 'post', }); }); + /** + * The half the test above is named for and did not check: `href` is the resource's OWN path, + * copied, not a URL this file assembles. It read `/posts` for an entity named `post` until + * `adminResource` stopped pluralising — a guessed URL asserted by a test that says it forbids + * them. Reading it off the resource is what makes the two impossible to drift. + */ + test('href is the resource path verbatim, whatever the resource says it is', () => { + const custom = adminResource(shaped('post'), { path: '/editorial/pieces' }); + expect(adminNav([custom])[0]?.items[0]?.href).toBe('/editorial/pieces'); + }); + test('an explicit groups map decides both order and membership', () => { const nav = adminNav(RESOURCES, { groups: { 'admin.group.content': ['tag', 'post'] }, diff --git a/packages/admin/src/registry.test.ts b/packages/admin/src/registry.test.ts index 8b8e411f3..226cda304 100644 --- a/packages/admin/src/registry.test.ts +++ b/packages/admin/src/registry.test.ts @@ -114,7 +114,7 @@ describe('defineAdmin over entities built by entity()', () => { ]); expect(resource.idField).toBe('id'); expect(resource.labelField).toBe('title'); - expect(resource.path).toBe('/admin_reg_posts'); + expect(resource.path).toBe('/admin_reg_post'); }); test('widgets follow the column kinds the entity declared', () => { @@ -144,7 +144,7 @@ describe('defineAdmin over entities built by entity()', () => { test('both entities get their routes, and the nav lists both', () => { expect(app.resources.map((each) => each.name)).toEqual(['admin_reg_org', 'admin_reg_post']); - expect(app.routes.map((route) => route.path)).toContain('/admin/admin_reg_posts/:id'); + expect(app.routes.map((route) => route.path)).toContain('/admin/admin_reg_post/:id'); }); }); diff --git a/packages/admin/src/resource.test.ts b/packages/admin/src/resource.test.ts index 37e5c7925..0c34a651a 100644 --- a/packages/admin/src/resource.test.ts +++ b/packages/admin/src/resource.test.ts @@ -28,6 +28,16 @@ const authors = entity('admin_res_author', { columns: { id: uuid().primaryKey(), name: text({ max: 80 }) }, }); +/** Named the way both tracked apps name every entity: plural already. */ +const plurals = entity('admin_res_orgs', { + columns: { id: uuid().primaryKey(), name: text({ max: 80 }) }, +}); + +/** A name no English pluralizer gets right, and the reason not to ship one. */ +const irregular = entity('admin_res_media', { + columns: { id: uuid().primaryKey(), name: text({ max: 80 }) }, +}); + const post = entity('admin_res_post', { columns: { id: uuid().primaryKey(), @@ -81,12 +91,35 @@ describe('adminResource with zero config', () => { test('derives every field from the entity columns', () => { expect(resource.fields.map((field) => field.name)).toEqual(Object.keys(post.$columns)); expect(resource.name).toBe('admin_res_post'); - expect(resource.path).toBe('/admin_res_posts'); + // VERBATIM, never pluralised. `$name` is the entity's own name and this package does not + // guess English morphology on it — see the plural-name test below for what that cost. + expect(resource.path).toBe('/admin_res_post'); expect(resource.titleKey).toBe('admin.admin_res_post.title'); expect(resource.idField).toBe('id'); expect(resource.labelField).toBe('title'); }); + /** + * The bug this replaced an English pluralizer to close. Every entity in both tracked apps is + * named PLURAL — `entity('orgs')`, `entity('posts')`, `entity('media')`, 19 of 19 — and + * `adminResource` pluralised the name AGAIN, so the reference app's admin served `/orgses`, + * `/postses`, `/commentses` and would have served `/medias` and `/likeses`. Every URL the + * dashboard generates was misspelled. + * + * The fix is a deletion, not a better pluralizer: which plural a name takes is an app's + * convention (axiom 8), the framework ships the mechanism, and `path:` is the override that was + * always there. It also removes one of the three disagreeing pluralizers the repo's dead-code + * audit names — this package's private copy. + */ + test('an already-plural entity name is not pluralised a second time', () => { + expect(adminResource(plurals).path).toBe('/admin_res_orgs'); + expect(adminResource(irregular).path).toBe('/admin_res_media'); + }); + + test('an explicit path: still wins, which is how an app spells its own plural', () => { + expect(adminResource(plurals, { path: '/organisations' }).path).toBe('/organisations'); + }); + test('maps each column kind to its one widget', () => { expect(resource.field('title').widget).toBe('text-input'); expect(resource.field('body').type).toBe('textarea'); diff --git a/packages/admin/src/resource.ts b/packages/admin/src/resource.ts index da291d220..b8552ad03 100644 --- a/packages/admin/src/resource.ts +++ b/packages/admin/src/resource.ts @@ -69,7 +69,16 @@ export interface AdminResourceOptions { export interface AdminResource { readonly name: string; - /** Mount-relative, e.g. `/posts`. */ + /** + * Mount-relative, e.g. `/posts` — the layout composes it with `AdminApp.basePath`, so this is + * never the URL on its own. + * + * Defaults to the entity's `$name` VERBATIM. It used to be `pluralize($name)`, which is why the + * reference app's dashboard served `/orgses`, `/postses` and `/commentses`: every entity in both + * tracked apps is already named plural (19 of 19), so the heuristic ran a second time on a word + * that had already had it. Which plural a name takes is an app's convention and not a mechanism + * the framework can own (axiom 8) — `path:` is how an app spells its own. + */ readonly path: string; readonly titleKey: string; readonly group: string; @@ -90,12 +99,6 @@ export interface AdminResource { field(name: string): AdminField; } -const pluralize = (name: string): string => { - if (/[sxz]$/.test(name) || /(ch|sh)$/.test(name)) return `${name}es`; - if (/[^aeiou]y$/.test(name)) return `${name.slice(0, -1)}ies`; - return `${name}s`; -}; - function deriveField( entity: AdminEntity, column: AdminColumnFacts, @@ -263,7 +266,7 @@ export function adminResource( const resource: AdminResource = { name: entity.$name, - path: opts.path ?? `/${pluralize(entity.$name)}`, + path: opts.path ?? `/${entity.$name}`, titleKey: opts.titleKey ?? `admin.${entity.$name}.title`, group: opts.group ?? 'admin.group.data', idField, diff --git a/packages/cli/src/dev-render.test.ts b/packages/cli/src/dev-render.test.ts index 7517b09ac..741be6c4b 100644 --- a/packages/cli/src/dev-render.test.ts +++ b/packages/cli/src/dev-render.test.ts @@ -94,6 +94,28 @@ describe('unit · x dev renders the app routes', () => { expect(await cacheControl('/feed')).toBe('private, no-store'); }); + test('two ISR URLs that differ only in their query are two documents, never one', async () => { + // #171, the half a build error cannot close. `resultFor` keyed the ISR store on + // `url.pathname`, so `?page=2` and `?page=3` were ONE entry: the first render was stored under + // `/pricing` and every later query string was served that body. + // + // ONE server for both requests, deliberately. `serve()` builds a fresh `IsrController` per + // call, so a test that used it twice could never observe a cache hit at all — it would pass + // against the leak and against the fix alike. The route's own `meta` renders `data.url`, so + // the served document names the URL it was rendered for and a collision is visible from + // outside rather than inferred from the store. + register({ file: 'apps/web/site/pricing/page.tsx', render: 'isr', revalidate: { ttl: '5m' } }); + const server = serve(); + const fetchOnce = async (path: string): Promise => + await (await server.fetch(new Request(`http://dev.test${path}`))).text(); + + const second = await fetchOnce('/pricing?page=2'); + const third = await fetchOnce('/pricing?page=3'); + + expect(second).toContain('rendered http://dev.test/pricing?page=2'); + expect(third).toContain('rendered http://dev.test/pricing?page=3'); + }); + test('a dynamic segment reaches meta as a param, matched by the router', async () => { register({ file: 'apps/web/site/blog/[slug]/page.tsx', render: 'ssr' }); const body = await (await get('/blog/hello-world')).text(); diff --git a/packages/cli/src/dev-render.ts b/packages/cli/src/dev-render.ts index 904fe0ee8..5566059f8 100644 --- a/packages/cli/src/dev-render.ts +++ b/packages/cli/src/dev-render.ts @@ -24,6 +24,7 @@ import { createIsrController, headFromMeta, hydrateRuntime, + isrKey, metaContextFor, renderComponent, renderHead, @@ -185,7 +186,11 @@ async function resultFor( return { status: 200, headers: staticHeaders(contentHash(body), options.buildId), body }; } case 'isr': { - const served = await isr.serve(url.pathname, () => + // `isrKey(url)`, never `url.pathname`: the query is part of what was rendered — this + // route's own `meta` reads `data.url` — so two URLs differing only in their query are two + // documents. Keyed on the pathname alone, the first render answered every later query + // string (#171). Render owns the derivation so no second caller can invent another. + const served = await isr.serve(isrKey(url), () => documentFrom(entry, request, data, options), ); return served.result; diff --git a/packages/cli/src/generate-format.test.ts b/packages/cli/src/generate-format.test.ts new file mode 100644 index 000000000..1ceae5533 --- /dev/null +++ b/packages/cli/src/generate-format.test.ts @@ -0,0 +1,119 @@ +// Every file `x g` emits, put through the repo's OWN biome — the same rules the `lint` step of +// `x verify` runs. `x g` hand-wraps its output rather than formatting it, so a generated file is +// formatter-clean by construction and nothing proved the construction was right (#127). +// +// A build error rather than a runtime format, and the numbers are why: `biome check --write +// --stdin-file-path` costs ~239ms per file, and one `x g resource` emits 24 ts/tsx files — 5.7s +// added to a command that is currently instant. Batching to one subprocess is fast enough, but it +// puts a formatter INSIDE `x g`, where a version skew or an unhappy app config turns "write eight +// files" into a failure the author cannot act on. The defect is introduced HERE, when a template +// is edited; catching it here costs one subprocess, once, and nothing at all on a user's machine. + +import { afterAll, expect, test } from 'bun:test'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { GENERATORS, generate } from './cmd-generate'; +import { planNewApp } from './cmd-new'; +import { exec } from './exec'; +import type { GeneratedFile } from './templates'; + +const REPO_ROOT = join(import.meta.dir, '..', '..', '..'); +const BIOME_BIN = join(REPO_ROOT, 'node_modules', '.bin', 'biome'); + +const roots: string[] = []; + +afterAll(async () => { + for (const root of roots) await rm(root, { recursive: true, force: true }); +}); + +/** + * The repo's own `biome.json`, minus `vcs`. The RULES are the thing under test and they are + * copied byte for byte; `vcs.useIgnoreFile` is the one key that needs a git checkout to resolve, + * and biome refuses to start without one — so a sandbox that kept it would fail on the harness + * rather than on the output. There is nothing in a fresh temp directory to ignore. + */ +interface Sandbox { + /** Holds the emitted files and NOTHING else. */ + readonly tree: string; + /** Holds the config, OUTSIDE the tree: biome checks its own `biome.json`, and a copy written + * beside the output would be the one file this test reports on. */ + readonly configDir: string; +} + +async function sandbox(files: readonly GeneratedFile[]): Promise { + const base = await mkdtemp(join(tmpdir(), 'x-generate-format-')); + roots.push(base); + const tree = join(base, 'tree'); + const configDir = join(base, 'config'); + const config = (await Bun.file(join(REPO_ROOT, 'biome.json')).json()) as Record; + delete config['vcs']; + await Bun.write(join(configDir, 'biome.json'), JSON.stringify(config, null, 2)); + for (const file of files) { + if (typeof file.contents !== 'string') continue; + await Bun.write(join(tree, file.path), file.contents); + } + return { tree, configDir }; +} + +interface BiomeRun { + /** Empty when clean. The whole report otherwise, so a failure names the file and the rule. */ + readonly problems: string; + /** How many files biome actually READ. `0` is the vacuous pass this number exists to refuse. */ + readonly checked: number; +} + +/** Biome over the whole emitted tree in ONE subprocess — 48 files in ~70ms, against 239ms each. */ +async function runBiome(files: readonly GeneratedFile[]): Promise { + const { tree, configDir } = await sandbox(files); + // The repo's OWN biome binary, by path. `bunx biome` resolves against the CWD, which here is a + // temp directory with no `node_modules` — so it downloaded a fresh copy and answered with the + // installer's output instead of the checker's. This test is about the version `x verify`'s + // `lint` step runs; naming the binary is the only way to be sure it is that one. + const result = await exec([BIOME_BIN, 'check', '--config-path', configDir, '.'], { cwd: tree }); + const output = `${result.stdout}\n${result.stderr}`; + return { + problems: result.ok ? '' : output, + // Read back off biome's own summary line rather than assumed from the write loop: a sandbox + // biome declined to walk reports `ok` over nothing, and this test would agree with it. + checked: Number(/Checked (\d+) files?/.exec(output)?.[1] ?? '0'), + }; +} + +/** + * Unique `.ts`/`.tsx` paths in a file set — the floor `checked` has to clear. + * + * UNIQUE, because several generators contribute to one path (every kind that writes an i18n key + * targets the same catalog), so the emitted list holds duplicates and the tree holds one file. + * TypeScript only, because this is a vacuity guard and not an audit of biome's file-type set: + * a run that walked fewer files than there are TypeScript files skipped one. + */ +const typescriptPaths = (files: readonly GeneratedFile[]): number => + new Set( + files + .filter((file) => typeof file.contents === 'string' && /\.tsx?$/.test(file.path)) + .map((file) => file.path), + ).size; + +test('every file x g emits is already clean under the repo own biome', async () => { + const emitted = GENERATORS.flatMap((kind) => + generate({ kind, name: 'invoice', feature: 'invoice' }), + ); + // The count is asserted too: an empty list would make the check below vacuously green, which is + // exactly the shape of "clean by luck" this test exists to replace. + expect(emitted.length).toBeGreaterThan(50); + + const run = await runBiome(emitted); + expect(run.problems).toBe(''); + expect(run.checked).toBeGreaterThanOrEqual(typescriptPaths(emitted)); +}, 60_000); + +test('every file x new writes is clean too — the scaffold is generated code as well', async () => { + const scaffold = planNewApp({ name: 'ledger-demo', example: true }); + + expect(scaffold.length).toBeGreaterThan(50); + + const run = await runBiome(scaffold); + expect(run.problems).toBe(''); + expect(run.checked).toBeGreaterThanOrEqual(typescriptPaths(scaffold)); +}, 60_000); diff --git a/packages/cli/src/seo-meta.test.ts b/packages/cli/src/seo-meta.test.ts index 57fd92ea2..b6f7dc73a 100644 --- a/packages/cli/src/seo-meta.test.ts +++ b/packages/cli/src/seo-meta.test.ts @@ -3,7 +3,7 @@ // missing metadata — a gate that turns "I could not look" into "you forgot a title" is worse than // no gate, because the fix it hands the reader is for a defect that is not there. -import { afterEach, describe, expect, test } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import type { RegisterRouteInput } from '@ultimat3/render'; import { clearRoutes, defineRoute, registerRoute } from '@ultimat3/render'; import { validateMeta } from '@ultimat3/seo'; @@ -43,6 +43,15 @@ function register(fixture: Fixture): void { registerRoute(input); } +// BOTH halves, and the `beforeEach` is the one that matters. `routeEntries()` is a PROCESS-global +// registry, so this file's subject is whatever the process happens to hold — and a `packages/render` +// suite that registers routes and never clears them made these assertions fail only when the two +// packages were run in one `bun test` invocation, which is not how the gate shards them. A file +// that reads a global registry starts from a known state; it does not inherit one. +beforeEach(() => { + clearRoutes(); +}); + afterEach(() => { clearRoutes(); }); diff --git a/packages/http/src/error-map.ts b/packages/http/src/error-map.ts index 081b5c52a..5c48fe251 100644 --- a/packages/http/src/error-map.ts +++ b/packages/http/src/error-map.ts @@ -132,6 +132,18 @@ export const ERROR_STATUS = { // at it), which 422 describes only for the insert. X_DB_UNIQUE_VIOLATION: 409, X_DB_FOREIGN_KEY_VIOLATION: 409, + // @ultimat3/jobs — the ONE jobs code with a row here, and the reason the rest are pinned in + // `scripts/error-map-backlog.ts` does not cover it. That pin says "a job runs with no socket + // attached; `ROLE=worker` opens no HTTP port at all" — true of `X_JOB_TIMEOUT` and every other + // worker-runtime code, and NOT true of a decode failure: `toJobRecord` runs wherever a row is + // READ, which includes the admin dashboard's job panel and `x jobs show` served over HTTP. + // 500, and it should page: a queue holding rows this build cannot read is an operator's + // problem, and nothing the caller sent is wrong. + X_JOB_ROW_STATUS_UNKNOWN: 500, + // Thrown by `registerJobs()` while the app's modules load, so no request is ever answered with + // it either — the row exists for the reason `X_CORS_CONFIG_INVALID`'s does: this table is the + // closed one, and a code with no row is a 500 anyway. + X_ACTION_JOB_UNBRIDGED: 500, // @ultimat3/policy X_POLICY_MISSING: 500, X_PERMISSION_UNKNOWN: 500, @@ -165,6 +177,11 @@ export const ERROR_STATUS = { X_STORAGE_CHECKSUM_MISMATCH: 422, X_STORAGE_URL_INVALID: 403, X_STORAGE_URL_EXPIRED: 410, + // 500, not 403, and the distinction is the whole reason this code exists rather than + // reusing X_STORAGE_URL_INVALID: nothing is wrong with the caller's URL. The disk was + // built with no way to check a signature, which is the operator's misconfiguration and + // not an attacker — reporting it as 403 sends the on-call hunting somebody who is not there. + X_STORAGE_URL_UNVERIFIABLE: 500, // 409, not the 500 it fell through to: the object exists and the request is well formed — the // STATE is wrong. A validated upload lands under the quarantine segment and `promoteAttachment` // refuses it until the app's own scanner calls `releaseQuarantine`, which is a thing the caller diff --git a/packages/jobs/CLAUDE.md b/packages/jobs/CLAUDE.md index 7992dfa98..487635f08 100644 --- a/packages/jobs/CLAUDE.md +++ b/packages/jobs/CLAUDE.md @@ -641,7 +641,8 @@ picture from the other side. | `backfill-pending.ts` | declared minus completed, per environment: the alarm `--pending` reads | | `backfill-rate.ts` | the `rate` throttle: batches/sec as an interval, and the cancellable wait | | `backfill-inspect.ts` | the ledger projected for `x db backfill`, `x jobs`, `/_x` and MCP | -| `register.ts` | `registerJobs`/`registerTasks` over a module namespace + the registrar announcements | +| `backfill-errors.ts` | the seven `X_BACKFILL_*` classes — split out of `errors.ts`, which was over the 500-line ceiling. The codes themselves stay declared in `errors.ts`: one registry, one place | +| `register.ts` | `registerJobs`/`registerTasks` over a module namespace + the registrar announcements. Skips a non-job in silence — a module namespace is full of helpers — EXCEPT an `@ultimat3/action` projection (`kind: 'action-job'`), which is `X_ACTION_JOB_UNBRIDGED` | | `describe.ts` | the JSON projection one handle emits; `describeJobs()` is a map over it | | `steps.ts` | `StepStore`, `StepApi`, memoized-replay executor, `StepSuspension` | | `outbox.ts` | staging in a `Tx`, the relay, the ambient `JobsFacade` slot | diff --git a/packages/jobs/src/backfill-errors.ts b/packages/jobs/src/backfill-errors.ts new file mode 100644 index 000000000..d791a98df --- /dev/null +++ b/packages/jobs/src/backfill-errors.ts @@ -0,0 +1,137 @@ +// The seven `X_BACKFILL_*` codes, apart from `errors.ts` for the reason `driver-pg-rows.ts` is +// apart from `driver-pg.ts`: one file, one job, and `errors.ts` was over the 500-line ceiling +// `x verify`'s `filesize` step enforces. The registry stays there — `JOB_OWNED_ERROR_CODES`, +// `JOB_ERROR_TITLES` and the single `registerErrorCodes()` call — because a package's codes are +// declared in ONE place; only the classes that throw them live here, beside `backfill-ledger.ts`, +// `backfill-pending.ts` and `backfill-registry.ts`. + +import { UltimateError } from '@ultimat3/core'; +import { docsFor } from './errors'; + +/** + * The seven backfill codes below all answer one question — "why is this sweep not running?" — and + * each is here because it sends the reader somewhere different: run it, force it, change + * environment, migrate first, wait, fix the predicates, or fix the name. A code that shared a fix + * line with another one would be code inflation, which is why `X_BACKFILL_WRITE_UNCONFIRMED` was + * considered and rejected: a dry run that wrote nothing did exactly what it was asked to. + * + * Every `fix` below is ONE line something can execute — a shell command, or the edit to make. + * Never a command with prose appended: a `fix:` is copied and run verbatim, so a trailing clause + * turns a working command into a syntax error at the one moment the reader is following it + * literally. Explanations belong in `cause`, which is read and never run. + */ + +/** + * Declared and never completed. The alarm the framework did not have: an author could + * `x g backfill`, merge and deploy, and nothing anywhere said the pass had not run. + */ +export class BackfillPendingError extends UltimateError { + constructor(input: { backfill: string; environment: string }) { + super({ + code: 'X_BACKFILL_PENDING', + cause: `backfill "${input.backfill}" is declared and x_backfills holds no completed pass for it in ${input.environment}`, + fix: `x db backfill ${input.backfill} --write --json`, + docs: docsFor('X_BACKFILL_PENDING'), + }); + } +} + +/** Already swept. A rerun is legitimate, so this names the flag rather than refusing outright. */ +export class BackfillAppliedError extends UltimateError { + constructor(input: { backfill: string; runId: string; completedAt: string }) { + super({ + code: 'X_BACKFILL_APPLIED', + cause: `backfill "${input.backfill}" completed as run ${input.runId} at ${input.completedAt}; a forced rerun writes a NEW ledger row and never edits that one`, + fix: `x db backfill ${input.backfill} --write --force --json`, + docs: docsFor('X_BACKFILL_APPLIED'), + }); + } +} + +/** + * The declaration names the environments it belongs to and this is not one. Declared DATA, never a + * hardcoded "cleanups are production": a staging rehearsal is correct practice, so which + * environments a sweep belongs to is the app's convention and this is only the mechanism carrying + * it (axiom 8). + */ +export class BackfillEnvironmentError extends UltimateError { + constructor(input: { backfill: string; environment: string; declared: readonly string[] }) { + // The first declared environment, because the fix has to be ONE runnable line and the list is + // ordered by the author. The empty case cannot arise from `checkBackfillEnvironment`, which + // treats an empty list as "every environment" — but this constructor is public, so it answers + // with the command that lists what IS declared rather than an `ULTIMATE_ENV=undefined`. + const target = input.declared[0]; + super({ + code: 'X_BACKFILL_ENVIRONMENT', + cause: `backfill "${input.backfill}" declares environments: ${input.declared.join(', ')} and this process resolved ${input.environment} — add "${input.environment}" to that list if this deploy should sweep too`, + fix: + target === undefined + ? 'x db backfill --pending --json' + : `ULTIMATE_ENV=${target} x db backfill ${input.backfill} --write --json`, + docs: docsFor('X_BACKFILL_ENVIRONMENT'), + }); + } +} + +/** + * `requires` names a migration the ledger has not applied. Checked where `x_migrations` is + * readable — this package holds no `@ultimat3/db` dependency and growing one to read a ledger + * would put the migration engine on the tier-3 queue's import graph. + */ +export class BackfillMigrationPendingError extends UltimateError { + constructor(input: { backfill: string; migration: string }) { + super({ + code: 'X_BACKFILL_MIGRATION_PENDING', + cause: `backfill "${input.backfill}" requires migration ${input.migration}, which x_migrations does not record as applied`, + fix: 'x db migrate --json', + docs: docsFor('X_BACKFILL_MIGRATION_PENDING'), + }); + } +} + +/** + * The enqueue deduped: one live pass per name, so this run is the one already going. Distinct from + * `X_BACKFILL_APPLIED`, which is a pass that finished — the response there is `--force`, and the + * response here is to look at the run that is holding the key. + */ +export class BackfillRunningError extends UltimateError { + constructor(input: { backfill: string; jobId: string }) { + super({ + code: 'X_BACKFILL_RUNNING', + cause: `backfill "${input.backfill}" already has a live pass queued as ${input.jobId}, and one name holds one live pass; its step trace names the batch it is on, and a pass that is not advancing is a worker that lost its lease`, + fix: `x jobs show ${input.jobId} --json`, + docs: docsFor('X_BACKFILL_RUNNING'), + }); + } +} + +/** + * The source ran out of rows and the declaration's own `count()` still matches some. Two + * predicates that disagree is an authoring bug in any business — the sweep reported success over + * rows it never visited — so the pass fails rather than writing a completed row nobody can trust. + */ +export class BackfillStalledError extends UltimateError { + constructor(input: { backfill: string; remaining: number; swept: number }) { + super({ + code: 'X_BACKFILL_STALLED', + cause: `backfill "${input.backfill}" swept ${input.swept} rows, exhausted its source, and count() still matches ${input.remaining} — a WHERE the sweep narrows and the count does not is what leaves rows behind`, + fix: `make count() select on exactly what source() selects on in backfill("${input.backfill}")`, + docs: docsFor('X_BACKFILL_STALLED'), + }); + } +} + +/** A name no declaration in this app carries — a typo, or a backfill whose module was deleted. */ +export class BackfillUnknownError extends UltimateError { + constructor(input: { backfill: string; known: readonly string[] }) { + super({ + code: 'X_BACKFILL_UNKNOWN', + cause: + input.known.length === 0 + ? `no backfill named "${input.backfill}" is declared, and this app declares none at all` + : `no backfill named "${input.backfill}" is declared (declared: ${input.known.join(', ')})`, + fix: 'x db backfill --pending --json', + docs: docsFor('X_BACKFILL_UNKNOWN'), + }); + } +} diff --git a/packages/jobs/src/backfill-gate.test.ts b/packages/jobs/src/backfill-gate.test.ts index a224cb1a8..66706de04 100644 --- a/packages/jobs/src/backfill-gate.test.ts +++ b/packages/jobs/src/backfill-gate.test.ts @@ -4,10 +4,10 @@ import { describe, expect, test } from 'bun:test'; import type { Environment } from '@ultimat3/core'; +import { BackfillEnvironmentError } from './backfill-errors'; import { checkBackfillEnvironment, gateBackfill } from './backfill-gate'; import type { BackfillProgress } from './backfill-inspect'; import type { BackfillDeclaration } from './backfill-registry'; -import { BackfillEnvironmentError } from './errors'; const declaration = (over: Partial = {}): BackfillDeclaration => ({ kind: 'backfill', diff --git a/packages/jobs/src/backfill-gate.ts b/packages/jobs/src/backfill-gate.ts index a8685f6f0..25fd6bc9c 100644 --- a/packages/jobs/src/backfill-gate.ts +++ b/packages/jobs/src/backfill-gate.ts @@ -8,15 +8,15 @@ // convention rather than a rail (axiom 3). import type { Environment, UltimateError } from '@ultimat3/core'; -// `BackfillProgress`, the one ledger projection every surface already reads — never the driver's -// own row shape, which would make this a second reader of `x_backfills`. -import type { BackfillProgress } from './backfill-inspect'; -import type { BackfillDeclaration } from './backfill-registry'; import { BackfillAppliedError, BackfillEnvironmentError, BackfillMigrationPendingError, -} from './errors'; +} from './backfill-errors'; +// `BackfillProgress`, the one ledger projection every surface already reads — never the driver's +// own row shape, which would make this a second reader of `x_backfills`. +import type { BackfillProgress } from './backfill-inspect'; +import type { BackfillDeclaration } from './backfill-registry'; export type BackfillGate = | { readonly run: true } diff --git a/packages/jobs/src/backfill-pass.ts b/packages/jobs/src/backfill-pass.ts index 902b957e5..0f8771d06 100644 --- a/packages/jobs/src/backfill-pass.ts +++ b/packages/jobs/src/backfill-pass.ts @@ -16,13 +16,13 @@ import { appVersion, assert, logger, resolveEnvironment } from '@ultimat3/core'; import type { BatchIterator } from '@ultimat3/entity'; import type { BackfillDefinition, BackfillInput, BackfillReport } from './backfill'; +import { BackfillStalledError } from './backfill-errors'; import { checkBackfillEnvironment } from './backfill-gate'; import type { BackfillLedger, BackfillRun } from './backfill-ledger'; import { decideBackfill } from './backfill-ledger'; import type { Pacer } from './backfill-rate'; import { withBackfillScope } from './backfill-scope'; import { jobDriver } from './driver'; -import { BackfillStalledError } from './errors'; import type { JobRunArgs } from './job'; import { isStepSuspension } from './steps'; diff --git a/packages/jobs/src/driver-pg-rows.test.ts b/packages/jobs/src/driver-pg-rows.test.ts index 60c687aed..650f36b43 100644 --- a/packages/jobs/src/driver-pg-rows.test.ts +++ b/packages/jobs/src/driver-pg-rows.test.ts @@ -4,8 +4,43 @@ // `tenantId: null` to a tenant guard that only refuses `undefined`. import { describe, expect, test } from 'bun:test'; +import { isUltimateError } from '@ultimat3/core'; +import { BACKFILL_STATUSES } from './backfill-ledger'; +import { JOB_STATES } from './driver'; import type { BackfillRow, JobRow, StepRow } from './driver-pg-rows'; import { num, toBackfillRun, toJobRecord, toStepRecord } from './driver-pg-rows'; +import { isStepStatus, STEP_STATUSES } from './steps'; + +/** Row builders at file scope: the validation block below needs all three. */ +const stepRowFor = (overrides: Partial = {}): StepRow => ({ + run_id: 'run-1', + name: 'charge', + status: 'completed', + output: { chargeId: 'ch_1' }, + started_at: '1000', + completed_at: null, + wake_at: null, + event: null, + correlation_key: null, + attempts: 1, + error: null, + ...overrides, +}); + +const backfillRowFor = (overrides: Partial = {}): BackfillRow => ({ + run_id: 'run-1', + name: 'sweep', + checksum: 'aaaa', + status: 'running', + app_version: '1.2.0', + rows_processed: '4200', + last_cursor: 'id-42', + started_at: '1000', + completed_at: null, + ...overrides, +}); + +const jobRowFor = (overrides: Partial = {}): JobRow => jobRow(overrides); const jobRow = (overrides: Partial = {}): JobRow => ({ id: 'job-1', @@ -178,3 +213,104 @@ describe('toBackfillRun', () => { expect(Object.hasOwn(run, 'completedAt')).toBe(false); }); }); + +// The three decoders each cast a `text` column onto a closed union — `row.status as +// StepRecord['status']`, `row.state as JobRecord['state']`, `row.status as BackfillStatus`. A cast +// is not a check, and this data crosses a process boundary: the row was written by whatever build +// was deployed when the job was enqueued. `isBackfillStatus` one file over already states the rule +// this broke — "Narrows a string the CLI, MCP or a URL handed over. Never a cast — the list +// decides" — and `toBackfillRun` was the one caller ignoring it. +// +// What a laundered status COSTS is `steps.ts:263`: `if (existing?.status === 'completed')` returns +// the memoized output, and every other value falls through and RE-EXECUTES the step. So a status +// this build cannot read turns "this step already ran" into "run it again" — a second charge on +// `step.run('charge', …)`, in a file whose header promises the welcome email is not sent twice. +describe('a status column is validated, never cast', () => { + const codeOf = (run: () => unknown): string => { + try { + run(); + return 'resolved'; + } catch (error) { + return isUltimateError(error) ? error.code : String(error); + } + }; + + test('a step status this build does not know is refused, not passed through', () => { + // `'suspended'` is a JOB state, never a `StepStatus`. It reached `StepRecord.status` unchanged. + expect(codeOf(() => toStepRecord(stepRowFor({ status: 'suspended' })))).toBe( + 'X_JOB_ROW_STATUS_UNKNOWN', + ); + expect(codeOf(() => toStepRecord(stepRowFor({ status: '' })))).toBe('X_JOB_ROW_STATUS_UNKNOWN'); + }); + + test('every status the step vocabulary declares still decodes', () => { + for (const status of STEP_STATUSES) { + expect(toStepRecord(stepRowFor({ status })).status).toBe(status); + } + }); + + test('a job state this build does not know is refused', () => { + expect(codeOf(() => toJobRecord(jobRowFor({ state: 'paused' })))).toBe( + 'X_JOB_ROW_STATUS_UNKNOWN', + ); + for (const state of JOB_STATES) { + expect(toJobRecord(jobRowFor({ state })).state).toBe(state); + } + }); + + test('a backfill status this build does not know is refused', () => { + expect(codeOf(() => toBackfillRun(backfillRowFor({ status: 'cancelled' })))).toBe( + 'X_JOB_ROW_STATUS_UNKNOWN', + ); + for (const status of BACKFILL_STATUSES) { + expect(toBackfillRun(backfillRowFor({ status })).status).toBe(status); + } + }); + + // The refusal is an instruction: an operator reading it has to learn WHICH column held WHAT, and + // that the row is almost certainly a newer deploy's, not corruption. + test('the refusal names the column, the value and the table it came from', () => { + try { + toStepRecord(stepRowFor({ status: 'compensated' })); + throw new Error('expected a refusal'); + } catch (error) { + if (!isUltimateError(error)) throw error; + expect(error.cause).toContain('compensated'); + expect(error.cause).toContain('status'); + expect(error.cause).toContain('ultimate_job_steps'); + expect(error.fix).toContain('x jobs'); + } + }); +}); + +// What a rolling deploy does now, said out loud, because refusing changed it. +// +// A status string only reaches these columns because some build of this framework wrote it, so +// the two directions are not symmetric. N+1 reading N's rows is the normal direction and cannot +// refuse: a release that only ADDS a status still knows every older one, which the first test +// below pins by decoding the whole vocabulary. N reading N+1's row is the rare direction — a +// status added in N+1, seen by a worker still on N — and that one job now fails loudly with +// `X_JOB_ROW_STATUS_UNKNOWN` instead of silently re-running its steps. +describe('two builds sharing one queue', () => { + test('the older build refuses ONE row and leaves the rest of the queue readable', () => { + const fromNewerDeploy = stepRowFor({ name: 'compensate', status: 'compensated' }); + const written = STEP_STATUSES.map((status) => stepRowFor({ name: status, status })); + + expect(() => toStepRecord(fromNewerDeploy)).toThrow(/X_JOB_ROW_STATUS_UNKNOWN/); + // Every row this build did write still decodes: the refusal is per row, not per queue. + expect(written.map((row) => toStepRecord(row).status)).toEqual([...STEP_STATUSES]); + }); + + // The severity, demonstrated rather than asserted in prose: `stepRun` returns the memoized + // output only for `'completed'`, so a laundered status is not a cosmetic type hole — it is a + // step that runs a second time. This is the behaviour the cast used to produce. + test('a status that is not exactly `completed` is what makes a step re-run', () => { + const memoized = STEP_STATUSES.filter((status) => status === 'completed'); + const rerun = STEP_STATUSES.filter((status) => status !== 'completed'); + + expect(memoized).toEqual(['completed']); + // Anything outside the vocabulary lands in the second bucket, which is why it may not land. + expect(rerun).not.toContain('compensated'); + expect(isStepStatus('compensated')).toBe(false); + }); +}); diff --git a/packages/jobs/src/driver-pg-rows.ts b/packages/jobs/src/driver-pg-rows.ts index 1331c0d83..785278cd6 100644 --- a/packages/jobs/src/driver-pg-rows.ts +++ b/packages/jobs/src/driver-pg-rows.ts @@ -3,9 +3,10 @@ // decoding a row is not control flow — every number arrives as `number | string` (a bigint is a // string in every client) and every absent column as `null`, and that translation is its own job. -import type { BackfillRun, BackfillStatus } from './backfill-ledger'; -import type { JobRecord } from './driver'; -import type { StepRecord } from './steps'; +import { BACKFILL_STATUSES, type BackfillRun, isBackfillStatus } from './backfill-ledger'; +import { isJobState, JOB_STATES, type JobRecord, type JobState } from './driver'; +import { JobRowStatusUnknownError } from './errors'; +import { isStepStatus, STEP_STATUSES, type StepRecord, type StepStatus } from './steps'; export interface StepRow { readonly run_id: string; @@ -54,6 +55,21 @@ export interface JobRow { readonly enqueued_by?: string | null; } +/** + * The one narrowing for all three status columns. `as` is not a check, and these rows cross a + * process boundary — a queue row was written by whatever build was deployed when the job was + * enqueued, which on a rolling deploy is not this one. `isBackfillStatus`'s own doc already stated + * the rule ("Never a cast — the list decides") and this file was the caller ignoring it. + */ +const statusIn = ( + known: readonly T[], + is: (value: string) => value is T, + input: { table: string; column: string; value: string }, +): T => { + if (is(input.value)) return input.value; + throw new JobRowStatusUnknownError({ ...input, known }); +}; + export const num = (value: number | string | null | undefined): number => value === null || value === undefined ? 0 : Number(value); @@ -71,7 +87,11 @@ export function toJobRecord(row: JobRow): JobRecord { runId: row.run_id, attempt: row.attempt, maxAttempts: row.max_attempts, - state: row.state as JobRecord['state'], + state: statusIn(JOB_STATES, isJobState, { + table: 'ultimate_jobs', + column: 'state', + value: row.state, + }), runAt: num(row.run_at), createdAt: num(row.created_at), updatedAt: num(row.updated_at), @@ -94,7 +114,11 @@ export function toStepRecord(row: StepRow): StepRecord { return { runId: row.run_id, name: row.name, - status: row.status as StepRecord['status'], + status: statusIn(STEP_STATUSES, isStepStatus, { + table: 'ultimate_job_steps', + column: 'status', + value: row.status, + }), output: row.output, startedAt: num(row.started_at), attempts: row.attempts, @@ -112,7 +136,11 @@ export function toBackfillRun(row: BackfillRow): BackfillRun { runId: row.run_id, name: row.name, checksum: row.checksum, - status: row.status as BackfillStatus, + status: statusIn(BACKFILL_STATUSES, isBackfillStatus, { + table: 'ultimate_backfills', + column: 'status', + value: row.status, + }), appVersion: row.app_version, // `rows_processed` is a bigint, which every Postgres client hands back as a string. rows: num(row.rows_processed), diff --git a/packages/jobs/src/driver.ts b/packages/jobs/src/driver.ts index 21e748ffc..dd911d794 100644 --- a/packages/jobs/src/driver.ts +++ b/packages/jobs/src/driver.ts @@ -17,15 +17,22 @@ import type { StepStore } from './steps'; * a cancellation is work an operator stopped on purpose and `x jobs retry` must not resurrect by * accident. It appears in no claim predicate, so the queue never hands a cancelled row out again. */ -export type JobState = - | 'ready' - | 'delayed' - | 'running' - | 'suspended' - | 'done' - | 'failed' - | 'dead' - | 'cancelled'; +export const JOB_STATES = [ + 'ready', + 'delayed', + 'running', + 'suspended', + 'done', + 'failed', + 'dead', + 'cancelled', +] as const; + +export type JobState = (typeof JOB_STATES)[number]; + +/** Narrows a state read back off a queue row. Never a cast — the list decides. */ +export const isJobState = (value: string): value is JobState => + (JOB_STATES as readonly string[]).includes(value); export interface JobRecord { readonly id: string; diff --git a/packages/jobs/src/errors.ts b/packages/jobs/src/errors.ts index 255ea9868..91e2529a6 100644 --- a/packages/jobs/src/errors.ts +++ b/packages/jobs/src/errors.ts @@ -23,6 +23,8 @@ export const JOB_OWNED_ERROR_CODES = [ 'X_BACKFILL_RUNNING', 'X_BACKFILL_STALLED', 'X_BACKFILL_UNKNOWN', + 'X_JOB_ROW_STATUS_UNKNOWN', + 'X_ACTION_JOB_UNBRIDGED', ] as const; /** @@ -58,6 +60,8 @@ export const JOB_ERROR_TITLES: Readonly> = { X_BACKFILL_RUNNING: 'a pass under this name is already live', X_BACKFILL_STALLED: 'the sweep ended with rows its own count still matches', X_BACKFILL_UNKNOWN: 'no declaration carries this backfill name', + X_JOB_ROW_STATUS_UNKNOWN: 'a queue row carries a status this build does not know', + X_ACTION_JOB_UNBRIDGED: 'an action projection was registered as a job', }; // One unconditional call, so a second package claiming one of jobs' codes throws @@ -89,7 +93,8 @@ registerErrorRetry({ X_BACKFILL_APPLIED: 'terminal', }); -const docsFor = (code: JobErrorCode): string => `https://ultimate.dev/errors/${code}`; +/** Shared with `backfill-errors.ts`, which holds the seven `X_BACKFILL_*` classes. */ +export const docsFor = (code: JobErrorCode): string => `https://ultimate.dev/errors/${code}`; /** An enqueue collided with a live job holding the same idempotency key under `onConflict: 'error'`. */ export class JobDuplicateError extends UltimateError { @@ -124,6 +129,57 @@ export class JobNameTakenError extends UltimateError { } } +/** + * A `text` status column holds a value outside the vocabulary this build compiled. + * + * Refused rather than passed through, because the alternative is what used to happen: the three + * decoders in `driver-pg-rows.ts` cast the column, and `stepRun`'s `existing?.status === + * 'completed'` then read false for the laundered value and RE-EXECUTED the step. A second charge + * is a worse answer than a failed attempt, and "an unrecognised fact is never a satisfied one" is + * the rule the rest of the framework already follows. + * + * Almost always a NEWER deploy's row, not corruption: a status string only reaches the table + * because some version of this framework wrote it. A rolling deploy that only ADDS a status is + * safe in the normal direction — the new build knows every old value — and it is the old build + * reading the new build's row that lands here, on that one job, loudly. + */ +export class JobRowStatusUnknownError extends UltimateError { + constructor(input: { table: string; column: string; value: string; known: readonly string[] }) { + super({ + code: 'X_JOB_ROW_STATUS_UNKNOWN', + cause: + `${input.table}.${input.column} holds "${input.value}", which this build does not know — ` + + `it reads ${input.known.join(', ')}`, + fix: `x jobs show --json # then drain the older workers: a status this build cannot read was almost certainly written by a newer deploy`, + docs: docsFor('X_JOB_ROW_STATUS_UNKNOWN'), + }); + } +} + +/** + * `registerJobs()` was handed `someAction.job()`. + * + * That call answers an `ActionJobHandle` — `kind: 'action-job'`, deliberately a different literal + * from `'job'` — which is the four fields `job()` takes, not a job. It cannot be one: `action` and + * `jobs` are both tier 3, so neither may import the other, and only `job()` seats a handle the + * queue, the worker and the manifest accept. + * + * Refused BY NAME rather than skipped, which is what used to happen. `registerJobs(module)` is + * handed a whole module namespace, so silently ignoring a constant or a helper exported beside a + * job is right — but ignoring this one meant `registerJobs({ publishPost: publishPost.job() })` + * registered nothing, returned `[]`, and the job never ran, with nothing failing anywhere. + */ +export class ActionJobUnbridgedError extends UltimateError { + constructor(input: { export: string; job: string }) { + super({ + code: 'X_ACTION_JOB_UNBRIDGED', + cause: `export "${input.export}" is the action projection "${input.job}", which is not a job handle and cannot be registered as one`, + fix: `wrap it: agentJob(${input.export}, { name: '${input.export}', tenant, retry }) from @ultimat3/ai — that composes job() and returns a handle the queue accepts`, + docs: docsFor('X_ACTION_JOB_UNBRIDGED'), + }); + } +} + /** Two steps in one run share a name, so replay cannot tell their persisted results apart. */ export class StepDuplicateError extends UltimateError { constructor(input: { job: string; step: string }) { @@ -334,134 +390,6 @@ export class OutboxNoTxError extends UltimateError { } } -/** - * The seven backfill codes below all answer one question — "why is this sweep not running?" — and - * each is here because it sends the reader somewhere different: run it, force it, change - * environment, migrate first, wait, fix the predicates, or fix the name. A code that shared a fix - * line with another one would be code inflation, which is why `X_BACKFILL_WRITE_UNCONFIRMED` was - * considered and rejected: a dry run that wrote nothing did exactly what it was asked to. - * - * Every `fix` below is ONE line something can execute — a shell command, or the edit to make. - * Never a command with prose appended: a `fix:` is copied and run verbatim, so a trailing clause - * turns a working command into a syntax error at the one moment the reader is following it - * literally. Explanations belong in `cause`, which is read and never run. - */ - -/** - * Declared and never completed. The alarm the framework did not have: an author could - * `x g backfill`, merge and deploy, and nothing anywhere said the pass had not run. - */ -export class BackfillPendingError extends UltimateError { - constructor(input: { backfill: string; environment: string }) { - super({ - code: 'X_BACKFILL_PENDING', - cause: `backfill "${input.backfill}" is declared and x_backfills holds no completed pass for it in ${input.environment}`, - fix: `x db backfill ${input.backfill} --write --json`, - docs: docsFor('X_BACKFILL_PENDING'), - }); - } -} - -/** Already swept. A rerun is legitimate, so this names the flag rather than refusing outright. */ -export class BackfillAppliedError extends UltimateError { - constructor(input: { backfill: string; runId: string; completedAt: string }) { - super({ - code: 'X_BACKFILL_APPLIED', - cause: `backfill "${input.backfill}" completed as run ${input.runId} at ${input.completedAt}; a forced rerun writes a NEW ledger row and never edits that one`, - fix: `x db backfill ${input.backfill} --write --force --json`, - docs: docsFor('X_BACKFILL_APPLIED'), - }); - } -} - -/** - * The declaration names the environments it belongs to and this is not one. Declared DATA, never a - * hardcoded "cleanups are production": a staging rehearsal is correct practice, so which - * environments a sweep belongs to is the app's convention and this is only the mechanism carrying - * it (axiom 8). - */ -export class BackfillEnvironmentError extends UltimateError { - constructor(input: { backfill: string; environment: string; declared: readonly string[] }) { - // The first declared environment, because the fix has to be ONE runnable line and the list is - // ordered by the author. The empty case cannot arise from `checkBackfillEnvironment`, which - // treats an empty list as "every environment" — but this constructor is public, so it answers - // with the command that lists what IS declared rather than an `ULTIMATE_ENV=undefined`. - const target = input.declared[0]; - super({ - code: 'X_BACKFILL_ENVIRONMENT', - cause: `backfill "${input.backfill}" declares environments: ${input.declared.join(', ')} and this process resolved ${input.environment} — add "${input.environment}" to that list if this deploy should sweep too`, - fix: - target === undefined - ? 'x db backfill --pending --json' - : `ULTIMATE_ENV=${target} x db backfill ${input.backfill} --write --json`, - docs: docsFor('X_BACKFILL_ENVIRONMENT'), - }); - } -} - -/** - * `requires` names a migration the ledger has not applied. Checked where `x_migrations` is - * readable — this package holds no `@ultimat3/db` dependency and growing one to read a ledger - * would put the migration engine on the tier-3 queue's import graph. - */ -export class BackfillMigrationPendingError extends UltimateError { - constructor(input: { backfill: string; migration: string }) { - super({ - code: 'X_BACKFILL_MIGRATION_PENDING', - cause: `backfill "${input.backfill}" requires migration ${input.migration}, which x_migrations does not record as applied`, - fix: 'x db migrate --json', - docs: docsFor('X_BACKFILL_MIGRATION_PENDING'), - }); - } -} - -/** - * The enqueue deduped: one live pass per name, so this run is the one already going. Distinct from - * `X_BACKFILL_APPLIED`, which is a pass that finished — the response there is `--force`, and the - * response here is to look at the run that is holding the key. - */ -export class BackfillRunningError extends UltimateError { - constructor(input: { backfill: string; jobId: string }) { - super({ - code: 'X_BACKFILL_RUNNING', - cause: `backfill "${input.backfill}" already has a live pass queued as ${input.jobId}, and one name holds one live pass; its step trace names the batch it is on, and a pass that is not advancing is a worker that lost its lease`, - fix: `x jobs show ${input.jobId} --json`, - docs: docsFor('X_BACKFILL_RUNNING'), - }); - } -} - -/** - * The source ran out of rows and the declaration's own `count()` still matches some. Two - * predicates that disagree is an authoring bug in any business — the sweep reported success over - * rows it never visited — so the pass fails rather than writing a completed row nobody can trust. - */ -export class BackfillStalledError extends UltimateError { - constructor(input: { backfill: string; remaining: number; swept: number }) { - super({ - code: 'X_BACKFILL_STALLED', - cause: `backfill "${input.backfill}" swept ${input.swept} rows, exhausted its source, and count() still matches ${input.remaining} — a WHERE the sweep narrows and the count does not is what leaves rows behind`, - fix: `make count() select on exactly what source() selects on in backfill("${input.backfill}")`, - docs: docsFor('X_BACKFILL_STALLED'), - }); - } -} - -/** A name no declaration in this app carries — a typo, or a backfill whose module was deleted. */ -export class BackfillUnknownError extends UltimateError { - constructor(input: { backfill: string; known: readonly string[] }) { - super({ - code: 'X_BACKFILL_UNKNOWN', - cause: - input.known.length === 0 - ? `no backfill named "${input.backfill}" is declared, and this app declares none at all` - : `no backfill named "${input.backfill}" is declared (declared: ${input.known.join(', ')})`, - fix: 'x db backfill --pending --json', - docs: docsFor('X_BACKFILL_UNKNOWN'), - }); - } -} - export class JobsNotImplementedError extends UltimateError { constructor(input: { feature: string; fix: string }) { super({ diff --git a/packages/jobs/src/index.ts b/packages/jobs/src/index.ts index 2ec97ceca..fb14b4a2c 100644 --- a/packages/jobs/src/index.ts +++ b/packages/jobs/src/index.ts @@ -17,6 +17,15 @@ export type { BackfillReport, } from './backfill'; export { backfill, DEFAULT_BACKFILL_BATCH } from './backfill'; +export { + BackfillAppliedError, + BackfillEnvironmentError, + BackfillMigrationPendingError, + BackfillPendingError, + BackfillRunningError, + BackfillStalledError, + BackfillUnknownError, +} from './backfill-errors'; export type { BackfillGate, BackfillGateInput } from './backfill-gate'; export { checkBackfillEnvironment, gateBackfill } from './backfill-gate'; export type { BackfillProgress } from './backfill-inspect'; @@ -77,6 +86,8 @@ export type { export { DEFAULT_QUEUE, DEFAULT_VISIBILITY_TIMEOUT_MS, + isJobState, + JOB_STATES, jobDriver, resetJobDriver, setJobDriver, @@ -121,13 +132,7 @@ export type { RedisDriverOptions } from './driver-redis'; export { createRedisDriver } from './driver-redis'; export type { JobErrorCode } from './errors'; export { - BackfillAppliedError, - BackfillEnvironmentError, - BackfillMigrationPendingError, - BackfillPendingError, - BackfillRunningError, - BackfillStalledError, - BackfillUnknownError, + ActionJobUnbridgedError, CancelUnsupportedError, ConcurrencyUnenforceableError, DriverUnavailableError, @@ -139,6 +144,7 @@ export { JobMaxAttemptsError, JobNameTakenError, JobNotCancellableError, + JobRowStatusUnknownError, JobSlotLostError, JobsNotImplementedError, JobTenantRequiredError, @@ -248,8 +254,10 @@ export type { export { createMemoryStepStore, createStepRunner, + isStepStatus, isStepSuspension, MAX_TRACE_NAMES, + STEP_STATUSES, StepSuspension, } from './steps'; export type { diff --git a/packages/jobs/src/register.test.ts b/packages/jobs/src/register.test.ts index 4b4d46382..f9874e542 100644 --- a/packages/jobs/src/register.test.ts +++ b/packages/jobs/src/register.test.ts @@ -284,3 +284,52 @@ describe('the registrar announcement', () => { expect(getJob('onboardOrg')).toBe(onboardOrg as AnyJobHandle); }); }); + +// `registerJobs(module)` is handed a whole module namespace, so skipping a non-job silently is +// correct for a constant, a type or a helper exported beside the job — that is the case above. +// An `@ultimat3/action` job PROJECTION is the one shape where silence is wrong: `.job()` is named +// `job`, it carries `name`, `input` and `idempotencyKey`, and it is exactly what an author reaches +// for when they want to queue an action. It is not a job and cannot be — `action` and `jobs` are +// both tier 3, so neither may import the other — so `registerJobs({ publishPost: p.job() })` +// registered NOTHING, returned `[]`, and the job simply never ran. Nothing failed, at any point. +describe('an action projection is refused by name, never skipped in silence', () => { + /** + * The shape `packages/action/src/job-handle.ts` builds, detected structurally because the tier + * rule forbids importing it. `kind: 'action-job'` is deliberately a different literal from + * `'job'` — that file says so — which is exactly what makes it recognisable here. + */ + const actionProjection = { + kind: 'action-job' as const, + name: 'action:publishPost', + input: passthrough(), + idempotencyKey: (input: OrgInput) => `action:publishPost:${input.orgId}`, + invoke: async () => undefined, + }; + + test('registering one throws instead of registering nothing', () => { + expect(codeOf(() => registerJobs({ publishPost: actionProjection }))).toBe( + 'X_ACTION_JOB_UNBRIDGED', + ); + expect(registeredJobs()).toHaveLength(0); + }); + + test('the refusal names the export and the bridge that fixes it', () => { + try { + registerJobs({ publishPost: actionProjection }); + throw new Error('expected a refusal'); + } catch (error) { + if (!(error instanceof UltimateError)) throw error; + expect(error.cause).toContain('publishPost'); + expect(error.fix).toContain('agentJob'); + } + }); + + // The reason silence is still right for everything else: a module namespace is full of exports + // that are not jobs, and refusing those would make every helper a boot failure. + test('an ordinary export beside a job is still skipped without a word', () => { + expect(registerJobs({ notifySubscribers: anonymousJob(), MAX: 5, helper: () => 1 })).toEqual([ + expect.objectContaining({ kind: 'job' }), + ]); + expect(registeredJobs()).toHaveLength(1); + }); +}); diff --git a/packages/jobs/src/register.ts b/packages/jobs/src/register.ts index 80e43cc20..1ba5ac213 100644 --- a/packages/jobs/src/register.ts +++ b/packages/jobs/src/register.ts @@ -5,9 +5,24 @@ */ import { type RegisteredPrimitive, registerPrimitiveRegistrar } from '@ultimat3/core'; +import { ActionJobUnbridgedError } from './errors'; import { isJobHandle, registerJob } from './job'; import { isTaskHandle, registerTask } from './task'; +/** + * `@ultimat3/action`'s job PROJECTION, recognised structurally because that package is this tier + * and may never be imported here. `kind: 'action-job'` is a literal chosen to be distinguishable + * from `'job'` rather than a near-miss (`packages/action/src/job-handle.ts` says so), which is + * precisely what makes this check possible without an import. + */ +const isActionProjection = ( + value: unknown, +): value is { readonly kind: 'action-job'; readonly name: string } => + typeof value === 'object' && + value !== null && + 'kind' in value && + (value as { readonly kind: unknown }).kind === 'action-job'; + /** `registerJobs(await import('./jobs'))` — export names become job names. */ export function registerJobs( module: Readonly>, @@ -15,7 +30,16 @@ export function registerJobs( const registered: RegisteredPrimitive[] = []; for (const name of Object.keys(module).sort()) { const value = module[name]; - if (isJobHandle(value)) registered.push(registerJob(name, value)); + if (isJobHandle(value)) { + registered.push(registerJob(name, value)); + continue; + } + // Everything else is skipped in silence — a module namespace is full of constants, types and + // helpers exported beside the jobs. Everything else EXCEPT this one, which is unambiguously + // someone trying to queue an action and getting nothing at all. + if (isActionProjection(value)) { + throw new ActionJobUnbridgedError({ export: name, job: value.name }); + } } return registered; } diff --git a/packages/jobs/src/steps.ts b/packages/jobs/src/steps.ts index c4bfc9e7e..5e826c888 100644 --- a/packages/jobs/src/steps.ts +++ b/packages/jobs/src/steps.ts @@ -12,7 +12,20 @@ import type { DurationInput } from './clock'; import { nowMs, toMs } from './clock'; import { JobAbortedError, JobTimeoutError, StepDuplicateError } from './errors'; -export type StepStatus = 'completed' | 'sleeping' | 'waiting' | 'failed'; +/** + * The runtime list is the declaration and `StepStatus` is derived from it, the shape + * `BACKFILL_STATUSES` and `PRIMITIVE_KINDS` already have. A bare union cannot narrow a `text` + * column, so `driver-pg-rows.ts` cast one instead — and a cast that lands an unknown status on a + * record makes `stepRun`'s `existing?.status === 'completed'` false, which RE-EXECUTES a step + * this file promises runs once. + */ +export const STEP_STATUSES = ['completed', 'sleeping', 'waiting', 'failed'] as const; + +export type StepStatus = (typeof STEP_STATUSES)[number]; + +/** Narrows a status read back out of a store. Never a cast — the list decides. */ +export const isStepStatus = (value: string): value is StepStatus => + (STEP_STATUSES as readonly string[]).includes(value); export interface StepRecord { readonly runId: string; diff --git a/packages/render/src/index.ts b/packages/render/src/index.ts index 45b39f6b9..5a874a2f5 100644 --- a/packages/render/src/index.ts +++ b/packages/render/src/index.ts @@ -135,6 +135,7 @@ export { createIsrController, DEFAULT_ISR_MAX_ENTRIES, invalidateAndRevalidate, + isrKey, memoryIsrStore, } from './render-isr'; export type { SpaShell, SpaShellInput } from './render-spa'; diff --git a/packages/render/src/render-isr.test.ts b/packages/render/src/render-isr.test.ts index 09173cf0f..97e2e9af5 100644 --- a/packages/render/src/render-isr.test.ts +++ b/packages/render/src/render-isr.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeEach, describe, expect, test } from 'bun:test'; import type { CacheTag } from '@ultimat3/cache'; import { invalidateTags, isolateGraph, resetGraph, tag } from '@ultimat3/cache'; import { clearRoutes, describeRoutes, registerRoute } from './registry'; -import { createIsrController, memoryIsrStore } from './render-isr'; +import { createIsrController, isrKey, memoryIsrStore } from './render-isr'; import type { RenderResult, RouteMetaFn } from './route'; import { defineRoute } from './route'; @@ -38,7 +38,13 @@ beforeEach(() => { resetGraph(); }); -afterAll(restoreGraph); +afterAll(() => { + // The graph AND the registry. `beforeEach` clears routes for this file's own sake; this clears + // them for everyone else's — the registry is process-global, so the last test in this file left + // its routes visible to every suite that ran after it, in whatever order `bun test` chose. + clearRoutes(); + restoreGraph(); +}); describe('single-flight regeneration', () => { test('a burst of concurrent requests renders exactly once', async () => { @@ -310,3 +316,70 @@ describe('the default ISR store is bounded', () => { expect(controller.revalidateByTags([postTag])).toEqual(['/blog/a', '/blog/c']); }); }); + +// #171's second half. A gated ISR route is refused at `defineRoute` (`modes.ts`), which closes the +// actor axis at build time — but two URLs that differ only in their query are two documents no +// build error can tell apart, and the store keyed both as the bare pathname. +describe('the store key carries the query, and the route lookup does not', () => { + test('two queries on one path are two entries, not one served twice', async () => { + isrRoute('apps/web/site/blog/page.tsx', [postTag], '5m'); + const isr = createIsrController({ store: memoryIsrStore() }); + + const first = await isr.serve('/blog?page=2', () => '

page 2

'); + const second = await isr.serve('/blog?page=3', () => '

page 3

'); + + expect(first.result.body).toBe('

page 2

'); + // The whole issue in one assertion: keyed on `/blog`, this was `

page 2

` and `state` + // was `hit` — a second visitor served the first visitor's document. + expect(second.result.body).toBe('

page 3

'); + expect(second.state).toBe('miss'); + expect(isr.store().paths()).toEqual(['/blog?page=2', '/blog?page=3']); + }); + + test('a query-keyed entry still resolves ITS ROUTE for the declared TTL', async () => { + // The trap in the obvious fix. `descriptorFor` asks the route TABLE, and `/blog?page=2` + // matches no route pattern — so a key change without stripping the query first would answer + // `ttlMs: null`, and `revalidate: { ttl: '5m' }` would silently become tag-only. `s-maxage` + // is where that surfaces: 300 is the declared five minutes, 60 is the tag-only fallback. + isrRoute('apps/web/site/blog/page.tsx', [postTag], '5m'); + const isr = createIsrController({ store: memoryIsrStore() }); + + const served = await isr.serve('/blog?page=2', () => '

x

'); + + expect(sMaxAge(served.result)).toBe('300'); + expect(served.entry.ttlMs).toBe(300_000); + }); + + test('a dynamic route with a query resolves through its pattern too', async () => { + isrRoute('apps/web/site/blog/[slug]/page.tsx', [postTag], '5m'); + const isr = createIsrController({ store: memoryIsrStore() }); + + const served = await isr.serve('/blog/hello?utm_source=x', () => '

x

'); + + expect(sMaxAge(served.result)).toBe('300'); + }); + + test('a tag bust marks the query-keyed entries, so nothing escapes invalidation', async () => { + isrRoute('apps/web/site/blog/page.tsx', [postTag], '5m'); + const isr = createIsrController({ store: memoryIsrStore() }); + await isr.serve('/blog?page=2', () => '

a

'); + await isr.serve('/blog?page=3', () => '

b

'); + + expect(isr.revalidateByTags([postTag])).toEqual(['/blog?page=2', '/blog?page=3']); + }); +}); + +describe('isrKey', () => { + test('a bare path is its own key — no trailing `?` for a URL that had none', () => { + expect(isrKey(new URL('https://app.test/blog'))).toBe('/blog'); + }); + + test('params are SORTED, so one page is not rendered twice under two spellings', () => { + expect(isrKey(new URL('https://app.test/blog?b=2&a=1'))).toBe('/blog?a=1&b=2'); + expect(isrKey(new URL('https://app.test/blog?a=1&b=2'))).toBe('/blog?a=1&b=2'); + }); + + test('the fragment is never in the key — a browser never sends it', () => { + expect(isrKey(new URL('https://app.test/blog?a=1#section'))).toBe('/blog?a=1'); + }); +}); diff --git a/packages/render/src/render-isr.ts b/packages/render/src/render-isr.ts index 284cfa86c..836ac7e50 100644 --- a/packages/render/src/render-isr.ts +++ b/packages/render/src/render-isr.ts @@ -23,6 +23,11 @@ import type { RenderResult } from './route'; export type IsrState = 'miss' | 'hit' | 'stale'; export interface IsrEntry { + /** + * The store key: the request's pathname AND its query, params sorted. Not the route's pattern + * and not the bare pathname — `/blog?page=2` and `/blog?page=3` render different documents, and + * keying both as `/blog` served the second visitor the first one's HTML (#171). + */ readonly path: string; readonly html: string; readonly hash: string; @@ -75,6 +80,37 @@ export function memoryIsrStore(options: MemoryIsrStoreOptions = {}): IsrStore { }; } +/** + * The ISR store key for one request URL: pathname plus the query, **params sorted**. + * + * Exported because deriving it is the caller's job and there may only be ONE derivation — a + * server that keyed on `url.pathname` while the store believed it held a whole URL is the shape + * of #171. Sorting makes `?a=1&b=2` and `?b=2&a=1` one entry rather than two renders of one page. + * + * A query-carrying URL therefore gets its own entry, which is correct and is not free: a crawler + * appending `?utm_source=…` mints one entry per value. That is bounded, not unbounded — + * `DEFAULT_ISR_MAX_ENTRIES` evicts least-recently-generated first — and a bounded cache that + * thrashes is the right failure next to an unbounded one that answers the wrong document. + */ +export function isrKey(url: URL): string { + if (url.search === '') return url.pathname; + const params = new URLSearchParams(url.search); + params.sort(); + return `${url.pathname}?${params.toString()}`; +} + +/** + * The route pattern a key belongs to. Every lookup that asks the ROUTE TABLE a question — the + * descriptor, and therefore the TTL — has to strip the query first: `descriptorFor('/blog?page=2')` + * matches no route, so the entry would silently fall back to `ttlMs: null` and a declared + * `revalidate: { ttl: '5m' }` would become tag-only. Keying without this split is a half-fix that + * trades a leak for a wrong TTL. + */ +function routePathOf(key: string): string { + const query = key.indexOf('?'); + return query === -1 ? key : key.slice(0, query); +} + export type IsrRenderFn = (path: string) => string | Promise; export interface IsrServeResult { @@ -131,7 +167,8 @@ export function createIsrController(options: IsrControllerOptions = {}): IsrCont const pending = new Map>(); const registered = new Set(); - function descriptorFor(path: string): RouteDescriptor | undefined { + function descriptorFor(key: string): RouteDescriptor | undefined { + const path = routePathOf(key); const table = routes(); return table.find((r) => r.path === path) ?? table.find((r) => matchesRoute(path, r.path)); } diff --git a/packages/scraping/src/cdp-fake.test.ts b/packages/scraping/src/cdp-fake.test.ts index a7468ac72..546d46f7a 100644 --- a/packages/scraping/src/cdp-fake.test.ts +++ b/packages/scraping/src/cdp-fake.test.ts @@ -131,7 +131,7 @@ describe('unit · the fake page`s DOM answers the real target`s reads', () => { }); const target = await openOver(browser); - await target.click('#pay', 0); + await target.click('#pay'); expect(target.url()).toBe('https://shop.test/receipt'); expect(await target.content()).toContain('Paid'); @@ -141,7 +141,7 @@ describe('unit · the fake page`s DOM answers the real target`s reads', () => { test('a click with no route leaves the page where it was', async () => { const browser = fakeCdpBrowser(PAGE); const target = await openOver(browser); - await target.click('#pay', 0); + await target.click('#pay'); expect(target.url()).toBe('https://shop.test/orders'); }); @@ -185,8 +185,8 @@ describe('unit · the fake page`s DOM answers the real target`s reads', () => { test('screenshot and pdf answer bytes, so the artifact path has something to write', async () => { const browser = fakeCdpBrowser(PAGE); const target = await openOver(browser); - expect([...(await target.screenshot({ fullPage: true, timeoutMs: 1_000 }))]).toEqual([1, 2, 3]); - expect([...(await target.pdf({ timeoutMs: 1_000 }))]).toEqual([4, 5]); + expect([...(await target.screenshot({ fullPage: true }))]).toEqual([1, 2, 3]); + expect([...(await target.pdf({}))]).toEqual([4, 5]); }); }); diff --git a/packages/scraping/src/cdp-target-surface.test.ts b/packages/scraping/src/cdp-target-surface.test.ts index cd7ffa71e..c5679c55c 100644 --- a/packages/scraping/src/cdp-target-surface.test.ts +++ b/packages/scraping/src/cdp-target-surface.test.ts @@ -171,7 +171,7 @@ describe('unit · the target`s input calls reach the page, not a reimplementatio test('click, type and select pass their selector and values straight through', async () => { const fixture = rich(); const target = await targetOver(fixture); - await target.click('#buy', 0); + await target.click('#buy'); await target.type('#email', 'a@b.test'); await target.select('#size', ['m', 'l']); expect(fixture.calls).toContain('click #buy'); @@ -207,7 +207,7 @@ describe('unit · capture', () => { // Some builds answer text and some answer bytes; an artifact written from the string would be // a PNG nobody can open. const fixture = rich({ screenshotBase64: true }); - const shot = await (await targetOver(fixture)).screenshot({ fullPage: true, timeoutMs: 1_000 }); + const shot = await (await targetOver(fixture)).screenshot({ fullPage: true }); expect(shot).toBeInstanceOf(Uint8Array); expect(new TextDecoder().decode(shot)).toBe('PNG'); expect(fixture.calls).toContain('screenshot fullPage=true'); @@ -215,14 +215,14 @@ describe('unit · capture', () => { test('bytes are passed through unchanged, and fullPage defaults to false', async () => { const fixture = rich(); - const shot = await (await targetOver(fixture)).screenshot({ timeoutMs: 1_000 }); + const shot = await (await targetOver(fixture)).screenshot({}); expect([...shot]).toEqual([1, 2, 3]); expect(fixture.calls).toContain('screenshot fullPage=false'); }); test('pdf comes straight off the page', async () => { const fixture = rich(); - expect([...(await (await targetOver(fixture)).pdf({ timeoutMs: 1_000 }))]).toEqual([4, 5]); + expect([...(await (await targetOver(fixture)).pdf({}))]).toEqual([4, 5]); }); test('download() is an honest X_NOT_IMPLEMENTED, never empty bytes', async () => { @@ -302,7 +302,7 @@ describe('unit · frames', () => { expect(inner.url()).toBe('https://shop.test/checkout-frame'); expect(await inner.content()).toBe('

frame

'); expect((await inner.query('a')).map((element) => element.text)).toEqual(['Order 1']); - await inner.click('#pay', 0); + await inner.click('#pay'); await inner.type('#card', '4242'); await inner.select('#country', ['de']); await inner.evaluate('1 + 1'); @@ -329,7 +329,7 @@ describe('unit · a dead renderer is a CODE, not a hang', () => { fixture.emit('error', { message: 'Renderer process crashed' }); await expect(target.content()).rejects.toThrow(/X_SCRAPE_PAGE_CRASHED|crashed/); - await expect(target.click('#buy', 0)).rejects.toThrow(/X_SCRAPE_PAGE_CRASHED|crashed/); + await expect(target.click('#buy')).rejects.toThrow(/X_SCRAPE_PAGE_CRASHED|crashed/); // And the call never reached the page: a crashed tab would wait out its own timeout. expect(fixture.calls).not.toContain('click #buy'); }); diff --git a/packages/scraping/src/driver-parity.test.ts b/packages/scraping/src/driver-parity.test.ts index 3381003c5..b5b385ab9 100644 --- a/packages/scraping/src/driver-parity.test.ts +++ b/packages/scraping/src/driver-parity.test.ts @@ -25,6 +25,8 @@ const HTML_A = `

Orders

  • One
  • Two
+ + `; @@ -74,7 +76,12 @@ const drivers = (): readonly (readonly [string, ScrapeDriver])[] => [ launcher: fakeCdpLauncher({ url: URL_A, html: HTML_A, - routes: { '#next': { url: URL_B, html: HTML_B } }, + routes: { + '#next': { url: URL_B, html: HTML_B }, + // Keyed by SELECTOR, not by element: this fake has no DOM index for clicks, which is + // exactly why the `index` divergence below could hide from every existing test. + '.go': { url: URL_B, html: HTML_B }, + }, covered: ['#covered'], }), }), @@ -111,6 +118,30 @@ describe('unit · every driver answers the vocabulary the same way', () => { }); }); + /** + * `ScrapeTarget.click` took a second `index` argument that `html-target.ts` honoured + * (`at(selector, index)`) and `cdp-target.ts` silently dropped — its implementations are + * `click: (selector) => …`, so puppeteer clicked the first match whatever was asked for. They + * agreed only because the sole production caller, `page-over-target.ts`, always passed `0`. + * + * The parameter is deleted rather than implemented in the second driver: no public vocabulary + * can set it (`ScrapeFrame.click(selector, options?: WaitOptions)` has no index), so it was a + * port member no app could reach and one driver ignored. This is the test that would have + * caught the divergence — a multi-match selector, driven through the page, on all three. + */ + test('a multi-match selector clicks the FIRST match on all three', async () => { + await forEachDriver(async (session, name) => { + await session.page.goto(URL_A); + await session.page.click('.go'); + // The first `.go` goes to /orders/2; the second points at a URL no recording covers, so an + // offline driver that clicked it raises `X_SCRAPE_FIXTURE_MISSING` instead of landing here. + // The puppeteer path routes by selector and cannot tell the two apart — that blindness is + // the reason the divergence survived, and it is why the port no longer carries an `index` + // for one driver to honour and the other to drop. + expect(await session.page.text('#title'), name).toBe('Page two'); + }); + }); + test('a disabled control is refused with X_SCRAPE_NOT_ACTIONABLE on all three', async () => { await forEachDriver(async (session, name) => { await session.page.goto(URL_A); diff --git a/packages/scraping/src/error-throws.ts b/packages/scraping/src/error-throws.ts index 40f601900..d9cea7c62 100644 --- a/packages/scraping/src/error-throws.ts +++ b/packages/scraping/src/error-throws.ts @@ -75,6 +75,21 @@ export const notActionable = (selector: string, reason: string, waitedMs: number meta: { selector, reason, waitedMs }, }); +/** + * Declared, and structurally unable to fire. `maxDrop` is a fraction of a TRAILING MEDIAN, and the + * only source of one is a `history:` store — with none, `guardYield` reads an empty array and the + * `MIN_BASELINE_RUNS` gate is true forever, so the alarm never raises `X_SCRAPE_YIELD_COLLAPSED` + * on any run. Two halves that must be set together, where setting one alone did nothing and + * nothing said so. + */ +export const yieldHistoryMissing = (scrapeName: string): ScrapeError => + new ScrapeError({ + code: 'X_SCRAPE_YIELD_HISTORY_MISSING', + cause: `scrape "${scrapeName}" declares expect.maxDrop with no history: store, so there is no baseline to measure a drop against and the alarm can never fire`, + fix: `add history: memoryYieldHistory() to scrape("${scrapeName}") for a test, or your own YieldHistory in production — or drop expect.maxDrop and keep expect.minRows, which needs no baseline`, + meta: { scrape: scrapeName }, + }); + export const scrapeTimeout = (what: string, ms: number): ScrapeError => new ScrapeError({ code: 'X_SCRAPE_TIMEOUT', diff --git a/packages/scraping/src/errors.ts b/packages/scraping/src/errors.ts index 25df52009..9b8a1c56c 100644 --- a/packages/scraping/src/errors.ts +++ b/packages/scraping/src/errors.ts @@ -25,6 +25,7 @@ export const SCRAPE_OWNED_ERROR_CODES = [ 'X_SCRAPE_PAGE_CRASHED', 'X_SCRAPE_OUTPUT_INVALID', 'X_SCRAPE_YIELD_COLLAPSED', + 'X_SCRAPE_YIELD_HISTORY_MISSING', 'X_SCRAPE_DOWNLOAD_TIMEOUT', 'X_SCRAPE_ROBOTS_DISALLOWED', 'X_SCRAPE_FIXTURE_MISSING', @@ -70,6 +71,8 @@ export const SCRAPE_ERROR_TITLES: Readonly> X_SCRAPE_PAGE_CRASHED: 'the renderer process died', X_SCRAPE_OUTPUT_INVALID: 'the extracted rows do not match the extract schema', X_SCRAPE_YIELD_COLLAPSED: 'the run succeeded and returned far too little', + X_SCRAPE_YIELD_HISTORY_MISSING: + 'a maxDrop is declared and no history store can supply its baseline', X_SCRAPE_DOWNLOAD_TIMEOUT: 'the download never landed', X_SCRAPE_ROBOTS_DISALLOWED: 'robots.txt disallows this path', X_SCRAPE_FIXTURE_MISSING: 'the fixture driver has no recording for this request', @@ -132,6 +135,9 @@ export const SCRAPE_ERROR_RETRY = { // gigabyte and dies the same way. The fix is a number on the request, so a human decides it. X_SCRAPE_BODY_TOO_LARGE: 'terminal', X_SCRAPE_YIELD_COLLAPSED: 'terminal', + // A declaration error, raised by `scrape()` before any attempt exists — there is no run to + // retry, and the same definition would refuse identically forever. + X_SCRAPE_YIELD_HISTORY_MISSING: 'terminal', X_SCRAPE_ROBOTS_DISALLOWED: 'terminal', X_SCRAPE_FIXTURE_MISSING: 'terminal', X_SCRAPE_FIXTURE_STALE: 'terminal', diff --git a/packages/scraping/src/html-target.test.ts b/packages/scraping/src/html-target.test.ts index 59e41a25c..e760e72fc 100644 --- a/packages/scraping/src/html-target.test.ts +++ b/packages/scraping/src/html-target.test.ts @@ -79,7 +79,7 @@ describe('unit · a download is armed only by a RECORDED selector', () => { html: 'Export', downloads: { '#export': 'orders.csv:id,1' }, }); - await target.click('#constructor', 0); + await target.click('#constructor'); expect(await codeOf(() => target.download({ timeoutMs: 10 }))).toBe( 'X_SCRAPE_DOWNLOAD_TIMEOUT', ); @@ -103,7 +103,7 @@ describe('unit · a download is armed only by a RECORDED selector', () => { html: 'Export', downloads: { '#export': 'orders.csv:id,1' }, }); - await target.click('#export', 0); + await target.click('#export'); const file = await target.download({ timeoutMs: 10 }); expect(file.filename).toBe('orders.csv'); }); diff --git a/packages/scraping/src/html-target.ts b/packages/scraping/src/html-target.ts index a5b9b8849..2bdec53f4 100644 --- a/packages/scraping/src/html-target.ts +++ b/packages/scraping/src/html-target.ts @@ -171,8 +171,8 @@ export function htmlTarget(init: HtmlTargetInit): ScrapeTarget { return Promise.resolve(page.html); }, query, - async click(selector: string, index: number): Promise { - const element = await at(selector, index); + async click(selector: string): Promise { + const element = await at(selector, 0); if (element === undefined) throw fixtureMissing(`${page.url} ${selector}`, init.source); const download = recorded(page.downloads, selector) ?? recorded(page.downloads, element.attrs['id'] ?? ''); diff --git a/packages/scraping/src/page-over-target.test.ts b/packages/scraping/src/page-over-target.test.ts index 0caf8792b..7804c7b4b 100644 --- a/packages/scraping/src/page-over-target.test.ts +++ b/packages/scraping/src/page-over-target.test.ts @@ -148,3 +148,53 @@ describe('unit · a promise-typed page method rejects whatever the driver under expect((caught as { code?: string } | undefined)?.code).toBe('X_SCRAPE_DOWNLOAD_TIMEOUT'); }); }); + +describe('unit · what a capture actually asks the driver for', () => { + /** + * `CaptureOptions.timeoutMs` used to be REQUIRED by the port and honoured by no driver: + * `page.screenshot({ timeout })` was documented, threaded down here, and dropped by + * `cdp-target.ts` and `html-target.ts` alike. It is deleted rather than implemented. + * + * Implementing it was the tempting answer and it is the wrong one twice over. The CDP port's + * `screenshot({ fullPage })` has no timeout slot, and a generic race in this file would have to + * race `clock.sleep`, which under `testClock` resolves on the FIRST microtask — so every capture + * in every test would have timed out instead of running. A knob no driver ever honoured takes + * nothing away when it goes; a deadline that fires in tests and not in production would. + * + * What the driver is handed is now exactly what a driver can act on: `fullPage`, and nothing it + * would have to ignore. + */ + test('the driver is handed fullPage and nothing else', async () => { + const recording: PageRecording = { url: 'https://shop.test/o', html: '

o

' }; + const base = htmlTarget({ + driver: 'fixture', + lookup: () => Promise.resolve(recording), + rules: { allowHosts: ['shop.test'] }, + clock: testClock(), + source: 'test/fixtures', + start: recording, + }); + const asked: unknown[] = []; + const target: ScrapeTarget = { + ...base, + screenshot: (options) => { + asked.push({ ...options }); + return Promise.resolve(new Uint8Array([1])); + }, + pdf: (options) => { + asked.push({ ...options }); + return Promise.resolve(new Uint8Array([2])); + }, + }; + const page = pageOverTarget(target, { + clock: testClock(), + allowHosts: ['shop.test'], + defaultTimeoutMs: 100, + }); + + await page.screenshot({ fullPage: true }); + await page.pdf(); + + expect(asked).toEqual([{ fullPage: true }, { fullPage: undefined }]); + }); +}); diff --git a/packages/scraping/src/page-over-target.ts b/packages/scraping/src/page-over-target.ts index 91a91fe74..6345d3430 100644 --- a/packages/scraping/src/page-over-target.ts +++ b/packages/scraping/src/page-over-target.ts @@ -105,7 +105,7 @@ function frameOver( waitFor: (selector, options) => wait(selector, options, 'actionable'), async click(selector, options): Promise { await wait(selector, options, 'actionable'); - await (await resolve()).click(selector, 0); + await (await resolve()).click(selector); }, async type(selector, text, options): Promise { await wait(selector, options, 'actionable'); @@ -187,8 +187,7 @@ export function pageOverTarget(target: ScrapeTarget, ctx: PageContext): ScrapePa // in object storage, forever. Refused rather than masked — a mask over pixels is a guess // about layout, and `page.html()` already gives a redacted artifact that is exact. if (state.tainted) throw secretExposed(kind, target.url()); - const timeoutMs = options?.timeout ?? ctx.defaultTimeoutMs; - const request = { fullPage: options?.fullPage, timeoutMs }; + const request = { fullPage: options?.fullPage }; return kind === 'screenshot' ? target.screenshot(request) : target.pdf(request); }; return { diff --git a/packages/scraping/src/page.ts b/packages/scraping/src/page.ts index c496ea511..4abcffe37 100644 --- a/packages/scraping/src/page.ts +++ b/packages/scraping/src/page.ts @@ -64,9 +64,9 @@ export interface ScrapeFrame { frame(nameOrSelector: string): ScrapeFrame; } +/** `fullPage` only. `timeout` is gone with the port's — see `CaptureOptions` for why. */ export interface CaptureRequest { readonly fullPage?: boolean | undefined; - readonly timeout?: number | undefined; } export interface DownloadRequest { diff --git a/packages/scraping/src/scrape.test.ts b/packages/scraping/src/scrape.test.ts index 29c1af43a..92fa9cdb0 100644 --- a/packages/scraping/src/scrape.test.ts +++ b/packages/scraping/src/scrape.test.ts @@ -120,6 +120,35 @@ describe('unit · one attempt, end to end, with no browser', () => { expect(await codeOf(handle.run(runArgs({ page: 1 })))).toBe('X_SCRAPE_YIELD_COLLAPSED'); }); + /** + * `maxDrop` is measured against a trailing median that only a `history:` store can supply. With + * no store, `guardYield` reads an empty array, `history.length < MIN_BASELINE_RUNS` is true on + * every run forever, and the alarm can never fire — a declared yield guard that cannot fail, + * which is the silent green this whole file exists to prevent. Refused where it is written. + * + * `minRows` is deliberately still legal alone: it is an absolute floor and needs no baseline. + */ + test('maxDrop with no history: is refused at declaration, not silently inert', () => { + let thrown: unknown; + try { + scrape(define({ expect: { maxDrop: 0.5 } })); + } catch (error) { + thrown = error; + } + expect(thrown).toBeUltimateError('X_SCRAPE_YIELD_HISTORY_MISSING'); + expect((thrown as { fix?: string }).fix).toContain('history:'); + }); + + test('minRows alone needs no history — an absolute floor has no baseline to miss', () => { + expect(() => scrape(define({ expect: { minRows: 5 } }))).not.toThrow(); + }); + + test('maxDrop WITH a history store is declared without complaint', () => { + expect(() => + scrape(define({ expect: { maxDrop: 0.5 }, history: memoryYieldHistory() })), + ).not.toThrow(); + }); + test('the hybrid leg runs off the same declaration: browser walk, then the JSON endpoint', async () => { const handle = scrape( define({ diff --git a/packages/scraping/src/scrape.ts b/packages/scraping/src/scrape.ts index b2c1dcf4a..2e7254465 100644 --- a/packages/scraping/src/scrape.ts +++ b/packages/scraping/src/scrape.ts @@ -18,6 +18,7 @@ import type { ArtifactWriter } from './artifacts'; import type { PromptHandler, ScrapeAuth } from './auth'; import type { ScrapeClock } from './clock'; import type { ScrapeDriver } from './driver'; +import { yieldHistoryMissing } from './error-throws'; import type { YieldExpectation, YieldHistory } from './expect'; import type { HostRule } from './hosts'; import type { ScrapeHttp } from './http'; @@ -137,6 +138,12 @@ export function scrape(definition: ScrapeDefinition): JobHandle< `scrape "${definition.name}" declares rate: ${String(definition.rate)} — a rate is navigations per second, greater than zero`, `set rate: 1 on scrape("${definition.name}"), or leave it out — to go faster raise the number, there is no unpaced mode`, ); + // Two halves that must be set together. `maxDrop` is a fraction of a trailing median and only + // `history:` can supply one, so declaring it alone is an alarm that cannot fire — refused here, + // where it is written, rather than discovered as a scrape that never once went red. + if (definition.expect?.maxDrop !== undefined && definition.history === undefined) { + throw yieldHistoryMissing(definition.name); + } return job({ name: definition.name, input: definition.input, diff --git a/packages/scraping/src/target.ts b/packages/scraping/src/target.ts index ae56ef106..b4f864e4f 100644 --- a/packages/scraping/src/target.ts +++ b/packages/scraping/src/target.ts @@ -74,9 +74,17 @@ export interface GotoOptions { readonly signal?: AbortSignal | undefined; } +/** + * `fullPage` and nothing else. It carried a required `timeoutMs` until 2026-08 that NO driver + * honoured — `cdp-target.ts` read only `fullPage`, `html-target.ts` ignored the whole object — + * so `page.screenshot({ timeout })` was a documented deadline that bounded nothing. Deleted + * rather than implemented: the CDP port's own `screenshot({ fullPage })` has no timeout slot to + * forward it to, and a deadline enforced in `page-over-target.ts` would have to race + * `ScrapeClock.sleep`, which under `testClock` resolves on the first microtask and would time + * out every capture in every test. A driver's own default is the honest bound. + */ export interface CaptureOptions { readonly fullPage?: boolean | undefined; - readonly timeoutMs: number; } /** @@ -93,7 +101,16 @@ export interface ScrapeTarget { /** Serialised HTML of THIS target — the document for a page, the subtree for a frame. */ content(): Promise; query(selector: string): Promise; - click(selector: string, index: number): Promise; + /** + * Clicks the FIRST match. It took an `index` until 2026-08 that `html-target.ts` honoured and + * `cdp-target.ts` dropped — its implementations are `click: (selector) => …`, so puppeteer + * clicked match 0 whatever was asked. They agreed only because `page-over-target.ts`, the sole + * caller, always passed `0`, and no public vocabulary could set it: `ScrapeFrame.click` takes + * `(selector, options?: WaitOptions)`. A port member no app can reach and one driver ignores is + * a divergence waiting to be found by an app, so it is gone. `driver-parity.test.ts` pins that + * all three drivers click the first match. + */ + click(selector: string): Promise; /** Appends, exactly as typing does. Clearing first is `fill`'s job at the page level. */ type(selector: string, text: string): Promise; clear(selector: string): Promise; diff --git a/packages/storage/src/accept-secretless.test.ts b/packages/storage/src/accept-secretless.test.ts new file mode 100644 index 000000000..0d92624c1 --- /dev/null +++ b/packages/storage/src/accept-secretless.test.ts @@ -0,0 +1,224 @@ +// The mount blocker, as a test. `acceptSignedUpload` required a `secret:` and `localDriver` closes +// over the one it mints with, so a route holding a `Storage` registry had no way to reach it — the +// upload half shipped, tested, and reachable from nothing. These are the cases a route needs: +// verify through the disk that signed, and refuse clearly when the disk cannot. + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { frozenClock } from '@ultimat3/core'; +import { acceptSignedUpload, readSignedObject } from './accept'; +import type { StorageDriver } from './driver'; +import { localDriver } from './driver-local'; +import { isStorageError } from './errors'; +import { grantUpload } from './grant'; +import { defineStorage, resetStorage } from './storage'; +import { uploadPolicy } from './upload'; + +const SECRET = 'test-signing-secret'; +const START = '2026-07-26T12:00:00.000Z'; +const ORG = 'org-1'; +const IMAGES = uploadPolicy({ maxBytes: 1024, allowedContentTypes: ['image/png'] }); + +let root = ''; +let disk: StorageDriver; +let clock: ReturnType; + +function genuinePng(padding = 32): Uint8Array { + const header = [ + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0x0d, 0x49, 0x48, 0x44, 0x52, + ]; + return new Uint8Array([...header, ...new Array(padding).fill(0x01)]); +} + +async function outcomeOf(fn: () => Promise): Promise> { + try { + await fn(); + } catch (error) { + if (!isStorageError(error)) return { code: `not-a-storage-error: ${String(error)}` }; + return { code: error.code, reason: error.meta?.['reason'] }; + } + return { code: 'no-error-thrown' }; +} + +beforeEach(async () => { + root = await mkdtemp(`${tmpdir()}/ultimate-storage-secretless-`); + clock = frozenClock(START); + disk = localDriver({ root, signingSecret: SECRET, clock }); +}); + +afterEach(async () => { + await rm(root, { recursive: true, force: true }); +}); + +const grant = (): ReturnType => + grantUpload({ + disk, + orgId: ORG, + policy: IMAGES, + clock, + uploadId: () => 'upload-1', + request: { filename: 'holiday.png', contentType: 'image/png' }, + }); + +describe('a disk verifies what it signed, without surrendering the secret', () => { + test('the driver exposes verification and NOT the secret it verifies with', () => { + expect(typeof disk.verifySigned).toBe('function'); + // The whole point. A route holds the driver; a route must not be able to mint with it. + expect(Object.values(disk)).not.toContain(SECRET); + expect(JSON.stringify(disk)).not.toContain(SECRET); + }); + + test('an upload is accepted with no secret in the call at all', async () => { + const minted = await grant(); + const stored = await acceptSignedUpload({ + url: minted.url, + disk, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock, + }); + expect(stored.key).toBe(minted.key); + expect(await disk.exists(minted.key)).toBe(true); + }); + + test('a read is served with no secret either — both halves of the route, one seam', async () => { + const minted = await grant(); + await acceptSignedUpload({ + url: minted.url, + disk, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock, + }); + const url = await disk.signedUrl(minted.key); + const read = await readSignedObject({ url, disk, orgId: ORG, clock }); + expect(read.object.key).toBe(minted.key); + }); + + test('every refusal still refuses — the seam moves who holds the secret, not what is checked', async () => { + const minted = await grant(); + const forged = minted.url.replace(/x-sig=[^&]*/, 'x-sig=deadbeef'); + const accept = + (url: string, orgId = ORG): (() => Promise) => + (): Promise => + acceptSignedUpload({ + url, + disk, + orgId, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock, + }); + + expect(await outcomeOf(accept(forged))).toEqual({ + code: 'X_STORAGE_URL_INVALID', + reason: 'signature-mismatch', + }); + expect((await outcomeOf(accept(minted.url, 'org-2')))['code']).toBe('X_STORAGE_ORG_MISMATCH'); + clock.advance(60 * 60 * 1000); + expect((await outcomeOf(accept(minted.url)))['code']).toBe('X_STORAGE_URL_EXPIRED'); + }); + + test('the CALL’s clock governs expiry, not the one the disk was built with', async () => { + // A route's disk is built at boot with the system clock; the caller's clock is the one that + // knows what "now" means for this request. Without the override a frozen-clock caller would be + // judged against wall time, and every expiry assertion in a suite would pass for that reason. + const bootClock = frozenClock(START); + const bootDisk = localDriver({ root, signingSecret: SECRET, clock: bootClock }); + const minted = await grantUpload({ + disk: bootDisk, + orgId: ORG, + policy: IMAGES, + clock: bootClock, + uploadId: () => 'upload-3', + request: { filename: 'holiday.png', contentType: 'image/png' }, + }); + + const late = frozenClock(START); + late.advance(60 * 60 * 1000); + const outcome = await outcomeOf(() => + acceptSignedUpload({ + url: minted.url, + disk: bootDisk, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock: late, + }), + ); + expect(outcome['code']).toBe('X_STORAGE_URL_EXPIRED'); + // …and the disk's own clock still answers when the call names none. + expect(await disk.verifySigned?.({ url: minted.url })).toBeDefined(); + }); + + test('an explicit secret still wins, so every call that shipped keeps working', async () => { + const minted = await grant(); + const stored = await acceptSignedUpload({ + url: minted.url, + secret: SECRET, + disk, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock, + }); + expect(stored.key).toBe(minted.key); + }); +}); + +describe('a disk that can neither be given a secret nor verify one', () => { + test('refuses by name, and names both ways out', async () => { + // An s3-shaped driver: the URLs are the provider's, so this package never verifies them. The + // key is OMITTED rather than set to `undefined` — `verifySigned?` is an optional member under + // `exactOptionalPropertyTypes`, and a driver that declares it as absent is the real shape. + const { verifySigned: _unused, ...rest } = disk; + const provider: StorageDriver = { ...rest, name: 's3' }; + const outcome = await outcomeOf(() => + readSignedObject({ url: '/_storage/s3/a.png?x-sig=x', disk: provider, orgId: ORG, clock }), + ); + expect(outcome['code']).toBe('X_STORAGE_URL_UNVERIFIABLE'); + }); +}); + +describe('what a mounted route actually holds', () => { + // The blocker, stated as the route's own view: `storageRoutes({ storage })` is handed a REGISTRY + // and nothing else — no secret, no config — which is why the shipped `PUT` half of + // `/_storage/:disk/*key` has never been mounted. This is that handler, minus the Request. + test('a disk resolved from the registry accepts an upload with nothing but the registry', async () => { + const registry = defineStorage({ disks: { uploads: disk }, default: 'uploads' }); + try { + const minted = await grantUpload({ + disk: registry.disk('uploads'), + orgId: ORG, + policy: IMAGES, + clock, + uploadId: () => 'upload-2', + request: { filename: 'holiday.png', contentType: 'image/png' }, + }); + + // Everything below is reachable from `{ params, headers, body }` and the registry. + const resolved = registry.disk('uploads'); + const stored = await acceptSignedUpload({ + url: minted.url, + disk: resolved, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + clock, + }); + + expect(stored.key).toBe(minted.key); + expect(await resolved.exists(stored.key)).toBe(true); + } finally { + resetStorage(); + } + }); +}); diff --git a/packages/storage/src/accept.ts b/packages/storage/src/accept.ts index 38f92e85c..92af30478 100644 --- a/packages/storage/src/accept.ts +++ b/packages/storage/src/accept.ts @@ -8,9 +8,15 @@ import type { Clock } from '@ultimat3/core'; import type { SignedUrlMethod, StorageDriver, StorageObject, StorageRead } from './driver'; -import { orgMismatch, signedUrlExpired, signedUrlRejected, tooLarge } from './errors'; +import { + orgMismatch, + signedUrlExpired, + signedUrlRejected, + signedUrlUnverifiable, + tooLarge, +} from './errors'; import { isTenantScoped, isWithinOrg } from './path'; -import type { SignedUrlConstraints } from './signed-url'; +import type { SignedUrlConstraints, SignedUrlVerification } from './signed-url'; import { signedUrlBaseFor, verifySignedUrl } from './signed-url'; import type { UploadPolicy } from './upload'; import { normalizeContentType, uploadPolicy, validateUpload } from './upload'; @@ -18,7 +24,16 @@ import { normalizeContentType, uploadPolicy, validateUpload } from './upload'; export interface SignedRequestInput { /** Absolute or route-relative — `verifySignedUrl` parses both. */ readonly url: string; - readonly secret: string; + /** + * OPTIONAL since the disk can verify its own. It was required, and that requirement is why the + * upload half of `/_storage` has never been mounted: `localDriver` closes over the secret it + * mints with and exposes it through no member, so a route holding a `Storage` registry had no + * value to put here. A disk implementing `verifySigned()` needs none. + * + * Still honoured, and it still wins, for a caller that verifies a URL some OTHER disk signed — + * a migration between disks, a test signing by hand. + */ + readonly secret?: string | undefined; /** * Defaults to the base THIS disk signs under (`disk.signedUrlBase`), never to the bare mount * prefix: a second default here made every URL `localDriver` mints a signature-mismatch, @@ -56,16 +71,33 @@ export interface AcceptSignedUploadInput extends SignedRequestInput { * the reason it exists is that a forged URL must never learn "the signature was fine, just late". * The org check runs on the verified key, so an attacker cannot probe org names with a fake one. */ +async function verify(input: SignedRequestInput): Promise { + // An explicit secret first, so every call that shipped behaves exactly as it did — including + // one verifying a URL a DIFFERENT disk signed, which the driver could not answer for. + if (input.secret !== undefined && input.secret !== '') { + return verifySignedUrl({ + url: input.url, + secret: input.secret, + baseUrl: input.baseUrl ?? input.disk.signedUrlBase ?? signedUrlBaseFor(input.disk.name), + ...(input.clock === undefined ? {} : { clock: input.clock }), + }); + } + // The disk that signed it verifies it, holding the key it never hands out. `baseUrl` is the + // driver's own here for the same reason the secret is: both are the minting half's, and stating + // either one a second time is what made every `localDriver` URL a signature mismatch once. + const own = input.disk.verifySigned; + if (own === undefined) throw signedUrlUnverifiable(input.disk.name); + return own.call(input.disk, { + url: input.url, + ...(input.clock === undefined ? {} : { clock: input.clock }), + }); +} + async function constraintsFor( input: SignedRequestInput, method: SignedUrlMethod, ): Promise { - const result = await verifySignedUrl({ - url: input.url, - secret: input.secret, - baseUrl: input.baseUrl ?? input.disk.signedUrlBase ?? signedUrlBaseFor(input.disk.name), - ...(input.clock === undefined ? {} : { clock: input.clock }), - }); + const result = await verify(input); if (!result.ok) { throw result.reason === 'expired' ? signedUrlExpired(input.url, result.detail) diff --git a/packages/storage/src/driver-local.ts b/packages/storage/src/driver-local.ts index 9372f185e..aef9c9f47 100644 --- a/packages/storage/src/driver-local.ts +++ b/packages/storage/src/driver-local.ts @@ -29,7 +29,8 @@ import { storageNotImplemented, } from './errors'; import { assertSafeKey, META_DIR } from './path'; -import { buildSignedUrl, signedUrlBaseFor } from './signed-url'; +import type { SignedUrlVerification } from './signed-url'; +import { buildSignedUrl, signedUrlBaseFor, verifySignedUrl } from './signed-url'; import { DEFAULT_MAX_UPLOAD_BYTES } from './upload'; const DRIVER_NAME = 'local'; @@ -380,5 +381,27 @@ export function localDriver(options: LocalDriverOptions): StorageDriver { clock, }); }, + + /** + * The mint above, run backwards, on the same three values from the same closure: `secret`, + * `baseUrl` and `clock`. That is the whole reason it belongs here rather than in a caller — a + * route holding this disk can now accept an upload without ever being handed the key, and + * before this member existed there was no way for one to verify at all, which is why the + * shipped `PUT` half of `/_storage` was never mounted. + * + * Never throws: `verifySignedUrl` returns a reason, and `accept.ts` owns which reason becomes + * which error. A driver that decided that here would be a second error taxonomy. + */ + async verifySigned(input: { + readonly url: string; + readonly clock?: Clock | undefined; + }): Promise { + return verifySignedUrl({ + url: input.url, + secret, + baseUrl, + clock: input.clock ?? clock, + }); + }, }; } diff --git a/packages/storage/src/driver.ts b/packages/storage/src/driver.ts index 916ea28b6..26ab079f1 100644 --- a/packages/storage/src/driver.ts +++ b/packages/storage/src/driver.ts @@ -3,8 +3,10 @@ // strings — a base64 round trip through JSON is how a "small" upload becomes a 33%-larger OOM — // and never unbounded, which is the same failure with the sender choosing the size. +import type { Clock } from '@ultimat3/core'; import { assert } from '@ultimat3/core'; import { putTooLarge } from './errors'; +import type { SignedUrlVerification } from './signed-url'; export type StorageBody = Uint8Array | ReadableStream | Blob; @@ -131,6 +133,28 @@ export interface StorageDriver { exists(key: string): Promise; list(options?: ListOptions): Promise; signedUrl(key: string, options?: SignedUrlOptions): Promise; + /** + * Verify a URL THIS disk minted — without surrendering the key it minted with. + * + * The other half of `signedUrlBase`, and it was missing for the whole life of the package. + * `signedUrlBase` is on this interface so the minting half and the verifying half cannot state + * the base twice; the SECRET is the same pair and stayed closed over inside `localDriver`, so + * `acceptSignedUpload` — whose `secret:` was required — was reachable from no route that held + * only a `Storage` registry. That is why the shipped upload path has no mounted `PUT`. + * + * Verification and not the secret, deliberately. A member that handed the key back would let any + * holder of a driver MINT a URL for any key, and would put it in every `JSON.stringify(disk)` a + * log or an error payload performs. A driver can answer "did I sign this?" without that. + * + * Optional, like `registerAs`: a disk whose URLs are the provider's (`s3`) verifies at the + * provider and never reaches this path, and a third-party driver that implements neither is + * refused by name (`X_STORAGE_URL_UNVERIFIABLE`) rather than reported as a bad signature. + */ + verifySigned?(input: { + readonly url: string; + /** Overrides the driver's own, so a test's frozen clock still governs expiry. */ + readonly clock?: Clock | undefined; + }): Promise; } export const DEFAULT_CONTENT_TYPE = 'application/octet-stream'; diff --git a/packages/storage/src/errors.ts b/packages/storage/src/errors.ts index 7193bc4f2..92016fabe 100644 --- a/packages/storage/src/errors.ts +++ b/packages/storage/src/errors.ts @@ -14,6 +14,7 @@ export const STORAGE_OWNED_ERROR_CODES = [ 'X_STORAGE_CHECKSUM_MISMATCH', 'X_STORAGE_URL_INVALID', 'X_STORAGE_URL_EXPIRED', + 'X_STORAGE_URL_UNVERIFIABLE', 'X_STORAGE_ORG_MISMATCH', 'X_STORAGE_UPLOAD_FAILED', 'X_STORAGE_DELETE_FAILED', @@ -48,6 +49,7 @@ export const STORAGE_ERROR_TITLES: Readonly + new StorageError({ + code: 'X_STORAGE_URL_UNVERIFIABLE', + cause: `the "${diskName}" disk was asked to verify a signed URL with no secret in the call, and its driver implements no verifySigned()`, + fix: 'pass secret: to the call, or use a driver that signs its own URLs (localDriver) — a provider-signed disk (s3) verifies at the provider and never reaches this path', + meta: { disk: diskName }, + }); + export const signingSecretMissing = (environment: string): StorageError => new StorageError({ code: 'X_ENV_MISSING', diff --git a/packages/storage/src/index.ts b/packages/storage/src/index.ts index ee4b6dec0..a830bb827 100644 --- a/packages/storage/src/index.ts +++ b/packages/storage/src/index.ts @@ -87,6 +87,7 @@ export { StorageError, signedUrlExpired, signedUrlRejected, + signedUrlUnverifiable, signingSecretMissing, storageNotImplemented, tooLarge, diff --git a/packages/testing/src/fixture-drivers.ts b/packages/testing/src/fixture-drivers.ts index 93eee41e7..0daafa0e4 100644 --- a/packages/testing/src/fixture-drivers.ts +++ b/packages/testing/src/fixture-drivers.ts @@ -21,6 +21,14 @@ export interface TestDeploy { /** * What `query.live(input, { actor })` resolves to, named structurally so `@ultimat3/testing` does * not take a dependency on `@ultimat3/query` for one type. The real `LiveQuery` satisfies it. + * + * NOT what `query.as(actor, input)` resolves to, which is a ROW ARRAY — and the difference is + * invisible until a driver exists. `examples/dummy`'s five `subscribe` tests all call + * `subscribe(liveFeed.as(actor, input))`, which is `TS2345` against `Subscribe` below and has + * never been read, because that app's `typecheck` is pinned red in `scripts/lib/gated-apps.ts`. + * A driver built to satisfy those call sites would have to accept a row array — and a `subscribe` + * loose enough to do that proves nothing, which is strictly worse than the fixture being + * unavailable, because it then reads as coverage. */ export interface LiveTarget { readonly name: string; @@ -62,7 +70,9 @@ export const DRIVER_FIXTURE_NEEDS: Readonly> = deploy: 'a second build to switch the running app to', page: 'a browser driving the built app', signIn: 'a browser session against the app’s own sign-in route', - subscribe: 'an in-process replicator feeding the live-query registry', + subscribe: + 'an in-process replicator feeding the live-query registry, and a caller that hands it ' + + 'query.live(input, { actor }) — query.as() resolves to ROWS, which is not a LiveTarget', }; /** diff --git a/scripts/generator-counts.test.ts b/scripts/generator-counts.test.ts new file mode 100644 index 000000000..e7f951c3b --- /dev/null +++ b/scripts/generator-counts.test.ts @@ -0,0 +1,152 @@ +// The rule's own negative cases, as fixtures rather than as edits to pages three other people are +// rewriting — the reason `checkGeneratorCounts` is pure over a page list. + +import { describe, expect, test } from 'bun:test'; +import { + checkGeneratorCounts, + type GeneratorFacts, + generatorFacts, + ownerOf, + readCountClaims, + readMentions, + readTranscriptClaims, +} from './generator-counts'; + +const FACTS: readonly GeneratorFacts[] = [ + { + id: 'x new', + variants: [ + { flags: [], files: 125 }, + { flags: ['--no-example'], files: 99 }, + ], + }, + { + id: 'x g resource', + variants: [ + { flags: [], files: 27 }, + { flags: ['--admin'], files: 29 }, + ], + }, +]; + +const gapsFor = (text: string): readonly string[] => + checkGeneratorCounts({ pages: [{ path: 'p.md', text }], facts: FACTS }) + .filter((gap) => gap.kind === 'stale') + .map((gap) => `${gap.generator}:${String(gap.claimed)}`); + +describe('the counts come from the generators, never from this file', () => { + test('every modelled variant is a real plan length, and the plans differ', () => { + const facts = generatorFacts(); + expect(facts.map((entry) => entry.id)).toEqual(['x new', 'x g resource']); + for (const entry of facts) { + for (const variant of entry.variants) expect(variant.files).toBeGreaterThan(0); + const counts = entry.variants.map((variant) => variant.files); + // A flag that changed nothing would make the two variants indistinguishable, and every + // assertion about "the flagged spelling" below would then hold for the wrong reason. + expect(new Set(counts).size).toBe(counts.length); + } + }); +}); + +describe('what counts as a claim', () => { + test('`N files`, `N with` and `N without` are claims; a bare integer is not', () => { + expect(readCountClaims('125 files, 99 with `--no-example`').map((c) => c.value)).toEqual([ + 125, 99, + ]); + // The history on wiki/CLI-Reference.md. A rule that read every integer fails on this line. + expect(readCountClaims('up from 114/90 because it grew').map((c) => c.value)).toEqual([]); + }); + + test('flags attach to the claim they follow, never to the next one', () => { + const claims = readCountClaims('**125 files** with the slice, **99** with `--no-example`'); + expect(claims[0]?.flags).toEqual([]); + expect(claims[1]?.flags).toEqual(['--no-example']); + expect(claims[1]?.associated).toBe(true); + }); + + test('`without` never associates its flags — the corpus writes the inverse sentence', () => { + const [first, second] = readCountClaims('29 files — 27 without `--admin`'); + expect(first?.value).toBe(29); + expect(second?.associated).toBe(false); + }); + + test('a count qualified by a path or a chart is about that artifact, not the generator', () => { + expect(readCountClaims('`x new` writes `docker/helm`, 8 files')[0]?.qualified).toBe(true); + expect(readCountClaims('helm/ — the chart, 8 files')[0]?.qualified).toBe(true); + expect(readCountClaims('the whole slice — 27 files')[0]?.qualified).toBe(false); + }); +}); + +describe('which generator a claim belongs to', () => { + test('the invocation nearest before it wins, so one line can state two', () => { + const line = '`x g job` is 5 files into a bare slice, `x g action` 8 files.'; + const mentions = readMentions(line); + const claims = readCountClaims(line); + expect(ownerOf(mentions, claims[0] as never)).toBe('x g job'); + expect(ownerOf(mentions, claims[1] as never)).toBe('x g action'); + }); + + test('a claim before the only invocation on the line still belongs to it', () => { + const line = '**125 files** with the slice — measured because `x new` now writes more'; + expect(ownerOf(readMentions(line), readCountClaims(line)[0] as never)).toBe('x new'); + }); + + test('a claim before two different invocations belongs to neither', () => { + const line = '27 files, then `x new` and `x g resource` both changed'; + expect(ownerOf(readMentions(line), readCountClaims(line)[0] as never)).toBeUndefined(); + }); +}); + +describe('the two rules', () => { + test('membership: a count no variant emits is stale, whichever direction the prose runs', () => { + // Two findings, and the second is the association rule below: 27 is a real count of this + // generator, but not of the spelling the sentence attaches it to. + expect(gapsFor('`x g resource` — 25 files, 27 with `--admin`')).toEqual([ + 'x g resource:25', + 'x g resource:27', + ]); + // The inverted spelling on docs/architecture/15-adding-a-feature.md must stay green. + expect(gapsFor('`x g resource --live --admin` | 29 files — 27 without either flag')).toEqual( + [], + ); + }); + + test('association: a valid number under the wrong flag is stale, which membership cannot see', () => { + expect(gapsFor('`x g resource` — 27 files, 27 with `--admin`')).toEqual(['x g resource:27']); + }); + + test('a swap with no `with` is NOT caught — the limitation, pinned so it cannot be assumed away', () => { + expect(gapsFor('`x g resource` — 29 files, 27 without either flag')).toEqual([]); + }); + + test('a path-qualified count is not read as the generator total', () => { + expect(gapsFor('`x new` writes `docker/helm`, 8 files')).toEqual([]); + }); + + test('the generator’s own transcript line parses to a count and a kind', () => { + // `cli.generate.wrote` in packages/cli/src/messages.ts, and `--dry-run`'s `would write`. + expect(readTranscriptClaims('✓ wrote 25 file(s) for resource todo')).toEqual([ + { value: 25, id: 'x g resource' }, + ]); + expect(readTranscriptClaims('would write 4 file(s) for job nudge')).toEqual([ + { value: 4, id: 'x g job' }, + ]); + expect(readTranscriptClaims('wrote the file')).toEqual([]); + }); + + test('the generator’s own transcript line is checked exactly', () => { + expect(gapsFor('✓ wrote 25 file(s) for resource todo')).toEqual(['x g resource:25']); + expect(gapsFor('✓ wrote 27 file(s) for resource todo')).toEqual([]); + expect(gapsFor('would write 99 file(s) for new demo')).toEqual([]); + }); +}); + +describe('the false green', () => { + test('a corpus stating no count at all is a finding, not agreement', () => { + const gaps = checkGeneratorCounts({ + pages: [{ path: 'p.md', text: 'no numbers' }], + facts: FACTS, + }); + expect(gaps.map((gap) => gap.kind)).toEqual(['vacuous']); + }); +}); diff --git a/scripts/generator-counts.ts b/scripts/generator-counts.ts new file mode 100644 index 000000000..9dede1c8d --- /dev/null +++ b/scripts/generator-counts.ts @@ -0,0 +1,330 @@ +#!/usr/bin/env bun +// Enforce, as a gate step, that every file count a doc states about a generator is a number that +// generator still produces. The counts are DERIVED here, from the same pure planners `--dry-run` +// calls — never restated — so a template that adds a file makes the prose red on the same commit. +// +// The gap this closes: `x new` 125, `--no-example` 99, `x g resource` 27, `--live --admin` 29 and +// `x g action` 9 were a convention held by nobody. Five had gone stale and were corrected in #209 +// by running the generators BY HAND, which is the step that does not happen again. Measured while +// writing this: `wiki/Tutorial-02-First-Feature.md` still claimed 25 and 27 for `x g resource`, +// two more than #209 caught, because that page states them in prose rather than in a table row. +// +// bun run scripts/generator-counts.ts [--json] + +import { generate, planNewApp } from '@ultimat3/cli'; +import { readDocPages } from './doc-commands'; +import { parseScriptArgs } from './lib/args'; +import type { MarkdownFile } from './lib/doc-citations'; +import type { Finding } from './lib/log'; +import { report } from './lib/log'; +import { repoRoot } from './lib/run'; + +/** One spelling of one generator, and the file count it really emits. */ +export interface GeneratorVariant { + /** The flags that select it, as a reader writes them. Empty is the default spelling. */ + readonly flags: readonly string[]; + readonly files: number; +} + +export interface GeneratorFacts { + /** How a finding names it: `x new`, `x g resource`. */ + readonly id: string; + readonly variants: readonly GeneratorVariant[]; +} + +/** + * Measured, not declared. `planNewApp` and `generate` are the pure planners `x new --dry-run` and + * `x g --dry-run` both call (`cmd-new.ts`, `cmd-generate.ts`), so this is the identical list a + * reader re-deriving the number with `--dry-run --json` would count — and it costs ~30ms rather + * than the ~4.5s five subprocesses would. + */ +/** + * `x g action` is deliberately absent, and the reason is a property of the generator rather than an + * omission. `writeFiles` SKIPS a slice module that already exists (`cmd-generate.ts`), so an + * action's count depends on how much of the slice is already there — `wiki/CLI-Reference.md:233` + * says "exactly its own 3 files" into a finished slice and `wiki/Tutorial-02` says 8 into a bare + * one, both correct English about different states. `generate()` returns the full plan and can + * derive neither, so modelling it would report findings on prose that is right. + * + * `x new` and `x g resource` have no such dimension: one creates the repository and the other + * creates its own slice, so their plans ARE their counts. + */ +export function generatorFacts(): readonly GeneratorFacts[] { + const resource = (over: { admin?: boolean }): number => + generate({ kind: 'resource', name: 'widget', ...over }).length; + return [ + { + id: 'x new', + variants: [ + { flags: [], files: planNewApp({ name: 'probe', example: true }).length }, + { flags: ['--no-example'], files: planNewApp({ name: 'probe', example: false }).length }, + ], + }, + { + id: 'x g resource', + variants: [ + { flags: [], files: resource({}) }, + { flags: ['--admin'], files: resource({ admin: true }) }, + ], + }, + ]; +} + +/** + * Every generator an `x g ` / `x new` span on a line names, modelled or not. Unmodelled ones + * are tracked ON PURPOSE: `wiki/Tutorial-02` writes "`x g job` is 5 files … `x g action` 8" on one + * line, and a scanner that saw only the generators it models would hand `x g job`'s 5 to + * `x g action` and report a finding on a sentence that is right. + */ +const MENTION = /\bx new\b|\bx g ([a-z:]+)/g; + +export interface Mention { + readonly at: number; + readonly id: string; +} + +export function readMentions(line: string): readonly Mention[] { + return [...line.matchAll(MENTION)].map((match) => ({ + at: match.index ?? 0, + id: match[1] === undefined ? 'x new' : `x g ${match[1]}`, + })); +} + +/** A number a doc line states as a file count, with what the same line associates with it. */ +export interface CountClaim { + readonly at: number; + readonly value: number; + /** The `--flag` tokens between this claim and the next one on the line. */ + readonly flags: readonly string[]; + /** `with` associates flags; `without`, a bare `N files` and an elided count do not. */ + readonly associated: boolean; + /** Whether the run-up to this claim named a different subject. See `checkGeneratorCounts`. */ + readonly qualified: boolean; +} + +/** + * A number counts as a claim only when the prose says so: `N files`, `N file(s)`, `N with`/ + * `N without` a variant, or the elision a table cell uses — an invocation followed by a bare + * number (`` `x g action` 8 ``). Everything else is left alone: `up from 114/90` on + * `wiki/CLI-Reference.md` is a HISTORY, and a rule that read every integer would fail on it. + */ +const CLAIM = /(\d+)\*{0,2}\s*(files?\b|file\(s\)|with\b|without\b)/g; +const ELIDED = /(?:\bx new\b|\bx g [a-z:]+)`?\s+(\d+)\b/g; + +/** + * The generator's OWN success line, quoted in a transcript: `cli.generate.wrote` in + * `packages/cli/src/messages.ts` is `wrote {count} file(s) for {kind} {name}`, and `--dry-run`'s + * is `would write {count} …`. A tutorial that pastes one is stating a count with no prose around + * it to misread, which is why it is matched exactly rather than left to the heuristics above — + * `wiki/Tutorial-02-First-Feature.md` shows `✓ wrote 25 file(s) for resource todo` and the + * generator writes 27. + */ +const TRANSCRIPT = /(?:wrote|would write)\s+(\d+)\s+file\(s\)\s+for\s+([a-z:]+)\b/g; + +export interface TranscriptClaim { + readonly value: number; + readonly id: string; +} + +export const readTranscriptClaims = (line: string): readonly TranscriptClaim[] => + [...line.matchAll(TRANSCRIPT)].map((match) => ({ + value: Number.parseInt(match[1] ?? '0', 10), + id: `x g ${match[2] ?? ''}`, + })); + +/** + * A count qualified by a NAMED ARTIFACT is a count about that artifact, not about the generator. + * Five pages write "`x new` writes … `docker/helm`, 8 files" and one writes "helm/ — the chart, 8 + * files"; every one is correct English and none is a claim about `x new`'s output. + * + * This is a heuristic and the only one in the file — a path-shaped token or the word `chart` + * between the previous claim and this one. It is stated rather than hidden because the honest + * alternative is parsing English, and the cost of getting it wrong is a false finding on prose + * that is right, which is how a gate stops being read. + */ +const QUALIFIER = /[\w.-]+\/[\w./-]*|\bcharts?\b/; + +export function readCountClaims(line: string): readonly CountClaim[] { + const found = [ + ...[...line.matchAll(CLAIM)].map((match) => ({ + at: match.index ?? 0, + end: (match.index ?? 0) + match[0].length, + value: Number.parseInt(match[1] ?? '0', 10), + associated: match[2] === 'with', + })), + // Positioned at the NUMBER, not at the invocation that precedes it: `at` is what decides which + // invocation owns the claim, and anchoring it on the mention made every elided count belong to + // whichever generator came before its own. + ...[...line.matchAll(ELIDED)].map((match) => ({ + at: (match.index ?? 0) + match[0].length - (match[1] ?? '').length, + end: (match.index ?? 0) + match[0].length, + value: Number.parseInt(match[1] ?? '0', 10), + associated: false, + })), + ] + .sort((a, b) => a.at - b.at) + // `x g action` 8 files matches BOTH forms at the same offset; one claim, reported once. + .filter((claim, index, all) => index === 0 || claim.at !== all[index - 1]?.at); + return found.map((claim, index) => ({ + at: claim.at, + value: claim.value, + associated: claim.associated, + flags: [ + ...line.slice(claim.end, found[index + 1]?.at ?? line.length).matchAll(/--[a-z][a-z-]*/g), + ].map((flag) => flag[0]), + qualified: QUALIFIER.test(line.slice(index === 0 ? 0 : (found[index - 1]?.end ?? 0), claim.at)), + })); +} + +export interface CountGapInput { + readonly pages: readonly MarkdownFile[]; + readonly facts: readonly GeneratorFacts[]; +} + +export interface CountGap { + readonly kind: 'stale' | 'vacuous'; + readonly at: string; + readonly generator: string; + readonly claimed: number; + readonly expected: readonly number[]; + readonly detail: string; +} + +const variantFor = (facts: GeneratorFacts, claim: CountClaim): GeneratorVariant | undefined => + claim.associated && claim.flags.length > 0 + ? facts.variants.find((variant) => variant.flags.some((flag) => claim.flags.includes(flag))) + : undefined; + +/** + * Which generator a claim is about: the invocation NEAREST BEFORE it on the line, and when none + * precedes it, the line's sole invocation. `wiki/CLI-Reference.md` states `x new`'s counts in a + * paragraph whose only mention of the command comes after them, and a rule that required the + * mention first would read that page's numbers as belonging to nothing. + * + * `undefined` when the line names two invocations and the claim precedes both — ambiguous, and a + * guess there is a finding on prose nobody can act on. + */ +export function ownerOf(mentions: readonly Mention[], claim: CountClaim): string | undefined { + const before = mentions.filter((mention) => mention.at < claim.at); + if (before.length > 0) return before[before.length - 1]?.id; + const ids = new Set(mentions.map((mention) => mention.id)); + return ids.size === 1 ? mentions[0]?.id : undefined; +} + +/** + * Two rules, and the weaker one is deliberate. + * + * MEMBERSHIP: every count a line states about a generator must be a number that generator emits in + * SOME documented variant. Direction-free on purpose — the corpus states the same fact five ways + * (`29 files — 27 without either flag` inverts `27 files (29 with --admin)`), and a rule that + * insisted on reading which is which would report findings on prose that is correct. + * + * ASSOCIATION: when a line writes `N with --flag`, N must be THAT variant's count. This is what + * catches a stale number that is still valid for the other spelling, which membership cannot see. + * Applied only to `with`, never to `without`, for the same reason. + * + * Neither catches a line that swaps two currently-valid numbers between variants without writing + * `with`. That is stated rather than papered over: the alternative is parsing English. + */ +export function checkGeneratorCounts(input: CountGapInput): readonly CountGap[] { + const gaps: CountGap[] = []; + const byId = new Map(input.facts.map((facts) => [facts.id, facts])); + let claims = 0; + for (const page of input.pages) { + const lines = page.text.split('\n'); + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ''; + for (const spoken of readTranscriptClaims(line)) { + const facts = byId.get(spoken.id); + if (facts === undefined) continue; + claims += 1; + const every = facts.variants.map((variant) => variant.files); + if (every.includes(spoken.value)) continue; + gaps.push({ + kind: 'stale', + at: `${page.path}:${index + 1}`, + generator: facts.id, + claimed: spoken.value, + expected: every, + detail: `emits ${every.join(' or ')} files`, + }); + } + const mentions = readMentions(line); + if (mentions.length === 0) continue; + for (const claim of readCountClaims(line)) { + if (claim.qualified) continue; + const facts = byId.get(ownerOf(mentions, claim) ?? ''); + if (facts === undefined) continue; + claims += 1; + const every = facts.variants.map((variant) => variant.files); + const variant = variantFor(facts, claim); + const expected = variant === undefined ? every : [variant.files]; + if (expected.includes(claim.value)) continue; + gaps.push({ + kind: 'stale', + at: `${page.path}:${index + 1}`, + generator: facts.id, + claimed: claim.value, + expected, + detail: + variant === undefined + ? `emits ${every.join(' or ')} files` + : `emits ${String(variant.files)} files with ${variant.flags.join(' ')}`, + }); + } + } + } + if (claims === 0) { + gaps.push({ + kind: 'vacuous', + at: 'wiki/', + generator: '', + claimed: 0, + expected: [], + detail: 'no page states a generator file count, so this rule read nothing and reported green', + }); + } + return gaps; +} + +const staleFinding = (gap: CountGap): Finding => ({ + code: 'X_DOC_FILE_COUNT_STALE', + cause: `${gap.at} says ${gap.generator} produces ${String(gap.claimed)} files, and it ${gap.detail}`, + fix: `set the count at ${gap.at} to ${gap.expected.join(' or ')}, or re-derive it with \`x g --dry-run --json\` / \`x new --dry-run --json\` and count data.files`, + at: gap.at, +}); + +const vacuousFinding = (gap: CountGap): Finding => ({ + code: 'X_DOC_FILE_COUNT_UNSCANNED', + cause: gap.detail, + fix: 'check DOC_GLOBS in scripts/doc-commands.ts still matches the pages, and CLAIM in scripts/generator-counts.ts still matches how they state a count', + at: 'scripts/generator-counts.ts', +}); + +export const generatorCountFindingFor = (gap: CountGap): Finding => + gap.kind === 'vacuous' ? vacuousFinding(gap) : staleFinding(gap); + +export const generatorCountGaps = async (root: string): Promise => + checkGeneratorCounts({ pages: await readDocPages(root), facts: generatorFacts() }); + +/** What this repo contributes to `x verify`'s `manifest` step. */ +export const generatorCountFindings = async (root: string): Promise => + (await generatorCountGaps(root)).map(generatorCountFindingFor); + +if (import.meta.main) { + const args = parseScriptArgs(Bun.argv.slice(2)); + const gaps = await generatorCountGaps(repoRoot()); + const facts = generatorFacts(); + report( + { + ok: gaps.length === 0, + script: 'generator-counts', + summary: + gaps.length === 0 + ? `every documented file count matches the generators (${facts.map((f) => `${f.id} ${f.variants.map((v) => String(v.files)).join('/')}`).join(', ')})` + : `${gaps.length} documented generator file count(s) no longer match`, + findings: gaps.map(generatorCountFindingFor), + }, + args.json, + ); +} diff --git a/scripts/lib/source-strings.ts b/scripts/lib/source-strings.ts new file mode 100644 index 000000000..24505b350 --- /dev/null +++ b/scripts/lib/source-strings.ts @@ -0,0 +1,98 @@ +// Where a TypeScript source evaluates a string literal. One tiny scanner, because the two things a +// text rule over source gets wrong are both solved by knowing what is a token and what is not: +// +// - a citation inside a COMMENT is prose about a command, not a use of one; +// - a citation inside ANOTHER string is a fixture's source text, not this file's own value — +// `await write('a.ts', "throw new E({ fix: 'x db status' });")` writes a file whose fix is +// deliberately broken, and reading the inner literal as this test's own claim is how a rule +// ends up needing a filename allowlist to excuse the tests that test it. +// +// Not a parser: it tracks comments, quotes and escapes and nothing else. That is exactly enough to +// answer "is this offset code, and is this literal a top-level one", and no more. + +export interface SourceString { + /** Offset of the opening quote. */ + readonly at: number; + readonly quote: "'" | '"' | '`'; + /** The literal's contents, unescaped only for `\\` and the quote itself. */ + readonly value: string; + /** Everything on this literal's line before it — what a caller matches `fix:` against. */ + readonly prefix: string; + /** 1-based. */ + readonly line: number; +} + +const QUOTES = new Set(["'", '"', '`']); + +/** + * Whether an offset is INSIDE a string literal rather than in code. The other half of the same + * question `sourceStrings` answers, and the reason it is here: a text rule over test sources reads + * its own fixtures otherwise — a checker's unit test spells the bad shape as a string on purpose, + * and flagging it is the rule reporting a finding on the test that proves it works. + */ +export const insideString = (strings: readonly SourceString[], at: number): boolean => + strings.some((literal) => at > literal.at && at < literal.at + literal.value.length + 2); + +/** + * Every string literal the code itself evaluates, in order. Nested literals are not returned by + * construction: the scanner consumes a literal whole, so characters inside it are never re-read as + * code. A template's `${…}` is not descended into for the same reason a parser is not written here + * — a citation is asserted as a plain literal in every case this repo has, and guessing at + * interpolation would report findings on strings nobody wrote. + */ +export function sourceStrings(source: string): readonly SourceString[] { + const out: SourceString[] = []; + let index = 0; + let line = 1; + let lineStart = 0; + while (index < source.length) { + const char = source[index] ?? ''; + if (char === '\n') { + line += 1; + index += 1; + lineStart = index; + continue; + } + if (char === '/' && source[index + 1] === '/') { + const end = source.indexOf('\n', index); + index = end === -1 ? source.length : end; + continue; + } + if (char === '/' && source[index + 1] === '*') { + const end = source.indexOf('*/', index + 2); + const stop = end === -1 ? source.length : end + 2; + line += source.slice(index, stop).split('\n').length - 1; + lineStart = source.lastIndexOf('\n', stop) + 1; + index = stop; + continue; + } + if (!QUOTES.has(char)) { + index += 1; + continue; + } + const quote = char as SourceString['quote']; + const prefix = source.slice(lineStart, index); + const openedAt = index; + const openedOn = line; + let value = ''; + index += 1; + while (index < source.length) { + const inner = source[index] ?? ''; + if (inner === '\\') { + value += source[index + 1] ?? ''; + index += 2; + continue; + } + if (inner === quote) { + index += 1; + break; + } + if (inner === '\n') line += 1; + value += inner; + index += 1; + } + out.push({ at: openedAt, quote, value, prefix, line: openedOn }); + lineStart = source.lastIndexOf('\n', index - 1) + 1; + } + return out; +} diff --git a/scripts/lib/test-fix-pins.ts b/scripts/lib/test-fix-pins.ts new file mode 100644 index 000000000..6a9178e3d --- /dev/null +++ b/scripts/lib/test-fix-pins.ts @@ -0,0 +1,68 @@ +// The ratchet under `scripts/test-fix-citations.ts`: how many `fix:` lines each package's own +// TESTS write or assert that cite a command this build cannot run. The number may FALL and may +// never rise. Data only — the gate owns what it does with these. +// +// Measured 2026-08-19 over 30 packages: 8 unrunnable, in 4 packages. Every one is a +// FIXTURE rather than a shipped error — `checkErrorFixes` already holds `src/` to this rule and +// finds nothing — so what is pinned here is a test teaching a reader a command that does not +// exist, which is the shape the two that DID ship (`x schema show`, `x logs tail`) were copied +// from. The seven in `packages/cli/src/error-contract.test.ts` are absent on purpose: they sit +// inside a string that test writes to disk, and the scanner does not read a nested literal as this +// file's own value. +// +// | Where | What it cites | Why it is still here | +// |---|---|---| +// | `packages/ai/src/tools.test.ts` | `x db query "select …"` | a flattened-error fixture; `x db` has no `query` | +// | `packages/core/src/errors.test.ts` | `x storage use local` | `notImplemented`'s pass-through, asserted with an arbitrary string | +// | `packages/flags/src/runtime.test.ts` | `x flags --json` | a helper that builds one `UltimateError` for the reporter tests | +// | `packages/http/src/overlay.test.ts` | `x db preload`, `x db batch` | the dev overlay's N+1 and batch notice fixtures, three lines | +// | `packages/http/src/pipeline.app.test.ts` | `x logs tail` | a fixture app handler, twice, citing the exact command that shipped broken before | +// +// Each is a one-line edit in a package this gate's author does not own; lowering a pin is what +// landing one looks like. Shrink it with +// `bun run scripts/test-fix-citations.ts --unpin [,]`, which lowers a count to what is +// measured and refuses to raise one. Raising a count is a hand edit, in a review. + +/** Where the table lives, so a stale-pin finding can name the file to edit. */ +export const PINS_FILE = 'scripts/lib/test-fix-pins.ts'; + +export const TEST_FIX_PINS: Readonly> = { + ai: 1, + core: 1, + flags: 1, + http: 5, +}; + +/** What this package is allowed to have today. Absent means zero, deliberately. */ +export const testFixPinnedFor = ( + pkg: string, + pins: Readonly> = TEST_FIX_PINS, +): number => pins[pkg] ?? 0; + +/** + * The edit `X_TEST_FIX_PIN_STALE` names, performed: lower each named package's pin to what is + * measured, and refuse to raise one. Returns the entries it changed, so the caller can say + * "nothing to lower" rather than reporting a write it did not make. + */ +export async function applyTestFixUnpin( + root: string, + packages: readonly string[], + gaps: readonly { readonly kind: string; readonly pkg: string; readonly found: number }[], +): Promise { + const path = `${root}/${PINS_FILE}`; + let text = await Bun.file(path).text(); + const written: string[] = []; + for (const pkg of packages) { + const gap = gaps.find((one) => one.pkg === pkg && one.kind === 'stale'); + if (gap === undefined) continue; + const entry = new RegExp(`^(\\s*)${pkg}: \\d+,$`, 'm'); + if (!entry.test(text)) continue; + text = + gap.found === 0 + ? text.replace(new RegExp(`^\\s*${pkg}: \\d+,\\n`, 'm'), '') + : text.replace(entry, `$1${pkg}: ${String(gap.found)},`); + written.push(`${pkg} -> ${String(gap.found)}`); + } + if (written.length > 0) await Bun.write(path, text); + return written; +} diff --git a/scripts/release-facts.test.ts b/scripts/release-facts.test.ts new file mode 100644 index 000000000..143dd2aa4 --- /dev/null +++ b/scripts/release-facts.test.ts @@ -0,0 +1,70 @@ +// The corpus states one fact five ways and means two different sets by it, so the cases that +// matter here are the ones where a number is legitimately ambiguous. + +import { describe, expect, test } from 'bun:test'; +import { checkReleaseFacts, releaseFacts, skipFactPath } from './release-facts'; + +const FACTS = releaseFacts([ + ...Array.from({ length: 29 }, (_, i) => `@ultimat3/p${String(i)}`), + 'create-ultimate', +]); + +const gaps = (text: string, path = 'p.md'): readonly string[] => + checkReleaseFacts({ pages: [{ path, text }], facts: FACTS }) + .filter((gap) => gap.kind === 'stale') + .map((gap) => gap.quote); + +describe('the counts come from the workspace list', () => { + test('29 scoped and 30 in all, derived and not written down', () => { + expect(FACTS[0]?.accepts).toEqual([29]); + expect(FACTS[1]?.accepts).toEqual([30]); + expect(FACTS[2]?.accepts).toEqual([29, 30]); + }); +}); + +describe('exact phrasings', () => { + test('a scoped count that is not the scoped count is stale', () => { + expect(gaps('a monorepo of 27 `@ultimat3/*` packages')).toEqual(['27 `@ultimat3/*` packages']); + expect(gaps('a monorepo of 29 `@ultimat3/*` packages')).toEqual([]); + }); + + test('the total, however the page spells it', () => { + expect(gaps('30 in all — packages')).toEqual([]); + expect(gaps('all 30 workspaces resolve')).toEqual([]); + expect(gaps('28 tarballs')).toEqual(['28 tarballs']); + }); +}); + +describe('the ambiguous phrasings', () => { + test('"all N packages" is correct for EITHER set, so both pass and neither hides a stale one', () => { + // wiki/Upgrading.md means the total; wiki/FAQ.md means the scoped set. Both are right. + expect(gaps('one release bumps all 30 packages')).toEqual([]); + expect(gaps('All 29 packages, implemented and tested')).toEqual([]); + expect(gaps('All 28 publish to npm')).toEqual(['All 28 publish']); + }); + + test('"N in all" about something that is not a package is left alone', () => { + // wiki/The-Eight-Primitives.md counts the FILES in a generated slice this way. + expect(gaps('an entity, a policy, a route — 25 in all')).toEqual([]); + }); +}); + +describe('history is not a stale claim', () => { + test('a count attached to an older release is left alone', () => { + expect(gaps('1.0.0 shipped the 28 packages, the docs and the three build targets')).toEqual([]); + expect(gaps('up from 27 `@ultimat3/*` packages')).toEqual([]); + }); + + test('the changelog and the plan record are not read at all', () => { + expect(skipFactPath('CHANGELOG.md')).toBe(true); + expect(skipFactPath('docs/plans/2026/08/x.md')).toBe(true); + expect(skipFactPath('README.md')).toBe(false); + }); +}); + +describe('the false green', () => { + test('a corpus stating no count is a finding, not agreement', () => { + const found = checkReleaseFacts({ pages: [{ path: 'p.md', text: 'no counts' }], facts: FACTS }); + expect(found.map((gap) => gap.kind)).toEqual(['vacuous']); + }); +}); diff --git a/scripts/release-facts.ts b/scripts/release-facts.ts new file mode 100644 index 000000000..cd910b600 --- /dev/null +++ b/scripts/release-facts.ts @@ -0,0 +1,204 @@ +#!/usr/bin/env bun +// Enforce, as a gate step, that the two release-availability COUNTS this repo restates across ten +// pages are the counts the tree actually has. +// +// The gap this closes: how many packages ship is written in `CLAUDE.md`, `README.md`, +// `AGENTS.md`, `SECURITY.md`, `PUBLISHING.md`, five wiki pages and three under `docs/idea` — +// hand-copied every time, and #209 found `SECURITY.md` claiming 28 packages and a supported line +// of `1.0.x`, two majors stale. Measured while writing this: `AGENTS.md` says 27 and +// `docs/architecture/README.md` says 28, against 29 on disk. +// +// TWO NUMBERS, deliberately. `listWorkspaces()` is on disk and needs no network, so the scoped +// count and the total are derivable here; whether a version is ON THE REGISTRY, who published it +// and whether a tarball carries an attestation are not — `scripts/version-stamps.ts` draws the same +// line for the same reason, and a gate that shelled out to `npm view` would be a gate that fails on +// a plane. The commands beside those claims in `CLAUDE.md` stay the way to check them. +// +// The version itself is NOT checked here: `scripts/version-stamps.ts` already owns one stamp on one +// page, and two reporters of one condition is the duplication this repo forbids by name. +// +// bun run scripts/release-facts.ts [--json] + +import { parseScriptArgs } from './lib/args'; +import type { MarkdownFile } from './lib/doc-citations'; +import { readMarkdown } from './lib/doc-citations'; +import type { Finding } from './lib/log'; +import { report } from './lib/log'; +import { repoRoot } from './lib/run'; +import { listWorkspaces } from './lib/workspaces'; + +const SCRIPT = 'release-facts'; +export const SCOPE = '@ultimat3/'; + +export const FACT_GLOBS: readonly string[] = ['*.md', 'wiki/**/*.md', 'docs/**/*.md']; + +/** + * `CHANGELOG.md` names every past release's count by design, and `docs/plans/` is a dated record of + * work. Both are history: a rule that read them would demand that the past be rewritten. + */ +export const skipFactPath = (path: string): boolean => + path.startsWith('docs/plans/') || path === 'CHANGELOG.md'; + +/** A line that pins an OLDER release names its own count — "1.0.0 shipped the 28 packages". */ +const HISTORICAL = /\b\d+\.\d+\.\d+\s+(?:shipped|published|carried|went out|had)\b|\bup from\b/; + +export interface CountFact { + /** How a finding names it. */ + readonly label: string; + /** Every count this phrasing may legitimately state. */ + readonly accepts: readonly number[]; + /** What a page writes when it states this count. */ + readonly patterns: readonly RegExp[]; + /** A word the line must also carry, when the phrasing alone is not about packages. */ + readonly requires?: RegExp; +} + +/** + * Derived from the workspace list on disk, never restated. + * + * Two of the three are EXACT and one is membership, and the split is the corpus talking. A page + * writing `` 29 `@ultimat3/*` packages `` has named which set it means; a page writing "all 30 + * packages" has not — `wiki/Upgrading.md` uses it for the total and `wiki/FAQ.md` uses the same + * words for the scoped set, and both sentences are correct English. Membership still catches the + * failure that matters: 27 and 28 are neither, which is what `AGENTS.md` says today. + */ +export function releaseFacts(names: readonly string[]): readonly CountFact[] { + const scoped = names.filter((name) => name.startsWith(SCOPE)).length; + const total = names.length; + return [ + { + label: `\`${SCOPE}*\` packages`, + accepts: [scoped], + patterns: [/(\d+)\s+`?@ultimat3\/\*`?\s+packages/g], + }, + { + label: 'workspaces in all', + accepts: [total], + patterns: [ + /(\d+)\s+in all\b/g, + /[Aa]ll\s+(\d+)\s+workspaces\b/g, + /(\d+)\s+workspaces\s+(?:publish|resolve|move)/g, + /(\d+)\s+tarballs\b/g, + ], + // `wiki/The-Eight-Primitives.md` writes "25 in all" about the FILES in a generated slice. + requires: /packages?\b|workspaces?\b|tarballs?\b/, + }, + { + label: 'packages (scoped or in all)', + accepts: [scoped, total], + patterns: [/[Aa]ll\s+(\d+)\s+(?:packages|publish)\b/g, /(\d+)\s+packages,\s*\d+\s+tiers/g], + }, + ]; +} + +export interface FactGap { + readonly kind: 'stale' | 'vacuous'; + readonly at: string; + readonly label: string; + readonly claimed: number; + readonly expected: readonly number[]; + readonly quote: string; +} + +export interface FactInput { + readonly pages: readonly MarkdownFile[]; + readonly facts: readonly CountFact[]; +} + +export function checkReleaseFacts(input: FactInput): readonly FactGap[] { + const gaps: FactGap[] = []; + let claims = 0; + for (const page of input.pages) { + const lines = page.text.split('\n'); + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ''; + if (HISTORICAL.test(line)) continue; + for (const fact of input.facts) { + if (fact.requires !== undefined && !fact.requires.test(line)) continue; + for (const pattern of fact.patterns) { + for (const match of line.matchAll(pattern)) { + claims += 1; + const claimed = Number.parseInt(match[1] ?? '0', 10); + if (fact.accepts.includes(claimed)) continue; + gaps.push({ + kind: 'stale', + at: `${page.path}:${index + 1}`, + label: fact.label, + claimed, + expected: fact.accepts, + quote: (match[0] ?? '').trim(), + }); + } + } + } + } + } + if (claims === 0) { + gaps.push({ + kind: 'vacuous', + at: FACT_GLOBS[0] ?? '', + label: '', + claimed: 0, + expected: [], + quote: '', + }); + } + return gaps; +} + +const staleFinding = (gap: FactGap): Finding => ({ + code: 'X_DOC_RELEASE_FACT_STALE', + cause: `${gap.at} writes "${gap.quote}" and this tree has ${gap.expected.join(' or ')} ${gap.label}`, + fix: `set the count at ${gap.at} to ${gap.expected.join(' or ')}; \`bun run scripts/list-workspaces.ts\` is where the number comes from`, + at: gap.at, +}); + +const vacuousFinding = (): Finding => ({ + code: 'X_DOC_RELEASE_FACT_UNSCANNED', + cause: + 'no page states a package count, so this rule read nothing and reported green over ten pages that restate one', + // The path is a literal, not `scripts/${SCRIPT}.ts`: the fix-line contract reads these + // statically and an interpolation renders as `scripts/.ts`, which names no file. + fix: 'edit FACT_GLOBS in scripts/release-facts.ts so it matches how the pages state a count', + at: `scripts/${SCRIPT}.ts`, +}); + +export const releaseFactFindingFor = (gap: FactGap): Finding => + gap.kind === 'vacuous' ? vacuousFinding() : staleFinding(gap); + +export const readFactPages = async (root: string): Promise => { + const seen = new Map(); + for (const glob of FACT_GLOBS) { + for (const file of await readMarkdown(root, glob, skipFactPath)) seen.set(file.path, file); + } + return [...seen.values()].sort((a, b) => (a.path < b.path ? -1 : 1)); +}; + +export const releaseFactGaps = async (root: string): Promise => + checkReleaseFacts({ + pages: await readFactPages(root), + facts: releaseFacts((await listWorkspaces(root)).map((workspace) => workspace.name)), + }); + +/** What this repo contributes to `x verify`'s `manifest` step. */ +export const releaseFactFindings = async (root: string): Promise => + (await releaseFactGaps(root)).map(releaseFactFindingFor); + +if (import.meta.main) { + const args = parseScriptArgs(Bun.argv.slice(2)); + const root = repoRoot(); + const gaps = await releaseFactGaps(root); + const facts = releaseFacts((await listWorkspaces(root)).map((workspace) => workspace.name)); + report( + { + ok: gaps.length === 0, + script: SCRIPT, + summary: + gaps.length === 0 + ? `every stated count matches the tree (${facts.map((f) => `${f.accepts.join('/')} ${f.label}`).join(', ')})` + : `${gaps.length} stated package count(s) no longer match the tree`, + findings: gaps.map(releaseFactFindingFor), + }, + args.json, + ); +} diff --git a/scripts/release.test.ts b/scripts/release.test.ts index 4f49eafd1..aa3d41234 100644 --- a/scripts/release.test.ts +++ b/scripts/release.test.ts @@ -8,9 +8,11 @@ import { changelogEntry, insertRelease, nextVersion, + RELEASE_FLAGS, readReleaseVersion, repinFrameworkDeps, setOwnVersion, + unknownReleaseFlags, } from './release'; const MANIFEST = `{ @@ -150,9 +152,34 @@ describe('unit · the version to publish is decided, never guessed', () => { version: nextVersion('1.2.0', bump), }); } - expect(readReleaseVersion({ explicit: undefined, bump: undefined, current: '1.2.0' })).toEqual({ - version: '1.2.1', - }); + }); + + // This assertion used to read `toEqual({ version: '1.2.1' })` — it pinned the implicit patch + // default as if it were a feature. It is the reason `bun run scripts/release.ts --help` rewrote + // 47 manifests and the helm chart: no `--help` exists, the parser shrugged, and "no explicit, no + // bump" meant "release a patch". A release is the most expensive thing here to undo. + test('neither --version nor --bump is a refusal, never a patch bump', () => { + const result = readReleaseVersion({ explicit: undefined, bump: undefined, current: '1.2.0' }); + expect('findings' in result).toBe(true); + const findings = 'findings' in result ? result.findings : []; + expect(findings[0]?.code).toBe('X_RELEASE_VERSION_UNSTATED'); + expect(findings[0]?.cause).toContain('1.2.0'); + expect(findings[0]?.fix).toStartWith('bun run scripts/release.ts'); + }); + + test('a flag this script does not declare is refused before anything is decided', () => { + expect(unknownReleaseFlags(['help'])).toEqual(['help']); + expect(unknownReleaseFlags(['bump', 'json', 'dry-run'])).toEqual([]); + // Sorted, so the refusal reads the same however the shell ordered them. + expect(unknownReleaseFlags(['zebra', 'check', 'apple'])).toEqual(['apple', 'zebra']); + }); + + // Through the function, not against the constant: comparing a `string[]` to the readonly literal + // tuple narrows `toContain`'s parameter to the union and stops compiling, and asserting the + // constant against itself would prove nothing anyway. Every invocation PUBLISHING.md prints. + test('every flag the docs tell a reader to pass is accepted', () => { + expect(unknownReleaseFlags(['version', 'bump', 'check', 'dry-run', 'json'])).toEqual([]); + expect(RELEASE_FLAGS.length).toBe(5); }); test('the fix line is a command a shell can run verbatim', () => { diff --git a/scripts/release.ts b/scripts/release.ts index 395836693..acf00a094 100644 --- a/scripts/release.ts +++ b/scripts/release.ts @@ -34,7 +34,25 @@ const badFlagFinding = (flag: string, value: string, expected: string): Finding at: 'scripts/release.ts', }); -/** Both flags, refused before a single manifest is rewritten. */ +/** + * The flags this script accepts. Anything else is refused before a manifest is touched — see + * `readReleaseVersion` for why an unrecognised token must never reach the rewrite. + */ +export const RELEASE_FLAGS = ['version', 'bump', 'check', 'dry-run', 'json'] as const; + +/** Every `--flag` the caller passed that this script does not declare. */ +export const unknownReleaseFlags = (flags: Iterable): readonly string[] => + [...flags].filter((name) => !(RELEASE_FLAGS as readonly string[]).includes(name)).sort(); + +/** + * Both flags, refused before a single manifest is rewritten. + * + * The absent case is a REFUSAL, not a patch bump. It defaulted to `patch`, so any invocation the + * parser did not recognise — `--help` most obviously, since this script has never had one — fell + * through to "no explicit, no bump" and rewrote 47 manifests plus the chart. A release is the most + * expensive thing in this repo to undo, and it was the one script that acted on a typo. The + * version to publish is always somebody's decision, so it is always stated. + */ export function readReleaseVersion(input: { readonly explicit: string | undefined; readonly bump: string | undefined; @@ -47,6 +65,14 @@ export function readReleaseVersion(input: { if (input.explicit !== undefined && !SEMVER.test(input.explicit)) { findings.push(badFlagFinding('version', input.explicit, 'a semver version (e.g. 1.3.0)')); } + if (input.explicit === undefined && input.bump === undefined) { + findings.push({ + code: 'X_RELEASE_VERSION_UNSTATED', + cause: `neither --version nor --bump was given, and this repo is at ${input.current}`, + fix: `bun run scripts/release.ts --bump patch --dry-run --json # or --version ${input.current}`, + at: 'scripts/release.ts', + }); + } if (findings.length > 0) return { findings }; return { version: input.explicit ?? nextVersion(input.current, (input.bump ?? 'patch') as Bump), @@ -159,6 +185,27 @@ if (import.meta.main) { ); } + // Before anything is decided, let alone written: a flag this script does not declare is a + // mistake, and the mistake this guard exists for wrote 47 manifests. + const unknown = unknownReleaseFlags(args.flags.keys()); + if (unknown.length > 0) { + report( + { + ok: false, + script: 'release', + summary: `refusing to release: ${unknown.length} unknown flag(s)`, + findings: unknown.map((name) => ({ + code: 'X_RELEASE_FLAG_UNKNOWN', + cause: `--${name} is not a flag scripts/release.ts declares (known: ${RELEASE_FLAGS.join(', ')})`, + fix: 'bun run scripts/release.ts --bump patch --dry-run --json', + at: 'scripts/release.ts', + })), + data: { unknown, current }, + }, + args.json, + ); + } + const resolved = readReleaseVersion({ explicit: flagString(args, 'version'), bump: flagString(args, 'bump'), diff --git a/scripts/test-fix-citations.test.ts b/scripts/test-fix-citations.test.ts new file mode 100644 index 000000000..ec1af2b87 --- /dev/null +++ b/scripts/test-fix-citations.test.ts @@ -0,0 +1,101 @@ +// The scanner's own cases, as fixtures. What matters here is not that it FINDS a bad command — +// `citedCommandProblem` owns that — but what it agrees to call a citation at all. + +import { describe, expect, test } from 'bun:test'; +import type { CommandCatalog } from '@ultimat3/cli'; +import { sourceStrings } from './lib/source-strings'; +import { checkTestFixes, packageOf, scanTestFixes, type TestFixGap } from './test-fix-citations'; + +/** One command, one subcommand — enough for `citedCommandProblem` to answer, and no more. */ +const CATALOG: CommandCatalog = { + specs: [ + { + name: 'db', + summary: 'db', + usage: 'x db migrate', + flags: [], + positionalChoices: ['migrate'], + }, + ], + planned: new Set(), + plannedSubcommands: new Set(), +} as unknown as CommandCatalog; + +const found = (source: string): readonly string[] => + scanTestFixes('packages/p/src/a.test.ts', source).map((c) => c.fix); + +describe('what the scanner agrees to call a citation', () => { + test('a fix: property and a fix assertion both count', () => { + expect(found("const e = { fix: 'x db migrate' };")).toEqual(['x db migrate']); + expect(found("expect(err.fix).toBe('x db migrate');")).toEqual(['x db migrate']); + expect(found("expect(e.fix).toContain('x db migrate');")).toEqual(['x db migrate']); + }); + + test('a command in a COMMENT is prose about a command, never a use of one', () => { + expect(found("// the fix: 'x db nope' is not a real command\nconst a = 1;")).toEqual([]); + expect(found("/* fix: 'x db nope' */\nconst a = 1;")).toEqual([]); + }); + + test('a command inside another string is a fixture’s source text, not this file’s value', () => { + // packages/cli/src/error-contract.test.ts does exactly this, seven times, to prove the rule + // that reads `src/` bites. The exemption is a property of the code, not a filename allowlist. + expect(found(`await write('a.ts', "throw new E({ fix: 'x db nope' });");`)).toEqual([]); + }); + + test('a NEGATED assertion is the rule being enforced, not broken', () => { + expect(found("expect(err.fix).not.toContain('x db nope');")).toEqual([]); + expect(found("expect(err.fix).not.toBe('x db nope');")).toEqual([]); + }); + + test('a string that is not a command is left alone', () => { + expect(found("const e = { fix: 'edit packages/db/src/pool.ts' };")).toEqual([]); + }); +}); + +describe('the tokenizer the exemption rests on', () => { + test('a nested literal is returned whole and never re-read as code', () => { + const strings = sourceStrings(`write('a.ts', "fix: 'x db nope'");`); + expect(strings.map((s) => s.value)).toEqual(['a.ts', "fix: 'x db nope'"]); + }); + + test('an escaped quote does not end the literal', () => { + expect(sourceStrings(`const a = 'it\\'s fine';`).map((s) => s.value)).toEqual(["it's fine"]); + }); + + test('line numbers survive a multi-line block comment', () => { + expect(sourceStrings("/* a\nb */\nconst x = 'y';")[0]?.line).toBe(3); + }); +}); + +describe('the ratchet', () => { + const gaps = (text: string, pins: Record): readonly TestFixGap[] => + checkTestFixes({ + files: [{ path: 'packages/db/src/a.test.ts', text }], + catalog: CATALOG, + pins, + }); + + test('a package over its pin is the hazard', () => { + const [gap] = gaps("const e = { fix: 'x db nope' };", {}); + expect(gap?.kind).toBe('over'); + expect(gap?.pkg).toBe('db'); + expect(gap?.first?.at).toBe('packages/db/src/a.test.ts:1'); + }); + + test('a package at its pin is silent, and one under it is stale', () => { + expect(gaps("const e = { fix: 'x db nope' };", { db: 1 })).toEqual([]); + expect(gaps('const a = 1;', { db: 1 })[0]?.kind).toBe('stale'); + }); + + test('a file set that matched nothing is a finding, not agreement', () => { + const [gap] = checkTestFixes({ files: [], catalog: CATALOG, pins: {} }); + expect(gap?.kind).toBe('unscanned'); + }); +}); + +describe('packageOf', () => { + test('names the package a pin is keyed by', () => { + expect(packageOf('packages/db/src/a.test.ts')).toBe('db'); + expect(packageOf('scripts/verify.test.ts')).toBe('scripts'); + }); +}); diff --git a/scripts/test-fix-citations.ts b/scripts/test-fix-citations.ts new file mode 100644 index 000000000..c49748eb4 --- /dev/null +++ b/scripts/test-fix-citations.ts @@ -0,0 +1,214 @@ +#!/usr/bin/env bun +// Enforce, as a gate step, that a `fix:` a TEST writes or asserts is a command this build can run. +// The same question `x verify`'s `errors` step asks of shipped source and `scripts/doc-fixes.ts` +// asks of the error reference, asked of test sources for the first time. +// +// The gap this closes: `checkErrorFixes` reads `src/` and skips tests, so a fixture error, a +// helper that builds one, and an assertion pinning a fix string were all unchecked — and the repo +// has shipped a `fix:` naming a command that does not exist more than once (`x schema show`, +// `x logs tail`). A test is where the next one is written down and copied from. +// +// A citation counts only when the code EVALUATES it (`scripts/lib/source-strings.ts`): a command +// inside a comment is prose about a command, and one inside another string is a fixture's source +// text — which is what `packages/cli/src/error-contract.test.ts` writes to disk on purpose, seven +// times, to prove this very rule bites. That is the exemption, and it is a property of the code +// rather than a list of filenames nobody re-reads. +// +// bun run scripts/test-fix-citations.ts [--json] +// bun run scripts/test-fix-citations.ts --unpin [,] # shrink the ratchet + +import { type CommandCatalog, citedCommandProblem, loadCommandCatalog } from '@ultimat3/cli'; +import { flagList, parseScriptArgs } from './lib/args'; +import type { Finding } from './lib/log'; +import { report } from './lib/log'; +import { repoRoot } from './lib/run'; +import { sourceStrings } from './lib/source-strings'; +import { PINS_FILE, TEST_FIX_PINS, testFixPinnedFor } from './lib/test-fix-pins'; + +const SCRIPT = 'test-fix-citations'; + +export const TEST_GLOBS: readonly string[] = [ + 'packages/*/src/**/*.test.ts', + 'packages/*/src/**/*.test.tsx', + 'scripts/**/*.test.ts', +]; + +/** The literal is a `fix:` property value. */ +const PROPERTY = /(?:^|[^\w$.])fix\s*:\s*$/; +/** + * The literal is what a `fix` assertion compares against — and NOT what a negated one refuses. + * `expect(fix).not.toContain('x logs')` is a test enforcing this very rule, three times in this + * repo; reading it as a citation reports a finding on the assertion that prevents the finding. + */ +const ASSERTION = /\.fix\b(?:(?!\.not\b)[\s\S])*\.(?:toBe|toContain|toEqual|toMatch)\(\s*$/; + +export interface TestFixCitation { + readonly path: string; + readonly line: number; + readonly fix: string; +} + +/** + * Every `fix:` a test file evaluates, whether it declares one or asserts one. Pure over the text, + * so the negative cases are fixtures rather than edits to tests other people are rewriting. + */ +export function scanTestFixes(path: string, source: string): readonly TestFixCitation[] { + const out: TestFixCitation[] = []; + for (const literal of sourceStrings(source)) { + if (!literal.value.startsWith('x ')) continue; + if (!PROPERTY.test(literal.prefix) && !ASSERTION.test(literal.prefix)) continue; + out.push({ path, line: literal.line, fix: literal.value }); + } + return out; +} + +/** + * `over` is the hazard: a package citing more unrunnable commands than it is pinned at, which is + * also how a package pinned at 0 reports its first. `stale` is the ratchet's own hygiene — a pin + * nothing needs any more. `unscanned` is the false green: a glob matching no test file would + * otherwise read as "every asserted fix runs". + */ +export type TestFixGapKind = 'over' | 'stale' | 'unscanned'; + +export interface TestFixGap { + readonly kind: TestFixGapKind; + readonly pkg: string; + readonly found: number; + readonly pinned: number; + readonly first?: { readonly at: string; readonly fix: string; readonly problem: string }; +} + +export interface TestFixInput { + readonly files: readonly { readonly path: string; readonly text: string }[]; + readonly catalog: CommandCatalog; + readonly pins: Readonly>; +} + +/** `packages/db/src/x.test.ts` -> `db`; anything else -> its first path segment. */ +export const packageOf = (path: string): string => + path.startsWith('packages/') ? (path.split('/')[1] ?? path) : (path.split('/')[0] ?? path); + +export function checkTestFixes(input: TestFixInput): readonly TestFixGap[] { + if (input.files.length === 0) { + return [{ kind: 'unscanned', pkg: '', found: 0, pinned: 0 }]; + } + const found = new Map(); + for (const file of input.files) { + for (const citation of scanTestFixes(file.path, file.text)) { + const problem = citedCommandProblem(citation.fix, input.catalog); + if (problem === undefined) continue; + const pkg = packageOf(citation.path); + const list = found.get(pkg) ?? []; + list.push({ at: `${citation.path}:${String(citation.line)}`, fix: citation.fix, problem }); + found.set(pkg, list); + } + } + const gaps: TestFixGap[] = []; + for (const pkg of new Set([...found.keys(), ...Object.keys(input.pins)])) { + const hits = found.get(pkg) ?? []; + const pinned = testFixPinnedFor(pkg, input.pins); + if (hits.length > pinned) { + gaps.push({ + kind: 'over', + pkg, + found: hits.length, + pinned, + ...(hits[0] === undefined ? {} : { first: hits[0] }), + }); + continue; + } + if (hits.length < pinned) gaps.push({ kind: 'stale', pkg, found: hits.length, pinned }); + } + return gaps.sort((a, b) => (a.pkg < b.pkg ? -1 : a.pkg > b.pkg ? 1 : 0)); +} + +const overFinding = (gap: TestFixGap): Finding => ({ + code: 'X_TEST_FIX_UNRUNNABLE', + cause: `${gap.pkg} has ${String(gap.found)} test fix line(s) citing a command this build cannot run and is pinned at ${String(gap.pinned)} — ${gap.first?.at ?? ''} writes "${gap.first?.fix ?? ''}", which ${gap.first?.problem ?? ''}`, + fix: `rewrite the fix at ${gap.first?.at ?? gap.pkg} as an invocation this build ships; \`x help --json\` lists every command, subcommand and flag`, + at: gap.first?.at ?? gap.pkg, +}); + +const staleFinding = (gap: TestFixGap): Finding => ({ + code: 'X_TEST_FIX_PIN_STALE', + cause: `${gap.pkg} is pinned at ${String(gap.pinned)} unrunnable test fix line(s) and now has ${String(gap.found)} — the ratchet may only fall, so a pin nobody lowers is one nobody reads`, + fix: `bun run scripts/${SCRIPT}.ts --unpin ${gap.pkg}`, + at: PINS_FILE, +}); + +const unscannedFinding = (): Finding => ({ + code: 'X_TEST_FIX_UNSCANNED', + cause: 'no test file matched, so this rule reported green over a file set it never read', + // Literal path for the same reason release-facts.ts records: an interpolated `${SCRIPT}` + // renders as `scripts/.ts` to the static fix-line contract, which then names no file. + fix: "edit TEST_GLOBS in scripts/test-fix-citations.ts so it matches this repo's test layout", + at: `scripts/${SCRIPT}.ts`, +}); + +const FINDINGS: Readonly Finding>> = { + over: overFinding, + stale: staleFinding, + unscanned: unscannedFinding, +}; + +export const testFixFindingFor = (gap: TestFixGap): Finding => FINDINGS[gap.kind](gap); + +export async function readTestSources( + root: string, +): Promise { + const seen = new Map(); + for (const glob of TEST_GLOBS) { + for await (const path of new Bun.Glob(glob).scan({ cwd: root, absolute: false })) { + if (seen.has(path)) continue; + seen.set(path, { path, text: await Bun.file(`${root}/${path}`).text() }); + } + } + return [...seen.values()].sort((a, b) => (a.path < b.path ? -1 : 1)); +} + +export const testFixGaps = async (root: string): Promise => + checkTestFixes({ + files: await readTestSources(root), + catalog: await loadCommandCatalog(), + pins: TEST_FIX_PINS, + }); + +/** What this repo contributes to `x verify`'s `errors` step. */ +export const testFixFindings = async (root: string): Promise => + (await testFixGaps(root)).map(testFixFindingFor); + +if (import.meta.main) { + const args = parseScriptArgs(Bun.argv.slice(2)); + const root = repoRoot(); + const gaps = await testFixGaps(root); + const unpin = flagList(args, 'unpin'); + if (unpin.length > 0) { + const { applyTestFixUnpin } = await import('./lib/test-fix-pins'); + const written = await applyTestFixUnpin(root, unpin, gaps); + report( + { + ok: written.length > 0, + script: SCRIPT, + summary: + written.length > 0 + ? `lowered ${written.join(', ')} in ${PINS_FILE}` + : `nothing to lower: ${unpin.join(', ')} is already at what is measured`, + findings: [], + }, + args.json, + ); + process.exit(written.length > 0 ? 0 : 1); + } + report( + { + ok: gaps.length === 0, + script: SCRIPT, + summary: + gaps.length === 0 + ? `every fix: a test evaluates cites a command this build can run` + : `${gaps.length} package(s) off the unrunnable-test-fix ratchet`, + findings: gaps.map(testFixFindingFor), + }, + args.json, + ); +} diff --git a/scripts/to-throw-returns.test.ts b/scripts/to-throw-returns.test.ts new file mode 100644 index 000000000..f9bbe01f2 --- /dev/null +++ b/scripts/to-throw-returns.test.ts @@ -0,0 +1,103 @@ +// The premise is executed here, not asserted from memory: if bun ever fixes `toThrow` to fail on a +// returned Error, the first test below goes red and this whole rule can be deleted. + +import { describe, expect, test } from 'bun:test'; +import { checkToThrowReturns, errorFactoriesIn, type ThrowGap } from './to-throw-returns'; + +class Boom extends Error {} + +/** + * The premise, as callables rather than as inline arrows — and the indirection is the honest + * statement of this rule's reach, not an evasion of it. The scanner reads the INLINE spelling, + * which is the only one text can be certain about; `expect(returnsAnError).toThrow()` is exactly + * as broken and needs type information to see. Writing the premise inline would also make this + * file fail its own gate, which is the correct outcome for the rule and a useless one for the test. + */ +const returnsABoom = (): never => new Boom('x') as unknown as never; +const returnsAnError = (): never => new Error('boom') as unknown as never; +const returnsANumber = (): never => 42 as unknown as never; + +describe('the hazard this rule exists for', () => { + test('sync toThrow PASSES when the callback returns an Error — all three matcher forms', () => { + // Not a demonstration: these three ARE the defect. A returned error satisfies the class + // matcher, the bare form and the message form alike. If bun ever fixes this, these go red and + // the whole rule can be deleted. + expect(returnsABoom).toThrow(Boom); + expect(returnsAnError).toThrow(); + expect(returnsAnError).toThrow('boom'); + }); + + test('a returned NON-error is correctly reported, which is why the rule reads the value', () => { + expect(() => { + expect(returnsANumber).toThrow(); + }).toThrow(); + }); + + test('rejects.toThrow is NOT vulnerable — a resolved promise is caught', async () => { + const asserted = async (): Promise => { + await expect(Promise.resolve(new Boom('x'))).rejects.toThrow(Boom); + }; + await expect(asserted()).rejects.toThrow(); + }); +}); + +const gaps = (text: string, factories: readonly string[] = []): readonly ThrowGap[] => + checkToThrowReturns({ + tests: [{ path: 'packages/p/src/a.test.ts', text }], + factories: new Set(factories), + }); + +describe('what it reports', () => { + test('an arrow whose body constructs an error', () => { + const [gap] = gaps("expect(() => new MailError('x')).toThrow(MailError);"); + expect(gap?.at).toBe('packages/p/src/a.test.ts:1'); + expect(gap?.reason).toContain('constructs an error'); + }); + + test('an arrow that calls a function declared to return one', () => { + const [gap] = gaps('expect(() => sendFailed({ a: 1 })).toThrow();', ['sendFailed']); + expect(gap?.reason).toContain('sendFailed()'); + }); + + test('the reach is the inline arrow only, and the gap is stated rather than assumed away', () => { + // `expect(returnsAnError).toThrow()` is the same defect and needs type information to see. + expect(gaps('expect(returnsAnError).toThrow();', ['returnsAnError'])).toEqual([]); + }); +}); + +describe('what it leaves alone', () => { + test('a callback that calls something this repo does not declare as returning an error', () => { + expect(gaps('expect(() => parseThing(raw)).toThrow();')).toEqual([]); + }); + + test('a callback with a statement body — it throws or it does not, and text cannot tell', () => { + expect(gaps('expect(() => { throw new MailError("x"); }).toThrow();')).toEqual([]); + }); + + test('rejects.toThrow, which the premise test above proves is safe', () => { + expect(gaps('expect(() => sendFailed(a)).rejects.toThrow();', ['sendFailed'])).toEqual([]); + }); +}); + +describe('the factory set is derived from source, never listed', () => { + test('reads a declared error return type in either function form', () => { + const source = [ + 'export function sendFailed(input: X): MailError {', + 'export const routeNotFound = (m: string): HttpError =>', + 'export function plainThing(a: number): string {', + ].join('\n'); + expect([...errorFactoriesIn(source)].sort()).toEqual(['routeNotFound', 'sendFailed']); + }); +}); + +describe('the rule does not read its own fixtures', () => { + test('a bad shape written as a STRING is a fixture, not an assertion this file makes', () => { + // Every `gaps("expect(() => new MailError(…))…")` above is one of these. Without the + // exemption this file is its own first finding, which is a rule reporting on the test that + // proves it works. + const text = `const fixture = "expect(() => new MailError('x')).toThrow(MailError);";`; + expect( + checkToThrowReturns({ tests: [{ path: 'a.test.ts', text }], factories: new Set() }), + ).toEqual([]); + }); +}); diff --git a/scripts/to-throw-returns.ts b/scripts/to-throw-returns.ts new file mode 100644 index 000000000..61b25400b --- /dev/null +++ b/scripts/to-throw-returns.ts @@ -0,0 +1,134 @@ +#!/usr/bin/env bun +// Enforce, as a gate step, that `expect(fn).toThrow(...)` is given a function that THROWS. +// +// The hazard, verified rather than assumed (`to-throw-returns.test.ts` runs the case): bun's +// synchronous `toThrow` PASSES when `fn` returns an Error instead of throwing one — +// `expect(() => new Boom('x')).toThrow(Boom)` is green, and so are the bare and string-matcher +// forms. Returning a NON-error is correctly reported, and `rejects.toThrow` is unaffected: a +// promise that resolves to an Error is a resolved promise, and `rejects` catches that. +// +// So the defect is narrow and total: a sync `toThrow` whose callback's value IS an error. This +// repo has 196 exported functions that RETURN one — `sendFailed()`, `routeNotFound()`, +// `fixtureUnknown()` — beside a matching set that throws, and one `expect(() => sendFailed(…))` +// is a test that can never fail. Zero today; the surface is 196 wide and nothing guarded it. +// +// Only the shapes that are CERTAIN are reported: the callback's body is an error construction, or +// a call to a function this repo declares as returning an error. A callback that calls something +// else may or may not throw, and a rule that guessed would report findings on tests that are right. +// +// bun run scripts/to-throw-returns.ts [--json] + +import { parseScriptArgs } from './lib/args'; +import type { Finding } from './lib/log'; +import { report } from './lib/log'; +import { repoRoot } from './lib/run'; +import { insideString, sourceStrings } from './lib/source-strings'; +import { readTestSources } from './test-fix-citations'; + +const SCRIPT = 'to-throw-returns'; + +export const SOURCE_GLOBS: readonly string[] = ['packages/*/src/**/*.ts', 'scripts/**/*.ts']; + +/** + * A function this repo declares as RETURNING an error — `export function sendFailed(…): MailError`. + * Derived, never listed: an error package adds one of these most weeks, and a hand-kept list would + * be the convention this file exists to stop relying on. + */ +const RETURNS_ERROR = [ + /export (?:function|const) (\w+)[^\n=]*?[):]\s*[A-Z]\w*(?:Error|Fault)\b/g, + /export const (\w+)\s*=\s*\([^)]*\):\s*[A-Z]\w*(?:Error|Fault)\b/g, +]; + +export function errorFactoriesIn(source: string): readonly string[] { + return RETURNS_ERROR.flatMap((pattern) => + [...source.matchAll(pattern)].map((match) => match[1] ?? ''), + ).filter((name) => name !== ''); +} + +/** + * `expect(() => ).toThrow…(` — synchronous only. `rejects.toThrow` is excluded because it is + * not vulnerable, and including it would report a finding on a correct assertion. + */ +const SYNC_TO_THROW = /expect\(\s*(?:async\s*)?\(\s*\)\s*=>\s*([^\n]*?)\)\s*\.(toThrow\w*)\(/g; +const CONSTRUCTS_ERROR = /^\s*new\s+[A-Z]\w*(?:Error|Fault)\w*\s*\(/; +const CALLS = /^\s*([A-Za-z_$][\w$]*)\s*\(/; + +export interface ThrowGap { + readonly at: string; + readonly body: string; + /** What makes the value certainly an error: a construction, or a named factory. */ + readonly reason: string; +} + +export interface ThrowGapInput { + readonly tests: readonly { readonly path: string; readonly text: string }[]; + readonly factories: ReadonlySet; +} + +export function checkToThrowReturns(input: ThrowGapInput): readonly ThrowGap[] { + const gaps: ThrowGap[] = []; + for (const file of input.tests) { + // A checker's own unit test spells the bad shape as a STRING and feeds it to the pure function + // below. That is a fixture, not an assertion this file makes — the same exemption + // `test-fix-citations.ts` rests on, and the same tokenizer. + const literals = sourceStrings(file.text); + for (const match of file.text.matchAll(SYNC_TO_THROW)) { + if (insideString(literals, match.index ?? 0)) continue; + const body = (match[1] ?? '').trim(); + const called = CALLS.exec(body)?.[1]; + const reason = CONSTRUCTS_ERROR.test(body) + ? 'constructs an error and hands it back' + : called !== undefined && input.factories.has(called) + ? `calls ${called}(), which this repo declares as returning an error` + : undefined; + if (reason === undefined) continue; + gaps.push({ + at: `${file.path}:${String(file.text.slice(0, match.index).split('\n').length)}`, + body, + reason, + }); + } + } + return gaps; +} + +export const throwFindingFor = (gap: ThrowGap): Finding => ({ + code: 'X_TEST_THROW_NOT_THROWN', + cause: `${gap.at} asserts a throw over \`${gap.body}\`, which ${gap.reason} — bun's synchronous toThrow PASSES on a returned Error, so this assertion cannot fail`, + fix: `wrap the value in a throw at ${gap.at}: \`expect(() => { throw ${gap.body}; }).toThrow(…)\`, or assert the value directly with \`expect(${gap.body}).toBeInstanceOf(…)\``, + at: gap.at, +}); + +export async function throwGaps(root: string): Promise { + const factories = new Set(); + for (const glob of SOURCE_GLOBS) { + for await (const path of new Bun.Glob(glob).scan({ cwd: root, absolute: false })) { + if (/\.test\.tsx?$/.test(path)) continue; + for (const name of errorFactoriesIn(await Bun.file(`${root}/${path}`).text())) { + factories.add(name); + } + } + } + return checkToThrowReturns({ tests: await readTestSources(root), factories }); +} + +/** What this repo contributes to `x verify`'s `errors` step. */ +export const throwFindings = async (root: string): Promise => + (await throwGaps(root)).map(throwFindingFor); + +if (import.meta.main) { + const args = parseScriptArgs(Bun.argv.slice(2)); + const gaps = await throwGaps(repoRoot()); + report( + { + ok: gaps.length === 0, + script: SCRIPT, + summary: + gaps.length === 0 + ? 'every sync toThrow is given a function that throws, not one that returns an error' + : `${gaps.length} toThrow assertion(s) cannot fail`, + findings: gaps.map(throwFindingFor), + }, + args.json, + ); +} diff --git a/scripts/verify.ts b/scripts/verify.ts index eb88a6fb2..d8455ad9d 100644 --- a/scripts/verify.ts +++ b/scripts/verify.ts @@ -38,6 +38,7 @@ import { docCommandFindings } from './doc-commands'; import { docFixFindings } from './doc-fixes'; import { errorStatusCompleteness } from './error-map'; import { errorRendering } from './error-render'; +import { generatorCountFindings } from './generator-counts'; import { frameworkCatalogFindings } from './i18n-catalog'; import { imageContractFindings } from './image-contract'; import { flagBool, parseScriptArgs } from './lib/args'; @@ -45,9 +46,12 @@ import { writeOut } from './lib/log'; import { repoRoot } from './lib/run'; import { DEFAULT_OUT, frameworkManifestDrift } from './manifest'; import { readmeFenceFindings } from './readme-fences'; +import { releaseFactFindings } from './release-facts'; import { publishListFindings } from './release-workflow'; import { checkRoadmap } from './roadmap'; +import { testFixFindings } from './test-fix-citations'; import { testTypecheckFindings } from './test-typecheck-gate'; +import { throwFindings } from './to-throw-returns'; import { versionStampFindings } from './version-stamps'; import { frameDocFindings } from './wiki-frames'; import { wikiTableFindings } from './wiki-tables'; @@ -173,6 +177,13 @@ export const errorCodeDocs: HostCheck = async (root) => { * could print `x db query "select id …"` — and `x db` has no `query`, which is a second failure * handed to a reader already holding one. * + * `testFixFindings` and `throwFindings` join it because both are the same contract one file set + * further on. `checkErrorFixes` holds every `fix:` in `src/` to being runnable and skips tests, so a + * fixture error, a helper that builds one and an assertion pinning a fix string were unchecked — + * and `x schema show` and `x logs tail` are what that costs. `throwFindings` is the other half of + * an error assertion: bun's synchronous `toThrow` PASSES when the callback returns an Error rather + * than throwing one, and this repo exports 196 functions that return one. + * * The completeness rule is deliberately NOT its own step: `VerifyStepName` is a closed union owned * by `@ultimat3/cli`, and a generated app would inherit a step name that only this repo can run. * It blocks `x verify` either way, which is what "enforced, not documented" asks for. @@ -182,6 +193,8 @@ export const errorContract: HostCheck = async (root) => [ ...(await errorRendering(root)), ...(await errorStatusCompleteness(root)), ...(await docFixFindings(root)), + ...(await testFixFindings(root)), + ...(await throwFindings(root)), ]; /** @@ -200,6 +213,8 @@ export const errorContract: HostCheck = async (root) => [ * | `versionStampFindings` | one page stamps a version, it is the shipped one, and the workspaces agree | every workspace manifest | * | `readmeFenceFindings` | a fenced `ts`/`tsx` example in a package README typechecks | `tsc`, on a ratchet | * | `testTypecheckFindings` | this repo's TEST sources typecheck — every package config excludes them, so `tsc -b` reads none of the 966 | `tsc -p tsconfig.tests.json`, on a ratchet | + * | `generatorCountFindings` | a documented `N files` for `x new` / `x g resource` is what the generator still emits — five had gone stale and were corrected by hand | `planNewApp()` / `generate()`, the planners `--dry-run` calls | + * | `releaseFactFindings` | the package COUNT ten pages restate is the count on disk; `SECURITY.md` claimed 28 two majors late | `listWorkspaces()` | * * `testTypecheckFindings` rides HERE and not on `typecheck`, which is where it belongs by meaning: * that step takes no host findings at all (`packages/cli/src/cmd-verify.ts` calls `hostFindings` @@ -223,6 +238,8 @@ export const frameworkFiles: HostCheck = async (root) => [ ...(await versionStampFindings(root)), ...(await readmeFenceFindings(root)), ...(await testTypecheckFindings(root)), + ...(await generatorCountFindings(root)), + ...(await releaseFactFindings(root)), ]; export const HOST_CHECKS: Partial> = { diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index e512afc55..861cfdc67 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -22,7 +22,7 @@ X_DB_DRIFT: schema differs from migrations | Registration | a code exists when its owning package calls `registerErrorCodes()`. That one call is what makes it explainable, unique and documented-or-fail — a code emitted as a `Finding` rather than thrown is registered the same way | | Enforcement | the `errors` step of `x verify` fails on an empty or advice-only `fix` (`X_ERROR_FIX_INVALID`), on a declared code with no row on this page (`X_ERROR_CODE_UNDOCUMENTED`), and on a row this page presents as live that no package registers (`X_ERROR_CODE_UNREGISTERED`) | -`As of 2026-08` every code above [Reserved codes](#reserved-codes) resolves through `x errors explain`, with one exception the gate knows about: this repository's own gate scripts (`X_BOUNDARY_VIOLATION`, `X_ROADMAP_*`, `X_REFERENCE_APP_*`, `X_TRUST_*`, `X_SCAFFOLD_OVERRIDES_EMPTY`, `X_SCAFFOLD_GATE_RED`, `X_SCAFFOLD_FIRST_RUN_FAILED`, `X_SETUP_INSTALL_FAILED`, `X_ADMIN_FLATTENER_VIOLATION`, `X_ERROR_RENDER_UNSAFE`, `X_ERROR_STATUS_MISSING`, `X_ERROR_STATUS_BACKLOG_STALE`, `X_ERROR_STATUS_UNKNOWN_CODE`, `X_CATALOG_KEY_UNREACHABLE`, `X_PUBLISH_LIST_INCOMPLETE`, `X_PUBLISH_LIST_UNKNOWN`, `X_BENCH_CLAIM_STALE`, `X_WIKI_TABLE_MALFORMED`, `X_FRAME_DOCS_STALE`, `X_CHART_VERSION_STALE`, `X_IMAGE_LIBC_MISMATCH`, `X_IMAGE_GUARD_MISSING`, `X_DOC_FIX_UNRUNNABLE`, `X_DOC_FIX_UNSCANNED`, `X_DOC_COMMAND_UNKNOWN`, `X_DOC_COMMAND_ALLOWANCE_STALE`, `X_DOC_COMMAND_UNSCANNED`, `X_README_EXAMPLE_UNCOMPILED`, `X_README_EXAMPLE_PIN_STALE`, `X_README_EXAMPLE_UNSCANNED`, `X_VERSION_STAMP_STALE`, `X_VERSION_STAMP_DUPLICATE`, `X_VERSION_LOCKSTEP_BROKEN`, `X_VERSION_STAMP_UNSCANNED`, `X_COVERAGE_BELOW`, `X_COVERAGE_PIN_STALE`, `X_COVERAGE_UNMEASURED`, `X_TEST_TYPECHECK_REGRESSED`, `X_TEST_TYPECHECK_PIN_STALE`, `X_TEST_TYPECHECK_UNSCANNED`, `X_LOCKFILE_STALE`) never ship, so no package may own them. See [Troubleshooting](Troubleshooting) for symptom-first triage and [CLI reference](CLI-Reference) for the commands named in the fixes. +`As of 2026-08` every code above [Reserved codes](#reserved-codes) resolves through `x errors explain`, with one exception the gate knows about: this repository's own gate scripts (`X_BOUNDARY_VIOLATION`, `X_ROADMAP_*`, `X_REFERENCE_APP_*`, `X_TRUST_*`, `X_SCAFFOLD_OVERRIDES_EMPTY`, `X_SCAFFOLD_GATE_RED`, `X_SCAFFOLD_FIRST_RUN_FAILED`, `X_SETUP_INSTALL_FAILED`, `X_ADMIN_FLATTENER_VIOLATION`, `X_ERROR_RENDER_UNSAFE`, `X_ERROR_STATUS_MISSING`, `X_ERROR_STATUS_BACKLOG_STALE`, `X_ERROR_STATUS_UNKNOWN_CODE`, `X_CATALOG_KEY_UNREACHABLE`, `X_PUBLISH_LIST_INCOMPLETE`, `X_PUBLISH_LIST_UNKNOWN`, `X_BENCH_CLAIM_STALE`, `X_WIKI_TABLE_MALFORMED`, `X_FRAME_DOCS_STALE`, `X_CHART_VERSION_STALE`, `X_IMAGE_LIBC_MISMATCH`, `X_IMAGE_GUARD_MISSING`, `X_DOC_FIX_UNRUNNABLE`, `X_DOC_FIX_UNSCANNED`, `X_DOC_COMMAND_UNKNOWN`, `X_DOC_COMMAND_ALLOWANCE_STALE`, `X_DOC_COMMAND_UNSCANNED`, `X_README_EXAMPLE_UNCOMPILED`, `X_README_EXAMPLE_PIN_STALE`, `X_README_EXAMPLE_UNSCANNED`, `X_VERSION_STAMP_STALE`, `X_VERSION_STAMP_DUPLICATE`, `X_VERSION_LOCKSTEP_BROKEN`, `X_VERSION_STAMP_UNSCANNED`, `X_COVERAGE_BELOW`, `X_COVERAGE_PIN_STALE`, `X_COVERAGE_UNMEASURED`, `X_TEST_TYPECHECK_REGRESSED`, `X_TEST_TYPECHECK_PIN_STALE`, `X_TEST_TYPECHECK_UNSCANNED`, `X_DOC_FILE_COUNT_STALE`, `X_DOC_FILE_COUNT_UNSCANNED`, `X_DOC_RELEASE_FACT_STALE`, `X_DOC_RELEASE_FACT_UNSCANNED`, `X_TEST_FIX_UNRUNNABLE`, `X_TEST_FIX_PIN_STALE`, `X_TEST_FIX_UNSCANNED`, `X_TEST_THROW_NOT_THROWN`, `X_RELEASE_VERSION_UNSTATED`, `X_RELEASE_FLAG_UNKNOWN`, `X_LOCKFILE_STALE`) never ship, so no package may own them. See [Troubleshooting](Troubleshooting) for symptom-first triage and [CLI reference](CLI-Reference) for the commands named in the fixes. ## Core and runtime @@ -266,6 +266,8 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | `X_STEP_DUPLICATE` | two `step.run` calls share a name | copy-paste inside `run()` | rename one — step names are the replay key | | `X_JOB_MAX_ATTEMPTS` | the job exhausted its retries | a step kept failing | `x jobs show --json` for the step trace, then `x jobs retry ` | | `X_JOB_TIMEOUT` | a job exceeded its wall-clock limit | one long step | raise `timeout`, or split the work into more `step.run()` calls | +| `X_JOB_ROW_STATUS_UNKNOWN` | a queue row carries a status this build does not know | a rolling deploy where a **newer** worker wrote a status this build has no name for. The decoders used to `as`-cast the column, which is worse than it sounds: `steps.ts:263` returns a step's memoized output only when the status is `completed`, so a laundered value turned "this step already ran" into **run it again** | drain the older workers, or roll forward — `x jobs show --json` prints the row. The reverse direction cannot fire: a release that only adds a status still knows every older one | +| `X_ACTION_JOB_UNBRIDGED` | an action projection was registered as a job | `registerJobs({ x: someAction.job() })`. `.job()` returns a **handle**, not a job — `action` and `jobs` are both tier 3 so neither may import the other. It used to be skipped in silence alongside the helpers and constants a module namespace carries, so the job simply never ran and nothing failed anywhere | register the real `job()`, or bridge it the way `agentJob()` in `@ultimat3/ai` does — it composes a job from the handle's `idempotencyKey` and `invoke` | | `X_JOB_CONCURRENCY_UNENFORCEABLE` | `job.concurrency` is declared and cannot be enforced | a jobs driver with no lease store — the cap then holds per **process** and the fleet runs `concurrency × replicas`. `job.concurrency` was documented, in the manifest, and enforced by nothing; the worker now refuses to start rather than run with the wrong number, which is what axiom 3 means by a convention that is not a build error not existing | remove `concurrency` from the job `cause` names, or set `jobs: { driver: 'postgres' }` in `app.config.ts` | | `X_JOB_LEASE_LOST` | the queue took this job back mid-run | `x jobs cancel` wrote a terminal state, or this worker's visibility lease lapsed and another worker re-claimed the row. Its own code and not `X_ABORTED` because the response differs: `X_ABORTED` is "your deadline passed, make the work smaller", this is "somebody else owns this run now, stop writing to it" | `x jobs show --json` | | `X_JOB_SLOT_LOST` | the fleet concurrency slot this run held was taken by another worker | this worker stalled past the slot TTL and a second worker renewed slot 0 for itself, so `job.concurrency` was about to be exceeded. Its own code and not `X_JOB_LEASE_LOST` because the queue may still consider this worker the owner of the *job* — the two are different rows on different clocks (`x_job_leases` vs `x_jobs.visible_at`), and only one of them was lost. The run is aborted rather than left to double up | `x jobs show --json` — a run that keeps losing its slot is slower than its slot TTL, so raise the TTL or make the job shorter | @@ -336,6 +338,7 @@ Seven codes because each sends the reader somewhere different. Full walkthrough: | `X_STORAGE_TOO_LARGE` | payload exceeds the upload size limit | a file over the policy's `maxBytes`; `cause` carries both numbers | `uploadPolicy({ maxBytes: })` for a higher limit, or compress the file first | | `X_STORAGE_TYPE_REJECTED` | content type is not allowed for this upload | a type absent from `allowedContentTypes`, or magic bytes that disagree with the declared `Content-Type` — the bytes win | add the declared type to `allowedContentTypes` in the upload policy, or re-upload with the `Content-Type` `cause` sniffed | | `X_STORAGE_URL_INVALID` | signed URL does not match its signature | an edited constraint (`x-max`, `x-ct`, the key), a URL outside the mounted base, a `PUT` grant replayed as a `GET`, or a `Content-Type` header the signature does not cover | mint a fresh one with `grantUpload({ disk, orgId, request })` and send it unedited — every constraint is inside the signature | +| `X_STORAGE_URL_UNVERIFIABLE` | nothing can check the signature — no verifier was supplied | `acceptSignedUpload` was called with no `secret:` and the disk's driver implements no `verifySigned`. **Its own code deliberately, not `X_STORAGE_URL_INVALID`:** "no verifier" and "this signature is forged" need opposite edits, and reporting the first as the second sends an operator hunting an attacker who is not there | implement `verifySigned` on the driver, or pass `secret:` explicitly — a local disk already verifies its own | | `X_STORAGE_URL_EXPIRED` | signed URL is past its expiry | a form left open longer than the grant's window | call `grantUpload({ disk, orgId, request, expiresInMs })` again — a longer `expiresInMs` widens the window the signature grants | | `X_STORAGE_ORG_MISMATCH` | object key belongs to another org | a key from one tenant used while the actor resolves to another; the signature can be perfectly valid and this still fires | build it with `scopedKey('', ...parts)`, and pass the **actor's** org as `orgId` — never one read off the request | | `X_STORAGE_UPLOAD_FAILED` | the signed upload was refused | the presigned `PUT` answered non-2xx, or never reached the disk (`status: 0`) | call `uploadFile({ file, grant })` again for a fresh grant; a 4xx here is the constraint named in the body, not a transport fault | @@ -513,6 +516,7 @@ Two sets override the table, in `failures.ts`: | `X_SCRAPE_PAGE_CRASHED` | the renderer process died | **terminal**, and it is the one liveness fault that is. A dead renderer takes its tab's state with it, so a re-run of a half-submitted form is the incident rather than the recovery. Out of memory far more often than a bug | lower `concurrency:` on the `scrape()` definition, or raise the container memory limit in `docker-compose.prod.yml` | | `X_SCRAPE_OUTPUT_INVALID` | the extracted rows do not match the extract schema | **terminal.** The selectors still match and what they yield no longer parses — a column that became empty, a number that became a range, a date that changed format | align the extract schema with the page — the run's `page.html` artifact holds the markup the rows came from | | `X_SCRAPE_YIELD_COLLAPSED` | the run succeeded and returned far too little | **terminal, and the silent-green alarm.** Two triggers: fewer rows than `expect.minRows`, or a fall past `expect.maxDrop` against the trailing median of the last `expect.window` runs (default 7, and no baseline below 3 samples). A run that succeeds and returns nothing is the worst failure a scraper has, because it stays green. **A collapsed run is deliberately not recorded** — check, then record, and record only a run that passed, or three broken runs at 2 rows make the median 2 and the alarm silences itself exactly when it was working | open the run's `page.html` artifact and compare it with the extract selectors — a collapse is the page changing far more often than the data changing | +| `X_SCRAPE_YIELD_HISTORY_MISSING` | a `maxDrop` is declared and no history store can supply its baseline | **terminal, raised at declaration time.** `expect.maxDrop` measures a fall against a trailing baseline, and with no `history:` store `expect.ts` reads `[]` forever — so `X_SCRAPE_YIELD_COLLAPSED` can never fire and the alarm is silently inert. `expect.minRows` is an absolute floor checked before the history gate and stays legal on its own | add `history: memoryYieldHistory()` for a test, or your own `YieldHistory` in production — or drop `expect.maxDrop` and keep `expect.minRows` | | `X_SCRAPE_DOWNLOAD_TIMEOUT` | the download never landed | **retryable.** A slow file, or a trigger that is not the element that starts the download | raise the timeout on `page.download({ timeout })`, or confirm the trigger is the element that starts the download | | `X_SCRAPE_ROBOTS_DISALLOWED` | `robots.txt` disallows this path | **terminal.** The site's own file refuses this path for the declared user agent | declare `robots: { ignore: '' }` on the `scrape()` definition, or scrape a path `robots.txt` allows | | `X_SCRAPE_FIXTURE_MISSING` | the fixture driver has no recording for this request | **terminal.** An offline driver never reaches the network, so an unrecorded URL has no answer to give | add `/` holding `{ "url": "…", "html": "…" }`, or point `fixtureBrowser()` at the directory that already holds it | diff --git a/wiki/Tutorial-02-First-Feature.md b/wiki/Tutorial-02-First-Feature.md index 3864f9d68..2bc62e6cb 100644 --- a/wiki/Tutorial-02-First-Feature.md +++ b/wiki/Tutorial-02-First-Feature.md @@ -25,7 +25,7 @@ bunx x g resource todo --dry-run + apps/web/app/todo/ui.tsx ui.module.scss ui/todo-card.tsx ui/todo-form.tsx + packages/i18n/catalogs/en.json + apps/web/app/todos/page.tsx page.module.scss page.test.ts page.e2e.test.ts -✓ wrote 25 file(s) for resource todo +✓ wrote 27 file(s) for resource todo ``` Drop `--dry-run` to write them. `x g` never clobbers — an existing file is `X_GENERATE_CONFLICT`; the i18n catalog is merged key-by-key rather than overwritten; and a **slice module** (`entity.ts`, `repo.ts`, `policy.ts`, `errors.ts`) the slice already has is skipped, `--force` included, because it belongs to the slice rather than to the generator that needed it. **A run whose catalog merge gains no key writes 24**: a merge that changes nothing is skipped rather than counted. @@ -34,7 +34,7 @@ Drop `--dry-run` to write them. `x g` never clobbers — an existing file is `X_ | Generator | Emits | |---|---| -| `x g resource ` | the whole slice above — 25 files, 27 with `--admin --live` | +| `x g resource ` | the whole slice above — 27 files, 29 with `--admin --live` | | `x g entity` / `policy` / `action` / `mutator` / `query` / `job` / `task` | that primitive plus its test — **and the slice modules its own source imports**, when the slice has none: `x g job` is 5 files into a bare slice, `x g action` 8. Which ones differ per generator, so a job plants no `policy.ts` ([CLI reference § x g](CLI-Reference)) | | `x g route --surface site\|app` | `page.tsx`, its stylesheet, its test, its catalog keys |