Fix F16 Step Rename: parameter mismatch, closed-.cs staleness, keyword duplication

src/LSP/Reqnroll.IdeSupport.LSP.Core/Rename/FeatureStepTextBuilder.cs

@@ -231,6 +231,84 @@ public static string Build(
         return sb.ToString();
     }
 
+    /// <summary>
+    /// Derives a new abstract expression from an in-place feature-file edit. <paramref
+    /// name="oldStepText"/> and <paramref name="newStepText"/> are the step's concrete text
+    /// (no keyword) before and after the user's edit in the rename dialog; <paramref
+    /// name="oldExpression"/> is the binding's current abstract expression. The parameter
+    /// values are located in <paramref name="oldStepText"/> using <paramref
+    /// name="oldExpression"/>'s static segments, then re-located verbatim in <paramref
+    /// name="newStepText"/> so the original parameter slots (<c>{int}</c>, a regex group, …)
+    /// can be preserved around whatever static wording the user typed. Returns <see
+    /// langword="null"/> when a parameter value can no longer be found verbatim in the edited
+    /// text — i.e. the user changed a parameter value rather than the step's wording, which
+    /// this flow does not support.
+    /// </summary>
+    public static string? DeriveExpressionFromEditedText(string oldExpression, string oldStepText, string newStepText)
+    {
+        var oldSegments = StepExpressionParameters.StaticSegments(oldExpression);
+
+        // No parameters: the whole step text is static, so it becomes the new expression as-is.
+        if (oldSegments.Count == 1)
+            return newStepText;
+
+        var prefix = oldSegments[0];
+        var suffix = oldSegments[oldSegments.Count - 1];
+        if (!oldStepText.StartsWith(prefix, StringComparison.Ordinal) ||
+            !oldStepText.EndsWith(suffix, StringComparison.Ordinal))
+            return null;
+
+        var regionStart = prefix.Length;
+        var regionEnd = oldStepText.Length - suffix.Length;
+        if (regionEnd < regionStart)
+            return null;
+
+        // Extract each parameter value from the original concrete text, in order.
+        var values = new List<string>();
+        var cursor = regionStart;
+        for (int i = 1; i < oldSegments.Count - 1; i++)
+        {
+            var seg = oldSegments[i];
+            int idx = seg.Length == 0
+                ? cursor
+                : oldStepText.IndexOf(seg, cursor, regionEnd - cursor, StringComparison.Ordinal);
+            if (idx < 0)
+                return null;
+            values.Add(oldStepText.Substring(cursor, idx - cursor));
+            cursor = idx + seg.Length;
+        }
+        values.Add(oldStepText.Substring(cursor, regionEnd - cursor));
+
+        var slots = StepExpressionParameters.ExtractSlots(oldExpression);
+        if (slots.Count != values.Count)
+            return null;
+
+        // Re-locate the same values, in order, within the edited text to recover the new
+        // static segments around them.
+        var searchFrom = 0;
+        var newSegments = new List<string>();
+        foreach (var value in values)
+        {
+            int idx = value.Length == 0
+                ? searchFrom
+                : newStepText.IndexOf(value, searchFrom, StringComparison.Ordinal);
+            if (idx < 0)
+                return null;
+            newSegments.Add(newStepText.Substring(searchFrom, idx - searchFrom));
+            searchFrom = idx + value.Length;
+        }
+        newSegments.Add(newStepText.Substring(searchFrom));
+
+        var result = new StringBuilder();
+        for (int i = 0; i < newSegments.Count; i++)
+        {
+            result.Append(newSegments[i]);
+            if (i < slots.Count)
+                result.Append(slots[i]);
+        }
+        return result.ToString();
+    }
+
     private static readonly System.Text.RegularExpressions.Regex PlaceholderPattern
         = new(@"\<([^>]+)\>", System.Text.RegularExpressions.RegexOptions.Compiled);
 

src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/StepRenameHandler.cs

