fix(gate): close five silent-pass holes in x verify, plus the CLI surface agents actually hit - #83
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (79)
Comment |
…face agents actually hit Tier 5 and the build tooling — the last of a four-PR audit sweep, and the one that matters most: **several findings are holes in `bun run verify` itself.** A gate step that silently passes is worse than no step, because it is read as coverage. **Stacked on #80** — the doctor check consumes a storage export from it, and `noNonNullAssertion` could only be raised once its `ids.ts` fix landed. ## Gate holes closed | Where | The hole | |---|---| | `cli/src/cmd-verify.ts:147` | **The `budgets` step skipped its per-route JS/LCP half entirely when `.x/build-stats.json` was absent — and `.x/` is gitignored.** So it has never run, in CI or on either gated app, and reported green throughout. `bunx create-ultimate myapp && x verify` gives every generated route a `budget: {js:'60kb', lcp:2500}` and weighs none of them. `budgets.ts`' own docstring: "A declared budget with no measurement is a finding, never a pass … exactly the false green axiom 5 exists to prevent." An absent stats file is now `X_BUDGET_UNMEASURED` per budgeted route. **Both gated apps go red as a result**, so `budgets` is pinned in `scripts/lib/gated-apps.ts` for each, naming the `x build`-before-the-gate work that closes it — the honest outcome, rather than restoring the silent pass. | | `scripts/boundaries.ts:44` | **A relative cross-package import was invisible to the tier check** — `scopedName()` only recognised `@ultimat3/…` specifiers. The repo already contained one, with a comment describing it as a deliberate evasion: `cli/src/serve.live.test.ts` imported `../../testing/src/sealed-network` precisely because the package specifier would be refused. Any package could bypass its tier with `../../<pkg>/src/x`. Relative specifiers now resolve back to a package before the tier check. | | `scripts/boundaries.ts:56` | **`Bun.Transpiler.scanImports` erases type-only imports, so a type-only upward import was not a build error** — contradicting CLAUDE.md's "Enforced by `bun run boundaries`; a violation is a build error". `packages/core` could `import type` from `packages/cli` and the check would report clean. Zero live instances, so this was latent; now caught by a second pass over the source with the `type` keyword rewritten (not a regex, so template literals and doc blocks are not false positives). | | `biome.json` | The `lint` step printed "biome: no any, **no default exports**, **no raw colours**" — and enforced neither. `noDefaultExport` is not in Biome's `recommended` and was never enabled; Biome ignores `.scss` entirely, so all 64 stylesheets under `packages/ui/src` were unlinted. `noDefaultExport` is now `error`, `noNonNullAssertion` is raised from `warn` to `error` (`biome check` exits 0 on warnings, so `foo!` was unenforced), and the step's summary now names only what is actually enforced. | | `ui/src/tokens/tokens.test.ts:88` | The only raw-colour enforcement in the repo checked **exactly three files**, none of them a component stylesheet — the 51 `.module.scss` files under `components/` were covered by neither this test nor Biome. Now globs every `.scss` under `packages/ui/src` except the canonical token files, matches `rgb(`/`hsl(` as well as hex, and asserts it found >50 files so an empty glob cannot pass. | | `x.verify.json` | The suite floor omitted `job` and `eval`, both of which apply at the repo root — so deleting those suites turned them into silent skips and the gate stayed green at "17/17 (1 skipped)". | | `cli/src/test-select.ts:19` | `TEST_GLOB` was `**/*.test.ts`, missing `.test.tsx` — a JSX test would be excluded from the gate's `unit`/`contract`/`job` steps while `bun run test` at the root still ran it. | | `cli/src/source-files.ts:5` | `packages/*/e2e/**` was outside both `SOURCE_GLOBS` and the boundary collector, so `filesize`, `errors` and `boundaries` never saw three real source directories. | | `scripts/boundaries.ts:194` | The `shared/` leaf rule globbed `examples/*` only, so the **deployed demo app** — the one CI publishes an image for on every push to main — was checked by nothing blocking. | | `scripts/roadmap.ts:44` | Milestone numbers came from a hardcoded map, so "every milestone row carries a status marker" only covered 0–11. Appending a milestone 12 with no marker passed. Now parses the table itself; a row with no artifacts entry is `X_ROADMAP_MILESTONE_UNTRACKED`. | | `scripts/release.ts:98` | `--bump` was cast to its union with **no validation**: `--bump majr` fell through to `1.2.1`, so a breaking change ships as a patch. `--version 1.2` wrote `"version": "1.2"` into all 29 manifests. `report()` was also called with `ok: true` unconditionally, so pre-existing skew findings exited 0. | | `cli/src/workspace-checks.ts:97` | **`X_RELEASE_VERSION_SKEW` compared packages only to each other, so it had no anchor.** Nine tags (`v1.3.0`…`v1.10.1`) have been cut against 29 packages all still stamped `1.2.0` — the gate is green while the tag lies, and a real publish would die `EPUBLISHCONFLICT` on all 29. `package-shape` gains a `--release <version>` mode; `bun run scripts/release.ts --check <version>` asserts the lockstep version equals the version being published. Verified live: `--check 1.2.0` passes, `--check 1.10.1` reports 141 findings and exits 1. **`.github/workflows/release.yml` still needs the call added before the publish loop — that file is outside this PR and is tracked separately.** | ## CLI correctness — the agent-facing surface | Where | Defect | |---|---| | `cli/src/dispatch.ts:51` | The parse-failure path read `argv.includes('--json')`, so **`-j` was ignored and errors rendered as prose on stdout**. `x doctor -j --bogusflag` and `x nonexistentcmd -j` both emit human text — exactly the two cases an agent hits while always passing `-j`, and `JSON.parse` throws on the result. | | `cli/src/templates/route.ts:25` | **`x g route "posts/[slug]"` silently scaffolded a static route.** Every segment went through `kebab()`, which strips `[` and `]`, so the agent got `apps/web/app/posts/slug/page.tsx` with exit 0 and no warning — and a generated test hard-coding `params: {}`. | | `cli/src/cmd-generate.ts:376` | A missing `<name>` was reported as `X_CLI_UNKNOWN_COMMAND` for a command form that *is* known, with `fix: x g route <name>` — which pasted into bash is a **redirect**. Now a missing-positional error with a concrete runnable example. Swept the same shape across `mcp-errors.ts` (14 entries), `errors.ts` (5) and `cmd-planned.ts`: any `<placeholder>` now sits behind a `#`, never in the runnable half. | | `cli/src/templates/scaffold-repo.ts:225` | **`x new` scaffolded two house-rule violations into every generated app**: a restated `Money` interface (CLAUDE.md: "one declaration in `@ultimat3/schema` … never restated") and a bare `RangeError`. Flagged independently by two auditors. Now re-exports `MoneyValue as Money` and throws a generated `UltimateError` subclass. | | `cli/src/cmd-doctor.ts:165` | `--port` was `parseInt`'d unvalidated, so **a bad value turned the port probe into a check that cannot fail** — `x doctor --port abc` reports the environment shippable while 3000 is occupied. Same at `cmd-dev.ts:268` (`NaN` to `Bun.serve` binds an arbitrary port) and `cmd-test.ts` (`--workers 4abc` accepted as 4, while `cmd-verify.ts`' comment claimed it was refused). One `readIntFlag` now serves all four. | | `cli/src/cmd-doctor.ts` | New `X_STORAGE_SECRET_DEV` finding mirroring the existing dev-cursor-secret check, wired to `usesDevStorageSecret()` from #80. | | `cli/src/dev-runtime.ts:150` | `startServices` falls back to the local disk driver whenever `S3_ENDPOINT`/`S3_BUCKET` are unset, which after #80 throws at boot in production. Made that failure an *instruction*: it leads with the storage choice ("no S3_ENDPOINT/S3_BUCKET, so this production process fell back to the embedded disk at …") and names object storage first. Deliberately **not** an outright ban on the local disk in production — a single-node Compose deploy on a mounted volume with a real secret is a legitimate rung on the scale ladder, and refusing it is a deploy-shape decision, not a security fix. | | `cli/src/templates/action.ts:93` | Every generated feature shipped `code: 'X_INVOICE_NOT_FOUND'` beside `docs: '…/errors/X_NOT_FOUND'` — following the link landed on a different code's page. | | `cli/src/cmd-generate.ts:400` | `--dry-run` reported `summary: "wrote 4 file(s)"` while `data.dryRun` was true. | | `cli/src/cmd-errors.ts:88` | `x errors` with no code reported a `--code` flag **that does not exist**; an agent reading the cause literally gets a second error. | | `scripts/new-package.ts:98` | Every scaffolded package documented "may import tiers 0-5" regardless of `--tier`, because the allowed range was derived from the tier table the new package is not yet in. `--tier abc` produced "Tier NaN". The file had no test; it has one now. | | `scripts/lib/tiers.ts:46` | `create-ultimate` resolved to the unlisted tier, so its declared `→ cli` edge restricted nothing — it could import all 28 packages. Now pinned above the table with that edge as its only permitted import. | | `scripts/lib/run.ts:18` | A bare `RangeError` at the scripts' single subprocess boundary, where `cli/src/exec.ts` does the identical guard correctly and explains why. | | `scripts/scaffold-smoke-overrides.ts` | Wrote prose to stdout, took no `--json`, and exited 1 with no code — and if it found no workspace manifests it wrote `{}`, silently making the smoke job install from **the npm registry** instead of the working tree. That is now a hard error. | | `create-ultimate/src/bin.ts:11` | The only published entry point using `process.stdout.write` + `process.exit`, the combination both `cli/src/bin.ts` and `scripts/lib/log.ts` carry comments explaining truncates at 64KB under a pipe. | | `ui/src/components/date-time-view.ts:42` | `toDate` fell back to `new Date(value)`, whose parse of an offset-less datetime uses the **host's** zone — an ambient default in the one package that forbids them. Every formatting path was correctly zoned; only the parse was not. | | `scripts/help.ts:21` | Claimed 16 verify steps against 17, and omitted the app gate — a headline command in CLAUDE.md — from the catalogue an agent reads to discover the repo. | ## Not closed, and named An undriven `e2eTest` still reports **green** rather than skipped. The filename half is fixed (the generated assertion now lands in `page.e2e.test.ts`, which the `e2e` step actually selects on) and `hasE2eDriver()` is exported so the seam has one name — but the driver registers inside the `bun test` child and the step's only channel is an exit code, which is 0 on skips. Making it red would turn a scaffolded app's `e2e` step red on `x g route` output, and that blast radius could not be proven here. `test-types.ts`' docstring no longer claims behaviour nothing implements. ## New error codes `X_STORAGE_SECRET_DEV`, `X_ROADMAP_MILESTONE_UNTRACKED`, `X_SCAFFOLD_OVERRIDES_EMPTY` — documented, registered, in the manifest. `X_BUDGET_UNMEASURED` already existed and was already documented; only the code path that emits it was missing. ## Behaviour changes `x doctor --port` / `x dev --port` / `x test --workers` now **refuse** bad values with `X_CLI_BAD_FLAG` instead of coercing them. Same surface, stricter contract; `wiki/Error-Codes.md`'s row for that code is widened to name positionals. ## Gate `bun run verify` green on this branch: 14/17, same three structural skips as `main`. `bunx biome check .` clean across 3547 files with both new rules at `error`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6217e2f to
d9a5c8f
Compare
Tier 5 and the build tooling — the last of a four-PR audit sweep, and the one that matters most: several findings are holes in
bun run verifyitself. A gate step that silently passes is worse than no step, because it is read as coverage.Stacked on #80 — the doctor check consumes a storage export from it, and
noNonNullAssertioncould only be raised once itsids.tsfix landed.Gate holes closed
cli/src/cmd-verify.ts:147budgetsstep skipped its per-route JS/LCP half entirely when.x/build-stats.jsonwas absent — and.x/is gitignored. So it has never run, in CI or on either gated app, and reported green throughout.bunx create-ultimate myapp && x verifygives every generated route abudget: {js:'60kb', lcp:2500}and weighs none of them.budgets.ts' own docstring: "A declared budget with no measurement is a finding, never a pass … exactly the false green axiom 5 exists to prevent." An absent stats file is nowX_BUDGET_UNMEASUREDper budgeted route. Both gated apps go red as a result, sobudgetsis pinned inscripts/lib/gated-apps.tsfor each, naming thex build-before-the-gate work that closes it — the honest outcome, rather than restoring the silent pass.scripts/boundaries.ts:44scopedName()only recognised@ultimat3/…specifiers. The repo already contained one, with a comment describing it as a deliberate evasion:cli/src/serve.live.test.tsimported../../testing/src/sealed-networkprecisely because the package specifier would be refused. Any package could bypass its tier with../../<pkg>/src/x. Relative specifiers now resolve back to a package before the tier check.scripts/boundaries.ts:56Bun.Transpiler.scanImportserases type-only imports, so a type-only upward import was not a build error — contradicting CLAUDE.md's "Enforced bybun run boundaries; a violation is a build error".packages/corecouldimport typefrompackages/cliand the check would report clean. Zero live instances, so this was latent; now caught by a second pass over the source with thetypekeyword rewritten (not a regex, so template literals and doc blocks are not false positives).biome.jsonlintstep printed "biome: no any, no default exports, no raw colours" — and enforced neither.noDefaultExportis not in Biome'srecommendedand was never enabled; Biome ignores.scssentirely, so all 64 stylesheets underpackages/ui/srcwere unlinted.noDefaultExportis nowerror,noNonNullAssertionis raised fromwarntoerror(biome checkexits 0 on warnings, sofoo!was unenforced), and the step's summary now names only what is actually enforced.ui/src/tokens/tokens.test.ts:88.module.scssfiles undercomponents/were covered by neither this test nor Biome. Now globs every.scssunderpackages/ui/srcexcept the canonical token files, matchesrgb(/hsl(as well as hex, and asserts it found >50 files so an empty glob cannot pass.x.verify.jsonjobandeval, both of which apply at the repo root — so deleting those suites turned them into silent skips and the gate stayed green at "17/17 (1 skipped)".cli/src/test-select.ts:19TEST_GLOBwas**/*.test.ts, missing.test.tsx— a JSX test would be excluded from the gate'sunit/contract/jobsteps whilebun run testat the root still ran it.cli/src/source-files.ts:5packages/*/e2e/**was outside bothSOURCE_GLOBSand the boundary collector, sofilesize,errorsandboundariesnever saw three real source directories.scripts/boundaries.ts:194shared/leaf rule globbedexamples/*only, so the deployed demo app — the one CI publishes an image for on every push to main — was checked by nothing blocking.scripts/roadmap.ts:44X_ROADMAP_MILESTONE_UNTRACKED.scripts/release.ts:98--bumpwas cast to its union with no validation:--bump majrfell through to1.2.1, so a breaking change ships as a patch.--version 1.2wrote"version": "1.2"into all 29 manifests.report()was also called withok: trueunconditionally, so pre-existing skew findings exited 0.cli/src/workspace-checks.ts:97X_RELEASE_VERSION_SKEWcompared packages only to each other, so it had no anchor. Nine tags (v1.3.0…v1.10.1) have been cut against 29 packages all still stamped1.2.0— the gate is green while the tag lies, and a real publish would dieEPUBLISHCONFLICTon all 29.package-shapegains a--release <version>mode;bun run scripts/release.ts --check <version>asserts the lockstep version equals the version being published. Verified live:--check 1.2.0passes,--check 1.10.1reports 141 findings and exits 1..github/workflows/release.ymlstill needs the call added before the publish loop — that file is outside this PR and is tracked separately.CLI correctness — the agent-facing surface
cli/src/dispatch.ts:51argv.includes('--json'), so-jwas ignored and errors rendered as prose on stdout.x doctor -j --bogusflagandx nonexistentcmd -jboth emit human text — exactly the two cases an agent hits while always passing-j, andJSON.parsethrows on the result.cli/src/templates/route.ts:25x g route "posts/[slug]"silently scaffolded a static route. Every segment went throughkebab(), which strips[and], so the agent gotapps/web/app/posts/slug/page.tsxwith exit 0 and no warning — and a generated test hard-codingparams: {}.cli/src/cmd-generate.ts:376<name>was reported asX_CLI_UNKNOWN_COMMANDfor a command form that is known, withfix: x g route <name>— which pasted into bash is a redirect. Now a missing-positional error with a concrete runnable example. Swept the same shape acrossmcp-errors.ts(14 entries),errors.ts(5) andcmd-planned.ts: any<placeholder>now sits behind a#, never in the runnable half.cli/src/templates/scaffold-repo.ts:225x newscaffolded two house-rule violations into every generated app: a restatedMoneyinterface (CLAUDE.md: "one declaration in@ultimat3/schema… never restated") and a bareRangeError. Flagged independently by two auditors. Now re-exportsMoneyValue as Moneyand throws a generatedUltimateErrorsubclass.cli/src/cmd-doctor.ts:165--portwasparseInt'd unvalidated, so a bad value turned the port probe into a check that cannot fail —x doctor --port abcreports the environment shippable while 3000 is occupied. Same atcmd-dev.ts:268(NaNtoBun.servebinds an arbitrary port) andcmd-test.ts(--workers 4abcaccepted as 4, whilecmd-verify.ts' comment claimed it was refused). OnereadIntFlagnow serves all four.cli/src/cmd-doctor.tsX_STORAGE_SECRET_DEVfinding mirroring the existing dev-cursor-secret check, wired tousesDevStorageSecret()from #80.cli/src/dev-runtime.ts:150startServicesfalls back to the local disk driver wheneverS3_ENDPOINT/S3_BUCKETare unset, which after #80 throws at boot in production. Made that failure an instruction: it leads with the storage choice ("no S3_ENDPOINT/S3_BUCKET, so this production process fell back to the embedded disk at …") and names object storage first. Deliberately not an outright ban on the local disk in production — a single-node Compose deploy on a mounted volume with a real secret is a legitimate rung on the scale ladder, and refusing it is a deploy-shape decision, not a security fix.cli/src/templates/action.ts:93code: 'X_INVOICE_NOT_FOUND'besidedocs: '…/errors/X_NOT_FOUND'— following the link landed on a different code's page.cli/src/cmd-generate.ts:400--dry-runreportedsummary: "wrote 4 file(s)"whiledata.dryRunwas true.cli/src/cmd-errors.ts:88x errorswith no code reported a--codeflag that does not exist; an agent reading the cause literally gets a second error.scripts/new-package.ts:98--tier, because the allowed range was derived from the tier table the new package is not yet in.--tier abcproduced "Tier NaN". The file had no test; it has one now.scripts/lib/tiers.ts:46create-ultimateresolved to the unlisted tier, so its declared→ cliedge restricted nothing — it could import all 28 packages. Now pinned above the table with that edge as its only permitted import.scripts/lib/run.ts:18RangeErrorat the scripts' single subprocess boundary, wherecli/src/exec.tsdoes the identical guard correctly and explains why.scripts/scaffold-smoke-overrides.ts--json, and exited 1 with no code — and if it found no workspace manifests it wrote{}, silently making the smoke job install from the npm registry instead of the working tree. That is now a hard error.create-ultimate/src/bin.ts:11process.stdout.write+process.exit, the combination bothcli/src/bin.tsandscripts/lib/log.tscarry comments explaining truncates at 64KB under a pipe.ui/src/components/date-time-view.ts:42toDatefell back tonew Date(value), whose parse of an offset-less datetime uses the host's zone — an ambient default in the one package that forbids them. Every formatting path was correctly zoned; only the parse was not.scripts/help.ts:21Not closed, and named
An undriven
e2eTeststill reports green rather than skipped. The filename half is fixed (the generated assertion now lands inpage.e2e.test.ts, which thee2estep actually selects on) andhasE2eDriver()is exported so the seam has one name — but the driver registers inside thebun testchild and the step's only channel is an exit code, which is 0 on skips. Making it red would turn a scaffolded app'se2estep red onx g routeoutput, and that blast radius could not be proven here.test-types.ts' docstring no longer claims behaviour nothing implements.New error codes
X_STORAGE_SECRET_DEV,X_ROADMAP_MILESTONE_UNTRACKED,X_SCAFFOLD_OVERRIDES_EMPTY— documented, registered, in the manifest.X_BUDGET_UNMEASUREDalready existed and was already documented; only the code path that emits it was missing.Behaviour changes
x doctor --port/x dev --port/x test --workersnow refuse bad values withX_CLI_BAD_FLAGinstead of coercing them. Same surface, stricter contract;wiki/Error-Codes.md's row for that code is widened to name positionals.Gate
bun run verifygreen on this branch: 14/17, same three structural skips asmain.bunx biome check .clean across 3547 files with both new rules aterror.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.