Add failing tests for issue #409: Environment Variable Pollution

[Serhan-Asad]

Summary

Adds failing tests that detect the environment variable pollution bug reported in #409.

Test Files

• Unit test: tests/test_commands_generate.py
  • Added: test_issue_409_env_vars_do_not_pollute_os_environ
• E2E tests: tests/test_e2e_issue_409_env_var_pollution.py
  • test_env_vars_do_not_pollute_os_environ_after_command
  • test_sequential_commands_do_not_accumulate_env_vars
  • test_env_vars_security_credentials_do_not_leak

What This PR Contains

• ✅ Failing unit test that reproduces the reported bug
• ✅ Failing E2E tests that verify the bug at integration level
• ✅ Tests are verified to fail on current code and will pass once the bug is fixed
• ✅ Comprehensive test coverage for pollution, security leaks, and multi-command scenarios

Root Cause

Line 152 in pdd/commands/generate.py calls os.environ.update(env_vars) without any cleanup mechanism, causing environment variables set via the -e/--env flag to persist permanently in the process environment. This contradicts both the inline comment ("for the duration of this command") and the documentation (README.md:948 states "for this command").

The pollution is unnecessary because the code already passes env_vars as a parameter to code_generator_main() on line 161, which properly handles variable expansion without polluting os.environ.

Test Results (Current Code)

All tests FAIL as expected:

• Unit test detects 3 polluted variables
• E2E test 1 detects 3 polluted variables
• E2E test 2 detects 5 pollution scenarios across sequential commands
• E2E test 3 detects security credential leaks (API_KEY, SECRET_TOKEN, DB_PASSWORD)

Next Steps

1. Review the failing tests to understand the expected behavior
2. Implement the fix: Remove os.environ.update(env_vars) at line 152
3. Verify the unit test passes
4. Verify all E2E tests pass
5. Run full test suite to check for regressions
6. Mark PR as ready for review

Fixes #409

---

Generated by PDD agentic bug workflow (Steps 1-10)

[Copilot]

Pull request overview

This PR adds comprehensive test coverage to detect and verify the environment variable pollution bug reported in issue #409, where variables set via the -e/--env flag persist in os.environ after command completion instead of being scoped to the command's execution.

Changes:

• Added unit test in test_commands_generate.py that verifies environment variables don't pollute os.environ after command execution
• Removed the problematic os.environ.update(env_vars) call in pdd/commands/generate.py that caused permanent environment pollution
• Tests verify that env vars are properly passed to code_generator_main() without polluting the global process environment

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
tests/test_commands_generate.py	Added unit test test_issue_409_env_vars_do_not_pollute_os_environ to detect environment variable pollution after command execution
pdd/commands/generate.py	Removed os.environ.update(env_vars) call and associated comment that caused environment variable pollution

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Serhan-Asad]

Fixed both Copilot comments:

1. Removed unused env_before and env_after variables
2. Removed redundant assertion about unexpected_new_keys (lines 310-316) that duplicated the pollution_detected check

The test is now simpler and focuses on a single assertion: verifying that environment variables set via the -e flag do not persist in os.environ after command completion.

[gltanaka]

Review & Test Results

Bug Verification

The bug is real. Line 152 of pdd/commands/generate.py on main calls os.environ.update(env_vars) with no cleanup, permanently polluting the process environment. The comment says "for the duration of this command" but no restoration logic exists. The env_vars dict is already passed as a parameter to code_generator_main() on line 161, and all downstream code checks the env_vars dict first before falling back to os.environ, making the os.environ.update() call redundant.

Test Results

Cherry-picked all 7 commits onto a clean branch from origin/main and ran:

Test	Result
test_issue_409_env_vars_do_not_pollute_os_environ (PR's new test)	PASSED
tests/test_commands_generate.py (12 tests)	11 passed, 1 skipped
Full test suite (3022 tests)	3014 passed, 1 failed, 7 skipped

Issue: Existing test not updated

The single failure is an existing test that was treating the pollution as intended behavior:

tests/commands/test_generate.py::test_generate_env_vars (line 263):

# Verify os.environ was updated
assert os.environ.get("NEW_VAR") == "new_val"  # ← expects pollution

This test needs to be updated to match the corrected behavior. It should verify that NEW_VAR is not in os.environ after the command (or verify the env_vars dict was correctly passed to code_generator_main without polluting the global environment).

Note on manual testing

A manual smoke test (running pdd generate -e VAR=val from the shell) would not surface this bug because each CLI invocation is a separate process — env vars can't leak between separate shell invocations. The bug only manifests when generate() is called multiple times within the same Python process (e.g., test suites or programmatic usage), which the automated tests cover correctly.

[gltanaka]

target 2/3

[Serhan-Asad]

Test Conflict with PR #267

Summary

This PR fixes environment variable pollution (Issue #409), which will cause one test in the steering-sync branch to fail once PR #267 merges.

Affected Test

• Branch: steering-sync (PR Add steerable option to pdd sync + fix textual sync animation resize issues #267 - not yet merged)
• File: tests/commands/test_generate.py
• Test: test_generate_env_vars (line 249)
• Issue: The test expects os.environ to be polluted with variables passed via -e flag

Current Test Behavior (Incorrect)

# Lines 15-16 of the old test
assert os.environ.get("NEW_VAR") == "new_val"  
assert os.environ.get("HOST_VAR") == "host_value"

This test explicitly checks that NEW_VAR exists in os.environ after the command runs, which is the buggy behavior this PR fixes.

Expected Timeline

1. This PR (Environment Variable Pollution: -e flag variables persist after command completion #409) merges - fixes the pollution bug
2. PR Add steerable option to pdd sync + fix textual sync animation resize issues #267 (steering-sync) is still under review
3. When Add steerable option to pdd sync + fix textual sync animation resize issues #267 merges → test_generate_env_vars will fail (expected)
4. I'll submit a follow-up PR to fix the test

Proposed Test Fix

I have the corrected test ready. It will:

• Verify env vars are passed as parameters to code_generator_main
• Assert NEW_VAR does NOT pollute os.environ
• Verify no environment variables are added or removed
• Include clear error messages referencing Issue Environment Variable Pollution: -e flag variables persist after command completion #409

Click to see the corrected test (ready to submit after #267 merges)

def test_generate_env_vars(runner, mock_code_gen):
    """Test environment variable parsing without polluting os.environ (Issue #409)."""
    with runner.isolated_filesystem():
        with open("p.txt", "w") as f: f.write("p")

        # Set a host env var to test pass-through
        os.environ["HOST_VAR"] = "host_value"

        # Save the current state of os.environ before the test
        env_before = dict(os.environ)

        # We invoke with one explicit KV pair and one pass-through key
        result = runner.invoke(generate_module.generate, ["p.txt", "-e", "NEW_VAR=new_val", "-e", "HOST_VAR"])

        assert result.exit_code == 0

        # Verify code_generator_main was called with the correct env_vars
        mock_code_gen.assert_called_once()
        call_kwargs = mock_code_gen.call_args.kwargs

        # The env_vars should be passed as a parameter to code_generator_main
        assert "env_vars" in call_kwargs
        assert call_kwargs["env_vars"] == {"NEW_VAR": "new_val", "HOST_VAR": "host_value"}

        # CRITICAL: Verify os.environ was NOT polluted (Issue #409)
        # NEW_VAR should NOT exist in os.environ after the command
        assert "NEW_VAR" not in os.environ, (
            "BUG: NEW_VAR was added to os.environ! "
            "Environment variables passed via -e flag should NOT pollute os.environ. "
            "See Issue #409."
        )

        # HOST_VAR should still exist (it was there before)
        assert os.environ.get("HOST_VAR") == "host_value"

        # Verify no other environment variables were added or removed
        env_after = dict(os.environ)
        assert env_before == env_after, (
            f"os.environ was modified! "
            f"Added: {set(env_after.keys()) - set(env_before.keys())}, "
            f"Removed: {set(env_before.keys()) - set(env_after.keys())}"
        )

        # Cleanup the HOST_VAR we added for the test
        if "HOST_VAR" in os.environ:
            del os.environ["HOST_VAR"]

[gltanaka]

Reopening: this PR was automatically closed by a force push to main on Feb 9, not intentionally.