Audit cleanup: typecheck, 253-tool test coverage, client consistency

[JoeyJoziah]

Summary

Implements the action items from a project audit that surfaced 5 findings plus 3 pre-existing typecheck errors. Every commit is atomic, individually typecheck-clean, and tested against the same baseline (1004 → 1238 passing, 3 pre-existing Windows-only registry test failures unchanged).

Why

Audit findings (severity / status):

#	Severity	Finding	Status
1	HIGH	src/tools/* (the MCP surface, 253 tools across 28 files) had zero tests	✅
2	HIGH	19 of 253 client methods silently dropped the per-request options bag (no abort signal / per-request context); other 234 forwarded it	✅
3	MEDIUM	Type-safety holes in FMPClient (any indexer / any POST body)	✅
4	MEDIUM	src/tools/index.ts maintained three coupled sources of truth for the registration list (imports + MODULE_REGISTRATIONS + explicit body of registerAllTools)	✅
5	LOW	One utility (showHelp) had no tests; other 6 in src/utils/ were already exercised by the central utils.test.ts	✅
bonus	—	3 pre-existing typecheck errors on main (SDK signature drift + missing types for @smithery/sdk/server/stateful.js)	✅

Changes (8 atomic commits)

f08629d refactor(api): tighten FMPClient error indexer and post body types
4ee1ec2 refactor(api): extract buildConfig and toFmpError helpers in FMPClient
b267528 chore: remove dead code surfaced by depcheck/ts-prune
e11c599 fix(types): resolve 3 pre-existing typecheck errors
e7b261d test(tools): cover all 28 tool registration modules (253 tools)
4bff459 refactor(api): normalize options forwarding across AnalystClient and SearchClient
c6e06d6 refactor(tools): collapse registerAllTools to iterate MODULE_REGISTRATIONS
4658d4d test(utils): cover showHelp; verify all 7 utils now have tests

Highlights per change

FMPClient modernization (f08629d, 4ee1ec2)

• [key:string]: any → unknown on FMPErrorResponse (still narrows correctly because message: string is explicitly declared).
• data: any → unknown on post() (zero internal callers; future-proofing).
• Extracted buildConfig and toFmpError private helpers, eliminating ~3× duplicated try/catch and apikey/signal config code across get, getCSV, post. 170 → 132 lines (-22%), same public surface, same runtime behavior.

Dead code cleanup (b267528)

• Dropped unused mcp-trace dependency (never imported).
• Dropped DEFAULT_API_KEY constant (zero references project-wide).
• Dropped src/utils/index.ts barrel (zero internal importers; every consumer uses leaf paths).
• Left 28 src/api/<domain>/index.ts barrels untouched: also internally dead, but the package has no exports field so external deep-imports are technically possible.

Pre-existing typecheck fixes (e11c599)

• McpServerFactory.ts: moved capabilities from Implementation to ServerOptions (per current @modelcontextprotocol/sdk v1.0.0 surface) and dropped the unused configSchema block (the SDK constructor never consumed it).
• Added src/types/smithery-shim.d.ts declaring the public surface of @smithery/sdk/server/stateful.js. Upstream's package.json maps ./* → ./dist/* but lacks an explicit "types" condition, which prevents NodeNext from locating the bundled .d.ts. Inlining the small public surface in the shim restores types and transitively eliminates the implicit-any on the createStatefulServer callback parameter.
• Synced the McpServer test mock to the new (serverInfo, options) signature.

Tool layer coverage (e7b261d)

• New src/tools/__tests__/toolRegistration.test.ts (+267 lines, 227 new assertions) exercises every register*Tools against a mock McpServer with FMPClient prototype methods spied (get / getCSV / post).
• Per-module checks: ≥1 tool registered, names unique within module, names follow MCP conventions, descriptions non-empty, schemas are ZodRawShape, handlers are async, handlers return { content: [...] } on success and { isError: true } on client failure.
• Global checks: tool names globally unique across every module (no collisions), total tool count (asserted as ≥200, actual = 253), registerAllTools() registers the same total as the per-module sum (catches drift between MODULE_REGISTRATIONS and registerAllTools).
• meta-tools.ts intentionally excluded: different signature, already covered by DynamicToolsetManager suite.

Client options normalization (4bff459)

• AnalystClient (12 methods): added the standard options?: { signal?: AbortSignal; context?: FMPContext } parameter and forwarded it via super.get.
• SearchClient (7 methods): replaced the bespoke context?: FMPContext positional trailer with the standard options shape, picking up AbortSignal support for the first time. Internal call sites no longer wrap { context } inline; the parameter is forwarded directly.
• Tests updated: 2-arg toHaveBeenCalledWith assertions extended to expect the 3rd forwarded argument (undefined when no options supplied); previous positional-context calls now pass { context }. Behavior is identical; assertions verify forwarding.
• After this change all 253 client methods across 29 domain clients accept and forward options uniformly. (A larger architectural rewrite of the client layer to a typed function table is intentionally out of scope.)

Registration dedup (c6e06d6)

• registerAllTools() now iterates Object.values(MODULE_REGISTRATIONS) instead of duplicating the call list. ES2015+ guarantees insertion-order preservation, so registration order is unchanged.
• Adding a new domain now requires only an import + one map entry. The toolRegistration smoke test asserts that registerAllTools() total == per-module sum, so any omission fails CI.
• Tightened the map's type with satisfies Record<string, (server: McpServer, accessToken?: string) => void>.

Utility coverage (4658d4d)

• 7 new tests for showHelp: writes to console.log, includes the server name, mentions DEFAULT_PORT, lists every supplied toolset, handles empty arrays, documents all CLI flags, documents configuration precedence and environment variables.
• The module-level consoleSpy in utils.test.ts was getting detached by earlier suites' vi.restoreAllMocks(); the showHelp describe block now re-installs its own spy in beforeEach/afterEach.

Validation

Build:          ✅ tsc emits dist/ cleanly
Typecheck:      ✅ 0 errors (was 3)
Tests:          ✅ 1238 passed / 3 pre-existing Windows-only failures
Secrets:        ✅ 0 (scanned src/ for sk-*, api_key=*, FMP_ACCESS_TOKEN literals)
New console.log: 0 in production code

Coverage (excluding broken-on-Windows registry tests)

Area	% Stmts	% Branches	% Funcs
Overall	63.46	76.74	78.58
src/tools	97.30	67.84	93.54
src/utils	96.41	95.97	100
src/api (FMPClient)	96.19	85.71	100
src/server	78.44	93.93	85.71
src/prompts	80.33	83.33	100
src/types	0.00	0.00	0.00

Per-domain api/<domain> clients show lower statement coverage (16–68%) because each method body is a single super.get call; invocation correctness is asserted via the 227 tool tests plus the 46 direct Analyst/Search tests.

Notes for reviewers

• 3 pre-existing test failures unchanged: InstallationMethodVerification.test.ts and NpmPackageIntegration.test.ts spawn('npm', ...) directly; on Windows the launched npm needs npm.cmd. These tests pass on Linux/macOS CI. Not introduced by this PR; identical results on clean main. One-line fix is to swap to cross-spawn or detect process.platform.
• 47 production console.log calls in ClientStorage, DynamicToolsetManager, McpServerFactory, etc. are intentional structured logging and were not touched. Zero new console.log introduced by this PR.
• 28 src/api/*/index.ts barrels are internally dead but left intact because the package's package.json has no exports field, so external deep-imports through dist/api/<x>/index.js are technically reachable and may be relied upon by downstream consumers. Worth a follow-up decision: add an explicit exports field and delete the barrels, or keep them as published API surface.

