fix: NixOS symlink compatibility for mtime and permission checks (#459)

* fix: use os.Lstat for symlink-safe mtime and permission checks On NixOS and other systems using symlinks heavily (e.g., home-manager), os.Stat follows symlinks and returns the target's metadata. This causes: 1. False staleness detection when JSONL is symlinked - mtime of target changes unpredictably when symlinks are recreated 2. os.Chmod failing or changing wrong file's permissions when target is in read-only location (e.g., /nix/store) 3. os.Chtimes modifying target's times instead of the symlink itself Changes: - autoimport.go: Use Lstat for JSONL mtime in CheckStaleness() - import.go: Use Lstat in TouchDatabaseFile() for JSONL mtime - export.go: Skip chmod for symlinked files - multirepo.go: Use Lstat for JSONL mtime cache - multirepo_export.go: Use Lstat for mtime, skip chmod for symlinks - doctor/fix/permissions.go: Skip permission fixes for symlinked paths These changes are safe cross-platform: - On systems without symlinks, Lstat behaves identically to Stat - Symlink permission bits are ignored on Unix anyway - The extra Lstat syscall overhead is negligible Fixes symlink-related data loss on NixOS. See GitHub issue #379. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test: add symlink behavior tests for NixOS compatibility Add tests that verify symlink handling behavior: - TestCheckStaleness_SymlinkedJSONL: verifies mtime detection uses symlink's own mtime (os.Lstat), not target's mtime (os.Stat) - TestPermissions_SkipsSymlinkedBeadsDir: verifies chmod is skipped for symlinked .beads directories - TestPermissions_SkipsSymlinkedDatabase: verifies chmod is skipped for symlinked database files while still fixing .beads dir perms Also adds devShell to flake.nix for local development with go, gopls, golangci-lint, and sqlite tools. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
2025-12-05 13:22:09 -08:00
parent 07c97a2b74
commit e0734e230f
9 changed files with 353 additions and 11 deletions
--- a/cmd/bd/doctor/fix/permissions.go
+++ b/cmd/bd/doctor/fix/permissions.go
@@ -16,11 +16,18 @@ func Permissions(path string) error {
 	beadsDir := filepath.Join(path, ".beads")

 	// Check if .beads/ directory exists
-	info, err := os.Stat(beadsDir)
+	// Use Lstat to detect symlinks - we shouldn't chmod symlinked directories
+	// as this would change the target's permissions (problematic on NixOS).
+	info, err := os.Lstat(beadsDir)
 	if err != nil {
 		return fmt.Errorf("failed to stat .beads directory: %w", err)
 	}

+	// Skip permission fixes for symlinked .beads directories (common on NixOS with home-manager)
+	if info.Mode()&os.ModeSymlink != 0 {
+		return nil // Symlink permissions are not meaningful on Unix
+	}
+
 	// Ensure .beads directory has exactly 0700 permissions (owner rwx only)
 	expectedDirMode := os.FileMode(0700)
 	if info.Mode().Perm() != expectedDirMode {
@@ -30,8 +37,14 @@ func Permissions(path string) error {
 	}

 	// Fix permissions on database file if it exists
+	// Use Lstat to detect symlinks - skip chmod for symlinked database files
 	dbPath := filepath.Join(beadsDir, "beads.db")
-	if dbInfo, err := os.Stat(dbPath); err == nil {
+	if dbInfo, err := os.Lstat(dbPath); err == nil {
+		// Skip permission fixes for symlinked database files (NixOS)
+		if dbInfo.Mode()&os.ModeSymlink != 0 {
+			return nil
+		}
+
 		// Ensure database has exactly 0600 permissions (owner rw only)
 		expectedFileMode := os.FileMode(0600)
 		currentPerms := dbInfo.Mode().Perm()
--- a/cmd/bd/doctor/fix/symlink_test.go
+++ b/cmd/bd/doctor/fix/symlink_test.go
@@ -0,0 +1,165 @@
+package fix
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// TestPermissions_SkipsSymlinkedBeadsDir verifies that permission fixes are skipped
+// when .beads directory is a symlink (common on NixOS with home-manager).
+//
+// Behavior being tested:
+// - When .beads is a symlink, Permissions() should return nil without changing anything
+// - This prevents attempts to chmod symlink targets (which may be read-only like /nix/store)
+func TestPermissions_SkipsSymlinkedBeadsDir(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Create target .beads directory with wrong permissions
+	targetDir := filepath.Join(tmpDir, "target-beads")
+	if err := os.MkdirAll(targetDir, 0777); err != nil { // intentionally wrong permissions
+		t.Fatal(err)
+	}
+
+	// Create workspace with symlinked .beads
+	workspaceDir := filepath.Join(tmpDir, "workspace")
+	if err := os.MkdirAll(workspaceDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	symlinkPath := filepath.Join(workspaceDir, ".beads")
+	if err := os.Symlink(targetDir, symlinkPath); err != nil {
+		t.Fatal(err)
+	}
+
+	// Get target's permissions before fix
+	targetInfoBefore, err := os.Stat(targetDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	permsBefore := targetInfoBefore.Mode().Perm()
+
+	// Run Permissions fix
+	err = Permissions(workspaceDir)
+	if err != nil {
+		t.Fatalf("Permissions() returned error for symlinked .beads: %v", err)
+	}
+
+	// Verify target's permissions were NOT changed
+	targetInfoAfter, err := os.Stat(targetDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	permsAfter := targetInfoAfter.Mode().Perm()
+
+	if permsAfter != permsBefore {
+		t.Errorf("Target directory permissions were changed through symlink!")
+		t.Errorf("Before: %o, After: %o", permsBefore, permsAfter)
+		t.Error("This could cause issues on NixOS where target may be in /nix/store (read-only)")
+	}
+}
+
+// TestPermissions_SkipsSymlinkedDatabase verifies that chmod is skipped for
+// symlinked database files, but .beads directory permissions are still fixed.
+func TestPermissions_SkipsSymlinkedDatabase(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Create real .beads directory with wrong permissions
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	if err := os.MkdirAll(beadsDir, 0777); err != nil { // intentionally wrong
+		t.Fatal(err)
+	}
+
+	// Create target database file with wrong permissions
+	targetDir := filepath.Join(tmpDir, "target")
+	if err := os.MkdirAll(targetDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	targetDB := filepath.Join(targetDir, "beads.db")
+	if err := os.WriteFile(targetDB, []byte("test"), 0644); err != nil { // intentionally world-readable
+		t.Fatal(err)
+	}
+
+	// Create symlink to database
+	symlinkPath := filepath.Join(beadsDir, "beads.db")
+	if err := os.Symlink(targetDB, symlinkPath); err != nil {
+		t.Fatal(err)
+	}
+
+	// Get target's permissions before fix
+	targetInfoBefore, err := os.Stat(targetDB)
+	if err != nil {
+		t.Fatal(err)
+	}
+	permsBefore := targetInfoBefore.Mode().Perm()
+
+	// Run Permissions fix
+	err = Permissions(tmpDir)
+	if err != nil {
+		t.Fatalf("Permissions() returned error for symlinked database: %v", err)
+	}
+
+	// Verify .beads directory permissions WERE fixed (not a symlink)
+	beadsInfo, err := os.Stat(beadsDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if beadsInfo.Mode().Perm() != 0700 {
+		t.Errorf("Expected .beads to have 0700 permissions, got %o", beadsInfo.Mode().Perm())
+	}
+
+	// Verify target database permissions were NOT changed (it's a symlink)
+	targetInfoAfter, err := os.Stat(targetDB)
+	if err != nil {
+		t.Fatal(err)
+	}
+	permsAfter := targetInfoAfter.Mode().Perm()
+
+	if permsAfter != permsBefore {
+		t.Errorf("Target database permissions were changed through symlink!")
+		t.Errorf("Before: %o, After: %o", permsBefore, permsAfter)
+		t.Error("chmod should not be called on symlinked files")
+	}
+}
+
+// TestPermissions_FixesRegularFiles verifies that permissions ARE fixed for
+// regular (non-symlinked) files.
+func TestPermissions_FixesRegularFiles(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Create .beads directory with wrong permissions
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	if err := os.MkdirAll(beadsDir, 0777); err != nil { // intentionally wrong
+		t.Fatal(err)
+	}
+
+	// Create database with wrong permissions
+	dbPath := filepath.Join(beadsDir, "beads.db")
+	if err := os.WriteFile(dbPath, []byte("test"), 0644); err != nil { // intentionally world-readable
+		t.Fatal(err)
+	}
+
+	// Run Permissions fix
+	err := Permissions(tmpDir)
+	if err != nil {
+		t.Fatalf("Permissions() failed: %v", err)
+	}
+
+	// Verify .beads directory now has 0700
+	beadsInfo, err := os.Stat(beadsDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if beadsInfo.Mode().Perm() != 0700 {
+		t.Errorf("Expected .beads to have 0700 permissions, got %o", beadsInfo.Mode().Perm())
+	}
+
+	// Verify database now has at least 0600 (read/write for owner)
+	dbInfo, err := os.Stat(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if dbInfo.Mode().Perm()&0600 != 0600 {
+		t.Errorf("Expected database to have at least 0600 permissions, got %o", dbInfo.Mode().Perm())
+	}
+}
--- a/cmd/bd/export.go
+++ b/cmd/bd/export.go
@@ -455,8 +455,12 @@ Examples:
 			}

 			// Set appropriate file permissions (0600: rw-------)
-			if err := os.Chmod(finalPath, 0600); err != nil {
-			fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err)
+			// Skip chmod for symlinks - os.Chmod follows symlinks and would change the target's
+			// permissions, which may be in a read-only location (e.g., /nix/store on NixOS).
+			if info, err := os.Lstat(finalPath); err == nil && info.Mode()&os.ModeSymlink == 0 {
+				if err := os.Chmod(finalPath, 0600); err != nil {
+					fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err)
+				}
 			}

 		// Verify JSONL file integrity after export
--- a/cmd/bd/import.go
+++ b/cmd/bd/import.go
@@ -498,8 +498,9 @@ func TouchDatabaseFile(dbPath, jsonlPath string) error {
 	targetTime := time.Now()

 	// If we have the JSONL path, use max(JSONL mtime, now) to handle clock skew
+	// Use Lstat to get the symlink's own mtime, not the target's (NixOS fix).
 	if jsonlPath != "" {
-		if info, err := os.Stat(jsonlPath); err == nil {
+		if info, err := os.Lstat(jsonlPath); err == nil {
 			jsonlTime := info.ModTime()
 			if jsonlTime.After(targetTime) {
 				targetTime = jsonlTime.Add(time.Nanosecond)
--- a/flake.nix
+++ b/flake.nix
@@ -31,6 +31,22 @@
            type = "app";
            program = "${self.packages.${system}.default}/bin/bd";
          };
+
+          devShells.default = pkgs.mkShell {
+            buildInputs = with pkgs; [
+              go
+              git
+              gopls
+              gotools
+              golangci-lint
+              sqlite
+            ];
+
+            shellHook = ''
+              echo "beads development shell"
+              echo "Go version: $(go version)"
+            '';
+          };
        }
      );
 }
--- a/internal/autoimport/autoimport.go
+++ b/internal/autoimport/autoimport.go
@@ -289,7 +289,11 @@ func CheckStaleness(ctx context.Context, store storage.Storage, dbPath string) (
 	dbDir := filepath.Dir(dbPath)
 	jsonlPath := utils.FindJSONLInDir(dbDir)

-	stat, err := os.Stat(jsonlPath)
+	// Use Lstat to get the symlink's own mtime, not the target's.
+	// This is critical for NixOS and other systems where JSONL may be symlinked.
+	// Using Stat would follow symlinks and return the target's mtime, which can
+	// cause false staleness detection when symlinks are recreated (e.g., by home-manager).
+	stat, err := os.Lstat(jsonlPath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			// JSONL doesn't exist - expected for new repo
--- a/internal/autoimport/symlink_test.go
+++ b/internal/autoimport/symlink_test.go
@@ -0,0 +1,133 @@
+package autoimport
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/steveyegge/beads/internal/storage/memory"
+)
+
+// TestCheckStaleness_SymlinkedJSONL verifies that mtime detection uses the symlink's
+// own mtime, not the target's mtime. This is critical for NixOS and similar systems
+// where files may be symlinked to read-only locations.
+//
+// Behavior being tested:
+// - When JSONL is a symlink, CheckStaleness should use os.Lstat (symlink mtime)
+// - NOT os.Stat (which would follow the symlink and get target's mtime)
+func TestCheckStaleness_SymlinkedJSONL(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Create the target JSONL file with old mtime
+	targetDir := filepath.Join(tmpDir, "target")
+	if err := os.MkdirAll(targetDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	targetPath := filepath.Join(targetDir, "issues.jsonl")
+	if err := os.WriteFile(targetPath, []byte(`{"id":"test-1"}`), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Set target's mtime to 1 hour ago
+	oldTime := time.Now().Add(-1 * time.Hour)
+	if err := os.Chtimes(targetPath, oldTime, oldTime); err != nil {
+		t.Fatal(err)
+	}
+
+	// Create the .beads directory structure with a symlink to the target
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	if err := os.MkdirAll(beadsDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	symlinkPath := filepath.Join(beadsDir, "issues.jsonl")
+	if err := os.Symlink(targetPath, symlinkPath); err != nil {
+		t.Fatal(err)
+	}
+
+	// The symlink itself was just created (recent mtime)
+	// The target file has old mtime (1 hour ago)
+	// If we use os.Stat (follows symlink), we'd get the target's old mtime
+	// If we use os.Lstat (symlink's own mtime), we'd get the recent mtime
+
+	// Set last_import_time to 30 minutes ago (between target mtime and symlink mtime)
+	importTime := time.Now().Add(-30 * time.Minute)
+	store := memory.New("")
+	ctx := context.Background()
+	store.SetMetadata(ctx, "last_import_time", importTime.Format(time.RFC3339))
+
+	dbPath := filepath.Join(beadsDir, "beads.db")
+
+	// With correct behavior (os.Lstat):
+	// - Symlink mtime: now (just created)
+	// - Import time: 30 min ago
+	// - Result: stale = true (symlink is newer than import)
+	//
+	// With incorrect behavior (os.Stat):
+	// - Target mtime: 1 hour ago
+	// - Import time: 30 min ago
+	// - Result: stale = false (target is older than import) - WRONG!
+	stale, err := CheckStaleness(ctx, store, dbPath)
+	if err != nil {
+		t.Fatalf("CheckStaleness failed: %v", err)
+	}
+
+	if !stale {
+		t.Error("Expected stale=true when symlinked JSONL is newer than last import")
+		t.Error("This indicates os.Stat is being used instead of os.Lstat")
+		t.Error("os.Stat follows the symlink and returns target's mtime (old)")
+		t.Error("os.Lstat returns the symlink's own mtime (recent)")
+	}
+}
+
+// TestCheckStaleness_SymlinkedJSONL_NotStale verifies the inverse case:
+// when the symlink itself is older than the last import, it should not be stale.
+func TestCheckStaleness_SymlinkedJSONL_NotStale(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Create target file
+	targetDir := filepath.Join(tmpDir, "target")
+	if err := os.MkdirAll(targetDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	targetPath := filepath.Join(targetDir, "issues.jsonl")
+	if err := os.WriteFile(targetPath, []byte(`{"id":"test-1"}`), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Create symlink
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	if err := os.MkdirAll(beadsDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	symlinkPath := filepath.Join(beadsDir, "issues.jsonl")
+	if err := os.Symlink(targetPath, symlinkPath); err != nil {
+		t.Fatal(err)
+	}
+
+	// Set symlink's mtime to 1 hour ago
+	oldTime := time.Now().Add(-1 * time.Hour)
+	// Note: os.Chtimes follows symlinks, so we use os.Lchtimes if available
+	// On most systems, symlink mtime is set at creation and can't be changed
+	// So we'll set the import time to be in the future instead
+	_ = oldTime
+
+	// Set last_import_time to just now (after symlink creation)
+	importTime := time.Now().Add(1 * time.Second)
+	store := memory.New("")
+	ctx := context.Background()
+	store.SetMetadata(ctx, "last_import_time", importTime.Format(time.RFC3339))
+
+	dbPath := filepath.Join(beadsDir, "beads.db")
+
+	stale, err := CheckStaleness(ctx, store, dbPath)
+	if err != nil {
+		t.Fatalf("CheckStaleness failed: %v", err)
+	}
+
+	if stale {
+		t.Error("Expected stale=false when last import is after symlink creation")
+	}
+}
--- a/internal/storage/sqlite/multirepo.go
+++ b/internal/storage/sqlite/multirepo.go
@@ -71,7 +71,8 @@ func (s *SQLiteStorage) hydrateFromRepo(ctx context.Context, repoPath, sourceRep
 	jsonlPath := filepath.Join(absRepoPath, ".beads", "issues.jsonl")

 	// Check if file exists
-	fileInfo, err := os.Stat(jsonlPath)
+	// Use Lstat to get the symlink's own mtime, not the target's (NixOS fix).
+	fileInfo, err := os.Lstat(jsonlPath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			// No JSONL file - skip this repo
--- a/internal/storage/sqlite/multirepo_export.go
+++ b/internal/storage/sqlite/multirepo_export.go
@@ -162,13 +162,18 @@ func (s *SQLiteStorage) exportToRepo(ctx context.Context, repoPath string, issue
 	}

 	// Set file permissions
-	if err := os.Chmod(jsonlPath, 0644); err != nil { // nolint:gosec // G302: 0644 intentional for git-tracked files
-		// Non-fatal
-		debug.Logf("Debug: failed to set permissions on %s: %v\n", jsonlPath, err)
+	// Skip chmod for symlinks - os.Chmod follows symlinks and would change the target's
+	// permissions, which may be in a read-only location (e.g., /nix/store on NixOS).
+	if info, statErr := os.Lstat(jsonlPath); statErr == nil && info.Mode()&os.ModeSymlink == 0 {
+		if err := os.Chmod(jsonlPath, 0644); err != nil { // nolint:gosec // G302: 0644 intentional for git-tracked files
+			// Non-fatal
+			debug.Logf("Debug: failed to set permissions on %s: %v\n", jsonlPath, err)
+		}
 	}

 	// Update mtime cache for this repo
-	fileInfo, err := os.Stat(jsonlPath)
+	// Use Lstat to get the symlink's own mtime, not the target's (NixOS fix).
+	fileInfo, err := os.Lstat(jsonlPath)
 	if err == nil {
 		_, err = s.db.ExecContext(ctx, `
 			INSERT OR REPLACE INTO repo_mtimes (repo_path, jsonl_path, mtime_ns, last_checked)