fix(dry-run): stop dry-run and read-only commands persisting config migrations (#893)

docs/designs/data-directory-layout.md

@@ -397,8 +397,9 @@ stays in the checkout's `.teamai/`.
   self-heal bootstrap — which now writes the config into the PARTITION — then reads
   it back FROM the partition (`selfHealAndReadPartition`). A pre-P2 install whose
   config still sits in `<repo>/.teamai` is read via the legacy branch (double-read
-  compat) until migration relocates it. A `--dry-run` detection
-  (`roles set`, `tags subscribe`, `tags unsubscribe`) previews the bootstrap
+  compat) until migration relocates it. A `--dry-run` detection (a command that
+  forwards `--dry-run` to its loader, or a read-only one such as `status`, `list`,
+  `doctor` or `mcp list`, which loads this way unconditionally) previews the bootstrap
   instead (`previewSelfBootstrap`): it builds the config it would write, keeps it
   in memory, and prints `[dry-run] Would bootstrap ...` without locking, writing,
   injecting hooks or registering the member. It makes no provider auth call

src/__tests__/dry-run-load-path.test.ts

@@ -29,14 +29,33 @@ vi.mock('../utils/reports-branch.js', async (importOriginal) => ({
   updateReports: vi.fn(),
 }));
 
+import { codebaseCmd } from '../codebase-cmd.js';
 import { contribute } from '../contribute.js';
+import { generateDigest } from '../digest.js';
 import { loadLocalConfigForScope } from '../config.js';
+import { resolveDoctorContext } from '../doctor.js';
+import { excludeList } from '../exclude.js';
+import { hooksList } from '../hooks-cmd.js';
+import { importCmd } from '../import.js';
+import { mcpInject, mcpList } from '../mcp-cmd.js';
+import { maybeMigrate, queueKeptInCheckout } from '../migrate.js';
+import { modelsList, modelsSwitch } from '../models-cmd.js';
+import { pkgInstall } from '../pkg/commands.js';
+import { listMembers } from '../members.js';
+import { projectsAdd, projectsList, projectsMembers, projectsRemove, projectsUpdate } from '../projects-cmd.js';
 import { pull } from '../pull.js';
 import { push } from '../push.js';
 import { recall } from '../recall.js';
-import { rolesSet } from '../roles-cmd.js';
+import { recallStatus } from '../recall-toggle.js';
+import { remove } from '../remove.js';
+import { rolesAdd, rolesInit, rolesList, rolesRemove, rolesSet, rolesUpdate } from '../roles-cmd.js';
+import { skillList, skillShow } from '../skill-cmd.js';
+import { skillGet, skillPath } from '../skill-content.js';
+import { sourceAdd, sourceAddHttp, sourceBrowse, sourceList, sourceRemove } from '../source.js';
 import { list, status } from '../status.js';
-import { tagsSubscribe, tagsUnsubscribe } from '../tags.js';
+import { tagsAdd, tagsList, tagsRemove, tagsSubscribe, tagsUnsubscribe } from '../tags.js';
+import { uninstall } from '../uninstall.js';
+import { listWebhooks } from '../webhook.js';
 import { updateReports } from '../utils/reports-branch.js';
 import { log } from '../utils/logger.js';
 import { legacyProjectSlug } from '../utils/partition.js';
@@ -196,6 +215,7 @@ describe('--dry-run through the loaders the commands share (#850)', () => {
     vi.restoreAllMocks();
     vi.unstubAllEnvs();
     process.chdir(originalCwd);
+    process.exitCode = undefined;
     for (const dir of roots.splice(0)) fs.rmSync(dir, { recursive: true, force: true });
   });
 
