diff --git a/package.json b/package.json index 331a5cb49..df5c2b666 100644 --- a/package.json +++ b/package.json @@ -156,6 +156,7 @@ "commands": "./dist/oclif/commands", "hooks": { "init": [ + "./dist/oclif/hooks/init/block-autoupdate-when-off", "./dist/oclif/hooks/init/block-command-update-npm", "./dist/oclif/hooks/init/welcome", "./dist/oclif/hooks/init/update-notifier", diff --git a/src/agent/infra/settings/agent-settings-snapshot.ts b/src/agent/infra/settings/agent-settings-snapshot.ts index ca41fb483..310c83d46 100644 --- a/src/agent/infra/settings/agent-settings-snapshot.ts +++ b/src/agent/infra/settings/agent-settings-snapshot.ts @@ -37,7 +37,9 @@ export async function loadAgentSettingsSnapshot(client: ITransportClient): Promi const response = await client.requestWithAck(SettingsEvents.LIST) const values: Record = {} for (const item of response.items) { - values[item.key] = item.current + // Boolean descriptors round-trip the same wire shape but no agent + // consumer reads them today; ship-when-needed in a later ticket. + if (typeof item.current === 'number') values[item.key] = item.current } snapshot = values diff --git a/src/oclif/commands/settings/get.ts b/src/oclif/commands/settings/get.ts index d1396afce..34ad47a6f 100644 --- a/src/oclif/commands/settings/get.ts +++ b/src/oclif/commands/settings/get.ts @@ -74,15 +74,14 @@ export default class SettingsGet extends Command { } private printTextBlock(item: SettingsItemDTO): void { - const current = renderValue(item, item.current) - const defaultStr = renderValue(item, item.default) - const min = renderValue(item, item.min) - const max = renderValue(item, item.max) - this.log(item.key) - this.log(` current: ${current}`) - this.log(` default: ${defaultStr}`) - this.log(` range: ${min}-${max}`) + this.log(` current: ${renderValue(item, item.current)}`) + this.log(` default: ${renderValue(item, item.default)}`) + if (item.type === 'integer' && item.min !== undefined && item.max !== undefined) { + const range = `${renderInteger(item, item.min)}-${renderInteger(item, item.max)}` + this.log(` range: ${range}`) + } + this.log(` scope: ${item.scope ?? 'global'}`) } @@ -104,7 +103,12 @@ export default class SettingsGet extends Command { } } -function renderValue(item: SettingsItemDTO, value: number): string { +function renderValue(item: SettingsItemDTO, value: boolean | number): string { + if (typeof value === 'boolean') return value ? 'true' : 'false' + return renderInteger(item, value) +} + +function renderInteger(item: SettingsItemDTO, value: number): string { if (item.unit === 'ms') return formatDuration(value) return formatCount(value) } diff --git a/src/oclif/commands/settings/index.ts b/src/oclif/commands/settings/index.ts index 5637ed27f..9375c30e6 100644 --- a/src/oclif/commands/settings/index.ts +++ b/src/oclif/commands/settings/index.ts @@ -9,14 +9,15 @@ import {formatCount, formatDuration} from '../../../shared/utils/format-duration import {type DaemonClientOptions, formatConnectionError, withDaemonRetry} from '../../lib/daemon-client.js' import {writeJsonResponse} from '../../lib/json-response.js' -type CategoryName = 'concurrency' | 'llm' | 'task-history' +type CategoryName = 'concurrency' | 'llm' | 'task-history' | 'updates' -const CATEGORY_ORDER: readonly CategoryName[] = ['concurrency', 'llm', 'task-history'] +const CATEGORY_ORDER: readonly CategoryName[] = ['concurrency', 'llm', 'task-history', 'updates'] const CATEGORY_HEADERS: Readonly> = { concurrency: 'CONCURRENCY', llm: 'LLM', 'task-history': 'TASK HISTORY', + updates: 'UPDATES', } const OTHER_HEADER = 'OTHER' @@ -113,14 +114,20 @@ function formatRow(item: SettingsItemDTO): string { return ` ${pad(item.key, 40)} ${pad(current, 7)} (default ${defaultStr})${''.padEnd(Math.max(0, 8 - defaultStr.length))} ${range}` } -function renderValue(item: SettingsItemDTO, value: number): string { +function renderValue(item: SettingsItemDTO, value: boolean | number): string { + if (typeof value === 'boolean') return value ? 'true' : 'false' + return renderInteger(item, value) +} + +function renderInteger(item: SettingsItemDTO, value: number): string { if (item.unit === 'ms') return formatDuration(value) return formatCount(value) } function renderRange(item: SettingsItemDTO): string { - const min = renderValue(item, item.min) - const max = renderValue(item, item.max) + if (item.type !== 'integer' || item.min === undefined || item.max === undefined) return '' + const min = renderInteger(item, item.min) + const max = renderInteger(item, item.max) const base = `${min}-${max}` if (item.key === 'llm.requestTimeoutMs') return `${base}, max loop budget` return base diff --git a/src/oclif/commands/settings/reset.ts b/src/oclif/commands/settings/reset.ts index 99df66f34..57d00803f 100644 --- a/src/oclif/commands/settings/reset.ts +++ b/src/oclif/commands/settings/reset.ts @@ -73,10 +73,8 @@ export default class SettingsReset extends Command { success: true, }) } else { - this.log( - `Setting reset: ${args.key} back to default (${renderValue(descriptor, descriptor.default)}). ` + - 'Run `brv restart` to apply.', - ) + const base = `Setting reset: ${args.key} back to default (${renderValue(descriptor, descriptor.default)}).` + this.log(descriptor.restartRequired ? `${base} Run \`brv restart\` to apply.` : base) } return @@ -99,7 +97,8 @@ export default class SettingsReset extends Command { } } -function renderValue(item: SettingsItemDTO, value: number): string { +function renderValue(item: SettingsItemDTO, value: boolean | number): string { + if (typeof value === 'boolean') return value ? 'true' : 'false' if (item.unit === 'ms') return formatDuration(value) return formatCount(value) } diff --git a/src/oclif/commands/settings/set.ts b/src/oclif/commands/settings/set.ts index e4f273307..bdc33eb8a 100644 --- a/src/oclif/commands/settings/set.ts +++ b/src/oclif/commands/settings/set.ts @@ -22,7 +22,11 @@ const DURATION_RE = /\d+\s*(?:ms|s|m|h)/i export default class SettingsSet extends Command { public static args = { key: Args.string({description: 'Settings key to write', required: true}), - value: Args.string({description: 'New value (integer for count keys, duration like 30m / 1h 30m / 1800000 for ms keys)', required: true}), + value: Args.string({ + description: + 'New value (integer for count keys, duration like 30m / 1h 30m / 1800000 for ms keys, boolean true/false/on/off/1/0/yes/no for boolean keys)', + required: true, + }), } public static description = 'Update one settings value. Changes apply after `brv restart`.' @@ -90,7 +94,8 @@ export default class SettingsSet extends Command { success: true, }) } else { - this.log(`Setting saved: ${args.key} = ${parsed.display}. Run \`brv restart\` to apply.`) + const base = `Setting saved: ${args.key} = ${parsed.display}.` + this.log(response.restartRequired ? `${base} Run \`brv restart\` to apply.` : base) } return @@ -114,7 +119,7 @@ export default class SettingsSet extends Command { protected async writeSetting( key: string, - value: unknown, + value: boolean | number, options?: DaemonClientOptions, ): Promise { return withDaemonRetry( @@ -126,14 +131,41 @@ export default class SettingsSet extends Command { } type ParseResult = - | {readonly display: string; readonly kind: 'ok'; readonly value: number} + | {readonly display: string; readonly kind: 'ok'; readonly value: boolean | number} | {readonly kind: 'error'; readonly message: string} +const BOOLEAN_TOKENS = new Map([ + ['0', false], + ['1', true], + ['false', false], + ['no', false], + ['off', false], + ['on', true], + ['true', true], + ['yes', true], +]) + +const BOOLEAN_TOKENS_HINT = 'true, false, on, off, 1, 0, yes, no' + function parseValue(descriptor: SettingsItemDTO, raw: string): ParseResult { + if (descriptor.type === 'boolean') return parseAsBoolean(descriptor, raw) if (descriptor.unit === 'ms') return parseAsDuration(descriptor, raw) return parseAsCount(descriptor, raw) } +function parseAsBoolean(descriptor: SettingsItemDTO, raw: string): ParseResult { + const lowered = raw.trim().toLowerCase() + const value = BOOLEAN_TOKENS.get(lowered) + if (value === undefined) { + return { + kind: 'error', + message: `${descriptor.key} expected boolean (${BOOLEAN_TOKENS_HINT}), got '${raw}'.`, + } + } + + return {display: String(value), kind: 'ok', value} +} + function parseAsDuration(descriptor: SettingsItemDTO, raw: string): ParseResult { const parsed = parseDuration(raw) if (typeof parsed === 'number') { diff --git a/src/oclif/hooks/init/block-autoupdate-when-off.ts b/src/oclif/hooks/init/block-autoupdate-when-off.ts new file mode 100644 index 000000000..25944360c --- /dev/null +++ b/src/oclif/hooks/init/block-autoupdate-when-off.ts @@ -0,0 +1,37 @@ +import type {Hook} from '@oclif/core' + +import {checkForUpdatesSetting} from '../../lib/check-for-updates-setting.js' + +export type BlockAutoupdateWhenOffDeps = { + argv: readonly string[] + commandId: string | undefined + exitFn: (code: number) => never +} + +/** + * Short-circuits `brv update --autoupdate` (the background invocation + * `@oclif/plugin-update` spawns on a debounce) when the user has set + * `update.checkForUpdates` to `false`. Exits silently with code 0 so + * plugin-update sees a normal-looking termination rather than an error + * it might retry or log. + * + * The user-initiated `brv update` (no `--autoupdate` flag) is **not** + * blocked — that remains the user's manual escape hatch. + */ +export function handleBlockAutoupdateWhenOff(deps: BlockAutoupdateWhenOffDeps): void { + if (deps.commandId !== 'update') return + if (!deps.argv.includes('--autoupdate')) return + if (checkForUpdatesSetting()) return + deps.exitFn(0) +} + +const hook: Hook<'init'> = function (opts): Promise { + handleBlockAutoupdateWhenOff({ + argv: opts.argv, + commandId: opts.id, + exitFn: process.exit, + }) + return Promise.resolve() +} + +export default hook diff --git a/src/oclif/hooks/init/update-notifier.ts b/src/oclif/hooks/init/update-notifier.ts index 356f554ee..c0551edfc 100644 --- a/src/oclif/hooks/init/update-notifier.ts +++ b/src/oclif/hooks/init/update-notifier.ts @@ -4,6 +4,8 @@ import {confirm} from '@inquirer/prompts' import {execSync, spawn} from 'node:child_process' import updateNotifier from 'update-notifier' +import {checkForUpdatesSetting} from '../../lib/check-for-updates-setting.js' + /** * Check interval for update notifications (1 hour) */ @@ -46,7 +48,29 @@ export const isNpmGlobalInstall = (execSyncFn: typeof execSync): boolean => { } /** - * Core update notification logic, extracted for testability + * Resolves whether the init hook should run the update check at all. + * + * Returns `false` when any of the following holds: + * - The `update.checkForUpdates` setting is explicitly `false`. + * - The command being invoked is itself `update` (anti-recursion). + * - `BRV_ENV` is `development` (dev installs handle updates manually). + * + * Returns `true` otherwise (default behaviour). + */ +export function shouldRunUpdateCheck(args: {commandId: string | undefined}): boolean { + if (args.commandId === 'update') return false + if (process.env.BRV_ENV === 'development') return false + return checkForUpdatesSetting() +} + +/** + * Core update notification logic, extracted for testability. + * + * Interactive flow only: shows a confirm prompt, then runs `npm update -g` + * + `brv restart` if the user types `y`. There is **no** unattended / + * automatic execution path — that was deliberately rejected by the team + * (May 2026). When the user types `n` (or the prompt is unavailable), + * the hook just notifies and returns. */ export async function handleUpdateNotification(deps: UpdateNotifierDeps): Promise { const {confirmPrompt, execSyncFn, exitFn, isNpmGlobalInstalled, isTTY, log, notifier} = deps @@ -89,7 +113,9 @@ export async function handleUpdateNotification(deps: UpdateNotifierDeps): Promis } } -const hook: Hook<'init'> = async function (): Promise { +const hook: Hook<'init'> = async function (opts): Promise { + if (!shouldRunUpdateCheck({commandId: opts.id})) return + const pkgInfo = {name: this.config.name, version: this.config.version} const notifier = updateNotifier({pkg: pkgInfo, updateCheckInterval: UPDATE_CHECK_INTERVAL_MS}) const isNpmGlobalInstalled = isNpmGlobalInstall(execSync) diff --git a/src/oclif/lib/check-for-updates-setting.ts b/src/oclif/lib/check-for-updates-setting.ts new file mode 100644 index 000000000..a1c70c6fd --- /dev/null +++ b/src/oclif/lib/check-for-updates-setting.ts @@ -0,0 +1,41 @@ +import {existsSync, readFileSync} from 'node:fs' +import {join} from 'node:path' + +import {getGlobalDataDir} from '../../server/utils/global-data-path.js' + +const SETTINGS_KEY = 'update.checkForUpdates' +const SETTINGS_FILENAME = 'settings.json' + +/** + * Synchronous read of the user's `update.checkForUpdates` setting. + * + * Default is `true` (update check enabled). Returns `false` only when the + * persisted settings file explicitly contains + * `{values: {"update.checkForUpdates": false}}`. + * + * Lives in `oclif/lib/` because the only consumers today are oclif init + * hooks that must run before the daemon connection is established and + * therefore cannot read the setting via the `settings:list` transport + * event. Path resolution reuses `getGlobalDataDir()` so the helper lands + * on the exact same file the daemon writes to. + */ +export function checkForUpdatesSetting(): boolean { + try { + const path = join(getGlobalDataDir(), SETTINGS_FILENAME) + if (!existsSync(path)) return true + + const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) + if (!isRecord(parsed)) return true + if (!isRecord(parsed.values)) return true + + const value = parsed.values[SETTINGS_KEY] + if (typeof value !== 'boolean') return true + return value + } catch { + return true + } +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} diff --git a/src/server/constants.ts b/src/server/constants.ts index 5cef98aa4..e882f31e7 100644 --- a/src/server/constants.ts +++ b/src/server/constants.ts @@ -127,7 +127,14 @@ export const REVIEW_BACKUPS_DIR = 'review-backups' // User-configurable operational settings (global, restart-required) export const SETTINGS_FILE = 'settings.json' -export const SETTINGS_SCHEMA_VERSION = '1' +// Bumped from '1' to '2' alongside the boolean-descriptor work: v2 files +// persist `{[key]: number | boolean}`. v1 files are migrated forward on +// first read by `FileSettingsStore`; the migration is idempotent. +export const SETTINGS_SCHEMA_VERSION = '2' + +// Default for the boolean `update.checkForUpdates` setting. ON by default; users +// flip to false to disable the startup update-notifier hook entirely. +export const UPDATE_CHECK_FOR_UPDATES_DEFAULT = true // Default wall-clock budget for the agentic loop (`llm.iterationBudgetMs`). // Slow local-LLM users override via `brv settings set llm.iterationBudgetMs `. diff --git a/src/server/core/domain/entities/settings.ts b/src/server/core/domain/entities/settings.ts index 1f785a05e..11a11bfe3 100644 --- a/src/server/core/domain/entities/settings.ts +++ b/src/server/core/domain/entities/settings.ts @@ -4,6 +4,7 @@ import { AGENT_MAX_CONCURRENT_TASKS, AGENT_POOL_MAX_SIZE, TASK_HISTORY_DEFAULT_MAX_ENTRIES, + UPDATE_CHECK_FOR_UPDATES_DEFAULT, } from '../../../constants.js' /** @@ -11,7 +12,7 @@ import { * and TUI render output (uppercased). Web docs / WebUI consume this * field to render the same groupings independently of key naming. */ -export type SettingCategory = 'concurrency' | 'llm' | 'task-history' +export type SettingCategory = 'concurrency' | 'llm' | 'task-history' | 'updates' /** * Value-kind for dispatch between the duration formatter / parser @@ -23,31 +24,50 @@ export type SettingCategory = 'concurrency' | 'llm' | 'task-history' export type SettingUnit = 'count' | 'ms' /** - * Descriptor for a single user-configurable setting. - * Defaults reference the existing constants module so a constant change - * automatically updates the setting's default. + * Fields shared by every descriptor regardless of value type. Specific + * value-kind variants extend this with their own `default` shape and any + * type-specific constraints (range for integers, etc). */ -export type SettingDescriptor = { +type BaseSettingDescriptor = { readonly category?: SettingCategory - readonly default: number readonly description: string readonly key: string + readonly restartRequired: boolean +} + +export type IntegerSettingDescriptor = BaseSettingDescriptor & { + readonly default: number readonly max: number readonly min: number - readonly restartRequired: true readonly type: 'integer' readonly unit?: SettingUnit } +export type BooleanSettingDescriptor = BaseSettingDescriptor & { + readonly default: boolean + readonly type: 'boolean' +} + +/** + * Descriptor for a single user-configurable setting. Discriminated on + * `type` so consumers narrow with a single check before reading + * type-specific fields (`min`/`max` on integers, etc). + * + * Defaults reference the existing constants module so a constant change + * automatically updates the setting's default. + */ +export type SettingDescriptor = BooleanSettingDescriptor | IntegerSettingDescriptor + /** * View of one setting: the key, the user's current override (or the default - * if none is set), and the registered default. + * if none is set), and the registered default. Carries the union of value + * shapes; consumers narrow on the corresponding descriptor's `type`. */ export type SettingItem = { - readonly current: number - readonly default: number + readonly current: boolean | number + readonly default: boolean | number readonly key: string - readonly restartRequired: true + readonly restartRequired: boolean } /** @@ -62,6 +82,7 @@ export const SETTINGS_KEYS = { LLM_ITERATION_BUDGET_MS: 'llm.iterationBudgetMs', LLM_REQUEST_TIMEOUT_MS: 'llm.requestTimeoutMs', TASK_HISTORY_MAX_ENTRIES: 'taskHistory.maxEntries', + UPDATE_CHECK_FOR_UPDATES: 'update.checkForUpdates', } as const export const SETTINGS_REGISTRY: readonly SettingDescriptor[] = [ @@ -117,6 +138,14 @@ export const SETTINGS_REGISTRY: readonly SettingDescriptor[] = [ restartRequired: true, type: 'integer', }, + { + category: 'updates', + default: UPDATE_CHECK_FOR_UPDATES_DEFAULT, + description: 'Check for brv updates at startup and notify when one is available', + key: SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES, + restartRequired: false, + type: 'boolean', + }, ] export function findSettingDescriptor(key: string): SettingDescriptor | undefined { diff --git a/src/server/core/interfaces/storage/i-settings-store.ts b/src/server/core/interfaces/storage/i-settings-store.ts index c29846879..e9f6ad96a 100644 --- a/src/server/core/interfaces/storage/i-settings-store.ts +++ b/src/server/core/interfaces/storage/i-settings-store.ts @@ -12,7 +12,7 @@ export type SettingsStartupSnapshot = { * Daemon startup logs this once; all values fall back to defaults. */ readonly parseError?: string - readonly values: Readonly> + readonly values: Readonly> } /** diff --git a/src/server/infra/daemon/settings-bootstrap.ts b/src/server/infra/daemon/settings-bootstrap.ts index bc2b57c23..3afa78577 100644 --- a/src/server/infra/daemon/settings-bootstrap.ts +++ b/src/server/infra/daemon/settings-bootstrap.ts @@ -1,4 +1,4 @@ -import type {ISettingsStore} from '../../core/interfaces/storage/i-settings-store.js' +import type {ISettingsStore, SettingsStartupSnapshot} from '../../core/interfaces/storage/i-settings-store.js' import { AGENT_MAX_CONCURRENT_TASKS, @@ -45,8 +45,13 @@ export async function bootstrapSettings(options: BootstrapSettingsOptions): Prom } return { - agentMaxConcurrentTasks: snapshot.values[SETTINGS_KEYS.AGENT_POOL_MAX_CONCURRENT_TASKS] ?? AGENT_MAX_CONCURRENT_TASKS, - agentPoolMaxSize: snapshot.values[SETTINGS_KEYS.AGENT_POOL_MAX_SIZE] ?? AGENT_POOL_MAX_SIZE, - taskHistoryMaxEntries: snapshot.values[SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES] ?? TASK_HISTORY_DEFAULT_MAX_ENTRIES, + agentMaxConcurrentTasks: readNumber(snapshot, SETTINGS_KEYS.AGENT_POOL_MAX_CONCURRENT_TASKS, AGENT_MAX_CONCURRENT_TASKS), + agentPoolMaxSize: readNumber(snapshot, SETTINGS_KEYS.AGENT_POOL_MAX_SIZE, AGENT_POOL_MAX_SIZE), + taskHistoryMaxEntries: readNumber(snapshot, SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES, TASK_HISTORY_DEFAULT_MAX_ENTRIES), } } + +function readNumber(snapshot: SettingsStartupSnapshot, key: string, fallback: number): number { + const value = snapshot.values[key] + return typeof value === 'number' ? value : fallback +} diff --git a/src/server/infra/storage/file-settings-store.ts b/src/server/infra/storage/file-settings-store.ts index c0fbc19ab..46ca7a34b 100644 --- a/src/server/infra/storage/file-settings-store.ts +++ b/src/server/infra/storage/file-settings-store.ts @@ -112,7 +112,15 @@ export class FileSettingsStore implements ISettingsStore { const overrides = await this.readOverrides() const proposed = {...overrides, [key]: validatedValue} - const violations = this.validator.validateCoupling(proposed) + // `validateCoupling` only covers integer-keyed invariants today, so + // hand it the numeric subset; boolean overrides are passed through + // unchanged when persisted. + const numericProposed: Record = {} + for (const [k, v] of Object.entries(proposed)) { + if (typeof v === 'number') numericProposed[k] = v + } + + const violations = this.validator.validateCoupling(numericProposed) if (violations.length > 0) { throw new InvalidSettingValueError(key, value, violations[0].reason) } @@ -124,7 +132,7 @@ export class FileSettingsStore implements ISettingsStore { return join(this.baseDir, SETTINGS_FILE) } - private async readOverrides(): Promise> { + private async readOverrides(): Promise> { const raw = await this.readRawValues() const {valid} = this.validator.partition(raw) return {...valid} @@ -157,12 +165,24 @@ export class FileSettingsStore implements ISettingsStore { return {kind: 'corrupt', parseError: 'expected top-level JSON object'} } - if (parsed.values === undefined) return {kind: 'ok', values: {}} - if (!isRecord(parsed.values)) { + let valuesMap: Record + if (parsed.values === undefined) { + valuesMap = {} + } else if (isRecord(parsed.values)) { + valuesMap = {...parsed.values} + } else { return {kind: 'corrupt', parseError: "expected object at '.values'"} } - return {kind: 'ok', values: {...parsed.values}} + // Forward-migrate older schemas. Idempotent: re-reading a file already + // on the current version does not rewrite. Unknown future versions are + // left untouched so a newer brv's v3 file isn't silently downgraded by + // an older brv that only knows about v2. + if (parsed.version === '1') { + await this.writeFile({values: valuesMap, version: SETTINGS_SCHEMA_VERSION}) + } + + return {kind: 'ok', values: valuesMap} } private async writeFile(file: SettingsFile): Promise { diff --git a/src/server/infra/storage/settings-validator.ts b/src/server/infra/storage/settings-validator.ts index 41ffa17b7..c37fc2762 100644 --- a/src/server/infra/storage/settings-validator.ts +++ b/src/server/infra/storage/settings-validator.ts @@ -1,4 +1,8 @@ -import type {SettingDescriptor} from '../../core/domain/entities/settings.js' +import type { + BooleanSettingDescriptor, + IntegerSettingDescriptor, + SettingDescriptor, +} from '../../core/domain/entities/settings.js' import {findSettingDescriptor, SETTINGS_KEYS} from '../../core/domain/entities/settings.js' @@ -26,7 +30,7 @@ export class InvalidSettingValueError extends Error { export type PartitionedSettings = { readonly invalid: ReadonlyArray<{readonly key: string; readonly reason: string; readonly value: unknown}> - readonly valid: Readonly> + readonly valid: Readonly> } export type CouplingViolation = { @@ -52,7 +56,7 @@ export class SettingsValidator { * log a warning about. */ public partition(record: Record): PartitionedSettings { - const valid: Record = {} + const valid: Record = {} const invalid: Array<{key: string; reason: string; value: unknown}> = [] for (const [key, value] of Object.entries(record)) { @@ -78,7 +82,7 @@ export class SettingsValidator { // a violation demotes every key participating in the rule back to // its registered default; surfaced via `invalid` so the daemon // startup loader can log one warning per demoted key. - for (const violation of this.validateCoupling(valid)) { + for (const violation of this.validateCoupling(numericSubset(valid))) { for (const key of violation.keys) { if (key in valid) { invalid.push({key, reason: violation.reason, value: valid[key]}) @@ -92,9 +96,10 @@ export class SettingsValidator { /** * Validates a single key/value pair. Throws on unknown key or invalid value. - * Returns the coerced numeric value on success. + * Returns the coerced value on success (integer for integer descriptors, + * boolean for boolean descriptors). */ - public validate(key: string, value: unknown): number { + public validate(key: string, value: unknown): boolean | number { const descriptor = this.validateKey(key) return this.validateAgainst(descriptor, value) } @@ -131,25 +136,51 @@ export class SettingsValidator { return descriptor } - private validateAgainst(descriptor: SettingDescriptor, value: unknown): number { - if (typeof value !== 'number' || !Number.isInteger(value)) { - throw new InvalidSettingValueError( - descriptor.key, - value, - `expected integer, got ${describeType(value)}`, - ) - } + private validateAgainst(descriptor: SettingDescriptor, value: unknown): boolean | number { + if (descriptor.type === 'boolean') return validateBoolean(descriptor, value) + return validateInteger(descriptor, value) + } +} - if (value < descriptor.min || value > descriptor.max) { - throw new InvalidSettingValueError( - descriptor.key, - value, - `value ${value} is outside allowed range [${descriptor.min}, ${descriptor.max}]`, - ) - } +function validateInteger(descriptor: IntegerSettingDescriptor, value: unknown): number { + if (typeof value !== 'number' || !Number.isInteger(value)) { + throw new InvalidSettingValueError( + descriptor.key, + value, + `expected integer, got ${describeType(value)}`, + ) + } + + if (value < descriptor.min || value > descriptor.max) { + throw new InvalidSettingValueError( + descriptor.key, + value, + `value ${value} is outside allowed range [${descriptor.min}, ${descriptor.max}]`, + ) + } - return value + return value +} + +function validateBoolean(descriptor: BooleanSettingDescriptor, value: unknown): boolean { + if (typeof value !== 'boolean') { + throw new InvalidSettingValueError( + descriptor.key, + value, + `expected boolean, got ${describeType(value)}`, + ) } + + return value +} + +function numericSubset(values: Readonly>): Record { + const result: Record = {} + for (const [key, value] of Object.entries(values)) { + if (typeof value === 'number') result[key] = value + } + + return result } function describeType(value: unknown): string { diff --git a/src/server/infra/transport/handlers/settings-handler.ts b/src/server/infra/transport/handlers/settings-handler.ts index d5b6ff197..d48410cce 100644 --- a/src/server/infra/transport/handlers/settings-handler.ts +++ b/src/server/infra/transport/handlers/settings-handler.ts @@ -68,9 +68,12 @@ export class SettingsHandler { this.transport.onRequest( SettingsEvents.SET, async (data) => { + const typeError = checkValueType(data.key, data.value) + if (typeError !== undefined) return {error: typeError, ok: false} + try { await this.store.set(data.key, data.value) - return {ok: true, restartRequired: true} + return {ok: true, restartRequired: restartRequiredFor(data.key)} } catch (error) { return {error: errorToDTO(error, data.key, data.value), ok: false} } @@ -82,7 +85,7 @@ export class SettingsHandler { async (data) => { try { await this.store.reset(data.key) - return {ok: true, restartRequired: true} + return {ok: true, restartRequired: restartRequiredFor(data.key)} } catch (error) { return {error: errorToDTO(error, data.key), ok: false} } @@ -91,6 +94,52 @@ export class SettingsHandler { } } +function restartRequiredFor(key: string): boolean { + return findSettingDescriptor(key)?.restartRequired ?? true +} + +/** + * Pre-validates `value`'s runtime type against the descriptor for `key`. + * + * Returns a structured `invalid_value_type` error when the type does not + * match. Returns `undefined` either on match or when the key has no + * descriptor at all — in the second case the store's `UnknownSettingKeyError` + * surfaces as `unknown_key` through the existing error path, so this helper + * intentionally does not duplicate that check. + * + * Range, coupling, and fractional-number violations are left to the store's + * validator and still surface as `invalid_value`. + */ +function checkValueType(key: string, value: boolean | number): SettingsErrorDTO | undefined { + const descriptor = findSettingDescriptor(key) + if (descriptor === undefined) return undefined + + const got = typeof value + if (descriptor.type === 'integer' && got !== 'number') { + return { + code: 'invalid_value_type', + expected: 'integer', + got, + key, + message: `expected integer for '${key}', got ${got}`, + value, + } + } + + if (descriptor.type === 'boolean' && got !== 'boolean') { + return { + code: 'invalid_value_type', + expected: 'boolean', + got, + key, + message: `expected boolean for '${key}', got ${got}`, + value, + } + } + + return undefined +} + function toItemDTO(item: SettingItem): SettingsItemDTO { const descriptor = findSettingDescriptor(item.key) if (descriptor === undefined) { @@ -100,19 +149,22 @@ function toItemDTO(item: SettingItem): SettingsItemDTO { return descriptorToDTO(descriptor, item.current) } -function descriptorToDTO(descriptor: SettingDescriptor, current: number): SettingsItemDTO { +function descriptorToDTO(descriptor: SettingDescriptor, current: boolean | number): SettingsItemDTO { const dto: SettingsItemDTO = { current, default: descriptor.default, description: descriptor.description, key: descriptor.key, - max: descriptor.max, - min: descriptor.min, - restartRequired: true, + restartRequired: descriptor.restartRequired, type: descriptor.type, } if (descriptor.category !== undefined) dto.category = descriptor.category - if (descriptor.unit !== undefined) dto.unit = descriptor.unit + if (descriptor.type === 'integer') { + dto.min = descriptor.min + dto.max = descriptor.max + if (descriptor.unit !== undefined) dto.unit = descriptor.unit + } + return dto } diff --git a/src/shared/transport/events/settings-events.ts b/src/shared/transport/events/settings-events.ts index 72f13152c..4bcea3d9e 100644 --- a/src/shared/transport/events/settings-events.ts +++ b/src/shared/transport/events/settings-events.ts @@ -11,27 +11,33 @@ export const SettingsEvents = { * surfaces (CLI / TUI / WebUI) can consume it without crossing the * server import boundary. * - * M7 T2 added three optional fields. `category` and `unit` flow through - * from the descriptor; `scope` is reserved for the future project-store - * ticket and is always omitted in v1. All three are additive only — - * existing JSON consumers that ignore them continue to parse correctly. + * M7 T2 added three optional fields (`category`, `unit`, `scope`); T1 of + * the Update-check toggle project widened `type`, `current`, `default`, + * and `restartRequired` to also cover boolean descriptors, and made + * `min` / `max` optional (only integer descriptors carry them). All + * widenings are additive at the JSON layer, so consumers that read + * existing integer fields continue to parse the wire format. */ export interface SettingsItemDTO { - category?: 'concurrency' | 'llm' | 'task-history' - current: number - default: number + category?: 'concurrency' | 'llm' | 'task-history' | 'updates' + current: boolean | number + default: boolean | number description: string key: string - max: number - min: number - restartRequired: true + max?: number + min?: number + restartRequired: boolean scope?: 'global' | 'project' - type: 'integer' + type: 'boolean' | 'integer' unit?: 'count' | 'ms' } export interface SettingsErrorDTO { - code: 'invalid_value' | 'unknown_key' + code: 'invalid_value' | 'invalid_value_type' | 'unknown_key' + /** Expected runtime kind, only set when `code === 'invalid_value_type'`. */ + expected?: 'boolean' | 'integer' + /** `typeof` of the offending value, only set when `code === 'invalid_value_type'`. */ + got?: string key: string message: string value?: unknown @@ -53,12 +59,12 @@ export type SettingsGetResponse = export interface SettingsSetRequest { key: string - value: unknown + value: boolean | number } export type SettingsSetResponse = | {readonly error: SettingsErrorDTO; readonly ok: false} - | {readonly ok: true; readonly restartRequired: true} + | {readonly ok: true; readonly restartRequired: boolean} export interface SettingsResetRequest { key: string @@ -66,4 +72,4 @@ export interface SettingsResetRequest { export type SettingsResetResponse = | {readonly error: SettingsErrorDTO; readonly ok: false} - | {readonly ok: true; readonly restartRequired: true} + | {readonly ok: true; readonly restartRequired: boolean} diff --git a/src/shared/types/settings-row.ts b/src/shared/types/settings-row.ts index abbf1e08d..23cf81983 100644 --- a/src/shared/types/settings-row.ts +++ b/src/shared/types/settings-row.ts @@ -1,26 +1,41 @@ -export type SettingsRowCategory = 'concurrency' | 'llm' | 'other' | 'task-history' +export type SettingsRowCategory = 'concurrency' | 'llm' | 'other' | 'task-history' | 'updates' export type SettingsRowUnit = 'count' | 'ms' +/** + * View-model for one settings row consumed by the TUI. Discriminated on + * `type` so the renderer narrows before reading integer-only fields + * (`min`, `max`, `unit`) or treating `current` / `default` as numeric. + * + * Restart requirement is propagated from the descriptor verbatim (no + * literal `true` constraint) so the dirty-banner filter on the page can + * gate the restart warning per row. + */ export interface SettingsRow { readonly category: SettingsRowCategory - readonly current: number - readonly default: number + readonly current: boolean | number + readonly default: boolean | number readonly description: string readonly displayCurrent: string readonly displayDefault: string readonly displayRange: string readonly key: string readonly label: string - readonly max: number - readonly min: number + readonly max?: number + readonly min?: number readonly modified: boolean - readonly restartRequired: true - readonly type: 'integer' - readonly unit: SettingsRowUnit + readonly restartRequired: boolean + readonly type: 'boolean' | 'integer' + readonly unit?: SettingsRowUnit } export type RowParseResult = | {readonly displayValue: string; readonly kind: 'ok'; readonly value: number} | {readonly kind: 'error'; readonly message: string} -export const CATEGORY_ORDER: readonly SettingsRowCategory[] = ['concurrency', 'llm', 'task-history', 'other'] +export const CATEGORY_ORDER: readonly SettingsRowCategory[] = [ + 'concurrency', + 'llm', + 'task-history', + 'updates', + 'other', +] diff --git a/src/shared/utils/format-settings.ts b/src/shared/utils/format-settings.ts index 87f97e454..79d415e7e 100644 --- a/src/shared/utils/format-settings.ts +++ b/src/shared/utils/format-settings.ts @@ -1,12 +1,21 @@ import type {SettingsItemDTO} from '../transport/events/settings-events.js' -import type {RowParseResult, SettingsRow, SettingsRowUnit} from '../types/settings-row.js' +import type {RowParseResult, SettingsRow, SettingsRowCategory, SettingsRowUnit} from '../types/settings-row.js' import {CATEGORY_ORDER} from '../types/settings-row.js' import {formatCount, formatDuration, parseDuration} from './format-duration.js' export function buildSettingsRows(items: readonly SettingsItemDTO[]): SettingsRow[] { - const rows = items.map((item) => toRow(item)) - return [...rows].sort((a, b) => CATEGORY_ORDER.indexOf(a.category) - CATEGORY_ORDER.indexOf(b.category)) + const rows: SettingsRow[] = [] + for (const item of items) { + if (item.type === 'boolean' && typeof item.current === 'boolean' && typeof item.default === 'boolean') { + rows.push(toBooleanRow(item, item.current, item.default)) + continue + } + + if (isIntegerItem(item)) rows.push(toIntegerRow(item)) + } + + return rows.sort((a, b) => CATEGORY_ORDER.indexOf(a.category) - CATEGORY_ORDER.indexOf(b.category)) } export function parseRowInput(row: SettingsRow, raw: string): RowParseResult { @@ -18,6 +27,10 @@ export function parseRowInput(row: SettingsRow, raw: string): RowParseResult { } function parseAsDuration(row: SettingsRow, raw: string): RowParseResult { + if (row.min === undefined || row.max === undefined) { + return {kind: 'error', message: `${row.key} has no numeric range`} + } + const parsed = parseDuration(raw) if (typeof parsed === 'number') { if (parsed < row.min || parsed > row.max) { @@ -34,6 +47,10 @@ function parseAsDuration(row: SettingsRow, raw: string): RowParseResult { } function parseAsCount(row: SettingsRow, raw: string): RowParseResult { + if (row.min === undefined || row.max === undefined) { + return {kind: 'error', message: `${row.key} has no numeric range`} + } + if (/\d+\s*(?:ms|s|m|h)/i.test(raw)) { return {kind: 'error', message: `${row.key} expects an integer count, got duration '${raw}'.`} } @@ -52,12 +69,24 @@ function parseAsCount(row: SettingsRow, raw: string): RowParseResult { return {displayValue: formatCount(numeric), kind: 'ok', value: numeric} } -function toRow(item: SettingsItemDTO): SettingsRow { +type IntegerSettingsItemDTO = Omit & { + readonly current: number + readonly default: number + readonly max: number + readonly min: number + readonly type: 'integer' +} + +function isIntegerItem(item: SettingsItemDTO): item is IntegerSettingsItemDTO { + return item.type === 'integer' && typeof item.min === 'number' && typeof item.max === 'number' +} + +function toIntegerRow(item: IntegerSettingsItemDTO): SettingsRow { const unit: SettingsRowUnit = item.unit ?? 'count' - const category = item.category ?? 'other' + const category = toRowCategory(item.category) const displayCurrent = unit === 'ms' ? formatDuration(item.current) : formatCount(item.current) const displayDefault = unit === 'ms' ? formatDuration(item.default) : formatCount(item.default) - const displayRange = formatRange(item, unit) + const displayRange = formatIntegerRange(item, unit) return { category, @@ -73,12 +102,41 @@ function toRow(item: SettingsItemDTO): SettingsRow { min: item.min, modified: item.current !== item.default, restartRequired: item.restartRequired, - type: item.type, + type: 'integer', unit, } } -function formatRange(item: SettingsItemDTO, unit: SettingsRowUnit): string { +function toBooleanRow(item: SettingsItemDTO, current: boolean, defaultValue: boolean): SettingsRow { + return { + category: toRowCategory(item.category), + current, + default: defaultValue, + description: item.description, + displayCurrent: renderBoolean(current), + displayDefault: renderBoolean(defaultValue), + displayRange: '', + key: item.key, + label: item.key, + modified: current !== defaultValue, + restartRequired: item.restartRequired, + type: 'boolean', + } +} + +function renderBoolean(value: boolean): string { + return value ? '[ on ]' : '[ off ]' +} + +function toRowCategory(category: SettingsItemDTO['category']): SettingsRowCategory { + if (category === 'concurrency' || category === 'llm' || category === 'task-history' || category === 'updates') { + return category + } + + return 'other' +} + +function formatIntegerRange(item: IntegerSettingsItemDTO, unit: SettingsRowUnit): string { const min = unit === 'ms' ? formatDuration(item.min) : formatCount(item.min) const max = unit === 'ms' ? formatDuration(item.max) : formatCount(item.max) const base = `${min}-${max}` diff --git a/src/tui/features/settings/components/settings-page.tsx b/src/tui/features/settings/components/settings-page.tsx index 925e182c4..3a7ca99f9 100644 --- a/src/tui/features/settings/components/settings-page.tsx +++ b/src/tui/features/settings/components/settings-page.tsx @@ -32,6 +32,19 @@ export function SettingsPage({onCancel, onComplete}: CustomDialogCallbacks): Rea const hintMode: 'browse' | 'edit' | 'edit-error' | 'saving' = mode === 'edit' && rowError !== undefined ? 'edit-error' : mode + // Restart warning fires only when at least one dirty key actually + // requires a daemon restart. Boolean toggles (e.g. update.checkForUpdates, + // restartRequired: false) must not produce a misleading prompt. + const restartRequiredDirty = useMemo(() => { + const filtered = new Set() + for (const dirtyKey of dirtyKeys) { + const row = rows.find((r) => r.key === dirtyKey) + if (row?.restartRequired === true) filtered.add(dirtyKey) + } + + return filtered + }, [dirtyKeys, rows]) + const enterEdit = useCallback((row: SettingsRow) => { setEditBuffer(preFillBufferFor(row)) setRowError(undefined) @@ -86,6 +99,28 @@ export function SettingsPage({onCancel, onComplete}: CustomDialogCallbacks): Rea [resetMutation], ) + const toggleBoolean = useCallback( + async (row: SettingsRow) => { + if (row.type !== 'boolean' || typeof row.current !== 'boolean') return + setMode('saving') + setRowError(undefined) + const response = await setMutation.mutateAsync({key: row.key, value: !row.current}) + if (response.ok) { + setDirtyKeys((previous) => { + const next = new Set(previous) + next.add(row.key) + return next + }) + setMode('browse') + return + } + + setRowError(response.error.message) + setMode('browse') + }, + [setMutation], + ) + useInput( (input, key) => { if (key.escape) { @@ -107,8 +142,14 @@ export function SettingsPage({onCancel, onComplete}: CustomDialogCallbacks): Rea return } - if (key.return) { - enterEdit(rows[cursor]) + if (key.return || input === ' ') { + const row = rows[cursor] + if (row.type === 'boolean') { + toggleBoolean(row).catch(() => {}) + } else { + enterEdit(row) + } + return } @@ -175,7 +216,7 @@ export function SettingsPage({onCancel, onComplete}: CustomDialogCallbacks): Rea return ( - {dirtyKeys.size > 0 && ( + {restartRequiredDirty.size > 0 && ( Settings changed. Run `brv restart` to apply. @@ -184,7 +225,7 @@ export function SettingsPage({onCancel, onComplete}: CustomDialogCallbacks): Rea SETTINGS {' '} - scope: global - `brv restart` to apply + {restartRequiredDirty.size > 0 ? 'scope: global - `brv restart` to apply' : 'scope: global'} {groups.map((group) => ( diff --git a/src/tui/features/settings/utils/format-settings.ts b/src/tui/features/settings/utils/format-settings.ts index 03a360161..4c171fb83 100644 --- a/src/tui/features/settings/utils/format-settings.ts +++ b/src/tui/features/settings/utils/format-settings.ts @@ -6,6 +6,7 @@ const CATEGORY_HEADERS: Readonly> = { llm: 'LLM', other: 'OTHER', 'task-history': 'TASK HISTORY', + updates: 'UPDATES', } export function groupRowsByCategory(rows: readonly SettingsRow[]): ReadonlyArray<{ @@ -52,6 +53,11 @@ export function bottomHintFor(mode: 'browse' | 'edit' | 'edit-error' | 'saving', } export function preFillBufferFor(row: SettingsRow): string { + // preFillBufferFor only runs when entering integer text-input mode. + // Boolean rows take the toggle path in the page and never reach here; + // guard the narrowing so the function still compiles under the wider + // SettingsRow union. + if (typeof row.current !== 'number') return String(row.current) if (row.unit === 'ms') return formatDuration(row.current) return String(row.current) } diff --git a/src/webui/features/settings/components/boolean-settings-row.tsx b/src/webui/features/settings/components/boolean-settings-row.tsx new file mode 100644 index 000000000..e0a755efd --- /dev/null +++ b/src/webui/features/settings/components/boolean-settings-row.tsx @@ -0,0 +1,58 @@ +import {Switch} from '@campfirein/byterover-packages/components/switch' +import {useId} from 'react' +import {toast} from 'sonner' + +import type {SettingsRow as SettingsRowData} from '../../../../shared/types/settings-row' + +import {formatError} from '../../../lib/error-messages' +import {noop} from '../../../lib/noop' +import {useSetSetting} from '../api/set-setting' +import {labelFor} from '../lib/labels' +import {useRestartBannerStore} from '../stores/restart-banner-store' + +type Props = { + row: SettingsRowData +} + +export function BooleanSettingsRow({row}: Props) { + const setMutation = useSetSetting() + const markDirty = useRestartBannerStore((s) => s.markDirty) + const descriptionId = useId() + + const label = labelFor(row.key) + const checked = typeof row.current === 'boolean' ? row.current : false + + const toggle = async (next: boolean) => { + try { + const response = await setMutation.mutateAsync({key: row.key, value: next}) + if (response.ok) { + markDirty(row.key, row.restartRequired) + toast.success(`${label} ${next ? 'enabled' : 'disabled'}`) + return + } + + toast.error(response.error.message) + } catch (error) { + toast.error(formatError(error, `Failed to update ${label}`)) + } + } + + return ( +
+
+ {label} + + {row.description} + +
+ { + toggle(next).catch(noop) + }} + /> +
+ ) +} diff --git a/src/webui/features/settings/components/settings-row.tsx b/src/webui/features/settings/components/settings-row.tsx index cc81b124c..09278996a 100644 --- a/src/webui/features/settings/components/settings-row.tsx +++ b/src/webui/features/settings/components/settings-row.tsx @@ -14,12 +14,19 @@ import {useResetSetting} from '../api/reset-setting' import {useSetSetting} from '../api/set-setting' import {labelFor} from '../lib/labels' import {useRestartBannerStore} from '../stores/restart-banner-store' +import {BooleanSettingsRow} from './boolean-settings-row' type Props = { row: SettingsRowData } export function SettingsRow({row}: Props) { + if (row.type === 'boolean') return + + return +} + +function IntegerSettingsRow({row}: Props) { const setMutation = useSetSetting() const resetMutation = useResetSetting() const markDirty = useRestartBannerStore((s) => s.markDirty) @@ -57,7 +64,7 @@ export function SettingsRow({row}: Props) { setError(undefined) const response = await setMutation.mutateAsync({key: row.key, value: parsed.value}) if (response.ok) { - markDirty(row.key) + markDirty(row.key, row.restartRequired) isUserEditingRef.current = false toast.success(`${label} set to ${toastValue(parsed.value)}`) return @@ -70,7 +77,7 @@ export function SettingsRow({row}: Props) { setError(undefined) const response = await resetMutation.mutateAsync({key: row.key}) if (response.ok) { - markDirty(row.key) + markDirty(row.key, row.restartRequired) isUserEditingRef.current = false toast.success(`${label} reset to default`) return diff --git a/src/webui/features/settings/components/updates-panel.tsx b/src/webui/features/settings/components/updates-panel.tsx new file mode 100644 index 000000000..8b09d2960 --- /dev/null +++ b/src/webui/features/settings/components/updates-panel.tsx @@ -0,0 +1,42 @@ +import {LoaderCircle} from 'lucide-react' +import {Fragment, useMemo} from 'react' + +import {buildSettingsRows} from '../../../../shared/utils/format-settings' +import {noop} from '../../../lib/noop' +import {SettingsSection} from '../../vc/components/settings-section' +import {useGetSettings} from '../api/list-settings' +import {SettingsRow} from './settings-row' +import {SettingsSkeleton} from './settings-skeleton' + +export function UpdatesPanel() { + const {data, error, isError, isLoading, refetch} = useGetSettings() + + const rows = useMemo(() => { + if (!data) return [] + return buildSettingsRows(data.items).filter((row) => row.category === 'updates') + }, [data?.items]) + + return ( + : undefined} + description="Update checks performed when brv starts." + error={isError ? error : undefined} + errorFallback="Failed to load updates settings" + onRetry={() => refetch().catch(noop)} + title="Updates" + > + {data ? ( +
+ {rows.map((row, index) => ( + + + {index < rows.length - 1 &&
} + + ))} +
+ ) : ( + + )} + + ) +} diff --git a/src/webui/features/settings/lib/labels.ts b/src/webui/features/settings/lib/labels.ts index 83e9112e2..1c5368197 100644 --- a/src/webui/features/settings/lib/labels.ts +++ b/src/webui/features/settings/lib/labels.ts @@ -4,6 +4,7 @@ const LABELS: Record = { 'llm.iterationBudgetMs': 'Agentic loop budget', 'llm.requestTimeoutMs': 'LLM request timeout', 'taskHistory.maxEntries': 'Task history size', + 'update.checkForUpdates': 'Check for updates at startup', } export function labelFor(key: string): string { diff --git a/src/webui/features/settings/stores/restart-banner-store.ts b/src/webui/features/settings/stores/restart-banner-store.ts index 27b4ae802..653e3879c 100644 --- a/src/webui/features/settings/stores/restart-banner-store.ts +++ b/src/webui/features/settings/stores/restart-banner-store.ts @@ -3,14 +3,15 @@ import {create} from 'zustand' interface RestartBannerState { clear: () => void dirtyKeys: ReadonlySet - markDirty: (key: string) => void + markDirty: (key: string, restartRequired: boolean) => void } export const useRestartBannerStore = create((set) => ({ clear: () => set({dirtyKeys: new Set()}), dirtyKeys: new Set(), - markDirty: (key) => + markDirty: (key, restartRequired) => set((state) => { + if (!restartRequired) return state if (state.dirtyKeys.has(key)) return state const next = new Set(state.dirtyKeys) next.add(key) diff --git a/src/webui/pages/configuration/general.tsx b/src/webui/pages/configuration/general.tsx index 5b4f85d56..a5e5c1237 100644 --- a/src/webui/pages/configuration/general.tsx +++ b/src/webui/pages/configuration/general.tsx @@ -1,6 +1,7 @@ import {ConcurrencyPanel} from '../../features/settings/components/concurrency-panel' import {LlmPanel} from '../../features/settings/components/llm-panel' import {TaskHistoryPanel} from '../../features/settings/components/task-history-panel' +import {UpdatesPanel} from '../../features/settings/components/updates-panel' export function GeneralSection() { return ( @@ -8,6 +9,7 @@ export function GeneralSection() { + ) } diff --git a/test/commands/settings/index.test.ts b/test/commands/settings/index.test.ts index 52125557e..5a5597152 100644 --- a/test/commands/settings/index.test.ts +++ b/test/commands/settings/index.test.ts @@ -310,4 +310,69 @@ describe('brv settings (index)', () => { expect(loggedMessages.some((m) => m.includes('Connection failed'))).to.be.true }) + + describe('boolean rows (T4 UPDATES group)', () => { + it('renders an UPDATES section with the boolean row when the daemon returns one', async () => { + const requestStub = mockClient.requestWithAck as sinon.SinonStub + requestStub.resolves({ + items: [ + { + category: 'concurrency', + current: 10, + default: 10, + description: 'h', + key: 'agentPool.maxSize', + max: 100, + min: 1, + restartRequired: true, + type: 'integer', + }, + { + category: 'updates', + current: true, + default: true, + description: 'Check for brv updates at startup and notify when one is available', + key: 'update.checkForUpdates', + restartRequired: false, + type: 'boolean', + }, + ], + }) + + await createCommand().run() + const output = loggedMessages.join('\n') + + expect(output, 'UPDATES header must appear').to.include('UPDATES') + const booleanRow = loggedMessages.find((m) => m.includes('update.checkForUpdates')) + expect(booleanRow, 'row for update.checkForUpdates').to.exist + expect(booleanRow).to.include('true') + expect(booleanRow).to.match(/default\s*true/) + // Range (min-max) is meaningless for booleans and must not be rendered. + expect(booleanRow).to.not.match(/\d+\s*-\s*\d+/) + }) + + it('omits the UPDATES section entirely when the daemon returns no boolean items', async () => { + const requestStub = mockClient.requestWithAck as sinon.SinonStub + requestStub.resolves({ + items: [ + { + category: 'concurrency', + current: 10, + default: 10, + description: 'h', + key: 'agentPool.maxSize', + max: 100, + min: 1, + restartRequired: true, + type: 'integer', + }, + ], + }) + + await createCommand().run() + const output = loggedMessages.join('\n') + + expect(output).to.not.include('UPDATES') + }) + }) }) diff --git a/test/commands/settings/reset.test.ts b/test/commands/settings/reset.test.ts index 9b82f99d8..14287882c 100644 --- a/test/commands/settings/reset.test.ts +++ b/test/commands/settings/reset.test.ts @@ -33,6 +33,19 @@ class TestableSettingsReset extends SettingsReset { } } +function makeBooleanGetResponse(key: string, current: boolean): unknown { + return { + category: 'updates', + current, + default: true, + description: 'desc', + key, + ok: true, + restartRequired: false, + type: 'boolean', + } +} + function makeGetResponse(key: string): unknown { if (key === 'llm.iterationBudgetMs') { return { @@ -231,4 +244,34 @@ describe('brv settings reset', () => { it('emits a one-line help mentioning the restart-required behavior', () => { expect(SettingsReset.description ?? '').to.match(/restart/i) }) + + describe('boolean keys (T4)', () => { + it('echoes the boolean default ("back to default (true)") and omits the restart hint', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', false) + if (event === SettingsEvents.RESET) return {ok: true, restartRequired: false} + throw new Error('unexpected event') + }) + + await createCommand('update.checkForUpdates').run() + + const output = loggedMessages.join('\n') + expect(output).to.match(/Setting reset: update\.checkForUpdates back to default \(true\)/) + expect(output, 'must not mention `brv restart` for restartRequired:false keys').to.not.match(/brv restart/i) + expect(process.exitCode ?? 0).to.equal(0) + }) + + it('keeps the restart hint for integer keys that require restart (regression)', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeGetResponse('agentPool.maxSize') + if (event === SettingsEvents.RESET) return {ok: true, restartRequired: true} + throw new Error('unexpected event') + }) + + await createCommand('agentPool.maxSize').run() + + const output = loggedMessages.join('\n') + expect(output).to.match(/Run `brv restart` to apply/) + }) + }) }) diff --git a/test/commands/settings/set.test.ts b/test/commands/settings/set.test.ts index 397120839..4364d3613 100644 --- a/test/commands/settings/set.test.ts +++ b/test/commands/settings/set.test.ts @@ -24,7 +24,7 @@ class TestableSettingsSet extends SettingsSet { }) } - protected override async writeSetting(key: string, value: unknown) { + protected override async writeSetting(key: string, value: boolean | number) { return super.writeSetting(key, value, { maxRetries: 1, retryDelayMs: 0, @@ -42,6 +42,19 @@ type DescriptorOverrides = Partial<{ unit: 'count' | 'ms' }> +function makeBooleanGetResponse(key: string, current: boolean): unknown { + return { + category: 'updates', + current, + default: true, + description: 'desc', + key, + ok: true, + restartRequired: false, + type: 'boolean', + } +} + function makeGetResponse(key: string, current: number, overrides: DescriptorOverrides = {}): unknown { const defaults: Record = { 'agentPool.maxSize': {category: 'concurrency', default: 10, max: 100, min: 1}, @@ -286,4 +299,107 @@ describe('brv settings set', () => { it('emits a one-line help mentioning the restart-required behavior', () => { expect(SettingsSet.description ?? '').to.match(/restart/i) }) + + describe('boolean keys (T4)', () => { + const ACCEPTED_TRUE = ['true', 'TRUE', 'True', 'on', 'ON', 'On', '1', 'yes', 'YES', 'Yes'] + const ACCEPTED_FALSE = ['false', 'FALSE', 'False', 'off', 'OFF', 'Off', '0', 'no', 'NO', 'No'] + + for (const token of ACCEPTED_TRUE) { + it(`parses '${token}' as true and dispatches SET with boolean true`, async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', false) + if (event === SettingsEvents.SET) return {ok: true, restartRequired: false} + throw new Error('unexpected event') + }) + + await createCommand('update.checkForUpdates', token).run() + + const requestStub = mockClient.requestWithAck as sinon.SinonStub + const setCall = requestStub.getCalls().find((c) => c.args[0] === SettingsEvents.SET) + expect(setCall?.args[1]).to.deep.equal({key: 'update.checkForUpdates', value: true}) + }) + } + + for (const token of ACCEPTED_FALSE) { + it(`parses '${token}' as false and dispatches SET with boolean false`, async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', true) + if (event === SettingsEvents.SET) return {ok: true, restartRequired: false} + throw new Error('unexpected event') + }) + + await createCommand('update.checkForUpdates', token).run() + + const requestStub = mockClient.requestWithAck as sinon.SinonStub + const setCall = requestStub.getCalls().find((c) => c.args[0] === SettingsEvents.SET) + expect(setCall?.args[1]).to.deep.equal({key: 'update.checkForUpdates', value: false}) + }) + } + + it('rejects "5" on a boolean key with the documented expected-boolean message', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', true) + throw new Error('SET should not be dispatched on parse error') + }) + + await createCommand('update.checkForUpdates', '5').run() + + const stderr = loggedMessages.join('\n') + expect(stderr).to.include('expected boolean (true, false, on, off, 1, 0, yes, no)') + expect(process.exitCode).to.equal(1) + }) + + it('rejects "maybe" on a boolean key with the same documented message', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', true) + throw new Error('SET should not be dispatched on parse error') + }) + + await createCommand('update.checkForUpdates', 'maybe').run() + + const stderr = loggedMessages.join('\n') + expect(stderr).to.include('expected boolean') + expect(process.exitCode).to.equal(1) + }) + + it('integer key still rejects "true" with the "expects an integer count" message (parser dispatch correct)', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeGetResponse('agentPool.maxSize', 10) + throw new Error('SET should not be dispatched on parse error') + }) + + await createCommand('agentPool.maxSize', 'true').run() + + const stderr = loggedMessages.join('\n') + expect(stderr).to.match(/expects an integer count/) + expect(process.exitCode).to.equal(1) + }) + + it('omits the "Run `brv restart`" line on success when descriptor does not require restart', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeBooleanGetResponse('update.checkForUpdates', true) + if (event === SettingsEvents.SET) return {ok: true, restartRequired: false} + throw new Error('unexpected event') + }) + + await createCommand('update.checkForUpdates', 'off').run() + + const output = loggedMessages.join('\n') + expect(output).to.include('Setting saved: update.checkForUpdates = false') + expect(output, 'must not mention `brv restart` for restartRequired:false keys').to.not.match(/brv restart/i) + }) + + it('keeps the "Run `brv restart`" line on success for restart-required integer keys (regression)', async () => { + dispatchByEvent((event) => { + if (event === SettingsEvents.GET) return makeGetResponse('agentPool.maxSize', 10) + if (event === SettingsEvents.SET) return {ok: true, restartRequired: true} + throw new Error('unexpected event') + }) + + await createCommand('agentPool.maxSize', '25').run() + + const output = loggedMessages.join('\n') + expect(output).to.match(/Run `brv restart` to apply/) + }) + }) }) diff --git a/test/hooks/init/block-autoupdate-when-off.test.ts b/test/hooks/init/block-autoupdate-when-off.test.ts new file mode 100644 index 000000000..43f4ac382 --- /dev/null +++ b/test/hooks/init/block-autoupdate-when-off.test.ts @@ -0,0 +1,76 @@ +import {expect} from 'chai' +import {mkdtempSync, rmSync, writeFileSync} from 'node:fs' +import {tmpdir} from 'node:os' +import {join} from 'node:path' +import * as sinon from 'sinon' + +import {handleBlockAutoupdateWhenOff} from '../../../src/oclif/hooks/init/block-autoupdate-when-off.js' + +describe('block-autoupdate-when-off hook (T7)', () => { + let tempDir: string + let priorBrvDataDir: string | undefined + let exitStub: sinon.SinonStub<[number], never> + + beforeEach(() => { + priorBrvDataDir = process.env.BRV_DATA_DIR + tempDir = mkdtempSync(join(tmpdir(), 'brv-blockauto-')) + process.env.BRV_DATA_DIR = tempDir + exitStub = sinon.stub<[number], never>() + }) + + afterEach(() => { + rmSync(tempDir, {force: true, recursive: true}) + if (priorBrvDataDir === undefined) delete process.env.BRV_DATA_DIR + else process.env.BRV_DATA_DIR = priorBrvDataDir + sinon.restore() + }) + + function writeSetting(value: boolean): void { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': value}, version: '2'}), + ) + } + + it('exits silently when commandId=update + --autoupdate flag + setting off', () => { + writeSetting(false) + handleBlockAutoupdateWhenOff({argv: ['--autoupdate'], commandId: 'update', exitFn: exitStub}) + expect(exitStub.calledOnce).to.equal(true) + expect(exitStub.firstCall.args[0]).to.equal(0) + }) + + it('does NOT exit when commandId=update + --autoupdate flag + setting ON', () => { + writeSetting(true) + handleBlockAutoupdateWhenOff({argv: ['--autoupdate'], commandId: 'update', exitFn: exitStub}) + expect(exitStub.called).to.equal(false) + }) + + it('does NOT exit when commandId=update + --autoupdate flag + setting missing (default true)', () => { + handleBlockAutoupdateWhenOff({argv: ['--autoupdate'], commandId: 'update', exitFn: exitStub}) + expect(exitStub.called).to.equal(false) + }) + + it('does NOT exit when commandId=update WITHOUT --autoupdate flag (manual update)', () => { + writeSetting(false) + handleBlockAutoupdateWhenOff({argv: [], commandId: 'update', exitFn: exitStub}) + expect(exitStub.called).to.equal(false) + }) + + it('does NOT exit when commandId is something else, even with --autoupdate in argv', () => { + writeSetting(false) + handleBlockAutoupdateWhenOff({argv: ['--autoupdate'], commandId: 'status', exitFn: exitStub}) + expect(exitStub.called).to.equal(false) + }) + + it('does NOT exit when commandId is undefined', () => { + writeSetting(false) + handleBlockAutoupdateWhenOff({argv: ['--autoupdate'], commandId: undefined, exitFn: exitStub}) + expect(exitStub.called).to.equal(false) + }) + + it('matches the flag even when it appears alongside other argv entries', () => { + writeSetting(false) + handleBlockAutoupdateWhenOff({argv: ['--verbose', '--autoupdate', '--foo'], commandId: 'update', exitFn: exitStub}) + expect(exitStub.calledOnce).to.equal(true) + }) +}) diff --git a/test/hooks/init/update-notifier.test.ts b/test/hooks/init/update-notifier.test.ts index 118845864..96a3e2f17 100644 --- a/test/hooks/init/update-notifier.test.ts +++ b/test/hooks/init/update-notifier.test.ts @@ -1,4 +1,7 @@ import {expect} from 'chai' +import {mkdtempSync, rmSync, writeFileSync} from 'node:fs' +import {tmpdir} from 'node:os' +import {join} from 'node:path' import * as sinon from 'sinon' import type {NarrowedUpdateNotifier, UpdateNotifierDeps} from '../../../src/oclif/hooks/init/update-notifier.js' @@ -6,6 +9,7 @@ import type {NarrowedUpdateNotifier, UpdateNotifierDeps} from '../../../src/ocli import { handleUpdateNotification, isNpmGlobalInstall, + shouldRunUpdateCheck, UPDATE_CHECK_INTERVAL_MS, } from '../../../src/oclif/hooks/init/update-notifier.js' @@ -205,6 +209,54 @@ describe('update-notifier hook', () => { }) }) + describe('shouldRunUpdateCheck (T7 gate)', () => { + let tempDir: string + let priorBrvDataDir: string | undefined + let priorBrvEnv: string | undefined + + beforeEach(() => { + priorBrvDataDir = process.env.BRV_DATA_DIR + priorBrvEnv = process.env.BRV_ENV + tempDir = mkdtempSync(join(tmpdir(), 'brv-updnotify-')) + process.env.BRV_DATA_DIR = tempDir + delete process.env.BRV_ENV + }) + + afterEach(() => { + rmSync(tempDir, {force: true, recursive: true}) + if (priorBrvDataDir === undefined) delete process.env.BRV_DATA_DIR + else process.env.BRV_DATA_DIR = priorBrvDataDir + if (priorBrvEnv === undefined) delete process.env.BRV_ENV + else process.env.BRV_ENV = priorBrvEnv + }) + + it('returns true by default (setting missing, not dev, not update command)', () => { + expect(shouldRunUpdateCheck({commandId: 'status'})).to.equal(true) + }) + + it('returns false when the setting is explicitly off', () => { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': false}, version: '2'}), + ) + expect(shouldRunUpdateCheck({commandId: 'status'})).to.equal(false) + }) + + it('returns false when the command being invoked is the update command (anti-recursion)', () => { + expect(shouldRunUpdateCheck({commandId: 'update'})).to.equal(false) + }) + + it('returns false when BRV_ENV=development', () => { + process.env.BRV_ENV = 'development' + expect(shouldRunUpdateCheck({commandId: 'status'})).to.equal(false) + }) + + it('returns true when BRV_ENV is set to a non-development value', () => { + process.env.BRV_ENV = 'production' + expect(shouldRunUpdateCheck({commandId: 'status'})).to.equal(true) + }) + }) + describe('isNpmGlobalInstall', () => { it('should return true when npm list succeeds', () => { const execSyncStub = sinon.stub().returns(Buffer.from('')) diff --git a/test/unit/core/domain/entities/settings-registry.test.ts b/test/unit/core/domain/entities/settings-registry.test.ts index ed99867cc..449c1e895 100644 --- a/test/unit/core/domain/entities/settings-registry.test.ts +++ b/test/unit/core/domain/entities/settings-registry.test.ts @@ -6,6 +6,17 @@ import { SETTINGS_REGISTRY, } from '../../../../../src/server/core/domain/entities/settings.js' +function integerMaxOf(key: string): number { + const descriptor = findSettingDescriptor(key) + if (descriptor?.type !== 'integer') throw new Error(`expected integer descriptor for ${key}`) + return descriptor.max +} + +function unitOf(key: string): string | undefined { + const descriptor = findSettingDescriptor(key) + return descriptor?.type === 'integer' ? descriptor.unit : undefined +} + describe('settings registry — M7 T2 shape', () => { it('declares category on every descriptor', () => { for (const descriptor of SETTINGS_REGISTRY) { @@ -13,6 +24,7 @@ describe('settings registry — M7 T2 shape', () => { 'concurrency', 'llm', 'task-history', + 'updates', ]) } }) @@ -32,29 +44,29 @@ describe('settings registry — M7 T2 shape', () => { }) it('declares unit=ms on the two llm.*Ms keys', () => { - expect(findSettingDescriptor(SETTINGS_KEYS.LLM_ITERATION_BUDGET_MS)?.unit).to.equal('ms') - expect(findSettingDescriptor(SETTINGS_KEYS.LLM_REQUEST_TIMEOUT_MS)?.unit).to.equal('ms') + expect(unitOf(SETTINGS_KEYS.LLM_ITERATION_BUDGET_MS)).to.equal('ms') + expect(unitOf(SETTINGS_KEYS.LLM_REQUEST_TIMEOUT_MS)).to.equal('ms') }) it('omits unit (or sets count) on non-ms keys', () => { - const maxSize = findSettingDescriptor(SETTINGS_KEYS.AGENT_POOL_MAX_SIZE)?.unit + const maxSize = unitOf(SETTINGS_KEYS.AGENT_POOL_MAX_SIZE) expect(maxSize === undefined || maxSize === 'count').to.equal(true) - const tasks = findSettingDescriptor(SETTINGS_KEYS.AGENT_POOL_MAX_CONCURRENT_TASKS)?.unit + const tasks = unitOf(SETTINGS_KEYS.AGENT_POOL_MAX_CONCURRENT_TASKS) expect(tasks === undefined || tasks === 'count').to.equal(true) - const history = findSettingDescriptor(SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES)?.unit + const history = unitOf(SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES) expect(history === undefined || history === 'count').to.equal(true) }) it('tightens llm.iterationBudgetMs max to 3_600_000 (1h)', () => { - expect(findSettingDescriptor(SETTINGS_KEYS.LLM_ITERATION_BUDGET_MS)?.max).to.equal(3_600_000) + expect(integerMaxOf(SETTINGS_KEYS.LLM_ITERATION_BUDGET_MS)).to.equal(3_600_000) }) it('tightens llm.requestTimeoutMs max to 3_600_000 (1h)', () => { - expect(findSettingDescriptor(SETTINGS_KEYS.LLM_REQUEST_TIMEOUT_MS)?.max).to.equal(3_600_000) + expect(integerMaxOf(SETTINGS_KEYS.LLM_REQUEST_TIMEOUT_MS)).to.equal(3_600_000) }) it('tightens taskHistory.maxEntries max to 10_000', () => { - expect(findSettingDescriptor(SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES)?.max).to.equal(10_000) + expect(integerMaxOf(SETTINGS_KEYS.TASK_HISTORY_MAX_ENTRIES)).to.equal(10_000) }) it('keeps every description string at <= 80 chars (WebUI tooltip budget)', () => { @@ -65,4 +77,54 @@ describe('settings registry — M7 T2 shape', () => { ).to.be.at.most(80) } }) + + describe('update.checkForUpdates (T1 boolean descriptor)', () => { + it('exposes UPDATE_CHECK_FOR_UPDATES on SETTINGS_KEYS', () => { + expect(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES).to.equal('update.checkForUpdates') + }) + + it('registers a descriptor for update.checkForUpdates', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES) + expect(descriptor, 'descriptor must exist in SETTINGS_REGISTRY').to.exist + }) + + it('declares the descriptor as type=boolean with default=true', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES) + expect(descriptor?.type).to.equal('boolean') + expect(descriptor?.default).to.equal(true) + }) + + it('marks the descriptor as not requiring a daemon restart', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES) + expect(descriptor?.restartRequired).to.equal(false) + }) + + it('groups the descriptor under category=updates', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES) + expect(descriptor?.category).to.equal('updates') + }) + + it('narrows to BooleanSettingDescriptor when descriptor.type === boolean', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.UPDATE_CHECK_FOR_UPDATES) + if (descriptor?.type === 'boolean') { + // If this assignment compiles, narrowing works. Otherwise the test + // file fails to type-check at build time, which is the test's + // primary value. + const defaultValue: boolean = descriptor.default + expect(defaultValue).to.equal(true) + } else { + expect.fail('expected boolean descriptor for update.checkForUpdates') + } + }) + + it('narrows existing integer descriptors to IntegerSettingDescriptor when descriptor.type === integer', () => { + const descriptor = findSettingDescriptor(SETTINGS_KEYS.AGENT_POOL_MAX_SIZE) + if (descriptor?.type === 'integer') { + const {max, min} = descriptor + expect(min).to.be.lessThan(max) + } else { + expect.fail('expected integer descriptor for agentPool.maxSize') + } + }) + }) }) diff --git a/test/unit/infra/storage/file-settings-store.test.ts b/test/unit/infra/storage/file-settings-store.test.ts index 960710198..6c3a593ac 100644 --- a/test/unit/infra/storage/file-settings-store.test.ts +++ b/test/unit/infra/storage/file-settings-store.test.ts @@ -52,10 +52,10 @@ describe('FileSettingsStore', () => { 'llm.iterationBudgetMs', 'llm.requestTimeoutMs', 'taskHistory.maxEntries', + 'update.checkForUpdates', ]) for (const item of items) { expect(item.current).to.equal(item.default) - expect(item.restartRequired).to.equal(true) } }) @@ -404,4 +404,85 @@ describe('FileSettingsStore', () => { expect(files.filter((f) => f.endsWith('.tmp'))).to.have.lengthOf(0) }) }) + + describe('schema migration (T2: v1 -> v2)', () => { + const CURRENT_SCHEMA_VERSION = '2' + + it('writes the current schema version on a fresh set', async () => { + await store.set('agentPool.maxSize', 25) + const file = asSettingsFile(JSON.parse(await readFile(join(tempDir, SETTINGS_FILENAME), 'utf8'))) + expect(file.version).to.equal(CURRENT_SCHEMA_VERSION) + }) + + it('round-trips a boolean value (true, not 1) for update.checkForUpdates', async () => { + await store.set('update.checkForUpdates', true) + const item = await store.get('update.checkForUpdates') + expect(item.current).to.equal(true) + const file = asSettingsFile(JSON.parse(await readFile(join(tempDir, SETTINGS_FILENAME), 'utf8'))) + expect(file.values['update.checkForUpdates']).to.equal(true) + }) + + it('round-trips a boolean false value', async () => { + await store.set('update.checkForUpdates', false) + const item = await store.get('update.checkForUpdates') + expect(item.current).to.equal(false) + }) + + it('migrates a pre-existing v1 file to v2 on first read, preserving every value', async () => { + const v1Payload = { + values: { + 'agentPool.maxConcurrentTasksPerProject': 4, + 'agentPool.maxSize': 25, + 'llm.iterationBudgetMs': 600_000, + }, + version: '1', + } + await writeFile(join(tempDir, SETTINGS_FILENAME), JSON.stringify(v1Payload, null, 2), 'utf8') + + await store.list() + + const file = asSettingsFile(JSON.parse(await readFile(join(tempDir, SETTINGS_FILENAME), 'utf8'))) + expect(file.version).to.equal(CURRENT_SCHEMA_VERSION) + expect(file.values).to.deep.equal(v1Payload.values) + }) + + it('does not rewrite the file on a second read once it is already v2 (idempotent)', async () => { + // Trigger an initial migration so the file is v2 on disk. + const v1Payload = {values: {'agentPool.maxSize': 25}, version: '1'} + const path = join(tempDir, SETTINGS_FILENAME) + await writeFile(path, JSON.stringify(v1Payload, null, 2), 'utf8') + await store.list() + + // Snapshot mtime after migration, then read again and compare. + const {readFile: readFileFs, stat} = await import('node:fs/promises') + const afterMigration = await stat(path) + const contentBeforeRead = await readFileFs(path, 'utf8') + + await store.list() + + const afterSecondRead = await stat(path) + const contentAfterRead = await readFileFs(path, 'utf8') + + expect(afterSecondRead.mtimeMs, 'mtime must not change on a re-read of a v2 file').to.equal(afterMigration.mtimeMs) + expect(contentAfterRead).to.equal(contentBeforeRead) + }) + + it('preserves pre-existing invalid entries through migration (does not collateral-damage)', async () => { + // v1 file with a mix of valid and invalid entries. Migration must not + // drop the invalid ones (per the existing reset() preservation rule). + const v1Payload = { + values: { + 'agentPool.maxSize': 25, + 'unknown.key': 'something', + }, + version: '1', + } + await writeFile(join(tempDir, SETTINGS_FILENAME), JSON.stringify(v1Payload, null, 2), 'utf8') + await store.list() + + const file = asSettingsFile(JSON.parse(await readFile(join(tempDir, SETTINGS_FILENAME), 'utf8'))) + expect(file.version).to.equal(CURRENT_SCHEMA_VERSION) + expect(file.values).to.deep.equal(v1Payload.values) + }) + }) }) diff --git a/test/unit/infra/storage/settings-validator.test.ts b/test/unit/infra/storage/settings-validator.test.ts index 95c6c6faa..0a3aa2d96 100644 --- a/test/unit/infra/storage/settings-validator.test.ts +++ b/test/unit/infra/storage/settings-validator.test.ts @@ -13,10 +13,13 @@ describe('SettingsValidator', () => { it('returns the descriptor for a known key', () => { const descriptor = validator.validateKey('agentPool.maxSize') expect(descriptor.key).to.equal('agentPool.maxSize') - expect(descriptor.type).to.equal('integer') - expect(descriptor.default).to.be.a('number') - expect(descriptor.min).to.be.a('number') - expect(descriptor.max).to.be.a('number') + if (descriptor.type === 'integer') { + expect(descriptor.default).to.be.a('number') + expect(descriptor.min).to.be.a('number') + expect(descriptor.max).to.be.a('number') + } else { + expect.fail('expected integer descriptor for agentPool.maxSize') + } }) it('throws UnknownSettingKeyError for an unknown key', () => { @@ -73,19 +76,31 @@ describe('SettingsValidator', () => { it('throws InvalidSettingValueError when value is above max', () => { const descriptor = validator.validateKey('agentPool.maxSize') - expect(() => validator.validate('agentPool.maxSize', descriptor.max + 1)).to.throw( - InvalidSettingValueError, - ) + if (descriptor.type === 'integer') { + expect(() => validator.validate('agentPool.maxSize', descriptor.max + 1)).to.throw( + InvalidSettingValueError, + ) + } else { + expect.fail('expected integer descriptor') + } }) it('accepts the minimum boundary value', () => { const descriptor = validator.validateKey('agentPool.maxSize') - expect(validator.validate('agentPool.maxSize', descriptor.min)).to.equal(descriptor.min) + if (descriptor.type === 'integer') { + expect(validator.validate('agentPool.maxSize', descriptor.min)).to.equal(descriptor.min) + } else { + expect.fail('expected integer descriptor') + } }) it('accepts the maximum boundary value', () => { const descriptor = validator.validateKey('agentPool.maxSize') - expect(validator.validate('agentPool.maxSize', descriptor.max)).to.equal(descriptor.max) + if (descriptor.type === 'integer') { + expect(validator.validate('agentPool.maxSize', descriptor.max)).to.equal(descriptor.max) + } else { + expect.fail('expected integer descriptor') + } }) it('validates each registered key independently', () => { @@ -213,4 +228,71 @@ describe('SettingsValidator', () => { expect(result.invalid[0].value).to.equal(0) }) }) + + describe('validate (boolean descriptor — T1 update.checkForUpdates)', () => { + it('accepts true', () => { + expect(validator.validate('update.checkForUpdates', true)).to.equal(true) + }) + + it('accepts false', () => { + expect(validator.validate('update.checkForUpdates', false)).to.equal(false) + }) + + it('rejects a numeric value', () => { + try { + validator.validate('update.checkForUpdates', 1) + expect.fail('expected throw') + } catch (error) { + expect(error).to.be.instanceOf(InvalidSettingValueError) + if (error instanceof InvalidSettingValueError) { + expect(error.key).to.equal('update.checkForUpdates') + expect(error.message.toLowerCase()).to.include('boolean') + } + } + }) + + it('rejects a string value', () => { + expect(() => validator.validate('update.checkForUpdates', 'true')).to.throw( + InvalidSettingValueError, + ) + }) + + it('rejects null', () => { + expect(() => validator.validate('update.checkForUpdates', null)).to.throw( + InvalidSettingValueError, + ) + }) + + it('rejects missing values (undefined)', () => { + // Surface a true undefined to the validator via a property access + // rather than a literal `undefined` argument, to avoid the + // unicorn/no-useless-undefined rule on direct argument passing. + const missing: {absent?: unknown} = {} + expect(() => validator.validate('update.checkForUpdates', missing.absent)).to.throw( + InvalidSettingValueError, + ) + }) + }) + + describe('partition (mixed integer + boolean)', () => { + it('puts a valid boolean entry into valid and keeps integer entries beside it', () => { + const result = validator.partition({ + 'agentPool.maxSize': 25, + 'update.checkForUpdates': true, + }) + expect(result.valid).to.deep.equal({ + 'agentPool.maxSize': 25, + 'update.checkForUpdates': true, + }) + expect(result.invalid).to.deep.equal([]) + }) + + it('puts a string masquerading as boolean into invalid', () => { + const result = validator.partition({'update.checkForUpdates': 'yes'}) + expect(result.valid).to.deep.equal({}) + expect(result.invalid).to.have.lengthOf(1) + expect(result.invalid[0].key).to.equal('update.checkForUpdates') + expect(result.invalid[0].value).to.equal('yes') + }) + }) }) diff --git a/test/unit/infra/transport/handlers/settings-handler.test.ts b/test/unit/infra/transport/handlers/settings-handler.test.ts index 027471de9..8b4defc87 100644 --- a/test/unit/infra/transport/handlers/settings-handler.test.ts +++ b/test/unit/infra/transport/handlers/settings-handler.test.ts @@ -89,6 +89,7 @@ describe('SettingsHandler', () => { 'llm.iterationBudgetMs', 'llm.requestTimeoutMs', 'taskHistory.maxEntries', + 'update.checkForUpdates', ]) const maxSizeItem = result.items.find((i) => i.key === 'agentPool.maxSize') expect(maxSizeItem?.current).to.equal(25) @@ -217,6 +218,68 @@ describe('SettingsHandler', () => { expect(result.error.message).to.include('range') } }) + + describe('type pre-validation (T3 invalid_value_type)', () => { + it('rejects a boolean value sent to an integer key', async () => { + const result = await invokeSet({key: 'agentPool.maxSize', value: true}) + + expect(result.ok).to.be.false + if (!result.ok) { + expect(result.error.code).to.equal('invalid_value_type') + expect(result.error.key).to.equal('agentPool.maxSize') + expect(result.error.expected).to.equal('integer') + expect(result.error.got).to.equal('boolean') + } + + // Pre-validation must happen BEFORE the store is touched. + expect(store.calls.filter((c) => c.method === 'set')).to.have.lengthOf(0) + }) + + it('rejects a numeric value sent to a boolean key', async () => { + const result = await invokeSet({key: 'update.checkForUpdates', value: 5}) + + expect(result.ok).to.be.false + if (!result.ok) { + expect(result.error.code).to.equal('invalid_value_type') + expect(result.error.key).to.equal('update.checkForUpdates') + expect(result.error.expected).to.equal('boolean') + expect(result.error.got).to.equal('number') + } + + expect(store.calls.filter((c) => c.method === 'set')).to.have.lengthOf(0) + }) + + it('accepts a boolean value sent to a boolean key and forwards to the store', async () => { + const result = await invokeSet({key: 'update.checkForUpdates', value: false}) + + expect(result.ok).to.be.true + const setCalls = store.calls.filter((c) => c.method === 'set') + expect(setCalls).to.have.lengthOf(1) + expect(setCalls[0].args).to.deep.equal(['update.checkForUpdates', false]) + }) + + it('falls through to unknown_key when the descriptor itself is missing (does not pre-validate type)', async () => { + store.setBehavior = async (key) => { + throw new UnknownSettingKeyError(key) + } + + const result = await invokeSet({key: 'not.a.real.key', value: 1}) + + expect(result.ok).to.be.false + if (!result.ok) expect(result.error.code).to.equal('unknown_key') + }) + + it('still surfaces a range violation as invalid_value (not invalid_value_type)', async () => { + store.setBehavior = async (key, value) => { + throw new InvalidSettingValueError(key, value, 'value 0 is outside allowed range [1, 100]') + } + + const result = await invokeSet({key: 'agentPool.maxSize', value: 0}) + + expect(result.ok).to.be.false + if (!result.ok) expect(result.error.code).to.equal('invalid_value') + }) + }) }) describe('RESET', () => { diff --git a/test/unit/oclif/lib/check-for-updates-setting.test.ts b/test/unit/oclif/lib/check-for-updates-setting.test.ts new file mode 100644 index 000000000..a430dda85 --- /dev/null +++ b/test/unit/oclif/lib/check-for-updates-setting.test.ts @@ -0,0 +1,88 @@ +import {expect} from 'chai' +import {mkdirSync, mkdtempSync, rmSync, writeFileSync} from 'node:fs' +import {tmpdir} from 'node:os' +import {join} from 'node:path' + +import {checkForUpdatesSetting} from '../../../../src/oclif/lib/check-for-updates-setting.js' + +describe('checkForUpdatesSetting (T7)', () => { + let tempDir: string + let priorBrvDataDir: string | undefined + + beforeEach(() => { + priorBrvDataDir = process.env.BRV_DATA_DIR + tempDir = mkdtempSync(join(tmpdir(), 'brv-checkforupdates-')) + process.env.BRV_DATA_DIR = tempDir + }) + + afterEach(() => { + rmSync(tempDir, {force: true, recursive: true}) + if (priorBrvDataDir === undefined) delete process.env.BRV_DATA_DIR + else process.env.BRV_DATA_DIR = priorBrvDataDir + }) + + it('returns true (default) when the settings file is missing', () => { + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns true when the settings file exists but the key is absent', () => { + writeFileSync(join(tempDir, 'settings.json'), JSON.stringify({values: {'agentPool.maxSize': 25}, version: '2'})) + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns true when the key is explicitly set to true', () => { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': true}, version: '2'}), + ) + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns false when the key is explicitly set to false', () => { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': false}, version: '2'}), + ) + expect(checkForUpdatesSetting()).to.equal(false) + }) + + it('returns true (fallback) when the file is invalid JSON', () => { + writeFileSync(join(tempDir, 'settings.json'), 'this is not json') + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns true (fallback) when the top-level JSON is not an object', () => { + writeFileSync(join(tempDir, 'settings.json'), JSON.stringify([1, 2, 3])) + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns true (fallback) when values is not an object', () => { + writeFileSync(join(tempDir, 'settings.json'), JSON.stringify({values: 'not-an-object', version: '2'})) + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('returns true (fallback) when the value is the wrong type (string)', () => { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': 'no'}, version: '2'}), + ) + expect(checkForUpdatesSetting()).to.equal(true) + }) + + it('reads from a v1 file the same way (key may or may not exist)', () => { + writeFileSync( + join(tempDir, 'settings.json'), + JSON.stringify({values: {'update.checkForUpdates': false}, version: '1'}), + ) + expect(checkForUpdatesSetting()).to.equal(false) + }) + + it('falls back to true when BRV_DATA_DIR points to a non-existent directory', () => { + const missing = join(tempDir, 'does-not-exist') + process.env.BRV_DATA_DIR = missing + // sanity: nothing on disk + expect(() => mkdirSync(missing)).to.not.throw() + rmSync(missing, {force: true, recursive: true}) + expect(checkForUpdatesSetting()).to.equal(true) + }) +}) diff --git a/test/unit/shared/utils/format-settings.test.ts b/test/unit/shared/utils/format-settings.test.ts index 9c5a52c52..8c9ea961e 100644 --- a/test/unit/shared/utils/format-settings.test.ts +++ b/test/unit/shared/utils/format-settings.test.ts @@ -20,6 +20,18 @@ function makeItem(overrides: Partial = {}): SettingsItemDTO { } } +function makeBooleanItem(current: boolean): SettingsItemDTO { + return { + category: 'updates', + current, + default: true, + description: 'Check for brv updates at startup and notify when one is available', + key: 'update.checkForUpdates', + restartRequired: false, + type: 'boolean', + } +} + function makeRow(overrides: Partial = {}): SettingsRow { return { category: 'concurrency', @@ -180,4 +192,65 @@ describe('format-settings (shared)', () => { expect(parseRowInput(row, ' ').kind).to.equal('error') }) }) + + describe('boolean rows (T5)', () => { + it('includes boolean items in the output (no longer filtered)', () => { + const rows = buildSettingsRows([makeBooleanItem(true)]) + expect(rows).to.have.lengthOf(1) + expect(rows[0].key).to.equal('update.checkForUpdates') + }) + + it('formats current=true as "[ on ]" and current=false as "[ off ]"', () => { + const onRow = buildSettingsRows([makeBooleanItem(true)])[0] + const offRow = buildSettingsRows([makeBooleanItem(false)])[0] + expect(onRow.displayCurrent).to.equal('[ on ]') + expect(offRow.displayCurrent).to.equal('[ off ]') + }) + + it('formats default=true the same way as current', () => { + const row = buildSettingsRows([makeBooleanItem(false)])[0] + expect(row.displayDefault).to.equal('[ on ]') + }) + + it('emits an empty displayRange for boolean rows (no min/max to render)', () => { + const row = buildSettingsRows([makeBooleanItem(true)])[0] + expect(row.displayRange).to.equal('') + }) + + it('preserves the "updates" category on the row', () => { + const row = buildSettingsRows([makeBooleanItem(true)])[0] + expect(row.category).to.equal('updates') + }) + + it('marks the row as modified when current differs from default', () => { + // current=false but default=true (per the helper) -> dirty + const row = buildSettingsRows([makeBooleanItem(false)])[0] + expect(row.modified).to.equal(true) + }) + + it('propagates restartRequired=false from the DTO onto the row', () => { + const row = buildSettingsRows([makeBooleanItem(true)])[0] + expect(row.restartRequired).to.equal(false) + }) + + it('keeps integer rows working when mixed with boolean rows', () => { + const rows = buildSettingsRows([ + makeItem({current: 25, key: 'agentPool.maxSize'}), + makeBooleanItem(true), + ]) + expect(rows.map((r) => r.key)).to.have.members(['agentPool.maxSize', 'update.checkForUpdates']) + const integerRow = rows.find((r) => r.key === 'agentPool.maxSize') + expect(integerRow?.displayCurrent).to.equal('25') + expect(integerRow?.displayRange).to.equal('1-100') + }) + + it('orders the updates category after task-history', () => { + const rows = buildSettingsRows([ + makeBooleanItem(true), + makeItem({category: 'task-history', key: 'taskHistory.maxEntries'}), + makeItem({category: 'concurrency', key: 'agentPool.maxSize'}), + ]) + expect(rows.map((r) => r.category)).to.deep.equal(['concurrency', 'task-history', 'updates']) + }) + }) }) diff --git a/test/unit/tui/features/settings/format-settings.test.ts b/test/unit/tui/features/settings/format-settings.test.ts index e48783f13..36dc8d01b 100644 --- a/test/unit/tui/features/settings/format-settings.test.ts +++ b/test/unit/tui/features/settings/format-settings.test.ts @@ -40,6 +40,44 @@ describe('format-settings (tui)', () => { expect(groups[0].rows).to.have.lengthOf(1) expect(groups[1].rows).to.have.lengthOf(1) }) + + it('renders an UPDATES header when an updates-category row is present (T5)', () => { + const groups = groupRowsByCategory([ + makeRow({ + category: 'updates', + current: true, + default: true, + displayCurrent: '[ on ]', + displayDefault: '[ on ]', + displayRange: '', + key: 'update.checkForUpdates', + label: 'update.checkForUpdates', + restartRequired: false, + type: 'boolean', + }), + ]) + expect(groups.map((g) => g.header)).to.deep.equal(['UPDATES']) + expect(groups[0].rows[0].key).to.equal('update.checkForUpdates') + }) + + it('orders updates after task-history when both groups are present (T5)', () => { + const groups = groupRowsByCategory([ + makeRow({ + category: 'updates', + current: true, + default: true, + displayCurrent: '[ on ]', + displayDefault: '[ on ]', + displayRange: '', + key: 'update.checkForUpdates', + label: 'update.checkForUpdates', + restartRequired: false, + type: 'boolean', + }), + makeRow({category: 'task-history', key: 'taskHistory.maxEntries'}), + ]) + expect(groups.map((g) => g.header)).to.deep.equal(['TASK HISTORY', 'UPDATES']) + }) }) describe('bottomHintFor', () => { diff --git a/test/unit/webui/features/settings/stores/restart-banner-store.test.ts b/test/unit/webui/features/settings/stores/restart-banner-store.test.ts index eb30c1e00..ac4b55b9d 100644 --- a/test/unit/webui/features/settings/stores/restart-banner-store.test.ts +++ b/test/unit/webui/features/settings/stores/restart-banner-store.test.ts @@ -11,34 +11,61 @@ describe('useRestartBannerStore', () => { expect(useRestartBannerStore.getState().dirtyKeys.size).to.equal(0) }) - it('markDirty adds the key to the dirty set', () => { - useRestartBannerStore.getState().markDirty('agentPool.maxSize') + it('markDirty adds the key to the dirty set when restartRequired is true', () => { + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) expect(useRestartBannerStore.getState().dirtyKeys.has('agentPool.maxSize')).to.equal(true) }) it('markDirty is idempotent — same key twice yields size 1', () => { - useRestartBannerStore.getState().markDirty('agentPool.maxSize') - useRestartBannerStore.getState().markDirty('agentPool.maxSize') + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) expect(useRestartBannerStore.getState().dirtyKeys.size).to.equal(1) }) it('markDirty tracks multiple distinct keys', () => { - useRestartBannerStore.getState().markDirty('agentPool.maxSize') - useRestartBannerStore.getState().markDirty('llm.iterationBudgetMs') + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) + useRestartBannerStore.getState().markDirty('llm.iterationBudgetMs', true) expect(useRestartBannerStore.getState().dirtyKeys.size).to.equal(2) }) it('clear empties the dirty set', () => { - useRestartBannerStore.getState().markDirty('agentPool.maxSize') - useRestartBannerStore.getState().markDirty('llm.iterationBudgetMs') + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) + useRestartBannerStore.getState().markDirty('llm.iterationBudgetMs', true) useRestartBannerStore.getState().clear() expect(useRestartBannerStore.getState().dirtyKeys.size).to.equal(0) }) - it('produces a new Set instance on each mutation so React selectors detect the change', () => { + it('produces a new Set instance on each restart-required mutation so React selectors detect the change', () => { const before = useRestartBannerStore.getState().dirtyKeys - useRestartBannerStore.getState().markDirty('agentPool.maxSize') + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) const after = useRestartBannerStore.getState().dirtyKeys expect(after).to.not.equal(before) }) + + it('markDirty with restartRequired false does not add the key to the dirty set', () => { + useRestartBannerStore.getState().markDirty('update.checkForUpdates', false) + expect(useRestartBannerStore.getState().dirtyKeys.size).to.equal(0) + }) + + it('markDirty with restartRequired false preserves Set identity so React selectors do not re-fire', () => { + const before = useRestartBannerStore.getState().dirtyKeys + useRestartBannerStore.getState().markDirty('update.checkForUpdates', false) + const after = useRestartBannerStore.getState().dirtyKeys + expect(after).to.equal(before) + }) + + it('mixed sequence: only restart-required keys land in the dirty set', () => { + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) + useRestartBannerStore.getState().markDirty('update.checkForUpdates', false) + const {dirtyKeys} = useRestartBannerStore.getState() + expect(dirtyKeys.size).to.equal(1) + expect(dirtyKeys.has('agentPool.maxSize')).to.equal(true) + expect(dirtyKeys.has('update.checkForUpdates')).to.equal(false) + }) + + it('a later non-restart change for the same key does not unset its dirty marker', () => { + useRestartBannerStore.getState().markDirty('agentPool.maxSize', true) + useRestartBannerStore.getState().markDirty('agentPool.maxSize', false) + expect(useRestartBannerStore.getState().dirtyKeys.has('agentPool.maxSize')).to.equal(true) + }) })