From bbd187391e0221ab1263c37ab53d07703e934e18 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Wed, 22 Apr 2026 12:42:33 -0300 Subject: [PATCH] noteUntrackedSkipped: fix subdir underreport + be best-effort everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in the untracked-skipped warning, both surfaced by review. 1. Wrong scope for subdir inputs. The helper accepted any path the caller had (sourcePath, which may be a user-supplied subdirectory) and ran `git -C ls-files --others --exclude-standard`. Git scopes that output to the cwd, so pointing `vm run ./repo/sub` at a subdir silently underreported untracked files living elsewhere in the repo — exactly the files the warning exists to surface. Fix: resolve sourcePath to the repo root inside the helper via `rev-parse --show-toplevel` before counting. 2. Inconsistent failure handling. The comment said the helper should be silent when the count can't be determined; the body returned the error. vm_run.go treated the error as non-fatal (logged a warning, continued); workspace prepare and --dry-run aborted the whole operation on the same helper failure. A courtesy notice shouldn't kill the operation. Fix: make the helper best-effort in signature and body — no error return, swallows rev-parse + count failures, emits nothing when there's nothing to say. All three callers lose their error branches. Regression tests: - subdir input reports the root-level untracked file (the bug case) - non-repo path produces silence, not a fatal error - inspector whose runner errors on every call produces silence Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cli/commands_vm.go | 4 +- internal/cli/vm_run.go | 4 +- internal/cli/workspace_preview.go | 36 +++++--- internal/cli/workspace_preview_test.go | 120 +++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 20 deletions(-) create mode 100644 internal/cli/workspace_preview_test.go diff --git a/internal/cli/commands_vm.go b/internal/cli/commands_vm.go index d4f010a..21618f7 100644 --- a/internal/cli/commands_vm.go +++ b/internal/cli/commands_vm.go @@ -625,9 +625,7 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command { return err } if !includeUntracked { - if err := d.noteUntrackedSkipped(cmd.Context(), cmd.ErrOrStderr(), resolvedPath); err != nil { - return err - } + d.noteUntrackedSkipped(cmd.Context(), cmd.ErrOrStderr(), resolvedPath) } result, err := d.vmWorkspacePrepare(cmd.Context(), layout.SocketPath, api.VMWorkspacePrepareParams{ IDOrName: args[0], diff --git a/internal/cli/vm_run.go b/internal/cli/vm_run.go index e758b86..e3a049f 100644 --- a/internal/cli/vm_run.go +++ b/internal/cli/vm_run.go @@ -195,9 +195,7 @@ func (d *deps) runVMRun(ctx context.Context, socketPath string, cfg model.Daemon fromRef = repo.fromRef } if !repo.includeUntracked { - if err := d.noteUntrackedSkipped(ctx, stderr, repo.sourcePath); err != nil { - printVMRunWarning(stderr, fmt.Sprintf("count untracked files failed: %v", err)) - } + d.noteUntrackedSkipped(ctx, stderr, repo.sourcePath) } prepared, err := d.vmWorkspacePrepare(ctx, socketPath, api.VMWorkspacePrepareParams{ IDOrName: vmRef, diff --git a/internal/cli/workspace_preview.go b/internal/cli/workspace_preview.go index 15528c9..956d6ea 100644 --- a/internal/cli/workspace_preview.go +++ b/internal/cli/workspace_preview.go @@ -29,25 +29,33 @@ func (d *deps) runWorkspaceDryRun(ctx context.Context, out io.Writer, resolvedPa fmt.Fprintln(out, path) } if !includeUntracked { - if err := d.noteUntrackedSkipped(ctx, out, spec.RepoRoot); err != nil { - return err - } + d.noteUntrackedSkipped(ctx, out, spec.RepoRoot) } return nil } -// noteUntrackedSkipped prints a one-line notice to stderr-analog when -// the repo has untracked non-ignored files that will NOT be copied -// because --include-untracked was not passed. Silent when there are -// no such files, or when the count can't be determined. -func (d *deps) noteUntrackedSkipped(ctx context.Context, out io.Writer, repoRoot string) error { - count, err := d.repoInspector.CountUntrackedPaths(ctx, repoRoot) - if err != nil { - return err +// noteUntrackedSkipped prints a one-line notice when the repo holds +// untracked non-ignored files that will NOT be copied because +// --include-untracked was not passed. +// +// Best-effort: if sourcePath isn't inside a git repo, or git errors, +// or there are no untracked files, the helper stays silent. The +// notice is a courtesy — failing the whole operation over a courtesy +// would be worse than the notice being missing. +// +// Resolves sourcePath to the repo root internally via `git rev-parse +// --show-toplevel` so callers can pass whatever path the user typed. +// Before this helper normalised, subdir inputs ran `ls-files +// --others` scoped to the subdir, which silently underreported the +// skipped files the user needed to know about. +func (d *deps) noteUntrackedSkipped(ctx context.Context, out io.Writer, sourcePath string) { + repoRoot, err := d.repoInspector.GitTrimmedOutput(ctx, sourcePath, "rev-parse", "--show-toplevel") + if err != nil || repoRoot == "" { + return } - if count == 0 { - return nil + count, err := d.repoInspector.CountUntrackedPaths(ctx, repoRoot) + if err != nil || count == 0 { + return } fmt.Fprintf(out, "---\nnote: %d untracked non-ignored file(s) were NOT copied (git-tracked files only by default — pass --include-untracked to include them)\n", count) - return nil } diff --git a/internal/cli/workspace_preview_test.go b/internal/cli/workspace_preview_test.go new file mode 100644 index 0000000..74cac66 --- /dev/null +++ b/internal/cli/workspace_preview_test.go @@ -0,0 +1,120 @@ +package cli + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "banger/internal/daemon/workspace" +) + +// seedRepoWithSubdir creates a git repo with one tracked file, and an +// untracked non-ignored file at the repo root (not under the subdir). +// Returns the repo root and the subdir path. +func seedRepoWithSubdir(t *testing.T) (repoRoot, subDir string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skipf("git not on PATH: %v", err) + } + repoRoot = t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = repoRoot + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t", + "GIT_CONFIG_GLOBAL=/dev/null", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v: %v\n%s", args, err, out) + } + } + writeFile := func(relPath, content string) { + t.Helper() + full := filepath.Join(repoRoot, relPath) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + run("git", "init", "-q", "-b", "main") + run("git", "config", "commit.gpgsign", "false") + writeFile("tracked.md", "hello\n") + writeFile("sub/kept.txt", "kept\n") + run("git", "add", ".") + run("git", "commit", "-q", "-m", "init") + // Untracked non-ignored file at the ROOT — not under sub/. This is + // what the pre-fix noteUntrackedSkipped would miss when the user + // passed sub/ as the workspace source. + writeFile("ROOT-SECRET.env", "TOKEN=abc\n") + subDir = filepath.Join(repoRoot, "sub") + return repoRoot, subDir +} + +// TestNoteUntrackedSkippedCountsRepoWideEvenFromSubdir pins the bug +// fix: when the user passes a subdirectory of a repo as the workspace +// source, the untracked-files notice must still reflect what will +// actually be skipped at the guest-shipping layer — which is a +// repo-wide concern. Before the fix the helper ran `git -C +// ls-files --others --exclude-standard`, which only sees files under +// the subdir, silently underreporting the real skip count. +func TestNoteUntrackedSkippedCountsRepoWideEvenFromSubdir(t *testing.T) { + repoRoot, subDir := seedRepoWithSubdir(t) + + d := defaultDeps() + d.repoInspector = workspace.NewInspector() + + var out bytes.Buffer + d.noteUntrackedSkipped(context.Background(), &out, subDir) + + got := out.String() + if !strings.Contains(got, "1 untracked") { + t.Fatalf("note = %q, want mention of 1 untracked file (the root-level SECRET.env)", got) + } + _ = repoRoot +} + +// TestNoteUntrackedSkippedSilentOutsideRepo verifies the best-effort +// contract: when sourcePath is not inside any git repo, the helper +// prints nothing and does not error. Callers rely on this so a user +// who points vm run at an ad-hoc directory (or an export tarball +// that's been unpacked) doesn't get the whole operation aborted +// over a courtesy notice. +func TestNoteUntrackedSkippedSilentOutsideRepo(t *testing.T) { + d := defaultDeps() + d.repoInspector = workspace.NewInspector() + + nonRepo := t.TempDir() + var out bytes.Buffer + d.noteUntrackedSkipped(context.Background(), &out, nonRepo) + + if got := out.String(); got != "" { + t.Fatalf("note = %q, want no output outside a git repo", got) + } +} + +// TestNoteUntrackedSkippedSwallowsInspectorErrors verifies that a +// runner that errors on every call produces no output and no panic. +// This is the other half of best-effort: even if git-the-binary is +// somehow broken or missing, the live flow keeps running. +func TestNoteUntrackedSkippedSwallowsInspectorErrors(t *testing.T) { + d := defaultDeps() + d.repoInspector = &workspace.Inspector{ + Runner: func(context.Context, string, ...string) ([]byte, error) { + return nil, &exec.Error{Name: "git", Err: exec.ErrNotFound} + }, + } + + var out bytes.Buffer + d.noteUntrackedSkipped(context.Background(), &out, t.TempDir()) + if got := out.String(); got != "" { + t.Fatalf("note = %q, want silence when inspector runner errors", got) + } +}