From 71f39eb77856fd92d01ca6423f6a87d628ec07ab Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 20:10:55 -0400 Subject: [PATCH 1/3] fix(3582): regression test for Codex install skill-materialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GSD 1.42.2 reported a successful Codex install but printed `Skipped Codex skill-copy generation (Codex discovers official skills directly)` and left users with no routable `$gsd-*` entrypoints in Codex CLI 0.130.0+. Confirmed reproducible on Windows 11 and macOS by three independent reporters. Root cause: Codex CLI 0.130.0 does NOT auto-discover commands from `~/.codex/get-shit-done/workflows/*.md` or `agents/*.md`. It only registers slash commands derived from `~/.codex/skills//SKILL.md`. The "Codex discovers official skills directly" assumption was wrong. The fix already shipped in #3562 — `bin/install.js` now calls `copyCommandsAsCodexSkills()` for the Codex install path, materializing one `SKILL.md` per `commands/gsd/*.md` with Codex-flavored frontmatter and the `` body. Verified locally: a Codex global install into a temp `CODEX_HOME` produces 67 `gsd-*/SKILL.md` files including `gsd-map-codebase` (the literal command from the bug report). This change adds the regression test the triage diagnose pass called for. It pins five facets of the contract so the 1.42.2 failure mode cannot silently come back: 1. `/skills/gsd-*/SKILL.md` exists for every shipped command (count matches `commands/gsd/**/*.md` exactly). 2. Each generated SKILL.md has YAML frontmatter with `name:` matching the directory and a non-empty `description:`. 3. Every SKILL.md body contains the `` block — without it Codex can't route `$gsd-` invocations. 4. The five representative commands named in the report and triage (`gsd-map-codebase`, `gsd-execute-phase`, `gsd-plan-phase`, `gsd-new-project`, `gsd-health`) are all present. 5. Installer output never prints "Skipped Codex skill-copy generation" or "Codex discovers official skills directly", and DOES print "Installed N skills to skills/" — locking the success-while-skipping anti-pattern out. Validation: - New test: 5/5 pass on Mac and Linux Docker. - Full suite (`gsd-test-both` per CLAUDE.md): 10,682/10,682 on Mac, 10,682/10,682 on Linux Docker, zero cross-platform diff. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/tidy-voles-rally.md | 5 + ...ug-3582-codex-skills-materialized.test.cjs | 297 ++++++++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 .changeset/tidy-voles-rally.md create mode 100644 tests/bug-3582-codex-skills-materialized.test.cjs diff --git a/.changeset/tidy-voles-rally.md b/.changeset/tidy-voles-rally.md new file mode 100644 index 000000000..4f84467d7 --- /dev/null +++ b/.changeset/tidy-voles-rally.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3582 +--- +**Locked Codex skill-materialization contract** — added a regression test for the GSD 1.42.2 failure where Codex global install completed successfully but printed `Skipped Codex skill-copy generation (Codex discovers official skills directly)` and left users with no routable `$gsd-*` entrypoints in Codex CLI 0.130.0+. The fix shipped in #3562 (Codex install now materializes `$CODEX_HOME/skills/gsd-/SKILL.md` for every shipped command); this test pins the contract so the regression cannot silently come back. diff --git a/tests/bug-3582-codex-skills-materialized.test.cjs b/tests/bug-3582-codex-skills-materialized.test.cjs new file mode 100644 index 000000000..d9f35e7b5 --- /dev/null +++ b/tests/bug-3582-codex-skills-materialized.test.cjs @@ -0,0 +1,297 @@ +/** + * Regression test for bug #3582 — Codex install must materialize the skill + * surface under `~/.codex/skills//SKILL.md`. + * + * Background: GSD 1.42.2 reported the user-visible failure + * > Skipped Codex skill-copy generation (Codex discovers official skills directly) + * which left users with a "successful" install but no routable `$gsd-*` + * entrypoints in Codex CLI 0.130.0. Codex CLI does NOT auto-discover + * commands from `~/.codex/get-shit-done/workflows/*.md` or `agents/*.md`; + * it only registers slash commands derived from `~/.codex/skills//SKILL.md`. + * The "Codex discovers official skills directly" assumption was wrong. + * + * The current installer (#3562 / current main) calls + * `copyCommandsAsCodexSkills()` to materialize one SKILL.md per + * commands/gsd/*.md, with Claude-flavored command frontmatter rewritten + * into Codex skill frontmatter (name, description, and the + * `` body). + * + * This test locks the install contract so the 1.42.2 regression cannot + * silently come back: + * + * 1. A Codex global install populates `/skills/` with one + * `gsd-/SKILL.md` per shipped command. + * 2. Each generated SKILL.md is non-empty, has YAML frontmatter, and + * declares the canonical hyphen-form `name:` matching its directory. + * 3. The installer never prints "Skipped Codex skill-copy generation" + * while reporting a successful install. + * 4. At least the commands the issue reporter and triage explicitly + * named (`gsd-map-codebase`, `gsd-execute-phase`, `gsd-plan-phase`, + * `gsd-new-project`) are present — proves the representative + * surface, not just an arbitrary count. + * + * Test invokes `install(isGlobal=true, runtime='codex')` directly with + * `CODEX_HOME` pointing at a temp dir so no developer config is touched. + */ + +'use strict'; + +// GSD_TEST_MODE neutralizes side-effecting branches (auto-detection, +// VS Code launches, etc.). Must be set BEFORE requiring bin/install.js; +// scoped to module load only so downstream tests in the same process +// don't see it. Mirrors the bug-2760 codex install harness. +const previousGsdTestMode = process.env.GSD_TEST_MODE; +process.env.GSD_TEST_MODE = '1'; + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); + +const { install } = require('../bin/install.js'); + +if (previousGsdTestMode === undefined) { + delete process.env.GSD_TEST_MODE; +} else { + process.env.GSD_TEST_MODE = previousGsdTestMode; +} + +const ROOT = path.join(__dirname, '..'); + +/** + * Run a Codex global install into a temp CODEX_HOME and capture stdout/stderr. + * Returns { codexHome, logs, warnings }. Caller is responsible for cleanup. + */ +function runCodexInstallCaptured() { + const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3582-codex-')); + const logs = []; + const warnings = []; + const origLog = console.log; + const origWarn = console.warn; + console.log = (...a) => { logs.push(a.join(' ')); }; + console.warn = (...a) => { warnings.push(a.join(' ')); }; + + const previousCodexHome = process.env.CODEX_HOME; + const previousCwd = process.cwd(); + process.env.CODEX_HOME = codexHome; + process.env.GSD_TEST_MODE = '1'; + try { + process.chdir(ROOT); + install(true, 'codex'); + } finally { + process.chdir(previousCwd); + console.log = origLog; + console.warn = origWarn; + if (previousCodexHome === undefined) { + delete process.env.CODEX_HOME; + } else { + process.env.CODEX_HOME = previousCodexHome; + } + if (previousGsdTestMode === undefined) { + delete process.env.GSD_TEST_MODE; + } else { + process.env.GSD_TEST_MODE = previousGsdTestMode; + } + } + return { codexHome, logs, warnings }; +} + +// Strip ANSI color codes so log assertions don't depend on TTY detection. +function stripAnsi(s) { + return s.replace(/\x1b\[[0-9;]*m/g, ''); +} + +// Parse SKILL.md frontmatter into a flat key→value map. Mirrors the shape +// used by tests/helpers.cjs and the install.js converters (single-line +// scalars only; the Codex converter does not emit block scalars). +function parseFrontmatter(content) { + const lines = content.split('\n'); + if (lines[0].trim() !== '---') { + throw new Error('expected --- on first line'); + } + let end = -1; + for (let i = 1; i < lines.length; i++) { + if (lines[i].trim() === '---') { end = i; break; } + } + if (end === -1) { + throw new Error('frontmatter not closed'); + } + const fm = {}; + for (let i = 1; i < end; i++) { + const line = lines[i]; + const m = line.match(/^([A-Za-z_][A-Za-z0-9_-]*):\s*(.*)$/); + if (!m) continue; + let val = m[2].trim(); + if (/^".*"$/.test(val)) { + try { val = JSON.parse(val); } catch { /* fall through */ } + } else if (/^'.*'$/.test(val)) { + val = val.slice(1, -1); + } + fm[m[1]] = val; + } + return fm; +} + +describe('bug-3582: Codex global install materializes the skill surface', () => { + let installRun; + + beforeEach(() => { + installRun = runCodexInstallCaptured(); + }); + + afterEach(() => { + if (installRun && installRun.codexHome) { + fs.rmSync(installRun.codexHome, { recursive: true, force: true }); + } + }); + + test('writes ~/.codex/skills/gsd-*/SKILL.md for every shipped command', () => { + const skillsDir = path.join(installRun.codexHome, 'skills'); + assert.ok( + fs.existsSync(skillsDir), + `Codex install must create ${skillsDir} (the 1.42.2 regression skipped this entirely)`, + ); + + const skillDirs = fs.readdirSync(skillsDir, { withFileTypes: true }) + .filter(e => e.isDirectory() && e.name.startsWith('gsd-')) + .map(e => e.name); + + assert.ok( + skillDirs.length > 0, + `expected at least one gsd-* skill directory under ${skillsDir}, got none`, + ); + + // Source-of-truth count: number of commands/gsd/*.md in the repo. Every + // command must produce a corresponding skill (recursing into subdirs). + const commandsDir = path.join(ROOT, 'commands', 'gsd'); + function countCommandMd(dir) { + let n = 0; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + n += countCommandMd(path.join(dir, entry.name)); + } else if (entry.name.endsWith('.md')) { + n += 1; + } + } + return n; + } + const expectedCount = countCommandMd(commandsDir); + assert.strictEqual( + skillDirs.length, + expectedCount, + `expected one skill per commands/gsd/*.md (${expectedCount}), got ${skillDirs.length}`, + ); + + // Every skill dir contains a SKILL.md file. Empty dirs or skill bodies + // would defeat Codex's slash-command registration as silently as the + // 1.42.2 "skipped" branch did. + for (const name of skillDirs) { + const skillMd = path.join(skillsDir, name, 'SKILL.md'); + assert.ok(fs.existsSync(skillMd), `missing SKILL.md for ${name}`); + const stat = fs.statSync(skillMd); + assert.ok(stat.isFile(), `${skillMd} must be a regular file`); + assert.ok(stat.size > 0, `${skillMd} must not be empty`); + } + }); + + test('SKILL.md frontmatter declares hyphen-form name matching the directory', () => { + const skillsDir = path.join(installRun.codexHome, 'skills'); + const skillDirs = fs.readdirSync(skillsDir, { withFileTypes: true }) + .filter(e => e.isDirectory() && e.name.startsWith('gsd-')) + .map(e => e.name); + + for (const name of skillDirs) { + const content = fs.readFileSync( + path.join(skillsDir, name, 'SKILL.md'), + 'utf-8', + ); + const fm = parseFrontmatter(content); + assert.strictEqual( + fm.name, + name, + `SKILL.md name field must match directory name for ${name} (got ${JSON.stringify(fm.name)})`, + ); + assert.ok( + typeof fm.description === 'string' && fm.description.length > 0, + `SKILL.md description must be a non-empty string for ${name}`, + ); + // Codex 0.130.0+ skill body must include the adapter header so the + // skill knows to map `$gsd-` → workflow execution. + assert.ok( + content.includes(''), + `${name}/SKILL.md must contain the body so Codex can route $gsd-`, + ); + } + }); + + test('representative skills named in the issue report are present', () => { + // The bug report and triage explicitly named these. Locking them as a + // representative set so a future regression that touches dispatch + // logic (filtering, profile resolution, etc.) cannot drop just the + // commands the original user was trying to run. + const representative = [ + 'gsd-map-codebase', // the literal command from the bug report + 'gsd-execute-phase', + 'gsd-plan-phase', + 'gsd-new-project', + 'gsd-health', + ]; + const skillsDir = path.join(installRun.codexHome, 'skills'); + for (const name of representative) { + const skillMd = path.join(skillsDir, name, 'SKILL.md'); + assert.ok( + fs.existsSync(skillMd), + `${name}/SKILL.md must exist after Codex install (was unrouteable in 1.42.2)`, + ); + } + }); + + test('installer success log mentions skills/ — never claims success while skipping', () => { + // Lock the contract that the 1.42.2 user-visible failure mode + // ("Skipped Codex skill-copy generation") can NEVER coexist with a + // success indicator. The fix in main prints "✓ Installed N skills"; + // the broken 1.42.2 branch printed "Skipped Codex skill-copy + // generation (Codex discovers official skills directly)" while + // leaving the user with no entrypoints. + const cleanLogs = installRun.logs.map(stripAnsi); + const cleanWarnings = installRun.warnings.map(stripAnsi); + const allOutput = [...cleanLogs, ...cleanWarnings].join('\n'); + + assert.ok( + !/Skipped Codex skill-copy generation/i.test(allOutput), + `installer must never print "Skipped Codex skill-copy generation" (the 1.42.2 failure mode). Output:\n${allOutput}`, + ); + assert.ok( + !/Codex discovers official skills directly/i.test(allOutput), + `installer must never claim "Codex discovers official skills directly" (the 1.42.2 incorrect assumption). Output:\n${allOutput}`, + ); + + // Positive proof — at least one log line acknowledges the skills install. + const hasSkillsInstalledLog = cleanLogs.some(line => /Installed\s+\d+\s+skills\s+to\s+skills\//.test(line)); + assert.ok( + hasSkillsInstalledLog, + `installer must print a success line of the form "Installed N skills to skills/". Logs:\n${cleanLogs.join('\n')}`, + ); + }); + + test('every generated SKILL.md file is on a Codex-discoverable path', () => { + // Codex 0.130.0 reads from CODEX_HOME/skills//SKILL.md. Any + // other path (e.g. CODEX_HOME/get-shit-done/workflows/*.md) is + // invisible to slash-command registration. Lock the literal layout + // here so a future "skills root" refactor cannot accidentally + // re-introduce the 1.42.2 "wrong directory" failure mode. + const skillsDir = path.join(installRun.codexHome, 'skills'); + const entries = fs.readdirSync(skillsDir, { withFileTypes: true }) + .filter(e => e.isDirectory() && e.name.startsWith('gsd-')); + + for (const dirent of entries) { + const expectedSkillMd = path.join(skillsDir, dirent.name, 'SKILL.md'); + const stat = fs.statSync(expectedSkillMd); + assert.ok( + stat.isFile() && stat.size > 0, + `Codex requires SKILL.md at exact path ${expectedSkillMd}; got ${stat.isFile() ? 'file size ' + stat.size : 'non-file'}`, + ); + } + }); +}); From 8e728fd2d663c13448164194292785dc783d868f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 20:21:12 -0400 Subject: [PATCH 2/3] fix(3582): address codex-review findings on PR #3609 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review surfaced 1 HIGH, 2 MED, 2 LOW; all addressed: 1. (HIGH, CONTRIBUTING.md violation) Adapter body assertion was raw text matching `content.includes('')` on the rendered SKILL.md. Replaced with a structural assertion against the exported builder: `content.includes(getCodexSkillAdapterHeader(name))`. The expected value is now the full closed adapter block produced by the production IR — open tag, body, and `` closing tag — so a truncated, empty, or missing-closing-tag adapter cannot satisfy the assertion. Pre-flight sanity-checks the builder itself emits the expected open/close shape. 2. (MED, sensitivity) Count-only check on installed skills was replaceable by a same-count partial install that swapped real commands for bogus `gsd-*` dirs. Replaced with `deepStrictEqual` on the sorted full set, computed from `commands/gsd/**/*.md` via a local `expectedSkillNames()` walker that mirrors the installer's naming rule (nested dirs collapse to `gsd--`). 3. (LOW, flakiness) `runCodexInstallCaptured()` could throw after creating the temp `codexHome` but before returning, so the describe-level `afterEach` couldn't see the path and the dir would leak. Added a try/catch around `install()` that cleans up the temp dir before rethrowing. 4. (LOW, flakiness) Harness mutates process-global `console.*`, `process.env`, `process.cwd()` — added `{ concurrency: false }` to the describe block, matching the existing convention in `tests/bug-3562-codex-install-skill-surface.test.cjs:45`. 5. (LOW, CONTRIBUTING.md helpers) Local `parseFrontmatter()` was duplicating the shared helper. Switched to the canonical `parseFrontmatter` exported by `tests/helpers.cjs`; also adopted `createTempDir` and `cleanup` from the same module for consistency. Validation: - New test still 5/5 pass. - Full suite via gsd-test-both: 10,682/10,682 on Mac AND Linux Docker, zero cross-platform diff. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...ug-3582-codex-skills-materialized.test.cjs | 240 ++++++++---------- 1 file changed, 111 insertions(+), 129 deletions(-) diff --git a/tests/bug-3582-codex-skills-materialized.test.cjs b/tests/bug-3582-codex-skills-materialized.test.cjs index d9f35e7b5..9a3e6514e 100644 --- a/tests/bug-3582-codex-skills-materialized.test.cjs +++ b/tests/bug-3582-codex-skills-materialized.test.cjs @@ -13,33 +13,21 @@ * The current installer (#3562 / current main) calls * `copyCommandsAsCodexSkills()` to materialize one SKILL.md per * commands/gsd/*.md, with Claude-flavored command frontmatter rewritten - * into Codex skill frontmatter (name, description, and the - * `` body). + * into Codex skill frontmatter and the `` body + * produced by `getCodexSkillAdapterHeader()`. * * This test locks the install contract so the 1.42.2 regression cannot - * silently come back: - * - * 1. A Codex global install populates `/skills/` with one - * `gsd-/SKILL.md` per shipped command. - * 2. Each generated SKILL.md is non-empty, has YAML frontmatter, and - * declares the canonical hyphen-form `name:` matching its directory. - * 3. The installer never prints "Skipped Codex skill-copy generation" - * while reporting a successful install. - * 4. At least the commands the issue reporter and triage explicitly - * named (`gsd-map-codebase`, `gsd-execute-phase`, `gsd-plan-phase`, - * `gsd-new-project`) are present — proves the representative - * surface, not just an arbitrary count. - * - * Test invokes `install(isGlobal=true, runtime='codex')` directly with - * `CODEX_HOME` pointing at a temp dir so no developer config is touched. + * silently come back. It asserts the full expected skill-name set + * (deepStrictEqual, not just count), the full adapter block (using + * the exported `getCodexSkillAdapterHeader` IR as the expected value, + * not raw substring search), and the success/skip log invariant. */ 'use strict'; -// GSD_TEST_MODE neutralizes side-effecting branches (auto-detection, -// VS Code launches, etc.). Must be set BEFORE requiring bin/install.js; -// scoped to module load only so downstream tests in the same process -// don't see it. Mirrors the bug-2760 codex install harness. +// GSD_TEST_MODE neutralizes side-effecting branches (auto-detection, etc.). +// Must be set BEFORE requiring bin/install.js; scoped to module load only +// so downstream tests don't see it. Mirrors the bug-2760 codex harness. const previousGsdTestMode = process.env.GSD_TEST_MODE; process.env.GSD_TEST_MODE = '1'; @@ -49,7 +37,8 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); -const { install } = require('../bin/install.js'); +const { install, getCodexSkillAdapterHeader } = require('../bin/install.js'); +const { parseFrontmatter, createTempDir, cleanup } = require('./helpers.cjs'); if (previousGsdTestMode === undefined) { delete process.env.GSD_TEST_MODE; @@ -58,13 +47,42 @@ if (previousGsdTestMode === undefined) { } const ROOT = path.join(__dirname, '..'); +const COMMANDS_DIR = path.join(ROOT, 'commands', 'gsd'); + +// Strip ANSI color codes so log assertions don't depend on TTY detection. +function stripAnsi(s) { + return s.replace(/\x1b\[[0-9;]*m/g, ''); +} + +/** + * Walk commands/gsd/**\/*.md and return the set of skill names the installer + * is contractually obligated to produce. Naming rule mirrors + * `copyCommandsAsCodexSkills` in bin/install.js: nested dirs collapse to + * `gsd--` with the .md stripped. + */ +function expectedSkillNames() { + const names = new Set(); + function recurse(dir, prefix) { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + recurse(path.join(dir, entry.name), `${prefix}-${entry.name}`); + } else if (entry.name.endsWith('.md')) { + const base = entry.name.slice(0, -3); + names.add(`${prefix}-${base}`); + } + } + } + recurse(COMMANDS_DIR, 'gsd'); + return names; +} /** * Run a Codex global install into a temp CODEX_HOME and capture stdout/stderr. - * Returns { codexHome, logs, warnings }. Caller is responsible for cleanup. + * Cleans up codexHome on throw so a partial-install failure never leaks + * temp directories. */ function runCodexInstallCaptured() { - const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3582-codex-')); + const codexHome = createTempDir('gsd-3582-codex-'); const logs = []; const warnings = []; const origLog = console.log; @@ -79,6 +97,12 @@ function runCodexInstallCaptured() { try { process.chdir(ROOT); install(true, 'codex'); + return { codexHome, logs, warnings }; + } catch (err) { + // Always reclaim the temp dir if install throws — otherwise the + // describe-level afterEach can't see codexHome and it leaks. + try { cleanup(codexHome); } catch { /* best-effort */ } + throw err; } finally { process.chdir(previousCwd); console.log = origLog; @@ -94,46 +118,11 @@ function runCodexInstallCaptured() { process.env.GSD_TEST_MODE = previousGsdTestMode; } } - return { codexHome, logs, warnings }; } -// Strip ANSI color codes so log assertions don't depend on TTY detection. -function stripAnsi(s) { - return s.replace(/\x1b\[[0-9;]*m/g, ''); -} - -// Parse SKILL.md frontmatter into a flat key→value map. Mirrors the shape -// used by tests/helpers.cjs and the install.js converters (single-line -// scalars only; the Codex converter does not emit block scalars). -function parseFrontmatter(content) { - const lines = content.split('\n'); - if (lines[0].trim() !== '---') { - throw new Error('expected --- on first line'); - } - let end = -1; - for (let i = 1; i < lines.length; i++) { - if (lines[i].trim() === '---') { end = i; break; } - } - if (end === -1) { - throw new Error('frontmatter not closed'); - } - const fm = {}; - for (let i = 1; i < end; i++) { - const line = lines[i]; - const m = line.match(/^([A-Za-z_][A-Za-z0-9_-]*):\s*(.*)$/); - if (!m) continue; - let val = m[2].trim(); - if (/^".*"$/.test(val)) { - try { val = JSON.parse(val); } catch { /* fall through */ } - } else if (/^'.*'$/.test(val)) { - val = val.slice(1, -1); - } - fm[m[1]] = val; - } - return fm; -} - -describe('bug-3582: Codex global install materializes the skill surface', () => { +// concurrency:false — harness mutates console.* / process.env / process.cwd(). +// Matches the convention used by tests/bug-3562-codex-install-skill-surface.test.cjs. +describe('bug-3582: Codex global install materializes the skill surface', { concurrency: false }, () => { let installRun; beforeEach(() => { @@ -142,53 +131,36 @@ describe('bug-3582: Codex global install materializes the skill surface', () => afterEach(() => { if (installRun && installRun.codexHome) { - fs.rmSync(installRun.codexHome, { recursive: true, force: true }); + cleanup(installRun.codexHome); } }); - test('writes ~/.codex/skills/gsd-*/SKILL.md for every shipped command', () => { + test('writes the exact expected set of gsd-*/SKILL.md skills (deepEqual on name set)', () => { const skillsDir = path.join(installRun.codexHome, 'skills'); assert.ok( fs.existsSync(skillsDir), `Codex install must create ${skillsDir} (the 1.42.2 regression skipped this entirely)`, ); - const skillDirs = fs.readdirSync(skillsDir, { withFileTypes: true }) + const actualNames = fs.readdirSync(skillsDir, { withFileTypes: true }) .filter(e => e.isDirectory() && e.name.startsWith('gsd-')) .map(e => e.name); - assert.ok( - skillDirs.length > 0, - `expected at least one gsd-* skill directory under ${skillsDir}, got none`, + // deepStrictEqual on the sorted full set — not just count — so a + // partial install that drops a real command and substitutes a bogus + // same-count `gsd-*` directory cannot pass. + const expected = [...expectedSkillNames()].sort(); + assert.deepStrictEqual( + [...actualNames].sort(), + expected, + `installed Codex skills must exactly match commands/gsd/**/*.md (one skill per command)`, ); - // Source-of-truth count: number of commands/gsd/*.md in the repo. Every - // command must produce a corresponding skill (recursing into subdirs). - const commandsDir = path.join(ROOT, 'commands', 'gsd'); - function countCommandMd(dir) { - let n = 0; - for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { - if (entry.isDirectory()) { - n += countCommandMd(path.join(dir, entry.name)); - } else if (entry.name.endsWith('.md')) { - n += 1; - } - } - return n; - } - const expectedCount = countCommandMd(commandsDir); - assert.strictEqual( - skillDirs.length, - expectedCount, - `expected one skill per commands/gsd/*.md (${expectedCount}), got ${skillDirs.length}`, - ); - - // Every skill dir contains a SKILL.md file. Empty dirs or skill bodies - // would defeat Codex's slash-command registration as silently as the - // 1.42.2 "skipped" branch did. - for (const name of skillDirs) { + // Every skill dir contains a non-empty SKILL.md file. Empty dirs or + // empty SKILL.md bodies would defeat Codex's slash-command + // registration as silently as the 1.42.2 "skipped" branch did. + for (const name of actualNames) { const skillMd = path.join(skillsDir, name, 'SKILL.md'); - assert.ok(fs.existsSync(skillMd), `missing SKILL.md for ${name}`); const stat = fs.statSync(skillMd); assert.ok(stat.isFile(), `${skillMd} must be a regular file`); assert.ok(stat.size > 0, `${skillMd} must not be empty`); @@ -206,6 +178,8 @@ describe('bug-3582: Codex global install materializes the skill surface', () => path.join(skillsDir, name, 'SKILL.md'), 'utf-8', ); + // Uses the shared `parseFrontmatter` from tests/helpers.cjs per the + // CONTRIBUTING.md "tests parse, never grep" convention. const fm = parseFrontmatter(content); assert.strictEqual( fm.name, @@ -216,20 +190,49 @@ describe('bug-3582: Codex global install materializes the skill surface', () => typeof fm.description === 'string' && fm.description.length > 0, `SKILL.md description must be a non-empty string for ${name}`, ); - // Codex 0.130.0+ skill body must include the adapter header so the - // skill knows to map `$gsd-` → workflow execution. + } + }); + + test('SKILL.md body contains the full block produced by the exported builder', () => { + // Structural check against the production builder's output — NOT a + // raw substring grep on the rendered file. `getCodexSkillAdapterHeader` + // is the typed IR exported by bin/install.js (#3582 PR #3609 codex + // review); the file on disk must contain its full output verbatim + // (open tag, body, closing ``). A truncated, + // empty, or missing-closing-tag adapter cannot satisfy this assertion. + const skillsDir = path.join(installRun.codexHome, 'skills'); + const skillDirs = fs.readdirSync(skillsDir, { withFileTypes: true }) + .filter(e => e.isDirectory() && e.name.startsWith('gsd-')) + .map(e => e.name); + + for (const name of skillDirs) { + const expectedAdapter = getCodexSkillAdapterHeader(name); + // Sanity: the builder itself must produce a closed block for the + // assertion below to be meaningful. assert.ok( - content.includes(''), - `${name}/SKILL.md must contain the body so Codex can route $gsd-`, + expectedAdapter.startsWith(''), + `getCodexSkillAdapterHeader(${name}) must start with the opening tag`, + ); + assert.ok( + expectedAdapter.trimEnd().endsWith(''), + `getCodexSkillAdapterHeader(${name}) must end with the closing tag`, + ); + + const content = fs.readFileSync( + path.join(skillsDir, name, 'SKILL.md'), + 'utf-8', + ); + assert.ok( + content.includes(expectedAdapter), + `${name}/SKILL.md must contain the full adapter block produced by getCodexSkillAdapterHeader(${name}); Codex routes $${name} via this exact body`, ); } }); test('representative skills named in the issue report are present', () => { // The bug report and triage explicitly named these. Locking them as a - // representative set so a future regression that touches dispatch - // logic (filtering, profile resolution, etc.) cannot drop just the - // commands the original user was trying to run. + // representative set so a future dispatch / filter / profile change + // cannot drop just the commands the original user was trying to run. const representative = [ 'gsd-map-codebase', // the literal command from the bug report 'gsd-execute-phase', @@ -248,23 +251,22 @@ describe('bug-3582: Codex global install materializes the skill surface', () => }); test('installer success log mentions skills/ — never claims success while skipping', () => { - // Lock the contract that the 1.42.2 user-visible failure mode - // ("Skipped Codex skill-copy generation") can NEVER coexist with a - // success indicator. The fix in main prints "✓ Installed N skills"; - // the broken 1.42.2 branch printed "Skipped Codex skill-copy - // generation (Codex discovers official skills directly)" while - // leaving the user with no entrypoints. + // The 1.42.2 user-visible failure mode was a successful install that + // printed "Skipped Codex skill-copy generation (Codex discovers + // official skills directly)" while leaving the user with no + // entrypoints. Lock that the broken strings can NEVER coexist with a + // success indicator. Current main prints "✓ Installed N skills". const cleanLogs = installRun.logs.map(stripAnsi); const cleanWarnings = installRun.warnings.map(stripAnsi); const allOutput = [...cleanLogs, ...cleanWarnings].join('\n'); assert.ok( !/Skipped Codex skill-copy generation/i.test(allOutput), - `installer must never print "Skipped Codex skill-copy generation" (the 1.42.2 failure mode). Output:\n${allOutput}`, + `installer must never print "Skipped Codex skill-copy generation" (1.42.2 failure). Output:\n${allOutput}`, ); assert.ok( !/Codex discovers official skills directly/i.test(allOutput), - `installer must never claim "Codex discovers official skills directly" (the 1.42.2 incorrect assumption). Output:\n${allOutput}`, + `installer must never claim "Codex discovers official skills directly" (1.42.2 incorrect assumption). Output:\n${allOutput}`, ); // Positive proof — at least one log line acknowledges the skills install. @@ -274,24 +276,4 @@ describe('bug-3582: Codex global install materializes the skill surface', () => `installer must print a success line of the form "Installed N skills to skills/". Logs:\n${cleanLogs.join('\n')}`, ); }); - - test('every generated SKILL.md file is on a Codex-discoverable path', () => { - // Codex 0.130.0 reads from CODEX_HOME/skills//SKILL.md. Any - // other path (e.g. CODEX_HOME/get-shit-done/workflows/*.md) is - // invisible to slash-command registration. Lock the literal layout - // here so a future "skills root" refactor cannot accidentally - // re-introduce the 1.42.2 "wrong directory" failure mode. - const skillsDir = path.join(installRun.codexHome, 'skills'); - const entries = fs.readdirSync(skillsDir, { withFileTypes: true }) - .filter(e => e.isDirectory() && e.name.startsWith('gsd-')); - - for (const dirent of entries) { - const expectedSkillMd = path.join(skillsDir, dirent.name, 'SKILL.md'); - const stat = fs.statSync(expectedSkillMd); - assert.ok( - stat.isFile() && stat.size > 0, - `Codex requires SKILL.md at exact path ${expectedSkillMd}; got ${stat.isFile() ? 'file size ' + stat.size : 'non-file'}`, - ); - } - }); }); From 8bc4148eea0e738946a0ed4d451b5475e59a06ca Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 22:56:32 -0400 Subject: [PATCH 3/3] test(3582): annotate adapter text assertion for source-grep lint --- tests/bug-3582-codex-skills-materialized.test.cjs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/bug-3582-codex-skills-materialized.test.cjs b/tests/bug-3582-codex-skills-materialized.test.cjs index 9a3e6514e..52d9399b8 100644 --- a/tests/bug-3582-codex-skills-materialized.test.cjs +++ b/tests/bug-3582-codex-skills-materialized.test.cjs @@ -22,6 +22,9 @@ * the exported `getCodexSkillAdapterHeader` IR as the expected value, * not raw substring search), and the success/skip log invariant. */ +// allow-test-rule: source-text-is-the-product +// This assertion validates the generated adapter block that is shipped to +// users in SKILL.md; matching exact emitted text is the contract under test. 'use strict';