fix: [AI-198] prioritize managed engine venv over CWD .venv#199
Merged
anandgupta42 merged 7 commits intomainfrom Mar 16, 2026
Merged
fix: [AI-198] prioritize managed engine venv over CWD .venv#199anandgupta42 merged 7 commits intomainfrom
anandgupta42 merged 7 commits intomainfrom
Conversation
…olvePython()` `ensureEngine()` installs altimate-engine into the managed venv at `~/.opencode/data/engine/venv/`, but `resolvePython()` checked for a `.venv` in the user's current working directory first. When running from any Python project with its own `.venv`, the bridge would spawn that project's Python — which doesn't have altimate-engine — and fail. Swap the priority so the managed engine venv is checked before the CWD `.venv`. Add a test covering the new ordering. Closes #198
Claude Code ReviewThis repository is configured for manual code reviews. Comment |
The managed engine install was missing the `[warehouses]` optional dependency group, which includes `snowflake-connector-python`, `psycopg2-binary`, `duckdb`, and other database connectors. This caused FinOps tools and warehouse operations to fail with "snowflake-connector-python not installed" errors. Closes #196
Contributor
Author
|
@claude review |
…ws paths, add startup mutex - Validate Python binary exists on disk before trusting manifest (fixes: manifest exists but venv manually deleted → silent failure) - Track installed extras in manifest so version match with different extras (e.g. upgrading from no-extras to `[warehouses]`) triggers reinstall - Use platform-aware `venvPythonBin()` helper for dev/cwd venv paths (fixes: Windows uses `Scripts/python.exe`, not `bin/python`) - Add `pendingStart` mutex to `Bridge.call()` preventing concurrent `start()` calls from spawning duplicate Python processes - Add 25 new tests covering: priority ordering, env var edge cases, extras tracking, manifest validation, Windows paths, startup mutex, concurrent calls
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage. Once credits are available, reopen this pull request to trigger a review. |
…s in tests Address Sentry bot review comments on PR #199: 1. When `start()` spawns a child but `ping` verification fails, kill and clear the `child` handle so subsequent `call()` invocations trigger a restart instead of writing to a broken process and hanging for 30s timeout. 2. Use platform-aware `testVenvPythonBin()` helper in test file for dev/cwd venv detection, matching production `venvPythonBin()` behavior. Fixes incorrect test skip logic on Windows.
- Remove dead else branch in `ENGINE_INSTALL_SPEC` conditional (constant is always truthy) - Add `extras` field to `engine_started` telemetry event for debugging - Update `ENGINE_INSTALL_SPEC` comment to reflect production usage - Tighten concurrent startup test assertion to verify mutex coalescing (`ensureEngineCalls <= 2` instead of just `>= 1`)
Address Gemini review findings: 1. Add `error` event handler on spawned child process to prevent unhandled ENOENT/EACCES from crashing the host process when `resolvePython()` returns a nonexistent or non-executable path. 2. Recreate venv when Python binary is missing even if the venv directory still exists (e.g. user deleted just the binary). 3. Re-check `child` state after awaiting `pendingStart` mutex — the process may have died between startup completing and the secondary caller resuming.
When `child.kill()` is called (e.g. after ping failure), the exit handler fires with `code = null`. The check `code !== 0` treated this as a crash, incrementing `restartCount`. After two ping failures, the bridge would be permanently disabled. Changed to `code !== null && code !== 0` so only actual non-zero exit codes (real crashes) count toward the restart limit.
Contributor
Author
|
@claude review |
anandgupta42
added a commit
that referenced
this pull request
Mar 17, 2026
* fix: [AI-198] prioritize managed engine venv over CWD `.venv` in `resolvePython()` `ensureEngine()` installs altimate-engine into the managed venv at `~/.opencode/data/engine/venv/`, but `resolvePython()` checked for a `.venv` in the user's current working directory first. When running from any Python project with its own `.venv`, the bridge would spawn that project's Python — which doesn't have altimate-engine — and fail. Swap the priority so the managed engine venv is checked before the CWD `.venv`. Add a test covering the new ordering. Closes #198 * fix: [AI-196] install `altimate-engine[warehouses]` extra by default The managed engine install was missing the `[warehouses]` optional dependency group, which includes `snowflake-connector-python`, `psycopg2-binary`, `duckdb`, and other database connectors. This caused FinOps tools and warehouse operations to fail with "snowflake-connector-python not installed" errors. Closes #196 * fix: harden engine bootstrap — validate venv, track extras, fix Windows paths, add startup mutex - Validate Python binary exists on disk before trusting manifest (fixes: manifest exists but venv manually deleted → silent failure) - Track installed extras in manifest so version match with different extras (e.g. upgrading from no-extras to `[warehouses]`) triggers reinstall - Use platform-aware `venvPythonBin()` helper for dev/cwd venv paths (fixes: Windows uses `Scripts/python.exe`, not `bin/python`) - Add `pendingStart` mutex to `Bridge.call()` preventing concurrent `start()` calls from spawning duplicate Python processes - Add 25 new tests covering: priority ordering, env var edge cases, extras tracking, manifest validation, Windows paths, startup mutex, concurrent calls * fix: clean up child process on ping failure + use platform-aware paths in tests Address Sentry bot review comments on PR #199: 1. When `start()` spawns a child but `ping` verification fails, kill and clear the `child` handle so subsequent `call()` invocations trigger a restart instead of writing to a broken process and hanging for 30s timeout. 2. Use platform-aware `testVenvPythonBin()` helper in test file for dev/cwd venv detection, matching production `venvPythonBin()` behavior. Fixes incorrect test skip logic on Windows. * fix: address multi-model code review feedback - Remove dead else branch in `ENGINE_INSTALL_SPEC` conditional (constant is always truthy) - Add `extras` field to `engine_started` telemetry event for debugging - Update `ENGINE_INSTALL_SPEC` comment to reflect production usage - Tighten concurrent startup test assertion to verify mutex coalescing (`ensureEngineCalls <= 2` instead of just `>= 1`) * fix: handle spawn errors, venv recovery, and post-mutex child guard Address Gemini review findings: 1. Add `error` event handler on spawned child process to prevent unhandled ENOENT/EACCES from crashing the host process when `resolvePython()` returns a nonexistent or non-executable path. 2. Recreate venv when Python binary is missing even if the venv directory still exists (e.g. user deleted just the binary). 3. Re-check `child` state after awaiting `pendingStart` mutex — the process may have died between startup completing and the secondary caller resuming. * fix: don't increment `restartCount` when process is killed by signal When `child.kill()` is called (e.g. after ping failure), the exit handler fires with `code = null`. The check `code !== 0` treated this as a crash, incrementing `restartCount`. After two ping failures, the bridge would be permanently disabled. Changed to `code !== null && code !== 0` so only actual non-zero exit codes (real crashes) count toward the restart limit.
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.
What does this PR do?
Fixes altimate-engine not being found when the user runs the CLI from a directory that has its own
.venv(any Python project).ensureEngine()installs altimate-engine into the managed venv (~/.opencode/data/engine/venv/), butresolvePython()was checking the CWD.venvfirst. This meant the bridge would pick up the user's project Python — which doesn't have altimate-engine — and fail.Fix: Swap the priority so managed engine venv is checked before CWD
.venv.Type of change
Issue for this PR
Closes #198
How did you verify your code works?
bun test test/bridge/client.test.ts— 8/8 pass)prefers managed engine venv over .venv in cwdChecklist