Conversation
No changeset foundThis PR does not add a changeset, so it will not appear in the changelog or trigger a release. If the change is user-facing, add one: pnpm changesetPick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated If this PR is docs-only or a chore that needs no release note, you can ignore this — or run |
nc-review: no blocking issuesPR changes the fallback SkillPriority in synthesizeCommandSkills from 'project' to 'built-in' for commands whose source is neither 'project' nor 'personal'. The change is type-safe (SkillPriority includes 'built-in') and does not break the contract, but the else branch it modifies is unreachable in the current pipeline: synthesizeCommandSkills runs against loader.getAllCommands() AFTER CustomCommandLoader.loadCommands() populates from disk (which always sets source) but BEFORE registerSkills adds bundle commands (the only path that can produce commands with source: undefined). No regression test was added, and the change diverges from the codebase's existing fallback convention in source/commands/custom-commands.tsx (line 186), which uses 'project' as the source fallback. These are worth raising but not blocking.
Automated code review — correctness, security, design, tests, plus duplicates and scope. Advisory: a human still decides. Not a substitute for review, and not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with |
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for picking this up, but I don't think this closes #1134.
CustomCommand.source is typed 'personal' | 'project' and loadCommands() always passes one of them (source/custom-commands/loader.ts:45,50), so the fallback branch is only reached when source is undefined. The commands that actually have no source are bundle members: bundle-loader.ts:276-282 builds the CustomCommand without one and registrar.ts:76-89 registers it verbatim. Their real level is the bundle layer's priority, usually project, so mapping them to built-in is less accurate than the current default, not more. Today it's invisible only because synthesizeCommandSkills runs before registerSkills in bootSkillPipeline.
What I'd like instead:
- Propagate the true source for bundle commands in the registrar, mirroring how the subagent branch maps
skill.source.prioritythroughsubagentPriorityFor. That likely means wideningCustomCommand.sourceto include'built-in'and making it required. - With that in place the ternary collapses to
const priority: SkillPriority = command.source ?? 'built-in';, which is exhaustive by construction so a future source variant becomes a type error rather than a silent default. source/commands/custom-commands.tsx:186-187still doescommand.source ?? 'project'. Whatever default wins should apply there too, otherwise/skillsand/commandslabel the same command differently.- Please add a test asserting that a project bundle's command shows project priority in
/skills.
Also: the branch needs a rebase on main (Config Schema Freshness fails on a missing generate:schema script and the coverage drift check is comparing against a moved baseline, neither is caused by this diff), and a patch changeset naming @nanocollective/nanocoder would be good for a user-visible labelling change.
will-lamerton
left a comment
There was a problem hiding this comment.
Closer. The type widening and the /commands alignment are right. Two things still block:
-
bundle-loader.ts:283hardcodessource: 'project'.loadBundlealready has the layerpriority(line 172), so please thread it intoloadCommandMembersand setsource: priority. As written, a personal bundle under~/.config/nanocoder/skills/and a built-in bundle both reportproject, which is the same mislabelling in a new place.registrar.ts:96already does this correctly for the subagent branch viasubagentPriorityFor. -
The new test passes with the
bundle-loader.tschange reverted, I checked locally.foo?.source.priorityreads the bundle Skill's layer priority, not the command member's, so it is green either way. Asserting the command's priority meanssynthesizeCommandSkillshas to run afterregisterSkills(bootstrap.ts:110vs:138), or be exported and tested directly.
Two smaller things:
sourceis required now, so the?? 'built-in'is dead inbootstrap.ts:217andcustom-commands.tsx:186-187.const priority: SkillPriority = command.source;is the exhaustive form, which is what makes a future source variant a type error instead of a silent default.- Could you revert the formatting-only hunks in
bootstrap.spec.ts(double quotes, bracket spacing, wrapped arrow params)? Spec files are biome-excluded so CI stays green, but ~100 lines of churn buries the 20 lines of real change.
Then a rebase on main and a patch changeset naming @nanocollective/nanocoder and this is good to go.
|
Hi @Forzun, thanks for this PR! It looks like a maintainer has left feedback Whenever you get a chance, could you take a look at the open comments? |
|
Hi @Forzun, just a heads-up that this PR still has outstanding items If we don't hear back within about a week, we'll close it to keep the queue tidy. |
Fixes the issue where custom commands without a defined source were incorrectly
classified as
projectskills.Changes
SkillPriorityfromprojecttobuilt-in.fix:#1134