test: add unit tests for CSP utility functions#167
test: add unit tests for CSP utility functions#167gabrypavanello wants to merge 6 commits intomainfrom
Conversation
…SK-037-02) Replace all raw console.log/warn/error calls across 17 inspector/src files with scoped logger instances using createLogger(). Each module gets a descriptive source name for log filtering. - 17 files migrated to structured logger - Stale eslint-disable no-console comments removed - Tests updated to match new console.info output format - bin/mcp-inspector.ts and widget-server-templates.ts untouched - proxy-resources.ts skipped (console call is in client-side template)
Add comprehensive tests for all 4 exported functions in utils/csp.ts: - generateMcpCSPMetadata: empty config, field mapping, ChatGPT-only field filtering - generateOpenAICSPMetadata: snake_case mapping, all 4 CSP fields - generateMcpUIMetadata: name fallback, description, CSP inclusion/omission, prefersBorder - generateOpenAIUIMetadata: widgetCSP with snake_case, domain field, empty CSP omission Covers 16 test cases for previously untested pure utility module. Kaizen: daily incremental improvement.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request introduces a structured, level-aware logger module across the codebase, replacing direct console logging with a centralized logger respecting the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Code Review: PR #167 - CSP Utility Tests & Structured Logger MigrationOverviewThis PR adds comprehensive unit tests for previously untested CSP utilities and migrates console logging to a structured logger across multiple packages. The changes are well-structured and improve code quality. ✅ Strengths1. Excellent Test Coverage for CSP Utilities (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/inspector/src/session/session-renderer.ts (1)
206-219:⚠️ Potential issue | 🔴 Critical
loggeris not available insidepage.evaluate()— this will throw ReferenceError at runtime.The
page.evaluate()callback executes in the browser context (Chromium), not in Node.js. Theloggervariable from the module scope is not serialized or available there. This code will fail withReferenceError: logger is not definedwhen the MCP host context update runs.The same issue exists at lines 255, 286, 303, 312, and 331.
🐛 Proposed fix: revert to console.log inside browser context
/* eslint-disable no-undef */ await page.evaluate((ctx: typeof hostContext) => { const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { const message = { jsonrpc: "2.0", method: "ui/notifications/host-context-changed", params: { hostContext: ctx }, }; iframe.contentWindow.postMessage(message, "*"); - logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); + console.log("[MCP Host] Sent ui/notifications/host-context-changed", ctx); } }, hostContext); /* eslint-enable no-undef */Apply similar changes at lines 255, 286, 303, 312, and 331.
🤖 Fix all issues with AI agents
In `@packages/inspector/src/hosts/mcp-host.ts`:
- Line 222: The embedded Playwright init script returned by
getPlaywrightInitScript() uses logger.info calls that do not exist in the
browser context; update the script string to use console (e.g.,
console.log/console.warn/console.error as appropriate) instead of logger.info at
each occurrence inside the string (the occurrences originally at the logger.info
calls around ui/initialize and the other noted spots). Locate the
getPlaywrightInitScript() method and replace those in-string logger.* references
with console.* so the code runs in the browser context without ReferenceError.
In `@packages/inspector/src/hosts/openai-host.ts`:
- Around line 248-249: The injected script returned by getPlaywrightInitScript
contains a browser-side call to logger (see the requestDisplayMode block), but
logger is a Node-only variable and will cause a ReferenceError in the Playwright
page; replace the logger usage with console.log (or remove the log) inside the
template so the browser-executed code uses the global console instead of the
undefined logger. Ensure the change targets the getPlaywrightInitScript()
template string where requestDisplayMode returns { mode: mode } and references
logger.
In `@packages/inspector/src/oauth/preset-config.ts`:
- Around line 397-400: The catch block that logs the authorization URL currently
uses logger.info which can be filtered out by MCP_APPS_LOG_LEVEL; update the
catch handler where browser auto-open is attempted to emit the URL with
console.log (or logger.warn) instead of logger.info so users always see the
fallback URL (replace the logger.info(`\nOpen this URL... ${urlStr}\n`) call
with console.log(...) keeping the same message/format).
In `@packages/inspector/src/widget-session-manager.ts`:
- Line 412: The calls to logger inside page.evaluate are invalid because logger
is a Node-side module not available in the browser context; locate the
page.evaluate(...) callbacks (references: page.evaluate used where messages are
sent such as the MCP/OpenAI host ui/notifications and synced events) and replace
those logger.info calls with console.log/console.info (or remove the logging)
inside the evaluate callback so the code runs in the Playwright browser context
without ReferenceError; ensure you update all occurrences that the reviewer
listed (the evaluate blocks that emit "[MCP Host] Sent
ui/notifications/host-context-changed", "[OpenAI Host] Sent globals sync:",
"[MCP Host] Stored hostContext update...", "[MCP Host] Sent synced event:",
"[OpenAI Host] Sent synced event:") to use console instead of logger.
In `@packages/ui-react/src/hooks.ts`:
- Around line 22-40: The check for process in the uiReactLogger factory is
triggering ESLint because accessing process.env inside the ternary can be
transformed by bundlers; update the envLevel calculation in uiReactLogger to
avoid direct property access on process by using safe optional chaining or a
guarded accessor (e.g. use process?.env?.MCP_APPS_LOG_LEVEL or wrap the
retrieval in a try/catch/helper function), leaving ts, LEVELS, threshold and ok
as-is so the logger still computes the threshold correctly.
🧹 Nitpick comments (1)
packages/codegen/src/watcher.ts (1)
24-46: Consider consolidating with existing logger utility.This file introduces a new structured logger while
packages/codegen/src/utils/logger.tsalready exports adefaultLoggerwith a different format ([mcp-apps-plugin] ${message}vs the new timestamped format). This creates:
- Inconsistent log formats within the same package — some modules will log with timestamps and level tags, others won't
- Duplicate logger implementations to maintain
If the goal is to migrate to structured logging, consider updating the centralized
utils/logger.tsto support the new format and reusing it here, rather than defining a parallel logger.
| // Handle ui/initialize request | ||
| if (message && message.jsonrpc === '2.0' && message.method === 'ui/initialize') { | ||
| console.log('[MCP Host Emulator] Received ui/initialize, responding...'); | ||
| logger.info('[MCP Host Emulator] Received ui/initialize, responding...'); |
There was a problem hiding this comment.
Critical: logger is not available in Playwright init script browser context.
The getPlaywrightInitScript() method returns a JavaScript string that executes in the Playwright browser context. The logger.info(...) calls embedded in this string template (lines 222, 247, 251, 280) reference a logger variable that doesn't exist in the browser—only console is available there.
This will cause ReferenceError: logger is not defined when the init script runs.
🐛 Proposed fix: use console.log in the init script string
// Line 222
- logger.info('[MCP Host Emulator] Received ui/initialize, responding...');
+ console.log('[MCP Host Emulator] Received ui/initialize, responding...'); // Line 247
- logger.info('[MCP Host Emulator] Sent ui/initialize response');
+ console.log('[MCP Host Emulator] Sent ui/initialize response'); // Line 251
- logger.info('[MCP Host Emulator] Sending ui/notifications/tool-result...');
+ console.log('[MCP Host Emulator] Sending ui/notifications/tool-result...'); // Line 280
- logger.info('[MCP Host Emulator] Display mode changed to:', requestedMode, 'sizing:', sizing);
+ console.log('[MCP Host Emulator] Display mode changed to:', requestedMode, 'sizing:', sizing);Also applies to: 247-247, 251-251, 280-280
🤖 Prompt for AI Agents
In `@packages/inspector/src/hosts/mcp-host.ts` at line 222, The embedded
Playwright init script returned by getPlaywrightInitScript() uses logger.info
calls that do not exist in the browser context; update the script string to use
console (e.g., console.log/console.warn/console.error as appropriate) instead of
logger.info at each occurrence inside the string (the occurrences originally at
the logger.info calls around ui/initialize and the other noted spots). Locate
the getPlaywrightInitScript() method and replace those in-string logger.*
references with console.* so the code runs in the browser context without
ReferenceError.
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | ||
| return { mode: mode }; |
There was a problem hiding this comment.
Critical: logger is undefined in Playwright browser context.
Line 248 is inside the getPlaywrightInitScript() template string that gets injected into the browser page. The logger variable only exists in the Node.js context — it won't be defined when this JavaScript executes in the browser, causing a ReferenceError.
This should use console.log (or be removed entirely) since it's browser-side code:
🐛 Proposed fix
- logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);
+ console.log('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| return { mode: mode }; | |
| console.log('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| return { mode: mode }; |
🤖 Prompt for AI Agents
In `@packages/inspector/src/hosts/openai-host.ts` around lines 248 - 249, The
injected script returned by getPlaywrightInitScript contains a browser-side call
to logger (see the requestDisplayMode block), but logger is a Node-only variable
and will cause a ReferenceError in the Playwright page; replace the logger usage
with console.log (or remove the log) inside the template so the browser-executed
code uses the global console instead of the undefined logger. Ensure the change
targets the getPlaywrightInitScript() template string where requestDisplayMode
returns { mode: mode } and references logger.
| } catch { | ||
| // If browser open fails, log the URL for manual copy | ||
| // eslint-disable-next-line no-console | ||
| console.log(`\nOpen this URL in your browser to authorize:\n ${urlStr}\n`); | ||
| logger.info(`\nOpen this URL in your browser to authorize:\n ${urlStr}\n`); | ||
| } |
There was a problem hiding this comment.
User-facing fallback message may be filtered by log level.
This message provides the authorization URL when browser auto-open fails — it's critical for CLI users to complete OAuth. Using logger.info subjects it to MCP_APPS_LOG_LEVEL filtering. If the level is set to warn or higher, users won't see the URL they need to manually open.
Consider using console.log directly (as it was before) or logger.warn to ensure visibility, since this is a user-facing fallback rather than debug output.
🔧 Suggested fix
} catch {
// If browser open fails, log the URL for manual copy
- logger.info(`\nOpen this URL in your browser to authorize:\n ${urlStr}\n`);
+ // Use warn level to ensure visibility even with elevated log levels
+ logger.warn(`\nOpen this URL in your browser to authorize:\n ${urlStr}\n`);
}🤖 Prompt for AI Agents
In `@packages/inspector/src/oauth/preset-config.ts` around lines 397 - 400, The
catch block that logs the authorization URL currently uses logger.info which can
be filtered out by MCP_APPS_LOG_LEVEL; update the catch handler where browser
auto-open is attempted to emit the URL with console.log (or logger.warn) instead
of logger.info so users always see the fallback URL (replace the
logger.info(`\nOpen this URL... ${urlStr}\n`) call with console.log(...) keeping
the same message/format).
| iframe.contentWindow.postMessage(message, "*"); | ||
| // eslint-disable-next-line no-console | ||
| console.log("[MCP Host] Sent ui/notifications/host-context-changed", ctx); | ||
| logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); |
There was a problem hiding this comment.
Critical: logger is not available inside page.evaluate() browser context.
The logger object is a Node.js module-level variable, but page.evaluate() executes code in the Playwright browser context where logger doesn't exist. These calls will throw ReferenceError: logger is not defined at runtime.
The original console.log calls worked because console exists in both Node.js and browser contexts. You should revert these specific lines to use console.log/console.info or remove the logging from within page.evaluate() callbacks entirely.
Affected locations:
- Line 412:
logger.info("[MCP Host] Sent ui/notifications/host-context-changed"...) - Line 442:
logger.info("[OpenAI Host] Sent globals sync:"...) - Line 747:
logger.info("[MCP Host] Stored hostContext update...") - Line 760:
logger.info("[MCP Host] Sent synced event:"...) - Line 796:
logger.info("[OpenAI Host] Sent synced event:"...)
🐛 Proposed fix: revert to console.log inside page.evaluate
// Line 412
- logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx);
+ console.log("[MCP Host] Sent ui/notifications/host-context-changed", ctx); // Line 442
- logger.info("[OpenAI Host] Sent globals sync:", message.data);
+ console.log("[OpenAI Host] Sent globals sync:", message.data); // Line 747
- logger.info("[MCP Host] Stored hostContext update for ui/initialize:", p);
+ console.log("[MCP Host] Stored hostContext update for ui/initialize:", p); // Line 760
- logger.info("[MCP Host] Sent synced event:", m, p);
+ console.log("[MCP Host] Sent synced event:", m, p); // Line 796
- logger.info("[OpenAI Host] Sent synced event:", message.syncType, message.data);
+ console.log("[OpenAI Host] Sent synced event:", message.syncType, message.data);Also applies to: 442-442, 747-747, 760-760, 796-796
🤖 Prompt for AI Agents
In `@packages/inspector/src/widget-session-manager.ts` at line 412, The calls to
logger inside page.evaluate are invalid because logger is a Node-side module not
available in the browser context; locate the page.evaluate(...) callbacks
(references: page.evaluate used where messages are sent such as the MCP/OpenAI
host ui/notifications and synced events) and replace those logger.info calls
with console.log/console.info (or remove the logging) inside the evaluate
callback so the code runs in the Playwright browser context without
ReferenceError; ensure you update all occurrences that the reviewer listed (the
evaluate blocks that emit "[MCP Host] Sent
ui/notifications/host-context-changed", "[OpenAI Host] Sent globals sync:",
"[MCP Host] Stored hostContext update...", "[MCP Host] Sent synced event:",
"[OpenAI Host] Sent synced event:") to use console instead of logger.
| // Lightweight scoped logger (mirrors inspector's createLogger, avoids cross-package dep) | ||
| const uiReactLogger = (() => { | ||
| const source = "ui-react:hooks"; | ||
| const ts = () => new Date().toISOString(); | ||
| const LEVELS: Record<string, number> = { debug: 0, info: 1, warn: 2, error: 3, silent: 4 }; | ||
| // eslint-disable-next-line no-undef, @typescript-eslint/no-unnecessary-condition | ||
| const envLevel = | ||
| typeof process !== "undefined" | ||
| ? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase() | ||
| : ""; | ||
| const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info | ||
| const ok = (l: string): boolean => (LEVELS[l] ?? 1) >= threshold; | ||
| return { | ||
| // eslint-disable-next-line no-console | ||
| warn: (...args: unknown[]) => { | ||
| if (ok("warn")) console.warn(`${ts()} [WARN] [${source}]`, ...args); | ||
| }, | ||
| }; | ||
| })(); |
There was a problem hiding this comment.
ESLint flags process as undefined — guard needs adjustment.
The static analysis reports 'process' is not defined on line 30. While you have typeof process !== "undefined" check, accessing process.env inside the ternary can still cause issues in some bundler configurations where process is stripped entirely.
Consider using optional chaining on the environment access:
🔧 Suggested fix
- // eslint-disable-next-line no-undef, `@typescript-eslint/no-unnecessary-condition`
- const envLevel =
- typeof process !== "undefined"
- ? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase()
- : "";
+ // eslint-disable-next-line no-undef
+ const envLevel = (
+ typeof process !== "undefined" && typeof process.env !== "undefined"
+ ? ((process.env.MCP_APPS_LOG_LEVEL as string | undefined) ?? "")
+ : ""
+ ).toLowerCase();Alternatively, wrap the entire access in a try-catch to handle edge cases where bundlers transform process checks unexpectedly.
🧰 Tools
🪛 ESLint
[error] 30-30: 'process' is not defined.
(no-undef)
🤖 Prompt for AI Agents
In `@packages/ui-react/src/hooks.ts` around lines 22 - 40, The check for process
in the uiReactLogger factory is triggering ESLint because accessing process.env
inside the ternary can be transformed by bundlers; update the envLevel
calculation in uiReactLogger to avoid direct property access on process by using
safe optional chaining or a guarded accessor (e.g. use
process?.env?.MCP_APPS_LOG_LEVEL or wrap the retrieval in a try/catch/helper
function), leaving ts, LEVELS, threshold and ok as-is so the logger still
computes the threshold correctly.
1 participant
Kaizen 2026-02-12
Adds 16 unit tests for
packages/core/src/utils/csp.ts— a previously untested pure utility module.What's tested
generateMcpCSPMetadata— empty config, field mapping, ChatGPT-only field filtering, empty array omissiongenerateOpenAICSPMetadata— snake_case mapping, all 4 CSP fields, empty array omissiongenerateMcpUIMetadata— name fallback from key, description, CSP inclusion/omission, prefersBordergenerateOpenAIUIMetadata— widgetCSP with snake_case, domain field, empty CSP omissionWhy
Test gap:
csp.ts(203 lines, 4 exported functions) had zero test coverage. Pure functions with no side effects — ideal for unit testing.Build ✅ | Tests ✅ (1669 passing)