Skip to content

fix(doctor): stop reporting a healthy Yarn PnP project as broken - #5

Merged
robmclarty merged 1 commit into
robmclarty:mainfrom
robmclarty-minga:fix/doctor-yarn-pnp
Jul 30, 2026
Merged

fix(doctor): stop reporting a healthy Yarn PnP project as broken#5
robmclarty merged 1 commit into
robmclarty:mainfrom
robmclarty-minga:fix/doctor-yarn-pnp

Conversation

@robmclarty-minga

Copy link
Copy Markdown
Contributor

Stacked on #4. This branch is based on fix/resolve-slot-tools-locally, because the PnP detection belongs in the src/pm/tools.ts resolver that PR introduces. A fork PR's base has to be a branch in this repo, so I couldn't target #4 directly — hence this PR shows both commits. Merge #4 first and this collapses to the single fix(doctor) commit. Review only 9511268 if #4 is already reviewed.

The problem

A Yarn Plug'n'Play project has no node_modules/ at all — the install artifact is .pnp.cjs, and binaries resolve through Yarn's runtime rather than a node_modules/.bin directory. Both of doctor's install questions were path tests against a tree that layout never produces:

  • checkInstall asserted node_modules/ exists → reported install: missing, found "lockfile only"
  • probeTool looked for node_modules/.bin/<tool> → reported every tool missing

Both rows are required: true, so doctor exits 1 and declares the environment broken — on a repo where every tool is installed and every check passes. Demonstrated on a real Yarn 4 PnP fixture with markdownlint-cli2 genuinely installed:

package manager: yarn (detected)
✘ install                lockfile only  (node_modules/ populated)
    -> Run `yarn install` from the repo root.
✘ docs   markdownlint-cli2  default  not installed
✘ environment has problems (see above)

...while the gate on the same repo, same moment:

✔ docs        453ms  Markdown linting
✔ all checks passed

The two commands contradict each other, and #4 sharpens that: it makes the run path deliberately PnP-safe (the pre-flight is scoped to the npm/bun launcher caches, and yarn <tool> resolves through the PnP runtime) while leaving doctor's assumption in place. I flagged this while writing #4 but it never made it into that PR body — my omission.

The change

Detect the layout, then ask the question that layout can answer:

  • install is satisfied by .pnp.cjs plus the lockfile. Reports .pnp.cjs + lockfile (Yarn PnP).
  • A slot's tool is resolved with yarn bin <tool>, which prints the path and exits 0, or exits 1 when the tool isn't a dependency — so the exit code carries the whole answer. Shares the version probe's timeout, since this spawns a package manager and a hung doctor is worse than an unresolved tool.

isPnPInstall lives in src/pm/tools.ts beside the other resolver, so the run path and doctor keep sharing one answer about where a tool lives. It recognises .pnp.cjs, .pnp.js (Yarn 2) and .pnp.data.json, and deliberately does not walk up — PnP is per-project, and a parent's manifest says nothing about how this project is installed.

Two guards worth reviewing specifically:

  1. It must not become optimistic. A tool that genuinely doesn't resolve still reports missing — the PnP path asks a looser question, not a softer one. Trading a false red for a false green would be the worse bug, so there's a dedicated test for it.
  2. Gated on yarn. PnP is a Yarn feature, so a .pnp.cjs left behind by a migration off Yarn can't reroute an npm or pnpm repo into asking npm bin for its tools. Also tested.

Verification

Against a real Yarn 4 PnP fixture (.pnp.cjs, .pnp.loader.mjs, .yarn/, no node_modules), both directions:

doctor run
tool installed ✔ install / ✔ docs / environment ok ✔ docs green
tool removed ✔ install / ✘ docs missing ✘ docs red

Note install correctly stays green in the second row — the project is installed, it's just missing that one tool. The two commands now agree in both directions, which was the whole point.

Plus 7 unit tests: the three PnP manifest names, no-walk-up, the non-PnP layout, PM-resolution success and failure, and the stale-.pnp.cjs guard.

Note on health

The first cut of this pushed probeTool to CRAP 31.6 (cyclomatic 10, cognitive 14) and turned the health slot red. I split it rather than baseline it: probeTool now dispatches on adapter shape and probeExecTool owns the two-layout resolution. health is green.

Same pre-existing failure as #4

pnpm check is green on every slot except test, which fails the same 2 signal-timing tests in src/triage/__tests__/env.test.ts. Those are an artifact of my sandboxed environment: they pass on a normal machine and passed on all four CI jobs for #4 (ubuntu + macos × node 22.18/24). Not related to this change.

Both of doctor's install questions were path tests against a `node_modules/`
tree, which a PnP project does not have. `install` asserted the directory and
reported `lockfile only`; each slot's tool was looked for at
`node_modules/.bin/<tool>` and reported missing — on a repo where every tool is
installed and every check runs green. So `doctor` called the environment broken
while the gate passed, the two commands disagreeing about the same repo.

`install` is now satisfied by `.pnp.cjs` plus the lockfile, and a tool is
resolved with `yarn bin <tool>`, whose 0/1 exit carries the answer. A tool that
genuinely does not resolve still reports missing: the PnP path asks a looser
question, not a softer one — trading a false red for a false green would be the
worse bug.

Detection is gated on yarn, so a `.pnp.cjs` left behind by a migration off Yarn
cannot reroute an npm or pnpm repo into asking `npm bin` for its tools.

Verified against a real Yarn 4 PnP fixture (no node_modules) in both directions:
tool installed -> doctor ok and run green; tool removed -> doctor missing and run
red. `probeTool` was split so the layout dispatch and the resolution logic are
separate functions, keeping it under the health gate's CRAP threshold.
@robmclarty
robmclarty merged commit 0ae85a6 into robmclarty:main Jul 30, 2026
4 checks passed
robmclarty added a commit that referenced this pull request Jul 30, 2026
… the repo

Three defects in the tool-resolution work merged by #4 and #5.

A slot refused for a missing tool carried `exit_code: -1`, which is reserved
for a spawn failure or timeout. `triage` matched it and printed "that is a
harness problem, not a finding" over the one failure the pre-flight exists to
surface, remediation and all. It now exits 1; a triage test pins the two apart.

The upward search for `node_modules/.bin/<tool>` stopped only at the filesystem
root, so a stray install in a parent of the checkout satisfied it — the same
machine-state dependence local resolution was added to remove, reached by
another route. It now stops at the repo root, marked by `.git` or a lockfile,
searching that root before it does.

Under Yarn PnP a timed-out `yarn bin` probe was folded into "not a dependency",
so a slow package manager reported a tool that was already installed as
missing. A timeout is now `unknown`, matching the `--version` probe beside it.
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.

2 participants