@@ -146,19 +146,24 @@ public StepRenameHandler(
         // subsequent textDocument/rename would fail with "Internal Error".
         if (path.EndsWith(".feature", StringComparison.OrdinalIgnoreCase))
         {
-            var featureBindings = FindBindingsAtFeatureStep(uri, path, request.Position);
+            var featureBindings = FindBindingsAtFeatureStep(uri, path, request.Position, out var stepRange);
             if (featureBindings.Count == 0)
             {
                 _logger.LogVerbose("StepRenameHandler: prepareRename — no defined binding at feature step position");
                 return Task.FromResult<LspRange?>(null);
             }
 
-            var line = request.Position.Line;
-            return Task.FromResult<LspRange?>(new LspRange
+            if (stepRange == null)
             {
-                Start = new Position(line, 0),
-                End   = new Position(line, 200)
-            });
+                // Should not happen alongside a non-empty featureBindings, but refuse rather
+                // than fall back to a whole-line range: that used to seed the dialog with the
+                // keyword/indentation, which then got duplicated when the resulting edit was
+                // applied at the step-text-only range HandleRenameAsync actually replaces.
+                _logger.LogVerbose("StepRenameHandler: prepareRename — matched a binding but could not resolve the step's text range");
+                return Task.FromResult<LspRange?>(null);
+            }
+
+            return Task.FromResult<LspRange?>(stepRange);
         }
 
         return Task.FromResult<LspRange?>(null);
@@ -268,14 +273,15 @@ public StepRenameHandler(
             bindingLocation = new SourceLocation(path, line, column);
         }
 
-        // ── 2. Validate new name ───────────────────────────────────────────────
         var expression = binding.Expression ?? string.Empty;
-        var nameError = StepRenameValidator.ValidateNewName(expression, newName);
-        if (nameError != null)
-        {
-            _logger.LogVerbose($"StepRenameHandler: validation failed — {nameError.Message}");
-            return null;
-        }
+
+        // ── 2. Resolve feature step locations ──────────────────────────────────
+        var owners = _scopeManager.ResolveOwners(uri);
+        var projectFilter = owners.Count > 0
+            ? owners.Select(p => new ProjectOwner(p.ProjectFullName, p.TargetFrameworkMoniker)).ToArray()
+            : null;
+
+        var usages = _matchService.FindUsages(bindingLocation, projectFilter);
 
         // Resolve the live source expression once (preserves the original parameter syntax).
         // For a .cs-invoked rename this is the attribute string literal; otherwise it falls back
