feat(policy)!: harden and finalize the policy contract - #109
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Public broker options still accept duplicates when directly deserializing several exposed nested DTO types.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds strict duplicate JSON-member rejection across policy and broker deserialization, with shared Rust/.NET fixtures and client error handling.
Changes:
- Adds duplicate-name scanning and serializer converters.
- Covers nested, escaped, and conflicting duplicates.
- Adds cross-language and broker-client tests.
File summaries
| File | Description |
|---|---|
policies/rust/now-policy/tests/policy_samples.rs |
Tests duplicate fixtures. |
policies/rust/now-policy/assets/samples/invalid/duplicates/duplicate-rule-id.policy.json |
Nested rule duplicate fixture. |
.../duplicate-policy-format-version-same.policy.json |
Equal-value duplicate fixture. |
.../duplicate-policy-format-version-escaped.policy.json |
Escaped-name duplicate fixture. |
.../duplicate-policy-format-version-conflicting.policy.json |
Conflicting duplicate fixture. |
.../duplicate-metadata-publisher.policy.json |
Metadata duplicate fixture. |
.../duplicate-metadata-id.policy.json |
Escaped metadata duplicate fixture. |
.../duplicate-match-field.policy.json |
Match duplicate fixture. |
.../duplicate-constraints-field.policy.json |
Constraints duplicate fixture. |
policies/dotnet/Devolutions.Now.Policy.Model/README.md |
Documents strict parsing. |
...Model/PolicySerializer.cs |
Integrates duplicate rejection. |
...Model/PolicyJsonInput.cs |
Implements scanner and converter. |
...Model.Tests/PolicyTests.cs |
Tests policy entry points. |
...Client.Tests/PolicyManagementClientTests.cs |
Tests management and error responses. |
...Client.Tests/BrokerClientTests.cs |
Tests client response rejection. |
...Api/README.md |
Documents broker behavior. |
...Api/BrokerSerializer.cs |
Integrates broker converters. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently covers documented entry points, nested objects, escaped names, caller options, AOT operation, and client error handling with comprehensive tests.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The invalid validation-response fixture still exposes removed match paths and boolean-array semantics.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 54/54 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Duplicate collection values have inconsistent validity and canonicalization across schemas, Rust, and .NET.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
policies/rust/now-policy/src/policy.rs:640
- This cardinality check runs after
Managershas been deserialized into aBTreeSet, so{"Managers":["Winget","Winget"],"SourceNames":["corp"]}collapses to one manager and is accepted by Rust. The generated schema rejects that array viauniqueItems, and .NET seesCount == 2and rejects it, so the new “exactly one manager” rule differs across implementations. Reject duplicate manager elements before set conversion (or otherwise align all three contracts).
policies/rust/now-policy/src/policy.rs:811 uniqueItems: truemakes duplicate exact identifiers invalid under the schema, but both typed readers accept them: Rust checks theVeclength and then silently deduplicates it into aBTreeSet(lines 748-752), while .NET validates count/characters without checking uniqueness (PolicySerializer.cs:191-197) and preserves duplicates. This gives the same document different validity and canonical output across schema, Rust, and .NET. Either reject duplicate elements in both readers or remove this constraint and define one canonicalization behavior.
This issue also appears in the following locations of the same file:
- line 825
- line 913
policies/rust/now-policy/src/policy.rs:825
- The schema rejects duplicate
Patterns, but Rust accepts and deduplicates them when collecting the inputVecinto aBTreeSet(lines 753-756), while .NET accepts and retains them (PolicySerializer.cs:199-205). Align runtime validation and canonicalization withuniqueItems, or remove the schema restriction consistently.
policies/rust/now-policy/src/policy.rs:913 Version.Exacthas the same contract mismatch: the schema requires unique values, Rust silently deduplicates duplicate input at lines 856-860, and .NET accepts and preserves duplicates (PolicySerializer.cs:236-243). This prevents consistent validation and canonical output; make both readers reject duplicates or removeuniqueItemsand standardize canonicalization.
- Files reviewed: 54/54 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Version ranges still accept empty or non-SemVer boundaries across both model implementations and generated schemas.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 54/54 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Public broker serializer options currently bypass standalone VersionRange semantic validation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 54/54 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Rust, .NET, and generated schemas disagree on accepted semantic-version boundaries, leaving a cross-platform contract inconsistency.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
policies/rust/now-policy/src/newtypes.rs:45
- This schema regex accepts numeric components larger than
u64(for example1.18446744073709551616.0), whileSemanticVersion::parsedelegates tosemver::Version::parse, whose major/minor/patch fields areu64, so Rust rejects the same value. The new .NET regex also accepts it. This leaves the generated schemas and .NET contract accepting a version that Rust rejects; align the numeric bounds or use the same SemVer validation semantics across implementations.
policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:385 ExecutionElevationnow represents effective execution privilege, but its referencedElevationenum is still documented as “Requested elevation level” in bothEnums.cs:85andpolicies/rust/now-policy/src/enums.rs:64; that stale text is also emitted forPolicyModelElevationin the generated schemas. Update or rename the model enum so generated API documentation does not describe this effective value as a client request.
- Files reviewed: 54/54 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Rust condition serializers can emit schema-invalid pattern and version strings constructed through public newtype fields.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
policies/rust/now-policy/src/policy.rs:829
StringPatternexposes its innerString, so callers can construct an empty or oversized pattern withoutparse. This branch checks only the collection size and then serializes those invalid elements, producing JSON rejected by the generated schema and by deserialization. Revalidate every pattern on output, as the other new condition serializers do for their mutable invariants.
policies/rust/now-policy/src/policy.rs:927VersionStringhas a public innerString, so a programmatically constructedVersionCondition::Exactcan contain empty or over-128-scalar values. This serializer validates only the number of entries and emits those invalid strings, despite the new input/output invariant and schema bounds. Revalidate each exact version before serializing.
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Rust and .NET accept more than 128 source names despite generated schemas rejecting them.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs:173
- The generated policy schemas cap
SourceNamesat 128 items, but model validation only checks duplicates and each string's length. Thus .NET accepts and serializes 129 unique source names that schema-based consumers reject. Add the collection-count check here to keep runtime and schema validation aligned.
policies/rust/now-policy/src/policy.rs:669 SourceNamesis declared withmaxItems: 128, but this deserializer only checks duplicates and per-name validity. A policy containing 129 unique source names is therefore accepted by Rust while the committed schemas reject it. Enforce the collection bound before converting the vector to a set.
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Runtime manager-list validation remains inconsistent with the 16-item limit published by the generated schemas.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs:170
ManagerNamehas 17 variants (Enums.cs:46-64), but the generated schemas capManagersat 16 while this validation only rejects duplicates. A policy containing all 17 distinct managers is therefore accepted and serialized by the runtime but rejected by schema clients. Align the contract by either enforcing 16 in both runtimes or raising every generatedmaxItemsto 17, and add a parity test.
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Mutable package and version mode objects retain stale presence flags, preventing valid in-place mode changes.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:459
ExactSpecifiedandRangeSpecifiedare permanent latches, so a consumer cannot switch this public mutable condition fromExacttoRange: after settingExact = nulland assigningRange, both flags remain true and every serializer/conversion rejects the object even though null properties are omitted from its JSON. Track property presence only while deserializing (for example in a converter), or provide an atomic mode-switch API that clears the previous value/state while still rejecting documents that contain both properties.
This issue also appears on line 492 of the same file.
policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:496
- These presence flags never reset, so changing a valid public condition from
ExacttoPatternsby clearingExactand assigningPatternsleaves both flags true. Serialization and draft conversion then reject an object whose canonical JSON would contain onlyPatterns. Separate wire-property presence tracking from mutable model state, or expose an atomic mode-switch API that clears the stale mode without accepting both properties from JSON.
set
{
ExactSpecified = true;
_exact = value;
}
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Also add atomic .NET condition mode switching and preserve structured validation paths across broker response nesting. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The coordinated breaking contract changes span security-sensitive deserialization, two runtimes, generated schemas, and numerous consumer-facing APIs.
Review details
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Remove the redundant strict/non-strict model contexts and entry points now that every policy input uses the closed contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The coordinated breaking wire-contract, serializer, and schema changes across Rust and .NET warrant final human integration review.
Review details
- Files reviewed: 55/55 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
PolicySerializer.Options,Deserialize<T>, and the documentParseJsonhelpers; remove redundant strict/non-strict model contexts and entry pointsnull/omitted = unrestricted, canonical output omitted)ValidFrommust be strictly earlier thanValidUntilby normalized instantConstraintsvalid only on Allow rulesPolicyType,RulePrecedence, andPackageNames; precedence is fixed as lower priority first, Deny wins Allow/Deny ties, then document orderElevationtoExecutionElevationandSourcesto exactSourceNames; runtime collection bounds match the published schemas, and nonempty source names require exactly one managerExactorPatternsmodes and versions explicitExactor semanticRangemodes; provide atomic .NET mode-switch APIsnullable)PolicyFormatVersionremains in the compatible 1.x line. This is an intentional pre-release package/API break with no legacy aliases or conversion.Canonical semantics
null: does not narrow;false/true: exact request characteristic[]: does not narrow; canonical output omits empty collections; duplicate values are invalidnull: that side is unbounded; canonical output omits it; equal or inverted two-sided windows are invalidValidFromor afterValidUntilon every request, with no fallback policyPackageIdentifiers.Exact: validated stable identifiers; wildcard characters rejectedPackageIdentifiers.Patterns: explicit wildcard patterns that may authorize multiple identifiersVersion.Exact: one or more arbitrary real package version strings;Version.Range: semantic versions onlySourceNames: exact configured source names, no URLs/pattern matching, exactly one selected manager, at most 128 namesManagers: at most 16 distinct manager valuesExecutionElevation:Elevatedwhen scope is Machine or requested elevation is Elevated;StandardotherwiseConsumer migration
Gateway
PolicyType,RulePrecedence,PackageNames,Sources,Elevation,Versions, andVersionRangePolicySerializer.Deserialize<T>or the documentParseJsonhelpersExecutionElevationexactly from Machine scope or requested elevationSourceNames, package identifier modes, and version modesValidFrom/ValidUntilboundary tests if missingUniGetUI
PolicySerializer.Deserialize<T>or the documentParseJsonhelpersValidFrom < ValidUntilvalidationValidation
now-policy36/36,now-policy-api18/18, server 7/7, server samples 28/28-D warnings, dotnet/cargo formatting, deterministic schema/OpenAPI regenerationRelease impact
Required coordinated Rust releases:
now-policy0.5.0,now-policy-api0.6.0 (updating itsnow-policydependency), andnow-policy-server-template0.6.0. Publish the Model/API/Client NuGets together using the next date-based workflow version (validated with 2026.09.17.0, normalized to 2026.9.17). No release, package publication, merge, or auto-merge is included.