pick: tegg service worker#5875
Open
akitaSummer wants to merge 1 commit intonextfrom
Open
Conversation
Deploying egg with
|
| Latest commit: |
fc6e1c1
|
| Status: | 🚫 Build failed. |
Contributor
📝 WalkthroughWalkthroughAdds a standalone service-worker package with runtime, routing, MCP support, types, decorators, test utilities/fixtures, and many tests; pins Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NodeServer as Node HTTP Server
participant TestUtil as StandaloneTestUtil
participant Runner as ServiceWorkerRunner
participant Handler as FetchEventHandler
participant Router as FetchRouter
participant MethodReg as HTTPMethodRegister
participant Controller
Client->>NodeServer: HTTP request
NodeServer->>TestUtil: wrap as Fetch Request
TestUtil->>Runner: set Context + call runner
Runner->>Handler: delegate event handling
Handler->>Router: execute middleware chain
Router->>MethodReg: match route & invoke handler
MethodReg->>Controller: instantiate & invoke method
Controller-->>MethodReg: returns Response/body
MethodReg-->>Handler: set ctx.response
Handler-->>TestUtil: return Response
TestUtil-->>NodeServer: stream/write Node response
NodeServer-->>Client: HTTP response
sequenceDiagram
participant MCPClient
participant NodeServer as Node HTTP Server
participant TestUtil as StandaloneTestUtil
participant MCPReg as MCPControllerRegister
participant MCPHelper as MCPServerHelper
participant EggFactory as EggContainerFactory
participant Controller
MCPClient->>NodeServer: POST /mcp/<name>/stream
NodeServer->>TestUtil: wrap as Fetch Request
TestUtil->>MCPReg: deliver FetchEvent
MCPReg->>MCPHelper: get/create helper for server name
MCPHelper->>EggFactory: resolve controller instance
EggFactory-->>MCPHelper: controller object
MCPHelper->>Controller: invoke tool/resource/prompt
Controller-->>MCPHelper: result
MCPHelper-->>MCPReg: stream response
MCPReg-->>TestUtil: Response stream
TestUtil-->>NodeServer: stream Node response
NodeServer-->>MCPClient: streamed HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Deploying egg-v3 with
|
| Latest commit: |
fc6e1c1
|
| Status: | 🚫 Build failed. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the @eggjs/tegg-service-worker package, which allows Egg/Tegg applications to run in a service worker environment. It implements a ServiceWorkerApp to handle FetchEvents, utilizing a Koa-based router to dispatch requests to standard HTTP or Model Context Protocol (MCP) controllers. The review feedback identifies critical architectural issues regarding the use of static singletons in HTTPControllerRegister and MCPControllerRegister that prevent multiple app instances from running concurrently. Additionally, there are concerns regarding robust error handling in the fetch handler, incorrect header processing in the MCP response bridge, and potential argument duplication when invoking MCP tools.
Comment on lines
+16
to
+29
| static instance?: HTTPControllerRegister; | ||
|
|
||
| private readonly router: KoaRouter; | ||
| private readonly checkRouters: Map<string, KoaRouter>; | ||
| private controllerProtos: EggPrototype[] = []; | ||
|
|
||
| static create(proto: EggPrototype, controllerMeta: ControllerMetadata, router: KoaRouter) { | ||
| assert(controllerMeta.type === ControllerType.HTTP, 'controller meta type is not HTTP'); | ||
| if (!HTTPControllerRegister.instance) { | ||
| HTTPControllerRegister.instance = new HTTPControllerRegister(router); | ||
| } | ||
| HTTPControllerRegister.instance.controllerProtos.push(proto); | ||
| return HTTPControllerRegister.instance; | ||
| } |
Contributor
There was a problem hiding this comment.
The use of a static instance singleton in HTTPControllerRegister prevents multiple ServiceWorkerApp instances from operating correctly within the same process. When create is called for a second app, it will reuse the singleton instance which is already bound to the first app's router. This means controllers from the second app will be registered to the first app's router, and the second app's router will remain empty.
Comment on lines
+106
to
+127
| static instance?: MCPControllerRegister; | ||
|
|
||
| private readonly router: KoaRouter; | ||
| private controllerProtos: EggPrototype[] = []; | ||
| private registeredControllerProtos: EggPrototype[] = []; | ||
| private controllerMeta: MCPControllerMeta; | ||
| mcpServerHelperMap: Record<string, () => MCPServerHelper> = {}; | ||
| middlewaresMap: Record<string, Array<(ctx: ServiceWorkerFetchContext, next: () => Promise<void>) => Promise<void>>> = {}; | ||
| registerMap: Record<string, { | ||
| tools: ServerRegisterRecord<MCPToolMeta>[]; | ||
| prompts: ServerRegisterRecord<MCPPromptMeta>[]; | ||
| resources: ServerRegisterRecord<MCPResourceMeta>[]; | ||
| }> = {}; | ||
|
|
||
| static create(proto: EggPrototype, controllerMeta: ControllerMetadata, router: KoaRouter) { | ||
| assert(controllerMeta.type === ControllerType.MCP, 'controller meta type is not MCP'); | ||
| if (!MCPControllerRegister.instance) { | ||
| MCPControllerRegister.instance = new MCPControllerRegister(controllerMeta as MCPControllerMeta, router); | ||
| } | ||
| MCPControllerRegister.instance.controllerProtos.push(proto); | ||
| return MCPControllerRegister.instance; | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to HTTPControllerRegister, the static instance singleton here breaks multi-app support. Each ServiceWorkerApp should manage its own registration state rather than relying on a shared static property.
| return new Response(null, { status: 404 }); | ||
| } catch (e) { | ||
| console.error('handle event failed:', e); | ||
| return new Response(e.message, { status: 500 }); |
Contributor
There was a problem hiding this comment.
If e is not an instance of Error (e.g., a string is thrown), e.message will be undefined, resulting in an empty response body. It is safer to convert the error to a string.
Suggested change
| return new Response(e.message, { status: 500 }); | |
| return new Response(e instanceof Error ? e.message : String(e), { status: 500 }); |
Comment on lines
+84
to
+89
| ...(reason ? { | ||
| ...reason, | ||
| ...(reason['content-length'] ? {} : { 'transfer-encoding': 'chunked' }), | ||
| } : {}), | ||
| 'X-Accel-Buffering': 'no', | ||
| }, |
Contributor
There was a problem hiding this comment.
The spread operator ...reason assumes that reason is an object. However, the writeHead signature allows reason to be an OutgoingHttpHeader[] (an array). If an array is passed, spreading it into an object will result in numeric keys, which is incorrect for headers. Additionally, the check for content-length will fail on an array.
| } else if (toolMeta.extra) { | ||
| newArgs[toolMeta.extra] = args[0]; | ||
| } | ||
| newArgs = [ ...newArgs, ...args ]; |
Contributor
There was a problem hiding this comment.
The logic newArgs = [ ...newArgs, ...args ] appears to duplicate arguments. Since newArgs is already populated with args[0] and args[1] at specific indices (lines 81-83), spreading the original args at the end will result in redundant parameters being passed to the controller method. You should likely just use newArgs if it has been populated, or ensure the mapping doesn't overlap.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
tegg/standalone/service-worker/test/fixtures/http-params/package.json-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorAdd the required Node engine constraint.
This fixture manifest is missing the Node.js version constraint required across all tegg packages.
Proposed fix
{ "name": "http-params-app", + "engines": { + "node": ">=22.18.0" + }, "eggModule": { "name": "httpParamsApp" } }Per the coding guidelines,
tegg/**/package.jsonpackages must include Node.js version>=22.18.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http-params/package.json` around lines 1 - 6, The package.json for the http-params fixture (package "http-params-app" / eggModule.name "httpParamsApp") is missing the required Node engine constraint; add an "engines" field with "node": ">=22.18.0" to the manifest so the tegg package meets the repo guideline.tegg/standalone/service-worker/src/utils/ResponseUtils.ts-1-16 (1)
1-16:⚠️ Potential issue | 🟡 MinorAdd explicit return type and avoid
any.
- Per coding guidelines, all exported methods must have explicit return type annotations (for
--isolatedDeclarations), andanyshould be replaced byunknown.- Also consider:
Buffer.isBufferties this utility to a Node environment. In a real Service Worker runtimeBufferis undefined, so this check will throwReferenceError. If the utility is expected to run in both Node test harness and actual SW runtime, guard theBufferaccess (e.g.,typeof Buffer !== 'undefined' && Buffer.isBuffer(body)).Proposed fix
export class ResponseUtils { - static createResponseByBody(body: any) { + static createResponseByBody(body: unknown): Response { if (typeof body === 'undefined' || body === null) { return new Response(null, { status: 204 }); } - if (Buffer.isBuffer(body) || typeof body === 'string' || body instanceof ReadableStream) { + const isNodeBuffer = typeof Buffer !== 'undefined' && Buffer.isBuffer(body); + if (isNodeBuffer || typeof body === 'string' || body instanceof ReadableStream) { return new Response(body as BodyInit, { status: 200 }); } return new Response(JSON.stringify(body), {As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown'" and "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/utils/ResponseUtils.ts` around lines 1 - 16, Replace the loose any and add an explicit return type on the exported method: change the signature of ResponseUtils.createResponseByBody to accept body: unknown and declare the return type as Response; guard the Node-specific check by using typeof Buffer !== 'undefined' && Buffer.isBuffer(body) before calling Buffer.isBuffer, keep the existing string and ReadableStream checks, and when falling back to JSON serialize safely (e.g., JSON.stringify(body)) after narrowing/assuming unknown to a serializable value so the method always returns a Response instance.tegg/core/types/standalone/fetch.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorChange
Promise<any>toPromise<unknown>to comply with TypeScript guidelines.The
waitUntilmethod usesPromise<any>, which violates the coding guideline to avoidanytypes. While this matches the Web Service Worker API specification, update it toPromise<unknown>to align with the project's TypeScript standards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/types/standalone/fetch.ts` around lines 1 - 5, Update the FetchEvent interface by changing the waitUntil parameter type from Promise<any> to Promise<unknown>; locate the FetchEvent declaration and modify the waitUntil(f: Promise<any>): void signature to waitUntil(f: Promise<unknown>): void so it conforms to the project's TypeScript guidelines while keeping the request and respondWith members unchanged.tegg/standalone/service-worker/test/http/inject.test.ts-1-40 (1)
1-40:⚠️ Potential issue | 🟡 MinorTests use mocha-style hooks (
before/after) without importing from Vitest.Vitest does not provide
before/afterhooks—the equivalents arebeforeAll/afterAll. Per coding guidelines, test files must "import test functions directly from vitest". Update the imports and hooks to match the established pattern (e.g.,tegg/core/metadata/test/ModuleDescriptorDumper.test.ts):import { describe, it, beforeAll, afterAll } from 'vitest'; import assert from 'node:assert/strict';Then replace
before/afterwithbeforeAll/afterAlland useassertfor response body validation where feasible instead of supertest's.expect().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/http/inject.test.ts` around lines 1 - 40, The test file is using Mocha hooks (before/after) and no Vitest imports; update the top imports to "import { describe, it, beforeAll, afterAll } from 'vitest'; import assert from 'node:assert/strict';", replace the mocha hooks "before" -> "beforeAll" and "after" -> "afterAll" (keeping the same bodies that call TestUtils.createFetchApp and cleanup), and change assertions that rely on supertest's .expect(...) to use supertest to fetch the response and then validate the response body with assert (e.g., call httpRequest(server).get(...).then(res => assert.deepStrictEqual(res.body, expected))). Ensure you update the tests referencing ServiceWorkerApp, server, TestUtils.createFetchApp, and maintain server?.close() and app?.destroy() in the afterAll cleanup.tegg/standalone/service-worker/test/http/params.test.ts-1-88 (1)
1-88:⚠️ Potential issue | 🟡 MinorImport test functions explicitly from vitest for consistency with other tegg test files.
The test file uses mocha-style globals (
describe,before,after,it) without importing them. Other tegg packages liketegg/core/metadataexplicitly import test functions from vitest:import { describe, it } from 'vitest'. Update this file to follow the same pattern and comply with the coding guideline: "Test files must follow the pattern 'test/**/*.test.ts' and import test functions directly from vitest."Suggested imports to add
import { describe, it, before, after } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/http/params.test.ts` around lines 1 - 88, The test file currently uses mocha-style globals (describe, before, after, it) without importing them; update the top imports to explicitly import the vitest test functions (describe, it, before, after) so the file follows the project's test pattern; locate the usages in this file (the top-level describe block and the before/after/it calls) and add an import like "import { describe, it, before, after } from 'vitest';" alongside the existing imports to ensure consistent test function sourcing.tegg/standalone/service-worker/src/controller/ServiceWorkerContext.ts-5-17 (1)
5-17:⚠️ Potential issue | 🟡 Minor
#responsetype is inconsistent with getter / strict-mode init.
#responseis declared asResponsebut is never initialized and the getter returnsResponse | undefined. Understrict(specificallystrictPropertyInitialization) this field should either have an initializer or be typed as optional. Type it to match the getter's actual possible values:🔧 Suggested fix
- `#response`: Response; + `#response`: Response | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/controller/ServiceWorkerContext.ts` around lines 5 - 17, The private field `#response` is declared as Response but never initialized while the getter returns Response | undefined; update the declaration to match the getter by changing the type of `#response` to Response | undefined (or initialize it) so it satisfies strictPropertyInitialization; modify the field declaration for `#response` (and keep the existing get response and set response methods) to use Response | undefined.tegg/standalone/service-worker/test/mcp/mcp.test.ts-1-31 (1)
1-31:⚠️ Potential issue | 🟡 MinorTest uses Mocha-style globals; coding guidelines require Vitest.
As per coding guidelines: "Test files must follow the pattern 'test/**/*.test.ts' and import test functions directly from vitest". This file:
- Uses global
describe/it/before/afterwith no imports.- Uses Mocha-only APIs:
before/after(Vitest isbeforeAll/afterAll) andthis.skip()inside afunction()hook (Vitest'sskipisctx.skip()via the first arg).Either migrate to Vitest or clarify which runner this package uses. Suggested Vitest conversion sketch:
+import { describe, it, beforeAll, afterAll } from 'vitest'; import assert from 'node:assert'; ... - before(async function() { - if (StandaloneTestUtil.skipOnNode()) { - return this.skip(); - } + beforeAll(async ctx => { + if (StandaloneTestUtil.skipOnNode()) { + return ctx.skip(); + } ... - }); + }); - after(async () => { + afterAll(async () => { await client?.close(); server?.close(); await app?.destroy(); });Note: the same pattern likely applies to the other
test/http/*.test.tsfiles in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/mcp/mcp.test.ts` around lines 1 - 31, This test uses Mocha globals and hooks; convert it to Vitest by importing the test helpers and replacing Mocha APIs: replace global describe/it with imports from 'vitest' (e.g., import { describe, it, beforeAll, afterAll, expect } from 'vitest'), change before -> beforeAll and after -> afterAll, and change the before hook signature from function() to (ctx) => { if (StandaloneTestUtil.skipOnNode()) return ctx.skip(); } so you call ctx.skip() instead of this.skip(); also ensure you close resources in afterAll and await client?.close()/server?.close()/app?.destroy(); apply the same Vitest migration to other tests using Mocha-style hooks (e.g., other test/http/*.test.ts files).tegg/standalone/service-worker/package.json-41-70 (1)
41-70:⚠️ Potential issue | 🟡 MinorMissing test runner devDependency and
testscript.The package ships integration tests under
test/**(e.g.test/mcp/mcp.test.ts, HTTP tests usingsupertest), but:
- No
test(orlint) script is defined — onlytypecheck.devDependenciesdoes not include a test runner (vitest/mocha) norsupertest, which is imported by the HTTP tests.Tests will fail to resolve their runner/support deps unless they're hoisted from the workspace root. Per coding guidelines, tests should run under Vitest (
**/test/**/*.test.ts). Please add the appropriate entries:"scripts": { - "typecheck": "tsgo --noEmit" + "typecheck": "tsgo --noEmit", + "test": "vitest run" }, ... "devDependencies": { "@eggjs/module-test-util": "workspace:*", "@eggjs/tegg": "workspace:*", "@types/node": "catalog:", "@types/type-is": "catalog:", + "supertest": "catalog:", "typescript": "catalog:", "undici": "catalog:" },#!/bin/bash # Confirm what runner/assertion setup the test files expect rg -nP -C2 '\b(from\s+[\x27"]vitest[\x27"]|from\s+[\x27"]mocha[\x27"]|describe\s*\(|^import\s+.*supertest)' tegg/standalone/service-worker/test fd -t f 'vitest\.config' tegg/standalone/service-worker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/package.json` around lines 41 - 70, The package.json is missing a test script and test runner/dev deps required by the files under test/**; update package.json to add a "test" script that runs Vitest (e.g. "vitest" or "vitest --run --reporter=dot") and add Vitest and Supertest to devDependencies (and types like `@types/supertest` if needed) so tests such as test/mcp/mcp.test.ts can run locally; modify the "scripts" object (where "typecheck" is defined) to include "test" (and optionally "lint"), and add "vitest" and "supertest" entries to "devDependencies" so imports in the test files resolve without relying on workspace hoisting.tegg/core/test-util/src/StandaloneTestUtil.ts-45-70 (1)
45-70:⚠️ Potential issue | 🟡 MinorFake
FetchEventomitswaitUntil/respondWith.The synthetic event at Line 49 is
any-cast to satisfyFetchEventListener, but the realFetchEventcontract exposeswaitUntil(promise)andrespondWith(response). Handlers that happen to call either on the event (e.g. future middleware or MCP flows underFetchEventHandler) would thrownot a function. Worth stubbing both now — even as no-ops returningundefined/consumed promise — so the test harness doesn't diverge silently from production behavior. The existing// TODO currently fake FetchEventacknowledges this.Happy to open a follow-up to flesh out a proper
FetchEventpolyfill for the test util.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/test-util/src/StandaloneTestUtil.ts` around lines 45 - 70, The synthetic FetchEvent created in StandaloneTestUtil.#createHTTPServerListener lacks waitUntil and respondWith so handlers calling them will fail; update the event to stub both: add event.waitUntil = (p) => { store pending promises in an array } and event.respondWith = (r) => { accept a Response or Promise<Response> and make that override the listener's returned response }, then before writing the HTTP response await Promise.allSettled(pendingPromises) and prefer any response produced via respondWith over the original listener result; implement these changes inside StandaloneTestUtil.#createHTTPServerListener around where event/request/response are handled.tegg/standalone/service-worker/src/controller/RootProtoManager.ts-10-37 (1)
10-37:⚠️ Potential issue | 🟡 MinorMap key uses unseparated concatenation; risks collisions and double-invocation.
method + hostwith no separator is fragile: any method/host pair whose concatenation coincides with another pair collides. More concretely,registerRootProto(method, cb, '')writes to keymethod, which is the same key the fallback path at Line 27 reads — so whenctx.hostis empty (orctx.method + ctx.host === ctx.method), every method-only callback is invoked twice (once viahostCbList, once viacbList). SinceHTTPMethodRegister.registeralways calls withh || '', any route registered without a host hits this path.Use an explicit separator and skip the fallback pass when it aliases the host pass.
🔧 Proposed fix
- registerRootProto(method: string, cb: GetRootProtoCallback, host?: string) { - host = host || ''; - const cbList = MapUtil.getOrStore(this.protoMap, method + host, []); - cbList.push(cb); - } - - getRootProto(ctx: EggContext): EggPrototype | undefined { - const hostCbList = this.protoMap.get(ctx.method + ctx.host); - if (hostCbList) { - for (const cb of hostCbList) { - const proto = cb(ctx); - if (proto) { - return proto; - } - } - } - - const cbList = this.protoMap.get(ctx.method); - if (!cbList) { - return; - } - for (const cb of cbList) { - const proto = cb(ctx); - if (proto) { - return proto; - } - } - } + private static key(method: string, host: string): string { + return `${method}\u0000${host}`; + } + + registerRootProto(method: string, cb: GetRootProtoCallback, host?: string): void { + const key = RootProtoManager.key(method, host ?? ''); + MapUtil.getOrStore(this.protoMap, key, []).push(cb); + } + + getRootProto(ctx: EggContext): EggPrototype | undefined { + const hostKey = RootProtoManager.key(ctx.method, ctx.host); + const fallbackKey = RootProtoManager.key(ctx.method, ''); + const lists = hostKey === fallbackKey + ? [ this.protoMap.get(hostKey) ] + : [ this.protoMap.get(hostKey), this.protoMap.get(fallbackKey) ]; + for (const cbList of lists) { + if (!cbList) continue; + for (const cb of cbList) { + const proto = cb(ctx); + if (proto) return proto; + } + } + return undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/controller/RootProtoManager.ts` around lines 10 - 37, The protoMap key concatenation is colliding and causing duplicate invocations; update registerRootProto to build keys with an explicit separator (e.g. use `${method}::${host||''}` or similar) when calling MapUtil.getOrStore, and update getRootProto to lookup the same separated key for host-specific callbacks (using the same separator for ctx.method and ctx.host) and skip the fallback lookup when the host-specific key would equal the method-only key (i.e. when ctx.host is empty) so that callbacks registered under the method-only key are not invoked twice; adjust all references to the old method+host concatenation in registerRootProto and getRootProto to use the new separator and matching key format.tegg/core/test-util/src/StandaloneTestUtil.ts-53-58 (1)
53-58:⚠️ Potential issue | 🟡 Minor
set-cookieheaders are silently merged into a single comma-joined value.Iterating
response.headersviafor…ofuses the undiciHeadersiterator, which combines multi-value headers (includingset-cookie) into a single comma-separated string.Set-Cookiemust remain as separate header lines on the wire — comma-joining it breaks cookie parsing in clients. If any test fixture ever sets multiple cookies, the assertion will be ambiguous or wrong.Use
Headers.getSetCookie()(available on undici 7.10.0+) to retrieve cookies as an array and pass them separately towriteHead:Suggested fix
- const headers: OutgoingHttpHeaders = {}; - for (const [ key, value ] of response.headers) { - headers[key.toLowerCase()] = value; - } - - res.writeHead(response.status, headers); + const headers: OutgoingHttpHeaders = {}; + for (const [ key, value ] of response.headers) { + if (key.toLowerCase() === 'set-cookie') continue; + headers[key.toLowerCase()] = value; + } + const setCookies = typeof (response.headers as any).getSetCookie === 'function' + ? (response.headers as any).getSetCookie() + : []; + if (setCookies.length) headers['set-cookie'] = setCookies; + res.writeHead(response.status, headers);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/test-util/src/StandaloneTestUtil.ts` around lines 53 - 58, When copying response.headers into the OutgoingHttpHeaders before calling res.writeHead in StandaloneTestUtil (the loop over response.headers and the res.writeHead(response.status, headers) call), ensure Set-Cookie values are not comma-joined: use response.headers.getSetCookie() to retrieve an array of cookie strings (if present) and add them to the headers object as separate entries (e.g., set headers['set-cookie'] = array) instead of relying on the for..of iteration which merges multi-value headers; keep the existing header copy for other headers and then call res.writeHead(response.status, headers).tegg/standalone/service-worker/src/http/HTTPMethodRegister.ts-78-82 (1)
78-82:⚠️ Potential issue | 🟡 Minor
searchParams.get()may returnnull; cast tostringhides it.
URLSearchParams.getreturnsstring | nullwhen the key is absent. Casting tostringpropagates anullto the controller's declaredstringparameter, which will surface asTypeErrors in downstream string operations. Drop the cast and let consumers see the realstring | null, or coerce to''/undefinedexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/http/HTTPMethodRegister.ts` around lines 78 - 82, The current assignment in the HTTPParamType.QUERY branch casts ctx.url.searchParams.get(...) to string which can hide nulls; update the QueryParamMeta handling inside the switch (the HTTPParamType.QUERY case) so args[index] receives a safe type instead of force-casting to string—either assign the raw result (string | null) by removing the cast or explicitly coerce null to a safe value (e.g., undefined or an empty string) before assigning to args[index]; adjust any downstream typing if you choose to change the runtime value.tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts-239-250 (1)
239-250:⚠️ Potential issue | 🟡 MinorDuplicate route name
'mcpStatelessStreamNotAllowed'for GET and DEL.
@eggjs/routertreats the first positional string as a name and maintains a name→route index; registering two routes with the same name (once on GET, once on DEL) collides and either overwrites the prior entry inrouter.route(name)lookups or throws, depending on version. Give each a unique name (e.g.,'mcpStatelessStreamGetNotAllowed'/…DelNotAllowed) or drop the name argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines 239 - 250, The route registration uses the same route name 'mcpStatelessStreamNotAllowed' for both GET and DEL which collides in the router; update the Reflect.apply calls that use getRouterFunc and delRouterFunc on this.router to supply unique names (e.g. 'mcpStatelessStreamGetNotAllowed' and 'mcpStatelessStreamDelNotAllowed') or remove the name argument entirely, ensuring you keep streamPath and notAllowedHandler unchanged so the handlers and paths remain the same.tegg/standalone/service-worker/src/ServiceWorkerApp.ts-62-74 (1)
62-74:⚠️ Potential issue | 🟡 MinorUser-supplied
fetchRouter/rootProtoManagerhandlers are silently overwritten.The spread of
options?.innerObjectHandlershappens before the two framework-mandated keys are assigned, so any caller-providedfetchRouterorrootProtoManagerentries are silently clobbered. If you intend those two slots to be framework-owned, consider throwing/warning on conflict; otherwise re-order the spread so caller values win. The same object-identity caveat applies tologger/httpclient, but those at least use a!fooguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` around lines 62 - 74, The current construction of innerObjectHandlers spreads options?.innerObjectHandlers before assigning framework-owned keys fetchRouter and rootProtoManager, which silently overwrites any caller-provided handlers; update the logic in ServiceWorkerApp where innerObjectHandlers is built (the innerObjectHandlers variable, options?.innerObjectHandlers, fetchRouter, rootProtoManager, logger, httpclient, getDefaultHttpClient) to avoid clobbering caller values—either move the spread to come after the framework assignments so caller entries take precedence, or explicitly detect conflicts for fetchRouter/rootProtoManager and throw or log a warning when options?.innerObjectHandlers contains those keys; keep the existing !guard behavior for logger/httpclient or apply the same conflict-detection approach for consistency.tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts-200-202 (1)
200-202:⚠️ Potential issue | 🟡 MinorUnguarded
request.json()on the POST body.
ctx.event.request.json()throws on malformed/empty JSON. In that case the handler aborts beforectx.responseis set, surfacing as a framework 500 instead of a JSON-RPC 2.0 error envelope (compare with the-32000envelope produced on GET/DELETE at Lines 221–238). Wrap intry/catchand respond with a JSON-RPC-32700("Parse error") to stay spec-compliant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines 200 - 202, The handler calls ctx.event.request.json() unguarded which throws on malformed or empty JSON; wrap that call in a try/catch around the parsing in MCPControllerRegister (the block where body = await ctx.event.request.json()) and on a parse failure set ctx.response to a JSON-RPC 2.0 error envelope with code -32700 and an appropriate "Parse error" message (matching the error envelope shape used for GET/DELETE paths), ensuring the handler returns that JSON-RPC response instead of letting the exception bubble to the framework.
🧹 Nitpick comments (18)
tegg/standalone/service-worker/src/types.ts (1)
1-4: Make proto metadata immutable.
protoNameandcontextKeyare identity metadata used for context lookup; mark themreadonlyto prevent accidental reassignment by consumers.Proposed fix
export interface ProtoMeta { - protoName: PropertyKey; - contextKey: string | symbol; + readonly protoName: PropertyKey; + readonly contextKey: string | symbol; }As per coding guidelines, use
readonlymodifiers where appropriate in TypeScript to prevent unintended mutations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/types.ts` around lines 1 - 4, The ProtoMeta interface exposes mutable identity fields; make protoName and contextKey readonly to prevent accidental reassignment by consumers. Update the ProtoMeta declaration (interface ProtoMeta) to mark the protoName and contextKey properties as readonly so both PropertyKey protoName and string | symbol contextKey become immutable.tegg/standalone/service-worker/test/fixtures/http/AopMiddlewareController.ts (1)
8-10: Nit: add explicit return type for isolatedDeclarations compatibility.- async aopMiddlewareTest() { + async aopMiddlewareTest(): Promise<{ msg: string }> { return { msg: 'hello' }; }As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http/AopMiddlewareController.ts` around lines 8 - 10, The method aopMiddlewareTest is missing an explicit return type which breaks --isolatedDeclarations; update the async method signature for aopMiddlewareTest to include an explicit Promise return type (e.g. Promise<{ msg: string }>) so the method has a clear, explicit return type annotation.tegg/standalone/service-worker/test/http/priority.test.ts (1)
23-28: Nit: test title doesn't match the asserted route.The test is titled
should /* workbut exercisesGET /view/foo(ViewController'sGET /*fallback). Consider renaming toshould fallback /* route workorshould /view/* workfor clarity when a failure is reported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/http/priority.test.ts` around lines 23 - 28, Rename the test title to accurately reflect the exercised route: update the it(...) string in the test containing the GET '/view/foo' request (the test function currently titled "should /* work") to something like "should fallback /* route work" or "should /view/* work" so the name matches the exercised ViewController fallback route and improves clarity when failures are reported.tegg/standalone/service-worker/test/fixtures/http/GetController.ts (1)
10-13: Minor:nilquery parameter typing.
@HTTPQuery() nil: stringwill beundefinedwhen the query is missing (perHTTPMethodRegister.createHandler, which usessearchParams.get()... cast to string). The type should arguably bestring | undefined, but since this is a test fixture exercising falsy/truthy branches, it's acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http/GetController.ts` around lines 10 - 13, The test handler nullBody uses `@HTTPQuery`() nil: string but the query can be missing (HTTPMethodRegister.createHandler uses searchParams.get() which yields string | null/undefined), so update the parameter type to string | undefined (e.g., change the signature of nullBody to accept nil: string | undefined) and adjust any related test expectations if needed; reference the nullBody method and `@HTTPQuery` usage to locate the change.tegg/standalone/service-worker/test/fixtures/mcp/McpTestAdvice.ts (1)
11-15:middlewaresignature narrows thenexttype vs. the abstract declaration.
AbstractControllerAdvice.middlewaredeclaresnext: Next(from@eggjs/tegg-types/controller-decorator), but herenextis typed as() => Promise<void>. While compatible in practice, it deviates from the base contract and from the exportedNexttype. Prefernext: Nextfor consistency with the base class and the HTTP counterpart wiring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/mcp/McpTestAdvice.ts` around lines 11 - 15, The middleware method here narrows the parameter type for next; update middleware to match the abstract contract by using the exported Next type from `@eggjs/tegg-types/controller-decorator` (i.e. change the signature in McpTestAdvice.middleware from next: () => Promise<void> to next: Next), add the Next import if missing, and keep the implementation body unchanged so it conforms to AbstractControllerAdvice.middleware.tegg/standalone/service-worker/src/mcp/AbstractControllerAdvice.ts (1)
1-12: Consider relocatingAbstractControllerAdviceout of themcp/folder.This base class is consumed by both HTTP (
HttpTestAdvice) and MCP (McpTestAdvice) advices, so its placement undersrc/mcp/is misleading. Consider moving it to a neutral location (e.g.src/controller/orsrc/advice/) to reflect the shared scope.Also, the
_ctxparameter inaroundis untyped beyond the default generics and the return type isPromise<any>; this matchesIAdvicestructurally, but if you can narrowK/result type for service-worker usage it would be safer thanany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/AbstractControllerAdvice.ts` around lines 1 - 12, AbstractControllerAdvice is placed under src/mcp/ but is shared by HttpTestAdvice and McpTestAdvice; move the class file to a neutral location (e.g. src/controller/ or src/advice/) and update any imports in HttpTestAdvice and McpTestAdvice to the new module; also tighten the typing of the around method by replacing the untyped _ctx and Promise<any> with the appropriate AdviceContext generic and a concrete result type suited for service-worker usage (reference IAdvice, AdviceContext, AbstractControllerAdvice.around, middleware, and ServiceWorkerFetchContext).tegg/standalone/service-worker/test/fixtures/http/HttpTestAdvice.ts (1)
1-16: Near-duplicate ofMcpTestAdvice— consider consolidating.
HttpTestAdviceandMcpTestAdviceonly differ by the name of the exported log array. For test fixtures this is acceptable, but a small shared helper (e.g.createLoggingAdvice(log)) would reduce duplication if more advice fixtures are added. Also, same nit as inMcpTestAdvice: prefernext: Nextto match the abstract base class signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http/HttpTestAdvice.ts` around lines 1 - 16, HttpTestAdvice duplicates McpTestAdvice; extract a small factory helper (e.g., createLoggingAdvice) that returns a class or Advice instance wired to a provided log array to eliminate duplication, and replace the exported httpAdviceExecutionLog usage with the shared helper; also update the middleware signature in HttpTestAdvice (and the factory) to use the concrete Next type to match AbstractControllerAdvice.middleware (refer to symbols HttpTestAdvice, httpAdviceExecutionLog, McpTestAdvice, AbstractControllerAdvice, middleware, and Next).tegg/standalone/service-worker/test/http/router.test.ts (1)
47-57: AOP log assertions are order-insensitive; consider asserting strict order.
includes('before')+includes('after')will pass even if the log ends up as['after', 'before']or contains spurious entries from a prior test run. Since you already reset the log on line 48, asserting ordering gives stronger regression coverage:Proposed strengthening
- assert(httpAdviceExecutionLog.length > 0, 'middleware should have been executed'); - assert(httpAdviceExecutionLog.includes('before'), 'middleware before should have been called'); - assert(httpAdviceExecutionLog.includes('after'), 'middleware after should have been called'); + assert.deepStrictEqual(httpAdviceExecutionLog, [ 'before', 'after' ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/http/router.test.ts` around lines 47 - 57, The test "should execute AOP middleware for HTTP controller" uses order-insensitive assertions on the httpAdviceExecutionLog; replace the includes checks with a strict order assertion to ensure middleware ran in sequence and no stray entries remain. After resetting httpAdviceExecutionLog.length = 0, assert the array contents and order (e.g. check length is exactly 2 and that httpAdviceExecutionLog[0] === 'before' and httpAdviceExecutionLog[1] === 'after', or use a deep equality assertion against ['before','after']) referencing the httpAdviceExecutionLog used in the test so the assertion will fail if order or extra entries are present.tegg/standalone/service-worker/src/controller/ControllerRegisterFactory.ts (1)
18-24: Optional: re-registering the same type silently overwrites the existing creator.
registerControllerRegisterusesMap.setwithout detecting a prior registration for the sameControllerTypeLike. A later/duplicate registration would silently replace the HTTP or MCP creator — usually a bug in wiring. Consider throwing (or at least warning) if a creator is already registered fortype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/controller/ControllerRegisterFactory.ts` around lines 18 - 24, The registerControllerRegister call currently uses the internal `#registerCreatorMap.set` without checking for existing entries, so duplicate registrations silently overwrite prior creators; update registerControllerRegister to check `#registerCreatorMap.has`(type) (where type is the ControllerTypeLike key) before calling set and either throw an error or log a warning and skip the overwrite, and ensure getControllerRegister and any callers still retrieve the intended creator from `#registerCreatorMap` consistently.tegg/core/types/standalone/ServiceWorkerContext.ts (1)
7-14: Preferunknown(or a third generic) overanyforbody.As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown". The
bodyaccessors here are the root source ofanypropagating intoBaseServiceWorkerContextImpland downstream code.🔧 Suggested fix
-export interface ServiceWorkerContext<Event, Response> { +export interface ServiceWorkerContext<Event, Response, Body = unknown> { event: Event; get response(): Response | undefined; set response(response: Response); - get body(): any | undefined; - set body(body: any); + get body(): Body | undefined; + set body(body: Body); }If breaking the public shape is undesired, at minimum switch the two
anytounknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/types/standalone/ServiceWorkerContext.ts` around lines 7 - 14, The ServiceWorkerContext interface currently uses `any` for the body accessors which propagates unsafe typing; update the `get body()` and `set body(...)` signatures in ServiceWorkerContext<Event, Response> to use `unknown` (or introduce a third generic like Body) instead of `any`, and then update downstream implementations such as BaseServiceWorkerContextImpl to accept and handle `unknown` (or the new Body generic) accordingly so types remain safe without changing external behavior.tegg/standalone/service-worker/test/fixtures/http-builtin/BuiltinController.ts (1)
13-22: Nit: add explicit return types.As per coding guidelines, "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag". Applies even to fixtures for consistency:
- async log() { + async log(): Promise<{ ok: boolean }> { ... - async httpclientCheck() { + async httpclientCheck(): Promise<{ hasRequest: boolean }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http-builtin/BuiltinController.ts` around lines 13 - 22, The methods in BuiltinController lack explicit return type annotations; update async log to declare its return type (e.g., Promise<{ ok: boolean }>) and async httpclientCheck to declare its return type (e.g., Promise<{ hasRequest: boolean }>) so both exported methods have explicit TypeScript return types to satisfy --isolatedDeclarations and project guidelines.tegg/standalone/service-worker/test/Utils.ts (1)
6-29: Add explicit return types on the publicTestUtilshelpers.As per coding guidelines, exported methods require explicit return types for
--isolatedDeclarations.- static baseDir(name: string) { + static baseDir(name: string): string { ... - static async createApp(name: string, init?: ServiceWorkerAppOptions) { + static async createApp(name: string, init?: ServiceWorkerAppOptions): Promise<ServiceWorkerApp> { ... - static async createFetchApp(name: string, init?: ServiceWorkerAppOptions) { + static async createFetchApp(name: string, init?: ServiceWorkerAppOptions): Promise<{ app: ServiceWorkerApp; server: import('node:http').Server }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/Utils.ts` around lines 6 - 29, Add explicit return type annotations to the exported TestUtils helper methods: give baseDir(name: string) a return type of string, annotate createApp(name: string, init?: ServiceWorkerAppOptions) with Promise<ServiceWorkerApp>, and annotate createFetchApp(name: string, init?: ServiceWorkerAppOptions) with Promise<{ app: ServiceWorkerApp; server: ReturnType<typeof StandaloneTestUtil.startHTTPServer> } or the concrete HTTP server type used); update the method signatures in the TestUtils class (baseDir, createApp, createFetchApp) accordingly so the file compiles with --isolatedDeclarations.tegg/standalone/service-worker/test/mcp/mcp.test.ts (1)
33-114: Sharedclientacross tests can leak connections / cause flakes.Each
itreassigns the module-scopedclientwithout closing the previous one. Only the lastclientis closed inafter. Because the tests share state and run sequentially, a failure betweenconnectand theafterhook can leave open transports. Also, reusing the same name/version per test is fine but unnecessary — consider scoping the client to each test and closing it in afinally/afterEach:- it('should call echo tool', async () => { - client = new Client({ ... }); + it('should call echo tool', async () => { + const localClient = new Client({ ... }); const transport = new StreamableHTTPClientTransport(...); - await client.connect(transport); + await localClient.connect(transport); try { - const result = await client.callTool({ ... }); + const result = await localClient.callTool({ ... }); assert.deepStrictEqual(result, { ... }); } finally { - // nothing + await localClient.close(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/mcp/mcp.test.ts` around lines 33 - 114, Tests reuse a module-scoped client variable which can leak open connections; change each test to create its own Client and StreamableHTTPClientTransport locally and ensure the transport/client is closed in a finally or an afterEach hook. Specifically, stop reassigning the module-scoped client and instead instantiate Client(...) and StreamableHTTPClientTransport(...) inside each it block, call await client.connect(...) as before, and then call the client.close() (or transport.close()) in a finally block or implement an afterEach that checks and closes the client; update references to client.connect, client.callTool and any use of the shared client variable accordingly so no test leaves connections open.tegg/standalone/service-worker/src/http/HTTPControllerRegister.ts (1)
50-76: Minor: instantiateHTTPMethodRegisteronce per method.Each method is wrapped in a
HTTPMethodRegistertwice — once forcheckDuplicate()(Lines 64–66) and once forregister()(Lines 72–74). Building the instances once in the first pass and reusing them in the second would remove the duplication and the extra allocations.Separate nit on Line 1: use
node:assertto match thenode:protocol used by sibling modules (e.g.ContextProtoLoadUnitHook.ts).♻️ Proposed fix
-import assert from 'assert'; +import assert from 'node:assert'; ... - doRegister(rootProtoManager: RootProtoManager) { + doRegister(rootProtoManager: RootProtoManager): void { const methodMap = new Map<HTTPMethodMeta, EggPrototype>(); for (const proto of this.controllerProtos) { const metadata = proto.getMetaData(CONTROLLER_META_DATA) as HTTPControllerMeta; for (const method of metadata.methods) { methodMap.set(method, proto); } } const allMethods = Array.from(methodMap.keys()) .sort((a, b) => b.priority - a.priority); - for (const method of allMethods) { - const controllerProto = methodMap.get(method)!; - const controllerMeta = controllerProto.getMetaData(CONTROLLER_META_DATA) as HTTPControllerMeta; - const methodRegister = new HTTPMethodRegister( - controllerProto, controllerMeta, method, this.router, this.checkRouters); - methodRegister.checkDuplicate(); - } - - for (const method of allMethods) { - const controllerProto = methodMap.get(method)!; - const controllerMeta = controllerProto.getMetaData(CONTROLLER_META_DATA) as HTTPControllerMeta; - const methodRegister = new HTTPMethodRegister( - controllerProto, controllerMeta, method, this.router, this.checkRouters); - methodRegister.register(rootProtoManager); - } + const registers = allMethods.map(method => { + const controllerProto = methodMap.get(method)!; + const controllerMeta = controllerProto.getMetaData(CONTROLLER_META_DATA) as HTTPControllerMeta; + return new HTTPMethodRegister(controllerProto, controllerMeta, method, this.router, this.checkRouters); + }); + for (const r of registers) r.checkDuplicate(); + for (const r of registers) r.register(rootProtoManager); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/http/HTTPControllerRegister.ts` around lines 50 - 76, In doRegister, avoid creating HTTPMethodRegister twice for each method: during the first loop where you call checkDuplicate(), instantiate a single HTTPMethodRegister per method (using methodMap/allMethods to locate the method and controllerProto) and store these instances in a local array or Map keyed by the HTTPMethodMeta; then in the second loop retrieve and reuse the stored HTTPMethodRegister instances to call register(rootProtoManager) instead of re-creating them; also update the module import for assert to use node:assert to match sibling modules.tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts (1)
31-41: Fragile FormData detection and silent duplicate-key overwrite.
- Line 33:
body.constructor.name === 'FormData'breaks under bundlers that mangle class names and returnsfalsefor subclasses. Preferbody instanceof FormData(with atypeof FormData !== 'undefined'guard if the target runtime may lack it).- Line 36: assigning
res[key] = valuein the loop silently overwrites earlier values when the same key is sent multiple times —FormDatasupports multi-value keys. Tests that exercise multi-value form fields would get misleading results.♻️ Proposed fix
- if (body.constructor.name === 'FormData') { - const res = {}; - for (const [ key, value ] of (body as FormData).entries()) { - res[key] = value; - } + if (typeof FormData !== 'undefined' && body instanceof FormData) { + const res: Record<string, unknown> = {}; + for (const key of new Set(body.keys())) { + const values = body.getAll(key); + res[key] = values.length > 1 ? values : values[0]; + } return { type: 'formData', body: res }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts` around lines 31 - 41, In echoBody (async echoBody(`@HTTPBody`() body: object)) replace the fragile FormData check and single-value assignment: instead of body.constructor.name === 'FormData' use a safe runtime check (typeof FormData !== 'undefined' && body instanceof FormData) to detect FormData across bundlers/runtimes; when iterating (body as FormData).entries(), accumulate values per key so duplicate keys are preserved — if a key is seen once store the value, if seen again convert to/append into an array — and return that structure under { type: 'formData', body: res } so multi-value form fields are not silently overwritten.tegg/standalone/service-worker/src/hook/ContextProtoLoadUnitHook.ts (1)
23-37: Avoidas anyon the proto factory; tighten the function type instead.The
(() => { ... }) as anyat Line 31 silently widens the callback to bypassStandaloneInnerObjectProto's constructor signature. If that signature later changes, the mismatch won't surface. Prefer annotating the thunk with the expected factory type (or extracting a typedcreateEventProtohelper) so the assertion narrows real type drift.Also add an explicit return type on
static registerPrototypeper the--isolatedDeclarations/strict-mode guideline.As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown" and "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/hook/ContextProtoLoadUnitHook.ts` around lines 23 - 37, The registerPrototype method uses an untyped thunk "(() => { const ctx = ContextHandler.getContext(); ... }) as any" which hides type mismatches; remove the cast and give the thunk the exact factory type expected by StandaloneInnerObjectProto (or extract a typed helper like createContextProto(factoryKey: string): ExpectedFactoryType that returns ctx.get(protoMeta.contextKey) with proper typing), and add an explicit return type to static registerPrototype to satisfy isolatedDeclarations/strict-mode; update references to StandaloneInnerObjectProto, ContextHandler.getContext, and protoMeta.contextKey when applying the typed thunk/helper so the constructor signature is respected.tegg/standalone/service-worker/src/http/ServiceWorkerFetchContext.ts (1)
23-39: Alignresponsegetter and setter types for consistency.The
responsesetter parameter type (Response) does not match the getter return type (Response | undefined). The parent class has the same asymmetry, which could cause confusion. For clarity, consider making both useResponse | undefinedto accurately reflect that the property may be unset.Minor also: the pattern of assigning
response.bodyto#bodyin the response setter is safe in the current codebase because middleware handlers use eitherctx.responseorctx.bodyexclusively (not both), andFetchEventHandleronly returns the response without accessing the body field afterward. However, documenting this intent or restructuring to make the boundary clearer would improve maintainability if the pattern evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/http/ServiceWorkerFetchContext.ts` around lines 23 - 39, The response property types are inconsistent—make the setter accept Response | undefined to match the getter and avoid surprises; change the signature of the response setter to (response: Response | undefined) and update its body assignment to this.#body = response?.body (handle undefined safely); ensure any uses of ServiceWorkerFetchContext.response handle undefined accordingly and keep the body setter unchanged (it can still call ResponseUtils.createResponseByBody(body)).tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts (1)
62-65: Dead code:void controllerMeta.
controllerMetais fetched, tagged withvoidto suppress the unused warning, and never used. Either use it (e.g., to propagate name/version into argument resolution) or drop the lookup entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts` around lines 62 - 65, The code fetches controllerMeta via controllerProto.getMetaData(CONTROLLER_META_DATA) into a MCPControllerMeta and then immediately voids it, leaving dead code; either remove the lookup entirely or actually use controllerMeta (for example, pass controllerMeta.name/version into the argument resolver or include it when building MCP controller info inside resolveControllerArgs or wherever controllerProto is processed). Locate the lines using controllerProto.getMetaData, CONTROLLER_META_DATA, and the controllerMeta variable and then either delete the fetch and the unused variable, or thread controllerMeta into the relevant logic (e.g., resolveControllerArgs or controller registration) so its name/version are propagated rather than suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 677e6a1c-bbf9-4174-8ee0-cdb98c064206
📒 Files selected for processing (60)
pnpm-workspace.yamltegg/core/standalone-decorator/src/event/EventHandler.tstegg/core/standalone-decorator/src/index.tstegg/core/test-util/package.jsontegg/core/test-util/src/StandaloneTestUtil.tstegg/core/types/package.jsontegg/core/types/standalone/ServiceWorkerContext.tstegg/core/types/standalone/fetch.tstegg/core/types/standalone/index.tstegg/standalone/service-worker/package.jsontegg/standalone/service-worker/src/ServiceWorkerApp.tstegg/standalone/service-worker/src/constants.tstegg/standalone/service-worker/src/controller/ControllerMetadataManager.tstegg/standalone/service-worker/src/controller/ControllerRegister.tstegg/standalone/service-worker/src/controller/ControllerRegisterFactory.tstegg/standalone/service-worker/src/controller/RootProtoManager.tstegg/standalone/service-worker/src/controller/ServiceWorkerContext.tstegg/standalone/service-worker/src/hook/ContextProtoLoadUnitHook.tstegg/standalone/service-worker/src/hook/ControllerLoadUnitHook.tstegg/standalone/service-worker/src/hook/ControllerPrototypeHook.tstegg/standalone/service-worker/src/http/FetchEventHandler.tstegg/standalone/service-worker/src/http/FetchRouter.tstegg/standalone/service-worker/src/http/HTTPControllerRegister.tstegg/standalone/service-worker/src/http/HTTPMethodRegister.tstegg/standalone/service-worker/src/http/ServiceWorkerFetchContext.tstegg/standalone/service-worker/src/index.tstegg/standalone/service-worker/src/mcp/AbstractControllerAdvice.tstegg/standalone/service-worker/src/mcp/MCPControllerRegister.tstegg/standalone/service-worker/src/mcp/MCPServerHelper.tstegg/standalone/service-worker/src/types.tstegg/standalone/service-worker/src/utils/RequestUtils.tstegg/standalone/service-worker/src/utils/ResponseUtils.tstegg/standalone/service-worker/test/Utils.tstegg/standalone/service-worker/test/fixtures/http-builtin/BuiltinController.tstegg/standalone/service-worker/test/fixtures/http-builtin/package.jsontegg/standalone/service-worker/test/fixtures/http-inject/UserController.tstegg/standalone/service-worker/test/fixtures/http-inject/UserService.tstegg/standalone/service-worker/test/fixtures/http-inject/package.jsontegg/standalone/service-worker/test/fixtures/http-params/ParamController.tstegg/standalone/service-worker/test/fixtures/http-params/package.jsontegg/standalone/service-worker/test/fixtures/http-priority/PriorityController.tstegg/standalone/service-worker/test/fixtures/http-priority/ViewController.tstegg/standalone/service-worker/test/fixtures/http-priority/package.jsontegg/standalone/service-worker/test/fixtures/http/AopMiddlewareController.tstegg/standalone/service-worker/test/fixtures/http/GetController.tstegg/standalone/service-worker/test/fixtures/http/HttpTestAdvice.tstegg/standalone/service-worker/test/fixtures/http/PostController.tstegg/standalone/service-worker/test/fixtures/http/package.jsontegg/standalone/service-worker/test/fixtures/mcp/MCPTestController.tstegg/standalone/service-worker/test/fixtures/mcp/McpTestAdvice.tstegg/standalone/service-worker/test/fixtures/mcp/package.jsontegg/standalone/service-worker/test/http/builtin.test.tstegg/standalone/service-worker/test/http/inject.test.tstegg/standalone/service-worker/test/http/params.test.tstegg/standalone/service-worker/test/http/priority.test.tstegg/standalone/service-worker/test/http/response.test.tstegg/standalone/service-worker/test/http/router.test.tstegg/standalone/service-worker/test/mcp/mcp.test.tstegg/standalone/service-worker/tsconfig.jsontsconfig.json
Comment on lines
+4
to
+20
| export abstract class AbstractEventHandler<E = any, R = any> { | ||
| abstract handleEvent(event: E): Promise<R>; | ||
| } | ||
|
|
||
| export const EVENT_HANDLER_ATTRIBUTE = Symbol('EVENT_HANDLER_ATTRIBUTE'); | ||
|
|
||
| export type EventType = Record<string, string>; | ||
|
|
||
| export const EventHandler: ImplDecorator<AbstractEventHandler, EventType> = | ||
| QualifierImplDecoratorUtil.generatorDecorator(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE); | ||
|
|
||
| export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams) => { | ||
| return (clazz: EggProtoImplClass<AbstractEventHandler>) => { | ||
| EventHandler(type)(clazz); | ||
| SingletonProto(params)(clazz); | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tighten types on the newly exported public API.
Several guideline-level issues on this new decorator module:
- Line 8 —
EVENT_HANDLER_ATTRIBUTEis declared as a bareSymbol(). Exported symbols should carry aunique symbolannotation to remain nominal across consumers. As per coding guidelines: "Use 'unique symbol' type annotation for exported symbols instead of bare Symbol() declarations". - Line 4 —
AbstractEventHandler<E = any, R = any>usesanydefaults. As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown". - Lines 15–20 —
EventHandlerProtoand its inner returned function both lack explicit return types. As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🔧 Proposed fix
-export abstract class AbstractEventHandler<E = any, R = any> {
+export abstract class AbstractEventHandler<E = unknown, R = unknown> {
abstract handleEvent(event: E): Promise<R>;
}
-export const EVENT_HANDLER_ATTRIBUTE = Symbol('EVENT_HANDLER_ATTRIBUTE');
+export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE');
export type EventType = Record<string, string>;
export const EventHandler: ImplDecorator<AbstractEventHandler, EventType> =
QualifierImplDecoratorUtil.generatorDecorator(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE);
-export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams) => {
- return (clazz: EggProtoImplClass<AbstractEventHandler>) => {
+export const EventHandlerProto = (
+ type: EventType[keyof EventType],
+ params?: SingletonProtoParams,
+): ((clazz: EggProtoImplClass<AbstractEventHandler>) => void) => {
+ return (clazz: EggProtoImplClass<AbstractEventHandler>): void => {
EventHandler(type)(clazz);
SingletonProto(params)(clazz);
};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export abstract class AbstractEventHandler<E = any, R = any> { | |
| abstract handleEvent(event: E): Promise<R>; | |
| } | |
| export const EVENT_HANDLER_ATTRIBUTE = Symbol('EVENT_HANDLER_ATTRIBUTE'); | |
| export type EventType = Record<string, string>; | |
| export const EventHandler: ImplDecorator<AbstractEventHandler, EventType> = | |
| QualifierImplDecoratorUtil.generatorDecorator(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE); | |
| export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams) => { | |
| return (clazz: EggProtoImplClass<AbstractEventHandler>) => { | |
| EventHandler(type)(clazz); | |
| SingletonProto(params)(clazz); | |
| }; | |
| }; | |
| export abstract class AbstractEventHandler<E = unknown, R = unknown> { | |
| abstract handleEvent(event: E): Promise<R>; | |
| } | |
| export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE'); | |
| export type EventType = Record<string, string>; | |
| export const EventHandler: ImplDecorator<AbstractEventHandler, EventType> = | |
| QualifierImplDecoratorUtil.generatorDecorator(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE); | |
| export const EventHandlerProto = ( | |
| type: EventType[keyof EventType], | |
| params?: SingletonProtoParams, | |
| ): ((clazz: EggProtoImplClass<AbstractEventHandler>) => void) => { | |
| return (clazz: EggProtoImplClass<AbstractEventHandler>): void => { | |
| EventHandler(type)(clazz); | |
| SingletonProto(params)(clazz); | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts` around lines 4 -
20, The exported API needs stronger TypeScript typing: change
AbstractEventHandler's generic defaults from "any" to "unknown" (update
AbstractEventHandler<E = unknown, R = unknown>), annotate
EVENT_HANDLER_ATTRIBUTE as a unique symbol (use "export const
EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE')"),
and add explicit return type annotations for EventHandlerProto and its returned
decorator function (declare the return type of EventHandlerProto and the inner
function explicitly to match EggProtoImplClass<AbstractEventHandler> -> void or
the appropriate decorator signature). Ensure you update references to these
symbols/types in the file to match the new signatures.
| export * from './typing.ts'; | ||
| export * from './util/index.ts'; | ||
| export * from './decorator/index.ts'; | ||
| export * from './event/EventHandler.ts'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and look at the content around line 4
fd -t f "index.ts" tegg/core/standalone-decorator/src/ -x cat -n {}Repository: eggjs/egg
Length of output: 301
🏁 Script executed:
# Also check the EventHandler.ts file to see the actual exports
fd -t f "EventHandler.ts" tegg/core/standalone-decorator/ -x cat -n {}Repository: eggjs/egg
Length of output: 1014
Update re-export specifier and exposed public API to meet strict TypeScript guidelines.
Line 4 uses .ts extension which violates ESM requirements. Additionally, the re-exported EventHandler.ts module contains several strict mode violations that must be fixed before exposure:
AbstractEventHandlerusesanydefaults instead ofunknownEVENT_HANDLER_ATTRIBUTEis a bareSymbol()withoutunique symbolannotationEventHandlerProtolacks explicit return type annotationEventTypeuses type alias instead of interface- Exported symbols lack JSDoc documentation
Required changes
Update line 4 in tegg/core/standalone-decorator/src/index.ts:
-export * from './event/EventHandler.ts';
+export * from './event/EventHandler.js';Update tegg/core/standalone-decorator/src/event/EventHandler.ts:
/**
* Abstract base class for event handlers
*/
export abstract class AbstractEventHandler<E = unknown, R = unknown> {
abstract handleEvent(event: E): Promise<R>;
}
/** Attribute key for event handler qualifiers */
export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE');
/** Event type mapping */
export interface EventType {
readonly [key: string]: string;
}
/**
* Decorator to create an event handler prototype scoped as singleton
*/
export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams): ((clazz: EggProtoImplClass<AbstractEventHandler>) => void) => {
return (clazz: EggProtoImplClass<AbstractEventHandler>) => {
EventHandler(type)(clazz);
SingletonProto(params)(clazz);
};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export * from './event/EventHandler.ts'; | |
| export * from './event/EventHandler.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/standalone-decorator/src/index.ts` at line 4, Update the public
re-export to drop the .ts extension (export * from './event/EventHandler') and
modify the EventHandler module: change AbstractEventHandler generic defaults
from any to unknown and make handleEvent return Promise<R>, declare
EVENT_HANDLER_ATTRIBUTE as const EVENT_HANDLER_ATTRIBUTE: unique symbol =
Symbol('EVENT_HANDLER_ATTRIBUTE'), convert EventType to an interface using
readonly index signature, give EventHandlerProto an explicit function return
type ((type: EventType[keyof EventType], params?: SingletonProtoParams) =>
(clazz: EggProtoImplClass<AbstractEventHandler>) => void) and ensure
EventHandlerProto calls EventHandler(type)(clazz) and
SingletonProto(params)(clazz); add concise JSDoc comments for exported symbols
(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE, EventType, EventHandlerProto) to
meet strict TypeScript guidelines.
Comment on lines
+127
to
+129
| "./standalone": "./standalone/index.ts", | ||
| "./standalone/fetch": "./standalone/fetch.ts", | ||
| "./standalone/ServiceWorkerContext": "./standalone/ServiceWorkerContext.ts", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a -t f . tegg/core/types/standalone tegg/core/types/src/standalone 2>/dev/null
fd -a 'tsdown.config*' tegg/core/types
cat tegg/core/types/tsconfig.json 2>/dev/nullRepository: eggjs/egg
Length of output: 274
🏁 Script executed:
cat tegg/core/types/package.jsonRepository: eggjs/egg
Length of output: 15242
🏁 Script executed:
#!/bin/bash
# Look for tsdown.config in parent directories
find tegg -name 'tsdown.config*' -type f
# Also check if there's a build script in package.json
fd -a 'tsdown.config*' . 2>/dev/null | head -20Repository: eggjs/egg
Length of output: 2267
🏁 Script executed:
#!/bin/bash
# Check the structure and understand the pattern
ls -la tegg/core/types/
ls -la tegg/core/types/dist/ 2>/dev/null || echo "No dist directory yet"Repository: eggjs/egg
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Check what's in the standalone directory
ls -la tegg/core/types/standalone/
head -20 tegg/core/types/standalone/index.tsRepository: eggjs/egg
Length of output: 419
🏁 Script executed:
#!/bin/bash
# Compare with another tegg/core package to see the pattern
cat tegg/core/metadata/tsdown.config.ts 2>/dev/null | head -20Repository: eggjs/egg
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check if there's a root tsdown or build script handling all packages
cat pnpm-workspace.yaml 2>/dev/null | grep -A 10 "tegg/core"
ls -la tegg/ | grep -i build
cat tegg/package.json 2>/dev/null | jq '.scripts' 2>/dev/null || echo "No tegg/package.json"Repository: eggjs/egg
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Check root tsdown or build configuration
cat package.json | jq '.scripts' 2>/dev/null | head -30
# Check if tegg/core/types is mentioned anywhere special
rg "tegg/core/types" --type yaml --type json pnpm-workspace.yaml package.jsonRepository: eggjs/egg
Length of output: 1629
🏁 Script executed:
#!/bin/bash
# Look for any build or documentation about how types package is built
rg "tsdown.config" tegg/core/types
rg "types package" tegg/CLAUDE.md 2>/dev/null | head -5
rg "type-only\|types-only" tegg/ 2>/dev/null | head -5Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check the git history or if this is a new file
git log --oneline -n 20 -- tegg/core/types/package.json 2>/dev/null || echo "No git history available"
git status tegg/core/types/ 2>/dev/null | head -20Repository: eggjs/egg
Length of output: 153
Add missing tsdown.config.ts and reconsider the ./standalone/* convention.
The ./standalone/* entries break from the ./src/* convention used throughout the package. Additionally, tegg/core/types is the only tegg/core package without a tsdown.config.ts file. While other tegg/core packages configure tsdown.config.ts with entry: { index: 'src/index.ts' }, this package has none.
The published exports declare ./dist/standalone/index.js, but without a tsdown config, the build won't generate the dist/standalone/ directory. Either add tsdown.config.ts to configure the standalone directory build, or move the files to src/standalone/ to follow the established pattern used by all other tegg/core packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/types/package.json` around lines 127 - 129, This package declares
exports like "./standalone" and references files ./standalone/index.ts,
./standalone/fetch.ts, and ./standalone/ServiceWorkerContext.ts but lacks a
tsdown.config.ts and therefore the dist/standalone/ artifacts won't be built;
either add a tsdown.config.ts that adds entry mappings for the standalone
outputs (pointing each standalone path to its source .ts file) so tsdown will
emit dist/standalone/*, or move the standalone sources into src/standalone/ and
update package.json exports to follow the ./src/* convention used across other
tegg/core packages.
Comment on lines
+7
to
+20
| addController(metadata: ControllerMetadata) { | ||
| const typeControllers = MapUtil.getOrStore(this.controllers, metadata.type, []); | ||
| // 1.check controller name | ||
| // 2.check proto name | ||
| const sameNameControllers = typeControllers.filter(c => c.controllerName === metadata.controllerName); | ||
| if (sameNameControllers.length) { | ||
| throw new Error(`duplicate controller name ${metadata.controllerName}`); | ||
| } | ||
| const sameProtoControllers = typeControllers.filter(c => c.protoName === metadata.protoName); | ||
| if (sameProtoControllers.length) { | ||
| throw new Error(`duplicate proto name ${String(metadata.protoName)}`); | ||
| } | ||
| typeControllers.push(metadata); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return types and prefer some() over filter().length.
Public methods should have explicit return type annotations (per coding guidelines for --isolatedDeclarations). The duplicate checks can also be simplified to short-circuit instead of walking the full list twice.
♻️ Proposed refactor
- addController(metadata: ControllerMetadata) {
+ addController(metadata: ControllerMetadata): void {
const typeControllers = MapUtil.getOrStore(this.controllers, metadata.type, []);
// 1.check controller name
// 2.check proto name
- const sameNameControllers = typeControllers.filter(c => c.controllerName === metadata.controllerName);
- if (sameNameControllers.length) {
+ if (typeControllers.some(c => c.controllerName === metadata.controllerName)) {
throw new Error(`duplicate controller name ${metadata.controllerName}`);
}
- const sameProtoControllers = typeControllers.filter(c => c.protoName === metadata.protoName);
- if (sameProtoControllers.length) {
+ if (typeControllers.some(c => c.protoName === metadata.protoName)) {
throw new Error(`duplicate proto name ${String(metadata.protoName)}`);
}
typeControllers.push(metadata);
}
- clear() {
+ clear(): void {
this.controllers.clear();
}As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/controller/ControllerMetadataManager.ts`
around lines 7 - 20, The addController method lacks an explicit return type and
uses filter().length to detect duplicates; update the method signature for
addController(metadata: ControllerMetadata) to include an explicit return type
(void) and replace the duplicate checks that use
typeControllers.filter(...).length with short-circuit checks using
typeControllers.some(c => c.controllerName === metadata.controllerName) and
typeControllers.some(c => c.protoName === metadata.protoName) (keeping the same
thrown Error messages), leaving MapUtil.getOrStore(this.controllers,
metadata.type, []) and the final typeControllers.push(metadata) unchanged.
Comment on lines
+14
to
+16
| registerControllerRegister(type: ControllerTypeLike, creator: RegisterCreator) { | ||
| this.#registerCreatorMap.set(type, creator); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotations to public methods.
registerControllerRegister has no explicit return type. Per coding guidelines, "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag". getControllerRegister is already annotated; please add : void here for consistency.
Proposed fix
- registerControllerRegister(type: ControllerTypeLike, creator: RegisterCreator) {
+ registerControllerRegister(type: ControllerTypeLike, creator: RegisterCreator): void {
this.#registerCreatorMap.set(type, creator);
}As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| registerControllerRegister(type: ControllerTypeLike, creator: RegisterCreator) { | |
| this.#registerCreatorMap.set(type, creator); | |
| } | |
| registerControllerRegister(type: ControllerTypeLike, creator: RegisterCreator): void { | |
| this.#registerCreatorMap.set(type, creator); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/controller/ControllerRegisterFactory.ts`
around lines 14 - 16, The public method registerControllerRegister in
ControllerRegisterFactory lacks an explicit return type; update its signature to
include ": void" so it matches getControllerRegister and satisfies the
isolatedDeclarations requirement, e.g., change the registerControllerRegister
method declaration in the ControllerRegisterFactory class (the method that sets
this.#registerCreatorMap) to have an explicit void return type.
Comment on lines
+193
to
+209
| // Create bridge ServerResponse → PassThrough → Fetch Response | ||
| let callback: (value: object) => void; | ||
| const resPromise = new Promise<object>(resolve => { | ||
| callback = resolve; | ||
| }); | ||
| const response = new ServiceWorkerMCPServerResponse(req, callback!); | ||
|
|
||
| // Parse body from Fetch Request | ||
| const body = await ctx.event.request.json(); | ||
|
|
||
| // Handle the request (don't await - handleRequest writes to response stream) | ||
| transport.handleRequest(req, response, body); | ||
|
|
||
| const init = await resPromise; | ||
|
|
||
| ctx.response = new Response(Readable.toWeb(response.stream) as any, init) as any; | ||
| }; |
Contributor
There was a problem hiding this comment.
Handler can hang or swallow errors from transport.handleRequest.
Two related correctness bugs:
transport.handleRequest(req, response, body)is fire-and-forget (Line 204). If it rejects (bad payload, transport failure), the rejection becomes an unhandled promise and the outer handler continues toawait resPromise.resPromise(Lines 195–197) is only resolved fromwriteHeadvia thecallback. If the request errors beforewriteHeadis called, or the transport decides not to respond, the route handler blocks forever — no timeout, norejectpath — leaking sockets/contexts for every such request.
Please wire handleRequest's rejection into a reject branch of resPromise (or Promise.race it against a close/error event on response), and surface errors as a proper Fetch Response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines
193 - 209, The current handler fires transport.handleRequest(req, response,
body) without wiring failures into resPromise (created with callback) so
rejections become unhandled and the route can hang if writeHead never runs;
change the logic around resPromise to create a proper promise with resolve and
reject, attach .catch on transport.handleRequest to call reject with the error,
and also race that promise with a timeout or response stream "close"/"error"
event (from ServiceWorkerMCPServerResponse.stream) so unresolved requests are
rejected; finally, when the promise rejects return a proper Fetch Response
(e.g., 500 + error info) instead of letting the handler hang.
Comment on lines
+79
to
+89
| let newArgs: any[] = []; | ||
| if (schema && toolMeta.detail) { | ||
| newArgs[toolMeta.detail.index] = args[0]; | ||
| if (toolMeta.extra) { | ||
| newArgs[toolMeta.extra] = args[1]; | ||
| } | ||
| } else if (toolMeta.extra) { | ||
| newArgs[toolMeta.extra] = args[0]; | ||
| } | ||
| newArgs = [ ...newArgs, ...args ]; | ||
| return Reflect.apply(realMethod, realObj, newArgs) as ReturnType<ToolCallback>; |
Contributor
There was a problem hiding this comment.
Argument remapping looks buggy for tools and prompts.
Two problems in both mcpToolRegister (Lines 79–89) and mcpPromptRegister (Lines 112–122):
if (toolMeta.extra)/if (promptMeta.extra)—extrais documented as a parameter index (seeMCPToolMeta.extra?: number). A falsy-check on a numeric index drops the legitimate caseextra === 0, so whichever parameter sits at position 0 will never receiveargs[1](orargs[0]in the no-schema branch). Useextra !== undefined.newArgs = [ ...newArgs, ...args ](Lines 88, 121) appends the original MCP args onto the tail of the already-remapped array. Concretely, withdetail.index = 2,newArgsbecomes[undefined, undefined, args[0], args[0], args[1], …]and the underlying controller method is invoked with duplicated/trailing positional arguments. Either the remap is unnecessary or the final spread is — as written, both are performed.
🔧 Proposed fix (mcpToolRegister shown; mirror for prompt)
- let newArgs: any[] = [];
+ const newArgs: any[] = [];
if (schema && toolMeta.detail) {
newArgs[toolMeta.detail.index] = args[0];
- if (toolMeta.extra) {
+ if (toolMeta.extra !== undefined) {
newArgs[toolMeta.extra] = args[1];
}
- } else if (toolMeta.extra) {
+ } else if (toolMeta.extra !== undefined) {
newArgs[toolMeta.extra] = args[0];
}
- newArgs = [ ...newArgs, ...args ];
return Reflect.apply(realMethod, realObj, newArgs) as ReturnType<ToolCallback>;Also applies to: 112-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts` around lines 79 -
89, The argument remapping in mcpToolRegister and mcpPromptRegister is buggy:
change the falsy checks on toolMeta.extra / promptMeta.extra to explicit
undefined checks (use extra !== undefined) so index 0 is handled, and stop
appending the original args with newArgs = [...newArgs, ...args]; instead
construct newArgs that only sets the specific positions (e.g., set
newArgs[detail.index] = args[0] and newArgs[extra] = args[1] when provided) and
then call Reflect.apply(realMethod, realObj, newArgs) — remove the final spread
that duplicates the original args (mirror this fix in both mcpToolRegister and
mcpPromptRegister).
| EggPrototypeLifecycleUtil.registerLifecycle(this.controllerPrototypeHook); | ||
|
|
||
| // Build dependencies list - include this package as a framework dependency | ||
| const frameworkDep = { baseDir: path.join(__dirname, '..'), extraFilePattern: [ '!**/test' ] }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the package is configured as ESM and whether __dirname is polyfilled anywhere
fd -H package.json tegg/standalone/service-worker --exec cat {}
echo '---'
fd -H tsconfig.json tegg/standalone/service-worker --exec cat {}
echo '---'
rg -nP '__dirname|fileURLToPath' tegg/standalone/service-workerRepository: eggjs/egg
Length of output: 2755
🏁 Script executed:
#!/bin/bash
# Find all imports of ServiceWorkerApp and related usage
rg -nP 'ServiceWorkerApp|from.*ServiceWorkerApp' tegg/standalone/service-worker
echo '---'
# Check the full ServiceWorkerApp.ts file structure
cat -n tegg/standalone/service-worker/src/ServiceWorkerApp.ts | head -80Repository: eggjs/egg
Length of output: 6684
Use fileURLToPath(import.meta.url) instead of __dirname to support native ESM.
In native ESM, __dirname is undefined and will cause a ReferenceError when the ServiceWorkerApp constructor runs. The coding guidelines mandate ESM-only for tegg/**, and this package is configured as "type": "module". Fix by importing fileURLToPath from node:url and computing __dirname at the module level:
import path from 'node:path';
+import { fileURLToPath } from 'node:url';
import {
EggPrototypeLifecycleUtil,
LoadUnitLifecycleUtil,
} from '@eggjs/tegg-metadata';
import { Runner, RunnerOptions, StandaloneContext } from '@eggjs/tegg-standalone';
import type { Logger } from '@eggjs/tegg-types';
import { getDefaultHttpClient } from 'urllib';
import { ContextProtoProperty } from './constants.ts';
import { FetchRouter } from './http/FetchRouter.ts';
import { RootProtoManager } from './controller/RootProtoManager.ts';
import { ControllerMetadataManager } from './controller/ControllerMetadataManager.ts';
import { ControllerRegisterFactory } from './controller/ControllerRegisterFactory.ts';
import { ContextProtoLoadUnitHook } from './hook/ContextProtoLoadUnitHook.ts';
import { ControllerPrototypeHook } from './hook/ControllerPrototypeHook.ts';
import { ControllerLoadUnitHook } from './hook/ControllerLoadUnitHook.ts';
import { HTTPControllerRegister } from './http/HTTPControllerRegister.ts';
import { MCPControllerRegister } from './mcp/MCPControllerRegister.ts';
+const __dirname = path.dirname(fileURLToPath(import.meta.url));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const frameworkDep = { baseDir: path.join(__dirname, '..'), extraFilePattern: [ '!**/test' ] }; | |
| import path from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| import { | |
| EggPrototypeLifecycleUtil, | |
| LoadUnitLifecycleUtil, | |
| } from '@eggjs/tegg-metadata'; | |
| import { Runner, RunnerOptions, StandaloneContext } from '@eggjs/tegg-standalone'; | |
| import type { Logger } from '@eggjs/tegg-types'; | |
| import { getDefaultHttpClient } from 'urllib'; | |
| import { ContextProtoProperty } from './constants.ts'; | |
| import { FetchRouter } from './http/FetchRouter.ts'; | |
| import { RootProtoManager } from './controller/RootProtoManager.ts'; | |
| import { ControllerMetadataManager } from './controller/ControllerMetadataManager.ts'; | |
| import { ControllerRegisterFactory } from './controller/ControllerRegisterFactory.ts'; | |
| import { ContextProtoLoadUnitHook } from './hook/ContextProtoLoadUnitHook.ts'; | |
| import { ControllerPrototypeHook } from './hook/ControllerPrototypeHook.ts'; | |
| import { ControllerLoadUnitHook } from './hook/ControllerLoadUnitHook.ts'; | |
| import { HTTPControllerRegister } from './http/HTTPControllerRegister.ts'; | |
| import { MCPControllerRegister } from './mcp/MCPControllerRegister.ts'; | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| const frameworkDep = { baseDir: path.join(__dirname, '..'), extraFilePattern: [ '!**/test' ] }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` at line 58, The code
uses __dirname when constructing frameworkDep in ServiceWorkerApp which breaks
in native ESM; import fileURLToPath from 'node:url' and compute a module-level
dirname via const __dirname = path.dirname(fileURLToPath(import.meta.url)), then
update the frameworkDep assignment (the variable named frameworkDep in
ServiceWorkerApp) to use that computed __dirname (path.join(__dirname, '..')) so
the constructor works under ESM.
Comment on lines
+16
to
+36
| static async getRequestBody(request: Request) { | ||
| if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.json)) { | ||
| return await request.json(); | ||
| } | ||
| if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.text)) { | ||
| return await request.text(); | ||
| } | ||
| if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.form)) { | ||
| return await request.formData(); | ||
| } | ||
| } | ||
|
|
||
| static matchContentTypes(request: Request, types: string[]) { | ||
| const contentType = request.headers.get('content-type'); | ||
| const contentTypeValue = typeof contentType === 'string' | ||
| // trim extra semicolon | ||
| ? contentType.replace(/;$/, '') | ||
| : contentType; | ||
|
|
||
| return typeis.is(contentTypeValue!, types); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid any return, missing explicit return types, and unsafe non-null assertion.
Several concerns on this helper:
getRequestBodyhas no explicit return type and implicitly returnsPromise<any>(viarequest.json()). Per guidelines, public APIs need explicit return types andanyshould be avoided — preferPromise<unknown>.matchContentTypeslacks an explicit return type (boolean | string | nullfromtypeis.is) — also, its return value is used as a truthy check ingetRequestBody, so narrowing semantics matter.contentTypeValue!asserts non-null, butheaders.get('content-type')returnsnullwhen absent. Passingnullintotypeis.isis undefined behavior and hides the real contract. Guard explicitly and returnfalse/undefinedwhen absent.ContentTypesis a mutablestatic— consideras const/readonlyto prevent accidental mutation.- Order note: JSON → text → form. If you ever add more permissive matchers, be aware that
type-ismatches by mime group, so order is fine today but worth a brief comment.
♻️ Proposed refactor
-export class RequestUtils {
- static ContentTypes = {
+export class RequestUtils {
+ static readonly ContentTypes = {
json: [
'application/json',
'application/json-patch+json',
'application/vnd.api+json',
'application/csp-report',
'application/scim+json',
],
form: [ 'application/x-www-form-urlencoded' ],
text: [ 'text/plain' ],
- };
+ } as const;
- static async getRequestBody(request: Request) {
+ static async getRequestBody(request: Request): Promise<unknown> {
if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.json)) {
return await request.json();
}
if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.text)) {
return await request.text();
}
if (RequestUtils.matchContentTypes(request, RequestUtils.ContentTypes.form)) {
return await request.formData();
}
+ return undefined;
}
- static matchContentTypes(request: Request, types: string[]) {
- const contentType = request.headers.get('content-type');
- const contentTypeValue = typeof contentType === 'string'
- // trim extra semicolon
- ? contentType.replace(/;$/, '')
- : contentType;
-
- return typeis.is(contentTypeValue!, types);
+ static matchContentTypes(request: Request, types: readonly string[]): boolean {
+ const contentType = request.headers.get('content-type');
+ if (!contentType) return false;
+ // trim trailing semicolon
+ const contentTypeValue = contentType.replace(/;$/, '');
+ return Boolean(typeis.is(contentTypeValue, types as string[]));
}
}As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations" and "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/utils/RequestUtils.ts` around lines 16 -
36, Update RequestUtils: add explicit return types — change getRequestBody to
return Promise<unknown | undefined> and matchContentTypes to return boolean.
Make ContentTypes readonly (e.g., as const) to prevent mutation. In
matchContentTypes, guard against missing header by checking
headers.get('content-type') and return false if null, then pass the trimmed
string to typeis.is and convert its result to a boolean (e.g., !!typeis.is(...))
instead of using a non-null assertion; update getRequestBody to use the
boolean-returning matchContentTypes to decide between request.json(),
request.text(), request.formData(), and return undefined if none match.
Comment on lines
+1
to
+40
| import { Server } from 'node:http'; | ||
| import httpRequest from 'supertest'; | ||
| import { ServiceWorkerApp } from '../../src/ServiceWorkerApp.ts'; | ||
| import { StandaloneTestUtil } from '@eggjs/module-test-util/StandaloneTestUtil'; | ||
| import { TestUtils } from '../Utils.ts'; | ||
|
|
||
| describe('standalone/service-worker/test/http/response.test.ts', () => { | ||
| let app: ServiceWorkerApp; | ||
| let server: Server; | ||
|
|
||
| before(async function() { | ||
| if (StandaloneTestUtil.skipOnNode()) { | ||
| return this.skip(); | ||
| } | ||
| ({ app, server } = await TestUtils.createFetchApp('http')); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| server?.close(); | ||
| await app?.destroy(); | ||
| }); | ||
|
|
||
| it('should return Response work', async () => { | ||
| await httpRequest(server) | ||
| .get('/api/response') | ||
| .expect(500, 'full response') | ||
| .expect('Content-Type', 'text/plain') | ||
| .expect('x-custom-header', 'custom-value'); | ||
| }); | ||
|
|
||
| it('should return 204 with no content', async () => { | ||
| await httpRequest(server) | ||
| .get('/api/null-body') | ||
| .expect(204, ''); | ||
|
|
||
| await httpRequest(server) | ||
| .get('/api/null-body?nil=1') | ||
| .expect(204, ''); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e ts . tegg/standalone -p 'test/.*\.test\.ts$' | head -20 | xargs -I {} sh -c 'echo "=== {} ==="; head -10 "{}"'Repository: eggjs/egg
Length of output: 8842
🏁 Script executed:
#!/bin/bash
# Find test files in tegg/standalone
find tegg/standalone -type f -name '*.test.ts' | head -10Repository: eggjs/egg
Length of output: 491
🏁 Script executed:
#!/bin/bash
# For each test file found, show the imports
find tegg/standalone -type f -name '*.test.ts' | head -10 | xargs -I {} sh -c 'echo "=== {} ==="; head -20 "{}"'Repository: eggjs/egg
Length of output: 6142
🏁 Script executed:
#!/bin/bash
# Double-check the full vitest config and whether service-worker tests are meant to use mocha
cd tegg/standalone && find . -name 'vitest.config.ts' -o -name 'vite.config.ts' -o -name 'package.json' | head -5 | xargs ls -la 2>/dev/nullRepository: eggjs/egg
Length of output: 554
🏁 Script executed:
#!/bin/bash
# Check if service-worker package has any test configuration
cat tegg/standalone/service-worker/package.json 2>/dev/null | grep -A 10 '"test"' || echo "No test script found"Repository: eggjs/egg
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check service-worker package.json content
cat tegg/standalone/service-worker/package.jsonRepository: eggjs/egg
Length of output: 1979
🏁 Script executed:
#!/bin/bash
# Check root vitest config in tegg/standalone
cat tegg/standalone/vitest.config.ts 2>/dev/null || cat tegg/vitest.config.ts 2>/dev/null || echo "No vitest config found in tegg/standalone"Repository: eggjs/egg
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Check root tegg vitest config
cat tegg/vitest.config.ts 2>/dev/null || cat vitest.config.ts 2>/dev/null | head -50Repository: eggjs/egg
Length of output: 1640
🏁 Script executed:
#!/bin/bash
# Check if there's a monorepo-level vitest config in root
ls -la vitest.config.ts pnpm-workspace.yaml 2>/dev/nullRepository: eggjs/egg
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Verify that tegg/standalone tests are indeed using global vitest scope by checking if describe/before/it/after are defined without imports
grep -n "describe\|before\|after\|it" tegg/standalone/service-worker/test/http/response.test.ts | head -5Repository: eggjs/egg
Length of output: 279
Use explicit Vitest imports and Node.js assert for test framework compliance.
The test file uses mocha-style describe/before/after/it in global scope with supertest assertions. Per coding guidelines for tegg packages, explicitly import test functions from vitest: import { describe, it, before, after } from 'vitest' and use assert from 'node:assert' for assertions. The tegg/standalone/standalone/test/index.test.ts file in the same monorepo follows this pattern correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/test/http/response.test.ts` around lines 1 -
40, The test file uses mocha-style globals; replace those with explicit Vitest
imports by adding import { describe, it, before, after } from 'vitest' at the
top and stop relying on global describe/before/after/it, and swap any assertions
to use Node's built-in assert by importing import assert from 'node:assert'
(update any expectation usages that depend on mocha globals to use supertest
chained expectations where appropriate and use assert for any direct value
checks); ensure the identifiers describe, it, before, after, and assert are
referenced in the file instead of implicit globals so the test matches the tegg
vitest pattern used by other tests like standalone/test/index.test.ts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tegg/core/standalone-decorator/src/event/EventHandler.ts (1)
4-20: 🛠️ Refactor suggestion | 🟠 MajorExisting exported API typing cleanup still applies.
This unresolved block still needs the previously requested changes: replace
anydefaults withunknown, typeEVENT_HANDLER_ATTRIBUTEasunique symbol, and add explicit return types toEventHandlerProtoand its returned decorator.As per coding guidelines, avoid
any, useunique symbolfor exported symbols, and give all exported functions explicit return type annotations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/standalone-decorator/src/event/EventHandler.ts` around lines 4 - 20, Replace the loose typings: change AbstractEventHandler generic defaults from any to unknown, declare EVENT_HANDLER_ATTRIBUTE as export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE'), and ensure EventHandler keeps its explicit type; add explicit return types to EventHandlerProto so its signature is (type: EventType[keyof EventType], params?: SingletonProtoParams) => (clazz: EggProtoImplClass<AbstractEventHandler>) => void, and update any internal anonymous decorator to return void — update references to these symbols (AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE, EventType, EventHandler, EventHandlerProto) accordingly.
🧹 Nitpick comments (2)
tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts (2)
3-5: ImportEGG_OBJECT_FACTORY_PROTO_IMPLE_TYPEfrom the package root instead of a deep internal path.
@eggjs/tegg-dynamic-inject-runtimepublicly exportsEGG_OBJECT_FACTORY_PROTO_IMPLE_TYPEfrom its entry point; change the import from@eggjs/tegg-dynamic-inject-runtime/src/EggObjectFactoryPrototypeto@eggjs/tegg-dynamic-inject-runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts` around lines 3 - 5, The import for EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE is using a deep internal path; update the import statement in StandaloneEggObjectFactory.ts to import EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE from the package root '@eggjs/tegg-dynamic-inject-runtime' instead of '@eggjs/tegg-dynamic-inject-runtime/src/EggObjectFactoryPrototype' so the module uses the public API surface (look for the existing import of EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE to replace).
7-10: Add explicit proto name for clarity.
ServiceWorkerRunnerinjectsstandaloneEggObjectFactoryby name, but this proto currently relies on the framework's default naming convention. Make the name explicit in the decorator so the contract is clear and not dependent on class-name normalization behavior.🔧 Suggested refactor
`@SingletonProto`({ protoImplType: EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE, + name: 'standaloneEggObjectFactory', accessLevel: AccessLevel.PRIVATE, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts` around lines 7 - 10, The SingletonProto decorator on StandaloneEggObjectFactory relies on default naming; add an explicit proto name so ServiceWorkerRunner's named injection works reliably. Update the SingletonProto options (the decorator that currently references EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE and accessLevel) to include a protoName (e.g., "standaloneEggObjectFactory") so the proto is registered under that explicit name and ServiceWorkerRunner can inject it by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts`:
- Around line 92-103: The public methods on ServiceWorkerApp need explicit
return types and the handleEvent parameter should be narrowed to the
FetchEventLike shape used by ServiceWorkerRunner/FetchEventHandler: update
ServiceWorkerApp.init to declare an explicit Promise<void> return type, change
ServiceWorkerApp.handleEvent(event: Event) to accept event: FetchEventLike and
declare its return type as Promise<T | undefined> (or another appropriate
generic Promise<T>), and add an explicit Promise<void> return type to
ServiceWorkerApp.destroy; keep the existing logic that creates StandaloneContext
and sets ContextProtoProperty.Event.contextKey before calling
this.runner.run<T>(context).
- Around line 57-83: The controller discovery hook can race with inner-class
prototype registration because LoadUnitInnerClassHook is registered after
ControllerLoadUnitHook; move creation and registration of
this.loadUnitInnerClassHook (which registers StandaloneEggObjectFactory,
ServiceWorkerRunner, FetchEventHandler) so it is registered before
ControllerLoadUnitHook (and before any call paths that lead to
LoadUnitLifecycleUtil.objectPostCreate()/ControllerLoadUnitHook.postCreate());
ensure innerObjectHandlers and frameworkDep setup remain intact but create new
LoadUnitInnerClassHook and call
LoadUnitLifecycleUtil.registerLifecycle(this.loadUnitInnerClassHook) prior to
registering this.controllerLoadUnitHook/this.contextProtoLoadUnitHook to
guarantee loadUnit.iterateEggPrototype() in ControllerLoadUnitHook.postCreate()
sees those prototypes.
---
Duplicate comments:
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts`:
- Around line 4-20: Replace the loose typings: change AbstractEventHandler
generic defaults from any to unknown, declare EVENT_HANDLER_ATTRIBUTE as export
const EVENT_HANDLER_ATTRIBUTE: unique symbol =
Symbol('EVENT_HANDLER_ATTRIBUTE'), and ensure EventHandler keeps its explicit
type; add explicit return types to EventHandlerProto so its signature is (type:
EventType[keyof EventType], params?: SingletonProtoParams) => (clazz:
EggProtoImplClass<AbstractEventHandler>) => void, and update any internal
anonymous decorator to return void — update references to these symbols
(AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE, EventType, EventHandler,
EventHandlerProto) accordingly.
---
Nitpick comments:
In `@tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts`:
- Around line 3-5: The import for EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE is using a
deep internal path; update the import statement in StandaloneEggObjectFactory.ts
to import EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE from the package root
'@eggjs/tegg-dynamic-inject-runtime' instead of
'@eggjs/tegg-dynamic-inject-runtime/src/EggObjectFactoryPrototype' so the module
uses the public API surface (look for the existing import of
EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE to replace).
- Around line 7-10: The SingletonProto decorator on StandaloneEggObjectFactory
relies on default naming; add an explicit proto name so ServiceWorkerRunner's
named injection works reliably. Update the SingletonProto options (the decorator
that currently references EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE and accessLevel)
to include a protoName (e.g., "standaloneEggObjectFactory") so the proto is
registered under that explicit name and ServiceWorkerRunner can inject it by
name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f17cc7d7-41fe-44b2-bb4f-a51c0c254d4e
📒 Files selected for processing (5)
tegg/core/standalone-decorator/src/event/EventHandler.tstegg/standalone/service-worker/src/ServiceWorkerApp.tstegg/standalone/service-worker/src/ServiceWorkerRunner.tstegg/standalone/service-worker/src/StandaloneEggObjectFactory.tstegg/standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts
Comment on lines
+92
to
+103
| async init() { | ||
| await this.runner.init(); | ||
| } | ||
|
|
||
| async handleEvent<T = unknown>(event: Event) { | ||
| const context = new StandaloneContext(); | ||
| context.set(ContextProtoProperty.Event.contextKey, event); | ||
|
|
||
| return await this.runner.run<T>(context); | ||
| } | ||
|
|
||
| async destroy() { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add public return types and narrow the event parameter.
init, handleEvent, and destroy are public methods on an exported class, but they rely on inferred return types. Also, handleEvent(event: Event) is broader than the FetchEventLike shape consumed by ServiceWorkerRunner and FetchEventHandler.
🔧 Proposed typing fix
import { FetchEventHandler } from './http/FetchEventHandler.ts';
+import type { FetchEventLike } from '@eggjs/tegg/standalone';
@@
- async init() {
+ async init(): Promise<void> {
await this.runner.init();
}
- async handleEvent<T = unknown>(event: Event) {
+ async handleEvent<T = unknown>(event: FetchEventLike): Promise<T> {
const context = new StandaloneContext();
context.set(ContextProtoProperty.Event.contextKey, event);
return await this.runner.run<T>(context);
}
- async destroy() {
+ async destroy(): Promise<void> {As per coding guidelines, all exported functions, methods, and getters must have explicit return type annotations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async init() { | |
| await this.runner.init(); | |
| } | |
| async handleEvent<T = unknown>(event: Event) { | |
| const context = new StandaloneContext(); | |
| context.set(ContextProtoProperty.Event.contextKey, event); | |
| return await this.runner.run<T>(context); | |
| } | |
| async destroy() { | |
| async init(): Promise<void> { | |
| await this.runner.init(); | |
| } | |
| async handleEvent<T = unknown>(event: FetchEventLike): Promise<T> { | |
| const context = new StandaloneContext(); | |
| context.set(ContextProtoProperty.Event.contextKey, event); | |
| return await this.runner.run<T>(context); | |
| } | |
| async destroy(): Promise<void> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` around lines 92 -
103, The public methods on ServiceWorkerApp need explicit return types and the
handleEvent parameter should be narrowed to the FetchEventLike shape used by
ServiceWorkerRunner/FetchEventHandler: update ServiceWorkerApp.init to declare
an explicit Promise<void> return type, change
ServiceWorkerApp.handleEvent(event: Event) to accept event: FetchEventLike and
declare its return type as Promise<T | undefined> (or another appropriate
generic Promise<T>), and add an explicit Promise<void> return type to
ServiceWorkerApp.destroy; keep the existing logic that creates StandaloneContext
and sets ContextProtoProperty.Event.contextKey before calling
this.runner.run<T>(context).
akitaSummer
force-pushed
the
pick/tegg-service-worker
branch
from
aff770d to
c46b053
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5875 +/- ##
==========================================
+ Coverage 85.48% 85.96% +0.47%
==========================================
Files 660 9 -651
Lines 18828 57 -18771
Branches 3646 11 -3635
==========================================
- Hits 16096 49 -16047
+ Misses 2361 7 -2354
+ Partials 371 1 -370 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
akitaSummer
force-pushed
the
pick/tegg-service-worker
branch
from
c46b053 to
fc6e1c1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
tegg/standalone/service-worker/src/ServiceWorkerApp.ts (3)
92-103: 🛠️ Refactor suggestion | 🟠 MajorAdd explicit return types and narrow
handleEvent's event param.Public methods
init,handleEvent, anddestroyrely on inferred return types, andevent: Eventis broader than theFetchEventLikecontract consumed downstream byServiceWorkerRunner/FetchEventHandler.🔧 Proposed fix
- async init() { + async init(): Promise<void> { await this.runner.init(); } - async handleEvent<T = unknown>(event: Event) { + async handleEvent<T = unknown>(event: FetchEventLike): Promise<T> { const context = new StandaloneContext(); context.set(ContextProtoProperty.Event.contextKey, event); return await this.runner.run<T>(context); } - async destroy() { + async destroy(): Promise<void> {As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` around lines 92 - 103, Methods init, handleEvent, and destroy lack explicit return types and handleEvent's parameter is too broad: change init(): Promise<void> and destroy(): Promise<void>, and change handleEvent<T = unknown>(event: FetchEventLike): Promise<T> (or Promise<unknown> if generic result not required), updating the import to bring in the FetchEventLike type and keep usage of StandaloneContext and ContextProtoProperty.Event.contextKey the same so ServiceWorkerRunner/FetchEventHandler get the narrowed event type.
63-63:⚠️ Potential issue | 🔴 Critical
__dirnameis not defined in native ESM.This package is
"type": "module"and tegg guidelines mandate ESM-only, so__dirnamehere will throw aReferenceErrorat construction time. Derive it fromimport.meta.url:🔧 Proposed fix
import path from 'node:path'; +import { fileURLToPath } from 'node:url'; @@ +const __dirname = path.dirname(fileURLToPath(import.meta.url));As per coding guidelines: "Use ESM only (no CommonJS)" for
tegg/**/*.{js,ts,mjs,cjs}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` at line 63, The code uses __dirname in ServiceWorkerApp.ts when constructing frameworkDep which will throw in ESM; replace the __dirname usage by deriving the directory from import.meta.url (e.g., using new URL(import.meta.url) and path utilities to compute the directory) and update the frameworkDep initialization so baseDir = path.join(<derivedDir>, '..') while keeping extraFilePattern unchanged; ensure this change targets the frameworkDep constant in ServiceWorkerApp.ts and removes any reliance on __dirname.
57-83:⚠️ Potential issue | 🟠 MajorHook registration order can race with controller discovery.
ControllerLoadUnitHook(registered at Line 59) iteratesloadUnit.iterateEggPrototype()inpostCreate, whileLoadUnitInnerClassHook(registered at Line 83) registersServiceWorkerRunner/FetchEventHandler/StandaloneEggObjectFactoryprototypes in its ownpostCreate.LoadUnitLifecycleUtilinvokes post-create hooks viaPromise.all, so the controller hook may finish iteration before the inner-class hook has registered its prototypes, causing controllers defined on those inner classes to be silently missed. RegisterloadUnitInnerClassHookbeforecontrollerLoadUnitHook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` around lines 57 - 83, The controller discovery can race with inner-class prototype registration because ControllerLoadUnitHook (controllerLoadUnitHook) calls loadUnit.iterateEggPrototype() in its postCreate while LoadUnitInnerClassHook (loadUnitInnerClassHook / class LoadUnitInnerClassHook) registers ServiceWorkerRunner/FetchEventHandler/StandaloneEggObjectFactory prototypes in its postCreate; to fix, register the loadUnitInnerClassHook with LoadUnitLifecycleUtil.registerLifecycle(...) before registering controllerLoadUnitHook so inner-class prototypes are in place prior to controller iteration.tegg/core/standalone-decorator/src/event/EventHandler.ts (1)
4-20:⚠️ Potential issue | 🟠 MajorTighten exported API typings (still unresolved from earlier feedback).
At Lines 4-20:
anydefaults, bareSymbol()export, and missing explicit return types on exported decorator factory should be corrected for guideline compliance and declaration safety.🔧 Proposed fix
-export abstract class AbstractEventHandler<E = any, R = any> { +export abstract class AbstractEventHandler<E = unknown, R = unknown> { abstract handleEvent(event: E): Promise<R>; } -export const EVENT_HANDLER_ATTRIBUTE = Symbol('EVENT_HANDLER_ATTRIBUTE'); +export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE'); @@ -export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams) => { - return (clazz: EggProtoImplClass<AbstractEventHandler>) => { +export const EventHandlerProto = ( + type: EventType[keyof EventType], + params?: SingletonProtoParams, +): ((clazz: EggProtoImplClass<AbstractEventHandler>) => void) => { + return (clazz: EggProtoImplClass<AbstractEventHandler>): void => { EventHandler(type)(clazz); SingletonProto(params)(clazz); }; };As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown", "Use 'unique symbol' type annotation for exported symbols instead of bare Symbol() declarations", and "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/standalone-decorator/src/event/EventHandler.ts` around lines 4 - 20, The exported API uses unsafe any defaults, a plain Symbol export, and lacks explicit return types; update AbstractEventHandler's generics to default to unknown (AbstractEventHandler<E = unknown, R = unknown>), change EVENT_HANDLER_ATTRIBUTE to a typed unique symbol export (export const EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE')), and add explicit return type annotations for the exported decorators — ensure EventHandler is declared with its full ImplDecorator type return and EventHandlerProto explicitly returns a ClassDecorator/void (e.g., const EventHandler: ImplDecorator<AbstractEventHandler, EventType> = ... and export const EventHandlerProto = (type: EventType[keyof EventType], params?: SingletonProtoParams): ClassDecorator => { ... }) while keeping references to AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE, EventHandler, and EventHandlerProto to locate the changes.tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts (3)
41-50:⚠️ Potential issue | 🟠 MajorAvoid invoking stream callbacks twice.
writeandendpass the caller callback to bothsuperand#stream; Node stream callbacks should be observed once by the caller.🐛 Proposed fix
- super.write(chunk, encoding as any, callback); + super.write(chunk, encoding as any); return this.#stream.write(chunk, encoding as any, callback); @@ end(...args: any[]): this { this.#stream.end(...args); - super.end(...args); + const argsWithoutCallback = + typeof args.at(-1) === 'function' ? args.slice(0, -1) : args; + super.end(...argsWithoutCallback); return this; }Also applies to: 95-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines 41 - 50, The write and end methods (in MCPControllerRegister) currently forward the same caller callback to both super.write/super.end and this.#stream.write/this.#stream.end, causing the callback to be invoked twice; fix by ensuring the caller callback is passed to only one side: invoke super.write/super.end without a callback (or pass an internal wrapper that only forwards errors once) and call this.#stream.write/this.#stream.end with the original callback (or vice versa), so that either super or `#stream` handles the caller callback but not both; update both write(...) and end(...) implementations to stop forwarding the raw caller callback to both super and `#stream`.
105-144:⚠️ Potential issue | 🟠 MajorAvoid the process-global MCP controller register.
static instancemakes latercreate()calls reuse the firstcontrollerMetaand can leak registrations across multipleServiceWorkerAppinstances or tests.Prefer owning
MCPControllerRegisterfrom the application instance and disposing it through the app lifecycle instead ofstatic create()/static clean().As per coding guidelines: "Avoid global state in worker processes; use the Application instance for shared state instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines 105 - 144, Replace the process-global singleton pattern: remove the static instance property and the static create() and clean() methods and instead have callers instantiate MCPControllerRegister via its constructor (new MCPControllerRegister(controllerMeta, router)) and manage its lifecycle on the ServiceWorkerApp; move any logic that pushed to controllerProtos from create() into an instance method (e.g., addControllerProto(proto)) and expose an instance cleanup method (e.g., dispose()) that clears controllerProtos, registeredControllerProtos, registerMap, mcpServerHelperMap and middlewaresMap; update all call sites that used MCPControllerRegister.create(...) or MCPControllerRegister.clean() to use the app-owned instance, call addControllerProto(...) and dispose() so state no longer leaks across apps or tests.
223-239:⚠️ Potential issue | 🔴 CriticalWire
handleRequestfailures into the response promise.
transport.handleRequest(...)is fire-and-forget; if it rejects or never callswriteHead, this handler can hang indefinitely waiting onresPromise.🐛 Proposed fix sketch
- const resPromise = new Promise<object>(resolve => { + const resPromise = new Promise<object>((resolve, reject) => { callback = resolve; + response.stream.once('error', reject); + response.stream.once('close', () => reject(new Error('MCP response stream closed before headers were written'))); }); @@ - transport.handleRequest(req, response, body); + void transport.handleRequest(req, response, body).catch(error => { + response.stream.destroy(error); + }); - const init = await resPromise; + const init = await resPromise;Please also return a bounded error
Responseon rejection so the fetch route never waits forever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines 223 - 239, The current handler can hang because transport.handleRequest(req, response, body) is fire-and-forget and resPromise only resolves when ServiceWorkerMCPServerResponse calls writeHead; fix by wiring handleRequest failures and a bounded timeout into resPromise: capture the Promise (if any) returned by transport.handleRequest and attach .catch to resolve the response promise with a 500-style init and end/close the response.stream; also race resPromise against a short timeout (e.g., 5s) that resolves with a bounded error init and closes response.stream to ensure ctx.response is always set. Update the block around resPromise/response creation (symbols: resPromise, callback, ServiceWorkerMCPServerResponse, transport.handleRequest, response.stream, ctx.response) to implement try/catch for JSON parsing, attach rejection handling to handleRequest, and use Promise.race to provide a fallback error Response on rejection or timeout.tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts (1)
79-89:⚠️ Potential issue | 🔴 CriticalFix argument remapping before invoking tool and prompt handlers.
extrais a numeric index, so0is currently skipped, andnewArgs = [ ...newArgs, ...args ]duplicates the original MCP callback args after the remapped positions.🐛 Proposed fix
- let newArgs: any[] = []; + const newArgs: unknown[] = []; if (schema && toolMeta.detail) { newArgs[toolMeta.detail.index] = args[0]; - if (toolMeta.extra) { + if (toolMeta.extra !== undefined) { newArgs[toolMeta.extra] = args[1]; } - } else if (toolMeta.extra) { + } else if (toolMeta.extra !== undefined) { newArgs[toolMeta.extra] = args[0]; } - newArgs = [ ...newArgs, ...args ]; return Reflect.apply(realMethod, realObj, newArgs) as ReturnType<ToolCallback>; @@ - let newArgs: any[] = []; + const newArgs: unknown[] = []; if (schema && promptMeta.detail) { newArgs[promptMeta.detail.index] = args[0]; - if (promptMeta.extra) { + if (promptMeta.extra !== undefined) { newArgs[promptMeta.extra] = args[1]; } - } else if (promptMeta.extra) { + } else if (promptMeta.extra !== undefined) { newArgs[promptMeta.extra] = args[0]; } - newArgs = [ ...newArgs, ...args ]; return Reflect.apply(realMethod, realObj, newArgs) as ReturnType<PromptCallback>;Also applies to: 112-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts` around lines 79 - 89, The remapping logic wrongly treats numeric indices like 0 as falsy and appends args again causing duplicates; change the conditional checks from truthy (if (toolMeta.extra) / if (toolMeta.detail)) to explicit undefined checks (if (toolMeta.extra !== undefined) / if (toolMeta.detail !== undefined)), and replace the final "newArgs = [...newArgs, ...args]" step with a merge that starts from a copy of the original args (e.g., finalArgs = args.slice()) and then assigns any remapped positions from newArgs into finalArgs by index before calling Reflect.apply(realMethod, realObj, finalArgs); make the same edits for the second occurrence around the 112-122 block.
🧹 Nitpick comments (1)
tegg/core/standalone-decorator/src/event/EventHandler.ts (1)
10-10: Prefer an interface forEventTypeat Line 10.
Record<string, string>here is an object shape; guideline preference is interface for exported object shapes.🔧 Proposed refactor
-export type EventType = Record<string, string>; +export interface EventType { + [key: string]: string; +}As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/standalone-decorator/src/event/EventHandler.ts` at line 10, Replace the exported type alias EventType with an exported interface to match the coding guideline; specifically change the declaration for EventType (currently "export type EventType = Record<string, string>") to an interface (e.g., "export interface EventType { [key: string]: string }"), update any imports/usages if necessary, and ensure the exported symbol name remains EventType so existing consumers continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts`:
- Line 1: The import currently marks only EggProtoImplClass as a type,
triggering TS1484 under verbatimModuleSyntax; update the import to use type-only
imports for the exported types by marking ImplDecorator and SingletonProtoParams
as type-only as well (e.g., change the clause to import type {
EggProtoImplClass, ImplDecorator, SingletonProtoParams } from
'@eggjs/tegg-types' or prefix each with type), so adjust the import line that
references EggProtoImplClass, ImplDecorator, and SingletonProtoParams in
EventHandler.ts accordingly.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts`:
- Line 121: MCPControllerRegister is an exported class whose public methods
(including the static create method) lack explicit return type annotations;
update the declaration of create(proto: EggPrototype, controllerMeta:
ControllerMetadata, router: KoaRouter) to include an explicit return type (e.g.,
: void or : Promise<void> as appropriate) and add explicit return types to any
other exported/public methods on MCPControllerRegister so the class API is fully
typed; ensure the chosen types accurately reflect whether the method is
synchronous or returns a Promise.
- Around line 18-22: Update ESM imports to use .js specifiers and mark
interface-only imports with the type keyword: change imports for
AbstractControllerAdvice, ControllerRegister, ServiceWorkerFetchContext,
MCPServerHelper (and any other relative imports in this file) from './X.ts' to
'./X.js' and import ControllerRegister as "type { ControllerRegister }". Also
add explicit return type annotations to the public/static methods: declare
static create(): MCPControllerRegister, static clean(): void, async
connectStatelessStreamTransport(): Promise<void>, async register():
Promise<void>, and async doRegister(): Promise<void> so the signatures for
create, clean, connectStatelessStreamTransport, register, and doRegister are
explicitly typed.
---
Duplicate comments:
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts`:
- Around line 4-20: The exported API uses unsafe any defaults, a plain Symbol
export, and lacks explicit return types; update AbstractEventHandler's generics
to default to unknown (AbstractEventHandler<E = unknown, R = unknown>), change
EVENT_HANDLER_ATTRIBUTE to a typed unique symbol export (export const
EVENT_HANDLER_ATTRIBUTE: unique symbol = Symbol('EVENT_HANDLER_ATTRIBUTE')), and
add explicit return type annotations for the exported decorators — ensure
EventHandler is declared with its full ImplDecorator type return and
EventHandlerProto explicitly returns a ClassDecorator/void (e.g., const
EventHandler: ImplDecorator<AbstractEventHandler, EventType> = ... and export
const EventHandlerProto = (type: EventType[keyof EventType], params?:
SingletonProtoParams): ClassDecorator => { ... }) while keeping references to
AbstractEventHandler, EVENT_HANDLER_ATTRIBUTE, EventHandler, and
EventHandlerProto to locate the changes.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts`:
- Around line 41-50: The write and end methods (in MCPControllerRegister)
currently forward the same caller callback to both super.write/super.end and
this.#stream.write/this.#stream.end, causing the callback to be invoked twice;
fix by ensuring the caller callback is passed to only one side: invoke
super.write/super.end without a callback (or pass an internal wrapper that only
forwards errors once) and call this.#stream.write/this.#stream.end with the
original callback (or vice versa), so that either super or `#stream` handles the
caller callback but not both; update both write(...) and end(...)
implementations to stop forwarding the raw caller callback to both super and
`#stream`.
- Around line 105-144: Replace the process-global singleton pattern: remove the
static instance property and the static create() and clean() methods and instead
have callers instantiate MCPControllerRegister via its constructor (new
MCPControllerRegister(controllerMeta, router)) and manage its lifecycle on the
ServiceWorkerApp; move any logic that pushed to controllerProtos from create()
into an instance method (e.g., addControllerProto(proto)) and expose an instance
cleanup method (e.g., dispose()) that clears controllerProtos,
registeredControllerProtos, registerMap, mcpServerHelperMap and middlewaresMap;
update all call sites that used MCPControllerRegister.create(...) or
MCPControllerRegister.clean() to use the app-owned instance, call
addControllerProto(...) and dispose() so state no longer leaks across apps or
tests.
- Around line 223-239: The current handler can hang because
transport.handleRequest(req, response, body) is fire-and-forget and resPromise
only resolves when ServiceWorkerMCPServerResponse calls writeHead; fix by wiring
handleRequest failures and a bounded timeout into resPromise: capture the
Promise (if any) returned by transport.handleRequest and attach .catch to
resolve the response promise with a 500-style init and end/close the
response.stream; also race resPromise against a short timeout (e.g., 5s) that
resolves with a bounded error init and closes response.stream to ensure
ctx.response is always set. Update the block around resPromise/response creation
(symbols: resPromise, callback, ServiceWorkerMCPServerResponse,
transport.handleRequest, response.stream, ctx.response) to implement try/catch
for JSON parsing, attach rejection handling to handleRequest, and use
Promise.race to provide a fallback error Response on rejection or timeout.
In `@tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts`:
- Around line 79-89: The remapping logic wrongly treats numeric indices like 0
as falsy and appends args again causing duplicates; change the conditional
checks from truthy (if (toolMeta.extra) / if (toolMeta.detail)) to explicit
undefined checks (if (toolMeta.extra !== undefined) / if (toolMeta.detail !==
undefined)), and replace the final "newArgs = [...newArgs, ...args]" step with a
merge that starts from a copy of the original args (e.g., finalArgs =
args.slice()) and then assigns any remapped positions from newArgs into
finalArgs by index before calling Reflect.apply(realMethod, realObj, finalArgs);
make the same edits for the second occurrence around the 112-122 block.
In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts`:
- Around line 92-103: Methods init, handleEvent, and destroy lack explicit
return types and handleEvent's parameter is too broad: change init():
Promise<void> and destroy(): Promise<void>, and change handleEvent<T =
unknown>(event: FetchEventLike): Promise<T> (or Promise<unknown> if generic
result not required), updating the import to bring in the FetchEventLike type
and keep usage of StandaloneContext and ContextProtoProperty.Event.contextKey
the same so ServiceWorkerRunner/FetchEventHandler get the narrowed event type.
- Line 63: The code uses __dirname in ServiceWorkerApp.ts when constructing
frameworkDep which will throw in ESM; replace the __dirname usage by deriving
the directory from import.meta.url (e.g., using new URL(import.meta.url) and
path utilities to compute the directory) and update the frameworkDep
initialization so baseDir = path.join(<derivedDir>, '..') while keeping
extraFilePattern unchanged; ensure this change targets the frameworkDep constant
in ServiceWorkerApp.ts and removes any reliance on __dirname.
- Around line 57-83: The controller discovery can race with inner-class
prototype registration because ControllerLoadUnitHook (controllerLoadUnitHook)
calls loadUnit.iterateEggPrototype() in its postCreate while
LoadUnitInnerClassHook (loadUnitInnerClassHook / class LoadUnitInnerClassHook)
registers ServiceWorkerRunner/FetchEventHandler/StandaloneEggObjectFactory
prototypes in its postCreate; to fix, register the loadUnitInnerClassHook with
LoadUnitLifecycleUtil.registerLifecycle(...) before registering
controllerLoadUnitHook so inner-class prototypes are in place prior to
controller iteration.
---
Nitpick comments:
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts`:
- Line 10: Replace the exported type alias EventType with an exported interface
to match the coding guideline; specifically change the declaration for EventType
(currently "export type EventType = Record<string, string>") to an interface
(e.g., "export interface EventType { [key: string]: string }"), update any
imports/usages if necessary, and ensure the exported symbol name remains
EventType so existing consumers continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19a69a44-e63c-4436-90e7-de6c30f51105
📒 Files selected for processing (9)
tegg/core/standalone-decorator/src/event/EventHandler.tstegg/standalone/service-worker/package.jsontegg/standalone/service-worker/src/ServiceWorkerApp.tstegg/standalone/service-worker/src/ServiceWorkerRunner.tstegg/standalone/service-worker/src/StandaloneEggObjectFactory.tstegg/standalone/service-worker/src/hook/LoadUnitInnerClassHook.tstegg/standalone/service-worker/src/http/FetchEventHandler.tstegg/standalone/service-worker/src/mcp/MCPControllerRegister.tstegg/standalone/service-worker/src/mcp/MCPServerHelper.ts
✅ Files skipped from review due to trivial changes (2)
- tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts
- tegg/standalone/service-worker/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tegg/standalone/service-worker/src/ServiceWorkerRunner.ts
- tegg/standalone/service-worker/src/http/FetchEventHandler.ts
| @@ -0,0 +1,27 @@ | |||
| import { type EggProtoImplClass, ImplDecorator, SingletonProtoParams } from '@eggjs/tegg-types'; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TS1484 risk and import style usage in this file + tsconfig flag presence.
rg -n "verbatimModuleSyntax" --iglob "tsconfig*.json"
rg -n "from '@eggjs/tegg-types'" tegg/core/standalone-decorator/src/event/EventHandler.tsRepository: eggjs/egg
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Find where ImplDecorator and SingletonProtoParams are defined in `@eggjs/tegg-types`
find . -type f -name "*.ts" -path "*tegg-types*" | head -20Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Read the EventHandler.ts file to verify all issues
cat -n tegg/core/standalone-decorator/src/event/EventHandler.tsRepository: eggjs/egg
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Find the `@eggjs/tegg-types` package and check exports
find . -type f -name "*.ts" | xargs grep -l "export.*ImplDecorator" | grep -v node_modules | head -5Repository: eggjs/egg
Length of output: 654
🏁 Script executed:
#!/bin/bash
# Search for SingletonProtoParams export
find . -type f -name "*.ts" | xargs grep -l "export.*SingletonProtoParams" | grep -v node_modules | head -5Repository: eggjs/egg
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Find where `@eggjs/tegg-types` is defined/exported
find . -type f -name "*.ts" -path "*tegg*" | xargs grep -l "export.*type.*ImplDecorator\|export.*interface.*ImplDecorator" | grep -v node_modules | grep -v test | head -10Repository: eggjs/egg
Length of output: 92
🏁 Script executed:
#!/bin/bash
# List tegg packages to find tegg-types
ls -la tegg/*/package.json | grep -i typesRepository: eggjs/egg
Length of output: 119
🏁 Script executed:
#!/bin/bash
# List tegg directory structure
ls -la tegg/Repository: eggjs/egg
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Check the types package
cat tegg/core/types/src/dynamic-inject.ts | head -80Repository: eggjs/egg
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Search for SingletonProtoParams definition
rg -n "export.*SingletonProtoParams" --type ts tegg/Repository: eggjs/egg
Length of output: 1098
🏁 Script executed:
#!/bin/bash
# Check the entire dynamic-inject.ts file for SingletonProtoParams
cat tegg/core/types/src/dynamic-inject.ts | grep -A 5 "SingletonProtoParams"Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check the SingletonProtoParams definition
cat tegg/core/types/src/core-decorator/SingletonProto.tsRepository: eggjs/egg
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Check the full types index to see what's exported from `@eggjs/tegg-types`
find tegg/core/types -name "index.ts" -o -name "*.ts" | xargs grep -l "export.*ImplDecorator\|export.*SingletonProtoParams" | head -5Repository: eggjs/egg
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check what tegg-types actually exports (check its index)
find tegg/core/types -name "index.ts" -exec cat {} \; | head -100Repository: eggjs/egg
Length of output: 3523
🏁 Script executed:
#!/bin/bash
# Verify the package name in tegg/core/types
cat tegg/core/types/package.json | grep -E '"name"|"exports"' -A 5Repository: eggjs/egg
Length of output: 841
Fix TS1484 by using type-only imports at Line 1.
ImplDecorator and SingletonProtoParams are types exported from @eggjs/tegg-types. With verbatimModuleSyntax: true in root tsconfig.json, type-only imports require the type keyword.
🔧 Proposed fix
-import { type EggProtoImplClass, ImplDecorator, SingletonProtoParams } from '@eggjs/tegg-types';
+import { type EggProtoImplClass, type ImplDecorator, type SingletonProtoParams } from '@eggjs/tegg-types';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { type EggProtoImplClass, ImplDecorator, SingletonProtoParams } from '@eggjs/tegg-types'; | |
| import { type EggProtoImplClass, type ImplDecorator, type SingletonProtoParams } from '@eggjs/tegg-types'; |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 1-1: typescript(TS1484)
'SingletonProtoParams' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 1-1: typescript(TS1484)
'ImplDecorator' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/standalone-decorator/src/event/EventHandler.ts` at line 1, The
import currently marks only EggProtoImplClass as a type, triggering TS1484 under
verbatimModuleSyntax; update the import to use type-only imports for the
exported types by marking ImplDecorator and SingletonProtoParams as type-only as
well (e.g., change the clause to import type { EggProtoImplClass, ImplDecorator,
SingletonProtoParams } from '@eggjs/tegg-types' or prefix each with type), so
adjust the import line that references EggProtoImplClass, ImplDecorator, and
SingletonProtoParams in EventHandler.ts accordingly.
Comment on lines
+18
to
+22
| import type { AbstractControllerAdvice } from './AbstractControllerAdvice.ts'; | ||
| import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; | ||
| import { ControllerRegister } from '../controller/ControllerRegister.ts'; | ||
| import { ServiceWorkerFetchContext } from '../http/ServiceWorkerFetchContext.ts'; | ||
| import { MCPServerHelper } from './MCPServerHelper.ts'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify interface-only export and ESM-related TS config.
fd -a 'ControllerRegister\.ts$' . --exec sed -n '1,20p' {}
fd -a 'MCPControllerRegister\.ts$' . --exec sed -n '18,22p' {}
fd -a 'tsconfig.*\.json$' . --exec sh -c '
printf "\n== %s ==\n" "$1"
jq -r ".compilerOptions | {module, moduleResolution, verbatimModuleSyntax, isolatedDeclarations, allowImportingTsExtensions}" "$1" 2>/dev/null || true
' sh {}Repository: eggjs/egg
Length of output: 12942
🏁 Script executed:
#!/bin/bash
# Read full MCPControllerRegister.ts to verify return types and imports
echo "=== MCPControllerRegister.ts ==="
wc -l tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts
echo ""
cat -n tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | head -50
echo "..."
echo "=== Lines around 121, 135, 151, 283, 357 ==="
sed -n '115,130p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo "---"
sed -n '130,145p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo "---"
sed -n '145,160p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -nRepository: eggjs/egg
Length of output: 4895
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json and verify exported types
echo "=== tegg/standalone/service-worker/tsconfig.json ==="
cat tegg/standalone/service-worker/tsconfig.json | jq '.compilerOptions'
echo ""
echo "=== Check ControllerRegister exports ==="
rg -A 5 'export interface ControllerRegister' tegg/standalone/service-worker/src/
echo ""
echo "=== Check AbstractControllerAdvice exports ==="
rg -A 3 'export.*AbstractControllerAdvice' tegg/standalone/service-worker/src/
echo ""
echo "=== Check ServiceWorkerFetchContext exports ==="
rg -A 3 'export.*ServiceWorkerFetchContext' tegg/standalone/service-worker/src/Repository: eggjs/egg
Length of output: 2120
🏁 Script executed:
#!/bin/bash
# Get exact method signatures at specified lines
echo "=== Line 121 context ==="
sed -n '120,125p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo ""
echo "=== Line 135 context ==="
sed -n '134,139p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo ""
echo "=== Line 151 context ==="
sed -n '150,155p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo ""
echo "=== Line 283 context ==="
sed -n '282,287p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -n
echo ""
echo "=== Line 357 context ==="
sed -n '356,361p' tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | cat -nRepository: eggjs/egg
Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Get tsconfig.json content and check MCPServerHelper
echo "=== tsconfig.json content ==="
cat tegg/standalone/service-worker/tsconfig.json
echo ""
echo "=== MCPServerHelper exports ==="
rg -A 3 'export.*MCPServerHelper' tegg/standalone/service-worker/src/Repository: eggjs/egg
Length of output: 891
Use ESM .js specifiers for all relative imports and add type keyword for interface-only imports.
Lines 18–22 use .ts extensions instead of .js (violates ESM import guideline). Additionally, ControllerRegister is interface-only and should be imported with type keyword; the others are classes but still need .js extensions for consistency.
Multiple public/static methods lack explicit return type annotations:
- Line 121:
static create()missing return type (should beMCPControllerRegister) - Line 135:
static clean()missing return type (should bevoid) - Line 151:
async connectStatelessStreamTransport()missing return type (should bePromise<void>) - Line 283:
async register()missing return type (should bePromise<void>) - Line 357:
async doRegister()missing return type (should bePromise<void>)
♻️ Proposed fix
-import type { AbstractControllerAdvice } from './AbstractControllerAdvice.ts';
+import type { AbstractControllerAdvice } from './AbstractControllerAdvice.js';
import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
-import { ControllerRegister } from '../controller/ControllerRegister.ts';
-import { ServiceWorkerFetchContext } from '../http/ServiceWorkerFetchContext.ts';
-import { MCPServerHelper } from './MCPServerHelper.ts';
+import type { ControllerRegister } from '../controller/ControllerRegister.js';
+import type { ServiceWorkerFetchContext } from '../http/ServiceWorkerFetchContext.js';
+import { MCPServerHelper } from './MCPServerHelper.js';For return type annotations, add explicit types to: static create(), static clean(), async connectStatelessStreamTransport(), async register(), async doRegister().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines
18 - 22, Update ESM imports to use .js specifiers and mark interface-only
imports with the type keyword: change imports for AbstractControllerAdvice,
ControllerRegister, ServiceWorkerFetchContext, MCPServerHelper (and any other
relative imports in this file) from './X.ts' to './X.js' and import
ControllerRegister as "type { ControllerRegister }". Also add explicit return
type annotations to the public/static methods: declare static create():
MCPControllerRegister, static clean(): void, async
connectStatelessStreamTransport(): Promise<void>, async register():
Promise<void>, and async doRegister(): Promise<void> so the signatures for
create, clean, connectStatelessStreamTransport, register, and doRegister are
explicitly typed.
| resources: ServerRegisterRecord<MCPResourceMeta>[]; | ||
| }> = {}; | ||
|
|
||
| static create(proto: EggPrototype, controllerMeta: ControllerMetadata, router: KoaRouter) { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify exported method signatures and declaration-related TS flags.
fd -a 'MCPControllerRegister\.ts$' . --exec sed -n '105,365p' {}
fd -a 'tsconfig.*\.json$' . --exec sh -c '
printf "\n== %s ==\n" "$1"
jq -r ".compilerOptions | {strict, isolatedDeclarations, declaration, verbatimModuleSyntax}" "$1" 2>/dev/null || true
' sh {}Repository: eggjs/egg
Length of output: 40439
🏁 Script executed:
cat -n tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | sed -n '115,160p; 280,290p'Repository: eggjs/egg
Length of output: 2666
🏁 Script executed:
cat -n tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | sed -n '350,365p'Repository: eggjs/egg
Length of output: 555
🏁 Script executed:
grep -n "^export" tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | head -5Repository: eggjs/egg
Length of output: 123
Add explicit return types to the exported class methods.
The class MCPControllerRegister is exported and these methods are part of its public API. All must have explicit return type annotations per project guidelines for TypeScript files.
♻️ Proposed fix
- static create(proto: EggPrototype, controllerMeta: ControllerMetadata, router: KoaRouter) {
+ static create(proto: EggPrototype, controllerMeta: ControllerMetadata, router: KoaRouter): MCPControllerRegister {
@@
- static clean() {
+ static clean(): void {
@@
- async connectStatelessStreamTransport(name?: string) {
+ async connectStatelessStreamTransport(name?: string): Promise<void> {
@@
- async register() {
+ async register(): Promise<void> {
@@
- async doRegister() {
+ async doRegister(): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` at line 121,
MCPControllerRegister is an exported class whose public methods (including the
static create method) lack explicit return type annotations; update the
declaration of create(proto: EggPrototype, controllerMeta: ControllerMetadata,
router: KoaRouter) to include an explicit return type (e.g., : void or :
Promise<void> as appropriate) and add explicit return types to any other
exported/public methods on MCPControllerRegister so the class API is fully
typed; ensure the chosen types accurately reflect whether the method is
synchronous or returns a Promise.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
tegg/standalone/service-worker/src/ServiceWorkerApp.ts (1)
63-103:⚠️ Potential issue | 🔴 CriticalPreviously-flagged issues still present:
__dirnamein ESM and missing public return types.Two items from prior review rounds remain unresolved in the current revision:
- Line 63 still uses
__dirnamewhile this package is"type": "module". In native ESM this identifier is undefined and the constructor will throwReferenceError: __dirname is not definedthe first timeServiceWorkerAppis instantiated. Compute it fromimport.meta.url.- Lines 92, 96, 103:
init,handleEvent,destroyare public methods on an exported class and still lack explicit return type annotations (andhandleEventstill takes the too-broad DOMEventrather than theFetchEventLikethe runner actually expects).🔧 Proposed fix
import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { EggPrototypeLifecycleUtil, LoadUnitLifecycleUtil, } from '@eggjs/tegg-metadata'; @@ import { FetchEventHandler } from './http/FetchEventHandler.ts'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ - async init() { + async init(): Promise<void> { await this.runner.init(); } - async handleEvent<T = unknown>(event: Event) { + async handleEvent<T = unknown>(event: FetchEventLike): Promise<T> { const context = new StandaloneContext(); context.set(ContextProtoProperty.Event.contextKey, event); return await this.runner.run<T>(context); } - async destroy() { + async destroy(): Promise<void> {As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations" and "Use ESM only (no CommonJS)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts` around lines 63 - 103, The constructor still uses the CommonJS __dirname; replace that by computing the package directory from import.meta.url and use that value for frameworkDep.baseDir (reference the constructor and the frameworkDep constant), and add explicit public return types to the exported class methods: annotate init(): Promise<void>, handleEvent<T = unknown>(event: FetchEventLike): Promise<T>, and destroy(): Promise<void> (update the handleEvent parameter type from DOM Event to FetchEventLike to match Runner expectations and keep method signatures explicit).tegg/standalone/service-worker/src/http/HTTPMethodRegister.ts (1)
113-133:⚠️ Potential issue | 🟠 Major
hostRouterleaks across iterations, andcheckDuplicate()still mutates state.Two problems with this block:
- Closure-leak bug:
hostRouteris declared outside theforEachand only reassigned insideif (h). IfgetMethodHosts(...)returns a mixed list such as['a.example', '', 'b.example'](or a trailingundefined), iteration 2 sees a falsyh, skips theif (h)branch, and then re-uses iteration 1'shostRouterfor bothcheckDuplicateInRouterandregisterToRouter. This double-registers routea.exampleand spuriously reports duplicates.- Side-effecting "check": as noted previously,
checkDuplicate()callsregisterToRouter(hostRouter), so a read-only-sounding API mutatescheckRouters. Every subsequent call registers the same route again, breaking idempotency and making a dry-run impossible.🔧 Proposed fix — scope `hostRouter` per iteration and keep `checkDuplicate()` read-only
checkDuplicate() { // 1. check duplicate with egg controller this.checkDuplicateInRouter(this.router); // 2. check duplicate with host tegg controller - let hostRouter; const hosts = this.controllerMeta.getMethodHosts(this.methodMeta) || []; hosts.forEach(h => { - if (h) { - hostRouter = this.checkRouters.get(h); - if (!hostRouter) { - hostRouter = new KoaRouter({ sensitive: true }); - this.checkRouters.set(h, hostRouter); - } - } - if (hostRouter) { - this.checkDuplicateInRouter(hostRouter); - this.registerToRouter(hostRouter); - } + if (!h) return; + let hostRouter = this.checkRouters.get(h); + if (!hostRouter) { + hostRouter = new KoaRouter({ sensitive: true }); + this.checkRouters.set(h, hostRouter); + } + this.checkDuplicateInRouter(hostRouter); + // NOTE: move registerToRouter(hostRouter) into register() so this stays a pure check. }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/standalone/service-worker/src/http/HTTPMethodRegister.ts` around lines 113 - 133, The checkDuplicate method leaks hostRouter across iterations and mutates state; fix it by scoping hostRouter inside the iteration and making the method read-only: inside the forEach over this.controllerMeta.getMethodHosts(this.methodMeta) declare a local hostRouter variable (e.g., const hostRouter = this.checkRouters.get(h) ? this.checkRouters.get(h) : new KoaRouter({ sensitive: true })) so each iteration uses its own router instance without writing into this.checkRouters, and remove the call to registerToRouter(hostRouter) so checkDuplicate only calls checkDuplicateInRouter(hostRouter) and does not mutate checkRouters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/test-util/src/StandaloneTestUtil.ts`:
- Around line 45-71: In StandaloneTestUtil.#createHTTPServerListener, wrap the
call to listener(event) and all response writing in a try/catch so any thrown
error results in a safe fallback (writeHead(500) and res.end()) rather than
bubbling to the http server; ensure the catch also ends the response when
headers/body may already be partially written. In the pipeline callback,
filter/ignore expected client disconnect errors (e.g. ECONNRESET,
ERR_STREAM_PREMATURE_CLOSE) and only log unexpected errors, then ensure
res.end() is called on error paths. Finally stop mutating the incoming
req.headers.host directly in `#createHTTPServerListener/`#buildRequest — set host
on the copied Headers (from StandaloneTestUtil.#buildRequest) or document the
side effect so the original request object is not modified unexpectedly.
- Around line 13-16: Add explicit return type annotations for the exported
public static methods and type the private helpers: change skipOnNode to have a
boolean return type (skipOnNode(minVersion = 18): boolean), annotate
startHTTPServer with an explicit Promise return (e.g. startHTTPServer(...):
Promise<import("http").Server> or the project's Server type), and add types for
the private helpers `#buildRequest` (e.g. (req: import("http").IncomingMessage) =>
SomeRequestType) and `#createHTTPServerListener` (e.g. (req:
import("http").IncomingMessage, res: import("http").ServerResponse) => void) so
the signatures are explicit and satisfy --isolatedDeclarations.
- Around line 1-4: Convert the type-only imports to "import type" to satisfy
TS1484: keep value imports for runtime symbols (createServer, pipeline) but
change all pure types to type-only imports — specifically change
IncomingMessage, OutgoingHttpHeaders, Server, ServerOptions, ServerResponse
(from 'node:http') and Headers, BodyInit, Request, Response (from 'undici') and
FetchEvent (from '@eggjs/tegg-types/standalone') to use "import type" while
leaving createServer and pipeline as regular imports so runtime behavior is
unchanged.
In `@tegg/core/types/standalone/ServiceWorkerContext.ts`:
- Around line 1-16: Change the import of FetchEvent to a type-only import and
rename the generic parameters on ServiceWorkerContext and
ServiceWorkerContextInit from Event/Response to non-shadowing names (e.g.
TEvent, TResponse) so ServiceWorkerFetchContext =
ServiceWorkerContext<FetchEvent, Response> is unambiguous; update the
ServiceWorkerContext signature (get/set response) to use TResponse and TEvent
accordingly and replace the body property type from any to unknown to follow
guidelines. Ensure FetchEvent is imported with the "type" modifier and all
references to Event/Response in ServiceWorkerContext, ServiceWorkerContextInit,
and ServiceWorkerFetchContext are updated to the new identifiers.
In `@tegg/standalone/service-worker/src/constants.ts`:
- Line 1: The current import of ProtoMeta triggers TS1484 under
verbatimModuleSyntax; change the import in constants.ts to a type-only import by
replacing the regular import of ProtoMeta with an "import type { ProtoMeta }
from './types.ts'" so the symbol ProtoMeta is treated as a type-only import and
the module emits no value import; apply this change where ProtoMeta is
referenced (constants.ts) and ensure no runtime usage requires a non-type
import.
In `@tegg/standalone/service-worker/src/controller/ControllerMetadataManager.ts`:
- Line 1: The current named import of ControllerMetadata and ControllerTypeLike
should be changed to type-only imports to avoid emitting runtime imports
(TS1484); update the import statement that brings in ControllerMetadata and
ControllerTypeLike from '@eggjs/tegg' to use the TypeScript "import type" form
so those symbols are treated purely as types (leave any value imports alone if
present) and ensure no runtime reference is introduced by the
ControllerMetadataManager.ts code that uses ControllerMetadata or
ControllerTypeLike.
In `@tegg/standalone/service-worker/src/controller/RootProtoManager.ts`:
- Around line 1-10: The imports for EggPrototype and EggContext are being used
only as types and must be imported with the `type` modifier to satisfy
verbatimModuleSyntax/TS1484, and the public method registerRootProto on class
RootProtoManager needs an explicit return type per repo rules; update the top
import line to use type-only imports for EggPrototype and EggContext (e.g.,
`import type { EggPrototype } from ...` and `import type { EggContext } from
...`) and add an explicit `: void` return type to the `registerRootProto` method
signature.
In `@tegg/standalone/service-worker/src/http/FetchEventHandler.ts`:
- Around line 25-32: The initRoutes method has a race when multiple requests
call it concurrently because it only uses the boolean `#initialized` and awaits
MCPControllerRegister.instance?.doRegister before flipping it; capture and reuse
an in-flight promise instead: add a private promise field (e.g. initPromise) and
in initRoutes, if initPromise exists return await initPromise; otherwise set
initPromise = (async () => {
HTTPControllerRegister.instance?.doRegister(this.rootProtoManager); await
MCPControllerRegister.instance?.doRegister(); this.#routes =
this.fetchRouter.middleware(); this.#initialized = true; initPromise =
undefined; })(); then await and return; this ensures
HTTPControllerRegister.instance?.doRegister,
MCPControllerRegister.instance?.doRegister, and this.#routes assignment run only
once across concurrent callers.
In `@tegg/standalone/service-worker/src/http/ServiceWorkerFetchContext.ts`:
- Around line 27-39: The body setter currently calls this.response =
ResponseUtils.createResponseByBody(body) which triggers the overridden set
response and temporarily clobbers `#body`; change the body setter to assign
this.#body = body first and then assign the synthesized Response directly to the
base class via super.response = ResponseUtils.createResponseByBody(body) to
avoid invoking the override, and change the body type from any to unknown
(update the getter/setter signatures: get body(): unknown | undefined and set
body(body: unknown)). This ensures `#body` is authoritative and not dependent on
setter ordering or response implementation.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts`:
- Around line 357-364: The code collapses a real metadata.name === 'default'
into the unnamed bucket; fix by preserving undefined as the true "nameless"
sentinel instead of using the string 'default': change the map key creation to
use metadata.name ?? undefined (so registerMap becomes Map<string|undefined,
...> or an object keyed by string|undefined), update doRegister to iterate
entries (for (const [name, ...] of this.registerMap)) and call
mcpStatelessStreamServerInit(name) and connectStatelessStreamTransport(name)
directly (where name may be undefined), and ensure any other lookups that
currently expect the literal 'default' are updated to accept undefined;
alternatively, if you prefer rejecting the literal "default", add a validation
in the registration code to throw when metadata.name === 'default' instead of
remapping it.
- Around line 269-281: The duplicate route name issue in MCPControllerRegister
occurs where Reflect.apply(getRouterFunc, this.router, [...
'mcpStatelessStreamNotAllowed' ...]) and Reflect.apply(delRouterFunc,
this.router, [... 'mcpStatelessStreamNotAllowed' ...]) register the same name
for GET and DELETE; change these to distinct names (e.g.,
'mcpStatelessStreamNotAllowed:get' and 'mcpStatelessStreamNotAllowed:delete')
and, inside doRegister where routes are created per MCP server, append or
interpolate the server identifier (server name or id) into the route names
(e.g., `${serverName}:mcpStatelessStreamInit:get`) so route names are unique
across verbs and across multiple MCP server registrations; update any
router.url(...) calls to use the new name scheme accordingly.
In `@tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts`:
- Line 17: All exported async controller methods lack explicit return types
required by isolatedDeclarations; update the method signatures to add Promise
return annotations: change hello to async hello(...): Promise<{ name: string;
type: string[] }>, change find to async find(...): Promise<{ id: string }>, add
an appropriate Promise<...> return type for echoBody (reflecting the echoed body
shape), change headers to async headers(...): Promise<{ headers: Record<string,
string> }>, and add a proper Promise<...> return type for request to represent
the returned request info; modify the signatures for the methods named hello,
find, echoBody, headers, and request accordingly so the exported class methods
have explicit Promise return types.
- Around line 32-38: The echoBody method should accept the body as unknown and
safely narrow it to FormData using instanceof rather than constructor.name,
update the inferred result type to a typed container (e.g., Record<string,
unknown>) and add an explicit return type for the method to satisfy
isolatedDeclarations; specifically, change the parameter type on
echoBody(`@HTTPBody`() body: unknown), guard with if (body instanceof FormData)
and build res as Record<string, unknown>, then return a correctly typed value
(and add the method's explicit return type signature) so other branches
(JSON/string) remain type-safe.
In `@tegg/standalone/service-worker/test/http/router.test.ts`:
- Around line 1-58: Replace Mocha globals with Vitest imports and context usage:
import describe, it, beforeAll, afterAll from 'vitest' (or use named imports)
and change the lifecycle hooks in this file so that before -> beforeAll and
after -> afterAll; update the beforeAll callback to accept a ctx parameter and
replace this.skip() with ctx.skip(); ensure assertions and
TestUtils.createFetchApp usage remain unchanged and keep references to
httpAdviceExecutionLog, ServiceWorkerApp, and server as-is.
---
Duplicate comments:
In `@tegg/standalone/service-worker/src/http/HTTPMethodRegister.ts`:
- Around line 113-133: The checkDuplicate method leaks hostRouter across
iterations and mutates state; fix it by scoping hostRouter inside the iteration
and making the method read-only: inside the forEach over
this.controllerMeta.getMethodHosts(this.methodMeta) declare a local hostRouter
variable (e.g., const hostRouter = this.checkRouters.get(h) ?
this.checkRouters.get(h) : new KoaRouter({ sensitive: true })) so each iteration
uses its own router instance without writing into this.checkRouters, and remove
the call to registerToRouter(hostRouter) so checkDuplicate only calls
checkDuplicateInRouter(hostRouter) and does not mutate checkRouters.
In `@tegg/standalone/service-worker/src/ServiceWorkerApp.ts`:
- Around line 63-103: The constructor still uses the CommonJS __dirname; replace
that by computing the package directory from import.meta.url and use that value
for frameworkDep.baseDir (reference the constructor and the frameworkDep
constant), and add explicit public return types to the exported class methods:
annotate init(): Promise<void>, handleEvent<T = unknown>(event: FetchEventLike):
Promise<T>, and destroy(): Promise<void> (update the handleEvent parameter type
from DOM Event to FetchEventLike to match Runner expectations and keep method
signatures explicit).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f56cdf49-aa63-4bcd-9270-b001db362246
📒 Files selected for processing (63)
pnpm-workspace.yamltegg/core/standalone-decorator/src/event/EventHandler.tstegg/core/standalone-decorator/src/index.tstegg/core/test-util/package.jsontegg/core/test-util/src/StandaloneTestUtil.tstegg/core/types/package.jsontegg/core/types/standalone/ServiceWorkerContext.tstegg/core/types/standalone/fetch.tstegg/core/types/standalone/index.tstegg/standalone/service-worker/package.jsontegg/standalone/service-worker/src/ServiceWorkerApp.tstegg/standalone/service-worker/src/ServiceWorkerRunner.tstegg/standalone/service-worker/src/StandaloneEggObjectFactory.tstegg/standalone/service-worker/src/constants.tstegg/standalone/service-worker/src/controller/ControllerMetadataManager.tstegg/standalone/service-worker/src/controller/ControllerRegister.tstegg/standalone/service-worker/src/controller/ControllerRegisterFactory.tstegg/standalone/service-worker/src/controller/RootProtoManager.tstegg/standalone/service-worker/src/controller/ServiceWorkerContext.tstegg/standalone/service-worker/src/hook/ContextProtoLoadUnitHook.tstegg/standalone/service-worker/src/hook/ControllerLoadUnitHook.tstegg/standalone/service-worker/src/hook/ControllerPrototypeHook.tstegg/standalone/service-worker/src/hook/LoadUnitInnerClassHook.tstegg/standalone/service-worker/src/http/FetchEventHandler.tstegg/standalone/service-worker/src/http/FetchRouter.tstegg/standalone/service-worker/src/http/HTTPControllerRegister.tstegg/standalone/service-worker/src/http/HTTPMethodRegister.tstegg/standalone/service-worker/src/http/ServiceWorkerFetchContext.tstegg/standalone/service-worker/src/index.tstegg/standalone/service-worker/src/mcp/AbstractControllerAdvice.tstegg/standalone/service-worker/src/mcp/MCPControllerRegister.tstegg/standalone/service-worker/src/mcp/MCPServerHelper.tstegg/standalone/service-worker/src/types.tstegg/standalone/service-worker/src/utils/RequestUtils.tstegg/standalone/service-worker/src/utils/ResponseUtils.tstegg/standalone/service-worker/test/Utils.tstegg/standalone/service-worker/test/fixtures/http-builtin/BuiltinController.tstegg/standalone/service-worker/test/fixtures/http-builtin/package.jsontegg/standalone/service-worker/test/fixtures/http-inject/UserController.tstegg/standalone/service-worker/test/fixtures/http-inject/UserService.tstegg/standalone/service-worker/test/fixtures/http-inject/package.jsontegg/standalone/service-worker/test/fixtures/http-params/ParamController.tstegg/standalone/service-worker/test/fixtures/http-params/package.jsontegg/standalone/service-worker/test/fixtures/http-priority/PriorityController.tstegg/standalone/service-worker/test/fixtures/http-priority/ViewController.tstegg/standalone/service-worker/test/fixtures/http-priority/package.jsontegg/standalone/service-worker/test/fixtures/http/AopMiddlewareController.tstegg/standalone/service-worker/test/fixtures/http/GetController.tstegg/standalone/service-worker/test/fixtures/http/HttpTestAdvice.tstegg/standalone/service-worker/test/fixtures/http/PostController.tstegg/standalone/service-worker/test/fixtures/http/package.jsontegg/standalone/service-worker/test/fixtures/mcp/MCPTestController.tstegg/standalone/service-worker/test/fixtures/mcp/McpTestAdvice.tstegg/standalone/service-worker/test/fixtures/mcp/package.jsontegg/standalone/service-worker/test/http/builtin.test.tstegg/standalone/service-worker/test/http/inject.test.tstegg/standalone/service-worker/test/http/params.test.tstegg/standalone/service-worker/test/http/priority.test.tstegg/standalone/service-worker/test/http/response.test.tstegg/standalone/service-worker/test/http/router.test.tstegg/standalone/service-worker/test/mcp/mcp.test.tstegg/standalone/service-worker/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (31)
- tegg/standalone/service-worker/tsconfig.json
- tegg/core/types/standalone/index.ts
- pnpm-workspace.yaml
- tegg/standalone/service-worker/src/controller/ControllerRegister.ts
- tegg/standalone/service-worker/test/fixtures/http-params/package.json
- tsconfig.json
- tegg/core/standalone-decorator/src/index.ts
- tegg/standalone/service-worker/test/fixtures/http-inject/package.json
- tegg/standalone/service-worker/test/fixtures/http-builtin/package.json
- tegg/standalone/service-worker/test/fixtures/mcp/package.json
- tegg/core/types/standalone/fetch.ts
- tegg/standalone/service-worker/test/fixtures/http-priority/ViewController.ts
- tegg/standalone/service-worker/test/fixtures/http-priority/PriorityController.ts
- tegg/standalone/service-worker/test/fixtures/http-priority/package.json
- tegg/standalone/service-worker/test/http/params.test.ts
- tegg/standalone/service-worker/test/fixtures/http/PostController.ts
- tegg/standalone/service-worker/test/http/priority.test.ts
- tegg/standalone/service-worker/src/mcp/AbstractControllerAdvice.ts
- tegg/standalone/service-worker/test/http/builtin.test.ts
- tegg/standalone/service-worker/src/controller/ControllerRegisterFactory.ts
- tegg/standalone/service-worker/test/fixtures/mcp/McpTestAdvice.ts
- tegg/standalone/service-worker/src/types.ts
- tegg/standalone/service-worker/src/http/FetchRouter.ts
- tegg/standalone/service-worker/test/Utils.ts
- tegg/standalone/service-worker/src/http/HTTPControllerRegister.ts
- tegg/standalone/service-worker/package.json
- tegg/standalone/service-worker/src/index.ts
- tegg/standalone/service-worker/test/fixtures/http-inject/UserService.ts
- tegg/core/standalone-decorator/src/event/EventHandler.ts
- tegg/standalone/service-worker/test/fixtures/http/HttpTestAdvice.ts
- tegg/standalone/service-worker/test/fixtures/http-builtin/BuiltinController.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- tegg/standalone/service-worker/test/fixtures/http/AopMiddlewareController.ts
- tegg/standalone/service-worker/src/hook/ControllerPrototypeHook.ts
- tegg/core/test-util/package.json
- tegg/standalone/service-worker/test/http/inject.test.ts
- tegg/standalone/service-worker/src/StandaloneEggObjectFactory.ts
- tegg/standalone/service-worker/src/utils/ResponseUtils.ts
- tegg/standalone/service-worker/src/ServiceWorkerRunner.ts
- tegg/standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts
- tegg/standalone/service-worker/test/fixtures/http/package.json
- tegg/standalone/service-worker/test/fixtures/http-inject/UserController.ts
- tegg/standalone/service-worker/src/controller/ServiceWorkerContext.ts
- tegg/standalone/service-worker/test/fixtures/mcp/MCPTestController.ts
- tegg/standalone/service-worker/src/hook/ControllerLoadUnitHook.ts
- tegg/standalone/service-worker/src/hook/ContextProtoLoadUnitHook.ts
- tegg/standalone/service-worker/test/http/response.test.ts
- tegg/standalone/service-worker/src/utils/RequestUtils.ts
- tegg/standalone/service-worker/test/mcp/mcp.test.ts
- tegg/standalone/service-worker/src/mcp/MCPServerHelper.ts
- tegg/standalone/service-worker/test/fixtures/http/GetController.ts
Comment on lines
+1
to
+4
| import { createServer, IncomingMessage, OutgoingHttpHeaders, Server, ServerOptions, ServerResponse } from 'node:http'; | ||
| import { pipeline } from 'node:stream'; | ||
| import { Headers, BodyInit, Request, Response } from 'undici'; | ||
| import { FetchEvent } from '@eggjs/tegg-types/standalone'; |
Contributor
There was a problem hiding this comment.
Convert type-only imports (TS1484).
♻️ Proposed fix
-import { createServer, IncomingMessage, OutgoingHttpHeaders, Server, ServerOptions, ServerResponse } from 'node:http';
+import { createServer, IncomingMessage, Server, ServerResponse } from 'node:http';
+import type { OutgoingHttpHeaders, ServerOptions } from 'node:http';
import { pipeline } from 'node:stream';
-import { Headers, BodyInit, Request, Response } from 'undici';
-import { FetchEvent } from '@eggjs/tegg-types/standalone';
+import { Headers, Request, Response } from 'undici';
+import type { BodyInit } from 'undici';
+import type { FetchEvent } from '@eggjs/tegg-types/standalone';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { createServer, IncomingMessage, OutgoingHttpHeaders, Server, ServerOptions, ServerResponse } from 'node:http'; | |
| import { pipeline } from 'node:stream'; | |
| import { Headers, BodyInit, Request, Response } from 'undici'; | |
| import { FetchEvent } from '@eggjs/tegg-types/standalone'; | |
| import { createServer, IncomingMessage, Server, ServerResponse } from 'node:http'; | |
| import type { OutgoingHttpHeaders, ServerOptions } from 'node:http'; | |
| import { pipeline } from 'node:stream'; | |
| import { Headers, Request, Response } from 'undici'; | |
| import type { BodyInit } from 'undici'; | |
| import type { FetchEvent } from '@eggjs/tegg-types/standalone'; |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 4-4: typescript(TS1484)
'FetchEvent' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 3-3: typescript(TS1484)
'BodyInit' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 1-1: typescript(TS1484)
'ServerOptions' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 1-1: typescript(TS1484)
'OutgoingHttpHeaders' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/test-util/src/StandaloneTestUtil.ts` around lines 1 - 4, Convert
the type-only imports to "import type" to satisfy TS1484: keep value imports for
runtime symbols (createServer, pipeline) but change all pure types to type-only
imports — specifically change IncomingMessage, OutgoingHttpHeaders, Server,
ServerOptions, ServerResponse (from 'node:http') and Headers, BodyInit, Request,
Response (from 'undici') and FetchEvent (from '@eggjs/tegg-types/standalone') to
use "import type" while leaving createServer and pipeline as regular imports so
runtime behavior is unchanged.
Comment on lines
+13
to
+16
| static skipOnNode(minVersion = 18) { | ||
| const version = parseInt(process.versions.node.split('.')[0], 10); | ||
| return version < minVersion; | ||
| } |
Contributor
There was a problem hiding this comment.
Add explicit return types for public static methods.
skipOnNode and startHTTPServer are exported public APIs and need explicit return type annotations per coding guidelines. Also consider typing #buildRequest and #createHTTPServerListener for consistency.
♻️ Proposed fix
- static skipOnNode(minVersion = 18) {
+ static skipOnNode(minVersion = 18): boolean {
...
- static startHTTPServer(host: string, port: number, { listener, ...options }: StartHTTPServerOptions) {
+ static startHTTPServer(host: string, port: number, { listener, ...options }: StartHTTPServerOptions): Promise<Server> {As per coding guidelines: "All exported functions, methods, and getters must have explicit return type annotations to support TypeScript's --isolatedDeclarations flag".
Also applies to: 73-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/test-util/src/StandaloneTestUtil.ts` around lines 13 - 16, Add
explicit return type annotations for the exported public static methods and type
the private helpers: change skipOnNode to have a boolean return type
(skipOnNode(minVersion = 18): boolean), annotate startHTTPServer with an
explicit Promise return (e.g. startHTTPServer(...):
Promise<import("http").Server> or the project's Server type), and add types for
the private helpers `#buildRequest` (e.g. (req: import("http").IncomingMessage) =>
SomeRequestType) and `#createHTTPServerListener` (e.g. (req:
import("http").IncomingMessage, res: import("http").ServerResponse) => void) so
the signatures are explicit and satisfy --isolatedDeclarations.
Comment on lines
+45
to
+71
| static #createHTTPServerListener(listener: FetchEventListener) { | ||
| return async (req: IncomingMessage, res: ServerResponse) => { | ||
| const request = StandaloneTestUtil.#buildRequest(req); | ||
| // TODO currently fake FetchEvent | ||
| const event: any = new Event('fetch'); | ||
| event.request = request; | ||
| const response = await listener(event); | ||
|
|
||
| const headers: OutgoingHttpHeaders = {}; | ||
| for (const [ key, value ] of response.headers) { | ||
| headers[key.toLowerCase()] = value; | ||
| } | ||
|
|
||
| res.writeHead(response.status, headers); | ||
|
|
||
| if (!response.body) { | ||
| res.end(); | ||
| return; | ||
| } | ||
| pipeline(response.body, res, e => { | ||
| if (e) { | ||
| console.error(`pipeline writing response error for url ${response.url}`, e); | ||
| res.end(); | ||
| } | ||
| }); | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Client-disconnect / listener error paths swallow errors and may leave response hanging.
A couple of robustness concerns in the per-request listener:
- If
listener(event)throws, the exception bubbles out of the async handler and Node'shttpserver emits'clientError'/ leaves the socket without a written response. Wrap the body intry/catchandres.writeHead(500).end()as a fallback. - Inside
pipeline,console.erroris used for logging — OK for a test util, but note that if the client disconnects early (ECONNRESET/ERR_STREAM_PREMATURE_CLOSE), you'll get spurious errors. Consider filtering those. req.headers.host = url.host;mutates the incoming request headers object as a side effect before copying — harmless but non-obvious; it should be documented or done only on the copiedHeaders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/test-util/src/StandaloneTestUtil.ts` around lines 45 - 71, In
StandaloneTestUtil.#createHTTPServerListener, wrap the call to listener(event)
and all response writing in a try/catch so any thrown error results in a safe
fallback (writeHead(500) and res.end()) rather than bubbling to the http server;
ensure the catch also ends the response when headers/body may already be
partially written. In the pipeline callback, filter/ignore expected client
disconnect errors (e.g. ECONNRESET, ERR_STREAM_PREMATURE_CLOSE) and only log
unexpected errors, then ensure res.end() is called on error paths. Finally stop
mutating the incoming req.headers.host directly in
`#createHTTPServerListener/`#buildRequest — set host on the copied Headers (from
StandaloneTestUtil.#buildRequest) or document the side effect so the original
request object is not modified unexpectedly.
Comment on lines
+1
to
+16
| import { FetchEvent } from './fetch.ts'; | ||
|
|
||
| export interface ServiceWorkerContextInit<T> { | ||
| event: T; | ||
| } | ||
|
|
||
| export interface ServiceWorkerContext<Event, Response> { | ||
| event: Event; | ||
| get response(): Response | undefined; | ||
| set response(response: Response); | ||
|
|
||
| get body(): any | undefined; | ||
| set body(body: any); | ||
| } | ||
|
|
||
| export type ServiceWorkerFetchContext = ServiceWorkerContext<FetchEvent, Response>; |
Contributor
There was a problem hiding this comment.
Type-only import + generic param names shadow DOM globals.
FetchEventshould be a type-only import (TS1484).- The generic parameters
EventandResponseshadow the DOM globals with the same names, which makesServiceWorkerFetchContext = ServiceWorkerContext<FetchEvent, Response>ambiguous when reading the file and leaks into editor tooling. Prefer distinct names (e.g.TEvent,TResponse).
♻️ Proposed fix
-import { FetchEvent } from './fetch.ts';
+import type { FetchEvent } from './fetch.ts';
-export interface ServiceWorkerContext<Event, Response> {
- event: Event;
- get response(): Response | undefined;
- set response(response: Response);
-
- get body(): any | undefined;
- set body(body: any);
-}
-
-export type ServiceWorkerFetchContext = ServiceWorkerContext<FetchEvent, Response>;
+export interface ServiceWorkerContext<TEvent, TResponse> {
+ event: TEvent;
+ get response(): TResponse | undefined;
+ set response(response: TResponse);
+
+ get body(): unknown | undefined;
+ set body(body: unknown);
+}
+
+export type ServiceWorkerFetchContext = ServiceWorkerContext<FetchEvent, Response>;Also note the any on body — coding guidelines call for unknown when the type is truly unknown.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 1-1: typescript(TS1484)
'FetchEvent' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/types/standalone/ServiceWorkerContext.ts` around lines 1 - 16,
Change the import of FetchEvent to a type-only import and rename the generic
parameters on ServiceWorkerContext and ServiceWorkerContextInit from
Event/Response to non-shadowing names (e.g. TEvent, TResponse) so
ServiceWorkerFetchContext = ServiceWorkerContext<FetchEvent, Response> is
unambiguous; update the ServiceWorkerContext signature (get/set response) to use
TResponse and TEvent accordingly and replace the body property type from any to
unknown to follow guidelines. Ensure FetchEvent is imported with the "type"
modifier and all references to Event/Response in ServiceWorkerContext,
ServiceWorkerContextInit, and ServiceWorkerFetchContext are updated to the new
identifiers.
| @@ -0,0 +1,8 @@ | |||
| import { ProtoMeta } from './types.ts'; | |||
Contributor
There was a problem hiding this comment.
Use a type-only import for ProtoMeta.
Typecheck fails with TS1484 under verbatimModuleSyntax.
♻️ Proposed fix
-import { ProtoMeta } from './types.ts';
+import type { ProtoMeta } from './types.ts';🧰 Tools
🪛 GitHub Check: typecheck
[failure] 1-1: typescript(TS1484)
'ProtoMeta' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/constants.ts` at line 1, The current
import of ProtoMeta triggers TS1484 under verbatimModuleSyntax; change the
import in constants.ts to a type-only import by replacing the regular import of
ProtoMeta with an "import type { ProtoMeta } from './types.ts'" so the symbol
ProtoMeta is treated as a type-only import and the module emits no value import;
apply this change where ProtoMeta is referenced (constants.ts) and ensure no
runtime usage requires a non-type import.
Comment on lines
+269
to
+281
| const getRouterFunc = this.router.get; | ||
| const delRouterFunc = this.router.del; | ||
| Reflect.apply(getRouterFunc, this.router, [ | ||
| 'mcpStatelessStreamNotAllowed', | ||
| streamPath, | ||
| notAllowedHandler, | ||
| ]); | ||
| Reflect.apply(delRouterFunc, this.router, [ | ||
| 'mcpStatelessStreamNotAllowed', | ||
| streamPath, | ||
| notAllowedHandler, | ||
| ]); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm `@eggjs/router` behavior on duplicate route names.
rg -nP 'routes\[name\]|this\.name|layer\.name' node_modules/@eggjs/router/ 2>/dev/null | head -40 || true
fd -H 'package.json' tegg/standalone/service-worker --exec jq -r '.dependencies["@eggjs/router"], .peerDependencies["@eggjs/router"]' {}Repository: eggjs/egg
Length of output: 129
🏁 Script executed:
# Read the exact code at the specified lines
cat -n tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts | sed -n '269,281p'Repository: eggjs/egg
Length of output: 507
🏁 Script executed:
# Find the package.json and check router dependency
fd -H 'package.json' tegg/standalone/service-worker | head -1 | xargs cat | jq '.dependencies, .peerDependencies'Repository: eggjs/egg
Length of output: 633
🏁 Script executed:
# Check the router implementation to understand how it handles route names
fd -H '@eggjs/router' tegg --type d | head -5Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Find `@eggjs/router` location in the monorepo
find . -name router -type d -path '*/packages/*' 2>/dev/null | head -10Repository: eggjs/egg
Length of output: 70
🏁 Script executed:
# Look for router package.json
fd -H 'package.json' --path '*router*' | grep -E '/@eggjs/router|packages.*router' | head -5Repository: eggjs/egg
Length of output: 277
🏁 Script executed:
# Check the actual router implementation in packages
ls -la packages/ 2>/dev/null | grep routerRepository: eggjs/egg
Length of output: 108
🏁 Script executed:
# Check the router package structure and main files
ls -la packages/router/Repository: eggjs/egg
Length of output: 779
🏁 Script executed:
# Look at the router source to understand how it handles route names
cat packages/router/package.json | jq '.main, .exports'Repository: eggjs/egg
Length of output: 136
🏁 Script executed:
# Find and examine the router's route registration logic
fd -e ts -e js packages/router/lib packages/router/src 2>/dev/null | head -10Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# List source files in the router package
ls -la packages/router/src/Repository: eggjs/egg
Length of output: 466
🏁 Script executed:
# Look at the main router implementation
cat packages/router/src/index.ts | head -100Repository: eggjs/egg
Length of output: 285
🏁 Script executed:
# Search for how route names are stored and managed
rg -n 'this\.routes|this\.named|name.*=|routes\[' packages/router/src/Router.ts | head -30Repository: eggjs/egg
Length of output: 566
🏁 Script executed:
# Look at the method signatures for get/del methods
ast-grep --pattern 'get($_, $_, $_) { $$$ }' packages/router/src/Router.tsRepository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check how the get and del methods handle the name parameter
sed -n '1,150p' packages/router/src/Router.ts | grep -A 20 'get('Repository: eggjs/egg
Length of output: 571
🏁 Script executed:
# Look at the section around line 462 to understand how routes with the same name are handled
sed -n '450,480p' packages/router/src/Router.tsRepository: eggjs/egg
Length of output: 673
🏁 Script executed:
# Check the Layer.ts to see how route names are stored
cat packages/router/src/Layer.ts | head -80Repository: eggjs/egg
Length of output: 2547
🏁 Script executed:
# Search for all occurrences of name assignment in Router.ts
rg -n "route\.name\s*=" packages/router/src/Router.tsRepository: eggjs/egg
Length of output: 89
Duplicate koa-router route name registered for GET and DELETE.
Both the GET and DELETE registrations on lines 271–275 and 276–280 pass 'mcpStatelessStreamNotAllowed' as the route name. The router.route() method iterates through the route stack and returns the first match; registering two routes under the same name means the DELETE route becomes invisible to name-based lookups. Any call to router.url('mcpStatelessStreamNotAllowed', ...) will generate a URL based only on the GET route.
🔧 Proposed fix — give each HTTP verb a distinct route name
Reflect.apply(getRouterFunc, this.router, [
- 'mcpStatelessStreamNotAllowed',
+ 'mcpStatelessStreamGetNotAllowed',
streamPath,
notAllowedHandler,
]);
Reflect.apply(delRouterFunc, this.router, [
- 'mcpStatelessStreamNotAllowed',
+ 'mcpStatelessStreamDeleteNotAllowed',
streamPath,
notAllowedHandler,
]);Additionally, when multiple MCP servers are registered, the same literal name 'mcpStatelessStreamInit'/'mcpStatelessStreamNotAllowed' is reused across doRegister() iterations — consider suffixing with the server name to avoid collisions across instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines
269 - 281, The duplicate route name issue in MCPControllerRegister occurs where
Reflect.apply(getRouterFunc, this.router, [... 'mcpStatelessStreamNotAllowed'
...]) and Reflect.apply(delRouterFunc, this.router, [...
'mcpStatelessStreamNotAllowed' ...]) register the same name for GET and DELETE;
change these to distinct names (e.g., 'mcpStatelessStreamNotAllowed:get' and
'mcpStatelessStreamNotAllowed:delete') and, inside doRegister where routes are
created per MCP server, append or interpolate the server identifier (server name
or id) into the route names (e.g., `${serverName}:mcpStatelessStreamInit:get`)
so route names are unique across verbs and across multiple MCP server
registrations; update any router.url(...) calls to use the new name scheme
accordingly.
Comment on lines
+357
to
+364
| async doRegister() { | ||
| // Initialize MCP routes for each server name | ||
| const names = Object.keys(this.registerMap); | ||
| for (const name of names) { | ||
| await this.mcpStatelessStreamServerInit(name === 'default' ? undefined : name); | ||
| await this.connectStatelessStreamTransport(name); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
name === 'default' string comparison collapses a user-supplied "default" metadata name.
registerMap keys are built with metadata.name ?? 'default', so an MCP controller that legitimately declares metadata.name === 'default' is indistinguishable from the unnamed-default bucket. The branch name === 'default' ? undefined : name then strips the name when building the URL (mcpStatelessStreamServerInit(undefined) → path /mcp/stream), while connectStatelessStreamTransport(name) receives the string 'default' and looks up transports/helpers at key 'default' — the route path and the transport key diverge.
Prefer tracking "nameless" explicitly (e.g. a sentinel undefined key or a parallel Map<string|undefined, …>), or reject metadata.name === 'default' at registration time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/src/mcp/MCPControllerRegister.ts` around lines
357 - 364, The code collapses a real metadata.name === 'default' into the
unnamed bucket; fix by preserving undefined as the true "nameless" sentinel
instead of using the string 'default': change the map key creation to use
metadata.name ?? undefined (so registerMap becomes Map<string|undefined, ...> or
an object keyed by string|undefined), update doRegister to iterate entries (for
(const [name, ...] of this.registerMap)) and call
mcpStatelessStreamServerInit(name) and connectStatelessStreamTransport(name)
directly (where name may be undefined), and ensure any other lookups that
currently expect the literal 'default' are updated to accept undefined;
alternatively, if you prefer rejecting the literal "default", add a validation
in the registration code to throw when metadata.name === 'default' instead of
remapping it.
| @HTTPController() | ||
| export class ParamController { | ||
| @HTTPMethod({ method: HTTPMethodEnum.GET, path: '/query' }) | ||
| async hello(@HTTPQuery() name: string, @HTTPQueries() type: string[]) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: locate package TypeScript settings that make explicit public API types required.
fd -i 'tsconfig.*json' | xargs rg -n -C2 '"(strict|isolatedDeclarations|declaration)"\s*:'Repository: eggjs/egg
Length of output: 11663
🏁 Script executed:
#!/bin/bash
# Find tsconfig.json files in tegg directory
echo "=== Looking for tsconfig.json in tegg/standalone/service-worker and parent dirs ==="
fd -i 'tsconfig.*json' tegg/standalone/service-worker/ 2>/dev/null | head -20
# Check if there's a tsconfig in tegg root
echo -e "\n=== Checking tegg directory structure ==="
ls -la tegg/ 2>/dev/null | head -20
# Check tegg/standalone directory
echo -e "\n=== Checking tegg/standalone structure ==="
ls -la tegg/standalone/ 2>/dev/null | head -20
# Read the actual ParamController.ts file to see method signatures
echo -e "\n=== Reading ParamController.ts ==="
cat -n tegg/standalone/service-worker/test/fixtures/http-params/ParamController.tsRepository: eggjs/egg
Length of output: 2832
🏁 Script executed:
#!/bin/bash
# Check what the root tsconfig extends and includes/excludes
echo "=== Root tsconfig.json ==="
head -50 tsconfig.json
echo -e "\n=== Checking for exclude patterns in root ==="
rg -n '"exclude"' tsconfig.json -A 5
# Check if tegg has its own tsconfig
echo -e "\n=== Looking for any config in tegg package ==="
find tegg -maxdepth 2 -name 'tsconfig*.json' -o -name 'package.json' | head -10Repository: eggjs/egg
Length of output: 1312
🏁 Script executed:
#!/bin/bash
# Read the tegg/standalone/service-worker tsconfig.json
echo "=== tegg/standalone/service-worker/tsconfig.json ==="
cat tegg/standalone/service-worker/tsconfig.json
# Also check parent tegg/standalone if it exists
echo -e "\n=== Check for tegg/standalone/tsconfig.json ==="
if [ -f tegg/standalone/tsconfig.json ]; then
cat tegg/standalone/tsconfig.json
else
echo "Not found"
fiRepository: eggjs/egg
Length of output: 206
Add explicit return types to the controller action methods.
These public methods on an exported class lack explicit return type annotations. With isolatedDeclarations: true in the root tsconfig.json (which this file inherits), explicit return types are required. Add Promise<...> return types to each async method:
- Line 17:
async hello(...): Promise<{ name: string; type: string[] }> - Line 25:
async find(...): Promise<{ id: string }> - Line 32:
async echoBody(...): Promise<...> - Line 44:
async headers(...): Promise<{ headers: Record<string, string> }> - Line 51:
async request(...): Promise<...>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts`
at line 17, All exported async controller methods lack explicit return types
required by isolatedDeclarations; update the method signatures to add Promise
return annotations: change hello to async hello(...): Promise<{ name: string;
type: string[] }>, change find to async find(...): Promise<{ id: string }>, add
an appropriate Promise<...> return type for echoBody (reflecting the echoed body
shape), change headers to async headers(...): Promise<{ headers: Record<string,
string> }>, and add a proper Promise<...> return type for request to represent
the returned request info; modify the signatures for the methods named hello,
find, echoBody, headers, and request accordingly so the exported class methods
have explicit Promise return types.
Comment on lines
+32
to
+38
| async echoBody(@HTTPBody() body: object) { | ||
| if (body.constructor.name === 'FormData') { | ||
| const res = {}; | ||
| for (const [ key, value ] of (body as FormData).entries()) { | ||
| res[key] = value; | ||
| } | ||
| return { type: 'formData', body: res }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the ParamController.ts file
find . -name "ParamController.ts" -type fRepository: eggjs/egg
Length of output: 223
🏁 Script executed:
# Also search for the `@HTTPBody` decorator definition to understand its type signature
rg "@HTTPBody" -A 5 -B 5 --type ts --type tsxRepository: eggjs/egg
Length of output: 80
🏁 Script executed:
# Check tsconfig.json to see strict mode settings
find . -name "tsconfig.json" -type f | head -5Repository: eggjs/egg
Length of output: 272
🏁 Script executed:
# Read the ParamController.ts file to see the actual code
cat -n ./tegg/standalone/service-worker/test/fixtures/http-params/ParamController.tsRepository: eggjs/egg
Length of output: 1810
🏁 Script executed:
# Search for `@HTTPBody` decorator definition
rg "@HTTPBody" -A 5 -B 5 --type tsRepository: eggjs/egg
Length of output: 13380
🏁 Script executed:
# Find tsconfig.json in tegg directory for strict mode settings
find ./tegg -name "tsconfig.json" -type f | head -5Repository: eggjs/egg
Length of output: 246
🏁 Script executed:
# Check the tsconfig.json for the tegg standalone service-worker to see strict mode settings
find ./tegg/standalone -name "tsconfig.json" -type f | xargs cat -nRepository: eggjs/egg
Length of output: 452
🏁 Script executed:
# Look at the tests for this ParamController to understand what types of data are actually sent
cat -n ./tegg/standalone/service-worker/test/http/params.test.ts | grep -A 30 "@HTTPBody"Repository: eggjs/egg
Length of output: 1171
🏁 Script executed:
# Check the root tsconfig.json to see strict mode settings
cat -n ./tsconfig.jsonRepository: eggjs/egg
Length of output: 3135
🏁 Script executed:
# Let's also check what the actual return type expectations are and other FormData handling in the codebase
rg "FormData" --type ts -B 3 -A 3 | head -50Repository: eggjs/egg
Length of output: 1066
🏁 Script executed:
# Check if there's a specific tsconfig for strict mode in extended tsconfig
npm list `@eggjs/tsconfig` 2>/dev/null | head -20 || echo "Checking `@eggjs/tsconfig`..."Repository: eggjs/egg
Length of output: 113
🏁 Script executed:
# Verify what types `@HTTPBody` actually accepts - check the decorator definition
rg "class HTTPBody|function HTTPBody|export.*HTTPBody" -B 5 -A 10 --type tsRepository: eggjs/egg
Length of output: 1631
🏁 Script executed:
# Check if strict mode is actually enabled in the `@eggjs/tsconfig` or compiler options
cat -n ./node_modules/@eggjs/tsconfig/tsconfig.json 2>/dev/null || echo "Will search for tsconfig base"Repository: eggjs/egg
Length of output: 82
🏁 Script executed:
# Let's check what the actual return type of echoBody should be by looking at the full method
cat -n ./tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts | sed -n '31,41p'Repository: eggjs/egg
Length of output: 489
🏁 Script executed:
# Check if isolatedDeclarations requires explicit return types
rg "isolatedDeclarations" -B 5 -A 5 --type ts --type jsonRepository: eggjs/egg
Length of output: 2285
🏁 Script executed:
# Verify strict mode setting by checking for "strict" in tsconfig
find . -name "tsconfig.json" -type f | xargs grep -l "\"strict\"" | head -5Repository: eggjs/egg
Length of output: 318
🏁 Script executed:
# Check if the tests actually pass with text body - to confirm body can be a string
cat -n ./tegg/standalone/service-worker/test/http/params.test.ts | sed -n '59,65p'Repository: eggjs/egg
Length of output: 319
🏁 Script executed:
# Check what `@eggjs/tsconfig` extends and whether strict mode is enabled
find . -name "*.json" -path "*/node_modules/@eggjs/tsconfig*" 2>/dev/null | head -3Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Verify if there are any compilation errors/warnings reported when running tests
find ./tegg/standalone/service-worker -name "*.test.ts" | head -1 | xargs grep -l "echoBody"Repository: eggjs/egg
Length of output: 37
🏁 Script executed:
# Check if body.constructor could actually throw - verify null handling
cat -n ./tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts | sed -n '30,42p'Repository: eggjs/egg
Length of output: 505
Type the body as unknown and guard FormData safely.
The @HTTPBody() decorator accepts multiple types (JSON objects, FormData, and plain text strings per the test cases), making object too narrow. Also, const res = {} is not type-safe (should be Record<string, unknown>), and using body.constructor.name is fragile compared to instanceof FormData. Additionally, the method lacks an explicit return type, which is required by isolatedDeclarations.
Suggested fix
- async echoBody(`@HTTPBody`() body: object) {
- if (body.constructor.name === 'FormData') {
- const res = {};
- for (const [ key, value ] of (body as FormData).entries()) {
+ async echoBody(`@HTTPBody`() body: unknown): Promise<{ type: string; body: Record<string, unknown> } | { body: unknown }> {
+ if (body instanceof FormData) {
+ const res: Record<string, unknown> = {};
+ for (const [ key, value ] of body.entries()) {
res[key] = value;
}
return { type: 'formData', body: res };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async echoBody(@HTTPBody() body: object) { | |
| if (body.constructor.name === 'FormData') { | |
| const res = {}; | |
| for (const [ key, value ] of (body as FormData).entries()) { | |
| res[key] = value; | |
| } | |
| return { type: 'formData', body: res }; | |
| async echoBody(`@HTTPBody`() body: unknown): Promise<{ type: string; body: Record<string, unknown> } | { body: unknown }> { | |
| if (body instanceof FormData) { | |
| const res: Record<string, unknown> = {}; | |
| for (const [ key, value ] of body.entries()) { | |
| res[key] = value; | |
| } | |
| return { type: 'formData', body: res }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/test/fixtures/http-params/ParamController.ts`
around lines 32 - 38, The echoBody method should accept the body as unknown and
safely narrow it to FormData using instanceof rather than constructor.name,
update the inferred result type to a typed container (e.g., Record<string,
unknown>) and add an explicit return type for the method to satisfy
isolatedDeclarations; specifically, change the parameter type on
echoBody(`@HTTPBody`() body: unknown), guard with if (body instanceof FormData)
and build res as Record<string, unknown>, then return a correctly typed value
(and add the method's explicit return type signature) so other branches
(JSON/string) remain type-safe.
Comment on lines
+1
to
+58
| import assert from 'node:assert'; | ||
| import { Server } from 'node:http'; | ||
| import httpRequest from 'supertest'; | ||
| import { ServiceWorkerApp } from '../../src/ServiceWorkerApp.ts'; | ||
| import { StandaloneTestUtil } from '@eggjs/module-test-util/StandaloneTestUtil'; | ||
| import { TestUtils } from '../Utils.ts'; | ||
| import { httpAdviceExecutionLog } from '../fixtures/http/HttpTestAdvice.ts'; | ||
|
|
||
| describe('standalone/service-worker/test/http/router.test.ts', () => { | ||
| let app: ServiceWorkerApp; | ||
| let server: Server; | ||
|
|
||
| before(async function() { | ||
| if (StandaloneTestUtil.skipOnNode()) { | ||
| return this.skip(); | ||
| } | ||
| ({ app, server } = await TestUtils.createFetchApp('http')); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| server?.close(); | ||
| await app?.destroy(); | ||
| }); | ||
|
|
||
| it('should get work', async () => { | ||
| await httpRequest(server) | ||
| .get('/hello') | ||
| .expect(200, 'hello'); | ||
| }); | ||
|
|
||
| it('should post work', async () => { | ||
| await httpRequest(server) | ||
| .post('/echo') | ||
| .send({ name: 'tegg' }) | ||
| .expect(200, { | ||
| success: true, | ||
| data: { name: 'tegg' }, | ||
| }); | ||
| }); | ||
|
|
||
| it('should return 404 with invalid path', async () => { | ||
| await httpRequest(server) | ||
| .get('/invalid-path') | ||
| .expect(404); | ||
| }); | ||
|
|
||
| it('should execute AOP middleware for HTTP controller', async () => { | ||
| httpAdviceExecutionLog.length = 0; | ||
|
|
||
| await httpRequest(server) | ||
| .get('/middleware/aop') | ||
| .expect(200, { msg: 'hello' }); | ||
|
|
||
| assert(httpAdviceExecutionLog.length > 0, 'middleware should have been executed'); | ||
| assert(httpAdviceExecutionLog.includes('before'), 'middleware before should have been called'); | ||
| assert(httpAdviceExecutionLog.includes('after'), 'middleware after should have been called'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'package.json' tegg/core/test-util | head -5 | xargs -I{} sh -c 'echo "--- {} ---"; cat {} | head -5'
rg -nP "from '@eggjs/module-test-util" tegg/
rg -nP "^\s*(describe|it|before|after)\(" tegg/standalone/service-worker/test -g '*.test.ts' | head -20Repository: eggjs/egg
Length of output: 3951
🏁 Script executed:
# Check vitest configuration for tegg
find . -name 'vitest.config.ts' -o -name 'vitest.config.js' | grep -E '(vitest|tegg)' | head -5Repository: eggjs/egg
Length of output: 247
🏁 Script executed:
# Check actual test files to see import patterns
rg -l "from ['\"]vitest['\"]" tegg/ --type ts | head -10Repository: eggjs/egg
Length of output: 517
🏁 Script executed:
# Check if any tegg test files import test functions from vitest
rg "import.*\{.*describe.*\}" tegg/ --type ts | head -10Repository: eggjs/egg
Length of output: 1180
🏁 Script executed:
# Look at the vitest config in repo root
cat vitest.config.ts | head -100Repository: eggjs/egg
Length of output: 1668
🏁 Script executed:
# Check a test file that imports from vitest to see if/how they use skip
rg -A5 "beforeAll|beforeEach" tegg/plugin/tegg/test/Inject.test.ts | head -30Repository: eggjs/egg
Length of output: 922
🏁 Script executed:
# Check if any Vitest tests use skip() and how they do it
rg "\.skip\(\)" tegg/ --type ts | head -10Repository: eggjs/egg
Length of output: 627
🏁 Script executed:
# Check all test files in standalone/service-worker to see if they all have same issue
find tegg/standalone/service-worker/test -name '*.test.ts' -exec grep -l "^import.*from.*vitest" {} \;Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check how skip is done in other Vitest tests
rg "skip\(\)" tegg/plugin/tegg/test/ --type ts -B2 -A2 | head -30Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Verify the exact Vitest lifecycle hook names in documentation or examples
rg "beforeAll|beforeEach" tegg/plugin/config/test/ReadModule.test.ts -B2 -A10Repository: eggjs/egg
Length of output: 603
🏁 Script executed:
# Check all standalone/service-worker tests to confirm they ALL have the same pattern issue
find tegg/standalone/service-worker/test -name '*.test.ts' | xargs grep -l "before\|after" | wc -lRepository: eggjs/egg
Length of output: 54
Update test to use Vitest imports instead of Mocha globals.
The test uses describe/before/after/it as globals (Mocha style) rather than importing from vitest. Per coding guidelines, all test files must import test functions directly from vitest. Replace before/after with beforeAll/afterAll and change this.skip() to use the context parameter: ctx.skip().
♻️ Proposed fix
import assert from 'node:assert';
import { Server } from 'node:http';
+import { describe, it, beforeAll, afterAll } from 'vitest';
import httpRequest from 'supertest';
...
- before(async function() {
- if (StandaloneTestUtil.skipOnNode()) {
- return this.skip();
- }
+ beforeAll(async (ctx) => {
+ if (StandaloneTestUtil.skipOnNode()) {
+ ctx.skip();
+ return;
+ }
({ app, server } = await TestUtils.createFetchApp('http'));
});
- after(async () => {
+ afterAll(async () => {
server?.close();
await app?.destroy();
});Note: All test files in tegg/standalone/service-worker/test/ have this same issue and need the same corrections.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import assert from 'node:assert'; | |
| import { Server } from 'node:http'; | |
| import httpRequest from 'supertest'; | |
| import { ServiceWorkerApp } from '../../src/ServiceWorkerApp.ts'; | |
| import { StandaloneTestUtil } from '@eggjs/module-test-util/StandaloneTestUtil'; | |
| import { TestUtils } from '../Utils.ts'; | |
| import { httpAdviceExecutionLog } from '../fixtures/http/HttpTestAdvice.ts'; | |
| describe('standalone/service-worker/test/http/router.test.ts', () => { | |
| let app: ServiceWorkerApp; | |
| let server: Server; | |
| before(async function() { | |
| if (StandaloneTestUtil.skipOnNode()) { | |
| return this.skip(); | |
| } | |
| ({ app, server } = await TestUtils.createFetchApp('http')); | |
| }); | |
| after(async () => { | |
| server?.close(); | |
| await app?.destroy(); | |
| }); | |
| it('should get work', async () => { | |
| await httpRequest(server) | |
| .get('/hello') | |
| .expect(200, 'hello'); | |
| }); | |
| it('should post work', async () => { | |
| await httpRequest(server) | |
| .post('/echo') | |
| .send({ name: 'tegg' }) | |
| .expect(200, { | |
| success: true, | |
| data: { name: 'tegg' }, | |
| }); | |
| }); | |
| it('should return 404 with invalid path', async () => { | |
| await httpRequest(server) | |
| .get('/invalid-path') | |
| .expect(404); | |
| }); | |
| it('should execute AOP middleware for HTTP controller', async () => { | |
| httpAdviceExecutionLog.length = 0; | |
| await httpRequest(server) | |
| .get('/middleware/aop') | |
| .expect(200, { msg: 'hello' }); | |
| assert(httpAdviceExecutionLog.length > 0, 'middleware should have been executed'); | |
| assert(httpAdviceExecutionLog.includes('before'), 'middleware before should have been called'); | |
| assert(httpAdviceExecutionLog.includes('after'), 'middleware after should have been called'); | |
| }); | |
| }); | |
| import assert from 'node:assert'; | |
| import { Server } from 'node:http'; | |
| import { describe, it, beforeAll, afterAll } from 'vitest'; | |
| import httpRequest from 'supertest'; | |
| import { ServiceWorkerApp } from '../../src/ServiceWorkerApp.ts'; | |
| import { StandaloneTestUtil } from '@eggjs/module-test-util/StandaloneTestUtil'; | |
| import { TestUtils } from '../Utils.ts'; | |
| import { httpAdviceExecutionLog } from '../fixtures/http/HttpTestAdvice.ts'; | |
| describe('standalone/service-worker/test/http/router.test.ts', () => { | |
| let app: ServiceWorkerApp; | |
| let server: Server; | |
| beforeAll(async (ctx) => { | |
| if (StandaloneTestUtil.skipOnNode()) { | |
| ctx.skip(); | |
| return; | |
| } | |
| ({ app, server } = await TestUtils.createFetchApp('http')); | |
| }); | |
| afterAll(async () => { | |
| server?.close(); | |
| await app?.destroy(); | |
| }); | |
| it('should get work', async () => { | |
| await httpRequest(server) | |
| .get('/hello') | |
| .expect(200, 'hello'); | |
| }); | |
| it('should post work', async () => { | |
| await httpRequest(server) | |
| .post('/echo') | |
| .send({ name: 'tegg' }) | |
| .expect(200, { | |
| success: true, | |
| data: { name: 'tegg' }, | |
| }); | |
| }); | |
| it('should return 404 with invalid path', async () => { | |
| await httpRequest(server) | |
| .get('/invalid-path') | |
| .expect(404); | |
| }); | |
| it('should execute AOP middleware for HTTP controller', async () => { | |
| httpAdviceExecutionLog.length = 0; | |
| await httpRequest(server) | |
| .get('/middleware/aop') | |
| .expect(200, { msg: 'hello' }); | |
| assert(httpAdviceExecutionLog.length > 0, 'middleware should have been executed'); | |
| assert(httpAdviceExecutionLog.includes('before'), 'middleware before should have been called'); | |
| assert(httpAdviceExecutionLog.includes('after'), 'middleware after should have been called'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/standalone/service-worker/test/http/router.test.ts` around lines 1 - 58,
Replace Mocha globals with Vitest imports and context usage: import describe,
it, beforeAll, afterAll from 'vitest' (or use named imports) and change the
lifecycle hooks in this file so that before -> beforeAll and after -> afterAll;
update the beforeAll callback to accept a ctx parameter and replace this.skip()
with ctx.skip(); ensure assertions and TestUtils.createFetchApp usage remain
unchanged and keep references to httpAdviceExecutionLog, ServiceWorkerApp, and
server as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores