ci: add VS Code extension publish workflow

[himerus]

Summary

• Adds .github/workflows/publish-vscode.yml triggered on vscode-v* tags
• Single job: builds helixir, bundles extension, publishes to VS Code Marketplace and Open VSX
• References VSCE_PAT and OVSX_PAT secrets
• Includes gitleaks secret scanning matching existing CI pattern

Test plan

• Tag a release with vscode-v* to verify the workflow triggers
• Confirm VSCE_PAT and OVSX_PAT secrets are set in repository settings before first publish

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Released Helixir VS Code Extension for integrated component intelligence and MCP server registration.
  • Expanded MCP tool library from 30+ to 87+, including new Scaffold, Extend, Theme, and comprehensive Styling validation tools.

• Documentation

  • Added VS Code Extension documentation with setup and configuration guidance.
  • Enhanced main README with detailed MCP tool reference and social card.

• Improvements

  • Standardized error handling across handlers with proper error categorization.
  • Improved theme generation with CSS variable fallbacks for unknown token categories.

• Chores

  • Updated MCP SDK dependencies and added automated extension publishing workflow.

[coderabbitai]

Walkthrough

The PR introduces a VS Code extension for Helixir with MCP server support, updates documentation to reflect 87+ available tools, refactors error handling in core handlers to use typed MCPError instances, expands tool registrations for scaffold/extend/theme features, adds comprehensive test coverage for analyzers and tool dispatchers, and updates dependencies and configuration to prefer helixir.mcp.json over legacy mcpwc.config.json.

Changes

Cohort / File(s)	Summary
VS Code Extension Infrastructure packages/vscode/package.json, packages/vscode/esbuild.config.mjs, packages/vscode/tsconfig.json, packages/vscode/.vscodeignore, packages/vscode/README.md	Added extension manifest with MCP server definition provider registration, build configuration for dual-output bundling (extension host + MCP server), TypeScript config for ES2022 target, packaging exclusions, and comprehensive documentation of extension capabilities and setup requirements.
VS Code Extension Runtime packages/vscode/src/extension.ts, packages/vscode/src/mcp-server-entry.ts, packages/vscode/src/mcpProvider.ts, packages/vscode/src/commands/configureCursorWindsurf.ts	Added extension entrypoint with MCP provider registration and health check command, MCP server entry point with error handling, environment variable setup for workspace root and config paths, and Cursor/Windsurf configuration writer for mcp.json.
Publishing Pipeline .github/workflows/publish-vscode.yml	Added GitHub Actions workflow triggered on vscode-v* tags that performs gitleaks scanning, dependency installation, project build, and publishes to VS Code Marketplace and Open VSX using vsce/ovsx with authentication tokens.
Handler Error Handling packages/core/src/handlers/cem.ts, packages/core/src/handlers/dependencies.ts, packages/core/src/handlers/extend.ts	Replaced generic Error throws with typed MCPError instances using appropriate ErrorCategory values (VALIDATION for invalid tokens, NOT_FOUND for missing components/parents).
Theme Handler & Config Refactoring packages/core/src/handlers/theme.ts, packages/core/src/config.ts	Updated light-mode placeholder for unknown token categories to return CSS var() references instead of TODO comments; removed legacy mcpwc.config.json fallback and refactored environment-variable overrides using mapping-based loops, added support for MCP_WC_CDN_STYLESHEET via nullable overrides.
MCP Tool Integration src/mcp/index.ts, src/cli/index.ts	Registered scaffold and extend tools in tool definitions, added dispatch branches for scaffold/extend/theme tools with CEM cache validation, updated error reporting to use handleToolError() for consistent message formatting across CLI and startup logs.
Dependency Updates package.json, packages/core/package.json, packages/mcp/package.json	Added typecheck script alias, bumped @modelcontextprotocol/sdk from ^1.26.0 to ^1.27.1 in core and mcp packages, raised flatted override to >=3.4.2, added picomatch override >=4.0.4.
Documentation Updates README.md	Added social-card image, updated tool count from 30+ to 87+, added new tool documentation sections (TypeGenerate, Theme, Scaffold, Extend, Styling with 30+ validation checkers), updated Health tools table with audit_library entry and comprehensive styling validator tooling reference.
Analyzer Test Coverage tests/handlers/analyzers/api-surface.test.ts, tests/handlers/analyzers/css-architecture.test.ts, tests/handlers/analyzers/event-architecture.test.ts, tests/handlers/analyzers/mixin-resolver.test.ts, tests/handlers/analyzers/naming-consistency.test.ts, tests/handlers/analyzers/slot-architecture.test.ts, tests/handlers/analyzers/source-accessibility.test.ts, tests/handlers/analyzers/type-coverage.test.ts	Added comprehensive Vitest suites covering scoring logic, edge cases, result shape validation, and scoring invariants for all analyzer functions including fixture-based scoring assertions and confidence categorization.
Tool Dispatcher Test Coverage tests/tools/bundle.test.ts, tests/tools/extend.test.ts, tests/tools/scaffold.test.ts, tests/tools/styling.test.ts, tests/tools/theme.test.ts, tests/tools/cdn.test.ts, tests/tools/composition.test.ts, tests/tools/framework.test.ts, tests/tools/story.test.ts, tests/tools/tokens.test.ts, tests/tools/typegenerate.test.ts, tests/tools/typescript.test.ts, tests/tools/validate.test.ts	Added Vitest suites for all tool dispatchers validating tool name matching, input validation, successful dispatch with expected output formats, error handling for missing/invalid arguments, and error propagation from underlying handlers.
Handler Test Updates tests/handlers/theme.test.ts	Added test case for unknown token categories producing CSS var() references in light mode placeholder fallback path.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• #191: Adds scaffold_component MCP tool implementation and handler with overlapping edits to packages/core handlers and tool wiring.
• #192: Modifies theme tool handlers and dispatch logic, directly affecting theme handler behavior and MCP registration overlap.
• #194: Implements extend-component feature with shared modifications to packages/core/src/handlers/extend.ts and tool wiring/tests.

Suggested labels

skip-changeset

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is incomplete. It provides a Summary and Test plan but does not follow the template structure with sections like 'Type of change', 'Related issue', 'Tests added', or 'Checklist'.	Fill in all required template sections: select a Type of change, add Related issue reference, clarify whether tests were added, and complete the Checklist items (sign-off, type-check, lint, format, test, build, CHANGELOG entry).
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'ci: add VS Code extension publish workflow' is directly related to the main change—adding a new GitHub Actions workflow for publishing the VS Code extension.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/feat-add-vs-code-extension-publish-ci

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)

49-53: ⚠️ Potential issue | 🟠 Major

Config filename mismatch with CLI init output.

Line 49 and Line 444 document mcpwc.config.json, but runInit() writes helixir.mcp.json (src/cli/index.ts, Line 758-760). This will mislead first-time setup.

📝 Proposed doc fix

-# → writes mcpwc.config.json to the current directory
+# → writes helixir.mcp.json to the current directory
 ...
-Edit `mcpwc.config.json` to point at your library:
+Edit `helixir.mcp.json` to point at your library:
 ...
-### `mcpwc.config.json`
+### `helixir.mcp.json`

Also applies to: 442-447

🤖 Prompt for AI Agents

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

In `@README.md` around lines 49 - 53, The README references mcpwc.config.json
while the CLI's runInit() actually writes helixir.mcp.json, causing a mismatch;
fix by making the two consistent: either update the README occurrences of
mcpwc.config.json to helixir.mcp.json (preferred) so docs match the CLI
behavior, or change runInit() to write mcpwc.config.json instead—locate the
runInit() implementation and the README entries and apply the same filename in
both places, updating any example commands and explanatory text accordingly.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In @.github/workflows/publish-vscode.yml:
- Around line 46-52: The publish steps "Publish to VS Code Marketplace" and
"Publish to Open VSX" run from the repo root but need the extension manifest in
packages/vscode; update both steps (the actions named Publish to VS Code
Marketplace and Publish to Open VSX) to set working-directory: packages/vscode
so npx `@vscode/vsce` and npx ovsx run against packages/vscode/package.json
instead of the root.

In `@packages/vscode/README.md`:
- Line 55: The README contains an outdated hardcoded phrase "30+ tools" (the
same phrase appears elsewhere) — update that literal text to match the current
actual tool count or replace it with a non-numeric phrase like "dozens of tools"
or "many tools" to avoid drift; locate the string "30+ tools" in the README and
edit the sentence to either reflect the accurate number or use a generic
descriptor so both occurrences remain consistent.
- Line 10: Update the hardcoded tool-count text "**30+ MCP tools**" in the
README to reflect the current documented total (replace with "73 MCP tools" or a
dynamic phrasing like "73+ MCP tools" / "70+ MCP tools" to match the main
README); locate the exact string "**30+ MCP tools**" in the README and change it
so the count aligns with the main README's documented 73 tools.

In `@packages/vscode/src/mcpProvider.ts`:
- Line 21: The current assignment to workspaceFolder in mcpProvider.ts uses a
fallback to process.cwd(), which can point to an unexpected directory; change
the logic in the code that computes workspaceFolder (the const workspaceFolder =
... line) to detect when vscode.workspace.workspaceFolders is empty and handle
that case explicitly: either log a warning via the extension logger/console and
return an empty array from the surrounding function (so the MCP server doesn't
try to read custom-elements.json), or prompt/notify the user to open a workspace
before proceeding; ensure you reference and update the code paths that use
workspaceFolder so they skip file reads when no workspace is available.

In `@README.md`:
- Around line 398-400: Update the README table entry for the extend_component
tool to reflect the actual schema: replace the single Required Arg `tagName`
with both required arguments `parentTagName` and `newTagName`, and optionally
mention the optional `newClassName` parameter; ensure the table row for
`extend_component` (tool name) and its Description remain unchanged while only
adjusting the Required Args column to list `parentTagName, newTagName` (and
optionally `newClassName`).

In `@tests/tools/bundle.test.ts`:
- Around line 242-250: The test currently only checks result.isError is false
but must also assert that unknownProp was removed before the handler ran; update
the test using the mock/spy for the 'estimate_bundle_size' handler (the same
mock used by handleBundleCall) to assert it was invoked with an argument object
containing only the expected property (e.g., { tagName: 'sl-button' }) and not
containing unknownProp when calling handleBundleCall('estimate_bundle_size', {
tagName: 'sl-button', unknownProp: 'ignored' }, FAKE_CONFIG). Ensure you
reference the mocked handler/spy used in the test harness (the mock for the
estimate_bundle_size handler) and add a Jest assertion that checks the last call
payload does not include unknownProp and matches the expected sanitized object.

In `@tests/tools/extend.test.ts`:
- Around line 219-232: The test mixes an async/await dynamic import with a
synchronous call to handleExtendCall; make it consistent by removing the dynamic
await import and importing extendComponent the same way as other tests (so the
test is synchronous), i.e., import extendComponent at module scope and delete
the test's async/await around import and the async keyword on the test; keep the
mocked implementation for extendComponent and call handleExtendCall
synchronously as currently done.

In `@tests/tools/theme.test.ts`:
- Around line 122-129: The CreateThemeArgsSchema currently permits unknown
properties because it's defined as z.object({...}) without strict mode, causing
the test in tests/tools/theme.test.ts that calls handleThemeCall('create_theme',
...) with an extra unknownProp to unexpectedly succeed; fix by updating the
production schema CreateThemeArgsSchema to use .strict() (e.g.,
CreateThemeArgsSchema.strict()) so Zod rejects extra properties, or
alternatively, if extra properties are intended, update the failing test to
remove the strict-validation assertion and adjust expectations for
handleThemeCall accordingly; reference CreateThemeArgsSchema and handleThemeCall
to locate the change.

---

Outside diff comments:
In `@README.md`:
- Around line 49-53: The README references mcpwc.config.json while the CLI's
runInit() actually writes helixir.mcp.json, causing a mismatch; fix by making
the two consistent: either update the README occurrences of mcpwc.config.json to
helixir.mcp.json (preferred) so docs match the CLI behavior, or change runInit()
to write mcpwc.config.json instead—locate the runInit() implementation and the
README entries and apply the same filename in both places, updating any example
commands and explanatory text accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e2fa133d-2cd9-4274-9583-bc92f80cf1f3

📥 Commits

Reviewing files that changed from the base of the PR and between 35b76a7 and 4db9125.

⛔ Files ignored due to path filters (2)

