From f14741cad976a97f92f977553f215e19cf49cd18 Mon Sep 17 00:00:00 2001 From: LiangshouX Date: Tue, 3 Nov 2026 13:23:25 +0800 Subject: [PATCH] fix(dsh): write the default backend when the selection is cleared The backend selection is the one managed key with a default. Clearing it wrote an empty value, and an empty value is not the default: ReMe applies "${LLM_BACKEND:-openai}" only when the variable is absent, so the service started with no backend at all and refused to run - while the plugin's own probe resolved the same empty value to openai and reported a working configuration. The cleared selection now writes the default out, and the probe reports an explicitly empty backend as an incomplete configuration instead of testing a configuration the service cannot use. A file without the key keeps the default, and an unknown backend name stays probed tolerantly: the list of backends here is a snapshot, and ReMe may know more. --- .../dsh/src/environment/env-fields.ts | 40 +++++- .../dsh/src/environment/host/workspace.ts | 40 ++++-- .../dsh/src/environment/llm-backend.ts | 16 ++- integrations/dsh/tests/env-fields.test.mjs | 56 ++++++++- integrations/dsh/tests/env-workspace.test.mjs | 115 +++++++++++++++++- 5 files changed, 245 insertions(+), 22 deletions(-) diff --git a/integrations/dsh/src/environment/env-fields.ts b/integrations/dsh/src/environment/env-fields.ts index 0cf78056..396f1f5c 100644 --- a/integrations/dsh/src/environment/env-fields.ts +++ b/integrations/dsh/src/environment/env-fields.ts @@ -3,15 +3,20 @@ * * This module is the single source of truth for the managed keys and for which * of them are write-only secrets. An empty secret draft means "leave blank to - * keep" and an empty non-secret draft is an explicit clear; both rules are + * keep" and an empty non-secret draft is an explicit clear; the rules are * derived from the `secret` flag alone, so a key cannot carry a rule that - * contradicts its own kind. The Host and the browser both derive their - * behaviour from it, so the two can never drift apart about the same file. + * contradicts its own kind. A key that names a default (`defaultValue`) is the + * one exception: its empty draft means "use the default", and the default is + * written out, because the service only falls back to it when the variable is + * absent. The Host and the browser both derive their behaviour from it, so the + * two can never drift apart about the same file. * * It stays browser-safe: no node built-in is imported here, and the browser * bundle must stay loadable without one. */ +import { DEFAULT_LLM_BACKEND } from "./llm-backend.js"; + export interface EnvFieldMeta { /** The `.env` key. */ key: string; @@ -28,6 +33,15 @@ export interface EnvFieldMeta { * is off; `EMPTY` is the documented placeholder for "no embedding key". */ disabledValue?: string; + /** + * The value an empty selection is written as. + * + * An empty selection means "use the default". The default is written + * explicitly, because ReMe's configuration only falls back to it when the + * variable is absent: an empty value stays empty and names no backend, and + * the service then refuses to start. + */ + defaultValue?: string; } /** @@ -37,7 +51,14 @@ export interface EnvFieldMeta { * and the browser form list the same keys in the same order. */ export const ENV_FIELDS: readonly EnvFieldMeta[] = [ - { key: "LLM_BACKEND", secret: false }, + { + key: "LLM_BACKEND", + secret: false, + // ReMe resolves `${LLM_BACKEND:-openai}` only when the variable is + // absent, so a cleared selection must write the default out rather than + // an empty value. + defaultValue: DEFAULT_LLM_BACKEND, + }, { key: "LLM_MODEL_NAME", secret: false }, { key: "LLM_API_KEY", secret: true }, { key: "LLM_BASE_URL", secret: false }, @@ -87,7 +108,9 @@ export function storedState( * Only touched keys travel: an untouched key keeps its stored value. An empty * secret draft is dropped rather than written, so "leave blank to keep" holds * even after the user typed a replacement and cleared it again. An empty - * non-secret draft is an explicit clear and keeps that meaning. + * non-secret draft is an explicit clear and keeps that meaning — unless the + * key names a default, in which case the empty selection means "use the + * default" and the default is written out. * * `stored` reports, per key, whether the file already holds a non-empty value. * It is what decides whether a key that must not stay empty gets its @@ -121,6 +144,13 @@ export function envUpdates( updates[field.key] = field.disabledValue; continue; } + // "Use the default": the default is written out, because the service + // only falls back to it when the variable is absent. An empty value + // would name no backend at all. + if (field.defaultValue) { + updates[field.key] = field.defaultValue; + continue; + } updates[field.key] = ""; continue; } diff --git a/integrations/dsh/src/environment/host/workspace.ts b/integrations/dsh/src/environment/host/workspace.ts index a43abe8b..94e29cfe 100644 --- a/integrations/dsh/src/environment/host/workspace.ts +++ b/integrations/dsh/src/environment/host/workspace.ts @@ -278,7 +278,14 @@ export const LLM_PROTOCOLS: Record = { /** The backend ReMe itself falls back to when `.env` names none. */ export { DEFAULT_LLM_BACKEND }; -/** The protocol a backend speaks; unknown backends use ReMe's default. */ +/** + * The protocol a backend speaks; an unknown backend uses the OpenAI-style + * request. + * + * The list of backends here is a snapshot of the ones the tab offers, and + * ReMe may support more, so an unknown name is probed tolerantly rather than + * refused. + */ export function llmProtocolFor(backend: string): LlmProtocol { return LLM_PROTOCOLS[llmBackendOf(backend)] ?? OPENAI_PROTOCOL; } @@ -353,17 +360,22 @@ except Exception as error: } /** - * The LLM backend the workspace `.env` selects. An unreadable or incomplete - * file names none, which leaves the probe on ReMe's default backend. + * The LLM backend the workspace `.env` selects. + * + * `undefined` means the file does not hold the key at all: the service then + * falls back to its default. A present key is returned as its trimmed value, + * which may be the empty string — that names no backend at all, and ReMe + * refuses to start with it, so the caller has to know the difference. */ -export function readLlmBackend(envPath: string): string { +export function readLlmBackend(envPath: string): string | undefined { let text = ""; try { text = readFileSync(envPath, "utf8"); } catch { - return ""; + return undefined; } - return (parseEnvFile(text)["LLM_BACKEND"] ?? "").trim(); + const value = parseEnvFile(text)["LLM_BACKEND"]; + return value === undefined ? undefined : value.trim(); } /** Parse the JSON line the LLM test snippet prints. */ @@ -399,9 +411,19 @@ export async function testLlm( // The backend in the workspace `.env` decides the protocol; the probe must // not fall back to an OpenAI-style request for a provider that speaks its // own, or it would report a working configuration as broken. - const protocol = llmProtocolFor( - readLlmBackend(join(options.workspaceDir, ".env")), - ); + const backend = readLlmBackend(join(options.workspaceDir, ".env")); + // A present but empty backend names no backend. ReMe resolves its default + // only when the variable is absent, so the service would start with no + // backend at all and refuse to run: the honest answer is an incomplete + // configuration, and the request is not sent. + if (backend === "") + return { + ok: false, + status: 0, + latencyMs: 0, + error: "LLM config incomplete (LLM_BACKEND)", + }; + const protocol = llmProtocolFor(backend ?? DEFAULT_LLM_BACKEND); try { const result = await io.run( options.pythonPath, diff --git a/integrations/dsh/src/environment/llm-backend.ts b/integrations/dsh/src/environment/llm-backend.ts index d3b5185d..726bde34 100644 --- a/integrations/dsh/src/environment/llm-backend.ts +++ b/integrations/dsh/src/environment/llm-backend.ts @@ -1,17 +1,23 @@ /** * The LLM backend the workspace `.env` selects. * - * ReMe's own configuration resolves an empty value to `openai` - * (`${LLM_BACKEND:-openai}`), and the Host's probe resolves it the same way. - * The tab has to agree: a backend that is only picked in the form and equals - * the default is not a change to the configuration. + * ReMe applies its default (`${LLM_BACKEND:-openai}`) only when the variable + * is **absent**: an explicitly empty value stays empty and names no backend, + * and the service then refuses to start. So the default has to be written out + * rather than left blank — see `environment/env-fields.ts`, which owns that + * rule. + * + * `llmBackendOf` is the *form's* reading, not the service's: an empty + * selection there means "use the default", so it resolves to the default + * backend. That is what the tab compares with, so picking the default in the + * form is not a change to the configuration. * * This module stays browser-safe: no Node built-ins, because the client * imports it too. */ export const DEFAULT_LLM_BACKEND = "openai"; -/** The backend a value means, with the default the configuration applies. */ +/** The backend a value means in the form, with the default the configuration applies. */ export function llmBackendOf(value: string): string { return (value ?? "").trim().toLowerCase() || DEFAULT_LLM_BACKEND; } diff --git a/integrations/dsh/tests/env-fields.test.mjs b/integrations/dsh/tests/env-fields.test.mjs index a01dab00..7b35788b 100644 --- a/integrations/dsh/tests/env-fields.test.mjs +++ b/integrations/dsh/tests/env-fields.test.mjs @@ -9,6 +9,7 @@ import { SECRET_KEYS, storedState, } from "../dist/environment/env-fields.js"; +import { DEFAULT_LLM_BACKEND } from "../dist/environment/llm-backend.js"; /** A file that already holds a value for every key that needs one. */ const STORED = { EMBEDDING_API_KEY: true }; @@ -40,8 +41,9 @@ test("only the two credentials are secret", () => { }); test("the metadata of a key carries its secret flag and placeholder", () => { - // What an empty draft means is not metadata: it follows the `secret` flag - // alone, which the update rules below apply. + // What an empty draft means follows the `secret` flag, which the update + // rules below apply. A key that names a default is the documented + // exception. assert.equal(envFieldMeta("LLM_API_KEY")?.secret, true); assert.equal(envFieldMeta("LLM_BASE_URL")?.secret, false); assert.equal(envFieldMeta("EMBEDDING_API_KEY")?.disabledValue, "EMPTY"); @@ -124,3 +126,53 @@ test("a key the tab does not manage travels unchanged", () => { SECRET_OTHER: "value", }); }); + +test("an empty backend selection is written as the default, not as an empty value", () => { + // ReMe resolves `${LLM_BACKEND:-openai}` only when the variable is absent: + // an explicitly empty value names no backend, and the service refuses to + // start with it. So the cleared selection writes the default out. + const updates = envUpdates({ LLM_BACKEND: "" }, STORED); + assert.deepEqual(updates, { LLM_BACKEND: DEFAULT_LLM_BACKEND }); + assert.equal(updates.LLM_BACKEND, "openai"); + assert.notEqual(updates.LLM_BACKEND, ""); +}); + +test("a typed backend still travels", () => { + const updates = envUpdates({ LLM_BACKEND: "dashscope" }, STORED); + assert.deepEqual(updates, { LLM_BACKEND: "dashscope" }); +}); + +test("an untouched backend key does not travel", () => { + // The stored backend survives the save, exactly like every other key. + const updates = envUpdates({ LLM_MODEL_NAME: "qwen3.7-plus" }, STORED); + assert.equal("LLM_BACKEND" in updates, false); +}); + +test("the default rule does not touch the other keys", () => { + // A blank secret still keeps the stored credential, and a key that must not + // stay empty still gets its placeholder. + const secret = envUpdates( + { LLM_API_KEY: "", LLM_BACKEND: "" }, + { LLM_API_KEY: true, EMBEDDING_API_KEY: true }, + ); + assert.equal("LLM_API_KEY" in secret, false); + assert.equal(secret.LLM_BACKEND, "openai"); + + const placeholder = envUpdates( + { EMBEDDING_API_KEY: "" }, + { EMBEDDING_API_KEY: false }, + ); + assert.equal(placeholder.EMBEDDING_API_KEY, "EMPTY"); + + // A non-secret key without a default is still an explicit clear. + const cleared = envUpdates({ LLM_BASE_URL: "" }, STORED); + assert.deepEqual(cleared, { LLM_BASE_URL: "" }); +}); + +test("only the backend names a default", () => { + // The embedding backend has no default: ReMe's embedding components are + // commented out, so the plugin must not invent a rule for it. + assert.equal(envFieldMeta("LLM_BACKEND")?.defaultValue, "openai"); + assert.equal(envFieldMeta("EMBEDDING_BACKEND")?.defaultValue, undefined); + assert.equal(envFieldMeta("LLM_MODEL_NAME")?.defaultValue, undefined); +}); diff --git a/integrations/dsh/tests/env-workspace.test.mjs b/integrations/dsh/tests/env-workspace.test.mjs index 2ae55d30..f103f8f3 100644 --- a/integrations/dsh/tests/env-workspace.test.mjs +++ b/integrations/dsh/tests/env-workspace.test.mjs @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -17,8 +17,30 @@ import { testLlm, validateEnvUpdate, } from "../dist/environment/host/workspace.js"; +import { ReMeEnvManager } from "../dist/environment/host/manager.js"; import { MANAGED_KEYS } from "../dist/environment/env-fields.js"; +/** A manager whose process work is replaced by facts. */ +function managerIn(t) { + const directory = mkdtempSync(join(tmpdir(), "reme-env-save-")); + t.after(() => rmSync(directory, { recursive: true, force: true })); + const manager = new ReMeEnvManager( + { DSH_HOME: directory }, + { + fetchHealth: async () => ({ ok: false, version: "", error: "" }), + portOpen: async () => false, + spawnService: async () => ({ pid: 1 }), + killTree() {}, + probePid() {}, + processImage: async () => "python.exe", + processCommandLine: async () => "", + exists: () => true, + platform: "win32", + }, + ); + return { manager, directory }; +} + test("resolves the workspace directory with ~/.reme defaults", () => { const home = process.platform === "win32" ? "C:\\Users\\u" : "/home/u"; assert.equal(resolveWorkspaceDir("", home), join(home, ".reme")); @@ -390,3 +412,94 @@ test("a backend that ignores LLM_BASE_URL is probed with its own endpoint", asyn assert.match(openai, /missing\.append\("LLM_BASE_URL"\)/); assert.match(openai, /env\.get\("LLM_BASE_URL"/); }); + +test("a cleared backend is saved as the default, never as an empty value", (t) => { + // ReMe resolves `${LLM_BACKEND:-openai}` only when the variable is absent. + // An empty value would name no backend, and the service would refuse to + // start, so the saved file must hold the default. + const { manager, directory } = managerIn(t); + const envPath = join(directory, ".env"); + writeFileSync( + envPath, + "LLM_BACKEND=dashscope\nLLM_API_KEY=sk-keep\nEMBEDDING_API_KEY=EMPTY\n", + ); + + manager.writeEnv(directory, { LLM_BACKEND: "" }); + + const text = readFileSync(envPath, "utf8"); + assert.match(text, /^LLM_BACKEND=openai$/m); + assert.doesNotMatch(text, /^LLM_BACKEND=$/m); + // The other keys are untouched. + assert.match(text, /^LLM_API_KEY=sk-keep$/m); + assert.match(text, /^EMBEDDING_API_KEY=EMPTY$/m); +}); + +test("the probe refuses a .env whose backend is empty", async (t) => { + // An explicitly empty backend names no backend, and ReMe would start with + // none at all. The honest answer is an incomplete configuration, and no + // request is sent. + const directory = mkdtempSync(join(tmpdir(), "reme-llm-empty-")); + t.after(() => rmSync(directory, { recursive: true, force: true })); + writeFileSync( + join(directory, ".env"), + [ + "LLM_BACKEND=", + "LLM_MODEL_NAME=test-model", + "LLM_API_KEY=sk-test", + "LLM_BASE_URL=https://provider.invalid", + ].join("\n"), + ); + const calls = []; + const result = await testLlm( + { + run: async (exe, args, options) => { + calls.push({ exe, args, options }); + return { + code: 0, + stdout: '{"ok": true, "status": 200, "latencyMs": 12, "error": ""}\n', + stderr: "", + }; + }, + }, + { workspaceDir: directory, pythonPath: "C:\\env\\python.exe" }, + ); + assert.deepEqual(result, { + ok: false, + status: 0, + latencyMs: 0, + error: "LLM config incomplete (LLM_BACKEND)", + }); + assert.equal(calls.length, 0, "the snippet is not run"); +}); + +test("a .env without the backend key is probed with the default protocol", async (t) => { + // The key is absent, so ReMe applies its default; the probe does the same. + const directory = mkdtempSync(join(tmpdir(), "reme-llm-absent-")); + t.after(() => rmSync(directory, { recursive: true, force: true })); + writeFileSync( + join(directory, ".env"), + [ + "LLM_MODEL_NAME=test-model", + "LLM_API_KEY=sk-test", + "LLM_BASE_URL=https://provider.invalid", + ].join("\n"), + ); + const calls = []; + await testLlm( + { + run: async (exe, args, options) => { + calls.push({ exe, args, options }); + return { + code: 0, + stdout: '{"ok": true, "status": 200, "latencyMs": 12, "error": ""}\n', + stderr: "", + }; + }, + }, + { workspaceDir: directory, pythonPath: "C:\\env\\python.exe" }, + ); + const snippet = String(calls[0]?.args[1] ?? ""); + assert.match(snippet, /\/chat\/completions/); + assert.match(snippet, /"Authorization"/); + assert.match(snippet, /Bearer /); +});