Skip to content

Commit 4ad5b53

Browse files
harry-miller-trimbleharry-miller-trimble
authored
fix(doctor): stop emitting fresh-clone false positive in Dolt server mode (#3755)
When dolt_mode=server is configured but the Dolt server is unreachable (TCP refused, TLS mismatch, auth failure), CheckFreshClone previously fell through to the legacy "Fresh clone detected (N issues in issues.jsonl, no database)" warning and recommended bd bootstrap. In server mode the absence of a local Dolt directory is expected, so the warning is a false positive and the recommended fix is wrong: bd bootstrap won't help when the actual problem is connectivity or credentials. The "fall through (FR-030)" comment misread FR-030, which only requires that an unreachable server not panic the caller — not that we silently degrade to a misleading message. This change adds freshCloneServerUnreachableResult, which: - identifies that we're in server mode and points at host:port, - surfaces the underlying connection error in Detail for diagnostics, - and recommends verifying server health, host/port, TLS, and credentials rather than bd bootstrap. CheckFreshClone now routes the unreachable branch through this helper instead of falling through. The existing TestCheckFreshClone_ServerModeUnreachable is updated to assert the new behavior, and a new TestFreshCloneServerUnreachableResult adds direct unit coverage for the helper, including a nil-error path.
1 parent 25703f8 commit 4ad5b53

3 files changed

Lines changed: 135 additions & 14 deletions

File tree

‎cmd/bd/doctor/fresh_clone_server.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,40 @@ func freshCloneServerResult(dbExists bool, dbName, host string, port int, syncRe
105105
Fix: fix,
106106
}
107107
}
108+
109+
// freshCloneServerUnreachableResult builds the DoctorCheck for the case where
110+
// dolt_mode=server is configured but the server cannot be reached (TCP refused,
111+
// TLS mismatch, auth failure, etc.). Falling through to the legacy "Fresh clone
112+
// detected (no database)" warning is misleading in server mode because the
113+
// absence of a local database is expected — see GH#35.
114+
//
115+
// The message identifies that we're in server mode, points at the configured
116+
// host:port, surfaces the underlying connection error for diagnostics, and
117+
// suggests connectivity/credential checks rather than bd bootstrap (which
118+
// won't help when the server itself is unreachable).
119+
func freshCloneServerUnreachableResult(dbName, host string, port int, connErr error) DoctorCheck {
120+
var msg strings.Builder
121+
fmt.Fprintf(&msg, "Dolt server unreachable at %s:%d (database %q, server mode configured).", host, port, dbName)
122+
msg.WriteString(" The local database directory is not expected in server mode, so this is not a fresh clone — it's a connectivity or auth problem.")
123+
124+
var detail strings.Builder
125+
detail.WriteString("dolt_mode=server is configured but the doctor check could not reach the server.\n")
126+
detail.WriteString(" In server mode, beads stores data on the Dolt server, so no local .beads/dolt directory is expected.\n")
127+
if connErr != nil {
128+
fmt.Fprintf(&detail, " Underlying error: %v\n", connErr)
129+
}
130+
detail.WriteString(" Common causes: server not running, wrong host/port, TLS misconfiguration, or invalid credentials.")
131+
132+
return DoctorCheck{
133+
Name: "Fresh Clone",
134+
Status: StatusWarning,
135+
Message: msg.String(),
136+
Detail: detail.String(),
137+
Fix: "Verify Dolt server connectivity:\n" +
138+
" 1. Confirm the server is running and reachable from this host\n" +
139+
" 2. Check .beads/metadata.json: dolt_server_host, dolt_server_port, dolt_server_tls\n" +
140+
" 3. Verify credentials in ~/.config/beads/credentials (or BEADS_DOLT_PASSWORD)\n" +
141+
" 4. Try a CRUD command (e.g. 'bd ready') to confirm the server is usable\n" +
142+
" 5. Re-run 'bd doctor' once connectivity is restored",
143+
}
144+
}

