feat: enhance plugin provider handling and add tests for decorator providers#320
Merged
frontegg-david merged 4 commits intorelease/1.0.xfrom Mar 29, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNormalize plugin metadata to prevent inline providers from overriding decorator-declared providers; add/extend tests for provider-merge and registry behavior; integrate manifest-driven, versioned "Skills and Tools" blocks into CLAUDE.md generation with CLI scaffolding and tests; remove several inline ESLint disable comments; update skill docs to stop catching infrastructure errors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI scaffold/create
participant FS as Local filesystem (skills/catalog)
participant PKG as `@frontmcp/skills` package
participant Generator as generateClaudeMd / buildSkillsSection
CLI->>FS: try read skills-manifest.json
FS-->>CLI: manifest (or not found)
alt manifest found locally
CLI->>Generator: filter by target/bundle, map to entries
else not found
CLI->>PKG: resolve package manifest
PKG-->>CLI: manifest (or fail)
CLI->>Generator: filter/map (or empty)
end
CLI->>Generator: buildSkillsSection(version, entries)
Generator-->>CLI: skills block
CLI->>FS: read/write CLAUDE.md (insert/replace/prepend block)
FS-->>CLI: file written
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/plugin/plugin.utils.ts (1)
53-65:⚠️ Potential issue | 🟠 MajorStrip inline
providersfrom metadata in bothuseFactoryanduseClassbranches.The
useValuebranch correctly excludes inlineprovidersfrom metadata (line 76) and returns them separately (line 83). However, bothuseFactory(line 63) anduseClass(line 49) pass metadata without strippingproviders, which breaks the intended separation. If a factory or class plugin is registered with inlineproviders, they remain inmetadata.providersinstead of being extracted torec.providersfor registration at line 277 in plugin.registry.ts.Apply the same pattern as
useValue:
- Extract
providersfrom metadata:const { providers: _dynamicProviders, ...inlineMetadata } = metadata- Return
providers: (item.providers ?? [])in the record- Use
inlineMetadata(without providers) for metadata merging🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/plugin/plugin.utils.ts` around lines 53 - 65, In the useFactory and useClass branches of the plugin builder (the blocks that return records with kind PluginKind.FACTORY and PluginKind.CLASS), strip inline providers from metadata the same way useValue does: destructure metadata to extract providers (e.g. const { providers: _dynamicProviders, ...inlineMetadata } = metadata), return a providers field set to (item.providers ?? []) on the record, and use inlineMetadata (without providers) for the metadata property; ensure the InvalidUseFactoryError/type checks remain unchanged and that tokenName(provide) usage is preserved.
🤖 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/cli/src/core/__tests__/help.spec.ts`:
- Around line 58-59: Restore the ESLint suppression for the control character in
the regex used by the stripAnsi function: add the comment "//
eslint-disable-next-line no-control-regex" immediately above the stripAnsi
declaration (the line defining const stripAnsi = (s: string) =>
s.replace(/\x1b\[[0-9;]*m/g, '');) so the no-control-regex rule is ignored for
this required ANSI-stripping regular expression.
---
Outside diff comments:
In `@libs/sdk/src/plugin/plugin.utils.ts`:
- Around line 53-65: In the useFactory and useClass branches of the plugin
builder (the blocks that return records with kind PluginKind.FACTORY and
PluginKind.CLASS), strip inline providers from metadata the same way useValue
does: destructure metadata to extract providers (e.g. const { providers:
_dynamicProviders, ...inlineMetadata } = metadata), return a providers field set
to (item.providers ?? []) on the record, and use inlineMetadata (without
providers) for the metadata property; ensure the InvalidUseFactoryError/type
checks remain unchanged and that tokenName(provide) usage 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: 58e34534-6aae-428e-981d-24515d3776d0
📒 Files selected for processing (4)
libs/cli/src/core/__tests__/help.spec.tslibs/sdk/src/plugin/__tests__/plugin.registry.spec.tslibs/sdk/src/plugin/__tests__/plugin.utils.spec.tslibs/sdk/src/plugin/plugin.utils.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-29T06:33:53.897Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/cli/src/commands/skills/__tests__/install.spec.ts (1)
125-142: Add migration coverage for legacy blocks followed by##headings.Current migration tests only validate a next
#heading. Add a case with##(common in realCLAUDE.md) to prevent accidental truncation regressions.🧪 Suggested additional test
+ it('should migrate legacy section and preserve content under subheadings', async () => { + mockFiles['/test/project/CLAUDE.md'] = [ + '# Skills and Tools', + 'Legacy content', + '', + '## Commands', + 'Run yarn dev.', + ].join('\n'); + + await ensureClaudeMdSkillsInstructions('/test/project'); + + const content = mockFiles['/test/project/CLAUDE.md']; + expect(content).toContain('<!-- frontmcp:skills-start v1.0.0-test -->'); + expect(content).toContain('## Commands'); + expect(content).toContain('Run yarn dev.'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/skills/__tests__/install.spec.ts` around lines 125 - 142, Add a new test in install.spec.ts that verifies migration of legacy "# Skills and Tools" when the next heading is a "##" level (not just "#"); update or add a case using mockFiles['/test/project/CLAUDE.md'] where after the legacy block you have '## Other Heading' and content, then call ensureClaudeMdSkillsInstructions('/test/project') and assert the file contains the marker comments ('<!-- frontmcp:skills-start v1.0.0-test -->' and '<!-- frontmcp:skills-end -->'), does not contain the old hardcoded content, and still contains the '## Other Heading' and its following content; reference the existing test name and function ensureClaudeMdSkillsInstructions to locate where to add this case.
🤖 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/cli/src/commands/scaffold/create.ts`:
- Around line 312-325: The CLAUDE.md generator (generateClaudeMd) is using the
full manifest's skillEntries which can include skills not actually scaffolded;
update generateClaudeMd (and the callers that pass skillEntries) to filter
skillEntries the same way the scaffolding/install logic does (by target and
bundle) before calling buildSkillsSection so the skills block only lists
installed skills (reuse the same filter function or inline the target/bundle
check used in the scaffold flow). Ensure you reference the same filtering
criteria used in the scaffold/install code paths so all places that call
generateClaudeMd (and related ranges around buildSkillsSection) stay consistent.
In `@libs/cli/src/commands/skills/install.ts`:
- Around line 111-113: The current heading-detection only matches "\n" followed
by a single "#" (const nextHeadingMatch = afterLegacy.match(/\n(?=# )/)), which
causes content after "##" or "###" headings to be dropped; update the regex to
detect any Markdown heading level (for example use /\n(?=#+\s)/ or
/\n(?=#{1,6}\s)/) when computing nextHeadingMatch, leaving the subsequent slice
logic (after = nextHeadingMatch ? afterLegacy.slice(nextHeadingMatch.index!) :
'') unchanged so all following headings (##, ###, etc.) are preserved.
- Around line 94-99: The end-marker search should begin at the start-marker
index to ensure we match the correct block: change the lookup of
SKILLS_BLOCK_END to search from startIdx (use indexOf with the start position)
so endIdx = content.indexOf(SKILLS_BLOCK_END, startIdx) (then keep the existing
slicing into before = content.slice(0, startIdx) and after =
content.slice(endIdx + SKILLS_BLOCK_END.length) logic); this prevents picking an
earlier unrelated end marker and ensures proper replacement boundaries for
SKILLS_BLOCK_START/SKILLS_BLOCK_END handling.
---
Nitpick comments:
In `@libs/cli/src/commands/skills/__tests__/install.spec.ts`:
- Around line 125-142: Add a new test in install.spec.ts that verifies migration
of legacy "# Skills and Tools" when the next heading is a "##" level (not just
"#"); update or add a case using mockFiles['/test/project/CLAUDE.md'] where
after the legacy block you have '## Other Heading' and content, then call
ensureClaudeMdSkillsInstructions('/test/project') and assert the file contains
the marker comments ('<!-- frontmcp:skills-start v1.0.0-test -->' and '<!--
frontmcp:skills-end -->'), does not contain the old hardcoded content, and still
contains the '## Other Heading' and its following content; reference the
existing test name and function ensureClaudeMdSkillsInstructions to locate where
to add this case.
🪄 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: 6ced55b2-b35d-490c-927d-5258606c62c5
📒 Files selected for processing (8)
CLAUDE.mdlibs/cli/src/commands/scaffold/create.tslibs/cli/src/commands/skills/__tests__/install.spec.tslibs/cli/src/commands/skills/install.tslibs/skills/catalog/frontmcp-development/references/create-tool.mdlibs/skills/catalog/frontmcp-guides/references/example-task-manager.mdlibs/skills/catalog/frontmcp-guides/references/example-weather-api.mdlibs/skills/catalog/frontmcp-testing/references/setup-testing.md
✅ Files skipped from review due to trivial changes (3)
- libs/skills/catalog/frontmcp-testing/references/setup-testing.md
- CLAUDE.md
- libs/skills/catalog/frontmcp-development/references/create-tool.md
…signatures in TypeScript files
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/cli/src/commands/scaffold/create.ts (1)
1404-1434: Unify skill-manifest loading/filtering to avoid future drift.Line 1404 duplicates manifest resolution + target/bundle filtering logic already present in
scaffoldSkills. This is the same class of logic that previously drifted and caused CLAUDE.md mismatch behavior. Centralizing into one helper will reduce regression risk.♻️ Suggested refactor sketch
+async function loadSkillManifest(): Promise<SkillManifest | null> { + try { + const catalogDir = path.resolve(__dirname, '..', '..', '..', '..', 'skills', 'catalog'); + const manifestPath = path.join(catalogDir, 'skills-manifest.json'); + if (await fileExists(manifestPath)) { + return JSON.parse(await readFile(manifestPath)) as SkillManifest; + } + const require_ = createRequire(__filename); + const pkgManifest = require_.resolve('@frontmcp/skills/catalog/skills-manifest.json'); + return JSON.parse(await readFile(pkgManifest)) as SkillManifest; + } catch { + return null; + } +} + +function filterSkillsByTargetAndBundle( + skills: SkillCatalogEntry[], + options: Pick<CreateOptions, 'deploymentTarget' | 'skillsBundle'>, +): SkillCatalogEntry[] { + const bundle = options.skillsBundle ?? 'recommended'; + if (bundle === 'none') return []; + return skills.filter((s) => { + const targetMatch = s.targets.includes('all') || s.targets.includes(options.deploymentTarget); + const bundleMatch = s.bundle?.includes(bundle) === true; + return targetMatch && bundleMatch; + }); +}🤖 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 1404 - 1434, The function loadSkillEntriesForClaudeMd duplicates manifest resolution and target/bundle filtering logic found in scaffoldSkills; extract a single helper (e.g., loadSkillManifest and filterSkillsByTargetAndBundle) that performs manifest path resolution (including fallback to require_.resolve), reading/parsing into SkillManifest, and the filter (targets includes 'all' or deploymentTarget and bundle includes skillsBundle), then update loadSkillEntriesForClaudeMd and scaffoldSkills to call that helper and return the {name, description} mapping; keep error handling behavior (return [] on failure) and reuse unique symbols loadSkillEntriesForClaudeMd and scaffoldSkills to locate call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Around line 8-23: The global "ignores" array currently contains test-file glob
patterns (e.g., **/*.spec.ts, **/*.test.tsx, **/__tests__/**) which cause ESLint
to skip those files before any overrides run; remove those test-related globs
from the "ignores" array so the override block that relaxes rules for test files
(the existing override block for test patterns) can actually apply, leaving only
build/artifact globs (like **/dist, **/*.d.ts, apps/e2e/**) in "ignores" and
relying on the overrides section to target test files.
In `@libs/cli/src/commands/skills/install.ts`:
- Around line 84-87: The CLAUDE.md generation currently passes manifest.skills
to buildSkillsSection which includes catalog entries not actually installed;
change this to derive the skills list from the installed skill directories under
the project (use cwd to locate the provider/installed skills folder), collect
only the installed skill names/metadata, and pass that installed-skills array
into buildSkillsSection(version, installedSkills) instead of manifest.skills so
the generated CLAUDE.md reflects what is actually installed.
---
Nitpick comments:
In `@libs/cli/src/commands/scaffold/create.ts`:
- Around line 1404-1434: The function loadSkillEntriesForClaudeMd duplicates
manifest resolution and target/bundle filtering logic found in scaffoldSkills;
extract a single helper (e.g., loadSkillManifest and
filterSkillsByTargetAndBundle) that performs manifest path resolution (including
fallback to require_.resolve), reading/parsing into SkillManifest, and the
filter (targets includes 'all' or deploymentTarget and bundle includes
skillsBundle), then update loadSkillEntriesForClaudeMd and scaffoldSkills to
call that helper and return the {name, description} mapping; keep error handling
behavior (return [] on failure) and reuse unique symbols
loadSkillEntriesForClaudeMd and scaffoldSkills to locate call sites.
🪄 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: 8754a4cb-280f-4cda-bc04-94716250365e
⛔ Files ignored due to path filters (1)
libs/cli/src/commands/build/exec/cli-runtime/daemon-client.tsis excluded by!**/build/**
📒 Files selected for processing (12)
eslint.config.mjslibs/auth/src/machine-id/machine-id.tslibs/cli/src/commands/scaffold/create.tslibs/cli/src/commands/skills/__tests__/install.spec.tslibs/cli/src/commands/skills/install.tslibs/nx-plugin/src/generators/workspace/workspace.spec.tslibs/ui/src/renderer/common/lazy-import.tslibs/ui/src/renderer/react/index.tslibs/uipack/src/component/transpiler.tslibs/utils/src/crypto/index.tslibs/utils/src/crypto/secret-persistence/persistence.tslibs/utils/src/event-emitter/browser-event-emitter.ts
💤 Files with no reviewable changes (4)
- libs/utils/src/event-emitter/browser-event-emitter.ts
- libs/ui/src/renderer/common/lazy-import.ts
- libs/nx-plugin/src/generators/workspace/workspace.spec.ts
- libs/ui/src/renderer/react/index.ts
✅ Files skipped from review due to trivial changes (5)
- libs/uipack/src/component/transpiler.ts
- libs/auth/src/machine-id/machine-id.ts
- libs/utils/src/crypto/secret-persistence/persistence.ts
- libs/utils/src/crypto/index.ts
- libs/cli/src/commands/skills/tests/install.spec.ts
…signatures in TypeScript files
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
Bug Fixes
New Features
Documentation
Tests
Style