From e0734e230f26197b8c1f6c81e9fa12a82c07d012 Mon Sep 17 00:00:00 2001 From: Darin <86675935+darinkishore@users.noreply.github.com> Date: Fri, 5 Dec 2025 13:22:09 -0800 Subject: [PATCH] fix: NixOS symlink compatibility for mtime and permission checks (#459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Co-authored-by: Claude --- cmd/bd/doctor/fix/permissions.go | 17 +- cmd/bd/doctor/fix/symlink_test.go | 165 ++++++++++++++++++++ cmd/bd/export.go | 8 +- cmd/bd/import.go | 3 +- flake.nix | 16 ++ internal/autoimport/autoimport.go | 6 +- internal/autoimport/symlink_test.go | 133 ++++++++++++++++ internal/storage/sqlite/multirepo.go | 3 +- internal/storage/sqlite/multirepo_export.go | 13 +- 9 files changed, 353 insertions(+), 11 deletions(-) create mode 100644 cmd/bd/doctor/fix/symlink_test.go create mode 100644 internal/autoimport/symlink_test.go diff --git a/cmd/bd/doctor/fix/permissions.go b/cmd/bd/doctor/fix/permissions.go index a2394140..770d11d7 100644 --- 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() diff --git a/cmd/bd/doctor/fix/symlink_test.go b/cmd/bd/doctor/fix/symlink_test.go new file mode 100644 index 00000000..99f0fe38 --- /dev/null +++ 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()) + } +} diff --git a/cmd/bd/export.go b/cmd/bd/export.go index a2451919..df8cde5f 100644 --- 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 diff --git a/cmd/bd/import.go b/cmd/bd/import.go index 3a3154cb..af1a86d1 100644 --- 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) diff --git a/flake.nix b/flake.nix index 9cd3c3cb..f923ca70 100644 --- 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)" + ''; + }; } ); } diff --git a/internal/autoimport/autoimport.go b/internal/autoimport/autoimport.go index 1f472b2c..f4d94ee2 100644 --- 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 diff --git a/internal/autoimport/symlink_test.go b/internal/autoimport/symlink_test.go new file mode 100644 index 00000000..5e1a387d --- /dev/null +++ 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") + } +} diff --git a/internal/storage/sqlite/multirepo.go b/internal/storage/sqlite/multirepo.go index 21d16869..be2cf978 100644 --- 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 diff --git a/internal/storage/sqlite/multirepo_export.go b/internal/storage/sqlite/multirepo_export.go index 96bc135f..cadd53ad 100644 --- 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)