One timezone rule everywhere, and CI runs the Bun this repo runs - #265
Merged
Conversation
… table forbids importing it 6.0.0 made the rule structural in `@ultimat3/time` — a zone is `Area/Location`, or `UTC` — because `Intl` answers "can I format this" and never "is this IANA", and ICU 78 made the two disagree. Three packages kept asking `Intl`, so the rule reached `task()` and every `@ultimat3/time` entry point and stopped everywhere else. `defaultTimeZone: 'CET'` booted clean and threw on the first format call, from a stack naming no configuration (#257). `t.timezone` is worse and is not ICU drift: it accepts `+01:00` off a request body or a query string, and ES2024 `Intl` has resolved that under every runtime this framework has shipped on. Three sites, three different correct fixes, and the difference is the point: - `core` (tier 0) may not import `time`, so it states the rule itself — `time-zone-name.ts`, 43 lines, pointing at `zone-canonical.ts` for the reasoning rather than re-deriving it. - `schema` (tier 0) is the same case for the same reason, and is the one that guards caller input. - `entity` (tier 2) may import `time` (tier 1), so it does, and its local probe is deleted. A third structural statement here would have been wrong: the tier table permits the real thing. A fourth site was already correct — `packages/jobs/src/task.ts:125` calls the validator and its comment says why. Noted so the class reads as closed. Two structural statements of one rule may not drift, so `packages/cli/src/timezone-validator-pin.test.ts` asserts all four predicates agree — `time`'s, core's, schema's, and `t.timezone` itself, which is the one an app actually calls. A correct copy wired to nothing still ships the bug; mutating the wiring alone fails 34 cases. It checks the shared corpus AND a sweep over `Intl.supportedValuesOf('timeZone')`. The sweep is the half a hardcoded list cannot replace: it catches a rule that got too NARROW. Proved by narrowing schema's to `/^[A-Za-z_]+\/[A-Za-z_]+$/`, which the corpus waves through and the sweep fails on `America/Argentina/Buenos_Aires`. Neither tracked app migrates: every `SUPPORTED_ZONES` entry and every `defaultTimeZone` in this repo and in `x new`'s scaffold is already `Area/Location` or `UTC`. Refs #257 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KgsU1WBJMAjnLaazvfuJmD
…comment This box ran Bun 1.4.0 while CI was pinned `1.3.x`, and today that let a green local `bun run verify` sit on top of a change CI rejected: Bun 1.4 bundles an unresolvable `require()` inside a `catch` as a runtime throw, where 1.3.14 fails the build. A PR merged red because of it. Until the versions agree, a green local gate is not evidence of a green CI, which is worse than any single bug. `@types/bun@1.4.0` published today, which removed the last blocker. CI and release.yml move to the `1.4.x` SERIES — still not `latest`, for the reason the existing comment gives: `latest` lets a major land unannounced in every job. `docker/Dockerfile` moves to `oven/bun:1.4-slim` (x3), the scaffold's image to `1.4-alpine` (x2), and `@types/bun` to `^1.4.0` at all THREE sites — including `scaffold-repo.ts`, which would otherwise give every new app types a series behind the image it builds. `scripts/bun-pin.test.ts` asserts all five pin sites agree and that none says `latest`. It counts matches as well as checking them, because an empty match list agrees with everything — replacing a `FROM oven/bun:` line with `debian:trixie-slim` is what proves that guard fires. `engines.bun` stays `>=1.3.0` in all 41 manifests, deliberately. It is a CONSUMER install floor and the thing being bumped is a TOOLCHAIN pin. `Bun.Image` — the newest surface the framework touches — answers `function` on 1.3.14, verified inside `oven/bun:1.3-slim`, and that image installs from the committed lockfile under `--frozen-lockfile`. Raising the floor would forbid an install that demonstrably works. The honest cost: nothing in CI exercises 1.3 after this, so `>=1.3.0` becomes an untested claim — closing that is a 1.3 matrix leg or a deliberate floor move in the next major, and both are their own work. `scripts/setup.ts`'s `REQUIRED_BUN` does move, for a different reason: it is the repo's only statement of which Bun a CONTRIBUTOR should run, and a contributor whose Bun differs from CI's is not running the gate CI runs. The hand-restated number in its `cause:` now interpolates the constant. There is a second `REQUIRED_BUN` in `packages/cli/src/app-root.ts` behind `x doctor` — a consumer floor, correctly left at 1.3.0, and the split is documented so nobody "fixes" the wrong one. The v2 lockfile hazard did not materialise: Bun 1.4's install left `lockfileVersion: 1` in place, and 1.3.14 still reads it. `bun install` did correct pre-existing drift — the lockfile recorded `@ultimat3/*: ^1.2.0` for the demo app whose manifest declares `5.0.1`. Verified by building the image and running it: `/app/x --version` answers `5.0.1` inside distroless as `nonroot`, which exercises `--compile`, `--define` and `--external @babel/preset-typescript` on 1.4 end to end. `oven/bun:1.4-slim` is still Debian 13 trixie, so the `cc-debian13` runtime pairing holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KgsU1WBJMAjnLaazvfuJmD
…e tags `PUBLISHING.md` gains "Releasing without a human in the loop", because the flow was never written down: publish a GitHub Release for the tag, and CI publishes to npm. No token is involved — the workflow authenticates over OIDC, which is what stamps every tarball with provenance. `.env` is NOT part of that flow. It is needed for exactly one thing: the one-time bootstrap of a package that has never been published, which no trusted publisher can do because a publisher cannot attach to a package that does not exist yet. Publishing with `NPM_TOKEN` anywhere else is what made 2.0.0 the one release with `_npmUser: sebyx07` and no attestations. The section also carries the `gh api` call that approves the `npm-publish` environment without the UI, and a pre-approval checklist — because when a release runs unattended, those checks ARE the review. `wiki/Upgrading.md`'s `defaultTimeZone` row said "boots clean, then throws downstream — nothing will tell you". That was an accurate description of a defect this branch fixes; it now describes the refusal. `wiki/Error-Codes.md`'s `X_BUN_VERSION` cause named one floor. There are two: the consumer floor `x doctor` checks, and the contributor floor `scripts/setup.ts` checks, which tracks CI's series and is higher. Four docs quoted `oven/bun:1.3-*` image tags the tree no longer builds. The one in `docker/README.md` that is historical narrative correctly stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KgsU1WBJMAjnLaazvfuJmD
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Limit details: You’ve used the included review currently available. Your 77 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (28)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two things 6.0.0 needs before it is tagged.
bun run verify— 14 of 19 passed, 5 skipped, 0 failed.1. The timezone rule reached three packages and stopped — #257
6.0.0 made it structural in
@ultimat3/time: a zone isArea/Location, orUTC. Three packages kept askingIntl, which answers "can I format this" and never "is this IANA".The
t.timezonehalf is not ICU drift — ES2024Intlhas resolved+01:00under every runtime this framework has shipped on. It guards caller input, so the accepted value reached aformatcall that failed three layers later.Three sites, three different correct fixes, and the difference is the point:
coretimeschemaentityisValidTimeZoneand deletes its probe — the tier table permits the real thing, so a third statement would be wrongA fourth site was already correct:
packages/jobs/src/task.ts:125calls the validator, with a comment saying why. Noted so the class reads as closed.Two structural statements of one rule may not drift.
packages/cli/src/timezone-validator-pin.test.ts(tier 5, the lowest that can see all of them) asserts four predicates agree —time's, core's, schema's, andt.timezoneitself, which is what an app actually calls. A correct copy wired to nothing still ships the bug: mutating only the wiring fails 34 cases.It checks the shared corpus and a sweep over
Intl.supportedValuesOf('timeZone'). That sweep is the half a hardcoded list cannot replace — it catches a rule that got too narrow. Proved by narrowing schema's to/^[A-Za-z_]+\/[A-Za-z_]+$/, which the corpus waves through and the sweep fails onAmerica/Argentina/Buenos_Aires.Neither tracked app migrates. Every
SUPPORTED_ZONESentry and everydefaultTimeZonein this repo and inx new's scaffold is alreadyArea/LocationorUTC.2. CI runs the Bun this repo runs
The dev box ran 1.4.0 while CI was pinned
1.3.x. That let a green local gate sit on top of a change CI rejected — Bun 1.4 bundles an unresolvablerequire()inside acatchas a runtime throw where 1.3.14 fails the build, and a PR merged red because of it. That divergence is worse than any single bug.CI,
release.yml, three Dockerfile stages, two scaffold stages, and@types/bunat three sites — includingscaffold-repo.ts, which would otherwise hand every new app types a series behind its own image.scripts/bun-pin.test.tsasserts the five pin sites agree and that none sayslatest. It counts matches as well as checking them, because an empty match list agrees with everything.engines.bunstays>=1.3.0, deliberately. It is a consumer install floor; the thing bumped is a toolchain pin.Bun.Image— the newest surface the framework touches — answersfunctionon 1.3.14, verified insideoven/bun:1.3-slim, and that image installs from the committed lockfile under--frozen-lockfile. Raising it would forbid an install that demonstrably works.The honest cost, stated rather than hidden: nothing in CI exercises 1.3 after this, so
>=1.3.0becomes an untested claim. Closing that is a 1.3 matrix leg or a deliberate floor move in the next major — both their own work.The v2 lockfile hazard did not materialise. Bun 1.4's install left
lockfileVersion: 1, and 1.3.14 still reads it. The install did correct pre-existing drift: the lockfile recorded@ultimat3/*: ^1.2.0for the demo app whose manifest says5.0.1.Verified by building the image and running it —
/app/x --versionanswers5.0.1inside distroless asnonroot, exercising--compile,--defineand--external @babel/preset-typescripton 1.4 end to end.3. Docs
PUBLISHING.mdgains "Releasing without a human in the loop", because the flow was never written down: publish a GitHub Release for the tag, CI publishes to npm, no token involved..envis needed for exactly one thing — the one-time bootstrap of a never-published package. Plus thegh apicall that approves the environment, and a pre-approval checklist, because for an unattended release those checks are the review.wiki/Upgrading.md'sdefaultTimeZonerow said "boots clean, then throws downstream — nothing will tell you". That was an accurate description of the defect this PR fixes.Filed while here: #264 — two tests assert wall-clock behaviour and lose under 8-worker contention. One claims constant time, which a loaded box cannot honestly demonstrate.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KgsU1WBJMAjnLaazvfuJmD
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.