Skip to content

chore: remove unused exported types#283

Merged
sjnims merged 3 commits intomainfrom
chore/281-remove-unused-exported-types
Jan 18, 2026
Merged

chore: remove unused exported types#283
sjnims merged 3 commits intomainfrom
chore/281-remove-unused-exported-types

Conversation

@sjnims
Copy link
Copy Markdown
Owner

@sjnims sjnims commented Jan 18, 2026

Description

Remove 2 unused exported types identified by knip dead code analysis:

  • ValidatedEvalConfig (src/config/schema.ts) - Zod-inferred type, never imported
  • ResumeOptions (src/state/types.ts) - different from CLI's ResumeOptions in options.ts

Note: CreateRunOptions is used internally by src/state/core.ts via type-only import, but knip doesn't detect this usage (false positive). Marked with @internal tag and configured knip to exclude @internal tagged exports.

Type of Change

  • Refactoring (code change that neither fixes a bug nor adds a feature)

Component(s) Affected

Core Infrastructure

  • Configuration (src/config/)
  • State Management (src/state/)

Motivation and Context

Continuing dead code cleanup as part of the knip integration. This removes unused exported types to keep the codebase lean and maintainable.

Fixes #281

How Has This Been Tested?

Test Configuration:

  • Node.js version: 25.3.0
  • OS: macOS

Test Steps:

  1. Run npm run typecheck - passes
  2. Run npm run lint - passes
  3. Run npm test - 1519 tests pass
  4. Run npm run knip - no more unused exported types (only configuration hints remain)

Checklist

General

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings or errors

TypeScript / Code Quality

  • All functions have explicit return types
  • Strict TypeScript checks pass (npm run typecheck)
  • ESM import/export patterns used correctly
  • Unused parameters prefixed with _
  • No any types without justification

Linting

  • I have run npm run lint and fixed all issues
  • I have run npm run format:check

Testing

  • I have run npm test and all tests pass

Additional Notes

Knip False Positive Investigation

During this cleanup, I discovered that knip has a blind spot with type-only imports (import type). The CreateRunOptions interface is used by src/state/core.ts:

import type { CreateRunOptions, PipelineState } from "./types.js";
export function createPipelineState(options: CreateRunOptions): PipelineState { ... }

However, knip's trace shows no imports from core.ts for CreateRunOptions, while PipelineState (on the same line!) IS tracked. The difference appears to be:

  • PipelineState is used in return types of exported functions
  • CreateRunOptions is used in parameter types

This suggests knip may not fully track type usage in parameter positions. Worked around by marking CreateRunOptions as @internal and configuring knip to exclude @internal tagged exports.

Reviewer Notes

Areas that need special attention:

  • The @internal tag solution for the knip false positive
  • Verify that CreateRunOptions is indeed used (check src/state/core.ts:48)

🤖 Generated with Claude Code

Remove 2 unused exported types identified by knip dead code analysis:
- ValidatedEvalConfig (src/config/schema.ts) - Zod-inferred type, never imported
- ResumeOptions (src/state/types.ts) - different from CLI's ResumeOptions in options.ts

Note: CreateRunOptions is used internally by src/state/core.ts via type-only
import, but knip doesn't detect this usage (false positive). Marked with
@internal tag and configured knip to exclude @internal tagged exports.

Fixes #281

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 18, 2026

Claude finished @sjnims's task in 1m 18s —— View job


Reviewing PR #283: Remove unused exported types

Todo List

  • Run quality checks (typecheck, lint, format)
  • Review PR diff
  • Verify knip false positive claim about CreateRunOptions
  • Provide feedback

Verdict: ✅ Looks Good

This is a clean refactoring that removes genuinely unused exports while properly handling a knip false positive.


Quality Checks ✅

All checks pass:

  • TypeScript: ✅ No errors
  • ESLint: ✅ No issues
  • Prettier: ✅ All files formatted correctly

What's Good

  1. Correct removals: Both ValidatedEvalConfig and ResumeOptions (from state/types.ts) are genuinely unused

    • ValidatedEvalConfig has zero references in the codebase
    • The removed ResumeOptions is a different type from the CLI's version (which has additional fields: error, optional pluginName/runId)
  2. Proper false positive handling: The CreateRunOptions interface IS actually used at src/state/core.ts:48:

    export function createPipelineState(options: CreateRunOptions): PipelineState

    Your diagnosis is spot-on - knip doesn't track type-only imports used in parameter positions. The @internal JSDoc tag + knip configuration is the right workaround.

  3. Clean diff: Small, focused change (-15 lines, +3 lines) that only touches what's necessary

  4. Good documentation: The @internal JSDoc comment clearly explains why CreateRunOptions should be excluded from knip's export tracking


Suggestions

No issues found. This PR is ready to merge.


@claude claude bot added chore Maintenance tasks (dependencies, CI, tooling) component:config Configuration loading (src/config/) component:state Resume and checkpoint management (src/state/) labels Jan 18, 2026
Rename two files to work around knip module resolution bug:
- src/state/core.ts → operations.ts
- src/stages/4-evaluation/detection/core.ts → orchestrator.ts

Root cause: Files named "core.ts" are completely invisible to knip's module
graph tracker. The files don't appear in --trace output and their imports/
exports aren't tracked, causing false positives for unused exports.