@@ -254,19 +274,128 @@ describe('--dry-run through the loaders the commands share (#850)', () => {
     ['push --dry-run', () => push({ dryRun: true })],
     ['status', () => status({})],
     ['list', () => list(undefined, {})],
+    ['mcp inject --dry-run', () => mcpInject({ dryRun: true })],
+    ['mcp list', () => mcpList({})],
+    ['roles list', () => rolesList()],
+    ['projects list', () => projectsList({})],
+    ['tags list', () => tagsList()],
+    ['source list', () => sourceList()],
+    ['hooks list', () => hooksList({})],
+    ['exclude list', () => excludeList({})],
+    ['recall status', () => recallStatus({})],
+    ['doctor (its loader)', async () => { await resolveDoctorContext(); }],
+    ['skill get share', () => skillGet(['share'])],
+    ['skill path share', () => skillPath('share')],
+    ['models list', () => modelsList()],
+    ['codebase --status', () => codebaseCmd({ status: true })],
+    ['uninstall --dry-run', () => uninstall({ dryRun: true, force: true })],
+    ['packages install --dry-run', () => pkgInstall(undefined, { dryRun: true })],
+    ['source browse', () => sourceBrowse('x', {})],
+    ['codebase --lint', () => codebaseCmd({ lint: true })],
+    ['skill list', () => skillList({})],
+    ['skill show', () => skillShow('core', {})],
+    ['webhook list', async () => { await listWebhooks(); }],
+    ['digest', () => generateDigest()],
   ];
 
+  // The loader logs this line when it previews the migration, so it proves the
+  // row reached the load: an early return would leave the file unchanged too.
+  const PREVIEWED = expect.stringContaining('[dry-run] Would migrate legacy teamai config');
+
   it.each(LOAD_ONLY_COMMANDS)('%s migrates nothing it loads (#850)', async (_command, run) => {
     const { root, configPath } = legacyRoot();
+    const info = vi.spyOn(log, 'info').mockImplementation(() => {});
     const before = snapshotTree(root);
     const error = await run().then(() => null, (e: unknown) => e);
     expect(error).toBeNull();
+    expect(info).toHaveBeenCalledWith(PREVIEWED);
     expect(snapshotTree(root)).toEqual(before);
     expect(fs.readFileSync(configPath, 'utf-8')).not.toContain('primaryRole');
     // A dry run may parse the remote, but nothing else may reach a provider.
     expect(providerCalls).toEqual([]);
   });
 
+  // The rest of #893: commands that honour `--dry-run` in their own logic but
+  // loaded the scope bare. Each one reaches the loader before anything this
+  // fixture lacks (a team repo remote, a provider, a network) stops it, so the
+  // test asserts only what the loader decides: whether `config.yaml` is
+  // rewritten. The same call without the flag is the positive control: it DOES
+  // migrate, so an unchanged file under the flag is a result, not a command that
+  // failed before it loaded anything. Writes these commands make on their own
+  // dry-run path, past the loader, are out of scope here.
+  const PREVIEWS: Array<[string, (dryRun: boolean) => Promise<unknown>]> = [
+    ['mcp inject', (dryRun) => mcpInject({ dryRun })],
+    ['roles init', (dryRun) => rolesInit({ dryRun })],
+    ['roles add', (dryRun) => rolesAdd('ops', { dryRun, namespaces: 'ops' })],
+    ['roles remove', (dryRun) => rolesRemove('pm', { dryRun })],
+    ['roles update', (dryRun) => rolesUpdate('pm', { dryRun, description: 'x' })],
+    ['projects add', (dryRun) => projectsAdd('checkout', { dryRun, namespaces: 'checkout' })],
+    ['tags add', (dryRun) => tagsAdd('skills', 'x', ['a'], { dryRun })],
+    ['tags remove', (dryRun) => tagsRemove('skills', 'x', ['a'], { dryRun })],
+    ['source add', (dryRun) => sourceAdd('https://github.com/acme/other.git', { dryRun })],
+    ['source remove', (dryRun) => sourceRemove('x', { dryRun })],
+    ['source add-http', (dryRun) => sourceAddHttp('https://h.test', { dryRun, token: 't' })],
+    ['remove', (dryRun) => remove('skills', ['x'], { dryRun })],
+    ['packages install', (dryRun) => pkgInstall(undefined, { dryRun })],
+    ['import --from-iwiki', (dryRun) => importCmd({ fromIwiki: 'x', dryRun })],
+    ['import --from-mr', (dryRun) => importCmd({ fromMr: 'https://github.com/acme/app/pull/1', dryRun })],
+    ['codebase --reconcile', (dryRun) => codebaseCmd({ reconcile: true, dryRun })],
+    ['projects update', (dryRun) => projectsUpdate('checkout', { dryRun, description: 'x' })],
+    ['projects remove', (dryRun) => projectsRemove('checkout', { dryRun })],
+    // No candidates on this fixture: covers the scan's load, not the one after review.
+    ['import --from-claude', (dryRun) => importCmd({ fromClaude: true, all: true, dryRun })],
+    ['codebase --deep-enrich', (dryRun) => codebaseCmd({ deepEnrich: true, project: 'x', dryRun })],
+    ['models switch', (dryRun) => modelsSwitch('p', { dryRun })],
+  ];
+
+  describe.each(PREVIEWS)('%s', (_command, run) => {
+    beforeEach(() => {
+      // Past the loader these commands may exit or set an exit code; neither is under test.
+      vi.spyOn(process, 'exit').mockImplementation((code) => {
+        throw new Error(`process.exit(${String(code)})`);
+      });
+      vi.spyOn(console, 'log').mockImplementation(() => {});
+    });
+    afterEach(() => {
+      process.exitCode = undefined;
+      // `source add` reaches the provider's clone after the load; not what this asserts.
+      providerCalls.length = 0;
+    });
+
+    it('--dry-run leaves a config pending the role migration as it was (#893)', async () => {
+      const { configPath } = legacyRoot();
+      const info = vi.spyOn(log, 'info').mockImplementation(() => {});
+      const before = fs.readFileSync(configPath, 'utf-8');
+      await run(true).catch(() => {});
+      expect(info).toHaveBeenCalledWith(PREVIEWED);
+      expect(fs.readFileSync(configPath, 'utf-8')).toBe(before);
+    });
+
+    it('without the flag, the same call migrates it, so the load is on this path', async () => {
+      const { configPath } = legacyRoot();
+      await run(false).catch(() => {});
+      expect(fs.readFileSync(configPath, 'utf-8')).toContain('primaryRole: hai');
+    });
+  });
+
+  // Read-only commands that, past the load, fail on this fixture: there is no
+  // reports branch to read members from. Only the load is asserted.
+  const READ_ONLY_PAST_THE_LOAD: Array<[string, () => Promise<unknown>]> = [
+    ['members', () => listMembers({})],
+    ['projects members', () => projectsMembers('checkout', {})],
+  ];
+
+  it.each(READ_ONLY_PAST_THE_LOAD)('%s migrates nothing it loads (#893)', async (_command, run) => {
+    const { configPath } = legacyRoot();
+    const info = vi.spyOn(log, 'info').mockImplementation(() => {});
+    vi.spyOn(console, 'log').mockImplementation(() => {});
+    const before = fs.readFileSync(configPath, 'utf-8');
+    await run().catch(() => {});
+    providerCalls.length = 0;
+    expect(info).toHaveBeenCalledWith(PREVIEWED);
+    expect(fs.readFileSync(configPath, 'utf-8')).toBe(before);
+  });
+
   /** A git project whose partition still carries its pre-#546 name, i.e. project scope. */
   function projectRoot(): string {
     const root = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-dry-run-project-'));
@@ -288,6 +417,13 @@ describe('--dry-run through the loaders the commands share (#850)', () => {
     ['pull --dry-run', () => pull({ dryRun: true })],
     ['status', () => status({})],
     ['list', () => list(undefined, {})],
+    // What the CLI's preAction hook runs before a command under --dry-run:
+    // `maybeMigrate` before every write command (`pull`, `push`, ...), and
+    // `queueKeptInCheckout` before one that queues a learning. Calling
+    // `pull()` directly, as the rows above do, skips both.
+    ['the pre-command migration under --dry-run', async () => {
+      await queueKeptInCheckout(await maybeMigrate({ dryRun: true }), { dryRun: true });
+    }],
   ];
 
   it.each(PROJECT_SCOPE_COMMANDS)('%s adopts no legacy partition on a git project (#850)', async (_command, run) => {

src/__tests__/local-agent.test.ts

@@ -2021,3 +2021,61 @@ describe('local-agent: per-worktree claudemd isolation (issue #374 P1-2C)', () =
     await fse.remove(wtBReal).catch(() => {});
   });
 });
+
+describe('local-agent: loadLocalAgentConfig({ dryRun: true }) writes nothing (#893)', () => {
+  const configPath = () => path.join(tmpDir, '.teamai', 'local-agent', 'config.json');
+
+  it('drops a legacy group binding in memory and leaves config.json as it was', async () => {
+    await setupConfig({ '/ws/legacy': { groupId: 7, boundAt: 'x' } });
+    const before = await fse.readFile(configPath(), 'utf8');
+
+    const { loadLocalAgentConfig } = await import('../local-agent.js');
+    const config = await loadLocalAgentConfig({ dryRun: true });
+
+    expect(config!.workspaceBindings).toEqual({});
+    expect(await fse.readFile(configPath(), 'utf8')).toBe(before);
+  });
+
+  it('collapses path aliases in memory and leaves config.json as it was', async () => {
+    const realWs = path.join(tmpDir, 'ws-real');
+    const aliasWs = path.join(tmpDir, 'ws-alias');
+    await fse.ensureDir(realWs);
+    await fse.symlink(realWs, aliasWs, 'dir');
+    await setupConfig({
+      [realWs]: { projectId: 11, projectName: 'real', boundAt: 'x', ideType: 'codebuddy' },
+      [aliasWs]: { projectId: 11, projectName: 'alias', boundAt: 'x', ideType: 'codebuddy' },
+    });
+    const before = await fse.readFile(configPath(), 'utf8');
+
+    const { loadLocalAgentConfig } = await import('../local-agent.js');
+    const config = await loadLocalAgentConfig({ dryRun: true });
+
+    expect(Object.keys(config!.workspaceBindings)).toEqual([fse.realpathSync(realWs)]);
+    expect(await fse.readFile(configPath(), 'utf8')).toBe(before);
+  });
+
+  it('backfills from an http config.yaml in memory without creating config.json', async () => {
+    await fse.ensureDir(path.join(tmpDir, '.teamai'));
+    await fse.writeFile(
+      path.join(tmpDir, '.teamai', 'config.yaml'),
+      [
+        'username: tester',
+        'repo:',
+        '  kind: http',
+        '  url: https://team.example/api',
+        `  localPath: ${path.join(tmpDir, '.teamai', 'team-repo')}`,
+        '  remote: https://team.example/api',
+        '',
+      ].join('\n'),
+    );
+
+    const { loadLocalAgentConfig } = await import('../local-agent.js');
+    const dry = await loadLocalAgentConfig({ dryRun: true });
+    expect(dry?.endpoint).toBe('https://team.example/api');
+    expect(await fse.pathExists(configPath())).toBe(false);
+
+    // Positive control: the same load without the flag persists the backfill.
+    await loadLocalAgentConfig();
+    expect(await fse.pathExists(configPath())).toBe(true);
+  });
+});

src/__tests__/pull-model-namespaces.test.ts

@@ -332,6 +332,17 @@ describe('pull: team model profiles by namespace', () => {
     expect(await claude()).toEqual({ url: `${COMPANY}/checkout`, token: 'company-secret', model: 'checkout-model' });
   });
 
+  it('models list reads a key stored before namespaces existed bound, and leaves the file as it was (#893)', async () => {
+    const file = getTeamValuesPath(configFor([]));
+    await saveModelInputs(file, { 'team:gw': { API_KEY: { env: 'COMPANY_KEY' } } });
+    const before = await fse.readFile(file, 'utf8');
+
+    const output = await captureOutput(() => modelsList('team:gw'));
+
+    expect(output).toContain('  API key: environment COMPANY_KEY');
+    expect(await fse.readFile(file, 'utf8')).toBe(before);
+  });
+
   it('binds a beta key to the gateway its agent was switched to, so a root profile that moved since never gets it', async () => {
     const beta = { 'team:gw': { API_KEY: { env: 'COMPANY_KEY' } } };
     await saveModelInputs(getTeamValuesPath(configFor([])), beta);

src/codebase-cmd.ts

@@ -80,7 +80,9 @@ export async function codebaseCmd(opts: CodebaseCmdOptions): Promise<void> {
     } else {
         try {
             const { autoDetectInit } = await import('./config.js');
-            const { localConfig: lc } = await autoDetectInit();
+            // `--lint` alone only reads, so it never persists a migration (#893).
+            const readOnly = !opts.reconcile && !opts.deepEnrich;
+            const { localConfig: lc } = await autoDetectInit(undefined, { dryRun: opts.dryRun || readOnly });
             teamwikiDir = path.join(lc.repo.localPath, 'teamwiki');
         } catch {
             teamwikiDir = path.join(cwd, '.teamai', 'team-repo', 'teamwiki');
@@ -200,7 +202,8 @@ async function printCodebaseStatus(opts: CodebaseCmdOptions): Promise<void> {
     } else {
         try {
             const { autoDetectInit } = await import('./config.js');
-            const { localConfig: lc } = await autoDetectInit();
+            // Read-only: the load never persists a migration (#893).
+            const { localConfig: lc } = await autoDetectInit(undefined, { dryRun: true });
             teamwikiDir = path.join(lc.repo.localPath, 'teamwiki');
         } catch {
             teamwikiDir = path.join(cwd, '.teamai', 'team-repo', 'teamwiki');

src/config.ts

@@ -391,11 +391,14 @@ export async function resolveConfigForDir(dir?: string): Promise<LocalConfig | n
  * the project was set up. Readers that hold no resolved config (the local
  * agent, import, usage tracking) go through here.
  */
-export async function resolveMemberToolRoots(dir?: string): Promise<Record<string, string> | undefined> {
+export async function resolveMemberToolRoots(
+  dir?: string,
+  options: LoadOptions = {},
+): Promise<Record<string, string> | undefined> {
   // A hook can report a directory that no longer exists (a deleted worktree);
   // git probing there throws, so it means user scope — as resolveConfigForDir.
-  const project = dir === undefined || await pathExists(dir) ? await detectProjectConfig(dir) : null;
-  return project?.toolRoots ?? (await loadLocalConfig())?.toolRoots;
+  const project = dir === undefined || await pathExists(dir) ? await detectProjectConfig(dir, undefined, options) : null;
+  return project?.toolRoots ?? (await loadLocalConfig(options))?.toolRoots;
 }
 
 /**
@@ -611,9 +614,9 @@ function describeConfigError(e: unknown): string {
  * the wrong one. So a broken higher-priority file is reported even when a later
  * candidate loads.
  */
-export async function findUnreadableProjectConfig(cwd?: string): Promise<string | null> {
+export async function findUnreadableProjectConfig(cwd?: string, options: LoadOptions = {}): Promise<string | null> {
   let problem: string | null = null;
-  await detectProjectConfig(cwd, (configPath, error) => { problem ??= `${configPath}: ${error}`; });
+  await detectProjectConfig(cwd, (configPath, error) => { problem ??= `${configPath}: ${error}`; }, options);
   return problem;
 }
 

src/digest.ts

@@ -407,8 +407,9 @@ export function formatTrendLines(periods: { current: TrendPeriod; previous: Tren
  */
 export async function generateDigest(): Promise<void> {
   try {
-    const projectConfig = await detectProjectConfig();
-    const localConfig = projectConfig ?? (await requireInit()).localConfig;
+    // Read-only: the load never persists a migration (#893).
+    const projectConfig = await detectProjectConfig(undefined, undefined, { dryRun: true });
+    const localConfig = projectConfig ?? (await requireInit({ dryRun: true })).localConfig;
     const repoPath = localConfig.repo.localPath;
 
     // Knowledge (learnings, skill git-log) lives under localPath on the default

src/doctor.ts

@@ -305,8 +305,9 @@ async function hasInstalledCodexHooks(toolPaths: TeamaiConfig['toolPaths'], base
  * when TeamAI is not initialized here — the caller decides how to report that.
  */
 export async function resolveDoctorContext(): Promise<DoctorContext | null> {
-  const projectConfig = await detectProjectConfig();
-  const localConfig = projectConfig ?? (await loadLocalConfig());
+  // Read-only: the load never persists a migration (#893).
+  const projectConfig = await detectProjectConfig(undefined, undefined, { dryRun: true });
+  const localConfig = projectConfig ?? (await loadLocalConfig({ dryRun: true }));
   if (!localConfig) return null;
 
   const teamConfig = await loadTeamConfig(localConfig.repo.localPath);

src/exclude.ts

@@ -9,9 +9,9 @@ import {
 import { log } from './utils/logger.js';
 import type { GlobalOptions, LocalConfig } from './types.js';
 
-async function resolveExcludeScope(): Promise<LocalConfig> {
-  const projectConfig = await detectProjectConfig();
-  return projectConfig ?? (await requireInit()).localConfig;
+async function resolveExcludeScope(options: GlobalOptions = {}): Promise<LocalConfig> {
+  const projectConfig = await detectProjectConfig(undefined, undefined, options);
+  return projectConfig ?? (await requireInit(options)).localConfig;
 }
 
 async function saveExcludeScopeConfig(localConfig: LocalConfig): Promise<void> {
@@ -33,7 +33,8 @@ async function saveExcludeScopeConfig(localConfig: LocalConfig): Promise<void> {
 }
 
 export async function excludeList(_options: GlobalOptions): Promise<void> {
-  const config = await resolveExcludeScope();
+  // Read-only: the load never persists a migration (#893).
+  const config = await resolveExcludeScope({ dryRun: true });
   const excludedSkills = config.excludedSkills ?? [];
   if (excludedSkills.length === 0) {
     log.info('No excluded skills. (pull syncs all role skills)');