From 549624ffb2046e5c1ca8a4e2c1057e9dbf294100 Mon Sep 17 00:00:00 2001 From: HCL Date: Thu, 30 Apr 2026 07:22:11 +0800 Subject: [PATCH] fix(doctor): add skipped discriminator to distinguish probe skip from gateway timeout Previously both a planned probe skip (probe:false path) and a transport timeout returned checked:false, so the renderer's !checked early return would silently suppress diagnostics for key-optional providers even when the gateway had timed out. - Add `skipped?: boolean` to GatewayMemoryProbe: true for gateway-confirmed skip, false for timeout/unavailable paths - Renderer now guards on `probe.skipped` instead of `!probe.checked`, so timeouts fall through to the existing warning path - Update doctor-memory-search inline type and buildGatewayProbeWarning signature - Update skipped-probe tests to pass { skipped: true }; add regression test for key-optional timeout (lmstudio gateway timeout now warns) Addresses clawsweeper P2: src/commands/doctor-memory-search.ts:416 --- src/commands/doctor-gateway-health.test.ts | 13 ++++++-- src/commands/doctor-gateway-health.ts | 12 ++++++++ src/commands/doctor-memory-search.test.ts | 36 ++++++++++++++++++---- src/commands/doctor-memory-search.ts | 14 ++++++--- 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/commands/doctor-gateway-health.test.ts b/src/commands/doctor-gateway-health.test.ts index 4cd21a5dd94..050fb7889d6 100644 --- a/src/commands/doctor-gateway-health.test.ts +++ b/src/commands/doctor-gateway-health.test.ts @@ -74,6 +74,7 @@ describe("probeGatewayMemoryStatus", () => { checked: true, ready: true, error: undefined, + skipped: false, }); expect(callGateway).toHaveBeenCalledWith({ @@ -84,7 +85,9 @@ describe("probeGatewayMemoryStatus", () => { }); }); - it("treats outer gateway timeouts as inconclusive", async () => { + it("treats outer gateway timeouts as inconclusive (skipped: false)", async () => { + // A transport timeout must NOT be treated as a skipped probe. It is a real + // diagnostic signal and the renderer should warn for key-optional providers. callGateway.mockRejectedValue( new Error("gateway timeout after 8000ms\nGateway target: ws://127.0.0.1:18789"), ); @@ -93,12 +96,15 @@ describe("probeGatewayMemoryStatus", () => { checked: false, ready: false, error: expect.stringContaining("gateway memory probe timed out"), + skipped: false, }); }); - it("propagates checked: false when gateway skipped the embedding probe", async () => { + it("propagates checked: false and skipped: true when gateway skipped the embedding probe", async () => { // Gateway returns checked: false when called with probe: false and no cached - // availability data (SKIPPED_MEMORY_EMBEDDING_PROBE shape). + // availability data (SKIPPED_MEMORY_EMBEDDING_PROBE shape). The adapter must + // also set skipped: true so renderers can distinguish this from a transport + // timeout (which also returns checked: false but skipped: false). callGateway.mockResolvedValue({ embedding: { ok: false, @@ -112,6 +118,7 @@ describe("probeGatewayMemoryStatus", () => { checked: false, ready: false, error: expect.stringContaining("not checked"), + skipped: true, }); }); diff --git a/src/commands/doctor-gateway-health.ts b/src/commands/doctor-gateway-health.ts index 86a78131978..3a4ae71910d 100644 --- a/src/commands/doctor-gateway-health.ts +++ b/src/commands/doctor-gateway-health.ts @@ -11,6 +11,13 @@ export type GatewayMemoryProbe = { checked: boolean; ready: boolean; error?: string; + /** + * True when the probe was intentionally skipped by the gateway (probe: false + * path). Distinct from checked: false caused by a network timeout or + * unavailable gateway. Renderers should suppress warnings only for skipped + * probes, not for transport failures. + */ + skipped?: boolean; }; function isGatewayCallTimeout(message: string): boolean { @@ -91,11 +98,14 @@ export async function probeGatewayMemoryStatus(params: { // readiness determination was made. Mapping that to checked: true here would // cause the renderer to treat a skipped probe as a checked-but-not-ready // failure and emit a false-positive warning for key-optional providers. + // We also carry skipped: true so renderers can distinguish an intentional + // non-deep skip from a transport timeout (which also returns checked: false). const gatewayChecked = payload.embedding.checked !== false; return { checked: gatewayChecked, ready: payload.embedding.ok, error: payload.embedding.error, + skipped: !gatewayChecked, }; } catch (err) { const message = formatErrorMessage(err); @@ -104,12 +114,14 @@ export async function probeGatewayMemoryStatus(params: { checked: false, ready: false, error: `gateway memory probe timed out: ${message}`, + skipped: false, }; } return { checked: true, ready: false, error: `gateway memory probe unavailable: ${message}`, + skipped: false, }; } } diff --git a/src/commands/doctor-memory-search.test.ts b/src/commands/doctor-memory-search.test.ts index 3171dfedaba..93b7215342a 100644 --- a/src/commands/doctor-memory-search.test.ts +++ b/src/commands/doctor-memory-search.test.ts @@ -459,10 +459,11 @@ describe("noteMemorySearchHealth", () => { expect(message).toContain("embeddings are not ready"); }); - it("does not warn when key-optional provider (lmstudio) probe was skipped (checked: false)", async () => { + it("does not warn when key-optional provider (lmstudio) probe was skipped (skipped: true)", async () => { // When `openclaw doctor` runs without --deep, the probe is skipped and returns - // { checked: false, ready: false }. This must NOT produce a false-positive warning — - // it means readiness was never checked, not that embeddings are unavailable. + // { checked: false, ready: false, skipped: true }. This must NOT produce a + // false-positive warning — it means readiness was never checked, not that + // embeddings are unavailable. // Regression test for: https://github.com/openclaw/openclaw/issues/74608 resolveMemorySearchConfig.mockReturnValue({ provider: "lmstudio", @@ -471,13 +472,13 @@ describe("noteMemorySearchHealth", () => { }); await noteMemorySearchHealth(cfg, { - gatewayMemoryProbe: { checked: false, ready: false }, + gatewayMemoryProbe: { checked: false, ready: false, skipped: true }, }); expect(note).not.toHaveBeenCalled(); }); - it("does not warn when key-optional provider (ollama) probe was skipped (checked: false)", async () => { + it("does not warn when key-optional provider (ollama) probe was skipped (skipped: true)", async () => { // Same guard for ollama — the most commonly reported false-positive case. resolveMemorySearchConfig.mockReturnValue({ provider: "ollama", @@ -486,12 +487,35 @@ describe("noteMemorySearchHealth", () => { }); await noteMemorySearchHealth(cfg, { - gatewayMemoryProbe: { checked: false, ready: false }, + gatewayMemoryProbe: { checked: false, ready: false, skipped: true }, }); expect(note).not.toHaveBeenCalled(); }); + it("warns for key-optional provider (lmstudio) when gateway probe timed out", async () => { + // A gateway timeout sets checked: false but skipped: false/absent. This is a + // real diagnostic signal — embeddings may be unavailable — so we should warn. + // Regression guard: https://github.com/openclaw/openclaw/issues/74608 + resolveMemorySearchConfig.mockReturnValue({ + provider: "lmstudio", + local: {}, + remote: {}, + }); + + await noteMemorySearchHealth(cfg, { + gatewayMemoryProbe: { + checked: false, + ready: false, + error: "gateway memory probe timed out: gateway timeout after 8000ms", + skipped: false, + }, + }); + + const message = String(note.mock.calls[0]?.[0] ?? ""); + expect(message).toContain('provider "lmstudio" is configured'); + }); + it("notes when gateway probe reports embeddings ready and CLI API key is missing", async () => { resolveMemorySearchConfig.mockReturnValue({ provider: "gemini", diff --git a/src/commands/doctor-memory-search.ts b/src/commands/doctor-memory-search.ts index 02979d7419e..490ac2f31ec 100644 --- a/src/commands/doctor-memory-search.ts +++ b/src/commands/doctor-memory-search.ts @@ -316,6 +316,7 @@ export async function noteMemorySearchHealth( checked: boolean; ready: boolean; error?: string; + skipped?: boolean; }; }, ): Promise { @@ -410,10 +411,14 @@ export async function noteMemorySearchHealth( if (opts?.gatewayMemoryProbe?.checked && opts.gatewayMemoryProbe.ready) { return; } - // When the probe was skipped (checked: false), we have no embedding status - // information — do not warn. A skipped probe means the user ran `openclaw doctor` - // without --deep; it does not mean embeddings are unavailable. - if (opts?.gatewayMemoryProbe && !opts.gatewayMemoryProbe.checked) { + // When the probe was intentionally skipped (skipped: true / checked: false + // due to probe:false path), we have no embedding status information — do + // not warn. A skipped probe means the user ran `openclaw doctor` without + // --deep; it does not mean embeddings are unavailable. + // NOTE: a transport timeout also sets checked: false, but skipped stays + // false/absent — a timeout is a real diagnostic signal and should fall + // through to the warning below. + if (opts?.gatewayMemoryProbe?.skipped) { return; } const gatewayProbeWarning = buildGatewayProbeWarning(opts?.gatewayMemoryProbe); @@ -585,6 +590,7 @@ function buildGatewayProbeWarning( checked: boolean; ready: boolean; error?: string; + skipped?: boolean; } | undefined, ): string | null {