Repository navigation
fix(#3542): an absent update policy fails closed, and an explicit one survives - #3607
Merged
Merged
Conversation
…erge
Implements the issue's proposal 3 ("an absent policy must read as None, not as
enabled") in two halves, because one alone does nothing:
* UpdatePolicyKind reordered so None is the ZERO value. The hub serializer sets
DefaultIgnoreCondition = WhenWritingDefault, so the zero member is omitted when
written; with Continuous at zero an explicit Continuous was DROPPED and became
indistinguishable from absent. Members travel by name, so stored "Continuous"
and "Stable" are unaffected.
* UpdatePolicyContent.Policy loses its `= Continuous` initialiser. This is the half
that actually decides what an ABSENT field reads as — System.Text.Json leaves an
absent field at the initialiser, so the reorder alone left the defect intact. My
first attempt shipped only the reorder and the new test caught it.
New test AbsentUpdatePolicyFailsClosedTest, falsified both ways.
🚨 NOT MERGEABLE AS-IS. Seven existing tests encode the OPPOSITE contract, most
explicitly VersionSelectTest.ParseContent_Null_DefaultsToContinuous. Landing this
means every install whose record lacks a `policy` field stops auto-updating. That is
a production rollout behaviour change and a maintainer decision, not a test-fixing
exercise. Committed so the analysis is not lost; awaiting that decision.
Full suite: 961 passed, 7 failed — all seven asserting absent/default = Continuous.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… survives
Proposal 3: "an absent policy must read as None, not as enabled".
🚨 The obvious repair is WRONG and I measured it: reordering UpdatePolicyKind so
None is the zero value does make an absent field read as None -- and simultaneously
makes an explicit None UNWRITABLE, because the serializer sets
DefaultIgnoreCondition = WhenWritingDefault and drops the zero member. The MCP patch
that turns auto-update OFF then fails with "the patch did not land within the
confirmation window", because the write is a no-op. That is the safety-critical
direction, so the repair broke something worse than it fixed.
The shape that satisfies both halves is a nullable backing field:
[JsonPropertyName("policy")] public UpdatePolicyKind? DeclaredPolicy
[JsonIgnore] public UpdatePolicyKind Policy
=> DeclaredPolicy ?? UpdatePolicyKind.None;
null is the default of a nullable, so EVERY named member round-trips and only genuine
absence is dropped. Consumers keep reading .Policy unchanged; the enum is untouched,
so no wire values move.
Six existing tests encoded the old contract. One asserted it head-on
(VersionSelectTest.ParseContent_Null_DefaultsToContinuous -> _FailsClosedToNone).
The other five were testing TAG derivation and merely leaned on the default
constructor; they now say which policy they assume rather than depending on a
default that changed underneath them.
Behaviour change, stated plainly: an install whose record carries no policy field
stops auto-updating until one is set. Approved by the maintainer.
Verified: Memex.Portal.Shared.Test 968/968; Release -warnaserror clean. The new
AbsentUpdatePolicyFailsClosedTest is falsified both ways.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new DeclaredPolicy/JsonPropertyName("policy") shape is currently incompatible with MeshNodeContentEditorControl’s field/key derivation, which would break editing the policy in the admin UI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR changes how the platform self-update policy is interpreted when the Admin/UpdatePolicy record cannot provide a policy value: instead of defaulting to the most permissive behavior, it now fails closed to UpdatePolicyKind.None (no auto-update), while still allowing explicitly chosen policies to round-trip correctly.
Changes:
- Update update-policy content modeling to distinguish “policy missing on the wire” from an explicitly chosen policy, and fail closed when absent.
- Adjust existing tests to be explicit about assumed policies and add a new regression test for the absent-field case.
- Add a What’s New entry describing the user-visible behavior change.
File summaries
| File | Description |
|---|---|
| test/Memex.Portal.Shared.Test/VersionSelectTest.cs | Updates the null-content default expectation to fail closed. |
| test/Memex.Portal.Shared.Test/PlatformUpdateStatusTest.cs | Makes tests explicit about policy assumptions (avoids relying on defaults). |
| test/Memex.Portal.Shared.Test/AbsentUpdatePolicyFailsClosedTest.cs | Adds regression coverage for “missing policy field” + round-trip of explicit Continuous. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-07-an-install-that-cannot-read-its-update-policy-no-longer-updates-itself.md | Documents the behavior change for users/operators. |
| memex/Memex.Portal.Shared/SelfUpdate/UpdatePolicyNodeType.cs | Implements the fail-closed policy interpretation via an optional “declared” backing value. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
44
to
+47
| [Description("Update strategy")] | ||
| [Translation("de", "Update-Strategie")] | ||
| public UpdatePolicyKind Policy { get; init; } = UpdatePolicyKind.Continuous; | ||
| [JsonPropertyName("policy")] | ||
| public UpdatePolicyKind? DeclaredPolicy { get; init; } |
Comment on lines
+64
to
+65
| [JsonIgnore] | ||
| public UpdatePolicyKind Policy |
Comment on lines
+29
to
+43
| /// <summary> | ||
| /// The update strategy. A NEWLY constructed record defaults to | ||
| /// <see cref="UpdatePolicyKind.Continuous"/> — but an ABSENT value on a persisted record reads as | ||
| /// <see cref="UpdatePolicyKind.None"/>, not as Continuous (#3542). | ||
| /// | ||
| /// <para>Those two are different questions and used to have the same answer. Because | ||
| /// <c>None</c> is now the enum's zero value, an explicit <c>Continuous</c> is non-default and is | ||
| /// therefore always written out, so "the admin chose Continuous" is distinguishable from "this | ||
| /// record lost its policy" — and the latter fails closed instead of enabling an unattended roll.</para> | ||
| /// </summary> | ||
| /// <summary> | ||
| /// The update strategy AS DECLARED on the record — <c>null</c> when the record carries no | ||
| /// <c>policy</c> field at all. Read <see cref="Policy"/> instead; this exists so that "absent" | ||
| /// and "explicitly chosen" stay different facts on the wire. | ||
| /// </summary> |
Comment on lines
+46
to
+48
| // The other half: because None is now the zero value, Continuous is non-default and must be | ||
| // WRITTEN OUT — otherwise an admin's explicit choice would decay into the absent case and be | ||
| // silently downgraded to None on the next read. |
Contributor
Contributor
Contributor
Contributor
This was referenced Sep 7, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An absent update policy fails closed — and an explicit one survives
#3542's proposal 3: "an absent
policymust read asNone, not as 'enabled'".An install records whether it auto-updates. If that field went missing — and it could, under the
install's own bookkeeping writes — it read back as Continuous: the most permissive state, reached
by losing information rather than by anyone choosing it. That is how memex-cloud rolled onto a
withdrawn
3.1.0-ciline "on a policy record that lost its own policy".🚨 The obvious repair is wrong, and I measured it before finding out the hard way
Reorder
UpdatePolicyKindsoNoneis the zero value. Absent then reads asNone— and an explicitNonebecomes unwritable, because the serializer setsDefaultIgnoreCondition = WhenWritingDefaultand drops whichever member is zero. The MCP patch that turns auto-update off then fails with
because the write is a no-op. That is the safety-critical direction: the repair broke something worse
than it fixed. Two tests caught it, and neither was the one I wrote.
The shape that satisfies both halves
nullis the default of a nullable, so every named member round-trips and only genuine absence isdropped. Consumers keep reading
.Policyunchanged, and the enum is untouched — no wire valuesmove, and records already storing
"Continuous"or"Stable"are unaffected.This is the same "model the third state" rule the rest of this handler family already follows: absent,
chosen-None and chosen-Continuous are three facts, and two of them used to share an answer.
The six existing tests
One asserted the old contract head-on —
VersionSelectTest.ParseContent_Null_DefaultsToContinuous,now
..._FailsClosedToNonewith the reason recorded inline. The other five were testing tagderivation and merely leaned on the default constructor; they now state which policy they assume
rather than depending on a default that changed underneath them. That is a better test either way.
🚨 Behaviour change, stated plainly
An install whose policy record carries no
policyfield stops auto-updating until one is set.Installs currently rolling on an absent field will stop. That is the point — but it is a live rollout
change, not a refactor. Approved by the maintainer before landing.
Verification
Memex.Portal.Shared.Test968/968.dotnet build -c Release -warnaserror, exit read directly — 0 Error(s).AbsentUpdatePolicyFailsClosedTest, falsified both ways: it is a pair on purpose —fail-closed alone would be satisfied by an enum nobody can express Continuous in, and round-tripping
alone would be satisfied by the old shape. Together they pin the distinction.
Not included
Proposal 3's other half — moving
lastCheckedAtoff the policy record so the bookkeeping writes stopclobbering the policy in the first place — is not here. This fixes what an absent value means;
it does not stop the value going absent. #3542 stays open for that.
Refs #3542
🤖 Generated with Claude Code