News

已认领本任务

**Low test coverage:** This file covers only 5 basic edge cases. There are zero tests for `network_policy.py` — the most security-critical module (default-deny egress, credential vault). Consider adding at least import/structural tests for `network_policy.py` and a CI step to run these tests. Same pattern across all three integration directories.

**Naming inconsistency:** This file calls its renderer `jsonl_render_writer()` / `_render_jsonl_line()` / `_render_message()`, while the Claude-code and OpenCode `_common.py` equivalents are named `stream_json_render_writer()` / `_render_stream_json_line()`. The core `_BufferedJSONWriter` buffering logic is identical across all three. Adopting consistent naming across all integrations would make the shared-vs-divergent code structure clearer, especially if a shared module is introduced.

**Redundant package:** `bash` is already the default shell in the Debian-based base image. Including it in the apt install list is a no-op. Same pattern in all three Dockerfiles (`codebuddy`, `opencode`).

**Version constraint vs docs mismatch:** This pins `cubesandbox==0.3.0`, but the integration guide (`docs/guide/integrations/claude-code.md`) states the CubeEgress credential vault feature requires platform `>= 0.4.0`. The `network_policy.py` scripts import `Rule`, `Match`, `Action`, `Inject` from `cubesandbox` — if the Python SDK version 0.3.0 does not export these symbols, those scripts will fail at import time. Either bump to `cubesandbox>=0.4.0`, or verify that 0.3.0 supports these APIs and align the doc to match.

**Code duplication concern:** This `_common.py` is ~80% identical to `examples/codebuddy-integration/_common.py` and `examples/opencode-integration/_common.py`. The `_BufferedJSONWriter` class and the `run_command`, `ensure_success`, `sandbox_identifier`, and `stream_writer` functions are near-verbatim copies across all three integrations. Consider extracting these shared helpers into an `examples/_shared/` directory to avoid maintaining three copies. Every bug fix or improvement to this file must currently be applied to all three locations.

Could a human maintainer please clarify the stable expected behavior before further automated-review changes are requested? The recent automated reviews contain directly conflicting or factually incorrect findings that cause the implementation to oscillate between commits: 1. **npm lifecycle scripts** - An earlier review warned that `--ignore-scripts` could break CLI/native dependency installation and asked us to remove it. - A later review asked us to add `--ignore-scripts` back to the same three Dockerfiles. 2. **OpenCode server bind address** - An earlier review asked us to change the helper default from `0.0.0.0` to the safer `127.0.0.1` default. - The latest review now asks us to change it back to `0.0.0.0` for host port forwarding. - The current server-mode documentation already passes `--hostname 0.0.0.0` explicitly, while the reusable helper retains a secure loopback default. This supports both use cases without making every invocation externally reachable by default. 3. **Incorrect unused-import finding** - The review reported `provider_key_name` as unused in `examples/codebuddy-integration/network_policy.py`, but the same file uses it in `key_name = provider_key_name(provider)`. The non-conflicting `OPENAI_API_KEY` `.env.example` finding was valid and has been fixed in commit `3514015`. Other valid findings have also been addressed with regression tests. Repeatedly switching the same code back and forth based on contradictory automated comments is not sustainable and increases regression risk. Until a maintainer confirms otherwise, I propose keeping lifecycle scripts enabled for functional CLI installation, retaining `127.0.0.1` as the reusable helper's secure default, and using explicit `0.0.0.0` only in the documented server/port-forwarding flow. Could a maintainer please confirm these two policies and treat the contradictory/incorrect automated findings accordingly?

Agreed this is a real UX gap for native gRPC clients: routing failures currently reuse the HTTP JSON helpers (`respond_not_found` / etc.), which aren't valid gRPC status frames. I'd treat proper `grpc-status` mapping as a follow-up rather than expand this ingress PR — same Lua phases are shared with HTTP today. Open to doing it next if you want that tracked.

Reproduced on our base image `openresty/1.21.4.1`: Reproduced on our base image `openresty/1.21.4.1`: `strings` on the nginx binary shows `grpc_buffer_size` but not `grpc_buffering`. nginx 1.13.10 introduced the gRPC *module* (`grpc_pass`), not a `grpc_buffering` directive — that name isn't in the OSS `ngx_http_grpc_module` command table we checked (1.21.4 / 1.25.1 / master). The module hardcodes `upstream.buffering = 0`. So we cannot add the directive without breaking CubeProxy startup. Happy to walk through any specific streaming failure if you see one in practice.

Good catch — fixed. `local.cubeproxy_nginx_conf` now replaces `__CUBE_PROXY_GRPC_PORT__` → `9090`, and `create.sh`'s template generator also emits/checks that token (aligned with the release-bundle sed). Force-pushed in 8adaa7a.

**Error responses from Lua phase handler won't be valid gRPC frames** This location shares `rewrite_by_lua_file` and `header_filter_by_lua_file` with the HTTP/HTTPS blocks. When the Lua code encounters a routing failure (malformed `:authority`, sandbox not found, etc.), it calls `utils:respond_bad_request()` / `respond_not_found()` / `respond_unavailable()`, which send a `Content-Type: application/json` body. Over this HTTP/2 connection (opened by a gRPC client), the client expects `application/grpc` framed responses with gRPC status codes. A raw JSON body will produce a cryptic `RpcError`. Suggestion: check the listener context (e.g., `ngx.var.server_port == 9090` or a variable flag) and format routing errors as proper gRPC status responses (`grpc-status: 5 (NOT_FOUND)`, etc.) on this block.

**Potential issue: missing `grpc_buffering off` — streaming gRPC responses may be fully buffered** The PR description states `grpc_buffering off` was "addressed from #679 review feedback," but this directive does not appear. The author's response in the existing review claims it "is not available in our OpenResty 1.21.4 build" and that "the gRPC module already hardcodes `upstream.buffering = 0`." For reference: `grpc_buffering` was introduced in nginx 1.13.10, and OpenResty 1.21.4.1 ships the standard `ngx_http_grpc_module`. Without `grpc_buffering off`, nginx's default is `on`, which means the entire gRPC response body is buffered before sending — breaking server-streaming and bidirectional-streaming RPCs. The existing HTTP blocks handle this via dedicated `proxy_buffering off` includes. If the `ngx_http_grpc_module` truly doesn't support `grpc_buffering`, that's a significant upstream deviation. Could you share the actual error when adding the directive?

**Missing `__CUBE_PROXY_GRPC_PORT__` substitution in Terraform replace chain** The `local.cubeproxy_nginx_conf` replace chain handles `__CUBE_PROXY_HTTP_PORT__`, `__CUBE_PROXY_HTTPS_PORT__`, `__CUBE_PROXY_SSL_CERT__`, `__CUBE_PROXY_SSL_KEY__`, and `__CUBE_PROXY_ADMIN_LISTEN__:8082`, but there is no `replace()` call for `__CUBE_PROXY_GRPC_PORT__`. The bundle's `cubeproxy-nginx.conf` is generated by `generate_cube_proxy_nginx_template()`, which applies a sed rule to replace `9090` with `__CUBE_PROXY_GRPC_PORT__`. So the file placed in the Terraform directory contains the literal token. Since the Terraform replace chain doesn't resolve this token, the rendered ConfigMap will contain: ``` listen __CUBE_PROXY_GRPC_PORT__ http2 reuseport; ``` which is not a valid nginx listen directive — **nginx will fail to start in TKE deployments.** A `replace()` call for `__CUBE_PROXY_GRPC_PORT__` → `"9090"` needs to wrap the existing outermost replace.

Noted. HTTP has `proxy_next_upstream` at the http level; those directives don't apply to `grpc_pass`. For this path the Lua balancer already picks the peer via `balancer.set_current_peer()`. Transient connect failures are rare after that, and gRPC clients typically retry themselves. Skipping `grpc_next_upstream*` for now to keep the gRPC location minimal; we can add them later if we see real need in production.

Intentionally omitted — @chenhengqi asked to drop `$host_proxy_port` from the gRPC server block, and nothing in the Lua phases / access log currently reads it on that path. Re-adding it only for “future consistency” would undo that review feedback. If we later start consuming `$host_proxy_port`, we can set it in the gRPC block then.

Thanks — updated the Option B ufw comment for port 9090 to call out that it is plaintext and should be restricted to trusted networks when opened publicly (EN/ZH). Force-pushed in 78db4a0.

Good catch on the broken-symlink edge case. This helper is only used by the minimal example to load `.env` from the script dir or cwd; for the common paths we care about, `path` and `path.resolve()` agree. Happy to tighten it in a follow-up if we expand the example; leaving as-is for now to keep the demo tiny.

Thanks — this is intentional and mirrors how ports 80/443 are gated by `enable_public_network`. Port 9090 exists for clients that cannot use TLS / wildcard SNI DNS. Operators who enable public network already accept exposing CubeProxy ingress; we document the plaintext risk in the gRPC example READMEs (trusted networks / TLS terminator in front). A separate `enable_public_grpc` knob would be a follow-up if operators need asymmetric exposure; keeping the same boolean for this PR to avoid extra deploy surface.

**Missing `grpc_next_upstream` / `grpc_next_upstream_tries` directives** The HTTP `proxy_pass` path is covered by `proxy_next_upstream` and `proxy_next_upstream_tries` at the http block level (lines 88-89). However, nginx's documentation states these only apply to `proxy_pass`, not to `grpc_pass`. The gRPC equivalents — `grpc_next_upstream` and `grpc_next_upstream_tries` — are not configured. Impact is low since the Lua balancer (`balancer_phase.lua`) handles dynamic backend selection and has already committed to a route by this point. But a transient connection failure *before* the upstream processes the request (e.g., connection refused during `grpc_pass`) won't be retried. **Suggested fix:** Add `grpc_next_upstream error timeout;` and `grpc_next_upstream_tries 2;` here, or add a comment explaining why gRPC intentionally omits them (e.g., because the Lua balancer or gRPC client is expected to handle retries).

**Latent inconsistency: `$host_proxy_port` is not declared in the gRPC server block** The 8081 and 8080 server blocks both set `$host_proxy_port 8081;` and `$host_proxy_port 8080;` in every location. The gRPC block omits this variable. No Lua code currently reads `$host_proxy_port`, so this is not a runtime bug today, but it is a latent inconsistency — if any phase script (rewrite, header filter, log) or `global.conf` were extended to use it, requests through the gRPC port would see an empty string with potentially silent misbehavior. **Suggested fix:** Add `set $host_proxy_port 9090;` alongside the other `set` directives in the 9090 server block to keep the variable contract consistent across all three ingress ports.

**Inconsistency: port 9090 ufw rule lacks a source-restriction qualifier** Port 80's ufw line above reads "cube-proxy (public if needed)", but this new 9090 line just says "cube-proxy plaintext gRPC". Since 9090 carries no TLS (unlike 443), operators applying "Option B" verbatim would open plaintext gRPC to all sources without an explicit signal that this port is different from TLS-protected 443. **Suggested fix:** Add "(public if needed) (plaintext — restrict to trusted networks)" or similar qualifier to the comment, matching the pattern on port 80.

**Minor path dedup correctness issue** The dedup key is the resolved path (`resolved_path = path.resolve()`), but the existence check uses the unresolved path (`path.is_file()`). If `Path(__file__).with_name(".env")` is a broken symlink, `path.is_file()` returns `False`, so the function skips it — but `resolved_path` is already in `seen_paths`. On the next iteration, if `Path.cwd() / ".env"` is a loadable file that resolves to the same physical path, it is also skipped because the resolved path was already added to the set. The result: a loadable `.env` is silently not loaded. **Suggested fix:** Change `path.is_file()` to `resolved_path.is_file()` so the existence check and dedup key are consistent.

**Security concern: plaintext gRPC exposed to `0.0.0.0/0` under `enable_public_network`** This rule opens plaintext gRPC (no TLS, no transport-layer security) to the entire internet with the same `enable_public_network` boolean that controls TLS-protected ports 80/443. Operators enabling public network access for legitimate HTTPS sandbox traffic may not realize this also opens unencrypted gRPC to `0.0.0.0/0`. Consider one of: 1. Always scope port 9090 to `10.0.0.0/16` regardless of `enable_public_network`, with a separate opt-in variable. 2. Or add a dedicated `var.enable_public_grpc` variable (default: `false`) gating this port's public exposure, with a prominent warning in `variables.tf` documenting the plaintext risk. 3. At minimum, document in the Terraform deployment guide that `enable_public_network` also opens plaintext gRPC.

Small inconsistency here — worth a quick fix. On the write path you already keep snapshots out of alias ownership: only templates can claim/steal an alias, and a snapshot's `display_name` is just a free-form label even if it collides. That part is clear. But `GetTemplateByAlias` on the read path looks incomplete: it filters by `display_name` only, with no `kind = template`. Combined with `First()` and no ordering, you get whichever row the DB happens to return first. So if a snapshot happens to share the same `display_name` as a template alias, resolving by alias (sandbox create / template lookup) can hit the snapshot instead of the template that actually owns the alias. Writes are guarded; reads aren't. Suggest adding `kind = template` to the query, plus a unit test: when a template and a snapshot share the same name, resolution only returns the template.

