Skip to content

Commit 47b790c

Browse files
committed
chore: Address code review feedback
Addresses Claude Code Review feedback items: 1. Delete deprecated setup-vertex-ai.sh script - Script was replaced by Makefile automation - Prevents confusion and risk of modifying tracked files 2. Replace hardcoded sleeps with kubectl wait - Use 'kubectl wait --for=condition=ready' for pods - More reliable than fixed timeouts - Reduced final sleep from 2s to 1s 3. Update SECURITY_DEV_MODE.md to reflect allow-list implementation - Code uses allow-list (ambient-code, default, vteam-dev) - Documentation was describing old deny-list approach - Now accurately documents current security posture 4. Add ShellCheck directive for intentional set +e - Documents why we continue on errors (collect all test results) 5. Document sys.path.insert in wrapper.py - Explains why runner-shell path manipulation is necessary - Clarifies dependency on RunnerShell framework All changes improve code quality, documentation accuracy, and reliability.
1 parent 7489556 commit 47b790c

File tree

5 files changed

+20
-88
lines changed

5 files changed

+20
-88
lines changed

Makefile

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -521,17 +521,18 @@ _auto-port-forward: ## Internal: Auto-start port forwarding on macOS with Podman
521521
echo ""; \
522522
echo "$(COLOR_BLUE)$(COLOR_RESET) Starting port forwarding in background..."; \
523523
echo " Waiting for services to be ready..."; \
524-
sleep 3; \
524+
kubectl wait --for=condition=ready pod -l app=backend -n $(NAMESPACE) --timeout=60s 2>/dev/null || true; \
525+
kubectl wait --for=condition=ready pod -l app=frontend -n $(NAMESPACE) --timeout=60s 2>/dev/null || true; \
525526
mkdir -p /tmp/ambient-code; \
526527
kubectl port-forward -n $(NAMESPACE) svc/backend-service 8080:8080 > /tmp/ambient-code/port-forward-backend.log 2>&1 & \
527528
echo $$! > /tmp/ambient-code/port-forward-backend.pid; \
528529
kubectl port-forward -n $(NAMESPACE) svc/frontend-service 3000:3000 > /tmp/ambient-code/port-forward-frontend.log 2>&1 & \
529530
echo $$! > /tmp/ambient-code/port-forward-frontend.pid; \
530-
sleep 2; \
531+
sleep 1; \
531532
if ps -p $$(cat /tmp/ambient-code/port-forward-backend.pid 2>/dev/null) > /dev/null 2>&1 && \
532533
ps -p $$(cat /tmp/ambient-code/port-forward-frontend.pid 2>/dev/null) > /dev/null 2>&1; then \
533534
echo "$(COLOR_GREEN)$(COLOR_RESET) Port forwarding started"; \
534-
echo " $(COLOR_BOLD)Wait ~30s for pods to be ready, then access:$(COLOR_RESET)"; \
535+
echo " $(COLOR_BOLD)Access at:$(COLOR_RESET)"; \
535536
echo " Frontend: $(COLOR_BLUE)http://localhost:3000$(COLOR_RESET)"; \
536537
echo " Backend: $(COLOR_BLUE)http://localhost:8080$(COLOR_RESET)"; \
537538
else \

components/runners/claude-code-runner/wrapper.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
os.umask(0o022)
2121

2222
# Add runner-shell to Python path
23+
# Required: runner-shell is installed in /app/runner-shell and contains the core RunnerShell framework
24+
# that this wrapper depends on. Must be imported before runner_shell module below.
2325
sys.path.insert(0, '/app/runner-shell')
2426

2527
from runner_shell.core.shell import RunnerShell

docs/SECURITY_DEV_MODE.md

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,23 +62,26 @@ func isLocalDevEnvironment() bool {
6262

6363
## Identified Risks
6464

65-
### 🔴 **HIGH RISK: Weak Namespace Check**
65+
### 🟢 **MITIGATED: Allow-List Namespace Validation**
6666

67-
**Current:** Only rejects if namespace contains "prod"
67+
**Current:** Uses allow-list of specific namespaces (ambient-code, default, vteam-dev)
6868

69-
**Risk Scenarios:**
69+
**Protection:**
7070
```bash
71-
# Would PASS (incorrectly enable dev mode):
72-
NAMESPACE=staging DISABLE_AUTH=true ENVIRONMENT=local # ❌ Dangerous
73-
NAMESPACE=qa-env DISABLE_AUTH=true ENVIRONMENT=local # ❌ Dangerous
74-
NAMESPACE=demo DISABLE_AUTH=true ENVIRONMENT=local # ❌ Dangerous
75-
NAMESPACE=customer-abc DISABLE_AUTH=true ENVIRONMENT=local # ❌ Dangerous
71+
# Would PASS (correctly enable dev mode):
72+
NAMESPACE=ambient-code DISABLE_AUTH=true ENVIRONMENT=local # ✅ Allowed
73+
NAMESPACE=default DISABLE_AUTH=true ENVIRONMENT=local # ✅ Allowed
74+
NAMESPACE=vteam-dev DISABLE_AUTH=true ENVIRONMENT=local # ✅ Allowed
7675

7776
# Would FAIL (correctly reject):
78-
NAMESPACE=production DISABLE_AUTH=true ENVIRONMENT=local # ✅ Good
79-
NAMESPACE=prod-east DISABLE_AUTH=true ENVIRONMENT=local # ✅ Good
77+
NAMESPACE=staging DISABLE_AUTH=true ENVIRONMENT=local # ❌ Rejected
78+
NAMESPACE=qa-env DISABLE_AUTH=true ENVIRONMENT=local # ❌ Rejected
79+
NAMESPACE=production DISABLE_AUTH=true ENVIRONMENT=local # ❌ Rejected
80+
NAMESPACE=customer-abc DISABLE_AUTH=true ENVIRONMENT=local # ❌ Rejected
8081
```
8182

83+
**Implementation:** See `components/backend/handlers/middleware.go:315-327`
84+
8285
### 🟡 **MEDIUM RISK: No Cluster Type Detection**
8386

8487
Dev mode could activate on real Kubernetes clusters if someone:

scripts/setup-vertex-ai.sh

Lines changed: 0 additions & 75 deletions
This file was deleted.

tests/local-dev-test.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#
1212

1313
# Don't exit on error - we want to collect all test results
14+
# shellcheck disable=SC2103 # Intentional: continue on errors to collect all test results
1415
set +e
1516

1617
# Colors for output

0 commit comments

Comments
 (0)