refactor(python): use Os interface in ActivateVenvIfPresent for testability

Add Unsetenv to the Os interface/implementation/mock and refactor
ActivateVenvIfPresent to accept types.Os instead of calling the os
package directly. This allows the test to use mocks instead of mutating
the real process environment.

Author: mwbrooks

internal/runtime/python/python.go

@@ -19,7 +19,6 @@ import (
 	"context"
 	_ "embed"
 	"fmt"
-	"os"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -95,21 +94,23 @@ func getVenvBinDir(venvPath string) string {
 // the same approach. Sourcing the shell-specific activate script (activate,
 // activate.fish, Activate.ps1) would be higher maintenance because it varies
 // by shell.
-func ActivateVenvIfPresent(fs afero.Fs, projectDir string) (bool, error) {
+func ActivateVenvIfPresent(fs afero.Fs, osClient types.Os, projectDir string) (bool, error) {
 	venvPath := getVenvPath(projectDir)
 	if !venvExists(fs, venvPath) {
 		return false, nil
 	}
 
 	binDir := getVenvBinDir(venvPath)
 
-	if err := os.Setenv("VIRTUAL_ENV", venvPath); err != nil {
+	if err := osClient.Setenv("VIRTUAL_ENV", venvPath); err != nil {
 		return false, err
 	}
-	if err := os.Setenv("PATH", binDir+string(filepath.ListSeparator)+os.Getenv("PATH")); err != nil {
+	if err := osClient.Setenv("PATH", binDir+string(filepath.ListSeparator)+osClient.Getenv("PATH")); err != nil {
+		return false, err
+	}
+	if err := osClient.Unsetenv("PYTHONHOME"); err != nil {
 		return false, err
 	}
-	os.Unsetenv("PYTHONHOME")
 
 	return true, nil
 }

internal/runtime/python/python_test.go

@@ -17,7 +17,6 @@ package python
 import (
 	"encoding/json"
 	"fmt"
-	"os"
 	"path/filepath"
 	"runtime"
 	"testing"
@@ -342,14 +341,13 @@ func Test_ActivateVenvIfPresent(t *testing.T) {
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			fs := slackdeps.NewFsMock()
+			osMock := slackdeps.NewOsMock()
 			projectDir := "/path/to/project"
 			venvPath := filepath.Join(projectDir, ".venv")
 
-			// Save and restore env vars
-			t.Setenv("VIRTUAL_ENV", "")
-			t.Setenv("PYTHONHOME", "original-pythonhome")
-			originalPath := os.Getenv("PATH")
-			t.Setenv("PATH", originalPath)
+			originalPath := "/usr/bin:/bin"
+			osMock.On("Getenv", "PATH").Return(originalPath)
+			osMock.AddDefaultMocks()
 
 			if tc.createVenv {
 				// Create the pip executable so venvExists returns true
@@ -360,21 +358,18 @@ func Test_ActivateVenvIfPresent(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			activated, err := ActivateVenvIfPresent(fs, projectDir)
+			activated, err := ActivateVenvIfPresent(fs, osMock, projectDir)
 			require.NoError(t, err)
 			require.Equal(t, tc.expectedActivated, activated)
 
 			if tc.expectedActivated {
-				require.Equal(t, venvPath, os.Getenv("VIRTUAL_ENV"))
 				expectedBinDir := getVenvBinDir(venvPath)
-				require.Contains(t, os.Getenv("PATH"), expectedBinDir+string(filepath.ListSeparator))
-				// PYTHONHOME should be unset (empty)
-				_, pythonHomeSet := os.LookupEnv("PYTHONHOME")
-				require.False(t, pythonHomeSet)
+				osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
+				osMock.AssertCalled(t, "Setenv", "PATH", expectedBinDir+string(filepath.ListSeparator)+originalPath)
+				osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
 			} else {
-				require.Equal(t, "", os.Getenv("VIRTUAL_ENV"))
-				require.Equal(t, originalPath, os.Getenv("PATH"))
-				require.Equal(t, "original-pythonhome", os.Getenv("PYTHONHOME"))
+				osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
+				osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
 			}
 		})
 	}

internal/runtime/runtime.go

@@ -70,8 +70,8 @@ func New(runtimeName string) (Runtime, error) {
 // exists in the given project directory. This sets VIRTUAL_ENV, prepends the
 // venv bin directory to PATH, and unsets PYTHONHOME on the current process so
 // that child processes (hook scripts) inherit the activated venv.
-func ActivatePythonVenvIfPresent(fs afero.Fs, projectDir string) (bool, error) {
-	return python.ActivateVenvIfPresent(fs, projectDir)
+func ActivatePythonVenvIfPresent(fs afero.Fs, os types.Os, projectDir string) (bool, error) {
+	return python.ActivateVenvIfPresent(fs, os, projectDir)
 }
 
 // NewDetectProject returns a new Runtime based on the project type of dirPath

internal/shared/clients.go

@@ -226,7 +226,7 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error
 	}
 	// Activate Python virtual environment if present, so hook scripts
 	// can resolve the venv's Python and installed packages.
-	if activated, err := runtime.ActivatePythonVenvIfPresent(c.Fs, dirPath); err != nil {
+	if activated, err := runtime.ActivatePythonVenvIfPresent(c.Fs, c.Os, dirPath); err != nil {
 		c.IO.PrintDebug(ctx, "failed to activate Python virtual environment: %s", err)
 	} else if activated {
 		c.IO.PrintDebug(ctx, "Activated Python virtual environment .venv")

internal/shared/types/slackdeps.go

@@ -38,6 +38,9 @@ type Os interface {
 	// Setenv defaults to `os.Setenv` and can be mocked to test
 	Setenv(key string, value string) error
 
+	// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
+	Unsetenv(key string) error
+
 	// Getwd defaults to `os.Getwd` and can be mocked to test
 	Getwd() (dir string, err error)
 

internal/slackdeps/os.go

@@ -53,6 +53,11 @@ func (c *Os) Setenv(key string, value string) error {
 	return os.Setenv(key, value)
 }
 
+// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
+func (c *Os) Unsetenv(key string) error {
+	return os.Unsetenv(key)
+}
+
 // Getwd defaults to `os.Getwd` and can be mocked to test
 func (c *Os) Getwd() (dir string, err error) {
 	return os.Getwd()

internal/slackdeps/os_mock.go

@@ -46,6 +46,7 @@ func (m *OsMock) AddDefaultMocks() {
 	m.On("LookPath", mock.Anything).Return("", nil)
 	m.On("LookupEnv", mock.Anything).Return("", false)
 	m.On("Setenv", mock.Anything, mock.Anything).Return(nil)
+	m.On("Unsetenv", mock.Anything).Return(nil)
 	m.On("Getwd").Return(MockWorkingDirectory, nil)
 	m.On("UserHomeDir").Return(MockHomeDirectory, nil)
 	m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
@@ -83,6 +84,14 @@ func (m *OsMock) Setenv(key string, value string) error {
 	return args.Error(0)
 }
 
+// Unsetenv mocks the unsetting of an environment variable
+func (m *OsMock) Unsetenv(key string) error {
+	m.On("Getenv", key).Return("")
+	m.On("LookupEnv", key).Return("", false)
+	args := m.Called(key)
+	return args.Error(0)
+}
+
 // Getwd mocks returning the working directory.
 func (m *OsMock) Getwd() (_dir string, _err error) {
 	args := m.Called()