Skip to content

Commit 89cf4d6

Browse files
huangyiireneclaude
andauthored
fix(spec): the dist freshness rule can be answered — a declaration stamp acquits a tree whose sources were re-checked-out unchanged (#16176)
* wip: declaration stamp acquits the mtime rule * test: pin the acquittal and the digest that drives it * test: pin the declaration stamp's OS_SKIP_DTS behaviour in the self-test * fix: name all three declaration-stamp states honestly in the refusal * chore: changeset for the declaration-stamp acquittal * refactor: extract the build-input digest and its two stamps into their own module --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0c5d035 commit 89cf4d6

8 files changed

Lines changed: 560 additions & 132 deletions
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/spec": patch
3+
---
4+
5+
`check:api-surface` (and every other gate that reads `packages/spec/dist`) no longer refuses a dist that is exactly current because a source file's mtime moved without its bytes changing.
6+
7+
The freshness rule shared by four gates and the pre-commit hook compares `dist/**/*.d.ts` mtimes against `src/**/*.ts` mtimes. That is the right primitive — it is the artifact those gates consume, and it sees the hand-edited dist and the toolchain change no content digest can — but it cannot tell a real edit from a rewrite that left the bytes alone. A `git merge` re-checks-out an unchanged source file and bumps its mtime; the build that follows correctly does not run, because turbo's cache hashes content, so it is a cache hit that rewrites nothing and leaves every `dist/` mtime where the previous build left it. The gate then refused a correct dist, and prescribed a full rebuild — minutes, under the shared verify lock — of an artifact that needed none.
8+
9+
The mtime rule keeps its power to convict and gains one way to be answered. `packages/spec`'s build now records a second stamp beside the existing one, `dist/.build-input-hash-dts`, holding the same build-input digest — but written **only** by a build that actually emitted declarations, so `OS_SKIP_DTS=1` leaves it alone. When that digest equals the sources on disk, the declarations demonstrably describe them and the refusal is cleared. The evidence may only ever **acquit**: a missing, unreadable or mismatched stamp leaves the mtime verdict standing, so nothing that passed before can start failing, and the `OS_SKIP_DTS=1`-on-a-built-tree shape that ruled out `dist/.build-input-hash` for this purpose still fails, because that build never refreshes the new file.
10+
11+
The refusal message was wrong in the same case and is now driven by what was measured: it names a real content change and prints both digests when the stamp disagrees, says plainly that there is nothing to compare against when no stamp exists, and no longer sends every reader after `OS_SKIP_DTS` regardless of cause. It also notes that a repo-wide `pnpm build` may be a cache hit that rewrites nothing, so the remedy names the package build directly.
12+
13+
The published tarball gains one 65-byte file next to the stamp it already shipped.

packages/spec/scripts/build-api-surface.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,10 @@
4848
* older than `src/`. On a stale dist this script does not fail, it writes a
4949
* baseline missing every export added since the build — and `--check` then
5050
* agrees with it against the same stale dist, so the phantom breaking removal is
51-
* green at every step. See lib/dist-freshness.ts for the mechanism and for why
52-
* the mtime rule, not `dist/.build-input-hash`, is the primitive that covers it.
51+
* green at every step. See lib/dist-freshness.ts for the mechanism, for why the
52+
* mtime rule — not `dist/.build-input-hash` — is the primitive that convicts,
53+
* and for the sibling stamp (`dist/.build-input-hash-dts`) that may acquit a
54+
* tree whose sources were re-checked-out unchanged.
5355
*/
5456
import ts from 'typescript';
5557
import { createHash } from 'node:crypto';

packages/spec/scripts/dist-freshness.test.ts

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import path from 'node:path';
3232
import { fileURLToPath } from 'node:url';
3333

3434
import { inspectDistFreshness, packageDirLabel } from './lib/dist-freshness';
35+
import { declarationStamp } from '../../../scripts/check-regen-pending.mjs';
3536

3637
const HERE = path.dirname(fileURLToPath(import.meta.url));
3738
const PKG = path.resolve(HERE, '..');
@@ -56,6 +57,27 @@ function write(rel: string, content: string, mtimeEpochSeconds: number): string
5657
const OLD = Math.floor(Date.now() / 1000) - 3600;
5758
const NEW = Math.floor(Date.now() / 1000) - 60;
5859

60+
/**
61+
* The digest `--stamp` would write for `sandbox` as it stands right now.
62+
*
63+
* Asked of the rule's own reader rather than hardcoded, deliberately: the input
64+
* set includes turbo.json's `globalDependencies` and the repo-relative path of
65+
* every file, so a literal here would be a fixture that rots on the next
66+
* unrelated edit to either — and a rotted literal fails as `mismatch`, which
67+
* reads exactly like the refusal these cases are trying to distinguish from.
68+
*
69+
* Two steps, because `declarationStamp` computes `actual` only when there is a
70+
* recorded digest to compare it against (the ~30ms hash stays off the path where
71+
* no stamp exists): seed a syntactically valid placeholder, read what the
72+
* sources really hash to, then let the caller write that.
73+
*/
74+
function currentDigest(): string {
75+
write('dist/.build-input-hash-dts', `${'0'.repeat(64)}\n`, OLD);
76+
const { actual } = declarationStamp(sandbox);
77+
if (!actual) throw new Error('the sandbox digest could not be computed — the fixture is wrong');
78+
return actual;
79+
}
80+
5981
// The caller's own re-run command. Passed rather than assumed since #7181: the
6082
// wording used to name `api-surface` by hand, so the three gates that adopted
6183
// this next each printed a fourth gate's name. `dist-freshness-adoption.test.ts`
@@ -210,17 +232,87 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e
210232
//
211233
// Fresh JS, fresh stamp, stale declarations: a stamp-based guard is GREEN
212234
// here. This one is red.
235+
//
236+
// Since #14985 the guard DOES consult a digest, so the fixture writes the
237+
// REAL one rather than a placeholder: `dist/.build-input-hash` holding the
238+
// exact hash of these sources is the strongest possible form of the shape,
239+
// and it must still be refused. What acquits is the sibling file an
240+
// OS_SKIP_DTS=1 build never writes, and this fixture deliberately has none.
213241
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
214242
write('dist/contracts/index.d.ts', 'export {};', OLD);
215243
write('dist/contracts/index.js', 'export {};', NEW);
216-
write('dist/.build-input-hash', `${'a'.repeat(64)}\n`, NEW);
244+
write('dist/.build-input-hash', `${currentDigest()}\n`, NEW);
217245

218246
const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN);
219247
expect(verdict.fresh).toBe(false);
220248
if (verdict.fresh) return;
221249
expect(verdict.state).toBe('stale');
222250
});
223251

