fix(runner): validate outer hop node_modules against declared deps#1390
Conversation
Consume PopulateOuterHop's result in prepareRunDir, threading both the result value and onInstallStart callback. Updates PrepareRunDirArgs with optional onInstallStart callback and PrepareRunDirResult with outerHop field. Fixes type errors in test mocks across flows commands.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR refactors outer-hop dependency discovery to validate an ancestor Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/outerHop.test.ts`:
- Around line 50-51: The module-scoped installMock in outerHop.test.ts can
retain call history and implementation across it blocks because mock.restore()
does not clear a standalone mock() state. Update the test cleanup in the
outerHop test suite to call installMock.mockClear() after each test, or use
installMock.mockReset() if you also need to remove the mocked implementation,
alongside the existing afterEach cleanup.
🪄 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 Plus
Run ID: 3479e6f2-989a-40d0-be9c-8094dd5f6037
📒 Files selected for processing (17)
.changeset/validate-outer-hop-discovery.mdsrc/commands/flows/buildFlowsRunDeps.tssrc/commands/flows/buildRunReporter.tssrc/commands/flows/hybridRun.test.tssrc/commands/flows/runDefaults.handle.test.tssrc/commands/flows/runDefaults.reporterWiring.test.tssrc/commands/flows/runStagedFlows.test.tssrc/commands/flows/runStagedFlows.tssrc/core/errors.test.tssrc/core/errors.tssrc/core/messages/runner.tssrc/domains/runtimeEnv/outerHop.test.tssrc/domains/runtimeEnv/outerHop.tssrc/domains/runtimeEnv/prepareRunDir.test.tssrc/domains/runtimeEnv/prepareRunDir.tssrc/shell/reporter/createConsoleReporter.test.tssrc/shell/reporter/createConsoleReporter.ts
Michael Price (michael-pr)
left a comment
There was a problem hiding this comment.
Mostly suggestions, nothing blocking. Good to merge after addressing them.
Note
PR body AI drafted & edited as needed
Overview of Changes
qawolf flows runcould fail withERR_MODULE_NOT_FOUNDfor a dependency the flow project correctly declares. Root cause: the outer hop discovery inpopulateOuterHopwalked up from the flow project and symlinked the first ancestornode_modulesit found, with no check that it actually contains the project's deps, and finding one suppressed thenpm installfallback that would have worked.How it works
outerHop.ts): a candidatenode_modulesis symlinked only if every installable declared dep resolves in it (pinned executor packages are excluded, the inner hop owns those). Failing candidates are recorded and the walk continues; if none satisfies, the existing fallback installs the project's declared deps into the run dir (executor packages stripped, soworkspace:*pins don't break the install).populateOuterHopreturns a result union (symlink | install | none) threaded throughprepareRunDir>flows run, which rendersInstalling N project dependencies…before the (potentially slow) fallback install and debug logs each rejected candidate with the deps it was missing.package.json.Path like specifiers (broken
./relativeimports) are excluded so the hint never suggests "installing" a file path.Testing
Cannot find package 'date-fns'(3/3 channels)Installing 1 project dependency…(3/3)node_moduleslatest+workspace:*specs), nestedERR_MODULE_NOT_FOUND: Cannot find package 'date-fns'Installing 15 project dependencies…Checklist