fix: Resolve ESLint issues in server (empty function, unsafe body access)

Author: alvinveroy

CHANGELOG.md

@@ -97,6 +97,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 +    - Resolved runtime error `TypeError: vi.mocked(...).mockReturnValue is not a function` in `src/tests/server.test.ts` by directly casting `http.createServer` to `vi.Mock` and calling `mockReturnValue` on it, bypassing issues with `vi.mocked()`.
 +    - Fixed TypeScript error `TS2345` for `process.exit` mock in `src/tests/server.test.ts` by casting `vi.fn()` to the expected `(code?: number) => never` signature.
     - Addressed TypeScript error `TS2345` concerning `http.Server` type compatibility in `src/tests/server.test.ts` by enhancing the `mockHttpServer` object with more properties common to `http.Server` and using a type assertion (`as unknown as http.Server`).
+- **ESLint Fixes (server.ts - Empty Function & Unsafe Access) (Git Commit ID: [GIT_COMMIT_ID_PLACEHOLDER]):**
+    - Resolved ESLint warning `@typescript-eslint/no-empty-function` in `src/lib/server.ts` for `httpServerSetupReject` initialization by adding an `eslint-disable-next-line` comment, as this is a valid pattern for initializing promise reject functions.
+    - Addressed ESLint errors `@typescript-eslint/no-unsafe-assignment` and `@typescript-eslint/no-unsafe-member-access` related to `req.body.id` access in MCP error responses. Introduced a `RequestBodyWithId` interface and type-safe checks before accessing `req.body.id` to ensure `req.body` is an object and contains the `id` property.
 - **Linting Finalization & Build Stability (Git Commit ID: [GIT_COMMIT_ID_PLACEHOLDER]):**
     - Resolved all ESLint errors in `src/tests/server.test.ts`.
     - The primary fix involved ensuring that `eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion` directives are present and preserved before all `await importOriginal() as typeof import(...)` type assertions in mock factories. These assertions are essential for ESLint's subsequent type-checking rules (`no-unsafe-return`, `no-unsafe-assignment`) to function correctly, even if `tsc` alone might not strictly require the disable.

RETROSPECTION.md

@@ -767,3 +767,24 @@
 - Continue to use `eslint-disable-next-line @typescript-eslint/require-await` with justification for functions that are intentionally `async` without current `await` expressions for API consistency or future-proofing.
 
 ---
+# Retrospection for ESLint Fixes (server.ts - Empty Function & Unsafe Access) (Git Commit ID: [GIT_COMMIT_ID_PLACEHOLDER])
+
+## What went well?
+- ESLint clearly identified the specific lines and rules causing the warnings and errors.
+- The `no-empty-function` warning was correctly identified as a case where disabling the rule for a specific pattern (promise rejector initialization) is appropriate.
+- The `no-unsafe-assignment` and `no-unsafe-member-access` errors highlighted a genuine type safety concern with accessing `req.body.id` without proper checks.
+
+## What could be improved?
+- **Type Safety for `req.body`:** While `express.json()` middleware parses the body, its type remains `any` by default. Consistently applying type guards or casting to a known interface (like `RequestBodyWithId`) for `req.body` access improves type safety throughout the Express route handlers.
+- **ESLint Rule Configuration:** For `no-empty-function`, if this pattern of initializing promise rejectors is common, a more global ESLint configuration (e.g., allowing empty functions with specific names or in specific contexts) could be considered, though targeted disables are also fine.
+
+## What did we learn?
+- **`no-empty-function`:** This rule is generally useful but has valid exceptions, such as initializing placeholder functions that will be reassigned. `eslint-disable` is appropriate here.
+- **`no-unsafe-assignment` / `no-unsafe-member-access`:** These rules are crucial for maintaining type safety when dealing with `any` types. Accessing properties on `req.body` (which is often `any` after middleware parsing) requires careful type checking or casting to a more specific type.
+- **Type Guards/Checks for `req.body`:** Before accessing properties like `id` on `req.body`, it's important to verify that `req.body` is an object and actually contains the property. This prevents runtime errors and satisfies ESLint's safety rules.
+
+## Action Items / Follow-ups
+- Review other instances of `req.body` access in Express route handlers to ensure similar type safety measures (type guards or casting with checks) are applied.
+- Consider if a project-wide helper type or type guard for Express request bodies that might contain an `id` would be beneficial for consistency.
+
+---

src/lib/server.ts

@@ -36,6 +36,11 @@ import { VERSION } from "./version";
 import { getOrCreateSession, addQuery, addSuggestion, updateContext, getRecentQueries, getRelevantResults } from "./state";
 
 // Define this interface at the top level of the file, e.g., after imports
+interface RequestBodyWithId {
+  id?: unknown; // id can be string, number, or null
+  [key: string]: unknown; // Allow other properties
+}
+
 interface PingResponseData {
   service?: string;
   status?: string;
@@ -359,7 +364,8 @@ export async function startServer(repoPath: string): Promise<void> {
 
   logger.info("Starting CodeCompass MCP server...");
 
-  let httpServerSetupReject: (reason?: unknown) => void = () => {}; // Initialize with a no-op
+  // eslint-disable-next-line @typescript-eslint/no-empty-function -- Initializing with a no-op, will be reassigned.
+  let httpServerSetupReject: (reason?: unknown) => void = () => {}; 
   const httpServerSetupPromise = new Promise<void>((_resolve, reject) => {
     httpServerSetupReject = reject;
   });
@@ -490,10 +496,11 @@ export async function startServer(repoPath: string): Promise<void> {
         transport = newTransport;
       } else {
         logger.warn(`MCP POST: Bad Request. No valid session ID and not an init request.`);
+        const bodyWithId = req.body as RequestBodyWithId;
         res.status(400).json({
           jsonrpc: '2.0',
           error: { code: -32000, message: 'Bad Request: No valid session ID or not an init request.' },
-          id: req.body?.id || null,
+          id: (typeof bodyWithId === 'object' && bodyWithId !== null && 'id' in bodyWithId) ? bodyWithId.id : null,
         });
         return;
       }
@@ -502,8 +509,9 @@ export async function startServer(repoPath: string): Promise<void> {
       } catch (transportError) {
         logger.error("Error handling MCP POST request via transport:", transportError);
         if (!res.headersSent) {
+          const bodyWithId = req.body as RequestBodyWithId;
           res.status(500).json({
-            jsonrpc: '2.0', error: { code: -32000, message: 'Internal MCP transport error.' }, id: req.body?.id || null,
+            jsonrpc: '2.0', error: { code: -32000, message: 'Internal MCP transport error.' }, id: (typeof bodyWithId === 'object' && bodyWithId !== null && 'id' in bodyWithId) ? bodyWithId.id : null,
           });
         }
       }