diff --git a/.opencode/plans/fix-step-here-insertion.md b/.opencode/plans/fix-step-here-insertion.md new file mode 100644 index 000000000..3cc6cc81a --- /dev/null +++ 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 +/// +/// 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. +/// +bool HasStepInsertedHere { get; } + +/// +/// Consumes the "step inserted here" flag (resets it to false). +/// Called by StepsRunner after handling the insertion. +/// +void ConsumeStepInsertedHere(); + +/// +/// Sets the "step inserted here" flag. +/// Called by StepManipulator when --here insertion occurs. +/// +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(); + 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 GetAllSteps() +{ + var result = new List(); + 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(); + 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) diff --git a/src/Runner.Worker/Dap/DapDebugSession.cs b/src/Runner.Worker/Dap/DapDebugSession.cs index 0422313ad..f3eb47951 100644 --- a/src/Runner.Worker/Dap/DapDebugSession.cs +++ b/src/Runner.Worker/Dap/DapDebugSession.cs @@ -247,6 +247,24 @@ namespace GitHub.Runner.Worker.Dap /// The index of the checkpoint to restore /// The job execution context void RestoreCheckpoint(int checkpointIndex, IExecutionContext jobContext); + + /// + /// 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. + /// + bool HasStepInsertedHere { get; } + + /// + /// Consumes the "step inserted here" flag (resets it to false). + /// Called by StepsRunner after handling the insertion. + /// + void ConsumeStepInsertedHere(); + + /// + /// Sets the "step inserted here" flag. + /// Called by StepManipulator when --here insertion occurs. + /// + void SetStepInsertedHere(); } /// @@ -320,6 +338,9 @@ namespace GitHub.Runner.Worker.Dap // Job cancellation token for REPL commands and blocking waits private CancellationToken _jobCancellationToken; + // Flag for step inserted "here" (before current step) + private bool _hasStepInsertedHere; + public bool IsActive => _state == DapSessionState.Ready || _state == DapSessionState.Paused || _state == DapSessionState.Running; public DapSessionState State => _state; @@ -328,6 +349,18 @@ namespace GitHub.Runner.Worker.Dap public bool HasPendingRestore => _pendingRestoreCheckpoint.HasValue; + public bool HasStepInsertedHere => _hasStepInsertedHere; + + public void ConsumeStepInsertedHere() + { + _hasStepInsertedHere = false; + } + + public void SetStepInsertedHere() + { + _hasStepInsertedHere = true; + } + public override void Initialize(IHostContext hostContext) { base.Initialize(hostContext); diff --git a/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs b/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs index 765667e00..7ecf1753c 100644 --- a/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs +++ b/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs @@ -4,6 +4,7 @@ using System.Linq; using GitHub.DistributedTask.Pipelines; using GitHub.Runner.Common; using GitHub.Runner.Sdk; +using GitHub.Runner.Worker.Dap; namespace GitHub.Runner.Worker.Dap.StepCommands { @@ -195,8 +196,13 @@ namespace GitHub.Runner.Worker.Dap.StepCommands index++; } - // Add current step if present - if (_currentStep != null) + // Check if we're in "step inserted here" state + var debugSession = HostContext.GetService(); + 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); @@ -207,12 +213,19 @@ namespace GitHub.Runner.Worker.Dap.StepCommands // Add pending steps from queue if (_jobContext?.JobSteps != null) { + bool isFirstPending = true; foreach (var step in _jobContext.JobSteps) { - var info = StepInfo.FromStep(step, index, StepStatus.Pending); + // 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; } } @@ -316,26 +329,29 @@ namespace GitHub.Runner.Worker.Dap.StepCommands var pending = _jobContext.JobSteps.ToList(); _jobContext.JobSteps.Clear(); - // Re-queue the current step at the front of pending - pending.Insert(0, _currentStep); - - // Insert the new step before it (at position 0) + // 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); } - // The new step becomes the current step - _currentStep = step; + // 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(); + debugSession?.SetStepInsertedHere(); - // Calculate the 1-based index (new step takes current step's position) + // 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.Current); + var stepInfo = StepInfo.FromStep(step, newIndex, StepStatus.Pending); stepInfo.Change = ChangeType.Added; if (step is IActionRunner runner && runner.Action != null) @@ -345,6 +361,9 @@ namespace GitHub.Runner.Worker.Dap.StepCommands _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; } @@ -461,22 +480,25 @@ namespace GitHub.Runner.Worker.Dap.StepCommands var step = pending[fromQueueIndex]; pending.RemoveAt(fromQueueIndex); - // Re-queue the current step at the front of pending - pending.Insert(0, _currentStep); - - // Insert the moved step before the current step (at position 0) + // Insert the moved 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); } - // The moved step becomes the current step - _currentStep = step; + // Signal to StepsRunner that it should skip the current iteration + // and re-process from the queue (which now has our moved step first) + var debugSession = HostContext.GetService(); + debugSession?.SetStepInsertedHere(); - // Calculate the new 1-based index (step takes current step's position) + // Calculate the new 1-based index (step takes position after completed steps) var newIndex = _completedSteps.Count + 1; // Track the change @@ -488,6 +510,9 @@ namespace GitHub.Runner.Worker.Dap.StepCommands _modifiedStepIds.Add(runner.Action.Id); } + // Note: We do NOT update _currentStep here. The StepsRunner will + // pick up the moved step from the queue and that will become current. + Trace.Info($"Moved step '{step.DisplayName}' from position {fromIndex} to {newIndex} (--here, before current step)"); return newIndex; } diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index e2e33bae6..e2ef195ab 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -242,6 +242,25 @@ namespace GitHub.Runner.Worker } } + // 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; + } + // User pressed next/continue - create checkpoint NOW // This captures any REPL modifications made while paused if (debugSession.ShouldCreateCheckpoint())