Skip to content

chore(deadcode): fix dead-code ratchet — remove unused devDeps + rebaseline knip (#607) #673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
scottschreckengaust wants to merge 3 commits into
base: main
Choose a base branch
from
+14 −7
Open

chore(deadcode): fix dead-code ratchet — remove unused devDeps + rebaseline knip (#607) #673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b317776
chore(deadcode): fix dead-code ratchet — remove unused devDeps + reba…
scottschreckengaust Jul 29, 2026
b42f0b9
chore(deadcode): scope jira-forge-app knip workspace (baseline 88->85…
scottschreckengaust Jul 29, 2026
d0eb13d
docs(deadcode): correct the COUNTED_KEYS schema note per review (#607)
Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions knip-baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"count": 78,
"comment": "Dead-code ratchet baseline for issue #282 (cairn MVG gate #6). This is the knip issue count at the time the gate was introduced — pre-existing unused exports/types that are out of scope to remove in the gate PR. The ratchet (scripts/check-deadcode-ratchet.mjs) fails the build only if the count rises above this number. When dead code is removed and the count drops, lower this value in the same PR to lock in the gain. Per-category false positives belong in knip.json, not here."
"count": 85,
"comment": "Dead-code ratchet baseline for issue #282 (cairn MVG gate #6). The ratchet (scripts/check-deadcode-ratchet.mjs) reads only `count` and fails the build only if the current knip issue count rises above it; when dead code is removed and the count drops, lower this value in the same PR to lock in the gain. Suppress false positives in knip.json — via ignore/ignoreDependencies, or by declaring the workspace so its entry points are reachable — never absorb them into this baseline. The remaining 85 are pre-existing unused exports and types, tracked in #682."

@isadeks isadeks Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both durability nits confirmed FIXED. Reading the prose as shipped at 950a9ce9:

  1. The (44)/(41) split is gone — it now reads "pre-existing unused exports and types, tracked in chore(deadcode): ratchet down the 85 pre-existing unused exports/types (follow-up to #607) #682", with count as the only number. That was the point: check-deadcode-ratchet.mjs:117 reads .count and nothing else, so any breakdown here is decoration that rots the moment chore(deadcode): ratchet down the 85 pre-existing unused exports/types (follow-up to #607) #682 removes one export. It also now agrees with chore(deadcode): ratchet down the 85 pre-existing unused exports/types (follow-up to #607) #682, whose title independently tracks 85.
  2. The mechanism wording is fixed and is now actually actionable: "via ignore/ignoreDependencies, or by declaring the workspace so its entry points are reachable" — which is what landed, and is the case the next person will hit.

The "never absorb them into this baseline" phrasing is a genuine improvement on the original — it states the rule as a prohibition rather than a preference, which is the right register for the one file whose job is to stay durable.

I also re-confirmed @theagenticguy's second blocking ask stays fixed here: no "plus the local ambient declaration" clause, no "fixture files" mischaracterisation of live Forge source, and the underlying v1-decl-over-v2 runtime bug is filed as #683 (bug/infra-cdk/security) rather than being documented as policy. Right disposition — that bug predates this PR and fixing it here would have been scope creep.

And the number itself: I re-derived 85 from scratch against the pinned knip 6.23.0 at this head rather than trusting my prior read, since the head moved. It reproduces, and CI at head_sha 950a9ce9… prints ✅ Dead-code count holding at baseline (85).

}
6 changes: 5 additions & 1 deletion knip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@
"docs": {
"entry": ["astro.config.mjs", "src/content.config.ts", "src/components/**/*.astro"],
"project": ["src/**/*.{astro,ts,tsx}"],
"ignoreDependencies": ["@astrojs/check", "remark-gfm", "@pagefind/default-ui", "jest"]
"ignoreDependencies": ["@astrojs/check", "markdown-link-check", "remark-gfm", "@pagefind/default-ui", "jest"]

@theagenticguy theagenticguy Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The markdown-link-check addition here is correct and well-justified. The gap is what is not here.

knip-baseline.json calls the 3 remaining files findings "the jira-forge-app fixture files," but they are live application source:

  • integrations/jira-forge-app/manifest.yml:10 declares handler: index.handler, so src/index.js is the Forge function entry point.
  • src/index.js:22 imports ./proxy.js.
  • test/proxy.test.js runs under mise //:test:jira-forge-app, which is a dependency of mise run build.

knip flags all three only because integrations/ is not in the root workspaces array (cdk, cli, docs) and is not in knip's ignore, so nothing there is reachable from a configured entry point. That is the textbook definition of the per-category false positive this file's own guidance says to suppress here rather than absorb into the baseline.

I verified the fix. Adding "integrations/**" to the top-level ignore:

"ignore": [
  ".semgrep/**",
  "scripts/**",
  "integrations/**"
]

drops the count from 88 to 85, with files going to 0 and only the pre-existing exports: 44 / types: 41 remaining. A scoped knip workspace entry for the Forge app would work too and is arguably better, since it would keep real dead code there detectable instead of blanket-ignoring the directory.

Worth doing rather than deferring: the baseline is the durable artifact every future dead-code PR ratchets against, so 3 phantom findings embedded now are 3 that can never be cleaned up, and they push the "driven to zero" flip-to-blocking milestone permanently out of reach.

@scottschreckengaust scottschreckengaust Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — but as a scoped knip workspace entry rather than a blanket integrations/** ignore (the option you flagged as arguably better). knip.json now declares integrations/jira-forge-app with entry: ["src/index.js!", "test/**/*.test.js"] and project: ["src/**/*.js"], so the Forge function entry point and its tests are reachable and any real dead code there stays detectable instead of being blanket-ignored.

Verified locally: files findings → 0, count drops 88 → 85 (exports: 44 / types: 41 remaining), and node scripts/check-deadcode-ratchet.mjs → ✅ Dead-code count holding at baseline (85). exit 0. Fixed in e825a75.

@scottschreckengaust scottschreckengaust Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theagenticguy Landed as the scoped workspace you flagged as arguably better (not a blanket integrations/** ignore): knip.json now declares integrations/jira-forge-app with entry: ["src/index.js!", "test/**/*.test.js"] and project: ["src/**/*.js!", "test/**/*.js"], so the Forge handler + its tests are reachable and real dead code there stays detectable. files findings → 0, count drops 88 → 85, ratchet exit 0. Commit 950a9ce. 🤖 @scottschreckengaust (agent:w3)

@scottschreckengaust scottschreckengaust Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theagenticguy Landed as the scoped workspace you flagged as arguably better (not a blanket integrations/** ignore): knip.json now declares integrations/jira-forge-app with entry: ["src/index.js!", "test/**/*.test.js"] and project: ["src/**/*.js!", "test/**/*.js"], so the Forge handler + its tests are reachable and real dead code there stays detectable. files findings → 0, count drops 88 → 85, ratchet exit 0. Commit 950a9ce. 🤖 @scottschreckengaust (agent:w3)

},
"integrations/jira-forge-app": {
"entry": ["src/index.js!", "test/**/*.test.js"],

@isadeks isadeks Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right fix, and better than the blanket "integrations/**" ignore that was originally suggested — a scoped workspace keeps real dead code here detectable. I mutation-tested it against the pinned knip 6.23.0 to confirm it suppresses the 3 false positives without going blind:

mutation detected?
dead src/orphan.js ✅ files (0→1)
dead export unusedHelper in src/proxy.js ✅ exports (0→1)
rename entry point src/index.js → src/main.js ✅ 0→2, ratchet fails loud

That third row is the one that matters most: a hardcoded entry glob usually rots into a silent no-op when the file it names moves. Here it surfaces as a count increase instead, so the config can't quietly stop working.

Two nits, neither blocking:

  1. project on line 27 omits the ! production marker that cdk (line 13) and cli (line 18) both use. I verified "src/**/*.js!" yields the same count (0), so it's consistency only.
  2. Narrow blind spot: because project is src/**/*.js, a dead non-test helper under test/ is invisible — I confirmed adding test/helper.js leaves the count at 0. ["src/**/*.js", "test/**/*.js"] would close it. Low stakes with one test file today.

For the record, I also chased down (and refuted) a concern that this entry makes the count install-dependent, since this directory is npm-managed and absent from root workspaces. Findings are 0 both with and without integrations/jira-forge-app/node_modules: the sole external import (@forge/api) is in src/index.js, which is an entry and therefore never reported unused whether or not the import resolves. mise run install doesn't install this directory at all, so the uninstalled state is the normal one — and it counts 0, matching CI.

@scottschreckengaust scottschreckengaust Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isadeks Both nits applied in 950a9ce. (1) project now carries the ! production marker for consistency with cdk/cli: ["src/**/*.js!", "test/**/*.js"]. (2) Added test/**/*.js to project so a dead non-test helper under test/ is no longer invisible — closes the blind spot you flagged. Count still 85 (verified: your mutation matrix — dead file, dead export, moved entry point all still surface; ratchet fails loud on the moved-entry case). Thanks for the mutation-testing and the install-independence check. 🤖 @scottschreckengaust (agent:w3)

"project": ["src/**/*.js!", "test/**/*.js"]

@isadeks isadeks Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking) — this is inert as shipped, and my prior nit that asked for it was wrong. Credit to you for applying it in good faith.

I asked for test/**/*.js here on the theory that project: ["src/**/*.js"] made a dead non-test helper under test/ invisible. I re-tested at this head and the addition changes nothing, because my diagnosis of the mechanism was incorrect.

The real cause is knip's Node.js plugin. Because integrations/jira-forge-app/package.json declares "test": "node --test", the plugin auto-registers these as entry patterns:

**/*{.,-,_}test.{cjs,mjs,js,…}
**/test-*.{cjs,mjs,js,…}
**/test.{cjs,mjs,js,…}
**/test/**/*.{cjs,mjs,js,…}

That last glob makes everything under test/ an entry point, and entry files are never reported unused — regardless of project scope. The 2×2 I ran against pinned knip 6.23.0, dropping a dead test/helper.js in each cell:

node --test script test/**/*.js in project dead test/helper.js detected
present (as shipped) yes (this head) NO — count 0
present no (previous head) NO — count 0
absent yes ✅ YES — files 0→1
absent no NO — count 0

So the glob only bites if the node --test script goes away. No action needed on the code — it is harmless (I confirmed it cannot raise the count: an unused export added to test/proxy.test.js still reports 0, since test files are entries), and it becomes correct if that script ever changes.

The only thing worth fixing is the rationale in the commit message and PR description — "test/**/*.js is in project scope so dead test helpers stay visible" is not true today, and a future maintainer reading it will believe this directory has test-helper coverage that it does not have. If you want the blind spot genuinely closed, the lever is entry/plugin config, not project. Entirely optional, and I would not reopen the thread for it.

For the record, the rest of this entry re-verified clean at 950a9ce9: findings 3→0, the ! marker is confirmed count-neutral (consistency only, as I said), and the mutation matrix still fails loud on a dead file, a dead export, and a moved entry point.

}
}
}
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 7 additions & 3 deletions scripts/check-deadcode-ratchet.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,13 @@ const baselinePath = join(repoRoot, 'knip-baseline.json');
// array-of-arrays (one inner array per duplicate group), so its `.length`
// counts groups, which is the unit we ratchet on.
//
// This is the complete set of countable keys for the installed knip (6.20.0,
// pinned exactly). There is no `nsExports`/`nsTypes`/`classMembers` in this
// schema — namespace/enum members surface as `namespaceMembers`/`enumMembers`.
// This is the complete set of countable keys for the installed knip (6.23.0,

@isadeks isadeks Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: root package.json:20 declares "knip": "6.23.0" with no range specifier, so both the version and the "pinned exactly" claim are now accurate.

Worth calling out what this comment protects, since it reads as boilerplate but isn't. COUNTED_KEYS is derived from a specific knip JSON schema, and the countIssues guard below fails closed (exit 2) if report.issues stops being an array — rather than letting ?? [] yield count 0, sail under any baseline, and report a permanently green gate. Combined with the exact pin, a schema change has to arrive as a deliberate reviewed bump, and the note to re-derive this list on a bump is the correct instruction to leave behind.

I could not verify offline that 6.23.0 still surfaces namespace/enum members as namespaceMembers/enumMembers, but that's fine here: the fail-closed guard means a wrong assumption surfaces as an error rather than a silently-disabled ratchet, which is the property that actually matters.

@scottschreckengaust scottschreckengaust Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isadeks Corrected the stale pin in 950a9ce: the docstring now reads "installed knip (6.23.0, pinned exactly)" to match root package.json:21 ("knip": "6.23.0", no range). Agreed on what the comment protects — COUNTED_KEYS + the fail-closed countIssues guard (exit 2 if report.issues stops being an array) mean a schema change surfaces as a reviewed bump rather than a silently-green gate; left the note to re-derive the list on a bump in place. 🤖 @scottschreckengaust (agent:w3)

@isadeks isadeks Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking, pre-existing — but inside the block you just edited). The version bump to 6.23.0 is correct and I verified it: root package.json:21 pins "knip": "6.23.0" with no range, and yarn.lock:5873-5876 resolves exactly that. The "pinned exactly" claim holds.

The next sentence (line 53-54) does not, and it is now stated about 6.23.0 rather than 6.20.0:

There is no nsExports/nsTypes/classMembers in this schema

classMembers is indeed absent, but nsExports and nsTypes both exist in the installed 6.23.0:

  • dist/types/issues.d.ts:37-38 declares nsExports: IssueRecords; nsTypes: IssueRecords;
  • dist/constants.js lists both in ISSUE_TYPES with titles "Exports in used namespace" / "Exported types in used namespace"
  • dist/reporters/json.js emits nsExports/nsTypes arrays in each row when they are enabled

They are not missing from the schema — they are default-excluded: dist/util/get-included-issue-types.js:3 is defaultExcludedIssueTypes = ['nsExports', 'nsTypes'].

Why this is only a nit: the ratchet invokes knip with no --include, so those two keys are never emitted today and the count of 85 is unaffected. The gate is correct as it stands.

Why it is still worth a line: this comment's explicit job is to be re-derived on a knip bump, and it currently tells that reader those keys cannot exist. If anyone later enables them (--include nsExports, or a future default flip), the reporter would emit findings that COUNTED_KEYS silently drops — an under-count in the one direction this gate is meant to catch. Suggested rewording:

// `nsExports`/`nsTypes` also exist in this schema but are default-excluded by
// knip, so they never appear in our JSON; add them here if that ever changes.
// There is no `classMembers` — namespace/enum members surface as
// `namespaceMembers`/`enumMembers`.

Separately, on the earlier thread: I can now confirm offline what I previously could not, from the installed 6.23.0 — namespaceMembers and enumMembers are the correct current key names. And the countIssues fail-closed guard (exit 2 when report.issues is not an array) still holds at this head, which is what keeps a schema change from presenting as a silently-green gate.

// pinned exactly). `nsExports`/`nsTypes` also exist in this schema but are
// default-excluded by knip, so they never appear in our JSON; add them here if
// that ever changes — otherwise the reporter would emit findings COUNTED_KEYS
// silently drops, an under-count in the one direction this gate exists to
// catch. There is no `classMembers` — namespace/enum members surface as
// `namespaceMembers`/`enumMembers`.
// If knip is bumped, re-derive this list from its JSON (the countIssues guard
// below will fail loud if the top-level shape changes).
const COUNTED_KEYS = [
Expand Down
Loading