Repository navigation
Prow presubmit unit tests - #1273
kubernetes-prow[bot] merged 14 commits into
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
Hi @alexatakvelon. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds unit-test suites for multiple sandbox examples and sandboxed tools. Adds Python presubmit runners for local tests and Kubernetes-based end-to-end runners for Python runtime and Gemini computer-use sandboxes. Adds a Go tester for the Python runtime sandbox. ChangesPresubmit runners
Example test suites
Sandboxed tools tests
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
examples/sandboxed-tools/pkg/tools/fake_sandbox_test.go (1)
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail unexpected sandbox calls instead of returning success.
Returning an empty successful result can let extra
ExecCommandcalls pass unnoticed. Return an error so tests fail when behavior adds an unqueued operation.Proposed fix
-import "context" +import ( + "context" + "fmt" +) ... if i >= len(f.responses) { - return &ExecCommandResult{}, nil + return nil, fmt.Errorf("unexpected ExecCommand call %d", i+1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/sandboxed-tools/pkg/tools/fake_sandbox_test.go` around lines 38 - 40, Update the fake sandbox response handling in the relevant ExecCommand implementation so an index at or beyond len(f.responses) returns an error instead of an empty successful ExecCommandResult. Preserve the existing queued-response behavior for valid indices, and make the error clearly identify an unexpected or unqueued sandbox call.examples/python-runtime-sandbox/tester.go (1)
37-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo request timeout on HTTP calls.
doGet,doPostJSON, anddoPostUploadusehttp.Get/http.Postwith the default client, which has no timeout. If the server hangs (e.g., pod not ready, network issue), this CI tool can block indefinitely instead of failing fast.Proposed fix
+var httpClient = &http.Client{Timeout: 30 * time.Second} + func doGet(target string) (*http.Response, []byte) { - resp, err := http.Get(target) + resp, err := httpClient.Get(target)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/python-runtime-sandbox/tester.go` around lines 37 - 91, Update doGet, doPostJSON, and doPostUpload to perform their HTTP requests through a client with an explicit finite timeout instead of the package-level http.Get/http.Post helpers. Reuse a shared configured client or timeout consistently across all three functions, while preserving their existing request bodies, headers, response reading, and error handling.dev/ci/presubmits/test-python-runtime-sandbox (1)
103-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed sleep for port-forward readiness is flaky.
A hardcoded
time.sleep(3)assumes the port-forward is ready in time; under CI load this can be flaky. Consider polling the port (e.g., retry a lightweight GET) until it responds or a timeout elapses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/ci/presubmits/test-python-runtime-sandbox` around lines 103 - 106, Replace the fixed time.sleep(3) after starting the port-forward in the sandbox setup with readiness polling for the forwarded port, such as repeated lightweight requests, until it responds or a bounded timeout expires. Keep using the port_forward process from this block and fail clearly if readiness is not achieved within the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dev/ci/presubmits/test-gemini-cu-sandbox`:
- Around line 100-106: Update the secret creation flow around the kubectl
subprocess calls to stop embedding GEMINI_API_KEY in command-line arguments.
Generate the Secret manifest in memory and provide it only through stdin to the
kubectl apply invocation, preserving the existing secret name and key while
ensuring command failures cannot expose the credential through arguments or
CalledProcessError output.
In `@dev/ci/presubmits/test-langchain`:
- Around line 40-41: Update the dependency installation step in the langchain
presubmit before pytest runs to install LangGraph using the version constraint
declared by the langchain example. Keep the existing pytest installation and
ensure both dependencies are available before test collection.
In `@dev/ci/presubmits/test-python-runtime-sandbox`:
- Around line 58-66: Update cleanup() to handle subprocess.TimeoutExpired from
port_forward.wait(timeout=10), ensuring the port-forward process is terminated
or otherwise reaped before continuing. Preserve execution of the kubectl delete
for sandbox-python-kind.yaml even when waiting for the port-forward times out.
- Around line 108-110: Update the presubmit command in the
python-runtime-sandbox runner to execute the Go client with go run tester.go
127.0.0.1 8888 from EXAMPLE_DIR, replacing the existing python3 tester.py
invocation while preserving the current run options.
---
Nitpick comments:
In `@dev/ci/presubmits/test-python-runtime-sandbox`:
- Around line 103-106: Replace the fixed time.sleep(3) after starting the
port-forward in the sandbox setup with readiness polling for the forwarded port,
such as repeated lightweight requests, until it responds or a bounded timeout
expires. Keep using the port_forward process from this block and fail clearly if
readiness is not achieved within the timeout.
In `@examples/python-runtime-sandbox/tester.go`:
- Around line 37-91: Update doGet, doPostJSON, and doPostUpload to perform their
HTTP requests through a client with an explicit finite timeout instead of the
package-level http.Get/http.Post helpers. Reuse a shared configured client or
timeout consistently across all three functions, while preserving their existing
request bodies, headers, response reading, and error handling.
In `@examples/sandboxed-tools/pkg/tools/fake_sandbox_test.go`:
- Around line 38-40: Update the fake sandbox response handling in the relevant
ExecCommand implementation so an index at or beyond len(f.responses) returns an
error instead of an empty successful ExecCommandResult. Preserve the existing
queued-response behavior for valid indices, and make the error clearly identify
an unexpected or unqueued sandbox call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e5e5530-1443-4278-9233-06a3e6ad3836
📒 Files selected for processing (17)
dev/ci/presubmits/test-analytics-tooldev/ci/presubmits/test-gemini-cu-sandboxdev/ci/presubmits/test-langchaindev/ci/presubmits/test-mcp-server-sandboxdev/ci/presubmits/test-python-runtime-sandboxexamples/analytics-tool/analytics-tool/test_main.pyexamples/gemini-cu-sandbox/test_main.pyexamples/langchain/test_coding_agent.pyexamples/mcp-server-sandbox/test_mcp_server.pyexamples/python-runtime-sandbox/test_main.pyexamples/python-runtime-sandbox/tester.goexamples/sandboxed-tools/pkg/tools/fake_sandbox_test.goexamples/sandboxed-tools/pkg/tools/list_files_test.goexamples/sandboxed-tools/pkg/tools/read_file_test.goexamples/sandboxed-tools/pkg/tools/registry_test.goexamples/sandboxed-tools/pkg/tools/run_command_test.goexamples/sandboxed-tools/pkg/tools/write_file_test.go
| secret_yaml = subprocess.run( | ||
| ["kubectl", "create", "secret", "generic", "gemini-api-key", | ||
| f"--from-literal=key={gemini_api_key}", "--dry-run=client", "-o", "yaml"], | ||
| env=env, capture_output=True, text=True, check=True, | ||
| ).stdout | ||
| subprocess.run(["kubectl", "apply", "-f", "-"], | ||
| input=secret_yaml, text=True, env=env, check=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep GEMINI_API_KEY out of process arguments and failure logs.
--from-literal=key={gemini_api_key} exposes the credential via the running process command line; if this command fails, line 130 also prints it through CalledProcessError. Generate the Secret manifest in memory and pass it only as stdin to kubectl apply.
Proposed fix
+import base64
import os
...
- secret_yaml = subprocess.run(
- ["kubectl", "create", "secret", "generic", "gemini-api-key",
- f"--from-literal=key={gemini_api_key}", "--dry-run=client", "-o", "yaml"],
- env=env, capture_output=True, text=True, check=True,
- ).stdout
+ encoded_key = base64.b64encode(gemini_api_key.encode()).decode()
+ secret_yaml = (
+ "apiVersion: v1\nkind: Secret\nmetadata:\n"
+ " name: gemini-api-key\n"
+ "type: Opaque\ndata:\n"
+ f" key: {encoded_key}\n"
+ )
subprocess.run(["kubectl", "apply", "-f", "-"],
input=secret_yaml, text=True, env=env, check=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secret_yaml = subprocess.run( | |
| ["kubectl", "create", "secret", "generic", "gemini-api-key", | |
| f"--from-literal=key={gemini_api_key}", "--dry-run=client", "-o", "yaml"], | |
| env=env, capture_output=True, text=True, check=True, | |
| ).stdout | |
| subprocess.run(["kubectl", "apply", "-f", "-"], | |
| input=secret_yaml, text=True, env=env, check=True) | |
| import base64 | |
| ... | |
| encoded_key = base64.b64encode(gemini_api_key.encode()).decode() | |
| secret_yaml = ( | |
| "apiVersion: v1\nkind: Secret\nmetadata:\n" | |
| " name: gemini-api-key\n" | |
| "type: Opaque\ndata:\n" | |
| f" key: {encoded_key}\n" | |
| ) | |
| subprocess.run(["kubectl", "apply", "-f", "-"], | |
| input=secret_yaml, text=True, env=env, check=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/ci/presubmits/test-gemini-cu-sandbox` around lines 100 - 106, Update the
secret creation flow around the kubectl subprocess calls to stop embedding
GEMINI_API_KEY in command-line arguments. Generate the Secret manifest in memory
and provide it only through stdin to the kubectl apply invocation, preserving
the existing secret name and key while ensuring command failures cannot expose
the credential through arguments or CalledProcessError output.
There was a problem hiding this comment.
Pull request overview
This PR expands presubmit CI coverage for multiple examples/ by adding new unit tests (Go + Python) and introducing dedicated Prow presubmit scripts to run those tests; it also fixes the gemini-cu-sandbox presubmit so it installs the Python client from the correct directory.
Changes:
- Added Go unit tests for
examples/sandboxed-tools/pkg/toolsand supporting fakes. - Added Python unit tests for several FastAPI-based examples (analytics-tool, python-runtime-sandbox, mcp-server-sandbox, langchain, gemini-cu-sandbox).
- Added/updated
dev/ci/presubmits/*scripts to run these suites (some unit-only, some including kind/e2e flows).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/sandboxed-tools/pkg/tools/fake_sandbox_test.go | Adds a Sandbox test double for tool unit tests. |
| examples/sandboxed-tools/pkg/tools/list_files_test.go | Unit tests for ListFilesTool schema and runtime behavior. |
| examples/sandboxed-tools/pkg/tools/read_file_test.go | Unit tests for ReadFileTool schema and runtime behavior. |
| examples/sandboxed-tools/pkg/tools/registry_test.go | Unit tests for registry validation, sorting, dispatch, and argument parsing. |
| examples/sandboxed-tools/pkg/tools/run_command_test.go | Unit tests for RunCommand schema and execution formatting. |
| examples/sandboxed-tools/pkg/tools/write_file_test.go | Unit tests for WriteFileTool mkdir/write behavior and error handling. |
| examples/python-runtime-sandbox/tester.go | Adds a Go port of the python runtime e2e tester. |
| examples/python-runtime-sandbox/test_main.py | Adds FastAPI endpoint + traversal-guard unit tests for python-runtime-sandbox. |
| examples/mcp-server-sandbox/test_mcp_server.py | Adds unit tests for the blob-store MCP server functions. |
| examples/langchain/test_coding_agent.py | Adds unit tests for coding agent logic + executor env allowlisting/timeout behavior. |
| examples/gemini-cu-sandbox/test_main.py | Updates gemini-cu-sandbox unit tests to cover API key precedence and missing-key behavior. |
| examples/analytics-tool/analytics-tool/test_main.py | Adds FastAPI endpoint unit tests and working dir isolation. |
| dev/ci/presubmits/test-python-runtime-sandbox | New presubmit that runs unit tests, builds/loads image, deploys kind resources, and runs the python tester. |
| dev/ci/presubmits/test-mcp-server-sandbox | New unit-tests-only presubmit for mcp-server-sandbox. |
| dev/ci/presubmits/test-langchain | New unit-tests-only presubmit for the langchain coding_agent example. |
| dev/ci/presubmits/test-gemini-cu-sandbox | Updates gemini-cu-sandbox presubmit to install the Python client from the correct directory before running unittest. |
| dev/ci/presubmits/test-analytics-tool | New unit-tests-only presubmit for analytics-tool. |
Comments suppressed due to low confidence (3)
examples/python-runtime-sandbox/tester.go:41
- doGet uses http.Get (no timeout). With the shared timeout-enabled client, use httpClient.Get to ensure the test fails promptly instead of hanging.
Please apply this change locally (don’t use GitHub’s “Commit suggestion” button), otherwise the Kubernetes CLA check can fail due to Copilot being added as a co-author.
func doGet(target string) (*http.Response, []byte) {
resp, err := http.Get(target)
if err != nil {
fail("An error occurred: %v", err)
}
examples/python-runtime-sandbox/tester.go:55
- This POST uses http.Post (no timeout). Using the shared timeout-enabled httpClient.Post helps avoid hanging the presubmit on a stalled endpoint.
Please apply this change locally (don’t use GitHub’s “Commit suggestion” button), otherwise the Kubernetes CLA check can fail due to Copilot being added as a co-author.
resp, err := http.Post(target, "application/json", bytes.NewReader(encoded))
examples/python-runtime-sandbox/tester.go:81
- This upload request uses http.Post (no timeout). Switching to the shared timeout-enabled httpClient.Post keeps the tester from hanging forever if the server stops responding.
Please apply this change locally (don’t use GitHub’s “Commit suggestion” button), otherwise the Kubernetes CLA check can fail due to Copilot being added as a co-author.
resp, err := http.Post(target, w.FormDataContentType(), &buf)
| _workspace_dir = tempfile.mkdtemp(prefix="mcp-server-test-") | ||
| os.environ["MCP_WORKSPACE"] = _workspace_dir |
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "mime/multipart" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "strings" | ||
| ) | ||
|
|
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Nice test coverage overall (I verified the new unit tests against the current runtime sources, and the sandboxed-tools Go tests slot into the existing test-unit go list ./... run with go-cmp already in go.mod). However, the two new e2e-style presubmits cannot pass as generated by test-infra's job generator, so requesting changes on the CI wiring. Details inline.
|
|
||
| class GeminiCUSandboxTestRunner(TestRunner): | ||
| def __init__(self): | ||
| super().__init__("gemini-cu-test", "e2e test for the gemini-cu-sandbox example") |
There was a problem hiding this comment.
Blocking (job would never pass): Prow jobs for this repo are generated by test-infra's config/jobs/kubernetes-sigs/agent-sandbox/generate_jobs.py, and its is_e2e() only picks the e2e profile (privileged container + preset-dind-enabled + runner.sh wrapper) when the script name matches e2e|benchmark. test-gemini-cu-sandbox doesn't, so it will be generated as an unprivileged "local" job with no Docker daemon — docker build, kind and the cluster bring-up in TestRunner.setup_cluster() will all fail unconditionally.
Please rename the script to include e2e (e.g. test-e2e-gemini-cu-sandbox), or land a matching override in test-infra first. Getting the name right now is cheap; per the generator's own comments, renaming a presubmit later churns branch-protection contexts and testgrid history.
| env["KUBECONFIG"] = os.path.join(self.repo_root, "bin", "KUBECONFIG") | ||
|
|
||
| gemini_api_key = env.get("GEMINI_API_KEY", "") | ||
| if not gemini_api_key: |
There was a problem hiding this comment.
Blocking until the secret is wired up: no Prow preset currently supplies GEMINI_API_KEY to agent-sandbox presubmits, so the generated job will fail on every PR regardless of the code under test. Until a test-infra secret preset exists, this job should be generated as optional/manual (like test-skill-eval, which is kept manual in PRESUBMIT_OVERRIDES "until its credential requirements are sorted out").
Also, this check runs inside run_tests(), i.e. after TestRunner.main() has already spent several minutes creating the kind cluster and deploying the controller. Consider checking the env var at script start (before runner.main()) so a missing key fails fast.
|
|
||
| class PythonRuntimeSandboxTestRunner(TestRunner): | ||
| def __init__(self): | ||
| super().__init__("python-runtime-sandbox-test", "e2e test for the python-runtime-sandbox example") |
There was a problem hiding this comment.
Blocking (same job-generation issue as test-gemini-cu-sandbox): without e2e in the script name, test-infra's generate_jobs.py emits this as an unprivileged local job with no Docker-in-Docker, so the docker build/kind load/cluster bring-up below cannot work. Please rename (e.g. test-e2e-python-runtime-sandbox) or add a generator override in test-infra.
|
|
||
| print("\n-- Waiting for sandbox pod to be ready --") | ||
| kubectl("wait", "--for=condition=ready", "pod", | ||
| "--selector", POD_LABEL_SELECTOR, "--timeout=120s") |
There was a problem hiding this comment.
Flake risk: kubectl apply above only creates the Sandbox CR; the pod is created asynchronously by the controller. kubectl wait --for=condition=ready pod --selector ... does not wait for a matching resource to appear — if the pod object doesn't exist yet it exits immediately with no matching resources found, which raises via check=True and fails the whole run. Wait for the Sandbox itself first (e.g. kubectl wait sandbox/sandbox-python-example --for=condition=Ready --timeout=120s) or poll until a pod matches the selector before this wait.
kubernetes-sigs/agent-sandbox#1273 renamed test-gemini-cu-sandbox and test-python-runtime-sandbox to test-e2e-gemini-cu-sandbox / test-e2e-python-runtime-sandbox so generate_jobs.py's is_e2e() name matching (and any future regeneration) picks up the privileged DinD profile these jobs need. Update run_if_changed and command to match.
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
The unit-test scaffolding is a welcome addition, but the gemini e2e presubmit has a step that can never pass — see inline.
| print("\n-- Running computer-use unittest --") | ||
| result = run( | ||
| ["python3", "-m", "unittest", | ||
| "clients.python.agentic-sandbox-client.test_computer_use_extension"], |
There was a problem hiding this comment.
This dotted name is not importable — "agentic-sandbox-client" contains hyphens, so unittest wraps it in a _FailedTest and this step always fails (verified: FAILED (errors=1)). Run it as python3 -m unittest test_computer_use_extension with cwd set to the client dir instead.
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Reviewed at 1e22d5f. No findings.
|
/lgtm |
kubernetes-prow
Bot
added
ok-to-test
kubernetes-prow
Bot
added
the
lgtm
Adds test_main.py covering the FastAPI endpoints and the get_safe_path traversal guard, and a new test-python-runtime-sandbox presubmit that runs the unit tests before the existing kind/e2e flow.
Adds unit tests for RunCommand, ReadFileTool, WriteFileTool, ListFilesTool, and Registry (nil/non-pointer validation, sorted schema listing, dispatch-by-name, JSON argument unmarshaling, and that registered tool templates aren't mutated across calls). Uses a shared fakeSandbox test double refactored to record a call sequence rather than a single call, since WriteFileTool invokes ExecCommand twice. Also fixes test-python-runtime-sandbox: the docker build for the runtime image was missing --load, which the sibling test-gemini-cu-sandbox job already required. Without it, the image likely wouldn't be visible to kind load docker-image in this CI environment's buildx configuration.
…e2e tests test-e2e-gemini-cu-sandbox and test-e2e-python-runtime-sandbox each build their own example-specific runtime image(s) directly in run_tests(), but inherited TestRunner's default setup_cluster(), which shells out to push-images with no --images filter and builds every Dockerfile in the repo (~17 images) before the test-specific logic ever runs. Confirmed via grep over k8s/ that deploy-to-kube's manifests reference exactly one image (agent-sandbox-controller), so nothing else built there is needed. Follows the same override already used by test-load-test and test-migration (both scope to --controller-only); this uses --images agent-sandbox-controller instead, which is strictly narrower since --controller-only would still build the unrelated root-level sandbox-router-go and kata-aks example images that push-images doesn't exclude by default.
Requesting /exists/. gets encoded_file_path="." on the server side
(main.py's /exists/{encoded_file_path:path} just echoes back
urllib.parse.unquote(encoded_file_path) verbatim in the response, with
no normalization to empty string anywhere). The assertion expected
path == "" instead of path == ".", so this check could never pass.
Confirmed by actually running the e2e test end-to-end for the first
time against a live cluster: Health Check, Execute, and List Files all
passed, only this assertion failed. tester.py (the now-superseded
Python version this was ported from) has the identical wrong
assertion, which is presumably why it went unnoticed -- no CI job ran
either tester against a live server before this PR.
fake_sandbox_test.go's ExecCommand and registry_test.go's valueTool.Run implement interface methods that don't need every parameter; name the unused ones _ per revive's unused-parameter check.
1e22d5f to
048259b
Compare
kubernetes-prow
Bot
removed
the
lgtm
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dev/ci/presubmits/test-langchain (1)
47-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider using
sys.executableinstead of barepip/python3.Line 53 calls
pip installand Line 56 callspython3 -m pytestusing bare binary names. If the CI environment has multiple Python installations,piponPATHcan install packages into an interpreter different from the one that runspython3 -m pytest, solanggraphmight install successfully but pytest still fails withModuleNotFoundError. Usesys.executableto guarantee the install target matches the interpreter that runs the tests.♻️ Proposed fix
print("\n-- Installing langchain unit test dependencies --") langgraph_requirement = read_requirement("langgraph") - run(["pip", "install", "--break-system-packages", "pytest", langgraph_requirement], check=True) + run([sys.executable, "-m", "pip", "install", "--break-system-packages", "pytest", langgraph_requirement], check=True) print("\n-- Running langchain unit tests --") - result = run(["python3", "-m", "pytest", "test_coding_agent.py", "-v"], cwd=EXAMPLE_DIR) + result = run([sys.executable, "-m", "pytest", "test_coding_agent.py", "-v"], cwd=EXAMPLE_DIR)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/ci/presubmits/test-langchain` around lines 47 - 56, Update the test dependency installation and test execution in the run flow to use sys.executable instead of the bare “pip” and “python3” commands, ensuring both operations target the same interpreter. Preserve the existing arguments, working directory, and check behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/mcp-server-sandbox/test_mcp_server.py`:
- Around line 29-33: Update the setup around the mcp_server import to save the
existing MCP_WORKSPACE value, set the temporary workspace only for importing
mcp_server, then restore the original environment state afterward, including
removing the variable if it was previously unset. Leave mcp_server.WORKSPACE
using the scratch directory resolved during import.
---
Nitpick comments:
In `@dev/ci/presubmits/test-langchain`:
- Around line 47-56: Update the test dependency installation and test execution
in the run flow to use sys.executable instead of the bare “pip” and “python3”
commands, ensuring both operations target the same interpreter. Preserve the
existing arguments, working directory, and check behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74255f13-ba4a-4032-badc-1de2fc6f130b
📒 Files selected for processing (17)
dev/ci/presubmits/test-analytics-tooldev/ci/presubmits/test-e2e-gemini-cu-sandboxdev/ci/presubmits/test-e2e-python-runtime-sandboxdev/ci/presubmits/test-langchaindev/ci/presubmits/test-mcp-server-sandboxexamples/analytics-tool/analytics-tool/test_main.pyexamples/gemini-cu-sandbox/test_main.pyexamples/langchain/test_coding_agent.pyexamples/mcp-server-sandbox/test_mcp_server.pyexamples/python-runtime-sandbox/test_main.pyexamples/python-runtime-sandbox/tester.goexamples/sandboxed-tools/pkg/tools/fake_sandbox_test.goexamples/sandboxed-tools/pkg/tools/list_files_test.goexamples/sandboxed-tools/pkg/tools/read_file_test.goexamples/sandboxed-tools/pkg/tools/registry_test.goexamples/sandboxed-tools/pkg/tools/run_command_test.goexamples/sandboxed-tools/pkg/tools/write_file_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/sandboxed-tools/pkg/tools/run_command_test.go
- dev/ci/presubmits/test-analytics-tool
- examples/sandboxed-tools/pkg/tools/read_file_test.go
- examples/sandboxed-tools/pkg/tools/write_file_test.go
- examples/analytics-tool/analytics-tool/test_main.py
- examples/sandboxed-tools/pkg/tools/list_files_test.go
- examples/sandboxed-tools/pkg/tools/registry_test.go
- examples/gemini-cu-sandbox/test_main.py
- examples/langchain/test_coding_agent.py
- dev/ci/presubmits/test-mcp-server-sandbox
- examples/python-runtime-sandbox/test_main.py
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Reviewed at 048259b: presubmit wiring, mocked-boundary unit tests, and the tester.go port all look solid; no findings.
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
"clients.python.agentic-sandbox-client.test_computer_use_extension" is not a valid Python module path -- agentic-sandbox-client contains a dash, which Python's import system can't parse as a package segment. Run the test by name from CLIENT_DIR instead, matching how pytest/unittest actually discover it.
kubernetes-prow
Bot
removed
the
lgtm
| env = os.environ.copy() | ||
| env["KUBECONFIG"] = os.path.join(self.repo_root, "bin", "KUBECONFIG") | ||
|
|
||
| gemini_api_key = env["GEMINI_API_KEY"] |
There was a problem hiding this comment.
Direct dict access raises KeyError if GEMINI_API_KEY is absent when run_tests is called outside the __main__ guard. Use env.get('GEMINI_API_KEY') with an explicit check to surface a clear error message.
| path = os.path.join(EXAMPLE_DIR, "requirements.txt") | ||
| with open(path) as f: | ||
| for line in f: | ||
| if line.strip().startswith(name): |
There was a problem hiding this comment.
startswith(name) will match any package whose name is a prefix of another (e.g. langgraph matches langgraph-community). Anchor with line.strip().startswith(name + '==') or split on ==/>= to avoid pulling the wrong version.
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
- test-e2e-gemini-cu-sandbox: use env.get() with an explicit check for GEMINI_API_KEY in run_tests() instead of direct dict access, so a missing key raises a clear error instead of KeyError (aditya-shantanu). - test-langchain: anchor the requirements.txt line match on "name==" / "name>=" instead of a bare startswith(name), which could match a same-prefixed package (e.g. langgraph-community) and install the wrong pin; also run pip/pytest via sys.executable instead of bare pip/python3 so both target the same interpreter (aditya-shantanu, CodeRabbit). - test_mcp_server.py: save and restore the prior MCP_WORKSPACE value around the mcp_server import instead of leaving the scratch dir set for the rest of the pytest process (CodeRabbit).
kubernetes-prow
Bot
removed
the
lgtm
pip install mcp with no version pin currently resolves to mcp==2.0.0, a breaking rewrite that dropped mcp.server.fastmcp -- the exact module mcp_server.py imports. requirements.txt already pins mcp==1.27.2, so read that pin the same way test-langchain already does for langgraph, instead of letting the presubmit float onto whatever's newest on PyPI.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dev/ci/presubmits/test-mcp-server-sandbox (2)
40-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the
mcprequirement exact.
read_requirementacceptsmcp>=..., although the example declaresmcp==1.27.2. Restrict the helper tomcp==...so the presubmit installs the tested MCP SDK version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/ci/presubmits/test-mcp-server-sandbox` around lines 40 - 42, Update read_requirement to accept the mcp dependency only when it starts with the exact mcp== prefix, removing support for mcp>= matches so the presubmit selects the declared tested SDK version.
52-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBind installation and test execution to the same Python interpreter.
Use
sys.executable -m pipandsys.executable -m pytestto prevent dependencies from being installed into a different environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/ci/presubmits/test-mcp-server-sandbox` around lines 52 - 55, Update the installation and test commands in the presubmit script to use sys.executable: invoke pip via sys.executable -m pip and pytest via sys.executable -m pytest, ensuring both operations use the same Python interpreter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dev/ci/presubmits/test-mcp-server-sandbox`:
- Around line 40-42: Update read_requirement to accept the mcp dependency only
when it starts with the exact mcp== prefix, removing support for mcp>= matches
so the presubmit selects the declared tested SDK version.
- Around line 52-55: Update the installation and test commands in the presubmit
script to use sys.executable: invoke pip via sys.executable -m pip and pytest
via sys.executable -m pytest, ensuring both operations use the same Python
interpreter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6447976-a800-4474-8b62-c106397062a4
📒 Files selected for processing (1)
dev/ci/presubmits/test-mcp-server-sandbox
|
@alexatakvelon: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aditya-shantanu, alexatakvelon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
f65b6b9
into
kubernetes-sigs:main
main.py is a self-contained FastAPI app (health/metrics/init/envs/exec/ files), same shape as python-runtime-sandbox and mcp-server-sandbox already covered in kubernetes-sigs#1273, with no existing test coverage of its own (test_client.py, already in the repo, is a manual live-pod script, not pytest tests -- confirmed it defines no `def test_*` functions, so pytest's recursive discovery only emits a benign collection warning about it, nothing runs). SANDBOX_WORKSPACE is redirected to a temp dir before import, since WORKSPACE is created on the real filesystem at import time and defaults to /workspace. Covers: health/root probes, _safe_path's traversal guard (relative "../", absolute-outside-workspace, both rejected; absolute-inside- workspace allowed), /init's env merge and timestamp-skew calculation, /exec's args-mode (no shell) vs. shell-mode (args omitted) dispatch, env passthrough, nonexistent-binary -> 127, timeout -> -1 with a stderr note, cwd validation (missing -> 400, traversal -> 403), and /files upload+download round-tripping including nested directory creation. Added httpx to requirements.txt: fastapi.testclient.TestClient needs it and the example didn't previously depend on it for anything. Verified passing locally, from a from-scratch venv install via the updated requirements.txt (22/22).
…me-sandbox Registers two nightly periodics that reuse the existing e2e-in-kind presubmit scripts (dev/ci/presubmits/test-e2e-gemini-cu-sandbox, test-e2e-python-runtime-sandbox) on a 24h schedule instead of only on file-change presubmit, to catch drift from changes elsewhere in the repo (controller/CRD, base images, SDK) that a run_if_changed filter on the example's own directory would miss. Same shape as periodic-agent-sandbox-migration-test: runner.sh + privileged + dind/kind-volume-mounts, extra_refs pinned to base_ref main since periodics have no PR context. gemini-cu-sandbox reuses the gemini-api-key secretKeyRef wiring already added for the presubmit twin in kubernetes#37536. No new scripts needed -- both presubmit scripts are self-contained with no PR-specific assumptions, so pointing a scheduled job at the same path guarantees the periodic tests exactly what the presubmit tests. Depends on kubernetes-sigs/agent-sandbox#1273: the referenced script paths don't exist on agent-sandbox main until that PR merges. Same merge-order constraint as kubernetes#37536.
What this PR does / why we need it:
Adds unit-test coverage and matching presubmit CI scripts for five examples that previously had none, and fixes a bug that made the existing gemini-cu-sandbox e2e presubmit fail unconditionally.
test_main.pyunit tests for the FastAPI runtime, run as a step indev/ci/presubmits/test-gemini-cu-sandboxbefore the kind/e2e flow.test_main.pycovering the FastAPI endpoints and theget_safe_pathtraversal guard, a Go port of the e2e tester (tester.go), and a newdev/ci/presubmits/test-python-runtime-sandboxpresubmit.test_coding_agent.pycoveringCodeGenerationLLM._clean_code,CodingAgent.should_continue, the LangGraph node functions, andLocalCodeExecutor's subprocess/env-var allowlisting, plus a unit-tests-only presubmit.pkg/tools(RunCommand,ReadFileTool,WriteFileTool,ListFilesTool,Registry), and fixes a missing--loadflag on the python-runtime-sandbox docker build that could leave the image invisible tokind load docker-imageunder this CI environment's buildx config.dev/ci/presubmits/test-gemini-cu-sandboxranpip install -e . --break-system-packageswithcwdset to the repo root, but the onlypyproject.tomlfor that package lives inclients/python/agentic-sandbox-client/. Running pip from the repo root fails immediately with "does not appear to be a Python project," so this presubmit could never pass as written. Fixed by pointingcwdat the client directory.This PR intentionally covers presubmit-only unit-test jobs. A periodic nightly rerun of these suites (with automatic GitHub issue filing on failure) is tracked separately.
Summary by CodeRabbit
Tests
Chores