Test plan

• npm run build succeeds
• npm run typecheck reports 0 errors
• npm run test:run shows 1238 passed / 3 pre-existing Windows failures
• npm run test:coverage produces a coverage report
• src/tools/__tests__/toolRegistration.test.ts registers 253 tools across 28 modules, all names globally unique
• Affected client suites (AnalystClient.test.ts, SearchClient.test.ts, FMPClient.test.ts) pass with new signatures

[JoeyJoziah]

Heads-up: upstream main has diverged significantly since this branch was created

This branch was prepared against 4477713 (the tip of main at the time). Since then main has advanced by 10 commits including the feat(repo): quality gates PR which made several large structural changes:

• express → fastify swap in production dependencies
• Version bumped 2.5.1 → 2.6.10
• Node engine requirement raised to >=25.3.0
• Registry tests moved from src/registry-tests/ to __tests__/unit/registry-tests/
• mcp-trace already removed
• @types/axios / @types/express removed
• Added toolception, knip, oxlint, new lint / knip / audit:prod scripts
• feat(repo): quality gates independently added { cause: error } to all FMPClient error throws

I attempted a rebase locally. The first conflict (FMPClient.ts) was easy to resolve — my refactor extracted a toFmpError helper that just needs the cause added to its two new Error calls. The second conflict (package.json / package-lock.json) revealed the scale of the divergence and several of the patches here are likely partially or fully obsolete on current main:

Patch	Likely status on current main
f08629d tighten FMPClient any → unknown	Probably mergeable, small overlap with cause: error work
4ee1ec2 extract buildConfig / toFmpError	Mergeable if cause: error is added to the helper (one-line tweak shown above)
b267528 remove mcp-trace + DEFAULT_API_KEY + src/utils/index.ts	mcp-trace already removed upstream; DEFAULT_API_KEY / src/utils/index.ts likely still applicable
e11c599 fix 3 typecheck errors	Possibly already fixed by the SDK bumps in feat(repo): quality gates. Worth re-verifying.
e7b261d 227 tests for src/tools/	Mergeable if src/tools/ structure was preserved (please confirm)
4bff459 normalize Analyst/Search options forwarding	Independent of the divergent changes; should still apply cleanly to those two clients
c6e06d6 registerAllTools iterates MODULE_REGISTRATIONS	Mergeable if src/tools/index.ts exists in the same shape
4658d4d cover showHelp	Independent; should still apply

If any of this work is still wanted, the cleanest path is probably:

1. Cherry-pick the still-applicable patches onto current main rather than rebasing the whole branch (the rebase will hit several intertwined conflicts because the surrounding files have changed). The order to consider:

   • 4658d4d (showHelp tests) — most independent
   • 4bff459 (Analyst/Search options) — independent
   • c6e06d6 (registerAllTools dedup) — if src/tools/index.ts is unchanged
   • e7b261d (227 tools tests) — if src/tools/ is unchanged
   • b267528 minus the mcp-trace portion (already done upstream)
   • f08629d + 4ee1ec2 merged together with cause: error added to toFmpError

2. Or close this PR and I can re-prepare a fresh branch against the current main if useful. Happy to do either.

No expectation that this gets merged as-is; the validation in the original PR body still describes what's on this branch, not how it interacts with current main. Posting the heads-up so the divergence is visible.

[JoeyJoziah]

Superseded by #99, which forward-ports the still-applicable subset of this work onto current main (post-divergence). #98 was a clean apply on the old base; #99 re-validates against the current architecture (express→fastify, toolception adapters, etc.) and drops the patches that became obsolete or already-done upstream. Closing in favor of #99.