> ## Review: PR #837 — Plaintext gRPC Ingress on Port 9090 > Overall this is a well-structured PR. The core implementation (nginx config, Terraform plumbing, shell-script readiness probes, test coverage, and documentation) is thorough and consistent. I have a few findings, including **one correctness bug** that should be addressed before merging. > > ### 1. (Critical) Missing grpc_buffering off in nginx config > **File:** CubeProxy/nginx.conf lines 399-416 The PR description says "grpc_buffering off for streaming RPCs" was addressed from the #679 review, but this directive does not appear in the config. nginx grpc_buffering defaults to on, buffering the entire response body before sending. Unary RPCs work fine but server-streaming/bidirectional streaming RPCs break - the client receives zero data until the stream completes. **Fix:** Add grpc_buffering off inside the location / block before grpc_pass. > > ### 2. (High) Docs say 403, code returns 404 for token violations > **Files:** docs/guide/restrict-public-access.md and docs/zh/guide/restrict-public-access.md The error-model table says traffic token violations return 403, but sandbox_backend.lua calls utils:respond_not_found() which returns 404 (deliberately, to avoid leaking sandbox existence). **Fix:** Update both docs to say 404 with a footnote explaining the deliberate choice. > > ### 3. (Medium) Public plaintext gRPC with no TLS alternative > **File:** deploy/one-click/terraform/tencentcloud/main.tf lines 389-395 When enable_public_network=true, TCP/9090 is open to 0.0.0.0/0 with no encryption. Unlike port 80 (which has 443/HTTPS), port 9090 has no TLS alternative - all gRPC frames including traffic access tokens are in cleartext. **Consideration:** Add a TLS-enabled gRPC port or document that this port is for trusted networks only. > > ### 4. (Medium) Missing security warning in example README > **File:** examples/grpc-ingress/README.md Describes plaintext gRPC but has no warning about using grpc.insecure_channel() over public networks. **Suggestion:** Add a note that port 9090 uses plaintext gRPC and should only be used over trusted networks. > > ### 5. (Low) Auto-pause gating on gRPC port not documented > **Files:** docs/guide/https-and-domain.md and docs/zh/guide/https-and-domain.md The gRPC block runs rewrite_phase.lua which invokes state.gate() - auto-pause/resume lifecycle applies but is not mentioned. **Suggestion:** Document that lifecycle gating applies transparently. > > ### 6. (Low) Missing test: custom gRPC port with default HTTP port > **File:** deploy/one-click/tests/test_runtime_file_safety.sh Test matrix covers default+default, custom HTTP+default gRPC, and custom HTTP+custom gRPC but not default HTTP+custom gRPC. > > Summary: Findings 1 and 2 are the most actionable - one is a correctness bug for streaming RPCs, the other is a documentation inaccuracy that would mislead operators debugging access-control issues. Thanks for the auto-review — addressed the actionable items in 6069af4. 1. `grpc_buffering off` — not added back. That directive is not available in our OpenResty 1.21.4 (nginx 1.21.4) build and would fail config load. The gRPC module already hardcodes `upstream.buffering = 0`; buffering is not on by default for `grpc_pass`. 2. 403 vs 404 — fixed both `restrict-public-access` docs (EN/ZH) to match `sandbox_backend.lua`: token failures return **404** by design (same as unknown sandbox, to avoid existence leaks). 3. Public plaintext 9090 — by design for clients that cannot use TLS/SNI wildcard DNS. Clarified in the gRPC example READMEs that this port is plaintext and should only be used over trusted networks (or behind a TLS terminator). 4. Example security warning — added. 5–6 (auto-pause docs / extra test matrix) — leaving for a follow-up unless you'd like them in this PR.

<!-- cubesandbox-auto-review:start --> ## Review: PR #837 — Plaintext gRPC Ingress on Port 9090 Overall this is a well-structured PR. The core implementation (nginx config, Terraform plumbing, shell-script readiness probes, test coverage, and documentation) is thorough and consistent. I have a few findings, including **one correctness bug** that should be addressed before merging. ### 1. (Critical) Missing grpc_buffering off in nginx config **File:** CubeProxy/nginx.conf lines 399-416 The PR description says "grpc_buffering off for streaming RPCs" was addressed from the #679 review, but this directive does not appear in the config. nginx grpc_buffering defaults to on, buffering the entire response body before sending. Unary RPCs work fine but server-streaming/bidirectional streaming RPCs break - the client receives zero data until the stream completes. **Fix:** Add grpc_buffering off inside the location / block before grpc_pass. ### 2. (High) Docs say 403, code returns 404 for token violations **Files:** docs/guide/restrict-public-access.md and docs/zh/guide/restrict-public-access.md The error-model table says traffic token violations return 403, but sandbox_backend.lua calls utils:respond_not_found() which returns 404 (deliberately, to avoid leaking sandbox existence). **Fix:** Update both docs to say 404 with a footnote explaining the deliberate choice. ### 3. (Medium) Public plaintext gRPC with no TLS alternative **File:** deploy/one-click/terraform/tencentcloud/main.tf lines 389-395 When enable_public_network=true, TCP/9090 is open to 0.0.0.0/0 with no encryption. Unlike port 80 (which has 443/HTTPS), port 9090 has no TLS alternative - all gRPC frames including traffic access tokens are in cleartext. **Consideration:** Add a TLS-enabled gRPC port or document that this port is for trusted networks only. ### 4. (Medium) Missing security warning in example README **File:** examples/grpc-ingress/README.md Describes plaintext gRPC but has no warning about using grpc.insecure_channel() over public networks. **Suggestion:** Add a note that port 9090 uses plaintext gRPC and should only be used over trusted networks. ### 5. (Low) Auto-pause gating on gRPC port not documented **Files:** docs/guide/https-and-domain.md and docs/zh/guide/https-and-domain.md The gRPC block runs rewrite_phase.lua which invokes state.gate() - auto-pause/resume lifecycle applies but is not mentioned. **Suggestion:** Document that lifecycle gating applies transparently. ### 6. (Low) Missing test: custom gRPC port with default HTTP port **File:** deploy/one-click/tests/test_runtime_file_safety.sh Test matrix covers default+default, custom HTTP+default gRPC, and custom HTTP+custom gRPC but not default HTTP+custom gRPC. Summary: Findings 1 and 2 are the most actionable - one is a correctness bug for streaming RPCs, the other is a documentation inaccuracy that would mislead operators debugging access-control issues. <!-- cubesandbox-auto-review:end -->

**Version mismatch with PR title** PR title says "v0.5.0" but this chart declares version 0.5.1 and appVersion 0.5.1. Align the PR title with the chart version.

**Test coverage gap: only happy path covered** This test exercises the case where all three env vars are set. Missing test cases: 1. `CUBE_SANDBOX_NODE_ID` set to non-IP string ("node-b") + no `CUBE_SANDBOX_ENDPOINT_IP` → should fall through to `detectPrimaryIPv4()` for localIPv4 2. Invalid `CUBE_SANDBOX_ENDPOINT_IP` (loopback 127.0.0.1, or "not-an-ip") → should error 3. Both env vars empty + `detectPrimaryIPv4()` fails → error path

**No resource requests/limits on master Pod** `resources: {}` means the scheduler has zero reservation data. Under memory pressure, master could be OOM-killed. Provide a conservative default (e.g., `requests: {cpu: 500m, memory: 512Mi}`). Same issue exists for api (line 262), webui (308), lifecycle-manager (352), mysql (391), redis (416), and proxy (715).

**Mandatory 5-minute wait delays data-plane readiness** `INITIAL_WAIT_SECONDS=300` means the egress-net sidecar spins in a sleep loop for 5 minutes before applying iptables rules. Combined with the readiness probe (failureThreshold: 30 × periodSeconds: 10 = 300s cumulative), the Pod is not Ready for egress for at least 5 minutes on cold start. Reduce this to 60s and let the readiness probe handle slower cases.

**Sequential sentinel waits create 20-minute startup bottleneck** These four `wait_sentinel` calls run **sequentially**, each with a 300-second timeout. If the installer DaemonSet is slow pulling any image, cubelet is blocked for up to 20 minutes before starting. Consider polling all four sentinel files concurrently within a single 300-second budget, or replacing with `inotifywait`-based watches.

**Minor:** `MiddlewareLogging` no longer exists in the codebase — it was removed during the migration. A reader unfamiliar with the codebase won't know what this refers to. Consider either removing the reference or adding a brief explanation (e.g., "the original mux logging middleware, now consolidated here").

**Suggestion:** The error from `FastestJsoniter.Marshal(data)` is silently discarded (`_`). Since the function writes HTTP 200 + `Content-Type: application/json` *before* marshaling (lines 21-22), a serialization failure produces a 200 OK with an empty body and no diagnostic. Log the error and write a fallback error envelope so callers get a meaningful response.

<!-- cubesandbox-auto-review:start --> ## PR #922 Code Review: gorilla/mux → gin-gonic/gin Migration ### Overall Assessment This is a well-executed router migration. The architecture is sound: middleware is correctly mounted on a root group so NoRoute/NoMethod stay bare (preserving mux parity), handlers use `c.Param()`/`c.Query()` consistently, response writing goes through `common.WriteAPI` (FastestJsoniter), and the route table matches previous observable behavior. The test suite covers the contract-critical aspects. `go build ./...`, `go vet`, and `gofmt` are clean. ### Noteworthy Issues #### 1. `meta.writeErr` bypasses gin with inconsistent status codes (Medium) **File:** `CubeMaster/pkg/service/httpservice/meta/meta.go:228` `writeErr` takes a raw `http.ResponseWriter` and callers pass inconsistent HTTP status codes — `registerNodeGinHandler` uses HTTP 400 (line 104) while other error paths use HTTP 200. Replace with `common.WriteErr(c, code, msg)`. #### 2. Triple JSON deep-copy in `previewSandbox` (Medium) **File:** `CubeMaster/pkg/service/httpservice/cube/sandbox_preview.go:58/71/98` `cloneCreateReq` does a full `jsoniter.Marshal`+`Unmarshal` round-trip 3 times per request. Use a hand-written deep-copy instead. #### 3. Silent marshal error discard (Low-Medium) **File:** `CubeMaster/pkg/service/httpservice/common/res.go:23` `FastestJsoniter.Marshal(data)` error discarded. A serialization failure produces 200 OK with empty body. #### 4. Stale `MiddlewareLogging` reference (Low) **File:** `CubeMaster/pkg/service/httpservice/middleware/gin_middleware.go:28` References removed `MiddlewareLogging` function. #### 5. Duplicate FastestJsoniter frozen instances (Low) **Files:** `common/res.go:114-119` and `types/types.go:682-687` Two independent `.Froze()` calls doubling the type-cache. Deduplicate. ### Test Coverage Gaps - **`deleteSnapshot` (`snapshot.go:429`):** Untested — `c.Param`, content-length branching, sync job execution (High) - **`handleTemplateBuildStatusAction` (`template_commit.go:175`):** Untested — status mapping logic (High) - **`createTemplate` (`template.go:146`):** Untested — error-code mapping, 9-field response (Medium-High) - **`snapshotErrorCode` (`snapshot.go:519`):** Only MySQL lock errors tested (Medium) ### Verdict Sound migration structurally. Address the meta `writeErr` inconsistency and test coverage gaps. Pre-existing issues (error leakage, path param validation, no nonce dedup) should be filed separately. <!-- cubesandbox-auto-review:end -->

已认领本任务：Nix + k3s 沙箱模板 计划分两个 PR 渐进式贡献： 1. k3s-sandbox — 一次性 Kubernetes 集群沙箱，从 Nix 构建的 OCI 镜像创建模板，演示 snapshot/restore 全流程（部署应用 → 快照 → 销毁 → 从快照恢复 → 应用仍在运行），展示 CubeSandbox 对多服务有状态工作负载的差异能力。 2. nix-dev-env + k3s deploy target — 在 1 的基础上叠加可复现开发环境，用 Nix shell 提供工具链、k3s 作为本地部署目标，构建-部署-快照一体化演示。

**Test gap: `CUBE_SANDBOX_DEP_HEALTH_RETRIES/DELAY` knobs have no unit-level coverage** The new health-wait knobs are only exercised incidentally in `test_postcheck_skips_when_external_host_set` as a means to an end. No test directly verifies that `wait_for_container_health` reads and applies these parameters correctly with a stubbed `docker`. The function also lacks stubbed tests for edge cases (invalid values, non-numeric strings, zero/negative values).

**Consider cross-referencing the Python-side duplicate mapping** The Python upgrade merge script in `lib/common.sh` (lines 1204-1223 in the embedded heredoc) has its own `EXTERNAL_ALIAS_MAP` that duplicates the mapping defined here. These must stay in sync, but there's no cross-reference comment on either side pointing at the other. If a new legacy key is added in the future, only one side might be updated. Suggestion: add a comment to the Python `EXTERNAL_ALIAS_MAP` referencing this shell‑side array as the canonical definition.

**Bug: `[:space:]` POSIX character class does not match whitespace inside a `case` (glob) pattern** In bash, `case` patterns use POSIX glob matching, not regex. The `[:space:]` character class is only interpreted by bracket expressions inside `[[ ... == pattern ]]` or `[[ ... =~ pattern ]]` contexts. Inside a `case` pattern's `[...]` bracket expression, `[:space:]` is treated as **literal characters** — matching `:`, `s`, `p`, `a`, `c`, `e` — not whitespace. This means a credential component containing a space, tab, or newline would **silently pass through** this guard and produce a corrupted DSN that could be exploited if injected into a `bash -c` command (CWE-78). Fix: use a `[[ ... =~ ... ]]` regex test instead: ```bash if [[ "${_dsn_part}" =~ [@:/#%\"\`\$\'\\[[:space:]] ]]; then die "..." fi ``` Same issue exists in `cube-api-start.sh:46`.

