fix(doctor): address PR feedback

- Check errors in test setup helpers instead of silently discarding them
  (health_test.go, git_test.go)
- Skip provenance-comment assertions on git < 2.45 instead of failing
  (config_test.go, e2e/doctor_test.go)
- Discriminate "unknown option" from real errors in setForkPointWithComment
  so permissions/locking failures propagate instead of triggering the
  old-git warning (cmd/doctor.go, internal/config/config.go)
- Delete unused GetConfigPath (dead code left over from the
  commentForkPointChange removal)

Note: PR.md also updated to clarify abbreviated SHA in provenance
comments (not included in this commit; will be added manually).

Author: cds-amal

cmd/doctor.go

@@ -4,6 +4,7 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/boneskull/gh-stack/internal/config"
@@ -248,14 +249,24 @@ func setForkPointWithComment(s *style.Style, cfg *config.Config, branch, oldSHA,
 		return nil
 	}
 
-	// --comment likely unsupported; fall back to plain set and warn once.
+	// Only fall back for unsupported --comment flag; propagate real errors.
+	if !isUnsupportedCommentErr(err) {
+		return err
+	}
 	if setErr := cfg.SetForkPoint(branch, newSHA); setErr != nil {
 		return setErr
 	}
 	warnOldGit(s)
 	return nil
 }
 
+// isUnsupportedCommentErr returns true if the error indicates that
+// git config --comment is not recognized (git < 2.45).
+func isUnsupportedCommentErr(err error) bool {
+	msg := err.Error()
+	return strings.Contains(msg, "unknown option")
+}
+
 var oldGitWarned bool
 
 // warnOldGit prints a one-time warning that the user's git version is too old

e2e/doctor_test.go

@@ -139,17 +139,19 @@ func TestDoctorFixUpdatesConfig(t *testing.T) {
 	// Verify inline provenance comment was written to the config value line.
 	// git config --comment (Git 2.45+) appends "# replaces <abbrev> (<timestamp>)"
 	// on the same line as the stackForkPoint value.
+	// On older git the comment won't be present; that's fine (doctor falls back gracefully).
 	configData, err := os.ReadFile(filepath.Join(env.WorkDir, ".git", "config"))
 	if err != nil {
 		t.Fatalf("failed to read .git/config: %v", err)
 	}
 	configStr := string(configData)
-	if !strings.Contains(configStr, "# replaces") {
-		t.Error("expected inline provenance comment '# replaces ...' in .git/config")
-	}
-	abbrevOld := originalFP[:7]
-	if !strings.Contains(configStr, abbrevOld) {
-		t.Errorf("expected abbreviated old fork point %s in config comment", abbrevOld)
+	if strings.Contains(configStr, "# replaces") {
+		abbrevOld := originalFP[:7]
+		if !strings.Contains(configStr, abbrevOld) {
+			t.Errorf("expected abbreviated old fork point %s in config comment", abbrevOld)
+		}
+	} else {
+		t.Log("git config --comment not supported; skipping provenance comment assertion")
 	}
 }
 

internal/config/config.go

@@ -118,9 +118,20 @@ func (c *Config) SetForkPoint(branch, sha string) error {
 
 // SetForkPointWithComment stores the fork point SHA with an inline comment.
 // Requires Git 2.45+. The comment appears after the value on the same line.
+// Returns an error with stderr content on failure, so callers can distinguish
+// "unknown option" (old git) from other failures.
 func (c *Config) SetForkPointWithComment(branch, sha, comment string) error {
 	key := "branch." + branch + ".stackForkPoint"
-	return exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha).Run()
+	cmd := exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha)
+	var stderr bytes.Buffer
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		if stderr.Len() > 0 {
+			return errors.New(strings.TrimSpace(stderr.String()))
+		}
+		return err
+	}
+	return nil
 }
 
 // RemoveForkPoint removes the stored fork point for a branch.

internal/config/config_test.go

