docs: add comprehensive kagent-dev skill and streamline CLAUDE.md#1439
docs: add comprehensive kagent-dev skill and streamline CLAUDE.md#1439
Conversation
EItanya
commented
Mar 5, 2026
- Add kagent-dev skill with detailed development workflows
- CRD field addition guide with complete examples
- E2E testing setup and debugging
- PR review workflow
- CI failure troubleshooting
- Translator patterns and golden file testing
- Remove redundant run-e2e-tests skill (integrated into kagent-dev)
- Streamline CLAUDE.md to ~200 lines (from ~657)
- Keep high-level conventions and quick reference
- Delegate detailed workflows to kagent-dev skill
- Improve separation of concerns (what/why in CLAUDE.md, how/when in skill)
- Add kagent-dev skill with detailed development workflows - CRD field addition guide with complete examples - E2E testing setup and debugging - PR review workflow - CI failure troubleshooting - Translator patterns and golden file testing - Remove redundant run-e2e-tests skill (integrated into kagent-dev) - Streamline CLAUDE.md to ~200 lines (from ~657) - Keep high-level conventions and quick reference - Delegate detailed workflows to kagent-dev skill - Improve separation of concerns (what/why in CLAUDE.md, how/when in skill) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
There was a problem hiding this comment.
Pull request overview
This PR restructures agent-facing documentation by shrinking CLAUDE.md to a high-level guide and moving detailed “how-to” development workflows into a new .claude/skills/kagent-dev skill, while removing the old dedicated E2E skill.
Changes:
- Add a comprehensive
kagent-devskill (CRD workflows, translator patterns, E2E debugging, CI troubleshooting, PR review workflow). - Remove the redundant
run-e2e-testsskill (content folded intokagent-dev). - Streamline
CLAUDE.mdto focus on core conventions + pointers to the skill.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
CLAUDE.md |
Slimmed guide; points to kagent-dev; keeps conventions + quick reference. |
.claude/skills/run-e2e-tests/SKILL.md |
Removed legacy E2E-only skill. |
.claude/skills/kagent-dev/SKILL.md |
New top-level development skill with consolidated workflows and commands. |
.claude/skills/kagent-dev/references/translator-guide.md |
New translator workflow + golden-file guidance. |
.claude/skills/kagent-dev/references/e2e-debugging.md |
New detailed E2E debugging/runbook content. |
.claude/skills/kagent-dev/references/crd-workflow-detailed.md |
New CRD-field examples for multiple field types + validation patterns. |
.claude/skills/kagent-dev/references/ci-failures.md |
New CI failure pattern catalog + fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
.claude/skills/kagent-dev/SKILL.md
Outdated
| **Key points:** | ||
| - Add kubebuilder markers for validation (`+kubebuilder:validation:...`) | ||
| - Use `+optional` for optional fields | ||
| - Use pointers for optional fields (`*string` not `string`) |
There was a problem hiding this comment.
The guidance says to use pointers for all optional fields, but the current v1alpha2 API uses value types for several optional strings/ints where the zero value is acceptable (e.g., AgentSpec.Description and DeclarativeAgentSpec.SystemMessage are string). Consider softening this to: use pointers when you need to distinguish “unset” from an empty/zero value, and follow existing patterns in the surrounding types.
| - Use pointers for optional fields (`*string` not `string`) | |
| - Use pointers when you need to distinguish "unset" from an empty/zero value, and follow the patterns of surrounding fields (some optional fields intentionally use value types). |
CLAUDE.md
Outdated
| | Lint code | `make -C go lint` | | ||
| | Generate CRD code | `make -C go generate` | | ||
| | Run controller locally | `make -C go run` | | ||
| | Access UI | `kubectl port-forward -n kagent svc/kagent-ui 3000:80` | |
There was a problem hiding this comment.
The UI Service in the Helm chart exposes port 8080 (see helm/kagent/values.yaml), so port-forwarding to ... 3000:80 will fail or forward the wrong port. Update this quick reference to forward to the Service port (e.g., 3000:8080 or 8082:8080), consistent with DEVELOPMENT.md and the CLI dashboard command.
| | Access UI | `kubectl port-forward -n kagent svc/kagent-ui 3000:80` | | |
| | Access UI | `kubectl port-forward -n kagent svc/kagent-ui 3000:8080` | |
| ```bash | ||
| cd go/core | ||
| go run hack/makeagentconfig/main.go | ||
| AGENT_CONFIG_DIR=$PWD/go/core |
There was a problem hiding this comment.
In Option B you cd go/core and then set AGENT_CONFIG_DIR=$PWD/go/core, which results in a duplicated path (.../go/core/go/core). Align this snippet with go/core/test/e2e/README.md (either run the go run from repo root and keep AGENT_CONFIG_DIR=$PWD/go/core, or if already in go/core set AGENT_CONFIG_DIR=$PWD).
| AGENT_CONFIG_DIR=$PWD/go/core | |
| AGENT_CONFIG_DIR=$PWD |
| If your field requires a new test scenario: | ||
|
|
||
| ```go | ||
| // go/core/internal/controller/translator/agent/translate_test.go |
There was a problem hiding this comment.
This guide points readers to go/core/internal/controller/translator/agent/translate_test.go, but that file doesn't exist in the repo. Update the reference to an actual translator test file (e.g., adk_translator_golden_test.go / adk_api_translator_test.go), or rename the example path to a generic placeholder to avoid broken guidance.
| // go/core/internal/controller/translator/agent/translate_test.go | |
| // go/core/internal/controller/translator/agent/adk_api_translator_test.go |
| if res.CPURequest != nil { | ||
| container.Resources.Requests[corev1.ResourceCPU] = resource.MustParse(*res.CPURequest) | ||
| } | ||
| if res.MemoryRequest != nil { | ||
| container.Resources.Requests[corev1.ResourceMemory] = resource.MustParse(*res.MemoryRequest) | ||
| } |
There was a problem hiding this comment.
This example uses resource.MustParse(...) for CRD-provided values. In real controller/translator code these values are user input and MustParse can panic the controller process. Prefer resource.ParseQuantity (or equivalent) and return a wrapped error on invalid quantities.
| ```go | ||
| // +optional | ||
| // +kubebuilder:default="info" | ||
| LogLevel *string `json:"logLevel,omitempty"` |
There was a problem hiding this comment.
The default-values example defines LogLevel as *string, but earlier in this document LogLevel is introduced as a custom enum type (type LogLevel string). Consider updating this snippet to use the enum type (e.g., *LogLevel) so the examples are consistent and match how kubebuilder enum validation is typically modeled.
| LogLevel *string `json:"logLevel,omitempty"` | |
| LogLevel *LogLevel `json:"logLevel,omitempty"` |
| agent := &v1alpha2.Agent{ | ||
| Spec: v1alpha2.AgentSpec{ | ||
| Declarative: &v1alpha2.DeclarativeAgentSpec{ | ||
| LogLevel: pointer.String("invalid"), // Not in enum |
There was a problem hiding this comment.
In this test snippet LogLevel is assigned pointer.String("invalid"), but the earlier enum example defines the field as *LogLevel (not *string). Update the example to construct a *LogLevel (or change the field type in the earlier example) so the code compiles as written.
| agent := &v1alpha2.Agent{ | |
| Spec: v1alpha2.AgentSpec{ | |
| Declarative: &v1alpha2.DeclarativeAgentSpec{ | |
| LogLevel: pointer.String("invalid"), // Not in enum | |
| invalidLevel := v1alpha2.LogLevel("invalid") | |
| agent := &v1alpha2.Agent{ | |
| Spec: v1alpha2.AgentSpec{ | |
| Declarative: &v1alpha2.DeclarativeAgentSpec{ | |
| LogLevel: &invalidLevel, // Not in enum |
- Fix UI port-forward command (8080 not 80) in CLAUDE.md - Soften pointer guidance to match existing patterns - Fix AGENT_CONFIG_DIR path duplication in e2e-debugging - Update test file reference to actual file (adk_api_translator_test.go) - Replace MustParse with ParseQuantity for safe error handling - Fix LogLevel type consistency across examples Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>