Skip to content

fix(cli/doctor-pi): skip unrelated local packages when probing embedding runtime - #414

Merged
ualtinok merged 3 commits into
cortexkit:masterfrom
qsgy-edge:fix/doctor-pi-embedding-candidate-filter
Sep 14, 2026
Merged

ualtinok merged 3 commits into
cortexkit:masterfrom
qsgy-edge:fix/doctor-pi-embedding-candidate-filter

Conversation

@qsgy-edge

@qsgy-edge qsgy-edge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem

Pi doctor probes unrelated local-path extensions for embedding dependencies, and an earlier broken or WASM-only tree can mask a later healthy native installation. Its duplicate-load diagnostic also misses npm + local Magic Context registrations, and when all candidates are broken it reports only the first failure.

Changes

  • Identify local Magic Context directories by the @cortexkit/pi-magic-context name in package.json; share local-path resolution between embedding candidate discovery and duplicate-load/other-extension diagnostics, including object-form { source } entries.
  • Continue past broken and WASM-fallback candidates to find a healthy native installation.
  • When no working candidate exists, report every broken candidate's path and runtime failure details.
  • Report npm + local Magic Context registrations as duplicate loading, while retaining a healthy embedding PASS when a working candidate is available.
  • Add regression coverage for unrelated packages, candidate precedence, all-broken diagnostics, and string/object local identities. Format both changed files with the repository-installed Biome.

Rebased onto master 7680bd8fd06f015a905b7e00f37fa107754f84ba.

Validation

Windows, Bun 1.3.14:

  • Six targeted candidate/identity regressions: 6 pass, 0 fail, 23 expectations. The three newly added cases failed before the fix and pass afterward.
  • CLI bun run typecheck and git diff --check: pass.
  • Repository Biome check src against a temporary LF-normalized copy: 91 files pass. Ordinary Windows checkout lint reports CRLF formatting on 89 untouched files; no unrelated files were reformatted in this PR.
  • Doctor tests: 22 pass, 3 fail. All three failures reproduce on clean master (details below).
  • CLI bun run test: 345 pass / 5 skip / 31 fail, compared with clean master 339 pass / 5 skip / 31 fail; failure-name sets are identical.
  • The runner stops before four isolated test files after the initial failure. Each was also run separately on both trees: repair-db 3 pass / 2 skip / 1 fail; omp-helpers 6 pass / 2 fail; opencode-helpers 4 pass / 4 skip / 0 fail; pi-helpers 7 pass / 1 fail. Results match master.
  • Aggregate executed CLI tests: 365 pass / 11 skip / 35 fail, versus master 359 pass / 11 skip / 35 fail.

Reproduced doctor failures on Windows

  1. leaves an older supported shared DB schema unchanged: cleanup rmSync fails with EBUSY: resource busy or locked on the temporary DB directory.
  2. migrates legacy Pi user config before --force writes a default: expected protected_tokens to be 13; received undefined.
  3. does not write a default when legacy user configs conflict: expected the target config to be absent (false); received true.

The complete repository gates are not green on this Windows host. These failures match the tested master baseline; their root causes have not been exhaustively diagnosed. macOS/Linux were not run locally in this follow-up.

Dependency setup used the frozen lockfile with install scripts disabled after the configured mirror failed to provide an unrelated OpenCode Windows binary. The lockfile is unchanged.

Greptile Summary

The PR fixes Pi doctor’s embedding-runtime candidate selection and completes the prior fallback-scanning fix.

  • Filters local package entries by the @cortexkit/pi-magic-context package identity.
  • Continues past broken and WASM-fallback trees so a later native-capable installation can pass.
  • Adds regression coverage for unrelated, broken, WASM-only, and managed installation combinations.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/cli/src/commands/doctor-pi.ts Filters local candidates by package identity and defers degraded results while searching for a healthy native runtime.
packages/cli/src/commands/doctor-pi.test.ts Adds focused regression coverage showing that unrelated and degraded local trees no longer mask a healthy managed installation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Pi packages and managed roots] --> B[Build plugin candidates]
    B --> C{Magic Context package?}
    C -->|No| D[Skip candidate]
    C -->|Yes| E[Probe embedding runtime]
    E -->|Native OK| F[Report PASS and stop]
    E -->|WASM fallback| G[Remember fallback and continue]
    E -->|Broken| H[Remember warning and continue]
    E -->|Unknown| I[Remember reason and continue]
    G --> J{More candidates?}
    H --> J
    I --> J
    J -->|Yes| E
    J -->|No| K[Report best deferred result]
Loading

Reviews (3): Last reviewed commit: "fix(cli/doctor-pi): report broken candid..." | Re-trigger Greptile

Comment thread packages/cli/src/commands/doctor-pi.ts

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/cli/src/commands/doctor-pi.test.ts
Comment thread packages/cli/src/commands/doctor-pi.ts
@qsgy-edge

Copy link
Copy Markdown
Contributor Author

Both review findings addressed in 296e340:

  1. WASM fallback no longer masks a native-capable install (Greptile P1 + cubic P2): the loop previously break-ed at the first candidate with a working WASM fallback, so an earlier WASM-only dev tree hid a later healthy managed install. Now the fallback result is recorded (firstFallback) and probing continues; the fallback WARN is only reported when no candidate is fully OK.

  2. Regression test actually detects the skip (cubic P2): the original test could not fail if isPiMagicContextPackageDir were reverted, because the healthy managed install masked the unrelated package. Added reports unverified, not a broken-runtime WARN, when only unrelated local packages are registered — with only an unrelated package registered and no magic-context tree, doctor must report selected runtime unverified rather than blaming the unrelated package for missing onnxruntime deps.

Also added prefers a later native-capable install over an earlier WASM fallback to lock in finding #1.

Test run: 20 tests, 17 pass, 3 pre-existing failures (DB schema / legacy config migration, reproduced on clean master). tsc --noEmit clean.

@magic-alfonso magic-alfonso Bot 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.

