runner/.opencode/plans/fix-step-here-insertion.md

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):

/// <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):

private bool _hasStepInsertedHere;

Add property implementation (around line 329):

public bool HasStepInsertedHere => _hasStepInsertedHere;

Add methods:

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):

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):

// 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:

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)