Skip to content

Commit 64baa68

Browse files
os-steveclaude
andauthored
fix(spec): dist-freshness refusal derives package label and build remedy from pkgDir (#11471)
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/<name> 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 Claude-Session: https://claude.ai/code/session_01T9cDbY2NBiVJWYx3BpWfH2 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 466b389 commit 64baa68

3 files changed

Lines changed: 138 additions & 10 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@objectstack/spec': patch
3+
---
4+
5+
fix(spec): the dist-freshness refusal derives its package label and build remedy from `pkgDir` (#11250)
6+
7+
`inspectDistFreshness()` / `inspectBundleFreshness()` in `packages/spec/scripts/lib/dist-freshness.ts`
8+
already take an arbitrary `pkgDir`, but their refusal `cause` strings and the `pnpm --filter <pkg> build`
9+
remedy line hardcoded `packages/spec` / `@objectstack/spec` regardless of it. Every real caller passed
10+
`SPEC_DIR` until #10969 gave `check:skill-examples` a second surface (`packages/client-react` /
11+
`packages/client`) — confirmed live: a stale-dist refusal on that surface named `packages/spec` while
12+
`packages/spec` was freshly built and `client`/`client-react` were the actually-unbuilt packages, so
13+
following the printed remedy verbatim rebuilt an already-fresh package and re-red identically.
14+
15+
Both cause strings now interpolate a `packages/<name>` label derived from `pkgDir`'s own path (falling
16+
back to the raw path when the shape doesn't match), and the build-remedy line now reads
17+
`package.json#name` from `pkgDir` (falling back to the same label when it's missing or unparsable). The
18+
freshness verdict itself, and every caller's own `rerun` argument, are unchanged — this is diagnostic
19+
text only.

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

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import os from 'node:os';
3131
import path from 'node:path';
3232
import { fileURLToPath } from 'node:url';
3333

34-
import { inspectDistFreshness } from './lib/dist-freshness';
34+
import { inspectDistFreshness, packageDirLabel } from './lib/dist-freshness';
3535

3636
const HERE = path.dirname(fileURLToPath(import.meta.url));
3737
const PKG = path.resolve(HERE, '..');
@@ -84,7 +84,12 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e
8484
expect(verdict.fresh).toBe(false);
8585
if (verdict.fresh) return;
8686
expect(verdict.state).toBe('stale');
87-
expect(verdict.message).toContain('OLDER than packages/spec/src');
87+
// The label is DERIVED from `sandbox`, not a hardcoded `packages/spec` —
88+
// `sandbox` is a bare tmpdir with no `packages` path segment, so this
89+
// exercises the fallback-to-raw-path branch of `packageDirLabel` (#11250).
90+
// The `packages/<name>`-shaped branch is covered by its own case below.
91+
expect(verdict.message).toContain(`OLDER than ${packageDirLabel(sandbox)}/src`);
92+
expect(verdict.message).not.toContain('packages/spec');
8893
});
8994

9095
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
107112
});
108113

