feat(snapshot): add workspace snapshots - #848
Merged
Merged
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds registry-backed workspace snapshots. It adds snapshot creation, listing, restoration, deletion, export/import support, OCI storage, volume streaming, workspace integration, end-to-end coverage, CI setup, and documentation. ChangesWorkspace snapshots
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
skevetter
force-pushed
the
feat/container-versioning-v2
branch
4 times, most recently
from
August 1, 2026 20:46
e104839 to
f810ee7
Compare
Snapshots a workspace's committed container filesystem and bind-mounted volumes into an OCI registry, and restores or transfers them into a new workspace (optionally on a different provider) via `devsy snapshot create/list/restore/delete` and `devsy up --from-snapshot`. Fixes applied during review: snapshot create now rejects multi-mount workspaces up front so a snapshot can never be created that restore can't consume; restore derives its tar strip-depth from a recorded create-time mount prefix annotation instead of guessing from the restore-side target's path depth; the host.docker.internal insecure-registry override needed only by the snapshot e2e fixture is scoped to pkg/snapshot instead of leaking into pkg/image's general-purpose, security-sensitive reference parsing; --from-snapshot is rejected in platform mode to match snapshot create's local-only constraint; `devsy describe` renders snapshot-sourced workspaces instead of an empty source line; CommitContainer moved off the shared ImageDriver interface onto a narrow SnapshotCapableDriver capability interface so drivers that can't commit a container don't have to stub it; and WorkspaceSource.String()/Type() now share one field-precedence method so they can't independently disagree on a source's kind.
skevetter
force-pushed
the
feat/container-versioning-v2
branch
from
August 1, 2026 20:50
f810ee7 to
d8ff273
Compare
Adds a Workspace Snapshots page under Developing in a Workspace covering create/list/restore/delete, the up --from-snapshot shortcut, provider transfer, export/import carry-through, and current limitations. Also fixes two small gaps found while writing the docs: the missing-registry error told users to run a command that doesn't exist (devsy context set-options instead of devsy context set -o), and committed snapshot images carried no identifying marker for anyone inspecting them outside devsy tooling, so CommitContainer now labels them sh.devsy.snapshot=true.
The snapshot e2e suite's cat/pwd/create/restore commands never passed
--debug, so the current CI failure ("SSH command failed with exit code
1") carries no detail about what actually failed on the remote side —
every Infof/Debugf in RestoreVolumes/syncMounts was silently dropped at
the default Error-only verbosity. Also surface the failing command's
stderr in execWithSSHRetry's final error instead of discarding it.
…empty
The restored workspace's marker.txt is missing in CI ("no such file or
directory"), and the log shows RestoreVolumes never ran: syncMounts saw
the mount target already non-empty and skipped the restore. Log which
entries are already there so the next CI run reveals what's populating
the directory before RestoreVolumes gets a chance to run.
…pace content RestoreVolumes never ran on any snapshot restore: syncMounts' "is the mount target already populated" guard counted the .devcontainer.devsy.json devsy synthesizes for image-sourced workspaces (written into the mount target before syncMounts runs) as pre-existing content, so it always skipped straight past the real restore. cat marker.txt then failed with "no such file or directory" because the volumes blob was never extracted.
…tore Restoring a snapshot pins DevContainerSource to the committed "-fs" image, which bypasses the project devcontainer.json entirely -- so runArgs it set (e.g. --add-host=host.docker.internal:host-gateway, needed to reach a registry only resolvable by that hostname) were silently dropped, breaking snapshot pull/restore from inside the restored container. snapshot create now captures the resolved devcontainer.json's runArgs into a new manifest annotation; both `devsy snapshot restore` and `devsy up --from-snapshot` read it back and pass it through to the image-sourced devcontainer config that runs the restored container.
- pkg/snapshot/manifest_test.go: hoist the repeated "uid-123" literal into a testWorkspaceUID constant (goconst, now used 4x). - pkg/provider/workspace.go: move the SSHAuthSockID doc comment above the field instead of trailing it, so gofmt's struct-tag realignment around the new RunArgs field doesn't push it over 120 chars (lll).
… runArgs fidelity Adds four e2e specs closing the highest-risk gaps in the snapshot suite: (1) snapshot create rejects a workspace with more than one bind mount, without leaving the workspace broken; (2) restoring the same snapshot ref into an already-populated workspace id is a no-op that doesn't clobber content added after the first restore -- the exact scenario the skipSnapshotRestore synthesized-devcontainer.json bug lived in; (3) devsy up --from-snapshot --reset forces a real restore over a non-empty target, wiping stale content the snapshot doesn't know about; (4) restore replays a devcontainer.json runArg that isn't --add-host, proving the runArgs plumbing generalizes rather than only working for the one flag the test harness itself depends on.
…ry env loss RunArgs replay alone wasn't enough: the e2e suite's own opt-in (DEVSY_INSECURE_DOCKER_INTERNAL=true, set via devcontainer.json containerEnv) is also dropped when restore pins DevContainerSource to the committed image, so RestoreVolumes' manifest pull inside the restored container tried HTTPS against a plain-HTTP registry: "server gave HTTP response to HTTPS client". Generalizes the same fix already applied to runArgs: snapshot create now also captures the resolved devcontainer.json's containerEnv into the manifest, and restore/--from-snapshot replay it. This is a real gap beyond the test too -- any devcontainer.json containerEnv (API keys, feature flags) was previously silently dropped on restore. Also fixes two new cyclop violations these changes introduced (BuildManifest, validateFromSnapshot) via straightforward extractions, and dedupes a "image:python" literal in config_test.go against the already-existing testImgSrc constant.
…nv test goconst flagged the fourth occurrence of "true" against the package's already-defined stringTrue constant (pkg/devcontainer/setup.go).
…resh id resolveWorkspace only takes its create-new-workspace path when args is non-empty (pkg/workspace/workspace.go), but --from-snapshot explicitly forbids a positional source, so cobra always calls prepareClient with an empty args slice. The result: `devsy up --from-snapshot <ref>` -- exactly the form documented and the form `devsy snapshot restore` mirrors -- could only ever resolve to an *existing* workspace, and failed with "workspace <id> doesn't exist" the moment it needed to create one, which is the entire point of a restore. `devsy snapshot restore`'s own path (up.RunFromOptions) happened to dodge this because it synthesizes args from opts.Source before calling prepareClient. This carries that same synthesis into the cobra-invoked path via a new ensureArgsForFromSnapshot helper: DesiredID and Source (both already set by resolveExplicitSource) take priority over the synthesized value, so it's never actually read for anything but satisfying the len(args) > 0 check.
skevetter
force-pushed
the
feat/container-versioning-v2
branch
from
August 2, 2026 05:01
3fd4f7d to
6bcd179
Compare
The --reset test failed twice, identically, with "fork/exec /usr/bin/bash: permission denied" on the very first SSH command against a container that had just been created via restore -- despite setup logs showing the container running fine moments earlier (an su -c invocation into the same container succeeded right before). This is a docker-commit/overlay2 settling race, not a real permission problem: every other snapshot test happens to have a few hundred ms of buffer before its first SSH call from intervening setup steps, which this test's tighter timing exposed. execWithSSHRetry already retries connection-level transients; extend it to cover this container-startup race too.
BeforeEach failed all 7 snapshot specs with "start local registry: exit status 125" ~2.4s after the CI workflow's `systemctl restart docker` + `docker info` readiness check passed. docker info reporting the daemon reachable doesn't guarantee its container-creation subsystem has finished warming up, so the very first `docker run` in a job can transiently fail right after a daemon restart. Retry a few times with a short backoff, and capture stderr so a real failure is actually diagnosable instead of just "exit status 125".
Root cause of the --reset test's "fork/exec /usr/bin/bash: permission
denied" failure, confirmed via a minimal reproduction: an unreadable
cmd.Dir makes Go's exec.Cmd.Start() report exactly this error,
misattributing it to the executable rather than the actual cause.
chownWorkspace's marker file lives at /var/devsy/chownWorkspace.marker
-- outside any workspace mount, so it survives docker commit and gets
baked into every snapshot's "-fs" image. On restore, that marker is
already present (written when the *original* workspace was set up),
so chownWorkspace's markerFileExists("chownWorkspace", "") check wrongly
treats the restored container as already chowned and skips it entirely
-- even though RestoreVolumes just freshly extracted the mount's
content as root, moments earlier, into a workspace with a different
ID and path. The remote user is then left unable to even chdir into
its own workspace folder.
Fix: key the marker on DEVSY_WORKSPACE_ID (already injected via
docker run -e for every container) instead of using empty/
content-agnostic marker content, so a restore under a different
workspace ID naturally invalidates the marker it inherited from the
original container's committed image.
extract.Extract only ever writes/overwrites entries present in the volumes tar; it never deletes anything already at the target that the tar doesn't mention. So --reset forcing RestoreVolumes to run again was not enough to give --reset its expected meaning: content added to a workspace after its first restore (not part of the snapshot at all) silently survived a --reset restore instead of being wiped, as confirmed by the e2e --reset test's stale.txt assertion. RestoreVolumes now takes a reset bool; when true, it clears the mount target's existing contents before extracting, so the workspace ends up in exactly the snapshot's state rather than the snapshot's state merged with whatever was already there.
- retry.go: the SSH permission-denied retry's comment attributed the failure to an overlay2/docker-commit settling race; the actual root cause (chownWorkspace's stale marker) was found and fixed separately. Update the comment to reflect that and note the retry is now defensive-only. - registry_test.go: remove a comment's false citation of a non-existent e2e test file. - tunnelserver_test.go: drop a hedged "(if any)" comment referencing tests that don't exist.
Verified findings from a CodeRabbit CLI review: - reference.go: referenceOptions checked os.Getenv(...) != "" instead of parsing the value, so DEVSY_INSECURE_DOCKER_INTERNAL=false (or any non-empty string) silently enabled the insecure-registry override. Use strconv.ParseBool, matching the convention already used for EnvInsecureTLS elsewhere. Strengthened the existing tests to assert concrete schemes and added coverage for the "false" case. - manifest.go: BuildManifest hardcoded the container image layer's media type to the Docker v2 manifest format, discarding the actual media type create.go already reads back from the registry after pushing (desc.MediaType) but never passed through. Added BuildManifestOptions.ContainerImageMediaType (falling back to the Docker v2 type when unset, e.g. in existing tests), and threaded the real value from cmd/snapshot/create.go's pushedImageDigestAndSize. Two other CodeRabbit findings were investigated and not actioned: adding strict layer-count/media-type validation to ParseManifest would encode a positional (Layers[0]/[1]) assumption the actual consumer (resolveVolumesLayer) deliberately avoids by matching on media type instead of index; and the suggested --from-snapshot vs. explicit RunArgs/ContainerEnv conflict check is unreachable, since no CLI flag exposes cmd.RunArgs/ContainerEnv for a user to set explicitly.
…nerEnv Verified finding from a second CodeRabbit CLI review: snapshot create captured result.MergedConfig.ContainerEnv verbatim into the manifest's sh.devsy.snapshot.container-env annotation for replay on restore. injectDaemonEntrypoint (pkg/devcontainer/single.go) writes the platform access key (base64-encoded, not encrypted) into that same ContainerEnv map under EnvWorkspaceDaemonConfig for platform-managed workspaces, so a snapshot of such a workspace would have carried that key into a potentially shared registry artifact. redactedContainerEnv drops EnvWorkspaceDaemonConfig before it's captured; the restored container still gets a fresh copy of this value at container-start time independent of any snapshot, so this doesn't change restore's actual behavior. Three other findings from this review round were investigated and not actioned: stricter ParseManifest layer/media-type validation (same positional-coupling concern as the previous round, still not a crash risk given resolveVolumesLayer matches by media type); negative-size validation in BuildManifest (both ContainerImageSize and VolumesSize are always library-computed byte counts from go-containerregistry/ io.Copy, never user input, so a negative value is unreachable); and hardening snapshot-ref whitespace parsing in the e2e test (stdout for these commands is never merged with stderr/debug output, so the captured ref is always exactly one clean line).
skevetter
marked this pull request as ready for review
August 2, 2026 19:09
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
e2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.json (1)
5-5: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider a narrower second mount.
The fixture only needs a second mount so
snapshot createrejects the workspace. Bind-mounting the host/tmpread-write gives the container write access to the runner's shared temp directory. A read-only mount limits that access without changing the mount count.♻️ Proposed narrower mount
- "mounts": ["source=/tmp,target=/extra,type=bind"], + "mounts": ["source=/tmp,target=/extra,type=bind,readonly"],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.json` at line 5, Update the second mount in the devcontainer fixture to be read-only while preserving its existing source, target, and bind-mount type. Keep the mount count unchanged so snapshot create still rejects the workspace..github/workflows/pr-ci.yml (1)
716-722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge into the existing
/etc/docker/daemon.jsoninstead of replacing it.The GitHub-hosted runner image ships a
/etc/docker/daemon.json.sudo tee /etc/docker/daemon.jsondiscards its current contents, so any preconfigured daemon settings are lost for this job. Merge the new key into the existing file.♻️ Proposed merge-based configuration
echo "127.0.0.1 host.docker.internal" | sudo tee -a /etc/hosts - echo '{"insecure-registries": ["host.docker.internal:15500"]}' | sudo tee /etc/docker/daemon.json + sudo mkdir -p /etc/docker + existing='{}' + if [ -s /etc/docker/daemon.json ]; then + existing=$(sudo cat /etc/docker/daemon.json) + fi + echo "$existing" \ + | jq '. + {"insecure-registries": ["host.docker.internal:15500"]}' \ + | sudo tee /etc/docker/daemon.json sudo systemctl restart docker timeout 30 bash -c 'until docker info >/dev/null 2>&1; do sleep 1; done'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-ci.yml around lines 716 - 722, Update the “configure insecure registry for snapshot tests (Linux)” step to merge the host.docker.internal insecure registry entry into the existing /etc/docker/daemon.json rather than overwriting it, preserving all preconfigured Docker daemon settings before restarting Docker.e2e/tests/snapshot/helper.go (2)
117-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the
registry:2fixture image by digest.The workflow already pins other fixture images by digest, for example
busybox@sha256:...in.github/workflows/pr-ci.yml.registry:2is a mutable tag, so an upstream re-push can change fixture behavior without any change in this repository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/snapshot/helper.go` around lines 117 - 123, Update the Docker image argument in the dockerHelper.Run invocation to use a pinned registry fixture digest instead of the mutable "registry:2" tag, matching the digest-pinning convention used by other fixtures while preserving the existing registry image version and command flow.
130-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the retry wait cancellable and clear leaked containers between attempts.
Two gaps exist in the retry loop:
time.Sleepignoresctx. If the spec context is cancelled during the wait, the loop still blocks for the full 3 seconds and then starts anotherdocker run.- A failed attempt can still leave a container holding
registryHostPort. The label sweep runs only once, before the loop, so the next attempt fails with a port-allocation error instead of the original cause.♻️ Proposed retry-loop hardening
if attempt < registryStartAttempts { ginkgo.GinkgoWriter.Printf( "[retry] start local registry: attempt %d failed, retrying: %v\n", attempt, lastErr, ) - time.Sleep(3 * time.Second) + if leaked, ferr := dockerHelper.FindContainer(ctx, []string{ + "devsy-e2e-snapshot-registry=true", + }); ferr == nil { + for _, id := range leaked { + _ = dockerHelper.Stop(ctx, id) + _ = dockerHelper.Remove(ctx, id) + } + } + select { + case <-ctx.Done(): + return "", fmt.Errorf("start local registry: %w (last error: %w)", ctx.Err(), lastErr) + case <-time.After(3 * time.Second): + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/snapshot/helper.go` around lines 130 - 140, Update the retry loop around the local registry startup to wait with a context-aware mechanism using ctx instead of time.Sleep, returning promptly when cancellation occurs and avoiding another docker run. Also rerun the existing container cleanup/label sweep between failed attempts before retrying, so leaked containers no longer retain registryHostPort.e2e/tests/snapshot/snapshot.go (1)
203-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the spec name with what it asserts.
The name states that no manifest stays visible. The registry container is removed before
snapshot createruns, so no manifest visibility check happens. The spec only asserts that create fails and the workspace stays reachable. Rename it, for example to "fails snapshot create when the registry is unreachable and leaves the workspace intact".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/snapshot/snapshot.go` around lines 203 - 223, Rename the Ginkgo spec containing the registry cleanup and snapshotCmd create invocation to describe its actual assertions: snapshot creation fails when the registry is unreachable and the workspace remains intact. Remove the misleading “leaves no manifest visible” wording while preserving the test implementation.sites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx (1)
71-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the transfer wording.
The sentence uses informal emphasis and an idiom. State the behavior directly.
✏️ Proposed wording
-Restoring into a different provider than the snapshot was created with **is** the provider-transfer story — it's the same restore command, just pointed at a different `--provider`: +To transfer a workspace between providers, restore the snapshot with a different `--provider`. No separate command is required:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx` around lines 71 - 77, Update the “Transferring Between Providers” paragraph to state directly that restoring a snapshot with a different --provider transfers it between providers, removing the informal emphasis and “provider-transfer story” phrasing while preserving the restore command example.Source: Linters/SAST tools
pkg/snapshot/registry.go (2)
173-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the close contract on
PullBlob.
PullBlobis exported and returns anio.ReadCloser. The caller owns the stream. The function has no doc comment, unlikePushBlobandPushBlobStreaming. A caller that misses theCloseleaks the underlying HTTP response body for the life of the process.State the ownership in a doc comment.
📝 Proposed doc comment
+// PullBlob opens the content-addressed blob identified by digest in +// repository. The caller must close the returned reader; it holds an open +// registry response body until closed. func PullBlob(ctx context.Context, repository, digest string) (io.ReadCloser, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/snapshot/registry.go` around lines 173 - 194, Add a Go doc comment immediately above the exported PullBlob function documenting that callers own the returned io.ReadCloser and must close it after reading. Match the existing documentation style used by PushBlob and PushBlobStreaming without changing PullBlob’s behavior.
314-332: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the cost of listing every tag, and log skipped tags.
remote.Listreturns every tag in the repository.ListRefsthen discards the tags that do not belong toworkspaceID. If many workspaces share one snapshot repository, eachsnapshot listcall fetches the full tag list, which is paginated over multiple round trips. The existing comment records the tradeoff against per-manifest GETs, which is reasonable. Plan for a bound as repositories grow, for example a per-workspace repository suffix or a cached tag index.The
continueat Line 327 also drops the parse error. If a snapshot tag is malformed,snapshot listomits it with no signal. Log the skip at debug level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/snapshot/registry.go` around lines 314 - 332, Update ListRefs to log malformed snapshot tags at debug level before continuing, including the tag and parse error. Also reduce unbounded remote.List pagination by introducing a repository-scope bound such as a per-workspace repository suffix or cached tag index, while preserving filtering by workspaceID.pkg/snapshot/compose.go (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the existing
providerconstant for theimage:prefix.The file already imports
pkg/providerand usesprovider.WorkspaceSourceSnapshot. The"image:"literal duplicatesprovider.WorkspaceSourceImage. Use the constant so both prefixes stay in one place.♻️ Proposed refactor
- return provider.WorkspaceSourceSnapshot + ref.String(), "image:" + ref.FSImageRef(), nil + return provider.WorkspaceSourceSnapshot + ref.String(), + provider.WorkspaceSourceImage + ref.FSImageRef(), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/snapshot/compose.go` at line 15, Update the return expression in the snapshot composition logic to replace the duplicated "image:" literal with the existing provider.WorkspaceSourceImage constant, while preserving the current ref.String() and ref.FSImageRef() behavior.pkg/snapshot/ref.go (1)
97-102: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo change needed.
ListRefsskips tags thatParseRefcannot parse, so-fstags in the same repository do not fail the listing path, althoughTestListRefs_FiltersByWorkspaceIDshould cover*-fstags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/snapshot/ref.go` around lines 97 - 102, No implementation change is required for Ref.FSImageRef. Preserve the existing "-fs" suffix behavior, and add coverage to TestListRefs_FiltersByWorkspaceID for matching *-fs tags while retaining ListRefs filtering of unparseable refs.pkg/driver/docker/lifecycle.go (1)
161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the workspace ID in the not-found error.
The caller wraps this error as
commit container: container not found. The message does not identify the workspace. AddworkspaceIDso users can diagnose the failure.♻️ Proposed change
} else if container == nil { - return fmt.Errorf("container not found") + return fmt.Errorf("container not found for workspace %s", workspaceID) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/driver/docker/lifecycle.go` around lines 161 - 167, Update the container-not-found branch in CommitContainer to include the workspaceID in the returned error message, while preserving the existing error propagation for FindDevContainer failures.cmd/snapshot/create.go (1)
86-94: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe committed image stays in the local daemon after the push.
CommitContainercreates a local image taggedfsTag. Nothing removes it afterPushDevContainersucceeds. Eachdevsy snapshot createcall adds one full container-filesystem image to local disk. Repeated snapshots consume disk without bound.Remove the local tag after a successful push, or document the cleanup requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/snapshot/create.go` around lines 86 - 94, Update the snapshot creation flow around cmd.commitAndPushImage to remove the local fsTag image after the push succeeds, ensuring cleanup occurs only after a successful push while preserving existing error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/snapshot/create.go`:
- Around line 91-99: Ensure snapshot capture in the flow containing
commitAndPushImage and pushVolumes is consistent by quiescing the container for
the entire operation: stop or otherwise pause writes before commitAndPushImage,
keep it quiesced through pushVolumes, and restore its prior running state after
completion, including error paths. If this cannot be implemented, document the
limitation and instruct users to stop write activity before creating snapshots.
- Around line 333-345: Update redactedContainerEnv to remove all secret-like
container environment keys, including names matching credential patterns such as
TOKEN, SECRET, PASSWORD, and KEY, while continuing to exclude
config.EnvWorkspaceDaemonConfig. Log only the dropped key names, never their
values, before the resulting map is passed through snapshot metadata creation
and PushManifest.
In `@cmd/snapshot/restore.go`:
- Around line 39-55: Update NewRestoreCmd’s flag registration for
cmd.ProviderName to use a distinct target-provider flag name instead of
provider, while preserving its existing description and binding. Leave the
global --provider configuration flag bound to cmd.Provider unchanged.
In `@cmd/workspace/up/up_client.go`:
- Around line 481-522: Update the --from-snapshot handling around
applyFromSnapshotOverrides to explicitly resolve conflicts between manifest
RunArgs/ContainerEnv and user-provided cmd.RunArgs/cmd.ContainerEnv. Reject
independently supplied overrides like source options, or merge/retain them with
a clearly defined precedence, ensuring the chosen behavior is documented and
consistent.
In `@e2e/tests/snapshot/snapshot.go`:
- Around line 249-254: Verify the rejection producer used by the snapshot-create
flow around ExecCommandCapture before asserting its message. If the command does
not emit “does not yet support multiple mounts,” either implement that rejection
behavior in the relevant snapshot-create command path or remove/rework the
stderr assertion to match the actual contract; do not retain an assertion for
text the command cannot produce.
In `@pkg/driver/microsandbox/microsandbox.go`:
- Around line 245-257: Update CommitContainer to query the microsandbox
workspace for a Docker commit target before calling
snapshotCapable.CommitContainer; when no target exists, return the existing
“microsandbox's underlying docker driver does not support snapshot commit”
error, and only delegate with the resolved target when available.
In `@pkg/snapshot/manifest.go`:
- Around line 207-226: Update ParseManifest to require exactly two layers and
validate each layer’s expected media type, rejecting reordered or incomplete
manifests before returning. Add Manifest.ContainerImage and Manifest.Volumes
accessors for the positional descriptors, then update consumers to use these
accessors instead of indexing Layers directly.
In `@sites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx`:
- Line 85: Rewrite the delete-semantics sentence in the workspace snapshots
documentation to state that `devsy snapshot delete` removes only the snapshot
manifest from the registry, while orphaned blobs remain until registry garbage
collection. Remove the claim that deletion waits for workspace sources to finish
uploading or guarantees atomic visibility.
---
Nitpick comments:
In @.github/workflows/pr-ci.yml:
- Around line 716-722: Update the “configure insecure registry for snapshot
tests (Linux)” step to merge the host.docker.internal insecure registry entry
into the existing /etc/docker/daemon.json rather than overwriting it, preserving
all preconfigured Docker daemon settings before restarting Docker.
In `@cmd/snapshot/create.go`:
- Around line 86-94: Update the snapshot creation flow around
cmd.commitAndPushImage to remove the local fsTag image after the push succeeds,
ensuring cleanup occurs only after a successful push while preserving existing
error propagation.
In `@e2e/tests/snapshot/helper.go`:
- Around line 117-123: Update the Docker image argument in the dockerHelper.Run
invocation to use a pinned registry fixture digest instead of the mutable
"registry:2" tag, matching the digest-pinning convention used by other fixtures
while preserving the existing registry image version and command flow.
- Around line 130-140: Update the retry loop around the local registry startup
to wait with a context-aware mechanism using ctx instead of time.Sleep,
returning promptly when cancellation occurs and avoiding another docker run.
Also rerun the existing container cleanup/label sweep between failed attempts
before retrying, so leaked containers no longer retain registryHostPort.
In `@e2e/tests/snapshot/snapshot.go`:
- Around line 203-223: Rename the Ginkgo spec containing the registry cleanup
and snapshotCmd create invocation to describe its actual assertions: snapshot
creation fails when the registry is unreachable and the workspace remains
intact. Remove the misleading “leaves no manifest visible” wording while
preserving the test implementation.
In `@e2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.json`:
- Line 5: Update the second mount in the devcontainer fixture to be read-only
while preserving its existing source, target, and bind-mount type. Keep the
mount count unchanged so snapshot create still rejects the workspace.
In `@pkg/driver/docker/lifecycle.go`:
- Around line 161-167: Update the container-not-found branch in CommitContainer
to include the workspaceID in the returned error message, while preserving the
existing error propagation for FindDevContainer failures.
In `@pkg/snapshot/compose.go`:
- Line 15: Update the return expression in the snapshot composition logic to
replace the duplicated "image:" literal with the existing
provider.WorkspaceSourceImage constant, while preserving the current
ref.String() and ref.FSImageRef() behavior.
In `@pkg/snapshot/ref.go`:
- Around line 97-102: No implementation change is required for Ref.FSImageRef.
Preserve the existing "-fs" suffix behavior, and add coverage to
TestListRefs_FiltersByWorkspaceID for matching *-fs tags while retaining
ListRefs filtering of unparseable refs.
In `@pkg/snapshot/registry.go`:
- Around line 173-194: Add a Go doc comment immediately above the exported
PullBlob function documenting that callers own the returned io.ReadCloser and
must close it after reading. Match the existing documentation style used by
PushBlob and PushBlobStreaming without changing PullBlob’s behavior.
- Around line 314-332: Update ListRefs to log malformed snapshot tags at debug
level before continuing, including the tag and parse error. Also reduce
unbounded remote.List pagination by introducing a repository-scope bound such as
a per-workspace repository suffix or cached tag index, while preserving
filtering by workspaceID.
In `@sites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx`:
- Around line 71-77: Update the “Transferring Between Providers” paragraph to
state directly that restoring a snapshot with a different --provider transfers
it between providers, removing the informal emphasis and “provider-transfer
story” phrasing while preserving the restore command example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b981e8f5-0de9-49e5-ad81-e92031bb61b5
⛔ Files ignored due to path filters (2)
pkg/agent/tunnel/tunnel.pb.gois excluded by!**/*.pb.gopkg/agent/tunnel/tunnel_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (75)
.github/workflows/pr-ci.ymlcmd/internal/agentcontainer/setup.gocmd/internal/agentcontainer/setup_internal_test.gocmd/internal/agentcontainer/setup_test.gocmd/internal/agentworkspace/up.gocmd/root.gocmd/snapshot/create.gocmd/snapshot/create_test.gocmd/snapshot/delete.gocmd/snapshot/delete_test.gocmd/snapshot/list.gocmd/snapshot/list_test.gocmd/snapshot/restore.gocmd/snapshot/restore_test.gocmd/snapshot/snapshot.gocmd/snapshot/tunnel_test.gocmd/workspace/describe.gocmd/workspace/describe_test.gocmd/workspace/export.gocmd/workspace/import.gocmd/workspace/import_test.gocmd/workspace/up/up.gocmd/workspace/up/up_client.gocmd/workspace/up/up_client_test.gocmd/workspace/up/up_flags.gocmd/workspace/up/up_test.goe2e/e2e_suite_test.goe2e/framework/command.goe2e/framework/retry.goe2e/framework/retry_test.goe2e/tests/snapshot/helper.goe2e/tests/snapshot/snapshot.goe2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.jsone2e/tests/snapshot/testdata/docker-runargs/.devcontainer.jsone2e/tests/snapshot/testdata/docker/.devcontainer.jsonpkg/agent/snapshot/restore.gopkg/agent/snapshot/restore_test.gopkg/agent/tunnel/tunnel.protopkg/agent/tunnelserver/stream.gopkg/agent/tunnelserver/stream_test.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/tunnelserver/tunnelserver_test.gopkg/config/context.gopkg/config/context_test.gopkg/config/env.gopkg/devcontainer/config.gopkg/devcontainer/config_test.gopkg/devcontainer/setup/setup.gopkg/driver/apple/driver.gopkg/driver/docker.gopkg/driver/docker/docker.gopkg/driver/docker/lifecycle.gopkg/driver/docker/lifecycle_test.gopkg/driver/microsandbox/microsandbox.gopkg/flags/names/names.gopkg/image/image.gopkg/image/image_test.gopkg/provider/export.gopkg/provider/export_test.gopkg/provider/workspace.gopkg/provider/workspace_source_test.gopkg/snapshot/compose.gopkg/snapshot/compose_test.gopkg/snapshot/create.gopkg/snapshot/create_test.gopkg/snapshot/manifest.gopkg/snapshot/manifest_test.gopkg/snapshot/ref.gopkg/snapshot/ref_test.gopkg/snapshot/reference.gopkg/snapshot/reference_test.gopkg/snapshot/registry.gopkg/snapshot/registry_test.gosites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdxsites/docs-devsy-sh/sidebars.js
Comment on lines
+481
to
+522
| // resolveExplicitSource returns an explicit WorkspaceSource when --from-snapshot | ||
| // is set, composed identically to `devsy snapshot restore` via | ||
| // snapshot.RestoreComposition ("snapshot:<ref>" source, "image:<repo>:<tag>-fs" | ||
| // DevContainerSource), taking priority over positional-arg source resolution. | ||
| // It also sets cmd.DevContainerSource so the workspace runs the snapshot's | ||
| // committed filesystem image instead of rebuilding, matching restore's | ||
| // behavior exactly. | ||
| // | ||
| // Since --from-snapshot forbids a positional source (validateFromSnapshot), | ||
| // there is no other way for the workspace ID to reach ResolveParams.DesiredID | ||
| // on this path; without defaulting it here, workspace resolution falls back | ||
| // to selecting an unrelated existing workspace (or fails confusingly in | ||
| // non-TTY contexts). So when --id wasn't given explicitly, default it from | ||
| // the snapshot ref's workspace id, mirroring `devsy snapshot restore`'s | ||
| // buildWorkspace. | ||
| func (cmd *UpCmd) resolveExplicitSource() (*provider2.WorkspaceSource, error) { | ||
| if cmd.FromSnapshot == "" { | ||
| return nil, nil | ||
| } | ||
| sourceStr, devContainerSource, err := snapshotpkg.RestoreComposition(cmd.FromSnapshot) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parse --from-snapshot ref: %w", err) | ||
| } | ||
| cmd.DevContainerSource = devContainerSource | ||
|
|
||
| if cmd.ID == "" { | ||
| ref, err := snapshotpkg.ParseRef(cmd.FromSnapshot) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parse --from-snapshot ref: %w", err) | ||
| } | ||
| cmd.ID = ref.WorkspaceID | ||
| } | ||
|
|
||
| source := provider2.ParseWorkspaceSource(sourceStr) | ||
| if source == nil { | ||
| return nil, fmt.Errorf( | ||
| "compose workspace source from --from-snapshot: unexpected source %q", | ||
| sourceStr, | ||
| ) | ||
| } | ||
| return source, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether up_flags.go exposes RunArgs/ContainerEnv as CLI flags, and what mergeDevsyUpOptions populates.
rg -n "RunArgs|ContainerEnv" cmd/workspace/up/up_flags.go
rg -n "func mergeDevsyUpOptions" -A 30 --type=goRepository: devsy-org/devsy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
echo "Repository files around up cmd:"
git ls-files | rg '(^|/)cmd/workspace/up/|^cmd/workspace/up/' | sed -n '1,120p'
echo
echo "Search RunArgs/ContainerEnv in repository:"
rg -n "RunArgs|ContainerEnv" . --glob '!vendor/**' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | sed -n '1,200p'
echo
echo "Locate mergeDevsyUpOptions:"
rg -n "mergeDevsyUpOptions|CLIOptions" --type=go | sed -n '1,220p'
echo
echo "Candidate up files line counts:"
for f in cmd/workspace/up/*.go; do
echo "--- $f"
wc -l "$f"
doneRepository: devsy-org/devsy
Length of output: 18878
🏁 Script executed:
#!/bin/bash
set -u
echo "up_flags.go relevant contents:"
wc -l cmd/workspace/up/up_flags.go
cat -n cmd/workspace/up/up_flags.go | sed -n '1,240p'
echo
echo "up_client.go relevant contents:"
wc -l cmd/workspace/up/up_client.go
cat -n cmd/workspace/up/up_client.go | sed -n '40,230p'
cat -n cmd/workspace/up/up_client.go | sed -n '460,575p'
echo
echo "All exact CLI option references in cmd/workspace/up/up_flags.go:"
rg -n "(RunArgs|ContainerEnv|DevContainerSource|Snapshot|from-snapshot)" cmd/workspace/up/up_flags.goRepository: devsy-org/devsy
Length of output: 24679
🏁 Script executed:
#!/bin/bash
set -u
echo "Search mergeDevsyUpOptions definitions/usages in repo:"
rg -n "mergeDevsyUpOptions|type CLIOptions|CLIOptions struct|RunArgs|ContainerEnv|DevContainerSource" --type=go | sed -n '1,260p'
echo
echo "Candidate files with CLIOptions declarations:"
rg -l "type CLIOptions|CLIOptions struct|RunArgs \\[\\]string|ContainerEnv map" --type=goRepository: devsy-org/devsy
Length of output: 300
🌐 Web query:
DevSy mergeDevsyUpOptions CLIOptions RunArgs ContainerEnv
💡 Result:
The terms "mergeDevsyUpOptions," "CLIOptions," "RunArgs," and "ContainerEnv" relate to the configuration and orchestration of development environments, specifically within the context of the DevContainer specification and tools that implement it, such as DevPod or similar orchestration platforms [1][2]. 1. CLIOptions: Typically refers to a structured set of command-line interface parameters used to control the initialization or execution of development environments [2]. This often includes configuration flags for builds, registries, or provider-specific settings [2]. 2. ContainerEnv: A property defined in the DevContainer specification (usually within a devcontainer.json file) that specifies environment variables to be set on the Docker container [3][4]. These variables are applied to the container itself, meaning they are available to all processes running within that container for its lifetime [3][4]. 3. RunArgs: Also a property within the DevContainer configuration, this is an array of strings representing Docker CLI arguments (e.g., --cap-add, --security-opt, --env-file) that are applied when the container is executed [3][4]. 4. mergeDevsyUpOptions: This appears to be a specific internal function or utility within a tool (likely DevPod or a related project in the devsy-org ecosystem) responsible for merging user-provided or CLI-provided configuration options with default settings [1][2]. In such architectures, this pattern is used to combine multiple sources of truth (e.g., CLI arguments, environment configuration, and default workspace settings) to produce a final, resolved configuration that governs the container's lifecycle [2]. In practice, these components work together to resolve the final state of a workspace: RunArgs and ContainerEnv define the technical specification for the container's runtime environment [3][5], while CLIOptions allow users to override these settings during command execution, and a merge function (like mergeDevsyUpOptions) ensures that these overrides are correctly integrated into the final operational configuration [2].
Citations:
- 1: https://pkg.go.dev/github.com/devsy-org/devsy
- 2: https://github.com/loft-sh/devpod/blob/5a0efcbf/pkg/devcontainer/run.go
- 3: https://containers.dev/implementors/json_reference/
- 4: https://github.com/microsoft/vscode-docs/blob/main/remote/advancedcontainers/environment-variables.md
- 5: https://github.com/devcontainers/cli/blob/da642b4d/src/spec-node/singleContainer.ts
🏁 Script executed:
#!/bin/bash
set -u
echo "All files containing CLIOptions:"
rg -l "CLIOptions" --type=go | sed -n '1,200p'
echo
echo "Files where CLIOptions appears around RunArgs/ContainerEnv:"
for f in $(rg -l "CLIOptions" --type=go | sed -n '1,100p'); do
matches=$(rg -n "CLIOptions|RunArgs|ContainerEnv|DevContainerSource" "$f" --type=go | wc -l || true)
if [ "${matches:-0}" -gt 5 ]; then
echo "--- $f"
rg -n "CLIOptions|RunArgs|ContainerEnv|DevContainerSource" "$f" | sed -n '1,120p'
fi
doneRepository: devsy-org/devsy
Length of output: 194
🌐 Web query:
github.com/devsy-org/devsy CLIOptions UpCmd mergeDevsyUpOptions
💡 Result:
There is no evidence of a function named mergeDevsyUpOptions, an UpCmd struct, or a CLIOptions type specifically associated with the github.com/devsy-org/devsy repository in the provided search results [1][2][3][4]. The github.com/devsy-org/devsy project is an engineering workspace tool that implements the DevContainer standard [1][2]. While command-line interface structures and option merging are common patterns in Go-based CLI tools (often implemented using libraries like Cobra) [5], the specific identifiers requested do not appear in the documentation or code references for this project [1][2]. Some search results point to similarly named concepts in other projects, such as: 1. UpCmd, which refers to the Ultimate Provisioner, a separate automation tool [6][7]. 2. DevPod, which contains an UpCmd struct and CLI options in its own codebase [5]. 3. General configuration merging functions found in other developer tool repositories [8][9][10].
Citations:
- 1: https://pkg.go.dev/github.com/devsy-org/devsy@v1.9.8
- 2: https://pkg.go.dev/github.com/devsy-org/devsy
- 3: https://github.com/devsy-org
- 4: https://github.com/devsy-org/devsy/blob/v1.9.8/go.mod
- 5: https://github.com/loft-sh/devpod/blob/5a0efcbf/cmd/up.go
- 6: https://upcmd.netlify.app/
- 7: https://github.com/upcmd/up
- 8: https://github.com/loft-sh/devpod/blob/5a0efcbf/pkg/devcontainer/config/merge.go
- 9: https://github.com/deeyi2000/merge-options
- 10: https://delfick-project.readthedocs.io/en/latest/api/option_merge/api/merged_options.html
Clarify or reject snapshot runtime overrides of explicit config.
--from-snapshot rejects cmd.Source and a positional source, but applyFromSnapshotOverrides still replaces cmd.RunArgs and cmd.ContainerEnv with manifest values. Add the same explicit conflict check if those options can be supplied independently, or handle them by merging/retaining them and documenting the precedence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/workspace/up/up_client.go` around lines 481 - 522, Update the
--from-snapshot handling around applyFromSnapshotOverrides to explicitly resolve
conflicts between manifest RunArgs/ContainerEnv and user-provided
cmd.RunArgs/cmd.ContainerEnv. Reject independently supplied overrides like
source options, or merge/retain them with a clearly defined precedence, ensuring
the chosen behavior is documented and consistent.
Comment on lines
+249
to
+254
| _, stderr, err := f.ExecCommandCapture(ctx, []string{ | ||
| snapshotCmd, snapshotVerbCreate, tempDir, registryFlag, | ||
| registryHost + "/e2e/snapshots", debugFlag, | ||
| }) | ||
| gomega.Expect(err).To(gomega.HaveOccurred()) | ||
| gomega.Expect(stderr).To(gomega.ContainSubstring("does not yet support multiple mounts")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the multi-mount rejection message emitted by snapshot create.
rg -nP -C3 'multiple mounts|more than one mount' --type=goRepository: devsy-org/devsy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## tracked Go files count in snapshot-related paths"
git ls-files | rg -n 'e2e/tests/snapshot/snapshot\.go|snapshot|mount' || true
echo
echo "## exact file size and relevant lines"
wc -l e2e/tests/snapshot/snapshot.go
sed -n '220,270p' e2e/tests/snapshot/snapshot.go
echo
echo "## broader search for mount-related rejection text"
rg -n -C2 'does not yet support|multiple mount|mounts' --type=go || true
echo
echo "## git diff stat/name"
git diff --stat || true
git diff --name-only || trueRepository: devsy-org/devsy
Length of output: 4499
Do not assume the multi-mount rejection text will be emitted.
This test asserts stderr contains does not yet support multiple mounts, but no snapshot-create error string with that text exists in the checked-in Go code. Find the producer for this rejection first; if there is none, add the command behavior or remove/rework the assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/snapshot/snapshot.go` around lines 249 - 254, Verify the rejection
producer used by the snapshot-create flow around ExecCommandCapture before
asserting its message. If the command does not emit “does not yet support
multiple mounts,” either implement that rejection behavior in the relevant
snapshot-create command path or remove/rework the stderr assertion to match the
actual contract; do not retain an assertion for text the command cannot produce.
Comment on lines
+245
to
+257
| func (d *microsandboxDriver) CommitContainer(ctx context.Context, workspaceID, tag string) error { | ||
| dockerDriver, err := d.dockerImageDriver() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| snapshotCapable, ok := dockerDriver.(driver.SnapshotCapableDriver) | ||
| if !ok { | ||
| return fmt.Errorf( | ||
| "microsandbox's underlying docker driver does not support snapshot commit", | ||
| ) | ||
| } | ||
| return snapshotCapable.CommitContainer(ctx, workspaceID, tag) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect microsandbox container creation and how sandboxes map to docker containers.
fd -t f 'microsandbox' -e go | xargs rg -n -C5 'sandboxName|IDLabels|GetIDLabels|docker run|createSandbox'Repository: devsy-org/devsy
Length of output: 10765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l pkg/driver/microsandbox/microsandbox.go pkg/driver/docker/docker.go 2>/dev/null || true
echo "== microsandbox relevant sections =="
sed -n '1,430p' pkg/driver/microsandbox/microsandbox.go | cat -n
echo "== docker FindDevContainer relevant section =="
sed -n '150,225p' pkg/driver/docker/docker.go | cat -nRepository: devsy-org/devsy
Length of output: 17448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== driver snapshot interface and commit details usages =="
rg -n -C4 'type SnapshotCapableDriver|CommitContainer\(ctx context.Context' pkg --glob '*.go'
echo "== config GetIDLabels and labels constants =="
rg -n -C4 'func GetIDLabels|const .*Label|GetDevContainer|IDLabels' pkg/devcontainer/pkg pkg/driver/pkg/pkg pkg --glob '*.go'
echo "== docker helper FindDevContainer implementations =="
rg -n -C6 'FindDevContainer\(|FindContainerByID\(|IDLabels|ContainerID' pkg/image pkg/config pkg/provider pkg/driver/docker pkg/driver --glob '*.go'Repository: devsy-org/devsy
Length of output: 33174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lifecycle CommitContainer full implementation =="
sed -n '155,185p' pkg/driver/docker/lifecycle.go | cat -n
echo "== docker helper FindDevContainer FindContainerByID implementations =="
fd -t f 'dockerhelper|docker' pkg -e go | while read -r f; do
if rg -q 'func .*FindDevContainer|func .*FindContainerByID|labels' "$f"; then
echo "--- $f"
rg -n -C6 'func .*FindDevContainer|func .*FindContainerByID|labels|ContainerID|IDLabels' "$f"
fi
done
echo "== docker command line label construction and running filters =="
rg -n -C5 'filterArgs|ContainerList|Labels|ContainerID|runargs|BuildRunArgs|devsy' pkg/driver/docker pkg/config pkg --glob '*.go'
echo "== provider workspace source and run command mapping =="
sed -n '200,280p' pkg/provider/workspace.go | cat -n
sed -n '180,260p' pkg/docker/dockerhelper.go | cat -nRepository: devsy-org/devsy
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -u
echo "== lifecycle CommitContainer =="
sed -n '155,185p' pkg/driver/docker/lifecycle.go | cat -n
echo "== docker helper FindDevContainer and FindContainerByID =="
fd -t f 'helper.go' pkg/docker pkg/api pkg --exclude '*_test.go' | while read -r f; do
if rg -q 'FindDevContainer|FindContainerByID|labels' "$f"; then
echo "--- $f"
rg -n -C8 'FindDevContainer|FindContainerByID|ContainerList|ContainerID|IDLabels|labels' "$f"
fi
done
echo "== config labels and Docker label constants =="
rg -n -C4 'DevcontainerIDLabel|DockerUserLabel|GetDockerLabelForID|GetIDLabels|Microsandbox|MicrosandboxDriver|provider' pkg/config pkg/devcontainer/config pkg/provider --glob '*.go'
echo "== focused microsandbox labels/buildSpec/delegations =="
sed -n '350,370p;245,260p;137,176p' pkg/driver/microsandbox/microsandbox.go | cat -nRepository: devsy-org/devsy
Length of output: 50372
Make CommitContainer fail with an explicit unsupported error.
CommitContainer delegates to Docker’s FindDevContainer, which matches non-microsandbox Docker containers by ID labels. A microsandbox VM has the same ID labels but is not a Docker container, so snapshot commit can return container not found instead of the microsandbox-specific microsandbox's underlying docker driver does not support snapshot commit. Guard the delegation by asking the microsandbox workspace whether it has a Docker commit target, and return the existing unsupported error when it does not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/driver/microsandbox/microsandbox.go` around lines 245 - 257, Update
CommitContainer to query the microsandbox workspace for a Docker commit target
before calling snapshotCapable.CommitContainer; when no target exists, return
the existing “microsandbox's underlying docker driver does not support snapshot
commit” error, and only delegate with the resolved target when available.
skevetter
marked this pull request as draft
August 2, 2026 19:23
Fixed: - restore.go: renamed the local --provider flag to --target-provider; it collided with the persistent global --provider flag (both bound the same pflag name), silently leaving config.LoadConfig's provider argument empty no matter what --provider value was passed to `snapshot restore`. - create.go: remove the locally-committed fsTag image after a successful push via the new SnapshotCapableDriver.RemoveImage, to stop unbounded local disk growth across repeated `snapshot create` runs. - create.go: broadened redactedContainerEnv to also drop any containerEnv key matching a credential-like naming pattern (token/secret/password/ api_key), logging only the dropped key names, not values. - create.go: documented why workspace filesystem commit and volumes read aren't quiesced against each other (volumes are read off host bind mount paths, not through the container, so stopping it wouldn't close the consistency window anyway, and would interrupt the live workspace). - microsandbox: removed its (incorrect) SnapshotCapableDriver delegation — microsandbox VMs aren't docker containers, so CommitContainer would always fail via the underlying docker driver's container lookup instead of surfacing create.go's existing clear "no container commit support" error. - lifecycle.go: CommitContainer's "container not found" error now includes workspaceID. - pkg/snapshot: PullBlob doc comment clarifies caller-owns-and-must-Close; ListRefs logs skipped/malformed tags at debug level and documents remote.List's per-repository (not per-workspace) pagination cost; compose.go uses provider.WorkspaceSourceImage instead of a raw "image:" literal; added Manifest.ContainerImage()/Volumes() accessors and switched pkg/agent/snapshot/restore.go's positional layer lookup to Volumes(). - e2e: pinned the registry:2 fixture image by digest; made the registry start retry loop's wait ctx-cancellable and re-sweep leaked containers between attempts, not just once before the loop; made the multi-mount fixture's second bind mount read-only; renamed the mid-push-failure spec to match what it actually asserts. - .github/workflows/pr-ci.yml: merge into any pre-existing /etc/docker/daemon.json via jq instead of overwriting it with tee. - docs: workspace-snapshots.mdx's --provider references updated to --target-provider; corrected the delete-semantics section, which described create's atomicity guarantee rather than delete's actual by-digest-delete-with-blob-GC behavior; simplified the transferring- between-providers paragraph. Investigated and skipped: - up_client.go's --from-snapshot vs RunArgs/ContainerEnv precedence: not reachable today. up.Options.RunArgs/ContainerEnv have no CLI flags; the only two callers that set them (snapshot restore, mcp tools_workspace) never also set FromSnapshot, and validateFromSnapshot short-circuits before applyFromSnapshotOverrides when FromSnapshot is empty. - e2e's multi-mount rejection spec assertion: re-verified checkSingleMount in create.go produces the exact "does not yet support multiple mounts" substring the spec checks for; the assertion is accurate.
The previous commit removed the locally-committed fsTag image right after pushing it, to address a CodeRabbit disk-growth nitpick. That broke the CI snapshot e2e suite: `up --from-snapshot`/`snapshot restore` commonly run against the same tag moments after create, and once the local image is gone, pkg/docker's InspectImage falls back to a remote fetch (pkg/image.GetImage -> remote.Image) that does not honor pkg/snapshot/reference.go's dockerInternalHost insecure-registry override. Against the e2e suite's plain-HTTP registry:2 fixture this fails with "server gave HTTP response to HTTPS client", which is exactly what CI hit. Revert the removal and drop the now-unused SnapshotCapableDriver.RemoveImage method entirely rather than leave dead code.
skevetter
marked this pull request as ready for review
August 2, 2026 22:22
skevetter
marked this pull request as draft
August 2, 2026 22:23
skevetter
marked this pull request as ready for review
August 3, 2026 00:16
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/snapshot/create.go`:
- Around line 347-353: Extend credentialKeyPattern to match private-key,
access-key, credential, authorization, and personal-access-token key names,
including SSH_PRIVATE_KEY, AWS_ACCESS_KEY_ID, GITHUB_PAT, CREDENTIAL, and
AUTHORIZATION while preserving existing matches. Add redactedContainerEnv tests
covering each newly supported category.
In `@pkg/snapshot/manifest.go`:
- Around line 200-216: Update ParseManifest to validate the complete layer
contract before returning a manifest: require exactly one container image layer
and exactly one VolumesMediaType layer, reject duplicates, missing layers, and
any unsupported media types, and preserve the validated manifest for
ContainerImage and Volumes to access. Add malformed-manifest tests covering each
rejection case; the accessors should no longer determine validity by selecting
the first matching layer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7652ccfd-7122-43ee-9f51-0de0038f4f5e
📒 Files selected for processing (15)
.github/workflows/pr-ci.ymlcmd/snapshot/create.gocmd/snapshot/create_test.gocmd/snapshot/restore.goe2e/tests/snapshot/helper.goe2e/tests/snapshot/snapshot.goe2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.jsonpkg/agent/snapshot/restore.gopkg/driver/docker/lifecycle.gopkg/snapshot/compose.gopkg/snapshot/manifest.gopkg/snapshot/manifest_test.gopkg/snapshot/registry.gopkg/snapshot/registry_test.gosites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx
🚧 Files skipped from review as they are similar to previous changes (10)
- sites/docs-devsy-sh/pages/developing-in-workspaces/workspace-snapshots.mdx
- e2e/tests/snapshot/testdata/docker-multi-mount/.devcontainer.json
- pkg/driver/docker/lifecycle.go
- pkg/agent/snapshot/restore.go
- pkg/snapshot/compose.go
- cmd/snapshot/restore.go
- e2e/tests/snapshot/snapshot.go
- pkg/snapshot/registry_test.go
- e2e/tests/snapshot/helper.go
- pkg/snapshot/registry.go
skevetter
marked this pull request as draft
August 3, 2026 00:19
skevetter
marked this pull request as ready for review
August 3, 2026 01:04
skevetter
marked this pull request as draft
August 3, 2026 01:13
skevetter
marked this pull request as ready for review
August 3, 2026 01:32
skevetter
marked this pull request as draft
August 3, 2026 01:48
…ontract - create.go: extend credentialKeyPattern to also match private-key, access-key, credential, authorization, and *_PAT-style key names (SSH_PRIVATE_KEY, AWS_ACCESS_KEY_ID, GITHUB_PAT, CREDENTIAL, AUTHORIZATION), with tests covering each new category plus a does-not-drop-unrelated-keys regression test. - manifest.go: ParseManifest now validates the full layer contract (exactly one container image layer, exactly one volumes layer, no duplicates, no missing layers) instead of leaving it to ContainerImage()/Volumes() to silently pick the first matching layer. Both accessors now go through the same validateLayers check, so a malformed manifest fails at parse time with a specific error rather than resolving to whichever layer happened to match first.
- manifest.go: ParseManifest now also rejects a wrong top-level mediaType (previously only schemaVersion/artifactType were checked), with a test. - helper.go: startLocalRegistry now polls the registry's /v2/ endpoint until it responds before returning, since `docker run -d` succeeding only means the container was created, not that its HTTP server is accepting connections yet -- a push immediately after could otherwise race a connection-refused. - tunnel_test.go: bound newLocalTunnelClient/StreamSnapshotVolumes to a 10s context instead of context.Background() so a stuck Recv can't hang the test suite, and assert the terminal Recv error is io.EOF specifically instead of treating any error as clean completion. Investigated and skipped: a suggested Linkname re-prefixing fix in tunnelserver.go's appendDirToTar -- pkg/extract.WriteTarExclude only ever emits tar.TypeSymlink entries (via tar.FileInfoHeader with an os.Readlink target), never tar.TypeLink, so there's no hardlink Linkname to re-prefix. The other three findings (compose_build.go build-args, setup_gpg.go lock path, port_forward.go GPG state) are pre-existing on origin/main, from PRs unrelated to this branch's snapshot work -- confirmed via `git diff origin/main HEAD` showing no changes to those files on this branch.
It was already implicitly bounded by the caller's ctx (ginkgo's SpecTimeout), but had no timeout of its own -- a future caller passing an unbounded context would hang forever. Switch to wait.PollUntilContextTimeout with a 30s bound, matching the wait.Backoff-based retry pattern already used elsewhere in this suite (e2e/framework/retry.go) instead of a hand-rolled for/select loop.
skevetter
force-pushed
the
feat/container-versioning-v2
branch
from
August 3, 2026 02:40
7fe6b2c to
8caca24
Compare
skevetter
marked this pull request as ready for review
August 3, 2026 03:25
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/snapshot/helper.go`:
- Around line 100-105: Update the readiness callback around client.Do(req) to
inspect resp.StatusCode and return true only when the /v2/ request returns
http.StatusOK; return false for all other HTTP statuses while preserving
response-body cleanup and existing error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cafedbce-2089-4361-bdc2-516acfdb1547
📒 Files selected for processing (7)
cmd/snapshot/create.gocmd/snapshot/create_test.gocmd/snapshot/tunnel_test.goe2e/framework/retry.goe2e/tests/snapshot/helper.gopkg/snapshot/manifest.gopkg/snapshot/manifest_test.go
💤 Files with no reviewable changes (1)
- e2e/framework/retry.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/snapshot/manifest_test.go
- cmd/snapshot/tunnel_test.go
- cmd/snapshot/create.go
- pkg/snapshot/manifest.go
skevetter
marked this pull request as draft
August 3, 2026 03:36
waitForRegistryReady only checked that the HTTP round-trip succeeded, not that the registry actually returned a healthy response -- a transient non-200 (e.g. from something still initializing) would be misread as ready, letting a push race an unready backend.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
devsy snapshot create/list/restore/deleteanddevsy up --from-snapshot: snapshots a workspace's committed container filesystem and bind-mounted volumes into an OCI registry, and restores or transfers them into a new workspace (optionally on a different provider).ImageDriver.CommitContainervia a new narrowSnapshotCapableDrivercapability interface) and pushes it alongside the tarred volume mounts and a manifest tying both together by digest.devsy workspace export/importcarry a snapshot ref end-to-end for full state export, not just metadata.Notable design/correctness decisions from review
host.docker.internal-is-insecure override needed only by the snapshot e2e fixture is scoped topkg/snapshotrather than leaking intopkg/image's general-purpose, security-sensitive reference parsing.--from-snapshotis rejected in platform mode, matching snapshot create's local-only constraint.CommitContainermoved off the sharedImageDriverinterface onto a narrowSnapshotCapableDrivercapability interface, so drivers that can't commit a container filesystem (e.g. Apple'scontainer) don't have to stub it.WorkspaceSource.String()/.Type()now share one field-precedence method so they can't independently disagree on a source's kind.CI prerequisite
E2E snapshot tests require a local registry fixture reachable from both host and container, which needs Docker daemon config:
{ "insecure-registries": ["host.docker.internal:15500"] }Summary by CodeRabbit
workspace up --from-snapshotfor launching workspaces from saved snapshots.