Improve testkit robustness and release readiness#1
Conversation
Harden snapshot restore path handling, simplify git output parsing with stdlib helpers, add CI checks, and expand README usage/requirements to improve maintainability and consumer confidence. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded CI workflow and developer documentation; downgraded module Go version; refactored git output parsing; introduced safe path handling and snapshot name normalization with tests; added USB fixture helpers and corresponding tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Improve README onboarding for test usage and add a developer guide covering testing, contribution expectations, and maintainer release workflow. Made-with: Cursor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Parse `git remote -v` output without truncating whitespace-containing URLs and add a regression test to lock in behavior for local remotes with spaced paths. Made-with: Cursor
Provide reusable helpers for creating .git-fire volume roots, reading/writing marker config, asserting bare/non-bare git destinations, and file URL conversion.
|
@cursor bugbot review |
|
Implemented proactive fixes for the bugbot review context and pushed them to the PR branch. What I changed
Validation
Git actions completed
I also attempted to update the PR body with the new scope, but PR body editing is restricted because it is currently non-agent-managed; branch and PR are updated with the new commit and ready for bugbot/CI re-evaluation.   |
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEVELOPER_GUIDE.md`:
- Around line 20-24: Update the clone example in DEVELOPER_GUIDE.md to use an
HTTPS URL (or show both HTTPS and SSH options) so first-time contributors
without SSH keys can clone easily; replace the existing "git clone
git@github.com:git-fire/git-testkit.git" line with an HTTPS variant "git clone
https://github.com/git-fire/git-testkit.git" (or include both lines labeled
"HTTPS" and "SSH") and keep the subsequent "cd git-testkit" and "go test ./..."
instructions unchanged.
In `@fixtures.go`:
- Around line 202-210: The parser that iterates over lines and uses
strings.Fields incorrectly splits remotes with spaces; update the loop that
builds remotes (the code using variables name, url and map remotes) to split
each non-empty line on the first tab only (e.g., strings.SplitN(line, "\t", 2)),
then extract name = fields[0] and raw = fields[1]; remove the trailing mode
suffix such as " (fetch)" or " (push)" with strings.TrimSuffix or by trimming
the last token (or strings.TrimSpace and strings.TrimSuffix for both modes) to
obtain url, and then assign remotes[name] = url. Ensure you skip lines that
don't contain a tab or lack a second field.
In `@snapshots.go`:
- Around line 97-100: Snapshot names are normalized in LoadSnapshotFromDisk but
not in SnapshotRepo, causing RestoreSnapshot's safeJoin(tmpDir, snapshot.name)
to reject names like "."; extract a shared normalizeSnapshotName(name string)
string (or similar) that applies the same rules used in LoadSnapshotFromDisk
(e.g., map "." and "/" to "snapshot" and otherwise sanitize via filepath.Base or
the current replacement logic) and call this function from both SnapshotRepo
(where name is set from filepath.Base(repoPath)) and LoadSnapshotFromDisk before
storing or returning snapshot.name so RestoreSnapshot and safeJoin always
receive a normalized, safe name.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1efe036b-9ddc-4f7e-8905-650be3e4df52
📒 Files selected for processing (7)
.github/workflows/ci.ymlDEVELOPER_GUIDE.mdREADME.mdfixtures.gogo.modsnapshots.gosnapshots_test.go
- DEVELOPER_GUIDE: document HTTPS clone first, SSH as optional
- GetRemotes: parse git remote -v by tab; strip (fetch)/(push) so URLs with spaces work
- snapshots: shared normalizeSnapshotName for SnapshotRepo and LoadSnapshotFromDisk
- Add regression test for SnapshotRepo(".") after chdir into repo
|
bugbot run |
Validate layout_dir stays relative under the fixture root, surface invalid schema_version when parsing .git-fire, and build RFC 8089-style file URLs on Windows drive-letter paths. Tighten FileURLForPath test. Made-with: Cursor
Extract validateFixtureLayoutDir and readUSBVolumeConfigBytes so reads reject bad layout_dir, invalid schema_version, and bad or empty created_at. Normalize non-empty layout_dir on read. Add table tests for validation and parse errors. Made-with: Cursor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
normalizeSnapshotNamemisses the".."edge case- normalizeSnapshotName now maps
".."(including inputs likefoo/..) to"snapshot", and a focused test was added to verify the behavior.
- normalizeSnapshotName now maps
Preview (d088c0835f)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
new file mode 100644
--- /dev/null
+++ b/.github/workflows/ci.yml
@@ -1,0 +1,30 @@
+name: CI
+
+on:
+ push:
+ branches:
+ - main
+ pull_request:
+
+jobs:
+ test:
+ runs-on: ubuntu-latest
+ strategy:
+ fail-fast: false
+ matrix:
+ go-version: ["1.22.x", "stable"]
+
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Setup Go
+ uses: actions/setup-go@v5
+ with:
+ go-version: ${{ matrix.go-version }}
+
+ - name: Run vet
+ run: go vet ./...
+
+ - name: Run tests
+ run: go test ./...
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
new file mode 100644
--- /dev/null
+++ b/DEVELOPER_GUIDE.md
@@ -1,0 +1,132 @@
+# Developer Guide
+
+This guide covers how to use `git-testkit`, contribute safely, run checks locally, and maintain releases.
+
+## Audience
+
+- **Users** writing tests that need real git repositories.
+- **Contributors** making code/docs changes.
+- **Maintainers/admins** preparing releases and keeping CI healthy.
+
+## Local setup
+
+Requirements:
+
+- Go 1.22+
+- `git` on `PATH`
+
+Clone and verify:
+
+```bash
+# HTTPS (no SSH keys required)
+git clone https://github.com/git-fire/git-testkit.git
+
+# or SSH
+# git clone git@github.com:git-fire/git-testkit.git
+
+cd git-testkit
+go test ./...
+```
+
+## Project structure
+
+- `fixtures.go`: base repo creation and command helpers.
+- `scenarios.go`: fluent scenario builder and predefined multi-repo scenarios.
+- `snapshots.go`: snapshot/restore utilities for expensive test setup reuse.
+- `fixtures_test.go`: external package tests (`testutil_test`) that validate public API usage.
+- `scenarios_test.go`: package-internal tests for scenario/snapshot behavior.
+
+## Design principles
+
+- Prefer real git commands over mocks for behavior confidence.
+- Keep helper APIs composable and minimal.
+- Fail fast in setup helpers (`t.Fatalf`) so fixture errors are obvious.
+- Keep tests isolated using `t.TempDir()`.
+
+## Testing and quality checks
+
+Run before opening a PR:
+
+```bash
+gofmt -w *.go
+go vet ./...
+go test ./...
+```
+
+Optional:
+
+```bash
+go test -short ./...
+```
+
+CI mirrors the required checks (`go vet` and `go test`) on pull requests.
+
+## Usage guidance for library consumers
+
+- Prefer scenario builders for integration-style tests with remotes/worktrees.
+- Prefer base fixtures for small unit/integration tests that only need one repo.
+- Keep assertions close to setup; helper failures already include command output.
+- Use snapshots for expensive setups that are reused across subtests.
+
+Common usage split:
+
+- `CreateTestRepo`: fast setup for single-repo state.
+- `NewScenario`: multi-repo topologies and fluent setup.
+- `SnapshotRepo`/`RestoreSnapshot`: performance optimization for repeated expensive setups.
+
+## Adding new helpers
+
+When adding new exported helpers:
+
+- Add or update tests in `fixtures_test.go` and/or `scenarios_test.go`.
+- Add usage notes/examples to `README.md` when the helper is user-facing.
+- Keep function names explicit and test-focused (avoid generic utility names).
+
+## Snapshot safety expectations
+
+- Snapshot restore must never write outside the restore root.
+- Avoid introducing behavior that can restore unsupported file types silently.
+- Keep snapshot behavior deterministic for repeatable tests.
+
+## Pull request guidance
+
+- Keep PRs focused and small when possible.
+- Include a clear "why" in the commit/PR description.
+- Include a short test plan with commands you ran locally.
+- Prefer additive, backward-compatible changes for existing exported helpers.
+
+Suggested PR checklist:
+
+- [ ] public API changes documented in `README.md`
+- [ ] tests added/updated for behavior changes
+- [ ] `gofmt`, `go vet`, and `go test` all pass locally
+- [ ] no unrelated refactors mixed into functional changes
+
+## Maintainer/admin workflow
+
+### Branch and PR flow
+
+1. Create a feature branch from `main`.
+2. Commit focused changes with clear commit messages.
+3. Open a PR with summary and test plan.
+4. Merge only after CI is green.
+
+### Release flow
+
+1. Verify `main` has passed CI.
+2. Confirm `README.md` and this guide reflect current API/behavior.
+3. Pull release notes from merged PRs (user-visible changes first).
+4. Create and push a version tag.
+5. Publish a GitHub release with:
+ - notable changes
+ - compatibility notes (for example, minimum Go version)
+ - migration notes when behavior changed
+
+## Release notes checklist
+
+Before cutting a release:
+
+- Ensure CI is green on `main`.
+- Summarize user-visible changes (new helpers, behavior changes, compatibility notes).
+- Call out any minimum Go version changes.
+- Verify examples in `README.md` still compile conceptually with current API.
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -2,18 +2,57 @@
`git-testkit` provides helpers for writing Go tests that exercise real Git repositories.
+## Why use this
+
+- Exercise real git behavior instead of mocking command output.
+- Build common repo states quickly (dirty trees, detached HEAD, diverged remotes, worktrees).
+- Reuse expensive setups across tests with in-memory snapshots.
+
## Install
```bash
go get github.com/git-fire/git-testkit+## Requirements
+
+- git must be installed and available on PATH
+- Go 1.22+
+
What it includes
- Repository fixtures (
CreateTestRepo,CreateBareRemote,RunGitCmd) - Scenario builders for common multi-repo states (
NewScenario, conflict/worktree helpers) - Snapshot helpers for capturing and restoring repository state in tests
+## API overview
+
+- CreateTestRepo(t, RepoOptions) creates a real repository with optional files/remotes/branches.
+- CreateBareRemote(t, name) creates a bare repository for remote testing.
+- NewScenario(t) returns a fluent builder for multi-repo test topologies.
+- SnapshotRepo(t, path) and RestoreSnapshot(t, snap) speed up repeated fixture setup.
+- IsDirty, GetCurrentSHA, GetBranches, GetRemotes provide common assertions/helpers.
+
+## Quickstart for using in tests
+
+1. Create fixture repos with CreateTestRepo or NewScenario.
+2. Apply setup operations with fluent helpers (AddFile, Commit, WithRemote, Push).
+3. Run your code under test against the real repository paths.
+4. Assert with helper methods (IsDirty, GetCurrentSHA, GetBranches).
+
+Minimal test flow:
+
+```go
+func TestMyGitBehavior(t *testing.T) {
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{Name: "subject"})
- testutil.RunGitCmd(t, repoPath, "checkout", "-b", "feature")
- // call your package functions here
- if testutil.IsDirty(t, repoPath) {
-
t.Fatal("repo should be clean") - }
+}
+```
Example
@@ -34,3 +73,50 @@
}
}+## Cleanup behavior
+
+- All helper-created repositories use t.TempDir().
+- Repos/worktrees are automatically removed by Go's test framework at test completion.
+- As with any temp directories, force-killed test processes may leave files behind.
+
+## Common patterns
+
+### Build a conflict scenario
+
+```go
+func TestConflictFlow(t *testing.T) {
- _, local, _ := testutil.CreateConflictScenario(t)
- // Exercise your logic against a real diverged local clone.
- testutil.RunGitCmd(t, local.Path(), "status")
+}
+```
+### Snapshot expensive setup
+
+```go
+func TestUsingSnapshot(t *testing.T) {
- _, repo := testutil.CreateLargeRepoScenario(t, 20, 10)
- snap := testutil.SnapshotRepo(t, repo.Path())
- clonePath := testutil.RestoreSnapshot(t, snap)
- // Use clonePath in assertions without rebuilding the fixture each time.
- testutil.RunGitCmd(t, clonePath, "status")
+}
+```
+## Notes
+
+- Snapshots are intended for deterministic test fixtures and only restore regular files/directories.
+- Helpers fail tests immediately (t.Fatalf) when git commands fail, so errors surface close to setup code.
+- When tests build large repo graphs repeatedly, prefer snapshot/restore to reduce total runtime.
+
+## Developer docs
+
+- See DEVELOPER_GUIDE.md for:
-
- testing and quality gates
-
- usage guidance for library consumers
-
- administration/maintenance and release workflow
-
- contribution process and PR expectations
diff --git a/fixtures.go b/fixtures.go
--- a/fixtures.go
+++ b/fixtures.go
@@ -4,6 +4,7 @@
"os"
"os/exec"
"path/filepath"
- "strings"
"testing"
)
@@ -193,71 +194,32 @@
// "origin /path/to/remote (push)"
remotes := make(map[string]string)
- lines := string(output)
- lines := strings.TrimSpace(string(output))
if lines == "" {
return remotes
}
- // Simple parsing - just extract remote names
- // Full parsing not needed for tests
- for _, line := range splitLines(lines) {
- for _, line := range strings.Split(lines, "\n") {
-
line = strings.TrimSpace(line) if line == "" { continue }
-
// Just check if "origin" appears in the line -
// Good enough for test validation -
if len(line) > 0 { -
// Extract first word (remote name) -
parts := splitWhitespace(line) -
if len(parts) >= 2 { -
name := parts[0] -
url := parts[1] -
remotes[name] = url -
}
-
name, rest, ok := strings.Cut(line, "\t") -
if !ok { -
continue } -
name = strings.TrimSpace(name) -
rest = strings.TrimSpace(rest) -
rest = strings.TrimSuffix(rest, " (fetch)") -
rest = strings.TrimSuffix(rest, " (push)") -
if name != "" && rest != "" { -
remotes[name] = rest -
}}
return remotes
}
-// Helper: split by newlines
-func splitLines(s string) []string {
- var lines []string
- current := ""
- for _, ch := range s {
-
if ch == '\n' { -
lines = append(lines, current) -
current = "" -
} else { -
current += string(ch) -
} - }
- if current != "" {
-
lines = append(lines, current) - }
- return lines
-}
-// Helper: split by whitespace/tabs
-func splitWhitespace(s string) []string {
- var parts []string
- current := ""
- for _, ch := range s {
-
if ch == ' ' || ch == '\t' { -
if current != "" { -
parts = append(parts, current) -
current = "" -
} -
} else { -
current += string(ch) -
} - }
- if current != "" {
-
parts = append(parts, current) - }
- return parts
-}
// RunGitCmd runs a git command and fails the test if it errors
// Exported version of runGit for use in other test packages
func RunGitCmd(t *testing.T, dir string, args ...string) {
@@ -277,13 +239,7 @@
t.Fatalf("Failed to get current SHA: %v", err)
}
- sha := string(output)
- // Trim newline
- if len(sha) > 0 && sha[len(sha)-1] == '\n' {
-
sha = sha[:len(sha)-1] - }
- return sha
- return strings.TrimSpace(string(output))
}
// GetBranches returns all branches in the repo
@@ -298,7 +254,7 @@
t.Fatalf("Failed to get branches: %v", err)
}
- branches := splitLines(string(output))
-
branches := strings.Split(strings.TrimSpace(string(output)), "\n")
// Filter out empty lines
var result []string
diff --git a/go.mod b/go.mod
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
module github.com/git-fire/git-testkit
-go 1.24.2
+go 1.22
diff --git a/snapshots.go b/snapshots.go
--- a/snapshots.go
+++ b/snapshots.go
@@ -11,6 +11,14 @@
"testing"
)
+func normalizeSnapshotName(path string) string {
- name := filepath.Base(path)
- if name == "." || name == ".." || name == string(filepath.Separator) {
-
return "snapshot" - }
- return name
+}
// Snapshot represents a saved state of a git repository
type Snapshot struct {
name string
@@ -43,6 +51,9 @@
if err != nil {
return fmt.Errorf("failed to get relative path: %w", err)
}
-
if relPath == "." { -
return nil -
} header.Name = relPath // Write header
@@ -79,7 +90,7 @@
}
return &Snapshot{
-
name: filepath.Base(repoPath),
-
name: normalizeSnapshotName(repoPath), tarball: buf.Bytes(),}
}
@@ -91,7 +102,10 @@// Create temp directory for restoration
tmpDir := t.TempDir()
- restorePath := filepath.Join(tmpDir, snapshot.name)
-
restorePath, err := safeJoin(tmpDir, snapshot.name)
-
if err != nil {
-
t.Fatalf("Invalid snapshot name %q: %v", snapshot.name, err) -
}
if err := os.MkdirAll(restorePath, 0755); err != nil {
t.Fatalf("Failed to create restore directory: %v", err)
@@ -117,7 +131,10 @@
}// Construct full path
-
targetPath := filepath.Join(restorePath, header.Name)
-
targetPath, err := safeJoin(restorePath, header.Name) -
if err != nil { -
t.Fatalf("Invalid snapshot path %q: %v", header.Name, err) -
} // Handle different file types switch header.Typeflag {
@@ -154,6 +171,30 @@
return restorePath
}
+func safeJoin(base, name string) (string, error) {
- cleanName := filepath.Clean(name)
- if cleanName == "." || cleanName == string(filepath.Separator) {
-
return "", fmt.Errorf("empty or root path is not allowed") - }
- if filepath.IsAbs(cleanName) {
-
return "", fmt.Errorf("absolute paths are not allowed") - }
- if cleanName == ".." || len(cleanName) >= 3 && cleanName[:3] == ".."+string(filepath.Separator) {
-
return "", fmt.Errorf("path traversal is not allowed") - }
- target := filepath.Join(base, cleanName)
- rel, err := filepath.Rel(base, target)
- if err != nil {
-
return "", err - }
- if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) {
-
return "", fmt.Errorf("resolved path escapes restore directory") - }
- return target, nil
+}
// SnapshotSize returns the size of the snapshot in bytes
func (s *Snapshot) Size() int {
return len(s.tarball)
@@ -174,16 +215,16 @@
}
// LoadSnapshotFromDisk loads a snapshot from a file
-func LoadSnapshotFromDisk(t *testing.T, filepath string) *Snapshot {
+func LoadSnapshotFromDisk(t *testing.T, filePath string) *Snapshot {
t.Helper()
- data, err := os.ReadFile(filepath)
-
data, err := os.ReadFile(filePath)
if err != nil {
t.Fatalf("Failed to load snapshot from disk: %v", err)
}return &Snapshot{
-
name: filepath,
-
}
name: normalizeSnapshotName(filePath), tarball: data,
}
diff --git a/snapshots_test.go b/snapshots_test.go
new file mode 100644
--- /dev/null
+++ b/snapshots_test.go
@@ -1,0 +1,118 @@
+package testutil
+
+import (
- "os"
- "path/filepath"
- "testing"
+)
+func TestRestoreSnapshotRejectsUnsafeSnapshotNames(t *testing.T) {
- tests := []struct {
-
name string -
base string -
joinArg string - }{
-
{ -
name: "path traversal name", -
base: filepath.Join("tmp", "root"), -
joinArg: "../escape", -
}, -
{ -
name: "absolute name", -
base: filepath.Join("tmp", "root"), -
joinArg: string(filepath.Separator) + "tmp/escape", -
}, - }
- for _, tt := range tests {
-
tt := tt -
t.Run(tt.name, func(t *testing.T) { -
if _, err := safeJoin(tt.base, tt.joinArg); err == nil { -
t.Fatalf("expected safeJoin to reject %q", tt.joinArg) -
} -
}) - }
+}
+func TestRestoreSnapshotRejectsUnsafeArchivePaths(t *testing.T) {
- tests := []struct {
-
name string -
base string -
joinArg string - }{
-
{ -
name: "relative traversal path", -
base: filepath.Join("tmp", "root"), -
joinArg: "../escape.txt", -
}, -
{ -
name: "absolute path", -
base: filepath.Join("tmp", "root"), -
joinArg: string(filepath.Separator) + "tmp/escape.txt", -
}, - }
- for _, tt := range tests {
-
tt := tt -
t.Run(tt.name, func(t *testing.T) { -
if _, err := safeJoin(tt.base, tt.joinArg); err == nil { -
t.Fatalf("expected safeJoin to reject %q", tt.joinArg) -
} -
}) - }
+}
+func TestSnapshotRepoNormalizesTrailingDotPath(t *testing.T) {
- _, repo := CreateCleanRepoScenario(t)
- oldWd, err := os.Getwd()
- if err != nil {
-
t.Fatal(err) - }
- t.Cleanup(func() { _ = os.Chdir(oldWd) })
- if err := os.Chdir(repo.path); err != nil {
-
t.Fatal(err) - }
- snap := SnapshotRepo(t, ".")
- if got, want := snap.Name(), "snapshot"; got != want {
-
t.Fatalf("expected snapshot name %q, got %q", want, got) - }
- _ = RestoreSnapshot(t, snap)
+}
+func TestNormalizeSnapshotNameHandlesDotDot(t *testing.T) {
- tests := []struct {
-
input string -
want string - }{
-
{input: "..", want: "snapshot"}, -
{input: filepath.Join("foo", ".."), want: "snapshot"}, - }
- for _, tt := range tests {
-
if got := normalizeSnapshotName(tt.input); got != tt.want { -
t.Fatalf("normalizeSnapshotName(%q) = %q, want %q", tt.input, got, tt.want) -
} - }
+}
+func TestLoadSnapshotFromDiskUsesBaseName(t *testing.T) {
- _, repo := CreateCleanRepoScenario(t)
- snapshot := SnapshotRepo(t, repo.path)
- snapshotPath := filepath.Join(t.TempDir(), "nested", "snapshot.tar.gz")
- if err := os.MkdirAll(filepath.Dir(snapshotPath), 0755); err != nil {
-
t.Fatalf("failed to create snapshot dir: %v", err) - }
- SaveSnapshotToDisk(t, snapshot, snapshotPath)
- loaded := LoadSnapshotFromDisk(t, snapshotPath)
- if got, want := loaded.Name(), filepath.Base(snapshotPath); got != want {
-
t.Fatalf("expected loaded snapshot name %q, got %q", want, got) - }
- restoredPath := RestoreSnapshot(t, loaded)
- if got, want := filepath.Base(restoredPath), loaded.Name(); got != want {
-
t.Fatalf("expected restore dir base %q, got %q", want, got) - }
+}
</details>
<sub>You can send follow-ups to the cloud agent <a href="https://cursor.com/agents/bc-f43618ee-6c24-4588-a599-50bfb664c8bf">here</a>.</sub>
</details>
…aths Resolve GetRemotes conflict: keep tab-or-space parsing and (fetch)/(push) trimming for paths with spaces; trim remote names and URLs consistently. Made-with: Cursor
Fix GetRemotes parsing for URLs with spaces
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
usb_fixtures.go (1)
16-27: Alignlayout_dirvalidation semantics with snapshot path hardening.This validator currently allows
".", while the snapshot-sidesafeJoinpolicy rejects root-like/empty targets. Keeping both checks aligned will reduce security-rule drift and avoid ambiguous “root layout” behavior.♻️ Minimal alignment change
func validateFixtureLayoutDir(layoutDir string) error { if layoutDir == "" { return nil } clean := filepath.Clean(layoutDir) + if clean == "." || clean == string(filepath.Separator) { + return fmt.Errorf("must be relative to fixture root: %q", layoutDir) + } if filepath.IsAbs(clean) { return fmt.Errorf("must be relative to fixture root: %q", layoutDir) } if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { return fmt.Errorf("must be relative to fixture root: %q", layoutDir) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usb_fixtures.go` around lines 16 - 27, The validator validateFixtureLayoutDir currently returns nil for an empty layoutDir and allows "." but snapshot safeJoin rejects root-like/empty targets; change validateFixtureLayoutDir so that an empty string or a cleaned value of "." (and root-like variants like ".." handled already) are treated as invalid and return the same error ("must be relative to fixture root: %q", layoutDir). Concretely, remove the early return for layoutDir == "" and add a check that if clean == "." or clean == "" (or matches "."+Sep variants) then return the error; keep the existing absolute and parent-dir checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usb_fixtures.go`:
- Around line 114-120: The parser in readUSBVolumeConfigBytes accepts zero or
negative schema_version values; update the "schema_version" case (where
cfg.SchemaVersion is set) to validate that the parsed integer n is > 0 and
return an error if not (e.g., treat 0 or negative as invalid with a descriptive
fmt.Errorf), so invalid configs are rejected rather than accepted.
---
Nitpick comments:
In `@usb_fixtures.go`:
- Around line 16-27: The validator validateFixtureLayoutDir currently returns
nil for an empty layoutDir and allows "." but snapshot safeJoin rejects
root-like/empty targets; change validateFixtureLayoutDir so that an empty string
or a cleaned value of "." (and root-like variants like ".." handled already) are
treated as invalid and return the same error ("must be relative to fixture root:
%q", layoutDir). Concretely, remove the early return for layoutDir == "" and add
a check that if clean == "." or clean == "" (or matches "."+Sep variants) then
return the error; keep the existing absolute and parent-dir checks unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c18d7cc8-c680-4098-850e-83073b2446a7
📒 Files selected for processing (4)
fixtures.gofixtures_test.gousb_fixtures.gousb_fixtures_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- fixtures.go
| case "schema_version": | ||
| n, err := strconv.Atoi(val) | ||
| if err != nil { | ||
| return cfg, fmt.Errorf("invalid schema_version %q: %w", val, err) | ||
| } | ||
| cfg.SchemaVersion = n | ||
| case "layout_dir": |
There was a problem hiding this comment.
Reject non-positive schema_version during parse.
readUSBVolumeConfigBytes currently accepts 0 and negative versions. That allows invalid config to parse as “valid” and weakens corruption detection.
🔧 Proposed fix
case "schema_version":
n, err := strconv.Atoi(val)
if err != nil {
return cfg, fmt.Errorf("invalid schema_version %q: %w", val, err)
}
+ if n <= 0 {
+ return cfg, fmt.Errorf("invalid schema_version %q: must be > 0", val)
+ }
cfg.SchemaVersion = n📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "schema_version": | |
| n, err := strconv.Atoi(val) | |
| if err != nil { | |
| return cfg, fmt.Errorf("invalid schema_version %q: %w", val, err) | |
| } | |
| cfg.SchemaVersion = n | |
| case "layout_dir": | |
| case "schema_version": | |
| n, err := strconv.Atoi(val) | |
| if err != nil { | |
| return cfg, fmt.Errorf("invalid schema_version %q: %w", val, err) | |
| } | |
| if n <= 0 { | |
| return cfg, fmt.Errorf("invalid schema_version %q: must be > 0", val) | |
| } | |
| cfg.SchemaVersion = n | |
| case "layout_dir": |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usb_fixtures.go` around lines 114 - 120, The parser in
readUSBVolumeConfigBytes accepts zero or negative schema_version values; update
the "schema_version" case (where cfg.SchemaVersion is set) to validate that the
parsed integer n is > 0 and return an error if not (e.g., treat 0 or negative as
invalid with a descriptive fmt.Errorf), so invalid configs are rejected rather
than accepted.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Missing parentheses in security-sensitive path traversal check
- Replaced both precedence-reliant traversal checks in
safeJoinwith explicitstrings.HasPrefixlogic matching the safer pattern used elsewhere.
- Replaced both precedence-reliant traversal checks in
- ✅ Fixed: Asymmetric encode/decode pair corrupts special-character values
- Made decoding symmetric with
%qoutput by unquoting string fields viastrconv.Unquote, preserving escaped backslashes and quotes on round-trip.
- Made decoding symmetric with
Preview (d0c6cb2998)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
new file mode 100644
--- /dev/null
+++ b/.github/workflows/ci.yml
@@ -1,0 +1,30 @@
+name: CI
+
+on:
+ push:
+ branches:
+ - main
+ pull_request:
+
+jobs:
+ test:
+ runs-on: ubuntu-latest
+ strategy:
+ fail-fast: false
+ matrix:
+ go-version: ["1.22.x", "stable"]
+
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Setup Go
+ uses: actions/setup-go@v5
+ with:
+ go-version: ${{ matrix.go-version }}
+
+ - name: Run vet
+ run: go vet ./...
+
+ - name: Run tests
+ run: go test ./...
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
new file mode 100644
--- /dev/null
+++ b/DEVELOPER_GUIDE.md
@@ -1,0 +1,132 @@
+# Developer Guide
+
+This guide covers how to use `git-testkit`, contribute safely, run checks locally, and maintain releases.
+
+## Audience
+
+- **Users** writing tests that need real git repositories.
+- **Contributors** making code/docs changes.
+- **Maintainers/admins** preparing releases and keeping CI healthy.
+
+## Local setup
+
+Requirements:
+
+- Go 1.22+
+- `git` on `PATH`
+
+Clone and verify:
+
+```bash
+# HTTPS (no SSH keys required)
+git clone https://github.com/git-fire/git-testkit.git
+
+# or SSH
+# git clone git@github.com:git-fire/git-testkit.git
+
+cd git-testkit
+go test ./...
+```
+
+## Project structure
+
+- `fixtures.go`: base repo creation and command helpers.
+- `scenarios.go`: fluent scenario builder and predefined multi-repo scenarios.
+- `snapshots.go`: snapshot/restore utilities for expensive test setup reuse.
+- `fixtures_test.go`: external package tests (`testutil_test`) that validate public API usage.
+- `scenarios_test.go`: package-internal tests for scenario/snapshot behavior.
+
+## Design principles
+
+- Prefer real git commands over mocks for behavior confidence.
+- Keep helper APIs composable and minimal.
+- Fail fast in setup helpers (`t.Fatalf`) so fixture errors are obvious.
+- Keep tests isolated using `t.TempDir()`.
+
+## Testing and quality checks
+
+Run before opening a PR:
+
+```bash
+gofmt -w *.go
+go vet ./...
+go test ./...
+```
+
+Optional:
+
+```bash
+go test -short ./...
+```
+
+CI mirrors the required checks (`go vet` and `go test`) on pull requests.
+
+## Usage guidance for library consumers
+
+- Prefer scenario builders for integration-style tests with remotes/worktrees.
+- Prefer base fixtures for small unit/integration tests that only need one repo.
+- Keep assertions close to setup; helper failures already include command output.
+- Use snapshots for expensive setups that are reused across subtests.
+
+Common usage split:
+
+- `CreateTestRepo`: fast setup for single-repo state.
+- `NewScenario`: multi-repo topologies and fluent setup.
+- `SnapshotRepo`/`RestoreSnapshot`: performance optimization for repeated expensive setups.
+
+## Adding new helpers
+
+When adding new exported helpers:
+
+- Add or update tests in `fixtures_test.go` and/or `scenarios_test.go`.
+- Add usage notes/examples to `README.md` when the helper is user-facing.
+- Keep function names explicit and test-focused (avoid generic utility names).
+
+## Snapshot safety expectations
+
+- Snapshot restore must never write outside the restore root.
+- Avoid introducing behavior that can restore unsupported file types silently.
+- Keep snapshot behavior deterministic for repeatable tests.
+
+## Pull request guidance
+
+- Keep PRs focused and small when possible.
+- Include a clear "why" in the commit/PR description.
+- Include a short test plan with commands you ran locally.
+- Prefer additive, backward-compatible changes for existing exported helpers.
+
+Suggested PR checklist:
+
+- [ ] public API changes documented in `README.md`
+- [ ] tests added/updated for behavior changes
+- [ ] `gofmt`, `go vet`, and `go test` all pass locally
+- [ ] no unrelated refactors mixed into functional changes
+
+## Maintainer/admin workflow
+
+### Branch and PR flow
+
+1. Create a feature branch from `main`.
+2. Commit focused changes with clear commit messages.
+3. Open a PR with summary and test plan.
+4. Merge only after CI is green.
+
+### Release flow
+
+1. Verify `main` has passed CI.
+2. Confirm `README.md` and this guide reflect current API/behavior.
+3. Pull release notes from merged PRs (user-visible changes first).
+4. Create and push a version tag.
+5. Publish a GitHub release with:
+ - notable changes
+ - compatibility notes (for example, minimum Go version)
+ - migration notes when behavior changed
+
+## Release notes checklist
+
+Before cutting a release:
+
+- Ensure CI is green on `main`.
+- Summarize user-visible changes (new helpers, behavior changes, compatibility notes).
+- Call out any minimum Go version changes.
+- Verify examples in `README.md` still compile conceptually with current API.
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -2,18 +2,57 @@
`git-testkit` provides helpers for writing Go tests that exercise real Git repositories.
+## Why use this
+
+- Exercise real git behavior instead of mocking command output.
+- Build common repo states quickly (dirty trees, detached HEAD, diverged remotes, worktrees).
+- Reuse expensive setups across tests with in-memory snapshots.
+
## Install
```bash
go get github.com/git-fire/git-testkit+## Requirements
+
+- git must be installed and available on PATH
+- Go 1.22+
+
What it includes
- Repository fixtures (
CreateTestRepo,CreateBareRemote,RunGitCmd) - Scenario builders for common multi-repo states (
NewScenario, conflict/worktree helpers) - Snapshot helpers for capturing and restoring repository state in tests
+## API overview
+
+- CreateTestRepo(t, RepoOptions) creates a real repository with optional files/remotes/branches.
+- CreateBareRemote(t, name) creates a bare repository for remote testing.
+- NewScenario(t) returns a fluent builder for multi-repo test topologies.
+- SnapshotRepo(t, path) and RestoreSnapshot(t, snap) speed up repeated fixture setup.
+- IsDirty, GetCurrentSHA, GetBranches, GetRemotes provide common assertions/helpers.
+
+## Quickstart for using in tests
+
+1. Create fixture repos with CreateTestRepo or NewScenario.
+2. Apply setup operations with fluent helpers (AddFile, Commit, WithRemote, Push).
+3. Run your code under test against the real repository paths.
+4. Assert with helper methods (IsDirty, GetCurrentSHA, GetBranches).
+
+Minimal test flow:
+
+```go
+func TestMyGitBehavior(t *testing.T) {
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{Name: "subject"})
- testutil.RunGitCmd(t, repoPath, "checkout", "-b", "feature")
- // call your package functions here
- if testutil.IsDirty(t, repoPath) {
-
t.Fatal("repo should be clean") - }
+}
+```
Example
@@ -34,3 +73,50 @@
}
}+## Cleanup behavior
+
+- All helper-created repositories use t.TempDir().
+- Repos/worktrees are automatically removed by Go's test framework at test completion.
+- As with any temp directories, force-killed test processes may leave files behind.
+
+## Common patterns
+
+### Build a conflict scenario
+
+```go
+func TestConflictFlow(t *testing.T) {
- _, local, _ := testutil.CreateConflictScenario(t)
- // Exercise your logic against a real diverged local clone.
- testutil.RunGitCmd(t, local.Path(), "status")
+}
+```
+### Snapshot expensive setup
+
+```go
+func TestUsingSnapshot(t *testing.T) {
- _, repo := testutil.CreateLargeRepoScenario(t, 20, 10)
- snap := testutil.SnapshotRepo(t, repo.Path())
- clonePath := testutil.RestoreSnapshot(t, snap)
- // Use clonePath in assertions without rebuilding the fixture each time.
- testutil.RunGitCmd(t, clonePath, "status")
+}
+```
+## Notes
+
+- Snapshots are intended for deterministic test fixtures and only restore regular files/directories.
+- Helpers fail tests immediately (t.Fatalf) when git commands fail, so errors surface close to setup code.
+- When tests build large repo graphs repeatedly, prefer snapshot/restore to reduce total runtime.
+
+## Developer docs
+
+- See DEVELOPER_GUIDE.md for:
-
- testing and quality gates
-
- usage guidance for library consumers
-
- administration/maintenance and release workflow
-
- contribution process and PR expectations
diff --git a/fixtures.go b/fixtures.go
--- a/fixtures.go
+++ b/fixtures.go
@@ -4,6 +4,7 @@
"os"
"os/exec"
"path/filepath"
- "strings"
"testing"
)
@@ -193,69 +194,39 @@
// "origin /path/to/remote (push)"
remotes := make(map[string]string)
- lines := string(output)
- lines := strings.TrimSpace(string(output))
if lines == "" {
return remotes
}
- // Simple parsing - just extract remote names
- // Full parsing not needed for tests
- for _, line := range splitLines(lines) {
- for _, line := range strings.Split(lines, "\n") {
-
line = strings.TrimSpace(line) if line == "" { continue }
-
// Just check if "origin" appears in the line -
// Good enough for test validation -
if len(line) > 0 { -
// Extract first word (remote name) -
parts := splitWhitespace(line) -
if len(parts) >= 2 { -
name := parts[0] -
url := parts[1] -
remotes[name] = url
-
name, remainder, ok := strings.Cut(line, "\t") -
if !ok { -
// Fallback for unusual formatting that does not use tabs. -
idx := strings.IndexAny(line, " \t") -
if idx == -1 { -
continue } -
name = strings.TrimSpace(line[:idx]) -
remainder = strings.TrimSpace(line[idx+1:]) -
} else { -
name = strings.TrimSpace(name) -
remainder = strings.TrimSpace(remainder) }
-
}
-
return remotes
-}
-
remainder = strings.TrimSuffix(remainder, " (fetch)") -
remainder = strings.TrimSuffix(remainder, " (push)")
-// Helper: split by newlines
-func splitLines(s string) []string {
- var lines []string
- current := ""
- for _, ch := range s {
-
if ch == '\n' { -
lines = append(lines, current) -
current = "" -
} else { -
current += string(ch)
-
if name != "" && remainder != "" { -
}
remotes[name] = remainder }
- if current != "" {
-
lines = append(lines, current) - }
- return lines
-}
-// Helper: split by whitespace/tabs
-func splitWhitespace(s string) []string {
- var parts []string
- current := ""
- for _, ch := range s {
-
if ch == ' ' || ch == '\t' { -
if current != "" { -
parts = append(parts, current) -
current = "" -
} -
} else { -
current += string(ch) -
} - }
- if current != "" {
-
parts = append(parts, current) - }
- return parts
- return remotes
}
// RunGitCmd runs a git command and fails the test if it errors
@@ -277,13 +248,7 @@
t.Fatalf("Failed to get current SHA: %v", err)
}
- sha := string(output)
- // Trim newline
- if len(sha) > 0 && sha[len(sha)-1] == '\n' {
-
sha = sha[:len(sha)-1] - }
- return sha
- return strings.TrimSpace(string(output))
}
// GetBranches returns all branches in the repo
@@ -298,7 +263,7 @@
t.Fatalf("Failed to get branches: %v", err)
}
- branches := splitLines(string(output))
-
branches := strings.Split(strings.TrimSpace(string(output)), "\n")
// Filter out empty lines
var result []string
diff --git a/fixtures_test.go b/fixtures_test.go
--- a/fixtures_test.go
+++ b/fixtures_test.go
@@ -79,6 +79,26 @@
}
}
+func TestCreateTestRepo_WithRemotePathContainingSpaces(t *testing.T) {
- remotePath := testutil.CreateBareRemote(t, "origin with space")
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{
-
Name: "remote-space-repo", -
Remotes: map[string]string{ -
"origin": remotePath, -
}, - })
- remotes := testutil.GetRemotes(t, repoPath)
- originURL, exists := remotes["origin"]
- if !exists {
-
t.Fatal("Expected 'origin' remote to be configured") - }
- if originURL != remotePath {
-
t.Fatalf("Expected origin URL %q, got %q", remotePath, originURL) - }
+}
func TestCreateBareRemote(t *testing.T) {
remotePath := testutil.CreateBareRemote(t, "test-remote")
diff --git a/go.mod b/go.mod
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
module github.com/git-fire/git-testkit
-go 1.24.2
+go 1.22
diff --git a/snapshots.go b/snapshots.go
--- a/snapshots.go
+++ b/snapshots.go
@@ -8,9 +8,18 @@
"io"
"os"
"path/filepath"
- "strings"
"testing"
)
+func normalizeSnapshotName(path string) string {
- name := filepath.Base(path)
- if name == "." || name == ".." || name == string(filepath.Separator) {
-
return "snapshot" - }
- return name
+}
// Snapshot represents a saved state of a git repository
type Snapshot struct {
name string
@@ -43,6 +52,9 @@
if err != nil {
return fmt.Errorf("failed to get relative path: %w", err)
}
-
if relPath == "." { -
return nil -
} header.Name = relPath // Write header
@@ -79,7 +91,7 @@
}
return &Snapshot{
-
name: filepath.Base(repoPath),
-
name: normalizeSnapshotName(repoPath), tarball: buf.Bytes(),}
}
@@ -91,7 +103,10 @@// Create temp directory for restoration
tmpDir := t.TempDir()
- restorePath := filepath.Join(tmpDir, snapshot.name)
-
restorePath, err := safeJoin(tmpDir, snapshot.name)
-
if err != nil {
-
t.Fatalf("Invalid snapshot name %q: %v", snapshot.name, err) -
}
if err := os.MkdirAll(restorePath, 0755); err != nil {
t.Fatalf("Failed to create restore directory: %v", err)
@@ -117,7 +132,10 @@
}// Construct full path
-
targetPath := filepath.Join(restorePath, header.Name)
-
targetPath, err := safeJoin(restorePath, header.Name) -
if err != nil { -
t.Fatalf("Invalid snapshot path %q: %v", header.Name, err) -
} // Handle different file types switch header.Typeflag {
@@ -154,6 +172,30 @@
return restorePath
}
+func safeJoin(base, name string) (string, error) {
- cleanName := filepath.Clean(name)
- if cleanName == "." || cleanName == string(filepath.Separator) {
-
return "", fmt.Errorf("empty or root path is not allowed") - }
- if filepath.IsAbs(cleanName) {
-
return "", fmt.Errorf("absolute paths are not allowed") - }
- if cleanName == ".." || strings.HasPrefix(cleanName, ".."+string(filepath.Separator)) {
-
return "", fmt.Errorf("path traversal is not allowed") - }
- target := filepath.Join(base, cleanName)
- rel, err := filepath.Rel(base, target)
- if err != nil {
-
return "", err - }
- if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
-
return "", fmt.Errorf("resolved path escapes restore directory") - }
- return target, nil
+}
// SnapshotSize returns the size of the snapshot in bytes
func (s *Snapshot) Size() int {
return len(s.tarball)
@@ -174,16 +216,16 @@
}
// LoadSnapshotFromDisk loads a snapshot from a file
-func LoadSnapshotFromDisk(t *testing.T, filepath string) *Snapshot {
+func LoadSnapshotFromDisk(t *testing.T, filePath string) *Snapshot {
t.Helper()
- data, err := os.ReadFile(filepath)
-
data, err := os.ReadFile(filePath)
if err != nil {
t.Fatalf("Failed to load snapshot from disk: %v", err)
}return &Snapshot{
-
name: filepath,
-
}
name: normalizeSnapshotName(filePath), tarball: data,
}
diff --git a/snapshots_test.go b/snapshots_test.go
new file mode 100644
--- /dev/null
+++ b/snapshots_test.go
@@ -1,0 +1,118 @@
+package testutil
+
+import (
- "os"
- "path/filepath"
- "testing"
+)
+func TestRestoreSnapshotRejectsUnsafeSnapshotNames(t *testing.T) {
- tests := []struct {
-
name string -
base string -
joinArg string - }{
-
{ -
name: "path traversal name", -
base: filepath.Join("tmp", "root"), -
joinArg: "../escape", -
}, -
{ -
name: "absolute name", -
base: filepath.Join("tmp", "root"), -
joinArg: string(filepath.Separator) + "tmp/escape", -
}, - }
- for _, tt := range tests {
-
tt := tt -
t.Run(tt.name, func(t *testing.T) { -
if _, err := safeJoin(tt.base, tt.joinArg); err == nil { -
t.Fatalf("expected safeJoin to reject %q", tt.joinArg) -
} -
}) - }
+}
+func TestRestoreSnapshotRejectsUnsafeArchivePaths(t *testing.T) {
- tests := []struct {
-
name string -
base string -
joinArg string - }{
-
{ -
name: "relative traversal path", -
base: filepath.Join("tmp", "root"), -
joinArg: "../escape.txt", -
}, -
{ -
name: "absolute path", -
base: filepath.Join("tmp", "root"), -
joinArg: string(filepath.Separator) + "tmp/escape.txt", -
}, - }
- for _, tt := range tests {
-
tt := tt -
t.Run(tt.name, func(t *testing.T) { -
if _, err := safeJoin(tt.base, tt.joinArg); err == nil { -
t.Fatalf("expected safeJoin to reject %q", tt.joinArg) -
} -
}) - }
+}
+func TestSnapshotRepoNormalizesTrailingDotPath(t *testing.T) {
- _, repo := CreateCleanRepoScenario(t)
- oldWd, err := os.Getwd()
- if err != nil {
-
t.Fatal(err) - }
- t.Cleanup(func() { _ = os.Chdir(oldWd) })
- if err := os.Chdir(repo.path); err != nil {
-
t.Fatal(err) - }
- snap := SnapshotRepo(t, ".")
- if got, want := snap.Name(), "snapshot"; got != want {
-
t.Fatalf("expected snapshot name %q, got %q", want, got) - }
- _ = RestoreSnapshot(t, snap)
+}
+func TestNormalizeSnapshotNameHandlesDotDot(t *testing.T) {
- tests := []struct {
-
input string -
want string - }{
-
{input: "..", want: "snapshot"}, -
{input: filepath.Join("foo", ".."), want: "snapshot"}, - }
- for _, tt := range tests {
-
if got := normalizeSnapshotName(tt.input); got != tt.want { -
t.Fatalf("normalizeSnapshotName(%q) = %q, want %q", tt.input, got, tt.want) -
} - }
+}
+func TestLoadSnapshotFromDiskUsesBaseName(t *testing.T) {
- _, repo := CreateCleanRepoScenario(t)
- snapshot := SnapshotRepo(t, repo.path)
- snapshotPath := filepath.Join(t.TempDir(), "nested", "snapshot.tar.gz")
- if err := os.MkdirAll(filepath.Dir(snapshotPath), 0755); err != nil {
-
t.Fatalf("failed to create snapshot dir: %v", err) - }
- SaveSnapshotToDisk(t, snapshot, snapshotPath)
- loaded := LoadSnapshotFromDisk(t, snapshotPath)
- if got, want := loaded.Name(), filepath.Base(snapshotPath); got != want {
-
t.Fatalf("expected loaded snapshot name %q, got %q", want, got) - }
- restoredPath := RestoreSnapshot(t, loaded)
- if got, want := filepath.Base(restoredPath), loaded.Name(); got != want {
-
t.Fatalf("expected restore dir base %q, got %q", want, got) - }
+}
diff --git a/usb_fixtures.go b/usb_fixtures.go
new file mode 100644
--- /dev/null
+++ b/usb_fixtures.go
@@ -1,0 +1,192 @@
+package testutil
+
+import (
- "fmt"
- "net/url"
- "os"
- "path/filepath"
- "strconv"
- "strings"
- "testing"
- "time"
+)
+// validateFixtureLayoutDir reports whether layoutDir may be joined under a fixture root.
+// Empty layoutDir is allowed (caller may default it).
+func validateFixtureLayoutDir(layoutDir string) error {
- if layoutDir == "" {
-
return nil - }
- clean := filepath.Clean(layoutDir)
- if filepath.IsAbs(clean) {
-
return fmt.Errorf("must be relative to fixture root: %q", layoutDir) - }
- if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) {
-
return fmt.Errorf("must be relative to fixture root: %q", layoutDir) - }
- return nil
+}
+type USBVolumeOptions struct {
- LayoutDir string
- Strategy string
- CreateReposDir bool
+}
+type USBVolumeConfig struct {
- SchemaVersion int
- LayoutDir string
- Strategy string
- CreatedAt time.Time
+}
+func mustRelativeLayoutDir(t *testing.T, layoutDir string) string {
- t.Helper()
- if layoutDir == "" {
-
return "repos" - }
- if err := validateFixtureLayoutDir(layoutDir); err != nil {
-
t.Fatalf("layout_dir %v", err) - }
- return filepath.Clean(layoutDir)
+}
+func MustUSBVolumeRoot(t *testing.T, opts USBVolumeOptions) string {
- t.Helper()
- root := t.TempDir()
- cfg := USBVolumeConfig{
-
SchemaVersion: 1, -
LayoutDir: mustRelativeLayoutDir(t, opts.LayoutDir), -
Strategy: opts.Strategy, -
CreatedAt: time.Now().UTC(), - }
- if cfg.Strategy == "" {
-
cfg.Strategy = "git-mirror" - }
- WriteUSBVolumeConfig(t, root, cfg)
- if opts.CreateReposDir {
-
if err := os.MkdirAll(filepath.Join(root, cfg.LayoutDir), 0o755); err != nil { -
t.Fatalf("failed creating repos dir: %v", err) -
} - }
- return root
+}
+func WriteUSBVolumeConfig(t *testing.T, root string, cfg USBVolumeConfig) {
- t.Helper()
- if cfg.SchemaVersion <= 0 {
-
cfg.SchemaVersion = 1 - }
- cfg.LayoutDir = mustRelativeLayoutDir(t, cfg.LayoutDir)
- if cfg.Strategy == "" {
-
cfg.Strategy = "git-mirror" - }
- if cfg.CreatedAt.IsZero() {
-
cfg.CreatedAt = time.Now().UTC() - }
- content := fmt.Sprintf(
-
"schema_version = %d\nlayout_dir = %q\nstrategy = %q\ncreated_at = %q\n", -
cfg.SchemaVersion, -
cfg.LayoutDir, -
cfg.Strategy, -
cfg.CreatedAt.Format(time.RFC3339), - )
- if err := os.WriteFile(filepath.Join(root, ".git-fire"), []byte(content), 0o644); err != nil {
-
t.Fatalf("failed writing .git-fire: %v", err) - }
+}
+func readUSBVolumeConfigBytes(data []byte) (USBVolumeConfig, error) {
- cfg := USBVolumeConfig{}
- lines := strings.Split(string(data), "\n")
- for _, line := range lines {
... diff truncated: showing 800 of 1078 lines
</details>
<sub>You can send follow-ups to the cloud agent <a href="https://cursor.com/agents/bc-fee2c5b4-a89b-42dd-b7d4-8922d0099437">here</a>.</sub>
<!-- BUGBOT_AUTOFIX_REVIEW_FOOTNOTE_END -->
<sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 628a737553e65efb611ac242e795968eadc548f0. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
2 participants
Summary
go vet+go test) and expand README requirements/common usage patterns for release readinessTest plan
gofmt -w fixtures.go snapshots.gogo test ./...go vet ./...Made with Cursor
Note
Medium Risk
Touching snapshot restore and path handling can break existing tests or introduce regressions in file extraction behavior, though changes add explicit safety checks and new test coverage. CI/toolchain updates are low risk but may surface new failures on older Go versions.
Overview
Improves release readiness by adding a GitHub Actions CI workflow that runs
go vetandgo teston PRs andmain, and by expandingREADME.mdplus a newDEVELOPER_GUIDE.mdwith usage and contribution/release guidance.Hardens snapshot creation/restoration by skipping the tar root pseudo-entry, normalizing snapshot names, and preventing path traversal/absolute paths during
RestoreSnapshotviasafeJoin(with new snapshot safety tests).Makes git-output parsing more robust by switching
GetRemotes,GetBranches, andGetCurrentSHAto standard-library string trimming/splitting, adds coverage for remote URLs containing spaces, introduces new USB-volume fixture helpers (usb_fixtures.go) with tests, and updatesgo.modto Go 1.22.Reviewed by Cursor Bugbot for commit d0c6cb2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores
Bug Fixes
New Features
Tests