test: 100% coverage for index.js + close categoryNo numeric validation gap - #14
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
此 PR 針對 judicial-mcp 的開放資料工具做輸入驗證補強,並補齊入口 src/index.js 的測試覆蓋,讓核心模組(index.js/tools.js/response.js)達到完整覆蓋,同時同步更新 openspec 文件與型別註記。
Changes:
- 新增
__tests__/index.test.js,覆蓋 MCP 伺服器入口的 handler 註冊、onerror、啟動成功/失敗路徑。 - 在
list_resources補上validateInput.numericString(categoryNo)的 runtime 驗證,並同步更新 schema pattern 與 openspec 範例。 - 補齊
download_file缺少content-typeheader 的 fallback 測試,並調整/擴充 response 測試覆蓋。
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types/index.d.ts | 為 list_resources.categoryNo 補上 runtime 驗證註解,降低型別/行為誤解 |
| src/tools.js | list_resources 增加 categoryNo 純數字驗證;schema 加上 pattern;額外 export validateInput |
| openspec/specs/open-data.md | 同步補上 numericString 驗證步驟與 schema pattern,移除已修補的 Known Gap 說明 |
| openspec/changes/typescript-migration/design.md | 新增 TS/ESM/Vitest 遷移設計文件(與本 PR 主題關聯度需釐清) |
| CLAUDE.md | 更新測試檔案清單與說明,補充 index.test.js 的 mock 範圍 |
| bun.lock | 新增/提交 Bun lockfile 以固定依賴解析結果 |
| tests/tools.test.js | 配合 categoryNo 新驗證更新測資,並新增 numericString 與 content-type fallback 測試 |
| tests/response.test.js | 增加 createErrorResponse 與 createBlobResponse 的分支覆蓋 |
| tests/index.test.js | 新增入口測試,驗證 ListTools/CallTool/onerror 與啟動失敗處理 |
…egoryNo numeric validation Rebases the prior JS/Jest-era PR onto the TypeScript/Vitest codebase that develop migrated to in the meantime. Ports the same two logical changes: - src/tools.ts: add validateInput.numericString and wire it into list_resources, rejecting a non-digit categoryNo before the upstream request (closing the gap already present in the migrated code — the JS version's numericString validator never survived the TS port). Also add the matching "pattern": "^\d+$" to categoryNo's inputSchema. The check requires typeof value === 'string' before the regex test, since RegExp.test() silently coerces non-string input (e.g. a JSON number). - __tests__/index.test.ts (new): covers src/index.ts's console.log/info/warn -> stderr redirection and the safeSerialize helper's branches (string, JSON.stringify success, undefined-returning values, circular references). - __tests__/server-startup-failure.test.ts (new): covers server.connect() rejecting -> process.exit(1), in its own file with static top-level mocks rather than vi.resetModules()/vi.doMock() in a shared file. - __tests__/server.test.ts (extended): covers the registered ListTools/ CallTool handlers (including the non-Error-throw branch), server.onerror, and that main() constructed a transport and called server.connect(). - __tests__/tools.test.ts, __tests__/response.test.ts: close remaining branch gaps (missing content-type header, createBlobResponse's Buffer-vs-string branch, createErrorResponse's String(error) fallback). - openspec/specs/*.md: document the new validateInput.numericString step, add the schema pattern example, and correct src/tools.ts:N line references that shifted because of the new code. src/, response.ts, server.ts, tools.ts, index.ts now at 100% coverage (bin/judicial-mcp.ts's one-line re-export remains untested, pre-existing and out of scope here).
618f8d2 to
215b8db
Compare
Summary
Note: this PR was reworked after
developcompleted a full TypeScript + Vitest migration mid-review (the original diff targeted the now-obsolete CommonJS/Jest codebase). The two logical changes below are the same intent, ported onto the currentsrc/*.ts/ Vitest test suite.validateInput.numericStringintolist_resources(src/tools.ts), rejecting a non-digitcategoryNobefore the upstream request. Requirestypeof value === 'string'before the regex test, sinceRegExp.test()silently coerces non-string input (e.g. a JSON number) — caught in review, see reply below."pattern": "^\d+$"tocategoryNo'sinputSchema, and update theopenspec/specs/open-data.mdGIVEN/WHEN/THEN + JSON schema example accordingly.src/index.tsandsrc/server.ts— previously untested entry-point/bootstrap code — to 100% coverage: new__tests__/index.test.ts(console redirection +safeSerializebranches), new__tests__/server-startup-failure.test.ts(server.connect()rejecting →process.exit(1)), and an extended__tests__/server.test.ts(registered ListTools/CallTool handlers,server.onerror,main()'s transport/connect call).__tests__/tools.test.ts/__tests__/response.test.ts(missingcontent-typeheader,createBlobResponse's Buffer-vs-string branch,createErrorResponse'sString(error)fallback).src/tools.ts:Nline references acrossopenspec/specs/*.mdthat shifted because of the new code.src/index.ts,src/response.ts,src/server.ts,src/tools.tsare now all at 100% coverage (bin/judicial-mcp.ts's one-line re-export remains untested, pre-existing and out of scope here).Review
Ran
/code-review(multi-angle + verification) both before the original diff and again after the TS rework. Findings addressed in the rework:numericString's implicit type coercion viaRegExp.test()(also independently flagged by Copilot on this PR) — fixed with an explicittypeofcheck + test.__tests__/index.test.ts's original structure had each test redundantly re-import()ingsrc/index.ts, relying on ESM module caching so only the first import actually ran — restructured to import once inbeforeAllwith a sharedbeforeEach/afterEachspy.String(circularObj)coincides withString(plainObj), so it didn't actually prove thecatchbranch ran) — strengthened by assertingJSON.stringifywas called and threw.Deferred (accepted tradeoff, not blocking):
__tests__/server.test.ts/__tests__/server-startup-failure.test.tsmock the MCP SDK'sServer/StdioServerTransportclasses directly rather than using the SDK'sInMemoryTransport.createLinkedPair()to drive a realClient/Serverpair. Real but larger-scope improvement for a future PR.Test plan
bun run typecheck— cleanbun run test:coverage— 77 tests pass, 100% coverage onsrc/index.ts,src/response.ts,src/server.ts,src/tools.tsbun run lint— clean