Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
200 changes: 31 additions & 169 deletions src/server/skillsRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -514,14 +514,12 @@ function groupRpcSkillRecords<T extends RpcSkillRecord>(skills: T[]): T[] {
}

type InstalledSkillInfo = { name: string; path: string; enabled: boolean }
type SyncedSkill = { owner?: string; name: string; enabled: boolean }

type SkillsSyncState = {
githubToken?: string
githubUsername?: string
repoOwner?: string
repoName?: string
installedOwners?: Record<string, string>
lastPullCommitSha?: string
lastPushCommitSha?: string
lastSyncAttemptCount?: number
Expand All @@ -541,7 +539,6 @@ type GithubTokenResponse = { access_token?: string; error?: string }

const GITHUB_DEVICE_CLIENT_ID = 'Iv1.b507a08c87ecfe98'
const DEFAULT_SKILLS_SYNC_REPO_NAME = 'codexskills'
const SKILLS_SYNC_MANIFEST_PATH = 'installed-skills.json'
const SYNC_UPSTREAM_SKILLS_OWNER = 'OpenClawAndroid'
const SYNC_UPSTREAM_SKILLS_REPO = 'skills'
const PRIVATE_SYNC_BRANCH = 'main'
Expand Down Expand Up @@ -806,60 +803,6 @@ async function ensurePrivateForkFromUpstream(token: string, username: string, re
}
}

async function readRemoteSkillsManifest(token: string, repoOwner: string, repoName: string): Promise<SyncedSkill[]> {
const url = `https://api.github.com/repos/${repoOwner}/${repoName}/contents/${SKILLS_SYNC_MANIFEST_PATH}`
const resp = await fetch(url, {
headers: {
Accept: 'application/vnd.github+json',
Authorization: `Bearer ${token}`,
'X-GitHub-Api-Version': '2022-11-28',
'User-Agent': 'codex-web-local',
},
})
if (resp.status === 404) return []
if (!resp.ok) throw new Error(`Failed to read remote manifest (${resp.status})`)
const payload = await resp.json() as { content?: string }
const content = payload.content ? Buffer.from(payload.content.replace(/\n/g, ''), 'base64').toString('utf8') : '[]'
const parsed = JSON.parse(content) as unknown
if (!Array.isArray(parsed)) return []
const skills: SyncedSkill[] = []
for (const row of parsed) {
const item = asRecord(row)
const owner = typeof item?.owner === 'string' ? item.owner : ''
const name = typeof item?.name === 'string' ? item.name : ''
if (!name) continue
skills.push({ ...(owner ? { owner } : {}), name, enabled: item?.enabled !== false })
}
return skills
}

async function writeRemoteSkillsManifest(token: string, repoOwner: string, repoName: string, skills: SyncedSkill[]): Promise<boolean> {
const url = `https://api.github.com/repos/${repoOwner}/${repoName}/contents/${SKILLS_SYNC_MANIFEST_PATH}`
let sha = ''
const nextContent = JSON.stringify(skills, null, 2)
const existing = await fetch(url, {
headers: {
Accept: 'application/vnd.github+json',
Authorization: `Bearer ${token}`,
'X-GitHub-Api-Version': '2022-11-28',
'User-Agent': 'codex-web-local',
},
})
if (existing.ok) {
const payload = await existing.json() as { sha?: string; content?: string }
sha = payload.sha ?? ''
const currentContent = payload.content ? Buffer.from(payload.content.replace(/\n/g, ''), 'base64').toString('utf8') : ''
if (currentContent === nextContent) return false
}
const content = Buffer.from(nextContent, 'utf8').toString('base64')
await getGithubJson(url, token, 'PUT', {
message: 'Update synced skills manifest',
content,
...(sha ? { sha } : {}),
})
return true
}

function toGitHubTokenRemote(repoOwner: string, repoName: string, token: string): string {
return `https://x-access-token:${encodeURIComponent(token)}@github.com/${repoOwner}/${repoName}.git`
}
Expand Down Expand Up @@ -900,27 +843,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
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
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 +957,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 +969,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 +1094,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 All @@ -1198,32 +1123,7 @@ async function bootstrapSkillsFromUpstreamIntoLocal(): Promise<void> {
await ensureSkillsWorkingTreeRepo(repoUrl, branch)
}

