Hybrid polyglot testkit: Go CLI bridge + wrapper smoke tests#6
Merged
Conversation
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a new JSON-over-stdio Go CLI entrypoint that dispatches fixture, git-query, and snapshot operations to refactored, error-returning Go helpers; introduces thin Python and Java wrapper clients that call the CLI, spec-kit documentation and validation, cross-language smoke tests, and CI jobs to validate conformance. Changes
Sequence Diagram(s)sequenceDiagram
participant Wrapper as Wrapper (Python/Java)
participant CLI as git-testkit-cli (Go)
participant Core as Go helpers (fixtures/snapshots/*E)
participant Git as Git + Filesystem
Wrapper->>CLI: write JSON request (op, baseDir, repoPath, options, args)
CLI->>CLI: parseRequest() / validate op
CLI->>Core: handle(req) — dispatch to specific helper
Core->>Git: perform git ops / filesystem / snapshot (tar/read/write)
Git-->>Core: outputs, tarball bytes, state
Core-->>CLI: return structured result (paths, output, snapshot metadata)
CLI->>Wrapper: write JSON response to stdout
Wrapper-->>Wrapper: parse response, surface fields/errors
sequenceDiagram
participant Test as Wrapper Test
participant CLI as git-testkit-cli
participant Snapshot as Snapshot Helpers
participant Disk as Filesystem
Test->>CLI: op: "snapshot_repo" (repoPath)
CLI->>Snapshot: SnapshotRepoE(repoPath)
Snapshot->>Disk: tar repo (including .git and symlinks)
Snapshot-->>CLI: {snapshotName, snapshotSize}
Test->>CLI: op: "snapshot_load_restore" (snapshotPath, baseDir)
CLI->>Snapshot: LoadSnapshotFromDiskE(snapshotPath) + RestoreSnapshotToDir(...)
Snapshot->>Disk: read tar, validate entries, extract under baseDir
Snapshot-->>CLI: {restorePath, snapshotName, snapshotSize}
Test-->>Test: verify restored repo SHA/branches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Comment |
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapshots.go (1)
82-90:⚠️ Potential issue | 🟠 MajorClose each file inside the walk callback.
defer file.Close()here waits until the whole snapshot finishes, so large repos can keep hundreds or thousands of descriptors open at once and fail withEMFILE. Close immediately afterio.Copy, including the error path.💡 Proposed fix
if info.Mode().IsRegular() { file, err := os.Open(path) if err != nil { return fmt.Errorf("failed to open file %s: %w", path, err) } - defer file.Close() if _, err := io.Copy(tarWriter, file); err != nil { + file.Close() return fmt.Errorf("failed to write file %s to tar: %w", path, err) } + if err := file.Close(); err != nil { + return fmt.Errorf("failed to close file %s: %w", path, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snapshots.go` around lines 82 - 90, The walk callback in snapshots.go defers file.Close(), which keeps many file descriptors open; change this to close the file immediately after use: after opening file (file, err := os.Open(path)) perform the io.Copy to tarWriter, then call file.Close() in all paths (success and error) instead of deferring; ensure you close before returning on io.Copy error (and include any close error if desired) so each file descriptor is released promptly within the callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/git-testkit-cli/main.go`:
- Around line 65-76: The parseRequest function reads from the hard-coded
"/dev/stdin" path which is non-portable; change it to read from os.Stdin using a
streaming decoder: replace the os.ReadFile("/dev/stdin") + json.Unmarshal(raw,
&req) pattern with json.NewDecoder(os.Stdin).Decode(&req) (keeping the same
error wrapping/messages), and retain the existing validation for req.Op
(strings.TrimSpace(req.Op) == "") so the request struct is still validated as
before.
In `@fixtures.go`:
- Around line 74-75: The code assumes the post-init default branch is
"main"/"master" when creating extra branches; instead capture the current branch
after the initial commit (e.g., run RunGitCmdE(repoPath, "rev-parse",
"--abbrev-ref", "HEAD") or equivalent right after the first commit) and store it
in a variable (e.g., originalBranch) and later restore that exact branch with
RunGitCmdE(repoPath, "checkout", originalBranch) rather than hardcoding
"main"/"master"; update any loops or checkout calls that reference opts.Branches
or the hardcoded branch to use this originalBranch and ensure the same logic is
applied in the other affected block (lines ~100-111) and aligns with
testkit/GIT_TESTKIT_SPEC.md.
- Around line 46-47: Validate and reject or sanitize repo names before calling
filepath.Join so callers cannot pass absolute paths or traverse outside baseDir:
in CreateTestRepoInDir inspect opts.Name (and any other places that call
filepath.Join(baseDir, name+".git") such as the other helper around the same
file) and ensure the name is not empty, not an absolute path, does not contain
path separators or ".." segments (e.g., compare to filepath.Base(name) and check
!filepath.IsAbs(name) and !strings.Contains(name, "..")/path separators), then
use the validated name for filepath.Join; if invalid, return an error.
In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java`:
- Around line 187-200: In CliBridge.invoke, don't immediately throw on non-zero
exit; instead inspect result.stdout (trimmed) for the CLI's structured payload
(use ERROR_PATTERN and extractOptional) and prefer that error text when present;
only if no structured error is found, fall back to throwing the existing "CLI
failed with code X: <stderr>" message. Ensure you still validate non-empty
stdout (as currently) and keep checks for "\"ok\":true" to succeed; reference
methods/fields: invoke, runCli, CliResult.code, CliResult.stdout,
CliResult.stderr, ERROR_PATTERN, extractOptional.
In `@testkit/python/git_testkit/cli.py`:
- Around line 18-34: When subprocess.run in _call returns a non-zero returncode,
parse proc.stdout as JSON (like the successful path does) and, if it contains
{"ok": false, "error": ...}, raise a RuntimeError with that error message; if
stdout cannot be parsed or lacks an error field, fall back to including
proc.stderr (and the raw stdout) in the RuntimeError. Update the error handling
in _call (which builds requests via _cli_cmd() and runs in _repo_root()) so
callers receive the CLI's JSON error instead of only stderr.
---
Outside diff comments:
In `@snapshots.go`:
- Around line 82-90: The walk callback in snapshots.go defers file.Close(),
which keeps many file descriptors open; change this to close the file
immediately after use: after opening file (file, err := os.Open(path)) perform
the io.Copy to tarWriter, then call file.Close() in all paths (success and
error) instead of deferring; ensure you close before returning on io.Copy error
(and include any close error if desired) so each file descriptor is released
promptly within the callback.
🪄 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: e5134c5c-5590-4bdb-ae3e-0dca749d8a87
📒 Files selected for processing (15)
cmd/git-testkit-cli/main.gofixtures.gosnapshots.gotestkit/GIT_TESTKIT_SPEC.mdtestkit/ROADMAP.mdtestkit/java/pom.xmltestkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.javatestkit/python/README.mdtestkit/python/git_testkit/__init__.pytestkit/python/git_testkit/cli.pytestkit/python/pyproject.tomltestkit/python/tests/test_fixtures.pytestkit/python/tests/test_scenarios.pytestkit/python/tests/test_snapshots.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testkit/python/tests/test_specify_conformance.py (1)
13-13: Derive expected snapshot name from created repo to reduce test coupling.This works today, but computing the expected value from
repois more resilient than a hardcoded literal.Suggested refactor
- assert snapshot_name == "specify-contract" + assert snapshot_name == Path(repo).name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/python/tests/test_specify_conformance.py` at line 13, Replace the hardcoded expected snapshot literal with a value derived from the created repo object: locate the assertion using snapshot_name (assert snapshot_name == "specify-contract") in test_specify_conformance.py and compute the expected name from the repo variable (e.g., use repo.name or repo.full_name to build the expected snapshot suffix/prefix such as f"{repo.name}-contract" or similar) so the assertion compares snapshot_name to that derived value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testkit/README.md`:
- Line 27: Replace the environment-specific command "Go regression: `cd
/workspace && go test ./...`" with a portable, repo-relative instruction—e.g.
"Go regression: `go test ./...`" (or instruct users to run `cd` to their repo
root before running `go test ./...`) so the README no longer relies on the
/workspace path.
In `@testkit/ROADMAP.md`:
- Line 20: Update the ROADMAP entry that currently references the CLI binary at
"testkit/cli" to the actual binary path used in the PR ("cmd/git-testkit-cli");
locate the line mentioning "Add CLI binary (`testkit/cli`) with JSON
request/response protocol" and change the path string to "cmd/git-testkit-cli"
so docs match the implemented binary name.
---
Nitpick comments:
In `@testkit/python/tests/test_specify_conformance.py`:
- Line 13: Replace the hardcoded expected snapshot literal with a value derived
from the created repo object: locate the assertion using snapshot_name (assert
snapshot_name == "specify-contract") in test_specify_conformance.py and compute
the expected name from the repo variable (e.g., use repo.name or repo.full_name
to build the expected snapshot suffix/prefix such as f"{repo.name}-contract" or
similar) so the assertion compares snapshot_name to that derived value.
🪄 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: c475f8f4-6346-4b11-a4e5-188c87df4220
📒 Files selected for processing (10)
testkit/.specify/memory/constitution.mdtestkit/.specify/specs/001-polyglot-testkit/checklists/quality.mdtestkit/.specify/specs/001-polyglot-testkit/contracts/cli-protocol.jsontestkit/.specify/specs/001-polyglot-testkit/plan.mdtestkit/.specify/specs/001-polyglot-testkit/spec.mdtestkit/.specify/specs/001-polyglot-testkit/tasks.mdtestkit/README.mdtestkit/ROADMAP.mdtestkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.javatestkit/python/tests/test_specify_conformance.py
✅ Files skipped from review due to trivial changes (5)
- testkit/.specify/specs/001-polyglot-testkit/spec.md
- testkit/.specify/specs/001-polyglot-testkit/checklists/quality.md
- testkit/.specify/specs/001-polyglot-testkit/contracts/cli-protocol.json
- testkit/.specify/specs/001-polyglot-testkit/tasks.md
- testkit/.specify/specs/001-polyglot-testkit/plan.md
🚧 Files skipped from review as they are similar to previous changes (1)
- testkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.java
![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 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Process deadlock: waitFor before draining output streams
- Updated the Java CLI bridge to drain stdout/stderr concurrently via futures before collecting process completion results, eliminating waitFor pipe deadlock risk.
- ✅ Fixed: Query functions mix stderr into stdout via CombinedOutput
- Changed RunGitCmdE to use stdout-only capture with explicit stderr in error paths so IsDirtyE, GetCurrentSHAE, and GetRemotesE no longer parse mixed streams.
- ✅ Fixed: Exported NewSnapshot and Payload have no callers
- Removed the unused exported NewSnapshot function and Payload method from snapshots.go to reduce dead API surface.
Preview (199142a06d)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -7,6 +7,52 @@
pull_request:
jobs:
+ spec-kit-conformance:
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Validate spec-kit artifact set
+ run: |
+ test -f testkit/.specify/memory/constitution.md
+ test -f testkit/.specify/specs/001-polyglot-testkit/spec.md
+ test -f testkit/.specify/specs/001-polyglot-testkit/plan.md
+ test -f testkit/.specify/specs/001-polyglot-testkit/tasks.md
+ test -f testkit/.specify/specs/001-polyglot-testkit/contracts/cli-protocol.json
+ test -f testkit/.specify/specs/001-polyglot-testkit/checklists/quality.md
+
+ - name: Validate spec-kit scaffold and status
+ run: ./testkit/.specify/scripts/validate_specify.sh
+
+ - name: Setup Go
+ uses: actions/setup-go@v5
+ with:
+ go-version: "stable"
+
+ - name: Setup Python
+ uses: actions/setup-python@v5
+ with:
+ python-version: "3.12"
+
+ - name: Setup Java
+ uses: actions/setup-java@v4
+ with:
+ distribution: temurin
+ java-version: "21"
+ cache: maven
+
+ - name: Run Python spec conformance smoke tests
+ run: |
+ cd testkit/python
+ python -m pip install -e ".[dev]"
+ python -m pytest tests/ -v
+
+ - name: Run Java spec conformance smoke tests
+ run: |
+ cd testkit/java
+ mvn test
+
test:
runs-on: ubuntu-latest
strategy:
diff --git a/cmd/git-testkit-cli/main.go b/cmd/git-testkit-cli/main.go
new file mode 100644
--- /dev/null
+++ b/cmd/git-testkit-cli/main.go
@@ -1,0 +1,246 @@
+package main
+
+import (
+ "encoding/json"
+ "fmt"
+ "os"
+ "path/filepath"
+ "strings"
+
+ testutil "github.com/git-fire/git-testkit"
+)
+
+type request struct {
+ Op string `json:"op"`
+ BaseDir string `json:"baseDir,omitempty"`
+ RepoPath string `json:"repoPath,omitempty"`
+ Args []string `json:"args,omitempty"`
+ Options *repoOptionsInput `json:"options,omitempty"`
+
+ SnapshotPath string `json:"snapshotPath,omitempty"`
+}
+
+type repoOptionsInput struct {
+ Name string `json:"name"`
+ Dirty bool `json:"dirty,omitempty"`
+ Files map[string]string `json:"files,omitempty"`
+ Remotes map[string]string `json:"remotes,omitempty"`
+ Branches []string `json:"branches,omitempty"`
+ InitialCommit string `json:"initialCommit,omitempty"`
+}
+
+type response struct {
+ OK bool `json:"ok"`
+
+ Error string `json:"error,omitempty"`
+
+ RepoPath string `json:"repoPath,omitempty"`
+ RemotePath string `json:"remotePath,omitempty"`
+ FSRoot string `json:"fsRoot,omitempty"`
+ Output string `json:"output,omitempty"`
+ Dirty *bool `json:"dirty,omitempty"`
+ Remotes map[string]string `json:"remotes,omitempty"`
+ SHA string `json:"sha,omitempty"`
+ Branches []string `json:"branches,omitempty"`
+ SnapshotName string `json:"snapshotName,omitempty"`
+ SnapshotSize int `json:"snapshotSize,omitempty"`
+ RestorePath string `json:"restorePath,omitempty"`
+}
+
+func main() {
+ req, err := parseRequest()
+ if err != nil {
+ writeResponse(response{OK: false, Error: err.Error()})
+ os.Exit(1)
+ }
+
+ res, err := handle(req)
+ if err != nil {
+ writeResponse(response{OK: false, Error: err.Error()})
+ os.Exit(1)
+ }
+ writeResponse(res)
+}
+
+func parseRequest() (request, error) {
+ raw, err := os.ReadFile("/dev/stdin")
+ if err != nil {
+ return request{}, fmt.Errorf("failed reading stdin: %w", err)
+ }
+ var req request
+ if err := json.Unmarshal(raw, &req); err != nil {
+ return request{}, fmt.Errorf("invalid JSON request: %w", err)
+ }
+ if strings.TrimSpace(req.Op) == "" {
+ return request{}, fmt.Errorf("missing required field: op")
+ }
+ return req, nil
+}
+
+func handle(req request) (response, error) {
+ switch req.Op {
+ case "create_test_repo":
+ base, err := ensureBaseDir(req.BaseDir)
+ if err != nil {
+ return response{}, err
+ }
+ if req.Options == nil {
+ return response{}, fmt.Errorf("missing options")
+ }
+ repoPath, err := testutil.CreateTestRepoInDir(base, testutil.RepoOptions{
+ Name: req.Options.Name,
+ Dirty: req.Options.Dirty,
+ Files: req.Options.Files,
+ Remotes: req.Options.Remotes,
+ Branches: req.Options.Branches,
+ InitialCommit: req.Options.InitialCommit,
+ })
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, RepoPath: repoPath}, nil
+
+ case "create_bare_remote":
+ base, err := ensureBaseDir(req.BaseDir)
+ if err != nil {
+ return response{}, err
+ }
+ if req.Options == nil || req.Options.Name == "" {
+ return response{}, fmt.Errorf("missing options.name")
+ }
+ remotePath, err := testutil.CreateBareRemoteInDir(base, req.Options.Name)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, RemotePath: remotePath}, nil
+
+ case "setup_fake_filesystem":
+ base, err := ensureBaseDir(req.BaseDir)
+ if err != nil {
+ return response{}, err
+ }
+ root, err := testutil.SetupFakeFilesystemInDir(base)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, FSRoot: root}, nil
+
+ case "run_git_cmd":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ output, err := testutil.RunGitCmdE(req.RepoPath, req.Args...)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, Output: output}, nil
+
+ case "is_dirty":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ dirty, err := testutil.IsDirtyE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, Dirty: &dirty}, nil
+
+ case "get_remotes":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ remotes, err := testutil.GetRemotesE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, Remotes: remotes}, nil
+
+ case "get_current_sha":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ sha, err := testutil.GetCurrentSHAE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, SHA: sha}, nil
+
+ case "get_branches":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ branches, err := testutil.GetBranchesE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, Branches: branches}, nil
+
+ case "snapshot_repo":
+ if req.RepoPath == "" {
+ return response{}, fmt.Errorf("missing repoPath")
+ }
+ snapshot, err := testutil.SnapshotRepoE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ return response{OK: true, SnapshotName: snapshot.Name(), SnapshotSize: snapshot.Size()}, nil
+
+ case "snapshot_save":
+ if req.RepoPath == "" || req.SnapshotPath == "" {
+ return response{}, fmt.Errorf("missing repoPath or snapshotPath")
+ }
+ snapshot, err := testutil.SnapshotRepoE(req.RepoPath)
+ if err != nil {
+ return response{}, err
+ }
+ if err := testutil.SaveSnapshotToDiskE(snapshot, req.SnapshotPath); err != nil {
+ return response{}, err
+ }
+ return response{OK: true, SnapshotName: snapshot.Name(), SnapshotSize: snapshot.Size()}, nil
+
+ case "snapshot_load_restore":
+ if req.SnapshotPath == "" {
+ return response{}, fmt.Errorf("missing snapshotPath")
+ }
+ base, err := ensureBaseDir(req.BaseDir)
+ if err != nil {
+ return response{}, err
+ }
+ snapshot, err := testutil.LoadSnapshotFromDiskE(req.SnapshotPath)
+ if err != nil {
+ return response{}, err
+ }
+ restorePath, err := testutil.RestoreSnapshotToDir(snapshot, base)
+ if err != nil {
+ return response{}, err
+ }
+ return response{
+ OK: true,
+ RestorePath: restorePath,
+ SnapshotName: snapshot.Name(),
+ SnapshotSize: snapshot.Size(),
+ }, nil
+
+ default:
+ return response{}, fmt.Errorf("unsupported op: %s", req.Op)
+ }
+}
+
+func ensureBaseDir(baseDir string) (string, error) {
+ if strings.TrimSpace(baseDir) == "" {
+ return "", fmt.Errorf("missing baseDir")
+ }
+ clean := filepath.Clean(baseDir)
+ if err := os.MkdirAll(clean, 0755); err != nil {
+ return "", err
+ }
+ return clean, nil
+}
+
+func writeResponse(res response) {
+ enc := json.NewEncoder(os.Stdout)
+ enc.SetEscapeHTML(false)
+ if err := enc.Encode(res); err != nil {
+ fmt.Fprintf(os.Stderr, `{"ok":false,"error":"failed writing response: %s"}`+"\n", err.Error())
+ }
+}
diff --git a/fixtures.go b/fixtures.go
--- a/fixtures.go
+++ b/fixtures.go
@@ -1,6 +1,7 @@
package testutil
import (
+ "fmt"
"os"
"os/exec"
"path/filepath"
@@ -34,102 +35,128 @@
func CreateTestRepo(t *testing.T, opts RepoOptions) string {
t.Helper()
- // Create temp directory
- tmpDir := t.TempDir()
- repoPath := filepath.Join(tmpDir, opts.Name)
+ repoPath, err := CreateTestRepoInDir(t.TempDir(), opts)
+ if err != nil {
+ t.Fatalf("Failed to create test repo: %v", err)
+ }
+ return repoPath
+}
+// CreateTestRepoInDir creates a test repository under the provided base directory.
+func CreateTestRepoInDir(baseDir string, opts RepoOptions) (string, error) {
+ repoPath := filepath.Join(baseDir, opts.Name)
+
if err := os.MkdirAll(repoPath, 0755); err != nil {
- t.Fatalf("Failed to create repo directory: %v", err)
+ return "", fmt.Errorf("failed to create repo directory: %w", err)
}
- // Initialize git repo
- runGit(t, repoPath, "init")
- runGit(t, repoPath, "config", "user.email", "test@example.com")
- runGit(t, repoPath, "config", "user.name", "Test User")
+ if _, err := RunGitCmdE(repoPath, "init"); err != nil {
+ return "", err
+ }
+ if _, err := RunGitCmdE(repoPath, "config", "user.email", "test@example.com"); err != nil {
+ return "", err
+ }
+ if _, err := RunGitCmdE(repoPath, "config", "user.name", "Test User"); err != nil {
+ return "", err
+ }
- // Create initial commit (required for most operations)
- initialFile := filepath.Join(repoPath, "README.md")
commitMsg := opts.InitialCommit
if commitMsg == "" {
commitMsg = "Initial commit"
}
-
+ initialFile := filepath.Join(repoPath, "README.md")
if err := os.WriteFile(initialFile, []byte("# Test Repo\n"), 0644); err != nil {
- t.Fatalf("Failed to create README: %v", err)
+ return "", fmt.Errorf("failed to create README: %w", err)
}
+ if _, err := RunGitCmdE(repoPath, "add", "README.md"); err != nil {
+ return "", err
+ }
+ if _, err := RunGitCmdE(repoPath, "commit", "-m", commitMsg); err != nil {
+ return "", err
+ }
- runGit(t, repoPath, "add", "README.md")
- runGit(t, repoPath, "commit", "-m", commitMsg)
-
- // Create additional files if specified
for filename, content := range opts.Files {
filePath := filepath.Join(repoPath, filename)
-
- // Create parent directories if needed
- dir := filepath.Dir(filePath)
- if err := os.MkdirAll(dir, 0755); err != nil {
- t.Fatalf("Failed to create directory for %s: %v", filename, err)
+ if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil {
+ return "", fmt.Errorf("failed to create directory for %s: %w", filename, err)
}
-
if err := os.WriteFile(filePath, []byte(content), 0644); err != nil {
- t.Fatalf("Failed to create file %s: %v", filename, err)
+ return "", fmt.Errorf("failed to create file %s: %w", filename, err)
}
- runGit(t, repoPath, "add", filename)
- runGit(t, repoPath, "commit", "-m", "Add "+filename)
+ if _, err := RunGitCmdE(repoPath, "add", filename); err != nil {
+ return "", err
+ }
+ if _, err := RunGitCmdE(repoPath, "commit", "-m", "Add "+filename); err != nil {
+ return "", err
+ }
}
- // Add remotes
for name, url := range opts.Remotes {
- runGit(t, repoPath, "remote", "add", name, url)
+ if _, err := RunGitCmdE(repoPath, "remote", "add", name, url); err != nil {
+ return "", err
+ }
}
- // Create branches
for _, branch := range opts.Branches {
- runGit(t, repoPath, "checkout", "-b", branch)
+ if _, err := RunGitCmdE(repoPath, "checkout", "-b", branch); err != nil {
+ return "", err
+ }
}
- // Return to main/master branch
if len(opts.Branches) > 0 {
- // Try main first, fallback to master
- if err := exec.Command("git", "-C", repoPath, "checkout", "main").Run(); err != nil {
- runGit(t, repoPath, "checkout", "master")
+ if _, err := RunGitCmdE(repoPath, "checkout", "main"); err != nil {
+ if _, fallbackErr := RunGitCmdE(repoPath, "checkout", "master"); fallbackErr != nil {
+ return "", fallbackErr
+ }
}
}
- // Make repo dirty if requested
if opts.Dirty {
dirtyFile := filepath.Join(repoPath, "uncommitted.txt")
if err := os.WriteFile(dirtyFile, []byte("uncommitted changes\n"), 0644); err != nil {
- t.Fatalf("Failed to create dirty file: %v", err)
+ return "", fmt.Errorf("failed to create dirty file: %w", err)
}
}
- return repoPath
+ return repoPath, nil
}
// CreateBareRemote creates a bare git repository to use as a remote
func CreateBareRemote(t *testing.T, name string) string {
t.Helper()
- tmpDir := t.TempDir()
- remotePath := filepath.Join(tmpDir, name+".git")
+ remotePath, err := CreateBareRemoteInDir(t.TempDir(), name)
+ if err != nil {
+ t.Fatalf("Failed to create bare remote: %v", err)
+ }
+ return remotePath
+}
+// CreateBareRemoteInDir creates a bare remote repository under the provided base directory.
+func CreateBareRemoteInDir(baseDir, name string) (string, error) {
+ remotePath := filepath.Join(baseDir, name+".git")
if err := os.MkdirAll(remotePath, 0755); err != nil {
- t.Fatalf("Failed to create bare repo directory: %v", err)
+ return "", fmt.Errorf("failed to create bare repo directory: %w", err)
}
-
- runGit(t, remotePath, "init", "--bare")
-
- return remotePath
+ if _, err := RunGitCmdE(remotePath, "init", "--bare"); err != nil {
+ return "", err
+ }
+ return remotePath, nil
}
// SetupFakeFilesystem creates a fake filesystem structure for scanning tests
func SetupFakeFilesystem(t *testing.T) string {
t.Helper()
- tmpDir := t.TempDir()
+ root, err := SetupFakeFilesystemInDir(t.TempDir())
+ if err != nil {
+ t.Fatalf("Failed to setup fake filesystem: %v", err)
+ }
+ return root
+}
- // Create directory structure
+// SetupFakeFilesystemInDir creates a deterministic fake filesystem tree under baseDir.
+func SetupFakeFilesystemInDir(baseDir string) (string, error) {
dirs := []string{
"home/testuser/projects",
"home/testuser/src",
@@ -138,27 +165,22 @@
"root/sys",
"root/proc",
}
-
for _, dir := range dirs {
- path := filepath.Join(tmpDir, dir)
+ path := filepath.Join(baseDir, dir)
if err := os.MkdirAll(path, 0755); err != nil {
- t.Fatalf("Failed to create directory %s: %v", dir, err)
+ return "", fmt.Errorf("failed to create directory %s: %w", dir, err)
}
}
-
- return tmpDir
+ return baseDir, nil
}
// runGit is a helper to run git commands in a specific directory
func runGit(t *testing.T, dir string, args ...string) {
t.Helper()
- cmd := exec.Command("git", args...)
- cmd.Dir = dir
-
- output, err := cmd.CombinedOutput()
+ _, err := RunGitCmdE(dir, args...)
if err != nil {
- t.Fatalf("Git command failed: git %v\nOutput: %s\nError: %v", args, output, err)
+ t.Fatalf("%v", err)
}
}
@@ -166,39 +188,98 @@
func IsDirty(t *testing.T, repoPath string) bool {
t.Helper()
- cmd := exec.Command("git", "status", "--porcelain")
- cmd.Dir = repoPath
-
- output, err := cmd.Output()
+ dirty, err := IsDirtyE(repoPath)
if err != nil {
t.Fatalf("Failed to check git status: %v", err)
}
-
- return len(output) > 0
+ return dirty
}
// GetRemotes returns the configured remotes for a repo
func GetRemotes(t *testing.T, repoPath string) map[string]string {
t.Helper()
- cmd := exec.Command("git", "remote", "-v")
- cmd.Dir = repoPath
+ remotes, err := GetRemotesE(repoPath)
+ if err != nil {
+ t.Fatalf("Failed to get remotes: %v", err)
+ }
+ return remotes
+}
+// 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) {
+ t.Helper()
+ runGit(t, dir, args...)
+}
+
+// RunGitCmdE runs git command in dir and returns trimmed command output.
+func RunGitCmdE(dir string, args ...string) (string, error) {
+ cmd := exec.Command("git", args...)
+ cmd.Dir = dir
+
output, err := cmd.Output()
if err != nil {
- t.Fatalf("Failed to get remotes: %v", err)
+ if exitErr, ok := err.(*exec.ExitError); ok {
+ return "", fmt.Errorf(
+ "git command failed: git %v\nStdout: %s\nStderr: %s\nError: %w",
+ args,
+ strings.TrimSpace(string(output)),
+ strings.TrimSpace(string(exitErr.Stderr)),
+ err,
+ )
+ }
+ return "", fmt.Errorf("git command failed: git %v\nError: %w", args, err)
}
+ return strings.TrimSpace(string(output)), nil
+}
- // Parse output into map
- // Format: "origin /path/to/remote (fetch)"
- // "origin /path/to/remote (push)"
+// GetCurrentSHA returns the current commit SHA
+func GetCurrentSHA(t *testing.T, repoPath string) string {
+ t.Helper()
+
+ sha, err := GetCurrentSHAE(repoPath)
+ if err != nil {
+ t.Fatalf("Failed to get current SHA: %v", err)
+ }
+ return sha
+}
+
+// GetBranches returns all branches in the repo
+func GetBranches(t *testing.T, repoPath string) []string {
+ t.Helper()
+
+ branches, err := GetBranchesE(repoPath)
+ if err != nil {
+ t.Fatalf("Failed to get branches: %v", err)
+ }
+ return branches
+}
+
+// IsDirtyE checks if a git repo has uncommitted changes.
+func IsDirtyE(repoPath string) (bool, error) {
+ output, err := RunGitCmdE(repoPath, "status", "--porcelain")
+ if err != nil {
+ return false, err
+ }
+ return len(output) > 0, nil
+}
+
+// GetRemotesE returns configured remotes for a repo.
+func GetRemotesE(repoPath string) (map[string]string, error) {
+ output, err := RunGitCmdE(repoPath, "remote", "-v")
+ if err != nil {
+ return nil, err
+ }
+ return parseRemotesOutput(output), nil
+}
+
+func parseRemotesOutput(output string) map[string]string {
remotes := make(map[string]string)
-
- lines := strings.TrimSpace(string(output))
+ lines := strings.TrimSpace(output)
if lines == "" {
return remotes
}
-
for _, line := range strings.Split(lines, "\n") {
line = strings.TrimSpace(line)
if line == "" {
@@ -206,7 +287,6 @@
}
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
@@ -218,8 +298,6 @@
remainder = strings.TrimSpace(remainder)
}
- // Strip only the trailing git remote role suffix once so paths that end
- // with text like " (push)" are not damaged by sequential TrimSuffix calls.
if strings.HasSuffix(remainder, " (fetch)") {
remainder = strings.TrimSuffix(remainder, " (fetch)")
} else if strings.HasSuffix(remainder, " (push)") {
@@ -230,42 +308,22 @@
remotes[name] = remainder
}
}
-
return remotes
}
-// 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) {
- t.Helper()
- runGit(t, dir, args...)
+// GetCurrentSHAE returns the current commit SHA.
+func GetCurrentSHAE(repoPath string) (string, error) {
+ return RunGitCmdE(repoPath, "rev-parse", "HEAD")
}
-// GetCurrentSHA returns the current commit SHA
-func GetCurrentSHA(t *testing.T, repoPath string) string {
- t.Helper()
-
- cmd := exec.Command("git", "rev-parse", "HEAD")
- cmd.Dir = repoPath
-
- output, err := cmd.Output()
- if err != nil {
- t.Fatalf("Failed to get current SHA: %v", err)
- }
-
- return strings.TrimSpace(string(output))
-}
-
-// GetBranches returns all branches in the repo
-func GetBranches(t *testing.T, repoPath string) []string {
- t.Helper()
-
+// GetBranchesE returns all branches in a repo.
+func GetBranchesE(repoPath string) ([]string, error) {
cmd := exec.Command("git", "branch", "--format=%(refname:short)")
cmd.Dir = repoPath
output, err := cmd.Output()
if err != nil {
- t.Fatalf("Failed to get branches: %v", err)
+ return nil, err
}
branches := strings.Split(strings.TrimSpace(string(output)), "\n")
@@ -278,5 +336,5 @@
}
}
- return result
+ return result, nil
}
diff --git a/fixtures_test.go b/fixtures_test.go
--- a/fixtures_test.go
+++ b/fixtures_test.go
@@ -2,7 +2,9 @@
import (
"os"
+ "os/exec"
"path/filepath"
+ "strings"
"testing"
testutil "github.com/git-fire/git-testkit"
@@ -151,3 +153,47 @@
}
}
}
+
+func TestQueryHelpersIgnoreGitTraceStderr(t *testing.T) {
+ t.Setenv("GIT_TRACE", "1")
+
+ remotePath := testutil.CreateBareRemote(t, "origin")
+ repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{
+ Name: "trace-safe-repo",
+ Remotes: map[string]string{
+ "origin": remotePath,
+ },
+ })
+
+ dirty, err := testutil.IsDirtyE(repoPath)
+ if err != nil {
+ t.Fatalf("IsDirtyE failed: %v", err)
+ }
+ if dirty {
+ t.Fatal("expected clean repo to remain clean when git writes trace to stderr")
+ }
+
+ sha, err := testutil.GetCurrentSHAE(repoPath)
+ if err != nil {
+ t.Fatalf("GetCurrentSHAE failed: %v", err)
+ }
+
+ cmd := exec.Command("git", "rev-parse", "HEAD")
+ cmd.Dir = repoPath
+ expectedSHABytes, err := cmd.Output()
+ if err != nil {
+ t.Fatalf("failed to get expected sha: %v", err)
+ }
+ expectedSHA := strings.TrimSpace(string(expectedSHABytes))
+ if sha != expectedSHA {
+ t.Fatalf("expected sha %q, got %q", expectedSHA, sha)
+ }
+
+ remotes, err := testutil.GetRemotesE(repoPath)
+ if err != nil {
+ t.Fatalf("GetRemotesE failed: %v", err)
+ }
+ if got := remotes["origin"]; got != remotePath {
+ t.Fatalf("expected origin remote %q, got %q", remotePath, got)
+ }
+}
diff --git a/snapshots.go b/snapshots.go
--- a/snapshots.go
+++ b/snapshots.go
@@ -30,7 +30,15 @@
// This allows fast restoration of expensive test setups
func SnapshotRepo(t *testing.T, repoPath string) *Snapshot {
t.Helper()
+ snapshot, err := SnapshotRepoE(repoPath)
+ if err != nil {
+ t.Fatalf("Failed to create snapshot: %v", err)
+ }
+ return snapshot
+}
+// SnapshotRepoE creates an in-memory snapshot of a repository and returns errors.
+func SnapshotRepoE(repoPath string) (*Snapshot, error) {
var buf bytes.Buffer
gzipWriter := gzip.NewWriter(&buf)
tarWriter := tar.NewWriter(gzipWriter)
@@ -77,23 +85,20 @@
return nil
})
... diff truncated: showing 800 of 2350 linesYou can send follow-ups to the cloud agent here.
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testkit/README.md (1)
21-28: Document required tooling before conformance commands.Please add a brief prerequisite note that Go 1.22+ and
gitmust be installed and available onPATHbefore running these commands.Based on learnings: Ensure Go 1.22+ and
gitare installed and available on PATH before running any commands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/README.md` around lines 21 - 28, Add a short prerequisites note above the "Conformance execution" section stating that Go 1.22+ and git must be installed and on PATH; update the README.md "Conformance execution" section header so it is preceded by this prerequisite line mentioning exact tooling (Go 1.22+ and git) and that these are required before running the Python/Java/Go commands..github/workflows/ci.yml (1)
16-26: Avoid duplicate spec artifact checks in CI.Line 16–24 duplicates checks already implemented by
testkit/.specify/scripts/validate_specify.sh(Line 25–26). Keep one source of truth to avoid drift when required files change.Suggested simplification
- - name: Validate spec-kit artifact set - run: | - test -f testkit/.specify/memory/constitution.md - test -f testkit/.specify/specs/001-polyglot-testkit/spec.md - test -f testkit/.specify/specs/001-polyglot-testkit/plan.md - test -f testkit/.specify/specs/001-polyglot-testkit/tasks.md - test -f testkit/.specify/specs/001-polyglot-testkit/contracts/cli-protocol.json - test -f testkit/.specify/specs/001-polyglot-testkit/checklists/quality.md - - name: Validate spec-kit scaffold and status run: ./testkit/.specify/scripts/validate_specify.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 16 - 26, Remove the duplicate explicit file existence checks and keep the single source of truth script run: delete the entire CI step titled "Validate spec-kit artifact set" (the block that runs multiple test -f commands) and retain the step that runs ./testkit/.specify/scripts/validate_specify.sh ("Validate spec-kit scaffold and status"); this ensures the validate_specify.sh script is the authoritative check and avoids drift.testkit/.specify/scripts/validate_specify.sh (1)
23-28: Add explicit assertion messages for content checks.Line 23–28 can fail with minimal context (
grepexit only), which makes CI triage slower. Prefer a small helper that prints which file/pattern failed and use fixed-string matching.Suggested refactor
+assert_contains() { + local file="$1" + local expected="$2" + if ! grep -Fq -- "$expected" "$file"; then + echo "missing expected content in $file: $expected" >&2 + exit 1 + fi +} + -grep -q "Status\\*\\*: Implemented (canonical spec-kit baseline)" "$SPEC_DIR/spec.md" -grep -q "Status\\*\\*: Implemented (canonical spec-kit baseline)" "$SPEC_DIR/plan.md" -grep -q "T015 Add spec-kit command workflow doc + shell helper" "$SPEC_DIR/tasks.md" -grep -q "\\[x\\] T015" "$SPEC_DIR/tasks.md" -grep -q "\"supported_ops\"" "$SPEC_DIR/contracts/cli-protocol.json" -grep -q "\\[x\\] Smoke test coverage exists for Go, Python wrapper, and Java wrapper paths." "$SPEC_DIR/checklists/quality.md" +assert_contains "$SPEC_DIR/spec.md" "**Status**: Implemented (canonical spec-kit baseline)" +assert_contains "$SPEC_DIR/plan.md" "**Status**: Implemented (canonical spec-kit baseline)" +assert_contains "$SPEC_DIR/tasks.md" "T015 Add spec-kit command workflow doc + shell helper" +assert_contains "$SPEC_DIR/tasks.md" "[x] T015" +assert_contains "$SPEC_DIR/contracts/cli-protocol.json" "\"supported_ops\"" +assert_contains "$SPEC_DIR/checklists/quality.md" "[x] Smoke test coverage exists for Go, Python wrapper, and Java wrapper paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/.specify/scripts/validate_specify.sh` around lines 23 - 28, The content checks in validate_specify.sh (the grep calls checking "$SPEC_DIR/spec.md", "$SPEC_DIR/plan.md", "$SPEC_DIR/tasks.md", "contracts/cli-protocol.json", and "checklists/quality.md") should be replaced by a small helper (e.g., assert_contains) that uses fixed-string matching (grep -F -q) and, on failure, prints a clear assertion message including the filename and the pattern that failed and then exits non-zero; update the six calls that currently run bare grep (the patterns "Status**: Implemented (canonical spec-kit baseline)", "T015 Add spec-kit command workflow doc + shell helper", "[x] T015", "\"supported_ops\"", and the smoke test checklist line) to call this helper so CI failures show which file/pattern failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testkit/README.md`:
- Line 3: Update the README's phrase "spec-kit style workspace" to the
hyphenated compound adjective "spec-kit-style workspace" wherever that exact
phrase appears (specifically replace the string "spec-kit style workspace" in
the README) to improve clarity and consistency.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 16-26: Remove the duplicate explicit file existence checks and
keep the single source of truth script run: delete the entire CI step titled
"Validate spec-kit artifact set" (the block that runs multiple test -f commands)
and retain the step that runs ./testkit/.specify/scripts/validate_specify.sh
("Validate spec-kit scaffold and status"); this ensures the validate_specify.sh
script is the authoritative check and avoids drift.
In `@testkit/.specify/scripts/validate_specify.sh`:
- Around line 23-28: The content checks in validate_specify.sh (the grep calls
checking "$SPEC_DIR/spec.md", "$SPEC_DIR/plan.md", "$SPEC_DIR/tasks.md",
"contracts/cli-protocol.json", and "checklists/quality.md") should be replaced
by a small helper (e.g., assert_contains) that uses fixed-string matching (grep
-F -q) and, on failure, prints a clear assertion message including the filename
and the pattern that failed and then exits non-zero; update the six calls that
currently run bare grep (the patterns "Status**: Implemented (canonical spec-kit
baseline)", "T015 Add spec-kit command workflow doc + shell helper", "[x] T015",
"\"supported_ops\"", and the smoke test checklist line) to call this helper so
CI failures show which file/pattern failed.
In `@testkit/README.md`:
- Around line 21-28: Add a short prerequisites note above the "Conformance
execution" section stating that Go 1.22+ and git must be installed and on PATH;
update the README.md "Conformance execution" section header so it is preceded by
this prerequisite line mentioning exact tooling (Go 1.22+ and git) and that
these are required before running the Python/Java/Go commands.
🪄 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: 485a8b18-825b-45bc-80db-3f1c2bb2396c
📒 Files selected for processing (6)
.github/workflows/ci.ymltestkit/.specify/scripts/validate_specify.shtestkit/.specify/specs/001-polyglot-testkit/plan.mdtestkit/.specify/specs/001-polyglot-testkit/spec.mdtestkit/.specify/specs/001-polyglot-testkit/tasks.mdtestkit/README.md
✅ Files skipped from review due to trivial changes (3)
- testkit/.specify/specs/001-polyglot-testkit/tasks.md
- testkit/.specify/specs/001-polyglot-testkit/plan.md
- testkit/.specify/specs/001-polyglot-testkit/spec.md
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fixtures.go (1)
319-340: Inconsistent:GetBranchesEbypassesRunGitCmdE.Other
*Ehelpers (IsDirtyE,GetRemotesE,GetCurrentSHAE) delegate toRunGitCmdE, which provides richer error messages including stderr output.GetBranchesEuses rawexec.Commanddirectly, resulting in less informative errors on failure.♻️ Proposed refactor to use RunGitCmdE
// GetBranchesE returns all branches in a repo. func GetBranchesE(repoPath string) ([]string, error) { - cmd := exec.Command("git", "branch", "--format=%(refname:short)") - cmd.Dir = repoPath - - output, err := cmd.Output() + output, err := RunGitCmdE(repoPath, "branch", "--format=%(refname:short)") if err != nil { return nil, err } - branches := strings.Split(strings.TrimSpace(string(output)), "\n") + branches := strings.Split(output, "\n") // Filter out empty lines var result []string for _, b := range branches { if b != "" { result = append(result, b) } } return result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures.go` around lines 319 - 340, GetBranchesE currently invokes exec.Command directly which bypasses RunGitCmdE and loses stderr-rich errors; change GetBranchesE to call RunGitCmdE with the same args ("branch", "--format=%(refname:short)") and repoPath, then parse the returned stdout (trim, split on "\n", filter empty strings) and return that slice and error from RunGitCmdE so error messages include stderr details; keep the function name and signature (GetBranchesE(repoPath string) ([]string, error)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fixtures.go`:
- Around line 319-340: GetBranchesE currently invokes exec.Command directly
which bypasses RunGitCmdE and loses stderr-rich errors; change GetBranchesE to
call RunGitCmdE with the same args ("branch", "--format=%(refname:short)") and
repoPath, then parse the returned stdout (trim, split on "\n", filter empty
strings) and return that slice and error from RunGitCmdE so error messages
include stderr details; keep the function name and signature
(GetBranchesE(repoPath string) ([]string, error)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7a00c2d-c4ed-4240-a33c-ac1bc3f2ac83
📒 Files selected for processing (4)
fixtures.gofixtures_test.gosnapshots.gotestkit/java/src/main/java/io/gitfire/testkit/CliBridge.java
🚧 Files skipped from review as they are similar to previous changes (2)
- testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java
- snapshots.go
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fixtures.go (1)
349-369: Consider usingRunGitCmdEfor consistency.
GetBranchesEdirectly usesexec.Commandwhile other*Efunctions delegate toRunGitCmdE. This means errors from this function won't include stderr in the error message, making debugging harder.♻️ Proposed refactor for consistency
// GetBranchesE returns all branches in a repo. func GetBranchesE(repoPath string) ([]string, error) { - cmd := exec.Command("git", "branch", "--format=%(refname:short)") - cmd.Dir = repoPath - - output, err := cmd.Output() + output, err := RunGitCmdE(repoPath, "branch", "--format=%(refname:short)") if err != nil { return nil, err } - branches := strings.Split(strings.TrimSpace(string(output)), "\n") + branches := strings.Split(output, "\n") // Filter out empty lines var result []string for _, b := range branches { if b != "" { result = append(result, b) } } return result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures.go` around lines 349 - 369, GetBranchesE currently invokes exec.Command directly so errors lack stderr context; change it to call the existing RunGitCmdE helper (use RunGitCmdE(repoPath, "branch", "--format=%(refname:short)") or the equivalent signature in your codebase), capture its stdout result, split/trim into lines and filter out empty strings as before, and return stdout-derived []string; return the error from RunGitCmdE so stderr is included in failure messages. Ensure you keep the function name GetBranchesE and preserve the same output behavior (trim, split on "\n", filter empty).testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java (3)
107-130: Regex-based JSON parsing is fragile but acceptable for controlled protocol.The
getRemotesmethod parses JSON by splitting on commas, which would fail if remote URLs contained commas. However, since the Go CLI generates the JSON and git remote URLs typically don't contain commas, this is acceptable for the controlled protocol.If robustness becomes a concern later, consider using a lightweight JSON parser.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java` around lines 107 - 130, The getRemotes method in CliBridge currently parses the JSON body by splitting on commas which can break if values contain commas; replace the manual split-based parsing in getRemotes (use REMOTES_PATTERN to extract the JSON body from invoke(payload)), then parse that JSON string into a Map<String,String> using a proper JSON parser (e.g., Jackson ObjectMapper or org.json.JSONObject) instead of split/unquote logic; remove the pairs loop and unquote calls and return the parsed Map, keeping invoke(...) and REMOTES_PATTERN usage to locate the payload.
209-240: Consider destroying the process on interruption.When
InterruptedExceptionis caught, the thread is properly re-interrupted, but the spawned process may continue running. For cleaner resource management, consider destroying the process.♻️ Proposed fix
private CliResult runCli(String payload) { ExecutorService streamReaderPool = null; + Process process = null; try { ProcessBuilder pb = new ProcessBuilder("bash", "-lc", cliCommand); pb.directory(workspaceRoot.toFile()); - Process process = pb.start(); + process = pb.start(); streamReaderPool = Executors.newFixedThreadPool(2); // ... rest unchanged } catch (InterruptedException ex) { Thread.currentThread().interrupt(); + if (process != null) { + process.destroyForcibly(); + } throw new RuntimeException("interrupted while invoking CLI", ex); } finally { if (streamReaderPool != null) { streamReaderPool.shutdownNow(); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java` around lines 209 - 240, The runCli method currently spawns a Process inside the try block but does not ensure the Process is destroyed if an InterruptedException occurs; move the Process declaration so it’s visible to the catch/finally blocks, and in the InterruptedException handler (and/or finally) check if the Process variable is non-null and call process.destroyForcibly() (optionally waitFor a short timeout) to ensure the spawned process is terminated, then re-interrupt the thread as already done; update runCli, Process process, and the InterruptedException catch/finally logic accordingly.
212-212:bash -lcinvocation limits Windows portability.The
ProcessBuilderusesbash -lcto execute the CLI command, which requires bash to be available. This won't work on standard Windows without Git Bash or WSL.This is part of the Phase 1 interim thin-wrapper architecture (as documented in the roadmap). Windows support is planned in Phase 2 with native implementations. Consider documenting this Unix-only requirement for the Java wrapper in the testkit README, or deferring until the native Java implementation phase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java` at line 212, The ProcessBuilder in CliBridge currently invokes bash via new ProcessBuilder("bash", "-lc", cliCommand), which breaks on Windows where bash isn't available; either document this Unix-only requirement or defer changes until the planned native Java implementation: update the testkit README and/or comments in the CliBridge class to state that CliBridge (and its use of ProcessBuilder with "bash -lc" for cliCommand execution) is intentionally Unix-only for Phase 1, and add a TODO referencing the Phase 2 native Java implementation to avoid surprising Windows users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/git-testkit-cli/main.go`:
- Around line 236-242: The fallback in writeResponse manually formats JSON with
err.Error() which can break JSON if the error contains quotes/newlines; change
the fallback to produce properly escaped JSON (e.g., build a value like
map[string]interface{}{"ok": false, "error": err.Error()} or a small struct and
call json.NewEncoder(os.Stderr).Encode(...) or use json.Marshal on the error
string) instead of fmt.Fprintf with string interpolation, keeping the rest of
writeResponse (including enc.Encode(res)) unchanged.
---
Nitpick comments:
In `@fixtures.go`:
- Around line 349-369: GetBranchesE currently invokes exec.Command directly so
errors lack stderr context; change it to call the existing RunGitCmdE helper
(use RunGitCmdE(repoPath, "branch", "--format=%(refname:short)") or the
equivalent signature in your codebase), capture its stdout result, split/trim
into lines and filter out empty strings as before, and return stdout-derived
[]string; return the error from RunGitCmdE so stderr is included in failure
messages. Ensure you keep the function name GetBranchesE and preserve the same
output behavior (trim, split on "\n", filter empty).
In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java`:
- Around line 107-130: The getRemotes method in CliBridge currently parses the
JSON body by splitting on commas which can break if values contain commas;
replace the manual split-based parsing in getRemotes (use REMOTES_PATTERN to
extract the JSON body from invoke(payload)), then parse that JSON string into a
Map<String,String> using a proper JSON parser (e.g., Jackson ObjectMapper or
org.json.JSONObject) instead of split/unquote logic; remove the pairs loop and
unquote calls and return the parsed Map, keeping invoke(...) and REMOTES_PATTERN
usage to locate the payload.
- Around line 209-240: The runCli method currently spawns a Process inside the
try block but does not ensure the Process is destroyed if an
InterruptedException occurs; move the Process declaration so it’s visible to the
catch/finally blocks, and in the InterruptedException handler (and/or finally)
check if the Process variable is non-null and call process.destroyForcibly()
(optionally waitFor a short timeout) to ensure the spawned process is
terminated, then re-interrupt the thread as already done; update runCli, Process
process, and the InterruptedException catch/finally logic accordingly.
- Line 212: The ProcessBuilder in CliBridge currently invokes bash via new
ProcessBuilder("bash", "-lc", cliCommand), which breaks on Windows where bash
isn't available; either document this Unix-only requirement or defer changes
until the planned native Java implementation: update the testkit README and/or
comments in the CliBridge class to state that CliBridge (and its use of
ProcessBuilder with "bash -lc" for cliCommand execution) is intentionally
Unix-only for Phase 1, and add a TODO referencing the Phase 2 native Java
implementation to avoid surprising Windows users.
🪄 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: 1c58ec99-2200-4ff5-b73f-d5f18b017e07
📒 Files selected for processing (10)
cmd/git-testkit-cli/main.gofixtures.gofixtures_test.gosnapshots.gotestkit/GIT_TESTKIT_SPEC.mdtestkit/README.mdtestkit/ROADMAP.mdtestkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/python/git_testkit/cli.pytestkit/python/tests/test_specify_conformance.py
✅ Files skipped from review due to trivial changes (3)
- testkit/README.md
- testkit/GIT_TESTKIT_SPEC.md
- snapshots.go
🚧 Files skipped from review as they are similar to previous changes (2)
- testkit/python/tests/test_specify_conformance.py
- fixtures_test.go
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Member
Author
|
@coderabbitai is this merge ready? Is this safe? are we releaes ready? |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 203422
✅ What's Green
❌ Not Yet Merge-Ready — Blockers1. No Formal Approval
2. Pre-Merge Check FailureDocstring coverage is 28.09%, which is below the required 80% threshold. This is a configured pre-merge check that is currently failing. 3. Cursor Bugbot Still RunningThe
The Java Recommended Before Merging
Would you like me to trigger a fresh review, or open a follow-up issue to track the Java ✅ Actions performedReview triggered.
|
Member
Author
|
bugbot is this ready to go? same thing as coderabbit safe and release ready? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the current CI status, open review comments, and check runs before giving you a verdict. 🏁 Script executed: Length of output: 67082
✅ What's in good shape
🚨 Unresolved bugs (Cursor Bugbot — none addressed)🔴 HIGH —
|
| Gate | Status |
|---|---|
| CI green | ✅ |
| No formal approval | |
| CodeRabbit findings resolved | ✅ |
| Cursor Bugbot HIGH finding resolved | ❌ |
| Cursor Bugbot MEDIUM findings resolved | ❌ (4 open) |
| Release-ready | ❌ Not yet |
The minimum bar before merging should be: (1) fix RunGitCmdE to use separate stdout/stderr (cmd.Output() + cmd.Stderr = &stderrBuf) and (2) fix the Java runCli deadlock by reading stdout/stderr concurrently before calling waitFor(). The Java regex/escaping issues should also be fixed before this reaches a release tag, as they make the Java wrapper unreliable with real-world paths containing special characters.
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
Member
Author
|
@coderabbitai new changes, please review! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the latest state of the PR before giving you a verdict. 🏁 Script executed: Length of output: 3678 Good — let me now inspect the actual code in the key files to verify whether the previously flagged bugs have been fixed. 🏁 Script executed: Length of output: 13064 Good progress — let me check the remaining parsing helpers to complete the verification. 🏁 Script executed: Length of output: 17336
✅ All Critical/Major Bugs Resolved (from prior rounds)
🚨 Still Open — Must Fix Before Merge1. 🔴
|
| Issue | File | Notes |
|---|---|---|
| Fallback error JSON (line ~240) not escaped | main.go |
If err.Error() contains " or \n, the manually built JSON string is malformed. Use json.Marshal or fmt.Sprintf with proper escaping. |
stdout.contains("\"ok\":true") fragile substring check |
CliBridge.java |
Could spuriously match inside a string value. Use the OK_PATTERN regex already defined in the class. |
cd /workspace in README; testkit/cli path in ROADMAP |
Docs | Brittle/wrong paths — minor doc fixes. |
NewSnapshot/Payload() exported, no callers |
snapshots.go |
Dead exported API added in this PR. |
GitTestKitCLI alias not in __all__, unreferenced |
cli.py |
Dead code. |
| CI doesn't pre-build Go CLI before Python/Java tests | ci.yml |
Each go run re-compiles; one go build -o step + PATH export would speed up the suite. |
📋 Merge-Readiness Gate
| Gate | Status |
|---|---|
| CI green (Go test, spec-kit-conformance) | ✅ |
| All prior HIGH/MEDIUM bugs fixed | ✅ |
| CodeRabbit Major open items | ❌ 2 open (/dev/stdin, Python error parsing) |
| Human approval | |
| Cursor Bugbot | ⏳ Still running (most findings already fixed) |
The two remaining Major items (/dev/stdin portability and Python error-discard) are quick fixes and should be addressed before merging. Once those land and a human LGTM is recorded, this is ready to ship.
Contributor
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 `@fixtures.go`:
- Around line 87-96: The loop currently joins unchecked user-controlled
filenames onto repoPath and calls RunGitCmdE, allowing absolute paths, `..`
traversal, and `.git` mutations; fix by validating each filename before use
(implement and call a validator like validateFixturePath or safeJoin to produce
a repo-relative relPath, reject absolute paths, `..` escapes, and any top-level
`.git` component), then use filepath.Join(repoPath, relPath) for MkdirAll and
WriteFile, and call RunGitCmdE with "--" before the relPath to ensure
option-like names are treated as paths (reference the loop handling
RepoOptions.Files, the filename variable, and RunGitCmdE).
In `@snapshots.go`:
- Around line 151-173: RestoreSnapshotToDir currently skips non-dir/non-regular
tar entries which loses symlinks and special files; update the switch in
RestoreSnapshotToDir (handling header.Typeflag and targetPath) to explicitly
handle or fail on unsupported types instead of continuing: for symlinks
(tar.TypeSymlink / tar.TypeLink) recreate links using header.Linkname, for
device/other special types either return a descriptive error (including
header.Typeflag and targetPath) or implement appropriate creation, and ensure
any error bubbles up (same fmt.Errorf style) so SnapshotRepoE restores fail
loudly rather than silently dropping entries.
In `@testkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.java`:
- Around line 17-20: The test helper bridgeWithJsonResponse currently builds a
POSIX shell command string and should be replaced with a platform-agnostic test
double: stop constructing the shell string in bridgeWithJsonResponse and instead
create/inject a mock CLI invoker or stub that returns the provided JSON as the
process stdout for the CliBridge instance; modify or add a constructor/factory
on CliBridge (or use an existing injection point) to accept this fake invoker so
tests call bridgeWithJsonResponse(json) and receive a CliBridge wired to the
deterministic invoker rather than relying on "cat >/dev/null; printf ..." shell
plumbing.
🪄 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: 30b25184-7790-4007-8491-17c23e20e093
📒 Files selected for processing (11)
cmd/git-testkit-cli/main.gofixtures.gofixtures_test.gosnapshots.gotestkit/GIT_TESTKIT_SPEC.mdtestkit/README.mdtestkit/ROADMAP.mdtestkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.javatestkit/python/git_testkit/cli.pytestkit/python/tests/test_specify_conformance.py
✅ Files skipped from review due to trivial changes (3)
- testkit/README.md
- testkit/GIT_TESTKIT_SPEC.md
- testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java
🚧 Files skipped from review as they are similar to previous changes (2)
- testkit/python/tests/test_specify_conformance.py
- fixtures_test.go
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 054f0b3. Configure here.
| "unsupported snapshot entry type %d for %q", | ||
| header.Typeflag, | ||
| header.Name, | ||
| ) |
There was a problem hiding this comment.
Snapshot round-trip can fail due to asymmetric entry handling
Medium Severity
SnapshotRepoE archives all file types encountered by filepath.Walk, including symlinks (via tar.FileInfoHeader), but RestoreSnapshotToDir now hard-fails on any entry type other than TypeDir and TypeReg. This creates an asymmetry where a snapshot created from a repo containing symlinks cannot be restored, breaking the round-trip contract. The previous behavior silently skipped unsupported entries.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 054f0b3. Configure here.
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Member
Author
|
@coderabbitai review please |
Contributor
🧩 Analysis chainThe PR description images show:
🏁 Script executed: Length of output: 7463 The PR description images show:
What I checked just now
Remaining gate
Verdict
Optional tiny polish (non‑blocking)
Want me to approve? Say “@coderabbitai approve” and I’ll record an approval immediately. Otherwise, you can merge once a human LGTM is in place. ✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai approve |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (1)
83-87:⚠️ Potential issue | 🟡 MinorEnforce
gofmtin CI alongsidego vet.This job currently gates Go changes on
go vetonly, so unformatted.gofiles can still merge. Please add a formatting check here as well.🧹 Proposed fix
- name: Run vet run: go vet ./... + + - name: Check gofmt + run: | + unformatted="$(gofmt -l .)" + if [ -n "$unformatted" ]; then + printf '%s\n' "$unformatted" + exit 1 + fiAs per coding guidelines, "
**/*.go: Usegofmtfor code formatting; check format withgofmt -l *.goand fix withgofmt -w *.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 83 - 87, CI currently runs "Run vet" and "Run tests" but lacks a formatting check; add a new step (e.g., name "Run gofmt") before "Run vet" that runs gofmt in check mode (gofmt -l ./...) and fails the job if any files are unformatted (exit non-zero when output present), so the workflow enforces formatting for Go files alongside go vet and go test.snapshots.go (1)
68-75:⚠️ Potential issue | 🟠 MajorNormalize tar entry names to forward slashes.
filepath.Relreturns\-separated paths on Windows, and those go straight intoheader.Name. With the new disk-backed snapshot flow, a tarball created on Windows and restored on POSIX will produce filenames containing backslashes instead of nested paths. Tar format requires forward slashes as path separators across all platforms. Normalize once when writing the archive.🧩 Proposed fix
- header.Name = relPath + header.Name = filepath.ToSlash(relPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snapshots.go` around lines 68 - 75, The tar header names must use forward slashes; after computing relPath with filepath.Rel (and after the "." check) normalize separators by converting relPath to forward-slash form before assigning header.Name (e.g., use filepath.ToSlash(relPath)); update the assignment to set header.Name to the normalized value so Windows backslashes won't be written into the tar entries.
🧹 Nitpick comments (3)
testkit/python/git_testkit/cli.py (1)
125-127: Consider defensive handling forsnapshotSize.The Go CLI defines
SnapshotSizeas*int(nullable pointer). While the current Go implementation always populates this field for snapshot operations, directly indexingres["snapshotSize"]and passing toint()would raise an error if the value is evernullor missing.🛡️ Optional defensive fix
def save_snapshot(self, repo_path: str, snapshot_path: Path | str) -> tuple[str, int]: res = _call("snapshot_save", repoPath=repo_path, snapshotPath=str(snapshot_path)) - return str(res["snapshotName"]), int(res["snapshotSize"]) + return str(res["snapshotName"]), int(res.get("snapshotSize") or 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/python/git_testkit/cli.py` around lines 125 - 127, The save_snapshot function should defensively handle a nullable/missing snapshotSize from the CLI response: in save_snapshot, check res.get("snapshotSize") (or test if res["snapshotSize"] is None) before calling int(), and if it's None or missing return a safe default (e.g. 0) for the int in the tuple; keep returning str(res["snapshotName"]) for the name but guard snapshotSize conversion to avoid a TypeError when the Go CLI emits null or omits the field.testkit/java/src/test/java/io/gitfire/testkit/SampleRepoFlowSmoke.java (1)
13-14: Let the sample auto-detect the workspace root.Hard-coding
../..only works when Maven starts intestkit/java.CliBridge()already walks upward forgo.modand the CLI entrypoint, so using it here makes the sample runnable from IDEs and repo-root invocations too.♻️ Proposed fix
- Path workspaceRoot = Path.of("../..").toAbsolutePath().normalize(); - CliBridge bridge = new CliBridge(workspaceRoot); + CliBridge bridge = new CliBridge();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/java/src/test/java/io/gitfire/testkit/SampleRepoFlowSmoke.java` around lines 13 - 14, Replace the hard-coded workspace root Path workspaceRoot = Path.of("../..").toAbsolutePath().normalize() with a current-directory path so CliBridge can auto-detect the repo root; i.e., initialize workspaceRoot using Path.of(".") (or equivalent current-dir resolution) before calling new CliBridge(workspaceRoot) so CliBridge's upward walk for go.mod and the CLI entrypoint works from IDEs and repo-root invocations..github/workflows/ci.yml (1)
28-45: Add a quick Git preflight in the setup blocks.Both jobs invoke Git through the CLI almost immediately, so a small
git --versionstep here gives a much clearer failure than waiting for the later smoke tests to die on a PATH issue.Based on learnings, "Ensure Go 1.22+ and
gitare installed and available on PATH before running any commands".Also applies to: 100-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 28 - 45, Add quick Git and Go preflight checks immediately after each language setup step to fail fast on missing PATH tools: for the "Setup Go" block add a step named like "Check git and go versions" that runs `git --version` and `go version` (verify Go is 1.22+), and for the "Setup Python" and "Setup Java" blocks add a similar short step that runs `git --version` to ensure git is available; place these steps directly after the "Setup Go", "Setup Python", and "Setup Java" actions (referencing those step names) so CI errors surface with clear messages before later commands run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 83-87: CI currently runs "Run vet" and "Run tests" but lacks a
formatting check; add a new step (e.g., name "Run gofmt") before "Run vet" that
runs gofmt in check mode (gofmt -l ./...) and fails the job if any files are
unformatted (exit non-zero when output present), so the workflow enforces
formatting for Go files alongside go vet and go test.
In `@snapshots.go`:
- Around line 68-75: The tar header names must use forward slashes; after
computing relPath with filepath.Rel (and after the "." check) normalize
separators by converting relPath to forward-slash form before assigning
header.Name (e.g., use filepath.ToSlash(relPath)); update the assignment to set
header.Name to the normalized value so Windows backslashes won't be written into
the tar entries.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 28-45: Add quick Git and Go preflight checks immediately after
each language setup step to fail fast on missing PATH tools: for the "Setup Go"
block add a step named like "Check git and go versions" that runs `git
--version` and `go version` (verify Go is 1.22+), and for the "Setup Python" and
"Setup Java" blocks add a similar short step that runs `git --version` to ensure
git is available; place these steps directly after the "Setup Go", "Setup
Python", and "Setup Java" actions (referencing those step names) so CI errors
surface with clear messages before later commands run.
In `@testkit/java/src/test/java/io/gitfire/testkit/SampleRepoFlowSmoke.java`:
- Around line 13-14: Replace the hard-coded workspace root Path workspaceRoot =
Path.of("../..").toAbsolutePath().normalize() with a current-directory path so
CliBridge can auto-detect the repo root; i.e., initialize workspaceRoot using
Path.of(".") (or equivalent current-dir resolution) before calling new
CliBridge(workspaceRoot) so CliBridge's upward walk for go.mod and the CLI
entrypoint works from IDEs and repo-root invocations.
In `@testkit/python/git_testkit/cli.py`:
- Around line 125-127: The save_snapshot function should defensively handle a
nullable/missing snapshotSize from the CLI response: in save_snapshot, check
res.get("snapshotSize") (or test if res["snapshotSize"] is None) before calling
int(), and if it's None or missing return a safe default (e.g. 0) for the int in
the tuple; keep returning str(res["snapshotName"]) for the name but guard
snapshotSize conversion to avoid a TypeError when the Go CLI emits null or omits
the field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9de043be-fb0f-48af-a401-4353e40fff83
📒 Files selected for processing (9)
.github/workflows/ci.ymlsnapshots.gosnapshots_test.gotestkit/java/README.mdtestkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/java/src/test/java/io/gitfire/testkit/CliBridgeTest.javatestkit/java/src/test/java/io/gitfire/testkit/SampleRepoFlowSmoke.javatestkit/python/git_testkit/cli.pytestkit/python/samples/smoke_snapshot_flow.py
✅ Files skipped from review due to trivial changes (1)
- testkit/java/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- snapshots_test.go
- testkit/python/samples/smoke_snapshot_flow.py
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
92-131: Consider building the CLI binary once before wrapper tests for efficiency.Unlike the
spec-kit-conformancejob (lines 32-33), this job doesn't pre-build the CLI binary. Both Python and Java wrappers invokego run ./cmd/git-testkit-cli, which compiles the CLI on every invocation. This means the CLI is compiled twice per matrix entry (once for Python tests, once for Java tests) across three platforms.While not a correctness issue, adding a build step would reduce CI time.
⚡ Proposed fix to build CLI once
- name: Setup Java uses: actions/setup-java@v4 with: distribution: temurin java-version: "21" cache: maven + - name: Build git-testkit CLI binary + run: go build ./cmd/git-testkit-cli + - name: Run Python wrapper smoke tests shell: bash run: |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 92 - 131, The wrapper-cross-platform job currently compiles the CLI twice by using "go run ./cmd/git-testkit-cli" in both the "Run Python wrapper smoke tests" and "Run Java wrapper smoke tests" steps; add a single build step earlier in that job (e.g., run "go build -o ./bin/git-testkit-cli ./cmd/git-testkit-cli") so the CLI is built once per matrix entry, then update the "Run Python wrapper smoke tests" and "Run Java wrapper smoke tests" steps to invoke the prebuilt binary (./bin/git-testkit-cli) instead of using "go run", mirroring the approach used in the spec-kit-conformance job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 86-87: The "Check gofmt" workflow step currently only runs gofmt
on root-level files using the "*.go" glob; update that step (named "Check
gofmt") to run gofmt recursively across the repository (e.g., invoke gofmt with
a directory argument such as "." so it lists all files in subdirectories) and
keep the test that fails if any files are listed; this ensures gofmt checks all
Go files including those under cmd/, pkg/, etc.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 92-131: The wrapper-cross-platform job currently compiles the CLI
twice by using "go run ./cmd/git-testkit-cli" in both the "Run Python wrapper
smoke tests" and "Run Java wrapper smoke tests" steps; add a single build step
earlier in that job (e.g., run "go build -o ./bin/git-testkit-cli
./cmd/git-testkit-cli") so the CLI is built once per matrix entry, then update
the "Run Python wrapper smoke tests" and "Run Java wrapper smoke tests" steps to
invoke the prebuilt binary (./bin/git-testkit-cli) instead of using "go run",
mirroring the approach used in the spec-kit-conformance job.
🪄 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: 3f34cb6e-666e-4c50-bcad-834b8f8854ef
📒 Files selected for processing (2)
.github/workflows/ci.ymlscenarios.go
✅ Files skipped from review due to trivial changes (1)
- scenarios.go
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snapshots.go`:
- Around line 140-143: RestoreSnapshotToDir currently dereferences snapshot
(snapshot.name) without validating nil, causing a panic for nil inputs; update
RestoreSnapshotToDir to first check if snapshot == nil and return a clear error
(e.g., fmt.Errorf("nil snapshot")) before calling safeJoin or accessing
snapshot.name, and apply the same nil-check-and-error pattern to the other
non-testing helpers that accept *Snapshot (the "*E" variants) to avoid panics.
- Around line 52-56: The current behavior in the snapshot path silently skips
unsupported entry types (FIFOs, sockets, device files) by returning nil; change
this to return an explicit error so callers know the snapshot is lossy. In the
code path that calls supportsSnapshotEntry (the block that currently does "if
!supportsSnapshotEntry(info) { return nil }"), replace the silent nil return
with a descriptive error (e.g., using fmt.Errorf) that includes identifying info
(file path/name and file mode/type) and ensure callers of the surrounding
function propagate that error instead of assuming success; reference
supportsSnapshotEntry and the surrounding snapshot/restore routine when making
the change.
In `@testkit/python/git_testkit/cli.py`:
- Around line 86-94: The create_test_repo function currently ignores kwargs when
an explicit options object is passed; update create_test_repo to fail fast by
detecting when options is not None and kwargs is non-empty and raising an error
(e.g., TypeError or ValueError) instead of silently ignoring them; keep the
current behavior of constructing RepoOptions(**kwargs) only when options is None
and then call _call("create_test_repo", baseDir=str(base_dir),
options=options.to_payload()) as before.
- Around line 24-39: The _call function currently only catches
subprocess.TimeoutExpired; update it to also catch FileNotFoundError from
subprocess.run and raise a RuntimeError that includes the CLI path and op and
chains the original exception (using "from"). Additionally, modify _cli_cmd()
(or its caller) so any relative path returned (e.g., from the GIT_TESTKIT_CLI
env var) is resolved against _repo_root() to produce an absolute path before
passing to subprocess.run, ensuring cross-platform/CI reliability; keep using
_CLI_TIMEOUT_SECONDS in the error message and preserve existing behavior for
TimeoutExpired.
🪄 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: d68570e7-31c1-43cc-98e6-ae0d5a933639
📒 Files selected for processing (5)
.github/workflows/ci.ymlsnapshots.gosnapshots_test.gotestkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/python/git_testkit/cli.py
✅ Files skipped from review due to trivial changes (1)
- testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- snapshots_test.go
Comment on lines
+52
to
+56
| // Keep snapshot/restore symmetric: only archive entry types restore supports. | ||
| // This intentionally skips device files, sockets, and FIFOs. | ||
| if !supportsSnapshotEntry(info) { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Don't silently produce lossy snapshots for unsupported entry types.
This turns FIFOs, sockets, and device files into silent data loss: the snapshot succeeds, but a save/load/restore roundtrip no longer matches the source tree. Returning an error here is safer than pretending the snapshot is complete.
🧩 Proposed fix
- if !supportsSnapshotEntry(info) {
- return nil
- }
+ if !supportsSnapshotEntry(info) {
+ return fmt.Errorf(
+ "unsupported snapshot entry type %v for %q",
+ info.Mode()&os.ModeType,
+ path,
+ )
+ }📝 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.
Suggested change
| // Keep snapshot/restore symmetric: only archive entry types restore supports. | |
| // This intentionally skips device files, sockets, and FIFOs. | |
| if !supportsSnapshotEntry(info) { | |
| return nil | |
| } | |
| // Keep snapshot/restore symmetric: only archive entry types restore supports. | |
| // This intentionally skips device files, sockets, and FIFOs. | |
| if !supportsSnapshotEntry(info) { | |
| return fmt.Errorf( | |
| "unsupported snapshot entry type %v for %q", | |
| info.Mode()&os.ModeType, | |
| path, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@snapshots.go` around lines 52 - 56, The current behavior in the snapshot path
silently skips unsupported entry types (FIFOs, sockets, device files) by
returning nil; change this to return an explicit error so callers know the
snapshot is lossy. In the code path that calls supportsSnapshotEntry (the block
that currently does "if !supportsSnapshotEntry(info) { return nil }"), replace
the silent nil return with a descriptive error (e.g., using fmt.Errorf) that
includes identifying info (file path/name and file mode/type) and ensure callers
of the surrounding function propagate that error instead of assuming success;
reference supportsSnapshotEntry and the surrounding snapshot/restore routine
when making the change.
Comment on lines
+140
to
+143
| func RestoreSnapshotToDir(snapshot *Snapshot, baseDir string) (string, error) { | ||
| restorePath, err := safeJoin(baseDir, snapshot.name) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid snapshot name %q: %w", snapshot.name, err) |
Contributor
There was a problem hiding this comment.
Return an error for nil snapshots instead of panicking.
These new *E helpers are the reusable non-testing.T surface, but both still dereference snapshot before validating it. nil should come back as an error, not a panic.
🧩 Proposed fix
func RestoreSnapshotToDir(snapshot *Snapshot, baseDir string) (string, error) {
+ if snapshot == nil {
+ return "", fmt.Errorf("snapshot is nil")
+ }
restorePath, err := safeJoin(baseDir, snapshot.name)
if err != nil {
return "", fmt.Errorf("invalid snapshot name %q: %w", snapshot.name, err)
@@
func SaveSnapshotToDiskE(snapshot *Snapshot, filePath string) error {
+ if snapshot == nil {
+ return fmt.Errorf("snapshot is nil")
+ }
if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil {
return err
}Also applies to: 277-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@snapshots.go` around lines 140 - 143, RestoreSnapshotToDir currently
dereferences snapshot (snapshot.name) without validating nil, causing a panic
for nil inputs; update RestoreSnapshotToDir to first check if snapshot == nil
and return a clear error (e.g., fmt.Errorf("nil snapshot")) before calling
safeJoin or accessing snapshot.name, and apply the same nil-check-and-error
pattern to the other non-testing helpers that accept *Snapshot (the "*E"
variants) to avoid panics.
Comment on lines
+24
to
+39
| def _call(op: str, **payload: Any) -> dict[str, Any]: | ||
| request = {"op": op, **payload} | ||
| try: | ||
| proc = subprocess.run( | ||
| _cli_cmd(), | ||
| cwd=_repo_root(), | ||
| input=json.dumps(request), | ||
| text=True, | ||
| capture_output=True, | ||
| check=False, | ||
| timeout=_CLI_TIMEOUT_SECONDS, | ||
| ) | ||
| except subprocess.TimeoutExpired as exc: | ||
| raise RuntimeError( | ||
| f"git-testkit-cli timed out after {_CLI_TIMEOUT_SECONDS}s (op={op})" | ||
| ) from exc |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check Python wrapper command construction and subprocess usage:"
nl -ba testkit/python/git_testkit/cli.py | sed -n '13,55p'
echo
echo "Find CI wiring for git-testkit-cli path/env:"
rg -n -C2 'GIT_TESTKIT_CLI|git-testkit-cli(\.exe)?|bin/git-testkit-cli' .github workflows testkit || true
echo
echo "Check whether expected prebuilt binaries exist at repo root:"
for p in ./bin/git-testkit-cli ./bin/git-testkit-cli.exe; do
if [ -e "$p" ]; then
ls -l "$p"
else
echo "missing: $p"
fi
doneRepository: git-fire/git-testkit
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check Python wrapper command construction and subprocess usage ==="
cat -n testkit/python/git_testkit/cli.py | sed -n '13,55p'
echo
echo "=== Find CI wiring for git-testkit-cli path/env ==="
rg -n -C2 'GIT_TESTKIT_CLI|git-testkit-cli(\.exe)?|bin/git-testkit-cli' .github workflows testkit || true
echo
echo "=== Check whether expected prebuilt binaries exist at repo root ==="
for p in ./bin/git-testkit-cli ./bin/git-testkit-cli.exe; do
if [ -e "$p" ]; then
ls -l "$p"
else
echo "missing: $p"
fi
done
echo
echo "=== Check repository root structure ==="
ls -la | head -20Repository: git-fire/git-testkit
Length of output: 8883
Catch FileNotFoundError from subprocess.run and normalize relative CLI paths.
subprocess.run(...) at line 27 can raise FileNotFoundError (when the executable is missing or path is invalid), but currently only subprocess.TimeoutExpired is caught. This causes unhandled exceptions in CI on Windows and other platforms when the binary path is invalid. Additionally, relative paths provided via GIT_TESTKIT_CLI (e.g., ./bin/git-testkit-cli.exe from CI) should be resolved against the repo root for reliability across different execution contexts.
Proposed fix
def _cli_cmd() -> list[str]:
cli = os.environ.get("GIT_TESTKIT_CLI", "").strip()
if cli:
- return [cli]
+ cli_path = Path(cli)
+ if not cli_path.is_absolute():
+ cli_path = (_repo_root() / cli_path).resolve()
+ return [str(cli_path)]
return ["go", "run", "./cmd/git-testkit-cli"]
def _call(op: str, **payload: Any) -> dict[str, Any]:
request = {"op": op, **payload}
+ cmd = _cli_cmd()
try:
proc = subprocess.run(
- _cli_cmd(),
+ cmd,
cwd=_repo_root(),
input=json.dumps(request),
text=True,
capture_output=True,
check=False,
timeout=_CLI_TIMEOUT_SECONDS,
)
+ except FileNotFoundError as exc:
+ raise RuntimeError(
+ f"git-testkit-cli executable not found: {cmd[0]!r} (op={op})"
+ ) from exc
except subprocess.TimeoutExpired as exc:
raise RuntimeError(
f"git-testkit-cli timed out after {_CLI_TIMEOUT_SECONDS}s (op={op})"
) from exc🧰 Tools
🪛 GitHub Actions: CI
[error] 27-27: Command './bin/git-testkit-cli.exe' failed with FileNotFoundError: [WinError 2] The system cannot find the file specified (subprocess.run in _call).
🪛 Ruff (0.15.9)
[error] 27-27: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testkit/python/git_testkit/cli.py` around lines 24 - 39, The _call function
currently only catches subprocess.TimeoutExpired; update it to also catch
FileNotFoundError from subprocess.run and raise a RuntimeError that includes the
CLI path and op and chains the original exception (using "from"). Additionally,
modify _cli_cmd() (or its caller) so any relative path returned (e.g., from the
GIT_TESTKIT_CLI env var) is resolved against _repo_root() to produce an absolute
path before passing to subprocess.run, ensuring cross-platform/CI reliability;
keep using _CLI_TIMEOUT_SECONDS in the error message and preserve existing
behavior for TimeoutExpired.
Comment on lines
+86
to
+94
| def create_test_repo( | ||
| self, | ||
| base_dir: Path | str, | ||
| options: RepoOptions | None = None, | ||
| **kwargs: Any, | ||
| ) -> str: | ||
| if options is None: | ||
| options = RepoOptions(**kwargs) | ||
| res = _call("create_test_repo", baseDir=str(base_dir), options=options.to_payload()) |
Contributor
There was a problem hiding this comment.
Reject mixed options + kwargs to avoid silent argument loss.
If options is passed, any kwargs are currently ignored silently. Fail fast instead.
Proposed fix
def create_test_repo(
self,
base_dir: Path | str,
options: RepoOptions | None = None,
**kwargs: Any,
) -> str:
if options is None:
options = RepoOptions(**kwargs)
+ elif kwargs:
+ raise ValueError("Pass either 'options' or keyword repo options, not both")
res = _call("create_test_repo", baseDir=str(base_dir), options=options.to_payload())
return str(res["repoPath"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testkit/python/git_testkit/cli.py` around lines 86 - 94, The create_test_repo
function currently ignores kwargs when an explicit options object is passed;
update create_test_repo to fail fast by detecting when options is not None and
kwargs is non-empty and raising an error (e.g., TypeError or ValueError) instead
of silently ignoring them; keep the current behavior of constructing
RepoOptions(**kwargs) only when options is None and then call
_call("create_test_repo", baseDir=str(base_dir), options=options.to_payload())
as before.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
testkit/python/git_testkit/cli.py (2)
95-97:⚠️ Potential issue | 🟡 MinorReject mixed
optionsandkwargsto avoid silent argument loss.At Line 95, when
optionsis provided, any extra kwargs are ignored without error.Proposed fix
) -> str: if options is None: options = RepoOptions(**kwargs) + elif kwargs: + raise ValueError("Pass either 'options' or keyword repo options, not both") res = _call("create_test_repo", baseDir=str(base_dir), options=options.to_payload()) return str(res["repoPath"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/python/git_testkit/cli.py` around lines 95 - 97, The code silently ignores extra kwargs when an explicit options object is passed to the call; update the function that builds the payload (the block that calls RepoOptions(...) and _call("create_test_repo" ...)) to validate inputs: if the parameter options is not None and kwargs is non-empty, raise a ValueError (or similar) indicating mixed usage is not allowed, otherwise behave as before (construct RepoOptions(**kwargs) when options is None and call options.to_payload()). Ensure references: RepoOptions, options, kwargs, _call("create_test_repo", ...).
29-43:⚠️ Potential issue | 🟠 MajorCatch missing CLI executable and rethrow a normalized wrapper error.
Line 30 can raise
FileNotFoundError, but only timeout is handled at Line 39. That leaks a raw exception instead of wrapper-consistent failure context.Proposed fix
def _call(op: str, **payload: Any) -> dict[str, Any]: request = {"op": op, **payload} + cmd = _cli_cmd() try: proc = subprocess.run( - _cli_cmd(), + cmd, cwd=_repo_root(), input=json.dumps(request), text=True, capture_output=True, check=False, timeout=_CLI_TIMEOUT_SECONDS, ) + except FileNotFoundError as exc: + raise RuntimeError( + f"git-testkit-cli executable not found: {cmd[0]!r} (op={op})" + ) from exc except subprocess.TimeoutExpired as exc: raise RuntimeError( f"git-testkit-cli timed out after {_CLI_TIMEOUT_SECONDS}s (op={op})" ) from exc#!/bin/bash rg -n -C2 'def _call|subprocess\.run|FileNotFoundError|TimeoutExpired' testkit/python/git_testkit/cli.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testkit/python/git_testkit/cli.py` around lines 29 - 43, The subprocess.run call in the function that performs CLI calls (the block using _cli_cmd(), _repo_root(), _CLI_TIMEOUT_SECONDS and variable op) only handles TimeoutExpired and will leak FileNotFoundError if the git-testkit-cli executable is missing; wrap the subprocess.run in a try/except that also catches FileNotFoundError and re-raise a normalized RuntimeError (similar to the TimeoutExpired handler) including the op and attach the original FileNotFoundError as the cause so caller code sees a consistent wrapper error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java`:
- Around line 374-403: The current CLI invocation blocks indefinitely on
process.waitFor() and may leave child processes running on exceptional exits;
fix by declaring Process process = null before starting, create the
streamReaderPool after pb.start(), and enforce a bounded wait using
process.waitFor(timeout, TimeUnit.SECONDS) (choose a sensible timeout); if the
wait times out or any exception occurs after pb.start(), call
process.destroyForcibly() and cancel stdoutFuture/stderrFuture, then
shutdownNow() the streamReaderPool and await termination to ensure threads exit;
update the ExecutionException/IOException/InterruptedException handling and the
finally block to always clean up the started Process, futures
(stdoutFuture/stderrFuture), and the ExecutorService to avoid orphaned processes
and hangs (refer to symbols: pb.start(), process, process.waitFor,
streamReaderPool, stdoutFuture, stderrFuture).
- Around line 154-196: defaultCliCommandArgs() should accept the caller's
workspaceRoot and not wrap a resolved filesystem path in a shell invocation;
change its signature to defaultCliCommandArgs(Path workspaceRoot) and update the
constructors that call it (the ones that currently call this(workspaceRoot,
defaultCliCommandArgs(), ...) and CliBridge(Path workspaceRoot,
java.util.function.Function<String, CliResult> cliInvoker) which delegates to
that) to pass workspaceRoot. Inside defaultCliCommandArgs(Path), when
GIT_TESTKIT_CLI is set resolve it against the provided workspaceRoot (not
detectWorkspaceRoot()), and if the resolved value is a filesystem path return
List.of(resolvedPath.toString()) rather than
shellCommand(resolvedPath.toString()); keep shellCommand only for cases where
you need to execute a raw command string (e.g., when GIT_TESTKIT_CLI is not a
path).
---
Duplicate comments:
In `@testkit/python/git_testkit/cli.py`:
- Around line 95-97: The code silently ignores extra kwargs when an explicit
options object is passed to the call; update the function that builds the
payload (the block that calls RepoOptions(...) and _call("create_test_repo"
...)) to validate inputs: if the parameter options is not None and kwargs is
non-empty, raise a ValueError (or similar) indicating mixed usage is not
allowed, otherwise behave as before (construct RepoOptions(**kwargs) when
options is None and call options.to_payload()). Ensure references: RepoOptions,
options, kwargs, _call("create_test_repo", ...).
- Around line 29-43: The subprocess.run call in the function that performs CLI
calls (the block using _cli_cmd(), _repo_root(), _CLI_TIMEOUT_SECONDS and
variable op) only handles TimeoutExpired and will leak FileNotFoundError if the
git-testkit-cli executable is missing; wrap the subprocess.run in a try/except
that also catches FileNotFoundError and re-raise a normalized RuntimeError
(similar to the TimeoutExpired handler) including the op and attach the original
FileNotFoundError as the cause so caller code sees a consistent wrapper error.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f84c4ac3-d496-4d4d-ba82-b21ee8ab3db5
📒 Files selected for processing (2)
testkit/java/src/main/java/io/gitfire/testkit/CliBridge.javatestkit/python/git_testkit/cli.py
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Member
Author
|
@coderabbitai we found an issue with your change that we addressed. Please look at the pr and do a follow up review. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainThe PR description images show:
Let me pull the latest PR state and inspect what changed. 🏁 Script executed: Length of output: 30406
🔍 What HappenedThe CodeRabbit auto-fix commit ( ✅ Current Status
🟡 Remaining Low-Priority Items (non-blocking)These are all low severity and don't block merging:
📋 Merge Gate
The PR is technically merge-ready from a code-quality standpoint. The only remaining gate is a human approval — once you have that LGTM recorded, you're clear to merge. 🚀 |
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
2 participants
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
testing.Tcallers can reuse the same behaviorcmd/git-testkit-clito expose fixture/query/snapshot operations via JSON request-responsetestkit/python/samples/smoke_repo_flow.py,testkit/python/samples/smoke_snapshot_flow.py) that exercise and verify the wrapper API end-to-endSampleRepoFlowSmoke,SampleSnapshotFlowSmoke) that exercise and verify the wrapper API end-to-endtestkit/python/samples/README.md,testkit/java/README.md)testkit/.specifywith constitution, spec, plan, tasks, contract, and checklistspec-kit-conformancejobWhy
Validation plan
Summary by CodeRabbit
New Features
Fixture & Snapshot
Documentation
Tests / CI