Skip to content

Commit 1997df3

Browse files
committed
fix(devx): resolve a regen row's gen:/check: in its DECLARED owner, not only in packages/spec
`git-merge-regen.mjs --self-test` resolved every row's script names in `packages/spec/package.json` and nowhere else, so an artifact owned by ROOT tooling could not be registered for `merge=os-regen` at all — however exactly it matched the pathology the driver exists for. The refusal was correct about the tree and wrong about the world: it read as "you named a script that does not exist" when the truth was "this artifact is not owned by packages/spec", and an author following it literally moves root tooling into a package it does not belong to, purely to satisfy a lookup path. Rows now carry an `owner` (defaulting to @objectstack/spec, which is what all 13 declared implicitly), and the refusal names the manifests it searched. The owner is DECLARED rather than searched for, because a lookup-only widening would have left the worse half standing. Two consumers need to know WHICH manifest owns a row, not merely that some manifest has the name: the driver PRINTS a regeneration command and `check-regen-pending.mjs` SPAWNS one, and both were bound to `packages/spec`. A root-owned row under a widened lookup would have reconciled green and then been spawned in a directory that does not define its script — measured, `pnpm -s check:sdui-lockstep` exits 254 there and 0 at the repo root — leaving the artifact permanently stale and every commit refused. Registered-and-unreconcilable is a worse defect than unregisterable. `reconcileOwnership()` pins the rule against the real root manifest on every run, including the case a permissive lookup would fail: a root-only script name must NOT resolve under @objectstack/spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pk26oZ12t5N1hwGW1m1MgC
1 parent 9c120f0 commit 1997df3

3 files changed

Lines changed: 257 additions & 24 deletions

File tree

scripts/check-regen-pending.mjs

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -70,19 +70,41 @@ import { tmpdir } from 'node:os';
7070
import { dirname, join, resolve } from 'node:path';
7171
import { fileURLToPath } from 'node:url';
7272

73-
import { PENDING_MARKER, entryForPath } from './regen-artifacts.mjs';
73+
import { PENDING_MARKER, entryForPath, ownerDir, ownerOf, ownerRunCommand } from './regen-artifacts.mjs';
7474
import { isEntrypoint } from './invoked-as.mjs';
75+
import { workspacePackages } from './workspace-enumerator.mjs';
7576

7677
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..');
7778
const SPEC_DIR = join(REPO_ROOT, 'packages/spec');
7879

7980
/**
80-
* Where the `check:*` gates are spawned. `--self-test`'s fixtures point this at a
81-
* throwaway package so the two-commit sequence can be replayed without spawning
82-
* the real spec gates; nothing else sets it. A mistake here fails SAFE — a
83-
* directory without those scripts makes pnpm exit non-zero, which reads as stale.
81+
* Overrides where the `check:*` gates are spawned. `--self-test`'s fixtures point
82+
* this at a throwaway package so the two-commit sequence can be replayed without
83+
* spawning the real spec gates; nothing else sets it.
8484
*/
85-
const GATE_CWD = process.env.OS_REGEN_GATE_CWD || SPEC_DIR;
85+
const GATE_CWD_OVERRIDE = process.env.OS_REGEN_GATE_CWD || null;
86+
87+
/**
88+
* Where ONE artifact's gate is spawned: the directory of the package that declares
89+
* it (#13585).
90+
*
91+
* This was `packages/spec` for every row unconditionally, and it is the half of the
92+
* single-manifest assumption a lookup-only fix would have left standing. A
93+
* root-owned row would have reconciled clean in `check:merge-driver` and then been
94+
* spawned here in a directory that does not define its script — measured, `pnpm -s
95+
* check:sdui-lockstep` exits 254 in this directory and 0 at the repo root — so the
96+
* artifact reads as permanently stale and `pre-commit` refuses every commit from
97+
* then on. Registered and unreconcilable is a worse defect than unregisterable,
98+
* which is why the owner is read here and not only by the gate.
99+
*
100+
* A mistake still fails SAFE, in the direction it always did: a directory without
101+
* the script makes pnpm exit non-zero, which reads as stale rather than as current.
102+
*/
103+
function gateCwd(entry, workspace) {
104+
if (GATE_CWD_OVERRIDE) return GATE_CWD_OVERRIDE;
105+
const dir = ownerDir(ownerOf(entry), workspace);
106+
return dir === null || dir === '.' ? REPO_ROOT : join(REPO_ROOT, dir);
107+
}
86108