• assets/social-card.png is excluded by !**/*.png
• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (26)

• .github/workflows/publish-vscode.yml
• README.md
• assets/social-card.webp
• package.json
• packages/core/package.json
• packages/core/src/handlers/cem.ts
• packages/core/src/handlers/dependencies.ts
• packages/core/src/handlers/extend.ts
• packages/core/src/handlers/theme.ts
• packages/mcp/package.json
• packages/vscode/.vscodeignore
• packages/vscode/README.md
• packages/vscode/esbuild.config.mjs
• packages/vscode/package.json
• packages/vscode/src/extension.ts
• packages/vscode/src/mcp-server-entry.ts
• packages/vscode/src/mcpProvider.ts
• packages/vscode/tsconfig.json
• src/cli/index.ts
• src/mcp/index.ts
• tests/handlers/theme.test.ts
• tests/tools/bundle.test.ts
• tests/tools/extend.test.ts
• tests/tools/scaffold.test.ts
• tests/tools/styling.test.ts
• tests/tools/theme.test.ts

[coderabbitai]

Actionable comments posted: 22

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@packages/vscode/src/commands/configureCursorWindsurf.ts`:
- Around line 75-78: The catch block swallowing JSON parse errors should capture
and log the error so users know why mcp.json was reset: change the empty catch
to catch (err) and call a warning logger (e.g., console.warn or the extension
logger) including the filename ("mcp.json") and err.message/stack, then continue
to set existing = { mcpServers: {} } as before; if you replace console.warn with
an async logger, ensure the inner try-catch remains compatible (await where
needed) so logging doesn't break error handling in the configureCursorWindsurf
flow.
- Around line 81-86: The mcp entry created in configureCursorWindsurf (the
existing.mcpServers['helixir'] upsert) leaves env empty so MCP_WC_PROJECT_ROOT
is missing; update that env object to include MCP_WC_PROJECT_ROOT set to the
same workspace root path used by mcpProvider (the workspace folder used when
creating the server), and add a short comment explaining users should update
MCP_WC_PROJECT_ROOT if they move the config or work across multiple projects.

In `@packages/vscode/src/extension.ts`:
- Around line 5-9: Update the top docstring for the extension activation to
mention the missing command: include the "Helixir: Configure Cursor Windsurf"
(configureCursorWindsurf) registration alongside the existing Helixir MCP server
definition provider and "Helixir: Run Health Check" command; locate the
docstring near the activate function in extension.ts and add a brief phrase
describing that configureCursorWindsurf is registered so the comment accurately
reflects all commands being registered.

In `@README.md`:
- Line 20: The README has an inconsistent tool count: the badge text "tools-73"
in the badge markup and the written claim "87+ MCP tools" differ; pick the
correct canonical count and make both places match by updating either the badge
label (the "tools-73" segment in the badge markdown) or the written text "87+
MCP tools" so they show the same number, and ensure any other occurrences of the
tool count in the README are updated consistently.

In `@tests/handlers/analyzers/api-surface.test.ts`:
- Around line 325-339: The metric currently uses Math.max(...) which undercounts
mixed cases; in analyzeApiSurface update the "Attribute reflection" calculation
(in the logic that builds attrMetric) to count members that have reflects ===
true OR a defined attribute (i.e., compute the union of members satisfying
reflects or attribute) instead of taking Math.max; ensure you deduplicate by
member name so a single member with both reflects and attribute is only counted
once and then set attrMetric.score to that union count (capped by
attrMetric.maxScore).

In `@tests/handlers/analyzers/mixin-resolver.test.ts`:
- Around line 17-24: The test file imports two type symbols ResolvedSource and
InheritanceChainResult that ESLint reports as unused; either remove them or mark
them as intentionally unused by prefixing the type names with an underscore
(e.g., _ResolvedSource, _InheritanceChainResult) in the import. Update the
import statement in mixin-resolver.test.ts to drop unused types or rename them
with the _ prefix so the linter recognizes them as intentionally unused while
leaving the other imports (resolveInheritanceChain, CemDeclaration) unchanged.
- Around line 72-82: The test file contains an unused fixture constant
MIXIN_IMPORT_SOURCE; remove the dead code by deleting the const declaration
named MIXIN_IMPORT_SOURCE (and any surrounding unused comment block) from
tests/handlers/analyzers/mixin-resolver.test.ts, or alternatively add one or
more tests that reference MIXIN_IMPORT_SOURCE to exercise mixin import
resolution (e.g., a test that feeds MIXIN_IMPORT_SOURCE into the analyzer and
asserts expected resolved mixins); choose removal if you don't intend to add new
tests.
- Around line 28-29: The test uses a hardcoded WORKTREE constant pointing to an
absolute path; replace it with a dynamically derived PROJECT_ROOT variable
(e.g., compute from __dirname or process.cwd() with path.resolve) and update all
usages of WORKTREE in this test to use PROJECT_ROOT instead; ensure the new
PROJECT_ROOT computation mirrors the portable approach used in
source-accessibility.test.ts so all relative path joins in tests remain valid
and platform-independent.

In `@tests/handlers/analyzers/slot-architecture.test.ts`:
- Around line 278-284: The test currently only asserts presence of the 'icon'
slot but not that jsdoc-derived type info is applied; update the assertion in
the test that calls analyzeSlotArchitecture(JSDOC_SLOT_DECL) so it checks the
slot's type constraint (e.g., assert iconSlot?.typeConstraint is defined or
equals/contains the expected 'svg' constraint) instead of only
expect(iconSlot).toBeDefined(); locate the iconSlot variable in this test and
assert its typeConstraint property directly to validate jsdoc `@slot` parsing.

In `@tests/handlers/analyzers/source-accessibility.test.ts`:
- Line 24: Remove the unused named import "resolve" from the import statement
that imports from "node:path" in this test file; locate the import line that
reads "import { resolve } from 'node:path';" (or a combined import including
resolve) and delete "resolve" (or remove the entire import if nothing else is
imported) so the file no longer includes an unused symbol.
- Around line 203-211: Two tests duplicate the same assertion using the same
fixture: the it blocks named 'detects screenReaderSupport from aria-labelledby
and aria-describedby' and 'detects screenReaderSupport from .sr-only class' both
call scanSourceForA11yPatterns(SCREEN_READER_SOURCE) and assert
markers.screenReaderSupport. Remove the redundancy by either merging into one
test that describes both patterns or create a new fixture (e.g.,
SCREEN_READER_SR_ONLY) and change the second test to use that fixture; update
the test descriptions accordingly and keep calls to scanSourceForA11yPatterns
and the assertion on markers.screenReaderSupport as-is.
- Around line 449-476: The test hardcodes WORKTREE to a developer-specific
absolute path and conditionally asserts only if a file exists, causing CI
flakiness; replace the WORKTREE constant with a portable test fixture or a
temporary project path resolved from the test directory (e.g., use
path.resolve(__dirname, 'fixtures', 'some-project') or create a tmp dir with
known files) so resolveComponentSourceFilePath is run against deterministic
files, and update the ".ts equivalent for .js path" test to assert
deterministically (explicitly expect a non-null result and that it
endsWith('.ts') || endsWith('.js') for the known fixture file, or assert null
when testing non-existence) rather than only asserting conditionally. Ensure
references to resolveComponentSourceFilePath and the WORKTREE replacement are
updated in the test file.
- Around line 438-446: The test name and expectation conflict because the
event-name regex used by isInteractiveComponent currently matches substrings (so
"visibility-change" matches "change"); update the regex used by
isInteractiveComponent (the pattern `/click|press|select|change|input|submit/i`)
to use word boundaries or a grouped non-capturing boundary like
`\b(?:click|press|select|change|input|submit)\b` (case-insensitive) so only
whole-word event names match, then run tests and, if you intentionally want
substring matching, alternatively rename the test to reflect the current
behavior instead of changing the regex.

In `@tests/handlers/analyzers/type-coverage.test.ts`:
- Around line 219-228: The test currently only checks the metric exists;
strengthen it by asserting the actual scoring/counts for the 'Event typed
payloads' metric produced by analyzeTypeCoverage(BARE_EVENT_TYPE). Locate the
metric via result!.subMetrics.find((m) => m.name === 'Event typed payloads') and
add assertions that reflect the expected outcome: that exactly 1 event is typed
and 3 events were considered (e.g., metric.passed === 1 and metric.total === 3)
and/or that metric.score equals 1/3 (or equivalent numeric score used by the
metric). Ensure the assertions validate the edge-case that the bare "Event" is
excluded while "FocusEvent" counts.
- Around line 133-137: The test name contradicts its assertion: update the
it(...) description for the test using analyzeTypeCoverage and METHODS_ONLY to
reflect that a non-null (scoreable) result is expected when only methods exist;
e.g. rename the string to "returns non-null when only methods exist but no
fields or events" or "returns scoreable result when only methods exist" so the
test name matches the expect(result).not.toBeNull() assertion.

In `@tests/tools/framework.test.ts`:
- Around line 52-55: The test for the detect_framework tool currently asserts
that def.inputSchema.required is strictly undefined, which fails for
semantically-equivalent schemas that set required: []; update the assertion in
the test (the it block named 'detect_framework schema has no required fields'
that looks up FRAMEWORK_TOOL_DEFINITIONS.find(t => t.name ===
'detect_framework')) to accept either undefined or an empty array by checking
that def.inputSchema.required is either falsy/absent or has zero elements (e.g.,
treat undefined as equivalent to an empty array) instead of requiring undefined.
- Around line 80-83: The test currently only checks for success but not that
handleFrameworkCall forwards FAKE_CONFIG to the dispatcher; update the test to
spy/mock the detectFramework/dispatcher entry used by handleFrameworkCall (the
function named detectFramework or the dispatcher object used inside
handleFrameworkCall) and add an assertion that it was called with FAKE_CONFIG
(or that its config argument equals FAKE_CONFIG) in addition to the existing
isError check so the test verifies config passthrough.
- Around line 17-24: The mock for detectFramework in the test uses an
out-of-date result shape: update the vi.fn mock returned object used by
detectFramework to match the real handler contract by renaming regenerationNotes
to notes and adding the missing cemPath property (keep existing fields
framework, version, cemGenerator, formatted) so the mocked return shape aligns
with the detectFramework signature and prevents contract regressions.

In `@tests/tools/story.test.ts`:
- Around line 125-126: Replace loose falsy assertions on the isError flag with
explicit boolean checks: change expect(result.isError).toBeFalsy() to
expect(result.isError).toBe(false) wherever the test asserts success for
result.isError (to mirror the error-case expect(result.isError).toBe(true));
also update any other occurrences of toBeFalsy() used for result.isError in the
same test file so all isError assertions are explicit and symmetric.
- Around line 165-168: Update the test in tests/tools/story.test.ts that calls
handleStoryCall('generate_story', {}, BUTTON_CEM) so it not only checks
result.isError but also asserts the error message contains the expected
validation text (e.g., mentions "tagName" or "required"); locate the failing
test that currently just calls handleStoryCall and add an assertion against
result.error or result.message (depending on how handleStoryCall returns errors)
to ensure the Zod validation error includes "tagName" (or another clear
indicator), keeping references to handleStoryCall and BUTTON_CEM to find the
test quickly.

In `@tests/tools/tokens.test.ts`:
- Around line 17-24: The mock for getDesignTokens currently hard-codes the count
field which can drift from the filtered tokens; update the mock in
getDesignTokens to compute count from the filtered tokens array (e.g., const
filtered = tokens.filter(...); return { tokens: filtered, count:
filtered.length, categories: [...] }) and apply the same change to the other
mocked instance noted (the second getDesignTokens mock around the 26-32 area) so
both return a count derived from their filtered token arrays rather than a
hard-coded number.

In `@tests/tools/validate.test.ts`:
- Around line 152-154: The test builds longHtml with '<hx-button>' +
'x'.repeat(49_980) + '</hx-button>' which yields a total length >50000 because
the wrapper tags add 23 chars; change the repeat counts so total length checks
are correct: set the accept-case longHtml inner repeat to 49_977 (50000 - 23)
and in the over-limit test (around lines 193-194) use 49_978 to produce 50001
total; update the variable longHtml in the tests that call handleValidateCall
accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9a4a213a-1fe6-41cd-ad24-cd9cbc326b60

📥 Commits

Reviewing files that changed from the base of the PR and between 4db9125 and 7325d29.

📒 Files selected for processing (23)

• README.md
• packages/core/src/config.ts
• packages/vscode/package.json
• packages/vscode/src/commands/configureCursorWindsurf.ts
• packages/vscode/src/extension.ts
• src/cli/index.ts
• tests/handlers/analyzers/api-surface.test.ts
• tests/handlers/analyzers/css-architecture.test.ts
• tests/handlers/analyzers/event-architecture.test.ts
• tests/handlers/analyzers/mixin-resolver.test.ts
• tests/handlers/analyzers/naming-consistency.test.ts
• tests/handlers/analyzers/slot-architecture.test.ts
• tests/handlers/analyzers/source-accessibility.test.ts
• tests/handlers/analyzers/type-coverage.test.ts
• tests/integration/server.test.ts
• tests/tools/cdn.test.ts
• tests/tools/composition.test.ts
• tests/tools/framework.test.ts
• tests/tools/story.test.ts
• tests/tools/tokens.test.ts
• tests/tools/typegenerate.test.ts
• tests/tools/typescript.test.ts
• tests/tools/validate.test.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider logging parse errors for debugging.

When mcp.json exists but is malformed, the error is silently swallowed. This makes it hard for users to understand why their existing configuration was reset. Consider showing a warning or logging the error.

♻️ Proposed improvement

       } catch {
-        // If the file is malformed, start fresh but preserve the attempt.
-        existing = { mcpServers: {} };
+        // If the file is malformed, warn the user and start fresh.
+        await vscode.window.showWarningMessage(
+          `Helixir: Existing ${configFilePath} could not be parsed. Creating a fresh config.`,
+        );
+        existing = { mcpServers: {} };
       }

Note: This requires making the inner try-catch async-aware, or you can use console.warn for simpler logging.

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/commands/configureCursorWindsurf.ts` around lines 75 -
78, The catch block swallowing JSON parse errors should capture and log the
error so users know why mcp.json was reset: change the empty catch to catch
(err) and call a warning logger (e.g., console.warn or the extension logger)
including the filename ("mcp.json") and err.message/stack, then continue to set
existing = { mcpServers: {} } as before; if you replace console.warn with an
async logger, ensure the inner try-catch remains compatible (await where needed)
so logging doesn't break error handling in the configureCursorWindsurf flow.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Missing MCP_WC_PROJECT_ROOT environment variable in generated config.

The generated mcp.json entry has an empty env object, but mcpProvider.ts (lines 22-24) sets MCP_WC_PROJECT_ROOT to the workspace folder. Without this environment variable, the MCP server won't know where to find the component library's custom-elements.json, causing it to fail or use incorrect defaults.

🐛 Proposed fix

+    // Resolve the workspace root for the MCP_WC_PROJECT_ROOT env var.
+    const workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? '';
+
     // Upsert the helixir entry.
     existing.mcpServers['helixir'] = {
       command: 'node',
       args: [serverScriptPath],
-      env: {},
+      env: workspaceRoot ? { MCP_WC_PROJECT_ROOT: workspaceRoot } : {},
     };

Note: You may want to add a comment explaining that users should update MCP_WC_PROJECT_ROOT if they move the config or work with multiple projects.

📝 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

	// Upsert the helixir entry.
	existing.mcpServers['helixir'] = {
	command: 'node',
	args: [serverScriptPath],
	env: {},
	};
	// Resolve the workspace root for the MCP_WC_PROJECT_ROOT env var.
	const workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? '';
	// Upsert the helixir entry.
	existing.mcpServers['helixir'] = {
	command: 'node',
	args: [serverScriptPath],
	env: workspaceRoot ? { MCP_WC_PROJECT_ROOT: workspaceRoot } : {},
	};

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/commands/configureCursorWindsurf.ts` around lines 81 -
86, The mcp entry created in configureCursorWindsurf (the
existing.mcpServers['helixir'] upsert) leaves env empty so MCP_WC_PROJECT_ROOT
is missing; update that env object to include MCP_WC_PROJECT_ROOT set to the
same workspace root path used by mcpProvider (the workspace folder used when
creating the server), and add a short comment explaining users should update
MCP_WC_PROJECT_ROOT if they move the config or work across multiple projects.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Docstring is incomplete.

The docstring mentions registering the MCP server provider and "Run Health Check" command but omits the configureCursorWindsurf command that's also registered on line 12.

📝 Proposed fix

 /**
  * Called when the extension is activated.
  * Registers the Helixir MCP server definition provider and the
- * "Helixir: Run Health Check" command.
+ * "Helixir: Run Health Check" and "Helixir: Configure for Cursor/Windsurf" commands.
  */

