PR review: roboflow/roboflow #11753

roboflow-11753-20260513-132003.md

PR Review: Add devcontainer staging mode with read-only service-account sandbox

Repository: roboflow/roboflow PR: #11753 Branch: tonylampada/devcontainer-staging → master Author: tonylampada Base commit: d66eb3da5cdb03b351265952ab57e870b49f8468 Head commit: a41198420b4c6d4f0a5b7256271accbb935628cc Changed files: 4 — cli.sh, docker-compose.yml, nginx.conf.template (new), SKILL.md Additions/Deletions: ~+340 / −12 Review Date: 2026-05-13 Disclaimer: Generated by Claude.

---

📋 Summary

Adds cli.sh serve-staging — a mode that runs Firebase hosting + functions inside the devcontainer against the real roboflow-staging project via a scoped service-account key. nginx (TLS termination via mkcert wildcard cert) and redis are added as compose services. An opt-in cli.sh proxy up sidecar exposes host :443 for manual browser access; agent-browser is pre-installed for in-container automation. Documentation updated in SKILL.md.

No previous review exists for this PR.

---

🏗️ Architectural Review

The PR extends an existing developer-tooling layer (.claude/skills/devcontainer/) rather than touching application code. The three-layer architecture guidelines and domain invariants from review-guidelines.md are not directly relevant here.

Structurally, the change introduces a second operating mode alongside the existing emulator-only mode. Networking is correctly contained: nginx gets no host port by default and the devcontainer communicates with it via Docker DNS. The opt-in proxy up sidecar handles the single-worktree constraint for host :443 correctly.

One structural tension: nginx and redis are unconditionally added to cmd_up, coupling the base setup to staging prerequisites. Users who only run tests now start two extra services they don't need and must satisfy the mkcert dependency (see finding #1).

flowchart LR
    subgraph Host
        A[cli.sh serve-staging] -->|docker cp SA key| B[devcontainer]
        A -->|_exec_in_container| B
        C[cli.sh proxy up] -->|docker run socat| D[proxy sidecar]
        D -->|TCP:443| E[nginx]
    end

    subgraph DockerCompose ["compose network"]
        E["nginx\n*.roboflow.one:443"]
        B["devcontainer\n(firebase emu :5000 + webpack)"]
        F[redis :6379]
        E -->|"http://${DEVCONTAINER_HOST}:5000"| B
        B -->|redis:6379| F
    end

Loading

The NGINX_ENVSUBST_FILTER: ^DEVCONTAINER_HOST$$ correctly restricts envsubst to only replace that variable, preserving nginx's own $host, $remote_addr, etc. The double-dollar escaping in Docker Compose YAML is correct.

---

🐛 Code Quality Issues

1. ⚠️ _ensure_certs in cmd_up breaks the test-only workflow

File: .claude/skills/devcontainer/cli.sh:40

cmd_up now unconditionally calls _ensure_certs, which hard-exits if mkcert is not installed:

if ! command -v mkcert > /dev/null 2>&1; then
    echo "ERROR: mkcert not installed..."
    exit 1
fi

Before this PR, cli.sh up had no mkcert dependency. Anyone using the devcontainer only to run tests or lint — the primary advertised workflow — now cannot up at all without installing mkcert. On non-Mac machines (or in any CI-like environment), brew install mkcert isn't applicable. The SKILL.md documents mkcert under "Prerequisites (one-time) → Running the App Against Staging", implying it's staging-only; but the code makes it universal.

The reason mkcert must run at up time is that nginx's volume mount requires the cert files to exist before the container starts. The cleanest fix: don't start nginx in cmd_up at all; start it lazily inside cmd_serve_staging alongside the other staging-specific setup. This would leave the emulator workflow unchanged.

---

2. ⚠️ Service-account key not cleaned up after serve-staging exits

File: .claude/skills/devcontainer/cli.sh:314–316, ~356

The key is copied into the container before _exec_in_container runs but is never removed afterward:

docker cp "$sa_key" "$CONTAINER_NAME:$CONTAINER_SA_KEY"
docker exec "$CONTAINER_NAME" chmod 600 "$CONTAINER_SA_KEY"
# ...
_exec_in_container bash -c "..."
# ← key still at /root/.config/claude-sandbox-key.json

After the session ends (Ctrl+C or normal exit), any subsequent cli.sh exec bash or test run in the same container can read the staging SA key from its predictable path. The in-container trap only kills the emulator; it doesn't clean the key.

Fix: add cleanup after _exec_in_container returns:

_exec_in_container bash -c "..."
docker exec "$CONTAINER_NAME" rm -f "$CONTAINER_SA_KEY" 2>/dev/null || true

---

3. 🤨 Dead redis-server startup check

File: .claude/skills/devcontainer/cli.sh:327–329

if command -v redis-server > /dev/null 2>&1 && ! redis-cli ping > /dev/null 2>&1; then
    redis-server --daemonize yes --loglevel warning
fi

redis-cli ping without arguments targets 127.0.0.1:6379. Nothing runs on localhost:6379 inside the container — the actual redis is the compose service at redis:6379. So this guard always fires (if redis-server is installed), starts a localhost redis that nothing connects to, and the environment variable EMULATED_REDIS_HOST_PORT=redis:6379 (set two lines later) routes the app to the compose service anyway. The fallback redis-server is unreachable and unused.

Remove the block entirely, or change the ping to redis-cli -h redis ping if the intent was to wait for the compose service.

---

4. 🤨 alpine:latest floating tag in _proxy_up

File: .claude/skills/devcontainer/cli.sh:393

docker run -d --rm \
    --name "$PROXY_CONTAINER_NAME" \
    ...
    alpine:latest \
    sh -c "apk add --no-cache socat > /dev/null && socat ..."

alpine:latest is a mutable tag. A future docker pull could update to a breaking Alpine version. Pin to a minor version (e.g., alpine:3.21) for reproducibility.

---

💡 Recommendations

• nginx container naming fragility (cli.sh:358): NGINX_CONTAINER_NAME="${PROJECT_NAME}-nginx-1" hardcodes Docker Compose's current <project>-<service>-1 naming convention. This works today but is implementation-dependent. Alternatively, reference the nginx container via the compose network hostname nginx from within the sidecar, removing the need to know the container name.

• SKILL.md mkcert placement: The prerequisites section lists mkcert under "Running the App Against Staging", but after this PR it's required for all cli.sh up invocations. Either move the prerequisite to the top-level "Important Details" section, or defer it to staging-only (see finding #1).

---

🧪 Tests

No tests are applicable here — this is a developer tooling change (shell scripts, Docker Compose, nginx config). The observable test is: does cli.sh up succeed, does serve-staging proxy correctly, and does the proxy sidecar bind/unbind cleanly. No automated coverage is expected or missing.

---

⚖️ Verdict

Request changes. The serve-staging concept is solid and the nginx/mkcert/agent-browser wiring is correct. Two items block a clean merge:

1. _ensure_certs in cmd_up imposes a new mandatory host dependency on all devcontainer users — not just staging users. The existing test workflow breaks for anyone without mkcert.
2. The SA key is not cleaned up after the session ends, leaving staging credentials at a predictable path in a container that continues to be used for tests.

The redis dead-code and alpine:latest issues are low-stakes but worth fixing in the same pass.