fix(remote-worker): make the leaf images usable as sandboxes (/workspace + advertised capabilities)

[pdettori]

Three fixes that together make the leaf images actually usable as sandboxes. All were found by
running the P6 VM deployment on real hardware; the first two are image-only changes, the third
is the test that would have caught them.

1. /workspace did not exist

The harness execs every tool call from its sandbox working directory, which defaults to /workspace
(KAGENTI_SANDBOX_CWD, select-sandbox.ts). Neither image created it, so the worker's bash -c died
before running anything the caller asked for:

bash: line 1: cd: /workspace: No such file or directory
Command exited with code 1

The harness then reports a tool error for a sandbox that is attached, healthy and reachable — a
more confusing failure than the exec not arriving at all, because the pool looks fine, the relay looks
fine, and only the tool result dissents.

How it was found: while verifying that a turn's tool calls reach a leased sandbox. A direct probe
of the relay's Exec RPC returned hello-from-sandbox / uid=1001 / exit 0 from the same container
at the same moment, so the transport was provably fine — the exec arrived and died on the cd.

Fix: install -d -o 1001:0 -m 775 /workspace plus WORKDIR /workspace — the uid/gid these images run
as under the nonroot-v2 SCC, so the agent can write in its own workspace, and the same shape both
earlier sandbox images already use.

2. The leaf advertised capabilities it did not have

cmd/worker/main.go's probed list advertises bash, rg, base64, file, python3, git as this
worker's Hello.capabilities, and its comment states "the pool will eventually match on these, so
they must be true".

Three of the six were not installed. On a real VM the leaf logged:

worker: relay=… sandbox_id=sh-sandbox-0 tls=false capacity=4 caps=[bash base64 file]

So it advertised a set it could not honour. All six now resolve: git and python3 from the UBI 9
repositories, rg vendored.

rg is not a scheduling nicety — this is the part the first revision of this PR got wrong, caught
in review. It is the implementation of two registered tools: the Find/Glob tool runs
rg --files --hidden (k8s-sandbox/operations.ts:170) and the Grep tool runs rg directly
(grep-tool.ts), and both reach the leaf whenever a leased gRPC presence record supplies a relay
transport, because extension.ts:47-49 overrides the fast and the stream transport. Without it the
model's Glob returns glob failed in pod (rg exited 127) and its Grep rg failed in pod (exit 127) —
the same attached-and-healthy-yet-every-tool-fails signature as bug 1, on images this PR was
declaring fit for use. Both other sandbox images in the repo install it.

Confirmed ripgrep is genuinely absent from UBI 9 (microdnf install ripgrep → No package matches),
so it is vendored: the upstream static musl build, pinned to 15.2.0 and checked against the
sha256 the release publishes, for both x86_64 and aarch64 (the images are built for the host, so
an arm64 build is a real path). Fetched in its own stage, so the tar/gzip needed to unpack it never
reach the runtime image. Arch from uname -m rather than TARGETARCH, because buildah and
oc new-build --strategy=docker do not populate BuildKit's automatic args — and build-image.sh's
default path is exactly that.

The pinned version is not arbitrary, and review caught that too. createPodFindOps.glob leans on
two documented .gitignore nuances, and operations.ts plus the M3 spec record them as verified on
rg 14.1.0 — two majors below what this PR pins. Pinning back is not available: 14.1.0 publishes
no aarch64-unknown-linux-musl asset, only -gnu, so it would break the arm64 path or give up the
static property. So both nuances were re-verified on 15.2.0, against the exact pinned tarballs under
ubi9/ubi-minimal, on aarch64 natively and x86_64 emulated, with both digests matching the published
.sha256:

	rg 14.1.0 (alpine:3.20)	rg 15.2.0 (pinned)
gitignored DIRECTORY pruned even though -g matches inside	excluded	excluded
individually-gitignored FILE re-included by positive -g	re-included	re-included

Unchanged, so the pin stands. The 14.1.0 baseline was run first to prove the fixture can detect
the nuance rather than pass for unrelated reasons. operations.ts and both spec notes now say
14.1.0 verified / 15.2.0 re-verified, and the Dockerfiles say a bump must re-check the nuances, not
only the digests.

This also gates measurement fidelity. Spec §2.3's duty bases were derived from workloads whose git
operations cost ~470 ms. Without git in the sandbox an E8 tool call can only be a ~0 ms no-op, so the
hands tier is exercised structurally but carries no load, and the measured duty cycle describes a
cheaper workload than the basis it is compared against.

3. Nothing related probed to the images

Neither Dockerfile is built by CI — build.yaml's matrix is the harness, the OCP sandbox and the echo
target, and it only fires on push to main; ci.yml's remote-worker job is the Go build; hadolint
parses these files but does not resolve packages. So the exact mismatch in bug 2 had no guard, which
is why it survived to be found on a VM, and the two Dockerfiles' agreement with each other rested on
a "see the same block in ./Dockerfile" comment.

remote-worker/cmd/worker/dockerfile_parity_test.go is that guard. In-package, so it references
probed directly rather than parsing main.go — the list it checks cannot drift from the list the
worker advertises. It asserts every probed tool is installed by both files, that the two install the
same package set, that both create /workspace identically and set WORKDIR, and that the vendored
binary stays pinned and digest-verified. Runs in the existing go test -race ./... step: no daemon,
no network, no new job.

The first version of this guard was hollow, and review caught it. It matched each token with
strings.Contains over the whole file, so four of the six were satisfied by comment prose — bash
in "the worker runs bash -c", git and python3 in "git and python3 are here because…", and
file inside the word "Dockerfile". Only rg and base64 were genuinely checked, their tokens
being a tarball name and coreutils-single. Deleting git python3 from both install lines passed;
so did stripping the line down to coreutils-single findutils.

Worth stating plainly because it is the more useful lesson: the negative test originally cited here
("against the pre-fix tree it reports rg, git and python3 missing") held only because those
explanatory comment blocks did not yet exist. The guard was validated in the one tree state where
the hole was invisible, and the commit adding the comments is what opened it. A guard a comment can
satisfy is worse than none, because it reads as covered.

Every assertion now runs against instructions with comment lines stripped, and package tokens must
appear as an exact field of the parsed microdnf install list rather than as a substring anywhere.
provides still maps each tool to what puts it on PATH (base64 ← coreutils-single, rg ← the
tarball) — a test grepping for the tool's own name would have passed on the broken image — and now
also records where that has to appear, so neither form is satisfiable by prose.

Twelve negative cases, each mutation applied and checked against its expectation:

drop git+python3 from both install lines      -> FAIL (names git, python3)
drop bash+file+git+python3 from both          -> FAIL (names all four)
drop coreutils-single from both               -> FAIL (names base64)
remove the rg tarball reference from both     -> FAIL (does not vendor "rg")
drop git from Dockerfile.runtime only         -> FAIL (different package sets)
remove `install -d /workspace` from one file  -> FAIL
remove `WORKDIR /workspace` from one file     -> FAIL
remove the whole /workspace block from both   -> FAIL
change /workspace owner in one file only      -> FAIL (different owner/group/mode)
remove the sha256sum step from both           -> FAIL
unpin RG_VERSION + drop the x86_64 digest     -> FAIL
reorder Dockerfile.runtime's package list     -> PASS (must not fire)

What it cannot do is prove a package still resolves in a future UBI 9 minor — only building can.
deploy/knative/verify-sandbox-inventory.sh is the in-image counterpart and the pattern to follow if
these images are ever published to GHCR.

Notes

• All changes are applied to Dockerfile (multi-stage) and Dockerfile.runtime (prebuilt
  binary), since either can be the sandbox.
• The DL3041 ignore directives were repositioned to abut their RUN lines — a comment block
  between a hadolint directive and its target silences nothing, which make lint caught. The digest
  check writes a file rather than piping into sha256sum -c - for the same reason: DL4006, and
  de-piping keeps the check exactly as strict without depending on pipefail.
• Dockerfile.runtime now needs egress to github.com for the pinned tarball, narrowing its
  header's claim that the in-cluster OpenShift build needs no egress. It would fail in a disconnected
  cluster. Consistency between the two files won over preserving that property, since the alternative
  was leaving Grep and Find broken in every sandbox built from it; the header records the fallback
  (have build-image.sh place rg in dist/ and COPY it, as the worker binary already arrives).
• Together these falsify the Dockerfile's own claim that "this stage exists so the standalone demo
  image works on its own": as a sandbox it failed every tool call.
• Image size grows by roughly git + python3 + a 1.9 MB static rg. Acceptable for an image whose
  stated purpose is standalone demo and measurement use.

Verification

Both files built on both architectures:

• the leaf logs caps=[bash rg base64 file python3 git] — the full probed list, against the
  caps=[bash base64 file] observed on the VM
• /workspace is drwxrwxr-x 2 1001 root, pwd is /workspace, and uid 1001 can write there
• rg 15.2.0 runs and matches in-image, proving the static musl binary is viable on this base
• a corrupted digest fails the build closed (sha256sum: WARNING: 1 computed checksum did NOT match)
• all three leaves log attached, serving execs
• a /turn tool call's file landed in sh-sandbox-2, and nothing appeared in the supervisor
  unit's PrivateTmp namespace — the negative half being the one that matters
• make lint clean (hadolint included); remote-worker Go tests pass, including the new parity test
• after review round 2: 12/12 CI green, hadolint clean on both files with all four
  hadolint ignore= directives still abutting their targets, and the twelve parity negative cases
  above re-run against the final tree

Related

• fix(harness): route /turn's tool calls to the sandbox pool #253 — /turn routing tool calls to the pool, which is what surfaced all of these
• The supervisor-side pool telemetry follows once feat(supervisor): P6 VM process manager — fd hand-off, admission control, systemd deployment #250 and fix(harness): route /turn's tool calls to the sandbox pool #253 land

Assisted-By: Claude Code

[cwiklik]

Reviewed 8ba6bee in full — 2 files, +34/−2, small enough to read line by line rather than by summary, and I verified every claim the comments make against the source rather than against the narrative.

One must-fix, one suggestion, one nit. To be clear up front: both fixes here are real and the diagnosis behind each is correct. This is a strict improvement over main and I would not want it read otherwise. The blocker is that the PR stops one capability short of its own title, and the in-code rationale for stopping there is wrong about the consequence — rg's absence is not a scheduling-metadata gap, it breaks the agent's Grep and Find tools on exactly the images this PR is declaring fit for use as sandboxes.

Claims verified

Claim in the diff	Verified
probed advertises bash, rg, base64, file, python3, git	cmd/worker/main.go:41, exactly that list
Three of the six were missing	capabilities() (:76-84) is a real exec.LookPath probe over probed, and the pre-fix install line was bash coreutils-single findutils file — so bash/base64/file resolve and rg/python3/git do not. The observed caps=[bash base64 file] is exactly what that image should report.
The harness execs from /workspace by default	select-sandbox.ts:83 — env.KAGENTI_SANDBOX_CWD ?? '/workspace', and the same default appears in k8s-sandbox/src/config.ts:26 and resolve-pod.ts:76
A missing /workspace kills the exec before the command runs	internal/exec/runner.go:159 — "Every command the harness sends is self-contained (cd 'cwd' && …)". With no such directory the cd fails and nothing after && runs, which is the reported bash: line 1: cd: /workspace: No such file or directory
1001:0 matches how the image runs	USER 1001 is in the image, gid 0 + mode 775 also covers the arbitrary-UID case if a restricted-v2-style SCC replaces the UID, so this is right for both. deploy/knative/sandbox.Dockerfile:15-17 reaches the same place via chgrp -R 0 + chmod -R g=u.
rg is not in the UBI 9 repositories	Correct — ripgrep is EPEL, not RHEL 9 base or AppStream. It is the conclusion drawn from this that I disagree with, not the fact.

One thing worth recording because it is easy to misread: the pre-fix behaviour was not a false advertisement. capabilities() probes PATH and reports only what it finds, and main_test.go:41-50 pins capabilities ⊆ probed, so the wire was always honest. The defect is that the image was less capable than probed says it was designed to be — which is why no test caught it and why a test that could is worth adding (see the suggestion on the install line).

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Dockerfile (both), plus the Go capability probe, the harness sandbox-selection and tool-operation paths, and the CI build matrix as cross-references
Agent/IDE config (.claude/.vscode): none — grepped both +++ b/ and rename to forms; 0 renames in the diff
Commits: 2 (5d553ca, 8ba6bee), both signed off, both Assisted-By per house convention, both fix(remote-worker):, subjects 56 and 64 chars
CI status: passing — 12/12 green on 8ba6bee (DCO, CodeQL, Trivy, shellcheck, hadolint, dependency-review, lint, proto, deploy-scripts, check). Note that none of these builds either Dockerfile — see the suggestion below.
Base: 694464d (merge-base confirmed); mergeable_state: blocked pending review; head is from the pdettori/serverless-harness fork

[cwiklik]

must-fix — rg is not a scheduling nicety. Two of the agent's tools shell out to it, so on this image they fail — the same symptom this PR is fixing for /workspace.

This comment justifies leaving rg out purely in terms of capability matching: the pool "will eventually match on these" (main.go:39-40, deferred at relay.ts:74), so an unhonoured entry is treated as metadata drift that only matters to a future scheduler, and the remedy offered is "add it or shorten probed". That understates it. rg is the implementation of two registered tools, and the repo says so in its own words — deploy/knative/sandbox.Dockerfile:13, describing the canonical sandbox image:

findutils, grep and ripgrep (the agent's find/grep tools shell out to rg) on PATH.

That image installs it (sandbox.Dockerfile:19, apk add … ripgrep), and packages/k8s-sandbox/deploy/sandbox.yaml:35 installs it too. This PR makes the leaf images the only sandbox images in the repo without it.

The path is live for exactly the deployment this PR targets. I traced it rather than assuming:

1. A leased gRPC presence record gets a relay transport — select-sandbox.ts:113-118, transport = GrpcRelayTransport(name, …).
2. run-leaf.ts:523 passes it through as sandbox: { config, transport }.
3. extension.ts:20-22 — when opts.transport is supplied it overrides both the fast and stream transports, so every pod operation now runs over the relay, inside the leaf.
4. extension.ts:67 registers the Find tool on createPodFindOps, which runs cd <cwd> && rg --files --hidden <globs> | head -n <limit> (operations.ts:170).
5. extension.ts:74 registers the Grep tool on createPodGrepTool, which runs rg directly (grep-tool.ts).

So on a leaf sandbox the model's Glob call returns glob failed in pod (rg exited 127) (operations.ts:195) and its Grep call returns rg failed in pod (exit 127) (grep-tool.ts:52). That is the same failure signature as the /workspace bug documented 16 lines below: a sandbox that is attached and healthy, returning a tool error for something the caller legitimately asked for. grep-tool.test.ts:41 even flags this text as "blames ripgrep for our own cap" — and here it would be blaming ripgrep for its absence.

Options, in the order I would take them:

• Vendor the static binary. ripgrep publishes a x86_64-unknown-linux-musl tarball per release; a pinned curl | tar into /usr/local/bin in the build stage is self-contained, needs no extra repository, and adds one file. This is the usual answer for UBI images and keeps the "no EPEL" property the comment is protecting.
• EPEL, if adding a repository to a demo image is acceptable — simpler, but it is a real supply-chain surface on an image that executes model-authored commands, so I would not push for it.
• Ship without rg, but resolve it honestly: drop rg from probed and rewrite this comment to say that the Grep and Find tools do not work on this image. That is a legitimate call for an experiment-only image — but then the PR title's "usable as sandboxes" needs narrowing, because two tools the model will reach for do not work. I would rather have the binary.

The reason I am treating this as blocking rather than a follow-up: the comment as written tells the next reader that the remaining gap costs nothing until capability matching lands. Someone will read that, and it is the one part of this otherwise careful diff that will mislead them.

[pdettori]

Fixed by vendoring it — rg 15.2.0's static musl build, pinned and checksummed, in 7dd8bec.

Your trace is right, and I re-walked it rather than taking it on faith: extension.ts:47-49 overrides both transports when opts.transport is present, createPodFindOps runs rg --files --hidden (operations.ts:170) and createPodGrepTool runs rg directly, so on a leaf both return exit 127 for something the caller legitimately asked for. Two of seven tools, on the images this PR was calling fit for use. The comment as written would have told the next reader that cost nothing until capability matching lands, which is the part that mattered most in your review.

I took option 1. Two things I checked before choosing it:

• UBI 9 really does not have it — microdnf install ripgrep in ubi9/ubi-minimal:latest returns error: No package matches 'ripgrep'. So the premise held; only the conclusion drawn from it was wrong.
• Both architectures are covered as static musl. 15.2.0 publishes x86_64-unknown-linux-musl and aarch64-unknown-linux-musl with a .sha256 alongside each. That mattered: README-worker.md notes these images are built for the host, so an arm64 laptop build is a real path, and a hardcoded x86_64 binary would have been a worse bug than the one being fixed.

Shape of it, on both files:

• Its own FROM ubi-minimal AS rg stage. ubi-minimal has curl, sha256sum and install but no tar or gzip — the separate stage is what keeps those two out of the runtime image.
• Version and both per-arch digests as ARGs, verified before use. Bumping means changing three values together, and a wrong digest fails the build closed — verified, not assumed: corrupting one gives sha256sum: WARNING: 1 computed checksum did NOT match and exit 1.
• Arch from uname -m, not TARGETARCH: buildah and oc new-build --strategy=docker do not populate BuildKit's automatic args, and build-image.sh's default path goes through exactly that.
• The digest check writes a file instead of piping into sha256sum -c -. make lint flagged the pipe as DL4006, and de-piping is strictly better than an ignore directive or a SHELL line — the check stays exactly as strict without depending on pipefail.

Verified by building both files on both arches. The leaf now logs caps=[bash rg base64 file python3 git] — the full probed list, against the caps=[bash base64 file] from the VM — and rg runs and matches in-image, which is the part that proves the static musl binary is actually viable on this base.

One consequence worth flagging rather than burying: Dockerfile.runtime's header claimed the in-cluster OpenShift build needs no egress, and that is now narrower — it needs github.com for the tarball, and would fail in a disconnected cluster. I chose consistency between the two files over preserving that property, since the alternative was leaving Grep and Find broken in every sandbox built from it. The header now says so, with the fallback if it ever bites: have build-image.sh drop rg into dist/ and COPY it, the way the worker binary already arrives.

[cwiklik]

suggestion — nothing in CI builds either of these Dockerfiles, so the package-availability judgements this PR turns on are unverified, and the drift it fixes can recur silently.

The whole diff rests on claims about what UBI 9 provides — git and python3 resolve, rg does not. Those are exactly the claims no job checks:

• build.yaml's matrix is three images — serverless-harness (./Dockerfile), serverless-harness-sandbox (deploy/knative/sandbox.Dockerfile) and echo-target. Neither remote-worker/Dockerfile nor remote-worker/Dockerfile.runtime is in it, and the workflow only triggers on push to main and v* tags, so it would not run on this PR regardless.
• ci.yml:170's "Build and test remote-worker" is the Go build, not an image build.
• hadolint parses these files but does not resolve packages; Trivy scans the repo, not an image built from them.

So a package name that is absent or renamed in a future UBI 9 minor surfaces to whoever next runs setup-vm.sh's require_build by hand, not to CI. And more to the point, the specific mismatch this PR exists to fix — probed naming a tool the image does not install — has no guard, which is precisely why it survived to be found on a real VM.

The cheap guard is a parity test, and the repo already has the pattern. packages/knative-server/test/authbridge-manifests.test.ts parses deployment files and asserts invariants across them; its line 262 even enumerates the baked-in tool set in prose. The same technique applied here: parse the microdnf install line out of both Dockerfiles, parse probed out of main.go, and assert every probed tool is installed except an explicit, named exclusion list. That test fails today with rg — which is the point — and after this PR it holds the two Dockerfiles in sync with each other and with the Go list, none of which any test currently relates.

Adding the two files to build.yaml's matrix would additionally prove the packages resolve, and would publish images that setup-vm.sh currently requires an operator to build locally. Worth it independently, though it is the larger change of the two.

Keeping both Dockerfiles textually in step is a real maintenance cost now: they carry the same install line and the same three-command /workspace block, related only by a "see the same block in ./Dockerfile" comment. A test is what makes that relationship enforceable rather than aspirational.

[pdettori]

Added as a Go test — remote-worker/cmd/worker/dockerfile_parity_test.go in 86bf033.

I went with Go rather than the vitest parse you suggested, for one reason that I think makes it strictly better: in-package it can reference probed directly instead of regex-matching main.go. The list the test checks is therefore the same variable the worker advertises, so those two cannot drift — which removes one of the two drift surfaces rather than adding a parser for it. It also lands in the go test -race ./... step ci.yml:170 already runs, so it needs no new job, no daemon and no network.

Three properties, and I negative-tested each rather than trusting a green run:

Property	Negative test	Result
every probed tool is installed by both files	checked out the pre-fix tree	exit 1, naming rg, git, python3
the two Dockerfiles install the same set	dropped git from Dockerfile.runtime only	exit 1, install different package sets
the vendored binary stays pinned and verified	removed the sha256sum step	exit 1

All three pass on the branch. Your framing was the useful part here — a test that could not fail would have been worse than none, and verify-sandbox-inventory.sh documents that exact trap in its own comments, so the regex asserts a match exists and fails loudly if the pattern goes stale rather than passing vacuously.

Two details worth calling out because they are where this kind of test usually rots:

• provides maps tool → what puts it on PATH, not tool → itself. base64 comes from coreutils-single and rg from the tarball, so a test grepping for the tool's own name would have passed on the broken image. That indirection is the whole content of the test.
• knownGap exists but is empty. With rg vendored there is no gap left; keeping the mechanism means a future omission has to be written down in one place, and an unmapped tool errors rather than being silently skipped.

On the two larger items you raised: I did not add these files to build.yaml's matrix. Static analysis cannot prove a package still resolves in a future UBI 9 minor — only building can — so the gap you identified is narrowed, not closed. I did build both files on both architectures by hand for this change (caps=[bash rg base64 file python3 git], rg running in-image), but you are right that nothing repeats that automatically. The reason I left it out is that build.yaml only fires on push to main, so it would not have guarded this PR either; the honest fix is a PR-time build, and publishing these images has consumer implications that deserve their own PR rather than riding along here. verify-sandbox-inventory.sh is the pattern to follow when it happens, and the test comment says so.

[cwiklik]

nit — no WORKDIR /workspace, and this can be one command instead of three.

Both in-repo precedents create the directory and set the working directory: deploy/knative/sandbox.Dockerfile:15-19 (mkdir + chgrp + chmod, then WORKDIR /workspace) and deploy/knative/build-swebench-sandbox.sh:268-269:

RUN install -d -o 65532 -g 0 /workspace
WORKDIR /workspace

Nothing breaks today without it — runner.go:159 guarantees every command the harness sends is self-contained as cd 'cwd' && …, so the process cwd is never consulted. But that guarantee is the only thing standing between this image and a second round of the bug documented right here: any exec path that ever sends a bare command, or an operator running podman exec to debug a sandbox, lands in / instead. WORKDIR costs nothing and makes the image's contract match the two sandbox images that came before it.

The three commands also collapse to the form the swebench builder already uses, which is one layer and states owner, group and mode in one place:

RUN install -d -o 1001 -g 0 -m 775 /workspace
WORKDIR /workspace

Same result as mkdir -p + chown 1001:0 + chmod 775. Both files, since they carry identical blocks.

[pdettori]

Both applied, both files, in 7dd8bec:

RUN install -d -o 1001 -g 0 -m 775 /workspace
WORKDIR /workspace

Checked the collapse produces the identical result rather than assuming it: in ubi9/ubi-minimal, install -d -o 1001 -g 0 -m 775 /workspace gives drwxrwxr-x. 2 1001 root, the same as the three commands it replaces, in one layer.

Your reasoning on WORKDIR is the part I'd want kept in the record: nothing breaks today because runner.go:159 guarantees every command is self-contained as cd 'cwd' && …, and that single guarantee is the only thing standing between this image and a second round of the bug documented directly above it. An operator's podman exec landing in / is the likely first casualty. I put that argument in the comment instead of just the line, so the next person changing it knows why it is there — and pwd in the built image is now /workspace, verified.

Also worth noting the ordering matters and is deliberate: install -d runs first so WORKDIR finds the directory already owned 1001:0 rather than creating it root-owned.

[cwiklik]

Three fixes whose diagnosis I was able to confirm independently, and one defect in the guard that is meant to keep them fixed.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Dockerfile ×2, Go test, plus cross-file verification into packages/k8s-sandbox and harness/
Agent/IDE config (.claude/.vscode): none — supply-chain gate clear
Commits: 5, all signed-off (5/5), all conventional prefixes
CI status: passing (12/12, incl. hadolint, DCO, Trivy, CodeQL, dependency-review)

Verified rather than taken on trust

• probed is exactly {"bash","rg","base64","file","python3","git"} (read through the GitHub API, not a local checkout)
• Both pinned digests match the published .sha256 byte-for-byte — 33e15bcf… for x86_64, 800b1e72… for aarch64. ripgrep 15.2.0 is a real release (2026-07-15) and both x86_64-unknown-linux-musl and aarch64-unknown-linux-musl assets exist, so the arm64 path is genuine rather than aspirational. The build does fail closed.
• The rg-is-a-tool-implementation argument is precise: operations.ts:170 is literally cd … && rg --files --hidden, grep-tool.ts:52 throws the exact error string quoted, and extension.ts:49 and :51 both use opts?.transport ??, so the override really does cover the stream and the fast transport.
• select-sandbox.ts:83 (?? '/workspace'), runner.go:159, build-swebench-sandbox.sh:268, and ripgrep + WORKDIR /workspace in both earlier sandbox images all check out as cited.

Verdict

REQUEST_CHANGES on finding 1 only. The two image fixes are correct and I would take them as-is — the blocking item is that the parity test does not actually guard 4 of the 6 capabilities it reports on, which matters because that test is this PR's stated defence against bug 2 recurring. It is a two-line fix in a file that is already open.

One limitation worth stating

git fetch fails in my environment (self-signed certificate in the proxy chain), so my local clone is pinned at a982ffd (2026-09-09), five days behind this PR. The probed list I re-verified through the API; the TypeScript and Go citations above were read from that older tree, so if any of them moved in the last five days those line numbers are stale rather than wrong.

[cwiklik]

must-fix — this check is satisfiable by prose, including prose this PR adds.

strings.Contains(body, token) scans the whole file, comments included. Four of the six tokens now appear in comment text in both Dockerfiles:

• file — matches the substring inside "Dockerfile" and "dockerfile_parity_test.go"
• bash — "the worker runs bash -c <command>"
• git, python3 — "git and python3 are here because…" / "git and python3: see the same block in ./Dockerfile"

So deleting git python3 from both install lines still passes this test (the words are in the comments) and passes TestBothDockerfilesInstallTheSamePackages (both files equally broken). Only rg and base64 are genuinely checked, because their tokens are a tarball name and coreutils-single, neither of which appears in prose.

The negative test in the PR description — "against the pre-fix tree it reports rg, git and python3 missing" — held only because those comment blocks did not exist yet. The guard was validated in the one tree state where this hole was invisible; adding the comments is what opened it.

The provides indirection was introduced precisely so that "a test that merely grepped for the tool's own name would pass while the image stayed broken" could not happen. Whole-body matching reintroduces that failure mode one level up.

Fix: assert against the extracted install line rather than the file body — installLine in this same file already parses it for the drift test, so the machinery exists.

[pdettori]

Fixed in 52a1c01 — every assertion now runs against instructions with comment lines stripped, and package tokens must be an exact field of the parsed install list rather than a substring of the file.

You were right about the scope, and it is worse than the four tokens: I reproduced it before changing anything. Deleting git python3 from both install lines passes the old test, and so does stripping the line all the way down to coreutils-single findutils — bash, file, git and python3 all satisfied by prose, on an image the test reports as covered. Only rg and base64 were ever checked, exactly as you said.

The part worth recording is your point that the PR's own negative test held only because those comment blocks did not yet exist. That is the real defect: the guard was validated in the one tree state where the hole was invisible, and the commit that added the explanatory comments is what opened it. A guard a comment can satisfy is worse than no guard, because it reads as covered — so code() strips comment lines before anything is matched, and source now says where a tool has to appear (a package as a field of the runtime stage's microdnf list, a vendored binary in RUN text). Neither form is satisfiable by prose.

