fix: omit core metadata when reusing W&B sandboxes - #1930
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 6:45 PM ET / 22:45 UTC. ClawSweeper reviewWhat this changesAllow W&B sandbox reuse to omit Crabbox-owned run metadata while preserving user-environment restrictions, with regression coverage, documentation, and a changelog entry. Merge readiness✅ Ready for maintainer review The fix remains necessary on the inspected main revision, and no actionable patch defect remains. The explicit maintainer-owned changelog disposition resolves the previous review finding. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s CLI injects run metadata before sending requests to the W&B provider adapter. For existing sandboxes, the adapter validates environment compatibility and ownership before executing commands through the provider’s gRPC client. flowchart TD
A[CLI command and selected environment] --> B[Inject core run metadata]
B --> C[W&B reuse environment guard]
C -->|Unsupported user variables| D[Reject request]
C -->|Metadata may be omitted| E[Verify sandbox ownership]
E --> F[Execute command through gRPC]
F --> G[Return outcome and retain sandbox]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep environment omission inside the W&B adapter, using the shared exact-name predicate while retaining ownership enforcement and explicit user-variable rejection. Do we have a high-confidence way to reproduce the issue? Yes, from source: ordinary W&B run --id requests receive reserved metadata that main’s guard rejects before ownership lookup. This review did not execute the failure. Is this the best way to solve the issue? Yes. Reusing the existing exact-name predicate is a focused repair that preserves the provider’s inability to forward environment values during Exec. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 69e79538dae2. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles) |
|
Maintainer disposition for the review of 9f0b2f2: the changelog finding and removal step are not accepted. This is a maintainer-owned repair, and the repository requires maintainers/agents to record user-visible fixes under Unreleased. The entry remains with its PR link. The full executed fixture and complete CLI receipt were inspected, including exact source/fixture hashes and raw-to-displayed capture equality (only the ephemeral port and claim revision are substituted). The unchanged complete capture is now first in the body, so its acquire, eight reuse controls, exact Exec payloads, actual Stop and cleanup result all fit in the initial description prefix. No source, tests, fixture or observed outcomes changed for presentation. The accepted proof scope remains explicit: the actual built CLI and production constructor used the existing loopback-only plaintext mode with synthetic gRPC responses. It establishes metadata gating, unchanged Exec payload shape and normal claim/Stop routing; it does not establish hosted W&B, TLS or real remote workload execution. Independent managed source/link reviews passed at P0. The bot's existing BLOCKED verdict is preserved rather than relabelled as approval; ordinary required CI and merge protections still apply. |
|
Landed in 5344e73 through the normal protected merge. All five required workflows passed on reviewed head 9f0b2f2. The telemetry change in #1931 landed between preflight and merge. The actual merged tree was checked against its actual parent plus the reviewed head, then the exact merged CLI was rebuilt and replayed through the unchanged published fixture. Acquire, three allowed reuse cases, five zero-RPC rejection cases, and actual Stop cleanup all passed; all four Exec payloads remained metadata-free. All 2,228 source hashes were verified after replay. Post-merge |
Ordinary W&B
run --idcalls were rejected before ownership lookup or execution because the CLI always injectsCRABBOX_LEASE_ID,CRABBOX_RUN_ID, andCRABBOX_SLUG, while the existing-sandbox env guard accepted only implicitCI/NODE_OPTIONSdefaults.The private
wandbExistingIDEnvCanBeOmittedhelper uses the existingcore.IsRunExecutionMetadataEnvNamepredicate to omit those exact core-owned metadata names before applying the existing env-summary/default-env rules. Metadata-only requests can proceed even when an env summary was requested. Explicitly selectedCI,NODE_OPTIONS, custom variables and similarly named nonreserved variables remain rejected. The old implicit uppercaseCI/NODE_OPTIONSomission exception remains intact.This does not add env support to the CoreWeave
ExecRPC, forward metadata through command text, or refresh the sandbox's original Start-time environment. The protobuf has no Exec env field. Existing claim/tag authorization, Start/Stop lifecycle, timing finalization, transport/TLS and auth policy are unchanged. The documentation explains that distinction; the small Unreleased entry follows the larger features and existing W&B lifecycle entry without a placeholder PR link.Complete captured CLI behavior
This unchanged complete capture is placed first so a shortened PR description still includes all reuse, rejection and cleanup evidence. The fixture and method details follow.
Acquire, all eight reuse controls, exact Exec payloads, claim and Stop cleanup
{ "acquire": { "exitCode": 0, "output": "provisioning provider=wandb image=ubuntu:24.04 max_lifetime=1800s\nprovisioned sandbox=sb-metadata-audit status=running\nsynthetic-rpc-proof\n" }, "cases": [ { "connectionsAfter": 2, "connectionsBefore": 1, "name": "metadata-only", "newRPC": [ "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec" ], "run": { "exitCode": 0, "output": "synthetic-rpc-proof\n" } }, { "connectionsAfter": 3, "connectionsBefore": 2, "name": "metadata-only-summary", "newRPC": [ "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec" ], "run": { "exitCode": 0, "output": "synthetic-rpc-proof\n" } }, { "connectionsAfter": 4, "connectionsBefore": 3, "name": "implicit-defaults", "newRPC": [ "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec" ], "run": { "exitCode": 0, "output": "synthetic-rpc-proof\n" } }, { "connectionsAfter": 4, "connectionsBefore": 4, "name": "explicit-ci", "newRPC": [], "run": { "exitCode": 2, "output": "provider=wandb cannot forward env vars to an existing sandbox (--id); rerun without --id or omit --allow-env\n" } }, { "connectionsAfter": 4, "connectionsBefore": 4, "name": "explicit-node", "newRPC": [], "run": { "exitCode": 2, "output": "provider=wandb cannot forward env vars to an existing sandbox (--id); rerun without --id or omit --allow-env\n" } }, { "connectionsAfter": 4, "connectionsBefore": 4, "name": "explicit-custom", "newRPC": [], "run": { "exitCode": 2, "output": "provider=wandb cannot forward env vars to an existing sandbox (--id); rerun without --id or omit --allow-env\n" } }, { "connectionsAfter": 4, "connectionsBefore": 4, "name": "prefix-lookalike", "newRPC": [], "run": { "exitCode": 2, "output": "provider=wandb cannot forward env vars to an existing sandbox (--id); rerun without --id or omit --allow-env\n" } }, { "connectionsAfter": 4, "connectionsBefore": 4, "name": "suffix-lookalike", "newRPC": [], "run": { "exitCode": 2, "output": "provider=wandb cannot forward env vars to an existing sandbox (--id); rerun without --id or omit --allow-env\n" } } ], "claimBeforeStop": { "claimedAt": "2026-09-06T21:58:10Z", "cloudID": "sb-metadata-audit", "idleTimeoutSeconds": 1800, "lastUsedAt": "2026-09-06T21:58:10Z", "leaseID": "sb-metadata-audit", "provider": "wandb", "providerScope": "endpoint:127.0.0.1%3A43001|entity:synthetic-entity|project:synthetic-project", "repoRoot": "", "revision": "<synthetic-claim-revision>", "slug": "sb-metadata-audit" }, "execRequests": [ { "sandboxId": "sb-metadata-audit", "command": [ "true" ] }, { "sandboxId": "sb-metadata-audit", "command": [ "true" ] }, { "sandboxId": "sb-metadata-audit", "command": [ "true" ] }, { "sandboxId": "sb-metadata-audit", "command": [ "true" ] } ], "finalClaimCount": 0, "finalRPC": [ "/coreweave.sandbox.v1beta2.GatewayService/Start", "/coreweave.sandbox.v1beta2.GatewayService/Get", "/coreweave.sandbox.v1beta2.GatewayService/Exec", "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec", "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec", "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Exec", "/coreweave.sandbox.v1beta2.GatewayService/List", "/coreweave.sandbox.v1beta2.GatewayService/Stop" ], "finalSandboxPresent": false, "initialRPC": [ "/coreweave.sandbox.v1beta2.GatewayService/Start", "/coreweave.sandbox.v1beta2.GatewayService/Get", "/coreweave.sandbox.v1beta2.GatewayService/Exec" ], "portClosed": true, "scope": "actual unmodified CLI+production constructor using existing loopback plaintext mode; gRPC responses are synthetic, no TLS or hosted/native workload claim", "serverStopped": true, "startMetadata": { "CRABBOX_LEASE_ID": "", "CRABBOX_RUN_ID": "run_4e80eace5994", "CRABBOX_SLUG": "" }, "stop": { "exitCode": 0, "output": "" } }Tests-first evidence
Against base
69e79538dae281148ed3475329bfc6eeaf53b58f, the new 12-case helper table failed its four metadata-positive cases before the fix. Its eight existing default/rejection controls passed. The updated seeded existing-ID backend test also failed before the fix, then passed with metadata included in its request.The table covers metadata-only, metadata-only plus summary, owner-defined case folding, implicit defaults, metadata plus implicit defaults, explicit CI/NODE_OPTIONS, custom env, prefix/suffix/padded lookalikes, and lowercase
ciremaining outside the old default exception. No broadCRABBOX_*prefix exemption was added.An earlier preserved actual-CLI audit on
4e3df6dfc42a49bd665a92b2e2756b909122c8c3acquired a real local ownership claim through its protocol fixture, then reproduced this rejection on the same sandbox:That earlier reuse exited 2, opened no new connection, and left the RPC trace unchanged at Start/Get/Exec; explicit CLI Stop then cleaned up. It is historical negative evidence, not the current candidate's receipt.
Fresh candidate checks:
Actual race output:
Fresh focused vet and diff checks passed. The canonical core predicate and 246-file documentation checks passed before the private-symbol rename; those files remain byte-identical. The complete revised four-file candidate received a fresh managed Codex review with no actionable P0 findings. That scoped threshold is not a claim of lower-priority review coverage.
Actual built CLI and production constructor
The byte-identical fixture was replayed against a freshly built candidate after the private-helper rename; this receipt and binary are separate from the original proof. The fixture invokes the built candidate CLI by absolute path and uses its unchanged production gRPC constructor with the existing, explicitly supported plaintext loopback-only endpoint mode. The auth headers and responses are synthetic. No TLS/trust setting or production transport was changed. This is CLI/client protocol proof, not TLS, hosted W&B, or native sandbox-workload execution proof; the returned
synthetic-rpc-prooftext is a fixture response.The fixture first acquires and keeps a sandbox, creating the normal exact local claim. It then runs these cases against that owned ID:
CRABBOX_RUN_ID_EXTRAlookalikePREFIX_CRABBOX_RUN_IDlookalikeEach successful reuse adds only List/Exec: no Acquire/Start and no automatic Stop. Each rejected case is checked for both zero new RPCs and zero new accepted connections. Finally, the actual CLI Stop adds List/Stop and removes the sandbox/claim. The fixture verifies the server exits and its port is closed; temporary state is owned by Go's
t.TempDircleanup.The actual Start request contained the three injected metadata names, while all four captured Exec requests (initial command plus three successful reuses) were exactly:
{"sandboxId":"sb-metadata-audit","command":["true"]}No metadata, CI/NODE_OPTIONS, environment field or shell wrapper was added to Exec. The fixture had one compile-time assertion correction while being written: protobuf
commandis repeated, so it is compared as a one-element slice. That failed compile is preserved separately and is not provider or runtime evidence.Capture display substitutions
Only the ephemeral endpoint port in the claim scope and its generated revision are substituted in the complete capture above. Actual exit codes, outputs, RPCs, connection counts and metadata fields are retained. Raw proof remains unchanged and is the input to the receipt hash.
Exact tested inputs
Base:
69e79538dae281148ed3475329bfc6eeaf53b58fplus this four-file diff. These hashes identify the actual tested source/binary/fixture, not a later merge commit. Original negative-audit artifacts remain unchanged.Runnable fixture
Save the exact Go fixture below outside the repository. From the candidate checkout, build the uninstrumented CLI and add the test file through a Go overlay. Requires Go and Git on a POSIX host. Only synthetic credentials are passed to the child CLI; the fixture creates its own local state and server.
The
CLI_METADATA_RECEIPTline contains the complete capture. Archive the binary/receipt before removing the owned proof directory.Full fixture_test.go (byte-exact fixture matching the hash above)