feat(platform-objects): sys_job.timezone and sys_report_schedule.timezone are validated against the IANA domain - #16296
Conversation
…dule.timezone against the IANA domain Both columns predate `valueDomain` and disagreed in three dimensions at once (length 100 vs 64, default none vs 'UTC', validation neither). This closes the validation dimension only: both now declare `valueDomain: 'iana_time_zone'`, the same declaration `sys_business_unit.timezone` / `sys_organization.timezone` carry, and the same shared `Intl.DateTimeFormat` membership probe. The reader measurement that decides what this is worth is recorded beside each declaration: `sys_report_schedule.timezone` is read back into croner by `ReportService.nextRunAt`, whose catch turned a non-member zone into a silent fall back to `interval_minutes` (the wrong instant, permanently), while `sys_job.timezone` is written and never read. Defaults and bounds are deliberately left unconverged and pinned as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
📓 Docs Drift CheckThis PR changes 1 package(s): 5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 2 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 29dd8c7deadc561a6cd3364354b202cd417cb1d4 && git checkout 29dd8c7deadc561a6cd3364354b202cd417cb1d4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7beaaa32ccb1295eeaec73bdca4aea36e713232d 0491ac9db33877c85adda29574d38304aa2bd882 && git checkout -B drift-repro 7beaaa32ccb1295eeaec73bdca4aea36e713232d && git merge --no-ff 0491ac9db33877c85adda29574d38304aa2bd882
node scripts/docs-audit/affected-docs.mjs --json 7beaaa32ccb1295eeaec73bdca4aea36e713232d
|
Correction — #16229 is ruled (option C, 13:51Z), so this PR's
|
Contract review (clause ②) — PASS, bound to head
|
probe (update) |
base | head |
|---|---|---|
timezone: 'Mars/Olympus' |
ACCEPT | REFUSE timezone:value_domain |
'UTC+8', 'China Standard Time' |
ACCEPT | REFUSE |
'Asia/Shanghai', 'UTC', '', null |
ACCEPT | ACCEPT |
…on both objects. The refusal is loud, not a strip: record-validator.ts:655-659 tests membership, :668-674 returns fail('value_domain', …), :1178 if (errors.length > 0) throw new ValidationError(errors);. ⇒ Clause-②: yes is correct on limb 2 alone.
3. The question I was most worried about — answered by the published contract text
My Zone 2.5 asked whether a stored non-IANA row could now make a read or a boot fail. The answer is in field.zod.ts:1023, verified verbatim by this seat:
Checked on the WRITTEN value only (the
min/max/maxLengthtransition-gate class): a stored value outside a domain declared later is never re-read and survives unrelated edits — only a write carrying a non-member is refused, with the field error codevalue_domain.
Corroborated by measurement: the validator has no read or boot mode (record-validator.ts:155 type Mode = 'insert' | 'update';); isMissing (:188-190) skips null / '', which is what sys_job's upsert writes; the boot value-shape scanner is blind to text by construction (valueShapeViolation short-circuits; 0 valueDomain hits against a firing control of 3); driver-sql reads valueDomain nowhere for DDL (control: maxLength 95 hits); and DbJobAdapter.upsertJobRow runs after the cron adapter accepted the schedule (:205) and is caught into a warn (:513-515), so even a refused upsert cannot fail boot. ⇒ no blocking finding.
Zone 2.4 confirmed: both identity/ edits are comment-only — 0 non-comment changed lines, against a firing control (the same filter on sys-job.object.ts yields the one valueDomain line), and dist/identity/index.d.ts is byte-identical. Both objects already declared the domain at base.
Zone 1 respected: Q1 (the length bound) stays open and the diff moves no length constraint — maxLength: 100 / 64 are unchanged context lines.
4. ⚠️ One finding I am escalating rather than closing
The changeset self-describes as "A NON-BREAKING ADDITION" and carries no **BREAKING** banner, while the PR declares Clause-②: yes — i.e. an accept-set narrowing. The reviewer classified it a note, on grounds this seat verified: AGENTS.md:1021-1022 defines breaking as "removes or renames anything an author can write", which a value-set narrowing is not; check-changeset-no-major.mjs:41-42 confirms minor is the ceiling in the launch window; and precedent #14238 (6acb37eb9) shipped the identical shape in this same package with no banner.
⛔ It does not block this PR — the in-package precedent is unambiguous. But a sibling review this round found the opposite convention in @objectstack/core, where two precedents of the same shape (d8024f0, the Plugin.type narrowing, and 4635f3e) do carry the banner. Two packages have drifted apart on whether a narrowing owes it. That is a policy question, not this author's, and this seat is filing it as a finding rather than resolving it here.
5. Landing
① this record, ② --pair 16296 exit 0, ③ 37/37 complete, 0 failing. Card #15872 carries no sibling PR. Released.
Generated by Claude Code
⛔ Correction — I released this PR 40 minutes ago on an incomplete reading. Pulled back out of the merge queue. Nothing landed.
State right now: auto-merge disabled, PR returned to draft, so it is out of the queue. Verified by content on What I missedThe review I accepted judged the missing Neither survives contact with the more specific text, which a sibling review on #16363 found and which I have now read at this PR's own head:
That text is repo-wide, not package-local; it names "an accept-set narrowing" in those words as the breaking shape; and it says in terms that the banner is mandatory and explicitly not a nicety. ⇒ This PR narrows the accept set of a published door — that is measured, not disputed:
Why I am holding rather than waving it throughI called this exact defect blocking on PR #16363 fifteen minutes before writing this. The only difference between the two PRs is which package they live in, and the text above is not package-scoped. Releasing this one while blocking that one would not be a judgement — it would be me protecting an action I had already taken. ⛔ Not acceptable. What is NOT in question⭐ The clause-② verdict stands, and so does the code. The entire remedy is text, in the
⛔ No code change. ⛔ No re-review of clause ② is owed once the text lands — the head will move, so a fresh condition-① record is owed, but the measurement above transfers. On precedent #14238It shipped the identical shape with no banner, so it is non-conforming with this text too. ⛔ That is not this author's problem and I am not asking anyone to fix it here. It is the subject of #16421, which I filed 20 minutes ago describing this as a divergence between two packages — that framing is now wrong, and I am correcting the card: there is a governing rule, and one package's precedent does not follow it. Generated by Claude Code |
⛔ This branch is ref-locked by a surviving merge-queue entry. The fix commit is ready and cannot be pushed until someone dequeues this PR.
What is neededPR page → "Remove from queue" (or the merge queue view for The state, measuredThe fix for the two findings in ⛔ Not a permissions problem — a probe push to a throwaway branch on the same remote succeeded (and was deleted). The cause is in this PR's own timeline, read from REST with a firing control (18 total events): ⇒ Converting to draft turned auto-merge off but did not remove the queue entry, and a surviving entry ref-locks the branch. So this PR is stranded in a state where it cannot merge (it is a draft) and cannot be fixed (the ref is locked).
This seat cannot clear it: the ⛔ What I deliberately did NOT do
The fix that is waitingBoth findings from
⭐ And the root cause of the old reading is now understood precisely: the file's only occurrence of the token was inside the words "A NON-BREAKING ADDITION", while the predicate is The new body also states the consumer delta the old one lacked — which spellings stop being accepted ( Once dequeued: Generated by Claude Code |
⛔ This merged, with the defective changeset. My hold did not hold, and the reason is an assumption of mine that was wrong.
What landedVerified by content on
And the changeset that came with it, which is the problem: ⇒ the code is fine and was never in question. What shipped is a release note that tells an upgrading author this change cannot break them, immediately before a write that worked yesterday is refused. Why the hold failedI converted this PR to draft at 22:28Z to hold it. That turned auto-merge off but left the merge-queue entry alive (timeline: Then I reasoned my way into waiting, in
That was wrong on the load-bearing clause. A queued PR merges from the queue; the draft flag did not stop it. ⇒ drafting a queued PR does not hold it — only dequeuing does, and this seat has no dequeue (GraphQL is disabled here entirely; there is no REST merge-queue route). So a queued PR is not something I can hold at all, and I should have said exactly that instead of describing it as held. ⛔ The deeper error is the one worth carrying: "a draft cannot merge" was an assumption I never measured, and I used it as the premise for a decision not to act. Two real observations — it was a draft, and it hadn't merged in 50 minutes — were consistent with it, and I took that as confirmation. But "has not merged yet" is not "cannot merge". I would not have accepted that as a control reading; I accepted it as a safety argument, which is the same mistake wearing a different hat. Remedy, in flightA changeset is consumed at release, so while it sits unreleased on ⭐ And the root cause of the original miss is worth stating once, because it is a trap anyone can fall into: the file's only occurrence of the token was inside the words "A NON-BREAKING ADDITION", while ⛔ Nothing here is asked of this PR's author. The contract review passed and the code is correct. Generated by Claude Code |
Part of #15872 — the validation dimension only. The bound convergence row stays open on that card; see "What is deliberately not landed" below.
Clause-②: yes
needs:contract-reviewis hung on this PR and on the card. This seat does not clear it and does not release this PR.What this changes
valueDomain: 'iana_time_zone'onsys_job.timezoneandsys_report_schedule.timezone— the same declaration and the same sharedIntl.DateTimeFormatmembership probe thatsys_business_unit.timezone/sys_organization.timezonealready carry (#14238). Four columns, one spelling of "is this a real zone". A written non-member is now refused with the ADR-0114 field error codevalue_domain.Also in the diff: a pin test for both columns, and three prose sites that my own change made false (the
#14238pin test's docblock and the two identity object comments all stated in as many words that these two precedents were "neither validated").The card's FIRST STEP, which decides its severity — done before any declaration was touched
The card said this was not measured and that it decides the severity. It was measured at the actual consumption sites. The two readers behave completely differently, and only one of them was dangerous.
sys_report_schedule.timezone-> plugin-reports: it schedules at the WRONG INSTANT, silently and permanently. Not a throw, and not a fall back to UTC.ReportService.rowFromSchedule) and handed to croner innextRunAt:new Cron(cron, { timezone }).nextRun(from).Cronconstructs fine with an invalid zone and throws fromnextRun().nextRunAtcatches that and falls back tofrom + interval_minutes, loggingReportService: invalid cron '...'; falling back to interval— a warning that names the cron expression, which was fine, rather than the timezone, which was not.dispatchDue->advanceSchedule->nextRunAtruns it on every sweep, so it never self-corrects.sys_job.timezone-> service-job: nothing reads it.DbJobAdapter.upsertJobRowwrites it; its threesys_jobread sites takeid/run_count/failure_countonly. The tree's singlerow.timezoneread belongs tosys_report_schedule— same search shape, one fires, the other is zero, so this is a measurement rather than a blind spot.toBoundaryJobSchedule->CronJobAdapter.schedule-> croner). There croner is constructed with a callback, which does throw on a non-member, andAppPlugincatches it per job asBackground job FAILED TO SCHEDULE — it will never run(error level plusjobScheduleFailuresTotal).DbJobAdapter.scheduleawaits the cron adapter beforeupsertJobRow, so that path cannot even write a non-member into the column. The door this declaration actually closes is the other one: a direct write from Studio, REST or a script, which had no validation at all.This falsifies nothing the PM assumed and confirms assumption 2.4: the two consumers do differ, which is exactly why the measurement was the card's first step.
What is deliberately NOT landed, and why the card stays open
The bound convergence (100 -> 64 on
sys_job) is dropped. The triage comment forbids narrowing without reading what the column physically holds, and there is no deployment data readable from this repo. What I could measure, offered so the decision can be taken on evidence:maxLengthis not only a write bound — it reaches DDL.driver-sql's drift checker plans anarrow_varcharop at severityerror, category destructive, for a declared bound below the physical column's: "metadata caps at 64 chars but the column allows 100 — narrowing may truncate.os migrate apply --allow-destructive."timezoneis not keyed onsys_job(its only index is{ fields: ['name'], unique: 'global' }), and the emitter's rule iskeyable = keyed ? keyableTextLength(field) : null— an unkeyed text field is TEXT, somaxLengthnever reaches its DDL, and thenarrow_varcharbranch is gated onisCharacterColumn. A column of some other provenance is what cannot be ruled out from here.America/Argentina/Rio_Gallegosat 30 characters; the longest tzdb link,America/Argentina/ComodRivadavia, is 32 and the probe admits it; a 65-character string is refused. So after this PR, 100 admits nothing 64 would not — the convergence is cosmetic on the write path and carries only the DDL question. That is an argument about the domain, though, not a reading of the data, which is precisely what the triage comment says is not sufficient.The defaults are deliberately NOT converged (
sys_report_schedulekeeps'UTC',sys_jobkeeps none) — a default here is a consumer semantic, not a shape question. Both non-convergences are pinned in the new test so that leaving them alone stays a decision rather than becoming a drift someone repairs by reflex.Published-surface measurement (clause ②)
A real ablation: build at head, swap the four changed sources back to
dacb73f4fby blob (proved on disk: each file's hash equal to the base blob and unequal to the head blob), rebuild withtsupinvoked directly so no turbo cache is on the path, snapshot, restore, prove the restore (all four blobs equal their HEAD blob,git diff HEADempty), rebuild, and re-prove the marker is back indist/withscripts/ablation-dist-preflight.mjs(16 built files carry it). Rebuilds proved by mtimes, not assumed.dist/**/*.d.ts,*.d.mts) are BYTE-IDENTICAL. The type surface does not move.dist/index.js,dist/index.mjs,dist/audit/index.js,dist/audit/index.mjscarry exactly two non-comment additions each — the twovalueDomain: "iana_time_zone"lines.dist/identity/index.jsanddist/identity/index.mjshave zero non-comment hunks: comment-only, so not a surface change.So clause ② is yes on the accept set, not on the type surface: a write this package accepted is now rejected. Worth stating plainly because a
.d.ts-only census would have answered "no" here with confidence — for a package that publishes object schemas as data, the accept/reject change lives indist/**/*.js.Verification
Gate families derived with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(re-derived after the changeset landed: 46 families became 55). All 55 run green at0491ac9db, the commit this PR carries, on a clean tree — exit codes captured by redirect-then-read, never through a pipe.pnpm --filter @objectstack/platform-objects test— 36 files, 539 tests pass. The new file is proved in the swept set by a targeted verbose re-run: 7 of 7 pass.pnpm --filter @objectstack/platform-objects typecheck— clean, but that is NOT a reading about the new test file: the package's tsconfig excludes**/*.test.ts, so--listFilescounts 0 for it and 0 for the existing#14238pin test. Measured separately with a config that includes them: 0 errors in the new file, which was in the swept set; the only 3 errors are pre-existing in the untouchedsrc/feature-gate-guard.test.ts.pnpm check:type-check-debt— 12 ledger entries re-measured, 140 raw errors, none above its recorded number. Needed the CI-shaped 6144 MB ceiling: at a tighter cap it OOMs and exits 3, which isPREREQUISITE NOT METand not a red.check:i18nandcheck:i18n-stale-fillgreen with 9 packages all in sync, andgit statusis empty after a full workspace build — no generated baseline, form or snapshot moved.valueDomainis not an extracted string.check:keyed-text-bounds,check:dts-closure(71 built packages swept),check:dual-build-cjs-loads,check:adr-0087-registration,check:nul-bytes— green.Three gates first returned exit 3 =
PREREQUISITE NOT METon an unbuilt tree. Those are recorded as NOT MEASURED and re-run green afterturbo run build; none of them was ever a finding.Changeset
minoron@objectstack/platform-objects— the act, not the commit type: a declared shape on a publishedisSystemobject narrows what it accepts. The ADR-0087 disposition isnot-required (no-migration-prescription), which is the card's own binding reading rather than a flag invented to satisfy a gate:valueDomainis themin/max/maxLengthtransition-gate class, so only a written value is judged, a stored non-member is never re-read, and no DDL is planned because no bound moved.Findings raised, not fixed here
Both are outside this card and were filed unassigned after a dedup search (REST
/search/issuesanswers 403 on this session — repo-scoped endpoints only — so one targeted MCPsearch_issueswas used instead, with this card returning as a firing control; no open card covered either):timezonesilently discards a report schedule's cron and fires it on the interval cadence forever — and the create-time guard cannot see it #16291 — plugin-reports' create-time guard is blind to the timezone half of its own input, andnextRunAt's warning names the wrong one. This PR closes the write door for new rows; it does not repair either of those.CronScheduleSchema.timezonein packages/spec is an unvalidatedz.string()— the authoring tier for a job cron zone does not use the membership predicate its own package exports #16292 —CronScheduleSchema.timezoneinpackages/specis an unvalidatedz.string(), in the same package that exports the membership predicate.Neither is addressed here and both remain open.
🤖 Generated with Claude Code
https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
Generated by Claude Code