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;