Skip to content

Commit 48d5685

Browse files
cds-amalcds-amal
committed
refactor(doctor): extract reusable branch health validation into internal/health
Move the pure validation logic out of doctor's checkBranch into a new health.CheckBranch API that returns structured []Issue values (with Kind, Message, Fixable fields) instead of styled strings. This makes the health checks composable so future commands (cascade, sync) can run them as pre-flight validation before rebasing without pulling in style or fix logic. Doctor becomes a thin wrapper: call health.CheckBranch for diagnosis, then layer styling and repair logic on top based on issue kind.
1 parent 0aefd85 commit 48d5685

3 files changed

Lines changed: 478 additions & 96 deletions

File tree

‎cmd/doctor.go‎

Lines changed: 70 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/boneskull/gh-stack/internal/config"
1111
"github.com/boneskull/gh-stack/internal/git"
12+
"github.com/boneskull/gh-stack/internal/health"
1213
"github.com/boneskull/gh-stack/internal/style"
1314
"github.com/spf13/cobra"
1415
)
@@ -106,125 +107,98 @@ func runDoctor(cmd *cobra.Command, args []string) error {
106107
func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, fix bool) branchResult {
107108
result := branchResult{name: branch}
108109

109-
// Check 1: branch exists locally
110-
if !g.BranchExists(branch) {
111-
result.issues = append(result.issues, s.Error("Branch does not exist locally"))
110+
issues := health.CheckBranch(g, cfg, branch)
111+
if len(issues) == 0 {
112+
result.healthy = true
112113
return result
113114
}
114115

115-
// Check 2: parent exists locally
116-
parent, err := cfg.GetParent(branch)
117-
if err != nil {
118-
result.issues = append(result.issues, s.Error("No parent configured"))
116+
// If not fixing, format issues with styling and return.
117+
if !fix {
118+
for _, iss := range issues {
119+
switch iss.Kind {
120+
case health.KindBranchMissing, health.KindParentMissing, health.KindNoForkPoint:
121+
result.issues = append(result.issues, s.Error(iss.Message))
122+
default:
123+
result.issues = append(result.issues, iss.Message)
124+
}
125+
}
119126
return result
120127
}
121128

122-
trunk, _ := cfg.GetTrunk() //nolint:errcheck // already validated in runDoctor
123-
if parent == trunk {
124-
if !g.BranchExists(trunk) {
125-
result.issues = append(result.issues, s.Errorf("Trunk branch %s does not exist locally", trunk))
126-
return result
129+
// Attempt to fix: dispatch based on the issue kind.
130+
kind := issues[0].Kind
131+
if !issues[0].Fixable && kind != health.KindDrift {
132+
// Unfixable issues (missing branch, missing parent, check failures)
133+
for _, iss := range issues {
134+
result.issues = append(result.issues, s.Error(iss.Message))
127135
}
128-
} else if !g.BranchExists(parent) {
129-
result.issues = append(result.issues, s.Errorf("Parent branch %s does not exist locally", parent))
130136
return result
131137
}
132138

133-
// Check 3: fork point is stored
134-
storedFP, err := cfg.GetForkPoint(branch)
135-
if err != nil {
136-
if fix {
137-
newFP, fixErr := computeForkPoint(g, parent, branch)
138-
if fixErr != nil {
139-
result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr))
140-
return result
141-
}
142-
if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil {
143-
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
144-
return result
145-
}
146-
result.fixed = true
147-
result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP))
139+
parent, _ := cfg.GetParent(branch) //nolint:errcheck // health check validated parent exists
140+
storedFP, _ := cfg.GetForkPoint(branch) //nolint:errcheck // may be empty for KindNoForkPoint
141+
142+
switch kind {
143+
case health.KindNoForkPoint:
144+
newFP, fixErr := computeForkPoint(g, parent, branch)
145+
if fixErr != nil {
146+
result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr))
148147
return result
149148
}
150-
result.issues = append(result.issues, s.Error("No fork point stored"))
151-
return result
152-
}
153-
154-
// Check 4: fork point commit exists
155-
if !g.CommitExists(storedFP) {
156-
if fix {
157-
newFP, fixErr := computeForkPoint(g, parent, branch)
158-
if fixErr != nil {
159-
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr))
160-
return result
161-
}
162-
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil {
163-
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
164-
return result
165-
}
166-
result.fixed = true
167-
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP))
149+
if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil {
150+
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
168151
return result
169152
}
170-
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist", git.AbbrevSHA(storedFP)))
171-
return result
172-
}
153+
result.fixed = true
154+
result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP))
173155