I also took both of your other findings on this file while it was open — /workspace assertions and the sorted comparison — and re-ran the whole thing as twelve negative cases rather than trusting a green run:

drop git+python3 from both install lines      -> FAIL (names git, python3)
drop bash+file+git+python3 from both          -> FAIL (names all four)
drop coreutils-single from both               -> FAIL (names base64)
remove the rg tarball reference from both     -> FAIL (does not vendor "rg")
drop git from Dockerfile.runtime only         -> FAIL (different package sets)
remove `install -d /workspace` from one file  -> FAIL
remove `WORKDIR /workspace` from one file     -> FAIL
remove the whole /workspace block from both   -> FAIL
change /workspace owner in one file only      -> FAIL (different owner/group/mode)
remove the sha256sum step from both           -> FAIL
unpin RG_VERSION + drop the x86_64 digest     -> FAIL
reorder Dockerfile.runtime's package list     -> PASS (must not fire)

One process note, since it caused a scare mid-review: my first mutation harness restored with git checkout --, which silently discarded the uncommitted Dockerfile edits from your other two findings. It now backs up to files instead. Nothing was lost, but the negative tests above were re-run against the final tree, not the intermediate one.

[cwiklik]

suggestion — version pinned here is two majors ahead of the one the dependent semantics were verified on.

