ST5: automate the live gate — prove leaf execs land on the relay worker

[pdettori]

Automates ST5's live gate

Adds deploy/knative/relay-leaf-smoke.sh: a gated Kind smoke that proves a leaf's tool
calls executed on the remote gRPC worker rather than an in-cluster sandbox pod, plus the
two gaps that blocked anyone from reproducing that by following the docs.

Until now the remote path had no automation at all — nothing under deploy/ referenced
SH_REMOTE_SANDBOX. It had only ever been validated by hand.

Why the assertion is about which backend served

A green leaf run does not prove the remote worker handled it. select-sandbox builds
candidates = [...pods, ...grpcRecs] and leases least-loaded-first, so with idle
in-cluster sandbox pods present a pod wins even when SH_REMOTE_SANDBOX=1. Validating
this by hand, my first remote run returned a correct-looking verdict that had been produced
by an Alpine pod, and it looked like success.

So the gate asserts the backend, using a filesystem discriminator — the sandbox-N pods run
Alpine, the worker image runs RHEL, and the model's stated reason names the OS it read:

Backend	pattern: "Alpine"	pattern: "Red Hat"
kubectl sandbox pod	FLAGGED	CLEAR
remote worker	CLEAR	FLAGGED

Both fingerprints are asserted on the remote path, so a silent landing on a pod fails either
way, with a message saying so rather than a bare verdict mismatch.

The discriminator is verified, not assumed. The script reads /etc/os-release from both
backends and aborts if they no longer differ as expected — otherwise the day someone changes
a base image, this quietly degrades into a tautology that passes while proving nothing.

Negative test. With the pool selector left at its default so a pod wins, the
"Alpine must be CLEAR" assertion fails as designed. A smoke that cannot detect the wrong
backend was the one thing this must not ship as, so that path was exercised deliberately.

What it asserts (6)

• worker's live Attach appears in Redis presence with transport=grpc
• the Alpine/RHEL discriminator holds
• remote: Alpine → CLEAR
• remote: Red Hat → FLAGGED
• after restore, control run on the pod path: Alpine → FLAGGED again
• presence record is gone after teardown

Restore is trap-driven

An EXIT trap restores the harness env and deletes the relay/worker even on assertion
failure or interrupt. Leaving a cluster flipped to the remote path with a pool selector
matching no pods would break every subsequent leaf run — a worse outcome than the smoke
failing. Two mechanical traps are handled: kubectl set env does not work on a Knative
Service (no kind "Service" is registered), and a merge patch that drops the container's
volumeMounts is rejected by Knative's webhook (volume with name "tmp" not mounted).

The two gaps it depends on

