fix: roll back failed skill activation - #17
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Rollback must also handle failures during stale-link cleanup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Rolls back failed skill activation to remove partial links and preserve the previous snapshot.
Changes:
- Tracks and removes links created during activation failures.
- Preserves the previous cache pointer for clean retries.
File summaries
| File | Summary | Review |
|---|---|---|
.origin89/sync-engineering.py |
Adds activation rollback handling. | Moderate issue: stale-link cleanup can fail after switching snapshots, leaving a dangling link and blocking retry. |
Review details
Suppressed comments (1)
.origin89/sync-engineering.py:176
- If rollback of one path raises, this loop exits and never attempts the remaining
createdentries, so a failure while removing one link can leave other newly created links (ornext) dangling and mask the original activation error. Catch cleanup errors per entry, continue through the full list, and surface the original or combined failure only after all rollback attempts.
for link in reversed(created):
link.unlink()
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| created.append(link) | ||
| next_pointer.symlink_to(f"versions/{state['revision']}", target_is_directory=True) | ||
| created.append(next_pointer) | ||
| os.replace(next_pointer, cache / "current") |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Change
A filesystem error during skills activation could leave dangling links in
.agents/skillsor.claude/skills. Remove links created by the failed attempt, keep the previous snapshot, and allow a clean retry. This adopts the fix from engineering #1.Validation
All 29 bootstrap tests passed against this consumer script. Six injected failure cases cover discovery-link creation and snapshot switching during first install and update, including preservation of local skills and successful retry.
just checkpassed 316 application tests, lint, type checks, record validation, site build, and two matching 35-file dataset builds.