Harden skills sync login and stash recovery

[friuns2]

Summary

• show an actionable Device Login error when GitHub blocks private repo creation
• abort before reset when local skills changes cannot be stashed
• restore untracked local skill files after stash-pop conflicts

Tests

• git diff --check origin/main..HEAD
• pnpm run build:frontend

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

Review Summary by Qodo

Harden skills sync login and stash recovery with error handling

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Improve Device Login error handling for private repo creation failures
  - Detect GitHub 403 permission errors and provide actionable recovery instructions
  - Replace generic API error with user-friendly guidance to create repo manually
• Guard skills sync reset behind stash checkpoint validation
  - Abort before hard reset if local changes cannot be stashed
  - Prevent data loss when stash operation fails
• Restore untracked skill files after stash-pop conflicts
  - Extract untracked files from stash reference when pop conflicts occur
  - Ensure local skill files remain available for commit after sync resolution
• Add comprehensive test documentation for all three scenarios

Diagram

flowchart LR
  A["Device Login<br/>Repo Creation"] -->|403 Permission Error| B["Detect GitHub<br/>Integration Limit"]
  B -->|Actionable Message| C["User Creates Repo<br/>or Uses Regular Login"]
  D["Skills Sync<br/>Pull"] -->|Local Changes| E["Stash Checkpoint"]
  E -->|Stash Fails| F["Abort Before<br/>Hard Reset"]
  E -->|Stash Success| G["Hard Reset &<br/>Stash Pop"]
  G -->|Pop Conflicts| H["Resolve by<br/>File Time"]
  H -->|Restore Untracked| I["Recover Files<br/>from Stash"]

Loading

File Changes

1. src/server/skillsRoutes.ts Bug fix, error handling, enhancement +46/-7

Enhance error handling and stash recovery in skills sync

• Replace generic getGithubJson call with explicit fetch to capture and parse error responses
• Detect 403 status with "Resource not accessible by integration" message and throw actionable error
• Add autostashRef variable to track stash commit hash for later file recovery
• Wrap stash operation in try-catch that throws if local changes exist but stashing fails
• Call new restoreMissingUntrackedFilesFromStash function after stash-pop conflicts
• Implement restoreMissingUntrackedFilesFromStash function to extract untracked files from stash
 tree

src/server/skillsRoutes.ts

---

2. tests.md Tests, documentation +92/-0

Add test documentation for stash and login hardening

• Add test case for Device Login repo-create permission error with 403 response
• Add test case for skills sync aborting before hard reset on stash failure
• Add test case for skills sync restoring untracked files after stash-pop conflicts
• Each test includes prerequisites, steps, expected results, and cleanup instructions
• Tests cover both light and dark theme rendering

tests.md

---

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

Code Review by Qodo

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

1. Locale-dependent stash detection 🐞 Bug ☼ Reliability

Description

ensureSkillsWorkingTreeRepo decides whether an autostash was created by substring-matching the
human-readable output "No local changes to save", which is not a stable signal and can misclassify
stash creation. Misclassification can incorrectly drive the later "stash pop"/conflict-recovery
path, obscuring the true repo state and failure mode.

Code

src/server/skillsRoutes.ts[R920-924]

   const stashOutput = await runCommandWithOutput('git', ['stash', 'push', '--include-untracked', '-m', 'codex-skills-autostash'], { cwd: localDir })
   createdAutostash = !stashOutput.includes('No local changes to save')
-  } catch {}
+    if (createdAutostash) {
+      autostashRef = (await runCommandWithOutput('git', ['rev-parse', 'stash@{0}'], { cwd: localDir })).trim()
+    }

Evidence

The code sets createdAutostash based on a literal message check, then uses that flag to decide
whether to run git stash pop and conflict recovery. The helper used (runCommandWithOutput)
returns only trimmed stdout on success, making this approach especially brittle to output
differences and not tied to a machine-checkable stash existence/ref.

src/server/skillsRoutes.ts[915-943]
src/server/skillsRoutes.ts[162-200]

Agent prompt

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

