internal/workspaces/doctor_fix.go (2)

355-369: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Merge the two os.Lstat calls to avoid redundant syscalls and TOCTOU window.

os.Lstat(symlinkPath) is called twice (lines 355 and 366) with os.Stat(expectedTarget) interleaved between them. The result from the first call is fully sufficient to determine both the idempotency and the non-symlink guard, but Go's if-initialiser scoping forces the redeclaration. Hoisting Lstat before both blocks eliminates the extra syscall and the TOCTOU window between the two checks.

♻️ Proposed refactor

 func fixSymlinkMissing() error {
 	_, symlinkPath, expectedTarget, err := symlinkPaths()
 	if err != nil {
 		return err
 	}
-	// If it already exists and points at the right place, nothing to do.
-	if info, lerr := os.Lstat(symlinkPath); lerr == nil && info.Mode()&os.ModeSymlink != 0 {
-		if cur, rerr := os.Readlink(symlinkPath); rerr == nil &&
-			filepath.Clean(cur) == filepath.Clean(expectedTarget) {
-			return nil
-		}
+	info, lerr := os.Lstat(symlinkPath)
+	if lerr == nil {
+		if info.Mode()&os.ModeSymlink == 0 {
+			// Regular file or directory — partial-migration case, refuse to clobber.
+			return fmt.Errorf("%w: ~/.wuphf is a regular path, not a symlink — see docs/multi-workspace.md",
+				ErrManualFixRequired)
+		}
+		// It's a symlink; check whether it already points at the right place.
+		if cur, rerr := os.Readlink(symlinkPath); rerr == nil &&
+			filepath.Clean(cur) == filepath.Clean(expectedTarget) {
+			return nil
+		}
 	}
 	if _, err := os.Stat(expectedTarget); err != nil {
 		return fmt.Errorf("symlink:missing: target %s does not exist: %w", expectedTarget, err)
 	}
-	// If ~/.wuphf already exists as a regular file or directory, that's the
-	// partial-migration case — refuse to clobber user data (mirrors fixSymlinkWrong).
-	if info, lerr := os.Lstat(symlinkPath); lerr == nil && info.Mode()&os.ModeSymlink == 0 {
-		return fmt.Errorf("%w: ~/.wuphf is a regular path, not a symlink — see docs/multi-workspace.md",
-			ErrManualFixRequired)
-	}
 	if err := os.Symlink(expectedTarget, symlinkPath); err != nil {
 		return fmt.Errorf("symlink:missing: create %s → %s: %w", symlinkPath, expectedTarget, err)
 	}
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workspaces/doctor_fix.go` around lines 355 - 369, Hoist the initial
os.Lstat(symlinkPath) call and reuse its result instead of calling Lstat twice:
call os.Lstat once into variables (e.g. info, lerr) before the idempotency and
non-symlink checks, then use info.Mode()&os.ModeSymlink,
os.Readlink(symlinkPath) and filepath.Clean comparisons with expectedTarget to
detect an already-correct symlink, and only call os.Stat(expectedTarget) when
Lstat indicates no valid symlink; keep the existing error returns and the
ErrManualFixRequired path but remove the second os.Lstat to eliminate the
redundant syscall and TOCTOU window.

---

399-405: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

fixSymlinkWrong still swallows os.ErrExist, leaving the symlink uncreated while returning nil.

This PR removes the !errors.Is(err, os.ErrExist) guard from fixSymlinkMissing, but line 402 in fixSymlinkWrong retains it:

if err := os.Symlink(expectedTarget, symlinkPath); err != nil && !errors.Is(err, os.ErrExist) {

After os.Remove succeeds at line 390 (or when lerr != nil at line 384 — path absent), a concurrent actor can create a path at symlinkPath before os.Symlink runs. If os.Symlink then returns EEXIST, the guard swallows the error and the function returns nil — signalling success even though the intended symlink was never created. The PR's stated rationale applies equally here.

🐛 Proposed fix

-	if err := os.Symlink(expectedTarget, symlinkPath); err != nil && !errors.Is(err, os.ErrExist) {
-		return fmt.Errorf("symlink:wrong: recreate %s → %s: %w", symlinkPath, expectedTarget, err)
-	}
+	if err := os.Symlink(expectedTarget, symlinkPath); err != nil {
+		return fmt.Errorf("symlink:wrong: recreate %s → %s: %w", symlinkPath, expectedTarget, err)
+	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workspaces/doctor_fix.go` around lines 399 - 405, In
fixSymlinkWrong, stop swallowing os.ErrExist: remove the "&& !errors.Is(err,
os.ErrExist)" guard in the os.Symlink call (the check in the block that
currently reads if err := os.Symlink(...); err != nil && !errors.Is(err,
os.ErrExist) { ... }) so that any error from os.Symlink (including EEXIST) is
returned to the caller; keep the prior existence check of expectedTarget and
otherwise return the wrapped error from os.Symlink so callers see failures
instead of receiving nil when the symlink was not actually created.

