fix(cli): unify the "tool not found" remedies on qawolf install - #1416
Conversation
Six messages described one state — a pinned binary missing from the resolved deps root — and gave four remedies between them. Two told the user to `npm install playwright`, which is wrong: the CLI pins and installs playwright and appium itself via ensureRuntimeEnv, so the user's project never needs a copy. The remedy now follows the caller, not the tool. `qawolf doctor` is diagnostic and says `qawolf install`. Callers reached after the deps root resolved cannot repeat that step, so they name the incomplete directory and say `qawolf install clear`. Every message names the path it looked for, including the two Playwright ones that named none. Collapses doctor's appium.noEnvDir and appium.binaryMissing into one appium.notFound: "no env dir" is the no-path case of the same message. Closes WIZ-11284
allPinnedResolved required the playwright shim but checked only appium's package version. A matching version does not imply a runnable shim, so a partial install resolved as complete and `flows run` then failed to spawn appium — after install had reported success. Requiring both shims makes that state self-heal: a missing shim now invalidates the root, so ensureRuntimeEnv reinstalls it or falls through to a fresh managed dir.
The shim fix makes an incomplete-but-resolved deps root unreachable, so the two-variant split from the previous commit no longer earns its keep. Every site now gives one remedy, `qawolf install`, and names the path it looked for. Also covers four load-failure sites of the same class that the ticket missed. @qawolf/emails and @qawolf/testkit both told the user to run `npm install` for packages the CLI pins and installs itself. Refs WIZ-11284
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds shared remediation helpers for missing or unloadable runtime tools and applies them across doctor checks, installation flows, runners, and SDK loaders. Playwright and Appium diagnostics now include candidate paths where available. Pinned environment resolution now requires both Playwright and Appium CLI shims, with updated test fixtures for Linux and Windows. New tests verify standardized messages and preserved load-failure causes. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant configureEmails
participant packageLoadFailed
participant toolNotRunnable
participant Error
configureEmails->>packageLoadFailed: package name, env directory, load detail
packageLoadFailed->>toolNotRunnable: package load description and detail
toolNotRunnable-->>packageLoadFailed: standardized remediation message
packageLoadFailed-->>configureEmails: formatted failure message
configureEmails->>Error: throw with original cause
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/messages/toolNotFound.test.ts`:
- Around line 5-32: Add a direct test case in the existing message tests for
packageLoadFailed, asserting its output includes the package name, environment
path, original failure detail, and the shared qawolf install remedy. Follow the
existing toolNotInstalled and toolNotRunnable assertion style.
In `@src/domains/runtimeEnv/resolvePinned.test.ts`:
- Around line 122-128: Rename the Windows-specific tests around the pinned
package resolution cases to explicitly mention both required shims, playwright
and appium, for the .cmd and .exe variants. Update only the test titles,
including the case around the second referenced location, while preserving their
setup and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a78e8403-70eb-4f07-83bc-c451461c0bbe
📒 Files selected for processing (27)
src/commands/resolveDepsRoot.test.tssrc/core/messages/doctor.tssrc/core/messages/index.tssrc/core/messages/install.tssrc/core/messages/runner.tssrc/core/messages/toolNotFound.test.tssrc/core/messages/toolNotFound.tssrc/domains/doctor/checks/androidAppium.test.tssrc/domains/doctor/checks/androidAppium.tssrc/domains/doctor/checks/playwright.test.tssrc/domains/doctor/checks/playwright.tssrc/domains/emails/configureEmails.test.tssrc/domains/emails/configureEmails.tssrc/domains/install/android/driver.test.tssrc/domains/install/browsers.test.tssrc/domains/install/browsers.tssrc/domains/runner/runWebFlowDeps.test.tssrc/domains/runner/runWebFlowDeps.tssrc/domains/runtimeEnv/ensureRuntimeEnv.test.tssrc/domains/runtimeEnv/installPinned.test.tssrc/domains/runtimeEnv/installPinned.tssrc/domains/runtimeEnv/resolveDepsRootIfPresent.test.tssrc/domains/runtimeEnv/resolvePinned.test.tssrc/domains/runtimeEnv/resolvePinned.tssrc/shell/appium/createAppiumServer.test.tssrc/shell/testkit.test.tssrc/shell/testkit.ts
The Windows shim tests seed both shims but their names claimed only playwright, so a failure pointed at the wrong contract. The absent-shim test now seeds the other shim, so it fails for the reason it names.
2 participants
Relates to WIZ-11284
Overview of Changes
Ten messages described one state — a pinned binary missing from the resolved deps root — and gave four different remedies between them. Two told the user to
npm install playwright, which is wrong: the CLI pins and installs playwright and appium itself, so a user's project never needs a copy. Every site now names the path it looked for and gives one remedy,qawolf install.Getting there needed a resolution fix first.
allPinnedResolvedrequired the playwright shim but checked only appium's package version, and a matching version does not imply a runnable shim — so a partial install resolved as complete andflows runfailed to spawn appium after install had reported success. Requiring both shims makes that state self-heal, which is what lets every caller give the same remedy instead of splitting on whether it can safely say "runqawolf install". Kept as three commits: the original two-variant pass, the shim fix, then the collapse.The ticket listed five messages; the sweep found ten.
@qawolf/emailsand@qawolf/testkitcarried the same wrongnpm installadvice for packages the CLI pins, and neither had a test.moduleNotFoundHintkeeps itsnpm installadvice on purpose — that one is about a user's own flow dependency.Testing
All ten failure paths were driven against an empty deps directory and the rendered output read by hand, not just asserted in tests. Four seams gained first-time coverage:
installBrowserList,defaultRunWebFlowDeps,configureEmails, andconfigureTestkit.Checklist
allPinnedResolvedis stricter, so a deps root whose appium shim is missing but whose package version matches no longer resolves. That root previously produced a runtime spawn failure, so the change trades a late failure for an install. This also tightens whatqawolf flows run --deps <dir>accepts: a prepared directory now needs the appium shim, not just a matching appium version.ensureRuntimeEnv.ts:56rejects such a directory without naming the missing packages — left out of scope and tracked separately.