📝 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

	/**
	* Called when the extension is activated.
	* Registers the Helixir MCP server definition provider and the
	* "Helixir: Run Health Check" command.
	*/
	/**
	* Called when the extension is activated.
	* Registers the Helixir MCP server definition provider and the
	* "Helixir: Run Health Check" and "Helixir: Configure for Cursor/Windsurf" commands.
	*/

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/extension.ts` around lines 5 - 9, Update the top
docstring for the extension activation to mention the missing command: include
the "Helixir: Configure Cursor Windsurf" (configureCursorWindsurf) registration
alongside the existing Helixir MCP server definition provider and "Helixir: Run
Health Check" command; locate the docstring near the activate function in
extension.ts and add a brief phrase describing that configureCursorWindsurf is
registered so the comment accurately reflects all commands being registered.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Inconsistent tool count between badge and text.

The badge on line 20 shows tools-73 while line 31 claims "87+ MCP tools". These should be consistent to avoid confusion.

🔧 Proposed fix

Either update the badge to match the actual count:

-[![Tools](https://img.shields.io/badge/tools-73-purple)](https://www.npmjs.com/package/helixir)
+[![Tools](https://img.shields.io/badge/tools-87-purple)](https://www.npmjs.com/package/helixir)

Or update the text if 73 is correct:

-- **87+ MCP tools out of the box** — Component discovery, health scoring, design token lookup, TypeScript diagnostics, breaking-change detection, Storybook story generation, Shadow DOM styling validators, theme scaffolding, and scaffold/extend tools — all callable by any MCP-compatible AI agent.
+- **73+ MCP tools out of the box** — Component discovery, health scoring, design token lookup, TypeScript diagnostics, breaking-change detection, Storybook story generation, Shadow DOM styling validators, theme scaffolding, and scaffold/extend tools — all callable by any MCP-compatible AI agent.

Also applies to: 31-31

🤖 Prompt for AI Agents

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

In `@README.md` at line 20, The README has an inconsistent tool count: the badge
text "tools-73" in the badge markup and the written claim "87+ MCP tools"
differ; pick the correct canonical count and make both places match by updating
either the badge label (the "tools-73" segment in the badge markdown) or the
written text "87+ MCP tools" so they show the same number, and ensure any other
occurrences of the tool count in the README are updated consistently.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

CI-blocking mismatch: this expectation conflicts with current reflection counting logic.

Line 338 fails (13 vs 25). The test expects union semantics (“attribute or reflects”), but packages/core/src/handlers/analyzers/api-surface.ts currently uses Math.max(...), which undercounts mixed cases.

Root-cause fix in analyzer (recommended)

-  const fieldsWithAttribute = fields.filter(
-    (m) => typeof m.attribute === 'string' && m.attribute.length > 0,
-  );
-  const fieldsReflecting = fields.filter((m) => m.reflects === true);
-  const reflectedCount = Math.max(fieldsWithAttribute.length, fieldsReflecting.length);
+  const reflectedCount = fields.filter(
+    (m) =>
+      (typeof m.attribute === 'string' && m.attribute.length > 0) || m.reflects === true,
+  ).length;

🧰 Tools

🪛 GitHub Actions: Test

[error] 338-338: AssertionError: expected attrMetric.score to equal attrMetric.maxScore (expected reflects/attribute reflection metric to reach maxScore; received 13 instead of 25).

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/api-surface.test.ts` around lines 325 - 339, The
metric currently uses Math.max(...) which undercounts mixed cases; in
analyzeApiSurface update the "Attribute reflection" calculation (in the logic
that builds attrMetric) to count members that have reflects === true OR a
defined attribute (i.e., compute the union of members satisfying reflects or
attribute) instead of taking Math.max; ensure you deduplicate by member name so
a single member with both reflects and attribute is only counted once and then
set attrMetric.score to that union count (capped by attrMetric.maxScore).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Add passthrough assertion for dispatcher config forwarding.

Line 81 validates success but not that handleFrameworkCall forwards FAKE_CONFIG to detectFramework.

Proposed enhancement

   it('returns a success result for detect_framework with empty args', async () => {
+    const { detectFramework } = await import('../../packages/core/src/handlers/framework.js');
     const result = await handleFrameworkCall('detect_framework', {}, FAKE_CONFIG);
     expect(result.isError).toBeFalsy();
+    expect(vi.mocked(detectFramework)).toHaveBeenCalledWith(FAKE_CONFIG);
   });

🤖 Prompt for AI Agents

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

In `@tests/tools/framework.test.ts` around lines 80 - 83, The test currently only
checks for success but not that handleFrameworkCall forwards FAKE_CONFIG to the
dispatcher; update the test to spy/mock the detectFramework/dispatcher entry
used by handleFrameworkCall (the function named detectFramework or the
dispatcher object used inside handleFrameworkCall) and add an assertion that it
was called with FAKE_CONFIG (or that its config argument equals FAKE_CONFIG) in
addition to the existing isError check so the test verifies config passthrough.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Use explicit toBe(false) instead of toBeFalsy() for isError checks.

The error case tests use toBe(true) for isError assertions (e.g., line 155), but success cases use toBeFalsy(). This inconsistency can mask issues—toBeFalsy() would pass for undefined, null, 0, or '', not just false. Use toBe(false) for symmetric, explicit assertions.

♻️ Suggested fix

   it('returns a success result for a known component', async () => {
     const result = await handleStoryCall('generate_story', { tagName: 'hx-button' }, BUTTON_CEM);
-    expect(result.isError).toBeFalsy();
+    expect(result.isError).toBe(false);
   });

   it('works for a second component in a multi-module CEM', async () => {
     const result = await handleStoryCall('generate_story', { tagName: 'hx-card' }, MULTI_CEM);
-    expect(result.isError).toBeFalsy();
+    expect(result.isError).toBe(false);
     expect(result.content[0].text).toContain('hx-card');
   });

Also applies to: 140-141

🤖 Prompt for AI Agents

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

In `@tests/tools/story.test.ts` around lines 125 - 126, Replace loose falsy
assertions on the isError flag with explicit boolean checks: change
expect(result.isError).toBeFalsy() to expect(result.isError).toBe(false)
wherever the test asserts success for result.isError (to mirror the error-case
expect(result.isError).toBe(true)); also update any other occurrences of
toBeFalsy() used for result.isError in the same test file so all isError
assertions are explicit and symmetric.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider asserting the error message for missing tagName.

This test only checks that isError is true, but doesn't verify the error message. Other error tests assert message content. Adding an assertion would ensure Zod validation produces a meaningful error (e.g., containing "tagName" or "required").

♻️ Suggested improvement

   it('returns error when tagName is missing', async () => {
     const result = await handleStoryCall('generate_story', {}, BUTTON_CEM);
     expect(result.isError).toBe(true);
+    expect(result.content[0].text).toMatch(/tagName|required/i);
   });

📝 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

	it('returns error when tagName is missing', async () => {
	const result = await handleStoryCall('generate_story', {}, BUTTON_CEM);
	expect(result.isError).toBe(true);
	});
	it('returns error when tagName is missing', async () => {
	const result = await handleStoryCall('generate_story', {}, BUTTON_CEM);
	expect(result.isError).toBe(true);
	expect(result.content[0].text).toMatch(/tagName|required/i);
	});

🤖 Prompt for AI Agents

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

In `@tests/tools/story.test.ts` around lines 165 - 168, Update the test in
tests/tools/story.test.ts that calls handleStoryCall('generate_story', {},
BUTTON_CEM) so it not only checks result.isError but also asserts the error
message contains the expected validation text (e.g., mentions "tagName" or
"required"); locate the failing test that currently just calls handleStoryCall
and add an assertion against result.error or result.message (depending on how
handleStoryCall returns errors) to ensure the Zod validation error includes
"tagName" (or another clear indicator), keeping references to handleStoryCall
and BUTTON_CEM to find the test quickly.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Keep mocked count derived from filtered tokens to avoid drift.

The mock currently hard-codes count; deriving it from the filtered array will prevent inconsistent fixtures as scenarios grow.

Proposed refactor

-  getDesignTokens: vi.fn(async (_config: unknown, category?: string) => ({
-    tokens: [
+  getDesignTokens: vi.fn(async (_config: unknown, category?: string) => {
+    const tokens = [
       { name: '--color-primary', value: '#0066cc', category: 'color' },
       { name: '--color-secondary', value: '#666', category: 'color' },
       { name: '--spacing-md', value: '1rem', category: 'spacing' },
-    ].filter((t) => !category || t.category === category),
-    count: category ? 2 : 3,
-    categories: ['color', 'spacing'],
-  })),
-  findToken: vi.fn(async (_config: unknown, query: string) => ({
-    tokens: [{ name: '--color-primary', value: '#0066cc', category: 'color' }].filter(
+    ].filter((t) => !category || t.category === category);
+    return {
+      tokens,
+      count: tokens.length,
+      categories: ['color', 'spacing'],
+    };
+  }),
+  findToken: vi.fn(async (_config: unknown, query: string) => {
+    const tokens = [{ name: '--color-primary', value: '#0066cc', category: 'color' }].filter(
       (t) => t.name.includes(query) || t.value.includes(query),
-    ),
-    count: 1,
-    query,
-  })),
+    );
+    return {
+      tokens,
+      count: tokens.length,
+      query,
+    };
+  }),
 }));

