fix step insertion

2026-01-23 13:01:14 +08:00 · 2026-01-22 11:22:13 +00:00
parent 8fbe9aa963
commit 514d122c9d
4 changed files with 376 additions and 18 deletions
--- a/.opencode/plans/fix-step-here-insertion.md
+++ b/.opencode/plans/fix-step-here-insertion.md
@@ -0,0 +1,281 @@
+# 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)