* refactor(#3180): one owner for completion ratio, a prompt-layer drift guard, and a written behavior contract The 2026-08-08 coverage audit on #3180 found the epic's copy counts were a lower bound for the third consecutive time, and that two derivation families had never been named at all. ADR-3180 gains Decision 7 — a normative behavior contract that says what the right answer IS for each derivation, not merely who owns it. A reviewer with no written rule can only ask "does this look like the others", which is how a fifth copy passes review. Decision 4 gains (d) scan surface is every authored surface and an owner FILE is never exempt, only its named functions; and (e) a surface that cannot be consolidated today ships ratcheted, never unguarded. Completion ratio: `clampPercent` sat exported and unused beside six hand-inlined copies of its own body across five modules. All six now route through it; `clampPercentFromFraction` is added for the one caller that already held a fraction. Every migration is behaviour-identical — clampPercent's first line IS the `total > 0 ? … : 0` ternary each copy carried. Guarded by lint-completion-ratio-drift.cjs, which reports zero re-derivations with no file-level exemption. Prompt layer: workflow markdown re-derives live-plan counting in raw shell (#1762), invisible to every `src/`-scoped guard. lint-planning-prompt-drift.cjs scans it with a shrink-only baseline of the 7 sites that exist today — new sites fail, and a baseline entry that stops firing fails too, so an acknowledgment can never outlive the thing it describes. lint-milestone-window-drift.cjs stops exempting its owner file wholesale; only the four named canonical functions are exempt now. The blanket exemption was pointed at the one file most likely to grow the next copy, and it had. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3180): link Phases 6-8 sub-issues (#3216, #3217, #3218) from ADR-3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): address orthogonal review — consumer-output identity tests, count-keyed ratchet, property coverage Five findings from the two orthogonal review passes, all fixed. Decision 4(c) breach: the completion-ratio identity test asserted at the OWNER, which is exactly the bypass that decision exists to close — a consumer can call clampPercent and then post-process locally, leaving both the lint and an owner-level test green. It now drives `roadmap analyze`, `query progress` and `stats` and asserts on their own output, over a fixture containing a `status: superseded` plan so a consumer that re-counted raw files would report 60 where the owner reports 75. Decision 4(e) breach: ratchet entries named the epic (#3180) rather than the issue that removes them. They name Phase 8 (#3218) now. The ratchet keyed on (file, text) alone, so plan-phase.md's two byte-identical sites were one indistinguishable key and migrating either would have left the guard green with the other alive. Entries carry an occurrence count; fewer than acknowledged fails as a partial migration, more fails as a new copy. Adds the missing MAX_REGEX_LITERAL_LEN boundary coverage the sibling guard's test already had, and the fast-check property tests CONTRIBUTING requires for clamp/budget-limit functions. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: stop wrapping a nested double-spawn in a 15s wall-clock budget (bug #641 probes) `tests/ci-test-scope.test.cjs`'s `bug #641` block spawned `run-tests.cjs` under PROBE_TIMEOUT_MS=15000; that child then spawned a nested `node --test`. A fixed wall-clock budget around a double spawn, running inside a container that is concurrently executing the full ~31k-test suite, fails by construction under load. Confirmed against three full matrix runs. Every failure was shaped `null !== 0` — the child was KILLED, never an assertion about the thing under test. One captured probe had already printed the correct resolution (`suite="all" files=2: a.test.cjs b.test.cjs`) and was killed anyway. It reproduces on `next` alone: 5 failures on linux-node22, 0 on linux-node24. The victim subset varies by run and by lane. What these tests are actually about is suite-token RESOLUTION — `unit` as a bare token in --files/--files-from. Executing the seeded trivial files is incidental and is the entire timeout surface, so the assertions move in-process against the same functions `main()` calls, in the same order. `parseArgs`, `selectExplicitFiles`, `selectFiles` and `walkTestFiles` are exported for that; no behavior, signature or logic changed. No coverage lost: `tests/run-tests-harness.test.cjs` already spawns the harness for real and asserts exit codes end to end, on a 120s budget. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: delete the three elapsed-time assertions CLAUDE.md forbids asserting on wall-clock time. Three assertions did, and all three are load-sensitive: on a saturated bench each can fail while the code under test is correct. In every case the load-bearing assertion sits on the line above and the timing line adds no discrimination. run-with-timeout: the stated worry — "was this 124 the cap firing or the 30s harness backstop?" — is already answered by the assertion above it. A backstop kills by signal, which surfaces as status null, never 124. Observed directly this session: three matrix runs produced exactly that null shape from killed children. normalize-test-command and context-predicates: both bounded a ReDoS check. A threshold only ever separates "fast" from "slightly slow", which is bench load, not correctness — catastrophic backtracking on 800 KB of input does not take 251ms, it does not finish at all. A real regression therefore shows up as the suite being killed on that test, which is louder and more reliable than a number. The structural assertions (returned unchanged; cleanly rejected) are what actually carry those tests, and they stay. The sweep now reports zero elapsed-time assertions in tests/. The remaining Date.now() uses are unique-path suffixes, barrier deadlines, fixture timestamps and fake mtimes — none of them assertions. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3180): backfill changeset PR number (#3223) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): key the prompt-drift ratchet on POSIX paths so it works on Windows The baseline keys on (file, trimmed text). `file` came from scanTree's `path.relative()`, which uses NATIVE separators, while the committed baseline stores POSIX. On Windows every violation was therefore unmatched — reported as FRESH — and every baseline entry matched nothing — reported as STALE. The guard failed 100% of the time there, on both CI shards: ✖ scanRepo(repoRoot) matches the baseline exactly: zero fresh AND zero stale + { file: 'gsd-core\\workflows\\execute-plan.md', ... } The remote runner this repo gates on is Linux-only and cannot see this class at all; the GitHub Actions Windows lane is what caught it. Normalization is unconditional — never gated on process.platform. A platform-conditional normalizer makes the POSIX path the special case and leaves the Windows branch unexercised on every other OS, which is the same blind spot in a different place. It is applied at one seam inside findPromptDrift, which builds `file` on every returned violation, so the baseline key, the --update writer, the stderr report and the tests all consume one normalized value. The regression tests drive a Windows-shaped relPath directly and run on every OS rather than skipping off-Windows — a test that only runs on the platform where the bug lives is why this escaped. They include a sanity check that un-normalized input does NOT match, so the assertion cannot pass vacuously. Audited the three sibling guards: none keys against a committed cross-platform baseline, and their exemption keys are path.join-built, so producer and consumer share the native convention. Left correct code alone rather than making them look alike. scripts/lib/drift-scan.cjs is untouched — normalizing there would break those three on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
215 lines
11 KiB
JavaScript
215 lines
11 KiB
JavaScript
#!/usr/bin/env node
|
|
'use strict';
|
|
|
|
/**
|
|
* Anti-divergence drift guard for the completion-RATIO seam
|
|
* (epic #3180, ADR-3180 "Planning Semantic Model Single Owner").
|
|
*
|
|
* `src/phase-lifecycle.cts`'s `clampPercent(completed, total)` /
|
|
* `clampPercentFromFraction(fraction)` are the SINGLE canonical owner of
|
|
* "turn a completed/total pair into an integer completion percentage,
|
|
* clamped to 100". Until just before this guard was added, the identical
|
|
* expression `total > 0 ? Math.min(100, Math.round((completed / total) * 100)) : 0`
|
|
* was hand-inlined at six call sites across five modules while the owner sat
|
|
* exported and unused by them — the exact ADR-3180 divergence class, in a
|
|
* derivation the epic had not previously named. Those six sites have been
|
|
* migrated onto the owner; this guard is what stops a seventh copy.
|
|
*
|
|
* Per ADR-3180 Decision 4(a) this guard discovers call sites by SCANNING THE
|
|
* WHOLE `src/` TREE, not by consulting an allowlist of known files — an
|
|
* allowlist only measures re-derivations in files someone remembered to
|
|
* list, and a new call site added anywhere else would sail through silently.
|
|
*
|
|
* DETECTION. A line is a re-derivation when ALL THREE hold, on that ONE
|
|
* source line:
|
|
* (a) it calls one of the `Math.round(`/`Math.floor(`/`Math.trunc(`/
|
|
* `Math.ceil(` rounding family — MATH_ROUND_FAMILY_RE;
|
|
* (b) it SCALES by 100 — a `*` followed by optional whitespace then `100`
|
|
* at a word boundary — SCALE_100_RE;
|
|
* (c) it contains a DIVISION — an identifier/closing-bracket, optional
|
|
* whitespace, `/`, optional whitespace, an identifier/opening-paren —
|
|
* DIVISION_RE — AND that division's index in the line is EARLIER than
|
|
* the index of the `* 100` scale from (b).
|
|
*
|
|
* Clause (c)'s ORDERING requirement is the whole precision of this guard.
|
|
* `(a / b) * 100` — divide FIRST, scale SECOND — is a percentage: the
|
|
* completed/total-derived shape this guard exists to catch. `Math.round(n *
|
|
* 100) / 100` — scale FIRST, divide SECOND — is a completely unrelated
|
|
* idiom (2-decimal-place rounding of an already-fractional value) that
|
|
* happens to share both a rounding call and a `* 100` token; it appears in
|
|
* this repo at `src/eval.cts` and `src/commands.cts` and MUST stay
|
|
* unflagged. Comparing leftmost-match indices (rather than merely testing
|
|
* "does a division exist anywhere on the line") is what tells the two
|
|
* idioms apart: this guard finds the EARLIEST division and the EARLIEST
|
|
* `* 100` scale on the line and requires divIdx < scaleIdx, so a line with a
|
|
* scale-then-divide shape (divIdx > scaleIdx, or no division at all) never
|
|
* matches, regardless of what else is on the line.
|
|
*
|
|
* `Math.floor(Math.random() * 100)` carries (a) and (b) but no division
|
|
* anywhere on the line (DIVISION_RE finds nothing, divIdx === -1) and is
|
|
* correctly excluded by clause (c) alone.
|
|
*
|
|
* `src/context-utilization.cts`'s `Math.min(Math.round(ratio * 100), 100)`
|
|
* is OUT OF SCOPE BY DOMAIN, not by exemption: it scales an
|
|
* ALREADY-COMPUTED fraction (`ratio`, a context-window utilization figure —
|
|
* unrelated to `.planning/` phase/plan completion) and there is no division
|
|
* anywhere on that line either, so clause (c) excludes it the same way as
|
|
* the `Math.random()` case above; it needs no FUNCTION_SCOPED_EXEMPTIONS
|
|
* entry because it was never going to match.
|
|
*
|
|
* Every regex below is small, bounded, and has no nested/overlapping
|
|
* quantifiers — each character class is followed by a fixed literal or a
|
|
* single `\s*` run bounded by the next required literal, so there is
|
|
* nothing for a backtracking engine to explore more than linearly.
|
|
* `npm run lint:ci` runs CodeQL js/redos over this repo; mirrors the
|
|
* ReDoS discipline of `lint-plan-count-drift.cjs` / `lint-milestone-window-drift.cjs`.
|
|
*
|
|
* The tree-walk / root-confinement / sanitizer machinery is SHARED with the
|
|
* sibling drift guards via `scripts/lib/drift-scan.cjs` (ADR-3180 Decision 4)
|
|
* — see that module for the `isInsideRoot` case-sensitivity note and the
|
|
* `walk` symlink-confinement rationale. This guard's detection shape needs
|
|
* no regex-LITERAL extraction (unlike the milestone-window guard), so it
|
|
* does not use `readRegexLiteralAt`; the reported fragment is simply the
|
|
* trimmed source line, bounded to MAX_REGEX_LITERAL_LEN characters.
|
|
*
|
|
* KNOWN, ACCEPTED limits of a per-line textual scan (same tradeoff the
|
|
* sibling drift guards document): a re-derivation whose division and
|
|
* `Math.round`/scale are split across two DIFFERENT lines with no single
|
|
* line carrying all three tokens is not caught by this narrow shape, nor is
|
|
* one routed through a helper that itself performs the division one call
|
|
* away from the rounding. That is left to code review, not this regex.
|
|
*/
|
|
|
|
const path = require('node:path');
|
|
const driftScan = require('./lib/drift-scan.cjs');
|
|
const { MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree } = driftScan;
|
|
|
|
// (a) The `Math.round`/`Math.floor`/`Math.trunc`/`Math.ceil` rounding family,
|
|
// called with an open paren. `\b` before `Math` keeps this from matching
|
|
// inside a longer identifier (e.g. `fooMath.round(` never occurs in this
|
|
// codebase, but the boundary costs nothing and documents intent).
|
|
const MATH_ROUND_FAMILY_RE = /\bMath\.(?:round|floor|trunc|ceil)\(/;
|
|
|
|
// (b) A `* 100` scale — a `*` operator, optional whitespace, then the
|
|
// literal digits `100` at a word boundary (so `*1000` or `*100.5` do not
|
|
// match a bare `100` inside a longer number).
|
|
const SCALE_100_RE = /\*\s*100\b/;
|
|
|
|
// (c) A division: an identifier character/closing-bracket (the end of the
|
|
// numerator expression), optional whitespace, `/`, optional whitespace, an
|
|
// identifier character/opening-paren (the start of the denominator
|
|
// expression). Deliberately does not try to distinguish this from a regex
|
|
// literal or a `//` comment — the detection window is a Math.round-family
|
|
// call on the same line, which neither idiom co-occurs with in practice, and
|
|
// keeping the class small is what keeps the regex non-backtracking.
|
|
const DIVISION_RE = /[A-Za-z0-9_$)\]]\s*\/\s*[A-Za-z0-9_$(]/;
|
|
|
|
// Authored TypeScript source only (the generated bin/lib/*.cjs mirror it).
|
|
const SCAN_DIRS = ['src'];
|
|
const SCAN_EXT = new Set(['.cts', '.ts', '.mts']);
|
|
|
|
// The canonical owner defines the ratio-to-percent grammar. It is NOT
|
|
// exempt as a whole file (ADR-3180 Decision 4(a) forbids bare file
|
|
// allowlists) — it is scanned like every other file in SCAN_DIRS, and only
|
|
// the two named functions below are exempt, each for a documented reason.
|
|
// An unrelated re-derivation added elsewhere in this same file (including a
|
|
// future one) is still caught.
|
|
const OWNER_FILE = path.join('src', 'phase-lifecycle.cts');
|
|
|
|
// Per ADR-3180 Decision 4(a): function-scoped, not a bare file allowlist.
|
|
// - clampPercentFromFraction: `Math.min(100, Math.round(fraction * 100))`
|
|
// IS the canonical fraction-to-percent kernel this guard exists to
|
|
// protect, not a copy of it — every other caller in the tree is
|
|
// expected to CALL this function rather than re-express its body.
|
|
// - clampPercent: the canonical count-shaped entry point; it delegates to
|
|
// `clampPercentFromFraction(completed / total)` rather than computing
|
|
// `Math.round(...)` itself, so it is exempted for the same reason even
|
|
// though its own line does not currently carry a Math.round-family call.
|
|
const FUNCTION_SCOPED_EXEMPTIONS = new Map([[OWNER_FILE, new Set(['clampPercent', 'clampPercentFromFraction'])]]);
|
|
|
|
// Optional `export ` modifier, matching the sibling guards' convention —
|
|
// only a column-0 top-level `function` declaration updates the
|
|
// current-function tracker.
|
|
const TOP_LEVEL_FUNCTION_RE = /^(?:export\s+)?function\s+([A-Za-z0-9_]+)\s*\(/;
|
|
|
|
/**
|
|
* Pure: find every unsanctioned completion-ratio re-derivation in `text`.
|
|
* `relPath` is the repo-relative path, used both to report file:line and to
|
|
* apply the narrow, function-scoped owner exemptions above.
|
|
* Returns [{ line, found }].
|
|
*/
|
|
function findCompletionRatioDrift(text, relPath) {
|
|
const out = [];
|
|
const lines = text.split('\n');
|
|
const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null;
|
|
let currentFunction = null;
|
|
for (let i = 0; i < lines.length; i++) {
|
|
const line = lines[i];
|
|
const fnMatch = TOP_LEVEL_FUNCTION_RE.exec(line);
|
|
if (fnMatch) currentFunction = fnMatch[1];
|
|
|
|
if (!MATH_ROUND_FAMILY_RE.test(line)) continue;
|
|
const scaleIdx = line.search(SCALE_100_RE);
|
|
if (scaleIdx === -1) continue;
|
|
const divIdx = line.search(DIVISION_RE);
|
|
if (divIdx === -1 || divIdx >= scaleIdx) continue;
|
|
|
|
if (exemptFunctions && exemptFunctions.has(currentFunction)) continue;
|
|
|
|
out.push({ line: i + 1, found: line.trim().slice(0, MAX_REGEX_LITERAL_LEN) });
|
|
}
|
|
return out;
|
|
}
|
|
|
|
/**
|
|
* Scan the authored source tree and return every unsanctioned re-derivation,
|
|
* each annotated with the repo-relative file path.
|
|
*/
|
|
function scanRepo(root) {
|
|
return scanTree({
|
|
root,
|
|
scanDirs: SCAN_DIRS,
|
|
scanExt: SCAN_EXT,
|
|
onFile(rel, text) {
|
|
// `rel` is already the REAL (canonical) path (scanTree resolves
|
|
// symlinks before calling onFile), so this — and
|
|
// FUNCTION_SCOPED_EXEMPTIONS above, also keyed on `rel` — match
|
|
// consistently regardless of which symlink reached the file.
|
|
return findCompletionRatioDrift(text, rel).map((d) => ({ file: rel, ...d }));
|
|
},
|
|
});
|
|
}
|
|
|
|
function main() {
|
|
const root = path.join(__dirname, '..');
|
|
const violations = scanRepo(root);
|
|
if (violations.length === 0) {
|
|
process.stdout.write('ok completion-ratio-drift: no unsanctioned completed/total percent re-derivations outside phase-lifecycle.cts\n');
|
|
return;
|
|
}
|
|
process.stderr.write('completion-ratio-drift: independent re-derivation(s) of completed/total percent found.\n');
|
|
process.stderr.write('Use src/phase-lifecycle.cjs `clampPercent(completed, total)` (or `clampPercentFromFraction(fraction)`\n');
|
|
process.stderr.write('when you already hold a fraction) instead of re-deriving Math.round((completed / total) * 100):\n');
|
|
for (const d of violations) {
|
|
// `d.file` is exactly as attacker-controlled as `d.found`: a repo can
|
|
// legally track a filename containing control bytes / bidi overrides,
|
|
// and it is a fork-PR-authored value reaching a CI log the same way the
|
|
// matched line text does — sanitize it at the same reporting boundary.
|
|
process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${sanitizeForReport(d.found)}\n`);
|
|
}
|
|
process.exitCode = 1;
|
|
}
|
|
|
|
if (require.main === module) main();
|
|
|
|
module.exports = {
|
|
findCompletionRatioDrift,
|
|
scanRepo,
|
|
MATH_ROUND_FAMILY_RE,
|
|
SCALE_100_RE,
|
|
DIVISION_RE,
|
|
OWNER_FILE,
|
|
FUNCTION_SCOPED_EXEMPTIONS,
|
|
MAX_REGEX_LITERAL_LEN,
|
|
};
|