# Fix Step Addition with `--here` Position

## Problem Summary

When adding a step with `--here` position, two bugs occur:

1. **Duplicate display**: The newly added step appears twice in `steps list` - once as "current" and once in "pending"
2. **Wrong execution order**: After stepping over, the original step (checkout) runs instead of the newly added step

### Root Cause

When paused at a breakpoint, the current step has already been dequeued from `JobSteps` by `StepsRunner`. The `InsertStepHere` function:
1. Incorrectly re-inserts `_currentStep` back into the queue
2. Sets `_currentStep = newStep`, but newStep is also in the queue → duplicate
3. When execution continues, the loop iteration that already dequeued the original step continues to execute it, not the new step

### Why This Matters

The ability to insert a step that runs BEFORE the current step is essential for debugging workflows. Example scenario:
- Debugging a job, a step fails because it's missing something
- Step backwards in the debugger
- Need to add a new step here that downloads a dependency to fix the failing step
- Without proper `--here` support, you'd have to go back two steps (what if you don't have them?)

## Solution Overview

Implement a mechanism similar to step-back: when a step is inserted "here", signal `StepsRunner` to skip the current step execution and re-process from the modified queue.

## Files to Modify

| File | Changes |
|------|---------|
| `src/Runner.Worker/Dap/DapDebugSession.cs` | Add `HasStepInsertedHere` flag and `ConsumeStepInsertedHere()` method |
| `src/Runner.Worker/Dap/StepCommands/StepManipulator.cs` | Fix `InsertStepHere()` logic |
| `src/Runner.Worker/StepsRunner.cs` | Add check for `HasStepInsertedHere` after `OnStepStartingAsync` |

## Detailed Changes

### 1. `DapDebugSession.cs` - Add Interface and Implementation

**Add to `IDapDebugSession` interface (around line 166):**
```csharp
/// <summary>
/// Gets whether a step was inserted "here" (before current step) while paused.
/// When true, StepsRunner should skip current step execution and re-process from queue.
/// </summary>
bool HasStepInsertedHere { get; }

/// <summary>
/// Consumes the "step inserted here" flag (resets it to false).
/// Called by StepsRunner after handling the insertion.
/// </summary>
void ConsumeStepInsertedHere();

/// <summary>
/// Sets the "step inserted here" flag.
/// Called by StepManipulator when --here insertion occurs.
/// </summary>
void SetStepInsertedHere();
```

**Add field (around line 299):**
```csharp
private bool _hasStepInsertedHere;
```

**Add property implementation (around line 329):**
```csharp
public bool HasStepInsertedHere => _hasStepInsertedHere;
```

**Add methods:**
```csharp
public void ConsumeStepInsertedHere()
{
    _hasStepInsertedHere = false;
}

public void SetStepInsertedHere()
{
    _hasStepInsertedHere = true;
}
```

### 2. `StepManipulator.cs` - Fix `InsertStepHere()`

The manipulator needs access to the debug session to set the `HasStepInsertedHere` flag. Access via HostContext since `DapDebugSession` is already registered as a service.

**Modify `InsertStepHere()` method (lines 307-350):**

```csharp
private int InsertStepHere(IStep step)
{
    if (_currentStep == null)
    {
        throw new StepCommandException(StepCommandErrors.InvalidPosition,
            "Can only use --here when paused at a breakpoint.");
    }

    // Convert queue to list for manipulation
    var pending = _jobContext.JobSteps.ToList();
    _jobContext.JobSteps.Clear();

    // Insert the new step at the front (it will run first)
    pending.Insert(0, step);
    
    // Insert the original current step after it (it will run second)
    // This re-queues the step that was already dequeued by StepsRunner
    pending.Insert(1, _currentStep);

    // Re-queue all steps
    foreach (var s in pending)
    {
        _jobContext.JobSteps.Enqueue(s);
    }

    // Signal to StepsRunner that it should skip the current iteration
    // and re-process from the queue (which now has our new step first)
    var debugSession = HostContext.GetService<IDapDebugSession>();
    debugSession?.SetStepInsertedHere();

    // Calculate the 1-based index (new step takes position after completed steps)
    var newIndex = _completedSteps.Count + 1;

    // Track the change
    var stepInfo = StepInfo.FromStep(step, newIndex, StepStatus.Pending);
    stepInfo.Change = ChangeType.Added;

    if (step is IActionRunner runner && runner.Action != null)
    {
        _addedStepIds.Add(runner.Action.Id);
    }

    _changes.Add(StepChange.Added(stepInfo, newIndex));

    // Note: We do NOT update _currentStep here. The StepsRunner will
    // pick up the new step from the queue and that will become current.

    Trace.Info($"Inserted step '{step.DisplayName}' at position {newIndex} (--here, before current step)");
    return newIndex;
}
```

### 3. `StepsRunner.cs` - Handle the Flag

