Skip to content

Make skills sync use generic file sync #136

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

Make skills sync use generic file sync #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ca9f92
xx
friuns May 6, 2026
023322d
Add generic GitHub skills 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
153 changes: 10 additions & 143 deletions src/server/skillsRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,23 +210,6 @@ function withTimeout<T>(promise: Promise<T>, ms: number, label: string): Promise
})
}

async function detectUserSkillsDir(appServer: AppServerLike): Promise<string> {
try {
const result = (await appServer.rpc('skills/list', {})) as {
data?: Array<{ skills?: Array<{ scope?: string; path?: string }> }>
}
for (const entry of result.data ?? []) {
for (const skill of entry.skills ?? []) {
if (skill.scope !== 'user' || !skill.path) continue
const skillInfo = deriveSkillPathInfo(skill.path)
if (!skillInfo) continue
return skillInfo.installDir
}
}
} catch {}
return getSkillsInstallDir()
}

async function ensureInstalledSkillIsValid(appServer: AppServerLike, skillPath: string): Promise<void> {
const result = (await appServer.rpc('skills/list', { forceReload: true })) as {
data?: Array<{ errors?: Array<{ path?: string; message?: string }> }>
Expand Down Expand Up @@ -514,14 +497,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 +522,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 +786,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 @@ -1182,7 +1108,7 @@ async function syncInstalledSkillsFolderToRepo(
await runCommand('git', ['diff', '--cached', '--quiet', '--exit-code'], { cwd: repoDir })
return
} catch {}
await runCommand('git', ['commit', '-m', 'Sync installed skills folder and manifest'], { cwd: repoDir })
await runCommand('git', ['commit', '-m', 'Sync skills files'], { cwd: repoDir })
await pushWithNonFastForwardRetry(repoDir, branch)
}

Expand All @@ -1198,32 +1124,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 +1138,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 @@ -1282,7 +1181,10 @@ async function ensureCodexAgentsSymlinkToSkillsAgents(): Promise<void> {
await symlink(relativeTarget, codexAgentsPath)
}

async function runSkillsSyncStartup(appServer: AppServerLike): Promise<void> {
async function runSkillsSyncStartup(
appServer: AppServerLike,
options: { propagateErrors?: boolean } = {},
): Promise<void> {
if (startupSyncStatus.inProgress) return
startupSyncStatus.inProgress = true
startupSyncStatus.lastRunAtIso = new Date().toISOString()
Expand Down Expand Up @@ -1324,6 +1226,7 @@ async function runSkillsSyncStartup(appServer: AppServerLike): Promise<void> {
} catch (error) {
startupSyncStatus.lastError = getErrorMessage(error, 'startup-sync-failed')
startupSyncStatus.lastAction = 'startup-sync-failed'
if (options.propagateErrors) throw error
} finally {
startupSyncStatus.inProgress = false
}
Expand Down Expand Up @@ -1489,11 +1392,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 @@ -1502,7 +1403,7 @@ export async function handleSkillsRoutes(

if (req.method === 'POST' && url.pathname === '/codex-api/skills-sync/startup-sync') {
try {
await runSkillsSyncStartup(appServer)
await runSkillsSyncStartup(appServer, { propagateErrors: true })
setJson(res, 200, { ok: true })
} catch (error) {
setJson(res, 502, { error: getErrorMessage(error, 'Failed to run startup sync') })
Comment on lines 1404 to 1409
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Action required

2. Github token in errors 🐞 Bug ⛨ Security

Git command failures can leak the GitHub token because toGitHubTokenRemote() embeds it in the
remote URL and runCommand() includes full arguments in the thrown error message. With
propagateErrors: true for the manual startup-sync endpoint, these token-bearing errors can now be
returned to the HTTP caller/UI.
Agent Prompt 
### Issue description
GitHub access tokens can be exposed in HTTP responses/UI because the token is embedded in git remote URLs and command failure messages include full arguments.

### Issue Context
- `toGitHubTokenRemote()` builds `https://x-access-token:*******@github.com/...`.
- `runCommand()` throws `Command failed (cmd args...)` including those args.
- Manual startup sync now propagates errors to the HTTP handler, which returns the error message.

### Fix Focus Areas
- src/server/skillsRoutes.ts[789-791]
- src/server/skillsRoutes.ts[121-158]
- src/server/skillsRoutes.ts[1226-1229]
- src/server/skillsRoutes.ts[1404-1410]

### Suggested fix
Pick one (best to do both):
1) **Redact secrets in error messages**: before constructing/returning error messages, scrub patterns like `x-access-token:<anything>@github.com` or more generally replace the token portion with `x-access-token:[REDACTED]@github.com`.
2) **Avoid embedding tokens in argv**: use a credential helper/`GIT_ASKPASS`, or pass auth via git http extraheader (so the token never appears in the command string/args).

Also ensure `startupSyncStatus.lastError` and `lastSyncError` store redacted messages.

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

Expand All @@ -1519,46 +1420,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 } })
Comment on lines 1423 to +1434
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Action required

1. Enabled state not synced 🐞 Bug ≡ Correctness

The sync flow no longer propagates per-skill enabled/disabled state: push commits the skills
directory as-is while discarding the collected enabled flags, and pull no longer applies any remote
enabled state back via skills/config/write. As a result, toggling a skill enabled/disabled can
fail to reproduce on another machine after pull/startup-sync.
Agent Prompt 
### Issue description
Enabled/disabled status is no longer synchronized across devices because push ignores the collected `enabled` flags and pull no longer applies remote enablement back into local configuration.

### Issue Context
- UI toggles enabled state via `skills/config/write` and then calls `/skills-sync/push`.
- Server sync now commits the skills working tree and discards the installed map that contains `enabled`.
- Pull/startup-sync reloads skills but does not apply enablement.

### Fix Focus Areas
- src/server/skillsRoutes.ts[1099-1112]
- src/server/skillsRoutes.ts[1384-1397]
- src/server/skillsRoutes.ts[1414-1434]
- src/components/content/SkillsHub.vue[327-336]

### Suggested fix
Implement a file-tree source of truth for enablement (since the PR goal is “generic file sync”), e.g.:
1) Write a tracked config file under the skills repo (e.g. `.codex/skills-enabled.json` or similar) during push that contains `{ [skillName]: boolean }` derived from `collectInstalledSkillsMap(appServer)`.
2) On pull/startup-sync, after `pullInstalledSkillsFolderFromRepo(...)`, read that file (if present) and apply it via `appServer.rpc('skills/config/write', { path, enabled })` for each affected skill, then `skills/list` with `forceReload: true`.
3) Ensure missing config file does not break sync (treat as “no enablement overrides”).

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

} catch (error) {
setJson(res, 502, { error: getErrorMessage(error, 'Failed to pull synced skills') })
}
Expand Down Expand Up @@ -1633,12 +1506,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
15 changes: 8 additions & 7 deletions tests.md
View file
Open in desktop
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 treats the skills repository as generic files 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,17 @@ 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.
2. Click `Startup Sync` when no tracked skill file content has changed.
3. Confirm the sync completes without adding a new manifest-only 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.
6. Confirm the sync does not show a no-change `git commit` failure 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.
- Skills Sync does not read, write, or rely on `installed-skills.json`; the repository file tree is the sync source of truth.
- A missing `installed-skills.json` does not delete local skills during Pull or Startup Sync.
- 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 Down Expand Up @@ -1481,7 +1482,7 @@ Model, skill, thinking, and plan controls remain usable while a thread turn is i
5. Verify `AGENTS.md` still exists locally and in remote `origin/main`.

#### Expected Results
- Startup sync may update manifest, but must not delete `AGENTS.md`.
- Startup sync may update skill files, but must not delete `AGENTS.md`.
- If sync creates a commit, changed files do not include `D AGENTS.md`.
- Local and remote `AGENTS.md` hashes remain equal after sync.

Expand Down