feat(cli): add gentle-ai doctor command for ecosystem health diagnostics#304
feat(cli): add gentle-ai doctor command for ecosystem health diagnostics#304baltasarblanco wants to merge 4 commits intoGentleman-Programming:mainfrom
Conversation
GerardoFC8
left a comment
There was a problem hiding this comment.
Great foundation — nice work!
Hey @baltasarblanco, I'm the author of #302. Really solid implementation — you nailed the core architecture: concurrent runner with bounded semaphore, DI via package-level vars (matching the existing updateCheckAll pattern), symlink deduplication, Windows support, and 28 test cases with -race. That's thorough.
A few things to clean up before this is merge-ready, and some suggestions for the future:
1. Leftover review/debug comments (must fix)
There are three comments that look like development notes left in the code — see inline comments below. These should be removed before merge.
2. Docstring regression in path.go (must fix)
The diff introduces an unintended change to the escapePowerShellString docstring — it changes '' (correct: PowerShell escapes a single quote with two single quotes) to " (incorrect: that's a double quote, completely different semantics). The function body still does ReplaceAll("'", "''"), so the code is correct but the doc now contradicts it. See inline comment.
3. FindAllBinaryCopies — missing doc comment (suggestion)
The function is exported and is a new public API. A GoDoc comment explaining what it does, its return value semantics (ordered by PATH precedence, symlinks resolved for dedup but original paths returned), and edge cases (empty PATH returns nil) would help future contributors.
4. Future PRs — general tips
- Clean your diff before opening: run
git diffand scan for comments like// <-- ADD THISor// NUEVO. These are useful while coding but shouldn't ship. - Avoid unrelated changes in the same PR: the
escapePowerShellStringdocstring change is unrelated to the doctor feature. One PR = one concern makes reviews faster and reverts safer.
I'd love to contribute the remaining check categories (platform, agents, env) as follow-up PRs once this lands. The full spec with requirements and scenarios is in #302 if that helps as reference. Happy to collaborate!
| "github.com/gentleman-programming/gentle-ai/internal/backup" | ||
| "github.com/gentleman-programming/gentle-ai/internal/cli" | ||
| componentuninstall "github.com/gentleman-programming/gentle-ai/internal/components/uninstall" | ||
| componentuninstall "github.com/gentleman-programming/gentle-ai/internal/components/uninstall" // ← 1a. NUEVO |
There was a problem hiding this comment.
Leftover development comment — // ← 1a. NUEVO should be removed. This line wasn't changed functionally (just the import that was already there), so the comment adds noise to the diff.
| case "uninstall": | ||
| _, err := cli.RunUninstall(args[1:], stdout) | ||
| return err | ||
| case "doctor": // <-- ADD THIS |
There was a problem hiding this comment.
Both // <-- ADD THIS comments (this line and the next) are development notes that should be removed before merge. The code is self-explanatory — a case "doctor" dispatch doesn't need annotation.
|
|
||
| // escapePowerShellString escapes a string for safe use inside a PowerShell | ||
| // single-quoted string literal by replacing each ' with '' (PowerShell's escape | ||
| // single-quoted string literal by replacing each ' with ” (PowerShell's escape |
There was a problem hiding this comment.
Bug: this change is incorrect. The original comment said '' (two single quotes), which is correct — PowerShell escapes a literal single quote inside a single-quoted string with ''. The new text says " (a double quote), which is a completely different character and contradicts the function body (strings.ReplaceAll(s, "'", "''")).
This looks like an unintended edit — the original docstring was correct. Should be reverted.
There was a problem hiding this comment.
Thanks for the thorough review @GerardoFC8! All three issues are fixed in 3d94efe:
- Removed
// ← 1a. NUEVOfrom import line - Removed both
// <-- ADD THIScomments from case dispatch - Restored
escapePowerShellStringdocstring (''instead of the Unicode right double quote that slipped in)
Also added a GoDoc comment to FindAllBinaryCopies per your suggestion.
Appreciate the clean-diff lesson — I'll run git diff as a final check before every push going forward.
… add GoDoc to FindAllBinaryCopies
|
Awesome, all three fixes look good in 3d94efe — thanks for the quick turnaround! The Looking forward to building on this foundation with the remaining check categories (platform, agents, env) once this lands. Great first contribution to doctor! 🤝 |
🔗 Linked Issue
Closes #302
🏷️ PR Type
type:bugtype:featuretype:docstype:refactortype:choretype:breaking-change📝 Summary
Implements the MVP foundation for
gentle-ai doctor— a read-only diagnostic command that scans the ecosystem and reports health status. This PR establishes the architecture (Check struct, concurrent runner, rendering) and ships the first check category: duplicate binary detection across PATH.The command detects shadowed binaries (e.g., engram installed via both Homebrew AND go install), which is the root cause behind issues like #114, #177, and #246.
📂 Changes
internal/system/path.goFindAllBinaryCopies()— walks all PATH entries, resolves symlinks, deduplicatesinternal/system/binaries_test.gointernal/doctor/doctor.goCheck,CheckResult,Report,Options) and concurrent runner with bounded semaphoreinternal/doctor/doctor_test.gointernal/doctor/deps.goDuplicateBinaryCheckfor gentle-ai/engram/gga +AllChecks()entry point + DI setterinternal/doctor/deps_test.gointernal/doctor/render.gointernal/doctor/render_test.gointernal/app/app.gocase "doctor"dispatch (first switch block) +runDoctorhandler + DI wiringinternal/app/help.godoctorto COMMANDS list🧪 Test Plan
Unit Tests
Manual Tests
go test ./...)cd e2e && ./docker-test.sh)🤖 Automated Checks
Closes/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be appliedgo test ./...must passcd e2e && ./docker-test.shmust pass✅ Contributor Checklist
status:approvedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
This is the MVP foundation — intentionally scoped to one check category (deps: duplicate binary detection) to keep the PR reviewable. The architecture supports incremental addition of the remaining categories from the spec:
update.CheckFiltered, engram HTTP health, GGA assetsDetectScoop(), npm writabilityDesign follows existing project patterns: DI via package-level vars (like
updateCheckAll), reusesverify.CheckStatusconstants,Checkstruct mirrorsverify.Check, concurrent runner matchesdeps.goWaitGroup pattern.28 test cases across 2 packages, all pass with
-race.