Add doctor actions for skill path overlap warnings#180
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an action field to Doctor checks and validates it via unit + integration tests so users get clearer remediation guidance for cross-target discovery/shared-path warnings.
Changes:
- Extend Doctor JSON schema/result model to include an optional
actionfield. - Emit and test remediation guidance for shared-path collisions and cross-target discovery.
- Add/extend unit and integration tests to assert action presence in both text and JSON output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/doctor_test.go | Adds integration coverage asserting Action: is shown in CLI output and present in --json output. |
| cmd/skillshare/doctor_shared_paths_test.go | Ensures produced warnings now include non-empty Action. |
| cmd/skillshare/doctor_shared_paths.go | Introduces action strings and prints remediation guidance; wires action into Doctor checks. |
| cmd/skillshare/doctor_json_test.go | Adds unit test asserting action is included when marshaling Doctor JSON. |
| cmd/skillshare/doctor_json.go | Adds Action field to the doctorCheck JSON type. |
| cmd/skillshare/doctor.go | Adds addCheckWithAction and routes existing addCheck through it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| details := make([]string, 0, len(collisions)) | ||
| for _, c := range collisions { | ||
| ui.Warning("Shared path %s ← %s", c.path, strings.Join(c.targets, ", ")) | ||
| fmt.Println(ui.DimText(" Action: " + sharedTargetPathsRemediation(c.targets))) |
| details = append(details, fmt.Sprintf("%s (%s) also scans %s ← %s", | ||
| so.scanner, so.scannerPath, p.sharedPath, strings.Join(p.writers, ", "))) | ||
| } | ||
| fmt.Println(ui.DimText(" Action: " + crossTargetDiscoveryRemediation(so.scanner, writers))) |
| const ( | ||
| sharedTargetPathsAction = "Choose one authoritative target for each shared directory; remove or reconfigure the other target(s) before syncing." | ||
| crossTargetDiscoveryAction = "Choose one authoritative route for each overlapping runtime; remove or reconfigure duplicate targets, or exclude overlapping skills from one side before syncing." | ||
| ) |
|
|
||
| msg := fmt.Sprintf("%d shared target path(s) — enabled targets writing to the same directory may produce duplicate skills in runtime pickers", len(collisions)) | ||
| result.addCheck("shared_target_paths", checkWarning, msg, details) | ||
| result.addCheckWithAction("shared_target_paths", checkWarning, msg, details, sharedTargetPathsAction) |
|
|
||
| msg := fmt.Sprintf("%d target(s) overlap with other targets' content via cross-runtime discovery", len(scannerNames)) | ||
| result.addCheck("cross_target_discovery", checkWarning, msg, details) | ||
| result.addCheckWithAction("cross_target_discovery", checkWarning, msg, details, crossTargetDiscoveryAction) |
| sb := testutil.NewSandbox(t) | ||
| defer sb.Cleanup() | ||
|
|
||
| os.MkdirAll(filepath.Join(sb.Home, ".agents", "skills"), 0755) |
| sb := testutil.NewSandbox(t) | ||
| defer sb.Cleanup() | ||
|
|
||
| os.MkdirAll(filepath.Join(sb.Home, ".agents", "skills"), 0755) |
| out := buildDoctorOutput(result) | ||
| data, err := json.Marshal(out) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(data), `"action":"Choose one authoritative route."`) { | ||
| t.Fatalf("doctor JSON missing action field: %s", data) | ||
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces actionable remediation guidance to the doctor command when detecting shared target paths or cross-target discovery overlaps. It adds an Action field to the doctor check results, updates the CLI output to display these actions, includes them in the JSON output, and adds corresponding unit and integration tests to verify the new behavior. There are no review comments, and the changes look solid, so I have no additional feedback to provide.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 141bf518e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| func sharedTargetPathsRemediation(targets []string) string { | ||
| return fmt.Sprintf("Choose one authoritative target from %s; preview removing duplicate targets with `skillshare target remove <name> --dry-run`.", |
There was a problem hiding this comment.
When skillshare doctor --global is run from inside a directory that has a project config, this suggested command does not operate on the same config that doctor just checked: cmdTarget leaves the command in auto mode and resolves auto mode to project whenever projectConfigExists(cwd) is true (cmd/skillshare/target.go lines 35-40). In that context the user will preview or later remove a project target instead of the global duplicate target being reported; pass the current doctor mode into the remediation and include --global/--project in these target-removal suggestions.
Useful? React with 👍 / 👎.
|
Thanks for this, @PeterTianbuhan! 🙏 Heads-up: this overlaps with #177, which landed on That said, one part of your PR is genuinely nicer than what we merged: surfacing the concrete Would you be up for rebasing onto the latest Happy to help if anything about the merged structure is unclear. Thanks again for digging into #135! |
PeterTianbuhan
force-pushed
the
codex/doctor-overlap-suggestions
branch
from
141bf51 to
1caa32b
Compare
Implemented actionable remediation guidance for
doctorwarnings that already detect shared target paths and cross-target discovery overlap.The text output now shows a concrete
Action:line for each overlap, including the involved targets and the safeskillshare target remove <name> --dry-runpreview path. The JSON output also exposes an optionalactionfield so UI and automation can surface the same guidance.This keeps the existing detection behavior unchanged. It does not implement the full adopt/claim workflow from #135; it only makes the current duplicate-root diagnostics more actionable.
Refs #135
Tests:
go test ./cmd/skillshare -run 'TestCheckSharedTargetPaths|TestCheckCrossTargetDiscovery|TestDoctorCheckActionJSON'go test ./tests/integration -run 'TestDoctor_SharedDiscoveryShowsAction|TestDoctor_JSON_SharedDiscoveryIncludesAction'./scripts/test.sh --quiet