Skip to content

Forward-port of #98: FMPClient cleanup, options normalization, showHelp tests#99

Open
JoeyJoziah wants to merge 3 commits into
imbenrabi:mainfrom
JoeyJoziah:improvements/forwardport-findings
Open

Forward-port of #98: FMPClient cleanup, options normalization, showHelp tests#99
JoeyJoziah wants to merge 3 commits into
imbenrabi:mainfrom
JoeyJoziah:improvements/forwardport-findings

Conversation

@JoeyJoziah
Copy link
Copy Markdown

Summary

Forward-port of three audit-finding patches from #98, rebuilt cleanly on top of the current main (which has substantially diverged since #98 was opened — express→fastify swap, toolception adapter layer, mcp-server-factory / dynamic-toolset-manager removed, registry tests relocated, etc.). Closes #98 in favor of this focused branch.

Each commit is atomic, individually typecheck-clean, and tested.

What's in scope

Three of the original eight commits still apply meaningfully against current main:

7f7b49d refactor(api): tighten FMPClient types, DRY error/config helpers
88438b3 refactor(api): normalize options forwarding in AnalystClient and SearchClient
6443060 test(utils): cover showHelp (7 assertions)

What's intentionally dropped from #98

Verified against current main; these are either obsolete, already done, or incompatible with the new architecture:

  • fix(types) for 3 pre-existing typecheck errorsmcp-server-factory/ was removed; the smithery shim is no longer needed because @smithery/sdk is no longer imported anywhere in src/.
  • registerAllTools dedupsrc/tools/index.ts was removed; the new src/toolception-adapters/coreModuleAdapters.ts is the registration entry point.
  • 227-assertion tools test suite — depended on registerAllTools and src/tools/index.ts. Could be re-architected for ToolCollector later, but that's a separate PR and the existing __tests__/unit/toolception-adapters/ToolCollector.test.ts already covers the collector primitive.
  • Dead-code cleanupmcp-trace, DEFAULT_API_KEY, and src/utils/index.ts were all removed by upstream's feat(repo): quality gates work.

Per-commit detail

7f7b49d refactor(api): tighten FMPClient types, DRY error/config helpers

Combines the two FMPClient improvements from #98 into a single change against current main. The feat(repo): quality gates PR added { cause: error } to all six inline throws without otherwise touching the duplication; this commit consolidates that error handling into one place and tightens the surface.

  • Type tightening: FMPErrorResponse[key: string] and FMPClient.post() data parameter: anyunknown. The named message: string member is preserved, so axiosError.response?.data?.message narrowing still works at the three call sites.
  • DRY refactor: Extract buildConfig (params + apikey + signal + extras) and toFmpError (axios + unknown error normalization, preserving cause). Three methods × ~30 lines of duplicated catch+config become three methods × ~10 lines that delegate to the helpers.
  • Record<string, any>object on params to avoid the well-known TS quirk where typed XParams interfaces fail to satisfy Record<string, unknown> due to missing index signatures.

File: 170 → 132 lines (-22%). Public surface unchanged. Runtime behavior unchanged. The 22 existing __tests__/unit/api/FMPClient.test.ts assertions all pass.

88438b3 refactor(api): normalize options forwarding in AnalystClient and SearchClient

19 of 253 client methods across these two clients could not be passed an AbortSignal or per-request context. Every other client (26 of 29) already accepts and forwards the standard options?: { signal?: AbortSignal; context?: FMPContext } shape. This commit aligns the outliers.

AnalystClient: 12 methods (getAnalystEstimates, getRatingsSnapshot, getHistoricalRatings, getPriceTargetSummary, getPriceTargetConsensus, getPriceTargetNews, getPriceTargetLatestNews, getStockGrades, getHistoricalStockGrades, getStockGradeSummary, getStockGradeNews, getStockGradeLatestNews) now accept and forward options.

SearchClient: 7 methods (searchSymbol, searchName, searchCIK, searchCUSIP, searchISIN, stockScreener, searchExchangeVariants). The bespoke context?: FMPContext positional trailer is replaced with options?: RequestOptions; AbortSignal support is added for the first time on this client. Internal call sites forward options directly instead of wrapping { context } inline.

Test updates are behavior-preserving:

  • 2-arg toHaveBeenCalledWith assertions are extended to expect the 3rd forwarded argument (undefined when no options supplied).
  • Test call sites that previously passed context positionally now pass { context } to match the unified shape; the underlying forwarding assertion is identical.

All 46 tests in the two affected suites pass.

6443060 test(utils): cover showHelp (7 assertions)

src/utils/showHelp.ts had no test coverage. The existing __tests__/unit/utils/utils.test.ts had a comment indicating intent ("Mock console for showHelp tests") and an unused _consoleSpy, but no actual assertions.

New describe block adds 7 tests:

  1. writes output to console.log
  2. includes the server name
  3. mentions DEFAULT_PORT from constants/
  4. lists every supplied toolset key, name, and description
  5. handles an empty toolset array without throwing
  6. includes documented CLI flags (--port, --fmp-token, --dynamic-tool-discovery, --fmp-tool-sets, --help)
  7. documents configuration precedence and environment variables (FMP_ACCESS_TOKEN, DYNAMIC_TOOL_DISCOVERY, FMP_TOOL_SETS, PORT)

The block installs its own console.log spy per test in beforeEach (the module-level _consoleSpy gets detached by earlier vi.restoreAllMocks() calls and would report zero invocations).

Validation

