Skip to content

Checkpoint skills changes before sync rebase #135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
friuns2 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Checkpoint skills changes before sync rebase #135

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dac450
Checkpoint skills changes before sync rebase
friuns May 6, 2026
716d3cd
Make skills sync generic file sync
friuns May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 28 additions & 46 deletions src/server/skillsRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,27 +900,25 @@ async function ensureSkillsWorkingTreeRepo(repoUrl: string, branch: string): Pro
await runCommand('git', ['checkout', '-B', branch], { cwd: localDir })
}
await resolveMergeConflictsByNewerCommit(localDir, branch, localMtimesBeforeSync)
const hasLocalChangesBeforePull = await hasLocalUncommittedChanges(localDir)
const localMtimesBeforePull = hasLocalChangesBeforePull ? await snapshotFileMtimes(localDir) : new Map<string, number>()
let createdAutostash = false
try {
const stashOutput = await runCommandWithOutput('git', ['stash', 'push', '--include-untracked', '-m', 'codex-skills-autostash'], { cwd: localDir })
createdAutostash = !stashOutput.includes('No local changes to save')
} catch {}
let pulledMtimes = new Map<string, number>()
await checkpointLocalSkillsChanges(localDir)
await runGitFetchWithRefLockRetry(localDir, ['fetch', 'origin', branch])
await runCommand('git', ['reset', '--hard', `origin/${branch}`], { cwd: localDir })
pulledMtimes = await snapshotFileMtimes(localDir)
if (createdAutostash) {
try {
await runCommand('git', ['stash', 'pop'], { cwd: localDir })
} catch {
await resolveStashPopConflictsByFileTime(localDir, localMtimesBeforePull, pulledMtimes)
}
try {
await runCommand('git', ['rebase', `origin/${branch}`], { cwd: localDir })
} catch {
await resolveMergeConflictsByNewerCommit(localDir, branch, localMtimesBeforeSync)
}
Comment on lines +848 to 852

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Conflict resolver drops locals 🐞 Bug ≡ Correctness

localMtimesBeforeSync is only snapshotted when there are uncommitted changes, but the new
rebase-based reconcile can conflict while replaying existing local commits (clean working tree,
commits ahead of origin). When that happens, the resolver treats missing mtimes as 0 and will
consistently prefer the remote side, potentially discarding local committed changes during conflict
auto-resolution.
Agent Prompt 
### Issue description
The conflict resolver’s decision input (`localMtimesBeforeSync`) is only populated when there are uncommitted changes, but rebase conflicts can occur even when the working tree is clean (e.g., local commits ahead of origin). In that case, the resolver defaults local mtime to 0 and can systematically pick the wrong side, discarding local committed changes.

### Issue Context
- `ensureSkillsWorkingTreeRepo()` now uses `git rebase origin/<branch>` for remote reconcile.
- `resolveMergeConflictsByNewerCommit()` uses `localMtimesBeforeSync.get(path) ?? 0`.

### Fix Focus Areas
- src/server/skillsRoutes.ts[891-909]
- src/server/skillsRoutes.ts[934-943]
- src/server/skillsRoutes.ts[1104-1116]

### Suggested fix
- Compute a meaningful “local recency” signal even when there are no uncommitted changes:
  - Option A (simplest): snapshot mtimes unconditionally before running rebase/pull-rebase.
  - Option B (more correct for committed changes): when resolving each conflicted `path`, derive local and remote commit times via `git log -1 --format=%ct <ref> -- <path>` for both `HEAD` (or the relevant local side) and `origin/<branch>`, and compare those instead of using an mtime map.
- Apply the same fix in `pushWithNonFastForwardRetry()` since it can also rebase/pull-rebase and invoke the same resolver.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

return localDir
}

async function checkpointLocalSkillsChanges(repoDir: string): Promise<void> {
await runCommand('git', ['add', '-A'], { cwd: repoDir })
try {
await runCommand('git', ['diff', '--cached', '--quiet', '--exit-code'], { cwd: repoDir })
return
} catch {}
await runCommand('git', ['commit', '-m', 'Local skills checkpoint before sync'], { cwd: repoDir })
}
Comment on lines +856 to +863

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Commit identity not set 🐞 Bug ☼ Reliability

checkpointLocalSkillsChanges() runs git commit (and ensureSkillsWorkingTreeRepo() runs `git
rebase`) without ensuring user.name/user.email are configured for existing repos, which can
hard-fail sync/pull with “Author identity unknown”. syncInstalledSkillsFolderToRepo() only
configures git identity after calling ensureSkillsWorkingTreeRepo(), so the failure can occur before
the config is applied.
Agent Prompt 
### Issue description
`ensureSkillsWorkingTreeRepo()` can now create commits (checkpoint commits) and run `git rebase` before the repo has a configured committer identity for existing repos. This can cause sync/pull to fail with an “Author identity unknown” error.

### Issue Context
`syncInstalledSkillsFolderToRepo()` sets `user.email` / `user.name` only after it calls `ensureSkillsWorkingTreeRepo()`. But `ensureSkillsWorkingTreeRepo()` now calls `checkpointLocalSkillsChanges()` (which commits) and then runs a rebase.

### Fix Focus Areas
- src/server/skillsRoutes.ts[867-910]
- src/server/skillsRoutes.ts[913-920]
- src/server/skillsRoutes.ts[1146-1152]

### Suggested fix
- In `ensureSkillsWorkingTreeRepo()` (both the init and existing-repo paths), run:
  - `git config user.email skills-sync@local`
  - `git config user.name Skills Sync`
  before any operation that can create commits (`checkpointLocalSkillsChanges`, `rebase --continue`, merge commits).