109114
it('names the writing damage in generate mode, and prescribes the build', () => {
115+
// The build-remedy line now reads `package.json#name` (#11250) rather than
116+
// hardcoding `@objectstack/spec`, so THIS case seeds one — matching
117+
// `sandbox`'s own docblock, "a throwaway `packages/spec`-SHAPED directory" —
118+
// to keep asserting the spec-shaped remedy line. The non-spec-shaped case
119+
// below is what proves that name is genuinely read, not assumed.
120+
write('package.json', JSON.stringify({ name: '@objectstack/spec' }), NEW);
110121
write('dist/contracts/index.d.ts', 'export {};', OLD);
111122
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
112123

@@ -118,6 +129,45 @@ describe('inspectDistFreshness — the dist precondition gen:api-surface never e
118129
expect(verdict.message).toContain('gen:api-surface');
119130
});
120131

132+
it('derives BOTH the cause label and the build-remedy package name from a NON-spec-shaped pkgDir (#11250)', () => {
133+
// Every case above runs against `sandbox` directly, which (bare tmpdir, no
134+
// `packages` path segment) already proves the label is not hardcoded — but
135+
// it never exercises the `packages/<name>` MATCHING branch of
136+
// `packageDirLabel`, and none of them give `pkgDir` a `package.json` naming
137+
// anything other than `@objectstack/spec`. So a build that quietly special-
138+
// cased "packages/spec" back in, or that only ever prints the one name
139+
// every other case's fixture happens to share, would still pass all of
140+
// them. This case is shaped like neither: a real `packages/widgets` path
141+
// segment, with its OWN unrelated `package.json#name`.
142+
const pkgDir = path.join(sandbox, 'packages', 'widgets');
143+
write('packages/widgets/package.json', JSON.stringify({ name: '@acme/widgets' }), NEW);
144+
write('packages/widgets/dist/index.d.ts', 'export {};', OLD);
145+
write('packages/widgets/src/index.ts', 'export const live = 1;', NEW);
146+
147+
const verdict = inspectDistFreshness(pkgDir, 'generate', 'pnpm --filter @acme/widgets check:something');
148+
expect(verdict.fresh).toBe(false);
149+
if (verdict.fresh) return;
150+
expect(verdict.state).toBe('stale');
151+
expect(verdict.message).toContain('OLDER than packages/widgets/src');
152+
expect(verdict.message).toContain('pnpm --filter @acme/widgets build');
153+
expect(verdict.message).not.toContain('packages/spec');
154+
expect(verdict.message).not.toContain('@objectstack/spec');
155+
});
156+
157+
it('falls back to the raw pkgDir for the build-remedy name when package.json is unreadable', () => {
158+
// `packageName`'s OTHER branch (#11250): a `pkgDir` with no `package.json`
159+
// at all (or one that fails to parse) must not throw out of a diagnostic
160+
// path. `sandbox` itself has none, so this reuses it directly rather than
161+
// constructing a second fixture.
162+
write('dist/contracts/index.d.ts', 'export {};', OLD);
163+
write('src/contracts/job-service.ts', 'export interface JobRunOutcome { ok: boolean }', NEW);
164+
165+
const verdict = inspectDistFreshness(sandbox, 'generate', GEN_RERUN);
166+
expect(verdict.fresh).toBe(false);
167+
if (verdict.fresh) return;
168+
expect(verdict.message).toContain(`pnpm --filter ${packageDirLabel(sandbox)} build`);
169+
});
170+
121171
it('reads a MISSING dist as its own condition, not as staleness', () => {
122172
// A missing dist and a stale one are different facts with different fixes to
123173
// 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
130180
expect(verdict.fresh).toBe(false);
131181
if (verdict.fresh) return;
132182
expect(verdict.state).toBe('missing');
133-
expect(verdict.message).toContain('no .d.ts declarations');
183+
// Same fallback branch as above: `sandbox` has no `packages` segment, so
184+
// the label in the cause string is the raw sandbox path, not `packages/spec`.
185+
expect(verdict.message).toContain(`${packageDirLabel(sandbox)}/dist holds no .d.ts declarations`);
186+
expect(verdict.message).not.toContain('packages/spec');
134187
});
135188

136189
it('reads a JS-only dist as missing — the OS_SKIP_DTS=1 shape on a virgin tree', () => {

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

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,68 @@
9494
* command string rather than an npm script name — that path is not reachable
9595
* through one.
9696
*/
97-
import { existsSync, readdirSync } from 'node:fs';
97+
import { existsSync, readdirSync, readFileSync } from 'node:fs';
9898
import { join } from 'node:path';
9999

100100
import { bundlesAreStale, distIsStale } from '../../../../scripts/check-regen-pending.mjs';
101101

102102
/** What the generator is about to do, so the refusal can name the real damage. */
103103
export type DistReadMode = 'generate' | 'check';
104104

105+
/**
106+
* A `packages/<name>` display label for `pkgDir`, derived from the path
107+
* itself rather than assumed — the refusal text used to hardcode
108+
* `packages/spec` unconditionally, which misnamed the stale package for
109+
* every non-spec caller (#11250; `check:skill-examples` became the first
110+
* real one, adopting this for `packages/client` / `packages/client-react`
111+
* in #10969).
112+
*
113+
* Finds the LAST `packages` path segment and takes the one right after it.
114+
* Every real caller today passes an absolute path with a single `packages`
115+
* segment, so "last" and "first" coincide; "last" is chosen so a
116+
* hypothetical path with an ancestor directory also named `packages`
117+
* (`.../packages/foo/packages/bar`) still names the package itself (`bar`)
118+
* rather than the misleading earlier match.
119+
*
120+
* Falls back to the raw `pkgDir` when the shape doesn't match at all (no
121+
* `packages` segment, or nothing after it) — a raw path is uglier than a
122+
* clean label, but it is still the TRUE location, whereas a hardcoded
123+
* `packages/spec` from a `pkgDir` this rule failed to parse would be a wrong
124+
* one dressed as a right one.
125+
*/
126+
export function packageDirLabel(pkgDir: string): string {
127+
const segments = pkgDir.split(/[\\/]+/).filter(Boolean);
128+
const idx = segments.lastIndexOf('packages');
129+
if (idx === -1 || idx === segments.length - 1) return pkgDir;
130+
return `packages/${segments[idx + 1]}`;
131+
}
132+
133+
/**
134+
* The package's own declared `package.json#name` — what `pnpm --filter`
135+
* actually resolves against, which is why the build-remedy line needs THIS
136+
* and not `packageDirLabel` above (a directory label like `packages/client`
137+
* is not a valid `--filter` argument; the published name is
138+
* `@objectstack/client`).
139+
*
140+
* Read defensively: a missing or unparsable `package.json` falls back to the
141+
* directory label. That fallback command will not resolve either, but it is
142+
* no worse than the pre-fix behaviour (which unconditionally printed
143+
* `@objectstack/spec` regardless of `pkgDir`) and it stays legible rather
144+
* than throwing out of a diagnostic path.
145+
*/
146+
function packageName(pkgDir: string): string {
147+
try {
148+
const parsed = JSON.parse(readFileSync(join(pkgDir, 'package.json'), 'utf8')) as {
149+
name?: unknown;
150+
};
151+
if (typeof parsed.name === 'string' && parsed.name.length > 0) return parsed.name;
152+
} catch {
153+
// Falls through to the directory-label fallback below — unreadable or
154+
// unparsable package.json is not fatal to a diagnostic message.
155+
}
156+
return packageDirLabel(pkgDir);
157+
}
158+
105159
export type DistFreshness =
106160
| { fresh: true }
107161
| { fresh: false; state: 'missing' | 'stale'; message: string };
@@ -164,11 +218,12 @@ export function inspectDistFreshness(
164218
` check would reach its conclusion without ever reading the declarations under test —\n` +
165219
` a FALSE GREEN on exactly the change it exists to catch (#7122).`;
166220

221+
const label = packageDirLabel(pkgDir);
167222
const cause =
168223
state === 'missing'
169-
? `packages/spec/dist holds no .d.ts declarations — the package is not built (or was built\n` +
224+
? `${label}/dist holds no .d.ts declarations — the package is not built (or was built\n` +
170225
` with OS_SKIP_DTS=1, which emits JS and skips exactly the artifact this reads).`
171-
: `packages/spec/dist/**/*.d.ts is OLDER than packages/spec/src — the declarations on disk\n` +
226+
: `${label}/dist/**/*.d.ts is OLDER than ${label}/src — the declarations on disk\n` +
172227
` predate the sources. If you built with OS_SKIP_DTS=1, that build did not rebuild them.`;
173228

174229
return {
@@ -178,7 +233,7 @@ export function inspectDistFreshness(
178233
`\n❌ ${cause}\n\n` +
179234
` ${damage}\n\n` +
180235
` Build first, then re-run:\n\n` +
181-
` pnpm --filter @objectstack/spec build\n` +
236+
` pnpm --filter ${packageName(pkgDir)} build\n` +
182237
` ${rerun}\n\n` +
183238
` (Do NOT use OS_SKIP_DTS=1 for this one — AGENTS.md §9 names it as the flag that emits JS\n` +
184239
` and skips exactly the declarations this reads.)`,
@@ -239,11 +294,12 @@ export function inspectBundleFreshness(
239294
` check would reach its conclusion without ever reading the module graph under test —\n` +
240295
` NOT MEASURED reported as if it were measured and clean (#4690).`;
241296

297+
const label = packageDirLabel(pkgDir);
242298
const cause =
243299
state === 'missing'
244-
? `packages/spec/dist holds no .mjs/.js bundles — the package is not built. This gate reads\n` +
300+
? `${label}/dist holds no .mjs/.js bundles — the package is not built. This gate reads\n` +
245301
` the module a consumer's import actually loads, so there is nothing here to read.`
246-
: `packages/spec/dist's .mjs/.js bundles are OLDER than packages/spec/src (or than\n` +
302+
: `${label}/dist's .mjs/.js bundles are OLDER than ${label}/src (or than\n` +
247303
` tsup.config.ts, which decides the entries, the externals and whether entries are\n` +
248304
` self-contained). The bundles on disk predate the sources.`;
249305

@@ -254,7 +310,7 @@ export function inspectBundleFreshness(
254310
`\n❌ ${cause}\n\n` +
255311
` ${damage}\n\n` +
256312
` Build first, then re-run:\n\n` +
257-
` pnpm --filter @objectstack/spec build\n` +
313+
` pnpm --filter ${packageName(pkgDir)} build\n` +
258314
` ${rerun}\n\n` +
259315
` (OS_SKIP_DTS=1 is fine for THIS gate — it still emits every bundle this reads. It is\n` +
260316
` the .d.ts-reading gates next door that it blinds.)`,

0 commit comments

Comments
 (0)