Skip to content

fix: align doctor checks with skill lock paths - #434

Open
kkkhs wants to merge 1 commit into
rolecraft-sh:mainfrom
kkkhs:fix/doctor-lock-paths-330
Open

kkkhs wants to merge 1 commit into
rolecraft-sh:mainfrom
kkkhs:fix/doctor-lock-paths-330

Conversation

@kkkhs

@kkkhs kkkhs commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #330.

rolecraft doctor now derives skill directory checks from each lockfile entry's recorded agents, so agent-specific installs such as ~/.claude/skills are not reported missing. It also compares shared skill directory names against normalized lock slugs so scoped slugs are not reported as orphaned.

Validation run:

  • node --test src/api/doctor.test.js
  • node --test src/commands/doctor.test.js
  • PATH=/tmp/rolecraft-gitwrap:$PATH npm test (local Git is 2.20.1, so the wrapper only translates git init -b used by release-prep tests)
  • node --check src/api/doctor.js
  • node --check src/api/doctor.test.js
  • git diff --check
  • GitHub CI: Tests on Node 24/26 for ubuntu/macos and CodeQL passed; Node 24 jobs include npm run lint.

Local npm run lint reaches Biome, but this machine's glibc is 2.28 and the installed Biome binary requires GLIBC_2.29/2.30.

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor

Checklist

  • Tests pass locally (npm test)
  • Lint passes (npm run lint)
  • No new dependencies added
  • Docs updated if needed

Related issue

Fixes #330

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

🎉 Thanks for opening your first pull request in RoleCraft!

A maintainer will review it soon — it helps if npm run lint and npm test pass locally.

@kkkhs
kkkhs force-pushed the fix/doctor-lock-paths-330 branch from c9591a0 to e3e98ea Compare October 6, 2026 12:18
@kkkhs
kkkhs force-pushed the fix/doctor-lock-paths-330 branch from e3e98ea to 27e2021 Compare October 6, 2026 12:20
@kkkhs
kkkhs marked this pull request as ready for review October 6, 2026 12:24
@kkkhs
kkkhs requested a review from sametcelikbicak as a code owner October 6, 2026 12:24
Copilot AI balanced review requested due to automatic review settings October 6, 2026 12:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sametcelikbicak sametcelikbicak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on the orphan check — the name.replace(/-/g, '/') heuristic was genuinely broken, and keying the search dirs off the lock entry's recorded agents is the right shape. Two things before merge.

1. getDirForAgent ignores the cwd argument.

lockEntrySkillDirs receives cwd and uses it for the project entry on the line above, but project-scoped agents resolve through proj() → process.cwd(). The two disagree whenever they differ:

skill really at     /tmp/x/.devin/skills/acme-devin-skill
apiDoctor('/other') → missingDirs: 1

devin, copilot, eve, promptscript and forge all use proj(), so all five are affected. The CLI never shows it — doctorCommand passes process.cwd() — but apiDoctor is exported as doctor from src/index.js, so an API caller passing any other directory gets the same false positive this PR is fixing.

else addDir(getDirForAgent(agent))

Every other path in apiDoctor resolves through the cwd parameter, so this looks like the one place it was missed.

2. This overlaps #332.

#332 ("project-scoped agents write to the global lockfile") is open and touches the same area. Worth deciding together whether they land as one change — the two share a root cause, and splitting them means whoever reviews second inherits a half-migrated function.

Verified on this branch: the #330 fixture reproduces clean — orphaned: 0, missingDirs: 0, hashMismatches: 0 — with a scoped slug (acme/my-skill → acme-my-skill) and a non-markdown file in the tree. npm run lint passes, 1395/1395 tests pass.

This branch has not been deployed

No deployments
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.

fix: doctor reports three false positives (orphan dirs, hashes, missing dirs)

3 participants