Chore: Smithery build metadata (smithery.yaml) and docs/tests alignment#17
Conversation
|
Warning Rate limit exceeded@cloudbring has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe configuration file Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
smithery.yaml (1)
4-4: Add Node source maps for better TS stack traces (optional)Consider enabling source maps at runtime for readable production traces, provided you emit source maps in the build.
- command: ["node", "dist/server.js"] + command: ["node", "--enable-source-maps", "dist/server.js"]If you haven’t already, ensure
sourceMap: true(or equivalent) in your TS/bundler config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smithery.yaml(1 hunks)
🔇 Additional comments (2)
smithery.yaml (2)
2-2: Entry configuration verifiedAll checks pass—
src/server.tsexists,tsconfig.jsonoutputs to./distfrom./src, andpackage.jsonscripts andmain/binfields point todist/server.js. No changes needed.
5-5: Ensuresmithery.yamlport matches your server’s actual listen portI didn’t find any
process.env.PORTusage orapp.listen(or similar) calls binding to port 8000 in the codebase. If your server doesn’t bind to 8000, health checks and startup may fail.• No occurrences of
process.env.PORTor.listen(insrc/
• No hard-coded8000,3000or8080found in the codePlease verify that your server startup code uses port 8000 (or update
smithery.yamlto the port your app actually listens on).
…y setting NEW_RELIC_API_KEY during server.start(); fix tool schema assertions for type-safety; vitest: tweak exclude globs; package: add ai:test:integration script
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
vitest.config.ts (1)
13-13: Harden test.exclude globs; verify coverage exclusion removal is intentional
- Globs like 'node_modules/' and 'dist/' may not consistently exclude nested files across engines. Prefer '/node_modules/' and 'dist/**' (or rely on Vitest defaults, which already exclude node_modules).
- Also double-check that dropping coverage.exclude is intentional; it can pull test files and configs into coverage and skew metrics.
Suggested tweak:
- exclude: ['node_modules/', 'dist/'], + exclude: ['**/node_modules/**', 'dist/**'],test/server/server.test.ts (6)
58-65: Avoid env bleed; prefer vi.stubEnv when availableSetting process.env directly works, but Vitest offers helpers that sandbox and auto-restore. Consider:
If your Vitest version supports it:
- const originalApiKey = process.env.NEW_RELIC_API_KEY; - process.env.NEW_RELIC_API_KEY = 'test-key'; - try { - await server.start(); - expect(mockClient.validateCredentials).toHaveBeenCalled(); - } finally { - process.env.NEW_RELIC_API_KEY = originalApiKey; - } + vi.stubEnv('NEW_RELIC_API_KEY', 'test-key'); + await server.start(); + expect(mockClient.validateCredentials).toHaveBeenCalled(); + vi.unstubAllEnvs?.();Fallback to your current try/finally if
vi.stubEnvisn’t available in your version.
69-76: Use vi.spyOn instead of reassigning mock method; also consider vi.stubEnvReassigning the mock is fine but
vi.spyOnis more idiomatic and restores cleanly.- const originalApiKey = process.env.NEW_RELIC_API_KEY; - process.env.NEW_RELIC_API_KEY = 'test-key'; - try { - mockClient.validateCredentials = vi.fn().mockResolvedValue(false); - await expect(server.start()).rejects.toThrow('Invalid New Relic API credentials'); - } finally { - process.env.NEW_RELIC_API_KEY = originalApiKey; - } + vi.stubEnv?.('NEW_RELIC_API_KEY', 'test-key'); + vi.spyOn(mockClient, 'validateCredentials').mockResolvedValue(false); + await expect(server.start()).rejects.toThrow('Invalid New Relic API credentials'); + vi.unstubAllEnvs?.();If
vi.stubEnvis unavailable, keep your try/finally env restoration but still prefervi.spyOn.
82-82: Narrow casts: replaceas anywith a tiny test-only typeCasting the entire tool to
anyloses type safety. Prefer a minimal test interface for the properties you assert.Add once at the top of this test file:
// Test-only type to keep assertions type-safe type TestTool = { name: string; description: string; inputSchema: { properties?: Record<string, any>; required?: string[]; }; };Then change here:
- const tool = server.getTool('run_nrql_query') as any; + const tool = server.getTool('run_nrql_query') as TestTool;
87-87: Optional: localize the unsafe cast to just the schema propertyYou already assert the property exists; keep the cast tight:
- expect((tool.inputSchema as any).properties.nrql.type).toBe('string'); + expect((tool.inputSchema as any).properties.nrql.type).toBe('string'); // OKNote: This line is fine; the main improvement is avoiding
as anyon the whole tool (see prior comment).
92-92: Apply the same typed cast pattern to APM tool- const tool = server.getTool('list_apm_applications') as any; + const tool = server.getTool('list_apm_applications') as TestTool;
100-100: Apply the same typed cast pattern to entity search tool- const tool = server.getTool('search_entities') as any; + const tool = server.getTool('search_entities') as TestTool;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)test/server/server.test.ts(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
… tests; expand REST alerts tests; limit coverage scope to src
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/tools/rest/alerts.test.ts (2)
45-56: Assert single request when auto_paginate is falseTo lock in the intent that pagination is disabled, assert the client was called only once.
const out = await tool.listIncidents({ auto_paginate: false }); expect(Array.isArray((out as any).items)).toBeTruthy(); expect((out as any).items).toHaveLength(1); expect((out as any).items[0].id).toBe(10); + expect(get).toHaveBeenCalledTimes(1);
58-64: Strengthen tool-definition assertions (description + schema presence)Names are good; also assert descriptions and a schema-like object exist to better cover the definitions.
const tool = new RestAlertsTool(); const policiesTool = tool.getListPoliciesTool(); const incidentsTool = tool.getListIncidentsTool(); - expect(policiesTool.name).toBe('list_alert_policies_rest'); - expect(incidentsTool.name).toBe('list_open_incidents_rest'); + expect(policiesTool.name).toBe('list_alert_policies_rest'); + expect(typeof (policiesTool as any).description).toBe('string'); + const policiesSchema = + (policiesTool as any).parameters ?? + (policiesTool as any).schema ?? + (policiesTool as any).inputSchema; + expect(policiesSchema).toBeDefined(); + + expect(incidentsTool.name).toBe('list_open_incidents_rest'); + expect(typeof (incidentsTool as any).description).toBe('string'); + const incidentsSchema = + (incidentsTool as any).parameters ?? + (incidentsTool as any).schema ?? + (incidentsTool as any).inputSchema; + expect(incidentsSchema).toBeDefined();test/client/rest-client/rest-client-requests.test.ts (4)
8-15: Use vi.stubGlobal/vi.unstubAllGlobals for fetch stubbing and cleanupAvoid manual global mutation and ts-expect-error by leveraging Vitest’s globals stubbing.
- let originalFetch: typeof global.fetch; - - beforeEach(() => { - originalFetch = global.fetch; - }); + // No need to capture original fetch when using vi.stubGlobal afterEach(() => { - global.fetch = originalFetch; - vi.restoreAllMocks(); + vi.unstubAllGlobals(); // restores any vi.stubGlobal calls + vi.restoreAllMocks(); }); @@ - // @ts-expect-error: assign to global in tests - global.fetch = mockFetch; + vi.stubGlobal('fetch', mockFetch); @@ - // @ts-expect-error: assign to global in tests - global.fetch = mockFetch; + vi.stubGlobal('fetch', mockFetch); @@ - // @ts-expect-error: assign to global in tests - global.fetch = mockFetch; + vi.stubGlobal('fetch', mockFetch);Also applies to: 32-34, 58-60, 85-86
39-45: Add header assertions for GETAlso verify the Accept header is sent (and implicitly that we don’t set Content-Type on GET).
expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining('/applications.json?page=1'), expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ 'Api-Key': apiKey }), }) ); + const init = (mockFetch.mock.calls[0]?.[1] ?? {}) as RequestInit; + expect((init.headers as any)).toEqual( + expect.objectContaining({ 'Api-Key': apiKey, Accept: 'application/json' }) + );
66-72: Assert JSON headers for POSTConfirm Content-Type and Accept headers accompany the JSON body.
expect(mockFetch).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ method: 'POST', body: JSON.stringify(payload), }) ); + const init = (mockFetch.mock.calls[0]?.[1] ?? {}) as RequestInit; + expect((init.headers as any)).toEqual( + expect.objectContaining({ + 'Api-Key': apiKey, + 'Content-Type': 'application/json', + Accept: 'application/json', + }) + );
87-90: Also assert DELETE request shape (method, URL, headers)Strengthen this test by checking the request construction alongside error handling.
await expect(client.delete('/applications/1/deployments/2')).rejects.toThrow( 'REST API error: 404 Not Found' ); + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('https://api.newrelic.com/v2/applications/1/deployments/2.json'), + expect.objectContaining({ + method: 'DELETE', + headers: expect.objectContaining({ 'Api-Key': apiKey }), + }) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/client/rest-client/rest-client-requests.test.ts(1 hunks)test/tools/rest/alerts.test.ts(1 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vitest.config.ts
…n coverage in CI; upload HTML report artifact; add markdown summary to PR job
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/ci.yml (1)
47-55: Codecov action input name is incorrect; use “files” (and consider upgrading to v4).codecov/codecov-action@v3 expects “files” (comma-separated), not “file”. With “file” nothing may upload.
Minimal fix for v3:
- - name: Upload coverage reports to Codecov + - name: Upload coverage reports to Codecov if: matrix.node-version == '20.x' - uses: codecov/codecov-action@v3 + uses: codecov/codecov-action@v3 with: - file: ./coverage/coverage-final.json + files: ./coverage/coverage-final.json flags: unittests name: codecov-umbrella fail_ci_if_error: falseOptional: upgrade to v4 (recommended by Codecov). Inputs slightly differ; example:
- uses: codecov/codecov-action@v3 + uses: codecov/codecov-action@v4 with: - files: ./coverage/coverage-final.json + files: ./coverage/coverage-final.json flags: unittests - name: codecov-umbrella - fail_ci_if_error: false + name: codecov-umbrella + token: ${{ secrets.CODECOV_TOKEN }} # required for private repos; omit if public + fail_ci_if_error: false
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
44-46: Propagate env vars to the coverage run for parity with tests.The coverage step likely re-runs tests; mirror env to avoid divergent behavior between “Run tests” and “Check test coverage”.
- - name: Check test coverage (with thresholds) - run: npm run ai:test:coverage + - name: Check test coverage (with thresholds) + env: + USE_REAL_ENV: ${{ vars.USE_REAL_ENV }} + NEW_RELIC_API_KEY: ${{ secrets.NEW_RELIC_API_KEY }} + NEW_RELIC_ACCOUNT_ID: ${{ secrets.NEW_RELIC_ACCOUNT_ID }} + run: npm run ai:test:coverage
56-63: Make artifact upload resilient to missing coverage.Add if-no-files-found to avoid failing the job when the coverage directory is absent (e.g., on early failures).
- name: Upload HTML coverage as artifact if: always() uses: actions/upload-artifact@v4 with: name: coverage-html path: coverage + if-no-files-found: warn retention-days: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/server/server-execute-tools.test.ts(1 hunks)test/tools/nerdgraph.test.ts(1 hunks)test/tools/rest/apm.test.ts(1 hunks)test/tools/rest/metrics.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
test/server/server-execute-tools.test.ts
[error] 1-1: Test failure in NewRelicMCPServer.executeTool coverage > validates search_entities inputs: expected function to throw error including 'query' but got 'Account ID must be provided'.
🔇 Additional comments (4)
test/tools/nerdgraph.test.ts (1)
5-12: Looks good – solid mocking & isolation.
The mock client pattern combined withbeforeEachreset keeps tests independent and readable.test/tools/rest/apm.test.ts (1)
34-59: Tests read well and assert the right things.
No issues noted; coverage for non-paginate branch andfilter_ids=[]is valuable.test/tools/rest/metrics.test.ts (2)
34-43: Make the “no auto_paginate” intent explicit.Pass
auto_paginate: falsein thelistMetricNamescall to guard against future default-value changes and keep the assertion deterministic.-const out = await tool.listMetricNames({ application_id: 1, host_id: 2, page: 3 }); +const out = await tool.listMetricNames({ + application_id: 1, + host_id: 2, + page: 3, + auto_paginate: false, +});
64-87: Nice parameter-serialization assertions – LGTM.
Covers optional params & pagination off path well.
… test: fix search_entities validation to include target_account_id
1 participant
Add explicit Smithery build metadata (entry/start) and align docs/tests with tool discovery behavior.
entryandstart.commandtosmithery.yamlso the Smithery CLI can find the entry point and start command.CI: lint + tests pass locally.
Summary by CodeRabbit