Add deployment validation check for DinD devserver integration

[james-in-a-box]

Add deployment validation check for DinD devserver integration

The check phase currently validates agent-authored changes with lint, tests, and
merge-conflict detection — all static or unit-level. This PR adds orchestrator-driven
Docker-in-Docker deployment validation so the check phase can spin up a target
application's devserver stack and run HTTP health checks and API smoke tests against
real running services with the agent's code mounted in.

Key components:

• orchestrator/devserver.py — Full devserver lifecycle manager: extracts compose
  config from committed state (not worktree), generates override files with read-only
  agent code mounts, creates air-gapped egg-check networks (internal: true, no
  gateway, restricted inter-container traffic), manages startup/health-wait/teardown
  with resource limits and hard time caps.

• orchestrator/routes/checks.py — New /checks/deployment routes that coordinate
  the orchestrator-managed devserver with sandbox-based validation. The orchestrator
  starts services before spawning the checker and tears them down after.

• shared/egg_contracts/deployment.py — Deployment configuration contracts: service
  mappings, health check definitions, network policies, and resource limits with
  validation.

• .github/scripts/checks/deployment_check.py — Sandbox-side checker that runs
  health probes, API smoke tests, and service readiness validation against the
  orchestrator-managed devserver stack. Parses responses defensively against
  attacker-controlled service output.

• shared/egg_contracts/phase_defaults.py — Adds check-deployment definition
  (optional, retry-enabled) to the check phase.

Security model: The sandbox never gets Docker socket access. The orchestrator
(trusted) manages the entire devserver lifecycle. Agent code runs inside devserver
containers on an air-gapped network with no egress, no credentials, no capabilities,
default seccomp, and read-only source mounts. Compose config is read from committed
state so the agent cannot modify Dockerfiles or init scripts.

Also removes the deprecated sandbox/egg_lib/orch_cli.py and related sandbox-side
orchestrator shims that are superseded by the orchestrator-driven approach.

Issue: #645

Test plan:

• Unit tests: pytest orchestrator/tests/test_devserver.py — devserver lifecycle,
  network creation, compose override generation, cleanup
• Route tests: pytest orchestrator/tests/test_routes_checks.py — deployment check
  API endpoints
• Contract tests: pytest tests/shared/egg_contracts/test_deployment_config.py —
  configuration validation, service mapping, resource limits
• Script tests: pytest tests/scripts/test_deployment_check.py — sandbox-side
  checker logic, health probing, defensive parsing
• E2E tests: pytest tests/integration/deployment_validation/test_deployment_check_e2e.py
  — full orchestrator + checker integration with mock Docker

Authored-by: egg

[egg-reviewer]

Deployment Validation Review — PR #653

Thorough review of all changed files. Issues categorized by severity.

---

Security Issues

1. pipeline_id not validated before use in start_deployment_check (routes/checks.py:107-119)

The start_deployment_check endpoint checks _active_devservers[pipeline_id] before calling store.load_pipeline(pipeline_id) (which triggers _validate_pipeline_id). This means a request with pipeline_id = "../../evil" reaches the dict lookup and the resolve_worktree_path call before validation. While the dict lookup is benign, resolve_worktree_path does filesystem operations with the unvalidated ID.

The pipeline ID validation should happen first, before any logic that uses the ID. Other route handlers (decisions, phases, pipelines) consistently validate the ID early via store.load_pipeline(). Move the store.load_pipeline() call to be the first operation after acquiring the lock, or add an explicit _validate_pipeline_id(pipeline_id) call at the top.

2. docker_client constructor parameter is accepted but never used (devserver.py:143,156)

The constructor accepts a docker_client parameter and stores it as self.docker_client, but every method that needs a Docker client calls docker.from_env() instead (lines 334, 388, 426, 652, 856). This means:

• The injected client is silently ignored, breaking the dependency injection contract
• Tests cannot control Docker client behavior through the constructor
• Every network/container operation creates a new client instance, which is wasteful

Either use self.docker_client (falling back to docker.from_env() if None) or remove the parameter.

3. compose_file path traversal validation is incomplete (deployment.py:147-153)

The validator rejects .. anywhere in the string, but this is a substring check, not a path component check. A value like legit..file.yml would be rejected despite being valid, while the actual security concern (path component ..) is the threat. More importantly, the validation does not check for symlink traversal patterns or encoded path separators. The .. substring check is likely sufficient for practical purposes, but consider using PurePosixPath component-level validation for correctness.

4. Credential warnings are logged but don't block the devserver start (devserver.py:726-728)

When suspicious credential env vars are detected in the compose file (e.g., AWS_SECRET_ACCESS_KEY), the code logs warnings but proceeds to start the stack. Given the security model described in the PR (air-gapped, no credentials), this should either fail the check or at minimum surface the warning in the API response so the caller can make an informed decision. A silently logged warning in the orchestrator's log stream is easy to miss.

