Skip to content

fix(doctor): address Sage review findings on duplication-check PR #601 - #602

Merged
HenryLach merged 1 commit into
mainfrom
fix/sage-review-followup-doctor
Jun 17, 2026
Merged

fix(doctor): address Sage review findings on duplication-check PR #601#602
HenryLach merged 1 commit into
mainfrom
fix/sage-review-followup-doctor

Conversation

@HenryLach

Copy link
Copy Markdown
Owner

Follow-up to PR #601. Sage's review flagged three minor issues; all three are addressed here.

Findings addressed

1. Summary message no longer recommends taskplane init for duplicate installs ([minor], confidence high)

Sage's finding: the duplication block increments issues, so doctor exits non-zero and the final line was '$N issue(s) found. Run taskplane init to fix config issues.' — actively misleading when the underlying issue is a duplicate install (whose fix is npm uninstall -g taskplane, not init).

Fix: track a separate duplicationDetected flag; emit a context-aware summary:

  • with duplication: 'See remediation hints above (npm uninstall -g taskplane for the duplicate install)'
  • without: 'See remediation hints above; run taskplane init to fix config issues'

Either way the FAIL exit code is preserved so CI gates on doctor still catch the issue.

2. PowerShell-aware remediation in the duplication block ([minor], confidence high)

Sage's finding: the inline fix only showed bash syntax (export PATH=...), even though Taskplane is cross-platform and the duplicate-install issue is especially common on Windows + NVM-for-Windows (see #598). Windows users would copy/paste a non-working command.

Fix: branches on process.platform:

  • Windows: shows the $env:PATH = '$HOME\.pi\...' PowerShell snippet first with the Git Bash equivalent as a secondary 'or, for Git Bash:' line
  • Non-Windows: shows the bash form as before

3. 5-second timeout on execSync('npm root -g') ([question], medium confidence — verify)

Sage's finding: execSync had no timeout, raising potential hang risk if npm is slow or misconfigured.

Fix: added timeout: 5000 to the execSync options. On timeout the catch branch runs, returning null and silently skipping the duplication check (best-effort by design).

Findings deliberately NOT addressed

Sage's centralization suggestion (part of finding 3)

Sage suggested centralizing npm-root probing by sharing logic with extensions/taskplane/path-resolver.ts. I'm not doing this, with reasoning:

  • bin/taskplane.mjs is a self-contained CLI script with no cross-module imports from the extensions tree
  • Pulling in the resolver would expand the import surface for what amounts to a 5-line execSync call
  • The two functions have distinct purposes: the path resolver finds Pi/Taskplane for spawning; this helper detects on-disk duplication for diagnostics
  • Sage flagged this as 'medium confidence — verify'; on verification, the local copy is the right call

Out-of-scope: docs/specifications/** references to npm install -g taskplane

Sage correctly labeled this as out-of-scope. Leaving it for a future docs consistency sweep.

Validation

Gate Result
npm run typecheck ✅ pass
npm run format:check ✅ pass
Full test suite ✅ 3,714 / 3,715 pass, 1 skipped — zero new failures
Doctor smoke (no duplication) ✅ clean output, summary unchanged
Doctor smoke (simulated dup, Windows) ✅ WARN block fires, PowerShell line first, Git Bash secondary, FAIL summary references npm uninstall instead of taskplane init

Diff summary

Single file: bin/taskplane.mjs, +42 −6.

Sage's review of #601 flagged three minor issues. All three are
addressed here:

1. Summary message no longer recommends 'taskplane init' as a generic
   remediation when the underlying issue was a duplicate install.
   Previously, the doctor's final line was always
   '$N issue(s) found. Run taskplane init to fix config issues.',
   which was actively misleading when the only issue was a duplicate
   install (whose fix is 'npm uninstall -g taskplane', not init).
   Now tracks a separate duplicationDetected flag and emits a
   context-aware summary: 'See remediation hints above (npm uninstall
   -g taskplane for the duplicate install)' when duplication fired,
   or the original-style 'See remediation hints above; run taskplane
   init to fix config issues' otherwise. Either way the FAIL exit code
   is preserved so CI gates on doctor still catch the issue.

2. PowerShell-aware remediation in the duplication block. The original
   inline fix only showed bash syntax (export PATH=...), even though
   Taskplane is cross-platform and the duplicate-install issue is
   especially common on Windows + NVM-for-Windows (see #598). Now
   branches on process.platform: Windows users see the $env:PATH
   PowerShell snippet first with the Git Bash equivalent as a
   secondary 'or, for Git Bash:' line; non-Windows hosts see the
   bash form. Either OS+shell combination has a one-line copy/paste
   path that matches the host's likely shell.

3. 5-second timeout on execSync('npm root -g') in
   detectDuplicateTaskplaneInstall(). Guards against the rare case
   where npm is on PATH but hangs (slow registry probe, misconfigured
   custom prefix, etc.). taskplane doctor is a diagnostic and must
   never make the CLI feel hung. On timeout the catch branch runs,
   returning null and silently skipping the duplication check
   (best-effort by design).

Sage's question-level finding about centralizing npm-root probing
(sharing logic with extensions/taskplane/path-resolver.ts) was NOT
addressed in this commit. Reasoning: bin/taskplane.mjs is a
self-contained CLI script with no cross-module imports from the
extensions tree; pulling in the resolver would expand the import
surface for a 5-line execSync. The duplicated logic is small and the
two functions have distinct purposes (the path-resolver finds Pi/
Taskplane for spawning; this helper detects on-disk duplication for
diagnostics). Sage flagged this as 'medium confidence — verify',
and on verification I'm keeping the duplication local.

The out-of-scope finding (docs/specifications/** still referencing
'npm install -g taskplane') was correctly labeled by Sage as
out-of-scope and is left for a docs consistency sweep.

Validation

  npm run typecheck             pass
  npm run format:check          pass
  Full test suite               3714/3715 pass, 1 skipped, zero new failures
  Doctor smoke (no duplication) clean output, summary unchanged
  Doctor smoke (simulated dup)  WARN block fires, PowerShell line shown
                                first on Windows, FAIL summary now
                                references npm uninstall instead of
                                taskplane init
@HenryLach
HenryLach enabled auto-merge June 17, 2026 21:20
@HenryLach
HenryLach merged commit 94fa231 into main Jun 17, 2026
1 check passed
@HenryLach
HenryLach deleted the fix/sage-review-followup-doctor branch June 17, 2026 21:21
HenryLach added a commit that referenced this pull request Jun 17, 2026
Move the three improvements from [Unreleased] into a dated [0.30.3]
section:

  Fixed:
    - taskplane --version 'vundefined' placeholder (#601)

  Enhanced:
    - taskplane doctor duplicate-install detection with platform-aware
      remediation (#601, #602)

  Docs:
    - README install section + install tutorial Pi 0.75.0 PATH guidance
      (#601)

Insert fresh empty [Unreleased] placeholder per keep-a-changelog
convention.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant