From abac3a565b58feb3109e53d9a91d9ab70879c694 Mon Sep 17 00:00:00 2001 From: serkanalgur Date: Wed, 30 Sep 2026 12:34:44 +0400 Subject: [PATCH] feat: add a memory config block, and make the config dialog searchable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config dialog discovers its rows by walking the config, so any block that exists is already editable. But persistent memory was not reachable at all: it had no block (the one on the wider embedder type was removed because nothing read it), its db path and entry cap were hardcoded, and the store was frozen at construction. A ninth block fixes that. memory: { enabled, storage, maxEntries } enabled is a per-call gate on recallForTask, so turning recall off takes effect on the next task and nothing else. It defaults to true because recall is already unconditional today — false would silently delete a running feature on upgrade. storage and maxEntries resolve per use rather than at construction. Editing storage in nexus.jsonc and saving now moves the database without a restart; the old file is left intact, and a path that cannot be opened throws with the previous database still current rather than leaving the store with no handle. The swap is ordered open-then-close and contains no await, so concurrent recalls cannot observe a partial state. The cap is a pure read and needs no reopen. Three fixes fell out of the same work: A relative memory.storage opened the grouped model picker, and picking a model wrote a provider/id into the database path. A field is now a model reference because of what it is, never because a string contains a slash — which also fixes customRoles[].prompt, whose text nearly always names a file, and customRoles[].displayName. memoryBase was seeded by spreading an object literal whose keys were present but undefined, so storage and maxEntries were dropped from getConfig() and from every file getSaveableConfig() wrote. 1373 green tests missed it because they all built a bare NexusConfigManager; the orchestrator-configured path is now covered. The Save row said "Write 3 changes to disk" while the save wrote nine blocks. The number described what the user touched, not what was written, so the wording no longer claims to be a count of the write. Config dialog: Search on the hub, matching block names and setting names, landing straight on the field's editor. A query that matches nothing is a selectable row, not a zero-option dialog, so the screen is escapable and a typo is visible in the quoted query. Dirty counts per block, and a hub header naming the number of changes not yet written. Every marker is a glyph plus a number, so none of them depends on colour. A boolean toggles on its own screen in one keystroke, with a visible way out beneath it, instead of opening a second dialog to pick from two rows. One mutating row is never the only row on a screen. 1386 tests pass. --- docs/API.md | 37 +- package.json | 2 +- src/config-flow.ts | 563 +++++++++++++++++++++++++++---- src/config.ts | 133 +++++++- src/memory-store.ts | 228 +++++++++++-- src/orchestrator.ts | 56 ++- src/tui.tsx | 74 ++-- test/config-knobs.test.ts | 122 ++++++- test/issue-regressions.test.ts | 2 +- test/memory-injection.test.ts | 41 +++ test/memory-store.test.ts | 196 +++++++++++ test/tui-config-flow.test.ts | 600 +++++++++++++++++++++++++++++---- 12 files changed, 1859 insertions(+), 195 deletions(-) diff --git a/docs/API.md b/docs/API.md index 6bf9a1f..5bc18e0 100644 --- a/docs/API.md +++ b/docs/API.md @@ -129,10 +129,32 @@ editing any level takes effect without a restart. "notifications": { "enabled": true }, "gitFlow": { "enabled": true, "conventionalCommits": true, "requireBranch": true, "prBeforeMerge": true }, "effort": { "enabled": false, "maxEffort": "high", "minDifficulty": 0 }, - "customRoles": [] + "customRoles": [], + // `storage` is an ABSOLUTE path, resolved as join($HOME, ".local/share/opencode-nexus/memory.db"). + // There is no tilde expansion, so "~" written here is a literal directory name. + "memory": { "enabled": true, "storage": "/Users/you/.local/share/opencode-nexus/memory.db", "maxEntries": 1000 } } ``` +`memory.enabled` is honoured: it is the gate at the top of `recallForTask`, so +`false` stops both automatic injections — the `executeTask` spawn and the +`spawnAgent` path — and the gate lives in the method rather than at either call +site so a third injection point cannot miss it. `memory.storage` is the sqlite +file the store opens, and `memory.maxEntries` is a **per-scope** cap: the +`project` scope is exempt from it by design, so lowering this never evicts a +durable note. + +**Both take effect without a restart**, which is the whole point of them being +in the schema at all. The store resolves its path and its cap on every use +rather than holding a copy taken at construction, so saving the file is enough: +`storage` closes the open database and opens the new one on the next store call, +and `maxEntries` applies to the very next write — the store does not need to be +rebuilt, and rebuilding it is not something a caller has to remember to do. The +old database file is left on disk untouched when `storage` changes, so +repointing the setting is reversible. `getConfig().memory.storage` and +`orchestrator.memoryStore.path` are the same string because the store reads the +config rather than a snapshot of it; they cannot disagree. + `dashboard.enabled` is **honoured**: `NexusOrchestrator.startDashboard()` refuses to start and names the key when it is `false`, and the TUI's `/nexus dashboard` command reports the same reason instead of probing a port that is switched off. @@ -140,8 +162,8 @@ The other `dashboard` fields are the defaults for `startDashboard()`; explicit arguments to that method still win. `saveProjectConfig` and `saveGlobalConfig` write `models`, `budget`, -`selfHealing`, `dashboard`, `notifications`, `gitFlow`, `effort` and -`customRoles` — all eight, unconditionally. They overwrite the whole file, so a +`selfHealing`, `dashboard`, `notifications`, `gitFlow`, `effort`, `customRoles` +and `memory` — all nine, unconditionally. They overwrite the whole file, so a block left out of that list would be a block *deleted* from the user's config on the first save. @@ -149,10 +171,11 @@ Blocks on the `NexusConfig` type that are **not** in this schema — `agents`, `learning`, `cost` — are settable through the `NexusOrchestrator` constructor only, and are documented that way rather than as user-configurable, because they are not. (`cost` is optional, and `mergeConfig` fills in its default, so an -omitted block behaves exactly as a configured one.) `memory`, `security` and +omitted block behaves exactly as a configured one.) `security` and `communication` are not listed because no such blocks exist on the type at all — they were deleted rather than left unread, and `saveProjectConfig` cannot drop -what is not there. +what is not there. (`memory` was one of those deleted blocks; it is in the +schema above now, because `recallForTask` reads its `enabled` flag.) #### getConfig(): NexusFullConfig @@ -178,7 +201,7 @@ saveProjectConfig(basePath: string): void Persist the current configuration to `/.opencode/nexus.jsonc`. Writes `models`, `budget`, `selfHealing`, `dashboard`, `notifications`, `gitFlow`, -`effort` and `customRoles` — see the schema above. +`effort`, `customRoles` and `memory` — see the schema above. #### saveGlobalConfig(): void @@ -186,7 +209,7 @@ Persist the current configuration to `/.opencode/nexus.jsonc`. Writes saveGlobalConfig(): void ``` -Persist the current configuration to `~/.config/opencode/nexus.jsonc`. Same eight +Persist the current configuration to `~/.config/opencode/nexus.jsonc`. Same nine blocks. #### applyPreset(name: string): void diff --git a/package.json b/package.json index 289ba15..9f79eed 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@serkanalgur/opencode-nexus", - "version": "2.13.2", + "version": "2.14.0", "description": "Adaptive Multi-Agent Orchestration with Cost Intelligence for OpenCode V2", "keywords": [ "opencode", diff --git a/src/config-flow.ts b/src/config-flow.ts index f3c8f2a..cffb0c4 100644 --- a/src/config-flow.ts +++ b/src/config-flow.ts @@ -30,9 +30,11 @@ * close. The hub: nothing is written until `save` is chosen. * 2. `fields` — one row per field of the block you entered, each labelled * with its CURRENT value, plus a row back to the hub. - * 3. `editor` — the widget for one field: a two-row `select` for a boolean, a - * `prompt` for a number or a string, the grouped provider - * picker for a model reference. + * 3. `editor` — the widget for one field: a one-keystroke `select` for a + * boolean, a `prompt` for a number or a string, the grouped + * provider picker for a model reference. + * 4. `search` — reached from the hub's own second row, so it is one escape + * from the hub rather than a fourth level to climb out of. * * ESCAPE IS "GO UP ONE LEVEL", EVERYWHERE, AND IT NEVER DISCARDS SILENTLY — the * draft lives in memory and nothing has been written, so escaping from an editor @@ -80,15 +82,39 @@ export type ConfigScope = 'project' | 'global' export type ConfigPathSegment = string | number /** - * The widget a field gets, chosen from the VALUE's type and never from its name. + * The widget a field gets. Chosen from the VALUE's type, with exactly two + * exceptions, both listed below — so the rule is not "never from its name" but + * "from the value, unless the value cannot tell you." * - * `model` is the one kind that is not purely structural: a string containing a - * `/` opens the grouped provider picker instead of a free-text field. That is - * the same rule `getModelForRole` uses when it warns about a missing provider - * prefix, so the two agree on what a model reference looks like, and a value - * without a slash degrades to a text field rather than being rejected. + * `model` is the one kind that is not purely structural: it opens the grouped + * provider picker instead of a free-text field, so a pick there WRITES + * `provider/id` and any misclassification is silent and total. + * + * ── WHY IT IS DECIDED BY NAME, NOT BY SHAPE ── + * + * By the field's own address, never by the value (see `MODEL_SNIFF_FIELDS`). A + * value-shaped test asks "does this string contain a `/`?", and a `/` is not a + * property of model references — it is a property of STRINGS. `memory.storage` + * is a sqlite file path, so a user who set a relative one + * (`./.opencode/memory.db`) got the provider picker, and picking a model there + * wrote a `provider/id` into the memory DB path: silent, total, and it + * destroyed the store it was editing. Narrowing the shape test to two BLOCKS + * did not retire the class, only the instance — `customRoles` is full of prose, + * so a role whose `prompt` reads "Read src/index.ts and report", or whose + * `displayName` is `QA/Engineer`, was still classified `model` and a pick + * overwrote the prompt. The address is the only thing that answers the + * question, and a role's model reference is `customRoles[].model` BY NAME — + * `getModelForRole` reads that field, which is the entire warrant for it. + * + * `path` is decided the same way, from the BLOCK plus the LABEL — + * `memory` + `storage` — because there is no value-based rule that could tell a + * file path from a model reference: they are both strings, and the slash is the + * only thing distinguishing them, which is the whole problem. The block knows + * what it holds; the string does not. It edits as free text today, and exists so + * that adding a path-validating editor later is a `case` in one switch rather + * than a re-classification of every field that happens to contain a slash. */ -export type ConfigFieldKind = 'boolean' | 'number' | 'text' | 'model' | 'opaque' +export type ConfigFieldKind = 'boolean' | 'number' | 'text' | 'model' | 'path' | 'opaque' /** One addressable value in the config. */ export interface ConfigField { @@ -139,9 +165,14 @@ export type ConfigDraft = Record * for one field. An `editor` is a screen rather than an instant because the * question "can the user get out of here" has to be answerable, and it cannot * be answered for something that is not a state. + * + * `search` is a screen for the same reason, and it is only ONE level above the + * hub so that the "at most two escapes to the hub" guarantee survives it: a + * search that took three to leave would be a feature that can strand someone. */ export type ConfigScreen = | { readonly kind: 'blocks' } + | { readonly kind: 'search'; readonly query: string } | { readonly kind: 'fields'; readonly block: string } | { readonly kind: 'editor'; readonly field: ConfigField } @@ -155,11 +186,71 @@ function isPlainObject(value: unknown): value is Record { } /** - * A model reference, by the same rule `getModelForRole` applies: a `provider/id` - * pair. See `ConfigFieldKind` for why the flow uses it to pick a widget. + * A field whose kind comes from what the field IS, never from what its value + * looks like. + * + * ── THE INVARIANT ── + * + * A field is a model reference because of its NAME, and never because some + * string in it contains a slash. A slash is what a `provider/id` pair and a + * filesystem path and a role's `displayName` (`QA/Engineer`) and a prompt that + * says "read src/index.ts" all have in common, so a value-shaped test cannot + * tell them apart — and getting it wrong is SILENT AND TOTAL, because + * `runEditor`'s `case 'model'` opens the provider picker and + * `commitConfigModel` then writes `provider/id` over the user's text. + * + * That class is retired by asking about the field, not the value, which is the + * same treatment `PATH_FIELDS` already gives. Concretely, the sniffing that + * `customRoles` used to get is gone: a role's `model` is listed by name below, + * and its `prompt` and `displayName` are `text` however many slashes they hold. + */ + +/** + * The fields that hold a model reference, as `block.field` addresses. + * + * `models.*` is a WILDCARD, not a name: `models` is an open keyed block, so + * there is no closed key set to enumerate — every leaf under it is a + * `provider/id` pair by definition of the block, and the `.*` says so without + * pretending to know the keys a user invents. + * + * `customRoles[].model` is by name, and that is the whole warrant for keeping + * any of this: `getModelForRole` reads the field (asserted at + * `test/custom-roles-config.test.ts:204`), so a text field there is a working + * setting demoted to second class by nothing at all. + * + * The `[]` matches one array element, so the entry covers `customRoles[0].model` + * and `customRoles[7].model` alike without naming either. + */ +const MODEL_SNIFF_FIELDS: ReadonlySet = new Set(['models.*', 'customRoles[].model']) + +/** + * Whether the field at `path` is a model reference. + * + * Path-based on purpose: the array element index and the invented key under an + * open block are both positions rather than names, and matching on the block + * and the label separately is how the pre-existing bug got in — `label` alone + * cannot tell `customRoles[0].model` from `models.model`. + */ +function isModelField(path: readonly ConfigPathSegment[]): boolean { + const address = configPathLabel(path).replace(/\[\d+\]/g, '[]') + if (MODEL_SNIFF_FIELDS.has(address)) return true + return path.length === 2 && path[0] === 'models' && typeof path[1] === 'string' +} + +/** + * Fields whose kind comes from what the field HOLDS rather than from the value. + * + * `memory.storage` is a filesystem path and nothing else, and it is the only one + * today. Kept as a table rather than an `if` so a second path-shaped field + * (`export.path`, a log file, a docs url) is an entry here instead of a + * condition buried in `walkConfig`. */ -function looksLikeModelRef(value: string): boolean { - return value.includes('/') && value.indexOf('/') > 0 && value.length > 1 +const PATH_FIELDS: ReadonlySet = new Set(['memory.storage']) + +function fieldKindFor(block: string, path: readonly ConfigPathSegment[], label: string): ConfigFieldKind { + if (PATH_FIELDS.has(`${block}.${label}`)) return 'path' + if (isModelField(path)) return 'model' + return 'text' } /** @@ -196,8 +287,12 @@ function walkConfig( return } if (typeof value === 'string') { - const kind: ConfigFieldKind = looksLikeModelRef(value) ? 'model' : 'text' - fields.push({ ...base, kind }) + // The PATH, not the value. A string is a string: the only thing a file path + // and a `provider/id` and a role prompt naming a source file have in common + // is a slash, so deciding from the value means every path in the config + // claims to be a model — and a role's instructions claim it too. See + // `ConfigFieldKind` and `MODEL_SNIFF_FIELDS`. + fields.push({ ...base, kind: fieldKindFor(String(base.block), path, label) }) return } if (Array.isArray(value)) { @@ -357,6 +452,72 @@ function configSameValue(a: unknown, b: unknown): boolean { return false } +// ── How much of the draft has moved ───────────────────────────────── + +/** + * How many of `block`'s LEAVES differ between `base` and `draft`. + * + * LEAVES, not changed PATHS, and the difference is not pedantry: a count of + * paths would report `1` for a write to `budget` whether the user moved one key + * or six, and the badge exists precisely to answer "how much have I touched in + * here". A badge that cannot distinguish a single edit from a sweep is a badge + * that gets ignored after the first time it is wrong. + * + * The comparison is the same structural one `configIsDirty` uses, and it is + * reached through the same private function rather than a second equality: two + * definitions of "equal" in one file is how a dirty flag starts flickering. + */ +export function configBlockDirtyCount( + block: ConfigBlock, + base: ConfigDraft, + draft: ConfigDraft +): number { + return block.fields.filter( + field => !configSameValue(configValueAt(base, field.path), configValueAt(draft, field.path)) + ).length +} + +/** Every changed leaf in the config, counted across all of its blocks. */ +export function configDirtyCount( + discovery: ConfigDiscovery, + base: ConfigDraft, + draft: ConfigDraft +): number { + return discovery.blocks.reduce( + (total, block) => total + configBlockDirtyCount(block, base, draft), + 0 + ) +} + +/** + * The window's title, which is where the flow's "N changes not written" lives. + * + * ON THE HUB ONLY, and only when there is something to say. A title on every + * screen would make the count inescapable; a user who has just staged an edit + * needs to see that it is not on disk, and a user who has not should not be + * reading a number about changes they did not make. + * + * A decision rather than a ternary in the adapter, and moving it here is the + * point rather than the tidiness: the adapter was choosing between three strings + * it had no test for. + * + * NO SCOPE IN THE SIGNATURE. The scope is a ROW the hub carries, not a screen, + * so there is one hub screen for both scopes and a title naming the target file + * would have to change under the user's cursor. A parameter kept "for a future + * title that might need it" is a parameter the next reader tries to use, so the + * honest thing is to drop it and re-add it when there is a title to vary. + */ +export function configScreenTitle(screen: ConfigScreen, changes: number): string { + if (screen.kind === 'search') return `Nexus configuration — search "${screen.query}"` + if (screen.kind === 'fields') return `Nexus configuration — ${screen.block}` + if (screen.kind === 'editor') { + return `Nexus configuration — ${configPathLabel(screen.field.path)}` + } + return changes > 0 + ? `Nexus configuration — ${changes} changes not written` + : 'Nexus configuration' +} + /** * What `save` hands the config manager. * @@ -383,7 +544,13 @@ export function configSaveUpdate(draft: ConfigDraft): Partial { * answer, and the test that every real field has one is an assertion about this * rather than about a rendering. */ -export type ConfigEditor = 'boolean' | 'number' | 'text' | 'model' | 'none' +/** + * `path` edits as free text today and is named separately from `text` on + * purpose: a file path is not a string the user gets to make up, and the + * eventual path-validating editor belongs in the switch beside the others rather + * than replacing a case that `text` also serves. See `ConfigFieldKind`. + */ +export type ConfigEditor = 'boolean' | 'number' | 'text' | 'path' | 'model' | 'none' /** The editor for a field. `opaque` is the only kind with none. */ export function editorFor(field: ConfigField): ConfigEditor { @@ -538,8 +705,19 @@ export function configOptionTitle(option: ConfigOption, scope: ConfigScope): str return option.block.name case 'field': return option.field.label - case 'toggle': - return option.title + case 'search': + return '🔍 Search settings…' + case 'no-results': + // QUOTES the query. A miss whose row says only "no results" cannot be told + // from a typo by a user who does not remember what they typed, and the + // quotes make a query containing a space or a dot read as the one thing it + // is rather than as a sentence. + return `No setting matches "${option.query}"` + case 'switch': + // The direction is in WORDS. The glyph is reinforcement, not the carrier: + // a host that dropped it, or rendered it monochrome, would still leave a + // row a user can act on correctly. + return option.value ? '✅ Switch to Enabled' : '☐ Switch to Disabled' case 'back': return '← Back' case 'save': @@ -557,34 +735,74 @@ export function configOptionTitle(option: ConfigOption, scope: ConfigScope): str * row shows its value AND whether it has been changed since the flow opened — * the dialog equivalent of the panel's `*`, and the only way a staged edit is * visible before the save that commits it. + * + * `base` is REQUIRED, not defaulted. It is the only way a row can say how much + * has changed, and an omitted argument defaulted to `draft` would report `0 + * changed` for every block on every save — a plausible-looking lie rather than a + * type error, and the harder of the two to notice. A required parameter makes + * the call site fail to compile instead, which is the correct time to find out. + * + * EVERY DIRTY MARKER CARRIES A NUMBER as well as its glyph. The `*` is a glyph: + * it survives colour loss, a narrow font and a monochrome terminal, but it does + * not say HOW MUCH, and "which block did I change" is a question a count + * answers and a star does not. Where the number would be 0 or 1 it is left off + * instead — a field row reading `true * 1 changed` is noise, and a block with + * one edit does not need a count to be found. */ export function configOptionDescription( option: ConfigOption, draft: ConfigDraft, - dirty: boolean + base: ConfigDraft, + dirty: boolean, + changes?: number ): string { switch (option.kind) { case 'scope': return 'Nothing is written until you choose Save' + case 'search': + return 'Find a setting by block or name' + case 'no-results': + return 'Searches block names and setting names' case 'block': { const count = option.block.fields.length // The empty case is said out loud. `customRoles` is empty on a fresh // install, and a list holding nothing but "← Back" reads as a screen that // failed to load rather than as a block with nothing in it. - return count === 0 - ? 'no settings here yet — add custom roles in nexus.jsonc' - : `${count} ${count === 1 ? 'setting' : 'settings'}` + if (count === 0) { + return 'no settings here yet — add custom roles in nexus.jsonc' + } + const changed = configBlockDirtyCount(option.block, base, draft) + return changed === 0 + ? `${count} ${count === 1 ? 'setting' : 'settings'}` + : `${count} ${count === 1 ? 'setting' : 'settings'} ${changed} changed *` } case 'field': { const value = describeConfigValue(configValueAt(draft, option.field.path)) return dirty ? `${value} *` : value } - case 'toggle': - return '' + case 'switch': + return option.value + ? 'Turns it on — press Enter, then Save.' + : 'Turns it off — press Enter, then Save.' case 'back': return 'Your changes are kept — nothing is written yet' - case 'save': - return dirty ? 'Write the staged changes to disk' : 'No changes to write' + case 'save': { + // NO NUMBER, and dropping it is the correction rather than a loss of + // information. `configSaveUpdate` hands `updateStorageConfig` the WHOLE + // draft, which replaces nine blocks wholesale and shadows the project and + // global files for every key it names — so "Write 3 changes to disk" + // counted things the user TOUCHED and said they were things being + // WRITTEN. The values are identical either way (the draft IS the merged + // config), so the number was never false about intent; it was false + // about the write, and a sentence that claims to describe a write should + // not carry a number. `changes` is still honoured where it is honest — + // the title's unsaved count, and the per-block and per-field markers, + // which genuinely are "what you changed". + if (changes === undefined) { + return dirty ? 'Write the staged changes to disk' : 'No changes to write' + } + return changes === 0 ? 'No changes to write' : 'Write the staged changes to disk' + } case 'close': return 'Leave. Nothing is written' } @@ -610,9 +828,18 @@ export function configPromptDescription(field: ConfigField): string | undefined * omitting it would make a setting look non-existent rather than unchangeable, * which is the same defect as a dead knob — and the host's `disabled` flag * renders it and refuses it, which is a real host feature rather than a - * hand-rolled grey row. A boolean's own two rows are never disabled, whatever - * the current value: choosing the row a key is already on is a way to leave it - * alone, and disabling it would make "off, and I mean off" unreachable. + * hand-rolled grey row. + * + * `save` IS NEVER DISABLED, and this is deliberate rather than an oversight, so + * it is stated here where the temptation to "fix" it lives. Greying Save out when + * nothing has changed looks tidier and is a worse dialog: the row is how a user + * discovers the flow's whole contract, that Save is the only thing that writes, + * and a user who opens the dialog to find out where their settings go is not + * served by a button that has quietly become inert. It is also a state the + * adapter cannot cheaply know at render time — `draft` is reassigned on every + * edit, so "is it clean" is a function of the moment, not a value captured when + * the screen was built. Pressing Save on a clean draft is not an error: it + * writes the config back unchanged and says so, which is an answer. */ export function configOptionDisabled(option: ConfigOption): boolean { return option.kind === 'field' && editorFor(option.field) === 'none' @@ -646,8 +873,25 @@ export type ConfigOption = | { readonly kind: 'block'; readonly block: ConfigBlock } /** One field. Choosing it opens that field's editor. */ | { readonly kind: 'field'; readonly field: ConfigField } - /** One of a boolean's two rows. */ - | { readonly kind: 'toggle'; readonly value: boolean; readonly title: string } + /** The hub's search row. Choosing it asks the host for a query. */ + | { readonly kind: 'search' } + /** + * A search that matched nothing, standing in for the result list. + * + * A ROW rather than an absence of one, and that is the whole reason it exists: + * `ui.dialog.select` with zero options is a dialog the user can see but not + * leave, and it looks exactly like a failed load. A row that names the query + * that missed and says what the search covers turns "nothing here" into + * something the user can read and something they can answer. + */ + | { readonly kind: 'no-results'; readonly query: string } + /** + * The one answer a boolean editor offers: switch to the OTHER value. + * + * Carries the field it belongs to as well as the value, so a row that reaches + * the adapter by any route still says which path it writes. + */ + | { readonly kind: 'switch'; readonly field: ConfigField; readonly value: boolean } /** Go back up one level. */ | { readonly kind: 'back' } /** Write the draft. Only on the hub. */ @@ -656,13 +900,20 @@ export type ConfigOption = | { readonly kind: 'close' } /** - * The hub's rows: the save scope, then every block, then save and close. + * The hub's rows: the save scope, search, then every block, then save and close. * * `save` and `close` are rows rather than the absence of a selection, because * "press escape to leave" and "press escape to throw your changes away" are the * same key and only one of them is what a user means at the hub — where there is * nothing staged to throw away in the first place, since nothing is written * until save. + * + * SEARCH IS THE SECOND ROW, and its position is load-bearing rather than + * stylistic. The hub lists every config key, so it is past a screenful on any + * real config; a search row below the blocks would sit under the fold on every + * device this ships to, and a feature that is never seen is a feature with no + * users. Immediately after the scope row is the only place in the list a user + * reads before scrolling. */ export function blockOptions( discovery: ConfigDiscovery, @@ -670,6 +921,7 @@ export function blockOptions( ): readonly ConfigOption[] { return [ { kind: 'scope', scope }, + { kind: 'search' as const }, ...discovery.blocks.map(block => ({ kind: 'block' as const, block })), { kind: 'save' as const }, { kind: 'close' as const } @@ -693,32 +945,27 @@ export function fieldOptions(discovery: ConfigDiscovery, block: string): readonl } /** - * `✅ Enabled` / `☐ Disabled`, and the reason it is shaped like that. + * The one switch, plus a way out. * - * The host's `ui` object has no checkbox, no multi-select and no form: - * `DialogSelectOption` carries only `title`, `value`, `description?`, `footer?`, - * `category?` and `disabled?`, and `dialog.select` resolves a SINGLE value, so a - * switch in a list can only be two rows you pick between. That is a real - * limitation of the host and this workaround is honest — the glyph is part of - * the row's own title, the other row is the other value, and the row the key is - * already on is named as current so the host opens there rather than at the top. + * TWO ROWS, and the second is `back` rather than the other value. A boolean used + * to be offered as `✅ Enabled` / `☐ Disabled` and the user picked between them, + * which cost a second dialog per toggle and — worse — made "turn this on" and + * "confirm this is on" the same gesture, so a toggle that was already on could + * only be left alone by choosing a row that looked identical in effect. Now the + * editor screen IS the toggle surface: one row that means "switch", and one that + * means "don't". Flipping twice is two keystrokes and gets you back, so nothing + * is lost by not offering the value a key is already on. * - * The CURRENT row is placed first, which is also why the order is computed - * rather than written out: a two-row list whose order depended on the value - * would be two lists, and the one test here has to be about both of them. + * The title states the direction IN WORDS, with the glyph beside it, and that is + * not decoration. The row says `Switch to Disabled` because that is the thing the + * user needs to read; `☐` alone is the reinforcement, and a host that rendered + * it as monochrome or as a different font would still leave the row readable. */ -export function booleanOptions(current: boolean): readonly ConfigOption[] { - const rows: readonly { value: boolean; title: string }[] = [ - { value: true, title: '✅ Enabled' }, - { value: false, title: '☐ Disabled' } +export function booleanOptions(field: ConfigField, current: boolean): readonly ConfigOption[] { + return [ + { kind: 'switch', field, value: !current }, + { kind: 'back' } ] - return rows - .map(row => ({ - kind: 'toggle' as const, - value: row.value, - title: row.value === current ? `${row.title} (current)` : row.title - })) - .sort((a, b) => Number(b.value === current) - Number(a.value === current)) } /** @@ -745,8 +992,133 @@ export function configScreenOptions( current: (field: ConfigField) => unknown ): readonly ConfigOption[] { if (screen.kind === 'blocks') return blockOptions(discovery, scope) + if (screen.kind === 'search') return searchOptions(discovery, screen.query) if (screen.kind === 'fields') return fieldOptions(discovery, screen.block) - return screen.field.kind === 'boolean' ? booleanOptions(current(screen.field) === true) : [] + return screen.field.kind === 'boolean' + ? booleanOptions(screen.field, current(screen.field) === true) + : [] +} + +// ── Searching the hub ─────────────────────────────────────────────── + +/** + * The search's text, or `undefined` when there is nothing to search for. + * + * A dismissed prompt and an empty one are the SAME query, which is the point: + * both mean "no query", and both mean the flow stays where it was. Normalising + * them at the boundary means no later function has to ask which of the two it + * got, and an empty string can never reach `searchOptions` as a query that + * matches every row — which is the one outcome that would turn "search" into a + * second copy of the hub. + */ +export function configSearchQuery(raw: string | undefined): string | undefined { + const trimmed = raw?.trim() + return trimmed === undefined || trimmed === '' ? undefined : trimmed +} + +/** + * Lowercase both sides, once per call site that needs it. + * + * Search is a substring test, not a prefix and not a token: a user who types + * `cost` means `maxTotalCost`, and a prefix rule would have told them nothing + * matched. No fuzzy matching either — an edit distance invites the wrong row at + * the top of a list whose whole job is addressing a specific key, and a wrong + * match here is a config value changed to something the user never read. + */ +function normalized(value: string): string { + return value.toLowerCase() +} + +/** + * Whether `field` is what the user typed. + * + * The LABEL or the DOTTED PATH, and the path is not redundant: `enabled` exists + * in eight blocks, so a search for `budget.enabled` is only meaningful against + * the path, and matching labels alone would return eight rows for it. Deep + * matching is also the reason this is worth having over "jump to the block" — + * a user who remembers `maxTotalCost` and not which block it lives in should + * not have to walk nine blocks to find it. + */ +export function fieldMatches(field: ConfigField, query: string): boolean { + const needle = normalized(configSearchQuery(query) ?? '') + if (needle === '') return false + return ( + normalized(field.label).includes(needle) || + normalized(configPathLabel(field.path)).includes(needle) + ) +} + +/** Whether the block is what the user typed, OR holds something that is. */ +export function blockMatches(block: ConfigBlock, query: string): boolean { + const needle = normalized(configSearchQuery(query) ?? '') + if (needle === '') return false + if (normalized(block.name).includes(needle)) return true + return block.fields.some(field => fieldMatches(field, query)) +} + +/** The fields `query` names, in the discovery's own order. */ +export function filterConfigFields( + discovery: ConfigDiscovery, + query: string +): readonly ConfigField[] { + return discovery.fields.filter(field => fieldMatches(field, query)) +} + +/** The blocks `query` names or that hold something it names. */ +export function filterConfigBlocks( + discovery: ConfigDiscovery, + query: string +): readonly ConfigBlock[] { + return discovery.blocks.filter(block => blockMatches(block, query)) +} + +/** + * The search's rows. + * + * FIELDS FIRST, and that ordering is the feature. A user who typed a setting + * name wants that setting, and a result list that led with the block containing + * it would make them press Enter a second time to reach what they already + * named. Blocks follow for the case where someone searched for a block's name + * instead of a field's. + * + * A block appears ONLY if none of its fields matched, so a query like `enabled` + * does not list the same eight settings twice — once as fields and once under + * eight headings. A duplicate result is not a cosmetic problem here: each one + * opens a different path, and a list that offers the same key twice under two + * descriptions is a list nobody can learn to read. + * + * The `no-results` row is a ROW, and it is selectable, and it goes to the hub. + * A `select` handed zero options cannot be escaped by anything but the host's + * own key — and it is visually identical to a screen that failed to load, so a + * user who mistyped once would be looking at what looks like a bug. `back` is + * present unconditionally, which is what makes the search screen escapable even + * when it has results. + */ +export function searchOptions( + discovery: ConfigDiscovery, + query: string +): readonly ConfigOption[] { + // The empty query is the ONE case that gets its own shape, and the reason is + // the same one `configSearchQuery` exists for. The live path normalises at the + // boundary so this cannot be reached with `''` — but a function that is + // exported, and tested, on its own terms should not depend on every caller + // having remembered that. Without the guard, `''` matches no field and no + // block, so the user would be told "no results" while staring at a row that + // quotes the empty string: a dead end manufactured out of a question that was + // never asked. `[{ kind: 'back' }]` is the honest answer — there is nothing to + // show and the screen stays escapable. + if (configSearchQuery(query) === undefined) return [{ kind: 'back' }] + const fields = filterConfigFields(discovery, query) + const blocks = filterConfigBlocks(discovery, query).filter( + block => !block.fields.some(field => fieldMatches(field, query)) + ) + const rows: ConfigOption[] = [ + ...fields.map(field => ({ kind: 'field' as const, field })), + ...blocks.map(block => ({ kind: 'block' as const, block })) + ] + if (rows.length === 0) rows.push({ kind: 'no-results', query }) + rows.push({ kind: 'back' }) + return rows } // ── Navigation ────────────────────────────────────────────────────── @@ -769,6 +1141,18 @@ export type ConfigSelect = | { readonly kind: 'stay' } | { readonly kind: 'save' } | { readonly kind: 'close' } + /** + * The host has to produce a string before there is a screen to go to. + * + * Its own outcome rather than a `to`, and the reason is that the flow must not + * invent a state it has not decided to be in: `screen: {kind:'search'}` needs + * a `query`, and the only source of one is the host's prompt. Returning `to` + * with an empty query would make an EMPTY SEARCH a real screen — one that + * matched everything — so a dismissed prompt would silently drop the user into + * a list of every setting in the config. The adapter turns `ask` into the + * prompt, and stays put if there is no answer. + */ + | { readonly kind: 'ask'; readonly screen: ConfigScreen } /** * Act on a chosen row. @@ -804,11 +1188,22 @@ export function configSelect(screen: ConfigScreen, option: ConfigOption): Config if (editorFor(option.field) === 'none') return { kind: 'stay' } return { kind: 'to', screen: { kind: 'editor', field: option.field } } } - case 'toggle': - // A toggle row is only ever on a boolean's editor screen, and choosing it + case 'switch': + // A switch row is only ever on a boolean's editor screen, and choosing it // is an ANSWER, not a destination: the flow does not move, and the adapter // commits it with `commitConfigToggle` and returns to the field's list. return { kind: 'stay' } + case 'search': + // An ASK, not a `to`. The `search` screen carries a query, and the query + // does not exist until the host's prompt produces one — so naming a screen + // here would mean naming one whose contents the flow has not decided. + // See `ConfigSelect`'s `ask`. + return { kind: 'ask', screen: { kind: 'search', query: '' } } + case 'no-results': + // Somewhere REAL rather than nowhere: a select with no options is + // unescapable and looks like a failed load. The hub is the one place a user + // can see every block, so it is where a dead end is worth routing to. + return { kind: 'to', screen: { kind: 'blocks' } } case 'back': { const up = configUp(screen) // Unreachable from any list the flow builds — `back` is only ever offered @@ -842,6 +1237,24 @@ export function configScreenField(screen: ConfigScreen): ConfigField | undefined */ export function configUp(screen: ConfigScreen): ConfigScreen | undefined { if (screen.kind === 'blocks') return undefined + // ONE HOP, not a chain through the hub. The real path out of a search result + // is search → fields: → blocks, and this is its penultimate step: an + // editor reached FROM a search goes to its block's field list, exactly as an + // editor reached from that list does, and the search screen itself goes + // straight to the hub. So the deepest route out of the flow is one escape + // from the editor, one from the field list, one from the hub — and a search + // screen that had to route through the hub first would put the user's + // "get me back to where I was" behind an extra list they did not ask for. + // + // WHAT THIS RELIES ON: the editor screen is MODAL. `runEditor` awaits the + // host's prompt and returns the answer, so the transient editor is not itself + // an escape the user has to find — if a future refactor makes the editor a + // screen the user navigates away from with the same key, the bound changes + // and the counting above is no longer the guarantee. + // + // The query is not carried through, and does not need to be: escaping the + // search discards it, which is the same thing the user did by leaving. + if (screen.kind === 'search') return { kind: 'blocks' } if (screen.kind === 'fields') return { kind: 'blocks' } return { kind: 'fields', block: screen.field.block } } @@ -850,6 +1263,10 @@ export function configUp(screen: ConfigScreen): ConfigScreen | undefined { export function configScreenBlock(screen: ConfigScreen): string | undefined { switch (screen.kind) { case 'blocks': + // A search spans the whole config, so there is no single block it is in — + // and answering "which block" with the first one that matched would put the + // dialog's title on a block the user never entered. + case 'search': return undefined case 'fields': return screen.block @@ -871,20 +1288,30 @@ export function configFieldsScreen(field: ConfigField): ConfigScreen { } /** - * Commit a boolean's answer, given the row the user chose. + * Commit a boolean's answer, given the switch row the user chose. + * + * THE ONLY PATH BY WHICH A BOOLEAN REACHES THE DRAFT. It was already that, and + * the single-switch widget makes it more clearly so: there is now exactly one row + * that can produce a boolean, so a second route would be a second widget rather + * than a second gesture on one widget. + * + * The value is the row's, taken LITERALLY, and this is the property worth stating + * because the earlier two-row widget put it under pressure. That version also + * offered the row the key was already on, which meant "choosing the row you are + * on" and "flipping the switch" had to be told apart here — and a commit that + * read the current value and inverted it would have made the first mean the + * second, writing a change to a setting the user had explicitly declined to + * change. With one row standing only for the other value, that confusion is not + * expressible: the row says what it writes, and this writes what the row says. * - * A separate function because the boolean's two rows carry the value they stand - * for and there is no text to parse: choosing `☐ Disabled` writes `false`, and - * choosing the row the key is ALREADY on writes that same value — which is how a - * user says "off, and I mean off" without needing a separate cancel. The - * assignment is deliberately NOT an inversion of what was there: inverting would - * make "choose the row that is already current" mean "flip it", the opposite of - * what the row says. + * The guard is on the ROW's kind and not on the value's type, so a `back` row, a + * `save` row or a search result that reached here by mistake is refused rather + * than written to a path nothing chose. */ export function commitConfigToggle(field: ConfigField, chosen: ConfigOption): ConfigCommit { - if (chosen.kind !== 'toggle') { + if (chosen.kind !== 'switch') { throw new Error( - `a boolean editor was answered with a ${chosen.kind} row, not a toggle row` + `a boolean editor was answered with a ${chosen.kind} row, not a switch row` ) } return { kind: 'accepted', field, value: chosen.value } diff --git a/src/config.ts b/src/config.ts index f43b807..0b64047 100644 --- a/src/config.ts +++ b/src/config.ts @@ -344,6 +344,49 @@ export interface NexusFullConfig { */ effort: NexusEffortConfig customRoles: NexusCustomRoleConfig[] + /** + * The durable note store, the ninth block. + * + * This block used to live on the wider embedder type `NexusConfig` and was + * REMOVED from it, correctly: nothing read any of it. `enabled` gated + * nothing, `maxEntriesPerScope` was written into a defaults literal and read + * by no consumer, and a knob a user can set with no effect is worse than no + * knob — see `src/types.ts:465` for the full reasoning. It is listed here + * now because that is no longer true: `recallForTask` is called + * unconditionally at both injection points (`orchestrator.ts:2512` in + * `executeTask`, `orchestrator.ts:3742` in `spawnAgent`), so memory is + * unconditionally ON and the knob below is the first honest way to turn it + * off. It returns on the FILE-settable type rather than the embedder type + * because every field here has a consumer, which is what the removed block + * did not. + * + * Field by field, and named against its consumer: + * - `enabled` — the gate at the TOP of `recallForTask` + * (`orchestrator.ts:4801`). `true` by default, and `true` is what is + * already happening, so the default is behaviour-preserving. + * - `storage` — the sqlite file path, `MemoryStoreConfig.dbPath`, resolved + * at USE time through the resolver the orchestrator passes to + * `new PersistentMemoryStore(...)` (`orchestrator.ts`). A change of path + * closes the open database and opens the new one on the next call, so it + * needs no restart — see `database` in `src/memory-store.ts`. + * - `maxEntries` — `MemoryStoreConfig.maxEntries`, the per-scope FIFO cap + * enforced by `evictOverflow` in `src/memory-store.ts` and re-read on every + * write. The `project` scope is exempt from it BY DESIGN (a human's durable + * notes must not be evicted by disposable entries), and that exemption is + * not to be "fixed" here. + * + * There is deliberately NO `inject` field — considered and rejected. A second + * boolean that turns the prompt block on while leaving retrieval running would + * be a half-differentiated switch whose two states a user cannot tell apart + * after the fact, which is the SAME failure the block removed from + * `NexusConfig` was: a knob that changes nothing observable. `enabled` is + * the only knob, and it is wired to a real early return. + */ + memory: { + enabled: boolean + storage: string + maxEntries: number + } } /** @@ -683,7 +726,34 @@ const DEFAULT_CONFIG: NexusFullConfig = { // Empty rather than absent: a user with no custom roles is the default, and // an empty list is what every merge level falls through to, so "no // `customRoles` block anywhere" and "an empty one" resolve the same way. - customRoles: [] + customRoles: [], + // On by default, and the default is what is ALREADY happening: `recallForTask` + // is called unconditionally at `orchestrator.ts:2512` and `~:3742`, so `false` + // would be a behaviour change and `true` is merely the first honest way to + // say it. This is the deliberate difference from the `memory` block that was + // removed from `NexusConfig` — there, `enabled` gated nothing. + memory: { + enabled: true, + // MUST mirror `MemoryStoreConfig.DEFAULT_CONFIG.dbPath` + // (`src/memory-store.ts`) exactly, expression and all. The two literals are + // the same value by CONSTRUCTION, not by a "change both" convention resting + // on review: the orchestrator constructs the store with a RESOLVER over + // `configManager.getConfig().memory` (`orchestrator.ts`, `new + // PersistentMemoryStore(() => …)`), so this is the value the store opens and + // the store's own default only applies when a caller passes no config at + // all. `test/config-knobs.test.ts` asserts the two agree, so a one-sided + // change fails the suite rather than shipping a path the config does not + // name. + // + // The resolution is LIVE: editing this takes effect on the next store call, + // with no restart. See `database` in `src/memory-store.ts`. + storage: join(process.env.HOME || '~', '.local', 'share', 'opencode-nexus', 'memory.db'), + // Mirrors `MemoryStoreConfig.DEFAULT_CONFIG.maxEntries` + // (`src/memory-store.ts`), which is a PER-SCOPE cap: `project` is exempt + // by design, so a smaller number here still never evicts a durable note. + // Also live — re-read on every `set`, no reopen involved. + maxEntries: 1000 + } } /** @@ -823,6 +893,20 @@ export class NexusConfigManager { * `dashboardBase`. */ private selfHealingBase: NexusFullConfig['selfHealing'] + /** + * Programmatic starting point for the `memory` block, beneath the file levels + * and above `DEFAULT_CONFIG`. Same single-consumer rationale as + * `dashboardBase`: the orchestrator seeds it and every consumer reads the + * merged result, so the setting is readable from one place with a defined + * precedence. + * + * There is deliberately NO stored copy of `memory.enabled` on the + * orchestrator. The flag is read through `getConfig()` at the gate in + * `recallForTask`, so a second copy could not be updated by a TUI edit or a + * file reload and would go on answering with the value it was constructed + * with — which is the exact failure `dashboardBase`'s doc comment describes. + */ + private memoryBase: NexusFullConfig['memory'] constructor( dashboardBase?: Partial, @@ -831,7 +915,8 @@ export class NexusConfigManager { gitFlowBase?: Partial, effortBase?: Partial, budgetBase?: Partial, - selfHealingBase?: Partial + selfHealingBase?: Partial, + memoryBase?: Partial ) { // Config files are loaded later via loadFromPath(basePath) this.projectConfig = null @@ -868,6 +953,27 @@ export class NexusConfigManager { alertThreshold: budgetBase?.alertThreshold ?? DEFAULT_CONFIG.budget.alertThreshold } this.selfHealingBase = { ...DEFAULT_CONFIG.selfHealing, ...selfHealingBase } + // Last one added, so last one seeded. A spread rather than three `??` + // chains because this block has no file-shape subset to protect: there is + // no constructor-only key in it, unlike `budget`'s `hardLimit`. + // + // …but the seed is FILTERED first, and that filter is the point. Object + // spread copies own enumerable keys INCLUDING ones whose value is + // `undefined`, so `{ storage: undefined }` would not fall through to the + // default — it would DELETE `storage` from the base. The orchestrator's 8th + // seed is built as a literal of possibly-absent fields + // (`{ storage: memoryStoreConfig?.dbPath, maxEntries: ... }`), which is + // exactly that shape, so an unfiltered spread here left + // `getConfig().memory` as `{ enabled: true }`: `getSaveableConfig()` then + // wrote `"memory": {"enabled": true}` on the next save, `discoverConfig` + // classified both lost keys `opaque`, and `getConfig().memory.storage` + // disagreed with `memoryStore.path` with nothing logging it. Assigning a + // field only when its source is `!== undefined` keeps the "the store's + // config and the store agree" promise the mirror comments above make. + const memorySeed: Partial = {} + if (memoryBase?.storage !== undefined) memorySeed.storage = memoryBase.storage + if (memoryBase?.maxEntries !== undefined) memorySeed.maxEntries = memoryBase.maxEntries + this.memoryBase = { ...DEFAULT_CONFIG.memory, ...memorySeed } } /** @@ -1088,7 +1194,21 @@ export class NexusConfigManager { customRoles: (this.storageConfig?.customRoles ?? this.projectConfig?.customRoles ?? this.globalConfig?.customRoles - ?? this.customRolesBase).map(role => ({ ...role })) + ?? this.customRolesBase).map(role => ({ ...role })), + // Field by field, for the same reason as every other object block above: + // a level that sets only `enabled` — which is the single field a user is + // most likely to write, and the one `/nexus config` toggles — must not + // blank `storage` back to the level beneath. A spread would resolve the + // same way here, but the rule is uniform across the blocks and one + // exception is how the `retryDelay` class of bug got in. + memory: { + enabled: this.storageConfig?.memory?.enabled ?? this.projectConfig?.memory?.enabled + ?? this.globalConfig?.memory?.enabled ?? this.memoryBase.enabled, + storage: this.storageConfig?.memory?.storage ?? this.projectConfig?.memory?.storage + ?? this.globalConfig?.memory?.storage ?? this.memoryBase.storage, + maxEntries: this.storageConfig?.memory?.maxEntries ?? this.projectConfig?.memory?.maxEntries + ?? this.globalConfig?.memory?.maxEntries ?? this.memoryBase.maxEntries + } } } @@ -1481,6 +1601,13 @@ export class NexusConfigManager { // with nothing to restore them from. result.customRoles = current.customRoles.map(role => ({ ...role })) + // Memory — same reason, all three fields. This block is the one where an + // omission is least visible: the file keeps working, the store keeps + // working, and a user's chosen DB path and entry cap silently reset to the + // defaults at the first model change, leaving notes in a file nothing names + // any more. `saveProjectConfig` writes this return value as the whole file. + result.memory = { ...current.memory } + return result } diff --git a/src/memory-store.ts b/src/memory-store.ts index e9ef6f0..e0347e2 100644 --- a/src/memory-store.ts +++ b/src/memory-store.ts @@ -48,10 +48,57 @@ export interface MemoryStoreOptions { } const DEFAULT_CONFIG: MemoryStoreConfig = { + // MUST mirror `NexusFullConfig`'s `DEFAULT_CONFIG.memory.storage` + // (`src/config.ts`) exactly, expression and all. The same default is written + // down in two places because a user can now see and change it in + // `nexus.jsonc`; if the two drift, the config reports a path the store is not + // using, with nothing logging the disagreement. If you change one, change + // both. + // + // The two literals are the same value by CONSTRUCTION rather than by + // coincidence: the store never falls back to this one behind the config's + // back, because the orchestrator feeds it a resolver over + // `configManager.getConfig().memory` (see the constructor), and this default + // only applies when a caller supplies no config at all. So the mirror is not a + // "change both" convention resting on review — a change to one and not the + // other is a bug `test/config-knobs.test.ts` fails on. The resolution is LIVE: + // an edit to either key takes effect on the next store call, with no restart. dbPath: join(process.env.HOME || '~', '.local', 'share', 'opencode-nexus', 'memory.db'), + // Mirrored by `DEFAULT_CONFIG.memory.maxEntries` in `src/config.ts`, and + // applied live: read on every `set` rather than frozen here. See + // `evictOverflow`. maxEntries: 1000, } +/** + * A live view of the store's settings. + * + * `() => Partial` rather than the full config, because every + * real caller has the merged result already and a required `dbPath` would make + * each of them restate a default. Both fields merge over `DEFAULT_CONFIG`. + */ +export type MemoryStoreConfigResolver = () => Partial + +/** + * Merge config layers over the defaults, IGNORING `undefined`-valued keys. + * + * Not a spread, and the difference is the same one that cost `memory.storage` a + * whole key once already (`src/config.ts`, the filtered `memorySeed`): object + * spread copies own enumerable keys INCLUDING ones whose value is + * `undefined`, so `{ storage: undefined }` would DELETE the key rather than fall + * through to the default. Callers legitimately pass literals of + * possibly-absent fields — the orchestrator's constructor parameter is one — so + * this is the expected shape here, not a hypothetical. + */ +function mergeStoreConfig(...layers: Array | undefined>): MemoryStoreConfig { + const merged: MemoryStoreConfig = { ...DEFAULT_CONFIG } + for (const layer of layers) { + if (layer?.dbPath !== undefined) merged.dbPath = layer.dbPath + if (layer?.maxEntries !== undefined) merged.maxEntries = layer.maxEntries + } + return merged +} + /** * Scopes exempt from `maxEntries`, because what they hold is a human's * deliberate record rather than a cache line. @@ -84,8 +131,36 @@ export interface MemoryEviction { } export class PersistentMemoryStore { - private db: Database - private config: MemoryStoreConfig + /** + * The open handle, or `null` when nothing is open. + * + * Nullable because it is no longer opened once in the constructor and held for + * the life of the process: it is opened for the path that is CURRENTLY + * resolved, and re-opened when that changes. See `database`. + */ + private db: Database | null = null + /** + * The path `db` was opened for, and the thing `database` compares a freshly + * resolved path against. Stored beside the handle rather than recomputed from + * it, because the resolved path is a config value and the handle does not + * report the file it came from. + */ + private openPath: string | null = null + /** + * Set by `close`. Distinguished from "nothing is open yet" so a closed store + * stays closed: lazily reopening on the next call would turn an explicit + * `shutdown()` into a no-op that silently resurrects the file. + */ + private closed = false + /** + * Where the settings come from, re-read on every use. + * + * A FUNCTION, not a snapshot, and that is the whole point of the field: a + * `storage` or `maxEntries` edit made in `nexus.jsonc` has to be observable by + * the running process. Holding a copy — which is what this field used to be — + * made the config panel report a saved value the store ignored until restart. + */ + private resolveConfig: MemoryStoreConfigResolver /** * Cumulative per-scope count of entries removed by `maxEntries`, so a * surface can report "12 notes have been evicted" rather than leaving a user @@ -137,19 +212,91 @@ export class PersistentMemoryStore { */ private now: () => number - constructor(config?: Partial, options?: MemoryStoreOptions) { - this.config = { ...DEFAULT_CONFIG, ...config } + /** + * @param config Either a fixed `Partial`, or a RESOLVER + * re-read on every use so `storage` / `maxEntries` take effect live. The + * union rather than a second parameter because a fixed config is just the + * degenerate resolver, and every existing caller passes the former — which + * is why this is backward compatible and not a breaking change. + * @param options Clock seam, unchanged. + */ + constructor( + config?: Partial | MemoryStoreConfigResolver, + options?: MemoryStoreOptions, + ) { + this.resolveConfig = typeof config === 'function' ? config : () => config ?? {} this.now = options?.now ?? (() => Date.now()) - // Ensure directory exists - try { mkdirSync(dirname(this.config.dbPath), { recursive: true }) } catch {} + // Opened eagerly rather than truly on first use, and the difference is + // worth naming: this is still the first USE, it just happens to be the + // constructor. Opening here means a bad path or an unwritable directory + // throws at construction — where the stack names the caller — instead of + // surfacing as a mysterious failure inside some agent's recall. A path + // change LATER is the part that is deferred, and that is the part that has + // to be, because the value cannot exist before the config is re-read. + this.database() + } - this.db = new Database(this.config.dbPath) - this.init() + /** + * The settings as of right now. + * + * A fresh object per call, so a caller cannot capture one and hold a stale + * copy — `evictOverflow` deliberately re-reads this per write rather than + * trusting a value resolved at the top of `set`. + */ + private currentConfig(): MemoryStoreConfig { + return mergeStoreConfig(this.resolveConfig()) } - private init(): void { - this.db.exec(` + /** + * The open database for the CURRENTLY resolved path, opening or swapping it + * as needed. + * + * ── WHY A SWAP IS SAFE HERE, AND WHY IT IS SYNCHRONOUS ── + * + * `bun:sqlite` is synchronous, so this whole method runs to completion with no + * await point in it. JavaScript is single-threaded, so no other `recallForTask` + * — however many agents are mid-flight — can observe a state between "decided + * to swap" and "finished swapping": the next caller to arrive runs after this + * one returns. The race a rebuild-in-`reloadConfigFromDisk` design has (every + * in-flight reader holding a handle to a store that is being closed underneath + * it) does not exist when the swap is confined to one synchronous block. + * + * The ORDER is what makes a failure recoverable rather than destructive: the + * new handle is opened AND initialised first, and only then is the old one + * closed. A path that cannot be opened throws with the previous database + * still open and still current, so the store keeps serving reads from the last + * good path instead of ending up with no handle at all. A `close` that throws + * is swallowed for the mirror-image reason: the new handle is already open, and + * losing it over a cleanup failure would be strictly worse than leaking the old + * one. + */ + private database(): Database { + if (this.closed) throw new Error('memory store is closed') + const { dbPath } = this.currentConfig() + if (this.db !== null && this.openPath === dbPath) return this.db + + try { mkdirSync(dirname(dbPath), { recursive: true }) } catch {} + const opened = new Database(dbPath) + try { + this.init(opened) + } catch (error) { + // Never leave a half-open handle behind: the old database is still the + // live one, and an extra connection to a file the caller is about to be + // told is unusable is a lock they did not ask for. + try { opened.close() } catch {} + throw error + } + if (this.db !== null) { + try { this.db.close() } catch {} + } + this.db = opened + this.openPath = dbPath + return opened + } + + private init(db: Database): void { + db.exec(` CREATE TABLE IF NOT EXISTS memory ( id TEXT PRIMARY KEY, key TEXT NOT NULL, @@ -222,7 +369,7 @@ export class PersistentMemoryStore { const id = `mem-${timestamp}-${sequence}-${Math.random().toString(36).substr(2, 9)}` const expiresAt = entry.ttl ? timestamp + entry.ttl : null - const stmt = this.db.prepare(` + const stmt = this.database().prepare(` INSERT INTO memory (id, key, value, scope, author, timestamp, confidence, tags, ttl, expires_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `) @@ -250,7 +397,14 @@ export class PersistentMemoryStore { */ private evictOverflow(scope: MemoryScope): void { if (COUNT_EXEMPT_SCOPES.has(scope)) return - if (this.config.maxEntries <= 0) return + // THE CAP IS A PURE READ, resolved on EVERY write. It is not snapshotted at + // construction, and it needs no database swap: lowering `maxEntries` in + // `nexus.jsonc` makes the very next `set` evict down to the new number, with + // no restart and no reopening. Reading it once per `set` rather than once + // per process is what makes the setting live. + const maxEntries = this.currentConfig().maxEntries + if (maxEntries <= 0) return + const db = this.database() // Expired first: they are unreadable already, so spending cap headroom on // them evicts a LIVE row in their place for no gain. @@ -258,19 +412,19 @@ export class PersistentMemoryStore { // see `getRecent`. Without it, "the oldest N" is an arbitrary set whenever // two writes land in the same millisecond, and the entry evicted would be // whichever the query plan reached first. - const expired = this.db.prepare( + const expired = db.prepare( 'SELECT id, timestamp FROM memory WHERE scope = ? AND expires_at IS NOT NULL AND expires_at <= ? ORDER BY timestamp ASC, id ASC' ).all(scope, this.now()) as Array<{ id: string; timestamp: number }> - const live = (this.db.prepare( + const live = (db.prepare( 'SELECT COUNT(*) as count FROM memory WHERE scope = ? AND (expires_at IS NULL OR expires_at > ?)' ).get(scope, this.now()) as { count: number }).count - let over = expired.length + (live - this.config.maxEntries) + let over = expired.length + (live - maxEntries) let oldestEvictedAt: number | null = null let count = 0 - const deleteStmt = this.db.prepare('DELETE FROM memory WHERE id = ?') + const deleteStmt = db.prepare('DELETE FROM memory WHERE id = ?') if (over > 0) { for (const row of expired) { @@ -283,7 +437,7 @@ export class PersistentMemoryStore { } if (over > 0) { - const liveRows = this.db.prepare( + const liveRows = db.prepare( 'SELECT id, timestamp FROM memory WHERE scope = ? AND (expires_at IS NULL OR expires_at > ?) ORDER BY timestamp ASC, id ASC LIMIT ?' ).all(scope, this.now(), over) as Array<{ id: string; timestamp: number }> for (const row of liveRows) { @@ -337,7 +491,7 @@ export class PersistentMemoryStore { * `describeEmptyResult` in `src/memory-recall.ts`. */ get path(): string { - return this.config.dbPath + return this.currentConfig().dbPath } get(key: string, scope?: MemoryScope): MemoryEntry | null { @@ -358,6 +512,7 @@ export class PersistentMemoryStore { * decided by the order the storage engine happened to return them in. */ getByKey(key: string, scope?: MemoryScope): MemoryEntry[] { + const db = this.database() let query = 'SELECT * FROM memory WHERE key = ?' const params: SQLQueryBindings[] = [key] @@ -371,12 +526,12 @@ export class PersistentMemoryStore { query += ' ORDER BY timestamp ASC, id ASC' - const rows = this.db.prepare(query).all(...params) as RowData[] + const rows = db.prepare(query).all(...params) as RowData[] return rows.map(row => this.rowToEntry(row)) } getByScope(scope: MemoryScope): MemoryEntry[] { - const rows = this.db.prepare( + const rows = this.database().prepare( 'SELECT * FROM memory WHERE scope = ? AND (expires_at IS NULL OR expires_at > ?) ORDER BY timestamp DESC, id DESC' ).all(scope, this.now()) as RowData[] return rows.map(row => this.rowToEntry(row)) @@ -405,6 +560,7 @@ export class PersistentMemoryStore { * blob, nested `memoryEntries` and all, rendered as though it were a note. */ search(query: string, scope?: MemoryScope): MemoryEntry[] { + const db = this.database() let sql = `SELECT * FROM memory WHERE (key LIKE ? OR value LIKE ?) AND (expires_at IS NULL OR expires_at > ?)` const params: SQLQueryBindings[] = [`%${query}%`, `%${query}%`, this.now()] if (scope) { @@ -414,7 +570,7 @@ export class PersistentMemoryStore { // `id DESC` tie-breaks same-millisecond writes, which share a `timestamp`. // See `getRecent` for why that is not optional. sql += ' ORDER BY timestamp DESC, id DESC' - const rows = this.db.prepare(sql).all(...params) as RowData[] + const rows = db.prepare(sql).all(...params) as RowData[] return rows.map(row => this.rowToEntry(row)) } @@ -431,14 +587,14 @@ export class PersistentMemoryStore { * `id DESC` is "newest first" with no gaps. */ getRecent(count: number): MemoryEntry[] { - const rows = this.db.prepare( + const rows = this.database().prepare( 'SELECT * FROM memory WHERE (expires_at IS NULL OR expires_at > ?) ORDER BY timestamp DESC, id DESC LIMIT ?' ).all(this.now(), count) as RowData[] return rows.map(row => this.rowToEntry(row)) } getByAuthor(author: string): MemoryEntry[] { - const rows = this.db.prepare( + const rows = this.database().prepare( 'SELECT * FROM memory WHERE author = ? AND (expires_at IS NULL OR expires_at > ?) ORDER BY timestamp DESC, id DESC' ).all(author, this.now()) as RowData[] return rows.map(row => this.rowToEntry(row)) @@ -466,18 +622,20 @@ export class PersistentMemoryStore { * than any caller above means to express. Pass the scope. */ delete(key: string, scope?: MemoryScope): boolean { + const db = this.database() let query = 'DELETE FROM memory WHERE key = ?' const params: SQLQueryBindings[] = [key] if (scope) { query += ' AND scope = ?'; params.push(scope) } - const result = this.db.prepare(query).run(...params) + const result = db.prepare(query).run(...params) return result.changes > 0 } clear(scope?: MemoryScope): number { + const db = this.database() let query = 'DELETE FROM memory' const params: SQLQueryBindings[] = [] if (scope) { query += ' WHERE scope = ?'; params.push(scope) } - const result = this.db.prepare(query).run(...params) + const result = db.prepare(query).run(...params) return result.changes } @@ -492,13 +650,14 @@ export class PersistentMemoryStore { * as what it is — a per-run total, not a lifetime figure. */ getStats(): { total: number; byScope: Record; expired: number; evicted: Record } { - const total = (this.db.prepare('SELECT COUNT(*) as count FROM memory').get() as CountRow).count + const db = this.database() + const total = (db.prepare('SELECT COUNT(*) as count FROM memory').get() as CountRow).count const byScope: Record = {} - const scopes = this.db.prepare('SELECT DISTINCT scope FROM memory').all() as Array<{ scope: string }> + const scopes = db.prepare('SELECT DISTINCT scope FROM memory').all() as Array<{ scope: string }> for (const { scope } of scopes) { - byScope[scope] = (this.db.prepare('SELECT COUNT(*) as count FROM memory WHERE scope = ?').get(scope) as CountRow).count + byScope[scope] = (db.prepare('SELECT COUNT(*) as count FROM memory WHERE scope = ?').get(scope) as CountRow).count } - const expired = (this.db.prepare('SELECT COUNT(*) as count FROM memory WHERE expires_at IS NOT NULL AND expires_at <= ?').get(this.now()) as CountRow).count + const expired = (db.prepare('SELECT COUNT(*) as count FROM memory WHERE expires_at IS NOT NULL AND expires_at <= ?').get(this.now()) as CountRow).count return { total, byScope, expired, evicted: this.getEvictionTotals() } } @@ -536,8 +695,17 @@ export class PersistentMemoryStore { } } + /** + * Close the open handle. Idempotent, and terminal: a later store call throws + * rather than reopening, so `shutdown()` cannot be undone by a recall that + * races teardown. + */ close(): void { - this.db.close() + this.closed = true + this.openPath = null + const db = this.db + this.db = null + db?.close() } } diff --git a/src/orchestrator.ts b/src/orchestrator.ts index 019961f..2bbf6be 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -1093,10 +1093,44 @@ export class NexusOrchestrator { constructor(config?: Partial, messageStoreConfig?: Partial, memoryStoreConfig?: Partial) { this.config = this.mergeConfig(config) this.budget = this.config.budget - this.configManager = new NexusConfigManager(this.config.dashboard, this.config.notifications, this.config.customRoles, this.config.gitFlow, this.config.effort, this.config.budget, this.config.selfHealing) + // The 8th seed is the `memory` block, and it is seeded from + // `memoryStoreConfig` rather than from `config`. `NexusConfig` deliberately + // does not carry a `memory` block — it was removed from that type for doing + // nothing, and re-adding it to feed a seed would resurrect the inert block + // rather than seed a live one. The store's own parameter is where a + // programmatic caller already expresses a DB path and a cap, so the config + // manager is given the same two values: `getConfig().memory.storage` then + // reports the path the store will actually open instead of the default, and + // the two cannot disagree. `enabled` is absent from `memoryStoreConfig` (it + // is a user-facing gate, not a store setting) and so falls through to the + // default `true`, which is what already happens. + this.configManager = new NexusConfigManager( + this.config.dashboard, this.config.notifications, this.config.customRoles, + this.config.gitFlow, this.config.effort, this.config.budget, this.config.selfHealing, + { storage: memoryStoreConfig?.dbPath, maxEntries: memoryStoreConfig?.maxEntries }, + ) this.moduleRegistry = new ModuleRegistry() this.messageStore = new MessageStore(messageStoreConfig) - this.memoryStore = new PersistentMemoryStore(memoryStoreConfig) + // A RESOLVER, not the snapshot. `memoryStoreConfig` still seeds + // `configManager` above, so the constructor parameter keeps working + // unchanged — but it seeds a CONFIG LEVEL, and the store reads that level + // live. Passing the object itself froze the path for the life of the + // process: `getConfig().memory.storage` then reported whatever + // `nexus.jsonc` said while the store went on opening the path it was + // constructed with, and the config modal reported a saved setting the app + // was ignoring. + // + // Deliberately NOT a rebuild inside `reloadConfigFromDisk`, which calls + // `notifyStateChange()` and emits `config:reloaded`: swapping the database + // there would race every in-flight `recallForTask` from a running agent, + // and a reconstructed store would reset `evictedTotals` — the running + // eviction count a surface shows. Resolving at USE time puts the swap on a + // call that is already reading, which is synchronous, so the transition + // completes before any other caller can observe it. + this.memoryStore = new PersistentMemoryStore(() => { + const memory = this.configManager.getConfig().memory + return { dbPath: memory.storage, maxEntries: memory.maxEntries } + }) this.messageRouter = new MessageRouter() // Initialize escalation policy from config selfHealing settings. @@ -4799,6 +4833,24 @@ export class NexusOrchestrator { * one of them is a problem. */ recallForTask(request: RecallRequest): RecallOutcome { + // The `memory.enabled` gate, read through the config manager rather than + // from a second stored copy — so a TUI toggle, a file edit, or a reload + // takes effect on the very next task, with no re-seeding to forget. + // + // HERE and not at the two call sites (`executeTask` ~:2512 and + // `spawnAgent` ~:3742) because this method already owns the rule "an + // enhancement never takes down a task" — the try/catch below is the + // existing expression of it. One guard at the one choke point beats two + // guards at the call sites, which can drift: a third injection point added + // later would silently miss both, and a spy on either call site would go on + // reporting recalls that were never injected. + // + // Returns exactly what the catch below returns, so "off" and "the read + // failed" are the same observable outcome to a caller — which is correct, + // because to a task they are: nothing was injected. + if (!this.configManager.getConfig().memory.enabled) { + return { block: null, matched: 0, shown: 0, characters: 0 } + } try { return runRecall(this.memoryStore, request) } catch (err) { diff --git a/src/tui.tsx b/src/tui.tsx index a839f4d..40889fa 100644 --- a/src/tui.tsx +++ b/src/tui.tsx @@ -2,10 +2,10 @@ import { Plugin } from "@opencode/plugin/tui" import { NexusConfigManager } from "./config" import { - booleanOptions, commitConfigModel, commitConfigText, commitConfigToggle, + configDirtyCount, configFieldsScreen, configIsDirty, configOptionDescription, @@ -14,6 +14,8 @@ import { configPathLabel, configPromptDescription, configSaveUpdate, + configScreenTitle, + configSearchQuery, configScreenOptions, configSelect, configUp, @@ -1275,10 +1277,27 @@ export default Plugin.define({ const discovery = discoverConfig(base) let screen: ConfigScreen = { kind: 'blocks' } + /** + * How many leaves of the whole config have moved — a FUNCTION, never a + * `const`. + * + * `draft` is reassigned on every staged edit, so a count captured when a + * screen was built would be frozen at whatever the draft held then. That + * is a real bug rather than a style preference: the number is what tells a + * user their edit landed, so a frozen one reports the state before it. + */ + const changes = (): number => configDirtyCount(discovery, base, draft) + /** The row for one option. The row's VALUE is the option itself. */ const toDialogRow = (option: ConfigOption): ConfigRow => ({ title: configOptionTitle(option, configSaveScope), - description: configOptionDescription(option, draft, configIsDirty(base, draft)), + description: configOptionDescription( + option, + draft, + base, + configIsDirty(base, draft), + changes() + ), value: option, ...(configOptionDisabled(option) ? { disabled: true } : {}) }) @@ -1312,19 +1331,16 @@ export default Plugin.define({ const runEditor = async (field: ConfigField): Promise => { const back = configFieldsScreen(field) switch (editorFor(field)) { - case 'boolean': { - // TWO ROWS, because the host has no checkbox: `dialog.select` - // resolves a single value out of a list, and a switch in a list can - // only be two rows you pick between. The glyph is in the row's title. - const rows = booleanOptions(configValueAt(draft, field.path) === true) - const chosen = await context.ui.dialog.select({ - title: configPathLabel(field.path), - options: rows.map(toDialogRow) - }) - if (chosen !== undefined) stage(commitConfigToggle(field, chosen)) + case 'boolean': + // Nothing to open. The boolean's OWN screen is the toggle surface: + // `configSelect` answers a `switch` row with `stay`, and the loop + // below handles it in its `case 'stay'` arm. This is what removes + // the second dialog a boolean used to cost — there is no `select` + // here to open, so pressing Enter on a boolean flips it in place and + // comes straight back to the block's list. return back - } case 'number': + case 'path': case 'text': { const typed = await context.ui.dialog.prompt({ title: configPathLabel(field.path), @@ -1373,12 +1389,10 @@ export default Plugin.define({ const options = configScreenOptions(screen, discovery, configSaveScope, field => configValueAt(draft, field.path) ) - const title = - screen.kind === 'blocks' - ? 'Nexus configuration' - : screen.kind === 'fields' - ? `Nexus configuration — ${screen.block}` - : `Nexus configuration — ${configPathLabel(screen.field.path)}` + // The title, including "N changes not written" on the hub, is + // `configScreenTitle`'s decision. It was a ternary here, which is three + // strings this file had no test for. + const title = configScreenTitle(screen, changes()) // `undefined` from the host IS the escape key, and the only way out of a // list. That is why escape needs no binding here: the host owns it, and @@ -1402,8 +1416,30 @@ export default Plugin.define({ // edit in this flow will be written to, and nothing is written // until save, so it is safe to change after staging them. configSaveScope = chosen.scope === 'project' ? 'global' : 'project' + } else if (chosen.kind === 'switch') { + // A boolean is answered in place — there is no second dialog for it + // any more. `commitConfigToggle` stays the ONLY way a boolean + // reaches the draft, and this is its one call site, which is why + // the write is here rather than in `runEditor`: an editor screen + // that answered its own widget would be a second path. + stage(commitConfigToggle(chosen.field, chosen)) } break + case 'ask': { + // The hub's search row. The flow named a screen but no query — the + // only source of a query is this prompt — so a dismissal or an empty + // line leaves the user exactly where they were rather than dropping + // them into a search that matched every setting in the config. + const typed = await context.ui.dialog.prompt({ + title: 'Search settings', + description: 'Block names and setting names', + placeholder: 'e.g. maxTotalCost, or budget' + }) + const query = configSearchQuery(typed) + if (query === undefined) break + screen = { kind: 'search', query } + break + } case 'to': screen = outcome.screen.kind === 'editor' diff --git a/test/config-knobs.test.ts b/test/config-knobs.test.ts index 5147eff..c46b8df 100644 --- a/test/config-knobs.test.ts +++ b/test/config-knobs.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, mock, afterAll } from 'bun:test' import * as realOs from 'node:os' -import { mkdtempSync, readFileSync } from 'node:fs' +import { mkdtempSync, existsSync, readFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { blankNonCode, readDashboardHtml, readSourceFile, interfaceFieldNames } from './helpers/dashboard-page' @@ -339,9 +339,11 @@ describe('retryDelay is file-settable, and round-trips', () => { // `effort` is in this list because this test is the mechanism that put it // there: the block was added to `getSaveableConfig()` and this assertion // failed until the list named it, which is the whole reason the list is - // written out in full instead of being derived. + // written out in full instead of being derived. `memory` is here for the + // same reason, and the failure it would have caused is the quiet one — a + // dropped `storage` leaves the store writing to a file nothing names. expect(Object.keys(written).sort()).toEqual([ - 'budget', 'customRoles', 'dashboard', 'effort', 'gitFlow', 'models', 'notifications', 'selfHealing', + 'budget', 'customRoles', 'dashboard', 'effort', 'gitFlow', 'memory', 'models', 'notifications', 'selfHealing', ]) // The custom role survived, and so did the settings a user would be most @@ -761,6 +763,120 @@ describe('a file-set knob reaches the ORCHESTRATOR, not just the config manager' }) }) +describe('the memory seed does not DELETE the defaults it is meant to fall through to', () => { + /** + * WHY THIS SUITE MISSED IT, stated once so the next test here knows the trap. + * + * Every other test in this file builds a BARE `NexusConfigManager`, so the + * 8th-seed argument is either absent or a real value. The orchestrator is the + * only caller that passes a literal of possibly-absent fields + * (`{ storage: cfg?.dbPath, maxEntries: cfg?.maxEntries }`), and object spread + * copies own enumerable keys INCLUDING `undefined`-valued ones — so + * `{ ...DEFAULT_CONFIG.memory, { storage: undefined } }` does not fall through + * to the default, it DELETES the key. `??` would have fallen through; spread + * does not. That is the whole defect. + */ + it('keeps all three keys when the seed leaves storage and maxEntries undefined', () => { + const manager = new NexusConfigManager( + undefined, undefined, undefined, undefined, undefined, undefined, undefined, + { storage: undefined, maxEntries: undefined }, + ) + const memory = manager.getConfig().memory + // The WHOLE default, key for key — not "storage is a string", which a + // partial assertion would also pass if only `enabled` had survived. + expect(memory).toEqual({ + enabled: true, + storage: expect.any(String), + maxEntries: expect.any(Number), + }) + expect(Object.keys(memory).sort()).toEqual(['enabled', 'maxEntries', 'storage']) + // And they are the VALUES, not merely the keys. + expect((memory.storage as string).length).toBeGreaterThan(0) + expect(memory.maxEntries).toBe(1000) + }) + + it('reports a storage path on the ORCHESTRATOR, matching the store it opened', () => { + // The disagreement this is really about: `getConfig().memory.storage` and + // `memoryStore.path` are two literals of the same default, and an empty + // string on the config side with a real path on the store side is exactly + // what the mirror comments in `config.ts` and `memory-store.ts` exist to + // prevent. + const orchestrator = new NexusOrchestrator() + const memory = orchestrator.configManager.getConfig().memory + expect(typeof memory.storage).toBe('string') + expect((memory.storage as string).length).toBeGreaterThan(0) + expect(memory).toEqual(orchestrator.configManager.getConfig().memory) + }) + + it('gives an ORCHESTRATOR-configured manager the whole three-key default', () => { + // The same seed the orchestrator actually passes — a literal of + // possibly-absent fields — built the way the orchestrator builds it. The + // tests above this one all use a BARE manager, which is precisely the gap + // that let the `undefined`-valued spread delete two keys and go unnoticed: + // the orchestrator is the only caller that passes absent fields on + // purpose, so it is the only caller the seed has to survive. + const manager = new NexusConfigManager( + undefined, undefined, undefined, undefined, undefined, undefined, undefined, + { storage: undefined, maxEntries: undefined }, + ) + expect(manager.getConfig().memory).toEqual({ + enabled: true, + storage: expect.any(String), + maxEntries: 1000, + }) + }) + + it('has the live store resolve to the SAME path the config reports', () => { + // THE AGREEMENT THIS CHANGE EXISTS TO MAKE. `memory.storage` used to be + // frozen into the store's constructor argument while the config manager + // read it live, so the two could diverge the instant a file was edited — + // and the config panel would report the edit as saved while the app used + // the old path. The store now resolves through the config manager, so + // `memoryStore.path` and `getConfig().memory.storage` are the same string + // by construction, and this asserts it on a REAL orchestrator rather than on + // a bare config manager, because the wiring is the thing that can regress. + const orchestrator = new NexusOrchestrator() + const memory = orchestrator.configManager.getConfig().memory + expect(orchestrator.memoryStore.path).toBe(memory.storage) + // And it is the real default rather than an empty string, which is what a + // deleted key looks like from this side. + expect(orchestrator.memoryStore.path).toContain(join('.local', 'share', 'opencode-nexus', 'memory.db')) + + // A session-level override — the storage precedence level, which is the + // level a TUI edit lands on — moves BOTH sides together, with no restart + // and no reconstruction of the store. + const moved = join(tmpdir(), 'nexus-config-knobs-live.db') + // A PARTIAL `memory` block is enough, because `getConfig()` merges key by + // key across the levels rather than replacing the object — which is the same + // reason a TUI edit that toggles one key cannot silently reset the other two. + orchestrator.configManager.updateStorageConfig({ + memory: { ...memory, storage: moved }, + } as never) + expect(orchestrator.configManager.getConfig().memory.storage).toBe(moved) + expect(orchestrator.memoryStore.path).toBe(moved) + // And the store really writes there, so this is a resolution and not a + // string comparison that would pass with a stale handle. + orchestrator.memoryStore.set({ key: 'k', value: 'v', scope: 'temp', author: 'a', confidence: null, tags: [] }) + expect(existsSync(moved)).toBe(true) + orchestrator.memoryStore.close() + try { rmSync(moved, { force: true }) } catch {} + }) + + it('carries all three memory keys into the file a save writes', () => { + // The consequence that made this a data-loss bug rather than a display one: + // `getSaveableConfig()` spreads `{ ...current.memory }`, so a block that lost + // two keys wrote `"memory": {"enabled": true}` — the user's `storage` absent + // from the very file the tool had just written. + const manager = new NexusConfigManager() + manager.updateStorageConfig(manager.getConfig() as never) + expect(manager['getSaveableConfig']().memory).toEqual({ + enabled: true, + storage: expect.any(String), + maxEntries: expect.any(Number), + }) + }) +}) + describe('every preset still loads, and states a whole selfHealing block', () => { it('gives all four presets the three budget keys and all four selfHealing keys', () => { // The presets are the one place a block literal can be silently partial: diff --git a/test/issue-regressions.test.ts b/test/issue-regressions.test.ts index b156071..934c22f 100644 --- a/test/issue-regressions.test.ts +++ b/test/issue-regressions.test.ts @@ -347,7 +347,7 @@ describe('#86 — the config block count is stated once, correctly, and follows it('writes every block NexusFullConfig models, and the docs list the same ones', async () => { const blocks = saveableConfigBlocks() expect([...blocks].sort()).toEqual([ - 'budget', 'customRoles', 'dashboard', 'effort', 'gitFlow', 'models', 'notifications', 'selfHealing', + 'budget', 'customRoles', 'dashboard', 'effort', 'gitFlow', 'memory', 'models', 'notifications', 'selfHealing', ]) // The same answer, from the RUNTIME rather than from the source text, so the diff --git a/test/memory-injection.test.ts b/test/memory-injection.test.ts index 9c6692e..803dcb0 100644 --- a/test/memory-injection.test.ts +++ b/test/memory-injection.test.ts @@ -301,3 +301,44 @@ describe('recall degrades to silence rather than taking a task down', () => { expect(prompted[0] ?? '').toContain('## Task') }) }) + +describe('memory.enabled is a gate that actually gates', () => { + it('injects nothing when the block is off, on the same path that injects when it is on', async () => { + const prompted: string[] = [] + const orchestrator = await makeOrchestrator(prompted) + seedProjectNote(orchestrator, 'file:src/a.ts', 'GATED-MARKER') + + // The control: on by default, so the note is in the prompt. Without this the + // test below would also pass if the path had simply stopped working. + orchestrator['dag'] = stubDAG() + await orchestrator['spawnAndExecute']({ + id: 'n1', task: taskWith({ files: { include: ['src/a.ts'] } }), + dependencies: [], status: 'pending', + } as never) + expect(prompted[0] ?? '').toContain('GATED-MARKER') + + // Now off. The gate is read from the config manager on every call, so + // flipping it here is enough — there is no second stored copy to re-seed, + // which is the property that makes the flag live rather than decorative. + orchestrator.configManager.updateStorageConfig({ memory: { enabled: false } } as never) + + orchestrator['dag'] = stubDAG() + await orchestrator['spawnAndExecute']({ + id: 'n2', task: taskWith({ files: { include: ['src/a.ts'] } }), + dependencies: [], status: 'pending', + } as never) + + const second = prompted[1] ?? '' + expect(second).not.toContain('GATED-MARKER') + // The TASK still went out. A gate that stopped the prompt would be a worse + // bug than a gate that did nothing. + expect(second).toContain('## Task') + + // And the outcome is indistinguishable from a miss, which is the same value + // the existing catch block returns — "off" and "the read failed" look + // identical to a caller, and correctly so: nothing was injected either way. + expect(orchestrator.recallForTask({ files: ['src/a.ts'], text: 'tidy' })) + .toEqual({ block: null, matched: 0, shown: 0, characters: 0 }) + await orchestrator.shutdown() + }) +}) diff --git a/test/memory-store.test.ts b/test/memory-store.test.ts index eb7ec8b..6e9a00a 100644 --- a/test/memory-store.test.ts +++ b/test/memory-store.test.ts @@ -608,4 +608,200 @@ describe('PersistentMemoryStore', () => { store2.close() }) }) + // ── LIVE RESOLUTION ── + // + // `storage` and `maxEntries` were frozen at CONSTRUCTION: the store opened + // `dbPath` in its constructor and held the handle, and `maxEntries` was read + // from a field nobody updated. So a user who edited `nexus.jsonc` saw the new + // value in the config modal — `getConfig().memory` reports it — while the + // running process kept using the old path until restart. That is a knob that + // reports itself saved and does nothing, which is the failure the whole + // `memory` block was rebuilt to remove. + // + // These tests drive the RESOLVER directly rather than through a config file, + // because the resolver is the mechanism: a file-level test would pass whether + // the value arrived by resolver or by a snapshot taken at construction, which + // is precisely the distinction under test. The end-to-end half — the store and + // the config manager agreeing — is in `test/config-knobs.test.ts`. + describe('the config is resolved LIVE, not snapshotted at construction', () => { + it('swaps the open database when the resolved path changes, leaving the old file intact', () => { + const first = join(TEST_DIR, 'first.db') + const second = join(TEST_DIR, 'second.db') + // A mutable view of "what nexus.jsonc says now", which is the shape the + // orchestrator's resolver has over `configManager.getConfig().memory`. + let resolved: Partial = { dbPath: first, maxEntries: 1000 } + const store = new PersistentMemoryStore(() => resolved) + + store.set({ key: 'before', value: 'v1', scope: 'project', author: 'a', confidence: null, tags: [] }) + expect(store.path).toBe(first) + + // The edit. No reconstruction, no reload hook, no restart. + resolved = { dbPath: second, maxEntries: 1000 } + store.set({ key: 'after', value: 'v2', scope: 'project', author: 'a', confidence: null, tags: [] }) + expect(store.path).toBe(second) + + // The live store sees only the new file. Seeing both would mean one + // process was querying two stores and calling it one. + expect(store.getByScope('project').map(e => e.key).sort()).toEqual(['after']) + + // The OLD FILE IS STILL THERE AND STILL COMPLETE. A swap that deleted or + // truncated the previous database would silently destroy a user's notes + // the moment they repointed the setting — the least recoverable way this + // feature could be wrong. + expect(existsSync(first)).toBe(true) + const old = new PersistentMemoryStore({ dbPath: first, maxEntries: 1000 }) + expect(old.getByScope('project').map(e => e.key)).toEqual(['before']) + old.close() + + // And the new one, read by a fresh handle, is where the second write went. + const fresh = new PersistentMemoryStore({ dbPath: second, maxEntries: 1000 }) + expect(fresh.getByScope('project').map(e => e.key)).toEqual(['after']) + fresh.close() + + store.close() + }) + + it('applies a changed maxEntries to the VERY NEXT write, with no reconstruction', () => { + // The cap is a PURE READ — no database swap is involved — so this is the + // cheaper half of the fix and the one that was silently wrong: raising + // `maxEntries` in the config did nothing at all until restart, and + // nothing about the store's identity had changed to hint at it. + const dbPath = join(TEST_DIR, 'cap.db') + let maxEntries = 1000 + const store = new PersistentMemoryStore(() => ({ dbPath, maxEntries })) + + for (let i = 0; i < 5; i++) { + store.set({ key: `k${i}`, value: 'v', scope: 'temp', author: 'a', confidence: null, tags: [] }) + } + expect(store.getByScope('temp')).toHaveLength(5) + expect(store.getEvictionTotals()['temp']).toBeUndefined() + + // Lower the cap below what is already stored. The NEXT write is the one + // that observes it — there is no "rebuild" step in between that a caller + // could be expected to remember to trigger. + maxEntries = 2 + store.set({ key: 'k5', value: 'v', scope: 'temp', author: 'a', confidence: null, tags: [] }) + expect(store.getByScope('temp')).toHaveLength(2) + expect(store.getEvictionTotals()['temp']).toBe(4) + + // Raising it again stops eviction immediately, and does not resurrect + // anything already evicted (it could not: the rows are gone, which is + // what `evictedTotals` is reporting). + maxEntries = 1000 + store.set({ key: 'k6', value: 'v', scope: 'temp', author: 'a', confidence: null, tags: [] }) + expect(store.getByScope('temp')).toHaveLength(3) + store.close() + }) + + it('keeps `project` exempt from a cap that is ALSO resolved live', () => { + // The exemption is load-bearing and the change could have broken it by + // making the cap a per-write read: an exempt scope must still be exempt + // when the cap arrives from a resolver rather than a field. + const maxEntries = 1 + const store = new PersistentMemoryStore(() => ({ dbPath: join(TEST_DIR, 'exempt.db'), maxEntries })) + store.set({ key: 'durable', value: 'keep me', scope: 'project', author: 'a', confidence: null, tags: [] }) + for (let i = 0; i < 5; i++) { + store.set({ key: `junk-${i}`, value: 'x', scope: 'temp', author: 'a', confidence: null, tags: [] }) + } + expect(store.get('durable', 'project')?.value).toBe('keep me') + expect(store.getByScope('temp')).toHaveLength(1) + store.close() + }) + + it('survives concurrent reads and writes ACROSS a path change, losing nothing', () => { + // The safety claim is about MANY calls in flight, because that is the + // shape a rebuild-inside-`reloadConfigFromDisk` design breaks: a store + // closed underneath a reader. Here the swap is a synchronous block with + // no await point in it, so a caller either sees the whole transition or + // none of it. What is asserted is not the happy path — it is that every + // write either landed in one of the two files, and that the file the + // write was issued against is the one holding it. + const first = join(TEST_DIR, 'concurrent-a.db') + const second = join(TEST_DIR, 'concurrent-b.db') + let resolved: Partial = { dbPath: first, maxEntries: 1000 } + const store = new PersistentMemoryStore(() => resolved) + + const paths = [first, second] + const errors: unknown[] = [] + // A write and a read per iteration, with the path flipped every other + // iteration so the change happens repeatedly and mid-sequence rather than + // once at a convenient moment. + const work: Array> = [] + for (let i = 0; i < 24; i++) { + const at = i % 2 === 0 ? first : second + work.push((async () => { + try { + if (i % 4 === 0) { + store.set({ key: `k${i}`, value: 'v', scope: 'project', author: 'a', confidence: null, tags: [] }) + } else { + // A read must not throw whatever the handle is doing, and must + // never see a half-swapped store. + expect(Array.isArray(store.getByScope('project'))).toBe(true) + } + // Every third caller moves the config on, from inside the same + // interleaving. + if (i % 3 === 0) resolved = { dbPath: at, maxEntries: 1000 } + } catch (error) { + errors.push(error) + } + })()) + } + + return Promise.all(work).then(() => { + expect(errors).toEqual([]) + // Nothing was lost: every write is in exactly one of the two files. + const a = new PersistentMemoryStore({ dbPath: paths[0]!, maxEntries: 1000 }) + const b = new PersistentMemoryStore({ dbPath: paths[1]!, maxEntries: 1000 }) + const keys = [...a.getByScope('project'), ...b.getByScope('project')].map(e => e.key) + expect(new Set(keys).size).toBe(keys.length) + for (let i = 0; i < 24; i += 4) expect(keys).toContain(`k${i}`) + a.close() + b.close() + store.close() + }) + }) + + it('keeps the LAST GOOD database open when a new path cannot be opened', () => { + // The order inside `database` earns its keep here. Opening first and + // closing second means a path that blows up throws with the previous + // database still serving reads, rather than leaving a store with no + // handle at all — a user who mistypes `storage` gets an error, not a + // memory feature that has quietly stopped working. + const good = join(TEST_DIR, 'good.db') + const store = new PersistentMemoryStore(() => ({ dbPath: good, maxEntries: 1000 })) + store.set({ key: 'k', value: 'v', scope: 'project', author: 'a', confidence: null, tags: [] }) + + // A directory, not a file: `new Database` on this path fails. + store['resolveConfig'] = () => ({ dbPath: TEST_DIR, maxEntries: 1000 }) + expect(() => store.getByScope('project')).toThrow() + // Still readable, from the path that worked. + store['resolveConfig'] = () => ({ dbPath: good, maxEntries: 1000 }) + expect(store.getByScope('project').map(e => e.key)).toEqual(['k']) + store.close() + }) + + it('still accepts a fixed config object, so existing callers are unaffected', () => { + // The union, not a new required parameter: `new + // PersistentMemoryStore({ dbPath })` is how the store is constructed + // everywhere else in the repository, and the type is re-exported from + // `index.ts` for embedders who construct it themselves. + const dbPath = join(TEST_DIR, 'fixed.db') + const store = new PersistentMemoryStore({ dbPath, maxEntries: 2 }) + expect(store.path).toBe(dbPath) + store.set({ key: 'a', value: '1', scope: 'temp', author: 'a', confidence: null, tags: [] }) + store.set({ key: 'b', value: '2', scope: 'temp', author: 'a', confidence: null, tags: [] }) + store.set({ key: 'c', value: '3', scope: 'temp', author: 'a', confidence: null, tags: [] }) + expect(store.getByScope('temp').map(e => e.key)).toEqual(['c', 'b']) + store.close() + }) + + it('reports the RESOLVED path from `path`, not the one it was built with', () => { + // `path` is what a surface prints to tell a user where to look, so it has + // to be the path in effect. Returning the construction-time one is how a + // diagnostic tool sends someone to the wrong file. + const store = new PersistentMemoryStore(() => ({ dbPath: join(TEST_DIR, 'live.db'), maxEntries: 1000 })) + expect(store.path).toBe(join(TEST_DIR, 'live.db')) + store.close() + }) + }) }) diff --git a/test/tui-config-flow.test.ts b/test/tui-config-flow.test.ts index c93b06a..66d552a 100644 --- a/test/tui-config-flow.test.ts +++ b/test/tui-config-flow.test.ts @@ -15,11 +15,14 @@ mock.module('node:os', () => ({ ...realOs, default: realOs, homedir: () => SANDB const { NexusConfigManager } = await import('../src/config') const { + blockMatches, blockOptions, booleanOptions, commitConfigModel, commitConfigText, commitConfigToggle, + configBlockDirtyCount, + configDirtyCount, configFieldsScreen, configIsDirty, configOptionDescription, @@ -29,14 +32,20 @@ const { configPromptDescription, configSaveUpdate, configScreenOptions, + configScreenTitle, + configSearchQuery, configSelect, configUp, configValueAt, describeConfigValue, discoverConfig, editorFor, + fieldMatches, fieldOptions, + filterConfigBlocks, + filterConfigFields, parseConfigNumber, + searchOptions, setConfigValue } = await import('../src/config-flow') @@ -117,9 +126,18 @@ function reachableScreens( const key = (screen: ConfigScreen): string => screen.kind === 'blocks' ? 'blocks' - : screen.kind === 'fields' - ? `fields:${screen.block}` - : `editor:${screen.field.path.join('.')}` + // The query is part of the ID, and it has to be: two different queries are + // two different screens with two different result lists, and keying on the + // kind alone would collide them in `seen` — the search would then be + // explored ONCE with whichever query the walk happened to try first, and + // the state-space count below would be quietly too small. That is a silent + // under-exploration: the walk returns a smaller `seen` and every assertion + // about it still passes. + : screen.kind === 'search' + ? `search:${screen.query}` + : screen.kind === 'fields' + ? `fields:${screen.block}` + : `editor:${screen.field.path.join('.')}` const seen = new Map([[key(initial), initial]]) const queue: ConfigScreen[] = [initial] @@ -215,15 +233,37 @@ describe('config flow discovery', () => { } }) + it('classifies `memory.storage` as a path and never as a model, in the REAL config', () => { + // The memory block was added after the model sniff was narrowed, and the + // narrowing is what stopped `memory.storage` — a sqlite file path, slashes + // and all — from opening the provider picker and writing `provider/id` over + // the store's location. Those two changes are independent and both must hold: + // a later "simplify" that re-applies the sniff to every string would restore + // that bug, and the existing tests would not catch it because they feed the + // classifier a hand-written object rather than the real config, where + // `memory.storage` may or may not happen to contain a slash. + const discovery = realDiscovery() + const storage = fieldAt(discovery, 'memory.storage') + expect(storage.kind).toBe('path') + expect(editorFor(storage)).toBe('path') + // Read from the real config, so a default path with no slash is still + // asserted: the KIND comes from the block plus the label, never the value. + expect(editorFor(storage)).not.toBe('model') + // And the widening is scoped: the model blocks still get the picker. + expect(editorFor(fieldAt(discovery, 'models.coder'))).toBe('model') + }) + it('gives every real field an editor, so no row opens nothing', () => { const { fields } = realDiscovery() const withoutEditor = fields.filter(field => editorFor(field) === 'none').map(field => configPathLabel(field.path)) expect(withoutEditor).toEqual([]) - // And the four widgets between them are all reachable in the shipped config, + // And the five widgets between them are all reachable in the shipped config, // so a flow that quietly stopped offering one of them would fail here rather - // than in a report a user files. + // than in a report a user files. `path` is reachable because + // `memory.storage` is one — it edits as free text today, and is named + // separately so the path editor has a `case` of its own to grow into. const used = new Set(fields.map(editorFor)) - expect([...used].sort()).toEqual(['boolean', 'model', 'number', 'text']) + expect([...used].sort()).toEqual(['boolean', 'model', 'number', 'path', 'text']) }) it('reports no value it has no editor for, rather than hiding it', () => { @@ -257,9 +297,78 @@ describe('config flow discovery', () => { expect(blockOptions(discovery, 'global').some(o => o.kind === 'block' && o.block.name === 'telemetry')).toBe(true) }) - it('gives a model reference the picker and a plain string the text field', () => { + it('gives everything under `models` the picker, whatever it holds', () => { + // A DELIBERATE behaviour change, and the one edge of the name-based rule + // that loses information. The old shape test let a `models` entry without a + // slash — a typo, or a note a user parked in the model block — fall back to a + // text field. Under a name-based rule `models.*` is a wildcard, so such a + // value now opens the picker. That is the right way round: `models` is an + // OPEN keyed block whose every leaf is a model reference by definition, the + // value a user would want corrected in is a provider picker rather than a + // text box, and a text editor there would be the shape of value a + // `provider/id` typo most often takes. A string OUTSIDE `models` is + // unaffected — see the test below. const { fields } = discoverConfig({ models: { coder: 'anthropic/claude-sonnet-4-6', note: 'hello' } }) - expect(fields.map(field => field.kind)).toEqual(['model', 'text']) + expect(fields.map(field => field.kind)).toEqual(['model', 'model']) + }) + + it('does not read a slash as a model reference outside the model fields', () => { + // The bug this guards: EVERY string containing a `/` was classified as a + // model, so a relative `memory.storage` opened the grouped provider picker + // and picking a model there wrote `provider/id` into the memory DB path. + // Silent, total, and it destroyed the store it was editing. Classification + // is by the field's ADDRESS, so everywhere else a slash is just a slash. + const { fields } = discoverConfig({ + memory: { storage: './.opencode/memory.db' }, + gitFlow: { note: 'feature/branch-name' }, + export: { path: 'dist/bundle.js' }, + models: { coder: 'anthropic/claude-sonnet-4-6' }, + }) + const byPath = Object.fromEntries(fields.map(f => [configPathLabel(f.path), f.kind])) + expect(byPath['memory.storage']).toBe('path') + expect(byPath['gitFlow.note']).toBe('text') + expect(byPath['export.path']).toBe('text') + // Still the picker where it belongs — scoping must not have disarmed it. + expect(byPath['models.coder']).toBe('model') + }) + + it('gives a custom role\'s own `model` the picker, and its PROSE a text field', () => { + // `getModelForRole` really does read `customRoles[].model` (see + // `test/custom-roles-config.test.ts`), so a text field there would demote a + // working setting to second class by its block's name alone. + // + // And the other half, which is the test whose ABSENCE let the class survive + // for as long as it did: every earlier fixture used `prompt: 'p'` — one + // letter, no slash — so the misclassification it needed a role prompt to + // have could not show up. A real role prompt names a file, and a real + // display name is often `Team/Role`. Those are prose, and classifying them + // as models is the silent-total failure: `runEditor`'s `case 'model'` opens + // the provider picker and `commitConfigModel` then writes `provider/id` + // over the user's instructions. + const { fields } = discoverConfig({ + customRoles: [ + { + name: 'qa', + prompt: 'Read src/a.ts and report what it does', + displayName: 'QA/Engineer', + model: 'openai/gpt-5-mini', + }, + ], + }) + const byPath = Object.fromEntries(fields.map(f => [configPathLabel(f.path), f.kind])) + expect(byPath['customRoles[0].model']).toBe('model') + expect(byPath['customRoles[0].prompt']).toBe('text') + expect(byPath['customRoles[0].displayName']).toBe('text') + expect(byPath['customRoles[0].name']).toBe('text') + }) + + it('edits `memory.storage` as a path, and the real config reaches that editor', () => { + const field = fieldAt(realDiscovery(), 'memory.storage') + expect(field.kind).toBe('path') + // A path is still typed by hand, so the editor is the free-text prompt — + // named apart from `text` so a path-validating widget can replace the case + // without also changing every other string field. + expect(editorFor(field)).toBe('path') }) it('treats an array element as an ordinary field of its block, with no array code', () => { @@ -320,10 +429,19 @@ describe('config flow reachability', () => { const discovery = discoverConfig(config) const reached = reachableScreens({ kind: 'blocks' }, discovery, config, 'global') - // The search covered the WHOLE state space, not a subset of it. One hub, one - // list per block, one editor per field — and the count is derived from the - // discovery rather than written down, so a block or a field that produced no - // screen at all fails here instead of quietly shrinking the search. + // SEARCH IS NOT REACHABLE BY THE WALK, stated directly so the guarantee is + // not resting on a count. A `search` screen's `query` does not exist until the + // host's prompt produces one, and the hub's search row yields `ask` rather than + // `to` precisely so the flow cannot navigate to a screen with a query it never + // got. This is the assertion that pins it. + expect(reached.some(screen => screen.kind === 'search')).toBe(false) + // The walk covers the WHOLE rest of the state space. One hub, one list per + // block, one editor per field — derived from the discovery rather than written + // down, so a block or a field that produced no screen at all fails here + // instead of quietly shrinking the search. The `+1` the search screen would + // add is absent, which is the count-level echo of the assertion above: a + // change that made `ask` a `to` — the fix that would let a blank prompt drop a + // user into a list of the whole config — also breaks this number. expect(reached).toHaveLength(1 + discovery.blocks.length + discovery.fields.length) for (const screen of reached) { @@ -434,60 +552,110 @@ describe('config flow escape', () => { } }) - it('a boolean editor\'s two rows are the only things it can answer with, and neither navigates', () => { - // The answer to a switch is a VALUE, not a destination. If choosing a row + it('a boolean editor\'s switch is the only answer it can give, and answering does not navigate', () => { + // The answer to a switch is a VALUE, not a destination. If choosing the row // moved the flow, the value would have nowhere to go. + // + // BOTH DIRECTIONS, which the previous version of this did not do: it passed + // `true` alone, so a `switch` row that was correct when the key was on and + // wrong when it was off passed the whole suite. The widget is symmetric and + // has to be tested symmetrically. const field = fieldAt(discovery, 'notifications.enabled') const editor: ConfigScreen = { kind: 'editor', field } - for (const row of booleanOptions(true)) { - expect(configSelect(editor, row).kind).toBe('stay') + for (const current of [true, false]) { + const rows = booleanOptions(field, current) + expect(rows.map(row => row.kind)).toEqual(['switch', 'back']) + // The switch stays put… + expect(configSelect(editor, rows[0])).toEqual({ kind: 'stay' }) + // …and the way out leaves, which is what a user pressing escape means. + expect(configSelect(editor, rows[1])).toEqual({ + kind: 'to', + screen: { kind: 'fields', block: 'notifications' } + }) } }) }) -// ── A boolean is two rows ─────────────────────────────────────────── +// ── A boolean is one keystroke ────────────────────────────────────── +// +// The describe title used to be "as two rows in a single-select list", which +// described a design this file no longer has. A stale title is how a reader ends +// up trusting an implementation that has been replaced: they read the heading, +// assume the shape matches, and skip the body. -describe('a boolean, as two rows in a single-select list', () => { - it('offers both values with the glyph in the row title, whichever is current', () => { +describe('a boolean, as one switch and a way out', () => { + const discovery = realDiscovery() + const field = fieldAt(discovery, 'notifications.enabled') + + it('offers one switch to the other value, with the glyph beside the direction in words', () => { for (const current of [true, false]) { - const rows = booleanOptions(current) - // Two rows, both values, both glyphs — and no third row and no disabled - // one, because the host has no checkbox to put here instead. + const rows = booleanOptions(field, current) + // Two rows, and the second is `back` — not the other value. There is + // exactly one boolean widget in this file. expect(rows).toHaveLength(2) - expect(rows.map(row => (row.kind === 'toggle' ? row.value : null)).sort()).toEqual([false, true]) - const titles = rows.map(row => configOptionTitle(row, 'global')) - expect(titles.filter(title => title.includes('✅'))).toHaveLength(1) - expect(titles.filter(title => title.includes('☐'))).toHaveLength(1) - // The current row is named, and it is the one the host opens on, so the - // user does not have to read both rows to know where they are. - expect(titles.filter(title => title.includes('(current)'))).toHaveLength(1) - // And neither is refused: choosing the row a key is already on is how a - // user says "off, and I mean off". + expect(rows.map(row => row.kind)).toEqual(['switch', 'back']) + const switched = rows[0] + if (switched.kind !== 'switch') throw new Error('the first row is not the switch') + // The row stands for the OTHER value, whichever that is. + expect(switched.value).toBe(!current) + // And it names the field it writes, so a row that reached the adapter by + // any route still says which path it belongs to. + expect(switched.field).toBe(field) + + const title = configOptionTitle(switched, 'global') + // The glyph is RECONDITIONED on the direction rather than asserted + // present: `✅` on "Switch to Disabled" would be a lie told in an emoji, + // and the old assertion (`titles` contains exactly one of each) would have + // passed on that lie because the wrong glyph was still somewhere in the list. + if (switched.value) { + expect(title).toBe('✅ Switch to Enabled') + expect(configOptionDescription(switched, realConfig(), realConfig(), false)).toContain('Turns it on') + } else { + expect(title).toBe('☐ Switch to Disabled') + expect(configOptionDescription(switched, realConfig(), realConfig(), false)).toContain('Turns it off') + } + // Both rows are answerable. Disabling the switch on the value a key is + // already on would make "off, and I mean off" unreachable — the row is + // never greyed, and `back` is the escape. expect(rows.map(configOptionDisabled)).toEqual([false, false]) } }) - it('writes the value the chosen row stands for, not the inverse of what was there', () => { - const discovery = realDiscovery() - const field = fieldAt(discovery, 'notifications.enabled') - const rows = booleanOptions(false) - - const off = commitConfigToggle(field, rows.find(r => r.kind === 'toggle' && r.value === false) as never) - expect(off).toEqual({ kind: 'accepted', field, value: false }) + it('writes the value the switch stands for, and flipping it twice returns it', () => { + // BOTH DIRECTIONS again, and the assertion is the row's own `value` — the + // commit takes what the row says rather than inverting the draft, which is + // what made the old two-row version fragile. + for (const current of [true, false]) { + const commit = commitConfigToggle(field, booleanOptions(field, current)[0]) + expect(commit).toEqual({ kind: 'accepted', field, value: !current }) + } - // And the row that IS current is a no-op change, not a flip. Inverting here - // would make "choose the row I am already on" mean the opposite of what it - // says, which is the one thing a two-row switch must not do. - const current = commitConfigToggle(field, rows.find(r => r.kind === 'toggle' && r.value === true) as never) - const after = setConfigValue(realConfig(), field.path, current.kind === 'accepted' ? current.value : undefined) - expect(configValueAt(after, field.path)).toBe(true) + // REVERSIBILITY. The old second half of this test — "the row that IS current + // writes a no-op, not a flip" — is GONE, and its absence is a consequence of + // the redesign rather than a loss of coverage: no row stands for the current + // value any more, so the hazard that test guarded (a commit that inverted + // whatever was in the draft, turning "leave it alone" into "flip it") is no + // longer expressible. What replaced it is the property that actually matters + // about a switch: a second flip gives the value back, and it does so through + // the same rows and the same commit. + const after = setConfigValue(realConfig(), field.path, !false) + const backAgain = booleanOptions(field, configValueAt(after, field.path) === true)[0] + if (backAgain.kind !== 'switch') throw new Error('the first row is not the switch') + expect(backAgain.value).toBe(false) + const restored = commitConfigToggle(field, backAgain) + expect(restored).toEqual({ kind: 'accepted', field, value: false }) }) - it('refuses an answer that is not one of its own two rows', () => { - // The two rows are the only things a boolean editor can return, and a commit + it('refuses an answer that is not one of its own rows', () => { + // The switch is the only thing a boolean editor can return, and a commit // that accepted anything else would be a write to a path nothing chose. - const field = fieldAt(realDiscovery(), 'notifications.enabled') - expect(() => commitConfigToggle(field, { kind: 'back' })).toThrow(/toggle row/) + // Every hub row is tried, not just `back`: the search and no-results rows + // are the ones a later change could plausibly have let through here. + for (const kind of ['back', 'save', 'search', 'no-results'] as const) { + const option = + kind === 'no-results' ? ({ kind, query: 'x' } as const) : ({ kind } as const) + expect(() => commitConfigToggle(field, option as never)).toThrow(/switch row/) + } }) }) @@ -622,14 +790,19 @@ describe('config flow rows', () => { const field = fieldAt(discovery, 'budget.maxTotalCost') const row = fieldOptions(discovery, 'budget').find(o => o.kind === 'field' && o.field === field) expect(row).toBeDefined() - expect(configOptionDescription(row as never, config, false)).toBe('10') + // `base` is the third argument and is REQUIRED, which is the point: an + // omitted one defaulted to `draft` would report "0 changed" on every row + // rather than failing to compile. + expect(configOptionDescription(row as never, config, config, false)).toBe('10') // The staged value shows immediately, marked, because the only other place it - // appears is a toast that has already gone. + // appears is a toast that has already gone. The `25 *` string is UNCHANGED + // by the dirty-count work — a field row gets no number, because 0 and 1 are + // not information and `25 * 1 changed` is noise. const staged = setConfigValue(config, field.path, 25) - expect(configOptionDescription(row as never, staged, true)).toBe('25 *') + expect(configOptionDescription(row as never, staged, config, true)).toBe('25 *') // …and an UNSTAGED value is not marked, so a `*` always means "not on disk". - expect(configOptionDescription(row as never, staged, false)).toBe('25') + expect(configOptionDescription(row as never, staged, config, false)).toBe('25') }) it('says plainly that nothing is written until Save, on the two rows that decide it', () => { @@ -643,9 +816,10 @@ describe('config flow rows', () => { // cannot check afterwards. expect(configOptionTitle(scopeRow as never, 'project')).toContain('.opencode/nexus.jsonc') expect(configOptionTitle(scopeRow as never, 'global')).toContain('~/.config/opencode/nexus.jsonc') - expect(configOptionDescription(scopeRow as never, realConfig(), false)).toMatch(/Save/) - expect(configOptionDescription(saveRow as never, realConfig(), true)).toMatch(/Write/) - expect(configOptionDescription(closeRow as never, realConfig(), true)).toMatch(/Nothing is written/) + const config = realConfig() + expect(configOptionDescription(scopeRow as never, config, config, false)).toMatch(/Save/) + expect(configOptionDescription(saveRow as never, config, config, true)).toMatch(/Write/) + expect(configOptionDescription(closeRow as never, config, config, true)).toMatch(/Nothing is written/) }) it('warns under a number prompt and stays quiet under a text one', () => { @@ -669,13 +843,16 @@ describe('config flow rows', () => { const customRoles = blockOptions(discovery, 'global').find( o => o.kind === 'block' && o.block.name === 'customRoles' ) - expect(configOptionDescription(customRoles as never, realConfig(), false)).toMatch(/no settings here yet/) + const config = realConfig() + expect(configOptionDescription(customRoles as never, config, config, false)).toMatch(/no settings here yet/) const budget = blockOptions(discovery, 'global').find(o => o.kind === 'block' && o.block.name === 'budget') // A SHAPE, not a count. The exact number is a fact about `budget` and would // fail for a reason no user would recognise when a key is added; what matters // is that a block with fields counts them and a block with none says so. - expect(configOptionDescription(budget as never, realConfig(), false)).toMatch(/^\d+ settings$/) + // UNCHANGED by the dirty badge: a CLEAN block's description is still exactly + // `N settings`, so this regex has to keep passing verbatim. + expect(configOptionDescription(budget as never, config, config, false)).toMatch(/^\d+ settings$/) }) }) @@ -821,6 +998,290 @@ describe('config flow save path', () => { }) }) +// ── Search ────────────────────────────────────────────────────────── +// +// Search is ONE-SHOT, and that is a property of the host rather than a choice: +// `ui.dialog.select` owns the lifetime of its list and exposes no `onInput`, so +// a persistent filter field would have to keep the query in the ADAPTER, where +// no test can drive it and where a decision would live that this file does not +// own. A prompt, then a list, keeps the query in a `ConfigScreen` the flow +// decided — so `searchOptions` is headlessly testable at all, which is the only +// reason this can be tested rather than hoped for. + +describe('config flow search', () => { + const config = realConfig() + const discovery = discoverConfig(config) + + it('treats an empty or absent prompt as no search, so a blank query is never a query', () => { + // The load-bearing one. An empty string reaching `searchOptions` would match + // every row — turning "search" into a second copy of the hub, reachable by + // pressing Enter on an empty prompt. + for (const raw of ['', ' ', '\t', undefined]) { + expect(configSearchQuery(raw)).toBeUndefined() + } + // Trimmed, not merely tested for emptiness: a query with a stray leading + // space should match the same rows as one without. + expect(configSearchQuery(' budget ')).toBe('budget') + // And the same rule reaches the matchers, so a blank query matches nothing + // rather than everything. + const field = fieldAt(discovery, 'budget.maxTotalCost') + expect(fieldMatches(field, '')).toBe(false) + }) + + it('matches a field by its label OR its dotted path, case-insensitively, as a substring', () => { + const cost = fieldAt(discovery, 'budget.maxTotalCost') + // By label… + expect(fieldMatches(cost, 'maxTotalCost')).toBe(true) + // …by path, which is not redundant: `enabled` exists in eight blocks, so a + // qualified search is the only one that can identify a single setting. + expect(fieldMatches(cost, 'budget.maxTotal')).toBe(true) + // Case is folded on BOTH sides, and it is a SUBSTRING rather than a prefix: + // a user who types `cost` means `maxTotalCost`. + expect(fieldMatches(cost, 'MAXtotalcost')).toBe(true) + expect(fieldMatches(cost, 'cost')).toBe(true) + // And something that is not there is not there. No fuzzy matching: a wrong + // row at the top of a list whose job is addressing one key is a config value + // changed to something the user never read. + expect(fieldMatches(cost, 'costt')).toBe(false) + expect(fieldMatches(cost, 'gateway')).toBe(false) + }) + + it('matches a block by its own name OR by any field it holds', () => { + const budget = discovery.blocks.find(block => block.name === 'budget') + if (budget === undefined) throw new Error('no budget block') + expect(blockMatches(budget, 'budget')).toBe(true) + // A field's name is enough to find its block, which is the "I remember the + // setting but not where it lives" case search exists for. + expect(blockMatches(budget, 'maxTotalCost')).toBe(true) + expect(blockMatches(budget, 'gateway')).toBe(false) + // Case-insensitivity is `blockMatches`' OWN fold, and the field tests do not + // reach it: a block is matched by its NAME as well as by its fields, so the + // name fold has to hold separately or a block search is case-sensitive while + // a field search is not — the same word, two answers, depending on where the + // user looked for it. + expect(blockMatches(budget, 'BUDGET')).toBe(true) + expect(blockMatches(budget, 'BuDgEt')).toBe(true) + }) + + it('answers an empty search with `back` alone, not a row quoting nothing', () => { + // `searchOptions` is exported and tested on its own terms, so it cannot rely + // on every caller having normalised at the boundary. The live path does + // (`configSearchQuery`, and the adapter before it), but a function called + // directly with `''` would otherwise match nothing and report "no results" + // while quoting the empty string: a dead end manufactured out of a question + // nobody asked. + for (const empty of ['', ' ', '\t\n ']) { + expect(searchOptions(discovery, empty)).toEqual([{ kind: 'back' }]) + } + }) + + it('leads with the matching FIELD, and lists a block only if none of its fields matched', () => { + const cost = fieldAt(discovery, 'budget.maxTotalCost') + const rows = searchOptions(discovery, 'maxTotalCost') + const budgetBlock = discovery.blocks.find(block => block.name === 'budget') as never + + // The FIELD is first, and this ordering is the feature: the result opens the + // setting's editor directly, with no second Enter to descend through the + // block. Searching for a block name is the case that still needs the hop. + expect(rows[0]).toEqual({ kind: 'field', field: cost }) + // `budget` matched (it holds `maxTotalCost`) but is NOT listed as a block + // row: the same setting appearing twice under two descriptions is a list + // nobody can learn to read. + expect(rows.some(row => row.kind === 'block' && (row.block as { name: string }).name === 'budget')).toBe(false) + expect(rows).not.toContainEqual(budgetBlock) + // Exactly one way out, always. + expect(rows.filter(row => row.kind === 'back')).toHaveLength(1) + }) + + it('lists a block as a block only when none of its fields matched', () => { + // The no-double-listing rule needs a block whose fields do NOT match, and in + // the real config no such block exists for a real query: every field's PATH + // carries the block's name, so a query naming a block matches all of that + // block's fields and the block is correctly suppressed in favour of the + // fields. So this is pinned on a fixture built for the case rather than + // wished for on the real config — a `gateway` block whose fields are called + // `enabled` and `port`, exactly the shape that makes the two rules differ. + const gateway = discoverConfig({ gateway: { enabled: true, port: 1 } }) + const rows = searchOptions(gateway, 'gateway') + + // `gateway.enabled`'s PATH contains "gateway", so it matches and leads… + expect(rows[0].kind).toBe('field') + // …and the block is not listed beside it. + expect(rows.filter(row => row.kind === 'block')).toHaveLength(0) + + // A query naming a block that HAS fields can never produce a block row, and + // that is a property of the addressing rather than a gap: every field's + // dotted path begins with its own block name, so the block always matches + // and so do all of its fields. Which means the block row is reachable in + // exactly one shape — a block with NO fields, which `customRoles` is on a + // fresh install. That is the case worth pinning: without it, "search for + // customRoles" would return only a `no-results` row, which is a lie, because + // the block exists and has nothing in it yet. + const empty = discoverConfig({ customRoles: [] }) + const rows2 = searchOptions(empty, 'customRoles') + expect(rows2.map(row => row.kind)).toEqual(['block', 'back']) + expect(blockMatches(empty.blocks[0], 'customRoles')).toBe(true) + expect(filterConfigFields(empty, 'customRoles')).toEqual([]) + expect(filterConfigBlocks(empty, 'customRoles').map(b => b.name)).toEqual(['customRoles']) + }) + + it('answers a search that matched NOTHING with a quotable row that goes somewhere', () => { + const rows = searchOptions(discovery, 'zzznotasetting') + // A `select` with zero options is a dialog the user can see but not leave, + // and it looks exactly like a failed load. So the miss is a ROW. + expect(rows.map(row => row.kind)).toEqual(['no-results', 'back']) + const miss = rows[0] + if (miss.kind !== 'no-results') throw new Error('the miss is not a no-results row') + + // It QUOTES the query: a user who does not remember what they typed cannot + // otherwise tell a typo from a missing setting. + expect(configOptionTitle(miss, 'global')).toBe('No setting matches "zzznotasetting"') + // And it says what the search COVERS, so a user who assumed it searched + // values learns that it does not. + expect(configOptionDescription(miss, config, config, false)).toBe( + 'Searches block names and setting names' + ) + // Selectable, and it leads somewhere real. + expect(configSelect({ kind: 'search', query: 'zzznotasetting' }, miss)).toEqual({ + kind: 'to', + screen: { kind: 'blocks' } + }) + }) + + it('is one escape from the hub, and an unmatched search screen is still escapable', () => { + // The "at most two escapes to the hub" guarantee. A search that had to be + // left before a result could be opened would push the deepest screen in the + // flow to three. + expect(configUp({ kind: 'search', query: 'budget' })).toEqual({ kind: 'blocks' }) + // `search` is a screen rather than an instant for the same reason the editor + // is: "can the user get out of here" has to be answerable, and it cannot be + // answered for something that is not a state. + expect(configScreenTitle({ kind: 'search', query: 'budget' }, 0)).toBe( + 'Nexus configuration — search "budget"' + ) + }) + + it('reaches the search screen from the hub\'s SECOND row, before any block', () => { + // Positional, and load-bearing: the hub lists every config key, so it is + // past a screenful on any real config. A search row below the blocks would + // sit under the fold on every device — a feature nobody finds. + const hub = blockOptions(discovery, 'global') + expect(hub[0].kind).toBe('scope') + expect(hub[1].kind).toBe('search') + const firstBlock = hub.findIndex(option => option.kind === 'block') + expect(firstBlock).toBeGreaterThan(1) + expect(hub.filter(option => option.kind === 'search')).toHaveLength(1) + }) + + it('asks the host for a query rather than navigating to a search it has not been given', () => { + // The `ask` outcome exists so the flow never invents a state containing a + // string it never decided. An empty-query screen would be a real screen that + // matched EVERYTHING, so a dismissed prompt would drop the user into a list + // of the whole config. + const outcome = configSelect({ kind: 'blocks' }, { kind: 'search' }) + expect(outcome.kind).toBe('ask') + if (outcome.kind !== 'ask') throw new Error('the search row did not ask') + expect(outcome.screen).toEqual({ kind: 'search', query: '' }) + }) +}) + +// ── Dirty badges ──────────────────────────────────────────────────── + +describe('config flow dirty counts', () => { + const config = realConfig() + const discovery = discoverConfig(config) + + it('counts LEAVES, not changed paths, so one write cannot look like a sweep', () => { + const budget = discovery.blocks.find(block => block.name === 'budget') + if (budget === undefined) throw new Error('no budget block') + const base = { budget: { maxTotalCost: 10, warnAt: 5, dailyCap: 1 } } + const block = { name: 'budget', fields: discoverConfig(base).fields } + expect(configBlockDirtyCount(block, base, base)).toBe(0) + // One key of three. A count of changed PATHS would also say 1 here, so the + // distinguishing case is below. + const one = setConfigValue(base, ['budget', 'maxTotalCost'], 25) + expect(configBlockDirtyCount(block, base, one)).toBe(1) + // All three, through one nested write. Counting paths would report 1. + const all = setConfigValue(one, ['budget', 'dailyCap'], 3) + expect(configBlockDirtyCount(block, base, all)).toBe(2) + }) + + it('sums the whole config, so the title counts every staged edit and not just one block', () => { + const staged = setConfigValue(config, ['budget', 'maxTotalCost'], 25) + const withTwo = setConfigValue(staged, ['notifications', 'enabled'], false) + expect(configDirtyCount(discovery, config, config)).toBe(0) + expect(configDirtyCount(discovery, config, staged)).toBe(1) + expect(configDirtyCount(discovery, config, withTwo)).toBe(2) + }) + + it('marks a dirty block with a NUMBER and a glyph, and leaves a clean one exactly as it was', () => { + // Readable WITHOUT colour: the glyph alone is a mark with no magnitude, and + // "which block did I change" is a question a count answers. + const budget = blockOptions(discovery, 'global').find(o => o.kind === 'block' && o.block.name === 'budget') + const clean = configOptionDescription(budget as never, config, config, true) + expect(clean).toMatch(/^\d+ settings$/) + + const staged = setConfigValue(config, ['budget', 'maxTotalCost'], 25) + const dirty = configOptionDescription(budget as never, staged, config, true) + expect(dirty).toMatch(/^\d+ settings {2}1 changed {2}\*$/) + // A block nobody touched is untouched, so a `*` still means "not on disk". + const models = blockOptions(discovery, 'global').find(o => o.kind === 'block' && o.block.name === 'models') + expect(configOptionDescription(models as never, staged, config, true)).toMatch(/^\d+ settings$/) + }) + + it('reports Save as clean only when the draft is clean, and never counts', () => { + // NO NUMBER ON THIS ROW, and that is the assertion. `configSaveUpdate` hands + // the WHOLE draft to `updateStorageConfig`, which rewrites nine blocks and + // shadows the project and global files for every key it names — so a count + // here described what the user TOUCHED while the sentence said it described + // what is WRITTEN. The counts that remain (the title, the `*` markers) are + // honestly about edits. + const save = blockOptions(discovery, 'global').find(o => o.kind === 'save') + const staged = setConfigValue(config, ['budget', 'maxTotalCost'], 25) + const withTwo = setConfigValue(staged, ['notifications', 'enabled'], false) + + expect(configOptionDescription(save as never, config, config, false, 0)).toBe('No changes to write') + // One edit and two edits read identically: the row does not claim a count. + expect(configOptionDescription(save as never, staged, config, true, 1)).toBe( + 'Write the staged changes to disk' + ) + expect(configOptionDescription(save as never, withTwo, config, true, 2)).toBe( + 'Write the staged changes to disk' + ) + // A supplied `0` is still honoured rather than being overridden by + // dirtiness — the caller's count wins when it says zero. + expect(configOptionDescription(save as never, withTwo, config, true, 0)).toBe('No changes to write') + // With NO count supplied, the count-free wording stands rather than a + // fabricated `Write 0 changes to disk` on a draft that is dirty. + expect(configOptionDescription(save as never, staged, config, true)).toBe( + 'Write the staged changes to disk' + ) + }) + + it('leaves Save enabled when nothing has changed, and says so on purpose', () => { + // DELIBERATE, and pinned because it is the kind of decision a later change + // makes for a good-looking reason. Greying Save out on a clean draft looks + // tidier and is a worse dialog: the row is how a user discovers that Save is + // the only thing that writes, and a user who opens the dialog to find out + // where their settings go is not served by an inert button. Pressing it is + // an answer, not an error. + const save = blockOptions(discovery, 'global').find(o => o.kind === 'save') + expect(configOptionDisabled(save as never)).toBe(false) + }) + + it('reports the change count in the hub title, and only when there is one to report', () => { + expect(configScreenTitle({ kind: 'blocks' }, 0)).toBe('Nexus configuration') + expect(configScreenTitle({ kind: 'blocks' }, 3)).toBe( + 'Nexus configuration — 3 changes not written' + ) + // The other screens are unchanged by this — a number on every window would + // be a number nobody could ignore and a user could not escape. + expect(configScreenTitle({ kind: 'fields', block: 'budget' }, 3)).toBe( + 'Nexus configuration — budget' + ) + }) +}) + // ── The adapter, which no harness can drive ───────────────────────── // // There is no TUI harness in this repository, so `runConfigFlow` in @@ -890,6 +1351,16 @@ describe('the config flow adapter in src/tui.tsx', () => { // call, so there is no second path to a value. expect(body).toContain('commitConfigText') expect(body.match(/setConfigValue\(/g)?.length).toBe(1) + // …and a BOOLEAN goes through `commitConfigToggle`, AS A CALL. Nothing else + // would catch an adapter that wrote a boolean straight into `draft`: + // `setConfigValue` would still appear exactly once, the `stay` arm would still + // be there, and the flow would still be internally consistent — but the one + // function that says "this is the only path a boolean reaches the draft" would + // be bypassed, and its guard (which refuses anything that is not a `switch` + // row) with it. A bare `toContain` would pass on that adapter, since the + // helper would still be imported and merely unused, so the SHAPE is what is + // asserted: the commit is handed to `stage`. + expect(body).toMatch(/stage\(\s*commitConfigToggle\(/) }) it('reaches every config screen through the tested navigation, not its own', async () => { @@ -905,7 +1376,10 @@ describe('the config flow adapter in src/tui.tsx', () => { // choices, and they are left alone deliberately: counting them is what the // first version of this test did, and it counted six and meant nothing. expect(body).toContain('editorFor') - expect(body).toMatch(/switch\s*\(\s*editorFor\(field\)\s*\)/) + // The parameter's NAME is not the claim — the call being the switch's + // discriminant is — so it is matched as a word rather than spelled out, which + // is also what a rename of the adapter's local would do. + expect(body).toMatch(/switch\s*\(\s*editorFor\(\s*\w+\s*\)\s*\)/) expect(body).not.toMatch(/field\.kind\s*===\s*'(boolean|number|model)'/) expect(body).not.toMatch(/field\.kind\s*===\s*"/) }) @@ -918,7 +1392,10 @@ describe('the config flow adapter in src/tui.tsx', () => { * to not have, reintroduced through the one function no test can execute. The * guard is positional: `commitConfigText` produces the commit, the `rejected` * branch decides what happens to it, and only then does `stage` write. Moving - * the write above the check, or deleting the check, fails here. + * the write above the check, or deleting the check, fails here. The ORDER is + * the real thing being proven, so the offsets are found by shape — the + * adapter's local variable names are not part of the claim and must not be + * load-bearing in a test. * * WHAT THIS STILL CANNOT CATCH, stated rather than implied: a rejection * disabled by a condition no static reading can evaluate — `if (false)`, or a @@ -930,12 +1407,13 @@ describe('the config flow adapter in src/tui.tsx', () => { */ it('handles a rejected commit before it can reach the draft', async () => { const body = await flow() - const commit = body.indexOf('commitConfigText(field, typed)') + const commit = body.search(/commitConfigText\(/) const rejects = body.indexOf("if (commit.kind === 'rejected')") const alerts = body.indexOf('ui.dialog.alert') - const stages = body.indexOf('stage(commit)') + const stages = body.search(/stage\(\s*commit\s*\)/) expect(commit).toBeGreaterThan(-1) + expect(stages).toBeGreaterThan(-1) expect(rejects).toBeGreaterThan(commit) expect(alerts).toBeGreaterThan(rejects) expect(stages).toBeGreaterThan(rejects)