@@ -284,13 +290,63 @@ public StepRenameHandler(
         var sourceLiteral = await FindAttributeLiteralAsync(uri, binding);
         var sourceExpression = sourceLiteral?.Token.ValueText ?? expression;
 
-        // ── 3. Resolve feature step locations ──────────────────────────────────
-        var owners = _scopeManager.ResolveOwners(uri);
-        var projectFilter = owners.Count > 0
-            ? owners.Select(p => new ProjectOwner(p.ProjectFullName, p.TargetFrameworkMoniker)).ToArray()
-            : null;
+        // A .feature-triggered rename can arrive in two shapes, both via the same
+        // textDocument/rename call, with no protocol-level way to tell them apart:
+        //  - VS Code's native F2 seeds the dialog via prepareRename's whole-line range, so
+        //    `newName` comes back as concrete step text (real parameter values, e.g.
+        //    "I have 10 cukes" rather than "I have {int} cukes"). Comparing that straight
+        //    against the abstract expression always trips ValidateNewName's parameter-count
+        //    check, silently discarding every rename of a parameterized step.
+        //  - VS's custom "Rename Step" command builds its own prompt seeded with the binding's
+        //    abstract expression (RenameStepCommand.cs), so `newName` already carries the
+        //    correct placeholder syntax and needs no reconciliation — attempting it anyway would
+        //    fail to find any parameter "value" to locate in already-abstract text and wrongly
+        //    reject a rename that never needed fixing up.
+        // Try the abstract form first (matching parameter-slot count against the live source
+        // expression); only when that count differs do we attempt to derive the abstract
+        // expression by diffing the edited concrete text against the original.
+        var effectiveNewName = newName;
+        if (path.EndsWith(".feature", StringComparison.OrdinalIgnoreCase) &&
+            StepExpressionParameters.ExtractSlots(newName).Count != StepExpressionParameters.ExtractSlots(sourceExpression).Count)
+        {
+            var currentUsage = usages.FirstOrDefault(u =>
+                string.Equals(u.FeatureDocumentId, uri.ToString(), StringComparison.OrdinalIgnoreCase) &&
+                u.Range != null &&
+                request.Position.Line >= u.Range.ToLspRange().Start.Line &&
+                request.Position.Line <= u.Range.ToLspRange().End.Line);
+
+            var oldStepText = currentUsage?.Range != null
+                ? ReadStepText(uri, currentUsage.Range.ToLspRange())
+                : null;
+
+            if (oldStepText == null)
+            {
+                // Can't read the pre-edit step text (buffer and disk both unavailable) — fall
+                // back to treating newName as-is, same as before this reconciliation existed.
+                _logger.LogVerbose("StepRenameHandler: could not read original step text for the edited position — using newName as-is");
+            }
+            else
+            {
+                var derived = FeatureStepTextBuilder.DeriveExpressionFromEditedText(sourceExpression, oldStepText, newName);
+                if (derived == null)
+                {
+                    _logger.LogVerbose("StepRenameHandler: could not reconcile edited step text with the binding's parameter positions — the parameter values, not just the wording, appear to have changed");
+                    return null;
+                }
+
+                effectiveNewName = derived;
+                _logger.LogVerbose($"StepRenameHandler: derived abstract expression '{effectiveNewName}' from edited step text '{newName}'");
+            }
+        }
+
+        // ── 3. Validate new name ───────────────────────────────────────────────
+        var nameError = StepRenameValidator.ValidateNewName(expression, effectiveNewName);
+        if (nameError != null)
+        {
+            _logger.LogVerbose($"StepRenameHandler: validation failed — {nameError.Message}");
+            return null;
+        }
 
-        var usages = _matchService.FindUsages(bindingLocation, projectFilter);
         var changes = new Dictionary<DocumentUri, List<TextEdit>>();
 
         // ── 4. Build .feature file edits ───────────────────────────────────────
@@ -311,7 +367,7 @@ public StepRenameHandler(
                 stepText = ReadStepText(featureUri, stepRange);
             }
 
-            var featureNewText = FeatureStepTextBuilder.Build(newName, sourceExpression, binding.Regex, stepText);
+            var featureNewText = FeatureStepTextBuilder.Build(effectiveNewName, sourceExpression, binding.Regex, stepText);
             list.Add(new TextEdit
             {
                 Range = usage.Range!.ToLspRange(),
@@ -322,7 +378,7 @@ public StepRenameHandler(
         // ── 5. Build .cs file edit ────────────────────────────────────────────
         if (sourceLiteral != null)
         {
-            var csEdit = BuildCSharpEdit(sourceLiteral, newName);
+            var csEdit = BuildCSharpEdit(sourceLiteral, effectiveNewName);
             if (csEdit != null)
             {
                 var csUri = path.EndsWith(".cs", StringComparison.OrdinalIgnoreCase)
@@ -469,8 +525,22 @@ public StepRenameHandler(
     /// by querying the binding match cache for the owning projects.
     /// </summary>
     private List<ProjectStepDefinitionBinding> FindBindingsAtFeatureStep(
-        DocumentUri uri, string path, Position position)
+        DocumentUri uri, string path, Position position) =>
+        FindBindingsAtFeatureStep(uri, path, position, out _);
+
+    /// <summary>
+    /// Finds all bindings that match the feature step at the given cursor position, and the
+    /// matched step's own text span (excluding the keyword/indentation) via <paramref
+    /// name="matchedRange"/>. Callers that only need the range for editing (prepareRename must
+    /// offer exactly the text that HandleRenameAsync will later replace at <c>usage.Range</c> —
+    /// otherwise the keyword/indentation the client seeds the dialog with gets duplicated when
+    /// the edit is applied) should use this overload.
+    /// </summary>
+    private List<ProjectStepDefinitionBinding> FindBindingsAtFeatureStep(
+        DocumentUri uri, string path, Position position, out LspRange? matchedRange)
     {
+        matchedRange = null;
+
         var uriStr = uri.ToString();
         var owners = _scopeManager.ResolveOwners(uri);
         if (owners.Count == 0)
@@ -499,6 +569,7 @@ private List<ProjectStepDefinitionBinding> FindBindingsAtFeatureStep(
                     var stepEndChar   = (position.Line == endPos.Line)   ? endPos.Character   : int.MaxValue;
                     if (position.Character >= stepStartChar && position.Character <= stepEndChar)
                     {
+                        matchedRange ??= step.Range.ToLspRange();
                         foreach (var item in step.Result.Items)
                         {
                             if (item.MatchedStepDefinition != null)

src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/WorkspaceEditApplier.cs

@@ -167,13 +167,26 @@ internal static string ApplyEditsToText(string fileText, IEnumerable<TextEditIte
 
     private void NotifyDidChange(string localPath, string? newContent, CancellationToken cancellationToken)
     {
-        if (newContent is null || !localPath.EndsWith(".feature", StringComparison.OrdinalIgnoreCase))
+        if (newContent is null || !ShouldNotifyDidChange(localPath))
             return;
 
         _ = _service.SendDidChangeAsync(localPath, newContent, cancellationToken);
         _logger.LogInfo($"WorkspaceEditApplier: sent didChange for '{localPath}'.");
     }
 
+    /// <summary>
+    /// .cs files need this notification just as much as .feature files: the server's Roslyn
+    /// binding registry (CSharpBindingDiscoveryService) is only refreshed by textDocument/didOpen
+    /// or textDocument/didChange (TextDocumentSyncHandler) — there is no file-system watcher for
+    /// .cs content changes (WatchedFilesHandler only watches reqnroll.json/.editorconfig/output
+    /// assemblies, plus .cs *deletions*). Without this, a rename that rewrites a closed .cs file's
+    /// attribute leaves the registry stale — the renamed feature step shows unbound — until the
+    /// file happens to be opened, which triggers didOpen and a live re-parse.
+    /// </summary>
+    internal static bool ShouldNotifyDidChange(string localPath) =>
+        localPath.EndsWith(".feature", StringComparison.OrdinalIgnoreCase) ||
+        localPath.EndsWith(".cs", StringComparison.OrdinalIgnoreCase);
+
     // ── Helpers ──────────────────────────────────────────────────────────────
 
     /// <summary>Applies a single <see cref="TextEditItem"/> to a VS text buffer.</summary>

tests/LSP/Reqnroll.IdeSupport.LSP.Core.Tests/Rename/FeatureStepTextBuilderTests.cs

@@ -200,4 +200,48 @@ public void Build_outline_placeholder_count_mismatch_falls_through()
 
         result.Should().Be("the result is {int}");
     }
+
+    // ── DeriveExpressionFromEditedText: the inverse operation, used when a .feature-triggered
+    //    rename hands back the edited concrete step text (real parameter values) instead of an
+    //    abstract expression, so the rest of the rename pipeline can keep working with the
+    //    abstract form. ─────────────────────────────────────────────────────────────────────
+
+    [Fact]
+    public void DeriveExpressionFromEditedText_single_param_preserves_slot_and_wording_edit()
+    {
+        var result = FeatureStepTextBuilder.DeriveExpressionFromEditedText(
+            "I have {int} cukes", "I have 5 cukes", "I have 5 pickles");
+
+        result.Should().Be("I have {int} pickles");
+    }
+
+    [Fact]
+    public void DeriveExpressionFromEditedText_multiple_params_preserve_slots_in_order()
+    {
+        var result = FeatureStepTextBuilder.DeriveExpressionFromEditedText(
+            "the {int} was {string}", "the 3 was \"ok\"", "the 3 became \"ok\"");
+
+        result.Should().Be("the {int} became {string}");
+    }
+
+    [Fact]
+    public void DeriveExpressionFromEditedText_no_parameters_returns_edited_text_verbatim()
+    {
+        var result = FeatureStepTextBuilder.DeriveExpressionFromEditedText(
+            "to be or not to be", "to be or not to be", "to be and not to be");
+
+        result.Should().Be("to be and not to be");
+    }
+
+    [Fact]
+    public void DeriveExpressionFromEditedText_parameter_value_changed_returns_null()
+    {
+        // The user changed the value (5 → 6), not just the wording — this rename flow only
+        // supports renaming the step's wording, so it should be rejected rather than silently
+        // renaming the binding to accept a different concrete value.
+        var result = FeatureStepTextBuilder.DeriveExpressionFromEditedText(
+            "I have {int} cukes", "I have 5 cukes", "I have 6 cukes");
+
+        result.Should().BeNull();
+    }
 }