fix(drivers): name a location when the missing-driver error searched nothing - #1192
Conversation
…nothing Drivers are deliberately not shipped, so "driver not installed" is a normal state users hit by design rather than an edge case. `driverSearchRoots()` returns only directories that exist, so on a machine that has never installed a driver it returns nothing and the error ended in a bare "Searched 0 locations:" — a colon with nothing after it, naming nowhere to look. Say plainly that there was nothing to search, and name the `node_modules` directory the printed install command creates. Resolution behaviour is unchanged; this is the error text and one branch. Two tests: the empty case must not render "Searched 0 locations:" and must name the install directory, and the populated case must still list the roots it actually searched.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
full receipts (1 session)
builder ·
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 31a73e1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 31a73e1)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 4dba9bf)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by deepseek-v4-pro · Input: 42.9K · Output: 13K · Cached: 481.2K Review guidance: REVIEW.md from base branch |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…into codex/pr-1192-merge-ready
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a73e1493
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e005323
into
fix/warehouse-driver-bootstrap
Issue for this PR
Closes #1191.
This PR targets
fix/warehouse-driver-bootstrap(#1122), notmain; it builds on the optional-driver resolver introduced by that PR and includes the latest parent head.Type of change
What does this PR do?
When optional-driver resolution fails and
driverSearchRoots()returns an empty list, the old error ended with a danglingSearched 0 locations:line. The first version of this PR improved that line but overclaimed that the driver directory itself did not exist.The final wording states only what the empty list proves and still gives the user an actionable destination:
When roots exist, the existing
Searched N location(s): ...message is unchanged. Resolution, root precedence, package selection, dynamic import, installation, and permissions are unchanged. In particular, the parent branch deliberately excludes project and ancestornode_modules; it retains the consent-gated managed root and explicitly configured/runtime roots.Review feedback addressed
The empty-list regression test explicitly models the constructor boundary. Forcing
driverSearchRoots()to return[]in a unit-test checkout would be host-dependent because executable- or package-adjacent roots may legitimately exist.A late review found the complementary portability problem in the older non-empty-root test: it asserted
Searchedwithout creating any root, so it could fail in a dependency-free checkout. The final test-only commit creates its own managednode_modulesroot, pointsALTIMATE_DRIVER_DIRat its parent, and asserts that exact root. The direct-constructor test still covers the genuine empty-root wording.Codebase graph tracing confirms that
loadOptionalDriver()passes the exactdriverSearchRoots()array toDriverNotInstalledErrorwithout transformation.Verification
packages/driversfull suitebun run typecheckgit diff --checkThe source branch was synchronized with final #1122 head
c49149a26fusing a normal merge. Final reviewed/pushed head:212f4546a7. No rebase or force-push was used.Screenshots / recordings
Not a UI change.
Checklist
Note
Low Risk
User-facing error text only in the drivers package; no changes to resolution, install paths, or security-sensitive logic.
Overview
When optional driver resolution fails and
driverSearchRoots()returns no existing directories (common on a first compiled run),DriverNotInstalledErrorno longer ends with a uselessSearched 0 locations:line. It now says that no searchable locations were found and points at the expected managednode_modulespath underdriverInstallDir(), while keeping the existingSearched N location(s): …wording when roots were actually checked.The
npm install --prefixhint still uses the same install directory (now via a singleinstallDirvariable). Resolver behavior is unchanged.Tests cover the empty-
searchedmessage directly (host-independent), assert listed roots whensearchedis non-empty, and tighten the integration case so the error mentions the managed root.Reviewed by Cursor Bugbot for commit 212f454. Bugbot is set up for automated code reviews on this repo. Configure here.