add blocked-string rewrite validation fixtures#8
Conversation
Add reusable fixtures and assertions for coverage across file contents, commit messages, refs, and paths so sanitize/rewrite workflows can be tested consistently. Made-with: Cursor
📝 WalkthroughWalkthroughThis pull request adds testing utilities for sanitize/rewrite validation in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
bugbot-review |
![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 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Duplicated commit hash prefix in file content matches
- File content match keys now use each git grep line as-is, since output already begins with commit:path:line:content.
- ✅ Fixed: Missing dash-prefix validation causes silent false negatives
- validateBlockedRefPathToken rejects leading hyphens and runGitGrepForCommit passes the pattern with -e so git never treats it as flags.
Preview (47551d10f8)
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
--- a/DEVELOPER_GUIDE.md
+++ b/DEVELOPER_GUIDE.md
@@ -31,9 +31,11 @@
## Project structure
- `fixtures.go`: base repo creation and command helpers.
+- `sanitize_fixtures.go`: reusable sanitize/rewrite fixtures plus blocked-string scan/assert helpers.
- `scenarios.go`: fluent scenario builder and predefined multi-repo scenarios.
- `snapshots.go`: snapshot/restore utilities for expensive test setup reuse.
- `fixtures_test.go`: external package tests (`testutil_test`) that validate public API usage.
+- `sanitize_fixtures_test.go`: coverage tests for blocked-string detection/removal across content/history/refs/paths.
- `usb_fixtures.go` / `usb_fixtures_test.go`: USB volume root and `.git-fire` config helpers for backup-mode style tests.
- `scenarios_test.go`: package-internal tests for scenario/snapshot behavior.
@@ -74,7 +76,19 @@
- `CreateTestRepo`: fast setup for single-repo state.
- `NewScenario`: multi-repo topologies and fluent setup.
- `SnapshotRepo`/`RestoreSnapshot`: performance optimization for repeated expensive setups.
+- `CreateRewriteValidationFixture` + blocked-string assertions: sanitize/rewrite validation against file contents, commit messages, refs, and paths.
+Sanitize/rewrite focused validation loop:
+
+```bash
+# Run only sanitize/rewrite fixture and assertion tests.
+go test ./... -run 'RewriteValidation|BlockedStringCoverage'
+
+# Full required local gate.
+go vet ./...
+go test ./...
+```
+
## Adding new helpers
When adding new exported helpers:
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -30,6 +30,7 @@
- Repository fixtures (`CreateTestRepo`, `CreateBareRemote`, `RunGitCmd`)
- Scenario builders for common multi-repo states (`NewScenario`, conflict/worktree helpers)
- Snapshot helpers for capturing and restoring repository state in tests
+- Sanitize/rewrite validation helpers for blocked-string coverage across content/history/refs/paths
## API overview
@@ -38,6 +39,8 @@
- `NewScenario(t)` returns a fluent builder for multi-repo test topologies.
- `SnapshotRepo(t, path)` and `RestoreSnapshot(t, snap)` speed up repeated fixture setup.
- `IsDirty`, `GetCurrentSHA`, `GetBranches`, `GetRemotes` provide common assertions/helpers.
+- `CreateRewriteValidationFixture(t, opts)` seeds blocked strings across file contents, commit messages, refs, and paths.
+- `AssertBlockedStringCoverageDetected` and `AssertBlockedStringCoverageRemoved` validate sanitize/rewrite coverage before/after rewriting.
## Quickstart for using in tests
@@ -113,6 +116,33 @@
}+### Validate sanitize/rewrite coverage
+
+```go
+func TestRewriteRemovesBlockedString(t *testing.T) {
- fixture := testutil.CreateRewriteValidationFixture(t, testutil.RewriteValidationFixtureOptions{
-
Name: "rewrite-fixture", -
BlockedString: "blockedtoken", - })
- // Confirm fixture coverage before running your rewrite logic.
- testutil.AssertBlockedStringCoverageDetected(t, fixture.RepoPath, fixture.BlockedString)
- // Run your sanitizer/rewrite command against fixture.RepoPath here.
- // Example:
- // testutil.RunGitCmd(t, fixture.RepoPath, "filter-branch", ...)
- // Validate the blocked string is removed from all tracked surfaces.
- testutil.AssertBlockedStringCoverageRemoved(t, fixture.RepoPath, fixture.BlockedString)
+}
+```
+Run just sanitize/rewrite-focused tests:
+
+bash +go test ./... -run 'RewriteValidation|BlockedStringCoverage' +
+
Notes
- Snapshots are intended for deterministic test fixtures and only restore regular files/directories.
diff --git a/doc.go b/doc.go
--- a/doc.go
+++ b/doc.go
@@ -11,6 +11,8 @@
// remote, diverged clones, worktrees, conflict states).
// - [SnapshotRepo] / [RestoreSnapshot] for capturing expensive fixture state
// once and restoring it cheaply across subtests.
+// - [CreateRewriteValidationFixture] plus blocked-string assertions for
+// sanitize/rewrite validation across content, commit history, refs, and paths.
//
// All helpers integrate with Go's testing.T: repositories are created inside
// t.TempDir() and cleaned up automatically, and setup failures call t.Fatalf
diff --git a/sanitize_fixtures.go b/sanitize_fixtures.go
new file mode 100644
--- /dev/null
+++ b/sanitize_fixtures.go
@@ -1,0 +1,465 @@
+package testutil
+
+import (
- "bytes"
- "fmt"
- "os"
- "os/exec"
- "path/filepath"
- "regexp"
- "sort"
- "strings"
- "testing"
+)
+const defaultRewriteValidationFixtureName = "rewrite-validation-fixture"
+
+var blockedRefPathTokenPattern = regexp.MustCompile(^[A-Za-z0-9._-]+$)
+
+// RewriteValidationFixtureOptions configures creation of a sanitize/rewrite fixture repository.
+type RewriteValidationFixtureOptions struct {
- // Name controls the repository directory name under the fixture root.
- // Defaults to "rewrite-validation-fixture".
- Name string
- // BlockedString is the token intentionally seeded across file contents,
- // commit messages, refs, and object paths.
- //
- // The value must be ref/path safe (letters, numbers, dot, underscore, dash)
- // so it can be embedded in branch/tag names and file paths.
- BlockedString string
+}
+// RewriteValidationFixture captures important fixture metadata for rewrite tests.
+type RewriteValidationFixture struct {
- RepoPath string
- DefaultBranch string
- BlockedString string
- ContentFile string
- PathWithBlocked string
- BranchWithBlocked string
- TagWithBlocked string
+}
+// BlockedStringSurfaceMatches reports where a blocked string was found.
+type BlockedStringSurfaceMatches struct {
- BlockedString string
- FileContents []string
- CommitMessages []string
- Refs []string
- Paths []string
+}
+// IsClean reports whether no blocked-string matches were found.
+func (m BlockedStringSurfaceMatches) IsClean() bool {
- return len(m.FileContents) == 0 &&
-
len(m.CommitMessages) == 0 && -
len(m.Refs) == 0 && -
len(m.Paths) == 0
+}
+
+// MissingSurfaces reports which required surfaces did not contain a match.
+func (m BlockedStringSurfaceMatches) MissingSurfaces() []string {
- var missing []string
- if len(m.FileContents) == 0 {
-
missing = append(missing, "file-contents") - }
- if len(m.CommitMessages) == 0 {
-
missing = append(missing, "commit-messages") - }
- if len(m.Refs) == 0 {
-
missing = append(missing, "refs") - }
- if len(m.Paths) == 0 {
-
missing = append(missing, "paths") - }
- return missing
+}
+// CreateRewriteValidationFixture creates a repository seeded for sanitize/rewrite validation.
+func CreateRewriteValidationFixture(t *testing.T, opts RewriteValidationFixtureOptions) RewriteValidationFixture {
- t.Helper()
- fixture, err := CreateRewriteValidationFixtureInDir(t.TempDir(), opts)
- if err != nil {
-
t.Fatalf("Failed to create rewrite validation fixture: %v", err) - }
- return fixture
+}
+// CreateRewriteValidationFixtureInDir creates a repository seeded for sanitize/rewrite validation.
+func CreateRewriteValidationFixtureInDir(baseDir string, opts RewriteValidationFixtureOptions) (RewriteValidationFixture, error) {
- blocked := strings.TrimSpace(opts.BlockedString)
- if err := validateBlockedRefPathToken(blocked); err != nil {
-
return RewriteValidationFixture{}, fmt.Errorf("invalid blocked string %q: %w", opts.BlockedString, err) - }
- name := strings.TrimSpace(opts.Name)
- if name == "" {
-
name = defaultRewriteValidationFixtureName - }
- repoPath, err := CreateTestRepoInDir(baseDir, RepoOptions{Name: name})
- if err != nil {
-
return RewriteValidationFixture{}, err - }
- defaultBranch, err := RunGitCmdE(repoPath, "rev-parse", "--abbrev-ref", "HEAD")
- if err != nil {
-
return RewriteValidationFixture{}, err - }
- contentFile := filepath.ToSlash(filepath.Join("fixtures", "blocked-content.txt"))
- pathWithBlocked := filepath.ToSlash(filepath.Join("fixtures", blocked+"-path.txt"))
- if err := writeFixtureFile(repoPath, contentFile, "fixture blocked content: "+blocked+"\n"); err != nil {
-
return RewriteValidationFixture{}, err - }
- if err := writeFixtureFile(repoPath, pathWithBlocked, "fixture path marker\n"); err != nil {
-
return RewriteValidationFixture{}, err - }
- if _, err := RunGitCmdE(repoPath, "add", "--", contentFile, pathWithBlocked); err != nil {
-
return RewriteValidationFixture{}, err - }
- if _, err := RunGitCmdE(repoPath, "commit", "-m", "seed blocked string "+blocked+" for rewrite coverage"); err != nil {
-
return RewriteValidationFixture{}, err - }
- branchWithBlocked := "rewrite/" + blocked + "-branch"
- tagWithBlocked := "rewrite-" + blocked + "-tag"
- if _, err := RunGitCmdE(repoPath, "branch", branchWithBlocked); err != nil {
-
return RewriteValidationFixture{}, err - }
- if _, err := RunGitCmdE(repoPath, "tag", tagWithBlocked); err != nil {
-
return RewriteValidationFixture{}, err - }
- return RewriteValidationFixture{
-
RepoPath: repoPath, -
DefaultBranch: defaultBranch, -
BlockedString: blocked, -
ContentFile: contentFile, -
PathWithBlocked: pathWithBlocked, -
BranchWithBlocked: branchWithBlocked, -
TagWithBlocked: tagWithBlocked, - }, nil
+}
+// ScanBlockedStringSurfaces scans file contents, commit messages, refs, and object paths.
+func ScanBlockedStringSurfaces(t *testing.T, repoPath, blockedString string) BlockedStringSurfaceMatches {
- t.Helper()
- matches, err := ScanBlockedStringSurfacesE(repoPath, blockedString)
- if err != nil {
-
t.Fatalf("Failed to scan blocked-string surfaces: %v", err) - }
- return matches
+}
+// ScanBlockedStringSurfacesE scans file contents, commit messages, refs, and object paths.
+func ScanBlockedStringSurfacesE(repoPath, blockedString string) (BlockedStringSurfaceMatches, error) {
- blocked := strings.TrimSpace(blockedString)
- if blocked == "" {
-
return BlockedStringSurfaceMatches{}, fmt.Errorf("blocked string cannot be empty") - }
- commits, err := listAllCommits(repoPath)
- if err != nil {
-
return BlockedStringSurfaceMatches{}, err - }
- fileMatches, err := collectBlockedFileContentMatches(repoPath, commits, blocked)
- if err != nil {
-
return BlockedStringSurfaceMatches{}, err - }
- commitMatches, err := collectBlockedCommitMessageMatches(repoPath, blocked)
- if err != nil {
-
return BlockedStringSurfaceMatches{}, err - }
- refMatches, err := collectBlockedRefMatches(repoPath, blocked)
- if err != nil {
-
return BlockedStringSurfaceMatches{}, err - }
- pathMatches, err := collectBlockedPathMatches(repoPath, blocked)
- if err != nil {
-
return BlockedStringSurfaceMatches{}, err - }
- sort.Strings(fileMatches)
- sort.Strings(commitMatches)
- sort.Strings(refMatches)
- sort.Strings(pathMatches)
- return BlockedStringSurfaceMatches{
-
BlockedString: blocked, -
FileContents: fileMatches, -
CommitMessages: commitMatches, -
Refs: refMatches, -
Paths: pathMatches, - }, nil
+}
+// ValidateBlockedStringCoverageDetectedE verifies blocked-string detection covers all surfaces.
+func ValidateBlockedStringCoverageDetectedE(repoPath, blockedString string) error {
- matches, err := ScanBlockedStringSurfacesE(repoPath, blockedString)
- if err != nil {
-
return err - }
- missing := matches.MissingSurfaces()
- if len(missing) > 0 {
-
return fmt.Errorf( -
"blocked string %q missing expected surfaces: %s", -
blockedString, -
strings.Join(missing, ", "), -
) - }
- return nil
+}
+// ValidateBlockedStringCoverageRemovedE verifies blocked-string matches are gone from all surfaces.
+func ValidateBlockedStringCoverageRemovedE(repoPath, blockedString string) error {
- matches, err := ScanBlockedStringSurfacesE(repoPath, blockedString)
- if err != nil {
-
return err - }
- if matches.IsClean() {
-
return nil - }
- return fmt.Errorf("blocked string %q still present: %s", blockedString, summarizeBlockedMatches(matches))
+}
+// AssertBlockedStringCoverageDetected fails the test when any required surface is missing.
+func AssertBlockedStringCoverageDetected(t *testing.T, repoPath, blockedString string) BlockedStringSurfaceMatches {
- t.Helper()
- matches, err := ScanBlockedStringSurfacesE(repoPath, blockedString)
- if err != nil {
-
t.Fatalf("scan blocked-string surfaces: %v", err) - }
- if missing := matches.MissingSurfaces(); len(missing) > 0 {
-
t.Fatalf("blocked string %q missing expected surfaces: %s", blockedString, strings.Join(missing, ", ")) - }
- return matches
+}
+// AssertBlockedStringCoverageRemoved fails the test when any blocked-string matches remain.
+func AssertBlockedStringCoverageRemoved(t *testing.T, repoPath, blockedString string) {
- t.Helper()
- matches, err := ScanBlockedStringSurfacesE(repoPath, blockedString)
- if err != nil {
-
t.Fatalf("scan blocked-string surfaces: %v", err) - }
- if !matches.IsClean() {
-
t.Fatalf("blocked string %q still present: %s", blockedString, summarizeBlockedMatches(matches)) - }
+}
+func validateBlockedRefPathToken(token string) error {
- if token == "" {
-
return fmt.Errorf("value cannot be empty") - }
- if !blockedRefPathTokenPattern.MatchString(token) {
-
return fmt.Errorf("must contain only letters, numbers, dot, underscore, or dash") - }
- if strings.HasPrefix(token, "-") {
-
return fmt.Errorf("cannot start with hyphen") - }
- if strings.HasPrefix(token, ".") || strings.HasSuffix(token, ".") {
-
return fmt.Errorf("cannot start or end with dot") - }
- if strings.HasSuffix(token, ".lock") {
-
return fmt.Errorf("cannot end with .lock") - }
- if strings.Contains(token, "..") {
-
return fmt.Errorf("cannot contain consecutive dots") - }
- return nil
+}
+func writeFixtureFile(repoPath, relPath, content string) error {
- absPath := filepath.Join(repoPath, filepath.FromSlash(relPath))
- if err := os.MkdirAll(filepath.Dir(absPath), 0755); err != nil {
-
return fmt.Errorf("failed to create directory for %s: %w", relPath, err) - }
- if err := os.WriteFile(absPath, []byte(content), 0644); err != nil {
-
return fmt.Errorf("failed to write file %s: %w", relPath, err) - }
- return nil
+}
+func listAllCommits(repoPath string) ([]string, error) {
- output, err := RunGitCmdE(repoPath, "rev-list", "--all")
- if err != nil {
-
return nil, err - }
- return splitNonEmptyLines(output), nil
+}
+func collectBlockedFileContentMatches(repoPath string, commits []string, blocked string) ([]string, error) {
- matchSet := make(map[string]struct{})
- for _, commit := range commits {
-
lines, err := runGitGrepForCommit(repoPath, commit, blocked) -
if err != nil { -
return nil, err -
} -
for _, line := range lines { -
matchSet[line] = struct{}{} -
} - }
- return sortedKeys(matchSet), nil
+}
+func runGitGrepForCommit(repoPath, commit, blocked string) ([]string, error) {
- cmd := exec.Command("git", "grep", "-n", "-I", "-F", "-e", blocked, commit, "--")
- cmd.Dir = repoPath
- var stderr bytes.Buffer
- cmd.Stderr = &stderr
- output, err := cmd.Output()
- if err != nil {
-
if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { -
return nil, nil -
} -
return nil, fmt.Errorf( -
"git command failed: git %v\nStdout: %s\nStderr: %s\nError: %w", -
[]string{"grep", "-n", "-I", "-F", "-e", blocked, commit, "--"}, -
strings.TrimSpace(string(output)), -
strings.TrimSpace(stderr.String()), -
err, -
) - }
- return splitNonEmptyLines(string(output)), nil
+}
+func collectBlockedCommitMessageMatches(repoPath, blocked string) ([]string, error) {
- output, err := RunGitCmdE(repoPath, "log", "--all", "--format=%H%x09%B%x00")
- if err != nil {
-
return nil, err - }
- var matches []string
- for _, entry := range strings.Split(output, "\x00") {
-
entry = strings.TrimSpace(entry) -
if entry == "" { -
continue -
} -
sha, message, ok := strings.Cut(entry, "\t") -
if !ok { -
continue -
} -
if strings.Contains(message, blocked) { -
firstLine := firstNonEmptyLine(message) -
if firstLine == "" { -
firstLine = "<empty-message>" -
} -
matches = append(matches, sha+"\t"+firstLine) -
} - }
- return matches, nil
+}
+func collectBlockedRefMatches(repoPath, blocked string) ([]string, error) {
- output, err := RunGitCmdE(repoPath, "for-each-ref", "--format=%(refname)")
- if err != nil {
-
return nil, err - }
- var matches []string
- for _, line := range splitNonEmptyLines(output) {
-
if strings.Contains(line, blocked) { -
matches = append(matches, line) -
} - }
- return matches, nil
+}
+func collectBlockedPathMatches(repoPath, blocked string) ([]string, error) {
- output, err := RunGitCmdE(repoPath, "rev-list", "--all", "--objects")
- if err != nil {
-
return nil, err - }
- matchSet := make(map[string]struct{})
- for _, line := range splitNonEmptyLines(output) {
-
_, path, ok := strings.Cut(line, " ") -
if !ok { -
continue -
} -
path = strings.TrimSpace(path) -
if path == "" { -
continue -
} -
if strings.Contains(path, blocked) { -
matchSet[path] = struct{}{} -
} - }
- return sortedKeys(matchSet), nil
+}
+func summarizeBlockedMatches(matches BlockedStringSurfaceMatches) string {
- var parts []string
- if len(matches.FileContents) > 0 {
-
parts = append(parts, fmt.Sprintf("file-contents=%d [%s]", len(matches.FileContents), sampleEntries(matches.FileContents))) - }
- if len(matches.CommitMessages) > 0 {
-
parts = append(parts, fmt.Sprintf("commit-messages=%d [%s]", len(matches.CommitMessages), sampleEntries(matches.CommitMessages))) - }
- if len(matches.Refs) > 0 {
-
parts = append(parts, fmt.Sprintf("refs=%d [%s]", len(matches.Refs), sampleEntries(matches.Refs))) - }
- if len(matches.Paths) > 0 {
-
parts = append(parts, fmt.Sprintf("paths=%d [%s]", len(matches.Paths), sampleEntries(matches.Paths))) - }
- return strings.Join(parts, "; ")
+}
+func sampleEntries(values []string) string {
- const max = 3
- if len(values) == 0 {
-
return "" - }
- limit := max
- if len(values) < limit {
-
limit = len(values) - }
- sample := strings.Join(values[:limit], ", ")
- if len(values) > limit {
-
return sample + ", ..." - }
- return sample
+}
+func firstNonEmptyLine(text string) string {
- for _, line := range strings.Split(text, "\n") {
-
line = strings.TrimSpace(line) -
if line != "" { -
return line -
} - }
- return ""
+}
+func splitNonEmptyLines(output string) []string {
- rawLines := strings.Split(output, "\n")
- lines := make([]string, 0, len(rawLines))
- for _, line := range rawLines {
-
line = strings.TrimSpace(line) -
if line != "" { -
lines = append(lines, line) -
} - }
- return lines
+}
+func sortedKeys(set map[string]struct{}) []string {
- items := make([]string, 0, len(set))
- for key := range set {
-
items = append(items, key) - }
- sort.Strings(items)
- return items
+}
diff --git a/sanitize_fixtures_test.go b/sanitize_fixtures_test.go
new file mode 100644
--- /dev/null
+++ b/sanitize_fixtures_test.go
@@ -1,0 +1,112 @@
+package testutil_test
+
+import (
- "os"
- "path/filepath"
- "strings"
- "testing"
- testutil "github.com/git-fire/git-testkit"
+)
+func TestCreateRewriteValidationFixtureAndCoverageDetection(t *testing.T) {
- fixture := testutil.CreateRewriteValidationFixture(t, testutil.RewriteValidationFixtureOptions{
-
Name: "rewrite-detection", -
BlockedString: "blockedtoken", - })
- matches := testutil.AssertBlockedStringCoverageDetected(t, fixture.RepoPath, fixture.BlockedString)
- if len(matches.FileContents) == 0 {
-
t.Fatal("expected blocked string in file contents") - }
- if len(matches.CommitMessages) == 0 {
-
t.Fatal("expected blocked string in commit messages") - }
- if len(matches.Refs) == 0 {
-
t.Fatal("expected blocked string in refs") - }
- if len(matches.Paths) == 0 {
-
t.Fatal("expected blocked string in paths") - }
- if !containsSubstring(matches.Refs, fixture.BranchWithBlocked) {
-
t.Fatalf("expected blocked branch ref %q in scan results", fixture.BranchWithBlocked) - }
- if !containsSubstring(matches.Refs, fixture.TagWithBlocked) {
-
t.Fatalf("expected blocked tag ref %q in scan results", fixture.TagWithBlocked) - }
- if !containsString(matches.Paths, fixture.PathWithBlocked) {
-
t.Fatalf("expected blocked path %q in scan results", fixture.PathWithBlocked) - }
+}
+func TestAssertBlockedStringCoverageRemovedAfterHistoryRewrite(t *testing.T) {
- fixture := testutil.CreateRewriteValidationFixture(t, testutil.RewriteValidationFixtureOptions{
-
Name: "rewrite-clean", -
BlockedString: "leaktoken", - })
- repoPath := fixture.RepoPath
- testutil.RunGitCmd(t, repoPath, "checkout", "--orphan", "sanitized-history")
- testutil.RunGitCmd(t, repoPath, "rm", "-rf", "--", ".")
- cleanFile := filepath.Join(repoPath, "sanitized.txt")
- if err := os.WriteFile(cleanFile, []byte("sanitized fixture\n"), 0644); err != nil {
-
t.Fatalf("failed writing clean file: %v", err) - }
- testutil.RunGitCmd(t, repoPath, "add", "--", "sanitized.txt")
- testutil.RunGitCmd(t, repoPath, "commit", "-m", "sanitize fixture history")
- testutil.RunGitCmd(t, repoPath, "branch", "-D", fixture.DefaultBranch)
- testutil.RunGitCmd(t, repoPath, "branch", "-m", fixture.DefaultBranch)
- testutil.RunGitCmd(t, repoPath, "branch", "-D", fixture.BranchWithBlocked)
- testutil.RunGitCmd(t, repoPath, "tag", "-d", fixture.TagWithBlocked)
- testutil.AssertBlockedStringCoverageRemoved(t, repoPath, fixture.BlockedString)
- if err := testutil.ValidateBlockedStringCoverageRemovedE(repoPath, fixture.BlockedString); err != nil {
-
t.Fatalf("expected removed coverage validation to pass: %v", err) - }
+}
+func TestCreateRewriteValidationFixtureInDir_RejectsInvalidBlockedString(t *testing.T) {
- _, err := testutil.CreateRewriteValidationFixtureInDir(t.TempDir(), testutil.RewriteValidationFixtureOptions{
-
Name: "bad-blocked", -
BlockedString: "blocked token with spaces", - })
- if err == nil {
-
t.Fatal("expected invalid blocked string to be rejected") - }
+}
+func TestValidateBlockedStringCoverageDetectedE_FailsWhenCoverageMissing(t *testing.T) {
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{Name: "missing-coverage"})
- err := testutil.ValidateBlockedStringCoverageDetectedE(repoPath, "blockedtoken")
- if err == nil {
-
t.Fatal("expected missing coverage validation to fail") - }
- if !strings.Contains(err.Error(), "missing expected surfaces") {
-
t.Fatalf("expected missing coverage error, got: %v", err) - }
+}
+func containsString(values []string, target string) bool {
- for _, value := range values {
-
if value == target { -
return true -
} - }
- return false
+}
+func containsSubstring(values []string, target string) bool {
- for _, value := range values {
-
if strings.Contains(value, target) { -
return true -
} - }
- return false
+}
</details>
<sub>You can send follow-ups to the cloud agent <a href="https://cursor.com/agents/bc-fb1fd048-5e3c-4dc6-9d0b-638f1be21801">here</a>.</sub>
<!-- BUGBOT_AUTOFIX_REVIEW_FOOTNOTE_END -->
<sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7ba5e52ea58db8753bacb9bffb50b6004ea24089. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
Stop prepending the commit hash to git grep lines, which already use commit:path:line:content. Reject blocked strings that start with a hyphen so they cannot be misparsed as git grep flags, and pass patterns with -e for correct argument handling. Co-authored-by: Ben Schellenberger <TBRX103@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sanitize_fixtures_test.go (1)
74-82: Consider adding explicit test for leading-hyphen rejection.The PR commits mention "rejecting blocked strings that start with a hyphen (to avoid being parsed as git grep flags)" as a hardening measure. This test covers spaces (invalid characters), but an explicit test for leading hyphen would ensure this security-relevant validation remains intact.
🧪 Suggested additional test case
func TestCreateRewriteValidationFixtureInDir_RejectsLeadingHyphen(t *testing.T) { _, err := testutil.CreateRewriteValidationFixtureInDir(t.TempDir(), testutil.RewriteValidationFixtureOptions{ Name: "hyphen-blocked", BlockedString: "-blockedtoken", }) if err == nil { t.Fatal("expected leading-hyphen blocked string to be rejected") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sanitize_fixtures_test.go` around lines 74 - 82, Add an explicit unit test that asserts CreateRewriteValidationFixtureInDir rejects blocked strings that begin with a hyphen: create a new test function (e.g., TestCreateRewriteValidationFixtureInDir_RejectsLeadingHyphen) that calls testutil.CreateRewriteValidationFixtureInDir with RewriteValidationFixtureOptions{ Name: "hyphen-blocked", BlockedString: "-blockedtoken" } and fails the test if err == nil; this ensures the leading-hyphen validation added to CreateRewriteValidationFixtureInDir is covered and won't regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sanitize_fixtures_test.go`:
- Around line 74-82: Add an explicit unit test that asserts
CreateRewriteValidationFixtureInDir rejects blocked strings that begin with a
hyphen: create a new test function (e.g.,
TestCreateRewriteValidationFixtureInDir_RejectsLeadingHyphen) that calls
testutil.CreateRewriteValidationFixtureInDir with
RewriteValidationFixtureOptions{ Name: "hyphen-blocked", BlockedString:
"-blockedtoken" } and fails the test if err == nil; this ensures the
leading-hyphen validation added to CreateRewriteValidationFixtureInDir is
covered and won't regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c3a0a25-8f33-4848-a685-67ba6316801e
📒 Files selected for processing (5)
DEVELOPER_GUIDE.mdREADME.mddoc.gosanitize_fixtures.gosanitize_fixtures_test.go
|
@cursoragent address the nit feedback from coderabbit Verify each finding against the current code and only fix it if needed. In |
|
Verification: Change: Added func TestCreateRewriteValidationFixtureInDir_RejectsLeadingHyphen(t *testing.T) {
_, err := testutil.CreateRewriteValidationFixtureInDir(t.TempDir(), testutil.RewriteValidationFixtureOptions{
Name: "hyphen-blocked",
BlockedString: "-blockedtoken",
})
if err == nil {
t.Fatal("expected leading-hyphen blocked string to be rejected")
}
}Checks:   |
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
2 participants
Summary
Test plan
go test ./... -run 'RewriteValidation|BlockedStringCoverage'go test ./...Made with Cursor
Note
Medium Risk
Adds new exported helpers that execute git commands and parse repo history/refs, so edge cases and performance of scans could affect downstream tests; changes are otherwise additive and well-covered by new tests.
Overview
Adds a new sanitize/rewrite validation fixture (
CreateRewriteValidationFixture) that seeds a repo with a configurable blocked token across file content, commit messages, refs (branch/tag), and object paths.Introduces scan/validate/assert helpers (
ScanBlockedStringSurfaces*,ValidateBlockedStringCoverage*,AssertBlockedStringCoverage*) to verify blocked-string detection coverage pre-rewrite and confirm complete removal post-rewrite.Updates
README.md,DEVELOPER_GUIDE.md, and package docs with usage examples and a focused test command, and addssanitize_fixtures_test.goto exercise fixture creation, invalid token rejection, and a simulated history rewrite cleanup flow.Reviewed by Cursor Bugbot for commit dc8e18b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
New Features
Tests