* feat(#3987): guard slug re-derivation, and record why the swallow shape cannot be guarded Epic #3473's Decision 1 requires the wrong call site be UNREPRESENTABLE. #3984 measured that two of the nine §8 rules had no guard at all and recorded both as "Shipped - test-covered". This closes one of them, proves the other cannot be closed the same way, and corrects two false claims I merged yesterday. 1. §8.3 - scripts/lint-slug-derivation-drift.cjs. generateSlugInternal (src/core-utils.cts) is the canonical owner; #3883 removed 11 inline copies. Nothing prevented a twelfth: no slug guard existed in scripts/ or eslint-rules/. The detector is STATEMENT-scoped and matches the shape the real copies took - one statement carrying BOTH .replace(<negated class>, '-') and .replace(/^-+|-+$/, ''). Statement scoping is what buys the precision: the loose LINE-level form yields 18 hits with 7 unrelated, a material false-positive rate. Measured on the tree: 5 flags, 2 TRUE, 3 SANCTIONED, 0 FALSE. The three sanctioned sites are allowlisted with a reason each, following lint-phase-enumeration-drift's form rather than a bare denylist. The owner itself is listed explicitly even though it escapes by construction - an implicit escape is a latent bug, and the next person to touch line 192 would not know the guard depended on it. 2. Both TRUE positives were live defects, not style. scripts/qa-smell-ratchet.cjs reproduced the canonical formula including the 60-cap but trimmed BEFORE truncating - the #2849 bug - and never transliterated. The divergence is total, not cosmetic: canonical "privet-mir-privet-mir-privet-mir-privet-mir-privet-mir-prive" inline "tail" Cyrillic collapsed to nothing and only the ASCII remainder survived, so the ratchet was keying on wrong identifiers for any non-ASCII input. tests/planning-inspect.test.cjs carried a helper whose comment claimed parity with getPhaseDirFromPhaseId. That function now transliterates; the helper did not, so the test asserted against a stale formula while looking correct. Both now route through the seam. 3. §8.5 - measured, and deliberately NOT shipped. A candidate detector (swallowing catch + errno-retry-set test in the same function) gives 26 flags across 11 functions: 0 TRUE, 26 FALSE. Every one is best-effort unlink/rm/close cleanup, lost-rename-race backoff, or a deliberate null fallback. The file-scoped variant is worse at 71. Worse than the noise: the only known true instance was removed by #3885, so there is NO POSITIVE CONTROL - the guard cannot be shown capable of failing, which this repo requires of every drift guard. Shipping it would add a guard nobody can trust and nobody can test. The ADR now records the measurement and the reason, keeps §8.5 at "Shipped - test-covered", and points at the #1884 regression test as what actually enforces it. An honest "not detectable at acceptable precision" beats a guard that only ever passes. 4. Two claims I merged into the ADR yesterday were wrong. §8.9 said 17 of 19 subsumed children have a test citing their issue number, and that #3364 and #3812 have none. Both halves are false, and the claim came from a NUMBER-GREP - inside an amendment whose own subject is that a text match is not a fact. #3364 IS cited: tests/runtime-marker-resolution.test.cjs:107, T3 installMarkerResolvesWhenEnvAndConfigAbsent_3897 (#3364), asserting at :115-119. #3812 IS covered: tests/gen-state-md-docs.test.cjs:374, asserting at :382. Corrected to 19 of 19. #3812 does carry a real finding, though a different one: it is PARTIALLY DELIVERED on a CLOSED issue. The shipped fix declares cardinality for frontmatter keys, but #3812's stated acceptance was about the ## Current Position BODY section, and docs/reference/state-md.md:196-208 still has no normative single-valued/overwrite sentence and no pointer to ## Performance Metrics for history. Recorded in the ADR and left for #3812 to re-open - fixing it here would bury a scope question inside an unrelated PR. Note on B6: this ADDS a guard, and B6 said the net count must fall. #3951 already amended that clause - a guard ledger is a claim about COVERAGE, not count - which is what makes adding this one honest rather than contradictory. Verified: the guard flags 0 on the fixed tree, and PROVES IT CAN FAIL - a fresh inline copy planted in src/ makes it exit 1 naming the exact statement. All three sanctioned sites were confirmed exempt BY the allowlist, not by accident of the pattern, by re-attributing each to a non-exempt path and watching it flag. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): add the changeset fragment Doc-only, so it carries forward from the verified sha rather than costing a second matrix run. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): §8.5 IS guardable — I was wrong, and the guard found a live defect Two orthogonal reviews. The correctness review overturned my central judgment, and it was right. 1. I concluded §8.5 was "not detectable at acceptable precision" and recorded that in the ADR. False. My evidence was 26 flags / 0 TRUE / 26 FALSE. The reviewer pointed out what I had not: all 26 false positives are CLEANUP verbs - rmSync 54, unlinkSync 43, closeSync 17, chmodSync 12 - and the obvious narrower predicate was never tried. A swallowed cleanup is legitimate best-effort. A swallowed CREATION is a precondition silently lost, which is exactly the #1884 shape. Measured properly, in three stages: swallowing catch 911 + try-block calls a CREATION verb 24 + enclosing function references a *_ERRNOS set 0 0 flags, 0 false positives. The `*_ERRNOS` naming key is empirically total - all 10 retry/tolerate sets in src/ follow it. My second claim was worse. I wrote that no positive control exists because #3885 removed the only true instance, so the guard "cannot be shown capable of failing". That is self-refuting: this very PR's slug guard proves-it-can-fail on a synthetic tree, and the pre-#3885 blob is available as exactly such a fixture. It is now the control, and it works in both directions - the rule flags 0c43d853e^:src/planning-workspace.cts at line 210, the line the fix commit's own message cites, and reports zero on the post-fix code. I stopped at the first negative result on the option that meant less work. Shipped as eslint-rules/no-swallowed-precondition.cjs, wired into the existing src/**/*.cts ESLint block rather than a scripts/lint-*-drift.cjs: no script in scripts/ requires typescript/espree/acorn, and scripts/ ships to consumers, so a .cts-parsing standalone guard would add a devDep at consumer runtime. The ESLint block already parses .cts for free. 2. The guard immediately found a live defect of the same class. src/capability-lock.cts swallowed a mkdirSync on the lock directory, then acquireLock classified the follow-on failure as `code !== 'EEXIST' → return null`. A real EACCES/EROFS makes openSync(lockPath,'wx') fail ENOENT, which is not EEXIST - so a fatal filesystem error was laundered into "lock unavailable". Same defect as #1884, different laundering target. Fixed the way #3885 fixed #1884: the creation failure propagates. Regression test proven fail-first by hand - with the fix stashed, EACCES was laundered to null; restored, it throws. The strict rule does NOT catch this shape (its errno classification is an inline literal, not a named set). The rule is deliberately left strict: the broadened form had 2 false positives - capability-lock.cts:408, the deliberate EEXIST steal protocol, and commonjs-marker.cts:131, which returns a distinct documented outcome. The gap is noted in code rather than papered over with a noisy predicate. 3. The security review found the slug guard's exemption FAILED OPEN. currentFunction was never reset, and only a column-0 `function` declaration updated it, so exemption bled from an allowlisted declaration to the next one. generateSlugInternal exempted 50 lines for an 11-line function. A re-derivation planted anywhere in that window was silently exempt - the same fail-open shape that produced a blocker in #3897, and an allowlist is a SUBTRACTION so a mismatch fails open by construction. Extent is now tracked by real brace depth, and a test plants a violation after each allowlisted function's real closing brace and asserts it IS flagged. 4. Also from the security review: the guard was a CI-DoS and narrower than I claimed. Its unbounded [^\]]* was re-scanned from every `.replace(/[^` start: 54.3s on a 1.28MB line. It imported MAX_REGEX_LITERAL_LEN and never called readRegexLiteralAt - the bounded tokenizer that exists for exactly this. Now routed through it with a 2MB file cap: ~200ms. 15 of 25 genuine re-derivations evaded. Widened to catch replaceAll, {1,}, \s*-wrapped classes, escaped ], literal new RegExp(...), five trim spellings, .split().join(), and multi-line .replace( args - still 0 false positives. Two forms still evade and are documented as deliberate gaps with negative tests: the two-statement/temp-var form and new RegExp built from a variable. Both need data flow, and guessing at it is how a guard becomes noisy. Also fixed: // inside a string truncated the line, a ; inside the collapse regex split the statement (a one-character bypass), and SCAN_EXT omitted .mjs/.tsx/.jsx. 5. A regression I introduced, caught by the same review. qa-smell-ratchet.cjs top-level-required a build output that is not git-tracked, so the script hard-failed MODULE_NOT_FOUND before build:lib - including for --help, which previously had no build dependency. The require is now lazy at the point of use. 6. Four of my own tests were vacuous or weak. T9's input yielded an identical string under the buggy formula, so it passed on the implementation it was meant to catch. T12 compared maxLen null vs 60 on an 18-char name, where they agree trivially. T9-T12 all asserted generateSlugInternal directly, so they would pass unchanged if both call-site fixes were reverted. And prove-it-can-fail was scoped to scanRepo, never the CLI - dropping main()'s exit-code line would have kept every row green. All rewritten with discriminating inputs, per-call-site rows that red when the fix is reverted, 59/60/61 boundaries, an entirely-non-alphanumeric row, and a CLI row asserting the real subprocess exit code and both sanitizeForReport sites. Verified: both guards flag 0 on the tree and both prove they can fail. The swallow rule's control is confirmed in both directions - pre-#1884 shape flagged, post-#3885 shape clean. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3987): record that §8.5 IS guardable, and correct a correction that made a ledger worse Three ADR corrections, two of them to text this branch wrote hours ago. §8.5 advances to Enforced. Its previous entry said the rule was not detectable at acceptable precision. That was wrong twice: the 26 false positives were uniformly CLEANUP verbs, which is a reason to narrow the predicate rather than abandon it, and the claim that no positive control exists was self-refuting - the pre-#3885 blob is available as a fixture and this repo's own guards prove-it-can- fail on synthetic trees. Narrowed to creation verbs plus a *_ERRNOS reference: 911 -> 24 -> 0 flags, 0 false positives, control confirmed in both directions. The entry keeps the wrong reasoning visible, because a high false-positive count being evidence the predicate is wrong - not evidence the rule is unguardable - is the transferable part, and the first negative result is most seductive when it is also the answer that means less work. §8.9's correction is itself corrected. The original 17-of-19 claim was CORRECT for the predicate it stated; this branch silently swapped cited -> covered and declared 19 of 19. #3812 appears in zero test files. Changing what a word means to make a ledger read better is a worse failure than the miscount it claimed to repair. Both predicates are now reported separately - 18 of 19 cited, 19 of 19 covered - because §8.9 asks for a test NAMING each child, so 18 is the number that answers it. #3812 is also re-opened for real, rather than the first draft's promise that it could be. §8.3 stays Shipped - test-covered rather than advancing. The slug guard catches the copy-paste class and a dozen variants, but two forms still evade by decision (temp-var split, new RegExp from a variable) because both need data flow. Naming them keeps the status honest: the wrong call site is much harder to write, not unrepresentable. Closes #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): backfill changeset pr number Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): replace my own wall-clock assertion, and close the guard that let me write it CI went red on ubuntu shard 2/3. The failing test was mine, and the failure was the test, not the code. a 1.28MB line ... scans in well under a second (was 54.3s pre-fix) 7368ms It asserted ELAPSED TIME. ~200ms locally, 7.4s on a shared CI runner. The bound introduced for the MAJOR-2 DoS fix works - 7.4s against a 54.3s pre-fix baseline is the fix doing its job - but an absolute wall-clock threshold on shared hardware is a race, not an assertion. CLAUDE.md says so directly: "Clock Seams: Do not assert on wall-clock time." I wrote the anti-pattern the project bans, in a PR about guards. Raising the threshold would only move the flake. The row now asserts a DETERMINISTIC bound instead: an instrumentation seam on drift-scan.cjs counts readRegexLiteralAt calls and characters examined, and the test asserts charsExamined stays under an absolute ceiling. Measured on the same 1.28MB fixture: 120,000 calls, 48,000,000 chars - two orders under the ceiling. The pathological fixture is kept; only the thing being asserted changed. Proven to still discriminate: with MAX_REGEX_LITERAL_LEN raised to simulate the unbounded pre-fix behavior, the same fixture does not complete in 120 seconds, versus ~0.3s bounded. It is a real regression test, not a tautology. Then the second half, which is the same defect class as the rest of this PR. eslint-rules/no-elapsed-assertion.cjs matched only the EXACT identifiers ^(elapsed|duration|took|ms)$. I used `elapsedMs`. It evaded the rule entirely. tookMs, durationMs, elapsedTime and msElapsed evade the same way. A guard that cannot see the violation it exists to catch is exactly what this PR is about - it just happened to be an existing rule rather than one of the two I came here for, and it was found because I committed the violation it should have blocked. Widened to /^(?:elapsed|duration|took|ms)(?:[A-Z]\w*)?$/ plus a narrow start/endMs delta pair. Deliberately NOT a blanket *Ms suffix: a first draft did that and produced 2 false positives on `timeoutMs` in plan-phase-stall-detection, which is a configured timeout and not a measurement. Verified negative on params, items, forms, terms, dirnames, timeoutMs, cacheTtlMs and staleAfterMs. Measured over the five files carrying camelCase timing identifiers: 0 true positives beyond my own, so nothing else needed rewriting. The rule's own test file gains a row asserting `elapsedMs` flags, proven to fail against the pre-widening rule - the same prove-it-can-fail standard both new guards in this PR are held to. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a comment I added leaked a Claude reference into every runtime install The runner went red with 4 failures in tests/install.test.cjs: Leaking: .hermes/scripts/lib/drift-scan.cjs Leaking: .qwen/scripts/lib/drift-scan.cjs The instrumentation seam added for the deterministic bound carried a comment naming CLAUDE.md as the source of the no-wall-clock-assertions rule. scripts/ SHIPS to consumers, so that comment was installed verbatim into hermes and qwen trees, and the install suite scans for exactly this - a Claude-specific reference reaching a non-Claude runtime. The rule is real and worth citing; the filename is not portable. The comment now says "this repo's test rules" and states the rule inline, which is what a reader of an installed tree actually needs anyway. Worth noting what caught it: not lint, and not the two guards this PR adds - the install suite's full-tree scan, which exists precisely because a shipped file is read by runtimes that have never heard of CLAUDE.md. Same lesson as the rest of this PR from the other direction: the check that matters is the one that can see the surface where the defect actually lands. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a test fixture swallowed 46 git exit codes and produced a silent false negative CI red on ubuntu shard 3/3: tests/health-validation.test.cjs:2029 expected exactly one W024, got [{"code":"W006", ...}] Not caused by this branch, and the evidence is decisive rather than a hunch: the SIBLING test at :2039 builds the IDENTICAL fixture with the identical commitsAhead and asserts the same thing, and it PASSED in the same process, same file, same run. Same input, both outcomes - which rules out logic, ordering, sharding and environment, and leaves a per-invocation nondeterministic failure inside one fixture build. The mechanism is an unchecked exit code, 46 times over. The W024 fixture performs ~46 runGit spawns and never checks a single one. runGit returns failures as DATA and never throws, so one silently-failed `git commit` yields 19 commits instead of 20, or a silently-empty `git rev-parse HEAD` yields a blank state_head. Either drops readStateHeadFreshness below the advisory threshold, W024 never fires, and only W006 remains. Reproduced exactly: 20 commits -> ["W006","W024"]; 19 -> ["W006"]; blank state_head -> ["W006"] - byte-identical to the CI assertion dump. The arithmetic is what hid it. At threshold-1 and threshold+1 a lost commit still produces the asserted answer; only the exactly-at-threshold cases sit one commit from a false negative. Two of the seven tests are in that position, and CI hit one. That is why it had never been seen before, and why it surfaced now: this branch adds three test files, which reshuffles the cost-weighted shard partition and moved this file into a chunk where the latent flake fired. My files were checked as suspects first and cleared: all fixtures mkdtemp-unique, no process.chdir, no .planning/ writes, no git spawns, and node --test gives per-file process isolation regardless. Fixed at the cause, not the symptom. A mustGit wrapper throws on a non-zero exit with the command, exit code and stderr, and all nine call sites route through it. The fixture now asserts its OWN preconditions before the assertion under test runs - the seed head is non-empty, and `git rev-list --count <seed>..HEAD` equals the requested commitsAhead - so a fixture that did not build what it claims fails loudly as a FIXTURE ERROR naming got-versus-asked, instead of quietly handing a weaker input to the assertion. Proven: dropping one commit now raises FIXTURE ERROR: requested commitsAhead=19 but git rev-list --count reports 18 where it previously produced a silent ["W006"] pass-for-the-wrong-reason. 64/64 tests in that block pass unperturbed. Deliberately NOT done: no threshold change, no retry, no loosened assertion, no skip. The assertion was correct; the input was silently wrong. Worth naming, because it is the same shape from the other side: this PR ships eslint-rules/no-swallowed-precondition.cjs, whose entire subject is a swallowed precondition failure being laundered into a plausible downstream outcome. This fixture is that defect in test code - the swallowed git failure was laundered into a legitimate-looking "W024 did not fire". The rule does not cover test fixtures, so the connection is noted at the fix site rather than enforced. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): two tests wrote to committed files; the shard packing decided when that mattered CI red on windows-latest shard 1/3 only: "gen-exit-code-registry: CLI" > "a --write run redirected to a tmpdir leaves every committed artifact untouched" AssertionError: hooks artifact must be untouched The Linux runner passed the same sha at 40425/40425. It is Linux-only, so a Windows-scheduling defect is structurally invisible to it. Root cause, established by measurement rather than inference. tests/cli-exit.test.cjs appended a corruption marker to the REAL COMMITTED hooks/lib/exit-code-registry.js, held it corrupted across a full subprocess, and restored it in a finally. tests/exit-code-registry.test.cjs reads that same real file before and after its own subprocess and asserts byte equality. If it samples while the other test holds the file corrupted, it fails. The landmine is pre-existing, from2ea5efc15(#3911). What this branch changed is WHEN the two run together. scripts/run-tests.cjs shards by cost-weighted LPT over the sorted unit list, so adding three test files repacks the bins: merge-basec3e667df3(838 files): cli-exit -> shard 1, exit-code-registry -> shard 3 HEAD 03b342601 (841 files): BOTH in shard 1, same argv chunk, one node --test process, concurrent Co-location is necessary but not sufficient - Linux shard 1/3 also had both and passed. Windows loses because TEST_CONCURRENCY defaults to 2 there against 4 elsewhere, spawn cost is ~10x, and the sibling corruptor holds one of only two slots through a ~90s tsc compile. That turns a sub-second overlap into seconds. Not a path-separator or case-sensitivity issue, and not CRLF - .gitattributes pins * text=auto eol=lf. Redirection was not at fault either: ensureScriptsOut derives all five --out flags correctly and gen-exit-code-registry.cjs honours them with no __dirname escape. Fixed at the cause: no test writes to a committed file any more. Both corruptors now copy to a mkdtempSync tmpdir, corrupt the COPY, and point the generator at it. Repinning or reordering the shards would have turned CI green while leaving the landmine armed for the next reshuffle. That required closing an inconsistency between two sibling generators. gen-exit-code-registry.cjs already accepts --out/--scripts-out/--hooks-out/--dts-out/--sh-out and honours them under --check; gen-hooks-cli-exit.cjs hardcoded OUTPUT_PATH and had no flag surface at all, so its corruptor could not be redirected anywhere. It now takes --out in the same style, honoured by both --write and --check, and is a no-op when absent - verified: a bare --check on the default path still exits 0. ensureScriptsOut moved to tests/helpers/exit-code-artifact-flags.cjs and both test files import it. Hand-rolling a second copy of the flag derivation would have been a re-derivation of exactly the kind this PR ships a guard against. Verified: both tests still detect corruption (proven by defeating the check and watching them red, with a positive control showing an uncorrupted copy exits 0); SHA-256 of hooks/lib/exit-code-registry.js and hooks/lib/cli-exit.js identical before and after running both rewritten bodies, and git reports nothing under hooks/ modified - that is the property that was violated. A repo-wide search for the corrupt-then-restore-in-finally shape against hooks/ found no other instances. One detail worth recording: the tmpdir test keeps --declaration pointed at the real committed declaration rather than copying it, because the generated banner embeds path.relative(REPO_ROOT, declarationPath) - copying it would produce a false drift unrelated to the injected corruption. The declaration is read-only on that path and never written. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
823 lines
35 KiB
JavaScript
823 lines
35 KiB
JavaScript
#!/usr/bin/env node
|
|
'use strict';
|
|
|
|
/**
|
|
* qa-smell-ratchet.cjs — turn a QA-walk "smell" into a decision (#2966).
|
|
*
|
|
* WHY THIS FILE EXISTS
|
|
* ────────────────────
|
|
* `tests/qa/run-report.cjs` computes "smells" — legal-but-questionable engine
|
|
* behavior (see `oracles.cjs`'s `SEVERITY.SMELL`) — and writes them into a
|
|
* gitignored `qa-report.json` that nothing reads. In CI, that means every
|
|
* smell is invisible: a NEW one can appear silently and nobody notices. This
|
|
* script is the pipeline that turns a smell into a decision.
|
|
*
|
|
* ══════════════════════════════════════════════════════════════════════════
|
|
* THE DESIGN INVARIANT (read this before touching anything below)
|
|
* ══════════════════════════════════════════════════════════════════════════
|
|
* A smell must NEVER fail a build on its own merits. What fails is an
|
|
* UNACKNOWLEDGED NEW smell — i.e. the absence of a human decision.
|
|
* Existing/known smells stay green forever.
|
|
*
|
|
* Concretely, that means:
|
|
* - A smell whose fingerprint (`tests/qa/smell-fingerprint.cjs`) is already
|
|
* recorded in `tests/qa/smell-baseline.json` OR in any fragment under
|
|
* `tests/qa/smell-acks/` is KNOWN and never fails the build, no matter
|
|
* how many times it fires or how bad it sounds.
|
|
* - A smell whose fingerprint has never been seen before is NEW, and fails
|
|
* the build — not because the behavior is wrong (it may be perfectly
|
|
* fine), but because nobody has looked at it and said so in writing.
|
|
* - The baseline is SHRINK-ONLY: an entry that stops firing (the engine
|
|
* was fixed, or the scenario changed) becomes STALE and ALSO fails the
|
|
* build, forcing `--update` to prune it. A baseline that only ever grows
|
|
* would let acknowledgments outlive the behavior they describe.
|
|
* - A VIOLATION (`SEVERITY.VIOLATION` — the engine broke a documented
|
|
* contract) is a completely different thing and is NEVER acknowledgeable
|
|
* through this mechanism: it always fails, baseline or no baseline. This
|
|
* script's whole ratchet apparatus applies to smells alone.
|
|
* - A scenario EXPECTATION FAILURE (a step's declared `expect` did not
|
|
* hold — `step.expectFailures`) is, like a VIOLATION, NEVER
|
|
* acknowledgeable through this ratchet: it has no fingerprint and no
|
|
* baseline/fragment path, and it always fails the build.
|
|
*
|
|
* WHY A BASELINE FILE *AND* A FRAGMENTS DIRECTORY (not just one)
|
|
* ──────────────────────────────────────────────────────────────
|
|
* This follows the exact idiom `tests/emitted-drift-acks/` and `.changeset/`
|
|
* already use in this repo, for the exact same reason: `smell-baseline.json`
|
|
* is a single shared file every PR that acknowledges a smell would otherwise
|
|
* have to rewrite, guaranteeing merge conflicts between any two such PRs in
|
|
* flight at once. A fragment per PR under `tests/qa/smell-acks/` — uniquely
|
|
* named (its own issue/PR number) — means two PRs can never conflict on this
|
|
* seam. A maintainer periodically folds spent fragments into the committed
|
|
* baseline via `--update` and deletes them (see that directory's README).
|
|
*
|
|
* USAGE
|
|
* ─────
|
|
* node scripts/qa-smell-ratchet.cjs # check (CI entry point)
|
|
* node scripts/qa-smell-ratchet.cjs --update # regenerate the baseline
|
|
* node scripts/qa-smell-ratchet.cjs --json <path> # also write the full qa-report
|
|
* node scripts/qa-smell-ratchet.cjs --keep # preserve scenario temp dirs
|
|
* # (real repro commands; see
|
|
* # `report.cjs`'s buildRepro)
|
|
*
|
|
* Exit code 0 only when: zero violations, zero scenario expectation
|
|
* failures, zero NEW smells, zero STALE baseline/fragment entries. Exit
|
|
* code 1 otherwise.
|
|
*/
|
|
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
const { runAllScenarios } = require('../tests/qa/run-report.cjs');
|
|
const { buildReport } = require('../tests/qa/report.cjs');
|
|
const { fingerprint } = require('../tests/qa/smell-fingerprint.cjs');
|
|
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
|
|
|
|
const REPO_ROOT = path.join(__dirname, '..');
|
|
const BASELINE_REL_PATH = 'tests/qa/smell-baseline.json';
|
|
const ACKS_DIR_REL_PATH = 'tests/qa/smell-acks';
|
|
const BASELINE_PATH = path.join(REPO_ROOT, ...BASELINE_REL_PATH.split('/'));
|
|
const ACKS_DIR = path.join(REPO_ROOT, ...ACKS_DIR_REL_PATH.split('/'));
|
|
|
|
/** Bump when `smell-baseline.json` / fragment shape changes incompatibly. */
|
|
const BASELINE_VERSION = 1;
|
|
|
|
/**
|
|
* Upper bound on fragment files read in one pass — this cap is this script's own,
|
|
* for its own acknowledgment set (`tests/qa/smell-acks/`), which ADR-3942 does not
|
|
* touch. The emitted-drift ack this cap used to mirror moved to a commit trailer
|
|
* (ADR-3942) and no longer has a fragment-directory cap of its own to mirror.
|
|
* Exceeding it throws rather than silently truncating the listing, which would
|
|
* silently drop acknowledgments from consideration — exactly the class of silent
|
|
* failure this whole seam exists to prevent.
|
|
*/
|
|
const MAX_ACK_FRAGMENTS = 500;
|
|
|
|
/**
|
|
* `--update` writes this into a newly-discovered entry's OPTIONAL `reason`
|
|
* field (alongside `issue: null`) as a note-to-self, never as a substitute for
|
|
* `issue` — see the header's "THE DESIGN INVARIANT" and #2966 FIX 3. A plain
|
|
* (non-`--update`) run rejects any entry whose `issue` is not a positive
|
|
* integer regardless of what `reason` says, and additionally rejects a
|
|
* `reason` still carrying this placeholder prefix (see `isPlaceholderReason`),
|
|
* so the baseline can never silently ship with a smell nobody has triaged.
|
|
*/
|
|
const PLACEHOLDER_REASON_PREFIX = 'TODO(qa-smell-ratchet):';
|
|
const PLACEHOLDER_REASON =
|
|
`${PLACEHOLDER_REASON_PREFIX} triage this smell — either file a defect and set "issue" to its number (REAL), ` +
|
|
'or fix the oracle so it stops firing (FALSE POSITIVE). A "reason" alone, with no "issue", is never accepted.';
|
|
|
|
const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v);
|
|
const isPlaceholderReason = (reason) => typeof reason === 'string' && reason.startsWith(PLACEHOLDER_REASON_PREFIX);
|
|
|
|
/**
|
|
* Parse CLI argv into `{ update, jsonOut, keep }`.
|
|
*
|
|
* @param {string[]} argv
|
|
* @returns {{ update: boolean, jsonOut: string|null, keep: boolean }}
|
|
*/
|
|
function parseArgs(argv) {
|
|
let update = false;
|
|
let jsonOut = null;
|
|
let keep = false;
|
|
for (let i = 0; i < argv.length; i += 1) {
|
|
const arg = argv[i];
|
|
if (arg === '--update') {
|
|
update = true;
|
|
} else if (arg === '--json') {
|
|
const value = argv[i + 1];
|
|
if (typeof value !== 'string' || value === '') {
|
|
throw new ExitError(2, 'qa-smell-ratchet: --json requires a path argument');
|
|
}
|
|
jsonOut = path.resolve(value);
|
|
i += 1;
|
|
} else if (arg === '--keep') {
|
|
keep = true;
|
|
} else {
|
|
throw new ExitError(
|
|
2,
|
|
`qa-smell-ratchet: unrecognized argument "${arg}" (expected --update, --json <path>, and/or --keep)`,
|
|
);
|
|
}
|
|
}
|
|
return { update, jsonOut, keep };
|
|
}
|
|
|
|
/**
|
|
* Validate one baseline/fragment entry, pushing a message per problem onto
|
|
* `errors`. Does not mutate `entry`.
|
|
*
|
|
* Every entry MUST carry the three string fields (`key`, `id`, `scenario`)
|
|
* AND a positive-integer `issue` — the ONLY two terminal states for a smell
|
|
* are REAL (an assigned defect, cited by its issue number) or FALSE POSITIVE
|
|
* (the oracle gets fixed and the entry is never baselined at all); there is
|
|
* no third "accepted with a good explanation" state, so a free-text `reason`
|
|
* can NEVER substitute for `issue` (#2966 FIX 3). `reason` remains an OPTIONAL
|
|
* human note: when present it must be a non-empty, non-placeholder string,
|
|
* but its absence is never itself an error.
|
|
*
|
|
* @param {unknown} entry
|
|
* @param {string} where human-readable location for error messages
|
|
* (e.g. `"tests/qa/smell-baseline.json.smells[3]"` or a fragment's own
|
|
* relative path).
|
|
* @param {string[]} errors
|
|
* @returns {boolean} true when `entry` has all required fields, a valid
|
|
* `issue`, and (if present) a real (non-placeholder) `reason`.
|
|
*/
|
|
function validateEntryFields(entry, where, errors) {
|
|
if (!isPlainObject(entry)) {
|
|
errors.push(`${where} must be an object, got ${JSON.stringify(entry)}`);
|
|
return false;
|
|
}
|
|
let ok = true;
|
|
for (const field of ['key', 'id', 'scenario']) {
|
|
if (typeof entry[field] !== 'string' || entry[field] === '') {
|
|
errors.push(`${where}.${field} must be a non-empty string, got ${JSON.stringify(entry[field])}`);
|
|
ok = false;
|
|
}
|
|
}
|
|
if (!Number.isInteger(entry.issue) || entry.issue <= 0) {
|
|
errors.push(
|
|
`${where}.issue must be a positive integer, got ${JSON.stringify(entry.issue)} — every acknowledged smell ` +
|
|
'must be REAL (an assigned defect, cited by issue number) or a FALSE POSITIVE (the oracle is fixed, never ' +
|
|
'baselined); a free-text "reason" can never substitute for a tracked issue number',
|
|
);
|
|
ok = false;
|
|
}
|
|
if (entry.reason !== undefined) {
|
|
if (typeof entry.reason !== 'string' || entry.reason === '') {
|
|
errors.push(`${where}.reason, when present, must be a non-empty string, got ${JSON.stringify(entry.reason)}`);
|
|
ok = false;
|
|
} else if (isPlaceholderReason(entry.reason)) {
|
|
errors.push(
|
|
`${where}.reason is still the placeholder ("${entry.reason}") — either remove it or replace it with a `
|
|
+ 'real human note; either way, "issue" (not "reason") is what makes this entry valid',
|
|
);
|
|
ok = false;
|
|
}
|
|
}
|
|
return ok;
|
|
}
|
|
|
|
/**
|
|
* Read and validate `tests/qa/smell-baseline.json`.
|
|
*
|
|
* @param {{ allowMissing: boolean }} opts `allowMissing: true` is used only
|
|
* by `--update`'s bootstrap path — a not-yet-existing baseline is the
|
|
* expected first-run state there, never an error. In check mode a missing
|
|
* baseline is always an error (there is nothing to ratchet against).
|
|
* @returns {{ entries: Array<{key:string,id:string,scenario:string,issue:number,reason?:string}>, errors: string[], existed: boolean }}
|
|
*/
|
|
function readBaseline({ allowMissing }) {
|
|
const existed = fs.existsSync(BASELINE_PATH);
|
|
if (!existed) {
|
|
if (allowMissing) return { entries: [], errors: [], existed };
|
|
return {
|
|
entries: [],
|
|
errors: [`${BASELINE_REL_PATH} is missing — run \`node scripts/qa-smell-ratchet.cjs --update\` to generate it`],
|
|
existed,
|
|
};
|
|
}
|
|
|
|
const raw = fs.readFileSync(BASELINE_PATH, 'utf8');
|
|
if (raw.trim() === '') {
|
|
return { entries: [], errors: [`${BASELINE_REL_PATH} is present but empty`], existed };
|
|
}
|
|
let doc;
|
|
try {
|
|
doc = JSON.parse(raw);
|
|
} catch (err) {
|
|
return { entries: [], errors: [`${BASELINE_REL_PATH} is not valid JSON: ${err.message}`], existed };
|
|
}
|
|
const errors = [];
|
|
if (!isPlainObject(doc)) {
|
|
errors.push(`${BASELINE_REL_PATH} must be a JSON object, got ${Array.isArray(doc) ? 'array' : typeof doc}`);
|
|
return { entries: [], errors, existed };
|
|
}
|
|
if (doc.version !== BASELINE_VERSION) {
|
|
errors.push(`${BASELINE_REL_PATH}: unsupported version ${JSON.stringify(doc.version)} (expected ${BASELINE_VERSION})`);
|
|
}
|
|
if (!Array.isArray(doc.smells)) {
|
|
errors.push(`${BASELINE_REL_PATH}: "smells" must be an array, got ${JSON.stringify(doc.smells)}`);
|
|
return { entries: [], errors, existed };
|
|
}
|
|
|
|
const entries = [];
|
|
doc.smells.forEach((entry, i) => {
|
|
const where = `${BASELINE_REL_PATH}.smells[${i}]`;
|
|
if (validateEntryFields(entry, where, errors)) entries.push(entry);
|
|
});
|
|
return { entries, errors, existed };
|
|
}
|
|
|
|
/**
|
|
* Fragment filenames under `tests/qa/smell-acks/`, sorted. Absent directory
|
|
* == zero fragments. Throws (naming the dir, cap, and actual count) rather
|
|
* than silently truncating when the cap is exceeded.
|
|
*
|
|
* @returns {string[]}
|
|
*/
|
|
function listFragmentFiles() {
|
|
if (!fs.existsSync(ACKS_DIR)) return [];
|
|
const names = fs.readdirSync(ACKS_DIR).filter((n) => n.endsWith('.json')).sort();
|
|
if (names.length > MAX_ACK_FRAGMENTS) {
|
|
throw new ExitError(
|
|
1,
|
|
`qa-smell-ratchet: ${ACKS_DIR_REL_PATH} contains ${names.length} ack fragments, exceeding the cap of `
|
|
+ `${MAX_ACK_FRAGMENTS}. Refusing to read only some of them — a truncated read would silently drop `
|
|
+ 'acknowledgments. Prune spent fragments from this directory.',
|
|
);
|
|
}
|
|
return names;
|
|
}
|
|
|
|
/**
|
|
* Read and validate every fragment under `tests/qa/smell-acks/`. Each
|
|
* fragment is ONE acknowledgment: the same shape as a baseline entry — `key`,
|
|
* `id`, `scenario`, a positive-integer `issue`, and an OPTIONAL `reason` —
|
|
* validated identically via `validateEntryFields` (#2966 FIX 3: there is no
|
|
* separate "acknowledge via PR number" path; every acknowledgment cites the
|
|
* issue tracking the underlying defect).
|
|
*
|
|
* @returns {{ entries: Array<{key:string,id:string,scenario:string,issue:number,reason?:string,_source:string}>, errors: string[] }}
|
|
*/
|
|
function readAckFragments() {
|
|
const errors = [];
|
|
const entries = [];
|
|
for (const name of listFragmentFiles()) {
|
|
const label = `${ACKS_DIR_REL_PATH}/${name}`;
|
|
const raw = fs.readFileSync(path.join(ACKS_DIR, name), 'utf8');
|
|
if (raw.trim() === '') {
|
|
errors.push(`${label} is present but empty`);
|
|
continue;
|
|
}
|
|
let doc;
|
|
try {
|
|
doc = JSON.parse(raw);
|
|
} catch (err) {
|
|
errors.push(`${label} is not valid JSON: ${err.message}`);
|
|
continue;
|
|
}
|
|
if (!validateEntryFields(doc, label, errors)) continue;
|
|
entries.push({ ...doc, _source: label });
|
|
}
|
|
return { entries, errors };
|
|
}
|
|
|
|
/**
|
|
* Merge baseline entries and ack fragments into one `key -> entry` map (the
|
|
* full set of KNOWN smells this run is ratcheted against), plus the list of
|
|
* fragments that are now redundant because the baseline already carries
|
|
* their key (an advisory, not a failure — see this file's header on why
|
|
* cross-source duplication is not hard-blocked here).
|
|
*
|
|
* @param {Array<{key:string}>} baselineEntries
|
|
* @param {Array<{key:string,_source:string}>} fragmentEntries
|
|
* @returns {{ byKey: Map<string, object>, redundantFragments: Array<{key:string, source:string}> }}
|
|
*/
|
|
function mergeKnown(baselineEntries, fragmentEntries) {
|
|
const byKey = new Map();
|
|
for (const e of baselineEntries) byKey.set(e.key, { ...e, source: BASELINE_REL_PATH });
|
|
|
|
const redundantFragments = [];
|
|
for (const e of fragmentEntries) {
|
|
if (byKey.has(e.key)) {
|
|
redundantFragments.push({ key: e.key, source: e._source });
|
|
continue;
|
|
}
|
|
byKey.set(e.key, { ...e, source: e._source });
|
|
}
|
|
return { byKey, redundantFragments };
|
|
}
|
|
|
|
/**
|
|
* Walk `reportObject.scenarios[].steps[]` and split every finding into three
|
|
* buckets: `smells` (fingerprinted, ratcheted against the baseline),
|
|
* `violations` (never acknowledgeable — see this file's header), and
|
|
* `expectationFailures` (a step's declared `expect` did not hold; also never
|
|
* acknowledgeable — see this file's header). All three carry the step's
|
|
* `repro` command for later use in failure messages / the GitHub step
|
|
* summary.
|
|
*
|
|
* `expectationFailures` entries deliberately carry NO `key` field and are
|
|
* never passed through `fingerprint()`: unlike a smell, an expectation
|
|
* failure has no baseline/fragment acknowledgment path at all, so giving it
|
|
* a fingerprint would invite exactly the laundering this ratchet exists to
|
|
* prevent (#3597).
|
|
*
|
|
* INVARIANT: `violations.length + expectationFailures.length` must always
|
|
* equal `reportObject.totals.violations` — see `tests/qa/report.cjs`'s
|
|
* `buildReport()`, which computes that total the same way. This is the
|
|
* parity that broke in #3597: this function used to read only
|
|
* `step.violations`, so a scenario whose `expect` failed produced
|
|
* `totals.violations: 1` while this script counted (and printed) 0.
|
|
*
|
|
* @param {ReturnType<import('../tests/qa/report.cjs').buildReport>} reportObject
|
|
* @returns {{
|
|
* smells: Array<{key:string,id:string,scenario:string,argv:string[],detail:string,at:string,repro:string}>,
|
|
* violations: Array<{id:string,scenario:string,argv:string[],detail:string,at:string,repro:string}>,
|
|
* expectationFailures: Array<{scenario:string,argv:string[],detail:string,at:string,repro:string}>,
|
|
* }}
|
|
*/
|
|
function collectFindings(reportObject) {
|
|
const smells = [];
|
|
const violations = [];
|
|
const expectationFailures = [];
|
|
for (const scenario of reportObject.scenarios) {
|
|
for (const step of scenario.steps) {
|
|
for (const v of step.violations || []) {
|
|
violations.push({
|
|
id: v.id, scenario: scenario.name, argv: step.argv, detail: v.detail, at: step.at, repro: step.repro,
|
|
});
|
|
}
|
|
for (const detail of step.expectFailures || []) {
|
|
expectationFailures.push({
|
|
scenario: scenario.name, argv: step.argv, detail, at: step.at, repro: step.repro,
|
|
});
|
|
}
|
|
for (const smell of step.smells || []) {
|
|
const key = fingerprint(scenario.name, { id: smell.id, subject: smell.subject, argv: step.argv });
|
|
smells.push({
|
|
key,
|
|
id: smell.id,
|
|
scenario: scenario.name,
|
|
argv: step.argv,
|
|
detail: smell.detail,
|
|
at: step.at,
|
|
repro: step.repro,
|
|
});
|
|
}
|
|
}
|
|
}
|
|
return { smells, violations, expectationFailures };
|
|
}
|
|
|
|
/**
|
|
* Lowercase, transliterate, hyphenate, and strip anything that isn't
|
|
* `[a-z0-9-]`, for a fragment-filename skeleton.
|
|
*
|
|
* Routed through the canonical `generateSlugInternal` seam (`src/core-utils.cts`,
|
|
* issue #3987) instead of hand-rolling the same collapse/strip/truncate shape:
|
|
* this local copy trimmed leading/trailing hyphens BEFORE truncating to 60
|
|
* chars, which is the live #2849 bug (`.slice(0, 60)` can land on a separator,
|
|
* re-introducing a trailing hyphen the strip step was meant to prevent), and
|
|
* it never transliterated non-Latin scripts (#2848). `generateSlugInternal`
|
|
* returns `null` for empty/nullish input; a fragment-filename skeleton needs a
|
|
* string, so `?? ''` preserves this function's prior never-null contract.
|
|
*
|
|
* `gsd-core/bin/lib/core-utils.cjs` is required LAZILY, here, rather than at
|
|
* module load — it is `src/core-utils.cts`'s gitignored `build:lib` output,
|
|
* so a top-level `require` made this ENTIRE script (including `--help`, which
|
|
* never calls `slugify`) hard-fail `MODULE_NOT_FOUND` on a fresh clone before
|
|
* any build ran. Deferring the require to the one call site that actually
|
|
* needs it means every other code path (in particular `--help`) still works
|
|
* with `gsd-core/bin/lib/` absent, and a genuinely missing build only surfaces
|
|
* as an error when a NEW smell finding is rendered (the only caller of this
|
|
* function).
|
|
*/
|
|
function slugify(value) {
|
|
let generateSlugInternal;
|
|
try {
|
|
({ generateSlugInternal } = require('../gsd-core/bin/lib/core-utils.cjs'));
|
|
} catch (err) {
|
|
if (err && err.code === 'MODULE_NOT_FOUND') {
|
|
throw new ExitError(
|
|
1,
|
|
'qa-smell-ratchet: gsd-core/bin/lib/core-utils.cjs is missing — run `npm run build:lib` first.',
|
|
);
|
|
}
|
|
throw err;
|
|
}
|
|
return generateSlugInternal(value, 60) ?? '';
|
|
}
|
|
|
|
/**
|
|
* Render the paste-ready fragment skeleton for one NEW smell finding.
|
|
*
|
|
* @param {{key:string,id:string,scenario:string}} finding
|
|
* @returns {string}
|
|
*/
|
|
function fragmentSkeleton(finding) {
|
|
const doc = {
|
|
version: 1,
|
|
key: finding.key,
|
|
id: finding.id,
|
|
scenario: finding.scenario,
|
|
issue: '<YOUR ISSUE NUMBER>',
|
|
};
|
|
const suggestedName = `${ACKS_DIR_REL_PATH}/<issue>-${slugify(finding.id)}-${slugify(finding.scenario)}.json`;
|
|
return `${suggestedName}:\n${JSON.stringify(doc, null, 2)}`;
|
|
}
|
|
|
|
/**
|
|
* Flatten one untrusted, scenario-authored string for safe single-line
|
|
* rendering into CI logs and the GitHub step summary.
|
|
*
|
|
* `detail` / `scenario` / `at` values originate in scenario JSON
|
|
* (`expect[].path` reaches `detail` verbatim via `evaluateExpectations`)
|
|
* and are validated only as non-empty strings. Rendered raw into
|
|
* `$GITHUB_STEP_SUMMARY` — which GitHub renders as markdown — a newline
|
|
* plus a forged heading or a fake "0 expectation failures" line lets a
|
|
* red run present a green-looking summary; a backtick breaks out of the
|
|
* code span it is rendered inside; an ANSI escape repaints the CI log.
|
|
*
|
|
* The 300-char truncation is a SEPARATE concern from the neutralization
|
|
* above and is controllable via `maxLen`: every existing caller keeps the
|
|
* default (a long `detail`/`scenario`/`at` value is fine to summarize), but
|
|
* the `repro` field is a copy-pasteable command — truncating it produces a
|
|
* string that *looks* like a complete, runnable command but silently isn't
|
|
* (it dies mid-argv or mid-path), which is worse than no repro at all. Pass
|
|
* `{ maxLen: Infinity }` at those call sites to lift the cap while keeping
|
|
* every other neutralization (newlines/control chars/backticks) intact.
|
|
*
|
|
* @param {unknown} value
|
|
* @param {{maxLen?: number}} [opts]
|
|
* @returns {string}
|
|
*/
|
|
function flattenUntrusted(value, { maxLen = 300 } = {}) {
|
|
let s = String(value)
|
|
// eslint-disable-next-line no-control-regex -- deliberately stripping C0/C1 control chars (incl. CR/LF/ANSI escapes)
|
|
.replace(/[\x00-\x1f\x7f-\x9f]/g, ' ')
|
|
.replace(/`/g, "'")
|
|
.replace(/\s+/g, ' ')
|
|
.trim();
|
|
if (s.length > maxLen) s = `${s.slice(0, maxLen)}…`;
|
|
return s;
|
|
}
|
|
|
|
/**
|
|
* Build the markdown block appended to `GITHUB_STEP_SUMMARY`, when set —
|
|
* kept intentionally compact (a PR reviewer's first read, not a log dump).
|
|
*
|
|
* @param {{
|
|
* smells: ReturnType<typeof collectFindings>['smells'],
|
|
* violations: ReturnType<typeof collectFindings>['violations'],
|
|
* expectationFailures: ReturnType<typeof collectFindings>['expectationFailures'],
|
|
* newKeys: string[],
|
|
* staleEntries: Array<{key:string,id:string,scenario:string,source:string}>,
|
|
* smellSummary: Array<{id:string,count:number,examples:string[]}>,
|
|
* }} data
|
|
* @returns {string}
|
|
*/
|
|
function buildStepSummaryMarkdown({ smells, violations, expectationFailures, newKeys, staleEntries, smellSummary }) {
|
|
const lines = [];
|
|
lines.push('## QA smell ratchet');
|
|
lines.push('');
|
|
lines.push(
|
|
`**${smells.length} smells** (${newKeys.length} new, ${staleEntries.length} stale) · `
|
|
+ `**${violations.length} violations** · **${expectationFailures.length} expectation failures**`,
|
|
);
|
|
lines.push('');
|
|
|
|
if (expectationFailures.length) {
|
|
lines.push('### ❌ Scenario expectation failures');
|
|
lines.push('');
|
|
for (const f of expectationFailures) {
|
|
lines.push(`- \`${flattenUntrusted(f.scenario)}\` at **${flattenUntrusted(f.at)}** — ${flattenUntrusted(f.detail)}`);
|
|
}
|
|
lines.push('');
|
|
}
|
|
|
|
if (newKeys.length) {
|
|
lines.push('### 🚨 NEW (unacknowledged) smells');
|
|
lines.push('');
|
|
for (const key of newKeys) {
|
|
const f = smells.find((s) => s.key === key);
|
|
lines.push(`- \`${flattenUntrusted(f.id)}\` in **${flattenUntrusted(f.scenario)}** — ${flattenUntrusted(f.detail)}`);
|
|
}
|
|
lines.push('');
|
|
}
|
|
|
|
if (staleEntries.length) {
|
|
lines.push('### Stale baseline/fragment entries (no longer produced)');
|
|
lines.push('');
|
|
for (const e of staleEntries) {
|
|
lines.push(`- \`${flattenUntrusted(e.id)}\` in **${flattenUntrusted(e.scenario)}** (${flattenUntrusted(e.source)})`);
|
|
}
|
|
lines.push('');
|
|
}
|
|
|
|
if (smellSummary.length) {
|
|
lines.push('### Smells by oracle');
|
|
lines.push('');
|
|
lines.push('| oracle id | count |');
|
|
lines.push('|---|---|');
|
|
for (const entry of smellSummary) {
|
|
lines.push(`| \`${flattenUntrusted(entry.id)}\` | ${entry.count} |`);
|
|
}
|
|
lines.push('');
|
|
}
|
|
|
|
const firstFailingRepro = (violations[0] && violations[0].repro)
|
|
|| (expectationFailures[0] && expectationFailures[0].repro)
|
|
|| (newKeys.length && smells.find((s) => s.key === newKeys[0]).repro);
|
|
if (firstFailingRepro) {
|
|
lines.push('### Repro (first failing step)');
|
|
lines.push('');
|
|
lines.push('```sh');
|
|
// Backticks are replaced with `'` by flattenUntrusted, so the flattened
|
|
// value can never contain a ``` run that would close this fence early.
|
|
// maxLen: Infinity — a truncated repro looks runnable and isn't, which
|
|
// is worse than no repro at all (see flattenUntrusted's JSDoc).
|
|
lines.push(flattenUntrusted(firstFailingRepro, { maxLen: Infinity }));
|
|
lines.push('```');
|
|
lines.push('');
|
|
}
|
|
|
|
return lines.join('\n');
|
|
}
|
|
|
|
function main() {
|
|
const { update, jsonOut, keep } = parseArgs(process.argv.slice(2));
|
|
|
|
const scenarioReports = runAllScenarios({ keep });
|
|
const meta = {
|
|
nodeVersion: process.version,
|
|
platform: process.platform,
|
|
// Only ever used for report METADATA (and, when --json is passed, the
|
|
// written artifact's meta.generatedAt) — never fed into a fingerprint or
|
|
// into smell-baseline.json, which is what keeps this script's fingerprint
|
|
// and baseline output deterministic despite this one real clock read.
|
|
generatedAt: new Date().toISOString(),
|
|
};
|
|
const reportObject = buildReport(scenarioReports, meta);
|
|
|
|
if (jsonOut) {
|
|
fs.mkdirSync(path.dirname(jsonOut), { recursive: true });
|
|
fs.writeFileSync(jsonOut, `${JSON.stringify(reportObject, null, 2)}\n`, 'utf8');
|
|
}
|
|
|
|
const { smells, violations, expectationFailures } = collectFindings(reportObject);
|
|
const runKeys = new Set(smells.map((s) => s.key));
|
|
|
|
const baseline = readBaseline({ allowMissing: update });
|
|
const fragments = readAckFragments();
|
|
const sourceErrors = [...baseline.errors, ...fragments.errors];
|
|
|
|
if (update) {
|
|
if (sourceErrors.length) {
|
|
for (const e of sourceErrors) console.error(` - ${e}`);
|
|
throw new ExitError(
|
|
1,
|
|
`qa-smell-ratchet --update: ${sourceErrors.length} problem(s) in existing baseline/fragment source(s) `
|
|
+ '(printed above) — fix or delete the offending source(s) by hand before regenerating.',
|
|
);
|
|
}
|
|
|
|
const { byKey: knownBeforeUpdate } = mergeKnown(baseline.entries, fragments.entries);
|
|
const oldBaselineKeys = new Set(baseline.entries.map((e) => e.key));
|
|
|
|
// `--update` NEVER invents an issue number (#2966 FIX 3). A key already
|
|
// carrying a real `issue` (from the committed baseline or a fragment) keeps
|
|
// it, along with its `reason` if any. A genuinely NEW smell — no prior
|
|
// acknowledgment exists — gets `issue: null` and a TODO `reason`; the very
|
|
// next plain (non-`--update`) run REJECTS that entry, forcing a human to
|
|
// triage it as REAL (cite the issue) or FALSE POSITIVE (fix the oracle).
|
|
const newBaselineEntries = [...runKeys].sort().map((key) => {
|
|
const representative = smells.find((s) => s.key === key);
|
|
const carried = knownBeforeUpdate.get(key);
|
|
const hasKnownIssue = !!carried && Number.isInteger(carried.issue) && carried.issue > 0;
|
|
const entry = {
|
|
key,
|
|
id: representative.id,
|
|
scenario: representative.scenario,
|
|
issue: hasKnownIssue ? carried.issue : null,
|
|
};
|
|
if (hasKnownIssue && typeof carried.reason === 'string' && !isPlaceholderReason(carried.reason)) {
|
|
entry.reason = carried.reason;
|
|
} else if (!hasKnownIssue) {
|
|
entry.reason = PLACEHOLDER_REASON;
|
|
}
|
|
return entry;
|
|
});
|
|
const newBaselineKeys = new Set(newBaselineEntries.map((e) => e.key));
|
|
|
|
const added = [...newBaselineKeys].filter((k) => !oldBaselineKeys.has(k)).sort();
|
|
const removed = [...oldBaselineKeys].filter((k) => !newBaselineKeys.has(k)).sort();
|
|
|
|
fs.mkdirSync(path.dirname(BASELINE_PATH), { recursive: true });
|
|
fs.writeFileSync(
|
|
BASELINE_PATH,
|
|
`${JSON.stringify({ version: BASELINE_VERSION, smells: newBaselineEntries }, null, 2)}\n`,
|
|
'utf8',
|
|
);
|
|
|
|
console.log(
|
|
`qa-smell-ratchet --update: ${oldBaselineKeys.size} -> ${newBaselineKeys.size} baseline entries`
|
|
+ (added.length ? ` | added: ${added.length}` : '')
|
|
+ (removed.length ? ` | removed: ${removed.length}` : ''),
|
|
);
|
|
for (const key of added) {
|
|
const e = newBaselineEntries.find((x) => x.key === key);
|
|
const placeholderNote = e.issue === null ? ' [issue: null — TODO, needs triage before the next check run]' : '';
|
|
console.log(` + ${key}${placeholderNote}`);
|
|
}
|
|
for (const key of removed) console.log(` - ${key}`);
|
|
|
|
const redundant = fragments.entries.filter((e) => newBaselineKeys.has(e.key));
|
|
if (redundant.length) {
|
|
console.log(
|
|
`\n${redundant.length} fragment(s) are now redundant — their key is already in the regenerated baseline. `
|
|
+ 'Delete them (CONTRIBUTING.md fragment idiom: fold, then delete):',
|
|
);
|
|
for (const e of redundant) console.log(` - ${e._source}`);
|
|
}
|
|
|
|
if (process.env.GITHUB_STEP_SUMMARY) {
|
|
const md = buildStepSummaryMarkdown({
|
|
smells,
|
|
violations,
|
|
expectationFailures,
|
|
newKeys: added,
|
|
staleEntries: removed.map((key) => ({ key, id: '(pruned)', scenario: '(pruned)', source: BASELINE_REL_PATH })),
|
|
smellSummary: reportObject.smellSummary,
|
|
});
|
|
fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, `${md}\n`);
|
|
}
|
|
|
|
console.log(
|
|
`\nqa-smell-ratchet: ${smells.length} smells (${added.length} new, ${removed.length} stale), `
|
|
+ `${violations.length} violations, ${expectationFailures.length} expectation failures`,
|
|
);
|
|
|
|
if (violations.length || expectationFailures.length) {
|
|
if (violations.length) printViolations(violations);
|
|
if (expectationFailures.length) printExpectationFailures(expectationFailures);
|
|
throw new ExitError(
|
|
1,
|
|
'qa-smell-ratchet --update: baseline regenerated, but VIOLATIONS and/or SCENARIO EXPECTATION FAILURES remain '
|
|
+ '(neither is ever acknowledgeable — see above)',
|
|
);
|
|
}
|
|
return;
|
|
}
|
|
|
|
// ── check mode ──────────────────────────────────────────────────────────
|
|
const { byKey: known, redundantFragments } = mergeKnown(baseline.entries, fragments.entries);
|
|
const newKeys = [...runKeys].filter((k) => !known.has(k)).sort();
|
|
const staleKeys = [...known.keys()].filter((k) => !runKeys.has(k)).sort();
|
|
const staleEntries = staleKeys.map((key) => known.get(key));
|
|
|
|
if (sourceErrors.length) {
|
|
console.error(`qa-smell-ratchet: ${sourceErrors.length} problem(s) in baseline/fragment source(s):\n`);
|
|
for (const e of sourceErrors) console.error(` - ${e}`);
|
|
}
|
|
|
|
if (violations.length) {
|
|
printViolations(violations);
|
|
}
|
|
|
|
if (expectationFailures.length) {
|
|
printExpectationFailures(expectationFailures);
|
|
}
|
|
|
|
if (newKeys.length) {
|
|
console.error(`\nqa-smell-ratchet: ${newKeys.length} NEW (unacknowledged) smell(s):\n`);
|
|
for (const key of newKeys) {
|
|
const f = smells.find((s) => s.key === key);
|
|
console.error(`NEW smell: ${flattenUntrusted(f.key)}`);
|
|
console.error(` oracle: ${flattenUntrusted(f.id)}`);
|
|
console.error(` scenario: ${flattenUntrusted(f.scenario)}`);
|
|
console.error(` detail: ${flattenUntrusted(f.detail)}`);
|
|
console.error(' remedy: exactly two options — no third "accepted with an explanation" state:');
|
|
console.error(' 1. fix the detector if this is a FALSE POSITIVE (the oracle is wrong; make it stop firing);');
|
|
console.error(' 2. file a defect and add an entry citing its issue number (REAL) — a fragment:\n');
|
|
console.error(`${fragmentSkeleton(f).split('\n').map((l) => ` ${l}`).join('\n')}\n`);
|
|
}
|
|
}
|
|
|
|
if (staleKeys.length) {
|
|
console.error(`\nqa-smell-ratchet: ${staleKeys.length} STALE baseline/fragment entr${staleKeys.length === 1 ? 'y' : 'ies'} (no longer produced by the run):\n`);
|
|
for (const e of staleEntries) {
|
|
console.error(`STALE entry: ${flattenUntrusted(e.key)}`);
|
|
console.error(` source: ${flattenUntrusted(e.source)}`);
|
|
console.error(` oracle: ${flattenUntrusted(e.id)}`);
|
|
console.error(` scenario: ${flattenUntrusted(e.scenario)}`);
|
|
console.error(` issue: ${flattenUntrusted(e.issue)}`);
|
|
if (e.reason !== undefined) console.error(` reason: ${flattenUntrusted(e.reason)}`);
|
|
}
|
|
console.error('\n remedy: node scripts/qa-smell-ratchet.cjs --update');
|
|
}
|
|
|
|
if (redundantFragments.length) {
|
|
console.log(
|
|
`\n${redundantFragments.length} fragment(s) are already covered by the baseline and can be deleted:`,
|
|
);
|
|
for (const e of redundantFragments) console.log(` - ${e.source} (key ${e.key})`);
|
|
}
|
|
|
|
if (process.env.GITHUB_STEP_SUMMARY) {
|
|
const md = buildStepSummaryMarkdown({
|
|
smells,
|
|
violations,
|
|
expectationFailures,
|
|
newKeys,
|
|
staleEntries,
|
|
smellSummary: reportObject.smellSummary,
|
|
});
|
|
fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, `${md}\n`);
|
|
}
|
|
|
|
console.log(
|
|
`\nqa-smell-ratchet: ${smells.length} smells (${newKeys.length} new, ${staleKeys.length} stale), `
|
|
+ `${violations.length} violations, ${expectationFailures.length} expectation failures`,
|
|
);
|
|
|
|
if (sourceErrors.length || violations.length || expectationFailures.length || newKeys.length || staleKeys.length) {
|
|
throw new ExitError(1);
|
|
}
|
|
}
|
|
|
|
/**
|
|
* @param {ReturnType<typeof collectFindings>['violations']} violations
|
|
*/
|
|
function printViolations(violations) {
|
|
console.error(`qa-smell-ratchet: ${violations.length} VIOLATION(s) — never acknowledgeable, always fail:\n`);
|
|
for (const v of violations) {
|
|
console.error(`VIOLATION: ${flattenUntrusted(v.id)}`);
|
|
console.error(` scenario: ${flattenUntrusted(v.scenario)}`);
|
|
console.error(` argv: ${flattenUntrusted(v.argv.join(' '))}`);
|
|
console.error(` detail: ${flattenUntrusted(v.detail)}`);
|
|
// maxLen: Infinity — a truncated repro looks runnable and isn't.
|
|
console.error(` repro: ${flattenUntrusted(v.repro, { maxLen: Infinity })}`);
|
|
}
|
|
}
|
|
|
|
/**
|
|
* @param {ReturnType<typeof collectFindings>['expectationFailures']} expectationFailures
|
|
*/
|
|
function printExpectationFailures(expectationFailures) {
|
|
console.error(`qa-smell-ratchet: ${expectationFailures.length} SCENARIO EXPECTATION FAILURE(S) — never acknowledgeable, always fail:\n`);
|
|
for (const f of expectationFailures) {
|
|
console.error('EXPECTATION FAILURE:');
|
|
console.error(` scenario: ${flattenUntrusted(f.scenario)}`);
|
|
console.error(` at: ${flattenUntrusted(f.at)}`);
|
|
console.error(` argv: ${flattenUntrusted(f.argv.join(' '))}`);
|
|
console.error(` detail: ${flattenUntrusted(f.detail)}`);
|
|
// maxLen: Infinity — a truncated repro looks runnable and isn't.
|
|
console.error(` repro: ${flattenUntrusted(f.repro, { maxLen: Infinity })}`);
|
|
}
|
|
}
|
|
|
|
// `require()`ing this module (from `tests/loop-walk.qa.test.cjs`) must not
|
|
// trigger a real 20-scenario walk as a side effect — that's what made
|
|
// `collectFindings` untestable before #3597. Guard `runMain` so it only
|
|
// fires when this file is executed directly (`node scripts/qa-smell-ratchet.cjs`).
|
|
if (require.main === module) {
|
|
runMain(main);
|
|
}
|
|
|
|
module.exports = {
|
|
parseArgs,
|
|
readBaseline,
|
|
readAckFragments,
|
|
mergeKnown,
|
|
collectFindings,
|
|
fragmentSkeleton,
|
|
slugify,
|
|
isPlaceholderReason,
|
|
PLACEHOLDER_REASON_PREFIX,
|
|
BASELINE_REL_PATH,
|
|
ACKS_DIR_REL_PATH,
|
|
MAX_ACK_FRAGMENTS,
|
|
};
|