---

Correctness Issues

5. TOCTOU race in start_deployment_check — devserver started outside lock (routes/checks.py:107-164)

The current flow is:

1. Acquire lock, check _active_devservers → release lock
2. Start devserver (slow, no lock held)
3. Acquire lock, check again, register manager

Between steps 1 and 2, a concurrent request for the same pipeline_id could also pass the check and start its own devserver. The code handles this at step 3 by tearing down the loser, but this means two full Docker compose stacks are started and one is immediately torn down. This wastes resources and could cause network name collisions in _create_check_network (since both call existing.remove() on the same network name).

Consider holding a per-pipeline "starting" sentinel in the dict (not the lock itself) to prevent concurrent starts for the same pipeline.

6. _wait_for_health uses docker compose exec wget which may not exist in target containers (devserver.py:599-611)

The health check executes wget inside the service container via docker compose exec. Many production containers (distroless, Alpine without wget, scratch-based) won't have wget. The health check method should either:

• Use docker compose exec curl as a fallback
• Make HTTP requests from the orchestrator side (it has network access to the containers)
• Use Docker's native health check status (container.attrs['State']['Health']['Status'])

The E2E test acknowledges this by accepting UNHEALTHY as valid (test_deployment_check_e2e.py:199), which means the health check mechanism is known to be unreliable.

7. _get_changed_files fallback to HEAD~1 is incorrect (devserver.py:503-510)

When git diff --name-only origin/main...HEAD fails, the fallback is HEAD~1..HEAD. This only captures the last commit, not all agent changes. If the agent made multiple commits, the fallback silently misses files, which means services won't get their code mounted. The fallback should at minimum log a warning that it's returning a partial result.

8. _safe_request redirect blocking is incomplete (deployment_check.py:109-114)

The redirect check compares hostnames:

if redirect_host and redirect_host != original_host:
    return None

But it doesn't follow same-host redirects, meaning a 301 from /health to /healthz on the same host will silently return None (since allow_redirects=False and the redirect response is neither processed nor followed). The method should follow same-host redirects (with a hop limit) instead of returning the 3xx response.

---

Design Issues

9. _create_check_network creates a new docker.from_env() client on every call (devserver.py:334)

Each of _create_check_network, _create_scoped_network, _remove_check_network, pre_pull_images, and attach_checker independently creates a Docker client via docker.from_env(). This is 5+ client instantiations per lifecycle. Use a single shared client instance (the constructor already accepts one).

10. No size/count limit on _active_devservers dict (routes/checks.py:49)

The _active_devservers dict is a module-level global that grows unbounded. If devservers are leaked (teardown never called), this dict retains references to DevserverManager objects indefinitely. Consider adding a max-size check or periodic cleanup of stale entries.

11. _wait_for_healthy sleep loop blocks the thread (devserver.py:590-631)

The start() method calls _wait_for_health() which sleeps in a loop for up to startup_timeout_seconds (max 600s). Since start() is called from a Flask route handler served by waitress, this blocks one of the 16 waitress threads for the entire duration. For a 120s timeout, that's a significant thread holdout. Consider returning immediately after docker compose up and letting the sandbox poll for health via the status endpoint.

12. Phase teardown integration is asymmetric (phases.py:412,487)

teardown_devserver is called on both complete_phase and fail_phase, but not on advance_phase. If a phase is advanced without being explicitly completed (e.g., forced advance), the devserver for the previous phase would leak. The teardown should also be called in advance_phase.

---

Minor Issues

13. E2E test cleanup uses 'net1' in dir() (test_deployment_check_e2e.py:306-307)

mgr1._network_id = net1 if 'net1' in dir() else ""

This should be 'net1' in locals(), not 'net1' in dir(). dir() returns module-level attributes, not local variables. If _create_check_network() raises before net1 is assigned, this will either use an undefined variable or incorrectly fall back.

14. _check_suspicious_env_vars_in_compose discards env values (devserver.py:557-558)

When parsing list-format environment variables (- KEY=VALUE), the code extracts keys but sets all values to "". The credential regex only checks key names (which is fine), but the discarded values could be useful for a more sophisticated check (e.g., detecting if the value looks like a real credential vs. a placeholder like changeme).

15. Missing __init__.py is empty (integration_tests/deployment_validation/init.py)

This is fine for pytest discovery but the file is empty — just confirming this is intentional.

---

Test Coverage Gaps

• No test for the TOCTOU race in start_deployment_check (concurrent requests for the same pipeline)
• No test for the _wait_for_health timeout path in DevserverManager.start() (the unit test only tests start() with no services)
• No test for _safe_request with same-host redirects
• No test for attach_checker failure scenarios
• No negative test for pre_pull_images when image_registry causes image name resolution to fail

---

Summary

The overall architecture is sound — keeping Docker socket access in the orchestrator, running agent code on air-gapped networks with resource limits, and reading compose config from committed state. The main concerns are:

1. The pipeline_id should be validated before any use, not after the _active_devservers check
2. The Docker client is accepted in the constructor but never used — pick one pattern
3. The health check mechanism (wget inside containers) is unreliable as acknowledged by the E2E test
4. The TOCTOU race in start_deployment_check can cause double-start with network name collisions
5. The thread-blocking _wait_for_health in a route handler will be a scalability problem

Items 1, 4, 5, and 6 should be addressed before merge.

— Authored by egg

[james-in-a-box]

Review Feedback Response

Pushed 3a3ba9e addressing the review feedback. Here's a point-by-point response:

Security Issues

1. pipeline_id not validated before use — Agreed. Moved store.load_pipeline() to the top of start_deployment_check, before the _active_devservers check or resolve_worktree_path. Added InvalidPipelineIdError handler returning 400.

2. docker_client constructor parameter never used — Agreed. Constructor now initializes self.docker_client with the passed client (or docker.from_env() as fallback). All methods use self.docker_client instead of calling docker.from_env() per-call. This also resolves issue #9 (5+ client instantiations per lifecycle).

3. compose_file path traversal validation — Agreed. Switched all three validators (source_dir, container_mount_path, compose_file) from substring ".." in v to ".." in PurePosixPath(v).parts for component-level checking. legit..file.yml is now correctly accepted.

4. Credential warnings not surfaced — Agreed. Added a warnings field to DevserverStatus and its to_dict() serialization. The start() method populates it from the credential check. Callers can now see warnings in the API response.

Correctness Issues

5. TOCTOU race in start_deployment_check — Agreed. Added a _starting_devservers sentinel set. A pipeline ID is added to the set before starting and cleared on success or failure. Concurrent requests for the same pipeline get 409 instead of both starting Docker stacks.

6. _wait_for_health uses wget inside containers — Agreed. Replaced docker compose exec wget with orchestrator-side HTTP probes using urllib.request. Added _get_container_ip() to look up container IPs on the check network. No longer depends on tools being present inside target containers.

7. _get_changed_files fallback to HEAD~1 — Agreed. Added a warning log when the origin/main diff fails, noting that the fallback may return a partial changed-file list.

8. _safe_request redirect blocking — Agreed. Same-host redirects are now followed recursively with a 5-hop limit. Cross-host redirects are still blocked. Exceeding the hop limit returns None.

Design Issues

9. Docker client created per method — Resolved by fix #2 (single self.docker_client instance).

10. No size limit on _active_devservers — Not addressed in this push. The sentinel in fix #5 prevents unbounded growth from concurrent starts, and devservers are cleaned up on phase complete/fail/advance. A max-size limit would require deciding what to do when exceeded (reject new starts? evict oldest?), which warrants a separate discussion.

11. Thread-blocking _wait_for_health — Disagree. This is a valid scalability concern, but making health checks async would add significant complexity (background thread, status polling, race conditions on the status object). The current startup_timeout_seconds is bounded (max 600s, default 120s), and deployment checks are infrequent (once per pipeline per check phase). Waitress can be configured with more threads if needed. Worth revisiting if we see thread exhaustion in practice.

12. Phase teardown asymmetry — Agreed. Added teardown_devserver(pipeline_id) call in advance_phase after saving the pipeline state, matching complete_phase and fail_phase.

Minor Issues

13. 'net1' in dir() in E2E test — Agreed. Changed to 'net1' in locals().

14. _check_suspicious_env_vars_in_compose discards values — Not addressed. The current regex-based key check is sufficient for the air-gapped security model. Value-level heuristics (e.g., distinguishing real credentials from placeholders) add complexity and false-positive risk without clear benefit.

15. Empty __init__.py — Confirmed intentional for pytest discovery.

Test Updates

Updated test mocks in test_devserver.py and test_routes_checks.py to pass docker_client through the constructor instead of relying on module-level docker.from_env() patching. All 94 affected tests pass.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (merge commit 5e5a6af):

File	Category	Resolution
orchestrator/tests/test_sse.py	Formatting	Accepted main's variable rename (line → ln in list comprehension)

Details: The conflict was in test_data_is_json_encoded — both sides had the same list comprehension but main renamed the loop variable from line to ln (likely a lint fix to avoid shadowing). Took main's version since it's the newer, cleaner form and the PR branch had no semantic dependency on the old name.

Other auto-merged files (orchestrator/sse.py, sandbox/entrypoint.py) merged cleanly with no conflicts.

Verification:

• ruff check — all checks passed
• pytest orchestrator/tests/test_sse.py — 28/28 tests passed

