src/mcp_v2/controllers/mcp-v2.controller.ts (3)

44-44: Unnecessary non-null assertion on apiKey.

apiKey is already validated as truthy on Line 33 (the function returns early if falsy). The ! assertion on Line 44 is redundant.

Suggested fix

-            await this.sessionStore.setApiKey(newSessionId, apiKey!);
+            await this.sessionStore.setApiKey(newSessionId, apiKey);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/controllers/mcp-v2.controller.ts` at line 44, The non-null
assertion on apiKey is redundant; update the controller code that calls
this.sessionStore.setApiKey(newSessionId, apiKey!) to pass apiKey directly
(remove the trailing '!') since apiKey is already checked earlier, e.g., in the
method where newSessionId and apiKey are used; keep the call as
this.sessionStore.setApiKey(newSessionId, apiKey) so the types remain consistent
and no needless assertion is present.

---

17-20: @ApiExcludeEndpoint() overrides the other Swagger decorators.

@ApiExcludeEndpoint() removes this endpoint from the generated Swagger document, making @ApiOperation and @ApiResponse on Lines 18-19 dead code. Either remove the Swagger decorators or remove @ApiExcludeEndpoint() depending on the intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/controllers/mcp-v2.controller.ts` around lines 17 - 20, The
decorators on the MCP v2 controller are contradictory: `@ApiExcludeEndpoint`()
hides the endpoint from Swagger while `@ApiOperation` and `@ApiResponse` on the same
handler are therefore ineffective; decide the intent and make them consistent—if
you want the endpoint documented, remove `@ApiExcludeEndpoint`() from the
controller (retain `@ApiOperation` and `@ApiResponse`), otherwise remove the
`@ApiOperation` and `@ApiResponse` decorators (keep `@ApiExcludeEndpoint`) so there
are no dead annotations; look for the decorators on the method where `@All`(),
`@ApiOperation`, `@ApiResponse`, and `@ApiExcludeEndpoint` are applied to update
accordingly.

---

29-63: No horizontal scaling: session state is process-local.

The transports Map is local to this controller instance. In a multi-instance deployment, a returning client with a valid mcp-session-id could hit a different instance that doesn't have the transport, causing it to fall into the "new session" path and fail with Missing x-api-key.

If multi-instance deployment is planned, consider sticky sessions (e.g., via load balancer affinity on mcp-session-id) or document this as a single-instance constraint.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/controllers/mcp-v2.controller.ts` around lines 29 - 63, The
transports Map (this.transports) is process-local so a returning sessionId may
not be found on other instances; update the handler to avoid silently treating a
missing transport as a "new session" and either (A) require/declare sticky
sessions: when sessionId is present but this.transports.has(sessionId) is false,
respond with a clear 410/400 JSON-RPC error referencing the missing session and
instruct the client/operator to use load-balancer affinity on mcp-session-id, or
(B) implement a distributed session-to-instance registry (reuse sessionStore to
persist the owning instance id when onsessioninitialized in
StreamableHTTPServerTransport and, on a missing transport in transport lookup,
proxy/forward the request to the recorded instance or return a redirect/error).
Modify the code paths around sessionId lookup, the transport creation block, and
onsessioninitialized/transport.onclose to set and remove the instance id in the
shared sessionStore so other instances can detect and handle missing transports.

src/main.ts (1)

12-21: enableCors is computed but never used — CORS is always enabled.

Line 13 computes enableCors from the config, but it's never checked before app.enableCors(...) on Line 16. CORS is unconditionally applied regardless of the ENABLE_CORS setting.

Suggested fix

Either remove the dead variable (as the comment says "always enable for MCP compatibility") or actually gate the call:

-  const enableCors = configService.get<string>('ENABLE_CORS') !== 'false';
-  const origins = configService.get<string>('CORS_ORIGINS')?.split(',') || ['*'];
-
-  app.enableCors({
+  const origins = configService.get<string>('CORS_ORIGINS')?.split(',') || ['*'];
+  app.enableCors({

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main.ts` around lines 12 - 21, The variable enableCors is computed but
never used, so update the logic around app.enableCors: either remove the unused
enableCors variable and its comment if CORS should always be enabled, or use
enableCors to gate the call to app.enableCors (e.g. if (!enableCors) skip
calling app.enableCors). Locate the computation of enableCors and the
app.enableCors(...) invocation in main.ts and implement the chosen option so the
ENABLE_CORS config actually controls whether app.enableCors is invoked.

src/mcp_v2/services/mcp-v2.service.ts (1)

89-115: Magic command numbers reduce readability.

The numeric command codes (1, 28, 29, 20, 17) are hardcoded without explanation. Consider extracting them as named constants for maintainability:

Example

