chore: update Node.js version requirements and bug fixes#314
Merged
frontegg-david merged 12 commits intorelease/1.0.xfrom Mar 28, 2026
Merged
chore: update Node.js version requirements and bug fixes#314frontegg-david merged 12 commits intorelease/1.0.xfrom
frontegg-david merged 12 commits intorelease/1.0.xfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps Node.js minimum to 24+ across manifests, templates and docs; upgrades multiple GitHub Actions major versions; introduces skills manifest generation and catalog updates; adds multi-select CLI skills install and CodeCall app scoping; changes OpenAPI adapter duplicate-name detection; awaits plugin readiness before registry construction; various test and HTTP default tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Scope as Scope
participant ScopePlugins as ScopePlugins
participant Registries as Registries (Tool/Resource/Prompt/Agent/Skill)
Note over Scope,Registries: New sequential initialization (plugins awaited before registries)
Scope->>ScopePlugins: create scopePlugins
Scope->>ScopePlugins: await scopePlugins.ready
Scope->>Registries: construct ToolRegistry, ResourceRegistry, PromptRegistry, AgentRegistry, SkillRegistry
Registries-->>Scope: registries ready
sequenceDiagram
autonumber
participant Scope as Scope
participant ScopePlugins as ScopePlugins
participant Registries as Registries (Tool/Resource/Prompt/Agent/Skill)
Note over Scope,Registries: Previous parallel initialization (plugins awaited in Promise.all with registries)
Scope->>ScopePlugins: create scopePlugins
Scope->>Registries: Promise.all([..., scopePlugins.ready])
Registries-->>Scope: registries ready (concurrently with plugins)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/cli/src/commands/scaffold/create.ts (1)
637-646:⚠️ Potential issue | 🟠 MajorUpdate
PM_CONFIGengines to Node 24 for consistency with SAM template.
TEMPLATE_SAM_YAMLspecifiesRuntime: nodejs24.x, butPM_CONFIGstill declaresengines.node: '>=22'across all package managers (lines 86, 96, 106). New projects will have mismatched Node baselines between the serverless template andpackage.json.Update package manager configurations
const PM_CONFIG: Record<PackageManager, PmConfig> = { npm: { - engines: { node: '>=22', npm: '>=10' }, + engines: { node: '>=24', npm: '>=10' }, }, yarn: { - engines: { node: '>=22' }, + engines: { node: '>=24' }, }, pnpm: { - engines: { node: '>=22' }, + engines: { node: '>=24' }, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/scaffold/create.ts` around lines 637 - 646, TEMPLATE_SAM_YAML sets Runtime: nodejs24.x but PM_CONFIG still requires Node >=22, causing a mismatch; update the PM_CONFIG constant in this file (refer to the PM_CONFIG symbol used for package.json/pnpm/npm/yarn templates) so that engines.node is '>=24' (or an equivalent Node 24-compatible semver) for all package manager templates, ensuring consistency with TEMPLATE_SAM_YAML and any generated package.json files.README.md (1)
42-50:⚠️ Potential issue | 🟡 MinorAlign README badge and install command with the new Node 24 baseline.
After Line 42 was updated, Lines 13 and 49 still point to Node 22, which is now inconsistent.📌 Suggested doc fix
-[](https://nodejs.org) +[](https://nodejs.org) @@ -npm i -D frontmcp `@types/node`@^22 +npm i -D frontmcp `@types/node`@^24🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 42 - 50, Update the README to consistently reflect the new Node 24 baseline: change any badge or text that still says "Node 22" to "Node 24" and update the install command in the snippet (the "@types/node@^22" token) to "@types/node@^24" (or remove the `@types` package if you no longer require it); ensure the header/badge and the install snippet strings (e.g., the "Node.js 24+" badge and the "@types/node@^22" install token) are all changed so the doc consistently references Node 24..github/workflows/publish-release.yml (1)
199-205:⚠️ Potential issue | 🟠 MajorSame version mismatch:
upload-artifact@v6paired withdownload-artifact@v5.As noted in
perf-release.yml, thepreparejob uploads artifacts withactions/upload-artifact@v6(line 199), but thepublishjob downloads them withactions/download-artifact@v5(line 249). This inconsistency could cause artifact retrieval failures if the v6 artifact format is incompatible with v5's download logic.🔧 Suggested fix: Align versions
Either upgrade both to v6:
- name: Download build artifacts - uses: actions/download-artifact@v5 + uses: actions/download-artifact@v6 with: name: dist-packagesOr keep both at v5 until v6 compatibility is confirmed.
Also applies to: 248-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-release.yml around lines 199 - 205, The artifact actions use mismatched versions—upload uses actions/upload-artifact@v6 while download uses actions/download-artifact@v5—so update the workflow to use the same action version for both upload and download; locate the upload step referencing actions/upload-artifact@v6 and the download step referencing actions/download-artifact@v5 and change one so both use the same stable version (either both `@v6` or both `@v5`) to ensure artifact compatibility.
🧹 Nitpick comments (1)
.github/workflows/push.yml (1)
32-33: Pin workflow actions to immutable commit SHAs.All external actions in this workflow currently use mutable version tags (
@v5,@v6). Pin them to specific commit SHAs instead to reduce supply-chain risk and prevent unexpected breaking changes from action updates.This applies to all instances of:
actions/checkout@v6actions/setup-node@v6actions/upload-artifact@v6actions/download-artifact@v5nrwl/nx-set-shas@v5codecov/codecov-action@v5At lines: 32, 47, 52, 64, 82, 87, 99, 119, 138, 141, 171, 176, 185, 193, 205, 227, 232, 241, 249, 276, 293, 296, 305, 311, 321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 32 - 33, The workflow uses mutable action tags (e.g., actions/checkout@v6, actions/setup-node@v6, actions/upload-artifact@v6, actions/download-artifact@v5, nrwl/nx-set-shas@v5, codecov/codecov-action@v5); update each occurrence to a pinned immutable commit SHA instead of the version tag: locate each step referencing those action identifiers (actions/checkout, actions/setup-node, actions/upload-artifact, actions/download-artifact, nrwl/nx-set-shas, codecov/codecov-action) and replace the `@vX` tag with the corresponding full commit SHA from the action’s repository (use the action repo’s GitHub releases/tags to get the exact commit SHA), ensuring you update all listed instances in the workflow to use the SHA to prevent supply-chain risks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/auth/package.json`:
- Line 48: The engines.node constraint in libs/auth/package.json for the
`@frontmcp/auth` package is set to ">=24.0.0" which is an unnecessary breaking
change; revert or relax it to ">=18.0.0" (or the project-wide policy) unless you
have a documented, intentional reason to require Node 24; update the
engines.node field in libs/auth/package.json accordingly and add a short comment
or changelog note if this is an intentional policy so consumers understand the
decision.
In `@libs/cli/package.json`:
- Line 29: Update the package.json "engines" node constraint to match actual
dependency requirements: change the "node" field value from ">=24.0.0" to
">=18.12.0" (to align with `@rspack/core` and other deps). Locate the "node"
engines entry in package.json and replace the version string, then run
CI/lockfile checks to ensure no other constraints break.
In `@libs/nx-plugin/package.json`:
- Line 33: The workspace generator template package.json__tmpl__ still declares
an older Node engine; update its "engines"."node" value to match the host
package.json ("node": ">=24.0.0") so newly generated workspaces require Node 24;
specifically edit package.json__tmpl__ to replace the existing "node" semver
with ">=24.0.0" (matching libs/nx-plugin/package.json) and regenerate/verify a
sample workspace to confirm the change.
In `@libs/skills/catalog/frontmcp-deployment/references/deploy-to-lambda.md`:
- Line 55: The CDK example is out of sync with the SAM example: change any usage
of lambda.Runtime.NODEJS_22_X in the CDK sample to lambda.Runtime.NODEJS_24_X so
the CDK runtime matches the SAM runtime (nodejs24.x); search for the symbol
lambda.Runtime.NODEJS_22_X in the CDK snippet and replace it with
lambda.Runtime.NODEJS_24_X and verify any accompanying runtime strings or
comments also reference nodejs24.x for consistency.
---
Outside diff comments:
In @.github/workflows/publish-release.yml:
- Around line 199-205: The artifact actions use mismatched versions—upload uses
actions/upload-artifact@v6 while download uses actions/download-artifact@v5—so
update the workflow to use the same action version for both upload and download;
locate the upload step referencing actions/upload-artifact@v6 and the download
step referencing actions/download-artifact@v5 and change one so both use the
same stable version (either both `@v6` or both `@v5`) to ensure artifact
compatibility.
In `@libs/cli/src/commands/scaffold/create.ts`:
- Around line 637-646: TEMPLATE_SAM_YAML sets Runtime: nodejs24.x but PM_CONFIG
still requires Node >=22, causing a mismatch; update the PM_CONFIG constant in
this file (refer to the PM_CONFIG symbol used for package.json/pnpm/npm/yarn
templates) so that engines.node is '>=24' (or an equivalent Node 24-compatible
semver) for all package manager templates, ensuring consistency with
TEMPLATE_SAM_YAML and any generated package.json files.
In `@README.md`:
- Around line 42-50: Update the README to consistently reflect the new Node 24
baseline: change any badge or text that still says "Node 22" to "Node 24" and
update the install command in the snippet (the "@types/node@^22" token) to
"@types/node@^24" (or remove the `@types` package if you no longer require it);
ensure the header/badge and the install snippet strings (e.g., the "Node.js 24+"
badge and the "@types/node@^22" install token) are all changed so the doc
consistently references Node 24.
---
Nitpick comments:
In @.github/workflows/push.yml:
- Around line 32-33: The workflow uses mutable action tags (e.g.,
actions/checkout@v6, actions/setup-node@v6, actions/upload-artifact@v6,
actions/download-artifact@v5, nrwl/nx-set-shas@v5, codecov/codecov-action@v5);
update each occurrence to a pinned immutable commit SHA instead of the version
tag: locate each step referencing those action identifiers (actions/checkout,
actions/setup-node, actions/upload-artifact, actions/download-artifact,
nrwl/nx-set-shas, codecov/codecov-action) and replace the `@vX` tag with the
corresponding full commit SHA from the action’s repository (use the action
repo’s GitHub releases/tags to get the exact commit SHA), ensuring you update
all listed instances in the workflow to use the SHA to prevent supply-chain
risks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b1c1f88-d64b-46d1-ad6f-7b4f0f41b344
⛔ Files ignored due to path filters (1)
libs/cli/src/commands/build/adapters/vercel.tsis excluded by!**/build/**
📒 Files selected for processing (49)
.github/actions/nx-cache-restore/action.yml.github/actions/nx-cache-save/action.yml.github/workflows/cherry-pick-prompt.yml.github/workflows/codeql.yml.github/workflows/create-release-branch.yml.github/workflows/package-e2e.yml.github/workflows/perf-release.yml.github/workflows/perf.yml.github/workflows/pr-testing-registry.yml.github/workflows/publish-release.yml.github/workflows/push.yml.github/workflows/trigger-docs-update.ymlREADME.mdSECURITY.mddocs/frontmcp/deployment/serverless.mdxdocs/frontmcp/getting-started/installation.mdxdocs/frontmcp/getting-started/quickstart.mdxdocs/frontmcp/guides/publishing-esm-packages.mdxdocs/frontmcp/nx-plugin/installation.mdxdocs/frontmcp/sdk-reference/decorators/skill.mdxdocs/frontmcp/ui/getting-started.mdxlibs/adapters/package.jsonlibs/auth/package.jsonlibs/cli/package.jsonlibs/cli/src/commands/scaffold/create.tslibs/di/package.jsonlibs/guard/package.jsonlibs/nx-plugin/package.jsonlibs/nx-plugin/src/generators/server/files/lambda/template.yaml__tmpl__libs/plugins/package.jsonlibs/protocol/package.jsonlibs/react/package.jsonlibs/sdk/package.jsonlibs/skills/README.mdlibs/skills/catalog/frontmcp-config/SKILL.mdlibs/skills/catalog/frontmcp-deployment/references/build-for-cli.mdlibs/skills/catalog/frontmcp-deployment/references/deploy-to-lambda.mdlibs/skills/catalog/frontmcp-deployment/references/deploy-to-node-dockerfile.mdlibs/skills/catalog/frontmcp-deployment/references/deploy-to-node.mdlibs/skills/catalog/frontmcp-development/references/create-skill.mdlibs/skills/catalog/frontmcp-guides/SKILL.mdlibs/skills/catalog/frontmcp-setup/SKILL.mdlibs/skills/package.jsonlibs/storage-sqlite/package.jsonlibs/testing/package.jsonlibs/ui/package.jsonlibs/uipack/package.jsonlibs/utils/package.jsonpackage.json
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-28T17:54:43.035Z |
…ns and documentation
…and AGENTS.md for AI coding assistants
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
libs/testing/src/fixtures/test-fixture.ts (1)
23-40: Explicit@jest/globalsimports improve clarity; type casting approach is acceptable.The migration from ambient Jest globals to explicit
@jest/globalsimports is a good practice for explicitness and avoiding global namespace pollution. The double cast throughunknownis a pragmatic workaround for Jest's type signature differences.One consideration: this pattern creates a coupling between
@jest/globals(for runtime) and@types/jestglobal types (forjest.Describe,jest.Lifecycle,jest.It). If the types ever diverge significantly, runtime behavior could differ from what TypeScript expects.Consider extracting these adapter types explicitly to make the contract clearer:
♻️ Optional: Extract explicit type definitions
import { describe as _describe, beforeAll as _beforeAll, beforeEach as _beforeEach, afterEach as _afterEach, afterAll as _afterAll, it as _it, } from '@jest/globals'; +// Type adapters for `@jest/globals` functions to match global Jest declarations +// This allows TestWithFixtures interface to remain compatible with both import styles +type DescribeFn = typeof _describe; +type LifecycleFn = typeof _beforeAll; +type ItFn = typeof _it; + // Re-export with compatible types (Jest globals may differ in type signatures // between `@jest/globals` and the global declarations used by our TestWithFixtures) -const describe = _describe as unknown as jest.Describe; -const beforeAll = _beforeAll as unknown as jest.Lifecycle; -const beforeEach = _beforeEach as unknown as jest.Lifecycle; -const afterEach = _afterEach as unknown as jest.Lifecycle; -const afterAll = _afterAll as unknown as jest.Lifecycle; -const it = _it as unknown as jest.It; +const describe: DescribeFn = _describe; +const beforeAll: LifecycleFn = _beforeAll; +const beforeEach: LifecycleFn = _beforeEach; +const afterEach: LifecycleFn = _afterEach; +const afterAll: LifecycleFn = _afterAll; +const it: ItFn = _it;This would require updating
TestWithFixturesinterface infixture-types.tsto use the actual@jest/globalstypes rather than globaljest.*types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/fixtures/test-fixture.ts` around lines 23 - 40, The current file imports Jest runtime functions from `@jest/globals` then force-casts them to global jest types (describe, beforeAll, beforeEach, afterEach, afterAll, it), which couples runtime imports to ambient `@types/jest`; extract explicit adapter type aliases (e.g., DescribeAdapter, LifecycleAdapter, ItAdapter) based on the actual `@jest/globals` types and use those aliases for the constants instead of casting through unknown, and update the TestWithFixtures interface in fixture-types.ts to reference these adapter types (or the `@jest/globals` types directly) so the runtime imports and TypeScript types stay aligned and clear (locate the cast lines for describe, beforeAll, beforeEach, afterEach, afterAll, it to apply the change).plugins/plugin-codecall/src/__tests__/codecall.plugin.spec.ts (1)
266-300: Consider extracting shared test helpers to reduce duplication.The
createMockFlowCtx,createPluginWithLogger, andmockLoggersetup are duplicated from the parentadjustListTools hookdescribe block (lines 101-133). Extracting these to a shared scope or a test utility module would improve maintainability.♻️ Suggested refactor to deduplicate helpers
describe('CodeCallPlugin', () => { + // Shared test helpers + let mockLogger: { verbose: jest.Mock; info: jest.Mock; warn: jest.Mock; error: jest.Mock; child: jest.Mock }; + + const createMockFlowCtx = (tools: Array<{ tool: Record<string, unknown> }>) => { + let resolvedTools = tools; + return { + state: { + resolvedTools, + set: (key: string, value: any) => { + if (key === 'resolvedTools') { + resolvedTools = value; + } + }, + }, + get resolvedToolsAfter() { + return resolvedTools; + }, + } as any; + }; + + const createPluginWithLogger = (options: CodeCallPluginOptionsInput = {}) => { + const p = new CodeCallPlugin(options); + jest.spyOn(p, 'get').mockReturnValue({ child: () => mockLogger } as unknown as never); + return p; + }; + + const setupMockLogger = () => { + mockLogger = { + verbose: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + child: jest.fn().mockReturnThis(), + }; + }; + describe('constructor', () => { // ... }); describe('adjustListTools hook', () => { - let plugin: CodeCallPlugin; - let mockLogger: { verbose: jest.Mock; ... }; - const createMockFlowCtx = ... - const createPluginWithLogger = ... + let plugin: CodeCallPlugin; + beforeEach(() => { - mockLogger = { ... }; + setupMockLogger(); plugin = createPluginWithLogger({ mode: 'codecall_only' }); }); // ... }); describe('multi-app scoping with appIds', () => { - let mockLogger: { ... }; - const createMockFlowCtx = ... - const createPluginWithLogger = ... beforeEach(() => { - mockLogger = { ... }; + setupMockLogger(); }); // ... }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-codecall/src/__tests__/codecall.plugin.spec.ts` around lines 266 - 300, The test file duplicates helper setup (createMockFlowCtx, createPluginWithLogger, and mockLogger) already defined in the parent describe; extract these into a shared test helper to remove duplication by moving createMockFlowCtx, createPluginWithLogger, and the mockLogger initialization into a common scope or a separate test utility module and import/consume them in both the parent "adjustListTools hook" and the "multi-app scoping with appIds" describe blocks so both can reuse the same functions and logger; ensure the moved helpers still create a CodeCallPlugin instance (referencing CodeCallPlugin) and that jest.spyOn(p, 'get') mocking behavior is preserved.libs/sdk/src/plugin/__tests__/plugin.registry.spec.ts (1)
306-321: Strengthen this regression with a real tool.These assertions only prove that a
ToolRegistryobject was created. They would still pass if server-level plugin tools stopped being discoverable again, so please add a dummy tool and assert it is visible from the returned registry or, better, from the scope-levelToolRegistry.Also applies to: 323-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/plugin/__tests__/plugin.registry.spec.ts` around lines 306 - 321, Add a real dummy tool to the test so it verifies discovery of server-level tools rather than just existence of a ToolRegistry: update the FrontMcpPlugin-decorated class PluginExposingTools to include a simple tool definition (e.g., a static/dummy tool object in the tools array), then after creating providers with createProviderRegistryWithScope() and instantiating PluginRegistry, assert that registry.getToolRegistries() (or the returned scope-level ToolRegistry) contains that dummy tool by checking the registry length and/or that a tool with the dummy tool's name/id is present; use the existing PluginExposingTools, createProviderRegistryWithScope, PluginRegistry, and getToolRegistries symbols to locate where to modify and where to add the new assertion.libs/cli/src/commands/skills/install.ts (1)
30-34: Avoid the non-null assertion onoptions.tag.This branch already establishes the tag selector. Hoist it into a local
const tag = options.tagand use that through the filter/error path instead ofoptions.tag!.♻️ Suggested cleanup
} else if (options.tag) { - skills = skills.filter((s) => s.tags.includes(options.tag!)); + const tag = options.tag; + skills = skills.filter((s) => s.tags.includes(tag)); if (skills.length === 0) { - console.error(c('red', `No skills found with tag "${options.tag}".`)); + console.error(c('red', `No skills found with tag "${tag}".`)); console.log(c('gray', "Use 'frontmcp skills list --tag <tag>' to see available tags.")); process.exit(1); }As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/skills/install.ts` around lines 30 - 34, Hoist options.tag into a local const (e.g., const tag = options.tag) at the start of the else-if branch and use that local variable in the skills.filter and error/log messages instead of the non-null assertion options.tag!; this removes the non-null assertion, keeps the same behavior in the skills filtering (skills.filter((s) => s.tags.includes(tag))) and in the console output, and follow-up by throwing or returning a specific error if tag is unexpectedly undefined before filtering to satisfy the no-non-null-assertion rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/adapters/src/openapi/openapi.adapter.ts`:
- Around line 148-153: The normalization step that lowercases tool names
(transformedTools.map with tool.name.replace) breaks existing per-tool keyed
lookups (toolTransforms.perTool, inputTransforms.perTool,
schemaTransforms.*.perTool); modify the transform pipeline to (1) persist the
original name on each tool (e.g., set a metadata property originalToolName
before renaming), (2) continue normalizing tool.name to the snake_case
lowercased form, and (3) update all per-tool lookup code to attempt resolution
first by normalized name and then fallback to originalToolName when checking
toolTransforms.perTool, inputTransforms.perTool, and schemaTransforms.*.perTool
so existing keys remain compatible with mixed-case names.
- Around line 148-153: After normalizing tool names in the transformedTools
mapping (the block that replaces uppercase with lowercase via
tool.name.replace), add a collision detection step that collects normalized
names (from transformedTools.map(t => t.name)) and checks for duplicates, then
surface an explicit error or throw that lists the original conflicting tool
names and their normalized name so registration doesn't silently break; update
the code near the transformedTools normalization to compute normalizedName,
detect duplicate normalizedName values, and fail fast with a clear message
referencing transformedTools and the normalization step.
In `@libs/cli/src/commands/skills/install.ts`:
- Around line 64-80: After the install loop finishes, add a check for installed
=== 0 and exit with a non-zero status so the CLI reports failure when nothing
was copied: if no SKILL.md files were installed (installed remains 0) print an
error or warning mentioning the provider/skills and call process.exit(1). Update
the existing post-loop logic that currently only prints the success summary to
first handle the zero-installed case (using installed, skills, provider) and
only print the success messages when installed > 0.
- Around line 25-56: Before filtering the manifest, validate that exactly one
selector is supplied instead of letting options.all silently win; check the four
selectors (options.all, options.tag, options.category, and name) and if the
count of truthy selectors is not exactly one, print a clear error (use
c('red')/c('gray') as in nearby messages) explaining that selectors are mutually
exclusive and call process.exit(1). Add this validation at the top of the
selector logic (before the existing chain that assigns skills and filters by
options.* or name) so the rest of the code can assume a single selector.
In `@libs/sdk/src/common/types/options/__tests__/http.options.spec.ts`:
- Around line 7-9: The test for httpOptionsSchema.parse relies on ambient
process.env.PORT making it flaky; update the test (the spec using
httpOptionsSchema.parse and expect(result.port)) to set process.env.PORT to a
known value at the start of the test, run the assertion against that known value
(or explicitly delete it to assert default 3000), and then restore the original
process.env.PORT in a finally or after block so the environment is not mutated
for other tests.
In `@libs/skills/catalog/frontmcp-testing/SKILL.md`:
- Around line 63-74: Update the two docs (docs/frontmcp/testing/overview.mdx and
docs/frontmcp/guides/testing-your-first-tool.mdx) to match the new testing
patterns: replace references to the deprecated jest.e2e.config.ts and any
examples invoking "jest --config ..." with the single recommended "frontmcp
test" command, change example file names from ".e2e.ts" to ".e2e.spec.ts" (and
note the general ".spec.ts" convention), and add the httpMock scope
clarification from SKILL.md that httpMock only intercepts HTTP in the test
process (not in the MCP server subprocess) so server-to-API calls in the child
process must be mocked differently; ensure the updated docs mirror the bullets
in SKILL.md (file naming, test runner, and httpMock scope).
---
Nitpick comments:
In `@libs/cli/src/commands/skills/install.ts`:
- Around line 30-34: Hoist options.tag into a local const (e.g., const tag =
options.tag) at the start of the else-if branch and use that local variable in
the skills.filter and error/log messages instead of the non-null assertion
options.tag!; this removes the non-null assertion, keeps the same behavior in
the skills filtering (skills.filter((s) => s.tags.includes(tag))) and in the
console output, and follow-up by throwing or returning a specific error if tag
is unexpectedly undefined before filtering to satisfy the no-non-null-assertion
rule.
In `@libs/sdk/src/plugin/__tests__/plugin.registry.spec.ts`:
- Around line 306-321: Add a real dummy tool to the test so it verifies
discovery of server-level tools rather than just existence of a ToolRegistry:
update the FrontMcpPlugin-decorated class PluginExposingTools to include a
simple tool definition (e.g., a static/dummy tool object in the tools array),
then after creating providers with createProviderRegistryWithScope() and
instantiating PluginRegistry, assert that registry.getToolRegistries() (or the
returned scope-level ToolRegistry) contains that dummy tool by checking the
registry length and/or that a tool with the dummy tool's name/id is present; use
the existing PluginExposingTools, createProviderRegistryWithScope,
PluginRegistry, and getToolRegistries symbols to locate where to modify and
where to add the new assertion.
In `@libs/testing/src/fixtures/test-fixture.ts`:
- Around line 23-40: The current file imports Jest runtime functions from
`@jest/globals` then force-casts them to global jest types (describe, beforeAll,
beforeEach, afterEach, afterAll, it), which couples runtime imports to ambient
`@types/jest`; extract explicit adapter type aliases (e.g., DescribeAdapter,
LifecycleAdapter, ItAdapter) based on the actual `@jest/globals` types and use
those aliases for the constants instead of casting through unknown, and update
the TestWithFixtures interface in fixture-types.ts to reference these adapter
types (or the `@jest/globals` types directly) so the runtime imports and
TypeScript types stay aligned and clear (locate the cast lines for describe,
beforeAll, beforeEach, afterEach, afterAll, it to apply the change).
In `@plugins/plugin-codecall/src/__tests__/codecall.plugin.spec.ts`:
- Around line 266-300: The test file duplicates helper setup (createMockFlowCtx,
createPluginWithLogger, and mockLogger) already defined in the parent describe;
extract these into a shared test helper to remove duplication by moving
createMockFlowCtx, createPluginWithLogger, and the mockLogger initialization
into a common scope or a separate test utility module and import/consume them in
both the parent "adjustListTools hook" and the "multi-app scoping with appIds"
describe blocks so both can reuse the same functions and logger; ensure the
moved helpers still create a CodeCallPlugin instance (referencing
CodeCallPlugin) and that jest.spyOn(p, 'get') mocking behavior is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae7d7b84-4dfe-4748-9620-97a2fded395f
📒 Files selected for processing (22)
libs/adapters/src/openapi/openapi.adapter.tslibs/cli/src/commands/scaffold/__tests__/create.spec.tslibs/cli/src/commands/scaffold/create.tslibs/cli/src/commands/skills/install.tslibs/cli/src/commands/skills/register.tslibs/cli/src/core/help.tslibs/sdk/src/common/types/options/__tests__/http.options.spec.tslibs/sdk/src/common/types/options/http/schema.tslibs/sdk/src/front-mcp/front-mcp.providers.tslibs/sdk/src/plugin/__tests__/plugin.registry.spec.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/scope/scope.instance.tslibs/skills/catalog/frontmcp-development/SKILL.mdlibs/skills/catalog/frontmcp-development/references/official-adapters.mdlibs/skills/catalog/frontmcp-development/references/official-plugins.mdlibs/skills/catalog/frontmcp-production-readiness/SKILL.mdlibs/skills/catalog/frontmcp-testing/SKILL.mdlibs/skills/catalog/skills-manifest.jsonlibs/testing/src/fixtures/test-fixture.tsplugins/plugin-codecall/src/__tests__/codecall.plugin.spec.tsplugins/plugin-codecall/src/codecall.plugin.tsplugins/plugin-codecall/src/codecall.types.ts
✅ Files skipped from review due to trivial changes (4)
- libs/skills/catalog/frontmcp-development/SKILL.md
- libs/cli/src/core/help.ts
- libs/skills/catalog/frontmcp-production-readiness/SKILL.md
- libs/skills/catalog/skills-manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/cli/src/commands/scaffold/create.ts
libs/sdk/src/common/types/options/__tests__/http.options.spec.ts
Outdated
Show resolved
Hide resolved
…s and update skill categories
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/common/decorators/skill.decorator.ts (1)
154-156: Stack trace index assumption is fragile.Hardcoding
i = 3assumes a fixed stack structure (Error,resolveCallerDir,frontMcpSkill). This can break with:
- V8 inlining optimizations
- Different call patterns (e.g., if wrapped by another function)
- Source maps altering stack representation
The current skip logic (checking for
skill.decoratorin the path) already handles skipping this file, so starting from index 1 (after "Error") and relying on that filter would be more robust.♻️ Suggested simplification
const lines = stack.split('\n'); - // Skip: "Error", this function, frontMcpSkill — start from index 3 - for (let i = 3; i < lines.length; i++) { + // Skip "Error" line, then rely on path filtering to skip internal frames + for (let i = 1; i < lines.length; i++) { const line = lines[i];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/skill.decorator.ts` around lines 154 - 156, The stack parsing loop assumes a fixed layout by starting at index 3 which is fragile; change the loop to start from index 1 (after the "Error" header) and rely on the existing filter that excludes this file (the check for "skill.decorator" in the path) to skip internal frames. Update the loop initialization that uses the variables stack and lines (and the for-loop that currently starts with i = 3) so it starts with i = 1 and keep the existing filename filter logic (references: stack, lines, and the "skill.decorator" path check) to make caller resolution robust against inlining and varied stack formats.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/skill.decorator.ts`:
- Around line 159-170: The stack-frame parsing in skill.decorator (the block
using const match = line.match(...), reading match[1] into file, and manually
using lastIndexOf('/') to derive a directory) fails on Windows drive-letter
paths and is brittle; replace the manual string-slicing logic by importing and
using dirname from `@frontmcp/utils` to compute the directory from the captured
path (after you still extract the file path via the regex match), and remove the
lastIndexOf('/') logic and string.includes checks that assume POSIX separators
so path handling is cross-platform and robust.
---
Nitpick comments:
In `@libs/sdk/src/common/decorators/skill.decorator.ts`:
- Around line 154-156: The stack parsing loop assumes a fixed layout by starting
at index 3 which is fragile; change the loop to start from index 1 (after the
"Error" header) and rely on the existing filter that excludes this file (the
check for "skill.decorator" in the path) to skip internal frames. Update the
loop initialization that uses the variables stack and lines (and the for-loop
that currently starts with i = 3) so it starts with i = 1 and keep the existing
filename filter logic (references: stack, lines, and the "skill.decorator" path
check) to make caller resolution robust against inlining and varied stack
formats.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3dd1c5e-bbcf-47db-9cc0-c162642e3379
📒 Files selected for processing (10)
libs/sdk/src/common/decorators/skill.decorator.tslibs/sdk/src/common/records/skill.record.tslibs/sdk/src/skill/skill.instance.tslibs/skills/__tests__/manifest.spec.tslibs/skills/catalog/frontmcp-development/SKILL.mdlibs/skills/catalog/frontmcp-development/references/create-skill.mdlibs/skills/catalog/frontmcp-development/references/official-plugins.mdlibs/skills/catalog/frontmcp-production-readiness/SKILL.mdlibs/skills/catalog/skills-manifest.jsonlibs/skills/src/manifest.ts
✅ Files skipped from review due to trivial changes (3)
- libs/skills/tests/manifest.spec.ts
- libs/sdk/src/common/records/skill.record.ts
- libs/skills/catalog/frontmcp-production-readiness/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/skills/catalog/frontmcp-development/references/create-skill.md
- libs/skills/catalog/skills-manifest.json
- libs/skills/catalog/frontmcp-development/SKILL.md
…and standardize test file extensions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/frontmcp/guides/testing-your-first-tool.mdx`:
- Around line 64-66: Update the example test file path in the docs so the sample
uses the e2e directory, not src, because the test discovery patterns in
libs/cli/src/commands/dev/test.ts only match files under e2e/ (patterns:
<rootDir>/e2e/**/*.e2e.ts and <rootDir>/e2e/**/*.e2e.spec.ts); replace the
example filename "src/main.e2e.spec.ts" with "e2e/main.e2e.spec.ts" so it aligns
with the scaffolding (e.g., e2e/server.e2e.spec.ts) and will be discovered by
frontmcp test.
- Line 303: The code block title in the example is incorrect: change the code
fence header from ```typescript title="src/main.e2e.spec.ts" to ```typescript
title="e2e/main.e2e.spec.ts" (i.e., update the string "src/main.e2e.spec.ts" to
"e2e/main.e2e.spec.ts" in the markdown snippet) so the example matches the
frontmcp test discovery pattern; preserve the surrounding triple-backtick
fencing and the rest of the example content.
- Around line 243-251: Update the example file tree so e2e tests are placed
under an e2e/ directory that matches frontmcp's testMatch pattern: replace the
top-level main.e2e.spec.ts and apps/.../calculator.e2e.spec.ts entries with an
e2e/ folder containing server.e2e.spec.ts and calculator.e2e.spec.ts, and update
any referenced filenames (main.ts, apps/calculator/index.ts,
e2e/server.e2e.spec.ts, e2e/calculator.e2e.spec.ts) accordingly so the docs
reflect the actual test discovery behavior.
In `@libs/adapters/src/openapi/openapi.adapter.ts`:
- Around line 160-177: The collision check is currently using pre-transform
names (using metadata.originalToolName/displayName) and runs before
applyToolTransforms(), so move this uniqueness validation to run after
applyToolTransforms() (i.e., after all transforms including transforms.name and
any toolTransforms.perTool have been applied) and validate against the final
tool.name values on the transformedTools array; update the logic that builds
nameMap to use tool.name as the normalized key and throw if duplicates are found
so the perTool escape hatch can actually resolve collisions introduced by later
transforms.
- Around line 148-158: The current map over transformedTools overwrites the
public tool identifier by lowercasing tool.name (in the transformedTools =
transformedTools.map(...) block), which breaks SDK consumers that index by the
original operationId; instead leave tool.name unchanged and place the lowercased
form into a metadata field (e.g., metadata.normalizedToolName or metadata.alias)
while still setting metadata.originalToolName only when different—so update the
transform to stop mutating tool.name and store the normalizedName in metadata
for internal lookups (referencing transformedTools map and the normalizedName
variable).
- Around line 292-299: The resolvePerTool function currently performs raw
property lookups on perTool which can return inherited properties like toString;
update resolvePerTool (and its checks for tool.name and originalToolName) to
only accept own properties by using an own-property guard (e.g.,
Object.prototype.hasOwnProperty.call(perTool, key)) before returning
perTool[key]; ensure you apply this guard for both the normalized name lookup
(tool.name) and the fallback originalName lookup ((tool.metadata as
Record<string, unknown>)?.originalToolName) so only actual map entries are
returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8f43961-3265-4924-b559-99b48c0947d5
📒 Files selected for processing (6)
docs/frontmcp/guides/testing-your-first-tool.mdxdocs/frontmcp/testing/overview.mdxlibs/adapters/src/openapi/openapi.adapter.tslibs/cli/src/commands/skills/install.tslibs/sdk/src/common/decorators/skill.decorator.tslibs/sdk/src/common/types/options/__tests__/http.options.spec.ts
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/common/types/options/tests/http.options.spec.ts
- docs/frontmcp/testing/overview.mdx
- libs/cli/src/commands/skills/install.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/common/decorators/skill.decorator.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/adapters/src/openapi/openapi.adapter.ts (2)
158-176: Collision tracking stores redundant data; consider tracking original identifiers.The
occurrencesarray collectstool.namefor each collision, but since all colliding tools have the sametool.namevalue by definition, the array content (e.g.,['createUser', 'createUser']) doesn't help identify which original operations collided. Consider tracking the original operationId or${method} ${path}from metadata so the error message can pinpoint the conflicting sources.Suggested improvement
- const nameMap = new Map<string, string[]>(); + const nameMap = new Map<string, { operationId: string; method: string; path: string }[]>(); for (const tool of transformedTools) { + const meta = tool.metadata as Record<string, unknown>; + const toolIdentity = { + operationId: (meta['operationId'] as string) ?? tool.name, + method: (meta['method'] as string) ?? '?', + path: (meta['path'] as string) ?? '?', + }; const existing = nameMap.get(tool.name); if (existing) { - existing.push(tool.name); + existing.push(toolIdentity); } else { - nameMap.set(tool.name, [tool.name]); + nameMap.set(tool.name, [toolIdentity]); } } for (const [name, occurrences] of nameMap) { if (occurrences.length > 1) { + const sources = occurrences.map((o) => `${o.method.toUpperCase()} ${o.path} (${o.operationId})`).join(', '); throw new Error( - `Tool name collision: "${name}" appears ${occurrences.length} times after transforms. ` + + `Tool name collision: "${name}" appears ${occurrences.length} times after transforms (sources: ${sources}). ` + `Rename conflicting operations in your OpenAPI spec or use toolTransforms.perTool to assign unique names.`, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/adapters/src/openapi/openapi.adapter.ts` around lines 158 - 176, The collision tracking currently stores duplicate tool.name values in nameMap, which doesn't show what original operations collided; update the logic that builds nameMap (iterating transformedTools) to push a unique source identifier (e.g., tool.operationId or `${tool.method} ${tool.path}` or another originalId property) instead of tool.name, so nameMap maps finalName -> [sourceIdentifiers]; then update the error thrown in the loop over nameMap (the block referencing transformedTools, nameMap, tool.name, and occurrences) to include those source identifiers in the message so the error pinpoints which original operations produced the conflicting final name.
274-283: Docstring mentions fallback behavior that is not implemented.The docstring states it "falls back to the tool's original (pre-normalization) name" but the implementation only checks
tool.name. Since the lowercase normalization step appears to have been removed from this branch, either:
- Update the docstring to remove the misleading fallback reference, or
- Implement the fallback to
metadata.originalToolNameif tool renaming viatoolTransforms.nameshould still allowperToolkeys to use original namesThis inconsistency could confuse maintainers and users configuring
perToolmaps.Option A: Simplify docstring (if fallback is not needed)
/** - * Look up a value in a perTool map, falling back to the tool's original (pre-normalization) name. - * This ensures perTool configs keyed with mixed-case names still match after normalization. + * Look up a value in a perTool map using the current tool name. + * Uses own-property check to prevent prototype pollution. * `@private` */Option B: Implement fallback (if backward compatibility is needed)
private resolvePerTool<T>(perTool: Record<string, T> | undefined, tool: McpOpenAPITool): T | undefined { if (!perTool) return undefined; + // Try current name first if (Object.prototype.hasOwnProperty.call(perTool, tool.name)) return perTool[tool.name]; + // Fallback to original name (before toolTransforms.name rename) + const meta = tool.metadata as Record<string, unknown>; + const originalName = (meta['adapter'] as Record<string, unknown> | undefined)?.originalToolName as string | undefined; + if (originalName && Object.prototype.hasOwnProperty.call(perTool, originalName)) { + return perTool[originalName]; + } return undefined; }Note: Option B also requires storing
originalToolNameinapplyToolTransformswhen renaming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/adapters/src/openapi/openapi.adapter.ts` around lines 274 - 283, The docstring for resolvePerTool<T> claims it falls back to the tool's original (pre-normalization) name but the implementation only checks tool.name; to fix this, update resolvePerTool to also check perTool[tool.metadata?.originalToolName] (after the existing check) and return that value if present, and ensure applyToolTransforms records the original name by setting tool.metadata.originalToolName before changing tool.name so the fallback value is available; alternatively, if fallback is undesired, remove the fallback reference from the resolvePerTool docstring and ensure no code relies on metadata.originalToolName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/adapters/src/openapi/openapi.adapter.ts`:
- Around line 158-176: The collision tracking currently stores duplicate
tool.name values in nameMap, which doesn't show what original operations
collided; update the logic that builds nameMap (iterating transformedTools) to
push a unique source identifier (e.g., tool.operationId or `${tool.method}
${tool.path}` or another originalId property) instead of tool.name, so nameMap
maps finalName -> [sourceIdentifiers]; then update the error thrown in the loop
over nameMap (the block referencing transformedTools, nameMap, tool.name, and
occurrences) to include those source identifiers in the message so the error
pinpoints which original operations produced the conflicting final name.
- Around line 274-283: The docstring for resolvePerTool<T> claims it falls back
to the tool's original (pre-normalization) name but the implementation only
checks tool.name; to fix this, update resolvePerTool to also check
perTool[tool.metadata?.originalToolName] (after the existing check) and return
that value if present, and ensure applyToolTransforms records the original name
by setting tool.metadata.originalToolName before changing tool.name so the
fallback value is available; alternatively, if fallback is undesired, remove the
fallback reference from the resolvePerTool docstring and ensure no code relies
on metadata.originalToolName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67205e2a-5e46-44fa-bfdd-0491dd23183a
📒 Files selected for processing (2)
libs/adapters/src/openapi/openapi.adapter.tslibs/cli/src/commands/scaffold/__tests__/create.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/cli/src/commands/scaffold/tests/create.spec.ts
…s and modify skills manifest resource flag
…rity and organization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/skills/project.json`:
- Around line 15-23: The generate-manifest target lacks an outputs declaration
so Nx cannot cache/restore the produced skills-manifest.json; update the
"generate-manifest" target to declare its output (the generated file produced by
libs/skills/scripts/generate-manifest.mjs, e.g. the path to
skills-manifest.json) so that the dependent "build-tsc" target will get the
cached artifact on restore; add an outputs array referencing the exact generated
file path relative to the project (or a glob) in the generate-manifest target
configuration.
In `@libs/skills/scripts/generate-manifest.mjs`:
- Around line 101-109: The manifest emitter currently copies fm.targets,
fm.tags, and fm.bundle directly (from parseFrontmatter) which may be plain
scalars; update the skills.push construction to normalize and validate these
fields: coerce fm.targets/fm.tags/fm.bundle into arrays of strings when they are
scalars, enforce defaults (targets -> ['all'], tags -> [], bundle -> ['full'])
and validate category and bundle entries against the runtime allow-lists defined
in libs/skills/src/manifest.ts (and reject or log+fail on unknown values); use
the existing fm, parseFrontmatter, hasResources, and CATALOG_DIR symbols to
locate the code and implement the shape/coercion and allow-list checks before
serializing skills-manifest.json.
- Around line 36-60: The current yaml parsing loop in generate-manifest.mjs
skips all indented lines, which silently drops block sequences and nested keys;
update the loop that iterates over yaml.split('\n') so when you see a key with
an empty value (let value = line.slice(colonIdx + 1).trim() yields ''), you
perform a lookahead over subsequent indented lines instead of unconditionally
skipping them: if the following indented lines start with '-' collect them into
an array and set result[key] = [...items]; if they are nested key:value pairs
either parse them into a nested object (e.g., result[key] = {subkey: value}) or
throw a clear error indicating unsupported nested structure; ensure you adjust
the loop index to consume the looked-ahead lines and still use the existing
variables (line, key, value, result) so we no longer silently drop frontmatter
like "targets:" with "- all".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5169b0fb-6451-4107-ae9a-253f950bfa83
📒 Files selected for processing (10)
libs/skills/catalog/frontmcp-config/SKILL.mdlibs/skills/catalog/frontmcp-deployment/SKILL.mdlibs/skills/catalog/frontmcp-development/SKILL.mdlibs/skills/catalog/frontmcp-guides/SKILL.mdlibs/skills/catalog/frontmcp-production-readiness/SKILL.mdlibs/skills/catalog/frontmcp-setup/SKILL.mdlibs/skills/catalog/frontmcp-testing/SKILL.mdlibs/skills/catalog/skills-manifest.jsonlibs/skills/project.jsonlibs/skills/scripts/generate-manifest.mjs
✅ Files skipped from review due to trivial changes (5)
- libs/skills/catalog/frontmcp-guides/SKILL.md
- libs/skills/catalog/frontmcp-deployment/SKILL.md
- libs/skills/catalog/frontmcp-development/SKILL.md
- libs/skills/catalog/frontmcp-production-readiness/SKILL.md
- libs/skills/catalog/frontmcp-config/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/skills/catalog/frontmcp-setup/SKILL.md
- libs/skills/catalog/frontmcp-testing/SKILL.md
…rity and organization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/skills/scripts/generate-manifest.mjs (2)
101-110:⚠️ Potential issue | 🟠 MajorNormalize and validate manifest field shapes before writing JSON.
At Line 106/108/109, scalar frontmatter values are copied as-is (
stringinstead ofstring[]), and at Line 103 category can resolve to values outsideSkillCategory. Validate/coerce here soskills-manifest.jsonalways matcheslibs/skills/src/manifest.ts.Suggested fix
+const VALID_CATEGORIES = new Set([ + 'setup', + 'deployment', + 'development', + 'config', + 'testing', + 'guides', + 'production', +]); +const VALID_BUNDLES = new Set(['recommended', 'minimal', 'full']); + +function toStringArray(value, fallback) { + if (Array.isArray(value)) return value.map((v) => String(v)); + if (typeof value === 'string' && value.trim()) return [value.trim()]; + return fallback; +} + @@ - skills.push({ + const category = fm.category || dir.replace('frontmcp-', ''); + if (!VALID_CATEGORIES.has(category)) { + throw new Error(`Invalid category "${category}" in ${dir}/SKILL.md`); + } + + const bundle = toStringArray(fm.bundle, ['full']); + for (const b of bundle) { + if (!VALID_BUNDLES.has(b)) { + throw new Error(`Invalid bundle "${b}" in ${dir}/SKILL.md`); + } + } + + skills.push({ name: fm.name, - category: fm.category || dir.replace('frontmcp-', ''), + category, description: fm.description || '', path: dir, - targets: fm.targets || ['all'], + targets: toStringArray(fm.targets, ['all']), hasResources: hasResources(path.join(CATALOG_DIR, dir)), - tags: fm.tags || [], - bundle: fm.bundle || ['full'], + tags: toStringArray(fm.tags, []), + bundle, });As per coding guidelines,
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/scripts/generate-manifest.mjs` around lines 101 - 110, The manifest writer currently copies frontmatter directly (fm.name, fm.category, fm.targets, fm.tags, fm.bundle) which allows scalars and invalid categories into the generated skills-manifest.json; update the skills.push creation to coerce scalar values into arrays (ensure fm.targets, fm.tags, fm.bundle are string[] by wrapping non-arrays) and validate/coerce fm.category against the SkillCategory enum (fallback to a safe default category if not recognized), and ensure fm.name and description are strings; use the same shape/keys as libs/skills/src/manifest.ts so hasResources(path.join(CATALOG_DIR, dir)) remains untouched but the other fields are normalized and validated before pushing.
36-61:⚠️ Potential issue | 🟠 MajorUnsupported indented YAML is silently ignored.
At Line 38, indented YAML is skipped, so block arrays/nested keys in frontmatter are dropped instead of parsed or rejected. This can silently corrupt emitted metadata.
Suggested fix
- for (const line of yaml.split('\n')) { - // Skip empty lines and nested keys (e.g., metadata.docs) - if (!line.trim() || /^\s+/.test(line)) continue; + const lines = yaml.split('\n'); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (!line.trim()) continue; + if (/^\s+/.test(line)) { + throw new Error(`Unsupported indented YAML in frontmatter at line ${i + 1}`); + } @@ - const value = line.slice(colonIdx + 1).trim(); + const value = line.slice(colonIdx + 1).trim(); + + // Block-style array: + // key: + // - a + // - b + if (!value) { + const items = []; + while (i + 1 < lines.length && /^\s*-\s+/.test(lines[i + 1])) { + const item = lines[++i].replace(/^\s*-\s+/, '').trim(); + items.push(item.replace(/^["']|["']$/g, '')); + } + if (items.length > 0) { + result[key] = items; + continue; + } + throw new Error(`Unsupported empty YAML value for "${key}"`); + } @@ - else if (value) { + else { result[key] = value; } }As per coding guidelines,
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/scripts/generate-manifest.mjs` around lines 36 - 61, The parser currently silently skips any indented YAML lines (the /^\s+/ check in the for-loop), which drops nested keys and block values; update the loop in generate-manifest.mjs to stop silently ignoring indented lines—either parse them as nested/frontmatter blocks or raise a clear error so metadata isn't silently lost. Concretely, remove or modify the /^\s+/ skip logic and implement handling that associates indented lines with the last top-level key (e.g., keep a currentKey variable and append indented lines to result[currentKey] as a block or array), or if you don't want to support nested YAML, throw a descriptive error when an indented line is encountered so callers know the frontmatter is unsupported. Ensure changes reference the existing variables/structures: the for (const line of yaml.split('\n')) loop, the /^\s+/ test, the colonIdx/key/value extraction, and the result object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/skills/scripts/generate-manifest.mjs`:
- Around line 96-99: Replace the silent-warning-and-continue behavior for
malformed SKILL.md by failing the build: in the block that checks "if (!fm ||
!fm.name)" (the variables fm and dir in generate-manifest.mjs), replace the
console.error + continue with a hard failure such as throwing a descriptive
Error or calling process.exit(1) (e.g., throw new Error(`Invalid SKILL.md in
${dir}: missing frontmatter or 'name'`)) so manifest generation aborts on
invalid skills.
---
Duplicate comments:
In `@libs/skills/scripts/generate-manifest.mjs`:
- Around line 101-110: The manifest writer currently copies frontmatter directly
(fm.name, fm.category, fm.targets, fm.tags, fm.bundle) which allows scalars and
invalid categories into the generated skills-manifest.json; update the
skills.push creation to coerce scalar values into arrays (ensure fm.targets,
fm.tags, fm.bundle are string[] by wrapping non-arrays) and validate/coerce
fm.category against the SkillCategory enum (fallback to a safe default category
if not recognized), and ensure fm.name and description are strings; use the same
shape/keys as libs/skills/src/manifest.ts so hasResources(path.join(CATALOG_DIR,
dir)) remains untouched but the other fields are normalized and validated before
pushing.
- Around line 36-61: The parser currently silently skips any indented YAML lines
(the /^\s+/ check in the for-loop), which drops nested keys and block values;
update the loop in generate-manifest.mjs to stop silently ignoring indented
lines—either parse them as nested/frontmatter blocks or raise a clear error so
metadata isn't silently lost. Concretely, remove or modify the /^\s+/ skip logic
and implement handling that associates indented lines with the last top-level
key (e.g., keep a currentKey variable and append indented lines to
result[currentKey] as a block or array), or if you don't want to support nested
YAML, throw a descriptive error when an indented line is encountered so callers
know the frontmatter is unsupported. Ensure changes reference the existing
variables/structures: the for (const line of yaml.split('\n')) loop, the /^\s+/
test, the colonIdx/key/value extraction, and the result object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7060d5e7-cd73-44d1-b5b5-92cfd191ecc1
📒 Files selected for processing (2)
libs/skills/scripts/generate-manifest.mjslibs/skills/src/loader.ts
💤 Files with no reviewable changes (1)
- libs/skills/src/loader.ts
… handling and structure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eslint.config.mjs (1)
62-64: Narrow the E2E test glob to canonical spec files only.Line 62 and Line 64 make the relaxed override broader than “test files”:
**/*.e2e.tsallows non-standard naming, and**/e2e/**/*.tsdisables strict rules for every TS file ine2e/. Prefer targeting spec files only.Proposed diff
files: [ '**/*.test.ts', '**/*.spec.ts', - '**/*.e2e.ts', '**/*.e2e.spec.ts', - '**/e2e/**/*.ts', + '**/e2e/**/*.spec.ts', '**/__tests__/**/*.ts', '**/__test-utils__/**/*.ts', '**/fixtures/**/*.ts', '**/mocks/**/*.ts', '**/*.mock.ts', ],Based on learnings: Use
.e2e.spec.tsfor E2E tests and maintain strict TypeScript correctness (avoid broad relaxations).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.mjs` around lines 62 - 64, The E2E override globs are too broad; change the patterns in eslint.config.mjs so they only match canonical spec filenames (e.g., replace '**/*.e2e.ts' with '**/*.e2e.spec.ts' and replace '**/e2e/**/*.ts' with a path-restricted pattern like '**/e2e/**/*.e2e.spec.ts' or similar) so that only true E2E spec files (matching '*.e2e.spec.ts') receive the relaxed rules; update the glob entries near the existing '**/*.e2e.spec.ts' entry to be explicit about '*.e2e.spec.ts' only.libs/skills/scripts/generate-manifest.mjs (1)
25-28: Deduplicate the manifest allow-lists.These enums already exist in
libs/skills/src/manifest.ts. Keeping a second copy here means the generator and runtime validation can drift again the next time a category, target, or bundle changes. Please move them into a shared JS/JSON module both sides import.As per coding guidelines,
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/scripts/generate-manifest.mjs` around lines 25 - 28, The allow-list arrays VALID_CATEGORIES, VALID_TARGETS, and VALID_BUNDLES are duplicated in generate-manifest.mjs and libs/skills/src/manifest.ts; extract these constants into a single shared module (e.g., export from a new JS/JSON file) and update both generate-manifest.mjs and libs/skills/src/manifest.ts to import the constants instead of redefining them so the generator and runtime use the same source of truth and avoid drift; specifically remove the local definitions of VALID_CATEGORIES / VALID_TARGETS / VALID_BUNDLES from generate-manifest.mjs, create and export them from the shared module, and change imports in generate-manifest.mjs and the existing manifest.ts to reference the shared names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/skills/scripts/generate-manifest.mjs`:
- Around line 82-109: parseFrontmatter() can yield object values for keys like
name or tags, but the manifest-writing code only checks fm.name truthiness and
uses toStringArray() which stringifies objects, producing invalid SkillManifest
entries; update the validation in generate-manifest.mjs so before serializing
you assert and coerce types: ensure fm.name is a non-empty string (throw or skip
if it's an object/array), ensure fm.tags (and any array fields handled by
toStringArray()) are actual arrays of strings (or convert only when scalar
strings), and reject or sanitize object-valued mappings by checking typeof
fm[name] !== 'object' for scalar fields and Array.isArray for list fields;
locate uses around parseFrontmatter(), the toStringArray() helper, and the main
loop that assigns result[key] to validate and coerce values (or emit a clear
error) before adding to result and writing skills-manifest.json.
- Around line 35-39: The frontmatter regex in parseFrontmatter only matches LF
and fails on CRLF files; update the pattern used in parseFrontmatter to accept
optional CR before newlines (e.g., use \r?\n in place of \n) and enable
multiline if needed so the opening and closing `---` delimiters and the capture
group work for both LF and CRLF line endings; modify the regex in
parseFrontmatter accordingly to match /^---\r?\n([\s\S]*?)\r?\n---/ (or
equivalent) so CRLF SKILL.md files are parsed correctly.
---
Nitpick comments:
In `@eslint.config.mjs`:
- Around line 62-64: The E2E override globs are too broad; change the patterns
in eslint.config.mjs so they only match canonical spec filenames (e.g., replace
'**/*.e2e.ts' with '**/*.e2e.spec.ts' and replace '**/e2e/**/*.ts' with a
path-restricted pattern like '**/e2e/**/*.e2e.spec.ts' or similar) so that only
true E2E spec files (matching '*.e2e.spec.ts') receive the relaxed rules; update
the glob entries near the existing '**/*.e2e.spec.ts' entry to be explicit about
'*.e2e.spec.ts' only.
In `@libs/skills/scripts/generate-manifest.mjs`:
- Around line 25-28: The allow-list arrays VALID_CATEGORIES, VALID_TARGETS, and
VALID_BUNDLES are duplicated in generate-manifest.mjs and
libs/skills/src/manifest.ts; extract these constants into a single shared module
(e.g., export from a new JS/JSON file) and update both generate-manifest.mjs and
libs/skills/src/manifest.ts to import the constants instead of redefining them
so the generator and runtime use the same source of truth and avoid drift;
specifically remove the local definitions of VALID_CATEGORIES / VALID_TARGETS /
VALID_BUNDLES from generate-manifest.mjs, create and export them from the shared
module, and change imports in generate-manifest.mjs and the existing manifest.ts
to reference the shared names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b9f7fb5-2e34-47c7-b7af-fcb06f995458
📒 Files selected for processing (3)
eslint.config.mjslibs/skills/project.jsonlibs/skills/scripts/generate-manifest.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/skills/project.json
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
New Features
skills installsupports selectors (--all/--tag/--category) and optional name; help shows Skills examples.frontmcp test.Behavior
Documentation
frontmcp testand .e2e.spec.ts patterns; Node.js 24+ updates.