After renaming:
- knip properly traces operations.ts and orchestrator.ts
- CreateRunOptions usage is now correctly detected (removed @internal workaround)
- All tests pass, typecheck passes, knip reports clean

Upstream issue filed: webpro-nl/knip#1475

Related to #281

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@sjnims
Copy link
Copy Markdown
Owner Author

sjnims commented Jan 18, 2026

Update: Root Cause Identified

While investigating the knip false positive, I discovered the actual issue: knip fails to resolve files named core.ts.

Evidence

  • Renaming core.tscrud.ts immediately fixed knip's tracking
  • Both src/state/core.ts and src/stages/4-evaluation/detection/core.ts were invisible to knip
  • These files appeared zero times in --trace output despite being re-exported

Solution

Renamed both files to avoid the bug:

  • src/state/core.tsoperations.ts (state CRUD operations)
  • src/stages/4-evaluation/detection/core.tsorchestrator.ts (detection orchestration)

After renaming:
✅ knip now properly traces both files
CreateRunOptions usage correctly detected (no workaround needed)
✅ All tests pass, typecheck passes

Upstream Issue

Filed bug report: webpro-nl/knip#1475

The @internal tag workaround has been removed since it's no longer needed.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 18, 2026

Claude finished @sjnims's task in 1m 54s —— View job


Reviewing PR #283: Remove unused exported types

Todo List

  • Run quality checks (typecheck, lint, format)
  • Review PR diff
  • Verify file renaming and imports
  • Provide feedback

Verdict: ✅ Excellent Work

This PR successfully works around a knip bug and removes genuinely unused exports. The root cause analysis and upstream bug report demonstrate thorough problem-solving.


Quality Checks ✅

All checks pass:

  • TypeScript: ✅ No errors
  • ESLint: ✅ No issues
  • Prettier: ✅ All files formatted correctly

What's Good

  1. Root cause identified: The knip false positive was traced to a bug where knip fails to resolve files named core.ts. Great debugging work and upstream bug report.

  2. Clean file renaming:

    • src/state/core.tsoperations.ts (more descriptive name for CRUD operations)
    • src/stages/4-evaluation/detection/core.tsorchestrator.ts (better reflects detection orchestration)
    • All imports updated correctly through index files
    • No lingering references to old filenames
  3. Correct type removals:

    • ValidatedEvalConfig (src/config/schema.ts) - Zero references, genuinely unused
    • ResumeOptions (src/state/types.ts) - Different from CLI's ResumeOptions in options.ts which has error field and optional pluginName/runId
  4. Workaround removed: The @internal tag workaround for CreateRunOptions is no longer needed since knip now properly traces the file after renaming. CreateRunOptions is correctly used at src/state/operations.ts:48.

  5. Documentation updated: CLAUDE.md, CONTRIBUTING.md, and README.md all updated to reflect the file renames.

  6. Clean diff: Focused changes (-28 lines, +17 lines) with no unnecessary modifications.


Knip Configuration

The "tags": ["-internal"] addition to knip.json is correct - it excludes CLI-only helpers marked with @internal JSDoc tags from knip's export tracking. This is appropriate since these functions (formatters, option parsers, resume handlers) are intentionally not part of the public API.


Suggestions

None - this PR is ready to merge.


Note: Since knip requires approval to run, I couldn't verify the final knip output, but based on the code review, the changes should resolve the false positive and leave no unused exports (except the expected configuration hints).


@claude claude bot added the effort:small < 1 hour label Jan 18, 2026
1. Remove track_progress from claude-issue-analysis.yml
   - track_progress forces tag mode which adds BASE_ALLOWED_TOOLS (Edit, Write, git commands)
   - Agent mode respects tool restrictions (read-only analysis as intended)
   - Prevents Claude from creating branches during issue analysis

2. Add npm run knip to claude-pr-review.yml
   - Enables knip verification during automated PR reviews
   - Prevents "knip requires approval" blocking automated reviews

Root cause: Tag mode merges BASE_ALLOWED_TOOLS with user restrictions instead
of replacing them. Agent mode (no track_progress) uses only user-specified tools.

See: claude-code-action/src/create-prompt/index.ts (BASE_ALLOWED_TOOLS)
     claude-code-action/src/modes/tag/index.ts (tagModeTools)
     claude-code-action/src/modes/detector.ts (track_progress → tag mode)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude claude bot added refactor Code restructuring without behavior change and removed refactor Code restructuring without behavior change labels Jan 18, 2026
@sjnims sjnims merged commit 8cd5a27 into main Jan 18, 2026
17 of 18 checks passed
@sjnims sjnims deleted the chore/281-remove-unused-exported-types branch January 18, 2026 23:32
sjnims added a commit that referenced this pull request Jan 19, 2026
- Add eslint-plugin-sonarjs for additional code quality rules
- Add jscpd for copy/paste detection
- Add madge for circular dependency detection
- Remove stale .gitignore exceptions for core.ts files that were
  renamed to operations.ts and orchestrator.ts in PR #283

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance tasks (dependencies, CI, tooling) component:config Configuration loading (src/config/) component:state Resume and checkpoint management (src/state/) effort:small < 1 hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: remove unused exported types (3 exports)

1 participant