From 22756509ccf6fd04d3aff45329fd0e1a59a9d810 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Sun, 2 Nov 2025 18:51:25 -0800 Subject: [PATCH] Refactor: Extract path canonicalization into utils.CanonicalizePath() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts duplicated path canonicalization logic (filepath.Abs + EvalSymlinks) into a reusable helper function utils.CanonicalizePath() in internal/utils/path.go. Changes: - Add internal/utils/path.go with CanonicalizePath() function - Add comprehensive tests in internal/utils/path_test.go - Replace inline canonicalization in beads.go:131-140 - Replace inline canonicalization in cmd/bd/main.go:446-454 - Replace inline canonicalization in cmd/bd/nodb.go:25-33 The new helper maintains identical behavior: 1. Converts path to absolute form via filepath.Abs 2. Resolves symlinks via filepath.EvalSymlinks 3. Falls back gracefully on errors (returns absPath if EvalSymlinks fails, returns original path if Abs fails) Fixes bd-efe8 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- beads.go | 12 +---- cmd/bd/main.go | 11 +--- cmd/bd/nodb.go | 11 +--- internal/utils/path.go | 34 ++++++++++++ internal/utils/path_test.go | 105 ++++++++++++++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 28 deletions(-) create mode 100644 internal/utils/path.go create mode 100644 internal/utils/path_test.go diff --git a/beads.go b/beads.go index 99692cc5..15ea4ea3 100644 --- a/beads.go +++ b/beads.go @@ -18,6 +18,7 @@ import ( "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/storage/sqlite" "github.com/steveyegge/beads/internal/types" + "github.com/steveyegge/beads/internal/utils" ) // CanonicalDatabaseName is the required database filename for all beads repositories @@ -127,16 +128,7 @@ func FindDatabasePath() string { // 1. Check BEADS_DIR environment variable (preferred) if beadsDir := os.Getenv("BEADS_DIR"); beadsDir != "" { // Canonicalize the path to prevent nested .beads directories - var absBeadsDir string - if absPath, err := filepath.Abs(beadsDir); err == nil { - if canonical, err := filepath.EvalSymlinks(absPath); err == nil { - absBeadsDir = canonical - } else { - absBeadsDir = absPath - } - } else { - absBeadsDir = beadsDir - } + absBeadsDir := utils.CanonicalizePath(beadsDir) // Check for config.json first (single source of truth) if cfg, err := configfile.Load(absBeadsDir); err == nil && cfg != nil { diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 0277526e..0beef33d 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -15,6 +15,7 @@ import ( "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/storage/memory" "github.com/steveyegge/beads/internal/storage/sqlite" + "github.com/steveyegge/beads/internal/utils" ) // DaemonStatus captures daemon connection state for the current command @@ -443,15 +444,7 @@ var rootCmd = &cobra.Command{ var beadsDir string if envDir := os.Getenv("BEADS_DIR"); envDir != "" { // Canonicalize the path - if absDir, err := filepath.Abs(envDir); err == nil { - if canonical, err := filepath.EvalSymlinks(absDir); err == nil { - beadsDir = canonical - } else { - beadsDir = absDir - } - } else { - beadsDir = envDir - } + beadsDir = utils.CanonicalizePath(envDir) } else { // Fall back to current directory cwd, err := os.Getwd() diff --git a/cmd/bd/nodb.go b/cmd/bd/nodb.go index eb11a7dc..f3cc0eb1 100644 --- a/cmd/bd/nodb.go +++ b/cmd/bd/nodb.go @@ -12,6 +12,7 @@ import ( "github.com/steveyegge/beads/internal/config" "github.com/steveyegge/beads/internal/storage/memory" "github.com/steveyegge/beads/internal/types" + "github.com/steveyegge/beads/internal/utils" ) // initializeNoDbMode sets up in-memory storage from JSONL file @@ -23,15 +24,7 @@ func initializeNoDbMode() error { // Check BEADS_DIR environment variable first if envDir := os.Getenv("BEADS_DIR"); envDir != "" { // Canonicalize the path - if absDir, err := filepath.Abs(envDir); err == nil { - if canonical, err := filepath.EvalSymlinks(absDir); err == nil { - beadsDir = canonical - } else { - beadsDir = absDir - } - } else { - beadsDir = envDir - } + beadsDir = utils.CanonicalizePath(envDir) } else { // Fall back to current directory cwd, err := os.Getwd() diff --git a/internal/utils/path.go b/internal/utils/path.go new file mode 100644 index 00000000..b3979909 --- /dev/null +++ b/internal/utils/path.go @@ -0,0 +1,34 @@ +// Package utils provides utility functions for issue ID parsing and path handling. +package utils + +import ( + "path/filepath" +) + +// CanonicalizePath converts a path to its canonical form by: +// 1. Converting to absolute path +// 2. Resolving symlinks +// +// If either step fails, it falls back to the best available form: +// - If symlink resolution fails, returns absolute path +// - If absolute path conversion fails, returns original path +// +// This function is used to ensure consistent path handling across the codebase, +// particularly for BEADS_DIR environment variable processing. +func CanonicalizePath(path string) string { + // Try to get absolute path + absPath, err := filepath.Abs(path) + if err != nil { + // If we can't get absolute path, return original + return path + } + + // Try to resolve symlinks + canonical, err := filepath.EvalSymlinks(absPath) + if err != nil { + // If we can't resolve symlinks, return absolute path + return absPath + } + + return canonical +} diff --git a/internal/utils/path_test.go b/internal/utils/path_test.go new file mode 100644 index 00000000..364f6474 --- /dev/null +++ b/internal/utils/path_test.go @@ -0,0 +1,105 @@ +package utils + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCanonicalizePath(t *testing.T) { + tests := []struct { + name string + input string + validate func(t *testing.T, result string) + }{ + { + name: "absolute path", + input: "/tmp/test", + validate: func(t *testing.T, result string) { + if !filepath.IsAbs(result) { + t.Errorf("expected absolute path, got %q", result) + } + }, + }, + { + name: "relative path", + input: ".", + validate: func(t *testing.T, result string) { + if !filepath.IsAbs(result) { + t.Errorf("expected absolute path, got %q", result) + } + }, + }, + { + name: "current directory", + input: ".", + validate: func(t *testing.T, result string) { + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("failed to get cwd: %v", err) + } + // Result should be canonical form of current directory + if !filepath.IsAbs(result) { + t.Errorf("expected absolute path, got %q", result) + } + // The result should be related to cwd (could be same or canonical version) + if result != cwd { + // Try to canonicalize cwd to compare + canonicalCwd, err := filepath.EvalSymlinks(cwd) + if err == nil && result != canonicalCwd { + t.Errorf("expected %q or %q, got %q", cwd, canonicalCwd, result) + } + } + }, + }, + { + name: "empty path", + input: "", + validate: func(t *testing.T, result string) { + // Empty path should be handled (likely becomes "." then current dir) + if result == "" { + t.Error("expected non-empty result for empty input") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := CanonicalizePath(tt.input) + tt.validate(t, result) + }) + } +} + +func TestCanonicalizePathSymlink(t *testing.T) { + // Create a temporary directory + tmpDir := t.TempDir() + + // Create a test file + testFile := filepath.Join(tmpDir, "test.txt") + if err := os.WriteFile(testFile, []byte("test"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + // Create a symlink to the temp directory + symlinkPath := filepath.Join(tmpDir, "link") + if err := os.Symlink(tmpDir, symlinkPath); err != nil { + t.Skipf("failed to create symlink (may not be supported): %v", err) + } + + // Canonicalize the symlink path + result := CanonicalizePath(symlinkPath) + + // The result should be the resolved path (tmpDir), not the symlink + if result != tmpDir { + // Try to get canonical form of tmpDir for comparison + canonicalTmpDir, err := filepath.EvalSymlinks(tmpDir) + if err != nil { + t.Fatalf("failed to canonicalize tmpDir: %v", err) + } + if result != canonicalTmpDir { + t.Errorf("expected %q or %q, got %q", tmpDir, canonicalTmpDir, result) + } + } +}