‎cmd/bd/doctor/fresh_clone_server_test.go‎

Lines changed: 91 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -113,12 +113,15 @@ func TestCheckFreshCloneDB_ServerUnreachable(t *testing.T) {
113113
}
114114

115115
func TestCheckFreshClone_ServerModeUnreachable(t *testing.T) {
116-
// bd-tzo9: When metadata.json declares dolt_mode=server but the server
117-
// is unreachable, CheckFreshClone must resolve credentials by the
118-
// *resolved runtime port* (not the deprecated metadata port default),
119-
// invoke checkFreshCloneDB, observe Reachable=false, and fall through
120-
// to the existing JSONL-based warning. This exercises the
121-
// GetDoltServerPasswordForPort(port) code path.
116+
// GH#35 + bd-tzo9: When metadata.json declares dolt_mode=server but the
117+
// server is unreachable, CheckFreshClone must:
118+
// 1. Resolve credentials by the resolved runtime port (bd-tzo9), not
119+
// the deprecated metadata port default — exercised by going through
120+
// the GetDoltServerPasswordForPort(port) call before the failed ping.
121+
// 2. Surface a server-mode-aware warning instead of the misleading
122+
// legacy "Fresh clone detected (no database)" message (GH#35). In
123+
// server mode the local DB absence is expected; suggesting bd
124+
// bootstrap is wrong when the actual problem is connectivity/auth.
122125
tmpDir := t.TempDir()
123126
beadsDir := filepath.Join(tmpDir, ".beads")
124127
if err := os.MkdirAll(beadsDir, 0o755); err != nil {
@@ -132,9 +135,7 @@ func TestCheckFreshClone_ServerModeUnreachable(t *testing.T) {
132135
}
133136

134137
// metadata.json declaring server mode pointed at an unreachable host:port.
135-
// Port 1 is guaranteed-unreachable on loopback; Reachable=false exercises
136-
// the FR-030 fall-through branch that contains the fixed password-resolution
137-
// line.
138+
// Port 1 is guaranteed-unreachable on loopback.
138139
meta := `{
139140
"database": "beads.db",
140141
"dolt_mode": "server",
@@ -149,16 +150,93 @@ func TestCheckFreshClone_ServerModeUnreachable(t *testing.T) {
149150

150151
check := CheckFreshClone(tmpDir)
151152

152-
// Server unreachable → falls through to legacy JSONL-based warning.
153153
if check.Status != StatusWarning {
154-
t.Fatalf("expected %q on unreachable server fall-through, got %q (message: %s)",
154+
t.Fatalf("expected %q on unreachable server, got %q (message: %s)",
155155
StatusWarning, check.Status, check.Message)
156156
}
157-
if !strings.Contains(check.Fix, "bd bootstrap") {
158-
t.Errorf("expected fall-through fix to mention 'bd bootstrap', got: %s", check.Fix)
157+
158+
// GH#35: must NOT emit the misleading legacy "Fresh clone detected
159+
// (N issues in issues.jsonl, no database)" message in server mode.
160+
if strings.Contains(check.Message, "issues in issues.jsonl, no database") {
161+
t.Errorf("server-mode unreachable should not emit legacy fresh-clone message; got: %s", check.Message)
162+
}
163+
164+
// Must call out server-mode connectivity, not "fresh clone".
165+
wantSubstrings := []string{"server", "127.0.0.1:1"}
166+
for _, want := range wantSubstrings {
167+
if !strings.Contains(strings.ToLower(check.Message), strings.ToLower(want)) {
168+
t.Errorf("expected message to mention %q, got: %s", want, check.Message)
169+
}
170+
}
171+
172+
// Fix should suggest connectivity/credential checks, not bd bootstrap
173+
// (which won't help when the server itself is unreachable).
174+
if strings.Contains(check.Fix, "bd bootstrap") {
175+
t.Errorf("server-mode unreachable fix should not suggest 'bd bootstrap'; got: %s", check.Fix)
176+
}
177+
for _, want := range []string{"server", "credentials"} {
178+
if !strings.Contains(strings.ToLower(check.Fix), want) {
179+
t.Errorf("expected fix to mention %q, got: %s", want, check.Fix)
180+
}
181+
}
182+
}
183+
184+
func TestFreshCloneServerUnreachableResult(t *testing.T) {
185+
// Pure-function coverage for the server-unreachable branch (GH#35).
186+
// Verifies the message identifies server mode, includes host:port and
187+
// the underlying error, and avoids the misleading bd bootstrap fix.
188+
check := freshCloneServerUnreachableResult(
189+
"acf_beads",
190+
"dolt.example.com",
191+
3306,
192+
errSentinelForTest{msg: "dial tcp: i/o timeout"},
193+
)
194+
195+
if check.Name != "Fresh Clone" {
196+
t.Errorf("expected Name %q, got %q", "Fresh Clone", check.Name)
197+
}
198+
if check.Status != StatusWarning {
199+
t.Errorf("expected Status %q, got %q", StatusWarning, check.Status)
200+
}
201+
202+
wantInMsg := []string{
203+
"unreachable",
204+
"dolt.example.com:3306",
205+
`"acf_beads"`,
206+
"server mode",
207+
}
208+
for _, want := range wantInMsg {
209+
if !strings.Contains(check.Message, want) {
210+
t.Errorf("expected message to contain %q, got: %s", want, check.Message)
211+
}
212+
}
213+
214+
if !strings.Contains(check.Detail, "dial tcp: i/o timeout") {
215+
t.Errorf("expected detail to surface underlying error, got: %s", check.Detail)
216+
}
217+
218+
// Must NOT recommend bd bootstrap — the problem is connectivity, not
219+
// missing local state.
220+
if strings.Contains(check.Fix, "bd bootstrap") {
221+
t.Errorf("server-unreachable fix should not suggest 'bd bootstrap'; got: %s", check.Fix)
222+
}
223+
224+
// Nil error path should still produce a usable message and not panic.
225+
checkNil := freshCloneServerUnreachableResult("beads", "127.0.0.1", 3307, nil)
226+
if checkNil.Status != StatusWarning {
227+
t.Errorf("nil-err path: expected Status %q, got %q", StatusWarning, checkNil.Status)
228+
}
229+
if !strings.Contains(checkNil.Message, "unreachable") {
230+
t.Errorf("nil-err path: expected message to mention 'unreachable', got: %s", checkNil.Message)
159231
}
160232
}
161233

234+
// errSentinelForTest is a minimal error type used to drive the server-unreachable
235+
// helper without depending on the network/MySQL driver. Local to this test file.
236+
type errSentinelForTest struct{ msg string }
237+
238+
func (e errSentinelForTest) Error() string { return e.msg }
239+
162240
func TestCheckFreshClone_EmbeddedMode(t *testing.T) {
163241
// AC-005: Embedded mode (not server mode) uses only filesystem checks.
164242
// No server connection should be attempted.

‎cmd/bd/doctor/legacy.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,13 @@ func CheckFreshClone(repoPath string) DoctorCheck {
499499
}
500500
return freshCloneServerResult(result.Exists, dbName, host, port, syncRemote)
501501
}
502-
// Server unreachable — fall through to existing behavior (FR-030).
502+
// Server unreachable in server mode — emit a server-aware warning
503+
// instead of falling through to the legacy "Fresh clone detected
504+
// (no database)" message, which is a false positive when
505+
// dolt_mode=server (the local DB absence is expected). See GH#35.
506+
// FR-030 only requires that we don't panic on unreachable; it does
507+
// not mandate the misleading fall-through.
508+
return freshCloneServerUnreachableResult(dbName, host, port, result.Err)
503509
}
504510
default:
505511
// SQLite (default): check configured .db file path.

0 commit comments

Comments
 (0)