feat(harness): Initial Claude Code Support - #2602
Conversation
e59e0a4 to
312dfe6
Compare
| - [x] Durable Claude session resume between turns | ||
| - [x] Claude Code built-in tools | ||
| - [x] Shared local subagents | ||
| - [x] Standalone skills and plugin-provided skills |
There was a problem hiding this comment.
Note to reviewer: This short list summarizes what has been done and what will come next for Claude harness.
Plugin provided skills are not entirely working due to #2604
Codex review[P1] Terminate the entire Claude process group — When a turn is canceled or parsing/event delivery fails while Claude has spawned a built-in tool process, signaling and killing only |
47759d9 to
1a6da57
Compare
|
There is too much happening in this PR for a focused review. Please split the ToolService / RemoteMCPServer / MCPServer changes into a separate PR and keep this one scoped to the Claude harness work. Those changes have their own catalog, reconciliation, lifecycle, and UI implications and need focused review and testing on their own. |
88b1568 to
eabf1c1
Compare
|
I've moved the service and MCP discovery changes to #2639 which I tested separately from this as well, this PR is now focused on Claude changes |
| run: | | ||
| echo "Cache key: ${{ needs.setup.outputs.cache-key }}" | ||
| make build-controller build-golang-adk | ||
| make build-controller build-golang-adk build-claude-harness |
There was a problem hiding this comment.
In the future we should add -j here so we can parallelize these builds across the runner CPUs
| Server AgentTemplateTypedLocalReference `json:"server"` | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // Tools optionally limits which server tools are exposed. An omitted or empty | ||
| // list exposes every tool. Harnesses that cannot enforce a partial selection |
There was a problem hiding this comment.
Does claude not allow filtering?
There was a problem hiding this comment.
not directly with an allowed tools option, but I guess we can do this conversely with disallow tools? See this comment
One issue I can see with this is that since this is just emulating allowlist, if MCP server adds a new tool after compilation but before discovery / reconciliation catches up, the new tool is not denied (despite not being allowed) for a given window
There was a problem hiding this comment.
Let's take this as a follow-up, good to get something merged for now
| matchLabels: | ||
| kagent.dev/e2e-runtime: claude | ||
| --- | ||
| apiVersion: kagent.dev/v1alpha3 |
There was a problem hiding this comment.
We have the kagent template below and not the claude one, why?
There was a problem hiding this comment.
Claude agent templates are created by a helper function in the test file, since it relies on test-time generated mock modelconfig name
There was a problem hiding this comment.
Renamed e2e tests accordingly
|
|
||
| type MCPConfig struct { | ||
| // ADKPaths adds the plugin data destination needed by ADK MCP servers. | ||
| type ADKPaths struct { |
There was a problem hiding this comment.
Should this be in adk specific code?
There was a problem hiding this comment.
I agree, I've moved them to ADK module, agentplugin is now runtime-neutral see b739599
| return err | ||
| } | ||
|
|
||
| func materializeForADK(ctx context.Context, resources agentplugin.Resources, paths ADKPaths) (mcpConfig, error) { |
| useBedrockEnv = "CLAUDE_CODE_USE_BEDROCK" | ||
| useVertexEnv = "CLAUDE_CODE_USE_VERTEX" | ||
| awsRegionEnv = "AWS_REGION" | ||
| awsAccessKeyEnv = "AWS_ACCESS_KEY_ID" | ||
| awsSecretKeyEnv = "AWS_SECRET_ACCESS_KEY" | ||
| awsSessionTokenEnv = "AWS_SESSION_TOKEN" | ||
| awsBedrockTokenEnv = "AWS_BEARER_TOKEN_BEDROCK" | ||
| anthropicAPIKeyEnv = "ANTHROPIC_API_KEY" | ||
| anthropicBaseURLEnv = "ANTHROPIC_BASE_URL" | ||
| vertexProjectEnv = "ANTHROPIC_VERTEX_PROJECT_ID" | ||
| vertexRegionEnv = "CLOUD_ML_REGION" |
There was a problem hiding this comment.
Once egress policy is supported we should move everything there
There was a problem hiding this comment.
Yes definitely
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
b1d9b2d to
b739599
Compare
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
|
A simple demo of Claude Code with Bedrock, also tested with Vertex input.mov |
EItanya
left a comment
There was a problem hiding this comment.
A couple of things need following up on:
- Need more code comments, things can be pretty hard to follow
- Need to decide on a policy for validating fields which can't apply to certain harnesses. I tend to favor letting it through and logging
- I noticed in the video that it takes a couple seconds to respond, I think we need to switch to memory snapshot for Claude since it's node.
| RUNTIME_DIGEST=$(docker buildx imagetools inspect "localhost:5001/kagent-dev/kagent/golang-adk:${VERSION}" | awk '$1 == "Digest:" { print $2; exit }') | ||
| test -n "$RUNTIME_DIGEST" | ||
| export KAGENT_E2E_RUNTIME_IMAGE="localhost:5001/kagent-dev/kagent/golang-adk@${RUNTIME_DIGEST}" | ||
| CLAUDE_DIGEST=$(docker buildx imagetools inspect "localhost:5001/kagent-dev/kagent/claude-harness:${VERSION}" | awk '$1 == "Digest:" { print $2; exit }') |
There was a problem hiding this comment.
Not blocking, I don't want to end up in a situation like last release where the controller build was completely blocked on these dependencies
| if state.Revision == nil || state.RevisionID.IsZero() { | ||
| return r.cleanupUnreferencedRevisions(ctx) | ||
| } | ||
| for _, warning := range state.Revision.Warnings { |
There was a problem hiding this comment.
In the future can we bubble these up to a status somewhere? Or just somewhere that's more easily trackable
| "Claude RemoteMCPServer %q cannot verify selected tools %v because no current discovered tool set is available; exposing the whole server", | ||
| server.Name, selected, |
There was a problem hiding this comment.
This should be a warning not a failfast
| if !server.Spec.TLS.IsEmpty() { | ||
| return "", v2translator.NewValidationError("Claude RemoteMCPServer %q does not support custom TLS configuration", server.Name) | ||
| } | ||
| if server.Spec.Timeout != nil && server.Spec.Timeout.Duration != 30*time.Second { | ||
| return "", v2translator.NewValidationError("Claude RemoteMCPServer %q supports only the default 30s timeout", server.Name) | ||
| } | ||
| if server.Spec.TerminateOnClose != nil && !*server.Spec.TerminateOnClose { | ||
| return "", v2translator.NewValidationError("Claude RemoteMCPServer %q requires terminateOnClose", server.Name) | ||
| } |
There was a problem hiding this comment.
Similar to the above I actually think these are more akin to warnings than errors, i don't think it's worth failing translation for this. let's fix these in a follow-up
There was a problem hiding this comment.
Moved these to a warning field on AgentTemplate status
| claudeConfigDirEnv = "CLAUDE_CONFIG_DIR" | ||
| disableUpdatesEnv = "DISABLE_UPDATES" | ||
| googleApplicationCredentialsEnv = "GOOGLE_APPLICATION_CREDENTIALS" |
There was a problem hiding this comment.
There are so many lists of env vars for claude, can we centrailze them?
| } | ||
| } | ||
|
|
||
| type boundedBuffer struct { |
There was a problem hiding this comment.
Something like this doesn't exist in the stdlib?
There was a problem hiding this comment.
Unfortunately there isn't anything in io or bufio that does this, there are also bounded buffer utils implemented elsewhere in the codebase as well
| if _, err := tmp.Write(b); err != nil { | ||
| _ = tmp.Close() | ||
| return fmt.Errorf("write temporary session state: %w", err) | ||
| } | ||
| if err := tmp.Sync(); err != nil { | ||
| _ = tmp.Close() | ||
| return fmt.Errorf("sync temporary session state: %w", err) | ||
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return fmt.Errorf("close temporary session state: %w", err) | ||
| } | ||
| if err := os.Rename(tmpName, s.path); err != nil { | ||
| return fmt.Errorf("replace session state: %w", err) | ||
| } |
There was a problem hiding this comment.
There's no os stdlib fn for this? Seems fairly common
There was a problem hiding this comment.
stdlib only provides individual file ops nothing like atomic write file
| data state | ||
| } | ||
|
|
||
| func New(durableDir string) (*Store, error) { |
There was a problem hiding this comment.
Again more comments, It's not clear how this fits into the picture
|
|
||
| // Runner is the execution capability consumed by the A2A supervisor. | ||
| type Runner interface { | ||
| Run(context.Context, runtime.Turn, runtime.EventSink) (runtime.Outcome, error) |
There was a problem hiding this comment.
Do you think we'll be able to re-use this for codex, or is that wishful thinking?
Runs Codex on top of Substrate. Reuses most plumbings from Claude harness, plus some refactoring, and address some follow-up comments on #2602. Feature coverage for Codex is identical to Claude Code right now, and is mostly in parity with Kagent's own ADK harness. The harness driver uses Codex App Server (JSON-RPC over stdio). ## What Changed - Adds Codex as a supported Harness, including controller compilation, runtime image, and release packaging. - Runs Codex with approvals set to never and native sandboxing set to danger-full-access; the Substrate Actor is intentionally the security boundary. - Supports OpenAI Responses and Bedrock gpt-* models, Streamable HTTP MCP servers, selected skills/plugins, and Shared agents; unsupported provider and runtime options are rejected during compilation. - Adds non-blocking compilation warnings to `AgentTemplate` harness status; Claude now reports ignored MCP TLS, timeout, and terminateOnClose settings there instead of rejecting the template. - Moves native continuation persistence and process/file security helpers into a shared runtime layer used by both Codex and Claude, while keeping harness-specific protocol behavior separate. ## Validation - Unit test coverage for Codex - Cluster E2E coverage for Codex - Manual verification (see below recording) --------- Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
…untimes (#2663) ## Summary Restores distributed tracing for the v2 controller and the established kagent runtime. - Initialize the controller tracer provider before constructing the instrumented gRPC server. - Forward controller `OTEL_*` variables into kagent runtime revisions; tracing configuration therefore participates in the revision digest. - Add focused coverage for tracer registration and OTEL environment forwarding. <img width="1920" height="959" alt="jaeger-trace" src="https://github.com/user-attachments/assets/6005119d-fb6e-4f55-846b-6399bb9e50f6" /> ## Verification - `go test ./core/...` - `make -C go lint` - Focused tests: `go test ./core/internal/telemetry ./core/v2/translator ./core/v2/translator/kagent ./core/v2/translator/claude` - Kind smoke test: a kagent-harness invocation produces one connected Jaeger trace from `kagent-controller` into the actor. ## Deferred Claude harness tracing needs runtime telemetry initialization and its mock E2E fixture/image from #2602. That work will be handled with the new harness rather than broaden this controller regression fix. --------- Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Basic support for running Claude Code on Substrate and add extensible interfaces for future harness like Codex.