Skip to content

Commit 4b17fa0

Browse files
committed
fix: push unpushed commits when working tree is clean but local is ahead of remote
- Check rev-list ahead count instead of relying solely on isClean() - Handle first push when remote branch doesn't exist yet - Add isCancel check for shouldCreate confirm prompt - Use --set-upstream on push for initial branch setup - Show distinct messages for committed+pushed vs pushed-only vs no-op
1 parent b86a088 commit 4b17fa0

4 files changed

Lines changed: 103 additions & 19 deletions

File tree

src/commands/push.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,4 +139,38 @@ describe('push command logic', () => {
139139

140140
expect(mockPrompts.note).toHaveBeenCalledOnce();
141141
});
142+
143+
it('throws CliError when user cancels shouldCreate confirm prompt', async () => {
144+
const { CliError } = await import('../errors.js');
145+
vi.mocked(getGitHubToken).mockReturnValue('ghp_test_token');
146+
vi.mocked(ensureGitRepo).mockResolvedValue({ initialized: true });
147+
vi.mocked(getRepoRemoteUrl).mockResolvedValue(null);
148+
mockPrompts.text.mockResolvedValue('owner/my-skills');
149+
vi.mocked(ensureRemote).mockResolvedValue({
150+
remoteUrl: 'https://github.com/owner/my-skills.git',
151+
added: true,
152+
});
153+
// Simulate user pressing Ctrl+C on confirm prompt
154+
mockPrompts.confirm.mockResolvedValue(Symbol('cancel'));
155+
mockPrompts.isCancel.mockImplementation((val) => typeof val === 'symbol');
156+
157+
const { pushCommand } = await import('./push.js');
158+
await expect(pushCommand({})).rejects.toThrow(CliError);
159+
expect(pushSkills).not.toHaveBeenCalled();
160+
});
161+
162+
it('shows correct message when committed and pushed', async () => {
163+
vi.mocked(getGitHubToken).mockReturnValue('ghp_test_token');
164+
vi.mocked(ensureGitRepo).mockResolvedValue({ initialized: false });
165+
vi.mocked(getRepoRemoteUrl).mockResolvedValue('https://github.com/user/repo.git');
166+
vi.mocked(pushSkills).mockResolvedValue({ committed: false, pushed: true });
167+
168+
const spinnerStop = vi.fn();
169+
mockPrompts.spinner = () => ({ start: vi.fn(), stop: spinnerStop, error: vi.fn() });
170+
171+
const { pushCommand } = await import('./push.js');
172+
await pushCommand({});
173+
174+
expect(spinnerStop).toHaveBeenCalledWith('Unpushed commits pushed successfully!');
175+
});
142176
});

src/commands/push.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ export async function pushCommand(options: { message?: string }): Promise<void>
4040
const shouldCreate = await p.confirm({
4141
message: 'Create this repo on GitHub? (requires gh CLI)',
4242
});
43+
if (p.isCancel(shouldCreate)) {
44+
p.cancel('Push cancelled.');
45+
throw new CliError('Push cancelled by user.');
46+
}
4347

