fix: address issue #1266 follow-ups from git-native transport review

[wfscot]

Summary

Addresses all 11 items from #1266 (follow-up improvements from the git-native transport PR #1249), plus fixes from code and security review.

Refactoring:

• Extract shared helpers from fetchSource and fetchSourceViaGit to eliminate ~150 lines of duplication (cleanPreviousCuratedSkills, shouldSkipSkill, writeSkillAndComputeIntegrity, buildLockUpdate)
• Deduplicate findControlCharacter/hasControlCharacters into src/utils/validation.ts
• Use path.relative() instead of manual substring in walkDirectory
• Use const for non-reassigned lock destructuring

Security hardening:

• Validate skillsPath in fetchSkillFiles (path traversal, absolute paths, control characters)
• Validate ref returned by resolveDefaultRef to prevent argument injection
• Anchor SCP-style URL regex to prevent trailing junk bypass
• Add total file count (10,000) and size (100MB) limits to walkDirectory
• Validate resolvedRef in lockfile schema as 40-character hex SHA
• Use segment-based path traversal check instead of substring match

Observability:

• Add GitClientError hints in error handler for troubleshooting
• Add descriptions to source entry fields in JSON schema

Tests:

• 7 new git transport tests in sources.test.ts (frozen mode, SHA cache-hit, skill filter, local precedence, duplicate detection, integrity mismatch, GitClientError handling)
• 3 new skillsPath validation tests in git-client.test.ts
• 10 new unit tests for src/utils/validation.ts

Test plan

• pnpm cicheck passes (4262 tests, format, lint, types, spelling, secrets)
• Code review subagent: approved
• Security review subagent: approved, no issues medium or above

Closes #1266

[github-actions]

PR #1277 Review Summary

Overall Mergeability Verdict: MERGEABLE ✅

---

Code Review Findings

No critical or high-severity issues found. All findings are low severity or informational:

1. [LOW] Off-by-one error message wording - Error says "exceeds max file count of 10,000" but throws at exactly 10,000 (src/lib/git-client.ts:218-226)
2. [LOW] Missing tests for total file/size limits - New walk limits lack explicit test coverage (src/lib/git-client.test.ts)
3. [LOW] GitClientError hints could be more specific - Only checks for "not installed", could add network/auth-specific hints (src/lib/sources.ts:182-188)
4. [INFO] Good refactoring following DRY - Extraction of shared helpers eliminates ~150 lines duplication ✅
5. [INFO] Good validation utility extraction - findControlCharacter/hasControlCharacters properly extracted and tested ✅
6. [INFO] Comprehensive security hardening - Multiple layers of defense added ✅
7. [LOW] Windows path separator edge case - Defensive coding, not a bug (src/lib/git-client.ts:135)
8. [LOW] Missing test for resolvedRef validation - Lockfile schema validation lacks negative test case (src/lib/sources-lock.test.ts)
9. [INFO] Comprehensive test coverage - 20 new tests covering all major scenarios ✅

Code Review Verdict: APPROVED with minor suggestions

---

Security Review Findings

1 medium and 4 low severity issues found (none exploitable):

1. [MEDIUM] SSH URL option injection potential - Defense-in-depth gap: ssh://-oProxyCommand=evil passes regex, though execFileAsync and -- separator prevent exploitation (src/lib/git-client.ts:24)
2. [LOW] Off-by-one in DoS limit enforcement - Allows 10,001 files instead of 10,000 (src/lib/git-client.ts:216-227)
3. [LOW] URL regex allows special characters - Characters like ;, $ pass but safe with execFileAsync (src/lib/git-client.ts:24)
4. [LOW] Path traversal check platform dependency - isAbsolute() is platform-specific (src/lib/git-client.ts:135)
5. [LOW] Lockfile integrity hash not validated - Malformed hashes cause warnings but not exploitation (src/lib/sources-lock.ts:96-121)

Positive Security Observations:

• Excellent command injection prevention with execFileAsync and -- separators
• Comprehensive control character filtering
• Proper ref validation prevents option injection
• Multiple layers of path traversal defense
• Appropriate DoS limits (depth 20, files 10K, size 100MB)
• Cryptographic integrity with SHA-256

Security Review Verdict: APPROVED - medium issue is defense-in-depth, not exploitable

---

Conclusion

PR #1277 is ready to merge. All issues are low severity or defense-in-depth recommendations. The refactoring is well-executed, security hardening is thorough, and test coverage is comprehensive.

github run