From 6c46ee46ffd0610361bac23c980e2998800212f8 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Fri, 25 Sep 2026 18:47:09 +0200 Subject: [PATCH 1/4] fix(push): stop reverting a teammate's update from HOME or a stale .teamai copy (#823) Item 4, user scope: push compared HOME's rules and skills with the shared lastPullRev only, because the per-checkout push bases of #819 were keyed for project scope alone. After a push synced HOME's unedited copy to a teammate's R2, the next push compared it with R1 and offered it back over the teammate's R3. A user-scope pull now records HOME under checkoutKey(HOME) in the user state.json, and push reads and extends it like a project checkout's. The user-scope fast path still reads the shared fields, and an install with no record yet keeps comparing with lastPullRev without the unrecorded-checkout refusal: HOME is the scope's only checkout, so that revision is its own. Item 19, inherited user scope: a project pull with inheritUserScope rewrites HOME's skills, rules and agents under lastInheritedPullRev without moving the user scope's push bases, so the next user-scope push offered a teammate's newer update back the same way. That pull now adds its revision to HOME's pushBaseRevs, creating the record from lastPullRev if there is none, and leaves the record's rev, lastPullRev and the fast paths alone. Item 10, single-repo: the active tree's .teamai/rules and .teamai/skills are push sources, and on a branch behind the default branch they hold older team versions nobody edited, which push listed as modified. The isPastVersionOf guard that held only placed rules now covers every .teamai/rules copy, and .teamai/skills gets the same guard: a skill is skipped with a warning when every team file whose copy differs is an older version of it. A team file missing locally is a teammate's addition when the member's branch never added it, and the member's deletion otherwise; member-only files are ignored, as the equality check already ignores them. The checkout-base resolution that push and the agents scan each repeated (key, record, checkoutBaseRevs, lastPullRev fallback) is now one exported helper, resolveCheckoutBases, next to checkoutBaseRevs in pull.ts; pull uses the same key for its record. --- docs/designs/data-directory-layout.md | 10 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- .../e2e/push-sync-followups-823.test.ts | 241 +++++++++++++++++- src/pull.ts | 74 +++++- src/push.ts | 23 +- src/resources/agents.ts | 8 +- src/resources/rules.ts | 13 +- src/resources/skills.ts | 41 ++- src/types.ts | 3 +- 10 files changed, 374 insertions(+), 43 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 87ff235cb..5838f751a 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -93,7 +93,15 @@ 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`, which only HOME moves, so push +does not stop there. 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 from `lastPullRev` if +there is none, and leaves the entry's `rev` alone. 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/src/__tests__/e2e/push-sync-followups-823.test.ts b/src/__tests__/e2e/push-sync-followups-823.test.ts index e8317de0a..322fcc5a2 100644 --- a/src/__tests__/e2e/push-sync-followups-823.test.ts +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -11,6 +11,18 @@ * 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. + * + * 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 +30,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 +73,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 +81,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 +107,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 +169,227 @@ 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); + }); + + it.each([ + ['a user-scope pull recorded HOME', false], + ['no pull has recorded HOME (upgraded install)', true], + ])('compares the next push with the revision an inheriting project\'s pull moved HOME\'s copy to, when %s (#823 item 19)', async (_case, unrecorded) => { + await run(['pull']); + if (unrecorded) { + const { lastPullByWorkspace: _dropped, ...rest } = userState(); + fs.writeFileSync(path.join(home, '.teamai', 'state.json'), `${JSON.stringify(rest, null, 2)}\n`); + } + 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'); + 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('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); + expect(userState().lastPullByWorkspace).toBeUndefined(); + + // 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)'); + }); }); describe('placed agent in a stale linked worktree (#823 item 3)', () => { diff --git a/src/pull.ts b/src/pull.ts index 33247a0de..c9e951868 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,52 @@ 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 + * lastPullRev until its next full pull. + */ +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: state.lastPullRev ? [state.lastPullRev] : [], + unrecorded: localConfig.scope === 'project' && key !== undefined && !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 +666,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(); @@ -1294,18 +1340,28 @@ async function pullForScope( ?? await getInstalledResourceTargets(freshConfig, localConfig); state[targetsField] = syncedTargets; const syncedRev = state[revisionField]; - if (workspaceKey && localConfig.projectRoot && !submodulesFailed && syncedRev) { + if (recordKey && !submodulesFailed && syncedRev && revisionField === 'lastInheritedPullRev') { + // An inherited pull moves HOME's skills, rules and agents, not the rest: + // add its revision to HOME's push bases and keep the full pull's. + const record = state.lastPullByWorkspace?.[recordKey] + ?? { rev: state.lastPullRev ?? FORCED_FULL_SYNC_REV, targets: state.lastPullTargets ?? [] }; + addPushBaseRev(record, syncedRev); + state.lastPullByWorkspace = { ...state.lastPullByWorkspace, [recordKey]: record }; + } else if (recordKey && !submodulesFailed && syncedRev) { // 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; state.lastPullByWorkspace = { ...others, - [workspaceKey]: { rev: syncedRev, targets: syncedTargets }, + [recordKey]: { rev: syncedRev, targets: syncedTargets }, }; } await saveStateForScope(state, localConfig); diff --git a/src/push.ts b/src/push.ts index 01d9c2377..95f94e080 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 } = 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); } @@ -1041,11 +1032,11 @@ async function pushCore( // --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 + const syncedRev = bases.source === 'checkout' && !teamRepoStale ? await getHeadCommit(localConfig.repo.localPath) : null; - if (checkoutRecord && syncedRev) { - addPushBaseRev(checkoutRecord, syncedRev); + if (bases.source === 'checkout' && syncedRev) { + addPushBaseRev(bases.record, syncedRev); try { await saveStateForScope(state, localConfig); } catch (e) { diff --git a/src/resources/agents.ts b/src/resources/agents.ts index e5b7e7310..62640d0f6 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -207,12 +207,8 @@ export class AgentsHandler extends ResourceHandler { // 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, 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..a41b3e046 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 ` 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..a81471b0a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -687,7 +687,8 @@ 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, and + * an inherited pull adds its revision to these bases (#823). */ lastPullByWorkspace: z.record(z.string(), z.object({ rev: z.string(), From ad0ce83c11bded9cddc6d6f622850c022a4c042c Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Sat, 26 Sep 2026 09:46:54 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(push):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20record=20HOME's=20push=20base=20in=20an=20upgraded=20install?= =?UTF-8?q?=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An upgraded user-scope install with no HOME record synced HOME from R1 to R2 on its first push but saved no base, because push recorded one only when the bases came from a record. A teammate's R3 then made the next push compare the R2 copy with lastPullRev R1 and offer it back. Push now creates HOME's record from lastPullRev (userScopeRecord, shared with the inherited pull) and adds the revision its sync reached. An unrecorded project checkout still records nothing, since its fallback base may be another checkout's. skill-data: contribute-member explains the stale .teamai copy warning and how to publish an edit of such a copy. --- docs/designs/data-directory-layout.md | 3 ++- .../core/references/contribute-member.md | 7 ++++++ .../e2e/push-sync-followups-823.test.ts | 24 ++++++++++++++++++- src/pull.ts | 20 ++++++++++++---- src/push.ts | 12 ++++++---- 5 files changed, 54 insertions(+), 12 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 5838f751a..7a83136c7 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -97,7 +97,8 @@ 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`, which only HOME moves, so push -does not stop there. A project pull that inherits the user scope +does not stop there; its first push creates the entry from `lastPullRev` 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 from `lastPullRev` if there is none, and leaves the entry's `rev` alone. Like a full pull, an 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 322fcc5a2..bf42cfe60 100644 --- a/src/__tests__/e2e/push-sync-followups-823.test.ts +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -382,7 +382,6 @@ describe('push base in user scope (#823 item 4)', () => { expect(push).not.toContain('team-rule (modified)'); expect(push).not.toContain('no pull record yet'); expect(fs.readFileSync(localRule(), 'utf8'), push).toBe(R2); - expect(userState().lastPullByWorkspace).toBeUndefined(); // 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. @@ -390,6 +389,29 @@ describe('push base in user scope (#823 item 4)', () => { 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)', () => { diff --git a/src/pull.ts b/src/pull.ts index c9e951868..797fa97b4 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -613,7 +613,7 @@ async function checkoutRecordKey(localConfig: LocalConfig): Promise { + const key = await checkoutKey(getUserHome()); + const record = state.lastPullByWorkspace?.[key] + ?? { rev: state.lastPullRev ?? FORCED_FULL_SYNC_REV, targets: state.lastPullTargets ?? [] }; + 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]) => { @@ -1343,10 +1356,7 @@ async function pullForScope( if (recordKey && !submodulesFailed && syncedRev && revisionField === 'lastInheritedPullRev') { // An inherited pull moves HOME's skills, rules and agents, not the rest: // add its revision to HOME's push bases and keep the full pull's. - const record = state.lastPullByWorkspace?.[recordKey] - ?? { rev: state.lastPullRev ?? FORCED_FULL_SYNC_REV, targets: state.lastPullTargets ?? [] }; - addPushBaseRev(record, syncedRev); - state.lastPullByWorkspace = { ...state.lastPullByWorkspace, [recordKey]: record }; + addPushBaseRev(await userScopeRecord(state), syncedRev); } else if (recordKey && !submodulesFailed && syncedRev) { // A forced full sync (lastPullRev cleared) leaves every other checkout // out of date too: reset their records so each one does its own full diff --git a/src/push.ts b/src/push.ts index 95f94e080..2058a71f5 100644 --- a/src/push.ts +++ b/src/push.ts @@ -1014,7 +1014,7 @@ 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 { resolveCheckoutBases, addPushBaseRev } = await import('./pull.js'); + const { resolveCheckoutBases, addPushBaseRev, userScopeRecord } = await import('./pull.js'); const bases = await resolveCheckoutBases(localConfig, state); unrecordedCheckout = bases.source === 'shared' && bases.unrecorded; placedRules = state.placedRules; @@ -1031,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 = bases.source === 'checkout' && !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 (bases.source === 'checkout' && syncedRev) { - addPushBaseRev(bases.record, syncedRev); + if (syncedRev) { + addPushBaseRev(bases.source === 'checkout' ? bases.record : await userScopeRecord(state), syncedRev); try { await saveStateForScope(state, localConfig); } catch (e) { From 32bffbdb88aa7681e3c4f29c2a98a0f2e401d4b1 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Sat, 26 Sep 2026 10:06:31 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(push):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20keep=20HOME's=20inherited=20base=20and=20a=20partial=20pull'?= =?UTF-8?q?s=20delivered=20base=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/designs/data-directory-layout.md | 18 +++-- .../e2e/push-sync-followups-823.test.ts | 52 +++++++++++-- src/__tests__/pull-sync-truth.test.ts | 19 ++++- src/pull.ts | 78 ++++++++++++------- src/resources/agents.ts | 4 +- src/types.ts | 5 +- 6 files changed, 127 insertions(+), 49 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 7a83136c7..a80a798b9 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -96,13 +96,17 @@ state.json written before this field has no entry, so each checkout does one 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`, which only HOME moves, so push -does not stop there; its first push creates the entry from `lastPullRev` 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 from `lastPullRev` if -there is none, and leaves the entry's `rev` alone. Like a full pull, an -inherited pull already synced at the team's revision writes nothing (#823). +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. 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/src/__tests__/e2e/push-sync-followups-823.test.ts b/src/__tests__/e2e/push-sync-followups-823.test.ts index bf42cfe60..d89cdc154 100644 --- a/src/__tests__/e2e/push-sync-followups-823.test.ts +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -17,7 +17,9 @@ * 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. + * 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 @@ -330,15 +332,18 @@ describe('push base in user scope (#823 item 4)', () => { 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', false], - ['no pull has recorded HOME (upgraded install)', true], - ])('compares the next push with the revision an inheriting project\'s pull moved HOME\'s copy to, when %s (#823 item 19)', async (_case, unrecorded) => { + ['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 (unrecorded) { - const { lastPullByWorkspace: _dropped, ...rest } = userState(); - fs.writeFileSync(path.join(home, '.teamai', 'state.json'), `${JSON.stringify(rest, null, 2)}\n`); - } + 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); @@ -360,12 +365,43 @@ describe('push base in user scope (#823 item 4)', () => { 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 () => { 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 797fa97b4..1c4533c39 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -613,7 +613,8 @@ async function checkoutRecordKey(localConfig: LocalConfig): Promise, + state: Pick, ): Promise { const key = await checkoutRecordKey(localConfig); const record = key ? state.lastPullByWorkspace?.[key] : undefined; @@ -629,20 +630,33 @@ export async function resolveCheckoutBases( if (record && revs.length > 0) return { source: 'checkout', record, revs }; return { source: 'shared', - revs: state.lastPullRev ? [state.lastPullRev] : [], + 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 (#823). + * 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: state.lastPullRev ?? FORCED_FULL_SYNC_REV, targets: state.lastPullTargets ?? [] }; + ?? { rev, targets: state.lastPullTargets ?? [], ...(pushBaseRevs.length > 0 ? { pushBaseRevs } : {}) }; state.lastPullByWorkspace = { ...state.lastPullByWorkspace, [key]: record }; return record; } @@ -1330,34 +1344,40 @@ 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]; const syncedTargets = currentTargets ?? await getInstalledResourceTargets(freshConfig, localConfig); - state[targetsField] = syncedTargets; - const syncedRev = state[revisionField]; - if (recordKey && !submodulesFailed && syncedRev && revisionField === 'lastInheritedPullRev') { - // An inherited pull moves HOME's skills, rules and agents, not the rest: - // add its revision to HOME's push bases and keep the full pull's. - addPushBaseRev(await userScopeRecord(state), syncedRev); - } else if (recordKey && !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 @@ -1371,7 +1391,7 @@ async function pullForScope( const others = previousRev === null && live ? awaitingFullSync(live) : live; state.lastPullByWorkspace = { ...others, - [recordKey]: { rev: syncedRev, targets: syncedTargets }, + [recordKey]: { rev: deliveredRev, targets: syncedTargets }, }; } await saveStateForScope(state, localConfig); diff --git a/src/resources/agents.ts b/src/resources/agents.ts index 62640d0f6..33be1bede 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -201,14 +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 { resolveCheckoutBases } = await import('../pull.js'); - return (await resolveCheckoutBases(localConfig, { lastPullRev, lastPullByWorkspace })).revs; + 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/types.ts b/src/types.ts index a81471b0a..07a3ac674 100644 --- a/src/types.ts +++ b/src/types.ts @@ -687,8 +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). The user scope's entry is HOME's, and - * an inherited pull adds its revision to these bases (#823). + * (`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(), From 5009e8fabd72335fb3ea7358c2477190f72828d5 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Sat, 26 Sep 2026 10:27:04 +0200 Subject: [PATCH 4/4] fix(push): keep the push bases of skills a pull held, and match a replaced root rule at every base (#823) --- docs/designs/data-directory-layout.md | 6 +- .../e2e/push-sync-followups-823.test.ts | 112 ++++++++++++++++++ src/__tests__/pull-namespace-override.test.ts | 9 +- src/pull.ts | 15 ++- src/resources/rules.ts | 25 ++-- 5 files changed, 154 insertions(+), 13 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index a80a798b9..e7f1eb523 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -105,8 +105,10 @@ 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. Like a full pull, an inherited pull already -synced at the team's revision writes nothing (#823). +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/src/__tests__/e2e/push-sync-followups-823.test.ts b/src/__tests__/e2e/push-sync-followups-823.test.ts index d89cdc154..0e806201e 100644 --- a/src/__tests__/e2e/push-sync-followups-823.test.ts +++ b/src/__tests__/e2e/push-sync-followups-823.test.ts @@ -603,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/pull.ts b/src/pull.ts index 1c4533c39..b91b7c867 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1053,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; @@ -1154,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; @@ -1355,6 +1358,15 @@ async function pullForScope( } } 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); if (!docsSyncFailed) { @@ -1389,9 +1401,10 @@ async function pullForScope( ? 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, - [recordKey]: { rev: deliveredRev, targets: syncedTargets }, + [recordKey]: keptBases.length > 0 ? { ...record, pushBaseRevs: keptBases } : record, }; } await saveStateForScope(state, localConfig); diff --git a/src/resources/rules.ts b/src/resources/rules.ts index a41b3e046..6fd56e174 100644 --- a/src/resources/rules.ts +++ b/src/resources/rules.ts @@ -473,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))) { @@ -493,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. @@ -520,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 { @@ -673,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; } /**