feat(local-node-host): put authority-version rules behind a policy pipeline - #3616
Conversation
…peline One durable cutover engine; per-version readiness, admission and retirement behind a stable interface. The orchestrator keeps the stage machine and the atomic transition and stops knowing what a version means. The gap this closes was accepted but not closable in the previous change. Committing the authority marker retires every v1 cookie audience permanently, and the only preconditions were about DATA -- a root designation, an identity row, an account, a grant. Every one of them passes in exactly the state that would leave the installation unable to sign anyone in, because the only registered challenge issuer and tenant selector are still the v1 implementations. The orchestrator could not see that: it holds a DbContext and cannot observe composition-root registrations. A policy can, because a policy is itself registered and takes its collaborators by injection. V2 reports ready only when its data substrate exists AND at least one IInstallationAuthorityV2SignInPath is registered. Nothing implements that today, which is not an omission -- it is the accurate answer to whether this installation can serve v2 sign-in, and it makes the cutover fail closed until a successor is built. Two other version assumptions leave the stage machine. The retired audience set now comes from the successor policy rather than enumerating the audience enum wholesale, which silently meant every audience that has ever existed is retired by whatever comes next. And the retirement refusal code is the policy's word, applied at BOTH admission call sites -- previously the ternary existed at only one, so a refused v1 mutation after the flip reported that a finished migration was still in progress. Adding V3 means adding a policy. A version that changes the transition shape adds a stage and a policy together. No caller names a version. Verified, with each failing test named. Four mutations all bite: removing the sign-in-path requirement turns three red, hardcoding V2 readiness turns seven, deleting the orchestrator's readiness call turns six, and letting the V2 policy admit legacy bearers turns one. The registry refuses an unknown version and refuses two policies for one version, so rules cannot depend on registration order. The convenience constructor is proven fail-closed rather than assumed. Full node-host suite 1819 passed, 0 failed, 18 skipped. Every existing test that reaches V2Authoritative now declares a successor, which is the pattern doing its job rather than incidental churn. The MTW-00A inventory was ratcheted 54 to 55 for the new file, through its own regeneration path. Refs: shipyard#3615
github-actions
Bot
added
the
needs-deep-review
📝 WalkthroughWalkthroughChangesThe change adds V1/V2 installation-authority policies and a fixed registry. The cutover orchestrator now uses policy-driven readiness, admission, refusal, and bearer-retirement rules. Host wiring and identity tests register successor sign-in paths where required. Installation authority policy
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant LocalNodeHost
participant InstallationIdentityCutoverOrchestrator
participant InstallationAuthorityVersionRegistry
participant V2InstallationAuthorityVersionPolicy
participant InstallationIdentityStore
LocalNodeHost->>InstallationIdentityCutoverOrchestrator: construct with registry
InstallationIdentityCutoverOrchestrator->>InstallationAuthorityVersionRegistry: resolve V2 policy
InstallationIdentityCutoverOrchestrator->>V2InstallationAuthorityVersionPolicy: check readiness
V2InstallationAuthorityVersionPolicy->>InstallationIdentityStore: check readable V2 candidate
V2InstallationAuthorityVersionPolicy-->>InstallationIdentityCutoverOrchestrator: readiness result
InstallationIdentityCutoverOrchestrator->>InstallationIdentityStore: commit V2 authority marker
InstallationIdentityCutoverOrchestrator->>V2InstallationAuthorityVersionPolicy: evaluate legacy admission
V2InstallationAuthorityVersionPolicy-->>InstallationIdentityCutoverOrchestrator: retirement refusal
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs (1)
489-531: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftissue [blocking]: Preserve the root-readiness refusal.
WithSuccessor()registers a V2 path, then these tests disable the V2 root data.successor_not_readyidentifies the wrong failed precondition. This makes an unreadable V2 candidate indistinguishable from an absent successor. Return a root-readiness refusal for this path, and reservesuccessor_not_readyfor an absent or unusable successor.Also applies to: 594-608
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs` around lines 489 - 531, Update InstallationIdentityCutoverOrchestrator.AdvanceAsync so an existing V2 successor whose root data is unreadable returns the dedicated root-readiness refusal, while successor_not_ready remains reserved for absent or unusable successors. Adjust FinalFlipRefusesWhenVerifiedRootStopsBeingReadable and the corresponding test near the other affected range to assert the root-readiness refusal code.
🧹 Nitpick comments (8)
apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs (3)
158-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuethought:
VersionOfanswers V1 for stages it does not recognise.Every stage except
V2Authoritativemaps toInstallationAuthorityVersion.V1, and the V1 policy admits unconditionally. A stage value outside the defined set therefore selects the permissive policy.Nothing is exploitable today.
ResolveAuthorityAsyncreturnscutover_stage_unknownwithIsReadablefalse for an unknown stage (Line 793), and both admission call sites requireauthority.IsReadableinsidestageAdmitsbefore the policy answer can matter.RefusalForalso falls to migration-in-progress, which is safe.The residual concern is shape, not behavior: this mapping uses the same "default to the permissive answer" form that
IInstallationAuthorityVersionRegistry.Getdeliberately rejects for unknown versions. An explicit switch keeps the two consistent and survives a future stage addition that lands before the admission guards are revisited.🛡️ Proposed explicit mapping
private static InstallationAuthorityVersion VersionOf(InstallationIdentityCutoverStage stage) => - stage == InstallationIdentityCutoverStage.V2Authoritative - ? InstallationAuthorityVersion.V2 - : InstallationAuthorityVersion.V1; + stage switch + { + InstallationIdentityCutoverStage.V2Authoritative => InstallationAuthorityVersion.V2, + InstallationIdentityCutoverStage.LegacyV1Authoritative or + InstallationIdentityCutoverStage.WriteBarrierActive or + InstallationIdentityCutoverStage.Copying or + InstallationIdentityCutoverStage.Verified => InstallationAuthorityVersion.V1, + // A stage nobody wrote a mapping for must not resolve to the permissive incumbent. + _ => throw new InvalidOperationException($"No authority version is mapped for stage {stage}."), + };Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs` around lines 158 - 161, Update VersionOf to use an explicit switch over InstallationIdentityCutoverStage, mapping only the defined V1 and V2 stages to their corresponding InstallationAuthorityVersion values and handling unknown stages explicitly rather than defaulting to V1. Keep ResolveAuthorityAsync and the existing admission behavior unchanged.Source: Path instructions
517-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: the
stageAdmitsexpression is now duplicated verbatim.Lines 517-519 repeat Lines 485-487 exactly. Both encode the same durable rule: a readable marker, the LegacyV1Authoritative stage, and no write barrier. Two copies of one security predicate can drift, and a drift here would be silent in one of the two gates.
Extracting a private helper keeps the rule in one place.
♻️ Proposed helper
+ /// <summary> + /// The STAGE half of an admission: properties of the transition rather than of a version. Shared + /// by both gates so the two cannot drift apart. + /// </summary> + private static bool StageAdmits( + InstallationIdentityCutoverStateRecord state, + InstallationIdentityAuthorityResolution authority) => + authority.IsReadable && + state.Stage == InstallationIdentityCutoverStage.LegacyV1Authoritative && + state.V1WriteBarrierVersion == 0;- var stageAdmits = authority.IsReadable && - state.Stage == InstallationIdentityCutoverStage.LegacyV1Authoritative && - state.V1WriteBarrierVersion == 0; + var stageAdmits = StageAdmits(state, authority);Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs` around lines 517 - 523, Extract the duplicated stageAdmits predicate into a private helper in the installation cutover orchestrator, then replace both occurrences—including the one near the LegacyBearer admission check—with calls to that helper. Preserve the existing conditions: readable authority, LegacyV1Authoritative stage, and V1WriteBarrierVersion equal to zero.
803-808: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: key the retirement refusal on the version, not on the specific stage.
Both call sites resolve
policyfromVersionOf(state.Stage), then this method re-derives the same fact by comparingstate.StageagainstV2Authoritative. One fact, expressed twice.That reintroduces the failure mode the XML comment above describes. Add a stage in which a V3 policy is authoritative, and this ternary returns
MigrationInProgressRefusalfor it — a finished migration reported as still running, which is the exact defect this method centralised in order to fix.Asking the resolved version instead keeps the answer correct for every future version without another edit here.
♻️ Proposed version-keyed selection
private static string RefusalFor( - InstallationIdentityCutoverStateRecord state, + InstallationAuthorityVersion version, IInstallationAuthorityVersionPolicy policy) => - state.Stage == InstallationIdentityCutoverStage.V2Authoritative + version != InstallationAuthorityVersion.V1 ? policy.RetirementRefusalCode : MigrationInProgressRefusal;Both call sites already have the version in hand:
- var policy = _versions.Get(VersionOf(state.Stage)); + var version = VersionOf(state.Stage); + var policy = _versions.Get(version); var allowed = stageAdmits && await policy.IsAdmissionAllowedAsync( InstallationCutoverAdmissionKind.V1Mutation, null, cancellationToken).ConfigureAwait(false); return allowed ? new InstallationIdentityV1MutationAdmission(true, null) - : new InstallationIdentityV1MutationAdmission(false, RefusalFor(state, policy)); + : new InstallationIdentityV1MutationAdmission(false, RefusalFor(version, policy));Related:
V1InstallationAuthorityVersionPolicy.RetirementRefusalCodereturns the sameLegacyAuthorityRetiredRefusalconstant as the V2 policy. V1 is never the retiring authority, so that value is currently unreachable and cannot be distinguished from V2's. The version-keyed form above makes it unreachable by construction rather than by coincidence.Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs` around lines 803 - 808, The RefusalFor method currently determines the refusal code by checking if state.Stage equals V2Authoritative, but this brittle check will fail if a V3 stage becomes authoritative in the future (incorrectly returning MigrationInProgressRefusal for a finished migration). Instead of re-deriving the version from the stage, change the ternary condition in RefusalFor to key the retirement refusal directly on the version represented by the policy parameter that both call sites already have in hand. This makes the method future-proof by construction rather than accident.apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs (4)
89-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: type the admission subject instead of
object?.
IsAdmissionAllowedAsynccarries the kind-specific detail asobject?. The pairing betweenInstallationCutoverAdmissionKind.LegacyBearerandInstallationIdentityLegacyBearerAudienceis documented but not enforced. A future policy that castssubjectcan compile against a wrong kind/subject pair and refuse or admit for the wrong reason.Both current implementations ignore
subject, so nothing is broken today. A typed carrier keeps the contract self-enforcing before a third version arrives.♻️ Proposed typed admission request
+/// <summary>An admission request. The kind selects which members carry meaning.</summary> +/// <param name="Kind">What the caller asks to be admitted to do.</param> +/// <param name="Audience"> +/// The bearer audience, set only for <see cref="InstallationCutoverAdmissionKind.LegacyBearer"/>. +/// </param> +public readonly record struct InstallationCutoverAdmissionRequest( + InstallationCutoverAdmissionKind Kind, + InstallationIdentityLegacyBearerAudience? Audience = null);Task<bool> IsAdmissionAllowedAsync( - InstallationCutoverAdmissionKind kind, - object? subject, + InstallationCutoverAdmissionRequest request, CancellationToken cancellationToken);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs` around lines 89 - 92, Replace the object? subject parameter in IsAdmissionAllowedAsync with a typed admission-request carrier that pairs each InstallationCutoverAdmissionKind with its required subject type, including the LegacyBearer and InstallationIdentityLegacyBearerAudience association. Update the interface and both current implementations/call sites to use the carrier while preserving their existing behavior of ignoring the subject.
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffthought: the readiness call inverts the declared layer dependency.
V2InstallationAuthorityVersionPolicycallsInstallationIdentityCutoverOrchestrator.HasReadableV2CandidateAsync. The orchestrator also injectsIInstallationAuthorityVersionRegistryand consults this policy. That makes the two types mutually dependent, and it is the reasonHasReadableV2CandidateAsynchad to widen from private to internal.The v2 data-substrate predicate describes the v2 substrate, so it reads naturally as a member of the v2 policy or of a shared readiness helper that both types call. Moving it removes the cycle and lets the helper return to private.
The behavior is correct as written, and the paired readiness tests are convincing. This is a placement question, not a defect. Please treat it as non-blocking.
As per path instructions: "Review for architectural fit, avoid duplicated sources of truth."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs` around lines 186 - 188, Move the v2 substrate readiness predicate out of InstallationIdentityCutoverOrchestrator and into V2InstallationAuthorityVersionPolicy or a shared readiness helper used by both types, then update HasReadableV2CandidateAsync callers accordingly. Remove the mutual dependency and restore the helper’s visibility to private where possible, preserving the existing readiness behavior and tests.Source: Path instructions
207-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion: validate registry completeness at construction, not at first lookup.
The duplicate rejection is good, and the message names the offending version. One gap remains: a registry that omits a version constructs successfully and fails only when
Getis first called for it.That matters because
CanonicalLegacyBearerAudiencesinInstallationIdentityCutoverOrchestrator(Line 874) callsGet(InstallationAuthorityVersion.V2)unconditionally, and it runs insideResolveAuthorityAsync. A registry without V2 therefore throwsInvalidOperationExceptionout ofCheckLegacyBearerAdmissionAsyncinstead of returning a refusal code. The outcome is still fail-closed, but the failure arrives as an unhandled exception on a request path rather than as a diagnosable refusal.
CreateDefaultalways supplies both, so no shipped composition hits this. Failing at construction turns a miswired root into a startup error instead of a runtime surprise.🛡️ Proposed completeness check
_policies = byVersion; + + // Every declared version needs rules. A registry that is missing one fails at the first + // lookup, which can be deep inside an admission check; failing here makes it a startup error. + foreach (var version in Enum.GetValues<InstallationAuthorityVersion>()) + { + if (!byVersion.ContainsKey(version)) + { + throw new InvalidOperationException( + $"No authority-version policy is registered for {version}."); + } + } }As per path instructions: this PR "must preserve fail-closed authority behavior: unknown versions, missing policies ... must refuse rather than fall through."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs` around lines 207 - 225, The InstallationAuthorityVersionRegistry constructor validates duplicate policies but does not validate that all required versions are present in the registry. Add a completeness check after populating the byVersion dictionary to ensure each required installation authority version exists as a registered policy. If any required version is missing, throw an InvalidOperationException with a message naming the missing version, so composition errors are caught at construction time rather than at first request-time lookup of a missing version like in CanonicalLegacyBearerAudiences or other unconditional calls to Get.Source: Path instructions
127-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winquestion: should V1 declare the same retired audience set as V2?
RetiredAudiencesis documented at Line 98 as "The bearer audiences this version retires". V1 is the incumbent and retires nothing; retirement happens when V2 becomes authoritative. This V1 value is identical to the V2 value at Lines 164-165, and the orchestrator only reads the V2 one throughCanonicalLegacyBearerAudiences.That leaves two copies of the retired set, which is the exact shape the property was introduced to remove. If a V3 retires a subset, this V1 copy still claims every audience and no test reads it, so nothing catches the drift.
Either return an empty list for V1, with a comment stating that V1 retires nothing, or rename the member to describe "audiences retired when this version becomes authoritative" so both values are correct by definition.
As per path instructions: "Review for architectural fit, avoid duplicated sources of truth."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs` around lines 127 - 128, The V1 RetiredAudiences property is incorrectly returning all audiences from the enum, which duplicates the retired set already declared in V2 and contradicts the documented behavior that V1, as the incumbent version, retires nothing. Update the V1 RetiredAudiences property to return an empty list instead, and add a comment explaining that V1 retires nothing because retirement occurs only when a newer version becomes authoritative. This removes the duplicate source of truth and ensures the V1 value correctly reflects that the incumbent version has no retired audiences to declare.Source: Path instructions
apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs (1)
611-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Centralize the ready-successor test fixture.
These sites model the same ready V2 successor and default registry composition. Keep one shared test helper so interface or policy changes update every cutover proof together.
apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L611-L618: DelegateWithSuccessor()to the shared helper.apps/local-node-host/tests/Identity/Mtw00CRedFixtures/LegacyV1CutoverAuthorityProof.cs#L509-L521: ReplaceProofSignInPathandSuccessorRegistry()with the shared helper.apps/local-node-host/tests/Identity/WebAccountAccessChallengeIssuerTests.cs#L430-L434: Use the shared helper and remove the localTestSignInPath.apps/local-node-host/tests/Identity/WebTenantSelectionAuthorityTests.cs#L375-L379: Use the shared helper and remove the localTestSignInPath.apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L794-L798: MoveStubSignInPathandRegistryWith()into the shared helper.As per path instructions, avoid hand-parallel duplicate copies of single-source things.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs` around lines 611 - 618, The ready-successor registry fixture is duplicated across cutover tests; centralize it in a shared helper. In apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L611-L618, delegate WithSuccessor() to that helper; in apps/local-node-host/tests/Identity/Mtw00CRedFixtures/LegacyV1CutoverAuthorityProof.cs#L509-L521, replace ProofSignInPath and SuccessorRegistry(); in apps/local-node-host/tests/Identity/WebAccountAccessChallengeIssuerTests.cs#L430-L434 and apps/local-node-host/tests/Identity/WebTenantSelectionAuthorityTests.cs#L375-L379, use the helper and remove each local TestSignInPath; and in apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L794-L798, move StubSignInPath and RegistryWith() into the shared helper.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs`:
- Around line 119-142:
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:119-142:
Add XML documentation for V1InstallationAuthorityVersionPolicy, Version,
RetirementRefusalCode, and IsAdmissionAllowedAsync, stating that V1 admits every
kind and orchestrator stage rules refuse during a barrier.
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:148-165:
Document the constructor and signInPaths parameter, noting the collection is
snapshotted, and document Version, RetirementRefusalCode, and RetiredAudiences.
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:203-231:
Document the constructor and policies parameter, including duplicate-version
InvalidOperationException behavior, and add <inheritdoc/> to Get.
apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs:141-149:
Document the three-argument constructor and versions parameter, noting callers
supplying a custom registry must provide the sign-in paths omitted by the
two-argument overload.
In
`@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs`:
- Around line 865-874: Update the XML documentation for
CanonicalLegacyBearerAudiences to explicitly state that the canonical audience
set is currently pinned to InstallationAuthorityVersion.V2 and does not yet
derive from the transition successor; document the resulting V3 limitation
rather than claiming successor-based protection. Keep the implementation
unchanged and avoid introducing a second source of truth.
- Around line 704-712: The readiness check at the IsReadyAsync call on
_versions.Get(VersionOf(targetStage)) currently returns only a boolean,
collapsing three distinct failure reasons (missing sign-in path, unverified
root, or missing data row) into a single "successor_not_ready" code. Modify the
IsReadyAsync result to include a reason string that describes why readiness
failed, then update the return statement here to use that reason string instead
of the hardcoded "successor_not_ready" code. This preserves the separation of
concerns where the policy states its reason and the stage orchestrator passes it
through unchanged, while allowing operators to distinguish between "build a v2
sign-in path" and "repair the account row" without requiring the stage machine
to interpret the causes.
In
`@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs`:
- Around line 866-893: Update
RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy to register a custom
V2InstallationAuthorityVersionPolicy with a distinct refusal code and a
non-default retired-audience set. Assert legacy bearer admission is refused only
for the configured retirement audiences and returns the custom code, while
non-retired audiences preserve their expected admission behavior; also assert
CheckV1MutationAdmissionAsync returns the same policy-provided refusal code
after the flip.
- Around line 814-974: Add XML documentation with a concise <summary> to each
newly added public test method:
FlipIsRefusedWhenNoSuccessorSignInPathIsRegistered,
FlipSucceedsOnceASuccessorSignInPathIsRegistered,
RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy,
RegistryRefusesAnUnknownVersionRatherThanDefaultingPermissively,
RegistryRefusesTwoPoliciesForOneVersion, DefaultCompositionIsFailClosed, and
V1PolicyIsReadyAndAdmits_V2PolicyIsNotReadyWithoutASignInPath. Place each
summary immediately above its corresponding [Fact] attribute and accurately
describe the behavior being tested.
---
Outside diff comments:
In
`@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs`:
- Around line 489-531: Update
InstallationIdentityCutoverOrchestrator.AdvanceAsync so an existing V2 successor
whose root data is unreadable returns the dedicated root-readiness refusal,
while successor_not_ready remains reserved for absent or unusable successors.
Adjust FinalFlipRefusesWhenVerifiedRootStopsBeingReadable and the corresponding
test near the other affected range to assert the root-readiness refusal code.
---
Nitpick comments:
In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs`:
- Around line 89-92: Replace the object? subject parameter in
IsAdmissionAllowedAsync with a typed admission-request carrier that pairs each
InstallationCutoverAdmissionKind with its required subject type, including the
LegacyBearer and InstallationIdentityLegacyBearerAudience association. Update
the interface and both current implementations/call sites to use the carrier
while preserving their existing behavior of ignoring the subject.
- Around line 186-188: Move the v2 substrate readiness predicate out of
InstallationIdentityCutoverOrchestrator and into
V2InstallationAuthorityVersionPolicy or a shared readiness helper used by both
types, then update HasReadableV2CandidateAsync callers accordingly. Remove the
mutual dependency and restore the helper’s visibility to private where possible,
preserving the existing readiness behavior and tests.
- Around line 207-225: The InstallationAuthorityVersionRegistry constructor
validates duplicate policies but does not validate that all required versions
are present in the registry. Add a completeness check after populating the
byVersion dictionary to ensure each required installation authority version
exists as a registered policy. If any required version is missing, throw an
InvalidOperationException with a message naming the missing version, so
composition errors are caught at construction time rather than at first
request-time lookup of a missing version like in CanonicalLegacyBearerAudiences
or other unconditional calls to Get.
- Around line 127-128: The V1 RetiredAudiences property is incorrectly returning
all audiences from the enum, which duplicates the retired set already declared
in V2 and contradicts the documented behavior that V1, as the incumbent version,
retires nothing. Update the V1 RetiredAudiences property to return an empty list
instead, and add a comment explaining that V1 retires nothing because retirement
occurs only when a newer version becomes authoritative. This removes the
duplicate source of truth and ensures the V1 value correctly reflects that the
incumbent version has no retired audiences to declare.
In
`@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs`:
- Around line 158-161: Update VersionOf to use an explicit switch over
InstallationIdentityCutoverStage, mapping only the defined V1 and V2 stages to
their corresponding InstallationAuthorityVersion values and handling unknown
stages explicitly rather than defaulting to V1. Keep ResolveAuthorityAsync and
the existing admission behavior unchanged.
- Around line 517-523: Extract the duplicated stageAdmits predicate into a
private helper in the installation cutover orchestrator, then replace both
occurrences—including the one near the LegacyBearer admission check—with calls
to that helper. Preserve the existing conditions: readable authority,
LegacyV1Authoritative stage, and V1WriteBarrierVersion equal to zero.
- Around line 803-808: The RefusalFor method currently determines the refusal
code by checking if state.Stage equals V2Authoritative, but this brittle check
will fail if a V3 stage becomes authoritative in the future (incorrectly
returning MigrationInProgressRefusal for a finished migration). Instead of
re-deriving the version from the stage, change the ternary condition in
RefusalFor to key the retirement refusal directly on the version represented by
the policy parameter that both call sites already have in hand. This makes the
method future-proof by construction rather than accident.
In
`@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs`:
- Around line 611-618: The ready-successor registry fixture is duplicated across
cutover tests; centralize it in a shared helper. In
apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L611-L618,
delegate WithSuccessor() to that helper; in
apps/local-node-host/tests/Identity/Mtw00CRedFixtures/LegacyV1CutoverAuthorityProof.cs#L509-L521,
replace ProofSignInPath and SuccessorRegistry(); in
apps/local-node-host/tests/Identity/WebAccountAccessChallengeIssuerTests.cs#L430-L434
and
apps/local-node-host/tests/Identity/WebTenantSelectionAuthorityTests.cs#L375-L379,
use the helper and remove each local TestSignInPath; and in
apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs#L794-L798,
move StubSignInPath and RegistryWith() into the shared helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a5b28c8b-8d5f-4317-8fd8-3407f5ee7f47
⛔ Files ignored due to path filters (1)
apps/local-node-host/tests/Identity/Mtw00AInventory/mtw-00a-identity-inventory.generated.mdis excluded by!**/*.generated.*
📒 Files selected for processing (7)
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.csapps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.csapps/local-node-host/Program.csapps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.csapps/local-node-host/tests/Identity/Mtw00CRedFixtures/LegacyV1CutoverAuthorityProof.csapps/local-node-host/tests/Identity/WebAccountAccessChallengeIssuerTests.csapps/local-node-host/tests/Identity/WebTenantSelectionAuthorityTests.cs
| public sealed class V1InstallationAuthorityVersionPolicy : IInstallationAuthorityVersionPolicy | ||
| { | ||
| public InstallationAuthorityVersion Version => InstallationAuthorityVersion.V1; | ||
|
|
||
| public string RetirementRefusalCode => | ||
| InstallationIdentityCutoverOrchestrator.LegacyAuthorityRetiredRefusal; | ||
|
|
||
| /// <summary>Every current audience is v1's, and all of them retire together at the flip.</summary> | ||
| public IReadOnlyList<InstallationIdentityLegacyBearerAudience> RetiredAudiences { get; } = | ||
| Enum.GetValues<InstallationIdentityLegacyBearerAudience>().Order().ToArray(); | ||
|
|
||
| /// <summary> | ||
| /// Always ready. You never advance TO v1 — it is where the installation starts, and a rollback | ||
| /// before the marker leaves it authoritative rather than transitioning into it. | ||
| /// </summary> | ||
| public Task<bool> IsReadyAsync( | ||
| NodeLocalInstallationIdentityDbContext context, | ||
| CancellationToken cancellationToken) => Task.FromResult(true); | ||
|
|
||
| public Task<bool> IsAdmissionAllowedAsync( | ||
| InstallationCutoverAdmissionKind kind, | ||
| object? subject, | ||
| CancellationToken cancellationToken) => Task.FromResult(true); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue (non-blocking): new public members are missing XML documentation.
The interfaces in this PR are documented well, and several implementation members carry useful remarks. The remaining public members have none. The review policy requires XML documentation on new or changed public C# members, so this is a uniform gap rather than four separate omissions.
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L119-L142: document the class and the undocumented membersVersion(Line 121),RetirementRefusalCode(Lines 123-124), andIsAdmissionAllowedAsync(Lines 138-141). State onIsAdmissionAllowedAsyncthat V1 admits every kind and that the stage rules in the orchestrator are what refuse during a barrier.apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L148-L165: document the constructor and itssignInPathsparameter, plusVersion,RetirementRefusalCode, andRetiredAudiences. Note on the constructor that the collection is snapshotted, so later registrations do not change the answer.apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L203-L231: document the constructor and itspoliciesparameter, including theInvalidOperationExceptionthrown for a duplicate version, and add<inheritdoc/>onGet.apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs#L141-L149: document the three-argument constructor and itsversionsparameter, and state that a caller supplying its own registry takes responsibility for the sign-in paths that the two-argument overload deliberately omits.
<inheritdoc/> covers most of these in one line each, since the interface documentation is already thorough.
As per path instructions: "New or changed public C# members require XML documentation."
📍 Affects 2 files
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L119-L142(this comment)apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L148-L165apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs#L203-L231apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs#L141-L149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs`
around lines 119 - 142,
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:119-142:
Add XML documentation for V1InstallationAuthorityVersionPolicy, Version,
RetirementRefusalCode, and IsAdmissionAllowedAsync, stating that V1 admits every
kind and orchestrator stage rules refuse during a barrier.
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:148-165:
Document the constructor and signInPaths parameter, noting the collection is
snapshotted, and document Version, RetirementRefusalCode, and RetiredAudiences.
apps/local-node-host/Data/Identity/InstallationAuthorityVersionPolicy.cs:203-231:
Document the constructor and policies parameter, including duplicate-version
InvalidOperationException behavior, and add <inheritdoc/> to Get.
apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs:141-149:
Document the three-argument constructor and versions parameter, noting callers
supplying a custom registry must provide the sign-in paths omitted by the
two-argument overload.
Source: Path instructions
| /// <summary> | ||
| /// The audience set the SUCCESSOR retires, asked of the successor rather than assumed. | ||
| /// | ||
| /// This used to enumerate the audience enum wholesale, which is a v1-shaped assumption baked | ||
| /// into the stage machine: it silently means "every audience that has ever existed is retired by | ||
| /// whatever comes next". A v3 retiring a different subset would have been wrong here with | ||
| /// nothing to catch it, because the digest would still have computed cleanly over the wrong set. | ||
| /// </summary> | ||
| private InstallationIdentityLegacyBearerAudience[] CanonicalLegacyBearerAudiences() => | ||
| _versions.Get(InstallationAuthorityVersion.V2).RetiredAudiences.ToArray(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
issue (non-blocking): this asks V2 by name, not "the successor", so the assumption moved rather than went away.
The XML comment states the previous code "silently means 'every audience that has ever existed is retired by whatever comes next'", and that "a v3 retiring a different subset would have been wrong here with nothing to catch it, because the digest would still have computed cleanly over the wrong set".
That failure mode survives this change. CanonicalLegacyBearerAudiences hardcodes InstallationAuthorityVersion.V2. Add a V3 that becomes the successor and retires a subset, and this method still returns V2's set, and the digest still computes cleanly over the wrong set. The old assumption was v1-shaped; the new one is v2-shaped.
The consequence is not theoretical, because the digest is durable. state.LegacyBearerRevocationDigest is staged once and recompared later by HasExactLegacyBearerRevocationEvidence, and that result feeds readable in ResolveAuthorityAsync at Line 784. If the canonical set ever changes identity between staging and verification, a committed authority marker becomes unreadable. The direction is fail-closed, so no admission is wrongly granted, but the file's own remarks note that rollback after the marker is capability disable rather than a return to v1.
Nothing is wrong today: no V3 exists and V2 retires every audience. The actionable part is that the comment claims a protection the code does not provide, so the next author will trust it. Two options:
- Correct the comment to say the successor version is currently pinned to V2, and record the V3 gap.
- Derive the successor version from the transition instead of naming it, for example by passing the target version into the digest computation so the canonical set always belongs to the version being cut over to.
📝 Minimum change: make the pinning explicit
/// <summary>
- /// The audience set the SUCCESSOR retires, asked of the successor rather than assumed.
- ///
- /// This used to enumerate the audience enum wholesale, which is a v1-shaped assumption baked
- /// into the stage machine: it silently means "every audience that has ever existed is retired by
- /// whatever comes next". A v3 retiring a different subset would have been wrong here with
- /// nothing to catch it, because the digest would still have computed cleanly over the wrong set.
+ /// The audience set the successor retires, asked of a policy rather than read off the enum.
+ ///
+ /// This used to enumerate the audience enum wholesale, which put the retired set in the stage
+ /// machine instead of in a policy. Asking a policy moves the answer to where the rules live.
+ ///
+ /// <b>The successor is still pinned to V2 here.</b> A V3 that retires a different subset would
+ /// compute this digest over V2's set, and the mismatch would surface as an unreadable authority
+ /// marker rather than a diagnosable refusal. Adding V3 must also make this version follow the
+ /// transition target.
/// </summary>As per path instructions: "Review for architectural fit, avoid duplicated sources of truth."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// The audience set the SUCCESSOR retires, asked of the successor rather than assumed. | |
| /// | |
| /// This used to enumerate the audience enum wholesale, which is a v1-shaped assumption baked | |
| /// into the stage machine: it silently means "every audience that has ever existed is retired by | |
| /// whatever comes next". A v3 retiring a different subset would have been wrong here with | |
| /// nothing to catch it, because the digest would still have computed cleanly over the wrong set. | |
| /// </summary> | |
| private InstallationIdentityLegacyBearerAudience[] CanonicalLegacyBearerAudiences() => | |
| _versions.Get(InstallationAuthorityVersion.V2).RetiredAudiences.ToArray(); | |
| /// <summary> | |
| /// The audience set the successor retires, asked of a policy rather than read off the enum. | |
| /// | |
| /// This used to enumerate the audience enum wholesale, which put the retired set in the stage | |
| /// machine instead of in a policy. Asking a policy moves the answer to where the rules live. | |
| /// | |
| /// <b>The successor is still pinned to V2 here.</b> A V3 that retires a different subset would | |
| /// compute this digest over V2's set, and the mismatch would surface as an unreadable authority | |
| /// marker rather than a diagnosable refusal. Adding V3 must also make this version follow the | |
| /// transition target. | |
| /// </summary> | |
| private InstallationIdentityLegacyBearerAudience[] CanonicalLegacyBearerAudiences() => | |
| _versions.Get(InstallationAuthorityVersion.V2).RetiredAudiences.ToArray(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/local-node-host/Data/Identity/InstallationIdentityCutoverOrchestrator.cs`
around lines 865 - 874, Update the XML documentation for
CanonicalLegacyBearerAudiences to explicitly state that the canonical audience
set is currently pinned to InstallationAuthorityVersion.V2 and does not yet
derive from the transition successor; document the resulting V3 limitation
rather than claiming successor-based protection. Keep the implementation
unchanged and avoid introducing a second source of truth.
Source: Path instructions
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public async Task FlipIsRefusedWhenNoSuccessorSignInPathIsRegistered() | ||
| { | ||
| // Everything the migration itself can produce is in place: the data substrate is ready and | ||
| // the legacy bearer revocations are staged. The ONLY thing missing is somewhere for a human | ||
| // to sign in afterwards, which is the state this installation is in today. | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| var time = new MutableTimeProvider(TestIdentityDatabase.StartedAtUtc); | ||
| var service = new InstallationIdentityCutoverOrchestrator( | ||
| database.Factory, time, RegistryWith()); | ||
| var lease = await DriveToVerifiedAsync(service); | ||
|
|
||
| var refused = await service.AdvanceAsync( | ||
| lease, InstallationIdentityCutoverStage.V2Authoritative, Evidence()); | ||
|
|
||
| Assert.Equal(InstallationIdentityCutoverAdvanceStatus.InvariantRefused, refused.Status); | ||
| Assert.Equal("successor_not_ready", refused.RefusalCode); | ||
|
|
||
| // And the refusal actually protected something: v1 is still authoritative and still serving. | ||
| var authority = await service.ResolveAuthorityAsync(); | ||
| Assert.True(authority.IsReadable); | ||
| Assert.Equal(InstallationIdentityAuthorityKind.LegacyV1, authority.Authority); | ||
| var admission = await service.CheckLegacyBearerAdmissionAsync( | ||
| InstallationIdentityLegacyBearerAudience.AccountChallenge); | ||
| Assert.False(admission.IsAllowed); | ||
| Assert.Equal( | ||
| InstallationIdentityCutoverOrchestrator.MigrationInProgressRefusal, | ||
| admission.RefusalCode); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public async Task FlipSucceedsOnceASuccessorSignInPathIsRegistered() | ||
| { | ||
| // The same database and the same evidence as the refusal above. The ONLY difference is a | ||
| // registered successor, which is what makes this pair a control rather than two assertions. | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| var time = new MutableTimeProvider(TestIdentityDatabase.StartedAtUtc); | ||
| var service = new InstallationIdentityCutoverOrchestrator( | ||
| database.Factory, time, RegistryWith(new StubSignInPath("v2-challenge-issuer"))); | ||
| var lease = await DriveToVerifiedAsync(service); | ||
|
|
||
| var advanced = await service.AdvanceAsync( | ||
| lease, InstallationIdentityCutoverStage.V2Authoritative, Evidence()); | ||
|
|
||
| Assert.Equal(InstallationIdentityCutoverAdvanceStatus.Advanced, advanced.Status); | ||
| Assert.Equal(InstallationIdentityCutoverStage.V2Authoritative, advanced.Stage); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public async Task RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy() | ||
| { | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| var time = new MutableTimeProvider(TestIdentityDatabase.StartedAtUtc); | ||
| var service = new InstallationIdentityCutoverOrchestrator( | ||
| database.Factory, time, RegistryWith(new StubSignInPath("v2-challenge-issuer"))); | ||
| var lease = await DriveToVerifiedAsync(service); | ||
| await service.AdvanceAsync(lease, InstallationIdentityCutoverStage.V2Authoritative, Evidence()); | ||
|
|
||
| // Every audience is refused, and with the code the SUCCESSOR names rather than one the stage | ||
| // machine hardcodes. A v3 retiring a different set changes this answer by adding a policy. | ||
| foreach (var audience in Enum.GetValues<InstallationIdentityLegacyBearerAudience>()) | ||
| { | ||
| var admission = await service.CheckLegacyBearerAdmissionAsync(audience); | ||
| Assert.False(admission.IsAllowed); | ||
| Assert.Equal( | ||
| InstallationIdentityCutoverOrchestrator.LegacyAuthorityRetiredRefusal, | ||
| admission.RefusalCode); | ||
| } | ||
|
|
||
| // The v1 mutation path reports retirement too. It previously reported "migration in | ||
| // progress" after the flip, because the retirement ternary existed at only one of the two | ||
| // call sites -- the migration had finished and the authority was gone. | ||
| var mutation = await service.CheckV1MutationAdmissionAsync(); | ||
| Assert.False(mutation.IsAllowed); | ||
| Assert.Equal( | ||
| InstallationIdentityCutoverOrchestrator.LegacyAuthorityRetiredRefusal, | ||
| mutation.RefusalCode); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public void RegistryRefusesAnUnknownVersionRatherThanDefaultingPermissively() | ||
| { | ||
| var registry = new InstallationAuthorityVersionRegistry( | ||
| [new V1InstallationAuthorityVersionPolicy()]); | ||
|
|
||
| Assert.Equal( | ||
| InstallationAuthorityVersion.V1, | ||
| registry.Get(InstallationAuthorityVersion.V1).Version); | ||
| // A missing policy must throw. Answering "allowed" for a version nobody wrote rules for is | ||
| // how a version check silently stops being a check. | ||
| Assert.Throws<InvalidOperationException>( | ||
| () => registry.Get(InstallationAuthorityVersion.V2)); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public void RegistryRefusesTwoPoliciesForOneVersion() | ||
| { | ||
| // Otherwise the effective rule set depends on DI registration order. | ||
| Assert.Throws<InvalidOperationException>(() => new InstallationAuthorityVersionRegistry( | ||
| [new V1InstallationAuthorityVersionPolicy(), new V1InstallationAuthorityVersionPolicy()])); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public async Task DefaultCompositionIsFailClosed() | ||
| { | ||
| // The two-argument orchestrator constructor exists so 30-odd call sites keep compiling. It | ||
| // must not be a back door: its default registry gives V2 no sign-in paths, so a caller that | ||
| // forgets to supply one gets a refused cutover rather than a permitted one. | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| var time = new MutableTimeProvider(TestIdentityDatabase.StartedAtUtc); | ||
| var service = new InstallationIdentityCutoverOrchestrator(database.Factory, time); | ||
| var lease = await DriveToVerifiedAsync(service); | ||
|
|
||
| var refused = await service.AdvanceAsync( | ||
| lease, InstallationIdentityCutoverStage.V2Authoritative, Evidence()); | ||
|
|
||
| Assert.Equal(InstallationIdentityCutoverAdvanceStatus.InvariantRefused, refused.Status); | ||
| Assert.Equal("successor_not_ready", refused.RefusalCode); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("PlanCard", "MTW-3615")] | ||
| public async Task V1PolicyIsReadyAndAdmits_V2PolicyIsNotReadyWithoutASignInPath() | ||
| { | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| await using var context = database.Factory.CreateDbContext(); | ||
|
|
||
| var v1 = new V1InstallationAuthorityVersionPolicy(); | ||
| Assert.True(await v1.IsReadyAsync(context, CancellationToken.None)); | ||
| Assert.True(await v1.IsAdmissionAllowedAsync( | ||
| InstallationCutoverAdmissionKind.LegacyBearer, | ||
| InstallationIdentityLegacyBearerAudience.AccountChallenge, | ||
| CancellationToken.None)); | ||
|
|
||
| var v2WithNoPath = new V2InstallationAuthorityVersionPolicy([]); | ||
| Assert.False(await v2WithNoPath.IsReadyAsync(context, CancellationToken.None)); | ||
| Assert.False(await v2WithNoPath.IsAdmissionAllowedAsync( | ||
| InstallationCutoverAdmissionKind.LegacyBearer, | ||
| InstallationIdentityLegacyBearerAudience.AccountChallenge, | ||
| CancellationToken.None)); | ||
|
|
||
| // BOTH halves are required, proved in both directions on the same policy instance. The | ||
| // prepared fixture already carries a readable v2 candidate, so with a registered path the | ||
| // policy reports ready... | ||
| var v2WithPath = new V2InstallationAuthorityVersionPolicy( | ||
| [new StubSignInPath("v2-challenge-issuer")]); | ||
| Assert.True(await v2WithPath.IsReadyAsync(context, CancellationToken.None)); | ||
|
|
||
| // ...and removing the DATA half alone takes it back to not-ready, so the sign-in-path check | ||
| // cannot be mistaken for the only thing this predicate looks at. | ||
| var account = await context.Accounts.SingleAsync(); | ||
| account.Status = InstallationAccountStatus.Disabled; | ||
| await context.SaveChangesAsync(); | ||
| Assert.False(await v2WithPath.IsReadyAsync(context, CancellationToken.None)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue [blocking]: Add XML documentation to the added public test methods.
The public test methods at Lines 816, 847, 866, 898, 914, 923, and 942 lack XML documentation. Add a /// <summary> to each method.
As per path instructions, new or changed public C# members require XML documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs`
around lines 814 - 974, Add XML documentation with a concise <summary> to each
newly added public test method:
FlipIsRefusedWhenNoSuccessorSignInPathIsRegistered,
FlipSucceedsOnceASuccessorSignInPathIsRegistered,
RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy,
RegistryRefusesAnUnknownVersionRatherThanDefaultingPermissively,
RegistryRefusesTwoPoliciesForOneVersion, DefaultCompositionIsFailClosed, and
V1PolicyIsReadyAndAdmits_V2PolicyIsNotReadyWithoutASignInPath. Place each
summary immediately above its corresponding [Fact] attribute and accurately
describe the behavior being tested.
Source: Path instructions
| public async Task RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy() | ||
| { | ||
| await using var database = await TestIdentityDatabase.CreatePreparedAsync(); | ||
| var time = new MutableTimeProvider(TestIdentityDatabase.StartedAtUtc); | ||
| var service = new InstallationIdentityCutoverOrchestrator( | ||
| database.Factory, time, RegistryWith(new StubSignInPath("v2-challenge-issuer"))); | ||
| var lease = await DriveToVerifiedAsync(service); | ||
| await service.AdvanceAsync(lease, InstallationIdentityCutoverStage.V2Authoritative, Evidence()); | ||
|
|
||
| // Every audience is refused, and with the code the SUCCESSOR names rather than one the stage | ||
| // machine hardcodes. A v3 retiring a different set changes this answer by adding a policy. | ||
| foreach (var audience in Enum.GetValues<InstallationIdentityLegacyBearerAudience>()) | ||
| { | ||
| var admission = await service.CheckLegacyBearerAdmissionAsync(audience); | ||
| Assert.False(admission.IsAllowed); | ||
| Assert.Equal( | ||
| InstallationIdentityCutoverOrchestrator.LegacyAuthorityRetiredRefusal, | ||
| admission.RefusalCode); | ||
| } | ||
|
|
||
| // The v1 mutation path reports retirement too. It previously reported "migration in | ||
| // progress" after the flip, because the retirement ternary existed at only one of the two | ||
| // call sites -- the migration had finished and the authority was gone. | ||
| var mutation = await service.CheckV1MutationAdmissionAsync(); | ||
| Assert.False(mutation.IsAllowed); | ||
| Assert.Equal( | ||
| InstallationIdentityCutoverOrchestrator.LegacyAuthorityRetiredRefusal, | ||
| mutation.RefusalCode); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
issue [blocking]: Make retirement-policy coverage distinguish policy data from defaults.
This test uses V2InstallationAuthorityVersionPolicy. Its retired-audience set contains every legacy audience, and its refusal code is LegacyAuthorityRetiredRefusal. An orchestrator that hardcodes those values would still pass. Register a custom V2 policy with a distinct refusal code and non-default retired-audience set, then assert that both admission paths use those exact policy values.
As per path instructions, tests must cover retirement audiences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/local-node-host/tests/Identity/InstallationIdentityCutoverOrchestratorTests.cs`
around lines 866 - 893, Update
RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicy to register a custom
V2InstallationAuthorityVersionPolicy with a distinct refusal code and a
non-default retired-audience set. Assert legacy bearer admission is refused only
for the configured retirement audiences and returns the custom code, while
non-retired audiences preserve their expected admission behavior; also assert
CheckV1MutationAdmissionAsync returns the same policy-provided refusal code
after the flip.
Source: Path instructions
The deep review found the admission half of the pipeline inert by construction. The stage half required the incumbent stage, and the version was derived from that same stage, so the policy consulted was always the incumbent -- whose admission check returns true unconditionally. The successor's refusal could never be reached, while its documentation said it enforced the post-cutover retirement. My own mutation evidence was worthless and I reported it as proof. Flipping the successor policy to admit turned exactly one test red, and that test called the policy object directly. The integration test that walks the real orchestrator past the flip stayed green. A mutation that reddens only an assertion on the thing you just mutated demonstrates the method exists, not that anything consults it. Every version-specific condition now lives in the policy that owns it. The incumbent owns its stage and the write barrier; the successor refuses always. The shared half keeps only authority readability, which is version-independent. Dropping the stage equality alone would not have been enough -- the barrier is non-zero once the successor is authoritative, so it would have kept pre-empting the successor and the guard would have stayed inert in a second way. The same mutation now reddens the integration test. Three honesty repairs alongside it. Stage-to-version mapping is exhaustive and throws on an unmapped stage rather than defaulting to the permissive version, matching the registry that already refuses an unknown version. The incumbent no longer declares a retirement code and a retired-audience set it would never issue. And the branch that resolves a committed marker now says why it deliberately reads the data directly rather than through readiness -- routing it through readiness would make a committed installation unreadable on any restart without a registered sign-in path, which is the exact outcome this guard exists to prevent. Suite 1820 passed, 0 failed, 18 skipped, against a 1819 baseline; the difference is the unknown-stage test added here. Refs: shipyard#3615
|
Blocking finding addressed in 6c3bcb6, taking remedy (a) — made the guard decisive rather than deleting it. You were right that my mutation evidence proved nothing. M4 reddened one test, and that test called the policy object directly; the integration test stayed green. I counted the red and shipped it. What changed. Every version-specific condition moved into the policy that owns it: the incumbent owns Your trap warning was load-bearing — dropping the stage equality alone would not have worked, since M4 re-run, per your acceptance criterion: making Full suite: 1820 passed / 0 failed / 18 skipped, against your confirmed 1819 baseline. The +1 is the unknown-stage fail-closed test added here. I ran this myself rather than accepting the agent's report, since it could not complete a full run. Also fixed, from your suggestions:
Carded rather than expanded into this PR, per your follow-up list:
The PR description is written with the mutation table. Ready for re-review. |
ctwoodwa
added
the
deep-review-passed
github-actions
Bot
removed
the
needs-deep-review
|
Deep review APPROVED on re-review; blocker discharged. Applying The reviewer verified the acceptance criterion rather than accepting it, and the result is better than the criterion asked for. Mutating
Those eight include the orchestrator path ( The reviewer also confirmed by case analysis that behaviour is identical to the previous head for every reachable state — no admission widened or narrowed. The rule moved without the answer moving, which is the correct shape for this change. And they ran the mutation my remedy made necessary: removing the redundancy means the V1 policy's predicate is now the sole guard on the write barrier, so they mutated that to Baseline reproduced exactly: 1820 / 0 / 18, with the +1 identified as One hand-back recorded on #3629 rather than lost here. This PR's V1 change creates a trap for that card: replacing the |
1 participant
What this fixes
The v1→v2 installation-authority cutover had its version rules scattered across the orchestrator. This puts them behind a versioned policy pipeline: one durable cutover orchestrator, and a per-version policy that owns its own readiness, admission and retirement. Callers stay unaware of version internals, and adding a v3 means adding a policy rather than editing the orchestrator.
The safety property it buys: the cutover refuses to commit the marker until a v2 sign-in path is registered. Without that, the marker can flip into an installation whose listener rejects every v1 cookie audience while no v2 path exists — nobody signs in again, ever, and the operation is one-way.
Refs ADR 0160 R3-H. Closes #3615
Mutation evidence
All four reproduced exactly by the deep reviewer.
RetirementRefusalAfterTheFlipComesFromTheSuccessorPolicyThat last row is the one that matters, and it is the fix in 6c3bcb6. As originally written the admission half was inert by construction — the stage half required the incumbent stage, the version was derived from that same stage, so the policy consulted was always the incumbent, whose admission check returns true unconditionally. The successor's refusal was unreachable while its documentation claimed it enforced the post-cutover retirement.
The original mutation for that row reddened only a unit assertion on the policy object itself, which demonstrates the method exists, not that anything calls it. Every version-specific condition now lives in the policy that owns it, and the same mutation reddens the integration test that walks the real orchestrator past the flip.
Also verified: the registry refuses an unknown version and refuses two policies for one version;
DefaultCompositionIsFailClosedproves the convenience constructor is fail-closed; a correctly-composed host can still cut over (FlipSucceedsOnceASuccessorSignInPathIsRegistered); andCanonicalLegacyBearerAudiences()is byte-identical to what main returned, so the durable digest cannot have narrowed.Suite: 1820 passed / 0 failed / 18 skipped, against a 1819 baseline. MTW-00A inventory ratcheted 54→55; that gate regenerates and byte-compares, so it fails in both directions.
Deep review
Reviewed under the
Deep review requiredgate. Verdict BLOCK on the inert-admission finding above, now addressed with remedy (a). Three of the reviewer's non-blocking suggestions were folded in — exhaustive stage mapping that throws rather than defaulting permissive, honest V1 policy declarations, and a comment explaining why the committed-marker branch deliberately bypasses readiness (routing it through readiness would brick an installation on a restart without a registered sign-in path).Four further findings are carded rather than expanded here: #3627, #3628, #3629.
Known and pre-existing
Booting the fully composed host throws
ArgumentException: The current trust anchor must be a non-empty genesis admission. Verified identical on clean main. Not caused by this PR.