diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 90b50d29a..8243e28ea 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -66,7 +66,10 @@ resets nothing, since a checkout recorded at an older revision already misses the fast path. `push` needs that entry too: before scanning, it syncs each rule and skill the member never edited, and "never edited" means equal to the version at a revision *this* checkout synced, not the shared `lastPullRev` -another checkout may have moved (#812). That sync brings the unedited copies up +another checkout may have moved (#812). A placed agent, which push does not +sync, is held when the team file has changed since any of those revisions, or +since it was added if one of them predates it (#823). That sync brings the +unedited copies up to the team repo, so when push has refreshed the team repo it adds the revision it synced to the entry's `pushBaseRevs`, newest first, even under `--dry-run`, since the sync has already written the files, and even when the diff --git a/docs/usage-guide.md b/docs/usage-guide.md index fd7c834de..2f8f26510 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -698,7 +698,7 @@ Choose namespace [1-3] (default: 1 = common): - `--role`/`--project` places new resources only. An edit of a shared-root rule or agent stays at the shared root, and push says so - A placed resource stays maintainable from the machine that published it. While its PR is open, the open-PR record routes a later edit of the author's own copy back to that PR; once the file is on the default branch, `state.json` records where push put it, so the edit goes back to the same file, and an agent published into a namespace this directory has not activated is still editable rather than skipped as having no active source - `teamai remove rules ` accepts the bare name the author's copy carries as well as the published `/`; it reports which one it resolved to, and removes both the namespaced team file and the author's copy at the rules root. If the team repo cannot be refreshed first, or this machine's placement records cannot be updated and saved, `remove` stops with exit 1 and removes nothing, because either can resolve the name to the wrong files -- A local agent is an edit of the team agent it was delivered from: one in an active namespace first, then one this machine placed, then the shared-root agent either of them replaces. Only when none exists does `--role`/`--project` decide, and the agent is new in that namespace; if that namespace already holds an agent of that name, the agent is skipped rather than written over it, as a rule would be. Two active agents of one name stay ambiguous and are skipped, flag or not. The same agent name may exist in several namespaces, so a copy in an inactive one you did not name never blocks publishing yours. A placed agent that changed on the team since this machine last synced it is held until you run `teamai pull`, because agents have no pre-push sync. In single-repo mode, a root copy under `.teamai/` that matches an older version of the file it was placed at is held too: nothing refreshes it, so it is an old copy rather than an edit +- A local agent is an edit of the team agent it was delivered from: one in an active namespace first, then one this machine placed, then the shared-root agent either of them replaces. Only when none exists does `--role`/`--project` decide, and the agent is new in that namespace; if that namespace already holds an agent of that name, the agent is skipped rather than written over it, as a rule would be. Two active agents of one name stay ambiguous and are skipped, flag or not. The same agent name may exist in several namespaces, so a copy in an inactive one you did not name never blocks publishing yours. A placed agent that changed on the team since this checkout last synced it is held until you run `teamai pull`, because agents have no pre-push sync. In single-repo mode, a root copy under `.teamai/` that matches an older version of the file it was placed at is held too: nothing refreshes it, so it is an old copy rather than an edit - A new resource is never placed on top of one that is already there. If the resolved namespace already holds that name, the push stops and names the file: pull and edit the existing copy, rename yours, or pick another namespace with `--role ` - An agent whose namespace is not active here stays editable through its placement record, and `pull` delivers it for the same reason, so your copy tracks the team file. It replaces a shared-root agent of the same name, as an active namespace's agent would. An active namespace holding that name wins: that agent is the one deployed here - A resource awaiting review in an open PR keeps that PR's destination — unless this push names a namespace other than the one recorded (the shared root counts as one), in which case the flag decides, the open PR is left untouched, and the collision is reported diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index b81aa131c..6cf4f68ac 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -654,7 +654,7 @@ Choose namespace [1-3] (default: 1 = common): - `--role`/`--project` 只放置新资源。对共享根目录 rule 或 agent 的修改仍留在共享根目录，push 会给出提示 - 已落点的资源在发布它的机器上仍可维护：PR 未合并期间，待评审 PR 记录会把作者对自己副本的修改带回该 PR；文件进入默认分支后，`state.json` 会记录 push 的落点，因此修改仍会写回同一个文件；即使 agent 落在本目录未激活的 namespace，也不会被当作“无活跃源”跳过 - `teamai remove rules ` 同时接受作者副本的简名和发布名 `/`：会打印实际解析到的名字，并同时删除带 namespace 的团队文件和作者在 rules 根目录的副本。若无法先刷新团队仓库，或本机的落点记录无法更新并保存，`remove` 会以退出码 1 停止且不删除任何内容，因为两者都可能把名字解析到错误的文件 -- 本地 agent 被视为其来源团队 agent 的编辑：优先是活跃 namespace 中的 agent，其次是本机放置的 agent，最后是被二者替换的共享根目录 agent。只有三者都不存在时，才由 `--role`/`--project` 决定，此时该 agent 在该 namespace 中是新的；若该 namespace 已有同名 agent，则跳过该 agent 而不是覆盖它，与 rule 的处理一致。两个活跃的同名 agent 无论是否指定参数都视为有歧义并跳过。同名 agent 允许存在于多个 namespace，因此你未指定的非活跃 namespace 中的同名副本不会阻止你发布。本机放置的 agent 若在本机上次同步后被团队修改，会暂缓推送，直到你运行 `teamai pull`，因为 agents 没有推送前同步。单仓库模式下，`.teamai/` 中的根目录副本若与其落点文件的某个旧版本相同，也会暂缓推送：没有任何操作会刷新它，因此它是旧副本而不是编辑 +- 本地 agent 被视为其来源团队 agent 的编辑：优先是活跃 namespace 中的 agent，其次是本机放置的 agent，最后是被二者替换的共享根目录 agent。只有三者都不存在时，才由 `--role`/`--project` 决定，此时该 agent 在该 namespace 中是新的；若该 namespace 已有同名 agent，则跳过该 agent 而不是覆盖它，与 rule 的处理一致。两个活跃的同名 agent 无论是否指定参数都视为有歧义并跳过。同名 agent 允许存在于多个 namespace，因此你未指定的非活跃 namespace 中的同名副本不会阻止你发布。本机放置的 agent 若在当前检出上次同步后被团队修改，会暂缓推送，直到你运行 `teamai pull`，因为 agents 没有推送前同步。单仓库模式下，`.teamai/` 中的根目录副本若与其落点文件的某个旧版本相同，也会暂缓推送：没有任何操作会刷新它，因此它是旧副本而不是编辑 - 新资源绝不会覆盖已存在的资源：若解析出的 namespace 下已有同名文件，命令会报错并指出该文件：请先 pull 并修改已有副本、重命名自己的资源，或用 `--role ` 换一个 namespace - 本目录未激活的 namespace 下的 agent 可通过落点记录继续编辑，`pull` 也会基于同一记录下发它，使本地副本与团队文件保持同步；它会像活跃 namespace 中的 agent 一样替换共享根目录的同名 agent。若已激活的 namespace 中已有同名 agent，则以它为准 - 待评审 PR 中的资源默认沿用该 PR 的落点；但若本次 push 明确指定的 namespace 与记录的落点不同（共享根目录也算一种落点），则以命令行为准，原 PR 保持不动，并提示该冲突 diff --git a/src/__tests__/agents.test.ts b/src/__tests__/agents.test.ts index 0ffdf5620..21705295e 100644 --- a/src/__tests__/agents.test.ts +++ b/src/__tests__/agents.test.ts @@ -337,7 +337,7 @@ projects: expect(await handler.scanLocalForPush(teamConfig, localConfig)).toEqual([]); }); - it('holds a recorded agent that changed on the team since this machine last synced it', async () => { + it('holds a recorded agent that changed on the team since this checkout last synced it', async () => { // Agents have no pre-push sync: a teammate's edit made before the author's // next pull would be overwritten by the stale local copy (#649 review). await nothingActive(); @@ -353,7 +353,7 @@ projects: const items = await handler.scanLocalForPush(teamConfig, localConfig); expect(items).toHaveLength(1); - expect(items[0]?.skipReason).toContain('changed on the team since this machine last synced it'); + expect(items[0]?.skipReason).toContain('changed on the team since this checkout last synced it'); expect(mockGetFileContentAtRev).toHaveBeenCalledWith(repoPath, 'abc1234', './agents/fe-agents/reviewer.yaml'); }); diff --git a/src/__tests__/e2e/push-sync-followups-823.test.ts b/src/__tests__/e2e/push-sync-followups-823.test.ts new file mode 100644 index 000000000..e8317de0a --- /dev/null +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -0,0 +1,308 @@ +/** + * E2E (#823, items 2 and 3): push must not offer a teammate's update back as + * the member's older copy. + * + * Item 2: in single-repo mode push runs against a knowledge worktree whose + * team root is `/.teamai`, a subdirectory of the git repo. The pre-push + * sync read each base version with a path relative to that subdirectory, which + * `git show :` resolves from the repo root, so it never found one: + * every rule a teammate updated read as a local edit. + * + * Item 3: an agent this machine placed with --role/--project was compared with + * the project's shared lastPullRev, which a pull in another checkout moves past + * a copy a stale worktree still holds unedited (the #812 revert, for agents). + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { execFileSync, spawn } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { projectSlug } from '../../utils/partition.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const ROOT = path.resolve(__dirname, '..', '..', '..'); +const CLI = path.join(ROOT, 'dist', 'index.js'); + +const GIT_ENV = { + GIT_AUTHOR_NAME: 'TeamAI CI', + GIT_AUTHOR_EMAIL: 'ci@teamai.test', + GIT_COMMITTER_NAME: 'TeamAI CI', + GIT_COMMITTER_EMAIL: 'ci@teamai.test', +}; + +interface RunResult { + code: number | null; + output: string; +} + +function runCLI(args: string[], cwd: string, home: string): Promise { + return new Promise((resolve) => { + const child = spawn('node', [CLI, ...args], { + cwd, + env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0' }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let output = ''; + child.stdout.on('data', (data: Buffer) => { output += data.toString(); }); + child.stderr.on('data', (data: Buffer) => { output += data.toString(); }); + child.on('close', (code) => resolve({ code, output })); + }); +} + +function git(args: string[], cwd: string): string { + return execFileSync('git', args, { cwd, encoding: 'utf8', env: { ...process.env, ...GIT_ENV } }).trim(); +} + +function requireCli(): void { + if (!fs.existsSync(CLI)) { + throw new Error(`CLI binary not found at ${CLI}. Run "npm run build" first.`); + } +} + +describe('pre-push sync in single-repo mode (#823 item 2)', () => { + let sandbox: string; + let home: string; + let projectRoot: string; + let teammate: string; + + const R1 = '# Team rule\n\nVersion one.\n'; + const R2 = '# Team rule\n\nVersion two, from a teammate.\n'; + const localRule = () => path.join(projectRoot, '.claude', 'rules', 'team-rule.md'); + + beforeEach(() => { + requireCli(); + sandbox = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-issue823-self-e2e-'))); + home = path.join(sandbox, 'home'); + projectRoot = path.join(sandbox, 'project'); + teammate = path.join(sandbox, 'teammate'); + const remote = path.join(sandbox, 'project-remote.git'); + + fs.mkdirSync(home, { recursive: true }); + fs.mkdirSync(path.join(projectRoot, '.teamai', 'rules'), { recursive: true }); + fs.mkdirSync(path.join(projectRoot, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(projectRoot, '.claude', 'settings.json'), '{}\n'); + fs.writeFileSync(path.join(projectRoot, '.gitignore'), '.claude/skills/\n.claude/rules/\n.claude/agents/\n'); + fs.writeFileSync(path.join(projectRoot, '.teamai', 'teamai.yaml'), [ + 'team: issue-823-self-e2e', + 'repo: https://github.com/acme/project.git', + 'provider: github', + 'mode: self', + '', + ].join('\n')); + fs.writeFileSync(path.join(projectRoot, '.teamai', 'rules', 'team-rule.md'), R1); + git(['init', '-q', '-b', 'main'], projectRoot); + git(['add', '-A'], projectRoot); + git(['commit', '-q', '-m', 'project'], projectRoot); + git(['clone', '-q', '--bare', projectRoot, remote], sandbox); + git(['remote', 'add', 'origin', remote], projectRoot); + git(['fetch', '-q', 'origin'], projectRoot); + git(['clone', '-q', remote, teammate], sandbox); + + const partition = path.join(home, '.teamai', 'projects', projectSlug(projectRoot)); + fs.mkdirSync(partition, { recursive: true }); + fs.writeFileSync(path.join(partition, 'anchor'), `${projectRoot}\n`); + fs.writeFileSync(path.join(partition, 'config.yaml'), [ + 'repo:', + ' kind: self', + ` localPath: ${path.join(projectRoot, '.teamai')}`, + " remote: ''", + ` businessRepoRoot: ${projectRoot}`, + 'username: ci-823-self', + 'updatePolicy: auto', + 'scope: project', + `projectRoot: ${projectRoot}`, + 'enabledAgents: [claude]', + '', + ].join('\n')); + }); + + afterEach(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + const run = async (args: string[]): Promise => { + const r = await runCLI(args, projectRoot, home); + expect(r.code, r.output).toBe(0); + return r.output; + }; + + it('syncs a teammate\'s update to .teamai/rules instead of listing the old copy as modified', async () => { + await run(['pull']); + expect(fs.readFileSync(localRule(), 'utf8')).toBe(R1); + + // A teammate lands R2 on the default branch. The member's branch takes it + // with git, but `teamai pull` has not run, so .claude/rules still has R1. + fs.writeFileSync(path.join(teammate, '.teamai', 'rules', 'team-rule.md'), R2); + git(['commit', '-q', '-am', 'rule R2'], teammate); + git(['push', '-q', 'origin', 'main'], teammate); + git(['fetch', '-q', 'origin'], projectRoot); + git(['merge', '-q', '--ff-only', 'origin/main'], projectRoot); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R2); + }); + + it('still lists a genuine local edit as modified', async () => { + await run(['pull']); + fs.writeFileSync(localRule(), `${R1}\nA local edit.\n`); + + const push = await run(['--dry-run', 'push']); + expect(push).toContain('[rules] team-rule (modified)'); + }); +}); + +describe('placed agent in a stale linked worktree (#823 item 3)', () => { + let sandbox: string; + let home: string; + let projectRoot: string; + let worktree: string; + let remote: string; + let teamRepo: string; + + const A1 = '---\nname: vr\ndescription: reviews code\n---\n\nYou review.\n'; + const agentIn = (root: string) => path.join(root, '.claude', 'agents', 'vr.md'); + + /** Commit on the remote's default branch through a throwaway clone, as a teammate or a merged PR does. */ + const onMain = (change: (clone: string) => void): void => { + const clone = fs.mkdtempSync(path.join(sandbox, 'mate-')); + git(['clone', '-q', remote, clone], sandbox); + change(clone); + git(['push', '-q', 'origin', 'main'], clone); + fs.rmSync(clone, { recursive: true, force: true }); + }; + + beforeEach(() => { + requireCli(); + sandbox = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-issue823-agent-e2e-'))); + home = path.join(sandbox, 'home'); + projectRoot = path.join(sandbox, 'project'); + worktree = path.join(sandbox, 'wt-b'); + remote = path.join(sandbox, 'team-remote.git'); + teamRepo = path.join(projectRoot, '.teamai', 'team-repo'); + const seed = path.join(sandbox, 'seed'); + + fs.mkdirSync(home, { recursive: true }); + for (const dir of ['skills', 'rules', 'agents']) { + fs.mkdirSync(path.join(seed, dir), { recursive: true }); + fs.writeFileSync(path.join(seed, dir, '.gitkeep'), ''); + } + fs.mkdirSync(path.join(seed, 'manifest'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'manifest', 'projects.yaml'), [ + 'version: 1', + 'projects:', + ' - id: front-app', + ' name: Front App', + ' description: Front end', + ' resources:', + ' knowledge: [fe-know]', + ' skills: [fe-skills]', + ' learnings: []', + ' agents: [fe-agents]', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), [ + 'team: issue-823-agent-e2e', + 'repo: https://example.com/team.git', + 'provider: tgit', + '', + ].join('\n')); + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + git(['clone', '-q', '--bare', seed, remote], sandbox); + + fs.mkdirSync(path.join(projectRoot, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(projectRoot, '.claude', 'settings.json'), '{}\n'); + fs.writeFileSync( + path.join(projectRoot, '.gitignore'), + '.teamai/\n.claude/skills/\n.claude/rules/\n.claude/agents/\n', + ); + git(['init', '-q', '-b', 'main'], projectRoot); + git(['add', '-A'], projectRoot); + git(['commit', '-q', '-m', 'project'], projectRoot); + + fs.mkdirSync(path.join(projectRoot, '.teamai'), { recursive: true }); + git(['clone', '-q', remote, teamRepo], sandbox); + fs.writeFileSync(path.join(projectRoot, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${teamRepo}`, + ` remote: ${remote}`, + 'username: ci-823', + 'updatePolicy: auto', + 'scope: project', + `projectRoot: ${projectRoot}`, + 'enabledAgents: [claude]', + '', + ].join('\n')); + fs.mkdirSync(path.dirname(agentIn(projectRoot)), { recursive: true }); + fs.writeFileSync(agentIn(projectRoot), A1); + }); + + afterEach(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + const run = async (args: string[], cwd: string): Promise => { + const r = await runCLI(args, cwd, home); + expect(r.code, r.output).toBe(0); + return r.output; + }; + + /** + * The author publishes the agent into front-app's namespace, never activated + * here, and the PR merges. A local bare remote has no PR API, so the push + * exits 1 after pushing the branch. + */ + const placeAndMerge = async (): Promise => { + const published = await runCLI(['push', '--project', 'front-app', '--all'], projectRoot, home); + expect(published.output).toContain('[agents] vr → agents/fe-agents/vr.yaml'); + const branch = git(['for-each-ref', '--format=%(refname:short)', 'refs/heads/teamai/'], remote) + .split('\n').filter(Boolean).at(-1) ?? ''; + expect(branch).not.toBe(''); + onMain((clone) => { + git(['merge', '--no-edit', '-q', `origin/${branch}`], clone); + git(['push', '-q', 'origin', '--delete', branch], clone); + }); + }; + const teammateRewrites = (): void => onMain((clone) => { + const file = path.join(clone, 'agents', 'fe-agents', 'vr.yaml'); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8').replace('You review.', 'A teammate rewrote this.')); + git(['commit', '-q', '-am', 'teammate: rewrite vr'], clone); + }); + const HELD = 'changed on the team since this checkout last synced it'; + + it('holds a placed agent a teammate changed that only another checkout has pulled', async () => { + await placeAndMerge(); + + // Both checkouts pull it; the worktree gets the agent from the record. + await run(['pull'], projectRoot); + git(['worktree', 'add', '-q', worktree, '-b', 'wt-b'], projectRoot); + await run(['pull'], worktree); + const pulled = fs.readFileSync(agentIn(worktree), 'utf8'); + expect(pulled).toContain('You review.'); + + // A teammate rewrites it, and only the main checkout pulls the rewrite. + teammateRewrites(); + await run(['pull'], projectRoot); + + const push = await run(['--dry-run', 'push'], worktree); + expect(push).toContain(HELD); + expect(fs.readFileSync(agentIn(worktree), 'utf8')).toBe(pulled); + }, 60_000); + + it('holds a placed agent a teammate changed after it landed, before this checkout pulled', async () => { + // The checkout's last pull predates the placement, so no pull revision has + // the file. Push records the team HEAD as a base before the scan, and the + // file there is the teammate's version, so only the version it was added + // with shows the author's copy is stale. + await run(['pull'], projectRoot); + await placeAndMerge(); + teammateRewrites(); + + const push = await run(['--dry-run', 'push'], projectRoot); + expect(push).toContain(HELD); + expect(fs.readFileSync(agentIn(projectRoot), 'utf8')).toBe(A1); + }, 60_000); +}); diff --git a/src/__tests__/pre-push-sync-skill-copy.test.ts b/src/__tests__/pre-push-sync-skill-copy.test.ts new file mode 100644 index 000000000..c612a86f3 --- /dev/null +++ b/src/__tests__/pre-push-sync-skill-copy.test.ts @@ -0,0 +1,98 @@ +/** + * #823 item 5: a skill copy the pre-push sync cannot finish must leave the + * local skill as it was. A half-written copy mixes files from two revisions, + * matches no base, and the next push lists the skill as modified. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import path from 'node:path'; +import os from 'node:os'; +import fse from 'fs-extra'; + +vi.mock('../utils/logger.js', () => ({ + log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), dim: vi.fn() }, +})); + +const mockGetFileContentAtRev = vi.fn<(repoPath: string, rev: string, filePath: string) => Promise>(); +vi.mock('../utils/git.js', () => ({ + getFileContentAtRev: (...args: [string, string, string]) => mockGetFileContentAtRev(...args), + getFileContentWhenAdded: vi.fn().mockResolvedValue(null), +})); + +// A copy that writes the first file of the source and then fails, as a full +// disk or a permission error partway through would. +vi.mock('../utils/fs.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + copyDir: async (src: string, dest: string): Promise => { + const [first] = (await actual.listFilesRecursive(src)).sort(); + if (first !== undefined) await actual.copyFile(path.join(src, first), path.join(dest, first)); + throw new Error('ENOSPC: no space left on device'); + }, + }; +}); + +import { syncTeamUpdatesToLocal } from '../utils/pre-push-sync.js'; +import type { TeamaiConfig, LocalConfig } from '../types.js'; + +describe('syncTeamUpdatesToLocal — a skill copy that fails partway', () => { + let tmpDir: string; + let homeDir: string; + let repoPath: string; + let teamConfig: TeamaiConfig; + let localConfig: LocalConfig; + + beforeEach(async () => { + tmpDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-pre-push-sync-copy-')); + homeDir = path.join(tmpDir, 'home'); + repoPath = path.join(tmpDir, 'team-repo'); + vi.stubEnv('HOME', homeDir); + + teamConfig = { + team: 'test', + description: '', + repo: 'https://git.woa.com/test/repo.git', + provider: 'tgit' as const, + reviewers: [], + sharing: { + skills: {}, + rules: { enforced: [] }, + docs: { localDir: '' }, + env: { injectShellProfile: true }, + }, + toolPaths: { claude: { skills: '.claude/skills', rules: '.claude/rules' } }, + }; + localConfig = { + repo: { localPath: repoPath, remote: 'https://git.woa.com/test/repo.git' }, + username: 'testuser', + updatePolicy: 'auto', + additionalRoles: [], + scope: 'user', + }; + mockGetFileContentAtRev.mockReset(); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + await fse.remove(tmpDir); + }); + + it('leaves the whole previous version in place, and nothing beside it', async () => { + const teamSkillDir = path.join(repoPath, 'skills', 'my-skill'); + await fse.outputFile(path.join(teamSkillDir, 'SKILL.md'), 'v2 skill'); + await fse.outputFile(path.join(teamSkillDir, 'notes.md'), 'v2 notes'); + const skillsDir = path.join(homeDir, '.claude', 'skills'); + const localSkillDir = path.join(skillsDir, 'my-skill'); + await fse.outputFile(path.join(localSkillDir, 'SKILL.md'), 'v1 skill'); + await fse.outputFile(path.join(localSkillDir, 'notes.md'), 'v1 notes'); + mockGetFileContentAtRev.mockImplementation(async (_repo, _rev, file) => ( + Buffer.from(file.endsWith('SKILL.md') ? 'v1 skill' : 'v1 notes') + )); + + await expect(syncTeamUpdatesToLocal(teamConfig, localConfig, 'rev1')).rejects.toThrow('ENOSPC'); + + expect(await fse.readFile(path.join(localSkillDir, 'SKILL.md'), 'utf-8')).toBe('v1 skill'); + expect(await fse.readFile(path.join(localSkillDir, 'notes.md'), 'utf-8')).toBe('v1 notes'); + expect(await fse.readdir(skillsDir)).toEqual(['my-skill']); + }); +}); diff --git a/src/__tests__/pre-push-sync.test.ts b/src/__tests__/pre-push-sync.test.ts index a2783300f..541239f2a 100644 --- a/src/__tests__/pre-push-sync.test.ts +++ b/src/__tests__/pre-push-sync.test.ts @@ -130,7 +130,7 @@ describe('syncTeamUpdatesToLocal — rules', () => { // the next push sends it over the teammate's update. const content = await fse.readFile(path.join(homeDir, '.claude/rules', 'my-rule.md'), 'utf-8'); expect(content).toBe('teammate v2'); - expect(mockGetFileContentAtRev).toHaveBeenCalledWith(repoPath, 'abc1234', 'rules/fe-know/my-rule.md'); + expect(mockGetFileContentAtRev).toHaveBeenCalledWith(repoPath, 'abc1234', './rules/fe-know/my-rule.md'); }); it('syncs a placement that landed after the last pull from the version it was added with', async () => { @@ -165,7 +165,7 @@ describe('syncTeamUpdatesToLocal — rules', () => { expect(await fse.readFile(path.join(homeDir, '.claude/rules', 'my-rule.md'), 'utf-8')) .toBe('teammate v2'); - expect(mockGetFileContentAtRev).toHaveBeenCalledWith(repoPath, 'abc1234', 'rules/fe-know/my-rule.md'); + expect(mockGetFileContentAtRev).toHaveBeenCalledWith(repoPath, 'abc1234', './rules/fe-know/my-rule.md'); }); it('leaves a root rule alone when no record maps it to a namespaced team rule', async () => { @@ -300,7 +300,7 @@ describe('syncTeamUpdatesToLocal — rules', () => { expect(mockGetFileContentAtRev).toHaveBeenCalledWith( repoPath, 'abc1234', - 'rules/python/tencent_standard.md', + './rules/python/tencent_standard.md', ); }); @@ -622,6 +622,41 @@ describe('syncTeamUpdatesToLocal — skills', () => { expect(await fse.readFile(path.join(localSkillDir, 'notes.md'), 'utf-8')).toBe('v1 notes'); }); + it('keeps files only the member has when it syncs a skill (#823)', async () => { + const teamSkillDir = path.join(repoPath, 'skills', 'my-skill'); + await fse.outputFile(path.join(teamSkillDir, 'SKILL.md'), 'v2 skill'); + const skillsDir = path.join(homeDir, '.claude/skills'); + const localSkillDir = path.join(skillsDir, 'my-skill'); + await fse.outputFile(path.join(localSkillDir, 'SKILL.md'), 'v1 skill'); + await fse.outputFile(path.join(localSkillDir, 'scratch', 'mine.md'), 'my notes'); + mockGetFileContentAtRev.mockResolvedValue(Buffer.from('v1 skill')); + + await syncTeamUpdatesToLocal(teamConfig, localConfig, 'abc1234'); + + expect(await fse.readFile(path.join(localSkillDir, 'SKILL.md'), 'utf-8')).toBe('v2 skill'); + expect(await fse.readFile(path.join(localSkillDir, 'scratch', 'mine.md'), 'utf-8')).toBe('my notes'); + expect(await fse.readdir(skillsDir)).toEqual(['my-skill']); + }); + + it.skipIf(process.getuid?.() === 0)('leaves a read-only skill as it was, with nothing beside it (#823)', async () => { + const teamSkillDir = path.join(repoPath, 'skills', 'my-skill'); + await fse.outputFile(path.join(teamSkillDir, 'SKILL.md'), 'v2 skill'); + const skillsDir = path.join(homeDir, '.claude/skills'); + const localSkillDir = path.join(skillsDir, 'my-skill'); + await fse.outputFile(path.join(localSkillDir, 'SKILL.md'), 'v1 skill'); + await fse.chmod(path.join(localSkillDir, 'SKILL.md'), 0o444); + await fse.chmod(localSkillDir, 0o555); + mockGetFileContentAtRev.mockResolvedValue(Buffer.from('v1 skill')); + + try { + await expect(syncTeamUpdatesToLocal(teamConfig, localConfig, 'abc1234')).rejects.toThrow(); + expect(await fse.readFile(path.join(localSkillDir, 'SKILL.md'), 'utf-8')).toBe('v1 skill'); + expect(await fse.readdir(skillsDir)).toEqual(['my-skill']); + } finally { + await fse.chmod(localSkillDir, 0o755); + } + }); + it('should skip skills that only exist locally (not in team repo)', async () => { const localSkillDir = path.join(homeDir, '.claude/skills', 'local-only'); await fse.ensureDir(localSkillDir); diff --git a/src/resources/agents.ts b/src/resources/agents.ts index a75fcc529..e5b7e7310 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -201,7 +201,19 @@ export class AgentsHandler extends ResourceHandler { // namespace this directory need not have activated. Without the record it // would read as "no active source" and the author could never edit the // agent they just created (#649 review). - const { placedAgents, lastPullRev, pendingPushes } = await loadStateForScope(localConfig); + const { placedAgents, lastPullRev, lastPullByWorkspace, pendingPushes } = await loadStateForScope(localConfig); + // The revisions THIS checkout's copies can be at, with the same fallback + // as the pre-push sync: state.json is shared by every worktree, and a pull + // in another checkout moves lastPullRev past a copy this one still holds + // unedited (#812, #823). + const checkoutBases = async (): Promise => { + const { checkoutKey, checkoutBaseRevs } = await import('../pull.js'); + const key = localConfig.scope === 'project' && localConfig.projectRoot + ? await checkoutKey(localConfig.projectRoot) + : undefined; + const bases = checkoutBaseRevs(key ? lastPullByWorkspace?.[key] : undefined); + return bases.length > 0 ? bases : lastPullRev ? [lastPullRev] : []; + }; // Agents this machine placed in a namespace and has awaiting review: the // open PR is their destination, not "no active source". const pendingPlacedAgents = new Set((pendingPushes ?? []).flatMap((entry) => entry.items) @@ -447,7 +459,7 @@ export class AgentsHandler extends ResourceHandler { // author's own merged edit included — has nothing to overwrite with. if (located && candidates === recorded) { const recordedPath = `agents/${located.namespace}/${stem}${located.ext}`; - if (await recordedAgentMovedOn(localConfig.repo.localPath, recordedPath, lastPullRev)) { + if (await recordedAgentMovedOn(localConfig.repo.localPath, recordedPath, await checkoutBases())) { items.push({ name: stem, type: 'agents', sourcePath: teamAgentsDir, relativePath: recordedPath, status: 'modified', namespace: located.namespace, skipReason: staleRecordedAgentReason(stem, recordedPath) }); @@ -949,23 +961,33 @@ type TeamAgentFile = { path: string; ext: '.yaml' | '.md'; namespace?: string }; /** * Whether a team agent reached through this machine's placement record has - * changed since this machine's copy of it was current: the version at the last - * pull, or — for a placement that landed after it — the version it was added - * with. Agents have no pre-push sync, so a teammate's edit made before the - * author's next pull would otherwise be overwritten by the stale local copy - * (#649 review). A guard, not a merge: `pull` delivers the recorded agent and - * moves the baseline, after which the edit can be pushed. + * changed since this checkout's copy of it was current: the version at any of + * the checkout's bases, or — for a placement that landed after one of them — + * the version it was added with. Agents have no pre-push sync, so a teammate's + * edit made before the author's next pull would otherwise be overwritten by the + * stale local copy (#649 review). The copy stays at the revision pull delivered + * while push bases move on (push records the team HEAD before the scan), so a + * difference from any of those versions counts. A guard, not a merge: `pull` + * delivers the recorded agent and resets the bases, after which the edit can be + * pushed. */ -async function recordedAgentMovedOn(repoPath: string, relPath: string, lastPullRev: string | null): Promise { +async function recordedAgentMovedOn(repoPath: string, relPath: string, bases: readonly string[]): Promise { const current = await readFileSafe(path.join(repoPath, relPath)); if (current === null) return false; - const baseline = (lastPullRev ? await getFileContentAtRev(repoPath, lastPullRev, `./${relPath}`) : null) - ?? await getFileContentWhenAdded(repoPath, relPath); - return baseline !== null && baseline.toString('utf-8') !== current; + const baselines: Buffer[] = []; + for (const rev of bases) { + const content = await getFileContentAtRev(repoPath, rev, `./${relPath}`); + if (content !== null) baselines.push(content); + } + if (baselines.length < bases.length || bases.length === 0) { + const added = await getFileContentWhenAdded(repoPath, relPath); + if (added !== null) baselines.push(added); + } + return baselines.some((baseline) => baseline.toString('utf-8') !== current); } function staleRecordedAgentReason(stem: string, relPath: string): string { - return `Agent "${stem}" (${relPath}) changed on the team since this machine last synced it, ` + return `Agent "${stem}" (${relPath}) changed on the team since this checkout last synced it, ` + 'so pushing your copy would overwrite that change. `teamai pull` replaces your copy with the team version, ' + 'so first copy your edit aside, then pull, reapply it, and push again.'; } diff --git a/src/utils/pre-push-sync.ts b/src/utils/pre-push-sync.ts index 4c70c3c37..77e633ea8 100644 --- a/src/utils/pre-push-sync.ts +++ b/src/utils/pre-push-sync.ts @@ -1,4 +1,6 @@ +import crypto from 'node:crypto'; import path from 'node:path'; +import fse from 'fs-extra'; import type { TeamaiConfig, LocalConfig } from '../types.js'; import { resolveBaseDir, scopedToolPaths } from '../types.js'; import { @@ -128,7 +130,7 @@ async function syncRulesToLocal( const baseVersions = async (): Promise => { const atBases: Buffer[] = []; for (const rev of bases) { - const content = await getFileContentAtRev(repoPath, rev, teamRelPath); + const content = await getFileContentAtRev(repoPath, rev, `./${teamRelPath}`); if (content !== null) atBases.push(content); } if (atBases.length > 0 || !viaRecord) return atBases; @@ -234,7 +236,7 @@ async function syncSkillsToLocal( for (const base of bases) { if (await skillAtBase(repoPath, localSkillDir, teamSkillDir, teamFiles, base)) { // All differing files match that base → team updated, user didn't → sync - await copyDir(teamSkillDir, localSkillDir); + await replaceSkillDir(teamSkillDir, localSkillDir); log.debug(`Pre-push sync: updated ${tool} skill ${skillName} to match team repo`); break; } @@ -243,6 +245,60 @@ async function syncSkillsToLocal( } } +/** + * Bring a local skill to the team version, or leave it as it was. A copy that + * failed partway mixed files from two revisions, matched no base, and the next + * push listed the skill as modified (#823). The update is built in a hidden + * sibling, starting from the local copy so files only the member has survive + * as they would a copy over it, and renamed into place. + */ +async function replaceSkillDir(teamSkillDir: string, localSkillDir: string): Promise { + const parent = path.dirname(localSkillDir); + const tag = `${path.basename(localSkillDir)}.${process.pid}.${crypto.randomBytes(6).toString('hex')}`; + const staged = path.join(parent, `.${tag}.teamai-sync`); + const previous = path.join(parent, `.${tag}.teamai-prev`); + try { + await fse.copy(localSkillDir, staged); + await copyDir(teamSkillDir, staged); + await fse.rename(localSkillDir, previous); + } catch (error) { + await removeLeftover(staged); + throw error; + } + try { + await fse.rename(staged, localSkillDir); + } catch (error) { + const restored = await fse.rename(previous, localSkillDir).then(() => true, () => false); + await removeLeftover(staged); + if (!restored) { + throw new Error(`${error instanceof Error ? error.message : String(error)}; the previous version of the skill ` + + `could not be put back and is at ${previous}. Move it back to ${localSkillDir}.`); + } + throw error; + } + await removeLeftover(previous); +} + +/** + * Remove a directory replaceSkillDir left beside the skill. It carries the + * local skill's modes, so a read-only one is made writable first; a symlink is + * removed without touching its target. Tools may read a leftover as a skill, + * so one that cannot be removed is reported. + */ +async function removeLeftover(dir: string): Promise { + try { + const stat = await fse.lstat(dir).catch((error: unknown) => { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') return null; + throw error; + }); + if (stat === null) return; + if (stat.isDirectory()) await fse.chmod(dir, 0o700); + await fse.remove(dir); + } catch (error) { + log.warn(`Could not remove ${dir} (${error instanceof Error ? error.message : String(error)}). Delete it by hand.`); + } +} + /** * Whether every file of a local skill that differs from the team repo is the * version at `base` (and some file differs), so the difference is a teammate's @@ -266,7 +322,7 @@ async function skillAtBase( if (!await pathExists(localFile)) { // File is new in team repo — check if it existed at base - const oldContent = await getFileContentAtRev(repoPath, base, relFromRepo); + const oldContent = await getFileContentAtRev(repoPath, base, `./${relFromRepo}`); // Existed at base but is missing locally — ambiguous, skip sync if (oldContent !== null) return false; // New file added by teammate since base → safe to sync @@ -278,7 +334,7 @@ async function skillAtBase( anyDiffers = true; - const oldContent = await getFileContentAtRev(repoPath, base, relFromRepo); + const oldContent = await getFileContentAtRev(repoPath, base, `./${relFromRepo}`); // Can't determine old version — ambiguous, don't sync if (oldContent === null) return false; // Local differs from old version → user edited this file