npm run typecheck:  clean (0 errors)
npm run test:run:   914 passed / 23 pre-existing failures
                    (baseline on this commit's parent is 907/23 - same 23 failures)

The 23 pre-existing failures are entirely in __tests__/unit/registry-tests/ and toolception-adapter Windows-related tests that spawn('npm', ...) directly; they fail on this Windows host but pass on Linux/macOS CI. Not introduced by this PR; same failures occur on a clean main checkout.

Coverage on this branch (filtering Windows-broken tests for v8 report)

Area % Stmts Notes
src/api/FMPClient.ts ~96% All 3 protected methods exercised
src/api/analyst/ covered by new options-forwarding assertions
src/api/search/ same
src/utils/showHelp.ts new coverage from 7 tests

Test plan

  • npm run typecheck reports 0 errors
  • npm run test:run shows 914 passed / 23 pre-existing failures (no new failures)
  • __tests__/unit/api/FMPClient.test.ts: 22/22
  • __tests__/unit/api/analyst/AnalystClient.test.ts: 23/23
  • __tests__/unit/api/search/SearchClient.test.ts: 23/23
  • __tests__/unit/utils/utils.test.ts: 62/62 (was 55, +7 showHelp tests)

Notes for reviewers

If you want the dropped pieces revived later:

  • The ToolCollector-based version of the tool registration test suite is straightforward — port the test logic from Audit cleanup: typecheck, 253-tool test coverage, client consistency #98's toolRegistration.test.ts to use new ToolCollector() instead of mockServer.tool, and verify against the 28 register* functions wired through coreModuleAdapters.ts. Happy to follow up with that as a separate PR if of interest.
  • The lint:fix / knip:fix scripts run cleanly on this branch.

Combines two improvements that apply cleanly to current main:

1. Type tightening (any -> unknown)
   - FMPErrorResponse[key: string] indexer
   - FMPClient.post() data parameter
   The named `message: string` member is preserved, so the existing
   `axiosError.response?.data?.message` narrowing keeps working.

2. DRY refactor of duplicated try/catch + config building
   - Extract buildConfig: handles apikey injection, signal forwarding,
     and merging of method-specific options (e.g. responseType for CSV).
   - Extract toFmpError: single normalization site for axios + unknown
     errors. Preserves the cause:error chaining added in
     feat(repo): quality gates - now in one place instead of six.

   Before: three methods x ~30 lines of duplicated config+catch.
   After:  three methods x ~10 lines, helpers own the shared logic.

   File: 170 -> 132 lines (-22%), same public surface, byte-identical
   runtime output (only annotations changed, helpers inline at JIT).

3. Loosen `params: Record<string, any>` to `params: object`
   Avoids a known TS quirk where typed XParams interfaces fail to
   satisfy Record<string, unknown> due to missing index signatures.
   `object` accepts every existing caller shape without forcing 16+
   interface edits.

Validation
- typecheck: clean
- __tests__/unit/api/FMPClient.test.ts: 22/22 passing
…chClient

Aligns these two clients with the standard options-forwarding shape used
by the other 26 domain clients. Without this change, callers cannot pass
an AbortSignal or per-request context to 19 of the 253 client methods.

AnalystClient (12 methods, was missing options support entirely)
- Added options?: { signal?: AbortSignal; context?: FMPContext } to:
  getAnalystEstimates, getRatingsSnapshot, getHistoricalRatings,
  getPriceTargetSummary, getPriceTargetConsensus, getPriceTargetNews,
  getPriceTargetLatestNews, getStockGrades, getHistoricalStockGrades,
  getStockGradeSummary, getStockGradeNews, getStockGradeLatestNews.
- super.get<T>(endpoint, params, options) at every call site.

SearchClient (7 methods, had bespoke context-only positional trailer)
- Replaced context?: FMPContext positional with options?: RequestOptions
  on searchSymbol, searchName, searchCIK, searchCUSIP, searchISIN,
  stockScreener, searchExchangeVariants. AbortSignal is now supported
  for the first time on this client.
- Internal call sites forward options directly instead of wrapping
  { context } inline.

Test updates (behavior-preserving)
- 2-arg toHaveBeenCalledWith assertions extended to expect the 3rd
  forwarded argument (undefined when options not supplied).
- Test call sites that previously passed `context` positionally now
  pass `{ context }` to match the unified options shape; the
  forwarding assertions are identical because the implementation
  forwards options through directly.

Validation
- typecheck: clean
- __tests__/unit/api/analyst/AnalystClient.test.ts: 23/23
- __tests__/unit/api/search/SearchClient.test.ts: 23/23
Adds the previously-missing test coverage for src/utils/showHelp.ts.
The existing utils.test.ts had a comment indicating intent ("Mock
console for showHelp tests") and an unused module-level _consoleSpy,
but zero assertions.

Coverage added
- writes output to console.log
- includes the server name
- mentions DEFAULT_PORT from constants/
- lists every supplied toolset key, name, and description
- handles an empty toolset array without throwing
- includes documented CLI flags (--port, --fmp-token,
  --dynamic-tool-discovery, --fmp-tool-sets, --help)
- documents configuration precedence and environment variables
  (FMP_ACCESS_TOKEN, DYNAMIC_TOOL_DISCOVERY, FMP_TOOL_SETS, PORT)

The describe block installs its own console.log spy per test (in
beforeEach) and restores in afterEach. This avoids cross-suite
interference from earlier `vi.restoreAllMocks()` calls that would
detach a module-level spy.

Validation
- __tests__/unit/utils/utils.test.ts: 62/62 (was 55)
- typecheck: clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants