feat(runner): isolate runtime deps to fix workspace monorepo failures#1381
Draft
Michael Price (michael-pr) wants to merge 10 commits into
Draft
Conversation
WalkthroughThis PR replaces the per-project Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domains/runner/initFlowRuntime.ts (1)
116-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
depsRootin the init cache keyLine 120 keys
initCacheonly bystartDir, but Line 122 passesoptions.depsRootintodoInit. Calls from the same directory with differentdepsRootvalues can incorrectly reuse the first initialization and load the wrong runner.Suggested fix
export function initFlowRuntime( flowPath: string, options: InitFlowRuntimeOptions, fs: Fs = makeDefaultFs(), ): Promise<void> { const startDir = path.dirname(flowPath); + const depsKey = + options.depsRoot === undefined ? "" : path.resolve(options.depsRoot); + const cacheKey = `${startDir}::${depsKey}`; // Cache key is startDir, not fs — tests reusing the same startDir must call // _resetInitCache() between runs. Timeout is omitted deliberately: it is a // single run-global flag, so every flow in a process shares one value. - let p = initCache.get(startDir); + let p = initCache.get(cacheKey); if (!p) { p = doInit(flowPath, options.timeout, fs, options.depsRoot); - initCache.set(startDir, p); + initCache.set(cacheKey, p); } return p; }🤖 Prompt for 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. In `@src/domains/runner/initFlowRuntime.ts` around lines 116 - 123, The initCache key in the initFlowRuntime function only includes startDir, but doInit receives both startDir and options.depsRoot as parameters. When the same directory is initialized with different depsRoot values, the cache incorrectly reuses the first initialization. Create a composite cache key that combines both startDir and options.depsRoot, then use this composite key in both the initCache.get() call and the initCache.set() call to ensure different depsRoot values for the same directory produce separate cache entries.
🤖 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/commands/install/all.ts`:
- Around line 57-59: Move the deps.ensureRuntimeEnv() call and depsRoot
assignment from its current position at line 57 to after the flow target
detection logic (after the hasWeb and hasAndroid checks around lines 73/82).
This ensures runtime environment resolution only occurs when there are actually
installable flows (web or android targets present), preventing unnecessary
managed installs or install failures when only iOS flows or no flows exist.
Update any references to depsRoot to use the value obtained from the deferred
ensureRuntimeEnv call.
In `@src/commands/install/browsers.ts`:
- Around line 21-41: The runtime environment resolution via ensureRuntimeEnv is
being called too early before confirming whether browser flows actually exist.
Move the ensureRuntimeEnv call and the subsequent resolvePlaywrightCli call to
execute lazily after browser targets have been confirmed to exist. This defers
unnecessary work when browser collection is a no-op and prevents offline
failures on otherwise skippable runs. Identify where browser targets are
actually validated in the flow and relocate the depsRoot resolution and runtime
environment setup to occur only after that point.
In `@src/domains/runtimeEnv/ensureRuntimeEnv.ts`:
- Around line 64-65: After the install(managed) call completes successfully, add
a validation check to ensure all pinned dependencies are actually resolved
before returning installed: true. Use the allPinnedResolved function to verify
that the install operation fully materialized the pinned deps for the managed
depsRoot. If the validation check fails, throw an error or fail fast to prevent
reporting success when dependencies were not fully installed.
In `@src/domains/runtimeEnv/installPinned.ts`:
- Around line 35-40: The readiness check in the `installPinned` function at line
35-40 uses `existsSync` to verify that `node_modules/.bin/playwright` exists,
but this is insufficient because the binary could exist while pinned versions
are missing or mismatched. Replace the `existsSync` check on the playwright
binary path with a call to `allPinnedResolved` to properly validate that all
pinned dependencies are correctly installed and resolved. Apply the same fix to
the duplicate check mentioned at lines 64-67.
- Around line 52-54: The error message in the Error constructor that handles
failed managed runtime installation is directly interpolating raw stderr output
from npm, which can expose sensitive information. Replace the raw
`result.stderr.trim()` in the thrown error message with a generic sanitized
error summary. If detailed stderr information is needed for debugging, move the
raw stderr to a separate debug logging call using controlled logging paths,
keeping the thrown error message safe and user-facing while preserving debugging
capabilities.
In `@src/domains/runtimeEnv/resolvePinned.ts`:
- Around line 38-40: In the resolvePinned.ts file, the fs.existsSync check that
verifies the playwright binary existence only looks for the extension-less
playwright script. Modify this condition to check for both the extension-less
variant and the .cmd variant (playwright.cmd) since Windows npm/bun
installations create the .cmd shim alongside or instead of the extension-less
version. Update the condition to return false only if neither variant exists,
allowing the check to properly detect Playwright installations on both Windows
and POSIX systems.
---
Outside diff comments:
In `@src/domains/runner/initFlowRuntime.ts`:
- Around line 116-123: The initCache key in the initFlowRuntime function only
includes startDir, but doInit receives both startDir and options.depsRoot as
parameters. When the same directory is initialized with different depsRoot
values, the cache incorrectly reuses the first initialization. Create a
composite cache key that combines both startDir and options.depsRoot, then use
this composite key in both the initCache.get() call and the initCache.set() call
to ensure different depsRoot values for the same directory produce separate
cache entries.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63fe5f4a-7d7c-4936-a062-1d63212bee77
⛔ Files ignored due to path filters (1)
src/commands/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (47)
knip.config.tssrc/commands/doctor/handler.tssrc/commands/flows/hybridRun.test.tssrc/commands/flows/hybridRunDefaults.tssrc/commands/flows/run.register.tssrc/commands/flows/runDefaults.handle.test.tssrc/commands/flows/runDefaults.reporterWiring.test.tssrc/commands/flows/runDefaults.tssrc/commands/install/all.test.tssrc/commands/install/all.tssrc/commands/install/android.tssrc/commands/install/browsers.tssrc/core/messages/flows.tssrc/core/messages/runner.tssrc/core/paths.tssrc/domains/flows/ensureDeps.test.tssrc/domains/flows/ensureDeps.tssrc/domains/runner/createRunner.guards.test.tssrc/domains/runner/createRunner.test.tssrc/domains/runner/initFlowRuntime.test.tssrc/domains/runner/initFlowRuntime.tssrc/domains/runner/loadFlowDefault.test.tssrc/domains/runner/loadFlowDefault.tssrc/domains/runner/runAndroidFlow.test.tssrc/domains/runner/runAndroidFlow.tssrc/domains/runner/runAndroidFlowDeps.tssrc/domains/runner/runInternals.tssrc/domains/runner/runWebFlow.fixtures.tssrc/domains/runner/runWebFlow.tssrc/domains/runner/runWebFlowDeps.tssrc/domains/runner/runnerDeps.test.tssrc/domains/runner/runnerDeps.tssrc/domains/runner/types.tssrc/domains/runtimeEnv/ensureRuntimeEnv.test.tssrc/domains/runtimeEnv/ensureRuntimeEnv.tssrc/domains/runtimeEnv/index.tssrc/domains/runtimeEnv/installPinned.test.tssrc/domains/runtimeEnv/installPinned.tssrc/domains/runtimeEnv/managedEnvDir.test.tssrc/domains/runtimeEnv/managedEnvDir.tssrc/domains/runtimeEnv/pinnedPackages.tssrc/domains/runtimeEnv/resolveDepsRootIfPresent.test.tssrc/domains/runtimeEnv/resolveDepsRootIfPresent.tssrc/domains/runtimeEnv/resolvePinned.test.tssrc/domains/runtimeEnv/resolvePinned.tssrc/domains/runtimeEnv/shimDeps.test.tssrc/domains/runtimeEnv/shimDeps.ts
💤 Files with no reviewable changes (2)
- src/core/messages/flows.ts
- src/domains/flows/ensureDeps.ts
Contributor
Author
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
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/commands/doctor/handler.ts`:
- Around line 44-47: The issue is in the resolveDepsRootIfPresent call where
when projectDir is undefined, an empty object is passed as the second argument
instead of providing a fallback. When resolveProjectDirSafe returns undefined,
you should provide the current working directory (cwd) as a fallback value for
projectDir in the ternary operator passed to resolveDepsRootIfPresent, so that
it can still probe for dependencies in the current workspace instead of skipping
the project probe entirely and only checking managed runtime.
In `@src/domains/flows/ensureDeps.ts`:
- Around line 43-46: The catch block around the resolveUniqueEnvDir function
call is too broad and silently masks all errors, including unexpected filesystem
or logic errors. Narrow the catch block to only handle the expected
multi-package conflict error by checking the error message or type before
returning undefined. For unexpected errors, re-throw them so they surface as
actual failures rather than silently falling back to managed runtime. Ideally,
work with the resolveUniqueEnvDir function to use a dedicated error class or
code for the multi-package conflict scenario and match against that specific
error type instead of catching all exceptions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd85e8fb-94b8-4eef-9d45-fa9b8af7fefa
📒 Files selected for processing (29)
src/commands/doctor/handler.tssrc/commands/flows/buildFlowsRunDeps.tssrc/commands/flows/hybridRun.test.tssrc/commands/flows/hybridRunDefaults.tssrc/commands/flows/runDefaults.handle.test.tssrc/commands/flows/runDefaults.reporterWiring.test.tssrc/commands/flows/runDefaults.tssrc/commands/install/all.test.tssrc/commands/install/all.tssrc/commands/install/android.tssrc/commands/install/browsers.fixtures.tssrc/commands/install/browsers.tssrc/commands/resolveDepsRoot.test.tssrc/commands/resolveDepsRoot.tssrc/domains/flows/ensureDeps.tssrc/domains/install/android/index.tssrc/domains/install/android/installAndroid.test.tssrc/domains/install/browsers.tssrc/domains/runner/loadFlowDefault.tssrc/domains/runner/runInternals.tssrc/domains/runtimeEnv/ensureRuntimeEnv.test.tssrc/domains/runtimeEnv/ensureRuntimeEnv.tssrc/domains/runtimeEnv/index.tssrc/domains/runtimeEnv/installPinned.test.tssrc/domains/runtimeEnv/installPinned.tssrc/domains/runtimeEnv/resolveDepsRootIfPresent.tssrc/domains/runtimeEnv/resolvePinned.test.tssrc/domains/runtimeEnv/resolvePinned.tssrc/domains/runtimeEnv/shimDeps.ts
💤 Files with no reviewable changes (1)
- src/domains/runtimeEnv/index.ts
Contributor
Author
✅ Manual verification — monorepo / multi-package compatibilityReproduced the reported issue ( Headline before/after — workspace monorepo, flows at repo root, no playwrightBEFORE ( …and on retry it "worked" only by installing 587 MB of AFTER (this branch): Full scenario matrix (all on this branch's binary)
What proves the issue won't recur
Backwards-compatibility & tests
Verification repos and captured output retained locally under |
Contributor
Author
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/domains/runtimeEnv/managedEnvDir.test.ts`:
- Around line 42-46: The test assertion on the line with
expect(managedEnvDir()).toBe() is comparing against join("/custom/cache", hash)
but managedEnvDir() actually returns join(resolve("/custom/cache"), hash) as
shown in managedEnvDir.ts line 30. On Windows, resolve() adds a drive prefix
which causes the path mismatch. Update the expectation to use
resolve("/custom/cache") when calling join to match the actual behavior of
managedEnvDir() function and ensure the assertion correctly validates the
implementation across all platforms.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee675fa0-4627-4bb1-896b-23a82ad0c3e8
⛔ Files ignored due to path filters (1)
src/commands/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
src/commands/flows/hybridRunDefaults.tssrc/commands/flows/run.register.tssrc/commands/flows/runDefaults.handle.test.tssrc/commands/flows/runDefaults.tssrc/core/messages/runner.tssrc/domains/runtimeEnv/index.tssrc/domains/runtimeEnv/managedEnvDir.test.tssrc/domains/runtimeEnv/managedEnvDir.ts
Contributor
Author
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
The managed runtime lives at <data-dir>/runtime/<hash> with no in-CLI way to clear it, forcing a hand-typed `rm -rf` that risks deleting the wrong path. Add `qawolf install clear`: a destructive, confirmed command that removes the whole runtime base dir (honoring QAWOLF_RUNTIME_DIR), with a `--yes` flag and structured json/agent output.
The destructive confirm used Clack's selectKey, which renders the message inline on the prompt line. The embedded newline (path on its own line) broke the framed timeline and garbled the y/n keystroke prompt so it could not be answered. Move the path into a Clack note box and keep the confirm message to a single line, matching the destructive-confirm pattern in init.
The destructive confirm used Clack's selectKey, a single-keystroke hotkey prompt with no up/down navigation: arrows did nothing and Enter cancelled instead of submitting, so the prompt felt frozen. Replace it with Clack's standard arrow-navigable confirm, starting the cursor on No so a stray Enter stays safe. Removes the selectKey path entirely; also fixes the same prompt in init and flows pull.
Wrap the removal in withProgress so the user gets immediate feedback while the directory is deleted, matching auth logout. Drop the path from the final message — the confirm note already shows it (human) and the structured output carries it in `dir` (json/agent).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to WIZ-10907
Overview of Changes
Running
qawolf flows runcould fail with "Could not find Playwright", and more broadly, users don't want the CLI installing its dependencies into their own project. The cause: the CLI installed its runtime (Playwright, etc.) into the user's project, and in workspace monorepos (pnpm/yarn/npm) those installs get hoisted to the repo root — leaving the package's ownnode_modulesempty, so resolution failed.This PR moves the runtime into an isolated location the CLI owns (
<data-dir>/runtime/<version-hash>), installed once on demand. The user's project is never written to. If a project already has the exact runtime installed it's reused as-is; otherwise the isolated dir is used.QAWOLF_RUNTIME_DIRrelocates that dir (e.g. CI / read-only home), and--deps <dir>lets you supply your own.Out of scope (follow-ups): resolving a bare
import "playwright"inside a flow in the binary, and GC of stale runtime dirs after a version bump.Testing
node_modules), runqawolf flows run <pattern>— the flow passes, a note confirms the managed runtime is in use, and nothing is written into the repo.qawolf flows run --deps <prepared-dir> <pattern>— uses the given dir, installs nothing.QAWOLF_RUNTIME_DIR=<dir> qawolf flows run <pattern>— installs/uses the runtime under that dir.qawolf doctor— reports the managed runtime location without installing.Checklist
🤖 Generated with Claude Code