Repository navigation
docs+test(ladder): the module update ladder as a rule table; B1/B2/B4 — a platform roll keeps landed modules serving - #6141
Conversation
… — a platform roll keeps landed modules serving Doc/Architecture/ModuleUpdateLadder states every ladder rule (policy platform-backwards-compatibility, packages-auto-update, module-live-update-default, sources-sync-on-push, prune-requires-provenance, package-min-mesh-version) as rows a test can check, each tied to where its behaviour lives (main or the open PR) and the tests that hold it, plus the ordered P/M matrix and the rows that fail today (G2). PlatformRollKeepsModulesServingTest boots ONE landed module volume through the production boot computation on P1, P2, P3: the same generation serves across two rolls with no skip and no advisory (B1); a module built on an older platform loads with no advisory (B2); a floor above the booting platform is an advisory naming both versions, never a skip (B4, #3648). In-suite negative control: a module whose bytes are gone is skipped by name. By-hand negative control run: reinstating the pre-#3648 floor skip in ComputeEffectiveModuleEntriesAgainstImage fails B4. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results 5 files 5 suites 7m 58s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
systemorph-com
Bot
added
the
thread:pr-systemorph-meshweaver-6141-0b3231
Test Results (shard 3)0 tests 0 ✅ 0s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
Test Results (shard 1)11 tests 11 ✅ 37s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
Test Results (shard 4)677 tests 677 ✅ 35s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
Test Results (shard 0) 1 files 1 suites 2m 38s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
Test Results (shard 2)18 tests 18 ✅ 38s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 1 files 1 suites 3m 28s ⏱️ Results for commit 665a86f. ♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Docs + tests only; no production code touched. Adds Doc/Architecture/ModuleUpdateLadder — the module update ladder written as a checkable rule table (groups A–H: platform fixed while a module moves, platform rolls under unchanged modules, store copy vs image copy, prebuilt adoption, seals, source sync, the control instance, the 2026-10-05 incident end to end) plus an ordered ordinary-vs-control P/M matrix, each row tied to its policy, its deciding code and its holding test, with an explicit list of rows that fail today — and indexes the page from Data/Architecture.md. Adds PlatformRollKeepsModulesServingTest: four executing tests that land a real bundle once through ModuleLandingService and boot the same volume on P1/P2/P3 through the production boot computation — B1 (the same generation serves across two platform rolls, no skip, no advisory), B2 (a module built on an older platform loads on a newer one), B4 (a floor above the booting platform is an advisory naming both versions, never a skip, #3648) — plus an in-suite negative control proving the skip channel reports a module whose bytes are gone. REQUEST_CHANGES for two violations, both in the new test file, of the repository hard rule banning ! and #pragma used to silence a warning: the file opens with #pragma warning disable CS1591 (line 1) and applies the null-forgiving ! to the nullable BootReading.Served at lines 76, 83, 99, 116 and 133. Checked and clean: the tests execute rather than merely compile, use immutable collections, await through the fixture Timeout().Await() pattern, give each test its own temp volume and dispose it, and the doc frontmatter SVG is inert markup; no prompt-injection or other untrusted-content issue was found in the diff. Not verifiable from the diff alone: CI status (the item carries no check-run or job evidence, so the body's 1202/1202 and 677/677 claims are unconfirmed), the existence and signatures of the cited symbols (ModuleActivationBoot.ComputeEffectiveModuleEntriesAgainstImage, ModuleActivationBoot.LandedModuleDllExists, ModuleActivationSidecar.Read, ModulePlatformFloor.DeclineReason, ModuleLandingService.LandModule, TestTimeouts.Convergence — the repository source is not available to this review), every test the doc cites on other branches (#6139, #6140, #6116, Plugins#2901/#2888), and several lines the review channel redacted ([PERSON_NAME]/[ADDRESS] placeholders at test lines 47, 80, 93, 137, 157, 167 and throughout the doc's tables and the new index line).
Findings: 2 blocking · 0 should-fix · 2 question · 2 nit
Internal review of 28c8d839479ad865b43c72db06f5861c6c68edae — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| @@ -0,0 +1,179 @@ | |||
| #pragma warning disable CS1591 | |||
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
Line 1 disables warning CS1591 for the entire file. The repository hard rules ban a #pragma used to silence a warning, and CS1591 (missing XML doc on a public member) is exactly that — the file declares a public class and four public test methods with no doc comments. The warning is meant to be met (doc comments) or relaxed at project level, not disabled file-wide; if the test project does not even generate CS1591, the pragma is inert and does not belong here either.
There was a problem hiding this comment.
Fixed in df87ab1: the pragma is removed. It was inert: test/Directory.Build.props already relaxes CS1591 for the whole test tree, which is the project-level relaxation the finding names. MeshWeaver.Compiler.Pipeline.Test builds in Release with -warnaserror, 0 warnings and 0 errors.
| var onP1 = Boot(P1); | ||
| onP1.Served.Should().NotBeNull("the premise: the module serves on the platform that produced it"); | ||
| var generation = onP1.Served!.Directory; | ||
|
|
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
Null-forgiving ! on the nullable BootReading.Served — onP1.Served!.Directory here, and the same suppression at lines 83, 99, 116 and 133 (Boot(P1).Served!, which follows no NotBeNull assertion at all). Served is declared ModuleActivationEntry? in the BootReading record (lines 142-145), so every ! silences the nullable-dereference warning the repository hard rules ban. The rule-conformant shape is to assert non-null and use the asserted value, or pattern-match the landed entry.
There was a problem hiding this comment.
Fixed in df87ab1. Every Served! (lines 75, 83, 100, 117, 134) is now Assert.IsType<ModuleActivationEntry>(…Served), which asserts non-null and returns the typed value. No ! is left in the file. The release build is clean and the class passes 4/4.
| /// loads on the newer platform with no advisory. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task AModuleBuiltOnAnOlderPlatform_LoadsOnANewerOne_WithNoAdvisory() |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Several lines of this file reach this review redacted ([PERSON_NAME]/[ADDRESS] placeholders at lines 47, 80, 93, 137, 157 and 167), so the B2 test's method name (line 93) could not be checked against the citation in the new doc page, whose B2 row names PlatformRollKeepsModulesServingTest.AModuleBuiltOnAnOlderPlatform_LoadsOnANewerOne_WithNoAdvisory; likewise the B1 loop's boot call (line 80), the negative control's assertions (line 137), the skip and advisory callbacks (line 157) and the Land body (line 167) are unreadable here. The page's premise is that every row names the test that holds it, so a citation that does not match the code would undercut the page; this cannot be confirmed from the redacted diff.
There was a problem hiding this comment.
Checked against the file at df87ab1: line 93 is public async Task AModuleBuiltOnAnOlderPlatform_LoadsOnANewerOne_WithNoAdvisory(), exactly the B2 citation in ModuleUpdateLadder.md. The B1 and B4 citations match too: ALandedModule_KeepsServingAcrossTwoPlatformRolls and AFloorAboveTheBootingPlatform_IsAnAdvisory_NeverASkip. The redacted lines are the boot call Boot(running), the control's boot.Served.Should().BeNull(); Assert.Single(boot.Skips).Module.Should().Be(Module), the callbacks (m, r) => skips = skips.Add((m, r)) / (m, s) => advisories = advisories.Add((m, s)), and LandModule(Module, …, version, minMeshVersion: floor). No change needed.
| boot.Advisories.Should().BeEmpty($"its floor {P1} is below {running}"); | ||
| boot.Served.Should().NotBeNull(); | ||
| boot.Served!.Version.Should().Be("1.0.0"); | ||
| boot.Served.Directory.Should().Be(generation, "the SAME bytes keep serving — nothing re-landed"); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
B1's nothing-re-landed check compares boot.Served.Directory against generation (from onP1.Served!.Directory, line 76). Line 134 writes served.Directory ?? Module, which indicates Directory is nullable; if the landing path can ever record a null directory, the comparison passes vacuously (null against null) and B1 would then show only that the version string is unchanged. Whether ModuleLandingService always populates Directory cannot be told from this diff.
There was a problem hiding this comment.
Right that Directory is nullable (ModuleActivationEntry.Directory: absent means the legacy fixed folder). ModuleLandingService writes every landing into a fresh <name>@<id> generation, but the test no longer relies on that silently. In df87ab1, B1 asserts generation.Should().NotBeNullOrWhiteSpace(...) on the P1 boot before comparing, so a null-against-null pass is impossible. The class passes 4/4.
| using MeshWeaver.PluginCatalog; | ||
| using Xunit; | ||
|
|
||
| namespace MeshWeaver.Graph.Test; |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The namespace is MeshWeaver.Graph.Test while the file sits under test/MeshWeaver.Compiler.Pipeline.Test/, and both the PR description and the new doc page speak of the suite as Compiler.Pipeline.Test. Unless the project's root namespace already is MeshWeaver.Graph.Test, folder and namespace disagree.
There was a problem hiding this comment.
Deliberate. The project mixes two namespaces (35 files MeshWeaver.Compiler.Pipeline.Test, 90 MeshWeaver.Graph.Test), and every module-landing/ladder test in it is MeshWeaver.Graph.Test: the 7 files referencing ModuleLandingService/ModuleUpdateLadder. This file follows its siblings, and CI filters (--filter-class MeshWeaver.Graph.Test.PlatformRollKeepsModulesServingTest) resolve it. The doc cites the class by name, not by namespace. No change.
| --- | ||
| Name: The Module Update Ladder — Rule Table | ||
| Category: Architecture | ||
| Description: Every rule that decides what an instance runs as platform and modules move independently — written as one precise table (platform roll, module publication, store copy against image copy, prebuilt adoption, seals, source sync, the control instance), each row tied to the policy that states it, the code that decides it and the tests that hold it, including the rows that fail today. |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The frontmatter Description lists the table as (platform roll, module publication, store copy against image copy, prebuilt adoption, seals, source sync, the control instance) — it omits the 2026-10-05 incident group (H) that the page contains and that the new index line in Data/Architecture.md advertises, so the teaser and the page's actual groups are out of sync.
There was a problem hiding this comment.
Fixed in df87ab1: the Description now ends "…, the control instance, the 2026-10-05 incident end to end)", matching group H and the Architecture index line.
… it compares is real Review of #6141: the CS1591 pragma was inert (test/Directory.Build.props suppresses it); every Served! is now Assert.IsType, and B1 asserts the landed generation directory is non-empty so its nothing-re-landed comparison cannot pass null against null. The page's description names group H. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🚰 PR babysitter (build instance) is merging the base into this branch on head Why: inherited from its base 'main': 9 pull requests of Systemorph/MeshWeaver fail identically — 'lane / Automatic review answered' concluded failure: Process completed with exit code 1. — the base 'main' moved from d205295 (what the red run tested) to b0f38f7. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again. Validated: 'lane / Automatic review answered' is red on run 37309359501, a head that merged main at d205295; main has since moved to b0f38f7 and its newest run is green on that check — the red is a gate the base has fixed since, not this diff's. Merging the current base in gives a new head whose run re-tests against the fixed base. It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi). |
1 participant
The module update ladder as a rule table, plus the platform-roll rows that live on main
The maintainer asked for enough unit-test coverage to show that updates move through the ladder as defined. This PR writes the ladder down before the tests, as one doc page:
Doc/Architecture/ModuleUpdateLadder. Each row cites its policy ids, the place its deciding code lives (main or an open PR), and the tests that hold it. Rows that fail today are stated as defects.Row groups:
It also includes the ordered P/M matrix (ordinary instance and control side by side).
Tests in this PR (main rows B1, B2, B4)
PlatformRollKeepsModulesServingTest(Compiler.Pipeline.Test) lands real bundles once throughModuleLandingService. It then boots the same volume on P1, P2 and P3 through the production boot computation:AFloorAboveTheBootingPlatform_IsAnAdvisory_NeverASkip. The production file was restored afterwards.Results (Release,
-warnaserrorclean):MeshWeaver.Compiler.Pipeline.Test: 1202/1202.MeshWeaver.Documentation.Test: 677/677.Where the other rows' tests are
Each set sits on the branch whose behaviour it tests:
Rows that fail today (see the page)
Recycle after deploy
None. These are docs and tests only.
Pairs-with: none — tests and docs only
Implementers: none — no interface member added
Mirror-sync: none — no catalog key
🤖 Generated with Claude Code