**Bug: `[:space:]` POSIX character class does not match whitespace inside a `case` (glob) pattern** (duplicate of the same issue in up.sh:77) Same pattern as `up.sh` — `[:space:]` inside a `case` pattern is treated as literal characters `:`, `s`, `p`, `a`, `c`, `e`, not whitespace. Whitespace in credentials would silently bypass this guard and produce a corrupted DSN. Fix: use `[[ ... =~ ... ]]` regex: ```bash if [[ "${_dsn_part}" =~ [@:/#%\"\`\$\'\\[[:space:]] ]]; then die "..." fi ```

**Test gap: `CUBE_SANDBOX_DEP_HEALTH_RETRIES/DELAY` knobs have no unit-level coverage** The new health-wait knobs are only exercised incidentally in `test_postcheck_skips_when_external_host_set` as a means to an end. No stubbed-docker unit test directly verifies that `wait_for_container_health` reads and applies these parameters correctly for edge cases (invalid values, zero/negative, non-numeric strings).

**Consider cross-referencing the Python-side duplicate mapping** The Python upgrade merge script in `lib/common.sh` (lines 1204-1223 in the embedded heredoc) has its own `EXTERNAL_ALIAS_MAP` that duplicates the mapping defined here. These must stay in sync, but there's no cross-reference comment pointing at this shell-side array as the canonical definition. If a new legacy key is added in the future, only one side might be updated.

**Bug: `[:space:]` POSIX character class does not match whitespace inside a `case` (glob) pattern** (duplicate of the same issue in up.sh:77) Same pattern as `up.sh` — `[:space:]` inside a `case` pattern is treated as literal characters `:`, `s`, `p`, `a`, `c`, `e`, not whitespace. Whitespace in credentials would silently bypass this guard. Fix: use `[[ ... =~ ... ]]` regex: ```bash if [[ "${_dsn_part}" =~ [@:/#%\"\`\$\'\\[[:space:]] ]]; then die "..." fi ```

**Bug: `[:space:]` POSIX character class does not match whitespace inside a `case` (glob) pattern** In bash, `case` patterns use POSIX glob matching, not regex. The `[:space:]` character class is only interpreted by bracket expressions inside `[[ ... == pattern ]]` or `[[ ... =~ pattern ]]` contexts. Inside a `case` pattern's `[...]` bracket expression, `[:space:]` is treated as **literal characters** — matching `:`, `s`, `p`, `a`, `c`, `e` — not whitespace. This means a credential component containing a space, tab, or newline would **silently pass through** this guard and produce a corrupted DSN. Fix: use a `[[ ... =~ ... ]]` regex test instead: ```bash if [[ "${_dsn_part}" =~ [@:/#%\"\`\$\'\\[[:space:]] ]]; then die "..." fi ``` Same issue exists in `cube-api-start.sh:46`.

**Loopback classification semantic mismatch** The parenthetical `即不是 127.0.0.1/localhost/::1` defines an external host solely by being different from those three literals. This is narrower than the `_host_is_loopback()` implementation, which classifies the entire `127.0.0.0/8` range (any `127.x.y.z` with valid octets) as loopback — not just `127.0.0.1`. Lines 278-281 of the same document correctly note that `127.0.0.2` is a loopback address, but this line would lead a reader to believe `127.0.0.2` is external. ```suggestion 当某个依赖被判定为外部时——要么 `*_HOST` 指向非本机地址（即不在 `127.0.0.0/8`、`localhost`、`::1` 范围内），要么回环 host 配了 `*_MANAGED=0`——`install.sh` 会： ```

**Missing `mysql_port` in metacharacter validation loop** The degraded DATABASE_URL fallback checks `mysql_user`, `mysql_password`, `mysql_host`, and `mysql_db` for URL/shell metacharacters, but omits `mysql_port`. The port value is interpolated directly into the `export DATABASE_URL="mysql://...:${mysql_port}/..."` string which is later passed to `bash -c` via `start_with_pidfile`. A port with metacharacters (e.g. `"; rm -rf /; "`) could break out of the double-quote context. Consider adding `mysql_port` to the loop: ```bash for _dsn_part in "${mysql_user}" "${mysql_password}" "${mysql_host}" "${mysql_port}" "${mysql_db}"; do ``` Applies identically to `deploy/one-click/scripts/systemd/cube-api-start.sh` line 44.

**`remove_env_kv` prefix match silently misses indented keys & CRLF lines** The authoritative delete loop at line 651 uses `[[ "${line}" == "${key}="* ]]` to match. This requires the line to start exactly with the key — it won't match lines with leading whitespace (e.g., ` CUBE_EXTERNAL_MYSQL_PASSWORD=secret` from a hand-edited file). Additionally, `read -r` does not strip `\r`, so CRLF-formatted lines carry a trailing `\r`. The `*` glob DOES match through the `\r`, so CRLF values are properly deleted, but the surviving non-matching lines in the file still retain their `\r` suffix. Consider standardizing line ending handling across the env file read/write helpers. A minimal fix would strip `\r` in the read loop: ```bash while IFS= read -r line || [[ -n "${line}" ]]; do line="${line%%$'\r'}" # strip CR from CRLF ... done ```

**Missing boolean alias documentation for *_MANAGED** The Chinese README only documents `auto`, `1`, and `0` as valid *_MANAGED values. However, the implementation (`validation.sh` lines 84-101) also accepts `true`, `yes`, `on`, `false`, `no`, `off` (case-insensitive via `${managed,,}`). The English README correctly lists all accepted values. Please update the Chinese version for parity so a Chinese-reading user who sets `CUBE_SANDBOX_MYSQL_MANAGED=false` knows it's supported.

**8 sequential atomic rewrites for legacy key cleanup** The loop at line 1029 calls `remove_env_kv` once per legacy key, each doing a full `grep` + `mktemp` + read-rewrite + `mv` cycle on the same target file. That's 8 separate atomic replacements. On a low-end VM with burstable IOPS or a heavily loaded filesystem this multiplies the write cost significantly. Consider a single-pass approach: collect all legacy keys to remove in one go, then do one atomic rewrite. Even a single loop inline that builds a combined awk/sed filter would be more efficient than 8 sequential rewrites of the same file.

@novahe The integration tests have all failed: <details open> <summary>Integration test output: </summary> ``` running 2 tests test common_parallel::test_virtio_vsock_passthrough_fd ... FAILED test common_parallel::test_virtio_vsock_passthrough_fd_hotplug ... FAILED failures: ---- common_parallel::test_virtio_vsock_passthrough_fd stdout ---- ==== Start cube-hypervisor command-line ==== "target/x86_64-unknown-linux-gnu/release/cube-hypervisor" "--api-socket" "/tmp/chHLQ5Se/cloud-hypervisor.sock" "--cpus" "boot=1" "--memory" "size=512M" "--kernel" "/root/workloads/vmlinux" "--cmdline" "root=/dev/vda1 console=hvc0 rw systemd.journald.forward_to_console=1" "--disk" "path=/tmp/chHLQ5Se/osdisk.img" "path=/tmp/chHLQ5Se/cloudinit" "--net" "tap=,mac=12:34:56:78:90:01,ip=192.168.1.1,mask=255.255.255.0" "--vsock" "cid=3,socket=/tmp/chHLQ5Se/vsock" "-v" ==== End cube-hypervisor command-line ==== thread 'common_parallel::test_virtio_vsock_passthrough_fd' panicked at test_infra/src/lib.rs:1180:9: assertion `left == right` failed left: "" right: "GuestReply" stack backtrace: 0: rust_begin_unwind at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:647:5 1: core::panicking::panic_fmt at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:72:14 2: core::panicking::assert_failed_inner at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:342:17 3: core::panicking::assert_failed at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:297:5 4: test_infra::Guest::check_vsock_passthrough_fd_bidirectional at ./test_infra/src/lib.rs:1180:9 5: integration::_test_virtio_vsock_passthrough_fd::{{closure}} at ./tests/integration.rs:1717:9 6: std::panicking::try::do_call at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:554:40 7: __rust_try 8: std::panicking::try at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:518:19 9: std::panic::catch_unwind at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panic.rs:142:14 10: integration::_test_virtio_vsock_passthrough_fd at ./tests/integration.rs:1700:13 11: integration::common_parallel::test_virtio_vsock_passthrough_fd at ./tests/integration.rs:4675:9 12: integration::common_parallel::test_virtio_vsock_passthrough_fd::{{closure}} at ./tests/integration.rs:4674:42 13: core::ops::function::FnOnce::call_once at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/ops/function.rs:250:5 14: core::ops::function::FnOnce::call_once at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/ops/function.rs:250:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. ==== child killed by signal: 9 ==== thread 'common_parallel::test_virtio_vsock_passthrough_fd_hotplug' panicked at test_infra/src/lib.rs:1180:9: assertion `left == right` failed left: "" right: "GuestReply" stack backtrace: 0: rust_begin_unwind at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:647:5 1: core::panicking::panic_fmt at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:72:14 2: core::panicking::assert_failed_inner at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:342:17 3: core::panicking::assert_failed at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/panicking.rs:297:5 4: test_infra::Guest::check_vsock_passthrough_fd_bidirectional at ./test_infra/src/lib.rs:1180:9 5: integration::_test_virtio_vsock_passthrough_fd::{{closure}} at ./tests/integration.rs:1717:9 6: std::panicking::try::do_call at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:554:40 7: __rust_try 8: std::panicking::try at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panicking.rs:518:19 9: std::panic::catch_unwind at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/std/src/panic.rs:142:14 10: integration::_test_virtio_vsock_passthrough_fd at ./tests/integration.rs:1700:13 11: integration::common_parallel::test_virtio_vsock_passthrough_fd_hotplug at ./tests/integration.rs:4680:9 12: integration::common_parallel::test_virtio_vsock_passthrough_fd_hotplug::{{closure}} at ./tests/integration.rs:4679:50 13: core::ops::function::FnOnce::call_once at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/ops/function.rs:250:5 14: core::ops::function::FnOnce::call_once at /rustc/aedd173a2c086e558c2b66d3743b344f977621a7/library/core/src/ops/function.rs:250:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. ==== child killed by signal: 9 ==== ``` </details>

No, it's not architecture-specific. This 2 MiB deliberately mirrors the VMM's own hard-coded pmem check in hypervisor/vmm/src/device_manager.rs:2788: if size % 0x20_0000 != 0 { return Err(DeviceManagerError::PmemSizeNotAligned); } // The memory needs to be 2MiB aligned in order to support hugepages. 0x20_0000 is a fixed 2 MiB literal, not gated by any #[cfg(target_arch)], so it's the same value on x86_64 and aarch64. 2 MiB is the PMD-level huge-page size on x86_64 and on aarch64 with 4 KiB base pages — the configs this VMM's pmem/DAX path targets — which is why the VMM hard-codes it. By construction the Go side then produces exactly the size the VMM's check accepts, so it can't be under- or over-aligned relative to the VMM. If you'd prefer, I can add a short comment pointing at device_manager.rs:2788 as the source of truth so the two constants don't silently drift.

