Reviewing files that changed from the base of the PR and between 559c036 and f2ed4bd.

• pkg/compute/envvars.go (1 hunks)
• pkg/compute/envvars_test.go (1 hunks)
• pkg/compute/executor.go (1 hunks)
• pkg/executor/docker/executor.go (4 hunks)
• pkg/executor/docker/executor_test.go (2 hunks)
• pkg/executor/docker/models/types.go (1 hunks)
• pkg/executor/docker/models/types_test.go (1 hunks)
• pkg/executor/noop/executor.go (7 hunks)
• pkg/executor/types.go (1 hunks)
• pkg/lib/envvar/utils.go (1 hunks)
• pkg/lib/envvar/utils_test.go (1 hunks)
• pkg/models/constants.go (1 hunks)
• pkg/models/job.go (1 hunks)
• pkg/models/task.go (3 hunks)
• pkg/models/task_test.go (1 hunks)
• pkg/test/compute/resourcelimits_test.go (2 hunks)
• pkg/test/devstack/errorlogs_test.go (1 hunks)
• pkg/test/devstack/partitioning_test.go (1 hunks)
• pkg/test/devstack/target_all_test.go (1 hunks)
• pkg/test/devstack/timeout_test.go (1 hunks)
• pkg/test/scenario/example_noop_test.go (1 hunks)

pkg/executor/noop/executor.go (1)

22-28: Introduction of ExecutionContext enhances code clarity

The introduction of the ExecutionContext struct consolidates the necessary execution parameters into a single entity. This refactoring simplifies function signatures and improves code readability and maintainability.

pkg/test/scenario/example_noop_test.go (1)

24-25: JobHandler signature update aligns with ExecutionContext refactoring

The JobHandler function now accepts execContext noop.ExecutionContext, and the use of execContext.ResultsDir ensures the correct results directory is utilized. This change maintains consistency with the refactored executor code.

pkg/lib/envvar/utils.go (1)

48-60: Ensure deterministic order when merging environment variables

Maps in Go are unordered, which might lead to non-deterministic ordering when converting back to slices. If the order of environment variables is significant for downstream processes, consider sorting the keys to maintain a consistent order.

pkg/compute/envvars.go (3)

14-18: LGTM! Proper nil check handling.

The function correctly handles the nil case by returning an empty map, preventing potential null pointer dereferences.

---

25-40: LGTM! Comprehensive system variables setup.

The code properly sets up all required system environment variables with appropriate sanitization where needed. The use of the EnvVarPrefix constant ensures consistency.

---

42-44: Verify the merge behavior for environment variables.

The code correctly uses envvar.Merge to combine task and system variables, with system variables taking precedence. Let's verify the implementation of the merge function.

✅ Verification successful

Environment variable merge behavior verified correctly

The envvar.Merge implementation in pkg/lib/envvar/utils.go correctly ensures that system variables take precedence over task variables by using maps.Clone and maps.Copy operations, matching the intended behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of envvar.Merge function
# Expected: Find the implementation to confirm precedence handling

ast-grep --pattern 'func Merge($_, $_) map[string]string {
  $$$
}'

Length of output: 69

---

Script:

#!/bin/bash
# Search for files containing the Merge function definition
rg -l "func Merge" 

# Also search with package context to narrow down results
rg "package.*\n.*func Merge" -A 5

Length of output: 296

---

Script:

#!/bin/bash
# View the implementation of Merge function
cat pkg/lib/envvar/utils.go

Length of output: 1599

pkg/models/constants.go (1)

13-15: LGTM! Well-placed constant definition.

The EnvVarPrefix constant is appropriately defined and positioned within the constants block. The prefix "BACALHAU_" follows the common practice of using uppercase letters and underscores for environment variable prefixes.

pkg/compute/envvars_test.go (2)

13-132: LGTM! Comprehensive test coverage.

The test suite effectively covers various scenarios including:

• Edge cases (nil execution)
• Basic functionality
• Environment variable precedence
• Special character handling
• System vs. task-level variable conflicts

---

79-83: Good test for environment variable precedence.

The test explicitly verifies that system environment variables take precedence over task-level variables by attempting to override BACALHAU_NODE_ID.

pkg/test/devstack/target_all_test.go (1)

129-133: LGTM! Clean refactoring to use ExecutionContext.

The change properly updates the JobHandler to use the structured ExecutionContext instead of individual parameters, improving code organization.

pkg/lib/envvar/utils_test.go (1)

108-148: LGTM! Comprehensive sanitization tests.

The test cases for Sanitize thoroughly cover various scenarios including spaces, equals signs, and edge cases.

