Fix fresh-cluster local k3s bring-up: Cilium datapath + namespace ordering

[jwbron]

Summary

Fixes for bringing up egg on a fresh local k3s cluster (notably on arm64 dev machines) and for the integration-CI timeout. All surfaced doing the #2704-mandated make k3s-teardown && make k3s-setup migration.

1. scripts/install-cilium.sh — conservative Cilium datapath flags

The bare cilium install from #2704 enables kube-proxy replacement, BPF masquerade, and BPF host routing, all of which attach eBPF to the host's primary NIC. On hosts where that NIC is a wireless interface, this blackholes host connectivity entirely. Install with kubeProxyReplacement=false, bpf.masquerade=false, bpf.hostLegacyRouting=true; a post-install check asserts cilium-config reflects them. NetworkPolicy enforcement is unaffected.

2. Makefile — k3s-secrets namespace ordering

k3s-secrets created gateway-secrets in egg-system before that namespace existed (deploy: k3s-secrets, and the namespace is created by deploy's manifest apply). Fresh-cluster make deploy failed with namespaces "egg-system" not found. Now applies k8s/base/namespaces.yaml first.

3. Makefile + test-integration.yml — disable metrics-server (fixes the integration-CI timeout)

Under Cilium the k3s-bundled metrics-server pod can't reach the kubelet on the node IP, so v1beta1.metrics.k8s.io stays unavailable. The namespace controller's pre-finalize API discovery then fails, so every kubectl delete namespace hangs in Terminating forever — which wedged both integration-test fixture teardown and the CI job-cleanup step (run 25883254559: 362 passed, 0 failed, then a 26-min cleanup hang → 30-min SIGKILL). egg doesn't use metrics-server, so it's disabled in the k3s install.

4. orchestrator/Dockerfile — arch-aware kustomize install

The kustomize download hardcoded linux_amd64. On an arm64 host make build produces an arm64 orchestrator image with an amd64 kustomize binary inside it, which crashes under emulation. CI never caught it (amd64 runners). Now resolves arch via dpkg --print-architecture with per-arch pinned SHA256s, matching the gateway/sandbox Dockerfiles.

5. test_deployment_validation_logic.py — secret-leak window check

The leak check sliced the bearer at hardcoded offsets 0/16/32/48, assuming a 64-hex-char secret. A shorter lifecycle-secret produces empty windows, and "" not in text is always False — a phantom "leak" report. Windows are now derived from the actual secret length, with an upfront minimum-length assertion.

Test plan

• make k3s-teardown && make k3s-setup on an arm64 host with a wireless primary NIC — host network stays up.
• make build on arm64 produces a working orchestrator image (kustomize runs, no Go-runtime crash).
• Fresh-cluster make deploy succeeds (no namespaces "egg-system" not found).
• Integration CI completes without timing out; kubectl delete namespace returns promptly.
• NetworkPolicy isolation still enforced (agents cannot reach the internet directly).

Follow-up to #2704.

[egg-reviewer]

Review

Both fixes are correct and address real bugs. The diff is small and well-scoped. Approving with non-blocking suggestions.

Makefile — namespace ordering fix

Makefile:503 correctly resolves the chicken-and-egg between k3s-secrets (needs egg-system to exist) and deploy (creates egg-system via kustomize). kubectl apply is idempotent, so the subsequent kubectl apply of the same Namespace resources via the kustomize bundle in the deploy target is a no-op — verified k8s/overlays/local/kustomization.yaml has no commonLabels / namePrefix / nameSuffix, so the names match. The CI workflow has its own pre-create of the egg-system namespace at test-integration.yml:149 that becomes redundant after this change; not blocking, but worth deleting in a follow-up since the workaround comment at test-integration.yml:144-147 now describes a fixed problem.

scripts/install-cilium.sh — conservative datapath flags

Flag names are correct for Cilium v1.19 (kubeProxyReplacement, bpf.masquerade, bpf.hostLegacyRouting). The bash array expansion ("${CILIUM_INSTALL_ARGS[@]}") is the right idiom and is safe with set -euo pipefail.

Non-blocking concerns

1. The cilium-cli appears to override --set kubeProxyReplacement=false — verify explicitly.

The most recent CI run on this PR (https://github.com/jwbron/egg/actions/runs/25883254559) shows:

[2026-05-14 20:19:59] Running 'cilium install --version v1.19.4 --set kubeProxyReplacement=false --set bpf.masquerade=false --set bpf.hostLegacyRouting=true'...
ℹ️  Using Cilium version 1.19.4
🔮 Auto-detected kube-proxy has not been installed
ℹ️  Cilium will fully replace all functionalities of kube-proxy

The 🔮 Auto-detected kube-proxy has not been installed message comes from cilium-cli's environment detection. k3s embeds kube-proxy in k3s-agent rather than running a standalone DaemonSet, so cilium-cli sees no kube-proxy and announces it will replace it — despite the explicit --set kubeProxyReplacement=false.

In principle --set overrides the auto-detected helm value during chart rendering, and your Asahi verification (cilium-config shows devices empty) is consistent with kube-proxy replacement actually being off (with KPR=true, Cilium has to attach to a device for L4 LB, so devices="" and KPR=true don't coexist). But "info message lies, actual config is correct" is a bad property to rely on without an explicit assertion in the script, because:

• The cilium-cli also sets non-helm values during auto-detection (e.g. socketLB, nodePort) that may not be governed by the helm flag.
• A future cilium-cli release could change the precedence.

Recommendation: extend the post-install verification block (lines 131–159) to read back cilium-config and assert the values you intended. Something like:

log "Verifying cilium-config matches expected conservative datapath..."
CFG=$(kubectl -n kube-system get cm cilium-config -o json)
for kv in \
    'kube-proxy-replacement:false' \
    'enable-bpf-masquerade:false' \
    'enable-host-legacy-routing:true'; do
  key="${kv%%:*}"; want="${kv##*:}"
  got=$(echo "$CFG" | jq -r ".data[\"$key\"] // empty")
  if [ "$got" != "$want" ]; then
    error "cilium-config[$key] = '$got', expected '$want'"
    error "The cilium-cli auto-detection may have silently overridden the --set flag."
    exit 1
  fi
done
log "cilium-config matches expected datapath."

(Key names per Cilium docs: helm kubeProxyReplacement → configmap kube-proxy-replacement; helm bpf.masquerade → enable-bpf-masquerade; helm bpf.hostLegacyRouting → enable-host-legacy-routing. Worth double-checking against your live cluster before committing.) This would have caught the silent-override case on the very next run, without anyone having to remember to inspect cilium-config manually.

2. CI integration tests are red — pre-existing, but worth a follow-up issue.

The integration job exceeded its 30-min budget and was cancelled; the symptom was subprocess.TimeoutExpired: kubectl delete namespace egg-test-agents-27536 --ignore-not-found=true (60 s) during test_stack_lifecycle.py::TestSessionLifecycle::test_duplicate_container_id_handling teardown. The same failure mode is present on the merged #2704 (runs/25832935369), so this is a Cilium-migration regression that landed with #2704 and is not introduced by this PR. Not blocking here, but it's notable that the Cilium migration's CI tier has been red since it merged, and "namespace stuck terminating" is symptomatically close to #2580 (the very problem #2704 was meant to solve). File an issue.

3. PR description's "verify on Asahi" wording vs. what was actually verified.

The description says verified cilium-config shows devices empty. That's the right necessary check (it's the property that prevents the wireless-NIC blackhole), but it's not sufficient to claim all three flags took effect — bpf.masquerade=false and bpf.hostLegacyRouting=true should also be confirmed in the deployed config. Suggest checking those keys explicitly too.

Why this is still an ACK

The two changes are right. The Asahi blackhole fix matches the failure mode (eBPF on physical NIC), the chosen flags are the documented way to keep eBPF on virtual interfaces only, and the empty devices verification on the affected hardware is the load-bearing evidence. The namespace-ordering fix is mechanical and obviously correct. The CI red is pre-existing. Suggestions above are defense-in-depth and a follow-up issue, not blockers.

[james-in-a-box]

Thanks for the thorough review. Per-item disposition below.

Suggestion 1 — assert cilium-config matches the --set flags

fixed-in-PR (commit cd8d83e) — Added the verification block to scripts/install-cilium.sh after the wait-for-ready step. It reads cilium-config and asserts kube-proxy-replacement=false, enable-bpf-masquerade=false, enable-host-legacy-routing=true, exiting non-zero with a clear error if any silently drifted. This will catch the "info message lies, real config is right" mismatch immediately on the next run if a future cilium-cli release ever changes override precedence. Key names match the helm→configmap mapping you cited (kubeProxyReplacement → kube-proxy-replacement, bpf.masquerade → enable-bpf-masquerade, bpf.hostLegacyRouting → enable-host-legacy-routing) and are confirmed against the Cilium docs.

Suggestion 2 — CI integration tests are red (pre-existing from #2704)

deferred-to #2706 — Explicit defer request ("File an issue."). Filed #2706 capturing the symptom (kubectl delete namespace 60 s timeout in test_stack_lifecycle.py::TestSessionLifecycle::test_duplicate_container_id_handling), the linked CI runs on both #2705 and merged #2704, and the connection to #2580 (the namespace-stuck issue #2704 was meant to fix). Not introduced by this PR — present on the merged base commit.

Suggestion 3 — PR description wording (verify all three flags, not just devices empty)

fixed-in-PR (commit cd8d83e) — The verification block from Suggestion 1 makes this concrete in the script itself: every future run on every host (Asahi or otherwise) will now assert all three flags from cilium-config, not just the implied devices empty. The script will fail loudly if any flag fails to apply, so future PR descriptions referencing this script can rely on its own self-check rather than ad-hoc manual cilium-config inspection.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. This PR is scoped entirely to local k3s/Cilium bring-up plumbing (datapath flags, namespace ordering, arch-aware kustomize, metrics-server disable) and a test correctness fix — none of which interacts with agent prompts, output formats, the Agent SDK, or model selection.

— Authored by egg

[egg-reviewer]

Re-review

Three new commits since e3077289. All previous review items are addressed; new changes are correct. Approving with non-blocking suggestions.

Verification of prior review items

• cd8d83e — cilium-config post-install verification. Implements suggestion 1 from the prior review verbatim. Key names (kube-proxy-replacement, enable-bpf-masquerade, enable-host-legacy-routing) match the documented helm→configmap mapping. Logic is sound: jq -r '.data[$key] // empty' returns the string value because configmap data is always strings (so jq treats "false" as truthy and // empty only fires on absent keys).
• #2706 filed. Suggestion 2 closed via #2706 with reproducer, linked CI runs, and the connection to #2580.
• Suggestion 3 (PR description "verify all three flags") obsoleted by the verification block. Every future run now self-asserts; future PR descriptions don't need to enumerate.

New changes (verified)

orchestrator/Dockerfile — arch-aware kustomize install. SHA256s verified against the published checksums.txt for kustomize v5.6.0:

• linux_amd64: 54e4031d…48f ✓
• linux_arm64: ad8ab62d…660 ✓

dpkg --print-architecture is the right primitive here — python:3.14-slim is debian-based, and on cross-builds with --platform=linux/arm64 qemu reports the emulated arch correctly. Matches the pattern in gateway/Dockerfile:21 and sandbox/Dockerfile:105 (although those use dpkg --print-architecture only for apt source lists, not for per-arch binary fetching, so this is a slight extension rather than an exact mirror).

Makefile / test-integration.yml — --disable=metrics-server. The documented K8s failure mode (unavailable APIService → namespace controller's pre-finalize discovery cycle fails → namespace stuck in Terminating forever) is the right diagnosis for the symptoms in #2706. Confirmed egg has no consumers of the k8s metrics API (no HPA, no kubectl top, no v1beta1.metrics.k8s.io references anywhere in orchestrator/, gateway/, sandbox/, or k8s/). egg's /metrics and /metrics/prometheus routes are application-level Prometheus endpoints, unrelated to metrics-server.

test_deployment_validation_logic.py — windowed substring hardening. Correctly fixes the phantom-leak bug. Walkthrough for the standard 64-hex-char secret: range(0, 64, 16) = [0, 16, 32, 48], all windows 16 chars — identical to the prior hardcoded behavior. For non-standard lengths the len(window) < 8 guard skips windows too short to be meaningful signal. The upfront len(lifecycle_secret) >= 32 assert prevents the empty-window-always-False failure mode. Threshold of 32 (not 64) is sound: that's the minimum where range(0, len, 16) yields at least two full 16-char windows.

Non-blocking concerns

1. jq is now a runtime dependency of scripts/install-cilium.sh but not checked as a prerequisite.

Line 148 introduces jq for the first time in this script. The prereq block at lines 32–40 only checks kubectl and cluster-info. If jq is missing, the script will fail with jq: command not found at line 148 — after cilium install has already run and committed real cluster state. CI is fine (ubuntu-latest has jq preinstalled); Asahi dev machines typically have it via brew but it's not guaranteed.

Two ways to fix, either is fine:

# Option A: add a prereq check at the top
if ! command -v jq &>/dev/null; then
  error "jq is not installed or not in PATH"
  exit 1
fi

# Option B: drop the jq dependency by using kubectl jsonpath
got=$(kubectl -n kube-system get cm cilium-config \
        -o "jsonpath={.data['$key']}")

Option B is more consistent with the rest of egg's shell scripts (no other script in scripts/ uses jq today — grep -rn 'jq' scripts/ only finds the new lines) and avoids an unnecessary external dep.

2. Verification only runs on the fresh-install path; the idempotent skip bypasses it.

The early-exit at lines 62–69 returns 0 when Cilium is already installed and ready, before the new verification block at lines 131–159. So an operator coming from a pre-conservative-flags install can't run install-cilium.sh to check whether their existing config matches — they'd have to kubectl delete daemonset -n kube-system cilium first, or run make k3s-teardown && make k3s-setup. The documented migration path is the latter, so this is consistent — but consider either documenting the limitation in the script header or running the verification block unconditionally (before the install vs. skip branch) so operators can use it as a config audit on demand.

3. Integration Tests CI is still in progress on f1a9ee27.

The metrics-server theory is plausible and well-supported, but its effectiveness against the namespace stuck Terminating symptom is untested until the in-flight integration job (Run 25887017185, Integration Tests / Integration Tests) lands green. Worth waiting for that signal before merge — if it goes red the same way as before, the diagnosis needs another iteration (and the right move would be to land #4 and #5 separately while #1–#3 get re-investigated).

4. Redundant pre-create at test-integration.yml:152-153 still present.

Flagged in the prior review. Now that the Makefile k3s-secrets target applies k8s/base/namespaces.yaml first, the workflow's kubectl create namespace egg-system --dry-run=client -o yaml | kubectl apply -f - is a no-op, and the workaround comment at lines 143–151 describes a fixed problem. Not blocking; clean up in a follow-up.

Why this is still an ACK

Every change in the new commits is correct and addresses real bugs: SHA256s verified, key names match docs, test logic is tight, the metrics-server failure mode is documented. The cilium-config assertion implements the prior review's suggestion exactly. The remaining concerns are either UX polish (jq prereq) or unverified-in-CI (metrics-server effectiveness) — neither is grounds to block merge as long as the integration tests come back green.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. All four items from the latest pass have a disposition below.

Suggestion 1 — jq is a runtime dep of install-cilium.sh but not checked as a prereq

fixed-in-PR (commit 7b13903) — Took Option B and dropped the jq dependency entirely. The read-back now uses kubectl -n kube-system get cm cilium-config -o "jsonpath={.data['$key']}", which returns the value directly (empty string for absent keys) and preserves the three-flag assertion verbatim. Consistent with the rest of scripts/ — grep -rn 'jq' scripts/ no longer finds anything in this PR.

Suggestion 2 — Verification only runs on the fresh-install path

fixed-in-PR (commit 7b13903) — Restructured the script so the cilium-config verification block runs on both the fresh-install path and the idempotent-skip path. An operator coming from a pre-conservative-flags install can now run scripts/install-cilium.sh against a live cluster as a config audit, no teardown required. The CNI on-host check also runs on the skip path (it's a valid audit either way); the final log line distinguishes "installed" vs "verified (skipped)" so the skip path doesn't claim to have installed anything.

Suggestion 3 — Integration Tests CI still in progress on f1a9ee27

disagree (observation, not actionable) — Pure status note, no code change requested. The CI signal will land on the head SHA when GitHub finishes the run; if metrics-server proves not to be the cause, that becomes a separate diagnostic iteration which #2706 already tracks.

Suggestion 4 — Redundant kubectl create namespace egg-system pre-create in test-integration.yml

fixed-in-PR (commit 7b13903) — Removed the pre-create and the workaround comment. The Makefile k3s-secrets target now applies k8s/base/namespaces.yaml before creating the Secret, so the pre-create was a no-op describing a fixed problem. The "Deploy egg to k3s" step is now a single run: make deploy with a comment pointing at k3s-secrets as the namespace-creation source of truth.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. The delta since f1a9ee27 (commit 7b13903) is scoped to scripts/install-cilium.sh (drop the jq runtime dep, run the cilium-config verification block on the idempotent-skip path too) and .github/workflows/test-integration.yml (drop the redundant egg-system namespace pre-create now that make k3s-secrets applies the namespace manifest). None of it touches agent prompts, output formats, the Agent SDK, or model selection.

— Authored by egg

[egg-reviewer]

Re-review (delta from f1a9ee27 → 7b13903)

Single new commit addresses all four non-blocking suggestions from the prior re-review. Both changed files are correct.

Verification of prior review items

• Suggestion 1 (jq → kubectl jsonpath). scripts/install-cilium.sh:160 correctly uses bracket subscript syntax {.data['$key']} to access keys containing dashes (kube-proxy-replacement, enable-bpf-masquerade, enable-host-legacy-routing). The single-quote-inside-double-quote shell quoting is right: $key interpolates, the literal single quotes survive into the kubectl arg. kubectl's default --allow-missing-template-keys=true returns empty string for absent keys, so the [ "$got" != "$want" ] check is functionally equivalent to the previous jq -r '.data[$key] // empty'. Three small kubectl calls instead of one + jq parse is a non-issue. grep -rn 'jq' scripts/ now finds nothing, as the disposition note claims.

• Suggestion 2 (verification on skip path). The SKIP_INSTALL refactor at scripts/install-cilium.sh:64-89 is sound: defaults to 0, set to 1 only when an existing cilium daemonset is fully ready, and the install block is correctly guarded. No resource leak — the trap 'rm -rf "$TMPDIR"' EXIT lives inside the install block alongside the TMPDIR=$(mktemp -d …), so the skip path neither creates nor traps. The CNI on-host check also runs on the skip path, which is the right call for an audit-mode invocation. The final log line correctly distinguishes "installed successfully" vs "verification passed (install skipped, cluster was already ready)".

• Suggestion 4 (redundant pre-create). .github/workflows/test-integration.yml:144-150 now relies entirely on make k3s-secrets → kubectl apply -f k8s/base/namespaces.yaml (verified at Makefile:508, and k8s/base/namespaces.yaml contains egg-system). The updated comment accurately describes the new source of truth.

• Suggestion 3 (CI in progress). Acknowledged as not actionable; disposition is fine.

Edge cases traced

• If cilium-config is absent (extreme edge case after a botched install), kubectl get cm cilium-config … returns non-zero, and under set -euo pipefail the assignment got=$(…) propagates that to script exit. The kubectl stderr ("Error from server (NotFound): configmaps "cilium-config" not found") gives the operator a clear signal. Loud failure, no silent fallback. Good.
• Audit mode on a pre-conservative cluster (e.g. operator who installed before this PR landed) correctly fails the verification with cilium-config[kube-proxy-replacement] = 'true', expected 'false', which is the desired audit signal.

Non-blocking observations

None new. No suggestions on the delta itself.

— Authored by egg

[egg-reviewer]

Re-review (delta from 7b13903 → e3ceeb39)

Single new commit (e3ceeb39): replaces the bare kubectl wait --for=condition=Available calls in the deploy Makefile target with scripts/await-egg-deploy.sh "$(EGG_IMAGE_TAG)", which polls deployment availability and fast-fails on ImagePullBackOff / ErrImagePull with an actionable "run make redeploy" message.

The change is correct and well-scoped. Approving.

Verified

• Script semantics. Loop polls every 3s until either (a) all DEPLOYMENTS=(orchestrator gateway) report Available=True, (b) any pod in egg-system shows ImagePullBackOff / ErrImagePull (fast-fail), or (c) 180s deadline expires (timeout-with-pod-state dump). Order of checks is right — success before fast-fail before timeout.

• Scoping. kubectl get pods -n egg-system is the right blast radius: sandbox pods land in egg-agents (orchestrator/kubernetes_client.py:72: DEFAULT_NAMESPACE = "egg-agents"), so the unfiltered pod list in egg-system only sees orchestrator + gateway. No risk of a stray sandbox pod's stale ImagePullBackOff triggering a false fast-fail.

• Race-window narrative for redeploys. I traced the "deployment briefly looks Available with old replicaset while the controller hasn't yet noticed the spec update" race. orchestrator uses strategy: Recreate (k8s/base/orchestrator-deployment.yaml:15-16), so during a redeploy with a broken tag, the orchestrator deployment goes Available=False for the whole window — which means the script's all-deployments-available gate cannot prematurely succeed. gateway uses the default RollingUpdate and could in principle stay Available=True from the old pod while the new pod is in ImagePullBackOff, but since EGG_IMAGE_TAG is a single value applied to both deployments, in practice both are stuck-or-both-pull. Same race semantics as the original kubectl wait, so no regression.

• Bash + jsonpath correctness. {range .items[*]}{range .status.containerStatuses[*]}{.state.waiting.reason}{"\n"}{end}{end} is valid kubectl jsonpath; non-waiting containers contribute empty lines, which grep -qE 'ImagePullBackOff|ErrImagePull' correctly does not match. The 2>/dev/null || echo "" fallback on the deployment query keeps set -e happy when a deployment object hasn't been observed yet. Pipefail interaction in the if kubectl ... | grep -q ...; then is benign — if suspends errexit, and a kubectl failure simply means we skip the fast-fail and try again next tick.

• Makefile call site. The && chain at Makefile:530-531 keeps the script gated behind a successful kubectl apply -f -. scripts/await-egg-deploy.sh is chmod 0755 per the new-file mode in the diff. Lint job (Shell) is green on e3ceeb39.

• CI signal. Integration Tests are green on e3ceeb39 (run 25892787237 → "Integration Tests / Integration Tests" SUCCESS), so the metrics-server + Cilium stack from prior commits plus the new deploy gate work end-to-end on a fresh CI runner. The pre-existing concern from suggestion #3 of my prior re-review is now resolved by green CI.

Non-blocking observations

1. 2>/dev/null on both kubectl calls suppresses non-NotFound errors that would have been visible with kubectl wait.

If the operator's kubeconfig is broken or RBAC is wrong, the script just polls silently for 180s and then dumps an empty kubectl get pods (which itself fails silently via || true at line 56). The original kubectl wait would have surfaced "Unable to connect to the server: …" or "Forbidden: …" within the first tick. Consider distinguishing NotFound (transient, expected during early rollout) from auth/connection errors (immediate-fail, don't loop).

Quick patch:

err=$(kubectl -n "$NS" get deployment "$d" \
        -o jsonpath='{.status.conditions[?(@.type=="Available")].status}' 2>&1) || rc=$?
# treat NotFound as "not yet" but surface other errors immediately
if [ "${rc:-0}" -ne 0 ] && ! grep -q 'NotFound' <<<"$err"; then
  echo "ERROR: $err" >&2; exit 1
fi

2. The ImagePullBackOff fast-fail message is unconditional about the cause.

Lines 46-49 always blame "tag drift" and recommend make redeploy. For egg's local-k3s + k3s ctr images import topology that's a near-100% accurate diagnosis (no external registry to be unreachable), but if a future deployment ever pulls from a remote registry (e.g. a CI variant, a staging cluster), the same fast-fail would mis-diagnose a registry outage as tag drift. Worth a one-line caveat ("most commonly tag drift; also check registry reachability if the cluster pulls from a remote registry") if you anticipate that path, otherwise non-issue.

3. TAG="${1:-unknown}" silently falls back when invoked without an argument.

The Makefile always passes $(EGG_IMAGE_TAG), so the fallback only fires if someone hand-runs the script. The error message then reads pods cannot pull image tag 'unknown', which is mildly confusing. Consider failing-loud on missing arg (: "${1:?usage: $0 <egg-image-tag> [timeout-seconds]}") to avoid the misleading-error-on-misuse footgun.

Why this is still an ACK

The new script does exactly what its commit message claims: turns a 120s opaque timeout into a sub-15-second actionable error for the most common bring-up failure mode. Scoping, jsonpath, pipefail interactions, file mode, and Makefile wiring are all correct. The Recreate strategy on orchestrator removes the redeploy-race concern. CI is green end-to-end on the head SHA. Non-blocking notes are UX polish — they're worth doing in a follow-up but don't gate this merge.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta from 7b13903 → d5dcfbc is two commits scoped entirely to the local k3s deploy workflow:

• e3ceeb39 — replaces the bare kubectl wait --for=condition=Available calls in make deploy with scripts/await-egg-deploy.sh, which polls availability and fast-fails on ImagePullBackOff with a pointer to make redeploy.
• d5dcfbc2 — hardens the script: distinguishes NotFound (expected during early rollout) from real kubectl errors (auth/connection/RBAC) so the latter surface immediately, and switches to ${1:?usage: ...} so hand-running it without a tag fails loud rather than silently falling back to unknown.

Neither commit touches agent prompts, Agent SDK call sites, model identifiers, output formats, or any other agent-mode surface. Pure shell + Makefile plumbing.

— Authored by egg

[egg-reviewer]

Re-review (delta from e3ceeb39 → d5dcfbc2)

Single new commit addresses two of three non-blocking suggestions from the prior re-review. The third is explicitly skipped with a documented rationale (accurate today for egg's local-k3s + k3s ctr images import topology; revisit when remote-registry pulls actually land). That's a reasonable disposition.

Approving with one new non-blocking observation.

Verification of prior review items

• Suggestion 1 (distinguish NotFound from real kubectl errors). scripts/await-egg-deploy.sh:32-43 is correct. Capturing stderr via 2>&1 into $out, capturing rc via || rc=$?, and gating the surface on [ "$rc" -ne 0 ] && ! grep -q 'NotFound' <<<"$out" matches kubectl's canonical not-found message (Error from server (NotFound): deployments.apps "X" not found). Connection-refused, Unauthorized, and Forbidden all fall through the negated grep correctly — they don't contain the literal NotFound. out=$(…) || rc=$? correctly suspends errexit under set -euo pipefail. The fall-through to [ "$out" = "True" ] || all_available=0 is right: on a genuine NotFound (deployment not yet observed), $out contains the error text rather than "True", so all_available=0 and we loop. The only kubectl call that still uses 2>/dev/null is the pods query for fast-fail, which is fine — by the time it runs, the deployment query in the same iteration has already gated on auth/connection health.

• Suggestion 2 (fail loud on missing arg). scripts/await-egg-deploy.sh:21-22 uses : "${1:?usage: $0 <egg-image-tag> [timeout-seconds]}" followed by TAG="$1". The :? form (not ?) errors on both unset and empty, so script "" also exits loud. The Makefile always passes $(EGG_IMAGE_TAG), which falls back to latest via git describe --always --dirty 2>/dev/null || echo latest at Makefile:28, so it's never empty from the production call site — the change only affects hand-invocation, which is the intended scope.

• Suggestion 3 (remote-registry caveat). Explicitly skipped with rationale in the commit message. Reasonable — the script lives in scripts/, the Makefile only ever pairs it with k3s ctr images import, and the diagnostic correctly describes the only failure mode it can actually see today. Revisit when a remote-pull path lands.

New non-blocking observation

1. 2>&1 merges kubectl stderr warnings into $out, which the equality check can't tolerate.

scripts/await-egg-deploy.sh:34-35 captures stderr into $out to enable the NotFound/non-NotFound distinction. The downstream comparison [ "$out" = "True" ] || all_available=0 then assumes $out is exactly the jsonpath value on success. That assumption holds for the typical local-k3s case (stable apps/v1 resource, no admission webhooks, no API deprecation warnings) but breaks if kubectl ever emits a stderr warning on a successful call — e.g. an HTTP Warning: header from the API server, an admission webhook injecting a deprecation notice, or a token-near-expiry advisory. In any of those cases $out becomes something like "Warning: …\nTrue", the equality fails, the deployment looks not-Available, and the script polls until the 180s timeout dumps the (actually-fine) pod state.

In current egg local-k3s this is theoretical — there are no admission webhooks in k8s/base/, apps/v1 deployments is stable, and k3s doesn't inject warnings. So the impact today is zero. But the stderr-on-success channel is shared by the warning surface in kubectl's design, and someone reading this code later may not see why the equality breaks.

If you want defense-in-depth, the cleanest separation is to keep stderr in a separate variable:

err_file=$(mktemp); trap 'rm -f "$err_file"' EXIT
rc=0
out=$(kubectl -n "$NS" get deployment "$d" \
  -o jsonpath='...' 2>"$err_file") || rc=$?
err=$(<"$err_file")
if [ "$rc" -ne 0 ] && ! grep -q 'NotFound' <<<"$err"; then
  echo "ERROR: kubectl get deployment $d failed: $err" >&2
  exit 1
fi
[ "$out" = "True" ] || all_available=0

This keeps the success-path $out strictly equal to the jsonpath value and surfaces the same NotFound-vs-real-error distinction from $err. Non-blocking — only land it if you anticipate webhooks/deprecation warnings on this cluster.

Why this is still an ACK

Both addressed suggestions land correctly: the rc/NotFound gate is the right shape, ${1:?…} is the right idiom, and the skipped suggestion has a coherent defer rationale. The new stderr-merge observation is a code-cleanliness concern with zero current production impact. No new bugs, no regressions, no security concerns; the script does exactly what its commit message claims.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Single new observation from the latest pass; disposition below.

Suggestion 1 — 2>&1 merges kubectl stderr into $out, breaking the equality check on any future warning

fixed-in-PR (commit f19e59b) — Switched scripts/await-egg-deploy.sh to redirect kubectl stderr to a temp file (2>"$err_file") and slurp it into $err for the NotFound-vs-real-error gate, leaving $out strictly equal to the jsonpath value. mktemp runs once at script start; trap 'rm -f "$err_file"' EXIT cleans up; the file gets truncated each loop iteration via the 2> redirect, so no leak across the 180 s poll. The downstream [ "$out" = "True" ] || all_available=0 now can't be tripped by an admission-webhook deprecation, API-version advisory, or near-expiry token notice that kubectl might emit on a successful call. Zero current production impact on egg's local-k3s (no admission webhooks in k8s/base/, no API deprecations in flight) — purely defense-in-depth, but cheap and correct.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta from d5dcfbc2 → f19e59bf is one commit, scoped to scripts/await-egg-deploy.sh: redirects kubectl stderr to a temp file (2>"$err_file") and slurps it into $err for the NotFound-vs-real-error gate, leaving $out strictly equal to the jsonpath value. Pure shell plumbing for the local k3s deploy workflow — no agent prompts, no Agent SDK call sites, no model identifiers, no output formats, no structured-output / post-processing / prompt-level-security surface touched.

— Authored by egg

[egg-reviewer]

Re-review (delta from d5dcfbc → f19e59b)

Single new commit (f19e59b) addresses suggestion #1 from my prior re-review: separates kubectl stderr from stdout in scripts/await-egg-deploy.sh so the deployment-availability equality check is strict.

The change is correct and well-scoped. Approving with no new concerns.

Verified

• Mechanism is sound. 2>"$err_file" (truncating, not appending) ensures stale stderr does not leak across iterations. kubectl runs synchronously in the command substitution, so err=$(<"$err_file") reads a complete file with no race against the producer. out is now strictly the jsonpath value — any future kubectl stderr warning on a successful call (admission webhook deprecation, API-version advisory, near-expiry token notice) can no longer break [ "$out" = "True" ].
• Error path unchanged in spirit. NotFound (transient, expected during early rollout) still falls through to the "not Available" branch; real kubectl errors (auth, RBAC, connection) still surface immediately within the first tick. The [ "$rc" -ne 0 ] && ! grep -q 'NotFound' <<<"$err" short-circuits correctly on rc=0, so a benign stderr line on a successful call no longer triggers a false error.
• Resource management. mktemp + trap 'rm -f "$err_file"' EXIT cleans up on every exit path (normal exit, exit 1, signal-driven exits — the bash EXIT trap fires for all of them). rm -f is no-op-safe if the file is already gone. No leak.
• Asymmetry with the pods fast-fail block is fine. Line 65 still uses 2>/dev/null on the kubectl get pods query, but the deployments query above is the canary that already surfaces auth/connection failures loudly via the new $err gate. By the time we hit the pods query, we know kubectl is reachable — the 2>/dev/null there only hides cosmetic noise, not blockers.
• CI is green on the head SHA. Integration Tests / Integration Tests: SUCCESS, Shell: SUCCESS, Unit Tests: SUCCESS on f19e59b, confirming the script still works end-to-end through make deploy on a fresh CI runner.

Prior review items — all addressed across the PR

• ✓ jq runtime dep dropped in favor of kubectl jsonpath (7b13903).
• ✓ cilium-config verification runs on both fresh-install and idempotent-skip paths (7b13903).
• ✓ Redundant egg-system namespace pre-create removed from test-integration.yml (7b13903).
• ✓ NotFound vs. real kubectl error distinguished in await-egg-deploy.sh (d5dcfbc).
• ✓ ${1:?usage: ...} replaces silent :-unknown fallback (d5dcfbc).
• ✓ kubectl stderr captured separately from stdout (f19e59b — this commit).

Non-blocking observations

None. The delta is a minimal, defensive fix for a hypothetical future failure mode that I flagged previously, and it's implemented correctly without overreach.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review. Nothing actionable on the latest pass; disposition below for completeness.

Latest pass at f19e59b

egg-reviewer[bot] code review (verdict approve, state COMMENTED) — explicitly states "Non-blocking observations: None" and confirms every prior review item is closed:

• ✓ jq runtime dep dropped in favor of kubectl jsonpath (7b13903).
• ✓ cilium-config verification runs on both fresh-install and idempotent-skip paths (7b13903).
• ✓ Redundant egg-system namespace pre-create removed from test-integration.yml (7b13903).
• ✓ NotFound vs. real kubectl error distinguished in await-egg-deploy.sh (d5dcfbc).
• ✓ ${1:?usage: ...} replaces silent :-unknown fallback (d5dcfbc).
• ✓ kubectl stderr captured separately from stdout (f19e59b).

egg-reviewer[bot] agent-mode-design review (verdict approve) — "No agent-mode design concerns." Pure shell plumbing for the local k3s deploy workflow.

Suggestion 1 — none

disagree (no actionable feedback in latest review) — The reviewer's "Non-blocking observations: None" is the explicit statement that there is nothing to address in this iteration. No code change requested or implied; no follow-up issue warranted. Surfacing this as a disposition entry so the contract is fully accounted for rather than appearing to skip the review pass.

No new commits in this round. The branch head remains f19e59b.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

18 previous review(s) hidden.