<!-- cubesandbox-auto-review:start --> ## Code Review: PR #833 — Change-Aware Unit Test Check Workflow ### Critical **1. CubeMaster and Cubelet are missing from the path-to-component mapping** (`.github/workflows/unit-test-check.yml`, lines 121–129) These are the two most heavily tested components (~100 and ~143 test files respectively). The root `Makefile` already defines `cubemaster-test` and `cubelet-test` targets, but the workflow can never trigger them through change detection. A PR touching only `CubeMaster/` or `Cubelet/` produces `has_work=false` and silently skips all tests. The only way these tests run is if the PR also happens to modify `Makefile`, `Dockerfile.builder`, or a workflow YAML. **Fix**: Add the missing case branches and corresponding matrix entries. This is a blocker for the workflow to function as a meaningful CI gate. --- ### High **2. Duplicated diff logic and redundant work across jobs** (lines 99–111 vs. 189–211, and lines 248–258) The `git diff` computation is duplicated verbatim between `detect-changes` and every `test` job. Combined with the builder image resolution: when `Dockerfile.builder` changes, every matrix job independently calls `docker build` in parallel, wasting N-1 builds. This adds unnecessary wall-clock time and creates a maintenance liability. **Fix**: Have `detect-changes` emit `builder_inputs_changed` and the resolved image tag as job outputs. Test jobs can then skip their own diff logic and use `fetch-depth: 1`. For builder-image changes, add a dedicated `build-builder` phase: `detect-changes → build-builder → test (matrix)`. **3. `CubeNet/*` mapped to `network-agent`** (line 127) `CubeNet/` is a superset containing multiple sub-projects (`CubeNet/cubevs/`, `CubeNet/vmlinux/`, etc.). Changes to unrelated parts of `CubeNet/` incorrectly trigger network-agent tests. **Fix**: Map only `network-agent/*` to the network-agent component, or add `CubeNet/*` as its own distinct component. --- ### Medium **4. Mutable builder image tag — supply chain risk** (line 47) `BUILDER_IMAGE_REMOTE` uses the mutable tag `:ubuntu2004`. Tags can be overwritten, meaning a compromised tag could execute arbitrary code in every CI run. Pin the image by digest and use Dependabot/Renovate for automated updates. **5. Script injection via `workflow_dispatch` input** (line 87) `${{ github.event.inputs.component }}` is interpolated directly into a bash script. While the UI restricts inputs, the API allows arbitrary values. A collaborator with dispatch permission could achieve code execution. Pass the expression through the `env:` mapping instead, which handles escaping more safely. --- ### Low / Observations **6. `CI=true && go test` non-standard pattern** (`CubeMaster/Makefile`, line 103) Use `CI=true go test ...` instead — the `&&` after a variable assignment is misleading. The conventional one-shot environment prefix is clearer. **7. No `timeout-minutes` on the test step** (line 262) A hung/deadlocked test would consume the full 360-minute default GitHub Actions timeout. Add `timeout-minutes: 30` to the test execution step. **8. Workflow-level `paths:` filter hides skipped tests** The `pull_request` `paths:` filter means untracked directories like `CubeMaster/` don't even trigger the workflow to run — no status check appears. Consider removing the workflow-level `paths:` filter and relying entirely on `detect-changes`, or adding a visible "skip" status check. **9. CubeMaster `test/conf.yaml` path doesn't exist** (`CubeMaster/Makefile`, line 102) The conditional `if [ -f "$(ROOT_DIR)/test/conf.yaml" ]` is always false — only `CubeMaster/conf.yaml` exists. Verify whether tests need config and fix the path, or remove the vestigial conditional. **10. Make target naming inconsistency** `CubeShim` maps to `shim-test` rather than `cubeshim-test`. While functionally correct, this is inconsistent with the component directory name and may cause confusion. --- ### Positive Highlights - **Well-scoped permissions**: Only `contents: read` and `packages: read` — minimal blast radius. - **Graceful fork handling**: `continue-on-error: true` on GHCR login with a local-build fallback. - **`fail-fast: false`**: One component's test failure doesn't cancel others. - **Robust error handling**: `set -euo pipefail` on every shell step; the zero-changes early exit is clean. - **Dedup in change detection**: The `seen` associative array correctly prevents duplicate components. - **`ROOT_DIR` over `pwd`** in CubeMaster/Makefile: Fixes a reproducibility bug inside Docker builder containers. <!-- cubesandbox-auto-review:end -->

**`CI=true && go test` is a non-standard pattern** The `&&` after `CI=true` is misleading — readers may wonder why there's a conditional on a variable assignment. The conventional pattern is to prepend the assignment directly: ```makefile CI=true go test -v -timeout=20m ${TEST_FLAGS} \ ``` This sets `CI=true` exclusively for the duration of `go test`, which is both clearer and more conventional.

**Redundant full clone + diff in every test job** The diff computation logic (resolving `base_ref`/`head_ref` + `git diff --name-only`) is duplicated verbatim between `detect-changes` (lines 99-111) and this `test` job step. The `detect-changes` job already computes which files changed. Have it emit `builder_inputs_changed` as a job output so test jobs can skip their own diff and use a shallower checkout (`fetch-depth: 1` instead of 0).

**Mutable Docker image tag — supply chain risk** The builder image is referenced by a mutable tag (`:ubuntu2004`). Tags can be overwritten. If the GHCR repository were compromised, every CI run that pulls this tag would execute attacker-controlled code with the workflow's `GITHUB_TOKEN` permissions. Pin the image by digest (e.g., `@sha256:abc123...`) and use Dependabot/Renovate to automate digest updates when a new trusted image is published.

**Duplicate builder image builds when builder inputs change** When `Dockerfile.builder` or `build-builder-image.yml` changes, every matrix job independently calls `build_local_builder_image()` (lines 248-258) in parallel. With three components selected, this wastes two of three Docker builds, adding potentially 5-10+ minutes of unnecessary wall-clock time. Consider restructuring into three phases: detect-changes → build-builder (conditional) → test (matrix, always pulls). The builder resolution already happening in `detect-changes` could also emit the image tag as a job output so test jobs don't need their own diff logic.

**Potential script injection via `workflow_dispatch` input** `${{ github.event.inputs.component }}` is interpolated directly into a bash script block. While the `workflow_dispatch` UI restricts inputs to the listed `options`, the Actions API allows direct dispatch with arbitrary values. A collaborator with dispatch permission could craft a payload containing `"` to break out of the string assignment and inject arbitrary bash commands. Pass the expression through the `env:` mapping instead, which handles escaping more safely: ```yaml - name: Select components env: COMPONENT_INPUT: ${{ github.event.inputs.component || 'all' }} run: | selected_component="${COMPONENT_INPUT}" ... ```

**`CubeNet/*` is too broad for the `network-agent` component** `CubeNet/` is a superset directory containing multiple sub-projects (`CubeNet/cubevs/`, `CubeNet/vmlinux/`, etc.). Changes to `CubeNet/vmlinux/Makefile` or `CubeNet/cubevs/` would incorrectly trigger network-agent tests even when the actual changes are unrelated. Either map only `network-agent/*` to network-agent, or treat `CubeNet/*` as its own distinct component if it has meaningful test infrastructure.

**CubeMaster and Cubelet are missing from the path-to-component mapping** These are the two most heavily tested components in the repository (~100 and ~143 test files respectively). The root `Makefile` defines `cubemaster-test` and `cubelet-test` targets (lines 253-259) but the workflow can never trigger them through change detection. A PR touching only `CubeMaster/` or `Cubelet/` produces `has_work=false` and silently skips all tests. The only way CubeMaster/Cubelet tests run is if the PR also happens to modify `Makefile`, `Dockerfile.builder`, or a workflow YAML. Add the missing case branches and corresponding matrix entries before this PR is considered a complete CI gate for the repository.

已认领本任务

已认领本任务

@chenhengqi Addressed review feedback — removed unused `$host_proxy_port` from the gRPC server block (b819f55). Kept `grpc_buffering off` for server-streaming RPCs and the dummy upstream `server` entry for `balancer_by_lua`. Happy to adjust if you see a cleaner approach.

The `server 0.0.0.1:1235` entry is a dummy placeholder required by the upstream block syntax. The real backend peer is chosen dynamically in `balancer_by_lua` via `balancer.set_current_peer()` — same pattern as the HTTP `backend` upstream (`0.0.0.1:1234`). Removing it would make the upstream block invalid.

Good catch — removed `grpc_buffering off`. That directive isn't available in our OpenResty 1.21.4 (nginx 1.21.4) build; the gRPC module already keeps buffering disabled by default (`upstream.buffering = 0`), so the line was both unnecessary and would fail config load. Note: the `process.Process/Start` locations use HTTP `proxy_pass` with `proxy_buffering off`, not the gRPC path — different module. Force-pushed in bbcccba.

Thanks! Removed `$host_proxy_port` from the gRPC server block — it isn't consumed on that path. Force-pushed in b819f55.

**Nit: bare type assertion** — `muV.(*sync.Mutex)` could panic if the stored type ever changes. The existing codebase (`cache.go`) uses the two-value form `lock, _ := actual.(*sync.RWMutex)`. Consider using it here too for consistency, even though only `*sync.Mutex` values are ever stored.

**TrimSpace validates but doesn't sanitize** — The guard on line 154 calls `strings.TrimSpace` to check emptiness but the raw (untrimmed) values flow into the cubelet annotations on lines 171-173. `Ext4SHA256` or `DownloadToken` with leading/trailing whitespace would pass the guard but arrive corrupted at cubelet. Consider trimming the values before use, or rejecting values that differ from their trimmed form.

the `up-with-deps.sh` is deprecated, we use systemd to manage services instead. You can adjust the dev-env's scripts, just treat it as a VM, and use scp to copy files to it.

**resolveHostIdentity() has ~7 distinct code paths but only 1 is tested** — The function has 3 paths for `instanceID` and 3 for `localIPv4`, producing a decision tree of at least 7 code paths. The PR only adds `TestGetHostIdentitySeparatesNodeIDAndEndpointIP`, which exercises exactly one combination with all three env vars set to valid values. The error return for invalid `CUBE_SANDBOX_ENDPOINT_IP`, the double-`detectPrimaryIPv4` fallback path, and the non-IP `instanceID` → `detectPrimaryIPv4` fallback are all untested. **Suggestion:** Add a table-driven test for `resolveHostIdentity()` directly (not through `GetHostIdentity` with its `sync.Once` cache) covering invalid/missing/whitespace env values.

**⚠ Supply chain risk: SHA256 verification disabled by default** `ONE_CLICK_SHA256`, `PVM_KERNEL_RPM_SHA256`, and `PVM_KERNEL_DEB_SHA256` all default to empty strings (lines 68-70). When empty, `validate_download` (line 101) skips the integrity check, making the build silently accept tampered artifacts from SourceForge, GitHub releases, or the cnb.cool mirror. Since the extracted binaries run in privileged containers and the kernel package executes with ring 0, this is a supply-chain vulnerability. **Suggestion:** Either require SHA256 to be non-empty by default (fail the build), or at minimum emit a prominent warning when verification is skipped. The comment at line 66 says operators "should always set these" but provides no mechanism to fail closed.

**resolveHostIdentity() has ~7 distinct code paths but only 1 is tested** — The function has 3 paths for `instanceID` and 3 for `localIPv4`, producing a decision tree of at least 7 code paths. The PR only adds `TestGetHostIdentitySeparatesNodeIDAndEndpointIP`, which exercises exactly one combination with all three env vars set to valid values. The error return for invalid `CUBE_SANDBOX_ENDPOINT_IP`, the double-`detectPrimaryIPv4` fallback path, and the non-IP `instanceID` → `detectPrimaryIPv4` fallback are all untested. **Suggestion:** Add a table-driven test for `resolveHostIdentity()` directly covering invalid/missing/whitespace env values.

**CUBE_SANDBOX_NODE_ID accepted with no validation** — The env var value is only whitespace-trimmed; no length, character set, or format check is applied. This value becomes the node's InstanceID and likely flows into MySQL, REST API paths, and logs. While the Helm chart populates it from `spec.nodeName` (safe), downstream callers setting the env var directly could inject crafted values. **Suggestion:** Validate against an alphanumeric-dots-hyphens pattern (max 253 chars) and reject control characters.

**⚠ Supply chain risk: SHA256 verification disabled by default** `ONE_CLICK_SHA256`, `PVM_KERNEL_RPM_SHA256`, and `PVM_KERNEL_DEB_SHA256` all default to empty strings (lines 68-70). When empty, `validate_download` (line 101) skips integrity checking entirely, so tampered artifacts from SourceForge, GitHub releases, or the cnb.cool mirror are accepted silently. Since extracted binaries run in privileged containers and the kernel package runs with ring 0, this is a supply-chain vulnerability. **Suggestion:** Either require SHA256 to be non-empty by default (fail the build), or emit a prominent warning when verification is skipped.

已认领本任务

已认领本任务

The current PR includes the reproducible guest build configuration, but prebuilt kernel and rootfs artifacts are not published yet. Users currently build them with Docker. I plan to provide a versioned and verified guest bundle so CubeVZ can run on a Mac Studio without Docker at runtime.

Updated this PR with additional end-to-end verification and safety hardening for the OpenCode integration. ### What changed - Added `checkpoint_fork_opencode.py` to verify that OpenCode state survives an explicit CubeSandbox checkpoint/fork and can continue in the fork. - Hardened the CubeEgress flow: - the real provider key is absent from the VM global environment - the agent command receives only a placeholder key - non-LLM egress is blocked - CubeEgress injects credentials for the real LLM request - Pinned direct Python dependencies. - Replaced NodeSource setup with checksum-verified Node.js archive install. - Added workflow-level mocked orchestration tests for run/resume/network/checkpoint paths. - Updated English and Chinese docs. ### Verification Template: `tpl-02605c6a4f1a4ff4b16cf6c7` Image digest: `sha256:70c17815fdf2cbb710ad259f9e06ec6691037fa9585db70494b95d0809dad155` | Check | Result | | --- | --- | | `run_opencode.py` real DeepSeek coding task | PASS | | `resume_opencode.py` pause/reconnect/continue | PASS | | `network_policy.py` default-deny + CubeEgress injection | PASS | | `checkpoint_fork_opencode.py` checkpoint/fork/continue | PASS | | CubeEgress raw audit shows DeepSeek `/chat/completions` status 200 | PASS | | 46 local unit tests | PASS | | Python compile | PASS | | `git diff --check` | PASS | | Docs `npm ci && npm run docs:build` | PASS | | Temporary key scan | PASS | Raw logs are attached in `pr766-opencode-verification-20260716.zip`. Assisted-by: Codex:GPT-5 [pr766-opencode-verification-20260716.zip](https://github.com/user-attachments/files/30074954/pr766-opencode-verification-20260716.zip)

@fslongjin Hi, the requested real CubeSandbox validation evidence, including screenshots, logs, command scripts, terminal transcript, and the SHA256-verified archive, is available above. I have also addressed the subsequent automated-review findings; the current head is `c82aa32f8bb86f8a464c775e9fe09788d8fb677f`, and the latest `review` check passes. When convenient, could you please re-review the latest head and update the previous change request if the concerns are resolved? Thank you.

