diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 87ff235cb..e7f1eb523 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -93,7 +93,22 @@ through. Every full sync keeps only the entries of checkouts `git worktree list` still reports, so a deleted or re-created worktree's entry goes with the next full sync in any checkout. A state.json written before this field has no entry, so each checkout does one -full sync after the upgrade. +full sync after the upgrade. The user scope's pull records its one checkout, +HOME, the same way, for push's bases alone: its fast path still reads the +shared fields, and an install with no entry yet (upgraded, and not fully +synced since) compares with `lastPullRev` and `lastInheritedPullRev`, which +only HOME's pulls move and either of which may have run last, so push does not +stop there; its first push creates the entry from `lastPullRev`, with +`lastInheritedPullRev` as a push base, and adds the revision its sync reached. +A project pull that inherits the user scope (`inheritUserScope`) moves HOME's +skills, rules and agents too, so it adds its revision to that entry's push +bases, creating the entry the same way if there is none, and leaves the entry's +`rev` alone. So does any pull whose docs mirror or submodule update fails: it +leaves its revision marker for the retry, but the skills, rules and agents it +delivered are at the new revision. A full pull that holds skills or agents on a +namespace collision writes its revision as `rev` but keeps the entry's earlier +bases as push bases, since the held copies stay at them. Like a full pull, an +inherited pull already synced at the team's revision writes nothing (#823). ### Why the main worktree, not `git-common-dir` (verified) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 94477309d..71a05249d 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -494,7 +494,7 @@ knowledge on main is left exactly in place). - `.teamai/agents/` — subagent definitions (`.yaml`, or legacy `.md`) - `.teamai/env/env.yaml` — shared env vars - `teamai push` scans all of these plus your AI tool dirs, and only surfaces genuine additions or edits (already-committed content is skipped). If you rename an agent's extension (e.g. `helper.md` → `helper.yaml`), delete the old file — `teamai push` won't remove it for you, and two files with the same stem would collide on pull. + `teamai push` scans all of these plus your AI tool dirs, and only surfaces genuine additions or edits (already-committed content is skipped). A rule or skill under `.teamai/` that matches an older version of the team's file, as it does when your branch is behind the default branch, is not an edit either: push skips it with a warning rather than revert a teammate's update. If you rename an agent's extension (e.g. `helper.md` → `helper.yaml`), delete the old file — `teamai push` won't remove it for you, and two files with the same stem would collide on pull. 4. **docs / hooks / mcp** are contributed by editing their file directly — they don't go through `teamai push`; a normal `git commit` + push ships them: - `.teamai/docs/` — team docs - `.teamai/hooks/hooks.yaml` — team hooks diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 846d761a6..6605c6aca 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -435,7 +435,7 @@ main 的团队知识 —— `git status` 保持干净。旧版单仓装升级后 - `.teamai/agents/` —— subagent 定义(`.yaml`,或旧版 `.md`) - `.teamai/env/env.yaml` —— 共享环境变量 - `teamai push` 会同时扫描这些目录和你的 AI 工具目录,只呈现真正的新增或修改(已提交的内容会被跳过)。如果你改了某个 agent 的扩展名(如 `helper.md` → `helper.yaml`),请手动删掉旧文件 —— `teamai push` 不会替你删除,同 stem 的两个文件会在 pull 时冲突。 + `teamai push` 会同时扫描这些目录和你的 AI 工具目录,只呈现真正的新增或修改(已提交的内容会被跳过)。`.teamai/` 下的规则或 skill 如果与团队文件的某个旧版本一致(你的分支落后于默认分支时就会这样),也不算修改:push 会给出警告并跳过它,而不会覆盖队友的更新。如果你改了某个 agent 的扩展名(如 `helper.md` → `helper.yaml`),请手动删掉旧文件 —— `teamai push` 不会替你删除,同 stem 的两个文件会在 pull 时冲突。 4. **docs / hooks / mcp** 通过直接编辑对应文件来贡献 —— 它们不走 `teamai push`,用普通的 `git commit` + push 即可分发: - `.teamai/docs/` —— 团队文档 - `.teamai/hooks/hooks.yaml` —— 团队 hooks diff --git a/skill-data/core/references/contribute-member.md b/skill-data/core/references/contribute-member.md index e4ce6c9b2..82dd3ff70 100644 --- a/skill-data/core/references/contribute-member.md +++ b/skill-data/core/references/contribute-member.md @@ -104,6 +104,13 @@ The doc lands in the team's `learnings/` and appears for teammates on their next to reset a team-repo clone with user changes, so commit or stash unrelated modified, staged, untracked, or conflicted files before retrying. + In single-repo mode, a skill or rule under `.teamai/` that matches an older + version of the team's file, as it does when the branch is behind the default + branch, is skipped with a warning that it "is an older version of" that file: + pushing it would revert a teammate's update. To publish an edit of it, bring + the current version in first (`git fetch origin && git merge origin/`, + or copy the team's current file over it), redo the edit on top, and push again. + ## After contributing - Confirm it landed: `teamai list skills` (or `teamai status`). diff --git a/src/__tests__/e2e/push-sync-followups-823.test.ts b/src/__tests__/e2e/push-sync-followups-823.test.ts index e8317de0a..0e806201e 100644 --- a/src/__tests__/e2e/push-sync-followups-823.test.ts +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -11,6 +11,20 @@ * 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). + * + * Item 4: a user-scope install kept no push base, so after a push synced HOME's + * copy to a teammate's update, the next push compared it with the revision the + * last pull delivered and offered it back over the teammate's next update. + * + * Item 19: a project pull that inherits the user scope moves HOME's copies + * without moving the user scope's push bases, with the same result. So did a + * pull whose docs mirror failed, and an upgraded install whose last inherited + * pull an older CLI ran. + * + * Item 10: in single-repo mode the active tree's .teamai/rules and + * .teamai/skills are push sources themselves. On a branch behind the default + * branch they hold an older team version nobody edited, and push listed it as + * modified, ready to revert the teammate's update. */ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { execFileSync, spawn } from 'node:child_process'; @@ -18,6 +32,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { StateSchema } from '../../types.js'; import { projectSlug } from '../../utils/partition.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -60,7 +75,7 @@ function requireCli(): void { } } -describe('pre-push sync in single-repo mode (#823 item 2)', () => { +describe('pre-push sync in single-repo mode (#823 items 2 and 10)', () => { let sandbox: string; let home: string; let projectRoot: string; @@ -68,7 +83,10 @@ describe('pre-push sync in single-repo mode (#823 item 2)', () => { const R1 = '# Team rule\n\nVersion one.\n'; const R2 = '# Team rule\n\nVersion two, from a teammate.\n'; + const S1 = '---\nname: team-skill\ndescription: Team skill\n---\n\nVersion one.\n'; const localRule = () => path.join(projectRoot, '.claude', 'rules', 'team-rule.md'); + const activeRule = () => path.join(projectRoot, '.teamai', 'rules', 'team-rule.md'); + const activeSkill = () => path.join(projectRoot, '.teamai', 'skills', 'team-skill'); beforeEach(() => { requireCli(); @@ -91,6 +109,8 @@ describe('pre-push sync in single-repo mode (#823 item 2)', () => { '', ].join('\n')); fs.writeFileSync(path.join(projectRoot, '.teamai', 'rules', 'team-rule.md'), R1); + fs.mkdirSync(path.join(projectRoot, '.teamai', 'skills', 'team-skill'), { recursive: true }); + fs.writeFileSync(path.join(projectRoot, '.teamai', 'skills', 'team-skill', 'SKILL.md'), S1); git(['init', '-q', '-b', 'main'], projectRoot); git(['add', '-A'], projectRoot); git(['commit', '-q', '-m', 'project'], projectRoot); @@ -151,6 +171,283 @@ describe('pre-push sync in single-repo mode (#823 item 2)', () => { const push = await run(['--dry-run', 'push']); expect(push).toContain('[rules] team-rule (modified)'); }); + + /** A teammate lands `content` at `file` on the default branch; the member fetches it but stays on their branch. */ + const teammateLandsOnMain = (file: string, content: string): void => { + fs.writeFileSync(path.join(teammate, file), content); + git(['add', '-A'], teammate); + git(['commit', '-q', '-m', `teammate: ${file}`], teammate); + git(['push', '-q', 'origin', 'main'], teammate); + git(['fetch', '-q', 'origin'], projectRoot); + }; + const HELD = 'which has changed on the team since'; + + it('holds a stale .teamai/rules copy on a branch behind a teammate\'s update (#823 item 10)', async () => { + await run(['pull']); + teammateLandsOnMain('.teamai/rules/team-rule.md', R2); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(push).toContain(`[rules] Skipped team-rule: .teamai/rules/team-rule.md is an older version of rules/team-rule.md, ${HELD}`); + expect(fs.readFileSync(activeRule(), 'utf8')).toBe(R1); + }); + + it('still lists a genuine edit of a stale .teamai/rules copy as modified (#823 item 10)', async () => { + await run(['pull']); + teammateLandsOnMain('.teamai/rules/team-rule.md', R2); + fs.writeFileSync(activeRule(), `${R1}\nA local edit.\n`); + + const push = await run(['--dry-run', 'push']); + expect(push).toContain('[rules] team-rule (modified)'); + expect(push).not.toContain(HELD); + }); + + it('holds a stale .teamai/skills copy on a branch behind a teammate\'s update (#823 item 10)', async () => { + await run(['pull']); + teammateLandsOnMain('.teamai/skills/team-skill/SKILL.md', S1.replace('Version one.', 'Version two, from a teammate.')); + // A file only the member has does not make the copy an edit. + fs.writeFileSync(path.join(activeSkill(), 'notes.md'), 'Member notes.\n'); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-skill (modified)'); + expect(push).toContain(`[skills] Skipped team-skill: .teamai/skills/team-skill is an older version of skills/team-skill, ${HELD}`); + expect(fs.readFileSync(path.join(activeSkill(), 'SKILL.md'), 'utf8')).toBe(S1); + }); + + it('holds a stale .teamai/skills copy that lacks a file a teammate added (#823 item 10)', async () => { + await run(['pull']); + teammateLandsOnMain('.teamai/skills/team-skill/reference.md', 'Added by a teammate.\n'); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-skill (modified)'); + expect(push).toContain(`[skills] Skipped team-skill: .teamai/skills/team-skill is an older version of skills/team-skill, ${HELD}`); + }); + + it('still lists a stale .teamai/skills copy whose branch file the member deleted as modified (#823 item 10)', async () => { + await run(['pull']); + // The member's branch has the teammate's file, then falls behind again. + teammateLandsOnMain('.teamai/skills/team-skill/reference.md', 'On the branch.\n'); + git(['merge', '-q', '--no-edit', 'origin/main'], projectRoot); + teammateLandsOnMain('.teamai/skills/team-skill/SKILL.md', S1.replace('Version one.', 'Version two, from a teammate.')); + fs.rmSync(path.join(activeSkill(), 'reference.md')); + + const push = await run(['--dry-run', 'push']); + expect(push).toContain('[skills] team-skill (modified)'); + expect(push).not.toContain(HELD); + }); + + it('still lists a genuine edit of a stale .teamai/skills copy as modified (#823 item 10)', async () => { + await run(['pull']); + teammateLandsOnMain('.teamai/skills/team-skill/SKILL.md', S1.replace('Version one.', 'Version two, from a teammate.')); + fs.writeFileSync(path.join(activeSkill(), 'SKILL.md'), `${S1}\nA local edit.\n`); + + const push = await run(['--dry-run', 'push']); + expect(push).toContain('[skills] team-skill (modified)'); + expect(push).not.toContain(HELD); + }); +}); + +describe('push base in user scope (#823 item 4)', () => { + let sandbox: string; + let home: string; + let work: string; + let teammate: string; + + const R1 = '# Team rule\n\nVersion one.\n'; + const localRule = () => path.join(home, '.claude', 'rules', 'team-rule.md'); + const userState = () => StateSchema.parse(JSON.parse(fs.readFileSync(path.join(home, '.teamai', 'state.json'), 'utf8'))); + + beforeEach(() => { + requireCli(); + sandbox = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-issue823-user-e2e-'))); + home = path.join(sandbox, 'home'); + work = path.join(sandbox, 'work'); + teammate = path.join(sandbox, 'teammate'); + const seed = path.join(sandbox, 'seed'); + const remote = path.join(sandbox, 'team-remote.git'); + const teamRepo = path.join(home, '.teamai', 'team-repo'); + + fs.mkdirSync(work, { recursive: true }); + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'rules'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), [ + 'team: issue-823-user-e2e', + 'repo: https://example.com/team.git', + 'provider: tgit', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'rules', 'team-rule.md'), R1); + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + git(['clone', '-q', '--bare', seed, remote], sandbox); + git(['clone', '-q', remote, teammate], sandbox); + git(['clone', '-q', remote, teamRepo], sandbox); + fs.writeFileSync(path.join(home, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${teamRepo}`, + ` remote: ${remote}`, + 'username: ci-823-user', + 'updatePolicy: auto', + 'scope: user', + '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, work, home); + expect(r.code, r.output).toBe(0); + return r.output; + }; + const teammatePublishes = (rule: string): void => { + fs.writeFileSync(path.join(teammate, 'rules', 'team-rule.md'), rule); + git(['commit', '-q', '-am', 'rule update'], teammate); + git(['push', '-q', 'origin', 'main'], teammate); + }; + + it('compares the next push with the revision the last push synced HOME\'s copy to', async () => { + await run(['pull']); + expect(fs.readFileSync(localRule(), 'utf8')).toBe(R1); + expect(await run(['pull'])).toContain('Already synced'); + + // A push syncs the unedited copy to a teammate's R2; a teammate then + // publishes R3 before any pull. + teammatePublishes('# Team rule\n\nVersion two, from a teammate.\n'); + await run(['--dry-run', 'push']); + expect(fs.readFileSync(localRule(), 'utf8')).toContain('Version two'); + const R3 = '# Team rule\n\nVersion three, from a teammate.\n'; + teammatePublishes(R3); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R3); + // HOME is the user scope's one checkout: one record, holding both bases. + const records = Object.values(userState().lastPullByWorkspace ?? {}); + expect(records).toHaveLength(1); + expect(records[0]?.pushBaseRevs).toHaveLength(2); + }); + + const dropUserRecord = (): void => { + const { lastPullByWorkspace: _dropped, ...rest } = userState(); + fs.writeFileSync(path.join(home, '.teamai', 'state.json'), `${JSON.stringify(rest, null, 2)}\n`); + }; + + it.each([ + ['a user-scope pull recorded HOME', 'never'], + ['no pull has recorded HOME (upgraded install)', 'before'], + ['a CLI that kept no record ran that pull (upgraded install)', 'after'], + ] as const)('compares the next push with the revision an inheriting project\'s pull moved HOME\'s copy to, when %s (#823 item 19)', async (_case, dropRecord) => { + await run(['pull']); + if (dropRecord === 'before') dropUserRecord(); + const project = path.join(sandbox, 'project'); + const projectTeamRepo = path.join(project, '.teamai', 'team-repo'); + git(['clone', '-q', path.join(sandbox, 'team-remote.git'), projectTeamRepo], sandbox); + fs.writeFileSync(path.join(project, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${projectTeamRepo}`, + ` remote: ${path.join(sandbox, 'team-remote.git')}`, + 'username: ci-823-project', + 'scope: project', + `projectRoot: ${project}`, + 'inheritUserScope: true', + 'enabledAgents: [claude]', + '', + ].join('\n')); + + // The project's pull inherits the user scope and moves HOME's copy to R2; + // a teammate then publishes R3 before any user-scope pull. + teammatePublishes('# Team rule\n\nVersion two, from a teammate.\n'); + const inherited = await runCLI(['pull'], project, home); + expect(inherited.code, inherited.output).toBe(0); + expect(fs.readFileSync(localRule(), 'utf8'), inherited.output).toContain('Version two'); + // lastPullRev still names R1 and lastInheritedPullRev R2, the revision + // HOME's copy is at. + if (dropRecord === 'after') dropUserRecord(); + const R3 = '# Team rule\n\nVersion three, from a teammate.\n'; + teammatePublishes(R3); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R3); + }); + + it('compares the next push with the revision a pull moved HOME\'s copy to when its docs mirror failed', async () => { + fs.mkdirSync(path.join(teammate, 'docs'), { recursive: true }); + fs.writeFileSync(path.join(teammate, 'docs', 'guide.md'), '# Guide\n'); + git(['add', '-A'], teammate); + git(['commit', '-q', '-m', 'docs'], teammate); + git(['push', '-q', 'origin', 'main'], teammate); + await run(['pull']); + expect(fs.readFileSync(localRule(), 'utf8')).toBe(R1); + + // The next pull updates the rule to a teammate's R2, then fails to mirror + // the docs (a file stands where the docs directory goes); a teammate then + // publishes R3 before any other pull. + fs.rmSync(path.join(home, '.teamai', 'docs'), { recursive: true, force: true }); + fs.writeFileSync(path.join(home, '.teamai', 'docs'), 'not a directory\n'); + teammatePublishes('# Team rule\n\nVersion two, from a teammate.\n'); + const pull = await runCLI(['pull'], work, home); + expect(pull.output).toContain('Failed to sync docs'); + expect(fs.readFileSync(localRule(), 'utf8'), pull.output).toContain('Version two'); + const R3 = '# Team rule\n\nVersion three, from a teammate.\n'; + teammatePublishes(R3); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R3); + // The failed mirror still leaves the revision marker cleared for a retry. + expect(userState().lastPullRev).toBeNull(); + }); + + it('keeps comparing with the last pull\'s revision in an install no pull has recorded', async () => { + await run(['pull']); + // An install upgraded from a CLI that kept no user-scope record. + const statePath = path.join(home, '.teamai', 'state.json'); + const { lastPullByWorkspace: _dropped, ...unrecorded } = userState(); + fs.writeFileSync(statePath, `${JSON.stringify(unrecorded, null, 2)}\n`); + expect(await run(['pull'])).toContain('Already synced'); + + const R2 = '# Team rule\n\nVersion two, from a teammate.\n'; + teammatePublishes(R2); + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(push).not.toContain('no pull record yet'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R2); + + // HOME is the user scope's only checkout, so lastPullRev is its own: an + // edit is pushed, not refused as it is in an unrecorded project checkout. + fs.writeFileSync(localRule(), `${R2}\nA local edit.\n`); + const edited = await run(['--dry-run', 'push']); + expect(edited).toContain('[rules] team-rule (modified)'); + }); + + it('records the revision a push synced HOME\'s copy to in an install no pull has recorded', async () => { + await run(['pull']); + const { lastPullByWorkspace: _dropped, ...unrecorded } = userState(); + fs.writeFileSync(path.join(home, '.teamai', 'state.json'), `${JSON.stringify(unrecorded, null, 2)}\n`); + + // The first push syncs the unedited copy from R1 to a teammate's R2; a + // teammate then publishes R3 before any pull. + teammatePublishes('# Team rule\n\nVersion two, from a teammate.\n'); + await run(['--dry-run', 'push']); + expect(fs.readFileSync(localRule(), 'utf8')).toContain('Version two'); + const R3 = '# Team rule\n\nVersion three, from a teammate.\n'; + teammatePublishes(R3); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-rule (modified)'); + expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R3); + // The record starts from the last pull's revision and keeps both pushes'. + const records = Object.values(userState().lastPullByWorkspace ?? {}); + expect(records).toHaveLength(1); + expect(records[0]?.rev).toBe(unrecorded.lastPullRev); + expect(records[0]?.pushBaseRevs).toHaveLength(2); + }); }); describe('placed agent in a stale linked worktree (#823 item 3)', () => { @@ -306,3 +603,115 @@ describe('placed agent in a stale linked worktree (#823 item 3)', () => { expect(fs.readFileSync(agentIn(projectRoot), 'utf8')).toBe(A1); }, 60_000); }); + +describe('skills a pull held on a namespace collision (#823)', () => { + let sandbox: string; + let home: string; + let teammate: string; + + const skillMd = (body: string): string => `---\nname: team-skill\ndescription: Team skill\n---\n\n${body}\n`; + const S1 = skillMd('Version one.'); + + beforeEach(() => { + requireCli(); + sandbox = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-issue823-held-e2e-'))); + home = path.join(sandbox, 'home'); + teammate = path.join(sandbox, 'teammate'); + const seed = path.join(sandbox, 'seed'); + const remote = path.join(sandbox, 'team-remote.git'); + + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'manifest'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'skills', 'alpha', 'team-skill'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), [ + 'team: issue-823-held-e2e', + 'repo: https://example.com/team.git', + 'provider: tgit', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'manifest', 'roles.yaml'), [ + 'version: 1', + 'roles:', + ' - id: dev', + ' resources:', + ' knowledge: []', + ' skills: [alpha, beta]', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'skills', 'alpha', 'team-skill', 'SKILL.md'), S1); + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + git(['clone', '-q', '--bare', seed, remote], sandbox); + git(['clone', '-q', remote, teammate], sandbox); + }); + + afterEach(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + /** Clone the team repo for `scope` and return where the scope runs and delivers. */ + const install = (scope: 'user' | 'project'): { cwd: string; skill: string } => { + const remote = path.join(sandbox, 'team-remote.git'); + const cwd = scope === 'user' ? path.join(sandbox, 'work') : path.join(sandbox, 'project'); + const dataHome = scope === 'user' ? path.join(home, '.teamai') : path.join(cwd, '.teamai'); + const teamRepo = path.join(dataHome, 'team-repo'); + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + git(['clone', '-q', remote, teamRepo], sandbox); + fs.writeFileSync(path.join(dataHome, 'config.yaml'), [ + 'repo:', + ` localPath: ${teamRepo}`, + ` remote: ${remote}`, + 'username: ci-823-held', + 'updatePolicy: auto', + `scope: ${scope}`, + ...(scope === 'project' ? [`projectRoot: ${cwd}`] : []), + 'primaryRole: dev', + 'enabledAgents: [claude]', + '', + ].join('\n')); + const skillsHome = scope === 'user' ? home : cwd; + return { cwd, skill: path.join(skillsHome, '.claude', 'skills', 'team-skill', 'SKILL.md') }; + }; + const teammateCommits = (message: string, change: () => void): void => { + change(); + git(['add', '-A'], teammate); + git(['commit', '-q', '-m', message], teammate); + git(['push', '-q', 'origin', 'main'], teammate); + }; + + it.each(['user', 'project'] as const)('keeps the base of a %s-scope skill a pull held, so push does not list it as modified', async (scope) => { + const { cwd, skill } = install(scope); + const run = async (args: string[]): Promise => { + const r = await runCLI(args, cwd, home); + expect(r.code, r.output).toBe(0); + return r.output; + }; + const first = await run(['pull']); + expect(fs.existsSync(skill), first).toBe(true); + expect(fs.readFileSync(skill, 'utf8')).toBe(S1); + + // A teammate updates the skill and adds a second one of the same name to + // another active namespace: the next pull holds skills, so the copy stays + // at S1 while the pull records the new revision. + teammateCommits('update and collide', () => { + fs.writeFileSync(path.join(teammate, 'skills', 'alpha', 'team-skill', 'SKILL.md'), skillMd('Version two.')); + fs.mkdirSync(path.join(teammate, 'skills', 'beta', 'team-skill'), { recursive: true }); + fs.writeFileSync(path.join(teammate, 'skills', 'beta', 'team-skill', 'SKILL.md'), skillMd('Another one.')); + }); + const held = await run(['pull']); + expect(held).toContain('Skills were not updated this run'); + expect(fs.readFileSync(skill, 'utf8')).toBe(S1); + + // The teammate resolves the collision and updates the skill again. + const S3 = skillMd('Version three.'); + teammateCommits('resolve collision', () => { + fs.rmSync(path.join(teammate, 'skills', 'beta'), { recursive: true, force: true }); + fs.writeFileSync(path.join(teammate, 'skills', 'alpha', 'team-skill', 'SKILL.md'), S3); + }); + + const push = await run(['--dry-run', 'push']); + expect(push).not.toContain('team-skill (modified)'); + expect(fs.readFileSync(skill, 'utf8'), push).toBe(S3); + }, 60_000); +}); diff --git a/src/__tests__/pull-namespace-override.test.ts b/src/__tests__/pull-namespace-override.test.ts index a9bb2d62f..d679ca78f 100644 --- a/src/__tests__/pull-namespace-override.test.ts +++ b/src/__tests__/pull-namespace-override.test.ts @@ -270,7 +270,12 @@ describe('pull: an active namespace item replaces the root item of the same name // The admin edits the root rule and adds its namespace override in one push: // the member's copy is the version of the last pull, not a member edit. - it('withdraws the replaced root rule\'s copy when the root rule changed in the same push, and names an edited one', async () => { + // HOME's copy may come from a project pull that inherits the user scope, + // which records its revision apart from the user scope's own (#823). + it.each([ + ['the last pull', (rev: string) => ({ lastPullRev: rev })], + ['an inherited pull', (rev: string) => ({ lastPullRev: null, lastInheritedPullRev: rev })], + ])('withdraws the replaced root rule\'s copy when the root rule changed in the same push since %s, and names an edited one', async (_case, delivered) => { const base = await loadTeamConfig(repoPath); if (!base) throw new Error('no team config'); vi.mocked(loadTeamConfig).mockResolvedValue({ @@ -294,7 +299,7 @@ describe('pull: an active namespace item replaces the root item of the same name await team('rules/frontend/tone.md', '# Front tone\n'); git('add', '-A'); git('commit', '-q', '-m', 'v2'); - vi.mocked(loadStateForScope).mockResolvedValue({ lastPull: null, lastPullRev: v1 } as never); + vi.mocked(loadStateForScope).mockResolvedValue({ lastPull: null, ...delivered(v1) } as never); try { await pull({}); } finally { diff --git a/src/__tests__/pull-sync-truth.test.ts b/src/__tests__/pull-sync-truth.test.ts index 77ec33536..651829443 100644 --- a/src/__tests__/pull-sync-truth.test.ts +++ b/src/__tests__/pull-sync-truth.test.ts @@ -63,7 +63,7 @@ vi.mock('../update.js', () => ({ releaseLock: vi.fn().mockResolvedValue(undefined), })); -import { pull } from '../pull.js'; +import { checkoutKey, pull } from '../pull.js'; import { detectProjectConfig, loadLocalConfigForScope, loadTeamConfig, loadStateForScope, saveStateForScope } from '../config.js'; import { log } from '../utils/logger.js'; import type { TeamaiConfig, LocalConfig } from '../types.js'; @@ -210,6 +210,23 @@ describe('pull reports what reached the tool directory (#585)', () => { } }); + it('adds the revision a pull delivered to the checkout\'s push bases when its docs mirror fails (#823)', async () => { + const projectRoot = path.join(tmpDir, 'project'); + await fse.ensureDir(projectRoot); + vi.mocked(detectProjectConfig).mockResolvedValue({ ...localConfig, scope: 'project', projectRoot }); + const key = await checkoutKey(projectRoot); + const state = await loadStateForScope(localConfig); + state.lastPullByWorkspace = { [key]: { rev: 'old1234', targets: [] } }; + await fse.outputFile(path.join(projectRoot, 'docs'), 'blocks docs directory'); + + await pull({ silent: true, force: true }); + + expect(vi.mocked(log.warn).mock.calls.flat()).toContainEqual(expect.stringContaining('[project] Failed to sync docs:')); + // The marker stays cleared for a retry, and the record keeps its rev. + expect(state.lastPullRev).toBeNull(); + expect(state.lastPullByWorkspace?.[key]).toEqual({ rev: 'old1234', targets: [], pushBaseRevs: ['abc1234'] }); + }); + it.each(['empty', 'missing'])('prunes only stale empty directories when the team bundle is %s', async (state) => { await fse.remove(path.join(repoPath, 'docs')); if (state === 'empty') await fse.ensureDir(path.join(repoPath, 'docs')); diff --git a/src/pull.ts b/src/pull.ts index 33247a0de..b91b7c867 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -550,7 +550,7 @@ async function liveCheckoutRecords( return Object.fromEntries(Object.entries(records).filter(([key]) => live.has(key))); } -type CheckoutRecord = NonNullable[string]; +export type CheckoutRecord = NonNullable[string]; /** * The `rev` a forced full sync (lastPullRev cleared) leaves on every other @@ -588,6 +588,79 @@ export function addPushBaseRev(record: CheckoutRecord, rev: string): void { record.pushBaseRevs = [rev, ...older].slice(0, MAX_PUSH_BASE_REVS); } +/** + * The key of the checkout `localConfig`'s pulls deliver into: the project + * checkout, or HOME for the user scope, whose state.json no other checkout + * shares (#823). Undefined for a project scope without a root. + */ +async function checkoutRecordKey(localConfig: LocalConfig): Promise { + switch (localConfig.scope) { + case 'user': + return checkoutKey(getUserHome()); + case 'project': + return localConfig.projectRoot ? checkoutKey(localConfig.projectRoot) : undefined; + default: { + const unhandled: never = localConfig.scope; + throw new Error(`Unknown scope: ${String(unhandled)}`); + } + } +} + +/** + * The bases push compares this checkout's unedited copies with. `checkout`: + * the revisions its own record holds (see checkoutBaseRevs); push adds the one + * its sync reaches to `record`. `shared`: the record holds none, so the shared + * lastPullRev stands in. `unrecorded` marks a project checkout no pull has + * recorded, where that revision may be another checkout's (#812). The user + * scope has one checkout, HOME, so an install from before its record keeps + * the revisions of its last full and inherited pulls (see homeRevs) until a + * push or pull creates the record (see userScopeRecord). + */ +export type CheckoutBases = + | { source: 'checkout'; record: CheckoutRecord; revs: string[] } + | { source: 'shared'; revs: string[]; unrecorded: boolean }; + +export async function resolveCheckoutBases( + localConfig: LocalConfig, + state: Pick, +): Promise { + const key = await checkoutRecordKey(localConfig); + const record = key ? state.lastPullByWorkspace?.[key] : undefined; + const revs = checkoutBaseRevs(record); + if (record && revs.length > 0) return { source: 'checkout', record, revs }; + return { + source: 'shared', + revs: localConfig.scope === 'user' ? homeRevs(state) : state.lastPullRev ? [state.lastPullRev] : [], + unrecorded: localConfig.scope === 'project' && key !== undefined && !record, + }; +} + +/** + * The revisions HOME's copies may hold in a user-scope install without a + * record: the last full pull's and the last inherited pull's, as either may + * have run last and nothing records which. A copy at either is unedited (#823). + */ +function homeRevs(state: Pick): string[] { + return [...new Set([state.lastPullRev, state.lastInheritedPullRev])] + .filter((rev): rev is string => typeof rev === 'string' && rev !== FORCED_FULL_SYNC_REV); +} + +/** + * HOME's record in the user scope's `state`, added from the shared fields when + * an install from before the record has none: HOME is the scope's only + * checkout, so lastPullRev is its own revision, and the last inherited pull's + * one of its push bases (#823). + */ +export async function userScopeRecord(state: State): Promise { + const key = await checkoutKey(getUserHome()); + const rev = state.lastPullRev ?? FORCED_FULL_SYNC_REV; + const pushBaseRevs = homeRevs(state).filter((base) => base !== rev); + const record = state.lastPullByWorkspace?.[key] + ?? { rev, targets: state.lastPullTargets ?? [], ...(pushBaseRevs.length > 0 ? { pushBaseRevs } : {}) }; + state.lastPullByWorkspace = { ...state.lastPullByWorkspace, [key]: record }; + return record; +} + /** `records` after a forced full sync: see FORCED_FULL_SYNC_REV. */ function awaitingFullSync(records: Record): Record { return Object.fromEntries(Object.entries(records).map(([key, record]) => { @@ -620,11 +693,11 @@ async function pullForScope( ? 'lastPullTargets' as const : 'lastInheritedPullTargets' as const; // This checkout's key in state.lastPullByWorkspace. Only a project scope - // delivers into its own checkout; the user scope and inherited pulls write - // under HOME, which every worktree shares (#807). - const workspaceKey = revisionField === 'lastPullRev' && localConfig.scope === 'project' && localConfig.projectRoot - ? await checkoutKey(localConfig.projectRoot) - : null; + // delivers into its own checkout, so only its record gates the fast path. The + // user scope delivers under HOME, which every worktree shares (#807), and + // records it for push's bases alone, inherited pulls included (#823). + const recordKey = await checkoutRecordKey(localConfig); + const workspaceKey = revisionField === 'lastPullRev' && localConfig.scope === 'project' ? recordKey : undefined; // Step 1: refresh team repo (git pull, or HTTP /repo materialization) const pullSpin = spinner(`[${scopeLabel}] Pulling team repo...`).start(); @@ -980,6 +1053,8 @@ async function pullForScope( // Set when two active namespaces collide on a skill: skills are neither // installed nor cleaned up this run. let skillsHeld = false; + // Set on the same collision among agents. + let agentsHeld = false; let knownRepoSkillNames: Set | null = null; // name → team-repo source dir, for the data-safety check in Step 3b cleanup. let knownRepoSkillSources: Map | null = null; @@ -1081,6 +1156,7 @@ async function pullForScope( // Only agents stop; the revocation pass below sees the same collision // and leaves them alone too. log.warn(`[${scopeLabel}] ${describeDeliveryConflict(desired)}. Agents were not updated this run; the installed ones are kept.`); + agentsHeld = true; continue; } items = desired.items; @@ -1271,41 +1347,64 @@ async function pullForScope( // a chance to run. Inherited pulls use an independent marker so a partial, // safe sync can never suppress a later full user-scope pull. // A failed docs mirror must be retried even when the team revision is unchanged. - if (!options.dryRun && !docsSyncFailed) { + if (!options.dryRun) { const state = await loadStateForScope(localConfig); - if (revisionField === 'lastPullRev') { - state.lastPull = new Date().toISOString(); - } - const previousRev = state[revisionField]; - // A failed submodule update keeps the previous rev so the next pull - // retries the update (see refreshTeamRepo). - if (!submodulesFailed) { - if (currentRev !== null) { - state[revisionField] = currentRev; - } else { - try { - state[revisionField] = await getHeadRev(localConfig.repo.localPath); - } catch { - state[revisionField] = null; - } + let deliveredRev = currentRev; + if (deliveredRev === null) { + try { + deliveredRev = await getHeadRev(localConfig.repo.localPath); + } catch { + deliveredRev = null; } } + const previousRev = state[revisionField]; + // Skills or agents a collision held stay at the revisions push compared + // them with before this pull: read those before the marker moves below, so + // the new record keeps them, as push keeps its own (#823). + const heldBases = (skillsHeld || agentsHeld) && recordKey + ? await resolveCheckoutBases(localConfig, state) + : undefined; + const keptBases = heldBases && (heldBases.source === 'checkout' || localConfig.scope === 'user') + ? heldBases.revs.filter((rev) => rev !== deliveredRev).slice(0, MAX_PUSH_BASE_REVS) + : []; const syncedTargets = currentTargets ?? await getInstalledResourceTargets(freshConfig, localConfig); - state[targetsField] = syncedTargets; - const syncedRev = state[revisionField]; - if (workspaceKey && localConfig.projectRoot && !submodulesFailed && syncedRev) { + if (!docsSyncFailed) { + if (revisionField === 'lastPullRev') { + state.lastPull = new Date().toISOString(); + } + // A failed submodule update keeps the previous rev so the next pull + // retries the update (see refreshTeamRepo). + if (!submodulesFailed) state[revisionField] = deliveredRev; + state[targetsField] = syncedTargets; + } + const complete = !docsSyncFailed && !submodulesFailed; + if (recordKey && deliveredRev && (!complete || revisionField === 'lastInheritedPullRev')) { + // An inherited pull moves HOME's skills, rules and agents, not the rest, + // and an incomplete one keeps its marker for a retry, yet both delivered + // those at deliveredRev: add it to the push bases and keep the record's + // rev, or push reads the untouched copies as edits (#823). + const record = localConfig.scope === 'user' + ? await userScopeRecord(state) + : state.lastPullByWorkspace?.[recordKey] ?? { rev: FORCED_FULL_SYNC_REV, targets: syncedTargets }; + addPushBaseRev(record, deliveredRev); + state.lastPullByWorkspace = { ...state.lastPullByWorkspace, [recordKey]: record }; + } else if (recordKey && deliveredRev) { // A forced full sync (lastPullRev cleared) leaves every other checkout // out of date too: reset their records so each one does its own full // sync, instead of only the first checkout to pull, keeping the base its // push needs to tell a teammate's update from the member's edit (#812). // A new revision resets nothing: a checkout recorded at an older one // already misses the fast path. Records of removed worktrees are dropped. - const live = await liveCheckoutRecords(localConfig.projectRoot, state.lastPullByWorkspace); + // The user scope's state holds no other checkout. + const live = workspaceKey && localConfig.projectRoot + ? await liveCheckoutRecords(localConfig.projectRoot, state.lastPullByWorkspace) + : undefined; const others = previousRev === null && live ? awaitingFullSync(live) : live; + const record: CheckoutRecord = { rev: deliveredRev, targets: syncedTargets }; state.lastPullByWorkspace = { ...others, - [workspaceKey]: { rev: syncedRev, targets: syncedTargets }, + [recordKey]: keptBases.length > 0 ? { ...record, pushBaseRevs: keptBases } : record, }; } await saveStateForScope(state, localConfig); diff --git a/src/push.ts b/src/push.ts index 01d9c2377..2058a71f5 100644 --- a/src/push.ts +++ b/src/push.ts @@ -1014,24 +1014,15 @@ async function pushCore( // Compare with the revisions THIS checkout synced: state.json is shared by // every worktree, and a pull in another checkout moves the shared // lastPullRev past a copy this checkout still holds unedited (#812). - const { checkoutKey, checkoutBaseRevs, addPushBaseRev } = await import('./pull.js'); - const key = localConfig.scope === 'project' && localConfig.projectRoot - ? await checkoutKey(localConfig.projectRoot) - : undefined; - const checkoutRecord = key ? state.lastPullByWorkspace?.[key] : undefined; - unrecordedCheckout = key !== undefined && !checkoutRecord; + const { resolveCheckoutBases, addPushBaseRev, userScopeRecord } = await import('./pull.js'); + const bases = await resolveCheckoutBases(localConfig, state); + unrecordedCheckout = bases.source === 'shared' && bases.unrecorded; placedRules = state.placedRules; - const checkoutBases = checkoutBaseRevs(checkoutRecord); try { // placedRules redirects a root-authored rule to the rules// file push // put it in, so a teammate's newer version syncs down instead of being // overwritten by the stale root copy the scan would otherwise call modified. - await syncTeamUpdatesToLocal( - teamConfig, - localConfig, - checkoutBases.length > 0 ? checkoutBases : state.lastPullRev, - state.placedRules, - ); + await syncTeamUpdatesToLocal(teamConfig, localConfig, bases.revs, state.placedRules); } catch (e) { preSyncFailure = e instanceof Error ? e.message : String(e); } @@ -1040,12 +1031,14 @@ async function pushCore( // (the copies it did not reach still match an older base) and even under // --dry-run (the sync has already written the files). The pull record's // `rev` stays, or the next pull would skip the docs and agents of this - // revision. - const syncedRev = checkoutRecord && checkoutBases.length > 0 && !teamRepoStale + // revision. HOME gets a record here if it has none yet; an unrecorded + // project checkout does not, as its fallback base may be another's. + const recordsBase = bases.source === 'checkout' || localConfig.scope === 'user'; + const syncedRev = recordsBase && !teamRepoStale ? await getHeadCommit(localConfig.repo.localPath) : null; - if (checkoutRecord && syncedRev) { - addPushBaseRev(checkoutRecord, syncedRev); + if (syncedRev) { + addPushBaseRev(bases.source === 'checkout' ? bases.record : await userScopeRecord(state), syncedRev); try { await saveStateForScope(state, localConfig); } catch (e) { diff --git a/src/resources/agents.ts b/src/resources/agents.ts index e5b7e7310..33be1bede 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -201,18 +201,14 @@ 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, lastPullByWorkspace, pendingPushes } = await loadStateForScope(localConfig); + const { placedAgents, lastPullRev, lastInheritedPullRev, 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] : []; + const { resolveCheckoutBases } = await import('../pull.js'); + return (await resolveCheckoutBases(localConfig, { lastPullRev, lastInheritedPullRev, lastPullByWorkspace })).revs; }; // Agents this machine placed in a namespace and has awaiting review: the // open PR is their destination, not "no active source". diff --git a/src/resources/rules.ts b/src/resources/rules.ts index ded5391fc..6fd56e174 100644 --- a/src/resources/rules.ts +++ b/src/resources/rules.ts @@ -125,12 +125,13 @@ export class RulesHandler extends ResourceHandler { ? copilotInstructionsBodyEqualsTeamMd(localRule, teamRule) : await fileContentEqual(localFilePath, teamFilePath); if (equal) continue; // This tool dir's copy is identical, skip - // Single-repo mode: nothing refreshes the author's `.teamai/rules` - // copy of a rule placed under a namespace — pull deploys to tool - // dirs and the pre-push sync covers those — so a copy equal to an - // OLDER version of the team file is one nobody edited, and pushing - // it would revert whoever changed the rule since (#649 review). - if (tool === SELF_KNOWLEDGE_SCAN_KEY && teamFileName === placedName + // Single-repo mode: nothing refreshes the active tree's + // `.teamai/rules` — pull deploys to tool dirs and the pre-push sync + // covers those — and a branch behind the default branch holds its + // older copies. A copy equal to an OLDER version of the team file is + // one nobody edited, and pushing it would revert whoever changed the + // rule since (#649 review, #823). + if (tool === SELF_KNOWLEDGE_SCAN_KEY && await isPastVersionOf(localConfig.repo.localPath, localFilePath, teamRelPath)) { log.warn( `[rules] Skipped ${name}: ${path.relative(resolveToolBaseDir(tool, localConfig), localFilePath)} is an ` @@ -472,7 +473,8 @@ export class RulesHandler extends ResourceHandler { // and only while the team file it points at still exists. Before the PR // merges there is no record yet — the placement is on the pending entry — // and the copy is just as much ours then. - const { placedRules, pendingPushes, lastPullRev } = await loadStateForScope(localConfig); + const state = await loadStateForScope(localConfig); + const { placedRules, pendingPushes } = state; for (const name of Object.keys(placedRules ?? {})) { const placed = placedResourcePath(placedRules, 'rules', name); if (placed && await pathExists(path.join(localConfig.repo.localPath, placed))) { @@ -492,6 +494,11 @@ export class RulesHandler extends ResourceHandler { } const tombstones = await this.readTombstones(localConfig); const replacedByName = new Map(replacedRoots.map((rule) => [rule.name, rule])); + // The revisions this checkout's copies can be at: the shared lastPullRev + // may be another checkout's, and HOME's copy an inherited pull's (#823). + const deliveredRevs = replacedRoots.length > 0 + ? (await (await import('../pull.js')).resolveCheckoutBases(localConfig, state)).revs + : []; for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { if (!toolPath.rules) continue; // `pullItem` above skips excluded tools, so this pass must skip them too. @@ -519,7 +526,7 @@ export class RulesHandler extends ResourceHandler { const replaced = teamRuleNames.has(ruleName) ? undefined : replacedByName.get(ruleName); if (replaced === undefined || localFile !== `${ruleName}${ext}`) continue; const deployed = path.join(destDir, localFile); - if (await isDeliveredRender(tool, deployed, replaced, localConfig.repo.localPath, lastPullRev ?? null)) { + if (await isDeliveredRender(tool, deployed, replaced, localConfig.repo.localPath, deliveredRevs)) { await remove(deployed); log.debug(`Removed ${localFile} from ${tool}: a namespace rule replaces it`); } else { @@ -672,23 +679,26 @@ function renderRuleForTool(tool: string, source: string): string { /** * Whether `deployed` holds exactly what pull renders for `tool` from the team - * rule, as it is now or as it was at the last pull: a root rule edited in the - * same push that adds its namespace override leaves the older render behind, - * which nobody edited. + * rule, as it is now or as it was at one of `deliveredRevs`: a root rule + * edited in the same push that adds its namespace override leaves the older + * render behind, which nobody edited. */ async function isDeliveredRender( tool: string, deployed: string, rule: ResourceItem, repoPath: string, - lastPullRev: string | null, + deliveredRevs: readonly string[], ): Promise { const current = await readFileSafe(deployed); if (current === null) return false; const team = await readFileSafe(rule.sourcePath); if (team !== null && current === renderRuleForTool(tool, team)) return true; - const atLastPull = lastPullRev ? await getFileContentAtRev(repoPath, lastPullRev, `./${rule.relativePath}`) : null; - return atLastPull !== null && current === renderRuleForTool(tool, atLastPull.toString('utf-8')); + for (const rev of deliveredRevs) { + const delivered = await getFileContentAtRev(repoPath, rev, `./${rule.relativePath}`); + if (delivered !== null && current === renderRuleForTool(tool, delivered.toString('utf-8'))) return true; + } + return false; } /** diff --git a/src/resources/skills.ts b/src/resources/skills.ts index be78c10fe..72a5761d9 100644 --- a/src/resources/skills.ts +++ b/src/resources/skills.ts @@ -2,9 +2,10 @@ import path from 'node:path'; import YAML from 'yaml'; import { isToolInstalledForConfig, ResourceHandler } from './base.js'; import type { ResourceItem, ResourceItemStatus, DeliveryTarget, TeamaiConfig, LocalConfig } from '../types.js'; -import { getPushignorePath, isAgentExcluded, resolveToolBaseDir, scopedToolPaths } from '../types.js'; +import { getPushignorePath, isAgentExcluded, resolveToolBaseDir, scopedToolPaths, SELF_KNOWLEDGE_SCAN_KEY } from '../types.js'; import { listDirs, listFilesRecursive, pathExists, copyDir, remove, pruneEmptyDirs, dirContentEqual, dirTeamSubsetEqual, fileContentEqual, getDirLatestMtime, readFileSafe, writeFile } from '../utils/fs.js'; import { log } from '../utils/logger.js'; +import { getFileContentWhenAdded, isPastVersionOf } from '../utils/git.js'; import { isCliOwnedSkillName } from '../builtin-skills.js'; import { resolveOpenclawWorkspaceDir } from '../openclaw-hooks.js'; import { getHermesHome } from '../hermes-home.js'; @@ -426,6 +427,31 @@ async function removeLeftoverVersionFiles(source: string, dest: string, otherVer if (removed) await pruneEmptyDirs(dest); } +/** + * Whether every team file of the skill at `teamDir` (`teamRelDir` in the team + * repo at `repoPath`) whose copy under `localDir` differs is an older version + * of that team file. A team file missing locally is one a teammate added since + * when the active branch at `activeRoot` never added it, and the member's + * deletion otherwise. Files only the member has are ignored, as + * dirTeamSubsetEqual ignores them. + */ +async function isPastSkillVersion( + repoPath: string, activeRoot: string, localDir: string, teamDir: string, teamRelDir: string, +): Promise { + for (const rel of await listFilesRecursive(teamDir)) { + if (rel.split('/').includes(CONTRIBUTORS_FILE)) continue; + const localFile = path.join(localDir, rel); + if (await fileContentEqual(localFile, path.join(teamDir, rel))) continue; + if (!await pathExists(localFile)) { + const activeRel = path.relative(activeRoot, localFile).split(path.sep).join('/'); + if (await getFileContentWhenAdded(activeRoot, activeRel) === null) continue; + return false; + } + if (!await isPastVersionOf(repoPath, localFile, `${teamRelDir}/${rel}`)) return false; + } + return true; +} + export class SkillsHandler extends ResourceHandler { readonly type = 'skills' as const; @@ -546,6 +572,19 @@ export class SkillsHandler extends ResourceHandler { const teamDirPath = teamSkills.get(dir)!.dir; const equal = await dirTeamSubsetEqual(localDirPath, teamDirPath, [CONTRIBUTORS_FILE]); if (equal) continue; // This tool dir's copy is identical, skip + // Single-repo mode: like `.teamai/rules` (see the rules scan), the + // active tree's `.teamai/skills` is never refreshed, and a branch + // behind the default branch holds older copies nobody edited (#823). + const teamRelDir = path.relative(localConfig.repo.localPath, teamDirPath).split(path.sep).join('/'); + if (tool === SELF_KNOWLEDGE_SCAN_KEY && localConfig.projectRoot + && await isPastSkillVersion(localConfig.repo.localPath, localConfig.projectRoot, localDirPath, teamDirPath, teamRelDir)) { + log.warn( + `[skills] Skipped ${dir}: ${path.relative(resolveToolBaseDir(tool, localConfig), localDirPath)} is an older ` + + `version of ${teamRelDir}, which has changed on the team since. ` + + 'Copy the current files over it (or delete it) before editing.', + ); + continue; + } // Content differs — candidate for "modified" const mtime = await getDirLatestMtime(localDirPath); diff --git a/src/types.ts b/src/types.ts index b6cae87f2..07a3ac674 100644 --- a/src/types.ts +++ b/src/types.ts @@ -687,7 +687,9 @@ export const StateSchema = z.object({ * unedited rules and skills to since its last pull, newest first, the bases * its next push compares with; a pull's record drops them, since the pull * delivers `rev` (#812). A forced full sync elsewhere leaves `rev` empty - * (`FORCED_FULL_SYNC_REV` in pull.ts). + * (`FORCED_FULL_SYNC_REV` in pull.ts). The user scope's entry is HOME's. An + * inherited pull, and a pull whose docs mirror or submodule update fails, add + * the revision they delivered to these bases and keep `rev` (#823). */ lastPullByWorkspace: z.record(z.string(), z.object({ rev: z.string(),