diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index 284714e0b..0be031fe5 100644 --- a/src/__tests__/dry-run-load-path.test.ts +++ b/src/__tests__/dry-run-load-path.test.ts @@ -31,8 +31,11 @@ vi.mock('../utils/reports-branch.js', async (importOriginal) => ({ import { contribute } from '../contribute.js'; import { loadLocalConfigForScope } from '../config.js'; +import { pull } from '../pull.js'; +import { push } from '../push.js'; import { recall } from '../recall.js'; import { rolesSet } from '../roles-cmd.js'; +import { list, status } from '../status.js'; import { tagsSubscribe, tagsUnsubscribe } from '../tags.js'; import { updateReports } from '../utils/reports-branch.js'; import { log } from '../utils/logger.js'; @@ -236,4 +239,141 @@ describe('--dry-run through the loaders the commands share (#850)', () => { expect(loaded?.primaryRole).toBe('hai'); expect(fs.readFileSync(configPath, 'utf-8')).toContain('primaryRole: hai'); }); + + // The command-level half of #850. Each of these reaches the legacy role + // migration through a loader it used to call bare, so the fixture's + // `config.yaml` gained `primaryRole` even though nothing had asked to write. + // `pull`/`push` carry `--dry-run`; `status`/`list` are read-only and pass it + // unconditionally (see the note at their `autoDetectInit` call site). + // + // The positive control is the test directly above: the SAME fixture does gain + // `primaryRole` when the flag is absent, so an unchanged tree here is a real + // result and not the harness failing to look. + const LOAD_ONLY_COMMANDS: Array<[string, () => Promise]> = [ + ['pull --dry-run', () => pull({ dryRun: true })], + ['push --dry-run', () => push({ dryRun: true })], + ['status', () => status({})], + ['list', () => list(undefined, {})], + ]; + + it.each(LOAD_ONLY_COMMANDS)('%s migrates nothing it loads (#850)', async (_command, run) => { + const { root, configPath } = legacyRoot(); + const before = snapshotTree(root); + const error = await run().then(() => null, (e: unknown) => e); + expect(error).toBeNull(); + 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([]); + }); + + /** 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-')); + roots.push(root); + const home = path.join(root, 'home'); + fs.mkdirSync(path.join(home, '.teamai'), { recursive: true }); + // An installed agent, so a bootstrap that did run would seed and wire it. + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + vi.stubEnv('HOME', home); + process.chdir(setupLegacyNamedPartition(root)); + return root; + } + + // The project-scope half. These reach the same bare calls through + // `detectProjectConfig`, whose dry-run branch is what stops + // `adoptLegacyPartition` (a real `fs.rename`) and the self-heal bootstrap. + // The issue report located these by code path only; they are run here. + const PROJECT_SCOPE_COMMANDS: Array<[string, () => Promise]> = [ + ['pull --dry-run', () => pull({ dryRun: true })], + ['status', () => status({})], + ['list', () => list(undefined, {})], + ]; + + it.each(PROJECT_SCOPE_COMMANDS)('%s adopts no legacy partition on a git project (#850)', async (_command, run) => { + const root = projectRoot(); + const before = snapshotTree(root); + const error = await run().then(() => null, (e: unknown) => e); + expect(error).toBeNull(); + expect(snapshotTree(root)).toEqual(before); + expect(providerCalls).toEqual([]); + }); +}); + +// The self-mode half of #866. `pull` and `push` are the two commands that take a +// partition sync-lock, and every fixture above runs them at user scope, or on a +// project partition that already exists. A fresh self-mode clone is the one +// shape where the partition does NOT exist yet — so `acquireLock` creating the +// lock's parent directory creates a directory that nothing removes afterwards. +// `push` carries a second instance of the same mistake: its self-mode setup +// (lock, `.teamai/.gitignore` self-heal, knowledge worktree) all runs before +// `pushCore` reaches its own dry-run guard. +describe('--dry-run on a fresh self-mode clone (#866)', () => { + const originalCwd = process.cwd(); + let root: string; + + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-dry-run-self-')); + const home = path.join(root, 'home'); + fs.mkdirSync(path.join(home, '.teamai'), { recursive: true }); + // An installed agent, so a bootstrap that did run would seed and wire it. + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + vi.stubEnv('HOME', home); + process.chdir(setupSelfModeClone(root)); + vi.spyOn(log, 'info').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(updateReports).mockClear(); + providerCalls.length = 0; + vi.unstubAllEnvs(); + process.chdir(originalCwd); + fs.rmSync(root, { recursive: true, force: true }); + }); + + // Each command declares exactly which new entries it may leave behind. Both + // start empty: the point of #866 is that a preview writes nothing. + // + // `pull` is allowed one, and it is not this change's. `pull` counts the + // contribution queue so it can report how many learnings it would publish, and + // `publishQueuedLearnings` lists it through `listPendingForInstall` + // (`utils/pending-learnings.ts`), which holds the queue lock — `acquireLock` + // creates the lock's parent, so an install with no `/locks/` + // gets one and keeps it. That call is unchanged here, and identical on the + // base commit; it only became reachable on a fresh self-mode clone once + // detection stopped aborting first (#850). Declared and counted rather than + // filtered out, so any OTHER new entry still fails this test. + const PULL_LOCK_DIR = `${path.join('home', '.teamai', 'locks')}/`; + // `git fetch` — which the preview performs on purpose, so that its plan is + // based on the same `origin/` the real push would branch from — + // leaves its own one-line record behind. It names no ref, changes no working + // tree, and git overwrites it on the next fetch. Same treatment as the entry + // above: declared and counted, so any other new entry still fails this test. + const FETCH_HEAD = path.join('app', '.git', 'FETCH_HEAD'); + const SELF_COMMANDS: Array<[string, () => Promise, string[]]> = [ + ['pull --dry-run', () => pull({ dryRun: true }), [PULL_LOCK_DIR]], + ['push --dry-run', () => push({ dryRun: true }), [FETCH_HEAD]], + ]; + + it.each(SELF_COMMANDS)('%s creates no partition, worktree or lock file', async (_command, run, allowed) => { + // The reported symptom, named so a failure here reads as #866 rather than + // as an anonymous tree diff: taking the sync-lock used to create this + // directory, and releasing it removed the lock file but not the directory. + const partitionRoot = path.join(root, 'home', '.teamai', 'projects'); + expect(fs.existsSync(partitionRoot)).toBe(false); + const before = snapshotTree(root); + const error = await run().then(() => null, (e: unknown) => e); + expect(error).toBeNull(); + expect(fs.existsSync(partitionRoot)).toBe(false); + + const after = snapshotTree(root); + const appeared = Object.keys(after).filter((key) => !(key in before)); + const vanished = Object.keys(before).filter((key) => !(key in after)); + expect({ appeared, vanished }).toEqual({ appeared: allowed, vanished: [] }); + // Nothing that already existed may be rewritten, whatever it is. + for (const key of Object.keys(before)) expect(after[key]).toBe(before[key]); + // A dry run may parse the remote, but nothing else may reach a provider. + expect(providerCalls).toEqual([]); + }); }); diff --git a/src/__tests__/pull-scope-isolation.test.ts b/src/__tests__/pull-scope-isolation.test.ts index c5748a86c..afd6d2134 100644 --- a/src/__tests__/pull-scope-isolation.test.ts +++ b/src/__tests__/pull-scope-isolation.test.ts @@ -290,7 +290,11 @@ describe('pull scope isolation (issue #73)', () => { await pull({ silent: true }); - expect(loadLocalConfigForScope).toHaveBeenCalledWith('user'); + // The third argument is the LoadOptions the loader now receives so it can + // preview instead of migrate (#850). It rides along on every call, so the + // assertion has to mention it; `dryRun` is undefined here because this test + // drives `pull` without `--dry-run`. + expect(loadLocalConfigForScope).toHaveBeenCalledWith('user', undefined, { dryRun: undefined }); expect(loadStateForScope).toHaveBeenCalledWith(expect.objectContaining({ scope: 'user' })); expect(loadStateForScope).toHaveBeenCalledWith(expect.objectContaining({ scope: 'project', projectRoot })); expect(log.info).toHaveBeenCalledWith( @@ -346,7 +350,7 @@ describe('pull scope isolation (issue #73)', () => { await pull({ silent: true }); - expect(loadLocalConfigForScope).toHaveBeenCalledWith('user'); + expect(loadLocalConfigForScope).toHaveBeenCalledWith('user', undefined, { dryRun: undefined }); expect(log.info).not.toHaveBeenCalledWith(SKIP_MSG); expect(pullSources).toHaveBeenCalledTimes(1); expect(vi.mocked(pullSources).mock.calls[0][0]).toMatchObject({ scope: 'user' }); diff --git a/src/pull.ts b/src/pull.ts index 2faf84926..b3bfe0b47 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1863,8 +1863,13 @@ export async function pull( // needs no lock. if (config.repo.kind === 'http') return true; const lock = path.join(getDataHome(config), SYNC_LOCK_FILENAME); - if (await acquireLock(lock)) { - heldLocks.set(config, lock); + // `acquireLock` under a dry run reads the lock's state instead of creating + // it — taking one is itself a write (#866) — so the preview answers with + // what the real run would have found: a live holder reports this scope as + // contended and skips it, exactly as a real pull does. Nothing is recorded + // for release, because nothing was taken. + if (await acquireLock(lock, { dryRun: options.dryRun })) { + if (!options.dryRun) heldLocks.set(config, lock); return true; } // User-visible: this scope is skipped wholesale (no fetch/deploy/reconcile), @@ -1882,7 +1887,11 @@ export async function pull( let projectConfig: LocalConfig | null = null; const unreadable: string[] = []; try { - projectConfig = await detectProjectConfig(undefined, (configPath, error) => { unreadable.push(`${configPath}: ${error}`); }); + projectConfig = await detectProjectConfig( + undefined, + (configPath, error) => { unreadable.push(`${configPath}: ${error}`); }, + { dryRun: options.dryRun }, + ); } catch (e) { log.warn(`Project-scope detection error: ${(e as Error).message}`); } @@ -1911,7 +1920,7 @@ export async function pull( log.info('project scope detected, skipped user scope'); } else { try { - const loadedUserConfig = await loadLocalConfigForScope('user'); + const loadedUserConfig = await loadLocalConfigForScope('user', undefined, { dryRun: options.dryRun }); if (loadedUserConfig) { if (inheritUserScope) { inheritedUserConfig = loadedUserConfig; diff --git a/src/push.ts b/src/push.ts index 0ad93fd72..aa42ab42d 100644 --- a/src/push.ts +++ b/src/push.ts @@ -728,7 +728,7 @@ export async function push( result?: { completed: boolean }, ): Promise { // Auto-detect scope: project scope if cwd has project config, else user scope - const { localConfig, teamConfig } = await autoDetectInit(); + const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: options.dryRun }); assertNotReadOnly(localConfig, 'teamai push'); // --project is a destination override expressed as a logical project. Each @@ -777,8 +777,11 @@ export async function push( // migrateSelfA1 takes (for a pre-migration self install that is // /.teamai/.sync-lock). Like git-mode push, error on contention rather // than silently skipping (that would drop the user's changes). + // Under a dry run `acquireLock` reads the lock's state instead of creating + // it (#866), so the preview contends on exactly what a real push would, and + // leaves no partition directory behind. const selfSyncLock = path.join(getDataHome(localConfig), SYNC_LOCK_FILENAME); - if (!(await acquireLock(selfSyncLock))) { + if (!(await acquireLock(selfSyncLock, { dryRun: options.dryRun }))) { log.error('Another teamai pull/push/migration is in progress for this project. Re-run once it finishes.'); process.exitCode = 1; return; @@ -787,24 +790,26 @@ export async function push( // Self-heal an older .teamai/.gitignore that still ignores `env` (pre-beta.5). // Run against the ACTIVE tree (original localConfig, projectRoot intact) BEFORE // swapping into the worktree, so the fixed .gitignore lets env changes surface. - try { - const { migrateSelfModeGitignore } = await import('./init.js'); - await migrateSelfModeGitignore(localConfig); - } catch { /* best-effort */ } + // A dry run skips it: it rewrites a tracked file in the user's active tree, + // which outlives the preview (#866), and it is idempotent, so the next real + // push performs it. + if (!options.dryRun) { + try { + const { migrateSelfModeGitignore } = await import('./init.js'); + await migrateSelfModeGitignore(localConfig); + } catch { /* best-effort */ } + } + // A dry run runs this too, and must: the worktree is not a side effect of + // pushing, it is the only source of the CLEAN BASELINE self-mode scanners + // compare the active tree against. `env.ts` diffs `projectRoot/.teamai` + // against `repo.localPath`, and outside the worktree those are the same + // path in self mode — so skipping it makes the preview silently + // under-report every edit, rather than merely report it early (#866). It + // is disposable: `withKnowledgeWorktree` removes it in a `finally`. const { withKnowledgeWorktree, EmptyRepoError } = await import('./utils/reports-branch.js'); try { - const activeConfigPath = path.join(localConfig.repo.localPath, 'teamai.yaml'); - const activeConfig = await readFileSafe(activeConfigPath); - const businessRoot = localConfig.repo.businessRepoRoot ?? localConfig.projectRoot; - let pendingTeamConfig: string | null = null; - if (activeConfig !== null && businessRoot) { - const relativeConfigPath = path.relative(businessRoot, activeConfigPath).split(path.sep).join('/'); - const committed = await getFileContentAtRev(businessRoot, 'HEAD', relativeConfigPath); - if (committed === null || committed.toString() !== activeConfig) { - pendingTeamConfig = activeConfig; - } - } + const pendingTeamConfig = await pendingSelfTeamConfig(localConfig); await withKnowledgeWorktree(localConfig, async (wtConfig) => { if (pendingTeamConfig !== null) { await writeFile(path.join(wtConfig.repo.localPath, 'teamai.yaml'), pendingTeamConfig); @@ -830,7 +835,8 @@ export async function push( // A concurrent pull/push would corrupt it. Unlike pull, push must NOT silently // skip (that would drop the user's changes), so on contention we error out. const syncLock = path.join(getDataHome(localConfig), SYNC_LOCK_FILENAME); - const locked = await acquireLock(syncLock); + // Read-only acquisition under a dry run, exactly as the self-mode branch above. + const locked = await acquireLock(syncLock, { dryRun: options.dryRun }); if (!locked) { log.error('Another teamai pull/push is in progress for this project. Re-run once it finishes.'); process.exitCode = 1; @@ -843,6 +849,24 @@ export async function push( } } +/** + * The `teamai.yaml` a self-mode push has to carry, or null when HEAD already + * holds it. Read from the ACTIVE tree — the business repo is `businessRepoRoot`, + * and the worktree the push swaps into is a detached checkout of the same + * commits, so this is the one input `pushCore` cannot rediscover from the + * worktree alone. Read-only, which is why the `--dry-run` path calls it too + * (#866); writing it into the worktree stays with the real path. + */ +async function pendingSelfTeamConfig(localConfig: LocalConfig): Promise { + const activeConfigPath = path.join(localConfig.repo.localPath, 'teamai.yaml'); + const activeConfig = await readFileSafe(activeConfigPath); + const businessRoot = localConfig.repo.businessRepoRoot ?? localConfig.projectRoot; + if (activeConfig === null || !businessRoot) return null; + const relativeConfigPath = path.relative(businessRoot, activeConfigPath).split(path.sep).join('/'); + const committed = await getFileContentAtRev(businessRoot, 'HEAD', relativeConfigPath); + return committed === null || committed.toString() !== activeConfig ? activeConfig : null; +} + async function pushCore( localConfig: LocalConfig, teamConfig: TeamaiConfig, diff --git a/src/status.ts b/src/status.ts index 8717f4612..75a94058b 100644 --- a/src/status.ts +++ b/src/status.ts @@ -42,8 +42,14 @@ export async function status(options: GlobalOptions): Promise { await statusAll(); return; } - // Auto-detect scope - const { localConfig, teamConfig } = await autoDetectInit(); + // Auto-detect scope. + // This is a read-only command, so `dryRun` is passed unconditionally rather + // than forwarded from `options.dryRun`: the load must never migrate a legacy + // role config, adopt a pre-#546 partition, or run the self-heal bootstrap + // (#850). The preview path returns what a write would have produced, so the + // report below still tells the truth, and the migration then persists on the + // next command that writes. + const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: true }); const scopeLabel = localConfig.scope; // Scope info @@ -248,8 +254,9 @@ async function statusAll(): Promise { } export async function list(type: string | undefined, options: ListOptions): Promise { - // Auto-detect scope - const { localConfig, teamConfig } = await autoDetectInit(); + // Auto-detect scope — read-only, so `dryRun: true` unconditionally, as in + // `status` above (#850). + const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: true }); const repoPath = localConfig.repo.localPath; const source = options.source ?? 'all'; diff --git a/src/update.ts b/src/update.ts index 809ffbb47..384eaafa5 100644 --- a/src/update.ts +++ b/src/update.ts @@ -379,8 +379,20 @@ async function acquireReclaimSentinel(sentinel: string, owner: string): Promise< * residual window exists only if the reclaiming process itself dies mid-reclaim: * stealing its dead-pid sentinel is not yet race-free, see #760.) */ -export async function acquireLock(lockPath?: string): Promise { +export async function acquireLock( + lockPath?: string, + options: { dryRun?: boolean } = {}, +): Promise { const resolved = lockPath ?? expandHome(getUpdateLockPath()); + // A preview does not take the lock, because taking one is itself a write: + // `ensureDir` below creates the lock's parent directory, which a fresh + // self-mode clone has no partition for yet and which `releaseLock` has no + // reason to remove — the directory would outlive the command (#866). What the + // preview still owes its caller is the ANSWER the real command would get, so + // it reads the lock's state instead of creating it: a live holder means the + // real run would have reported contention, anything else means it would have + // won. No owner is recorded, so `releaseLock` has nothing to undo. + if (options.dryRun) return (await lockState(resolved)) !== 'live'; const owner = randomUUID(); const payload = JSON.stringify({ pid: process.pid,