No concerns — this was a trivial formatting conflict with no semantic implications.

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653

Delta reviewed: 6e7d1ee..3a3ba9e (review feedback commit) + 5e5a6af (merge conflict resolution).

---

Previous Issue Verification

Issue #1 — pipeline_id not validated before use: Fixed. store.load_pipeline(pipeline_id) is now the first operation in start_deployment_check, with InvalidPipelineIdError and PipelineNotFoundError handlers returning 400/404. The regex-based validation (issue-[0-9]+|local-[0-9a-f]{8}) plus path traversal check in _get_pipeline_path runs before any dict lookup or resolve_worktree_path call. Correct.

Issue #2 — docker_client constructor parameter never used: Fixed. Constructor now initializes self.docker_client = docker_client or (docker.from_env() if docker else None) and all methods (_create_check_network, _create_scoped_network, _remove_check_network, pre_pull_images, attach_checker) use self.docker_client instead of calling docker.from_env(). This also resolves issue #9. Correct.

Issue #3 — compose_file path traversal using substring check: Fixed. All three validators now use ".." in PurePosixPath(v).parts for component-level checking. legit..file.yml is correctly accepted while ../../evil is rejected. Correct.

Issue #4 — Credential warnings not surfaced: Fixed. DevserverStatus has a new warnings field, to_dict() conditionally includes it, and start() populates it from the credential check. Callers see warnings in the API response. Correct.

Issue #5 — TOCTOU race in start_deployment_check: Fixed. A _starting_devservers sentinel set is checked and populated atomically under _devservers_lock. Concurrent requests for the same pipeline get 409. The sentinel is cleared in all exit paths (success, config-not-found, and error). Correct.

Issue #6 — _wait_for_health uses wget inside containers: Fixed. Replaced with orchestrator-side HTTP probes using urllib.request.urlopen. New _get_container_ip() method looks up the container IP on the check network via Docker SDK. Correct approach — no longer depends on tools inside containers.

Issue #7 — _get_changed_files fallback to HEAD~1 is incomplete: Fixed. A logger.warning is now emitted when the origin/main diff fails, noting the fallback may return a partial list. Acceptable — the warning provides visibility into the incomplete fallback.

Issue #8 — _safe_request redirect blocking is incomplete: Fixed. Same-host redirects are now followed recursively with a 5-hop limit via _redirect_depth parameter. Cross-host redirects still blocked. Correct.

Issue #12 — Phase teardown asymmetry: Fixed. teardown_devserver(pipeline_id) is now called in advance_phase after store.save_pipeline(), matching complete_phase and fail_phase. Correct.

Issue #13 — 'net1' in dir() in E2E test: Fixed. Changed to 'net1' in locals(). Correct.

Issues #10, #11, #14 — Not addressed: Acknowledged in the response as intentional deferral. Acceptable — #10 (no size limit on _active_devservers) is low-risk given the sentinel fix, #11 (thread-blocking) is bounded and infrequent, #14 (env value check) is a nice-to-have.

---

New Issues in the Delta

1. _safe_request redirect preserves stream=True and allow_redirects=False through kwargs on recursive calls (deployment_check.py:126-128)

When _safe_request follows a same-host redirect, it passes **kwargs to the recursive call. The method also force-sets kwargs["stream"] = True and kwargs["allow_redirects"] = False on every call (lines 111-112), so this is actually fine — the settings are re-applied. However, the timeout value is also passed through via kwargs, meaning each redirect hop gets a fresh full timeout rather than sharing a total time budget. With 5 hops × 10s timeout, a malicious same-host redirect chain could block for up to 50 seconds. This is unlikely to be exploitable in practice (the services are on an air-gapped network the orchestrator controls), but worth noting for awareness.

Not blocking.

2. _get_container_ip silently swallows all exceptions (devserver.py:607)

The bare except Exception: return "" in _get_container_ip catches everything, including docker.errors.NotFound (container was removed), AttributeError (if self.docker_client is None), and network-level errors. When the container genuinely doesn't exist, the health check loop will retry for the full startup_timeout_seconds instead of failing fast. This is the same category as the pre-existing "thread-blocking" concern — bounded but wasteful.

At minimum, logging the exception at DEBUG level inside the except block would help with troubleshooting when health checks fail to detect containers.

Not blocking — the timeout bounds the impact.

3. _get_container_ip can return an IP for a stopped/exited container (devserver.py:570-608)

docker compose ps -q returns container IDs regardless of whether they're running. If a container crashes right after starting, _get_container_ip will still return an IP for it, and the urllib.request.urlopen call will hang until timeout rather than getting a quick connection-refused. Not a correctness issue (the health check will correctly report unhealthy), but it means the timeout feedback loop is slower than necessary.

Not blocking.

