diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index 3ad0039..d1c45f8 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 3 | +| Open findings | 0 | ## Summary @@ -575,11 +575,27 @@ The CLI test suite went from 42 to 77 passing tests. | | | |--|--| -| Severity | Medium | -| Category | Correctness & logic bugs | -| Status | Open | +| Severity | Low | +| Category | Design-document adherence | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/TemplateCommands.cs:77`, `src/ScadaLink.CLI/Commands/SiteCommands.cs:86`, `src/ScadaLink.CLI/Commands/ExternalSystemCommands.cs:40-42`, `src/ScadaLink.CLI/Commands/DataConnectionCommands.cs:39-40`, `src/ScadaLink.CLI/Commands/NotificationCommands.cs:40-41`, `src/ScadaLink.CLI/Commands/ApiMethodCommands.cs:79` | +**Re-triage 2026-05-17:** the finding was originally filed as a Medium "Correctness & +logic bugs" issue, but verification against the Commons message contracts shows the +**code is correct** and the defect is purely documentation drift. Every `Update*Command` +record (`UpdateTemplateCommand`, `UpdateSiteCommand`, `UpdateDataConnectionCommand`, +`UpdateExternalSystemCommand`, `UpdateNotificationListCommand`, `UpdateApiMethodCommand`, +`UpdateTemplateAttribute/Alarm/ScriptCommand`, `UpdateRoleMappingCommand`, +`UpdateSharedScriptCommand`, `UpdateDatabaseConnectionDefCommand`, `UpdateAreaCommand`) +carries **non-nullable** "core" fields — they are *whole-entity replace* records, not +sparse patches. Marking the corresponding `--name` / `--protocol` / `--script` flags +`Required = true` is therefore the *correct* CLI behaviour: making them optional would +let an omitted flag send `null`/empty and silently blank the field server-side. Adopting +the sparse-patch model (recommendation option a) would require changing the Commons +records and the server-side Management handlers — both outside this module's editable +surface. Re-triaged to Low / Design-document adherence; resolved per recommendation +option (b). + **Description** The design doc presents `update` commands with all non-`--id` fields as optional, e.g. @@ -618,7 +634,20 @@ entity. Option (a) matches the documented surface and the typical CLI expectatio **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit pending) per recommendation option (b). Verification of the +Commons `Update*Command` records confirmed whole-replace is the intentional contract, so +the CLI's `Required = true` flags are correct and were left unchanged. The in-repo +`src/ScadaLink.CLI/README.md` — which is authoritative and previously listed every +`update` core field as optional `[--name]` — was corrected: the core flags +(`--name`/`--protocol`/`--script`/`--code`/`--emails`/`--endpoint-url`/`--auth-type`/ +`--data-type`/`--trigger-type`/`--priority`/`--connection-string`/`--ldap-group`/`--role`) +are now marked `required: yes`, the command synopses drop the `[...]`, and each `update` +section states that an update replaces the whole entity. Added +`UpdateCommandContractTests` (10 tests) pinning the whole-replace contract — every +listed `update` command's core flags must be `Required`, and the genuinely-sparse +`external-system method update` must remain optional. Note: `docs/requirements/Component-CLI.md` +also still shows these flags as optional, but it is outside this module's editable +surface — that doc-side correction is owned by the docs surface. ### CLI-015 — `Component-CLI.md` command surface has drifted again in two places @@ -626,8 +655,16 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Design-document adherence | -| Status | Open | -| Location | `docs/requirements/Component-CLI.md:75`, `docs/requirements/Component-CLI.md:125-126` (vs. `src/ScadaLink.CLI/Commands/TemplateCommands.cs:404-413`, `src/ScadaLink.CLI/Commands/DataConnectionCommands.cs:41`, `:86`) | +| Status | Resolved | +| Location | `docs/requirements/Component-CLI.md:75`, `docs/requirements/Component-CLI.md:125-126`, `src/ScadaLink.CLI/README.md` (vs. `src/ScadaLink.CLI/Commands/TemplateCommands.cs:404-413`, `src/ScadaLink.CLI/Commands/DataConnectionCommands.cs:41`, `:86`) | + +**Re-triage 2026-05-17:** verification found the same two drifts also present in the +in-repo `src/ScadaLink.CLI/README.md` (the authoritative reference): its +`template composition delete` section used the non-existent `--template-id` / +`--instance-name` form, and `data-connection create`/`update` documented only +`--configuration` without the canonical `--primary-config` flag (`--configuration` is in +fact an alias of `--primary-config`). The README is in this module's editable surface; +`docs/requirements/Component-CLI.md` is not. **Description** @@ -655,7 +692,15 @@ documented elsewhere. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit pending). Both drifts were present in the in-repo +`src/ScadaLink.CLI/README.md` and were corrected there (the README is this module's +authoritative reference): `template composition delete` now documents the real single +`--id ` form, and `data-connection create`/`update` now document `--primary-config` +(with the `--configuration` alias noted) alongside `--backup-config` and +`--failover-retry-count`. Added `CommandTreeTests.TemplateCompositionDelete_IsKeyedByIdOnly` +pinning the composition-delete surface (`--id` present; `--template-id`/`--instance-name` +absent). The corresponding edit to `docs/requirements/Component-CLI.md:75,125-126` is +outside this module's editable surface and remains for the docs surface to apply. ### CLI-016 — `WriteAsTable` derives columns from the first array element only @@ -663,7 +708,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:184-200` | **Description** @@ -687,4 +732,12 @@ rows, so no element's data is silently dropped. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit pending). Root cause confirmed — `WriteAsTable` derived the +header set from `items[0].EnumerateObject()` only, dropping any property unique to a +later element. The header set is now computed as the union of property names across +*every* object element of the array, in first-seen order (a `HashSet` guards duplicates +while an ordered list preserves column order). Rows already project against the header +list and `OutputFormatter.WriteTable` pads missing cells, so heterogeneous arrays now +render every column. Regression tests added in `TableHeaderUnionTests` (3 tests: +later-element-only column included, first-seen column order preserved, +first-element-extra column still rendered). diff --git a/src/ScadaLink.CLI/Commands/CommandHelpers.cs b/src/ScadaLink.CLI/Commands/CommandHelpers.cs index 6d6dc9b..ccb73ed 100644 --- a/src/ScadaLink.CLI/Commands/CommandHelpers.cs +++ b/src/ScadaLink.CLI/Commands/CommandHelpers.cs @@ -181,9 +181,25 @@ internal static class CommandHelpers return; } - var headers = items[0].ValueKind == JsonValueKind.Object - ? items[0].EnumerateObject().Select(p => p.Name).ToArray() - : new[] { "Value" }; + // Derive the header set as the union of property names across *every* + // element, in first-seen order. Using only items[0] would silently drop + // columns for any later element with a different shape (CLI-016). + var objectItems = items.Where(i => i.ValueKind == JsonValueKind.Object).ToList(); + string[] headers; + if (objectItems.Count > 0) + { + var seen = new List(); + var known = new HashSet(StringComparer.Ordinal); + foreach (var item in objectItems) + foreach (var prop in item.EnumerateObject()) + if (known.Add(prop.Name)) + seen.Add(prop.Name); + headers = seen.ToArray(); + } + else + { + headers = new[] { "Value" }; + } var rows = items.Select(item => { diff --git a/src/ScadaLink.CLI/README.md b/src/ScadaLink.CLI/README.md index d6b7ade..134af60 100644 --- a/src/ScadaLink.CLI/README.md +++ b/src/ScadaLink.CLI/README.md @@ -120,16 +120,18 @@ scadalink --url template create --name [--description ] [ #### `template update` -Update an existing template's name, description, or parent. +Update an existing template. An update **replaces** the whole entity — every required +field below must be supplied with the value it should have after the update, even if +it is unchanged. ```sh -scadalink --url template update --id [--name ] [--description ] [--parent-id ] +scadalink --url template update --id --name [--description ] [--parent-id ] ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Template ID | -| `--name` | no | Updated template name | +| `--name` | yes | Template name | | `--description` | no | Updated description | | `--parent-id` | no | Updated parent template ID | @@ -313,16 +315,15 @@ scadalink --url template composition add --template-id --module-temp #### `template composition delete` -Remove a feature module composition from a template. +Remove a feature module composition from a template, identified by its own composition ID. ```sh -scadalink --url template composition delete --template-id --instance-name +scadalink --url template composition delete --id ``` | Option | Required | Description | |--------|----------|-------------| -| `--template-id` | yes | Target template ID | -| `--instance-name` | yes | Instance name of the composed module to remove | +| `--id` | yes | Composition ID to remove | --- @@ -520,17 +521,17 @@ scadalink --url site area create --site-id --name [--parent #### `site area update` -Update an area's name or parent. +Update an area's name. An update **replaces** the whole entity — the required field +below must be supplied, even if unchanged. ```sh -scadalink --url site area update --id [--name ] [--parent-area-id ] +scadalink --url site area update --id --name ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Area ID | -| `--name` | no | Updated area name | -| `--parent-area-id` | no | Updated parent area ID | +| `--name` | yes | Area name | #### `site area delete` @@ -620,7 +621,7 @@ scadalink --url data-connection list [--site-id ] Create a new data connection belonging to a specific site. ```sh -scadalink --url data-connection create --site-id --name --protocol [--configuration ] +scadalink --url data-connection create --site-id --name --protocol [--primary-config ] [--backup-config ] [--failover-retry-count ] ``` | Option | Required | Description | @@ -628,22 +629,27 @@ scadalink --url data-connection create --site-id --name --p | `--site-id` | yes | Site ID the connection belongs to | | `--name` | yes | Connection name | | `--protocol` | yes | Protocol identifier (e.g. `OpcUa`) | -| `--configuration` | no | Protocol-specific configuration as a JSON string | +| `--primary-config` | no | Primary protocol-specific configuration as a JSON string (alias `--configuration`) | +| `--backup-config` | no | Backup protocol-specific configuration as a JSON string | +| `--failover-retry-count` | no | Retries before failover to the backup configuration (default: `3`) | #### `data-connection update` -Update a data connection definition. +Update a data connection definition. An update **replaces** the whole entity — every +required field below must be supplied, even if unchanged. ```sh -scadalink --url data-connection update --id [--name ] [--protocol ] [--configuration ] +scadalink --url data-connection update --id --name --protocol [--primary-config ] [--backup-config ] [--failover-retry-count ] ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Data connection ID | -| `--name` | no | Updated connection name | -| `--protocol` | no | Updated protocol identifier | -| `--configuration` | no | Updated protocol-specific configuration as a JSON string | +| `--name` | yes | Connection name | +| `--protocol` | yes | Protocol identifier | +| `--primary-config` | no | Primary protocol-specific configuration as a JSON string (alias `--configuration`) | +| `--backup-config` | no | Backup protocol-specific configuration as a JSON string | +| `--failover-retry-count` | no | Retries before failover to the backup configuration (default: `3`) | #### `data-connection delete` @@ -698,18 +704,19 @@ scadalink --url external-system create --name --endpoint-url external-system update --id [--name ] [--endpoint-url ] [--auth-type ] [--auth-config ] +scadalink --url external-system update --id --name --endpoint-url --auth-type [--auth-config ] ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | External system ID | -| `--name` | no | Updated display name | -| `--endpoint-url` | no | Updated base URL | -| `--auth-type` | no | Updated authentication type | +| `--name` | yes | Display name | +| `--endpoint-url` | yes | Base URL | +| `--auth-type` | yes | Authentication type | | `--auth-config` | no | Updated auth credentials as a JSON string | #### `external-system delete` @@ -763,17 +770,18 @@ scadalink --url notification create --name --emails notification update --id [--name ] [--emails ] +scadalink --url notification update --id --name --emails ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Notification list ID | -| `--name` | no | Updated list name | -| `--emails` | no | Updated comma-separated list of recipient email addresses | +| `--name` | yes | List name | +| `--emails` | yes | Comma-separated list of recipient email addresses | #### `notification delete` @@ -885,17 +893,18 @@ scadalink --url security role-mapping create --ldap-group --role #### `security role-mapping update` -Update an LDAP role mapping. +Update an LDAP role mapping. An update **replaces** the whole entity — every required +field below must be supplied, even if unchanged. ```sh -scadalink --url security role-mapping update --id [--ldap-group ] [--role ] +scadalink --url security role-mapping update --id --ldap-group --role ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Mapping ID | -| `--ldap-group` | no | Updated LDAP group distinguished name or CN | -| `--role` | no | Updated ScadaLink role | +| `--ldap-group` | yes | LDAP group distinguished name or CN | +| `--role` | yes | ScadaLink role | #### `security role-mapping delete` @@ -1099,17 +1108,20 @@ scadalink --url shared-script create --name --code #### `shared-script update` -Update a shared script's name or code. +Update a shared script. An update **replaces** the whole entity — every required field +below must be supplied, even if unchanged. ```sh -scadalink --url shared-script update --id [--name ] [--code ] +scadalink --url shared-script update --id --name --code [--parameters ] [--return-def ] ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Shared script ID | -| `--name` | no | Updated script name | -| `--code` | no | Updated script source code (or `@filepath`) | +| `--name` | yes | Shared script name | +| `--code` | yes | Script source code | +| `--parameters` | no | Parameter definitions JSON | +| `--return-def` | no | Return type definition JSON | #### `shared-script delete` @@ -1163,18 +1175,18 @@ scadalink --url db-connection create --name --connection-string < #### `db-connection update` -Update a database connection definition. +Update a database connection definition. An update **replaces** the whole entity — +every required field below must be supplied, even if unchanged. ```sh -scadalink --url db-connection update --id [--name ] [--connection-string ] [--provider ] +scadalink --url db-connection update --id --name --connection-string ``` | Option | Required | Description | |--------|----------|-------------| | `--id` | yes | Database connection ID | -| `--name` | no | Updated connection name | -| `--connection-string` | no | Updated connection string | -| `--provider` | no | Updated database provider | +| `--name` | yes | Connection name | +| `--connection-string` | yes | Database connection string | #### `db-connection delete` @@ -1228,18 +1240,20 @@ scadalink --url api-method create --name --code [--descr #### `api-method update` -Update an inbound API method. +Update an inbound API method. An update **replaces** the whole entity — every required +field below must be supplied, even if unchanged. ```sh -scadalink --url api-method update --id [--name ] [--code ] [--description ] +scadalink --url api-method update --id --script [--timeout ] [--parameters ] [--return-def ] ``` -| Option | Required | Description | -|--------|----------|-------------| -| `--id` | yes | API method ID | -| `--name` | no | Updated method name | -| `--code` | no | Updated script source code (or `@filepath`) | -| `--description` | no | Updated description | +| Option | Required | Default | Description | +|--------|----------|---------|-------------| +| `--id` | yes | — | API method ID | +| `--script` | yes | — | Script source code implementing the method | +| `--timeout` | no | `30` | Timeout in seconds | +| `--parameters` | no | — | Parameter definitions JSON | +| `--return-def` | no | — | Return type definition JSON | Script changes take effect immediately — the updated code is recompiled in-memory on the active central node. No restart is required. diff --git a/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs b/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs index 387ddc9..9c5992e 100644 --- a/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs +++ b/tests/ScadaLink.CLI.Tests/CommandTreeTests.cs @@ -68,6 +68,22 @@ public class CommandTreeTests Assert.True(leaf.Action != null, $"Leaf command '{leaf.Name}' has no action.")); } + [Fact] + public void TemplateCompositionDelete_IsKeyedByIdOnly() + { + // CLI-015: the in-repo README documented `template composition delete` with + // --template-id / --instance-name, but the implementation keys deletion by the + // composition's own integer ID via a single --id option. Pin the real surface. + var template = TemplateCommands.Build(Url, Format, Username, Password); + var composition = template.Subcommands.Single(c => c.Name == "composition"); + var delete = composition.Subcommands.Single(c => c.Name == "delete"); + + var optionNames = delete.Options.Select(o => o.Name).ToList(); + Assert.Contains("--id", optionNames); + Assert.DoesNotContain("--template-id", optionNames); + Assert.DoesNotContain("--instance-name", optionNames); + } + [Theory] [InlineData(typeof(GetInstanceCommand))] [InlineData(typeof(ListSitesCommand))] diff --git a/tests/ScadaLink.CLI.Tests/TableHeaderUnionTests.cs b/tests/ScadaLink.CLI.Tests/TableHeaderUnionTests.cs new file mode 100644 index 0000000..85408f3 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/TableHeaderUnionTests.cs @@ -0,0 +1,91 @@ +using ScadaLink.CLI; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-016 — WriteAsTable previously derived the table +/// header set from the first array element only, so any property unique to a later +/// element was silently dropped from the rendered table. +/// +[Collection("Console")] +public class TableHeaderUnionTests +{ + [Fact] + public void HandleResponse_TableFormat_HeterogeneousArray_IncludesAllColumns() + { + // The second element has a "Status" property the first lacks. The pre-fix code + // derived headers from items[0] only, so "Status" (and its value "Faulted") + // were dropped from the table entirely. + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var json = "[{\"Id\":1,\"Name\":\"Alpha\"},{\"Id\":2,\"Name\":\"Beta\",\"Status\":\"Faulted\"}]"; + var response = new ManagementResponse(200, json, null, null); + var exitCode = CommandHelpers.HandleResponse(response, "table"); + + Assert.Equal(0, exitCode); + var output = writer.ToString(); + Assert.Contains("Status", output); + Assert.Contains("Faulted", output); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } + + [Fact] + public void HandleResponse_TableFormat_HeterogeneousArray_PreservesFirstSeenColumnOrder() + { + // Column order must be the first-seen order across all elements: the first + // element contributes Id, Name; the second contributes Status after them. + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var json = "[{\"Id\":1,\"Name\":\"Alpha\"},{\"Status\":\"Faulted\",\"Id\":2,\"Name\":\"Beta\"}]"; + var response = new ManagementResponse(200, json, null, null); + var exitCode = CommandHelpers.HandleResponse(response, "table"); + + Assert.Equal(0, exitCode); + var output = writer.ToString(); + var headerLine = output.Split('\n')[0]; + Assert.True(headerLine.IndexOf("Id") < headerLine.IndexOf("Name")); + Assert.True(headerLine.IndexOf("Name") < headerLine.IndexOf("Status")); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } + + [Fact] + public void HandleResponse_TableFormat_FirstElementHasExtraColumn_StillRendersAllRows() + { + // The reverse case: the first element has a property a later element lacks. + // The later row must still render (with an empty cell), and all columns kept. + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var json = "[{\"Id\":1,\"Name\":\"Alpha\",\"Note\":\"first\"},{\"Id\":2,\"Name\":\"Beta\"}]"; + var response = new ManagementResponse(200, json, null, null); + var exitCode = CommandHelpers.HandleResponse(response, "table"); + + Assert.Equal(0, exitCode); + var output = writer.ToString(); + Assert.Contains("Note", output); + Assert.Contains("first", output); + Assert.Contains("Beta", output); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } +} diff --git a/tests/ScadaLink.CLI.Tests/UpdateCommandContractTests.cs b/tests/ScadaLink.CLI.Tests/UpdateCommandContractTests.cs new file mode 100644 index 0000000..6cf0b5c --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/UpdateCommandContractTests.cs @@ -0,0 +1,90 @@ +using System.CommandLine; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-014. The Update*Command records in Commons carry +/// non-nullable "core" fields (e.g. string Name, string Protocol, +/// string Script) — an update is a whole-entity replace, not a sparse +/// patch. The CLI must therefore mark those core flags as Required: making them +/// optional would let an omitted flag send null/empty and silently blank the +/// field server-side. These tests pin that contract so the documented surface and the +/// implemented surface stay aligned. +/// +public class UpdateCommandContractTests +{ + private static readonly Option Url = new("--url") { Recursive = true }; + private static readonly Option Username = new("--username") { Recursive = true }; + private static readonly Option Password = new("--password") { Recursive = true }; + private static readonly Option Format = CliOptions.CreateFormatOption(); + + private static Command UpdateCommand(Command group, params string[] path) + { + var current = group; + foreach (var segment in path) + current = current.Subcommands.Single(c => c.Name == segment); + return current; + } + + private static void AssertRequired(Command command, params string[] requiredOptionNames) + { + foreach (var name in requiredOptionNames) + { + var option = command.Options.SingleOrDefault(o => o.Name == name); + Assert.True(option != null, $"'{command.Name}' is missing expected option '{name}'."); + Assert.True(option!.Required, $"'{command.Name}' option '{name}' must be Required (whole-replace contract)."); + } + } + + [Fact] + public void TemplateUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(TemplateCommands.Build(Url, Format, Username, Password), "update"), "--name"); + + [Fact] + public void TemplateAttributeUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(TemplateCommands.Build(Url, Format, Username, Password), "attribute", "update"), "--name", "--data-type"); + + [Fact] + public void TemplateAlarmUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(TemplateCommands.Build(Url, Format, Username, Password), "alarm", "update"), "--name", "--trigger-type", "--priority"); + + [Fact] + public void TemplateScriptUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(TemplateCommands.Build(Url, Format, Username, Password), "script", "update"), "--name", "--code"); + + [Fact] + public void SiteUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(SiteCommands.Build(Url, Format, Username, Password), "update"), "--name"); + + [Fact] + public void DataConnectionUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(DataConnectionCommands.Build(Url, Format, Username, Password), "update"), "--name", "--protocol"); + + [Fact] + public void ExternalSystemUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(ExternalSystemCommands.Build(Url, Format, Username, Password), "update"), "--name", "--endpoint-url", "--auth-type"); + + [Fact] + public void NotificationUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(NotificationCommands.Build(Url, Format, Username, Password), "update"), "--name", "--emails"); + + [Fact] + public void ApiMethodUpdate_CoreFieldsRequired() + => AssertRequired(UpdateCommand(ApiMethodCommands.Build(Url, Format, Username, Password), "update"), "--script"); + + [Fact] + public void ExternalSystemMethodUpdate_IsGenuinelySparse_CoreFieldsOptional() + { + // UpdateExternalSystemMethodCommand is the one update record whose fields are + // genuinely all-nullable, so its flags are correctly optional. Pin that too so + // it is not mistakenly forced to Required. + var update = UpdateCommand(ExternalSystemCommands.Build(Url, Format, Username, Password), "method", "update"); + foreach (var name in new[] { "--name", "--http-method", "--path" }) + { + var option = update.Options.SingleOrDefault(o => o.Name == name); + Assert.True(option != null, $"method update is missing option '{name}'."); + Assert.False(option!.Required, $"method update option '{name}' should be optional (sparse-patch record)."); + } + } +}