252+
it('ACQUITS an mtime-stale tree whose declaration stamp matches the sources (#14985)', () => {
253+
// The false refusal this card was filed for, in miniature. A `git merge`
254+
// re-checks-out an UNCHANGED `src/**` file — bytes identical, mtime bumped —
255+
// and the build that follows is a turbo cache hit that rewrites nothing, so
256+
// every `dist/` mtime stays where the previous build left it. The mtime rule
257+
// alone calls that stale and prescribes a multi-minute rebuild of a dist
258+
// that is already exactly right.
259+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
260+
write('dist/contracts/index.d.ts', 'export {};', OLD);
261+
write('dist/.build-input-hash-dts', `${currentDigest()}\n`, OLD);
262+
263+
expect(inspectDistFreshness(sandbox, 'generate', GEN_RERUN)).toEqual({ fresh: true });
264+
expect(inspectDistFreshness(sandbox, 'check', CHECK_RERUN)).toEqual({ fresh: true });
265+
});
266+
267+
it('and CONVICTS the same tree the moment a source byte actually changes', () => {
268+
// The half that makes the case above non-vacuous. If the acquittal were
269+
// keyed on the stamp's mere PRESENCE rather than on the digest, this would
270+
// stay green — and that is #7122's false green restored, one file over. The
271+
// stamp is written first and the source edited after, so the recorded digest
272+
// is genuinely stale rather than never-valid.
273+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
274+
write('dist/contracts/index.d.ts', 'export {};', OLD);
275+
write('dist/.build-input-hash-dts', `${currentDigest()}\n`, OLD);
276+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean; at: number }', NEW);
277+
278+
const verdict = inspectDistFreshness(sandbox, 'check', CHECK_RERUN);
279+
expect(verdict.fresh).toBe(false);
280+
if (verdict.fresh) return;
281+
expect(verdict.state).toBe('stale');
282+
// and it names the cause it actually measured, rather than sending the
283+
// reader after OS_SKIP_DTS — the wrong-cause half of #14985.
284+
expect(verdict.message).toContain('describe DIFFERENT sources');
285+
expect(verdict.message).toContain('now hashes to');
286+
expect(verdict.message).not.toContain('is absent or unreadable');
287+
});
288+
289+
it('reports a stale dist with NO declaration stamp as exactly that — no evidence, not a diagnosis', () => {
290+
// The state every tree built before this stamp existed is in, and the one an
291+
// OS_SKIP_DTS=1 build leaves behind. The mtime verdict stands, but the
292+
// message may not claim a content change it never measured: `unstamped` is
293+
// "cannot tell", and saying so is what stops the next reader spending a
294+
// round on the wrong cause.
295+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
296+
write('dist/contracts/index.d.ts', 'export {};', OLD);
297+
298+
const verdict = inspectDistFreshness(sandbox, 'check', CHECK_RERUN);
299+
expect(verdict.fresh).toBe(false);
300+
if (verdict.fresh) return;
301+
expect(verdict.message).toContain('is absent or unreadable');
302+
expect(verdict.message).not.toContain('describe DIFFERENT sources');
303+
});
304+
305+
it('ignores a declaration stamp that is not a digest at all', () => {
306+
// Absence of the freshness input is not licence to acquit (#4690), and
307+
// neither is a truncated write or a half-flushed file. Anything that is not
308+
// 64 hex characters is `unstamped`, which leaves the refusal standing.
309+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
310+
write('dist/contracts/index.d.ts', 'export {};', OLD);
311+
write('dist/.build-input-hash-dts', 'not-a-digest\n', OLD);
312+
313+
expect(inspectDistFreshness(sandbox, 'check', CHECK_RERUN).fresh).toBe(false);
314+
});
315+
224316
it('finds the newest source at ANY depth, not just the top level', () => {
225317
// A walk that stopped one level down would call this fresh and hand the
226318
// generator a dist that predates the only edit in the tree.

packages/spec/scripts/lib/dist-freshness.ts

Lines changed: 72 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -44,33 +44,49 @@
4444
* carries `schemaTreeIsStale` itself "so EVERY caller is covered rather than
4545
* this one". Same shape, same reason.
4646
*
47-
* ## Why the mtime rule and NOT `dist/.build-input-hash`
48-
*
49-
* #7122 suggested reusing the content stamp that `scripts/check-dev-prereqs.mjs`
50-
* writes. Measured, it is the wrong primitive FOR THIS CONSUMER, in the
51-
* dangerous direction:
52-
*
53-
* - The stamp is written by `packages/spec`'s build unconditionally, INCLUDING
54-
* under `OS_SKIP_DTS=1` — that build emits JS, leaves whatever `.d.ts` was
55-
* there before, and stamps anyway. check-dev-prereqs.mjs lists this as a
56-
* known FALSE GREEN and names this gate as the one it breaks; AGENTS.md
57-
* §"Added or removed a `packages/spec` export?" says the same in the other
58-
* direction ("skips exactly the artifact the gate inspects, and the check
59-
* passes locally while failing in CI"). A stamp-based guard would therefore
60-
* be green on the one local build flag that guarantees the declarations this
61-
* generator reads are stale.
47+
* ## Why the mtime rule CONVICTS, and what may acquit
48+
*
49+
* #7122 suggested REPLACING the mtime rule with the content stamp that
50+
* `scripts/check-dev-prereqs.mjs` writes. Measured, that swap is wrong FOR THIS
51+
* CONSUMER, in the dangerous direction, and it stays rejected:
52+
*
53+
* - `dist/.build-input-hash` is written by `packages/spec`'s build
54+
* unconditionally, INCLUDING under `OS_SKIP_DTS=1` — that build emits JS,
55+
* leaves whatever `.d.ts` was there before, and stamps anyway.
56+
* check-dev-prereqs.mjs lists this as a known FALSE GREEN and names this
57+
* gate as the one it breaks; AGENTS.md §"Added or removed a `packages/spec`
58+
* export?" says the same in the other direction ("skips exactly the artifact
59+
* the gate inspects, and the check passes locally while failing in CI").
6260
* - `distIsStale` keys on `dist/**` + `.d.ts` mtimes against `src/**` + `.ts`
6361
* — the artifact this generator actually consumes — so it catches the
64-
* `OS_SKIP_DTS=1` shape as well as #7122's rebase-behind-the-dist shape.
62+
* `OS_SKIP_DTS=1` shape as well as #7122's rebase-behind-the-dist shape,
63+
* and it sees the hand-edited dist and the toolchain change no digest can.
6564
* - It is also the rule the other three consumers already read, so this adds
6665
* no second notion of "is packages/spec/dist current"; a second copy drifts,
6766
* and the direction it drifts in is the one that writes a confident wrong
6867
* baseline (#4675).
6968
*
70-
* The stamp's own strength — content, not mtime (#5864) — is real and NOT
71-
* discarded: it still guards `pnpm dev`. It is simply blind to the half of the
72-
* dist that this generator is made of. Closing the `OS_SKIP_DTS` hole in the
73-
* stamp itself is a separate change to a separate script.
69+
* What mtimes cannot see is a rewrite that left the BYTES alone, and #14985
70+
* measured the cost of that blind spot. A `git merge` re-checks-out an unchanged
71+
* `src/**` file, bumping its mtime; the build that follows correctly does not
72+
* run (turbo's cache hashes content — measured `>>> FULL TURBO`, 56ms) and
73+
* rewrites nothing; every `dist/` mtime stays where the previous build left it,
74+
* and this guard refuses a dist that is exactly current. The remedy on offer was
75+
* a full rebuild, minutes long and under the shared verify lock, of an artifact
76+
* that needed none — and the refusal named `OS_SKIP_DTS` as the cause, which it
77+
* was not.
78+
*
79+
* So the mtime rule keeps its power to CONVICT and gains one way to be answered:
80+
* `dist/.build-input-hash-dts`, a sibling stamp written only by a build that
81+
* really emitted declarations (`--stamp` skips it under `OS_SKIP_DTS=1`). When
82+
* its digest equals the inputs on disk, the declarations demonstrably describe
83+
* these sources. That evidence may only ACQUIT — absent or unreadable leaves the
84+
* refusal standing — so the change cannot turn a pass into a refusal, and #7122's
85+
* shape still convicts because its stamp is the other file.
86+
*
87+
* The `dist/.build-input-hash` stamp's own strength — content, not mtime (#5864)
88+
* — is unchanged and still guards `pnpm dev`; it is simply not the file this
89+
* reads.
7490
*
7591
* ## Why the caller names ITSELF, and why that is not a third `mode` (#7181)
7692
*
@@ -97,7 +113,11 @@
97113
import { existsSync, readdirSync, readFileSync } from 'node:fs';
98114
import { join } from 'node:path';
99115

100-
import { bundlesAreStale, distIsStale } from '../../../../scripts/check-regen-pending.mjs';
116+
import {
117+
bundlesAreStale,
118+
declarationStamp,
119+
distIsStale,
120+
} from '../../../../scripts/check-regen-pending.mjs';
101121

102122
/** What the generator is about to do, so the refusal can name the real damage. */
103123
export type DistReadMode = 'generate' | 'check';
@@ -219,12 +239,38 @@ export function inspectDistFreshness(
219239
` a FALSE GREEN on exactly the change it exists to catch (#7122).`;
220240

221241
const label = packageDirLabel(pkgDir);
242+
// Which of the three stale causes this is. Recomputed rather than threaded out
243+
// of `distIsStale` because the boolean is the rule's whole contract and the
244+
// wording is this file's; the second digest costs ~30ms and is spent only on
245+
// the refusal path — the one that used to prescribe a multi-minute rebuild.
246+
const stamp = state === 'stale' ? declarationStamp(pkgDir) : { state: 'unstamped' as const };
247+
const digests =
248+
'recorded' in stamp && stamp.recorded && stamp.actual
249+
? `\n recorded ${stamp.recorded.slice(0, 16)}… · ${label}/src now hashes to ${stamp.actual.slice(0, 16)}…`
250+
: '';
222251
const cause =
223252
state === 'missing'
224253
? `${label}/dist holds no .d.ts declarations — the package is not built (or was built\n` +
225254
` with OS_SKIP_DTS=1, which emits JS and skips exactly the artifact this reads).`
226-
: `${label}/dist/**/*.d.ts is OLDER than ${label}/src — the declarations on disk\n` +
227-
` predate the sources. If you built with OS_SKIP_DTS=1, that build did not rebuild them.`;
255+
: stamp.state === 'mismatch'
256+
? `${label}/dist/**/*.d.ts describe DIFFERENT sources than the ones on disk. The last\n` +
257+
` build that emitted declarations recorded a build-input digest in\n` +
258+
` ${label}/dist/.build-input-hash-dts, and ${label}/src no longer hashes to it — so this\n` +
259+
` is a real content change, not a timestamp artefact.${digests}`
260+
: stamp.state === 'unstamped'
261+
? `${label}/dist/**/*.d.ts is OLDER than ${label}/src, and ${label}/dist/.build-input-hash-dts\n` +
262+
` is absent or unreadable, so there is nothing to check that timestamp against. Either\n` +
263+
` this dist predates that stamp, or the build that wrote it ran with OS_SKIP_DTS=1 and\n` +
264+
` never emitted declarations. One real build settles it and records the stamp, after\n` +
265+
` which a re-checkout that only bumps mtimes stops being refused.`
266+
: // `match` while still refusing: the freshness rule and this wording
267+
// read the tree at two different instants, so a build that landed
268+
// between them arrives here. Say only what was measured — claiming
269+
// a missing stamp that is right there is the wrong-cause defect
270+
// #14985 was filed for, one branch over.
271+
`${label}/dist/**/*.d.ts is OLDER than ${label}/src, while\n` +
272+
` ${label}/dist/.build-input-hash-dts matches those sources. The tree moved between the\n` +
273+
` two readings — most likely a build finished alongside this one. Re-run this check.`;
228274

229275
return {
230276
fresh: false,
@@ -235,8 +281,9 @@ export function inspectDistFreshness(
235281
` Build first, then re-run:\n\n` +
236282
` pnpm --filter ${packageName(pkgDir)} build\n` +
237283
` ${rerun}\n\n` +
238-
` (Do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as the flag that emits JS\n` +
239-
` and skips exactly the declarations this reads.)`,
284+
` (Build THAT package directly — a repo-wide \`pnpm build\` may be a turbo cache hit that\n` +
285+
` rewrites nothing. And do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as\n` +
286+
` the flag that emits JS and skips exactly the declarations this reads.)`,
240287
};
241288
}
242289

0 commit comments

Comments
 (0)