87109
/**
88110
* Marker line recording a deferral, distinguished from the driver's path lines by
@@ -264,9 +286,9 @@ export function decide({ blocked, merging, deferral, allowDefer = true }) {
264286
return 'refuse-stale';
265287
}
266288

267-
function runCheck(script) {
289+
function runCheck(script, cwd) {
268290
try {
269-
execSync(`pnpm -s ${script}`, { cwd: GATE_CWD, stdio: ['ignore', 'pipe', 'pipe'] });
291+
execSync(`pnpm -s ${script}`, { cwd, stdio: ['ignore', 'pipe', 'pipe'] });
270292
return { ok: true, output: '' };
271293
} catch (err) {
272294
return { ok: false, output: `${err?.stdout?.toString() ?? ''}${err?.stderr?.toString() ?? ''}`.trim() };
@@ -287,6 +309,9 @@ function main({ prePush = false } = {}) {
287309

288310
const entries = pending.map((p) => ({ path: p, entry: entryForPath(p) })).filter((x) => x.entry);
289311
const unknown = pending.filter((p) => !entryForPath(p));
312+
// Enumerated once, here rather than per gate: `gateCwd` needs an owner-to-directory
313+
// answer and this is the repo's one parse of the workspace globs.
314+
const workspace = workspacePackages(REPO_ROOT);
290315

291316
console.error(
292317
`\nos-regen: ${pending.length} generated artifact(s) were merged WITHOUT a text merge and must be `
@@ -309,7 +334,7 @@ function main({ prePush = false } = {}) {
309334
` ✗ ${paths.join(', ')}\n`
310335
+ ` ${check} reads packages/spec/dist, which is older than src — NOT running it.\n`
311336
+ ` On a stale dist this gate reports phantom removals and the generator WRITES them.\n`
312-
+ ` pnpm --filter @objectstack/spec build && pnpm --filter @objectstack/spec ${entry.gen}`,
337+
+ ` pnpm --filter @objectstack/spec build && ${ownerRunCommand(ownerOf(entry), entry.gen)}`,
313338
);
314339
continue;
315340
}
@@ -323,19 +348,19 @@ function main({ prePush = false } = {}) {
323348
` ✗ ${paths.join(', ')}\n`
324349
+ ` ${check} reads packages/spec/json-schema/, which is missing or older than src —\n`
325350
+ ` NOT running it. That tree is gitignored, so a merge never brings it with them.\n`
326-
+ ` pnpm --filter @objectstack/spec gen:schema && pnpm --filter @objectstack/spec ${entry.gen}`,
351+
+ ` pnpm --filter @objectstack/spec gen:schema && ${ownerRunCommand(ownerOf(entry), entry.gen)}`,
327352
);
328353
continue;
329354
}
330-
const { ok, output } = runCheck(check);
355+
const { ok, output } = runCheck(check, gateCwd(entry, workspace));
331356
if (ok) {
332357
console.error(` ✓ ${paths.join(', ')} — current`);
333358
continue;
334359
}
335360
blocked++;
336361
const detail = output.split('\n').filter(Boolean).slice(0, 3).map((l) => ` ${l}`).join('\n');
337362
console.error(` ✗ ${paths.join(', ')} — stale\n${detail ? `${detail}\n` : ''}`
338-
+ ` pnpm --filter @objectstack/spec ${entry.gen}`);
363+
+ ` ${ownerRunCommand(ownerOf(entry), entry.gen)}`);
339364
}
340365

341366
for (const p of unknown) {

scripts/git-merge-regen.mjs

Lines changed: 120 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,19 @@ import { dirname, join, relative, resolve } from 'node:path';
6161
import { fileURLToPath } from 'node:url';
6262

6363
import {
64+
DEFAULT_OWNER,
6465
DRIVER_NAME,
6566
GIT_SETTINGS,
6667
NOT_DRIVER_MANAGED,
6768
PENDING_MARKER,
6869
REGEN_ARTIFACTS,
70+
ROOT_OWNER,
6971
entryForPath,
72+
ownerDir,
73+
ownerOf,
74+
ownerRunCommand,
7075
} from './regen-artifacts.mjs';
76+
import { workspacePackages } from './workspace-enumerator.mjs';
7177

7278
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..');
7379

@@ -122,7 +128,7 @@ function drive(argv) {
122128
console.error(
123129
` ⟳ ${path}\n`
124130
+ ` not text-merged — it is generated. Regenerate from the merged tree:\n`
125-
+ ` pnpm --filter @objectstack/spec ${entry.gen}${dist}${tree}\n`
131+
+ ` ${ownerRunCommand(ownerOf(entry), entry.gen)}${dist}${tree}\n`
126132
+ ` The pre-commit hook will not let this commit through until you do.`,
127133
);
128134
return 0;
@@ -165,20 +171,123 @@ function reconcileAttributes() {
165171
return ok;
166172
}
167173

168-
/** Every `gen:`/`check:` the table names must still exist, or the driver's advice is a dead command. */
174+
/** Where an owner's manifest lives, repo-relative, given a resolved directory. */
175+
function manifestFor(dir) {
176+
return dir === '.' ? 'package.json' : `${dir}/package.json`;
177+
}
178+
179+
/**
180+
* Every `gen:`/`check:` the table names must still exist **in the manifest of the
181+
* row's declared owner**, or the driver's advice is a dead command.
182+
*
183+
* Resolution read `packages/spec/package.json` and nothing else until #13585, which
184+
* made a root-owned artifact unregisterable and said so in a way that pointed at the
185+
* wrong repair — "you named a script that does not exist" when the truth was "this
186+
* artifact is not owned by packages/spec". Two things follow from that, and the
187+
* second is the one worth guarding:
188+
*
189+
* - resolution reads the owner's manifest, whichever that is; and
190+
* - the refusal NAMES the manifests it searched, so the reader can see that the
191+
* lookup went somewhere else rather than conclude the script is missing.
192+
*
193+
* It stays exact in the direction that matters. A name is looked for in ONE
194+
* manifest — the declared owner's — never in "any manifest that has it", so a row
195+
* that names a root-only script while claiming a package owner still fails, and the
196+
* command the driver prints for it is the command the `pre-commit` gate spawns.
197+
*/
169198
function reconcileScripts() {
170-
const pkg = join(REPO_ROOT, 'packages/spec/package.json');
171-
if (!existsSync(pkg)) return fail('packages/spec/package.json not found');
172-
const scripts = JSON.parse(readFileSync(pkg, 'utf8')).scripts ?? {};
173-
const dead = [];
199+
const workspace = workspacePackages(REPO_ROOT);
200+
const byOwner = new Map();
174201
for (const e of REGEN_ARTIFACTS) {
175-
if (!scripts[e.gen]) dead.push(`${e.path}${e.gen}`);
176-
if (!scripts[e.check]) dead.push(`${e.path}${e.check}`);
202+
const owner = ownerOf(e);
203+
if (!byOwner.has(owner)) byOwner.set(owner, []);
204+
byOwner.get(owner).push(e);
205+
}
206+
207+
const dead = [];
208+
const unresolved = [];
209+
const searched = [];
210+
for (const [owner, entries] of byOwner) {
211+
const dir = ownerDir(owner, workspace);
212+
const file = dir === null ? null : join(REPO_ROOT, manifestFor(dir));
213+
if (file === null || !existsSync(file)) {
214+
unresolved.push(`${owner} — declared by ${entries.map((e) => e.path).join(', ')}`);
215+
continue;
216+
}
217+
searched.push(`${owner} (${manifestFor(dir)})`);
218+
const scripts = JSON.parse(readFileSync(file, 'utf8')).scripts ?? {};
219+
for (const e of entries) {
220+
for (const name of [e.gen, e.check]) {
221+
if (!scripts[name]) dead.push(`${e.path}${name} [owner ${owner}, ${manifestFor(dir)}]`);
222+
}
223+
}
224+
}
225+
226+
if (unresolved.length) {
227+
return fail(`owner(s) named by the table resolve to no manifest:\n ${unresolved.join('\n ')}\n`
228+
+ ' An owner is a workspace package name, or ROOT_OWNER for the root manifest.\n'
229+
+ ' Unresolved is a REFUSAL, not a skip: those rows\' scripts were never verified.');
177230
}
178231
if (dead.length) {
179-
return fail(`script(s) named by the table no longer exist in @objectstack/spec:\n ${dead.join('\n ')}`);
232+
return fail(`script(s) named by the table do not exist in their declared owner:\n ${dead.join('\n ')}\n`
233+
+ ` Manifests searched: ${searched.join(', ')}\n`
234+
+ ` A row that declares no \`owner\` defaults to ${DEFAULT_OWNER}, so this can mean the row is\n`
235+
+ ' in the wrong package rather than that the script is gone. If ROOT tooling owns the\n'
236+
+ ' artifact, declare it — `owner: ROOT_OWNER` in scripts/regen-artifacts.mjs. ⛔ Do NOT move\n'
237+
+ ' the scripts into a package to satisfy the lookup: that lets this tool decide code ownership.');
180238
}
181-
console.log(`✓ all ${REGEN_ARTIFACTS.length * 2} gen:/check: names resolve in @objectstack/spec`);
239+
console.log(`✓ all ${REGEN_ARTIFACTS.length * 2} gen:/check: names resolve in their declared owner`
240+
+ ` (${searched.join(', ')})`);
241+
return true;
242+
}
243+
244+
/**
245+
* The owner-resolution rule itself, pinned — the half a live tree cannot show.
246+
*
247+
* `reconcileScripts` above is green on this tree for the same reason it was green
248+
* before #13585: every row is spec-owned, so it exercises exactly one manifest and
249+
* would keep passing if the loosening were reverted. These cases read the REAL root
250+
* manifest through the same functions the driver and the `pre-commit` gate use, so
251+
* the root path is measured on every run rather than the first time somebody
252+
* registers a root-owned artifact.
253+
*
254+
* The two-way case is the third one. A permissive lookup — "resolve the name in any
255+
* manifest" — passes every other assertion here and fails that one, which is the
256+
* whole difference between a resolution and a search.
257+
*/
258+
function reconcileOwnership() {
259+
const workspace = workspacePackages(REPO_ROOT);
260+
const rootScripts = JSON.parse(readFileSync(join(REPO_ROOT, 'package.json'), 'utf8'));
261+
const specDir = ownerDir(DEFAULT_OWNER, workspace);
262+
const specScripts = specDir === null
263+
? {}
264+
: JSON.parse(readFileSync(join(REPO_ROOT, manifestFor(specDir)), 'utf8')).scripts ?? {};
265+
// A name this repo defines at the ROOT and nowhere else. Asserted, not assumed:
266+
// if it ever moves into a package, the assertion below says so instead of quietly
267+
// testing nothing.
268+
const rootOnly = 'check:merge-driver';
269+
270+
const cases = [
271+
['ROOT_OWNER is the root manifest\'s own name', rootScripts.name === ROOT_OWNER],
272+
['the root manifest resolves to the repo root', ownerDir(ROOT_OWNER, workspace) === '.'],
273+
[`${DEFAULT_OWNER} resolves to a workspace directory`, specDir !== null && specDir !== '.'],
274+
['a row with no owner defaults to DEFAULT_OWNER', ownerOf({ path: 'x' }) === DEFAULT_OWNER],
275+
['a declared owner is used verbatim', ownerOf({ owner: ROOT_OWNER }) === ROOT_OWNER],
276+
[`${rootOnly} exists in the root manifest`, Boolean(rootScripts.scripts?.[rootOnly])],
277+
// ⭐ The two-way case: resolution is per-owner, not "wherever the name turns up".
278+
[`${rootOnly} is NOT resolvable under ${DEFAULT_OWNER}`, !specScripts[rootOnly]],
279+
['an unknown owner refuses rather than skipping', ownerDir('@objectstack/not-a-package', workspace) === null],
280+
['the root command takes no --filter', ownerRunCommand(ROOT_OWNER, 'gen:x') === 'pnpm gen:x'],
281+
[
282+
'a package command filters to its owner',
283+
ownerRunCommand(DEFAULT_OWNER, 'gen:x') === `pnpm --filter ${DEFAULT_OWNER} gen:x`,
284+
],
285+
['a spawn asks for silence', ownerRunCommand(ROOT_OWNER, 'check:x', { silent: true }) === 'pnpm -s check:x'],
286+
];
287+
288+
const failures = cases.filter(([, ok]) => !ok).map(([name]) => name);
289+
if (failures.length) return fail(`owner resolution:\n ${failures.join('\n ')}`);
290+
console.log(`✓ owner resolution: ${cases.length} case(s) pinned, root manifest read as ${ROOT_OWNER}`);
182291
return true;
183292
}
184293

