feat: add distributed deployment adapter and related configuration files#350
Merged
frontegg-david merged 21 commits intomainfrom Apr 9, 2026
Merged
feat: add distributed deployment adapter and related configuration files#350frontegg-david merged 21 commits intomainfrom
frontegg-david merged 21 commits intomainfrom
Conversation
…ization in middleware
…g strategies and tool consent types
# Conflicts: # libs/auth/src/authorities/authorities.engine.ts # libs/auth/src/authorities/authorities.types.ts # libs/auth/src/context/__tests__/frontmcp-auth-context.spec.ts # libs/auth/src/context/frontmcp-auth-context.ts # libs/observability/src/plugin/observability.plugin.ts # libs/sdk/src/agent/flows/call-agent.flow.ts # libs/sdk/src/auth/flows/oauth.authorize.flow.ts # libs/sdk/src/auth/flows/oauth.token.flow.ts # libs/sdk/src/auth/flows/session.verify.flow.ts # libs/sdk/src/common/entries/scope.entry.ts # libs/sdk/src/common/interfaces/execution-context.interface.ts # libs/sdk/src/context/frontmcp-context.ts # libs/sdk/src/prompt/flows/get-prompt.flow.ts # libs/sdk/src/prompt/flows/prompts-list.flow.ts # libs/sdk/src/resource/flows/read-resource.flow.ts # libs/sdk/src/resource/flows/resources-list.flow.ts # libs/sdk/src/scope/scope.instance.ts # libs/sdk/src/tool/flows/call-tool.flow.ts
Contributor
📝 WalkthroughWalkthroughAdds a typed frontmcp config system and loader; introduces distributed HA (heartbeat, session takeover, notification relay, HaManager) with transport/session integration; centralizes machine‑id in utils; adds cookie/CSP HTTP helpers; broad import/type reorganizations and import-merge tooling; many tests, E2E apps, docs, and linting rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant LB
participant Pod
participant Redis
participant OtherPod
Client->>LB: HTTP request (session cookie / affinity)
LB->>Pod: Forward request
Pod->>Pod: onInitialize -> set machine-id header & Set-Cookie (if distributed)
Pod->>Redis: GET sessionKey
Redis-->>Pod: storedSession (nodeId = other-node)
Pod->>Redis: EVAL compare-and-swap (expected other-node, new this-node)
Redis-->>Pod: result (1 or 0)
alt takeover success
Pod->>Redis: PUBLISH notify:other-node {sessionId,...}
Redis->>OtherPod: deliver pub/sub message
OtherPod->>OtherPod: Relay handler processes message
Pod-->>Client: Response (machine-id header, Set-Cookie)
else takeover failure
Pod-->>Client: Abort / SessionClaimConflictError
end
sequenceDiagram
autonumber
participant App
participant Loader
participant FS
participant Validator
App->>Loader: loadFrontMcpConfig(cwd)
Loader->>FS: search frontmcp.config.(ts|js|json|mjs|cjs)
alt file found
FS-->>Loader: file/module
Loader->>Loader: require/import or parse JSON
else not found
Loader->>FS: read package.json
FS-->>Loader: package.json
Loader->>Loader: derive minimal config (name/version, node deployment)
end
Loader->>Validator: validateConfig(raw)
Validator-->>Loader: FrontMcpConfigParsed
Loader-->>App: Promise<FrontMcpConfigParsed>
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/auth/session/transport-session.manager.ts (1)
132-145:⚠️ Potential issue | 🔴 CriticalUse the factory pattern to instantiate the session store instead of direct construction.
Line 141 directly instantiates
RedisSessionStore, but the codebase provides factory functions (createSessionStore()orcreateSessionStoreSync()) that handle store creation with lazy-loaded dependencies. UsecreateSessionStoreSync()to maintain synchronous initialization while following the established factory pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/auth/session/transport-session.manager.ts` around lines 132 - 145, The constructor in transport-session.manager.ts directly instantiates RedisSessionStore (and InMemorySessionStore) instead of using the project factory; replace direct new RedisSessionStore(...) with a call to createSessionStoreSync(...) (and use the factory for the memory fallback) so store creation follows the lazy-loaded factory pattern; locate the constructor function, remove the direct new RedisSessionStore usage and call createSessionStoreSync(config) (or createSessionStoreSync({ mode: config.mode, store: config.store, config: config.config })) to obtain the SessionStore instance while keeping the existing stateless/in-memory allocation behavior.libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts (1)
11-49:⚠️ Potential issue | 🟠 MajorStop using direct Node crypto/custom crypto in this test mock.
This mock still uses
require('crypto')and reimplements crypto helpers; switch to@frontmcp/utilsimplementations and only overridegetMachineId.Suggested patch
jest.mock('@frontmcp/utils', () => { - const crypto = require('crypto'); + const actual = jest.requireActual('@frontmcp/utils'); return { + ...actual, getMachineId: mockGetMachineId, - sha256: (data: Uint8Array) => { - const hash = crypto.createHash('sha256').update(data); - return new Uint8Array(hash.digest()); - }, - encryptValue: jest.fn((obj: unknown, key: Uint8Array) => { - // Simplified AES-256-GCM encryption for testing - const iv = crypto.randomBytes(12); - const cipher = crypto.createCipheriv('aes-256-gcm', Buffer.from(key), iv); - const plaintext = JSON.stringify(obj); - let encrypted = cipher.update(plaintext, 'utf8', 'base64'); - encrypted += cipher.final('base64'); - const tag = cipher.getAuthTag(); - return { - iv: iv.toString('base64'), - tag: tag.toString('base64'), - data: encrypted, - }; - }), - decryptValue: jest.fn((envelope: { iv: string; tag: string; data: string; alg: string }, key: Uint8Array) => { - const iv = Buffer.from(envelope.iv, 'base64'); - const tag = Buffer.from(envelope.tag, 'base64'); - const decipher = crypto.createDecipheriv('aes-256-gcm', Buffer.from(key), iv); - decipher.setAuthTag(tag); - let decrypted = decipher.update(envelope.data, 'base64', 'utf8'); - decrypted += decipher.final('utf8'); - return JSON.parse(decrypted); - }), - // Required by AuthInternalError base class for errorId generation - randomBytes: (n: number) => crypto.randomBytes(n), - bytesToHex: (bytes: Uint8Array) => Buffer.from(bytes).toString('hex'), - getEnv: (key: string) => process.env[key], - isProduction: () => process.env['NODE_ENV'] === 'production', }; });Per coding guidelines "
**/*.{ts,tsx}: Always use@frontmcp/utilsfor cryptographic operations. Never usenode:cryptodirectly or implement custom crypto functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts` around lines 11 - 49, The test mock currently requires node:crypto and reimplements crypto helpers inside the jest.mock for '@frontmcp/utils' (see the mock block that defines sha256, encryptValue, decryptValue, randomBytes, bytesToHex, getEnv, isProduction and mockGetMachineId); replace this with importing the real implementations from '@frontmcp/utils' and only override getMachineId (mockGetMachineId) in the jest.mock return value so all cryptographic operations (sha256, encryptValue, decryptValue, randomBytes, bytesToHex) come from the official library rather than node:crypto or custom code.
🧹 Nitpick comments (18)
libs/utils/src/env/__tests__/runtime-context-distributed.spec.ts (1)
27-33: Inline cleanup is redundant givenafterEachrestoration.The
delete process.env['VERCEL']on line 32 and line 48 are unnecessary sinceafterEachalready restores the original environment. Consider removing for clarity.♻️ Remove redundant cleanup
it('should detect serverless from platform env vars when FRONTMCP_DEPLOYMENT_MODE not set', () => { delete process.env['FRONTMCP_DEPLOYMENT_MODE']; process.env['VERCEL'] = '1'; const ctx = detectRuntimeContext(); expect(ctx.deployment).toBe('serverless'); - delete process.env['VERCEL']; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/utils/src/env/__tests__/runtime-context-distributed.spec.ts` around lines 27 - 33, Remove the redundant manual env cleanup in the test "should detect serverless from platform env vars when FRONTMCP_DEPLOYMENT_MODE not set": delete the two lines `delete process.env['VERCEL']` (one inside this it block and the one at line 48) because afterEach already restores process.env; simply set process.env['VERCEL']='1', call detectRuntimeContext(), assert ctx.deployment === 'serverless', and let the existing afterEach restore the environment. Ensure you only modify the test in runtime-context-distributed.spec.ts and keep the call to detectRuntimeContext() and the expectation intact.scripts/merge-imports.ts (1)
107-109: Silent error swallowing may hide genuine failures.The empty catch block discards all errors including I/O failures and permission issues. Consider at least logging to stderr for debugging lint-staged issues.
♻️ Proposed minimal error logging
for (const f of files) { try { processFile(f); - } catch { - /* skip errors */ + } catch (e) { + console.error(`[merge-imports] Failed to process ${f}:`, e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/merge-imports.ts` around lines 107 - 109, The empty catch block after the try in the mergeImports routine silently swallows all errors; change it to log the caught error (e.g., console.error or process.stderr.write) with context so I/O/permission failures are visible (for example: catch (err) { console.error('merge-imports error:', err); }) – reference the try/catch in mergeImports (or the top-level import-merging routine) and ensure you keep the minimal logging without changing control flow.libs/utils/src/env/runtime-context.ts (1)
111-114: Consider a typed globalThis check for cleaner type safety.The
as anycasts work but could be replaced with a more explicit type guard pattern for better maintainability.♻️ Optional: typed global detection
+interface GlobalWithRuntimes { + Bun?: unknown; + Deno?: unknown; +} + function detectRuntime(): string { - if (typeof globalThis !== 'undefined' && 'Bun' in (globalThis as any)) return 'bun'; - - if (typeof globalThis !== 'undefined' && 'Deno' in (globalThis as any)) return 'deno'; + const g = globalThis as GlobalWithRuntimes; + if (typeof globalThis !== 'undefined' && 'Bun' in g) return 'bun'; + if (typeof globalThis !== 'undefined' && 'Deno' in g) return 'deno'; if (isEdgeRuntime()) return 'edge'; return 'node'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/utils/src/env/runtime-context.ts` around lines 111 - 114, Replace the unsafe "as any" casts in the globalThis checks with a small typed guard and use it in the runtime detection: add a reusable type guard (e.g., hasGlobalProp or isGlobalWithProp) and use it to check for 'Bun' and 'Deno' instead of casting; update the lines that currently check " 'Bun' in (globalThis as any) " and " 'Deno' in (globalThis as any) " to call the guard, leaving the isEdgeRuntime() call unchanged so runtime detection remains the same but with proper type safety.libs/auth/src/machine-id/index.ts (1)
1-4: Consider deprecating this compatibility re-export.The coding guidelines state to "avoid legacy or compatibility re-exports in barrel files." While the comment indicates this is for internal auth usage, consumers may still import from this path. Consider adding a
@deprecatedJSDoc tag to guide consumers toward the canonical@frontmcp/utilsimport.♻️ Proposed deprecation notice
// Machine ID has moved to `@frontmcp/utils`. // Import directly from '@frontmcp/utils' instead. // This file is kept for internal auth usage only. + +/** `@deprecated` Import from '@frontmcp/utils' instead */ export { getMachineId, setMachineIdOverride } from '@frontmcp/utils';As per coding guidelines: "Avoid legacy or compatibility re-exports in barrel files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/machine-id/index.ts` around lines 1 - 4, Add a deprecation JSDoc to this compatibility re-export so consumers are guided to the canonical package: annotate the exported symbols getMachineId and setMachineIdOverride with a `@deprecated` comment that says these are re-exports and directs users to import from '@frontmcp/utils' instead; ensure the JSDoc appears above the export statement and includes a short replacement note and since/version info if applicable so IDEs and TypeScript will surface the deprecation.libs/utils/src/machine-id/machine-id.ts (1)
58-58: Consider tightening the UUID validation regex.The current regex
/^[0-9a-f-]{32,36}$/iaccepts strings that don't conform to the standard UUID format (e.g., 32 hex chars without hyphens, or arbitrary hyphen placement). If strict UUID format is expected, consider validating the canonical format.♻️ Proposed stricter UUID validation
- if (/^[0-9a-f-]{32,36}$/i.test(content)) return content; + // Accept both hyphenated UUID and 32-char hex (compact UUID) + if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(content) || + /^[0-9a-f]{32}$/i.test(content)) return content;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/utils/src/machine-id/machine-id.ts` at line 58, The current loose check if (/^[0-9a-f-]{32,36}$/i.test(content)) return content; allows non-canonical UUIDs; replace this validation with a stricter canonical-UUID pattern (e.g. the 8-4-4-4-12 hex groups with proper version/variant if desired) so the branch only returns true for valid UUID strings. Update the conditional in machine-id.ts that tests the variable content to use the stricter regex (or a small helper isValidUuid(content)) referencing the same symbol so invalid formats (wrong hyphen placement or arbitrary lengths) no longer pass.libs/cli/frontmcp.schema.json (1)
259-264: Minor: Use"integer"type for portRange items.For consistency with the
portfield (line 51) which uses"type": "integer", theportRangearray items should also use"integer"rather than"number"since port numbers are always integers.♻️ Suggested fix
"portRange": { "type": "array", - "items": { "type": "number" }, + "items": { "type": "integer" }, "minItems": 2, "maxItems": 2 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/frontmcp.schema.json` around lines 259 - 264, The portRange schema currently defines items as "type": "number" but ports are integers; update the portRange definition to use "type": "integer" for its items to match the existing port field and ensure consistency (modify the "portRange" schema entry where "items" is defined and align it with the "port" field's type).libs/cli/src/config/frontmcp-config.schema.ts (1)
110-117: Consider aligning minimum constraints with the JSON Schema.The Zod schema uses
.positive()for HA timing fields, which allows any value > 0. However,libs/cli/frontmcp.schema.jsonspecifies stricter minimums:
heartbeatIntervalMs: minimum 1000heartbeatTtlMs: minimum 2000takeoverGracePeriodMs: minimum 0This could cause runtime validation to accept values the JSON Schema rejects during IDE validation, or vice versa.
♻️ Suggested alignment
export const haConfigSchema = z .object({ - heartbeatIntervalMs: z.number().int().positive().optional(), - heartbeatTtlMs: z.number().int().positive().optional(), - takeoverGracePeriodMs: z.number().int().positive().optional(), + heartbeatIntervalMs: z.number().int().min(1000).optional(), + heartbeatTtlMs: z.number().int().min(2000).optional(), + takeoverGracePeriodMs: z.number().int().min(0).optional(), redisKeyPrefix: z.string().optional(), }) .strict();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/config/frontmcp-config.schema.ts` around lines 110 - 117, The haConfigSchema currently uses .positive() for heartbeatIntervalMs, heartbeatTtlMs, and takeoverGracePeriodMs which is looser than the JSON Schema; update haConfigSchema (the z.object with heartbeatIntervalMs, heartbeatTtlMs, takeoverGracePeriodMs) to use z.number().int().min(...) with the JSON Schema minima (heartbeatIntervalMs.min(1000), heartbeatTtlMs.min(2000), takeoverGracePeriodMs.min(0)) while preserving .optional() and .strict() so runtime Zod validation aligns with libs/cli/frontmcp.schema.json.libs/cli/src/config/frontmcp-config.loader.ts (1)
46-49: Consider wrapping JSON.parse for better error messages.If the JSON file is malformed,
JSON.parsethrows a genericSyntaxError. A wrapper would provide context about which file failed.♻️ Suggested improvement
if (filename.endsWith('.json')) { const content = fs.readFileSync(configPath, 'utf-8'); - return JSON.parse(content); + try { + return JSON.parse(content); + } catch (err) { + throw new Error(`Failed to parse ${configPath}: ${(err as Error).message}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/config/frontmcp-config.loader.ts` around lines 46 - 49, Wrap the JSON.parse call inside the filename.endsWith('.json') branch (where content is read from configPath) with a try/catch so malformed JSON yields a clearer error: catch the SyntaxError from JSON.parse(content) and throw or log a new Error that includes the failing configPath/filename and the original error message (preserve the original error as cause or include its message), so callers can see which file failed; update the return path to rethrow the enriched error after catching.libs/sdk/src/server/middleware/__tests__/csp.middleware.spec.ts (1)
145-158: Consider adding a test for directives without values.If the middleware is updated to support CSP directives without values (e.g.,
upgrade-insecure-requests), a corresponding test case would be valuable.📝 Example test case
it('should parse directives without values', () => { process.env['FRONTMCP_CSP_ENABLED'] = '1'; process.env['FRONTMCP_CSP_DIRECTIVES'] = "default-src 'self'; upgrade-insecure-requests"; const csp = readCspFromEnv(); expect(csp).toBeDefined(); expect(csp!.directives['upgrade-insecure-requests']).toBe(''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/server/middleware/__tests__/csp.middleware.spec.ts` around lines 145 - 158, Add a unit test that verifies readCspFromEnv correctly handles directives without values: create a new it block (e.g., "should parse directives without values") that sets process.env['FRONTMCP_CSP_ENABLED']='1' and process.env['FRONTMCP_CSP_DIRECTIVES']="default-src 'self'; upgrade-insecure-requests", call readCspFromEnv(), assert the result is defined and that csp!.directives['upgrade-insecure-requests'] === '' (and keep existing checks for default-src if desired); this ensures the parsing logic in readCspFromEnv handles value-less directives.libs/sdk/src/server/middleware/csp.middleware.ts (1)
61-73: Handle empty-value directives in header construction.If the parsing is updated to support directives without values (empty string), this function should also handle them to avoid trailing spaces.
♻️ Suggested fix
export function buildCspHeaderValue(options: CspOptions): string { const parts: string[] = []; for (const [directive, value] of Object.entries(options.directives)) { - parts.push(`${directive} ${value}`); + parts.push(value ? `${directive} ${value}` : directive); } if (options.reportUri) { parts.push(`report-uri ${options.reportUri}`); } return parts.join('; '); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/server/middleware/csp.middleware.ts` around lines 61 - 73, buildCspHeaderValue currently concatenates directives as `${directive} ${value}` which adds a trailing space when a directive's value is an empty string; update the logic in buildCspHeaderValue to output just the directive name when value === '' (or value is falsy specifically only for empty-string cases) and otherwise output `${directive} ${value}`, and still append `report-uri ${options.reportUri}` only when options.reportUri is present; adjust handling of options.directives iteration to check the value before composing the part so empty-value directives produce e.g. "upgrade-insecure-requests" instead of "upgrade-insecure-requests "..prettierrc (1)
28-28: Consider updatingimportOrderTypeScriptVersionto align with workspace TypeScript.The import-order parser is pinned to TypeScript
5.0.0while the workspace uses~5.9.2. This can cause the sort-imports plugin to fail on syntax introduced in newer TypeScript versions (e.g., decorator improvements, const type parameters). Update line 28 to"5.9"or the minor version in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.prettierrc at line 28, The import-order parser version is pinned to "5.0.0" via the importOrderTypeScriptVersion setting; update the value of importOrderTypeScriptVersion to match the workspace TypeScript major/minor (e.g., "5.9" or the specific minor in use like "5.9.2") so the sort-imports plugin can parse newer TypeScript syntax (change the importOrderTypeScriptVersion property accordingly).libs/sdk/src/scope/scope.instance.ts (1)
1201-1229: Harden HA cleanup by coveringdispose()path too.Right now HA shutdown is only in
shutdown(). Make cleanup idempotent fromdispose()as well (or callshutdown()insidedispose()), so HA resources are always released.💡 Proposed fix
async dispose(): Promise<void> { + await this.shutdown(); this.scopeProviders.dispose(); if (this.notificationService) { await this.notificationService.destroy(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/scope/scope.instance.ts` around lines 1201 - 1229, The dispose() path currently doesn't ensure HA cleanup; modify dispose() in scope.instance.ts to perform the same HA shutdown as shutdown(): either call this.shutdown() at the start of dispose(), or replicate the haManager cleanup (check this.haManager, await this.haManager.stop() in try/catch and log errors) and also clear this._channelTeardown if present so teardown is idempotent (refer to shutdown(), dispose(), this.haManager, this.haManager.stop(), this._channelTeardown, and this.logger.error to locate the code).libs/sdk/src/ha/__tests__/session-takeover.spec.ts (1)
56-56: Replace non-null assertions in tests with explicit guards.The non-null assertions bypass strict null-safety and can mask fixture or setup mistakes.
♻️ Suggested pattern
-const updated = JSON.parse(redis.store.get('mcp:transport:sess1')!); +const updatedRaw = redis.store.get('mcp:transport:sess1'); +if (!updatedRaw) { + throw new Error('Expected session value for mcp:transport:sess1'); +} +const updated = JSON.parse(updatedRaw);Also applies to lines 76, 101, and 123.
Per coding guidelines: "Avoid non-null assertions (
!) - use proper error handling instead. Fail fast by throwing specific error classes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/ha/__tests__/session-takeover.spec.ts` at line 56, The test uses a non-null assertion on the redis.store.get call (JSON.parse(redis.store.get('mcp:transport:sess1')!)) which bypasses null-safety; replace each usage (the occurrences that assign to updated and similar variables) with an explicit guard: call redis.store.get(...), check if the result is null/undefined and if so throw a descriptive error (or a specific test error class) to fail fast, otherwise pass the string into JSON.parse; update the assertions at the other occurrences (the similar calls at the other test points) to the same guard pattern so tests don't rely on `!`.apps/e2e/demo-e2e-frontmcp-config/e2e/config-schema.e2e.spec.ts (1)
142-155: Prefer static ES module imports over dynamicrequire().The helper functions
findDeploymentandgetDeploymentTargetsare imported dynamically usingrequire(). For consistency with the codebase conventions and to enable better tree-shaking and type checking, use static ES module imports at the top of the file.♻️ Proposed refactor to use ES module imports
Add to the imports at the top of the file (around line 9):
-import { validateConfig } from '@frontmcp/cli'; +import { findDeployment, getDeploymentTargets, validateConfig } from '@frontmcp/cli';Then update the test cases:
describe('helper functions', () => { it('findDeployment should find by target', () => { - const { findDeployment } = require('@frontmcp/cli'); const config = validateConfig(loadFixture('multi-target.config.json')); expect(findDeployment(config, 'cli')?.target).toBe('cli'); expect(findDeployment(config, 'vercel')).toBeUndefined(); }); it('getDeploymentTargets should return all targets', () => { - const { getDeploymentTargets } = require('@frontmcp/cli'); const config = validateConfig(loadFixture('multi-target.config.json')); expect(getDeploymentTargets(config)).toEqual(['distributed', 'cli', 'cloudflare', 'browser']); }); });Based on learnings: "Use proper ES module imports instead of
require()for SDK imports; avoid dynamic require offrontmcp/sdkmodules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-schema.e2e.spec.ts` around lines 142 - 155, Replace the dynamic require() calls for findDeployment and getDeploymentTargets with static ES module imports: add an import for { findDeployment, getDeploymentTargets } from '@frontmcp/cli' at the top of the test file and remove the require() usages inside the it blocks so the tests call the imported functions directly (ensure the imported names match the existing functions findDeployment and getDeploymentTargets).scripts/fix-imports.ts (2)
82-87: Clarify the& {}type intersection pattern.The
& {}intersection on line 86 is a TypeScript trick to narrow outnullfrom the type, but it's non-obvious. A brief comment or using a more explicit type assertion would improve readability.♻️ Optional: Use explicit NonNullable or add comment
- const imports: Array<{ lineIndex: number; parsed: ReturnType<typeof parseImportLine> & {} }> = []; + // parsed is guaranteed non-null by the if-check before pushing + const imports: Array<{ lineIndex: number; parsed: NonNullable<ReturnType<typeof parseImportLine>> }> = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fix-imports.ts` around lines 82 - 87, The declaration for imports uses a non-obvious "& {}" to exclude null from ReturnType<typeof parseImportLine>; replace that with an explicit NonNullable wrapper (e.g. Array<{ lineIndex: number; parsed: NonNullable<ReturnType<typeof parseImportLine>> }>) or alternatively add a short inline comment above the imports declaration in processFile explaining that "& {}" is used to narrow out null from parseImportLine's return type; reference the symbols processFile, parseImportLine, imports and importLineIndices when making the change.
99-104: Consider using nullish coalescing to avoid non-null assertions.The non-null assertions on lines 103 and 126 are logically safe due to the preceding
has()check andset()call, but they could be avoided with a slight refactor for stricter code safety.♻️ Optional refactor to avoid non-null assertions
const byModule = new Map<string, typeof imports>(); for (const imp of imports) { const key = imp.parsed.module; - if (!byModule.has(key)) byModule.set(key, []); - byModule.get(key)!.push(imp); + const existing = byModule.get(key); + if (existing) { + existing.push(imp); + } else { + byModule.set(key, [imp]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fix-imports.ts` around lines 99 - 104, Replace the non-null assertion on byModule.get(key)! by using a nullish-coalescing pattern: retrieve the array with const group = byModule.get(key) ?? []; push the import into group (group.push(imp)) and then set it back with byModule.set(key, group); this changes the logic around byModule, imports, key and imp (where key = imp.parsed.module) to avoid using the non-null assertion while preserving existing behavior.libs/utils/src/http/__tests__/cookie.spec.ts (1)
14-16: Consider adding test for IPv6 bracket notation.The AI summary mentions
[::1](IPv6 bracket form) is supported byisLocalhost, but only the plain::1form is tested. Adding a test case likeexpect(isLocalhost('[::1]:3000')).toBe(true)would improve coverage of the bracket-notation branch in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/utils/src/http/__tests__/cookie.spec.ts` around lines 14 - 16, Add a unit test to cover IPv6 bracket notation: in libs/utils/src/http/__tests__/cookie.spec.ts, extend the existing isLocalhost tests to include a case like calling isLocalhost with a bracketed IPv6 address and port (e.g., '[::1]:3000') and assert it returns true; this will exercise the bracket-notation parsing branch in the isLocalhost implementation.libs/sdk/src/ha/heartbeat.service.ts (1)
81-85: Note:keyscommand may not scale well.Redis
KEYSis O(N) and can block the server with large keyspaces. For production deployments with many nodes, consider usingSCANinstead. This is acceptable for moderate cluster sizes but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/ha/heartbeat.service.ts` around lines 81 - 85, The getAliveNodes method currently uses Redis KEYS which is O(N); replace it with a SCAN-based iterative approach in getAliveNodes to avoid blocking large keyspaces: use your Redis client's scan or scanStream API to iterate with a cursor, collect keys matching `${this.keyPrefix}heartbeat:*`, extract node IDs by slicing off `${this.keyPrefix}heartbeat:` (same slice logic), deduplicate if needed, and return the array; ensure the implementation respects the existing getAliveNodes signature and uses this.keyPrefix and this.redis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 15-57: The test improperly requires and uses Node's fs
(require('fs')) and calls fs.mkdtempSync, fs.writeFileSync and fs.rmSync;
replace those direct fs calls with the repository file helpers from
`@frontmcp/utils` (e.g. the utils' temp-dir and file write/remove helpers) so the
test uses the sanctioned API instead of node:fs; update the top of the spec to
import the appropriate functions from '@frontmcp/utils' and swap fs.mkdtempSync,
fs.writeFileSync and fs.rmSync calls to the corresponding helpers while keeping
the same tmpDir/path logic used around loadFrontMcpConfig.
In `@apps/e2e/demo-e2e-frontmcp-config/fixtures/invalid-name.config.json`:
- Around line 1-4: The fixture for the “invalid name” negative test currently
can fail for multiple reasons; update the invalid-name.config.json fixture so
the only schema violation is the "name" field (keep "name": "invalid name with
spaces"), and add any required valid fields (e.g., a proper "version" value and
valid "deployments" entries) so that "version" and "deployments" conform to the
schema and the test isolates the name violation.
In `@apps/e2e/demo-e2e-machine-id/e2e/machine-id.e2e.spec.ts`:
- Around line 52-57: The test title claims to verify MACHINE_ID env var priority
but the body uses setMachineIdOverride/getMachineId to test the override
mechanism; update the test to match intent by either renaming the spec to
reflect override behavior (e.g., "should respect machine id override via
setMachineIdOverride") or change the implementation to actually assert env-var
precedence by isolating module load and setting process.env.MACHINE_ID before
requiring the module (use Jest's isolateModules or equivalent) and then call
getMachineId() to assert the env value; locate and update the test containing
setMachineIdOverride and getMachineId accordingly.
In `@apps/e2e/demo-e2e-machine-id/project.json`:
- Line 13: The E2E target currently sets "passWithNoTests": true which masks
missing/failed test discovery; change the configuration for this target by
setting "passWithNoTests" to false (or remove the property) so the E2E runner
(the target using "passWithNoTests") fails when no specs are found, ensuring CI
surfaces test discovery regressions; update the JSON entry for "passWithNoTests"
accordingly.
In `@libs/auth/src/index.ts`:
- Around line 409-410: Update the docs to add a migration note that the Machine
ID APIs were moved: callers should change imports of setMachineIdOverride and
getMachineId from the `@frontmcp/auth` package to `@frontmcp/utils`; include a short
before/after import example showing the old import (import {
setMachineIdOverride, getMachineId } from '@frontmcp/auth') and the new import
(import { setMachineIdOverride, getMachineId } from '@frontmcp/utils'), mention
any semantic compatibility or breaking behavior (none expected if API
unchanged), and add a brief checklist for consumers to update code and examples
and run tests to verify the transition.
In `@libs/sdk/src/ha/__tests__/heartbeat.service.spec.ts`:
- Line 61: Replace the non-null assertions on the test variables by adding
explicit guard checks that throw descriptive errors: check that the local
variable stored is not null/undefined before accessing stored.value (used where
parsed = JSON.parse(stored!.value) and elsewhere), and check that hb is present
before accessing hb.nodeId; if a guard fails throw a clear Error or a
test-specific error class (e.g., new Error('Expected stored to be defined') /
new Error('Expected heartbeat (hb) to be defined')), so tests fail fast with
meaningful messages instead of using `!`.
In `@libs/sdk/src/ha/__tests__/session-takeover.spec.ts`:
- Around line 127-139: The test name "should allow retaking if session was
already ours" contradicts its assertion (expect(result.claimed).toBe(false));
rename the test description to reflect that retaking is disallowed when the
stored session nodeId equals our nodeId (for example "should NOT allow retaking
if session was already ours" or "should disallow retaking when session.nodeId
equals our node"), keeping the test body and the call to
attemptSessionTakeover(redis, 'mcp:transport:sess1', 'dead-pod', 'my-pod')
unchanged.
In `@libs/sdk/src/ha/ha-manager.ts`:
- Around line 67-78: The log "Notification relay started" in HAManager.start()
is misleading because subscribeRelay() actually performs the relay subscription;
update the behavior by either changing the message to something like
"Notification relay available" or moving the logger.info call into
subscribeRelay() so the message is emitted only when the relay is subscribed;
modify the code paths referencing this.relay, heartbeat.start(), logger,
start(), and subscribeRelay() accordingly to ensure the logged text accurately
reflects whether the relay was merely created/available or truly subscribed.
In `@libs/sdk/src/ha/heartbeat.service.ts`:
- Around line 74-79: The getHeartbeat method currently calls JSON.parse(raw)
which can throw on malformed data; wrap the parse in a try/catch inside async
getHeartbeat(nodeId: string) so that if JSON.parse throws you return null (and
optionally log the parse error via the service logger if available) instead of
letting the exception bubble up; update the function surrounding code that reads
const raw = await this.redis.get(`${this.keyPrefix}heartbeat:${nodeId}`) to
perform the safe parse and return the parsed HeartbeatValue or null on parse
failure.
In `@libs/sdk/src/ha/notification-relay.ts`:
- Around line 67-76: The unsubscribe method currently calls
this.subscriber.removeAllListeners('message'), which may drop listeners owned by
other components; instead, stop only the specific listener this instance
registered: store the bound message handler (e.g., the function assigned to
this.handler or a new private property like this._boundOnMessage) when
subscribing, and on unsubscribe call this.subscriber.removeListener('message',
thatBoundHandler) before awaiting this.subscriber.unsubscribe(this.channel);
additionally, add a runtime check in unsubscribe or subscribe to validate that
the subscriber connection is dedicated (or document/throw if not) to prevent
misuse with shared Redis clients.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 190-192: The haManager is being created with a type-bypassing cast
(`redis: haRedis as never`) — replace that by instantiating a runtime Redis
client from the config (this.metadata.redis / RedisOptionsInput) following the
same pattern used in redis.event-store.ts, then pass that client instance into
HaManager.create({ redis: <client>, nodeId: getMachineId() }) instead of the
cast; remove the `as never` usage, store the created Redis client (e.g.,
haRedisClient) on the instance for proper disposal/shutdown, and ensure
HaManager and your shutdown/dispose logic use that client for cleanup.
In `@libs/sdk/src/server/middleware/csp.middleware.ts`:
- Around line 40-48: The parser in the CSP middleware loop over rawDirectives
currently ignores directives without a value due to the condition "spaceIdx >
0"; update the loop in the code that processes rawDirectives (the for loop using
trimmed, spaceIdx and directives) to also handle value-less directives by adding
the directive key when spaceIdx === -1 (or spaceIdx === 0 case) and assign an
explicit marker (e.g., true or empty string) as the value so tokens like
"upgrade-insecure-requests" and "block-all-mixed-content" are preserved in the
directives object; ensure existing behavior for "directive value" pairs (when
spaceIdx > 0) remains unchanged.
In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts`:
- Around line 379-382: The current code uses response.setHeader('Set-Cookie',
cookie) which can overwrite existing cookies; change to the safe cookie-append
pattern: read existing header via response.getHeader('Set-Cookie'), normalize it
to an array (if string/undefined), push the new cookie returned by
buildSetCookie({ name: DEFAULT_FRONTMCP_NODE_COOKIE, value: nodeId }, request)
and then call response.setHeader('Set-Cookie', updatedArray). Mirror the logic
used in applyCookies() in server.validation.ts to ensure existing cookies are
preserved and the new cookie is appended.
In `@libs/sdk/src/transport/transport.registry.ts`:
- Line 345: Replace the generic throw at the session-claim site that currently
does throw new Error(`Session ${sessionId} claimed by another pod`) with a
specific MCP/domain error (e.g., SessionClaimedError or TransportConflictError)
and throw that instead; ensure the new error class implements toJsonRpcError()
returning the appropriate JSON-RPC error code and payload so callers can map
behavior consistently, and reference the sessionId in the error message/metadata
for debugging.
- Line 341: The sessionKey construction uses this.pendingStoreConfig?.keyPrefix
which is cleared after init so HA takeover can target the wrong key; change it
to use a stable, persisted prefix saved at initialization (e.g., a dedicated
property like this.pendingStoreKeyPrefix or the immutable
this.storeConfig?.keyPrefix) instead of reading this.pendingStoreConfig at
runtime. Update the code that sets pendingStoreConfig during init to copy the
effective keyPrefix into that stable property and then use that property when
building sessionKey (referencing sessionKey, pendingStoreConfig, keyPrefix, and
sessionId) so the prefix remains consistent after init/clear.
In `@libs/utils/src/http/cookie.ts`:
- Around line 165-175: The decodeURIComponent calls in the cookie parsing loop
can throw on malformed percent-encoding; update the loop that iterates over
pairs (the block using cookieHeader.split(';'), eqIdx, name/value decoding and
assigning into cookies) to wrap the decodeURIComponent calls for both name and
value in a try/catch and skip the current pair on error (continue) so malformed
percent-encoded cookies are ignored gracefully; preserve the existing trim(),
eqIdx check, and the assignment cookies[name] = value when decoding succeeds.
In `@scripts/merge-imports.ts`:
- Around line 62-63: Replace the inline TypeScript suppression "// `@ts-ignore`"
used above the for loop iterating "for (const [, g] of byModule)" with "//
`@ts-expect-error`" so ESLint's ban-ts-comment rule is satisfied and the
suppression will fail loudly if the underlying issue is fixed; update the
comment directly above the iteration over the "byModule" variable and keep the
suppression scope minimal (only that line).
---
Outside diff comments:
In `@libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts`:
- Around line 11-49: The test mock currently requires node:crypto and
reimplements crypto helpers inside the jest.mock for '@frontmcp/utils' (see the
mock block that defines sha256, encryptValue, decryptValue, randomBytes,
bytesToHex, getEnv, isProduction and mockGetMachineId); replace this with
importing the real implementations from '@frontmcp/utils' and only override
getMachineId (mockGetMachineId) in the jest.mock return value so all
cryptographic operations (sha256, encryptValue, decryptValue, randomBytes,
bytesToHex) come from the official library rather than node:crypto or custom
code.
In `@libs/sdk/src/auth/session/transport-session.manager.ts`:
- Around line 132-145: The constructor in transport-session.manager.ts directly
instantiates RedisSessionStore (and InMemorySessionStore) instead of using the
project factory; replace direct new RedisSessionStore(...) with a call to
createSessionStoreSync(...) (and use the factory for the memory fallback) so
store creation follows the lazy-loaded factory pattern; locate the constructor
function, remove the direct new RedisSessionStore usage and call
createSessionStoreSync(config) (or createSessionStoreSync({ mode: config.mode,
store: config.store, config: config.config })) to obtain the SessionStore
instance while keeping the existing stateless/in-memory allocation behavior.
---
Nitpick comments:
In @.prettierrc:
- Line 28: The import-order parser version is pinned to "5.0.0" via the
importOrderTypeScriptVersion setting; update the value of
importOrderTypeScriptVersion to match the workspace TypeScript major/minor
(e.g., "5.9" or the specific minor in use like "5.9.2") so the sort-imports
plugin can parse newer TypeScript syntax (change the
importOrderTypeScriptVersion property accordingly).
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-schema.e2e.spec.ts`:
- Around line 142-155: Replace the dynamic require() calls for findDeployment
and getDeploymentTargets with static ES module imports: add an import for {
findDeployment, getDeploymentTargets } from '@frontmcp/cli' at the top of the
test file and remove the require() usages inside the it blocks so the tests call
the imported functions directly (ensure the imported names match the existing
functions findDeployment and getDeploymentTargets).
In `@libs/auth/src/machine-id/index.ts`:
- Around line 1-4: Add a deprecation JSDoc to this compatibility re-export so
consumers are guided to the canonical package: annotate the exported symbols
getMachineId and setMachineIdOverride with a `@deprecated` comment that says these
are re-exports and directs users to import from '@frontmcp/utils' instead;
ensure the JSDoc appears above the export statement and includes a short
replacement note and since/version info if applicable so IDEs and TypeScript
will surface the deprecation.
In `@libs/cli/frontmcp.schema.json`:
- Around line 259-264: The portRange schema currently defines items as "type":
"number" but ports are integers; update the portRange definition to use "type":
"integer" for its items to match the existing port field and ensure consistency
(modify the "portRange" schema entry where "items" is defined and align it with
the "port" field's type).
In `@libs/cli/src/config/frontmcp-config.loader.ts`:
- Around line 46-49: Wrap the JSON.parse call inside the
filename.endsWith('.json') branch (where content is read from configPath) with a
try/catch so malformed JSON yields a clearer error: catch the SyntaxError from
JSON.parse(content) and throw or log a new Error that includes the failing
configPath/filename and the original error message (preserve the original error
as cause or include its message), so callers can see which file failed; update
the return path to rethrow the enriched error after catching.
In `@libs/cli/src/config/frontmcp-config.schema.ts`:
- Around line 110-117: The haConfigSchema currently uses .positive() for
heartbeatIntervalMs, heartbeatTtlMs, and takeoverGracePeriodMs which is looser
than the JSON Schema; update haConfigSchema (the z.object with
heartbeatIntervalMs, heartbeatTtlMs, takeoverGracePeriodMs) to use
z.number().int().min(...) with the JSON Schema minima
(heartbeatIntervalMs.min(1000), heartbeatTtlMs.min(2000),
takeoverGracePeriodMs.min(0)) while preserving .optional() and .strict() so
runtime Zod validation aligns with libs/cli/frontmcp.schema.json.
In `@libs/sdk/src/ha/__tests__/session-takeover.spec.ts`:
- Line 56: The test uses a non-null assertion on the redis.store.get call
(JSON.parse(redis.store.get('mcp:transport:sess1')!)) which bypasses
null-safety; replace each usage (the occurrences that assign to updated and
similar variables) with an explicit guard: call redis.store.get(...), check if
the result is null/undefined and if so throw a descriptive error (or a specific
test error class) to fail fast, otherwise pass the string into JSON.parse;
update the assertions at the other occurrences (the similar calls at the other
test points) to the same guard pattern so tests don't rely on `!`.
In `@libs/sdk/src/ha/heartbeat.service.ts`:
- Around line 81-85: The getAliveNodes method currently uses Redis KEYS which is
O(N); replace it with a SCAN-based iterative approach in getAliveNodes to avoid
blocking large keyspaces: use your Redis client's scan or scanStream API to
iterate with a cursor, collect keys matching `${this.keyPrefix}heartbeat:*`,
extract node IDs by slicing off `${this.keyPrefix}heartbeat:` (same slice
logic), deduplicate if needed, and return the array; ensure the implementation
respects the existing getAliveNodes signature and uses this.keyPrefix and
this.redis.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 1201-1229: The dispose() path currently doesn't ensure HA cleanup;
modify dispose() in scope.instance.ts to perform the same HA shutdown as
shutdown(): either call this.shutdown() at the start of dispose(), or replicate
the haManager cleanup (check this.haManager, await this.haManager.stop() in
try/catch and log errors) and also clear this._channelTeardown if present so
teardown is idempotent (refer to shutdown(), dispose(), this.haManager,
this.haManager.stop(), this._channelTeardown, and this.logger.error to locate
the code).
In `@libs/sdk/src/server/middleware/__tests__/csp.middleware.spec.ts`:
- Around line 145-158: Add a unit test that verifies readCspFromEnv correctly
handles directives without values: create a new it block (e.g., "should parse
directives without values") that sets process.env['FRONTMCP_CSP_ENABLED']='1'
and process.env['FRONTMCP_CSP_DIRECTIVES']="default-src 'self';
upgrade-insecure-requests", call readCspFromEnv(), assert the result is defined
and that csp!.directives['upgrade-insecure-requests'] === '' (and keep existing
checks for default-src if desired); this ensures the parsing logic in
readCspFromEnv handles value-less directives.
In `@libs/sdk/src/server/middleware/csp.middleware.ts`:
- Around line 61-73: buildCspHeaderValue currently concatenates directives as
`${directive} ${value}` which adds a trailing space when a directive's value is
an empty string; update the logic in buildCspHeaderValue to output just the
directive name when value === '' (or value is falsy specifically only for
empty-string cases) and otherwise output `${directive} ${value}`, and still
append `report-uri ${options.reportUri}` only when options.reportUri is present;
adjust handling of options.directives iteration to check the value before
composing the part so empty-value directives produce e.g.
"upgrade-insecure-requests" instead of "upgrade-insecure-requests ".
In `@libs/utils/src/env/__tests__/runtime-context-distributed.spec.ts`:
- Around line 27-33: Remove the redundant manual env cleanup in the test "should
detect serverless from platform env vars when FRONTMCP_DEPLOYMENT_MODE not set":
delete the two lines `delete process.env['VERCEL']` (one inside this it block
and the one at line 48) because afterEach already restores process.env; simply
set process.env['VERCEL']='1', call detectRuntimeContext(), assert
ctx.deployment === 'serverless', and let the existing afterEach restore the
environment. Ensure you only modify the test in
runtime-context-distributed.spec.ts and keep the call to detectRuntimeContext()
and the expectation intact.
In `@libs/utils/src/env/runtime-context.ts`:
- Around line 111-114: Replace the unsafe "as any" casts in the globalThis
checks with a small typed guard and use it in the runtime detection: add a
reusable type guard (e.g., hasGlobalProp or isGlobalWithProp) and use it to
check for 'Bun' and 'Deno' instead of casting; update the lines that currently
check " 'Bun' in (globalThis as any) " and " 'Deno' in (globalThis as any) " to
call the guard, leaving the isEdgeRuntime() call unchanged so runtime detection
remains the same but with proper type safety.
In `@libs/utils/src/http/__tests__/cookie.spec.ts`:
- Around line 14-16: Add a unit test to cover IPv6 bracket notation: in
libs/utils/src/http/__tests__/cookie.spec.ts, extend the existing isLocalhost
tests to include a case like calling isLocalhost with a bracketed IPv6 address
and port (e.g., '[::1]:3000') and assert it returns true; this will exercise the
bracket-notation parsing branch in the isLocalhost implementation.
In `@libs/utils/src/machine-id/machine-id.ts`:
- Line 58: The current loose check if (/^[0-9a-f-]{32,36}$/i.test(content))
return content; allows non-canonical UUIDs; replace this validation with a
stricter canonical-UUID pattern (e.g. the 8-4-4-4-12 hex groups with proper
version/variant if desired) so the branch only returns true for valid UUID
strings. Update the conditional in machine-id.ts that tests the variable content
to use the stricter regex (or a small helper isValidUuid(content)) referencing
the same symbol so invalid formats (wrong hyphen placement or arbitrary lengths)
no longer pass.
In `@scripts/fix-imports.ts`:
- Around line 82-87: The declaration for imports uses a non-obvious "& {}" to
exclude null from ReturnType<typeof parseImportLine>; replace that with an
explicit NonNullable wrapper (e.g. Array<{ lineIndex: number; parsed:
NonNullable<ReturnType<typeof parseImportLine>> }>) or alternatively add a short
inline comment above the imports declaration in processFile explaining that "&
{}" is used to narrow out null from parseImportLine's return type; reference the
symbols processFile, parseImportLine, imports and importLineIndices when making
the change.
- Around line 99-104: Replace the non-null assertion on byModule.get(key)! by
using a nullish-coalescing pattern: retrieve the array with const group =
byModule.get(key) ?? []; push the import into group (group.push(imp)) and then
set it back with byModule.set(key, group); this changes the logic around
byModule, imports, key and imp (where key = imp.parsed.module) to avoid using
the non-null assertion while preserving existing behavior.
In `@scripts/merge-imports.ts`:
- Around line 107-109: The empty catch block after the try in the mergeImports
routine silently swallows all errors; change it to log the caught error (e.g.,
console.error or process.stderr.write) with context so I/O/permission failures
are visible (for example: catch (err) { console.error('merge-imports error:',
err); }) – reference the try/catch in mergeImports (or the top-level
import-merging routine) and ensure you keep the minimal logging without changing
control flow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 595fb1ce-0aa0-4a20-904e-bf6e90e13ca5
⛔ Files ignored due to path filters (10)
libs/cli/src/commands/build/adapters/cloudflare.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/distributed.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/index.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/lambda.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/vercel.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/config.tsis excluded by!**/build/**libs/cli/src/commands/build/index.tsis excluded by!**/build/**libs/cli/src/commands/build/register.tsis excluded by!**/build/**libs/cli/src/commands/build/types.tsis excluded by!**/build/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (111)
.gitignore.prettierrcapps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tsapps/e2e/demo-e2e-frontmcp-config/e2e/config-schema.e2e.spec.tsapps/e2e/demo-e2e-frontmcp-config/fixtures/empty-deployments.config.jsonapps/e2e/demo-e2e-frontmcp-config/fixtures/invalid-name.config.jsonapps/e2e/demo-e2e-frontmcp-config/fixtures/minimal.config.jsonapps/e2e/demo-e2e-frontmcp-config/fixtures/multi-target.config.jsonapps/e2e/demo-e2e-frontmcp-config/fixtures/with-server.config.jsonapps/e2e/demo-e2e-frontmcp-config/fixtures/wrong-target-field.config.jsonapps/e2e/demo-e2e-frontmcp-config/jest.e2e.config.tsapps/e2e/demo-e2e-frontmcp-config/project.jsonapps/e2e/demo-e2e-frontmcp-config/tsconfig.e2e.jsonapps/e2e/demo-e2e-frontmcp-config/tsconfig.jsonapps/e2e/demo-e2e-machine-id/e2e/machine-id.e2e.spec.tsapps/e2e/demo-e2e-machine-id/e2e/runtime-context.e2e.spec.tsapps/e2e/demo-e2e-machine-id/jest.e2e.config.tsapps/e2e/demo-e2e-machine-id/project.jsonapps/e2e/demo-e2e-machine-id/src/apps/machine-id/index.tsapps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-deployment-mode.tool.tsapps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-machine-id.tool.tsapps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-runtime-context.tool.tsapps/e2e/demo-e2e-machine-id/src/main.tsapps/e2e/demo-e2e-machine-id/tsconfig.app.jsonapps/e2e/demo-e2e-machine-id/tsconfig.e2e.jsonapps/e2e/demo-e2e-machine-id/tsconfig.jsonapps/e2e/demo-e2e-machine-id/webpack.config.jseslint.config.mjseslint.imports.mjslibs/adapters/eslint.config.mjslibs/auth/eslint.config.mjslibs/auth/src/authorities/authorities.engine.tslibs/auth/src/authorities/authorities.types.tslibs/auth/src/authorization/__tests__/authorization.class.spec.tslibs/auth/src/authorization/authorization.class.tslibs/auth/src/authorization/index.tslibs/auth/src/context/__tests__/frontmcp-auth-context.spec.tslibs/auth/src/context/frontmcp-auth-context.tslibs/auth/src/index.tslibs/auth/src/machine-id/index.tslibs/auth/src/session/utils/__tests__/session-crypto.utils.spec.tslibs/auth/src/session/utils/session-crypto.utils.tslibs/cli/frontmcp.schema.jsonlibs/cli/src/config/__tests__/frontmcp-config.loader.spec.tslibs/cli/src/config/__tests__/frontmcp-config.schema.spec.tslibs/cli/src/config/define-config.tslibs/cli/src/config/frontmcp-config.loader.tslibs/cli/src/config/frontmcp-config.schema.tslibs/cli/src/config/frontmcp-config.types.tslibs/cli/src/config/index.tslibs/cli/src/core/args.tslibs/cli/src/index.tslibs/observability/src/plugin/observability.hooks.tslibs/observability/src/plugin/observability.plugin.tslibs/plugins/eslint.config.mjslibs/sdk/eslint.config.mjslibs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/auth/flows/auth.verify.flow.tslibs/sdk/src/auth/flows/oauth.authorize.flow.tslibs/sdk/src/auth/flows/oauth.token.flow.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/auth/flows/well-known.prm.flow.tslibs/sdk/src/auth/session/transport-session.manager.tslibs/sdk/src/auth/session/utils/session-id.utils.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/common/interfaces/execution-context.interface.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/utils/path.utils.tslibs/sdk/src/context/frontmcp-context.tslibs/sdk/src/direct/create.tslibs/sdk/src/ha/__tests__/ha-manager.spec.tslibs/sdk/src/ha/__tests__/heartbeat.service.spec.tslibs/sdk/src/ha/__tests__/session-takeover.spec.tslibs/sdk/src/ha/ha-manager.tslibs/sdk/src/ha/ha.constants.tslibs/sdk/src/ha/ha.types.tslibs/sdk/src/ha/heartbeat.service.tslibs/sdk/src/ha/index.tslibs/sdk/src/ha/notification-relay.tslibs/sdk/src/ha/session-takeover.tslibs/sdk/src/health/health.service.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/sdk/src/resource/flows/read-resource.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/server/middleware/__tests__/csp.middleware.spec.tslibs/sdk/src/server/middleware/csp.middleware.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/transport/transport.registry.tslibs/skills/eslint.config.mjslibs/ui/eslint.config.mjslibs/utils/src/env/__tests__/runtime-context-distributed.spec.tslibs/utils/src/env/runtime-context.tslibs/utils/src/http/__tests__/cookie.spec.tslibs/utils/src/http/cookie.tslibs/utils/src/http/index.tslibs/utils/src/index.tslibs/utils/src/machine-id/__tests__/machine-id-distributed.spec.tslibs/utils/src/machine-id/index.tslibs/utils/src/machine-id/machine-id.tspackage.jsonscripts/fix-imports.tsscripts/merge-imports.ts
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts
Outdated
Show resolved
Hide resolved
…mprove config loading tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
libs/sdk/src/ha/notification-relay.ts (1)
94-103: Consider validating parsed message structure.
JSON.parse(raw) as RelayMessageis an unsafe type assertion. If the JSON parses successfully but doesn't match the expected structure (e.g., missingsessionIdornotification), the handler may receive malformed data causing silent failures downstream.♻️ Proposed fix to add basic validation
private onMessage = (_channel: string, raw: string): void => { if (!this.handler) return; try { - const message = JSON.parse(raw) as RelayMessage; + const parsed = JSON.parse(raw); + // Basic structure validation + if (!parsed || typeof parsed.sessionId !== 'string' || !parsed.notification) { + return; // Malformed message — skip + } + const message = parsed as RelayMessage; // Fire-and-forget — handler errors shouldn't crash the relay Promise.resolve(this.handler(message)).catch(() => {}); } catch { // Malformed message — skip } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/ha/notification-relay.ts` around lines 94 - 103, The onMessage handler currently casts JSON.parse(raw) to RelayMessage unsafely; change onMessage to validate the parsed object before calling this.handler: parse raw into an unknown, check that required properties exist and have expected types (e.g., sessionId is a string and notification is present/has expected shape) and only call this.handler(message) when validation passes; otherwise ignore or log the malformed payload. Use the existing onMessage function name, the RelayMessage type as the target shape, and this.handler to gate delivery so malformed JSON that parses but doesn't match RelayMessage won't be forwarded.scripts/merge-imports.ts (1)
105-109: Avoid silently swallowing processing failures.Lines 105-109 discard all errors, which can mask failed rewrites in lint-staged and make debugging difficult.
🔧 Proposed fix
for (const f of files) { try { processFile(f); - } catch { - /* skip errors */ + } catch (err) { + console.warn(`merge-imports: failed to process ${f}: ${(err as Error).message}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/merge-imports.ts` around lines 105 - 109, The current try/catch around processFile(f) silently swallows all errors; update the catch to surface failures by either logging the error (including the filename variable f and the error stack) with console.error or processLogger.error, or rethrow the error so lint-staged fails; specifically modify the block that calls processFile(f) to include the error object and context (e.g., `f`) in the error handling instead of an empty catch, so failures are visible and actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/ha/ha-manager.ts`:
- Around line 57-65: Replace the generic Error thrown in HaManager.create with a
domain-specific error: add a new HaConfigurationError class that extends
InternalMcpError (mirroring ScopeConfigurationError) in the SDK errors module
and throw that from HaManager.create; specifically, implement
HaConfigurationError(message: string) calling super(message,
'HA_CONFIGURATION_ERROR') in the errors file (export it), then update
HaManager.create to throw new HaConfigurationError(...) when options.redis is
missing instead of new Error(...).
In `@libs/utils/src/http/cookie.ts`:
- Around line 84-95: Normalize and robustly parse the x-forwarded-proto header
instead of exact-equal checking: in isSecureRequest, and the other
header-parsing block that computes forwardedStr (the duplicate at lines
~125-134), read req.headers['x-forwarded-proto'] as a string, split on commas,
trim each entry, ignore case, and treat the request as secure if any entry
equals "https" or starts with "https" (to allow ports like "https:443" or
variations). Replace the current Array.isArray/first-element logic with this
normalized check and reuse the same parsing logic in both locations (the
isSecureRequest function and the other forwardedStr usage) to ensure proxied
HTTPS is detected reliably.
- Around line 118-145: After computing secure, ensure SameSite=None isn't paired
with secure=false: in the cookie builder logic (use symbols: options.sameSite /
sameSite, secure, isLocalhost, isSecureRequest, parts), if sameSite
(case-insensitive) equals 'None' and secure is false, force secure = true so
browsers don't reject the cookie; then continue building parts as before (Path,
Max-Age, HttpOnly, Secure, SameSite, Domain).
In `@scripts/merge-imports.ts`:
- Around line 56-59: The code uses non-null assertions on Map.get(...) (e.g.,
byModule.get(imp.parsed.module)!) which hides potential invalid states; replace
these with explicit checks and fail-fast behavior: for the loop over imports,
replace the get(... )! push with a pattern like let arr = byModule.get(key); if
(!arr) { arr = []; byModule.set(key, arr); } arr.push(imp); and do the same for
the second occurrence (the other Map.get(...) usage around the 82nd line); if a
missing map entry represents an unrecoverable state instead of initializing,
throw a clear, specific error (e.g., throw new Error(`Missing map entry for
${imp.parsed.module}`)) rather than using `!`.
- Around line 84-96: The dedupe loop using seen/specs can drop runtime imports
when a symbol appears as both a type and a value; modify the logic that iterates
group -> g.parsed.specifiers so that for each spec (s.name, s.isType) you track
presence and prefer runtime (non-type) variants: if a name is not seen, add it;
if it is seen but the stored ImportSpec is a type and the current spec is a
value, replace the stored spec with the value; never replace an existing value
with a type. Update references to seen/specs/ImportSpec/isType/s.name
accordingly so the final parts list preserves runtime imports when duplicates
exist.
- Line 15: Replace the direct Node fs import with the project utility import:
change the import that currently brings in readFileSync and writeFileSync from
'fs' to import { readFileSync, writeFileSync } from '@frontmcp/utils'; update
any references to readFileSync/writeFileSync in this module (e.g., in
scripts/merge-imports.ts) to use these utilities so all file system operations
go through `@frontmcp/utils`.
---
Nitpick comments:
In `@libs/sdk/src/ha/notification-relay.ts`:
- Around line 94-103: The onMessage handler currently casts JSON.parse(raw) to
RelayMessage unsafely; change onMessage to validate the parsed object before
calling this.handler: parse raw into an unknown, check that required properties
exist and have expected types (e.g., sessionId is a string and notification is
present/has expected shape) and only call this.handler(message) when validation
passes; otherwise ignore or log the malformed payload. Use the existing
onMessage function name, the RelayMessage type as the target shape, and
this.handler to gate delivery so malformed JSON that parses but doesn't match
RelayMessage won't be forwarded.
In `@scripts/merge-imports.ts`:
- Around line 105-109: The current try/catch around processFile(f) silently
swallows all errors; update the catch to surface failures by either logging the
error (including the filename variable f and the error stack) with console.error
or processLogger.error, or rethrow the error so lint-staged fails; specifically
modify the block that calls processFile(f) to include the error object and
context (e.g., `f`) in the error handling instead of an empty catch, so failures
are visible and actionable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e6ad2bb-b6cf-43f5-a090-4c8b8e62e2a0
📒 Files selected for processing (16)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tsapps/e2e/demo-e2e-machine-id/e2e/machine-id.e2e.spec.tsapps/e2e/demo-e2e-machine-id/project.jsonlibs/sdk/src/errors/index.tslibs/sdk/src/errors/transport.errors.tslibs/sdk/src/ha/__tests__/heartbeat.service.spec.tslibs/sdk/src/ha/__tests__/session-takeover.spec.tslibs/sdk/src/ha/ha-manager.tslibs/sdk/src/ha/heartbeat.service.tslibs/sdk/src/ha/notification-relay.tslibs/sdk/src/server/middleware/__tests__/csp.middleware.spec.tslibs/sdk/src/server/middleware/csp.middleware.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/transport/transport.registry.tslibs/utils/src/http/cookie.tsscripts/merge-imports.ts
✅ Files skipped from review due to trivial changes (5)
- libs/sdk/src/errors/index.ts
- libs/sdk/src/errors/transport.errors.ts
- apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts
- libs/sdk/src/transport/flows/handle.streamable-http.flow.ts
- libs/sdk/src/server/middleware/tests/csp.middleware.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/e2e/demo-e2e-machine-id/project.json
- libs/sdk/src/ha/tests/heartbeat.service.spec.ts
- apps/e2e/demo-e2e-machine-id/e2e/machine-id.e2e.spec.ts
- libs/sdk/src/server/middleware/csp.middleware.ts
- libs/sdk/src/ha/tests/session-takeover.spec.ts
- libs/sdk/src/ha/heartbeat.service.ts
…er; update tool execution signatures
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/sdk/src/ha/ha-manager.ts (2)
58-65: Consider validatingnodeIdto fail fast on invalid input.The method validates
options.redisbut notoptions.nodeId. An empty or missingnodeIdcould cause issues with Redis key generation and heartbeat operations.🛡️ Proposed fix
static create(options: HaManagerOptions): HaManager { if (!options.redis) { throw new HaConfigurationError( 'Distributed mode requires Redis configuration. ' + 'Add redis: { host: "...", port: 6379 } to your `@FrontMcp`() config.', ); } + if (!options.nodeId) { + throw new HaConfigurationError('Distributed mode requires a valid nodeId.'); + } return new HaManager(options.redis, options.nodeId, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/ha/ha-manager.ts` around lines 58 - 65, The create method currently checks options.redis but not options.nodeId; add a validation in HaManager.create to ensure options.nodeId is a non-empty string (e.g., typeof === 'string' and options.nodeId.trim().length > 0) and throw HaConfigurationError with a clear message if invalid, before calling new HaManager(options.redis, options.nodeId, options); this will fail fast and prevent downstream Redis key/heartbeat issues.
49-51: Consider warning when pub/sub configuration is incomplete.If a user provides only one of
pubsubSubscriberorpubsubPublisher, the relay silently won't be created. This could mask configuration mistakes. Consider logging a warning when the configuration is asymmetric.💡 Suggested improvement
if (options.pubsubSubscriber && options.pubsubPublisher) { this.relay = new NotificationRelay(options.pubsubSubscriber, options.pubsubPublisher, nodeId, this.config); + } else if (options.pubsubSubscriber || options.pubsubPublisher) { + this.logger?.warn('[HA] Incomplete pub/sub config — both pubsubSubscriber and pubsubPublisher required for relay'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/ha/ha-manager.ts` around lines 49 - 51, When only one of options.pubsubSubscriber or options.pubsubPublisher is provided the NotificationRelay (constructed via new NotificationRelay(...)) is not created and the condition silently fails; add a conditional check in the constructor or init path of the HA manager (around the code that sets this.relay) to detect asymmetric configuration and call processLogger.warn or this.config.logger.warn with a clear message indicating which side is missing and that the relay will be disabled, while still creating the relay only when both options.pubsubSubscriber and options.pubsubPublisher are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/sdk/src/ha/ha-manager.ts`:
- Around line 58-65: The create method currently checks options.redis but not
options.nodeId; add a validation in HaManager.create to ensure options.nodeId is
a non-empty string (e.g., typeof === 'string' and options.nodeId.trim().length >
0) and throw HaConfigurationError with a clear message if invalid, before
calling new HaManager(options.redis, options.nodeId, options); this will fail
fast and prevent downstream Redis key/heartbeat issues.
- Around line 49-51: When only one of options.pubsubSubscriber or
options.pubsubPublisher is provided the NotificationRelay (constructed via new
NotificationRelay(...)) is not created and the condition silently fails; add a
conditional check in the constructor or init path of the HA manager (around the
code that sets this.relay) to detect asymmetric configuration and call
processLogger.warn or this.config.logger.warn with a clear message indicating
which side is missing and that the relay will be disabled, while still creating
the relay only when both options.pubsubSubscriber and options.pubsubPublisher
are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b4f5d31-c13f-4384-93f8-275e06916fe8
📒 Files selected for processing (7)
apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-deployment-mode.tool.tsapps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-machine-id.tool.tslibs/sdk/src/errors/index.tslibs/sdk/src/errors/sdk.errors.tslibs/sdk/src/ha/ha-manager.tslibs/utils/src/http/cookie.tsscripts/merge-imports.ts
✅ Files skipped from review due to trivial changes (1)
- apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-machine-id.tool.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/sdk/src/errors/index.ts
- apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-deployment-mode.tool.ts
- scripts/merge-imports.ts
- libs/utils/src/http/cookie.ts
…type safety checks
… and enhance test reliability
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-04-09T04:23:28.774Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 149-151: The zero-arg detection in the Job decorator's type
helpers is ambiguous because __Param<C> collapses both execute() and
execute(input: never) to never; update the conditional that currently uses
`[__Param<C>] extends [never]` to instead test `__ExecParams<C> extends readonly
[]` so empty-parameter lists are distinguished from impossible params (modify
the types __ExecParams and __Param usage and the conditional check where that
pattern appears). Also add a regression unit test for the Job decorator
mirroring Tool's ValidEmptySchema case to assert behavior when execute() has an
empty schema (i.e., no parameters) so future changes don't reintroduce the bug;
place the test alongside existing Job decorator tests and name it clearly (e.g.,
"ValidEmptySchema for Job.execute()").
In `@libs/sdk/src/common/decorators/tool.decorator.ts`:
- Around line 264-267: The current tuple-based inference for __ExecParams and
__Param conflates zero-arg execute() with execute(input: never) because it maps
both to never; update __ExecParams<C extends __Ctor> to infer parameter tuple P
as readonly unknown[] (or similar) and return never only when the tuple is
exactly [] (detect empty tuple shape) so __Param can distinguish [] (no
parameter) from [never] (explicit impossible param); also replace any return
types used in these utility types with unknown to follow strict typing
guidelines, and add a `@ts-expect-error` test case for a class with execute(input:
never) to prevent regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a3dd7b9-a5a9-4f41-841c-16d85224de61
📒 Files selected for processing (5)
apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-deployment-mode.tool.tsapps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-machine-id.tool.tslibs/sdk/src/common/decorators/__tests__/tool-type-safety.spec.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/tool.decorator.ts
✅ Files skipped from review due to trivial changes (2)
- apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-deployment-mode.tool.ts
- apps/e2e/demo-e2e-machine-id/src/apps/machine-id/tools/get-machine-id.tool.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts (1)
11-47:⚠️ Potential issue | 🟠 MajorReplace direct Node crypto usage in this test mock.
Line 12 uses
require('crypto')and custom crypto implementations inside this test file. This violates the codebase requirement to always use@frontmcp/utilsfor cryptographic operations and duplicates behavior already available through that module.Proposed fix
jest.mock('@frontmcp/utils', () => { - const crypto = require('crypto'); + const actual = jest.requireActual('@frontmcp/utils'); return { + ...actual, getMachineId: jest.fn(() => 'test-machine-id-12345'), - sha256: (data: Uint8Array) => { - const hash = crypto.createHash('sha256').update(data); - return new Uint8Array(hash.digest()); - }, - encryptValue: jest.fn((obj: unknown, key: Uint8Array) => { - // Simplified AES-256-GCM encryption for testing - const iv = crypto.randomBytes(12); - const cipher = crypto.createCipheriv('aes-256-gcm', Buffer.from(key), iv); - const plaintext = JSON.stringify(obj); - let encrypted = cipher.update(plaintext, 'utf8', 'base64'); - encrypted += cipher.final('base64'); - const tag = cipher.getAuthTag(); - return { - iv: iv.toString('base64'), - tag: tag.toString('base64'), - data: encrypted, - }; - }), - decryptValue: jest.fn((envelope: { iv: string; tag: string; data: string; alg: string }, key: Uint8Array) => { - const iv = Buffer.from(envelope.iv, 'base64'); - const tag = Buffer.from(envelope.tag, 'base64'); - const decipher = crypto.createDecipheriv('aes-256-gcm', Buffer.from(key), iv); - decipher.setAuthTag(tag); - let decrypted = decipher.update(envelope.data, 'base64', 'utf8'); - decrypted += decipher.final('utf8'); - return JSON.parse(decrypted); - }), - // Required by AuthInternalError base class for errorId generation - randomBytes: (n: number) => crypto.randomBytes(n), - bytesToHex: (bytes: Uint8Array) => Buffer.from(bytes).toString('hex'), getEnv: (key: string) => process.env[key], isProduction: () => process.env['NODE_ENV'] === 'production', }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts` around lines 11 - 47, The test mock is directly requiring Node's crypto and reimplementing sha256/encryptValue/decryptValue/randomBytes/bytesToHex which duplicates and violates the rule to use `@frontmcp/utils`; replace the custom crypto implementations in the jest.mock('@frontmcp/utils') block by delegating those functions to the real module via jest.requireActual('@frontmcp/utils') (or import the real functions) and only override test-specific items like getMachineId, getEnv, and isProduction; keep the mock for getMachineId consistent and ensure encryptValue/decryptValue/sha256/randomBytes/bytesToHex are the real implementations from `@frontmcp/utils` so tests use the canonical crypto logic instead of require('crypto').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/auth/session/__tests__/session-id.utils.spec.ts`:
- Line 7: The inline comment "Import after mocking" is stale because the module
import now appears before the jest.mock(...) blocks; either remove the comment
or update it to accurately describe the import order. Locate the top-of-file
import statement(s) in session-id.utils.spec.ts and the jest.mock(...) calls,
then adjust or delete the "Import after mocking" comment so it correctly
reflects the current order and avoids future confusion.
---
Outside diff comments:
In `@libs/auth/src/session/utils/__tests__/session-crypto.utils.spec.ts`:
- Around line 11-47: The test mock is directly requiring Node's crypto and
reimplementing sha256/encryptValue/decryptValue/randomBytes/bytesToHex which
duplicates and violates the rule to use `@frontmcp/utils`; replace the custom
crypto implementations in the jest.mock('@frontmcp/utils') block by delegating
those functions to the real module via jest.requireActual('@frontmcp/utils') (or
import the real functions) and only override test-specific items like
getMachineId, getEnv, and isProduction; keep the mock for getMachineId
consistent and ensure encryptValue/decryptValue/sha256/randomBytes/bytesToHex
are the real implementations from `@frontmcp/utils` so tests use the canonical
crypto logic instead of require('crypto').
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc87e885-844f-4c15-9d75-1cb3c6a798b5
📒 Files selected for processing (5)
CLAUDE.mdlibs/auth/src/session/utils/__tests__/session-crypto.utils.spec.tslibs/sdk/src/auth/session/__tests__/session-id.utils.spec.tslibs/sdk/src/direct/__tests__/create-e2e.spec.tslibs/sdk/src/direct/__tests__/create.spec.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
…rove mock handling
…headers configurations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts (1)
96-99: This note is stale relative to the active test suite.The comment says JS format tests are skipped, but Line 137 includes an active
.jsprecedence test. Consider narrowing/removing this note to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts` around lines 96 - 99, The comment block stating "JS/CJS/MJS format tests are skipped in Jest..." is now stale because there is an active .js precedence test present (the .js precedence test in this spec verifies JS config loading under Jest); update or remove that note to avoid confusion by either narrowing it to exactly which JS/CJS/MJS cases are skipped (e.g., temporary-directory require() transformation issues for certain file patterns) or deleting it entirely, and ensure the comment accurately reflects that a specific .js precedence test is intentionally run under Jest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/skills/catalog/frontmcp-production-readiness/examples/distributed-ha/ha-kubernetes-3-replicas.md`:
- Around line 99-110: The Dockerfile builds with
FRONTMCP_DEPLOYMENT_MODE=distributed (npx frontmcp build --target distributed)
but later CMD starts node dist/main.js which may not exist for the distributed
output; update the runtime COPY and CMD to match the distributed build layout
(e.g., COPY --from=builder /app/dist/distributed ./dist and CMD
["node","dist/distributed/main.js"]) or alternatively change the build step to
emit flat dist/main.js—ensure the paths referenced by COPY and CMD match the
actual output of the distributed build.
In
`@libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md`:
- Line 95: The heartbeat sample JSON in the README returns seconds but the HA
heartbeat uses millisecond timestamps; update the sample JSON object (the
example:
{"nodeId":"mcp-server-7b8f9-abc12","startedAt":1712620800,"lastBeat":1712620810,"sessionCount":5})
to use millisecond values (multiply startedAt and lastBeat by 1000) so the
sample matches real Redis payloads and runtime expectations.
- Line 178: Update the TTL guidance so it doesn't conflict: change the
recommendation in the "Sessions not transferred after pod death" row to say that
heartbeatTtlMs should be at least 2x heartbeatIntervalMs and also typically in
the 15–20s range (i.e., set heartbeatTtlMs = max(2 * heartbeatIntervalMs,
15s–20s)); reference the settings heartbeatTtlMs and heartbeatIntervalMs and
update the table entry text to reflect this combined rule.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 96-99: The comment block stating "JS/CJS/MJS format tests are
skipped in Jest..." is now stale because there is an active .js precedence test
present (the .js precedence test in this spec verifies JS config loading under
Jest); update or remove that note to avoid confusion by either narrowing it to
exactly which JS/CJS/MJS cases are skipped (e.g., temporary-directory require()
transformation issues for certain file patterns) or deleting it entirely, and
ensure the comment accurately reflects that a specific .js precedence test is
intentionally run under Jest.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93cfc404-09c5-425c-b813-1a6fea82d41c
📒 Files selected for processing (17)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tsdocs/docs.jsondocs/frontmcp/deployment/frontmcp-config.mdxdocs/frontmcp/deployment/high-availability.mdxdocs/frontmcp/deployment/machine-id.mdxdocs/frontmcp/deployment/security-headers.mdxlibs/skills/catalog/frontmcp-config/SKILL.mdlibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/distributed-ha-config.mdlibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/json-schema-ide-support.mdlibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/multi-target-with-security.mdlibs/skills/catalog/frontmcp-config/examples/configure-security-headers/csp-report-only.mdlibs/skills/catalog/frontmcp-config/examples/configure-security-headers/full-production-headers.mdlibs/skills/catalog/frontmcp-config/references/configure-deployment-targets.mdlibs/skills/catalog/frontmcp-config/references/configure-security-headers.mdlibs/skills/catalog/frontmcp-production-readiness/examples/distributed-ha/ha-kubernetes-3-replicas.mdlibs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.mdlibs/skills/catalog/skills-manifest.json
✅ Files skipped from review due to trivial changes (10)
- docs/docs.json
- libs/skills/catalog/frontmcp-config/examples/configure-security-headers/csp-report-only.md
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/multi-target-with-security.md
- docs/frontmcp/deployment/security-headers.mdx
- libs/skills/catalog/frontmcp-config/references/configure-deployment-targets.md
- docs/frontmcp/deployment/machine-id.mdx
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/distributed-ha-config.md
- docs/frontmcp/deployment/high-availability.mdx
- docs/frontmcp/deployment/frontmcp-config.mdx
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/json-schema-ide-support.md
...ls/catalog/frontmcp-production-readiness/examples/distributed-ha/ha-kubernetes-3-replicas.md
Show resolved
Hide resolved
libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md
Outdated
Show resolved
Hide resolved
libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md
Outdated
Show resolved
Hide resolved
…gs and session management
… and session management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md (1)
91-92: UseSCANinstead ofKEYSin verification guidance.
KEYScan block Redis on larger datasets. For operational docs, prefer cursor-based scanning.🛠️ Proposed docs patch
-# Check heartbeat keys exist for each pod -redis-cli KEYS "mcp:ha:heartbeat:*" +# Check heartbeat keys exist for each pod +redis-cli --scan --pattern "mcp:ha:heartbeat:*"Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md` around lines 91 - 92, Replace the blocking redis-cli KEYS usage that searches "mcp:ha:heartbeat:*" with a cursor-based SCAN pattern to avoid blocking Redis on large datasets; update both occurrences (the line containing redis-cli KEYS "mcp:ha:heartbeat:*" and the repeat around the later occurrence) to use SCAN with MATCH "mcp:ha:heartbeat:*" and a reasonable COUNT/batch size or the redis-cli --scan alternative so the docs show non-blocking verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 2-4: The suite header in config-loader.e2e.spec.ts incorrectly
claims coverage for JSON, JS, CJS, and TS; either update the top comment block
to accurately state that the file tests JSON and JS plus fallback/error flows,
or add the missing CJS/TS tests; locate the header comment at the top of
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts (the
multi-format config loading comment) and edit it to reflect the actual formats
tested or implement tests for the missing formats.
- Around line 172-176: The test description is misleading: the payload is valid
JSON but fails schema validation, so update the it(...) name to reflect schema
validation (e.g., change "should throw for invalid JSON config" to "should throw
for invalid config due to schema validation" or similar) in the test block that
writes frontmcp.config.json (the it(...) that uses writeFile with
JSON.stringify({ name: 'invalid name with spaces', deployments: [{ target:
'node' }] })). Keep the test logic unchanged—only update the description string
used in the it(...) call.
In
`@libs/skills/catalog/frontmcp-config/references/configure-security-headers.md`:
- Around line 103-110: The table for server.headers omits that hsts,
contentTypeOptions, and frameOptions support disabling via false; update the
`server.headers` reference table row entries for `hsts`, `contentTypeOptions`,
and `frameOptions` to indicate their schema is `string | false` (or show
“default: string, can be false to disable”) and mention the disable semantics
per the schema (oneOf: [string, false]) so readers know they can set these
fields to false to disable the header; refer to `server.headers`, `hsts`,
`contentTypeOptions`, `frameOptions`, and the `oneOf: [string, false]` schema
when making the change.
---
Nitpick comments:
In
`@libs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md`:
- Around line 91-92: Replace the blocking redis-cli KEYS usage that searches
"mcp:ha:heartbeat:*" with a cursor-based SCAN pattern to avoid blocking Redis on
large datasets; update both occurrences (the line containing redis-cli KEYS
"mcp:ha:heartbeat:*" and the repeat around the later occurrence) to use SCAN
with MATCH "mcp:ha:heartbeat:*" and a reasonable COUNT/batch size or the
redis-cli --scan alternative so the docs show non-blocking verification.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 056de49e-590a-413d-9f29-4d552fe128b4
📒 Files selected for processing (11)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tslibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/distributed-ha-config.mdlibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/json-schema-ide-support.mdlibs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/multi-target-with-security.mdlibs/skills/catalog/frontmcp-config/examples/configure-security-headers/csp-report-only.mdlibs/skills/catalog/frontmcp-config/examples/configure-security-headers/full-production-headers.mdlibs/skills/catalog/frontmcp-config/references/configure-deployment-targets.mdlibs/skills/catalog/frontmcp-config/references/configure-security-headers.mdlibs/skills/catalog/frontmcp-production-readiness/examples/distributed-ha/ha-kubernetes-3-replicas.mdlibs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.mdlibs/skills/catalog/skills-manifest.json
✅ Files skipped from review due to trivial changes (6)
- libs/skills/catalog/frontmcp-config/examples/configure-security-headers/csp-report-only.md
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/json-schema-ide-support.md
- libs/skills/catalog/frontmcp-config/examples/configure-security-headers/full-production-headers.md
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/multi-target-with-security.md
- libs/skills/catalog/frontmcp-config/references/configure-deployment-targets.md
- libs/skills/catalog/frontmcp-config/examples/configure-deployment-targets/distributed-ha-config.md
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/skills/catalog/frontmcp-production-readiness/examples/distributed-ha/ha-kubernetes-3-replicas.md
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts
Outdated
Show resolved
Hide resolved
libs/skills/catalog/frontmcp-config/references/configure-security-headers.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts (1)
79-83: Remove unnecessaryRecord<string, unknown>cast chains.Lines 79–83 bypass the strict typing provided by
FrontMcpConfigParsed. SinceloadFrontMcpConfigreturns a properly typedFrontMcpConfigParsedobject with all nested fields (deployments,server,http,csp,headers) fully defined in the schema, the casts are unnecessary and make schema drift easier to miss.Use property access directly or assertions with proper shape validation:
Suggested approach
- const deployment = config.deployments[0] as Record<string, unknown>; - const server = deployment.server as Record<string, unknown>; - const http = server.http as Record<string, unknown>; - const csp = server.csp as Record<string, unknown>; - const headers = server.headers as Record<string, unknown>; - expect(http.port).toBe(8080); - expect(csp.enabled).toBe(true); - expect(csp.directives).toEqual({ 'default-src': "'self'" }); - expect(headers.hsts).toBe('max-age=31536000'); - expect(headers.frameOptions).toBe('DENY'); + expect(config.deployments[0]).toMatchObject({ + target: 'node', + server: { + http: { port: 8080 }, + csp: { enabled: true, directives: { 'default-src': "'self'" } }, + headers: { hsts: 'max-age=31536000', frameOptions: 'DENY' }, + }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts` around lines 79 - 83, The casts chain on `deployment`, `server`, `http`, `csp`, and `headers` is unnecessary—`loadFrontMcpConfig` returns a fully typed `FrontMcpConfigParsed`, so remove the `as Record<string, unknown>` casts and access properties directly (e.g., use `const deployment = config.deployments[0]; const server = deployment.server; const http = server.http; const csp = server.csp; const headers = server.headers;`) or, if you need a narrower shape, replace with a single typed assertion to the expected interface. Update any downstream uses to rely on the typed properties from `FrontMcpConfigParsed` rather than the generic `Record` casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 13-21: The teardown can throw if beforeEach failed to set tmpDir;
update afterEach to guard cleanup by checking tmpDir before calling rm and only
attempt removal when tmpDir is defined (nullable check), e.g., in the afterEach
that currently calls rm(tmpDir, { recursive: true }) wrap that call in a
conditional or short-circuit so rm is not invoked when tmpDir is undefined/null;
reference tmpDir, beforeEach, afterEach, mkdtemp and rm to locate the change.
- Around line 167-188: The three rejection tests that call loadFrontMcpConfig
(the "should throw when no config file and no package.json", "should throw for
schema-invalid config (name with spaces)", and "should throw for config with no
deployments") only assert message text via rejects.toThrow; update each to also
assert the error class by chaining rejects.toBeInstanceOf(Error) (i.e., ensure
the Promise reject expectation uses both toBeInstanceOf(Error) and toThrow(...))
so the tests verify the thrown object's type as well as its message; locate the
assertions in the tests that use loadFrontMcpConfig, tmpDir and writeFile and
add the instanceof checks accordingly.
In
`@libs/skills/catalog/frontmcp-config/references/configure-security-headers.md`:
- Line 183: Replace the misleading phrase "HSTS locked to HTTP" with wording
that correctly states HSTS forces HTTPS (for example, "HSTS enforces HTTPS" or
"HSTS forcing HTTPS"); update the table cell that currently contains "HSTS
locked to HTTP" so the entry accurately describes that HSTS causes the browser
to upgrade or enforce HTTPS and then adjust the troubleshooting guidance
accordingly (the target text to change is the exact string "HSTS locked to HTTP"
in the table row shown).
---
Nitpick comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 79-83: The casts chain on `deployment`, `server`, `http`, `csp`,
and `headers` is unnecessary—`loadFrontMcpConfig` returns a fully typed
`FrontMcpConfigParsed`, so remove the `as Record<string, unknown>` casts and
access properties directly (e.g., use `const deployment = config.deployments[0];
const server = deployment.server; const http = server.http; const csp =
server.csp; const headers = server.headers;`) or, if you need a narrower shape,
replace with a single typed assertion to the expected interface. Update any
downstream uses to rely on the typed properties from `FrontMcpConfigParsed`
rather than the generic `Record` casts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f683c0f0-896f-4438-b69e-08e6408d45d3
📒 Files selected for processing (3)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tslibs/skills/catalog/frontmcp-config/references/configure-security-headers.mdlibs/skills/catalog/frontmcp-production-readiness/references/distributed-ha.md
libs/skills/catalog/frontmcp-config/references/configure-security-headers.md
Outdated
Show resolved
Hide resolved
…ty headers documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts (1)
20-25:⚠️ Potential issue | 🟡 MinorAvoid swallowing all teardown failures in
afterEach.Line 22 is wrapped in a blanket
try/catch, which can mask real cleanup regressions. Prefer a nullabletmpDirguard so only “not initialized” is ignored while realrmfailures still surface.💡 Suggested fix
- let tmpDir: string; + let tmpDir: string | null = null; beforeEach(async () => { tmpDir = await mkdtemp(path.join(os.tmpdir(), 'frontmcp-config-')); }); afterEach(async () => { - try { - await rm(tmpDir, { recursive: true }); - } catch { - // Ignore cleanup errors (e.g., if beforeEach failed before assigning tmpDir) + if (tmpDir) { + await rm(tmpDir, { recursive: true }); + tmpDir = null; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts` around lines 20 - 25, The afterEach teardown currently swallows all errors by wrapping rm(...) in a blanket try/catch; instead, guard the removal with a nullable tmpDir check so only "not initialized" cases are ignored and real fs errors still surface: in the afterEach block use a conditional check on tmpDir (the same variable set in beforeEach) and call rm(tmpDir, { recursive: true }) only when tmpDir is truthy, removing the broad try/catch so failures from rm will cause the test to fail and be visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/skills/catalog/frontmcp-config/references/configure-security-headers.md`:
- Line 174: The checklist item containing "X-Frame-Options: DENY present (or
`SAMEORIGIN` if iframes needed)" has a duplicated word "if"; edit that line in
configure-security-headers.md by removing the extra "if" so it reads e.g.
"X-Frame-Options: DENY present (or `SAMEORIGIN` if iframes are needed)" or
"X-Frame-Options: DENY present (or `SAMEORIGIN` when iframes are needed)" to fix
the duplication.
- Line 34: Update the single-line description that currently reads "A
`frontmcp.config.ts` or `.json` file" to reflect the full supported filename
patterns and loader precedence: state that FrontMCP looks for a typed
frontmcp.config.* at project root in this exact order: frontmcp.config.ts →
frontmcp.config.js → frontmcp.config.json → frontmcp.config.mjs →
frontmcp.config.cjs, and falls back to package.json only if none of those exist;
reference the existing mention of `frontmcp.config.*` and the
`configure-deployment-targets` context so the new wording matches actual loader
behavior.
---
Duplicate comments:
In `@apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.ts`:
- Around line 20-25: The afterEach teardown currently swallows all errors by
wrapping rm(...) in a blanket try/catch; instead, guard the removal with a
nullable tmpDir check so only "not initialized" cases are ignored and real fs
errors still surface: in the afterEach block use a conditional check on tmpDir
(the same variable set in beforeEach) and call rm(tmpDir, { recursive: true })
only when tmpDir is truthy, removing the broad try/catch so failures from rm
will cause the test to fail and be visible.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14ecb431-4c7c-43e8-9e11-bc6bc38102e5
📒 Files selected for processing (2)
apps/e2e/demo-e2e-frontmcp-config/e2e/config-loader.e2e.spec.tslibs/skills/catalog/frontmcp-config/references/configure-security-headers.md
libs/skills/catalog/frontmcp-config/references/configure-security-headers.md
Show resolved
Hide resolved
libs/skills/catalog/frontmcp-config/references/configure-security-headers.md
Show resolved
Hide resolved
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
Testing
Build & Development
Chores
Documentation