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 2 commits into
base: main
Choose a base branch
from
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
2 commits
Select commit Hold shift + click to select a range
bd55846
chore(deadcode): fix dead-code ratchet — remove unused devDeps + reba…
scottschreckengaust Jul 29, 2026
e825a75
chore(deadcode): move jira-forge-app findings to knip.json, baseline …
scottschreckengaust Jul 29, 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
1 change: 0 additions & 1 deletion cdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
"@types/jest": "^30.0.0",
"@types/js-yaml": "^4.0.9",
"@types/node": "^26",
"@types/pdf-parse": "^1.1.5",

@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.

Removing this is correct, and safe for a slightly different reason than the description gives. cdk/tsconfig.json sets "types": ["node"], which already blocked @types/pdf-parse from being auto-included, and the local declare module 'pdf-parse' in cdk/src/types/pdf-parse.d.ts shadows node_modules/@types on explicit import anyway. So this devDep was already inert and its removal is a genuine no-op. Good call either way, since a v1 type stub against an installed v2 is a trap.

The problem is the justification recorded in knip-baseline.json, which describes the removal as "superseded by pdf-parse@2's bundled types plus the local cdk/src/types/pdf-parse.d.ts ambient declaration." Those two are not complementary. The ambient declaration overrides v2's real bundled types, and it describes the v1 API:

function pdfParse(data: Buffer, options?: { max?: number }): Promise<PdfParseResult>;
export = pdfParse;

pdf-parse@2.4.5 exports no callable default. Verified against the installed package:

$ node -e "const m=require('pdf-parse'); console.log(Object.keys(m)); console.log(typeof m.default)"
[ 'AbortException', ..., 'PDFParse', 'VerbosityLevel', 'getException' ]
undefined

So in cdk/src/handlers/shared/attachment-screening.ts:258, pdfParseFn = (mod as any).default ?? mod resolves to the module namespace object, and the call at line 277 throws TypeError: pdfParseFn is not a function. It is caught and rewrapped, so every application/pdf attachment reaching line 201 fails as PDF "<name>" could not be processed. It may be corrupt or use unsupported features. PDF attachment screening is broken for all inputs, and the failure is indistinguishable from a genuinely corrupt PDF.

The { virtual: true } mock at cdk/test/handlers/shared/attachment-screening.test.ts:365 supplies { __esModule: true, default: jest.fn() }, so the suite asserts against a v1 shape that no longer exists and cannot catch this.

This predates the PR: pdf-parse@^2.4.5 and the ambient decl landed together in #434 (2026-06-30), and the as any plus the ambient override is what kept tsc quiet. I am not asking you to fix it here. Two smaller asks:

  1. Drop the "plus the local ambient declaration" clause from the baseline comment, so this PR does not turn a version-mismatched stub into documented policy.
  2. File a follow-up. The v2 shape is new PDFParse({ data: content }).getText() returning a TextResult, and fixing it means deleting the ambient decl, letting v2's own types apply, and replacing the mock with something that reflects the real module.

@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.

Thanks — reproduced the v2-shape mismatch independently (require('pdf-parse') exports PDFParse/VerbosityLevel/… and .default is undefined, so (mod as any).default ?? mod yields the namespace object and the call throws TypeError). Both asks done:

  1. Dropped the "plus the local ambient declaration" clause from knip-baseline.json so this PR no longer cites the version-mismatched stub as justification.
  2. Filed the follow-up: fix(cdk): PDF attachment screening broken — ambient pdf-parse decl describes v1 API against installed v2 #683 — captures the v1-decl-over-v2 root cause, the silent "could not be processed" symptom for every PDF, the v1-shaped virtual mock that hides it, and the fix (delete the ambient decl, adopt the v2 new PDFParse({ data }).getText() API, and add a real-PDF end-to-end test so a future mismatch fails loudly).

Fixed in e825a75.

"@types/ws": "^8.18.1",
"@typescript-eslint/eslint-plugin": "^8",
"@typescript-eslint/parser": "^8",
Expand Down
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). This is the knip issue count the ratchet (scripts/check-deadcode-ratchet.mjs) holds the build to: it fails 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. The remaining 85 are all pre-existing unused exports (44) and types (41), deferred to future dead-code PRs to ratchet down (tracking: #682)."
}
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.

},
"integrations/jira-forge-app": {
"entry": ["src/index.js!", "test/**/*.test.js"],
"project": ["src/**/*.js"]
}
}
}
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.

2 changes: 1 addition & 1 deletion scripts/check-deadcode-ratchet.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ 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,
// This is the complete set of countable keys for the installed knip (6.23.0,
// pinned exactly). There is no `nsExports`/`nsTypes`/`classMembers` in this
// schema — namespace/enum members surface as `namespaceMembers`/`enumMembers`.
// If knip is bumped, re-derive this list from its JSON (the countIssues guard
Expand Down
7 changes: 0 additions & 7 deletions yarn.lock
View file
Open in desktop

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

Loading