housekeeping: typecheck scope, dead code, drifted duplicates, and five tests that cannot fail #122

New issue

Closed

opened 2026-08-13 16:06:30 +00:00 by john · 1 comment

john commented

2026-08-13 16:06:30 +00:00

Owner

Low-severity residue from the v3.2.0 audit. None of these is a live defect; each is drift, dead weight, or a test that cannot fail. Grouped because they are all small and none earns its own issue.

Typecheck scope

The entire test suite is excluded from typechecking. tsconfig.ci.json:15 excludes cadence-core/bin/**/*.test.mjs - roughly 13,000 lines including the 5,051-line planning.test.mjs - while every non-test file is checked. Tests are exactly where a stale-shape assumption survives, because a test that only reads the properties that still exist keeps passing after the shape changes underneath it. self-verify cannot see this class: it lints prose against code, not test shapes. Include them, or write in the file why not.

@ts-check pragmas are decorative. 40 of 41 non-test files carry // @ts-check, and tsconfig.ci.json sets checkJs: true, which checks every included file regardless. review-provider.mjs is the one file without the pragma and is checked anyway. Harmless, except the tsconfig comment describes the pragmas as load-bearing ("the pragmas exist to catch real drift"). Either drop checkJs and let them mean something, or drop them.

strictNullChecks is worth pricing on its own. strict: false is defensible for zero-dep scripts and the tsconfig says why. But two confirmed findings in this milestone (#117's unreadable-CONTEXT exemption, and the plan-size item below) are null-versus-absent confusions, which is the class strictNullChecks alone catches. Price it separately from full strict.

Dead and unreachable

lib/config-merge.mjs:33 readJSON has zero callers. Repo-wide, including its own file and every test. readLayer superseded it. Delete.

lib/milestone-prune.mjs:60 has an unreachable clause.

while (j < lines.length && !/^###? /.test(lines[j]) && !/^## /.test(lines[j])) {

/^###? / already matches ## and ### , so /^## / is a strict subset and can never be the test that stops the loop.

Worth a second look while in there: the loop also does not stop at # or #### , so a phase detail section followed by an h1 appendix is deleted to EOF. The shipped ROADMAP template never produces that shape, which is why this is not filed as a bug.

Drifted duplicates

lib/planning-files.mjs:1164 uses in where the sibling uses hasOwnProperty.

for (const it of items) if (it.status in counts) counts[String(it.status)]++;

in walks the prototype chain, so status: "constructor" or "toString" increments a phantom counter. lib/trace.mjs:444 does the identical job with Object.prototype.hasOwnProperty.call(...) and states why. Same idiom, two implementations, one hardened. Observable effect today is nil; collapse the drift.

self-verify.mjs:1211 reads --root with an inline indexOf.

$ self-verify.mjs --root ""   -> {"ok":true,...}      # linted the cwd, not the named tree
$ self-verify.mjs --root      -> {"ok":false,"reason":"internal",...}

weight.mjs:46-53 has a flagValue() written specifically to close this, and its docblock calls it "the quiet-wrong-number class... worse here than a hard error because the envelope looks correct". weight.test.mjs pins three cases. self-verify.mjs got none of it. --root "$TREE" with $TREE empty silently lints whatever the job is cd'd into. Import flagValue.

Envelope shape

planning.mjs:1456 plan-size reports within: true when ROADMAP.md is absent or unreadable. read(...) || '' makes phaseRequirements('') return found:false, the phase-too-big comparison is skipped, and the envelope carries within: true beside requirements_found: false.

The seam is honest and says so, and workflows/plan.md:68 does read requirements_found, so the wiring holds today. The risk is the field name: within is the boolean-shaped key, and a future caller reaching for it gets a pass from a file that does not exist. Omit within, or emit within: null, when requirements_found is false.

Tests that cannot fail

design-brief.test.mjs - five tests, no product code. The file imports node:test, node:assert, node:fs, node:path, node:url and no Cadence module. Every assertion is over fixtures/verbatim.design-brief.md, which its own header describes as copied byte-for-byte and immutable: the file has a ## 17. Open items heading, the table has five rows, the rows name five substrings, no status cell contains OPEN, the file contains exactly two OPEN occurrences.

These cannot fail on a Cadence regression, only on a hand-edit of a file nobody edits. They are a checksum on committed evidence wearing test clothing, and the header concedes the actual suppression rule is untested. Either drive the real --brief suppression path or delete them; a green test that cannot go red is worse than no test, because it reports coverage that does not exist.

Not in scope

The rest of the suite is unusually strong and should not be swept along with this. bm25.test.mjs:28 hand-computes a score rather than snapshotting the implementation; retired-keys.test.mjs uses one test() per row with a written rationale; review-provider.test.mjs drives six real failure modes through an in-process transport seam; include-consumers.test.mjs pins an empty register and drives both bounds from a synthetic row. This issue is about one file, not a testing-practices sweep.

Low-severity residue from the v3.2.0 audit. None of these is a live defect; each is drift, dead weight, or a test that cannot fail. Grouped because they are all small and none earns its own issue. ## Typecheck scope **The entire test suite is excluded from typechecking.** `tsconfig.ci.json:15` excludes `cadence-core/bin/**/*.test.mjs` - roughly 13,000 lines including the 5,051-line `planning.test.mjs` - while every non-test file is checked. Tests are exactly where a stale-shape assumption survives, because a test that only reads the properties that still exist keeps passing after the shape changes underneath it. `self-verify` cannot see this class: it lints prose against code, not test shapes. Include them, or write in the file why not. **`@ts-check` pragmas are decorative.** 40 of 41 non-test files carry `// @ts-check`, and `tsconfig.ci.json` sets `checkJs: true`, which checks every included file regardless. `review-provider.mjs` is the one file without the pragma and is checked anyway. Harmless, except the tsconfig comment describes the pragmas as load-bearing ("the pragmas exist to catch real drift"). Either drop `checkJs` and let them mean something, or drop them. **`strictNullChecks` is worth pricing on its own.** `strict: false` is defensible for zero-dep scripts and the tsconfig says why. But two confirmed findings in this milestone (#117's unreadable-CONTEXT exemption, and the `plan-size` item below) are null-versus-absent confusions, which is the class `strictNullChecks` alone catches. Price it separately from full strict. ## Dead and unreachable **`lib/config-merge.mjs:33` `readJSON` has zero callers.** Repo-wide, including its own file and every test. `readLayer` superseded it. Delete. **`lib/milestone-prune.mjs:60` has an unreachable clause.** ```js while (j < lines.length && !/^###? /.test(lines[j]) && !/^## /.test(lines[j])) { ``` `/^###? /` already matches `## ` and `### `, so `/^## /` is a strict subset and can never be the test that stops the loop. Worth a second look while in there: the loop also does not stop at `# ` or `#### `, so a phase detail section followed by an h1 appendix is deleted to EOF. The shipped ROADMAP template never produces that shape, which is why this is not filed as a bug. ## Drifted duplicates **`lib/planning-files.mjs:1164` uses `in` where the sibling uses `hasOwnProperty`.** ```js for (const it of items) if (it.status in counts) counts[String(it.status)]++; ``` `in` walks the prototype chain, so `status: "constructor"` or `"toString"` increments a phantom counter. `lib/trace.mjs:444` does the identical job with `Object.prototype.hasOwnProperty.call(...)` and states why. Same idiom, two implementations, one hardened. Observable effect today is nil; collapse the drift. **`self-verify.mjs:1211` reads `--root` with an inline `indexOf`.** ``` $ self-verify.mjs --root "" -> {"ok":true,...} # linted the cwd, not the named tree $ self-verify.mjs --root -> {"ok":false,"reason":"internal",...} ``` `weight.mjs:46-53` has a `flagValue()` written specifically to close this, and its docblock calls it "the quiet-wrong-number class... worse here than a hard error because the envelope looks correct". `weight.test.mjs` pins three cases. `self-verify.mjs` got none of it. `--root "$TREE"` with `$TREE` empty silently lints whatever the job is cd'd into. Import `flagValue`. ## Envelope shape **`planning.mjs:1456` `plan-size` reports `within: true` when ROADMAP.md is absent or unreadable.** `read(...) || ''` makes `phaseRequirements('')` return `found:false`, the `phase-too-big` comparison is skipped, and the envelope carries `within: true` beside `requirements_found: false`. The seam is honest and says so, and `workflows/plan.md:68` does read `requirements_found`, so the wiring holds today. The risk is the field name: `within` is the boolean-shaped key, and a future caller reaching for it gets a pass from a file that does not exist. Omit `within`, or emit `within: null`, when `requirements_found` is false. ## Tests that cannot fail **`design-brief.test.mjs` - five tests, no product code.** The file imports `node:test`, `node:assert`, `node:fs`, `node:path`, `node:url` and no Cadence module. Every assertion is over `fixtures/verbatim.design-brief.md`, which its own header describes as copied byte-for-byte and immutable: the file has a `## 17. Open items` heading, the table has five rows, the rows name five substrings, no status cell contains `OPEN`, the file contains exactly two `OPEN` occurrences. These cannot fail on a Cadence regression, only on a hand-edit of a file nobody edits. They are a checksum on committed evidence wearing test clothing, and the header concedes the actual suppression rule is untested. Either drive the real `--brief` suppression path or delete them; a green test that cannot go red is worse than no test, because it reports coverage that does not exist. ## Not in scope The rest of the suite is unusually strong and should not be swept along with this. `bm25.test.mjs:28` hand-computes a score rather than snapshotting the implementation; `retired-keys.test.mjs` uses one `test()` per row with a written rationale; `review-provider.test.mjs` drives six real failure modes through an in-process transport seam; `include-consumers.test.mjs` pins an empty register and drives both bounds from a synthetic row. This issue is about one file, not a testing-practices sweep.

john added this to the v3.2.0 milestone

2026-08-13 16:06:30 +00:00

john added the

bug

label

2026-08-13 16:06:30 +00:00

john closed this issue

2026-08-14 17:01:44 +00:00

john commented

2026-08-14 17:02:02 +00:00

Author

Owner

Fixed in v3.2.0 (PR #124). HYG-01, phase 4. tsconfig.ci.json states why the test suite is excluded and that checkJs, not the pragma, is what checks a file. readJSON and the unreachable /^## / clause are deleted, planning-files.mjs uses the hasOwnProperty idiom, self-verify reads --root through weight.mjs's flagValue rule with its seam catch arm, plan-size carries an explicit no-comparison-ran signal, and design-brief.test.mjs is gone with its fixture kept.

Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.