Feat/tty update

[EdwardIrby]

No description provided.

[claude]

Pull Request Review: Feat/tty update

Summary

This PR introduces a significant architectural improvement by implementing a symlink-based installation system with multi-agent support and TTY detection.

✅ Strengths

Architecture

• Excellent design: The symlink-based approach with central storage is smart
• Two-phase installation: Clean separation between installing to central storage and creating symlinks
• Relative symlinks: Correctly uses relative paths ensuring portability

User Experience

• Interactive multi-select UI: Intuitive checkbox-based agent selection with arrow keys, vim-style, number keys
• TTY detection: Automatically adapts to headless environments
• Copilot detection fix: Special handling for .github/skills vs .github

Code Quality

• Comprehensive tests: 128 lines of new test coverage
• Good error handling: Conflict resolution prompts and abort options
• Security maintained: All existing security validations preserved

🔍 Critical Issues

1. README Outdated ❌

install.sh now uses --agents (plural) but README.md still shows --agent (singular)

Impact: Users following README will get "Unknown option: --agent" errors
Fix: Update all README examples to use --agents

2. handle_existing_skill Headless Hang ⚠️

install.sh:110-133 uses interactive read without TTY check

Issue: In headless mode (CI/CD), this will hang when prompting for conflict resolution
Fix: Add TTY check - default to "skip" in headless mode

3. Breaking Change Undocumented ⚠️

The change from .claude/ to .plaited/ as source directory is a breaking change

Fix: Document in README that this requires clean reinstall

🔧 Should Fix

4. Depth calculation (install.sh:96): grep -c . may miscount with trailing slashes
5. Symlink errors: 2>/dev/null silences useful debugging info
6. Test coverage: Missing multi-agent installation integration tests
7. Magic number: The + 5 in line 664 needs a comment

🔒 Security Review

✅ No new security vulnerabilities
✅ Path traversal validation maintained
✅ Symlink security correct (relative paths, validation)

📊 Test Coverage

✅ Excellent: 128 new test lines covering:

• get_relative_symlink_path calculation
• Symlink creation integration
• Headless mode error handling
• --agent deprecation
• parse_source .plaited change

📝 Conclusion

Well-architected feature with good test coverage and thoughtful UX improvements.

However, 3 critical issues need fixing before merge:

1. Update README to use --agents
2. Add TTY check to handle_existing_skill
3. Document breaking change

Recommendation: Request changes for critical items, then approve.

Overall Assessment: 7/10

[EdwardIrby]

Addressed All Review Feedback

Critical Issues ✅

1. README updated - Changed --agent to --agents throughout, added architecture documentation
2. handle_existing_skill fixed - Added TTY check, defaults to "skip" in headless mode to prevent hanging
3. Breaking change documented - Added "Upgrading from Previous Versions" section

Should Fix Items ✅

4. Depth calculation - Replaced grep -c . with awk to handle trailing slashes correctly
5. Symlink error visibility - Errors now captured and displayed instead of silenced with 2>/dev/null
6. Multi-agent integration test - Added test verifying symlinks from 3 agents to single central storage
7. Magic number documented - Added comment explaining + 5 cursor positioning in render_menu

All 130 tests passing.

[claude]

Pull Request Review: Feat/tty update

Summary

This PR introduces a significant architectural change to the skills installer, moving from direct installation to agent directories to a centralized symlink-based approach. The changes also add multi-agent support and improve headless/CI-CD compatibility.

Code Quality & Best Practices

Strengths ✓

1. Excellent Security Practices

   • Path traversal validation is thorough (install.sh:95-97, 258-270, 329-363)
   • Input validation for sparse paths and scope components
   • Atomic file operations with proper cleanup on failure (install.sh:1074-1114)
   • Checksum verification for downloaded projects.json (install.sh:1508-1533)

2. Good Error Handling

   • Headless mode detection with fallback behavior (install.sh:122-127)
   • Conflict resolution for existing skills (install.sh:111-142)
   • Detailed error messages with context
   • ln errors now shown instead of silenced (install.sh:893-899)

3. Well-Structured Code

   • Clear separation of concerns (symlink management, agent detection, installation)
   • Comprehensive test coverage with integration tests for symlinks (install.spec.ts:1053-1198)
   • Good use of helper functions

4. Documentation

   • Breaking changes clearly documented in README
   • Architecture diagram helps users understand the symlink approach

Areas for Improvement

1. Magic Numbers (Minor)

   • Line 675: $((num_agents + 5)) has a comment explaining the calculation, which is good, but the hardcoded 10 for num_agents on line 666 should use ${#selected[@]} or count from $ALL_AGENTS dynamically

   local num_agents=10  # install.sh:666

   Recommendation: Calculate dynamically to avoid manual updates when agents are added/removed

2. Depth Calculation (Addressed in PR)

   • The fix in install.sh:97 using awk is good, but could be simplified:

   depth=$(echo "$agent_skills_dir" | awk -F'/' '{n=0; for(i=1;i<=NF;i++) if($i!="") n++; print n}')

   Alternative: depth=$(echo "$agent_skills_dir" | tr -cd '/' | wc -c) might be clearer, though the awk approach handles edge cases better

3. Error Propagation (Minor)

   • install.sh:1268-1271: Symlink failures print error but continue. Consider whether partial symlink failure should exit non-zero

   if ! create_agent_symlinks "$agents"; then
     print_error "Failed to create some symlinks"
     # Continue anyway - central storage is intact
   fi

   Recommendation: Add a flag or counter for failed symlinks and warn users at the end

Potential Bugs

1. Race Condition in Menu Rendering (Low Priority)

install.sh:717 - Reading arrow keys with timeout could miss rapid key presses:

read -rsn2 -t 0.1 key2

This is acceptable for an interactive menu but may feel laggy on slow terminals.

2. grep vs awk Inconsistency (Fixed in PR)

The PR addresses this correctly by switching from grep -c to awk for depth calculation, which properly handles trailing slashes.

Performance Considerations

Strengths ✓

1. Caching - Project names are cached (install.sh:201-230) to avoid re-parsing JSON
2. Efficient File Operations - Using symlinks saves disk space and improves installation speed
3. Sparse Checkouts - Only fetches necessary directories from git repos

Minor Optimizations

1. Parallel Git Operations - Consider adding parallel cloning for multiple projects (though this may complicate error handling)
2. Menu Redraw - The entire menu is redrawn on every keystroke. For 10 items this is fine, but consider partial updates if the list grows

Security Concerns

Excellent Security Posture ✓

1. Path Validation - Comprehensive checks prevent directory traversal
2. Checksum Verification - Mandatory for downloaded projects.json
3. Input Sanitization - Validates all user inputs and repo paths
4. Atomic Operations - Uses temp directories to prevent partial installs

Recommendations

1. Symlink Validation (install.sh:944-945)

   target=$(readlink "$symlink" 2>/dev/null)
   if [[ "$target" =~ \.plaited/skills/ ]]; then

   This regex could match unintended paths. Consider using exact pattern matching:

   if [[ "$target" == *"/.plaited/skills/"* ]] || [[ "$target" == "../"*"/.plaited/skills/"* ]]; then

2. TTY Detection - Line 1546 uses [ -t 0 ] which is correct, but document that this checks stdin specifically

Test Coverage

Strengths ✓

1. Comprehensive Tests

   • Symlink creation tests (install.spec.ts:1103-1197)
   • Multi-agent support tests
   • Headless mode tests (install.spec.ts:1023-1046)
   • Edge case coverage (install.spec.ts:1204-1511)

2. Test Quality

   • Good cleanup in finally blocks
   • Tests verify actual symlink targets, not just existence
   • Integration tests cover multi-agent scenarios

Gaps (Suggestions)

1. Missing Test Cases

   • Conflict resolution behavior (when user chooses skip/abort)
   • Symlink creation failure scenarios
   • Permission errors on symlink creation
   • Multiple concurrent installations

2. Test for PR Feedback Items

   • Test coverage for depth calculation with trailing slashes ✓ (could add more edge cases)
   • Test for multi-agent symlink creation ✓ (good coverage)

Breaking Changes

Well-Documented ✓

The README clearly documents the breaking change from previous versions (README.md:64-74):

• Old installations won't work with new architecture
• Clear upgrade path provided

Recommendation

Consider adding migration script or detection of old-style installations to warn users.

Additional Observations

Positive

1. Interactive UX - The multi-select menu with vim keybindings ('j', 'k'), number keys, and select all/none is excellent
2. Copilot Detection - Smart handling of .github directory to avoid false positives (install.sh:600-610)
3. Dependency Resolution - Automatic dependency detection and installation is well-implemented

Suggestions

1. Verbose Mode - Consider adding --verbose flag for debugging symlink creation
2. Dry Run - Add --dry-run to preview what would be installed
3. Symlink Verification - Add a --verify command to check symlink integrity

Final Verdict

Recommendation: Approve with minor suggestions

This is a well-crafted PR that introduces significant improvements:

• Better architecture with centralized storage
• Improved multi-agent support
• Excellent test coverage
• Strong security practices
• Good error handling

The minor issues noted above are suggestions for enhancement rather than blockers. The code is production-ready.

Priority Actions (Optional)

1. Consider dynamic calculation of num_agents instead of hardcoded value
2. Add test coverage for conflict resolution scenarios
3. Consider stricter symlink pattern matching in removal function

Great work on this feature! The symlink architecture is a solid improvement, and the attention to headless mode and security details shows thoughtful engineering.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

[EdwardIrby]

Implemented Additional Review Suggestions

Areas for Improvement ✅

• Dynamic num_agents - Now calculated from ALL_AGENTS using wc -w instead of hardcoded 10
• Symlink failure tracking - create_agent_symlinks now returns failure count, displayed at end of install

Security Concerns ✅

• Stricter symlink validation - Changed from regex =~ to exact pattern matching == with glob
• TTY detection documented - Added comments explaining [ -t 0 ] checks stdin file descriptor

All 130 tests passing.

[claude]

Pull Request Review: Feat/tty update

Summary

This is an excellent architectural improvement that modernizes the skills installer with a symlink-based approach. The PR includes 6 well-documented commits that progressively build the feature, TTY detection for headless environments, and comprehensive test coverage. The code quality is high with good attention to security and edge cases.

✅ Strengths

Architecture & Design

• Symlink-based installation is a smart solution that saves disk space and ensures consistency across agents
• Central storage in .plaited/skills/ with symlinks to agent directories is clean and maintainable
• Two-phase installation (central → symlinks) provides clear separation of concerns
• Multi-agent support with --agents (plural) is more intuitive than the previous --agent flag

Code Quality

• Excellent comments explaining non-obvious logic (e.g., TTY detection, magic numbers in cursor positioning)
• Consistent error handling with clear user feedback throughout
• Good separation of concerns with well-named helper functions
• Defensive programming with validation at boundaries

User Experience

• Interactive multi-select menu with keyboard shortcuts (arrows, vim keys, number toggles, select all/none) is very polished
• Copilot detection improvement avoiding false positives from .github/ directories
• Headless mode auto-detection with helpful error messages
• Conflict resolution for existing directories with interactive prompts
• Clear upgrade path documented in README with breaking change notice

Testing

• Comprehensive test coverage including:
  • Symlink path calculation for various agent directories
  • Multi-agent symlink integration tests
  • Headless mode behavior
  • Breaking change validation (--agent flag removal)
• Integration tests verify the entire symlink workflow end-to-end

Security

• Pattern matching improvements for symlink validation using exact string matching instead of regex
• Error visibility - removed silent error suppression (2>/dev/null) to expose issues
• Symlink failure tracking with end-of-install warnings
• TTY detection properly documented explaining what [ -t 0 ] checks

🔍 Areas for Consideration

Minor Issues

1. Symlink validation in remove_agent_symlinks (line 599)

   if [[ "$target" == *".plaited/skills/"* ]]; then

   This uses substring matching which could theoretically match unintended paths. Consider using a more precise check:

   # Get canonical path and check if it starts with our central dir
   if [[ "$target" == *"/.plaited/skills/"* ]] || [[ "$target" == ".plaited/skills/"* ]]; then

   However, the current implementation is probably sufficient for the use case.

2. Error handling in symlink creation (line 545)
   The code captures and displays symlink errors, which is good. However, consider checking if the target actually exists before creating the symlink:

   # Verify target exists before creating symlink
   if [ ! -d "$CENTRAL_SKILLS_DIR/$skill_name" ]; then
     print_error "    Target does not exist: $skill_name"
     failed_count=$((failed_count + 1))
     continue
   fi

3. Depth calculation edge case
   The get_relative_symlink_path function at line 97 handles trailing slashes well with awk, but it might be worth adding a test case for paths with multiple consecutive slashes (e.g., .claude//skills) to ensure robustness.

Documentation

4. Breaking change communication
   The README clearly documents the breaking change, but consider adding a version check or migration helper that detects old installations and offers to migrate them automatically. This would improve the upgrade experience.

5. Symlink limitations
   Consider documenting potential limitations:

   • Symlinks may not work on all filesystems (e.g., some Windows setups without developer mode)
   • What happens if users accidentally modify files through symlinks (they'll affect all agents)

Edge Cases

6. Circular symlink protection
   While unlikely in this use case, consider adding validation to prevent creating symlinks that point to themselves or create circular references.

7. Concurrent installation safety
   If multiple users/processes run the installer simultaneously, there could be race conditions when creating the central directory or symlinks. Consider adding lock files or atomic operations if this is a concern.

🎯 Test Coverage Assessment

Excellent - The PR adds comprehensive tests:

• ✅ Symlink path calculation for all agent types
• ✅ Multi-agent installation integration test
• ✅ Headless mode error handling
• ✅ Breaking change validation (--agent → --agents)
• ✅ Edge cases in existing test suites

Suggested additional tests:

• Conflict resolution behavior (existing directory vs symlink)
• Symlink failure scenarios and warning display
• Uninstall with mixed symlinks and real directories

🔒 Security Assessment

Strong - No significant security concerns. The PR actually improves security:

• ✅ Removed silent error suppression for better visibility
• ✅ Uses exact pattern matching for symlink validation
• ✅ Validates paths before operations
• ✅ Properly handles TTY detection to avoid hanging
• ✅ Maintains existing security validations (sparse_path, file size limits)

📊 Performance Considerations

Improved - The symlink approach:

• ✅ Reduces disk space usage (one copy vs N copies for N agents)
• ✅ Faster installation (no need to copy files multiple times)
• ✅ Atomic updates (update central storage, all agents see changes immediately)

Minor consideration: The multi-select UI redraws the entire menu on each keypress. For 10 agents this is fine, but if the list grows significantly, consider only redrawing changed lines.

🎨 Code Style

Excellent consistency with the existing codebase:

• Proper use of local variables
• Consistent quoting of variables
• Clear function documentation
• Meaningful variable names
• Proper error codes and return values

🚀 Recommendation

Approve with minor suggestions - This is a high-quality PR that significantly improves the installer. The suggestions above are minor enhancements rather than blocking issues. The architecture is sound, testing is comprehensive, and the user experience is polished.

Optional Enhancements (Post-Merge)

1. Add automatic migration from old to new architecture
2. Add --dry-run flag to show what would be installed without making changes
3. Consider adding a --repair command to fix broken symlinks
4. Add symlink integrity check command

Great work on this feature! 🎉

[EdwardIrby]

Implemented Final Review Items

Documentation ✅

• Removed breaking changes section - internal tool, not needed
• Added Windows requirements - documents need for WSL or Developer Mode for symlinks
• Added symlink edit warning - notes that edits through symlinks affect all agents

Safety Features ✅

• Circular symlink protection - validate_symlink_not_circular() prevents self-referencing or looping symlinks
• Concurrent installation safety - Lock file mechanism with:
  • Atomic lock acquisition using mkdir
  • Stale lock detection (removes locks from dead processes)
  • Lock released on exit/cleanup
• Target existence check - Verifies skill exists in central storage before creating symlink

All 130 tests passing.

[claude]

Pull Request Review: Feat/tty update

Summary

This PR introduces a significant architectural change to the skills installer, moving from installing skills directly to agent directories to a centralized storage approach with symlinks. It also adds multi-agent support and improves the interactive UX.

Overall Assessment: APPROVED with Minor Recommendations

This is a well-architected PR that significantly improves the installer's functionality and user experience. The code quality is high, with strong security practices and comprehensive test coverage.

Strengths

Architecture & Design

1. Central Storage Pattern - Excellent DRY approach using .plaited/skills/ as single source of truth
2. Symlink Strategy - Smart use of relative symlinks avoids duplication across agents
3. Atomic Operations - Lock file mechanism prevents race conditions during concurrent installations
4. Two-Phase Installation - Clean separation: install to central storage then symlink to agents

Code Quality

1. Security-First Approach

   • Path traversal validation
   • Circular symlink detection
   • Input sanitization for shell commands
   • Atomic lock acquisition with stale lock cleanup
   • Scope component validation

2. Robust Error Handling

   • Atomic replace-on-install with temp directories
   • Graceful fallback for headless mode (TTY detection)
   • Lock file with PID tracking and stale process detection
   • Comprehensive validation before destructive operations

3. User Experience

   • Interactive multi-select menu with keyboard shortcuts
   • Auto-detection of installed agents
   • Clear progress indicators and error messages
   • Conflict resolution prompts for existing directories

Testing

• Comprehensive test coverage for new functions
• Integration tests for symlink creation
• Multi-agent symlink scenarios tested
• Edge cases covered (headless mode, invalid agents, etc.)

Potential Issues & Recommendations

Critical Issues

None identified - No blocking issues found.

Medium Priority

1. Symlink Depth Calculation Inconsistency (install.sh:101, install.spec.ts:1230)

Location: get_relative_symlink_path() function

Issue: The production code uses awk with proper empty component filtering, but the test uses tr | grep which behaves differently with trailing slashes.

Impact: Tests may pass for edge cases that fail in production.

2. Lock Acquisition Race Condition (install.sh:223-232)

Location: acquire_lock() function

Issue: Between mkdir and mv, another process could create LOCK_FILE, causing mv to fail. The error handling removes lock_tmp but doesn't check if the directory is empty before rmdir.

Impact: Low - unlikely race condition, but current cleanup could fail if pid file exists.

3. Missing Symlink Target Validation on Removal (install.sh:1070)

Location: remove_agent_symlinks() function

Issue: Uses substring matching which could have false positives.

Impact: Low - edge case but could accidentally remove user symlinks.

Low Priority (Nice-to-have)

4. Windows WSL Documentation - README mentions Windows requires WSL or Developer Mode, but doesn't provide setup instructions.
5. Interactive Menu Accessibility - The multi-select menu doesn't announce selected count.
6. Lock File Location - Using .plaited/.install.lock could collide with user files.

Security Analysis

Strong Security Practices

• Path traversal prevention
• Command injection prevention
• Circular symlink detection
• Scope validation for skill naming
• Atomic lock file operations
• Safe file size limits (100KB max)

No Critical Security Issues Found

Performance Considerations

Optimizations

1. Project name caching
2. Sparse git checkout
3. Atomic directory moves instead of copies

Potential Improvements

1. Parallel skill installation
2. Symlink creation batching

Test Coverage

Well Covered

• Symlink path calculation
• Multi-agent scenarios
• Headless mode detection
• CLI argument parsing
• Edge cases

Could Add

• Lock acquisition/release scenarios
• Concurrent installation simulation
• Symlink circular reference detection tests

Final Recommendations

Must Fix Before Merge

None - All issues are minor or nice-to-have improvements.

Should Fix Soon

1. Align test depth calculation with production (Issue 1)
2. Improve lock cleanup robustness (Issue 2)

Consider for Future

1. Symlink removal validation enhancement
2. Windows setup documentation
3. Interactive menu improvements
4. Parallel installation for performance

PR Metrics

• Lines Changed: +925 / -158
• Files Changed: 3
• Test Coverage: Comprehensive (189 lines of new tests)
• Commits: 7

Conclusion

This is an excellent PR that significantly improves the installer architecture and UX. The code demonstrates strong security awareness, thoughtful error handling, comprehensive testing, and clear documentation.

The identified issues are all minor and don't block merging. The symlink-based centralized storage is a smart architectural choice that will scale well.

Recommendation: APPROVE AND MERGE ✅

Great work! The attention to detail in security, error handling, and user experience is commendable.

[EdwardIrby]

Medium Priority Fixes (4678ad7)

1. Symlink Depth Calculation Inconsistency

Updated test helper to use same awk-based logic as production code:

depth=$(echo "$agent_skills_dir" | awk -F'/' '{n=0; for(i=1;i<=NF;i++) if($i!="") n++; print n}')

This ensures test and production depth calculations always match.

2. Lock Acquisition Race Condition

Changed cleanup from rmdir to rm -rf since the temp directory contains the pid file. This prevents race conditions during lock release.

3. Symlink Target Validation on Removal

Replaced regex matching (=~) with strict pattern matching (== with glob) for safer validation:

# Before (regex - could have edge cases)
if [[ "$target" =~ ^(\.\./)*\.plaited/skills/ ]]; then

# After (strict glob patterns)
if [[ "$target" == "../"*".plaited/skills/"* ]] || [[ "$target" == ".plaited/skills/"* ]]; then

All 130 tests passing.

[claude]

PR Review: Feat/tty update - Symlink-based Multi-Agent Installation

Overview

This PR introduces a significant architectural improvement by implementing a central storage system with symlinks to support multiple agents efficiently. The implementation is well-thought-out with good safety features and comprehensive testing. Below is my detailed review.

---

✅ Strengths

Architecture & Design

• Excellent design decision: Central storage (.plaited/skills/) with symlinks eliminates duplication and ensures consistency across agents
• Strong safety features: Lock file mechanism, circular symlink validation, and target existence checks
• Good UX: Interactive multi-select menu with keyboard shortcuts (arrows, vim keys, number toggles)
• Headless mode support: Proper TTY detection with auto-detection fallback

Code Quality

• Comprehensive testing: Added 192 lines of tests covering symlink logic, multi-agent scenarios, and headless mode
• Good error handling: Symlink failures are tracked and reported at the end
• Documentation: README clearly explains the new architecture and requirements (Windows/WSL)

---

🔍 Potential Issues & Suggestions

Medium Priority

1. Race Condition in Lock Acquisition (install.sh:251-259)

The stale lock cleanup logic has a potential race condition:

rm -rf "$LOCK_FILE" 2>/dev/null
# Another process could acquire lock here
if mkdir "$lock_tmp" 2>/dev/null; then

Recommendation: After removing stale lock, use a retry loop with backoff or accept that another process might win.

2. Symlink Validation Edge Case (install.sh:171)

The circular symlink validation could fail if intermediate directories don't exist:

target_abs=$(cd "$(dirname "$target_abs")" 2>/dev/null && pwd -P)/$(basename "$target_abs")

If dirname "$target_abs" doesn't exist, this produces an incorrect path.
Recommendation: Check if the resolved path is empty and handle gracefully.

3. Glob Pattern Vulnerability (install.sh:950, 981, 1056)

Using unquoted glob patterns in loops:

for skill_folder in "$CENTRAL_SKILLS_DIR"/*; do

While generally safe, if CENTRAL_SKILLS_DIR contains spaces or special characters, this could misbehave.
Recommendation: Already properly quoted, but consider adding validation that CENTRAL_SKILLS_DIR doesn't contain special characters at initialization.

Low Priority

4. Magic Number Documentation (install.sh:786)

Good comment explaining the cursor positioning calculation, but could be clearer:

# Lines: 1 (title) + 1 (empty) + 3 (table header) + num_agents + 1 (table footer) = num_agents + 6
# But we start printing from title, so move up num_agents + 5 lines

Why num_agents + 5? Should be num_agents + 6 - 1 = num_agents + 5 (the title line we're about to overwrite). Consider clarifying.

5. Incomplete Error Context (install.sh:1030)

When symlink creation fails, the error message could be more helpful:

print_error "    Failed to link: $skill_name"
print_error "      $ln_error"

Recommendation: Include the target path in error message for debugging: "Failed to link $skill_name -> $relative_target"

6. Inconsistent Validation (install.sh:1070)

The regex validation for scope matching uses =~ which is fine, but the pattern could be more precise:

if [ -n "$scope" ] && [[ ! "$skill_name" =~ @${scope}$ ]]; then

Recommendation: Consider using exact string matching with == and glob patterns for consistency with the security-focused pattern matching in remove_agent_symlinks (line 1082).

---

🔒 Security Concerns

✅ Good Security Practices

1. Atomic lock creation using mkdir (install.sh:232)
2. Strict pattern matching for symlink removal (install.sh:1082) - using glob patterns instead of regex
3. Circular symlink protection (install.sh:151-184)
4. Target existence validation before symlink creation (install.sh:1011)
5. Stale lock detection with PID validation (install.sh:247)

Minor Improvements

7. Symlink Target Validation

The pattern matching for symlink removal is good but could be even stricter:

if [[ "$target" == "../"*".plaited/skills/"* ]] || [[ "$target" == ".plaited/skills/"* ]]; then

Consider: Validate that the symlink target resolves to an actual path within the central storage using readlink -f or similar to prevent removal of legitimate symlinks that happen to match the pattern.

---

⚡ Performance Considerations

8. Multiple Readlink Calls (install.sh:1080)

In remove_agent_symlinks, each symlink is checked individually. For repositories with many skills, this could be slow.
Impact: Minor - shell loops are reasonably fast for typical scale (< 100 skills)
Recommendation: No change needed unless performance issues arise.

9. Interactive Menu Responsiveness

The read -rsn2 -t 0.1 timeout in arrow key detection (install.sh:833) is fast enough for good UX.
Status: ✅ Well tuned

---

🧪 Test Coverage

✅ Excellent Coverage

• Symlink path calculation with various agent directories
• Multi-agent symlink creation
• Headless mode detection
• Breaking change validation (--agent vs --agents)
• Integration tests for symlink creation

Suggestions for Additional Tests

1. Circular symlink prevention: Test that validate_symlink_not_circular actually prevents circular references
2. Lock contention: Test concurrent installation attempts (challenging in bash tests)
3. Conflict resolution: Test all three conflict resolution options (replace/skip/abort)
4. Stale lock cleanup: Test that stale locks are properly detected and removed

---

📝 Documentation

✅ Well Documented

• Clear README with Windows requirements
• Architecture diagram showing symlink structure
• Warning about editing symlinked files affecting all agents
• Updated examples using --agents (plural)

Minor Improvements

• Consider adding troubleshooting section for common symlink issues on Windows
• Document the lock file mechanism for advanced users who might need to manually intervene

---

🎯 Breaking Changes

The PR handles breaking changes well:

• Changed from --agent to --agents ✅
• Clear error message when using old --agent flag ✅
• Updated all documentation and examples ✅
• Removed --skills-dir override (simplified the interface) ✅

---

Overall Assessment

This is a high-quality PR that introduces a valuable feature with strong implementation:

• ✅ Architecture is sound and well-tested
• ✅ Security considerations are properly addressed
• ✅ UX improvements (multi-select, headless mode) are well implemented
• ⚠️ Minor race condition and edge cases to consider
• ✅ Comprehensive test coverage
• ✅ Good documentation

Recommendation: Approve with minor suggestions

The identified issues are mostly edge cases and minor improvements. The core implementation is solid and the benefits (space savings, consistency, multi-agent support) significantly outweigh the minor concerns.

---

Nitpicks

1. Consider consistent use of local for all function variables (some functions mix styles)
2. The comment at install.sh:290 "# Security: Maximum file size..." seems disconnected from the lock acquisition code above it
3. install.spec.ts:1232 - The comment says "Uses same awk logic as production code for consistency" which is excellent - this is a good pattern to follow

---

Great work on this feature! The symlink-based architecture is a significant improvement. 🚀

[EdwardIrby]

All Review Items Addressed (c764a5c)

Medium Priority

1. Race Condition in Lock Acquisition (lines 251-259)
Added retry loop with backoff after removing stale lock:

for retry in 1 2 3; do
  if mkdir "$lock_tmp" 2>/dev/null; then
    # ... attempt lock
    sleep "0.$retry"  # Backoff on failure
  else
    break  # Another process won
  fi
done

2. Symlink Validation Edge Case (line 171)
Added check for failed directory resolution:

target_dir_resolved=$(cd "$(dirname "$target_abs")" 2>/dev/null && pwd -P)
if [ -z "$target_dir_resolved" ]; then
  return 0  # Can't determine circular, assume safe
fi

3. Glob Pattern Vulnerability
Added validate_central_dir() function that runs at script load time:

• Rejects paths with spaces, special chars, or path traversal
• Only allows alphanumeric, dots, hyphens, underscores, forward slashes

Low Priority

4. Magic Number Documentation (line 786)
Enhanced comment explaining cursor positioning:

# Total lines rendered:
#   1 (title "Select target agents...")
#   1 (empty line)
#   3 (table header: top border, header row, separator)
#   num_agents (one row per agent)
#   1 (table footer: bottom border)
# = num_agents + 6 lines total
# We move up (num_agents + 5) because cursor is already on line after last print

5. Incomplete Error Context (line 1030)
Error now includes full context:

print_error "    Failed to link: $skill_name -> $relative_target"
print_error "      Path: $symlink_path"
print_error "      Error: $ln_error"

6. Inconsistent Validation (line 1070)
Changed from regex to glob pattern matching:

# Before
if [ -n "$scope" ] && [[ ! "$skill_name" =~ @${scope}$ ]]; then
# After
if [ -n "$scope" ] && [[ ! "$skill_name" == *"@${scope}" ]]; then

Security

7. Symlink Target Validation
Added verification that symlink target actually resolves to path within central storage:

local resolved_target
resolved_target=$(cd "$(dirname "$symlink")" && cd "$(dirname "$target")" && pwd -P)
local central_abs
central_abs=$(cd "$CENTRAL_SKILLS_DIR" && pwd -P)
# Only remove if resolved path is within central storage
if [[ "$resolved_target" == "$central_abs"* ]]; then
  rm -f "$symlink"
fi

All 130 tests passing.

[claude]

Pull Request Review: feat/tty-update

Summary

This PR introduces a significant architectural change to the skills installer, moving from a single-agent installation model to a multi-agent model with centralized skill storage using symlinks. The implementation is comprehensive and well-thought-out.

Overall Assessment

Strong points:

• Well-architected solution that solves real disk space and maintenance issues
• Excellent error handling and edge case coverage
• Comprehensive test coverage for new functionality
• Good security practices (lock files, path validation, circular symlink detection)

Areas for improvement:

• Some complexity that could be simplified
• A few potential bugs in edge cases
• Performance considerations for large installations

---

Detailed Feedback

Code Quality & Best Practices

Strengths

1. Excellent validation and security: The path validation, circular symlink detection, and lock file mechanism are well-implemented.
2. Good user experience: The interactive multi-select menu with arrow keys, vim-style navigation, and number shortcuts is excellent.
3. Comprehensive error handling: The handle_existing_skill function properly handles conflicts.
4. Test coverage: The new tests for symlink functions and integration scenarios are thorough.

Potential Bugs

1. Lock File Race Condition (Minor)

Location: install.sh:256-275

Writing the PID after creating the directory creates a window where the lock exists but has no PID file.
Recommendation: Consider using mktemp -d or documenting this edge case.

2. Stale Lock Detection Could Fail

Location: install.sh:280-281

kill -0 may not work correctly in containers or when processes belong to different users.
Recommendation: Add timestamp-based stale lock detection as a fallback.

3. Menu Rendering Edge Case

Location: install.sh:469-476

The terminal escape sequence calculation may cause visual glitches if errors are displayed.
Recommendation: Use tput for more robust terminal manipulation.

4. Symlink TOCTOU

Location: install.sh:718-720

Target existence check before symlink creation has a race condition window.
Recommendation: Document this edge case or add post-creation validation.

---

Performance Considerations

1. Redundant Directory Traversals

Location: install.sh:657-664

Skills are counted in one loop, then traversed again. For large installations, this is inefficient.

2. Nested Loops

Location: install.sh:672-742

O(N*M) complexity for agents and skills. With 10 agents and 100 skills, that's 1000 operations.

---

Security

Good: Path validation and symlink security are comprehensive.
Recommendation: Add warning/log when skipping non-matching symlinks during removal.

---

Test Coverage

Strengths: Comprehensive integration tests, good edge case handling.

Gaps:

1. Missing tests for lock file mechanism (concurrent installs, stale lock cleanup)
2. Missing tests for circular symlink prevention

---

Documentation

Strengths: Excellent README updates with clear examples.

Suggestions:

1. Add troubleshooting section (Windows symlink issues, recovery procedures)
2. Document lock file mechanism in comments

---

Style

Minor issues: Some inconsistent quoting, some long lines exceed 100 characters.

---

Conclusion

This is a well-executed PR that significantly improves the installer. The multi-agent support with symlinks is the right architectural choice.

Priority Recommendations

High Priority:

1. Add timeout-based stale lock detection
2. Add tests for lock file mechanism
3. Add tests for circular symlink prevention

Medium Priority:
4. Extract ask_agents_multiselect into smaller functions
5. Optimize redundant directory traversals
6. Add troubleshooting section to README

Low Priority:
7. Use named constants for magic numbers
8. Consider tput for terminal handling

Approval Status

Overall: ✅ Approve with minor suggestions

The code is production-ready. The suggested improvements are optimizations and hardening for edge cases, not blockers. The PR demonstrates strong engineering practices.

Great work! 🎉

---

Review generated by Claude Code