diff --git a/app/api/settings/[section]/route.ts b/app/api/settings/[section]/route.ts index e7f15a2..aced475 100644 --- a/app/api/settings/[section]/route.ts +++ b/app/api/settings/[section]/route.ts @@ -15,9 +15,14 @@ * level CLAMPS to the named model's nearest supported level and the * clamped value is stored — model swaps just work, same as the * defaults route. The agent section additionally validates prompt - * switches: an ON switch must reference a CURRENT source segment with an - * existing override file (an OFF switch may reference anything — users - * cleaning up orphans must not be blocked). + * switches — under the orphan-heal contract (interlude 2026-09-26): an + * ON switch with its override file present is valid as-is; an ON whose + * file is gone (deleted outside the app, a disk wipe, or a record from + * before the delete-clears-switch fix) is an ORPHAN and is healed to + * OFF (the key drops) inside the same write — never a 400. Invariant: + * a user can never get stuck unable to write settings, and switch + * state never contradicts file reality for long (the delete route + * clears the switch by construction; this is the backstop). * * The static siblings (defaults, prompts) take precedence over this * dynamic segment — Next resolves static routes first, so "prompts" never @@ -58,6 +63,7 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st if (!parsed.ok) { return Response.json({ error: parsed.error }, { status: 400 }); } + let agentHealed: AgentSettings | undefined; // Section-specific cross-validation (against the catalog / the prompt // store) — the parsers only see shapes. @@ -77,19 +83,23 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st } } if (section === "agent") { + // Orphan-heal (contract fix, interlude 2026-09-26): an ON switch + // whose override file is gone (or whose segment left the source + // tree) is healed to OFF (key dropped) inside this write — never a + // 400. The old 400 trapped users: the record's own orphaned ON was + // mirrored back by every write while the disabled toggle couldn't + // reach the OFF escape hatch. An ON with a real file stays valid. const agent = parsed.settings as AgentSettings; const segments = new Set(allSegmentPaths()); + const orphans: string[] = []; for (const [path, on] of Object.entries(agent.prompts ?? {})) { - if (on && (!segments.has(path) || promptOverrides.readOverride(path) === null)) { - return Response.json( - { - error: `prompt switch for "${path}" cannot be on: ${ - segments.has(path) ? "no override file exists" : "not a current source segment" - }`, - }, - { status: 400 }, - ); - } + if (on && (!segments.has(path) || promptOverrides.readOverride(path) === null)) orphans.push(path); + } + if (orphans.length > 0) { + const prompts = { ...(agent.prompts ?? {}) }; + for (const path of orphans) delete prompts[path]; + agentHealed = { ...agent, prompts }; + console.info(`[settings] healed orphaned prompt switch(es) to off: ${orphans.join(", ")}`); } } @@ -100,12 +110,17 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st // value is what lands in the record. The chat section's bare levels // clamp against the record's resolved strong slot (the chain's // fallback); the models section's slots each carry their own model. + // The agent section lands HEALED when orphaned prompt switches were + // found above (the healed copy) — after every write the record and + // file reality agree. const sectionOut = section === "chat" ? clampChatLevel(parsed.settings as ChatSettings, effectiveSettings().models.strong) : section === "models" ? clampModelsLevels(parsed.settings as ModelsSettings) - : parsed.settings; + : section === "agent" && agentHealed + ? agentHealed + : parsed.settings; // Read-modify-write ONE section of the record; everything else rides // along untouched (other sections, unknown keys — sanitize preserves). diff --git a/app/api/settings/prompts/[...segment]/route.ts b/app/api/settings/prompts/[...segment]/route.ts index 574a93c..8fed774 100644 --- a/app/api/settings/prompts/[...segment]/route.ts +++ b/app/api/settings/prompts/[...segment]/route.ts @@ -7,16 +7,31 @@ * — derived from src/agent/prompts at runtime): overriding a * segment that doesn't compose is a typo or a vanished fragment, * a 400, never a silent orphan. - * DELETE → removes the override file (idempotent). Switches are NOT - * touched — an on-switch with no file composes the source - * default (inert by design), so the user's switch state survives - * a delete/re-write cycle. + * DELETE → removes the override file (idempotent) AND clears the + * record's switch for that segment in the same operation + * (contract fix, interlude 2026-09-26 — overrules the 2026-09-19 + * "switch survives a delete/re-write cycle" note). The old + * behavior orphaned the on-switch: the write validation then + * 400'd every agent-section write while the UI's disabled toggle + * couldn't reach the OFF escape hatch — a user stuck unable to + * write settings. Delete clears the switch so the route and the + * record agree BY CONSTRUCTION; the write path additionally heals + * any orphan that still arises (disk-level wipes outside the + * app's control) — see [section]/route.ts. Order here: clear the + * switch first, then unlink — a crash between the two leaves + * "off + file", a valid state, never an orphan. * * The ON/OFF switch itself lives in the record's agent.prompts map * (PUT /api/settings/agent) — file presence alone is never the override. */ -import { allSegmentPaths, isValidSegmentPath, promptOverrides } from "@/src/server"; +import { + allSegmentPaths, + isValidSegmentPath, + promptOverrides, + readSettingsRecord, + writeSettingsRecord, +} from "@/src/server"; export const dynamic = "force-dynamic"; @@ -49,6 +64,17 @@ export async function DELETE(_request: Request, ctx: { params: Promise<{ segment const { segment } = await ctx.params; const path = segmentPathFrom(segment); if (!path) return Response.json({ error: "invalid segment path" }, { status: 400 }); + // Clear the record's switch FIRST (crash between the two leaves "off + + // file" — valid — never an orphaned ON), then unlink the file. + const record = readSettingsRecord(); + const hadSwitch = record?.agent?.prompts?.[path] === true; + if (record && record.agent?.prompts?.[path] !== undefined) { + const prompts = { ...record.agent.prompts }; + delete prompts[path]; + writeSettingsRecord({ ...record, agent: { ...record.agent, prompts } }); + } promptOverrides.deleteOverride(path); - return Response.json({ path, deleted: true }); + // switchCleared tells the caller their on-switch went off WITH the file + // (additive; the GET view derives the same truth from record+file). + return Response.json({ path, deleted: true, switchCleared: hadSwitch }); } diff --git a/tests/prompt-overrides.test.ts b/tests/prompt-overrides.test.ts index 0247d43..357633e 100644 --- a/tests/prompt-overrides.test.ts +++ b/tests/prompt-overrides.test.ts @@ -4,21 +4,78 @@ * path discipline (traversal guard), and the SOURCE-DERIVED segment list * (the loader mirrors teacher-agent.ts's composeInstructions discovery; * these tests pin that the mirror tracks the real src/agent/prompts tree). - * Writes under the repo data/prompts/ (disposable scratch) and cleans up. + * + * TEST-DIR ISOLATION (contract fix, interlude 2026-09-26): data/prompts + * holds REAL user override files — this suite must never touch it (the + * old after() wiped it as "disposable scratch", killing real overrides + * and orphaning their ON switches). LH_DATA_DIR is the established + * data-location seam (Electron contract, 2026-09-17; see + * data-dir-seam.test.ts), evaluated at config.ts's MODULE LOAD — so it + * must be set before the first import that (transitively) loads config. + * Every app-module import below is dynamic for exactly that reason; + * only node builtins and config-independent libs stay static. The seam + * pins at the bottom assert the redirection, and after() snapshots that + * the REAL repo data/prompts survived the suite untouched. + * + * This file OWNS the temp dir (parallel-test discipline: one fs owner + * per file) — every override-writing test lives here, including the + * route-level contract pins for the same interlude (delete-clears-switch, + * orphaned-ON-heals-to-OFF). */ import assert from "node:assert/strict"; import fs from "node:fs"; -import path from "node:path"; +import * as os from "node:os"; +import * as path from "node:path"; import { after, describe, it } from "node:test"; -import { PROMPTS_OVERRIDE_DIR } from "../config.ts"; -import * as teacherAgent from "../src/agent/teacher-agent.ts"; -import { allSegmentPaths, effectivePromptSegments } from "../src/agent/prompt-segments.ts"; -import { isValidSegmentPath } from "../src/core/prompt-segments.ts"; -import { promptOverrides } from "../src/store/prompt-overrides.ts"; import { createFauxCore } from "@earendil-works/pi-ai"; import type { PiEngine } from "../src/agent/models.ts"; +// ── the seam, BEFORE any config-loading import ────────────────────────────── +const TMP_DATA = fs.mkdtempSync(path.join(os.tmpdir(), "lh-prompt-overrides-")); +process.env.LH_DATA_DIR = TMP_DATA; + +const { PROMPTS_OVERRIDE_DIR } = await import("../config.ts"); +const teacherAgent = await import("../src/agent/teacher-agent.ts"); +const { allSegmentPaths, effectivePromptSegments } = await import("../src/agent/prompt-segments.ts"); +const { isValidSegmentPath } = await import("../src/core/prompt-segments.ts"); +const { promptOverrides } = await import("../src/store/prompt-overrides.ts"); +// Route-level pins drive the actual app/api handlers (framework-free +// Request/Response), isolated under the same temp LH_DATA_DIR. +const { PUT: putSettingsSection } = await import("../app/api/settings/[section]/route.ts"); +const { DELETE: deletePromptText } = await import("../app/api/settings/prompts/[...segment]/route.ts"); +const { readSettingsRecord } = await import("../src/server/index.ts"); + +/** The REAL repo prompts dir this suite must never touch (read-only). */ +const REPO_PROMPTS = path.resolve(import.meta.dirname, "../data/prompts"); + +/** Read-only recursive snapshot of a dir (sorted "rel|size" lines) — the + * survival pin compares before/after the whole suite. */ +function snapshotDir(dir: string): string[] { + if (!fs.existsSync(dir)) return []; + const out: string[] = []; + const walk = (rel: string) => { + for (const entry of fs.readdirSync(path.join(dir, rel), { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name))) { + const child = rel ? `${rel}/${entry.name}` : entry.name; + if (entry.isDirectory()) walk(child); + else out.push(`${child}|${fs.statSync(path.join(dir, child)).size}`); + } + }; + walk(""); + return out; +} +const REAL_PROMPTS_BEFORE = snapshotDir(REPO_PROMPTS); + +after(() => { + // The suite's own scratch dies; the REAL user dir must be untouched. + assert.deepEqual( + snapshotDir(REPO_PROMPTS), + REAL_PROMPTS_BEFORE, + "real data/prompts survives npm test byte-for-byte (the old suite wiped it)", + ); + fs.rmSync(TMP_DATA, { recursive: true, force: true }); +}); + /** A scripted pi engine (pi-ai's faux provider) — the agent build needs an * injected engine since BYOK removed the factory pins (owner, 2026-09-22). */ function fauxEngine(): PiEngine { @@ -26,9 +83,24 @@ function fauxEngine(): PiEngine { return { model: faux.getModel(), streamFn: faux.streamSimple }; } -after(() => { - fs.rmSync(PROMPTS_OVERRIDE_DIR, { recursive: true, force: true }); -}); +/** PUT /api/settings/
with a fabricated params context. */ +function putSection(section: string, body: unknown): Promise { + return putSettingsSection( + new Request(`http://test/api/settings/${section}`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }), + { params: Promise.resolve({ section }) }, + ); +} + +/** DELETE /api/settings/prompts/ with a fabricated params context. */ +function deleteSegment(segment: string[]): Promise { + return deletePromptText(new Request("http://test/api/settings/prompts", { method: "DELETE" }), { + params: Promise.resolve({ segment }), + }); +} describe("isValidSegmentPath (the mirror-layout discipline)", () => { it("accepts the shared base and numeric fragments, rejects everything else", () => { @@ -94,9 +166,9 @@ describe("the source-derived segment list", () => { // The shared base composes both prompts, SAME path. assert.equal(prober[0]?.path, "system-prompt.md"); assert.equal(teacher[0]?.path, "system-prompt.md"); - assert.equal(prober[0]?.shared, true); - assert.equal(teacher[0]?.shared, true); - assert.equal(prober[0]?.sourceText.length > 0, true, "base prompt text loads"); + assert.equal(prober[0].shared, true); + assert.equal(teacher[0].shared, true); + assert.equal(prober[0].sourceText.length > 0, true, "base prompt text loads"); // Fragments track the REAL source tree (numeric order, right agent dir), // with the shared/ fragments between the base and the agent's own — // the exact order composeInstructions loads them in (P2, 2026-09-19). @@ -165,8 +237,8 @@ describe("the source-derived segment list", () => { }); // ─── composeInstructions honors overrides (P2 reconciliation, 2026-09-19) ──── -// This file OWNS PROMPTS_OVERRIDE_DIR (parallel-test discipline), so every -// override-writing composition test lives here. +// This file OWNS the (isolated) PROMPTS_OVERRIDE_DIR (parallel-test +// discipline), so every override-writing composition test lives here. describe("composeInstructions: override honoring + the byte-identical source path", () => { const { composeInstructions, createTeacherAgent } = teacherAgent; @@ -242,3 +314,113 @@ describe("composeInstructions: override honoring + the byte-identical source pat } }); }); + +// ─── the orphan-heal contract (interlude, 2026-09-26) ──────────────────────── +// The orphan-400 trap: an ON switch whose file dies orphaned the record; +// every agent-section write then 400'd while the disabled toggle couldn't +// reach the OFF escape hatch — a user stuck unable to write settings. +// Fix: the delete route clears the switch by construction, AND the write +// path heals any orphan that still arises (disk wipes outside the app). + +describe("delete clears the switch — the record and file agree by construction", () => { + const SEGMENT = "teacher/20-teaching.md"; + + it("DELETE removes the file AND clears the on-switch in one operation", async () => { + promptOverrides.writeOverride(SEGMENT, "custom teaching"); + const on = await putSection("agent", { prompts: { [SEGMENT]: true } }); + assert.equal(on.status, 200, "ON with a real file is valid"); + assert.equal(readSettingsRecord()?.agent?.prompts?.[SEGMENT], true, "switch landed in the record"); + + const res = await deleteSegment(["teacher", "20-teaching.md"]); + assert.equal(res.status, 200); + const body = (await res.json()) as { deleted: boolean; switchCleared: boolean }; + assert.deepEqual(body, { path: SEGMENT, deleted: true, switchCleared: true }, "the response names what happened"); + + assert.equal(readSettingsRecord()?.agent?.prompts?.[SEGMENT], undefined, "the switch is GONE from the record"); + assert.equal(promptOverrides.readOverride(SEGMENT), null, "the file is gone"); + // And the trap's step 4 — flipping any tool toggle afterwards — + // just works (nothing orphaned to mirror): + const followUp = await putSection("agent", { tools: { research: false } }); + assert.equal(followUp.status, 200, "a subsequent agent write succeeds"); + assert.equal(readSettingsRecord()?.agent?.tools?.research, false); + }); + + it("DELETE is idempotent for file and switch — nothing to clear, still 200", async () => { + const res = await deleteSegment(["teacher", "20-teaching.md"]); + assert.equal(res.status, 200); + const body = (await res.json()) as { switchCleared: boolean }; + assert.equal(body.switchCleared, false, "no switch was present — nothing cleared"); + }); +}); + +describe("an orphaned ON heals to OFF on write — never a 400 (the invariant: a user can never get stuck)", () => { + const SEGMENT = "teacher/20-teaching.md"; + + it("the exact repro: file wiped at disk level → UI mirrors the orphan and flips a tool toggle → the write SUCCEEDS and heals", async () => { + promptOverrides.writeOverride(SEGMENT, "custom teaching"); + const on = await putSection("agent", { prompts: { [SEGMENT]: true } }); + assert.equal(on.status, 200); + + // The disk-level wipe — OUTSIDE the app (the delete route now clears + // the switch, so this is the only way an orphan still arises). + fs.rmSync(path.join(PROMPTS_OVERRIDE_DIR, "teacher", "20-teaching.md")); + + // The exact trap shape: the client mirrors the record's orphaned ON + // while writing an unrelated tool toggle. Today (pre-fix) → 400 forever. + const res = await putSection("agent", { tools: { research: false }, prompts: { [SEGMENT]: true } }); + assert.equal(res.status, 200, "the write heals instead of 400-ing — the user is never stuck"); + + const record = readSettingsRecord(); + assert.equal(record?.agent?.prompts?.[SEGMENT], undefined, "the orphaned ON landed as OFF (key dropped)"); + assert.equal(record?.agent?.tools?.research, false, "the rest of the write landed — nothing was silently dropped"); + + // And the SECOND write (the "400 forever" of the trap) succeeds too: + const again = await putSection("agent", { tools: { research: false }, prompts: { [SEGMENT]: true } }); + assert.equal(again.status, 200, "repeated writes keep succeeding"); + }); + + it("an ON for a segment that left the source tree heals the same way", async () => { + promptOverrides.writeOverride(SEGMENT, "custom teaching"); + // A vanished-fragment orphan: the switch keys a path that is no + // longer a current source segment (simulate by keying junk that the + // store never guarded — the record is the only place it exists). + const on = await putSection("agent", { prompts: { "teacher/99-vanished.md": true, [SEGMENT]: true } }); + assert.equal(on.status, 200, "vanished-fragment ON heals too, not a 400"); + assert.equal(readSettingsRecord()?.agent?.prompts?.["teacher/99-vanished.md"], undefined, "the vanished key healed to OFF"); + promptOverrides.deleteOverride(SEGMENT); + }); + + it("a VALID ON (file present) stays valid — no heal, no 400", async () => { + promptOverrides.writeOverride(SEGMENT, "custom teaching"); + try { + const res = await putSection("agent", { prompts: { [SEGMENT]: true } }); + assert.equal(res.status, 200); + const record = (await res.json()) as { settings: { agent?: { prompts?: Record } } }; + assert.equal(record.settings.agent?.prompts?.[SEGMENT], true, "a real ON+file write keeps the switch on"); + } finally { + promptOverrides.deleteOverride(SEGMENT); + } + }); +}); + +describe("test isolation (the suite must never touch real user data)", () => { + it("every write this suite makes lands under the LH_DATA_DIR seam, not the repo data dir", () => { + assert.ok( + PROMPTS_OVERRIDE_DIR.startsWith(TMP_DATA), + `PROMPTS_OVERRIDE_DIR (${PROMPTS_OVERRIDE_DIR}) must live under the temp seam (${TMP_DATA})`, + ); + assert.notEqual( + PROMPTS_OVERRIDE_DIR, + REPO_PROMPTS, + "the seam must NOT resolve to the repo's real data/prompts", + ); + // The suite proves it writes here: + promptOverrides.writeOverride("system-prompt.md", "isolation probe"); + assert.equal( + fs.existsSync(path.join(TMP_DATA, "prompts", "system-prompt.md")), + true, + "the write landed under the temp dir", + ); + promptOverrides.deleteOverride("system-prompt.md"); + }); +});