Make install command non-destructive with merge support#2
Closed
ibrahimkteish wants to merge 3 commits intopointfreeco:mainfrom
Closed
Make install command non-destructive with merge support#2ibrahimkteish wants to merge 3 commits intopointfreeco:mainfrom
ibrahimkteish wants to merge 3 commits intopointfreeco:mainfrom
Conversation
- Remove destructive file deletion - all installs now merge files - Add support for --path . or --path current to install into current directory - Add path validation for ALL custom paths to ensure they're in .codex/skills or .claude/skills - Add helpful error messages with usage examples when validation fails - Add user confirmation prompt when installing to custom paths - Fix extraction to place skills directly in target path, not in nested skills/skills folder - Update help text to document current directory option This change ensures users never lose existing work when installing skills, and prevents installations to arbitrary directories outside the expected skills folders.
234b059 to
3af0178
Compare
- Add test target to Package.swift - Refactor Install.swift to extract testable helper functions: - isCurrentDirectoryPath: Detects if path is current directory - validateInstallPath: Validates path contains expected tool pattern - resolveInstallURL: Resolves final install URL based on inputs - Create comprehensive test suite with 21 tests using @test syntax: - Path validation tests (valid/invalid paths, tool matching) - Current directory detection tests - Install URL resolution tests - Tool default path tests - Edge cases (whitespace, case sensitivity, trailing slashes) - All tests passing with Swift Testing framework (macOS 15+)
johankool
reviewed
Jan 25, 2026
johankool
left a comment
There was a problem hiding this comment.
Some suggestions. Not saying it's up to you to fix them, just sharing what I found when trying to use pfw with copilot-cli.
- Remove 'current' keyword support to avoid conflicts with actual directory names - Make path validation flexible to support different naming conventions (.github, .copilot, .claude) - Update validation to only require '/skills' in path instead of tool-specific pattern - Update tests to reflect new validation logic
Contributor
Author
|
closing in favor of a better implementation here #3 |
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
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
--path ./skillsskills/skills/folder)Motivation
Previously,
pfw installwould delete everything in the target directory before installing, which could cause loss of custom or work-in-progress skills. Additionally, users could accidentally install to arbitrary locations like/tmpor project root directories. Finally, the zip extraction was creating a nestedskills/skills/structure instead of placing skills directly in the target path.This change makes the install command always merge new files with existing ones, validates that installations only happen in appropriate skills directories, and correctly extracts skills to the target location.
Changes
FileManager.default.removeItem()call for the install directory - installations now preserve existing files (individual skills are still updated to latest versions)--path .to install into their current working directory--pathargument is validated to ensure it contains/skills(supports various naming conventions like.copilot,.claude,.github,.config, etc.)skills/subfolderExamples
Valid usage:
Invalid usage (now properly rejected):
# Will show helpful error message pfw install --tool claude --path /tmp pfw install --tool claude --path /Users/me/MyProjectTest plan
pfw install- should merge into default locationpfw install --path /project/.claude/skills- should prompt and merge, skills directly in pathpfw install --path /tmp- should error with helpful messagecd ~/.claude/skills/test && pfw install --path .- should prompt and mergecd /tmp && pfw install --path .- should error with helpful message