chore(deadcode): fix dead-code ratchet — remove unused devDeps + rebaseline knip (#607) - #673
Conversation
✅ Acceptance summary (for the reviewer)#607 —
🔀 Merge guidance (for the reviewer)Independent — no ordering constraint. Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Reviewed by cloning the repo, checking out the head, and reproducing every claim rather than trusting the description. The core of this PR is right, and the description is unusually honest about its own numbers. I verified the arithmetic independently and it holds exactly.
Confirmed:
kniponorigin/main= 91, on this branch = 88. Each of the three edits removes exactly one finding, so91 - 3 = 88is measured, not padded to make the gate pass. The CI job log agrees:Dead-code count holding at baseline (88).- The drift is genuine code drift, not a tooling artifact. I ran
mainunder both knip6.20.0and the currently pinned6.23.0and got 91 under each, so the+13is real and the version bump in #522 did not cause it. @types/pdf-parseremoval is safe. That package tops out at v1.1.5 (the v1 API) against an installed[email protected], which ships its owndist/pdf-parse/cjs/index.d.cts.@astrojs/checkroot removal is safe, and the hoisting worry dissolves on inspection:docs/node_modules/.bin/astro-checkis already a symlink up into rootnode_modules, anddocs/package.jsonstill declares the identical^0.9.9range, so yarn v1 keeps hoisting it and the retained lock entry is legitimate.markdown-link-checkis a true false positive.docs/scripts/link-check.shinvokes./node_modules/.bin/markdown-link-check, wired through//docs:link-checkintodrift-preventionintomise run build. TheignoreDependenciesentry sits in the correct workspace scope, since the dep is declared indocs/package.json.- The
yarn.lockedit is minimal and correct. Nothing else requested@types/pdf-parse.
Re-greening an advisory gate that was already red on main is the right call, and rebaselining to the measured value instead of an inflated one is the right instinct. Two things should change first.
1. The baseline absorbs 3 findings that your own rule says belong in knip.json (should-fix). The three files findings under integrations/jira-forge-app/ are live Forge app source, not fixtures. Putting "integrations/**" in knip's ignore drops the count to 85, which I verified by running it. 85 is the honest number, and 88 bakes 3 phantom findings into durable state that every future ratchet PR inherits.
2. The new comment blesses an ambient type declaration that misdescribes the runtime, and there is a live bug behind it (should-fix, pre-existing). Details inline on cdk/package.json. Not introduced by this PR, so I am not asking you to fix it here, only to stop citing it as justification and to file it.
Nits, both pre-existing and neither blocking: scripts/check-deadcode-ratchet.mjs:52 still documents "the installed knip (6.20.0, pinned exactly)" while the pin is now 6.23.0, worth correcting while you are in this file. And the roughly 7 genuine unused export/type regressions inside the +13 merged unnoticed precisely because this job is continue-on-error: true. They deserve a tracking issue, otherwise the "flips to blocking once the baseline is driven to zero" milestone recedes with every merge.
Requesting changes narrowly, on the one-line knip.json change plus the comment corrections. Everything else here is sound and I would happily approve once the baseline reads 85.
…88→85 (#607) Addresses review on #673: - Add a scoped knip workspace entry for integrations/jira-forge-app (entry: src/index.js from manifest handler + test/**/*.test.js) instead of absorbing its 3 live-source `files` findings into the baseline. Keeps real dead code in the Forge app detectable rather than blanket-ignoring the directory. Drops the count from 88 to the honest 85. - Rebaseline knip-baseline.json count 88→85 and trim the comment to the durable rule; the remaining 85 are pre-existing exports (44) + types (41), tracked in #682. Drops the inaccurate "fixture files" wording and the "plus the local ambient declaration" clause (the pdf-parse v2 runtime bug behind it is filed as #683). - Correct the stale "knip (6.20.0)" pin comment to 6.23.0 in the ratchet. Ratchet: `node scripts/check-deadcode-ratchet.mjs` → holding at baseline (85), exit 0.
|
@theagenticguy — all review feedback addressed in e825a75. Thanks for the thorough, reproduce-everything review. Blocking (both resolved):
Nits + follow-ups:
Baseline now reads 85. Re-requesting review once checks are green. |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict — Approve (with nits)
Approve. Both of @theagenticguy's blocking asks are genuinely fixed in e825a756, and I re-derived the numbers myself rather than taking either the description or the prior review on trust. Zero blocking findings. The nits below are cosmetic/durability items and none should hold the merge.
Prior review — resolution status
I independently re-verified every blocking claim against the head commit. Both are fixed; I am not carrying either forward.
1. "Baseline absorbs 3 findings that belong in knip.json" → FIXED, and fixed the better way. The author took the scoped-workspace path (knip.json:25-28) rather than the blanket "integrations/**" ignore that was originally suggested. I reproduced the whole arithmetic in an isolated harness against the pinned knip 6.23.0, scoped to just this directory:
| config | integrations/jira-forge-app findings |
|---|---|
main's knip.json (no workspace entry) |
3 — src/index.js, src/proxy.js, test/proxy.test.js, all as files |
| this PR's knip.json | 0 |
That is exactly the delta the description claims, so 88 − 3 = 85 is measured, not asserted. The CI log at this exact head SHA agrees: ✅ Dead-code count holding at baseline (85). (run 30421544634).
More importantly, I mutation-tested the new entry to confirm it suppresses the false positives without going blind to real dead code — the failure mode a blanket ignore would have introduced:
| mutation | detected? |
|---|---|
add genuinely-dead src/orphan.js |
✅ files: [orphan.js] (count 0→1) |
add dead export unusedHelper to src/proxy.js |
✅ exports: [unusedHelper] (count 0→1) |
rename the manifest entry point src/index.js → src/main.js |
✅ count 0→2 — entry-point drift makes the ratchet fail loud rather than silently pass |
That last row is the one I most wanted to see, since a hardcoded entry glob is exactly the kind of config that usually rots into a silent no-op. It doesn't here.
2. "Baseline comment blesses the ambient pdf-parse decl" → FIXED. The "plus the local ambient declaration" clause is gone from knip-baseline.json:3, and the underlying runtime bug is filed as #683 with the root cause, the v1-decl-over-v2 mechanism, the silent "could not be processed" symptom, and the v1-shaped virtual mock that hides it. That was the right disposition — the bug predates this PR and fixing it here would have been scope creep.
Nits from the prior review → both FIXED. The stale knip (6.20.0) pin comment now reads 6.23.0 (scripts/check-deadcode-ratchet.mjs:52), and the ~7 genuine export/type regressions that slipped in under continue-on-error: true are tracked in #682, including the ask to drop continue-on-error once the count reaches zero. Filing that was the substantive part — it stops the flip-to-blocking milestone from receding silently on every merge.
I also re-checked the parts of the prior review I could have disagreed with, and they hold: @types/pdf-parse had exactly one requester in yarn.lock and the 7-line removal is minimal and complete.
Two agent-raised blockers I ran down and am NOT raising — they are wrong on the merits
Recording these with evidence, because both are superficially convincing and would waste a future reviewer's time.
Claim: "the count becomes environment-dependent, so the ratchet is non-deterministic." The reasoning was that integrations/jira-forge-app is npm-managed (own package-lock.json) and is not in root package.json workspaces ([cdk, cli, docs]), so knip would only analyze it when integrations/jira-forge-app/node_modules exists — meaning a fresh clone and an installed tree would disagree, and a contributor could merge a locally-green count that breaks in CI.
Plausible, and false. I tested both states directly:
- without
integrations/jira-forge-app/node_modules(fresh-clone state): 0 findings - with it present (
@forge/apistubbed at the declared 8.0.2): 0 findings
Identical. The reason is that the only external import in the tree is @forge/api in src/index.js, and that file is declared an entry — so it is never reported as unused regardless of whether its import resolves. Nothing in this workspace's contribution to the count depends on install state. Worth noting the repo's mise run install doesn't install this directory at all, so "absent node_modules" is in fact the normal state, and it counts 0 — which is what CI observed.
Claim: "removing root @astrojs/check breaks docs typechecking / hoisting." Also false, and I checked it against real installed worktrees rather than reasoning about yarn's behavior. docs/package.json:21 still declares the identical ^0.9.9 range as a dependency, yarn.lock:5 retains the @astrojs/check@^0.9.9 descriptor with exactly one requester, and in two independently-installed worktrees docs/node_modules/.bin/astro-check is already a symlink up to ../../../node_modules/@astrojs/check/bin/astro-check.js. Both consumers — docs/mise.toml [tasks.check] and the docs-astro-check pre-commit hook — invoke ./node_modules/.bin/astro check from docs/, so they resolve through that link and are untouched by the root removal. The root devDep was pure duplication.
Vision alignment
Fits cleanly. This serves bounded blast radius and reviewable outcomes (VISION.md) by restoring a quality gate that was red on main — while that gate is broken, every contributor's mise run build is red and the signal is trained away. No tenet is traded, so no ADR is required.
The judgment call worth naming explicitly is that the baseline rises 78 → 85, which normally deserves suspicion on a ratchet. It is defensible here because it is a reconciliation of already-merged drift (an advisory gate let it in), the number is empirically derived rather than padded to clear the bar, the removable items were actually removed rather than absorbed, and the residual is tracked in #682 with a stated path to zero. The alternative — leaving the gate red — is strictly worse. The PR description is unusually honest about this, including flagging the baseline rise for the reviewer unprompted.
Blocking issues
None.
Non-blocking nits
projectglob omits the!production marker —knip.json:27.cdkandcliboth usesrc/**/*.ts!; this entry usessrc/**/*.jswithout the!. I confirmed adding!yields the same count (0), so this is pure consistency, not behavior.- Narrow blind spot: non-
*.test.jsfiles undertest/—knip.json:26-27. Becauseprojectissrc/**/*.js, a dead helper such astest/helper.jsis invisible (verified: adding one keeps the count at 0). Wideningprojectto["src/**/*.js", "test/**/*.js"]would close it. Very low stakes — one test file exists today. - The
(44)/(41)breakdown will rot —knip-baseline.json:3.countis the only machine-read field; that prose split is a snapshot and drifts the moment #682 removes anything, in a file whose whole purpose is to be durable. Consider dropping the parenthetical and letting #682 hold the inventory. (Also: "Per-category false positives belong inknip.json, not here" is the right rule but slightly imprecise now — what landed was a workspace entry, not anignore/ignoreDependenciessuppression.) - Follow-ups lack priority labels — #682 carries only
toolingand #683 onlybug/infra-cdk/security, noP*. #683 in particular is a silent security-screening failure affecting every PDF input, which reads higher than unlabeled.
Documentation
No docs required, and correctly none shipped. No user-facing behavior, contract, env var, or command changed. I grepped docs/guides/, docs/design/, docs/decisions/, CONTRIBUTING.md, and AGENTS.md for knip-baseline / baseline 78 and found zero references, so no stale number is left behind. Mirror sync: N/A — nothing under docs/guides/, docs/design/, or CONTRIBUTING.md was touched, so mise //docs:sync is not implicated and the "Fail build on mutation" step has nothing to catch. Issue tracking is genuinely well handled: #607 closes, with #682 and #683 carving out what was deliberately left.
Tests & CI
All 5 checks green at e825a756 (build, dead-code-pr, security-pr, pull-request-lint, CodeQL), and I read the dead-code job log rather than trusting the check mark — it prints holding at baseline (85), which is the acceptance criterion of #607 met in the real CI environment, not just locally.
Bootstrap synth-coverage: not applicable. No construct, stack, or handler changed and no new CFN resource type is introduced, so cdk/src/bootstrap/policies/*, resource-action-map.ts, BOOTSTRAP_VERSION, and the DEPLOYMENT_ROLES golden baseline are all correctly untouched (ADR-002). No CDK tests changed, so the #366 synth-bundling performance concerns are also N/A.
One observation rather than a request: scripts/check-deadcode-ratchet.mjs has no unit test for its own logic (baseline compare, increase → exit 1, drop → advisory, schema-shape guard → exit 2). I checked before raising it, and no sibling script has one either (check-constants-sync.ts, check-types-sync.ts, check-coverage-thresholds-sync.ts — zero test files reference any of them), so this is a pre-existing repo-wide convention and not this PR's debt to pay. Credit where due: the countIssues guard already fails closed on a knip schema change (exit 2) instead of counting 0 and silently reporting a green gate — which is the failure mode that would otherwise matter most here.
Review agents run
code-reviewer— ran. Raised the "environment-dependent count" blocker; refuted empirically above.comment-analyzer— ran (in scope: this PR's payload is largely comment prose). Sourced nit 3 and theknip.json-vs-workspace imprecision.pr-test-analyzer— ran. Sourced nit 2 and the ratchet-has-no-test observation; its environment-dependency gap is refuted above.silent-failure-hunter— omitted: no error-handling, catch, or fallback code is touched (the only.mjschange is a one-word comment).type-design-analyzer— omitted: no new or modified types; the@types/pdf-parsechange is a manifest removal, not a type definition./security-review— omitted: no IAM, Cedar policy, network, secrets, or input-gateway change. I did separately confirm theyarn.lockedit removes only the@types/pdf-parseentry (single hunk, 7 lines, one requester), since dependency-graph edits are the one place a config-only PR can carry supply-chain risk.
Every agent-raised blocker was independently re-verified before inclusion; the two that did not survive verification are documented above with their counter-evidence rather than passed through.
Human heuristics
- Proportionality — pass. 8 additions / 13 deletions across 6 files for a build-gate fix. Two dead deps removed, one false positive scoped, one number moved. No new abstraction; the scoped-workspace choice is the smaller durable change than a blanket ignore, despite being slightly more config.
- Coherence — pass, one small deviation. The new
knip.jsonentry mirrors the existingcdk/cli/docsshape and lives where the baseline file's own rule says such suppressions belong; the missing!(nit 1) is the only stylistic drift from its siblings. - Clarity — pass. Names and structure communicate intent, and the baseline comment states the durable rule first. Only the snapshot counts undercut it (nit 3). Notably free of AI004 — nothing is hidden behind a plausible default; the count is the honest measured value and the description flags the baseline rise rather than burying it.
- Appropriateness — pass. This is the dimension the PR is strongest on: every claim was verified against the real tool at the pinned version rather than argued from first principles (AI001), the two genuinely-dead deps were removed while the one false positive was suppressed — the distinction that separates a real fix from a papered-over gate — and out-of-scope discoveries went to #682/#683 instead of expanding the diff.
Nice work on the reproduce-then-act discipline here, and on resisting the temptation to inflate the baseline to make the gate pass.
e825a75 to
950a9ce
Compare
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict — Approve (nits only)
Approve. Re-review at 950a9ce9. My previous approval was given at e825a756 and went stale when the head moved, so I re-derived the ratchet arithmetic from scratch rather than carrying the prior APPROVE forward — exactly because a one-line change to either the knip workspace scope or the baseline count can silently disable this gate.
It does not. 0 surviving blockers, 0 new blockers, 3 nits. One of the three is a correction to my own prior nit, which was based on a wrong mechanism.
Prior findings — adjudication
| # | Prior finding | Status |
|---|---|---|
| B1 | @theagenticguy: baseline absorbs 3 findings that belong in knip.json |
FIXED (re-verified at new head) |
| B2 | @theagenticguy: baseline comment blesses the ambient pdf-parse decl |
FIXED (re-verified at new head) |
| N1 | project omits the ! production marker |
FIXED |
| N2 | Dead non-test helper under test/ is invisible |
APPLIED BUT INERT — and my diagnosis was wrong; see below |
| N3 | The (44)/(41) split will rot |
FIXED |
| N4 | "belongs in knip.json" imprecise about the mechanism | FIXED |
Agent-raised concerns previously refuted (install-dependent count, @astrojs/check hoisting) stay refuted — nothing at this head changes that evidence.
What changed since e825a756, and what I re-proved
The only substantive delta is two lines: knip.json:27 (project gains ! and test/**/*.js) and knip-baseline.json:3 (prose). count is 85 at both SHAs. Note the branch was rebased, so e825a756 is no longer an ancestor of the head — I compared blobs, not just the patch.
The ratchet arithmetic, re-derived from zero. I rebuilt an isolated harness containing only integrations/jira-forge-app plus the head's knip.json, and ran the pinned knip 6.23.0 binary against it, counting with the ratchet's own COUNTED_KEYS:
| config | jira-forge-app findings |
|---|---|
no workspace entry (main's config) |
3 — src/index.js, src/proxy.js, test/proxy.test.js, all files |
this PR's config at 950a9ce9 |
0 |
So 88 − 3 = 85 is measured at the new head, not inherited from my prior read. CI agrees at this exact SHA: run 30451961530, head_sha = 950a9ce9…, ✅ Dead-code count holding at baseline (85). exit 0.
Mutation matrix re-run against the new config — the gate still fails loud on real dead code:
| mutation | detected? |
|---|---|
dead src/orphan.js |
✅ files 0→1 |
dead export unusedHelper in src/proxy.js |
✅ exports 0→1 |
rename entry src/index.js → src/main.js |
✅ 0→2, ratchet fails loud |
Regression check on the fix itself — can the new test/**/*.js in project raise the count? No. I added an unused export to the existing test/proxy.test.js: count stayed 0. Test files are entry points, so their exports are never reported. The widened scope introduces no false-positive risk. test:jira-forge-app still 9/9 locally.
Is the ! marker load-bearing? No — I confirmed the count is identical with and without it, so N1 was correctly treated as consistency-only. (--production mode does surface computeSignature, but the ratchet never runs --production, so it cannot affect the gate.)
On B1/B2 specifically: the workspace entry at knip.json:25-28 is still the scoped form, not a blanket integrations/** ignore, so real dead code there stays detectable; and knip-baseline.json:3 no longer cites the ambient decl, with the real runtime bug filed as #683 (bug/infra-cdk/security). Follow-up #682 exists and its title already tracks the number as 85, so the baseline and the issue agree. Neither blocker is carried forward.
Blocking issues
None.
Nits (non-blocking — do not hold the merge)
See the three inline comments. In summary:
knip.json:27— thetest/**/*.jsaddition is inert as shipped, and I owe you a correction. My prior nit N2 said addingtest/**/*.jstoproject"would close" the dead-test-helper blind spot. That was wrong, and you applied it in good faith. The real reason a dead helper undertest/is invisible is knip's Node.js plugin: becauseintegrations/jira-forge-app/package.jsondeclares"test": "node --test", the plugin auto-registers**/test/**/*.{cjs,mjs,js,…}as entry patterns, and entry files are never reported unused. I proved the 2×2 — a deadtest/helper.jsis detected only in the one cell where thenode --testscript is absent and the glob is present. The line is harmless and becomes useful if that script ever changes, but the commit-message rationale ("so dead test helpers stay visible") does not hold today. Worth correcting the rationale, not the code.scripts/check-deadcode-ratchet.mjs:53— the schema claim is factually wrong (pre-existing, but inside the block you edited).nsExports/nsTypesdo exist in knip 6.23.0 and are emitted by the JSON reporter; they are merely default-excluded. SinceCOUNTED_KEYSomits them, anyone who later enables them would get silently uncounted findings.- Commit message / PR description misattribute the
6.20.0 → 6.23.0pin correction to950a9ce9. By blob, that fix is already in the pre-rebasee825a756content;950a9ce9only re-lands it relative to its new parent. Pure narrative artifact of the rebase — no code impact.
Documentation
No drift. I grepped repo-wide for knip/deadcode references: the only consumers are knip.json, knip-baseline.json, mise.toml:101-107, and the ratchet script — no guide, ADR, or CONTRIBUTING.md hardcodes a baseline count, so nothing goes stale at 85. No docs/guides/ or docs/design/ source changed, so no Starlight mirror regeneration is owed. Backing issue #607 carries approved + bug; follow-ups #682/#683 are filed and labelled. Nothing needs an ADR — this is a gate re-green, not a tenet trade.
Tests & CI
All 9 checks green at 950a9ce9, including build (agentcore) and Dead-code detection (advisory). mergeStateStatus: BLOCKED is the review requirement, not a failure.
There is no unit test for check-deadcode-ratchet.mjs, so "do the new tests pin the fix?" has an honest answer: there are no new tests, and the effective test is the CI ratchet invocation itself. That is why I mutation-tested the config by hand rather than trusting the suite — and the moved-entry-point mutation (0→2, fails loud) is the property that keeps this config from rotting into a silent no-op. I am not asking for a test harness here; the gate is advisory and the config is exercised on every PR.
Bootstrap synth-coverage: not applicable — no construct, stack, handler, or CFN resource type changed. The only non-config edits are two devDependency removals and the matching 7-line yarn.lock deletion, which I confirmed is @types/pdf-parse only; @astrojs/check@^0.9.9 correctly retains its lock entry because docs/package.json:21 still declares it.
Review agents run
Process disclosure: this re-review ran in an execution context with no agent-spawn tool available — I confirmed by search that no Agent/Task/Workflow dispatch tool is exposed here, only to-do trackers. I did not silently skip the fan-out: I applied each pr-review-toolkit agent's rubric myself against the diff, and I am naming that limitation rather than implying subagents ran.
code-reviewer— applied. Config-only diff; JSON validity checked on all four manifests; workspace entry is structurally parallel to thecdk/cli/docssiblings.comment-analyzer— applied, and it earned its keep: produced nit 2 (thensExports/nsTypesschema claim) and nit 1 (the inaccuratetest/**/*.jsrationale). Both are comment-accuracy defects in a machine-adjacent file whose entire job is to stay durable.pr-test-analyzer— applied. Assessed the absent ratchet test and substituted a hand-run mutation matrix; verifiedtest:jira-forge-app9/9 at the new head.silent-failure-hunter— applied narrowly, and relevant here despite the tiny diff: the failure mode of this gate is silent success. VerifiedcountIssuesstill fails closed (exit 2) on a reshapedissuesarray, so a schema change cannot yield count 0 and a permanently green ratchet. Nit 2 is the one residual under-count path.type-design-analyzer— omitted: no types introduced or changed (no TypeScript in the diff)./security-review— omitted: no IAM, Cedar, network, secrets, or input-gateway surface touched. Removing two unused devDependencies narrows the dependency graph.
Human heuristics
- Proportionality — Pass. Two lines to close two cosmetic nits; no new abstraction. The scoped workspace remains the proportionate choice over a blanket
integrations/**ignore. - Coherence — Pass.
knip.json:25-28matches the sibling workspaces' shape and now their!convention too. - Clarity — Concern (nit-level,
knip.json:27andscripts/check-deadcode-ratchet.mjs:53): two comments/rationales assert mechanisms that do not hold. In a durable gate file, a comment that misdescribes why something works is the thing that misleads the next maintainer. - Appropriateness — Pass. Verified against the real pinned knip 6.23.0 binary and the real Forge sources, not a mock — which is precisely how nit 1 surfaced.
Thanks for taking the scoped-workspace path and for keeping #683 out of scope. The gate is honest at 85 and still fails loud.
950a9ce to
a25f2e9
Compare
a25f2e9 to
b51f848
Compare
…seline knip (#607) The dead-code ratchet (scripts/check-deadcode-ratchet.mjs) failed on main: knip counted 91 issues vs the committed baseline of 78 in knip-baseline.json, so `node scripts/check-deadcode-ratchet.mjs` exited 1. (The issue reported 81; main has drifted further since it was filed.) The +13 came from three devDependency findings plus pre-existing unused exports/types. Resolved all three devDep findings knip reported: - Removed @types/pdf-parse from cdk/package.json — genuinely dead. pdf-parse@2 ships its own types (dist/pdf-parse/cjs/index.d.cts) and cdk already has a local ambient declaration cdk/src/types/pdf-parse.d.ts; the @types stub was the stale v1 API. cdk tsc compile stays green after removal. - Removed @astrojs/check from the root package.json — a duplicate of the docs workspace's own dependency (docs/package.json), which is where `astro check` actually runs (docs:check script, docs-astro-check pre-commit hook). Root's copy was unused; astro check stays green (0 errors) after removal. - Suppressed markdown-link-check in knip.json (docs ignoreDependencies) — a knip FALSE POSITIVE, not dead code. It is invoked via ./node_modules/.bin/markdown-link-check inside docs/scripts/link-check.sh (wired into the //docs:link-check mise task), which knip cannot parse. Per the ratchet's own guidance, false positives belong in knip.json, not the baseline. Rebaselined knip-baseline.json count 78 -> 88 to the honest post-cleanup count (empirically measured, not inflated to paper over the failure). The remaining 88 are pre-existing unused exports/types and the jira-forge-app fixture files, out of scope for #607. The ratchet is advisory/non-blocking in CI today but now exits 0. Closes #607 Co-authored-by: Claude Opus 4.8 <[email protected]>
…) + fix baseline prose per review (#607) Review remediation for reviewers @theagenticguy and @isadeks: - knip.json: add a scoped workspace entry for integrations/jira-forge-app (entry src/index.js! + test/**/*.test.js; project src/**/*.js! + test/**/*.js) so the Forge function entry point named in manifest.yml and its tests are reachable. This makes the 3 phantom `files` findings drop to 0 without a blanket integrations/** ignore, keeping any real dead code there detectable (isadeks mutation-tested this variant). project carries the `!` production marker for consistency with cdk/cli, and test/**/*.js is in project scope so dead test helpers stay visible. - knip-baseline.json: count 88 -> 85. Prose fixes: drop the "jira-forge-app fixture files" wording (it is live source, not fixtures); drop the "plus the local cdk/src/types/pdf-parse.d.ts ambient declaration" clause from the @types/pdf-parse justification (that decl overrides v2's real types and is a latent bug, tracked in #683); replace the (44)/(41) numeric split with "pre-existing unused exports and types, tracked in #682" (count is the only machine-read field; numbers rot); trim narrative to the durable rule; fix the mechanism wording (workspace declaration, not just ignore). - scripts/check-deadcode-ratchet.mjs: correct the docstring pin 6.20.0 -> 6.23.0 to match root package.json (comment only). Ratchet holds at baseline 85 (exit 0). jira-forge-app tests still pass (9/9). Closes #607 Co-authored-by: Claude Opus 4.8 <[email protected]>
isadeks' open nit on scripts/check-deadcode-ratchet.mjs:53: the comment claimed `nsExports`/`nsTypes` do not exist in knip's schema. Verified against the installed, exactly-pinned knip 6.23.0 — they DO exist (dist/types/issues.d.ts:37-38) and are merely default-excluded (dist/util/get-included-issue-types.js:3). Only `classMembers` is genuinely absent. The distinction matters because this comment's stated job is to be re-derived on a knip bump: telling that reader the keys cannot exist would let a future `--include nsExports` (or a default flip) emit findings COUNTED_KEYS silently drops — an under-count, the one direction this gate exists to catch. No behavioral change: the ratchet passes no --include, so the count is unaffected. Co-Authored-By: Claude <[email protected]>
b51f848 to
d0eb13d
Compare
|
@isadeks Both remaining nits addressed in COUNTED_KEYS schema note — fixed. Verified your claims against the installed, exactly-pinned knip 6.23.0 before changing anything:
The comment now says they exist but are default-excluded, and names the consequence you identified: if anyone enables them, the reporter emits findings
You're right that the rationale was the thing worth fixing — "test/**/*.js is in project scope so dead test helpers stay visible" is not true today. One thing needing your call before this can go green: the baseline number has drifted, and not because of this PR. The I have deliberately not rebaselined to 107 — that would silently absorb two other PRs' debt into this one's baseline, which is exactly what the file's own guidance warns against ("When dead code is removed and the count drops, lower this value in the same PR to lock in the gain"). Options as I see them: (a) rebaseline to 107 here and note the drift explicitly, (b) land a separate baseline-reconciliation PR for the #345/#695 arrivals first, or (c) drive the new findings down. Preference? |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Re-review at 586abce3 (merge of main). Approve — both remaining nits from the July round were addressed in d0eb13d9: the COUNTED_KEYS schema note now correctly describes nsExports/nsTypes as present-but-default-excluded (with the right instruction if that ever changes), and the jira-forge-app workspace is scoped in knip.json rather than absorbed into the baseline — which is the distinction that keeps the ratchet honest. Baseline 85 with the remainder tracked in #682 is the right call. CI green (8/8), mergeable.
3 participants
Summary
The dead-code ratchet (
scripts/check-deadcode-ratchet.mjs) was failing onmain, independent of any PR. This re-greens it by removing 2 genuinely-dead devDependencies, suppressing 1 knip false positive, scoping the jira-forge-app as a proper knip workspace, and rebaselining the knip issue count to its honest post-cleanup value.Closes #607
Review remediation (2026-07-29, addressing @theagenticguy + @isadeks — commit 950a9ce)
Both reviewers requested changes across 6 threads. All addressed:
integrations/**ignore).knip.jsonnow declaresintegrations/jira-forge-appwithentry: ["src/index.js!", "test/**/*.test.js"]andproject: ["src/**/*.js!", "test/**/*.js"]. Thesrc/index.jshandler named inmanifest.ymland its tests are reachable, so the 3 phantomfilesfindings drop to 0 while any real dead code in the app stays detectable.projectcarries the!production marker for consistency with cdk/cli (isadeks nit 1) and includestest/**/*.jsso dead test helpers stay visible (isadeks nit 2). Count drops 88 → 85.knip-baseline.json): dropped the "jira-forge-app fixture files" wording (it is live source); dropped the "plus the local ambient declaration" clause from the@types/pdf-parsejustification (that decl overrides v2's real types and is a latent bug — see fix(cdk): PDF attachment screening broken — ambient pdf-parse decl describes v1 API against installed v2 #683); replaced the(44)/(41)numeric split with "pre-existing unused exports and types, tracked in chore(deadcode): ratchet down the 85 pre-existing unused exports/types (follow-up to #607) #682" (onlycountis machine-read; numbers rot); trimmed the narrative to the durable rule; fixed the mechanism wording to describe workspace declaration, not justignore.scripts/check-deadcode-ratchet.mjs): corrected the stale6.20.0pin comment to6.23.0, matching rootpackage.json.The original fix
I confirmed each candidate against knip and its actual usage before acting.
@types/pdf-parsefromcdk/package.json— genuinely dead.[email protected]ships its own types and the@typesstub was the stale v1 API against an installed v2. Nothing references it;mise //cdk:compilestays green.@astrojs/checkfrom the rootpackage.json— a duplicate of thedocsworkspace's own dependency.mise //docs:checkstays green after removal.markdown-link-checkinknip.json(docsignoreDependencies) — a knip false positive. It is invoked via./node_modules/.bin/markdown-link-checkindocs/scripts/link-check.sh, which knip cannot parse.integrations/jira-forge-appas a knip workspace — itssrc/index.jshandler and tests are live source, not dead files.Rebaselined
knip-baseline.jsoncount to85— the honest, empirically-measured post-cleanup count. The remaining 85 are pre-existing unused exports/types, tracked in #682.Note: this ratchet is advisory / non-blocking in CI today (
check:deadcode-ratchet), but it now exits 0 as the issue requires.Testing
node scripts/check-deadcode-ratchet.mjs→✅ Dead-code count holding at baseline (85).exit 0 (the acceptance criterion).mise //:test:jira-forge-app→ 9/9 pass (proves the Forge workspace entry points at live, tested source).pre-commit run --files <changed>→ all Passed / correctly Skipped.origin/main..HEAD) → no leaks.mise run build: the only failure is a pre-existing sandbox limitation —cdk:synth:quieterrors onec2:DescribeAvailabilityZones(the local IAM role lacks EC2 AZ-lookup permission). It is an AWS auth error, not a dep/type regression, and is unrelated to this knip/comment-only change.Files changed
package.json— drop root@astrojs/checkdevDepcdk/package.json— drop@types/pdf-parsedevDepknip.json— addmarkdown-link-checkto docsignoreDependencies; add scopedintegrations/jira-forge-appworkspaceknip-baseline.json— count78 → 85+ trimmed durable commentscripts/check-deadcode-ratchet.mjs— correct knip pin comment6.20.0 → 6.23.0yarn.lock— 7-line removal (@types/pdf-parseentry only)Follow-ups (out of scope for #607)
cdk/src/types/pdf-parse.d.tsdescribes the v1 API against installed v2, silently breaking every PDF; delete the decl, adopt v2'snew PDFParse({ data }).getText(), and add a real-PDF test.🤖 Generated with Claude Code