Skip to content

Guard the first-sync deadline's time zone (test-only; #416 was fixed by #419) - #427

Merged
philcunliffe merged 2 commits into
masterfrom
fix/issue-416
Jul 28, 2026
Merged

Guard the first-sync deadline's time zone (test-only; #416 was fixed by #419)#427
philcunliffe merged 2 commits into
masterfrom
fix/issue-416

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

Read this first: the bug in #416 is already fixed. This PR is test-only.

The rendering defect #416 reports (the first-sync deadline printing as a bare
local wall-clock time with no zone) was already fixed by #419, commit
0253fcc, merged 2026-07-28T15:40:40Z, about 15 hours after the issue was filed
against master @ dd3b438. formatFirstSyncDeadline on current master already
appends the zone.

This PR changes no production code. It adds only the regression guard the
issue also asked for:

Worth adding a case that pins the rendered deadline as carrying a zone token
at all, so the consent surface cannot silently regress to a bare time.

That ask was unmet: formatFirstSyncDeadline had no test coverage at all,
despite being the single formatter behind four consent surfaces (the enrolling
login message, hyp status, the hyp init privacy narration, and hyp sync).

Why the zone is required, not cosmetic

The deadline is the moment captured history first leaves the machine. A reader
deciding whether they have an hour or a day cannot answer that from a zoneless
time, and the string outlives its context: it is pasted, screenshotted, filed in
tickets, re-rendered by hyp status for the life of the hold, and read on hosts
whose clock is not the reader's.

  • llp/0100-enrollment-privacy-review.spec.md:104 (R1) requires the absolute
    local time with its time zone.
  • llp/0101-first-sync-review-window.decision.md:127: "an absolute time is only
    memorable if the deadline carries its time zone".

Reproduction evidence

Reverting src/core/usage-policy/first_sync_hold.js to its pre-#419 body
(git show 0253fcc^:src/core/usage-policy/first_sync_hold.js, i.e.
toLocaleString(undefined, { dateStyle: 'medium', timeStyle: 'short' })), both
new cases fail with exactly the symptom #416 describes:

not ok 6 - the rendered deadline names its time zone, never a bare wall-clock time (LLP 0100 R1)
---
error: 'America/Los_Angeles: the deadline must not render as a bare local time'
name: 'AssertionError'
expected: 'Jul 23, 2026, 11:59 PM'
actual: 'Jul 23, 2026, 11:59 PM'
operator: 'notStrictEqual'
not ok 7 - the rendered deadline reads as a full local date, time and zone in a pinned zone
---
error: |-
Expected values to be strictly equal:
+ actual - expected
+ 'Jul 23, 2026, 11:59 PM'
- 'Jul 23, 2026, 11:59 PM PDT'
^
name: 'AssertionError'
operator: 'strictEqual'

On current master, unmodified, the same file passes:

ok 6 - the rendered deadline names its time zone, never a bare wall-clock time (LLP 0100 R1)
ok 7 - the rendered deadline reads as a full local date, time and zone in a pinned zone
# tests 15
# pass 15
# fail 0

So the guard has teeth: it fails on the code that had the bug and passes on the
code that fixed it. The fix commit is #419's rather than one in this PR.

What the tests assert

Both are host-independent. withTimeZone pins process.env.TZ around each
render and restores it, and the instant is fixed (Date.UTC(2026, 6, 24, 6, 59),
11:59pm Pacific on 2026-07-23), so nothing depends on the host clock or zone.