@@ -203,7 +203,8 @@ func TestSetForkPointWithComment(t *testing.T) {
 	comment := "replaces deadbee (2026-02-14T00:00:00Z)"
 
 	if err := cfg.SetForkPointWithComment("feature", sha, comment); err != nil {
-		t.Fatalf("SetForkPointWithComment failed: %v", err)
+		// git config --comment requires Git 2.45+; skip on older versions.
+		t.Skipf("SetForkPointWithComment not supported (git too old?): %v", err)
 	}
 
 	// Value should be readable via GetForkPoint (git config ignores inline comments)

internal/git/git.go

@@ -366,13 +366,6 @@ func (g *Git) GetGitDir() string {
 	return filepath.Join(g.repoPath, ".git")
 }
 
-// GetConfigPath returns the path to the repo's git config file,
-// resolved via `git rev-parse --git-path config` so it works
-// correctly in linked worktrees and submodules.
-func (g *Git) GetConfigPath() (string, error) {
-	return g.run("rev-parse", "--git-path", "config")
-}
-
 // Fetch fetches from origin.
 func (g *Git) Fetch() error {
 	return g.runInteractive("fetch", "origin")

internal/git/git_test.go

@@ -730,15 +730,33 @@ func TestIsAncestor(t *testing.T) {
 	initial, _ := g.GetTip("HEAD")
 
 	// Create a linear chain: initial -> A -> B
-	os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0644)
-	exec.Command("git", "-C", dir, "add", ".").Run()
-	exec.Command("git", "-C", dir, "commit", "-m", "commit A").Run()
-	shaA, _ := g.GetTip("HEAD")
+	if err := os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0644); err != nil {
+		t.Fatalf("WriteFile failed: %v", err)
+	}
+	if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil {
+		t.Fatalf("git add failed: %v\n%s", err, out)
+	}
+	if out, err := exec.Command("git", "-C", dir, "commit", "-m", "commit A").CombinedOutput(); err != nil {
+		t.Fatalf("git commit failed: %v\n%s", err, out)
+	}
+	shaA, err := g.GetTip("HEAD")
+	if err != nil {
+		t.Fatalf("GetTip failed: %v", err)
+	}
 
-	os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b"), 0644)
-	exec.Command("git", "-C", dir, "add", ".").Run()
-	exec.Command("git", "-C", dir, "commit", "-m", "commit B").Run()
-	shaB, _ := g.GetTip("HEAD")
+	if err := os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b"), 0644); err != nil {
+		t.Fatalf("WriteFile failed: %v", err)
+	}
+	if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil {
+		t.Fatalf("git add failed: %v\n%s", err, out)
+	}
+	if out, err := exec.Command("git", "-C", dir, "commit", "-m", "commit B").CombinedOutput(); err != nil {
+		t.Fatalf("git commit failed: %v\n%s", err, out)
+	}
+	shaB, err := g.GetTip("HEAD")
+	if err != nil {
+		t.Fatalf("GetTip failed: %v", err)
+	}
 
 	// initial is ancestor of B
 	isAnc, err := g.IsAncestor(initial, shaB)
@@ -801,9 +819,15 @@ func TestGetForkPoint(t *testing.T) {
 	g.CreateAndCheckout("child")
 
 	// Add commits on child
-	os.WriteFile(filepath.Join(dir, "child.txt"), []byte("child"), 0644)
-	exec.Command("git", "-C", dir, "add", ".").Run()
-	exec.Command("git", "-C", dir, "commit", "-m", "child commit").Run()
+	if err := os.WriteFile(filepath.Join(dir, "child.txt"), []byte("child"), 0644); err != nil {
+		t.Fatalf("WriteFile failed: %v", err)
+	}
+	if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil {
+		t.Fatalf("git add failed: %v\n%s", err, out)
+	}
+	if out, err := exec.Command("git", "-C", dir, "commit", "-m", "child commit").CombinedOutput(); err != nil {
+		t.Fatalf("git commit failed: %v\n%s", err, out)
+	}
 
 	// GetForkPoint should return the divergence point
 	fp, err := g.GetForkPoint(trunk, "child")

internal/health/health_test.go

@@ -128,9 +128,13 @@ func TestCheckBranch_MissingParent(t *testing.T) {
 
 	// Now create "middle" as parent, point child at it, then delete it
 	run := func(args ...string) {
+		t.Helper()
 		cmd := exec.Command("git", args...)
 		cmd.Dir = dir
-		cmd.Run()
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			t.Fatalf("git %v failed: %v\n%s", args, err, out)
+		}
 	}
 	run("checkout", trunk)
 	run("checkout", "-b", "middle")