This vendoring exists to serve Find/Glob, but those semantics were characterised against a different ripgrep major:

• packages/k8s-sandbox/src/operations.ts:161 — "rg --files --hidden lists files under cwd, honouring .gitignore (verified on the pod's ripgrep 14.1.0)", followed by the gitignored-DIRECTORY-vs-FILE nuance
• docs/specs/2026-06-17-m3-persistent-channel-design.md:28 and :255 — the same nuance, recorded as a "Verified nuance (rg 14.1.0)"

15.0.0 was a major release, and after this PR the leaf is a sandbox that rg --files --hidden runs inside, so that nuance is unverified on the version actually being shipped. If it changed, Glob's ignore semantics differ by which sandbox image an operator built — silently, and only for gitignored paths.

Either pin to the version the semantics were verified on, or re-verify on 15.2.0 and update both the comment and the spec note.

[pdettori]

Re-verified on 15.2.0 and updated both the comment and the spec notes — e47e70d. I took your second option, because the first turned out not to be available.

Pinning back to 14.1.0 is not possible without giving something up. That release publishes no aarch64-unknown-linux-musl asset — its only aarch64 Linux build is -gnu. So pinning to the version the semantics were characterised on would either break the arm64 path (which README-worker.md makes a real one, since these images are built for the host) or trade the static property this fetch stage exists for. That is worth recording, because "just pin to the verified version" is the obvious reading of your finding and it does not survive contact with the asset list.

So I verified the version actually being shipped, against the exact pinned tarballs rather than a convenient local build:

	rg 14.1.0 (alpine:3.20)	rg 15.2.0 (pinned tarball)
gitignored DIRECTORY pruned even though -g matches inside	excluded	excluded
individually-gitignored FILE re-included by positive -g	re-included	re-included

Both nuances hold unchanged. Method, since a check like this is easy to do vacuously:

