process-compose: inherit the environment from the shell

[sandydoo]

Fixes #1951.

We can inherit all the env vars from the shell and avoid more complex filtering logic.
config.env isn't an exhaustive attrset of environment variables anyways, so it's not like the resulting process-compose config is a standalone file.

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review

I've analyzed this PR that removes explicit environment variable filtering from the process-compose configuration. Here's my comprehensive review:

Summary of Changes

The PR removes 6 lines from src/modules/process-managers/process-compose.nix:123-128 that were:

• Explicitly filtering out PC_CONFIG_FILES to prevent recursion
• Mapping all config.env variables to process-compose's environment format
• Creating an explicit environment array in the YAML configuration

Code Quality and Best Practices

✅ Positive aspects:

• Simplification: Removes complex filtering logic and makes the code more maintainable
• Clarity: The intent is clearer - inherit all environment variables naturally
• Less brittle: Eliminates potential bugs in the mapping/filtering logic

⚠️ Considerations:

• This is a behavioral change that affects how environment variables are passed to processes
• The change removes explicit control over which variables are exposed

Potential Bugs and Issues

🔴 Critical Concern - Recursive Loop:
The original code had a comment stating:

# Filter out the recursive PC_CONFIG_FILES env.
# Otherwise, we would get a loop:
#   PC_CONFIG_FILES -> configFile -> settings -> PC_CONFIG_FILES -> ...

Question: How is this recursive loop now being prevented? The PR description doesn't address this specific issue that the original code was designed to solve.

🟡 Behavior Change:

• Before: Only variables from config.env were passed to processes (minus PC_CONFIG_FILES)
• After: ALL environment variables from the shell are inherited

This could expose variables that weren't intended to be passed to processes.

Security Concerns

🔴 Environment Variable Exposure:

• Processes will now inherit ALL environment variables from the shell, including potentially sensitive ones (AWS keys, tokens, etc.)
• The previous approach was more controlled, only passing explicitly defined config.env variables
• This could be a security regression in environments where sensitive data exists in environment variables

Recommendation: Consider documenting this behavior change and providing guidance on cleaning sensitive variables from the environment before running devenv.

Performance Considerations

✅ Minor Performance Improvement:

• Eliminates the mapAttrsToList and removeAttrs operations
• Reduces memory usage by not creating the environment array in the YAML
• Faster startup time due to less processing

Functionality Impact

🟡 Compatibility Concerns:

1. Process-specific variables: Individual processes can still set their own environment variables via process-compose.environment, but the global environment handling has changed
2. Determinism: The configuration is now less deterministic as it depends on the shell environment
3. Standalone configs: As mentioned in the PR description, configs are no longer standalone - they depend on the shell environment

Test Coverage

🔴 Missing Test Coverage:

• No tests found that verify environment variable handling
• The recursive loop prevention scenario isn't tested
• No tests for the new behavior

Recommendation: Add tests to verify:

• Environment variables are properly inherited
• The recursive loop issue doesn't occur
• Process-specific environment variables still work correctly

Questions for the Author

1. Recursive Loop: How is the PC_CONFIG_FILES recursive loop now prevented?
2. Security: Have you considered the security implications of inheriting all environment variables?
3. Compatibility: Are there any existing devenv configurations that might break with this change?
4. Testing: How was this change tested? Are there any edge cases to consider?

Recommendation

🟡 Conditional Approval with Concerns:

The simplification is valuable, but the security implications and the unaddressed recursive loop concern need clarification. I'd recommend:

1. Address the recursive loop: Explain how this issue is now handled or provide evidence it's no longer a problem
2. Security documentation: Document the behavior change and security implications
3. Add tests: Verify the new behavior works correctly and doesn't introduce regressions
4. Consider a gradual approach: Maybe add a warning or transition period for this breaking change

The code quality improvement is good, but the functional and security implications need careful consideration.

---

Todo List

• Read the changed file to understand the current state
• Examine what code was removed via git diff
• Analyze the impact of inheriting environment variables
• Review for security, performance, and functionality concerns
• Provide comprehensive feedback

[sandydoo]

Question: How is this recursive loop now being prevented? The PR description doesn't address this specific issue that the original code was designed to solve.

Not an issue with the custom environment removed.

Processes will now inherit ALL environment variables from the shell, including potentially sensitive ones (AWS keys, tokens, etc.)

environment is additive AFAIKT.

[domenkozar]

fc9b40d