feat: CLI agent-friendly optimizations (P0+P1)#16
Merged
Conversation
- JSON error wrapping: --json mode outputs {"error": "..."} to stderr
- Git quiet mode: suppress git clone/fetch progress noise in --json mode
- Unify JSON tags to camelCase across all CLI output types
- Add help examples to core commands (install, inject, sync, etc.)
- Add `skillpm status --json` aggregated status command
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a JSON output mode and propagates it from the CLI root into the app and source manager, adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmd/skillpm)
participant App as App (internal/app)
participant Source as SourceManager (internal/source)
participant Git as GitExec (git subprocess)
CLI->>App: root command executes (JSONMode flag)
App->>Source: NewManager(httpClient, stateRoot, JSONMode)
Source->>Git: execute clone/fetch (quiet based on JSONMode)
Git-->>Source: result / error (progress suppressed when quiet)
Source-->>App: manager result
App-->>CLI: status / JSON output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/skillpm/main.go`:
- Around line 1692-1694: svc.ListInstalled()'s error is being ignored in the
status flow (installed, _ := svc.ListInstalled()), which can hide read failures;
change that call to capture the error (installed, err := svc.ListInstalled()),
and if err != nil return or surface the error (e.g., log and return a non-zero
exit / propagate the error up from the status command) instead of treating
installed as empty—only rely on len(installed) when err == nil; update the
surrounding code that builds the status output (the block that uses report :=
svc.DoctorRun(...) and sources := svc.SourceList()) to handle the propagated
error path consistently.
In `@internal/source/git_provider.go`:
- Around line 35-43: The clone/fetch branch currently calls cmd.Output() and
returns an error without including stderr when quiet=true; change this to use
cmd.CombinedOutput() (like the other branch) so you capture stdout+stderr into
out, and on error wrap it into the returned error message (use fmt.Errorf with
strings.Join(args, " ") and include string(out) alongside the original error).
Keep the existing cmd.Stderr = os.Stderr behavior for !quiet but always use
CombinedOutput() for the actual execution and error reporting in this
clone/fetch block (referencing cmd and the args slice).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmd/skillpm/main.gocmd/skillpm/main_test.gointernal/app/service.gointernal/config/types.gointernal/memory/context/context.gointernal/memory/scoring/scoring.gointernal/resolver/resolver_test.gointernal/source/clawhub_provider_test.gointernal/source/git_provider.gointernal/source/manager.gointernal/source/model.gointernal/store/types.gointernal/sync/sync_test.go
Comment on lines
+1692
to
+1694
| report := svc.DoctorRun(cmd.Context()) | ||
| installed, _ := svc.ListInstalled() | ||
| sources := svc.SourceList() |
There was a problem hiding this comment.
Do not swallow installed-state read failures in status.
At Line 1693, svc.ListInstalled() errors are ignored, which can silently report installedCount=0 on read failures instead of surfacing the real problem.
💡 Proposed fix
- installed, _ := svc.ListInstalled()
+ installed, err := svc.ListInstalled()
+ if err != nil {
+ return err
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/skillpm/main.go` around lines 1692 - 1694, svc.ListInstalled()'s error is
being ignored in the status flow (installed, _ := svc.ListInstalled()), which
can hide read failures; change that call to capture the error (installed, err :=
svc.ListInstalled()), and if err != nil return or surface the error (e.g., log
and return a non-zero exit / propagate the error up from the status command)
instead of treating installed as empty—only rely on len(installed) when err ==
nil; update the surrounding code that builds the status output (the block that
uses report := svc.DoctorRun(...) and sources := svc.SourceList()) to handle the
propagated error path consistently.
Comment on lines
+35
to
+43
| if len(args) > 0 && (args[0] == "clone" || args[0] == "fetch") { | ||
| if !quiet { | ||
| cmd.Stderr = os.Stderr | ||
| } | ||
| out, err := cmd.Output() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/source/git_provider.go | sed -n '20,55p'Repository: eddieran/skillpm
Length of output: 1290
🌐 Web query:
Go os/exec cmd.Output() stderr handling behavior
💡 Result:
(*exec.Cmd).Output() captures only stdout and returns it as []byte. If the command exits non-zero, the returned error is usually *exec.ExitError. Stderr handling depends on Cmd.Stderr:
- If
cmd.Stderr == niland the command fails with*exec.ExitError,Output()will capture stderr (a subset, possibly truncated) and store it inexitErr.Stderrfor debugging/error messages. [1] - If
cmd.Stderris non-nil, stderr is written there andexitErr.Stderrwill not be auto-populated byOutput(). [1]
If you want stdout + stderr together regardless of success, use cmd.CombinedOutput(). [1]
Source: Go os/exec docs (Cmd.Output, Cmd.CombinedOutput, ExitError.Stderr). [1]
References
[1] https://pkg.go.dev/os/exec
Include stderr details in clone/fetch error messages when in quiet mode.
At Lines 35-43, when quiet=true, the clone/fetch branch uses cmd.Output() without explicitly capturing stderr in the error message. Although cmd.Output() auto-populates ExitError.Stderr internally when cmd.Stderr==nil, this detail is not explicitly included in the formatted error message returned at Line 41. In JSON mode (quiet), callers cannot diagnose failures without inspecting the ExitError internals. Lines 46-50 correctly use CombinedOutput() and include output in the error message—apply the same pattern here.
💡 Proposed fix
if len(args) > 0 && (args[0] == "clone" || args[0] == "fetch") {
if !quiet {
cmd.Stderr = os.Stderr
+ out, err := cmd.Output()
+ if err != nil {
+ return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err)
+ }
+ return out, nil
}
- out, err := cmd.Output()
+ out, err := cmd.CombinedOutput()
if err != nil {
- return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err)
+ return nil, fmt.Errorf("git %s: %w\n%s", strings.Join(args, " "), err, string(out))
}
return out, nil
}📝 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
| if len(args) > 0 && (args[0] == "clone" || args[0] == "fetch") { | |
| if !quiet { | |
| cmd.Stderr = os.Stderr | |
| } | |
| out, err := cmd.Output() | |
| if err != nil { | |
| return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) | |
| } | |
| return out, nil | |
| if len(args) > 0 && (args[0] == "clone" || args[0] == "fetch") { | |
| if !quiet { | |
| cmd.Stderr = os.Stderr | |
| out, err := cmd.Output() | |
| if err != nil { | |
| return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) | |
| } | |
| return out, nil | |
| } | |
| out, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return nil, fmt.Errorf("git %s: %w\n%s", strings.Join(args, " "), err, string(out)) | |
| } | |
| return out, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/source/git_provider.go` around lines 35 - 43, The clone/fetch branch
currently calls cmd.Output() and returns an error without including stderr when
quiet=true; change this to use cmd.CombinedOutput() (like the other branch) so
you capture stdout+stderr into out, and on error wrap it into the returned error
message (use fmt.Errorf with strings.Join(args, " ") and include string(out)
alongside the original error). Keep the existing cmd.Stderr = os.Stderr behavior
for !quiet but always use CombinedOutput() for the actual execution and error
reporting in this clone/fetch block (referencing cmd and the args slice).
5 tasks
eddieran
added a commit
that referenced
this pull request
Mar 2, 2026
feat: CLI agent-friendly optimizations (P0+P1)
eddieran
added a commit
that referenced
this pull request
Mar 2, 2026
feat: CLI agent-friendly optimizations (P0+P1)
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
--jsonmode now outputs{"error": "..."}to stderr so agents can uniformly parse errors viajson.Unmarshal--jsonis active, preventing agents from misinterpreting progress output as errorsinstall,inject,sync,source add,uninstall,upgrade) now include usage examples in--helpso agents know parameter formatsskillpm status --json: New one-stop aggregated status command returning version, scope, health, installed count, source count, enabled adapters, and memory statusChanged files (13)
cmd/skillpm/main.gocmd/skillpm/main_test.gointernal/app/service.goJSONModeto Options, pass quiet to source managerinternal/config/types.gointernal/memory/context/context.gointernal/memory/scoring/scoring.gointernal/source/git_provider.gointernal/source/manager.gointernal/source/model.gointernal/store/types.gointernal/resolver/resolver_test.gointernal/source/clawhub_provider_test.gointernal/sync/sync_test.goTest plan
go vet ./...passesgo test ./... -count=1all packages pass (except e2e network-dependent)skillpm install nonexistent/skill --json 2>&1→ stderr is JSON{"error": "..."}skillpm source list --json→ camelCase keysskillpm memory scores --json→ camelCase keysskillpm status --json→ aggregated outputskillpm install --help→ shows Examples sectiongo build -o ./bin/skillpm ./cmd/skillpmcompiles🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style