fix: Restore PATH inheritance in workflow shell commands by osterman · Pull Request #1719 · cloudposse/atmos

osterman · 2025-10-27T20:16:18Z

what

Refactored to always merge custom env vars with parent environment
Fixes workflow shell commands failing with "executable file not found in $PATH"
Adds comprehensive unit and integration tests demonstrating the bug and verifying the fix

why

After commit 9fd7d15 (PR fix: Improve test infrastructure and fix critical environment variable bug #1543), workflow shell commands lost access to PATH environment variable
Users reported workflows that worked in v1.189.0 failed in v1.195.0 with commands like env, ls, grep not found
This is a critical regression affecting any workflow using external executables
Original fix conditionally replaced environment, which was inconsistent with executeCustomCommand behavior

Root Cause

The bug occurred in ExecuteShell() function in internal/exec/shell_utils.go:

Workflow commands call ExecuteShell with empty env slice: []string{}
ExecuteShell appends ATMOS_SHLVL to the slice: []string{"ATMOS_SHLVL=1"}
ShellRunner receives a non-empty env, so it doesn't fall back to os.Environ()
Shell command runs with ONLY ATMOS_SHLVL set, losing PATH and all other environment variables

Solution

Refactored ExecuteShell() to always merge custom env vars with parent environment:

// Always start with parent environment
mergedEnv := os.Environ()

// Merge custom env vars (overriding duplicates)
for _, envVar := range env {
    mergedEnv = u.UpdateEnvVar(mergedEnv, key, value)
}

// Add ATMOS_SHLVL
mergedEnv = append(mergedEnv, fmt.Sprintf("ATMOS_SHLVL=%d", newShellLevel))

This ensures:

✅ Empty env (workflows): Full parent environment including PATH
✅ Custom env (commands): Custom vars override parent, but PATH is preserved
✅ Consistent behavior: Matches executeCustomCommand pattern (line 393 in cmd_utils.go)

Testing

Unit Tests (internal/exec/shell_utils_test.go):

TestExecuteShell/empty_env_should_inherit_PATH_from_parent_process - Verifies env command works
TestExecuteShell/empty_env_should_inherit_PATH_for_common_commands - Tests ls, env, pwd, echo
TestExecuteShell/custom_env_vars_override_parent_env - Verifies custom vars properly override parent

Integration Test (tests/test-cases/workflows.yaml):

atmos workflow shell command with PATH - Full end-to-end workflow test using env | grep PATH

All tests pass, including existing workflow tests.

references

Closes A workflow command cannot be executed if it is of the shell type and the shell environment is cleared. #1718
Resolves DEV-3725
Regression introduced in: 9fd7d15 (PR fix: Improve test infrastructure and fix critical environment variable bug #1543)

Summary by CodeRabbit

Bug Fixes
- Shell commands now correctly inherit environment variables (including PATH) from the parent process, with custom env vars properly overriding parent values.
Tests
- Added tests covering environment inheritance for commands that require PATH, shell builtins, and custom env var overrides.
Workflows / Snapshots
- Added a workflow demonstrating PATH-dependent shell commands and updated related test snapshots and test cases.

Fixes workflow shell commands failing with "executable file not found in $PATH" when executing standard Unix commands like `env`, `ls`, `grep`, etc. ## Problem After commit 9fd7d15 (PR #1543), workflow shell commands lost access to the PATH environment variable, causing failures when executing any external executable: ``` "env": executable file not found in $PATH ``` The user reported this broke workflows that worked in v1.189.0 but failed in v1.195.0. ## Root Cause The bug occurred in ExecuteShell(): 1. Workflow commands call ExecuteShell with empty env: []string{} 2. ExecuteShell appends ATMOS_SHLVL: []string{"ATMOS_SHLVL=1"} 3. ShellRunner receives non-empty env, so doesn't fall back to os.Environ() 4. Shell runs with ONLY ATMOS_SHLVL set, losing PATH and all other vars ## Solution Check if env is empty and populate with os.Environ() BEFORE appending ATMOS_SHLVL. This ensures shell commands inherit the full parent process environment. ## Testing - Added unit tests in shell_utils_test.go demonstrating bug and fix - Added integration test for workflow shell commands with PATH - All existing workflow tests pass - Verified env, ls, grep commands now work in workflows Fixes #1718 Resolves DEV-3725 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2025-10-27T20:17:12Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

coderabbitai · 2025-10-27T20:20:07Z

📝 Walkthrough

Walkthrough

ExecuteShell now builds a merged environment starting from the current process env, applies provided KEY=value updates, appends ATMOS_SHLVL, and passes that merged environment to the shell runner; new unit and integration tests and a workflow fixture exercise PATH inheritance for shell commands.

Changes

Cohort / File(s)	Summary
Env merge logic `internal/exec/shell_utils.go`	Build mergedEnv from current process environment (os.Environ()), apply provided env updates (using new unexported helpers to parse KEY/value and UpdateEnvVar semantics), append `ATMOS_SHLVL`, and pass `mergedEnv` to `ShellRunner` instead of appending to the original slice.
Unit tests `internal/exec/shell_utils_test.go`	Add three Unix-only subtests to validate PATH inheritance and override behavior when calling `ExecuteShell` with empty or custom envs; use `t.Setenv` and OS skip guards.
Workflow fixture `tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml`	Add `shell-with-path` workflow step that runs `env
Snapshots & test cases `tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden`, `tests/test-cases/workflows.yaml`	Update not-found-workflow snapshot to include the new `shell-with-path` entry and add an integration test case asserting `PATH=` appears and exit code 0.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as ExecuteShell
  participant OS as os.Environ()
  participant Merge as merge logic
  participant Shell as ShellRunner

  Note over Caller,OS: Start ExecuteShell(envArg)
  Caller->>OS: read current env (os.Environ)
  Caller->>Merge: apply updates from envArg (parse KEY=VALUE)
  Merge-->>Caller: mergedEnv (parent env + overrides)
  Caller->>Merge: append ATMOS_SHLVL
  Caller->>Shell: invoke ShellRunner with mergedEnv
  Shell-->>Caller: command output / exit code

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Reasoning: small logic change with new helper parsing, multiple new tests and fixtures; changes touch execution internals and test surface.
Files to review closely:
- internal/exec/shell_utils.go — ensure merging/updating semantics and ATMOS_SHLVL handling are correct and platform-safe.
- internal/exec/shell_utils_test.go — verify tests correctly skip on Windows and reliably exercise PATH inheritance.
- New workflow fixture and snapshot — confirm expected CLI output change is intentional.

Possibly related PRs

Fix atmos terraform shell command. Improve !terraform.output YAML function #1252 — Also modifies internal/exec/shell_utils.go to change env construction and merging when launching shells.
increment ATMOS_SHLVL each time atmos terraform shell is called #803 — Related changes around ATMOS_SHLVL handling in the same shell utilities.
fix: !exec returning executable not found - !exec should preserve PATH #1634 — Fixes PATH inheritance for shell-executed commands by using/merging the parent process environment.

Suggested reviewers

aknysh

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "fix: Restore PATH inheritance in workflow shell commands" accurately and specifically identifies the core change. It's clear and concise, directly describing what the changeset accomplishes: restoring the ability for workflow shell commands to inherit PATH from the parent environment. The title aligns precisely with the modifications to ExecuteShell() and the associated test additions.
Linked Issues Check	✅ Passed	The changeset comprehensively addresses all requirements from linked issues #1718 and DEV-3725. The core fix modifies ExecuteShell() to initialize mergedEnv with os.Environ() when the incoming env is empty, ensuring PATH inheritance while still allowing custom env vars to override via UpdateEnvVar semantics. Supporting tests verify custom env vars override parent vars while preserving PATH, test PATH inheritance for commands like env and ls, and include an integration test (shell-with-path workflow) that validates end-to-end behavior matching the expected v1.189.0 functionality.
Out of Scope Changes Check	✅ Passed	All changes are directly scoped to the PATH inheritance fix. The modification to shell_utils.go implements the core solution, the new unit tests in shell_utils_test.go validate the fix, the workflow fixture and test case provide integration testing, and the snapshot update is a necessary artifact of adding a new workflow. No changes introduce unrelated functionality or address separate issues.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/workflow-shell-env-cleared

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Improves the fix to always merge custom environment variables with the parent process environment, rather than conditionally replacing it. This ensures: 1. Empty env (workflows): Inherits full parent environment including PATH 2. Custom env (commands): Merges custom vars with parent, overriding duplicates 3. Consistent behavior: All callers get PATH and system vars by default This matches the original behavior before commit 9fd7d15 where env was merged with append(os.Environ(), env...) rather than replaced. ## Changes - Always start with os.Environ() as the base - Merge custom env vars using UpdateEnvVar to override duplicates - Add helper functions parseEnvVarKey/parseEnvVarValue for parsing - Add test verifying custom vars override parent while preserving PATH Addresses code review feedback on PR #1719. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

codecov · 2025-10-27T20:33:49Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.60%. Comparing base (7cfd2f2) to head (3f6c155).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/exec/shell_utils.go	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1719      +/-   ##
==========================================
+ Coverage   68.57%   68.60%   +0.02%     
==========================================
  Files         372      372              
  Lines       33439    33450      +11     
==========================================
+ Hits        22932    22948      +16     
+ Misses       8349     8346       -3     
+ Partials     2158     2156       -2

Flag	Coverage Δ
unittests	`68.60% <84.61%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/exec/shell_utils.go	`55.03% <84.61%> (+1.19%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

internal/exec/shell_utils.go (1)
142-156: Consider combining the helpers for efficiency.

Both functions scan for the = separator independently. Combining them into a single parseEnvVar(envVar) (key, value) helper would avoid the duplicate IndexByte call on line 128 where both are invoked on the same string.

Apply this refactor if you'd like:
-// parseEnvVarKey extracts the key from an environment variable string (KEY=value).
-func parseEnvVarKey(envVar string) string {
-	if idx := strings.IndexByte(envVar, '='); idx >= 0 {
-		return envVar[:idx]
-	}
-	return envVar
-}
-
-// parseEnvVarValue extracts the value from an environment variable string (KEY=value).
-func parseEnvVarValue(envVar string) string {
-	if idx := strings.IndexByte(envVar, '='); idx >= 0 {
-		return envVar[idx+1:]
-	}
-	return ""
-}
+// parseEnvVar extracts the key and value from an environment variable string (KEY=value).
+func parseEnvVar(envVar string) (key, value string) {
+	if idx := strings.IndexByte(envVar, '='); idx >= 0 {
+		return envVar[:idx], envVar[idx+1:]
+	}
+	return envVar, ""
+}
Then update line 128:
-		mergedEnv = u.UpdateEnvVar(mergedEnv, parseEnvVarKey(envVar), parseEnvVarValue(envVar))
+		key, value := parseEnvVar(envVar)
+		mergedEnv = u.UpdateEnvVar(mergedEnv, key, value)
internal/exec/shell_utils_test.go (2)
652-677: Update the comment to reflect the fix.

The comment on lines 673-675 says "This currently fails" but should reference the past tense since the fix has been implemented and tests now pass.

Apply this diff:
-		// This currently fails with "env": executable file not found in $PATH
-		// because ExecuteShell adds ATMOS_SHLVL to the empty slice, making it non-empty,
-		// and ShellRunner then uses ONLY that environment, losing PATH.
+		// Before the fix, this failed with "env": executable file not found in $PATH
+		// because ExecuteShell added ATMOS_SHLVL to the empty slice, making it non-empty,
+		// and ShellRunner then used ONLY that environment, losing PATH.
679-715: Update comment to reflect the fix is implemented.

The comment on line 707 says "These currently fail due to the bug" but should use past tense since the fix has been applied and tests now pass.

Apply this diff:
 				if tc.requiresPATH {
-					// These currently fail due to the bug - PATH is not inherited.
+					// Before the fix, these failed due to the bug - PATH was not inherited.
 					assert.NoError(t, err, "should be able to execute '%s' when PATH is inherited", tc.cmd)

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2851aa2 and 3f6c155.

📒 Files selected for processing (2)

internal/exec/shell_utils.go (1 hunks)
internal/exec/shell_utils_test.go (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*_test.go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios

**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.

Files:

internal/exec/shell_utils_test.go

**/*.go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments

**/*.go: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.

Files:

internal/exec/shell_utils_test.go
internal/exec/shell_utils.go

**/!(*_test).go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Document all exported functions, types, and methods with Go doc comments

Files:

internal/exec/shell_utils.go

🧬 Code graph analysis (2)

internal/exec/shell_utils_test.go (1)

internal/exec/shell_utils.go (1)

ExecuteShell (107-140)

internal/exec/shell_utils.go (2)

pkg/utils/env_utils.go (1)

UpdateEnvVar (133-156)

pkg/utils/shell_utils.go (1)

ShellRunner (65-103)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

GitHub Check: Build (windows)
GitHub Check: Build (linux)
GitHub Check: autofix
GitHub Check: Analyze (go)
GitHub Check: Review Dependency Licenses
GitHub Check: Summary

🔇 Additional comments (3)

internal/exec/shell_utils.go (2)

120-131: Solid fix for PATH inheritance.

The implementation correctly addresses the regression by starting with os.Environ() and merging custom env vars via UpdateEnvVar. This ensures PATH and other system variables are preserved while custom vars can override as needed.

142-148: Helper function looks solid.

The key extraction logic handles typical env var formats correctly, including edge cases like empty keys or values.

internal/exec/shell_utils_test.go (1)

629-650: Excellent test coverage for the override scenario.

This test validates that custom env vars take precedence while PATH remains accessible. The combined assertion using test and grep efficiently verifies both behaviors.

github-actions · 2025-10-28T01:14:40Z

These changes were released in v1.196.0-rc.3.

github-actions · 2025-10-28T16:26:04Z

These changes were released in v1.196.0-test.3.

osterman requested a review from a team as a code owner October 27, 2025 20:16

github-actions bot added the size/s Small size PR label Oct 27, 2025

coderabbitai bot previously approved these changes Oct 27, 2025

View reviewed changes

osterman added the patch A minor, backward compatible change label Oct 27, 2025

osterman dismissed coderabbitai[bot]’s stale review via e431f70 October 27, 2025 20:21

osterman force-pushed the feature/workflow-shell-env-cleared branch from d2c47f8 to e431f70 Compare October 27, 2025 20:21

github-actions bot added size/m Medium size PR and removed size/s Small size PR labels Oct 27, 2025

Merge branch 'main' into feature/workflow-shell-env-cleared

3f6c155

coderabbitai bot reviewed Oct 27, 2025

View reviewed changes

coderabbitai bot approved these changes Oct 27, 2025

View reviewed changes

aknysh approved these changes Oct 28, 2025

View reviewed changes

aknysh merged commit d99ad68 into main Oct 28, 2025
53 checks passed

aknysh deleted the feature/workflow-shell-env-cleared branch October 28, 2025 00:34

This was referenced Dec 1, 2025

Support Atmos Toolchain for 3rd Party Tools #1686

Merged

Fix and redesign Atmos workflows #1849

Merged

coderabbitai bot mentioned this pull request Dec 16, 2025

feat: Add global env section to atmos.yaml #1829

Merged

coderabbitai bot mentioned this pull request Dec 28, 2025

feat: Implement workflow step types with registry pattern (DEV-263, DEV-2969) #1899

Open

coderabbitai bot mentioned this pull request Feb 2, 2026

feat: add workflow environment variable support with hierarchical merging #2050

Merged

coderabbitai bot mentioned this pull request Mar 2, 2026

fix(auth): propagate TTY state to subprocesses for SSO device flow in workflows #2126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Restore PATH inheritance in workflow shell commands#1719

fix: Restore PATH inheritance in workflow shell commands#1719
aknysh merged 3 commits intomainfrom
feature/workflow-shell-env-cleared

osterman commented Oct 27, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

codecov bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

github-actions bot commented Oct 28, 2025

Uh oh!

github-actions bot commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

osterman commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what

why

Root Cause

Solution

Testing

references

Summary by CodeRabbit

Uh oh!

github-actions bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

coderabbitai bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 28, 2025

Uh oh!

github-actions bot commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

osterman commented Oct 27, 2025 •

edited

Loading

github-actions bot commented Oct 27, 2025 •

edited

Loading

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

codecov bot commented Oct 27, 2025 •

edited

Loading