Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9d6e1d9
feat(python): add support for creating and activating venv
mwbrooks Jan 26, 2026
665e23c
fix(python): install pyproject.toml before requirements.txt
mwbrooks Feb 20, 2026
9008d02
chore(cmd): remove venv detail from init command description
mwbrooks Feb 20, 2026
51030ee
fix(python): use correct python executable on Windows for venv creation
mwbrooks Feb 20, 2026
6e6ba42
chore(python): add comment explaining venv activation is unnecessary …
mwbrooks Feb 20, 2026
fdd8e15
feat(python): auto-activate venv before hook execution
mwbrooks Feb 20, 2026
cf35c5c
chore(python): fix gofmt formatting in python_test.go
mwbrooks Feb 20, 2026
bff1d8c
chore(python): move venv creation message to debug output
mwbrooks Feb 20, 2026
b0f0050
chore(python): move dependency install messages to debug output
mwbrooks Feb 20, 2026
e641b20
chore(python): shorten install success messages and highlight filenames
mwbrooks Feb 20, 2026
4324103
fix(python): improve error handling for dependency file updates
mwbrooks Feb 20, 2026
3a76ba8
test: remove skipped test
mwbrooks Feb 20, 2026
180d499
fix(python): use HookExecutor for venv and pip commands to fix tests
mwbrooks Feb 21, 2026
253ee65
Merge branch 'mwbrooks-python-venv' into mwbrooks-python-venv-activation
mwbrooks Feb 21, 2026
adbda1b
feat(python): log debug message when venv is activated
mwbrooks Feb 21, 2026
91a07bf
refactor(python): use Os interface in ActivateVenvIfPresent for testa…
mwbrooks Feb 21, 2026
b75edb9
test: add tests for ActivatePythonVenvIfPresent in runtime package
mwbrooks Feb 21, 2026
ce15297
test: remove test comment
mwbrooks Feb 21, 2026
f8755c9
fix: merge variable declaration with assignment to satisfy linter
mwbrooks Feb 21, 2026
8554adc
chore: merge w main
zimeg Feb 27, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions internal/runtime/python/python.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,45 @@ func getPythonExecutable() string {
return "python3"
}

// getVenvBinDir returns the platform-specific bin directory inside a virtual environment
func getVenvBinDir(venvPath string) string {
if runtime.GOOS == "windows" {
return filepath.Join(venvPath, "Scripts")
}
return filepath.Join(venvPath, "bin")
}
Comment on lines +79 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👁️‍🗨️ issue(non-blocking): I'm finding the windows option is erroring with perhaps unrelated hook errors in execution explored in slackapi/python-slack-hooks#96


// ActivateVenvIfPresent sets the process environment variables that a Python
// virtual environment's activate script would set, so that child processes
// (hook scripts) inherit the activated venv. This is a no-op when no .venv
// directory exists in projectDir.
//
// The three env vars (VIRTUAL_ENV, PATH, PYTHONHOME) are the stable contract
// defined by PEP 405 (Python 3.3, 2012). Other tools (Poetry, tox, pipx) use
// 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, osClient types.Os, projectDir string) (bool, error) {
venvPath := getVenvPath(projectDir)
if !venvExists(fs, venvPath) {
return false, nil
}
Comment on lines +99 to +101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐍 question: Do we want to avoid reactivating a virtual environment if one is present? This might be an issue if different environments are activated for different reasons, but I'm unsure if that's a common practice.

I'm alright merging this without a clear answer to extend these approaches based on feedback! The happy path works so well and .venv appears to be convention without suggesting .venv.testing or similar.


binDir := getVenvBinDir(venvPath)

if err := osClient.Setenv("VIRTUAL_ENV", venvPath); err != nil {
return false, err
}
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
}

return true, nil
}

