feat: support optional skills packages - #1726
Open
scottt732 wants to merge 3 commits into
Open
Conversation
github-actions
Bot
added
kind/api
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/job_builder.go">
<violation number="1" location="internal/controller/job_builder.go:1406">
P2: When a required `npx skills add` succeeds after leaving an empty `.agents/skills`, this check passes and the agent starts without the required skills. Require at least one child entry before proceeding.</violation>
</file>
<file name="internal/conversion/agentconfig_test.go">
<violation number="1" location="internal/conversion/agentconfig_test.go:178">
P3: The new optional round-trip test only asserts the optional flag survives. The sibling TestAgentConfigRoundTrip_PreservesSkillsSecretRef also verifies the preservation annotation is removed from the hub after restore and not mutated on the source spoke; the optional test omits both checks, so a regression where the annotation leaks into the hub object would go undetected. Add assertions that hub.Annotations no longer contains preservedSkillsOptionalAnnotation after restore.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| lines = append(lines, fmt.Sprintf("mkdir -p %s", shellQuote(pluginSkillsDir))) | ||
| if hasRequiredSkills { | ||
| lines = append(lines, | ||
| fmt.Sprintf("[ -d %s ] || { echo 'No skills.sh skills were installed' >&2; exit 1; }", shellQuote(installDir)), |
There was a problem hiding this comment.
P2: When a required npx skills add succeeds after leaving an empty .agents/skills, this check passes and the agent starts without the required skills. Require at least one child entry before proceeding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/job_builder.go, line 1406:
<comment>When a required `npx skills add` succeeds after leaving an empty `.agents/skills`, this check passes and the agent starts without the required skills. Require at least one child entry before proceeding.</comment>
<file context>
@@ -1390,10 +1400,16 @@ func buildSkillsInstallScript(skills []kelos.SkillsShSpec, authEnvs []skillsAuth
+ lines = append(lines, fmt.Sprintf("mkdir -p %s", shellQuote(pluginSkillsDir)))
+ if hasRequiredSkills {
+ lines = append(lines,
+ fmt.Sprintf("[ -d %s ] || { echo 'No skills.sh skills were installed' >&2; exit 1; }", shellQuote(installDir)),
+ )
+ }
</file context>
Suggested change
| fmt.Sprintf("[ -d %s ] || { echo 'No skills.sh skills were installed' >&2; exit 1; }", shellQuote(installDir)), | |
| fmt.Sprintf("[ -d %s ] && [ -n \"$(find %s -mindepth 1 -maxdepth 1 -print -quit)\" ] || { echo 'No skills.sh skills were installed' >&2; exit 1; }", shellQuote(installDir), shellQuote(installDir)), |
| t.Fatalf("agentConfigToHub() error = %v", err) | ||
| } | ||
|
|
||
| if len(hub.Spec.Skills) != 1 || !hub.Spec.Skills[0].Optional { |
There was a problem hiding this comment.
P3: The new optional round-trip test only asserts the optional flag survives. The sibling TestAgentConfigRoundTrip_PreservesSkillsSecretRef also verifies the preservation annotation is removed from the hub after restore and not mutated on the source spoke; the optional test omits both checks, so a regression where the annotation leaks into the hub object would go undetected. Add assertions that hub.Annotations no longer contains preservedSkillsOptionalAnnotation after restore.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/conversion/agentconfig_test.go, line 178:
<comment>The new optional round-trip test only asserts the optional flag survives. The sibling TestAgentConfigRoundTrip_PreservesSkillsSecretRef also verifies the preservation annotation is removed from the hub after restore and not mutated on the source spoke; the optional test omits both checks, so a regression where the annotation leaks into the hub object would go undetected. Add assertions that hub.Annotations no longer contains preservedSkillsOptionalAnnotation after restore.</comment>
<file context>
@@ -158,6 +158,55 @@ func TestAgentConfigRoundTrip_PreservesSkillsSecretRef(t *testing.T) {
+ t.Fatalf("agentConfigToHub() error = %v", err)
+ }
+
+ if len(hub.Spec.Skills) != 1 || !hub.Spec.Skills[0].Optional {
+ t.Fatalf("Skills = %#v, want one optional skill", hub.Spec.Skills)
+ }
</file context>
scottt732
requested a deployment
to
ok-to-test
September 3, 2026 21:43 — with 
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/job_builder.go">
<violation number="1" location="internal/controller/job_builder.go:1411">
P3: The `for skill_path in <installDir>/*` glob replaces `find -mindepth 1 -maxdepth 1`, but `*` in POSIX sh does not match hidden (dot-prefixed) entries. Any top-level dotfile/directory in `$HOME/.agents/skills` is skipped and then removed by the subsequent `rm -rf`, silently dropping skill content that the previous `find` command relocated. If skills.sh writes a top-level hidden entry, its contents are lost. Include hidden entries (e.g. a `.[!.]*` glob) or keep the `find` form.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| fmt.Sprintf("mkdir -p %s", shellQuote(pluginSkillsDir)), | ||
| fmt.Sprintf("mv %s/* %s/", shellQuote(installDir), shellQuote(pluginSkillsDir)), | ||
| fmt.Sprintf("if [ -d %s ]; then", shellQuote(installDir)), | ||
| fmt.Sprintf(" for skill_path in %s/*; do", shellQuote(installDir)), |
There was a problem hiding this comment.
P3: The for skill_path in <installDir>/* glob replaces find -mindepth 1 -maxdepth 1, but * in POSIX sh does not match hidden (dot-prefixed) entries. Any top-level dotfile/directory in $HOME/.agents/skills is skipped and then removed by the subsequent rm -rf, silently dropping skill content that the previous find command relocated. If skills.sh writes a top-level hidden entry, its contents are lost. Include hidden entries (e.g. a .[!.]* glob) or keep the find form.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/job_builder.go, line 1411:
<comment>The `for skill_path in <installDir>/*` glob replaces `find -mindepth 1 -maxdepth 1`, but `*` in POSIX sh does not match hidden (dot-prefixed) entries. Any top-level dotfile/directory in `$HOME/.agents/skills` is skipped and then removed by the subsequent `rm -rf`, silently dropping skill content that the previous `find` command relocated. If skills.sh writes a top-level hidden entry, its contents are lost. Include hidden entries (e.g. a `.[!.]*` glob) or keep the `find` form.</comment>
<file context>
@@ -1408,7 +1408,10 @@ func buildSkillsInstallScript(skills []kelos.SkillsShSpec, authEnvs []skillsAuth
lines = append(lines,
fmt.Sprintf("if [ -d %s ]; then", shellQuote(installDir)),
- fmt.Sprintf(" find %s -mindepth 1 -maxdepth 1 -exec mv {} %s/ \\;", shellQuote(installDir), shellQuote(pluginSkillsDir)),
+ fmt.Sprintf(" for skill_path in %s/*; do", shellQuote(installDir)),
+ " [ -e \"$skill_path\" ] || continue",
+ fmt.Sprintf(" mv \"$skill_path\" %s/", shellQuote(pluginSkillsDir)),
</file context>
Suggested change
| fmt.Sprintf(" for skill_path in %s/*; do", shellQuote(installDir)), | |
| fmt.Sprintf(" for skill_path in %s/* %s/.[!.]*; do", shellQuote(installDir), shellQuote(installDir)), |
Contributor
Author
|
Gentle nudge on this one — it has been in Current state, in case it helps triage:
The field defaults to Happy to rebase, split the conversion change into its own PR, or narrow the scope if any of that would make it easier to review. |
Collaborator
|
/kelos claude-api-review |
Contributor
|
🤖 Kelos Task Status Task |
Collaborator
|
Looks good to me. |
Collaborator
|
/kelos claude-review |
Contributor
|
🤖 Kelos Task Status Task |
Contributor
|
🤖 Kelos Claude API Reviewer Agent @gjkim42 API Design ReviewVerdict: APPROVE No blocking issues. The field is additive, correctly scoped to the storage version, and follows existing Kelos and upstream naming. Two documentation/contract clarifications are worth landing before merge; everything else is non-blocking. FindingsCompatibility (no issues)
Documentation (P2, non-blocking but please fix)
Naming and shape (P3, no change needed)
Conversion (P3)
Suggestions (optional)
/kelos needs-input |
Contributor
|
🤖 Kelos Claude Reviewer Agent @gjkim42 Review SummaryVerdict: APPROVE Findings Overview
FindingsCorrectness
Tests
Correctness (low)
Key takeaways
|
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 type of PR is this?
/kind api
What this PR does / why we need it:
Adds an opt-in
spec.skills[].optionalfield to the v1alpha2 AgentConfig API.Required packages keep the existing fail-fast behavior. An optional package logs its source and allows the remaining packages and agent startup to continue when
npx skills addfails. This prevents an unavailable third-party skills package from blocking an otherwise usable agent when the caller has explicitly accepted degraded capability.The field defaults to false, is preserved across v1alpha1 conversion through an annotation, and does not add new surface area to the compatibility API version.
Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
make verifypasses.make testpasses.make buildpasses after removing a HomebrewLDFLAGSvalue from the local environment; the Makefile forwards that variable to Go's-ldflags.Does this PR introduce a user-facing change?
Summary by cubic
Adds
spec.skills[].optionalto the v1alpha2 AgentConfig API so agent startup continues when an optional skills package fails to install, while required packages keep the existing fail-fast behavior.falseand is preserved across v1alpha1 conversion via an annotation.Written for commit 8d4bf5a. Summary will update on new commits.