Make skills sync use generic file sync#136
Open
friuns2 wants to merge 2 commits into
Open
Conversation
Review Summary by QodoMake skills sync use generic file tree instead of manifest
WalkthroughsDescription• Remove installed-skills.json manifest as sync source of truth • Use repository file tree directly for skills sync operations • Propagate manual startup sync errors to HTTP caller • Simplify sync logic by eliminating manifest read/write functions Diagramflowchart LR
A["Skills Sync Flow"] --> B["Read Remote Manifest"]
A --> C["Collect Local Skills"]
B --> D["Old: Manifest-based"]
C --> D
D --> E["Write Manifest"]
E --> F["Sync Folder"]
A -.->|After| G["Generic File Sync"]
G --> H["Pull Folder from Repo"]
G --> I["Scan Disk Files"]
H --> J["New: File Tree-based"]
I --> J
J --> K["Sync Folder Only"]
K --> L["No Manifest Operations"]
File Changes1. src/server/skillsRoutes.ts
|
Code Review by Qodo
1. Enabled state not synced
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
1423
to
+1434
| 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 } }) |
There was a problem hiding this comment.
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
Comment on lines
1404
to
1409
| 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') }) |
There was a problem hiding this comment.
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
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