• relay-deployment.yaml omitted SH_RELAY_TOKEN. Relay auth is fail-closed, so
  applying the manifest as shipped produced a relay that rejected every worker Attach. Now
  set to dev-token, matching what worker-example.yaml already hardcodes, with a note that
  production wants a secretKeyRef (ST4 worker: follow-ups deferred during review #173).
• README-worker.md did not warn about the pool-peer trap. It now states plainly that
  SH_REMOTE_SANDBOX=1 alone does not route execs to a worker, and that a leaf can succeed
  against a pod while looking like a successful remote run.

Verification

Passing run: 6/6 assertions, exit 0. Negative test: fails as required. Cluster verified
restored afterwards — original harness env, no relay/worker, empty presence, and a leaf on
the kubectl path working again.

shellcheck -x -S warning (CI's gate, security-scans.yml:66) is clean. Note plain
pre-commit shellcheck -x reports style findings on this script (6) as it does on the
committed leaf-smoke.sh (10) — the new script is consistent with the existing baseline
rather than stricter or looser.

Scope

Kind only. #88's title claims "Kind → OCP" and the OCP half is untested — running this
against OCP is the remaining work there, along with a recorded decision on req_id
uniqueness (see #89), which is the one contract-level gap I would want a position on before
the epic closes.

Refs #88, #89

[pdettori]

Self-review (author = reviewer, so this is posted as a COMMENT — GitHub rejects APPROVE/REQUEST_CHANGES on one's own PR). No blocking issues found.

Every technical claim in the description was verified against upstream source, including the subtle one that motivates the whole gate:

Claim	Verified
select-sandbox pools pods + workers, least-loaded-first	select-sandbox.ts:103 candidates = [...pods, ...grpcRecs.map(r => r.sandboxId)], orderByLoad ascending — a pod really can win the lease under SH_REMOTE_SANDBOX=1
Relay auth is fail-closed	main.ts:95 expected !== undefined && token === expected — a missing SH_RELAY_TOKEN rejected every Attach. Real bug, correct fix
Alpine/RHEL discriminator	sandbox-pool.yaml = alpine:3.20 (×3); default selector sh.kagenti.io/sandbox-pool=default matches the script's fallback
Token matches the worker	worker-example.yaml:48 SANDBOX_TOKEN: dev-token
"transport":"grpc" grep	pool-records.ts:32 JSON.stringify(rec) + relay.ts:76 transport: "grpc" — substring matches exactly
shellcheck -x -S warning is CI's gate	security-scans.yml: find deploy/ -name '*.sh' -exec shellcheck -x -S warning — covers the new script; job passed
Assertions accumulate under set -e	lib.sh:36-37 — ok/ko both end in plain assignments (exit 0), so a failed assertion does not abort the run

The negative test and the verify-the-discriminator-before-relying-on-it step are the right instincts — a smoke that can't detect the wrong backend would have been worse than no smoke.

The four findings are all about the durability of the two fixes, not the smoke itself: the token fix has no CI-visible guard, its blast radius includes OCP, and the restore trap can be disarmed by exactly the patch failures the PR documents.

On the open scope question: agree Kind-only is the right boundary here. Finding #2 is the one that interacts with the OCP half of #88, so it's worth settling alongside req_id uniqueness (#89) before the epic closes.

Areas reviewed: Shell, YAML/K8s manifests, Docs, CI config, Security, Tests
Agent/IDE config (.claude/.vscode): none
Commits: 1, signed-off: yes
CI: passing (10/10, incl. trivy-scan)

Assisted-By: Claude Code

[pdettori]

suggestion — this fix has no CI-visible guard.

packages/knative-server/test/relay-deployment.test.ts already exists for exactly this manifest, and already carries a regression test for a past bug in the same spirit (the workingDir / #102 follow-up). Nothing in it asserts SH_RELAY_TOKEN.

The only thing currently asserting the token is the new smoke, which is gated behind RELAY_LIVE_SMOKE=1 — and no workflow sets that variable (checked every file under .github/workflows/). So if someone drops this env var again, the relay silently returns to rejecting every worker Attach and CI stays green.

A few lines in the existing test file would close it permanently. Worth noting the real invariant is cross-file — relay SH_RELAY_TOKEN must equal worker-example.yaml's SANDBOX_TOKEN — and right now that only lives in prose comments in both files, so a test is the only place it can actually be enforced:

it("sets SH_RELAY_TOKEN matching the worker example (relay auth is fail-closed)", () => {
  const c = docs().find((o) => o.kind === "Deployment").spec.template.spec.containers[0];
  const token = c.env.find((e) => e.name === "SH_RELAY_TOKEN");
  expect(token?.value).toBeTruthy();
  const worker = parse(readFileSync(resolve(DEPLOY, "worker-example.yaml"), "utf8"));
  const wEnv = worker.spec.template.spec.containers[0].env;
  expect(token.value).toBe(wEnv.find((e) => e.name === "SANDBOX_TOKEN").value);
});

[pdettori]

Fixed in 9ac340d. relay-deployment.test.ts is now 4/4, and the new test asserts the
cross-file invariant rather than just presence — relay SH_RELAY_TOKEN must equal
worker-example.yaml's SANDBOX_TOKEN.

Verified it can fail, since a guard that can't is worse than none: removing the env from the
manifest gives

AssertionError: relay auth is fail-closed ...: expected undefined to be truthy

then 4/4 green again after restoring.

Your framing was the useful part. I had treated "the smoke asserts it" as coverage, but the
smoke is gated behind RELAY_LIVE_SMOKE=1 and no workflow sets it — I checked, and you were
right — so the token had no CI-visible guard at all. And the invariant being cross-file means
prose comments in two manifests could never have enforced it.

[pdettori]

suggestion — "fine for kind/local dev" understates the blast radius: this manifest is not Kind-only.

overlays/ocp/kustomization.yaml lists ../../relay-deployment.yaml in resources, and its only patches: entry targets kind: Sandbox — there is no relay patch. So dev-token ships to OpenShift through setup-ocp.sh as well.

This is not a regression in posture — before this change the relay rejected every Attach on OCP too — but it does flip a committed, world-readable string into a valid credential on OCP-deployed relays. The relay Service is ClusterIP h2c, so exploitation needs in-cluster network access, which keeps it out of must-fix territory.

Two options, either is fine: narrow this comment to say the OCP overlay inherits the literal and that #173 is a prerequisite for any non-dev OCP worker use, or add an overlays/ocp patch swapping in a secretKeyRef now. Given #88's OCP half is still open, calling it out here means it can't be missed when that work lands.

[pdettori]

Corrected in 31fb80c. You were right and my comment was misleading — I verified it:
deploy/knative/kustomization.yaml:17 includes the manifest, and
overlays/ocp/kustomization.yaml:31 includes ../../relay-deployment.yaml while its
patches: block targets only kind: Sandbox. So dev-token does ship to OpenShift.

The comment now names the overlay path, says plainly that the literal is inherited there and
is therefore a valid credential on any OCP-deployed relay, and marks #173 (secretKeyRef)
as a prerequisite for non-dev OCP worker use. "Fine for kind/local dev" was simply wrong.

I took the narrow option rather than adding an overlays/ocp patch now: a secretKeyRef
needs a Secret created and managed in setup-ocp.sh, which is #88's OCP half and could break
that flow as a drive-by. Your point about #88's OCP work still being open is exactly why the
comment needed to be accurate — it is the thing that will be read when that work lands.

[pdettori]

suggestion — HARNESS_FLIPPED=0 runs even when the restore patch failed, which disarms the EXIT trap precisely when it's needed.

Line 64 only warns on failure, then execution falls through to line 66 and clears the flag. The trap's later restore_harness_env call then returns immediately at the line 59 guard, so there is no second attempt — the cluster stays flipped to the remote path with a pool selector matching no pods.

That's the outcome the file header singles out as "the worst outcome this script could produce", and the PR description documents two mechanical cases where Knative rejects patches on this very resource — so a failing patch here is a demonstrated possibility, not a hypothetical.

Clearing the flag only on success keeps the trap armed for a retry:

if kubectl patch ksvc "$KSVC" -n "$NS" --type=json \
     -p "[{\"op\":\"replace\",\"path\":\"/spec/template/spec/containers/0/env\",\"value\":$HARNESS_ENV_SNAPSHOT}]" \
     >/dev/null 2>&1; then
  wait_ksvc_ready
  HARNESS_FLIPPED=0
else
  echo "WARN: restore patch failed; leaving HARNESS_FLIPPED set so the EXIT trap retries" >&2
fi

Since the function is documented as idempotent and is called from both step 9 and the trap, a retry costs nothing when the first attempt already succeeded.

[pdettori]

Fixed in b2aef52. The flag now clears only on a successful patch, and the failure branch
says explicitly that it is leaving HARNESS_FLIPPED set so the trap retries.

You picked the one bug in here that actually matters. Everything else in this PR fails
loudly; this one fails silently in the direction of leaving the cluster broken — and it
disarms the very mechanism the file header calls the worst outcome to avoid. That the PR
itself documents two Knative patch rejections on this exact resource makes it a demonstrated
path, not a hypothetical one.

[pdettori]

nit — with set -e, a connection-level curl failure (timeout, connection refused) exits the script during the resp_alpine="$(dispatch_pattern ...)" assignment, before assert_verdict can print its "likely the model endpoint is unreachable; check the llm-credentials secret / SH_MODEL" hint.

That branch isn't dead — it still fires when the endpoint answers with an empty or non-JSON body (curl exits 0) — it just gets bypassed in the one scenario its message names, leaving a bare set -e exit instead.

This mirrors leaf-smoke.sh's dispatch_item, which is unguarded the same way, so it's consistent with the existing baseline rather than a new problem. If you want the hint reachable, || true on the curl is enough.

[pdettori]

Fixed in c080c11 — || true on the curl, so a connection-level failure yields an empty body
and lands in the existing hint path instead of a bare set -e exit.

Worth taking despite being a nit: the hint names "the model endpoint is unreachable" as the
likely cause, and an unreachable endpoint was precisely the case that bypassed it. A
diagnostic that goes missing in the scenario it was written for is worse than not having one.

Noted that leaf-smoke.sh's dispatch_item is unguarded the same way. Left alone here to
keep this PR scoped, but it is the same one-token fix if someone wants it.