From 55e0f0e7e40195881d3c5b400ff91c6e0ccefcee Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 8 Nov 2026 09:15:53 +0100 Subject: [PATCH] feat(agent-skill): switch a stored off from before 1.50.0 on, once per data dir Every App Settings Save up to 1.49.0 wrote `agentSkillEnabled: false`, chosen or not (the switch loaded an absent key as off and Save sends the whole form), so the new default never reached an install that had ever saved. On its first start the server now replaces a stored false with true, once per data dir, and logs one line when it does. It cannot tell a deliberate off from the old default; that is accepted and stated in the release notes. An off set after it ran stays off. It stores true rather than deleting the key: the page's merge overwrites only the synced keys the server has, so every device that saved on 1.49 kept its own false, showed the switch off and wrote false back on its next Save (measured in the JSDOM harness). True wins on every device, including a browser still on a cached settings-ui.js. For a server with no key the page now drops the device's copy as well, so the switch shows the default. The "done" marker is its own file, `agent-skill-default-migrated.json` in the data dir: it is not a setting, so GET /api/settings never returns it, no Save can send it back (the strict schema would refuse it) or drop it (which would re-arm the migration), and a config-backup restore leaves it alone. It is checked before anything is read and written only after the settings write landed. The migration runs in WebServer.start() before the first settings read and before listen (skipped in test mode, like the other boot-time writes), writes only through mutateSettingsFile(), drops the 2 s settings cache, and leaves a settings.json that does not parse alone with no marker, so the next start after the fix runs it. Co-Authored-By: Claude Opus 5.5 --- src/web/agent-skill-default-migration.ts | 106 +++++++++++ src/web/public/settings-ui.js | 7 + src/web/server.ts | 9 + test/agent-skill-default-migration.test.ts | 211 +++++++++++++++++++++ test/agent-skill-setting-ui.test.ts | 52 +++++ 5 files changed, 385 insertions(+) create mode 100644 src/web/agent-skill-default-migration.ts create mode 100644 test/agent-skill-default-migration.test.ts diff --git a/src/web/agent-skill-default-migration.ts b/src/web/agent-skill-default-migration.ts new file mode 100644 index 00000000..0cd25c1d --- /dev/null +++ b/src/web/agent-skill-default-migration.ts @@ -0,0 +1,106 @@ +/** + * @fileoverview One-time boot migration that lets the 1.50.0 agent skill default reach installs + * that saved App Settings before it. + * + * `agentSkillEnabled` is ON by default since 1.50.0 (an absent key is on, only an explicit false + * is off). Up to 1.49.0 the App Settings switch loaded an absent key as off (`?? false`) and Save + * sends the whole form, so every install whose App Settings were ever saved holds an explicit + * `false` the user may never have chosen, and the new default would never reach it. On the first + * start of a server that has this module, a stored `false` is switched on, exactly once per data + * dir. It cannot tell a deliberate off from the old default; that is accepted and stated as a + * behaviour change in the release notes. An off set after this ran is never touched again. + * + * Why a stored `false` becomes `true` rather than being deleted: deleting it would not reach the + * page. Every browser that saved App Settings keeps the form in its localStorage blob, and + * `loadAppSettingsFromServer()` only overwrites a synced key the server HAS: a key the server + * lacks keeps the device's `false`, the switch loads off, and the next Save of any setting writes + * `false` back (test/agent-skill-setting-ui.test.ts proves both shapes). A stored `true` wins on + * every device, including a browser still running a cached settings-ui.js from before the upgrade. + * Every reader treats `true` and an absent key alike (`!== false`). + * + * Why the "done" marker is its own file (`agent-skill-default-migrated.json` in the data dir) + * and not a key in settings.json: + * - it is not a setting, so `GET /api/settings` never returns it, the page never holds it, and + * the page's Save (rebuilt from the form) can neither send it back nor drop it; the `.strict()` + * `SettingsUpdateSchema` never sees it; + * - a PUT or any other settings.json writer cannot erase it, which would re-arm the migration and + * switch a deliberate later off back on; + * - it is not in `CONFIG_BACKUP_FILES` (like `config-backup-id`), so a restore leaves it alone: a + * restored settings.json, even one from before 1.50.0 holding `false`, is what the user asked + * for and stays as restored. + * A data dir is one instance, and settings.json is one file for the whole server, so in + * multi-user mode this runs once for the server, never per user. + * + * Order: the marker is checked first (present: nothing is read), the settings change goes through + * `mutateSettingsFile()` (serialized, atomic, refuses a file that does not parse), and the marker + * is written only after that resolves. A refused or failed settings write writes no marker, so the + * migration runs again at the next start (after the user fixed the file). A crash between the two + * writes is harmless: the next start finds `true` (nothing to change) and writes the marker. + * + * Called once from `WebServer.start()`, before the first settings read and before the server + * listens, so no request path can see the old value. + */ + +import fs from 'node:fs/promises'; +import { dirname } from 'node:path'; +import { dataPath } from '../config/instance.js'; +import { getErrorMessage } from '../types.js'; +import { mutateSettingsFile, SettingsWriteRefusedError } from './settings-writer.js'; + +/** The marker's file name in the data dir. Never in CONFIG_BACKUP_FILES (see the file comment). */ +export const AGENT_SKILL_DEFAULT_MARKER = 'agent-skill-default-migrated.json'; + +/** + * - `switched-on`: settings.json held `agentSkillEnabled: false`; it now holds `true`. + * - `nothing-to-change`: the key was absent or not `false` (or there is no settings.json); nothing was written to it. + * - `already-done`: the marker exists; settings.json was not read. + * - `refused`: settings.json could not be read, parsed or written, or the marker could not be checked. + * Nothing changed and no marker was written, so the next start tries again. + */ +export type AgentSkillDefaultMigration = 'switched-on' | 'nothing-to-change' | 'already-done' | 'refused'; + +/** The log line of a migration that switched the skill on. */ +export const AGENT_SKILL_DEFAULT_MIGRATED_LOG = + '[agent-skill] cleared the stored off from before 1.50.0 so the new default applies (turn it off again under App Settings, Agents & CLIs, Codeman Agent Skill; that choice is kept)'; + +export async function migrateAgentSkillDefault( + markerPath: string = dataPath(AGENT_SKILL_DEFAULT_MARKER) +): Promise { + try { + await fs.lstat(markerPath); + return 'already-done'; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { + // Not knowing whether it ran must not switch a deliberate off on a second time. + console.warn(`[agent-skill] default migration skipped: cannot check ${markerPath} (${getErrorMessage(err)})`); + return 'refused'; + } + } + + let switched = false; + try { + await mutateSettingsFile((current) => { + if (current.agentSkillEnabled !== false) return undefined; + switched = true; + return { ...current, agentSkillEnabled: true }; + }); + } catch (err) { + // A refusal is already logged by the writer (`[settings] write refused: ...`), with the fix. + const why = err instanceof SettingsWriteRefusedError ? 'settings.json was left as it is' : getErrorMessage(err); + console.warn(`[agent-skill] default migration skipped (${why}); it runs again at the next start`); + return 'refused'; + } + + try { + await fs.mkdir(dirname(markerPath), { recursive: true }); + const marker = { migratedAt: new Date().toISOString(), switchedOn: switched }; + await fs.writeFile(markerPath, `${JSON.stringify(marker, null, 2)}\n`, { mode: 0o600 }); + } catch (err) { + // settings.json is already right; without the marker the next start runs again, which + // changes nothing unless the user has switched it off by then. + console.warn(`[agent-skill] could not record the default migration at ${markerPath}: ${getErrorMessage(err)}`); + } + + if (switched) console.log(AGENT_SKILL_DEFAULT_MIGRATED_LOG); + return switched ? 'switched-on' : 'nothing-to-change'; +} diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 214244f8..21ee5037 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -5268,6 +5268,13 @@ Object.assign(CodemanApp.prototype, { merged[key] = value; } } + // The agent skill is a synced key that is ON when absent (since 1.50.0). A server + // without the key means that default, so this device's own copy goes rather than + // standing in for it: up to 1.49.0 the switch loaded an absent key as off, so a + // device whose save never reached the server still holds `false`, and its next + // Save would write it back. The server's one-time boot migration stores `true` for + // installs that had saved `false` (agent-skill-default-migration.ts). + if (!('agentSkillEnabled' in appSettings)) delete merged.agentSkillEnabled; this.saveAppSettingsToStorage(merged); // Apply notification prefs from server if present (only if localStorage has none) diff --git a/src/web/server.ts b/src/web/server.ts index d75a4385..d56ba7d4 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -195,6 +195,7 @@ import { resolveTerminalHistoryConfig } from '../config/terminal-history.js'; import { SseEvent } from './sse-events.js'; import { MAX_ROUTE_PARAM_LENGTH } from './route-helpers.js'; import { mutateSettingsFile } from './settings-writer.js'; +import { migrateAgentSkillDefault } from './agent-skill-default-migration.js'; import { getLatestPlanUsage, setLatestCodexPlanUsage, setLatestCopilotPlanUsage } from './plan-usage-latest.js'; import { telemetrySignature } from '../usage-telemetry.js'; import { readCodexPlanUsage, resolveCodexBinaryPath } from '../utils/codex-cli-resolver.js'; @@ -3401,6 +3402,14 @@ export class WebServer extends EventEmitter { reconcileUpdateOnBoot(); } + // One-time, once per data dir: a stored `agentSkillEnabled: false` written by the switch's + // old default (up to 1.49.0) becomes true, so the 1.50.0 default reaches installs that ever + // saved App Settings (agent-skill-default-migration.ts). Before the restore below reads + // settings (2 s cache) and before listen, so no session create can see the old value. + if (!this.testMode && (await migrateAgentSkillDefault()) === 'switched-on') { + this._settingsCache = null; + } + // Restore mux sessions BEFORE accepting connections // This prevents race conditions where clients connect before state is ready // CRITICAL: Skip in test mode to prevent tests from picking up user sessions diff --git a/test/agent-skill-default-migration.test.ts b/test/agent-skill-default-migration.test.ts new file mode 100644 index 00000000..aabfabf3 --- /dev/null +++ b/test/agent-skill-default-migration.test.ts @@ -0,0 +1,211 @@ +/** + * @fileoverview The one-time boot migration that lets the 1.50.0 agent skill default reach + * installs that saved App Settings before it (src/web/agent-skill-default-migration.ts). + * + * What is pinned, and why it matters: + * - An absent key, a stored `true` or no settings.json at all: settings.json is not written + * (byte-identical, odd formatting kept) and the marker is set. + * - A stored `false` (what every 1.49 save wrote, chosen or not) becomes `true`, every other key + * kept, exactly once: the log line prints once, and a second start reports `already-done` + * without reading the file. + * - An off set AFTER the migration stays off: through the real `PUT /api/settings` (what the + * page's Save sends), then a second start. The PUT neither writes nor drops the marker, and + * `GET /api/settings` never returns it, so the page cannot hold it to send it back. + * - A settings.json that does not parse is left byte-identical, no marker is written, nothing + * throws, and the next start (after the fix) does the migration. + * - A marker that cannot be checked switches nothing on (a deliberate off must not be cleared + * twice). + * - The marker is not a config-backup file, and `WebServer.start()` runs the migration before + * its first settings read and before it listens (source wiring guard: the boot call is skipped + * in test mode, like the other boot-time writes). + * + * The page's side (a device blob holding the old `false`) is pinned in + * test/agent-skill-setting-ui.test.ts. + * + * Port: 0 (one WebServer for the PUT/GET cases). + */ + +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { WebServer } from '../src/web/server.js'; +import { TmuxManager } from '../src/tmux-manager.js'; +import { SETTINGS_PATH } from '../src/web/route-helpers.js'; +import { dataPath } from '../src/config/instance.js'; +import { CONFIG_BACKUP_FILES } from '../src/config-backup.js'; +import { + AGENT_SKILL_DEFAULT_MARKER, + AGENT_SKILL_DEFAULT_MIGRATED_LOG, + migrateAgentSkillDefault, +} from '../src/web/agent-skill-default-migration.js'; + +vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true); + +const MARKER = dataPath(AGENT_SKILL_DEFAULT_MARKER); + +/** Written with a formatting no writer of ours produces, so any rewrite shows in the bytes. */ +const seedRaw = (text: string) => writeFileSync(SETTINGS_PATH, text); +const seed = (value: unknown) => seedRaw(JSON.stringify(value)); +const raw = () => readFileSync(SETTINGS_PATH, 'utf-8'); +const stored = () => JSON.parse(raw()) as Record; +const marker = () => JSON.parse(readFileSync(MARKER, 'utf-8')) as { migratedAt: string; switchedOn: boolean }; + +/** The bytes of a torn settings.json (test/settings-json-write-serialization.test.ts). */ +const TORN = '{\n "agentSkillEnabled": false\n}' + 'rt-1761000000000"\n}'; + +let logSpy: ReturnType; +let warnSpy: ReturnType; +const migratedLogs = () => logSpy.mock.calls.filter(([line]) => line === AGENT_SKILL_DEFAULT_MIGRATED_LOG).length; + +beforeEach(() => { + mkdirSync(dirname(SETTINGS_PATH), { recursive: true }); + rmSync(SETTINGS_PATH, { force: true }); + rmSync(MARKER, { force: true }); + logSpy = vi.spyOn(console, 'log'); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); +}); + +afterEach(() => { + logSpy.mockRestore(); + warnSpy.mockRestore(); +}); + +describe('nothing to switch on', () => { + it('an absent key: settings.json is not written, the marker is set', async () => { + seed({ lastUsedCase: 'demo' }); + const before = raw(); + expect(await migrateAgentSkillDefault()).toBe('nothing-to-change'); + expect(raw()).toBe(before); + expect(marker()).toMatchObject({ switchedOn: false }); + expect(migratedLogs()).toBe(0); + }); + + it('no settings.json at all: none is created, the marker is set', async () => { + expect(await migrateAgentSkillDefault()).toBe('nothing-to-change'); + expect(existsSync(SETTINGS_PATH)).toBe(false); + expect(marker()).toMatchObject({ switchedOn: false }); + }); + + it('a stored true is left untouched', async () => { + seed({ agentSkillEnabled: true, caseOrder: ['a'] }); + const before = raw(); + expect(await migrateAgentSkillDefault()).toBe('nothing-to-change'); + expect(raw()).toBe(before); + expect(marker()).toMatchObject({ switchedOn: false }); + }); +}); + +describe('a stored false from before 1.50.0', () => { + it('is switched on once, every other key kept, with one log line', async () => { + seed({ agentSkillEnabled: false, lastUsedCase: 'demo', nice: { enabled: true, niceValue: 5 } }); + expect(await migrateAgentSkillDefault()).toBe('switched-on'); + expect(stored()).toEqual({ agentSkillEnabled: true, lastUsedCase: 'demo', nice: { enabled: true, niceValue: 5 } }); + expect(marker()).toMatchObject({ switchedOn: true }); + expect(Number.isNaN(Date.parse(marker().migratedAt))).toBe(false); + expect(migratedLogs()).toBe(1); + + // A second start finds the marker and reads nothing. + const after = raw(); + expect(await migrateAgentSkillDefault()).toBe('already-done'); + expect(raw()).toBe(after); + expect(migratedLogs()).toBe(1); + }); + + it('a false written by hand after the migration is never switched on again', async () => { + seed({ agentSkillEnabled: false }); + expect(await migrateAgentSkillDefault()).toBe('switched-on'); + seed({ agentSkillEnabled: false }); + expect(await migrateAgentSkillDefault()).toBe('already-done'); + expect(stored()).toEqual({ agentSkillEnabled: false }); + }); +}); + +describe('a settings.json that does not parse', () => { + it('is left alone, writes no marker, does not throw, and the next start after the fix migrates', async () => { + seedRaw(TORN); + expect(await migrateAgentSkillDefault()).toBe('refused'); + expect(raw()).toBe(TORN); + expect(existsSync(MARKER)).toBe(false); + expect( + warnSpy.mock.calls.some(([line]) => String(line).startsWith('[agent-skill] default migration skipped')) + ).toBe(true); + + // The user repairs the file (or restores a backup): the migration has not run yet, so it runs now. + seed({ agentSkillEnabled: false }); + expect(await migrateAgentSkillDefault()).toBe('switched-on'); + expect(stored()).toEqual({ agentSkillEnabled: true }); + }); +}); + +describe('a marker that cannot be checked', () => { + it('switches nothing on and writes no marker', async () => { + seed({ agentSkillEnabled: false }); + // A path below a regular file: lstat answers ENOTDIR, not ENOENT. + expect(await migrateAgentSkillDefault(join(SETTINGS_PATH, 'marker.json'))).toBe('refused'); + expect(stored()).toEqual({ agentSkillEnabled: false }); + expect(existsSync(MARKER)).toBe(false); + }); +}); + +describe('with the page and the server (port 0)', () => { + let server: WebServer; + const url = (p: string) => `http://localhost:${server.boundPort}${p}`; + + beforeAll(async () => { + server = new WebServer(0, false, true); + await server.start(); + }, 30000); + + afterAll(async () => { + await server?.stop(); + }, 30000); + + it('an off saved through PUT /api/settings after the migration stays off at the next start, and the PUT leaves the marker alone', async () => { + seed({ agentSkillEnabled: false, lastUsedCase: 'demo' }); + expect(await migrateAgentSkillDefault()).toBe('switched-on'); + const markerBytes = readFileSync(MARKER, 'utf-8'); + + // What the page sends when the user turns the switch off (plus another synced key). + const res = await fetch(url('/api/settings'), { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ agentSkillEnabled: false, autoNameSessions: true }), + }); + expect(res.status).toBe(200); + expect(stored()).toEqual({ agentSkillEnabled: false, lastUsedCase: 'demo', autoNameSessions: true }); + expect(readFileSync(MARKER, 'utf-8')).toBe(markerBytes); + + expect(await migrateAgentSkillDefault()).toBe('already-done'); + expect(stored().agentSkillEnabled).toBe(false); + }); + + it('GET /api/settings never carries the marker, so the page cannot hold or send it', async () => { + seed({ agentSkillEnabled: false }); + await migrateAgentSkillDefault(); + const res = await fetch(url('/api/settings')); + const body = (await res.json()) as Record; + const data = (body.success === true ? body.data : body) as Record; + expect(data).toEqual({ agentSkillEnabled: true }); + }); +}); + +describe('placement', () => { + it('the marker is not a config-backup file, so a restore never re-arms or drops it', () => { + expect(CONFIG_BACKUP_FILES as readonly string[]).not.toContain(AGENT_SKILL_DEFAULT_MARKER); + }); + + it('WebServer.start() runs it before its first settings read, the restore and listen, and drops the settings cache', () => { + const source = readFileSync(resolve(import.meta.dirname, '../src/web/server.ts'), 'utf8'); + const start = source.slice(source.indexOf(' async start(): Promise {')); + const call = start.indexOf('await migrateAgentSkillDefault()'); + expect(call).toBeGreaterThan(0); + for (const later of [ + 'await this.getAgentInboxPersistEnabled()', + 'await this.restoreMuxSessions()', + 'await this.app.listen(', + ]) { + expect(start.indexOf(later), later).toBeGreaterThan(call); + } + expect(start.slice(call, call + 200)).toContain('this._settingsCache = null;'); + }); +}); diff --git a/test/agent-skill-setting-ui.test.ts b/test/agent-skill-setting-ui.test.ts index a62869dc..05ff2563 100644 --- a/test/agent-skill-setting-ui.test.ts +++ b/test/agent-skill-setting-ui.test.ts @@ -11,6 +11,10 @@ * device's blob or the server's settings.json, merged as a synced key). * - saveAppSettings() sends the switch in the settings PUT, and the body passes the * .strict() SettingsUpdateSchema (an undeclared key 400s every save). + * - A device that saved on 1.49.0 or earlier keeps `false` in its own blob. After the + * server's one-time boot migration (stored true) its switch shows on and a Save keeps it + * on; on a server without the key the device's copy is dropped for the default; a later + * explicit off on the server stays off. The Save body never carries the migration marker. * - The row says it is on by default, that the copy stays out of git, and how to * remove copies already written (turning it off sweeps nothing). * @@ -173,6 +177,54 @@ describe('Save', () => { }); }); +// Every device that saved App Settings on 1.49.0 or earlier holds `agentSkillEnabled: false` in +// its own blob (the switch loaded an absent key as off). The server's boot migration +// (src/web/agent-skill-default-migration.ts) switches a stored false on by storing TRUE, because +// the merge only overwrites a synced key the server has: a key it deleted would leave the +// device's false in place, and the next Save would write it back. +describe('a device that saved before 1.50.0', () => { + const OLD_BLOB = { agentSkillEnabled: false, autoNameSessions: false }; + + it("after the server's migration (stored true) the switch shows on, and a Save keeps it on and passes the schema", async () => { + const { app, checkbox, putBody } = await boot(OLD_BLOB); + await app.loadAppSettingsFromServer(Promise.resolve({ agentSkillEnabled: true, lastUsedCase: 'demo' })); + app.openAppSettings(); + expect(checkbox().checked).toBe(true); + await app.saveAppSettings(); + const body = putBody(); + expect(body.agentSkillEnabled).toBe(true); + // The migration's marker is a file of its own, never a setting: the Save can neither send it + // back nor be refused for it by the .strict() schema. + expect(Object.keys(body).filter((key) => /migrat/i.test(key))).toEqual([]); + expect(SettingsUpdateSchema.safeParse(body).error?.issues ?? []).toEqual([]); + }); + + it('on a server without the key (a save that never reached it) the switch shows the default, on', async () => { + const { app, checkbox, putBody } = await boot(OLD_BLOB); + await app.loadAppSettingsFromServer(Promise.resolve({ lastUsedCase: 'demo' })); + app.openAppSettings(); + expect(checkbox().checked).toBe(true); + await app.saveAppSettings(); + expect(putBody().agentSkillEnabled).toBe(true); + }); + + it('an off the user saved after the migration stays off on every device', async () => { + const { app, checkbox, putBody } = await boot({ agentSkillEnabled: true }); + await app.loadAppSettingsFromServer(Promise.resolve({ agentSkillEnabled: false })); + app.openAppSettings(); + expect(checkbox().checked).toBe(false); + await app.saveAppSettings(); + expect(putBody().agentSkillEnabled).toBe(false); + }); + + it('without a server answer the device keeps what it has', async () => { + const { app, checkbox } = await boot(OLD_BLOB); + await app.loadAppSettingsFromServer(Promise.resolve(null as unknown as Settings)); + app.openAppSettings(); + expect(checkbox().checked).toBe(false); + }); +}); + describe('the App Settings row', () => { const doc = new JSDOM(INDEX).window.document; const row = doc.getElementById('appSettingsAgentSkill')!.closest('.set-row') as HTMLElement;