From ddb48189642070034bb6b1485e06d2981d4a5b69 Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Thu, 24 Sep 2026 05:34:28 -0700 Subject: [PATCH 1/3] fix(push): address follow-up review findings --- src/__tests__/push-role-skill-e2e.test.ts | 82 +++++++++++++++++++++++ src/__tests__/push-role.test.ts | 5 +- src/push.ts | 71 +++++++++++++++----- 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/src/__tests__/push-role-skill-e2e.test.ts b/src/__tests__/push-role-skill-e2e.test.ts index 717e4f454..b0416b5ec 100644 --- a/src/__tests__/push-role-skill-e2e.test.ts +++ b/src/__tests__/push-role-skill-e2e.test.ts @@ -467,6 +467,62 @@ describe('push branch and dirty-clone e2e (issue #663)', () => { } }, 60_000); + it('routes config changes to the explicit new branch when an existing PR is also updated', async () => { + const fixture = makePushFixture('git', 'https://git.example.test/team/issue-663-mixed.git', 'claude'); + try { + const teamRepo = path.join(fixture.projectRoot, '.teamai', 'team-repo'); + git(['config', 'core.autocrlf', 'false'], teamRepo); + git(['checkout', '--', '.'], teamRepo); + const first = await pullModifyAndPush(fixture, 'claude'); + expect(first.output).toContain('Pushed branch teamai/push/issue-331-git/'); + const existingBranch = git( + ['for-each-ref', '--format=%(refname:short)', 'refs/heads/teamai/push/issue-331-git/'], + fixture.remote, + ); + expect(existingBranch).toMatch(/^teamai\/push\/issue-331-git\//); + + const statePath = path.join(fixture.projectRoot, '.teamai', 'state.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { + pendingPushes: Array<{ branch: string; prUrl: string | null }>; + }; + const pending = state.pendingPushes.find((entry) => entry.branch === existingBranch); + expect(pending).toBeDefined(); + if (!pending) return; + pending.prUrl = 'https://github.com/team/issue-663-mixed/pull/663'; + fs.writeFileSync(statePath, JSON.stringify(state, null, 2) + '\n'); + + fs.writeFileSync( + path.join(fixture.projectRoot, '.claude', 'skills', 'beta-proof', 'SKILL.md'), + '---\nname: beta-proof\ndescription: modified again\n---\n\n# Modified again\n', + ); + const newSkillDir = path.join(fixture.projectRoot, '.claude', 'skills', 'gamma-proof'); + fs.mkdirSync(newSkillDir, { recursive: true }); + fs.writeFileSync( + path.join(newSkillDir, 'SKILL.md'), + '---\nname: gamma-proof\ndescription: new\n---\n\n# New skill\n', + ); + fs.appendFileSync(path.join(teamRepo, 'teamai.yaml'), '\npublicSkills: []\n'); + + const second = await runCLI( + ['push', '--all', '--branch', 'feature/explicit-mixed'], + fixture.projectRoot, + fixture.home, + ); + expect(second.code, second.output).not.toBe(0); + expect(second.output) + .toContain('Existing PR updated: https://github.com/team/issue-663-mixed/pull/663'); + expect(second.output).toContain('Pushed branch feature/explicit-mixed'); + expect(git(['show', `${existingBranch}:teamai.yaml`], fixture.remote)) + .not.toContain('publicSkills: []'); + expect(git(['show', 'feature/explicit-mixed:teamai.yaml'], fixture.remote)) + .toContain('publicSkills: []'); + expect(git(['show', 'feature/explicit-mixed:skills/backend/gamma-proof/SKILL.md'], fixture.remote)) + .toContain('# New skill'); + } finally { + fs.rmSync(fixture.sandbox, { recursive: true, force: true }); + } + }, 60_000); + it('refuses a mode-only teamai.yaml change before reset --hard', async () => { const fixture = makePushFixture('git', 'https://git.example.test/team/issue-663-dirty.git', 'claude'); try { @@ -491,4 +547,30 @@ describe('push branch and dirty-clone e2e (issue #663)', () => { fs.rmSync(fixture.sandbox, { recursive: true, force: true }); } }, 60_000); + + it('refuses a content-and-mode teamai.yaml change before reset --hard', async () => { + const fixture = makePushFixture('git', 'https://git.example.test/team/issue-663-dirty-content.git', 'claude'); + try { + const teamRepo = path.join(fixture.projectRoot, '.teamai', 'team-repo'); + git(['config', 'core.autocrlf', 'false'], teamRepo); + git(['checkout', '--', 'teamai.yaml'], teamRepo); + const pulled = await runCLI(['pull'], fixture.projectRoot, fixture.home); + expect(pulled.code, pulled.output).toBe(0); + fs.appendFileSync(path.join(teamRepo, 'teamai.yaml'), '\npublicSkills: []\n'); + git(['config', 'core.fileMode', 'true'], teamRepo); + git(['update-index', '--chmod=+x', 'teamai.yaml'], teamRepo); + expect(git(['status', '--short'], teamRepo)).toContain('teamai.yaml'); + + const result = await runCLI(['push', '--all'], fixture.projectRoot, fixture.home); + expect(result.code, result.output).not.toBe(0); + expect(result.output).toContain('Cannot push: the team repo has uncommitted changes'); + expect(result.output).toContain('teamai.yaml'); + expect(git( + ['for-each-ref', '--format=%(refname:short)', 'refs/heads/teamai/push/'], + fixture.remote, + )).toBe(''); + } finally { + fs.rmSync(fixture.sandbox, { recursive: true, force: true }); + } + }, 60_000); }); diff --git a/src/__tests__/push-role.test.ts b/src/__tests__/push-role.test.ts index 78a4ee001..c384c7981 100644 --- a/src/__tests__/push-role.test.ts +++ b/src/__tests__/push-role.test.ts @@ -27,8 +27,10 @@ describe('team repo dirty-path guard', () => { renamed: [], }; - it('allows teamai.yaml only when its content was captured', () => { + it('allows teamai.yaml only when its content was captured and its mode is unchanged', () => { expect(collectUnsafeDirtyPaths({ ...cleanStatus, modified: ['teamai.yaml'] }, 'edited')).toEqual([]); + expect(collectUnsafeDirtyPaths({ ...cleanStatus, modified: ['teamai.yaml'] }, 'edited', new Set(['teamai.yaml']))) + .toEqual(['teamai.yaml']); expect(collectUnsafeDirtyPaths({ ...cleanStatus, modified: ['teamai.yaml'] }, null)).toEqual(['teamai.yaml']); }); @@ -81,6 +83,7 @@ const mockGitStatus = vi.fn().mockResolvedValue({ }); const mockCreateGit = vi.fn().mockReturnValue({ status: mockGitStatus, + raw: vi.fn().mockResolvedValue(''), merge: mockMerge, stash: mockStash, }); diff --git a/src/push.ts b/src/push.ts index 24743ca5b..7ac0ecc19 100644 --- a/src/push.ts +++ b/src/push.ts @@ -317,22 +317,50 @@ function collectDirtyPaths(status: PushRepoStatus): string[] { return [...paths].sort(); } -function isTeamaiOwnedDirtyPath(filePath: string, pendingTeamConfig: string | null): boolean { +async function hasGitModeChange( + git: { raw?: (args: string[]) => Promise }, + filePath: string, +): Promise { + // The content snapshot is enough only when the file mode is unchanged. Check + // both the index and worktree diffs because a chmod can be staged, unstaged, + // or both alongside a content edit (#690 review). + if (typeof git.raw !== 'function') return false; + try { + const [worktreeDiff, indexDiff] = await Promise.all([ + git.raw(['diff', '--summary', '--', filePath]), + git.raw(['diff', '--cached', '--summary', '--', filePath]), + ]); + return [worktreeDiff, indexDiff].some((diff) => /mode change \d+ => \d+/.test(diff)); + } catch { + // If Git cannot prove that metadata is unchanged, stop before reset rather + // than risk discarding a mode change that was not captured. + return true; + } +} + +function isTeamaiOwnedDirtyPath( + filePath: string, + pendingTeamConfig: string | null, + modeChangedPaths: ReadonlySet, +): boolean { const normalized = filePath.replaceAll('\\', '/'); // The sync lock is disposable TeamAI state. teamai.yaml is different: it is - // safe to restore only when its working-tree content was captured above. - // Deletion and mode-only changes leave pendingTeamConfig null and must stop - // before reset --hard, or the user's change is silently lost (#690 review). + // safe to restore only when its content was captured above and its mode is + // unchanged. Deletion, mode-only, and content+mode changes must stop before + // reset --hard, or the user's change is silently lost (#690 review). if (normalized === '.teamai/.sync-lock') return true; - return normalized === 'teamai.yaml' && pendingTeamConfig !== null; + return normalized === 'teamai.yaml' + && pendingTeamConfig !== null + && !modeChangedPaths.has(normalized); } export function collectUnsafeDirtyPaths( status: PushRepoStatus, pendingTeamConfig: string | null, + modeChangedPaths: ReadonlySet = new Set(), ): string[] { return collectDirtyPaths(status) - .filter((filePath) => !isTeamaiOwnedDirtyPath(filePath, pendingTeamConfig)); + .filter((filePath) => !isTeamaiOwnedDirtyPath(filePath, pendingTeamConfig, modeChangedPaths)); } /** @@ -859,7 +887,15 @@ async function pushCore( pendingTeamConfig = workingContent; } } - const unsafeDirtyPaths = collectUnsafeDirtyPaths(await git.status(), pendingTeamConfig); + const modeChangedPaths = new Set(); + if (pendingTeamConfig !== null && await hasGitModeChange(git, 'teamai.yaml')) { + modeChangedPaths.add('teamai.yaml'); + } + const unsafeDirtyPaths = collectUnsafeDirtyPaths( + await git.status(), + pendingTeamConfig, + modeChangedPaths, + ); if (unsafeDirtyPaths.length > 0) { pullSpin.fail( 'Cannot push: the team repo has uncommitted changes. Commit or stash them first. ' @@ -1475,12 +1511,6 @@ async function pushCore( // can happen in one run, so editing a resource under review updates its PR // without dragging unrelated resources into that review. const groups = planPushGroups(selectedItems, reusablePending); - const newGroupCount = groups.filter((group) => !group.reuse).length; - if (options.branch && newGroupCount > 1) { - log.error('`--branch` can only target one new push branch at a time; select one resource group or omit it.'); - process.exitCode = 2; - return; - } reuseRecordedDestinations(groups); // The conflicting entries dropped above are deliberately not reused, so they // are not "partly selected" either — warning about them would contradict the @@ -1498,19 +1528,25 @@ async function pushCore( })) return; // ── Step 5: Push each group — one branch/PR per group ────────────── - // Config edits ride along with the first group so they land in a single PR. - let configRider = pendingTeamConfig !== null; + // Config edits ride along with the first group normally. With --branch, an + // existing-PR group must not receive the config because the explicit branch + // is intended for the new group; route the config to that new group instead. + const configGroupIndex = pendingTeamConfig === null + ? -1 + : options.branch + ? Math.max(groups.findIndex((group) => !group.reuse), 0) + : 0; // Track the outcome across groups: a run counts as completed only if at least // one group actually pushed AND no group's PR creation failed (#702 follow-up). let anyPushed = false; let anyPrFailed = false; - for (const group of groups) { + for (const [groupIndex, group] of groups.entries()) { const outcome = await pushGroup({ group, teamConfig, localConfig, pushState, - includeTeamConfig: configRider, + includeTeamConfig: groupIndex === configGroupIndex, branch: options.branch, }); if (outcome === 'failed') { @@ -1526,7 +1562,6 @@ async function pushCore( // (`reconcilePlacementRecords`). if (outcome === 'pushed') anyPushed = true; if (outcome === 'pr-failed') anyPrFailed = true; - configRider = false; } // Update state (pushState already carries the pendingPushes records above) From afb4893f3d7ad2d361a4c48f717f18ccbc559be2 Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Thu, 24 Sep 2026 09:26:14 -0700 Subject: [PATCH 2/3] fix(push): preserve config across reuse cleanup --- src/__tests__/push-role-skill-e2e.test.ts | 74 +++++++++++++++++++++++ src/push.ts | 7 +++ 2 files changed, 81 insertions(+) diff --git a/src/__tests__/push-role-skill-e2e.test.ts b/src/__tests__/push-role-skill-e2e.test.ts index b0416b5ec..5e4c0f4a1 100644 --- a/src/__tests__/push-role-skill-e2e.test.ts +++ b/src/__tests__/push-role-skill-e2e.test.ts @@ -523,6 +523,80 @@ describe('push branch and dirty-clone e2e (issue #663)', () => { } }, 60_000); + it('preserves config after a metadata-only existing-PR group before the explicit new branch', async () => { + const fixture = makePushFixture('git', 'https://git.example.test/team/issue-800-metadata.git', 'claude'); + try { + const seed = path.join(fixture.sandbox, 'seed'); + fs.mkdirSync(path.join(seed, 'rules', 'backend'), { recursive: true }); + fs.writeFileSync( + path.join(seed, 'rules', 'backend', 'beta-rule.md'), + '---\ntitle: beta-rule\n---\n\n# Original rule\n', + ); + const seedConfigPath = path.join(seed, 'teamai.yaml'); + fs.writeFileSync( + seedConfigPath, + fs.readFileSync(seedConfigPath, 'utf8').replace( + ' skills: .claude/skills', + ' skills: .claude/skills\n rules: .claude/rules', + ), + ); + git(['add', 'teamai.yaml', 'rules/backend/beta-rule.md'], seed); + git(['commit', '-q', '-m', 'add rule fixture'], seed); + git(['push', '-q', fixture.remote, 'main'], seed); + + const pulled = await runCLI(['pull'], fixture.projectRoot, fixture.home); + expect(pulled.code, pulled.output).toBe(0); + const rulePath = path.join(fixture.projectRoot, '.claude', 'rules', 'backend', 'beta-rule.md'); + expect(fs.existsSync(rulePath)).toBe(true); + fs.writeFileSync(rulePath, '---\ntitle: beta-rule\n---\n\n# Modified rule\n'); + + const first = await runCLI(['push', '--all'], fixture.projectRoot, fixture.home); + expect(first.code, first.output).not.toBe(0); + expect(first.output).toContain('Pushed branch teamai/push/issue-331-git/'); + const existingBranch = git( + ['for-each-ref', '--format=%(refname:short)', 'refs/heads/teamai/push/issue-331-git/'], + fixture.remote, + ); + expect(existingBranch).toMatch(/^teamai\/push\/issue-331-git\//); + + const statePath = path.join(fixture.projectRoot, '.teamai', 'state.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { + pendingPushes: Array<{ branch: string; prUrl: string | null }>; + }; + const pending = state.pendingPushes.find((entry) => entry.branch === existingBranch); + expect(pending).toBeDefined(); + if (!pending) return; + pending.prUrl = 'https://github.com/team/issue-800-metadata/pull/800'; + fs.writeFileSync(statePath, JSON.stringify(state, null, 2) + '\n'); + + fs.writeFileSync( + rulePath, + '---\ntitle: beta-rule\nlastUpdated: 2026-09-24T00:00:00.000Z\n---\n\n# Original rule\n', + ); + const newSkillDir = path.join(fixture.projectRoot, '.claude', 'skills', 'gamma-proof'); + fs.mkdirSync(newSkillDir, { recursive: true }); + fs.writeFileSync( + path.join(newSkillDir, 'SKILL.md'), + '---\nname: gamma-proof\ndescription: new\n---\n\n# New skill\n', + ); + fs.appendFileSync(path.join(fixture.projectRoot, '.teamai', 'team-repo', 'teamai.yaml'), '\npublicSkills: []\n'); + + const second = await runCLI( + ['push', '--all', '--branch', 'feature/explicit-metadata'], + fixture.projectRoot, + fixture.home, + ); + expect(second.code, second.output).not.toBe(0); + expect(second.output).toContain('Pushed branch feature/explicit-metadata'); + expect(git(['show', 'feature/explicit-metadata:teamai.yaml'], fixture.remote)) + .toContain('publicSkills: []'); + expect(git(['show', `${existingBranch}:teamai.yaml`], fixture.remote)) + .not.toContain('publicSkills: []'); + } finally { + fs.rmSync(fixture.sandbox, { recursive: true, force: true }); + } + }, 60_000); + it('refuses a mode-only teamai.yaml change before reset --hard', async () => { const fixture = makePushFixture('git', 'https://git.example.test/team/issue-663-dirty.git', 'claude'); try { diff --git a/src/push.ts b/src/push.ts index 7ac0ecc19..32c664c6b 100644 --- a/src/push.ts +++ b/src/push.ts @@ -1549,6 +1549,13 @@ async function pushCore( includeTeamConfig: groupIndex === configGroupIndex, branch: options.branch, }); + // A preceding reuse group may take the metadata-only path in + // pushRepoBranch(), which resets and cleans the clone. Re-apply the + // captured config before the new explicit-branch group runs, or that + // cleanup would silently discard the user's edit (#800). + if (pendingTeamConfig !== null && groupIndex < configGroupIndex) { + await writeFile(path.join(localConfig.repo.localPath, 'teamai.yaml'), pendingTeamConfig); + } if (outcome === 'failed') { // The branch/PR for earlier groups is already on the remote, so their // records must survive this failure or the next run would duplicate them. From b3162b2ff53c6377d2adf6068cf84c0d2ddfcc76 Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:40:44 -0700 Subject: [PATCH 3/3] fix(push): handle config-only explicit branch with reused PR --- src/__tests__/push-role-skill-e2e.test.ts | 46 +++++++++++++++++++++++ src/push.ts | 23 +++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/__tests__/push-role-skill-e2e.test.ts b/src/__tests__/push-role-skill-e2e.test.ts index 5e4c0f4a1..5cd8fb667 100644 --- a/src/__tests__/push-role-skill-e2e.test.ts +++ b/src/__tests__/push-role-skill-e2e.test.ts @@ -597,6 +597,52 @@ describe('push branch and dirty-clone e2e (issue #663)', () => { } }, 60_000); + it('keeps config off an existing PR when --branch has no new resource group', async () => { + const fixture = makePushFixture('git', 'https://git.example.test/team/issue-800-config-only.git', 'claude'); + try { + const first = await pullModifyAndPush(fixture, 'claude'); + expect(first.output).toContain('Pushed branch teamai/push/issue-331-git/'); + const existingBranch = git( + ['for-each-ref', '--format=%(refname:short)', 'refs/heads/teamai/push/issue-331-git/'], + fixture.remote, + ); + expect(existingBranch).toMatch(/^teamai\/push\/issue-331-git\//); + + const statePath = path.join(fixture.projectRoot, '.teamai', 'state.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { + pendingPushes: Array<{ branch: string; prUrl: string | null }>; + }; + const pending = state.pendingPushes.find((entry) => entry.branch === existingBranch); + expect(pending).toBeDefined(); + if (!pending) return; + pending.prUrl = 'https://github.com/team/issue-800-config-only/pull/800'; + fs.writeFileSync(statePath, JSON.stringify(state, null, 2) + '\n'); + + fs.writeFileSync( + path.join(fixture.projectRoot, '.claude', 'skills', 'beta-proof', 'SKILL.md'), + '---\nname: beta-proof\ndescription: modified again\n---\n\n# Modified again\n', + ); + const teamRepo = path.join(fixture.projectRoot, '.teamai', 'team-repo'); + fs.appendFileSync(path.join(teamRepo, 'teamai.yaml'), '\npublicSkills: []\n'); + + const second = await runCLI( + ['push', '--all', '--branch', 'feature/explicit-config-only'], + fixture.projectRoot, + fixture.home, + ); + expect(second.code, second.output).not.toBe(0); + expect(second.output) + .toContain('Existing PR updated: https://github.com/team/issue-800-config-only/pull/800'); + expect(second.output).toContain('Pushed branch feature/explicit-config-only'); + expect(git(['show', `${existingBranch}:teamai.yaml`], fixture.remote)) + .not.toContain('publicSkills: []'); + expect(git(['show', 'feature/explicit-config-only:teamai.yaml'], fixture.remote)) + .toContain('publicSkills: []'); + } finally { + fs.rmSync(fixture.sandbox, { recursive: true, force: true }); + } + }, 60_000); + it('refuses a mode-only teamai.yaml change before reset --hard', async () => { const fixture = makePushFixture('git', 'https://git.example.test/team/issue-663-dirty.git', 'claude'); try { diff --git a/src/push.ts b/src/push.ts index 32c664c6b..f81c6d3d8 100644 --- a/src/push.ts +++ b/src/push.ts @@ -1530,11 +1530,15 @@ async function pushCore( // ── Step 5: Push each group — one branch/PR per group ────────────── // Config edits ride along with the first group normally. With --branch, an // existing-PR group must not receive the config because the explicit branch - // is intended for the new group; route the config to that new group instead. + // is intended for the new group. If there is no new group, use groups.length + // as a sentinel and push the config separately after all reuse groups finish. + const newGroupIndex = options.branch + ? groups.findIndex((group) => !group.reuse) + : -1; const configGroupIndex = pendingTeamConfig === null ? -1 : options.branch - ? Math.max(groups.findIndex((group) => !group.reuse), 0) + ? (newGroupIndex >= 0 ? newGroupIndex : groups.length) : 0; // Track the outcome across groups: a run counts as completed only if at least // one group actually pushed AND no group's PR creation failed (#702 follow-up). @@ -1590,6 +1594,21 @@ async function pushCore( } } await saveStateForScope(state, localConfig); + + // When every selected resource reuses an existing PR, --branch still names a + // real destination for the pending config edit. The reuse groups have already + // been saved above, so now push teamai.yaml alone on that explicit branch. + // Do not report completion if an earlier reuse PR creation failed. + if (pendingTeamConfig !== null && options.branch && newGroupIndex < 0) { + await pushTeamConfigOnly( + localConfig, + teamConfig, + options, + anyPrFailed ? undefined : result, + ); + return; + } + // A real push completed only when a group actually pushed and no PR creation // failed. Not set on dry-run/cancel (return earlier), a no-change run (every // group 'nochange' → anyPushed stays false), or a PR-creation failure