// getPipExecutable returns the path to the pip executable in the virtual environment
func getPipExecutable(venvPath string) string {
if runtime.GOOS == "windows" {
Expand Down
60 changes: 60 additions & 0 deletions internal/runtime/python/python_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,66 @@ func Test_venvExists(t *testing.T) {
}
}

func Test_getVenvBinDir(t *testing.T) {
result := getVenvBinDir("/path/to/.venv")
if runtime.GOOS == "windows" {
require.Equal(t, filepath.Join("/path/to/.venv", "Scripts"), result)
} else {
require.Equal(t, filepath.Join("/path/to/.venv", "bin"), result)
}
}

func Test_ActivateVenvIfPresent(t *testing.T) {
tests := map[string]struct {
createVenv bool
expectedActivated bool
}{
"activates venv when it exists": {
createVenv: true,
expectedActivated: true,
},
"no-op when venv does not exist": {
createVenv: false,
expectedActivated: false,
},
}
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")

originalPath := "/usr/bin:/bin"
osMock.On("Getenv", "PATH").Return(originalPath)
osMock.AddDefaultMocks()

if tc.createVenv {
// Create the pip executable so venvExists returns true
pipPath := getPipExecutable(venvPath)
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
require.NoError(t, err)
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
require.NoError(t, err)
}

activated, err := ActivateVenvIfPresent(fs, osMock, projectDir)
require.NoError(t, err)
require.Equal(t, tc.expectedActivated, activated)

if tc.expectedActivated {
expectedBinDir := getVenvBinDir(venvPath)
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
osMock.AssertCalled(t, "Setenv", "PATH", expectedBinDir+string(filepath.ListSeparator)+originalPath)
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
} else {
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
}
})
}
}

func Test_Python_InstallProjectDependencies(t *testing.T) {
tests := map[string]struct {
existingFiles map[string]string
Expand Down
8 changes: 8 additions & 0 deletions internal/runtime/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ func New(runtimeName string) (Runtime, error) {
return rt, nil
}

// ActivatePythonVenvIfPresent activates a Python virtual environment if one
// 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, os types.Os, projectDir string) (bool, error) {
return python.ActivateVenvIfPresent(fs, os, projectDir)
}

Comment on lines 69 to 76
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: While it feels wrong to have Python in the generic runtime this allows us to activate it for any project. We could move this into the Python runtime and think of a more general function that could be used across all runtimes.

Upside of this approach is that JS projects that include a .venv (perhaps for scripts) can be activated automatically.

// NewDetectProject returns a new Runtime based on the project type of dirPath
func NewDetectProject(ctx context.Context, fs afero.Fs, dirPath string, sdkConfig hooks.SDKCLIConfig) (Runtime, error) {
var rt Runtime
Expand Down
52 changes: 52 additions & 0 deletions internal/runtime/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@
package runtime

import (
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/runtime/deno"
"github.com/slackapi/slack-cli/internal/runtime/node"
"github.com/slackapi/slack-cli/internal/runtime/python"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/spf13/afero"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -57,6 +60,55 @@ func Test_Runtime_New(t *testing.T) {
}
}

func Test_ActivatePythonVenvIfPresent(t *testing.T) {
tests := map[string]struct {
createVenv bool
expectedActivated bool
}{
"activates venv when it exists": {
createVenv: true,
expectedActivated: true,
},
"no-op when venv does not exist": {
createVenv: false,
expectedActivated: false,
},
}
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")

osMock.On("Getenv", "PATH").Return("/usr/bin:/bin")
osMock.AddDefaultMocks()

if tc.createVenv {
// Create the pip executable so venvExists returns true
pipPath := filepath.Join(venvPath, "bin", "pip")
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
require.NoError(t, err)
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
require.NoError(t, err)
}

activated, err := ActivatePythonVenvIfPresent(fs, osMock, projectDir)
require.NoError(t, err)
require.Equal(t, tc.expectedActivated, activated)

if tc.expectedActivated {
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
osMock.AssertCalled(t, "Setenv", "PATH", mock.Anything)
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
} else {
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
}
})
}
}

func Test_Runtime_NewDetectProject(t *testing.T) {
tests := map[string]struct {
sdkConfig hooks.SDKCLIConfig
Expand Down
8 changes: 8 additions & 0 deletions internal/shared/clients.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,14 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error
}
dirPath = parentDir
}
// 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, 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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I'd love to display this in the regular output, but it's tough to get a nice format because it can be displayed in many different situations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ suggestion(non-blocking): Your approach is solid, but we might consider outputting the errors as a warning while keeping expected activations to the debug logs?

Would love to hear your thoughts on this! IIRC logic similar to what's here exists for the get-hooks activation, but we often miss problems that should be at least reported. Even if the command continues I think!

}

configFileBytes, err := afero.ReadFile(c.Fs, hooksJSONFilePath)
if err != nil {
return err // Fixes regression: do not wrap this error, so that the caller can use `os.IsNotExists`
Expand Down
3 changes: 3 additions & 0 deletions internal/shared/types/slackdeps.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 5 additions & 0 deletions internal/slackdeps/os.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
9 changes: 9 additions & 0 deletions internal/slackdeps/os_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down
Loading