From 0224f5bcf3e27235ebbd00c60b300b4768c6fbc5 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Sun, 21 Jun 2026 20:40:48 -0700 Subject: [PATCH] fix(#1383): resolve GSD version without a top-level require of the runtime-root package.json (#1409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1383): resolve GSD version without a top-level require of the runtime-root package.json The extracted runtime-artifact-conversion module sits in the gsd-tools loader chain and did a module-load `require('../../../package.json')`. On Codex (whose runtime root has no package.json) that threw `Cannot find module '../../../package.json'`, crashing every gsd-tools command before it did anything. Even on Claude the synthetic `{"type":"commonjs"}` has no `version`, so the sole consumer already emitted `version: undefined`. Resolve the version lazily and defensively instead: read the installed gsd-core/VERSION, else lazily require the runtime-root package.json, else degrade to '' so the caller omits the field. Both sources are validated against the repo's semver-prefix convention (mirrors update-context.cts) so a garbled VERSION is never emitted verbatim. install.js's dead duplicate converter is intentionally left untouched (scoped to the crash). Adds a #1383 regression block exercising resolveVersionFrom across VERSION-only / package.json-only / neither / malformed-VERSION layouts, asserting no-throw and the correct version string. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1383): add changeset for the Codex gsd-tools crash fix Co-Authored-By: Claude Opus 4.8 (1M context) * docs(#1383): record resolveVersionFrom export in CONTEXT.md glossary Maintainer review gate on PR #1409: the lazy resolveVersionFrom seam added on the Runtime Artifact Conversion Module must be recorded in CONTEXT.md so the canonical glossary doesn't drift from the exported surface. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1383): reword changeset to drop product-name parenthetical product-name-purity (#1777) rejects 'Codex (…)' parentheticals that render verbatim into CHANGELOG.md. Reword to a comma clause; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Tom Boucher --- .changeset/sturdy-birds-climb.md | 5 +++ CONTEXT.md | 2 +- src/runtime-artifact-conversion.cts | 46 +++++++++++++++++++- tests/hermes-skills-migration.test.cjs | 58 ++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 .changeset/sturdy-birds-climb.md diff --git a/.changeset/sturdy-birds-climb.md b/.changeset/sturdy-birds-climb.md new file mode 100644 index 000000000..3e8d3e2f6 --- /dev/null +++ b/.changeset/sturdy-birds-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1409 +--- +**Codex runtime no longer crashes on startup** — every `gsd-tools` command previously aborted with `Cannot find module '../../../package.json'` on Codex, whose runtime root has no `package.json`, because a module in the loader chain did a top-level require of it. The version emitted into Hermes skill frontmatter is now sourced lazily from the installed `gsd-core/VERSION` (validated semver), so `gsd-tools` loads on every runtime and never emits `version: undefined`. (#1383) diff --git a/CONTEXT.md b/CONTEXT.md index 2f9e9c495..8b99bd154 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -155,7 +155,7 @@ Module owning which skills and agents are written to runtime config directories Module owning the per-runtime mapping from artifact kind to filesystem placement. ADR-3660 defines the typed `kinds` per runtime (`commands`, `agents`, `skills`) with destination subpath, prefix, and stage adapter (with per-runtime converters in `bin/install.js`: `convertClaudeCommandToClaudeSkill`, `…CodexSkill`, `…CopilotSkill`, `…AntigravitySkill`). Owns the per-runtime `nested` skill-bundle decision (#69): a `skillsKind` flag in `src/runtime-artifact-layout.cts` drives whether a runtime receives the nested router layout (6 `gsd-ns-*` routers + concrete skills under `/skills//`) or the flat `skills/gsd-/` layout; the evidence/doc-link matrix is recorded in a comment above `resolveRuntimeArtifactLayout`. Phase 1 applies this seam to the Runtime Surface Module (`surface.cjs:applySurface`); as of #813, `applySurface` applies the same per-runtime skill-body path rewrites as `installRuntimeArtifacts` for `skills` kinds — re-surfacing no longer overwrites installed SKILL.md bodies with converter-default `~/.claude` paths. Per ADR-1508 / #1511 the former `getInstallExports`/`loadInstallExports` relay (a `GSD_TEST_MODE`-guarded `require('bin/install.js')` by which `surface.cjs` reached `computePathPrefix`/`applyRuntimeContentRewritesInPlace`) was DELETED from this module; content rewriting now lives in the Runtime Artifact Conversion Module and `surface.cjs:applySurface` calls its `rewriteStagedSkillBodies` directly. The resolved `scope` is still carried on the `Layout` object so `applySurface` derives the same `pathPrefix` (global `$HOME` form vs. absolute) as a fresh install. Phase 2 is planned to migrate install/uninstall in `bin/install.js` so all lifecycle sites iterate one shared layout table instead of re-encoding runtime layout logic. This design is intended to remove the #3659 class of omissions. Migrations remain under the Installer Migration Module (ADR-0008). See ADR-3660. ### Runtime Artifact Conversion Module -Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. SHIPPED (ADR-1508): the converter family relocated in #1510 Phase 1 (`getDirName`→runtime-name-policy, `processAttribution` here); #1511 Phase 2 moved the content-rewrite engine here in full — `_applyRuntimeRewrites` (per-runtime switch, injected attribution), the staged-content walkers `applyRuntimeContentRewritesInPlace`/`applyRuntimeContentRewritesForCommandsInPlace`, `computePathPrefix` (private; `_computePathPrefix` for tests), and the deep public seam `rewriteStagedSkillBodies`/`rewriteStagedCommandBodies({runtime,configDir,scope,homedir?,platform?,resolveAttribution?})`. `bin/install.js` binds these back (single owner, exports preserved); `getCommitAttribution` stays in `bin/install.js` (impure install-time config I/O) and is injected. The `getInstallExports` relay in Runtime Artifact Layout Module was deleted; the dependency direction installer/layout → conversion (never upward) is now enforced. Exception: opencode and kilo path-prefix rewriting is a deliberate `bin/install.js`-owned pre-conversion step (`applyOpencodeFamilyPathPrefix`) per #784, not a violation of the single-owner rule. Source: `gsd-core/bin/lib/runtime-artifact-conversion.cjs` (generated from `src/runtime-artifact-conversion.cts`). +Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. SHIPPED (ADR-1508): the converter family relocated in #1510 Phase 1 (`getDirName`→runtime-name-policy, `processAttribution` here); #1511 Phase 2 moved the content-rewrite engine here in full — `_applyRuntimeRewrites` (per-runtime switch, injected attribution), the staged-content walkers `applyRuntimeContentRewritesInPlace`/`applyRuntimeContentRewritesForCommandsInPlace`, `computePathPrefix` (private; `_computePathPrefix` for tests), and the deep public seam `rewriteStagedSkillBodies`/`rewriteStagedCommandBodies({runtime,configDir,scope,homedir?,platform?,resolveAttribution?})`. `bin/install.js` binds these back (single owner, exports preserved); `getCommitAttribution` stays in `bin/install.js` (impure install-time config I/O) and is injected. The `getInstallExports` relay in Runtime Artifact Layout Module was deleted; the dependency direction installer/layout → conversion (never upward) is now enforced. Exception: opencode and kilo path-prefix rewriting is a deliberate `bin/install.js`-owned pre-conversion step (`applyOpencodeFamilyPathPrefix`) per #784, not a violation of the single-owner rule. Source: `gsd-core/bin/lib/runtime-artifact-conversion.cjs` (generated from `src/runtime-artifact-conversion.cts`). Also exports `resolveVersionFrom(libDir)` — a lazy, defensive GSD-version resolver (installed-tree `gsd-core/VERSION` first, then the source/npm `package.json` three dirs up, both validated against the repo's shared semver-prefix shape, degrading to `''` on failure) that replaced a module-load-time `require('../../../package.json')` which crashed on runtimes whose root carries no `package.json` (e.g. Codex) (#1383). ### Runtime Artifact Install Plan Module Module owning install-time staging and content-rewrite selection for a pre-resolved Runtime Artifact Layout. Interface: `createRuntimeArtifactInstallPlan({ layout, resolvedProfile, homedir?, platform?, resolveAttribution?, deps? }) -> { ok:true, plan:{ items, cleanupDirs } } | { ok:false, kind:'stage_failed'|'rewrite_failed', message, cleanupDirs, failedKind? }`. It iterates `layout.kinds` in order, calls each kind's `stage(resolvedProfile)`, delegates `commands` to Runtime Artifact Conversion `rewriteStagedCommandBodies`, delegates `skills` and `kimi-agents` to `rewriteStagedSkillBodies`, leaves non-rewritten kinds unchanged, and projects copy items as `{ kind, sourceDir, destDir }`. It deliberately does not prune, copy, run legacy migrations, print output, or execute cleanup; those remain Installer Module adapter responsibilities until later slices wire the plan into `bin/install.js`. Source: `gsd-core/bin/lib/runtime-artifact-install-plan.cjs` (generated from `src/runtime-artifact-install-plan.cts`). See Runtime Artifact Layout Module and Runtime Artifact Conversion Module. diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index 7136316e6..a3f78c6cb 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -21,10 +21,46 @@ import os from 'node:os'; import fs from 'node:fs'; import commandRoster = require('./command-roster.cjs'); const { readGsdCommandNames, transformContentToHyphen } = commandRoster; -const pkg = require('../../../package.json'); import runtimeNamePolicy = require('./runtime-name-policy.cjs'); const { getDirName } = runtimeNamePolicy; +// #1383: resolve GSD's version WITHOUT a top-level +// `require('../../../package.json')`. That require ran at module load on every +// gsd-tools invocation (this module sits in the gsd-tools loader chain) and +// threw `Cannot find module '../../../package.json'` on runtimes whose root has +// no package.json — notably Codex, where the installer omits the synthetic root +// package.json — taking the entire CLI down before it did anything. And even +// where it resolved (Claude's synthetic `{"type":"commonjs"}`), there is no +// `version` field, so the single consumer below already emitted +// `version: undefined`. Resolve lazily and defensively instead: +// 1. Installed trees carry /gsd-core/VERSION (written by the installer); +// this module lives at /gsd-core/bin/lib, so VERSION is two dirs up. +// 2. The source / npm-package tree has no gsd-core/VERSION but carries a real +// package.json three dirs up — read it lazily, never at module-load time. +// A failed/invalid lookup degrades to '' (the caller omits the field) rather +// than crashing or emitting `version: undefined`. Both sources are validated +// against the same semver shape the repo's other VERSION reader enforces +// (src/update-context.cts) so a garbled VERSION file is never emitted verbatim. +// Exported for the #1383 regression. +const SEMVER_PREFIX = /^\d+\.\d+\.\d+/; // mirrors src/update-context.cts SEMVER_PREFIX +function resolveVersionFrom(libDir: string): string { + try { + const v = fs.readFileSync(path.join(libDir, '..', '..', 'VERSION'), 'utf8').trim(); + if (SEMVER_PREFIX.test(v)) return v; + } catch { /* not an installed tree (no gsd-core/VERSION) */ } + try { + const pkg = require(path.join(libDir, '..', '..', '..', 'package.json')); + if (pkg && typeof pkg.version === 'string' && SEMVER_PREFIX.test(pkg.version)) return pkg.version; + } catch { /* runtime root has no package.json (e.g. Codex) */ } + return ''; +} + +let cachedVersion: string | undefined; +function gsdVersion(): string { + if (cachedVersion === undefined) cachedVersion = resolveVersionFrom(__dirname); + return cachedVersion; +} + const colorNameToHex = { cyan: '#00FFFF', @@ -393,7 +429,10 @@ function convertClaudeCommandToClaudeSkill(content, skillName, runtime = null, c // Hermes' SKILL.md spec lists `version` as a required frontmatter field. // Track GSD's package version so Hermes' skill_view() reports a stable // identifier per install. - if (runtime === 'hermes') fm += `version: ${yamlQuote(pkg.version)}\n`; + if (runtime === 'hermes') { + const version = gsdVersion(); + if (version) fm += `version: ${yamlQuote(version)}\n`; + } // #778 (b) — Qwen-only numeric priority for /skills ordering. Scoped to qwen // so Claude/Hermes skill frontmatter is unchanged (they ignore the field, but // we keep their output byte-stable). skillName is the `gsd-` dir name. @@ -2541,6 +2580,9 @@ export = { convertClaudeCommandToKiloSkill, readGsdCommandNames, transformContentToHyphen, + // #1383: version resolver (exported for regression test of the Codex + // missing-package.json crash + the VERSION-file source of truth). + resolveVersionFrom, // #1182: agent converters + tool-name table dependency closure claudeToCopilotTools, convertCopilotToolName, diff --git a/tests/hermes-skills-migration.test.cjs b/tests/hermes-skills-migration.test.cjs index 7cad0385e..92b0a9fa1 100644 --- a/tests/hermes-skills-migration.test.cjs +++ b/tests/hermes-skills-migration.test.cjs @@ -336,3 +336,61 @@ describe('Hermes Agent: SKILL.md format validation', () => { assert.strictEqual(fm.name, 'gsd-plan'); }); }); + +// ─── #1383 regression: version lookup must not require a runtime-root package.json ── +// The extracted conversion module sits in the gsd-tools loader chain, so its old +// top-level `require('../../../package.json')` crashed EVERY gsd-tools command on +// Codex — whose runtime root has no package.json — with +// `Cannot find module '../../../package.json'`. The Hermes `version:` field (the +// require's only consumer) must instead be sourced from the installed +// gsd-core/VERSION, lazily and defensively, so the module loads everywhere and +// the emitted version is a real semver, never `undefined`. +describe('#1383 regression: gsd-tools version lookup without a runtime-root package.json', () => { + // Require the EXTRACTED module that the gsd-tools chain loads (not install.js's + // in-process copy), to assert the crash path itself is gone. + const conversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); + + let tmp; + beforeEach(() => { tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1383-')); }); + afterEach(() => { cleanup(tmp); }); + + // Build a fake install layout /gsd-core/bin/lib and return that libDir. + // `version` writes /gsd-core/VERSION; `rootPkg` writes /package.json. + function layout({ version, rootPkg } = {}) { + const libDir = path.join(tmp, 'gsd-core', 'bin', 'lib'); + fs.mkdirSync(libDir, { recursive: true }); + if (version !== undefined) fs.writeFileSync(path.join(tmp, 'gsd-core', 'VERSION'), version); + if (rootPkg !== undefined) fs.writeFileSync(path.join(tmp, 'package.json'), JSON.stringify(rootPkg)); + return libDir; + } + + test('reads gsd-core/VERSION when the runtime root has no package.json (Codex layout)', () => { + const libDir = layout({ version: '9.9.9\n' }); // deliberately NO root package.json + assert.ok(!fs.existsSync(path.join(tmp, 'package.json')), + 'precondition: Codex layout has no runtime-root package.json'); + let v; + assert.doesNotThrow(() => { v = conversion.resolveVersionFrom(libDir); }, + 'version lookup must not throw on a layout without a runtime-root package.json'); + assert.strictEqual(v, '9.9.9', 'version is read (trimmed) from the installed VERSION file'); + }); + + test('falls back to the runtime-root package.json when no VERSION file exists (source/npm layout)', () => { + const libDir = layout({ rootPkg: { version: '1.2.3' } }); // no VERSION file + assert.strictEqual(conversion.resolveVersionFrom(libDir), '1.2.3', + 'source/npm tree has a real package.json three dirs up'); + }); + + test('degrades to "" (never throws, never emits undefined) when neither source exists', () => { + const libDir = layout({}); // neither VERSION nor package.json + let v; + assert.doesNotThrow(() => { v = conversion.resolveVersionFrom(libDir); }); + assert.strictEqual(v, '', 'no source -> empty string, so the caller omits the version field'); + }); + + test('rejects a non-semver VERSION file rather than emitting it verbatim', () => { + const libDir = layout({ version: 'not-a-version\n' }); // malformed, no package.json fallback + let v; + assert.doesNotThrow(() => { v = conversion.resolveVersionFrom(libDir); }); + assert.strictEqual(v, '', 'garbled VERSION is rejected, so the caller omits the field'); + }); +});