- Alternatively (more robust), invoke commit/rebase commands with per-invocation config, e.g. `git -c user.email=... -c user.name=... commit ...` so behavior does not depend on repo/global config.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


async function resolveMergeConflictsByNewerCommit(
repoDir: string,
branch: string,
Expand Down Expand Up @@ -1016,29 +1014,6 @@ async function getCommitTime(repoDir: string, ref: string, path: string): Promis
}
}

async function resolveStashPopConflictsByFileTime(
repoDir: string,
localMtimesBeforePull: Map<string, number>,
pulledMtimes: Map<string, number>,
): Promise<void> {
const unmerged = (await runCommandWithOutput('git', ['diff', '--name-only', '--diff-filter=U'], { cwd: repoDir }))
.split(/\r?\n/)
.map((row) => row.trim())
.filter(Boolean)
if (unmerged.length === 0) return
for (const path of unmerged) {
const localMtime = localMtimesBeforePull.get(path) ?? 0
const pulledMtime = pulledMtimes.get(path) ?? 0
const side = localMtime >= pulledMtime ? '--theirs' : '--ours'
await checkoutConflictSideWithFallback(repoDir, path, side)
await runCommand('git', ['add', '--', path], { cwd: repoDir })
}
const mergeHead = await readOptionalGitRef(repoDir, 'MERGE_HEAD')
if (mergeHead) {
await runCommand('git', ['commit', '-m', 'Auto-resolve stash-pop conflicts by file time'], { cwd: repoDir })
}
}

async function snapshotFileMtimes(dir: string): Promise<Map<string, number>> {
const mtimes = new Map<string, number>()
await walkFileMtimes(dir, dir, mtimes)
Expand All @@ -1051,12 +1026,10 @@ async function hasLocalUncommittedChanges(repoDir: string): Promise<boolean> {
}

async function hasCommittableWorkingTreeChanges(repoDir: string): Promise<boolean> {
try {
await runCommand('git', ['diff', '--quiet', '--exit-code', '--ignore-submodules=dirty'], { cwd: repoDir })
await runCommand('git', ['diff', '--cached', '--quiet', '--exit-code', '--ignore-submodules=dirty'], { cwd: repoDir })
} catch {
return true
}
const unstaged = (await runCommandWithOutput('git', ['diff', '--name-only', '--ignore-submodules=dirty'], { cwd: repoDir })).trim()
if (unstaged.length > 0) return true
const staged = (await runCommandWithOutput('git', ['diff', '--cached', '--name-only', '--ignore-submodules=dirty'], { cwd: repoDir })).trim()
if (staged.length > 0) return true
const untracked = (await runCommandWithOutput('git', ['ls-files', '--others', '--exclude-standard'], { cwd: repoDir })).trim()
return untracked.length > 0
}
Expand Down Expand Up @@ -1178,10 +1151,19 @@ async function syncInstalledSkillsFolderToRepo(
await runCommand('git', ['config', 'user.name', 'Skills Sync'], { cwd: repoDir })
await restoreProtectedFilesFromOrigin(repoDir, branch)
await runCommand('git', ['add', '.'], { cwd: repoDir })
let hasStagedChanges = true
try {
await runCommand('git', ['diff', '--cached', '--quiet', '--exit-code'], { cwd: repoDir })
return
hasStagedChanges = false
} catch {}
if (!hasStagedChanges) {
const head = (await runCommandWithOutput('git', ['rev-parse', 'HEAD'], { cwd: repoDir })).trim()
const originHead = (await runCommandWithOutput('git', ['rev-parse', `origin/${branch}`], { cwd: repoDir })).trim()
if (head !== originHead) {
await pushWithNonFastForwardRetry(repoDir, branch)
}
return
}
await runCommand('git', ['commit', '-m', 'Sync installed skills folder and manifest'], { cwd: repoDir })
await pushWithNonFastForwardRetry(repoDir, branch)
}
Expand Down
33 changes: 33 additions & 0 deletions tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,39 @@ Skills Sync skips unchanged manifest writes and does not fail parent commits whe

---

### Skills sync checkpoints local changes before remote reconcile

#### Feature/Change Name
Skills Sync commits local skills edits before pulling remote updates and rebases instead of using `reset --hard`.

#### Prerequisites/Setup
1. Dev server running (`pnpm run dev --host 127.0.0.1 --port 5173`)
2. GitHub Skills Sync is configured and connected
3. `/Users/igor/.codex/skills` has a local tracked edit and an untracked local skill folder
4. Remote `main` has at least one newer commit than local
5. Light theme and dark theme are available from the appearance switcher

#### Steps
1. In light theme, open `#/skills`.
2. Create or edit a local skill under `/Users/igor/.codex/skills`.
3. Confirm the local skills repo shows uncommitted changes.
4. Click `Pull` or `Startup Sync`.
5. Inspect `/Users/igor/.codex/skills` Git history.
6. Confirm a `Local skills checkpoint before sync` commit exists before the remote reconcile.
7. Confirm the branch rebased onto `origin/main` and no `reset --hard origin/main` path was used.
8. Switch to dark theme and repeat steps 1 through 4.

#### Expected Results
- Local tracked and untracked skill edits become normal Git commits before remote reconciliation.
- Sync history shows the local checkpoint commit and remote commits instead of hiding local files in `refs/stash`.
- Remote updates are applied by rebase/merge-conflict resolution rather than by destructive hard reset.
- Skills Sync status and errors remain readable in light theme and dark theme.

#### Rollback/Cleanup
- Revert or reset any test-only skills repo commits after validation if they should not be kept.

---

### Header Git branch dropdown with commit reset

#### Feature/Change Name
Expand Down