feat(module): add agent skills installation #1201
Conversation
commit: |
📦 Bundle Size Comparison📈 nuxi
📈 nuxt-cli
📈 create-nuxt
|
onmax
force-pushed
the
feat/module-skills
branch
from
415c222 to
6676659
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 282d54438d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const moduleNames = depNames.filter(name => | ||
| knownModuleNames.has(name) || name.startsWith('@nuxt/') || name.startsWith('nuxt-'), | ||
| ) |
There was a problem hiding this comment.
Filtering depNames to Nuxt DB entries and only @nuxt//nuxt- prefixes means many valid Nuxt modules (for example scoped packages like @scope/nuxt-auth that are private or absent from the API response) never reach detectModuleSkills, so nuxi module skills silently misses their meta.agents.skills definitions and cannot install their skills. This is a functional regression whenever the modules API is incomplete or unavailable.
Useful? React with 👍 / 👎.
| if (isCancel(agentChoice)) | ||
| return |
There was a problem hiding this comment.
After dependencies are installed, canceling inside the skill "Change configuration" flow returns from setup immediately, which bypasses the later runCommand(prepareCommand, args) call. In practice, pressing cancel at this prompt can leave the project without the normal post-install prepare step even though module add already modified dependencies/config; cancellation here should only skip skill installation.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis diff introduces a comprehensive agent skills management system for Nuxi modules. It adds a new dependency (unagent ^0.0.5) and establishes skill detection, installation, and management functionality. A new "skills" command is added to the module command structure, alongside integration of skill installation workflows into the existing module add command. The changes include module metadata introspection, skill source detection, interactive CLI prompts for skill configuration, and error handling for various failure scenarios. Comprehensive unit tests are included to validate skill installation logic and error handling paths. Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@packages/nuxi/package.json`:
- Line 77: The package.json lists a non-existent dependency "unagent" which
causes runtime module resolution failures because code in
packages/nuxi/src/commands/module/add.ts, skills.ts and _skills.ts import from
it; either remove "unagent" from package.json and delete or refactor its imports
in those three files, or replace the entry with the correct package
name/registry and update package install configuration; specifically locate
imports of "unagent" in the three modules (import ... from 'unagent'), then
either (A) change those imports to the correct, existing package name used
elsewhere in the repo or a publicly available alternative, or (B) remove the
imports and their usage and implement equivalent logic, or (C) restore/add the
private registry/config that provides "unagent" and document it in package.json
so installs succeed.
In `@packages/nuxi/src/commands/module/_skills.ts`:
- Around line 113-131: The loop calling installSkill checks result.installed and
result.errors but currently only invokes callbacks.onSuccess when
installed.length>0, so the spinner isn't stopped if both arrays are empty;
change the post-install logic in the for loop (the block handling the result
from installSkill) to treat the "both empty" case as a success/empty case by
invoking callbacks.onSuccess?.(source, result) when result.installed.length ===
0 && result.errors.length === 0 (or simply call onSuccess when installed.length
> 0 OR both arrays are empty), keep invoking callbacks.onError?.(source, ...)
when result.errors.length > 0, and preserve the existing catch handling so the
spinner is always resolved in every path.
In `@packages/nuxi/src/commands/module/add.ts`:
- Around line 262-298: The handlers inside setup currently return early on
isCancel (agentChoice, modeChoice, skillSelection) which aborts the whole setup
and skips the eventual runCommand(prepareCommand, args); change the control flow
to never return from setup on these cancels: introduce a boolean like
skipSkillConfig (default false), set skipSkillConfig = true whenever
isCancel(...) would have returned (for agentChoice, modeChoice, skillSelection),
and replace subsequent code that depends on those choices with guards (e.g.,
only set selectedAgents/selectedMode/selectedSources or call
applySkillSelection/buildSkillGroups when !skipSkillConfig); keep
selectedSources as [] when skipped so the code that logs "No skills selected"
and later steps still run, and ensure runCommand(prepareCommand, args) in setup
always executes regardless of skipSkillConfig.
In `@packages/nuxi/src/commands/module/skills.ts`:
- Around line 16-101: Duplicate helper code (InstallMode, SKILL_VALUE_SEPARATOR,
SKILL_ALL_TOKEN, formatAgentList, formatSkillPlan, buildSkillGroups,
applySkillSelection, applyInstallMode) exists in both skills.ts and add.ts;
extract them to a shared module (e.g., _skill-utils.ts), export the InstallMode
type, constants, and functions, and update skills.ts and add.ts to import those
symbols instead of keeping local copies; when extracting, preserve
signatures/return shapes (e.g., applyInstallMode returns { sources, forcedCopy
}), ensure required types (DetectedAgent, ModuleSkillSource) and
getAgentDisplayNames are imported into the new module, and replace the
duplicated definitions in both files with imports of the exported symbols.
- Around line 220-226: The current module detection (using allDeps, depNames,
knownModules, knownModuleNames and moduleNames) misses scoped community packages
like `@scope/nuxt-`* because name.startsWith('nuxt-') returns false for scoped
names; update the heuristic to also accept names that contain a scope and then
the nuxt- prefix (e.g. test for names matching the pattern /^@[^/]+\/nuxt-/ or
check the part after the slash with startsWith('nuxt-')), or alternatively
iterate depNames and try reading skill metadata for each dependency if the
heuristic fails; apply this change where moduleNames is computed so scoped
nuxt-* packages are included.
In `@packages/nuxi/test/unit/commands/module/add.spec.ts`:
- Around line 228-241: The test currently mocks detectModuleSkills but
getBundledSkillSources(cwd) is invoked earlier and will throw for the fake cwd,
so detectModuleSkills is never exercised; update the test to also mock
getBundledSkillSources (from unagent) to return an empty array (and any other
unagent helpers used like detectInstalledAgents) before calling the add
subcommand so the try block proceeds to call detectModuleSkills and your
mockRejectedValueOnce on detectModuleSkills is actually triggered; reference
getBundledSkillSources and detectModuleSkills to locate the relevant mocks to
add.
🧹 Nitpick comments (7)
packages/nuxi/src/commands/module/skills.ts (1)
128-142: Inconsistent error handling: ENOENT returns silently but other errors exit.When
listInstalledSkills()throws an ENOENT error, the code logs a warning mentioning "Skipping invalid skill entry" and returns — but this message is misleading for the case where no skills directory exists yet (a normal state). Consider using a clearer message like "No skills directory found" for ENOENT, reserving the "invalid entry" wording for genuinely corrupt data.packages/nuxi/src/commands/module/_skills.ts (2)
79-111: Mutating input objects to carry spinner state is fragile.The
_spinnerproperty is monkey-patched onto thesourceobject via type assertions. This works because the same object reference flows through callbacks, but it's error-prone and would break if the objects were cloned between callbacks. Consider using aMap<SkillSource, ReturnType<typeof spinner>>instead.
40-53:getModuleMetasilently swallows all errors.The bare
catchblock returnsnullfor any failure — including unexpected errors like permission issues or out-of-memory. This is intentional for resilience, but consider at least logging atdebuglevel so that module authors can diagnose why their skills aren't being detected.♻️ Proposed improvement
catch { + logger.debug?.(`Failed to load meta for ${pkgName}`) return null }packages/nuxi/test/unit/commands/module/_skills.spec.ts (1)
49-101: Consider adding tests for error and edge-case paths.The current tests cover agent filtering well, but the following paths in
installModuleSkillsare untested:
installSkillthrowing an exception — verifies thecatchblock and that the spinner is stopped.installSkillreturning errors (non-emptyresult.errors) — verifies the "already installed" detection and the warning log.installSkillreturning both emptyinstalledand emptyerrors— would expose the spinner-hang bug noted in the_skills.tsreview.packages/nuxi/test/unit/commands/module/add.spec.ts (1)
116-146: Duplicated mock data between module-level setup andbeforeEach.The
fetchModulesSpy.mockResolvedValue(...)call at lines 89–114 sets up the same module fixture that is immediately overridden bybeforeEachat lines 120–145. The module-level call is redundant.packages/nuxi/test/unit/commands/module/skills.spec.ts (2)
102-116:vi.clearAllMocks()won't restoreprocess.exitspies — userestoreAllMocks()instead.
clearAllMocksresets call history and return values but does not restore the original implementation of spied methods. Theprocess.exitspy created in the first and third tests (with a throwingmockImplementation) will leak across tests. If a future test in this suite unexpectedly triggersprocess.exit, it will throw instead of exiting, producing a confusing failure.Proposed fix
beforeEach(() => { - vi.clearAllMocks() + vi.restoreAllMocks() getAgentDisplayNames.mockReturnValue(['OpenAI Codex CLI (codex)'])Note:
restoreAllMocksalso clears call history, so it's a superset ofclearAllMocks. The hoistedvi.fn()mocks are unaffected since they have no original implementation to restore.
97-99: Type assertion to accesssetupis fine but brittle.If the command shape changes, this cast will silently pass compilation but fail at runtime. Consider adding a brief comment or a type guard so future maintainers know this relies on
defineCommand's internal structure.
| "tsdown": "^0.19.0", | ||
| "typescript": "^5.9.3", | ||
| "ufo": "^1.6.3", | ||
| "unagent": "^0.0.5", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
unagent npm package latest version stability
💡 Result:
I couldn’t verify a current npm package named unagent on the public npm registry from web search results (no npmjs.com/package/unagent listing showed up), so I can’t report a “latest version” or assess release cadence/stability for an npm package with that exact name.
What I did find is “Unagent” as a Safari (iOS/macOS) extension project on GitHub, which is not an npm package. [1]
If you meant a different package (e.g., scoped like @scope/unagent, hyphenated like un-agent, or similarly named), tell me the exact npm name and I’ll look up its latest published version + stability signals (publish date history, weekly downloads, open issues, etc.).
Sources: [1]
🏁 Script executed:
cat -n packages/nuxi/package.json | grep -A 5 -B 5 "unagent"Repository: nuxt/cli
Length of output: 381
🏁 Script executed:
rg "unagent" --type json --type js --type ts -iRepository: nuxt/cli
Length of output: 1248
🏁 Script executed:
find . -name "package.json" -exec grep -l '"name".*:.*"unagent"' {} \;Repository: nuxt/cli
Length of output: 34
🏁 Script executed:
fd -t d unagentRepository: nuxt/cli
Length of output: 34
🏁 Script executed:
cat .npmrc 2>/dev/null || echo "No .npmrc found"Repository: nuxt/cli
Length of output: 67
🏁 Script executed:
rg "\"name\".*:.*unagent|workspace.*unagent" --type jsonRepository: nuxt/cli
Length of output: 34
🏁 Script executed:
cat -i package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null | grep -A 3 "unagent" | head -50Repository: nuxt/cli
Length of output: 34
🏁 Script executed:
git log --oneline -p packages/nuxi/package.json | grep -A 3 -B 3 "unagent" | head -20Repository: nuxt/cli
Length of output: 253
Remove or clarify the non-existent unagent dependency.
unagent does not exist as a public npm package and is not defined as a local workspace package. However, the codebase actively imports from it in packages/nuxi/src/commands/module/{add,skills,_skills}.ts. This will cause module resolution failures at runtime. Either this is a missing private registry configuration, a typo for an actual package name, or the dependency and its usages need to be removed entirely.
🤖 Prompt for AI Agents
In `@packages/nuxi/package.json` at line 77, The package.json lists a non-existent
dependency "unagent" which causes runtime module resolution failures because
code in packages/nuxi/src/commands/module/add.ts, skills.ts and _skills.ts
import from it; either remove "unagent" from package.json and delete or refactor
its imports in those three files, or replace the entry with the correct package
name/registry and update package install configuration; specifically locate
imports of "unagent" in the three modules (import ... from 'unagent'), then
either (A) change those imports to the correct, existing package name used
elsewhere in the repo or a publicly available alternative, or (B) remove the
imports and their usage and implement equivalent logic, or (C) restore/add the
private registry/config that provides "unagent" and document it in package.json
so installs succeed.
| for (const source of sources) { | ||
| callbacks.onStart?.(source) | ||
| try { | ||
| const result = await installSkill({ | ||
| source: source.source, | ||
| skills: source.skills, | ||
| mode: source.mode ?? 'copy', | ||
| agents: effectiveAgentIds, | ||
| }) | ||
| if (result.installed.length > 0) | ||
| callbacks.onSuccess?.(source, result) | ||
| if (result.errors.length > 0) | ||
| callbacks.onError?.(source, result.errors.map(e => e.error).join(', ')) | ||
| } | ||
| catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error) | ||
| callbacks.onError?.(source, message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Spinner never stopped when installSkill returns empty results.
If result.installed and result.errors are both empty arrays, neither onSuccess nor onError is called, leaving the spinner running indefinitely. The else branch in onSuccess (line 97–99) that would stop the spinner with "No skills to install" is dead code since onSuccess is only called when result.installed.length > 0.
🐛 Proposed fix
for (const source of sources) {
callbacks.onStart?.(source)
try {
const result = await installSkill({
source: source.source,
skills: source.skills,
mode: source.mode ?? 'copy',
agents: effectiveAgentIds,
})
- if (result.installed.length > 0)
- callbacks.onSuccess?.(source, result)
- if (result.errors.length > 0)
+ if (result.installed.length > 0) {
+ callbacks.onSuccess?.(source, result)
+ }
+ if (result.errors.length > 0) {
callbacks.onError?.(source, result.errors.map(e => e.error).join(', '))
+ }
+ if (result.installed.length === 0 && result.errors.length === 0) {
+ const info = source as ModuleSkillSource & { _spinner: ReturnType<typeof spinner> }
+ info._spinner?.stop('No skills to install')
+ }
}
catch (error) {
const message = error instanceof Error ? error.message : String(error)
callbacks.onError?.(source, message)
}
}📝 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.
| for (const source of sources) { | |
| callbacks.onStart?.(source) | |
| try { | |
| const result = await installSkill({ | |
| source: source.source, | |
| skills: source.skills, | |
| mode: source.mode ?? 'copy', | |
| agents: effectiveAgentIds, | |
| }) | |
| if (result.installed.length > 0) | |
| callbacks.onSuccess?.(source, result) | |
| if (result.errors.length > 0) | |
| callbacks.onError?.(source, result.errors.map(e => e.error).join(', ')) | |
| } | |
| catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| callbacks.onError?.(source, message) | |
| } | |
| } | |
| for (const source of sources) { | |
| callbacks.onStart?.(source) | |
| try { | |
| const result = await installSkill({ | |
| source: source.source, | |
| skills: source.skills, | |
| mode: source.mode ?? 'copy', | |
| agents: effectiveAgentIds, | |
| }) | |
| if (result.installed.length > 0) { | |
| callbacks.onSuccess?.(source, result) | |
| } | |
| if (result.errors.length > 0) { | |
| callbacks.onError?.(source, result.errors.map(e => e.error).join(', ')) | |
| } | |
| if (result.installed.length === 0 && result.errors.length === 0) { | |
| const info = source as ModuleSkillSource & { _spinner: ReturnType<typeof spinner> } | |
| info._spinner?.stop('No skills to install') | |
| } | |
| } | |
| catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| callbacks.onError?.(source, message) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/nuxi/src/commands/module/_skills.ts` around lines 113 - 131, The
loop calling installSkill checks result.installed and result.errors but
currently only invokes callbacks.onSuccess when installed.length>0, so the
spinner isn't stopped if both arrays are empty; change the post-install logic in
the for loop (the block handling the result from installSkill) to treat the
"both empty" case as a success/empty case by invoking
callbacks.onSuccess?.(source, result) when result.installed.length === 0 &&
result.errors.length === 0 (or simply call onSuccess when installed.length > 0
OR both arrays are empty), keep invoking callbacks.onError?.(source, ...) when
result.errors.length > 0, and preserve the existing catch handling so the
spinner is always resolved in every path.
| if (isCancel(agentChoice)) | ||
| return | ||
|
|
||
| if (agentChoice !== '__all__') | ||
| selectedAgents = [agentChoice] | ||
|
|
||
| const modeChoice = await select({ | ||
| message: 'Install mode:', | ||
| options: [ | ||
| { value: 'auto', label: 'Auto', hint: 'symlink local, copy remote' }, | ||
| { value: 'copy', label: 'Copy', hint: 'copy into agent skill dir' }, | ||
| { value: 'symlink', label: 'Symlink', hint: 'link to source (local only)' }, | ||
| ], | ||
| initialValue: 'auto', | ||
| }) | ||
|
|
||
| if (isCancel(modeChoice)) | ||
| return | ||
|
|
||
| selectedMode = modeChoice as InstallMode | ||
|
|
||
| const { groups, initialValues } = buildSkillGroups(skillInfos) | ||
| const skillSelection = await groupMultiselect({ | ||
| message: 'Select skills to install:', | ||
| options: groups, | ||
| initialValues, | ||
| required: false, | ||
| }) | ||
|
|
||
| if (isCancel(skillSelection)) | ||
| return | ||
|
|
||
| selectedSources = applySkillSelection(skillInfos, skillSelection as string[]) | ||
| if (selectedSources.length === 0) { | ||
| logger.info('No skills selected') | ||
| return | ||
| } |
There was a problem hiding this comment.
Cancelling skill configuration skips nuxt prepare.
When the user enters the "Change configuration" flow and then cancels any of the sub-prompts (agent choice, mode choice, or skill selection), the return statements on lines 263, 279, 292, and 297 exit the entire setup function. This skips the runCommand(prepareCommand, args) call on line 319, leaving the project in an inconsistent state — the module dependency is installed and nuxt.config is updated, but nuxt prepare never runs.
These cancellations should fall through to the prepare step instead of returning early.
🐛 Proposed fix — replace early returns with a flag or restructure control flow
One approach: use a skipSkills flag instead of returning:
if (action === 'config') {
const agentChoice = await select({
message: 'Which AI agent should receive the skills?',
options: [
{ value: '__all__', label: 'All detected agents', hint: 'default' },
...detectedAgents.map(agent => ({
value: agent.id,
label: `${agent.config.name} (${agent.id})`,
})),
],
initialValue: '__all__',
})
- if (isCancel(agentChoice))
- return
+ if (isCancel(agentChoice)) {
+ // User cancelled — skip skills but continue to prepare
+ }
+ else {
if (agentChoice !== '__all__')
selectedAgents = [agentChoice]
// ... rest of config flow ...
+ } // close else
}Alternatively, wrap the entire skill-install block so that the prepare command always runs regardless:
+ // Skill installation (non-blocking — prepare always runs after)
+ try {
if (skillInfos.length > 0) {
// ... skill flow ...
}
+ }
+ catch { /* skill errors don't block prepare */ }
// Run prepare command
const args = ...
await runCommand(prepareCommand, args)🤖 Prompt for AI Agents
In `@packages/nuxi/src/commands/module/add.ts` around lines 262 - 298, The
handlers inside setup currently return early on isCancel (agentChoice,
modeChoice, skillSelection) which aborts the whole setup and skips the eventual
runCommand(prepareCommand, args); change the control flow to never return from
setup on these cancels: introduce a boolean like skipSkillConfig (default
false), set skipSkillConfig = true whenever isCancel(...) would have returned
(for agentChoice, modeChoice, skillSelection), and replace subsequent code that
depends on those choices with guards (e.g., only set
selectedAgents/selectedMode/selectedSources or call
applySkillSelection/buildSkillGroups when !skipSkillConfig); keep
selectedSources as [] when skipped so the code that logs "No skills selected"
and later steps still run, and ensure runCommand(prepareCommand, args) in setup
always executes regardless of skipSkillConfig.
| type InstallMode = 'auto' | 'copy' | 'symlink' | ||
| const SKILL_VALUE_SEPARATOR = '::' | ||
| const SKILL_ALL_TOKEN = '*' | ||
|
|
||
| function formatAgentList(agents: DetectedAgent[]): string { | ||
| if (agents.length === 0) | ||
| return 'none' | ||
| return getAgentDisplayNames(agents).join(', ') | ||
| } | ||
|
|
||
| function formatSkillPlan(agents: DetectedAgent[], sources: ModuleSkillSource[]): string { | ||
| const lines = [ | ||
| `Agents: ${formatAgentList(agents)}`, | ||
| 'Mode: auto (symlink local, copy remote)', | ||
| 'Skills:', | ||
| ...sources.map((source) => { | ||
| const skills = source.skills?.length ? source.skills.join(', ') : 'all' | ||
| return `- ${source.moduleName}: ${skills}` | ||
| }), | ||
| ] | ||
| return lines.join('\n') | ||
| } | ||
|
|
||
| function buildSkillGroups(sources: ModuleSkillSource[]) { | ||
| const groups: Record<string, Array<{ label: string, value: string, hint?: string }>> = {} | ||
| const initialValues: string[] = [] | ||
|
|
||
| for (const source of sources) { | ||
| const skills = source.skills?.length ? source.skills : null | ||
| const options = skills | ||
| ? skills.map(skill => ({ label: skill, value: `${source.moduleName}${SKILL_VALUE_SEPARATOR}${skill}` })) | ||
| : [{ label: 'all', value: `${source.moduleName}${SKILL_VALUE_SEPARATOR}${SKILL_ALL_TOKEN}`, hint: 'includes all skills' }] | ||
| groups[source.moduleName] = options | ||
| initialValues.push(...options.map(option => option.value)) | ||
| } | ||
|
|
||
| return { groups, initialValues } | ||
| } | ||
|
|
||
| function applySkillSelection(sources: ModuleSkillSource[], selectedValues: string[]) { | ||
| const selectedByModule = new Map<string, { all: boolean, skills: Set<string> }>() | ||
|
|
||
| for (const value of selectedValues) { | ||
| const [moduleName, skillName] = value.split(SKILL_VALUE_SEPARATOR) | ||
| const entry = selectedByModule.get(moduleName) || { all: false, skills: new Set<string>() } | ||
| if (skillName === SKILL_ALL_TOKEN) | ||
| entry.all = true | ||
| else if (skillName) | ||
| entry.skills.add(skillName) | ||
| selectedByModule.set(moduleName, entry) | ||
| } | ||
|
|
||
| const selectedSources: ModuleSkillSource[] = [] | ||
| for (const source of sources) { | ||
| const selected = selectedByModule.get(source.moduleName) | ||
| if (!selected) | ||
| continue | ||
| if (selected.all) { | ||
| selectedSources.push({ ...source, skills: undefined }) | ||
| continue | ||
| } | ||
| const sourceSkills = source.skills?.length ? source.skills : [] | ||
| const filtered = sourceSkills.filter(skill => selected.skills.has(skill)) | ||
| if (filtered.length === 0) | ||
| continue | ||
| selectedSources.push({ ...source, skills: filtered }) | ||
| } | ||
|
|
||
| return selectedSources | ||
| } | ||
|
|
||
| function applyInstallMode(sources: ModuleSkillSource[], mode: InstallMode) { | ||
| if (mode === 'auto') | ||
| return { sources, forcedCopy: false } | ||
|
|
||
| let forcedCopy = false | ||
| const next = sources.map((source) => { | ||
| if (mode === 'symlink' && !source.isLocal) { | ||
| forcedCopy = true | ||
| return { ...source, mode: 'copy' as const } | ||
| } | ||
| return { ...source, mode } | ||
| }) | ||
|
|
||
| return { sources: next, forcedCopy } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication with add.ts.
The helper functions formatAgentList, formatSkillPlan, buildSkillGroups, applySkillSelection, applyInstallMode, the InstallMode type, and the SKILL_VALUE_SEPARATOR/SKILL_ALL_TOKEN constants are duplicated verbatim between this file and add.ts (lines 48–133). Extract these into a shared module (e.g., _skills.ts or a new _skill-utils.ts) to avoid drift between the two copies.
♻️ Suggested approach
Move the shared code into a common module, for example _skill-utils.ts:
// packages/nuxi/src/commands/module/_skill-utils.ts
import type { DetectedAgent } from 'unagent'
import type { ModuleSkillSource } from './_skills'
import { getAgentDisplayNames } from 'unagent'
export type InstallMode = 'auto' | 'copy' | 'symlink'
export const SKILL_VALUE_SEPARATOR = '::'
export const SKILL_ALL_TOKEN = '*'
export function formatAgentList(agents: DetectedAgent[]): string { /* ... */ }
export function formatSkillPlan(agents: DetectedAgent[], sources: ModuleSkillSource[]): string { /* ... */ }
export function buildSkillGroups(sources: ModuleSkillSource[]) { /* ... */ }
export function applySkillSelection(sources: ModuleSkillSource[], selectedValues: string[]) { /* ... */ }
export function applyInstallMode(sources: ModuleSkillSource[], mode: InstallMode) { /* ... */ }Then import from both skills.ts and add.ts.
🤖 Prompt for AI Agents
In `@packages/nuxi/src/commands/module/skills.ts` around lines 16 - 101, Duplicate
helper code (InstallMode, SKILL_VALUE_SEPARATOR, SKILL_ALL_TOKEN,
formatAgentList, formatSkillPlan, buildSkillGroups, applySkillSelection,
applyInstallMode) exists in both skills.ts and add.ts; extract them to a shared
module (e.g., _skill-utils.ts), export the InstallMode type, constants, and
functions, and update skills.ts and add.ts to import those symbols instead of
keeping local copies; when extracting, preserve signatures/return shapes (e.g.,
applyInstallMode returns { sources, forcedCopy }), ensure required types
(DetectedAgent, ModuleSkillSource) and getAgentDisplayNames are imported into
the new module, and replace the duplicated definitions in both files with
imports of the exported symbols.
| const allDeps = { ...pkg.dependencies, ...pkg.devDependencies } | ||
| const depNames = Object.keys(allDeps) | ||
| const knownModules = await fetchModules().catch(() => []) | ||
| const knownModuleNames = new Set(knownModules.map(m => m.npm)) | ||
| const moduleNames = depNames.filter(name => | ||
| knownModuleNames.has(name) || name.startsWith('@nuxt/') || name.startsWith('nuxt-'), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the filtering logic for scoped packages like `@onmax/nuxt-better-auth`
echo "Testing: does '@onmax/nuxt-better-auth'.startsWith('nuxt-') ?"
node -e "console.log('@onmax/nuxt-better-auth'.startsWith('nuxt-'))"
echo "Testing: does '@onmax/nuxt-better-auth'.startsWith('@nuxt/') ?"
node -e "console.log('@onmax/nuxt-better-auth'.startsWith('@nuxt/'))"
echo "Additional test: does '@nuxt/example'.startsWith('@nuxt/') ?"
node -e "console.log('@nuxt/example'.startsWith('@nuxt/'))"
echo "Additional test: does 'nuxt-example'.startsWith('nuxt-') ?"
node -e "console.log('nuxt-example'.startsWith('nuxt-'))"Repository: nuxt/cli
Length of output: 318
🏁 Script executed:
# Read the actual file to see the full context
head -n 240 packages/nuxi/src/commands/module/skills.ts | tail -n 30Repository: nuxt/cli
Length of output: 1266
Scoped packages with nuxt- naming won't be detected by the current filter.
The filter name.startsWith('@nuxt/') || name.startsWith('nuxt-') will skip scoped packages like @onmax/nuxt-better-auth that aren't in the knownModules registry. Since '@onmax/nuxt-better-auth'.startsWith('nuxt-') evaluates to false, community scoped modules following the @scope/nuxt-* pattern will be excluded unless they're explicitly registered.
Consider checking all dependencies for skills metadata or expanding the heuristic to account for scoped packages with nuxt-related naming.
🤖 Prompt for AI Agents
In `@packages/nuxi/src/commands/module/skills.ts` around lines 220 - 226, The
current module detection (using allDeps, depNames, knownModules,
knownModuleNames and moduleNames) misses scoped community packages like
`@scope/nuxt-`* because name.startsWith('nuxt-') returns false for scoped names;
update the heuristic to also accept names that contain a scope and then the
nuxt- prefix (e.g. test for names matching the pattern /^@[^/]+\/nuxt-/ or check
the part after the slash with startsWith('nuxt-')), or alternatively iterate
depNames and try reading skill metadata for each dependency if the heuristic
fails; apply this change where moduleNames is computed so scoped nuxt-* packages
are included.
| it('should continue module add when skill discovery fails', async () => { | ||
| vi.spyOn(moduleSkills, 'detectModuleSkills').mockRejectedValueOnce(new Error('broken skill scanner')) | ||
|
|
||
| const addCommand = await (commands as CommandsType).subCommands.add() | ||
| await addCommand.setup({ | ||
| args: { | ||
| cwd: '/fake-dir', | ||
| _: ['content'], | ||
| }, | ||
| }) | ||
|
|
||
| expect(addDependency).toHaveBeenCalled() | ||
| expect(runCommands.runCommand).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
Test may not exercise the intended failure path.
getBundledSkillSources(cwd) (line 202 in add.ts) is called before detectModuleSkills inside the same try block. Since cwd is /fake-dir (non-existent), getBundledSkillSources likely throws first, and the catch at line 220 handles it — meaning detectModuleSkills is never reached and the mock on line 229 has no effect.
The test still validates the right user-facing behavior (skill failure doesn't block add), but to specifically test detectModuleSkills failure, you'd also need to mock getBundledSkillSources from unagent to return an empty array.
🧪 Proposed fix to actually test detectModuleSkills failure
Add a mock for getBundledSkillSources (e.g., in the hoisted mocks or inline):
it('should continue module add when skill discovery fails', async () => {
+ // Ensure bundled scan succeeds so detectModuleSkills is actually reached
+ vi.mocked(getBundledSkillSources).mockReturnValue([])
vi.spyOn(moduleSkills, 'detectModuleSkills').mockRejectedValueOnce(new Error('broken skill scanner'))
// ...
})This requires also mocking unagent in this test file (at least getBundledSkillSources and detectInstalledAgents).
🤖 Prompt for AI Agents
In `@packages/nuxi/test/unit/commands/module/add.spec.ts` around lines 228 - 241,
The test currently mocks detectModuleSkills but getBundledSkillSources(cwd) is
invoked earlier and will throw for the fake cwd, so detectModuleSkills is never
exercised; update the test to also mock getBundledSkillSources (from unagent) to
return an empty array (and any other unagent helpers used like
detectInstalledAgents) before calling the add subcommand so the try block
proceeds to call detectModuleSkills and your mockRejectedValueOnce on
detectModuleSkills is actually triggered; reference getBundledSkillSources and
detectModuleSkills to locate the relevant mocks to add.
1 participant
nuxt module add, check if module exposes agent skills viameta.agents.skillsnpx skills addRelated