From 6e5298624cbaecfc4344d112e7a35f805ca73ae7 Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:25:16 +0800 Subject: [PATCH 1/5] fix(dry-run): thread { dryRun } through the loaders pull, push, status and list use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--dry-run` is documented as previewing without making changes, and #837 made that true for `tags subscribe`, `tags unsubscribe` and `roles set`. `pull`, `push` and `status` still wrote: each runs its scope-detection block before any dry-run guard, and that block called config loaders that were never given the flag. A preview could therefore persist the legacy role migration, and in a git repo adopt a pre-#546 partition or run the single-repo self-heal bootstrap. The loaders already take LoadOptions — #853 threaded them through `loadLocalConfigForScope` for contribute / session save / recall. These commands simply did not supply the flag. - pull.ts: both loaders take { dryRun: options.dryRun }. - push.ts: autoDetectInit takes it. - status.ts and list: { dryRun: true } unconditionally, because both are read-only and should never migrate, adopt a partition or bootstrap. Callers that pass nothing behave as before, the same compatibility promise #837 made. The one observable change is that the preview path logs, so `status`/`list` now surface a "[dry-run] Would ..." line where a migration or bootstrap is pending; the PR description asks for a decision on that label. Verification: seven new command-level cases, each failing on unmodified main with the identical test file (the project-scope three need a git project with a pre-#546 partition name, which the existing user-scope fixture never reaches); and the issue's own real-CLI reproduction, 6/6 — the control writes the reported fields, this change writes nothing. oxlint --deny-warnings and tsc --noEmit: rc=0 here and rc=0 on unmodified main. Closes #850 --- src/__tests__/dry-run-load-path.test.ts | 62 +++++++++++++++++++++++++ src/pull.ts | 8 +++- src/push.ts | 2 +- src/status.ts | 15 ++++-- 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index 284714e0b..d031f6c45 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,63 @@ 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([]); + }); }); diff --git a/src/pull.ts b/src/pull.ts index 2faf84926..67dcd75b3 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1882,7 +1882,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 +1915,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..1cee34f3c 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 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'; From cfd7c573dc9d5f7efdc0f413ac31487038b53842 Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:33:02 +0800 Subject: [PATCH 2/5] fix(pull,push): a --dry-run must leave a fresh self-mode clone alone Resolves both P1 findings on this PR. pull.ts:1891 - the previewed self-mode config reached lockScope(), whose acquireLock() calls ensureDir() on the lock's parent. On a fresh clone the partition does not exist yet, so the directory was created and stayed: releaseLock removes the lock file, not its parent. The guard sits inside lockScope(), the one choke point all three call sites share. push.ts:731 - the same previewed config ran the whole self-mode setup before pushCore reached its own dry-run guard at push.ts:1577: the sync-lock, migrateSelfModeGitignore(), and the disposable knowledge worktree. Both guards are deliberately narrow. A blanket early return before the git-mode branch would also skip resetToCleanMaster/pullRepo (push.ts:934), which a dry run performs on purpose so it can name the destination the real command would use. Only writes that outlive the command are gated. The preview still reads the uncommitted teamai.yaml that pushCore receives as initialPendingTeamConfig; that block is now pendingSelfTeamConfig(), the identical read, so the preview keeps describing the config edit it exists to describe. Fixture gap, also flagged: dry-run-load-path.test.ts already had a fresh-self-mode-clone fixture, but only tags/roles ran against it. pull and push ran at user scope, or on a project partition that already exists - never on the one shape where acquireLock has something new to create. Two cases added there; the unfixed tree fails them at fs.existsSync(/.teamai/projects) with "expected true to be false". Not fixed here, and named in the PR description: pull --dry-run on a fresh clone still creates an empty /.teamai/locks/, via listPendingForInstall in utils/pending-learnings.ts. That call is unchanged by this PR and the file is outside its scope; the test declares and counts the entry, so anything else appearing still fails. --- src/__tests__/dry-run-load-path.test.ts | 72 +++++++++++++++++++++++++ src/pull.ts | 6 +++ src/push.ts | 46 ++++++++++++---- 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index d031f6c45..8669f6a64 100644 --- a/src/__tests__/dry-run-load-path.test.ts +++ b/src/__tests__/dry-run-load-path.test.ts @@ -299,3 +299,75 @@ describe('--dry-run through the loaders the commands share (#850)', () => { 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')}/`; + const SELF_COMMANDS: Array<[string, () => Promise, string[]]> = [ + ['pull --dry-run', () => pull({ dryRun: true }), [PULL_LOCK_DIR]], + ['push --dry-run', () => push({ dryRun: true }), []], + ]; + + 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/pull.ts b/src/pull.ts index 67dcd75b3..3fca03b8a 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1862,6 +1862,12 @@ export async function pull( // migrateSelfA1 takes). http has no clone and no machine-data relocation, so it // needs no lock. if (config.repo.kind === 'http') return true; + // A dry run writes nothing for this scope, so it has no reason to hold the + // lock — and taking one is itself a write: `acquireLock` creates the lock's + // parent directory, which a fresh self-mode clone has no partition for yet, + // and `releaseLock` removes the lock file but leaves that directory behind + // (#866). Reporting the scope uncontended is exact — nothing was serialized. + if (options.dryRun) return true; const lock = path.join(getDataHome(config), SYNC_LOCK_FILENAME); if (await acquireLock(lock)) { heldLocks.set(config, lock); diff --git a/src/push.ts b/src/push.ts index 1cee34f3c..faecadd69 100644 --- a/src/push.ts +++ b/src/push.ts @@ -772,6 +772,22 @@ export async function push( // branch/commit/reset never touch the user's active tree. withKnowledgeWorktree // hands pushCore a config whose localPath is the worktree's .teamai. if (localConfig.repo.kind === 'self') { + // A dry run reports the plan and leaves the machine as it found it, so it + // skips this whole branch: the sync-lock (whose `acquireLock` creates + // ``, which a fresh self-mode clone has no partition for and + // `releaseLock` leaves behind), the `.teamai/.gitignore` self-heal, and the + // disposable knowledge worktree (#866). `pushCore` reaches its own dry-run + // guard without writing, and the active checkout is the truer preview in + // self mode — the worktree is cut from the same commits, and any uncommitted + // local edit is exactly what the real push would send. + if (options.dryRun) { + // The pending-config read is the one part of this branch that is + // read-only, and `pushCore` needs it: in self mode it is handed the + // uncommitted `teamai.yaml` rather than discovering it. Skipping it would + // make the preview under-report the very push it is describing. + await pushCore(localConfig, teamConfig, options, await pendingSelfTeamConfig(localConfig), result); + return; + } // Guard self machine-data writes against a concurrent P2 migration relocating // the same files. Contend on /.sync-lock — the exact path // migrateSelfA1 takes (for a pre-migration self install that is @@ -794,17 +810,7 @@ export async function push( 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); @@ -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, From 3a87a35150f2079e37253d102ba91406a8234c5f Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:33:06 +0800 Subject: [PATCH 3/5] test(pull): pin the loader call shape the widened signature produces Fixes the red Lint & Test on the previous head (all four matrix entries). pull-scope-isolation.test.ts asserted the exact argument list of loadLocalConfigForScope, which this change widens to carry LoadOptions: expect(loadLocalConfigForScope).toHaveBeenCalledWith('user'); received ['user', undefined, { dryRun: undefined }] The shape is not a free choice: recall.ts:464 and recall.ts:515 already pass the same third argument, landed with #853, so pull.ts matches the merged precedent. recall-scope-isolation.test.ts never asserts the argument list, which is why the same change left it green. The affected set is derived from the changed symbols rather than from the topic: every test file that mentions loadLocalConfigForScope, detectProjectConfig or autoDetectInit (76 files, 1221 tests). --- src/__tests__/pull-scope-isolation.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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' }); From c56480385827ad62d34746d040d03c21c829b8ae Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:18:13 +0800 Subject: [PATCH 4/5] fix(update): a --dry-run asks the lock for its state instead of taking it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `acquireLock` is not a read. It `ensureDir`s the lock's parent, which on a fresh self-mode clone is a `` partition that does not exist yet — and `releaseLock` removes the lock FILE, not that directory, so the directory outlives the command. Any preview that calls it therefore writes, which is the defect this PR is about (#866). The new `options.dryRun` returns what the preview actually owes its caller: the ANSWER the real run would get. `lockState` already separates `live` (a holder is running) from `stale` and `missing`, and the real run reclaims either of the latter and wins, so `acquireLock(path, { dryRun: true })` is exactly that verdict — no mkdir, no lock file, no reclaim sentinel. Nothing is recorded in `heldLockOwners`, which is what makes the preview safe alongside the existing `releaseLock` calls: it returns at its first line when it holds no owner token for the path, so a preview cannot delete a lock another process owns. No caller passes `dryRun` yet; this commit is the primitive only. --- src/update.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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, From a8325f6545ab813c8912f67176cb0b5fa9d1150e Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:18:18 +0800 Subject: [PATCH 5/5] fix(pull,push): acquire the preview's locks read-only, and stop hiding what it reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the guards added in cfd7c573. Those guards stopped the writes, and E2E (fork-safe) caught what they cost — 6 failures in push-sync-followups-823.test.ts, all of the same shape: expected '- Scanning local resources...\nNo new or modified resources to push' to contain '[rules] teamai-rule (modified)' A preview that reports no changes for a tree with a deliberately edited team rule is not a conservative preview; it is this PR's own defect with the sign flipped. pull.ts — `lockScope()` returned `true` outright for a dry run, asserting the scope was uncontended. That is a fabricated fact: its callers read `true` as "you hold the lock". It now acquires through the read-only primitive and records the lock for release only when it really took one, so a scope with a live holder is reported as contended and skipped, exactly as a real pull does. push.ts — the self-mode branch returned early for a dry run. Two of the three things it skipped are right to skip and one is not. - the sync-lock: read-only now, at both push.ts:784 (self) and push.ts:839 (git mode). - `migrateSelfModeGitignore()`: still skipped. It rewrites a tracked file in the user's ACTIVE tree, which outlives the preview, and it is idempotent, so the next real push performs it. - the knowledge worktree: MUST run, and that is the correction. `pushCore` adds the active tree's `.teamai/{skills,rules}` as scan sources and diffs them against `localConfig.repo.localPath` (push.ts:1095-1112) — the clean worktree checkout. Outside the worktree those are the same path in self mode, so the diff is empty by construction: skipping the worktree does not report an edit early, it hides the edit. `withKnowledgeWorktree` already removes it in a `finally`, so it stays disposable. Fixture — `push --dry-run` now performs its `git fetch` for real, which leaves `app/.git/FETCH_HEAD` behind. Declared and counted next to the existing `/locks/` entry, so any OTHER new entry still fails the case. --- src/__tests__/dry-run-load-path.test.ts | 8 ++++- src/pull.ts | 15 ++++----- src/push.ts | 44 ++++++++++++------------- 3 files changed, 36 insertions(+), 31 deletions(-) diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index 8669f6a64..0be031fe5 100644 --- a/src/__tests__/dry-run-load-path.test.ts +++ b/src/__tests__/dry-run-load-path.test.ts @@ -345,9 +345,15 @@ describe('--dry-run on a fresh self-mode clone (#866)', () => { // 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 }), []], + ['push --dry-run', () => push({ dryRun: true }), [FETCH_HEAD]], ]; it.each(SELF_COMMANDS)('%s creates no partition, worktree or lock file', async (_command, run, allowed) => { diff --git a/src/pull.ts b/src/pull.ts index 3fca03b8a..b3bfe0b47 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1862,15 +1862,14 @@ export async function pull( // migrateSelfA1 takes). http has no clone and no machine-data relocation, so it // needs no lock. if (config.repo.kind === 'http') return true; - // A dry run writes nothing for this scope, so it has no reason to hold the - // lock — and taking one is itself a write: `acquireLock` creates the lock's - // parent directory, which a fresh self-mode clone has no partition for yet, - // and `releaseLock` removes the lock file but leaves that directory behind - // (#866). Reporting the scope uncontended is exact — nothing was serialized. - if (options.dryRun) 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), diff --git a/src/push.ts b/src/push.ts index faecadd69..aa42ab42d 100644 --- a/src/push.ts +++ b/src/push.ts @@ -772,29 +772,16 @@ export async function push( // branch/commit/reset never touch the user's active tree. withKnowledgeWorktree // hands pushCore a config whose localPath is the worktree's .teamai. if (localConfig.repo.kind === 'self') { - // A dry run reports the plan and leaves the machine as it found it, so it - // skips this whole branch: the sync-lock (whose `acquireLock` creates - // ``, which a fresh self-mode clone has no partition for and - // `releaseLock` leaves behind), the `.teamai/.gitignore` self-heal, and the - // disposable knowledge worktree (#866). `pushCore` reaches its own dry-run - // guard without writing, and the active checkout is the truer preview in - // self mode — the worktree is cut from the same commits, and any uncommitted - // local edit is exactly what the real push would send. - if (options.dryRun) { - // The pending-config read is the one part of this branch that is - // read-only, and `pushCore` needs it: in self mode it is handed the - // uncommitted `teamai.yaml` rather than discovering it. Skipping it would - // make the preview under-report the very push it is describing. - await pushCore(localConfig, teamConfig, options, await pendingSelfTeamConfig(localConfig), result); - return; - } // Guard self machine-data writes against a concurrent P2 migration relocating // the same files. Contend on /.sync-lock — the exact path // 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; @@ -803,11 +790,23 @@ 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 pendingTeamConfig = await pendingSelfTeamConfig(localConfig); @@ -836,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;