174-
// Check 5: fork point is ancestor of branch
175-
isAnc, err := g.IsAncestor(storedFP, branch)
176-
if err != nil {
177-
result.issues = append(result.issues, fmt.Sprintf("Failed to check if stored fork point %s is an ancestor of %s: %v", git.AbbrevSHA(storedFP), branch, err))
178-
return result
179-
}
180-
if !isAnc {
181-
if fix {
182-
newFP, fixErr := computeForkPoint(g, parent, branch)
183-
if fixErr != nil {
184-
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr))
185-
return result
186-
}
187-
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil {
188-
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
189-
return result
190-
}
191-
result.fixed = true
192-
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP))
156+
case health.KindForkPointMissing:
157+
newFP, fixErr := computeForkPoint(g, parent, branch)
158+
if fixErr != nil {
159+
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr))
193160
return result
194161
}
195-
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s", git.AbbrevSHA(storedFP), branch))
196-
return result
197-
}
162+
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil {
163+
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
164+
return result
165+
}
166+
result.fixed = true
167+
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP))
198168

199-
// Check 6: drift detection
200-
mergeBase, err := g.GetMergeBase(parent, branch)
201-
if err != nil {
202-
result.issues = append(result.issues,
203-
fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err),
204-
)
205-
return result
206-
}
169+
case health.KindForkPointNotAncestor:
170+
newFP, fixErr := computeForkPoint(g, parent, branch)
171+
if fixErr != nil {
172+
result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr))
173+
return result
174+
}
175+
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil {
176+
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
177+
return result
178+
}
179+
result.fixed = true
180+
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP))
207181

208-
if storedFP != mergeBase {
209-
if fix {
210-
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil {
211-
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
212-
return result
213-
}
214-
result.fixed = true
215-
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase))
182+
case health.KindDrift:
183+
mergeBase, err := g.GetMergeBase(parent, branch)
184+
if err != nil {
185+
result.issues = append(result.issues, fmt.Sprintf("Failed to compute merge-base for fix: %v", err))
216186
return result
217187
}
218-
result.issues = append(result.issues,
219-
fmt.Sprintf("Stored parent: %s", parent),
220-
fmt.Sprintf("Stored fork: %s", git.AbbrevSHA(storedFP)),
221-
fmt.Sprintf("Actual fork: %s", git.AbbrevSHA(mergeBase)),
222-
"Drift detected: stored fork point does not match merge-base",
223-
)
224-
return result
188+
if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil {
189+
result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr))
190+
return result
191+
}
192+
result.fixed = true
193+
result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase))
194+
195+
default:
196+
// Shouldn't happen, but surface issues if it does
197+
for _, iss := range issues {
198+
result.issues = append(result.issues, iss.Message)
199+
}
225200
}
226201

227-
result.healthy = true
228202
return result
229203
}
230204

