fix(types): resolve all ~99 typecheck errors, fixing real bugs along the way - #8
Merged
Conversation
…the way
`npm run typecheck` was failing with ~99 errors across five files. Root
causes, from largest to smallest:
- **zod v3 vs v4 mismatch**: `@modelcontextprotocol/server@2.0.0` (the
package actually imported for `McpServer`, not `@modelcontextprotocol/sdk`)
bundles its own zod v4 and requires v4-shaped schemas for `registerTool`.
The project's zod was `^3.23.8`. Bumped to `^4.4.3` and fixed the resulting
v3->v4 breakage: `.default()` must now precede `.transform()` in a chain,
`z.record()` needs an explicit key schema, `.ip()` was replaced by
`z.union([z.ipv4(), z.ipv6()])`.
- **pino logger calls were backwards everywhere**: pino's actual signature
is `logger.info(mergingObject, message?)` (object first); the codebase
called `logger.info(message, mergingObject)` throughout `index.ts`. Fixed
every call site.
- **`ToolResult` had drifted from the SDK's actual `CallToolResult`**:
replaced the hand-rolled interface with a type alias to
`@modelcontextprotocol/server`'s own `CallToolResult`, which also resolved
every `registerTool` overload error without touching the 15+ call sites.
- **`node-fetch` conflicted with Node 22's native `fetch`**: removed the
dependency and switched `alienVault.ts`/`virusTotal.ts` to the global
`fetch`; updated the test mocks from `vi.mock('node-fetch', ...)` to
`vi.stubGlobal('fetch', ...)`.
- **Two real bugs**: `server.invokeTool(...)` doesn't exist on `McpServer` —
the 4 per-flavor scan tools (`scan_macos_pkg` etc.) were calling a
nonexistent method. Extracted the shared scan logic into a `scanEndpoint`
function called directly by all 5 tools. Separately, `capabilities` was
merged into the server-info object instead of the constructor's second
`options` argument, and `getRecentScans` results were read via snake_case
DB column names (`s.scan_id`) instead of the actual camelCase JS
properties (`s.scanId`).
- Relative imports needed explicit `.js` extensions (required by
`moduleResolution: NodeNext`), and a handful of genuinely dead
fields/params were removed (`this.database` never read, an unused
`apiKey` param already redundant with the header it was setting, etc).
## Test plan
- [x] `npm run typecheck` — 0 errors (was ~99)
- [x] `npm run lint` — clean
- [x] `npx vitest run` — 190 passed / 9 failed, identical to the baseline
before this change (those 9 are pre-existing, unrelated test-isolation
failures in alienVault.test.ts)
- [x] `npm run build` — succeeds
- [x] Smoke-tested the compiled server (`node dist/index.js`) — boots,
validates config, and starts serving on stdio with no errors
🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Pull request overview
This PR focuses on getting CI typechecking back to green by aligning local types and runtime behavior with @modelcontextprotocol/server@2.x, upgrading to Zod v4, and cleaning up several type-driven correctness issues in the server/tool implementations.
Changes:
- Align tool/result typing with MCP server types and refactor scan tool logic to avoid using nonexistent server APIs.
- Remove
node-fetchin favor of Node’s nativefetch, updating test setup/mocks accordingly. - Upgrade to
zod@4and adjust schemas/usages (record key schemas, default/transform ordering, IP validation), plus fix pino logging call signatures and ESM.jsimport specifiers.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/index.ts | Replaces hand-rolled ToolResult with MCP server’s CallToolResult type. |
| src/test/setup.ts | Switches global HTTP mocking from node-fetch module mocking to vi.stubGlobal('fetch', ...). |
| src/index.ts | Fixes MCP server construction args, refactors scan handling, updates logging, ESM imports, and DB field usage. |
| src/database/index.ts | Updates internal imports to ESM-style .js paths. |
| src/core/virusTotal.ts | Migrates to global fetch, updates Zod v4 schema details, and adjusts request/error parsing typings. |
| src/core/virusTotal.test.ts | Removes node-fetch import and relies on globally mocked fetch. |
| src/core/alienVault.ts | Migrates to global fetch, updates Zod v4 IP validation, and tightens response.json() typing. |
| src/core/alienVault.test.ts | Removes node-fetch import and relies on globally mocked fetch. |
| src/config/index.ts | Updates Zod v4 default()/transform() ordering and type imports. |
| package.json | Removes node-fetch; upgrades zod to v4. |
| package-lock.json | Lockfile updates reflecting node-fetch removal and zod@4 upgrade. |
Suppressed comments (1)
src/index.ts:560
- Same issue as
scanId: the DB column isthreats_detected, sos.threatsDetectedwill beundefinedunless the database layer aliases/massages column names. Add a fallback here (or fix the repository SQL to return camelCase).
threatsDetected: s.threatsDetected,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| `Recent Scans (last ${limit})`, | ||
| scans.map(s => ({ | ||
| scanId: s.scan_id, | ||
| scanId: s.scanId, |
Comment on lines
37
to
+38
| // Note: Global mocks are already set up in src/test/setup.ts | ||
| // for better-sqlite3-multiple-ciphers, node-fetch, and pino | ||
| // We use vi.mocked() to access the mocked implementations | ||
| // for better-sqlite3-multiple-ciphers, the global fetch API, and pino |
Comment on lines
39
to
41
| // Note: Global mocks are already set up in src/test/setup.ts | ||
| // for better-sqlite3-multiple-ciphers, node-fetch, and pino | ||
| // We use vi.mocked() to access the mocked implementations | ||
| // for better-sqlite3-multiple-ciphers, the global fetch API, and pino | ||
|
|
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
npm run typecheck(part of CI'sTestjob, which runs lint -> typecheck -> test -> coverage) was failing with ~99 errors. Root causes, largest to smallest:@modelcontextprotocol/server@2.0.0(the package actually imported forMcpServer, not@modelcontextprotocol/sdk) bundles its own zod v4 and requires v4-shaped schemas forregisterTool. Bumped zod^3.23.8->^4.4.3and fixed the resulting breakage (.default()must precede.transform(),z.record()needs an explicit key schema,.ip()->z.union([z.ipv4(), z.ipv6()])).logger.info(mergingObject, message?); the codebase calledlogger.info(message, mergingObject)throughoutindex.ts. Fixed every call site.ToolResulthad drifted from the SDK's actualCallToolResult: replaced the hand-rolled interface with a type alias to the real one, which also cleared everyregisterTooloverload error.node-fetchconflicted with Node 22's nativefetch: removed the dependency, switched to globalfetch, updated test mocks tovi.stubGlobal('fetch', ...).server.invokeTool(...)doesn't exist onMcpServer— the 4 per-flavor scan tools were calling a nonexistent method (extracted shared logic into ascanEndpointfunction instead).capabilitieswas misplaced in the server constructor's first argument instead of the second.getRecentScansresults were read via snake_case DB column names instead of the actual camelCase JS properties..jsextensions, plus a handful of genuinely dead fields/params removed.Test plan
npm run typecheck— 0 errors (was ~99)npm run lint— cleannpx vitest run— 190 passed / 9 failed, identical to the baseline before this change (pre-existing, unrelated test-isolation failures)npm run build— succeedsnode dist/index.js) — boots and serves on stdio with no errors🤖 Generated with Claude Code