Add gitresolver package for git:// skill references by JAORMX · Pull Request #4051 · stacklok/toolhive

JAORMX · 2026-03-09T12:55:26Z

Summary

The upcoming git-based skill install feature (thv skill install git://...) needs a package to parse git references, clone repos, resolve auth, and securely write extracted files. This PR adds that foundation as a self-contained package without wiring it into the skill service yet.
New pkg/skills/gitresolver/ package with four components: URL parsing with SSRF prevention, clone resolution with timeout, host-scoped credential resolution, and secure file writing with symlink/containment checks.
Adds HeadCommitHash helper to pkg/git/ for retrieving the HEAD commit hash after clone (used by the resolver to record provenance).

Relates to #4015

Type of change

New feature

Large PR Justification

This PR is 1268 lines but ~655 lines (over half) are unit tests. The remaining ~613 lines are a single self-contained package (pkg/skills/gitresolver/) with four tightly coupled components (URL parsing, clone resolution, auth, file writing) that share types and validation logic. Splitting these into separate PRs would create packages that don't compile independently and add review overhead without reducing risk. The 14-line HeadCommitHash addition to pkg/git/ is the only change outside the new package.

Test plan

Unit tests (task test)
Linting (task lint-fix)

Changes

File	Change
`pkg/skills/gitresolver/reference.go`	New — `git://` URL parsing with SSRF prevention (private IP/localhost rejection, absolute path/backslash rejection)
`pkg/skills/gitresolver/resolver.go`	New — `Resolver` interface: clones repo, extracts skill files, enforces 2-min clone timeout
`pkg/skills/gitresolver/auth.go`	New — host-scoped auth resolution (`GITHUB_TOKEN` only sent to github.com, etc.)
`pkg/skills/gitresolver/writer.go`	New — writes files to disk with symlink checks, containment validation, permission cap (0644)
`pkg/git/client.go`	Modified — added `HeadCommitHash` helper function
`*_test.go`	Comprehensive tests for all four components

Does this introduce a user-facing change?

No — this is a foundation package with no integration yet. The next PR will wire it into skillsvc.

Special notes for reviewers

Security design

Credential exfiltration prevented: Tokens are host-scoped per-clone — GITHUB_TOKEN is only sent to github.com, GITLAB_TOKEN only to gitlab.com. GIT_TOKEN is an opt-in fallback for other hosts.
Clone timeout: 2-minute context.WithTimeout in Resolve() prevents slowloris-style hangs.
Path validation hardened: validateSkillPath rejects absolute paths and backslashes in addition to .. traversal.
SSRF: Static host validation rejects localhost/private IPs.
Resource limits: LimitedFs caps clones at 10k files / 100MB.

Known limitations for follow-up

DNS rebinding not mitigated (needs custom http.Transport with DialContext hook)
No host allowlist (denylist only)
collectFiles reads only top-level files in the skill directory (flat structure)

This is PR 2 of 3 — split from #4042. PR 1 (#4045, merged) moved the git package. PR 3 will integrate gitresolver into skillsvc.

Generated with Claude Code

github-actions

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

This review will be automatically dismissed once you add the justification section.

codecov · 2026-03-09T13:04:20Z

Codecov Report

❌ Patch coverage is 80.16194% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.68%. Comparing base (42f1356) to head (5bd3e3d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/skills/gitresolver/resolver.go	76.28%	14 Missing and 9 partials ⚠️
pkg/skills/gitresolver/reference.go	92.20%	3 Missing and 3 partials ⚠️
pkg/skills/gitresolver/writer.go	64.70%	3 Missing and 3 partials ⚠️
pkg/fileutils/contained.go	63.63%	2 Missing and 2 partials ⚠️
pkg/skills/gitresolver/auth.go	86.20%	3 Missing and 1 partial ⚠️
pkg/skills/installer.go	42.85%	1 Missing and 3 partials ⚠️
pkg/git/client.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4051      +/-   ##
==========================================
+ Coverage   68.62%   68.68%   +0.06%     
==========================================
  Files         448      453       +5     
  Lines       45776    46008     +232     
==========================================
+ Hits        31412    31600     +188     
- Misses      11946    11968      +22     
- Partials     2418     2440      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

addressed

aponcedeleonch

Review of the gitresolver package. Nice self-contained foundation — the interface design and functional options are clean. A few issues to address before merging, ranging from a functional bug to code duplication and test coverage.

See inline comments below.

pkg/skills/gitresolver/resolver.go

pkg/skills/gitresolver/writer.go

pkg/skills/gitresolver/reference.go

pkg/skills/gitresolver/resolver_test.go

pkg/skills/gitresolver/resolver.go

pkg/git/client.go

pkg/skills/gitresolver/writer.go

pkg/skills/gitresolver/auth.go

Fixes all review comments from PR #4051: - BUG-1: Detect semver-like refs (v1.0.0) and use Tag field instead of Branch to fix tag-based clones - DUP-1/DUP-2: Reuse exported ValidatePathNoSymlinks, DirPermissions, and FilePermissionMask from pkg/skills/installer.go - DUP-3: Reuse pkg/networking.IsPrivateIP and IsLocalhost for SSRF validation instead of reimplementing - NEW-1: Move HeadCommitHash to Client interface for mockability - NEW-3: Remove redundant filepath.Clean in writer.go - NEW-5: Add debug log when fallback GIT_TOKEN is used for non-standard hosts - NEW-6: Replace network-dependent context cancellation test with a blocking mock client - MINOR-1: Fix isHex("") returning true - Coverage: Add tests for tag refs, commit refs, branch refs, semver detection, and isHex edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

pkg/skills/installer.go

Fixes all review comments from PR #4051: - BUG-1: Detect semver-like refs (v1.0.0) and use Tag field instead of Branch to fix tag-based clones - DUP-1/DUP-2: Reuse exported ValidatePathNoSymlinks, DirPermissions, and FilePermissionMask from pkg/skills/installer.go - DUP-3: Reuse pkg/networking.IsPrivateIP and IsLocalhost for SSRF validation instead of reimplementing - NEW-1: Move HeadCommitHash to Client interface for mockability - NEW-3: Remove redundant filepath.Clean in writer.go - NEW-5: Add debug log when fallback GIT_TOKEN is used for non-standard hosts - NEW-6: Replace network-dependent context cancellation test with a blocking mock client - MINOR-1: Fix isHex("") returning true - Coverage: Add tests for tag refs, commit refs, branch refs, semver detection, and isHex edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

aponcedeleonch

Most previous comments are addressed — nice work. Constants and ValidatePathNoSymlinks are now shared, and the tag/SSRF/auth/test issues are fixed.

One remaining concern: the file-writing loop in writer.go and installer.go still uses raw os.WriteFile without atomic writes or file locking. The codebase already has fileutils.AtomicWriteFile and fileutils.WithFileLock used by config_editor.go, encrypted.go, and file_status.go. Both skill installers should adopt the same pattern. See inline comment for a suggested consolidation.

pkg/skills/gitresolver/writer.go

aponcedeleonch

approved with the above suggestion

Introduces pkg/skills/gitresolver with URL parsing, clone resolution, host-scoped auth, and secure file writing for git-based skill install. This is the foundation package — skillsvc integration follows in the next PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fixes all review comments from PR #4051: - BUG-1: Detect semver-like refs (v1.0.0) and use Tag field instead of Branch to fix tag-based clones - DUP-1/DUP-2: Reuse exported ValidatePathNoSymlinks, DirPermissions, and FilePermissionMask from pkg/skills/installer.go - DUP-3: Reuse pkg/networking.IsPrivateIP and IsLocalhost for SSRF validation instead of reimplementing - NEW-1: Move HeadCommitHash to Client interface for mockability - NEW-3: Remove redundant filepath.Clean in writer.go - NEW-5: Add debug log when fallback GIT_TOKEN is used for non-standard hosts - NEW-6: Replace network-dependent context cancellation test with a blocking mock client - MINOR-1: Fix isHex("") returning true - Coverage: Add tests for tag refs, commit refs, branch refs, semver detection, and isHex edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Anchor semver regex and require at least one dot (v1.0, not v1) to prevent misclassifying branch names like v1-beta-branch as tags - Document DNS rebinding limitation in validateHost - Add HeadCommitHash unit tests for nil inputs and valid repos - Clarify collectFiles vs WriteFiles directory handling consistency - Add regression tests for branch-like refs with v-prefix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The containment-check + write loop in gitresolver/writer.go was near-identical to installer.go. Extract a shared WriteContainedFile helper in pkg/fileutils that performs path containment validation, parent directory creation, and atomic file writes via AtomicWriteFile. Both callers now delegate to this helper, and gitresolver/WriteFiles wraps its operation in WithFileLock to prevent concurrent installs from corrupting each other. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

aponcedeleonch

nice!

github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Mar 9, 2026

github-actions bot previously requested changes Mar 9, 2026

View reviewed changes

JAORMX requested a review from aponcedeleonch March 9, 2026 13:20

aponcedeleonch requested changes Mar 9, 2026

View reviewed changes

JAORMX requested review from ChrisJBurns, amirejaz, blkt, jhrozek and yrobla as code owners March 10, 2026 06:57

github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 10, 2026

github-advanced-security bot found potential problems Mar 10, 2026

View reviewed changes

pkg/skills/installer.go Dismissed Show dismissed Hide dismissed

github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 10, 2026

JAORMX force-pushed the feat/skill-install-git-refs-v3 branch from f37e048 to 646f27f Compare March 10, 2026 09:18

github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 10, 2026

aponcedeleonch requested changes Mar 11, 2026

View reviewed changes

pkg/skills/gitresolver/writer.go Show resolved Hide resolved

aponcedeleonch previously approved these changes Mar 11, 2026

View reviewed changes

JAORMX and others added 3 commits March 11, 2026 14:09

JAORMX force-pushed the feat/skill-install-git-refs-v3 branch from f7ab6ed to 03d5dda Compare March 11, 2026 12:10

github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 11, 2026

JAORMX dismissed aponcedeleonch’s stale review via 5bd3e3d March 11, 2026 12:21

github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 11, 2026

aponcedeleonch approved these changes Mar 11, 2026

View reviewed changes

JAORMX merged commit 37ce6a6 into main Mar 11, 2026
44 of 45 checks passed

JAORMX deleted the feat/skill-install-git-refs-v3 branch March 11, 2026 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gitresolver package for git:// skill references#4051

Add gitresolver package for git:// skill references#4051
JAORMX merged 4 commits intomainfrom
feat/skill-install-git-refs-v3

JAORMX commented Mar 9, 2026 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

codecov bot commented Mar 9, 2026 •

edited

Loading

Uh oh!

aponcedeleonch left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aponcedeleonch left a comment

Uh oh!

Uh oh!

aponcedeleonch left a comment

Uh oh!

aponcedeleonch left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JAORMX commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Large PR Justification

Test plan

Changes

Does this introduce a user-facing change?

Special notes for reviewers

Security design

Known limitations for follow-up

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Large PR Detected

How to unblock this PR:

Alternative:

Uh oh!

codecov bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JAORMX commented Mar 9, 2026 •

edited

Loading

codecov bot commented Mar 9, 2026 •

edited

Loading