‎internal/health/health.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// internal/health/health.go
2+
package health
3+
4+
import (
5+
"fmt"
6+
7+
"github.com/boneskull/gh-stack/internal/config"
8+
"github.com/boneskull/gh-stack/internal/git"
9+
)
10+
11+
// IssueKind identifies which health check produced an issue.
12+
type IssueKind int
13+
14+
const (
15+
// KindBranchMissing means the branch does not exist locally.
16+
KindBranchMissing IssueKind = iota + 1
17+
// KindParentMissing means the parent branch does not exist locally.
18+
KindParentMissing
19+
// KindNoForkPoint means no fork point is stored in config.
20+
KindNoForkPoint
21+
// KindForkPointMissing means the stored fork point commit doesn't exist.
22+
KindForkPointMissing
23+
// KindForkPointNotAncestor means the stored fork point is not an ancestor of the branch.
24+
KindForkPointNotAncestor
25+
// KindDrift means the stored fork point doesn't match the computed merge-base.
26+
KindDrift
27+
// KindCheckFailed means a check itself errored (e.g., merge-base computation failed).
28+
KindCheckFailed
29+
)
30+
31+
// Issue describes a branch health problem.
32+
type Issue struct {
33+
Kind IssueKind // which check produced this issue
34+
Message string // plain text, no ANSI styling
35+
Fixable bool // can `doctor --fix` repair this?
36+
}
37+
38+
// CheckBranch runs lightweight health checks on a tracked branch.
39+
// Returns nil if healthy. Checks run in order and bail on first blocker:
40+
// 1. Branch exists locally
41+
// 2. Parent exists locally (including trunk)
42+
// 3. Fork point is stored in config
43+
// 4. Fork point commit exists in repo
44+
// 5. Fork point is ancestor of branch
45+
// 6. Stored fork point matches merge-base (drift detection)
46+
func CheckBranch(g *git.Git, cfg *config.Config, branch string) []Issue {
47+
// Check 1: branch exists locally
48+
if !g.BranchExists(branch) {
49+
return []Issue{{Kind: KindBranchMissing, Message: "Branch does not exist locally", Fixable: false}}
50+
}
51+
52+
// Check 2: parent exists locally
53+
parent, err := cfg.GetParent(branch)
54+
if err != nil {
55+
return []Issue{{Kind: KindParentMissing, Message: "No parent configured", Fixable: false}}
56+
}
57+
58+
trunk, _ := cfg.GetTrunk() //nolint:errcheck // caller should validate trunk exists
59+
if parent == trunk {
60+
if !g.BranchExists(trunk) {
61+
return []Issue{{Kind: KindParentMissing, Message: fmt.Sprintf("Trunk branch %s does not exist locally", trunk), Fixable: false}}
62+
}
63+
} else if !g.BranchExists(parent) {
64+
return []Issue{{Kind: KindParentMissing, Message: fmt.Sprintf("Parent branch %s does not exist locally", parent), Fixable: false}}
65+
}
66+
67+
// Check 3: fork point is stored
68+
storedFP, err := cfg.GetForkPoint(branch)
69+
if err != nil {
70+
return []Issue{{Kind: KindNoForkPoint, Message: "No fork point stored", Fixable: true}}
71+
}
72+
73+
// Check 4: fork point commit exists
74+
if !g.CommitExists(storedFP) {
75+
return []Issue{{Kind: KindForkPointMissing, Message: fmt.Sprintf("Stored fork point %s does not exist", git.AbbrevSHA(storedFP)), Fixable: true}}
76+
}
77+
78+
// Check 5: fork point is ancestor of branch
79+
isAnc, err := g.IsAncestor(storedFP, branch)
80+
if err != nil {
81+
return []Issue{{Kind: KindCheckFailed, Message: fmt.Sprintf("Failed to check if stored fork point %s is an ancestor of %s: %v", git.AbbrevSHA(storedFP), branch, err), Fixable: false}}
82+
}
83+
if !isAnc {
84+
return []Issue{{Kind: KindForkPointNotAncestor, Message: fmt.Sprintf("Stored fork point %s is not an ancestor of %s", git.AbbrevSHA(storedFP), branch), Fixable: true}}
85+
}
86+
87+
// Check 6: drift detection
88+
mergeBase, err := g.GetMergeBase(parent, branch)
89+
if err != nil {
90+
return []Issue{{Kind: KindCheckFailed, Message: fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err), Fixable: false}}
91+
}
92+
93+
if storedFP != mergeBase {
94+
return []Issue{
95+
{Kind: KindDrift, Message: fmt.Sprintf("Stored parent: %s", parent), Fixable: false},
96+
{Kind: KindDrift, Message: fmt.Sprintf("Stored fork: %s", git.AbbrevSHA(storedFP)), Fixable: false},
97+
{Kind: KindDrift, Message: fmt.Sprintf("Actual fork: %s", git.AbbrevSHA(mergeBase)), Fixable: false},
98+
{Kind: KindDrift, Message: "Drift detected: stored fork point does not match merge-base", Fixable: true},
99+
}
100+
}
101+
102+
return nil
103+
}

0 commit comments

Comments
 (0)