**Add check after `OnStepStartingAsync` (after line 243, following the step-back pattern):**

```csharp
// Check if a step was inserted "here" (before current step)
if (debugSession.HasStepInsertedHere)
{
    debugSession.ConsumeStepInsertedHere();
    
    // The queue now contains: [new step, original current step, rest...]
    // We need to skip this iteration and let the loop pick up the new step
    
    // Clear pending step info since we're not executing this step now
    debugSession.ClearPendingStepInfo();
    
    // Don't increment stepIndex - the new step takes this position
    
    Trace.Info("Step inserted here - skipping current iteration to process new step");
    
    // Skip to next iteration - will dequeue and process the new step
    continue;
}
```

### 4. Fix `GetAllSteps()` Display in `StepManipulator.cs`

After the changes above, when `InsertStepHere` is called:
- `_currentStep` is NOT changed (still points to original step)
- Queue contains: [new step, original step, rest...]
- The `HasStepInsertedHere` flag is set

When `GetAllSteps()` is called while paused (before `continue` in StepsRunner):
- completed = []
- current = original step (from `_currentStep`)
- pending = [new step, original step, rest...] (from queue)

This would show the original step twice (as current AND in pending). Need to handle this.

**Modify `GetAllSteps()` to check the flag:**

```csharp
public IReadOnlyList<StepInfo> GetAllSteps()
{
    var result = new List<StepInfo>();
    int index = 1;

    // Add completed steps
    foreach (var step in _completedSteps)
    {
        // ... existing code ...
    }

    // Check if we're in "step inserted here" state
    var debugSession = HostContext.GetService<IDapDebugSession>();
    var stepInsertedHere = debugSession?.HasStepInsertedHere ?? false;

    // Add current step if present AND not in "step inserted here" state
    // (In that state, the current step has been re-queued and will show in pending)
    if (_currentStep != null && !stepInsertedHere)
    {
        var info = StepInfo.FromStep(_currentStep, index, StepStatus.Current);
        ApplyChangeInfo(info);
        result.Add(info);
        index++;
    }

    // Add pending steps from queue
    if (_jobContext?.JobSteps != null)
    {
        bool isFirstPending = true;
        foreach (var step in _jobContext.JobSteps)
        {
            // In "step inserted here" state, mark the first pending step as current
            var status = (stepInsertedHere && isFirstPending) 
                ? StepStatus.Current 
                : StepStatus.Pending;
            
            var info = StepInfo.FromStep(step, index, status);
            ApplyChangeInfo(info);
            result.Add(info);
            index++;
            isFirstPending = false;
        }
    }

    return result;
}
```

## Execution Context Handling

The new step needs a proper execution context. `StepFactory.WrapInRunner()` creates a child context via `jobContext.CreateChild()`. 

When `StepsRunner` picks up the new step from the queue, it goes through the normal initialization (lines 86-137 in StepsRunner.cs) which sets up expression values, env context, etc. This should work correctly because the new step already has an `ExecutionContext` from `WrapInRunner()`.

The `Start()` method on ExecutionContext just sets up timing and state, so dynamically created steps should work fine.

## Test Scenarios

After implementing this fix, verify:

1. **Basic `--here` insertion**: 
   - Pause at step 1 (checkout)
   - `steps add run "echo hello" --here`
   - `steps list` shows: `▶ 1. hello [ADDED]`, `2. checkout`, `3. ...`
   - Step over → hello runs
   - `steps list` shows: `✓ 1. hello`, `▶ 2. checkout`, `3. ...`

2. **Multiple `--here` insertions**:
   - Pause at step 1
   - Add step A with `--here`
   - Add step B with `--here` (should insert before A)
   - Verify order: B, A, original

3. **`--here` after step-back**:
   - Run step 1, pause at step 2
   - Step back to step 1
   - Add new step with `--here`
   - Verify new step runs before step 1

4. **Other position options still work**:
   - `--first`, `--last`, `--after N`, `--before N` should be unaffected

## Implementation Checklist

- [ ] Add `HasStepInsertedHere` flag and methods to `IDapDebugSession` interface
- [ ] Implement the flag and methods in `DapDebugSession` class
- [ ] Modify `InsertStepHere()` in `StepManipulator` to set the flag and NOT modify `_currentStep`
- [ ] Add check in `StepsRunner` to handle `HasStepInsertedHere` with `continue`
- [ ] Update `GetAllSteps()` to correctly display steps when flag is set
- [ ] Test the scenarios listed above

## Additional Note: Default Position

There's also a documentation inconsistency: the help text says `--here` is the default, but the code defaults to `--last` (see `AddRunCommand` and `AddUsesCommand` in `StepCommandParser.cs`). This should be reviewed and either:
- Update the code to default to `--here` (matches docs)
- Update the docs to say `--last` is the default (matches code)