mirror of
https://github.com/Tencent/teamai-cli.git
synced 2026-10-02 03:14:40 +08:00
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:
@@ -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([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user