async function collectLocalSyncedSkills(appServer: AppServerLike): Promise<SyncedSkill[]> {
const state = await readSkillsSyncState()
const owners = { ...(state.installedOwners ?? {}) }
const skills = (await appServer.rpc('skills/list', {})) as {
data?: Array<{ skills?: Array<{ name?: string; enabled?: boolean; path?: string; scope?: string }> }>
}
const seen = new Set<string>()
const synced: SyncedSkill[] = []
let ownersChanged = false
for (const entry of skills.data ?? []) {
for (const skill of groupRpcSkillRecords(entry.skills ?? [])) {
const name = typeof skill.name === 'string' ? skill.name : ''
if (!name || skill.scope !== 'user' || seen.has(name)) continue
seen.add(name)
const owner = owners[name] ?? ''
synced.push({ ...(owner ? { owner } : {}), name, enabled: skill.enabled !== false })
}
}
if (ownersChanged) {
await writeSkillsSyncState({ ...state, installedOwners: owners })
}
synced.sort((a, b) => `${a.owner ?? ''}/${a.name}`.localeCompare(`${b.owner ?? ''}/${b.name}`))
return synced
}

async function autoPushSyncedSkills(appServer: AppServerLike): Promise<void> {
async function autoPushSyncedSkills(_appServer: AppServerLike): Promise<void> {
const state = await readSkillsSyncState()
if (!state.githubToken || !state.repoOwner || !state.repoName) return
if (isUpstreamSkillsRepo(state.repoOwner, state.repoName)) {
Expand All @@ -1237,9 +1137,7 @@ async function autoPushSyncedSkills(appServer: AppServerLike): Promise<void> {
// After a successful pull, if local tree is already clean and equal to remote,
// skip push entirely to avoid rewriting/deleting remote-only updates.
if (!hasCommittableChanges && head === originHead) return
const local = await collectLocalSyncedSkills(appServer)
const installedMap = await scanInstalledSkillsFromDisk()
await writeRemoteSkillsManifest(state.githubToken, state.repoOwner, state.repoName, local)
await syncInstalledSkillsFolderToRepo(state.githubToken, state.repoOwner, state.repoName, installedMap)
}

Expand Down Expand Up @@ -1489,11 +1387,9 @@ export async function handleSkillsRoutes(
setJson(res, 400, { error: 'Refusing to push to upstream repository' })
return true
}
const local = await collectLocalSyncedSkills(appServer)
const installedMap = await collectInstalledSkillsMap(appServer)
await writeRemoteSkillsManifest(state.githubToken, state.repoOwner, state.repoName, local)
await syncInstalledSkillsFolderToRepo(state.githubToken, state.repoOwner, state.repoName, installedMap)
setJson(res, 200, { ok: true, data: { synced: local.length } })
setJson(res, 200, { ok: true, data: { synced: installedMap.size } })
} catch (error) {
setJson(res, 502, { error: getErrorMessage(error, 'Failed to push synced skills') })
}
Expand All @@ -1519,46 +1415,18 @@ export async function handleSkillsRoutes(
setJson(res, 200, { ok: true, data: { synced: 0, source: 'upstream' } })
return true
}
const remote = await readRemoteSkillsManifest(state.githubToken, state.repoOwner, state.repoName)
const localDir = await detectUserSkillsDir(appServer)
await pullInstalledSkillsFolderFromRepo(state.githubToken, state.repoOwner, state.repoName)
const localSkills = await scanInstalledSkillsFromDisk()
const missingAfterPull: string[] = []
for (const skill of remote) {
const owner = skill.owner || ''
if (!owner) continue
if (!localSkills.has(skill.name)) {
missingAfterPull.push(`${owner}/${skill.name}`)
continue
}
const skillPath = join(localDir, skill.name)
await appServer.rpc('skills/config/write', { path: skillPath, enabled: skill.enabled })
}
if (missingAfterPull.length > 0) {
throw new Error(`Missing skill folders after pull: ${missingAfterPull.join(', ')}`)
}
const remoteNames = new Set(remote.map((row) => row.name))
for (const [name, localInfo] of localSkills.entries()) {
if (!remoteNames.has(name)) {
await rm(localInfo.path.replace(/\/SKILL\.md$/, ''), { recursive: true, force: true })
}
}
const nextOwners: Record<string, string> = {}
for (const item of remote) {
const owner = item.owner || ''
if (owner) nextOwners[item.name] = owner
}
const pulledHead = await runCommandWithOutput('git', ['rev-parse', 'HEAD'], { cwd: getSkillsInstallDir() }).catch(() => '')
await writeSkillsSyncState({
...state,
installedOwners: nextOwners,
lastPullCommitSha: pulledHead.trim(),
lastSyncAttemptCount: 1,
lastSyncError: '',
lastSyncAtIso: new Date().toISOString(),
})
try { await appServer.rpc('skills/list', { forceReload: true }) } catch {}
setJson(res, 200, { ok: true, data: { synced: remote.length } })
setJson(res, 200, { ok: true, data: { synced: localSkills.size } })
} catch (error) {
setJson(res, 502, { error: getErrorMessage(error, 'Failed to pull synced skills') })
}
Expand Down Expand Up @@ -1633,12 +1501,6 @@ export async function handleSkillsRoutes(
return true
}
await rm(target, { recursive: true, force: true })
if (name) {
const syncState = await readSkillsSyncState()
const nextOwners = { ...(syncState.installedOwners ?? {}) }
delete nextOwners[name]
await writeSkillsSyncState({ ...syncState, installedOwners: nextOwners })
}
autoPushSyncedSkills(appServer).catch(() => {})
try { await withTimeout(appServer.rpc('skills/list', { forceReload: true }), 10_000, 'skills/list reload') } catch {}
setJson(res, 200, { ok: true, deletedPath: target })
Expand Down
63 changes: 51 additions & 12 deletions tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,10 @@ The accidental `npx run dev` command starts the repository dev wrapper instead o

---

### Skills sync idempotent commits and nested shared skills handling
### Skills sync generic file sync and nested shared skills handling

#### Feature/Change Name
Skills Sync skips unchanged manifest writes and does not fail parent commits when only nested `shared_skills` content is dirty.
Skills Sync uses the Git repository contents as the sync source of truth and does not fail parent commits when only nested `shared_skills` content is dirty.

#### Prerequisites/Setup
1. Dev server running (`pnpm run dev --host 127.0.0.1 --port 5173`)
Expand All @@ -357,16 +357,22 @@ Skills Sync skips unchanged manifest writes and does not fail parent commits whe

#### Steps
1. In light theme, open `#/skills`.
2. Click `Startup Sync` when no installed skills manifest content has changed.
3. Confirm the sync completes without adding a new `Update synced skills manifest` commit to the GitHub repo.
4. Modify a file inside `/Users/igor/.codex/skills/shared_skills` without committing it inside that nested repository.
5. Click `Push` or `Startup Sync` again.
6. Confirm the sync does not show `Command failed (git commit -m Sync installed skills folder and manifest)` for the parent `/Users/igor/.codex/skills` repository.
7. Confirm the startup auto-push path skips when the only local status is dirty nested `shared_skills` content and local `HEAD` equals `origin/main`.
8. Switch to dark theme and repeat steps 1, 2, and 5.

#### Expected Results
- Unchanged `installed-skills.json` content is not written back to GitHub, so repeated empty-looking manifest commits are not created.
2. Click `Startup Sync` when the skills repo already matches `origin/main`.
3. Confirm the sync completes without adding a new manifest-only commit to the GitHub repo.
4. Create a local skill folder that is not listed in `installed-skills.json`.
5. Click `Push` and confirm the skill folder is committed and pushed as normal repository content.
6. Delete a skill folder remotely and click `Pull`.
7. Confirm the local folder removal follows the Git remote commit, not an `installed-skills.json` membership check.
8. Modify a file inside `/Users/igor/.codex/skills/shared_skills` without committing it inside that nested repository.
9. Click `Push` or `Startup Sync` again.
10. Confirm the sync does not show `Command failed (git commit -m Sync installed skills folder and manifest)` for the parent `/Users/igor/.codex/skills` repository.
11. Confirm the startup auto-push path skips when the only local status is dirty nested `shared_skills` content and local `HEAD` equals `origin/main`.
12. Switch to dark theme and repeat steps 1, 2, and 9.

#### Expected Results
- Skills folders and files sync by normal Git commits, independent of `installed-skills.json`.
- Pull does not delete local skills solely because a manifest entry is missing.
- Push does not write `installed-skills.json` through the GitHub Contents API.
- A dirty nested `shared_skills` repository does not make the parent skills sync fail with `no changes added to commit`.
- Dirty nested `shared_skills` content alone does not keep triggering no-op startup push work.
- Skills Sync status, errors, and action buttons remain readable in light theme and dark theme.
Expand All @@ -376,6 +382,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