* fix(#3840): reject a malformed feature `order` instead of coercing it `scripts/gen-features.cjs` was the one field validated by coercion rather than by shape. `Number('')` is 0, and `0x10`, `0b11`, `0o17`, `1e3`, `1.` and `.5` all coerce to finite numbers, so a fragment declaring a bare `order:` sorted to position 0 -- ahead of every real feature, in both the body and the generated table of contents -- with zero violations, a clean `--check` and `--write` exiting 0. That is a fail-open in a gate whose entire contract is a typed violation rather than a silent guess. `order` is now shape-checked against an optionally-signed decimal literal before coercion, mirroring how ID_RE guards `id`. The finite check stays: the regex alone would admit a literal long enough to overflow to Infinity. Surfaced by re-running the feature-implementation directive's design and QA steps against the code merged in #3845, which shipped without them. Refs #3840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3840): backfill changeset PR number Refs #3840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/witty-lynx-sing.md
Normal file
5
.changeset/witty-lynx-sing.md
Normal file
@@ -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)
|
||||
@@ -361,7 +361,13 @@ shared file, so there is nothing to collide on.
|
||||
`docs/features/_groups/<slug>.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`).
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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'),
|
||||
|
||||
Reference in New Issue
Block a user