internal/workspaces/doctor_fix.go (1)

354-372: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

fixSymlinkMissing has two unhandled paths that produce confusing errors after the EEXIST swallow was removed.

Issue A — wrong-symlink fallthrough (Case C):
When ~/.wuphf already exists as a symlink but points to the wrong target (or os.Readlink fails for any reason), none of the early-return branches inside if lerr == nil { … } fire, so execution falls through to os.Symlink. Because the path exists, os.Symlink always returns EEXIST, and the caller receives "symlink:missing: create ~/.wuphf → …: symlink: file exists" — an error message that gives no actionable guidance. The previous EEXIST swallow silently returned nil here (which is the bug the PR fixes), but exposing it now reveals the underlying fallthrough rather than fixing it.

A concrete trigger: Doctor reports symlink:missing for a symlink whose target has been deleted. os.Lstat finds the dangling symlink, os.Readlink returns the stale (wrong) path, the idempotency check fails, and execution falls straight into os.Symlink → EEXIST.

Issue B — non-ErrNotExist Lstat error swallowed:
If os.Lstat fails for a reason other than the path not existing (e.g., a permission error), lerr is non-nil but the if lerr == nil guard simply skips the entire block. Execution proceeds to os.Stat(expectedTarget) and then os.Symlink, burying the original error under a secondary, unrelated failure. The rest of the file handles this correctly (fixOrphanTreeRegister lines 126–132, fixOrphanTreeDelete lines 220–225).

🐛 Proposed fix

 	info, lerr := os.Lstat(symlinkPath)
 	if lerr == nil {
 		if info.Mode()&os.ModeSymlink == 0 {
 			// Regular file or directory — partial-migration case, refuse to clobber.
 			return fmt.Errorf("%w: ~/.wuphf is a regular path, not a symlink — see docs/multi-workspace.md",
 				ErrManualFixRequired)
 		}
 		// It's a symlink; check whether it already points at the right place.
 		if cur, rerr := os.Readlink(symlinkPath); rerr == nil &&
 			filepath.Clean(cur) == filepath.Clean(expectedTarget) {
 			return nil
 		}
+		// Symlink exists but points elsewhere (or is unreadable) — that is the
+		// symlink:wrong case. Return a clear error so the caller/user knows to
+		// dispatch the right fix instead of seeing a confusing EEXIST below.
+		return fmt.Errorf("%w: ~/.wuphf symlink exists but does not point to the expected target — re-run doctor or use the symlink:wrong fix",
+			ErrManualFixRequired)
+	} else if !errors.Is(lerr, os.ErrNotExist) {
+		return fmt.Errorf("symlink:missing: stat %s: %w", symlinkPath, lerr)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workspaces/doctor_fix.go` around lines 354 - 372, fixSymlinkMissing
currently falls through to os.Symlink when a symlink exists but points to the
wrong target (or when os.Readlink fails) and also swallows non-ErrNotExist lstat
errors; change it so that if os.Lstat returns a non-nil error that is not
os.IsNotExist(err) you return/wrap that error immediately, and if lstat succeeds
and info.Mode()&os.ModeSymlink != 0 then handle two cases explicitly: if
os.Readlink(symlinkPath) returns an error, return/wrap that error (don’t
continue), and if Readlink returns a path different from expectedTarget, first
remove the existing symlink (os.Remove(symlinkPath)) and then create the new
symlink to expectedTarget (or return a clear, actionable error if removal
fails); update error messages from the os.Symlink failure to be more specific
(mention existing wrong symlink or inability to remove it) so callers get
actionable guidance.