feat: Add MCP Test Migration and Analysis System#43
Draft
aferd wants to merge 11 commits into
Draft
Conversation
Implements comprehensive test migration and analysis tooling for converting legacy Cypress tests to modern Playwright and Storybook while providing full repository test coverage analysis. ## New MCP Server: hcc-test-migration-mcp - AST-based Cypress test parsing using TypeScript Compiler API - Intelligent test categorization (storybook/unit/e2e/obsolete) - Path security with read-only enforcement for app code - Zod schema validation for all tool inputs ### Tools (4): 1. audit_test_coverage_and_relevance - Detects test obsolescence (90% accuracy) 2. extract_test_logic - Parses Cypress AST into Setup/Triggers/Assertions 3. check_msw_readiness - Identifies API mocking requirements 4. analyze_repo_structure - Scans ALL test types across repository ## New Agent: hcc-frontend-cypress-migration-specialist - Orchestrates complete migration workflow (7 execution modes) - Scope determination: Analysis (ALL tests) vs Migration (Cypress only) - Routes tests to specialist agents (storybook/unit/playwright) - Manages cleanup of migrated Cypress files - Generates actionable gap analysis reports ### Key Features: - Analysis Mode: Scans Cypress, Jest, Storybook, Playwright - Migration Mode: Converts ONLY Cypress tests to modern frameworks - Priority matrix for test generation (CRITICAL/HIGH/MEDIUM/LOW) - 4-week migration roadmap generation - MSW handler code generation from cy.intercept calls ## Documentation - docs/testing-guidelines.md - Comprehensive testing standards - AGENTS.md - Central AI agent index - SCOPE_AMENDMENT_SUMMARY.md - Analysis vs migration scope - TEST_RESULTS.md - Real-world validation results ## Testing Validated against real production repository (notifications-frontend): - Analyzed 219-line Cypress test - 90% selector match accuracy - Extracted 7 test cases via AST - Generated 810-line analysis report - Identified 68 components needing tests ## Breaking Changes None - purely additive ## Migration No migration required Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds a new MCP package and CLI toolset to analyze and migrate Cypress tests to Playwright/Storybook/Jest, four MCP tools for repository/test analysis, path-security utilities and types, multiple CLIs/scripts, TypeScript/Jest/Nx configs, and documentation and Claude/Cursor integration updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "CLI\n(analyze-test-coverage)"
participant MCP as "MCP Server\n(hcc-test-migration-mcp)"
participant FS as "Filesystem\n(pathSecurity)"
participant TS as "TypeScript\nCompiler API"
participant Report as "Report\nGenerator"
User->>CLI: run analysis
CLI->>MCP: start server (stdio) & register tools
MCP->>FS: setRepoRoot / validate paths
CLI->>MCP: call analyze_repo_structure
MCP->>FS: scan repo files
FS-->>MCP: return file list
MCP->>TS: parse files (AST)
TS-->>MCP: return selectors, api calls, test AST
MCP->>Report: compute coverage gaps, MSW handlers, migration plans
MCP-->>CLI: return structured result
CLI->>FS: write TEST_COVERAGE_ANALYSIS.md
CLI->>User: print summary & next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Updates package-lock.json to include @redhat-cloud-services/hcc-test-migration-mcp dependency. Fixes CI build failure where npm ci failed due to missing lock file entries. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
packages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.ts (1)
193-207: Test type detection may misclassify.specfiles.The heuristic assumes
.spec.(ts|tsx)files are Playwright tests (line 194-196), but Jest also commonly uses the.specconvention. This could lead to incorrect test type categorization.Consider checking for additional signals like imports or directory structure.
💡 More robust classification
if (filePath.match(/\.(test|spec)\.(ts|tsx|js|jsx)$/)) { - const testType = filePath.includes('playwright') || filePath.match(/\.spec\.(ts|tsx)$/) - ? 'playwright' - : 'jest'; + let testType: 'playwright' | 'jest' = 'jest'; + if (filePath.includes('playwright') || filePath.includes('/e2e/')) { + testType = 'playwright'; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.ts` around lines 193 - 207, The current heuristic in analyzeRepoStructure.ts that sets testType for files matching /\.(test|spec)\.(ts|tsx|js|jsx)$/ wrongly assumes .spec files are Playwright; update the detection in that block so .spec defaults to 'jest' unless additional Playwright signals are present: read the file content (using the same filePath) and look for Playwright-specific imports (e.g., /@playwright\/test/ or "from 'playwright'") or check directory cues like a 'playwright' or 'e2e' folder in the path; only set testType = 'playwright' when those signals (or filePath.includes('playwright')) are positive, otherwise set 'jest' and continue to push into tests and call markComponentWithTest/guessComponentPath as before.packages/hcc-test-migration-mcp/src/lib/tools/auditTestCoverageAndRelevance.ts (1)
153-158: Bidirectionalincludes()may produce false positive matches.The selector matching logic
element.includes(selector) || selector.includes(element)can incorrectly match substrings. For example, selector"button"would match element"button-submit"and vice versa, inflating the match percentage.Consider using exact matches or word-boundary-aware comparison for more accurate relevance scoring.
💡 Stricter matching approach
const matchingSelectors = testSelectors.filter(selector => componentElements.some(element => - element.includes(selector) || - selector.includes(element) + element === selector || + element.split(/[-_\s]/).includes(selector) || + selector.split(/[-_\s]/).includes(element) ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/auditTestCoverageAndRelevance.ts` around lines 153 - 158, The current bidirectional substring check that builds matchingSelectors (using element.includes(selector) || selector.includes(element)) causes false positives; replace it with a token/word-boundary aware comparison: normalize testSelectors and componentElements, split each into tokens (by non-alphanumeric/underscore) or escape and use RegExp with \b boundaries, then consider a match only when a selector token exactly equals an element token (or regex finds a whole-word match). Update the logic around matchingSelectors, referencing the variables testSelectors, componentElements and matchingSelectors so the relevance count uses exact-token or word-boundary matches instead of raw includes().packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts (1)
342-350: Generated handler names may be invalid JavaScript identifiers.If the URL starts with a number after stripping
/api/, the generated name likehandleGet123endpointis valid, but URLs like/api/123/userswould producehandle...123_userswhich starts the identifier part with a number after the method prefix - this is actually fine. However, empty URLs or URLs with only special characters could producehandle+ method with no suffix.Consider adding a fallback for edge cases.
💡 Add fallback for edge cases
function generateHandlerName(handler: MSWHandler): string { const urlPart = handler.url .replace(/^\/api\//, '') .replace(/[^a-zA-Z0-9]/g, '_') .replace(/_+/g, '_') .replace(/^_|_$/g, ''); - return `handle${handler.method.charAt(0) + handler.method.slice(1).toLowerCase()}${urlPart}`; + const safePart = urlPart || 'Endpoint'; + return `handle${handler.method.charAt(0) + handler.method.slice(1).toLowerCase()}${safePart.charAt(0).toUpperCase() + safePart.slice(1)}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts` around lines 342 - 350, The generateHandlerName function can produce an empty suffix when handler.url is blank or only special characters; update generateHandlerName (and validate MSWHandler.url) to use a safe fallback such as 'Root' or 'Index' when the sanitized urlPart is empty, e.g., compute urlPart then if (!urlPart) urlPart = 'Root'; keep the existing sanitization pipeline (replace regexes) and ensure handler.method exists before accessing charAt/slice to avoid crashes; this guarantees the returned name from generateHandlerName is never just the method prefix.packages/hcc-test-migration-mcp/src/lib/types.ts (1)
3-3: Consider stronger typing forMcpToolto improve type safety.Using
anyforinputSchemaand the callback args loses TypeScript's strict mode benefits. Since tools use Zod schemas, you could leveragez.infer<>for type inference.💡 Example with generic typing
import { z, ZodSchema } from 'zod'; import { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; export type McpTool<T extends ZodSchema = ZodSchema> = [ string, { description: string; inputSchema: z.ZodRawShape }, (args: z.infer<T>) => Promise<CallToolResult> ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/types.ts` at line 3, The McpTool type currently uses any for inputSchema and callback args which weakens type safety; update the McpTool declaration to be generic over a Zod schema (referencing McpTool, inputSchema and the callback function signature) so the tuple second element holds a Zod schema type and the function argument uses z.infer<T> and still returns Promise<CallToolResult>; in short, introduce a generic type parameter constrained to ZodSchema (or ZodRawShape as appropriate), replace the any occurrences with that schema type and z.infer<> for the callback arg to preserve runtime schema validation while restoring compile-time types.AGENTS.md (1)
13-38: Add language specifier to fenced code block.The static analysis tool flagged this code block as missing a language specifier. For ASCII diagrams, use
textorplaintext.📝 Suggested fix
-``` +```text AGENTS.md (this file) ├── Entry point for AI agent context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 13 - 38, The fenced diagram block in AGENTS.md is missing a language specifier; update the opening triple backticks for the ASCII tree to include a language like `text` or `plaintext` (e.g., change "```" to "```text") so the static analyzer stops flagging it; locate the diagram block shown in the file (the ASCII tree starting with "AGENTS.md (this file)") and apply the specifier to that opening fence.TEST_RESULTS.md (2)
283-286: Remove absolute local file paths from documentation.Line 283 contains an absolute local file path (
/Users/aferdina/notifications-frontend/) which is specific to the developer's machine and not portable. Consider using a relative path or placeholder.📝 Suggested fix
**Generated Artifacts**: -1. ✅ `/Users/aferdina/notifications-frontend/MIGRATION_ANALYSIS.md` (405 lines) +1. ✅ `<repo-root>/MIGRATION_ANALYSIS.md` (405 lines) - Complete analysis of NotificationsDrawer.cy.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TEST_RESULTS.md` around lines 283 - 286, Replace the absolute developer-specific path "/Users/aferdina/notifications-frontend/MIGRATION_ANALYSIS.md" in TEST_RESULTS.md with a portable reference (e.g., "MIGRATION_ANALYSIS.md", "notifications-frontend/MIGRATION_ANALYSIS.md", or a placeholder like "<REPO_ROOT>/notifications-frontend/MIGRATION_ANALYSIS.md"); locate the string in TEST_RESULTS.md (around the existing entry for the MIGRATION_ANALYSIS.md file) and update it so the documentation no longer contains a user-local absolute path.
33-41: Add language specifiers to code blocks.Static analysis flagged several code blocks missing language specifiers. For plain text output or lists, use
textorplaintext.Also applies to: 192-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TEST_RESULTS.md` around lines 33 - 41, Several fenced code blocks (e.g., the one listing selectors like `#drawer-toggle`, `[aria-label="Notification actions dropdown"]`, `.pf-m-read`, `[role="menuitem"]`, `.pf-v6-c-notification-drawer__list-item`) are missing language specifiers; update those fenced blocks to include an explicit language tag such as ```text or ```plaintext so static analysis treats them as plain text, and apply the same change to any other similar blocks elsewhere in the file.packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts (2)
11-14: Consider stricter typing for tool arguments.The
argsparameter usesanytype which bypasses TypeScript's type checking. While Zod validates at runtime, adding compile-time type safety would catch issues earlier.♻️ Suggested type improvement
- async function tool(args: any): Promise<CallToolResult> { + async function tool(args: unknown): Promise<CallToolResult> { try { const input = ExtractInputSchema.parse(args);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts` around lines 11 - 14, The tool's inner function currently types its parameter as any which disables compile-time checking; change the signature of tool to accept a typed argument inferred from the Zod schema (e.g. use z.infer<typeof ExtractInputSchema> or create a type alias like ExtractInput = z.infer<typeof ExtractInputSchema>) so the parameter for tool (and any outward McpTool type if applicable) is strongly typed while still using ExtractInputSchema.parse at runtime; update the function declaration for extractTestLogicTool -> tool to use that inferred type and adjust any callers/types accordingly (references: extractTestLogicTool, tool, ExtractInputSchema).
330-352: Selector extraction may miss some Cypress patterns.The
extractSelectorfunction only looks forcy.get()calls. Cypress tests commonly use other element selection methods likecy.contains(),cy.find(),cy.within(), orcy.get().find()chains that won't be captured.♻️ Extended selector extraction
function extractSelector(node: ts.Node): string | undefined { let current: ts.Node = node; // Walk up the chain looking for cy.get() while (current) { if (ts.isCallExpression(current)) { const expr = current.expression; - if (ts.isPropertyAccessExpression(expr) && expr.name.text === 'get') { + if (ts.isPropertyAccessExpression(expr) && + (expr.name.text === 'get' || expr.name.text === 'contains' || expr.name.text === 'find')) { return extractFirstStringArg(current); } if (ts.isPropertyAccessExpression(expr)) { current = expr.expression; continue; } } break; } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts` around lines 330 - 352, The extractSelector function currently only recognizes cy.get() calls; update it to detect other Cypress selection patterns (e.g., contains, find, within and chained calls like cy.get().find()) by checking for PropertyAccessExpression names 'get', 'contains', 'find', and 'within' in extractSelector and returning extractFirstStringArg for those calls; when encountering a PropertyAccessExpression that is a chained call, continue walking up the chain by setting current to the inner expression (expr.expression) or the call's expression as appropriate, and also allow detection when the base is the identifier 'cy' or another call expression so chains like cy.get(...).find(...).contains(...) are handled. Ensure extractSelector still returns undefined when no string argument is found.packages/hcc-test-migration-mcp/README.md (1)
63-67: Documentation slightly inconsistent with write-allowed patterns.The README lists
**/*.stories.tsxbut the implementation inpathSecurity.ts(line 33) uses/\.stories\.(ts|tsx|js|jsx)$/which also allows.stories.ts,.stories.js, and.stories.jsx. Consider updating the documentation to reflect all allowed patterns for consistency.📝 Suggested documentation update
- `**/tests/**` - `**/*.test.ts` - `**/*.test.tsx` -- `**/*.stories.tsx` +- `**/*.stories.ts`, `**/*.stories.tsx` - Playwright E2E configuration files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/README.md` around lines 63 - 67, The README's allowed write patterns list is narrower than the actual acceptance in pathSecurity.ts; update the README entry for stories to match the regex /\.stories\.(ts|tsx|js|jsx)$/ used in pathSecurity.ts by replacing the single `**/*.stories.tsx` line with explicit entries for all allowed story extensions (e.g., `**/*.stories.ts`, `**/*.stories.tsx`, `**/*.stories.js`, `**/*.stories.jsx`) so the documentation accurately reflects the code.claude/agents/hcc-frontend-cypress-migration-specialist.md (1)
270-282: Consider allowing at least one retry for flaky network conditions.The CI optimization config specifies
retries: 0which may cause false failures in E2E tests due to network flakiness or timing issues. Consider allowing at least 1 retry for production stability while still maintaining fast-fail behavior.♻️ Suggested CI config adjustment
export default defineConfig({ workers: 1, // Single-threaded - retries: 0, // No retries + retries: 1, // Single retry for flaky tests maxFailures: 2, // Fast fail use: { trace: 'retain-on-failure', screenshot: 'only-on-failure', }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude/agents/hcc-frontend-cypress-migration-specialist.md` around lines 270 - 282, Change the Playwright CI config to allow a single retry to reduce false negatives: update the defineConfig block where workers, retries, maxFailures, and use are defined (look for the export default defineConfig({...}) with keys workers, retries, maxFailures, use) to set retries: 1 instead of 0 while keeping workers: 1 and maxFailures: 2 and the tracing/screenshot settings intact so you retain fast-fail behavior but tolerate transient network flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing-guidelines.md`:
- Around line 181-203: The fenced code block showing the project tree is missing
a language specifier; update the opening fence from ``` to ```text (or
```plaintext) so the block is marked as plain text for markdown linters and
renderers — locate the project tree block in docs/testing-guidelines.md and
change its opening fence to include the language specifier.
- Line 18: Update the docs guidance that currently references the deprecated
package name "@testing-library/react-hooks" to instruct using the built-in
renderHook API from "@testing-library/react" for React 18+; specifically, change
the reference on the line mentioning "@testing-library/react-hooks" to recommend
importing renderHook from "@testing-library/react" and using renderHook for hook
testing (mentioning renderHook by name to locate the relevant guidance).
In `@packages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.ts`:
- Around line 54-65: The template renders percentage calculations using
totalComponents which can be zero; add a guard in analyzeRepoStructure.ts to
handle empty repositories by checking if totalComponents === 0 and returning 0%
(or a suitable "0" display) for all percentage fields instead of performing
divisions. Update calculations that reference totalComponents,
componentsWithNoTests, componentsWithJest, componentsWithStorybook,
componentsWithPlaywright, and componentsWithCypress so they compute percentages
only when totalComponents > 0 (fallback to 0 or "0.0" as the formatted string),
ensuring the report never produces NaN.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 254-269: The cy.intercept parsing currently treats the first
argument as the HTTP method which misclassifies single-argument calls; update
the CallExpression handling (where ts.isPropertyAccessExpression(expression) and
expression.name.text === 'intercept') to inspect arguments and types: if there
is one string argument treat it as the URL and set method = 'GET'; if there are
two string args treat args[0] as method and args[1] as URL; if the first arg is
an object (routeMatcher) attempt to read its 'url' property (falling back to
'dynamic') and default method to 'GET'; continue to push into intercepts (same
shape) using extractFirstStringArg as needed to safely extract strings.
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts`:
- Around line 377-403: The categorizeTest function's signature claims it can
return 'obsolete' but the implementation never does; remove 'obsolete' from its
return type to keep the contract accurate: change the return union in
categorizeTest from "'storybook' | 'unit' | 'e2e' | 'obsolete'" to "'storybook'
| 'unit' | 'e2e'" and update any dependent type definitions or imports (see
types.ts where 'obsolete' is defined) and callers expecting 'obsolete' so types
remain consistent; alternatively, if you prefer detecting obsolete here, add an
explicit branch at the top of categorizeTest that returns 'obsolete' for
empty/skip-only tests (e.g., setup,triggers,assertions length all zero or only
skip markers) and document the condition—pick one approach and apply the
corresponding signature and caller adjustments.
In `@packages/hcc-test-migration-mcp/tsconfig.json`:
- Around line 4-5: Add a top-level "type": "module" field to package.json to
explicitly mark the package as ESM so Node treats emitted .js files as ES
modules (this complements the tsconfig settings "module": "ESNext" and
"moduleResolution": "Node"); update package.json (top-level object) to include
"type": "module" alongside existing fields like "exports" to remove ambiguity
for Node and bundlers.
In `@test-migration-tools.ts`:
- Around line 36-39: The code dereferences auditResult.content[0] without
checking for an empty array; update each block that uses const content =
auditResult.content[0] (the four result blocks around the existing occurrences)
to first guard that auditResult.content exists and has length > 0, and handle
the empty case (e.g., log a clear warning/error or skip processing) instead of
proceeding to read content.type or content.text; ensure the same pattern is
applied consistently to all four result blocks so accessing content is safe.
- Around line 17-20: The hard-coded notificationsRepo path should be replaced
with a parameterized value: read from an environment variable (e.g.
process.env.NOTIFICATIONS_REPO) or a CLI argument and fall back to a safe
relative/default path; then call setRepoRoot(notificationsRepo) with that value
and keep the console.log. Update the variable currently named notificationsRepo
and the call to setRepoRoot so the script no longer contains the absolute
user-specific path.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 13-38: The fenced diagram block in AGENTS.md is missing a language
specifier; update the opening triple backticks for the ASCII tree to include a
language like `text` or `plaintext` (e.g., change "```" to "```text") so the
static analyzer stops flagging it; locate the diagram block shown in the file
(the ASCII tree starting with "AGENTS.md (this file)") and apply the specifier
to that opening fence.
In `@claude/agents/hcc-frontend-cypress-migration-specialist.md`:
- Around line 270-282: Change the Playwright CI config to allow a single retry
to reduce false negatives: update the defineConfig block where workers, retries,
maxFailures, and use are defined (look for the export default
defineConfig({...}) with keys workers, retries, maxFailures, use) to set
retries: 1 instead of 0 while keeping workers: 1 and maxFailures: 2 and the
tracing/screenshot settings intact so you retain fast-fail behavior but tolerate
transient network flakiness.
In `@packages/hcc-test-migration-mcp/README.md`:
- Around line 63-67: The README's allowed write patterns list is narrower than
the actual acceptance in pathSecurity.ts; update the README entry for stories to
match the regex /\.stories\.(ts|tsx|js|jsx)$/ used in pathSecurity.ts by
replacing the single `**/*.stories.tsx` line with explicit entries for all
allowed story extensions (e.g., `**/*.stories.ts`, `**/*.stories.tsx`,
`**/*.stories.js`, `**/*.stories.jsx`) so the documentation accurately reflects
the code.
In `@packages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.ts`:
- Around line 193-207: The current heuristic in analyzeRepoStructure.ts that
sets testType for files matching /\.(test|spec)\.(ts|tsx|js|jsx)$/ wrongly
assumes .spec files are Playwright; update the detection in that block so .spec
defaults to 'jest' unless additional Playwright signals are present: read the
file content (using the same filePath) and look for Playwright-specific imports
(e.g., /@playwright\/test/ or "from 'playwright'") or check directory cues like
a 'playwright' or 'e2e' folder in the path; only set testType = 'playwright'
when those signals (or filePath.includes('playwright')) are positive, otherwise
set 'jest' and continue to push into tests and call
markComponentWithTest/guessComponentPath as before.
In
`@packages/hcc-test-migration-mcp/src/lib/tools/auditTestCoverageAndRelevance.ts`:
- Around line 153-158: The current bidirectional substring check that builds
matchingSelectors (using element.includes(selector) ||
selector.includes(element)) causes false positives; replace it with a
token/word-boundary aware comparison: normalize testSelectors and
componentElements, split each into tokens (by non-alphanumeric/underscore) or
escape and use RegExp with \b boundaries, then consider a match only when a
selector token exactly equals an element token (or regex finds a whole-word
match). Update the logic around matchingSelectors, referencing the variables
testSelectors, componentElements and matchingSelectors so the relevance count
uses exact-token or word-boundary matches instead of raw includes().
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 342-350: The generateHandlerName function can produce an empty
suffix when handler.url is blank or only special characters; update
generateHandlerName (and validate MSWHandler.url) to use a safe fallback such as
'Root' or 'Index' when the sanitized urlPart is empty, e.g., compute urlPart
then if (!urlPart) urlPart = 'Root'; keep the existing sanitization pipeline
(replace regexes) and ensure handler.method exists before accessing charAt/slice
to avoid crashes; this guarantees the returned name from generateHandlerName is
never just the method prefix.
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts`:
- Around line 11-14: The tool's inner function currently types its parameter as
any which disables compile-time checking; change the signature of tool to accept
a typed argument inferred from the Zod schema (e.g. use z.infer<typeof
ExtractInputSchema> or create a type alias like ExtractInput = z.infer<typeof
ExtractInputSchema>) so the parameter for tool (and any outward McpTool type if
applicable) is strongly typed while still using ExtractInputSchema.parse at
runtime; update the function declaration for extractTestLogicTool -> tool to use
that inferred type and adjust any callers/types accordingly (references:
extractTestLogicTool, tool, ExtractInputSchema).
- Around line 330-352: The extractSelector function currently only recognizes
cy.get() calls; update it to detect other Cypress selection patterns (e.g.,
contains, find, within and chained calls like cy.get().find()) by checking for
PropertyAccessExpression names 'get', 'contains', 'find', and 'within' in
extractSelector and returning extractFirstStringArg for those calls; when
encountering a PropertyAccessExpression that is a chained call, continue walking
up the chain by setting current to the inner expression (expr.expression) or the
call's expression as appropriate, and also allow detection when the base is the
identifier 'cy' or another call expression so chains like
cy.get(...).find(...).contains(...) are handled. Ensure extractSelector still
returns undefined when no string argument is found.
In `@packages/hcc-test-migration-mcp/src/lib/types.ts`:
- Line 3: The McpTool type currently uses any for inputSchema and callback args
which weakens type safety; update the McpTool declaration to be generic over a
Zod schema (referencing McpTool, inputSchema and the callback function
signature) so the tuple second element holds a Zod schema type and the function
argument uses z.infer<T> and still returns Promise<CallToolResult>; in short,
introduce a generic type parameter constrained to ZodSchema (or ZodRawShape as
appropriate), replace the any occurrences with that schema type and z.infer<>
for the callback arg to preserve runtime schema validation while restoring
compile-time types.
In `@TEST_RESULTS.md`:
- Around line 283-286: Replace the absolute developer-specific path
"/Users/aferdina/notifications-frontend/MIGRATION_ANALYSIS.md" in
TEST_RESULTS.md with a portable reference (e.g., "MIGRATION_ANALYSIS.md",
"notifications-frontend/MIGRATION_ANALYSIS.md", or a placeholder like
"<REPO_ROOT>/notifications-frontend/MIGRATION_ANALYSIS.md"); locate the string
in TEST_RESULTS.md (around the existing entry for the MIGRATION_ANALYSIS.md
file) and update it so the documentation no longer contains a user-local
absolute path.
- Around line 33-41: Several fenced code blocks (e.g., the one listing selectors
like `#drawer-toggle`, `[aria-label="Notification actions dropdown"]`,
`.pf-m-read`, `[role="menuitem"]`, `.pf-v6-c-notification-drawer__list-item`)
are missing language specifiers; update those fenced blocks to include an
explicit language tag such as ```text or ```plaintext so static analysis treats
them as plain text, and apply the same change to any other similar blocks
elsewhere in the file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30da0fe5-3365-4dfd-adbe-56f4477771d7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
AGENTS.mdSCOPE_AMENDMENT_SUMMARY.mdTEST_RESULTS.mdclaude/.claude-plugin/plugin.jsonclaude/agents/hcc-frontend-cypress-migration-specialist.mdcursor/rules/cypress-migration-specialist.mdcdocs/testing-guidelines.mdpackages/hcc-test-migration-mcp/LICENSEpackages/hcc-test-migration-mcp/README.mdpackages/hcc-test-migration-mcp/jest.config.tspackages/hcc-test-migration-mcp/package.jsonpackages/hcc-test-migration-mcp/project.jsonpackages/hcc-test-migration-mcp/src/index.tspackages/hcc-test-migration-mcp/src/lib/index.tspackages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.tspackages/hcc-test-migration-mcp/src/lib/tools/auditTestCoverageAndRelevance.tspackages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.tspackages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tspackages/hcc-test-migration-mcp/src/lib/types.tspackages/hcc-test-migration-mcp/src/lib/utils/pathSecurity.tspackages/hcc-test-migration-mcp/tsconfig.jsonpackages/hcc-test-migration-mcp/tsconfig.lib.jsonpackages/hcc-test-migration-mcp/tsconfig.spec.jsontest-migration-tools.ts
Fixes CI test failure caused by Jest exiting with code 1 when no test files are found. Matches configuration pattern used in hcc-pf-mcp package. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Critical fixes: - Update deprecated @testing-library/react-hooks reference to renderHook - Add language specifiers to fenced code blocks (markdown lint) - Guard against division by zero in repository analysis percentages - Improve cy.intercept parsing to handle single-arg and object patterns - Remove 'obsolete' from categorizeTest return type (never returned) - Replace hard-coded path with environment variable in test script - Add guards for content[0] array access to prevent crashes - Fix generateHandlerName to use fallback when URL produces empty string - Enhance extractSelector to support contains/find/within methods - Remove absolute user paths from documentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add testCode parameter to categorizeTest function - Check for cy.mount() as definitive indicator of component tests - This fixes misclassification of component tests as E2E tests Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix regex to match **Category**: format in report output - Remove debug console.log statements - Add icon exclusion note to analysis reports - Update migration analysis script to correctly parse categories Tests now correctly categorized: - NotificationsDrawer.cy.tsx: STORYBOOK (was E2E) - UserAccessGroupsDataView.cy.tsx: STORYBOOK (was E2E) - spec.cy.ts: E2E (correct) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit reorganizes the test migration toolkit into two clear categories: **General Test Toolkit** (repository-agnostic, standard stack): - analyze-test-coverage.ts (renamed from generate-analysis-report.ts) - Analyzes coverage for ALL test types (Jest, Storybook, Playwright, Cypress) - Identifies gaps in standard testing stack - Repository-agnostic with no hardcoded paths - build-test-gaps.ts (NEW) - Builds action plan for filling test coverage gaps - Filters by priority (high/medium/low) and type (storybook/jest/playwright) - Routes to appropriate agents for test generation - Prescriptive: Storybook for components, Jest for units, Playwright for E2E **Cypress Migration Toolkit** (Cypress-specific): - migrate-cypress-tests.ts (enhanced) - Auto-discovers Cypress tests in any repository - Categorizes and routes to standard stack (Storybook/Jest/Playwright) - Repository-agnostic with no hardcoded paths or test lists **Key Changes**: - Removed all hardcoded paths (NOTIFICATIONS_REPO, /Users/aferdina/notifications-frontend) - Changed env var from NOTIFICATIONS_REPO to TARGET_REPO - Support command-line args: tsx <script> /path/to/repo - Auto-discovery of Cypress tests instead of hardcoded list - Added comprehensive USAGE.md documentation - Clarified prescriptive testing strategy: Storybook, Jest, Playwright - All tools now repository-agnostic and reusable Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Created test-toolkit-utils.ts to consolidate common functionality across all three test toolkit scripts, eliminating redundant code. **Extracted shared utilities:** - getRepoPath() - Get repository path from CLI args/env/cwd - validateRepoPath() - Validate path exists and show usage on error - extractTextContent() - Extract text from MCP tool CallToolResult - findCypressTests() - Recursively discover Cypress test files - parseOptions() - Parse command-line options in --key=value format **Before:** - Cypress test discovery duplicated in 2 files (60+ lines each) - Repository path validation duplicated in 3 files (8+ lines each) - MCP result extraction duplicated in 3 files (4+ lines each) **After:** - Single source of truth for all shared logic - Consistent behavior across all scripts - Easier maintenance and testing - Reduced file sizes **Impact:** - analyze-test-coverage.ts: -28 lines (cleaner) - migrate-cypress-tests.ts: -45 lines (cleaner) - build-test-gaps.ts: -15 lines (cleaner) - test-toolkit-utils.ts: +95 lines (new shared module) - Net reduction: ~93 lines of duplicated code removed No functional changes - pure refactoring for maintainability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed files from experimentation and early development that are not needed: - test-migration-tools.ts (old demo/testing script) - SCOPE_AMENDMENT_SUMMARY.md (internal documentation) - TEST_RESULTS.md (test results from development) - hcc-frontend-cypress-migration-specialist agent (superseded by general toolkit) - cypress-migration-specialist cursor rules (superseded) These were artifacts from the development process and are not part of the final toolkit deliverable. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
migrate-cypress-tests.ts (1)
35-37: Fragile regex patterns depend on exact markdown formatting.The patterns
Is Relevant**: ✅andRecommendation: \*\*([A-Z]+)\*\*/are tightly coupled to the audit tool's output format. If that format changes, these will silently fail and use defaults.Consider extracting a structured object from the tool result instead of parsing markdown, or adding a warning when patterns don't match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrate-cypress-tests.ts` around lines 35 - 37, The current parsing of auditText (variables isRelevant, recommendation, reason) uses brittle markdown-dependent regexes; update the logic in migrate-cypress-tests.ts to first attempt to read a structured result object from the audit tool (e.g., check for a JSON/fields payload instead of parsing markdown), and if that’s not available, replace the exact-match regexes with more tolerant patterns (e.g., case-insensitive, allow optional markdown tokens and emojis) and add explicit warnings/logging when a pattern fails to match (so defaults aren’t silently used); locate and update the code that sets isRelevant, recommendation, and reason and ensure auditText matching falls back to a logged-warning path.test-toolkit-utils.ts (2)
83-101: Avoidanyto satisfy TypeScript strict mode.The
options: any = {}(line 87) bypasses type checking. Consider using a type-safe accumulator pattern:♻️ Proposed type-safe implementation
export function parseOptions<T extends Record<string, string | boolean>>( startIndex: number, allowedFlags: string[] = [] ): T { - const options: any = {}; + const options: Record<string, string | boolean> = {}; process.argv.slice(startIndex).forEach(arg => { if (arg.includes('=')) { const [key, value] = arg.replace(/^--/, '').split('='); - options[key] = value; + if (key) options[key] = value ?? ''; } else if (arg.startsWith('--')) { const flag = arg.replace(/^--/, ''); if (allowedFlags.includes(flag)) { options[flag] = true; } } }); return options as T; }As per coding guidelines: "All code must pass TypeScript strict mode type checking".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-toolkit-utils.ts` around lines 83 - 101, Replace the unsafe any accumulator in parseOptions with a type-safe accumulator such as Partial<T> (or Record<string, string|boolean>) named options, and when assigning use typed assertions for keys/values (e.g., treat key as keyof T and assign string or boolean values appropriately), then return options as T; specifically update the variable declaration (options) and each assignment site in parseOptions to avoid any while preserving the existing runtime behavior.
61-63: Silent error swallowing may hide issues.The empty catch block discards all errors without logging. Consider logging at debug level or adding a comment explaining the intent:
💡 Suggested improvement
} catch (err) { - // Directory doesn't exist or can't be read + // Expected: directory doesn't exist or can't be read (non-fatal) + // Unexpected errors (permissions, symlink loops) are also silently ignored }If permission errors or other unexpected failures should be surfaced, consider logging them at a debug level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-toolkit-utils.ts` around lines 61 - 63, The empty catch(err) block silently swallows filesystem errors; update that catch to either log the error at debug level (including the err variable and context that the directory read failed) or add an explicit comment explaining why errors are intentionally ignored; ensure the change is made in the catch following the directory read try-block so callers can diagnose permission/IO issues when needed.packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts (1)
307-315: MSW detection heuristic may produce false positives.The check for
'initialize'is very generic and could match unrelated code (e.g.,initializeApp(),initializevariable). Consider more specific patterns:♻️ Proposed more specific check
function checkMSWSetup(storybookConfig: string): boolean { - // Check for MSW imports or setup return ( - storybookConfig.includes('msw') || - storybookConfig.includes('Mock Service Worker') || - storybookConfig.includes('initialize') || - storybookConfig.includes('mswDecorator') + storybookConfig.includes("from 'msw'") || + storybookConfig.includes('from "msw"') || + storybookConfig.includes('mswDecorator') || + storybookConfig.includes('mswLoader') || + /msw\s*\./.test(storybookConfig) // msw.something ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts` around lines 307 - 315, The current checkMSWSetup function uses a very generic 'initialize' substring which causes false positives; update checkMSWSetup to use more specific MSW indicators (e.g., 'msw/node', 'setupWorker', 'setupServer', 'mswDecorator', 'Mock Service Worker') and replace the plain 'initialize' check with a word-boundary/usage-aware pattern such as a regex that matches initialize as a call (e.g., /\binitialize\s*\(/) or other MSW-specific initialization functions; modify the logic inside checkMSWSetup to test these more precise strings/regexes so it only flags real MSW setup code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@analyze-test-coverage.ts`:
- Around line 69-73: The current extraction using lines, summaryStart and
summaryEnd can produce wrong slices when `## Summary` is missing or when there's
no subsequent `## ` heading; update the logic in the block that sets `lines`,
`summaryStart`, `summaryEnd` and `summary` so it first checks `if (summaryStart
=== -1)` and returns or sets `summary` to an empty string/skip processing, and
then normalizes `summaryEnd` with `if (summaryEnd === -1) summaryEnd =
lines.length;` before calling `lines.slice(summaryStart,
summaryEnd).join('\n')`; refer to the variables `lines`, `summaryStart`,
`summaryEnd`, and `summary` to locate and change the code.
In `@build-test-gaps.ts`:
- Around line 44-49: The code currently asserts options.type as
'jest'|'storybook'|'playwright' without validating it, so an invalid --type
silently yields no results; update the block that reads options.type and assigns
testType to first check that options.type is one of the allowed values
(['jest','storybook','playwright']), and if not, report a clear error (or usage
message) and exit/throw; then apply the existing filteredGaps =
filteredGaps.filter(g => g.missingTests.includes(testType)) only when the value
is valid. Ensure you reference options.type, testType, and the
filteredGaps.filter/missingTests logic when making the change.
In `@migrate-cypress-tests.ts`:
- Around line 78-79: The requiresMSW boolean is inverted because it checks
mswText for the wrong token; update the logic in the requiresMSW assignment
(which reads mswText) to detect the MSW-required marker used by
checkMSWReadiness.ts (look for the '⚠️ Yes' / '⚠️' indicator rather than 'Has
API Calls**: ✅') so requiresMSW is true when MSW is actually required; adjust
the console message use accordingly to keep output consistent with the corrected
requiresMSW value.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 273-281: The two-argument branch can miss setting method/url when
firstArg is truthy but node.arguments[1] isn't a string literal; update the
branch in checkMSWReadiness.ts so that when firstArg is truthy you always assign
method = firstArg, then attempt to set url: if node.arguments[1] is a string
literal use node.arguments[1].text, otherwise fall back to
ts.isStringLiteral(node.arguments[0]) ? node.arguments[0].text; keep references
to extractFirstStringArg, node.arguments, method, and url so the code reliably
captures the method and any available literal URL in the (arg0,arg1) case.
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts`:
- Around line 285-297: extractIntercept currently returns null for
single-argument cy.intercept calls; update extractIntercept to accept the
single-argument form by treating a lone string argument as the URL (and
defaulting method to 'GET' when the first argument is not a string literal),
mirroring the logic used in extractCypressIntercepts in checkMSWReadiness.ts:
use extractStringFromArg on the appropriate argument (arguments[0] when
length===1, or arguments[1] when length>=2) and set method to
ts.isStringLiteral(arguments[0]) ? arguments[0].text : 'GET', then return the
same SetupAction shape as before. Ensure you keep the function name
extractIntercept and reuse extractStringFromArg so behavior is consistent with
extractCypressIntercepts.
---
Nitpick comments:
In `@migrate-cypress-tests.ts`:
- Around line 35-37: The current parsing of auditText (variables isRelevant,
recommendation, reason) uses brittle markdown-dependent regexes; update the
logic in migrate-cypress-tests.ts to first attempt to read a structured result
object from the audit tool (e.g., check for a JSON/fields payload instead of
parsing markdown), and if that’s not available, replace the exact-match regexes
with more tolerant patterns (e.g., case-insensitive, allow optional markdown
tokens and emojis) and add explicit warnings/logging when a pattern fails to
match (so defaults aren’t silently used); locate and update the code that sets
isRelevant, recommendation, and reason and ensure auditText matching falls back
to a logged-warning path.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 307-315: The current checkMSWSetup function uses a very generic
'initialize' substring which causes false positives; update checkMSWSetup to use
more specific MSW indicators (e.g., 'msw/node', 'setupWorker', 'setupServer',
'mswDecorator', 'Mock Service Worker') and replace the plain 'initialize' check
with a word-boundary/usage-aware pattern such as a regex that matches initialize
as a call (e.g., /\binitialize\s*\(/) or other MSW-specific initialization
functions; modify the logic inside checkMSWSetup to test these more precise
strings/regexes so it only flags real MSW setup code.
In `@test-toolkit-utils.ts`:
- Around line 83-101: Replace the unsafe any accumulator in parseOptions with a
type-safe accumulator such as Partial<T> (or Record<string, string|boolean>)
named options, and when assigning use typed assertions for keys/values (e.g.,
treat key as keyof T and assign string or boolean values appropriately), then
return options as T; specifically update the variable declaration (options) and
each assignment site in parseOptions to avoid any while preserving the existing
runtime behavior.
- Around line 61-63: The empty catch(err) block silently swallows filesystem
errors; update that catch to either log the error at debug level (including the
err variable and context that the directory read failed) or add an explicit
comment explaining why errors are intentionally ignored; ensure the change is
made in the catch following the directory read try-block so callers can diagnose
permission/IO issues when needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e299b399-7f5f-40a3-9f37-49ff85a569d4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
AGENTS.mdUSAGE.mdanalyze-test-coverage.tsbuild-test-gaps.tsdocs/testing-guidelines.mdmigrate-cypress-tests.tspackages/hcc-test-migration-mcp/project.jsonpackages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.tspackages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.tspackages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tstest-toolkit-utils.ts
✅ Files skipped from review due to trivial changes (4)
- packages/hcc-test-migration-mcp/project.json
- AGENTS.md
- docs/testing-guidelines.md
- USAGE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hcc-test-migration-mcp/src/lib/tools/analyzeRepoStructure.ts
- Fix MSW detection to check for '⚠️ Yes' output - Add graceful handling for missing summary sections - Support single-argument cy.intercept() calls - Validate --type option in build-test-gaps - Improve intercept parsing for non-literal arguments - Make audit text parsing more robust with multiple patterns - Use MSW-specific patterns for detection (worker.start, setupWorker) - Fix TypeScript type safety in parseOptions - Add error logging for directory read failures Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
migrate-cypress-tests.ts (2)
35-39: Fragile regex parsing for audit results.The code uses multiple regex patterns to extract values from markdown output. If the output format changes slightly (e.g., extra whitespace, different markdown formatting), parsing will silently fail and fall back to defaults. Consider:
- Returning structured data from the tools alongside markdown
- Using more resilient parsing patterns
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrate-cypress-tests.ts` around lines 35 - 39, The markdown parsing for auditText is fragile: update the extraction logic (affecting isRelevant, recMatch/recommendation, reasonMatch/reason) to use resilient patterns and normalization—first normalize auditText (trim, collapse whitespace, make case-insensitive) then run single robust regexes that accept optional bold markers (**) and flexible spacing or fallback to parsing key:value pairs; alternatively have the tool return structured JSON alongside markdown and prefer that structured payload when present. Ensure the new patterns use non-greedy matches for reason, are case-insensitive, and only fall back to 'MIGRATE'/'Unknown' after both structured-data and resilient-regex attempts fail.
129-141: Error captured inreasonfield may expose stack traces.When an error occurs,
${error}is stored directly in the migration plan'sreason. Iferroris an Error object, this could include stack traces that clutter output. Consider using only the message.♻️ Cleaner error message
plans.push({ testPath: test.testPath, componentPath: test.componentPath, isRelevant: false, recommendation: 'KEEP', category: 'e2e', testCount: 0, requiresMSW: false, - reason: `Error: ${error}` + reason: `Error: ${error instanceof Error ? error.message : String(error)}` });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrate-cypress-tests.ts` around lines 129 - 141, The catch block that pushes to plans is inserting the full error object into the reason field (in migrate-cypress-tests.ts, inside the catch handling for each test), which can include stack traces; change the assignment so reason uses only the error message string (e.g., derive a safe string with error instanceof Error ? error.message : String(error)) before pushing to plans (reference: plans array push, test.testPath/test.componentPath and the reason property) so stack traces are not stored in the migration plan.analyze-test-coverage.ts (1)
65-65: Inconsistent spacing inrepeatcalls.Line 65 uses
'=' .repeat(80)(with a space before.) while other instances use'='.repeat(80). This is a minor style inconsistency.✏️ Fix spacing
- console.log('=' .repeat(80)); + console.log('='.repeat(80));- console.log('=' .repeat(80) + '\n'); + console.log('='.repeat(80) + '\n');Also applies to: 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@analyze-test-coverage.ts` at line 65, Remove the stray space before the property access operator in the string.repeat calls so they match the project's style; change occurrences like "'=' .repeat(80)" to "'='.repeat(80)" (also fix the similar instance around the other occurrence noted), locating them in analyze-test-coverage.ts where the '=' string's repeat is used.packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts (2)
311-322: MSW detection relies on brittle substring matching.The
checkMSWSetupfunction uses simpleincludes()checks which could produce false positives (e.g., a comment mentioning "msw" or a variable namedmsw_unrelated). Consider using more precise patterns or AST-based detection for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts` around lines 311 - 322, The MSW detection in checkMSWSetup is brittle because it uses simple substring includes; replace those naive checks with precise pattern matching (e.g., anchored/word-boundary regexes) or an AST-based check to avoid matching comments or identifiers like msw_unrelated; specifically, update checkMSWSetup to detect real import/require statements and function calls (look for patterns for import/from 'msw', require('msw'), mswDecorator as an identifier, and calls like worker.start(...), setupWorker(...), server.listen(...), setupServer(...)) or parse the storybookConfig into an AST and check import declarations and CallExpression identifiers to reliably identify MSW usage.
356-367: Return typeanyweakens type safety.The
generateMockResponsefunction returnsany, which bypasses TypeScript's type checking. Consider defining a more specific return type or usingunknownwith type guards.♻️ Suggested improvement
-function generateMockResponse(call: ApiCallInfo): any { +function generateMockResponse(call: ApiCallInfo): Record<string, unknown> | Array<Record<string, unknown>> { // Generate appropriate mock based on URL patterns if (call.url.includes('user')) { return { id: 1, name: 'Test User', email: 'test@example.com' }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts` around lines 356 - 367, The generateMockResponse function currently returns any which disables type checking; update its signature to return a well-defined union type (e.g., User | Item[] | GenericResponse) or use unknown and narrow it with type guards before use, define interfaces for the shapes returned (e.g., interface User { id:number; name:string; email?:string }, interface Item { id:number; name:string }, interface GenericResponse { success:boolean; data: Record<string, unknown> }) and change generateMockResponse(call: ApiCallInfo): any to generateMockResponse(call: ApiCallInfo): User | Item[] | GenericResponse (or : unknown with corresponding isUser/isItem type guard functions) and update call sites to use those guards to satisfy TypeScript.packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts (2)
339-367:extractSelectormay miss selectors in deeply chained calls.The while loop walks up the chain but breaks immediately if the current node is not a
CallExpression. For chains likecy.get('.foo').find('.bar').click(), this should work, but the logic could be simplified and made more robust by continuing to traverse even when encountering non-call nodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts` around lines 339 - 367, extractSelector currently stops traversal whenever the node isn't a CallExpression, so it can miss selectors in deeper chains; update the traversal in extractSelector to keep walking up through PropertyAccessExpression and parent nodes instead of breaking: when encountering a CallExpression handle property access as now (checking expr.name and using extractFirstStringArg), but if the current node is not a CallExpression advance current to current.parent (or to the PropertyAccessExpression's expression when appropriate) and continue the loop until the root, so chains like cy.get(...).find(...).click() are fully inspected for selector args.
419-421: Arbitrary threshold for E2E classification.The heuristic
triggers.length > 3to classify as E2E is arbitrary and may misclassify component tests with multiple interactions. Consider documenting the rationale or making this configurable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts` around lines 419 - 421, The hardcoded heuristic "triggers.length > 3" in extractTestLogic should be made configurable and documented: replace the magic number with a configurable threshold (e.g., parameter or env var like e2eTriggerThreshold/TEST_E2E_TRIGGER_THRESHOLD) used by the function that computes classification (identify the triggers variable and the function extractTestLogic), provide a sensible default (retain 3) and add a short comment/JSDoc explaining the rationale and that projects can override it; update any callers to pass the new option or ensure the env fallback is read.test-toolkit-utils.ts (2)
61-66: Silent error swallowing may hide permission issues.Errors other than
ENOENTare logged as warnings but allow execution to continue. Permission errors (EACCES) or other I/O issues might indicate real problems that should surface more prominently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-toolkit-utils.ts` around lines 61 - 66, The catch block currently swallows non-ENOENT errors with a console.warn which can hide serious issues like EACCES; update the handler in the try/catch around reading directory `dir` so that if the caught `err` is not an ENOENT it is rethrown (or at least logged as an error and thrown) instead of only warned; specifically check `err instanceof Error` and if `!err.message.includes('ENOENT')` then log via console.error (including `err.message`) and throw the error so callers can handle permission or other I/O failures.
86-105: Unsafe type assertion inparseOptions.The function uses
as Tto cast the result, but there's no runtime validation that the parsed options match typeT. If callers expect specific keys, they may receiveundefinedvalues or wrong types, causing runtime errors.♻️ Suggested improvement with Zod validation
+import { z, ZodSchema } from 'zod'; -export function parseOptions<T extends Record<string, string | boolean>>( - startIndex: number, - allowedFlags: string[] = [] -): T { +export function parseOptions<T extends Record<string, string | boolean>>( + startIndex: number, + allowedFlags: string[] = [], + schema?: ZodSchema<T> +): T { const options: Record<string, string | boolean> = {}; process.argv.slice(startIndex).forEach(arg => { if (arg.includes('=')) { const [key, value] = arg.replace(/^--/, '').split('='); options[key] = value; } else if (arg.startsWith('--')) { const flag = arg.replace(/^--/, ''); if (allowedFlags.includes(flag)) { options[flag] = true; } } }); + if (schema) { + return schema.parse(options); + } return options as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-toolkit-utils.ts` around lines 86 - 105, The parseOptions function currently returns options cast unsafely with "as T" without runtime validation; change its signature to accept an optional runtime validator (e.g., a Zod schema or a validator function) such as parseOptions<T>(startIndex: number, allowedFlags: string[] = [], schema?: ZodSchema<T> | ((obj: unknown)=>obj)), build the parsed plain object exactly as now, then if a schema/validator is provided run validation and either throw or return the validated/parsed T (do not use "as T"), otherwise return the parsed object typed conservatively (e.g., Partial<Record<...>>). Ensure you reference the existing parseOptions function and remove the unsafe type assertion so callers get a validated runtime-typed result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@analyze-test-coverage.ts`:
- Around line 93-112: The top-level promise from generateAnalysisReport
currently ends with .catch(console.error) which logs errors but leaves process
exit code as zero; change the catch to explicitly set a non-zero exit code and
log the error (e.g., replace .catch(console.error) with .catch(err => {
console.error('Analysis failed:', err); process.exitCode = 1; }) so CI detects
failures; reference generateAnalysisReport and the existing
.catch(console.error) when making the change.
In `@migrate-cypress-tests.ts`:
- Around line 217-219: Update the misleading console output in
migrate-cypress-tests.ts: replace the hardcoded "The analysis has been saved to
TEST_COVERAGE_ANALYSIS.md" console.log with an accurate message (e.g., indicate
that analysis is available or that the migration completed and that
TEST_COVERAGE_ANALYSIS.md is created only by analyze-test-coverage.ts) so users
aren't misled into thinking migrate-cypress-tests.ts writes that file; locate
and edit the console.log calls shown in the diff to reflect the corrected
wording or remove the filename mention.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 111-118: The tool registration is passing
CheckMSWInputSchema.shape (the raw Zod shape) to inputSchema which prevents
proper JSON Schema serialization; change the registration to pass the full Zod
schema (inputSchema: CheckMSWInputSchema) or inline the schema object like other
tools do, ensuring the value assigned to inputSchema is a Zod schema object
(refer to CheckMSWInputSchema and the inputSchema property in the tool tuple).
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts`:
- Around line 392-430: The getMigrationRecommendation function contains a dead
'obsolete' case because categorizeTest only returns 'storybook' | 'unit' |
'e2e'; remove the unreachable 'obsolete' branch from getMigrationRecommendation
and any associated logic or string literals that reference it (or,
alternatively, if you intended to detect obsolete tests, add explicit detection
in categorizeTest to return 'obsolete'); specifically, either delete the
'obsolete' case block inside getMigrationRecommendation and adjust any
fallback/default handling accordingly, or implement the obsolete-detection logic
inside categorizeTest (and update its return type) to emit 'obsolete' so the
existing case becomes reachable.
In `@test-toolkit-utils.ts`:
- Around line 30-36: The extractTextContent function currently reads
result.content[0] without verifying the array has elements; update
extractTextContent (working with the CallToolResult type) to defensively check
that result.content exists and has length > 0 before accessing index 0, and if
empty or missing throw a clear Error (e.g., "Expected non-empty content array
from MCP tool"); also keep the existing type check for content.type === 'text'
and return content.text as before.
---
Nitpick comments:
In `@analyze-test-coverage.ts`:
- Line 65: Remove the stray space before the property access operator in the
string.repeat calls so they match the project's style; change occurrences like
"'=' .repeat(80)" to "'='.repeat(80)" (also fix the similar instance around the
other occurrence noted), locating them in analyze-test-coverage.ts where the '='
string's repeat is used.
In `@migrate-cypress-tests.ts`:
- Around line 35-39: The markdown parsing for auditText is fragile: update the
extraction logic (affecting isRelevant, recMatch/recommendation,
reasonMatch/reason) to use resilient patterns and normalization—first normalize
auditText (trim, collapse whitespace, make case-insensitive) then run single
robust regexes that accept optional bold markers (**) and flexible spacing or
fallback to parsing key:value pairs; alternatively have the tool return
structured JSON alongside markdown and prefer that structured payload when
present. Ensure the new patterns use non-greedy matches for reason, are
case-insensitive, and only fall back to 'MIGRATE'/'Unknown' after both
structured-data and resilient-regex attempts fail.
- Around line 129-141: The catch block that pushes to plans is inserting the
full error object into the reason field (in migrate-cypress-tests.ts, inside the
catch handling for each test), which can include stack traces; change the
assignment so reason uses only the error message string (e.g., derive a safe
string with error instanceof Error ? error.message : String(error)) before
pushing to plans (reference: plans array push, test.testPath/test.componentPath
and the reason property) so stack traces are not stored in the migration plan.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts`:
- Around line 311-322: The MSW detection in checkMSWSetup is brittle because it
uses simple substring includes; replace those naive checks with precise pattern
matching (e.g., anchored/word-boundary regexes) or an AST-based check to avoid
matching comments or identifiers like msw_unrelated; specifically, update
checkMSWSetup to detect real import/require statements and function calls (look
for patterns for import/from 'msw', require('msw'), mswDecorator as an
identifier, and calls like worker.start(...), setupWorker(...),
server.listen(...), setupServer(...)) or parse the storybookConfig into an AST
and check import declarations and CallExpression identifiers to reliably
identify MSW usage.
- Around line 356-367: The generateMockResponse function currently returns any
which disables type checking; update its signature to return a well-defined
union type (e.g., User | Item[] | GenericResponse) or use unknown and narrow it
with type guards before use, define interfaces for the shapes returned (e.g.,
interface User { id:number; name:string; email?:string }, interface Item {
id:number; name:string }, interface GenericResponse { success:boolean; data:
Record<string, unknown> }) and change generateMockResponse(call: ApiCallInfo):
any to generateMockResponse(call: ApiCallInfo): User | Item[] | GenericResponse
(or : unknown with corresponding isUser/isItem type guard functions) and update
call sites to use those guards to satisfy TypeScript.
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts`:
- Around line 339-367: extractSelector currently stops traversal whenever the
node isn't a CallExpression, so it can miss selectors in deeper chains; update
the traversal in extractSelector to keep walking up through
PropertyAccessExpression and parent nodes instead of breaking: when encountering
a CallExpression handle property access as now (checking expr.name and using
extractFirstStringArg), but if the current node is not a CallExpression advance
current to current.parent (or to the PropertyAccessExpression's expression when
appropriate) and continue the loop until the root, so chains like
cy.get(...).find(...).click() are fully inspected for selector args.
- Around line 419-421: The hardcoded heuristic "triggers.length > 3" in
extractTestLogic should be made configurable and documented: replace the magic
number with a configurable threshold (e.g., parameter or env var like
e2eTriggerThreshold/TEST_E2E_TRIGGER_THRESHOLD) used by the function that
computes classification (identify the triggers variable and the function
extractTestLogic), provide a sensible default (retain 3) and add a short
comment/JSDoc explaining the rationale and that projects can override it; update
any callers to pass the new option or ensure the env fallback is read.
In `@test-toolkit-utils.ts`:
- Around line 61-66: The catch block currently swallows non-ENOENT errors with a
console.warn which can hide serious issues like EACCES; update the handler in
the try/catch around reading directory `dir` so that if the caught `err` is not
an ENOENT it is rethrown (or at least logged as an error and thrown) instead of
only warned; specifically check `err instanceof Error` and if
`!err.message.includes('ENOENT')` then log via console.error (including
`err.message`) and throw the error so callers can handle permission or other I/O
failures.
- Around line 86-105: The parseOptions function currently returns options cast
unsafely with "as T" without runtime validation; change its signature to accept
an optional runtime validator (e.g., a Zod schema or a validator function) such
as parseOptions<T>(startIndex: number, allowedFlags: string[] = [], schema?:
ZodSchema<T> | ((obj: unknown)=>obj)), build the parsed plain object exactly as
now, then if a schema/validator is provided run validation and either throw or
return the validated/parsed T (do not use "as T"), otherwise return the parsed
object typed conservatively (e.g., Partial<Record<...>>). Ensure you reference
the existing parseOptions function and remove the unsafe type assertion so
callers get a validated runtime-typed result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 32d282c2-2ec1-4129-9ffc-6ac0bce5dc69
📒 Files selected for processing (6)
analyze-test-coverage.tsbuild-test-gaps.tsmigrate-cypress-tests.tspackages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.tspackages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tstest-toolkit-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- build-test-gaps.ts
Comment on lines
+93
to
+112
| generateAnalysisReport(repoPath) | ||
| .then(({ cypressTests }) => { | ||
| console.log('\n' + '='.repeat(80)); | ||
| console.log('\n🎯 NEXT STEPS - What would you like to do?\n'); | ||
|
|
||
| if (cypressTests.length > 0) { | ||
| console.log(`1️⃣ Migrate Existing Cypress Tests (${cypressTests.length} found)`); | ||
| console.log(' → Convert legacy Cypress tests to modern Playwright/Storybook'); | ||
| console.log(' → Uses hcc-frontend-cypress-migration-specialist agent\n'); | ||
| } | ||
|
|
||
| console.log('2️⃣ Build Tests for Coverage Gaps'); | ||
| console.log(' → Generate new tests for components missing coverage'); | ||
| console.log(' → Focus on high-priority components'); | ||
| console.log(' → Uses test generation agents (Storybook, Jest, Playwright)\n'); | ||
|
|
||
| console.log('📖 Review the full report: TEST_COVERAGE_ANALYSIS.md'); | ||
| console.log('=' .repeat(80) + '\n'); | ||
| }) | ||
| .catch(console.error); |
There was a problem hiding this comment.
Top-level promise rejection may exit without cleanup.
The .catch(console.error) handler logs errors but doesn't set a non-zero exit code. Failed executions will appear successful to CI/CD pipelines.
🐛 Proposed fix to set exit code on failure
generateAnalysisReport(repoPath)
.then(({ cypressTests }) => {
// ... existing code ...
})
- .catch(console.error);
+ .catch((error) => {
+ console.error(error);
+ process.exit(1);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@analyze-test-coverage.ts` around lines 93 - 112, The top-level promise from
generateAnalysisReport currently ends with .catch(console.error) which logs
errors but leaves process exit code as zero; change the catch to explicitly set
a non-zero exit code and log the error (e.g., replace .catch(console.error) with
.catch(err => { console.error('Analysis failed:', err); process.exitCode = 1; })
so CI detects failures; reference generateAnalysisReport and the existing
.catch(console.error) when making the change.
Comment on lines
+217
to
+219
|
|
||
| console.log('💡 TIP: The analysis has been saved to TEST_COVERAGE_ANALYSIS.md'); | ||
| console.log('=' .repeat(80) + '\n'); |
There was a problem hiding this comment.
Misleading tip about saved analysis.
Line 218 states "The analysis has been saved to TEST_COVERAGE_ANALYSIS.md", but migrate-cypress-tests.ts does not write this file—it's written by analyze-test-coverage.ts. This could confuse users who only ran the migration script.
✏️ Suggested fix
- console.log('💡 TIP: The analysis has been saved to TEST_COVERAGE_ANALYSIS.md');
+ console.log('💡 TIP: Run analyze-test-coverage.ts first to generate TEST_COVERAGE_ANALYSIS.md');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('💡 TIP: The analysis has been saved to TEST_COVERAGE_ANALYSIS.md'); | |
| console.log('=' .repeat(80) + '\n'); | |
| console.log('💡 TIP: Run analyze-test-coverage.ts first to generate TEST_COVERAGE_ANALYSIS.md'); | |
| console.log('=' .repeat(80) + '\n'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrate-cypress-tests.ts` around lines 217 - 219, Update the misleading
console output in migrate-cypress-tests.ts: replace the hardcoded "The analysis
has been saved to TEST_COVERAGE_ANALYSIS.md" console.log with an accurate
message (e.g., indicate that analysis is available or that the migration
completed and that TEST_COVERAGE_ANALYSIS.md is created only by
analyze-test-coverage.ts) so users aren't misled into thinking
migrate-cypress-tests.ts writes that file; locate and edit the console.log calls
shown in the diff to reflect the corrected wording or remove the filename
mention.
Comment on lines
+111
to
+118
| return [ | ||
| 'check_msw_readiness', | ||
| { | ||
| description: 'Scans component files for API calls (fetch, axios, cy.intercept) and checks if Mock Service Worker (MSW) is configured for Storybook. Provides MSW handler recommendations.', | ||
| inputSchema: CheckMSWInputSchema.shape, | ||
| }, | ||
| tool | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @modelcontextprotocol/sdk support Zod schema shapes directly for inputSchema?
💡 Result:
Yes, @modelcontextprotocol/sdk supports Zod schema shapes directly for inputSchema. In v1.x versions, it accepts ZodRawShape objects (e.g., z.object({ name: z.string })) which are converted to JSON Schema. Recent versions (including latest as of 2026) and v2 use Zod v4 schemas natively via z.toJSONSchema, with examples like server.registerTool('greet', { inputSchema: z.object({ name: z.string }) }). Zod v4 works unchanged as it implements the required StandardSchemaWithJSON interface for validation and JSON Schema generation.
Citations:
- 1: Zod 4 supported modelcontextprotocol/typescript-sdk#555
- 2: feat: support Standard Schema for tool/prompt schemas modelcontextprotocol/typescript-sdk#1689
- 3: https://www.npmjs.com/package/%40modelcontextprotocol/sdk
- 4: https://github.com/modelcontextprotocol/typescript-sdk
- 5: https://github.com/modelcontextprotocol/typescript-sdk/tree/main
- 6: https://tessl.io/registry/tessl/npm-modelcontextprotocol--sdk/1.20.0/files/docs/server-tools.md
- 7: https://ts.sdk.modelcontextprotocol.io/v2/types/_modelcontextprotocol_server.server_mcp.RegisteredTool.html
- 8: Tool arguments are not passed into registered tool handlers modelcontextprotocol/typescript-sdk#1026
🏁 Script executed:
# First, find and examine the CheckMSWInputSchema definition
fd -t f "checkMSWReadiness\|CheckMSW" --extension ts --extension tsx | head -20Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 69
🏁 Script executed:
# Search for the schema definition in the file
rg "CheckMSWInputSchema" -A 5 -B 5 packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 1159
🏁 Script executed:
# Look for other MCP tool definitions in the codebase to see the pattern
rg "inputSchema:" -B 3 -A 3 --type ts --type tsx | head -100Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 115
🏁 Script executed:
# Search for other MCP tool registrations with inputSchema to see the pattern used
rg "inputSchema" packages/hcc-test-migration-mcp -A 2 -B 2 --type tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 2985
🏁 Script executed:
# Check the MCP SDK types to understand what inputSchema expects
rg "RegisteredTool\|inputSchema" node_modules/@modelcontextprotocol/sdk --type ts -A 3 | head -150Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 166
🏁 Script executed:
# Look at package.json to find MCP SDK version
cat packages/hcc-test-migration-mcp/package.json | rg "@modelcontextprotocol|zod" -A 1 -B 1Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 224
🏁 Script executed:
# Check the MCP types in the local SDK to understand the expected inputSchema format
find . -name "*.d.ts" -o -name "*.ts" | xargs grep -l "RegisteredTool\|inputSchema" 2>/dev/null | grep -i mcp | head -5Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 401
🏁 Script executed:
# Look for MCP SDK type definitions in the repo
rg "type McpTool\|interface.*Tool" packages/hcc-test-migration-mcp/src -A 3 -B 1Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 69
🏁 Script executed:
# Check if there are any imports or type hints about what MCP expects
rg "from.*@modelcontextprotocol" packages/hcc-test-migration-mcp/src -B 2 -A 2Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 2317
🏁 Script executed:
# Look at the actual MCP registration to see if types are enforced
rg "server\.\|register" packages/hcc-test-migration-mcp/src --type ts | head -20Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 69
🏁 Script executed:
# Check if there are any build/type checking errors or runtime tests
rg "CheckMSWInputSchema" packages/hcc-test-migration-mcp -B 5 -A 10 --type tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 4208
🏁 Script executed:
# Look for MCP server registration to see how tools are used
cat packages/hcc-test-migration-mcp/src/lib/index.ts | head -100Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 2723
🏁 Script executed:
# Check if the codebase compiles without TypeScript errors (strict mode check)
cat packages/hcc-test-migration-mcp/tsconfig.json 2>/dev/null || echo "No tsconfig found"Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 661
🏁 Script executed:
# Search for any error handling or validation related to inputSchema in MCP usage
rg "inputSchema|registerTool" packages/hcc-test-migration-mcp/src/lib/index.ts -B 5 -A 10Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 487
🏁 Script executed:
# Check if there's a specific pattern in the FEO MCP tools (another MCP package) for comparison
rg "inputSchema:" packages/hcc-feo-mcp/src -A 2 -B 2 --type ts 2>/dev/null | head -40Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 3663
Remove .shape from inputSchema—pass the full Zod schema instead.
Using CheckMSWInputSchema.shape extracts only the raw Zod shape object, which will not serialize to JSON Schema correctly. The MCP SDK expects a full Zod schema object that can be converted to JSON Schema. Either pass the schema directly (inputSchema: CheckMSWInputSchema) or inline it as other tools in this codebase do (see packages/hcc-feo-mcp/src/lib/tools/validateFEOConfig.ts for the correct pattern: inputSchema: { field: z.string().describe(...) }).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/hcc-test-migration-mcp/src/lib/tools/checkMSWReadiness.ts` around
lines 111 - 118, The tool registration is passing CheckMSWInputSchema.shape (the
raw Zod shape) to inputSchema which prevents proper JSON Schema serialization;
change the registration to pass the full Zod schema (inputSchema:
CheckMSWInputSchema) or inline the schema object like other tools do, ensuring
the value assigned to inputSchema is a Zod schema object (refer to
CheckMSWInputSchema and the inputSchema property in the tool tuple).
Comment on lines
+392
to
+430
| function categorizeTest( | ||
| setup: SetupAction[], | ||
| triggers: TriggerAction[], | ||
| assertions: AssertionAction[], | ||
| testCode: string | ||
| ): 'storybook' | 'unit' | 'e2e' { | ||
| // CRITICAL: Check for cy.mount() - definitive indicator of component test | ||
| if (testCode.includes('cy.mount(')) { | ||
| return 'storybook'; | ||
| } | ||
|
|
||
| // If has visit, definitely E2E | ||
| if (setup.some(s => s.type === 'visit')) { | ||
| return 'e2e'; | ||
| } | ||
|
|
||
| // If only assertions, likely unit test | ||
| if (triggers.length === 0 && assertions.length > 0) { | ||
| return 'unit'; | ||
| } | ||
|
|
||
| // If has intercepts and triggers, likely component test (Storybook) | ||
| if (setup.some(s => s.type === 'intercept') && triggers.length > 0) { | ||
| return 'storybook'; | ||
| } | ||
|
|
||
| // Multiple triggers without mount or visit, likely E2E | ||
| if (triggers.length > 3) { | ||
| return 'e2e'; | ||
| } | ||
|
|
||
| // Simple interactions, likely Storybook | ||
| if (triggers.length > 0) { | ||
| return 'storybook'; | ||
| } | ||
|
|
||
| // Default to E2E if unclear | ||
| return 'e2e'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'obsolete' category is used elsewhere in the codebase
rg -n "category.*obsolete|obsolete.*category" --type tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 69
🏁 Script executed:
#!/bin/bash
# First, let's find and read the extractTestLogic.ts file
find . -name "extractTestLogic.ts" -type fRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Read the file to understand line count and check for 'obsolete' references
wc -l ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts
# Search for any mention of 'obsolete' in the file
echo "=== Searching for 'obsolete' in the file ==="
rg -n "obsolete" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts
# Search for getMigrationRecommendation function
echo "=== Searching for getMigrationRecommendation ==="
rg -n "getMigrationRecommendation" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Read the categorizeTest function (lines 392-430)
echo "=== categorizeTest function (lines 392-430) ==="
sed -n '392,430p' ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts
echo ""
echo "=== getMigrationRecommendation function (around lines 494-543) ==="
sed -n '494,543p' ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 2904
🏁 Script executed:
#!/bin/bash
# Look for where testMap.category is set or where TestLogicMap is defined
echo "=== Searching for TestLogicMap definition ==="
rg -n "type TestLogicMap|interface TestLogicMap" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts
echo ""
echo "=== Searching for where category is assigned ==="
rg -n "\.category\s*=" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts
echo ""
echo "=== Searching for audit_test_coverage_and_relevance references ==="
rg -n "audit_test_coverage_and_relevance" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 346
🏁 Script executed:
#!/bin/bash
# Search for TestLogicMap across the entire repository
echo "=== Searching for TestLogicMap definition across repo ==="
rg -n "type TestLogicMap|interface TestLogicMap" --type ts
echo ""
echo "=== Searching for where 'obsolete' category might be assigned ==="
rg -n "category.*=.*['\"]obsolete['\"]" --type ts
echo ""
echo "=== Look at how categorizeTest result is used in extractTestLogic.ts ==="
rg -n "categorizeTest\(" ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts -A 3Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 615
🏁 Script executed:
#!/bin/bash
# Read the TestLogicMap interface definition
echo "=== TestLogicMap interface definition ==="
rg -n "TestLogicMap" ./packages/hcc-test-migration-mcp/src/lib/types.ts -A 10 -B 2
echo ""
echo "=== Check the full context around line 133-140 where categorizeTest is called ==="
sed -n '130,145p' ./packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.tsRepository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Find the TestCategory type definition
echo "=== TestCategory type definition ==="
rg -n "type TestCategory|TestCategory\s*=" ./packages/hcc-test-migration-mcp/src/lib/types.ts -A 3 -B 2
echo ""
echo "=== Look for all type definitions in types.ts ==="
rg -n "^(type|interface|export)" ./packages/hcc-test-migration-mcp/src/lib/types.ts | head -20Repository: RedHatInsights/platform-frontend-ai-toolkit
Length of output: 1032
Remove dead code path for 'obsolete' case in getMigrationRecommendation.
The categorizeTest function (lines 392-430) has a return type of 'storybook' | 'unit' | 'e2e' and cannot return 'obsolete'. However, getMigrationRecommendation (lines 530-536) includes a case handler for 'obsolete' that will never execute. Either remove the 'obsolete' case from getMigrationRecommendation or add logic to categorizeTest to detect and return 'obsolete' patterns (if that was the original intent for audit_test_coverage_and_relevance integration).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/hcc-test-migration-mcp/src/lib/tools/extractTestLogic.ts` around
lines 392 - 430, The getMigrationRecommendation function contains a dead
'obsolete' case because categorizeTest only returns 'storybook' | 'unit' |
'e2e'; remove the unreachable 'obsolete' branch from getMigrationRecommendation
and any associated logic or string literals that reference it (or,
alternatively, if you intended to detect obsolete tests, add explicit detection
in categorizeTest to return 'obsolete'); specifically, either delete the
'obsolete' case block inside getMigrationRecommendation and adjust any
fallback/default handling accordingly, or implement the obsolete-detection logic
inside categorizeTest (and update its return type) to emit 'obsolete' so the
existing case becomes reachable.
Comment on lines
+30
to
+36
| export function extractTextContent(result: CallToolResult): string { | ||
| const content = result.content[0]; | ||
| if (!content || content.type !== 'text') { | ||
| throw new Error('Expected text content from MCP tool'); | ||
| } | ||
| return content.text; | ||
| } |
There was a problem hiding this comment.
extractTextContent assumes content array is non-empty.
Accessing result.content[0] without checking array length could throw if content is empty. Consider adding a bounds check.
🛡️ Proposed defensive fix
export function extractTextContent(result: CallToolResult): string {
+ if (!result.content || result.content.length === 0) {
+ throw new Error('Expected content from MCP tool but received empty array');
+ }
const content = result.content[0];
if (!content || content.type !== 'text') {
throw new Error('Expected text content from MCP tool');
}
return content.text;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function extractTextContent(result: CallToolResult): string { | |
| const content = result.content[0]; | |
| if (!content || content.type !== 'text') { | |
| throw new Error('Expected text content from MCP tool'); | |
| } | |
| return content.text; | |
| } | |
| export function extractTextContent(result: CallToolResult): string { | |
| if (!result.content || result.content.length === 0) { | |
| throw new Error('Expected content from MCP tool but received empty array'); | |
| } | |
| const content = result.content[0]; | |
| if (!content || content.type !== 'text') { | |
| throw new Error('Expected text content from MCP tool'); | |
| } | |
| return content.text; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-toolkit-utils.ts` around lines 30 - 36, The extractTextContent function
currently reads result.content[0] without verifying the array has elements;
update extractTextContent (working with the CallToolResult type) to defensively
check that result.content exists and has length > 0 before accessing index 0,
and if empty or missing throw a clear Error (e.g., "Expected non-empty content
array from MCP tool"); also keep the existing type check for content.type ===
'text' and return content.text as before.
- Fixed regex to match newline before ## to avoid premature cutoff - Updated parseGapLines to match actual MCP output format: - **ComponentName**: Missing jest, storybook - Removed debug logging after verifying fix - Tested against notifications-frontend with 81 high-priority gaps Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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
Adds a repository-agnostic toolkit for analyzing test coverage and building tests using our standard stack (Storybook, Jest,
Playwright). Also includes tooling to migrate legacy Cypress tests if needed.
What's New
MCP Server: hcc-test-migration-mcp
4 tools for test analysis:
Orchestration Scripts (3)
General Testing (Primary):
Cypress Migration (Secondary):
Shared Utilities
Usage
Analyze any repository's test coverage
tsx analyze-test-coverage.ts /path/to/repo
Build tests for coverage gaps
tsx build-test-gaps.ts /path/to/repo --priority=high --type=storybook
(Optional) Migrate legacy Cypress tests if present
tsx migrate-cypress-tests.ts /path/to/repo
Key Features