feat(skills): inject skill to help configure kaiden workspaces#491
Open
feloy wants to merge 5 commits into
Open
feat(skills): inject skill to help configure kaiden workspaces#491feloy wants to merge 5 commits into
feloy wants to merge 5 commits into
Conversation
Introduces a skills/ package that embeds user-facing skill directories into the kdn binary at compile time. manager.Add() calls ExtractAll() to extract and mount all built-in skills automatically for agents that support them; user-configured skills with the same basename override the built-in version. The first built-in skill, config-kdn-workspace, helps agents running inside a workspace configure environment variables, mounts, secrets, MCP servers, network policies, and other settings interactively. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR adds an embedded built-in skills system (skills package + ExtractAll), wires extraction into Manager.Add to auto-inject built-in skills into merged workspace config, introduces the config-kdn-workspace skill, updates docs, and adds tests for extraction and injection behavior. ChangesBuilt-in Skills System & Configuration Skill
Sequence Diagram(s)sequenceDiagram
participant Client
participant Manager
participant skillsPkg as skills.ExtractAll
participant Storage as storageDir
Client->>Manager: Manager.Add(selectedAgent, workspace)
alt agent supports skills
Manager->>skillsPkg: ExtractAll(storageDir)
skillsPkg-->>Storage: write storageDir/skills/<name> (0755/0644)
skillsPkg-->>Manager: []string (extractedPaths)
Manager->>Manager: merge/dedupe extractedPaths into mergedConfig.Skills
Manager->>Client: create/run instance with augmented mounts
else agent no skills
Manager->>Client: create/run instance without built-in skill mounts
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. 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: 1
🧹 Nitpick comments (1)
pkg/instances/manager_test.go (1)
4135-4168: ⚡ Quick winConsider verifying that the user's skill path takes precedence over the built-in.
The test correctly verifies that no duplication occurs (count == 1), but it doesn't assert which skill is mounted. According to the PR objectives, "the user-configured skill overrides the built-in version." If the implementation incorrectly mounted only the built-in skill and ignored the user's, this test would still pass.
Suggested enhancement to verify user override behavior
count := 0 + var mountedPath string for _, m := range *params.WorkspaceConfig.Mounts { if filepath.Base(m.Host) == "config-kdn-workspace" { count++ + mountedPath = m.Host } } if count != 1 { t.Errorf("Expected exactly 1 config-kdn-workspace mount, got %d", count) } + if mountedPath != userSkillPath { + t.Errorf("Expected user skill path %q to be mounted, got %q", userSkillPath, mountedPath) + }🤖 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 `@pkg/instances/manager_test.go` around lines 4135 - 4168, Update the test "built-in skill is not duplicated when user configures same basename" to also assert that the mounted skill is the user's path (userSkillPath) rather than the built-in; after collecting the matching mounts from params := spy.LastCreateParams() and iterating params.WorkspaceConfig.Mounts, verify at least one mount has Host equal to userSkillPath (or prefer exact equality) and fail if the only matching mount points to a built-in location; use the existing userSkillPath, params.WorkspaceConfig.Mounts, and filepath.Base checks to locate and compare the mounted Host to ensure user-configured skill takes precedence.
🤖 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 `@README.md`:
- Line 33: Update the README wording to avoid saying built-in skills are present
in "every workspace" and instead clarify they are injected only when agent skill
support is enabled (i.e., when SkillsDir() != ""), e.g., change the sentence
referencing "Built-in skills automatically available in every workspace —
including `config-kdn-workspace`..." to indicate built-in skills are available
only in workspaces where SkillsDir() is set/enabled; apply the same rewording to
the repeated section around lines 1901-1907 so both references correctly reflect
the conditional injection behavior.
---
Nitpick comments:
In `@pkg/instances/manager_test.go`:
- Around line 4135-4168: Update the test "built-in skill is not duplicated when
user configures same basename" to also assert that the mounted skill is the
user's path (userSkillPath) rather than the built-in; after collecting the
matching mounts from params := spy.LastCreateParams() and iterating
params.WorkspaceConfig.Mounts, verify at least one mount has Host equal to
userSkillPath (or prefer exact equality) and fail if the only matching mount
points to a built-in location; use the existing userSkillPath,
params.WorkspaceConfig.Mounts, and filepath.Base checks to locate and compare
the mounted Host to ensure user-configured skill takes precedence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a62d98d1-4062-4725-933d-89a81078991f
📒 Files selected for processing (7)
AGENTS.mdREADME.mdpkg/instances/manager.gopkg/instances/manager_test.goskills/config-kdn-workspace/SKILL.mdskills/skills.goskills/skills_test.go
Refactor extractAll into an fs.FS-accepting inner function to enable custom file system injection in tests. Add TestExtractAll_internal with four cases: ReadDir failure, ReadFile failure during walk, non-directory root entries skipped, and the WalkDir callback error path via readDirErrorFS. Also add the extraction-failure path to TestManager_Add_InjectsBuiltInSkills. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
chmod-based write-permission simulation does not enforce access restrictions on Windows, causing the "not writable" error tests to fail. Skip them with runtime.GOOS == "windows", matching the pattern already used in pkg/config and pkg/credential tests. Co-Authored-By: Claude Code (claude-sonnet-4-6) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
The manager_test.go already imports pkg/runtime as "runtime"; the stdlib "runtime" package added for GOOS must use a goruntime alias to avoid the redeclared-in-block vet error. Co-Authored-By: Claude Code (claude-sonnet-4-6) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feloy
changed the title
…e skill Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
feloy
changed the title
Contributor
|
Nice, how can I test this? from the issue it said the new skills can help an agent set up kdn for the first time and help them configure the workspace and skill/mcp injection but it sounds like an agent needs to be running on host with kdn knowledge? Maybe im mistaken but would like a quick test plan! |
Contributor
Author
|
@bmahabirbu you need to have a model provider (vertex ai, etc) configured, then you create and start a workspace, and you ask the agent to help you configure an mcp server. If you look at https://www.youtube.com/watch?v=bB8t3U1m9pg, I'm using this several times during the recorded session |
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.
Introduces a skills/ package that embeds user-facing skill directories into the kdn binary at compile time. manager.Add() calls ExtractAll() to extract and mount all built-in skills automatically for agents that support them; user-configured skills with the same basename override the built-in version.
The first built-in skill, config-kdn-workspace, helps agents running inside a workspace configure environment variables, mounts, secrets, MCP servers, network policies, and other settings interactively.
Fixes #350