fix: persist conda environment state across execute_command calls #6629
Conversation
- Add activeEnvironment property to track conda environments in terminals - Update TerminalRegistry to prefer terminals with matching conda environments - Track conda activate/deactivate commands and update terminal state - Add comprehensive tests for conda environment persistence Fixes #6628
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Update the terminal's active environment based on the command and output | ||
| if (condaActivateMatch) { |
There was a problem hiding this comment.
Is this intentional? The conda activate/deactivate detection logic is duplicated here and in ExecaTerminalProcess.ts. Could we extract this into a shared utility function to maintain DRY principles?
| if (condaActivateMatch) { | |
| // Update the terminal's active environment based on the command and output | |
| const condaResult = CondaEnvironmentUtils.processCondaCommand(command, this.fullOutput); | |
| if (condaResult.environmentChanged) { | |
| this.terminal.activeEnvironment = condaResult.activeEnvironment; | |
| TerminalRegistry.setLastActiveEnvironment(condaResult.activeEnvironment); | |
| if (condaResult.activeEnvironment) { | |
| console.log(`[TerminalProcess] Conda environment activated: ${condaResult.activeEnvironment}`); | |
| } else { | |
| console.log("[TerminalProcess] Conda environment deactivated"); | |
| } | |
| } |
| // Extract the environment name from the command | ||
| const envName = condaActivateMatch[1].trim() | ||
| // Check if activation was successful by looking for common error indicators in output | ||
| const cleanOutput = this.fullOutput.toLowerCase() |
There was a problem hiding this comment.
The error detection only checks for "error" and "not found" in the output. What about other failure scenarios like permission issues ("Permission denied") or conda not being in PATH ("command not found")? Should we consider a more robust approach, perhaps checking for success indicators instead?
|
|
||
| return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd) | ||
| // Prefer terminals with matching environment | ||
| const envMatch = this.lastActiveEnvironment ? t.activeEnvironment === this.lastActiveEnvironment : true |
There was a problem hiding this comment.
Is there a potential race condition here? If multiple commands are executed simultaneously, the terminal selection logic might pick a terminal that's about to become busy. Should we consider adding some form of locking or reservation mechanism to prevent this?
| running: boolean | ||
| taskId?: string | ||
| process?: RooTerminalProcess | ||
| activeEnvironment?: string |
There was a problem hiding this comment.
Would it be beneficial to create a more specific type for environment names? Something like type CondaEnvironment = string or even validate that environment names follow conda's naming conventions?
| expect(terminal2.activeEnvironment).toBe("myenv") | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Great test coverage! Could we also add tests for edge cases like:
- Multiple rapid conda activate/deactivate commands
- Conda activate with spaces or special characters in environment names (e.g.,
conda activate "my env") - Terminal cleanup when an environment is still active
| */ | ||
| public static setLastActiveEnvironment(environment?: string): void { | ||
| this.lastActiveEnvironment = environment | ||
| console.log(`[TerminalRegistry] Last active environment updated: ${environment || "none"}`) |
There was a problem hiding this comment.
Should these console.log statements be behind a debug flag or use a proper logging framework? They might clutter production logs.
hannesrudolph
added
the
Issue/PR - Triage
|
This seems to be too specific, the issue is not properly scoped, Closing |
Summary
This PR fixes issue #6628 where conda environment activations were not persisting across multiple
execute_commandtool calls. Each command was potentially executed in a different terminal instance, losing the conda environment state.Changes
activeEnvironmentproperty to track conda environments in terminalsTerminalRegistryto prefer terminals with matching conda environments when selecting terminalsconda activateandconda deactivatecommands in bothTerminalProcessandExecaTerminalProcessHow it works
conda activate <env>command is executed, the terminal tracks the active environment nameTerminalRegistrymaintains a "last active environment" stateTesting
CondaEnvironmentPersistence.spec.tswith comprehensive test coverageFixes #6628
Important
Fixes conda environment persistence across terminal commands by tracking active environments and prioritizing matching terminals.
activeEnvironmentproperty toBaseTerminalto track conda environments.TerminalRegistryto prioritize terminals with matching conda environments.conda activateandconda deactivatecommands inTerminalProcessandExecaTerminalProcess.CondaEnvironmentPersistence.spec.tsfor comprehensive test coverage.TerminalRegistryto maintain a "last active environment" state.This description was created by
for 80a365a. You can customize this summary. It will automatically update as commits are pushed.