feat: add digdir-cli for provisioning agent skills#3
Open
olebhansen wants to merge 3 commits intomainfrom
Open
feat: add digdir-cli for provisioning agent skills#3olebhansen wants to merge 3 commits intomainfrom
olebhansen wants to merge 3 commits intomainfrom
Conversation
Interactive TUI and CLI tool that installs skills from digdir/skills into your local setup. Supports four agent frameworks (Claude Code, Cursor, Copilot, Codex) with framework-specific adapters that handle format conversion. Skills can be installed globally or to project repos. Built in Go with only stdlib + golang.org/x/term (no charmbracelet deps). Includes CI workflow and multi-platform release workflow (linux, macOS, windows; amd64 + arm64). Closes #1 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tags containing -rc, -alpha, or -beta are automatically marked as pre-release on GitHub Releases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- `digdir-cli --version` / `digdir-cli version` shows current version - `digdir-cli update` checks GitHub Releases and replaces the binary - Version injected via ldflags at build time (defaults to "dev") - Handles Windows binary replacement (rename-swap pattern) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a new Go CLI tool ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
cli/internal/adapter/adapter.go (1)
53-67: Document nil return behavior in the function comment.The
Get()function returnsnilfor unknown frameworks. While callers (likecmdInstallinmain.go) do check for this, the function comment should explicitly document this behavior to prevent future misuse.📝 Suggested documentation improvement
-// Get returns the adapter for the given framework. +// Get returns the adapter for the given framework, or nil if the framework is not supported. func Get(f Framework) Adapter {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/adapter/adapter.go` around lines 53 - 67, Update the comment for the Get function to explicitly state its nil-return behavior for unknown frameworks: describe that Get(f Framework) returns a framework-specific Adapter (e.g., *claudeCodeAdapter, *cursorAdapter, etc.) and that it will return nil when the provided Framework value is not recognized, so callers must check for nil before using the returned Adapter (see usages like cmdInstall). Modify the comment above Get to include this contract and any expected caller responsibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 3-8: The CI job is skipped for PRs that only modify workflow files
because the 'on:' triggers for push and pull_request include restrictive 'paths'
arrays; update the 'push:' and 'pull_request:' triggers (the 'on:' block) to
either remove the 'paths' filters entirely or add the workflow pattern (e.g.
include ".github/workflows/**") to the existing 'paths' arrays so changes to
workflow files (ci.yml, release.yml) will run the job; edit the 'paths' entries
under 'push:' and 'pull_request:' in the YAML accordingly.
In @.github/workflows/release.yml:
- Around line 28-30: The release matrix is missing the Windows arm64 target; add
a new matrix entry with goos: windows, goarch: arm64, suffix: ".exe" in the
release workflow so a Windows/ARM64 binary is built and published, and verify
the produced asset name matches the lookup logic in
cli/internal/updater/updater.go (GOOS/GOARCH-based naming) so the updater can
find the Windows ARM64 release.
In `@cli/internal/adapter/claudecode.go`:
- Around line 14-16: The GlobalPath method on claudeCodeAdapter currently
swallows the os.UserHomeDir() error and returns a relative path; change
GlobalPath to propagate the failure instead: update func (a *claudeCodeAdapter)
GlobalPath() string to return (string, error), call os.UserHomeDir() and if it
returns an error return "", wrapped error (with context like "getting user home
for GlobalPath"), and update all callers to handle the error; mirror the same
fix applied to the similar implementation in cursor.go so both adapters
consistently propagate home-dir failures.
In `@cli/internal/adapter/codex.go`:
- Around line 14-17: The GlobalPath implementation in codexAdapter should return
an error when os.UserHomeDir() fails instead of returning a relative path;
change the Adapter.GlobalPath signature to return (string, error) and update
codexAdapter.GlobalPath to capture and return the os.UserHomeDir() error, e.g.,
return "", err if UserHomeDir() fails. Propagate the signature change to the
other implementations (cursorAdapter.GlobalPath, claudecodeAdapter.GlobalPath)
and update all call sites in installer and tui to handle the (string, error)
result (check error, surface or handle it). Ensure all compilation/type changes
across the Adapter interface and its usages are applied consistently.
In `@cli/internal/adapter/copy.go`:
- Around line 32-52: The copyFile function currently uses os.Open and os.Create
which dereferences symlinks and resets file modes; change it to stat the source
with os.Lstat to detect and handle symlinks (skip or recreate as symlink using
os.Readlink + os.Symlink instead of copying target contents), read the source
file mode from the returned FileInfo, and create the destination with
os.OpenFile using that mode (after MkdirAll) rather than os.Create so executable
bits are preserved; update copy logic in copyFile to use os.Lstat,
os.Readlink/os.Symlink for symlinks, and os.OpenFile with the source mode when
copying regular files.
- Around line 55-66: In copyDirRecursive, reject symlinks instead of letting
copyFile follow them: call os.Lstat or inspect info.Mode() from the Walk
callback and if the entry is a symlink (mode&os.ModeSymlink != 0) return a
non-nil error to abort the copy; do not call os.Open on symlinks. Also stop
ignoring the error from filepath.Rel—capture and return it instead of discarding
so path calculation failures propagate. Update references in the Walk closure
(copyDirRecursive, copyFile, filepath.Walk, filepath.Rel) accordingly.
In `@cli/internal/adapter/cursor.go`:
- Around line 65-69: The code writes s.Description raw into YAML frontmatter
(see the b.WriteString(fmt.Sprintf("description: %s\n", s.Description)) line),
which breaks the .mdc when the description contains colons or newlines; change
this to emit a safe YAML block scalar or otherwise escape/serialize the field:
replace the single-line sprintf with output that writes "description: |-" (or
uses a YAML serializer) and then writes each line of s.Description indented
appropriately so newlines and special characters are preserved; update the code
around the description write in cursor.go where s.Description is used so the
generated frontmatter is always valid YAML.
In `@cli/internal/skill/skill.go`:
- Around line 77-80: The code sets Skill.Name directly from frontmatter (in the
assignment inside the if for "name:" in skill.go), which lets a user supply
paths like "../../x" that later get fed into filepath.Join in the adapters; to
fix, stop treating the frontmatter value as a filesystem path: set Skill.Name
(display string) to the raw unquoted value but also compute and store a separate
sanitized install slug (e.g., Skill.Slug or Skill.InstallName) using
filepath.Base(unquote(...)) and additional sanitization (remove path separators,
strip/replace unsafe chars, ensure non-empty with a safe default like "skill").
Update the adapters currently calling filepath.Join(...) with the frontmatter
name to use the new Skill.Slug/InstallName instead of Skill.Name so installs
cannot escape the target directory.
In `@cli/internal/skill/source.go`:
- Around line 91-103: The gitClone and gitPull helpers spawn external git
processes with no timeout; wrap each call in a context with a deadline (e.g.
context.WithTimeout) and use exec.CommandContext instead of exec.Command so the
process is killed when the deadline expires; ensure you call cancel() and return
the command error. Add the required imports (context and time) and keep the same
Stdout/Stderr assignments for gitClone and gitPull so behavior is unchanged
except for the timeout.
In `@cli/internal/tui/app.go`:
- Line 259: The code uses a byte slice buffer (var buf []byte) and byte-based
handling which drops non-ASCII UTF-8 path characters; change the buffer and
related key handling to be rune-aware: replace var buf []byte with var buf
[]rune (or use a strings.Builder) and update readKey() to return a rune (or
string) instead of a single byte, then adjust the append/remove logic in the
input handling (the code around the buf manipulations at the original 259 and
298-301 regions) to push/pop runes and convert the final rune slice to a string
(string(buf) or builder.String()) when constructing the repo path.
In `@cli/internal/updater/updater.go`:
- Around line 39-45: The code compares release.TagName (latest) and
currentVersion for equality only, causing false upgrade/downgrade prompts;
update the logic in the CheckVersion flow to parse both currentVersion and
release.TagName as semantic versions (using the project's semver utility or a
standard semver parser), handle prerelease/build metadata correctly, and perform
a proper version.Compare/CompareMajorMinorPatch result instead of string
equality; use the comparison result to print “already up to date”, “new version
available” or “no update (remote older)”, and apply the same semantic-compare
fix to the other occurrence referenced around lines 178-181 so prereleases and
local builds are handled correctly.
- Around line 78-96: fetchLatestRelease and downloadAndReplace currently call
http.Get with no timeout; create a package-level http.Client (e.g., var
httpClient = &http.Client{Timeout: 30 * time.Second}) and add the time import,
then replace uses of http.Get in fetchLatestRelease and downloadAndReplace with
httpClient.Get (keeping existing error handling and resp.Body.Close logic) so
both update-check and download flows use the 30s timeout.
In `@cli/main.go`:
- Line 92: The cmdInteractive handler currently calls tui.Run(skills) and
ignores its returned installation results; change it to capture both results and
error (e.g., res, err := tui.Run(skills)), keep the existing runtime-error
handling, then iterate the returned results slice (as cmdInstall does) and if
any result r has r.Err != nil call os.Exit(1) (or return a non-zero exit path).
Reference the functions/values tui.Run, cmdInteractive, cmdInstall and the
result field r.Err when locating where to add the results capture, iteration,
and exit-on-failure logic.
---
Nitpick comments:
In `@cli/internal/adapter/adapter.go`:
- Around line 53-67: Update the comment for the Get function to explicitly state
its nil-return behavior for unknown frameworks: describe that Get(f Framework)
returns a framework-specific Adapter (e.g., *claudeCodeAdapter, *cursorAdapter,
etc.) and that it will return nil when the provided Framework value is not
recognized, so callers must check for nil before using the returned Adapter (see
usages like cmdInstall). Modify the comment above Get to include this contract
and any expected caller responsibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08823a8c-65c3-4679-b0c8-049a04e0d179
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/ci.yml.github/workflows/release.yml.gitignorecli/go.modcli/internal/adapter/adapter.gocli/internal/adapter/claudecode.gocli/internal/adapter/codex.gocli/internal/adapter/copilot.gocli/internal/adapter/copy.gocli/internal/adapter/cursor.gocli/internal/installer/installer.gocli/internal/skill/skill.gocli/internal/skill/source.gocli/internal/tui/app.gocli/internal/tui/styles.gocli/internal/tui/term.gocli/internal/updater/updater.gocli/main.go
Comment on lines
+3
to
+8
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: ["cli/**"] | ||
| pull_request: | ||
| paths: ["cli/**"] |
There was a problem hiding this comment.
Let workflow-only PRs run CI.
With the current paths filter, PRs that only change .github/workflows/ci.yml or .github/workflows/release.yml will skip this job, so the pipeline cannot validate updates to itself.
Suggested patch
on:
push:
branches: [main]
- paths: ["cli/**"]
+ paths:
+ - "cli/**"
+ - ".github/workflows/ci.yml"
+ - ".github/workflows/release.yml"
pull_request:
- paths: ["cli/**"]
+ paths:
+ - "cli/**"
+ - ".github/workflows/ci.yml"
+ - ".github/workflows/release.yml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 3 - 8, The CI job is skipped for PRs
that only modify workflow files because the 'on:' triggers for push and
pull_request include restrictive 'paths' arrays; update the 'push:' and
'pull_request:' triggers (the 'on:' block) to either remove the 'paths' filters
entirely or add the workflow pattern (e.g. include ".github/workflows/**") to
the existing 'paths' arrays so changes to workflow files (ci.yml, release.yml)
will run the job; edit the 'paths' entries under 'push:' and 'pull_request:' in
the YAML accordingly.
Comment on lines
+28
to
+30
| - goos: windows | ||
| goarch: amd64 | ||
| suffix: ".exe" |
There was a problem hiding this comment.
The release matrix is still missing windows/arm64.
The PR objective says releases should cover linux/macOS/windows on both amd64 and arm64, but the matrix only builds Windows amd64. That leaves Windows ARM users without a published binary, and cli/internal/updater/updater.go looks up update assets by GOOS/GOARCH, so there is no matching release asset for that platform.
Add the missing target
- goos: windows
goarch: amd64
suffix: ".exe"
+ - goos: windows
+ goarch: arm64
+ suffix: ".exe"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - goos: windows | |
| goarch: amd64 | |
| suffix: ".exe" | |
| - goos: windows | |
| goarch: amd64 | |
| suffix: ".exe" | |
| - goos: windows | |
| goarch: arm64 | |
| suffix: ".exe" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 28 - 30, The release matrix is
missing the Windows arm64 target; add a new matrix entry with goos: windows,
goarch: arm64, suffix: ".exe" in the release workflow so a Windows/ARM64 binary
is built and published, and verify the produced asset name matches the lookup
logic in cli/internal/updater/updater.go (GOOS/GOARCH-based naming) so the
updater can find the Windows ARM64 release.
Comment on lines
+14
to
+16
| func (a *claudeCodeAdapter) GlobalPath() string { | ||
| home, _ := os.UserHomeDir() | ||
| return filepath.Join(home, ".claude", "skills") |
There was a problem hiding this comment.
Don't silently fall back to a relative global path.
If os.UserHomeDir() fails here, this resolves to .claude/skills under the current working directory, so a "global" install can end up writing into the repo or whatever directory the process was launched from. This should propagate a failure instead of ignoring it; the same pattern exists in cli/internal/adapter/cursor.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/adapter/claudecode.go` around lines 14 - 16, The GlobalPath
method on claudeCodeAdapter currently swallows the os.UserHomeDir() error and
returns a relative path; change GlobalPath to propagate the failure instead:
update func (a *claudeCodeAdapter) GlobalPath() string to return (string,
error), call os.UserHomeDir() and if it returns an error return "", wrapped
error (with context like "getting user home for GlobalPath"), and update all
callers to handle the error; mirror the same fix applied to the similar
implementation in cursor.go so both adapters consistently propagate home-dir
failures.
Comment on lines
+14
to
+17
| func (a *codexAdapter) GlobalPath() string { | ||
| home, _ := os.UserHomeDir() | ||
| return filepath.Join(home, ".codex", "skills") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd cli/internal/adapter && pwd && wc -l codex.go && head -30 codex.goRepository: digdir/skills
Length of output: 697
🏁 Script executed:
# Find the adapter interface definition
rg -B5 -A10 "type.*Adapter.*interface" cli/internal/adapter/Repository: digdir/skills
Length of output: 1039
🏁 Script executed:
# Search for all calls to GlobalPath()
rg "GlobalPath\(" --type go -B2 -A2 cli/Repository: digdir/skills
Length of output: 2905
🏁 Script executed:
# Test filepath.Join behavior with empty string
python3 << 'EOF'
import os
# Simulate what happens when os.UserHomeDir fails
result = os.path.join("", ".codex", "skills")
print(f"filepath.Join('', '.codex', 'skills') = '{result}'")
print(f"Is absolute: {os.path.isabs(result)}")
EOFRepository: digdir/skills
Length of output: 131
🏁 Script executed:
# Check the installer usage more closely to understand error handling
rg -B5 -A10 "globalPath := a.GlobalPath\(\)" cli/internal/installer/installer.goRepository: digdir/skills
Length of output: 457
🏁 Script executed:
# Check if UserHomeDir errors are documented or handled elsewhere
rg "UserHomeDir" cli/ -B3 -A3 --type goRepository: digdir/skills
Length of output: 1704
Surface UserHomeDir failures instead of silently falling back to a relative path.
If os.UserHomeDir() fails here, filepath.Join("", ".codex", "skills") resolves to .codex/skills (a relative path), causing global skills to install in the wrong location silently. The adapter interface currently uses an empty string to signal "not supported"; an actual error must be propagated instead. This requires updating the Adapter interface signature to return (string, error), modifying all implementations (codex, cursor, claudecode adapters), and updating call sites in installer and tui modules to handle the error.
Suggested patch
-func (a *codexAdapter) GlobalPath() string {
- home, _ := os.UserHomeDir()
- return filepath.Join(home, ".codex", "skills")
+func (a *codexAdapter) GlobalPath() (string, error) {
+ home, err := os.UserHomeDir()
+ if err != nil {
+ return "", fmt.Errorf("resolving home directory: %w", err)
+ }
+ return filepath.Join(home, ".codex", "skills"), nil
}Also update the Adapter interface signature, all implementations, and call sites to handle the returned error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/adapter/codex.go` around lines 14 - 17, The GlobalPath
implementation in codexAdapter should return an error when os.UserHomeDir()
fails instead of returning a relative path; change the Adapter.GlobalPath
signature to return (string, error) and update codexAdapter.GlobalPath to
capture and return the os.UserHomeDir() error, e.g., return "", err if
UserHomeDir() fails. Propagate the signature change to the other implementations
(cursorAdapter.GlobalPath, claudecodeAdapter.GlobalPath) and update all call
sites in installer and tui to handle the (string, error) result (check error,
surface or handle it). Ensure all compilation/type changes across the Adapter
interface and its usages are applied consistently.
Comment on lines
+32
to
+52
| func copyFile(src, dst string) error { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| return fmt.Errorf("opening %s: %w", src, err) | ||
| } | ||
| defer in.Close() | ||
|
|
||
| if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| out, err := os.Create(dst) | ||
| if err != nil { | ||
| return fmt.Errorf("creating %s: %w", dst, err) | ||
| } | ||
| defer out.Close() | ||
|
|
||
| if _, err := io.Copy(out, in); err != nil { | ||
| return fmt.Errorf("copying to %s: %w", dst, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "copy.go" --type fRepository: digdir/skills
Length of output: 85
🏁 Script executed:
cat -n cli/internal/adapter/copy.goRepository: digdir/skills
Length of output: 2092
🏁 Script executed:
python3 << 'EOF'
# Verify filepath.Walk behavior with symlinks
# filepath.Walk does NOT follow symlinks for directories by default
# However, os.Open will dereference symlink files
# This means if a script/ directory contains a symlink, it will be followed
info = """
filepath.Walk in Go:
- Does NOT follow symlinks to directories
- But for files: os.Open() will follow symlinks
- Symlink files get dereferenced when opened
Implication for copyFile:
- If scripts/program.sh is a symlink to /etc/passwd (outside source tree)
- os.Open(scripts/program.sh) will open /etc/passwd
- Content of /etc/passwd would be copied to destination
- This is a security concern
"""
print(info)
EOFRepository: digdir/skills
Length of output: 453
🏁 Script executed:
python3 << 'EOF'
# Verify os.Create behavior with file modes
import os
import tempfile
import stat
# Create a temporary directory
with tempfile.TemporaryDirectory() as tmpdir:
# Create a file with executable permission
src = os.path.join(tmpdir, "script.sh")
dst = os.path.join(tmpdir, "script_copy.sh")
# Create source file with executable bit
with open(src, "w") as f:
f.write("#!/bin/bash\necho hello")
# Set executable permission
os.chmod(src, 0o755)
src_mode = stat.filemode(os.stat(src).st_mode)
src_mode_perm = os.stat(src).st_mode & 0o777
# Copy using os.Create (as the code does)
with open(src, "rb") as f_in:
with open(dst, "wb") as f_out: # Python's open() is like os.Create
f_out.write(f_in.read())
dst_mode = stat.filemode(os.stat(dst).st_mode)
dst_mode_perm = os.stat(dst).st_mode & 0o777
print(f"Source mode: {oct(src_mode_perm)}")
print(f"Destination mode: {oct(dst_mode_perm)}")
print(f"Executable preserved: {src_mode_perm == dst_mode_perm}")
EOFRepository: digdir/skills
Length of output: 127
Preserve source file modes and handle symlinks safely in copy operations.
os.Create recreates files with default permissions (typically 0o644), so executable files in scripts/ lose their +x bit on Unix and become non-executable after install. Additionally, os.Open dereferences symlinks, so if a symlink in the source tree points outside it, the target file's content—rather than the symlink itself—gets copied to the destination, potentially creating a security exposure.
Retrieve source file mode before creating the destination, use os.OpenFile with the source mode, and ensure symlinks are skipped or copied as symlinks rather than dereferenced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/adapter/copy.go` around lines 32 - 52, The copyFile function
currently uses os.Open and os.Create which dereferences symlinks and resets file
modes; change it to stat the source with os.Lstat to detect and handle symlinks
(skip or recreate as symlink using os.Readlink + os.Symlink instead of copying
target contents), read the source file mode from the returned FileInfo, and
create the destination with os.OpenFile using that mode (after MkdirAll) rather
than os.Create so executable bits are preserved; update copy logic in copyFile
to use os.Lstat, os.Readlink/os.Symlink for symlinks, and os.OpenFile with the
source mode when copying regular files.
Comment on lines
+91
to
+103
| func gitClone(url, dest string) error { | ||
| cmd := exec.Command("git", "clone", "--depth", "1", url, dest) | ||
| cmd.Stdout = os.Stderr | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| func gitPull(repoDir string) error { | ||
| cmd := exec.Command("git", "-C", repoDir, "pull", "--ff-only") | ||
| cmd.Stdout = os.Stderr | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/internal/skill/source.go | sed -n '85,110p'Repository: digdir/skills
Length of output: 660
🏁 Script executed:
head -30 cli/internal/skill/source.goRepository: digdir/skills
Length of output: 749
🏁 Script executed:
rg -n "gitClone|gitPull" cli/internal/skill/Repository: digdir/skills
Length of output: 356
🏁 Script executed:
cat -n cli/internal/skill/source.go | sed -n '40,80p'Repository: digdir/skills
Length of output: 1350
🏁 Script executed:
rg -n "resolveGitHub|resolveLocal" cli/internal/skill/source.goRepository: digdir/skills
Length of output: 221
Put a deadline around git clone / git pull.
Both helpers start long-running external processes with no timeout. A stuck network operation or credential helper can block the CLI indefinitely.
Suggested patch
func gitClone(url, dest string) error {
- cmd := exec.Command("git", "clone", "--depth", "1", url, dest)
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+ defer cancel()
+ cmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", url, dest)
cmd.Stdout = os.Stderr
cmd.Stderr = os.Stderr
return cmd.Run()
}
func gitPull(repoDir string) error {
- cmd := exec.Command("git", "-C", repoDir, "pull", "--ff-only")
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+ defer cancel()
+ cmd := exec.CommandContext(ctx, "git", "-C", repoDir, "pull", "--ff-only")
cmd.Stdout = os.Stderr
cmd.Stderr = os.Stderr
return cmd.Run()
}Also add the context import.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/skill/source.go` around lines 91 - 103, The gitClone and gitPull
helpers spawn external git processes with no timeout; wrap each call in a
context with a deadline (e.g. context.WithTimeout) and use exec.CommandContext
instead of exec.Command so the process is killed when the deadline expires;
ensure you call cancel() and return the command error. Add the required imports
(context and time) and keep the same Stdout/Stderr assignments for gitClone and
gitPull so behavior is unchanged except for the timeout.
| // inputRepoPaths lets the user type in repo paths one at a time. | ||
| func inputRepoPaths() ([]string, bool) { | ||
| var paths []string | ||
| var buf []byte |
There was a problem hiding this comment.
Repo-path input is ASCII-only right now.
This buffer is byte-backed, and cli/internal/tui/term.go only emits printable ASCII, so any UTF-8 path component is dropped or truncated. Please make this input path rune-aware here and in readKey(), otherwise installs fail for repos under non-English usernames/directories.
Also applies to: 298-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/tui/app.go` at line 259, The code uses a byte slice buffer (var
buf []byte) and byte-based handling which drops non-ASCII UTF-8 path characters;
change the buffer and related key handling to be rune-aware: replace var buf
[]byte with var buf []rune (or use a strings.Builder) and update readKey() to
return a rune (or string) instead of a single byte, then adjust the
append/remove logic in the input handling (the code around the buf manipulations
at the original 259 and 298-301 regions) to push/pop runes and convert the final
rune slice to a string (string(buf) or builder.String()) when constructing the
repo path.
Comment on lines
+39
to
+45
| latest := release.TagName | ||
| if latest == currentVersion { | ||
| fmt.Printf("Already up to date (%s)\n", currentVersion) | ||
| return nil | ||
| } | ||
|
|
||
| fmt.Printf("New version available: %s → %s\n", currentVersion, latest) |
There was a problem hiding this comment.
Compare versions semantically before updating.
Any non-equal tag is treated as “newer” here. That means a locally newer build or prerelease can still trigger a downgrade if the server reports an older tag, and CheckVersion will print the same false-positive upgrade hint.
Also applies to: 178-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/updater/updater.go` around lines 39 - 45, The code compares
release.TagName (latest) and currentVersion for equality only, causing false
upgrade/downgrade prompts; update the logic in the CheckVersion flow to parse
both currentVersion and release.TagName as semantic versions (using the
project's semver utility or a standard semver parser), handle prerelease/build
metadata correctly, and perform a proper version.Compare/CompareMajorMinorPatch
result instead of string equality; use the comparison result to print “already
up to date”, “new version available” or “no update (remote older)”, and apply
the same semantic-compare fix to the other occurrence referenced around lines
178-181 so prereleases and local builds are handled correctly.
Comment on lines
+78
to
+96
| func fetchLatestRelease() (*ghRelease, error) { | ||
| resp, err := http.Get(apiURL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode == http.StatusNotFound { | ||
| return nil, fmt.Errorf("no releases found (have you created a release yet?)") | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("GitHub API returned %s", resp.Status) | ||
| } | ||
|
|
||
| var release ghRelease | ||
| if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { | ||
| return nil, fmt.Errorf("parsing response: %w", err) | ||
| } | ||
| return &release, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*cli/internal/updater/updater.go" -type fRepository: digdir/skills
Length of output: 90
🏁 Script executed:
cat -n ./cli/internal/updater/updater.go | sed -n '70,120p'Repository: digdir/skills
Length of output: 1684
🏁 Script executed:
cat -n ./cli/internal/updater/updater.go | head -30Repository: digdir/skills
Length of output: 836
🏁 Script executed:
cat -n ./cli/internal/updater/updater.goRepository: digdir/skills
Length of output: 5985
Use an HTTP client with timeouts for update checks and downloads.
Both fetchLatestRelease() (line 79) and downloadAndReplace() (line 108) call http.Get() directly without timeouts, causing potential indefinite hangs on stalled connections. This affects both the Update flow and the background CheckVersion check.
Create a global HTTP client with a 30-second timeout and use it in both functions. Also add the time import.
Suggested patch
+import (
"encoding/json"
"fmt"
"io"
"net/http"
"os"
"path/filepath"
"runtime"
"strings"
+ "time"
)
const (
repoOwner = "digdir"
repoName = "skills"
apiURL = "https://api.github.com/repos/" + repoOwner + "/" + repoName + "/releases/latest"
)
+
+var httpClient = &http.Client{Timeout: 30 * time.Second}
type ghRelease struct {Then update both functions:
func fetchLatestRelease() (*ghRelease, error) {
- resp, err := http.Get(apiURL)
+ resp, err := httpClient.Get(apiURL) func downloadAndReplace(url, destPath string) error {
- resp, err := http.Get(url)
+ resp, err := httpClient.Get(url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/updater/updater.go` around lines 78 - 96, fetchLatestRelease and
downloadAndReplace currently call http.Get with no timeout; create a
package-level http.Client (e.g., var httpClient = &http.Client{Timeout: 30 *
time.Second}) and add the time import, then replace uses of http.Get in
fetchLatestRelease and downloadAndReplace with httpClient.Get (keeping existing
error handling and resp.Body.Close logic) so both update-check and download
flows use the 30s timeout.
| os.Exit(1) | ||
| } | ||
|
|
||
| _, err = tui.Run(skills) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what tui.Run returns to understand if the discarded value matters
ast-grep --pattern $'func Run($_) ($_, $_) {
$$$
}'Repository: digdir/skills
Length of output: 2720
🏁 Script executed:
#!/bin/bash
# Get cmdInteractive function context around line 92
sed -n '80,105p' cli/main.goRepository: digdir/skills
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Get cmdInstall function to see how it handles installation results
rg -A 20 'func cmdInstall' cli/main.goRepository: digdir/skills
Length of output: 946
🏁 Script executed:
#!/bin/bash
# Get more of cmdInstall to see how it handles results and exit code
rg -A 50 'func cmdInstall' cli/main.go | head -70Repository: digdir/skills
Length of output: 1544
🏁 Script executed:
#!/bin/bash
# Get the rest of cmdInstall to see result handling
rg -A 70 'func cmdInstall' cli/main.go | tail -40Repository: digdir/skills
Length of output: 816
Exit with code 1 if any skill installation fails in interactive mode, like cmdInstall does.
The tui.Run() function returns installation results, but cmdInteractive discards them and only checks for TUI runtime errors. Meanwhile, cmdInstall properly iterates through results and exits with code 1 if any installation has r.Err != nil. Apply the same pattern to cmdInteractive to ensure the CLI reports failure when installations fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/main.go` at line 92, The cmdInteractive handler currently calls
tui.Run(skills) and ignores its returned installation results; change it to
capture both results and error (e.g., res, err := tui.Run(skills)), keep the
existing runtime-error handling, then iterate the returned results slice (as
cmdInstall does) and if any result r has r.Err != nil call os.Exit(1) (or return
a non-zero exit path). Reference the functions/values tui.Run, cmdInteractive,
cmdInstall and the result field r.Err when locating where to add the results
capture, iteration, and exit-on-failure logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
digdir-cli, a Go CLI/TUI tool for provisioning skills from this repo into local agent setupsinstall,list,help)--sourcegolang.org/x/term— no charmbracelet dependenciesCloses #1
Usage
Test plan
go vet ./...passesdigdir-cli list --source .lists both skillsdigdir-cli installverified for Claude Code, Cursor, Copilot adapters🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores