Drop order in ConfigWorld may reintroduce environment variable unexpectedly

[coderabbitai]

Context

In tests/steps/config_steps.rs at line 136, the Drop implementation for ConfigWorld releases env_guard before socket_guard:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

Issue

If a future scenario sets COMENQD_SOCKET_PATH before calling config_file_with_token, the current drop order could reintroduce the environment variable on drop, leading to unexpected test behaviour.

Recommendation

Consider dropping socket_guard first or consolidating guards for the same key to avoid ordering surprises.

References

• Pull request: Coverage test failure #125
• Comment: Coverage test failure #125 (comment)
• Requested by: @leynos

[traycerai]

Plan

Observations

The ConfigWorld struct in tests/steps/config_steps.rs contains two EnvVarGuard fields (env_guard and socket_guard) that can both manage the COMENQD_SOCKET_PATH environment variable. The current Drop implementation (lines 136-145) drops env_guard before socket_guard, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (lines 14-18 in tests/features/config.feature) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since socket_guard is typically set first (in config_file_with_token at line 32) and env_guard is set later (in set_env_var at line 90), the drop order should be reversed to drop socket_guard after env_guard. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

Implementation Steps

1. Fix Drop Order in ConfigWorld

File: tests/steps/config_steps.rs

Location: Lines 136-145 (the Drop implementation for ConfigWorld)

Change: Reverse the order of guard drops in the drop method:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

• Move the socket_guard drop block to execute before the env_guard drop block
• This ensures that the most recently set guard (env_guard) is dropped first, followed by the earlier guard (socket_guard)
• The guards will restore environment variables in the correct LIFO order

Rationale: When both guards manage the same environment variable key (COMENQD_SOCKET_PATH):