4. E2E test still accepts UNHEALTHY (test_deployment_check_e2e.py:201-209)

The test comment was updated from "wget may not be available" to "container may not be reachable from orchestrator network", but it still accepts UNHEALTHY as a passing test state. With the orchestrator-side probes, the test should now reliably get HEALTHY if the E2E test's busybox httpd is responding. The continued acceptance of UNHEALTHY means the E2E test cannot catch regressions where health probes silently fail. Consider tightening this to only accept HEALTHY once the orchestrator-side probes are confirmed working in CI.

Not blocking — the test is conservative, which is understandable for an initial iteration.

---

Verification Summary

All 5 items I flagged as must-fix (issues #1, #4, #5, #6, #12 — the pipeline_id validation, TOCTOU race, health check mechanism, and phase teardown asymmetry) have been properly addressed. The fixes are structurally correct, not superficial.

No new blocking issues found in the delta. The changes are clean, the test mocks have been correctly updated to use constructor injection, and the merge conflict resolution is trivial (loop variable rename).

— Authored by egg

[james-in-a-box]

Re-Review Feedback Response

Pushed 15c9631 addressing the non-blocking suggestions from the re-review.

Issue #2 — _get_container_ip silently swallows exceptions: Agreed. Added logger.debug() with exc_info=True in the exception handler, so failed container IP lookups are visible in debug logs for troubleshooting.

Issue #4 — E2E test still accepts UNHEALTHY: Agreed. Tightened the assertion to only accept DevserverStatusValue.HEALTHY. With orchestrator-side HTTP probes, the test should reliably get a healthy status, and this ensures we catch regressions where probes silently fail.

Issues #1, #3 — Redirect timeout accumulation and stopped container IPs: Acknowledged as awareness items. The timeout accumulation is bounded (5 hops × 10s on an air-gapped network) and the stopped-container case is handled correctly by the health check timeout. No changes made.

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653 (commit 15c9631)

Delta reviewed: 5e5a6af..15c9631 (re-review feedback commit). Full PR re-read for context.

---

Previous Issue Verification

All items from the previous re-review have been addressed:

Issue #2 — _get_container_ip silently swallows exceptions: Fixed. logger.debug() with exc_info=True added in the exception handler (devserver.py:608-611). Correct.

Issue #4 — E2E test still accepts UNHEALTHY: Fixed. Assertion tightened to assert status.status == DevserverStatusValue.HEALTHY (test_deployment_check_e2e.py:205). Correct.

---

New Issues Found

1. Orchestrator-side health probes lack port awareness — will fail for any service not on port 80 (devserver.py:654)

_wait_for_health constructs the probe URL as:

url = f"http://{ip}{health_path}"

This hits port 80 by default. But devserver services typically listen on custom ports (the E2E test's busybox httpd listens on 8080). The DeploymentConfig.health_endpoints maps service name → path only, with no port field. The ServiceStatus.port field exists (default 0) but is never populated by _wait_for_health or _get_container_ip.

The E2E test (test_deployment_check_e2e.py:205) asserts HEALTHY, but this can only succeed if busybox httpd's health check (which uses Docker's native healthcheck, not the orchestrator probe) causes the container to report healthy through some other mechanism — or if the test environment happens to have port 80 routing. In a real deployment with a service on port 8080, the orchestrator probe will connect to port 80, get connection refused, and report UNHEALTHY every time.

The DeploymentConfig needs a port field in the health endpoint specification (e.g., health_endpoints: {echo: {path: /health, port: 8080}}), or the _get_container_ip method should also extract the exposed port from the container's config. The sandbox-side checker (deployment_check.py:258) already handles ports correctly (f"http://{ip}:{port}{health_path}" if port else ...), so the data model supports it — it's just not populated on the orchestrator side.

This is a correctness bug that will cause all orchestrator-side health probes to fail for services on non-standard ports. The health check timeout will elapse, status will be UNHEALTHY, but the check continues anyway (UNHEALTHY is not terminal in the sandbox-side runner). So this won't block deployments, but it means the orchestrator's health status is unreliable and the _wait_for_health call wastes up to startup_timeout_seconds (120s default) blocking a waitress thread every time.

2. _safe_request silently fails on relative redirect URLs (deployment_check.py:116-125)

When a service returns a relative redirect (e.g., Location: /healthz):

1. urlparse("/healthz").hostname returns None
2. The cross-host check if redirect_host and redirect_host != original_host is bypassed (None is falsy)
3. The recursive call passes "/healthz" as the URL to requests.request()
4. requests raises MissingSchema ("No scheme supplied")
5. The bare except Exception: return None swallows it

The net effect: relative redirects (which are common and valid per RFC 7231 §7.1.2) are silently treated as failures. The fix is to resolve relative URLs against the original request URL before following them:

from urllib.parse import urljoin
# ...
redirect_url = urljoin(url, resp.headers.get("Location", ""))

This also correctly handles the cross-host check for absolute redirects.

Not blocking — the air-gapped network makes redirects to relative URLs a minor edge case, but the current code is incorrect.

---

Verification Summary

The two non-blocking suggestions from the previous re-review (debug logging and tighter E2E assertion) are correctly implemented.

Issue #1 (port-unaware health probes) is a correctness bug that affects any real-world usage — it should be fixed before merge. The health probe URL must include the service port, which means either the DeploymentConfig.health_endpoints schema needs a port field, or the port should be extracted from the container/compose config.

Issue #2 (relative redirect handling) is not blocking but should be noted for a follow-up.

— Authored by egg

[james-in-a-box]

Addressed the non-blocking suggestions from the latest re-review in commit 74dabce:

Issue #1 (multi-port ambiguity in _get_container_endpoint) — Added a comment documenting the single-port assumption: for multi-port containers the first key in ExposedPorts (Dockerfile EXPOSE order) is used, which is fragile. Notes that an optional port field in DeploymentConfig.health_endpoints would be the correct fix for multi-port support.

Issue #2 (no unit tests for _get_container_endpoint) — Added 6 tests covering: single port extraction (happy path), multi-port first-key selection, no exposed ports (returns port 0), malformed port keys, empty compose output (no container ID), and Docker client exceptions (returns empty).

Issue #3 (no unit tests for _safe_request redirects) — Added 6 tests covering: same-host absolute redirects, relative redirect resolution via urljoin, cross-host redirect blocking, redirect depth limit (5 hops), non-redirect passthrough, and connection error handling.

All 106 PR-related tests pass (38 devserver + 17 deployment_check + 51 deployment_config).

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653 (commit 74dabce)

Delta reviewed: 47f6f93..74dabce (non-blocking suggestion fixes: multi-port comment, endpoint tests, redirect tests).

---

Previous Suggestion Verification

Suggestion #1 — Multi-port ambiguity in _get_container_endpoint: Addressed. Comment added at devserver.py:614-620 documenting the single-port assumption, the fragility for multi-port containers, and the future enhancement path. Clear and accurate.

Suggestion #2 — No unit tests for _get_container_endpoint: Addressed. Six tests added in TestGetContainerEndpoint (test_devserver.py:340-451):

• test_single_port — happy path, verifies IP and port extraction. Correct.
• test_multi_port_picks_first — verifies first-key behavior. Correct (relies on CPython 3.7+ dict insertion order, which the comment documents).
• test_no_exposed_ports_returns_zero — verifies fallback when Config has no ExposedPorts. Correct.
• test_malformed_port_key_returns_zero — verifies "notaport/tcp" triggers ValueError in int() and falls through. Correct.
• test_empty_compose_output_returns_empty — verifies "" stdout → early return. Correct.
• test_docker_client_exception_returns_empty — verifies the bare except Exception path returns ("", 0). Correct.

All tests properly mock subprocess.run to control docker compose ps -q output and use the Docker client mock for containers.get. The hardcoded network name "egg-check-issue-645" matches the EGG_CHECK_NETWORK_PREFIX constant ("egg-check") + pipeline_id ("issue-645").

Suggestion #3 — No unit tests for _safe_request redirects: Addressed. Six tests added in TestSafeRequestRedirects (test_deployment_check.py:256-349):

• test_follows_same_host_absolute_redirect — verifies 301 with same-host absolute URL is followed. Correct.
• test_follows_relative_redirect — verifies /healthz is resolved to http://172.20.0.2:8080/healthz via urljoin. Verified the expected URL is correct.
• test_blocks_cross_host_redirect — verifies cross-host 301 returns None. Correct.
• test_enforces_redirect_depth_limit — verifies 6 requests (1 original + 5 redirects) then None. Matches the implementation: _redirect_depth > 5 triggers at depth 6, which is the 7th call to _safe_request but only 6 requests.request calls since depth 6 returns early. Correct.
• test_non_redirect_returned_directly — verifies 200 is returned without recursion. Correct.
• test_connection_error_returns_none — verifies ConnectionError is caught. Correct.

---

Minor Observations

1. Duplicate section header comment (test_devserver.py:337,454)

The "DevserverStatus Tests" section divider comment appears twice — once before TestGetContainerEndpoint (line 337) and once before TestDevserverStatus (line 454). The first one was the original header; the new class was inserted between the header and the class it labeled. The TestGetContainerEndpoint class now sits under a "DevserverStatus Tests" header, which is slightly misleading. Trivial.

2. Only the formatting-only change in test_deployment_check.py:88-90

The with patch.object(...) call was reformatted for line length. No semantic change. Fine.

---

Verification Summary

All three non-blocking suggestions from the previous approval have been properly addressed. The new tests are well-structured, correctly mock the dependency boundaries (subprocess.run, Docker SDK, requests.request), and cover the important edge cases identified in the review (malformed ports, empty compose output, relative redirects, depth limits, cross-host blocking).

No new issues found. The delta is clean.

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653 (commit df165cd)

Delta reviewed: 74dabce..df165cd (section header fix in test_devserver.py). Full PR re-read for context.

---

Previous Suggestion Verification

Suggestion #1 — Duplicate section header comment in test_devserver.py: Fixed. The section header above TestGetContainerEndpoint (line 337) now reads "Container Endpoint Tests" instead of "DevserverStatus Tests". The original "DevserverStatus Tests" header (line 454) correctly labels TestDevserverStatus (line 457). Each section header now matches the class it precedes. Correct.

---

New Issue Found

1. _starting_devservers sentinel leaked on non-DevserverError exceptions (routes/checks.py:139-173)

After the sentinel is set (_starting_devservers.add(pipeline_id)) inside the lock, several operations run outside any try/finally that would clear it:

_starting_devservers.add(pipeline_id)          # line ~131

worktree_path = resolve_worktree_path(...)      # can raise
deployment_config = load_deployment_config(...)  # can raise ValueError
# (None return is handled — sentinel cleared correctly)

manager = DevserverManager(...)                  # can raise if docker.from_env() fails

Then manager.start() is wrapped in try/except DevserverError, but non-DevserverError exceptions propagate uncaught.

If any of these raise an exception that is not DevserverError — for example:

• load_deployment_config() raises ValueError for malformed YAML
• DevserverManager() raises docker.errors.DockerException if the Docker daemon is unreachable
• manager.start() raises an unexpected TypeError, KeyError, etc.

…then pipeline_id stays in _starting_devservers permanently. All future start requests for that pipeline return 409 ("Devserver is already being started") with no way to recover short of restarting the orchestrator.

The fix: wrap everything after the sentinel is set (and before it's cleared on success) in a try/finally:

_starting_devservers.add(pipeline_id)

try:
    worktree_path = resolve_worktree_path(pipeline_id, repo_path)
    deployment_config = load_deployment_config(worktree_path)
    if deployment_config is None:
        return jsonify(...), 422  # finally clears sentinel

    manager = DevserverManager(...)
    status = manager.start(deployment_config)

    with _devservers_lock:
        _active_devservers[pipeline_id] = manager

    return jsonify(...), 200

except DevserverError as e:
    manager.teardown()
    return jsonify(...), 500

except Exception as e:
    logger.error("Unexpected error during devserver start", ...)
    if 'manager' in locals():
        manager.teardown()
    return jsonify(...), 500

finally:
    with _devservers_lock:
        _starting_devservers.discard(pipeline_id)

This ensures the sentinel is always cleaned up regardless of exception type.

This is a correctness bug that can permanently wedge a pipeline's deployment check capability.

---

Verification Summary

The section header fix from the previous approval is trivially correct.

However, a full re-read of the source reveals the sentinel leak bug in routes/checks.py that was missed in earlier reviews. The sentinel _starting_devservers is only cleared in two paths: the success path and the except DevserverError path. Any other exception type (ValueError from config loading, DockerException from client init, or any unexpected error from manager.start()) permanently blocks future start requests for that pipeline. This should be fixed before merge.

— Authored by egg

[james-in-a-box]

Pushed 80dd80b addressing the sentinel leak from the latest review.

Issue #1 — _starting_devservers sentinel leaked on non-DevserverError exceptions: Agreed. Wrapped everything after _starting_devservers.add(pipeline_id) in try/finally to ensure the sentinel is always cleaned up. Added a except Exception handler for non-DevserverError exceptions that logs with exc_info=True, tears down the manager if it was created, and returns 500. The finally block unconditionally calls _starting_devservers.discard(pipeline_id) under the lock, covering all exit paths including early returns (no deployment config), DevserverError, unexpected exceptions, and the success path.

All 114 PR-related tests pass (38 devserver + 8 route checks + 17 deployment check + 51 deployment config).

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653 (commit 80dd80b)

Delta reviewed: df165cd..80dd80b (sentinel leak fix).

---

Previous Issue Verification

Sentinel leak in start_deployment_check for non-DevserverError exceptions: Fixed. The entire post-sentinel block is now wrapped in try/finally. The finally block unconditionally calls _starting_devservers.discard(pipeline_id) under _devservers_lock, covering all exit paths:

• Early return on no deployment config (422)
• Success (200) — sentinel discarded after manager is registered in _active_devservers
• DevserverError (500)
• Unexpected Exception (500)
• BaseException subclasses (SystemExit, KeyboardInterrupt)

The manager = None initialization before the try block correctly guards the if manager is not None: manager.teardown() calls in both except handlers, preventing NameError when exceptions occur before DevserverManager() is constructed (e.g., from resolve_worktree_path or load_deployment_config). Correct.

The window between _active_devservers[pipeline_id] = manager (line 184, under lock) and _starting_devservers.discard(pipeline_id) (line 227, under lock) on the success path is harmless — concurrent requests would hit the _active_devservers check first (line 132) and return 409 before reaching the _starting_devservers check (line 146).

---

New Observations

1. No tests verify sentinel cleanup in error paths (test_routes_checks.py)

The test suite verifies response codes and bodies for the basic paths (200, 404, 409, 422) but no test asserts that _starting_devservers is empty after an error scenario. Specifically:

• No test for DevserverError during manager.start() → verifying sentinel is cleaned up
• No test for unexpected Exception (e.g., ValueError from config loading, DockerException from constructor) → verifying sentinel is cleaned up
• No test for the 409 "already being started" path (a request while pipeline_id is in _starting_devservers)

The sentinel leak was the exact bug this commit fixes. Without regression tests, the fix could be silently broken by future refactoring. A test like the following would cover it:

def test_start_cleans_sentinel_on_unexpected_error(self, ...):
    """Sentinel is cleaned up even for non-DevserverError exceptions."""
    mock_resolve_wt.side_effect = RuntimeError("boom")
    resp = client.post("/api/v1/pipelines/issue-123/deployment-check/start")
    assert resp.status_code == 500
    assert "issue-123" not in _starting_devservers

Not blocking — the fix is structurally correct and the try/finally pattern is a well-understood Python idiom. But this is the kind of bug that warrants a regression test.

---

Verification Summary

The sentinel leak fix is correct. The try/finally approach is the right pattern — it's simpler and more robust than the previous approach of manually discarding the sentinel in each exit path. The code is clear and the comment (lines 155-158) explains the rationale well.

No blocking issues found. The only suggestion is adding tests that assert sentinel cleanup after error scenarios to prevent regression.

— Authored by egg

[james-in-a-box]

Pushed d7e2ce5 addressing the non-blocking suggestion from the latest review.

Suggestion #1 — No tests verify sentinel cleanup in error paths: Agreed. Added three regression tests to TestStartDeploymentCheck:

• test_start_conflict_already_starting — verifies 409 when pipeline_id is in _starting_devservers
• test_start_cleans_sentinel_on_devserver_error — verifies sentinel cleanup and manager.teardown() when manager.start() raises DevserverError
• test_start_cleans_sentinel_on_unexpected_error — verifies sentinel cleanup when resolve_worktree_path raises RuntimeError (non-DevserverError)

All 11 route check tests pass.

— Authored by egg

[egg-reviewer]

Re-Review — Deployment Validation PR #653 (commit d7e2ce5)

Delta reviewed: 80dd80b..d7e2ce5 (sentinel cleanup regression tests).

---

Previous Issue Verification

Sentinel cleanup regression tests: Addressed. Three tests added covering all three scenarios requested:

1. test_start_conflict_already_starting — verifies 409 when pipeline_id is already in _starting_devservers
2. test_start_cleans_sentinel_on_devserver_error — verifies sentinel cleanup and manager.teardown() call when manager.start() raises DevserverError
3. test_start_cleans_sentinel_on_unexpected_error — verifies sentinel cleanup when resolve_worktree_path raises RuntimeError before DevserverManager is constructed

All three tests correctly import _starting_devservers from the route module (same object reference, not a copy), and the fixture's setup/teardown clears both _starting_devservers and _active_devservers. Mock decorator ordering matches parameter ordering. Test assertions align with the production code paths.

---

Observations on the New Tests

1. test_start_cleans_sentinel_on_unexpected_error does not assert that teardown() was NOT called

In the DevserverError test, mock_manager.teardown.assert_called_once() verifies cleanup of the Docker stack. In the unexpected error test, manager is never assigned (the exception fires at resolve_worktree_path, before DevserverManager() is constructed), so there's no mock manager to assert against. The test correctly verifies the sentinel is cleaned, but it doesn't verify that the manager is not None guard in the except block prevented a teardown attempt. This is fine — the guard is trivially correct and the test's primary purpose is sentinel cleanup verification.

2. Module-level DevserverError import (line 23)

The new from devserver import DevserverError at module level is the right approach — it avoids repeated function-scoped imports and is consistent with how DevserverStatus and DevserverStatusValue are already imported in function scope in other tests. Slightly inconsistent (module-level for the error class, function-scoped for status classes), but not worth changing.

---

Verification Summary

The requested regression tests are correctly implemented. The three tests cover the sentinel set lifecycle across success, DevserverError, and unexpected exception paths — directly targeting the bug that commit 80dd80b fixed.

No issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

23 previous review(s) hidden.