pkg/models/task.go (3)

128-133: LGTM! Environment variable validation is well-implemented.

The validation correctly prevents users from setting environment variables that could conflict with system-provided variables, using case-insensitive comparison.

---

165-183: LGTM! Input source validation is thorough.

The validation ensures uniqueness of both aliases and targets, preventing potential conflicts in the input sources.

---

185-202: LGTM! Result path validation is comprehensive.

The validation ensures uniqueness of both names and paths, preventing potential conflicts in the result paths.

pkg/test/devstack/partitioning_test.go (2)

37-80: LGTM! Single partition test is well-structured.

The test thoroughly validates the presence and correctness of all environment variables, including job metadata and partition information.

---

82-139: LGTM! Multiple partitions test is comprehensive.

The test effectively verifies:

1. Unique partition indices across executions
2. Distribution of partitions across nodes
3. Correct partition count reporting

pkg/models/task_test.go (1)

192-227: LGTM! Environment variable validation tests are thorough.

The test cases effectively cover:

1. Uppercase prefix validation
2. Lowercase prefix validation (case-insensitive)
3. Valid environment variables, including edge cases

pkg/models/job.go (1)

334-336: LGTM! Added safety check for empty Tasks slice.

The change prevents potential panic by checking for empty Tasks slice before accessing the first element.

pkg/compute/executor.go (1)

186-186: Verify environment variables precedence and security implications.

The addition of execution environment variables is a significant change that affects the execution context. Ensure that:

1. System-level environment variables take precedence over user-provided ones
2. Sensitive information is not leaked through environment variables
3. Environment variable names follow the established naming conventions

Run the following script to verify environment variable handling:

✅ Verification successful

Environment variable handling implementation verified

The implementation in GetExecutionEnvVars correctly handles environment variables with proper security measures:

• System-level variables take precedence over user-provided ones
• Environment variables are sanitized and validated
• The BACALHAU_ prefix is protected to prevent injection of system variables

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential security issues in environment variable handling
# Test: Look for direct exposure of sensitive data in environment variables

# Check for potential sensitive data in environment variable names/patterns
rg -i '(?:password|secret|key|token|credential).*=.*[^\s]' 

# Check for environment variable validation
ast-grep --pattern 'GetExecutionEnvVars($_)'

Length of output: 75844

---

Script:

#!/bin/bash
# Find GetExecutionEnvVars implementation and related code
ast-grep --pattern 'func GetExecutionEnvVars'

# Search for the function call and surrounding context
rg -A 10 -B 10 'GetExecutionEnvVars'

# Look for environment variable validation patterns
rg -i 'env.*var.*valid|valid.*env.*var'

# Check for environment variable security patterns
rg -i '(sanitize|clean|secure|validate).*env'

Length of output: 6367

pkg/executor/docker/executor.go (3)

309-315: LGTM! Clean method signature update.

The change simplifies the method signature by using executor.RunCommandRequest directly, improving code maintainability.

---

325-325: LGTM! Correct container environment configuration.

The merged environment variables are properly assigned to the container configuration.

---

316-321: Verify the order of environment variable merging.

The merging of environment variables appears correct, but we should ensure that the order of precedence is maintained (engine-level variables should override task-level variables).

Run the following script to verify the environment variable merging logic:

✅ Verification successful

Environment variable merging order is correct

The implementation ensures engine-level variables correctly override task-level variables, as verified by both unit tests and integration tests. The current implementation maintains the desired precedence order.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify environment variable merging implementation
# Test: Look for other merge operations to ensure consistency

# Check for other environment variable merging patterns
ast-grep --pattern 'envvar.MergeSlices($_, $_)'

# Look for potential conflicting merge operations
rg 'MergeSlices|merge.*env'

Length of output: 1393

---

Script:

#!/bin/bash
# Check the implementation of MergeSlices and its tests
cat pkg/lib/envvar/utils.go
cat pkg/lib/envvar/utils_test.go
cat pkg/executor/docker/executor_test.go

Length of output: 27436

pkg/executor/docker/executor_test.go (1)

582-621: Well-structured test cases for environment variables.

The test cases comprehensively cover:

• Task-level environment variables
• Engine-level environment variables
• Merged environment variables with correct precedence

pkg/test/devstack/errorlogs_test.go (1)

72-73: LGTM! Improved function signature with ExecutionContext.

The change consolidates execution-related parameters into a single context object, improving code organization and maintainability.

pkg/test/compute/resourcelimits_test.go (1)

101-107: LGTM! Good use of execution context.

The change correctly utilizes the execution context to access job metadata, which aligns with the PR objectives.