• fixture is a git repo with .gitignore covering node_modules/ and secret.txt, then rg --files --hidden -g '*.js' (directory case) and -g 'secret.txt' (file case)
• run under ubi9/ubi-minimal with the tarball fetched and sha256sum -c verified exactly as the Dockerfile does it, on aarch64-unknown-linux-musl natively and x86_64-unknown-linux-musl emulated. Both digests matched the published .sha256.
• the baseline was run first, on alpine:3.20's rg 14.1.0, and reproduces the documented behaviour. That ordering is the point: it shows the fixture can actually detect the nuance instead of passing for some unrelated reason. An early version of the script reported "nuance CHANGED" when the binary was simply missing, so it now refuses to report a result unless rg --version runs.

On 14.1.0 not being an arbitrary baseline — it is what alpine:3.20 ships, which is why both other sandbox images and the SMOKE.md run observed it. I left SMOKE.md alone: it is a dated result record, not a live claim.

The Dockerfiles now say the version is not free to choose and that a bump has to re-check those two nuances, not just the digests — the failure mode you named (semantics differing by which image an operator built, silently, and only for gitignored paths) is not something a digest check would catch.

[cwiklik]

suggestion — bug 1 ships without the guard this file exists to provide.

The parity test covers the capability list and the ripgrep pinning, but nothing here covers /workspace — which was the PR's primary find and the more confusing of the two failures. RUN install -d … /workspace and WORKDIR /workspace could be dropped from one file, or drift between the two, and every test in this file still passes.

That is the same "related only by a see-the-same-block-in-./Dockerfile comment" exposure this file was written to close, so it seems worth two more assertions while the file is open: both images create /workspace with the same owner/mode, and both set WORKDIR.

[pdettori]

Added in 52a1c01 — TestBothDockerfilesCreateWorkspaceIdentically.

Your framing was accurate: bug 1 was the PR's primary find and the only thing in this file with no guard at all. I confirmed the exposure rather than assuming it — deleting RUN install -d ... /workspace from Dockerfile.runtime alone, deleting WORKDIR /workspace alone, and deleting the whole block from both files each left every test in the file green.

The test asserts three things, each negative-tested:

• both files carry a RUN install -d ... /workspace instruction (missing from either -> FAIL)
• both set WORKDIR /workspace (missing from either -> FAIL)
• the captured owner/group/mode agree across the two files — changing one to -o 65532 reports different owner/group/mode, not just a diff

The third is the one I would not have written from your comment alone but is the same class of exposure: matching the two instructions independently would let the files drift to different owners while both tests passed, and an agent's ability to write in its own workspace would then depend on which build path an operator used. The regex captures the flags rather than the whole line so the comparison is on owner/group/mode instead of on formatting.

Like everything else in the file it matches against comment-stripped instructions, so the words /workspace and WORKDIR appearing in the long explanatory comment above the block cannot satisfy it — which was the trap in your other finding.

[cwiklik]

nit — order-sensitive comparison can fail on a no-op change.

strings.Join(a, " ") != strings.Join(b, " ") compares sequences, not sets, so merely reordering the package list in one file reports "install different package sets" while the sets are in fact identical — and the error message actively misleads, since the two %v lists will look equivalent to the reader.

slices.Sort both slices before comparing.

[pdettori]

Fixed in 52a1c01 — sorted copies compared with slices.Equal.

Confirmed the false positive first: reordering Dockerfile.runtime's list to python3 git file findutils coreutils-single bash reported different package sets on two lists that are identical as sets. Your point about the message being the worse half is right — the reader sees two %v lists containing the same packages and has no reason to suspect ordering.

slices.Clone before sorting, so the assertion does not reorder the slices the other tests read. That case is now one of the twelve negative tests, as the one that must not fire.

[cwiklik]

nit — bare line citation in the block that 3b64c12 de-lined.

operations.ts:170 is correct today (I checked: line 170 is exactly cd ${q(cwd)} && rg --files --hidden …), but it rots the same way the extension.ts:47-49 reference did — which commit 3b64c12 ("cite the transport override by symbol, not line") deliberately replaced with opts?.transport ?? in this very comment block. Worth citing the symbol here too: createPodFindOps's glob.

Same applies to the identical block in Dockerfile.runtime.

[pdettori]

Fixed in e47e70d, both files — operations.ts:170 -> `createPodFindOps`.glob, and the grep-tool.ts reference alongside it now names createPodGrepTool for the same reason.

I extended it past what you flagged, because the same block had two more of these and they rot identically: runner.go:159 -> BashRunner (the guarantee is stated in that type's doc comment, so the symbol is where the argument actually lives), and build-swebench-sandbox.sh:268 -> "its generated Dockerfile" — that one is a heredoc with no symbol to cite, so a description is the stable form. There are now no bare file:line citations left in either Dockerfile.

Checked the symbols resolve rather than guessing: createPodFindOps is at operations.ts:155 with glob: at :167, and createPodGrepTool at grep-tool.ts:13.

One thing to flag since it touched more lines than the change itself: the substitutions pushed several comment lines past the width the rest of the block uses, so the affected paragraphs are reflowed. That inflates the diff on both files — it is whitespace only, no claim changed. hadolint is clean on both, and I checked that all four hadolint ignore= directives still directly abut their RUN/FROM lines, since reflowing a comment block near one is exactly how the DL3041 problem from earlier in this PR would come back.

[cwiklik]

Re-review of 3b64c12...e47e70d (2 commits, 5 files). All five findings are addressed, and finding 2 is resolved better than the way I proposed it. Two residual items below, both test-only and neither blocking.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Go test, Dockerfile ×2, TypeScript comment, spec doc — plus execution of the test's own parsing logic against mutated Dockerfiles
Agent/IDE config (.claude/.vscode): none — supply-chain gate clear
Commits: 7, all signed-off (7/7), conventional prefixes
CI status: passing (12/12)

Per-finding verdicts

#	Item	Verdict
1	Parity test satisfiable by comment prose	Fixed — code() strips comment lines; provides became source{pkg,vendored}, with packages matched as exact fields of the parsed install list; the default: branch makes an entry that asserts nothing an error. See the residual below
2	rg pinned two majors past the verified semantics	Fixed, and your route is better than mine — see below
3	/workspace shipped without a guard	Fixed — TestBothDockerfilesCreateWorkspaceIdentically, including an owner/group/mode drift assertion I had not asked for. See the residual below
4	Order-sensitive package comparison	Fixed — slices.Clone + Sort + Equal, with the reorder case kept as the one negative test that must not fire
5	Bare file:line citations	Fixed and extended — verified no file.ext:NNN pattern remains in either Dockerfile, and both new symbols resolve (createPodGrepTool at grep-tool.ts:13, createPodFindOps at operations.ts:155)

On finding 2: "just pin back to 14.1.0" does not survive the asset list

I offered pin-back or re-verify as equivalent options. They are not, and I checked your claim against the releases API rather than taking it:

• 14.1.0 publishes aarch64-unknown-linux-gnu and nothing else for aarch64 Linux — no musl asset.
• 15.2.0 publishes aarch64-unknown-linux-musl.

So pinning back would have broken the arm64 path or given up the static property the fetch stage exists for. Option 1 was never available, and my finding was wrong to present it as a choice.

The re-verification is also structured the right way round: running the 14.1.0 baseline first, and refusing to report a result unless rg --version runs, is what makes the fixture's two results mean something rather than passing for an unrelated reason. I also searched the repo — the only surviving 14.1.0 mention outside the two files you updated is SMOKE.md, which is correctly left alone as a dated result record.

The must-fix is genuinely closed — verified by running it

I reproduced the test's parsing logic (commentLine/code(), installLine, runtimePackages, the pkg branch) and ran it against the real HEAD Dockerfile and against mutations:

Mutation	Result
unmodified HEAD	parses [bash coreutils-single findutils file git python3], all six satisfied
drop git python3 from the install line	git, python3 → FAILS, as intended

The old whole-body strings.Contains passed that second case. The fix holds for the form that was actually broken.

Verdict

APPROVE. Both items below are test-hardening, in a guard that is already far better than the one I flagged.

One limitation worth stating

git fetch fails in my environment (self-signed certificate in the proxy chain), so my clone is pinned at a982ffd. Everything above was read from e47e70d through the API, and the mutation runs used the HEAD Dockerfile fetched the same way.

[cwiklik]

suggestion — the stripper handles comment lines, and the install list is the one place a comment is not on its own line.

commentLine is ^\s*#.*$, so code() removes full-line comments only. But installLine captures everything up to the continuation, and a trailing shell comment on that line lands inside the captured text, which strings.Fields then splits into packages.

Verified by running this file's own parsing logic against a mutated Dockerfile — one that installs neither tool and is still valid and functional:

RUN microdnf install -y --nodocs bash coreutils-single findutils file # dropped git python3 for size
RUN microdnf clean all

parsed install list: [bash coreutils-single findutils file # dropped git python3 for size]
  bash     found -> PASSES
  base64   found -> PASSES
  file     found -> PASSES
  git      found -> PASSES      <- not installed
  python3  found -> PASSES      <- not installed

All six capabilities report as covered on an image missing two of them. Same shape as the finding this commit fixes, one level down: source's doc says "Neither is satisfiable by a comment", and for this form it still is.

Narrower than the original hole, to be fair — that one needed no edit at all, since the existing prose blocks already satisfied it. This one requires someone to add a trailing comment on that specific line. But it is a natural edit precisely when packages are being removed, which is the change the test exists to catch.

Worth noting why the twelve negative cases did not surface it: every one of them deletes something. A mutation harness that only subtracts cannot find a hole that adding text opens — which is also the exact shape of how the original defect got in (the commit that added the explanatory comments is what opened it).

Fix is one line here — cut at " #" before strings.Fields, or extend the capture group to stop at #.

[cwiklik]

suggestion — this comparison is order-sensitive, which is the finding you just fixed one test up.

flags[name] is the captured flags joined in source order, so two install -d instructions that create byte-identical directories compare unequal:

Dockerfile:         RUN install -d -m 775 -o 1001 -g 0 /workspace   -> "-m 775 -o 1001 -g 0"
Dockerfile.runtime: RUN install -d -o 1001 -g 0 -m 775 /workspace   -> "-o 1001 -g 0 -m 775"

Verified with this file's own regex: the first form captures -m 775 -o 1001 -g 0, so a != b fires and the test reports "different owner/group/mode" on two files whose /workspace is identical in owner, group and mode.

The failure message is the worse half, exactly as in TestBothDockerfilesInstallTheSamePackages: it prints two install -d lines that a reader will scan as equivalent, with nothing pointing at ordering. install itself does not care about flag order, so nothing outside this test does.

Same fix as the one applied to the package list — sort the captured fields before joining:

f := strings.Fields(m[1])
slices.Sort(f)
flags[name] = strings.Join(f, " ")

(Sorting ["-o","1001","-g","0","-m","775"] as flat fields would scramble flag/value pairs — worth pairing them up first, or capturing the three values individually.)