fix(#1006): rc CHANGELOG preview crash on malformed changeset fragment + validate fragment content at the gate (#1007)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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)}`);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user