the rendered deadline names its time zone, never a bare wall-clock time sweeps
five zones and, per zone, asserts the render is not the bare
dateStyle/timeStyle stamp, that it contains the zone token the runtime itself
names, and that it starts with that same stamp (the zone is appended, wording
unchanged). Comparing against the runtime's own token keeps it locale
independent. Verified on Node 24 (master's formatter, same instant):

America/Los_Angeles Jul 23, 2026, 11:59 PM PDT
UTC Jul 24, 2026, 6:59 AM UTC
Europe/London Jul 24, 2026, 7:59 AM GMT+1
Asia/Kolkata Jul 24, 2026, 12:29 PM GMT+5:30
Australia/Sydney Jul 24, 2026, 4:59 PM GMT+10

Zones with no common abbreviation fall back to a GMT offset, which still answers
the question.

the rendered deadline reads as a full local date, time and zone in a pinned zone pins the exact strings for the Pacific and UTC renders, which is the
crispest statement of the property. The formatter renders in the host locale by
design, so that case skips itself off an English host; the cross-zone case above
covers every locale.

For context on the constraint both the issue and #419 hit:
{ dateStyle: 'medium', timeStyle: 'short', timeZoneName: 'short' } throws
TypeError: Invalid option : option. #419 resolved it with a second
Intl.DateTimeFormat whose timeZoneName part is appended, degrading to the
bare stamp on a runtime that names no zone.

Checks

Local run (CI on this PR is the authority, not this):

  • npm run typecheck clean.
  • npm test: 2770 pass, 8 fail. Those 8 are all in
    test/core/leave-command.test.js (leave after join removes the seed and reports the server, leave clears an applied central slot..., and six more
    leave ... cases). They fail identically on clean origin/master with no
    changes applied
    , so they are pre-existing and unrelated to this PR. Flagging
    them so the reviewer is not surprised.

How to dispose of this

Merge to land the guard, or close#416 as already-fixed-by-#419 if you would
rather not carry the test. Either is a defensible call: the user-visible defect
is gone on master today.

Fixes#416

neutral-loopand others added 2 commits July 28, 2026 18:39
The rendering defect reported in #416 was already fixed by #419 (0253fcc):
formatFirstSyncDeadline appends the zone from a second Intl formatter, since
ECMA-402 forbids mixing timeZoneName with dateStyle/timeStyle. What the issue
also asked for is missing, and the formatter had no test coverage at all.
This adds that regression guard, so the four consent surfaces that render the
deadline (LLP 0100 R1 and R9, LLP 0101) cannot silently regress to a bare
wall-clock time. Test-only: no production code changes.
The assertions are pinned to explicit zones rather than the host's, and the
cross-zone case compares against the zone token the runtime itself names, so it
holds in any locale. The exact-string case is skipped off an English host.
Reverted to the pre-#419 formatter body, both cases fail with
'Jul 23, 2026, 11:59 PM' where 'Jul 23, 2026, 11:59 PM PDT' is expected.
Co-Authored-By: Claude <noreply@anthropic.com>
…nglish
The pinned expectation 'Jul 23, 2026, 11:59 PM PDT' is en-US-specific
formatting. Other English locales (en-GB, en-CA, en-AU) passed the old
locale.startsWith('en') guard and then failed the assertion. Tighten
the guard to the exact locale the strings encode.
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

🤖 neutral: review round 1 — ecfcf4a7

Verdict: findings (1 non-blocking, 0 blocking). Fixed and pushed.

Reviewed with code-review (no codex on this host, so no second-family review this round). Test-only diff, 67 lines, no production code.

The guard has teeth (the check that mattered)

The central risk in a test-only PR is an assertion that can never fail. It was verified empirically, not read: reverting formatFirstSyncDeadline to its pre-#419 body ({ dateStyle: 'medium', timeStyle: 'short' }) in a scratch worktree makes the new guard fail with exactly the reported symptom, and pass again on the real head.

not ok 6 - the rendered deadline names its time zone, never a bare wall-clock time (LLP 0100 R1)
error: 'America/Los_Angeles: the deadline must not render as a bare local time'
not ok 7 - + 'Jul 23, 2026, 11:59 PM' - 'Jul 23, 2026, 11:59 PM PDT'
# tests 15 # pass 13 # fail 2

Unmodified head: # tests 15 / # pass 15 / # fail 0. The reproduction claim in the PR body is accurate and reproducible.

Finding 1 — non-blocking — test/core/first-sync-hold.test.js:138

The locale self-skip was locale.startsWith('en'), but the pinned expectation 'Jul 23, 2026, 11:59 PM PDT' is en-US-specific. Every other English locale passed the guard and then failed the assertion:

LC_ALL=en-GB + '23 Jul 2026, 23:59 GMT-7' - 'Jul 23, 2026, 11:59 PM PDT'
LC_ALL=en-CA + 'Jul 23, 2026, 11:59 p.m. PDT' - 'Jul 23, 2026, 11:59 PM PDT'
LC_ALL=en-AU + '23 July 2026, 11:59 pm GMT-7' - 'Jul 23, 2026, 11:59 PM PDT'

A false red, never a false green (the zone-token sweep above it is locale-independent, and CI resolves to en-US), but it would have burned anyone on a UK/CA/AU/IN-locale machine, and it contradicted the PR body's own no-host-locale-dependence claim.

Fixed in 6d4e522: the guard is now locale !== 'en-US'. Verified both directions rather than assumed — under LC_ALL=en-GB the case skips cleanly (# pass 14 / # fail 0 / # skipped 1) while the locale-independent sweep still passes, and under LC_ALL=en-US it still runs and passes (# pass 15 / # skipped 0), so the guard did not become dead.

Checked and clean

  • Determinism: fixed instant (Date.UTC(2026, 6, 24, 6, 59)), never Date.now(). withTimeZone restores process.env.TZ in a finally and correctly deletes when it was previously unset rather than writing the string "undefined". Node runs each test file in its own process, so no cross-file TZ contamination.
  • Non-rotting expectations: the sweep compares against the token the runtime itself names via formatToParts, not a hardcoded zone list, so tzdata changes cannot rot it. None of the three assertions per zone is vacuous.
  • @ref LLP 0100#requirements [tests] (line 117): anchor exists (llp/0100-…spec.md:101), attached with no blank line, and the gloss is accurate. All four consent surfaces really do render through this one formatter, confirmed at remote_commands.js:224, status.js:385, wizard/index.js:329, and sync.js:110/:370.
  • Conventions: no semicolons, no U+2014 anywhere in the diff, JSDoc-only types, style matches sibling suites.

Note for the reviewer

The 8 leave … failures flagged in the PR body are environmental, not pre-existing-broken: CI is green at both heads (test and typecheck on Node 22 and 24). They reproduce only in this container.

Head has moved to 6d4e522, so neutral re-reviews the new head on its next tick.

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

🤖 neutral: review round 2 (final) - 6d4e5225

Verdict: clean. 0 findings, nothing pushed. Round 1's fix landed correctly and did not create a new problem.

Reviewed the delta ecfcf4a7..6d4e5225 plus a fresh skeptical pass over the whole 67-line diff. This is the final round the reconciler allows, so everything below is the closing record.

The round-1 fix did not kill the case it guarded

The whole delta is two lines: locale.startsWith('en') becomes locale !== 'en-US'. The obvious way that goes wrong is the pinned-string case going silently dead on CI. It did not. CI's own TAP output at this head, both matrix legs, shows it running and passing, not skipped:

test (22) ok 690 - the rendered deadline reads as a full local date, time and zone in a pinned zone
test (24) ✔ the rendered deadline reads as a full local date, time and zone in a pinned zone (0.971594ms)

(gh run view 30389050749 --log / 30389045003. A skipped test would carry a # SKIP suffix on the ok line; grepping the range for SKIP returns nothing.)

Why it survives: GitHub's ubuntu runners do not set a locale, and Node's ICU fallback resolves en-US in exactly that case. Verified rather than assumed:

env LANG=undefined LC_ALL=undefined -> en-US
LANG=C.UTF-8 (the runner's value) -> en-US
no LANG/LC_ALL/LANGUAGE at all -> en-US

The fix works in both directions, across the locales that were red

Full locale sweep at this head. The three locales round 1 caught as false reds are now clean skips, and en-US plus the CI value still run all 15:

en-US # pass 15 # fail 0 # skipped 0
en-GB # pass 14 # fail 0 # skipped 1
en-CA # pass 14 # fail 0 # skipped 1
en-AU # pass 14 # fail 0 # skipped 1
en-IN # pass 14 # fail 0 # skipped 1
fr-FR # pass 14 # fail 0 # skipped 1
de-DE # pass 14 # fail 0 # skipped 1
ja-JP # pass 14 # fail 0 # skipped 1
C.UTF-8 # pass 15 # fail 0 # skipped 0

The guard still has teeth at the new head, including off en-US

Round 1 proved teeth at ecfcf4a7. The guard changed since, so it was re-proved here rather than carried over. Reverting formatFirstSyncDeadline to its pre-#419 body in a scratch worktree:

not ok 6 - the rendered deadline names its time zone, never a bare wall-clock time (LLP 0100 R1)
error: 'America/Los_Angeles: the deadline must not render as a bare local time'
not ok 7 - + 'Jul 23, 2026, 11:59 PM' - 'Jul 23, 2026, 11:59 PM PDT'
# tests 15 # pass 13 # fail 2

And the more interesting question the tightened guard raises: does a non-en-US host still catch the regression at all, or does it now sail through green? It catches it. Same broken formatter, non-en-US locales, the pinned case skipping:

en-GB # pass 13 # fail 1 # skipped 1
fr-FR # pass 13 # fail 1 # skipped 1
ja-JP # pass 13 # fail 1 # skipped 1

The locale-independent zone-token sweep carries the regression coverage everywhere; the pinned case is a shape assertion layered on top of it, not the only thing standing between #416 and a green suite. Restored to # pass 15 # fail 0 after each experiment (git status --porcelain empty).

Fresh pass: what round 2 checked that round 1 did not

  • delete process.env.TZ really restores the clock, not just the env var. Round 1 checked the env var bookkeeping. The sharper question is whether V8's cached default zone follows a delete, because if it did not, every test after the render block would run in Australia/Sydney. It follows:
    before: Fri Jul 24 2026 06:59:00 GMT+0000 (Coordinated Universal Time)
    during: Fri Jul 24 2026 16:59:00 GMT+1000 (Australian Eastern Standard Time)
    after : Fri Jul 24 2026 06:59:00 GMT+0000 (Coordinated Universal Time)
    RESTORED? true
    
    The other branch (host already has TZ) is exercised too: TZ=Europe/Berlin and TZ=Pacific/Auckland both give # pass 15 # fail 0, so the zone-sensitive computeFirstSyncDeadline tests below the new block are unaffected.
  • Cross-file contamination.scripts/run-tests.js shells out to a single node --test <files>, which runs each file in its own child process, and this is the only file in test/ that touches env.TZ. No other suite can observe the mutation.
  • npm run typecheck at this head: exit 0.test/ is inside tsconfig.json's include, so the new @template T JSDoc on withTimeZone is actually checked, not decorative.
  • Full npm test: # tests 2779 / # pass 2770 / # fail 8 / # skipped 1. The 8 are the known environmental leave ... failures (CI is green at this head). The 1 skip is a pre-existing unrelated ZSTD-availability skip (ok 705 - resolveEncodeSettings falls back to SNAPPY ... # SKIP), not the new case.
  • Conventions. The file is pure ASCII (grep -P '[^\x00-\x7F]' returns nothing), so no U+2014 anywhere; no statement-terminating semicolons in the diff; JSDoc types only, no @typedef, no inline import('...'). No lint config in the repo, and the longest new line (136 chars) is well inside the existing test/ norm (max 320).
  • @ref LLP 0100#requirements [tests] (line 117) still resolves: llp/0100-enrollment-privacy-review.spec.md:101 is ## Requirements {#requirements}, and R1 does say the deadline prints "as an absolute local time with its time zone". Attached with no blank line. Still accurate at this head.
  • Delta hygiene.6d4e5225 touches only test/core/first-sync-hold.test.js, 2 insertions / 2 deletions. Nothing else rode along.

One residual note (not a finding, nothing to fix here)

The @ref gloss says one guard covers four consent surfaces. That is true of this tree: remote_commands.js:224, status.js:385, sync.js:110/:370, and wizard/index.js:329 all render through formatFirstSyncDeadline. But only two of the four have a test pinning that coupling (status-first-sync-hold.test.js:78 and remote-login-command.test.js:889 assert the surface text contains formatFirstSyncDeadline(deadline)). sync-command.test.js:167/:247 match only the prefix, and the wizard line has no test at all. So a future change that made sync or the wizard format its own deadline would escape this guard.

That is a pre-existing coverage shape, not something this PR introduced or regressed, and closing it is out of scope for a test-only fix for #416. Recording it so it is not rediscovered as a surprise.

Nothing left unresolved

No findings were opened this round, so nothing is carried forward for triage. Round 1's single finding is verified fixed at this head by the evidence above.

@philcunliffephilcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Jul 28, 2026
@philcunliffe
philcunliffe merged commit 482bcea into masterJul 28, 2026
8 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-416 branch July 28, 2026 19:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approvedneutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First-sync deadline prints a bare local time with no time zone

1 participant

@philcunliffe