Open
Conversation
Review Summary by QodoCheckpoint skills changes before sync rebase
WalkthroughsDescription• Replace stash-based skills sync with local checkpoint commits • Implement rebase strategy instead of destructive hard reset • Surface real git diff failures in committable-change checks • Push existing ahead checkpoint commits even without new staged files Diagramflowchart LR
A["Local Changes"] -->|"checkpoint commit"| B["Local Checkpoint"]
B -->|"fetch origin"| C["Remote Updates"]
C -->|"rebase"| D["Rebased Branch"]
E["No Staged Changes"] -->|"check HEAD vs origin"| F["Push if Ahead"]
D --> G["Preserved History"]
F --> G
File Changes1. src/server/skillsRoutes.ts
|
Code Review by Qodo
1. Commit identity not set
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+913
to
+920
| 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 }) | ||
| } |
There was a problem hiding this comment.
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
Comment on lines
+905
to
909
| try { | ||
| await runCommand('git', ['rebase', `origin/${branch}`], { cwd: localDir }) | ||
| } catch { | ||
| await resolveMergeConflictsByNewerCommit(localDir, branch, localMtimesBeforeSync) | ||
| } |
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests