Merge #787: turn the Codeman agent skill on by default

src/web/agent-skill-default-migration.ts

@@ -0,0 +1,110 @@
+/**
+ * @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 (measured with the real page code in the JSDOM harness of
+ * test/agent-skill-setting-ui.test.ts, which pins the shipped shape). 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:
+ *    once the migration has run, 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). Until then it has not
+ * run, so an off saved after the fix and before that start is switched on with the rest: the same
+ * "cannot tell" trade as above, in a window that needs an unparseable settings.json at the first
+ * 1.50.0 start. 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] switched on the off stored 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<AgentSkillDefaultMigration> {
+  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';
+}

src/web/ports/config-port.ts

@@ -17,7 +17,7 @@ export interface ConfigPort {
   getModelConfig(): Promise<{ defaultModel?: string; agentTypeOverrides?: Record<string, string> } | null>;
   getClaudeModeConfig(): Promise<{ claudeMode?: ClaudeMode; allowedTools?: string }>;
   getTerminalHistoryConfig(): Promise<TerminalHistoryConfig>;
-  /** Synced `agentSkillEnabled` app setting (default OFF); gates per-case agent-skill injection. */
+  /** Synced `agentSkillEnabled` app setting (default ON, only an explicit false is off); gates per-case agent-skill injection. */
   getAgentSkillEnabled(): Promise<boolean>;
   /** Synced `workspaceHooksEnabled` app setting (default ON); gates INSTALLING hooks into a session's workspace. */
   getWorkspaceHooksEnabled(): Promise<boolean>;

src/web/public/index.html

@@ -2973,7 +2973,7 @@
                   <div class="set-row" data-search="agent skill codeman api workers codex gemini opencode pi grok deepseek omp antigravity">
                     <div class="set-row-text">
                       <span class="set-row-label">Codeman Agent Skill</span>
-                      <span class="set-row-desc">Write Codeman's agent skill into each new session's folder, where that CLI looks for skills, so the agent in it can start workers in any installed CLI, send them prompts and wait for results. Every CLI except Pi and a plain shell.</span>
+                      <span class="set-row-desc">Write Codeman's agent skill into each new session's folder, where that CLI looks for skills, so the agent in it can start workers in any installed CLI, send them prompts and wait for results. Every CLI except Pi, GitHub Copilot and a plain shell. On by default. The copy ignores itself in git, so your repository never lists it as a new file. Off stops new copies but removes none: <code>codeman skill uninstall --case &lt;name&gt;</code> does that.</span>
                     </div>
                     <label class="switch switch-sm"><input type="checkbox" id="appSettingsAgentSkill"><span class="slider"></span></label>
                   </div>

src/web/public/settings-ui.js

@@ -711,7 +711,9 @@ Object.assign(CodemanApp.prototype, {
     this._applyCodexSettingsVisibility();
     // Claude Permissions settings
     document.getElementById('appSettingsAgentTeams').checked = settings.agentTeamsEnabled ?? false;
-    document.getElementById('appSettingsAgentSkill').checked = settings.agentSkillEnabled ?? false;
+    // Default ON (since 1.50.0), like the workspace hooks below: the server reads an
+    // absent key as on, so the switch must too, or the next Save would write false.
+    document.getElementById('appSettingsAgentSkill').checked = settings.agentSkillEnabled !== false;
     // Default ON: an absent key is a user who has never seen this setting, and OFF
     // for them means no tab alerts in any workspace Codeman did not scaffold.
     document.getElementById('appSettingsWorkspaceHooks').checked = settings.workspaceHooksEnabled !== false;
@@ -5270,6 +5272,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)

src/web/routes/session-routes.ts

@@ -979,8 +979,8 @@ export function _paneDeathWatcherCount(): number {
  *
  * `applyAgentSkill` declines two shapes rather than writing through them ('foreign':
  * an unmarked skills/codeman the user authored; 'symlink': the skill dir or its
- * parent is a link). Both were silent: the user flips `agentSkillEnabled` on, nothing
- * appears in the case, and there is nowhere to look for why. The ordinary outcomes
+ * parent is a link). Both were silent: `agentSkillEnabled` is on (the default since
+ * 1.50.0), nothing appears in the case, and there is nowhere to look for why. The ordinary outcomes
  * ('installed'/'refreshed'/'unchanged') stay unlogged since they would print on every
  * single session create.
  *
@@ -1004,7 +1004,7 @@ async function injectAgentSkill(casePath: string, dir: AgentSkillDir): Promise<v
       );
     } else if (result === 'symlink') {
       console.warn(
-        `[agent-skill] not injected: ${skillDir} (or its parent) is a symlink, refusing to write through it. Replace it with a real directory to let Codeman install the skill.`
+        `[agent-skill] not injected: ${skillDir}, its parent or its reference folder is a symlink, refusing to write through it. Replace it with a real directory to let Codeman install the skill.`
       );
     }
   } catch (err: unknown) {

src/web/schemas.ts

@@ -1471,11 +1471,13 @@ export const SettingsUpdateSchema = z
     ultracodeFloatingWindows: z.boolean().optional(),
     imageWatcherEnabled: z.boolean().optional(),
     /**
-     * Inject the Codeman agent skill (`skills/codeman`) into `<case>/.claude/skills/`
-     * on Claude session create, so an agent inside the session can drive the API
-     * (see docs/agent-control-plan.md §2). SYNCED, default OFF: every skill's
-     * name+description costs context on every turn, so it is opt-in. Injection is
-     * add-only at create; a marker keeps user-authored copies untouched.
+     * Inject the Codeman agent skill (`skills/codeman`) into each new session's case, in
+     * the directory its CLI scans for skills (`capabilities.agentSkillDir`), so an agent
+     * inside the session can drive the API (see docs/agent-control-plan.md §2). SYNCED,
+     * default ON since 1.50.0: an absent key is on, only an explicit false is off, and a
+     * false stored by 1.49.0 or earlier is switched on once at the first 1.50.0 start
+     * (agent-skill-default-migration.ts). Injection is add-only at create; a marker keeps
+     * user-authored copies untouched.
      */
     agentSkillEnabled: z.boolean().optional(),
     /**

src/web/server.ts

@@ -197,6 +197,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';
@@ -2465,11 +2466,15 @@ export class WebServer extends EventEmitter {
     return resolveTerminalHistoryConfig(settings);
   }
 
-  // Whether the Codeman agent skill is injected into cases on Claude session create
-  // (synced `agentSkillEnabled` setting, default OFF; docs/agent-control-plan.md §2).
+  // Whether the Codeman agent skill is injected into a session's workspace on create
+  // (synced `agentSkillEnabled` setting; docs/agent-control-plan.md §2). Default ON
+  // since 1.50.0, like `workspaceHooksEnabled` below: an absent key means a user who
+  // has never touched the switch, and only an explicit false turns it off. The copy
+  // ignores itself in git (`.gitignore` of `*`, see installAgentSkillInto), so the
+  // default leaves a user's `git status` clean.
   private async getAgentSkillEnabled(): Promise<boolean> {
     const settings = await this.readSettings();
-    return settings.agentSkillEnabled === true;
+    return settings.agentSkillEnabled !== false;
   }
 
   // Whether pending agent-inbox mail is snapshotted to disk (synced
@@ -3446,6 +3451,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