Also applies to: 26-32

🤖 Prompt for AI Agents

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

In `@tests/tools/tokens.test.ts` around lines 17 - 24, The mock for
getDesignTokens currently hard-codes the count field which can drift from the
filtered tokens; update the mock in getDesignTokens to compute count from the
filtered tokens array (e.g., const filtered = tokens.filter(...); return {
tokens: filtered, count: filtered.length, categories: [...] }) and apply the
same change to the other mocked instance noted (the second getDesignTokens mock
around the 26-32 area) so both return a count derived from their filtered token
arrays rather than a hard-coded number.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix the 50,000-character boundary construction in the test.

Line 153 currently builds an HTML string above 50,000 characters once tag wrappers are included, so the “accepts html up to 50000 characters” assertion is invalid and causes the CI failure.

Proposed fix

-  it('accepts html up to 50000 characters', () => {
-    const longHtml = '<hx-button>' + 'x'.repeat(49_980) + '</hx-button>';
+  it('accepts html up to 50000 characters', () => {
+    const wrapper = '<hx-button></hx-button>';
+    const maxHtmlLen = 50_000;
+    const longHtml = '<hx-button>' + 'x'.repeat(maxHtmlLen - wrapper.length) + '</hx-button>';
     const result = handleValidateCall(
       'validate_usage',
       { tagName: 'hx-button', html: longHtml },
       BUTTON_CEM,
     );
     expect(result.isError).toBeFalsy();
   });
@@
-  it('returns error when html exceeds 50000 characters', () => {
-    const tooLongHtml = '<hx-button>' + 'x'.repeat(50_000) + '</hx-button>';
+  it('returns error when html exceeds 50000 characters', () => {
+    const wrapper = '<hx-button></hx-button>';
+    const maxHtmlLen = 50_000;
+    const tooLongHtml =
+      '<hx-button>' + 'x'.repeat(maxHtmlLen - wrapper.length + 1) + '</hx-button>';
     const result = handleValidateCall(
       'validate_usage',
       { tagName: 'hx-button', html: tooLongHtml },
       BUTTON_CEM,
     );
     expect(result.isError).toBe(true);
   });

Also applies to: 193-194

🤖 Prompt for AI Agents

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

In `@tests/tools/validate.test.ts` around lines 152 - 154, The test builds
longHtml with '<hx-button>' + 'x'.repeat(49_980) + '</hx-button>' which yields a
total length >50000 because the wrapper tags add 23 chars; change the repeat
counts so total length checks are correct: set the accept-case longHtml inner
repeat to 49_977 (50000 - 23) and in the over-limit test (around lines 193-194)
use 49_978 to produce 50001 total; update the variable longHtml in the tests
that call handleValidateCall accordingly.

[coderabbitai]

Actionable comments posted: 14

♻️ Duplicate comments (20)

tests/tools/validate.test.ts (1)

152-154: ⚠️ Potential issue | 🔴 Critical

Fix boundary math for 50,000-character tests (current values invalidate both assertions).

Line 153 and Line 193 overshoot intended total lengths because tag wrappers add extra characters. This makes the “<= 50,000 passes / > 50,000 fails” checks incorrect and explains the failing CI test.

Proposed fix

-  it('accepts html up to 50000 characters', () => {
-    const longHtml = '<hx-button>' + 'x'.repeat(49_980) + '</hx-button>';
+  it('accepts html up to 50000 characters', () => {
+    const wrapper = '<hx-button></hx-button>';
+    const maxHtmlLen = 50_000;
+    const longHtml = '<hx-button>' + 'x'.repeat(maxHtmlLen - wrapper.length) + '</hx-button>';
     const result = handleValidateCall(
       'validate_usage',
       { tagName: 'hx-button', html: longHtml },
       BUTTON_CEM,
     );
     expect(result.isError).toBeFalsy();
   });
@@
-  it('returns error when html exceeds 50000 characters', () => {
-    const tooLongHtml = '<hx-button>' + 'x'.repeat(50_000) + '</hx-button>';
+  it('returns error when html exceeds 50000 characters', () => {
+    const wrapper = '<hx-button></hx-button>';
+    const maxHtmlLen = 50_000;
+    const tooLongHtml =
+      '<hx-button>' + 'x'.repeat(maxHtmlLen - wrapper.length + 1) + '</hx-button>';
     const result = handleValidateCall(
       'validate_usage',
       { tagName: 'hx-button', html: tooLongHtml },
       BUTTON_CEM,
     );
     expect(result.isError).toBe(true);
   });

Also applies to: 192-194

🤖 Prompt for AI Agents

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

In `@tests/tools/validate.test.ts` around lines 152 - 154, The tests build HTML
strings with tag wrappers but use hard-coded repeat counts that ignore the
wrapper length, causing both "≤50,000 passes" and ">50,000 fails" assertions to
be invalid; update the tests that create longHtml and the over-limit case (the
variables used in the 'accepts html up to 50000 characters' and corresponding
failing test that call handleValidateCall) to compute the repeat count as 50_000
minus the wrapper length (e.g., 50_000 - '<hx-button>'.length -
'</hx-button>'.length) for the boundary pass case, and use that value + 1 for
the failing case so the total string lengths correctly hit 50,000 and 50,001
respectively.

tests/tools/story.test.ts (2)

165-168: ⚠️ Potential issue | 🟡 Minor

Assert validation message when tagName is missing.

On Line 165-168, this test only checks isError. Add a message assertion so schema-validation behavior is actually verified.

♻️ Suggested fix

   it('returns error when tagName is missing', async () => {
     const result = await handleStoryCall('generate_story', {}, BUTTON_CEM);
     expect(result.isError).toBe(true);
+    expect(result.content[0].text).toMatch(/tagName|required/i);
   });

🤖 Prompt for AI Agents

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

In `@tests/tools/story.test.ts` around lines 165 - 168, The test "returns error
when tagName is missing" only asserts result.isError and should also assert the
validation message; update the test that calls handleStoryCall('generate_story',
{}, BUTTON_CEM) to additionally check the error message on the returned object
(e.g., assert result.error?.message or result.message contains a string
indicating tagName is required or mentions "tagName") so the schema-validation
behavior is verified; locate the test block in tests/tools/story.test.ts and add
a single assertion referencing result.error or result.message to ensure the
failure is caused by the missing tagName.

---

125-126: ⚠️ Potential issue | 🟡 Minor

Use explicit boolean assertions for isError in success cases.

On Line 125 and Line 140, toBeFalsy() is too loose for a boolean contract. Use toBe(false) for symmetry with error-path checks.

♻️ Suggested fix

-    expect(result.isError).toBeFalsy();
+    expect(result.isError).toBe(false);
...
-    expect(result.isError).toBeFalsy();
+    expect(result.isError).toBe(false);

Also applies to: 140-141

🤖 Prompt for AI Agents

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

In `@tests/tools/story.test.ts` around lines 125 - 126, Replace loose boolean
assertions using result.isError with an explicit equality check: change
expect(result.isError).toBeFalsy() to expect(result.isError).toBe(false) in the
test(s) where the success case is being asserted (both occurrences referencing
result.isError in the story tests), ensuring symmetry with the error-path
checks; update both places and run the test suite to confirm behavior.

tests/handlers/analyzers/mixin-resolver.test.ts (3)

19-23: ⚠️ Potential issue | 🟡 Minor

Drop unused type imports to fix lint failures.

ResolvedSource and InheritanceChainResult are imported but never used, which fails @typescript-eslint/no-unused-vars (Line 21, Line 22).

Proposed fix

 import {
   resolveInheritanceChain,
-  type ResolvedSource,
-  type InheritanceChainResult,
 } from '../../../packages/core/src/handlers/analyzers/mixin-resolver.js';

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/mixin-resolver.test.ts` around lines 19 - 23, Remove
the unused type imports to satisfy linting: edit the import that currently
brings in resolveInheritanceChain, ResolvedSource, and InheritanceChainResult
from mixin-resolver and drop ResolvedSource and InheritanceChainResult so only
resolveInheritanceChain is imported; this keeps the resolveInheritanceChain
symbol while eliminating the unused ResolvedSource and InheritanceChainResult
type imports.

---

72-82: ⚠️ Potential issue | 🟡 Minor

Remove unused MIXIN_IMPORT_SOURCE fixture (or add a test that uses it).

MIXIN_IMPORT_SOURCE is currently dead code and fails linting (Line 73).

Proposed fix (remove fixture)

-// A component that imports an a11y-relevant mixin
-const MIXIN_IMPORT_SOURCE = `
-import { FocusMixin } from './focus-mixin.js';
-import { KeyboardMixin } from './keyboard-mixin.js';
-
-class MyDropdown extends FocusMixin(KeyboardMixin(HTMLElement)) {
-  connectedCallback() {
-    this.setAttribute('role', 'listbox');
-  }
-}
-`;

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/mixin-resolver.test.ts` around lines 72 - 82, The
fixture constant MIXIN_IMPORT_SOURCE is unused and causing a lint failure;
either delete the constant declaration or add a test that imports and asserts
behavior using MIXIN_IMPORT_SOURCE. Locate the constant named
MIXIN_IMPORT_SOURCE in the test file and remove its declaration if it's not
needed, or create a new test (or update an existing one) that references
MIXIN_IMPORT_SOURCE (e.g., passing it into the analyzer under test) so the
symbol is used.

---

28-29: ⚠️ Potential issue | 🟠 Major

Replace hardcoded local worktree path with a portable project-root derivation.

The absolute path is machine-specific and will break on CI and other dev environments (Line 28, Line 29).

Proposed fix

-import { resolve } from 'node:path';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
 ...
-const WORKTREE =
-  '/Volumes/Development/booked/helixir/.worktrees/feature-test-add-test-suites-for-8-untested';
+const __dirname = dirname(fileURLToPath(import.meta.url));
+const PROJECT_ROOT = resolve(__dirname, '../../..');

Then replace all WORKTREE usages with PROJECT_ROOT in this file.

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/mixin-resolver.test.ts` around lines 28 - 29, The
test currently defines a hardcoded machine-specific WORKTREE constant which
breaks CI; replace it with a portable PROJECT_ROOT derived at runtime (e.g.,
using path and __dirname to resolve the repository/project root) and update
every usage of WORKTREE in this test to use PROJECT_ROOT instead; locate the
WORKTREE constant and all references in mixin-resolver.test.ts and swap them to
the new PROJECT_ROOT derivation so the path is computed relative to the repo
rather than hardcoded.

tests/tools/tokens.test.ts (1)

17-32: 🧹 Nitpick | 🔵 Trivial

Derive mocked count from filtered tokens to prevent fixture drift.

On Line 23 and Line 30, count is hard-coded. If fixture data changes, assertions can become inconsistent with returned arrays.

Proposed refactor

 vi.mock('../../packages/core/src/handlers/tokens.js', () => ({
-  getDesignTokens: vi.fn(async (_config: unknown, category?: string) => ({
-    tokens: [
+  getDesignTokens: vi.fn(async (_config: unknown, category?: string) => {
+    const tokens = [
       { name: '--color-primary', value: '#0066cc', category: 'color' },
       { name: '--color-secondary', value: '#666', category: 'color' },
       { name: '--spacing-md', value: '1rem', category: 'spacing' },
-    ].filter((t) => !category || t.category === category),
-    count: category ? 2 : 3,
-    categories: ['color', 'spacing'],
-  })),
-  findToken: vi.fn(async (_config: unknown, query: string) => ({
-    tokens: [{ name: '--color-primary', value: '#0066cc', category: 'color' }].filter(
+    ].filter((t) => !category || t.category === category);
+    return {
+      tokens,
+      count: tokens.length,
+      categories: ['color', 'spacing'],
+    };
+  }),
+  findToken: vi.fn(async (_config: unknown, query: string) => {
+    const tokens = [{ name: '--color-primary', value: '#0066cc', category: 'color' }].filter(
       (t) => t.name.includes(query) || t.value.includes(query),
-    ),
-    count: 1,
-    query,
-  })),
+    );
+    return {
+      tokens,
+      count: tokens.length,
+      query,
+    };
+  }),
 }));

🤖 Prompt for AI Agents

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

In `@tests/tools/tokens.test.ts` around lines 17 - 32, The mocked responses for
getDesignTokens and findToken hard-code count values which can drift from the
filtered tokens; update both mocks (getDesignTokens and findToken) to compute
the filtered tokens into a local variable (e.g., filteredTokens) and set count
to filteredTokens.length, returning filteredTokens as the tokens array and
keeping other fields (categories/query) unchanged so the count always matches
the returned tokens.

tests/tools/theme.test.ts (1)

122-129: ⚠️ Potential issue | 🔴 Critical

Fix failing strict-validation assertion (contract mismatch with CreateThemeArgsSchema).

Line 122–Line 129 expects strict rejection, but current CreateThemeArgsSchema is non-strict (z.object(...)), so this test fails in CI. Align either the schema (make strict) or this assertion (allow extra props).

#!/bin/bash
# Verify current schema strictness and the failing assertion side-by-side.
rg -n -C3 'CreateThemeArgsSchema|z\.object|strict\(' packages/core/src/tools/theme.ts
rg -n -C3 'rejects unexpected extra properties|unknownProp|result\.isError' tests/tools/theme.test.ts

🤖 Prompt for AI Agents

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

In `@tests/tools/theme.test.ts` around lines 122 - 129, The test expects extra
properties to be rejected but CreateThemeArgsSchema is currently permissive;
update the Zod schema (CreateThemeArgsSchema in theme.ts) to be strict by
calling .strict() on the z.object(...) (so parsing/validation will fail on
unknownProp) and ensure any helper that uses CreateThemeArgsSchema (e.g., the
handler that calls parse/safeParse) continues to use the same schema for
validation; this aligns the schema behavior with the test that asserts
result.isError for unknown properties.

tests/handlers/analyzers/source-accessibility.test.ts (4)

24-24: ⚠️ Potential issue | 🟡 Minor

Remove the unused resolve import.

Line 24 is currently unused and fails linting.

Proposed fix

-import { resolve } from 'node:path';

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/source-accessibility.test.ts` at line 24, Remove the
unused "resolve" import from the top of
tests/handlers/analyzers/source-accessibility.test.ts (the import line "import {
resolve } from 'node:path'"); delete that import statement so the file no longer
references the unused symbol and re-run lint/tests to confirm the lint error is
resolved.

---

84-95: ⚠️ Potential issue | 🟡 Minor

Isolate .sr-only detection instead of duplicating the same fixture assertion.

Both tests assert screenReaderSupport using SCREEN_READER_SOURCE, so the second case is redundant and doesn’t prove .sr-only independently.

Proposed fix

+const SCREEN_READER_SR_ONLY_SOURCE = `
+class MyBadge extends LitElement {
+  render() {
+    return html\`<span class="sr-only">Count: \${this.count}</span>\`;
+  }
+}
+`;
@@
   it('detects screenReaderSupport from .sr-only class', () => {
-    const markers = scanSourceForA11yPatterns(SCREEN_READER_SOURCE);
+    const markers = scanSourceForA11yPatterns(SCREEN_READER_SR_ONLY_SOURCE);
     expect(markers.screenReaderSupport).toBe(true);
   });

Also applies to: 203-211

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/source-accessibility.test.ts` around lines 84 - 95,
The two tests both assert screenReaderSupport using the same
SCREEN_READER_SOURCE fixture, so duplicate coverage doesn't prove .sr-only is
detected independently; modify the second test to use a minimal fixture that
isolates the .sr-only case (e.g., a small component markup containing only a
visually hidden element and an exposed text node) and assert screenReaderSupport
against that fixture instead of SCREEN_READER_SOURCE; update the test
referencing SCREEN_READER_SOURCE (and the similar block around lines 203-211) to
use the new isolated fixture name and keep the same assertion logic for
screenReaderSupport to validate .sr-only detection specifically.

---

449-470: ⚠️ Potential issue | 🟠 Major

Replace machine-specific WORKTREE and remove conditional assertion masking.

The absolute path at Lines 450-451 is not CI-portable, and Lines 467-469 can silently pass when resolution fails. Use a deterministic fixture/project root and assert non-null explicitly for the .js→.ts/.js case.

Proposed direction

-  const WORKTREE =
-    '/Volumes/Development/booked/helixir/.worktrees/feature-test-add-test-suites-for-8-untested';
+  const WORKTREE = /* deterministic fixture root resolved from test directory */;

@@
   it('resolves .ts equivalent for .js path', () => {
-    const result = resolveComponentSourceFilePath(WORKTREE, 'packages/core/src/config.js');
-    if (result) {
-      expect(result.endsWith('.ts') || result.endsWith('.js')).toBe(true);
-    }
+    const result = resolveComponentSourceFilePath(WORKTREE, 'packages/core/src/config.js');
+    expect(result).not.toBeNull();
+    expect(result!.endsWith('.ts') || result!.endsWith('.js')).toBe(true);
   });

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/source-accessibility.test.ts` around lines 449 -
470, Replace the hard-coded, machine-specific WORKTREE with a deterministic test
fixture/project root (e.g., a path under the test/fixtures or a tmp directory
created for tests) so the test is CI-portable, and change the ".js → .ts/.js"
case that currently uses a conditional assertion to assert the resolver returned
a non-null value explicitly (call resolveComponentSourceFilePath(WORKTREE,
'packages/core/src/config.js'), expect result not to be null) and then assert
result endsWith('.ts') || result.endsWith('.js'); reference
resolveComponentSourceFilePath to locate the logic to test and update the test
case accordingly.

---

438-446: ⚠️ Potential issue | 🟠 Major

Align test intent with expectation; current case codifies a substring-match false positive.

Line 438 says “returns false,” but Line 445 expects true due matching visibility-change via change substring. Either rename the test to reflect current behavior, or (preferably) tighten the event regex and assert false.

#!/bin/bash
# Verify analyzer event regex and this test's expectation are aligned
rg -nP --type=ts -C3 '\b(click\|press\|select\|change\|input\|submit)\b|visibility-change|returns false when events are non-interactive|toBe\(true\)' \
  packages/core/src/handlers/analyzers/source-accessibility.ts \
  tests/handlers/analyzers/source-accessibility.test.ts

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/source-accessibility.test.ts` around lines 438 -
446, The test currently expects a non-interactive result but passes because the
event-name regex performs a substring match (e.g., 'visibility-change' matches
'change'); update the event matching in the analyzer (the regex used by
isInteractiveComponent / the INTERACTIVE_EVENT regex in source-accessibility.ts)
to only match whole event names (e.g., use
^(click|press|select|change|input|submit)$ or word-boundary logic that prevents
hyphenated substrings) and then change the test assertion in
tests/handlers/analyzers/source-accessibility.test.ts to expect false for the
decl with events ['resize','visibility-change'] so the test name "returns false
when events are non-interactive" aligns with the behavior.

tests/handlers/analyzers/type-coverage.test.ts (2)

133-137: ⚠️ Potential issue | 🟡 Minor

Test name contradicts the assertion.

The test name says "returns null when only methods exist" but the assertion expects not.toBeNull(). The comment and assertion are correct (methods make the result scoreable per the implementation), but the test name is misleading.

Suggested fix

-    it('returns null when only methods exist but no fields or events', () => {
+    it('returns non-null when only methods exist (methods are scoreable)', () => {
       // Methods without return types still count as "methods" for scoring
       const result = analyzeTypeCoverage(METHODS_ONLY);
       expect(result).not.toBeNull(); // methods exist so it's scoreable
     });

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/type-coverage.test.ts` around lines 133 - 137, The
test name is misleading: update the test title so it matches the assertion that
analyzeTypeCoverage(METHODS_ONLY) should not be null; locate the spec in
tests/handlers/analyzers/type-coverage.test.ts (the it(...) block around the
call to analyzeTypeCoverage and METHODS_ONLY) and change the string from
"returns null when only methods exist but no fields or events" to something like
"returns non-null when only methods exist but no fields or events" (or
equivalent wording) so the test name aligns with the comment and
expect(result).not.toBeNull().

---

219-228: 🧹 Nitpick | 🔵 Trivial

Weak assertion doesn't validate the edge-case behavior.

This test only checks that the metric exists but doesn't verify the actual scoring behavior for bare "Event" types. The inline comment is also confusing. Consider asserting the actual score to validate the intended behavior.

Suggested fix

     it('treats bare "Event" as untyped payload', () => {
       const result = analyzeTypeCoverage(BARE_EVENT_TYPE);
       const eventMetric = result!.subMetrics.find((m) => m.name === 'Event typed payloads');
-      // 1 of 3 events has proper CustomEvent<T> type
-      // "Event" counts as untyped, "FocusEvent" is also bare (not CustomEvent<T>)
-      // Wait — "Event" is excluded but "FocusEvent" is NOT "Event" exactly, so...
-      // Actually "FocusEvent" !== 'Event', so it passes the filter
-      // Only bare 'Event' text is excluded → "change" with type.text='Event' is excluded
-      expect(eventMetric).toBeDefined();
+      // 2 of 3 events counted as typed: FocusEvent and CustomEvent<string>
+      // Only bare 'Event' is excluded → round(2/3 * 35) = 23
+      expect(eventMetric!.score).toBe(23);
     });

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/type-coverage.test.ts` around lines 219 - 228, The
test for analyzeTypeCoverage using BARE_EVENT_TYPE currently only asserts
presence of eventMetric and has a confusing comment; update the test to assert
the actual metric value (e.g., check eventMetric.score or eventMetric.counts to
confirm bare "Event" was treated as untyped and excluded from typed payloads)
and simplify the comment to state the expected numeric outcome for the scenario.
Locate the test block referencing analyzeTypeCoverage(BARE_EVENT_TYPE) and the
eventMetric lookup, replace or extend the expect(eventMetric).toBeDefined() with
a precise expectation on the metric's score/count that matches the intended
behavior, and remove the contradictory lines in the comment so the test
documents the expected numeric result.

packages/vscode/src/mcpProvider.ts (1)

21-21: ⚠️ Potential issue | 🟡 Minor

Don't fall back to process.cwd() when no folder is open.

That can point Helixir at an unrelated directory and produce very confusing custom-elements.json failures. Return no definitions until a workspace folder exists.

🔧 Proposed fix

-      const workspaceFolder = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd();
+      const workspaceFolder = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath;
+      if (!workspaceFolder) {
+        console.warn('[helixir-vscode] No workspace folder open. MCP server will not be registered.');
+        return [];
+      }

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/mcpProvider.ts` at line 21, The code currently falls back
to process.cwd() for workspaceFolder which can point to an unrelated directory;
remove the fallback so workspaceFolder is set to
vscode.workspace.workspaceFolders?.[0]?.uri.fsPath (or undefined) and update the
provider entrypoint (the method that uses workspaceFolder — e.g., the
definition/provider function that reads custom-elements.json) to return no
definitions/early-exit when workspaceFolder is undefined. In short: stop using
process.cwd(), treat workspaceFolder as optional, and add an early return that
yields empty results until a workspace folder exists.

packages/vscode/README.md (1)

39-44: ⚠️ Potential issue | 🟡 Minor

Document the Cursor/Windsurf setup command too.

packages/vscode/src/extension.ts registers a second user-facing command, but the Commands table only mentions the health check. Add it here so the feature is discoverable.

🤖 Prompt for AI Agents

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

In `@packages/vscode/README.md` around lines 39 - 44, The README Commands table is
missing the second user-facing command registered in
packages/vscode/src/extension.ts; add a new table row whose Command column
matches the command label/string used in the extension registration (the
registerCommand call in packages/vscode/src/extension.ts) and whose Description
column briefly states that it opens the guided Cursor/Windsurf setup (e.g.,
"Helixir: Setup Cursor/Windsurf" — "Guides you through Cursor/Windsurf setup").
Ensure the command label exactly matches the string passed to registerCommand so
users can discover it.

.github/workflows/publish-vscode.yml (1)

46-52: ⚠️ Potential issue | 🔴 Critical

Run the publish CLIs from packages/vscode.

Both publish steps execute at the repo root, so vsce and ovsx will read the wrong manifest instead of the extension manifest. That makes this workflow fail before it can publish anything.

🔧 Proposed fix

       - name: Publish to VS Code Marketplace
+        working-directory: packages/vscode
         run: npx `@vscode/vsce` publish --no-dependencies
         env:
           VSCE_PAT: ${{ secrets.VSCE_PAT }}

       - name: Publish to Open VSX
+        working-directory: packages/vscode
         run: npx ovsx publish --no-dependencies -p ${{ secrets.OVSX_PAT }}

🤖 Prompt for AI Agents

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

In @.github/workflows/publish-vscode.yml around lines 46 - 52, The publish steps
run at repo root and pick up the wrong manifest; update the two steps named
"Publish to VS Code Marketplace" and "Publish to Open VSX" so they execute from
the extension package directory (packages/vscode) — e.g., set working-directory
to packages/vscode or prepend a cd packages/vscode && to the existing run
commands for npx `@vscode/vsce` publish and npx ovsx publish so the tool reads the
correct extension manifest and uses the existing VSCE_PAT/OVSX_PAT env vars.

tests/tools/extend.test.ts (1)

219-220: 🧹 Nitpick | 🔵 Trivial

Keep this test synchronous for consistency with the rest of the suite.

Line 219/220 mixes async + dynamic import with a synchronous handleExtendCall path. Converting this to the same import style used elsewhere will simplify the test.

#!/bin/bash
# Verify async/dynamic-import usage in this suite for consistency checks.
rg -nP --type=ts "^\s*it\(.+\basync\b|await import\(" tests/tools/extend.test.ts

🤖 Prompt for AI Agents

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

In `@tests/tools/extend.test.ts` around lines 219 - 220, The test "returns error
when handler throws (e.g. parent not found in CEM)" mixes async/dynamic import
with a synchronous path; make it synchronous by removing the async from the it
callback and replacing the dynamic await import of extend.js with a synchronous
require/static import so you use the same import style as the rest of the suite
(locate the const { extendComponent } usage in this test and change import style
accordingly), ensuring handleExtendCall (the synchronous path) remains unchanged
and no awaits are left in the test body.

packages/vscode/src/commands/configureCursorWindsurf.ts (2)

75-78: ⚠️ Potential issue | 🟡 Minor

Do not silently swallow malformed config parse failures.

Line 75 resets config without surfacing why. This makes recovery/debugging harder when mcp.json is malformed.

♻️ Proposed fix

-      } catch {
+      } catch (err) {
+        console.warn(`Helixir: failed to parse ${configFilePath}`, err);
         // If the file is malformed, start fresh but preserve the attempt.
         existing = { mcpServers: {} };
       }

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/commands/configureCursorWindsurf.ts` around lines 75 -
78, The catch block that currently swallows JSON parse errors when reading
mcp.json (resetting existing = { mcpServers: {} }) should instead capture the
thrown error and surface it for debugging; update the catch in
configureCursorWindsurf.ts to accept the error (e.g., catch (err)), log or
display the error (using the module's logger or vscode.window.showErrorMessage)
including the filename and error.message, and only then fall back to resetting
existing to { mcpServers: {} } so the malformed-config details are not lost;
reference the existing variable name existing and the surrounding try/catch that
reads/parses mcp.json to locate where to make this change.

---

81-86: ⚠️ Potential issue | 🔴 Critical

Set MCP_WC_PROJECT_ROOT in the generated helixir server entry.

Line 85 currently writes an empty env. The generated config should include the workspace root so the MCP server resolves project assets consistently.

🐛 Proposed fix

     const { dirName, label } = resolveEditorConfig();

     // Resolve the base directory (workspace root or home directory).
     const baseDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? os.homedir();
+    const workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath;

@@
     existing.mcpServers['helixir'] = {
       command: 'node',
       args: [serverScriptPath],
-      env: {},
+      env: workspaceRoot ? { MCP_WC_PROJECT_ROOT: workspaceRoot } : {},
     };

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/commands/configureCursorWindsurf.ts` around lines 81 -
86, The generated helixir MCP server entry currently sets env: {} — update the
upsert at existing.mcpServers['helixir'] to include MCP_WC_PROJECT_ROOT pointing
to the workspace root variable used earlier (the same value you used to compute
serverScriptPath/workspaceRoot), e.g. set env: { MCP_WC_PROJECT_ROOT:
<workspaceRoot> } so the MCP server can resolve project assets; modify the block
that assigns existing.mcpServers['helixir'] (near serverScriptPath usage) to
include this env key.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@packages/vscode/README.md`:
- Around line 26-34: The README's example uses the legacy filename
"mcpwc.config.json" instead of the current "helixir.mcp.json"; update all
occurrences in the file (including the example under "Optional: Configure the
Config Path" and the other referenced lines) to use "helixir.mcp.json" and
ensure the setting key remains "helixir.configPath" so users are pointed to the
correct config file referenced by src/mcp/index.ts.

In `@packages/vscode/src/commands/configureCursorWindsurf.ts`:
- Around line 71-74: The parsed mcp.json may have a non-object mcpServers and
later reuse of parsed.mcpServers (used to build existing.mcpServers and then
upsert) can crash; update the parsing block in configureCursorWindsurf.ts to
validate parsed.mcpServers is a plain object (not null, not an array) before
assigning it to existing.mcpServers, and fallback to an empty object if
validation fails so the subsequent upsert using existing.mcpServers is safe.

In `@packages/vscode/src/mcpProvider.ts`:
- Around line 23-34: The extension sets MCP_WC_CONFIG_PATH from
helixir.configPath but the server never reads it; update the core config loader
so packages/core/src/config.ts consumes process.env.MCP_WC_CONFIG_PATH and uses
it when calling readConfigFile (or add an optional parameter to readConfigFile
and thread that value through the server bootstrap), ensuring that when
MCP_WC_CONFIG_PATH is present the resolved path is used instead of the
hardcoded/default config location; reference the MCP_WC_CONFIG_PATH env var, the
helixir.configPath setting, and the readConfigFile function in your change.

In `@tests/handlers/analyzers/mixin-resolver.test.ts`:
- Around line 105-361: Lots of tests repeatedly call
resolveInheritanceChain(...) with the same parameters; extract a small test
helper (e.g., runResolveInheritanceChain or makeChain) that wraps
resolveInheritanceChain(source, path, decl, worktree, maxDepth?) and returns the
chain, then replace the duplicated calls in each spec to call that helper with
appropriate arguments (use defaults for MINIMAL_SOURCE, resolve(WORKTREE,
'src/my-component.ts'), SIMPLE_DECL, WORKTREE and allow overriding
source/decl/path/maxDepth when needed); update tests that assert specific
sources/markers/architecture to call the helper so setup is centralized and
reduces duplication.

In `@tests/handlers/analyzers/type-coverage.test.ts`:
- Around line 298-312: The tests for analyzeTypeCoverage using PARTIAL_TYPED
currently use vague range assertions and ambiguous comments about rounding;
update the two cases for subMetrics named 'Event typed payloads' and 'Method
return types' to assert the exact rounded scores using JavaScript's Math.round
behavior (i.e., expect eventMetric.score === 18 and methodMetric.score === 13)
and remove the parenthetical “(or 17)/(or 12)” notes so the test and comments
are deterministic; locate these checks by the analyzeTypeCoverage call and the
subMetrics find by name.

In `@tests/tools/bundle.test.ts`:
- Around line 123-129: The tests "accepts optional explicit package name"
(calling handleBundleCall with 'estimate_bundle_size' and package:
'@shoelace-style/shoelace') only assert result.isError; update these tests (both
the one around that call and the similar one later) to assert that the requested
package is actually used by verifying the parsed output or the mock dispatcher
call includes args.package; locate the tests that call
handleBundleCall('estimate_bundle_size', {..., package:
'@shoelace-style/shoelace'}, FAKE_CONFIG) and add an assertion (e.g., against
the returned result payload or the mock function that receives the dispatch)
that the package equals '@shoelace-style/shoelace' so a dispatcher that drops
args.package would fail.

In `@tests/tools/cdn.test.ts`:
- Around line 97-131: Update the tests that call
handleCdnCall('resolve_cdn_cem', ...) so they assert the dispatcher actually
forwards the optional inputs to the handler: mock/spy the actual handler
function that handles the 'resolve_cdn_cem' action (the module/function backing
resolveCdnCem) before calling handleCdnCall, invoke the test case, then add
expect(mockedHandler).toHaveBeenCalledWith(...) (or toHaveBeenCalledTimes +
toHaveBeenCalledWith on the most recent call) asserting the passed-in fields
(version, registry, register, cemPath) appear in the handler call alongside the
FAKE_CONFIG context; do this for each optional-argument test (the four cases)
instead of only checking result.isError.

In `@tests/tools/composition.test.ts`:
- Around line 186-207: Add a test to assert that tagNames cannot contain empty
strings and/or update the Zod schema to reject empty strings: in
tests/tools/composition.test.ts add a case that calls
handleCompositionCall('get_composition_example', { tagNames: ['', 'hx-button']
}, FAKE_CEM) and expects isError to be true; if you prefer schema-side
validation, update the Context Zod schema from z.array(z.string()).min(1).max(4)
to z.array(z.string().min(1)).min(1).max(4) (or equivalent) so empty strings are
invalid, then run tests to ensure the new test passes.
- Around line 217-231: The test function is marked async but
handleCompositionCall is synchronous; remove the unnecessary async by importing
getCompositionExample at top-level (instead of using await inside the test) so
you can mock vi.mocked(getCompositionExample) synchronously, then call
handleCompositionCall('get_composition_example', { tagNames: ['unknown-element']
}, FAKE_CEM) and assert on the result; update the test to reference
getCompositionExample, handleCompositionCall, and FAKE_CEM accordingly.

In `@tests/tools/scaffold.test.ts`:
- Around line 211-220: The test currently only checks success; update it to
assert that the mocked scaffoldComponent actually received the componentPrefix
by spying/mocking the scaffoldComponent used by handleScaffoldCall and adding an
assertion such as
expect(scaffoldComponent).toHaveBeenCalledWith(expect.objectContaining({
componentPrefix: 'hx-' })) (or otherwise inspect the call args) after calling
handleScaffoldCall so the test verifies forwarding of componentPrefix from the
config.

In `@tests/tools/story.test.ts`:
- Around line 181-188: The test "returns error with known components list when
tagName not found" calls handleStoryCall and asserts message content but doesn't
assert the error flag; update the test to also assert result.isError is true by
adding expect(result.isError).toBe(true) after the call (referencing the result
variable returned by handleStoryCall and existing test inputs BUTTON_CEM and
'generate_story') so the error-path contract is explicit.

In `@tests/tools/typegenerate.test.ts`:
- Around line 83-86: The test uses a non-null assertion on
TYPEGENERATE_TOOL_DEFINITIONS.find(...) which can throw if the tool isn't found;
replace the `!` with an explicit existence assertion and then check the schema
(e.g., find the entry into a variable def, assert def is defined via
expect(def).toBeDefined() or use optional chaining like
def?.inputSchema.required), referencing TYPEGENERATE_TOOL_DEFINITIONS and the
'generate_types' entry so the test fails with a clear message rather than a
runtime error.

In `@tests/tools/typescript.test.ts`:
- Around line 57-64: Replace the non-null assertion used when locating tool
definitions so missing definitions produce a clear test failure: instead of
using find(...)! for TYPESCRIPT_TOOL_DEFINITIONS (for tools
'get_file_diagnostics' and 'get_project_diagnostics'), assign the result to a
variable (e.g., def), assert its existence with expect(def).toBeDefined() (or
fail if undefined), then inspect def.inputSchema.required as the existing
assertions do; update both test cases that reference these tool names
accordingly.
- Around line 186-207: Update these tests to not only assert result.isError but
also verify the actual handler for the 'get_file_diagnostics' action was never
invoked: create a spy/mock for the underlying diagnostics handler (the function
looked up when handleTypeScriptCall('get_file_diagnostics', ...) dispatches —
e.g., the exported getFileDiagnostics or the handlers map entry for
'get_file_diagnostics'), call handleTypeScriptCall with the invalid inputs,
assert result.isError is true and optionally assert result.error.message
contains a validation message, and assert handlerSpy.not.toHaveBeenCalled() to
prove short-circuiting.

---

Duplicate comments:
In @.github/workflows/publish-vscode.yml:
- Around line 46-52: The publish steps run at repo root and pick up the wrong
manifest; update the two steps named "Publish to VS Code Marketplace" and
"Publish to Open VSX" so they execute from the extension package directory
(packages/vscode) — e.g., set working-directory to packages/vscode or prepend a
cd packages/vscode && to the existing run commands for npx `@vscode/vsce` publish
and npx ovsx publish so the tool reads the correct extension manifest and uses
the existing VSCE_PAT/OVSX_PAT env vars.

In `@packages/vscode/README.md`:
- Around line 39-44: The README Commands table is missing the second user-facing
command registered in packages/vscode/src/extension.ts; add a new table row
whose Command column matches the command label/string used in the extension
registration (the registerCommand call in packages/vscode/src/extension.ts) and
whose Description column briefly states that it opens the guided Cursor/Windsurf
setup (e.g., "Helixir: Setup Cursor/Windsurf" — "Guides you through
Cursor/Windsurf setup"). Ensure the command label exactly matches the string
passed to registerCommand so users can discover it.

In `@packages/vscode/src/commands/configureCursorWindsurf.ts`:
- Around line 75-78: The catch block that currently swallows JSON parse errors
when reading mcp.json (resetting existing = { mcpServers: {} }) should instead
capture the thrown error and surface it for debugging; update the catch in
configureCursorWindsurf.ts to accept the error (e.g., catch (err)), log or
display the error (using the module's logger or vscode.window.showErrorMessage)
including the filename and error.message, and only then fall back to resetting
existing to { mcpServers: {} } so the malformed-config details are not lost;
reference the existing variable name existing and the surrounding try/catch that
reads/parses mcp.json to locate where to make this change.
- Around line 81-86: The generated helixir MCP server entry currently sets env:
{} — update the upsert at existing.mcpServers['helixir'] to include
MCP_WC_PROJECT_ROOT pointing to the workspace root variable used earlier (the
same value you used to compute serverScriptPath/workspaceRoot), e.g. set env: {
MCP_WC_PROJECT_ROOT: <workspaceRoot> } so the MCP server can resolve project
assets; modify the block that assigns existing.mcpServers['helixir'] (near
serverScriptPath usage) to include this env key.

In `@packages/vscode/src/mcpProvider.ts`:
- Line 21: The code currently falls back to process.cwd() for workspaceFolder
which can point to an unrelated directory; remove the fallback so
workspaceFolder is set to vscode.workspace.workspaceFolders?.[0]?.uri.fsPath (or
undefined) and update the provider entrypoint (the method that uses
workspaceFolder — e.g., the definition/provider function that reads
custom-elements.json) to return no definitions/early-exit when workspaceFolder
is undefined. In short: stop using process.cwd(), treat workspaceFolder as
optional, and add an early return that yields empty results until a workspace
folder exists.

In `@tests/handlers/analyzers/mixin-resolver.test.ts`:
- Around line 19-23: Remove the unused type imports to satisfy linting: edit the
import that currently brings in resolveInheritanceChain, ResolvedSource, and
InheritanceChainResult from mixin-resolver and drop ResolvedSource and
InheritanceChainResult so only resolveInheritanceChain is imported; this keeps
the resolveInheritanceChain symbol while eliminating the unused ResolvedSource
and InheritanceChainResult type imports.
- Around line 72-82: The fixture constant MIXIN_IMPORT_SOURCE is unused and
causing a lint failure; either delete the constant declaration or add a test
that imports and asserts behavior using MIXIN_IMPORT_SOURCE. Locate the constant
named MIXIN_IMPORT_SOURCE in the test file and remove its declaration if it's
not needed, or create a new test (or update an existing one) that references
MIXIN_IMPORT_SOURCE (e.g., passing it into the analyzer under test) so the
symbol is used.
- Around line 28-29: The test currently defines a hardcoded machine-specific
WORKTREE constant which breaks CI; replace it with a portable PROJECT_ROOT
derived at runtime (e.g., using path and __dirname to resolve the
repository/project root) and update every usage of WORKTREE in this test to use
PROJECT_ROOT instead; locate the WORKTREE constant and all references in
mixin-resolver.test.ts and swap them to the new PROJECT_ROOT derivation so the
path is computed relative to the repo rather than hardcoded.

In `@tests/handlers/analyzers/source-accessibility.test.ts`:
- Line 24: Remove the unused "resolve" import from the top of
tests/handlers/analyzers/source-accessibility.test.ts (the import line "import {
resolve } from 'node:path'"); delete that import statement so the file no longer
references the unused symbol and re-run lint/tests to confirm the lint error is
resolved.
- Around line 84-95: The two tests both assert screenReaderSupport using the
same SCREEN_READER_SOURCE fixture, so duplicate coverage doesn't prove .sr-only
is detected independently; modify the second test to use a minimal fixture that
isolates the .sr-only case (e.g., a small component markup containing only a
visually hidden element and an exposed text node) and assert screenReaderSupport
against that fixture instead of SCREEN_READER_SOURCE; update the test
referencing SCREEN_READER_SOURCE (and the similar block around lines 203-211) to
use the new isolated fixture name and keep the same assertion logic for
screenReaderSupport to validate .sr-only detection specifically.
- Around line 449-470: Replace the hard-coded, machine-specific WORKTREE with a
deterministic test fixture/project root (e.g., a path under the test/fixtures or
a tmp directory created for tests) so the test is CI-portable, and change the
".js → .ts/.js" case that currently uses a conditional assertion to assert the
resolver returned a non-null value explicitly (call
resolveComponentSourceFilePath(WORKTREE, 'packages/core/src/config.js'), expect
result not to be null) and then assert result endsWith('.ts') ||
result.endsWith('.js'); reference resolveComponentSourceFilePath to locate the
logic to test and update the test case accordingly.
- Around line 438-446: The test currently expects a non-interactive result but
passes because the event-name regex performs a substring match (e.g.,
'visibility-change' matches 'change'); update the event matching in the analyzer
(the regex used by isInteractiveComponent / the INTERACTIVE_EVENT regex in
source-accessibility.ts) to only match whole event names (e.g., use
^(click|press|select|change|input|submit)$ or word-boundary logic that prevents
hyphenated substrings) and then change the test assertion in
tests/handlers/analyzers/source-accessibility.test.ts to expect false for the
decl with events ['resize','visibility-change'] so the test name "returns false
when events are non-interactive" aligns with the behavior.

In `@tests/handlers/analyzers/type-coverage.test.ts`:
- Around line 133-137: The test name is misleading: update the test title so it
matches the assertion that analyzeTypeCoverage(METHODS_ONLY) should not be null;
locate the spec in tests/handlers/analyzers/type-coverage.test.ts (the it(...)
block around the call to analyzeTypeCoverage and METHODS_ONLY) and change the
string from "returns null when only methods exist but no fields or events" to
something like "returns non-null when only methods exist but no fields or
events" (or equivalent wording) so the test name aligns with the comment and
expect(result).not.toBeNull().
- Around line 219-228: The test for analyzeTypeCoverage using BARE_EVENT_TYPE
currently only asserts presence of eventMetric and has a confusing comment;
update the test to assert the actual metric value (e.g., check eventMetric.score
or eventMetric.counts to confirm bare "Event" was treated as untyped and
excluded from typed payloads) and simplify the comment to state the expected
numeric outcome for the scenario. Locate the test block referencing
analyzeTypeCoverage(BARE_EVENT_TYPE) and the eventMetric lookup, replace or
extend the expect(eventMetric).toBeDefined() with a precise expectation on the
metric's score/count that matches the intended behavior, and remove the
contradictory lines in the comment so the test documents the expected numeric
result.

In `@tests/tools/extend.test.ts`:
- Around line 219-220: The test "returns error when handler throws (e.g. parent
not found in CEM)" mixes async/dynamic import with a synchronous path; make it
synchronous by removing the async from the it callback and replacing the dynamic
await import of extend.js with a synchronous require/static import so you use
the same import style as the rest of the suite (locate the const {
extendComponent } usage in this test and change import style accordingly),
ensuring handleExtendCall (the synchronous path) remains unchanged and no awaits
are left in the test body.

In `@tests/tools/story.test.ts`:
- Around line 165-168: The test "returns error when tagName is missing" only
asserts result.isError and should also assert the validation message; update the
test that calls handleStoryCall('generate_story', {}, BUTTON_CEM) to
additionally check the error message on the returned object (e.g., assert
result.error?.message or result.message contains a string indicating tagName is
required or mentions "tagName") so the schema-validation behavior is verified;
locate the test block in tests/tools/story.test.ts and add a single assertion
referencing result.error or result.message to ensure the failure is caused by
the missing tagName.
- Around line 125-126: Replace loose boolean assertions using result.isError
with an explicit equality check: change expect(result.isError).toBeFalsy() to
expect(result.isError).toBe(false) in the test(s) where the success case is
being asserted (both occurrences referencing result.isError in the story tests),
ensuring symmetry with the error-path checks; update both places and run the
test suite to confirm behavior.

In `@tests/tools/theme.test.ts`:
- Around line 122-129: The test expects extra properties to be rejected but
CreateThemeArgsSchema is currently permissive; update the Zod schema
(CreateThemeArgsSchema in theme.ts) to be strict by calling .strict() on the
z.object(...) (so parsing/validation will fail on unknownProp) and ensure any
helper that uses CreateThemeArgsSchema (e.g., the handler that calls
parse/safeParse) continues to use the same schema for validation; this aligns
the schema behavior with the test that asserts result.isError for unknown
properties.

In `@tests/tools/tokens.test.ts`:
- Around line 17-32: The mocked responses for getDesignTokens and findToken
hard-code count values which can drift from the filtered tokens; update both
mocks (getDesignTokens and findToken) to compute the filtered tokens into a
local variable (e.g., filteredTokens) and set count to filteredTokens.length,
returning filteredTokens as the tokens array and keeping other fields
(categories/query) unchanged so the count always matches the returned tokens.

In `@tests/tools/validate.test.ts`:
- Around line 152-154: The tests build HTML strings with tag wrappers but use
hard-coded repeat counts that ignore the wrapper length, causing both "≤50,000
passes" and ">50,000 fails" assertions to be invalid; update the tests that
create longHtml and the over-limit case (the variables used in the 'accepts html
up to 50000 characters' and corresponding failing test that call
handleValidateCall) to compute the repeat count as 50_000 minus the wrapper
length (e.g., 50_000 - '<hx-button>'.length - '</hx-button>'.length) for the
boundary pass case, and use that value + 1 for the failing case so the total
string lengths correctly hit 50,000 and 50,001 respectively.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 66facc9b-2f33-4e12-a9bc-c78a4d3a007e

📥 Commits

Reviewing files that changed from the base of the PR and between 7325d29 and 4560b3d.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (23)

• .github/workflows/publish-vscode.yml
• package.json
• packages/vscode/README.md
• packages/vscode/src/commands/configureCursorWindsurf.ts
• packages/vscode/src/extension.ts
• packages/vscode/src/mcpProvider.ts
• src/mcp/index.ts
• tests/handlers/analyzers/event-architecture.test.ts
• tests/handlers/analyzers/mixin-resolver.test.ts
• tests/handlers/analyzers/source-accessibility.test.ts
• tests/handlers/analyzers/type-coverage.test.ts
• tests/tools/bundle.test.ts
• tests/tools/cdn.test.ts
• tests/tools/composition.test.ts
• tests/tools/extend.test.ts
• tests/tools/scaffold.test.ts
• tests/tools/story.test.ts
• tests/tools/styling.test.ts
• tests/tools/theme.test.ts
• tests/tools/tokens.test.ts
• tests/tools/typegenerate.test.ts
• tests/tools/typescript.test.ts
• tests/tools/validate.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use helixir.mcp.json here instead of the legacy filename.

src/mcp/index.ts already tells users to configure helixir.mcp.json, but this README still points them at mcpwc.config.json. That sends new users to the wrong file name before they even start.

📝 Proposed fix

-If your `mcpwc.config.json` is not at the workspace root, set the path via VS Code settings:
+If your `helixir.mcp.json` is not at the workspace root, set the path via VS Code settings:

 {
-  "helixir.configPath": "packages/web-components/mcpwc.config.json"
+  "helixir.configPath": "packages/web-components/helixir.mcp.json"
 }

-| `helixir.configPath` | `string` | `""`    | Path to `mcpwc.config.json`. Empty = workspace root. |
+| `helixir.configPath` | `string` | `""`    | Path to `helixir.mcp.json`. Empty = workspace root. |

-Additional configuration (token path, component prefix, health history dir) belongs in `mcpwc.config.json`.
+Additional configuration (token path, component prefix, health history dir) belongs in `helixir.mcp.json`.

Also applies to: 47-49, 66-66

🤖 Prompt for AI Agents

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

In `@packages/vscode/README.md` around lines 26 - 34, The README's example uses
the legacy filename "mcpwc.config.json" instead of the current
"helixir.mcp.json"; update all occurrences in the file (including the example
under "Optional: Configure the Config Path" and the other referenced lines) to
use "helixir.mcp.json" and ensure the setting key remains "helixir.configPath"
so users are pointed to the correct config file referenced by src/mcp/index.ts.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Validate mcpServers runtime shape before reuse.

If mcp.json contains a non-object mcpServers value, the later upsert at Line 82 can fail at runtime and abort the command.

♻️ Proposed fix

       try {
         const raw = fs.readFileSync(configFilePath, 'utf8');
         const parsed = JSON.parse(raw) as Partial<McpJson>;
+        const parsedServers = parsed.mcpServers;
         existing = {
-          mcpServers: parsed.mcpServers ?? {},
+          mcpServers:
+            typeof parsedServers === 'object' && parsedServers !== null && !Array.isArray(parsedServers)
+              ? (parsedServers as Record<string, McpServerEntry>)
+              : {},
         };
       } catch {

📝 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

	const parsed = JSON.parse(raw) as Partial<McpJson>;
	existing = {
	mcpServers: parsed.mcpServers ?? {},
	};
	const parsed = JSON.parse(raw) as Partial<McpJson>;
	const parsedServers = parsed.mcpServers;
	existing = {
	mcpServers:
	typeof parsedServers === 'object' && parsedServers !== null && !Array.isArray(parsedServers)
	? (parsedServers as Record<string, McpServerEntry>)
	: {},
	};

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/commands/configureCursorWindsurf.ts` around lines 71 -
74, The parsed mcp.json may have a non-object mcpServers and later reuse of
parsed.mcpServers (used to build existing.mcpServers and then upsert) can crash;
update the parsing block in configureCursorWindsurf.ts to validate
parsed.mcpServers is a plain object (not null, not an array) before assigning it
to existing.mcpServers, and fallback to an empty object if validation fails so
the subsequent upsert using existing.mcpServers is safe.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

helixir.configPath doesn't affect the server yet.

This provider sets MCP_WC_CONFIG_PATH, but packages/core/src/config.ts never reads that env var or passes a custom path into readConfigFile(). Right now the new setting and README instructions won't change which config file the spawned server loads.

🤖 Prompt for AI Agents

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

In `@packages/vscode/src/mcpProvider.ts` around lines 23 - 34, The extension sets
MCP_WC_CONFIG_PATH from helixir.configPath but the server never reads it; update
the core config loader so packages/core/src/config.ts consumes
process.env.MCP_WC_CONFIG_PATH and uses it when calling readConfigFile (or add
an optional parameter to readConfigFile and thread that value through the server
bootstrap), ensuring that when MCP_WC_CONFIG_PATH is present the resolved path
is used instead of the hardcoded/default config location; reference the
MCP_WC_CONFIG_PATH env var, the helixir.configPath setting, and the
readConfigFile function in your change.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider extracting a small test helper to reduce repeated setup.

Most tests repeat the same resolveInheritanceChain(...) invocation shape; a helper would improve readability and reduce maintenance churn.

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/mixin-resolver.test.ts` around lines 105 - 361, Lots
of tests repeatedly call resolveInheritanceChain(...) with the same parameters;
extract a small test helper (e.g., runResolveInheritanceChain or makeChain) that
wraps resolveInheritanceChain(source, path, decl, worktree, maxDepth?) and
returns the chain, then replace the duplicated calls in each spec to call that
helper with appropriate arguments (use defaults for MINIMAL_SOURCE,
resolve(WORKTREE, 'src/my-component.ts'), SIMPLE_DECL, WORKTREE and allow
overriding source/decl/path/maxDepth when needed); update tests that assert
specific sources/markers/architecture to call the helper so setup is centralized
and reduces duplication.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Misleading rounding comments.

The comments suggest uncertainty about rounding (= 18 (or 17), = 13 (or 12)), but Math.round() behavior for .5 values is deterministic in JavaScript (rounds to nearest even, which for 17.5 → 18 and 12.5 → 13). Consider either asserting exact values or removing the ambiguous parenthetical notes.

Suggested clarification

     it('scores event typed payloads at 50% for half-typed events', () => {
       const result = analyzeTypeCoverage(PARTIAL_TYPED);
       const eventMetric = result!.subMetrics.find((m) => m.name === 'Event typed payloads');
-      // 1 of 2 events has proper type → round(1/2 * 35) = 18 (or 17)
+      // 1 of 2 events has proper type → round(1/2 * 35) = 18
       expect(eventMetric!.score).toBeGreaterThan(0);
       expect(eventMetric!.score).toBeLessThan(35);
     });

     it('scores method return types at 50% for half-typed methods', () => {
       const result = analyzeTypeCoverage(PARTIAL_TYPED);
       const methodMetric = result!.subMetrics.find((m) => m.name === 'Method return types');
-      // 1 of 2 methods has return type → round(1/2 * 25) = 13 (or 12)
+      // 1 of 2 methods has return type → round(1/2 * 25) = 13
       expect(methodMetric!.score).toBeGreaterThan(0);
       expect(methodMetric!.score).toBeLessThan(25);
     });

🤖 Prompt for AI Agents

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

In `@tests/handlers/analyzers/type-coverage.test.ts` around lines 298 - 312, The
tests for analyzeTypeCoverage using PARTIAL_TYPED currently use vague range
assertions and ambiguous comments about rounding; update the two cases for
subMetrics named 'Event typed payloads' and 'Method return types' to assert the
exact rounded scores using JavaScript's Math.round behavior (i.e., expect
eventMetric.score === 18 and methodMetric.score === 13) and remove the
parenthetical “(or 17)/(or 12)” notes so the test and comments are
deterministic; locate these checks by the analyzeTypeCoverage call and the
subMetrics find by name.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

componentPrefix test is currently a false positive.

The test name claims forwarding verification, but it only checks success status. Please assert the mocked scaffoldComponent call actually receives componentPrefix: 'hx-'.

♻️ Proposed fix

+import { scaffoldComponent } from '../../packages/core/src/handlers/scaffold.js';
@@
   it('passes componentPrefix from config to scaffoldComponent', () => {
+    vi.clearAllMocks();
     const configWithPrefix: McpWcConfig = { ...FAKE_CONFIG, componentPrefix: 'hx-' };
     const result = handleScaffoldCall(
       'scaffold_component',
       { tagName: 'hx-badge' },
       configWithPrefix,
       HX_CEM,
     );
     expect(result.isError).toBeFalsy();
+    const [firstCall] = vi.mocked(scaffoldComponent).mock.calls;
+    expect(firstCall).toBeDefined();
+    expect(
+      firstCall?.some(
+        (arg) =>
+          typeof arg === 'object' &&
+          arg !== null &&
+          (arg as { componentPrefix?: string }).componentPrefix === 'hx-',
+      ),
+    ).toBe(true);
   });

🤖 Prompt for AI Agents

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

In `@tests/tools/scaffold.test.ts` around lines 211 - 220, The test currently only
checks success; update it to assert that the mocked scaffoldComponent actually
received the componentPrefix by spying/mocking the scaffoldComponent used by
handleScaffoldCall and adding an assertion such as
expect(scaffoldComponent).toHaveBeenCalledWith(expect.objectContaining({
componentPrefix: 'hx-' })) (or otherwise inspect the call args) after calling
handleScaffoldCall so the test verifies forwarding of componentPrefix from the
config.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

This error-path test should also assert isError === true.

On Line 181-188, the test validates message content but not the error flag. Add expect(result.isError).toBe(true) to make the failure-mode contract explicit.

♻️ Suggested fix

   it('returns error with known components list when tagName not found', async () => {
     const result = await handleStoryCall(
       'generate_story',
       { tagName: 'missing-component' },
       BUTTON_CEM,
     );
+    expect(result.isError).toBe(true);
     expect(result.content[0].text).toContain('hx-button');
   });

📝 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

	it('returns error with known components list when tagName not found', async () => {
	const result = await handleStoryCall(
	'generate_story',
	{ tagName: 'missing-component' },
	BUTTON_CEM,
	);
	expect(result.content[0].text).toContain('hx-button');
	});
	it('returns error with known components list when tagName not found', async () => {
	const result = await handleStoryCall(
	'generate_story',
	{ tagName: 'missing-component' },
	BUTTON_CEM,
	);
	expect(result.isError).toBe(true);
	expect(result.content[0].text).toContain('hx-button');
	});

🤖 Prompt for AI Agents

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

In `@tests/tools/story.test.ts` around lines 181 - 188, The test "returns error
with known components list when tagName not found" calls handleStoryCall and
asserts message content but doesn't assert the error flag; update the test to
also assert result.isError is true by adding expect(result.isError).toBe(true)
after the call (referencing the result variable returned by handleStoryCall and
existing test inputs BUTTON_CEM and 'generate_story') so the error-path contract
is explicit.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Avoid non-null assertion; assert existence explicitly.

The non-null assertion (!) on line 84 will cause a confusing runtime error if find() returns undefined. Prefer an explicit existence check for clearer test failure messages.

🔧 Proposed fix

   it('generate_types schema has no required fields', () => {
     const def = TYPEGENERATE_TOOL_DEFINITIONS.find((t) => t.name === 'generate_types')!;
+    expect(def).toBeDefined();
     expect(def.inputSchema.required).toBeUndefined();
   });

Or use optional chaining if you want a single assertion:

expect(def?.inputSchema.required).toBeUndefined();

🤖 Prompt for AI Agents

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

In `@tests/tools/typegenerate.test.ts` around lines 83 - 86, The test uses a
non-null assertion on TYPEGENERATE_TOOL_DEFINITIONS.find(...) which can throw if
the tool isn't found; replace the `!` with an explicit existence assertion and
then check the schema (e.g., find the entry into a variable def, assert def is
defined via expect(def).toBeDefined() or use optional chaining like
def?.inputSchema.required), referencing TYPEGENERATE_TOOL_DEFINITIONS and the
'generate_types' entry so the test fails with a clear message rather than a
runtime error.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Avoid non-null assertions in test lookup to improve failure diagnostics.

If a definition is missing, find(...)! can fail with a less useful runtime error before the intent is clear. Assert existence first, then inspect schema fields.

Suggested diff

   it('get_file_diagnostics schema requires filePath', () => {
-    const def = TYPESCRIPT_TOOL_DEFINITIONS.find((t) => t.name === 'get_file_diagnostics')!;
-    expect(def.inputSchema.required).toContain('filePath');
+    const def = TYPESCRIPT_TOOL_DEFINITIONS.find((t) => t.name === 'get_file_diagnostics');
+    expect(def).toBeDefined();
+    expect(def!.inputSchema.required).toContain('filePath');
   });

   it('get_project_diagnostics schema has no required fields', () => {
-    const def = TYPESCRIPT_TOOL_DEFINITIONS.find((t) => t.name === 'get_project_diagnostics')!;
-    expect(def.inputSchema.required).toBeUndefined();
+    const def = TYPESCRIPT_TOOL_DEFINITIONS.find((t) => t.name === 'get_project_diagnostics');
+    expect(def).toBeDefined();
+    expect(def!.inputSchema.required).toBeUndefined();
   });

🤖 Prompt for AI Agents

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

In `@tests/tools/typescript.test.ts` around lines 57 - 64, Replace the non-null
assertion used when locating tool definitions so missing definitions produce a
clear test failure: instead of using find(...)! for TYPESCRIPT_TOOL_DEFINITIONS
(for tools 'get_file_diagnostics' and 'get_project_diagnostics'), assign the
result to a variable (e.g., def), assert its existence with
expect(def).toBeDefined() (or fail if undefined), then inspect
def.inputSchema.required as the existing assertions do; update both test cases
that reference these tool names accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Validation-path tests are too permissive and can miss regressions.

These tests only check isError, so unrelated failures would still pass. Strengthen them by proving handler short-circuit on invalid input (no handler call).

Suggested diff

 import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { getFileDiagnostics } from '../../packages/core/src/handlers/typescript.js';
@@
 describe('handleTypeScriptCall — error cases', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
   it('returns error when filePath is missing for get_file_diagnostics', () => {
     const result = handleTypeScriptCall('get_file_diagnostics', {}, FAKE_CONFIG);
     expect(result.isError).toBe(true);
+    expect(vi.mocked(getFileDiagnostics)).not.toHaveBeenCalled();
   });
@@
   it('returns error for absolute filePath (path traversal)', () => {
@@
     expect(result.isError).toBe(true);
+    expect(vi.mocked(getFileDiagnostics)).not.toHaveBeenCalled();
   });
@@
   it('returns error for path traversal attempt in filePath', () => {
@@
     expect(result.isError).toBe(true);
+    expect(vi.mocked(getFileDiagnostics)).not.toHaveBeenCalled();
   });
 });

🤖 Prompt for AI Agents

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

In `@tests/tools/typescript.test.ts` around lines 186 - 207, Update these tests to
not only assert result.isError but also verify the actual handler for the
'get_file_diagnostics' action was never invoked: create a spy/mock for the
underlying diagnostics handler (the function looked up when
handleTypeScriptCall('get_file_diagnostics', ...) dispatches — e.g., the
exported getFileDiagnostics or the handlers map entry for
'get_file_diagnostics'), call handleTypeScriptCall with the invalid inputs,
assert result.isError is true and optionally assert result.error.message
contains a validation message, and assert handlerSpy.not.toHaveBeenCalled() to
prove short-circuiting.