@@ -378,6 +487,7 @@ if (process.argv.includes('--self-test')) {
378487
const results = [
379488
reconcileAttributes(),
380489
reconcileScripts(),
490+
reconcileOwnership(),
381491
hookIsExecutable(),
382492
registeredDriverResolves(),
383493
endToEnd(),

scripts/regen-artifacts.mjs

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,51 @@
1414
* `package.json` on every run.
1515
*/
1616

17+
/**
18+
* The manifest that owns a row's `gen:`/`check:` names when the row does not say.
19+
*
20+
* Every row declared this implicitly until #13585, and the reconciliation read it
21+
* and nothing else — see `REGEN_ARTIFACTS` for what that cost.
22+
*/
23+
export const DEFAULT_OWNER = '@objectstack/spec';
24+
25+
/**
26+
* The ROOT manifest, by the name it gives itself.
27+
*
28+
* Spelled as a name rather than as a path so it reads the same way as any other
29+
* owner, and pinned against the real root `package.json` by
30+
* `git-merge-regen.mjs --self-test` so the two cannot drift apart silently. The
31+
* root is the one owner that is not a workspace member, which is why `ownerDir`
32+
* answers it directly instead of looking for it.
33+
*/
34+
export const ROOT_OWNER = '@objectstack/spec-monorepo';
35+
1736
/**
1837
* Artifacts the driver takes over. `check` proves currency, `gen` restores it.
19-
* Both names are verified against `packages/spec/package.json` by `--self-test`,
20-
* so a renamed script fails loudly here instead of silently disarming a path.
38+
*
39+
* `owner` names the manifest that defines those two script names, and defaults to
40+
* `DEFAULT_OWNER`. `--self-test` verifies each name against THAT manifest, so a
41+
* renamed script fails loudly here instead of silently disarming a path.
42+
*
43+
* ## Why the owner is declared and not searched for (#13585)
44+
*
45+
* Until #13585 the verification read `packages/spec/package.json` alone, so an
46+
* artifact owned by ROOT tooling could not be registered at all: its `gen:`/
47+
* `check:` live in the root manifest, and the reconciliation reported them as
48+
* scripts that "no longer exist". That refusal was correct about the tree and
49+
* wrong about the world, and an author following it literally moves root tooling
50+
* into a package it does not belong to, purely to satisfy a lookup path.
51+
*
52+
* Widening the lookup to "resolve the name in any manifest" would have fixed the
53+
* refusal and left a worse seam behind, because the name is not what the other two
54+
* consumers need. The driver prints a regeneration command and the `pre-commit`
55+
* gate SPAWNS one, and both were bound to `packages/spec`; a row that resolved
56+
* somewhere else would be registered and unreconcilable — self-test green, while
57+
* the hook ran the gate in a directory that does not define it and refused the
58+
* commit forever. Measured before this field existed: `pnpm -s check:sdui-lockstep`
59+
* exits 254 (`Command not found`) in the gate's spawn directory and 0 at the repo
60+
* root. So the owner is a declaration all three consumers read, which is what keeps
61+
* the reconciliation two-way rather than merely permissive.
2162
*/
2263
export const REGEN_ARTIFACTS = Object.freeze([
2364
// Deliberately NOT sharded (#5837): keyed by version, so two PRs append under
@@ -241,6 +282,63 @@ export const GIT_SETTINGS = Object.freeze([
241282
{ key: 'core.hooksPath', value: '.githooks' },
242283
]);
243284

285+
/**
286+
* The manifest name that owns an entry's `gen:`/`check:` scripts.
287+
*
288+
* Pure, and the single place the default is applied — a consumer that spelled
289+
* `entry.owner ?? '@objectstack/spec'` inline would be a second definition of the
290+
* default, and the one that wins would be whichever consumer the reader opened.
291+
*
292+
* @param {{ owner?: string }} entry
293+
* @returns {string}
294+
*/
295+
export function ownerOf(entry) {
296+
return entry.owner ?? DEFAULT_OWNER;
297+
}
298+
299+
/**
300+
* The repo-relative directory an owner's `package.json` sits in, or `null` when no
301+
* such owner exists.
302+
*
303+
* Pure on purpose: it takes an ALREADY-enumerated workspace rather than reading one,
304+
* so this module keeps its "constants and pure functions, no top-level statement that
305+
* runs" shape (the property `check:entry-guard` relies on to leave it alone). Callers
306+
* pass `workspacePackages(REPO_ROOT)` from `workspace-enumerator.mjs`, which is the
307+
* repo's one parse of the workspace globs.
308+
*
309+
* `null` is a REFUSAL, never a skip: an owner nobody can resolve means a row whose
310+
* scripts were never verified, which is the state this whole reconciliation exists to
311+
* make impossible.
312+
*
313+
* @param {string} owner
314+
* @param {Array<{ dir: string, manifest: Record<string, unknown> }>} workspacePkgs
315+
* @returns {string | null}
316+
*/
317+
export function ownerDir(owner, workspacePkgs) {
318+
if (owner === ROOT_OWNER) return '.';
319+
const hit = workspacePkgs.find((p) => p?.manifest?.name === owner);
320+
return hit ? hit.dir : null;
321+
}
322+
323+
/**
324+
* The pnpm invocation that runs `script` for `owner`, FROM THE REPO ROOT.
325+
*
326+
* One builder, because the string the driver PRINTS and the command the
327+
* `pre-commit` gate SPAWNS have to be the same command; #13585 is what happens when
328+
* a lookup and its consumers disagree about which package a row belongs to. The root
329+
* manifest takes no `--filter`: it is not a workspace member, and `pnpm <script>` at
330+
* the root is how its scripts run.
331+
*
332+
* @param {string} owner
333+
* @param {string} script
334+
* @param {{ silent?: boolean }} [options] `silent` adds `-s`, for a spawn rather than advice
335+
* @returns {string}
336+
*/
337+
export function ownerRunCommand(owner, script, { silent = false } = {}) {
338+
const s = silent ? ' -s' : '';
339+
return owner === ROOT_OWNER ? `pnpm${s} ${script}` : `pnpm${s} --filter ${owner} ${script}`;
340+
}
341+
244342
/** Resolve the entry that owns a path, or undefined. Handles the one `**` entry. */
245343
export function entryForPath(p) {
246344
return REGEN_ARTIFACTS.find((e) =>

0 commit comments

Comments
 (0)