@fslongjin Hi, the requested real CubeSandbox HTTP-registry validation evidence, including screenshots, logs, command scripts, terminal transcript, and the SHA256-verified archive, is available above. The insecure-registry allowlist has also been moved into the CubeMaster configuration file as suggested. The current head is `7808fc1ae441ac1d56b46c3b58533afa5e4b9cea`, and the latest `review` check passes. When convenient, could you please re-review the latest head and update the previous change request if the concerns are resolved? Thank you.

@kinwin-ustc Done. I added a bilingual section to the existing Persistent Storage documentation: - [English: Nested Mounts — Shared Read, Writer-Scoped](https://github.com/zehongye-gaga/CubeSandbox/blob/fix/944-deterministic-host-mount-order/docs/guide/persistent-storage.md#nested-mounts-shared-read-writer-scoped) - [中文：嵌套挂载——共享可读、写入范围独立](https://github.com/zehongye-gaga/CubeSandbox/blob/fix/944-deterministic-host-mount-order/docs/zh/guide/persistent-storage.md#嵌套挂载共享可读写入范围独立) The section covers: - the Agent Team/project/department/tenant group-workspace use case; - a read-only shared parent plus a writer-scoped nested directory; - the resulting read/write visibility matrix; - AppSnapshot restore ordering and subtree shadowing; - Linux permission and multi-node considerations; - the authorization boundary: the calling platform derives trusted `hostPath` values from authenticated ownership and group context. Validation: - `npm run docs:build`: PASS; - `git diff --check`: PASS; - bilingual content and technical claims independently reviewed against the implementation and E2E: PASS. The existing runtime changes are unchanged, so the previously reported Linux package-test and 10/10 restore E2E results still apply. The PR remains a single commit (`326f434`). Please take another look when convenient. Thanks!

The `OPENAI_API_KEY` commented-out entry on line 21 is misleading — Claude Code's `env_utils.py` (`PASSTHROUGH_ENV_NAMES`) never forwards `OPENAI_API_KEY` into the sandbox environment, and the `build_claude_code_env()` function only handles `ANTHROPIC_API_KEY`. If a user uncomments this expecting it to work, the key will be silently ignored. Either remove this line or add `OPENAI_API_KEY` to the passthrough env list in `env_utils.py`.

`hostname="127.0.0.1"` binds to loopback only — the server won't be reachable from the host driver via the sandbox's port forwarding. For server-mode to work (as documented in `docs/guide/integrations/opencode.md` which shows `--hostname 0.0.0.0`), the default must be `"0.0.0.0"` so the HTTP server listens on all interfaces. The docs correctly use `0.0.0.0`; the code default should match.

The `OPENAI_API_KEY` commented-out entry is misleading — Claude Code's `env_utils.py` never forwards `OPENAI_API_KEY` into the sandbox environment. If a user uncomments this expecting it to work, the key will be silently ignored. Either remove this line or add `OPENAI_API_KEY` to the passthrough env list.

**Missing doc comment:** This function normalizes Docker Hub canonical aliases (`index.docker.io`, `registry-1.docker.io`) to `docker.io` so that allowlist entries using any alias match the same image ref. The behavior is correct, but the lack of a doc comment makes it non-obvious to readers and could lead to duplicate "canonicalization" logic elsewhere. Suggestion: add a comment explaining the alias mapping so callers know the normalization contract.

**Redundant processing (two issues on this line):** 1. `strings.TrimSpace(configuredHost)` — this is dead code. `validateNativeInsecureRegistry` in `config.go` already rejects entries with surrounding whitespace before they reach this function. Having a silent tolerance here while validation rejects it creates an inconsistency that could mask a future refactoring bug. 2. `normalizeRegistryHost(…)` — the comparison target `registryHost` is the return value of `registryHostFromImageRef`, which already calls `normalizeRegistryHost` internally (`ref.go:65`). On every loop iteration, a no-op normalization is performed on an already-normalized value. Consider removing both the `strings.TrimSpace` and the outer `normalizeRegistryHost` call.

**Security:** This warning only fires when registry credentials are present, meaning an anonymous pull over HTTP (no username/password) proceeds silently with no log signal. Consider either: 1. Renaming to `warnInsecureRegistryPull` and emitting a warning whenever TLS is bypassed (regardless of credentials), or 2. Moving the generic warning into the caller and keeping this function for the credential-specific addendum. Without this, an operator has no runtime signal that a particular image export is traversing the network in cleartext.

@toreleon Thanks for your contribution. We have many options for creating an arm64 Linux sandbox， If a macOS sandbox can be created, it would be a great extension for CubeSandBox use cases.

**"exact host" language should note case-insensitive matching** The matching in `nativeInsecureRegistryEnabled()` uses `strings.EqualFold`, so host comparison is case-insensitive (e.g., `REGISTRY.EXAMPLE.COM:5000` matches `registry.example.com:5000`). The docs say "exact host (including any non-default port)" which implies byte-for-byte matching. Consider adding a brief note that host comparison is case-insensitive.

**Edge case: `registryHostFromImageRef("/")` returns `"docker.io"`** When `imageRef` is `"/"`, `strings.Split("/", "/")` produces `["", ""]`, `first` is `""`, and none of the `.`, `:`, or `"localhost"` checks match, so it falls through to return `"docker.io"`. While no real-world reference looks like this, adding an early `if first == "" { return "" }` guard after the split would make the contract airtight.

**Warning-only credential exposure is insufficient defense** This function logs a warning when credentials will be sent in cleartext, but does not block the operation. Log warnings are routinely missed in production. Consider promoting this to a hard error unless the operator sets an explicit opt-in flag (e.g., `--allow-credential-exposure-over-http`), especially given that the native path (line 86) sends credentials over unencrypted HTTP — not even unverified TLS.

**Security: `name.Insecure` drops TLS entirely, not just certificate verification** The native path uses `name.Insecure` which connects over **literal HTTP** (no encryption at all), while the dockerless path uses `--tls-verify=false` which still negotiates an encrypted TLS channel (just skips cert verification). These are meaningfully different threat models — credentials sent over HTTP can be trivially captured by any on-path observer. Consider using `remote.WithTransport` with an `http.Transport` that sets `InsecureSkipVerify: true` instead, to match the dockerless path's posture (encrypted channel, no cert verification). If plain HTTP is genuinely required, document the credential exposure risk more prominently than a log warning.

## Validation update: payload protection and lifecycle accuracy I have updated PR #702 with two focused follow-up improvements: 1. Added a serialized Webhook payload-size limit before signing, delivery, and retry. 2. Improved `sandbox.resumed` event accuracy for connect and resume flows. ### Candidate - Upstream baseline used for this candidate: `8bcbbbf71929b3329a799afead61a86cbc11089c` - Original Webhook integration point: `8e21332f` - Validated and pushed commit: `6afafa68da173967a5939501d9f4716f973dbf26` The upstream branch advanced after the original integration, but the additional upstream commits did not overlap with the files changed by this PR. I kept the already validated candidate unchanged so that the pushed commit remains identical to the runtime-tested version. ### Automated validation - `cargo fmt --all -- --check`: PASS - Lifecycle targeted tests: `5 passed, 0 failed` - `cargo check --all-targets`: PASS - `cargo test --all-targets`: `144 passed, 0 failed` - `cargo clippy --all-targets`: PASS with non-blocking warnings - Candidate worktree remained clean after validation ### Payload-size runtime validation The following checks were run directly on commit `6afafa68`: - Safety check and CubeAPI build: PASS - Webhook receiver startup: PASS - Oversized serialized payload: dropped with `payload_too_large` - Receiver count for the oversized event: `0` - Normal Webhook payload delivery: PASS - Normal payload JSON validation: PASS - Runtime exit code: `0` <img width="2555" height="1349" alt="01-final-6afafa68-payload-runtime" src="https://github.com/user-attachments/assets/426d85ba-3fa0-4075-a9a4-63cc77dbf166" /> ### Lifecycle runtime validation The following scenarios were run directly on commit `6afafa68`: - Paused sandbox → connect: emitted one `sandbox.resumed` event - Running sandbox → connect: emitted no duplicate event - Explicit resume: emitted the second `sandbox.resumed` event - RFC3339 timestamp compatibility: PASS - Runtime exit code: `0` - Candidate worktree remained clean - Official CubeAPI service was restored successfully - Port `3000` was listening after validation <img width="2541" height="1341" alt="02-final-6afafa68-lifecycle-runtime" src="https://github.com/user-attachments/assets/fe3585bc-97ed-48ce-a89e-07cc48b7c891" /> The original four-event Webhook behavior was covered by the previous PR #702 verification. This update reruns the relevant checks for the two follow-up changes directly on the pushed commit. The attached validation report contains the complete environment information, automated-test results, runtime results, and acceptance-criteria mapping. [final-validation-report.txt](https://github.com/user-attachments/files/30073540/final-validation-report.txt)

**Suggestion: extract trimmed values and avoid leaking MasterNodeIP** Two things here: 1. **Redundant `strings.TrimSpace` calls** — the same expressions appear in the condition and then again in the format arguments. Extracting once improves clarity. 2. **Internal IP leak** — `artifact.MasterNodeIP` is included verbatim (`%q`) in the error message. This error is stored in job records (via `error_message` in `image_job_runner.go`) and can be surfaced to API callers, revealing internal infrastructure IPs. Use a boolean indicator instead: ```go sha256Set := strings.TrimSpace(artifact.Ext4SHA256) != "" tokenSet := strings.TrimSpace(artifact.DownloadToken) != "" ipSet := strings.TrimSpace(artifact.MasterNodeIP) != "" if artifact.Status != ArtifactStatusReady || artifact.Ext4SizeBytes == 0 || !sha256Set || !tokenSet || !ipSet { return nil, 0, 0, 0, fmt.Errorf( "artifact %s is not ready for distribution (status=%s size_bytes=%d sha256_set=%t token_set=%t master_node_ip_set=%t); template build likely did not complete — check cubemaster logs for buildRootfsArtifact errors", artifact.ArtifactID, artifact.Status, artifact.Ext4SizeBytes, sha256Set, tokenSet, ipSet, ) } ```

**Suggestion: use comma-ok form for type assertion** ```go mu, ok := muV.(*sync.Mutex) if !ok { return nil, nil, false, errors.New("artifactBuildLocks: unexpected type") } ``` `sync.Map` stores `any`, so a bare type assertion will panic at runtime if (through refactoring or error) a non-`*sync.Mutex` value is stored. The comma-ok form removes this latent panic site at essentially no cost.

## Web Terminal implementation and verification update This update completes and strengthens the Web Terminal execution path: - The interactive Terminal RPC now reuses Cubelet's existing Exec target-resolution and containerd task initialization logic. - The selected `containerID` is forwarded through CubeAPI and CubeMaster to Cubelet. - `ctr-a` and `ctr-b` are verified to resolve to distinct container handles before loading the selected containerd task. - Missing container targets are rejected. - Concurrent terminal output streams remain isolated. - Client disconnect/close cleanup deletes only the corresponding exec process while preserving the sandbox namespace. ### Verification results - Web lint: passed - Web production build: passed - CubeAPI full unit suite: 93 passed - CubeMaster relevant package tests: passed - Cubelet Web Terminal targeted tests: passed - CubeShim suite: 24 passed - `git diff --check`: passed - Release binaries for CubeAPI, CubeMaster, Cubelet, CubeShim, and cube-runtime: built successfully - Local one-click deployment services and health checks: passed - Real deployed WebUI single-container terminal: shell I/O, file operations, resize, reconnect, and audit logging passed The six failures observed in the complete Cubelet package were reproduced on the clean `cdb1af1` baseline and are existing host-environment-dependent NUMA, cgroup, volume, and probe failures. The previously attached screenshots remain the real deployed WebUI evidence. The raw targeted test log and consolidated verification summary are attached below. ### Attachments [02-verification-summary.txt](https://github.com/user-attachments/files/30073489/02-verification-summary.txt) [01-cubelet-terminal-targeted.log](https://github.com/user-attachments/files/30073490/01-cubelet-terminal-targeted.log)

**Graceful degradation**: Consider using a comma-ok type assertion here to avoid a panic if the map ever stores a non-`*sync.Mutex` value (e.g., from a refactor or test helper): ```go mu, ok := muV.(*sync.Mutex) if !ok { return nil, nil, false, fmt.Errorf("artifactBuildLocks: unexpected type %T for key %s", muV, artifactID) } ```

**Minor: Internal IP address in propagated error**: `MasterNodeIP` is embedded in the error message with `%q`. This error propagates up through `runTemplateImageJob` into the job record's `error_message` field, which might surface through API responses. Consider removing `master_node_ip=%q` from the user-facing error format to avoid leaking internal network topology; log it separately at a higher log level if needed for operations.

**Unbounded memory growth**: Entries are added to `artifactBuildLocks` via `LoadOrStore` on every unique `artifactID`, but never removed. Over the lifetime of a long-running process, this map accumulates a `*sync.Mutex` entry for every distinct image spec ever built. For most deployments this is negligible (~KB per unique spec), but in high-churn environments with many unique template configurations, memory grows linearly with unique artifact IDs. Consider either: - Adding cleanup (e.g., removing the entry after the build completes, with care to avoid races against concurrent `LoadOrStore`) - Documenting this as an intentional bounded-leak assumption (sync.Map usage pattern also seems inverted — a plain `map[string]*sync.Mutex` with a short-lived package mutex would be simpler and avoid allocating a new Mutex on every call.)

**Lock scope is broader than needed**: The mutex is acquired before the DB lookup (`findReusableRootfsArtifact`) and held across the entire `ensureRootfsArtifact` function, including `BuildExt4` which can take 30-120 seconds for large images. A second request for the same `artifactID` blocks for the full duration doing nothing. The DB operations (`findReusableRootfsArtifact`, `claimRootfsArtifactForBuild`) are already serialized by the `FOR UPDATE` row lock in `claimRootfsArtifactForBuild`. The Go mutex is only needed to protect the shared filesystem paths during `BuildExt4`. Narrowing the lock to just that critical section — with a re-check of artifact status after acquisition — would reduce blocking and allow DB operations to proceed concurrently.

**Graceful degradation**: Consider using the comma-ok form to avoid a panic if the map ever stores a non-`*sync.Mutex` value (e.g., from a refactor or test helper): ```go mu, ok := muV.(*sync.Mutex) if !ok { return nil, nil, false, fmt.Errorf("artifactBuildLocks: unexpected type %T for key %s", muV, artifactID) } ```

**Minor: Internal IP in propagated error**: `MasterNodeIP` with `%q` is embedded in the error message. This error propagates through `runTemplateImageJob` into the job record's `error_message` field, which may surface through API responses. Consider removing `master_node_ip=%q` from the propagated error to avoid leaking internal network topology; log it separately if needed for operations.

**Lock scope is broader than needed**: The mutex is acquired before the DB lookup (`findReusableRootfsArtifact`) and held across the entire `ensureRootfsArtifact` function, including `BuildExt4` which can take 30-120 seconds for large images. A second request for the same `artifactID` blocks for the full duration. The DB operations are already serialized by the `FOR UPDATE` row lock in `claimRootfsArtifactForBuild`. The Go mutex is only needed to protect the shared filesystem paths during `BuildExt4`. Narrowing the lock to just that critical section -- with a re-check of artifact status after acquisition -- would reduce blocking and allow DB operations to proceed concurrently.

**Unbounded memory growth**: Entries are added to `artifactBuildLocks` via `LoadOrStore` on every unique `artifactID`, but never removed. Over the lifetime of a long-running process, this map accumulates a `*sync.Mutex` entry for every distinct image spec ever built. For most deployments this is negligible, but in high-churn environments the memory grows linearly with unique artifact IDs. Consider either: - Adding cleanup (e.g., removing the entry after the build completes, with care to avoid races against concurrent `LoadOrStore`) - Documenting this as an intentional bounded-leak assumption (The `sync.Map` access pattern is also inverted for a write-mostly workload -- a plain `map[string]*sync.Mutex` with a short-lived package mutex would be simpler and avoid allocating a new `sync.Mutex` on every call.)

<!-- cubesandbox-auto-review:start --> ## Review: PR #194 — Serialize concurrent rootfs builds This is a focused, well-motivated fix for a real race condition. The synchronization design is correct, the defense-in-depth guard improves debuggability, and the comments are excellent. A few observations below. ### Observations **1. `artifactBuildLocks` has no eviction path (artifact_build.go:40)** The `sync.Map` accumulates one `*sync.Mutex` per unique `artifactID` and never removes entries. Consider adding a `defer artifactBuildLocks.Delete(artifactID)` after `mu.Unlock()` — safe because the last goroutine to hold the mutex runs the defer, and `Delete` leaves concurrent waiters unaffected. **2. TrimSpace validates but does not sanitize (distribution.go:154)** `Ext4SHA256` and `DownloadToken` with surrounding whitespace pass the guard but are used raw in cubelet annotations. Trim once and reuse, or reject values that differ from their trimmed form. **3. MySQL-specific duplicate-key detection (artifact_build.go:87–88)** The `"1062"` / `"Duplicate entry"` string checks are MySQL-specific. A small helper `isDuplicateKeyError` would centralize the DB-specific knowledge. **4. Pre-existing: duplicate `loadCubeEgressCA` call** `loadCubeEgressCA` is called in `image_job_runner.go:76` (discarded) and again inside `ensureRootfsArtifact`. Consider threading the data through to avoid the redundant I/O. ### Positives - The failure-mode documentation on `artifactBuildLocks` is excellent — it explains why DB-level serialization is insufficient and what the lock protects. - The defense-in-depth guard in `distributeRootfsArtifact` returns a diagnostic error with all relevant state. - Lock scope is correct: only serializes same-`artifactID` callers, different images build in parallel. - All error messages include identifiers for log-based debugging. <!-- cubesandbox-auto-review:end -->

> Would a bilingual section in the existing Persistent Storage documentation be sufficient? I think it's enough

@fslongjin I pushed another update addressing the latest bot review feedback. Changes in the new commit: - Removed the internal `stateFile` path from task API responses and replaced it with `persisted: true`. - Removed the redundant `@Autowired` from `TaskState`. - Added tests for task state read/write failure paths, malformed `createdAt` handling, blank-title defaulting, and atomic-move fallback. - Simplified the Dockerfile Maven cache layout while keeping `/workspace/.m2/repository` available for offline sandbox builds. Validation: - `mvn test`: 19 tests passed, 0 failures - Docker image build: passed - In-image `mvn --offline -DskipTests package`: passed Could you please take another look when you have time?

`GetNativeInsecureRegistries` reads `cfg` without synchronization, matching the existing pattern of `GetConfig()` and `GetAllowedHostMountPrefixes()`. If config is ever hot-reloaded, concurrent readers and the writer will race. If config is immutable after init, this is fine — worth noting for future refactoring.

This error message says *"must bracket IPv6 addresses"*, but entries with ≥2 colons can be simply malformed (e.g., `host:5000:5000`). The rejection is correct, but the message is misleading. Consider: *"entry %q is not a valid host[:port] and is not a bracketed IPv6 address"*.

`normalizeRegistryHost` only normalizes `index.docker.io` → `docker.io`. Docker Hub also resolves through `registry-1.docker.io`, which would not be collapsed. If an operator discovers this hostname and configures it, the allowlist match silently fails (safe failure — TLS stays on — but confusing). Consider also normalizing `registry-1.docker.io`, or document the limitation.

`GetNativeInsecureRegistries` reads `cfg` without synchronization, matching the existing pattern of `GetConfig()` and `GetAllowedHostMountPrefixes()`. If config is ever hot-reloaded, concurrent readers and the writer will race. If config is immutable after init, this is fine — worth noting for future hot-reload work.

This error message says "must bracket IPv6 addresses", but entries with ≥2 colons can be simply malformed (e.g., `host:5000:5000`). The rejection is correct, but the message is misleading. Consider a more generic message like: *"entry %q is not a valid host[:port] and is not a bracketed IPv6 address"*.

`normalizeRegistryHost` only normalizes `index.docker.io` → `docker.io`. Docker Hub also resolves through `registry-1.docker.io`, which is not collapsed. If an operator configures `registry-1.docker.io`, the allowlist match silently fails (safe failure — TLS stays on — but confusing). Consider normalizing `registry-1.docker.io` as well, or document the limitation.

Consider strengthening host:port validation `url.Parse("//" + registry)` accepts entries with invalid port values (e.g. `host:999999`, `host:0`, `host:trailingtext` all parse with the full string as `parsed.Host`). Consider using `net.SplitHostPort` for explicit port validation and `net.ParseIP` / RFC 1123 label checks for the host part. This would reject malformed entries earlier, at config validation time, rather than failing at connection time.

Consider refactoring to avoid the fragile double-append pattern ```go configArgs := append(append([]string{}, inspectArgs...), "--config", sourceRef) ``` This double-append relies on reader familiarity with Go's slice capacity semantics. While correct, it's easy to accidentally mutate `inspectArgs` during maintenance (e.g., if someone swaps the outer `append` target). A cleaner alternative: ```go // Build config args independently configArgs := []string{"inspect"} if insecure { configArgs = append(configArgs, "--tls-verify=false") } configArgs = append(configArgs, "--config", sourceRef) ``` This also makes the two skopeo command lines explicit rather than deriving `configArgs` from `inspectArgs`.

<!-- cubesandbox-auto-review:start --> ## PR Review: feat(cubeapi): add webhook event notifications ### Overview This PR adds asynchronous HTTP webhook delivery for sandbox lifecycle events. The implementation is well-structured overall — clean async architecture, bounded queues, HMAC-SHA256 signing, exponential retry, and good test coverage for the core delivery path. Below are the noteworthy issues found during review. --- ### Critical **1. Unbounded retry delay can hang background task** (http.rs:162) When attempt >= 64, checked_shl overflows and falls through to unwrap_or(u64::MAX), producing a Duration::from_millis(u64::MAX) — a ~584 million year sleep. Setting WEBHOOK_MAX_RETRIES=100 via env var triggers this silently. The background task blocks permanently, dropping all subsequent events. Fix: Cap max_retries at 32, or add an overflow guard in the retry loop. --- ### Medium **2. Filtering happens after queue enqueue, wasting capacity** (http.rs:86) config.accepts() runs inside the background task after the event has been dequeued from the bounded channel. Non-matching events still consume queue capacity. In high-volume scenarios this could cause legitimate subscribed events to be dropped. Fix: Move the filter check to HttpLogger::log() before try_send. **3. No HTTPS enforcement for webhook URLs** (main.rs:223) Webhook URLs from WEBHOOK_URLS are accepted without scheme validation. HTTP URLs leak the HMAC signing key and event payloads in cleartext. Fix: Add a scheme check that warns on non-HTTPS URLs (with an exemption for loopback addresses). --- ### Low **4. Response body discarded on HTTP failure** (http.rs:148) When delivery fails, the response body is discarded. Many webhook endpoints include structured error details in their response bodies. Fix: Read and log a short snippet (256 bytes) of the response body. **5. Queue-full warning lacks event identity** (http.rs:182) When events are dropped due to a full queue, the warning log does not include the event name, making correlation difficult. Fix: Destructure TrySendError::Full and log the event name. **6. Per-URL reqwest::Client instead of shared** (http.rs:75) Each webhook URL gets its own reqwest::Client with separate connection pool, while AppState already holds a shared one. Fix: Pass a shared &Client into HttpLogger::new(). **7. webhook_events cloned for each URL** (main.rs:225) The Vec<String> of subscribed events is cloned per URL. All URLs share the same event set. --- ### Test Coverage Gaps The 4 existing tests cover the happy path well, but these paths are untested: - Retry exhaustion (all attempts fail) — High - Non-retryable 4xx (400, 403, 404) — High - Network/HTTP error (DNS failure, connection refused) — High - parse_webhook_events() edge cases — High - Queue full / dropped events — Medium - Worker shutdown (channel closed) — Medium The main.rs wiring code (parse_webhook_events, default_webhook_events) has zero tests despite being pure string-parsing logic. --- ### Documentation Minor: The English webhook guide links to the directory (tree/...) while the Chinese guide links to a specific file (blob/...). For consistency, both should use the same URL structure. --- ### What is done well - Clean async architecture: try_send for non-blocking log(), bounded channel prevents unbounded memory growth - HMAC signature computed once before the retry loop and reused across attempts - Redirect policy set to none — good SSRF defense, and tested - Python receiver uses hmac.compare_digest — constant-time comparison, no timing side-channel - Request timeout is configured and enforced - Good Rust practices: bytes::Bytes for cheap clones, saturating_mul/checked_shl for overflow safety - English and Chinese documentation is accurate and matches the implementation - Receiver example works and matches its README --- Reviewed by: Claude Code <!-- cubesandbox-auto-review:end -->

**Per-URL `reqwest::Client` instead of sharing the one from AppState** Each webhook URL creates its own `reqwest::Client` with a separate connection pool and DNS cache. The `AppState` struct already holds a shared `reqwest::Client` configured with `pool_max_idle_per_host(100)`. **Suggestion:** Accept a `&Client` parameter (or cloned `Client`) in `HttpLogger::new()`. Bonus: this also makes `HttpLogger::new()` infallible (no `Result`), simplifying the initialization code in `main.rs`.

**No validation that webhook URLs use HTTPS** Webhook URLs are parsed directly from the `WEBHOOK_URLS` env var with no scheme validation. If a deployer accidentally uses `http://`, the webhook secret (HMAC signing key) and all event payloads are transmitted in cleartext. **Suggestion:** Add a scheme check when building the webhook config: ```rust for url in urls.split(',').map(str::trim).filter(|url| !url.is_empty()) { if !url.starts_with("https://") { if !url.starts_with("http://localhost") && !url.starts_with("http://127.0.0.1") { tracing::warn!(webhook_url = %url, "Webhook URL does not use HTTPS"); } } // ... } ``` (Allowing HTTP for loopback is standard for development setups.)

**Queue-full warning lacks event-level context** When the queue is full, the warning doesn't include which event was dropped or which webhook URL was the target. This makes it hard to correlate dropped events with sandbox activity in production. **Suggestion:** Destructure the `TrySendError::Full` to access the wrapped `LogEvent` and log the event name: ```rust Err(mpsc::error::TrySendError::Full(Message::Event(event))) => { warn!(event = %event.event, "Webhook queue is full; dropping event"); } ```

**Response body discarded on HTTP delivery failure** When a webhook endpoint returns an error status (retryable or not), the response body is dropped. Many webhook endpoints include structured error details in their response bodies (e.g., `{"error": "rate limited", "retry_after": 30}`) that would be valuable for debugging. **Suggestion:** Read a limited snippet of the response body (e.g., first 256 bytes) and include it in the warning log message. ```rust Ok(response) => { let body_snippet = response.text().await.unwrap_or_default(); warn!( event = %event.event, status = %response.status(), attempt, body = %body_snippet.chars().take(256).collect::<String>(), "Webhook delivery failed" ); } ```

**Non-matching events consume bounded queue capacity** Event filtering via `config.accepts()` happens *inside* the background task, after the event has already been dequeued from the bounded `mpsc` channel. This means events that no endpoint subscribes to still consume queue capacity. In a high-volume event scenario, this could cause legitimate subscribed events to be dropped (queue full) while the channel is busy cycling through filtered-out events. **Suggestion:** Move the `accepts()` check (or a copy of the event filter set) to `HttpLogger::log()`, before the `try_send` call. The filter set is a `HashSet<String>` which is cheap to clone and the config already derives `Clone`. (A secondary benefit: the `HttpLogger` struct currently doesn't hold the filter set; you'd need to store a copy. The performance cost is negligible for a few event types.)

**Unbounded retry delay can hang the background task forever** When `attempt >= 64`, `1u64.checked_shl(64)` returns `None` and the code falls back to `u64::MAX`. Combined with `saturating_mul` this produces a `Duration::from_millis(u64::MAX)` — roughly 584 million years. A user setting `WEBHOOK_MAX_RETRIES=100` via environment variable would trigger this. The background task blocks on that sleep permanently, silently dropping all subsequent events for this endpoint. **Suggestion:** Cap `max_retries` at 32 (or some sane value) in `HttpLogger::new`, or add an explicit overflow guard inside the retry loop.

**Consider aligning return convention with `GetAllowedHostMountPrefixes`** — This function returns `nil` when the configured list is empty, but the similar `GetAllowedHostMountPrefixes` returns `[]string{}` (non-nil empty slice). While `nil` and empty slices are functionally equivalent for `range` and `len`, returning a non-nil empty slice is more defensive against callers that use `== nil` checks (a pattern present in this codebase, e.g., `IsAppHooks` at line 1217).

**Consider adding test cases for `registryHostFromImageRef` edge cases** — The matching logic relies on `registryHostFromImageRef` for host extraction, but that function has only two existing tests. Untested scenarios include: - IPv4 address registries (`192.168.1.1:5000/team/app`) - IPv6 registries - localhost (`localhost:5000/team/app`) - Empty image ref string Adding a direct table-driven test for `registryHostFromImageRef` would make the matching logic's correctness easier to trust.

**`index.docker.io` normalization gap** — `registryHostFromImageRef` (ref.go:54-65) returns `index.docker.io` for long-form Docker Hub references (e.g. `index.docker.io/library/nginx:latest`), but `dockerlessCanonicalName` in source.go:412-417 normalizes this to `docker.io`. If a user configures `docker.io` in the allowlist, it will match short names (`library/nginx:latest`) but silently fail to match long-form references (`index.docker.io/...`). Consider normalizing `index.docker.io` → `docker.io` inside `registryHostFromImageRef` for consistency.

**Credential exposure risk** — When `nativeInsecureRegistryEnabled` returns true AND explicit credentials are provided via `RegistryUsername`/`RegistryPassword`, the credentials are sent in the HTTP Authorization header over an unencrypted connection. Consider adding a prominent warning in the documentation (and ideally an explicit opt-in config flag) to ensure operators are aware of this risk when using authentication with HTTP registries.

<!-- cubesandbox-auto-review:start --> ## Review: PR #873 — `fix(cubemaster): support configured HTTP registries` **Overall:** This is well-written code that follows Go conventions, has strong test coverage, and demonstrates good defensive programming. The allowlist architecture is clean and correctly limits the scope of HTTP usage. Below are the findings I consider noteworthy across all review dimensions. --- ### Security **1. Asymmetric TLS posture: native path uses HTTP, dockerless uses unverified TLS** (source.go:86) The native path passes `name.Insecure` to `go-containerregistry`, which drops TLS entirely and connects over plain HTTP. The dockerless path uses `--tls-verify=false`, which still negotiates encryption but skips certificate verification. Credentials and image content are trivially interceptable on the native path. Consider using `remote.WithTransport` with a TLS transport that sets `InsecureSkipVerify: true` instead of `name.Insecure` to match the dockerless path's encryption posture. **2. Credential exposure over HTTP is warning-only** (source.go:167-175) `warnIfInsecureRegistryCredentials` only logs a warning when credentials will be sent over cleartext. Log warnings are routinely missed; the operation proceeds regardless. Consider a hard error unless an explicit opt-in is configured. --- ### Code Quality **3. `registryHostFromImageRef` edge case for `"/"` input** (ref.go:54) `strings.Split("/", "/")` produces `["", ""]`, and the empty first segment falls through to return `"docker.io"` rather than `""`. An early `if first == "" { return "" }` guard would make the behavior airtight. --- ### Documentation Accuracy **4. "Exact host" language should note case-insensitive matching** (docs/guide/self-build-deploy.md:337) `nativeInsecureRegistryEnabled` uses `strings.EqualFold` for host comparison, so `REGISTRY.EXAMPLE.COM:5000` matches `registry.example.com:5000`. The documentation says "exact host (including any non-default port)" which implies byte-for-byte matching. Consider clarifying that matching is case-insensitive. --- ### Test Coverage **5. `warnIfInsecureRegistryCredentials` is untested** (source.go:167) The insecure+credentials combination is not tested in either the native or dockerless paths. The existing `TestPrepareDockerlessSourceUsesInsecureRegistryConfig` exercises the insecure path without credentials, so the warning branch is uncovered. **6. `normalizeRegistryHost` has no direct unit test** (ref.go:67) The function is exercised indirectly through `registryHostFromImageRef` and `nativeInsecureRegistryEnabled`, but its standalone behavior (Docker Hub alias normalization and passthrough for other hosts) is not tested in isolation. --- ### Positive Highlights - **Thorough validation**: 21 test cases cover empty entries, whitespace, schemes, paths, credentials, port boundaries, IPv4, IPv6, and edge combinations. - **Defensive copies**: `GetNativeInsecureRegistries()` returns `append([]string{}, ...)` so callers cannot mutate the live configuration. - **Security-conscious logging**: `warnIfInsecureRegistryCredentials` explicitly warns operators about the credential exposure risk. - **IPv6 handling**: Port validation correctly handles bracketed IPv6 and rejects unbracketed IPv6 and bracketed IPv4 — a notoriously tricky area. - **EN/CN documentation parity**: The English and Chinese docs have identical structure, examples, and warnings. <!-- cubesandbox-auto-review:end -->

**`body.clone()` performs an O(n) allocation + memcpy on every retry.** With the default `max_retries=3`, the full serialized body is cloned 4 times. Convert the body to `bytes::Bytes` (already a transitive dependency via reqwest) so `clone()` becomes an O(1) refcount bump: ```suggestion let body: bytes::Bytes = match serde_json::to_vec(&event).map(bytes::Bytes::from) { Ok(body) => body, Err(err) => { ... }; for attempt in 0..=config.max_retries { let mut request = client .post(&config.url) .header(reqwest::header::CONTENT_TYPE, "application/json") .body(body.clone()); ``` Similarly, the HMAC signature can be precomputed before the loop rather than recomputed on each retry.

@kinwin-ustc Would a bilingual section in the existing Persistent Storage documentation be sufficient? I plan to describe a generic group-scoped workspace pattern: a shared read-only parent mount with per-agent or per-sandbox writable descendants. In enterprise deployments, the group boundary may represent a project, team, department, tenant, or Agent Team, while identity-to-host-path authorization remains the responsibility of the calling platform. The section would also explain parent-before-child ordering, subtree shadowing, permission boundaries, and recommended constraints.

**`body.clone()` performs an O(n) allocation + memcpy on every retry.** With the default `max_retries=3`, the full serialized body is cloned 4 times. Convert the body to `bytes::Bytes` (already a transitive dependency via reqwest) so `clone()` becomes an O(1) refcount bump: ```suggestion let body: bytes::Bytes = match serde_json::to_vec(&event).map(bytes::Bytes::from) { Ok(body) => body, ... }; for attempt in 0..=config.max_retries { let mut request = client .post(&config.url) .header(reqwest::header::CONTENT_TYPE, "application/json") .body(body.clone()); ``` Similarly, the HMAC signature can be precomputed before the loop rather than recomputed on each retry.

**`expect` will silently kill all future webhook delivery if triggered.** `HmacSha256::new_from_slice` returns a `Result`, and while HMAC-SHA256 accepts any key length, this `expect` is a ticking bomb: if it ever panics, the background task dies, the `mpsc::Receiver` is dropped, and every subsequent `Logger::log` call sees `TrySendError::Closed` — silently dropping all future webhook events. Prefer graceful error handling: ```suggestion let mut mac = match HmacSha256::new_from_slice(secret.as_bytes()) { Ok(mac) => mac, Err(_) => { error!(event = %event.event, "HMAC initialization failed; dropping event"); return; } }; ```

**Security: SSRF via redirect following.** The `reqwest::Client` is built without a redirect policy, so it follows up to 10 redirects by default. A misconfigured or attacker-influenced webhook URL can redirect the POST to internal services (cloud metadata `169.254.169.254`, internal K8s DNS, localhost services). The event body and HMAC secret (when configured) would be forwarded to the redirect target. ```suggestion .timeout(Duration::from_secs(config.request_timeout_secs)) .redirect(reqwest::redirect::Policy::none()) .build()?; ```

**Orphan env var**: `OPENCODE_SERVER_PORT` is defined here but never read by the Python code. The `opencode_serve_command()` in `env_utils.py` takes `port` as an explicit parameter defaulting to 4096 and never checks the environment. Users who set this variable will not have it honored. Either wire it into `env_utils.py` or remove this comment.

**Consider adding `--ignore-scripts`**: The `npm install -g` command uses `--no-audit --no-fund` but omits `--ignore-scripts`. The pi-agent integration Dockerfile correctly includes it. Without `--ignore-scripts`, pre/post-install scripts from npm packages execute during `docker build`, which could pull in untrusted code at build time. Applies to all three integration Dockerfiles (claude-code line 49, opencode line 49).

**Unused import**: `provider_key_name` is imported on line 39 but is never referenced in this file. The equivalent in `opencode-integration/network_policy.py` uses it (line 39), but here it's dead code. Removing it would clean up the unused symbol.

@zehongye-gaga Could you add some documentation explaining the use cases and related issues of nested mounts?

@ls-ggg Thanks again for the feedback. Since our earlier discussion about nested mounts, I have addressed the review comments and added a concrete manual AppSnapshot restore integration E2E. The test covers: - child-first input being reordered parent-first; - automatic creation of mount target directories; - a shared read-only parent mount; - a writable agent-specific nested mount; - read-only isolation for sibling agent directories; - 10 consecutive successful restores. I ran it against the deployed PR Cubelet binary: all 10 restores passed. The relevant cubeboxcbri package tests and the deployment smoke check also pass, and all review threads are resolved. Do you think this PR is ready to merge now? If you still have any concerns about supporting nested mounts or the test coverage, please let me know.

`gnupg` adds ~15 MB to each image but is not strictly needed — the NodeSource setup script works with `gpgv`, which ships with the base image. Consider removing it to reduce image size across the three agent images.

Using `or` for coalescing means explicitly setting `VAR=""` returns the default rather than the empty string. This differs from the standard "get with default" pattern and should be documented, as it may surprise users expecting `os.environ.get(name, default)` semantics.

If `sandbox.pause()` returns a falsy value (indicating failure), the script continues with `Sandbox.connect()` against the original sandbox ID, which may appear to work while silently skipping the snapshot step. Consider adding `if not paused_id: raise SystemExit(...)` to surface the failure early.

If the stream ends with a partial JSON line (e.g., an error message without a trailing newline), the content remaining in `buffer["parts"]` is never flushed. While Claude Code newline-terminates normal events, error output may not. Consider adding a `flush()` method to the writer closure that renders any remaining buffer content, and calling it after the command completes.

If `exit_code` is `None` (e.g., the command timed out or the result is malformed), this returns silently without warning. A completely failed command producing no exit code would pass as "success." Consider at minimum logging a warning when `exit_code is None`.

When `include_secrets=False` (vault path), proxy env vars (`HTTP_PROXY`, `HTTPS_PROXY`) are still forwarded into the sandbox via `PASSTHROUGH_ENV_NAMES`, but the proxy host is NOT listed in the egress allow rule in `network_policy.py`. This means the agent silently fails to reach the LLM API when a proxy is configured in the vault path. Either strip proxy vars in the vault path or emit a warning that the proxy will be blocked.

I have resolved all pending review comments. PTAL @lisongqian @fslongjin

**Performance note:** Lines 59 and 66 copy the full Maven repository (~200-500MB) to and from the build cache mount during the second `RUN` layer. Combined with line 54, the repository is copied three times in total across the two layers. Consider restructuring to avoid this back-and-forth — either set the mount cache as the repo location directly for both layers, or use a single build stage. This would reduce build time and final image layer size.

**Test gap:** The `isExpired()` method treats non-String `createdAt` values (line 124-125) and unparseable date strings (line 129-130) as expired — which means data corruption in a single task's `createdAt` field causes that task to be silently deleted on the next prune cycle. Neither edge case is covered by the test suite. Consider adding tests with malformed JSON state files to verify these paths.

**Test gap:** The `IOException` path on line 140-142 (file read failure → 500), the `AtomicMoveNotSupportedException` fallback on line 156-157, and the `IOException` on line 159-161 (write failure → 500) are all untested. These are real-world failure paths (corrupt state file, disk full, non-atomic filesystem) that could surface in production. Consider adding tests that inject a corrupt state file or make `stateFile` a directory to trigger these paths.

**Suggestion:** Returning the internal filesystem path (`/tmp/cubesandbox-spring/state/tasks.json`) in every API response leaks infrastructure details. While this is a demo example, a production service would want to avoid embedding server paths in API payloads. Consider using a boolean flag like `"persisted": true` instead, or describing the persistence layer in documentation rather than response data.

**Suggestion (optional):** This `@Autowired` annotation is redundant — Spring 4.3+ automatically performs injection on any class with a single constructor, and the rest of this project (e.g., `TaskController`) follows that convention. Removing it would align the code with modern Spring practice and reduce clutter.

done

fixed

> > 2.2 Passfd IO Bypass Path Enablement > > Problem: Initial test showed all passfd ports were 0, logs were not captured. > > Hello, I'd like to know why we need to enable this `IsDebugStdout` here? Please ignore my previous comment, I misunderstood. I originally wanted to verify the log path from `guestos init` -> `vsock` -> `Cubelet log` to ensure that logging continues to work correctly in the pause/restore scenario. However, I found that the existing integration test cases in the `hypervisor` already cover this. I currently have the complete test cases, as shown below: | Test Case Name | Covered Scenario | | --- | --- | | `test_virtio_vsock_passthrough_fd` | Verifies vsock communication using file descriptor (FD) passthrough, including bidirectional data transfer verification (which is the mechanism used for routing `Cubelet` logs). | | `test_virtio_vsock_passthrough_fd_hotplug` | Verifies that bidirectional vsock FD passthrough communication works correctly when the vsock device is dynamically hot-plugged. | | `test_restored_vsock_reuses_guest_listener_for_new_passthrough_fd` | Verifies that after a VM restore, the vsock connection can reuse the existing guest listener for a new passthrough FD, and successfully performs bidirectional data verification before and after the snapshot to ensure the `guestos init` -> `vsock` -> `Cubelet log` connection fully recovers without data loss. |

My proposed approach is **option 3 (hybrid object)**: `get_info()` will return a new `SandboxInfo` dataclass that supports **both** E2B-style attribute access and the existing dict-style access, aligning with E2B's shape while staying backward-compatible. Implementation plan: - Add a `SandboxInfo` dataclass with E2B-compatible snake_case, typed fields: - `sandbox_id`, `template_id`, `sandbox_domain`, `started_at` / `end_at` (`datetime`), `cpu_count`, `memory_mb`, `envd_version`, `metadata`, `name` - `state` as a new `SandboxState` enum (`RUNNING` / `PAUSED`), gracefully falling back to the raw string for unknown values - Implement `__getitem__` and `.get()` on the object so existing code using `info["sandboxID"]` / `info.get("state")` keeps working (mapped back to the original CubeAPI keys). Dict-style access returns the **raw** values (`state` as string, timestamps as strings) to guarantee strict backward compatibility; attribute access returns the typed values. - Add a `from_dict` classmethod handling field mapping, ISO-8601 timestamp parsing (incl. trailing `Z`), and state normalization. - Export `SandboxInfo` and `SandboxState` from the package `__init__`. - Align the field set with the existing Go SDK `SandboxInfo` for cross-language consistency. - Add tests covering field mapping, timestamp parsing, state normalization, and dict/attr backward compatibility; update the `get_info()` docstring and README.

The goal of CubeVZ is to let Apple Silicon machines likeMac Studio to serve as native CubeSandbox hosts. CubeVZ runs directly on macOS using Virtualization.framework, without Lima, QEMU, or nested KVM. It is currently experimental, but I am continuing to optimize and harden it toward production use, with performance as close as possible to the Linux/KVM backend.

Consider making `validate_api_key` configurable (e.g., via an env var) rather than hardcoding `False`. Disabling validation unconditionally could mask misconfigured or revoked API keys in production-like environments.

The `known_bug` marker is declared here but no test currently uses it. Either remove the marker declaration or add a regression test that exercises it.

The `.env` loader splits by `\n` only. If the file was edited on Windows (CRLF), values will retain trailing `\r` characters. Consider `.rstrip("\r")` on each raw line in addition to `.strip()`.

The pagination loop `while getattr(entries, "has_next", False)` could spin indefinitely if `has_next` never becomes `False`. Consider adding a maximum page count as a safety guard.

`_list_sandboxes_via_cubeapi` is called at line 305 but defined at line 355. While this works at Python module level, consider moving it before the call site for top-to-bottom readability.

When `E2B_API_KEY` is not set, this falls back to `CUBE_API_KEY`, which may unintentionally leak the CubeSandbox control-plane API key to the E2B SDK. Consider making this an explicit opt-in (e.g., requiring a separate `E2B_API_KEY` env var) rather than silent fallback.

Consider making `validate_api_key` configurable (via env var or method parameter) rather than hardcoding `False`. Disabling validation unconditionally could mask misconfigured or revoked API keys.

**Fragile negation pattern** — this works correctly, but the sequence of negate-then-re-ignore-then-negate is brittle and not immediately obvious. Adding `!/tests/e2e/sdk_compat/` before `/tests/e2e/*` would silently break it, and `!/tests/e2e/sdk_compat/**` is needed alongside `!/tests/e2e/sdk_compat/` to catch nested files. Consider adding a comment explaining the intent, or simplifying by keeping the test directory at a top-level path instead.

**`list.pop(0)` is O(n) per eviction** — with `MAX_TRACE_EVENTS=100`, each eviction shifts ~99 elements. Consider using `collections.deque(maxlen=MAX_TRACE_EVENTS)` for O(1) eviction, which also eliminates the manual `_dropped_events` tracking at line 200-201.

**`None`-adapter flows into cleanup when `create_adapter` raises `ImportError`** — when `create_adapter` raises `ImportError`, `pytest.skip()` raises `pytest.skip.Exception` and the `finally` at line 291 still runs, calling `_cleanup_sdk_sandbox(adapter=None, ...)`. This cascades through `safe_kill()` where `adapter.info()` on `None` triggers `AttributeError`, producing confusing error messages. Guard `_cleanup_sdk_sandbox` with an early return for `None`, or restructure the fixture so the cleanup path never sees `adapter=None`.

**Backend-specific skip knowledge in generic fixture** — the e2b auto-pause skip couples backend-specific logic into the shared `sdk_sandbox` fixture. As more backends are added, this pattern will make the fixture harder to maintain. Consider expressing this as a capability marker (e.g., `requires_platform_lifecycle`) that E2B simply doesn't declare, moving the skip to a more appropriate layer.

**`E2B_CAPABILITIES` and `CUBESANDBOX_CAPABILITIES` are identical** — this means the `@pytest.mark.requires_capability()` marker system never differentiates between backends. The code path in `conftest.py:223-226` is effectively dead for filtering. If both backends genuinely support the same capabilities, consider whether the marker infrastructure is pulling its weight, or split out capabilities that are truly backend-specific.

**Silent fallback to default API key with validation disabled** — when neither `E2B_API_KEY` nor `CUBE_API_KEY` is set, this silently falls back to `"e2b_000000"` and passes `validate_api_key=False`. No warning is emitted. A misconfigured runner could proceed against development environments without proper authentication. Consider emitting a `warnings.warn()` or log message when the default key is used, and remove the hardcoded key fallback or make it fail-fast.

**`\btoken\b` is overly broad** — the regex will redact the RHS of any `token=value` assignment, including innocuous values like `csrf_token=abc`, `next_token=xyz`, or `token_remaining=42` in diagnostic output. This reduces trace usefulness and may obscure real failures. Consider tightening to more specific patterns like `api[_\-]?token`, `auth[_\-]?token`, or `bearer[_\-]?token`, or document it as a deliberate over-redaction tradeoff.

**Same catch-all `Exception` concern** — any `Exception` subclass passes, including `TypeError` if the adapter changes its return type, or `AssertionError` from an unrelated check. This test would also "pass" if `read_file` raised because the sandbox had been killed. Consider checking for a more specific exception type or at minimum asserting on the error message containing "not found" / "does not exist".

**Catch-all `Exception` can pass vacuously** — any `Exception` subclass satisfies this assertion, including `TypeError`, `KeyError`, or `AssertionError` from an unrelated failure. Consider narrowing to a known timeout exception class from the adapter, or at minimum asserting on the exception message containing "timeout" / "deadline" to avoid masking regressions.

Hello, I would like to know what the original requirement was for this feature. In my understanding, in production scenarios, most users do not use Macs for business cluster deployment. Therefore, I want to understand the original requirement of your issue: under what circumstances is it necessary to use macOS as a host server?

**Performance: O(N² log N) sorting cascade per reload cycle** Every call to `syncLocalcacheNodeHealth` → `UpsertNode` → `updateNodeFromMetaData` (in `localcache/node_cache.go:279`) ultimately calls `updateSortedNodes` (`node_cache.go:260`), which acquires `lockSortedNodes` and calls `AllSortByIndex` — an O(N log N) full sort of the node list. For the reload path, this is called N times (once per node), yielding **O(N² log N)** total work per reload cycle. On the old code this only ran for cordon-state-changed nodes (typically 0 in steady state); this PR makes it unconditional for all nodes. For deployments under ~100 nodes this is immaterial. For 500+ nodes, consider batching: add a `UpsertNodeBatch([]*node.Node)` to `localcache` that does the field copies for all nodes in one pass and calls `updateSortedNodes` (and thus `AllSortByIndex`) once, reducing per-reload work to O(N) + O(N log N).

**Lock hold time: allocation-heavy clone work done under `s.mu`** The full merge and clone work (struct copies, `cloneStringMap` for labels, slice clones for Conditions/Images/LocalTemplates/Versions) is done while holding `s.mu`. For 1,000 nodes this could be a 10–50ms critical section during which register/heartbeat/label-update handlers are blocked. Consider whether the clones could be built outside the lock (e.g., collect only the fields needed for `toSchedulerNode` under the lock, then allocate the shallow clone and schedule-node struct after the unlock). The `defer s.mu.Unlock()` pattern is correct for panic safety but doesn't mitigate contention.

**Test gap: no test exercises the full `applyReloadResult` path** Because `newTestService` (service_reload_test.go:14) never sets `s.ready = true`, every existing test (`TestApplyReloadResult*`) and the new test `TestMergeReloadResultReturnsAllTouchedNodes` skip the localcache sync. The core behavioral fix of this PR — that `syncLocalcacheNodeHealth` is actually called for every reloaded node — is **untested** at the unit level. Consider adding a test that sets `s.ready = true` and verifies that `localcache.UpsertNode` is invoked for each node (e.g., via a spy on `syncLocalcacheNodeHealth` or a gomonkey patch).

**Performance: O(N² log N) sorting cascade per reload cycle** Every call to `syncLocalcacheNodeHealth` → `UpsertNode` → `updateNodeFromMetaData` (`localcache/node_cache.go:279`) ultimately calls `updateSortedNodes` (`node_cache.go:260`), which acquires `lockSortedNodes` and calls `AllSortByIndex` — an O(N log N) full sort. Called N times per reload cycle → **O(N² log N)** total. The old code only ran this for cordon-state-changed nodes (≈0 in steady state); this PR makes it unconditional. Not a problem for deployments under ~100 nodes. For larger scale, consider a `UpsertNodeBatch([]*node.Node)` that defers the sort to a single `AllSortByIndex` call after all field copies.

Addressed in 12cb31d: both VMDelegate and SignalMonitor now use AsyncStream.makeStream, eliminating the captured optional continuations and force unwraps.

Addressed in 12cb31d: removed cloneArtifact and call FileCloner.clone directly for the kernel and optional initrd, matching the disk path.

Addressed in 12cb31d: both HTTPServer and DataPlaneProxy close their guarded listener descriptor in deinit through the shared POSIXSocket.close helper. Listener creation and failure cleanup are also centralized.

Addressed in 12cb31d: full errors are logged to stderr, while HTTP clients receive stable safe messages for bad request bodies, invalid arguments, and internal failures. The smoke test asserts that malformed JSON returns exactly the client-safe response.

Addressed in 12cb31d: the shared POSIXSocket.writeAll overload writes directly from the reusable byte buffer with an explicit count, removing the per-packet Data allocation in both relay directions.

Addressed in 12cb31d: relay poll now has a 60-second idle timeout. Timed-out relays exit through the existing defer path, close both endpoints, and return the connection slot. The deadline is documented in the README and the end-to-end relay smoke test passes.

Addressed in 12cb31d: readiness now uses a one-shot AsyncStream from the vsock listener instead of 1 ms polling. The first connection stops further acceptance; invalid responses and read failures close the connection and fail immediately. The end-to-end lifecycle smoke test passes.