## Issue description
Autostash creation is detected by string-matching `git stash push` output (`"No local changes to save"`), which is not a reliable, machine-stable signal.
### Issue Context
`createdAutostash` gates whether the code later runs `git stash pop` and the associated recovery steps; if the detection is wrong, the recovery flow becomes misleading and may hide the real failure mode.
### Fix Focus Areas
- src/server/skillsRoutes.ts[915-943]
### Suggested change
Replace output parsing with a ref-based check:
- Capture the current top stash hash (or empty) before stashing: `git rev-parse -q --verify stash@{0}` (catch -> empty).
- Run `git stash push ...`.
- Re-read `git rev-parse -q --verify stash@{0}`; if it changed (or became non-empty when previously empty), then a new stash was created.
- Set `createdAutostash` and `autostashRef` based on this hash, not on human-readable output.

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

---

2. Slow untracked restore loop 🐞 Bug ➹ Performance

Description

restoreMissingUntrackedFilesFromStash spawns a separate git checkout process per untracked file,
which can be very slow when many untracked files exist in the stash. This increases
conflict-recovery latency and can leave a partially-restored state if the loop fails mid-way.

Code

src/server/skillsRoutes.ts[R1065-1078]

+async function restoreMissingUntrackedFilesFromStash(repoDir: string, stashRef: string): Promise<void> {
+  let untrackedFiles = ''
+  try {
+    untrackedFiles = await runCommandWithOutput('git', ['ls-tree', '-r', '--name-only', `${stashRef}^3`], { cwd: repoDir })
+  } catch {
+    return
+  }
+  for (const filePath of untrackedFiles.split(/\r?\n/).map((row) => row.trim()).filter(Boolean)) {
+    try {
+      await stat(join(repoDir, filePath))
+      continue
+    } catch {}
+    await runCommand('git', ['checkout', `${stashRef}^3`, '--', filePath], { cwd: repoDir })
+  }

Evidence

The new helper enumerates all untracked files from the stash’s untracked parent commit and then
calls runCommand('git', ['checkout', ...]) once per file. runCommand spawns a new process per
call, amplifying overhead for N files.

src/server/skillsRoutes.ts[1065-1078]
src/server/skillsRoutes.ts[121-160]

Agent prompt

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

## Issue description
Untracked-file restoration runs `git checkout` once per file, which can be unnecessarily slow.
### Issue Context
This runs in the stash-pop conflict recovery path, where users already have a degraded experience; batching reduces latency and reduces partial-restore risk.
### Fix Focus Areas
- src/server/skillsRoutes.ts[1065-1078]
### Suggested change
Batch file restoration into fewer `git checkout` invocations:
- Build a list of missing paths.
- Run `git checkout ${stashRef}^3 -- <paths...>` in chunks (e.g., 50–200 paths per command or by total argv byte size) to avoid command-length limits.
- Keep the per-path existence check, but avoid spawning one git process per restored file.

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

---

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

1. Locale-dependent stash detection 🐞 Bug ☼ Reliability

ensureSkillsWorkingTreeRepo decides whether an autostash was created by substring-matching the
human-readable output "No local changes to save", which is not a stable signal and can misclassify
stash creation. Misclassification can incorrectly drive the later "stash pop"/conflict-recovery
path, obscuring the true repo state and failure mode.

Agent Prompt

### Issue description
Autostash creation is detected by string-matching `git stash push` output (`"No local changes to save"`), which is not a reliable, machine-stable signal.

### Issue Context
`createdAutostash` gates whether the code later runs `git stash pop` and the associated recovery steps; if the detection is wrong, the recovery flow becomes misleading and may hide the real failure mode.

### Fix Focus Areas
- src/server/skillsRoutes.ts[915-943]

### Suggested change
Replace output parsing with a ref-based check:
- Capture the current top stash hash (or empty) before stashing: `git rev-parse -q --verify stash@{0}` (catch -> empty).
- Run `git stash push ...`.
- Re-read `git rev-parse -q --verify stash@{0}`; if it changed (or became non-empty when previously empty), then a new stash was created.
- Set `createdAutostash` and `autostashRef` based on this hash, not on human-readable output.

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