fix(commons): resolve Commons-008 — replace ValueTuple in SetConnectionBindingsCommand with named ConnectionBinding record (CLI, ManagementService, TemplateEngine, CentralUI)

code-reviews/Commons/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 1 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -357,7 +357,7 @@ carve-out makes these types compliant by design.
 |--|--|
 | Severity | Low |
 | Category | Akka.NET conventions |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs:10` |
 
 **Description**
@@ -379,18 +379,26 @@ Replace the tuple with a small named record, e.g.
 
 **Resolution**
 
-_Open — deferred to a coordinated multi-module change._ The finding is confirmed valid:
-the `ValueTuple` is the only positional/tuple element in `Messages/` and is unfriendly to
-REQ-COM-5a additive evolution. However, `SetConnectionBindingsCommand.Bindings` is not a
-Commons-internal type — its `IReadOnlyList<(string,int)>` shape is part of a cross-module
-contract consumed by `ScadaLink.CLI` (`InstanceCommands.TryParseBindings` builds a
-`List<(string,int)>` and passes it to the constructor), `ScadaLink.ManagementService`
-(`ManagementActor` forwards `cmd.Bindings`), and `ScadaLink.TemplateEngine`
-(`InstanceService.SetConnectionBindingsAsync` takes an `IReadOnlyList<(string AttributeName,
-int DataConnectionId)>` parameter). Introducing a `ConnectionBinding` record therefore
-requires editing those three modules in lock-step, which is outside the scope of this
-Commons-only review pass (the constraint forbids touching other modules' source). Left
-Open and flagged for a follow-up cross-module change; the fix itself is unambiguous.
+Resolved 2026-05-16 (commit `<pending>`) — confirmed against the source: the `ValueTuple`
+was the only positional/tuple element in `Messages/` and `System.Text.Json` serializes it
+as `Item1`/`Item2`, unfriendly to REQ-COM-5a additive evolution. Introduced a named record
+`ConnectionBinding(string AttributeName, int DataConnectionId)` in
+`Messages/Management/InstanceCommands.cs` (alongside the command, matching the existing
+record-per-file convention) and changed `SetConnectionBindingsCommand.Bindings` to
+`IReadOnlyList<ConnectionBinding>`. All consumers were updated in lock-step: `ScadaLink.CLI`
+(`InstanceCommands.TryParseBindings` now builds a `List<ConnectionBinding>`),
+`ScadaLink.TemplateEngine` (`InstanceService.SetConnectionBindingsAsync` parameter),
+`ScadaLink.ManagementService` (`ManagementActor` forwards `cmd.Bindings` unchanged — the
+new element type flows through), and one consumer the finding did not list,
+`ScadaLink.CentralUI` (`InstanceConfigure.razor`'s `SaveBindings` built a `List<(string,int)>`).
+A repo-wide `src/` scan confirms no other references remain. Regression tests added in
+`ConnectionBindingSerializationTests` (`ConnectionBinding_SerializesWithNamedProperties`
+asserts named `AttributeName`/`DataConnectionId` JSON properties and the absence of
+`Item1`/`Item2`; `SetConnectionBindingsCommand_RoundTripsThroughJson` asserts a full
+JSON round-trip); existing parse/forward tests (`ParseBindings_ValidJson_ReturnsPairs`,
+`SetConnectionBindings_BulkAssignment_Success`) were updated to the new type. Affected
+suites are green (Commons 226, CLI 77, ManagementService 55, TemplateEngine 287) and
+`dotnet build ScadaLink.slnx` is clean.
 
 ### Commons-009 — `Component-Commons.md` is stale relative to the actual file set
 

src/ScadaLink.CLI/Commands/InstanceCommands.cs

@@ -73,7 +73,7 @@ public static class InstanceCommands
     /// </summary>
     internal static bool TryParseBindings(
         string json,
-        out List<(string, int)>? bindings,
+        out List<ConnectionBinding>? bindings,
         out string? error)
     {
         bindings = null;
@@ -88,7 +88,7 @@ public static class InstanceCommands
                 return false;
             }
 
-            var result = new List<(string, int)>(pairs.Count);
+            var result = new List<ConnectionBinding>(pairs.Count);
             foreach (var pair in pairs)
             {
                 if (pair.Count != 2)
@@ -107,7 +107,7 @@ public static class InstanceCommands
                     error = "The second element of each binding (dataConnectionId) must be an integer.";
                     return false;
                 }
-                result.Add((pair[0].GetString()!, connectionId));
+                result.Add(new ConnectionBinding(pair[0].GetString()!, connectionId));
             }
 
             bindings = result;

src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor

@@ -4,6 +4,7 @@
 @using ScadaLink.Commons.Entities.Sites
 @using ScadaLink.Commons.Entities.Templates
 @using ScadaLink.Commons.Interfaces.Repositories
+@using ScadaLink.Commons.Messages.Management
 @using ScadaLink.Commons.Types.Enums
 @using ScadaLink.TemplateEngine.Flattening
 @using ScadaLink.TemplateEngine.Services
@@ -467,7 +468,7 @@
         _saving = true;
         try
         {
-            var bindings = _bindingSelections.Select(kv => (kv.Key, kv.Value)).ToList();
+            var bindings = _bindingSelections.Select(kv => new ConnectionBinding(kv.Key, kv.Value)).ToList();
             var user = await GetCurrentUserAsync();
             var result = await InstanceService.SetConnectionBindingsAsync(Id, bindings, user);
             if (result.IsSuccess)

src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs

@@ -7,7 +7,15 @@ public record MgmtDeployInstanceCommand(int InstanceId);
 public record MgmtEnableInstanceCommand(int InstanceId);
 public record MgmtDisableInstanceCommand(int InstanceId);
 public record MgmtDeleteInstanceCommand(int InstanceId);
-public record SetConnectionBindingsCommand(int InstanceId, IReadOnlyList<(string AttributeName, int DataConnectionId)> Bindings);
+/// <summary>
+/// A single attribute-to-data-connection binding carried by
+/// <see cref="SetConnectionBindingsCommand"/>. This is a named record (not a
+/// <c>ValueTuple</c>) so it serializes with stable, named JSON properties and can
+/// evolve additively per REQ-COM-5a.
+/// </summary>
+public record ConnectionBinding(string AttributeName, int DataConnectionId);
+
+public record SetConnectionBindingsCommand(int InstanceId, IReadOnlyList<ConnectionBinding> Bindings);
 public record SetInstanceOverridesCommand(int InstanceId, IReadOnlyDictionary<string, string?> Overrides);
 public record SetInstanceAreaCommand(int InstanceId, int? AreaId);
 

src/ScadaLink.TemplateEngine/Services/InstanceService.cs

@@ -1,6 +1,7 @@
 using ScadaLink.Commons.Entities.Instances;
 using ScadaLink.Commons.Interfaces.Repositories;
 using ScadaLink.Commons.Interfaces.Services;
+using ScadaLink.Commons.Messages.Management;
 using ScadaLink.Commons.Types;
 using ScadaLink.Commons.Types.Enums;
 using ScadaLink.TemplateEngine;
@@ -276,7 +277,7 @@ public class InstanceService
     /// </summary>
     public async Task<Result<IReadOnlyList<InstanceConnectionBinding>>> SetConnectionBindingsAsync(
         int instanceId,
-        IReadOnlyList<(string AttributeName, int DataConnectionId)> bindings,
+        IReadOnlyList<ConnectionBinding> bindings,
         string user,
         CancellationToken cancellationToken = default)
     {

tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs

@@ -1,4 +1,5 @@
 using ScadaLink.CLI.Commands;
+using ScadaLink.Commons.Messages.Management;
 
 namespace ScadaLink.CLI.Tests;
 
@@ -17,8 +18,8 @@ public class InstanceArgumentParsingTests
         Assert.True(ok);
         Assert.Null(error);
         Assert.Equal(2, bindings!.Count);
-        Assert.Equal(("Speed", 5), bindings[0]);
-        Assert.Equal(("Mode", 7), bindings[1]);
+        Assert.Equal(new ConnectionBinding("Speed", 5), bindings[0]);
+        Assert.Equal(new ConnectionBinding("Mode", 7), bindings[1]);
     }
 
     [Fact]

tests/ScadaLink.Commons.Tests/Messages/ConnectionBindingSerializationTests.cs

@@ -0,0 +1,54 @@
+using System.Text.Json;
+using ScadaLink.Commons.Messages.Management;
+
+namespace ScadaLink.Commons.Tests.Messages;
+
+/// <summary>
+/// Regression tests for Commons-008 — <see cref="SetConnectionBindingsCommand"/>
+/// previously declared its bindings as <c>IReadOnlyList&lt;(string, int)&gt;</c>.
+/// A <c>ValueTuple</c> serializes as <c>Item1</c>/<c>Item2</c> and cannot evolve
+/// additively (REQ-COM-5a). It is now a named <see cref="ConnectionBinding"/> record.
+/// </summary>
+public class ConnectionBindingSerializationTests
+{
+    [Fact]
+    public void ConnectionBinding_SerializesWithNamedProperties()
+    {
+        var json = JsonSerializer.Serialize(new ConnectionBinding("Temperature", 42));
+
+        using var doc = JsonDocument.Parse(json);
+        Assert.Equal(JsonValueKind.String, doc.RootElement.GetProperty("AttributeName").ValueKind);
+        Assert.Equal("Temperature", doc.RootElement.GetProperty("AttributeName").GetString());
+        Assert.Equal(42, doc.RootElement.GetProperty("DataConnectionId").GetInt32());
+
+        // The ValueTuple failure mode: Item1/Item2 must NOT appear.
+        Assert.False(doc.RootElement.TryGetProperty("Item1", out _));
+        Assert.False(doc.RootElement.TryGetProperty("Item2", out _));
+    }
+
+    [Fact]
+    public void SetConnectionBindingsCommand_RoundTripsThroughJson()
+    {
+        var original = new SetConnectionBindingsCommand(
+            7,
+            new List<ConnectionBinding>
+            {
+                new("Speed", 5),
+                new("Mode", 11),
+            });
+
+        var json = JsonSerializer.Serialize(original);
+        var deserialized = JsonSerializer.Deserialize<SetConnectionBindingsCommand>(json);
+
+        Assert.NotNull(deserialized);
+        Assert.Equal(7, deserialized!.InstanceId);
+        Assert.Equal(2, deserialized.Bindings.Count);
+        Assert.Equal("Speed", deserialized.Bindings[0].AttributeName);
+        Assert.Equal(5, deserialized.Bindings[0].DataConnectionId);
+        Assert.Equal("Mode", deserialized.Bindings[1].AttributeName);
+        Assert.Equal(11, deserialized.Bindings[1].DataConnectionId);
+
+        // ConnectionBinding is a record: each element compares by value.
+        Assert.Equal(original.Bindings, deserialized.Bindings);
+    }
+}

tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs

@@ -3,6 +3,7 @@ using ScadaLink.Commons.Entities.Instances;
 using ScadaLink.Commons.Entities.Templates;
 using ScadaLink.Commons.Interfaces.Repositories;
 using ScadaLink.Commons.Interfaces.Services;
+using ScadaLink.Commons.Messages.Management;
 using ScadaLink.Commons.Types.Enums;
 using ScadaLink.TemplateEngine.Services;
 
@@ -260,7 +261,7 @@ public class InstanceServiceTests
         _repoMock.Setup(r => r.SaveChangesAsync(It.IsAny<CancellationToken>()))
             .ReturnsAsync(1);
 
-        var bindings = new List<(string, int)> { ("Temp", 100), ("Pressure", 200) };
+        var bindings = new List<ConnectionBinding> { new("Temp", 100), new("Pressure", 200) };
         var result = await _sut.SetConnectionBindingsAsync(1, bindings, "admin");
 
         Assert.True(result.IsSuccess);