test(integration): cover deployment-validation routes past auth gate (#2641)

[jwbron]

Closes #2641.
Closes #2646.
Closes #2647.
Closes #2648.
Closes #2652.

Summary

Adds integration_tests/test_deployment_validation_logic.py exercising the post-auth behaviour of POST /api/v1/deployment/validate-manifests and POST /api/v1/deployment/validate-network-isolation. The existing sibling test_k8s_deployment_tools.py covers @require_lifecycle_secret parity (401/503 on missing or wrong bearer) but stops at the auth gate — the lifecycle bearer wasn't surfaced through the shared EggStack fixture.

integration_tests/conftest.py now reads lifecycle-secret from gateway-secrets alongside launcher-secret and exposes a session-scoped lifecycle_secret fixture (skips cleanly when the Secret has no such key).

Bug fixes bundled (previously filed as follow-ups)

The original PR locked in the observable broken behaviour via xfail(strict=True) and "today's shape" assertions, and filed four follow-up issues. Those four are now fixed in this PR so the tests can assert the actual happy paths instead of the regressions; the bundled fixes are small, targeted, and the integration coverage exists in the same commit that flips them.

• Closes validate_network_isolation always short-circuits in production: orchestrator SA lacks kube-system DaemonSet RBAC #2646 — Add ClusterRole egg-cluster-topology-reader granting the orchestrator SA get,list on apps/daemonsets and cluster-scoped nodes (k8s/base/rbac.yaml). _detect_cni / _detect_k3s now run against a real cluster instead of returning null/false.
• Closes validate_deployment_manifests is unreachable in production: orchestrator container has no kustomize/kubectl #2647 — Install pinned kustomize (v5.6.0) in the orchestrator image (orchestrator/Dockerfile). _run_kustomize no longer raises kustomize_unavailable. The secondary gap noted in validate_deployment_manifests is unreachable in production: orchestrator container has no kustomize/kubectl #2647 (egg repo not bind-mounted under the CI overlay) is acknowledged out of scope; the CI default-overlay path therefore returns 404 (overlay not found) post-fix while the local-overlay happy path returns 200.
• Closes get_deployment_context reports images_unavailable: orchestrator SA missing 'list' verb on Deployments #2648 — Add list to the verbs on apps/deployments in the egg-service-log-reader Role (k8s/base/rbac.yaml). _collect_egg_image_tags now returns populated tags; get_deployment_context no longer reports images_unavailable: true.
• Closes validate_network_isolation probe field 'orchestrator_direct_blocked' is misleading #2652 — Rename probe field orchestrator_direct_blocked → orchestrator_api_reachable and flip its polarity in PROBE_COMMAND_TEMPLATE (orchestrator/routes/deployment.py). The agent→orchestrator heartbeat path is deliberately permitted by allow-agent-to-orchestrator, so the field now reads positively. Callers updated: orchestrator/mcp_tools.py, orchestrator/tests/test_mcp_tools.py, orchestrator/tests/test_deployment_routes.py, docs/reference/mcp-deployment-tools.md, skills/agent-diagnose/SKILL.md.

Test coverage

Class	Concern
TestValidateDeploymentManifestsLogic	404 on missing overlay, 400 on absolute traversal, 400 on relative ../ traversal, default-overlay returns 200 (repo mounted) or 404 (CI / unmounted), idempotent re-validation.
TestValidateNetworkIsolationLogic	400 on K8s-invalid pipeline_id/role, defaults accepted, regex boundary cases (parametrized: leading/trailing punctuation, ≤63/>63 length, uppercase, dot/underscore mid-token), and the happy-path probe-shape assertion (no longer xfail — _detect_cni now resolves to ("calico", True) against the integration cluster).
TestValidationRouteConcurrency	5-way parallel fan-out across both routes — no 5xx/409 contention, distinct probe-IDs.
TestValidationRouteSelfConsistency	Bearer prefix never echoed in error bodies; malformed JSON never produces a Flask traceback.
TestProbeJobCleanup	No orphan probe Jobs remain in egg-agents after the route returns.

validate_config (the third route named in #2641) is deliberately out of scope for the k3s tier — it's an MCP-side Pydantic handler with no HTTP route. Its coverage already exists in orchestrator/tests/test_mcp_tools.py::TestValidateConfig. The test module's docstring explains this.

Constraint per #2474

Per the constraint write-up in #2474, the SDLC sandbox can't bring up k3s. These tests are validated by CI's Test / aggregate integration tier; the canonical signal is the green required-check on this PR.

Test plan

• Test / aggregate integration job is green.
• No regressions in the sibling test_k8s_deployment_tools.py 21-case auth-reject suite.
• test_probe_runs_and_returns_expected_shape passes (the xfail marker is gone now that validate_network_isolation always short-circuits in production: orchestrator SA lacks kube-system DaemonSet RBAC #2646 is bundled).
• test_default_overlay_returns_404_or_200_depending_on_repo_mount returns 404 in CI (egg repo not bind-mounted) without 500 (kustomize is now in the image).

[egg-reviewer]

Review summary

The integration coverage is well-targeted and the bundled bug fixes are reasonable in scope. I'm posting this as --comment (this is a self-authored PR, see gh wrapper behavior). The probe-template change has a real bug worth fixing before merge; everything else is non-blocking polish or pre-existing nit.

Blocking-grade issue (please fix before merge)

1. New comment block in PROBE_COMMAND_TEMPLATE introduces unintended command substitution in the probe pod

orchestrator/routes/deployment.py:947-952 — the probe script runs as /bin/sh -c PROBE_COMMAND_TEMPLATE, and the script contains a python3 - <<PY ... PY heredoc with an unquoted delimiter. POSIX shells perform variable expansion, command substitution, and backslash escapes on the body of an unquoted heredoc. You're relying on $gw / $internet / $orch / $peer interpolation, which is fine — but the new comment lines you added contain backticks:

# `allow-agent-to-orchestrator` (k8s/base/network-policies.yaml)
# ...
# field was previously named `orchestrator_direct_blocked` with

/bin/sh evaluates those as command substitution. I reproduced this exactly outside the cluster:

$ /bin/sh /tmp/probe.sh 2>&1
/tmp/probe.sh: 1: allow-agent-to-orchestrator: not found
/tmp/probe.sh: 1: orchestrator_direct_blocked: not found
{"gateway_reachable": true, "internet_blocked": false, "agent_pods_unreachable": false, "orchestrator_api_reachable": true, "raw": {...}}

So the probe still emits correct JSON (and _parse_probe_output happens to be robust here because it iterates reversed(splitlines()) and finds the JSON line first), but every probe pod now writes two sh: ...: not found lines to stderr. That ends up in the pod logs the operator scrapes, breaks the "no stderr noise" assumption around the probe, and is one fragile rename away from corrupting the JSON line itself (e.g. if anyone adds a \date`or`hostname`` somewhere — those will substitute non-empty strings into the Python source).

The tests in this PR don't catch this — test_probe_runs_and_returns_expected_shape only inspects the parsed JSON, and test_template_references_expected_env_vars only does substring checks. Worth a unit test that runs PROBE_COMMAND_TEMPLATE under /bin/sh -n (or asserts no ``` in the heredoc body) so it doesn't silently regress.

Fix: drop the backticks from the comment (they only formatted as markdown for human readers anyway), or use plain single-quotes, or quote the heredoc delimiter as <<'PY' and pass gw=…orch=… via env vars to the Python child instead of doing template substitution. The first option is by far the cheapest.

Non-blocking suggestions

2. kustomize download has no integrity check

orchestrator/Dockerfile:14-19 — curl -fsSL … | tar -xz pulls a release binary from GitHub with no checksum or signature verification. Kustomize ships checksums.txt and a GPG signature alongside every release; fetching the checksum and running sha256sum -c is a 2-line addition and would protect against a release mirror compromise or in-flight tampering on the build node. I'll note this is consistent with the existing pattern in sandbox/Dockerfile (claude install via curl|bash), so it's defense-in-depth, not a regression. Worth a follow-up.

3. egg-cluster-topology-reader over-scopes the daemonset grant

k8s/base/rbac.yaml:108-126 — the new ClusterRole grants cluster-wide get/list on apps/daemonsets, but _detect_cni and _detect_k3s only call list_namespaced_daemon_set("kube-system"). nodes need cluster scope (cluster-scoped resource), but the daemonset rule could live in a Role + RoleBinding scoped to kube-system. Splitting them better matches least-privilege, and the cluster-wide read on daemonsets isn't useful for anything else the orchestrator does today.

4. test_validation_routes_reject_invalid_json doesn't catch the regression it names

integration_tests/test_deployment_validation_logic.py:564-594 — the docstring says the test guards against a switch from get_json(silent=True) to non-silent get_json(), which would 500 with a Flask traceback. But Flask's default behaviour on malformed JSON with get_json() (non-silent) is to raise werkzeug.exceptions.BadRequest, which renders as a 400 with a plain {"error": "..."} body — no Traceback string anywhere. The current assertion would silently pass through that regression.

To match the stated intent either (a) assert the status code stays in {200, 400} with the route's expected error message body rather than the bare-Flask 400 page, or (b) compare the response to a baseline empty-body call.

5. test_no_orphan_probe_jobs_after_call selects every probe Job in the namespace

integration_tests/test_deployment_validation_logic.py:633-660 — -l egg.probe=true matches every concurrent probe, including ones launched by TestValidationRouteConcurrency::test_concurrent_validate_network_isolation_calls_get_distinct_probe_ids if it ran moments earlier in the same session and any of those Jobs is still in ttlSecondsAfterFinished-pending state. The test will then "fail" attributing the leak to its own call. Easy fix: scope the selector to this test's pipeline_id (-l egg.pipeline.id=cleanup-2641 is already on the manifest).

6. test_default_overlay_returns_404_or_200_depending_on_repo_mount accepts an ambiguous outcome

integration_tests/test_deployment_validation_logic.py:168-207 — both 200 and 404 pass. In CI (where 404 is the expected shape) this means a regression that flips the happy path from 200 to 404 in local-dev would still be green in CI. Since CI is the canonical signal per #2474, that's a real coverage hole. Consider asserting on an env signal you control (e.g. assert resp.status_code == 200 if Path("/home/egg/repos/egg/k8s/overlays/local").exists() else 404) so the test pins exactly one outcome per environment.

7. Pre-existing Python-2-looking except X, Y: in files this PR touches

You're already in these files; small drive-by fix:

• integration_tests/conftest.py:168 — except FileNotFoundError, subprocess.TimeoutExpired:
• orchestrator/routes/deployment.py:821 — except OSError, RuntimeError:

Both compile (Python 3 parses the comma as a tuple expression, so both exceptions are caught) but every reader's first read is "this is the Python 2 as form, the second name is a variable". Wrap the types in parens — except (X, Y): — and the ambiguity disappears.

8. test_validation_routes_never_leak_secrets_in_error_messages only checks the secret prefix

integration_tests/test_deployment_validation_logic.py:536 — secret_snippet = lifecycle_secret[:16]. A bug that echoed back the suffix of the bearer wouldn't be caught. Cheap upgrade: check the full secret and a few rotations of it (or just assert lifecycle_secret not in resp.text — 64 hex chars is more than enough entropy that a false positive is impossible).

Things I checked and am satisfied with

• All consumers of orchestrator_direct_blocked → orchestrator_api_reachable rename are updated. Only references that survive are explicit "renamed-from" mentions in user-facing docs (docs/reference/mcp-deployment-tools.md:333) and PR-traceable planning artifacts under .egg-state/. No live consumer left on the old name.
• Polarity flip is correct: allow-agent-to-orchestrator permits agent→orch:9849, so orchestrator_api_reachable=True matches healthy state.
• _K8S_LABEL_VALUE_RE boundary parametrize is sound — the regex caps total length at 63 chars, and the test cases hit each corner (x*63 is valid, x*64 is invalid, leading/trailing - rejected, Pipe1 confirms the K8s label-value allowance for uppercase).
• Concurrent-fanout test uses uuid.uuid4().hex[:12] (48 bits) per probe; collision risk is negligible at the 5-way fan-out.
• lifecycle_secret fixture cleanly skips when the cluster's gateway-secrets lacks the key, and .github/workflows/test-integration.yml:116 seeds it on every CI run so the integration tier will actually exercise the new tests.
• The new ClusterRole + binding is correctly bound to the orchestrator SA in egg-system. No new tokens minted.
• Lifecycle bearer is base64-decoded and .strip()-ed — matches the existing pattern for launcher_secret (which had a real \n-in-header bug per the conftest comment).

— Authored by egg

[egg-reviewer]

Re-review summary

Delta since my previous review (0bb9c45):

• a83cb1f — one-line fix to PROBE_COMMAND_TEMPLATE: drops --show-error, removes the || echo 000 fallback, redirects stderr to /dev/null, and adds || true.
• 9952381 — clean merge from main.

Posting as --comment (self-authored PR; non-blocking note + outstanding nit). Verdict on the new commit: correct, and the right fix.

Things I verified about the new commit

The commit-message theory ("curl -w '%{http_code}' already outputs '000' on failure, so || echo 000 was double-appending") is correct. Confirmed locally that curl --silent --max-time 3 -o /dev/null -w '%{http_code}' writes 000 to stdout on all three failure modes a network probe will hit:

• Timeout (exit 28): 000
• DNS failure (exit 6): 000
• Connection refused (exit 7): 000

So after the fix:

• Egress allowed → curl writes a real status, exit 0, "$internet" == "000" is False, internet_blocked: False. ✓
• Egress denied → curl writes 000, exits nonzero, || true absorbs the exit, captured value is "000", internet_blocked: True. ✓

The previous behavior would write 000000 for the blocked case and flip internet_blocked to False — which is the exact regression test_probe_runs_and_returns_expected_shape (integration_tests/test_deployment_validation_logic.py:428) asserts against. So the integration suite will catch the regression if it ever returns.

Minor notes on the new line:

• 2>/dev/null is redundant given --silent and the removal of --show-error — but harmless and a reasonable belt-and-suspenders.
• || true is also currently redundant (the script uses set -u, not set -e) but is a sensible guard against a future set -e addition propagating through the $(probe …) substitution.

Outstanding from my previous review (still unaddressed in this delta)

Backticks in the heredoc comment block are still being executed by /bin/sh

orchestrator/routes/deployment.py:947-952 — the <<PY heredoc is unquoted, so the shell still performs command substitution on the backticks in the new comment block. Reproducing against a83cb1f:

$ /bin/sh /tmp/probe.sh 2>&1
/tmp/probe.sh: 1: allow-agent-to-orchestrator: not found
/tmp/probe.sh: 1: orchestrator_direct_blocked: not found
{"gateway_reachable": true, "internet_blocked": true, "agent_pods_unreachable": true, "orchestrator_api_reachable": true}

Result: every probe pod still writes two sh: …: not found lines to its container log. read_namespaced_pod_log (orchestrator/routes/deployment.py:1120) returns the merged container log, and _parse_probe_output (:1144) iterates reversed(splitlines()) and finds the JSON line first — so the parse still succeeds today, but:

1. The pod-log noise is undesirable on its own (operator scraping, log-volume budget).
2. The JSON is one fragile rename away from corrupting: a future edit that puts `date`, `hostname`, `$(…)`, or anything that resolves to a non-empty command output into a backtick-delimited word inside the heredoc will substitute into the Python source itself, not just into comments.

Cheapest fix (one character per backtick): drop the backticks from the comment lines — they were only formatting hints for human readers anyway. Alternative: quote the heredoc delimiter as <<'PY' and pipe $gw / $orch / $internet / $peer into the Python child via env vars or argv. Either is fine; the first is one diff hunk.

A unit-test guard would also be cheap — e.g. assert "\" not in PROBE_COMMAND_TEMPLATEalongside the existingTestProbeCommandTemplateshape guards inorchestrator/tests/test_deployment_routes.py:1461`. The existing assertions only check substring presence and won't catch this.

Non-blocking suggestions from my prior review

The non-blocking items from my prior review (Dockerfile kustomize-checksum, ClusterRole scope on apps/daemonsets, test_validation_routes_reject_invalid_json doesn't catch what its docstring claims, etc.) remain unaddressed. None are merge-blockers and I'm not re-listing them in detail — see the prior review at 0bb9c45 for the full list.

— Authored by egg

[egg-reviewer]

Re-review summary

Delta since the previous review at a83cb1f:

• 9952381 — clean merge from main. No conflict resolution.

I diffed the PR's contribution against origin/main at both heads — git diff origin/main...a83cb1f and git diff origin/main...9952381 — and the textual diff is byte-identical (only the Dockerfile blob index sha changes because main moved). There are no new PR-authored changes to review.

Posting as --comment (self-authored PR + restating an outstanding non-blocking nit).

Merge from main is clean

The interesting intersection is orchestrator/Dockerfile, where main added a COPY config/repo_config.py ./repo_config.py block (#2528 / #2561) and the PR adds the kustomize install ARG/RUN block higher up. The merged result (orchestrator/Dockerfile:8-19 for the PR's block, :44-50 for main's) keeps both in their natural positions and doesn't reorder or drop either. No regression introduced.

I spot-checked the other files main touched that overlap conceptually with the PR (orchestrator/routes/pipelines.py, shared/egg_restrictions/patterns.py) and the PR doesn't touch them — so no cross-cutting concerns from the merge.

Outstanding from the previous review (still unaddressed)

Backticks in the heredoc comment block are still executed by /bin/sh

orchestrator/routes/deployment.py:947-952 — the <<PY heredoc is still unquoted, and the comment lines still contain `allow-agent-to-orchestrator` and `orchestrator_direct_blocked`. /bin/sh evaluates those as command substitution before passing the body to python3, so every probe pod continues to write sh: …: not found lines to its container log. The JSON parse still works (_parse_probe_output finds the JSON line via reversed(splitlines())), but the pod log noise is the visible symptom and the bigger concern is the foot-gun: any future edit that puts a backtick around something that resolves to non-empty output (`date`, `hostname`, `$(…)`) substitutes into the Python source itself.

Cheapest fix: strip the backticks from the comment lines — they were only markdown hints for human readers. A one-line guard in orchestrator/tests/test_deployment_routes.py (assert "\" not in PROBE_COMMAND_TEMPLATE) alongside the existing TestProbeCommandTemplate` shape guards would prevent regressions cheaply.

Other non-blocking items from prior reviews still open

Carried over from 0bb9c45 (the initial review), none merge-blocking, see that review for detail:

• orchestrator/Dockerfile:14-19 — no checksum verification on the kustomize tarball.
• k8s/base/rbac.yaml:108-126 — apps/daemonsets cluster-scoped grant; usage is namespaced to kube-system.
• integration_tests/test_deployment_validation_logic.py:564-594 — test_validation_routes_reject_invalid_json does not catch the regression its docstring names (Flask's non-silent get_json() returns 400 cleanly, not 500 with a traceback).
• integration_tests/test_deployment_validation_logic.py:633-660 — test_no_orphan_probe_jobs_after_call selects every egg.probe=true Job in the namespace; should scope to this test's pipeline_id.
• integration_tests/test_deployment_validation_logic.py:168-207 — test_default_overlay_returns_404_or_200_depending_on_repo_mount accepts an ambiguous outcome; could pin to one expected value per environment.
• integration_tests/conftest.py:168 and orchestrator/routes/deployment.py:821 — except X, Y: reads as Python 2 syntax even though it's a valid Python 3 tuple; parenthesizing eliminates the ambiguity.
• integration_tests/test_deployment_validation_logic.py:536 — secret-leak check only inspects the first 16 chars of the secret.

— Authored by egg

[james-in-a-box]

Response to review feedback

Thanks for the three rounds of careful review. Disposition per item from the initial review at 0bb9c45 (and the re-reviews at a83cb1f / 9952381, which re-raised the same outstanding nits):

Blocking-grade

1. PROBE_COMMAND_TEMPLATE backticks — fixed-in-PR (commit 77c3317).
   Stripped the backticks from the comment block in
   orchestrator/routes/deployment.py:947-952, and added a NOTE inside
   the heredoc explaining why future edits must avoid them. Added two
   unit-test guards in orchestrator/tests/test_deployment_routes.py
   under TestProbeCommandTemplate:

   • test_template_contains_no_backticks — cheap substring guard you
     suggested verbatim.
   • test_template_is_shell_syntax_valid — runs the template through
     /bin/sh -n so any future quoting / heredoc-delimiter mistake
     surfaces in CI rather than in a probe pod.

   Verified locally: sh -n exits 0 on the new template, and the
   backtick assertion passes.

Non-blocking

2. kustomize download has no integrity check — deferred-to orchestrator Dockerfile: verify checksum on kustomize tarball install #2681.
   You explicitly labelled this one "Worth a follow-up" in the
   initial review and it's defense-in-depth rather than a regression
   (the existing sandbox/Dockerfile claude install via curl|bash
   has the same shape). Filed orchestrator Dockerfile: verify checksum on kustomize tarball install #2681 with the proposal (pin
   KUSTOMIZE_SHA256 and sha256sum -c).

3. egg-cluster-topology-reader over-scopes daemonsets —
   fixed-in-PR (commit 77c3317). Split the ClusterRole so only
   nodes keeps cluster-wide read (it's a cluster-scoped resource);
   moved apps/daemonsets to a new namespaced Role +
   RoleBinding named egg-kube-system-topology-reader in
   kube-system — the only namespace _detect_cni / _detect_k3s
   actually query.

4. test_validation_routes_reject_invalid_json doesn't catch the
   regression it names — fixed-in-PR (commit 77c3317). You were
   right — Flask's non-silent get_json() raises BadRequest, which
   renders as a clean 400 with no Traceback substring, so the bare
   negative assertion was a no-op. Replaced with the option-(b) shape
   from your suggestion: malformed JSON must produce the same status

   • success-flag as an empty body, which is the contract
     get_json(silent=True) or {} actually guarantees. A switch to
     non-silent get_json() would diverge them (400 BadRequest vs the
     route's default-path response) and fail the assertion. Kept the
     cheap "no Flask traceback" guard as belt-and-braces.

5. test_no_orphan_probe_jobs_after_call selects every probe Job
   — fixed-in-PR (commit 77c3317). Scoped the kubectl selector
   to this test's pipeline_id
   (-l egg.probe=true,egg.pipeline.id=cleanup-2641) so a concurrent
   probe from TestValidationRouteConcurrency can't be misattributed
   as a leak. The label is already on the manifest in
   _build_probe_job_manifest.

6. test_default_overlay_returns_404_or_200_depending_on_repo_mount
   accepts an ambiguous outcome — fixed-in-PR (commit 77c3317).
   Pinned the expected status per environment by probing the same
   host bind-mount path the orchestrator pod sees:
   Path.home() / "repos/egg/k8s/overlays/local". In CI the path
   doesn't exist (per the workflow's seed step) so the test asserts
   404; locally with the egg repo cloned at ~/repos/egg it asserts
   200. A regression that flips 200↔404 in either environment now
   fails.

7. Pre-existing except X, Y: reads as Python 2 —
   disagree (PEP 758 + project formatter). I tried the
   parenthesization in both integration_tests/conftest.py:168 and
   orchestrator/routes/deployment.py:821, but ruff format (target
   = py314) actively normalises except (A, B): back to
   except A, B: per PEP 758, so the edit is reverted on every
   formatter pass:

   $ ruff --version
   ruff 0.15.12
   $ ruff format --diff /tmp/test.py
   -except (FileNotFoundError, OSError):
   +except FileNotFoundError, OSError:
   

   The Python-2-as reading no longer applies on Python 3.14+ where
   PEP 758 makes this the canonical form. Going against the
   formatter would be perpetual noise for every future PR that
   touches these files. Leaving as-is; if the project wants
   parenthesized form, that's a project-wide formatter-config change
   (unparenthesized-tuple-in-except = false or equivalent), not a
   PR-local edit.

8. test_validation_routes_never_leak_secrets_in_error_messages
   only checks the secret prefix — fixed-in-PR (commit 77c3317).
   Now checks (a) the full 64-hex-char lifecycle_secret and (b)
   four non-overlapping 16-char windows at offsets 0/16/32/48 — a
   bug echoing the suffix, middle, or any contiguous slice of the
   bearer would now surface.

Notes on the curl-fix re-review

Thanks for verifying the curl --max-time already-emits-000
theory on a83cb1f. Agree on both minor notes — 2>/dev/null and
|| true are redundant under the current shape but cheap
belt-and-braces for any future set -e or --show-error addition.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 2}

[egg-reviewer]

Re-review summary

Delta since the previous review at 9952381:

• 77c33171 — bot-authored: addresses six of the eight items from prior reviews (one blocking-grade, five non-blocking).
• 6113fe9a — clean merge from main. No conflict resolution.
• 0e1befc9 — Fix #2681: pins KUSTOMIZE_SHA256 and runs sha256sum -c before extracting the tarball.
• 4978666 (497866683) — Fix #2658: bypass kubernetes-python's ApiClient.deserialize() for the probe pod log read by passing _preload_content=False.

Posting as --comment (self-authored PR; one outstanding nit re-raised non-blocking, plus two new non-blocking concerns introduced by 4978666).

Things I verified about each new commit

77c33171 — addresses prior review feedback

• Backticks in PROBE_COMMAND_TEMPLATE (blocking item) — fixed. The comment lines at orchestrator/routes/deployment.py:947-956 no longer contain backticks, and a NOTE warns future editors that the heredoc is unquoted. Two unit-test guards in orchestrator/tests/test_deployment_routes.py:1487-1526 lock this in: test_template_contains_no_backticks asserts the cheap shape invariant, and test_template_is_shell_syntax_valid shells out to sh -n so any future heredoc / quoting mistake fails CI rather than the probe pod. Reproduced locally: the new template passes sh -n and emits no sh: …: not found lines when executed.
• RBAC over-scope on apps/daemonsets — fixed correctly. k8s/base/rbac.yaml:90-127 keeps nodes on the ClusterRole (cluster-scoped resource, no namespace-scoped alternative) and moves apps/daemonsets to a new Role + RoleBinding (egg-kube-system-topology-reader) in kube-system. Verified that orchestrator/routes/deployment.py:213 and :242 only call list_namespaced_daemon_set("kube-system"), so namespace-scoped is sufficient. The RoleBinding correctly references the orchestrator SA in egg-system.
• test_validation_routes_reject_invalid_json — fixed correctly. The test now compares the malformed-body response against an empty-body baseline (same status + same success flag), which is the actual invariant get_json(silent=True) or {} provides. The bare-Flask 400 BadRequest page that a non-silent regression would produce diverges on both axes. Belt-and-braces "no traceback" assertion retained.
• test_no_orphan_probe_jobs_after_call — fixed. Selector now scopes to egg.pipeline.id=cleanup-2641 (the label is set on the Job's metadata.labels in _build_probe_job_manifest, verified at orchestrator/routes/deployment.py:1014-1020). Concurrent probes from TestValidationRouteConcurrency can no longer be misattributed as a leak.
• test_default_overlay_returns_404_or_200_depending_on_repo_mount — fixed. Pinned to a single expected status per environment via Path.home() / "repos/egg/k8s/overlays/local". The CI workflow at .github/workflows/test-integration.yml:119 creates $HOME/repos empty (no clone), so the path will not exist and the test will assert 404; in local-dev the test runner's $HOME/repos/egg is the same physical directory the orchestrator pod sees via bind-mount, so the host-side .exists() check is the correct proxy.
• test_validation_routes_never_leak_secrets_in_error_messages — fixed. Checks the full 64-hex-char secret plus four non-overlapping 16-char windows at offsets 0/16/32/48. The lifecycle-secret is openssl rand -hex 32 per .github/workflows/test-integration.yml:127, so the 64-char assumption holds in CI.
• PEP 758 except X, Y: disposition — accepted. Producer's rationale is correct: ruff format (target-version = py314) normalises to the unparenthesized form per PEP 758, so any parenthesization would be reverted on every formatter pass. Reasonable decision to leave as-is.

0e1befc9 — kustomize tarball checksum

orchestrator/Dockerfile:17-21 — clean. The verification runs before extraction so a tampered tarball fails the build; the SHA256 is documented as published-with-release, and the lockstep-bump invariant is called out in the comment. Verified the URL pattern preserves the URL-encoded / in the tag (kustomize%2Fv${VERSION}), which is necessary because the release tag contains a literal /.

4978666 — bypass kubernetes-python auto-deserialization

orchestrator/routes/deployment.py:1122-1142 — diagnosis is correct. Confirmed against kubernetes-client/python: ApiClient.deserialize() calls json.loads(response.data) first and only then coerces to the declared response_type=str, so a JSON-shaped body becomes dict and then str(dict) = Python repr (single quotes, True/False/None). _preload_content=False returns the raw urllib3.HTTPResponse and bypasses that path. .data is a property that reads-to-EOF and caches; for the tiny probe-pod log size this is the right shape. errors="replace" is a sensible decode policy for arbitrary container logs.

New non-blocking concerns introduced by 4978666

1. Exception handler in _read_probe_log no longer covers the body-read

orchestrator/routes/deployment.py:1130-1142 — with _preload_content=False, the actual network read happens at .data access (line 1139), outside the try/except block. If urllib3 raises during the body read (connection reset mid-stream, decode failure on a malformed gzip layer, etc.), the exception propagates up and the route handler returns 500. Before this commit the deserializer also ran inside the kubernetes-python call so the same exception was caught — so this is a small regression in the failure envelope, not a new failure mode for the happy path.

Cheapest fix: extend the try to cover the .data access, or call raw.read() explicitly inside the existing try:

try:
    raw = k8s.core_api.read_namespaced_pod_log(
        name=pod_name, namespace=namespace, _preload_content=False
    )
    if raw is None:
        return ""
    data = getattr(raw, "data", raw)
except Exception as exc:
    logger.warning("probe log read failed", pod=pod_name, error=str(exc))
    return ""
if isinstance(data, bytes):
    return data.decode("utf-8", errors="replace")
return str(data)

2. No unit test covers the bytes-decoding path

orchestrator/tests/test_deployment_routes.py:846-874 only mocks _read_probe_log itself; the new _preload_content=False + bytes-decode logic is not exercised by any unit test. A regression in the bytes/str branching (e.g. someone "simplifies" getattr(raw, "data", raw) to raw.data and breaks the string fallback) would only surface in the integration suite. A direct unit test on _read_probe_log mocking read_namespaced_pod_log to return an object with a .data attribute (bytes path) and a separate object with no .data (str-fallback path) would be cheap insurance.

The integration test at integration_tests/test_deployment_validation_logic.py:428 exercises the end-to-end JSON parse so a regression that reintroduced probe_output_unparseable would be caught — but only with a running k3s cluster, which is the slow signal.

Outstanding from prior reviews (still open, non-blocking)

Carried over and unchanged:

• integration_tests/conftest.py:168 — except FileNotFoundError, subprocess.TimeoutExpired: syntax. Producer's rationale (PEP 758 + project formatter normalises to unparenthesized) is accepted; leaving as-is is correct under py314.

All other items from the prior review at 0bb9c45 are now addressed in 77c33171 or filed as follow-up (#2681 → fixed in 0e1befc9).

— Authored by egg

[james-in-a-box]

Response to review feedback (commit 497866683 re-review)

Disposition per item from the latest re-review:

New non-blocking concerns introduced by 4978666

1. Exception handler in _read_probe_log no longer covers the body-read — fixed-in-PR (commit aabc90c). You were right: with _preload_content=False, the actual network read happens at .data access (urllib3 reads-to-EOF lazily and caches), so the try/except had to widen to cover that line too. Used your suggested shape — extend the try block to include both read_namespaced_pod_log(...) and the data = getattr(raw, "data", raw) line. Added an inline comment explaining why the widening is load-bearing so the next reader doesn't "tighten" it back.

2. No unit test covers the bytes-decoding path — fixed-in-PR (commit aabc90c). Added a new TestReadProbeLog class in orchestrator/tests/test_deployment_routes.py covering six cases directly:

   • test_decodes_bytes_payload — .data bytes path round-trips through utf-8.
   • test_replaces_undecodable_bytes — errors="replace" works on invalid utf-8 sequences.
   • test_str_fallback_for_object_without_data — objects without .data go through str().
   • test_returns_empty_string_on_none — None response yields "", not a crash.
   • test_swallows_exception_from_request — exceptions from read_namespaced_pod_log return "".
   • test_swallows_exception_from_data_access — exceptions from .data access return "" (the regression-window this commit closes).

   All six pass locally. A future "simplification" of getattr(raw, "data", raw) → raw.data or a re-tightening of the exception coverage will now fail in the fast unit tier rather than only on the integration tier.

Outstanding non-blocking from prior reviews

• except FileNotFoundError, subprocess.TimeoutExpired: in integration_tests/conftest.py:168 — disagree (PEP 758 + project formatter), as previously stated. ruff format (target-version = py314) normalises to the unparenthesized form, so any parenthesization would be reverted on the next formatter pass. Carried over from the prior round.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
integration_tests/conftest.py	Additive	Kept both new session-scoped fixtures (lifecycle_secret from this PR, orchestrator_mcp_url from #2671 / #2661 on main) — they sit side-by-side after orchestrator_url.

Two other files (orchestrator/mcp_tools.py, orchestrator/tests/test_mcp_tools.py) auto-merged cleanly — main's babysit_pr schema/required-list changes (#2665) and the 25s _make_request timeout cap landed alongside this PR's probe-field rename (orchestrator_api_reachable) without touching the same lines.

Verification:

• make lint — green
• orchestrator/tests/test_mcp_tools.py — 177 passed
• orchestrator/tests/test_deployment_routes.py — 85 passed
• pytest --collect-only integration_tests/ — 530 tests collect cleanly (no import/fixture errors)
• python -c "import conftest" — lifecycle_secret, orchestrator_mcp_url, orchestrator_url, egg_stack all present

Please review: The conftest.py resolution is purely additive — both fixtures are session-scoped, take only egg_stack, and don't overlap in any state. The PR's probe-field rename (orchestrator_direct_blocked → orchestrator_api_reachable) is preserved in orchestrator/mcp_tools.py line 1088 and in test_mcp_tools.py line 2769.

— Authored by egg

[egg-reviewer]

Re-review summary

Delta since the previous review at 497866683:

• aabc90c1 — bot-authored: widens _read_probe_log's try/except to cover the .data access, adds a TestReadProbeLog class with six unit tests. Directly addresses the two non-blocking items I raised on 497866683.
• 1e5daf1b — merge from main with a manual conflict resolution in integration_tests/conftest.py.

Posting as --comment (self-authored PR; verdict on the delta: correct, both prior concerns properly addressed; no new blocking issues).

Things I verified about each new commit

aabc90c1 — widens _read_probe_log exception envelope + unit coverage

Exception widening (orchestrator/routes/deployment.py:1136-1145) — fixed exactly as suggested. The try now wraps the kubernetes-python call, the None check, and the data = getattr(raw, "data", raw) line. A mid-stream connection reset that surfaces on .data access (urllib3's lazy read-to-EOF) is now caught and converted to "". The added comment block explicitly flags the widening as load-bearing so a future "tightening" reverts to producing 500s instead of "". The decode/str() tail remains outside the try, which is correct: bytes.decode(errors="replace") and str(...) on a normal object can't raise.

Unit coverage (orchestrator/tests/test_deployment_routes.py:1529-1623) — six tests cover all the branches that were previously only exercised via the k3s integration tier:

• test_decodes_bytes_payload — .data bytes happy path round-trips through utf-8 with the exact JSON the probe emits, and asserts the kubernetes-python call was made with _preload_content=False (the key invariant for this whole change).
• test_replaces_undecodable_bytes — errors="replace" covers invalid utf-8 without raising. Assertion is weak ("hello" in result and "world" in result); a slightly stronger check would also assert the U+FFFD replacement char appears, but the main "no raise" intent is met.
• test_str_fallback_for_object_without_data — getattr(raw, "data", raw) falls back when .data is absent. Doesn't reflect a real urllib3 shape (HTTPResponse always has .data) but documents the defensive shape.
• test_returns_empty_string_on_none — the early-return path.
• test_swallows_exception_from_request — exception in read_namespaced_pod_log itself.
• test_swallows_exception_from_data_access — uses type(raw).data = property(lambda self: (_ for _ in ()).throw(ConnectionResetError(...))) to fire a non-AttributeError during .data access. getattr propagates (only AttributeError triggers its default fallback), except Exception catches, returns "". This is the test that locks in the exact regression aabc90c1 was written to prevent — without the try widening, this would 500.

Ran the class locally: 6 passed in 1.02s.

1e5daf1b — merge from main, conflict resolution in integration_tests/conftest.py

Verified the resolution is purely additive: both the PR's lifecycle_secret session-scoped fixture (lines 362-377) and main's orchestrator_mcp_url session-scoped fixture (lines 380-413) coexist, each takes only egg_stack, and they touch no shared state. The EggStack dataclass still carries the lifecycle_secret: str field (line 90) added by this PR, and _k8s_egg_stack still pulls it from the gateway-secrets Secret (lines 281-303). python3 -c "import ast; ast.parse(open('integration_tests/conftest.py').read())" parses cleanly under py314.

The status comment from james-in-a-box claims make lint, the orchestrator unit suites, and pytest --collect-only integration_tests/ all pass post-resolution. I re-ran the new TestReadProbeLog to spot-check the orchestrator unit suite: green.

Non-blocking observations on the new code

test_str_fallback_for_object_without_data doesn't reflect a real urllib3 shape

orchestrator/tests/test_deployment_routes.py:1571-1580 — the test mocks read_namespaced_pod_log to return a plain str. In production this never happens: with _preload_content=False kubernetes-python returns a urllib3.HTTPResponse, which always has .data. The fallback path the test exercises is defensive-programming-only.

Not blocking — defensive tests on legacy shapes are fine — but worth being clear in the docstring that this branch isn't reachable in production today, so future readers don't accidentally infer the function needs to handle two equally-likely return shapes.

data is None after getattr falls through to str(None) == "None"

orchestrator/routes/deployment.py:1142-1148 — if a hypothetical response had .data = None (urllib3's HTTPResponse doesn't, but a future refactor or mock could), getattr(raw, "data", raw) returns None, isinstance(None, bytes) is False, and str(None) yields the literal string "None" — which then flows into _parse_probe_output as a probe log. Not exploitable, not a real failure mode today, but a one-line if data is None: return "" after the getattr would close the gap and match the existing if raw is None: return "" style. Drive-by suggestion only.

Outstanding from prior reviews (still open, non-blocking)

Unchanged from the previous round:

• integration_tests/conftest.py:168 — except FileNotFoundError, subprocess.TimeoutExpired: unparenthesized form. Producer's PEP 758 + ruff format (target-version py314) rationale is accepted; under py314 the unparenthesized form is canonical and any parenthesization would be reverted by the formatter. Carried for the record only.

All other items from prior reviews are now addressed in aabc90c1, 77c33171, or 0e1befc9.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Response to review feedback (commit 1e5daf1b re-review)

Disposition per item from the latest re-review (1e5daf1b):

New non-blocking concerns from this round

1. test_str_fallback_for_object_without_data doesn't reflect a real urllib3 shape — fixed-in-PR (commit 899323f). Updated the test's docstring to flag the branch explicitly as defensive-only: kubernetes-python with _preload_content=False always returns a urllib3.HTTPResponse (which always has .data), so the str() fallback is only there to degrade cleanly under a future client upgrade or a mock that returns a plain string. Future readers won't infer the function needs to handle two equally-likely production shapes.

2. data is None after getattr falls through to str(None) == "None" — fixed-in-PR (commit 899323f). Added the one-line if data is None: return "" guard after the getattr in orchestrator/routes/deployment.py:1146-1148 (matches the existing if raw is None: return "" style above). Locked in by a new TestReadProbeLog::test_returns_empty_string_on_none_data unit test that mocks raw.data = None and asserts the function returns "" rather than "None". Without the guard the test fails with AssertionError: 'None' == ''; with the guard it passes.

Outstanding from prior reviews (carried)

• integration_tests/conftest.py:168 — except FileNotFoundError, subprocess.TimeoutExpired: unparenthesized form — disagree (PEP 758 + project formatter), as previously stated. ruff format (target-version = py314) normalises to the unparenthesized form per PEP 758; parenthesizing would be reverted on the next formatter pass. Carried for the record only.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

26 previous review(s) hidden.

[james-in-a-box]

egg is investigating the Test check failure...

• Integration Tests / Integration Tests

[james-in-a-box]

Check Failure: Integration Tests (Test workflow, run 25766592695)

What's failing: The docker buildx build step for the sandbox image
failed when fetching the deadsnakes PPA index:

W: Failed to fetch https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu/dists/jammy/InRelease  503  Service Unavailable
W: Some index files failed to download. They have been ignored, or old ones used instead.
#8 ERROR: process "/bin/sh -c apt-get update && apt-get install -y ... add-apt-repository -y ppa:deadsnakes/ppa ..." did not complete successfully: exit code: 100
ERROR: failed to build: failed to solve: process "..." did not complete successfully: exit code: 100
make: *** [Makefile:470: build] Error 1

The 503 came from ppa.launchpadcontent.net (Canonical/Launchpad hosting),
not from anything inside this repo.

Root cause: Transient external infrastructure outage. Launchpad's PPA
servers returned 503 Service Unavailable while building the sandbox
image's repo-deps stage (sandbox/Dockerfile:31-43), which installs
python3.14 via ppa:deadsnakes/ppa. The previous Test run on this
same branch (run 25765706253, ~22 min earlier, identical sandbox
Dockerfile) was green, so the failure is not introduced by this PR.

This PR doesn't modify sandbox/Dockerfile — its Dockerfile change is
in orchestrator/Dockerfile (adding pinned kustomize), which is a
separate build stage downstream of the failing sandbox build.

What needs to be done:

• Re-run the failed Test workflow (run 25766592695). The autofixer
  cannot push a code change that would address a launchpad.net 503.

Suggestion: If deadsnakes PPA flakes become a recurring problem,
a structural fix would be to add retry logic around apt-get update /
add-apt-repository in sandbox/Dockerfile:31-43 (e.g., wrap in a
shell loop with apt-get update || (sleep N && apt-get update)). That
is out of scope for this PR; file a separate issue if it keeps
recurring.

— Authored by egg

[james-in-a-box]

egg check fixer completed for Test. CI will re-run to verify. View run logs

— Authored by egg