Add Docker validation lab - #1179
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
✅ Workflow security review — no issuesScanned 1 workflow file(s) with zizmor, pinact, OSV/GHSA, Claude, in full. |
burtenshaw
added
feature
size: extra-large
Cursor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alignment Review Report
Scope: PR diff 5a0b6bd…e79560c — the L2-02 "Docker validation lab" slice, stacked on the L2-01 contracts PR. Reviewed against .claude/docs/PRINCIPLES.md, .claude/docs/INVARIANTS.md, and the open RFCs. manifest.py, runtime/contracts.py, and the JSON schemas are out of scope here — they belong to the base (contracts) PR.
Automated Checks
- Lint: PASS (for this PR). Every file this PR changes passes
ruff format --check,ruff check, andusort check(pinned ruff 0.16.7 / usort 1.1.3). I also ranruffoverscripts/validation/*.py(whichlint.shdoes not cover) — clean.bash .claude/hooks/lint.shreports "57 files would be reformatted", but all 57 are pre-existing drift underenvs/**and**/README.md— none are touched by this PR. This is base-branch/tooling drift, not introduced here.
- Debug code: CLEAN.
check-debug.shsurfaces no new prints/TODOs/breakpoints in this PR'ssrc/files (the listed prints are all pre-existing modules;scripts/validation/reproduce.pyprints are legitimate CLI output andcheck-debug.shonly scopessrc/). - Tests: PASS. 47 PR unit tests green (
test_docker_provider,test_reproduction,test_protocols,test_served_probe). The real-Docker suite is correctly gated behind@pytest.mark.dockerand skips without an engine.
Open RFCs Context
- RFC 008 — Environment Auto-Validation (In Review, @zkwentz). This PR implements the "Level 2 execution amendment" (Docker-local reproducible-supervision slice). Directly relevant.
- RFC 000–005 (In Review; @Darktex et al.) are foundational; RFC 010/011 (Draft; @thegovind) are unrelated to validation.
Tier 1: Fixes Required
- None. No mechanical bugs, missing imports, type errors, or security issues found. The provider is unusually defensive: bounded stdout/stderr draining, process-group SIGKILL on timeout, symlink-race-safe source snapshotting (
O_NOFOLLOW, descriptor-relative opens), and thorough credential redaction.
Tier 2: Alignment Discussion
Principle Conflicts
None identified. The change actively reinforces two invariants worth calling out:
- Container isolation (INVARIANTS.md → "Container isolation"): subjects run non-root (
65532), read-only rootfs,--cap-drop ALL,no-new-privileges, pids/memory/swap ceilings, loopback-only publish (127.0.0.1::8000), and image-declaredbind/volumemounts are rejected before start (docker.py~L274–318). No host mounts, docker socket, or host namespaces. - No credential exposure (INVARIANTS.md → "No credential exposure"): all provider output is redacted (
_safe_text/_TOKEN,docker.pyL47–65),inspect()deliberately omits container env/command/host paths (L386–416), host env is never forwarded (LaunchSpec.env_varsonly; the lab also popsPYTHONPATH), and CI setspersist-credentials: false+permissions: contents: read.
(Also consistent with the dual-API boundary: the collector drives reset/step/state over /ws as orchestration, agent_boundary: api — not agent-facing — so "agents cannot reset" is not implicated.)
RFC Conflicts
ALIGNMENT FLAG: Implements an amendment to an RFC that is still In Review
- Principle/RFC at stake: RFC 008 (Status: In Review). The "Level 2 execution amendment" being implemented (manifest schema 2
validation.execution, the public probe sidecar,LaunchSpec, the provider protocol, the reproducibility lab) was added to RFC 008 by the PR author (@burtenshaw), on an RFC authored by @zkwentz. - The concern: This is not a technical conflict — the code tracks the amendment closely (public-only networking with named
UnsupportedCapabilityrefusals forno-network/allowlist/GPU;disk_mbsplit across/tmp+/dev/shm; idempotent owner-verified cleanup; the README/RFC caveat that one successful build ≠static.reproducible_build). The flag is process: an In-Review RFC whose amendment is written by the implementer should get explicit sign-off that the amendment reflects consensus before/with merging its implementation. Per the review guidance, flagging this focuses the design discussion — the code may be right and the RFC simply needs to move to Accepted. - Suggested reviewer: @zkwentz (RFC 008 author), @Darktex (owner of the container-isolation & credential invariants this slice leans on)
Minor (non-blocking) — provider-contract churn inside the stack
- The
ValidationProviderProtocol'sstart()is rewritten here from the L2-01 shapestart(image_ref, *, network, env_vars)tostart(spec: LaunchSpec), andbuild/supported_network_modes/inspect/logsare added (providers/__init__.py). SinceLaunchSpecalready lands in L2-01 (runtime/contracts.py), reviewers may prefer the contracts PR define the finalstart(spec)shape so the very next PR in the stack doesn't immediately supersede it. No released API is affected (the whole stack is unmerged), so this is purely a stacking-hygiene call. - Suggested reviewer: @zkwentz
Summary
- 0 mechanical issues to fix (Tier 1)
- 1 alignment point for human review (RFC 008 amendment consensus), +1 minor stacked-interface nit
- 0 hard RFC conflicts — the implementation aligns with the RFC 008 Level 2 amendment
Sent by Cursor Automation: Pre-review
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
f56bc0c to
e860f5a
Compare
There was a problem hiding this comment.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Wheel download omits newer manylinux tags
- Added the supported manylinux_2_34 target so pip selects hash-pinned native Linux wheels such as caio instead of generic fallbacks.
Or push these changes by commenting:
@cursor push ca2759bf2a
Preview (ca2759bf2a)
diff --git a/scripts/validation/reproduce.py b/scripts/validation/reproduce.py
--- a/scripts/validation/reproduce.py
+++ b/scripts/validation/reproduce.py
@@ -228,6 +228,8 @@
"--implementation",
"cp",
"--platform",
+ f"manylinux_2_34_{arch}",
+ "--platform",
f"manylinux_2_28_{arch}",
"--platform",
f"manylinux2014_{arch}",You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
REQUEST_CHANGES at 60a96a68
New commit correctly strips uv’s wheel/.gitignore before checksumming (including failed builds). That does not address the open Medium: pip download still targets only manylinux_2_28, manylinux2014, and linux, so newer compatible tags (e.g. manylinux_2_34) can still yield an incomplete wheelhouse on a capable 3.12 slim image.
Expand the platform list from actual runtime compatibility, or download inside the target image, and add a native-wheel regression. Holding approval until that lands (exact-head CI still running).
Sent by Cursor Automation: Release
60a96a6 to
ff5c113
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Secret filter excludes source modules
- Restricted secret-name filtering to extensionless credential names so dotted source modules remain in Docker snapshots.
- ✅ Fixed: Diff checksum uses sanitized output
- The diff checksum now hashes the command's complete raw stdout through a temporary file without redaction, truncation, decoding, or stripping.
You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
REQUEST_CHANGES at ff5c113e (Bugbot refresh after #1178 rebase)
Still open (Medium): incomplete manylinux platform set for pip download.
New Medium: _SECRET_NAME excludes source modules named secret.py / credentials.py from the build snapshot.
Low (Bugbot): diff_sha256 hashes sanitized git diff output — integrity of the lab evidence can drift from the true diff. Prefer hashing raw bytes before redaction (or hash outside the secret filter).
Bugbot autofix is active. Not Thursday 0.6.0 cargo.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
REQUEST_CHANGES at 30460b0e (autofix progress)
Fixed this head:
_SECRET_NAMEno longer matchessecret.py/credentials.py(extension-bearing source kept; baresecret/secrets/credentialsstill excluded) — with regression.diff_sha256now hashes complete unsanitizedgit diffbytes viacommand_digest— with regression.
Still open (Medium): incomplete manylinux platform set for wheel download.
Holding approval for that. Exact-head CI in flight. Not Thursday 0.6.0 cargo.
Sent by Cursor Automation: Release
|
Addressed 17 review threads, including the follow-up credential-filter findings; all 20 threads on this PR are now resolved. Final head The fixes derive wheel compatibility from the pinned target image, preserve timeout causes during failed cleanup, and match the importer's complete credential-filename policy while retaining bare source modules such as The earlier stack-interface nit is obsolete now that #1178 is merged. RFC 008 remains In Review; amendment consensus is still a maintainer decision. |
There was a problem hiding this comment.
APPROVE @ cd28610f
Prior Medium manylinux gap is fixed: download_linux_wheels now pulls the target base image and asks host pip download for that image’s platform_tags() (so newer tags like manylinux_2_34_* are included). Parametrized native-wheel regressions cover amd64/arm64 × 2.31/2.34/2.39 success and 2.40 fail-closed.
Also verified on this head:
_commandreturn annotation/tuple explicit; ProcessLookupError handlers documented- exec-timeout cleanup preserves the original timeout when stop verification fails (new regression)
- prior secret-name + exact
diff_sha256fixes still present
Local: 36 passed on test_docker_provider.py + test_reproduction.py.
Nits (non-blocking): CodeQL implicit string concat in the python -c probe; snapshot still won’t exclude credentials.json / secrets.yaml by basename (only bare credentials/secrets and .pem/.key).
Not Thursday 0.6.0 cargo until merged; if it lands before the cut, refresh #1211 / TestPyPI.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
APPROVE @ dfb463a2
Reconfirm after test-only follow-up: test_wheel_download_uses_target_image_compatibility now importorskip("pip") so hosts without the runtime-lab pip skip cleanly. Manylinux/platform_tags() fix from cd28610f unchanged.
Local: 8/8 compatibility regressions pass with pip present.
Not Thursday 0.6.0 cargo until merged.
Sent by Cursor Automation: Release
|
Addressed the credential-filename nit in 7656c56: the snapshot again excludes |
There was a problem hiding this comment.
APPROVE at 7b383985
7656c565 closes the residual credential-filename nit: _SECRET_NAME now excludes credentials.json and secrets.{json,toml,yaml,yml} (case-insensitive) while still keeping secret.py / credentials.py. Parametrized snapshot regression covers both cases.
Also synced onto main after #1219. Prior manylinux/platform_tags() fix unchanged.
Local: 37 passed on docker provider + reproduction suites.
Not Thursday 0.6.0 cargo until merge; if it lands, refresh #1211 / TestPyPI.
Sent by Cursor Automation: Release
f6371c0 to
37c7e44
Compare
There was a problem hiding this comment.
Saw the rebase onto main 43aa7528 at 37c7e44f. Feature content still carries the approved credential snapshot filter (_SECRET_NAME + regressions). Exact-head CI is re-running — will re-APPROVE once green.
Not Thursday 0.6.0 package cargo unless you explicitly want L2 lab in the cut.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Docker parses mixed stderr as data
- Docker probe calls now drain stderr separately so architecture and JSON parsing receives only stdout.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 37c7e44. Configure here.
There was a problem hiding this comment.
Re-APPROVE at 37c7e44f (rebase onto main 43aa7528)
Exact-head repository CI green, including Runtime validation / Linux Docker. Feature content for the Docker lab / credential snapshot filter matches the prior APPROVE at f6371c00 (_SECRET_NAME + regressions intact).
Bugbot medium (non-blocking): scripts/validation/reproduce.py run() still merges stderr into stdout for docker info / platform-tag JSON parses. Same helper was present on the previously approved tip; pull is already split out to avoid progress contamination. Worth a follow-up hygiene fix, not a merge gate — Autofix is in flight if you want it landed first.
Not Thursday 0.6.0 package cargo unless you explicitly fold L2 lab into the cut. Stacked #1181 still needs a conflict-free refresh onto this tip.
Sent by Cursor Automation: Release
6e36285 to
1c4932f
Compare
There was a problem hiding this comment.
Tip advanced to e1e3ccf9 (fix: isolate command stdout) — always separates stderr, returns stdout only, with regression coverage for warning contamination + secret redaction. Better than the Autofix flag. Watching exact-head CI before re-APPROVE.
Note: stacked #1181 is currently based on 1c4932f2 (one commit behind this tip).
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Re-APPROVE at e1e3ccf9
Exact-head CI green (incl. Runtime validation / Linux Docker + Bugbot). run() now always isolates stderr and returns stdout only, with solid regressions for warning contamination, redaction, oversized dual-stream drain, failures, and timeouts.
Credential snapshot filter (_SECRET_NAME) still intact from earlier tips.
Non-blocking: CodeQL flags intentional adjacent string literals in the new -c test scripts — style only; no behavior risk.
Not Thursday 0.6.0 package cargo unless you fold L2 lab into the cut. Stacked #1181 still wants a base refresh onto this tip (e1e3ccf9 vs its 1c4932f2).
Sent by Cursor Automation: Release
L2 Docker lab is package cargo for 0.6.0 after Ben merged #1179. Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
2 participants
This PR adds the Docker validation lifecycle and a pinned reproduction lab with shared fixtures and retained CI evidence. Builds on #1178; second of three stacked PRs tracked in #1177.