From ff51b7ce21be4dbca755d05a350f2ab0f972df73 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 14 Apr 2026 16:13:05 -0300 Subject: [PATCH] workspace.export: add base_commit to capture worker git commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without base_commit, export diffs against the current guest HEAD. If the worker ran git commit inside the VM, HEAD advanced and the diff came back empty — committed work was silently lost. With base_commit set to the head_commit from workspace.prepare, the diff uses that fixed point instead. After git add -A the index holds the full working state, so git diff --cached captures everything: committed deltas (HEAD moved past base) and any uncommitted changes on top, in one patch, applied with the same git apply flow. - WorkspaceExportParams gains base_commit - WorkspaceExportResult echoes back the ref actually used - CLI gains --base-commit flag - Tests assert scripts use the caller-supplied ref and that omitting it falls back to HEAD --- AGENTS.md | 2 + internal/api/types.go | 6 +- internal/cli/banger.go | 10 ++- internal/cli/cli_test.go | 27 ++++++++ internal/daemon/workspace.go | 22 +++++-- internal/daemon/workspace_test.go | 105 +++++++++++++++++++++++++++++- 6 files changed, 162 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b67b38f..e6c5039 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,5 +1,7 @@ # Repository Guidelines +Always run `make build` before commit. + ## Project Structure - `cmd/banger` and `cmd/bangerd` are the main user entrypoints. diff --git a/internal/api/types.go b/internal/api/types.go index eccb9c8..e1c89d8 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -215,12 +215,14 @@ type GuestSessionSendResult struct { } type WorkspaceExportParams struct { - IDOrName string `json:"id_or_name"` - GuestPath string `json:"guest_path,omitempty"` + IDOrName string `json:"id_or_name"` + GuestPath string `json:"guest_path,omitempty"` + BaseCommit string `json:"base_commit,omitempty"` } type WorkspaceExportResult struct { GuestPath string `json:"guest_path"` + BaseCommit string `json:"base_commit"` Patch []byte `json:"patch"` ChangedFiles []string `json:"changed_files"` HasChanges bool `json:"has_changes"` diff --git a/internal/cli/banger.go b/internal/cli/banger.go index 38393b9..996041f 100644 --- a/internal/cli/banger.go +++ b/internal/cli/banger.go @@ -941,13 +941,15 @@ func newVMWorkspacePrepareCommand() *cobra.Command { func newVMWorkspaceExportCommand() *cobra.Command { var guestPath string var outputPath string + var baseCommit string cmd := &cobra.Command{ Use: "export ", Short: "Pull changes from a guest workspace back to the host as a patch", - Long: "Stage all changes inside the guest workspace (git add -A) and emit a binary-safe unified diff against HEAD. With no --output flag the patch is written to stdout so it can be piped directly to git apply.", + Long: "Stage all changes inside the guest workspace (git add -A) and emit a binary-safe unified diff. Pass --base-commit with the head_commit from workspace prepare to capture changes even when the worker ran git commit inside the VM. Without --base-commit the diff is against the current guest HEAD, which misses committed changes.", Args: exactArgsUsage(1, "usage: banger vm workspace export "), Example: strings.TrimSpace(` banger vm workspace export devbox | git apply + banger vm workspace export devbox --base-commit abc1234 | git apply banger vm workspace export devbox --output worker.diff banger vm workspace export devbox --guest-path /root/project --output changes.diff `), @@ -957,8 +959,9 @@ func newVMWorkspaceExportCommand() *cobra.Command { return err } result, err := vmWorkspaceExportFunc(cmd.Context(), layout.SocketPath, api.WorkspaceExportParams{ - IDOrName: args[0], - GuestPath: guestPath, + IDOrName: args[0], + GuestPath: guestPath, + BaseCommit: baseCommit, }) if err != nil { return err @@ -981,6 +984,7 @@ func newVMWorkspaceExportCommand() *cobra.Command { } cmd.Flags().StringVar(&guestPath, "guest-path", "/root/repo", "guest workspace path") cmd.Flags().StringVar(&outputPath, "output", "", "write patch to this file instead of stdout") + cmd.Flags().StringVar(&baseCommit, "base-commit", "", "diff from this commit (use head_commit from workspace prepare to capture worker git commits)") return cmd } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index ca5f38b..2fa6efc 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -2218,3 +2218,30 @@ func TestVMWorkspaceExportGuestPathFlag(t *testing.T) { t.Fatalf("IDOrName = %q, want devbox", capturedParams.IDOrName) } } + +func TestVMWorkspaceExportBaseCommitFlag(t *testing.T) { + stubEnsureDaemonForSend(t) + + origExport := vmWorkspaceExportFunc + t.Cleanup(func() { vmWorkspaceExportFunc = origExport }) + + var capturedParams api.WorkspaceExportParams + vmWorkspaceExportFunc = func(_ context.Context, _ string, params api.WorkspaceExportParams) (api.WorkspaceExportResult, error) { + capturedParams = params + return api.WorkspaceExportResult{ + HasChanges: false, + BaseCommit: params.BaseCommit, + }, nil + } + + cmd := NewBangerCommand() + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{"vm", "workspace", "export", "devbox", "--base-commit", "abc1234deadbeef"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if capturedParams.BaseCommit != "abc1234deadbeef" { + t.Fatalf("BaseCommit = %q, want abc1234deadbeef", capturedParams.BaseCommit) + } +} diff --git a/internal/daemon/workspace.go b/internal/daemon/workspace.go index 85919dd..6510799 100644 --- a/internal/daemon/workspace.go +++ b/internal/daemon/workspace.go @@ -53,11 +53,23 @@ func (d *Daemon) ExportVMWorkspace(ctx context.Context, params api.WorkspaceExpo } defer client.Close() - // Stage all changes then emit a binary-safe unified diff against HEAD. - // --binary ensures binary files are handled correctly by git apply. + // diffRef is the git ref everything is diffed against. + // When the caller supplies BaseCommit (the HEAD at workspace.prepare time), + // we diff against that fixed point so committed guest changes are included. + // Without it we fall back to HEAD, which silently drops them. + diffRef := strings.TrimSpace(params.BaseCommit) + if diffRef == "" { + diffRef = "HEAD" + } + + // Stage all changes then emit a binary-safe unified diff against diffRef. + // After git add -A the index contains the full working state, so + // git diff --cached captures both committed deltas (HEAD moved + // past diffRef) and any additional uncommitted changes on top. patchScript := fmt.Sprintf( - "set -euo pipefail\ncd %s\ngit add -A\ngit diff --cached HEAD --binary\n", + "set -euo pipefail\ncd %s\ngit add -A\ngit diff --cached %s --binary\n", guestShellQuote(guestPath), + guestShellQuote(diffRef), ) patch, err := client.RunScriptOutput(ctx, patchScript) if err != nil { @@ -66,8 +78,9 @@ func (d *Daemon) ExportVMWorkspace(ctx context.Context, params api.WorkspaceExpo // Enumerate changed paths (index already staged; this is a cheap read). namesScript := fmt.Sprintf( - "set -euo pipefail\ncd %s\ngit diff --cached HEAD --name-only\n", + "set -euo pipefail\ncd %s\ngit diff --cached %s --name-only\n", guestShellQuote(guestPath), + guestShellQuote(diffRef), ) namesOut, _ := client.RunScriptOutput(ctx, namesScript) var changed []string @@ -79,6 +92,7 @@ func (d *Daemon) ExportVMWorkspace(ctx context.Context, params api.WorkspaceExpo return api.WorkspaceExportResult{ GuestPath: guestPath, + BaseCommit: diffRef, Patch: patch, ChangedFiles: changed, HasChanges: len(patch) > 0, diff --git a/internal/daemon/workspace_test.go b/internal/daemon/workspace_test.go index df0ea90..b43c09d 100644 --- a/internal/daemon/workspace_test.go +++ b/internal/daemon/workspace_test.go @@ -17,6 +17,7 @@ import ( // Each call to RunScriptOutput returns the next response from the queue. type exportGuestClient struct { responses []exportGuestResponse + scripts []string callIndex int } @@ -31,7 +32,8 @@ func (e *exportGuestClient) RunScript(_ context.Context, _ string, _ io.Writer) return nil } -func (e *exportGuestClient) RunScriptOutput(_ context.Context, _ string) ([]byte, error) { +func (e *exportGuestClient) RunScriptOutput(_ context.Context, script string) ([]byte, error) { + e.scripts = append(e.scripts, script) if e.callIndex >= len(e.responses) { return nil, nil } @@ -113,6 +115,107 @@ func TestExportVMWorkspace_HappyPath(t *testing.T) { if fake.callIndex != 2 { t.Fatalf("RunScriptOutput call count = %d, want 2", fake.callIndex) } + // No base_commit provided: diff ref must be HEAD. + for _, script := range fake.scripts { + if !strings.Contains(script, "HEAD") { + t.Fatalf("script missing HEAD ref: %q", script) + } + } + if result.BaseCommit != "HEAD" { + t.Fatalf("BaseCommit = %q, want HEAD", result.BaseCommit) + } +} + +func TestExportVMWorkspace_WithBaseCommit(t *testing.T) { + t.Parallel() + ctx := context.Background() + + apiSock := filepath.Join(t.TempDir(), "fc.sock") + firecracker := startFakeFirecracker(t, apiSock) + + vm := testVM("exportbox-base", "image-export", "172.16.0.105") + vm.State = model.VMStateRunning + vm.Runtime.State = model.VMStateRunning + vm.Runtime.PID = firecracker.Process.Pid + vm.Runtime.APISockPath = apiSock + + // Simulate: worker committed inside the VM. Without base_commit the diff + // against the new HEAD would be empty. With base_commit we capture + // everything since the original checkout. + patch := []byte("diff --git a/worker.go b/worker.go\nindex 0000000..abcdef 100644\n") + names := []byte("worker.go\n") + + fake := &exportGuestClient{ + responses: []exportGuestResponse{ + {output: patch}, + {output: names}, + }, + } + d := newExportTestDaemonStore(t, fake) + upsertDaemonVM(t, ctx, d.store, vm) + + const prepareCommit = "abc1234deadbeef" + result, err := d.ExportVMWorkspace(ctx, api.WorkspaceExportParams{ + IDOrName: vm.Name, + BaseCommit: prepareCommit, + }) + if err != nil { + t.Fatalf("ExportVMWorkspace: %v", err) + } + if !result.HasChanges { + t.Fatal("HasChanges = false, want true") + } + if result.BaseCommit != prepareCommit { + t.Fatalf("BaseCommit = %q, want %q", result.BaseCommit, prepareCommit) + } + // Both scripts must reference the caller-supplied commit, not HEAD. + for _, script := range fake.scripts { + if strings.Contains(script, " HEAD") { + t.Fatalf("script used HEAD instead of base_commit: %q", script) + } + if !strings.Contains(script, prepareCommit) { + t.Fatalf("script missing base_commit %q: %q", prepareCommit, script) + } + } +} + +func TestExportVMWorkspace_BaseCommitFallsBackToHEAD(t *testing.T) { + t.Parallel() + ctx := context.Background() + + apiSock := filepath.Join(t.TempDir(), "fc.sock") + firecracker := startFakeFirecracker(t, apiSock) + + vm := testVM("exportbox-nobase", "image-export", "172.16.0.106") + vm.State = model.VMStateRunning + vm.Runtime.State = model.VMStateRunning + vm.Runtime.PID = firecracker.Process.Pid + vm.Runtime.APISockPath = apiSock + + fake := &exportGuestClient{ + responses: []exportGuestResponse{ + {output: nil}, + {output: nil}, + }, + } + d := newExportTestDaemonStore(t, fake) + upsertDaemonVM(t, ctx, d.store, vm) + + result, err := d.ExportVMWorkspace(ctx, api.WorkspaceExportParams{ + IDOrName: vm.Name, + BaseCommit: "", // omitted + }) + if err != nil { + t.Fatalf("ExportVMWorkspace: %v", err) + } + if result.BaseCommit != "HEAD" { + t.Fatalf("BaseCommit = %q, want HEAD when not supplied", result.BaseCommit) + } + for _, script := range fake.scripts { + if !strings.Contains(script, "HEAD") { + t.Fatalf("script missing HEAD fallback: %q", script) + } + } } func TestExportVMWorkspace_NoChanges(t *testing.T) {