fix(resolve): forced PM biases task selection#71
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Semver Version Bump Validation | Source files changed, but Cargo.toml stays at 0.14.3 on both base and HEAD, so there’s no SemVer bump for this bug fix. | Bump the crate version in Cargo.toml from 0.14.3 to the next PATCH release (for example 0.14.4) and include that in the PR. |
✅ Passed checks (7 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title is concise, uses fix:, and matches the forced-PM task-selection bug. |
| Description check | ✅ Passed | The description clearly explains the same conflict-resolution bug and its fix. |
| Linked Issues check | ✅ Passed | The implementation now prefers the forced PM's own task source on conflicts, satisfying #70. |
| Out of Scope Changes check | ✅ Passed | No unrelated changes stand out; the edits all support the forced-PM source-bias fix. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 30.00%. |
| Changelog Update | ✅ Passed | Source files changed in src/, and CHANGELOG.md was updated under ## [Unreleased] with a ### Fixed entry covering the PM-bias change. |
| Agents.Md Documentation Updated | ✅ Passed | No AGENTS.md files exist in the repo, so these CLI/behaviour changes needed no AGENTS update. |
Comment @coderabbitai help to get the list of available commands.
When a task name exists in multiple sources (e.g. package.json:check and deno:check) and the user forces a package manager that is also a distinct task source via --pm deno / RUNNER_PM=deno, source selection kept picking package.json per the default tier and then ran that script through deno (deno task check), which breaks when the script relies on npm lifecycle build artifacts deno cannot honor. Fold a forced-PM source bias into source_priority: a --pm / RUNNER_PM override naming a package manager that owns a distinct task source (today only deno, via PackageManager::distinct_task_source -> DenoJson) wins its own same-name conflicts outright (priority 0); every other source drops one tier. The bias lives inside source_priority so cmd::why and doctor, which render that number, stay truthful with no schema churn. The unforced path is byte-for-byte identical to before, and single-candidate lookups are unaffected, so only genuine conflicts re-order. Verification: cargo build, cargo test --lib (705 pass, 6 new), cargo clippy --all-targets --all-features (exit 0), dprint check (exit 0). End-to-end with the built binary: RUNNER_PM=deno run check dispatches `deno task check`; run check (no override) still selects package.json; run deno:check FQN still works. Closes #70
c88de8a to
b763a72
Compare
|
Closing: this is the wrong fix for a real gap. What this PR does. 1. It special-cases deno. npm, yarn, pnpm, bun and deno all run scripts, and they do not run them the same way (each has its own shell, lifecycle, and runtime semantics). There is no principled reason deno alone should steer task resolution. 2. It re-ranks task selection with the package-manager flag. The real gap underneath. Choosing which tool runs a script is a runner concern, but there is no runner concept for the script-running PMs. The Correct direction. Model npm/yarn/pnpm/bun/deno as runners too and let them be chosen on the runner axis (the same Closing in favor of that. Tracking the runner-axis work separately. |
Generalize the forced-PM source bias from deno-only to every PM. `PackageManager::owned_task_sources` maps each PM to the task source(s) it runs natively, most-native first; `source_priority` ranks a forced `--pm`/`RUNNER_PM`'s owned sources ahead of all others on a same-name conflict. Deno is now one member of the rule, not a special case; a PM owning no modeled source (Bundler, Composer) re-orders nothing. `RUNNER_PM=deno run check` now selects deno.json's task (`deno task check`) instead of running a package.json script through deno and crashing; `--pm bun` likewise pulls package.json to the front. Replaces deno-only `distinct_task_source` with `owned_task_sources`. Closes #70.
|
Reopened with the fix generalized — it now biases same-name task selection toward any forced PM's own source, not deno alone.
709 lib tests pass (5 new generalization tests), clippy + dprint clean. Closes #70. |
--pm deno/RUNNER_PM=deno biases same-name conflicts to deno's task sourceThe #75 branch forked before #71 and #73 merged, so its CHANGELOG was missing their Unreleased entries. Three-way merged master's CHANGELOG.md (per-task chain timing in Added, forced-PM source bias in Fixed) on top of the branch's `run <path>` entry. The changelog now diverges from master by only this PR's entry, so it merges cleanly when #75 lands.
Problem
When a task name exists in multiple sources (e.g.
package.json:checkanddeno:check) and the user forces a package manager that is also a task source via--pm/RUNNER_PM, source selection still resolved the conflict by the default tier (package.jsonwins overdeno) and then ran thatpackage.jsonscript through the forced PM (RUNNER_PM=deno run check→deno task check). That is a broken dispatch even though a nativedeno:checkexists, because deno cannot honor the npm lifecycle/build artifacts the package.json script depends on (e.g.@biomejs/cli-*platform binary never built → "Cannot find module" crash).Forcing a PM should bias task selection toward that PM's own source.
See #70.
Change
Generalize the forced-PM source bias to every package manager — deno is one member of the rule, not a special case.
PackageManager::owned_task_sources()(src/types.rs): a total PM → task-source(s) map, most-native first. npm/yarn/pnpm/bun →[package.json]; deno →[deno.json, package.json](native first, also-runs second); cargo →[CargoAliases]; go →[GoPackage]; uv/poetry/pipenv →[pyproject scripts]; bundler/composer →[](no task source modeled, so they bias nothing). Replaces the deno-onlydistinct_task_source().source_priority(src/cmd/run/select.rs), not added as a new tuple component. A forced PM's owned sources win same-name conflicts ranked by position (native =0, next =1); every other source drops below them (saturating_add(owned.len())). Becausecmd::whyanddoctoralready rendersource_priority, their explanations stay truthful with zero JSON/schema churn.base_source_priority) is byte-for-byte identical to prior behavior; single-candidate lookups are unaffected (only one task to pick); only--pm/RUNNER_PM(the cross-ecosystemoverrides.pm) triggers it.runner.tomlPM overrides (pm_by_ecosystem) andprefer_runnersare untouched.Verification
Gates (all green):
cargo buildcargo test --lib— 709 pass;src/cmd/run/select.rscarries 10 tests (5 new for the generalization: bun-biases-package.json, bun-beats-turbo, deno-falls-back-to-package.json, forced-PM-without-owned-source, bun-priority-zero).cargo clippy --all-targets --all-features(exit 0)dprint check(exit 0 on tracked files)End-to-end with the built binary against a fixture carrying both
package.json:checkanddeno.json:check(runner why check):RUNNER_PM=deno→package.json [priority=1],deno [priority=0]→ selecteddeno(deno task check). Fixes the fix(resolve):--pm deno/RUNNER_PM=denoshould pick deno's task on name conflicts, not run the package.json script through deno #70 crash.--pm bun→package.json [priority=0],deno [priority=3]→ selectedpackage.json(bun run) — generalizes past deno.package.json [priority=1],deno [priority=2]→ selectedpackage.json(unchanged).run deno:checkfully-qualified syntax still works.Out of scope (follow-ups): a dedicated runner axis that accepts the PMs as runners, and lifecycle-script detection.
Closes #70