[PR #3296] [CLOSED] fix(doctor): resolve unqualified skill identity from installed destinations #5706

Closed
opened 2026-10-31 01:08:49 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3296
Author: @sx4im
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/3208-doctor-skill-scope


📝 Commits (1)

  • 77bcd04 fix(doctor): resolve unqualified skill identity from installed destinations

📊 Changes

4 files changed (+143 additions, -7 deletions)

View changed files

📝 src/application/Doctor.test.ts (+27 -0)
📝 src/application/Doctor.ts (+7 -4)
📝 src/application/SetupSkill.ts (+64 -3)
📝 tests/boundary/filesystem/setupSkill.test.ts (+45 -0)

📄 Description

Summary

Fixes #3208. Unqualified doctor checks (doctor --skill, plain doctor) now resolve skill identity from the destinations where a managed skill is actually installed, instead of from the client selection.

Problem and expected behavior

After setup --skill-client claude_code --skill=true, bare doctor --skill --json reported the installed skill missing (null version and tool count, failing skill:identity check). Creating an empty $HOME/.cursor made plain doctor --json report the same installed skill stale (config_drift, "update the installed skill"), even though all installed files were byte-identical to the bundled workflow. Adding --client claude_code reported aligned in both cases.

Both unqualified checks fed a client selection into the identity lookup. Bare --skill passed an empty client list, which resolves to the shared skill root only, so the installed client copy was never examined. The full audit passed every detected client, including marker-only ones like Cursor, which maps to the shared root; the absent shared copy made the inspection noncanonical and the check reported stale.

Change and scope

  • src/application/Doctor.ts: when the doctor scope has no explicit client selection, the identity check now receives an undefined client selection instead of an empty list (explicit --skill) or every detected registration (audit-wide). Explicit --client and setup's skillClients path are unchanged.
  • src/application/SetupSkill.ts: readInstalledSkillIdentity accepts an undefined selection and then inspects the destinations that hold an installed managed skill, plus destinations with blocking path issues so unmanaged or unreadable state still surfaces. Destinations that never held a skill are skipped, so an unselected client marker cannot flip the result.

Out of scope: registration and environment diagnostics are untouched; a genuinely empty home still reports missing, and a genuinely changed managed file still reports stale.

Contract and boundary impact

  • Semantic owner and earliest changed stage: application layer (Doctor / SetupSkill); no CLI or MCP surface change.
  • Contribution scope fit and linked maintainer decision: ordinary bug fix within existing boundaries, not applicable.
  • CLI and MCP/tool-catalog contract: none. doctor --json output shape unchanged; only the identity values for unqualified scopes are now correct.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: src/application/Doctor.test.ts (unqualified --skill and audit-wide scopes pass an undefined client selection to installedSkillIdentity); tests/boundary/filesystem/setupSkill.test.ts (new "unqualified doctor skill identity" block: installed skill found for bare doctor --skill; empty .cursor marker ignored by plain doctor; empty home still reports missing).
  • Base reproduction or other evidence: reproduced both failure modes on pristine main with the issue's CLI steps in an isolated HOME (bare --skill reported missing; plain doctor with an empty ~/.cursor reported stale). After the fix, the same steps report aligned with version 42 and tool count 207, and the required skill:identity check passes. Negative controls verified through the CLI: a tampered managed file still reports stale with a failing check; a setup refresh restores aligned; an empty home still reports missing with a failing check.
  • User-visible CLI/MCP output (if applicable): doctor --skill --json now shows "state": "aligned", "installed_version": "42", "installed_tool_count": 207 for the issue's setup.
  • Remaining proof gaps: no macOS or Windows run; Linux only. No real Hopper/Ghidra involvement (not applicable to this change).

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run src/application/Doctor.test.ts — 31 passed
  • npx vitest run tests/boundary/filesystem/setupSkill.test.ts -t "unqualified doctor skill identity" — 3 passed (all 3 failed on pristine main before the fix)
  • npx vitest run tests/boundary/filesystem/setupSkill.test.ts (full file) — 12 passed, 3 failed; the 3 failures are pre-existing on pristine main (verified by stashing the change and re-running: same 3 failures in "canonical skill transaction")
  • npm run check:fast — typecheck and lint passed
  • npm run verify:test-discovery — all 1262 test files discovered exactly once
  • Issue reproduction script with isolated HOME, before and after the fix — both failure modes reproduced on base, resolved with the fix, negative controls hold

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux CLI verification only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; the change only alters which installed skill destinations an unqualified doctor check inspects.

Author readiness

Human owner:

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare the current revision ready for maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3296 **Author:** [@sx4im](https://github.com/sx4im) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3208-doctor-skill-scope` --- ### 📝 Commits (1) - [`77bcd04`](https://github.com/morluto/rea/commit/77bcd040d0174493bd20fd7890027f732266730a) fix(doctor): resolve unqualified skill identity from installed destinations ### 📊 Changes **4 files changed** (+143 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `src/application/Doctor.test.ts` (+27 -0) 📝 `src/application/Doctor.ts` (+7 -4) 📝 `src/application/SetupSkill.ts` (+64 -3) 📝 `tests/boundary/filesystem/setupSkill.test.ts` (+45 -0) </details> ### 📄 Description ## Summary Fixes #3208. Unqualified doctor checks (`doctor --skill`, plain `doctor`) now resolve skill identity from the destinations where a managed skill is actually installed, instead of from the client selection. ## Problem and expected behavior After `setup --skill-client claude_code --skill=true`, bare `doctor --skill --json` reported the installed skill missing (null version and tool count, failing `skill:identity` check). Creating an empty `$HOME/.cursor` made plain `doctor --json` report the same installed skill stale (`config_drift`, "update the installed skill"), even though all installed files were byte-identical to the bundled workflow. Adding `--client claude_code` reported aligned in both cases. Both unqualified checks fed a client selection into the identity lookup. Bare `--skill` passed an empty client list, which resolves to the shared skill root only, so the installed client copy was never examined. The full audit passed every detected client, including marker-only ones like Cursor, which maps to the shared root; the absent shared copy made the inspection noncanonical and the check reported stale. ## Change and scope - `src/application/Doctor.ts`: when the doctor scope has no explicit client selection, the identity check now receives an undefined client selection instead of an empty list (explicit `--skill`) or every detected registration (audit-wide). Explicit `--client` and setup's `skillClients` path are unchanged. - `src/application/SetupSkill.ts`: `readInstalledSkillIdentity` accepts an undefined selection and then inspects the destinations that hold an installed managed skill, plus destinations with blocking path issues so unmanaged or unreadable state still surfaces. Destinations that never held a skill are skipped, so an unselected client marker cannot flip the result. Out of scope: registration and environment diagnostics are untouched; a genuinely empty home still reports missing, and a genuinely changed managed file still reports stale. ## Contract and boundary impact - Semantic owner and earliest changed stage: application layer (`Doctor` / `SetupSkill`); no CLI or MCP surface change. - Contribution scope fit and linked maintainer decision: ordinary bug fix within existing boundaries, not applicable. - CLI and MCP/tool-catalog contract: none. `doctor --json` output shape unchanged; only the identity values for unqualified scopes are now correct. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `src/application/Doctor.test.ts` (unqualified `--skill` and audit-wide scopes pass an undefined client selection to `installedSkillIdentity`); `tests/boundary/filesystem/setupSkill.test.ts` (new "unqualified doctor skill identity" block: installed skill found for bare `doctor --skill`; empty `.cursor` marker ignored by plain doctor; empty home still reports missing). - Base reproduction or other evidence: reproduced both failure modes on pristine main with the issue's CLI steps in an isolated HOME (bare `--skill` reported missing; plain doctor with an empty `~/.cursor` reported stale). After the fix, the same steps report aligned with version 42 and tool count 207, and the required `skill:identity` check passes. Negative controls verified through the CLI: a tampered managed file still reports stale with a failing check; a setup refresh restores aligned; an empty home still reports missing with a failing check. - User-visible CLI/MCP output (if applicable): `doctor --skill --json` now shows `"state": "aligned"`, `"installed_version": "42"`, `"installed_tool_count": 207` for the issue's setup. - Remaining proof gaps: no macOS or Windows run; Linux only. No real Hopper/Ghidra involvement (not applicable to this change). For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run src/application/Doctor.test.ts` — 31 passed - `npx vitest run tests/boundary/filesystem/setupSkill.test.ts -t "unqualified doctor skill identity"` — 3 passed (all 3 failed on pristine main before the fix) - `npx vitest run tests/boundary/filesystem/setupSkill.test.ts` (full file) — 12 passed, 3 failed; the 3 failures are pre-existing on pristine main (verified by stashing the change and re-running: same 3 failures in "canonical skill transaction") - `npm run check:fast` — typecheck and lint passed - `npm run verify:test-discovery` — all 1262 test files discovered exactly once - Issue reproduction script with isolated HOME, before and after the fix — both failure modes reproduced on base, resolved with the fix, negative controls hold ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux CLI verification only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; the change only alters which installed skill destinations an unqualified doctor check inspects. ## Author readiness Human owner: <!-- @GitHub-handle --> - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare the current revision ready for maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:08:49 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#5706
No description provided.