Make public skills pull overwrite local files

[friuns2]

Summary

• make public upstream skills pulls reset and clean the local skills tree from remote
• keep private GitHub skills sync on the existing local-edit-preserving flow
• document manual light/dark verification for public shared-skills pull behavior

Testing

• pnpm run build:frontend

Performance audit

• Server-side sync path remains bounded to fetch/reset/clean for public upstream pulls and avoids manifest cleanup loops for the public repo. Browser profiling was not run because this change does not affect browser startup, routing, or thread loading.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

(Agentic_describe updated until commit a26ee43)

Implement public shared skills pull with local file overwrite

✨ Enhancement

Walkthroughs

Description

• Add separate directory for public shared skills with overwrite behavior
• Implement git reset/clean for upstream public skills pulls
• Distinguish upstream vs private sync repos in pull logic
• Add comprehensive test documentation for public skills pull

Diagram

flowchart LR
  A["Pull Request"] --> B["Check Repo Type"]
  B --> C{Is Upstream?}
  C -->|Yes| D["Use shared_skills Dir"]
  C -->|Yes| E["Reset & Clean Files"]
  C -->|No| F["Use skills Dir"]
  C -->|No| G["Preserve Local Edits"]
  D --> H["Sync Shared Skills"]
  E --> H
  F --> I["Sync Private Skills"]
  G --> I

Loading

File Changes

1. src/server/skillsRoutes.ts ✨ Enhancement +52/-7

Separate public shared skills with overwrite sync logic

• Add getSharedSkillsInstallDir() function to isolate public shared skills in nested directory
• Extend ensureSkillsWorkingTreeRepo() with options for custom localDir and
 overwriteLocalFiles flag
• Implement git reset/clean/checkout sequence when overwriteLocalFiles is true for upstream pulls
• Update pullInstalledSkillsFolderFromRepo() to detect upstream repos and apply overwrite behavior
• Modify bootstrapSkillsFromUpstreamIntoLocal() to use shared skills directory with overwrite
 enabled
• Add upstream repo detection logic in sync pull handler to route public pulls separately

src/server/skillsRoutes.ts

---

2. tests.md 📝 Documentation +29/-0

Document public shared skills pull test procedures

• Add new test feature section for public shared skills pull behavior
• Document prerequisites including shared skills directory existence
• Outline step-by-step test procedure for pull operation and verification
• Specify expected results for file overwrite isolation and theme consistency
• Include rollback/cleanup instructions for test artifacts

tests.md

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Corrupts gitlink/worktree repos ✓ Resolved 🐞 Bug ☼ Reliability

Description

ensureSkillsWorkingTreeRepo() only recognizes a repo when .git is a directory; if shared_skills
is a gitlink/worktree (where .git is a file), it will run git init and can detach/corrupt the
existing repo. This PR newly calls ensureSkillsWorkingTreeRepo() on ~/.codex/skills/shared_skills,
making this failure mode much more likely.

Code

src/server/skillsRoutes.ts[R876-879]

+  const localDir = options.localDir ?? getSkillsInstallDir()
await mkdir(localDir, { recursive: true })
const gitDir = join(localDir, '.git')
let hasGitDir = false

Evidence

The repo existence check is stat(...).isDirectory() on .git, so a .git file is treated as “no
repo” and triggers a full git init path. The same file also documents shared_skills as a
“gitlink” (submodule-style), which typically uses a .git *file*, and the PR routes upstream pulls
into that directory.

