Conversation
…mlinks Determines whether both source and target paths are under the project root, in which case relative symlinks are portable and can be committed to git. Ref: #120
Unix: computes filepath.Rel from link dir to source, falls back to absolute on error. Windows: tries os.Symlink with relative path first (Developer Mode), falls back to mklink /J with absolute paths. All existing call sites pass false (no behavior change yet). Ref: #120
SyncTargetMerge, SyncTargetMergeWithSkills, CreateSymlink, and SyncTarget now accept projectRoot. When non-empty, createLink uses relative symlinks for paths under the project root. All existing callers pass empty string (global mode, no behavior change). Ref: #120
Project-mode sync now passes the project root directory through to createLink, enabling relative symlinks for all paths under the project. Server handlers use s.projectRoot for project-mode dashboards. Ref: #120
Replace direct os.Symlink calls in extras.go with createLink, gaining: - Relative symlink support in project mode - Windows junction fallback for extras (previously missing) Thread projectRoot through SyncExtra, CollectExtraFiles, and all callers. Ref: #120
- Use utils.PathHasPrefix/PathsEqual for Windows case-insensitivity - Eliminate redundant filepath.Clean call - Hoist shouldUseRelative in syncExtraPerFile (compute once, pass bool) - Add invariant comment in SyncTargetMergeWithSkills
status command used a legacy local parser that only looked for a top-level version: key in SKILL.md frontmatter, always returning empty since version now lives under metadata.version. Replace local readSkillVersion() and fetchRemoteSkillVersion() with the shared versioncheck.ReadLocalSkillVersion() and FetchRemoteSkillVersion() from internal/version, matching what doctor and upgrade already use. Remove now-unused skillshareSkillURL constant from init.go. Closes #121
…mode sync previously skipped symlinks that pointed to the correct source, even if their format (absolute vs relative) didn't match the expected mode. This meant existing absolute symlinks in project mode were never converted to relative on subsequent syncs. Add linkNeedsReformat(dest, wantRelative) helper that checks whether the raw readlink result has the wrong format. All four 'already correct' skip points now check format and recreate when mismatched: - SyncTargetMergeWithSkills (skills merge mode) - SyncTarget (skills symlink mode) - syncExtraSymlinkMode (extras directory symlink) - syncOneExtraFile (extras per-file merge) Global mode is unaffected: shouldUseRelative returns false, so linkNeedsReformat never triggers on absolute links. Also fix macOS /var → /private/var path mismatch in TestSyncProject_RelativeSymlinks by EvalSymlinks on expected path.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements relative symlinks for project-mode synchronization to improve directory portability and fixes a bug in the status command's version detection. The changes involve updating the sync stack to propagate a project root and modifying symlink creation logic for both Unix and Windows. Feedback identifies a path resolution bug in the extras sync logic and recommends more robust error handling by checking the results of file removals across several sync functions.
…move errors filepath.Abs(dest) on a relative readlink result resolves against CWD, not the symlink's parent directory. This broke the 'already correct' check when comparing relative symlinks to absolute source paths. Add resolveReadlink() helper that joins relative targets with filepath.Dir(linkPath) before calling filepath.Abs. Also check os.Remove errors in all reformat paths instead of ignoring them silently. Addresses PR #122 review feedback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements relative symlinks for project mode to ensure directory portability and fixes a bug in the status command's version detection logic. The implementation involves a new utility for path relativity and the propagation of project root context across the sync, extras, and server packages. Feedback highlights the need for more robust path prefix handling at the filesystem root and consistent use of absolute paths when calculating relative symlink targets to avoid potential failures.
- shouldUseRelative: avoid double separator when projectRoot is / - symlink_windows.go: use pre-resolved absTarget/absSource for Rel - extras.go: check os.Remove error on wrong symlink replacement
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces relative symlinks for project mode (skillshare sync -p), ensuring that project directories remain portable when moved or cloned. It also includes a bug fix for version detection in the status command by refactoring version parsing into a shared internal package. The changes involve updating function signatures across the CLI and server components to propagate the project root, as well as implementing logic to automatically upgrade existing absolute symlinks to relative ones during sync. Feedback was provided regarding a potential issue on Windows where the tool might enter a loop of removing and recreating directory junctions if the environment does not support relative symlinks.
On Windows without Developer Mode, createLink falls back to an NTFS junction (always absolute). linkNeedsReformat would then return true on every sync, causing a remove→recreate loop. Add build-tagged canCreateRelativeLink(): returns true on Unix, and on Windows probes once with sync.Once by creating a test relative symlink in a temp dir. linkNeedsReformat skips reformat when the platform cannot create relative links. Also use filepath.Clean instead of filepath.Abs in resolveReadlink (input is already absolute, avoids unnecessary os.Getwd). Addresses PR #122 review feedback.
Two correctness improvements from Codex review: 1. EvalSymlinks before relative-link decisions: shouldUseRelative and createLink now resolve symlinked ancestors via EvalSymlinks so that a project opened through a symlinked workspace, or a target dir that traverses a symlink, produces correct relative paths (or falls back to absolute when paths diverge from the project root). 2. Atomic reformat via reformatLink: instead of remove→create (which loses the link on createLink failure), reformat now creates a temp link and renames it over the original. On Unix this is atomic; on Windows it falls back to remove→create when rename fails. Tests added for symlinked project root, divergent target symlink, and reformatLink atomic swap.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces relative symlinks for project mode (skillshare sync -p), ensuring project portability by using paths relative to the project root. It also refactors the status command to use a shared version parser, fixing a detection bug. The review feedback highlights several opportunities to improve the robustness of the new symlink logic, specifically regarding error handling for os.Readlink and ensuring consistent absolute path resolution when calculating relative links.
…move errors filepath.Abs(dest) on a relative readlink result resolves against CWD, not the symlink's parent directory. This broke the 'already correct' check when comparing relative symlinks to absolute source paths. Add resolveReadlink() helper that joins relative targets with filepath.Dir(linkPath) before calling filepath.Abs. Also check os.Remove errors in all reformat paths instead of ignoring them silently. Addresses PR #122 review feedback.
On Windows without Developer Mode, createLink falls back to an NTFS junction (always absolute). linkNeedsReformat would then return true on every sync, causing a remove→recreate loop. Add build-tagged canCreateRelativeLink(): returns true on Unix, and on Windows probes once with sync.Once by creating a test relative symlink in a temp dir. linkNeedsReformat skips reformat when the platform cannot create relative links. Also use filepath.Clean instead of filepath.Abs in resolveReadlink (input is already absolute, avoids unnecessary os.Getwd). Addresses PR #122 review feedback.
1 participant
[0.18.9] - 2026-04-07
New Features
skillshare sync -pnow creates relative symlinks (e.g.,../../.skillshare/skills/my-skill) instead of absolute paths. This makes the project directory portable — rename it, move it, or clone it on another machine and all skill symlinks continue to work. Global mode continues to use absolute paths. Existing absolute symlinks are automatically upgraded to relative on the next syncBug Fixes
skillshare statusno longer reports! Skill: not found or missing versionwhen the version is stored undermetadata.versionin the SKILL.md frontmatter. Previously, thestatuscommand used its own local parser that only checked for a top-levelversion:key, whiledoctor(fixed in v0.18.7) andupgradealready used the correct shared parser