Repository navigation
test: resolve SonarCloud S7503 in the global fetch stub - #287
Conversation
SonarCloud's first main analysis after the maintenance sweep merged (44b4d41, scanner action v8.2.1 -> v8.3.0) opened one issue: typescript:S7503 on `stubGlobalFetch`, an async arrow with no `await`. The file did not change; the newer analyzer reports a pattern the older one let through. The `async` was doing a job, though. A handler that throws became a rejected promise, which is what the real `fetch` does with a network failure, and mcp-tool-kit.test.ts depends on it. Dropping `async` for `Promise.resolve(handler(...))` would let that throw escape `fetch()` synchronously. A `new Promise` executor keeps the behaviour: the executor runs the handler, and a throw inside it rejects the promise. A new test pins that property. It attaches `.then` to the call, so a synchronous throw fails the test before the assertion. With the `Promise.resolve` variant it fails; with this change and with the previous async wrapper it passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe global fetch stub now wraps handler execution in a promise executor, so synchronous handler errors reject the returned promise. A test covers a handler that throws ChangesGlobal fetch stub
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The stub preserves promise rejection for throwing handlers, and no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Biome (2.5.13)sdks/typescript/src/test-support/global-fetch-stub.test.tsBiome could not lint this file: nested root configuration. Check the repository's Biome configuration and plugins. sdks/typescript/src/test-support/global-fetch-stub.tsBiome could not lint this file: nested root configuration. Check the repository's Biome configuration and plugins. Comment |
|
Sonar's S4634 flagged the executor #287 wrote to clear S7503: a new Promise whose executor only calls resolve reads as a trivial promise. The executor existed only so a throwing handler rejects instead of throwing at the call. Promise.try does exactly that, calling the handler at once and turning a throw into a rejection, with no executor. Promise.resolve(handler(...)) would not: the throw escapes the call, and the throwing-handler test fails with it. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
SonarCloud's first
mainanalysis after the maintenance sweep merged (#286, 44b4d41) reported one new issue:typescript:S7503onstubGlobalFetchinsdks/typescript/src/test-support/global-fetch-stub.ts, anasyncarrow with noawait. The file did not change in #286. That merge movedsonarqube-scan-actionfrom v8.2.1 to v8.3.0, and the newer analyzer reports a pattern the old one let through. Every other repo in the sweep is at 0 open issues onmain.Why not just drop
asyncThe
asyncwrapper made a throwing handler reject the returned promise, the way the realfetchreports a network failure.mcp-tool-kit.test.tsstubs a handler that throws and expects a rejection.Promise.resolve(handler(url, init))would let that throw escapefetch()synchronously, which breaks any caller that attaches.catchto the call. The stub now returns anew Promisewhose executor calls the handler: a returned value or promise is adopted, and a throw rejects.Test
a throwing handler rejects the returned promise instead of throwing at the callattaches.thento thefetch(...)call, so a synchronous throw fails the test before the assertion. Red-proved: it fails with thePromise.resolvevariant, and passes with this change and with the previousasyncwrapper. It pins the behaviour, not the implementation.Verification
bun run lint: 162 files, no findingsbun run typecheck: cleanbun run test: 2068 pass, 0 failTest-only module, excluded from
tsconfig.build.json; nothing published changes.🤖 Generated with Claude Code
Summary by CodeRabbit
fetchis called. Network-failure scenarios now follow the expected promise-based behavior, allowing errors to be handled through standard rejection handling. This also makes fetch stub behavior more consistent with real-world fetch calls.