Thanks for isolating this — the premise holds (on master piPluginDirCandidates probes every local-path entry and aborts on the first broken one), the package-name filter is the right direction, and your regression is red on master. Three asks before merge:

  1. Pick the best result across all candidates, native OK > WASM OK > broken. The loop still stops on an earlier wasm-fallback, so an earlier local tree with a broken native binding but working WASM makes a later healthy native managed install report as degraded (greptile's P1 — I reproduced it with a variant of your test). Please add that exact regression: broken-native + working-WASM candidate first, healthy native candidate later, assert the native PASS.

  2. When every candidate fails, name every candidate. Only firstBroken survives into the warning today; the all-broken message should list each path with its native/WASM reason so the user can tell which tree is stale.

  3. Treat a local dev path as a second loaded identity. Pi keys npm:@cortexkit/pi-magic-context and a local directory whose package.json carries that name as different identities and loads both; the duplicate-load conflict check still only recognizes the npm spelling, so the doctor output should not imply the managed install is the sole runtime that will load. Please make the conflict diagnostic identity-aware and cover the local+npm case.

One formatter violation at :801-803 — run the repo's pinned biome (packages/cli/node_modules/.bin/biome check). On this Mac the CLI suite is 385/0 on your head; I could not verify the three Windows-only failures you mention, so if they are real please paste them.

@coleleavitt

Copy link
Copy Markdown
Contributor

Rechecked 296e340e. The follow-up correctly resolves the native-over-WASM selection finding, and the original unrelated-package regression is now meaningful. Two correctness gaps remain.

  1. The all-broken path retains only firstBroken, so later stale/broken candidate paths and reasons are discarded.
  2. Duplicate-load detection still uses specifier-only isPiMagicContextPackageEntry. A local path whose package.json names @cortexkit/pi-magic-context is classified as an unrelated extension, so npm + local Magic Context entries can load twice while doctor reports no conflict. Please share identity-aware classification and add npm+local plus multiple-broken regressions.

The current head also fails the repository lint gate on formatting in both changed files. Typecheck passes, and the focused doctor test passes 20/20 (86 expectations). The branch is hundreds of commits behind current master and GitHub currently shows reviewer/security checks rather than repo CI. Recommendation: keep open, rebase, fix those gaps, and rerun CLI lint/typecheck/tests before merge.

…ing runtime

piPluginDirCandidates treated every non-npm: entry in Pi packages[] as a
candidate plugin tree, so local dev-path extensions (any package.json,
regardless of name) were probed for the embedding runtime. The first broken
candidate made doctor report 'native runtime and WASM fallback both
unavailable' and stop, even when the real magic-context install was healthy.

Now only directories whose package.json names @cortexkit/pi-magic-context
qualify as candidates, and broken candidates no longer abort the scan —
a stale local dev tree cannot mask a healthy managed install.

Repro: register any local-path Pi extension (D:\repo\my-extension) in
settings.json packages[], run 'doctor --harness pi' — doctor blamed the
extension's package.json for missing onnxruntime-web deps instead of
reporting the actual plugin install.
…nstall

Address review findings (Greptile P1, cubic-dev-ai P2): the loop still
stopped at the first candidate with a working WASM fallback, reporting a
degraded runtime even when a later managed install had the native binding.
Record the best degraded candidate and keep probing; only report the
fallback WARN when no candidate is fully OK.

Tests: add regression coverage for (1) unrelated local packages never
probed (unverified, not a broken-runtime WARN, when only unrelated
packages are registered), and (2) a WASM-only dev tree not masking a
later native-capable install.
@qsgy-edge
qsgy-edge force-pushed the fix/doctor-pi-embedding-candidate-filter branch from 296e340 to 8ba8512 Compare September 14, 2026 02:48
@qsgy-edge

Copy link
Copy Markdown
Contributor Author

Addressed the two remaining findings in 8ba8512 and rebased onto master 7680bd8.

  • All-broken diagnostics: retain each failed candidate and report its path with its runtime failure details, instead of retaining only firstBroken.
  • Local loading identity: share local package.json classification between embedding candidates, duplicate-load checks, and other-extension reporting. npm + local registrations now fail the conflict check, including relative string and { source } entries.
  • Added three regressions that fail before this fix and pass afterward. The six candidate/identity regressions now pass (23 expectations). Existing npm+local tests now expect the conflict exit code while still asserting a healthy embedding PASS.

CLI typecheck and diff checks pass. Repository Biome passes all 91 CLI files in an LF-normalized temporary copy; the Windows checkout itself produces CRLF-only lint diagnostics in 89 untouched files. Both PR files were formatted with the repository-installed Biome.

Here are the requested Windows doctor failures, each reproduced independently on clean master 7680bd8:

leaves an older supported shared DB schema unchanged
  afterEach rmSync: EBUSY: resource busy or locked (temporary DB directory)

migrates legacy Pi user config before --force writes a default
  protected_tokens: expected 13, received undefined

does not write a default when legacy user configs conflict
  target config exists: expected false, received true

Full doctor: 22 pass / 3 fail. Full CLI runner: 345 pass / 5 skip / 31 fail versus master 339 pass / 5 skip / 31 fail, with identical failure-name sets. I also ran all four isolated test files that the runner skips after a failure; their results match master. Aggregate coverage is 365 pass / 11 skip / 35 fail versus master 359 pass / 11 skip / 35 fail. The PR description contains the per-file breakdown and dependency setup caveat.

I am not claiming green Windows repository gates or local macOS/Linux validation. Please re-review the two fixes; cross-platform CI/review is still needed.

@magic-alfonso magic-alfonso Bot 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.

Re-reviewed 8ba8512 by execution on macOS. All three asks are closed:

  1. Candidate precedence: the loop now continues past a WASM fallback and past broken trees, keeps the first fallback only as the degraded result, and a later healthy native install wins. The new regression "prefers a later native-capable install over an earlier WASM fallback" is the exact case from the first review.
  2. All-broken reporting: every failed candidate is retained with its path and its native/WASM reason, one warning per candidate ("reports every broken candidate with its native and WASM reasons").
  3. Duplicate-load identity: a local directory whose package.json names @cortexkit/pi-magic-context (string or { source } form, absolute or agent-dir-relative) now counts as a loaded Magic Context identity in both candidate discovery and the conflict check, so npm + local registrations report the conflict while a healthy embedding PASS is still reported.

Gates on the head: CLI typecheck clean, pinned Biome clean on both files, doctor-pi 25/0, full CLI suite 379 pass / 2 skip / 0 fail (the skips are Windows-only). The three Windows failures you listed reproduce on clean master per your run and are unrelated to this change; they are ours to chase separately. Merging. Thanks for the careful follow-through across three rounds.

@ualtinok
ualtinok merged commit e14e3da into cortexkit:master Sep 14, 2026
7 checks passed
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.

3 participants