4448
if (shouldCreate === true) {
4549
createGitHubRepo(repo);
@@ -52,11 +56,12 @@ export async function pushCommand(options: { message?: string }): Promise<void>
5256

5357
try {
5458
const result = await pushSkills(AGENTS_DIR, options.message, token);
55-
spinner.stop(
56-
result.committed
59+
const statusMsg = result.pushed
60+
? result.committed
5761
? 'Skills pushed successfully!'
58-
: 'No changes to push — already up to date.',
59-
);
62+
: 'Unpushed commits pushed successfully!'
63+
: 'No changes to push — already up to date.';
64+
spinner.stop(statusMsg);
6065

6166
// Warn about suspicious files that may contain secrets
6267
if (result.suspiciousFiles?.length) {

src/git-ops.test.ts

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,13 +202,15 @@ describe('pushSkills', () => {
202202

203203
expect(mockGit.add).toHaveBeenCalledWith('-A');
204204
expect(mockGit.commit).toHaveBeenCalledWith('test commit');
205-
expect(mockGit.push).toHaveBeenCalledWith('origin', 'main');
205+
expect(mockGit.push).toHaveBeenCalledWith('origin', 'main', { '--set-upstream': null });
206206
expect(result).toEqual({ committed: true, pushed: true });
207207
});
208208

209-
it('skips commit when working tree is clean', async () => {
209+
it('skips commit and push when working tree is clean and not ahead of remote', async () => {
210210
mockGit.add.mockResolvedValue(undefined);
211211
mockGit.status.mockResolvedValue({ isClean: () => true });
212+
mockGit.branchLocal.mockResolvedValue({ current: 'main' });
213+
mockGit.raw.mockResolvedValue('0\n');
212214

213215
const result = await pushSkills(tempDir);
214216

@@ -218,6 +220,37 @@ describe('pushSkills', () => {
218220
expect(result.pushed).toBe(false);
219221
});
220222

223+
it('pushes unpushed commits when working tree is clean but local is ahead of remote', async () => {
224+
mockGit.add.mockResolvedValue(undefined);
225+
mockGit.status.mockResolvedValue({ isClean: () => true });
226+
mockGit.branchLocal.mockResolvedValue({ current: 'main' });
227+
mockGit.raw.mockResolvedValue('2\n');
228+
mockGit.push.mockResolvedValue(undefined);
229+
230+
const result = await pushSkills(tempDir);
231+
232+
expect(mockGit.commit).not.toHaveBeenCalled();
233+
expect(mockGit.push).toHaveBeenCalledWith('origin', 'main', { '--set-upstream': null });
234+
expect(result.committed).toBe(false);
235+
expect(result.pushed).toBe(true);
236+
});
237+
238+
it('commits and pushes on first push when remote branch does not exist yet', async () => {
239+
mockGit.status.mockResolvedValue({ isClean: () => false });
240+
mockGit.add.mockResolvedValue(undefined);
241+
mockGit.commit.mockResolvedValue(undefined);
242+
mockGit.branchLocal.mockResolvedValue({ current: 'main' });
243+
// rev-list fails because origin/main doesn't exist yet
244+
mockGit.raw.mockRejectedValue(new Error('unknown revision origin/main'));
245+
mockGit.push.mockResolvedValue(undefined);
246+
247+
const result = await pushSkills(tempDir, 'initial');
248+
249+
expect(mockGit.commit).toHaveBeenCalledWith('initial');
250+
expect(mockGit.push).toHaveBeenCalledWith('origin', 'main', { '--set-upstream': null });
251+
expect(result).toEqual({ committed: true, pushed: true });
252+
});
253+
221254
it('uses token with temporary remote URL and restores clean URL', async () => {
222255
mockGit.status.mockResolvedValue({ isClean: () => false });
223256
mockGit.add.mockResolvedValue(undefined);
@@ -260,7 +293,7 @@ describe('pushSkills', () => {
260293
await pushSkills(tempDir, 'test', null);
261294

262295
expect(mockGit.remote).not.toHaveBeenCalled();
263-
expect(mockGit.push).toHaveBeenCalledWith('origin', 'master');
296+
expect(mockGit.push).toHaveBeenCalledWith('origin', 'master', { '--set-upstream': null });
264297
});
265298

266299
it('uses auto-generated commit message when none provided', async () => {
@@ -284,7 +317,7 @@ describe('pushSkills', () => {
284317
mockGit.push.mockRejectedValue(new Error('error: failed to push some refs... non-fast-forward'));
285318

286319
await expect(pushSkills(tempDir, 'test')).rejects.toThrow('Push rejected');
287-
await expect(pushSkills(tempDir, 'test')).rejects.toThrow('skills-manager pull');
320+
await expect(pushSkills(tempDir, 'test')).rejects.toThrow('sm pull');
288321
});
289322

290323
it('throws user-friendly error when push is rejected due to fetch first (with token)', async () => {

src/git-ops.ts

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -132,32 +132,44 @@ export async function pushSkills(
132132
await git.add('-A');
133133
// Check if anything to commit
134134
const status = await git.status();
135-
if (status.isClean()) {
136-
return { committed: false, pushed: false, suspiciousFiles: suspicious.length > 0 ? suspicious : undefined };
137-
}
135+
let committed = false;
138136

139-
// Commit
140-
await git.commit(msg);
137+
if (!status.isClean()) {
138+
// Commit
139+
await git.commit(msg);
140+
committed = true;
141+
}
141142

142-
// Push — if token provided, temporarily set the remote URL with auth, then restore
143+
// Push — always check if local is ahead of remote (handles previously committed but unpushed work)
143144
const branch = await getCurrentBranch(git);
145+
let aheadCount: number;
146+
try {
147+
const count = await git.raw(['rev-list', `origin/${branch}..HEAD`, '--count']);
148+
aheadCount = parseInt(count.trim(), 10) || 0;
149+
} catch {
150+
// Remote branch may not exist yet (first push) — treat as ahead if we committed
151+
aheadCount = committed ? 1 : 0;
152+
}
144153

154+
if (aheadCount === 0) {
155+
return { committed: false, pushed: false, suspiciousFiles: suspicious.length > 0 ? suspicious : undefined };
156+
}
145157
const doPush = async () => {
146158
try {
147-
await git.push('origin', branch);
159+
await git.push('origin', branch, { '--set-upstream': null });
148160
} catch (err) {
149161
const msg = String(err);
150162
if (msg.includes('non-fast-forward') || msg.includes('fetch first') || msg.includes('rejected') || msg.includes('failed to push')) {
151163
throw new Error(
152164
'Push rejected — remote contains commits that you do not have locally.\n'
153165
+ 'Pull the latest changes first, resolve any conflicts, then push again:\n'
154-
+ ' skills-manager pull\n'
155-
+ ' skills-manager push\n'
166+
+ ' sm pull\n'
167+
+ ' sm push\n'
156168
+ '\n'
157169
+ 'Or resolve manually:\n'
158170
+ ' cd ~/.agents\n'
159171
+ ` git pull --rebase origin ${branch} # resolve conflicts if any, then git rebase --continue\n`
160-
+ ' skills-manager push',
172+
+ ' sm push',
161173
{ cause: err },
162174
);
163175
}
@@ -184,7 +196,7 @@ export async function pushSkills(
184196
await doPush();
185197
}
186198

187-
return { committed: true, pushed: true, suspiciousFiles: suspicious.length > 0 ? suspicious : undefined };
199+
return { committed, pushed: true, suspiciousFiles: suspicious.length > 0 ? suspicious : undefined };
188200
}
189201

190202
/**

0 commit comments

Comments
 (0)