diff --git a/.changeset/witty-lynx-sing.md b/.changeset/witty-lynx-sing.md new file mode 100644 index 000000000..a366bbdf1 --- /dev/null +++ b/.changeset/witty-lynx-sing.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3851 +--- +**A feature fragment declaring a malformed `order:` no longer sorts silently to the top of `docs/FEATURES.md`** — the generator validated that field by coercion, so an empty value read as `0` and hex, octal, binary and exponential values read as numbers, all placing the section ahead of every real feature with no violation and a clean `--check`. (#3840) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 61144b2dc..9c6343d45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -361,7 +361,13 @@ shared file, so there is nothing to collide on. `docs/features/_groups/.md`, which a feature PR never touches. 4. **`order` is optional.** It defaults to the numeric part of `id`, which is right for almost everything. Declare it only to place a section somewhere its - number would not put it (`27b` precedes `27a` for historical reasons). + number would not put it (`27b` precedes `27a` for historical reasons). When + declared it must be an optionally-signed integer or decimal — `27`, `0`, + `-1`, `+3`, `27.2`. Anything else is an `order_invalid` violation, including + an empty value, a hex/binary/octal literal and exponential notation: all of + those coerce to a finite number under JavaScript's `Number()`, so before + #3840 a bare `order:` sorted the section to position 0 — ahead of every real + feature — with no violation and a clean `--check`. 5. **Bodies start at `####`.** A `##` or `###` inside a fragment body would forge a group or a sibling section with no id and no TOC entry; `--check` rejects it (`body_heading_too_shallow`). diff --git a/scripts/gen-features.cjs b/scripts/gen-features.cjs index a83008804..f2ad185d6 100644 --- a/scripts/gen-features.cjs +++ b/scripts/gen-features.cjs @@ -165,6 +165,20 @@ const GROUP_NOTE_FIELDS = Object.freeze(['group']); */ const ID_RE = /^(?:0|[1-9][0-9]*)(?:\.[0-9]+|[a-z])?$/; +/** + * A legal explicit `order`: an optionally-signed decimal literal. + * + * `order` was the only field validated by COERCION rather than by shape, and + * `Number()` is far more liberal than a docs ordering field has reason to be: + * `Number('')` and `Number(' ')` are 0, and `0x10`, `0b11`, `0o17`, `1e3`, `1.` + * and `.5` all coerce to finite numbers. A fragment declaring `order:` with + * nothing after it therefore sorted to position 0 — ahead of every real + * feature — with no violation and exit 0, in a gate whose whole contract is a + * typed violation rather than a silent guess. Shape first, then coerce, + * mirroring how ID_RE guards `id`. + */ +const ORDER_RE = /^[+-]?(?:0|[1-9][0-9]*)(?:\.[0-9]+)?$/; + /** The fragment filename shape: a kebab slug, mirroring `docs/adr/`'s rule. */ const FRAGMENT_FILENAME_RE = /^[a-z0-9]+(?:-[a-z0-9]+)*\.md$/; @@ -407,7 +421,10 @@ function readCorpus() { let order = defaultOrder(data.id); if (data.order !== undefined) { order = Number(data.order); - if (!Number.isFinite(order)) { + // Both guards are load-bearing: the regex rejects the shapes `Number` + // would silently accept, and `isFinite` still catches a well-shaped + // literal long enough to overflow to Infinity. + if (!ORDER_RE.test(data.order) || !Number.isFinite(order)) { add(REASON.ORDER_INVALID, rel, { order: data.order }); continue; } @@ -620,7 +637,7 @@ function describeViolation(v) { case REASON.ID_DUPLICATE: return `${v.file}: id '${v.id}' is already used by ${v.first} — pick another (any unique id is legal)`; case REASON.ORDER_INVALID: - return `${v.file}: order '${v.order}' is not a finite number`; + return `${v.file}: order '${v.order}' is not a decimal number (an optionally-signed integer or decimal)`; case REASON.ANCHOR_DUPLICATE: return `${v.file}: anchor '#${v.anchor}' collides with ${v.first}`; case REASON.BODY_EMPTY: diff --git a/tests/features-index-gate.test.cjs b/tests/features-index-gate.test.cjs index 2b300cf55..461513f12 100644 --- a/tests/features-index-gate.test.cjs +++ b/tests/features-index-gate.test.cjs @@ -571,6 +571,180 @@ describe('gen-features CLI', () => { assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID]); }); + // --------------------------------------------------------------------------- + // `order` validation (#3840 follow-up): `Number()` coercion is far more + // liberal than a docs ordering field has reason to be. `Number('')` is 0, + // and `0x10`, `0b11`, `0o17`, `1e3`, `1.` and `.5` all coerce to finite + // numbers — so a fragment declaring `order:` with nothing after it sorted + // to position 0, ahead of every real feature, with zero violations. + // --------------------------------------------------------------------------- + + test('an empty order is rejected, not coerced to zero', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order: '' }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID]); + }); + + test('a bare `order:` line is rejected too — the shape an author actually types', (t) => { + // The reject rows above build `order` through renderFrontmatter, which + // emits the QUOTED-empty form `order: ""`. A human types the bare form. + // Both converge to '' at the validator; this pins that they do, so a + // change to parseScalar's quoted branch cannot quietly split them. + for (const line of ['order:', 'order: ']) { + const root = makeRepo(t, { + 'a.md': `---\nid: 1\ntitle: A\ngroup: G\n${line}\n---\n\n**Purpose:** x.\n`, + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID], line); + } + }); + + test('a non-decimal radix order is rejected', (t) => { + for (const order of ['0x10', '0b11', '0o17']) { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID], `order: ${order}`); + } + }); + + test('an exponential order is rejected', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order: '1e3' }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID]); + }); + + test('a malformed decimal order is rejected', (t) => { + for (const order of ['1.', '.5']) { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID], `order: ${order}`); + } + }); + + test('Infinity, NaN and separators stay rejected', (t) => { + // Regression pin: these already fail today under plain Number() coercion + // and must keep failing once order is validated by shape as well. + for (const order of ['Infinity', 'NaN', '1_0']) { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID], `order: ${order}`); + } + }); + + test('an order that overflows to Infinity is rejected', (t) => { + // The regex alone would admit this shape; the finite guard must still fire. + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order: '1'.repeat(400) }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID]); + }); + + test('accepts a plain integer order', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('5', 'A', 'G', '**Purpose:** x.', { order: '27' }), + 'b.md': fragment('1', 'B', 'G'), + }); + assert.deepEqual(report(root).violations, []); + run(root, ['--write']); + const doc = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + // order 27 > B's default order (1), so A must render after B. + assert.equal(doc.indexOf('### 5.') > doc.indexOf('### 1.'), true); + }); + + test('accepts an explicit zero order', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('5', 'A', 'G', '**Purpose:** x.', { order: '0' }), + 'b.md': fragment('9', 'B', 'G'), + }); + assert.deepEqual(report(root).violations, []); + run(root, ['--write']); + const doc = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + // order 0 < B's default order (9), so A must render before B. + assert.equal(doc.indexOf('### 5.') < doc.indexOf('### 9.'), true); + }); + + test('accepts a negative order', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('5', 'A', 'G', '**Purpose:** x.', { order: '-1' }), + 'b.md': fragment('1', 'B', 'G'), + }); + assert.deepEqual(report(root).violations, []); + run(root, ['--write']); + const doc = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + // order -1 < B's default order (1), so A must render before B. + assert.equal(doc.indexOf('### 5.') < doc.indexOf('### 1.'), true); + }); + + test('accepts a leading-plus order', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('5', 'A', 'G', '**Purpose:** x.', { order: '+3' }), + 'b.md': fragment('1', 'B', 'G'), + 'c.md': fragment('10', 'C', 'G'), + }); + assert.deepEqual(report(root).violations, []); + run(root, ['--write']); + const doc = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + // order +3 sits between B's default order (1) and C's default order (10). + assert.equal(doc.indexOf('### 1.') < doc.indexOf('### 5.'), true); + assert.equal(doc.indexOf('### 5.') < doc.indexOf('### 10.'), true); + }); + + test('a quoted numeric order is accepted after unquoting', (t) => { + // Built by writing the raw fragment text so the frontmatter line literally + // reads `order: "27.2"`, rather than going through the `fragment()` helper + // (which would double-quote a JS string value). + const root = makeRepo(t, { + 'a.md': '---\nid: 5\ntitle: A\ngroup: G\norder: "27.2"\n---\n\n**Purpose:** x.\n', + 'b.md': fragment('1', 'B', 'G'), + }); + assert.deepEqual(report(root).violations, []); + run(root, ['--write']); + const doc = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + // order 27.2 > B's default order (1), so A must render after B. + assert.equal(doc.indexOf('### 5.') > doc.indexOf('### 1.'), true); + }); + + test('an invalid id short-circuits before order validation', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('12ab', 'A', 'G', '**Purpose:** x.', { order: '' }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ID_INVALID]); + }); + + test('an invalid order short-circuits before the body check', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '', { order: '' }), + }); + assert.deepEqual(reasonsIn(report(root)), [REASON.ORDER_INVALID]); + }); + + test('each malformed order is reported per file', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G', '**Purpose:** x.', { order: '' }), + 'b.md': fragment('2', 'B', 'G', '**Purpose:** x.', { order: '' }), + }); + const rep = report(root); + assert.equal(rep.violations.length, 2); + const files = rep.violations.map((v) => v.file).sort(); + assert.deepEqual(files, ['docs/features/a.md', 'docs/features/b.md']); + }); + + test('a malformed order refuses the write rather than reordering the document', (t) => { + const root = makeRepo(t, { + 'a.md': fragment('1', 'A', 'G'), + 'b.md': fragment('2', 'B', 'G'), + 'c.md': fragment('900', 'C', 'G', '**Purpose:** x.', { order: '' }), + }); + const before = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + assert.equal(run(root, ['--write']).exitCode, 1); + const after = fs.readFileSync(path.join(root, 'docs', 'FEATURES.md'), 'utf8'); + assert.equal(after, before); + }); + test('an explicit order overrides the id-derived default', (t) => { const root = makeRepo(t, { 'a.md': fragment('27', 'A', 'G'),