From f3f713d77a1a4bcdb6ccce5ccf176b9b63b57e90 Mon Sep 17 00:00:00 2001 From: Peter Chanthamynavong Date: Thu, 1 Jan 2026 10:51:22 -0800 Subject: [PATCH] fix(fork-protection): only apply protection to actual beads forks (#823) (#828) The fork protection logic incorrectly treated all repos where origin != steveyegge/beads as forks, including user's own projects that just use beads as a tool. Changes: - Add isForkOfBeads() that scans ALL remotes for steveyegge/beads - Only apply protection when a beads-related remote exists - Add git config opt-out: `git config beads.fork-protection false` (per-clone, never tracked, matches beads.role pattern) Test coverage for 8 scenarios plus edge cases for config values. --- cmd/bd/fork_protection.go | 47 ++++++- cmd/bd/fork_protection_test.go | 222 +++++++++++++++++++++++++++++++++ internal/config/yaml_config.go | 4 +- 3 files changed, 268 insertions(+), 5 deletions(-) diff --git a/cmd/bd/fork_protection.go b/cmd/bd/fork_protection.go index 9ea4f5d9..c0c701f1 100644 --- a/cmd/bd/fork_protection.go +++ b/cmd/bd/fork_protection.go @@ -13,21 +13,36 @@ import ( // ensureForkProtection prevents contributors from accidentally committing // the upstream issue database when working in a fork. // -// When we detect this is a fork (origin != steveyegge/beads), we add -// .beads/issues.jsonl to .git/info/exclude so it won't be staged. +// When we detect this is a fork (any remote points to steveyegge/beads), +// we add .beads/issues.jsonl to .git/info/exclude so it won't be staged. // This is a per-clone setting that doesn't modify tracked files. +// +// Users can disable this with: git config beads.fork-protection false func ensureForkProtection() { - // Find git root + // Find git root first (needed for git config check) gitRoot := git.GetRepoRoot() if gitRoot == "" { return // Not in a git repo } + // Check if fork protection is explicitly disabled via git config (GH#823) + // Use: git config beads.fork-protection false + if isForkProtectionDisabled(gitRoot) { + debug.Printf("fork protection: disabled via git config") + return + } + // Check if this is the upstream repo (maintainers) if isUpstreamRepo(gitRoot) { return // Maintainers can commit issues.jsonl } + // Only protect actual forks - repos with any remote pointing to beads (GH#823) + // This prevents false positives on user's own projects that just use beads + if !isForkOfBeads(gitRoot) { + return // Not a fork of beads, user's own project + } + // Check if already excluded excludePath := filepath.Join(gitRoot, ".git", "info", "exclude") if isAlreadyExcluded(excludePath) { @@ -69,6 +84,32 @@ func isUpstreamRepo(gitRoot string) bool { return false } +// isForkOfBeads checks if ANY remote points to steveyegge/beads. +// This handles any remote naming convention (origin, upstream, github, etc.) +// and correctly identifies actual beads forks vs user's own projects. (GH#823) +func isForkOfBeads(gitRoot string) bool { + cmd := exec.Command("git", "-C", gitRoot, "remote", "-v") + out, err := cmd.Output() + if err != nil { + return false // No remotes or git error - not a fork + } + + // If any remote URL contains steveyegge/beads, this is a beads-related repo + return strings.Contains(string(out), "steveyegge/beads") +} + +// isForkProtectionDisabled checks if fork protection is disabled via git config. +// Users can opt out with: git config beads.fork-protection false +// Only exact "false" disables; any other value or unset means enabled. +func isForkProtectionDisabled(gitRoot string) bool { + cmd := exec.Command("git", "-C", gitRoot, "config", "--get", "beads.fork-protection") + out, err := cmd.Output() + if err != nil { + return false // Not set or error - default to enabled + } + return strings.TrimSpace(string(out)) == "false" +} + // isAlreadyExcluded checks if issues.jsonl is already in the exclude file func isAlreadyExcluded(excludePath string) bool { content, err := os.ReadFile(excludePath) //nolint:gosec // G304: path is constructed from git root, not user input diff --git a/cmd/bd/fork_protection_test.go b/cmd/bd/fork_protection_test.go index a1ed9e27..4a326295 100644 --- a/cmd/bd/fork_protection_test.go +++ b/cmd/bd/fork_protection_test.go @@ -2,11 +2,56 @@ package main import ( "os" + "os/exec" "path/filepath" "strings" "testing" ) +// setupGitRepoForForkTest creates a temporary git repository for testing +func setupGitRepoForForkTest(t *testing.T) string { + t.Helper() + dir := t.TempDir() + + // Create .beads directory + beadsDir := filepath.Join(dir, ".beads") + if err := os.MkdirAll(beadsDir, 0755); err != nil { + t.Fatalf("failed to create .beads directory: %v", err) + } + + // Initialize git repo + cmd := exec.Command("git", "init", "--initial-branch=main") + cmd.Dir = dir + if err := cmd.Run(); err != nil { + t.Fatalf("failed to init git repo: %v", err) + } + + // Configure git user + cmd = exec.Command("git", "config", "user.email", "test@test.com") + cmd.Dir = dir + _ = cmd.Run() + + cmd = exec.Command("git", "config", "user.name", "Test User") + cmd.Dir = dir + _ = cmd.Run() + + return dir +} + +// addRemote adds a git remote to the test repo +func addRemote(t *testing.T, dir, name, url string) { + t.Helper() + cmd := exec.Command("git", "remote", "add", name, url) + cmd.Dir = dir + if err := cmd.Run(); err != nil { + t.Fatalf("failed to add remote %s: %v", name, err) + } +} + +// ============================================================================ +// Test isUpstreamRepo (existing tests, updated) +// ============================================================================ + func TestIsUpstreamRepo(t *testing.T) { tests := []struct { name string @@ -45,6 +90,118 @@ func TestIsUpstreamRepo(t *testing.T) { } } +// Test 1: Upstream maintainer (origin = steveyegge/beads) +func TestIsUpstreamRepo_Maintainer(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/steveyegge/beads.git") + + if !isUpstreamRepo(dir) { + t.Error("expected isUpstreamRepo to return true for steveyegge/beads") + } +} + +// Test 1b: Upstream maintainer with SSH URL +func TestIsUpstreamRepo_MaintainerSSH(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "git@github.com:steveyegge/beads.git") + + if !isUpstreamRepo(dir) { + t.Error("expected isUpstreamRepo to return true for SSH steveyegge/beads") + } +} + +// Test isUpstreamRepo with non-beads origin +func TestIsUpstreamRepo_NotUpstream(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/peterkc/beads.git") + + if isUpstreamRepo(dir) { + t.Error("expected isUpstreamRepo to return false for fork origin") + } +} + +// Test isUpstreamRepo with no origin +func TestIsUpstreamRepo_NoOrigin(t *testing.T) { + dir := setupGitRepoForForkTest(t) + // Don't add origin remote + + if isUpstreamRepo(dir) { + t.Error("expected isUpstreamRepo to return false when no origin exists") + } +} + +// ============================================================================ +// Test isForkOfBeads (new tests for GH#823) +// ============================================================================ + +// Test 2: Fork (standard) - origin=fork, upstream=beads +func TestIsForkOfBeads_StandardFork(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/peterkc/beads.git") + addRemote(t, dir, "upstream", "https://github.com/steveyegge/beads.git") + + if !isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return true for standard fork setup") + } +} + +// Test 3: Fork (custom naming) - origin=fork, github=beads +func TestIsForkOfBeads_CustomNaming(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/peterkc/beads.git") + addRemote(t, dir, "github", "https://github.com/steveyegge/beads.git") + + if !isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return true for custom remote naming") + } +} + +// Test 4: User's own project (no beads remote) - THE BUG CASE +func TestIsForkOfBeads_UserProject(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/mycompany/myapp.git") + + if isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return false for user's own project") + } +} + +// Test 5: User's project with unrelated upstream - THE BUG CASE +func TestIsForkOfBeads_UserProjectWithUpstream(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/mycompany/myapp.git") + addRemote(t, dir, "upstream", "https://github.com/other/repo.git") + + if isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return false for user's project with unrelated upstream") + } +} + +// Test 6: No remotes +func TestIsForkOfBeads_NoRemotes(t *testing.T) { + dir := setupGitRepoForForkTest(t) + // Don't add any remotes + + if isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return false when no remotes exist") + } +} + +// Test SSH URL detection +func TestIsForkOfBeads_SSHRemote(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "git@github.com:peterkc/beads.git") + addRemote(t, dir, "upstream", "git@github.com:steveyegge/beads.git") + + if !isForkOfBeads(dir) { + t.Error("expected isForkOfBeads to return true for SSH upstream") + } +} + +// ============================================================================ +// Test isAlreadyExcluded (existing tests) +// ============================================================================ + func TestIsAlreadyExcluded(t *testing.T) { // Create temp file with exclusion tmpDir := t.TempDir() @@ -72,6 +229,10 @@ func TestIsAlreadyExcluded(t *testing.T) { } } +// ============================================================================ +// Test addToExclude (existing tests) +// ============================================================================ + func TestAddToExclude(t *testing.T) { tmpDir := t.TempDir() infoDir := filepath.Join(tmpDir, ".git", "info") @@ -111,3 +272,64 @@ func TestAddToExclude(t *testing.T) { t.Errorf("exclude file missing .beads/issues.jsonl: %s", content) } } + +// ============================================================================ +// Test isForkProtectionDisabled (git config opt-out) +// ============================================================================ + +// Test isForkProtectionDisabled with various git config values +func TestIsForkProtectionDisabled(t *testing.T) { + tests := []struct { + name string + config string // value to set, empty = don't set + expected bool + }{ + {"not set", "", false}, + {"set to false", "false", true}, + {"set to true", "true", false}, + {"set to other", "disabled", false}, // only "false" disables + {"set to FALSE", "FALSE", false}, // case-sensitive + {"set to 0", "0", false}, // only "false" disables + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := setupGitRepoForForkTest(t) + + if tt.config != "" { + cmd := exec.Command("git", "-C", dir, "config", "beads.fork-protection", tt.config) + if err := cmd.Run(); err != nil { + t.Fatalf("failed to set git config: %v", err) + } + } + + result := isForkProtectionDisabled(dir) + if result != tt.expected { + t.Errorf("isForkProtectionDisabled() = %v, want %v (config=%q)", result, tt.expected, tt.config) + } + }) + } +} + +// Test 8: Config opt-out via git config (replaces YAML config) +func TestConfigOptOut_GitConfig(t *testing.T) { + dir := setupGitRepoForForkTest(t) + addRemote(t, dir, "origin", "https://github.com/peterkc/beads.git") + addRemote(t, dir, "upstream", "https://github.com/steveyegge/beads.git") + + // Verify this IS a fork of beads + if !isForkOfBeads(dir) { + t.Fatal("expected isForkOfBeads to return true for test setup") + } + + // Set git config opt-out + cmd := exec.Command("git", "-C", dir, "config", "beads.fork-protection", "false") + if err := cmd.Run(); err != nil { + t.Fatalf("failed to set git config: %v", err) + } + + // Verify opt-out is detected + if !isForkProtectionDisabled(dir) { + t.Error("expected isForkProtectionDisabled to return true after setting git config") + } +} diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index df473dd3..f991d88d 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -38,8 +38,8 @@ var YamlOnlyKeys = map[string]bool{ // Git settings "git.author": true, "git.no-gpg-sign": true, - "no-push": true, - "no-git-ops": true, // Disable git ops in bd prime session close protocol (GH#593) + "no-push": true, + "no-git-ops": true, // Disable git ops in bd prime session close protocol (GH#593) // Sync settings "sync-branch": true,