feat(cli): inline skills via defineSkill/skillFromFile; drop dir auto-discovery#1039
Merged
Conversation
…-discovery
Skills are now declared explicitly in `defineAgent({ skills: [...] })` instead of
being auto-discovered from `./skills` + `<agentDir>/skills`. Build a skill two
ways, both producing the same object:
- defineSkill({ name, content, network, nixPackages, mcpServers }) — inline
- skillFromFile("./path") — reads a SKILL.md (frontmatter + body)
The apply loader resolves both into the existing SkillConfig shape (deduped by
name), so the worker, gateway, and DB are unchanged. Removes the magic dir-walk
(loadSkillFiles/buildLocalSkills); init-from-org now emits explicit
skillFromFile refs, and the office-bot example is migrated.
📝 WalkthroughWalkthroughThis PR adds typed Skill APIs ( ChangesSkills API: Explicit declarative skill configuration
sequenceDiagram
participant Config as TypedConfig
participant Resolver as resolveAgentSkills
participant ReadFile as readSkillFile
participant Convert as skillToConfig
participant State as DesiredState
Config->>Resolver: agent.skills entries
Resolver->>ReadFile: skillFromFile(path)
ReadFile->>Convert: parsed SKILL.md frontmatter + content
Resolver->>Convert: defineSkill inline entries
Convert->>State: SkillConfig[] merged into agent artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/_lib/apply/desired-state.ts`:
- Around line 378-380: The fallback name logic in the assignment to the local
variable name (used by skillFromFile) currently derives the name from the file
basename, causing SKILL.md to yield "SKILL" instead of the containing folder
name; update the fallback so that when the file basename (after stripping .md)
equals "SKILL" you use the parent folder's basename (via path.dirname +
basename) as the fallback, otherwise keep using the file basename; preserve
precedence of nameOverride and frontmatter?.name and only change the final
fallback used when frontmatter?.name is missing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1ee6d30-6e07-4a98-8616-835cbbd532c6
📒 Files selected for processing (6)
examples/office-bot/lobu.config.tspackages/cli/src/commands/_lib/apply/__tests__/load-config.test.tspackages/cli/src/commands/_lib/apply/desired-state.tspackages/cli/src/commands/_lib/apply/map-config.tspackages/cli/src/commands/_lib/init-from-org/bootstrap.tspackages/cli/src/config/define.ts
Comment on lines
+378
to
+380
| const name = | ||
| nameOverride ?? frontmatter?.name ?? basename(abs.replace(/\.md$/, "")); | ||
| return { name, content: body, ...(frontmatter ? { fm: frontmatter } : {}) }; |
There was a problem hiding this comment.
Fix fallback name inference for skillFromFile(".../SKILL.md").
At Line 379, the fallback infers SKILL from the filename, but the contract says fallback should come from the folder name when frontmatter name is missing. This can create false duplicate-name errors across multiple SKILL.md files.
Proposed fix
-import { basename, isAbsolute, join, relative, resolve, sep } from "node:path";
+import {
+ basename,
+ dirname,
+ isAbsolute,
+ join,
+ relative,
+ resolve,
+ sep,
+} from "node:path";
@@
- const name =
- nameOverride ?? frontmatter?.name ?? basename(abs.replace(/\.md$/, ""));
+ const inferredName = abs.endsWith(".md")
+ ? basename(abs, ".md").toLowerCase() === "skill"
+ ? basename(dirname(abs))
+ : basename(abs, ".md")
+ : basename(abs);
+ const name = nameOverride ?? frontmatter?.name ?? inferredName;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/_lib/apply/desired-state.ts` around lines 378 -
380, The fallback name logic in the assignment to the local variable name (used
by skillFromFile) currently derives the name from the file basename, causing
SKILL.md to yield "SKILL" instead of the containing folder name; update the
fallback so that when the file basename (after stripping .md) equals "SKILL" you
use the parent folder's basename (via path.dirname + basename) as the fallback,
otherwise keep using the file basename; preserve precedence of nameOverride and
frontmatter?.name and only change the final fallback used when frontmatter?.name
is missing.
Member
Author
|
bug_free 62, simplicity 78, slop 6, bugs 1, 0 blockers Typecheck/unit passed. [env] Integration failed because DATABASE_URL points at unsafe database "postgres", not a test DB. Explored inline defineSkill with MCP headers/oauth; loadDesiredStateFromConfig produced only url/type, dropping auth fields. Suggested fixes
Full verdict JSON{
"bug_free_confidence": 62,
"bugs": 1,
"slop": 6,
"simplicity": 78,
"blockers": [],
"change_type": "feat",
"behavior_change_risk": "medium",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/cli/src/commands/_lib/apply/desired-state.ts",
"line": 345,
"change": "Preserve the full supported skill MCP server shape when converting defineSkill/skillFromFile entries, including headers, oauth, inputs/name, and collect secret/$VAR refs for inline skill MCP credentials."
},
{
"file": "packages/cli/src/commands/_lib/apply/map-config.ts",
"line": 268,
"change": "When merging skill MCP servers into agent settings, copy supported fields beyond url/type/command/args, especially headers and oauth, so inline skill MCP auth is not silently dropped."
}
],
"notes": "Typecheck/unit passed. [env] Integration failed because DATABASE_URL points at unsafe database \"postgres\", not a test DB. Explored inline defineSkill with MCP headers/oauth; loadDesiredStateFromConfig produced only url/type, dropping auth fields.",
"categories": {
"src": 372,
"tests": 119,
"docs": 63,
"config": 4,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
Address review: examples/lobu-crm relied on the removed skill auto-walk and silently lost its crm-ops skill — migrate it to skillFromFile like office-bot. Update the docs that described folder auto-discovery (skills.mdx, skill-md.md, lobu-config.md, agent-prompts.md) to the explicit defineSkill/skillFromFile workflow.
defineSkill accepted the full agent McpServer (headers/oauth/env), but the skill MCP pipeline only ever wires url/type/command/args (true for the SKILL.md frontmatter path too), so auth fields were silently dropped. Introduce a narrow SkillMcpServer authoring type so the API no longer promises fields it drops; servers needing auth belong on the agent's mcpServers. Adds a merge test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/commands/_lib/apply/__tests__/load-config.test.ts (1)
236-264: ⚡ Quick winAdd a coverage case for
type: "streamable-http"in skill MCP mergeGiven
SkillMcpServer.typeincludes"streamable-http", this test should also validate that variant is preserved in loaded agent settings. It will guard the public contract and catch silent type dropping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/_lib/apply/__tests__/load-config.test.ts` around lines 236 - 264, Update the test that asserts MCP server merging to include the "streamable-http" variant: when creating the inline skill via defineSkill (the `api` constant) add an MCP entry whose type is "streamable-http" (or change the existing mcpServers entry to use "streamable-http"), then call loadDesiredStateFromConfig and assert against state.agents[0].settings.mcpServers (the `mcp` variable) that the entry preserves type: "streamable-http" along with its url using the existing expect assertion pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/config/define.ts`:
- Around line 311-316: SkillMcpServer.type currently includes "streamable-http"
but the resolver that builds SkillConfigEntry (in the desired-state resolution
code) narrows only to "sse" | "stdio", losing "streamable-http"; either remove
"streamable-http" from the public SkillMcpServer.type or update the resolver and
SkillConfigEntry handling to preserve it end-to-end: in practice, update the
type union for SkillConfigEntry to include "streamable-http", adjust the
narrowing/merge logic in the function that converts SkillMcpServer ->
SkillConfigEntry (the resolver in desired-state.ts) to accept and pass through
"streamable-http", and ensure any merging/validation code copies relevant fields
(url/command/args) for that variant instead of dropping it.
---
Nitpick comments:
In `@packages/cli/src/commands/_lib/apply/__tests__/load-config.test.ts`:
- Around line 236-264: Update the test that asserts MCP server merging to
include the "streamable-http" variant: when creating the inline skill via
defineSkill (the `api` constant) add an MCP entry whose type is
"streamable-http" (or change the existing mcpServers entry to use
"streamable-http"), then call loadDesiredStateFromConfig and assert against
state.agents[0].settings.mcpServers (the `mcp` variable) that the entry
preserves type: "streamable-http" along with its url using the existing expect
assertion pattern.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94a51136-d66d-4424-a707-ef9b3bb665c4
📒 Files selected for processing (2)
packages/cli/src/commands/_lib/apply/__tests__/load-config.test.tspackages/cli/src/config/define.ts
Comment on lines
+311
to
+316
| export interface SkillMcpServer { | ||
| url?: string; | ||
| command?: string; | ||
| args?: string[]; | ||
| type?: "sse" | "streamable-http" | "stdio"; | ||
| } |
There was a problem hiding this comment.
SkillMcpServer.type currently over-promises a mode that is dropped at resolution
SkillMcpServer allows "streamable-http", but the resolver path in packages/cli/src/commands/_lib/apply/desired-state.ts currently narrows to "sse" | "stdio" when building SkillConfigEntry. That silently discards "streamable-http" skills at apply time.
Please align both layers: either remove "streamable-http" from this public type, or preserve it end-to-end in skill resolution/merge.
Also applies to: 349-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/config/define.ts` around lines 311 - 316,
SkillMcpServer.type currently includes "streamable-http" but the resolver that
builds SkillConfigEntry (in the desired-state resolution code) narrows only to
"sse" | "stdio", losing "streamable-http"; either remove "streamable-http" from
the public SkillMcpServer.type or update the resolver and SkillConfigEntry
handling to preserve it end-to-end: in practice, update the type union for
SkillConfigEntry to include "streamable-http", adjust the narrowing/merge logic
in the function that converts SkillMcpServer -> SkillConfigEntry (the resolver
in desired-state.ts) to accept and pass through "streamable-http", and ensure
any merging/validation code copies relevant fields (url/command/args) for that
variant instead of dropping it.
This was referenced May 25, 2026
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
2 participants
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.
What
Skills are now declared explicitly on the agent instead of being auto-discovered from magic folders. Two constructors, one object:
defineSkill({...})— inline:contentis the body, the rest is JSON frontmatter.skillFromFile("./path")— reads aSKILL.md(a dir holding one, or a.mdpath); the loader fills the fields from its frontmatter + body at apply time.Both produce the same
Skill;skills: []is a flat list, deduped by name.Why
The old model auto-walked
./skills+<agentDir>/skillsand loaded everything into every agent — invisible in config, no selective control, no way to share or generate. This replaces that one magic codepath with an explicit list (the Flue-style separation: convention is gone, selection is explicit). It also matches the existing pattern for watcher reaction scripts (a path resolved post-mapper).SOUL/IDENTITY/USER.mdstay convention-loaded from the agentdir— they're a fixed bundle with nothing to choose, so explicit refs there would be pure ceremony.How it stays small
The resolver lowers inline + file skills into the existing
SkillConfigshape, so the worker, gateway, instruction-service, and DB are all unchanged. Skill frontmatter (network/nix/mcp) still merges into the agent's worker sandbox at apply time — which is why skills resolve eagerly rather than at worker boot.Removed:
loadSkillFiles/buildLocalSkills(the dir-walk).init-from-orgnow emits explicitskillFromFile(...)refs alongside theSKILL.mdfiles it writes, so generated projects still round-trip.Verification
bun test load-config init-from-org→ 25 pass (new tests: inline skill, file skill, shared-by-two-agents, duplicate-name rejection, missing-file error; round-trip test exercises the new emission).bun run typecheck(strict, matches Dockerfile) → clean.biome check(repo config) → clean.examples/office-botresolvesdeliveroo-orderfromfile/, with body (2731b),judges: deliveroo, andnix: chromiumall parsed from its SKILL.md frontmatter.Note
SKILL.mdbundled sibling scripts (e.g. office-bot'sdeliveroo.ts) were never synced to the worker (onlySKILL.mdcontent is) — that's unchanged here, and remains a separate, unimplemented capability.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests