* fix(#2944): remove the catastrophic-backtracking regex from the example The non-shipping Option-E reference example carried its own copy of the predicate-id regex, which nested a dot-containing character class inside a dot-prefixed repeat. A run of N consecutive dots therefore had exponentially many partitions. Measured on next before this change: 30 dots 54ms, 35 66ms, 40 807ms — so roughly 55-60 dots hangs for hours. Not exploitable where it sits: the example is outside tsconfig.build.json, outside the npm package files list, outside the installer and outside tests, so no build step or CI job parses anything with it. Fixed because the entire point of a reference example is that people copy it forward, and ADR-1671 presents this one as the pattern for the platform. Ports the linear per-segment validation that #2928 gave the production module, so the two copies agree: both parse the real CONTEXT.md to 415 predicates across 20 classes with 0 duplicates. Doubled-dot ids are now rejected here too, matching production, and the grammar comment records it. Also refreshes the example's committed index, which #2928 made stale when it removed the duplicate predicate from CONTEXT.md. Closes #2944 * test(#2944): guard predicate-index sync and example/production parity Two regression tests for the two defects in this PR. Index sync: asserts the committed docs/CONTEXT-INDEX.json equals a fresh parse of CONTEXT.md, naming any diverging predicate ids. The merge race that reddened next was invisible to both PRs involved and only surfaced on the next PR to run lint:ci; this puts the same check inside the suite, which runs on every PR, and a mutation test proves the assertion is not vacuous. Example/production parity: asserts both copies of the parser report the same count, classes and duplicates for the real CONTEXT.md, and agree verdict-for- verdict over a table of id shapes. The divergence WAS the bug — production went linear-time while the example kept the backtracking regex, with nothing asserting they agreed. Also pins the example rejecting a 60-dot id, with the clean rejection as the binding assertion and wall-clock only as a smoke check. Notes a real tension rather than hiding it: ADR-1671 says the example sits outside tests/, and this imports it. The ADR's intent is that the example is not compiled, packaged or installed — not that it may silently rot. A parity guard does not ship it. The file states this so a reviewer can object. * fix(#2944): address both isolated review passes Two independent reviewers (correctness and security axes, neither the author). Security found nothing — it measured linearity to 100k chars across dots, hyphens, underscores and mixed classes, and showed prototype pollution is structurally unreachable because the first-segment pattern forbids lowercase and underscore-leading ids. The correctness pass found three blockers, all real. Blocker: the parity test violated ADR-1671 verbatim. The ADR lists FOUR exclusions for the reference example, the fourth being the CI test suite, and the test imported it from tests/ while its own justification comment cited only three -- constructing a rationale around the exclusion it broke. Moved to scripts/lint-example-parser-parity.cjs wired into lint:ci; a lint script is not the test suite, so the exclusion stands. The test file keeps only the docs/CONTEXT-INDEX.json sync check. Blocker: the mutation test leaked its temp dir. Its callback took no `t`, so a failing assertion skipped the bare cleanup call. Now registered via t.after(), matching the convention adr-index-gate.test.cjs documents. Blocker: the example's own committed index carries the identical merge-race staleness this PR fixes for the production one, and nothing guarded it. Deliberately NOT fixed by wiring the example's --check into CI: that artifact bakes line numbers, so it re-drifts on any unrelated CONTEXT.md line shift -- exactly ADR-1671 open question 4 -- and would make CI routinely red. The new lint asserts the line-INDEPENDENT facts instead: count, class map, duplicate set, and every (id, value) pair. Proven non-vacuous both ways: mutating a value fails and names the id, mutating only a line number passes. Major: a real divergence the parity claim would have missed. Production rejects values containing an embedded CR, LF, U+2028 or U+2029; the example did not, so a value with an embedded lone CR was rejected by one copy and accepted by the other. Ported, and now covered by the parity table. Also, found while verifying rather than reported: malformed diagnostics covered only empty values. A doubled-dot id, a space in an id, and a lowercase-leading id were all dropped silently. That contradicts the module's own intent -- a typo should be diagnosable, and a space in an id is a likely one -- and predicates are contractually cited, so a silently vanished predicate is the failure mode that matters. Each rejection class now carries a named reason in both copies, while ordinary inline code still yields none. Trues up counts my own change staled: the example README and ADR-1671's prototype figures said 416 and 393/18 against a real 415/20/0. Closes #2944 * chore(#2944): backfill changeset PR number 2950 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
@@ -18,10 +18,17 @@
|
||||
* `line` and `section`.
|
||||
*
|
||||
* One addition beyond the prototype: `ParseResult.malformed` collects
|
||||
* backtick lines that look like a predicate declaration but are rejected for
|
||||
* having an empty value (e.g. `` `ID=` ``), so the empty-value case is
|
||||
* surfaced as a diagnostic instead of being silently dropped. This does not
|
||||
* change any accept/reject outcome — only adds a diagnostic.
|
||||
* backtick lines that look like a predicate declaration attempt (contains a
|
||||
* backtick-wrapped `id=value`-shaped inner with an `=` at index >= 1) but are
|
||||
* rejected, with a distinct named `reason` per rejection class: `empty-value`
|
||||
* (e.g. `` `ID=` ``), `empty-segment` (a doubled dot in the id, e.g.
|
||||
* `` `A..b=1` ``), `invalid-id-chars` (disallowed characters in the id, e.g. a
|
||||
* space: `` `FOO BAR=1` ``), `lowercase-leading-class` (id's first segment
|
||||
* starts lowercase, e.g. `` `foo.bar=1` ``), and `value-contains-newline` (an
|
||||
* embedded CR/LF/U+2028/U+2029 in the value). A line with no `=` at all (e.g.
|
||||
* `` `ID` ``, ordinary inline code) is NOT a declaration attempt and never
|
||||
* produces a diagnostic. This does not change any accept/reject outcome —
|
||||
* only adds diagnostics.
|
||||
*
|
||||
* Grammar (from discovery facts):
|
||||
* Two line forms, each on exactly one source line:
|
||||
@@ -184,6 +191,39 @@ export interface ContextIndex {
|
||||
const ID_FIRST_SEGMENT_RE = /^[A-Z][A-Z0-9_-]*$/;
|
||||
const ID_SUBSEQUENT_SEGMENT_RE = /^[A-Za-z0-9_-]+$/;
|
||||
|
||||
/** Result of {@link validateIdDetailed}: valid, or invalid with a named reason. */
|
||||
interface IdValidation {
|
||||
valid: boolean;
|
||||
reason?: 'empty-segment' | 'invalid-id-chars' | 'lowercase-leading-class';
|
||||
}
|
||||
|
||||
/**
|
||||
* Structurally validate a candidate predicate id (linear time — no ambiguous
|
||||
* backtracking quantifier; see the ID grammar comment above), returning WHY it
|
||||
* is invalid so malformed diagnostics can name the exact rejection class.
|
||||
*
|
||||
* @param id - candidate id (everything before the first '=')
|
||||
*/
|
||||
function validateIdDetailed(id: string): IdValidation {
|
||||
const segments = id.split('.');
|
||||
|
||||
// A doubled dot (or leading/trailing dot) produces an empty segment.
|
||||
if (segments.some((seg) => seg === '')) return { valid: false, reason: 'empty-segment' };
|
||||
|
||||
const first = segments[0];
|
||||
if (!ID_FIRST_SEGMENT_RE.test(first)) {
|
||||
// Distinguish "starts lowercase" (a highly plausible typo, e.g.
|
||||
// `foo.bar=1`) from any other first-segment character-set violation
|
||||
// (e.g. a space, `FOO BAR=1`).
|
||||
if (/^[a-z]/.test(first)) return { valid: false, reason: 'lowercase-leading-class' };
|
||||
return { valid: false, reason: 'invalid-id-chars' };
|
||||
}
|
||||
for (let i = 1; i < segments.length; i++) {
|
||||
if (!ID_SUBSEQUENT_SEGMENT_RE.test(segments[i])) return { valid: false, reason: 'invalid-id-chars' };
|
||||
}
|
||||
return { valid: true };
|
||||
}
|
||||
|
||||
/**
|
||||
* Structurally validate a candidate predicate id (linear time — no ambiguous
|
||||
* backtracking quantifier; see the ID grammar comment above).
|
||||
@@ -191,12 +231,7 @@ const ID_SUBSEQUENT_SEGMENT_RE = /^[A-Za-z0-9_-]+$/;
|
||||
* @param id - candidate id (everything before the first '=')
|
||||
*/
|
||||
function isValidId(id: string): boolean {
|
||||
const segments = id.split('.');
|
||||
if (!ID_FIRST_SEGMENT_RE.test(segments[0])) return false;
|
||||
for (let i = 1; i < segments.length; i++) {
|
||||
if (!ID_SUBSEQUENT_SEGMENT_RE.test(segments[i])) return false;
|
||||
}
|
||||
return true;
|
||||
return validateIdDetailed(id).valid;
|
||||
}
|
||||
|
||||
// List markers recognized ahead of a backtick-wrapped declaration:
|
||||
@@ -273,11 +308,18 @@ function extractPredicate(raw: string): { id: string; value: string } | null {
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect the "looks like a declaration but has an empty value" malformed
|
||||
* case for a line that {@link extractPredicate} already rejected. Only
|
||||
* fires when the ID portion is grammatically valid on its own and the value
|
||||
* after the first '=' is empty (e.g. `` `ID=` ``). Does not change any
|
||||
* accept/reject decision — diagnostic only.
|
||||
* Detect the "looks like a predicate declaration attempt but is rejected"
|
||||
* malformed case for a line that {@link extractPredicate} already rejected —
|
||||
* naming WHY, so a maintainer's typo is diagnosable instead of silently
|
||||
* vanishing. Only fires when the line is backtick-wrapped (in either
|
||||
* recognized form) AND contains an `=` at index >= 1 — a plain inline-code
|
||||
* line with no `=` at all (e.g. `` `ID` ``) is not a declaration attempt and
|
||||
* never produces a diagnostic. Does not change any accept/reject decision —
|
||||
* diagnostic only.
|
||||
*
|
||||
* Reason precedence when a line fails more than one check at once: id
|
||||
* validity is checked first (an invalid id makes the value irrelevant), then
|
||||
* empty-value, then embedded-newline.
|
||||
*
|
||||
* @param raw - the original source line (with newline stripped)
|
||||
*/
|
||||
@@ -291,9 +333,16 @@ function detectMalformed(raw: string): { text: string; reason: string } | null {
|
||||
const id = inner.slice(0, eqIdx);
|
||||
const value = inner.slice(eqIdx + 1);
|
||||
|
||||
if (value === '' && isValidId(id)) {
|
||||
const idCheck = validateIdDetailed(id);
|
||||
if (!idCheck.valid) {
|
||||
return { text: raw.trimEnd(), reason: idCheck.reason as string };
|
||||
}
|
||||
if (value === '') {
|
||||
return { text: raw.trimEnd(), reason: 'empty-value' };
|
||||
}
|
||||
if (/[\n\r\u2028\u2029]/.test(value)) {
|
||||
return { text: raw.trimEnd(), reason: 'value-contains-newline' };
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user