Return explicit result when updating an unmanaged tool by selector

[ametel01]

Summary

• Targeted updates of externally managed tools now produce an explicit skip action with reason "tool is not managed by atb" instead of a silent empty plan
• Bulk updates (no selector) continue to silently skip external tools as before

Closes #28

Test plan

• TestBuildUpdatePlanExternalToolReturnsSkip — external tool produces skip action with reason
• TestBuildUpdatePlanUnknownToolReturnsError — unknown selector still returns error (unchanged)
• TestBuildUpdatePlanManagedOnly — bulk update behavior unchanged
• All tests pass, golangci-lint clean

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Targeted updates of externally managed tools now return an explicit skip reason instead of silently producing no result.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify BuildUpdatePlan to return an explicit skip action when a targeted update is requested for an externally managed tool, replacing the previous silent no-op behavior. A corresponding test validates this behavior, and the changelog documents the fix.

Changes

Cohort / File(s)	Summary
Implementation internal/plan/update.go	Added conditional logic to append a skip action with reason "tool is not managed by atb" when BuildUpdatePlan encounters a non-managed tool with a specific toolID provided, replacing silent continuation.
Tests internal/plan/plan_test.go	Added TestBuildUpdatePlanExternalToolReturnsSkip to verify that externally managed tools produce an explicit ActionSkip when targeted for update.
Documentation CHANGELOG.md	Documented the fix that targeted updates of externally managed tools now return an explicit skip reason instead of a silent empty plan.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• Return an explicit result when updating an unmanaged tool by selector #28: Directly addresses the issue's objective to return an explicit skip reason when updating an externally managed tool by selector, fulfilling all acceptance criteria.

Poem

🐰 A tool from afar, not ours to keep,
Now whispers its reason, no more silent sleep.
With atb boundaries clearly defined,
Explicit skips leave no doubts behind!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: returning an explicit result when updating an unmanaged tool by selector, which matches the primary objective and code changes.
Linked Issues check	✅ Passed	The PR successfully addresses all acceptance criteria from issue #28: externally managed tools now produce an explicit skip with reason, unknown selectors still error, managed selectors work normally, and tests cover all three cases.
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #28. The test addition, update.go modification, and CHANGELOG entry are all scoped to the targeted behavior change for unmanaged tool updates.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/28-explicit-result-unmanaged-update

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/plan/update.go (1)

25-31: Centralize the skip-reason string to avoid drift.

Now that this message is part of expected behavior, consider using a package-level constant so implementation and tests stay aligned.

♻️ Proposed refactor

+const reasonToolNotManaged = "tool is not managed by atb"
+
 func BuildUpdatePlan(snapshot discovery.Snapshot, managers []pkgmgr.Manager, toolID string) (Plan, error) {
@@
 		if presence.Ownership != state.OwnershipManaged || presence.Receipt == nil {
 			if toolID != "" {
 				actions = append(actions, Action{
 					Tool:   presence.Tool,
 					Type:   ActionSkip,
-					Reason: "tool is not managed by atb",
+					Reason: reasonToolNotManaged,
 				})
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/plan/update.go` around lines 25 - 31, Introduce a package-level
constant for the skip reason string and replace the inline literal in the
actions append with that constant; specifically, add something like const
SkipReasonToolNotManaged = "tool is not managed by atb" at the top of the
package and update the block that appends Action{Tool: presence.Tool, Type:
ActionSkip, Reason: "tool is not managed by atb"} to use
SkipReasonToolNotManaged instead (referencing Action, ActionSkip, and the append
in update.go). Ensure any tests that assert this message are updated to
reference the new constant so implementation and tests remain aligned.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/plan/update.go`:
- Around line 25-31: Introduce a package-level constant for the skip reason
string and replace the inline literal in the actions append with that constant;
specifically, add something like const SkipReasonToolNotManaged = "tool is not
managed by atb" at the top of the package and update the block that appends
Action{Tool: presence.Tool, Type: ActionSkip, Reason: "tool is not managed by
atb"} to use SkipReasonToolNotManaged instead (referencing Action, ActionSkip,
and the append in update.go). Ensure any tests that assert this message are
updated to reference the new constant so implementation and tests remain
aligned.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2427375b-afda-4297-a62d-637af2dddde2

📥 Commits

Reviewing files that changed from the base of the PR and between f14dd76 and f36204e.

📒 Files selected for processing (3)

• CHANGELOG.md
• internal/plan/plan_test.go
• internal/plan/update.go