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)