Skip to content

Show the path from where the skill is loaded#2869

Merged
rumpl merged 1 commit into
docker:mainfrom
rumpl:skills-path
May 22, 2026
Merged

Show the path from where the skill is loaded#2869
rumpl merged 1 commit into
docker:mainfrom
rumpl:skills-path

Conversation

@rumpl
Copy link
Copy Markdown
Member

@rumpl rumpl commented May 21, 2026

Screenshot 2026-05-22 at 01 53 44

@rumpl rumpl requested a review from a team as a code owner May 21, 2026 23:53
Copy link
Copy Markdown

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This PR introduces a clean pkg/path package (RelativeTo, ShortenHome, ShortenHomeDir, IsWithin) to replace ad-hoc path-shortening logic, and plumbs the skill load path through the skills dialog. The logic is correct: IsWithin properly guards against prefix-spoofing (e.g. /home/user2 wrongly matching /home/user), and ShortenHomeDir handles edge cases like the exact home-directory match. No high or medium severity issues found. Two minor observations are left as inline comments.

Comment thread pkg/path/display.go

// ShortenHome replaces the current user's home directory prefix with "~".
func ShortenHome(p string) string {
homeDir, err := os.UserHomeDir()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] ShortenHome calls os.UserHomeDir() on every invocation

ShortenHome calls os.UserHomeDir() directly on each call, which involves an environment variable lookup or syscall each time. The previous ShortenPath helper used the cached result from paths.GetHomeDir() (an atomic pointer). Since this function is called from multiple TUI rendering paths (directorytree, editfile, listdirectory, readfile, readmultiplefiles, searchfilescontent) which may be invoked on every render frame, this is a minor performance regression. Consider caching the home dir at package init or accepting it as a parameter (as ShortenHomeDir already does).

Comment thread pkg/tui/dialog/skills.go
return pathx.ShortenHome(path)
}
cwd, _ := os.Getwd()
return pathx.RelativeTo(path, cwd)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] skillLoadedFrom: RelativeTo may return a verbatim non-absolute path

When s.Local == true, skills.IsHomeSkillPath(path) is false (project-local skill), and path is not absolute, pathx.RelativeTo(path, cwd) returns the path cleaned but verbatim (since RelativeTo only relativizes when both paths are absolute). In production, loadSkillFile always produces an absolute BaseDir via filepath.Dir(path), so this edge case shouldn't arise. However, there's no guard/assertion enforcing this invariant. A defensive check (if !filepath.IsAbs(path)) could make the assumption explicit and prevent a confusing display string if the invariant is ever violated.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@rumpl rumpl merged commit fdee450 into docker:main May 22, 2026
5 checks passed
@aheritier aheritier added area/skills Skills system and custom slash commands area/tui For features/issues/fixes related to the TUI kind/fix PR fixes a bug (maps to fix: commit prefix) labels May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/skills Skills system and custom slash commands area/tui For features/issues/fixes related to the TUI kind/fix PR fixes a bug (maps to fix: commit prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants