From cafb874c4a6c1d0498f06866bdfb16eb5e9863e9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 14 Jun 2026 15:02:54 -0400 Subject: [PATCH] fix(#1224): accept --pr 0 placeholder at changeset creation (#1231) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1224): accept --pr 0 placeholder at changeset creation The required-field guard `!opts.pr` treated the integer 0 as falsy, rejecting the documented `pr: 0` two-push placeholder with a usage error (exit 2). Non-numeric `--pr abc` (NaN) was also silently accepted before (passes `!NaN === true`... actually `!NaN` is true, so NaN would trigger the guard already). The new explicit checks use `opts.pr === null` for missing flag and `Number.isNaN` for non-numeric input, accepting all finite integer values including 0. The merge-time safety net in parse.cjs (`pr <= 0` → INVALID_PR) is unchanged — a pr:0 fragment is still rejected at lint/render time. Co-Authored-By: Claude Opus 4.8 * chore(#1224): backfill changeset PR number (#1231) Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/nimble-yaks-munch.md | 5 ++ scripts/changeset/new.cjs | 20 ++++++-- tests/changeset-new.test.cjs | 88 ++++++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 .changeset/nimble-yaks-munch.md diff --git a/.changeset/nimble-yaks-munch.md b/.changeset/nimble-yaks-munch.md new file mode 100644 index 000000000..1677bd988 --- /dev/null +++ b/.changeset/nimble-yaks-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1231 +--- +**`changeset new --pr 0` now accepted at creation** — the required-field guard treated the integer 0 as a missing `--pr` flag, so the documented `pr: 0` placeholder could not be authored via the CLI. (#1231) diff --git a/scripts/changeset/new.cjs b/scripts/changeset/new.cjs index ea88a534a..674216e24 100755 --- a/scripts/changeset/new.cjs +++ b/scripts/changeset/new.cjs @@ -106,8 +106,14 @@ function parseArgs(argv) { const r = requireValue(a, i); if (!r.ok) return { ok: false, error: r.error }; if (a === '--type') opts.type = r.value; - else if (a === '--pr') opts.pr = Number(r.value); - else if (a === '--body') opts.body = r.value; + else if (a === '--pr') { + // Accept only decimal-integer strings (digits only, no sign, no dot, + // no hex prefix, no scientific notation). Non-integer input — including + // empty string and whitespace — is normalized to NaN so the prNaN + // guard below rejects it with the usage error. + const trimmed = r.value.trim(); + opts.pr = /^\d+$/.test(trimmed) ? Number(trimmed) : NaN; + } else if (a === '--body') opts.body = r.value; else if (a === '--repo') opts.repo = r.value; i++; continue; @@ -125,7 +131,15 @@ function main() { throw new ExitError(2); } const { opts } = parsed; - if (!opts.type || !opts.pr || !opts.body) { + // opts.pr starts as null (missing flag) and is set by parseArgs to a Number when + // the raw value is a pure decimal-integer string (digits only), or to NaN for any + // other input (empty, whitespace, floats, hex, negatives, scientific notation, etc.). + // Accept integer 0 (the documented pr:0 placeholder); reject a missing flag (null) + // and any non-decimal-integer value (NaN). The merge/lint gate separately + // enforces pr > 0 before a fragment can land, so 0 still cannot be merged. + const prMissing = opts.pr === null; + const prNaN = typeof opts.pr === 'number' && Number.isNaN(opts.pr); + if (!opts.type || prMissing || prNaN || !opts.body) { throw new ExitError(2, 'usage: changeset/new.cjs --type --pr NNNN --body "..."'); } const file = scaffoldFragment(opts); diff --git a/tests/changeset-new.test.cjs b/tests/changeset-new.test.cjs index ad3a5313c..2d72d3f13 100644 --- a/tests/changeset-new.test.cjs +++ b/tests/changeset-new.test.cjs @@ -6,15 +6,19 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); +const { spawnSync } = require('node:child_process'); const ROOT = path.join(__dirname, '..'); -const { generateFragmentName, scaffoldFragment, parseFragment } = (() => { +const { generateFragmentName, scaffoldFragment, parseFragment, parseArgs } = (() => { const newCs = require(path.join(ROOT, 'scripts', 'changeset', 'new.cjs')); const parse = require(path.join(ROOT, 'scripts', 'changeset', 'parse.cjs')); return { ...newCs, parseFragment: parse.parseFragment }; })(); +const { FRAGMENT_ERROR } = require(path.join(ROOT, 'scripts', 'changeset', 'parse.cjs')); const { cleanup } = require('./helpers.cjs'); +const NEW_CJS = path.join(ROOT, 'scripts', 'changeset', 'new.cjs'); + let tmp; before(() => { tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-new-changeset-')); }); after(() => { cleanup(tmp); }); @@ -95,3 +99,85 @@ describe('changeset new: name generator + scaffold writer (#2975)', () => { assert.deepEqual(r.opts, { type: 'Fixed', pr: 42, body: 'a body', repo: '/tmp/x' }); }); }); + +describe('changeset new: --pr 0 placeholder acceptance (bug #1224)', () => { + // (a) The headline bug: `main()` (not just parseArgs) must ACCEPT --pr 0. + // We exercise the end-to-end CLI path via spawnSync so that main()'s guard + // is on the critical path. Old code had `if (!opts.pr)` which treated 0 as + // falsy and exited with code 2. Fixed code uses explicit `=== null` / isNaN + // guards and must exit 0 and write a fragment file with pr: 0 in frontmatter. + test('(a) CLI main() accepts --pr 0 (exit 0) and writes a pr:0 fragment rejected at lint time', () => { + // Use an isolated temp dir per invocation to avoid .changeset/ pollution + // from other tests sharing `tmp`. + const isolatedDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-new-cs-a-')); + try { + const result = spawnSync( + process.execPath, + [NEW_CJS, '--type', 'Fixed', '--pr', '0', '--body', 'placeholder for pr-zero.', '--repo', isolatedDir], + { encoding: 'utf8', timeout: 10000 }, + ); + + // The bug: old main() did `if (!opts.pr)` → exit 2. Fixed: exit 0. + assert.equal(result.status, 0, + `CLI main() must exit 0 for --pr 0; got status=${result.status} stderr=${result.stderr}`); + + // A fragment file must exist under .changeset/ + const csDir = path.join(isolatedDir, '.changeset'); + const files = fs.readdirSync(csDir).filter(f => f.endsWith('.md')); + assert.equal(files.length, 1, + `Expected exactly one fragment file in ${csDir}, got: ${JSON.stringify(files)}`); + + // The written fragment must round-trip through parseFragment as INVALID_PR, + // confirming pr:0 was embedded (not absent, NaN, or a positive integer). + const src = fs.readFileSync(path.join(csDir, files[0]), 'utf8'); + const parsed = parseFragment(src); + assert.equal(parsed.ok, false, 'parseFragment must reject a pr:0 fragment at lint time'); + assert.equal(parsed.reason, FRAGMENT_ERROR.INVALID_PR, + `expected INVALID_PR, got: ${parsed.reason}`); + } finally { + cleanup(isolatedDir); + } + }); + + // (b) missing --pr: parseArgs leaves opts.pr as null, causing main() to reject. + test('(b) missing --pr leaves opts.pr as null (rejected by main validation)', () => { + const r = parseArgs(['--type', 'Fixed', '--body', 'body without pr flag.', '--repo', tmp]); + assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()'); + assert.equal(r.opts.pr, null, `opts.pr should be null when --pr is omitted, got: ${r.opts.pr}`); + // prMissing = opts.pr === null → would trigger rejection in main() + assert.equal(r.opts.pr === null, true, 'prMissing condition must be true'); + }); + + // (c) --pr abc: non-numeric input normalised to NaN by parseArgs. + test('(c) --pr abc (non-numeric) produces NaN in opts.pr (rejected by main validation)', () => { + const r = parseArgs(['--type', 'Fixed', '--pr', 'abc', '--body', 'body.', '--repo', tmp]); + assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()'); + assert.equal(Number.isNaN(r.opts.pr), true, + `opts.pr should be NaN for non-numeric input, got: ${r.opts.pr}`); + }); + + // (d1) --pr "" (empty string): trimmed to "", /^\d+$/ fails → NaN. + test('(d1) --pr "" (empty string) produces NaN in opts.pr (rejected by main validation)', () => { + const r = parseArgs(['--type', 'Fixed', '--pr', '', '--body', 'body.', '--repo', tmp]); + assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()'); + assert.equal(Number.isNaN(r.opts.pr), true, + `opts.pr should be NaN for empty string, got: ${r.opts.pr}`); + }); + + // (d2) --pr " " (whitespace-only): trimmed to "", /^\d+$/ fails → NaN. + test('(d2) --pr " " (whitespace-only) produces NaN in opts.pr (rejected by main validation)', () => { + const r = parseArgs(['--type', 'Fixed', '--pr', ' ', '--body', 'body.', '--repo', tmp]); + assert.equal(r.ok, true, 'parseArgs itself succeeds; rejection is in main()'); + assert.equal(Number.isNaN(r.opts.pr), true, + `opts.pr should be NaN for whitespace-only input, got: ${r.opts.pr}`); + }); + + // (e) parse.cjs rejects a pr:0 fragment — the merge-time lint safety net is intact. + test('(e) parseFragment rejects a pr:0 fragment with INVALID_PR (lint safety net intact)', () => { + const fragmentContent = `---\ntype: Fixed\npr: 0\n---\nsome body text.\n`; + const result = parseFragment(fragmentContent); + assert.equal(result.ok, false, 'parseFragment must reject pr:0 at lint time'); + assert.equal(result.reason, FRAGMENT_ERROR.INVALID_PR, + `expected INVALID_PR reason, got: ${result.reason}`); + }); +});