helm: verify the chart actually deploys, in CI and on a real cluster

[allamiro]

Scope changed after #282 merged. That PR — titled as a Dockerfile fix — carried the entire chart to main, so everything except the deployment testing is already there. See the comment below; the mistake was mine and worth reading before the diff.

What is left here is the half the chart never had: proof it deploys.

The gap

The helm CI job lints, renders, checks every manifest has kind/apiVersion, asserts four identity refusals, and checks name limits. All of it static. None of it can tell whether a cluster comes up.

That is not a hypothetical gap. The image shipped without vtop-node entirely (#281), so every pod would have CrashLoopBackOff'd — and every static check stayed green.

scripts/k8s-smoke.sh

Installs the chart against a live cluster and asserts what rendering cannot:

• all three pods reach Ready
• the metadata Raft group bootstraps and elects a leader
• records stream in, and the committed offset equals what was produced
• a pod nobody produced to is still empty — which turns the chart's "each pod is an independent standalone range" claim into a checked fact rather than a sentence in a README
• every record survives a force-deleted pod with no grace period — the SIGKILL path every stop takes today (node: vtop-node ignores SIGTERM, so every orderly stop is a crash stop #280), so this is ordinary shutdown behaviour, not an extreme

helm-deploy CI job

Stands up kind, builds the image, and runs the smoke test.

The binaries come out of the image, not from a second cargo build. The image already contains both — that is exactly what #281 was about — so extracting them means the test drives the artifact that would actually ship. It also skips a second full Rust build in a job that has already paid for one.

Gated on helm or rust, because the chart's behaviour depends on the image as much as on the templates.

Verified by running it

Three consecutive passes against Docker Desktop's Kubernetes, plus an earlier manual run that streamed 600 records and recovered all of them from a kill:

[k8s-smoke] all pods Ready
[k8s-smoke] Raft group bootstrapped with a leader
[k8s-smoke] streamed 300 records
[k8s-smoke] committed offset is 300, as produced
[k8s-smoke] pod 1 is empty, confirming per-pod independent ranges
[k8s-smoke] all 300 records survived a hard kill
[k8s-smoke] PASS

The helm-deploy job also already earned its place: on the pre-rebase branch it failed with Could not find /usr/local/bin/vtop-node, which is precisely the bug it was written to catch.

Two things the run taught the script

A fresh producer epoch per round — sequence state is keyed on (producer_id, producer_epoch), so replaying epoch 1 is correctly deduplicated, the offset stops moving, and that reads as a stall when it is idempotency working.

Status output is captured, never piped into grep -q — the short-circuit closes the pipe and kills vtopctl on SIGPIPE, reported as a panic that hides the real status.

README

Now states that a cold install typically restarts once or twice: nodes resolve peer DNS at startup and exit if it is not ready yet, so RESTARTS 2 on a fresh deploy is startup ordering settling — not a broken deployment, and not a reason to loosen the probes.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[cubic-dev-ai]

1 issue found across 16 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="helm/vtop/templates/statefulset.yaml">

<violation number="1" location="helm/vtop/templates/statefulset.yaml:44">
P2: The 60-second termination grace period does not provide a clean vtop-node shutdown because the binary has no SIGTERM handling, so StatefulSet updates and pod deletions take the crash-recovery path immediately; signal-aware shutdown or an equivalent pre-stop mechanism would make this grace period effective.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The 60-second termination grace period does not provide a clean vtop-node shutdown because the binary has no SIGTERM handling, so StatefulSet updates and pod deletions take the crash-recovery path immediately; signal-aware shutdown or an equivalent pre-stop mechanism would make this grace period effective.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helm/vtop/templates/statefulset.yaml, line 44:

<comment>The 60-second termination grace period does not provide a clean vtop-node shutdown because the binary has no SIGTERM handling, so StatefulSet updates and pod deletions take the crash-recovery path immediately; signal-aware shutdown or an equivalent pre-stop mechanism would make this grace period effective.</comment>

<file context>
@@ -0,0 +1,220 @@
+      # vtop-node never calls the Kubernetes API; a mounted token would be
+      # pure attack surface on a pod that also serves unauthenticated HTTP.
+      automountServiceAccountToken: false
+      terminationGracePeriodSeconds: {{ int .Values.terminationGracePeriodSeconds }}
+      {{- with .Values.imagePullSecrets }}
+      imagePullSecrets:
</file context>

[allamiro]

All seven fixed across d7aeb42 and 4283590. Two of them made the chart unusable in shapes it claimed to support, so taking them in order of how much they mattered.

P1 — the admin Service could not bootstrap the cluster

You are right, and this was the one that would have wasted somebody's afternoon. A load-balanced endpoint and per-pod certificate identity are simply incompatible: every pod presents a certificate whose SAN is that pod's own headless FQDN, so a client routed to an arbitrary pod has no name it can verify. server_name also defaults to localhost, which matches nothing. TLS fails before vtopctl meta init sends a byte.

I did not add a shared SAN contract, because the chart cannot inspect the certificates an operator minted and must not assume what is in them. Instead:

• The admin port is off the client Service by default, behind service.exposeMetaAdmin, documented with the exact condition that makes it safe (certificates carrying the Service DNS name as a shared SAN).
• NOTES.txt now forwards a specific pod and prints both lines the config needs:

endpoint: localhost:9200
server_name: <release>-0.<release>-headless.<ns>.svc.cluster.local

P1 — names collided at length

fullname truncated to 63, then the headless Service appended -headless and truncated to 63 again — which for a long name yields the same string as the client Service, so the chart renders two Services with one name. A second truncation cannot recover characters the first one dropped.

fullname now truncates to 54, reserving the 9 characters -headless needs. That also keeps StatefulSet pod names and the statefulset.kubernetes.io/pod-name label inside the DNS limit.

Verified rather than reasoned about — an 80-character fullnameOverride:

	before	after
fullname	63 chars	54 chars
headless	63 chars, identical string	63 chars, distinct

P2 — DNS egress was unrestricted

Correct. No to selector means port 53 to every destination, which is a standing hole in a default-deny policy on the classic exfiltration channel. Scoped to kube-dns in kube-system, overridable via networkPolicy.dnsTo.

P2 — vtopctl node status was blocked by the policy documenting it

Also correct. That command goes replica-to-replica over the replication listener, not the admin endpoint, and the client rule admitted only admin and native. Replica port added, with a comment saying why it is there so it does not get tidied away.

P2 — podLabels / podAnnotations could hijack chart-managed keys

Both taken as suggested. A user label redefining a selector label leaves the StatefulSet unable to match its own pods; a user annotation replacing checksum/config silently disables config-triggered rollouts, so pods keep running against a ConfigMap that no longer exists. Both omit-guarded, and CI asserts a hijack attempt is ignored.

P2 — the termination grace period

You are right that it provides nothing, and the chart was worse than silent about it: the comment claimed it was "long enough for a clean shutdown fsync". There is no clean shutdown to be long enough for — vtop-node installs no signal handler at all.

I did not add a preStop hook, because there is nothing to call. What I did:

• The chart now says so, in both values.yaml and the template. Stops take the crash-recovery path, which is safe by design — acknowledgement follows fsync, recovery truncates a torn tail, and live-chaos scenario 04 SIGKILLs the leader mid-produce and verifies every acknowledged record survives byte-exact. It is safe, it is just not graceful, and the interval buys nothing.
• Filed node: vtop-node ignores SIGTERM, so every orderly stop is a crash stop #280 for signal-aware shutdown. The part that actually matters there is not faster startup: a departing leader could release its range lease instead of making failover wait out a timer.

The chart had no CI at all

Worth flagging separately, because it is why two of these reached review unchallenged. Every check on this PR skipped — helm lint never ran, nothing rendered the chart.

A branch existed adding a helm path filter and a comment referring to "the helm job below". There is no such job; the filter output was wired to nothing, which is the shape where CI looks configured and gates nothing.

4283590 adds the job, checking the two contracts the chart makes:

• Rendering — lint, render, and count the manifests, because a chart that renders nothing also exits zero. Every manifest must carry a kind and an apiVersion.
• Refusing — three refusals asserted (no identities, no TLS secret names, a nodeUuids list that does not match replicaCount). This is the security property: security: lab compose binds to all interfaces with default credentials #81 exists because a lab compose shipped default credentials, and a refusal that quietly stopped refusing would look exactly like success.
• Names — rendered at helm's maximum release-name length, asserting Service names are distinct and every name fits in 63 including the ordinal. That is the check that would have caught the collision above.

Stdlib only, no YAML library: an install step that quietly fails leaves a check that no longer checks while still reporting green.

helm (lint + render + refusals) is now passing on this PR — the first automated signal this chart has ever had.

[cubic-dev-ai]

All reported issues were addressed across 18 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[allamiro]

All four fixed in 116c0d1. Taking the fourth first, because it is the one I should be embarrassed about.

The CI step I wrote did not test what it claimed

You are exactly right, and the reasoning is precise: refuses "TLS secret names" set only cluster.id, so rendering aborted at the nodeUuids length check inside the ConfigMap — which renders before the StatefulSet whose volumes reference the Secrets. Helm exits non-zero either way, so the step passed while never once reaching the TLS refusal it names.

I added that check one commit earlier, in a message arguing that a refusal which quietly stopped refusing "would look exactly like success". Then I wrote one that looks exactly like success. A regression removing the TLS requirement would have sailed straight through the check written to catch it.

Fixed as suggested — every other identity supplied so rendering reaches the TLS path — and verified by confirming the refusal message now actually cites TLS rather than trusting the exit code:

TLS is required and never defaulted: set tls.metaSecretName to an existing Secret hol…

I also added a fourth refusal for the duplicate-UUID case below, and left a comment on the step explaining why the extra --set flags are load-bearing, so nobody tidies them away.

Duplicate nodeUuids were accepted

Correct, and worse than it first looks. Two pods sharing a broker UUID share an identity: with data.lease.enabled they present the same one to the metadata plane and race each other for the same range lease, so the StatefulSet is not a set of replicas but two claimants wearing one name. The CN convention compounds it — a single certificate would be valid for both.

Refused before assignment, naming the duplicates. I used a template check rather than uniqueItems: true so the error can say why rather than reporting a schema violation.

String-valued names rendered unquoted

Right. The schema keeps the input a string; only quoting keeps the output one. A Secret or storage class named true was emitted as a YAML boolean and the manifest rejected on a type error rather than anything meaningful.

Verified:

$ helm template ... --set-string tls.metaSecretName=true | grep secretName
            secretName: "true"

The README still sent people to the admin Service

My own miss from the previous round: I fixed NOTES.txt and left the README pointing at a port that is now off by default, so the documented default workflow could not work. It now shows the per-pod port-forward, the server_name that has to accompany it, and why the Service does not publish that port.

---

All four verified locally: actionlint clean, helm lint clean, 6 manifests rendered, and all four refusals refusing.

[cubic-dev-ai]

6 issues found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="scripts/k8s-smoke.sh">

<violation number="1" location="scripts/k8s-smoke.sh:17">
P2: The smoke test instructs building the image with `docker build -f docker/Dockerfile`, but that Dockerfile only builds and copies the `vtopctl` binary — it never builds `vtop-node` (the chart's own README 'Image caveat' states exactly this). The script's first check waits for the StatefulSet pods to become Ready, and those pods exec `vtop-node node`; in an image lacking `vtop-node` every pod would CrashLoopBackOff and the test would fail at its very first assertion — the same #281 bug the test exists to detect. Building the image as documented therefore cannot produce a working run. Worth aligning the recipe with reality: either extend `docker/Dockerfile` to also build and copy `vtop-node`, or make the smoke test build/push an image that includes both binaries (e.g. `cargo build --release --bin vtopctl --bin vtop-node` and copy both, or a dedicated Dockerfile target) before the script runs.</violation>

<violation number="2" location="scripts/k8s-smoke.sh:25">
P1: Setting `CLUSTER_DOMAIN` to a non-default domain makes the certificates use one FQDN while the rendered peer configs still dial `cluster.local`, causing peer TLS/name validation to fail and the pods never to become Ready. The selected domain should also be passed to Helm as `clusterDomain` (or the script should reject the override).</violation>

<violation number="3" location="scripts/k8s-smoke.sh:28">
P1: Using a release name that does not contain `vtop` makes the smoke test target a non-existent pod and mint certificates for the wrong DNS names, so it cannot bootstrap. The script should resolve the chart’s actual fullname (including overrides/truncation) and use that for pod names, Service names, and SANs.</violation>

<violation number="4" location="scripts/k8s-smoke.sh:45">
P0: A normal completion or any failure can delete unrelated workloads in the caller-supplied namespace and uninstall an existing release with the same name. The smoke test should use an isolated, uniquely owned namespace/release or track ownership and clean up only resources it created.</violation>

<violation number="5" location="scripts/k8s-smoke.sh:187">
P2: The documented invocation fails before exercising the cluster unless `vtopctl`, `vtop-node`, and `curl` happen to be installed separately on the host; building the documented image does not provide those commands, especially `vtop-node`. The smoke test should preflight or build the host binaries and require `curl`, or invoke known binary paths and document that setup.</violation>

<violation number="6" location="scripts/k8s-smoke.sh:211">
P2: The independent-range check can pass without observing pod 1's offset metric, leaving a missing or renamed metric indistinguishable from zero records. Require a non-empty metric value before asserting it equals zero.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P0: A normal completion or any failure can delete unrelated workloads in the caller-supplied namespace and uninstall an existing release with the same name. The smoke test should use an isolated, uniquely owned namespace/release or track ownership and clean up only resources it created.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 45:

<comment>A normal completion or any failure can delete unrelated workloads in the caller-supplied namespace and uninstall an existing release with the same name. The smoke test should use an isolated, uniquely owned namespace/release or track ownership and clean up only resources it created.</comment>

<file context>
@@ -0,0 +1,229 @@
+  for pid in ${FORWARDS:-}; do kill "$pid" 2>/dev/null || true; done
+  if [ "${KEEP:-0}" != "1" ]; then
+    helm uninstall "$REL" -n "$NS" >/dev/null 2>&1 || true
+    kubectl delete namespace "$NS" --wait=false >/dev/null 2>&1 || true
+  fi
+  rm -rf "$WORK"
</file context>

[cubic-dev-ai]

P1: Setting CLUSTER_DOMAIN to a non-default domain makes the certificates use one FQDN while the rendered peer configs still dial cluster.local, causing peer TLS/name validation to fail and the pods never to become Ready. The selected domain should also be passed to Helm as clusterDomain (or the script should reject the override).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 25:

<comment>Setting `CLUSTER_DOMAIN` to a non-default domain makes the certificates use one FQDN while the rendered peer configs still dial `cluster.local`, causing peer TLS/name validation to fail and the pods never to become Ready. The selected domain should also be passed to Helm as `clusterDomain` (or the script should reject the override).</comment>

<file context>
@@ -0,0 +1,229 @@
+REL="${2:-vtop}"
+IMAGE_REPO="${IMAGE_REPO:-vtop-engine}"
+IMAGE_TAG="${IMAGE_TAG:-local}"
+DOMAIN="${CLUSTER_DOMAIN:-cluster.local}"
+WORK="$(mktemp -d)"
+CERTS="$WORK/certs"
</file context>

[cubic-dev-ai]

P1: Using a release name that does not contain vtop makes the smoke test target a non-existent pod and mint certificates for the wrong DNS names, so it cannot bootstrap. The script should resolve the chart’s actual fullname (including overrides/truncation) and use that for pod names, Service names, and SANs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 28:

<comment>Using a release name that does not contain `vtop` makes the smoke test target a non-existent pod and mint certificates for the wrong DNS names, so it cannot bootstrap. The script should resolve the chart’s actual fullname (including overrides/truncation) and use that for pod names, Service names, and SANs.</comment>

<file context>
@@ -0,0 +1,229 @@
+DOMAIN="${CLUSTER_DOMAIN:-cluster.local}"
+WORK="$(mktemp -d)"
+CERTS="$WORK/certs"
+HEADLESS="${REL}-headless"
+
+CLUSTER_ID=11111111-2222-3333-4444-555555555555
</file context>

[cubic-dev-ai]

P2: The independent-range check can pass without observing pod 1's offset metric, leaving a missing or renamed metric indistinguishable from zero records. Require a non-empty metric value before asserting it equals zero.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 211:

<comment>The independent-range check can pass without observing pod 1's offset metric, leaving a missing or renamed metric indistinguishable from zero records. Require a non-empty metric value before asserting it equals zero.</comment>

<file context>
@@ -0,0 +1,229 @@
+# deployment is not the shape the chart documents.
+forward "${REL}-1" 19501 9500
+other="$(curl -sf localhost:19501/metrics | awk '/^vtop_broker_next_offset\{/ { print $NF }' | head -1)"
+[ "${other:-0}" = "0" ] || fail "pod 1 holds $other records; ranges are not independent as documented"
+log "pod 1 is empty, confirming per-pod independent ranges"
+
</file context>

[cubic-dev-ai]

P2: The documented invocation fails before exercising the cluster unless vtopctl, vtop-node, and curl happen to be installed separately on the host; building the documented image does not provide those commands, especially vtop-node. The smoke test should preflight or build the host binaries and require curl, or invoke known binary paths and document that setup.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 187:

<comment>The documented invocation fails before exercising the cluster unless `vtopctl`, `vtop-node`, and `curl` happen to be installed separately on the host; building the documented image does not provide those commands, especially `vtop-node`. The smoke test should preflight or build the host binaries and require `curl`, or invoke known binary paths and document that setup.</comment>

<file context>
@@ -0,0 +1,229 @@
+PER_ROUND=50
+for epoch in $(seq 1 "$ROUNDS"); do
+  sed -i.bak "s/^producer_epoch: .*/producer_epoch: $epoch/" "$WORK/client.yaml"
+  vtop-node produce --client-config "$WORK/client.yaml" \
+    --addr "127.0.0.1:19400" --records "$PER_ROUND" --batch 10 \
+    --durability local-fsync >/dev/null || fail "produce round $epoch failed"
</file context>

[cubic-dev-ai]

P2: The smoke test instructs building the image with docker build -f docker/Dockerfile, but that Dockerfile only builds and copies the vtopctl binary — it never builds vtop-node (the chart's own README 'Image caveat' states exactly this). The script's first check waits for the StatefulSet pods to become Ready, and those pods exec vtop-node node; in an image lacking vtop-node every pod would CrashLoopBackOff and the test would fail at its very first assertion — the same #281 bug the test exists to detect. Building the image as documented therefore cannot produce a working run. Worth aligning the recipe with reality: either extend docker/Dockerfile to also build and copy vtop-node, or make the smoke test build/push an image that includes both binaries (e.g. cargo build --release --bin vtopctl --bin vtop-node and copy both, or a dedicated Dockerfile target) before the script runs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 17:

<comment>The smoke test instructs building the image with `docker build -f docker/Dockerfile`, but that Dockerfile only builds and copies the `vtopctl` binary — it never builds `vtop-node` (the chart's own README 'Image caveat' states exactly this). The script's first check waits for the StatefulSet pods to become Ready, and those pods exec `vtop-node node`; in an image lacking `vtop-node` every pod would CrashLoopBackOff and the test would fail at its very first assertion — the same #281 bug the test exists to detect. Building the image as documented therefore cannot produce a working run. Worth aligning the recipe with reality: either extend `docker/Dockerfile` to also build and copy `vtop-node`, or make the smoke test build/push an image that includes both binaries (e.g. `cargo build --release --bin vtopctl --bin vtop-node` and copy both, or a dedicated Dockerfile target) before the script runs.</comment>

<file context>
@@ -0,0 +1,229 @@
+# Usage:  scripts/k8s-smoke.sh [namespace] [release]
+#
+# Requires: a reachable cluster, kubectl, helm, openssl, and a locally built
+# image tagged vtop-engine:local (docker build -f docker/Dockerfile).
+
+set -euo pipefail
</file context>

[cubic-dev-ai]

3 issues found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".github/workflows/ci.yml">

<violation number="1" location=".github/workflows/ci.yml:301">
P2: Changes to `scripts/k8s-smoke.sh` do not rerun this deployment gate: paths-filter reports them as `scripts`, while this condition ignores that output. Gating on the smoke-script path as part of the Helm filter, or including the scripts output here, would prevent a broken harness from bypassing live validation.</violation>

<violation number="2" location=".github/workflows/ci.yml:328">
P1: The new deploy gate fails before the smoke test because the image under test does not contain `/usr/local/bin/vtop-node`. The Dockerfile needs to build and copy `vtop-node` (or this extraction must use an image that contains it) before this job can pass.</violation>

<violation number="3" location=".github/workflows/ci.yml:340">
P1: The kind smoke deployment cannot reach Ready with the chart defaults because its two PVCs require a default StorageClass, while this job creates no storage provisioner or test storage class. Installing a kind-compatible provisioner or configuring the smoke deployment with bound test volumes would make the live gate runnable.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: The kind smoke deployment cannot reach Ready with the chart defaults because its two PVCs require a default StorageClass, while this job creates no storage provisioner or test storage class. Installing a kind-compatible provisioner or configuring the smoke deployment with bound test volumes would make the live gate runnable.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 340:

<comment>The kind smoke deployment cannot reach Ready with the chart defaults because its two PVCs require a default StorageClass, while this job creates no storage provisioner or test storage class. Installing a kind-compatible provisioner or configuring the smoke deployment with bound test volumes would make the live gate runnable.</comment>

<file context>
@@ -295,6 +295,57 @@ jobs:
+      # The half the static checks cannot reach: pods Ready, Raft bootstrapped,
+      # records streamed and counted, and every record surviving a killed pod.
+      - name: Deploy the chart and verify it works
+        run: scripts/k8s-smoke.sh vtop-ci vtop
+
+      - name: Cluster state on failure
</file context>

[cubic-dev-ai]

P1: The new deploy gate fails before the smoke test because the image under test does not contain /usr/local/bin/vtop-node. The Dockerfile needs to build and copy vtop-node (or this extraction must use an image that contains it) before this job can pass.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 328:

<comment>The new deploy gate fails before the smoke test because the image under test does not contain `/usr/local/bin/vtop-node`. The Dockerfile needs to build and copy `vtop-node` (or this extraction must use an image that contains it) before this job can pass.</comment>

<file context>
@@ -295,6 +295,57 @@ jobs:
+          mkdir -p "$RUNNER_TEMP/bin"
+          container="$(docker create vtop-engine:local)"
+          for binary in vtopctl vtop-node; do
+            docker cp "$container:/usr/local/bin/$binary" "$RUNNER_TEMP/bin/$binary"
+          done
+          docker rm "$container" >/dev/null
</file context>

[cubic-dev-ai]

P2: Changes to scripts/k8s-smoke.sh do not rerun this deployment gate: paths-filter reports them as scripts, while this condition ignores that output. Gating on the smoke-script path as part of the Helm filter, or including the scripts output here, would prevent a broken harness from bypassing live validation.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 301:

<comment>Changes to `scripts/k8s-smoke.sh` do not rerun this deployment gate: paths-filter reports them as `scripts`, while this condition ignores that output. Gating on the smoke-script path as part of the Helm filter, or including the scripts output here, would prevent a broken harness from bypassing live validation.</comment>

<file context>
@@ -295,6 +295,57 @@ jobs:
+  helm-deploy:
+    name: helm (deploy to kind)
+    needs: changes
+    if: needs.changes.outputs.helm == 'true' || needs.changes.outputs.rust == 'true'
+    runs-on: ubuntu-latest
+    timeout-minutes: 45
</file context>

[allamiro]

Correcting the record: #282 merged far more than it said it did, and that was my mistake.

Its title and description were about shipping vtop-node in the image. What it actually merged was 19 files and 1,874 lines — the entire Helm chart plus all eleven review fixes and the helm CI job.

The cause: I created that branch with git checkout -b while sitting on feat/helm-chart instead of branching from main, so it carried this branch's whole history with it. Nobody reviewing #282 was told the chart was in it.

I flagged this exact failure earlier in this session, on #258, and wrote that flagging it beat letting it pass unnoticed. Then I did it again, and worse — #258's description at least described the code, while #282's did not mention the chart at all.

What that means in practice:

• The chart, all eleven fixes, and the helm lint/render/refusal job are on main already. They were reviewed here even if they merged there, so no code went in unreviewed — but the merge commit is misleading and will stay that way in the history.
• This PR is rebased onto main and now contains only what genuinely remains: the deployment smoke test, the helm-deploy kind job, and the README note about cold-start restarts. Three files, 308 lines.
• Every review fix survived the rebase; I checked the specific ones rather than assuming (contains duplicates guard, the | quote on secret names, the smoke test, the kind job), and re-ran lint, render, and actionlint afterwards.

The one thing worth taking from it beyond the apology: helm-deploy failed on the pre-rebase branch with Could not find /usr/local/bin/vtop-node — the exact bug it exists to catch, on a branch that predated the fix. The gate works, and it demonstrated that on its first run rather than in principle.