[scottt732]

What type of PR is this?

/kind api

What this PR does / why we need it:

Adds an opt-in spec.skills[].optional field 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 add fails. 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 verify passes.
• make test passes.
• make build passes after removing a Homebrew LDFLAGS value from the local environment; the Makefile forwards that variable to Go's -ldflags.
• Script-execution tests cover optional failure with a later successful package, required fail-fast behavior, and the all-optional/all-failed case.
• Conversion tests cover round-trip preservation and ambiguous duplicate protection.

Does this PR introduce a user-facing change?

AgentConfig v1alpha2 skills can set `optional: true` so a package installation failure is logged without blocking agent startup.

---

Summary by cubic

Adds spec.skills[].optional to 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.

• Optional package failures are logged with the package source; remaining packages and agent startup proceed, and an all-optional failure leaves an empty plugin skills directory.
• Required packages keep failing fast on install, relocation, or install-nothing failures; secretRef validation is unchanged and still fails before Job creation. The field defaults to false and is preserved across v1alpha1 conversion via an annotation.
• Optional is v1alpha2-only and rejected by the CLI when only v1alpha1 CRDs exist; script-execution and e2e tests cover the continuation and fail-fast paths.

^{Written for commit b94c89d. Summary will update on new commits.}

[cubic-dev-ai]

All reported issues were addressed across 9 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 3 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[scottt732]

Gentle nudge on this one — it has been in needs-triage/needs-actor since Sep 3 and has not picked up a reviewer yet.

Current state, in case it helps triage:

• MERGEABLE, no conflicts against main
• make verify, make test, and make build all pass
• Tests cover the three cases that matter: an optional failure followed by a successful package, required fail-fast, and the all-optional/all-failed case
• Conversion tests cover v1alpha1 round-trip preservation and ambiguous-duplicate protection

The field defaults to false and round-trips through v1alpha1 via an annotation, so it is inert for anyone not opting in and adds no surface area to the compatibility API version.

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.

[gjkim42]

/kelos claude-api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-claude-api-reviewer-issue-comment-16ea3782b289 has succeeded. ✅

[gjkim42]

Looks good to me.
Can you add an e2e test for this?

[gjkim42]

/kelos claude-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-claude-reviewer-issue-comment-2d85ee4cc183 has succeeded. ✅

[kelos-bot]

🤖 Kelos Claude API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: Adds spec.skills[].optional (bool, defaults to false) to the v1alpha2 AgentConfig SkillsShSpec, preserves it across v1alpha1 conversion via the kelos.dev/v1alpha2-skills-optional annotation, and regenerates both CRD copies plus docs/reference.md. Reviewed at head 8d4bf5a.

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.

Findings

Compatibility (no issues)

• api/v1alpha2/agentconfig_types.go:87-90 — Additive optional bool with omitempty and +optional. No existing field changes kind, no validation is tightened, and false/absent are equivalent, so every existing AgentConfig still applies. examples/ and self-development/ need no sweep.
• internal/manifests/install-crd.yaml:614 and the chart copy — The new property appears only under the v1alpha2 schema; the served v1alpha1 schema is untouched, matching the "latest version only" rule. Both generated CRD copies are updated and the CI verify job passes on this head, so make update artifacts are complete (no deepcopy change is needed for a bool, and pkg/generated has no apply-configurations to regenerate).

Documentation (P2, non-blocking but please fix)

• api/v1alpha2/agentconfig_types.go:87-88 and docs/reference.md:728 — The godoc and reference row say startup continues "when this package cannot be installed", but optional only relaxes the skills-install init-container step. validateSkillsAuthSecrets (internal/controller/task_controller.go:499-528, called from task_controller.go:372 and session_controller.go:1382) does not consult Optional, so a missing Secret or missing/empty GITHUB_TOKEN on an optional package still fails the Task before the Job is created. That fail-fast behavior is the right call for a configuration error, but the contract should say so. Suggested godoc:

  // Optional, when true, logs an installation failure for this package and
  // continues installing the remaining packages instead of failing the Task.
  // Defaults to false. SecretRef validation is not affected: a missing or
  // invalid Secret still fails the Task before the Job is created.

  and append a matching sentence to the reference row.
• api/v1alpha2/agentconfig_types.go:88 — "Required packages remain fail-fast" refers to a concept with no field behind it. State the default explicitly instead, the way AllowInsecure does (taskspawner_types.go:822), and use the "X, when true, ..." phrasing already used by MentionOptional (taskspawner_types.go:705). The snippet above covers both.

