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(<HOME>/.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 <HOME>/.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.
This commit is contained in:
Smilewithoutfalling
2026-09-28 13:33:02 +08:00
parent 6e5298624c
commit cfd7c573dc
3 changed files with 113 additions and 11 deletions
+72
View File
@@ -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 `<getTeamaiHome>/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<void>, 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([]);
});
});
+6
View File
@@ -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);
+35 -11
View File
@@ -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
// `<getDataHome>`, 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 <getDataHome>/.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<string | null> {
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,