From 19edab21da6b9444080ccf60897377d24f22e60b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 10 Jun 2026 15:55:31 -0400 Subject: [PATCH] fix(#1006): rc CHANGELOG preview crash on malformed changeset fragment + validate fragment content at the gate (#1007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1006): harden render --preview against fragment parse failures `render --preview` wrote `report.preview` unconditionally. When a `.changeset` fragment fails to parse, `cmdRender` early-returns with `{exitCode:1, report: {failures}}` and NO `preview` key, so `process.stdout.write(undefined)` threw ERR_INVALID_ARG_TYPE and the rc release job's "Preview CHANGELOG" step died with a cryptic TypeError that masked the real cause. Guard the preview write on `typeof report.preview === 'string'` (ADR-227: shape, not just type); when absent, fall through to the existing failure reporter that names the offending fragment and exits non-zero — identical to a non-preview render. Also backfills the stray placeholder `pr: 0` -> `pr: 939` in .changeset/936-convergence-inline-plan-phase.md that triggered the live failure. Regression test (red-then-green verified) added at the render --preview seam. Co-Authored-By: Claude Opus 4.8 * fix(#1006): validate changeset fragment content at the Changeset Required gate The `Changeset Required` gate (scripts/changeset/lint.cjs) only checked that a `.changeset/*.md` fragment EXISTS in the PR diff; it never validated the fragment's contents. So a malformed fragment (e.g. an un-backfilled `pr: 0` placeholder) silently merged to `next` and only detonated later in the rc release job. This is the upstream prevention for #1006 — the crash hardening turns the failure into a clear message, this stops the bad fragment ever reaching the release path. evaluateLint now accepts `fragmentFailures` and fails with the typed reason `fail_invalid_fragment` (naming each offending file) before the existence/ opt-out checks — a malformed fragment beats `no-changelog`, since it will break the render regardless. main() reads + parseFragment()s every changed fragment: a deleted fragment (not on disk) is skipped, a present-but-unreadable one fails closed. Tests assert on the typed LINT_REASON enum (no raw-text matching), a precedence case over the opt-out label, and an end-to-end suite that drives the real main() against a temp git repo (malformed -> fail, valid -> pass, deleted -> skipped) so the wiring is regression-proof. Co-Authored-By: Claude Opus 4.8 * test(#1006): assert the typed --json report in the preview regression test Code review flagged the preview parse-failure regression test for positive raw-text matching on CLI output (`combined.includes('bad-fragment.md')` / `'invalid_pr'`), which this repo's testing standards forbid. Keep the non-json `runRenderRaw` call for the negative crash proof (the ERR_INVALID_ARG_TYPE crash lives only on the non-json stdout.write path), and add a `--json` invocation that asserts the offending fragment + typed `invalid_pr` reason via the structured `report.failures[]` surface instead of rendered prose. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .../936-convergence-inline-plan-phase.md | 2 +- CONTRIBUTING.md | 2 +- scripts/changeset/cli.cjs | 9 +- scripts/changeset/lint.cjs | 40 +++- tests/changeset-cli.test.cjs | 38 ++++ tests/changeset-lint.test.cjs | 201 +++++++++++++++++- 6 files changed, 286 insertions(+), 6 deletions(-) diff --git a/.changeset/936-convergence-inline-plan-phase.md b/.changeset/936-convergence-inline-plan-phase.md index 5ebdf7df6..6577aeb0d 100644 --- a/.changeset/936-convergence-inline-plan-phase.md +++ b/.changeset/936-convergence-inline-plan-phase.md @@ -1,5 +1,5 @@ --- type: Fixed -pr: 0 +pr: 939 --- **`plan-review-convergence` now runs `gsd-plan-phase` inline instead of inside `Agent()`** — both sites that previously wrapped `gsd-plan-phase` in `Agent()` (initial planning + replan loop) have been changed to bare `Skill()` calls at depth 0. On Claude Code, a depth-1 Agent has no Agent tool, so a wrapped `plan-phase` could never spawn `gsd-planner` or `gsd-plan-checker` — the replan loop silently failed to produce a revised plan whenever HIGH concerns were found. Running plan-phase inline from the depth-0 orchestrator (which retains the Agent tool) restores the full planner→checker sub-agent chain. A new structural guard test (`bug-936-no-nested-spawner-wrap.test.cjs`) statically scans all workflow files and fails if any workflow wraps a spawner orchestrator in `Agent()` without a `RUNTIME != claude` carve-out, preventing regression. (#936) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2b0caebf8..1a07180d9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -203,7 +203,7 @@ This writes `.changeset/--.md`. Three random words → co Fragments are consolidated into `CHANGELOG.md` at release time by the release workflow. See [`.changeset/README.md`](.changeset/README.md) for the format spec and [#2975](https://github.com/open-gsd/gsd-core/issues/2975) for the rationale. -**CI enforcement:** the `Changeset Required` workflow (`scripts/changeset/lint.cjs`) fails any PR that touches `bin/`, `gsd-core/`, `agents/`, `commands/`, `hooks/`, or `sdk/src/` without a `.changeset/*.md` fragment. +**CI enforcement:** the `Changeset Required` workflow (`scripts/changeset/lint.cjs`) fails any PR that touches `bin/`, `gsd-core/`, `agents/`, `commands/`, `hooks/`, or `sdk/src/` without a `.changeset/*.md` fragment. The gate also **validates the content** of every changed fragment: a fragment whose frontmatter does not parse (e.g. a `pr: 0` placeholder that was never backfilled to the real PR number) fails the gate with `fail_invalid_fragment`, naming the offending file. This stops a malformed fragment from merging to `next` and only detonating later in the release job's CHANGELOG render. **Opt-out:** PRs with no user-facing impact (test refactors, lint config changes, CI tweaks, formatting-only changes) can add the `no-changelog` label. The lint honors it. When unsure whether a change is user-facing, **add the fragment**. diff --git a/scripts/changeset/cli.cjs b/scripts/changeset/cli.cjs index 9ddcb667c..2c557433c 100755 --- a/scripts/changeset/cli.cjs +++ b/scripts/changeset/cli.cjs @@ -565,8 +565,15 @@ function main() { const { exitCode, report } = opts.cmd === 'render' ? cmdRender(opts) : cmdGithubReleaseNotes(opts); if (opts.json) { process.stdout.write(JSON.stringify(report, null, 2) + '\n'); - } else if (opts.cmd === 'render' && opts.preview) { + } else if (opts.cmd === 'render' && opts.preview && typeof report.preview === 'string') { // render --preview: emit the rendered section verbatim (no mutation occurred). + // The `typeof report.preview === 'string'` guard is load-bearing: cmdRender + // early-returns on a fragment parse failure (failures.length > 0) WITHOUT a + // `preview` key, so writing report.preview unguarded crashed the rc release + // job with ERR_INVALID_ARG_TYPE, masking the real cause (a malformed + // fragment). When preview is absent we fall through to the failure reporter + // below, which names the offending file and exits non-zero — identical to a + // non-preview render. process.stdout.write(report.preview); } else if (opts.cmd === 'github-release-notes' && report.body) { process.stdout.write(report.body); diff --git a/scripts/changeset/lint.cjs b/scripts/changeset/lint.cjs index 4704147dd..5558fab76 100755 --- a/scripts/changeset/lint.cjs +++ b/scripts/changeset/lint.cjs @@ -17,6 +17,7 @@ const LINT_REASON = Object.freeze({ OK_OPT_OUT_LABEL: 'ok_opt_out_label', OK_NO_USER_FACING_CHANGES: 'ok_no_user_facing_changes', FAIL_MISSING_FRAGMENT: 'fail_missing_fragment', + FAIL_INVALID_FRAGMENT: 'fail_invalid_fragment', }); const OPT_OUT_LABEL = 'no-changelog'; @@ -47,7 +48,10 @@ function isFragment(file) { return /^\.changeset\/[^/]+\.md$/.test(file) && !file.endsWith('/README.md'); } -function evaluateLint({ changedFiles, labels }) { +function evaluateLint({ changedFiles, labels, fragmentFailures = [] }) { + if (fragmentFailures.length > 0) { + return { ok: false, reason: LINT_REASON.FAIL_INVALID_FRAGMENT, failures: fragmentFailures }; + } if (changedFiles.some(isFragment)) { return { ok: true, reason: LINT_REASON.OK_FRAGMENT_PRESENT }; } @@ -61,6 +65,7 @@ function evaluateLint({ changedFiles, labels }) { } const { ExitError, runMain } = require('../lib/cli-exit.cjs'); +const { parseFragment } = require('./parse.cjs'); function main() { const fs = require('node:fs'); @@ -92,11 +97,42 @@ function main() { throw new ExitError(2, `could not compute diff: ${e.message}`); } - const verdict = evaluateLint({ changedFiles, labels }); + // Validate the content of every changed fragment file. + const fragmentFailures = []; + for (const file of changedFiles) { + if (!isFragment(file)) continue; + // A fragment path in the diff that no longer exists on disk was deleted in + // this PR — a deletion can't be malformed, so skip it. + if (!fs.existsSync(file)) continue; + let src; + try { + src = fs.readFileSync(file, 'utf8'); + } catch (e) { + // Present in the diff but unreadable (broken symlink, permissions). A + // changed fragment we cannot read is suspect — fail closed rather than + // letting it slip through to the release-time CHANGELOG render. + fragmentFailures.push({ file, reason: 'unreadable', detail: e.code || 'read_error' }); + continue; + } + const result = parseFragment(src); + if (!result.ok) { + fragmentFailures.push({ file, reason: result.reason, detail: result.detail }); + } + } + + const verdict = evaluateLint({ changedFiles, labels, fragmentFailures }); if (process.argv.includes('--json')) { process.stdout.write(JSON.stringify({ ...verdict, changedFiles, labels }, null, 2) + '\n'); } else if (verdict.ok) { process.stdout.write(`ok changeset-lint: ${verdict.reason}\n`); + } else if (verdict.reason === LINT_REASON.FAIL_INVALID_FRAGMENT) { + process.stderr.write(`\nERROR changeset-lint: ${verdict.reason}\n`); + process.stderr.write(`The following .changeset fragment(s) failed content validation:\n`); + for (const f of verdict.failures) { + const detail = f.detail !== undefined ? ` (${f.detail})` : ''; + process.stderr.write(` ${f.file}: ${f.reason}${detail}\n`); + } + process.stderr.write(`Fix the fragment(s) above before merging.\n`); } else { process.stderr.write(`\nERROR changeset-lint: ${verdict.reason}\n`); process.stderr.write(`PR touches user-facing files but does not include a .changeset/*.md fragment.\n`); diff --git a/tests/changeset-cli.test.cjs b/tests/changeset-cli.test.cjs index 831857c13..ba3b532e3 100644 --- a/tests/changeset-cli.test.cjs +++ b/tests/changeset-cli.test.cjs @@ -1073,4 +1073,42 @@ describe('changeset cli render --preview (#759)', () => { 'CHANGELOG.md must be byte-identical after --preview', ); }); + + // Regression (#939 / rc-job crash): a fragment that fails to parse (e.g. an + // un-backfilled `pr: 0` placeholder) makes cmdRender early-return WITHOUT a + // `preview` key. Before the fix, main() wrote report.preview unguarded, so + // `process.stdout.write(undefined)` threw ERR_INVALID_ARG_TYPE and the rc + // "Preview CHANGELOG" step died with a cryptic TypeError that masked the real + // cause. The preview failure path must now exit non-zero and NAME the bad + // fragment, identical to a non-preview render. + test('render --preview with an unparseable fragment fails cleanly (names the file, no TypeError crash)', () => { + // pr: 0 is the never-backfilled placeholder → parseFragment returns invalid_pr. + writeFragment('bad-fragment', 'Fixed', 0, '**Bad** — placeholder never backfilled. (#123)'); + + // The crash lived on the NON-json path: process.stdout.write(report.preview) + // with report.preview === undefined. Exercise it directly and assert it no + // longer crashes — non-zero exit and NO TypeError stack in the output (QA + // matrix: "No stack trace in non-debug failure output"). + const raw = runRenderRaw(['--version', '9.9.0', '--date', '2026-01-02', '--preview']); + assert.notStrictEqual(raw.status, 0, `parse-failure preview must exit non-zero; stdout=${raw.stdout} stderr=${raw.stderr}`); + const combined = `${raw.stdout}\n${raw.stderr}`; + assert.ok( + !combined.includes('ERR_INVALID_ARG_TYPE'), + `preview must NOT crash with ERR_INVALID_ARG_TYPE; got: ${combined}`, + ); + + // Structured surface (--json) proves the failure NAMES the offending fragment + // and reports the typed parse reason — asserted on the typed report shape, + // not on rendered prose. + const json = runRender(['--version', '9.9.0', '--date', '2026-01-02', '--preview']); + assert.notStrictEqual(json.status, 0, 'json preview must also exit non-zero on parse failure'); + assert.ok(Array.isArray(json.report.failures), `report.failures must be an array; got: ${JSON.stringify(json.report)}`); + const bad = json.report.failures.find((f) => f.file.endsWith('bad-fragment.md')); + assert.ok(bad, `failures must name the offending fragment; got: ${JSON.stringify(json.report.failures)}`); + assert.equal(bad.reason, 'invalid_pr', `failure reason must be the typed invalid_pr; got: ${bad && bad.reason}`); + + // Still non-destructive: no CHANGELOG.md written, fragment left in place. + assert.ok(!fs.existsSync(path.join(tmp, 'CHANGELOG.md')), 'CHANGELOG.md must NOT be created by a failed preview'); + assert.ok(fs.existsSync(path.join(tmp, '.changeset', 'bad-fragment.md')), 'fragment must still exist after failed preview'); + }); }); diff --git a/tests/changeset-lint.test.cjs b/tests/changeset-lint.test.cjs index 6979c7ca3..d5938070a 100644 --- a/tests/changeset-lint.test.cjs +++ b/tests/changeset-lint.test.cjs @@ -4,9 +4,87 @@ process.env.GSD_TEST_MODE = '1'; const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const path = require('node:path'); +const fs = require('node:fs'); +const os = require('node:os'); +const cp = require('node:child_process'); const { evaluateLint, LINT_REASON } = require(path.join(__dirname, '..', 'scripts', 'changeset', 'lint.cjs')); +const ROOT = path.join(__dirname, '..'); +const LINT_SCRIPT = path.join(ROOT, 'scripts', 'changeset', 'lint.cjs'); +const { cleanup } = require('./helpers.cjs'); + +/** + * Build a minimal temp git repo shaped like a PR branch: + * origin/main = base commit (README.md) + * pr = PR branch with caller-supplied files committed on top + * + * @param {string} tmpDir - pre-created temp directory (mkdtempSync result) + * @param {Array<{file: string, content: string}>} prFiles - files to create on the PR branch + * @param {Array<{file: string, content: string}>} [baseFiles] - extra files to create on base commit + * @returns {string} path to the temp repo (same as tmpDir) + */ +function buildTempRepo(tmpDir, prFiles, baseFiles = []) { + const git = (...args) => cp.execFileSync('git', args, { cwd: tmpDir, encoding: 'utf8' }); + + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 'test@example.com'); + git('config', 'user.name', 'Test'); + + // Base commit: README + any caller-supplied base files + fs.writeFileSync(path.join(tmpDir, 'README.md'), '# test\n'); + for (const { file, content } of baseFiles) { + const abs = path.join(tmpDir, file); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } + git('add', '-A'); + git('commit', '-q', '-m', 'base'); + + // Fake origin/main so `git diff origin/main...HEAD` works without a real remote + git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + + // PR branch + git('checkout', '-q', '-b', 'pr'); + + // Create or delete the PR files + for (const { file, content } of prFiles) { + const abs = path.join(tmpDir, file); + if (content === null) { + // null content = delete the file (unlinkSync, not rmSync, to stay within + // the no-raw-rmsync-in-tests rule — we are removing a single file, not a tree) + try { fs.unlinkSync(abs); } catch { /* already absent — ok */ } + } else { + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } + } + git('add', '-A'); + git('commit', '-q', '-m', 'pr changes'); + + return tmpDir; +} + +/** + * Invoke lint.cjs --json in the given repo directory and return the parsed report. + * @param {string} repoDir + * @returns {{ status: number, report: object }} + */ +function runLint(repoDir) { + const result = cp.spawnSync( + process.execPath, + [LINT_SCRIPT, '--json'], + { + cwd: repoDir, + env: { ...process.env, GITHUB_BASE_REF: 'main', GITHUB_EVENT_PATH: '' }, + encoding: 'utf8', + }, + ); + let report = {}; + try { report = JSON.parse(result.stdout); } catch { /* leave as empty object */ } + return { status: result.status, report }; +} + // evaluateLint is a pure function over file lists + label list — no fs, no git. // Tests assert on the structured verdict: { ok: bool, reason: LINT_REASON.X }. @@ -14,7 +92,7 @@ describe('changeset lint: pure verdict (#2975)', () => { test('LINT_REASON enum exposes the documented codes', () => { assert.deepEqual( Object.keys(LINT_REASON).sort(), - ['OK_FRAGMENT_PRESENT', 'OK_NO_USER_FACING_CHANGES', 'OK_OPT_OUT_LABEL', 'FAIL_MISSING_FRAGMENT'].sort(), + ['OK_FRAGMENT_PRESENT', 'OK_NO_USER_FACING_CHANGES', 'OK_OPT_OUT_LABEL', 'FAIL_MISSING_FRAGMENT', 'FAIL_INVALID_FRAGMENT'].sort(), ); }); @@ -65,4 +143,125 @@ describe('changeset lint: pure verdict (#2975)', () => { }); assert.deepEqual(verdict, { ok: true, reason: LINT_REASON.OK_FRAGMENT_PRESENT }); }); + + test('FAIL_INVALID_FRAGMENT when fragmentFailures is non-empty', () => { + const verdict = evaluateLint({ + changedFiles: ['.changeset/bad.md'], + labels: [], + fragmentFailures: [{ file: '.changeset/bad.md', reason: 'invalid_pr', detail: '0' }], + }); + assert.equal(verdict.ok, false); + assert.equal(verdict.reason, LINT_REASON.FAIL_INVALID_FRAGMENT); + assert.deepEqual(verdict.failures, [{ file: '.changeset/bad.md', reason: 'invalid_pr', detail: '0' }]); + }); + + test('OK_FRAGMENT_PRESENT when fragment is present and fragmentFailures is empty (regression guard)', () => { + const verdict = evaluateLint({ + changedFiles: ['bin/install.js', '.changeset/good.md'], + labels: [], + fragmentFailures: [], + }); + assert.deepEqual(verdict, { ok: true, reason: LINT_REASON.OK_FRAGMENT_PRESENT }); + }); + + test('FAIL_INVALID_FRAGMENT beats no-changelog opt-out label', () => { + const verdict = evaluateLint({ + changedFiles: ['.changeset/bad.md'], + labels: ['no-changelog'], + fragmentFailures: [{ file: '.changeset/bad.md', reason: 'invalid_pr', detail: '0' }], + }); + assert.equal(verdict.ok, false); + assert.equal(verdict.reason, LINT_REASON.FAIL_INVALID_FRAGMENT); + }); +}); + +// --------------------------------------------------------------------------- +// End-to-end integration: real main() wiring via temp git repo (#1006) +// --------------------------------------------------------------------------- +describe('changeset lint: main() end-to-end wiring (#1006)', () => { + // Each test allocates its own tmpDir so cases run independently. + + test('malformed fragment (pr: 0) fails the gate end-to-end', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lint-e2e-')); + t.after(() => cleanup(tmpDir)); + + buildTempRepo(tmpDir, [ + // User-facing source change that requires a fragment + { file: 'bin/thing.js', content: '// placeholder\n' }, + // Malformed fragment: pr: 0 is rejected by parseFragment (pr must be > 0) + { + file: '.changeset/bad.md', + content: '---\ntype: Fixed\npr: 0\n---\n**Bad** placeholder. (#1)\n', + }, + ]); + + const { status, report } = runLint(tmpDir); + + assert.equal(status, 1, `expected exit 1, got ${status}`); + assert.equal( + report.reason, + LINT_REASON.FAIL_INVALID_FRAGMENT, + `expected FAIL_INVALID_FRAGMENT, got ${report.reason}`, + ); + assert.ok(Array.isArray(report.failures), 'failures must be an array'); + const badEntry = report.failures.find((f) => f.file.endsWith('.changeset/bad.md')); + assert.ok(badEntry, 'failures must contain the bad fragment file'); + assert.equal(badEntry.reason, 'invalid_pr', `expected reason invalid_pr, got ${badEntry.reason}`); + }); + + test('valid fragment (pr: 1) passes the gate end-to-end', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lint-e2e-')); + t.after(() => cleanup(tmpDir)); + + buildTempRepo(tmpDir, [ + { file: 'bin/thing.js', content: '// placeholder\n' }, + { + file: '.changeset/good.md', + content: '---\ntype: Fixed\npr: 1\n---\n**Good** fix. (#1)\n', + }, + ]); + + const { status, report } = runLint(tmpDir); + + assert.equal(status, 0, `expected exit 0, got ${status}`); + assert.equal( + report.reason, + LINT_REASON.OK_FRAGMENT_PRESENT, + `expected OK_FRAGMENT_PRESENT, got ${report.reason}`, + ); + }); + + test('deleted fragment is skipped and does not produce fail_invalid_fragment', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lint-e2e-')); + t.after(() => cleanup(tmpDir)); + + // Base commit includes a valid fragment (pr: 5) that the PR will delete. + buildTempRepo( + tmpDir, + [ + // PR deletes the old fragment (null = delete) + { file: '.changeset/old.md', content: null }, + // PR adds a new valid fragment + { + file: '.changeset/new.md', + content: '---\ntype: Fixed\npr: 6\n---\n**New** fix. (#6)\n', + }, + ], + // base files: the old fragment exists before the PR + [{ file: '.changeset/old.md', content: '---\ntype: Fixed\npr: 5\n---\n**Old** fix. (#5)\n' }], + ); + + const { status, report } = runLint(tmpDir); + + assert.equal(status, 0, `expected exit 0, got ${status}`); + assert.equal( + report.reason, + LINT_REASON.OK_FRAGMENT_PRESENT, + `expected OK_FRAGMENT_PRESENT, got ${report.reason}`, + ); + // The deleted fragment must NOT appear in failures + const failures = report.failures ?? []; + const deletedEntry = failures.find((f) => f.file.endsWith('.changeset/old.md')); + assert.ok(!deletedEntry, `deleted fragment must not appear in failures, got: ${JSON.stringify(failures)}`); + }); });