feat(sandbox,kubernetes): warm pool gRPC PoC (Milestone 1)#13
Conversation
Parent brainstorm (01) defines the layered measurement approach for evaluating Agent Sandbox warm pooling on OpenShift. Child documents cover cluster setup (02), measurements (03), and results synthesis (04). Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Add ActivateSandbox gRPC endpoint to the supervisor for claim-time identity binding of warm pool pods. The supervisor starts in an unidentified mode (no gateway connection, no identity, no OPA), listens for activation, and bootstraps fully when the gateway pushes identity and policy configuration. Key changes: - New proto/supervisor.proto with Supervisor service definition - Supervisor unidentified mode (--unidentified flag) with /readyz endpoint - ActivateSandbox handler with full bootstrap sequence - K8s driver warm pool detection and SandboxClaim flow - Cold-start fallback on activation failure - Direct JWT path (skip IssueSandboxToken for warm pool) - Integration tests for activation timing and smoke test Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesAdds Kubernetes warm-pool sandbox activation through a new Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Applied fixes from bot review comments: - Comment #3564029644: Use policy from GetSandboxConfig instead of empty default - Comment #3564029715: Require TLS for ActivateSandbox, reject plaintext fallback Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
| .serve(addr); | ||
|
|
||
| if health_check { | ||
| ready.store(true, std::sync::atomic::Ordering::Release); |
There was a problem hiding this comment.
🟡 Health readiness is signaled before the gRPC activation server has bound its socket
The readiness flag is set to true (ready.store(true) at crates/openshell-sandbox/src/lib.rs:949) before the gRPC server future is polled for the first time (which is when the socket actually binds), so a probe can report the pod as ready while the activation port is not yet accepting connections.
Impact: The gateway may attempt an ActivateSandbox call immediately after the readiness probe passes and receive a connection-refused error, causing an unnecessary cold-start fallback.
Mechanism: serve(addr) binds on first poll, not on construction
At crates/openshell-sandbox/src/lib.rs:944-950:
let server = tonic::transport::Server::builder()
.add_service(SupervisorServer::new(service))
.serve(addr); // Returns a future; socket not bound yet
if health_check {
ready.store(true, ...); // Readiness signaled HERE
}
tokio::select! { // server is first polled HERE (socket binds)The health HTTP server (spawned earlier at line 927) is already accepting connections, so a Kubernetes readiness probe hitting /readyz can succeed before the select! block even starts executing.
Prompt for agents
The readiness flag should only be set after the gRPC server has successfully bound its socket. One approach: use tonic's `Server::builder().serve_with_incoming()` with a pre-bound `TcpListener`, or use `tokio::net::TcpListener::bind(addr).await` first, then set the ready flag, then pass the listener to the server. Alternatively, move the `ready.store(true)` inside the `tokio::select!` block after the first successful poll of the server future (though this is harder to express). The simplest fix is to bind the TCP listener before the select and set ready after the bind succeeds.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let server = tonic::transport::Server::builder() | ||
| .add_service(SupervisorServer::new(service)) | ||
| .serve(addr); |
There was a problem hiding this comment.
🟨 Unauthenticated gRPC activation endpoint allows any network-reachable caller to push identity and policy
The supervisor's ActivateSandbox gRPC endpoint at crates/openshell-sandbox/src/lib.rs:944-946 binds to 0.0.0.0:9090 with no authentication or authorization. Any pod or process that can reach the supervisor's IP on port 9090 can call ActivateSandbox and push arbitrary sandbox identity, JWT token, and policy configuration. The only protection is the AtomicBool idempotency guard (crates/openshell-sandbox/src/activation.rs:54) which prevents a second activation, but a race between a malicious caller and the legitimate gateway could result in the supervisor being activated with attacker-controlled identity and policy.
Was this helpful? React with 👍 or 👎 to provide feedback.
| sandbox_name: sandbox.name.clone(), | ||
| sandbox_token, | ||
| gateway_endpoint: self.config.grpc_endpoint.clone(), | ||
| policy: Some(openshell_core::proto::sandbox::v1::SandboxPolicy::default()), |
There was a problem hiding this comment.
🟥 Warm pool sandbox can run with no policy enforcement if gateway settings fetch fails
The driver sends SandboxPolicy::default() (an empty policy with no rules) in the activation request (crates/openshell-driver-kubernetes/src/driver.rs:1035). The bootstrap_sandbox function at crates/openshell-sandbox/src/lib.rs:839-844 falls back to compiling this empty policy when fetch_settings_snapshot fails. An empty OPA policy means no network restrictions, no filesystem restrictions, and no process restrictions are enforced on the sandbox, giving the agent unrestricted access.
Was this helpful? React with 👍 or 👎 to provide feedback.
The Milestone 1 implementation used agents.x-k8s.io/v1alpha1 for the warm pool CRDs, but the Red Hat Agent Sandbox operator v0.9.0 ships extensions.agents.x-k8s.io/v1beta1. This fixes the API group, version, status field parsing (conditions instead of phase, podIPs instead of podIP), warmPoolRef format, and image matching (resolve SandboxTemplate reference instead of reading image from the pool directly). Also adds a smoke test guide and automated script for validating the end-to-end warm pool claim + ActivateSandbox flow, along with K8s manifests for deploying the unidentified supervisor warm pool. Validated on ROSA HCP 4.22.3: 3 runs averaged 1.9s (claim 1.35s + activate 0.56s), under the 2s target. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
.specify/extensions/spex/scripts/spex-ship-statusline.sh-269-271 (1)
269-271: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
render_ship()andrender_watch()use@tsvwithreaddespite the known field-misalignment risk.The comment at line 148 explicitly warns that
@tsvcausesreadto collapse consecutive tabs for empty fields, which is whyrender_flow()usesIFS='|'withjoin("|"). However,render_ship()(line 270) andrender_watch()(line 332) still use@tsvwith default IFS.When any field defaults to an empty string (e.g.,
stage_indexmissing from state JSON, orpr_numberabsent in watch mode),@tsvproduces consecutive tabs thatreadcollapses, shifting all subsequent fields. The guard checks at lines 273 and 335 won't catch this because the misaligned fields receive non-empty values from the next field.For example, if
stage_indexis absent inrender_ship(),INDEXgets"8"(fromtotal_stages),TOTALgets"smart"(fromask), and$((DISPLAY_INDEX * 9 / TOTAL))fails with a non-numeric division error.🐛 Proposed fix: apply the same pipe-delimiter pattern used in
render_flow()# render_ship() - line 269 - read -r STAGE INDEX TOTAL ASK STATUS < <( - echo "$STATE_JSON" | jq -r '[.stage // "", .stage_index // "", .total_stages // 8, .ask // "smart", .status // "running"] | `@tsv`' 2>/dev/null + IFS='|' read -r STAGE INDEX TOTAL ASK STATUS < <( + echo "$STATE_JSON" | jq -r '[.stage // "", .stage_index // "", .total_stages // 8, .ask // "smart", .status // "running"] | map(tostring) | join("|")' 2>/dev/null )# render_watch() - line 331 - read -r PR_NUMBER CI_STATUS STARTED_AT TIMEOUT TRIAGE_COUNT FIX_ATTEMPTS < <( - echo "$STATE_JSON" | jq -r '[.pr_number // "", .last_ci_status // "pending", .watch_started_at // "", .watch_timeout_minutes // 30, .triage_count // 0, .ci_fix_attempts // 0] | `@tsv`' 2>/dev/null + IFS='|' read -r PR_NUMBER CI_STATUS STARTED_AT TIMEOUT TRIAGE_COUNT FIX_ATTEMPTS < <( + echo "$STATE_JSON" | jq -r '[.pr_number // "", .last_ci_status // "pending", .watch_started_at // "", .watch_timeout_minutes // 30, .triage_count // 0, .ci_fix_attempts // 0] | map(tostring) | join("|")' 2>/dev/null )Also applies to: 331-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex/scripts/spex-ship-statusline.sh around lines 269 - 271, Update the state-field parsing in render_ship() and render_watch() to avoid jq `@tsv` with default read splitting; use the same pipe-delimited join and IFS pattern as render_flow(). Preserve the existing field order, defaults, and subsequent guard/calculation logic so empty fields remain positional and do not shift later values.Source: Linters/SAST tools
crates/openshell-sandbox/src/lib.rs-948-951 (1)
948-951: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReadiness flag is set before the gRPC server begins accepting connections.
ready.store(true, ...)executes after the server future is created but before it is polled intokio::select!. In Kubernetes, a readiness probe hitting/readyzduring this window would mark the pod ready before the activation gRPC endpoint is listening, causing activation requests to fail with connection refused.🛡️ Proposed fix: set readiness after server starts listening
- if health_check { - ready.store(true, std::sync::atomic::Ordering::Release); - info!(port = health_port, "Health endpoint /readyz is ready"); - } - - tokio::select! { - result = server => { + // Use a ready signal channel to notify when the server is listening + let (server_ready_tx, server_ready_rx) = tokio::sync::oneshot::channel(); + let server = tonic::transport::Server::builder() + .add_service(SupervisorServer::new(service)) + .serve_with_shutdown(addr, async move { + let _ = server_ready_tx.send(()); + // Keep the server running until activation completes + std::future::pending::<()>().await; + }); + + if health_check { + tokio::spawn(async move { + let _ = server_ready_rx.await; + ready.store(true, std::sync::atomic::Ordering::Release); + info!(port = health_port, "Health endpoint /readyz is ready"); + }); + } + + tokio::select! { + result = server => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-sandbox/src/lib.rs` around lines 948 - 951, Move the readiness update in the health_check startup flow so ready.store(true, Ordering::Release) runs only after the gRPC server has successfully begun listening, not immediately after creating its future and before tokio::select! polls it. Keep the /readyz log aligned with that successful-listening point.experiments/manifests/sandbox-template-unidentified.yaml-17-17 (1)
17-17: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPin image tags instead of using
:latestfor reproducible warm pool timing.Both the init container and main container use
base:latest. Since the PR reports validated warm pool startup timing of 1.9s across three runs, a drifting:latestimage could silently invalidate those measurements. Pin to a digest or specific tag for reproducibility.Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/manifests/sandbox-template-unidentified.yaml` at line 17, Replace the :latest tag for the base image in both the init container and main container with a fixed digest or immutable version tag, preserving the same image source and ensuring warm pool timing remains reproducible..specify/extensions/spex-gates/commands/speckit.spex-gates.verify.md-155-155 (1)
155-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix
grep -c ... || echo 0double-output bug.When
grep -cfinds 0 matches, it outputs"0"to stdout and returns exit code 1. The|| echo 0then also outputs"0", so the command substitution captures both, producing"0\n0"— not a valid integer for the later comparison at line 165.🐛 Proposed fix
-HAS_SCENARIOS=$(grep -c '\*\*Given\*\*' "$SPEC_FILE" 2>/dev/null || echo 0) +HAS_SCENARIOS=$(grep -c '\*\*Given\*\*' "$SPEC_FILE" 2>/dev/null) || HAS_SCENARIOS=0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex-gates/commands/speckit.spex-gates.verify.md at line 155, Update the HAS_SCENARIOS assignment in the verification logic to handle grep’s nonzero status for zero matches without emitting a second fallback value. Ensure the command substitution always produces exactly one integer, preserving the existing count for matched scenarios and returning 0 when none are found.crates/openshell-driver-kubernetes/src/activation_client.rs-75-92 (1)
75-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest does not exercise the path its name implies.
connection_to_nonexistent_host_failspassesNonefortls, soactivate_sandboxshort-circuits at the TLS guard (Lines 36-40) and returnsConnectionFailedbefore ever attempting to connect to192.0.2.1. The assertion therefore passes trivially and never covers connection/timeout behavior against an unreachable host. Provide aTlsConfig(dummy PEMs) so the connection attempt is actually made, or rename the test to reflect that it validates the missing-TLS guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-driver-kubernetes/src/activation_client.rs` around lines 75 - 92, Update the connection_to_nonexistent_host_fails test to pass a valid TlsConfig with dummy PEM values to activate_sandbox instead of None, ensuring execution reaches the unreachable-host connection path. Keep the existing ConnectionFailed or Timeout assertions unchanged..specify/extensions/spex-gates/scripts/spex-flow-state.sh-81-103 (1)
81-103: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHeredoc JSON creation doesn't escape variable values.
do_create()interpolates$branchand$spec_dirdirectly into JSON via heredoc. If--spec-diris passed with double quotes or backslashes, the resulting JSON is malformed. Sincejqis already used for updates, use it for initial creation too.🛡️ Proposed fix: use jq -n for initial JSON creation
if [ "$collab_enabled" = true ]; then - cat > "$STATE_FILE" <<EOF -{ - "mode": "flow", - "started_at": "$(date -u +%Y-%m-%dT%H:%M:%SZ)", - "feature_branch": "$branch", - "spec_dir": "$spec_dir", - "implemented": false, - "clarified": false, - "triage_spec_passed": false, - "triage_impl_passed": false -} -EOF + jq -n \ + --arg started "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --arg branch "$branch" \ + --arg dir "$spec_dir" \ + '{mode:"flow",started_at:$started,feature_branch:$branch,spec_dir:$dir,implemented:false,clarified:false,triage_spec_passed:false,triage_impl_passed:false}' \ + > "$STATE_FILE" else - cat > "$STATE_FILE" <<EOF -{ - "mode": "flow", - "started_at": "$(date -u +%Y-%m-%dT%H:%M:%SZ)", - "feature_branch": "$branch", - "spec_dir": "$spec_dir", - "implemented": false, - "clarified": false -} -EOF + jq -n \ + --arg started "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --arg branch "$branch" \ + --arg dir "$spec_dir" \ + '{mode:"flow",started_at:$started,feature_branch:$branch,spec_dir:$dir,implemented:false,clarified:false}' \ + > "$STATE_FILE" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex-gates/scripts/spex-flow-state.sh around lines 81 - 103, Update do_create() to build both initial state documents with jq -n rather than interpolating branch and spec_dir directly in heredocs. Pass the values as jq arguments so quotes, backslashes, and other special characters are JSON-escaped, while preserving the existing fields and mode-specific triage flags..specify/extensions/spex-collab/commands/speckit.spex-collab.phase-split.md-15-17 (1)
15-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShip pipeline guard is missing a
returnstatement.The bash snippet echoes "Ship mode active, skipping phase split" but never returns, so execution would continue past the guard and display the proposal anyway. Line 21 explicitly states "If ship mode is detected, return immediately."
🔧 Proposed fix
if [ "$MODE" = "ship" ] || [ "$STATUS" = "running" ]; then echo "Ship mode active, skipping phase split" + return 0 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex-collab/commands/speckit.spex-collab.phase-split.md around lines 15 - 17, Add an immediate return after the guard message in the phase-split flow for the condition checking MODE="ship" or STATUS="running". Preserve the existing echo, and ensure execution stops before displaying the proposal as required by the documented behavior.experiments/smoke-test.sh-63-65 (1)
63-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
ncandlsofto preflight checks.The script uses
nc -z(line 321) to verify port-forward readiness andlsof -ti(line 52) to clean up port-forwards, but neither tool is checked in the preflight section. Ifncis missing, the readiness loop silently fails and waits 5 seconds per run before proceeding with a possibly-unready port-forward. Iflsofis missing, stale port-forwards from previous runs are not killed, causing the new port-forward to fail on an already-bound port.🔧 Proposed fix
for tool in kubectl grpcurl; do command -v "$tool" &>/dev/null || { fail "$tool not found"; exit 1; } done +for tool in nc lsof; do + command -v "$tool" &>/dev/null || { fail "$tool not found"; exit 1; } +done pass "kubectl and grpcurl available"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/smoke-test.sh` around lines 63 - 65, Extend the preflight tool-validation loop to include nc and lsof alongside kubectl and grpcurl, so both port-forward readiness checks and cleanup prerequisites are validated before the script proceeds. Preserve the existing missing-tool failure behavior.
🧹 Nitpick comments (14)
.specify/extensions/spex/scripts/spex-detach.sh (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate shim script — consider consolidating.
This file is identical to
.specify/extensions/spex-detach/scripts/spex-detach.sh. Both are 5-line shims delegating to their respectivespex-detach.pyvia the samepython-resolve.shresolver. If the twospex-detach.pyscripts are also identical, consider symlinking or sharing a single entry point to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex/scripts/spex-detach.sh around lines 1 - 5, Consolidate the duplicate spex-detach.sh entry points by verifying whether both spex-detach.py implementations are identical, then use a single shared entry point or symlink rather than maintaining two shims. Preserve invocation arguments and resolution through python-resolve.sh..specify/extensions/spex-detach/scripts/spex-detach.py (2)
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
git()'scheckargument diverges fromsubprocess.runsemantics.Here
check=TruereturnsNoneon failure (rather than raising, assubprocess.run(check=True)does), whilecheck=Falsereturns"". ANonereturn would blow up in the common.split()/.strip()call sites ifcheck=Trueis ever used. The parameter appears unused today, so consider dropping it or renaming to avoid a future footgun.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex-detach/scripts/spex-detach.py around lines 9 - 13, Update the git() helper’s unused check parameter to avoid implying subprocess.run(check=True) semantics: remove the parameter and its conditional failure branch, while preserving the existing stdout-or-empty-string return behavior for all callers.
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit semicolon-joined statements to satisfy Ruff (E702).
Ruff flags these lines as errors (
E702, multiple statements on one line). If Ruff runs in CI, this file will fail linting. Split eachx = ...; i += ...onto separate lines.♻️ Example fix (apply to all flagged lines)
- branch = args[i + 1]; i += 2 + branch = args[i + 1] + i += 2Also applies to: 133-133, 136-136, 140-140, 242-242, 245-245, 248-248, 250-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex-detach/scripts/spex-detach.py at line 112, Split the semicolon-joined assignment and index-increment statements throughout the argument parsing and related logic, including the statements near branch, lines 133, 136, 140, 242, 245, 248, and 250, into separate statements on separate lines. Preserve the existing execution order and behavior while removing all Ruff E702 violations.Source: Linters/SAST tools
.specify/extensions/spex/scripts/spex-ship-statusline.sh (1)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
next_stepis set but never used.The
next_stepvariable is assigned in the if/elif chain (lines 173–177) but is never referenced afterward. Milestone rendering at lines 198–206 uses themile_nextarray for the running-state check, makingnext_stepdead code.♻️ Remove dead code
# Next milestone (first incomplete linear stage) - local next_step="" - if [ "$has_spec" = false ]; then next_step="specify" - elif [ "$has_plan" = false ]; then next_step="plan" - elif [ "$has_tasks" = false ]; then next_step="tasks" - elif [ "$has_impl" = false ]; then next_step="implement" - fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex/scripts/spex-ship-statusline.sh around lines 172 - 177, Remove the unused next_step variable and its if/elif assignment chain from the statusline script, leaving the existing mile_next-based milestone rendering and running-state logic unchanged.Source: Linters/SAST tools
specs/002-warm-pool-grpc-poc/contracts/supervisor.proto (1)
52-60: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider adding a timeout error code.
The
ErrorCodeenum lacks a timeout/deadline category. Activation involves potentially long-running steps (OPA compilation, gateway connection, entrypoint start). WithoutERROR_CODE_TIMEOUT, the client cannot distinguish between "activation failed" and "activation is still running but exceeded the deadline," which matters for retry decisions and cold-start fallback logic.♻️ Suggested addition
enum ErrorCode { ERROR_CODE_UNSPECIFIED = 0; ERROR_CODE_INVALID_REQUEST = 1; ERROR_CODE_POLICY_COMPILATION_FAILED = 2; ERROR_CODE_GATEWAY_UNREACHABLE = 3; ERROR_CODE_TOKEN_INVALID = 4; ERROR_CODE_ALREADY_ACTIVATED = 5; ERROR_CODE_INTERNAL = 6; + ERROR_CODE_TIMEOUT = 7; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/002-warm-pool-grpc-poc/contracts/supervisor.proto` around lines 52 - 60, Add ERROR_CODE_TIMEOUT to the ErrorCode enum as a distinct timeout/deadline category, preserving all existing numeric assignments and placing it at a new value after ERROR_CODE_INTERNAL.experiments/manifests/sandbox-template-unidentified.yaml (2)
78-82: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftPlan AppArmor and capability hardening before production.
UnconfinedAppArmor combined withSYS_ADMIN,SYS_PTRACE,NET_ADMIN,SYSLOG, andrunAsUser: 0grants near-host-level access. This is likely required for nested container sandbox functionality in the PoC, but should be tightened before any non-experiment deployment — consider a dedicated AppArmor profile and scoped capability drops where possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/manifests/sandbox-template-unidentified.yaml` around lines 78 - 82, Restrict this sandbox manifest’s privileged security settings before any non-experiment deployment: replace the Unconfined appArmorProfile with a dedicated restrictive profile, minimize the capabilities listed under capabilities.add and drop all unnecessary privileges, and avoid runAsUser: 0 by using a non-root user where nested-container functionality permits.
108-109: 🩺 Stability & Availability | 🔵 TrivialGate this manifest on ImageVolume-capable clusters
This template hardcodes the Kubernetes
imagevolume path, so it only works on K8s 1.35+ (or 1.33–1.34 withImageVolumeenabled); older clusters will reject the pod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/manifests/sandbox-template-unidentified.yaml` around lines 108 - 109, Gate the sandbox manifest template containing the image reference under an ImageVolume-capable-cluster condition. Ensure it is only rendered or applied for Kubernetes 1.35+ or clusters explicitly enabling ImageVolume, while preserving the existing image reference for supported clusters..specify/extensions/git/scripts/powershell/create-new-feature.ps1 (1)
136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
Write-Verboseto empty catch blocks for debuggability.PSScriptAnalyzer flags the empty catch blocks at lines 137 and 342. While intentionally swallowing errors (offline git fetch, missing HEAD ref), a
Write-Verbosewould aid troubleshooting without changing behavior.💡 Suggested improvement
try { git fetch --all --prune 2>$null | Out-Null -} catch { } +} catch { + Write-Verbose "git fetch failed (offline?): $_" +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/git/scripts/powershell/create-new-feature.ps1 around lines 136 - 137, Add Write-Verbose statements to the intentionally empty catch blocks in the git fetch and missing HEAD ref handling within the feature creation script, including useful context about the swallowed errors while preserving the existing behavior of ignoring those failures..specify/extensions/spex/scripts/spex-flow-state.sh (2)
108-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
jq --argfor therunningphase to avoid expression injection.
do_runningconstructs a jq expression viaprintf '.running = "%s"' "$phase". If$phasecontains a double quote, the jq expression would break and the script would exit underset -e. Usingjq --argwould handle this safely.♻️ Proposed refactor for safe jq expression construction
This requires a small modification to
update_stateto pass through extra jq arguments:update_state() { local expr="$1" + shift ensure_flow if is_flow; then local tmp tmp=$(mktemp) - jq "$expr" "$STATE_FILE" > "$tmp" && mv "$tmp" "$STATE_FILE" + jq "$@" "$expr" "$STATE_FILE" > "$tmp" && mv "$tmp" "$STATE_FILE" fi }Then update
do_running:do_running() { local phase="${1:-}" if [ "$phase" = "done" ]; then update_state '.running = ""' elif [ -n "$phase" ]; then - update_state "$(printf '.running = "%s"' "$phase")" + update_state --arg phase "$phase" '.running = $phase' fi }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex/scripts/spex-flow-state.sh around lines 108 - 115, Update update_state to accept and forward additional jq arguments, then change do_running’s non-empty phase branch to pass phase via jq --arg and use a static .running = $phase filter instead of constructing the expression with printf. Preserve the existing done and empty-phase behavior.
81-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
jq -nover heredoc for JSON construction.The
do_createfunction builds JSON via heredoc with variable interpolation, whileupdate_statecorrectly usesjq --argfor safe string passing. If$branchor$spec_direver contain double quotes or backslashes, the heredoc would produce invalid JSON. Usingjq -nwould be consistent with the rest of the script and handle escaping automatically.♻️ Proposed refactor for safe JSON construction
if [ "$collab_enabled" = true ]; then - cat > "$STATE_FILE" <<EOF -{ - "mode": "flow", - "started_at": "$(date -u +%Y-%m-%dT%H:%M:%SZ)", - "feature_branch": "$branch", - "spec_dir": "$spec_dir", - "implemented": false, - "clarified": false, - "triage_spec_passed": false, - "triage_impl_passed": false -} -EOF + jq -n \ + --arg started_at "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --arg branch "$branch" \ + --arg dir "$spec_dir" \ + '{mode:"flow", started_at:$started_at, feature_branch:$branch, spec_dir:$dir, implemented:false, clarified:false, triage_spec_passed:false, triage_impl_passed:false}' \ + > "$STATE_FILE" else - cat > "$STATE_FILE" <<EOF -{ - "mode": "flow", - "started_at": "$(date -u +%Y-%m-%dT%H:%M:%SZ)", - "feature_branch": "$branch", - "spec_dir": "$spec_dir", - "implemented": false, - "clarified": false -} -EOF + jq -n \ + --arg started_at "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --arg branch "$branch" \ + --arg dir "$spec_dir" \ + '{mode:"flow", started_at:$started_at, feature_branch:$branch, spec_dir:$dir, implemented:false, clarified:false}' \ + > "$STATE_FILE" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.specify/extensions/spex/scripts/spex-flow-state.sh around lines 81 - 103, Update the JSON construction in do_create to use jq -n with --arg for branch, spec_dir, and the UTC timestamp instead of interpolated heredocs. Preserve both state object variants, including their existing mode, flags, and triage fields, while relying on jq to escape string values safely.crates/openshell-sandbox/tests/activation_smoke.rs (1)
94-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo per-RPC timeout on
activate_sandboxcalls — test can hang indefinitely.
connect_timeouton the channel only bounds connection establishment, not the RPC itself. If the sandbox process hangs during activation, bothactivate_sandboxcalls will block forever with no timeout, hanging the test runner.⏱️ Proposed fix: wrap calls in tokio::time::timeout
let activate_start = Instant::now(); - let resp = client + let resp = tokio::time::timeout(Duration::from_secs(10), client .activate_sandbox(ActivateSandboxRequest { sandbox_id: "smoke-test-sandbox".into(), sandbox_name: "smoke-test".into(), sandbox_token: "fake-jwt-token".into(), gateway_endpoint: "https://gateway.invalid:8443".into(), policy: Some(SandboxPolicy::default()), }) - .await - .expect("gRPC call should not return transport error") - .into_inner(); + .await) + .expect("activate_sandbox should complete within 10s") + .expect("gRPC call should not return transport error") + .into_inner();Apply the same pattern to the second
activate_sandboxcall (lines 128-138).Also applies to: 128-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-sandbox/tests/activation_smoke.rs` around lines 94 - 105, Wrap both `activate_sandbox` calls in `crates/openshell-sandbox/tests/activation_smoke.rs` with `tokio::time::timeout` using an appropriate per-RPC duration. Handle the timeout result while preserving the existing transport-error expectation and response extraction for successful calls.experiments/smoke-test.sh (1)
314-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
POD_IPvariable.
POD_IPis extracted from the claim status but never referenced — the activation call useslocalhost:${LOCAL_PORT}via port-forward. Shellcheck confirms this (SC2034).♻️ Proposed fix
POD_NAME=$(kubectl -n "$NAMESPACE" get sandboxclaim "$CLAIM_NAME" \ -o jsonpath='{.status.sandbox.name}') -POD_IP=$(kubectl -n "$NAMESPACE" get sandboxclaim "$CLAIM_NAME" \ - -o jsonpath='{.status.sandbox.podIPs[0]}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/smoke-test.sh` around lines 314 - 315, Remove the unused POD_IP assignment from the smoke-test flow, including its kubectl lookup, while preserving the existing port-forward activation path using localhost:${LOCAL_PORT}.crates/openshell-driver-kubernetes/src/warm_pool.rs (2)
72-126: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider batching template fetches in
find_matching_poolfor production readiness.Each pool with ready replicas triggers a separate
tmpl_api.get()call (line 104), creating an N+1 query pattern. For a PoC with a handful of pools this is fine, but as pool count grows this will linearly increase API server load and latency. Consider collecting all template names first and batching via a singlelistcall with a field selector, or caching template images.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-driver-kubernetes/src/warm_pool.rs` around lines 72 - 126, Update find_matching_pool to avoid one tmpl_api.get call per ready pool: collect the referenced template names, fetch templates in a batched or cached manner, and then match each pool’s template image against image. Preserve skipping unready or malformed pools, warning on fetch failures, and returning the first matching pool.
155-233: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd exponential backoff or distinguish NotFound in
wait_for_claim_readypolling.The fixed 200ms poll interval (line 163) results in up to 75 API calls over a 15s timeout. Additionally, any API error — including a NotFound for a non-existent claim — is logged and retried until timeout (lines 226-228), wasting the full timeout budget. Consider: (1) using exponential backoff with jitter to reduce API server pressure, and (2) checking for
kube::Error::ApiwithNotFoundcode to fail fast instead of polling until timeout.♻️ Proposed improvement for fast-fail on NotFound
Err(e) => { + if let kube::Error::Api(api_err) = &e { + if api_err.code == 404 { + return Err(WarmPoolError::ClaimFailed(format!( + "SandboxClaim {claim_name} not found: {e}" + ))); + } + } warn!(claim = %claim_name, error = %e, "Error polling SandboxClaim"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-driver-kubernetes/src/warm_pool.rs` around lines 155 - 233, Update wait_for_claim_ready to distinguish Kubernetes NotFound errors from transient polling failures: when api.get returns kube::Error::Api with a 404 status, return the appropriate claim-not-found error immediately instead of retrying. Replace the fixed poll_interval sleep with bounded exponential backoff and jitter, while preserving the timeout deadline and existing handling for successful, failed, and other transient responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.specify/extensions/git/scripts/powershell/create-new-feature.ps1:
- Around line 386-403: Update the PowerShell create-new-feature flow to match
the bash script by creating the FEATURE_DIR and SPEC_FILE artifacts and
including SPEC_FILE in both the JSON object and human-readable output. Remove
HAS_GIT from the emitted output so the PowerShell contract matches the bash
script, while preserving the existing DRY_RUN behavior.
In @.specify/extensions/spex-collab/commands/speckit.spex-collab.reviewers.md:
- Line 97: Update the template reference in the speckit command instructions to
use the correct .specify/extensions/spex-collab/templates/reviewers-template.md
path, preserving the existing structural-skeleton guidance and consistency with
the other .specify/ references.
In @.specify/extensions/spex/commands/speckit.spex.submit.md:
- Around line 67-73: Fix DEFAULT_BRANCH initialization in the submit workflow so
failure or empty output from git symbolic-ref falls back to "main" rather than
allowing the sed pipeline to succeed with an empty value. Preserve the
CURRENT_BRANCH comparison and ensure DEFAULT_BRANCH remains valid for both the
default-branch guard and the later brainstorm exclusion diff.
In @.specify/extensions/spex/scripts/spex-detach.py:
- Around line 90-93: Update validate_path_component to reject absolute path
components in addition to values containing "..", using the platform-appropriate
path check before os.path.join is used for project or feature. Preserve the
existing stderr error and exit behavior for both traversal and absolute-path
violations.
- Around line 192-228: Update the git apply failure path in the surrounding
branch-creation flow so cleanup runs before terminating, or raise an exception
caught by the existing except Exception handler instead of calling sys.exit(1)
directly. Preserve the existing error JSON and exit status while ensuring
original_branch is restored and pr_branch is deleted when git apply fails.
In @.specify/extensions/spex/scripts/spex-ship-state.py:
- Around line 69-97: Stop verify_stage_artifacts from selecting a feature
directory by filename or newest modification time. Thread the orchestrator’s
resolved feature directory from check-prerequisites.sh --json --paths-only into
verify_stage_artifacts (or expose it through a --feature-dir argument), and use
that explicit path for artifact checks; retain filename-based discovery only as
an intentional, safe fallback if required.
In `@crates/openshell-driver-kubernetes/src/activation_client.rs`:
- Line 41: Update the endpoint construction around endpoint_uri to bracket IPv6
pod addresses before appending ACTIVATION_PORT, while preserving valid
formatting for IPv4 addresses. Ensure IPv6 values produce an authority like
https://[address]:port.
- Around line 49-54: Update the TLS configuration in activate_sandbox() so the
endpoint connecting to the pod IP validates against the supervisor certificate:
configure the pod IP as a server IP SAN when generating or loading the
certificate, or set tls_config.domain_name(...) to a DNS SAN covered by that
certificate. Preserve the existing CA certificate and client identity
configuration.
In `@crates/openshell-driver-kubernetes/src/driver.rs`:
- Around line 909-941: Update read_activation_tls and its try_warm_pool caller
so a configured but missing or incomplete TLS secret fails closed instead of
returning a usable plaintext activation path. Propagate the failure or return
None from try_warm_pool to trigger cold start, while preserving the existing
behavior when no client_tls_secret_name is configured and ensuring sandbox_token
is never sent over a downgraded plain channel.
- Around line 985-1065: Delete the successfully created warm-pool claim before
returning None from the wait_for_claim_ready and activate_sandbox failure
branches. Reuse the existing Kubernetes client, namespace, and claim_name with
the warm-pool claim deletion helper, while preserving the current warning logs
and cold-start fallback behavior.
In `@crates/openshell-sandbox/src/health.rs`:
- Around line 36-37: Update the accept loop in the health server around
listener.accept so transient accept errors are logged and the loop continues
instead of being propagated with ?. Preserve normal stream handling and only
terminate for errors that cannot be recovered from if the surrounding design
explicitly distinguishes them.
In `@crates/openshell-sandbox/src/lib.rs`:
- Around line 944-946: Update the activation gRPC listener construction around
SupervisorServer::new and Server::builder to configure TLS with .tls_config()
before serving, using the project’s existing certificate and private-key
configuration. Ensure the resulting serve path remains compatible with
SupervisorServer and fails clearly if TLS configuration cannot be applied.
- Around line 953-965: Update the gRPC server branch in the tokio::select! block
to return a non-zero exit status when server returns an error, while preserving
the existing warning log. Keep the activation_rx success path returning the
normal successful status.
In `@crates/openshell-sandbox/tests/activation_smoke.rs`:
- Around line 62-77: Wrap the spawned child in a Drop guard immediately after
the Command chain in the activation smoke test, ensuring the guard kills and
waits for the process when assertions unwind. Remove the redundant manual
cleanup at the test’s end, while preserving the existing child process usage
throughout the test.
---
Minor comments:
In @.specify/extensions/spex-collab/commands/speckit.spex-collab.phase-split.md:
- Around line 15-17: Add an immediate return after the guard message in the
phase-split flow for the condition checking MODE="ship" or STATUS="running".
Preserve the existing echo, and ensure execution stops before displaying the
proposal as required by the documented behavior.
In @.specify/extensions/spex-gates/commands/speckit.spex-gates.verify.md:
- Line 155: Update the HAS_SCENARIOS assignment in the verification logic to
handle grep’s nonzero status for zero matches without emitting a second fallback
value. Ensure the command substitution always produces exactly one integer,
preserving the existing count for matched scenarios and returning 0 when none
are found.
In @.specify/extensions/spex-gates/scripts/spex-flow-state.sh:
- Around line 81-103: Update do_create() to build both initial state documents
with jq -n rather than interpolating branch and spec_dir directly in heredocs.
Pass the values as jq arguments so quotes, backslashes, and other special
characters are JSON-escaped, while preserving the existing fields and
mode-specific triage flags.
In @.specify/extensions/spex/scripts/spex-ship-statusline.sh:
- Around line 269-271: Update the state-field parsing in render_ship() and
render_watch() to avoid jq `@tsv` with default read splitting; use the same
pipe-delimited join and IFS pattern as render_flow(). Preserve the existing
field order, defaults, and subsequent guard/calculation logic so empty fields
remain positional and do not shift later values.
In `@crates/openshell-driver-kubernetes/src/activation_client.rs`:
- Around line 75-92: Update the connection_to_nonexistent_host_fails test to
pass a valid TlsConfig with dummy PEM values to activate_sandbox instead of
None, ensuring execution reaches the unreachable-host connection path. Keep the
existing ConnectionFailed or Timeout assertions unchanged.
In `@crates/openshell-sandbox/src/lib.rs`:
- Around line 948-951: Move the readiness update in the health_check startup
flow so ready.store(true, Ordering::Release) runs only after the gRPC server has
successfully begun listening, not immediately after creating its future and
before tokio::select! polls it. Keep the /readyz log aligned with that
successful-listening point.
In `@experiments/manifests/sandbox-template-unidentified.yaml`:
- Line 17: Replace the :latest tag for the base image in both the init container
and main container with a fixed digest or immutable version tag, preserving the
same image source and ensuring warm pool timing remains reproducible.
In `@experiments/smoke-test.sh`:
- Around line 63-65: Extend the preflight tool-validation loop to include nc and
lsof alongside kubectl and grpcurl, so both port-forward readiness checks and
cleanup prerequisites are validated before the script proceeds. Preserve the
existing missing-tool failure behavior.
---
Nitpick comments:
In @.specify/extensions/git/scripts/powershell/create-new-feature.ps1:
- Around line 136-137: Add Write-Verbose statements to the intentionally empty
catch blocks in the git fetch and missing HEAD ref handling within the feature
creation script, including useful context about the swallowed errors while
preserving the existing behavior of ignoring those failures.
In @.specify/extensions/spex-detach/scripts/spex-detach.py:
- Around line 9-13: Update the git() helper’s unused check parameter to avoid
implying subprocess.run(check=True) semantics: remove the parameter and its
conditional failure branch, while preserving the existing stdout-or-empty-string
return behavior for all callers.
- Line 112: Split the semicolon-joined assignment and index-increment statements
throughout the argument parsing and related logic, including the statements near
branch, lines 133, 136, 140, 242, 245, 248, and 250, into separate statements on
separate lines. Preserve the existing execution order and behavior while
removing all Ruff E702 violations.
In @.specify/extensions/spex/scripts/spex-detach.sh:
- Around line 1-5: Consolidate the duplicate spex-detach.sh entry points by
verifying whether both spex-detach.py implementations are identical, then use a
single shared entry point or symlink rather than maintaining two shims. Preserve
invocation arguments and resolution through python-resolve.sh.
In @.specify/extensions/spex/scripts/spex-flow-state.sh:
- Around line 108-115: Update update_state to accept and forward additional jq
arguments, then change do_running’s non-empty phase branch to pass phase via jq
--arg and use a static .running = $phase filter instead of constructing the
expression with printf. Preserve the existing done and empty-phase behavior.
- Around line 81-103: Update the JSON construction in do_create to use jq -n
with --arg for branch, spec_dir, and the UTC timestamp instead of interpolated
heredocs. Preserve both state object variants, including their existing mode,
flags, and triage fields, while relying on jq to escape string values safely.
In @.specify/extensions/spex/scripts/spex-ship-statusline.sh:
- Around line 172-177: Remove the unused next_step variable and its if/elif
assignment chain from the statusline script, leaving the existing
mile_next-based milestone rendering and running-state logic unchanged.
In `@crates/openshell-driver-kubernetes/src/warm_pool.rs`:
- Around line 72-126: Update find_matching_pool to avoid one tmpl_api.get call
per ready pool: collect the referenced template names, fetch templates in a
batched or cached manner, and then match each pool’s template image against
image. Preserve skipping unready or malformed pools, warning on fetch failures,
and returning the first matching pool.
- Around line 155-233: Update wait_for_claim_ready to distinguish Kubernetes
NotFound errors from transient polling failures: when api.get returns
kube::Error::Api with a 404 status, return the appropriate claim-not-found error
immediately instead of retrying. Replace the fixed poll_interval sleep with
bounded exponential backoff and jitter, while preserving the timeout deadline
and existing handling for successful, failed, and other transient responses.
In `@crates/openshell-sandbox/tests/activation_smoke.rs`:
- Around line 94-105: Wrap both `activate_sandbox` calls in
`crates/openshell-sandbox/tests/activation_smoke.rs` with `tokio::time::timeout`
using an appropriate per-RPC duration. Handle the timeout result while
preserving the existing transport-error expectation and response extraction for
successful calls.
In `@experiments/manifests/sandbox-template-unidentified.yaml`:
- Around line 78-82: Restrict this sandbox manifest’s privileged security
settings before any non-experiment deployment: replace the Unconfined
appArmorProfile with a dedicated restrictive profile, minimize the capabilities
listed under capabilities.add and drop all unnecessary privileges, and avoid
runAsUser: 0 by using a non-root user where nested-container functionality
permits.
- Around line 108-109: Gate the sandbox manifest template containing the image
reference under an ImageVolume-capable-cluster condition. Ensure it is only
rendered or applied for Kubernetes 1.35+ or clusters explicitly enabling
ImageVolume, while preserving the existing image reference for supported
clusters.
In `@experiments/smoke-test.sh`:
- Around line 314-315: Remove the unused POD_IP assignment from the smoke-test
flow, including its kubectl lookup, while preserving the existing port-forward
activation path using localhost:${LOCAL_PORT}.
In `@specs/002-warm-pool-grpc-poc/contracts/supervisor.proto`:
- Around line 52-60: Add ERROR_CODE_TIMEOUT to the ErrorCode enum as a distinct
timeout/deadline category, preserving all existing numeric assignments and
placing it at a new value after ERROR_CODE_INTERNAL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 585113c5-54d0-4d9e-9024-a41244983105
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (132)
.claude/agent-memory/arch-doc-writer/MEMORY.md.specify/.pr-triage-state.json.specify/.spex-state.specify/extensions.yml.specify/extensions/.registry.specify/extensions/git/README.md.specify/extensions/git/commands/speckit.git.commit.md.specify/extensions/git/commands/speckit.git.feature.md.specify/extensions/git/commands/speckit.git.initialize.md.specify/extensions/git/commands/speckit.git.remote.md.specify/extensions/git/commands/speckit.git.validate.md.specify/extensions/git/config-template.yml.specify/extensions/git/extension.yml.specify/extensions/git/git-config.yml.specify/extensions/git/scripts/bash/auto-commit.sh.specify/extensions/git/scripts/bash/create-new-feature.sh.specify/extensions/git/scripts/bash/git-common.sh.specify/extensions/git/scripts/bash/initialize-repo.sh.specify/extensions/git/scripts/powershell/auto-commit.ps1.specify/extensions/git/scripts/powershell/create-new-feature.ps1.specify/extensions/git/scripts/powershell/git-common.ps1.specify/extensions/git/scripts/powershell/initialize-repo.ps1.specify/extensions/spex-collab/commands/speckit.spex-collab.phase-manager.md.specify/extensions/spex-collab/commands/speckit.spex-collab.phase-split.md.specify/extensions/spex-collab/commands/speckit.spex-collab.reconcile.md.specify/extensions/spex-collab/commands/speckit.spex-collab.reviewers.md.specify/extensions/spex-collab/commands/speckit.spex-collab.revise.md.specify/extensions/spex-collab/commands/speckit.spex-collab.triage.md.specify/extensions/spex-collab/config-template.yml.specify/extensions/spex-collab/extension.yml.specify/extensions/spex-collab/scripts/sanitize-gh-json.py.specify/extensions/spex-collab/scripts/spex-flow-state.sh.specify/extensions/spex-collab/scripts/spex-triage-state.sh.specify/extensions/spex-collab/templates/reviewers-template.md.specify/extensions/spex-deep-review/commands/speckit.spex-deep-review.run.md.specify/extensions/spex-deep-review/config-template.yml.specify/extensions/spex-deep-review/extension.yml.specify/extensions/spex-deep-review/scripts/spex-flow-state.sh.specify/extensions/spex-detach/commands/speckit.spex-detach.detach.md.specify/extensions/spex-detach/config-template.yml.specify/extensions/spex-detach/extension.yml.specify/extensions/spex-detach/scripts/spex-detach.py.specify/extensions/spex-detach/scripts/spex-detach.sh.specify/extensions/spex-gates/commands/speckit.spex-gates.review-code.md.specify/extensions/spex-gates/commands/speckit.spex-gates.review-plan.md.specify/extensions/spex-gates/commands/speckit.spex-gates.review-spec.md.specify/extensions/spex-gates/commands/speckit.spex-gates.stamp.md.specify/extensions/spex-gates/commands/speckit.spex-gates.verify.md.specify/extensions/spex-gates/extension.yml.specify/extensions/spex-gates/scripts/spex-closeout-gate.sh.specify/extensions/spex-gates/scripts/spex-flow-state.sh.specify/extensions/spex-teams/commands/speckit.spex-teams.implement.md.specify/extensions/spex-teams/commands/speckit.spex-teams.orchestrate.md.specify/extensions/spex-teams/commands/speckit.spex-teams.research.md.specify/extensions/spex-teams/config-template.yml.specify/extensions/spex-teams/extension.yml.specify/extensions/spex-worktrees/commands/speckit.spex-worktrees.manage.md.specify/extensions/spex-worktrees/extension.yml.specify/extensions/spex/commands/speckit.spex.brainstorm.md.specify/extensions/spex/commands/speckit.spex.clear.md.specify/extensions/spex/commands/speckit.spex.evolve.md.specify/extensions/spex/commands/speckit.spex.extensions.md.specify/extensions/spex/commands/speckit.spex.finish.md.specify/extensions/spex/commands/speckit.spex.flow-state.md.specify/extensions/spex/commands/speckit.spex.help.md.specify/extensions/spex/commands/speckit.spex.ship.md.specify/extensions/spex/commands/speckit.spex.smoke-test.md.specify/extensions/spex/commands/speckit.spex.spec-kit.md.specify/extensions/spex/commands/speckit.spex.spec-refactoring.md.specify/extensions/spex/commands/speckit.spex.submit.md.specify/extensions/spex/commands/speckit.spex.using-superpowers.md.specify/extensions/spex/extension.yml.specify/extensions/spex/scripts/spex-detach.py.specify/extensions/spex/scripts/spex-detach.sh.specify/extensions/spex/scripts/spex-finish-context.sh.specify/extensions/spex/scripts/spex-flow-state.sh.specify/extensions/spex/scripts/spex-ship-state.py.specify/extensions/spex/scripts/spex-ship-state.sh.specify/extensions/spex/scripts/spex-ship-statusline.sh.specify/extensions/spex/scripts/spex-worktree-cwd.sh.specify/feature.json.specify/init-options.json.specify/integration.json.specify/integrations/claude.manifest.json.specify/integrations/opencode.manifest.json.specify/integrations/speckit.manifest.json.specify/scripts/bash/check-prerequisites.sh.specify/scripts/bash/common.sh.specify/scripts/bash/create-new-feature.sh.specify/scripts/bash/setup-plan.sh.specify/templates/checklist-template.md.specify/templates/constitution-template.md.specify/templates/plan-template.md.specify/templates/spec-template.md.specify/templates/tasks-template.md.specify/workflows/runs/a931d65a/inputs.json.specify/workflows/runs/a931d65a/log.jsonl.specify/workflows/runs/a931d65a/state.json.specify/workflows/runs/a931d65a/workflow.yml.specify/workflows/speckit/workflow.yml.specify/workflows/workflow-registry.jsonarchitecture/compute-runtimes.mdarchitecture/sandbox.mdbrainstorm/06-warm-pool-grpc-poc.mdbrainstorm/07-warm-pool-sandbox-profile.mdcrates/openshell-core/src/grpc_client.rscrates/openshell-core/src/proto/mod.rscrates/openshell-driver-kubernetes/src/activation_client.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-driver-kubernetes/src/lib.rscrates/openshell-driver-kubernetes/src/warm_pool.rscrates/openshell-sandbox/Cargo.tomlcrates/openshell-sandbox/src/activation.rscrates/openshell-sandbox/src/health.rscrates/openshell-sandbox/src/lib.rscrates/openshell-sandbox/src/main.rscrates/openshell-sandbox/tests/activation_smoke.rscrates/openshell-sandbox/tests/activation_timing.rsexperiments/SMOKE-TEST.mdexperiments/manifests/sandbox-template-unidentified.yamlexperiments/manifests/warm-pool-unidentified.yamlexperiments/smoke-test.shproto/supervisor.protospecs/002-warm-pool-grpc-poc/REVIEW-CODE.mdspecs/002-warm-pool-grpc-poc/checklists/requirements.mdspecs/002-warm-pool-grpc-poc/contracts/supervisor.protospecs/002-warm-pool-grpc-poc/data-model.mdspecs/002-warm-pool-grpc-poc/plan.mdspecs/002-warm-pool-grpc-poc/research.mdspecs/002-warm-pool-grpc-poc/review-findings.mdspecs/002-warm-pool-grpc-poc/spec.mdspecs/002-warm-pool-grpc-poc/tasks.md
| if ($Json) { | ||
| $obj = [PSCustomObject]@{ | ||
| BRANCH_NAME = $branchName | ||
| FEATURE_NUM = $featureNum | ||
| HAS_GIT = $hasGit | ||
| } | ||
| if ($DryRun) { | ||
| $obj | Add-Member -NotePropertyName 'DRY_RUN' -NotePropertyValue $true | ||
| } | ||
| $obj | ConvertTo-Json -Compress | ||
| } else { | ||
| Write-Output "BRANCH_NAME: $branchName" | ||
| Write-Output "FEATURE_NUM: $featureNum" | ||
| Write-Output "HAS_GIT: $hasGit" | ||
| if (-not $DryRun) { | ||
| Write-Output "SPECIFY_FEATURE environment variable set to: $branchName" | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for consumers that parse create-new-feature output
rg -nP 'SPEC_FILE|BRANCH_NAME|FEATURE_NUM|HAS_GIT' --type=sh -g '!**/create-new-feature*' -g '!**/*.ps1'Repository: rhuss/OpenShell
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the two scripts and inspect their relevant sections.
git ls-files '.specify/**/create-new-feature.*' '.specify/**/create-new-feature*' | sed -n '1,20p'
echo '--- PowerShell outline ---'
ast-grep outline .specify/extensions/git/scripts/powershell/create-new-feature.ps1 | sed -n '1,220p'
echo '--- Bash outline ---'
ast-grep outline .specify/scripts/bash/create-new-feature.sh | sed -n '1,220p'
echo '--- PowerShell relevant lines ---'
ps_file=.specify/extensions/git/scripts/powershell/create-new-feature.ps1
ps_lines=$(wc -l < "$ps_file")
start=$(( ps_lines > 320 ? 320 : 1 ))
end=$(( ps_lines < 460 ? ps_lines : 460 ))
sed -n "${start},${end}p" "$ps_file" | cat -n
echo '--- Bash relevant lines ---'
sh_file=.specify/scripts/bash/create-new-feature.sh
sh_lines=$(wc -l < "$sh_file")
start=$(( sh_lines > 1 ? 1 : 1 ))
end=$(( sh_lines < 260 ? sh_lines : 260 ))
sed -n "${start},${end}p" "$sh_file" | cat -n
echo '--- Search for SPEC_FILE / HAS_GIT consumers ---'
rg -n --hidden --glob '!.git' 'SPEC_FILE|HAS_GIT|FEATURE_NUM|BRANCH_NAME' .Repository: rhuss/OpenShell
Length of output: 35387
Align PowerShell output and side effects with the bash script
.specify/extensions/git/scripts/powershell/create-new-feature.ps1 still skips FEATURE_DIR/SPEC_FILE creation and never emits SPEC_FILE, while .specify/extensions/git/scripts/bash/create-new-feature.sh does both. That leaves Windows behavior and the output contract inconsistent; HAS_GIT is extra here, but SPEC_FILE is the missing field that callers expect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.specify/extensions/git/scripts/powershell/create-new-feature.ps1 around
lines 386 - 403, Update the PowerShell create-new-feature flow to match the bash
script by creating the FEATURE_DIR and SPEC_FILE artifacts and including
SPEC_FILE in both the JSON object and human-readable output. Remove HAS_GIT from
the emitted output so the PowerShell contract matches the bash script, while
preserving the existing DRY_RUN behavior.
|
|
||
| ## Compose REVIEWERS.md | ||
|
|
||
| Read the template from `spex/extensions/spex-collab/templates/reviewers-template.md` for the structural skeleton. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix incorrect template path — missing .specify/ prefix.
Line 97 references spex/extensions/spex-collab/templates/reviewers-template.md, but the actual template lives at .specify/extensions/spex-collab/templates/reviewers-template.md. This is inconsistent with other paths in the same file (lines 29, 41 use .specify/). An agent following this command would fail to find the template.
🔧 Proposed fix
-Read the template from `spex/extensions/spex-collab/templates/reviewers-template.md` for the structural skeleton.
+Read the template from `.specify/extensions/spex-collab/templates/reviewers-template.md` for the structural skeleton.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Read the template from `spex/extensions/spex-collab/templates/reviewers-template.md` for the structural skeleton. | |
| Read the template from `.specify/extensions/spex-collab/templates/reviewers-template.md` for the structural skeleton. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.specify/extensions/spex-collab/commands/speckit.spex-collab.reviewers.md at
line 97, Update the template reference in the speckit command instructions to
use the correct .specify/extensions/spex-collab/templates/reviewers-template.md
path, preserving the existing structural-skeleton guidance and consistency with
the other .specify/ references.
| ```bash | ||
| DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@' || echo "main") | ||
| CURRENT_BRANCH=$(git branch --show-current) | ||
| if [ "$CURRENT_BRANCH" = "$DEFAULT_BRANCH" ]; then | ||
| echo "ERROR: Cannot submit from the default branch. Switch to a feature branch first." | ||
| # STOP | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DEFAULT_BRANCH fallback never triggers due to pipeline precedence.
sed always exits 0 even on empty input, so || echo "main" never fires when git symbolic-ref fails (e.g., no remote HEAD configured). This leaves DEFAULT_BRANCH empty, causing the default-branch guard on line 70 to silently pass and allowing PR submission from the default branch — the exact scenario the check is meant to prevent. An empty DEFAULT_BRANCH also breaks the brainstorm exclusion diff on line 316 (git diff ""..."$BRANCH").
🔧 Proposed fix
-DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@' || echo "main")
+DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@')
+DEFAULT_BRANCH=${DEFAULT_BRANCH:-main}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@' || echo "main") | |
| CURRENT_BRANCH=$(git branch --show-current) | |
| if [ "$CURRENT_BRANCH" = "$DEFAULT_BRANCH" ]; then | |
| echo "ERROR: Cannot submit from the default branch. Switch to a feature branch first." | |
| # STOP | |
| fi | |
| DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@') | |
| DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} | |
| CURRENT_BRANCH=$(git branch --show-current) | |
| if [ "$CURRENT_BRANCH" = "$DEFAULT_BRANCH" ]; then | |
| echo "ERROR: Cannot submit from the default branch. Switch to a feature branch first." | |
| # STOP | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.specify/extensions/spex/commands/speckit.spex.submit.md around lines 67 -
73, Fix DEFAULT_BRANCH initialization in the submit workflow so failure or empty
output from git symbolic-ref falls back to "main" rather than allowing the sed
pipeline to succeed with an empty value. Preserve the CURRENT_BRANCH comparison
and ensure DEFAULT_BRANCH remains valid for both the default-branch guard and
the later brainstorm exclusion diff.
| def validate_path_component(name, value): | ||
| if ".." in value: | ||
| print("ERROR: {} contains '..' path traversal".format(name), file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
validate_path_component doesn't block absolute paths.
Only .. is rejected. Since os.path.join(target, project, feature) (line 273) discards preceding components when a later segment is absolute (Python os.path.join semantics), a --project or --feature value like /etc would redirect shutil.copytree writes outside target, bypassing the archive sandbox.
🛡️ Proposed fix
def validate_path_component(name, value):
if ".." in value:
print("ERROR: {} contains '..' path traversal".format(name), file=sys.stderr)
sys.exit(1)
+ if os.path.isabs(value):
+ print("ERROR: {} must not be an absolute path".format(name), file=sys.stderr)
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_path_component(name, value): | |
| if ".." in value: | |
| print("ERROR: {} contains '..' path traversal".format(name), file=sys.stderr) | |
| sys.exit(1) | |
| def validate_path_component(name, value): | |
| if ".." in value: | |
| print("ERROR: {} contains '..' path traversal".format(name), file=sys.stderr) | |
| sys.exit(1) | |
| if os.path.isabs(value): | |
| print("ERROR: {} must not be an absolute path".format(name), file=sys.stderr) | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.specify/extensions/spex/scripts/spex-detach.py around lines 90 - 93, Update
validate_path_component to reject absolute path components in addition to values
containing "..", using the platform-appropriate path check before os.path.join
is used for project or feature. Preserve the existing stderr error and exit
behavior for both traversal and absolute-path violations.
| try: | ||
| r = subprocess.run(["git", "checkout", "-b", pr_branch, merge_base, "--quiet"], capture_output=True, text=True) | ||
| if r.returncode != 0: | ||
| json.dump({"error": "Failed to create PR branch"}, sys.stderr) | ||
| print(file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| p = subprocess.run(["git", "apply", "--index"], input=diff_output, capture_output=True) | ||
| if p.returncode != 0: | ||
| json.dump({"error": "Failed to apply filtered diff"}, sys.stderr) | ||
| print(file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| files_changed = len(git("diff", "--cached", "--name-only").split("\n")) | ||
|
|
||
| log_cmd = ["git", "log", "--format=%s", "{}..{}".format(merge_base, original_branch), "--", "."] + pathspec_excludes | ||
| r = subprocess.run(log_cmd, capture_output=True, text=True) | ||
| commit_subject = r.stdout.strip().split("\n")[0] if r.returncode == 0 and r.stdout.strip() else "" | ||
| if not commit_subject: | ||
| commit_subject = "feat: {}".format(original_branch.replace("-", " ").replace("_", " ")) | ||
|
|
||
| subprocess.run(["git", "commit", "-m", commit_subject, "--quiet"], capture_output=True) | ||
| commit_sha = git("rev-parse", "HEAD") | ||
|
|
||
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True) | ||
|
|
||
| print(json.dumps({ | ||
| "pr_branch": pr_branch, | ||
| "merge_base": merge_base, | ||
| "commit": commit_sha, | ||
| "files_changed": files_changed, | ||
| "empty": False, | ||
| })) | ||
| except Exception: | ||
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True, text=True) | ||
| git("branch", "-D", pr_branch) | ||
| raise |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Diff-apply failure leaves repo checked out on the half-built PR branch.
When git apply --index fails (line 200), the code calls sys.exit(1) directly instead of going through the except Exception cleanup block. sys.exit() raises SystemExit, which is not caught by except Exception, so the checkout back to original_branch and the pr_branch cleanup (lines 226-227) never run for this specific failure. The caller is left on a broken, half-applied pr_branch instead of their original branch.
🐛 Proposed fix
p = subprocess.run(["git", "apply", "--index"], input=diff_output, capture_output=True)
if p.returncode != 0:
json.dump({"error": "Failed to apply filtered diff"}, sys.stderr)
print(file=sys.stderr)
+ subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True)
+ git("branch", "-D", pr_branch)
sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| r = subprocess.run(["git", "checkout", "-b", pr_branch, merge_base, "--quiet"], capture_output=True, text=True) | |
| if r.returncode != 0: | |
| json.dump({"error": "Failed to create PR branch"}, sys.stderr) | |
| print(file=sys.stderr) | |
| sys.exit(1) | |
| p = subprocess.run(["git", "apply", "--index"], input=diff_output, capture_output=True) | |
| if p.returncode != 0: | |
| json.dump({"error": "Failed to apply filtered diff"}, sys.stderr) | |
| print(file=sys.stderr) | |
| sys.exit(1) | |
| files_changed = len(git("diff", "--cached", "--name-only").split("\n")) | |
| log_cmd = ["git", "log", "--format=%s", "{}..{}".format(merge_base, original_branch), "--", "."] + pathspec_excludes | |
| r = subprocess.run(log_cmd, capture_output=True, text=True) | |
| commit_subject = r.stdout.strip().split("\n")[0] if r.returncode == 0 and r.stdout.strip() else "" | |
| if not commit_subject: | |
| commit_subject = "feat: {}".format(original_branch.replace("-", " ").replace("_", " ")) | |
| subprocess.run(["git", "commit", "-m", commit_subject, "--quiet"], capture_output=True) | |
| commit_sha = git("rev-parse", "HEAD") | |
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True) | |
| print(json.dumps({ | |
| "pr_branch": pr_branch, | |
| "merge_base": merge_base, | |
| "commit": commit_sha, | |
| "files_changed": files_changed, | |
| "empty": False, | |
| })) | |
| except Exception: | |
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True, text=True) | |
| git("branch", "-D", pr_branch) | |
| raise | |
| try: | |
| r = subprocess.run(["git", "checkout", "-b", pr_branch, merge_base, "--quiet"], capture_output=True, text=True) | |
| if r.returncode != 0: | |
| json.dump({"error": "Failed to create PR branch"}, sys.stderr) | |
| print(file=sys.stderr) | |
| sys.exit(1) | |
| p = subprocess.run(["git", "apply", "--index"], input=diff_output, capture_output=True) | |
| if p.returncode != 0: | |
| json.dump({"error": "Failed to apply filtered diff"}, sys.stderr) | |
| print(file=sys.stderr) | |
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True) | |
| git("branch", "-D", pr_branch) | |
| sys.exit(1) | |
| files_changed = len(git("diff", "--cached", "--name-only").split("\n")) | |
| log_cmd = ["git", "log", "--format=%s", "{}..{}".format(merge_base, original_branch), "--", "."] + pathspec_excludes | |
| r = subprocess.run(log_cmd, capture_output=True, text=True) | |
| commit_subject = r.stdout.strip().split("\n")[0] if r.returncode == 0 and r.stdout.strip() else "" | |
| if not commit_subject: | |
| commit_subject = "feat: {}".format(original_branch.replace("-", " ").replace("_", " ")) | |
| subprocess.run(["git", "commit", "-m", commit_subject, "--quiet"], capture_output=True) | |
| commit_sha = git("rev-parse", "HEAD") | |
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True) | |
| print(json.dumps({ | |
| "pr_branch": pr_branch, | |
| "merge_base": merge_base, | |
| "commit": commit_sha, | |
| "files_changed": files_changed, | |
| "empty": False, | |
| })) | |
| except Exception: | |
| subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True, text=True) | |
| git("branch", "-D", pr_branch) | |
| raise |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 217-223: use jsonify instead of json.dumps for JSON output
Context: json.dumps({
"pr_branch": pr_branch,
"merge_base": merge_base,
"commit": commit_sha,
"files_changed": files_changed,
"empty": False,
})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 207-207: Use of unsanitized data to create processes
Context: subprocess.run(log_cmd, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 192-192: Command coming from incoming request
Context: subprocess.run(["git", "checkout", "-b", pr_branch, merge_base, "--quiet"], capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 198-198: Command coming from incoming request
Context: subprocess.run(["git", "apply", "--index"], input=diff_output, capture_output=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 207-207: Command coming from incoming request
Context: subprocess.run(log_cmd, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 212-212: Command coming from incoming request
Context: subprocess.run(["git", "commit", "-m", commit_subject, "--quiet"], capture_output=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 215-215: Command coming from incoming request
Context: subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 225-225: Command coming from incoming request
Context: subprocess.run(["git", "checkout", original_branch, "--quiet"], capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 193-193: subprocess call: check for execution of untrusted input
(S603)
[error] 193-193: Starting a process with a partial executable path
(S607)
[error] 199-199: Starting a process with a partial executable path
(S607)
[warning] 207-207: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[error] 208-208: subprocess call: check for execution of untrusted input
(S603)
[error] 213-213: subprocess call: check for execution of untrusted input
(S603)
[error] 213-213: Starting a process with a partial executable path
(S607)
[error] 216-216: subprocess call: check for execution of untrusted input
(S603)
[error] 216-216: Starting a process with a partial executable path
(S607)
[error] 226-226: subprocess call: check for execution of untrusted input
(S603)
[error] 226-226: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.specify/extensions/spex/scripts/spex-detach.py around lines 192 - 228,
Update the git apply failure path in the surrounding branch-creation flow so
cleanup runs before terminating, or raise an exception caught by the existing
except Exception handler instead of calling sys.exit(1) directly. Preserve the
existing error JSON and exit status while ensuring original_branch is restored
and pr_branch is deleted when git apply fails.
| let claim_name = match crate::warm_pool::create_claim( | ||
| &self.client, | ||
| &self.config.namespace, | ||
| pool_name, | ||
| &sandbox.id, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(name) => name, | ||
| Err(e) => { | ||
| warn!( | ||
| sandbox_id = %sandbox.id, | ||
| error = %e, | ||
| "Failed to create warm pool claim, falling back to cold start" | ||
| ); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let pod_ip = match crate::warm_pool::wait_for_claim_ready( | ||
| &self.client, | ||
| &self.config.namespace, | ||
| &claim_name, | ||
| Duration::from_secs(10), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(ip) => ip, | ||
| Err(e) => { | ||
| warn!( | ||
| sandbox_id = %sandbox.id, | ||
| claim = %claim_name, | ||
| error = %e, | ||
| "Warm pool claim failed, falling back to cold start" | ||
| ); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let tls = self.read_activation_tls().await; | ||
|
|
||
| let sandbox_token = sandbox | ||
| .spec | ||
| .as_ref() | ||
| .map(|s| s.sandbox_token.clone()) | ||
| .unwrap_or_default(); | ||
|
|
||
| let request = openshell_core::proto::supervisor::v1::ActivateSandboxRequest { | ||
| sandbox_id: sandbox.id.clone(), | ||
| sandbox_name: sandbox.name.clone(), | ||
| sandbox_token, | ||
| gateway_endpoint: self.config.grpc_endpoint.clone(), | ||
| policy: Some(openshell_core::proto::sandbox::v1::SandboxPolicy::default()), | ||
| }; | ||
|
|
||
| match crate::activation_client::activate_sandbox(&pod_ip, request, tls.as_ref()).await { | ||
| Ok(resp) if resp.success => { | ||
| info!( | ||
| sandbox_id = %sandbox.id, | ||
| pod_ip = %pod_ip, | ||
| "Warm pool activation succeeded" | ||
| ); | ||
| Some(Ok(())) | ||
| } | ||
| Ok(resp) => { | ||
| warn!( | ||
| sandbox_id = %sandbox.id, | ||
| error_message = %resp.error_message, | ||
| error_code = resp.error_code, | ||
| "Warm pool activation returned failure, falling back to cold start" | ||
| ); | ||
| None | ||
| } | ||
| Err(e) => { | ||
| warn!( | ||
| sandbox_id = %sandbox.id, | ||
| error = %e, | ||
| "Warm pool activation error, falling back to cold start" | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether warm_pool exposes a claim release/delete helper and whether it is invoked anywhere.
rg -nP 'fn (create_claim|delete_claim|release_claim|wait_for_claim_ready)' crates/openshell-driver-kubernetes/src/warm_pool.rs -C2
rg -nP 'delete_claim|release_claim' crates/openshell-driver-kubernetes/src -C2Repository: rhuss/OpenShell
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect warm_pool claim lifecycle and the surrounding activation flow.
ast-grep outline crates/openshell-driver-kubernetes/src/warm_pool.rs --view expanded || true
ast-grep outline crates/openshell-driver-kubernetes/src/driver.rs --view expanded || true
echo '--- warm_pool.rs (relevant section) ---'
sed -n '1,260p' crates/openshell-driver-kubernetes/src/warm_pool.rs | cat -n
echo '--- driver.rs (relevant section) ---'
sed -n '940,1095p' crates/openshell-driver-kubernetes/src/driver.rs | cat -nRepository: rhuss/OpenShell
Length of output: 33940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any cleanup or TTL/owner-reference logic that might automatically reap claims.
rg -n "ownerReferences|owner_references|ttl|garbage|garbage collection|cleanup|finalizer|finalizers|delete_claim|release_claim|claim_name|warm pool" crates/openshell-driver-kubernetes/src -C2Repository: rhuss/OpenShell
Length of output: 25079
Delete the warm-pool claim on fallback paths.
create_claim succeeds, but the wait_for_claim_ready and activate_sandbox failure branches return None without cleanup, leaving the warm pod claimed and reducing pool capacity. Clean up the claim before falling back to cold start.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 985 - 1065,
Delete the successfully created warm-pool claim before returning None from the
wait_for_claim_ready and activate_sandbox failure branches. Reuse the existing
Kubernetes client, namespace, and claim_name with the warm-pool claim deletion
helper, while preserving the current warning logs and cold-start fallback
behavior.
| loop { | ||
| let (stream, _) = listener.accept().await?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Accept loop terminates on transient errors.
listener.accept().await? propagates any accept error, killing the health server. For a Kubernetes readiness endpoint, a single transient failure (EMFILE, ECONNABORTED) would cause readiness probes to fail and potentially trigger pod restarts. Log and continue instead.
🔧 Proposed fix: handle accept errors gracefully
let ready = self.ready;
loop {
- let (stream, _) = listener.accept().await?;
+ match listener.accept().await {
+ Ok((stream, _)) => {
+ let io = TokioIo::new(stream);
+ let ready = ready.clone();
+ tokio::spawn(async move {
+ let svc = service_fn(move |req| {
+ let ready = ready.clone();
+ async move { Ok::<_, Infallible>(handle_request(req, &ready)) }
+ });
+ if let Err(err) = hyper_util::server::conn::auto::Builder::new(
+ hyper_util::rt::TokioExecutor::new(),
+ )
+ .serve_connection(io, svc)
+ .await
+ {
+ debug!(error = %err, "Health check connection error");
+ }
+ });
+ }
+ Err(err) => {
+ debug!(error = %err, "Health check accept error; continuing");
+ }
}
- let io = TokioIo::new(stream);
- let ready = ready.clone();
- tokio::spawn(async move {
- let svc = service_fn(move |req| {
- let ready = ready.clone();
- async move { Ok::<_, Infallible>(handle_request(req, &ready)) }
- });
- if let Err(err) = hyper_util::server::conn::auto::Builder::new(
- hyper_util::rt::TokioExecutor::new(),
- )
- .serve_connection(io, svc)
- .await
- {
- debug!(error = %err, "Health check connection error");
- }
- });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loop { | |
| let (stream, _) = listener.accept().await?; | |
| let ready = self.ready; | |
| loop { | |
| match listener.accept().await { | |
| Ok((stream, _)) => { | |
| let io = TokioIo::new(stream); | |
| let ready = ready.clone(); | |
| tokio::spawn(async move { | |
| let svc = service_fn(move |req| { | |
| let ready = ready.clone(); | |
| async move { Ok::<_, Infallible>(handle_request(req, &ready)) } | |
| }); | |
| if let Err(err) = hyper_util::server::conn::auto::Builder::new( | |
| hyper_util::rt::TokioExecutor::new(), | |
| ) | |
| .serve_connection(io, svc) | |
| .await | |
| { | |
| debug!(error = %err, "Health check connection error"); | |
| } | |
| }); | |
| } | |
| Err(err) => { | |
| debug!(error = %err, "Health check accept error; continuing"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-sandbox/src/health.rs` around lines 36 - 37, Update the
accept loop in the health server around listener.accept so transient accept
errors are logged and the loop continues instead of being propagated with ?.
Preserve normal stream handling and only terminate for errors that cannot be
recovered from if the surrounding design explicitly distinguishes them.
| let server = tonic::transport::Server::builder() | ||
| .add_service(SupervisorServer::new(service)) | ||
| .serve(addr); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TLS configuration in the activation and supervisor code
rg -n 'tls_config|TlsConfig|ServerTlsConfig|tls\(' --type rust crates/openshell-sandbox/ crates/openshell-core/
# Check the proto definition for TLS requirements
fd 'supervisor.proto' | xargs cat 2>/dev/null
# Check if there's a TLS config in the activation module
ast-grep outline crates/openshell-sandbox/src/activation.rs --items all --type function,struct,impl 2>/dev/null || trueRepository: rhuss/OpenShell
Length of output: 5146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant server setup =="
rg -n 'tonic::transport::Server::builder|SupervisorServer::new|tls_config|ServerTlsConfig|Identity|Certificate|rustls|tls\(' crates/openshell-sandbox/src crates/openshell-core/src
echo
echo "== Files in openshell-sandbox src =="
git ls-files crates/openshell-sandbox/src
echo
echo "== lib.rs around supervisor server =="
sed -n '900,980p' crates/openshell-sandbox/src/lib.rs
echo
echo "== activation module outline if present =="
fd -a 'activation.rs' crates/openshell-sandbox/src || trueRepository: rhuss/OpenShell
Length of output: 4835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== activation.rs outline =="
ast-grep outline crates/openshell-sandbox/src/activation.rs --items all 2>/dev/null || true
echo
echo "== activation.rs contents around likely server/wrapper logic =="
wc -l crates/openshell-sandbox/src/activation.rs
sed -n '1,260p' crates/openshell-sandbox/src/activation.rs
echo
echo "== search for activation port / topology / sidecar references =="
rg -n 'activation_grpc_port|SUPERVISOR_TOPOLOGY|sidecar|mesh|proxy|TLS|tls_config|rustls|tonic::transport::Server::builder' crates/openshell-sandbox/src crates/openshell-core/srcRepository: rhuss/OpenShell
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== activation server port and topology context =="
sed -n '860,960p' crates/openshell-sandbox/src/lib.rs
echo
echo "== grpc client TLS docs and env vars =="
sed -n '120,190p' crates/openshell-core/src/grpc_client.rs
echo
echo "== TLS config docs in config.rs =="
sed -n '370,470p' crates/openshell-core/src/config.rs
echo
echo "== driver_utils TLS mount docs =="
sed -n '50,90p' crates/openshell-core/src/driver_utils.rs
echo
echo "== any server-side TLS builder usage in sandbox code =="
rg -n 'ServerTlsConfig|tls_config\(|serve_with_(incoming|tls)|accept_http1|require_http2' crates/openshell-sandbox/src crates/openshell-core/srcRepository: rhuss/OpenShell
Length of output: 12591
Add TLS to the activation gRPC listener. crates/openshell-sandbox/src/lib.rs:937-946 starts SupervisorServer with a plain tonic::transport::Server::builder(), so ActivateSandbox’s JWT still travels in cleartext unless TLS is terminated outside this code. Wire in .tls_config() here, or document the external termination layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-sandbox/src/lib.rs` around lines 944 - 946, Update the
activation gRPC listener construction around SupervisorServer::new and
Server::builder to configure TLS with .tls_config() before serving, using the
project’s existing certificate and private-key configuration. Ensure the
resulting serve path remains compatible with SupervisorServer and fails clearly
if TLS configuration cannot be applied.
| let mut child = Command::new(env!("CARGO_BIN_EXE_openshell-sandbox")) | ||
| .arg("--unidentified") | ||
| .arg("--health-check") | ||
| .arg("--health-port") | ||
| .arg(health_port.to_string()) | ||
| .arg("--") | ||
| .arg("/bin/true") | ||
| .env("OPENSHELL_LOG_LEVEL", "info") | ||
| .env("OPENSHELL_ACTIVATION_PORT", grpc_port.to_string()) | ||
| .env_remove("RUST_LOG") | ||
| .env_remove("OPENSHELL_POLICY_RULES") | ||
| .env_remove("OPENSHELL_POLICY_DATA") | ||
| .env_remove("OPENSHELL_SANDBOX_ID") | ||
| .env_remove("OPENSHELL_ENDPOINT") | ||
| .spawn() | ||
| .expect("failed to spawn openshell-sandbox in unidentified mode"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Child process is orphaned if any assertion fails before cleanup.
std::process::Child does not kill the child on Drop. If any assert! between line 77 and line 156 panics, the sandbox process keeps running — holding the health and gRPC ports and consuming resources. In local dev this causes port conflicts on subsequent runs; in CI it wastes runner resources.
🔒️ Proposed fix: wrap child in a Drop guard
+struct ChildGuard(std::process::Child);
+impl Drop for ChildGuard {
+ fn drop(&mut self) {
+ let _ = self.0.kill();
+ let _ = self.0.wait();
+ }
+}
+
#[tokio::test]
async fn activated_supervisor_exercises_cold_start_bootstrap() {
let health_port = free_port();
let grpc_port = free_port();
- let mut child = Command::new(env!("CARGO_BIN_EXE_openshell-sandbox"))
+ let mut child = ChildGuard(
+ Command::new(env!("CARGO_BIN_EXE_openshell-sandbox"))
.arg("--unidentified")
.arg("--health-check")
.arg("--health-port")
.arg(health_port.to_string())
.arg("--")
.arg("/bin/true")
.env("OPENSHELL_LOG_LEVEL", "info")
.env("OPENSHELL_ACTIVATION_PORT", grpc_port.to_string())
.env_remove("RUST_LOG")
.env_remove("OPENSHELL_POLICY_RULES")
.env_remove("OPENSHELL_POLICY_DATA")
.env_remove("OPENSHELL_SANDBOX_ID")
.env_remove("OPENSHELL_ENDPOINT")
.spawn()
- .expect("failed to spawn openshell-sandbox in unidentified mode");
+ .expect("failed to spawn openshell-sandbox in unidentified mode"),
+ );Then remove the manual cleanup at the end since Drop handles it:
- let _ = child.kill();
- let _ = child.wait();
+ // Child is automatically killed when `child` (ChildGuard) goes out of scope.Also applies to: 156-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-sandbox/tests/activation_smoke.rs` around lines 62 - 77,
Wrap the spawned child in a Drop guard immediately after the Command chain in
the activation smoke test, ensuring the guard kills and waits for the process
when assertions unwind. Remove the redundant manual cleanup at the test’s end,
while preserving the existing child process usage throughout the test.
The supervisor's activation gRPC endpoint doesn't serve TLS in the PoC. Skip mTLS for the ActivateSandbox call and connect via plaintext HTTP. Production will add TLS to the supervisor's gRPC listener. Also makes the activation client support both TLS and plaintext channels based on whether TLS config is provided. Validated on ROSA HCP 4.22.3: gateway detects warm pool, claims pod, and calls ActivateSandbox successfully in ~340ms (in-cluster). Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Captures the plan for completing the end-to-end warm pool flow with full SSH support, proxy, OPA enforcement, and entrypoint process. Approach: extract shared bootstrap from run_sandbox() for functional parity between cold start and warm pool paths. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Specification for completing the end-to-end warm pool flow with full SSH support. Extracts networking/proxy/OPA/entrypoint from run_sandbox() into shared bootstrap code for functional parity between cold start and warm pool paths. Target: SSH shell in under 3 seconds. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Five-phase plan to extract post-identity bootstrap from run_sandbox() into shared code. Core design: BootstrapContext struct + shared post_identity_bootstrap() function called by both cold-start and warm pool paths. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
26 tasks across 5 phases: define BootstrapContext, extract post_identity_bootstrap(), wire into bootstrap_sandbox(), update lifecycle, build/deploy/test on cluster. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
…or warm pool E2E Refactor run_sandbox() to extract shared bootstrap logic into BootstrapContext struct and post_identity_bootstrap() function. This enables the warm pool activation path to run the full networking/SSH/process stack after gRPC identity push. - Define BootstrapContext with 18 fields for shared parameter passing - Extract post_identity_bootstrap() (~400 lines) from run_sandbox() - Wire bootstrap_sandbox() to build context and call shared function - Change oneshot channel from Sender<String> to Sender<i32> for exit codes - Update run_unidentified() to propagate exit code directly All 94 existing tests pass unchanged. Assisted-By: 🤖 Claude Code
- Add pool-quickstart.md: 5-minute tester recipe using image tag trick for warm vs cold comparison (no cluster changes needed) - Update SMOKE-TEST.md with Milestone 2 SSH results and mTLS cert extraction steps - Update plan.md and tasks.md with review-plan fixes (consolidated tasks, added interfaces, global constraints) - Add REVIEW-CODE.md and review-findings.md from deep review Assisted-By: 🤖 Claude Code
Replace the cluster admin password with a placeholder. The password was committed in plaintext and is visible in git history on a public fork. The cluster password should be rotated separately. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/openshell-sandbox/src/lib.rs (1)
1012-1021: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a dropped activation sender to avoid a stuck process.
Ok(exit_code) = activation_rxis a refutable pattern. If the spawnedpost_identity_bootstraptask inactivation.rspanics before sending, the oneshot sender is dropped andactivation_rxresolves toErr(RecvError). That disables this select branch, leaving only the indefinitely-servingserverbranch — the process neither exits nor completes activation and must be reaped by a liveness probe. This is FINDING-1 in the review doc; the fix is trivial.🛠️ Handle the dropped sender explicitly
- Ok(exit_code) = activation_rx => { - info!( - exit_code = exit_code, - "Activation complete, process exited" - ); - return Ok(exit_code); - } + activation_result = activation_rx => { + match activation_result { + Ok(exit_code) => { + info!(exit_code, "Activation complete, process exited"); + return Ok(exit_code); + } + Err(_) => { + warn!("Activation task dropped its sender without an exit code"); + return Ok(1); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-sandbox/src/lib.rs` around lines 1012 - 1021, Handle the Err case from activation_rx in the select branch instead of matching only Ok(exit_code). When the activation sender is dropped, log the activation failure and return an error or otherwise terminate the process so it cannot remain stuck serving indefinitely; preserve the existing successful exit_code path.
♻️ Duplicate comments (2)
crates/openshell-driver-kubernetes/src/driver.rs (2)
909-1026: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
read_activation_tls()is defined but never called — warm-pool activation is unconditionally plaintext, contradicting the PR's stated "TLS when configured" behavior.
try_warm_poolhardcodestls: Option<TlsConfig> = Noneat line 1026 instead of callingself.read_activation_tls().await. Combined with the earlier-flagged fallback-to-Nonebehavior insideread_activation_tlsitself, this means even a correctly configuredclient_tls_secret_namecan never enable mTLS for activation today — the helper is effectively dead code. If this is intentional for the current PoC milestone (as the inline comment at lines 1024-1025 states), consider removing the unused helper to avoid confusion/lint noise; otherwise wire it in.#!/bin/bash # Confirm read_activation_tls has no caller anywhere in the crate. rg -n 'read_activation_tls' crates/openshell-driver-kubernetes/src -C3🔒 Proposed fix (wire it in)
- // PoC: supervisor serves plain gRPC, skip mTLS for activation. - // Production will add TLS to the supervisor's activation endpoint. - let tls: Option<crate::activation_client::TlsConfig> = None; + let tls = self.read_activation_tls().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 909 - 1026, Wire the warm-pool activation flow in try_warm_pool to call self.read_activation_tls().await instead of unconditionally assigning None, so configured TLS secrets enable mTLS. Remove the PoC plaintext comment and preserve the helper’s None fallback when TLS is not configured or cannot be loaded.
985-1067: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWarm-pool claim isn't released on fallback paths (still unresolved).
create_claimsucceeds at line 985, but thewait_for_claim_readyfailure branch (1013-1021) and theactivate_sandboxfailure/error branches (1051-1067) all returnNonewithout deletingclaim_name, permanently reducing warm-pool capacity on every fallback. Nodelete_claim/release_claimhelper currently exists inwarm_pool.rsper prior investigation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 985 - 1067, Release the successfully created warm-pool claim before every cold-start fallback after claim creation. In the wait_for_claim_ready error branch and both activate_sandbox failure branches, delete claim_name through the Kubernetes client using the appropriate warm_pool cleanup logic, log cleanup failures without replacing the original fallback, and preserve the existing None returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/openshell-driver-kubernetes/src/activation_client.rs`:
- Around line 42-49: Update the TLS configuration in the activation client’s
endpoint-building flow to set an explicit domain name covered by the supervisor
certificate via ClientTlsConfig::domain_name, rather than relying on the raw
pod_ip URI host. Reuse the existing configured supervisor DNS name or localhost
SAN value, while preserving the CA certificate, client identity, and endpoint
error mapping.
- Around line 36-56: Update both URI constructions in the TLS and plaintext
branches of the endpoint setup to bracket IPv6 pod IP literals before appending
ACTIVATION_PORT, while preserving valid endpoint formatting for IPv4 addresses
and hostnames. Apply the same authority formatting consistently to the `https`
and `http` paths.
In `@crates/openshell-sandbox/src/activation.rs`:
- Around line 179-196: Update run_unidentified’s activation_rx result handling
to add an Err(_) arm for a dropped activation sender, returning a non-zero exit
code when the spawned bootstrap task exits without sending. Preserve the
existing Ok(exit_code) behavior.
---
Outside diff comments:
In `@crates/openshell-sandbox/src/lib.rs`:
- Around line 1012-1021: Handle the Err case from activation_rx in the select
branch instead of matching only Ok(exit_code). When the activation sender is
dropped, log the activation failure and return an error or otherwise terminate
the process so it cannot remain stuck serving indefinitely; preserve the
existing successful exit_code path.
---
Duplicate comments:
In `@crates/openshell-driver-kubernetes/src/driver.rs`:
- Around line 909-1026: Wire the warm-pool activation flow in try_warm_pool to
call self.read_activation_tls().await instead of unconditionally assigning None,
so configured TLS secrets enable mTLS. Remove the PoC plaintext comment and
preserve the helper’s None fallback when TLS is not configured or cannot be
loaded.
- Around line 985-1067: Release the successfully created warm-pool claim before
every cold-start fallback after claim creation. In the wait_for_claim_ready
error branch and both activate_sandbox failure branches, delete claim_name
through the Kubernetes client using the appropriate warm_pool cleanup logic, log
cleanup failures without replacing the original fallback, and preserve the
existing None returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 136edaa2-8bea-4e0d-9135-bfe32a52eaeb
📒 Files selected for processing (21)
.gitignore.specify/.pr-triage-state.json.specify/feature.json.specify/integrations/opencode.manifest.json.specify/integrations/speckit.manifest.jsonAGENTS.mdbrainstorm/00-overview.mdbrainstorm/08-warm-pool-e2e-ssh.mdbrainstorm/idea-inbox.mdcrates/openshell-driver-kubernetes/src/activation_client.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-sandbox/src/activation.rscrates/openshell-sandbox/src/lib.rsexperiments/SMOKE-TEST.mdexperiments/pool-quickstart.mdspecs/003-warm-pool-e2e-ssh/REVIEW-CODE.mdspecs/003-warm-pool-e2e-ssh/checklists/requirements.mdspecs/003-warm-pool-e2e-ssh/plan.mdspecs/003-warm-pool-e2e-ssh/review-findings.mdspecs/003-warm-pool-e2e-ssh/spec.mdspecs/003-warm-pool-e2e-ssh/tasks.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .specify/integrations/speckit.manifest.json
- .specify/feature.json
- .specify/integrations/opencode.manifest.json
- experiments/SMOKE-TEST.md
| let (endpoint_uri, mut endpoint) = if let Some(tls) = tls { | ||
| let uri = format!("https://{pod_ip}:{ACTIVATION_PORT}"); | ||
| let mut ep = Channel::from_shared(uri.clone()) | ||
| .map_err(|e| ActivationError::ConnectionFailed(e.to_string()))? | ||
| .connect_timeout(Duration::from_secs(2)); | ||
|
|
||
| let tls_config = ClientTlsConfig::new() | ||
| .ca_certificate(Certificate::from_pem(&tls.ca_cert)) | ||
| .identity(Identity::from_pem(&tls.client_cert, &tls.client_key)); | ||
| ep = ep | ||
| .tls_config(tls_config) | ||
| .map_err(|e| ActivationError::ConnectionFailed(e.to_string()))?; | ||
|
|
||
| (uri, ep) | ||
| } else { | ||
| let uri = format!("http://{pod_ip}:{ACTIVATION_PORT}"); | ||
| let ep = Channel::from_shared(uri.clone()) | ||
| .map_err(|e| ActivationError::ConnectionFailed(e.to_string()))? | ||
| .connect_timeout(Duration::from_secs(2)); | ||
| (uri, ep) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
IPv6 pod IPs still produce an invalid endpoint authority (now in both branches).
Both the TLS (https://{pod_ip}:{ACTIVATION_PORT}, line 37) and plaintext (http://{pod_ip}:{ACTIVATION_PORT}, line 51) URIs format pod_ip unbracketed. An IPv6 pod IP (e.g. fd00::1) yields an invalid authority like https://fd00::1:9090 — IPv6 literals must be bracketed (https://[fd00::1]:9090). This was previously flagged on the single-branch version and now applies to both new branches.
🐛 Proposed fix
+ let host = |ip: &str| {
+ if ip.contains(':') && !ip.starts_with('[') {
+ format!("[{ip}]")
+ } else {
+ ip.to_string()
+ }
+ };
let (endpoint_uri, mut endpoint) = if let Some(tls) = tls {
- let uri = format!("https://{pod_ip}:{ACTIVATION_PORT}");
+ let uri = format!("https://{}:{ACTIVATION_PORT}", host(pod_ip));
...
} else {
- let uri = format!("http://{pod_ip}:{ACTIVATION_PORT}");
+ let uri = format!("http://{}:{ACTIVATION_PORT}", host(pod_ip));
...
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-kubernetes/src/activation_client.rs` around lines 36
- 56, Update both URI constructions in the TLS and plaintext branches of the
endpoint setup to bracket IPv6 pod IP literals before appending ACTIVATION_PORT,
while preserving valid endpoint formatting for IPv4 addresses and hostnames.
Apply the same authority formatting consistently to the `https` and `http`
paths.
| let tls_config = ClientTlsConfig::new() | ||
| .ca_certificate(Certificate::from_pem(&tls.ca_cert)) | ||
| .identity(Identity::from_pem(&tls.client_cert, &tls.client_key)); | ||
| ep = ep | ||
| .tls_config(tls_config) | ||
| .map_err(|e| ActivationError::ConnectionFailed(e.to_string()))?; | ||
|
|
||
| (uri, ep) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
TLS hostname verification still not pinned to a SAN the supervisor cert actually covers.
The TLS branch builds ClientTlsConfig with CA + client identity but never calls .domain_name(...), so tonic will verify the server cert against the raw pod_ip used in the URI. Per the earlier review, the default supervisor certs only cover DNS SANs plus 127.0.0.1, not arbitrary pod IPs, unless serverIpAddresses is configured — this will fail TLS handshakes in the common case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-kubernetes/src/activation_client.rs` around lines 42
- 49, Update the TLS configuration in the activation client’s endpoint-building
flow to set an explicit domain name covered by the supervisor certificate via
ClientTlsConfig::domain_name, rather than relying on the raw pod_ip URI host.
Reuse the existing configured supervisor DNS name or localhost SAN value, while
preserving the CA certificate, client identity, and endpoint error mapping.
| let sender = self.activation_tx.lock().unwrap().take(); | ||
| if let Some(sender) = sender { | ||
| let sandbox_id = req.sandbox_id.clone(); | ||
| tokio::spawn(async move { | ||
| let exit_code = match crate::post_identity_bootstrap(bootstrap_ctx).await { | ||
| Ok(code) => code, | ||
| Err(e) => { | ||
| tracing::error!( | ||
| sandbox_id = %sandbox_id, | ||
| error = %e, | ||
| "post_identity_bootstrap failed" | ||
| ); | ||
| 1 | ||
| } | ||
| }; | ||
| let _ = sender.send(exit_code); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== activation.rs around cited lines ==\n'
sed -n '150,230p' crates/openshell-sandbox/src/activation.rs
printf '\n== search for post_identity_bootstrap ==\n'
rg -n "post_identity_bootstrap|activation_rx|activation_tx|run_unidentified" crates/openshell-sandbox/src -n
printf '\n== lib.rs around run_unidentified select ==\n'
sed -n '960,1060p' crates/openshell-sandbox/src/lib.rsRepository: rhuss/OpenShell
Length of output: 7775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== activation.rs around cited lines ==\n'
sed -n '150,230p' crates/openshell-sandbox/src/activation.rs
printf '\n== search for post_identity_bootstrap ==\n'
rg -n "post_identity_bootstrap|activation_rx|activation_tx|run_unidentified" crates/openshell-sandbox/src
printf '\n== lib.rs around run_unidentified select ==\n'
sed -n '960,1060p' crates/openshell-sandbox/src/lib.rsRepository: rhuss/OpenShell
Length of output: 7775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== post_identity_bootstrap implementation ==\n'
sed -n '330,430p' crates/openshell-sandbox/src/lib.rs
printf '\n== any panic handling / catch_unwind around bootstrap or activation ==\n'
rg -n "catch_unwind|panic|abort|spawn\(" crates/openshell-sandbox/src/lib.rs crates/openshell-sandbox/src/activation.rs crates/openshell-sandbox/src/main.rsRepository: rhuss/OpenShell
Length of output: 5399
Handle a dropped activation sender in run_unidentified
crates/openshell-sandbox/src/lib.rs:1012 — if the spawned bootstrap task panics or exits before sending, activation_rx resolves to Err(_). The current Ok(exit_code) = activation_rx arm skips that case and can leave the supervisor waiting indefinitely. Add an Err(_) arm and return a non-zero exit code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-sandbox/src/activation.rs` around lines 179 - 196, Update
run_unidentified’s activation_rx result handling to add an Err(_) arm for a
dropped activation sender, returning a non-zero exit code when the spawned
bootstrap task exits without sending. Preserve the existing Ok(exit_code)
behavior.
Bring over the warm pool feasibility RFC, gRPC-push RFC, and brainstorm documents from the 6111-warm-pool-feasibility branch so the PoC branch is self-contained for reviewers. Signed-off-by: Roland Huß <rhuss@redhat.com>
- Remove dead boolCount function that would fail golangci-lint (#1) - Emit EventAdded for the first watch event instead of EventModified, matching k8s watch semantics (#7) - Add mutex locking to all mock server methods that access the shared sandboxes map, fixing latent race conditions (#12) - Skip HealthCheck integration test that calls an unimplemented stub (#13) - Scope doc.go examples: mark sections for sub-clients not yet available in this PR with "available in a future release" (#4) - Document Config.Timeout/RetryPolicy/Logger and WatchOptions fields as reserved for future use (#2, #6) Signed-off-by: Roland Huß <rhuss@redhat.com>
1 participant
Summary
ActivateSandboxgRPC endpoint on the supervisor for claim-time identity binding of warm pool pods--unidentifiedflag), listens on gRPC port, serves/readyz, and bootstraps fully upon activationRelated Issue
Brainstorm 06: Warm Pool gRPC PoC. Implements NVIDIA-endorsed gRPC-push direction for warm pool identity binding.
Changes
New files
proto/supervisor.proto- NewSupervisorgRPC service withActivateSandboxRPCcrates/openshell-sandbox/src/activation.rs- ActivateSandbox handler with full bootstrap sequencecrates/openshell-sandbox/src/health.rs- HTTP/readyzendpoint (wires up existing--health-checkflag)crates/openshell-driver-kubernetes/src/warm_pool.rs- SandboxWarmPool/SandboxClaim CRD interactioncrates/openshell-driver-kubernetes/src/activation_client.rs- Gateway-side ActivateSandbox gRPC clientcrates/openshell-sandbox/tests/activation_smoke.rs- Functional equivalence smoke testcrates/openshell-sandbox/tests/activation_timing.rs- Readiness timing integration testModified files
crates/openshell-sandbox/src/main.rs---unidentifiedCLI flag, startup branchingcrates/openshell-sandbox/src/lib.rs-run_unidentified()+bootstrap_sandbox()extracted fromrun_sandbox()crates/openshell-core/src/grpc_client.rs- Direct JWT path (skip IssueSandboxToken for warm pool)crates/openshell-driver-kubernetes/src/driver.rs- Warm pool detection + claim path increate_sandbox()architecture/sandbox.md- Unidentified mode + activation flow documentationarchitecture/compute-runtimes.md- Warm pool claim path documentationTesting
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
/readyzreadiness probing.