refactor: simplify runtime images and add lifecycle e2e - #2498
Merged
Conversation
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
EItanya
requested review from
a team,
Charlesthebird,
peterj and
supreme-gg-gg
as code owners
August 19, 2026 16:03
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR simplifies agent runtime images and agent skill initialization by removing the skills-init init container and SRT-specific wiring, updates Substrate integration (v0.0.13), and adds a Substrate-backed AgentInstance lifecycle E2E test to CI.
Changes:
- Remove skills-init init-container support (CRD/schema, controller translator, UI form) and related image build/publish logic.
- Update Substrate integration to resolve SecretKeyRefs into literal env vars where required and bump Substrate to v0.0.13.
- Add/enable lifecycle E2E test and CI job steps to provision Substrate and validate AgentInstance create/delete behavior.
Reviewed changes
Copilot reviewed 87 out of 90 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/types/index.ts | Removes UI typing for skills init container fields. |
| ui/src/lib/agentSkillsForm.ts | Deletes S3 auth-secret/initContainer env helpers and validation. |
| ui/src/lib/agentFormDomain.ts | Removes skills S3 auth-secret state mapping from API → form. |
| ui/src/lib/tests/agentSkillsForm.test.ts | Updates tests to reflect removed S3 auth-secret/init-container behavior. |
| ui/src/components/agent-form/AgentSkillsFormSection.tsx | Removes S3 auth secret input and updates hint text. |
| ui/src/app/agents/new/page.tsx | Removes wiring for S3 auth-secret state updates. |
| scripts/controller-digest-ldflags.sh | Stops requiring/embedding “full” runtime image digests. |
| python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py | Updates execution/injection tests to match new bash execution path. |
| python/packages/kagent-skills/src/kagent/skills/shell.py | Removes SRT settings handling and runs commands via bash directly. |
| python/packages/kagent-adk/tests/unittests/test_config_materialize.py | Removes SRT settings materialization tests. |
| python/packages/kagent-adk/src/kagent/adk/tools/bash_tool.py | Updates tool docs to no longer claim SRT sandbox execution. |
| python/packages/kagent-adk/src/kagent/adk/_config_materialize.py | Stops materializing SRT settings JSON from env. |
| python/Dockerfile.full | Removes full Python runtime image definition. |
| python/Dockerfile.app | Clarifies app image purpose; still only changes startup behavior. |
| python/Dockerfile | Simplifies Python runtime image build and ensures /config exists. |
| helm/kagent/values.yaml | Removes skills-init image values and related commented pauseImage config. |
| helm/kagent/templates/controller-deployment.yaml | Removes SUBSTRATE_PAUSE_IMAGE env wiring. |
| helm/kagent/templates/controller-configmap.yaml | Removes skills-init image config vars. |
| helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml | Removes skills.initContainer schema and updates skill auth docs. |
| go/go.sum | Bumps Substrate dependency and adds k8s metrics dependency hashes. |
| go/go.mod | Bumps Substrate replace to v0.0.13 and promotes x/sync to direct dep. |
| go/core/v2/translator/compiler_test.go | Updates translator test expectations for Substrate env resolution. |
| go/core/v2/translator/compiler.go | Resolves SecretKeyRefs into literal env var values for Substrate. |
| go/core/v2/substrate/actor_template_test.go | Updates ActorTemplate tests for literal env values and new signature. |
| go/core/v2/substrate/actor_template.go | Removes pauseImage param and enforces literal-only envs for Substrate. |
| go/core/v2/controller/runtime.go | Simplifies runtime creation by removing CollectionConfig argument. |
| go/core/v2/controller/reconciler.go | Removes CollectionConfig and pause image usage in reconciliation graph. |
| go/core/v2/controller/collections_test.go | Updates collections test to match new NewCollections/newPairReconciliations signatures. |
| go/core/v2/controller/collections.go | Removes CollectionConfig wiring into collections graph creation. |
| go/core/test/e2e/testdata/skills/kebab-maker/scripts/make_kebab.py | Removes legacy kebab-maker skill fixture file. |
| go/core/test/e2e/testdata/skills/kebab-maker/SKILL.md | Removes legacy kebab-maker skill fixture metadata. |
| go/core/test/e2e/testdata/skills/kebab-maker/LICENSE.txt | Removes legacy kebab-maker test license. |
| go/core/test/e2e/testdata/skills/kebab-maker/Dockerfile | Removes legacy kebab-maker test image Dockerfile. |
| go/core/test/e2e/manifests/lifecycle.yaml.tmpl | Adds Harness/AgentTemplate fixtures for lifecycle E2E. |
| go/core/test/e2e/manifests/add-numbers-agent.yaml | Removes legacy SandboxAgent manifest fixture. |
| go/core/test/e2e/lifecycle_test.go | Adds AgentInstance lifecycle E2E test via public gRPC API. |
| go/core/test/e2e/agents/kebab/pyproject.toml | Removes legacy kebab BYO agent fixture project file. |
| go/core/test/e2e/agents/kebab/kebab/agent.py | Removes legacy kebab BYO agent implementation. |
| go/core/test/e2e/agents/kebab/kebab/agent-card.json | Removes legacy kebab agent-card fixture. |
| go/core/test/e2e/agents/kebab/kebab/init.py | Removes legacy kebab package init. |
| go/core/test/e2e/agents/kebab/agent.yaml | Removes legacy kebab SandboxAgent manifest. |
| go/core/test/e2e/agents/kebab/README.md | Removes legacy kebab BYO agent documentation. |
| go/core/test/e2e/agents/kebab/Dockerfile | Removes legacy kebab agent image build Dockerfile. |
| go/core/test/e2e/agents/kebab/.python-version | Removes legacy kebab fixture Python version pin. |
| go/core/test/e2e/README.md | Replaces legacy debugging notes with E2E suite usage and lifecycle fixture instructions. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_test.go | Updates lifecycle tests to match removed PauseImage in ActorTemplate spec. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_shared.go | Adds SecretKeyRef resolution for env vars; drops PauseImage default. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_openclaw_test.go | Updates test expectations to literal env values. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_openclaw.go | Resolves env vars before ActorTemplate conversion. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_actortemplate.go | Removes PauseImage from generated ActorTemplate spec. |
| go/core/pkg/sandboxbackend/substrate/lifecycle_acp.go | Resolves env vars before ActorTemplate conversion. |
| go/core/pkg/sandboxbackend/substrate/config_hash_test.go | Passes config secret into BuildSandbox and updates builder signatures. |
| go/core/pkg/sandboxbackend/substrate/agents_backend.go | Threads config secret/context into lifecycle builder. |
| go/core/pkg/sandboxbackend/substrate/agent_lifecycle_test.go | Updates env conversion expectations and new lifecycle builder signature. |
| go/core/pkg/sandboxbackend/substrate/agent_lifecycle.go | Resolves env vars (SecretKeyRefs) before building ActorTemplate env list; removes SRT settings env entry. |
| go/core/pkg/sandboxbackend/backend.go | Extends BuildInput to pass ConfigSecret for env resolution. |
| go/core/pkg/env/kagent.go | Removes KAGENT_SRT_SETTINGS_PATH env var definition. |
| go/core/pkg/app/app.go | Removes full image digests and pause image flag; updates v2 runtime init signature and lifecycle defaults. |
| go/core/internal/controller/translator/agent/manifest_builder_test.go | Removes SRT/skills-init tests; keeps minimal config secret data test. |
| go/core/internal/controller/translator/agent/manifest_builder.go | Removes skills-init and SRT settings generation; passes config secret into sandbox backend. |
| go/core/internal/controller/translator/agent/imageconfig_test.go | Removes “full” runtime image variant tests. |
| go/core/internal/controller/translator/agent/digest_testmain_external_test.go | Removes full image digest test setup. |
| go/core/internal/controller/translator/agent/deployments.go | Simplifies runtime image resolution (no “full” variant) and updates call sites. |
| go/core/internal/controller/translator/agent/adk_api_translator.go | Removes skills-init image config, removes skills-init container build logic, and drops skills-init from config hashing. |
| go/core/cmd/skills-init/main.go | Deletes obsolete skills-init binary entrypoint. |
| go/core/cmd/controller-v2/main.go | Removes pause image env/config from v2 runtime startup. |
| go/api/v1alpha3/agent_types.go | Removes SkillForAgent.InitContainer from API and updates skill auth docs. |
| go/api/config/crd/bases/kagent.dev_sandboxagents.yaml | Removes initContainer schema and updates S3 region/auth docs. |
| go/adk/pkg/tools/skills_test.go | Removes dependency on SRT settings env var for skills tool tests. |
| go/adk/pkg/tools/skills.go | Uses non-SRT command executor directly. |
| go/adk/pkg/skills/skills_tools.go | Uses non-SRT command executor directly. |
| go/adk/pkg/skills/shell_test.go | Removes fake SRT install/setup; uses new executor. |
| go/adk/pkg/skills/shell.go | Removes SRT settings requirement and executes via bash directly. |
| go/adk/pkg/skills/discovery_test.go | Removes fake SRT setup and uses new executor. |
| go/adk/pkg/config/config_usage.go | Updates docs to remove SRT settings mention. |
| go/adk/pkg/config/config_materialize.go | Stops materializing SRT settings file. |
| go/adk/pkg/config/config_loader_test.go | Updates materialize test to no longer expect SRT settings file. |
| go/adk/pkg/agent/agent_test.go | Removes KAGENT_SRT_SETTINGS_PATH requirement from agent tool building tests. |
| go/README.md | Updates controller digest embedding docs to reflect removed full images. |
| go/Dockerfile.full | Removes full Go runtime image definition. |
| go/Dockerfile | Switches Go runtime base to Alpine and installs bash/ca-certs. |
| examples/substrate-openclaw/README.md | Bumps Substrate version to 0.0.13. |
| docker/skills-init/Dockerfile | Removes skills-init image build definition. |
| Makefile | Removes build/push targets for skills-init and “full” images; updates CI-related targets accordingly. |
| .github/workflows/tag.yaml | Removes publishing/tagging for full golang-adk and skills-init images. |
| .github/workflows/image-scan.yaml | Removes image scan matrix entries for skills-init and golang-adk-full. |
| .github/workflows/ci.yaml | Enables E2E job, provisions Substrate v0.0.13, applies lifecycle fixtures, and runs lifecycle E2E tests. |
| .github/dependabot.yml | Drops dependabot updates for removed docker/skills-init path. |
Files not reviewed (1)
- go/api/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
go/core/v2/translator/compiler.go:1
resolveEnvironmentignoresSecretKeySelector.Optional. If an env var references an optional secret/key and it is missing, Kubernetes behavior is to omit the env var (not fail reconciliation). Please handleref.Optional != nil && *ref.Optionalby skipping/removing the variable when the secret is NotFound or the key is absent; for non-optional refs, return a wrapped error with context (secret name, key, env var name). This also needs a unit test covering optional secret missing + optional key missing behavior.
go/core/pkg/sandboxbackend/substrate/lifecycle_shared.go:1- For optional SecretKeyRefs, this currently leaves an env var present with an empty literal value (by clearing
ValueFrombut not removing the entry). That differs from Kubernetes semantics where the env var is omitted entirely when the optional secret/key is missing. Consider rebuilding theresolvedslice (filtering out missing optional entries) or marking entries for removal and compacting after the loop.
python/packages/kagent-skills/src/kagent/skills/shell.py:1 - Executing via
bash -cwill interpret shell metacharacters incommand. Ifcommandis derived from user/LLM input (even indirectly), this is a command-injection boundary. If the intent is to remain safe-by-default, consider switching to an argv-based execution model (no shell) and representing commands as[]string/list tokens instead of a single string; if the intent is explicitly “unsandboxed shell,” consider updating module/tool documentation to clearly warn that arbitrary shell evaluation occurs.
go/core/v2/translator/compiler.go:1 - Runtime env resolution is new behavior with several edge cases (optional SecretKeyRefs; missing secret vs missing key; error message context). The existing compiler test covers the “happy path” for resolved secrets; please add tests for optional SecretKeyRef behavior (missing secret and missing key) and for non-secret
ValueFromsources producing the intended error.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
122
| cmd := exec.CommandContext(ctx, "bash", "-c", command) | ||
| cmd.Dir = workingDir |
| # Use distroless as minimal base image to package the manager binary | ||
| # Refer to https://github.com/GoogleContainerTools/distroless for more details | ||
| FROM gcr.io/distroless/static:nonroot | ||
| FROM alpine:3.22 |
|
Warning Testing pausedMonthly snapshot limit reached. Update your plan for additional snapshots and to resume testing. |
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
brandonkeung
added a commit
to brandonkeung/kagent
that referenced
this pull request
Aug 30, 2026
…counting Addresses mesutoezdil's review findings on PR kagent-dev#2267. Six were real bugs; all are regression-tested (each new test verified to fail against the pre-fix code). Go: - ListDirContent reported a symlink's own size -- the byte length of its stored target path -- because entry.Info() is Lstat-based. It now stats the target, so a symlinked file reports the file's size and a broken link is listed bare, matching Python's pathlib behavior. The needed os.Stat result was already being computed and discarded. - ReadFileContent had no regular-file guard, so read_file on a FIFO with no writer blocked forever with no timeout on the path. It now rejects non-regular files, as GrepContent already did. - Line truncation sliced bytes, not runes. Beyond emitting invalid UTF-8 from a split sequence, it cut CJK text at ~668 characters rather than the 2000 the tool descriptions promise. Both sites now share a truncateRunes helper that cuts on a rune boundary, matching Python's per-code-point slicing. - Wrap the bare errors in GrepContent and ReadFileContent with %w. Python: - grep_content dropped broken symlinks silently. A dangling link is a genuine read failure, so it now counts toward the "N entries could not be read" annotation, matching Go's walkEntryUnreadable. FIFOs and sockets stay silent, matching walkEntrySkip. - Entries were validated only against allowed_root, which in production is the whole session dir plus the skills dir -- wider than the directory being searched. A symlink could therefore pull in a sibling the caller never asked about, contradicting both the tool description and the README. Entries are now also bounded by the search root, as Go already does. Also corrects comments in five places that described list_files/grep_file as opt-in "alongside bash". Upstream removed bash's gating entirely in kagent-dev#2498, so bash is now unconditional in both runtimes and that comparison was false. Adds a test pinning the KAGENT_ENABLE_FILE_SEARCH_TOOLS literal in go/adk/pkg/tools to the `kagent env` registry entry in go/core/pkg/env so the two cannot drift. The import is test-only and does not add a dependency from the agent runtime onto the control-plane module. Signed-off-by: brandonkeung <brandonlkeung@gmail.com>
brandonkeung
added a commit
to brandonkeung/kagent
that referenced
this pull request
Aug 30, 2026
grep_file's scanner set a 1MB line buffer; ReadFileContent kept bufio's 64KB default. A file with one longer line -- a minified bundle, a single-line JSON blob -- therefore failed read_file outright, losing every other line in the file, while grep_file handled the same file fine and read_file's own tool description promises such lines are truncated. Python truncated correctly throughout, so this was a Go-only regression introduced alongside grep_file. Extract scanFileLines as the single reader behind both. It owns the non-regular-file rejection (previously duplicated), the buffer cap, and error wrapping, so the two paths can no longer drift on any of the three. Lines past maxLineBytes still error rather than truncate: uncapping would mean buffering an arbitrarily long line in a sandbox reading untrusted files. Also in this change: - Wrap ListDirContent's os.ReadDir error, the last bare return beside a wrapped one. - Correct file_search_tools_enabled()'s docstring, which still claimed list_files/grep_file are "disabled by default, same as bash". Bash's gate was removed upstream in kagent-dev#2498; this was the last of six such sites. - Drop the _validate_path call in grep_content's entry loop. The search-root bound added beside it is strictly narrower, and file_or_dir_path is already validated against allowed_root, so the wider check is dead. - Give Python the _MAX_LINE_CHARS/_truncate_line pair Go already had, replacing four repetitions of the literal 2000. - Express the three path resolvers as pathPolicy values. allowSkillsRoot had been picking the denial message as a side effect, which would misdescribe any future resolver that denied the skills root for a reason other than writability. - Fold TestResolveReadPath_AllowsSymlinkedSkillsDirectory and TestResolveWritePath_BlocksSkillsSymlink into TestResolvePathContainment, which already covered both cells of that matrix. - Correct the grep_file call site's no-timeout rationale, which cited RE2's linearity -- an answer about the match, not the walk that GrepContent's own doc comment identifies as the unbounded part. Signed-off-by: brandonkeung <brandonlkeung@gmail.com>
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
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.
Summary
Validation
go test -v github.com/kagent-dev/kagent/go/core/test/e2e -failfast -shuffle=ongit diff --check