From 1997df33792c7af92aa46fad351cb36a59561bb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 02:53:14 +0000 Subject: [PATCH] fix(devx): resolve a regen row's gen:/check: in its DECLARED owner, not only in packages/spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01Pk26oZ12t5N1hwGW1m1MgC --- scripts/check-regen-pending.mjs | 49 +++++++++--- scripts/git-merge-regen.mjs | 130 +++++++++++++++++++++++++++++--- scripts/regen-artifacts.mjs | 102 ++++++++++++++++++++++++- 3 files changed, 257 insertions(+), 24 deletions(-) diff --git a/scripts/check-regen-pending.mjs b/scripts/check-regen-pending.mjs index 1822c08953..a9aedff9e9 100755 --- a/scripts/check-regen-pending.mjs +++ b/scripts/check-regen-pending.mjs @@ -70,19 +70,41 @@ import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { PENDING_MARKER, entryForPath } from './regen-artifacts.mjs'; +import { PENDING_MARKER, entryForPath, ownerDir, ownerOf, ownerRunCommand } from './regen-artifacts.mjs'; import { isEntrypoint } from './invoked-as.mjs'; +import { workspacePackages } from './workspace-enumerator.mjs'; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const SPEC_DIR = join(REPO_ROOT, 'packages/spec'); /** - * Where the `check:*` gates are spawned. `--self-test`'s fixtures point this at a - * throwaway package so the two-commit sequence can be replayed without spawning - * the real spec gates; nothing else sets it. A mistake here fails SAFE — a - * directory without those scripts makes pnpm exit non-zero, which reads as stale. + * Overrides where the `check:*` gates are spawned. `--self-test`'s fixtures point + * this at a throwaway package so the two-commit sequence can be replayed without + * spawning the real spec gates; nothing else sets it. */ -const GATE_CWD = process.env.OS_REGEN_GATE_CWD || SPEC_DIR; +const GATE_CWD_OVERRIDE = process.env.OS_REGEN_GATE_CWD || null; + +/** + * Where ONE artifact's gate is spawned: the directory of the package that declares + * it (#13585). + * + * This was `packages/spec` for every row unconditionally, and it is the half of the + * single-manifest assumption a lookup-only fix would have left standing. A + * root-owned row would have reconciled clean in `check:merge-driver` and then been + * spawned here in a directory that does not define its script — measured, `pnpm -s + * check:sdui-lockstep` exits 254 in this directory and 0 at the repo root — so the + * artifact reads as permanently stale and `pre-commit` refuses every commit from + * then on. Registered and unreconcilable is a worse defect than unregisterable, + * which is why the owner is read here and not only by the gate. + * + * A mistake still fails SAFE, in the direction it always did: a directory without + * the script makes pnpm exit non-zero, which reads as stale rather than as current. + */ +function gateCwd(entry, workspace) { + if (GATE_CWD_OVERRIDE) return GATE_CWD_OVERRIDE; + const dir = ownerDir(ownerOf(entry), workspace); + return dir === null || dir === '.' ? REPO_ROOT : join(REPO_ROOT, dir); +} /** * 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 }) { return 'refuse-stale'; } -function runCheck(script) { +function runCheck(script, cwd) { try { - execSync(`pnpm -s ${script}`, { cwd: GATE_CWD, stdio: ['ignore', 'pipe', 'pipe'] }); + execSync(`pnpm -s ${script}`, { cwd, stdio: ['ignore', 'pipe', 'pipe'] }); return { ok: true, output: '' }; } catch (err) { return { ok: false, output: `${err?.stdout?.toString() ?? ''}${err?.stderr?.toString() ?? ''}`.trim() }; @@ -287,6 +309,9 @@ function main({ prePush = false } = {}) { const entries = pending.map((p) => ({ path: p, entry: entryForPath(p) })).filter((x) => x.entry); const unknown = pending.filter((p) => !entryForPath(p)); + // Enumerated once, here rather than per gate: `gateCwd` needs an owner-to-directory + // answer and this is the repo's one parse of the workspace globs. + const workspace = workspacePackages(REPO_ROOT); console.error( `\nos-regen: ${pending.length} generated artifact(s) were merged WITHOUT a text merge and must be ` @@ -309,7 +334,7 @@ function main({ prePush = false } = {}) { ` ✗ ${paths.join(', ')}\n` + ` ${check} reads packages/spec/dist, which is older than src — NOT running it.\n` + ` On a stale dist this gate reports phantom removals and the generator WRITES them.\n` - + ` pnpm --filter @objectstack/spec build && pnpm --filter @objectstack/spec ${entry.gen}`, + + ` pnpm --filter @objectstack/spec build && ${ownerRunCommand(ownerOf(entry), entry.gen)}`, ); continue; } @@ -323,11 +348,11 @@ function main({ prePush = false } = {}) { ` ✗ ${paths.join(', ')}\n` + ` ${check} reads packages/spec/json-schema/, which is missing or older than src —\n` + ` NOT running it. That tree is gitignored, so a merge never brings it with them.\n` - + ` pnpm --filter @objectstack/spec gen:schema && pnpm --filter @objectstack/spec ${entry.gen}`, + + ` pnpm --filter @objectstack/spec gen:schema && ${ownerRunCommand(ownerOf(entry), entry.gen)}`, ); continue; } - const { ok, output } = runCheck(check); + const { ok, output } = runCheck(check, gateCwd(entry, workspace)); if (ok) { console.error(` ✓ ${paths.join(', ')} — current`); continue; @@ -335,7 +360,7 @@ function main({ prePush = false } = {}) { blocked++; const detail = output.split('\n').filter(Boolean).slice(0, 3).map((l) => ` ${l}`).join('\n'); console.error(` ✗ ${paths.join(', ')} — stale\n${detail ? `${detail}\n` : ''}` - + ` pnpm --filter @objectstack/spec ${entry.gen}`); + + ` ${ownerRunCommand(ownerOf(entry), entry.gen)}`); } for (const p of unknown) { diff --git a/scripts/git-merge-regen.mjs b/scripts/git-merge-regen.mjs index fd3bf4408c..cee3b7a0be 100755 --- a/scripts/git-merge-regen.mjs +++ b/scripts/git-merge-regen.mjs @@ -61,13 +61,19 @@ import { dirname, join, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { + DEFAULT_OWNER, DRIVER_NAME, GIT_SETTINGS, NOT_DRIVER_MANAGED, PENDING_MARKER, REGEN_ARTIFACTS, + ROOT_OWNER, entryForPath, + ownerDir, + ownerOf, + ownerRunCommand, } from './regen-artifacts.mjs'; +import { workspacePackages } from './workspace-enumerator.mjs'; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); @@ -122,7 +128,7 @@ function drive(argv) { console.error( ` ⟳ ${path}\n` + ` not text-merged — it is generated. Regenerate from the merged tree:\n` - + ` pnpm --filter @objectstack/spec ${entry.gen}${dist}${tree}\n` + + ` ${ownerRunCommand(ownerOf(entry), entry.gen)}${dist}${tree}\n` + ` The pre-commit hook will not let this commit through until you do.`, ); return 0; @@ -165,20 +171,123 @@ function reconcileAttributes() { return ok; } -/** Every `gen:`/`check:` the table names must still exist, or the driver's advice is a dead command. */ +/** Where an owner's manifest lives, repo-relative, given a resolved directory. */ +function manifestFor(dir) { + return dir === '.' ? 'package.json' : `${dir}/package.json`; +} + +/** + * Every `gen:`/`check:` the table names must still exist **in the manifest of the + * row's declared owner**, or the driver's advice is a dead command. + * + * Resolution read `packages/spec/package.json` and nothing else until #13585, which + * made a root-owned artifact unregisterable and said so in a way that pointed at the + * wrong repair — "you named a script that does not exist" when the truth was "this + * artifact is not owned by packages/spec". Two things follow from that, and the + * second is the one worth guarding: + * + * - resolution reads the owner's manifest, whichever that is; and + * - the refusal NAMES the manifests it searched, so the reader can see that the + * lookup went somewhere else rather than conclude the script is missing. + * + * It stays exact in the direction that matters. A name is looked for in ONE + * manifest — the declared owner's — never in "any manifest that has it", so a row + * that names a root-only script while claiming a package owner still fails, and the + * command the driver prints for it is the command the `pre-commit` gate spawns. + */ function reconcileScripts() { - const pkg = join(REPO_ROOT, 'packages/spec/package.json'); - if (!existsSync(pkg)) return fail('packages/spec/package.json not found'); - const scripts = JSON.parse(readFileSync(pkg, 'utf8')).scripts ?? {}; - const dead = []; + const workspace = workspacePackages(REPO_ROOT); + const byOwner = new Map(); for (const e of REGEN_ARTIFACTS) { - if (!scripts[e.gen]) dead.push(`${e.path} → ${e.gen}`); - if (!scripts[e.check]) dead.push(`${e.path} → ${e.check}`); + const owner = ownerOf(e); + if (!byOwner.has(owner)) byOwner.set(owner, []); + byOwner.get(owner).push(e); + } + + const dead = []; + const unresolved = []; + const searched = []; + for (const [owner, entries] of byOwner) { + const dir = ownerDir(owner, workspace); + const file = dir === null ? null : join(REPO_ROOT, manifestFor(dir)); + if (file === null || !existsSync(file)) { + unresolved.push(`${owner} — declared by ${entries.map((e) => e.path).join(', ')}`); + continue; + } + searched.push(`${owner} (${manifestFor(dir)})`); + const scripts = JSON.parse(readFileSync(file, 'utf8')).scripts ?? {}; + for (const e of entries) { + for (const name of [e.gen, e.check]) { + if (!scripts[name]) dead.push(`${e.path} → ${name} [owner ${owner}, ${manifestFor(dir)}]`); + } + } + } + + if (unresolved.length) { + return fail(`owner(s) named by the table resolve to no manifest:\n ${unresolved.join('\n ')}\n` + + ' An owner is a workspace package name, or ROOT_OWNER for the root manifest.\n' + + ' Unresolved is a REFUSAL, not a skip: those rows\' scripts were never verified.'); } if (dead.length) { - return fail(`script(s) named by the table no longer exist in @objectstack/spec:\n ${dead.join('\n ')}`); + return fail(`script(s) named by the table do not exist in their declared owner:\n ${dead.join('\n ')}\n` + + ` Manifests searched: ${searched.join(', ')}\n` + + ` A row that declares no \`owner\` defaults to ${DEFAULT_OWNER}, so this can mean the row is\n` + + ' in the wrong package rather than that the script is gone. If ROOT tooling owns the\n' + + ' artifact, declare it — `owner: ROOT_OWNER` in scripts/regen-artifacts.mjs. ⛔ Do NOT move\n' + + ' the scripts into a package to satisfy the lookup: that lets this tool decide code ownership.'); } - console.log(`✓ all ${REGEN_ARTIFACTS.length * 2} gen:/check: names resolve in @objectstack/spec`); + console.log(`✓ all ${REGEN_ARTIFACTS.length * 2} gen:/check: names resolve in their declared owner` + + ` (${searched.join(', ')})`); + return true; +} + +/** + * The owner-resolution rule itself, pinned — the half a live tree cannot show. + * + * `reconcileScripts` above is green on this tree for the same reason it was green + * before #13585: every row is spec-owned, so it exercises exactly one manifest and + * would keep passing if the loosening were reverted. These cases read the REAL root + * manifest through the same functions the driver and the `pre-commit` gate use, so + * the root path is measured on every run rather than the first time somebody + * registers a root-owned artifact. + * + * The two-way case is the third one. A permissive lookup — "resolve the name in any + * manifest" — passes every other assertion here and fails that one, which is the + * whole difference between a resolution and a search. + */ +function reconcileOwnership() { + const workspace = workspacePackages(REPO_ROOT); + const rootScripts = JSON.parse(readFileSync(join(REPO_ROOT, 'package.json'), 'utf8')); + const specDir = ownerDir(DEFAULT_OWNER, workspace); + const specScripts = specDir === null + ? {} + : JSON.parse(readFileSync(join(REPO_ROOT, manifestFor(specDir)), 'utf8')).scripts ?? {}; + // A name this repo defines at the ROOT and nowhere else. Asserted, not assumed: + // if it ever moves into a package, the assertion below says so instead of quietly + // testing nothing. + const rootOnly = 'check:merge-driver'; + + const cases = [ + ['ROOT_OWNER is the root manifest\'s own name', rootScripts.name === ROOT_OWNER], + ['the root manifest resolves to the repo root', ownerDir(ROOT_OWNER, workspace) === '.'], + [`${DEFAULT_OWNER} resolves to a workspace directory`, specDir !== null && specDir !== '.'], + ['a row with no owner defaults to DEFAULT_OWNER', ownerOf({ path: 'x' }) === DEFAULT_OWNER], + ['a declared owner is used verbatim', ownerOf({ owner: ROOT_OWNER }) === ROOT_OWNER], + [`${rootOnly} exists in the root manifest`, Boolean(rootScripts.scripts?.[rootOnly])], + // ⭐ The two-way case: resolution is per-owner, not "wherever the name turns up". + [`${rootOnly} is NOT resolvable under ${DEFAULT_OWNER}`, !specScripts[rootOnly]], + ['an unknown owner refuses rather than skipping', ownerDir('@objectstack/not-a-package', workspace) === null], + ['the root command takes no --filter', ownerRunCommand(ROOT_OWNER, 'gen:x') === 'pnpm gen:x'], + [ + 'a package command filters to its owner', + ownerRunCommand(DEFAULT_OWNER, 'gen:x') === `pnpm --filter ${DEFAULT_OWNER} gen:x`, + ], + ['a spawn asks for silence', ownerRunCommand(ROOT_OWNER, 'check:x', { silent: true }) === 'pnpm -s check:x'], + ]; + + const failures = cases.filter(([, ok]) => !ok).map(([name]) => name); + if (failures.length) return fail(`owner resolution:\n ${failures.join('\n ')}`); + console.log(`✓ owner resolution: ${cases.length} case(s) pinned, root manifest read as ${ROOT_OWNER}`); return true; } @@ -378,6 +487,7 @@ if (process.argv.includes('--self-test')) { const results = [ reconcileAttributes(), reconcileScripts(), + reconcileOwnership(), hookIsExecutable(), registeredDriverResolves(), endToEnd(), diff --git a/scripts/regen-artifacts.mjs b/scripts/regen-artifacts.mjs index 7605f19b20..0c85e9c7c8 100644 --- a/scripts/regen-artifacts.mjs +++ b/scripts/regen-artifacts.mjs @@ -14,10 +14,51 @@ * `package.json` on every run. */ +/** + * The manifest that owns a row's `gen:`/`check:` names when the row does not say. + * + * Every row declared this implicitly until #13585, and the reconciliation read it + * and nothing else — see `REGEN_ARTIFACTS` for what that cost. + */ +export const DEFAULT_OWNER = '@objectstack/spec'; + +/** + * The ROOT manifest, by the name it gives itself. + * + * Spelled as a name rather than as a path so it reads the same way as any other + * owner, and pinned against the real root `package.json` by + * `git-merge-regen.mjs --self-test` so the two cannot drift apart silently. The + * root is the one owner that is not a workspace member, which is why `ownerDir` + * answers it directly instead of looking for it. + */ +export const ROOT_OWNER = '@objectstack/spec-monorepo'; + /** * Artifacts the driver takes over. `check` proves currency, `gen` restores it. - * Both names are verified against `packages/spec/package.json` by `--self-test`, - * so a renamed script fails loudly here instead of silently disarming a path. + * + * `owner` names the manifest that defines those two script names, and defaults to + * `DEFAULT_OWNER`. `--self-test` verifies each name against THAT manifest, so a + * renamed script fails loudly here instead of silently disarming a path. + * + * ## Why the owner is declared and not searched for (#13585) + * + * Until #13585 the verification read `packages/spec/package.json` alone, so an + * artifact owned by ROOT tooling could not be registered at all: its `gen:`/ + * `check:` live in the root manifest, and the reconciliation reported them as + * scripts that "no longer exist". That refusal was correct about the tree and + * wrong about the world, and an author following it literally moves root tooling + * into a package it does not belong to, purely to satisfy a lookup path. + * + * Widening the lookup to "resolve the name in any manifest" would have fixed the + * refusal and left a worse seam behind, because the name is not what the other two + * consumers need. The driver prints a regeneration command and the `pre-commit` + * gate SPAWNS one, and both were bound to `packages/spec`; a row that resolved + * somewhere else would be registered and unreconcilable — self-test green, while + * the hook ran the gate in a directory that does not define it and refused the + * commit forever. Measured before this field existed: `pnpm -s check:sdui-lockstep` + * exits 254 (`Command not found`) in the gate's spawn directory and 0 at the repo + * root. So the owner is a declaration all three consumers read, which is what keeps + * the reconciliation two-way rather than merely permissive. */ export const REGEN_ARTIFACTS = Object.freeze([ // Deliberately NOT sharded (#5837): keyed by version, so two PRs append under @@ -241,6 +282,63 @@ export const GIT_SETTINGS = Object.freeze([ { key: 'core.hooksPath', value: '.githooks' }, ]); +/** + * The manifest name that owns an entry's `gen:`/`check:` scripts. + * + * Pure, and the single place the default is applied — a consumer that spelled + * `entry.owner ?? '@objectstack/spec'` inline would be a second definition of the + * default, and the one that wins would be whichever consumer the reader opened. + * + * @param {{ owner?: string }} entry + * @returns {string} + */ +export function ownerOf(entry) { + return entry.owner ?? DEFAULT_OWNER; +} + +/** + * The repo-relative directory an owner's `package.json` sits in, or `null` when no + * such owner exists. + * + * Pure on purpose: it takes an ALREADY-enumerated workspace rather than reading one, + * so this module keeps its "constants and pure functions, no top-level statement that + * runs" shape (the property `check:entry-guard` relies on to leave it alone). Callers + * pass `workspacePackages(REPO_ROOT)` from `workspace-enumerator.mjs`, which is the + * repo's one parse of the workspace globs. + * + * `null` is a REFUSAL, never a skip: an owner nobody can resolve means a row whose + * scripts were never verified, which is the state this whole reconciliation exists to + * make impossible. + * + * @param {string} owner + * @param {Array<{ dir: string, manifest: Record }>} workspacePkgs + * @returns {string | null} + */ +export function ownerDir(owner, workspacePkgs) { + if (owner === ROOT_OWNER) return '.'; + const hit = workspacePkgs.find((p) => p?.manifest?.name === owner); + return hit ? hit.dir : null; +} + +/** + * The pnpm invocation that runs `script` for `owner`, FROM THE REPO ROOT. + * + * One builder, because the string the driver PRINTS and the command the + * `pre-commit` gate SPAWNS have to be the same command; #13585 is what happens when + * a lookup and its consumers disagree about which package a row belongs to. The root + * manifest takes no `--filter`: it is not a workspace member, and `pnpm