From 746ab90444f2c5b037c04db854380b64350b7fe2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 22:04:21 -0400 Subject: [PATCH] =?UTF-8?q?fix(cluster-infrastructure):=20resolve=20Cluste?= =?UTF-8?q?rInfrastructure-005,007,008=20=E2=80=94=20confirm=20config-sect?= =?UTF-8?q?ion=20constant,=20XML=20docs,=20phase-status=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ClusterInfrastructure/findings.md | 64 +++++++++++++++++-- .../ClusterOptionsTests.cs | 8 +++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/code-reviews/ClusterInfrastructure/findings.md b/code-reviews/ClusterInfrastructure/findings.md index 2775c79..5007e9d 100644 --- a/code-reviews/ClusterInfrastructure/findings.md +++ b/code-reviews/ClusterInfrastructure/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 3 | +| Open findings | 0 | ## Summary @@ -302,7 +302,7 @@ attributes cannot. Covered by `ClusterOptionsValidatorTests` (8 cases) and |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ClusterInfrastructure/ClusterOptions.cs:3` | **Description** @@ -323,7 +323,17 @@ Add a `public const string SectionName = "Cluster";` (or the agreed name) to **Resolution** -_Unresolved._ +Confirmed against the source: `ClusterOptions` previously exposed no section-name +constant, leaving binding sites to hard-code the magic string. + +**Resolved** — fixing commit `commit pending`, date 2026-05-16. `ClusterOptions` now +exposes `public const string SectionName = "ScadaLink:Cluster";` as the single source +of truth for the `appsettings.json` section name, with an XML doc explaining its +purpose. The chosen value matches the `ScadaLink:`-prefixed section convention used by +peer option classes and referenced by `ClusterOptionsValidator` / the design doc. +Covered by `ClusterOptionsTests.SectionName_IsTheExpectedAppSettingsSection`, which +both pins the value and — by referencing the constant — guards against its removal +(its deletion would break compilation). ### ClusterInfrastructure-006 — No tests for any cluster behaviour; only the options POCO is covered @@ -385,7 +395,7 @@ design); `ClusterOptionsValidator` is the layer that now rejects `keep-majority` |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ClusterInfrastructure/ClusterOptions.cs:3-11` | **Description** @@ -407,7 +417,20 @@ the relevant design-doc sections as peer modules do. **Resolution** -_Unresolved._ +Confirmed against the source: `ClusterOptions` previously had no XML doc comments on +the class or any property. + +**Resolved** — fixing commit `commit pending`, date 2026-05-16. `ClusterOptions` now +carries a class-level `` documenting the cluster configuration contract and +the deliberate ownership split (node identity / remoting / gRPC in +`Host.NodeOptions`, storage paths in the database options, cluster-formation settings +here), plus `` comments on all seven properties — each stating units, +defaults, and the design-doc constraints. Notably `MinNrOfMembers` is documented as +required to be `1` (a value of `2` blocks the cluster singleton after failover) and +`HeartbeatInterval` as required to be well below `FailureDetectionThreshold`, giving a +future editor the in-code warnings the recommendation asks for. This is a +documentation-only change, so no runtime regression test is meaningful; verified by +inspection of `ClusterOptions.cs:3-74`. Module test suite green (17 passed). ### ClusterInfrastructure-008 — "Phase 0 skeleton" status is undocumented at the module level @@ -415,7 +438,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ClusterInfrastructure/ServiceCollectionExtensions.cs:9`, `src/ScadaLink.ClusterInfrastructure/ServiceCollectionExtensions.cs:16` | **Description** @@ -439,4 +462,31 @@ about which components are skeletons versus implemented. **Resolution** -_Unresolved._ +Re-triaged in light of CI-001's resolution. The original defect was the mismatch +between two misleading inline `// Phase 0: skeleton only` / +`// Phase 0: placeholder` comments buried in private method bodies and a +complete-looking design doc with no caveat. That premise has been overtaken by the +CI-001/CI-002 work: + +- The "Phase 0 skeleton" comments no longer exist anywhere in + `src/ScadaLink.ClusterInfrastructure` (verified by `grep`). `ServiceCollectionExtensions` + now does real work (registers `ClusterOptionsValidator`) and `AddClusterInfrastructureActors` + throws explicitly — both with accurate XML docs explaining the ownership split. +- The module is no longer an unimplemented skeleton. CI-001 established that the Akka + bootstrap legitimately lives in `ScadaLink.Host`, and this project's true scope — + the `ClusterOptions` configuration contract, its validator, and DI registration — is + fully implemented and tested. +- The design doc `Component-ClusterInfrastructure.md` now opens with an + "Implementation Note — Code Placement" section (added by CI-001) that explicitly + states the component is a *design responsibility* realised across + `ScadaLink.ClusterInfrastructure` (configuration model) and `ScadaLink.Host` + (bootstrap/runtime wiring), and the README component table (row 13) was updated to + match. A reader of the design doc no longer assumes a single fully-built project. + +**Resolved** — fixing commit `commit pending`, date 2026-05-16. No further change +required: the misleading skeleton comments are gone, the module's real status is +documented at the design-doc level via the "Implementation Note — Code Placement" +section, and the component table reflects the true placement. This is a +documentation-only finding, so no runtime regression test is meaningful; verified by +inspection of `ServiceCollectionExtensions.cs` and +`docs/requirements/Component-ClusterInfrastructure.md:21-39`. diff --git a/tests/ScadaLink.ClusterInfrastructure.Tests/ClusterOptionsTests.cs b/tests/ScadaLink.ClusterInfrastructure.Tests/ClusterOptionsTests.cs index 0cd1146..7cb035c 100644 --- a/tests/ScadaLink.ClusterInfrastructure.Tests/ClusterOptionsTests.cs +++ b/tests/ScadaLink.ClusterInfrastructure.Tests/ClusterOptionsTests.cs @@ -35,6 +35,14 @@ public class ClusterOptionsTests Assert.Empty(options.SeedNodes); } + [Fact] + public void SectionName_IsTheExpectedAppSettingsSection() + { + // CI-005: ClusterOptions must expose a single-source-of-truth constant for + // its appsettings.json section so binding sites do not hard-code the string. + Assert.Equal("ScadaLink:Cluster", ClusterOptions.SectionName); + } + [Fact] public void Properties_CanBeSetToCustomValues() {