|
| 1 | +# Design: Directory-level skills symlink |
| 2 | + |
| 3 | +## Technical Approach |
| 4 | + |
| 5 | +Change the default sync type for skills targets from `symlink-contents` to `symlink` across all agent init templates and this repo's config. No new code paths, types, or flags — `SyncType::Symlink` already handles directory sources correctly on both Unix and Windows. The change is purely config-level: update TOML strings in `src/init.rs` and `.agents/agentsync.toml`, then update tests to assert a single directory symlink instead of per-entry symlinks. |
| 6 | + |
| 7 | +This maps directly to the proposal's approach (Option 1 from exploration). |
| 8 | + |
| 9 | +## Architecture Decisions |
| 10 | + |
| 11 | +### Decision: Reuse existing `SyncType::Symlink` instead of a new variant |
| 12 | + |
| 13 | +**Choice**: Use the existing `Symlink` sync type for directory sources |
| 14 | +**Alternatives considered**: New `SyncType::SymlinkDirectory` variant; `link_directory` boolean flag on `SymlinkContents` |
| 15 | +**Rationale**: `create_symlink()` (`src/linker.rs:344-463`) already handles directory sources — on Unix it calls `std::os::unix::fs::symlink` (line 442) which works for directories, and on Windows it dispatches to `std::os::windows::fs::symlink_dir` (line 448) when the source is a directory. Adding a new variant or flag would be pure overhead with no functional benefit. The `process_target()` dispatch (line 204-208) routes `SyncType::Symlink` correctly without any changes. |
| 16 | + |
| 17 | +### Decision: Config-only change, no migration tooling |
| 18 | + |
| 19 | +**Choice**: Change defaults for new projects; existing projects keep `symlink-contents` until manual update |
| 20 | +**Alternatives considered**: Auto-migration on sync; deprecation warnings for `symlink-contents` |
| 21 | +**Rationale**: `symlink-contents` remains valid for targets that use `pattern` filtering (commands, prompts, agents). Auto-migration would be risky and unnecessary — users can opt in by editing their config or re-running `agentsync init`. Running `agentsync sync --clean` handles the transition cleanly via existing logic. |
| 22 | + |
| 23 | +### Decision: Accept `registry.json` exposure |
| 24 | + |
| 25 | +**Choice**: Allow `registry.json` and other non-skill files in `.agents/skills/` to be visible through the directory symlink |
| 26 | +**Alternatives considered**: Adding a `.gitignore`-style filter for directory symlinks; documenting as a blocker |
| 27 | +**Rationale**: Agents already read the skills directory contents. `registry.json` is non-sensitive metadata. The simplicity of a single directory symlink outweighs the minor exposure. |
| 28 | + |
| 29 | +## Data Flow |
| 30 | + |
| 31 | +No change to data flow. The existing path through the linker remains identical: |
| 32 | + |
| 33 | +``` |
| 34 | +agentsync sync |
| 35 | + │ |
| 36 | + ▼ |
| 37 | +Linker::sync() → process_target() |
| 38 | + │ |
| 39 | + ├── SyncType::Symlink ──→ create_symlink() |
| 40 | + │ (now used for skills) │ |
| 41 | + │ ├── dest is symlink? → check target, skip or update |
| 42 | + │ ├── dest is real dir? → backup to .bak.<timestamp> |
| 43 | + │ └── create symlink (unix: symlink, windows: symlink_dir) |
| 44 | + │ |
| 45 | + └── SyncType::SymlinkContents ──→ create_symlinks_for_contents() |
| 46 | + (still used for commands, prompts, agents) |
| 47 | +``` |
| 48 | + |
| 49 | +The only difference: skills targets now take the `Symlink` branch instead of `SymlinkContents`. |
| 50 | + |
| 51 | +## File Changes |
| 52 | + |
| 53 | +| File | Action | Description | |
| 54 | +|------|--------|-------------| |
| 55 | +| `src/init.rs:73` | Modify | Claude skills target: `type = "symlink-contents"` → `type = "symlink"` | |
| 56 | +| `src/init.rs:109` | Modify | Codex skills target: `type = "symlink-contents"` → `type = "symlink"` | |
| 57 | +| `src/init.rs:126` | Modify | Gemini skills target: `type = "symlink-contents"` → `type = "symlink"` | |
| 58 | +| `src/init.rs:148` | Modify | OpenCode skills target: `type = "symlink-contents"` → `type = "symlink"` | |
| 59 | +| `.agents/agentsync.toml:73` | Modify | Repo's OpenCode skills: `type = "symlink-contents"` → `type = "symlink"` | |
| 60 | +| `.agents/agentsync.toml:96` | Modify | Repo's Copilot skills: `type = "symlink-contents"` → `type = "symlink"` | |
| 61 | +| `src/linker.rs` (tests) | Modify | Add `test_sync_symlink_directory_for_skills` unit test | |
| 62 | +| `tests/test_agent_adoption.rs` | Modify | Update 5 test functions: change skills targets from `symlink-contents` to `symlink`, update assertions from per-entry checks to directory symlink checks | |
| 63 | + |
| 64 | +## Interfaces / Contracts |
| 65 | + |
| 66 | +No new interfaces. The only contract change is in the TOML config schema, where skills targets use a different value for an existing field: |
| 67 | + |
| 68 | +```toml |
| 69 | +# Before |
| 70 | +[agents.claude.targets.skills] |
| 71 | +source = "skills" |
| 72 | +destination = ".claude/skills" |
| 73 | +type = "symlink-contents" |
| 74 | + |
| 75 | +# After |
| 76 | +[agents.claude.targets.skills] |
| 77 | +source = "skills" |
| 78 | +destination = ".claude/skills" |
| 79 | +type = "symlink" |
| 80 | +``` |
| 81 | + |
| 82 | +The `SyncType` enum, `TargetConfig` struct, and all Rust APIs remain unchanged. |
| 83 | + |
| 84 | +## Testing Strategy |
| 85 | + |
| 86 | +| Layer | What to Test | Approach | |
| 87 | +|-------|-------------|----------| |
| 88 | +| Unit | Directory symlink creation for skills source | New test `test_sync_symlink_directory_for_skills` in `src/linker.rs`: create a `.agents/skills/` dir with multiple skill subdirectories, configure `type = "symlink"`, sync, assert dest is a single symlink pointing to source dir (not individual entries inside a real dir) | |
| 89 | +| Unit | Clean removes directory symlink | Verify existing `SyncType::Symlink` clean test covers this, or add assertion that cleaning a directory symlink works (line 894-905 already handles it) | |
| 90 | +| Integration | Claude adoption with directory symlink | Update `test_adoption_claude_with_skills_and_commands` (line 117-163): change skills target to `"symlink"`, replace `assert_symlink_points_to(root, ".claude/skills/debugging", ...)` with `assert_symlink_points_to(root, ".claude/skills", "skills")` — a single directory symlink | |
| 91 | +| Integration | Gemini adoption with directory symlink | Update `test_adoption_gemini_with_skills_and_commands` (line 169-261): same pattern | |
| 92 | +| Integration | Codex adoption with directory symlink | Update `test_adoption_codex_skills_only` (line 266-298): same pattern | |
| 93 | +| Integration | Multi-agent adoption | Update `test_adoption_multi_agent_shared_skills` (line 303-424): change all three agents' skills targets, update all per-skill assertions to directory-level assertions | |
| 94 | +| Integration | Dry-run | Update `test_adoption_dry_run_no_side_effects` (line 430-469): change skills target to `"symlink"` | |
| 95 | + |
| 96 | +### Test assertion pattern change |
| 97 | + |
| 98 | +```rust |
| 99 | +// Before: assert individual symlinks inside a real directory |
| 100 | +assert_symlink_points_to(root, ".claude/skills/debugging", "debugging"); |
| 101 | +assert_symlink_points_to(root, ".claude/skills/testing", "testing"); |
| 102 | + |
| 103 | +// After: assert the directory itself is a symlink to the source |
| 104 | +assert_symlink_points_to(root, ".claude/skills", "skills"); |
| 105 | +// Then verify contents are accessible through the symlink |
| 106 | +assert!(root.join(".claude/skills/debugging").exists()); |
| 107 | +assert!(root.join(".claude/skills/testing").exists()); |
| 108 | +``` |
| 109 | + |
| 110 | +## Migration / Rollout |
| 111 | + |
| 112 | +### New projects |
| 113 | +`agentsync init` emits `type = "symlink"` for skills targets. No action needed. |
| 114 | + |
| 115 | +### Existing projects |
| 116 | +- Config still says `symlink-contents` → behavior is unchanged, no breakage |
| 117 | +- User updates config to `symlink` → next `agentsync sync` triggers backup logic: |
| 118 | + 1. `create_symlink()` sees `.claude/skills/` is a real directory (not a symlink) |
| 119 | + 2. Renames to `.claude/skills.bak.<timestamp>` (line 412-423) |
| 120 | + 3. Creates single directory symlink `.claude/skills → ../../.agents/skills` |
| 121 | +- User runs `agentsync sync --clean` first → cleaner transition: |
| 122 | + 1. `clean()` with old `SymlinkContents` config removes per-entry symlinks + empty dir (line 860-892) |
| 123 | + 2. User updates config to `symlink` |
| 124 | + 3. `sync()` creates the directory symlink fresh |
| 125 | + |
| 126 | +### This repo |
| 127 | +Update `.agents/agentsync.toml`, run `pnpm run agents:sync:clean` to transition. |
| 128 | + |
| 129 | +## Open Questions |
| 130 | + |
| 131 | +- [x] Does `create_symlink` handle directory sources? → **Yes**, confirmed at lines 441-452 |
| 132 | +- [x] Does clean handle `Symlink` type for directories? → **Yes**, `fs::remove_file` on a directory symlink works on both Unix and Windows (line 900) |
| 133 | +- [x] Does backup logic handle existing real directories at dest? → **Yes**, `fs::rename` at line 417 |
| 134 | +- [ ] Should we add a note in CLI `--help` or docs about the `symlink` vs `symlink-contents` distinction for skills? → Non-blocking, can be a follow-up |
0 commit comments