From fa89c1551a509690dee767165124450aac322c58 Mon Sep 17 00:00:00 2001 From: Erik Kotsiuba Date: Mon, 1 Jun 2026 00:52:30 +0200 Subject: [PATCH] fix: avoid destructive ask config writes --- README.md | 6 +- docs/architecture.md | 12 ++-- docs/configuration.md | 10 +-- docs/contract.md | 6 +- src/config/path-migrations.ts | 56 ---------------- src/config/store.ts | 123 ++++++++++++++++++++++------------ src/ui/controller.ts | 20 ++++-- src/ui/settings-list.ts | 10 ++- tests/config-store.test.ts | 93 +++++++++++++++---------- tests/settings-list.test.ts | 36 +++++++++- 10 files changed, 213 insertions(+), 159 deletions(-) delete mode 100644 src/config/path-migrations.ts diff --git a/README.md b/README.md index 8419dce..75082dd 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ # @eko24ive/pi-ask [![npm downloads](https://badgen.net/npm/dm/@eko24ive/pi-ask)](https://www.npmjs.com/package/@eko24ive/pi-ask) -[![last commit](https://badgen.net/github/last-commit/eko24ive/pi-ask?v=45d8302)](https://github.com/eko24ive/pi-ask/commits/main) +[![last commit](https://badgen.net/github/last-commit/eko24ive/pi-ask?v=bf3c26d)](https://github.com/eko24ive/pi-ask/commits/main) [![stars](https://badgen.net/github/stars/eko24ive/pi-ask)](https://github.com/eko24ive/pi-ask/stargazers) `@eko24ive/pi-ask` is an ask tool that cares about your answers. @@ -115,7 +115,7 @@ Fixed bindings: Review-tab shortcuts can optionally require the same number key twice via `behaviour.doublePressReviewShortcuts`. `behaviour.presentSingleAsMulti` can render future single-select questions as multi-select while preserving the requested type in results; use `main.changeQuestionType` (`t` by default) to change the active question type live. -You can edit the config file yourself, ask pi to edit it for you, or use `/ask-settings` to find the exact config path, toggle behaviour/notification settings, or reset config to defaults with a guarded double press. +You can edit the config file yourself, ask pi to edit it for you, or use `/ask-settings` to find the exact config path, toggle behaviour/notification settings, or reset config to defaults with a guarded double press. pi-ask treats the config file as user-owned: load-time migrations and invalid files are handled in memory without rewriting or backing up the file, and read-only/externally managed configs fail gracefully with a manual-edit message. ```json { @@ -186,7 +186,7 @@ Accepted notation follows pi-tui key ids. Common aliases are normalized, for exa After installation, the extension registers the `ask_user` tool plus `/ask-settings`, `/answer`, `/answer:again`, and `/ask:replay` commands. -Agents can auto-discover and call `ask_user` when they need clarification instead of guessing. In interactive sessions, it opens a terminal UI flow for structured answers, supports native pi-style `@` file references while typing answers or notes, and returns normalized answers back to the agent. Ask settings are available both from `?` in the ask flow and from the `/ask-settings` command. Behaviour and notification settings are binary `on`/`off` toggles that save immediately, and the settings overlay includes a guarded double-press reset-to-defaults action; keymaps, notification channels, and extraction settings are changed by editing the shown config file path. +Agents can auto-discover and call `ask_user` when they need clarification instead of guessing. In interactive sessions, it opens a terminal UI flow for structured answers, supports native pi-style `@` file references while typing answers or notes, and returns normalized answers back to the agent. Ask settings are available both from `?` in the ask flow and from the `/ask-settings` command. Behaviour and notification settings are binary `on`/`off` toggles that save immediately when the config file is writable; save failures revert the toggle and show a manual-edit message. The settings overlay includes a guarded double-press reset-to-defaults action; keymaps, notification channels, and extraction settings are changed by editing the shown config file path. ### Answer and replay commands diff --git a/docs/architecture.md b/docs/architecture.md index 012ad14..fb562a0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -38,8 +38,7 @@ The codebase is split so the implementation reads through file boundaries and na - `src/config/schema.ts` — persisted config schema and runtime type - `src/config/migrate.ts` — persisted-file validation boundary, including schema migration and context-aware keymap normalization/fallback - `src/config/migrations/` — ordered schema-version migration framework for persisted config shape changes -- `src/config/path-migrations.ts` — file-location migration before config load -- `src/config/store.ts` — load/save/backup/runtime subscription store +- `src/config/store.ts` — non-destructive current/legacy config discovery, load, save, notice, and runtime subscription store ### UI @@ -75,16 +74,17 @@ The codebase is split so the implementation reads through file boundaries and na - deselected option notes stay in UI state - only selected option notes are emitted in the final result - editor lifecycle stays in the controller, not in the reducers -- persisted ask settings are migrated to the current schema version, validated, and normalized before use +- persisted ask settings are migrated to the current schema version in memory, validated, and normalized before use without rewriting the config file on load - config schema migrations preserve user-provided values and add new defaults only when fields are absent - replay payload lookup scans only the current session branch and revalidates payloads before use - invalid persisted keymaps fall back to default keymaps for the current session without discarding valid behaviour, notification, or answer settings - invalid notification channels are skipped and fall back to the default bell channel if none are valid -- ask settings behaviour and notification enabled changes persist immediately from the settings list; config reset is guarded by a short double-press confirmation +- ask settings behaviour and notification enabled changes attempt to persist immediately from the settings list; save failures revert the change and show an error; config reset is guarded by a short double-press confirmation - `presentSingleAsMulti` is applied at ask-flow creation; toggling it does not rewrite already-normalized questions in an open flow - `main.changeQuestionType` changes the active question type live (non-preview: `single <-> multi`; preview: `preview <-> multi`) and may require confirmation before destructive multi-to-single conversion -- when the ask config file is missing, the first ask use writes a default persisted config snapshot under `~/.pi/agent/extensions/` -- legacy root config files move into `~/.pi/agent/extensions/` only when the current config file is absent +- when the ask config file is missing, the first ask use attempts to write a default persisted config snapshot under `~/.pi/agent/extensions/`; if writing fails, built-in defaults are used for the session +- legacy root config files are read as a fallback only when the current config file is absent; disk is left untouched +- invalid config files are left untouched; defaults are loaded for the session with a notice - live config updates can affect an in-progress ask flow immediately ## Documentation rule diff --git a/docs/configuration.md b/docs/configuration.md index 7fccd72..29733be 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -8,21 +8,23 @@ When changing pi-ask settings: 2. Validate keymaps against the rules below. 3. Run `/reload` or restart pi so the new config is picked up. +pi-ask treats this file as user-owned config. It does not rewrite or back up the file just because it was loaded, migrated, or found invalid. Settings toggled in `/ask-settings` are saved only when the file is writable; if saving fails, pi-ask reverts the toggle and shows the config path to edit manually. + ## Config file path Default path: `~/.pi/agent/extensions/eko24ive-pi-ask.json` -If the file does not exist yet, pi-ask creates it with the current default settings the first time the ask flow is used. +If the file does not exist yet, pi-ask attempts to create it with the current default settings the first time the ask flow is used. If the config location is read-only or managed outside pi-ask, pi-ask uses built-in defaults for the session and leaves disk unchanged. -Older pi-ask versions wrote this file at `~/.pi/agent/eko24ive-pi-ask.json`. If that legacy file exists and the extensions config does not, pi-ask moves it into `~/.pi/agent/extensions/` on load. If both files exist, pi-ask uses the extensions config and leaves the legacy root file untouched. +Older pi-ask versions wrote this file at `~/.pi/agent/eko24ive-pi-ask.json`. If that legacy file exists and the extensions config does not, pi-ask reads the legacy file as a fallback and leaves disk unchanged. If both files exist, pi-ask uses the extensions config and leaves the legacy root file untouched. ## Config versions and migrations -`schemaVersion` identifies the persisted config shape. pi-ask migrates older supported schema versions forward before validation, then rewrites the file in the current shape. Migrations preserve existing user-provided values and add new fields from defaults when needed. +`schemaVersion` identifies the persisted config shape. pi-ask migrates older supported schema versions forward in memory before validation. Migrations preserve existing user-provided values and add new fields from defaults when needed, but loading a config does not rewrite the file. -Unsupported future versions or invalid files are backed up and defaults are loaded. +Unsupported future versions or invalid files are left unchanged and defaults are loaded for the current session. Fix the file, then run `/reload` or restart pi. ## Config shape diff --git a/docs/contract.md b/docs/contract.md index 764d11b..ec7d3f9 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -213,7 +213,7 @@ This document defines the stable external behavior. It does not explain internal - `/ask:replay` command to replay the latest real `ask_user` form on the current branch - ask settings list with binary behaviour/notification toggles and a guarded reset-to-defaults action - `?` in the ask flow and `/ask-settings` in pi open the same lightweight ask settings overlay -- settings persist immediately when changed: `Auto-submit when answered without notes`, `Confirm dismiss when dirty`, `Double-press review shortcuts`, `Notifications`, and `Show footer hints`; `Present single-select as multi-select` persists immediately but applies only to new/replayed ask flows; resetting config to defaults requires pressing the reset action twice within a short confirmation window +- settings attempt to persist immediately when changed: `Auto-submit when answered without notes`, `Confirm dismiss when dirty`, `Double-press review shortcuts`, `Notifications`, and `Show footer hints`; `Present single-select as multi-select` persists immediately when saving succeeds but applies only to new/replayed ask flows; save failures revert the setting and show a manual-edit message; resetting config to defaults requires pressing the reset action twice within a short confirmation window - `Keymaps` is a persisted, context-aware config section for global, main-flow, editor, note-editor, and settings-modal actions - the settings list shows the absolute config file path for customizing keymaps, notifications, and extraction settings - if the flow is already on the review tab, all questions are answered, and no notes exist, enabling auto-submit can complete the current ask flow immediately @@ -248,7 +248,7 @@ Settings modal: - `settingsModal.close` closes settings; defaults: `Esc`, `Ctrl+C`, `?` - `settingsModal.nextOption` / `settingsModal.previousOption` move between settings; defaults: `Down`, `Up` -- `settingsModal.toggle` toggles the highlighted setting and saves immediately; on the reset action, the same binding must be pressed twice within a short confirmation window; defaults: `Enter`, `Space` +- `settingsModal.toggle` toggles the highlighted setting and attempts to save immediately; if saving fails, the setting reverts and an error is shown; on the reset action, the same binding must be pressed twice within a short confirmation window; defaults: `Enter`, `Space` Dirty dismiss: @@ -261,7 +261,7 @@ If `ctx.hasUI === false`, the tool returns a `Needs user input` message in `cont Validation is handled inside the tool so malformed calls produce the same structured error shape as other invalid payloads instead of relying on pre-execution schema failures. -The ask flow subscribes to runtime settings updates while open. In practice, this means changing `Auto-submit when answered without notes`, `Confirm dismiss when dirty`, `Double-press review shortcuts`, `Notifications`, `Show footer hints`, resetting config to defaults, or reloading config-backed keymaps can affect the in-progress ask flow immediately instead of only future asks. `Present single-select as multi-select` is applied when an ask flow is created and does not rewrite question semantics for an already-open flow; use `main.changeQuestionType` for live per-question changes. +The ask flow subscribes to runtime settings updates while open. In practice, this means changing `Auto-submit when answered without notes`, `Confirm dismiss when dirty`, `Double-press review shortcuts`, `Notifications`, `Show footer hints`, resetting config to defaults, or reloading config-backed keymaps can affect the in-progress ask flow immediately instead of only future asks when the change is saved or otherwise applied in memory. Load-time migrations and invalid config handling do not rewrite, rename, or back up the config file; invalid files load defaults for the session and show a notice. `Present single-select as multi-select` is applied when an ask flow is created and does not rewrite question semantics for an already-open flow; use `main.changeQuestionType` for live per-question changes. ## Notifications diff --git a/src/config/path-migrations.ts b/src/config/path-migrations.ts deleted file mode 100644 index 798305f..0000000 --- a/src/config/path-migrations.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { mkdir, readFile, rename } from "node:fs/promises"; -import { dirname } from "node:path"; - -export interface AskConfigPathMigrationOptions { - currentPath: string; - legacyPaths: string[]; -} - -export async function migrateAskConfigPathIfNeeded({ - currentPath, - legacyPaths, -}: AskConfigPathMigrationOptions): Promise { - if (await pathExists(currentPath)) { - return; - } - - const legacyPath = await findFirstExistingPath(legacyPaths); - if (!legacyPath) { - return; - } - - await mkdir(dirname(currentPath), { recursive: true }); - await rename(legacyPath, currentPath); -} - -async function findFirstExistingPath( - paths: string[] -): Promise { - for (const path of paths) { - if (await pathExists(path)) { - return path; - } - } - return; -} - -async function pathExists(path: string): Promise { - try { - await readFile(path, "utf-8"); - return true; - } catch (error) { - if (isMissingFileError(error)) { - return false; - } - throw error; - } -} - -function isMissingFileError(error: unknown): boolean { - return ( - !!error && - typeof error === "object" && - "code" in error && - error.code === "ENOENT" - ); -} diff --git a/src/config/store.ts b/src/config/store.ts index 0390aab..e39c6b8 100644 --- a/src/config/store.ts +++ b/src/config/store.ts @@ -1,8 +1,6 @@ -import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; +import { mkdir, readFile, writeFile } from "node:fs/promises"; import { dirname, join } from "node:path"; -const JSON_EXTENSION_PATTERN = /\.json$/u; - import { getAgentDir } from "@earendil-works/pi-coding-agent"; import { DEFAULT_ASK_CONFIG, @@ -10,9 +8,13 @@ import { toAskConfigFileV5, } from "./defaults.ts"; import { AskConfigMigrationError, migrateAskConfig } from "./migrate.ts"; -import { migrateAskConfigPathIfNeeded } from "./path-migrations.ts"; import type { AskConfig } from "./schema.ts"; +const INVALID_CONFIG_NOTICE = + "Config was invalid or unsupported. Loaded defaults for this session and left the config file unchanged. Edit the config file or run /reload after fixing it."; +const MIGRATION_FAILED_NOTICE = + "Config migration failed. Loaded defaults for this session and left the config file unchanged. Edit the config file or run /reload after fixing it."; + export interface AskConfigNotice { kind: "error" | "warning" | "success"; text: string; @@ -70,8 +72,12 @@ export class AskConfigStore { null, 2 ).concat("\n"); - await mkdir(dirname(this.configPath), { recursive: true }); - await writeFile(this.configPath, content, "utf-8"); + try { + await mkdir(dirname(this.configPath), { recursive: true }); + await writeFile(this.configPath, content, "utf-8"); + } catch (error) { + throw createConfigSaveError(this.configPath, error); + } this.setConfig(normalized); return normalized; } @@ -85,33 +91,47 @@ export class AskConfigStore { } private async loadFromDisk(): Promise { - await this.migrateLegacyConfigIfNeeded(); - let content: string; - try { - content = await readFile(this.configPath, "utf-8"); - } catch (error) { - if (isMissingFileError(error)) { - const config = normalizeAskConfig(DEFAULT_ASK_CONFIG); - await this.save(config); - return { config }; + const content = await this.readDiskConfig(); + if (content === undefined) { + return this.loadMissingConfig(); + } + + const parsed = parseJson(content); + if (!parsed.ok) { + return this.loadDefaultsWithNotice(INVALID_CONFIG_NOTICE); + } + + return this.loadParsedConfig(parsed.value); + } + + private async readDiskConfig(): Promise { + for (const path of [this.configPath, ...this.legacyConfigPaths]) { + const content = await readConfigFileIfPresent(path); + if (content !== undefined) { + return content; } - throw error; } + } - let parsed: unknown; + private async loadMissingConfig(): Promise { + const config = normalizeAskConfig(DEFAULT_ASK_CONFIG); try { - parsed = JSON.parse(content); - } catch { - return this.backupAndReset( - "Config was invalid or unsupported. Backed it up and loaded defaults. Change any behaviour setting or edit the config file to save a fresh config." - ); + await this.save(config); + return { config }; + } catch (saveError) { + return { + config, + notice: { + kind: "warning", + text: getErrorMessage(saveError), + }, + }; } + } + private loadParsedConfig(parsed: unknown): AskConfigLoadResult { try { const migrated = migrateAskConfig(parsed); - if (migrated.migrated) { - await this.save(migrated.config); - } return { config: migrated.config, notice: migrated.notice @@ -123,20 +143,17 @@ export class AskConfigStore { }; } catch (error) { if (error instanceof AskConfigMigrationError) { - return this.backupAndReset( + return this.loadDefaultsWithNotice( error.reason === "migration_failed" - ? "Config migration failed. Backed up old config and loaded defaults. Change any behaviour setting or edit the config file to save a fresh config." - : "Config was invalid or unsupported. Backed it up and loaded defaults. Change any behaviour setting or edit the config file to save a fresh config." + ? MIGRATION_FAILED_NOTICE + : INVALID_CONFIG_NOTICE ); } throw error; } } - private async backupAndReset(text: string): Promise { - const backupPath = createBackupPath(this.configPath, new Date()); - await mkdir(dirname(this.configPath), { recursive: true }); - await rename(this.configPath, backupPath); + private loadDefaultsWithNotice(text: string): AskConfigLoadResult { return { config: normalizeAskConfig(DEFAULT_ASK_CONFIG), notice: { @@ -145,13 +162,6 @@ export class AskConfigStore { }, }; } - - private async migrateLegacyConfigIfNeeded(): Promise { - await migrateAskConfigPathIfNeeded({ - currentPath: this.configPath, - legacyPaths: this.legacyConfigPaths, - }); - } } let askConfigStore: AskConfigStore | undefined; @@ -169,9 +179,38 @@ export function getLegacyAskConfigPaths(): string[] { return [join(getAgentDir(), "eko24ive-pi-ask.json")]; } -function createBackupPath(path: string, date: Date): string { - const timestamp = date.toISOString().replaceAll(":", "-").replace(/\./g, "-"); - return path.replace(JSON_EXTENSION_PATTERN, `.${timestamp}.bak.json`); +function createConfigSaveError(path: string, error: unknown): Error { + const detail = getErrorMessage(error); + return new Error( + `Unable to save ask config at ${path}. The file may be read-only or managed outside pi-ask; edit it manually and run /reload. ${detail}` + ); +} + +function parseJson( + content: string +): { ok: true; value: unknown } | { ok: false } { + try { + return { ok: true, value: JSON.parse(content) }; + } catch { + return { ok: false }; + } +} + +async function readConfigFileIfPresent( + path: string +): Promise { + try { + return await readFile(path, "utf-8"); + } catch (error) { + if (isMissingFileError(error)) { + return; + } + throw error; + } +} + +function getErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); } function isMissingFileError(error: unknown): boolean { diff --git a/src/ui/controller.ts b/src/ui/controller.ts index d551328..f9f11ad 100644 --- a/src/ui/controller.ts +++ b/src/ui/controller.ts @@ -65,12 +65,14 @@ interface AskFlowOptions { type AskFlowParams = AskParams & Pick & { config: AskConfig; + configNotice?: string; ctx: ExtensionContext; flowOptions: AskFlowOptions; }; interface AskFlowController { config: AskConfig; + configNotice?: string; ctx: ExtensionContext; dismissNotice?: string; done: Done; @@ -91,7 +93,7 @@ export async function runAskFlow( options: AskFlowOptions = {} ): Promise { const store = getAskConfigStore(); - const config = await store.getConfig(); + const { config, notice } = await store.ensureLoaded(); const flowOptions = { ...options, presentSingleAsMulti: @@ -101,6 +103,7 @@ export async function runAskFlow( createAskFlowController(args, { ...params, config, + configNotice: notice?.text, cwd: ctx.cwd, ctx, flowOptions, @@ -119,6 +122,7 @@ function createAskFlowController( ) { const controller: AskFlowController = { config: params.config, + configNotice: params.configNotice, ctx: params.ctx, dismissNotice: undefined, done, @@ -137,6 +141,7 @@ function createAskFlowController( controller.unsubscribeConfig = getAskConfigStore().subscribe((config) => { controller.config = config; + controller.configNotice = undefined; controller.state = maybeAutoSubmitState( controller.state, controller.config @@ -172,7 +177,7 @@ function renderController( return renderAskScreen({ config: controller.config, editor: controller.editor, - footerNotice: getDismissNotice(controller), + footerNotice: getFooterNotice(controller), reviewShortcutHint: getActiveReviewShortcutHint(controller), state: controller.state, theme: controller.theme, @@ -378,7 +383,7 @@ function commitState( options: { finish?: boolean; syncSelection?: boolean } = {} ) { if (nextState.activeTabIndex !== controller.state.activeTabIndex) { - clearDismissNotice(controller); + clearFooterNotices(controller); } controller.suppressAutoInputForSelection = false; controller.state = nextState; @@ -397,7 +402,7 @@ function submitEditor(controller: AskFlowController, value: string) { controller.suppressAutoInputForSelection = false; const nextState = submitEditorDraft(controller.state, value); if (nextState.activeTabIndex !== controller.state.activeTabIndex) { - clearDismissNotice(controller); + clearFooterNotices(controller); } controller.state = nextState; syncSelection(controller); @@ -447,7 +452,8 @@ function shouldRequestDismissConfirmation( }); } -function clearDismissNotice(controller: AskFlowController) { +function clearFooterNotices(controller: AskFlowController) { + controller.configNotice = undefined; controller.dismissNotice = undefined; } @@ -505,8 +511,8 @@ function handleReviewShortcutNumber( return true; } -function getDismissNotice(controller: AskFlowController): string | undefined { - return controller.dismissNotice; +function getFooterNotice(controller: AskFlowController): string | undefined { + return controller.dismissNotice ?? controller.configNotice; } function showSettingsModal(controller: AskFlowController) { diff --git a/src/ui/settings-list.ts b/src/ui/settings-list.ts index 0087716..f35766a 100644 --- a/src/ui/settings-list.ts +++ b/src/ui/settings-list.ts @@ -121,8 +121,8 @@ export class AskSettingsList { private error?: string; private focusIndex = 0; private resetConfirmUntil = 0; + private notice?: AskConfigNotice; private readonly configPath: string; - private readonly notice?: AskConfigNotice; private readonly onClose: () => void; private readonly onSave: (config: AskConfig) => Promise; private readonly theme: Theme; @@ -264,11 +264,14 @@ export class AskSettingsList { } private appendNotice(lines: string[], innerWidth: number): void { - if (this.notice?.kind === "error") { + if (this.notice) { lines.push(this.line("", innerWidth)); this.appendWrapped( lines, - this.theme.fg("warning", this.notice.text), + this.theme.fg( + this.notice.kind === "success" ? "accent" : "warning", + this.notice.text + ), innerWidth ); } @@ -407,6 +410,7 @@ export class AskSettingsList { this.onSave(nextConfig) .then((savedConfig) => { this.config = savedConfig; + this.notice = undefined; this.tui.requestRender(); }) .catch((error: unknown) => { diff --git a/tests/config-store.test.ts b/tests/config-store.test.ts index 4ec2dce..08cb2a4 100644 --- a/tests/config-store.test.ts +++ b/tests/config-store.test.ts @@ -1,13 +1,16 @@ import assert from "node:assert/strict"; -import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, readdir, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import test from "node:test"; import { DEFAULT_ASK_CONFIG } from "../src/config/defaults.ts"; +import type { AskConfig } from "../src/config/schema.ts"; import { AskConfigStore } from "../src/config/store.ts"; const DEFAULT_KEYMAPS_NOTICE_PATTERN = /Using default ask keymaps for this session/; +const SAVE_FAILURE_PATTERN = + /Unable to save ask config .* managed outside pi-ask/; function expectedConfigFile( overrides: { behaviour?: typeof DEFAULT_ASK_CONFIG.behaviour } = {} @@ -42,6 +45,28 @@ test("config store writes defaults when file is missing", async () => { await rm(dirname(path), { force: true, recursive: true }); }); +test("config store uses defaults when initial config creation fails", async () => { + class FailingInitialSaveStore extends AskConfigStore { + override save(_config: AskConfig | Partial): Promise { + return Promise.reject( + new Error("Unable to save ask config; edit it manually.") + ); + } + } + const path = await makeTempPath("pi-ask-config-initial-save-failure-"); + const store = new FailingInitialSaveStore(path); + + const result = await store.ensureLoaded(); + + assert.deepEqual(result.config, DEFAULT_ASK_CONFIG); + assert.equal(result.notice?.kind, "warning"); + assert.equal( + result.notice?.text, + "Unable to save ask config; edit it manually." + ); + await rm(dirname(path), { force: true, recursive: true }); +}); + test("config store writes full normalized config on save", async () => { const path = await makeTempPath("pi-ask-config-save-"); const store = new AskConfigStore(path); @@ -69,43 +94,51 @@ test("config store writes full normalized config on save", async () => { await rm(dirname(path), { force: true, recursive: true }); }); -test("config store backs up broken json and loads defaults", async () => { +test("config store reports friendly save failures", async () => { + const path = await makeTempPath("pi-ask-config-save-failure-"); + await mkdir(path, { recursive: true }); + const store = new AskConfigStore(path); + + await assert.rejects(store.save(DEFAULT_ASK_CONFIG), SAVE_FAILURE_PATTERN); + await rm(path, { force: true, recursive: true }); +}); + +test("config store leaves broken json unchanged and loads defaults", async () => { const path = await makeTempPath("pi-ask-config-broken-"); await mkdir(dirname(path), { recursive: true }); await writeFile(path, "{bad json", "utf-8"); const store = new AskConfigStore(path); const result = await store.ensureLoaded(); - const dirEntries = await import("node:fs/promises").then(({ readdir }) => - readdir(dirname(path)) - ); + const dirEntries = await readdir(dirname(path)); assert.deepEqual(result.config, DEFAULT_ASK_CONFIG); assert.equal( result.notice?.text, - "Config was invalid or unsupported. Backed it up and loaded defaults. Change any behaviour setting or edit the config file to save a fresh config." + "Config was invalid or unsupported. Loaded defaults for this session and left the config file unchanged. Edit the config file or run /reload after fixing it." ); - assert(dirEntries.some((entry) => entry.includes(".bak.json"))); - await assert.rejects(readFile(path, "utf-8")); + assert.equal( + dirEntries.some((entry) => entry.includes(".bak.json")), + false + ); + assert.equal(await readFile(path, "utf-8"), "{bad json"); await rm(dirname(path), { force: true, recursive: true }); }); -test("config store loads current config version without rewriting", async () => { +test("config store loads migrated config without rewriting", async () => { const path = await makeTempPath("pi-ask-config-current-"); await mkdir(dirname(path), { recursive: true }); - await writeFile( - path, - JSON.stringify({ - schemaVersion: 1, - behaviour: { - autoSubmitWhenAnsweredWithoutNotes: true, - confirmDismissWhenDirty: true, - doublePressReviewShortcuts: true, - showFooterHints: false, - }, - keymaps: DEFAULT_ASK_CONFIG.keymaps, - }) - ); + const content = JSON.stringify({ + schemaVersion: 1, + behaviour: { + autoSubmitWhenAnsweredWithoutNotes: true, + confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, + showFooterHints: false, + }, + keymaps: DEFAULT_ASK_CONFIG.keymaps, + }); + await writeFile(path, content); const store = new AskConfigStore(path); const result = await store.ensureLoaded(); @@ -118,10 +151,11 @@ test("config store loads current config version without rewriting", async () => assert.equal(result.config.behaviour.doublePressReviewShortcuts, true); assert.equal(result.config.behaviour.showFooterHints, false); assert.deepEqual(result.config.keymaps, DEFAULT_ASK_CONFIG.keymaps); + assert.equal(await readFile(path, "utf-8"), content); await rm(dirname(path), { force: true, recursive: true }); }); -test("config store migrates the legacy root config path into extensions", async () => { +test("config store reads legacy root config without copying it", async () => { const root = await import("node:fs/promises").then(({ mkdtemp }) => mkdtemp(join(tmpdir(), "pi-ask-config-legacy-")) ); @@ -151,17 +185,8 @@ test("config store migrates the legacy root config path into extensions", async assert.equal(result.config.behaviour.confirmDismissWhenDirty, true); assert.equal(result.config.behaviour.doublePressReviewShortcuts, true); assert.equal(result.config.behaviour.showFooterHints, false); - await assert.rejects(readFile(legacyPath, "utf-8")); - assert.deepEqual( - JSON.parse(await readFile(path, "utf-8")), - expectedConfigFile({ - behaviour: { - ...DEFAULT_ASK_CONFIG.behaviour, - autoSubmitWhenAnsweredWithoutNotes: true, - showFooterHints: false, - }, - }) - ); + assert.ok(await readFile(legacyPath, "utf-8")); + await assert.rejects(readFile(path, "utf-8")); await rm(root, { force: true, recursive: true }); }); diff --git a/tests/settings-list.test.ts b/tests/settings-list.test.ts index c193ca0..5333db0 100644 --- a/tests/settings-list.test.ts +++ b/tests/settings-list.test.ts @@ -3,6 +3,7 @@ import test from "node:test"; import { visibleWidth } from "@earendil-works/pi-tui"; import { DEFAULT_ASK_CONFIG } from "../src/config/defaults.ts"; import type { AskConfig } from "../src/config/schema.ts"; +import type { AskConfigNotice } from "../src/config/store.ts"; import { AskSettingsList } from "../src/ui/settings-list.ts"; const savedConfig: AskConfig = { @@ -36,6 +37,7 @@ function plainTheme() { function createList( options: { + notice?: AskConfigNotice; onClose?: () => void; onSave?: (config: AskConfig) => Promise; savedConfig?: AskConfig; @@ -48,7 +50,7 @@ function createList( }); return new AskSettingsList(plainTheme(), { configPath: "/tmp/eko24ive-pi-ask.json", - notice: undefined, + notice: options.notice, onClose, onSave: options.onSave ?? ((config) => Promise.resolve(config)), savedConfig: options.savedConfig ?? savedConfig, @@ -130,6 +132,38 @@ test("settings list shows save failures and reverts the toggle", async () => { assert(text.includes("[off]")); }); +test("settings list renders load warnings", () => { + const list = createList({ + notice: { + kind: "warning", + text: "Unable to save ask config; edit it manually.", + }, + }); + + const text = list.render(72).join("\n"); + + assert(text.includes("Unable to save ask config; edit it manually.")); +}); + +test("settings list clears load warnings after successful save", async () => { + const list = createList({ + notice: { + kind: "warning", + text: "Unable to save ask config; edit it manually.", + }, + }); + + list.handleInput(" "); + await new Promise((resolve) => setImmediate(resolve)); + + const text = list.render(72).join("\n"); + + assert.equal( + text.includes("Unable to save ask config; edit it manually."), + false + ); +}); + test("settings list uses configured navigation and close keys", async () => { let saved: AskConfig | undefined; const customConfig: AskConfig = {