const COMMAND_CODES = {
  SWITCH: 1,
  BRIGHTNESS: 28,
  KELVIN: 29,
  TEMPERATURE: 20,
  MODE: 17,
} as const;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/services/mcp-v2.service.ts` around lines 89 - 115, Replace the
magic numeric command codes used when building the command array for args.action
by extracting them into clearly named constants (e.g., SWITCH, BRIGHTNESS,
KELVIN, TEMPERATURE, MODE) and then use those constants when constructing the
command arrays (replace occurrences of 1, 28, 29, 20, 17 with the corresponding
named constants); update the switch handling of args.action and the command
variable to reference these constants so the intent is clear and maintainability
improved.

src/mcp_v2/mcp-v2.module.ts (1)

9-14: Module wiring looks clean overall.

Two minor observations:

1. ConfigModule is already isGlobal: true in AppModule (see src/app.module.ts Line 12), so the import here is redundant (harmless, but unnecessary).
2. HttpModule is imported bare here, while AppModule registers it with timeout: 30000 and maxRedirects: 5. This means ApiClientV2Service gets an unconfigured HttpService instance. This is currently fine because ApiClientV2Service sets its own per-request timeout, but it's worth being aware of if you later rely on module-level Axios defaults.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/mcp-v2.module.ts` around lines 9 - 14, Remove the redundant import
of ConfigModule from the MCP V2 module since it is already globally imported in
AppModule. Additionally, consider importing HttpModule with the same
configuration used in AppModule (timeout and maxRedirects) to ensure consistent
HttpService settings for ApiClientV2Service, especially if default Axios
settings might be relied on later.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/mcp_v2/services/mcp-v2.service.ts (1)

90-92: Duplicate userId resolution logic — already flagged in a prior review.

The inline findUserId call + multi-shape fallback (r?.userId || r?.user_id || r?.data?.userId) is repeated here (lines 90-92) and again at lines 110-112, identical to the pattern already raised in the previous review for find_user_id. The prior suggestion to extract a private resolveUserId(apiKey, data) helper still applies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/services/mcp-v2.service.ts` around lines 90 - 92, Extract a
private helper method, e.g. private async resolveUserId(apiKey, data):
Promise<string>, that performs the post to '/api/v2.0/iot-core/user/findUserId'
via this.apiClient.post(apiPath, apiKey, { data }), normalizes the response into
a single uid using the existing shape checks (r?.userId || r?.user_id ||
r?.data?.userId) and throws the same Error('UserId not found') when missing;
then replace the duplicated inline blocks in this service with calls to
this.resolveUserId(apiKey, data) to centralize logic and remove the repeated
fallback code.

src/mcp_v2/services/mcp-v2.service.ts (2)

56-66: init_api_key throws raw errors while every other tool returns structured { isError: true } objects.

All other handlers are wrapped by withApiKey, which catches thrown errors and maps them to { isError: true, content: [...] }. init_api_key sits outside that wrapper and throws directly. If the MCP SDK's internal handler dispatch doesn't catch and re-wrap thrown errors the same way, the client will receive an unstructured error response. Consider wrapping the body in a try/catch that mirrors the withApiKey convention:

♻️ Proposed fix — consistent error-response shape

-    server.registerTool('init_api_key', ToolsListV2.init_api_key, async (args: any, extra: any) => {
-      const apiKey = args?.apiKey;
-      const sessionId = extra?.sessionId;
-      if (!sessionId) throw new Error('Missing sessionId');
-      if (!apiKey) throw new Error('apiKey is required');
-
-      await this.sessionStore.setApiKey(sessionId, apiKey);
-      this.logger.log(`Initialized apiKey for session ${sessionId}`);
-
-      return { content: [{ type: 'text', text: JSON.stringify({ success: true, message: 'apiKey initialized for session' }) }] } as any;
-    });
+    server.registerTool('init_api_key', ToolsListV2.init_api_key, async (args: any, extra: any) => {
+      try {
+        const apiKey = args?.apiKey;
+        const sessionId = extra?.sessionId;
+        if (!sessionId) return { isError: true, content: [{ type: 'text', text: 'Missing sessionId in request' }] } as any;
+        if (!apiKey) return { isError: true, content: [{ type: 'text', text: 'apiKey is required' }] } as any;
+
+        await this.sessionStore.setApiKey(sessionId, apiKey);
+        this.logger.log(`Initialized apiKey for session ${sessionId}`);
+
+        return { content: [{ type: 'text', text: JSON.stringify({ success: true, message: 'apiKey initialized for session' }) }] } as any;
+      } catch (err: any) {
+        this.logger.error(`init_api_key error: ${err?.message || err}`, err?.stack);
+        return { isError: true, content: [{ type: 'text', text: `Error: ${err?.message || err}` }] } as any;
+      }
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/services/mcp-v2.service.ts` around lines 56 - 66, The init_api_key
tool handler currently throws raw Errors (e.g., "Missing sessionId", "apiKey is
required") instead of returning the structured error object used elsewhere;
update the async handler passed to server.registerTool('init_api_key',
ToolsListV2.init_api_key, ...) to wrap its body in try/catch, call
this.sessionStore.setApiKey(...) and this.logger.log(...) in the try, and in
catch return the same shaped response used by withApiKey (an object with
isError: true and content: [...] containing the error message) so clients always
receive a consistent error-response shape.

---

50-52: withApiKey catch block logs nothing server-side — errors are silently swallowed.

Any exception thrown inside a withApiKey-wrapped handler (including upstream API failures, programming errors, and thrown validation errors) is caught here and returned to the client as a text message, but never logged with this.logger. This makes production debugging very difficult.

♻️ Proposed fix — add server-side error logging

         } catch (err: any) {
+          this.logger.error(`Tool handler error: ${err?.message || err}`, err?.stack);
           return { isError: true, content: [{ type: 'text', text: `Error: ${err?.message || err}` }] } as any;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_v2/services/mcp-v2.service.ts` around lines 50 - 52, The catch in the
withApiKey-wrapped handler currently returns the error to the client but never
logs it; update the catch block inside withApiKey (in mcp-v2.service.ts) to call
this.logger.error with the error and its stack/metadata (e.g.,
this.logger.error('withApiKey handler error', err)) before returning the
existing { isError... } response so server-side exceptions are recorded for
debugging.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro