cleanup: remove dead code, invalid test references, and aliases#7
Merged
cleanup: remove dead code, invalid test references, and aliases#7
Conversation
📝 WalkthroughWalkthroughRemoved several CLI commands and aliases, simplified command headers, decoupled the version command from app.Service, refactored inject to support mutually exclusive Changes
Sequence Diagram(s)mermaid (Note: colored rectangles are not required in this simple sequence.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/skillpm/main.go (1)
311-315: Add target context to inject errors in multi-agent mode.At Line 313, returning
errdirectly obscures which adapter failed when--allis used. Wrap with target info for actionable failures.Proposed patch
for _, target := range targets { res, err := svc.Inject(context.Background(), target, args) if err != nil { - return err + return fmt.Errorf("ADP_INJECT: inject failed for agent %q: %w", target, err) } fmt.Printf("injected %d skill(s) into %s\n", len(res.Injected), target) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/skillpm/main.go` around lines 311 - 315, When calling svc.Inject in the loop over targets, don't return err directly; wrap it with the target context so failures show which adapter failed in multi-agent (--all) mode. Locate the loop that iterates over targets and the call to svc.Inject(context.Background(), target, args) and replace the direct return of err with a wrapped error that includes the target identifier (e.g., target.Name or target.ID or the target variable) and the original error message (use fmt.Errorf or errors.Wrap/Wrapf consistent with the repo) so logs and returned errors indicate which specific target caused the failure.cmd/skillpm/main_test.go (1)
192-206: Add a test for the new--agent+--allconflict guard.This test now covers the “neither provided” branch; add a companion case for the new mutual-exclusion path to lock behavior.
Suggested test addition
func TestInjectRequiresAgentBeforeService(t *testing.T) { @@ } + +func TestInjectRejectsAgentAndAllBeforeService(t *testing.T) { + called := false + cmd := newInjectCmd(func() (*app.Service, error) { + called = true + return nil, errors.New("should not be called") + }) + cmd.SetArgs([]string{"demo/skill", "--agent", "ghost", "--all"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "cannot specify both --agent and --all") { + t.Fatalf("expected mutual exclusion error, got %v", err) + } + if called { + t.Fatalf("newSvc should not be called when both --agent and --all are set") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/skillpm/main_test.go` around lines 192 - 206, Add a companion test (e.g., TestInjectAgentAndAllConflict) that mirrors TestInjectRequiresAgentBeforeService but sets cmd arguments to include both --agent (with a name) and --all; use newInjectCmd to pass a stub that flips a called flag and returns an error; call cmd.Execute() and assert it returns a non-nil error, that the error message mentions both "--agent" and "--all" (to detect the mutual-exclusion guard), and that the stub (called) remains false so newInjectCmd's service factory is not invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/skillpm/main_test.go`:
- Around line 192-206: Add a companion test (e.g.,
TestInjectAgentAndAllConflict) that mirrors TestInjectRequiresAgentBeforeService
but sets cmd arguments to include both --agent (with a name) and --all; use
newInjectCmd to pass a stub that flips a called flag and returns an error; call
cmd.Execute() and assert it returns a non-nil error, that the error message
mentions both "--agent" and "--all" (to detect the mutual-exclusion guard), and
that the stub (called) remains false so newInjectCmd's service factory is not
invoked.
In `@cmd/skillpm/main.go`:
- Around line 311-315: When calling svc.Inject in the loop over targets, don't
return err directly; wrap it with the target context so failures show which
adapter failed in multi-agent (--all) mode. Locate the loop that iterates over
targets and the call to svc.Inject(context.Background(), target, args) and
replace the direct return of err with a wrapped error that includes the target
identifier (e.g., target.Name or target.ID or the target variable) and the
original error message (use fmt.Errorf or errors.Wrap/Wrapf consistent with the
repo) so logs and returned errors indicate which specific target caused the
failure.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f6991cd and 305cfa32bbfb8d26c022a69fa75ea069bd20fd71.
📒 Files selected for processing (6)
cmd/skillpm/main.gocmd/skillpm/main_test.gocmd/skillpm/version.gotest/e2e/critical_flows_test.gotest/e2e/leaderboard_test.gotools/coverage-gate.sh
💤 Files with no reviewable changes (3)
- test/e2e/leaderboard_test.go
- tools/coverage-gate.sh
- test/e2e/critical_flows_test.go
- Remove newRemoveCmd (superseded by uninstall) - Remove newHarvestCmd (references non-existent svc.HarvestRun) - Remove newValidateCmd (references non-existent svc.Validate) - Remove ALL command aliases across the CLI - Remove harvest package from coverage-gate.sh - Remove E2E tests referencing harvest/validate/remove/top-alias - Add inject --all flag for multi-agent injection - Clean newVersionCmd unused newSvc parameter - Re-enable DisableDefaultCmd for completion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eddieran
force-pushed
the
cleanup/remove-dead-code-and-aliases
branch
from
305cfa3 to
f9540bb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/skillpm/main.go (1)
337-346: Fail-fast behavior may discard partial progress with--all.When
--allis used and one agent fails, the loop exits immediately without reporting which agents succeeded. Consider whether accumulating errors and continuing would provide better UX, or at minimum logging which agents completed before the failure.Current behavior is acceptable for an initial implementation but worth noting.
🔧 Optional: Continue-on-error approach
+ var errs []error for _, target := range targets { res, err := svc.Inject(context.Background(), target, args) if err != nil { - return err + errs = append(errs, fmt.Errorf("agent %s: %w", target, err)) + continue } results = append(results, agentResult{Agent: target, Injected: len(res.Injected)}) if !*jsonOutput { fmt.Printf("injected %d skill(s) into %s\n", len(res.Injected), target) } } + if len(errs) > 0 { + // Still output successful results before returning error + if *jsonOutput { + _ = print(true, results, "") + } + return fmt.Errorf("injection failed for %d agent(s): %v", len(errs), errs) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/skillpm/main.go` around lines 337 - 346, The loop over targets currently returns on the first svc.Inject error and discards prior successes; update the behavior so failures don't hide completed agents: inside the for loop that calls svc.Inject(target, args) (symbols: svc.Inject, targets, results, agentResult, jsonOutput) capture errors per-agent rather than returning immediately — e.g., append a structured error record (agent name + err) to an errors slice and continue the loop, still appending successful agentResult entries and printing when !*jsonOutput; after the loop, if the errors slice is non-empty, print/log a concise summary of which agents failed and return an aggregated error (or non-zero exit) so callers see both successes and failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/skillpm/main.go`:
- Around line 337-346: The loop over targets currently returns on the first
svc.Inject error and discards prior successes; update the behavior so failures
don't hide completed agents: inside the for loop that calls svc.Inject(target,
args) (symbols: svc.Inject, targets, results, agentResult, jsonOutput) capture
errors per-agent rather than returning immediately — e.g., append a structured
error record (agent name + err) to an errors slice and continue the loop, still
appending successful agentResult entries and printing when !*jsonOutput; after
the loop, if the errors slice is non-empty, print/log a concise summary of which
agents failed and return an aggregated error (or non-zero exit) so callers see
both successes and failures.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 305cfa32bbfb8d26c022a69fa75ea069bd20fd71 and f9540bb.
📒 Files selected for processing (6)
cmd/skillpm/main.gocmd/skillpm/main_test.gocmd/skillpm/version.gotest/e2e/critical_flows_test.gotest/e2e/leaderboard_test.gotools/coverage-gate.sh
💤 Files with no reviewable changes (3)
- test/e2e/critical_flows_test.go
- test/e2e/leaderboard_test.go
- tools/coverage-gate.sh
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 323-331: When handling the --all flag, ensure we fail when no
enabled adapters are discovered instead of silently doing nothing: after the
allAgents branch that iterates svc.Config.Adapters and appends to targets, check
if len(targets) == 0 and return/exit with a clear error (e.g. "no enabled
adapters found" or "no targets for injection") so callers can distinguish a real
no-op from success; update the function that builds targets (referencing
allAgents, svc.Config.Adapters, targets, agentName) to perform this explicit
error return when targets is empty.
- Around line 337-349: The loop over targets aborts on the first svc.Inject
error and drops previously collected results; change the logic in the for loop
that calls svc.Inject so it continues processing all targets even when
svc.Inject returns an error, capture per-target error details (e.g., extend
agentResult to include an Error string or status) for each target, append either
the successful injected count or the error info to results, and after the loop
use *jsonOutput with print(true, results, "") to emit the full result list and
return a non-zero error/exit code if any target failed; update references to
svc.Inject, agentResult, jsonOutput, and print accordingly so callers get
per-target outcomes instead of an early return.
Comment on lines
+323
to
331
| if allAgents { | ||
| for _, a := range svc.Config.Adapters { | ||
| if a.Enabled { | ||
| targets = append(targets, a.Name) | ||
| } | ||
| } | ||
| } else { | ||
| targets = []string{agentName} | ||
| } |
There was a problem hiding this comment.
Handle --all with zero enabled adapters explicitly.
If no enabled adapters are found, the command currently succeeds as a silent no-op (non-JSON mode). Return a clear error so callers can distinguish “nothing targeted” from successful injection.
💡 Proposed fix
if allAgents {
for _, a := range svc.Config.Adapters {
if a.Enabled {
targets = append(targets, a.Name)
}
}
+ if len(targets) == 0 {
+ return &exitError{code: 2, msg: "INJECT_NO_TARGETS: no enabled adapters found"}
+ }
} else {
targets = []string{agentName}
}📝 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 allAgents { | |
| for _, a := range svc.Config.Adapters { | |
| if a.Enabled { | |
| targets = append(targets, a.Name) | |
| } | |
| } | |
| } else { | |
| targets = []string{agentName} | |
| } | |
| if allAgents { | |
| for _, a := range svc.Config.Adapters { | |
| if a.Enabled { | |
| targets = append(targets, a.Name) | |
| } | |
| } | |
| if len(targets) == 0 { | |
| return &exitError{code: 2, msg: "INJECT_NO_TARGETS: no enabled adapters found"} | |
| } | |
| } else { | |
| targets = []string{agentName} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/skillpm/main.go` around lines 323 - 331, When handling the --all flag,
ensure we fail when no enabled adapters are discovered instead of silently doing
nothing: after the allAgents branch that iterates svc.Config.Adapters and
appends to targets, check if len(targets) == 0 and return/exit with a clear
error (e.g. "no enabled adapters found" or "no targets for injection") so
callers can distinguish a real no-op from success; update the function that
builds targets (referencing allAgents, svc.Config.Adapters, targets, agentName)
to perform this explicit error return when targets is empty.
Comment on lines
+337
to
349
| for _, target := range targets { | ||
| res, err := svc.Inject(context.Background(), target, args) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| results = append(results, agentResult{Agent: target, Injected: len(res.Injected)}) | ||
| if !*jsonOutput { | ||
| fmt.Printf("injected %d skill(s) into %s\n", len(res.Injected), target) | ||
| } | ||
| } | ||
| if *jsonOutput { | ||
| return print(true, res, "") | ||
| return print(true, results, "") | ||
| } |
There was a problem hiding this comment.
Preserve per-target results instead of aborting on first inject failure.
The loop returns on the first failed target, so multi-target runs can partially mutate state while dropping accumulated results (especially for JSON callers). Continue processing targets, record per-target errors, then return a non-zero exit at the end if any failed.
💡 Proposed fix
type agentResult struct {
Agent string `json:"agent"`
Injected int `json:"injected"`
+ Error string `json:"error,omitempty"`
}
- var results []agentResult
+ var (
+ results []agentResult
+ hadError bool
+ )
for _, target := range targets {
res, err := svc.Inject(context.Background(), target, args)
if err != nil {
- return err
+ hadError = true
+ results = append(results, agentResult{Agent: target, Error: err.Error()})
+ if !*jsonOutput {
+ fmt.Fprintf(os.Stderr, "inject failed for %s: %v\n", target, err)
+ }
+ continue
}
results = append(results, agentResult{Agent: target, Injected: len(res.Injected)})
if !*jsonOutput {
fmt.Printf("injected %d skill(s) into %s\n", len(res.Injected), target)
}
}
if *jsonOutput {
- return print(true, results, "")
+ if err := print(true, results, ""); err != nil {
+ return err
+ }
+ if hadError {
+ return &exitError{code: 2, msg: "INJECT_PARTIAL_FAILURE: one or more agent injections failed"}
+ }
+ return nil
}
+ if hadError {
+ return &exitError{code: 2, msg: "INJECT_PARTIAL_FAILURE: one or more agent injections failed"}
+ }
return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/skillpm/main.go` around lines 337 - 349, The loop over targets aborts on
the first svc.Inject error and drops previously collected results; change the
logic in the for loop that calls svc.Inject so it continues processing all
targets even when svc.Inject returns an error, capture per-target error details
(e.g., extend agentResult to include an Error string or status) for each target,
append either the successful injected count or the error info to results, and
after the loop use *jsonOutput with print(true, results, "") to emit the full
result list and return a non-zero error/exit code if any target failed; update
references to svc.Inject, agentResult, jsonOutput, and print accordingly so
callers get per-target outcomes instead of an early return.
This was referenced Feb 26, 2026
eddieran
added a commit
that referenced
this pull request
Mar 2, 2026
cleanup: remove dead code, invalid test references, and aliases
eddieran
added a commit
that referenced
this pull request
Mar 2, 2026
cleanup: remove dead code, invalid test references, and aliases
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
Describe what changed and why.
Type of change
Validation
go test ./...go vet ./..../tools/coverage-gate.sh(when applicable)Risk / Compatibility
Notes
Anything reviewers should focus on.
Summary by CodeRabbit
New Features
Changed
Removed
Chores