src/server/skillsRoutes.ts[871-905]
src/server/skillsRoutes.ts[1125-1142]
src/server/skillsRoutes.ts[1213-1220]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ensureSkillsWorkingTreeRepo()` treats the working tree as a git repo only if `<dir>/.git` is a directory. If `<dir>/.git` is a file (gitlink/submodule/worktree-style layout), the function incorrectly assumes no repo exists and runs `git init`, potentially corrupting/detaching the existing nested repo.
### Issue Context
This PR starts using `ensureSkillsWorkingTreeRepo()` with `localDir: getSharedSkillsInstallDir()` for upstream pulls, and the codebase already refers to `shared_skills` as a gitlink.
### Fix Focus Areas
- src/server/skillsRoutes.ts[871-905]
- src/server/skillsRoutes.ts[1125-1142]
- src/server/skillsRoutes.ts[1213-1220]
### Suggested approach
- Replace the `.git` directory check with a more robust probe:
- Either treat `.git` as valid if it exists and is **file OR directory** (`lstat().isFile() || isDirectory()`), **or**
- Prefer `git rev-parse --is-inside-work-tree` / `git rev-parse --git-dir` and only fall back to init when that command fails.
- Ensure the init path never runs when the directory is already a functional git repo.

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

---

2. Wrong upstream pull branch ✓ Resolved 🐞 Bug ≡ Correctness

Description

When pulling from the upstream public skills repo, the code still uses PRIVATE_SYNC_BRANCH ('main')
instead of getPreferredPublicUpstreamBranch() (e.g. 'android' on Android), so an upstream pull can
overwrite local skills with the wrong branch contents.

Code

src/server/skillsRoutes.ts[R1209-1211]

+  await ensureSkillsWorkingTreeRepo(remoteUrl, branch, {
+    overwriteLocalFiles: isUpstreamSkillsRepo(repoOwner, repoName),
+  })

Evidence

The upstream branch is explicitly platform-dependent via getPreferredPublicUpstreamBranch(), and
bootstrapSkillsFromUpstreamIntoLocal() correctly uses it; however
pullInstalledSkillsFolderFromRepo() hardcodes PRIVATE_SYNC_BRANCH and now enables
overwriteLocalFiles for upstream pulls, making the wrong-branch overwrite destructive.

src/server/skillsRoutes.ts[542-550]
src/server/skillsRoutes.ts[729-736]
src/server/skillsRoutes.ts[1206-1212]
src/server/skillsRoutes.ts[1214-1218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Upstream public repo pulls are performed on `PRIVATE_SYNC_BRANCH` (`main`) even though upstream pulls are meant to track `getPreferredPublicUpstreamBranch()` (`android` on Android). With `overwriteLocalFiles` enabled for upstream pulls, this can replace the entire local skills tree with the wrong branch.
### Issue Context
- Upstream branch selection already exists (`getPreferredPublicUpstreamBranch`).
- Bootstrap uses the preferred upstream branch, but authenticated GitHub-sync pulls for the upstream repo do not.
### Fix Focus Areas
- src/server/skillsRoutes.ts[1206-1212]
- src/server/skillsRoutes.ts[1214-1218]
- src/server/skillsRoutes.ts[729-736]
### Suggested change
In `pullInstalledSkillsFolderFromRepo`, select the branch based on whether the repo is upstream:
- If `isUpstreamSkillsRepo(repoOwner, repoName)` is true: `branch = getPreferredPublicUpstreamBranch()`
- Else: `branch = PRIVATE_SYNC_BRANCH`
Then call `ensureSkillsWorkingTreeRepo(remoteUrl, branch, { overwriteLocalFiles: ... })` accordingly.

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

---

3. Checkout before untracked cleanup ✓ Resolved 🐞 Bug ☼ Reliability

Description

In overwriteLocalFiles mode, ensureSkillsWorkingTreeRepo runs git checkout -B ... origin/ before
git clean -fd, so the checkout can fail if untracked files would be overwritten by the remote
branch contents.

Code

src/server/skillsRoutes.ts[R903-907]

+  if (options.overwriteLocalFiles) {
+    try { await runCommand('git', ['reset', '--hard'], { cwd: localDir }) } catch {}
+    await runCommand('git', ['checkout', '-B', branch, `origin/${branch}`], { cwd: localDir })
+    await runCommand('git', ['reset', '--hard', `origin/${branch}`], { cwd: localDir })
+    await runCommand('git', ['clean', '-fd'], { cwd: localDir })

Evidence

The non-overwrite flow explicitly handles untracked files via git stash --include-untracked,
demonstrating untracked working tree state is expected. In overwrite mode, untracked files are only
removed after checkout, leaving a failure window where checkout can abort due to conflicting
untracked paths.

src/server/skillsRoutes.ts[901-909]
src/server/skillsRoutes.ts[921-926]
tests.md[1673-1685]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`overwriteLocalFiles` intends to force local skills to match the remote, including deleting untracked files. Currently the code performs the branch checkout before removing untracked files, which can cause `git checkout` to fail when an untracked path conflicts with the remote tree.
### Issue Context
- Untracked files are explicitly considered in the non-overwrite flow (`stash --include-untracked`).
- The PR's own test steps include creating a local-only (untracked) skill folder before pull.
### Fix Focus Areas
- src/server/skillsRoutes.ts[889-894]
- src/server/skillsRoutes.ts[901-909]
### Suggested change
Reorder overwrite steps so untracked files are removed *before* attempting `checkout -B` to the remote branch. For example (existing repo case):
1. `git reset --hard` (best-effort) to clear tracked changes
2. `git clean -fd` to remove untracked files/dirs
3. `git checkout -B <branch> origin/<branch>`
4. `git reset --hard origin/<branch>`
5. Optionally run `git clean -fd` again after reset if you want to ensure post-checkout untracked is also removed
Apply the same ordering in the "no .git dir" initialization path if applicable.

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

---

4. Upstream synced count incorrect ✓ Resolved 🐞 Bug ≡ Correctness

Description

The upstream pull route reports synced using scanInstalledSkillsFromDisk(), but that scanner only
checks direct child folders under ~/.codex/skills and won’t count skills stored under
~/.codex/skills/shared_skills. As a result, upstream pulls can succeed but return an incorrect
synced count (often 0).

Code

src/server/skillsRoutes.ts[R1553-1565]

+        const repoDir = await pullInstalledSkillsFolderFromRepo(state.githubToken, state.repoOwner, state.repoName)
+        const localSkills = await scanInstalledSkillsFromDisk()
+        const pulledHead = await runCommandWithOutput('git', ['rev-parse', 'HEAD'], { cwd: repoDir }).catch(() => '')
+        await writeSkillsSyncState({
+          ...state,
+          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: localSkills.size, source: 'upstream' } })
+        return true

Evidence

scanInstalledSkillsFromDisk() only scans getSkillsInstallDir() (not
getSharedSkillsInstallDir()), and it only counts entries with //SKILL.md. This PR pulls upstream
into shared_skills, and the accompanying test steps expect skills to live under
~/.codex/skills/shared_skills/..., so the reported count is not aligned with the new storage
location.

src/server/skillsRoutes.ts[576-590]
src/server/skillsRoutes.ts[117-119]
src/server/skillsRoutes.ts[1223-1228]
src/server/skillsRoutes.ts[1543-1565]
tests.md[1666-1689]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
After an upstream pull, the API response uses `scanInstalledSkillsFromDisk()` to compute `synced`, but that function only scans the parent skills directory and doesn’t include `shared_skills`, so the count can be wrong.
### Issue Context
The PR moves upstream pulls into `~/.codex/skills/shared_skills`.
### Fix Focus Areas
- src/server/skillsRoutes.ts[576-590]
- src/server/skillsRoutes.ts[117-119]
- src/server/skillsRoutes.ts[1543-1565]
### Suggested approach
- Either:
- Add a `scanInstalledSkillsFromDir(baseDir)` helper and call it with `getSharedSkillsInstallDir()` in the upstream pull path, or
- Compute the `synced` value from `appServer.rpc('skills/list', { forceReload: true })` results (if that list already reflects shared skills).
- Keep private sync behavior unchanged.

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

---

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit a26ee43