Naming and shape (P3, no change needed)

• optional matches upstream corev1 usage on SecretKeySelector, ConfigMapKeySelector, and EnvFromSource ("tolerate absence") and Kelos's own mentionOptional, so the name will age well. A failurePolicy enum was the alternative; with no concrete caller for retries or a Task condition today, the bool is the minimal surface and the right choice now. If a richer policy is ever needed it would be a new field rather than a new enum value, which is acceptable given the precedent.
• Plain bool rather than *bool is fine here because unset and false mean the same thing, consistent with allowInsecure.

Conversion (P3)

• internal/conversion/agentconfig.go:311-355 — Mirrors the existing secretRef preservation pattern (index + source + skill match, ambiguity guard, annotation stripped on the hub, malformed annotation ignored). Correct for the multi-version contract.
• internal/conversion/agentconfig_test.go:161-181 — The optional round-trip test asserts only that the flag survives. The sibling secretRef test (agentconfig_test.go:149-154) also checks that the hub no longer carries the preservation annotation and that the spoke annotation is not mutated during restore. Adding the same two assertions keeps the two annotations' contracts pinned identically.

Suggestions (optional)

• e2e coverage for the documented contract. The "startup continues" promise is verified only at the shell-script level. Entrypoints pass every plugin directory through (claude-code/kelos_entrypoint.sh:88-89 via --plugin-dir, codex/kelos_entrypoint.sh:37-41 via the copy loop), and the all-optional-failed case leaves an empty skills-sh/skills directory. An e2e Task with one optional: true unreachable source plus the existing kelos-dev/e2e-skills fixture, asserting Succeeded and the marker string, would prove the contract end to end. This aligns with the maintainer's request above.
• Observability follow-up. An optional failure is visible only in the skills-install init-container log. That is fine for the minimal API; if operators later need it, a Task event or condition would be the natural next step and does not require an API change now.
• CLI parity follow-up. kelos create agentconfig --skills-sh source:skill (internal/cli/create_agentconfig.go:132) has no way to set optional. Not required for this PR, but worth tracking so the CLI does not lag the API.

/kelos needs-input

[kelos-bot]

🤖 Kelos Claude Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds spec.skills[].optional to the v1alpha2 AgentConfig, makes the skills-install init script log and continue when an optional package fails while required packages stay fail-fast, and preserves the flag across v1alpha1 conversion via an annotation. Reviewed at head 8d4bf5a.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	2	internal/cli/agentconfig_compat.go:98	agentConfigFitsV1alpha1 ignores Optional, so the v1alpha1 fallback creates an object on which the flag is not honored
P2		test/e2e/skills_test.go:77	No e2e coverage for the "startup continues" contract (requested by maintainer after the last commit)
P3	2	internal/controller/job_builder.go:1406	Required-package guard no longer fails when the install dir exists but is empty
P3		internal/conversion/agentconfig_test.go:161	Round-trip test omits the annotation-cleanup assertions its secretRef sibling has

Findings

Correctness

• [P2] internal/cli/agentconfig_compat.go:98 — agentConfigFitsV1alpha1 does not treat Optional: true as a v1alpha2-only capability. On a cluster that serves only v1alpha1 CRDs, createAgentConfig (internal/cli/create_agentconfig.go:120) converts the spec and creates it as v1alpha1; the stored object then carries only the kelos.dev/v1alpha2-skills-optional annotation, which nothing on that cluster honors, so the package is installed as required and an install failure blocks the agent despite the caller's opt-in. The commit that introduced secretRef preservation (6ef2305) added its check to this helper plus TestCreateAgentConfigRejectsSkillsSecretRefWithoutV1alpha2 in the same change; do the same for Optional and extend the reason string at agentconfig_compat.go:72 (currently "MCP env valueFrom or skills secretRef"). This is unreachable from today's --skills-sh flag syntax, so it is latent, but the helper's contract is wrong as of this PR.

Tests

• [P2] test/e2e/skills_test.go:77 — gjkim42 asked for an e2e test today, after the last commit on Sep 3; the PR has none. The "startup continues" contract is currently verified only by running the generated script under sh with a stubbed npx. Since --skills-sh cannot set optional, create the AgentConfig through f.KelosClientset.ApiV1alpha2().AgentConfigs(ns).Create (as the authenticated case already does at skills_test.go:162) with the existing kelos-dev/e2e-skills:kelos-e2e required entry plus an optional: true entry pointing at a nonexistent repo, keep the verify-skills-install init container and the KELOS_E2E_SKILL_MARKER_x7k2p9 log assertion, and assert the Task reaches Succeeded. A second case with only the unreachable optional package asserting Succeeded would pin the empty-plugin-dir path through the real entrypoint.
• [P3] internal/conversion/agentconfig_test.go:161 — TestAgentConfigRoundTrip_PreservesOptionalSkill asserts only that Optional survives. The sibling TestAgentConfigRoundTrip_PreservesSkillsSecretRef (lines 149-154) also asserts that the hub no longer carries the preservation annotation after restore and that the spoke annotation is untouched; without those, a regression that leaks kelos.dev/v1alpha2-skills-optional onto hub objects passes. Add the same two assertions for the new annotation.

Correctness (low)

• [P3] internal/controller/job_builder.go:1406 — The required-package guard now checks only that .agents/skills exists. Before this PR, mv '<installDir>'/* … under set -e also failed when the directory existed but was empty, because the unmatched glob was passed literally to mv. With the tolerant loop at lines 1411-1412, a required package whose npx skills add exits 0 without producing a skill (for example a -s name absent from the package, if the CLI exits 0 in that case) now passes the "No skills.sh skills were installed" guard and the agent starts without the required skill. Counting moved entries in the loop and failing when hasRequiredSkills and the count is zero restores the previous strictness and can replace the [ -d ] check.

Key takeaways

• Script semantics are sound: optional packages run under if ! …; then, which set -e does not trip, while required packages stay bare commands so the first failure exits. All four entrypoints tolerate an empty skills-sh/skills directory ([ -d ] || continue loops in claude-code, codex, and cursor; cp -r of an empty dir in gemini), so the all-optional-failed case starts the agent as intended.
• The API review at this same head flagged the godoc and docs/reference.md wording: optional does not relax validateSkillsAuthSecrets (internal/controller/task_controller.go:499), so a missing or empty Secret still fails the Task before Job creation. That is still unaddressed at 8d4bf5a and is not repeated as a separate finding here.
• Both generated CRD copies and docs/reference.md are updated; a bool needs no deepcopy change.

[scottt732]

Thanks — e2e added, plus the other findings from both review agents. Pushed as 7abe613.

e2e (@gjkim42's request) — test/e2e/skills_test.go gains two cases in the Task with skills.sh AgentConfig suite, both creating the AgentConfig through the typed client since --skills-sh cannot set optional:

• unreachable optional: true package listed before the kelos-dev/e2e-skills:kelos-e2e fixture, with the existing verify-skills-install init container and the KELOS_E2E_SKILL_MARKER_x7k2p9 log assertion. Ordering matters: a regression that aborts the init container also skips the fixture, so the marker assertion fails.
• only an unreachable optional: true package, with an init container asserting skills-sh/skills exists and is empty, and the Task reaching Succeeded.

API review (P2, docs) — godoc and docs/reference.md now use the suggested wording and state explicitly that secretRef validation is unaffected: a missing Secret, missing GITHUB_TOKEN, or empty token still fails the Task before Job creation. Both generated CRD copies regenerated.

Code review (P2) — agentConfigFitsV1alpha1 now treats Optional: true as a v1alpha2-only capability, the reason string names it, and TestCreateAgentConfigRejectsSkillsOptionalWithoutV1alpha2 mirrors the secretRef test.

Code review (P3) / cubic — the relocation loop counts moved entries and the required guard is now [ "$moved_skills" -gt 0 ] instead of [ -d <installDir> ], so a required package whose installer exits 0 without writing a skill fails. The loop also matches .[!.]* and ..?* so hidden entries are relocated rather than dropped and then deleted by the following rm -rf. Two new script-execution tests cover both.

Code review (P3) / cubic — TestAgentConfigRoundTrip_PreservesOptionalSkill now also asserts the hub no longer carries kelos.dev/v1alpha2-skills-optional after restore and that the spoke annotation is not mutated.

Follow-ups I did not fold in, as suggested: observability for optional failures beyond the init-container log, and kelos create agentconfig CLI parity for optional.

Ran locally: make update, and the focused internal/controller, internal/cli, and internal/conversion tests plus go vet ./test/e2e/.... The full suites and e2e are left to CI.

[gjkim42]

Lgtm

@scottt732
Can you squash commits for this?

[scottt732]

@gjkim42 squashed into a single commit and force-pushed — now at b94c89d.

The tree is byte-identical to the reviewed head 7336261 (git diff 7336261 b94c89d is empty), so this is purely a history cleanup. Your approval carried over to the new head.