From ca8116159855bc936c3b443666bd73caa3653b6e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:59:35 +0000 Subject: [PATCH] fix(spec): dist-freshness refusal derives package label and build remedy from pkgDir inspectDistFreshness() / inspectBundleFreshness() hardcoded packages/spec in their refusal cause strings and the pnpm --filter @objectstack/spec build remedy line, regardless of pkgDir. Every real caller passed SPEC_DIR until #10969 gave check:skill-examples a second surface (packages/client-react / packages/client) -- confirmed live: a stale-dist refusal on that surface misnamed the stale package and printed a non-actionable remedy. Both cause strings now interpolate a packages/ label derived from pkgDir's own path, and the build-remedy line reads pkgDir/package.json#name -- both falling back sensibly when the shape doesn't match / the file is unreadable. dist-freshness.test.ts's pinned literals are updated to match, and two new cases prove the derivation is genuine: a non-spec-shaped pkgDir (packages/widgets, package.json name @acme/widgets), and the package.json- missing fallback. Fixes #11250 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01T9cDbY2NBiVJWYx3BpWfH2 --- ...freshness-refusal-derives-package-label.md | 19 +++++ packages/spec/scripts/dist-freshness.test.ts | 59 +++++++++++++++- packages/spec/scripts/lib/dist-freshness.ts | 70 +++++++++++++++++-- 3 files changed, 138 insertions(+), 10 deletions(-) create mode 100644 .changeset/dist-freshness-refusal-derives-package-label.md diff --git a/.changeset/dist-freshness-refusal-derives-package-label.md b/.changeset/dist-freshness-refusal-derives-package-label.md new file mode 100644 index 0000000000..c168523726 --- /dev/null +++ b/.changeset/dist-freshness-refusal-derives-package-label.md @@ -0,0 +1,19 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec): the dist-freshness refusal derives its package label and build remedy from `pkgDir` (#11250) + +`inspectDistFreshness()` / `inspectBundleFreshness()` in `packages/spec/scripts/lib/dist-freshness.ts` +already take an arbitrary `pkgDir`, but their refusal `cause` strings and the `pnpm --filter build` +remedy line hardcoded `packages/spec` / `@objectstack/spec` regardless of it. Every real caller passed +`SPEC_DIR` until #10969 gave `check:skill-examples` a second surface (`packages/client-react` / +`packages/client`) — confirmed live: a stale-dist refusal on that surface named `packages/spec` while +`packages/spec` was freshly built and `client`/`client-react` were the actually-unbuilt packages, so +following the printed remedy verbatim rebuilt an already-fresh package and re-red identically. + +Both cause strings now interpolate a `packages/` label derived from `pkgDir`'s own path (falling +back to the raw path when the shape doesn't match), and the build-remedy line now reads +`package.json#name` from `pkgDir` (falling back to the same label when it's missing or unparsable). The +freshness verdict itself, and every caller's own `rerun` argument, are unchanged — this is diagnostic +text only. diff --git a/packages/spec/scripts/dist-freshness.test.ts b/packages/spec/scripts/dist-freshness.test.ts index fef07de9f9..db7838fe11 100644 --- a/packages/spec/scripts/dist-freshness.test.ts +++ b/packages/spec/scripts/dist-freshness.test.ts @@ -31,7 +31,7 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { inspectDistFreshness } from './lib/dist-freshness'; +import { inspectDistFreshness, packageDirLabel } from './lib/dist-freshness'; const HERE = path.dirname(fileURLToPath(import.meta.url)); const PKG = path.resolve(HERE, '..'); @@ -84,7 +84,12 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('stale'); - expect(verdict.message).toContain('OLDER than packages/spec/src'); + // The label is DERIVED from `sandbox`, not a hardcoded `packages/spec` — + // `sandbox` is a bare tmpdir with no `packages` path segment, so this + // exercises the fallback-to-raw-path branch of `packageDirLabel` (#11250). + // The `packages/`-shaped branch is covered by its own case below. + expect(verdict.message).toContain(`OLDER than ${packageDirLabel(sandbox)}/src`); + expect(verdict.message).not.toContain('packages/spec'); }); it('refuses in --check mode too, so CI cannot pass against a stale dist either', () => { @@ -107,6 +112,12 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e }); it('names the writing damage in generate mode, and prescribes the build', () => { + // The build-remedy line now reads `package.json#name` (#11250) rather than + // hardcoding `@objectstack/spec`, so THIS case seeds one — matching + // `sandbox`'s own docblock, "a throwaway `packages/spec`-SHAPED directory" — + // to keep asserting the spec-shaped remedy line. The non-spec-shaped case + // below is what proves that name is genuinely read, not assumed. + write('package.json', JSON.stringify({ name: '@objectstack/spec' }), NEW); write('dist/contracts/index.d.ts', 'export {};', OLD); write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); @@ -118,6 +129,45 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e expect(verdict.message).toContain('gen:api-surface'); }); + it('derives BOTH the cause label and the build-remedy package name from a NON-spec-shaped pkgDir (#11250)', () => { + // Every case above runs against `sandbox` directly, which (bare tmpdir, no + // `packages` path segment) already proves the label is not hardcoded — but + // it never exercises the `packages/` MATCHING branch of + // `packageDirLabel`, and none of them give `pkgDir` a `package.json` naming + // anything other than `@objectstack/spec`. So a build that quietly special- + // cased "packages/spec" back in, or that only ever prints the one name + // every other case's fixture happens to share, would still pass all of + // them. This case is shaped like neither: a real `packages/widgets` path + // segment, with its OWN unrelated `package.json#name`. + const pkgDir = path.join(sandbox, 'packages', 'widgets'); + write('packages/widgets/package.json', JSON.stringify({ name: '@acme/widgets' }), NEW); + write('packages/widgets/dist/index.d.ts', 'export {};', OLD); + write('packages/widgets/src/index.ts', 'export const live = 1;', NEW); + + const verdict = inspectDistFreshness(pkgDir, 'generate', 'pnpm --filter @acme/widgets check:something'); + expect(verdict.fresh).toBe(false); + if (verdict.fresh) return; + expect(verdict.state).toBe('stale'); + expect(verdict.message).toContain('OLDER than packages/widgets/src'); + expect(verdict.message).toContain('pnpm --filter @acme/widgets build'); + expect(verdict.message).not.toContain('packages/spec'); + expect(verdict.message).not.toContain('@objectstack/spec'); + }); + + it('falls back to the raw pkgDir for the build-remedy name when package.json is unreadable', () => { + // `packageName`'s OTHER branch (#11250): a `pkgDir` with no `package.json` + // at all (or one that fails to parse) must not throw out of a diagnostic + // path. `sandbox` itself has none, so this reuses it directly rather than + // constructing a second fixture. + write('dist/contracts/index.d.ts', 'export {};', OLD); + write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW); + + const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN); + expect(verdict.fresh).toBe(false); + if (verdict.fresh) return; + expect(verdict.message).toContain(`pnpm --filter ${packageDirLabel(sandbox)} build`); + }); + it('reads a MISSING dist as its own condition, not as staleness', () => { // A missing dist and a stale one are different facts with different fixes to // suggest, and #7122's ruling asked for them to be distinguishable. A never @@ -130,7 +180,10 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e expect(verdict.fresh).toBe(false); if (verdict.fresh) return; expect(verdict.state).toBe('missing'); - expect(verdict.message).toContain('no .d.ts declarations'); + // Same fallback branch as above: `sandbox` has no `packages` segment, so + // the label in the cause string is the raw sandbox path, not `packages/spec`. + expect(verdict.message).toContain(`${packageDirLabel(sandbox)}/dist holds no .d.ts declarations`); + expect(verdict.message).not.toContain('packages/spec'); }); it('reads a JS-only dist as missing — the OS_SKIP_DTS=1 shape on a virgin tree', () => { diff --git a/packages/spec/scripts/lib/dist-freshness.ts b/packages/spec/scripts/lib/dist-freshness.ts index bae62118d1..e26d2b49c4 100644 --- a/packages/spec/scripts/lib/dist-freshness.ts +++ b/packages/spec/scripts/lib/dist-freshness.ts @@ -94,7 +94,7 @@ * command string rather than an npm script name — that path is not reachable * through one. */ -import { existsSync, readdirSync } from 'node:fs'; +import { existsSync, readdirSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { bundlesAreStale, distIsStale } from '../../../../scripts/check-regen-pending.mjs'; @@ -102,6 +102,60 @@ import { bundlesAreStale, distIsStale } from '../../../../scripts/check-regen-pe /** What the generator is about to do, so the refusal can name the real damage. */ export type DistReadMode = 'generate' | 'check'; +/** + * A `packages/` display label for `pkgDir`, derived from the path + * itself rather than assumed — the refusal text used to hardcode + * `packages/spec` unconditionally, which misnamed the stale package for + * every non-spec caller (#11250; `check:skill-examples` became the first + * real one, adopting this for `packages/client` / `packages/client-react` + * in #10969). + * + * Finds the LAST `packages` path segment and takes the one right after it. + * Every real caller today passes an absolute path with a single `packages` + * segment, so "last" and "first" coincide; "last" is chosen so a + * hypothetical path with an ancestor directory also named `packages` + * (`.../packages/foo/packages/bar`) still names the package itself (`bar`) + * rather than the misleading earlier match. + * + * Falls back to the raw `pkgDir` when the shape doesn't match at all (no + * `packages` segment, or nothing after it) — a raw path is uglier than a + * clean label, but it is still the TRUE location, whereas a hardcoded + * `packages/spec` from a `pkgDir` this rule failed to parse would be a wrong + * one dressed as a right one. + */ +export function packageDirLabel(pkgDir: string): string { + const segments = pkgDir.split(/[\\/]+/).filter(Boolean); + const idx = segments.lastIndexOf('packages'); + if (idx === -1 || idx === segments.length - 1) return pkgDir; + return `packages/${segments[idx + 1]}`; +} + +/** + * The package's own declared `package.json#name` — what `pnpm --filter` + * actually resolves against, which is why the build-remedy line needs THIS + * and not `packageDirLabel` above (a directory label like `packages/client` + * is not a valid `--filter` argument; the published name is + * `@objectstack/client`). + * + * Read defensively: a missing or unparsable `package.json` falls back to the + * directory label. That fallback command will not resolve either, but it is + * no worse than the pre-fix behaviour (which unconditionally printed + * `@objectstack/spec` regardless of `pkgDir`) and it stays legible rather + * than throwing out of a diagnostic path. + */ +function packageName(pkgDir: string): string { + try { + const parsed = JSON.parse(readFileSync(join(pkgDir, 'package.json'), 'utf8')) as { + name?: unknown; + }; + if (typeof parsed.name === 'string' && parsed.name.length > 0) return parsed.name; + } catch { + // Falls through to the directory-label fallback below — unreadable or + // unparsable package.json is not fatal to a diagnostic message. + } + return packageDirLabel(pkgDir); +} + export type DistFreshness = | { fresh: true } | { fresh: false; state: 'missing' | 'stale'; message: string }; @@ -164,11 +218,12 @@ export function inspectDistFreshness( ` check would reach its conclusion without ever reading the declarations under test —\n` + ` a FALSE GREEN on exactly the change it exists to catch (#7122).`; + const label = packageDirLabel(pkgDir); const cause = state === 'missing' - ? `packages/spec/dist holds no .d.ts declarations — the package is not built (or was built\n` + + ? `${label}/dist holds no .d.ts declarations — the package is not built (or was built\n` + ` with OS_SKIP_DTS=1, which emits JS and skips exactly the artifact this reads).` - : `packages/spec/dist/**/*.d.ts is OLDER than packages/spec/src — the declarations on disk\n` + + : `${label}/dist/**/*.d.ts is OLDER than ${label}/src — the declarations on disk\n` + ` predate the sources. If you built with OS_SKIP_DTS=1, that build did not rebuild them.`; return { @@ -178,7 +233,7 @@ export function inspectDistFreshness( `\n❌ ${cause}\n\n` + ` ${damage}\n\n` + ` Build first, then re-run:\n\n` + - ` pnpm --filter @objectstack/spec build\n` + + ` pnpm --filter ${packageName(pkgDir)} build\n` + ` ${rerun}\n\n` + ` (Do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as the flag that emits JS\n` + ` and skips exactly the declarations this reads.)`, @@ -239,11 +294,12 @@ export function inspectBundleFreshness( ` check would reach its conclusion without ever reading the module graph under test —\n` + ` NOT MEASURED reported as if it were measured and clean (#4690).`; + const label = packageDirLabel(pkgDir); const cause = state === 'missing' - ? `packages/spec/dist holds no .mjs/.js bundles — the package is not built. This gate reads\n` + + ? `${label}/dist holds no .mjs/.js bundles — the package is not built. This gate reads\n` + ` the module a consumer's import actually loads, so there is nothing here to read.` - : `packages/spec/dist's .mjs/.js bundles are OLDER than packages/spec/src (or than\n` + + : `${label}/dist's .mjs/.js bundles are OLDER than ${label}/src (or than\n` + ` tsup.config.ts, which decides the entries, the externals and whether entries are\n` + ` self-contained). The bundles on disk predate the sources.`; @@ -254,7 +310,7 @@ export function inspectBundleFreshness( `\n❌ ${cause}\n\n` + ` ${damage}\n\n` + ` Build first, then re-run:\n\n` + - ` pnpm --filter @objectstack/spec build\n` + + ` pnpm --filter ${packageName(pkgDir)} build\n` + ` ${rerun}\n\n` + ` (OS_SKIP_DTS=1 is fine for THIS gate — it still emits every bundle this reads. It is\n` + ` the .d.ts-reading gates next door that it blinds.)`,