• socket_guard is created first (captures original state)
• env_guard is created second (captures state after socket_guard modification)
• On drop, env_guard should restore first (to socket_guard's state)
• Then socket_guard should restore (to original state)

2. Verify Test Behavior

File: tests/features/config.feature

Scenario to verify: "environment variable overrides file" (lines 14-18)

Verification steps:

• Run the cucumber test suite with cargo test or the appropriate test command
• Ensure the scenario passes with the corrected drop order
• Confirm that COMENQD_SOCKET_PATH is properly cleaned up after the test
• Verify no environment variable pollution occurs between test scenarios

3. Review Related Code

Files to review:

• test-support/src/env_guard.rs - Confirm EnvVarGuard::Drop implementation is correct (it is)
• tests/steps/config_steps.rs - Review all functions that set env_guard or socket_guard to understand the creation order

Functions to review:

• config_file_with_token (line 26) - sets socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
• config_without_socket (line 61) - sets socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
• set_env_var (line 89) - sets env_guard via EnvVarGuard::set(&key, &value)

Confirmation: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

Execution Details

Code Change Summary

The fix is a simple reordering of two blocks in the Drop implementation:

Before:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

After:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

Test Scenario Walkthrough

For the "environment variable overrides file" scenario (lines 14-18 of config.feature):

1. Setup Phase:

   • config_file_with_token("abc") creates socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
     • Captures original state: COMENQD_SOCKET_PATH is unset
     • Removes the variable from environment
   • set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock") creates env_guard via EnvVarGuard::set()
     • Captures current state: COMENQD_SOCKET_PATH is unset (after socket_guard removed it)
     • Sets COMENQD_SOCKET_PATH to /tmp/env.sock

2. Cleanup Phase (with corrected drop order):

   • socket_guard drops first
     • Restores COMENQD_SOCKET_PATH to captured state: unset
   • env_guard drops second
     • Restores COMENQD_SOCKET_PATH to captured state: unset
   • Final state: COMENQD_SOCKET_PATH is unset ✓

Verification Commands

After implementing the fix, run:

cargo test --test '*' -- --test-threads=1

Or specifically for the config feature tests:

cargo test --test config_steps

Ensure all tests pass, particularly the "environment variable overrides file" scenario.

Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the Drop implementation for ConfigWorld. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios. The fix is minimal, focused, and addresses the root cause of the issue without requiring any additional refactoring.

Import In IDE

🤖 Prompt for AI Agents

## Observations

The `ConfigWorld` struct in `tests/steps/config_steps.rs` contains two `EnvVarGuard` fields (`env_guard` and `socket_guard`) that can both manage the `COMENQD_SOCKET_PATH` environment variable. The current `Drop` implementation (lines 136-145) drops `env_guard` before `socket_guard`, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (lines 14-18 in `tests/features/config.feature`) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

## Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since `socket_guard` is typically set first (in `config_file_with_token` at line 32) and `env_guard` is set later (in `set_env_var` at line 90), the drop order should be reversed to drop `socket_guard` after `env_guard`. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

## Implementation Steps

### 1. Fix Drop Order in ConfigWorld

**File**: `tests/steps/config_steps.rs`

**Location**: Lines 136-145 (the `Drop` implementation for `ConfigWorld`)

**Change**: Reverse the order of guard drops in the `drop` method:

```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

- Move the `socket_guard` drop block to execute **before** the `env_guard` drop block
- This ensures that the most recently set guard (`env_guard`) is dropped first, followed by the earlier guard (`socket_guard`)
- The guards will restore environment variables in the correct LIFO order

**Rationale**: When both guards manage the same environment variable key (`COMENQD_SOCKET_PATH`):
- `socket_guard` is created first (captures original state)
- `env_guard` is created second (captures state after socket_guard modification)
- On drop, `env_guard` should restore first (to socket_guard's state)
- Then `socket_guard` should restore (to original state)

### 2. Verify Test Behavior

**File**: `tests/features/config.feature`

**Scenario to verify**: "environment variable overrides file" (lines 14-18)

**Verification steps**:
- Run the cucumber test suite with `cargo test` or the appropriate test command
- Ensure the scenario passes with the corrected drop order
- Confirm that `COMENQD_SOCKET_PATH` is properly cleaned up after the test
- Verify no environment variable pollution occurs between test scenarios

### 3. Review Related Code

**Files to review**:
- `test-support/src/env_guard.rs` - Confirm `EnvVarGuard::Drop` implementation is correct (it is)
- `tests/steps/config_steps.rs` - Review all functions that set `env_guard` or `socket_guard` to understand the creation order

**Functions to review**:
- `config_file_with_token` (line 26) - sets `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
- `config_without_socket` (line 61) - sets `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
- `set_env_var` (line 89) - sets `env_guard` via `EnvVarGuard::set(&key, &value)`

**Confirmation**: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

## Execution Details

### Code Change Summary

The fix is a simple reordering of two blocks in the `Drop` implementation:

**Before**:
```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

**After**:
```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

### Test Scenario Walkthrough

For the "environment variable overrides file" scenario (lines 14-18 of `config.feature`):

1. **Setup Phase**:
   - `config_file_with_token("abc")` creates `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
     - Captures original state: `COMENQD_SOCKET_PATH` is unset
     - Removes the variable from environment
   - `set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock")` creates `env_guard` via `EnvVarGuard::set()`
     - Captures current state: `COMENQD_SOCKET_PATH` is unset (after socket_guard removed it)
     - Sets `COMENQD_SOCKET_PATH` to `/tmp/env.sock`

2. **Cleanup Phase (with corrected drop order)**:
   - `socket_guard` drops first
     - Restores `COMENQD_SOCKET_PATH` to captured state: unset
   - `env_guard` drops second
     - Restores `COMENQD_SOCKET_PATH` to captured state: unset
   - Final state: `COMENQD_SOCKET_PATH` is unset ✓

### Verification Commands

After implementing the fix, run:
```bash
cargo test --test '*' -- --test-threads=1
```

Or specifically for the config feature tests:
```bash
cargo test --test config_steps
```

Ensure all tests pass, particularly the "environment variable overrides file" scenario.

## Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the `Drop` implementation for `ConfigWorld`. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios. The fix is minimal, focused, and addresses the root cause of the issue without requiring any additional refactoring.

---

Execution Information

Branch: main
Commit: 86bc6e7

---

💡 Tips

Supported Commands (Inside Comments)

• Use @traycerai generate to iterate on the previous version of the implementation plan.

Supported Commands (Inside Description)

• Add @traycerai ignore anywhere in the ticket description to prevent this ticket from being processed.
• Add @traycerai branch:<branch-name> anywhere in the ticket description to specify the target branch for the implementation plan.

Community

• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[traycerai]

I have updated the Implementation Plan to reflect the recent changes in the ticket.

Previous Plan

Observations

The ConfigWorld struct in file:tests/steps/config_steps.rs contains two EnvVarGuard fields (env_guard and socket_guard) that can both manage the COMENQD_SOCKET_PATH environment variable. The current Drop implementation (lines 136-145) drops env_guard before socket_guard, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (line 14-18 in file:tests/features/config.feature) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since socket_guard is typically set first (in config_file_with_token at line 32) and env_guard is set later (in set_env_var at line 90), the drop order should be reversed to drop socket_guard after env_guard. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

Implementation Steps

1. Fix Drop Order in ConfigWorld

File: file:tests/steps/config_steps.rs

Location: Lines 136-145 (the Drop implementation for ConfigWorld)

Change: Reverse the order of guard drops in the drop method:

• Move the socket_guard drop block to execute before the env_guard drop block
• This ensures that the most recently set guard (env_guard) is dropped first, followed by the earlier guard (socket_guard)
• The guards will restore environment variables in the correct LIFO order

Rationale: When both guards manage the same environment variable key (COMENQD_SOCKET_PATH):

• socket_guard is created first (captures original state)
• env_guard is created second (captures state after socket_guard modification)
• On drop, env_guard should restore first (to socket_guard's state)
• Then socket_guard should restore (to original state)

2. Verify Test Behavior

File: file:tests/features/config.feature

Scenario to verify: "environment variable overrides file" (lines 14-18)

Verification steps:

• Run the cucumber test suite with cargo test or the appropriate test command
• Ensure the scenario passes with the corrected drop order
• Confirm that COMENQD_SOCKET_PATH is properly cleaned up after the test
• Verify no environment variable pollution occurs between test scenarios

3. Review Related Code

Files to review:

• file:test-support/src/env_guard.rs - Confirm EnvVarGuard::Drop implementation is correct (it is)
• file:tests/steps/config_steps.rs - Review all functions that set env_guard or socket_guard to understand the creation order

Functions to review:

• config_file_with_token (line 26) - sets socket_guard
• config_without_socket (line 61) - sets socket_guard
• set_env_var (line 89) - sets env_guard

Confirmation: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

Visual Representation

sequenceDiagram
    participant Test as Test Scenario
    participant CW as ConfigWorld
    participant SG as socket_guard
    participant EG as env_guard
    participant Env as Environment

    Note over Test,Env: Setup Phase
    Test->>CW: config_file_with_token("abc")
    CW->>SG: EnvVarGuard::remove("COMENQD_SOCKET_PATH")
    SG->>Env: Capture original (e.g., None)
    SG->>Env: Remove variable
    
    Test->>CW: set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock")
    CW->>EG: EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/env.sock")
    EG->>Env: Capture current (None, after socket_guard removed it)
    EG->>Env: Set to "/tmp/env.sock"
    
    Note over Test,Env: Cleanup Phase (CORRECTED ORDER)
    Test->>CW: Drop ConfigWorld
    CW->>EG: Drop env_guard first (LIFO)
    EG->>Env: Restore to None
    CW->>SG: Drop socket_guard second
    SG->>Env: Restore to original (None)
    
    Note over Env: Final state: COMENQD_SOCKET_PATH is None ✓

Loading

Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the Drop implementation for ConfigWorld. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios.

Import In IDE

🤖 Prompt for AI Agents

## Observations

The `ConfigWorld` struct in `file:tests/steps/config_steps.rs` contains two `EnvVarGuard` fields (`env_guard` and `socket_guard`) that can both manage the `COMENQD_SOCKET_PATH` environment variable. The current `Drop` implementation (lines 136-145) drops `env_guard` before `socket_guard`, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (line 14-18 in `file:tests/features/config.feature`) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

## Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since `socket_guard` is typically set first (in `config_file_with_token` at line 32) and `env_guard` is set later (in `set_env_var` at line 90), the drop order should be reversed to drop `socket_guard` after `env_guard`. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

## Implementation Steps

### 1. Fix Drop Order in ConfigWorld

**File**: `file:tests/steps/config_steps.rs`

**Location**: Lines 136-145 (the `Drop` implementation for `ConfigWorld`)

**Change**: Reverse the order of guard drops in the `drop` method:

- Move the `socket_guard` drop block to execute **before** the `env_guard` drop block
- This ensures that the most recently set guard (`env_guard`) is dropped first, followed by the earlier guard (`socket_guard`)
- The guards will restore environment variables in the correct LIFO order

**Rationale**: When both guards manage the same environment variable key (`COMENQD_SOCKET_PATH`):
- `socket_guard` is created first (captures original state)
- `env_guard` is created second (captures state after socket_guard modification)
- On drop, `env_guard` should restore first (to socket_guard's state)
- Then `socket_guard` should restore (to original state)

### 2. Verify Test Behavior

**File**: `file:tests/features/config.feature`

**Scenario to verify**: "environment variable overrides file" (lines 14-18)

**Verification steps**:
- Run the cucumber test suite with `cargo test` or the appropriate test command
- Ensure the scenario passes with the corrected drop order
- Confirm that `COMENQD_SOCKET_PATH` is properly cleaned up after the test
- Verify no environment variable pollution occurs between test scenarios

### 3. Review Related Code

**Files to review**:
- `file:test-support/src/env_guard.rs` - Confirm `EnvVarGuard::Drop` implementation is correct (it is)
- `file:tests/steps/config_steps.rs` - Review all functions that set `env_guard` or `socket_guard` to understand the creation order

**Functions to review**:
- `config_file_with_token` (line 26) - sets `socket_guard`
- `config_without_socket` (line 61) - sets `socket_guard`
- `set_env_var` (line 89) - sets `env_guard`

**Confirmation**: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

## Visual Representation

```mermaid
sequenceDiagram
    participant Test as Test Scenario
    participant CW as ConfigWorld
    participant SG as socket_guard
    participant EG as env_guard
    participant Env as Environment

    Note over Test,Env: Setup Phase
    Test->>CW: config_file_with_token("abc")
    CW->>SG: EnvVarGuard::remove("COMENQD_SOCKET_PATH")
    SG->>Env: Capture original (e.g., None)
    SG->>Env: Remove variable
    
    Test->>CW: set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock")
    CW->>EG: EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/env.sock")
    EG->>Env: Capture current (None, after socket_guard removed it)
    EG->>Env: Set to "/tmp/env.sock"
    
    Note over Test,Env: Cleanup Phase (CORRECTED ORDER)
    Test->>CW: Drop ConfigWorld
    CW->>EG: Drop env_guard first (LIFO)
    EG->>Env: Restore to None
    CW->>SG: Drop socket_guard second
    SG->>Env: Restore to original (None)
    
    Note over Env: Final state: COMENQD_SOCKET_PATH is None ✓
```

## Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the `Drop` implementation for `ConfigWorld`. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios.

---

Execution Information

Branch: main
Commit: 86bc6e7

New Plan

Observations

The ConfigWorld struct in tests/steps/config_steps.rs contains two EnvVarGuard fields (env_guard and socket_guard) that can both manage the COMENQD_SOCKET_PATH environment variable. The current Drop implementation (lines 136-145) drops env_guard before socket_guard, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (lines 14-18 in tests/features/config.feature) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since socket_guard is typically set first (in config_file_with_token at line 32) and env_guard is set later (in set_env_var at line 90), the drop order should be reversed to drop socket_guard after env_guard. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

Implementation Steps

1. Fix Drop Order in ConfigWorld

File: tests/steps/config_steps.rs

Location: Lines 136-145 (the Drop implementation for ConfigWorld)

Change: Reverse the order of guard drops in the drop method:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

• Move the socket_guard drop block to execute before the env_guard drop block
• This ensures that the most recently set guard (env_guard) is dropped first, followed by the earlier guard (socket_guard)
• The guards will restore environment variables in the correct LIFO order

Rationale: When both guards manage the same environment variable key (COMENQD_SOCKET_PATH):

• socket_guard is created first (captures original state)
• env_guard is created second (captures state after socket_guard modification)
• On drop, env_guard should restore first (to socket_guard's state)
• Then socket_guard should restore (to original state)

2. Verify Test Behavior

File: tests/features/config.feature

Scenario to verify: "environment variable overrides file" (lines 14-18)

Verification steps:

• Run the cucumber test suite with cargo test or the appropriate test command
• Ensure the scenario passes with the corrected drop order
• Confirm that COMENQD_SOCKET_PATH is properly cleaned up after the test
• Verify no environment variable pollution occurs between test scenarios

3. Review Related Code

Files to review:

• test-support/src/env_guard.rs - Confirm EnvVarGuard::Drop implementation is correct (it is)
• tests/steps/config_steps.rs - Review all functions that set env_guard or socket_guard to understand the creation order

Functions to review:

• config_file_with_token (line 26) - sets socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
• config_without_socket (line 61) - sets socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
• set_env_var (line 89) - sets env_guard via EnvVarGuard::set(&key, &value)

Confirmation: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

Execution Details

Code Change Summary

The fix is a simple reordering of two blocks in the Drop implementation:

Before:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

After:

impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}

Test Scenario Walkthrough

For the "environment variable overrides file" scenario (lines 14-18 of config.feature):

1. Setup Phase:

   • config_file_with_token("abc") creates socket_guard via EnvVarGuard::remove("COMENQD_SOCKET_PATH")
     • Captures original state: COMENQD_SOCKET_PATH is unset
     • Removes the variable from environment
   • set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock") creates env_guard via EnvVarGuard::set()
     • Captures current state: COMENQD_SOCKET_PATH is unset (after socket_guard removed it)
     • Sets COMENQD_SOCKET_PATH to /tmp/env.sock

2. Cleanup Phase (with corrected drop order):

   • socket_guard drops first
     • Restores COMENQD_SOCKET_PATH to captured state: unset
   • env_guard drops second
     • Restores COMENQD_SOCKET_PATH to captured state: unset
   • Final state: COMENQD_SOCKET_PATH is unset ✓

Verification Commands

After implementing the fix, run:

cargo test --test '*' -- --test-threads=1

Or specifically for the config feature tests:

cargo test --test config_steps

Ensure all tests pass, particularly the "environment variable overrides file" scenario.

Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the Drop implementation for ConfigWorld. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios. The fix is minimal, focused, and addresses the root cause of the issue without requiring any additional refactoring.

Import In IDE

🤖 Prompt for AI Agents

## Observations

The `ConfigWorld` struct in `tests/steps/config_steps.rs` contains two `EnvVarGuard` fields (`env_guard` and `socket_guard`) that can both manage the `COMENQD_SOCKET_PATH` environment variable. The current `Drop` implementation (lines 136-145) drops `env_guard` before `socket_guard`, violating LIFO (Last In, First Out) ordering. This creates a potential issue in scenarios like "environment variable overrides file" (lines 14-18 in `tests/features/config.feature`) where both guards are set for the same key, potentially reintroducing the environment variable unexpectedly on cleanup.

## Approach

The fix follows the LIFO principle for nested resource management: guards should be dropped in reverse order of their creation. Since `socket_guard` is typically set first (in `config_file_with_token` at line 32) and `env_guard` is set later (in `set_env_var` at line 90), the drop order should be reversed to drop `socket_guard` after `env_guard`. This ensures proper restoration of environment variable states and prevents unexpected reintroduction of values.

## Implementation Steps

### 1. Fix Drop Order in ConfigWorld

**File**: `tests/steps/config_steps.rs`

**Location**: Lines 136-145 (the `Drop` implementation for `ConfigWorld`)

**Change**: Reverse the order of guard drops in the `drop` method:

```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

- Move the `socket_guard` drop block to execute **before** the `env_guard` drop block
- This ensures that the most recently set guard (`env_guard`) is dropped first, followed by the earlier guard (`socket_guard`)
- The guards will restore environment variables in the correct LIFO order

**Rationale**: When both guards manage the same environment variable key (`COMENQD_SOCKET_PATH`):
- `socket_guard` is created first (captures original state)
- `env_guard` is created second (captures state after socket_guard modification)
- On drop, `env_guard` should restore first (to socket_guard's state)
- Then `socket_guard` should restore (to original state)

### 2. Verify Test Behavior

**File**: `tests/features/config.feature`

**Scenario to verify**: "environment variable overrides file" (lines 14-18)

**Verification steps**:
- Run the cucumber test suite with `cargo test` or the appropriate test command
- Ensure the scenario passes with the corrected drop order
- Confirm that `COMENQD_SOCKET_PATH` is properly cleaned up after the test
- Verify no environment variable pollution occurs between test scenarios

### 3. Review Related Code

**Files to review**:
- `test-support/src/env_guard.rs` - Confirm `EnvVarGuard::Drop` implementation is correct (it is)
- `tests/steps/config_steps.rs` - Review all functions that set `env_guard` or `socket_guard` to understand the creation order

**Functions to review**:
- `config_file_with_token` (line 26) - sets `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
- `config_without_socket` (line 61) - sets `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
- `set_env_var` (line 89) - sets `env_guard` via `EnvVarGuard::set(&key, &value)`

**Confirmation**: Ensure no other scenarios create guards in unexpected orders that might be affected by this change

## Execution Details

### Code Change Summary

The fix is a simple reordering of two blocks in the `Drop` implementation:

**Before**:
```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

**After**:
```rust
impl Drop for ConfigWorld {
    fn drop(&mut self) {
        if let Some(_guard) = self.socket_guard.take() {
            // dropping the guard restores the previous state
        }
        if let Some(_guard) = self.env_guard.take() {
            // dropping the guard restores the previous state
        }
    }
}
```

### Test Scenario Walkthrough

For the "environment variable overrides file" scenario (lines 14-18 of `config.feature`):

1. **Setup Phase**:
   - `config_file_with_token("abc")` creates `socket_guard` via `EnvVarGuard::remove("COMENQD_SOCKET_PATH")`
     - Captures original state: `COMENQD_SOCKET_PATH` is unset
     - Removes the variable from environment
   - `set_env_var("COMENQD_SOCKET_PATH", "/tmp/env.sock")` creates `env_guard` via `EnvVarGuard::set()`
     - Captures current state: `COMENQD_SOCKET_PATH` is unset (after socket_guard removed it)
     - Sets `COMENQD_SOCKET_PATH` to `/tmp/env.sock`

2. **Cleanup Phase (with corrected drop order)**:
   - `socket_guard` drops first
     - Restores `COMENQD_SOCKET_PATH` to captured state: unset
   - `env_guard` drops second
     - Restores `COMENQD_SOCKET_PATH` to captured state: unset
   - Final state: `COMENQD_SOCKET_PATH` is unset ✓

### Verification Commands

After implementing the fix, run:
```bash
cargo test --test '*' -- --test-threads=1
```

Or specifically for the config feature tests:
```bash
cargo test --test config_steps
```

Ensure all tests pass, particularly the "environment variable overrides file" scenario.

## Summary

The implementation requires a single, straightforward change: reversing the drop order of the two guards in the `Drop` implementation for `ConfigWorld`. This ensures proper LIFO cleanup semantics and prevents environment variable pollution between test scenarios. The fix is minimal, focused, and addresses the root cause of the issue without requiring any additional refactoring.

---

Execution Information

Branch: main
Commit: 86bc6e7